-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: show dismissed PR review findings in UI instead of silently dropping them #1852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 1 commit
d05ffd5
900a968
94c1420
d5ca83e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1289,12 +1289,33 @@ async def review(self, context: PRContext) -> PRReviewResult: | |
| f"{len(filtered_findings)} filtered" | ||
| ) | ||
|
|
||
| # No confidence routing - validation is binary via finding-validator | ||
| unique_findings = validated_findings | ||
| logger.info(f"[PRReview] Final findings: {len(unique_findings)} validated") | ||
| # Separate active findings (drive verdict) from dismissed (shown in UI only) | ||
| active_findings = [ | ||
| f | ||
| for f in validated_findings | ||
| if f.validation_status != "dismissed_false_positive" | ||
| ] | ||
| dismissed_findings = [ | ||
| f | ||
| for f in validated_findings | ||
| if f.validation_status == "dismissed_false_positive" | ||
| ] | ||
|
|
||
| safe_print( | ||
| f"[ParallelOrchestrator] Final: {len(active_findings)} active, " | ||
| f"{len(dismissed_findings)} disputed by validator", | ||
| flush=True, | ||
| ) | ||
| logger.info( | ||
| f"[PRReview] Final findings: {len(active_findings)} active, " | ||
| f"{len(dismissed_findings)} disputed" | ||
| ) | ||
|
|
||
| # All findings (active + dismissed) go in the result for UI display | ||
| unique_findings = validated_findings | ||
| logger.info( | ||
| f"[ParallelOrchestrator] Review complete: {len(unique_findings)} findings" | ||
| f"[ParallelOrchestrator] Review complete: {len(unique_findings)} findings " | ||
| f"({len(active_findings)} active, {len(dismissed_findings)} disputed)" | ||
| ) | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| # Fetch CI status for verdict consideration | ||
|
|
@@ -1304,9 +1325,9 @@ async def review(self, context: PRContext) -> PRReviewResult: | |
| f"{ci_status.get('failing', 0)} failing, {ci_status.get('pending', 0)} pending" | ||
| ) | ||
|
|
||
| # Generate verdict (includes merge conflict check, branch-behind check, and CI status) | ||
| # Generate verdict from ACTIVE findings only (dismissed don't affect verdict) | ||
| verdict, verdict_reasoning, blockers = self._generate_verdict( | ||
| unique_findings, | ||
| active_findings, | ||
| has_merge_conflicts=context.has_merge_conflicts, | ||
| merge_state_status=context.merge_state_status, | ||
| ci_status=ci_status, | ||
|
|
@@ -1937,12 +1958,38 @@ async def _validate_findings( | |
| validated_findings.append(finding) | ||
|
|
||
| elif validation.validation_status == "dismissed_false_positive": | ||
| # Dismiss - do not include | ||
| dismissed_count += 1 | ||
| logger.info( | ||
| f"[PRReview] Dismissed {finding.id} as false positive: " | ||
| f"{validation.explanation[:100]}" | ||
| ) | ||
| # Protect cross-validated findings from dismissal — | ||
| # if multiple specialists independently found the same issue, | ||
| # a single validator should not override that consensus | ||
| if finding.cross_validated: | ||
| finding.validation_status = "confirmed_valid" | ||
| finding.validation_evidence = validation.code_evidence | ||
| finding.validation_explanation = ( | ||
| f"[Auto-kept: cross-validated by {len(finding.source_agents)} agents] " | ||
| f"{validation.explanation}" | ||
| ) | ||
| validated_findings.append(finding) | ||
| safe_print( | ||
| f"[FindingValidator] Kept cross-validated finding '{finding.title}' " | ||
| f"despite dismissal (agents={finding.source_agents})", | ||
| flush=True, | ||
| ) | ||
| else: | ||
| # Keep finding but mark as dismissed (user can see it in UI) | ||
| finding.validation_status = "dismissed_false_positive" | ||
| finding.validation_evidence = validation.code_evidence | ||
| finding.validation_explanation = validation.explanation | ||
| validated_findings.append(finding) | ||
| dismissed_count += 1 | ||
| safe_print( | ||
| f"[FindingValidator] Disputed '{finding.title}': " | ||
| f"{validation.explanation} (file={finding.file}:{finding.line})", | ||
| flush=True, | ||
| ) | ||
| logger.info( | ||
| f"[PRReview] Disputed {finding.id}: " | ||
| f"{validation.explanation[:200]}" | ||
| ) | ||
|
Comment on lines
1957
to
+1989
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cross-validation protection and dismissed-finding retention look correct. Two downstream issues to address:
Proposed fixesFix 1 — Accurate log at line 2000: logger.info(
- f"[PRReview] Validation complete: {len(validated_findings)} valid, "
+ f"[PRReview] Validation complete: {len(validated_findings)} total "
+ f"({len(validated_findings) - dismissed_count} confirmed, "
f"{dismissed_count} dismissed, {needs_human_count} need human review"
)Fix 2 — Add label mapping at line 2205: status_label = {
"confirmed_valid": "Confirmed",
+ "dismissed_false_positive": "Disputed by Validator",
"needs_human_review": "Needs human review",
}.get(f.validation_status, f.validation_status)🤖 Prompt for AI Agents |
||
|
|
||
| elif validation.validation_status == "needs_human_review": | ||
| # Keep but flag | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||
| * - Quick select actions (Critical/High, All, None) | ||||||||||||||||||||||||||||||||||||||||||||||
| * - Collapsible sections for less important findings | ||||||||||||||||||||||||||||||||||||||||||||||
| * - Visual summary of finding counts | ||||||||||||||||||||||||||||||||||||||||||||||
| * - Disputed findings shown in a separate collapsible section | ||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| import { useState, useMemo } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -16,6 +17,9 @@ import { | |||||||||||||||||||||||||||||||||||||||||||||
| CheckSquare, | ||||||||||||||||||||||||||||||||||||||||||||||
| Square, | ||||||||||||||||||||||||||||||||||||||||||||||
| Send, | ||||||||||||||||||||||||||||||||||||||||||||||
| ChevronDown, | ||||||||||||||||||||||||||||||||||||||||||||||
| ChevronRight, | ||||||||||||||||||||||||||||||||||||||||||||||
| ShieldQuestion, | ||||||||||||||||||||||||||||||||||||||||||||||
| } from 'lucide-react'; | ||||||||||||||||||||||||||||||||||||||||||||||
| import { useTranslation } from 'react-i18next'; | ||||||||||||||||||||||||||||||||||||||||||||||
| import { Button } from '../../ui/button'; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -47,17 +51,29 @@ export function ReviewFindings({ | |||||||||||||||||||||||||||||||||||||||||||||
| const [expandedSections, setExpandedSections] = useState<Set<SeverityGroup>>( | ||||||||||||||||||||||||||||||||||||||||||||||
| new Set<SeverityGroup>(['critical', 'high']) // Critical and High expanded by default | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
| const [disputedExpanded, setDisputedExpanded] = useState(false); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // Filter out posted findings - only show unposted findings for selection | ||||||||||||||||||||||||||||||||||||||||||||||
| const unpostedFindings = useMemo(() => | ||||||||||||||||||||||||||||||||||||||||||||||
| findings.filter(f => !postedIds.has(f.id)), | ||||||||||||||||||||||||||||||||||||||||||||||
| [findings, postedIds] | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // Split unposted findings into active vs disputed | ||||||||||||||||||||||||||||||||||||||||||||||
| const activeFindings = useMemo(() => | ||||||||||||||||||||||||||||||||||||||||||||||
| unpostedFindings.filter(f => f.validationStatus !== 'dismissed_false_positive'), | ||||||||||||||||||||||||||||||||||||||||||||||
| [unpostedFindings] | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const disputedFindings = useMemo(() => | ||||||||||||||||||||||||||||||||||||||||||||||
| unpostedFindings.filter(f => f.validationStatus === 'dismissed_false_positive'), | ||||||||||||||||||||||||||||||||||||||||||||||
| [unpostedFindings] | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
| // Split unposted findings into active vs disputed | |
| const activeFindings = useMemo(() => | |
| unpostedFindings.filter(f => f.validationStatus !== 'dismissed_false_positive'), | |
| [unpostedFindings] | |
| ); | |
| const disputedFindings = useMemo(() => | |
| unpostedFindings.filter(f => f.validationStatus === 'dismissed_false_positive'), | |
| [unpostedFindings] | |
| ); | |
| const { activeFindings, disputedFindings } = useMemo(() => { | |
| const active: PRReviewFinding[] = []; | |
| const disputed: PRReviewFinding[] = []; | |
| for (const finding of unpostedFindings) { | |
| if (finding.validationStatus === 'dismissed_false_positive') { | |
| disputed.push(finding); | |
| } else { | |
| active.push(finding); | |
| } | |
| } | |
| return { activeFindings: active, disputedFindings: disputed }; | |
| }, [unpostedFindings]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve efficiency and readability, you can partition the
validated_findingsintoactive_findingsanddismissed_findingsin a single loop instead of iterating over the list twice.