fix(repl): suppress spurious ';' on trailing newline / trailing ';' (#620 burndown)#640
Merged
jsavin merged 2 commits intoMay 25, 2026
Merged
Conversation
…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>
…tics 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>
Owner
Author
Local /gate round 2 (post-polish)Verdict: PASS — zero P0/P1/P2 findings on the round-1 polish commit ( Round 1 → Round 2Round 1 (commit
Tests at HEAD
|
Owner
Author
/auto SummaryWhat went well:
What could improve:
Recommendations:
🤖 Generated with Claude Code /auto |
4 tasks
jsavin
added a commit
that referenced
this pull request
May 25, 2026
…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>
5 tasks
jsavin
added a commit
that referenced
this pull request
May 25, 2026
…#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
After #635 tightened newline normalization in
script/eval, scripts ending with a trailing newline (which is every YAML|block scalar — i.e. nearly every integration test script) got a;inserted before that final\n. Thebuild_wrapped_scriptsuffix\n}then made thewith-wrapper end in an empty trailing statement, andevaltreereturns the parser's truthy default (true) for an empty statement. The user observedtrueinstead of the last expression's value. A symmetric issue affected user-typed trailing;on the fallbacklangrunhandle_valuepath (no wrapper, but still1 + 2;-> empty trailing stmt ->true).Same bug class as #618 / #624 / #628 / #635 — wrapper / parser default masks lost statement value.
Files changed
frontier-cli/repl_variables.c— three coordinated changes tonormalize_newlines_to_semicolons:peek_next_real_byte(). Same skip rules now used by the}guard (from fix(repl): preserve try/else and if/else across newlines in protocol mode #635) and the new EOF guard.need_sepis true, peek past inter-token noise. If nothing real follows before\0, suppress the;insertion. The}check is subordinate — it only runs when something real does follow.;/ whitespace /\r/\nso a user-typed trailing;doesn't manifest as an empty top-level statement.tests/integration/test_cases/protocol_eval_compile_errors.yaml— five new regression tests covering: callback-style multi-line script, simple expr with trailing\n, trailing;\n, trailing whitespace/tabs, trailing//comment.Tests now passing
Integration before -> after:
Zero
got 'true'failures remain (was ~80 unique tests). All 49 tests incallback_tcp.yaml+callback_database.yamlpass.Unit tests: 489 / 489 pass (unchanged).
Cluster scope
This PR fully resolves the "callback test returns 'true' instead of expected value" cluster from #620 burndown:
callback_tcp.yaml— 12 failures resolvedcallback_database.yaml— 21 failures resolvedrepl_basic.yaml— multi-statement / multi-line variants resolvedbigstring_boundary.yaml— multi-line bigstring tests resolvedlang.evaluate,tryError,sys verbsmulti-line tests resolvedRemaining out-of-scope failures (next iteration)
71 failures remain, all in different clusters (no
got 'true'signature, different roots):html.rundirective,html.glossarypatcher,html.cleanforexport, etc. — likely verb implementation gaps.tcp.listenStream,tcp.readStream,tcp.writeStream,tcp.statusStream,tcp.getPeerAddress, etc. — networking integration, not callback parsing.sys environment verbs - isolation test, etc.Database - system table accessible after hydration, etc.None share root cause with this PR — separate investigations.
Verification
1 + 2\nreturning'true'instead of3.repl_eval_with_variables_valuepaths (wrapped and fallback) are now correct.'true'instead of expected value), then GREEN after.