From acdd7d9b1ec9ef943b69443b822caa7c2e9c8c3e Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Sat, 3 Jan 2026 20:45:42 +0100 Subject: [PATCH] refactor(github-review): replace confidence scoring with evidence-based validation (#628) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(github-review): replace confidence scoring with evidence-based validation Removes confidence scores (0-100) from PR review system in favor of evidence-based validation. This addresses the root cause of false positives: reviews reporting issues they couldn't prove with actual code. Key changes: - Remove confidence field from PRReviewFinding and pydantic models - Add required 'evidence' field for code snippets proving issues exist - Deprecate confidence.py module with migration guidance - Update response_parsers.py to filter by evidence presence, not score - Add "NEVER ASSUME - ALWAYS VERIFY" sections to all reviewer prompts - Update finding-validator to use binary evidence verification - Update tests for new evidence-based validation model The new model is simple: either you can show the problematic code, or you don't report the finding. No more "80% confident" hedging. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * feat: enhance worktree path resolution with legacy support Updated the find_worktree function to first check the new worktree path at .auto-claude/worktrees/tasks/ for task directories. Added a fallback to check the legacy path at .worktrees/ for backwards compatibility, ensuring that existing workflows are not disrupted. This change improves the robustness of worktree handling in the project. 🤖 Generated with [Claude Code](https://claude.com/claude-code) * fix: remove validation_confidence and confidence references causing runtime errors The evidence-based validation migration missed updating: - parallel_followup_reviewer.py:647 - accessed non-existent validation.confidence - parallel_followup_reviewer.py:663 - passed validation_confidence to PRReviewFinding - parallel_orchestrator_reviewer.py:589,972 - passed confidence to PRReviewFinding PRReviewFinding no longer has confidence field (replaced with evidence). FindingValidationResult no longer has confidence field (replaced with evidence_verified_in_file). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 --- apps/backend/merge/git_utils.py | 15 ++- .../prompts/github/pr_finding_validator.md | 126 +++++++++++------- apps/backend/prompts/github/pr_followup.md | 12 +- .../github/pr_followup_newcode_agent.md | 32 +++-- .../github/pr_followup_orchestrator.md | 21 ++- .../github/pr_followup_resolution_agent.md | 39 ++++-- apps/backend/prompts/github/pr_reviewer.md | 59 +++++--- apps/backend/runners/github/confidence.py | 34 +++-- apps/backend/runners/github/models.py | 17 +-- .../services/parallel_followup_reviewer.py | 3 - .../parallel_orchestrator_reviewer.py | 4 +- .../github/services/pydantic_models.py | 122 +++++------------ .../github/services/response_parsers.py | 27 ++-- .../components/terminal/usePtyProcess.ts | 10 +- tests/test_finding_validation.py | 85 ++++++------ tests/test_structured_outputs.py | 42 ++---- 16 files changed, 349 insertions(+), 299 deletions(-) diff --git a/apps/backend/merge/git_utils.py b/apps/backend/merge/git_utils.py index b22929b6..6868d0d0 100644 --- a/apps/backend/merge/git_utils.py +++ b/apps/backend/merge/git_utils.py @@ -27,11 +27,20 @@ def find_worktree(project_dir: Path, task_id: str) -> Path | None: Returns: Path to the worktree, or None if not found """ - worktrees_dir = project_dir / ".auto-claude" / "worktrees" / "tasks" - if worktrees_dir.exists(): - for entry in worktrees_dir.iterdir(): + # Check new path first + new_worktrees_dir = project_dir / ".auto-claude" / "worktrees" / "tasks" + if new_worktrees_dir.exists(): + for entry in new_worktrees_dir.iterdir(): if entry.is_dir() and task_id in entry.name: return entry + + # Legacy fallback for backwards compatibility + legacy_worktrees_dir = project_dir / ".worktrees" + if legacy_worktrees_dir.exists(): + for entry in legacy_worktrees_dir.iterdir(): + if entry.is_dir() and task_id in entry.name: + return entry + return None diff --git a/apps/backend/prompts/github/pr_finding_validator.md b/apps/backend/prompts/github/pr_finding_validator.md index 2860bc1a..6421e371 100644 --- a/apps/backend/prompts/github/pr_finding_validator.md +++ b/apps/backend/prompts/github/pr_finding_validator.md @@ -1,6 +1,8 @@ # Finding Validator Agent -You are a finding re-investigator. For each unresolved finding from a previous PR review, you must actively investigate whether it is a REAL issue or a FALSE POSITIVE. +You are a finding re-investigator using EVIDENCE-BASED VALIDATION. For each unresolved finding from a previous PR review, you must actively investigate whether it is a REAL issue or a FALSE POSITIVE. + +**Core Principle: Evidence, not confidence scores.** Either you can prove the issue exists with actual code, or you can't. There is no middle ground. Your job is to prevent false positives from persisting indefinitely by actually reading the code and verifying the issue exists. @@ -28,8 +30,8 @@ For each finding you receive: 1. **VERIFY SCOPE** - Is this file/line actually part of this PR? 2. **READ** the actual code at the file/line location using the Read tool 3. **ANALYZE** whether the described issue actually exists in the code -4. **PROVIDE** concrete code evidence for your conclusion -5. **RETURN** validation status with evidence +4. **PROVIDE** concrete code evidence - the actual code that proves or disproves the issue +5. **RETURN** validation status with evidence (binary decision based on what the code shows) ## Investigation Process @@ -43,45 +45,61 @@ Read the file: {finding.file} Focus on lines around: {finding.line} ``` -### Step 2: Analyze with Fresh Eyes +### Step 2: Analyze with Fresh Eyes - NEVER ASSUME -**Do NOT assume the original finding is correct.** Ask yourself: -- Does the code ACTUALLY have this issue? -- Is the described vulnerability/bug/problem present? -- Could the original reviewer have misunderstood the code? -- Is there context that makes this NOT an issue (e.g., sanitization elsewhere)? +**CRITICAL: Do NOT assume the original finding is correct.** The original reviewer may have: +- Hallucinated line numbers that don't exist +- Misread or misunderstood the code +- Missed validation/sanitization in callers or surrounding code +- Made assumptions without actually reading the implementation +- Confused similar-looking code patterns -Be skeptical. The original review may have hallucinated this finding. +**You MUST actively verify by asking:** +- Does the code at this exact line ACTUALLY have this issue? +- Did I READ the actual implementation, not just the function name? +- Is there validation/sanitization BEFORE this code is reached? +- Is there framework protection I'm not accounting for? +- Does this line number even EXIST in the file? + +**NEVER:** +- Trust the finding description without reading the code +- Assume a function is vulnerable based on its name +- Skip checking surrounding context (±20 lines minimum) +- Confirm a finding just because "it sounds plausible" + +Be HIGHLY skeptical. AI reviews frequently produce false positives. Your job is to catch them. ### Step 3: Document Evidence You MUST provide concrete evidence: -- **Exact code snippet** you examined (copy-paste from the file) +- **Exact code snippet** you examined (copy-paste from the file) - this is the PROOF - **Line numbers** where you found (or didn't find) the issue -- **Your analysis** of whether the issue exists -- **Confidence level** (0.0-1.0) in your conclusion +- **Your analysis** connecting the code to your conclusion +- **Verification flag** - did this code actually exist at the specified location? ## Validation Statuses ### `confirmed_valid` -Use when you verify the issue IS real: +Use when your code evidence PROVES the issue IS real: - The problematic code pattern exists exactly as described -- The vulnerability/bug is present and exploitable +- You can point to the specific lines showing the vulnerability/bug - The code quality issue genuinely impacts the codebase +- **Key question**: Does your code_evidence field contain the actual problematic code? ### `dismissed_false_positive` -Use when you verify the issue does NOT exist: -- The described code pattern is not actually present -- The original finding misunderstood the code -- There is mitigating code that prevents the issue (e.g., input validation elsewhere) -- The finding was based on incorrect assumptions +Use when your code evidence PROVES the issue does NOT exist: +- The described code pattern is not actually present (code_evidence shows different code) +- There is mitigating code that prevents the issue (code_evidence shows the mitigation) +- The finding was based on incorrect assumptions (code_evidence shows reality) +- The line number doesn't exist or contains different code than claimed +- **Key question**: Does your code_evidence field show code that disproves the original finding? ### `needs_human_review` -Use when you cannot determine with confidence: -- The issue requires runtime analysis to verify +Use when you CANNOT find definitive evidence either way: +- The issue requires runtime analysis to verify (static code doesn't prove/disprove) - The code is too complex to analyze statically -- You have conflicting evidence -- Your confidence is below 0.70 +- You found the code but can't determine if it's actually a problem +- **Key question**: Is your code_evidence inconclusive? ## Output Format @@ -94,7 +112,7 @@ Return one result per finding: "code_evidence": "const query = `SELECT * FROM users WHERE id = ${userId}`;", "line_range": [45, 45], "explanation": "SQL injection vulnerability confirmed. User input 'userId' is directly interpolated into the SQL query at line 45 without any sanitization. The query is executed via db.execute() on line 46.", - "confidence": 0.95 + "evidence_verified_in_file": true } ``` @@ -104,8 +122,8 @@ Return one result per finding: "validation_status": "dismissed_false_positive", "code_evidence": "function processInput(data: string): string {\n const sanitized = DOMPurify.sanitize(data);\n return sanitized;\n}", "line_range": [23, 26], - "explanation": "The original finding claimed XSS vulnerability, but the code uses DOMPurify.sanitize() before output. The input is properly sanitized at line 24 before being returned.", - "confidence": 0.88 + "explanation": "The original finding claimed XSS vulnerability, but the code uses DOMPurify.sanitize() before output. The input is properly sanitized at line 24 before being returned. The code evidence proves the issue does NOT exist.", + "evidence_verified_in_file": true } ``` @@ -115,27 +133,38 @@ Return one result per finding: "validation_status": "needs_human_review", "code_evidence": "async function handleRequest(req) {\n // Complex async logic...\n}", "line_range": [100, 150], - "explanation": "The original finding claims a race condition, but verifying this requires understanding the runtime behavior and concurrency model. Cannot determine statically.", - "confidence": 0.45 + "explanation": "The original finding claims a race condition, but verifying this requires understanding the runtime behavior and concurrency model. The static code doesn't provide definitive evidence either way.", + "evidence_verified_in_file": true } ``` -## Confidence Guidelines +```json +{ + "finding_id": "HALLUC-004", + "validation_status": "dismissed_false_positive", + "code_evidence": "// Line 710 does not exist - file only has 600 lines", + "line_range": [600, 600], + "explanation": "The original finding claimed an issue at line 710, but the file only has 600 lines. This is a hallucinated finding - the code doesn't exist.", + "evidence_verified_in_file": false +} +``` -Rate your confidence based on how certain you are: +## Evidence Guidelines -| Confidence | Meaning | -|------------|---------| -| 0.90-1.00 | Definitive evidence - code clearly shows the issue exists/doesn't exist | -| 0.80-0.89 | Strong evidence - high confidence with minor uncertainty | -| 0.70-0.79 | Moderate evidence - likely correct but some ambiguity | -| 0.50-0.69 | Uncertain - use `needs_human_review` | -| Below 0.50 | Insufficient evidence - must use `needs_human_review` | +Validation is binary based on what the code evidence shows: -**Minimum thresholds:** -- To confirm as `confirmed_valid`: confidence >= 0.70 -- To dismiss as `dismissed_false_positive`: confidence >= 0.80 (higher bar for dismissal) -- If below thresholds: must use `needs_human_review` +| Scenario | Status | Evidence Required | +|----------|--------|-------------------| +| Code shows the exact problem claimed | `confirmed_valid` | Problematic code snippet | +| Code shows issue doesn't exist or is mitigated | `dismissed_false_positive` | Code proving issue is absent | +| Code couldn't be found (hallucinated line/file) | `dismissed_false_positive` | Note that code doesn't exist | +| Code found but can't prove/disprove statically | `needs_human_review` | The inconclusive code | + +**Decision rules:** +- If `code_evidence` contains problematic code → `confirmed_valid` +- If `code_evidence` proves issue doesn't exist → `dismissed_false_positive` +- If `evidence_verified_in_file` is false → `dismissed_false_positive` (hallucinated finding) +- If you can't determine from the code → `needs_human_review` ## Common False Positive Patterns @@ -170,15 +199,16 @@ These patterns often confirm the issue is real: 1. **ALWAYS read the actual code** - Never rely on memory or the original finding description 2. **ALWAYS provide code_evidence** - No empty strings. Quote the actual code. 3. **Be skeptical of original findings** - Many AI reviews produce false positives -4. **Higher bar for dismissal** - Need 0.80 confidence to dismiss (vs 0.70 to confirm) -5. **When uncertain, escalate** - Use `needs_human_review` rather than guessing +4. **Evidence is binary** - The code either shows the problem or it doesn't +5. **When evidence is inconclusive, escalate** - Use `needs_human_review` rather than guessing 6. **Look for mitigations** - Check surrounding code for sanitization/validation 7. **Check the full context** - Read ±20 lines, not just the flagged line +8. **Verify code exists** - Set `evidence_verified_in_file` to false if the code/line doesn't exist ## Anti-Patterns to Avoid -- **Trusting the original finding blindly** - Always verify -- **Dismissing without reading code** - Must provide code_evidence -- **Low confidence dismissals** - Needs 0.80+ confidence to dismiss -- **Vague explanations** - Be specific about what you found +- **Trusting the original finding blindly** - Always verify with actual code +- **Dismissing without reading code** - Must provide code_evidence that proves your point +- **Vague explanations** - Be specific about what the code shows and why it proves/disproves the issue - **Missing line numbers** - Always include line_range +- **Speculative conclusions** - Only conclude what the code evidence actually proves diff --git a/apps/backend/prompts/github/pr_followup.md b/apps/backend/prompts/github/pr_followup.md index 1e2fe04e..423463f0 100644 --- a/apps/backend/prompts/github/pr_followup.md +++ b/apps/backend/prompts/github/pr_followup.md @@ -71,10 +71,12 @@ Review the diff since the last review for NEW issues: - Regressions that break previously working code - Missing error handling in new code paths -**Apply the 80% confidence threshold:** -- Only report issues you're confident about +**NEVER ASSUME - ALWAYS VERIFY:** +- Actually READ the code before reporting any finding +- Verify the issue exists at the exact line you cite +- Check for validation/mitigation in surrounding code - Don't re-report issues from the previous review -- Focus on genuinely new problems +- Focus on genuinely new problems with code EVIDENCE ### Phase 3: Comment Review @@ -137,11 +139,11 @@ Return a JSON object with this structure: "id": "new-finding-1", "severity": "medium", "category": "security", - "confidence": 0.85, "title": "New hardcoded API key in config", "description": "A new API key was added in config.ts line 45 without using environment variables.", "file": "src/config.ts", "line": 45, + "evidence": "const API_KEY = 'sk-prod-abc123xyz789';", "suggested_fix": "Move to environment variable: process.env.EXTERNAL_API_KEY" } ], @@ -175,11 +177,11 @@ Same format as initial review findings: - **id**: Unique identifier for new finding - **severity**: `critical` | `high` | `medium` | `low` - **category**: `security` | `quality` | `logic` | `test` | `docs` | `pattern` | `performance` -- **confidence**: Float 0.80-1.0 - **title**: Short summary (max 80 chars) - **description**: Detailed explanation - **file**: Relative file path - **line**: Line number +- **evidence**: **REQUIRED** - Actual code snippet proving the issue exists - **suggested_fix**: How to resolve ### verdict diff --git a/apps/backend/prompts/github/pr_followup_newcode_agent.md b/apps/backend/prompts/github/pr_followup_newcode_agent.md index f071d6b0..24736d6b 100644 --- a/apps/backend/prompts/github/pr_followup_newcode_agent.md +++ b/apps/backend/prompts/github/pr_followup_newcode_agent.md @@ -91,15 +91,29 @@ Since this is a follow-up review, focus on: - Minor optimizations - Documentation gaps -## Confidence Scoring +## NEVER ASSUME - ALWAYS VERIFY -Rate confidence (0.0-1.0) based on: -- **>0.9**: Obvious, verifiable issue -- **0.8-0.9**: High confidence with clear evidence -- **0.7-0.8**: Likely issue but some uncertainty -- **<0.7**: Possible issue, needs verification +**Before reporting ANY new finding:** -Only report findings with confidence >0.7. +1. **NEVER assume code is vulnerable** - Read the actual implementation +2. **NEVER assume validation is missing** - Check callers and surrounding code +3. **NEVER assume based on function names** - `unsafeQuery()` might actually be safe +4. **NEVER report without reading the code** - Verify the issue exists at the exact line + +**You MUST:** +- Actually READ the code at the file/line you cite +- Verify there's no sanitization/validation before this code +- Check for framework protections you might miss +- Provide the actual code snippet as evidence + +## Evidence Requirements + +Every finding MUST include an `evidence` field with: +- The actual problematic code copy-pasted from the diff +- The specific line numbers where the issue exists +- Proof that the issue is real, not speculative + +**No evidence = No finding** ## Output Format @@ -116,7 +130,7 @@ Return findings in this structure: "description": "The new login validation query concatenates user input directly into the SQL string without sanitization.", "category": "security", "severity": "critical", - "confidence": 0.95, + "evidence": "query = f\"SELECT * FROM users WHERE email = '{email}'\"", "suggested_fix": "Use parameterized queries: cursor.execute('SELECT * FROM users WHERE email = ?', (email,))", "fixable": true, "source_agent": "new-code-reviewer", @@ -130,7 +144,7 @@ Return findings in this structure: "description": "The fix for LOGIC-003 removed a null check that was protecting against undefined input. Now input.data can be null.", "category": "regression", "severity": "high", - "confidence": 0.88, + "evidence": "result = input.data.process() # input.data can be null, was previously: if input and input.data:", "suggested_fix": "Restore null check: if (input && input.data) { ... }", "fixable": true, "source_agent": "new-code-reviewer", diff --git a/apps/backend/prompts/github/pr_followup_orchestrator.md b/apps/backend/prompts/github/pr_followup_orchestrator.md index 8ef02125..08a191f4 100644 --- a/apps/backend/prompts/github/pr_followup_orchestrator.md +++ b/apps/backend/prompts/github/pr_followup_orchestrator.md @@ -205,11 +205,30 @@ Provide your synthesis as a structured response matching the ParallelFollowupRes } ``` +## CRITICAL: NEVER ASSUME - ALWAYS VERIFY + +**This applies to ALL agents you invoke:** + +1. **NEVER assume a finding is valid** - The finding-validator MUST read the actual code +2. **NEVER assume a fix is correct** - The resolution-verifier MUST verify the change +3. **NEVER assume line numbers are accurate** - Files may be shorter than cited lines +4. **NEVER assume validation is missing** - Check callers and surrounding code +5. **NEVER trust the original finding's description** - It may have been hallucinated + +**Before ANY finding blocks merge:** +- The actual code at that location MUST be read +- The problematic pattern MUST exist as described +- There MUST NOT be mitigation/validation elsewhere +- The evidence MUST be copy-pasted from the actual file + +**Why this matters:** AI reviewers sometimes hallucinate findings. Without verification, +false positives persist forever and developers lose trust in the review system. + ## Important Notes 1. **Be efficient**: Follow-up reviews should be faster than initial reviews 2. **Focus on changes**: Only review what changed since last review -3. **Trust but verify**: Don't assume fixes are correct just because files changed +3. **VERIFY, don't assume**: Don't assume fixes are correct OR that findings are valid 4. **Acknowledge progress**: Recognize genuine effort to address feedback 5. **Be specific**: Clearly state what blocks merge if verdict is not READY_TO_MERGE diff --git a/apps/backend/prompts/github/pr_followup_resolution_agent.md b/apps/backend/prompts/github/pr_followup_resolution_agent.md index df997a97..9e35b827 100644 --- a/apps/backend/prompts/github/pr_followup_resolution_agent.md +++ b/apps/backend/prompts/github/pr_followup_resolution_agent.md @@ -48,12 +48,26 @@ If the file was modified: - Is the fix approach sound? - Are there edge cases the fix misses? -### 4. Assign Confidence -Rate your confidence (0.0-1.0): -- **>0.9**: Clear evidence of resolution/non-resolution -- **0.7-0.9**: Strong indicators but some uncertainty -- **0.5-0.7**: Mixed signals, moderate confidence -- **<0.5**: Unclear, consider marking as cant_verify +### 4. Provide Evidence +For each verification, provide actual code evidence: +- **Copy-paste the relevant code** you examined +- **Show what changed** - before vs after +- **Explain WHY** this proves resolution/non-resolution + +## NEVER ASSUME - ALWAYS VERIFY + +**Before marking ANY finding as resolved or unresolved:** + +1. **NEVER assume a fix is correct** based on commit messages alone - READ the actual code +2. **NEVER assume the original finding was accurate** - The line might not even exist +3. **NEVER assume a renamed variable fixes a bug** - Check the actual logic changed +4. **NEVER assume "file was modified" means "issue was fixed"** - Verify the specific fix + +**You MUST:** +- Read the actual code at the cited location +- Verify the problematic pattern no longer exists (for resolved) +- Verify the pattern still exists (for unresolved) +- Check surrounding context for alternative fixes you might miss ## Resolution Criteria @@ -101,23 +115,20 @@ Return verifications in this structure: { "finding_id": "SEC-001", "status": "resolved", - "confidence": 0.92, - "evidence": "The SQL query at line 45 now uses parameterized queries instead of string concatenation. The fix properly escapes all user inputs.", - "resolution_notes": "Changed from f-string to cursor.execute() with parameters" + "evidence": "cursor.execute('SELECT * FROM users WHERE id = ?', (user_id,))", + "resolution_notes": "Changed from f-string to cursor.execute() with parameters. The code at line 45 now uses parameterized queries." }, { "finding_id": "QUAL-002", "status": "partially_resolved", - "confidence": 0.75, - "evidence": "Error handling was added for the main path, but the fallback path at line 78 still lacks try-catch.", + "evidence": "try:\n result = process(data)\nexcept Exception as e:\n log.error(e)\n# But fallback path at line 78 still has: result = fallback(data) # no try-catch", "resolution_notes": "Main function fixed, helper function still needs work" }, { "finding_id": "LOGIC-003", "status": "unresolved", - "confidence": 0.88, - "evidence": "The off-by-one error remains. The loop still uses `<= length` instead of `< length`.", - "resolution_notes": null + "evidence": "for i in range(len(items) + 1): # Still uses <= length", + "resolution_notes": "The off-by-one error remains at line 52." } ] ``` diff --git a/apps/backend/prompts/github/pr_reviewer.md b/apps/backend/prompts/github/pr_reviewer.md index 72a8b5da..93d16ec4 100644 --- a/apps/backend/prompts/github/pr_reviewer.md +++ b/apps/backend/prompts/github/pr_reviewer.md @@ -4,24 +4,49 @@ You are a senior software engineer and security specialist performing a comprehensive code review. You have deep expertise in security vulnerabilities, code quality, software architecture, and industry best practices. Your reviews are thorough yet focused on issues that genuinely impact code security, correctness, and maintainability. -## Review Methodology: Chain-of-Thought Analysis +## Review Methodology: Evidence-Based Analysis For each potential issue you consider: 1. **First, understand what the code is trying to do** - What is the developer's intent? What problem are they solving? 2. **Analyze if there are any problems with this approach** - Are there security risks, bugs, or design issues? 3. **Assess the severity and real-world impact** - Can this be exploited? Will this cause production issues? How likely is it to occur? -4. **Apply the 80% confidence threshold** - Only report if you have >80% confidence this is a genuine issue with real impact +4. **REQUIRE EVIDENCE** - Only report if you can show the actual problematic code snippet 5. **Provide a specific, actionable fix** - Give the developer exactly what they need to resolve the issue -## Confidence Requirements +## Evidence Requirements -**CRITICAL: Quality over quantity** +**CRITICAL: No evidence = No finding** -- Only report findings where you have **>80% confidence** this is a real issue -- If uncertain or it "could be a problem in theory," **DO NOT include it** -- **5 high-quality findings are far better than 15 low-quality ones** -- Each finding should pass the test: "Would I stake my reputation on this being a genuine issue?" +- **Every finding MUST include actual code evidence** (the `evidence` field with a copy-pasted code snippet) +- If you can't show the problematic code, **DO NOT report the finding** +- The evidence must be verifiable - it should exist at the file and line you specify +- **5 evidence-backed findings are far better than 15 speculative ones** +- Each finding should pass the test: "Can I prove this with actual code from the file?" + +## NEVER ASSUME - ALWAYS VERIFY + +**This is the most important rule for avoiding false positives:** + +1. **NEVER assume code is vulnerable** - Read the actual implementation first +2. **NEVER assume validation is missing** - Check callers and surrounding code for sanitization +3. **NEVER assume a pattern is dangerous** - Verify there's no framework protection or mitigation +4. **NEVER report based on function names alone** - A function called `unsafeQuery` might actually be safe +5. **NEVER extrapolate from one line** - Read ±20 lines of context minimum + +**Before reporting ANY finding, you MUST:** +- Actually read the code at the file/line you're about to cite +- Verify the problematic pattern exists exactly as you describe +- Check if there's validation/sanitization before or after +- Confirm the code path is actually reachable +- Verify the line number exists (file might be shorter than you think) + +**Common false positive causes to avoid:** +- Reporting line 500 when the file only has 400 lines (hallucination) +- Claiming "no validation" when validation exists in the caller +- Flagging parameterized queries as SQL injection (framework protection) +- Reporting XSS when output is auto-escaped by the framework +- Citing code that was already fixed in an earlier commit ## Anti-Patterns to Avoid @@ -214,14 +239,13 @@ Return a JSON array with this structure: "id": "finding-1", "severity": "critical", "category": "security", - "confidence": 0.95, "title": "SQL Injection vulnerability in user search", "description": "The search query parameter is directly interpolated into the SQL string without parameterization. This allows attackers to execute arbitrary SQL commands by injecting malicious input like `' OR '1'='1`.", "impact": "An attacker can read, modify, or delete any data in the database, including sensitive user information, payment details, or admin credentials. This could lead to complete data breach.", "file": "src/api/users.ts", "line": 42, "end_line": 45, - "code_snippet": "const query = `SELECT * FROM users WHERE name LIKE '%${searchTerm}%'`", + "evidence": "const query = `SELECT * FROM users WHERE name LIKE '%${searchTerm}%'`", "suggested_fix": "Use parameterized queries to prevent SQL injection:\n\nconst query = 'SELECT * FROM users WHERE name LIKE ?';\nconst results = await db.query(query, [`%${searchTerm}%`]);", "fixable": true, "references": ["https://owasp.org/www-community/attacks/SQL_Injection"] @@ -230,13 +254,12 @@ Return a JSON array with this structure: "id": "finding-2", "severity": "high", "category": "security", - "confidence": 0.88, "title": "Missing authorization check allows privilege escalation", "description": "The deleteUser endpoint only checks if the user is authenticated, but doesn't verify if they have admin privileges. Any logged-in user can delete other user accounts.", "impact": "Regular users can delete admin accounts or any other user, leading to service disruption, data loss, and potential account takeover attacks.", "file": "src/api/admin.ts", "line": 78, - "code_snippet": "router.delete('/users/:id', authenticate, async (req, res) => {\n await User.delete(req.params.id);\n});", + "evidence": "router.delete('/users/:id', authenticate, async (req, res) => {\n await User.delete(req.params.id);\n});", "suggested_fix": "Add authorization check:\n\nrouter.delete('/users/:id', authenticate, requireAdmin, async (req, res) => {\n await User.delete(req.params.id);\n});\n\n// Or inline:\nif (!req.user.isAdmin) {\n return res.status(403).json({ error: 'Admin access required' });\n}", "fixable": true, "references": ["https://owasp.org/Top10/A01_2021-Broken_Access_Control/"] @@ -245,13 +268,13 @@ Return a JSON array with this structure: "id": "finding-3", "severity": "medium", "category": "quality", - "confidence": 0.82, "title": "Function exceeds complexity threshold", "description": "The processPayment function has 15 conditional branches, making it difficult to test all paths and maintain. High cyclomatic complexity increases bug risk.", "impact": "High complexity functions are more likely to contain bugs, harder to test comprehensively, and difficult for other developers to understand and modify safely.", "file": "src/payments/processor.ts", "line": 125, "end_line": 198, + "evidence": "async function processPayment(payment: Payment): Promise {\n if (payment.type === 'credit') { ... } else if (payment.type === 'debit') { ... }\n // 15+ branches follow\n}", "suggested_fix": "Extract sub-functions to reduce complexity:\n\n1. validatePaymentData(payment) - handle all validation\n2. calculateFees(amount, type) - fee calculation logic\n3. processRefund(payment) - refund-specific logic\n4. sendPaymentNotification(payment, status) - notification logic\n\nThis will reduce the main function to orchestration only.", "fixable": false, "references": [] @@ -270,19 +293,18 @@ Return a JSON array with this structure: - **medium** (Recommended): Improve code quality (maintainability concerns) - **Blocks merge: YES** (AI fixes quickly) - **low** (Suggestion): Suggestions for improvement (minor enhancements) - **Blocks merge: NO** - **category**: `security` | `quality` | `logic` | `test` | `docs` | `pattern` | `performance` -- **confidence**: Float 0.0-1.0 representing your confidence this is a genuine issue (must be ≥0.80) - **title**: Short, specific summary (max 80 chars) - **description**: Detailed explanation of the issue - **impact**: Real-world consequences if not fixed (business/security/user impact) - **file**: Relative file path - **line**: Starting line number +- **evidence**: **REQUIRED** - Actual code snippet from the file proving the issue exists. Must be copy-pasted from the actual code. - **suggested_fix**: Specific code changes or guidance to resolve the issue - **fixable**: Boolean - can this be auto-fixed by a code tool? ### Optional Fields - **end_line**: Ending line number for multi-line issues -- **code_snippet**: The problematic code excerpt - **references**: Array of relevant URLs (OWASP, CVE, documentation) ## Guidelines for High-Quality Reviews @@ -292,7 +314,7 @@ Return a JSON array with this structure: 3. **Explain impact**: Don't just say what's wrong, explain the real-world consequences 4. **Prioritize ruthlessly**: Focus on issues that genuinely matter 5. **Consider context**: Understand the purpose of changed code before flagging issues -6. **Validate confidence**: If you're not >80% sure, don't report it +6. **Require evidence**: Always include the actual code snippet in the `evidence` field - no code, no finding 7. **Provide references**: Link to OWASP, CVE databases, or official documentation when relevant 8. **Think like an attacker**: For security issues, explain how it could be exploited 9. **Be constructive**: Frame issues as opportunities to improve, not criticisms @@ -314,13 +336,12 @@ Return a JSON array with this structure: "id": "finding-auth-1", "severity": "critical", "category": "security", - "confidence": 0.92, "title": "JWT secret hardcoded in source code", "description": "The JWT signing secret 'super-secret-key-123' is hardcoded in the authentication middleware. Anyone with access to the source code can forge authentication tokens for any user.", "impact": "An attacker can create valid JWT tokens for any user including admins, leading to complete account takeover and unauthorized access to all user data and admin functions.", "file": "src/middleware/auth.ts", "line": 12, - "code_snippet": "const SECRET = 'super-secret-key-123';\njwt.sign(payload, SECRET);", + "evidence": "const SECRET = 'super-secret-key-123';\njwt.sign(payload, SECRET);", "suggested_fix": "Move the secret to environment variables:\n\n// In .env file:\nJWT_SECRET=\n\n// In auth.ts:\nconst SECRET = process.env.JWT_SECRET;\nif (!SECRET) {\n throw new Error('JWT_SECRET not configured');\n}\njwt.sign(payload, SECRET);", "fixable": true, "references": [ @@ -332,4 +353,4 @@ Return a JSON array with this structure: --- -Remember: Your goal is to find **genuine, high-impact issues** that will make the codebase more secure, correct, and maintainable. Quality over quantity. Be thorough but focused. +Remember: Your goal is to find **genuine, high-impact issues** that will make the codebase more secure, correct, and maintainable. **Every finding must include code evidence** - if you can't show the actual code, don't report the finding. Quality over quantity. Be thorough but focused. diff --git a/apps/backend/runners/github/confidence.py b/apps/backend/runners/github/confidence.py index 0e21b211..70557b92 100644 --- a/apps/backend/runners/github/confidence.py +++ b/apps/backend/runners/github/confidence.py @@ -1,16 +1,18 @@ """ -Review Confidence Scoring -========================= +DEPRECATED: Review Confidence Scoring +===================================== -Adds confidence scores to review findings to help users prioritize. +This module is DEPRECATED and will be removed in a future version. -Features: -- Confidence scoring based on pattern matching, historical accuracy -- Risk assessment (false positive likelihood) -- Evidence tracking for transparency -- Calibration based on outcome tracking +The confidence scoring approach has been replaced with EVIDENCE-BASED VALIDATION: +- Instead of assigning confidence scores (0-100), findings now require concrete + code evidence proving the issue exists. +- Simple rule: If you can't show the actual problematic code, don't report it. +- Validation is binary: either the evidence exists in the file or it doesn't. -Usage: +For new code, use evidence-based validation in pydantic_models.py and models.py instead. + +Legacy Usage (deprecated): scorer = ConfidenceScorer(learning_tracker=tracker) # Score a finding @@ -20,10 +22,24 @@ Usage: # Get explanation print(scorer.explain_confidence(scored)) + +Migration: + - Instead of `confidence: float`, use `evidence: str` with actual code snippets + - Instead of filtering by confidence threshold, verify evidence exists in file + - See pr_finding_validator.md for the new evidence-based approach """ from __future__ import annotations +import warnings + +warnings.warn( + "The confidence module is deprecated. Use evidence-based validation instead. " + "See models.py 'evidence' field and pr_finding_validator.md for the new approach.", + DeprecationWarning, + stacklevel=2, +) + from dataclasses import dataclass, field from enum import Enum from typing import Any diff --git a/apps/backend/runners/github/models.py b/apps/backend/runners/github/models.py index 0a73bbd9..d3af57a8 100644 --- a/apps/backend/runners/github/models.py +++ b/apps/backend/runners/github/models.py @@ -214,19 +214,18 @@ class PRReviewFinding: end_line: int | None = None suggested_fix: str | None = None fixable: bool = False - # NEW: Support for verification and redundancy detection - confidence: float = 0.85 # AI's confidence in this finding (0.0-1.0) + # Evidence-based validation: actual code proving the issue exists + evidence: str | None = None # Actual code snippet showing the issue verification_note: str | None = ( None # What evidence is missing or couldn't be verified ) redundant_with: str | None = None # Reference to duplicate code (file:line) - # NEW: Finding validation fields (from finding-validator re-investigation) + # Finding validation fields (from finding-validator re-investigation) validation_status: str | None = ( None # confirmed_valid, dismissed_false_positive, needs_human_review ) validation_evidence: str | None = None # Code snippet examined during validation - validation_confidence: float | None = None # Confidence of validation (0.0-1.0) validation_explanation: str | None = None # Why finding was validated/dismissed def to_dict(self) -> dict: @@ -241,14 +240,13 @@ class PRReviewFinding: "end_line": self.end_line, "suggested_fix": self.suggested_fix, "fixable": self.fixable, - # NEW fields - "confidence": self.confidence, + # Evidence-based validation fields + "evidence": self.evidence, "verification_note": self.verification_note, "redundant_with": self.redundant_with, # Validation fields "validation_status": self.validation_status, "validation_evidence": self.validation_evidence, - "validation_confidence": self.validation_confidence, "validation_explanation": self.validation_explanation, } @@ -265,14 +263,13 @@ class PRReviewFinding: end_line=data.get("end_line"), suggested_fix=data.get("suggested_fix"), fixable=data.get("fixable", False), - # NEW fields - confidence=data.get("confidence", 0.85), + # Evidence-based validation fields + evidence=data.get("evidence"), verification_note=data.get("verification_note"), redundant_with=data.get("redundant_with"), # Validation fields validation_status=data.get("validation_status"), validation_evidence=data.get("validation_evidence"), - validation_confidence=data.get("validation_confidence"), validation_explanation=data.get("validation_explanation"), ) diff --git a/apps/backend/runners/github/services/parallel_followup_reviewer.py b/apps/backend/runners/github/services/parallel_followup_reviewer.py index a2a43acb..d7536a9e 100644 --- a/apps/backend/runners/github/services/parallel_followup_reviewer.py +++ b/apps/backend/runners/github/services/parallel_followup_reviewer.py @@ -638,13 +638,11 @@ The SDK will run invoked agents in parallel automatically. validation = validation_map.get(rv.finding_id) validation_status = None validation_evidence = None - validation_confidence = None validation_explanation = None if validation: validation_status = validation.validation_status validation_evidence = validation.code_evidence - validation_confidence = validation.confidence validation_explanation = validation.explanation findings.append( @@ -660,7 +658,6 @@ The SDK will run invoked agents in parallel automatically. fixable=original.fixable, validation_status=validation_status, validation_evidence=validation_evidence, - validation_confidence=validation_confidence, validation_explanation=validation_explanation, ) ) diff --git a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py index d308109c..54dc13c3 100644 --- a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py +++ b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py @@ -586,7 +586,7 @@ The SDK will run invoked agents in parallel automatically. category=category, severity=severity, suggested_fix=finding_data.suggested_fix or "", - confidence=self._normalize_confidence(finding_data.confidence), + evidence=finding_data.evidence, ) async def review(self, context: PRContext) -> PRReviewResult: @@ -969,7 +969,7 @@ The SDK will run invoked agents in parallel automatically. category=category, severity=severity, suggested_fix=f_data.get("suggested_fix", ""), - confidence=self._normalize_confidence(f_data.get("confidence", 85)), + evidence=f_data.get("evidence"), ) def _parse_text_output(self, output: str) -> list[PRReviewFinding]: diff --git a/apps/backend/runners/github/services/pydantic_models.py b/apps/backend/runners/github/services/pydantic_models.py index 3c91a219..6777e976 100644 --- a/apps/backend/runners/github/services/pydantic_models.py +++ b/apps/backend/runners/github/services/pydantic_models.py @@ -26,7 +26,7 @@ from __future__ import annotations from typing import Literal -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, Field # ============================================================================= # Common Finding Types @@ -46,6 +46,10 @@ class BaseFinding(BaseModel): line: int = Field(0, description="Line number of the issue") suggested_fix: str | None = Field(None, description="How to fix this issue") fixable: bool = Field(False, description="Whether this can be auto-fixed") + evidence: str | None = Field( + None, + description="Actual code snippet proving the issue exists. Required for validation.", + ) class SecurityFinding(BaseFinding): @@ -78,9 +82,6 @@ class DeepAnalysisFinding(BaseFinding): "performance", "logic", ] = Field(description="Issue category") - confidence: float = Field( - 0.85, ge=0.0, le=1.0, description="AI's confidence in this finding (0.0-1.0)" - ) verification_note: str | None = Field( None, description="What evidence is missing or couldn't be verified" ) @@ -315,21 +316,11 @@ class OrchestratorFinding(BaseModel): description="Issue severity level" ) suggestion: str | None = Field(None, description="How to fix this issue") - confidence: float = Field( - 0.85, - ge=0.0, - le=1.0, - description="Confidence (0.0-1.0 or 0-100, normalized to 0.0-1.0)", + evidence: str | None = Field( + None, + description="Actual code snippet proving the issue exists. Required for validation.", ) - @field_validator("confidence", mode="before") - @classmethod - def normalize_confidence(cls, v: int | float) -> float: - """Normalize confidence to 0.0-1.0 range (accepts 0-100 or 0.0-1.0).""" - if v > 1: - return v / 100.0 - return float(v) - class OrchestratorReviewResponse(BaseModel): """Complete response schema for orchestrator PR review.""" @@ -355,9 +346,6 @@ class LogicFinding(BaseFinding): category: Literal["logic"] = Field( default="logic", description="Always 'logic' for logic findings" ) - confidence: float = Field( - 0.85, ge=0.0, le=1.0, description="Confidence in this finding (0.0-1.0)" - ) example_input: str | None = Field( None, description="Concrete input that triggers the bug" ) @@ -366,14 +354,6 @@ class LogicFinding(BaseFinding): None, description="What the code should produce" ) - @field_validator("confidence", mode="before") - @classmethod - def normalize_confidence(cls, v: int | float) -> float: - """Normalize confidence to 0.0-1.0 range.""" - if v > 1: - return v / 100.0 - return float(v) - class CodebaseFitFinding(BaseFinding): """A codebase fit finding from the codebase fit review agent.""" @@ -381,9 +361,6 @@ class CodebaseFitFinding(BaseFinding): category: Literal["codebase_fit"] = Field( default="codebase_fit", description="Always 'codebase_fit' for fit findings" ) - confidence: float = Field( - 0.85, ge=0.0, le=1.0, description="Confidence in this finding (0.0-1.0)" - ) existing_code: str | None = Field( None, description="Reference to existing code that should be used instead" ) @@ -391,14 +368,6 @@ class CodebaseFitFinding(BaseFinding): None, description="Description of the established pattern being violated" ) - @field_validator("confidence", mode="before") - @classmethod - def normalize_confidence(cls, v: int | float) -> float: - """Normalize confidence to 0.0-1.0 range.""" - if v > 1: - return v / 100.0 - return float(v) - class ParallelOrchestratorFinding(BaseModel): """A finding from the parallel orchestrator with source agent tracking.""" @@ -423,8 +392,9 @@ class ParallelOrchestratorFinding(BaseModel): severity: Literal["critical", "high", "medium", "low"] = Field( description="Issue severity level" ) - confidence: float = Field( - 0.85, ge=0.0, le=1.0, description="Confidence in this finding (0.0-1.0)" + evidence: str | None = Field( + None, + description="Actual code snippet proving the issue exists. Required for validation.", ) suggested_fix: str | None = Field(None, description="How to fix this issue") fixable: bool = Field(False, description="Whether this can be auto-fixed") @@ -436,14 +406,6 @@ class ParallelOrchestratorFinding(BaseModel): False, description="Whether multiple agents agreed on this finding" ) - @field_validator("confidence", mode="before") - @classmethod - def normalize_confidence(cls, v: int | float) -> float: - """Normalize confidence to 0.0-1.0 range.""" - if v > 1: - return v / 100.0 - return float(v) - class AgentAgreement(BaseModel): """Tracks agreement between agents on findings.""" @@ -496,22 +458,14 @@ class ResolutionVerification(BaseModel): status: Literal["resolved", "partially_resolved", "unresolved", "cant_verify"] = ( Field(description="Resolution status after AI verification") ) - confidence: float = Field( - 0.85, ge=0.0, le=1.0, description="Confidence in the resolution status" + evidence: str = Field( + min_length=1, + description="Actual code snippet showing the resolution status. Required.", ) - evidence: str = Field(description="What evidence supports this resolution status") resolution_notes: str | None = Field( None, description="Detailed notes on how the issue was addressed" ) - @field_validator("confidence", mode="before") - @classmethod - def normalize_confidence(cls, v: int | float) -> float: - """Normalize confidence to 0.0-1.0 range.""" - if v > 1: - return v / 100.0 - return float(v) - class ParallelFollowupFinding(BaseModel): """A finding from parallel follow-up review with source agent tracking.""" @@ -534,8 +488,9 @@ class ParallelFollowupFinding(BaseModel): severity: Literal["critical", "high", "medium", "low"] = Field( description="Issue severity level" ) - confidence: float = Field( - 0.85, ge=0.0, le=1.0, description="Confidence in this finding (0.0-1.0)" + evidence: str | None = Field( + None, + description="Actual code snippet proving the issue exists. Required for validation.", ) suggested_fix: str | None = Field(None, description="How to fix this issue") fixable: bool = Field(False, description="Whether this can be auto-fixed") @@ -546,14 +501,6 @@ class ParallelFollowupFinding(BaseModel): None, description="ID of related previous finding if this is a regression" ) - @field_validator("confidence", mode="before") - @classmethod - def normalize_confidence(cls, v: int | float) -> float: - """Normalize confidence to 0.0-1.0 range.""" - if v > 1: - return v / 100.0 - return float(v) - class CommentAnalysis(BaseModel): """Analysis of a contributor or AI comment.""" @@ -640,6 +587,9 @@ class FindingValidationResult(BaseModel): The finding-validator agent uses this to report whether a previous finding is a genuine issue or a false positive that should be dismissed. + + EVIDENCE-BASED VALIDATION: No confidence scores - validation is binary. + Either the evidence shows the issue exists, or it doesn't. """ finding_id: str = Field(description="ID of the finding being validated") @@ -648,16 +598,17 @@ class FindingValidationResult(BaseModel): ] = Field( description=( "Validation result: " - "confirmed_valid = issue IS real, keep as unresolved; " - "dismissed_false_positive = original finding was incorrect, remove; " - "needs_human_review = cannot determine with confidence" + "confirmed_valid = code evidence proves issue IS real; " + "dismissed_false_positive = code evidence proves issue does NOT exist; " + "needs_human_review = cannot find definitive evidence either way" ) ) code_evidence: str = Field( min_length=1, description=( "REQUIRED: Exact code snippet examined from the file. " - "Must be actual code, not a description." + "Must be actual code copy-pasted from the file, not a description. " + "This is the proof that determines the validation status." ), ) line_range: tuple[int, int] = Field( @@ -666,27 +617,18 @@ class FindingValidationResult(BaseModel): explanation: str = Field( min_length=20, description=( - "Detailed explanation of why the finding is valid/invalid. " - "Must reference specific code and explain the reasoning." + "Detailed explanation connecting the code_evidence to the validation_status. " + "Must explain: (1) what the original finding claimed, (2) what the actual code shows, " + "(3) why this proves/disproves the issue." ), ) - confidence: float = Field( - ge=0.0, - le=1.0, + evidence_verified_in_file: bool = Field( description=( - "Confidence in the validation result (0.0-1.0). " - "Must be >= 0.80 to dismiss as false positive, >= 0.70 to confirm valid." - ), + "True if the code_evidence was verified to exist at the specified line_range. " + "False if the code couldn't be found (indicates hallucination in original finding)." + ) ) - @field_validator("confidence", mode="before") - @classmethod - def normalize_confidence(cls, v: int | float) -> float: - """Normalize confidence to 0.0-1.0 range (accepts 0-100 or 0.0-1.0).""" - if v > 1: - return v / 100.0 - return float(v) - class FindingValidationResponse(BaseModel): """Complete response from the finding-validator agent.""" diff --git a/apps/backend/runners/github/services/response_parsers.py b/apps/backend/runners/github/services/response_parsers.py index db318463..2df83ea0 100644 --- a/apps/backend/runners/github/services/response_parsers.py +++ b/apps/backend/runners/github/services/response_parsers.py @@ -33,8 +33,9 @@ except (ImportError, ValueError, SystemError): TriageResult, ) -# Confidence threshold for filtering findings (GitHub Copilot standard) -CONFIDENCE_THRESHOLD = 0.80 +# Evidence-based validation replaces confidence scoring +# Findings without evidence are filtered out instead of using confidence thresholds +MIN_EVIDENCE_LENGTH = 20 # Minimum chars for evidence to be considered valid class ResponseParser: @@ -65,9 +66,13 @@ class ResponseParser: @staticmethod def parse_review_findings( - response_text: str, apply_confidence_filter: bool = True + response_text: str, require_evidence: bool = True ) -> list[PRReviewFinding]: - """Parse findings from AI response with optional confidence filtering.""" + """Parse findings from AI response with optional evidence validation. + + Evidence-based validation: Instead of confidence scores, findings + require actual code evidence proving the issue exists. + """ findings = [] try: @@ -77,14 +82,14 @@ class ResponseParser: if json_match: findings_data = json.loads(json_match.group(1)) for i, f in enumerate(findings_data): - # Get confidence (default to 0.85 if not provided for backward compat) - confidence = float(f.get("confidence", 0.85)) + # Get evidence (code snippet proving the issue) + evidence = f.get("evidence") or f.get("code_snippet") or "" - # Apply confidence threshold filter - if apply_confidence_filter and confidence < CONFIDENCE_THRESHOLD: + # Apply evidence-based validation + if require_evidence and len(evidence.strip()) < MIN_EVIDENCE_LENGTH: print( f"[AI] Dropped finding '{f.get('title', 'unknown')}': " - f"confidence {confidence:.2f} < {CONFIDENCE_THRESHOLD}", + f"insufficient evidence ({len(evidence.strip())} chars < {MIN_EVIDENCE_LENGTH})", flush=True, ) continue @@ -105,8 +110,8 @@ class ResponseParser: end_line=f.get("end_line"), suggested_fix=f.get("suggested_fix"), fixable=f.get("fixable", False), - # NEW: Support verification and redundancy fields - confidence=confidence, + # Evidence-based validation fields + evidence=evidence if evidence.strip() else None, verification_note=f.get("verification_note"), redundant_with=f.get("redundant_with"), ) diff --git a/apps/frontend/src/renderer/components/terminal/usePtyProcess.ts b/apps/frontend/src/renderer/components/terminal/usePtyProcess.ts index 5a7d9a25..c40ee99f 100644 --- a/apps/frontend/src/renderer/components/terminal/usePtyProcess.ts +++ b/apps/frontend/src/renderer/components/terminal/usePtyProcess.ts @@ -29,10 +29,14 @@ export function usePtyProcess({ // Track cwd changes - if cwd changes while terminal exists, trigger recreate useEffect(() => { if (currentCwdRef.current !== cwd) { - if (isCreatedRef.current) { - // Terminal exists, reset refs to allow recreation + // Only reset if we're not already in a controlled recreation process. + // prepareForRecreate() sets isCreatingRef=true to prevent auto-recreation + // while awaiting destroyTerminal(). Without this check, we'd reset isCreatingRef + // back to false before destroyTerminal completes, causing a race condition + // where a new PTY is created before the old one is destroyed. + if (isCreatedRef.current && !isCreatingRef.current) { + // Terminal exists and we're not in a controlled recreation, reset refs isCreatedRef.current = false; - isCreatingRef.current = false; } currentCwdRef.current = cwd; } diff --git a/tests/test_finding_validation.py b/tests/test_finding_validation.py index f01b9601..6dd9f2d0 100644 --- a/tests/test_finding_validation.py +++ b/tests/test_finding_validation.py @@ -4,6 +4,10 @@ Tests for Finding Validation System Tests the finding-validator agent integration and FindingValidationResult models. This system prevents false positives from persisting by re-investigating unresolved findings. + +NOTE: The validation system has been updated to use EVIDENCE-BASED validation +instead of confidence scores. The key field is now `evidence_verified_in_file` +which is a boolean indicating whether the code evidence was found at the specified location. """ import sys @@ -55,12 +59,12 @@ class TestFindingValidationResultModel: code_evidence="const query = `SELECT * FROM users WHERE id = ${userId}`;", line_range=(45, 45), explanation="SQL injection is present - user input is concatenated directly into the query.", - confidence=0.92, + evidence_verified_in_file=True, ) assert result.finding_id == "SEC-001" assert result.validation_status == "confirmed_valid" assert "SELECT" in result.code_evidence - assert result.confidence == 0.92 + assert result.evidence_verified_in_file is True def test_valid_dismissed_false_positive(self): """Test creating a dismissed_false_positive validation result.""" @@ -70,10 +74,10 @@ class TestFindingValidationResultModel: code_evidence="const sanitized = DOMPurify.sanitize(data);", line_range=(23, 26), explanation="Original finding claimed XSS but code uses DOMPurify.sanitize() for protection.", - confidence=0.88, + evidence_verified_in_file=True, ) assert result.validation_status == "dismissed_false_positive" - assert result.confidence == 0.88 + assert result.evidence_verified_in_file is True def test_valid_needs_human_review(self): """Test creating a needs_human_review validation result.""" @@ -83,10 +87,23 @@ class TestFindingValidationResultModel: code_evidence="async function handleRequest(req) { ... }", line_range=(100, 150), explanation="Race condition claim requires runtime analysis to verify.", - confidence=0.45, + evidence_verified_in_file=True, ) assert result.validation_status == "needs_human_review" - assert result.confidence == 0.45 + assert result.evidence_verified_in_file is True + + def test_hallucinated_finding_not_verified(self): + """Test creating a result where evidence was not verified (hallucinated finding).""" + result = FindingValidationResult( + finding_id="HALLUC-001", + validation_status="dismissed_false_positive", + code_evidence="// Line 710 does not exist - file only has 600 lines", + line_range=(600, 600), + explanation="Original finding cited line 710 but file only has 600 lines. Hallucinated finding.", + evidence_verified_in_file=False, + ) + assert result.validation_status == "dismissed_false_positive" + assert result.evidence_verified_in_file is False def test_code_evidence_required(self): """Test that code_evidence cannot be empty.""" @@ -97,7 +114,7 @@ class TestFindingValidationResultModel: code_evidence="", # Empty string should fail line_range=(45, 45), explanation="This is a detailed explanation of the issue.", - confidence=0.92, + evidence_verified_in_file=True, ) errors = exc_info.value.errors() assert any("code_evidence" in str(e) for e in errors) @@ -111,34 +128,24 @@ class TestFindingValidationResultModel: code_evidence="const x = 1;", line_range=(45, 45), explanation="Too short", # Less than 20 chars - confidence=0.92, + evidence_verified_in_file=True, ) errors = exc_info.value.errors() assert any("explanation" in str(e) for e in errors) - def test_confidence_normalized_from_percentage(self): - """Test that confidence 0-100 is normalized to 0.0-1.0.""" - result = FindingValidationResult( - finding_id="SEC-001", - validation_status="confirmed_valid", - code_evidence="const query = `SELECT * FROM users`;", - line_range=(45, 45), - explanation="SQL injection vulnerability found in the query construction.", - confidence=85, # Percentage value - ) - assert result.confidence == 0.85 - - def test_confidence_range_validation(self): - """Test that confidence must be between 0.0 and 1.0 after normalization.""" - with pytest.raises(ValidationError): + def test_evidence_verified_required(self): + """Test that evidence_verified_in_file is required.""" + with pytest.raises(ValidationError) as exc_info: FindingValidationResult( finding_id="SEC-001", validation_status="confirmed_valid", - code_evidence="const x = 1;", + code_evidence="const query = `SELECT * FROM users`;", line_range=(45, 45), - explanation="This is a detailed explanation of the issue.", - confidence=150, # Will normalize to 1.5, which is out of range + explanation="SQL injection vulnerability found in the query construction.", + # Missing evidence_verified_in_file ) + errors = exc_info.value.errors() + assert any("evidence_verified_in_file" in str(e) for e in errors) def test_invalid_validation_status(self): """Test that invalid validation_status values are rejected.""" @@ -149,7 +156,7 @@ class TestFindingValidationResultModel: code_evidence="const x = 1;", line_range=(45, 45), explanation="This is a detailed explanation of the issue.", - confidence=0.92, + evidence_verified_in_file=True, ) @@ -166,7 +173,7 @@ class TestFindingValidationResponse: code_evidence="const query = `SELECT * FROM users`;", line_range=(45, 45), explanation="SQL injection confirmed in this query.", - confidence=0.92, + evidence_verified_in_file=True, ), FindingValidationResult( finding_id="QUAL-002", @@ -174,7 +181,7 @@ class TestFindingValidationResponse: code_evidence="const sanitized = DOMPurify.sanitize(data);", line_range=(23, 26), explanation="Code uses DOMPurify so XSS claim is false.", - confidence=0.88, + evidence_verified_in_file=True, ), ], summary="1 finding confirmed valid, 1 dismissed as false positive", @@ -197,7 +204,6 @@ class TestParallelFollowupResponseWithValidation: ResolutionVerification( finding_id="SEC-001", status="unresolved", - confidence=0.85, evidence="File was not modified", ) ], @@ -208,7 +214,7 @@ class TestParallelFollowupResponseWithValidation: code_evidence="const query = `SELECT * FROM users`;", line_range=(45, 45), explanation="SQL injection confirmed in this query.", - confidence=0.92, + evidence_verified_in_file=True, ) ], new_findings=[], @@ -231,7 +237,6 @@ class TestParallelFollowupResponseWithValidation: ResolutionVerification( finding_id="SEC-001", status="unresolved", - confidence=0.50, evidence="Line wasn't changed but need to verify", ) ], @@ -242,7 +247,7 @@ class TestParallelFollowupResponseWithValidation: code_evidence="const query = db.prepare('SELECT * FROM users WHERE id = ?').get(userId);", line_range=(45, 48), explanation="Original review misread - using parameterized query.", - confidence=0.95, + evidence_verified_in_file=True, ) ], new_findings=[], @@ -275,11 +280,10 @@ class TestPRReviewFindingValidationFields: line=42, validation_status="confirmed_valid", validation_evidence="const query = `SELECT * FROM users`;", - validation_confidence=0.92, validation_explanation="SQL injection confirmed in the query.", ) assert finding.validation_status == "confirmed_valid" - assert finding.validation_confidence == 0.92 + assert finding.validation_evidence is not None def test_finding_without_validation_fields(self): """Test that validation fields are optional.""" @@ -294,7 +298,6 @@ class TestPRReviewFindingValidationFields: ) assert finding.validation_status is None assert finding.validation_evidence is None - assert finding.validation_confidence is None assert finding.validation_explanation is None def test_finding_to_dict_includes_validation(self): @@ -309,13 +312,11 @@ class TestPRReviewFindingValidationFields: line=42, validation_status="confirmed_valid", validation_evidence="const query = ...;", - validation_confidence=0.92, validation_explanation="Issue confirmed.", ) data = finding.to_dict() assert data["validation_status"] == "confirmed_valid" assert data["validation_evidence"] == "const query = ...;" - assert data["validation_confidence"] == 0.92 assert data["validation_explanation"] == "Issue confirmed." def test_finding_from_dict_with_validation(self): @@ -330,12 +331,10 @@ class TestPRReviewFindingValidationFields: "line": 42, "validation_status": "dismissed_false_positive", "validation_evidence": "parameterized query used", - "validation_confidence": 0.88, "validation_explanation": "False positive - using prepared statements.", } finding = PRReviewFinding.from_dict(data) assert finding.validation_status == "dismissed_false_positive" - assert finding.validation_confidence == 0.88 # ============================================================================ @@ -365,7 +364,7 @@ class TestValidationIntegration: code_evidence="const query = `SELECT * FROM users`;", line_range=(45, 45), explanation="SQL injection confirmed in this query construction.", - confidence=0.92, + evidence_verified_in_file=True, ), FindingValidationResult( finding_id="QUAL-002", @@ -373,7 +372,7 @@ class TestValidationIntegration: code_evidence="const sanitized = DOMPurify.sanitize(data);", line_range=(23, 26), explanation="Original XSS claim was incorrect - uses DOMPurify.", - confidence=0.88, + evidence_verified_in_file=True, ), ], new_findings=[], @@ -409,6 +408,6 @@ class TestValidationIntegration: code_evidence="const x = 1;", line_range=(1, 1), explanation="This is a valid explanation for the finding status.", - confidence=0.85, + evidence_verified_in_file=True, ) assert result.validation_status == status diff --git a/tests/test_structured_outputs.py b/tests/test_structured_outputs.py index dc5f34a5..1c0f5375 100644 --- a/tests/test_structured_outputs.py +++ b/tests/test_structured_outputs.py @@ -223,7 +223,7 @@ class TestOrchestratorFinding: """Tests for OrchestratorFinding model.""" def test_valid_finding(self): - """Test valid orchestrator finding.""" + """Test valid orchestrator finding with evidence field.""" data = { "file": "src/api.py", "line": 25, @@ -232,40 +232,24 @@ class TestOrchestratorFinding: "category": "quality", "severity": "medium", "suggestion": "Add error handling with proper logging", - "confidence": 90, + "evidence": "def handle_request(req):\n result = db.query(req.id) # no try-catch", } result = OrchestratorFinding.model_validate(data) assert result.file == "src/api.py" - assert result.confidence == 0.9 # 90 normalized to 0.9 + assert result.evidence is not None + assert "no try-catch" in result.evidence - def test_confidence_bounds(self): - """Test confidence bounds (accepts 0-100 or 0.0-1.0, normalized to 0.0-1.0).""" - # Valid min + def test_evidence_optional(self): + """Test that evidence field is optional.""" data = { "file": "test.py", "title": "Test", - "description": "Test", + "description": "Test finding", "category": "quality", "severity": "low", - "confidence": 0, } result = OrchestratorFinding.model_validate(data) - assert result.confidence == 0 # 0 stays as 0 - - # Valid max (100% normalized to 1.0) - data["confidence"] = 100 - result = OrchestratorFinding.model_validate(data) - assert result.confidence == 1.0 # 100 normalized to 1.0 - - # Invalid: over 100 (would normalize to >1.0) - data["confidence"] = 101 - with pytest.raises(ValidationError): - OrchestratorFinding.model_validate(data) - - # Invalid: negative - data["confidence"] = -1 - with pytest.raises(ValidationError): - OrchestratorFinding.model_validate(data) + assert result.evidence is None class TestOrchestratorReviewResponse: @@ -284,7 +268,7 @@ class TestOrchestratorReviewResponse: "description": "API key exposed in source", "category": "security", "severity": "critical", - "confidence": 95, + "evidence": "API_KEY = 'sk-prod-12345abcdef'", } ], "summary": "Found 1 critical security issue", @@ -394,8 +378,8 @@ class TestSecurityFinding: class TestDeepAnalysisFinding: """Tests for DeepAnalysisFinding model.""" - def test_confidence_float(self): - """Test confidence is a float between 0 and 1.""" + def test_evidence_field(self): + """Test evidence field for proof of issue.""" data = { "id": "deep-1", "severity": "medium", @@ -404,10 +388,10 @@ class TestDeepAnalysisFinding: "file": "worker.py", "line": 100, "category": "logic", - "confidence": 0.75, + "evidence": "shared_state += 1 # no lock protection", } result = DeepAnalysisFinding.model_validate(data) - assert result.confidence == 0.75 + assert result.evidence == "shared_state += 1 # no lock protection" def test_verification_note(self): """Test verification note field."""