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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Andy
@@ -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.**
|
||||
@@ -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
|
||||
|
||||
<!-- SYNC: This section is shared. See partials/full_context_analysis.md for canonical 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.**
|
||||
|
||||
## Code Patterns to Flag
|
||||
|
||||
### Reinventing Existing Utilities
|
||||
|
||||
@@ -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."
|
||||
|
||||
<!-- SYNC: This section is shared. See partials/full_context_analysis.md for canonical 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.**
|
||||
|
||||
## Evidence Requirements
|
||||
|
||||
Every finding MUST include an `evidence` field with:
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -113,6 +113,33 @@ For each finding, provide:
|
||||
2. What the current code produces
|
||||
3. What it should produce
|
||||
|
||||
<!-- SYNC: This section is shared. See partials/full_context_analysis.md for canonical 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.**
|
||||
|
||||
## Code Patterns to Flag
|
||||
|
||||
### Off-By-One Errors
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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"
|
||||
|
||||
<!-- SYNC: This section is shared. See partials/full_context_analysis.md for canonical 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.**
|
||||
|
||||
## Code Patterns to Flag
|
||||
|
||||
### JavaScript/TypeScript
|
||||
|
||||
@@ -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?)
|
||||
|
||||
<!-- SYNC: This section is shared. See partials/full_context_analysis.md for canonical 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.**
|
||||
|
||||
## Code Patterns to Flag
|
||||
|
||||
### JavaScript/TypeScript
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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),
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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],
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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
|
||||
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user