fix(review): file a second defect as its own finding instead of burying it - #602
Conversation
…ng it Mock fidelity scored 2 YES / 6 PARTIAL across the round-3 corpus, and PARTIAL meant one specific thing every time: the exact reasoning was present, in prose, inside the body of a DIFFERENT finding — or in a walkthrough row — and never emitted as a finding of its own. The model had already judged the defect worth writing down, so this is not a reporting-threshold problem that #545's rebalancing could reach; the supporting observation simply never got promoted. State the promotion rule where the emission guidance already lives: every defect gets its own finding on its own dimension, and stating one inside another finding's description, a summary.file_summaries row or a description_gaps entry is not reporting it — those surfaces carry no severity, no anchor line and no review thread. Disambiguate it from the neighbouring "report each underlying defect exactly once" rule, which forbids restating ONE defect at several lines rather than filing a second one, and add the closing sweep over the response's own text. Dimension 8 and the injected mock-fidelity block say it again at the point the contradiction is usually noticed: while building the finding it was evidence for. Fixes #587
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds a promotion rule and a closing sweep to the review AI's system prompt — echoed in dimension 8 and the injected MOCK_FIDELITY_REQUEST block — requiring that a second, different defect stated as evidence inside another finding, a file_summaries row, or a description_gaps entry be emitted as its own finding, and adds a content-anchor test pinning the seven new markers. Control-Flow Diagram🔀 Show diagramflowchart TD
A["Review generation reads SYSTEM and MOCK_FIDELITY_REQUEST"] --> B["Model drafts findings"]
B --> C["A second defect is cited as evidence inside another finding's body"]
C --> D{"Closing sweep re-reads finding descriptions, file_summaries rows, description_gaps entries"}
D -- "defect stated with no covering finding" --> E["Promote it into its own finding"]
E --> F["Emit finding with severity, anchor line, thread"]
D -- "all defects already filed" --> G["Emit response as drafted"]
F --> G
Changes Overview
Changed Files
Risk Assessment
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| actionlint | 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: Fix efficacy unverifiable from this diff: test pins prompt markers, not model behavior (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java:264)
This PR claims to fix #587, whose trigger is the review model stating a second, different defect (e.g. a mock-fidelity defect) inside another finding's body, a walkthrough row, or a description_gaps entry and never filing it as its own finding. The primary changed line is the new promotion rule at line 264 ('EVERY defect gets its OWN finding, on the dimension it belongs to. ...') — prompt text that is consumed at review-generation time, so the change does reach the trigger path. What the provided material cannot demonstrate is whether it changes the model's output: the only in-diff test, PrReviewPromptsContentTest.aDefectOnAnotherDimensionMustBePromotedOutOfTheFindingItSupports, asserts that seven marker substrings exist in PrReviewPrompts.SYSTEM and MOCK_FIDELITY_REQUEST and cannot exercise the model's generation behavior, so it would pass even if the model ignored the new guidance. The PR description itself states the proof is the issue's corpus regression run, not this test. Verify via that corpus run (a dimension whose reasoning appears in a response must now also appear in the finding list) that the prompt change produces the claimed behavior; the marker test alone does not establish efficacy.
thrillhousebot
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
Addressing the open low-confidence finding, since it sits in the summary section with no inline thread and would otherwise read as unhandled.
Accurate, and deliberately not fixed in code. The caveat itself stands and is worth recording rather than dismissing: this PR's effect on #587 is unverified. Mock-fidelity emission currently measures 2/8 across the round-3 corpus (the reasoning appears inside another finding's body in six of eight languages and is never filed as its own finding). Nothing in this diff demonstrates that number moves. The proof is a corpus re-run against a deployed image containing this change, scored against the existing answer keys — the same method that confirmed #573 at 8/8 and, importantly, caught #575 still failing after it looked correct on paper. Dimension 7 rising above 2/8 is the acceptance test. Two sibling changes in this batch are in the same position and will be measured in the same run: #594 (equivalent injection sinks must publish equivalent severity — #570's acceptance test, which #575 failed once already) and #596 (one claim on one surface). No code change on this PR. |
1 participant
What type of PR is this?
Description
Mock fidelity scored 2 YES / 6 PARTIAL across the round-3 corpus, and
"PARTIAL" meant one specific thing every time:
React #26 stated the defect verbatim inside the producer/consumer finding
("both fixture scores are below threshold, so it can't distinguish correct
filtering from push-everything"). Angular #25 quoted the spec file's own
concession into a walkthrough row, reused it as supporting evidence inside
finding 5, and still never raised it. C# #24 noticed it three separate times,
inside findings 1, 2 and 4.
So the model has already judged the defect worth writing down — this is not the
reporting-threshold problem that #545's rebalancing fixed for the performance
and comment-contradiction dimensions. The failure is that a supporting
observation made while building finding A never gets promoted into its own
finding B, even when it is a different defect, in a different file, on a
different dimension.
What changed
PrReviewPrompts.SYSTEM, in the block that already governs emission ("Severityis not confidence, and neither one is a reason to stay silent"), gains the
promotion rule and the sweep:
second, different defect inside another finding's description, in a
summary.file_summariesrow, or in adescription_gapsentry is notreporting it — those surfaces carry no severity, no anchor line and no review
thread, so a defect that appears only there reaches nobody.
underlying defect exactly once", which forbids restating one defect at
several lines rather than filing a second one. Reading the two together is
a plausible reason the model treats a bury as compliance.
file_summarieslineand each
description_gapsentry for statements describing a defect nofinding covers, and promote each into its own finding at the risk and
confidence its own dimension prescribes. Framed as promotion of material
already written, not new analysis — which is what the issue's option 1 asks
for and what makes it cheap.
Dimension 8 and the injected
MOCK_FIDELITY_REQUESTblock repeat it at thepoint the contradiction is actually noticed: while building the finding it was
evidence for.
Related Issues
Fixes #587
How Has This Been Tested?
Prompt-text change: exempt from red/green proof per the round's standards, and
covered by the deterministic anchor test the repo already uses for this file.
PrReviewPromptsContentTest.aDefectOnAnotherDimensionMustBePromotedOutOfTheFindingItSupportspins the seven markers (five in
SYSTEM, one in dimension 8, one inMOCK_FIDELITY_REQUEST) so a future edit cannot silently revert the guidance.Whether the model acts on it is the corpus's job, and the issue states the
regression test it already provides: the corpus plants exactly one defect per
dimension per PR, so a dimension whose reasoning appears in the response but
not in the finding list is mechanically detectable.
Gates
Coverage — jacoco ∩
git diff -U0 fda4bc7...HEAD:All 29 changed main lines sit inside compile-time
Stringconstants, so theycarry no bytecode of their own; the class's own initialization is exercised by
the content test.
Checklist
Additional Notes
The issue notes that the performance dimension regressed the same way in three
languages and may share this root cause. The rule added here is written per
defect, not per dimension, so it covers that case too — and the sweep names
dimensions 4, 5 and 8 as the ones most often lost, since their evidence is
exactly what gets cited in support of something else.