fix(review): close four residual gaps in the injection-sink floor's defeaters - #701
Conversation
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! |
🤖 ThrillhouseBot PR SummaryWhat this PR doesTightens the injection-sink floor's defeater heuristics in FindingVerificationService: 'parameteriz' joins the defense-noun lookahead, the defense-noun separator admits weak punctuation, a new MITIGATION_ABSENT_MODAL pattern registers modal absence claims, and MITIGATION_DO_SUPPORTED admits one non-negator gap word; seven new tests cover each change and the over-fire guards. Description vs. ImplementationNo mismatch found between the PR description and the change. Changes Overview
Changed Files
Risk Assessment
Key Findings
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| format | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| trivy | 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}
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):
- LOW: Widened punctuation separator flips absence claims whose defense noun is itself denied (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:179)
The new separator class in MITIGATION_ABSENT_SUBJECT's negative lookahead ([\s;:,\u2013\u2014-]+) makes any defense stem within about two words of the escaping verb flip the absence claim into a mitigation, even when the defense mention is part of the denial. Input not in the diff: "Nothing escapes, but the sanitizer is disabled in this diff." — after "escapes", ", " is now a separator, "but " and "the " fill the two word groups, and "sanitiz" matches "sanitizer", so the lookahead's inner pattern succeeds, the negative lookahead fails, and the absence claim is not registered: the floor stays silent although the text explicitly says the sanitizer is NOT running. The same flip occurs for a list of denied defenses: "Nothing escapes, sanitizes, or validates the value" — the continuation verbs "sanitizes"/"validates" are part of the denial, yet the second/third defense stems flip the claim. Under the previous whitespace-only separator, the comma blocked the lookahead and both inputs floored, so this is a sensitivity regression on the same class the PR's own new test treats as absence ("Nothing escapes. The sanitizer was removed in this diff." floors). Verify by running these sentences through service.verify() with a sink-named finding — if they are expected to floor, the lookahead needs to distinguish a denied/disabled defense mention (e.g. restrict the separator or the word window to noun-object slots), or this residual should be documented the way the sentence-boundary case is.
thrillhousebot
Bot
added
bug
9b4d9c0 to
296ba61
Compare
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:179 — Widened punctuation separator flips absence claims whose defense noun is itself denied Both quoted regressions are fixed: the comma is removed from 🤖 Addressed by Claude Code |
|
The next review will close every previous finding this comment names by its |
060dca8 to
ee78bfe
Compare
🤖 ThrillhouseBot — changes since the last review
|
|
The review service encountered an error. Please reply with |
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
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: Comma-coordinated asserted mitigation keeps over-firing (undocumented comma-exclusion residual) (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:216)
The comma exclusion in DEFENSE_OBJECT_AFTER_VERB's separator class is meant to keep appositive denials ("Nothing escapes, but the sanitizer is disabled") as absence claims — correct and tested. But it also prevents a comma-coordinated ASSERTED mitigation from flipping the reading. Input not in the diff: "Nothing escapes, but the sanitizer runs on render." The verb token "escapes" has the "Nothing " prefix, the defense-object check fails on the leading comma (line 216: "[\s;:\u2013\u2014-]+..." cannot match ","), and no mitigation pattern matches "sanitizer runs" (the subject-slot verb list is sanitizes/escapes/validates/parameterizes/encodes and the clause has no be/get/do-support auxiliary) — so the floor fires at high on a sentence that explicitly asserts the sanitizer runs on render. Compare the PR's own test doesNotFloorWhenPunctuationSeparatesTheDefenseNounObject, where the semicolon twin "Nothing escapes; the sanitizer runs on render" does NOT floor: only the punctuation differs. The javadoc above this pattern documents the denial and coordination rationale ("a comma carries coordination ... and appositive denials ...") but not this asserted-follow-up residual, which is exactly the over-fire direction the PR calls dangerous — while the mirror residual (denied defense across a semicolon) IS documented as accepted. The clause splitter treats "but" as a real boundary, so this is a same-sentence asserted mitigation that gets floored. Verify by running "Nothing escapes, but the sanitizer runs on render." through service.verify() with a sink-named finding; if it floors as traced, handle or document the comma-asserted case.
There was a problem hiding this comment.
No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
d4389d2 to
cf0ed38
Compare
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:216 — Comma-coordinated asserted mitigation keeps over-firing (undocumented comma-exclusion residual) Fixed rather than documented: a new 🤖 Addressed by Claude Code |
|
The next review will close every previous finding this comment names by its |
🤖 ThrillhouseBot — changes since the last review
|
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):
- MEDIUM: Coordinator exclusion makes conjoined-verb mitigations over-fire the floor (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:325)
Input not in the diff: "Nothing escapes, but the framework renders the output and escapes it at render." The new MITIGATION_ASSERTED_SUBJECT excludes the coordinators and/or/nor from its subject slot, so the genuine mitigation pair "and escapes" can no longer match (the token before "escapes" is the excluded "and"), and no other subject-slot pair exists because "renders" is not in the verb list. The removed one-pattern alternative did match this sentence via the unexcluded "and" subject, so this is a new over-fire regression in exactly the direction the PR calls dangerous. Tracing the new code: the "escapes" token registers the absence claim (the comma is deliberately not a DEFENSE_OBJECT_AFTER_VERB separator), assertsMitigation() finds nothing (BE/GET none, do-support none, MITIGATION_DEFENSE_ACTION requires a defense-stem subject directly before an operate verb — "framework renders" and "escapes it" are neither), and the floor raises the finding to high on a sentence that asserts the framework escapes at render. The javadoc's own example ("the framework validates and escapes the value" still matches on "framework validates") only holds when the first conjoined verb is defense-listed; a verb-phrase coordination whose first verb is not in the list loses its only subject-verb match. Verify by running the sentence through service.verify() with a sink-named finding; a fix needs the subject-slot walk to step past a coordinator to the real noun phrase, which the current one-pattern shape cannot express. The in-diff tests cover the absence-coordination side ("Nothing escapes, sanitizes, or validates") but not this asserted-mitigation side. - MEDIUM: Defense-action verb must directly follow subject, so adverb-modified mitigations over-fire (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:360)
Input not in the diff: "Nothing escapes, but the sanitizer always runs on render." MITIGATION_DEFENSE_ACTION requires the operate-family verb immediately after the defense-stem subject, so "sanitizer always runs" does not match ("always" intervenes), the subject-slot and do-support patterns do not match either, while the comma keeps "Nothing escapes" registered as an absence claim — and the floor raises to high on a sentence that asserts the defense operates on render. This is the mirror of the do-support adverb case this PR fixed: MITIGATION_DO_SUPPORTED admits one gap word with a negator lookahead so "does always escape" defeats the floor while "does not escape" does not. The defense-action rule instead forbids the gap entirely, so positive adverbs ("always runs", "consistently runs") are invisible along with the intended "never runs". The javadoc documents the "never" side of the decision but not this over-fire cost on the asserted side. Verify with service.verify(); if it floors as traced, admit one non-negator gap word here the same way MITIGATION_DO_SUPPORTED does — "never runs" would then be handled by the negator lookahead plus NEGATING_DETERMINER instead of by absence of a match. The in-diff tests cover the unmodified "sanitizer runs" form but not the adverb-modified assertion.
… refused reviews as comments (#705) ## What type of PR is this? - [x] 🐛 Bug fix ## Description A review on #701 was lost to a GitHub 422 that the logs could not explain. This PR ships the three fixes from the postmortem: 1. **The 422 body is logged.** A rejected review post now reads the HTTP response off the failure's cause chain through the existing `GitHubApiError` seam and logs its diagnostics line — status, throttle headers, and the response body, with credential-shaped text redacted and the length capped. The next 422 will name its cause. 2. **A superseded run abandons its post.** Just before the first write, `ReviewOrchestrator` re-reads the PR head (a fresh read through the token-healing read seam), and when it moved from the sha the run reviewed, the post is abandoned: counted as a structured `HEAD_MOVED` skip via `ReviewSkipEmitter`, the stale check run concluded as `skipped`, and nothing user-facing posted — the dispatcher's coalesced run for the new head re-reviews and posts in its place. This also stops inline comments resolving against a diff that moved underneath them. Fail-open: a failed head read never abandons a finished review. 3. **A refused summary-only review falls back to an issue comment.** `createReviewWithFallback` keeps today's retry-without-comments path; when a no-comments review (or that retry) is refused, the same body is posted as an issue comment — through the capped, paced conversation-write path — with a note that GitHub refused the review post, instead of discarding the generation behind "review could not be completed". `ReviewPostException` now fires only when the comment fallback fails too. Per the issue, this does **not** attempt to fix the unknown 422 root cause (it cannot be named until fix 1 ships) and does not change review-event semantics beyond what the fallback needs. ## Related Issues Closes #704 ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing New tests cover: the rejection-diagnostics path with a response-carrying cause chain; the no-comments 422 falling back to an issue comment carrying the body (and the zero-issues message when the body is blank/null); the comment-carrying 422 keeping the retry-without-comments path and falling through to the comment fallback when the retry is refused too; `ReviewPostException` (with the comment failure suppressed) when the fallback also fails; the full-path head-moved abandon (no review, no comment, `HEAD_MOVED` counted, check run `skipped`); `headMoved` comparisons including fail-open; abandon surviving check-run and persistence failures; and `currentHeadSha` returning the fresh head and empty on every unreadable shape. `./mvnw verify` passes (3222 tests) and a local merge-base JaCoCo simulation of codecov/patch reports 100% on changed lines. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Screenshots / Logs N/A ## Additional Notes CHANGELOG updated under `[Unreleased]` → Fixed.
🤖 ThrillhouseBot — changes since the last review
|
…efeaters Rebased onto the #703 split shapes. DEFENSE_OBJECT_AFTER_VERB gains the parameteriz stem the verb group already had and accepts clause-break punctuation (semicolon, colon, dashes) as a separator, so "Nothing escapes parameterization" and "Nothing escapes; the sanitizer runs on render" read as the mitigations they assert instead of over-firing the floor; the comma is deliberately excluded so comma-coordinated absence verbs and appositive denials stay absence claims, and the denied defense across a clause break is documented as accepted residual under-fire. A new ABSENCE_MODAL_VERB token registers modal absence claims ("Nothing can sanitize the value", one non-negator adverb admitted), MITIGATION_DO_SUPPORTED admits one non-negator gap word so "does always escape" defeats the floor while "does not escape" stays an absence claim, and MITIGATION_ASSERTED_SUBJECT excludes coordinators from its subject slot so "Nothing escapes, sanitizes, or validates the value" does not read its own continuation as a mitigation. Closes #696
…tigation it asserts MITIGATION_DEFENSE_ACTION reads "the sanitizer runs on render" — no auxiliary, verb outside the subject-slot list — so the comma exclusion that keeps appositive denials flooring no longer over-fires the floor on an asserted follow-up; a NEGATING_DETERMINER guard keeps "no sanitizer runs" and "the sanitizer never runs" as the absence statements they are. The end-anchored negation checks now scan a bounded whitespace-aligned Matcher.region window instead of copying the whole prefix per token, keeping the defeater walks linear in the finding text.
…nse actions defeating the floor
The coordinator exclusion on MITIGATION_ASSERTED_SUBJECT dropped the
only subject-verb match a conjoined pair has when its first verb is not
defense-listed ("renders the output and escapes it"), over-firing the
floor. Coordinators stay in the subject slot; a coordinator match is
dropped only when an ABSENCE_VERB_CHAIN closes the text before it, so
"Nothing escapes, sanitizes, or validates the value" still floors.
MITIGATION_DEFENSE_ACTION admits one non-negator gap word, mirroring
the do-support adverb gap, so "the sanitizer always runs" defeats the
floor while "never runs" and "no sanitizer runs" stay absence
statements.
748f906 to
4d9ee16
Compare
🤖 ThrillhouseBot — changes since the last review
|
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):
- LOW: NEGATING_DETERMINER's two-word bound denies idiomatic "no doubt the sanitizer runs" (over-fire) (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:409)
NEGATING_DETERMINER\b(no|not|never|without|nor)\s+(\w+\s+){0,2}\zturns any negator within two words of a MITIGATION_DEFENSE_ACTION match into a denial. Input not in the diff: "Nothing escapes, no doubt the sanitizer runs on render." The action pattern matches "sanitizer runs"; the prefix "no doubt the " satisfies negator + two words +\z, so the denial fires, no other mitigation pattern matches the sentence, and the absence claim ("escapes" — comma excluded from DEFENSE_OBJECT_AFTER_VERB) stands — the floor fires HIGH on a sentence that emphatically asserts the sanitizer runs. The floor also fired before this change (no defense-action pattern existed), so this is a missed improvement rather than a regression — but the new rule contradicts its own documented purpose ("the clause then states the defense does NOT operate"), and over-fire is the direction the PR declares dangerous. This is the false-denial cost the previous review anticipated when the bound was widened from one to two words. Verify the intended handling with service.verify() on the synthesized input; if "no doubt"-type idioms are out of scope, document the trade-off in the javadoc.
… regex budget
ABSENCE_CHAIN_LINK is walked back link by link to the negated subject,
and the defense-action pair is decided by an end-anchored subject
prefix whose captured gap word carries the negator exclusion in code,
so both recognizers accept exactly what the one-pattern forms accepted
while each pattern stays under the S5843 complexity budget. The
negating determiner admits two modifier words ("not a single sanitizer
runs") and documents the idiomatic-negator residual.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:325 — Coordinator exclusion makes conjoined-verb mitigations over-fire the floor All three are addressed on the current head:
🤖 Addressed by Claude Code |
|
The next review will close every previous finding this comment names by its |
🤖 ThrillhouseBot — changes since the last review
|
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: Chain walk attributes a real-subject verb to 'Nothing' and can drop an asserted mitigation (over-fire) (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:911)
New rule:continuesAbsenceChain(withABSENCE_CHAIN_LINK=\b(sanitiz|escap|validat|parameteriz|encod)\w*[,\s]+\zand the confirmationif (negated.region(0, at).find())) drops a coordinator-subject mitigation match when a defense-verb link before the coordinator sits within the NEGATING_SUBJECT_BEFORE_VERB window of any negating pronoun — it never checks that the link verb's OWN subject is that pronoun. Input not in the diff: "Nothing escapes, the framework sanitizes, or validates the value before innerHTML receives it." The sentence asserts "the framework sanitizes or validates the value" (a mitigation); "Nothing escapes" is a separate clause. The absence claim registers: "escapes" is followed by a comma, deliberately excluded from DEFENSE_OBJECT_AFTER_VERB's separator, so no flip. The subject-slot match "framework sanitizes" is dropped by NEGATING_SUBJECT's prefix check ("Nothing" two words back, within the documented {0,3} word room). The new chain check then drops "or validates": the link matches "sanitizes, " and the negation confirmation at that link's start sees "Nothing escapes, the framework " — "Nothing" three words back — although "sanitizes" belongs to the real subject "the framework". No do-support and no defense-action match, so assertsMitigation returns false and the floor fires HIGH on an asserted mitigation — the over-fire direction the PR's javadocs call dangerous — and the ABSENCE_CHAIN_LINK javadoc's claim "so a real subject earlier in the sentence is never chained away" does not hold for this shape. Verify with service.verify() on the synthesized input; the decisive unknown is the exact word window of the pre-existing NEGATING_SUBJECT pattern (not fully shown in the diff) that drops "framework sanitizes" — if that window is narrower than two words, the earlier subject-verb match survives and itself defeats the floor. - LOW: Coordinated absence claim carrying a shared object is not recognized; 'or validates' reads as mitigation (under-fire) (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:346)
New rule:ABSENCE_CHAIN_LINK(\b(sanitiz|escap|validat|parameteriz|encod)\w*[,\s]+\z) only matches when the run straight before the coordinator ends in a defense-verb word plus comma/space. A coordinated absence claim that carries a direct object before its last coordinator escapes the drop: input not in the diff — "Nothing escapes, sanitizes the value, or validates the value before innerHTML receives it." The text before "or" ends "... the value, "; "value" is not a defense stem, the link cannot match, the walk returns false, and "or validates" is kept as a subject-slot mitigation. The absence verbs "escapes"/"sanitizes" register (the comma blocks the defense-object flip), yet a mitigation is also asserted, so the floor stays silent on a sentence asserting none of escaping, sanitizing or validating happens — the same claim class the PR's item-4 coordination test covers ("Nothing escapes, sanitizes, or validates the value"). Under-fire is the PR's documented safe direction, so this is a missed improvement rather than a regression, but item 4's acceptance criterion is only met for object-less verb chains. Verify with service.verify() on the synthesized input.
…arrying chain residual
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:911 — Chain walk attributes a real-subject verb to 'Nothing' and can drop an asserted mitigation (over-fire) Item 1 — declined as a false positive, now pinned by a test. The trace does not hold on two independent counts: (a) Item 2 — accepted residual, documented. "Nothing escapes, sanitizes the value, or validates the value" leaves the trailing "or validates" reading as a mitigation because the object breaks the link run — under-fire, the documented safe direction; the absence claim is still registered by the chain's earlier verbs, and carrying objects through the links is an open-ended grammar problem. Now documented in Per the stop-rule discussed on this PR's earlier rounds: further phrasing variants of the documented residuals (comma-asserted denials across clause breaks, idiomatic negators, object-carrying chains) will be declined citing the javadoc. 🤖 Addressed by Claude Code |
|
The next review will close every previous finding this comment names by its |
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
Additionally, No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
|
@thrillhousebot review |
🤖 ThrillhouseBot — changes since the last review
|
1 participant
What type of PR is this?
Description
Closes the four residual gaps in the injection-sink floor's defeater patterns in
FindingVerificationServicethat survived the #676/#685 hardening:parameterizadded toDEFENSE_OBJECT_AFTER_VERB's defense stems — "Nothing escapes parameterization before the query runs" is the mitigation it asserts and no longer over-fires the floor.ABSENCE_MODAL_VERBtoken reads "Nothing can sanitize the value" as the absence claim it is (one non-negator adverb admitted, sharing the finite wording's subject prefix), with the same defense-noun-object rejection so "nothing can escape validation" stays a mitigation.MITIGATION_DO_SUPPORTEDadmits one non-negator gap word — "the framework does always escape the value" now defeats the floor, while "does not escape" stays an absence claim via the negator lookahead.DEFENSE_OBJECT_AFTER_VERB's separator accepts clause-break punctuation (;:and dashes), so "Nothing escapes; the sanitizer runs on render" reads as mitigated. The comma and sentence-ending punctuation are deliberately excluded: comma-coordinated absence verbs ("Nothing escapes, sanitizes, or validates the value") and appositive denials ("Nothing escapes, but the sanitizer is disabled") stay absence claims, and a defense noun in the NEXT sentence cannot defuse this sentence's claim — all tested. The denied defense across a clause break is documented in the javadoc as accepted residual under-fire. As part of the coordination case,MITIGATION_ASSERTED_SUBJECTexcludes coordinators from its subject slot so "or validates" is not read as a subject-verb mitigation.Over-fire was the dangerous direction; items 1, 3, 4 stop flooring mitigated findings, and item 2's under-fire fix carries its own over-fire guard.
Related Issues
Closes #696
How Has This Been Tested?
Eleven new tests in
FindingVerificationServiceTestcover every acceptance criterion plus the over-fire guards (modal defense-noun object, negated do-support, next-sentence defense noun). Full./mvnw verifyclean locally.Checklist
Screenshots / Logs
N/A
Additional Notes
Each pattern's javadoc is updated to stay accurate, and the CHANGELOG gains an
[Unreleased]Fixed entry. Depends on #703: this branch is rebased ontorefactor/sonar-new-code-smellsand expresses the semantic changes in that PR's split pattern shapes (which is also what keeps each pattern under the S5843 complexity budget). Kept as draft until #703 merges; it will then be rebased onto main.