fix(review): read a diff line with a lexical state machine, not a character walk - #791
Open
devops-thiago wants to merge 3 commits into
Open
fix(review): read a diff line with a lexical state machine, not a character walk#791devops-thiago wants to merge 3 commits into
devops-thiago wants to merge 3 commits into
Conversation
…racter walk
RebuttalContradiction may only overrule a maintainer's decline on code the
revision runs, so it has to tell live code from quoted text. It did that with a
character walk that toggled quote state on any single ", ' or backtick, plus one
tell per shape the toggle got wrong. Every delimiter the walk did not model was a
defect, and they ran both ways: a dispatch inside a block comment, inside a
multi-line literal, or after a label colon read as a URL scheme counted as live
evidence and argued with a maintainer who was right, while a text block or a C++
raw string holding an interior quote closed the toggle early and truncated the
line at the // still inside it, and a template literal's ${...} interpolation was
blanked though it is live code.
The walk is replaced by LiveCodeScanner, a small state machine over the hunk. One
delimiter table drives the states and a stack carries an interpolation's live code
inside a quoted run and the multi-line delimiters from one body line to the next.
The shapes left over are not more tells, they are more states, and naming them
once removes tells rather than adding a seventh: blanking block comments upstream
is what deleted the 1024-character and 16-run bounds the serial-pool exclusion
carried, which three review rounds had raised in turn because a justification
comment kept outgrowing them.
The table is the union of the shapes the corpus carries rather than a lexer per
language, because a per-language profile needs a lexical spec per language and a
file header the caller does not always supply. The two ambiguities that genuinely
need that hint are left alone and documented: Python's // floor division, and a
Rust lifetime pairing with a later apostrophe. Both fail by blanking, which is the
direction every ambiguous decision here resolves toward — when this class fires it
overrules a human, so an over-fire costs an argument with a maintainer who was
right while an under-fire only leaves the decline standing.
Carried state is bounded to one hunk: the scanner is reset at every line that is
not a hunk body line, so a stray delimiter cannot silence anything past its own
hunk and the ```diff fence around each patch cannot open a template literal over
the hunk beneath it. Measuring over 80 commits of this repository's own history
(32535 right-side Java lines) added a state the issue does not name: a hunk that
starts inside a text block shows a closer with no opener, and reading it as an
opener inverted every literal below it and blanked 57 statement-shaped lines,
live methods of ReviewResult among them. A delimiter with a statement terminator
after it closes a literal, it does not start one (JLS 3.10.6); with that rule the
23 that remain are all text block bodies quoting a diff or a JSON payload.
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesReplaces RebuttalContradiction's character-walk line scan with LiveCodeScanner, a state machine that blanks quoted runs (text blocks, raw strings, block comments, template interpolations) and cuts line comments while carrying lexical state across hunk body lines and resetting at headers and fences. It also simplifies the newFixedThreadPool single-thread exclusion regex now that block comments are blanked to whitespace upstream. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["rightSideCode(patch text)"] --> B{"line starts with '-'?"}
B -->|yes| C["drop the removed line"]
B -->|no| D{"hunk body line? '+' or leading space"}
D -->|no| E["reset scanner, then scan line"]
D -->|yes| F["scan line with carried state"]
F --> G{"quoted region open?"}
G -->|yes| H["blank until closer, ${, or line end"]
G -->|no| I{"see /*, //, or a literal opener?"}
I -->|"// comment"| J["cut the line"]
I -->|"opener"| K["push Region; blank body"]
I -->|"ordinary char"| L["append and advance"]
H --> M["region may stay open into the next body line"]
K --> M
J --> M
M --> N["join kept lines"]
N --> O["match dispatch patterns + serial-pool exclusion"]
Changes Overview
Changed Files
Risk Assessment
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: Statement-terminator closer heuristic misfires on backtick/triple-quote bodies and misses '}' closers (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/LiveCodeScanner.java:265)
The rule at this line —if (delimiter.spansLines() && closesAStatement(line, body))— treats a spanning-delimiter opener followed by;,,,),.or+as a closer whose opener sits above the hunk. Its own javadoc states the premise that only holds for"""/''': "a text block's opening delimiter must be the last thing on its line (JLS 3.10.6) and a template literal's opener is followed by its body". For the backtick delimiter (Go raw string / JS template literal) and for Python''', the opener IS followed by its body, so a body that starts with one of those characters is misread. Inputs not in the diff:+ sep :=;abc`` followed on the next body line by+ executor.submit(() -> run(ctx));— the first backtick is read as a non-opener, the closing backtick (at end of line, nothing after it to terminate) is read as an OPENER, a template region is pushed, and the real dispatch on the next line is blanked — the exact false-negative class this PR exists to close. Conversely `+ const m = `, hand work to .submit( here`;` is scanned entirely as live text, so `.submit(` inside quoted prose over-fires against a correct decline, contradicting the "errs toward quoted" design claim. The same rule also misses the mirror input it was built for: a genuine Java text-block closer followed by `}` or `]` (e.g. a text block as the last element of an array initializer — ` """};`) is read as an OPENER and blanks the live code below it in the hunk, because `}` and `]` are absent from the terminator set `;,).+`. The in-diff tests exercise the rule only through `""";` and `""" + SUFFIX`, so neither direction is covered. The suggested fix restricts the heuristic to the multi-character triple-quote delimiters whose JLS rationale actually applies; this is safe for the backtick because a backtick closer at end of line has no terminator after it and is already read as an opener today. The `}`/`]` gap additionally needs the terminator set extended.
thrillhousebot
Bot
added
bug
…olds The rule that reads a spanning delimiter followed by ; , ) . or + as the closer of a literal opened above the hunk rests on one language fact: a Java text block's opening delimiter must be the last thing on its line, so whatever follows such a delimiter belongs to the statement a closer ends. It was applied to every delimiter that spans lines, and the premise is false for the backtick, where a Go raw string and a JavaScript template literal begin their body immediately. A Go separator `;abc` therefore read its own closing backtick as an opener — the first backtick had been dismissed as a closer because the body after it starts with a semicolon — pushed a template region and blanked the real dispatch on the next line, the false-negative class this scan exists to close. A template literal whose body opens with a comma failed the other way: scanned as live text end to end, its quoted prose matched as dispatch evidence and overruled a decline that was right. The same test also missed the shape it was written for. The closing brace and bracket were absent from the terminator set, so a text block that ends an array initialiser or a subscript still read as an opener and blanked the live code below it. Restricting the rule to the triple-quote spellings is not enough by itself, because Kotlin and Python do let a body follow the opener. A delimiter is now read as a closer only when nothing on the rest of its line can close it, so an inline literal opens whatever its body starts with. The cost is a lone backtick closer whose opener sits above the hunk, which now blanks the rest of that hunk — the under-fire direction, which keeps the decline, and the side every ambiguity here resolves to. The four fixtures SonarCloud flagged as java:S6126 are text blocks now, each checked byte for byte against the concatenation it replaces: the closer lines keep seven leading spaces and the diff's blank context lines are spelled \s, which text-block indentation stripping would otherwise have eaten.
Contributor
🤖 ThrillhouseBot — changes since the last review
|
…ustifies The statement-terminator rule reads a delimiter as the closer of a literal opened above the hunk, and it holds only where the language forbids a body after the opener. That is Java, and Java spells its text block """ — Python is the only language in the corpus that spells a literal ''' and it lets the body follow the opener, so the rule never had a premise there. The same-line closer lookahead hid most of it: '''); END''' closes on its own line, so the rule already backed off. A body that starts with a terminator and runs past the end of its line does not, and it scanned as live code, matching a dispatch construct quoted inside the string against a decline that was right. """ keeps the rule and keeps the trade with it, since Kotlin and Python also spell literals that way: the 57 blanked Java statement lines it was measured against are worth more than the Kotlin literal whose terminator-led body spans lines. The delimiter table now records that, and the neighbouring residue found alongside it — a Kotlin raw string ending in a backslash steps over the first quote of its own closer, because the escape column is right for Java and Python and wrong for Kotlin. Both readings of that column fail, in opposite directions: honouring the escape blanks the rest of the hunk, and dropping it closes a Java text block at an escaped quote run and hands the quoted text after it to the matcher as live code. Blanking is the affordable failure, so the column stays as it is.
|
Contributor
🤖 ThrillhouseBot — changes since the last review
|
Contributor
There was a problem hiding this comment.
ThrillhouseBot noted 2 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Raw-string delimiter window rejects the maximum legal 16-character C++ delimiter (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/LiveCodeScanner.java:358)
The scan window for a C++ raw-string d-char-sequence starts atat + 2and runsRAW_DELIMITER_LIMIT(16) positions, i.e.at+2 .. at+17, so the '(' is found only when the delimiter is at most 15 characters. A delimiter of exactly the legal maximum (C++ [lex.string] allows a d-char-sequence of up to 16 characters) puts its '(' atat+18, whichwhile (end < limit)never examines, andrawStringBodyStartreturns -1. The opener then falls back to a plain"string. For a legal raw string whose body holds an interior quote (input not in the diff:+ var s = R"0123456789abcdef(a"b//c)"; executor.submit(() -> run(ctx));), the fallback closes the"at the interior quote and the following//truncates the line, dropping the dispatch — the exact under-fire shape this PR's raw-string support exists to fix. The only rejection test uses an 18-character delimiter, and no fixture pins the 16-character boundary. Verify the C++ 16-char limit and, if confirmed, widen the window by one so the '(' of a full-length delimiter is examined; the 18-char test still rejects with the +1 fix. - LOW: Backslash-newline string continuation scans literal content as live code (over-fire) (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/LiveCodeScanner.java:285)
Input not in the diff: a"(or') string literal continued across lines with a trailing backslash, as C/C++/Python allow — e.g.+ const char* s = "part one\on one line and+ part two"; executor.submit(run);on the next."and'regions must close on their own line, socloserIndexfinds no closer,openLiteralreturns -1 and the quote is treated as ordinary text; the literal content scans as live code rather than being blanked. If that content names a dispatch construct (a wrapped log/error string stating ".submit( here"), the matcher over-fires — the expensive direction the scanner's documented 'errs toward quoted' contract says it resolves away from. No comment or test in the diff mentions backslash-newline continuation, so this is a verification request: if the reviewed corpus can carry C/C++/Python line-continued string literals, the"region would need to span the continuation. The fix direction is non-obvious (carrying a narrow continuation state across body lines), so no exact replacement is offered.
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/LiveCodeScanner.java:109— Closer heuristic misreads a Python/Kotlin triple-quoted opener whose body starts with a terminator and spans lines
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.
What type of PR is this?
Description
Problem
RebuttalContradictionmay only overrule a maintainer's "this runs serially"decline on code the revision actually runs, so it has to tell live code from
quoted text. It did that with a character walk that toggled quote state on any
single
",'or`, plus one tell per shape the toggle got wrong. Everydelimiter the walk did not model was a defect, and they ran in both directions:
who is right: a dispatch inside a block comment, inside a multi-line literal,
or after
case 1://(a label colon read as a URL scheme) counted as evidence.Java text block or C++ raw string holding an interior quote closed the toggle
early, so the
//still inside the literal truncated the line; a templateliteral's
${…}interpolation was blanked whole though it is live code; a URLwhose own path holds
//was cut at the second pair.#651 asks for the decision rather than a seventh tell.
What this does
The walk is replaced by
LiveCodeScanner, a small explicit state machine overthe hunk. One delimiter table — text blocks and triple quotes, C++ raw strings,
template literals and Go raw strings, quotes, char literals, block comments —
drives the states, and a stack carries a template literal's interpolation (live
code inside a quoted run) and the multi-line delimiters from one hunk body line
to the next.
Why a state machine and not another tell. The shapes left over are not more
tells, they are more states: naming them once and letting a table drive them is
what stops the class of bug recurring, and it deletes tells rather than adding
them. Blanking block comments upstream, for instance, removed the 1024-character
and 16-run bounds
newFixedThreadPool's serial-pool exclusion carried — threereview rounds had raised those bounds in turn, each time because a justification
comment ran past the last one. The issue asked for the "hard to spell around"
claim above that pattern to go when this was fixed properly; it is gone.
Why not a lexer per language. The delimiter table is the union of the shapes
the reviewed diffs carry, which answers every shape whose delimiters are
unambiguous across the corpus. A per-language profile would need a lexical spec
per language and a file header the caller does not always supply —
findis alsohanded bare patch fragments. Two ambiguities genuinely need that hint and are
left alone, documented, both failing by blanking: Python's
//floor division,and a Rust lifetime that pairs with a later apostrophe when neither a
&/<prefix, a
;in the span, nor a char-shaped closer marks it.Which way it errs. Toward quoted. When this class fires it overrules a
human, so an over-fire costs an argument with a maintainer who was right, while
an under-fire leaves the decline standing — the class's default outcome anyway.
So an opener whose closer never arrives blanks the rest of its hunk, and an
unlisted URL scheme reads as a comment. The single exception is
${…}, which isread as live code: blanking it erased real dispatches, and the over-fire it
admits needs a Go raw string that quotes
${…}text and names a dispatchconstruct inside those braces.
How far state is carried. Only from one diff body line to the next, and only
for the delimiters designed to span lines. The scanner is reset at every line
that is not a hunk body line —
diff --gitand@@headers, the### fileheading, the
```difffenceReviewDiffFormatterwraps each patch in — so onestray delimiter cannot silence anything past its own hunk, and the fence's own
backticks cannot open a template literal over the hunk beneath it.
States fixed
""", not at the interior quoteR"delim( … )delim"(incl.u8R")${…}interpolationcase 1://,default://note— a label colon read as a schemehttp://host//v1— a doubled slash in a URL pathThe last one is not in the issue; measurement found it. Reading the closer that
opens such a hunk as an opener inverts every literal below it: over 80 commits of
this repository's own history (32535 right-side Java lines) that blanked 57
statement-shaped lines, live methods of
ReviewResultamong them. With the rule,23 remain and every one is the body of a text block quoting a diff or a JSON
payload — the text that must be blanked.
Left deliberately
//floor division cuts the line as if it were a comment. It needsa language hint; it fails by cutting, so the decline stands.
#comment is not a comment to this scan, so a dispatch named in onereads as live code. Stripping
#everywhere would cut#includeand CSScolours; it needs the same hint. Recorded in the javadoc.
;betweenthem blanks that code. The three tells are kept as they were rather than grown.
'\u{1F600}') are missed" waschecked against the code and is not so — the tell already admits escapes up to
ten characters, and
'\u{1F600}'/'A'have had tests since fix(review): stop counting dispatch text inside a string literal as live concurrency evidence #780. Theystay green.
Tests
Every state has a test through the real entry point,
RebuttalContradiction.findwith a diff fixture — no private helper is called directly. New: text blocks,
triple quotes, C++ raw strings (bare, named delimiter, encoding prefix, and four
malformed shapes that must fall back to a plain string), block comments,
multi-line literals, template interpolations (nested, and with braces inside),
label colons, URL paths, the hunk-boundary bound, the
```difffence, and themid-text-block hunk start.
Red/green: the 20 new cases covering the issue's states fail against
mainwithverbatim assertion failures, and the mid-text-block case fails against the state
machine without the closer rule. Full suite: 3493 tests, 0 failures, 0 errors.
Patch coverage: every changed line and branch in
LiveCodeScannerandRebuttalContradictionis covered byRebuttalContradictionTestalone.Related Issues
Closes #651
How Has This Been Tested?
Local gates:
spotless:apply,clean compile spotbugs:check spotless:check(BugInstance size is 0),
clean test(3493 tests, 0 failures). Coverage measuredby intersecting
target/site/jacoco/jacoco.xmlwithgit diff -U0 origin/main.Behaviour measured by scanning 80 commits of this repository's own history
(44477 right-side lines) and counting the live-code lines the scan blanked.
Checklist
Screenshots / Logs
N/A
Additional Notes
LiveCodeScanneris package-private and has no test of its own on purpose: everycase is driven through
RebuttalContradiction.find, so the tests pin thebehaviour the bot has rather than the shape of the helper.