fix(review): keep real findings in the previous-findings context across a zero-finding round - #470
Merged
Conversation
…ss a zero-finding round A follow-up round that found nothing evicted the still-open finding raised before it and put the bot's own review body in its place: the fallback that exists for sessions with no persisted AI response was selected whenever the structured rendering came out empty, which is also true of a persisted round that legitimately found nothing. The model was then shown the sentence "No new issues in this revision, but N previous finding(s) remain unresolved" under a header saying those were the issues flagged in the previous review, the real finding lost its id, and because the backstop mapped each round's previous_findings_status over the empty round, nothing ever closed and the unresolved count climbed with every further round until APPROVE was permanently demoted to COMMENT. Three changes, all in the context-construction path: - The caller now states whether a readable response was persisted instead of the analyzer inferring it from the shape of its own output, so the review body fallback fires only for the case its javadoc describes. - That fallback discards a body it recognizes as one the bot generated about its own verdict, matched against the producers' own constants so the recognizer cannot drift from the text it recognizes. A body the bot did not write is still passed through. - The prior round a review reports on is the newest persisted round that actually raised findings, and the approve backstop pairs a status block with that same round rather than blindly with the round before it. The round's list is carried whole, so every id stays the 1-based position the finding had when it was posted — the index its inline comment's marker carries and the id space previous_findings_status and the decline re-check both reference. Refs #455
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesPrevents a zero-finding review round from evicting real prior findings and feeding the bot's own status body back to the model as a phantom finding, which inflated the unresolved count and blocked approval. It introduces effective-previous-finding logic that skips empty rounds, adds a persisted-response flag to avoid misusing the fallback, and detects and discards bot-generated status bodies. Control-Flow Diagram🔀 Show diagramflowchart TD
A["ReviewContextLoader.load() computes previousRoundIndex / effectivePreviousFindings"]
B["Calls buildPreviousFindingsContext(previousFindings, previousResponsePersisted, ...)"]
C{"structured not empty or previousResponsePersisted?"}
D["Return structured + answered"]
E["Fallback: buildPreviousFindingsContext(priorReviews)"]
F{"body null or isSelfAuthoredStatusBody?"}
G["Return '' + answered"]
H["Return body + answered"]
A --> B
B --> C
C -- yes --> D
C -- no --> E
E --> F
F -- yes --> G
F -- no --> H
Changes Overview
Changed Files
Risk Assessment
No new issues found in this PR, but the review cannot be approved until CI is confirmed green.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| trivy | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
thrillhousebot
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…elpers codecov/patch flagged four uncovered branches in the round-selection helpers added for #455: the absent-list and absent-slot guards in effectivePreviousRoundIndex, the absent-response guard in isPersistedResponse, and the second half of the isUnresolvedPreviousMessage match. Each is load-bearing rather than defensive noise. The helpers define the id space every downstream consumer keys off, so absent input has to degrade to "no previous round" instead of failing the review mid-flight, the way parsePreviousResponses and toStatuses already treat it — the javadoc now says so. isPersistedResponse discriminates by reference identity, so the natural "simplification" to equals() would report a round that legitimately found nothing as unpersisted and put the review-body fallback back in the path the fix removed it from. And both ends of isUnresolvedPreviousMessage are required because everything it accepts is discarded: a human review body that merely opens with the same words carries a real finding, and matching on the opening alone would throw it away. Refs #455
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
|
18 tasks
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
When a follow-up review round produced zero findings, the next round's
## Previous Review Findingssection was filled with the previous bot review's body text verbatim — including the bot's own"No new issues in this revision, but N previous finding(s) remain unresolved …"boilerplate — while the real prior finding vanished from tracking. The unresolved count then drifted upward every round andVerdictBuilderdemoted APPROVE → COMMENT indefinitely.Three defects, all in the context-construction path:
The review-body fallback fired on the wrong condition.
FollowUpAnalyzer.buildPreviousFindingsContext(...)selected it whenever the structured rendering came out empty, which is true both when there is genuinely no persisted AI response (its documented purpose) and when a persisted round legitimately found nothing. The caller now passes that fact explicitly (previousResponsePersisted) instead of it being inferred from the shape of the output.The fallback offered the bot's own prose as findings. Even in the legitimate no-persisted-response case, a review body the bot generated about its own verdict carries no finding, so presenting it under "the following issues were flagged … determine if it is resolved, unresolved, or justified" is a category error. The fallback now discards a body it recognizes as self-authored, matched against the producers' own constants (the unresolved-previous sentence, the clean-review message, the two CI-hold notices, the partial-review banner) so the recognizer cannot drift from the text it recognizes. A body it did not generate is still passed through.
The open set was re-derived from the latest round rather than carried. A zero-finding round exposes no ids, so treating it as "the previous round" evicted the still-open finding from the prompt, from
previous_findings_status, and from every id-keyed consumer. The prior round a review reports on is now the newest persisted round that actually raised findings, and the deterministic backstop pairs each round'sprevious_findings_statuswith that same round instead of blindly with the round before it — which is why the count could never come back down (the ids never mapped, so nothing ever closed).Depth chosen. The issue's suggested fix (3) — carry-forward — is implemented in the form that does not destabilise the id space the #169 decline re-check depends on: the effective previous round's list is carried whole, so every id stays exactly the 1-based position the finding had when it was posted, which is the index its inline comment's hidden
thrillhousebot:finding=Nmarker carries. Filtering closed findings out of the carried list (the other reading of "carry forward") would renumber the survivors and silently break marker-based thread matching, so it is deliberately not done. Accumulating findings across rounds that each raised some remains the backstop's job, unchanged.Files
FollowUpAnalyzer.java— explicitpreviousResponsePersistedflag on the context builder;effectivePreviousFindings/effectivePreviousRoundIndex(skip zero-finding rounds);isPersistedResponse(tells a parsed empty round from the blank stand-in without a second parse);isSelfAuthoredStatusBodyguard on the review-body fallback; backstop replay now pairs a status block with the newest earlier round that raised findings.ReviewContextLoader.java— resolves that round once, and derives the rendered context, the id space (previousFindingsList()), the raw JSON the supersede pass re-reads, and the older-rounds slice from it, so the three cannot drift apart.ReviewResult.java—isUnresolvedPreviousMessage(...)plus shared lead-in constants for the CI-hold and partial-review bodies, so the guard above matches text the producers own.ReviewPublisher.java—noIssuesBodynow builds from those shared constants (text unchanged, byte for byte).Related Issues
Fixes #455
How Has This Been Tested?
Eleven new tests, each validated red → green. Because the tests reference the new API, plain
git stash push -- src/main/javayields a compile failure rather than an assertion; the production change was therefore neutralized behaviourally instead — the four changed decisions reverted to their old form with the new signatures kept — so every test fails on its assertion. Verbatim red-phase output (?is the console rendering the em dash / emoji):Restoring the production change turns all seven green.
Four further tests landed in
a763f28to close thecodecov/patchgap, covering the absent-input branches of the new helpers (plus one added assertion on the existingreviewBodyFallback…test). Seven mutations in total, each neutralizing one guard or conjunct in the production method and nothing else:The
equals()mutation is the one worth keeping pinned: it is the obvious-looking simplification, and it silently puts the review-body fallback back into the exact path this PR removes it from, because a round that legitimately found nothing is equal to the blank stand-in without being the same object.The three regression tests the issue names:
ReviewContextLoaderTest.findingFromRoundOneSurvivesAZeroFindingRoundNumberedAndInTheIdSpace(drives the realload(...)with the realFollowUpAnalyzer, asserting the section actually handed to the model) plusVerdictBuilderTest.unresolvedCountAcrossAZeroFindingRoundStaysAtTheOneRealFinding.unresolvedPreviousMessageshape never appears in{{previousFindings}}—FollowUpAnalyzerTest.previousFindingsContextShouldNeverCarryTheBotsOwnStatusBody, covering both the zero-finding path and the legitimate fallback path;reviewBodyFallbackShouldDropEveryBotGeneratedBodyAndKeepTheRestextends it to every body the bot generates and pins that a body it did not generate is preserved.VerdictBuilderTest.resolvedPriorFindingNoLongerPhantomHoldsApproveAfterAZeroFindingRound.The two #169 pins:
ReviewContextLoaderTest.zeroFindingRoundWithNothingToCarryLeavesTheIdSpaceEmptyAndTheSectionAbsent— with nothing to carry,previousstays empty and no pseudo-finding fabricated from a review body ever enters the id spacerecheckDeclinesuses.ReviewContextLoaderTest.carriedFindingsKeepTheIdsTheirOwnRoundGaveThem— a real prior finding keeps the id its own round gave it across a zero-finding round, soprevious_findings_statusid 2 still names the same finding it always did.Build:
./mvnw -B spotless:apply— clean./mvnw -B clean compile spotbugs:check spotless:check—BugInstance size is 0, BUILD SUCCESS./mvnw -B clean test— 2325 tests, 0 failures, 0 errorsCoverage:
codecov/patchwent 91.49% → 100% (47/47 lines, 0 misses, 0 partials). What was uncovered was the absent-input handling on the three new helpers — thenulllist andnullslot guards ineffectivePreviousRoundIndex, thenullguard inisPersistedResponse, and thenullguard plus theendsWithhalf ofisUnresolvedPreviousMessage. All are now covered by the four tests above. Nocodecov.yml,pom.xml, or workflow file was changed.Checklist
Screenshots / Logs
Round 3 of PR #449, before this change (verbatim from the deployed app log):
After this change the same round renders round 1's real finding, numbered
1., with its original id.Additional Notes
README.md.Set.copyOf(plan.omittedFiles()).contains(file.filename())NPE atFindingPipeline.java:563. It is untouched and still open.parsePreviousResponsesends inList.copyOf, andReviewContext's compact constructor does too, so neither anulllist nor anullelement can actually reacheffectivePreviousRoundIndexorisPersistedResponsetoday. I kept them and tested them rather than deleting them, on two grounds: they arepublic staticmethods rather than private helpers, and null-tolerance is the established convention of this class —parsePreviousResponses(null),toStatuses(null)andformatAnsweredEarlier(null)all behave the same way and are already tested. Each guard now carries a javadoc sentence stating the contract, so the tests pin documented behaviour rather than an accident. The alternative — deleting the guards and letting the callers'List.copyOfbe the only defence — is a defensible reading, and it is a small change in each of the three spots if preferred.