fix(review): cap/reject "undefined symbol" findings whose definition is outside the diff hunk (#192) - #194
Conversation
…is outside the diff hunk (#192) GitHub serves only ~3 lines of context around each hunk, so a definition (an env var, import, variable, or config key) that sits a few unchanged lines outside the changed range is never in the reviewed material. The model then sees a use with no nearby definition and confidently flags the symbol as undefined — e.g. the false CRITICAL on release.yml in PR #88 claiming NEXT/TAG were undefined when the step's env: block defines them. Two prompt-level guards (the cheap interim fix ahead of the #55 codebase-aware-context spike): - Verifier (FindingVerifierPrompts): reject an "undefined / unset / missing symbol" finding when the symbol's definition is not present in the provided material — absence from the diff is not proof of absence. Calibrated to at most "low" confidence otherwise. A rejected verdict drops the finding in FindingVerificationService, so the false positive never posts. Includes the PR #88 case as an inline regression example. - Generator (PrReviewPrompts): generalize the existing "value is missing" self-check to all undefined/unset-symbol claims and spell out the hunk-window blind spot, so the finding is less likely to be emitted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds prompt‑level guards to the reviewer and verifier to avoid flagging symbols as undefined when their definitions are just outside the diff hunk context, suppressing a demonstrated false positive.
|
| Risk | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟠 High | 1 |
| 🟡 Medium | 0 |
| 🔵 Low | 0 |
Key Findings
- HIGH: Verifier rule will reject valid missing‑symbol findings (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerifierPrompts.java:50)
⚠️ Required CI Checks Status
Some required checks are still pending or have failed:
| Check | Type | Status | Detail |
|---|---|---|---|
| test | 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}
| @@ -48,6 +48,15 @@ public final class FindingVerifierPrompts { | |||
| that location's content in the diff contradicts the assertion, or the two | |||
| locations belong to different enclosing units (different functions, blocks, or | |||
| scopes) without the finding acknowledging it. | |||
There was a problem hiding this comment.
🟠 HIGH — Verifier rule will reject valid missing‑symbol findings
The new rejection rule in the verifier prompt instructs the LLM to "Reject such a finding unless the definition IS in the material and shown to be missing or misspelled." A genuine undefined/missing symbol (e.g., a missing import that is not in the diff) has no definition in the provided material. Consequently, the verifier will reject that valid finding, creating a false negative. This is a regression from the previous behaviour where such findings were not automatically discarded. The PR intends to suppress the false positive where a definition exists outside the hunk, but the rule is too broad and will suppress true positives as well.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #194 +/- ##
============================================
+ Coverage 99.15% 99.22% +0.06%
- Complexity 1745 1746 +1
============================================
Files 63 63
Lines 4639 4639
Branches 656 656
============================================
+ Hits 4600 4603 +3
+ Misses 10 8 -2
+ Partials 29 28 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…laims (#192) The first version told the verifier to "Reject such a finding unless the definition IS in the material and shown to be missing" — self-contradictory and, read literally, would reject every undefined-symbol finding, including genuinely missing symbols the diff itself demonstrates (a false negative regression flagged in review). Reframe the rule around verifiability: reject only when the scope a definition would occupy (import block, enclosing scope, env/config section) is NOT shown in the material — there the claim is unconfirmable because the definition may sit just outside the hunk. When the material does show that scope and the symbol is genuinely absent or misspelled (e.g. the diff removes the definition), the finding is demonstrable and stands. Calibration sentence and CHANGELOG updated to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
/review |
1 participant
What type of PR is this?
Description
The reviewer (and the verifier) see only GitHub's unified-diff patch — ~3 lines of context around each change. When a finding depends on an unchanged definition that sits just outside the hunk window in the same file, that definition is never in the reviewed material, so the model sees a use with no nearby definition and confidently flags the symbol as undefined.
Dogfood evidence — #88: the bot posted a 🔴 CRITICAL "NEXT and TAG variables not defined in bump PR step" on
.github/workflows/release.yml. The step'senv:block does defineTAG/NEXT, but those lines are unchanged context a few rows above the changedrun:line, collapsed out of the diff. The finding is false; the workflow is correct.This PR adds two prompt-level guards — the cheap interim fix ahead of the #55 codebase-aware-context spike, mirroring how #107 frames its guard:
FindingVerifierPrompts): rejects an "undefined / unset / missing symbol" finding (variable, parameter, import, function, env var, config key) when the symbol's definition is not present in the provided material — its absence from the diff is not proof it is undefined. Calibrated to at mostlowconfidence otherwise. Arejectedverdict drops the finding inFindingVerificationService, so the false positive never posts. The PR fix(release): unblock automated bump-version PR creation (#11) #88 case is embedded as an inline regression example.PrReviewPrompts): generalizes the existing "value is missing" self-check to all undefined/unset-symbol claims and spells out the hunk-window blind spot, so the finding is less likely to be emitted in the first place.Deliberately out of scope: the issue's other mitigation — widening same-file context (pull more of each changed file / the full file when small into the prompt) — is a heavier change (extra
getFileContentcalls per file, a budget knob interacting withmax-diff-lines) that overlaps the #55 and #53 spikes. This PR is the prompt-rule guard that fully neutralizes the demonstrated false positive.Related Issues
Fixes #192.
How Has This Been Tested?
Prompt-text + CHANGELOG change (no behavioral Java logic).
AiServicePromptRenderingTestpasses (5/5) — the real quarkus-langchain4j pipeline still renders both prompts with every@Vcontext variable. Verified inFindingVerificationServicethat arejectedverdict drops the finding from the kept set, so the new reject rule has teeth.Checklist
No new automated test: these are LLM prompt-rule additions with no deterministic behavior to assert; the existing prompt-rendering regression test already pins that both templates render with all context variables.
Additional Notes
The verifier rule is independent of #105 (confidence-gated posting): it rejects (removes) the finding rather than merely capping it to
low, so it reduces noise regardless of whether low-confidence findings still land inline.