fix(review): #118 backstop's maintainer-reply escape hatch fails for thread-less and null-title findings (#133) - #142
Merged
Conversation
…thread-less and null-title findings (#133) The #118 approve backstop holds APPROVE->COMMENT over a still-present prior finding the model silently dropped, unless a maintainer replied on its inline thread. Two cases made that escape hatch silently unavailable, and a third (introduced while fixing them) could over-clear the hold: (b) Null-title findings: the reply check (answeredRootComment) was title-only via rootCommentsByTitle, which returns List.of() for a null title — so a real maintainer reply was never seen and the finding was held every round. Fixed by locating the thread with the hidden thrillhousebot:finding=N marker (title-independent), as rootCommentId already does. (a) Thread-less findings: a finding whose flagged line was outside the diff is posted only in the summary comment, with no inline thread. The no-issues COMMENT body told the maintainer to "reply on their threads" — a thread that does not exist. unresolvedPreviousMessage now qualifies the reply path ("where one exists"); the hold itself is correct and is preserved. Over-clear guard: the marker index recurs every round, so a marker-only lookup could bind a thread-less finding to an EARLIER round's different finding that reuses the same index on the same file and was answered — clearing a still-open finding and re-introducing the #118 silent approve-over-open. The backstop now resolves the marked thread with requireOwnTitle, so a match must also carry the finding's own title; same-title siblings are still told apart by the marker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesThis change improves the handling of previous finding backstops: it uses hidden finding markers to locate maintainer replies even when the finding title is null, and adjusts the review comment text to acknowledge that a backstop-held finding may have no inline thread. The changes primarily affect FollowUpAnalyzer and the summary message in ReviewOrchestrator. Changes Overview
Risk Assessment
No new issues found in this PR, but the review cannot be approved until the required checks are passing.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| docker-pr | check-run | ⏳ Pending | - |
| Analyze (java-kotlin) | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #142 +/- ##
============================================
- Coverage 98.95% 98.87% -0.08%
- Complexity 1393 1407 +14
============================================
Files 51 51
Lines 3716 3730 +14
Branches 523 529 +6
============================================
+ Hits 3677 3688 +11
- Misses 8 10 +2
- Partials 31 32 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ker reuse An xhigh code review found the marker-keyed reply check could over-clear the #118 hold: the marker index recurs every round, so a thread-less finding (and, especially, a null-title finding for which the title guard was a no-op) could bind to an EARLIER round's DIFFERENT finding that reused the same index on the same file — clearing a still-open finding if that unrelated thread was answered, the dangerous #118 silent approve-over-open direction. Resolve the finding's own marked thread by content correspondence: the marked comment must also carry the finding's own content — its title, or its description when it has no title (the bot embeds both in every comment). A null-title finding is then matched by its description rather than the recurring marker alone; a finding with neither title nor description matches nothing and the backstop holds (the safe direction). Strengthen the bot-only-reply test to use a null-title finding so it genuinely exercises the marker branch, and add a regression test for the null-title cross-round marker-reuse over-clear. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…p-reply-escape-hatch # Conflicts: # src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java
…commented-out code The prose comment ended with a quoted phrase plus a semicolon, which tripped SonarCloud's commented-out-code heuristic (1 new MAJOR code smell on the PR). Reword to drop the code-like pattern; no behavior or assertion change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…p-reply-escape-hatch # Conflicts: # src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java
…mmented-code false positive Drop the parenthetical and em-dash from the comment so SonarCloud's commented-out-code heuristic (RSPEC-125) can't score it as code. Comment-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add tests for a finding with a blank title and no/blank description: ownContentKey yields no key, so the backstop holds it (the safe direction) rather than risk an over-clear. Covers the previously-missed return-null line and the blank/absent branches of ownContentKey and bodyCarriesOwnContent that failed codecov/patch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…p-reply-escape-hatch
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check CodeQL is pending
- Check docker-pr is pending
- Check test is pending
- Check Analyze (javascript-typescript) is pending
- Check Analyze (java-kotlin) is pending
…back over a mismatched marker Dogfood review (PR #142) flagged that bodyCarriesOwnContent's bare body.contains(key) can false-positive for a short title that is a substring of a different finding's comment at the same recurring marker index, over-clearing the hold. Anchor a titled finding's match on the header framing " — <title>**" (the literal suggestion dropped the emoji the header includes and assumed the key is the title, but null-title findings key on the description, which is in the body, not the header). Also hold instead of falling back to the title-only scan when a finding=N comment for a DIFFERENT finding is present on the file. Adds a regression test for the short-title-substring case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This was referenced Aug 11, 2026
devops-thiago
added a commit
that referenced
this pull request
Aug 11, 2026
… conversation (#553) ## What type of PR is this? - [x] 🐛 Bug fix ## Description A finding raised below the inline-posting bar (`Finding#postsInline` false: low confidence under high risk) is listed only under **Things to double-check** and opens no review thread. The maintainer-reply escape hatch (#133/#142) resolves a finding's *own* thread by marker plus content, so it can never reach one — while the deterministic backstop keeps holding the finding across rounds. On #544 a LOW was fixed in code and the next review still reported it unresolved: a merge-ready PR showed an unresolved count no action could clear, and APPROVE stayed held by the unresolved-previous gate. The bot's own message admitted the gap in passing ("reply on their review thread (where one exists)"). A write-capable maintainer can now clear such a finding from the PR conversation, and the status line states that path instead of the parenthetical. **Recognition.** A PR conversation comment clears a finding only when every leg holds; any one missing leaves it held: - the comment is a human's, not the bot's — the bot's own summary reproduces every finding verbatim, so reading it back would clear the whole round; - its `author_association` may hold write access (`OWNER`/`MEMBER`/`COLLABORATOR`), the same prefilter the review-thread hatch applies, so a drive-by commenter on a public repo cannot clear a hold; - it carries `@thrillhousebot resolved` **outside quoted context** — blockquote lines and fenced code are stripped first, so GitHub's "Quote reply" of the summary names nothing. Inline code is deliberately kept, because the summary prints each locator as `` `path:line` ``; - it names **that** finding by BOTH its printed `path:line` locator and its own content (its title, or its description when it has no title — the same content anchoring the marked-thread hatch uses). **Over-clearing guards.** No `thrillhousebot:finding=N` marker is read at all, so a pasted marker names nothing and a clear cannot leak across rounds through marker reuse. Locators match whole only (a match followed by another digit is skipped), so a comment about `src/A.java:10` cannot clear the distinct finding at `src/A.java:1`. A finding with neither title nor description matches nothing and stays held. `@thrillhousebot resolve` (the existing thread-resolving command) is not the directive. Both paths honour the clear: the backstop (`holdableTarget`) and the model-reported `unresolved` status (`clearNamedInConversation`, since the model never sees the conversation). The conversational-reply path answers the directive with a fixed acknowledgement rather than an assistant reply, so the bot cannot overstate what the directive does. Plumbing outside the analyzer is minimal and additive: `IssueComment` gains `author_association` (mirroring `PullRequestComment`), and `ReviewContext` gains `conversationComments` — both with back-compat constructors, so no existing construction site changed. The conversation is fetched on the same `hasContext` gate as the inline comments, so a first review never pays for the call. ## Documentation `README.md` documents the directive the same way the sibling behaviours are documented: - a feature bullet next to the follow-up-tracking one, naming both ways to close a finding; - a row in the commands table with its access level, plus a note that it is a directive rather than a slash command, is read by the next review, is not `/resolve`, and takes its access from the comment's `author_association` only (`THRILLHOUSEBOT_REVIEW_MANUAL_TRIGGER_ALLOWED_LOGINS` does not extend it); - a **Clearing a finding with no thread** section after *Re-checking declines*, which until now covered only the thread case — which findings have no thread, a copyable example, and every rule a maintainer must satisfy (both the `path:line` locator **and** the finding's own content, write-capable non-bot author, quoted and fenced text ignored), stating plainly that an ambiguous naming leaves the finding held. No `.env.example` entry: the directive introduces no configuration knob. ## Review findings fixed Two MEDIUM findings the bot raised on this PR, both fixed in `8b73d03`. **Using the directive vs. quoting it** (`FollowUpAnalyzer`). The token matched inside inline code, so documenting the command fired it — a comment explaining the feature (or this PR's own README example pasted into a PR) would clear the finding it names. Inline code was kept deliberately, because the summary prints each locator as `` `path:line` `` and maintainers copy the row, so the rule now separates the two texts rather than choosing between them: the **directive token** must be outside blockquotes, fences *and* inline code, while the **locator and content** are still matched with inline code intact. The inline-code pattern uses a `` `+ `` delimiter so the double-backtick documentation form is caught too. Red proof (directive token matched against the inline-code-preserving text, i.e. the reported defect): ``` [ERROR] Failures: [ERROR] FollowUpAnalyzerTest.backstopShouldSeparateUsingTheDirectiveFromQuotingIt:3064 inline-code directive with the naming inside it ==> expected: <[1, 2]> but was: <[2]> [ERROR] FollowUpAnalyzerTest.backstopShouldSeparateUsingTheDirectiveFromQuotingIt:3064 inline-code directive with the naming outside it ==> expected: <[1, 2]> but was: <[2]> [ERROR] FollowUpAnalyzerTest.backstopShouldSeparateUsingTheDirectiveFromQuotingIt:3064 double-backtick directive, as docs showing a span ==> expected: <[1, 2]> but was: <[2]> [ERROR] FollowUpAnalyzerTest.isClearDirectiveShouldRecognizeOnlyAnUnquotedResolvedInstruction:3012 a marked-up directive is documentation, not an instruction ==> expected: <false> but was: <true> [ERROR] Tests run: 167, Failures: 4, Errors: 0, Skipped: 0 ``` The same parameterized test pins the other direction in the same run — *a real directive naming a backticked locator* and *a real directive naming a plain locator* each clear exactly their own finding and leave the other held, under both the broken and the fixed rule. **Conversation read ceiling** (`ReviewContextLoader`). The fetch already paginates (`GitHubCommentClient.listComments`, 100/page × 10 pages), but hitting that bound was silent. GitHub serves issue comments oldest first with no reverse order on this endpoint, so a capped walk keeps the **oldest** window and drops the newest — exactly where a freshly written directive lives, so the feature would quietly do nothing on the busy PRs that need it most. The bound is now named (`MAX_CONVERSATION_COMMENTS`), recognized (`conversationWalkCapped`), logged with its cause and consequence when reached, and documented in the README. `botSummaryCommentExists` keeps the plain fetch: the summary is posted on the first round, so it lives in the window a capped walk retains. Red proof (ceiling neither matching the real walk nor recognized): ``` [ERROR] Failures: [ERROR] ReviewContextLoaderTest.shouldNameTheCeilingAsTheFullPagedWalk the documented ceiling must be the walk the client actually performs ==> expected: <1000> but was: <100> [ERROR] ReviewContextLoaderTest.shouldRecognizeAWalkThatStoppedAtTheCeiling a full window means the walk stopped at the bound, not at the end of the thread ==> expected: <true> but was: <false> [ERROR] Tests run: 66, Failures: 2, Errors: 0, Skipped: 0 ``` ## Related Issues Fixes #548 ## How Has This Been Tested? - [x] Unit tests Both directions are covered: a comment that clears exactly one finding and nothing else, and comments that must clear nothing (no directive; directive naming nothing; a pasted marker; another round's locator; one finding's locator with another's title; a quote-reply of every summary row; a quoted or fenced directive; the `resolve` command). Ineligible authors (fork author, `CONTRIBUTOR`, absent association, the bot itself, missing author or body) are covered, as is the whole-locator guard and the title/description/neither anchor matrix. ### Red proof With the recognition neutralized (`clearedInConversation` and `isClearDirective` returning `false`, the old message restored) and the new tests in place: ``` [ERROR] Failures: [ERROR] FollowUpAnalyzerTest.backstopShouldClearOnlyOnTheFindingsOwnContent:2872 title — a finding with no content of its own must stay held ==> expected: <[]> but was: <[1]> [ERROR] FollowUpAnalyzerTest.backstopShouldClearOnlyOnTheFindingsOwnContent:2872 description when the title is null — a finding with no content of its own must stay held ==> expected: <[]> but was: <[1]> [ERROR] FollowUpAnalyzerTest.backstopShouldClearOnlyOnTheFindingsOwnContent:2872 description when the title is blank — a finding with no content of its own must stay held ==> expected: <[]> but was: <[1]> [ERROR] FollowUpAnalyzerTest.backstopShouldClearOnlyTheThreadlessFindingTheConversationNames:2712 the named finding must be cleared and the one it does not name must stay held ==> expected: <[2]> but was: <[1, 2]> [ERROR] FollowUpAnalyzerTest.backstopShouldIgnoreAnAbsentConversationEntry:2829 expected: <[2]> but was: <[1, 2]> [ERROR] FollowUpAnalyzerTest.backstopShouldMatchWholeLocatorsOnly:2917 a later whole match still counts ==> expected: <[]> but was: <[1]> [ERROR] FollowUpAnalyzerTest.backstopShouldMatchWholeLocatorsOnly:2917 a match ending the comment counts ==> expected: <[]> but was: <[1]> [ERROR] FollowUpAnalyzerTest.backstopShouldScopeAConversationClearToTheNamedLocator:2738 expected: <[2]> but was: <[1, 2]> [ERROR] FollowUpAnalyzerTest.clearNamedInConversationShouldCloseOnlyTheModelReportedFindingThatWasNamed:2939 expected: <[resolved, unresolved]> but was: <[unresolved, unresolved]> [ERROR] FollowUpAnalyzerTest.isClearDirectiveShouldRecognizeOnlyAnUnquotedResolvedInstruction:3002 expected: <true> but was: <false> [ERROR] MaintainerReplyServiceTest.clearDirectiveIsAcknowledgedDeterministicallyWithoutTheAssistant:651 Wanted but not invoked: commentClient.createComment( "token gh-abc", <any string>, "owner", "repo", 42, <Capturing argument: CreateCommentRequest> ); -> at dev.thiagogonzaga.thrillhousebot.review.MaintainerReplyServiceTest.clearDirectiveIsAcknowledgedDeterministicallyWithoutTheAssistant(MaintainerReplyServiceTest.java:651) Actually, there were zero interactions with this mock. [ERROR] ReviewResultTest.unresolvedPreviousMessageShouldStateHowToClearAThreadlessFinding:581 No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread (where one exists) with why they are deferred. ==> expected: <true> but was: <false> [ERROR] Tests run: 215, Failures: 12, Errors: 0, Skipped: 0 ``` and, in the same neutralized state: ``` [ERROR] Failures: [ERROR] ReviewOrchestratorTest.shouldGiveAClearingPathForABackstopFindingWithNoThread a finding with no thread needs a path that does not depend on one (#548) ==> expected: <true> but was: <false> ``` All twelve pass with the fix in place. ### Gates - `./mvnw -B spotless:apply` then `./mvnw -B clean compile spotbugs:check spotless:check` — `BugInstance size is 0`, `BUILD SUCCESS` - `./mvnw -B clean test` — `Tests run: 2639, Failures: 0, Errors: 0, Skipped: 0` - JaCoCo ∩ `git diff -U0 abd76e5 HEAD` over changed main code — zero uncovered lines and zero uncovered branches (re-verified at each head) ## 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 ## Additional Notes `ReviewOrchestratorTest`/`VerdictBuilderTest` needed stub updates only because `unreportedUnresolvedStatusesFromParsed` gained a parameter and `clearNamedInConversation` is new on the mocked analyzer.
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
The #118 approve backstop holds
APPROVE → COMMENTover a still-present prior finding the model silently dropped, unless a maintainer replied on the finding's inline thread. Two cases made that escape hatch silently unavailable, so the only way to clear the hold was the model's ownresolved/justifiedverdict:answeredRootCommentconsultedrootCommentsByTitle, which returnsList.of()whenfinding.title() == null. A real maintainer reply on a null-title finding's thread was therefore never seen, and the backstop kept emittingunresolvedevery round with no human escape. The reply check now locates the thread by the hiddenthrillhousebot:finding=Nmarker (title-independent), the wayrootCommentIdalready does.COMMENTbody told the maintainer to "reply on their threads with why they are deferred" — but there was no thread to reply on.unresolvedPreviousMessagenow qualifies the reply path ("reply on their review thread (where one exists)"). The hold itself is correct per fix(review): previous-findings tracking doesn't survive force-push — re-raises and silently dropped findings #118 and is preserved; the only bug was the misleading guidance.Over-clear guard (caught in self-review)
The marker index is a 1-based position that recurs every round. A marker-only lookup could bind a thread-less newest-round finding to an earlier, unrelated round's finding that reuses the same index
Non the same file — and if that unrelated thread had been answered, the still-open finding would be silently cleared, re-introducing the exact #118 silent approve-over-open. The backstop now resolves the marked thread withrequireOwnTitle, so a match must also carry the finding's own title before its reply is treated as authoritative. Same-title siblings within a round are still told apart by the marker index. (rootCommentIdand the cross-rounddropRepliedDuplicatestitle path are intentionally left unchanged.)Related Issues
Fixes #133
How Has This Been Tested?
New tests in
FollowUpAnalyzerTest:New test in
ReviewOrchestratorTest > ApproveBackstop:COMMENTguidance is qualified ("where one exists") and no longer unconditionally promises a thread (#133a).Local verification (mirrors CI):
./mvnw spotless:check✅./mvnw verify✅ — 750 tests pass, JaCoCo gate met, SpotBugs cleanChecklist
Additional Notes
The model can still clear these findings via
resolved/justified, so the hold was never strictly permanent; this PR restores the documented maintainer-reply contract for both cases and makes the "reply on their threads" guidance honest. Scoped narrowly to the backstop's clearing decision —rootCommentId, the #129 presence anchor, and the #131 recognized-status handling are untouched.