fix(repl): preserve try/else and if/else across newlines in protocol mode by jsavin · Pull Request #635 · jsavin/Frontier

jsavin · 2026-05-23T22:22:04Z

Summary

Fix the }\nelse regression introduced by fix(repl): normalize bare CR/LF to semicolons in protocol script/eval (#624) #628's newline normalization: try/else and if/else across newlines were being split by an inserted ;, causing 24 integration tests to return 'true' (the with-wrapper's truthy default) instead of expected values.
Add } to the guard with lookahead: when the previous non-whitespace byte is } and the next non-whitespace token is else/;/}/EOF, don't insert a synthetic ;. Other followers (return, local, identifier, …) still get a ; because they're new statements.
Document the canonical UserTalk brace-placement convention in the primer and the usertalk-engineer agent (closing } and }; inlined at end of last statement of a block).

Why

#628 fixed local (x = 5)\rreturn x → 5 by normalizing bare \r/\n to ;. That worked for top-level multi-statement scripts but broke composite statements like try {...}\nelse {...} where the } shouldn't be followed by ; (it's not a statement terminator — the else clause attaches to the try). The fix is selective: only suppress ; when the next real token attaches to or closes the }.

Impact

24 integration tests fixed (146 failed → 122 failed), covering control_flow, try_error, error_handling_extended.
3 new regression tests added in protocol_eval_compile_errors.yaml:
- try/else across newlines, body succeeds (passes through)
- try/else across newlines, body throws (else catches)
- if/else across newlines

Test plan

Unit: ./tools/run_headless_tests.sh — required, passing
Integration: cd tests && make test-integration — required, 122 failed → was 146 failed pre-fix; the 24 newly-passing are all in the try/else/control-flow family
Pre-fix(repl): normalize bare CR/LF to semicolons in protocol script/eval (#624) #628 cases verified: bare \r separator, CRLF, semicolon-already-present, if-block all still pass
Documentation: primer + usertalk-engineer agent updated with canonical brace-placement convention

Refs: #628, #624.

…mode (#635) After #628 normalized bare \r/\n to semicolons in script/eval, multi-line try { ... }\nelse { ... } and if { ... }\nelse { ... } broke. The newline between '}' and 'else' got a ';' inserted, splitting the composite statement into a standalone try/if (executed silently) and an orphan 'else' keyword. YACC error recovery dropped the rest of the script and the with-wrapper returned its truthy default. Tests that depended on try/else semantics returned 'true' instead of the expected value. Fix: add a '}' guard with lookahead. When the previous non-whitespace byte is '}' and the next non-whitespace token is 'else' (or ';', '}', or EOF), don't insert a synthetic ';'. Other followers (return, local, identifier, ...) ARE new statements and still need an explicit ';' between them. Restores 21 previously-failing tests in control_flow.yaml, try_error.yaml, and error_handling_extended.yaml. Adds 3 new regression tests covering: - try/else with no error in body (passes through to return) - try/else with real error in body (caught by else) - if/else across newlines Also documents the canonical UserTalk brace placement style (closing '}' inlined at end of last statement of a block) in the primer and usertalk-engineer agent — discovered during the investigation. Integration count: 146 failed -> 122 failed (-24, of which 3 are new tests). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…lse, docs P1 (bar-raiser): a `//` line comment or `«...»` curly comment on its own line between `}` and `else` broke the lookahead — peek landed on `/` (or `«`) instead of `else`, so `;` got inserted and split try/else. Same bug class as the original #635 P0. Extend the peek loop to skip line comments and curly comments in addition to whitespace. Both are inter-token noise that hides the real next token. Also: - P2: update stale header comment (claimed "no '}' guard is needed"). - P2: renumber the duplicate `3.` list item in usertalk-engineer.md (the new brace rule plus the existing test-isolation rule were both `3.`). - P2: 3 new regression tests — comment between `}` and `else`, canonical inline `};\nelse`, nested try/else across newlines. - P2: comment explaining why `prev_nonws = ';'` after the suppressed `;` is intentional (the newline acts as the boundary signal even when no byte was emitted). Tests: integration 1956 passed / 122 failed / 207 skipped (was 1953/122/ 207 — three new tests added, all pass; no regressions). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

) Round-2 /gate follow-ups: - Add case/else-across-newlines integration test. case shares the grammar caseheader casebody elsetoken bracketedstatementlist with try/else and if/else, so the }-else lookahead applies the same way. Lock it in so a future refactor can't silently regress the third construct. - Reword the comment that incorrectly implied 'continue' itself skipped the \r/\n. It actually stops AT the terminator; the outer loop's whitespace pass consumes it on the next iteration. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

jsavin · 2026-05-23T22:47:06Z

Local /gate round 2 (post-round-1 fixes)

Verdict: PASS — zero P0/P1 findings.

Reviewers (HEAD = `ff3f213df`)

bar-raiser ✓ ran (always) — convergence reached, zero new P0/P1. One P2 nit (comment wording) addressed in ff3f213df.
security ✓ ran (always) — no findings. Comment-skip loops are bounded against NUL termination on both // and «...» branches. UTF-8 byte pattern for «/» is unambiguous (only U+00AB encodes as 0xC2 0xAB).
concurrency ✓ ran round 1 (auto: matched frontier-cli/**) — no findings. Not re-run for round 2 (no threading-relevant changes).
swiftui ✗ excluded.

Round 1 → Round 2 progression

Round	P1	P2	Tests
Round 1 (commit `9fa6f60a4`)	1 (comment-between-`}`-and-`else`)	4	24 fixed
Round 2 (commit `a54efc14b`)	0	1 nit (comment wording)	+3 new tests
Round 3 (commit `ff3f213df`)	0	0	+1 case/else test

Pre-existing items NOT introduced by this PR (filed-or-fileable, not blockers)

No quote-state tracking in normalize_newlines_to_semicolons — newlines inside "..." literals still get ; heuristically inserted (correctness only, not security)
src_len * 2 + 1 malloc not overflow-checked (theoretical only, 64-bit only, unreachable in practice over loopback JSON)

Test status

Unit: 489/489 pass
Integration: 1957 passed / 122 failed / 207 skipped / 2286 total (was 1932/146/207/2282 on develop pre-PR — net 24 newly-passing existing tests + 7 new behavioral tests added by this PR)

Ready to merge.

jsavin · 2026-05-24T07:25:57Z

/auto Summary

What went well: The local /gate review caught a real P1 (comment between } and else on its own line broke the lookahead) that would have shipped without it. TDD-first paid off — each fix was preceded by a failing behavioral test. 24 newly-passing integration tests + 7 new regression tests, no regressions.

What could improve: /auto ran inside a sub-agent with isolation: worktree. Phase 6's exit-then-merge sequence can't run from inside such a sub-agent — and when the worktree was removed mid-run (the merge happened via the GitHub UI), the session's cwd died, wedging post-merge cleanup and this comment.

Recommendations: Document that /auto should not be entered from inside an isolation: worktree sub-agent, OR add a fallback path that returns cleanup commands to the parent session when the agent can't ExitWorktree. Phase 5 criterion 8 may also need to require tests run from a genuinely independent context, not just "the main session".

🤖 Generated with Claude Code /auto

…ol mode (#637) The kernel scanner only recognizes single-byte MacRoman 0xC7/0xC8 for «», not UTF-8 0xC2 0xAB/0xC2 0xBB. Protocol-mode scripts (yaml integration tests, REPL paste, programmatic eval) get UTF-8 input and silently fail to recognize curly comments — leading to compile errors or worse, code that scans into the comment body. The normalization layer (#635) handles the lookahead correctly; the gap is at the scanner. Tracked in #637 (kernel-side scanner extension). Until that lands, // is the only portable comment form.

… burndown) (#636) * test(repl): regression sweep for newline normalization after #635 (#620 burndown) Audit every composite/block grammar production in langparser.y for cases where protocol script/eval's bare-CR/LF -> ';' rewrite (normalize_newlines_to_semicolons) could break the parse or silently change semantics. Add 14 regression-guard tests covering: - All non-else block constructs: on f () {...}, loop {...}, while x {...}, for i = 1 to 3 {...}, with table {...}, bundle {...}, try {...} with no else, case x {...} with multiple clauses but no else. These ALL require ';' insertion after the closing '}' before the next statement --- the default normalizer behavior is correct, this pins it down. - Edge cases for the }-else lookahead: blank lines between '}' and 'else', leading newlines at start of script, '//' comment at very end (no trailing newline), whitespace-only script (no crash). - String literal containing '}\nelse {' --- DOCUMENTS the known pre-existing limitation (normalizer does not track string-literal state). Tracked separately; not in scope for this PR. Test pins down current observed output so any future fix shows up as a deliberate test update. Sweep result: no normalizer bugs found. The current '}' lookahead in repl_variables.c correctly handles all composite forms because only if/case/try have an attached elsetoken production in the grammar. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(repl): address /gate review for #636 — fileloop coverage, with-test hygiene P1: add regression test for fileloopheader-bracketedstatementlist (langparser.y:652). Sweep missed this construct. Same shape as loop / for / while / with / bundle — no else continuation; closing '}' is terminal; '}\\nreturn' must gain ';'. Test wraps the fileloop in 'if false { ... }' so the parser exercises the production without touching the filesystem. P2: rewrite the 'with table { body }' test to use a non-mutating witness. The original wrote 'r = 7' from inside 'with system.temp' while 'local (r)' was in scope, propagating a state-mutation pattern future tests shouldn't copy. New version assigns to a local 'q' inside the with block and reads back through r. P2: soften the string-literal documentation test's claim about scanner LF behavior. Original stated 'parsepopchar swallows bare LF' as definitive; we haven't verified end-to-end. Reworded as best-current-hypothesis with a follow-up note. The behavioral assertion (observed output = ']else {') stays — that's the contract. Tests: integration 30/30 in protocol_eval_compile_errors.yaml (was 29). Total 2300 / 1971 passed / 207 skipped / 122 failed (pre-existing TCP cluster). Unit 489/489. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…620 burndown) (#640) * fix(repl): suppress spurious ';' on trailing newline / trailing ';' (#620 burndown) After #635 tightened newline normalization in script/eval, scripts ending with a trailing newline (which is every YAML | block scalar) got a ';' inserted before that final '\n'. The build_wrapped_script suffix '\n}' then made the with-block end in an empty statement, and evaltree returned the wrapper's truthy default ('true') instead of the user's last-expression value. A symmetric issue affected user-typed trailing ';' on the fallback langrunhandle_value path. Three coordinated changes in normalize_newlines_to_semicolons: 1. Factor the whitespace+comment skip into peek_next_real_byte(). The existing '}'-guard lookahead and the new EOF guard share the same skip rules (ASCII WS, '//' line comments, UTF-8 '«...»' comments). 2. EOF guard: when need_sep is true, peek past inter-token noise. If nothing real follows before '\0', suppress the ';' insertion. The '}' check is now subordinate to this -- it only runs when something real does follow. 3. Trailing strip: after the scan, drop trailing ';' / whitespace / '\r' / '\n' so a user-typed trailing ';' doesn't manifest as an empty top-level statement. Tested with five new regression cases in protocol_eval_compile_errors.yaml covering: callback-style multi-line script, simple expr with trailing newline, trailing semicolon-newline, trailing whitespace/tabs, trailing line comment. Integration: 1936 passed / 157 failed -> 2027 passed / 71 failed (net +91 passing, +5 new tests added). All 49 callback_tcp + callback_database tests pass. Zero "got 'true'" failures remain -- the cluster is fully fixed. Unit tests: 489 / 489 pass (unchanged). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(repl): address /gate P2s for #640 — clarify trailing-strip semantics Three comment-only polish items from the local /gate round 1 review: - P2 #1: docblock for normalize_newlines_to_semicolons now explicitly calls out that the trailing-';' strip changes user-visible semantics for explicitly-terminated scripts ("foo();" now returns foo()'s value, not the wrapper's truthy default 'true'). This is the REPL "always return the value of the last expression" convention. Previously the docblock framed it as symmetric closure with the EOF guard, which understated the behavioral change. - P2 #3: add an inline comment at the EOF-guard / '}'-guard branch noting that EOF is checked first and subsumes the '}' guard's EOF case. Order matters and was previously implicit in the if/else-if ordering. - P2 #4: add an inline comment at the trailing-strip loop explaining the `dst > out` guard — it prevents the strip from walking into the prefix on empty/fully-strippable input, since `dst == out` is its initial value before any byte was emitted. No behavioral change. Integration 2028/70/207/2305; unit 489/489. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ollow-up) (#641) * refactor(repl): table-driven normalize_newlines_to_semicolons (#640 follow-up) After three rounds of patches (#628 -> #635 -> #640), the precedence between the start, ';', '{', '}', and EOF/'else' guards in normalize_newlines_to_semicolons had become inline-conditional logic that was easy to misread and hard to extend. This refactor makes the specification explicit: a 5x5 (prev_kind, next_kind) table maps every transition to emit-or-suppress, and the loop becomes a single lookup. Behavioral contract is unchanged. The cells of kEmitSep map 1:1 to the previous code's branch outcomes: - START / SEMI / LBRACE rows: all suppress (matches start-of-script, ';' guard, '{' guard). - RBRACE row: suppress for EOF / ELSE / SEMI / RBRACE (the '}' guard from #635), emit for OTHER. - OTHER row: emit for ELSE / SEMI / RBRACE / OTHER, suppress for EOF (the trailing-newline guard from #620). Structural changes: - classify_prev() reduces the last non-whitespace byte to one of five kinds. classify_next() does the same for the peeked next byte, including the 'else'-keyword check with a trailing-delimiter test. - kEmitSep[5][5] is the new control surface. The cells are the spec; the loop just looks up and acts. - strip_trailing_terminators() factors out the post-pass that walks back over trailing ';' and whitespace (#620 trailing-';' case). - peek_next_real_byte() is unchanged from #635/#640. Tests added (transition-matrix cells previously implicit): - ';' x '}' - explicit ';' before newline-close-brace - '{' x '}' - empty block written across newlines - other x ';' - bare newline followed by explicit ';' - '}' x ';' - closing brace followed by newline then explicit ';' All four pass against the pre-refactor code as well, confirming the refactor preserves the existing contract for the previously-untested cells. Other cells of the matrix are already exercised by the 35 tests added across #624 / #635 / #620. External API is unchanged: normalize_newlines_to_semicolons(const char *) still returns a freshly-malloc'd C string or NULL on allocation failure. No new dependencies, no quote-state tracking (#638), no overflow-check addition (#639) - those are tracked separately. Test results: - Unit: 489/489 pass (unchanged) - Integration: 35 protocol_eval_compile_errors tests + 4 new pass; overall failure count unchanged from develop baseline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(repl): address /gate review for #641 (P1 #1, P2 #3, P2 #5) P1 #1: drop the orphaned 58-line prose docstring at lines 338-395. The refactor moved the function down ~190 lines (to make room for enums, the transition table, and helpers), leaving its docstring stranded above an unrelated typedef. Replace with a tight 17-line summary at the function itself. The "table IS the spec" framing means we no longer need a duplicate prose enumeration of the rules — the kEmitSep comment carries them. P2 #3: enumerate the 5 moot-by-construction cells explicitly near kEmitSep (START x ELSE, START x RBRACE, SEMI x ELSE, LBRACE x ELSE, LBRACE x EOF — inputs that would exercise them are parse errors before normalization matters). Previously the moot set was implicit and the agent's reported count (6) was off by one. P2 #5: rename strip_trailing_terminators -> strip_trailing_whitespace_and_semicolons. The "terminators" name read narrower than the actual stripped set (' ', '\t', '\r', '\n', ';'). No behavioral change. Tests: unit 489/489, integration 2034/68/207/2309 (target file 39/39 — even one flake-recovery vs the prior r1 run). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…#620 burndown) (#643) * fix(repl): PREV_BINOP normalizer class for binary-operator continuations (#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> * test(repl): add 4 BINOP regression tests for /gate P1 findings on #643 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> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

jsavin and others added 3 commits May 23, 2026 15:20

jsavin merged commit f74eb9a into develop May 23, 2026

jsavin deleted the worktree-fix-628-brace-newline-else branch May 24, 2026 07:25

jsavin mentioned this pull request May 24, 2026

kernel: scanner only recognizes MacRoman «» (0xC7/0xC8), not UTF-8 (0xC2 0xAB/0xC2 0xBB) #637

Open

jsavin mentioned this pull request May 25, 2026

refactor(repl): table-driven normalize_newlines_to_semicolons (#640 follow-up) #641

Merged

4 tasks

jsavin mentioned this pull request May 25, 2026

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

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(repl): preserve try/else and if/else across newlines in protocol mode#635

fix(repl): preserve try/else and if/else across newlines in protocol mode#635
jsavin merged 3 commits into
developfrom
worktree-fix-628-brace-newline-else

jsavin commented May 23, 2026

Uh oh!

jsavin commented May 23, 2026

Uh oh!

jsavin commented May 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

jsavin commented May 23, 2026

Summary

Why

Impact

Test plan

Uh oh!

jsavin commented May 23, 2026

Local /gate round 2 (post-round-1 fixes)

Reviewers (HEAD = ff3f213df)

Round 1 → Round 2 progression

Pre-existing items NOT introduced by this PR (filed-or-fileable, not blockers)

Test status

Uh oh!

jsavin commented May 24, 2026

/auto Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Reviewers (HEAD = `ff3f213df`)