From d8f4de9a06c01ed7248ea18e9eead1450bc76bcc Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Tue, 20 Jan 2026 18:22:57 +0100 Subject: [PATCH] feat(pr-review): add validation pipeline, context enrichment, and cross-validation (#1354) * docs(phase-1): research core validation pipeline Phase 1: Core Validation Pipeline - Finding-validator pattern from follow-up reviews documented - Orchestrator integration points identified - Context bug at line 1288 analyzed - Prompt patterns for Read tool instructions catalogued - Evidence/scope validation strategies defined Co-Authored-By: Claude Opus 4.5 * fix(01-01): include AI reviews in follow-up context - Fixed ai_bot_comments_since_review to include ai_reviews - Mirrors contributor_comments + contributor_reviews pattern - AI formal reviews (CodeRabbit, Cursor) now available to follow-up agents * feat(01-02): add tool usage instructions to follow-up agent prompts - Add "CRITICAL: Full Context Analysis" section to follow-up prompts - Require Read tool usage before reporting findings - Require +-20 lines context around flagged lines - Require actual code evidence, not descriptions - Require Grep search for mitigations Files: pr_followup_resolution_agent.md, pr_followup_newcode_agent.md * test(01-01): add tests for AI reviews inclusion in follow-up context - Test AI bot patterns include known bots (CodeRabbit, Gemini, Copilot) - Test FollowupReviewContext has ai_bot_comments_since_review field - Test FollowupContextGatherer.gather() includes AI formal reviews - Test AI reviews are correctly separated from contributor reviews * feat(01-03): add finding-validator agent to parallel orchestrator - Load pr_finding_validator.md prompt in _define_specialist_agents() - Add finding-validator AgentDefinition with tools [Read, Grep, Glob] - Description instructs to validate ALL findings after specialist agents * feat(01-03): add Phase 3.5 validation step to orchestrator prompt - Add finding-validator to Available Specialist Agents section - Add Phase 3.5: Finding Validation (CRITICAL - Prevent False Positives) - Instructions to invoke validator for ALL findings after synthesis - Filter based on validation status (confirmed_valid, dismissed_false_positive) - Re-calculate verdict based only on validated findings * feat(01-03): add validation fields to orchestrator output format - Add validation_summary top-level field (total, confirmed, dismissed, needs_review) - Add validation_status field per finding (confirmed_valid, dismissed_false_positive, needs_human_review) - Add validation_evidence field per finding with actual code snippet - Document that dismissed findings should be removed from output * feat(01-04): add evidence validation function for PR findings - Add _validate_finding_evidence() helper to validate evidence quality - Rejects findings with no evidence or very short evidence (<10 chars) - Filters findings that start with description patterns (not code) - Requires code syntax characters in evidence to pass validation * feat(01-04): add scope pre-filter function for PR findings - Add _is_finding_in_scope() to verify findings are within PR scope - Rejects findings for files not in changed files list - Allows impact findings (affect/break/depend) for unchanged files - Rejects findings with invalid line numbers (<= 0) * feat(01-04): integrate evidence and scope filters into finding processing - Apply _validate_finding_evidence to filter findings with poor evidence - Apply _is_finding_in_scope to filter findings outside PR scope - Log filtered findings with reasons for debugging - Replace unique_findings with validated_findings for verdict/summary * docs(02): create phase 2 plans for context enrichment Phase 02: Context Enrichment - 3 plans in 2 waves - Plans 01 & 02 parallel (Wave 1), Plan 03 sequential (Wave 2) - Ready for execution Plan details: - 02-01: JS/TS import analysis (path aliases, CommonJS, re-exports) - 02-02: Python import analysis via AST - 02-03: Related files enhancement (limit 50, prioritization, reverse deps) Co-Authored-By: Claude Opus 4.5 * feat(02-02): add Python import resolution methods - Add ast import for Python AST parsing - Add _resolve_python_import() to resolve module names to file paths - Add _find_python_imports() to extract imports using AST - Handles relative imports (from . import, from .. import) - Handles absolute imports that map to project files - Gracefully handles SyntaxError in Python files * feat(02-02): integrate Python import detection into _find_imports - Replace TODO comment with actual Python import detection - Call _find_python_imports() for .py files in _find_imports() - Python files now have their imports resolved to file paths * fix(02-01): prevent _load_json_safe from mangling path patterns with /* The regex-based comment stripping was incorrectly removing path patterns like "@/*" from tsconfig.json because /* looks like a multi-line comment. Fix: - Try standard JSON parse first (most tsconfigs don't have comments) - Fall back to smarter comment stripping that checks if // appears outside of strings by counting quotes before the comment position This ensures path aliases like "@/*": ["src/*"] are preserved. Co-Authored-By: Claude Opus 4.5 * feat(02-03): add reverse dependency detection - Add _find_dependents() method to find files that import a given file - Use grep with recursive search for import/from statements - Skip generic names (index, main, utils) to avoid too many matches - 5-second timeout protection prevents hanging on large repos - Exclude common non-code directories (node_modules, .git, __pycache__) - Limit results to prevent overwhelming context * feat(02-03): add smart file prioritization - Add _prioritize_related_files() method for relevance-based ordering - Priority: tests > type definitions > configs > other files - Sort alphabetically within each category for consistency - Supports limit parameter (default 50) - Fix .d.ts detection using name_lower.endswith('.d.ts') * feat(02-03): update _find_related_files with reverse deps and prioritization - Add reverse dependency detection call to _find_related_files() - Replace simple sorting with _prioritize_related_files() - Increase limit from 20 to 50 files - Update find_related_files_for_root() static method limit to 50 - Tests pass (1616 passed, 11 skipped) * docs(03): research phase 3 cross-validation domain Phase 3: Cross-Validation - Confidence threshold routing (REQ-011) - Multi-agent cross-validation (REQ-012) - Standard stack identified (built-in Python, existing Pydantic models) - Architecture patterns documented - Common pitfalls catalogued Co-Authored-By: Claude Opus 4.5 * docs(03): create phase 3 plans for cross-validation Phase 03: Cross-Validation - 2 plans in 2 waves - Plan 03-01: Confidence threshold routing (Wave 1) - Plan 03-02: Multi-agent agreement and confidence boost (Wave 2) - Ready for execution Co-Authored-By: Claude Opus 4.5 * fix(03): revise plans based on checker feedback Address checker issues: - 03-01: Add Task 0 to add confidence, source_agents, cross_validated fields to PRReviewFinding dataclass - 03-02: Update Task 1 to clarify it uses the new PRReviewFinding fields (not just pydantic model) - 03-02: Document that AgentAgreement is logged for monitoring, not persisted to PRReviewResult Co-Authored-By: Claude Opus 4.5 * feat(03-01): add cross-validation fields to PRReviewFinding - Add confidence: float = 0.5 field for confidence scoring - Add source_agents: list[str] field to track which agents reported finding - Add cross_validated: bool field to track multi-agent agreement - Update to_dict() to include all three new fields - Update from_dict() to handle all three new fields with defaults - Fix output_validator to treat confidence=0.5 as default (not explicit) * feat(03-01): add confidence routing function - Add ConfidenceTier class with HIGH/MEDIUM/LOW constants (0.8/0.5 thresholds) - Add _apply_confidence_routing() method to ParallelOrchestratorReviewer - HIGH (>=0.8): Include finding as-is - MEDIUM (0.5-0.8): Include with '[Potential]' prefix in title - LOW (<0.5): Log and exclude from output - Handle missing confidence gracefully (default to 0.5) - Log tier distribution after routing * feat(03-01): wire confidence routing into review pipeline - Call _apply_confidence_routing() after evidence/scope validation - Log routing results: included count vs dropped (low confidence) - Use routed findings for verdict and summary generation - Confidence routing happens AFTER validation, BEFORE verdict * docs(03-01): update orchestrator prompt with confidence tier guidance - Add 'Confidence Tiers' section after Phase 3.5 - Document tier thresholds: HIGH (>=0.8), MEDIUM (0.5-0.8), LOW (<0.5) - Include guidelines for assigning confidence scores - Provide examples of confidence score assignments - Placed between validation section and output format * docs(03-01): complete confidence threshold routing plan Tasks completed: 4/4 - Task 0: Add cross-validation fields to PRReviewFinding model - Task 1: Add confidence routing function - Task 2: Wire confidence routing into review pipeline - Task 3: Update orchestrator prompt with confidence tier guidance SUMMARY: .planning/phases/03-cross-validation/03-01-SUMMARY.md * feat(03-02): add _cross_validate_findings method - Groups findings by (file, line, category) for multi-agent agreement detection - Boosts confidence by 0.15 (capped at 0.95) when 2+ agents agree - Sets cross_validated=True and populates source_agents on PRReviewFinding - Returns AgentAgreement tracking object with agreed_findings list - Uses collections.defaultdict for efficient grouping - Merges evidence with '---' separator, keeps highest severity * feat(03-02): wire cross-validation into review pipeline - Call _cross_validate_findings after deduplication - Cross-validated findings flow through evidence/scope validation - Cross-validated findings flow through confidence routing - Log AgentAgreement: info level for summary, debug level for full JSON - Pipeline order: deduplicate -> cross-validate -> validate evidence/scope -> confidence route * docs(03-02): add multi-agent agreement documentation to orchestrator prompt - Add 'Multi-Agent Agreement' section documenting confidence boost behavior - Document +0.15 confidence boost when 2+ agents agree (max 0.95) - Add example showing merged finding with cross_validated and source_agents - Document agent_agreement tracking and logging behavior - Update Phase 3: Synthesis to reference cross-validation and confidence routing * docs(04): create phase plan for integration testing Phase 04: Integration Testing - 1 plan in 1 wave - Tests all Phase 1-3 features - Ready for execution Co-Authored-By: Claude Opus 4.5 * test(04-01): add Phase 1 feature tests - confidence, evidence, scope - Add TestConfidenceTierRouting with 7 tests for tier boundaries - Add TestEvidenceValidation with 6 tests for code syntax detection - Add TestScopeFiltering with 6 tests for scope filtering logic - Import ConfidenceTier, _validate_finding_evidence, _is_finding_in_scope - All 18 Phase 1 tests passing * test(04-01): add Phase 2 and Phase 3 feature tests Phase 2 - Import Detection (5 tests): - Path alias detection (@/utils -> src/utils.ts) - CommonJS require('./utils') detection - Re-export (export * from) detection - Python relative import via AST - Python absolute import resolution Phase 2 - Reverse Dependencies (3 tests): - Grep-based dependent file detection - Generic name skipping (index, main, utils) - Timeout handling for large repos Phase 3 - Cross-Validation (7 tests): - Multi-agent agreement confidence boost (+0.15) - Confidence cap at 0.95 - cross_validated flag on merged findings - Grouping by (file, line, category) tuple - Description combination with ' | ' separator - Single-agent findings not boosted - Highest severity preserved on merge All 33 tests passing * test(04-01): add integration pipeline verification tests TestIntegrationPipeline (9 tests): - Full pipeline flow: high confidence + valid evidence + in scope - Low confidence filtering behavior documentation - Cross-validation elevating MEDIUM to HIGH tier - Invalid evidence rejection regardless of confidence - Out-of-scope rejection - Impact finding allowance for unchanged files - End-to-end review scenario with multiple agents - Empty findings handling - Confidence tier routing documentation Total: 42 integration tests passing * gitignore planning for GSD test * chore: remove .planning/ from git tracking These files are in .gitignore but were committed before the ignore rule was added. Removing from tracking to keep planning files local. Co-Authored-By: Claude Opus 4.5 * fix: cross-platform _find_dependents and improved test assertions - Replace grep subprocess with pure Python os.walk() + re.compile() for cross-platform compatibility (Windows, macOS, Linux) - Add debug logging to _load_json_safe() for troubleshooting - Fix test assertion type (set instead of list) Co-Authored-By: Claude Opus 4.5 * fix: address all PR review findings (10 issues) HIGH priority fixes: - Fix path alias resolution to use project root instead of relative path - Rewrite test to mock os.walk instead of subprocess.run - Extract duplicated 'Full Context Analysis' to partials/ with sync comments MEDIUM priority fixes: - Extract _resolve_any_import() helper to eliminate DRY violation - Improve path alias test to verify actual resolution - Add guard for empty target_paths in tsconfig - Convert ConfidenceTier to str, Enum pattern - Add block comment stripping in _load_json_safe LOW priority fixes: - Remove unused tempfile import - Remove duplicate .planning/ gitignore entry Co-Authored-By: Claude Opus 4.5 * fix: restore phase_config module after mock to prevent test pollution The test_integration_phase4.py was mocking phase_config at module level during import, which polluted sys.modules for subsequent tests. This caused test_agent_configs::test_thinking_defaults_are_valid to fail because THINKING_BUDGET_MAP.keys() returned empty from the MagicMock. Fix: Save and restore the original phase_config module after loading the orchestrator module. Co-Authored-By: Claude Opus 4.5 * fix: add env cleanup fixture to test_client.py for test isolation Add autouse fixture to clear AUTH_TOKEN_ENV_VARS before and after each test in TestClientTokenValidation. This ensures test isolation and prevents env var pollution from previous tests in the suite. Co-Authored-By: Claude Opus 4.5 * fix: mock decrypt_token in encrypted token rejection tests Also mock decrypt_token to raise ValueError, ensuring the encrypted token flows through to validate_token_not_encrypted regardless of whether the CI environment has a claude CLI available that might attempt decryption. Co-Authored-By: Claude Opus 4.5 * fix: restore all mocked modules in test_integration_phase4.py The test was mocking core.client, phase_config, and other modules at module level but only restoring phase_config. This caused core.client to remain as a MagicMock, which made validate_token_not_encrypted a MagicMock that never raised ValueError. Now all mocked modules are saved before mocking and restored after the orchestrator module is loaded. Co-Authored-By: Claude Opus 4.5 * fix: normalize path separators for cross-platform test compatibility Windows returns paths with backslashes (src\utils.ts) while the test expected forward slashes (src/utils.ts). Normalize to forward slashes for comparison. Co-Authored-By: Claude Opus 4.5 * fix: normalize path separators in all import detection tests Apply the same Windows path normalization fix to: - test_commonjs_require_detection - test_reexport_detection - test_python_relative_import - test_python_absolute_import Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 --- .../github/partials/full_context_analysis.md | 39 + .../prompts/github/pr_codebase_fit_agent.md | 27 + .../github/pr_followup_newcode_agent.md | 27 + .../github/pr_followup_resolution_agent.md | 26 + apps/backend/prompts/github/pr_logic_agent.md | 27 + .../github/pr_parallel_orchestrator.md | 120 +- .../prompts/github/pr_quality_agent.md | 27 + .../prompts/github/pr_security_agent.md | 27 + .../runners/github/context_gatherer.py | 504 +++++++- apps/backend/runners/github/models.py | 15 + .../runners/github/output_validator.py | 8 +- .../parallel_orchestrator_reviewer.py | 361 +++++- tests/test_client.py | 27 + tests/test_context_gatherer.py | 237 ++++ tests/test_integration_phase4.py | 1132 +++++++++++++++++ 15 files changed, 2578 insertions(+), 26 deletions(-) create mode 100644 apps/backend/prompts/github/partials/full_context_analysis.md create mode 100644 tests/test_context_gatherer.py create mode 100644 tests/test_integration_phase4.py diff --git a/apps/backend/prompts/github/partials/full_context_analysis.md b/apps/backend/prompts/github/partials/full_context_analysis.md new file mode 100644 index 00000000..ef4d8771 --- /dev/null +++ b/apps/backend/prompts/github/partials/full_context_analysis.md @@ -0,0 +1,39 @@ +# Full Context Analysis (Shared Partial) + +This section is shared across multiple PR review agent prompts. +When updating this content, sync to all files listed below: + +- pr_security_agent.md +- pr_quality_agent.md +- pr_logic_agent.md +- pr_codebase_fit_agent.md +- pr_followup_newcode_agent.md +- pr_followup_resolution_agent.md (partial version) + +--- + +## CRITICAL: Full Context Analysis + +Before reporting ANY finding, you MUST: + +1. **USE the Read tool** to examine the actual code at the finding location + - Never report based on diff alone + - Get +-20 lines of context around the flagged line + - Verify the line number actually exists in the file + +2. **Verify the issue exists** - Not assume it does + - Is the problematic pattern actually present at this line? + - Is there validation/sanitization nearby you missed? + - Does the framework provide automatic protection? + +3. **Provide code evidence** - Copy-paste the actual code + - Your `evidence` field must contain real code from the file + - Not descriptions like "the code does X" but actual `const query = ...` + - If you can't provide real code, you haven't verified the issue + +4. **Check for mitigations** - Use Grep to search for: + - Validation functions that might sanitize this input + - Framework-level protections + - Comments explaining why code appears unsafe + +**Your evidence must prove the issue exists - not just that you suspect it.** diff --git a/apps/backend/prompts/github/pr_codebase_fit_agent.md b/apps/backend/prompts/github/pr_codebase_fit_agent.md index 9a14b56d..90c333cf 100644 --- a/apps/backend/prompts/github/pr_codebase_fit_agent.md +++ b/apps/backend/prompts/github/pr_codebase_fit_agent.md @@ -92,6 +92,33 @@ Before flagging a "should use existing utility" issue: 2. Check if existing utility has the right signature/behavior 3. Consider if the new implementation is intentionally different + +## CRITICAL: Full Context Analysis + +Before reporting ANY finding, you MUST: + +1. **USE the Read tool** to examine the actual code at the finding location + - Never report based on diff alone + - Get +-20 lines of context around the flagged line + - Verify the line number actually exists in the file + +2. **Verify the issue exists** - Not assume it does + - Is the problematic pattern actually present at this line? + - Is there validation/sanitization nearby you missed? + - Does the framework provide automatic protection? + +3. **Provide code evidence** - Copy-paste the actual code + - Your `evidence` field must contain real code from the file + - Not descriptions like "the code does X" but actual `const query = ...` + - If you can't provide real code, you haven't verified the issue + +4. **Check for mitigations** - Use Grep to search for: + - Validation functions that might sanitize this input + - Framework-level protections + - Comments explaining why code appears unsafe + +**Your evidence must prove the issue exists - not just that you suspect it.** + ## Code Patterns to Flag ### Reinventing Existing Utilities diff --git a/apps/backend/prompts/github/pr_followup_newcode_agent.md b/apps/backend/prompts/github/pr_followup_newcode_agent.md index 5021113b..c1e2e774 100644 --- a/apps/backend/prompts/github/pr_followup_newcode_agent.md +++ b/apps/backend/prompts/github/pr_followup_newcode_agent.md @@ -124,6 +124,33 @@ For findings claiming something is **missing** (no fallback, no validation, no e **Only report if you can confidently say**: "I verified the complete scope and the safeguard does not exist." + +## CRITICAL: Full Context Analysis + +Before reporting ANY finding, you MUST: + +1. **USE the Read tool** to examine the actual code at the finding location + - Never report based on diff alone + - Get +-20 lines of context around the flagged line + - Verify the line number actually exists in the file + +2. **Verify the issue exists** - Not assume it does + - Is the problematic pattern actually present at this line? + - Is there validation/sanitization nearby you missed? + - Does the framework provide automatic protection? + +3. **Provide code evidence** - Copy-paste the actual code + - Your `evidence` field must contain real code from the file + - Not descriptions like "the code does X" but actual `const query = ...` + - If you can't provide real code, you haven't verified the issue + +4. **Check for mitigations** - Use Grep to search for: + - Validation functions that might sanitize this input + - Framework-level protections + - Comments explaining why code appears unsafe + +**Your evidence must prove the issue exists - not just that you suspect it.** + ## Evidence Requirements Every finding MUST include an `evidence` field with: diff --git a/apps/backend/prompts/github/pr_followup_resolution_agent.md b/apps/backend/prompts/github/pr_followup_resolution_agent.md index 9e35b827..0323bbec 100644 --- a/apps/backend/prompts/github/pr_followup_resolution_agent.md +++ b/apps/backend/prompts/github/pr_followup_resolution_agent.md @@ -69,6 +69,32 @@ For each verification, provide actual code evidence: - Verify the pattern still exists (for unresolved) - Check surrounding context for alternative fixes you might miss +## CRITICAL: Full Context Analysis + +Before reporting ANY finding, you MUST: + +1. **USE the Read tool** to examine the actual code at the finding location + - Never report based on diff alone + - Get +-20 lines of context around the flagged line + - Verify the line number actually exists in the file + +2. **Verify the issue exists** - Not assume it does + - Is the problematic pattern actually present at this line? + - Is there validation/sanitization nearby you missed? + - Does the framework provide automatic protection? + +3. **Provide code evidence** - Copy-paste the actual code + - Your `evidence` field must contain real code from the file + - Not descriptions like "the code does X" but actual `const query = ...` + - If you can't provide real code, you haven't verified the issue + +4. **Check for mitigations** - Use Grep to search for: + - Validation functions that might sanitize this input + - Framework-level protections + - Comments explaining why code appears unsafe + +**Your evidence must prove the issue exists - not just that you suspect it.** + ## Resolution Criteria ### RESOLVED diff --git a/apps/backend/prompts/github/pr_logic_agent.md b/apps/backend/prompts/github/pr_logic_agent.md index 328ba13d..5cd1e3d2 100644 --- a/apps/backend/prompts/github/pr_logic_agent.md +++ b/apps/backend/prompts/github/pr_logic_agent.md @@ -113,6 +113,33 @@ For each finding, provide: 2. What the current code produces 3. What it should produce + +## CRITICAL: Full Context Analysis + +Before reporting ANY finding, you MUST: + +1. **USE the Read tool** to examine the actual code at the finding location + - Never report based on diff alone + - Get +-20 lines of context around the flagged line + - Verify the line number actually exists in the file + +2. **Verify the issue exists** - Not assume it does + - Is the problematic pattern actually present at this line? + - Is there validation/sanitization nearby you missed? + - Does the framework provide automatic protection? + +3. **Provide code evidence** - Copy-paste the actual code + - Your `evidence` field must contain real code from the file + - Not descriptions like "the code does X" but actual `const query = ...` + - If you can't provide real code, you haven't verified the issue + +4. **Check for mitigations** - Use Grep to search for: + - Validation functions that might sanitize this input + - Framework-level protections + - Comments explaining why code appears unsafe + +**Your evidence must prove the issue exists - not just that you suspect it.** + ## Code Patterns to Flag ### Off-By-One Errors diff --git a/apps/backend/prompts/github/pr_parallel_orchestrator.md b/apps/backend/prompts/github/pr_parallel_orchestrator.md index b26ffa97..569b59e4 100644 --- a/apps/backend/prompts/github/pr_parallel_orchestrator.md +++ b/apps/backend/prompts/github/pr_parallel_orchestrator.md @@ -58,6 +58,10 @@ You have access to these specialized review agents via the Task tool: **Description**: AI comment validator for triaging comments from CodeRabbit, Gemini Code Assist, Cursor, Greptile, and other AI reviewers. **When to use**: PRs that have existing AI review comments that need validation. +### finding-validator +**Description**: Finding validation specialist that re-investigates findings to confirm they are real issues, not false positives. +**When to use**: After ALL specialist agents have reported their findings. Invoke for EVERY finding to validate it exists in the actual code. + ## Your Workflow ### Phase 1: Analysis @@ -96,21 +100,114 @@ For a PR adding a new authentication endpoint: After receiving agent results, synthesize findings: 1. **Aggregate**: Collect all findings from all agents -2. **Cross-validate**: - - If multiple agents report the same issue → boost confidence - - If agents conflict → use your judgment to resolve +2. **Cross-validate** (see "Multi-Agent Agreement" section): + - Group findings by (file, line, category) + - If 2+ agents report same issue → merge into one, boost confidence by +0.15 + - Set `cross_validated: true` and populate `source_agents` list + - Track agreed finding IDs in `agent_agreement.agreed_findings` 3. **Deduplicate**: Remove overlapping findings (same file + line + issue type) -4. **Filter**: Only include findings with confidence ≥80% +4. **Route by Confidence** (see "Confidence Tiers" section): + - HIGH (>=0.8): Include as-is + - MEDIUM (0.5-0.8): Include with "[Potential]" prefix + - LOW (<0.5): Log and exclude 5. **Generate Verdict**: Based on severity of remaining findings +### Phase 3.5: Finding Validation (CRITICAL - Prevent False Positives) + +**MANDATORY STEP** - After synthesis, validate ALL findings before generating verdict: + +1. **Invoke finding-validator** for EACH finding from specialist agents +2. For each finding, the validator returns one of: + - `confirmed_valid` - Issue IS real, keep in findings list + - `dismissed_false_positive` - Original finding was WRONG, remove from findings + - `needs_human_review` - Cannot determine, keep but flag for human + +3. **Filter findings based on validation:** + - Keep only `confirmed_valid` findings + - Remove `dismissed_false_positive` findings entirely + - Keep `needs_human_review` but add note in description + +4. **Re-calculate verdict** based on VALIDATED findings only + - A finding dismissed as false positive does NOT count toward verdict + - Only confirmed issues determine severity + +**Why this matters:** Specialist agents sometimes flag issues that don't exist in the actual code. The validator reads the code with fresh eyes to catch these false positives before they're reported. + +**Example workflow:** +``` +Specialist finds 3 issues → finding-validator validates each → +Result: 2 confirmed, 1 dismissed → Verdict based on 2 issues +``` + +## Confidence Tiers + +After validation, findings are routed based on confidence scores: + +| Tier | Score Range | Treatment | +|------|-------------|-----------| +| HIGH | >= 0.8 | Included as reported, affects verdict | +| MEDIUM | 0.5 - 0.8 | Included with "[Potential]" prefix, affects verdict | +| LOW | < 0.5 | Logged for monitoring, excluded from output | + +**Guidelines for assigning confidence:** +- 0.9+ : Direct evidence in code, multiple indicators, clear violation +- 0.8-0.9 : Strong evidence, clear pattern, high certainty +- 0.6-0.8 : Likely issue but some uncertainty, may need context +- 0.4-0.6 : Possible issue, limited evidence, context-dependent +- < 0.4 : Speculation, no direct evidence, likely false positive + +**Example:** +- SQL injection with `userId` in query string: 0.95 (direct evidence) +- Missing null check where input could be null: 0.75 (likely but depends on callers) +- "This might cause issues" without specifics: 0.3 (speculation, will be dropped) + +## Multi-Agent Agreement + +When multiple specialist agents flag the same issue (same file + line + category), this is strong signal: + +### Confidence Boost +- If 2+ agents agree: confidence boosted by +0.15 (max 0.95) +- cross_validated field set to true +- source_agents lists all agents that flagged the issue + +### Why This Matters +- Independent verification increases certainty +- False positives rarely get flagged by multiple specialized agents +- Multi-agent agreement often indicates real issues + +### Example +``` +security-reviewer finds: XSS vulnerability at line 45 (confidence: 0.75) +quality-reviewer finds: Unsafe string interpolation at line 45 (confidence: 0.70) + +Result: Single finding with confidence 0.90 (0.75 + 0.15 boost) + source_agents: ["security-reviewer", "quality-reviewer"] + cross_validated: true +``` + +### Agent Agreement Tracking +The `agent_agreement` field in structured output tracks: +- `agreed_findings`: Finding IDs where 2+ agents agreed +- `conflicting_findings`: Finding IDs where agents disagreed (reserved for future) +- `resolution_notes`: How conflicts were resolved (reserved for future) + +**Note:** Agent agreement data is logged for monitoring. The cross-validation results +are reflected in each finding's source_agents, cross_validated, and confidence fields. + ## Output Format -After synthesis, output your final review in this JSON format: +After synthesis and validation, output your final review in this JSON format: ```json { "analysis_summary": "Brief description of what you analyzed and why you chose those agents", - "agents_invoked": ["security-reviewer", "quality-reviewer"], + "agents_invoked": ["security-reviewer", "quality-reviewer", "finding-validator"], + "validation_summary": { + "total_findings": 5, + "confirmed_valid": 3, + "dismissed_false_positive": 2, + "needs_human_review": 0 + }, "findings": [ { "id": "finding-1", @@ -125,7 +222,9 @@ After synthesis, output your final review in this JSON format: "suggested_fix": "Use parameterized queries", "fixable": true, "source_agents": ["security-reviewer"], - "cross_validated": false + "cross_validated": false, + "validation_status": "confirmed_valid", + "validation_evidence": "Actual code: `const query = 'SELECT * FROM users WHERE id = ' + userId`" } ], "agent_agreement": { @@ -138,6 +237,13 @@ After synthesis, output your final review in this JSON format: } ``` +**Note on validation fields:** +- `validation_summary` at top level tracks validation statistics +- Each finding includes `validation_status` ("confirmed_valid", "dismissed_false_positive", or "needs_human_review") +- Each finding includes `validation_evidence` with actual code snippet from validation +- Only include findings with `validation_status: "confirmed_valid"` or `"needs_human_review"` in the final output +- Dismissed findings should be removed from the findings array entirely + ## Verdict Types (Strict Quality Gates) We use strict quality gates because AI can fix issues quickly. Only LOW severity findings are optional. diff --git a/apps/backend/prompts/github/pr_quality_agent.md b/apps/backend/prompts/github/pr_quality_agent.md index 7a3445fc..ba9f2dba 100644 --- a/apps/backend/prompts/github/pr_quality_agent.md +++ b/apps/backend/prompts/github/pr_quality_agent.md @@ -114,6 +114,33 @@ When your finding claims something is **missing** (no error handling, no fallbac - Respect framework idioms (React hooks, etc.) - Distinguish between "wrong" and "not my style" + +## CRITICAL: Full Context Analysis + +Before reporting ANY finding, you MUST: + +1. **USE the Read tool** to examine the actual code at the finding location + - Never report based on diff alone + - Get +-20 lines of context around the flagged line + - Verify the line number actually exists in the file + +2. **Verify the issue exists** - Not assume it does + - Is the problematic pattern actually present at this line? + - Is there validation/sanitization nearby you missed? + - Does the framework provide automatic protection? + +3. **Provide code evidence** - Copy-paste the actual code + - Your `evidence` field must contain real code from the file + - Not descriptions like "the code does X" but actual `const query = ...` + - If you can't provide real code, you haven't verified the issue + +4. **Check for mitigations** - Use Grep to search for: + - Validation functions that might sanitize this input + - Framework-level protections + - Comments explaining why code appears unsafe + +**Your evidence must prove the issue exists - not just that you suspect it.** + ## Code Patterns to Flag ### JavaScript/TypeScript diff --git a/apps/backend/prompts/github/pr_security_agent.md b/apps/backend/prompts/github/pr_security_agent.md index 15061038..28bd2713 100644 --- a/apps/backend/prompts/github/pr_security_agent.md +++ b/apps/backend/prompts/github/pr_security_agent.md @@ -108,6 +108,33 @@ When your finding claims protection is **missing** (no validation, no sanitizati - Check if mitigation exists elsewhere (e.g., WAF, input validation) - Review framework security features (does React escape by default?) + +## CRITICAL: Full Context Analysis + +Before reporting ANY finding, you MUST: + +1. **USE the Read tool** to examine the actual code at the finding location + - Never report based on diff alone + - Get +-20 lines of context around the flagged line + - Verify the line number actually exists in the file + +2. **Verify the issue exists** - Not assume it does + - Is the problematic pattern actually present at this line? + - Is there validation/sanitization nearby you missed? + - Does the framework provide automatic protection? + +3. **Provide code evidence** - Copy-paste the actual code + - Your `evidence` field must contain real code from the file + - Not descriptions like "the code does X" but actual `const query = ...` + - If you can't provide real code, you haven't verified the issue + +4. **Check for mitigations** - Use Grep to search for: + - Validation functions that might sanitize this input + - Framework-level protections + - Comments explaining why code appears unsafe + +**Your evidence must prove the issue exists - not just that you suspect it.** + ## Code Patterns to Flag ### JavaScript/TypeScript diff --git a/apps/backend/runners/github/context_gatherer.py b/apps/backend/runners/github/context_gatherer.py index f80e4d0c..e71d0d55 100644 --- a/apps/backend/runners/github/context_gatherer.py +++ b/apps/backend/runners/github/context_gatherer.py @@ -16,8 +16,10 @@ Responsibilities: from __future__ import annotations +import ast import asyncio import json +import os import re from dataclasses import dataclass, field from pathlib import Path @@ -828,6 +830,7 @@ class PRContextGatherer: - Imported modules and dependencies - Configuration files in the same directory - Related type definition files + - Reverse dependencies (files that import changed files) """ related = set() @@ -848,12 +851,15 @@ class PRContextGatherer: if path.suffix in [".ts", ".tsx"]: related.update(self._find_type_definitions(path)) + # Find reverse dependencies (files that import this file) + related.update(self._find_dependents(changed_file.path)) + # Remove files that are already in changed_files changed_paths = {cf.path for cf in changed_files} related = {r for r in related if r not in changed_paths} - # Limit to 20 most relevant files - return sorted(related)[:20] + # Use smart prioritization with increased limit (50 instead of 20) + return self._prioritize_related_files(related, limit=50) def _find_test_files(self, source_path: Path) -> set[str]: """Find test files related to a source file.""" @@ -882,28 +888,113 @@ class PRContextGatherer: Find imported files from source code. Supports: - - JavaScript/TypeScript: import statements - - Python: import statements + - JavaScript/TypeScript: ES6 imports, path aliases, CommonJS, re-exports + - Python: import statements via AST """ imports = set() if source_path.suffix in [".ts", ".tsx", ".js", ".jsx"]: - # Match: import ... from './file' or from '../file' - # Only relative imports (starting with . or ..) - pattern = r"from\s+['\"](\.[^'\"]+)['\"]" - for match in re.finditer(pattern, content): + # Load tsconfig paths once for this file (for alias resolution) + ts_paths = self._load_tsconfig_paths() + + # Pattern 1: ES6 relative imports (existing) + # Matches: from './file', from '../file' + relative_pattern = r"from\s+['\"](\.[^'\"]+)['\"]" + for match in re.finditer(relative_pattern, content): import_path = match.group(1) resolved = self._resolve_import_path(import_path, source_path) if resolved: imports.add(resolved) + # Pattern 2: Path alias imports (NEW) + # Matches: from '@/utils', from '~/config', from '@shared/types' + alias_pattern = r"from\s+['\"](@[^'\"]+|~[^'\"]+)['\"]" + if ts_paths: + for match in re.finditer(alias_pattern, content): + import_path = match.group(1) + resolved = self._resolve_alias_import(import_path, ts_paths) + if resolved: + imports.add(resolved) + + # Pattern 3: CommonJS require (NEW) + # Matches: require('./utils'), require('@/config') + require_pattern = r"require\s*\(\s*['\"]([^'\"]+)['\"]\s*\)" + for match in re.finditer(require_pattern, content): + import_path = match.group(1) + resolved = self._resolve_any_import(import_path, source_path, ts_paths) + if resolved: + imports.add(resolved) + + # Pattern 4: Re-exports (NEW) + # Matches: export * from './module', export { x } from './module' + reexport_pattern = r"export\s+(?:\*|\{[^}]*\})\s+from\s+['\"]([^'\"]+)['\"]" + for match in re.finditer(reexport_pattern, content): + import_path = match.group(1) + resolved = self._resolve_any_import(import_path, source_path, ts_paths) + if resolved: + imports.add(resolved) + elif source_path.suffix == ".py": - # Python relative imports are complex, skip for now - # Could add support for "from . import" later - pass + # Python imports via AST + imports.update(self._find_python_imports(content, source_path)) return imports + def _resolve_alias_import( + self, import_path: str, ts_paths: dict[str, list[str]] + ) -> str | None: + """ + Resolve a path alias import to an actual file path. + + Path aliases (e.g., @/utils, ~/config) are project-root relative, + not relative to the importing file. + + Args: + import_path: Path alias import like '@/utils' or '~/config' + ts_paths: tsconfig paths mapping + + Returns: + Resolved path relative to project root, or None if not found + """ + resolved_alias = self._resolve_path_alias(import_path, ts_paths) + if not resolved_alias: + return None + + # Path aliases are project-root relative, so resolve from root + # by using an empty base path (Path(".").parent = Path(".")) + return self._resolve_import_path("./" + resolved_alias, Path(".")) + + def _resolve_any_import( + self, import_path: str, source_path: Path, ts_paths: dict[str, list[str]] | None + ) -> str | None: + """ + Resolve any import path (relative, alias, or node_modules). + + Handles all import types: + - Relative: './utils', '../config' + - Path aliases: '@/utils', '~/config' + - Node modules: 'lodash' (returns None - not project files) + + Args: + import_path: The import path from the source code + source_path: Path of the file doing the importing + ts_paths: tsconfig paths mapping, or None + + Returns: + Resolved path relative to project root, or None if not found/external + """ + if import_path.startswith("."): + # Relative import + return self._resolve_import_path(import_path, source_path) + elif import_path.startswith("@") or import_path.startswith("~"): + # Path alias import + if ts_paths: + return self._resolve_alias_import(import_path, ts_paths) + return None + else: + # Node modules package - skip + return None + def _resolve_import_path(self, import_path: str, source_path: Path) -> str | None: """ Resolve a relative import path to an absolute file path. @@ -976,6 +1067,391 @@ class PRContextGatherer: return set() + def _find_dependents(self, file_path: str, max_results: int = 15) -> set[str]: + """ + Find files that import the given file (reverse dependencies). + + Uses pure Python to search for import statements referencing this file. + Cross-platform compatible (Windows, macOS, Linux). + Limited to prevent performance issues on large codebases. + + Args: + file_path: Path of the file to find dependents for + max_results: Maximum number of dependents to return + + Returns: + Set of file paths that import this file. + """ + dependents: set[str] = set() + path_obj = Path(file_path) + stem = path_obj.stem # e.g., 'helpers' from 'utils/helpers.ts' + + # Skip if stem is too generic (would match too many files) + if stem in ["index", "main", "app", "utils", "helpers", "types", "constants"]: + return dependents + + # Build regex patterns and file extensions based on file type + pattern = None + file_extensions = [] + + if path_obj.suffix in [".ts", ".tsx", ".js", ".jsx"]: + # Match various import styles for JS/TS + # from './helpers', from '../utils/helpers', from '@/utils/helpers' + # Escape stem for regex safety + escaped_stem = re.escape(stem) + pattern = re.compile(rf"['\"].*{escaped_stem}['\"]") + file_extensions = [".ts", ".tsx", ".js", ".jsx"] + elif path_obj.suffix == ".py": + # Match Python imports: from .helpers import, import helpers + escaped_stem = re.escape(stem) + pattern = re.compile(rf"(from.*{escaped_stem}|import.*{escaped_stem})") + file_extensions = [".py"] + else: + return dependents + + # Directories to exclude + exclude_dirs = { + "node_modules", + ".git", + "dist", + "build", + "__pycache__", + ".venv", + "venv", + } + + # Walk the project directory + project_path = Path(self.project_dir) + files_checked = 0 + max_files_to_check = 2000 # Prevent infinite scanning on large codebases + + try: + for root, dirs, files in os.walk(project_path): + # Modify dirs in-place to exclude certain directories + dirs[:] = [d for d in dirs if d not in exclude_dirs] + + for filename in files: + # Check if we've hit the file limit + if files_checked >= max_files_to_check: + safe_print( + f"[Context] File limit reached finding dependents for {file_path}" + ) + return dependents + + # Check if file has the right extension + if not any(filename.endswith(ext) for ext in file_extensions): + continue + + file_full_path = Path(root) / filename + files_checked += 1 + + # Get relative path from project root + try: + relative_path = file_full_path.relative_to(project_path) + relative_path_str = str(relative_path).replace("\\", "/") + + # Don't include the file itself + if relative_path_str == file_path: + continue + + # Search for the pattern in the file + try: + with open( + file_full_path, encoding="utf-8", errors="ignore" + ) as f: + content = f.read() + if pattern.search(content): + dependents.add(relative_path_str) + if len(dependents) >= max_results: + return dependents + except (OSError, UnicodeDecodeError): + # Skip files that can't be read + continue + + except ValueError: + # File is not relative to project_path, skip it + continue + + except Exception as e: + safe_print(f"[Context] Error finding dependents: {e}") + + return dependents + + def _prioritize_related_files(self, files: set[str], limit: int = 50) -> list[str]: + """ + Prioritize related files by relevance. + + Priority order: + 1. Test files (most important for review context) + 2. Type definition files (.d.ts) + 3. Configuration files + 4. Direct imports/dependents + 5. Other files + + Args: + files: Set of file paths to prioritize + limit: Maximum number of files to return + + Returns: + List of files sorted by priority, limited to `limit`. + """ + test_files = [] + type_files = [] + config_files = [] + other_files = [] + + for f in files: + path = Path(f) + name_lower = path.name.lower() + + # Test files + if ( + ".test." in name_lower + or ".spec." in name_lower + or name_lower.startswith("test_") + or name_lower.endswith("_test.py") + or "__tests__" in f + ): + test_files.append(f) + # Type definition files + elif name_lower.endswith(".d.ts") or "types" in name_lower: + type_files.append(f) + # Config files + elif name_lower in [ + n.lower() for n in CONFIG_FILE_NAMES + ] or name_lower.endswith((".config.js", ".config.ts", "rc", "rc.json")): + config_files.append(f) + else: + other_files.append(f) + + # Sort within each category alphabetically for consistency, then combine + prioritized = ( + sorted(test_files) + + sorted(type_files) + + sorted(config_files) + + sorted(other_files) + ) + + return prioritized[:limit] + + def _load_json_safe(self, filename: str) -> dict | None: + """ + Load JSON file from project_dir, handling tsconfig-style comments. + + tsconfig.json allows // and /* */ comments, which standard JSON + parsers reject. This method first tries standard parsing (most + tsconfigs don't have comments), then falls back to comment stripping. + + Note: Comment stripping only handles comments outside strings to + avoid mangling path patterns like "@/*" which contain "/*". + + Args: + filename: JSON filename relative to project_dir + + Returns: + Parsed JSON as dict, or None on error + """ + try: + file_path = self.project_dir / filename + if not file_path.exists(): + return None + + content = file_path.read_text(encoding="utf-8") + + # Try standard JSON parse first (most tsconfigs don't have comments) + try: + return json.loads(content) + except json.JSONDecodeError: + pass + + # Fall back to comment stripping (outside strings only) + # First, remove block comments /* ... */ + # Simple approach: remove everything between /* and */ + # This handles multi-line block comments + while "/*" in content: + start = content.find("/*") + end = content.find("*/", start) + if end == -1: + # Unclosed block comment - remove to end + content = content[:start] + break + content = content[:start] + content[end + 2 :] + + # Then handle single-line comments + # This regex-based approach handles // comments + # outside of strings by checking for quotes + lines = content.split("\n") + cleaned_lines = [] + for line in lines: + # Strip single-line comments, but not inside strings + # Simple heuristic: if '//' appears and there's an even + # number of quotes before it, strip from there + comment_pos = line.find("//") + if comment_pos != -1: + # Count quotes before the // + before_comment = line[:comment_pos] + if before_comment.count('"') % 2 == 0: + line = before_comment + cleaned_lines.append(line) + content = "\n".join(cleaned_lines) + + return json.loads(content) + except (json.JSONDecodeError, OSError) as e: + safe_print(f"[Context] Could not load {filename}: {e}", style="dim") + return None + + def _load_tsconfig_paths(self) -> dict[str, list[str]] | None: + """ + Load path mappings from tsconfig.json. + + Handles the 'extends' field to merge paths from base configs. + + Returns: + Dict mapping path aliases to target paths, e.g.: + {"@/*": ["src/*"], "@shared/*": ["src/shared/*"]} + Returns None if no paths configured. + """ + config = self._load_json_safe("tsconfig.json") + if not config: + return None + + paths: dict[str, list[str]] = {} + + # Handle extends field - load base config first + if "extends" in config: + extends_path = config["extends"] + # Handle relative paths like "./tsconfig.base.json" + if extends_path.startswith("./"): + extends_path = extends_path[2:] + base_config = self._load_json_safe(extends_path) + if base_config: + base_paths = base_config.get("compilerOptions", {}).get("paths", {}) + paths.update(base_paths) + + # Override with current config's paths + current_paths = config.get("compilerOptions", {}).get("paths", {}) + paths.update(current_paths) + + return paths if paths else None + + def _resolve_path_alias( + self, import_path: str, paths: dict[str, list[str]] + ) -> str | None: + """ + Resolve a path alias import to an actual file path. + + Args: + import_path: Import path like '@/utils/helpers' or '~/config' + paths: tsconfig paths mapping from _load_tsconfig_paths() + + Returns: + Resolved path like 'src/utils/helpers', or None if no match + """ + for alias_pattern, target_paths in paths.items(): + # Skip empty target_paths (malformed tsconfig entry) + if not target_paths: + continue + # Convert '@/*' to regex pattern '^@/(.*)$' + regex_pattern = "^" + alias_pattern.replace("*", "(.*)") + "$" + match = re.match(regex_pattern, import_path) + if match: + suffix = match.group(1) if match.lastindex else "" + # Use first target path, replace * with suffix + target = target_paths[0].replace("*", suffix) + return target + return None + + def _resolve_python_import( + self, module_name: str, level: int, source_path: Path + ) -> str | None: + """ + Resolve a Python import to an actual file path. + + Args: + module_name: Module name like 'utils' or 'utils.helpers' + level: Import level (0=absolute, 1=from ., 2=from .., etc.) + source_path: Path of file doing the importing + + Returns: + Resolved path relative to project root, or None if not found. + """ + if level > 0: + # Relative import: from . or from .. + base_dir = source_path.parent + # level=1 means same package (.), level=2 means parent (..), etc. + for _ in range(level - 1): + base_dir = base_dir.parent + + if module_name: + # from .module import x -> look for module.py or module/__init__.py + parts = module_name.split(".") + candidate = base_dir / Path(*parts) + else: + # from . import x -> can't resolve without knowing what x is + return None + else: + # Absolute import - check if it's project-internal + parts = module_name.split(".") + candidate = Path(*parts) + + # Try as module file (e.g., utils.py) + file_path = self.project_dir / candidate.with_suffix(".py") + if file_path.exists() and file_path.is_file(): + try: + return str(file_path.relative_to(self.project_dir)) + except ValueError: + return None + + # Try as package directory (e.g., utils/__init__.py) + init_path = self.project_dir / candidate / "__init__.py" + if init_path.exists() and init_path.is_file(): + try: + return str(init_path.relative_to(self.project_dir)) + except ValueError: + return None + + return None + + def _find_python_imports(self, content: str, source_path: Path) -> set[str]: + """ + Find imported files from Python source code using AST. + + Uses ast.parse to extract Import and ImportFrom nodes, then resolves + them to actual file paths within the project. + + Args: + content: Python source code + source_path: Path of the file being analyzed + + Returns: + Set of resolved file paths relative to project root. + """ + imports: set[str] = set() + + try: + tree = ast.parse(content) + except SyntaxError: + # Invalid Python syntax - skip gracefully + return imports + + for node in ast.walk(tree): + if isinstance(node, ast.Import): + # import module, import module.submodule + for alias in node.names: + resolved = self._resolve_python_import(alias.name, 0, source_path) + if resolved: + imports.add(resolved) + + elif isinstance(node, ast.ImportFrom): + # from module import x, from . import x, from ..module import x + module = node.module or "" + level = node.level # 0=absolute, 1=from ., 2=from .., etc. + resolved = self._resolve_python_import(module, level, source_path) + if resolved: + imports.add(resolved) + + return imports + @staticmethod def find_related_files_for_root( changed_files: list[ChangedFile], @@ -1035,8 +1511,8 @@ class PRContextGatherer: changed_paths = {cf.path for cf in changed_files} related = {r for r in related if r not in changed_paths} - # Limit to 20 most relevant files - return sorted(related)[:20] + # Limit to 50 most relevant files (increased from 20) + return sorted(related)[:50] class FollowupContextGatherer: @@ -1285,7 +1761,7 @@ class FollowupContextGatherer: diff_since_review=diff_since_review, contributor_comments_since_review=contributor_comments + contributor_reviews, - ai_bot_comments_since_review=ai_comments, + ai_bot_comments_since_review=ai_comments + ai_reviews, pr_reviews_since_review=pr_reviews, has_merge_conflicts=has_merge_conflicts, merge_state_status=merge_state_status, diff --git a/apps/backend/runners/github/models.py b/apps/backend/runners/github/models.py index 9a384525..f1f51cc4 100644 --- a/apps/backend/runners/github/models.py +++ b/apps/backend/runners/github/models.py @@ -240,6 +240,13 @@ class PRReviewFinding: validation_evidence: str | None = None # Code snippet examined during validation validation_explanation: str | None = None # Why finding was validated/dismissed + # Cross-validation and confidence routing fields + confidence: float = 0.5 # Confidence score (0.0-1.0), defaults to medium confidence + source_agents: list[str] = field( + default_factory=list + ) # Which agents reported this finding + cross_validated: bool = False # Whether multiple agents agreed on this finding + def to_dict(self) -> dict: return { "id": self.id, @@ -260,6 +267,10 @@ class PRReviewFinding: "validation_status": self.validation_status, "validation_evidence": self.validation_evidence, "validation_explanation": self.validation_explanation, + # Cross-validation and confidence routing fields + "confidence": self.confidence, + "source_agents": self.source_agents, + "cross_validated": self.cross_validated, } @classmethod @@ -283,6 +294,10 @@ class PRReviewFinding: validation_status=data.get("validation_status"), validation_evidence=data.get("validation_evidence"), validation_explanation=data.get("validation_explanation"), + # Cross-validation and confidence routing fields + confidence=data.get("confidence", 0.5), + source_agents=data.get("source_agents", []), + cross_validated=data.get("cross_validated", False), ) diff --git a/apps/backend/runners/github/output_validator.py b/apps/backend/runners/github/output_validator.py index 4f29e508..bd626c0e 100644 --- a/apps/backend/runners/github/output_validator.py +++ b/apps/backend/runners/github/output_validator.py @@ -408,8 +408,9 @@ class FindingValidator: Returns: True if meets threshold, False otherwise """ - # If finding has explicit confidence field, use it - if hasattr(finding, "confidence") and finding.confidence: + # If finding has explicit confidence above default (0.5), use it directly + # Note: 0.5 is the default value, so we only use explicit confidence if set higher + if hasattr(finding, "confidence") and finding.confidence > 0.5: return finding.confidence >= self.HIGH_ACTIONABILITY_SCORE # Otherwise, use actionability score as proxy for confidence @@ -499,7 +500,8 @@ class FindingValidator: category_counts[finding.category.value] += 1 # Get actionability score - if hasattr(finding, "confidence") and finding.confidence: + # Note: 0.5 is the default confidence, only use explicit if set higher + if hasattr(finding, "confidence") and finding.confidence > 0.5: total_actionability += finding.confidence else: total_actionability += self._score_actionability(finding) diff --git a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py index 6a65dd47..00ed56ec 100644 --- a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py +++ b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py @@ -20,6 +20,8 @@ from __future__ import annotations import hashlib import logging import os +from collections import defaultdict +from enum import Enum from pathlib import Path from typing import Any @@ -42,7 +44,7 @@ try: from .category_utils import map_category from .io_utils import safe_print from .pr_worktree_manager import PRWorktreeManager - from .pydantic_models import ParallelOrchestratorResponse + from .pydantic_models import AgentAgreement, ParallelOrchestratorResponse from .sdk_utils import process_sdk_stream except (ImportError, ValueError, SystemError): from context_gatherer import PRContext, PRContextGatherer, _validate_git_ref @@ -61,7 +63,7 @@ except (ImportError, ValueError, SystemError): from services.category_utils import map_category from services.io_utils import safe_print from services.pr_worktree_manager import PRWorktreeManager - from services.pydantic_models import ParallelOrchestratorResponse + from services.pydantic_models import AgentAgreement, ParallelOrchestratorResponse from services.sdk_utils import process_sdk_stream @@ -74,6 +76,125 @@ DEBUG_MODE = os.environ.get("DEBUG", "").lower() in ("true", "1", "yes") PR_WORKTREE_DIR = ".auto-claude/github/pr/worktrees" +class ConfidenceTier(str, Enum): + """Confidence tiers for finding routing. + + Findings are routed based on their confidence score: + - HIGH (>=0.8): Included as-is + - MEDIUM (0.5-0.8): Included with "[Potential]" prefix + - LOW (<0.5): Logged but excluded from output + """ + + HIGH = "high" + MEDIUM = "medium" + LOW = "low" + + # Thresholds (class-level constants) + @classmethod + def get_tier(cls, confidence: float) -> ConfidenceTier: + """Get tier for a given confidence value.""" + if confidence >= 0.8: # HIGH_THRESHOLD + return cls.HIGH + elif confidence >= 0.5: # LOW_THRESHOLD + return cls.MEDIUM + else: + return cls.LOW + + +def _validate_finding_evidence(finding: PRReviewFinding) -> tuple[bool, str]: + """ + Check if finding has actual code evidence, not just descriptions. + + Returns: + Tuple of (is_valid, reason) + """ + if not finding.evidence: + return False, "No evidence provided" + + evidence = finding.evidence.strip() + if len(evidence) < 10: + return False, "Evidence too short (< 10 chars)" + + # Reject generic descriptions that aren't code + description_patterns = [ + "the code", + "this function", + "it appears", + "seems to", + "may be", + "could be", + "might be", + "appears to", + "there is", + "there are", + ] + evidence_lower = evidence.lower() + for pattern in description_patterns: + if evidence_lower.startswith(pattern): + return False, f"Evidence starts with description pattern: '{pattern}'" + + # Evidence should look like code (has syntax characters) + code_chars = [ + "=", + "(", + ")", + "{", + "}", + ";", + ":", + ".", + "->", + "::", + "[", + "]", + "'", + '"', + ] + has_code_syntax = any(char in evidence for char in code_chars) + + if not has_code_syntax: + return False, "Evidence lacks code syntax characters" + + return True, "Valid evidence" + + +def _is_finding_in_scope( + finding: PRReviewFinding, + changed_files: list[str], +) -> tuple[bool, str]: + """ + Check if finding is within PR scope. + + Args: + finding: The finding to check + changed_files: List of file paths changed in the PR + + Returns: + Tuple of (is_in_scope, reason) + """ + if not finding.file: + return False, "No file specified" + + # Check if file is in changed files + if finding.file not in changed_files: + # Allow impact findings (about how changes affect other files) + impact_keywords = ["breaks", "affects", "impact", "caller", "depends"] + description_lower = (finding.description or "").lower() + is_impact_finding = any(kw in description_lower for kw in impact_keywords) + + if not is_impact_finding: + return ( + False, + f"File '{finding.file}' not in PR changed files and not an impact finding", + ) + + # Check line number is reasonable (> 0) + if finding.line is not None and finding.line <= 0: + return False, f"Invalid line number: {finding.line}" + + return True, "In scope" + + class ParallelOrchestratorReviewer: """ PR reviewer using SDK subagents for parallel specialist analysis. @@ -188,6 +309,7 @@ class ParallelOrchestratorReviewer: logic_prompt = self._load_prompt("pr_logic_agent.md") codebase_fit_prompt = self._load_prompt("pr_codebase_fit_agent.md") ai_triage_prompt = self._load_prompt("pr_ai_triage.md") + validator_prompt = self._load_prompt("pr_finding_validator.md") return { "security-reviewer": AgentDefinition( @@ -246,6 +368,19 @@ class ParallelOrchestratorReviewer: tools=["Read", "Grep", "Glob"], model="inherit", ), + "finding-validator": AgentDefinition( + description=( + "Finding validation specialist. Re-investigates findings to validate " + "they are actually real issues, not false positives. " + "Reads the ACTUAL CODE at the finding location with fresh eyes. " + "CRITICAL: Invoke for ALL findings after specialist agents complete. " + "Can confirm findings as valid OR dismiss them as false positives." + ), + prompt=validator_prompt + or "You validate whether findings are real issues.", + tools=["Read", "Grep", "Glob"], + model="inherit", + ), } def _build_orchestrator_prompt(self, context: PRContext) -> str: @@ -677,6 +812,68 @@ The SDK will run invoked agents in parallel automatically. # Deduplicate findings unique_findings = self._deduplicate_findings(findings) + # Cross-validate findings: boost confidence when multiple agents agree + cross_validated_findings, agent_agreement = self._cross_validate_findings( + unique_findings + ) + + # Log cross-validation results + logger.info( + f"[PRReview] Cross-validation: {len(agent_agreement.agreed_findings)} multi-agent, " + f"{len(cross_validated_findings) - len(agent_agreement.agreed_findings)} single-agent" + ) + + # Log full agreement details at debug level for monitoring + logger.debug( + f"[PRReview] AgentAgreement: {agent_agreement.model_dump_json()}" + ) + + # Apply programmatic evidence and scope filters + # These catch edge cases that slip through the finding-validator + changed_file_paths = [f.path for f in context.changed_files] + validated_findings = [] + filtered_findings = [] + + for finding in cross_validated_findings: + # Check evidence quality + evidence_valid, evidence_reason = _validate_finding_evidence(finding) + if not evidence_valid: + logger.info( + f"[PRReview] Filtered finding {finding.id}: {evidence_reason}" + ) + filtered_findings.append((finding, evidence_reason)) + continue + + # Check scope + scope_valid, scope_reason = _is_finding_in_scope( + finding, changed_file_paths + ) + if not scope_valid: + logger.info( + f"[PRReview] Filtered finding {finding.id}: {scope_reason}" + ) + filtered_findings.append((finding, scope_reason)) + continue + + validated_findings.append(finding) + + logger.info( + f"[PRReview] Findings: {len(validated_findings)} valid, " + f"{len(filtered_findings)} filtered" + ) + + # Apply confidence routing to filter low-confidence findings + # and mark medium-confidence findings with "[Potential]" prefix + routed_findings = self._apply_confidence_routing(validated_findings) + + logger.info( + f"[PRReview] Confidence routing: {len(routed_findings)} included, " + f"{len(validated_findings) - len(routed_findings)} dropped (low confidence)" + ) + + # Use routed findings for verdict and summary + unique_findings = routed_findings + logger.info( f"[ParallelOrchestrator] Review complete: {len(unique_findings)} findings" ) @@ -928,6 +1125,166 @@ The SDK will run invoked agents in parallel automatically. return unique + def _cross_validate_findings( + self, findings: list[PRReviewFinding] + ) -> tuple[list[PRReviewFinding], AgentAgreement]: + """ + Cross-validate findings to boost confidence when multiple agents agree. + + Groups findings by location key (file, line, category) and: + - For groups with 2+ findings: merges into one, boosts confidence by 0.15, + sets cross_validated=True, collects all source agents + - For single-agent findings: keeps as-is, ensures source_agents is populated + + Args: + findings: List of deduplicated findings to cross-validate + + Returns: + Tuple of (cross-validated findings, AgentAgreement tracking object) + """ + # Confidence boost for multi-agent agreement + CONFIDENCE_BOOST = 0.15 + MAX_CONFIDENCE = 0.95 + + # Group findings by location key: (file, line, category) + groups: dict[tuple, list[PRReviewFinding]] = defaultdict(list) + for finding in findings: + key = (finding.file, finding.line, finding.category.value) + groups[key].append(finding) + + validated_findings: list[PRReviewFinding] = [] + agreed_finding_ids: list[str] = [] + + for key, group in groups.items(): + if len(group) >= 2: + # Multi-agent agreement: merge findings + # Sort by severity to keep highest severity finding + severity_order = { + ReviewSeverity.CRITICAL: 0, + ReviewSeverity.HIGH: 1, + ReviewSeverity.MEDIUM: 2, + ReviewSeverity.LOW: 3, + } + group.sort(key=lambda f: severity_order.get(f.severity, 99)) + primary = group[0] + + # Collect all source agents from group + all_agents: list[str] = [] + for f in group: + if f.source_agents: + for agent in f.source_agents: + if agent not in all_agents: + all_agents.append(agent) + + # Combine evidence from all findings + all_evidence: list[str] = [] + for f in group: + if f.evidence and f.evidence.strip(): + all_evidence.append(f.evidence.strip()) + combined_evidence = ( + "\n---\n".join(all_evidence) if all_evidence else None + ) + + # Combine descriptions + all_descriptions: list[str] = [primary.description] + for f in group[1:]: + if f.description and f.description not in all_descriptions: + all_descriptions.append(f.description) + combined_description = " | ".join(all_descriptions) + + # Boost confidence (capped at MAX_CONFIDENCE) + base_confidence = primary.confidence or 0.5 + boosted_confidence = min( + base_confidence + CONFIDENCE_BOOST, MAX_CONFIDENCE + ) + + # Update the primary finding with merged data + primary.confidence = boosted_confidence + primary.cross_validated = True + primary.source_agents = all_agents + primary.evidence = combined_evidence + primary.description = combined_description + + validated_findings.append(primary) + agreed_finding_ids.append(primary.id) + + logger.debug( + f"[PRReview] Cross-validated finding {primary.id}: " + f"merged {len(group)} findings, agents={all_agents}, " + f"confidence={boosted_confidence:.2f}" + ) + else: + # Single-agent finding: keep as-is + finding = group[0] + + # Ensure source_agents is populated (use empty list if not set) + if not finding.source_agents: + finding.source_agents = [] + + validated_findings.append(finding) + + # Create agent agreement tracking object + agent_agreement = AgentAgreement( + agreed_findings=agreed_finding_ids, + conflicting_findings=[], # Not implemented yet - reserved for future + resolution_notes=None, + ) + + return validated_findings, agent_agreement + + def _apply_confidence_routing( + self, findings: list[PRReviewFinding] + ) -> list[PRReviewFinding]: + """ + Route findings based on confidence scores. + + - HIGH (>=0.8): Keep as-is, include in output + - MEDIUM (0.5-0.8): Prepend "[Potential] " to title, include in output + - LOW (<0.5): Log with logger.info(), exclude from output + + Args: + findings: List of findings to route + + Returns: + Filtered list of findings (HIGH and MEDIUM only) + """ + routed = [] + tier_counts = {"high": 0, "medium": 0, "low": 0} + + for finding in findings: + # Handle missing confidence gracefully (default to 0.5) + confidence = getattr(finding, "confidence", 0.5) + if confidence is None: + confidence = 0.5 + confidence = self._normalize_confidence(confidence) + + tier = ConfidenceTier.get_tier(confidence) + tier_counts[tier] += 1 + + if tier == ConfidenceTier.HIGH: + # HIGH: Include as-is + routed.append(finding) + elif tier == ConfidenceTier.MEDIUM: + # MEDIUM: Prepend "[Potential] " to title + if not finding.title.startswith("[Potential] "): + finding.title = f"[Potential] {finding.title}" + routed.append(finding) + else: + # LOW: Log and exclude + logger.info( + f"[PRReview] Dropping low-confidence finding: " + f"'{finding.title}' (confidence={confidence:.2f}, " + f"file={finding.file}:{finding.line})" + ) + + logger.info( + f"[PRReview] Confidence routing: HIGH={tier_counts['high']}, " + f"MEDIUM={tier_counts['medium']}, LOW={tier_counts['low']} " + f"(dropped {tier_counts['low']} low-confidence findings)" + ) + + return routed + def _generate_verdict( self, findings: list[PRReviewFinding], diff --git a/tests/test_client.py b/tests/test_client.py index ca00ad2b..b3495548 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -9,14 +9,30 @@ Tests the client.py and simple_client.py module functionality including: - Client creation with valid tokens """ +import os from unittest.mock import MagicMock, patch import pytest +# Auth token env vars that need to be cleared between tests +AUTH_TOKEN_ENV_VARS = [ + "CLAUDE_CODE_OAUTH_TOKEN", + "ANTHROPIC_AUTH_TOKEN", +] + class TestClientTokenValidation: """Tests for client token validation.""" + @pytest.fixture(autouse=True) + def clear_env(self): + """Clear auth environment variables before and after each test.""" + for var in AUTH_TOKEN_ENV_VARS: + os.environ.pop(var, None) + yield + for var in AUTH_TOKEN_ENV_VARS: + os.environ.pop(var, None) + def test_create_client_rejects_encrypted_tokens(self, tmp_path, monkeypatch): """Verify create_client() rejects encrypted tokens.""" from core.client import create_client @@ -24,6 +40,12 @@ class TestClientTokenValidation: monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", "enc:test123456789012") # Mock keychain to ensure encrypted token is the only source monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + # Mock decrypt_token to raise ValueError (simulates decryption failure) + # This ensures the encrypted token flows through to validate_token_not_encrypted + monkeypatch.setattr( + "core.auth.decrypt_token", + lambda t: (_ for _ in ()).throw(ValueError("Decryption not supported")), + ) with pytest.raises(ValueError, match="encrypted format"): create_client(tmp_path, tmp_path, "claude-sonnet-4", "coder") @@ -35,6 +57,11 @@ class TestClientTokenValidation: monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", "enc:test123456789012") # Mock keychain to ensure encrypted token is the only source monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + # Mock decrypt_token to raise ValueError (simulates decryption failure) + monkeypatch.setattr( + "core.auth.decrypt_token", + lambda t: (_ for _ in ()).throw(ValueError("Decryption not supported")), + ) with pytest.raises(ValueError, match="encrypted format"): create_simple_client(agent_type="merge_resolver") diff --git a/tests/test_context_gatherer.py b/tests/test_context_gatherer.py new file mode 100644 index 00000000..55ae9773 --- /dev/null +++ b/tests/test_context_gatherer.py @@ -0,0 +1,237 @@ +#!/usr/bin/env python3 +""" +Tests for GitHub PR Context Gatherer +===================================== + +Tests the context gathering logic, specifically: +- AI bot review detection and inclusion in follow-up context +- Separation of AI bot vs contributor feedback +""" + +import sys +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch +from datetime import datetime +import tempfile + +import pytest + +# Add the backend directory to path +_backend_dir = Path(__file__).parent.parent / "apps" / "backend" +_github_dir = _backend_dir / "runners" / "github" +if str(_github_dir) not in sys.path: + sys.path.insert(0, str(_github_dir)) +if str(_backend_dir) not in sys.path: + sys.path.insert(0, str(_backend_dir)) + +from context_gatherer import AI_BOT_PATTERNS, FollowupContextGatherer +from models import PRReviewResult, FollowupReviewContext + + +class TestAIReviewsInclusion: + """Tests that AI bot formal reviews are included in follow-up context.""" + + def test_ai_bot_patterns_include_known_bots(self): + """Verify AI bot patterns include common AI review tools.""" + # CodeRabbit + assert "coderabbitai" in AI_BOT_PATTERNS + # Cursor/Gemini + assert any("gemini" in p for p in AI_BOT_PATTERNS.keys()) + # GitHub Copilot + assert "copilot" in AI_BOT_PATTERNS + + def test_followup_context_includes_ai_reviews_field(self): + """Verify FollowupReviewContext has ai_bot_comments_since_review field.""" + # Create a minimal previous review + previous_review = PRReviewResult( + pr_number=42, + repo="test/repo", + success=True, + findings=[], + summary="Test", + overall_status="approve", + reviewed_commit_sha="abc123", + reviewed_at=datetime.now().isoformat(), + ) + + # Create context with AI reviews included + context = FollowupReviewContext( + pr_number=42, + previous_review=previous_review, + previous_commit_sha="abc123", + current_commit_sha="def456", + ai_bot_comments_since_review=[ + {"user": {"login": "coderabbitai[bot]"}, "body": "AI review content"} + ], + ) + + # Verify AI reviews are accessible + assert len(context.ai_bot_comments_since_review) == 1 + assert context.ai_bot_comments_since_review[0]["body"] == "AI review content" + + @pytest.mark.asyncio + async def test_gather_followup_context_includes_ai_reviews(self): + """Test that FollowupContextGatherer.gather() includes AI formal reviews. + + This is the key test that verifies the fix for the bug where AI formal reviews + (from CodeRabbit, Cursor, etc.) were fetched but not included in the context. + """ + # Create a minimal previous review + previous_review = PRReviewResult( + pr_number=42, + repo="test/repo", + success=True, + findings=[], + summary="Test", + overall_status="approve", + reviewed_commit_sha="abc123", + reviewed_at=datetime.now().isoformat(), + ) + + # Create mock GitHub client + mock_gh_client = AsyncMock() + + # Mock get_pr_head_sha + mock_gh_client.get_pr_head_sha.return_value = "def456" + + # Mock PR info for merge status check + mock_gh_client.pr_get.return_value = { + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + } + + # Mock PR files changed since + mock_gh_client.get_pr_files_changed_since.return_value = ([], []) # (files, commits) + + # Mock comments since review - includes an AI bot comment + mock_gh_client.get_comments_since.return_value = { + "review_comments": [ + { + "id": 1, + "user": {"login": "coderabbitai[bot]"}, + "body": "AI inline comment", + } + ], + "issue_comments": [], + } + + # Mock formal PR reviews - THIS IS THE KEY DATA + # These are formal review submissions (not inline comments) + mock_gh_client.get_reviews_since.return_value = [ + { + "id": 100, + "user": {"login": "coderabbitai[bot]"}, + "body": "## CodeRabbit Summary\n\nThis PR looks good overall.", + "state": "COMMENTED", + }, + { + "id": 101, + "user": {"login": "gemini-code-assist[bot]"}, + "body": "## Gemini Review\n\nNo issues found.", + "state": "APPROVED", + }, + { + "id": 102, + "user": {"login": "human-reviewer"}, + "body": "LGTM", + "state": "APPROVED", + }, + ] + + # Create context gatherer with mocked GHClient + with tempfile.TemporaryDirectory() as tmpdir: + with patch("context_gatherer.GHClient", return_value=mock_gh_client): + gatherer = FollowupContextGatherer( + project_dir=Path(tmpdir), + pr_number=42, + previous_review=previous_review, + repo="test/repo", + ) + + # Replace the gh_client with our mock after init + gatherer.gh_client = mock_gh_client + + # Call the method under test + context = await gatherer.gather() + + # ASSERTION: AI formal reviews should be in ai_bot_comments_since_review + # The fix ensures ai_comments + ai_reviews are concatenated + ai_feedback = context.ai_bot_comments_since_review + + # Should include: + # - 1 AI inline comment (coderabbitai) + # - 2 AI formal reviews (coderabbitai, gemini-code-assist) + # Total = 3 AI feedback items + assert len(ai_feedback) == 3, ( + f"Expected 3 AI feedback items (1 comment + 2 reviews), got {len(ai_feedback)}" + ) + + # Verify the AI reviews are included (not just comments) + ai_bodies = [item.get("body", "") for item in ai_feedback] + assert any("CodeRabbit Summary" in body for body in ai_bodies), ( + "CodeRabbit formal review should be in ai_bot_comments_since_review" + ) + assert any("Gemini Review" in body for body in ai_bodies), ( + "Gemini formal review should be in ai_bot_comments_since_review" + ) + + # Verify contributor review is NOT in AI feedback + assert not any("LGTM" in body for body in ai_bodies), ( + "Human reviewer comment should not be in ai_bot_comments_since_review" + ) + + # Verify contributor review IS in contributor_comments + contributor_feedback = context.contributor_comments_since_review + contributor_bodies = [item.get("body", "") for item in contributor_feedback] + assert any("LGTM" in body for body in contributor_bodies), ( + "Human reviewer comment should be in contributor_comments_since_review" + ) + + @pytest.mark.asyncio + async def test_ai_reviews_counted_correctly_in_logs(self): + """Test that the logging correctly counts AI feedback including reviews.""" + previous_review = PRReviewResult( + pr_number=42, + repo="test/repo", + success=True, + findings=[], + summary="Test", + overall_status="approve", + reviewed_commit_sha="abc123", + reviewed_at=datetime.now().isoformat(), + ) + + mock_gh_client = AsyncMock() + mock_gh_client.get_pr_head_sha.return_value = "def456" + mock_gh_client.pr_get.return_value = { + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + } + mock_gh_client.get_pr_files_changed_since.return_value = ([], []) + mock_gh_client.get_comments_since.return_value = { + "review_comments": [], + "issue_comments": [], + } + # 2 AI reviews, 1 contributor review + mock_gh_client.get_reviews_since.return_value = [ + {"id": 1, "user": {"login": "coderabbitai[bot]"}, "body": "AI 1", "state": "COMMENTED"}, + {"id": 2, "user": {"login": "copilot[bot]"}, "body": "AI 2", "state": "COMMENTED"}, + {"id": 3, "user": {"login": "developer"}, "body": "Human", "state": "APPROVED"}, + ] + + with tempfile.TemporaryDirectory() as tmpdir: + with patch("context_gatherer.GHClient", return_value=mock_gh_client): + gatherer = FollowupContextGatherer( + project_dir=Path(tmpdir), + pr_number=42, + previous_review=previous_review, + repo="test/repo", + ) + gatherer.gh_client = mock_gh_client + context = await gatherer.gather() + + # 2 AI reviews should be in ai_bot_comments_since_review + assert len(context.ai_bot_comments_since_review) == 2 + + # 1 contributor review should be in contributor_comments_since_review + assert len(context.contributor_comments_since_review) == 1 diff --git a/tests/test_integration_phase4.py b/tests/test_integration_phase4.py new file mode 100644 index 00000000..9966bad0 --- /dev/null +++ b/tests/test_integration_phase4.py @@ -0,0 +1,1132 @@ +""" +Integration Tests for PR Review System - Phase 4 +================================================= + +Tests validating all Phase 1-3 features work correctly: +- Phase 1: Confidence routing, evidence validation, scope filtering +- Phase 2: Import detection (path aliases, Python), reverse dependencies +- Phase 3: Multi-agent cross-validation +""" + +import sys +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +# Add the backend directory to path for imports +backend_path = Path(__file__).parent.parent / "apps" / "backend" +sys.path.insert(0, str(backend_path)) + +# Import directly to avoid loading the full runners module with its dependencies +import importlib.util + +# Load file_lock first (models.py depends on it) +file_lock_spec = importlib.util.spec_from_file_location( + "file_lock", + backend_path / "runners" / "github" / "file_lock.py" +) +file_lock_module = importlib.util.module_from_spec(file_lock_spec) +sys.modules['file_lock'] = file_lock_module +file_lock_spec.loader.exec_module(file_lock_module) + +# Load models next +models_spec = importlib.util.spec_from_file_location( + "models", + backend_path / "runners" / "github" / "models.py" +) +models_module = importlib.util.module_from_spec(models_spec) +sys.modules['models'] = models_module +models_spec.loader.exec_module(models_module) +PRReviewFinding = models_module.PRReviewFinding +PRReviewResult = models_module.PRReviewResult +ReviewSeverity = models_module.ReviewSeverity +ReviewCategory = models_module.ReviewCategory + +# Load services module dependencies for parallel_orchestrator_reviewer +category_utils_spec = importlib.util.spec_from_file_location( + "category_utils", + backend_path / "runners" / "github" / "services" / "category_utils.py" +) +category_utils_module = importlib.util.module_from_spec(category_utils_spec) +sys.modules['services.category_utils'] = category_utils_module +category_utils_spec.loader.exec_module(category_utils_module) + +# Load io_utils +io_utils_spec = importlib.util.spec_from_file_location( + "io_utils", + backend_path / "runners" / "github" / "services" / "io_utils.py" +) +io_utils_module = importlib.util.module_from_spec(io_utils_spec) +sys.modules['services.io_utils'] = io_utils_module +io_utils_spec.loader.exec_module(io_utils_module) + +# Load pydantic_models +pydantic_models_spec = importlib.util.spec_from_file_location( + "pydantic_models", + backend_path / "runners" / "github" / "services" / "pydantic_models.py" +) +pydantic_models_module = importlib.util.module_from_spec(pydantic_models_spec) +sys.modules['services.pydantic_models'] = pydantic_models_module +pydantic_models_spec.loader.exec_module(pydantic_models_module) +AgentAgreement = pydantic_models_module.AgentAgreement + + +# Load parallel_orchestrator_reviewer (contains ConfidenceTier, validation functions) +orchestrator_spec = importlib.util.spec_from_file_location( + "parallel_orchestrator_reviewer", + backend_path / "runners" / "github" / "services" / "parallel_orchestrator_reviewer.py" +) +orchestrator_module = importlib.util.module_from_spec(orchestrator_spec) +# Mock dependencies that aren't needed for unit testing +# IMPORTANT: Save and restore ALL mocked modules to avoid polluting sys.modules for other tests +_modules_to_mock = [ + 'context_gatherer', + 'core.client', + 'gh_client', + 'phase_config', + 'services.pr_worktree_manager', + 'services.sdk_utils', + 'claude_agent_sdk', +] +_original_modules = {name: sys.modules.get(name) for name in _modules_to_mock} +for name in _modules_to_mock: + sys.modules[name] = MagicMock() +orchestrator_spec.loader.exec_module(orchestrator_module) +# Restore all mocked modules to avoid polluting other tests +for name in _modules_to_mock: + if _original_modules[name] is not None: + sys.modules[name] = _original_modules[name] + elif name in sys.modules: + del sys.modules[name] +ConfidenceTier = orchestrator_module.ConfidenceTier +_validate_finding_evidence = orchestrator_module._validate_finding_evidence +_is_finding_in_scope = orchestrator_module._is_finding_in_scope + + +# ============================================================================= +# Phase 1 Tests: Confidence Routing, Evidence Validation, Scope Filtering +# ============================================================================= + +class TestConfidenceTierRouting: + """Test confidence tier routing logic (Phase 1).""" + + def test_high_confidence_tier(self): + """Verify confidence >= 0.8 returns HIGH tier.""" + assert ConfidenceTier.get_tier(0.8) == ConfidenceTier.HIGH + assert ConfidenceTier.get_tier(0.85) == ConfidenceTier.HIGH + assert ConfidenceTier.get_tier(0.95) == ConfidenceTier.HIGH + assert ConfidenceTier.get_tier(1.0) == ConfidenceTier.HIGH + + def test_medium_confidence_tier(self): + """Verify confidence 0.5-0.8 returns MEDIUM tier.""" + assert ConfidenceTier.get_tier(0.5) == ConfidenceTier.MEDIUM + assert ConfidenceTier.get_tier(0.6) == ConfidenceTier.MEDIUM + assert ConfidenceTier.get_tier(0.7) == ConfidenceTier.MEDIUM + assert ConfidenceTier.get_tier(0.79) == ConfidenceTier.MEDIUM + + def test_low_confidence_tier(self): + """Verify confidence < 0.5 returns LOW tier.""" + assert ConfidenceTier.get_tier(0.0) == ConfidenceTier.LOW + assert ConfidenceTier.get_tier(0.1) == ConfidenceTier.LOW + assert ConfidenceTier.get_tier(0.3) == ConfidenceTier.LOW + assert ConfidenceTier.get_tier(0.49) == ConfidenceTier.LOW + + def test_boundary_values(self): + """Test exact boundary values: 0.5 (MEDIUM) and 0.8 (HIGH).""" + # 0.5 is MEDIUM threshold (inclusive) + assert ConfidenceTier.get_tier(0.5) == ConfidenceTier.MEDIUM + # 0.8 is HIGH threshold (inclusive) + assert ConfidenceTier.get_tier(0.8) == ConfidenceTier.HIGH + # Just below boundaries + assert ConfidenceTier.get_tier(0.4999) == ConfidenceTier.LOW + assert ConfidenceTier.get_tier(0.7999) == ConfidenceTier.MEDIUM + + def test_tier_constants_values(self): + """Verify tier constant values match expected strings.""" + assert ConfidenceTier.HIGH == "high" + assert ConfidenceTier.MEDIUM == "medium" + assert ConfidenceTier.LOW == "low" + + def test_threshold_values(self): + """Verify threshold values through behavior (0.8 for HIGH, 0.5 for LOW).""" + # HIGH threshold is 0.8 + assert ConfidenceTier.get_tier(0.8) == ConfidenceTier.HIGH + assert ConfidenceTier.get_tier(0.79) == ConfidenceTier.MEDIUM + + # LOW threshold is 0.5 + assert ConfidenceTier.get_tier(0.5) == ConfidenceTier.MEDIUM + assert ConfidenceTier.get_tier(0.49) == ConfidenceTier.LOW + + +class TestEvidenceValidation: + """Test evidence validation logic (Phase 1).""" + + @pytest.fixture + def make_finding(self): + """Factory fixture to create PRReviewFinding instances.""" + def _make_finding(evidence: str | None = None, **kwargs): + defaults = { + "id": "TEST001", + "severity": ReviewSeverity.MEDIUM, + "category": ReviewCategory.QUALITY, + "title": "Test Finding", + "description": "Test description", + "file": "src/test.py", + "line": 10, + "evidence": evidence, + } + defaults.update(kwargs) + return PRReviewFinding(**defaults) + return _make_finding + + def test_valid_evidence_with_code_syntax(self, make_finding): + """Evidence with =, (), {} should pass validation.""" + # Assignment operator + finding = make_finding(evidence="const x = getValue()") + is_valid, reason = _validate_finding_evidence(finding) + assert is_valid, f"Failed: {reason}" + + # Function call + finding = make_finding(evidence="someFunction(arg1, arg2)") + is_valid, reason = _validate_finding_evidence(finding) + assert is_valid, f"Failed: {reason}" + + # Object/dict literal + finding = make_finding(evidence="config = { 'key': 'value' }") + is_valid, reason = _validate_finding_evidence(finding) + assert is_valid, f"Failed: {reason}" + + def test_invalid_evidence_no_code_syntax(self, make_finding): + """Prose-only evidence without code syntax should fail.""" + finding = make_finding(evidence="This code is problematic and needs fixing") + is_valid, reason = _validate_finding_evidence(finding) + assert not is_valid + assert "lacks code syntax" in reason.lower() + + def test_empty_evidence_fails(self, make_finding): + """Empty or short evidence should fail validation.""" + # No evidence + finding = make_finding(evidence=None) + is_valid, reason = _validate_finding_evidence(finding) + assert not is_valid + assert "no evidence" in reason.lower() + + # Empty string + finding = make_finding(evidence="") + is_valid, reason = _validate_finding_evidence(finding) + assert not is_valid + + # Too short (< 10 chars) + finding = make_finding(evidence="x = 1") + is_valid, reason = _validate_finding_evidence(finding) + assert not is_valid + assert "too short" in reason.lower() + + def test_evidence_with_function_def(self, make_finding): + """Evidence with 'def ' or 'function ' patterns should pass.""" + # Python function definition + finding = make_finding(evidence="def vulnerable_function(user_input):") + is_valid, reason = _validate_finding_evidence(finding) + assert is_valid, f"Failed: {reason}" + + # JavaScript function + finding = make_finding(evidence="function handleRequest(req, res) {") + is_valid, reason = _validate_finding_evidence(finding) + assert is_valid, f"Failed: {reason}" + + def test_evidence_rejects_description_patterns(self, make_finding): + """Evidence starting with vague patterns should be rejected.""" + patterns = [ + "The code has an issue with security", + "This function could be improved", + "It appears there is a vulnerability", + "Seems to be missing error handling", + ] + for pattern in patterns: + finding = make_finding(evidence=pattern) + is_valid, reason = _validate_finding_evidence(finding) + assert not is_valid, f"Should reject: {pattern}" + assert "description pattern" in reason.lower() or "lacks code" in reason.lower() + + def test_evidence_with_various_syntax_chars(self, make_finding): + """Test various code syntax characters are recognized.""" + # Semicolon + finding = make_finding(evidence="let x = 5; let y = 10;") + is_valid, _ = _validate_finding_evidence(finding) + assert is_valid + + # Colon (Python dict/type hint) + finding = make_finding(evidence="config: Dict[str, int]") + is_valid, _ = _validate_finding_evidence(finding) + assert is_valid + + # Arrow + finding = make_finding(evidence="result->getValue()") + is_valid, _ = _validate_finding_evidence(finding) + assert is_valid + + # Brackets + finding = make_finding(evidence="array[0] = items[index]") + is_valid, _ = _validate_finding_evidence(finding) + assert is_valid + + +class TestScopeFiltering: + """Test scope filtering logic (Phase 1).""" + + @pytest.fixture + def make_finding(self): + """Factory fixture to create PRReviewFinding instances.""" + def _make_finding(file: str = "src/test.py", line: int = 10, **kwargs): + defaults = { + "id": "TEST001", + "severity": ReviewSeverity.MEDIUM, + "category": ReviewCategory.QUALITY, + "title": "Test Finding", + "description": "Test description", + "file": file, + "line": line, + } + defaults.update(kwargs) + return PRReviewFinding(**defaults) + return _make_finding + + def test_finding_in_changed_files_passes(self, make_finding): + """Finding for a file in changed_files should pass.""" + changed_files = ["src/auth.py", "src/utils.py", "tests/test_auth.py"] + finding = make_finding(file="src/auth.py", line=15) + + is_valid, reason = _is_finding_in_scope(finding, changed_files) + assert is_valid, f"Failed: {reason}" + + def test_finding_outside_changed_files_filtered(self, make_finding): + """Finding for a file NOT in changed_files should be filtered.""" + changed_files = ["src/auth.py", "src/utils.py"] + finding = make_finding( + file="src/database.py", + line=10, + description="This code has a bug" + ) + + is_valid, reason = _is_finding_in_scope(finding, changed_files) + assert not is_valid + assert "not in pr changed files" in reason.lower() + + def test_invalid_line_number_filtered(self, make_finding): + """Finding with invalid line number (<=0) should be filtered.""" + changed_files = ["src/test.py"] + + # Zero line + finding = make_finding(file="src/test.py", line=0) + is_valid, reason = _is_finding_in_scope(finding, changed_files) + assert not is_valid + assert "invalid line" in reason.lower() + + # Negative line + finding = make_finding(file="src/test.py", line=-5) + is_valid, reason = _is_finding_in_scope(finding, changed_files) + assert not is_valid + + def test_impact_finding_allowed_for_unchanged_files(self, make_finding): + """Finding with impact keywords should be allowed for unchanged files.""" + changed_files = ["src/auth.py"] + + # 'breaks' keyword + finding = make_finding( + file="src/utils.py", + line=10, + description="This change breaks the helper function in utils.py" + ) + is_valid, _ = _is_finding_in_scope(finding, changed_files) + assert is_valid + + # 'affects' keyword + finding = make_finding( + file="src/config.py", + line=5, + description="Changes in auth.py affects config loading" + ) + is_valid, _ = _is_finding_in_scope(finding, changed_files) + assert is_valid + + # 'depends' keyword + finding = make_finding( + file="src/database.py", + line=20, + description="database.py depends on modified auth module" + ) + is_valid, _ = _is_finding_in_scope(finding, changed_files) + assert is_valid + + def test_no_file_specified_fails(self, make_finding): + """Finding with no file specified should fail.""" + changed_files = ["src/test.py"] + finding = make_finding(file="") + is_valid, reason = _is_finding_in_scope(finding, changed_files) + assert not is_valid + assert "no file" in reason.lower() + + def test_none_line_number_passes(self, make_finding): + """Finding with None line number should pass (general finding).""" + changed_files = ["src/test.py"] + finding = make_finding(file="src/test.py", line=None) + # Line=None means general file-level finding + finding.line = None # Override since fixture sets it + is_valid, _ = _is_finding_in_scope(finding, changed_files) + assert is_valid + + +# ============================================================================= +# Phase 2 Tests: Import Detection, Reverse Dependencies +# ============================================================================= + +# For Phase 2 tests, we need the real PRContextGatherer methods +# We'll test the functions directly by extracting the relevant logic +github_dir = backend_path / "runners" / "github" + +# Load context_gatherer module directly using spec loader +# This avoids the complex package import chain +_cg_spec = importlib.util.spec_from_file_location( + "context_gatherer_isolated", + github_dir / "context_gatherer.py" +) +_cg_module = importlib.util.module_from_spec(_cg_spec) +# Set up minimal module environment +sys.modules['context_gatherer_isolated'] = _cg_module +# Mock only the gh_client dependency +_mock_gh = MagicMock() +sys.modules['gh_client'] = _mock_gh +_cg_spec.loader.exec_module(_cg_module) +PRContextGathererIsolated = _cg_module.PRContextGatherer + + +class TestImportDetection: + """Test import detection logic (Phase 2).""" + + @pytest.fixture + def temp_project(self, tmp_path): + """Create a temporary project structure for import testing.""" + # Create src directory + src_dir = tmp_path / "src" + src_dir.mkdir() + + # Create utils.ts file + (src_dir / "utils.ts").write_text("export const helper = () => {};") + + # Create config.ts file + (src_dir / "config.ts").write_text("export const config = { debug: true };") + + # Create index.ts that re-exports + (src_dir / "index.ts").write_text("export * from './utils';\nexport { config } from './config';") + + # Create shared directory + shared_dir = src_dir / "shared" + shared_dir.mkdir() + (shared_dir / "types.ts").write_text("export type User = { id: string };") + + # Create Python module + (src_dir / "python_module.py").write_text("from .helpers import util_func\nimport os") + (src_dir / "helpers.py").write_text("def util_func(): pass") + (src_dir / "__init__.py").write_text("") + + return tmp_path + + def test_path_alias_detection(self, temp_project): + """Path alias imports (@/utils) should be detected and resolved.""" + import json + # Create tsconfig.json with path aliases + tsconfig = { + "compilerOptions": { + "paths": { + "@/*": ["src/*"], + "@shared/*": ["src/shared/*"] + } + } + } + (temp_project / "tsconfig.json").write_text(json.dumps(tsconfig)) + + # Create the target file that the alias points to + (temp_project / "src" / "utils.ts").write_text("export const helper = () => {};") + + # Test file with alias import + test_content = "import { helper } from '@/utils';" + source_path = Path("src/test.ts") + + gatherer = PRContextGathererIsolated(temp_project, pr_number=1) + + # Call _find_imports + imports = gatherer._find_imports(test_content, source_path) + + # Should resolve @/utils to src/utils.ts + assert isinstance(imports, set) + # Normalize paths for cross-platform comparison (Windows uses backslashes) + normalized_imports = {p.replace("\\", "/") for p in imports} + assert "src/utils.ts" in normalized_imports, f"Expected 'src/utils.ts' in imports, got: {imports}" + + def test_commonjs_require_detection(self, temp_project): + """CommonJS require('./utils') should be detected.""" + test_content = "const utils = require('./utils');" + source_path = Path("src/test.ts") + + gatherer = PRContextGathererIsolated(temp_project, pr_number=1) + imports = gatherer._find_imports(test_content, source_path) + + # Should detect relative require + # Normalize paths for cross-platform comparison (Windows uses backslashes) + normalized_imports = {p.replace("\\", "/") for p in imports} + assert "src/utils.ts" in normalized_imports + + def test_reexport_detection(self, temp_project): + """Re-exports (export * from './module') should be detected.""" + test_content = "export * from './utils';\nexport { config } from './config';" + source_path = Path("src/index.ts") + + gatherer = PRContextGathererIsolated(temp_project, pr_number=1) + imports = gatherer._find_imports(test_content, source_path) + + # Should detect re-export targets + # Normalize paths for cross-platform comparison (Windows uses backslashes) + normalized_imports = {p.replace("\\", "/") for p in imports} + assert "src/utils.ts" in normalized_imports + assert "src/config.ts" in normalized_imports + + def test_python_relative_import(self, temp_project): + """Python relative imports (from .utils import) should be detected via AST.""" + test_content = "from .helpers import util_func" + source_path = Path("src/python_module.py") + + gatherer = PRContextGathererIsolated(temp_project, pr_number=1) + imports = gatherer._find_imports(test_content, source_path) + + # Should resolve relative Python import + # Normalize paths for cross-platform comparison (Windows uses backslashes) + normalized_imports = {p.replace("\\", "/") for p in imports} + assert "src/helpers.py" in normalized_imports + + def test_python_absolute_import(self, temp_project): + """Python absolute imports should be checked for project-internal modules.""" + # Create a project-internal module + (temp_project / "myapp").mkdir() + (temp_project / "myapp" / "__init__.py").write_text("") + (temp_project / "myapp" / "config.py").write_text("DEBUG = True") + + test_content = "from myapp import config" + source_path = Path("src/test.py") + + gatherer = PRContextGathererIsolated(temp_project, pr_number=1) + imports = gatherer._find_imports(test_content, source_path) + + # Should resolve absolute import to project module + # Normalize paths for cross-platform comparison (Windows uses backslashes) + normalized_imports = {p.replace("\\", "/") for p in imports} + assert any("myapp" in i for i in normalized_imports) + + +class TestReverseDepDetection: + """Test reverse dependency detection (Phase 2).""" + + @pytest.fixture + def temp_project_with_deps(self, tmp_path): + """Create a project with files that import each other.""" + src_dir = tmp_path / "src" + src_dir.mkdir() + + # Create a utility file with non-generic name (helpers is in skip list) + (src_dir / "formatter.ts").write_text( + "export function format(s: string) { return s; }" + ) + + # Create files that import formatter + (src_dir / "auth.ts").write_text( + "import { format } from './formatter';\nexport const login = () => {};" + ) + (src_dir / "api.ts").write_text( + "import { format } from './formatter';\nexport const fetch = () => {};" + ) + + # Create a file that does NOT import formatter + (src_dir / "standalone.ts").write_text( + "export const standalone = () => {};" + ) + + return tmp_path + + def test_finds_files_importing_changed_file(self, temp_project_with_deps): + """Verify grep-based detection finds files that import a given file.""" + gatherer = PRContextGathererIsolated(temp_project_with_deps, pr_number=1) + # Use non-generic name (helpers is in the skip list) + dependents = gatherer._find_dependents("src/formatter.ts", max_results=10) + + # Should find auth.ts and api.ts as dependents + assert any("auth.ts" in d for d in dependents) + assert any("api.ts" in d for d in dependents) + # Should NOT include standalone.ts + assert not any("standalone.ts" in d for d in dependents) + + def test_skips_generic_names(self, tmp_path): + """Generic names (index, main, utils) should be skipped to reduce noise.""" + src_dir = tmp_path / "src" + src_dir.mkdir() + + # Create files with generic names + (src_dir / "index.ts").write_text("export * from './utils';") + (src_dir / "main.ts").write_text("import { x } from './index';") + + gatherer = PRContextGathererIsolated(tmp_path, pr_number=1) + + # Generic names should return empty set (skipped) + dependents_index = gatherer._find_dependents("src/index.ts") + dependents_main = gatherer._find_dependents("src/main.ts") + + # These should be skipped due to generic names + assert len(dependents_index) == 0 + assert len(dependents_main) == 0 + + def test_respects_file_limit(self, tmp_path): + """Large repo search should stop after reaching file limit.""" + src_dir = tmp_path / "src" + src_dir.mkdir() + (src_dir / "unique_name.ts").write_text("export const x = 1;") + + gatherer = PRContextGathererIsolated(tmp_path, pr_number=1) + + # Mock os.walk to generate more files than the limit (2000) + # This simulates a large codebase without creating actual files + def mock_walk(path): + # Yield a directory with 3000 TypeScript files + yield (str(path), [], [f"file{i}.ts" for i in range(3000)]) + + with patch.object(_cg_module.os, "walk", mock_walk): + # The function should stop after max_files_to_check (2000) files + # and return gracefully without hanging + dependents = gatherer._find_dependents("src/unique_name.ts") + + # Should return a set (may be empty since mock files don't contain imports) + assert isinstance(dependents, set) + + +# ============================================================================= +# Phase 3 Tests: Multi-Agent Cross-Validation +# ============================================================================= + +# Import the cross-validation function from orchestrator +ParallelOrchestratorReviewer = orchestrator_module.ParallelOrchestratorReviewer + + +class TestCrossValidation: + """Test multi-agent cross-validation logic (Phase 3).""" + + @pytest.fixture + def make_finding(self): + """Factory fixture to create PRReviewFinding instances.""" + def _make_finding( + id: str = "TEST001", + file: str = "src/test.py", + line: int = 10, + category: ReviewCategory = ReviewCategory.SECURITY, + severity: ReviewSeverity = ReviewSeverity.HIGH, + confidence: float = 0.7, + source_agents: list = None, + **kwargs + ): + return PRReviewFinding( + id=id, + severity=severity, + category=category, + title=kwargs.get("title", "Test Finding"), + description=kwargs.get("description", "Test description"), + file=file, + line=line, + confidence=confidence, + source_agents=source_agents or [], + **{k: v for k, v in kwargs.items() if k not in ["title", "description"]} + ) + return _make_finding + + @pytest.fixture + def mock_reviewer(self, tmp_path): + """Create a mock ParallelOrchestratorReviewer instance.""" + from models import GitHubRunnerConfig + + config = GitHubRunnerConfig( + token="test-token", + repo="test/repo" + ) + # Create minimal directory structure + github_dir = tmp_path / ".auto-claude" / "github" + github_dir.mkdir(parents=True) + + reviewer = ParallelOrchestratorReviewer( + project_dir=tmp_path, + github_dir=github_dir, + config=config + ) + return reviewer + + def test_multi_agent_agreement_boosts_confidence(self, make_finding, mock_reviewer): + """When 2+ agents agree on same finding, confidence should increase by 0.15.""" + # Two findings from different agents on same (file, line, category) + finding1 = make_finding( + id="F1", + file="src/auth.py", + line=10, + category=ReviewCategory.SECURITY, + confidence=0.7, + source_agents=["security-reviewer"], + description="SQL injection risk" + ) + finding2 = make_finding( + id="F2", + file="src/auth.py", + line=10, + category=ReviewCategory.SECURITY, + confidence=0.6, + source_agents=["quality-reviewer"], + description="Input not sanitized" + ) + + validated, agreement = mock_reviewer._cross_validate_findings([finding1, finding2]) + + # Should merge into one finding + assert len(validated) == 1 + # Confidence should be boosted: max(0.7, 0.6) + 0.15 = 0.85 + assert validated[0].confidence == pytest.approx(0.85, rel=0.01) + # Should have cross_validated flag set + assert validated[0].cross_validated is True + # Should track in agreement + assert len(agreement.agreed_findings) == 1 + + def test_confidence_boost_capped_at_095(self, make_finding, mock_reviewer): + """Confidence boost should cap at 0.95, not exceed 1.0.""" + finding1 = make_finding( + id="F1", + file="src/auth.py", + line=10, + category=ReviewCategory.SECURITY, + confidence=0.85, + source_agents=["security-reviewer"], + ) + finding2 = make_finding( + id="F2", + file="src/auth.py", + line=10, + category=ReviewCategory.SECURITY, + confidence=0.90, + source_agents=["logic-reviewer"], + ) + + validated, _ = mock_reviewer._cross_validate_findings([finding1, finding2]) + + # 0.90 + 0.15 = 1.05, but should cap at 0.95 + assert validated[0].confidence == 0.95 + + def test_merged_finding_has_cross_validated_true(self, make_finding, mock_reviewer): + """Merged multi-agent findings should have cross_validated=True.""" + finding1 = make_finding(id="F1", file="src/test.py", line=5, source_agents=["agent1"]) + finding2 = make_finding(id="F2", file="src/test.py", line=5, source_agents=["agent2"]) + + validated, _ = mock_reviewer._cross_validate_findings([finding1, finding2]) + + assert validated[0].cross_validated is True + + def test_grouping_by_file_line_category(self, make_finding, mock_reviewer): + """Findings should be grouped by (file, line, category) tuple.""" + # Same file+line but different category - should NOT merge + finding1 = make_finding( + id="F1", + file="src/test.py", + line=10, + category=ReviewCategory.SECURITY, + ) + finding2 = make_finding( + id="F2", + file="src/test.py", + line=10, + category=ReviewCategory.QUALITY, # Different category + ) + + validated, _ = mock_reviewer._cross_validate_findings([finding1, finding2]) + + # Should remain as 2 separate findings + assert len(validated) == 2 + + # Same category but different line - should NOT merge + finding3 = make_finding( + id="F3", + file="src/test.py", + line=10, + category=ReviewCategory.SECURITY, + ) + finding4 = make_finding( + id="F4", + file="src/test.py", + line=20, # Different line + category=ReviewCategory.SECURITY, + ) + + validated2, _ = mock_reviewer._cross_validate_findings([finding3, finding4]) + assert len(validated2) == 2 + + def test_merged_description_combines_sources(self, make_finding, mock_reviewer): + """Merged findings should combine descriptions with ' | ' separator.""" + finding1 = make_finding( + id="F1", + file="src/auth.py", + line=10, + category=ReviewCategory.SECURITY, + description="SQL injection vulnerability", + ) + finding2 = make_finding( + id="F2", + file="src/auth.py", + line=10, + category=ReviewCategory.SECURITY, + description="Unsanitized user input", + ) + + validated, _ = mock_reviewer._cross_validate_findings([finding1, finding2]) + + # Should combine descriptions with ' | ' + assert " | " in validated[0].description + assert "SQL injection vulnerability" in validated[0].description + assert "Unsanitized user input" in validated[0].description + + def test_single_agent_finding_not_boosted(self, make_finding, mock_reviewer): + """Single-agent findings should not have confidence boosted.""" + finding = make_finding( + id="F1", + file="src/test.py", + line=10, + confidence=0.7, + source_agents=["security-reviewer"], + ) + + validated, agreement = mock_reviewer._cross_validate_findings([finding]) + + # Confidence should remain unchanged + assert validated[0].confidence == 0.7 + # Should not be marked as cross-validated + assert validated[0].cross_validated is False + # Should not be in agreed_findings + assert len(agreement.agreed_findings) == 0 + + def test_merged_finding_keeps_highest_severity(self, make_finding, mock_reviewer): + """Merged findings should keep the highest severity.""" + finding1 = make_finding( + id="F1", + file="src/test.py", + line=10, + severity=ReviewSeverity.MEDIUM, + ) + finding2 = make_finding( + id="F2", + file="src/test.py", + line=10, + severity=ReviewSeverity.CRITICAL, + ) + + validated, _ = mock_reviewer._cross_validate_findings([finding1, finding2]) + + # Should keep CRITICAL (highest severity) + assert validated[0].severity == ReviewSeverity.CRITICAL + + +# ============================================================================= +# Integration Verification Tests: Full Pipeline Tests +# ============================================================================= + +class TestIntegrationPipeline: + """Test complete pipeline integration of Phase 1-3 features.""" + + @pytest.fixture + def make_finding(self): + """Factory fixture to create PRReviewFinding instances.""" + def _make_finding( + id: str = "TEST001", + file: str = "src/test.py", + line: int = 10, + category: ReviewCategory = ReviewCategory.SECURITY, + severity: ReviewSeverity = ReviewSeverity.HIGH, + confidence: float = 0.7, + evidence: str = "const x = getValue()", + source_agents: list = None, + **kwargs + ): + return PRReviewFinding( + id=id, + severity=severity, + category=category, + title=kwargs.get("title", "Test Finding"), + description=kwargs.get("description", "Test description"), + file=file, + line=line, + confidence=confidence, + evidence=evidence, + source_agents=source_agents or [], + **{k: v for k, v in kwargs.items() if k not in ["title", "description"]} + ) + return _make_finding + + @pytest.fixture + def mock_reviewer(self, tmp_path): + """Create a mock ParallelOrchestratorReviewer instance.""" + from models import GitHubRunnerConfig + + config = GitHubRunnerConfig( + token="test-token", + repo="test/repo" + ) + github_dir = tmp_path / ".auto-claude" / "github" + github_dir.mkdir(parents=True) + + reviewer = ParallelOrchestratorReviewer( + project_dir=tmp_path, + github_dir=github_dir, + config=config + ) + return reviewer + + def test_pipeline_flow_high_confidence_valid_evidence_in_scope( + self, make_finding, mock_reviewer + ): + """Test complete flow: high confidence + valid evidence + in scope passes all checks.""" + changed_files = ["src/auth.py"] + finding = make_finding( + file="src/auth.py", + line=15, + confidence=0.85, # HIGH tier (>= 0.8) + evidence="const password = req.body.password; query(`SELECT * WHERE pwd='${password}'`)", + ) + + # Phase 1a: Confidence tier + tier = ConfidenceTier.get_tier(finding.confidence) + assert tier == ConfidenceTier.HIGH + + # Phase 1b: Evidence validation + is_valid_evidence, _ = _validate_finding_evidence(finding) + assert is_valid_evidence + + # Phase 1c: Scope filtering + is_in_scope, _ = _is_finding_in_scope(finding, changed_files) + assert is_in_scope + + # All checks pass - finding should be included in review + + def test_pipeline_flow_low_confidence_filtered(self, make_finding): + """Test that low confidence findings are filtered even with valid evidence.""" + changed_files = ["src/auth.py"] + finding = make_finding( + file="src/auth.py", + line=15, + confidence=0.3, # LOW tier (< 0.5) + evidence="const x = getValue()", + ) + + tier = ConfidenceTier.get_tier(finding.confidence) + assert tier == ConfidenceTier.LOW + + # Low confidence findings may be filtered based on tier routing + # This test documents the expected behavior + + def test_pipeline_flow_cross_validation_elevates_medium_to_high( + self, make_finding, mock_reviewer + ): + """Test that cross-validation can elevate MEDIUM tier to HIGH tier.""" + # Two agents find same issue with MEDIUM confidence + # Give finding2 CRITICAL severity so it becomes the primary (sorted first) + finding1 = make_finding( + id="F1", + file="src/auth.py", + line=10, + confidence=0.6, # MEDIUM tier + severity=ReviewSeverity.HIGH, + source_agents=["security-reviewer"], + ) + finding2 = make_finding( + id="F2", + file="src/auth.py", + line=10, + confidence=0.7, # MEDIUM tier - this will be primary (higher severity) + severity=ReviewSeverity.CRITICAL, + source_agents=["quality-reviewer"], + ) + + # Before cross-validation: both MEDIUM + assert ConfidenceTier.get_tier(finding1.confidence) == ConfidenceTier.MEDIUM + assert ConfidenceTier.get_tier(finding2.confidence) == ConfidenceTier.MEDIUM + + # Cross-validate + validated, _ = mock_reviewer._cross_validate_findings([finding1, finding2]) + + # After cross-validation: boosted to primary.confidence + 0.15 = 0.7 + 0.15 = 0.85 (HIGH tier) + # Note: The code uses primary finding's confidence, sorted by severity (CRITICAL first) + assert validated[0].confidence == pytest.approx(0.85, rel=0.01) + assert ConfidenceTier.get_tier(validated[0].confidence) == ConfidenceTier.HIGH + assert validated[0].severity == ReviewSeverity.CRITICAL # Highest severity preserved + + def test_pipeline_invalid_evidence_rejected(self, make_finding): + """Test that findings with invalid evidence are rejected regardless of confidence.""" + finding = make_finding( + file="src/auth.py", + line=15, + confidence=0.95, # HIGH confidence + evidence="This code looks problematic", # Prose, not code + ) + + is_valid_evidence, reason = _validate_finding_evidence(finding) + assert not is_valid_evidence + assert "lacks code syntax" in reason.lower() + + def test_pipeline_out_of_scope_rejected(self, make_finding): + """Test that findings outside changed files are rejected.""" + changed_files = ["src/auth.py", "src/utils.py"] + finding = make_finding( + file="src/database.py", # Not in changed files + line=15, + confidence=0.95, + evidence="const query = buildQuery(input)", + ) + + is_in_scope, reason = _is_finding_in_scope(finding, changed_files) + assert not is_in_scope + + def test_pipeline_impact_finding_allowed(self, make_finding): + """Test that impact findings for unchanged files are allowed.""" + changed_files = ["src/auth.py"] + finding = make_finding( + file="src/database.py", # Not in changed files + line=15, + confidence=0.85, + evidence="const query = buildQuery(input)", + description="Changes to auth.py breaks the database connection in database.py", + ) + + is_in_scope, _ = _is_finding_in_scope(finding, changed_files) + assert is_in_scope # Allowed because description mentions impact + + def test_pipeline_end_to_end_review_scenario(self, make_finding, mock_reviewer): + """Test realistic end-to-end review scenario with multiple findings.""" + changed_files = ["src/auth.py", "src/api.py"] + + # Findings from multiple agents + findings = [ + # High-confidence security finding from agent 1 + make_finding( + id="SEC001", + file="src/auth.py", + line=42, + category=ReviewCategory.SECURITY, + severity=ReviewSeverity.CRITICAL, + confidence=0.85, + evidence="password = req.body.password; db.query(`SELECT * WHERE pwd='${password}'`)", + source_agents=["security-reviewer"], + description="SQL injection in login", + ), + # Same finding from agent 2 (will be cross-validated) + make_finding( + id="SEC002", + file="src/auth.py", + line=42, + category=ReviewCategory.SECURITY, + severity=ReviewSeverity.HIGH, + confidence=0.75, + evidence="db.query(`SELECT * WHERE pwd='${password}'`)", + source_agents=["quality-reviewer"], + description="Unsanitized query parameter", + ), + # Valid quality finding + make_finding( + id="QUAL001", + file="src/api.py", + line=100, + category=ReviewCategory.QUALITY, + severity=ReviewSeverity.MEDIUM, + confidence=0.7, + evidence="function handleRequest(req) { /* missing error handling */ }", + source_agents=["quality-reviewer"], + description="Missing error handling", + ), + # Invalid: out of scope + make_finding( + id="OUT001", + file="src/database.py", # Not in changed files + line=10, + category=ReviewCategory.SECURITY, + confidence=0.9, + evidence="connection.close()", + source_agents=["security-reviewer"], + ), + # Invalid: prose evidence + make_finding( + id="PROSE001", + file="src/auth.py", + line=50, + category=ReviewCategory.QUALITY, + confidence=0.8, + evidence="This code should be refactored for better maintainability", + source_agents=["quality-reviewer"], + ), + ] + + # Step 1: Cross-validate + validated, agreement = mock_reviewer._cross_validate_findings(findings) + + # SEC001 and SEC002 should be merged (same file, line, category) + security_findings = [f for f in validated if f.category == ReviewCategory.SECURITY + and f.file == "src/auth.py" and f.line == 42] + assert len(security_findings) == 1 + # Cross-validated finding should have boosted confidence + assert security_findings[0].cross_validated is True + assert security_findings[0].confidence >= 0.85 # Boosted from max(0.85, 0.75) + 0.15 + + # Step 2: Validate evidence for each finding + valid_evidence_findings = [] + for f in validated: + is_valid, _ = _validate_finding_evidence(f) + if is_valid: + valid_evidence_findings.append(f) + + # PROSE001 should be filtered out (if present in validated) + prose_findings = [f for f in valid_evidence_findings + if f.evidence and "refactored" in f.evidence] + assert len(prose_findings) == 0 + + # Step 3: Filter by scope + in_scope_findings = [] + for f in valid_evidence_findings: + is_in_scope, _ = _is_finding_in_scope(f, changed_files) + if is_in_scope: + in_scope_findings.append(f) + + # OUT001 should be filtered out (src/database.py not in changed_files) + database_findings = [f for f in in_scope_findings if f.file == "src/database.py"] + assert len(database_findings) == 0 + + def test_pipeline_empty_findings_handled(self, mock_reviewer): + """Test that empty findings list is handled gracefully.""" + validated, agreement = mock_reviewer._cross_validate_findings([]) + + assert len(validated) == 0 + assert len(agreement.agreed_findings) == 0 + assert len(agreement.conflicting_findings) == 0 # Note: uses conflicting_findings, not disputed + + def test_pipeline_confidence_tier_determines_routing(self, make_finding): + """Test that confidence tier determines review routing behavior.""" + # Document the tier routing expectations + test_cases = [ + (0.95, ConfidenceTier.HIGH, "auto-include"), + (0.85, ConfidenceTier.HIGH, "auto-include"), + (0.80, ConfidenceTier.HIGH, "auto-include"), + (0.75, ConfidenceTier.MEDIUM, "needs verification"), + (0.60, ConfidenceTier.MEDIUM, "needs verification"), + (0.50, ConfidenceTier.MEDIUM, "needs verification"), + (0.45, ConfidenceTier.LOW, "consider filtering"), + (0.30, ConfidenceTier.LOW, "consider filtering"), + (0.10, ConfidenceTier.LOW, "consider filtering"), + ] + + for confidence, expected_tier, expected_routing in test_cases: + finding = make_finding(confidence=confidence) + tier = ConfidenceTier.get_tier(finding.confidence) + assert tier == expected_tier, f"Confidence {confidence} should be {expected_tier}"