From d98ff7d19c55fd76cd7ff585d5f93f0ce497ac16 Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Tue, 17 Feb 2026 10:19:24 +0100 Subject: [PATCH] fix: show dismissed PR review findings in UI instead of silently dropping them (#1852) * fix: show dismissed PR review findings in UI instead of silently dropping them Specialists would find issues but the AI validator could dismiss them all, leaving users seeing "0 findings" with no visibility into what was found or why it was dismissed. Now dismissed findings appear in a collapsible "Disputed by Validator" section so users can review and optionally post them. Co-Authored-By: Claude Opus 4.6 * refactor: optimize finding separation logic in parallel orchestrator and review findings component Updated the logic for separating active and dismissed findings in both the backend and frontend components. The new implementation uses a single pass to categorize findings, improving efficiency and readability. This change enhances the overall performance of the review process by reducing the number of iterations over the findings list. * fix: resolve PR review follow-up findings for dismissed findings handling Fix 2 MEDIUM blocking issues: add 'dismissed_false_positive' label to summary status_label dict (preventing raw string in GitHub comments), and preserve disputed finding selections in selectAll/selectImportant. Also fix 5 LOW issues: conditional opacity for selected disputed findings, remove unused i18n key, add missing validation fields to IPC interface, add aria-expanded to disputed toggle, rename variable for clarity. Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- .../parallel_orchestrator_reviewer.py | 95 +++++++++++++++---- .../main/ipc-handlers/github/pr-handlers.ts | 8 ++ .../src/preload/api/modules/github-api.ts | 4 + .../github-prs/components/FindingItem.tsx | 23 ++++- .../github-prs/components/FindingsSummary.tsx | 8 +- .../github-prs/components/ReviewFindings.tsx | 92 +++++++++++++++--- .../github-prs/hooks/useFindingSelection.ts | 22 +++-- .../src/shared/i18n/locales/en/common.json | 4 + .../src/shared/i18n/locales/fr/common.json | 4 + 9 files changed, 220 insertions(+), 40 deletions(-) diff --git a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py index d17beea3..ce73464a 100644 --- a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py +++ b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py @@ -1289,12 +1289,30 @@ The SDK will run invoked agents in parallel automatically. 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 = [] + dismissed_findings = [] + for f in validated_findings: + if f.validation_status == "dismissed_false_positive": + dismissed_findings.append(f) + else: + active_findings.append(f) + safe_print( + f"[ParallelOrchestrator] Final: {len(active_findings)} active, " + f"{len(dismissed_findings)} disputed by validator", + flush=True, + ) logger.info( - f"[ParallelOrchestrator] Review complete: {len(unique_findings)} findings" + f"[PRReview] Final findings: {len(active_findings)} active, " + f"{len(dismissed_findings)} disputed" + ) + + # All findings (active + dismissed) go in the result for UI display + all_review_findings = validated_findings + logger.info( + f"[ParallelOrchestrator] Review complete: {len(all_review_findings)} findings " + f"({len(active_findings)} active, {len(dismissed_findings)} disputed)" ) # Fetch CI status for verdict consideration @@ -1304,9 +1322,9 @@ The SDK will run invoked agents in parallel automatically. 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, @@ -1317,7 +1335,7 @@ The SDK will run invoked agents in parallel automatically. verdict=verdict, verdict_reasoning=verdict_reasoning, blockers=blockers, - findings=unique_findings, + findings=all_review_findings, agents_invoked=agents_invoked, ) @@ -1362,7 +1380,7 @@ The SDK will run invoked agents in parallel automatically. pr_number=context.pr_number, repo=self.config.repo, success=True, - findings=unique_findings, + findings=all_review_findings, summary=summary, overall_status=overall_status, verdict=verdict, @@ -1937,12 +1955,38 @@ For EACH finding above: 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]}" + ) elif validation.validation_status == "needs_human_review": # Keep but flag @@ -2127,11 +2171,16 @@ For EACH finding above: sev = f.severity.value emoji = severity_emoji.get(sev, "⚪") + is_disputed = f.validation_status == "dismissed_false_positive" + # Finding header with location line_range = f"L{f.line}" if f.end_line and f.end_line != f.line: line_range = f"L{f.line}-L{f.end_line}" - lines.append(f"#### {emoji} [{sev.upper()}] {f.title}") + if is_disputed: + lines.append(f"#### ⚪ [DISPUTED] ~~{f.title}~~") + else: + lines.append(f"#### {emoji} [{sev.upper()}] {f.title}") lines.append(f"**File:** `{f.file}` ({line_range})") # Cross-validation badge @@ -2161,6 +2210,7 @@ For EACH finding above: status_label = { "confirmed_valid": "Confirmed", "needs_human_review": "Needs human review", + "dismissed_false_positive": "Disputed by validator", }.get(f.validation_status, f.validation_status) lines.append("") lines.append(f"**Validation:** {status_label}") @@ -2182,18 +2232,27 @@ For EACH finding above: lines.append("") - # Findings count summary + # Findings count summary (exclude dismissed from active count) + active_count = 0 + dismissed_count = 0 by_severity: dict[str, int] = {} for f in findings: + if f.validation_status == "dismissed_false_positive": + dismissed_count += 1 + continue + active_count += 1 sev = f.severity.value by_severity[sev] = by_severity.get(sev, 0) + 1 summary_parts = [] for sev in ["critical", "high", "medium", "low"]: if sev in by_severity: summary_parts.append(f"{by_severity[sev]} {sev}") - lines.append( - f"**Total:** {len(findings)} finding(s) ({', '.join(summary_parts)})" + count_text = ( + f"**Total:** {active_count} finding(s) ({', '.join(summary_parts)})" ) + if dismissed_count > 0: + count_text += f" + {dismissed_count} disputed" + lines.append(count_text) lines.append("") lines.append("---") diff --git a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts index 61200666..1f63b6c4 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -268,6 +268,10 @@ export interface PRReviewFinding { endLine?: number; suggestedFix?: string; fixable: boolean; + validationStatus?: "confirmed_valid" | "dismissed_false_positive" | "needs_human_review" | null; + validationExplanation?: string; + sourceAgents?: string[]; + crossValidated?: boolean; } /** @@ -1341,6 +1345,10 @@ function getReviewResult(project: Project, prNumber: number): PRReviewResult | n endLine: f.end_line, suggestedFix: f.suggested_fix, fixable: f.fixable ?? false, + validationStatus: f.validation_status ?? null, + validationExplanation: f.validation_explanation ?? undefined, + sourceAgents: f.source_agents ?? [], + crossValidated: f.cross_validated ?? false, })) ?? [], summary: data.summary ?? "", overallStatus: data.overall_status ?? "comment", diff --git a/apps/frontend/src/preload/api/modules/github-api.ts b/apps/frontend/src/preload/api/modules/github-api.ts index c27da235..bca2a8f1 100644 --- a/apps/frontend/src/preload/api/modules/github-api.ts +++ b/apps/frontend/src/preload/api/modules/github-api.ts @@ -376,6 +376,10 @@ export interface PRReviewFinding { endLine?: number; suggestedFix?: string; fixable: boolean; + validationStatus?: 'confirmed_valid' | 'dismissed_false_positive' | 'needs_human_review' | null; + validationExplanation?: string; + sourceAgents?: string[]; + crossValidated?: boolean; } /** diff --git a/apps/frontend/src/renderer/components/github-prs/components/FindingItem.tsx b/apps/frontend/src/renderer/components/github-prs/components/FindingItem.tsx index f3be2e42..47dcde99 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/FindingItem.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/FindingItem.tsx @@ -14,6 +14,7 @@ interface FindingItemProps { finding: PRReviewFinding; selected: boolean; posted?: boolean; + disputed?: boolean; onToggle: () => void; } @@ -33,7 +34,7 @@ function getCategoryTranslationKey(category: string): string { return categoryMap[category.toLowerCase()] || category; } -export function FindingItem({ finding, selected, posted = false, onToggle }: FindingItemProps) { +export function FindingItem({ finding, selected, posted = false, disputed = false, onToggle }: FindingItemProps) { const { t } = useTranslation('common'); const CategoryIcon = getCategoryIcon(finding.category); @@ -45,8 +46,9 @@ export function FindingItem({ finding, selected, posted = false, onToggle }: Fin
{/* Finding Header */} @@ -72,6 +74,16 @@ export function FindingItem({ finding, selected, posted = false, onToggle }: Fin {t('prReview.posted')} )} + {disputed && ( + + {t('prReview.disputed')} + + )} + {finding.crossValidated && finding.sourceAgents && finding.sourceAgents.length > 1 && ( + + {t('prReview.crossValidatedBy', { count: finding.sourceAgents.length })} + + )} {finding.title} @@ -79,6 +91,11 @@ export function FindingItem({ finding, selected, posted = false, onToggle }: Fin

{finding.description}

+ {disputed && finding.validationExplanation && ( +

+ {finding.validationExplanation} +

+ )}
{finding.file}:{finding.line} diff --git a/apps/frontend/src/renderer/components/github-prs/components/FindingsSummary.tsx b/apps/frontend/src/renderer/components/github-prs/components/FindingsSummary.tsx index 2259b35a..eb28a0e0 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/FindingsSummary.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/FindingsSummary.tsx @@ -9,9 +9,10 @@ import type { PRReviewFinding } from '../hooks/useGitHubPRs'; interface FindingsSummaryProps { findings: PRReviewFinding[]; selectedCount: number; + disputedCount?: number; } -export function FindingsSummary({ findings, selectedCount }: FindingsSummaryProps) { +export function FindingsSummary({ findings, selectedCount, disputedCount = 0 }: FindingsSummaryProps) { const { t } = useTranslation('common'); // Count findings by severity @@ -46,6 +47,11 @@ export function FindingsSummary({ findings, selectedCount }: FindingsSummaryProp {counts.low} {t('prReview.severity.low')} )} + {disputedCount > 0 && ( + + {disputedCount} {t('prReview.disputed')} + + )}
{t('prReview.selectedOfTotal', { selected: selectedCount, total: counts.total })} diff --git a/apps/frontend/src/renderer/components/github-prs/components/ReviewFindings.tsx b/apps/frontend/src/renderer/components/github-prs/components/ReviewFindings.tsx index cd5e3356..8a9bba40 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/ReviewFindings.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/ReviewFindings.tsx @@ -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,6 +51,7 @@ export function ReviewFindings({ const [expandedSections, setExpandedSections] = useState>( new Set(['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(() => @@ -54,10 +59,24 @@ export function ReviewFindings({ [findings, postedIds] ); + // Split unposted findings into active vs disputed (single pass) + 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]); + // Check if all findings are posted const allFindingsPosted = findings.length > 0 && unpostedFindings.length === 0; - // Group unposted findings by severity (only show findings that haven't been posted) + // Group ACTIVE unposted findings by severity (disputed go in their own section) const groupedFindings = useMemo(() => { const groups: Record = { critical: [], @@ -66,7 +85,7 @@ export function ReviewFindings({ low: [], }; - for (const finding of unpostedFindings) { + for (const finding of activeFindings) { const severity = finding.severity as SeverityGroup; if (groups[severity]) { groups[severity].push(finding); @@ -74,20 +93,20 @@ export function ReviewFindings({ } return groups; - }, [unpostedFindings]); + }, [activeFindings]); - // Count by severity (unposted findings only) + // Count by severity (active findings only) const counts = useMemo(() => ({ critical: groupedFindings.critical.length, high: groupedFindings.high.length, medium: groupedFindings.medium.length, low: groupedFindings.low.length, - total: unpostedFindings.length, + total: activeFindings.length, important: groupedFindings.critical.length + groupedFindings.high.length, posted: postedIds.size, - }), [groupedFindings, unpostedFindings.length, postedIds.size]); + }), [groupedFindings, activeFindings.length, postedIds.size]); - // Selection hooks - use unposted findings only + // Selection hooks - use ACTIVE unposted findings only (Select All excludes disputed) const { toggleFinding, selectAll, @@ -95,7 +114,7 @@ export function ReviewFindings({ selectImportant, toggleSeverityGroup, } = useFindingSelection({ - findings: unpostedFindings, + findings: activeFindings, selectedIds, onSelectionChange, groupedFindings, @@ -114,6 +133,12 @@ export function ReviewFindings({ }); }; + // Count only active findings that are selected (excludes disputed from count) + const selectedActiveCount = useMemo( + () => activeFindings.filter(f => selectedIds.has(f.id)).length, + [activeFindings, selectedIds] + ); + // When all findings have been posted, show a success message instead of the selection UI if (allFindingsPosted) { return ( @@ -131,10 +156,11 @@ export function ReviewFindings({ return (
- {/* Summary Stats Bar - show unposted findings only */} + {/* Summary Stats Bar - show active findings + disputed count */} {/* Quick Select Actions */} @@ -170,7 +196,7 @@ export function ReviewFindings({
- {/* Grouped Findings (unposted only) */} + {/* Grouped Findings (active only) */}
{SEVERITY_ORDER.map((severity) => { const group = groupedFindings[severity]; @@ -220,6 +246,48 @@ export function ReviewFindings({ })}
+ {/* Disputed Findings Section */} + {disputedFindings.length > 0 && ( +
+ {/* Disputed Header */} + + + {/* Disputed Content */} + {disputedExpanded && ( +
+

+ {t('prReview.disputedSectionHint')} +

+ {disputedFindings.map((finding) => ( + toggleFinding(finding.id)} + /> + ))} +
+ )} +
+ )} + {/* Empty State - no findings at all */} {findings.length === 0 && (
diff --git a/apps/frontend/src/renderer/components/github-prs/hooks/useFindingSelection.ts b/apps/frontend/src/renderer/components/github-prs/hooks/useFindingSelection.ts index 1b14eb0c..b06ea440 100644 --- a/apps/frontend/src/renderer/components/github-prs/hooks/useFindingSelection.ts +++ b/apps/frontend/src/renderer/components/github-prs/hooks/useFindingSelection.ts @@ -30,21 +30,31 @@ export function useFindingSelection({ onSelectionChange(next); }, [selectedIds, onSelectionChange]); - // Select all findings + // Select all findings (preserving any disputed selections not in active findings) const selectAll = useCallback(() => { - onSelectionChange(new Set(findings.map(f => f.id))); - }, [findings, onSelectionChange]); + const activeIds = new Set(findings.map(f => f.id)); + // Preserve selections for disputed findings (IDs not in active findings list) + for (const id of selectedIds) { + if (!findings.some(f => f.id === id)) activeIds.add(id); + } + onSelectionChange(activeIds); + }, [findings, selectedIds, onSelectionChange]); // Clear all selections const selectNone = useCallback(() => { onSelectionChange(new Set()); }, [onSelectionChange]); - // Select only critical and high severity findings + // Select only critical and high severity findings (preserving disputed selections) const selectImportant = useCallback(() => { const important = [...groupedFindings.critical, ...groupedFindings.high]; - onSelectionChange(new Set(important.map(f => f.id))); - }, [groupedFindings, onSelectionChange]); + const importantIds = new Set(important.map(f => f.id)); + // Preserve selections for disputed findings (IDs not in active findings list) + for (const id of selectedIds) { + if (!findings.some(f => f.id === id)) importantIds.add(id); + } + onSelectionChange(importantIds); + }, [groupedFindings, findings, selectedIds, onSelectionChange]); // Toggle entire severity group selection const toggleSeverityGroup = useCallback((severity: SeverityGroup) => { diff --git a/apps/frontend/src/shared/i18n/locales/en/common.json b/apps/frontend/src/shared/i18n/locales/en/common.json index 8078a129..f504232d 100644 --- a/apps/frontend/src/shared/i18n/locales/en/common.json +++ b/apps/frontend/src/shared/i18n/locales/en/common.json @@ -402,6 +402,10 @@ "verifyChanges": "Verify Changes", "verifyAnyway": "Verify", "runFollowupAnyway": "Run follow-up verification even though no files overlap", + "disputed": "Disputed", + "disputedByValidator": "Disputed by Validator ({{count}})", + "crossValidatedBy": "Confirmed by {{count}} agents", + "disputedSectionHint": "These findings were reported by specialists but disputed by the validator. You can still select and post them.", "logs": { "agentActivity": "Agent Activity", "showMore": "Show {{count}} more", diff --git a/apps/frontend/src/shared/i18n/locales/fr/common.json b/apps/frontend/src/shared/i18n/locales/fr/common.json index d2c452b7..b1ec2c6f 100644 --- a/apps/frontend/src/shared/i18n/locales/fr/common.json +++ b/apps/frontend/src/shared/i18n/locales/fr/common.json @@ -402,6 +402,10 @@ "blockedStatusMessageTitle": "## 🤖 Auto Claude PR Review", "blockedStatusMessageFooter": "*This review identified blockers that must be resolved before merge. Generated by Auto Claude.*", "failedPostBlockedStatus": "Échec de la publication du statut", + "disputed": "Contesté", + "disputedByValidator": "Contesté par le validateur ({{count}})", + "crossValidatedBy": "Confirmé par {{count}} agents", + "disputedSectionHint": "Ces résultats ont été signalés par les spécialistes mais contestés par le validateur. Vous pouvez toujours les sélectionner et les publier.", "logs": { "agentActivity": "Activité des agents", "showMore": "Afficher {{count}} de plus",