Skip to content

fix(repl): normalizer PREV_BINOP class for binop-at-EOL continuations (#620 burndown)#643

Merged
jsavin merged 2 commits into
developfrom
fix/repl-prev-binop-normalizer
May 25, 2026
Merged

fix(repl): normalizer PREV_BINOP class for binop-at-EOL continuations (#620 burndown)#643
jsavin merged 2 commits into
developfrom
fix/repl-prev-binop-normalizer

Conversation

@jsavin

@jsavin jsavin commented May 25, 2026

Copy link
Copy Markdown
Owner

Summary

5th hole in normalize_newlines_to_semicolons (after #628/#635/#640): a UserTalk script ending a line with a binary-operator token (and, or, ==, >=, <=, !=, >, <, +, -, *, /, ,, () had a synthetic ; inserted before the continuation, producing source like x and; y that the parser rejected. The 5 failing sys.* integration tests using (typeof(...) == stringType) and\n (typeof(...) == stringType) patterns were the visible symptom; the same hole catches any multi-line expression with a trailing binop.

Add a PREV_BINOP row to the transition table that suppresses ; for every next-kind — a continuation is a continuation regardless of what follows.

Implementation

  • New PREV_BINOP member in prev_kind_t. The transition table grows one row; all five cells are suppress.
  • New prev_is_binop(out, dst) helper walks back through the output buffer to detect the trailing operator token. Single-char operators (+ - * / , ( > <) match on the last byte. Multi-char operators require disambiguation:
    • = is BINOP only if the byte before is =, >, <, or ! (so == / >= / <= / != match but bare assignment = does not).
    • and / or require a word boundary before the keyword (so identifiers like band or door are not misclassified).
  • classify_prev_full(prev_nonws, out, dst) is the buffer-aware wrapper used by the normalizer loop. Falls back to single-byte classify_prev for anchor bytes (\0 ; { }), then upgrades PREV_OTHER to PREV_BINOP when prev_is_binop matches.
  • Cells changed in kEmitSep: new BINOP row, all-zero. Existing rows untouched.

Judgement calls

  • Unary - is over-suppressed. x = -\n 1 (unary minus split across lines) is statically indistinguishable from binary - without a real tokenizer. We treat all trailing - as BINOP. The worst case is a script that was already malformed losing one parse-error surfacing, which is preferable to silently mis-normalizing the common x +\n y and x -\n y patterns.
  • ) is NOT BINOP. Close paren is end-of-expression, not continuation. Same for } and ; (handled by their own prev-kind classes).
  • No appisrunning work. The bundled task described a missing headless sys.appisrunning implementation, but tests/headless_sys_verbs.c already implements it via pgrep -x (committed earlier, marked COMPLETE). Manual testing confirms sys.appisrunning("frontier-cli") returns true when called from inside frontier-cli. The 5 failing tests are 100 percent normalizer-caused (all use and\n continuations). Change B from the task was unnecessary.

Tests

8 new tests in protocol_eval_compile_errors.yaml:

  • 6 positive-path BINOP continuations (boolean and / or, relational ==, arithmetic +, comma, open paren)
  • 1 word-boundary regression guard (local (band = 7)\n return band — identifier ending in and must still get ;)
  • 1 disambiguation regression guard (x =\n 5 — single = is assignment, must still get ;; result is a parse error, locked in as success: false)

Test plan

  • make -C frontier-cli -j$(sysctl -n hw.ncpu) — clean build, no new warnings
  • ./tools/run_headless_tests.sh — 489/489 pass
  • cd tests && make test-integration — 70 to 65 failures (-5 sys cluster), +8 new tests all green. Net: 2032 to 2045 passing, 2309 to 2317 total
  • All 39 pre-existing tests in protocol_eval_compile_errors.yaml still pass unchanged
  • Manual smoke: sys.appisrunning("frontier-cli") returns true via --protocol script/eval — appisrunning was already working

Per-cluster count change

  • sys verbs - system information suite — fixed (used and\n)
  • sys verbs - all info verbs return strings — fixed (used and\n)
  • sys verbs - numeric verbs return long — fixed (used and\n)
  • sys verbs - boolean verbs return bool — fixed (used and\n)
  • sys environment verbs - isolation test — fixed (used and\n)

Generated with Claude Code (https://claude.com/claude-code)

…ons (#620 burndown)

5th hole in normalize_newlines_to_semicolons after #628/#635/#640. When a
UserTalk script ends a line with a binary-operator token ('and', 'or', '==',
'>=', '<=', '!=', '>', '<', '+', '-', '*', '/', ',', '('), the next line is
a continuation of the same expression. Previously the (PREV_OTHER, NEXT_OTHER)
cell emitted ';' between them, producing source like 'x and; y' that the
parser rejected. The 5 failing sys.* integration tests with multi-line
expressions (typeof(sys.os()) == stringType) and ...) were the visible symptom.

Add PREV_BINOP class to the transition table. The classifier (prev_is_binop)
walks back through the output buffer to detect multi-byte operator tokens
('and' / 'or' word-bounded, '==' / '>=' / '<=' / '!=' via byte-before check)
so single '=' (assignment) is correctly distinguished from '=='. New BINOP
row suppresses ';' for every next-kind.

Adds 8 integration tests in protocol_eval_compile_errors.yaml covering the
new transition cell and the disambiguation paths (single '=' assignment,
identifier ending in 'and' / 'or').

Integration: -5 failures (sys cluster) + 8 new tests, all green. Unit: 489/489.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bar-raiser flagged P1 test gaps:
- Docblock claims BINOP coverage for >=, <=, !=, >, <, *, /, - but tests
  only exercised and, or, ==, +, ',', '('.
- 'and' had a band/kand word-boundary regression but 'or' did not.

Add 4 new tests:
- '>=' pins the '>' arm of the byte-before-'=' disambiguation
- '!=' pins the '!' arm
- '*' pins the BINOP single-char arithmetic byte-class
- 'door' as variable pins the 'or' word-boundary check (symmetric to 'band')

Integration: 2321 total / 2050 passed / 64 failed / 207 skipped — +4 total,
+5 passed (the 4 new tests + a flake recovery), -1 failed.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jsavin

jsavin commented May 25, 2026

Copy link
Copy Markdown
Owner Author

Local /gate round 2 (post-P1 fixes)

Verdict: PASS — both P1 findings addressed.

Round 1 → Round 2

Round 1 (35290de40) findings:

Tests at HEAD fa2a8ebeb

  • Unit: 489/489
  • Integration: 2321 / 2050 passed / 64 failed / 207 skipped (+4 total tests, all 4 new BINOP regression tests green)
  • protocol_eval_compile_errors.yaml: target file 47/47 pass

Why not a full multi-reviewer re-gate

Round-2 diff is tests-only (4 new YAML entries, +48 lines). No code changes to repl_variables.c. Re-running bar-raiser/security/concurrency would consume reviewer time for zero new signal on the C diff. Round-1 review of the underlying code stands.

Ready to merge.

@jsavin jsavin merged commit e21f14d into develop May 25, 2026
@jsavin jsavin deleted the fix/repl-prev-binop-normalizer branch May 25, 2026 09:39
@jsavin

jsavin commented May 25, 2026

Copy link
Copy Markdown
Owner Author

/auto Summary

What went well:

What could improve:

  • Anthropic API hit a 529 storm during the gate dispatch (4 reviewers all 529'd in their first attempt, then succeeded on retry). Not Frontier-specific. If we hit this again during /auto chains, the right move is to wait a few minutes and retry rather than dispatching new parallel agents.
  • Edit/Write tools silently no-op'd on frontier-cli/repl_variables.c for the PR agent. They worked around via Python read/replace/write. Worth filing if reproducible — this is the second time the same agent worktree showed file-edit drift (also seen in the security review of fix(repl): normalizer PREV_BINOP class for binop-at-EOL continuations (#620 burndown) #643 round 1).

Recommendations:

  • The "for / while / if at EOL" cell is the next normalizer hole. Not regressed by this PR (pre-existing). File a follow-up if anyone hits it; otherwise it can sit until a real script trips it.

🤖 Generated with Claude Code /auto

jsavin added a commit that referenced this pull request May 25, 2026
…for ODB editing (closes #644) (#647)

* feat(tooling): edit_virgin_root.sh stage-and-confirm wrapper (closes #644)

Interpose a staging step between the operator and databases/Virgin.root
for protocol-mode ODB edits. The wrapper copies Virgin.root to
/tmp/frontier-edit-<hash>/, spawns frontier-cli --protocol against the
staged copy, and prompts before promoting changes back to the canonical
file. A killed session, runaway script, or leaked --allow-mutate process
can no longer corrupt the source — worst case the staged copy is
discarded.

Also hardens tests/debug_protocol_test.sh to stage Virgin.root into a
tmpdir before launching protocol mode (latent risk if the #588 read-only
default ever regresses), and updates docs/ODB_SCRIPT_EDITING.md to point
operators at the wrapper as the recommended workflow with the raw
--allow-mutate invocation retained as an emergency/experts-only fallback.

Background: PR #643 saw databases/Virgin.root grow from 20MB to 31MB
during an autonomous agent run, traced to a leaked --allow-mutate
session in a sibling worktree. The documented workflow points
--allow-mutate directly at the canonical file by design — this wrapper
addresses that gap.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(tooling): address /gate review for #647 — mktemp, EXIT trap, retab, atomic promote, race-check

Round-2 review fixes for edit_virgin_root.sh:

1. (P1 security) Use `mktemp -d -t frontier-edit-<md5>-XXXXXXXX` instead of
   a predictable `/tmp/frontier-edit-<md5>` path. Closes symlink-hijack on
   /tmp. Adds belt-and-suspenders check that staged path is a regular file
   (not a symlink). Help text updated to describe "$TMPDIR" not /tmp.

2. (P1 bar-raiser) Install an EXIT trap to clean up the stage dir. Sets
   STAGE_PRESERVE=1 on the two forensic branches (rejected promotion and
   --read-only-changed) so the trap leaves the stage dir for inspection.
   Drops the scattered explicit `rm -rf "$STAGE_DIR"` calls.

3. (P1 bar-raiser) Retab tools/edit_virgin_root.sh and
   tests/integration/edit_virgin_root_wrapper_test.sh from tabs to 4-space
   indent. Every other *.sh in the repo uses 4-space; CLAUDE.md's tab rule
   is C-only.

4. (P2 promoted) Atomic promotion: copy staged copy to .promoting.$$ tmp
   in the canonical's directory, sync, then mv into place. A Ctrl-C
   mid-copy leaves the old Virgin.root intact; only the rename swaps.
   Trap also sweeps any orphan .promoting.$$ files in the canonical dir.

5. (P2 promoted) Stale-process race check: before promotion, recompute md5
   of canonical and refuse with exit 5 (preserving staged copy) if it
   changed externally during the session. Addresses the wrapper's whole
   reason for existing — another process holding the file writable.

Bash test updated to look for stage dirs under $TMPDIR (macOS default
/var/folders/.../T/) and /tmp, since mktemp -d -t honors $TMPDIR. All
5/5 wrapper tests pass.

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jsavin added a commit that referenced this pull request May 25, 2026
…648)

* fix(repl): recognize symbolic && / || in prev_is_binop (#643 burndown)

The newline normalizer's PREV_BINOP classifier covered only the keyword
forms 'and' / 'or'. A line ending in '&&' or '||' before a bare newline
was misclassified as PREV_OTHER, causing a synthetic ';' to be inserted
('&&;\n') and the parser to reject the script.

Add two-byte lookbehind for '&&' and '||' in prev_is_binop, mirroring
the existing '=' multi-byte arm. Single '&' / '|' return 0 — neither is
a UserTalk operator, so they correctly fall through to the default
';'-emission path.

Motivating failure: db_migration's "Database - system table accessible
after hydration" test, whose body is a multi-line 'sizeOf(x) > 0 &&\n...'
chain. Replacing '&&' with 'and' makes that test pass without any code
change — the falsifiable check that this was the root cause.

Tests: 4 new cases in protocol_eval_compile_errors.yaml exercise '&&'
EOL, '||' EOL, '&&' followed by parens, and mixed symbolic/keyword
operators across continuation lines.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(repl): complete PREV_BINOP coverage for all UserTalk infix operators

Builds on d092576 (which added `&&` and `||`). This commit:
- Adds `%` (modulo) to the symbolic single-char BINOP row.
- Refactors keyword-operator detection into a `prev_matches_keyword` helper
  with explicit word-boundary check. Replaces the ad-hoc `case 'd'` /
  `case 'r'` arms for `and` / `or`.
- Adds keyword infix operators: `equals`, `notEquals`, `lessThan`,
  `greaterThan`, `contains`, `beginsWith`, `endsWith`.

`lessThanOrEqual` / `greaterThanOrEqual` are documented in
docs/usertalk/operators_and_idioms.md but the UserTalk parser does NOT
actually accept them as infix operators (verified empirically:
`1 lessThanOrEqual 1` compiles and returns `1`, not `true`). They are
intentionally omitted; documented inline alongside the code.

Tests: 11 new positive cases in protocol_eval_compile_errors.yaml
covering each new operator across a newline, one word-boundary
regression guard for `contains`, plus negative cases asserting bare
`&` and bare `|` at EOL are not promoted to BINOP.

Bar-raiser P2-1 + P2-2 closure on PR #126.

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant