fix(review): drop false description gaps for ignored files#378
fix(review): drop false description gaps for ignored files#378devops-thiago wants to merge 3 commits into
Conversation
Ignored files like pom.xml appear in the diff without a patch, which made the model flag legitimate release bumps as missing from the change. Co-authored-by: Cursor <cursoragent@cursor.com>
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds a post-processing filter to remove description_gaps that incorrectly flag ignored files (e.g., pom.xml, lockfiles) as missing from the PR, and updates the AI review prompt to prevent the model from generating such gaps. Control-Flow Diagram🔀 Show diagramflowchart TD
A["VerdictBuilder.build()"]
B["AI response"]
C["withFilteredDescriptionGaps"]
D{"summary null?"}
E["return original"]
F["dropIgnoredFilePresenceGaps"]
G{"filtered equals original?"}
H["create new ReviewResponse"]
I["return new response"]
J["buildResult"]
A --> B --> C
C --> D
D -- yes --> E
D -- no --> F
F --> G
G -- yes --> E
G -- no --> H --> I
E --> J
I --> J
Changes Overview
Changed Files
Risk Assessment
Key Findings
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| actionlint | 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}
| return gaps.stream().filter(gap -> !isIgnoredFilePresenceGap(gap, ignoredPaths)).toList(); | ||
| } | ||
|
|
||
| private static boolean isIgnoredFilePresenceGap(String gap, Set<String> ignoredPaths) { |
There was a problem hiding this comment.
🟡 MEDIUM — Overly broad gap filter may drop valid description gaps about non-ignored files (medium confidence — verify before acting)
The filter in DescriptionGapFilter.isIgnoredFilePresenceGap drops any gap that contains any ignored file name and matches the absence claim regex, regardless of whether the absence claim actually refers to the ignored file. For example, a gap like “pom.xml is changed, but README is not in the diff” would be dropped because it mentions the ignored pom.xml and includes an absence marker, even though the real mismatch (missing README) is about a reviewable file. This can cause legitimate description/implementation mismatches to be incorrectly hidden. Consider tightening the logic to verify that the claimed-missing file is indeed the ignored one, e.g., by checking if the ignored file name appears adjacent to the absence phrase.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Only drop gaps whose absence claim targets the ignored file itself, not any gap that merely mentions an ignored path elsewhere. Add tests for the VerdictBuilder integration path and edge cases. Co-authored-by: Cursor <cursoragent@cursor.com>
Skip basename-only gap filtering when the gap cites a qualified path that differs from the ignored file, so module-b/pom.xml gaps are not dropped just because module-a/pom.xml changed. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
What type of PR is this?
Description
Filters false
description_gapsbullets that claim ignored files (e.g.pom.xml, lockfiles) are missing from the PR when those files appear asskipped: matches ignored patternin the diff. Also clarifies the review prompt so the model does not treat intentionally omitted patches as absent changes.The filter only drops gaps whose absence claim targets the ignored file itself (not gaps that merely mention an ignored path elsewhere), and skips basename-only matching when the gap cites a different qualified path (e.g. keeps
module-b/pom.xml is not in the diffwhen onlymodule-a/pom.xmlchanged).Fixes the false positive on PR #377 where a legitimate root
pom.xmlversion bump was flagged as a description/implementation mismatch.Related Issues
N/A
How Has This Been Tested?
mvn test -Dtest=DescriptionGapFilterTest,VerdictBuilderTestChecklist
Screenshots / Logs
N/A
Additional Notes
Targets
release/v0.4.0. Re-run/reviewon a release PR that bumpspom.xmlto confirm the "Description vs. Implementation" warning is gone.