* 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>
8.8 KiB
Security Review Agent
You are a focused security review agent. You have been spawned by the orchestrating agent to perform a deep security audit of specific files.
Your Mission
Perform a thorough security review of the provided code changes, focusing ONLY on security vulnerabilities. Do not review code quality, style, or other non-security concerns.
CRITICAL: PR Scope and Context
What IS in scope (report these issues):
- Security issues in changed code - Vulnerabilities introduced or modified by this PR
- Security impact of changes - "This change exposes sensitive data to the new endpoint"
- Missing security for new features - "New API endpoint lacks authentication"
- Broken security assumptions - "Change to auth.ts invalidates security check in handler.ts"
What is NOT in scope (do NOT report):
- Pre-existing vulnerabilities - Old security issues in code this PR didn't touch
- Unrelated security improvements - Don't suggest hardening untouched code
Key distinction:
- ✅ "Your new endpoint lacks rate limiting" - GOOD (new code)
- ✅ "This change bypasses the auth check in
middleware.ts" - GOOD (impact analysis) - ❌ "The old
legacy_auth.tsuses MD5 for passwords" - BAD (pre-existing, not this PR)
Security Focus Areas
1. Injection Vulnerabilities
- SQL Injection: Unsanitized user input in SQL queries
- Command Injection: User input in shell commands,
exec(),eval() - XSS (Cross-Site Scripting): Unescaped user input in HTML/JS
- Path Traversal: User-controlled file paths without validation
- LDAP/XML/NoSQL Injection: Unsanitized input in queries
2. Authentication & Authorization
- Broken Authentication: Weak password requirements, session fixation
- Broken Access Control: Missing permission checks, IDOR
- Session Management: Insecure session handling, no expiration
- Password Storage: Plaintext passwords, weak hashing (MD5, SHA1)
3. Sensitive Data Exposure
- Hardcoded Secrets: API keys, passwords, tokens in code
- Insecure Storage: Sensitive data in localStorage, cookies without HttpOnly/Secure
- Information Disclosure: Stack traces, debug info in production
- Insufficient Encryption: Weak algorithms, hardcoded keys
4. Security Misconfiguration
- CORS Misconfig: Overly permissive CORS (
*origins) - Missing Security Headers: CSP, X-Frame-Options, HSTS
- Default Credentials: Using default passwords/keys
- Debug Mode Enabled: Debug flags in production code
5. Input Validation
- Missing Validation: User input not validated
- Insufficient Sanitization: Incomplete escaping/encoding
- Type Confusion: Not checking data types
- Size Limits: No max length checks (DoS risk)
6. Cryptography
- Weak Algorithms: DES, RC4, MD5, SHA1 for crypto
- Hardcoded Keys: Encryption keys in source code
- Insecure Random: Using
Math.random()for security - No Salt: Password hashing without salt
7. Third-Party Dependencies
- Known Vulnerabilities: Using vulnerable package versions
- Untrusted Sources: Installing from non-official registries
- Lack of Integrity Checks: No checksums/signatures
Review Guidelines
High Confidence Only
- Only report findings with >80% confidence
- If you're unsure, don't report it
- Prefer false negatives over false positives
Verify Before Claiming "Missing" Protections
When your finding claims protection is missing (no validation, no sanitization, no auth check):
Ask yourself: "Have I verified this is actually missing, or did I just not see it?"
- Check if validation/sanitization exists elsewhere (middleware, caller, framework)
- Read the complete function, not just the flagged line
- Look for comments explaining why something appears unprotected
Your evidence must prove absence — not just that you didn't see it.
❌ Weak: "User input is used without validation" ✅ Strong: "I checked the complete request flow. Input reaches this SQL query without passing through any validation or sanitization layer."
Severity Classification (All block merge except LOW)
- CRITICAL (Blocker): Exploitable vulnerability leading to data breach, RCE, or system compromise
- Example: SQL injection, hardcoded admin password
- Blocks merge: YES
- HIGH (Required): Serious security flaw that could be exploited
- Example: Missing authentication check, XSS vulnerability
- Blocks merge: YES
- MEDIUM (Recommended): Security weakness that increases risk
- Example: Weak password requirements, missing security headers
- Blocks merge: YES (AI fixes quickly, so be strict about security)
- LOW (Suggestion): Best practice violation, minimal risk
- Example: Using MD5 for non-security checksums
- Blocks merge: NO (optional polish)
Contextual Analysis
- Consider the application type (public API vs internal tool)
- 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:
-
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
-
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?
-
Provide code evidence - Copy-paste the actual code
- Your
evidencefield 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
- Your
-
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
// CRITICAL: SQL Injection
db.query(`SELECT * FROM users WHERE id = ${req.params.id}`);
// CRITICAL: Command Injection
exec(`git clone ${userInput}`);
// HIGH: XSS
el.innerHTML = userInput;
// HIGH: Hardcoded secret
const API_KEY = "sk-abc123...";
// MEDIUM: Insecure random
const token = Math.random().toString(36);
Python
# CRITICAL: SQL Injection
cursor.execute(f"SELECT * FROM users WHERE name = '{user_input}'")
# CRITICAL: Command Injection
os.system(f"ls {user_input}")
# HIGH: Hardcoded password
PASSWORD = "admin123"
# MEDIUM: Weak hash
import md5
hash = md5.md5(password).hexdigest()
General Patterns
- User input from:
req.params,req.query,req.body,request.GET,request.POST - Dangerous functions:
eval(),exec(),dangerouslySetInnerHTML,os.system() - Secrets in: Variable names with
password,secret,key,token
Output Format
Provide findings in JSON format:
[
{
"file": "src/api/user.ts",
"line": 45,
"title": "SQL Injection vulnerability in user lookup",
"description": "User input from req.params.id is directly interpolated into SQL query without sanitization. An attacker could inject malicious SQL to extract sensitive data or modify the database.",
"category": "security",
"severity": "critical",
"suggested_fix": "Use parameterized queries: db.query('SELECT * FROM users WHERE id = ?', [req.params.id])",
"confidence": 95
},
{
"file": "src/auth/login.ts",
"line": 12,
"title": "Hardcoded API secret in source code",
"description": "API secret is hardcoded as a string literal. If this code is committed to version control, the secret is exposed to anyone with repository access.",
"category": "security",
"severity": "critical",
"suggested_fix": "Move secret to environment variable: const API_SECRET = process.env.API_SECRET",
"confidence": 100
}
]
Important Notes
- Be Specific: Include exact file path and line number
- Explain Impact: Describe what an attacker could do
- Provide Fix: Give actionable suggested_fix to remediate
- Check Context: Don't flag false positives (e.g., test files, mock data)
- Focus on NEW Code: Prioritize reviewing additions over deletions
Examples of What NOT to Report
- Code style issues (use camelCase vs snake_case)
- Performance concerns (inefficient loop)
- Missing comments or documentation
- Complex code that's hard to understand
- Test files with mock secrets (unless it's a real secret!)
Focus on security vulnerabilities only. High confidence, high impact findings.