From e5351b874ef0da7f49a0a862cd3436f45587037c Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 15:50:18 -0300 Subject: [PATCH 1/3] fix(review): strip inline code spans delimiter-aware in the decline re-check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the DOUBLE_BACKTICK_SPAN / SINGLE_BACKTICK_SPAN regex passes in RebuttalContradiction.assertedText with a small delimiter-aware scanner: an opening run of N backticks closes at the next run of exactly N (per CommonMark), an unclosed run stays literal text, the closer must sit within a 1000-character bound, and the body may cross at most one line ending. Spans whose body carries a longer backtick run and spans holding one newline are now stripped whole, so quoted claim text no longer reopens a correct decline. Anything the scan cannot classify is left in place — under-fire, the keep-the-decline direction. Blockquote lines are still dropped before span stripping. Closes #697 --- CHANGELOG.md | 4 + .../review/RebuttalContradiction.java | 133 +++++++++++++----- .../review/RebuttalContradictionTest.java | 73 ++++++++++ 3 files changed, 175 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e877f24d..d376f3d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to ThrillhouseBot. ## [Unreleased] +### Fixed + +- **Inline code spans in a decline are stripped delimiter-aware** (#697): the decline re-check now scans backtick runs the CommonMark way — an opening run of N backticks closes at the next run of exactly N — so a span whose body carries a longer backtick run (`` `a``b` ``) or one line ending is stripped whole instead of leaving quoted claim text to reopen a correct decline. An unclosed run stays literal, and a length bound still keeps a stray backtick from swallowing the reply + ## [0.6.1] — 2026-08-13 Fixes for defects found after 0.6.0 shipped, most of them during the audit of diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java index aa1251ca..a6e7c3f3 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java @@ -153,38 +153,13 @@ final class RebuttalContradiction { Pattern.compile("```.{0,10000}?```", Pattern.DOTALL | Pattern.MULTILINE); /** - * Inline code spans in a markdown reply — {@code `…`} or {@code ``…``} — quoted constructs, never - * the maintainer's assertion. A backticked quotation in a decline ("the bot's text says {@code - * `it never runs concurrently`}") must not be matched as the maintainer's own assertion about the - * code. Triple-backtick pairs are not handled here because {@link #FENCED_BLOCK} has already - * consumed every one this pattern's bounds could reach. The span body is bounded and confined to - * one line so an unclosed backtick in untrusted prose cannot swallow the rest of the reply; a - * span the bound misses is left in place and behaves as before. - * - *

Two patterns applied in order rather than one alternation, so each stays simple enough to - * read on its own. The double-backtick pass runs first and its body admits a lone backtick, - * because {@code ``…``} exists in markdown precisely to quote text containing one ({@code - * ``x`y``}). Without that, the first closable single-backtick pair inside the span was stripped - * instead and the rest of the quotation survived as prose. Both delimiters are guarded with - * lookarounds so a delimiter is a maximal backtick run: a single-backtick opener cannot start - * inside a {@code ``} delimiter, and {@code ``} cannot half-match as an empty single-backtick - * span. Splitting loses nothing against the alternation: the single pass cannot reach into what - * the double pass removed, because a span body never crosses the newline left behind. - * - *

Each span is replaced with a newline, not a space: a space would bridge the words abutting - * the backticks into a phrase that was never contiguous in the original reply ({@code one at - * a`beat`time} must not become "one at a time"), turning stripping into a way to add a - * claim match. The stripped text is matched with the newlines still in it — {@link #assertedText} - * joins lines with {@code \n}, never a space — and a newline appears inside no claim pattern - * while being a sentence boundary for the quoted note, so stripping can only remove matches — the - * keep-the-decline direction. + * Upper bound on how far {@link #stripInlineSpans} scans past an opening backtick run for its + * closer. An opener whose closer sits beyond the bound is treated as unclosed — literal text — so + * untrusted prose with a stray backtick cannot swallow the rest of the reply into one "span". + * Under-fire is the safe direction: text left in place can only keep matches, and stripping only + * removes them. */ - private static final Pattern DOUBLE_BACKTICK_SPAN = - Pattern.compile("(?Spans are stripped only after the blockquote filter: a span's newline replacement splits its * line, and splitting a {@code >} line before the filter would hand the fragment after the span * to the filter without its {@code >} prefix — quoted material surviving as an assertion, the - * over-fire this method exists to prevent. The order loses nothing in the other direction, - * because a span pattern never crosses a line boundary. + * over-fire this method exists to prevent. The order can only leave a span unstripped in the + * other direction — an opener whose closer lived on a dropped blockquote line no longer closes + * and stays literal — which is the under-fire, keep-the-decline direction. */ private static String assertedText(String rebuttal) { var withoutFences = FENCED_BLOCK.matcher(rebuttal).replaceAll(" "); @@ -419,8 +395,95 @@ private static String assertedText(String rebuttal) { } // Joined, not terminated: a reply that ends mid-sentence must stay unterminated, so // sentenceAround's end-of-text bound is a live case rather than an unreachable guard. - var withoutDoubleSpans = DOUBLE_BACKTICK_SPAN.matcher(String.join("\n", kept)).replaceAll("\n"); - return SINGLE_BACKTICK_SPAN.matcher(withoutDoubleSpans).replaceAll("\n"); + return stripInlineSpans(String.join("\n", kept)); + } + + /** + * Removes inline code spans — quoted constructs, never the maintainer's assertion — with a + * delimiter-aware scan instead of a regex. A backticked quotation in a decline ("the bot's text + * says {@code `it never runs concurrently`}") must not be matched as the maintainer's own + * assertion about the code. + * + *

Per CommonMark, an opening run of N backticks closes at the next run of exactly N + * backticks, so a body may carry any backtick run of a different length ({@code `a``b`} is one + * single-backtick span, {@code ``x`y``} one double-backtick span). A run that never closes is + * literal text, scanned past rather than matched — the delimiter-aware replacement for the regex + * passes this scanner supersedes, which stopped at any interior backtick and left such spans in + * place (#697). Two bounds keep untrusted prose from turning one stray backtick into a span that + * swallows the reply: the closer must sit within {@link #SPAN_BODY_BOUND} characters of the + * opener, and the body may contain at most one line ending (CommonMark allows a span to cross a + * line break; a multi-paragraph "span" here is far more likely an unclosed backtick). Anything + * the scan cannot classify stays in place — under-fire, the keep-the-decline direction. + * + *

Each span is replaced with a newline, not a space: a space would bridge the words abutting + * the backticks into a phrase that was never contiguous in the original reply ({@code one at + * a`beat`time} must not become "one at a time"), turning stripping into a way to add a + * claim match. The stripped text is matched with the newlines still in it — {@link #assertedText} + * joins lines with {@code \n}, never a space — and a newline appears inside no claim pattern + * while being a sentence boundary for the quoted note, so stripping can only remove matches. + * + *

Runs of three or more backticks are handled like any other length; {@link #FENCED_BLOCK} has + * already consumed every paired fence, so what reaches this scan is an inline triple-backtick + * remnant or an unpaired fence, and both resolve correctly (span or literal). + */ + private static String stripInlineSpans(String text) { + var out = new StringBuilder(text.length()); + var i = 0; + while (i < text.length()) { + var c = text.charAt(i); + if (c != '`') { + out.append(c); + i++; + continue; + } + var openerEnd = endOfBacktickRun(text, i); + var closer = closingRunStart(text, openerEnd, openerEnd - i); + if (closer < 0) { + // Unclosed within the bounds: the run is literal text, kept in place. + out.append(text, i, openerEnd); + i = openerEnd; + } else { + out.append('\n'); + i = closer + (openerEnd - i); + } + } + return out.toString(); + } + + /** + * Start index of the run of exactly {@code delimiter} backticks closing a span whose body begins + * at {@code from}, or {@code -1} when no such run sits within {@link #SPAN_BODY_BOUND} characters + * or the body would contain more than one line ending. + */ + private static int closingRunStart(String text, int from, int delimiter) { + var newlines = 0; + var i = from; + var bound = Math.min(text.length(), from + SPAN_BODY_BOUND); + while (i < bound) { + var c = text.charAt(i); + if (c == '`') { + var runEnd = endOfBacktickRun(text, i); + if (runEnd - i == delimiter) { + return i; + } + i = runEnd; + } else { + if (c == '\n' && ++newlines > 1) { + return -1; + } + i++; + } + } + return -1; + } + + /** Index just past the maximal run of backticks starting at {@code at}. */ + private static int endOfBacktickRun(String text, int at) { + var i = at; + while (i < text.length() && text.charAt(i) == '`') { + i++; + } + return i; } /** The sentence containing {@code index}, collapsed to one line and clipped for a note. */ diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java index 500c7874..c1c94694 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java @@ -458,6 +458,79 @@ void shouldStripADoubleBacktickSpanThatQuotesALoneBacktick() { + " at the first inner backtick pair"); } + @Test + void shouldStripASingleBacktickSpanWhoseBodyCarriesALongerBacktickRun() { + var rebuttal = + "Declining — the config literally reads `the pool is a``single-threaded pool` and we" + + " accept the risk for this release."; + + assertTrue( + RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE).isEmpty(), + "a single-backtick span closes at the next run of exactly one backtick, so a longer" + + " interior run is body text and the span must be stripped whole"); + } + + @Test + void shouldStripASpanContainingOneLineEnding() { + var rebuttal = + "Declining — the doc quotes `the handler is\nsingle-threaded by design` but that is the" + + " bot's wording, and the finding is accepted risk for this release."; + + assertTrue( + RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE).isEmpty(), + "a span crossing one line ending is a valid CommonMark span and must be stripped whole"); + } + + @Test + void shouldTreatABacktickRunSpanningTwoLineEndingsAsLiteralText() { + var rebuttal = + "See the note` about pooling.\n\nSeparately, the handler is single-threaded so there is" + + " no race here, per the runbook`."; + + assertTrue( + RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE).isPresent(), + "a backtick pair spanning more than one line ending is not stripped, so the assertion" + + " after the paragraph break must still be re-checked"); + } + + @Test + void shouldTreatAnUnclosedBacktickRunAsLiteralText() { + var rebuttal = + "Declining — see the `runbook section on pooling; the handler is single-threaded and" + + " there is no race here."; + + var contradiction = RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE); + + assertTrue( + contradiction.isPresent(), + "an unclosed backtick run is literal text and must not swallow the assertion after it"); + assertTrue( + contradiction.get().claim().contains("single-threaded"), + "the note must quote the assertion, was: " + contradiction.get().claim()); + } + + @Test + void shouldTreatABacktickRunAtTheEndOfTheReplyAsLiteralText() { + var rebuttal = "The handler is single-threaded, so there is no race here — see `"; + + assertTrue( + RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE).isPresent(), + "a backtick run that ends the reply never closes and must stay literal"); + } + + @Test + void shouldNotLetADistantCloserBeyondTheBoundSwallowTheReply() { + var rebuttal = + "Opening quote `starts here. " + + "x".repeat(1200) + + " The handler is single-threaded, there is no race.` end of quote."; + + assertTrue( + RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE).isPresent(), + "a closer beyond the length bound must not let one backtick swallow the reply; the" + + " assertion inside the unswallowed text must still be re-checked"); + } + @Test void shouldNotBridgeAClaimPhraseAcrossAStrippedSpan() { var rebuttal = From a9b5a8eb43bdb13b5f07151d6313424a186248be Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 16:06:27 -0300 Subject: [PATCH 2/3] fix(review): close the span length bound off-by-one and fix the ordering javadoc A closer exactly SPAN_BODY_BOUND characters past the opener is now found, matching the {0,1000} body the replaced regexes stripped, with a boundary test. The assertedText ordering javadoc no longer claims the blockquote drop can only leave a span unstripped, and drops the inverted under-fire label from the stays-in-place direction. --- .../review/RebuttalContradiction.java | 21 ++++++++++++------- .../review/RebuttalContradictionTest.java | 14 +++++++++++++ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java index a6e7c3f3..6a231c07 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java @@ -156,8 +156,8 @@ final class RebuttalContradiction { * Upper bound on how far {@link #stripInlineSpans} scans past an opening backtick run for its * closer. An opener whose closer sits beyond the bound is treated as unclosed — literal text — so * untrusted prose with a stray backtick cannot swallow the rest of the reply into one "span". - * Under-fire is the safe direction: text left in place can only keep matches, and stripping only - * removes them. + * Text the bound leaves in place behaves exactly as it did before this scanner existed, so the + * bound can narrow the fix, never widen the exposure; stripping only ever removes matches. */ private static final int SPAN_BODY_BOUND = 1000; @@ -381,9 +381,12 @@ private static String findingText(ReviewResponse.Finding finding) { *

Spans are stripped only after the blockquote filter: a span's newline replacement splits its * line, and splitting a {@code >} line before the filter would hand the fragment after the span * to the filter without its {@code >} prefix — quoted material surviving as an assertion, the - * over-fire this method exists to prevent. The order can only leave a span unstripped in the - * other direction — an opener whose closer lived on a dropped blockquote line no longer closes - * and stays literal — which is the under-fire, keep-the-decline direction. + * over-fire this method exists to prevent. In the other direction the order can change what the + * span scan sees both ways, and neither outcome is new exposure: an opener whose closer lived on + * a dropped blockquote line no longer closes and its run stays literal (text the reply carried as + * visible prose all along, exactly what the scan yields for any unclosed run), and a drop that + * pulls an opener and closer within the newline bound strips more, which only removes claim + * matches — the keep-the-decline direction. */ private static String assertedText(String rebuttal) { var withoutFences = FENCED_BLOCK.matcher(rebuttal).replaceAll(" "); @@ -413,7 +416,9 @@ private static String assertedText(String rebuttal) { * swallows the reply: the closer must sit within {@link #SPAN_BODY_BOUND} characters of the * opener, and the body may contain at most one line ending (CommonMark allows a span to cross a * line break; a multi-paragraph "span" here is far more likely an unclosed backtick). Anything - * the scan cannot classify stays in place — under-fire, the keep-the-decline direction. + * the scan cannot classify stays in place, which is not new exposure: unstripped text behaves + * exactly as every reply did before span stripping existed, while stripping only ever removes + * claim matches. * *

Each span is replaced with a newline, not a space: a space would bridge the words abutting * the backticks into a phrase that was never contiguous in the original reply ({@code one at @@ -458,7 +463,9 @@ private static String stripInlineSpans(String text) { private static int closingRunStart(String text, int from, int delimiter) { var newlines = 0; var i = from; - var bound = Math.min(text.length(), from + SPAN_BODY_BOUND); + // +1 so a body of exactly SPAN_BODY_BOUND characters still closes, matching the {0,1000} + // bound of the regex passes this scanner replaced. + var bound = Math.min(text.length(), from + SPAN_BODY_BOUND + 1); while (i < bound) { var c = text.charAt(i); if (c == '`') { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java index c1c94694..ec06ba1c 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java @@ -518,6 +518,20 @@ void shouldTreatABacktickRunAtTheEndOfTheReplyAsLiteralText() { "a backtick run that ends the reply never closes and must stay literal"); } + @Test + void shouldStripASpanWhoseBodyIsExactlyTheLengthBound() { + // 984 filler characters plus " single-threaded" make the body exactly 1000 characters, the + // same maximum the regex passes this scanner replaced would strip. + var rebuttal = + "Declining — the doc quotes `" + + "x".repeat(984) + + " single-threaded` and the finding is accepted risk for this release."; + + assertTrue( + RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE).isEmpty(), + "a span whose body sits exactly at the length bound must still be stripped whole"); + } + @Test void shouldNotLetADistantCloserBeyondTheBoundSwallowTheReply() { var rebuttal = From 1a79646a13817ea90a68a0ab1e414a3c2577ee07 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 16:18:37 -0300 Subject: [PATCH 3/3] docs(review): correct the triple-backtick claim in stripInlineSpans javadoc FENCED_BLOCK only pairs fences within its 10000-character bound and runs before the blockquote filter, so a paired triple run can still reach the span scan and be closed there; say so instead of claiming every paired fence was consumed. --- .../thrillhousebot/review/RebuttalContradiction.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java index 6a231c07..bc842f4b 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java @@ -427,9 +427,11 @@ private static String assertedText(String rebuttal) { * joins lines with {@code \n}, never a space — and a newline appears inside no claim pattern * while being a sentence boundary for the quoted note, so stripping can only remove matches. * - *

Runs of three or more backticks are handled like any other length; {@link #FENCED_BLOCK} has - * already consumed every paired fence, so what reaches this scan is an inline triple-backtick - * remnant or an unpaired fence, and both resolve correctly (span or literal). + *

Runs of three or more backticks are handled like any other length. {@link #FENCED_BLOCK} + * pairs triple-backtick runs only within its 10000-character bound, and the blockquote filter can + * re-join runs that were farther apart in the raw reply, so a paired triple run can reach this + * scan and be closed here even though CommonMark would read the region as a fenced block — an + * over-strip that only removes claim matches. */ private static String stripInlineSpans(String text) { var out = new StringBuilder(text.length());