diff --git a/apps/release_authority/release_authority_test.go b/apps/release_authority/release_authority_test.go index 0612f2b9..8a544ce6 100644 --- a/apps/release_authority/release_authority_test.go +++ b/apps/release_authority/release_authority_test.go @@ -85,12 +85,18 @@ func TestReleaseAuthority(t *testing.T) { t.Fatalf("extra output after decision: %v", err) } var proposal struct { - Target string `json:"target"` + Target *string `json:"target"` } if err := json.Unmarshal([]byte(proposalJSON), &proposal); err != nil { t.Fatal(err) } - if decision["target"] != proposal.Target { + // A missing target is nil from map_get, so the app's is_nil + // fallback reports "unknown" (2026-10-12 nil semantics). + wantTarget := "unknown" + if proposal.Target != nil { + wantTarget = *proposal.Target + } + if decision["target"] != wantTarget { t.Fatalf("target did not round-trip: %q", decision["target"]) } tokens := json.NewDecoder(strings.NewReader(outStr)) diff --git a/apps/task_api/task_api.howl b/apps/task_api/task_api.howl index 341de2e5..08d33d2c 100644 --- a/apps/task_api/task_api.howl +++ b/apps/task_api/task_api.howl @@ -26,7 +26,7 @@ (res_json 400 (dict ("error" "invalid_json"))) ) (let (title (map_get body "title")) - (if (= title "") + (if (or (is_nil title) (= title "")) (res_json 400 (dict ("error" "title_required"))) (let (id (call next_task_id)) (let (task (dict ("id" id) ("title" title) ("status" "open"))) @@ -84,7 +84,7 @@ (res_json 400 (dict ("error" "invalid_json"))) ) (let (id (map_get body "id")) - (if (= id "") + (if (or (is_nil id) (= id "")) (res_json 400 (dict ("error" "id_required"))) (do (store_open kv "memory://session") @@ -106,7 +106,7 @@ (res_json 400 (dict ("error" "invalid_json"))) ) (let (id (map_get body "id")) - (if (= id "") + (if (or (is_nil id) (= id "")) (res_json 400 (dict ("error" "id_required"))) (do (store_open kv "memory://session") @@ -135,7 +135,7 @@ (res_json 400 (dict ("error" "invalid_json"))) ) (let (id (map_get body "id")) - (if (= id "") + (if (or (is_nil id) (= id "")) (res_json 400 (dict ("error" "id_required"))) (do (store_open kv "memory://session") diff --git a/docs/journals/2026-10-12_heldout_correctness_fixes.md b/docs/journals/2026-10-12_heldout_correctness_fixes.md new file mode 100644 index 00000000..5cb4cd9e --- /dev/null +++ b/docs/journals/2026-10-12_heldout_correctness_fixes.md @@ -0,0 +1,125 @@ +# 2026-10-12: Held-out correctness fixes + +Base: main `c21a029c` (#136 merged). This slice fixes the four correctness +bugs the held-out benchmark (heldout-v0) surfaced. No new built-ins. No token +claims. No production `-compile-bc`/HFIR flip. + +## 1. Nested shorthand under the governed profile + +**Bug:** shorthand (`str_replace`, `read_text`, `str`, ...) nested inside the +argument of a value-taking built-in (`write_file`, `to_lower`, ...) was not +expanded. The governed build then failed with "no lowering for construct". + +**Fix:** `internal/ast/desugar.go` `isSugarExprContext` now lists every +value-taking built-in as a parent. These include write_file, read_file, +to_lower/to_upper, str_trim, the escapes, regex_match, is_nil, map_keys, env, +exec, store_*, the req_*/res_* readers, llm_generate and others. A new drift +test (`desugar_context_drift_test.go`) walks the construct registry. Every +supported, non-opaque construct must expand nested sugar unless it is listed as +structural, with a written reason. + +**Fail-closed is unchanged:** the order is still pre-desugar profile scan, +then desugar, then post-desugar `hfir.VerifyProfile`, then build. A forbidden +construct inside a nested shorthand argument is rejected before any artifact +exists, and nothing runs. `TestHeldoutNestedShorthandGovernedFailsClosed` +covers this: an `exec` hidden in `str_replace` inside `write_file` yields no +artifact and no side file. + +## 2. Short-circuit `and` / `or` + +**Bug:** both operands were always evaluated. + +**Fix:** +- **AST interpreter:** `evalBinop` decides before it evaluates the right side. +- **AST bytecode:** `bytecode.ShortCircuit` emits a JUMP_IF_FALSE guard around + the right operand. The HFIR bytecode lowering uses the same helper. +- **Go/JS backends:** they already used native `&&`/`||`. + +The right operand of `and` runs only when the left is true, and the right of +`or` only when the left is false. Effects (`write_file`) and errors +(`to_int "qq"`) on the skipped side do not happen. + +## 3. `map_get` on a missing key returns nil (behavior change) + +**Before:** a miss returned `""`, so `is_nil` and `nz` defaults never fired on a +miss. + +**Now:** a miss returns nil on every path: +- **VM paths:** interpreter, bytecode and HFIR bytecode. +- **JS:** returns `null`. +- **Go:** returns nil for dynamic maps. + +Text rendering is kept stable. `vm.TextOf` renders nil as `""` in `print`, +`stderr`, `to_string` and `str_join`. The Go/JS backends render nil/null as `""` +in `print` and `to_string`. So `(print (map_get m "zz"))` still prints an empty +field. `is_nil`, `nz` and `=` now tell a miss (`nil`) apart from a present `""`: +`(= (map_get m "zz") "")` is now **false**. + +Other consequences: +- A chained `map_get` through a missing intermediate is still `TYPE_ERROR`, + now "expected dict, got nil". +- A missing `store_get` record now prints as an empty field instead of ``. + `is_nil` is the way to observe absence. + +**Audit and migration:** +- `apps/task_api`: the title/id validation compared with `""`. It now uses + `(or (is_nil x) (= x ""))`, so it still rejects both a missing and an empty + value. Short-circuit makes this safe. +- `apps/release_authority`: it already had `is_nil` fallbacks to `"unknown"`, + which were dead code before. They now fire. The test expects `"unknown"` when + a proposal omits `target`. +- The vm/gogen/js absence and path tests were renamed `...MissIsNil` and their + expectations updated. +- The lowered-HFIR ABI conformance fixture `map_get_absence` now expects + `is_nil` of a miss to be `true`. +- A grep of all `.howl` for `(= (map_get …) "")` found nothing else. + +**Go typed-map caveat:** a Go `map[string]string` cannot hold nil, so a value +read of a miss is still `""`. `is_nil (map_get m k)` on a named map is compiled +as a key-presence check (`howlFrameMapGetIsNil`), so it agrees with the VM. +`nz (map_get m k) d` on a typed string map in the Go target may still see `""`. + +## 4. Lists are values (decision) + +**Decision:** explicit value semantics, applied the same way on every target. +- `append` rebinds only the named variable to a new list. +- A list stored in a map, or bound to another name, is unchanged. +- To update the map, `map_set` the list back. +- Dicts keep their in-place `map_set` mutation, which is unchanged. + +The VM paths already copied on append. The bug was that the backends did not: +- **JS:** used `push`, which aliased. `howlFrameAppend` now returns a copy and + the emitted code is `xs = howlFrameAppend(xs, item)`. +- **Go:** `append` could write into a shared backing array. It now uses a full + slice expression, `append(xs[:len(xs):len(xs)], item)`, in both the direct + form and the helper. + +## Regression tests + +- `heldout_correctness_cli_test.go`: each bug runs on the AST interpreter + (`-run`), governed AST bytecode (`build --profile governed` then `-run-bc`) + and HFIR bytecode (`-compile-hfir-bc` then `-run-bc`). It includes the + effect-skipping short-circuit test with `write_file` on the skipped side. +- `internal/backend/gogen/heldout_semantics_test.go` and + `internal/backend/javascript/heldout_semantics_test.go` cover list aliasing, + nil rendering, `is_nil` of a miss and short-circuit. Go dict literals are + string-valued, so the list-in-map case is covered on the VM and JS paths. +- `desugar_context_drift_test.go` keeps the shorthand parent list complete. + +## Benchmarks (verification only; no claims) + +- **token-efficiency-v0 `run.sh`:** howl 60/60, python 60/60, js 60/60, go 60/60. +- **token-efficiency-heldout-v0 `run.sh`:** exit 0. All 126 references verified + (howl 30, plus 32 each for python, js and go), with the same 2 recorded gaps + (h31, h32). The committed results files are unchanged. +- **h05/h07/h12 natural forms:** rewritten in a scratch run with nested + `str_replace (read_text …)`, `or` with `to_int` on the right, and + `nz (map_get …) 0`. All three pass the verifier. They were **not** committed, + so the frozen solution hashes and the #136 report stay as they are. + +## Known gaps + +- The JS backend's `nz` lowering emits `let` in expression position, which is + a syntax error. This bug predates this slice and is untouched here. +- Go dict literals cannot hold lists or ints (string-valued only). This also + predates this slice. diff --git a/docs/reference/lowered_hfir_abi_v1.md b/docs/reference/lowered_hfir_abi_v1.md index 32adef14..5bdb084c 100644 --- a/docs/reference/lowered_hfir_abi_v1.md +++ b/docs/reference/lowered_hfir_abi_v1.md @@ -53,7 +53,7 @@ These grant nothing. The suite runs them with an empty capability grant. * `let` and `print`. `print` writes one line. Several arguments are separated by a single space. * `dict`, `map_get`, `map_keys`, `list`, `list_len`, `str_join`, `is_nil`, `html_escape`, `attr_escape`, `regex_match`. -`(map_get dict key)` on a missing key is `""`. `is_nil` of that result is false. A present value is returned as stored. This is the #103 absence rule. `map_get` grants nothing. +`(map_get dict key)` on a missing key is nil. `is_nil` of that result is true. Nil renders as `""` in `print` and `to_string`, so a miss still prints as an empty field. A present value is returned as stored. Go caveat: on a typed `map[string]string` a miss still reads as `""` (that map cannot hold nil); `is_nil` of a named `map_get` is compiled as a key-presence check so it still agrees with the VM. This supersedes the #103 empty-string miss rule. `map_get` grants nothing. `(map_keys dict)` returns a list of strings in UTF-8 byte order (Go `sort.Strings`, Go string `<`). An empty dict is an empty list, so its length is `0`. A value that is not a dict fails at runtime with `TYPE_ERROR` and the text `map_keys expected dict`. `map_keys` grants nothing. The order, including keys outside the Basic Multilingual Plane, is the #109 rule. The suite replays `tests/fixtures/map_keys_sort_bmp.howl` and `tests/fixtures/map_keys_sort_nonbmp.howl`. Wasm does not emit `map_keys`; #73 must use this byte order if it ever does. diff --git a/heldout_correctness_cli_test.go b/heldout_correctness_cli_test.go new file mode 100644 index 00000000..bdeb9d28 --- /dev/null +++ b/heldout_correctness_cli_test.go @@ -0,0 +1,163 @@ +package main + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// Regression tests for the correctness bugs found by the held-out benchmark +// (docs/journals/2026-10-12_heldout_correctness_fixes.md). Each program runs +// on every applicable execution path: the AST interpreter, AST bytecode built +// under the governed profile (-run-bc), and the experimental HFIR bytecode. + +type heldoutPath struct { + name string + run func(t *testing.T, bin, dir, src string) (string, error) +} + +func heldoutPaths() []heldoutPath { + caps := func(dir string) string { + return "filesystem:read=" + dir + ",filesystem:write=" + dir + } + runBC := func(bin, dir, art string) (string, error) { + out, err := exec.Command(bin, "-run-bc", "-allow-caps", caps(dir), art).CombinedOutput() + return string(out), err + } + return []heldoutPath{ + {"interpreter", func(t *testing.T, bin, dir, src string) (string, error) { + out, err := exec.Command(bin, "-run", "-allow-caps", caps(dir), src).CombinedOutput() + return string(out), err + }}, + {"bytecode_governed", func(t *testing.T, bin, dir, src string) (string, error) { + art := filepath.Join(dir, "g.hfbc") + if out, err := exec.Command(bin, "build", "--profile", "governed", src, "-o", art).CombinedOutput(); err != nil { + t.Fatalf("governed build: %v %s", err, out) + } + return runBC(bin, dir, art) + }}, + {"hfir_bytecode", func(t *testing.T, bin, dir, src string) (string, error) { + art := filepath.Join(dir, "h.hfbc") + if out, err := exec.Command(bin, "-compile-hfir-bc", src, "-o", art).CombinedOutput(); err != nil { + t.Fatalf("hfir compile: %v %s", err, out) + } + return runBC(bin, dir, art) + }}, + } +} + +func runHeldoutProgram(t *testing.T, bin, program, want string, after func(t *testing.T, dir string)) { + t.Helper() + for _, p := range heldoutPaths() { + t.Run(p.name, func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "p.howl") + body := strings.ReplaceAll(program, "$DIR", dir) + if err := os.WriteFile(src, []byte(body), 0600); err != nil { + t.Fatal(err) + } + out, err := p.run(t, bin, dir, src) + if err != nil { + t.Fatalf("run: %v %s", err, out) + } + if out != want { + t.Fatalf("got %q want %q", out, want) + } + if after != nil { + after(t, dir) + } + }) + } +} + +// Bug 1: shorthand nested inside value-taking built-in arguments expands under +// the governed profile. +func TestHeldoutNestedShorthandExpands(t *testing.T) { + bin := c1c2Binary(t) + program := `(do + (let (t "a foo b") + (do + (write_file "$DIR/out.txt" (str_replace t "foo" "bar")) + (print (to_lower (str "AB" "C")) (read_text "$DIR/out.txt")))))` + runHeldoutProgram(t, bin, program, "abc a bar b\n", nil) +} + +// Bug 1, fail-closed half: a forbidden construct hidden in a nested shorthand +// argument is rejected by the governed build; no artifact, no effect. +func TestHeldoutNestedShorthandGovernedFailsClosed(t *testing.T) { + bin := c1c2Binary(t) + dir := t.TempDir() + side := filepath.Join(dir, "side") + src := filepath.Join(dir, "x.howl") + art := filepath.Join(dir, "x.hfbc") + program := `(do (write_file "` + side + `" "ran") (write_file "` + side + `2" (str_replace (exec "touch" "` + side + `.exec") "a" "b")))` + if err := os.WriteFile(src, []byte(program), 0600); err != nil { + t.Fatal(err) + } + if out, err := exec.Command(bin, "build", "--profile", "governed", src, "-o", art).CombinedOutput(); err == nil { + t.Fatalf("governed build accepted nested exec: %s", out) + } + for _, p := range []string{art, side, side + "2", side + ".exec"} { + if _, err := os.Stat(p); err == nil { + t.Fatalf("%s exists after a rejected governed build", p) + } + } +} + +// Bug 2: and/or short-circuit. The right side is not evaluated when the left +// decides, so its effects (write_file) and its errors (to_int "qq") never run. +func TestHeldoutShortCircuitSkipsRightSide(t *testing.T) { + bin := c1c2Binary(t) + program := `(do + (print (and (= 1 2) (do (write_file "$DIR/and.txt" "ran") (= 1 1))) + (or (= 1 1) (do (write_file "$DIR/or.txt" "ran") (= 1 1))) + (and (= 1 2) (= (to_int "qq") 1)) + (or (= 1 1) (= (to_int "zz") 1))) + (print (and (= 1 1) (do (write_file "$DIR/and_rhs.txt" "ran") (= 2 2))) + (or (= 1 2) (= 2 3))))` + runHeldoutProgram(t, bin, program, "false true false true\ntrue false\n", func(t *testing.T, dir string) { + for _, name := range []string{"and.txt", "or.txt"} { + if _, err := os.Stat(filepath.Join(dir, name)); err == nil { + t.Fatalf("short-circuited right side ran: %s exists", name) + } + } + if _, err := os.Stat(filepath.Join(dir, "and_rhs.txt")); err != nil { + t.Fatalf("undecided right side did not run: %v", err) + } + }) +} + +// Bug 3: map_get of a missing key is nil, so is_nil and nz defaults work. nil +// renders as "" in text. +func TestHeldoutMapGetMissIsNil(t *testing.T) { + bin := c1c2Binary(t) + program := `(do + (let (m (dict ("a" "x") ("e" ""))) + (do + (print (is_nil (map_get m "b")) (is_nil (map_get m "e")) (= (map_get m "b") "")) + (print (nz (map_get m "b") "dflt") (nz (map_get m "a") "dflt")) + (print (str "[" (map_get m "b") "]") (map_get m "b")))))` + runHeldoutProgram(t, bin, program, "true false false\ndflt x\n[] \n", nil) +} + +// Bug 4: lists are values. append rebinds only the named variable; a list held +// in a map (or another variable) is unchanged until it is map_set back. +func TestHeldoutListsAreValues(t *testing.T) { + bin := c1c2Binary(t) + program := `(cli_app + (let (m (dict ("xs" (list "a")))) + (let (xs (map_get m "xs")) + (let (ys xs) + (do + (append xs "b") + (print "map" (list_len (map_get m "xs"))) + (print "alias" (list_len ys)) + (print "var" (list_len xs)) + (map_set m "xs" xs) + (print "after" (list_len (map_get m "xs"))) + (print (str_join (map_get m "xs") ",")) + )))))` + runHeldoutProgram(t, bin, program, "map 1\nalias 1\nvar 2\nafter 2\na,b\n", nil) +} diff --git a/improvements.md b/improvements.md index e67540f7..4b3b6e0c 100644 --- a/improvements.md +++ b/improvements.md @@ -43,7 +43,7 @@ Pending rows are ranked by a diminishing-returns score: | 105 | [html_escape, attribute escape, and handler encode contract tests](#105-html_escape-attribute-escape-and-handler-encode-contract-tests) | Done (2026-09-29) | 1.75 (7×1÷4) | Sonnet 5 | Gemini 3.1 Pro (High) | gpt-5.6-sol | `html_escape` and `attr_escape` encode `& < > " '` on the VM, Go, and JS. No new capability. Handler contract tests keep ids out of `