diff --git a/claude/auto-review/action.yml b/claude/auto-review/action.yml index 8499aa4..ac6e86a 100644 --- a/claude/auto-review/action.yml +++ b/claude/auto-review/action.yml @@ -23,10 +23,57 @@ inputs: description: "Post inline PR comments generated from findings.json" required: false default: "true" + force_all_agents: + description: "Force all review agents regardless of heuristic (overrides selective spawning)" + required: false + default: "false" runs: using: "composite" steps: + - name: Determine agents to spawn + shell: bash + env: + GH_TOKEN: ${{ github.token }} + FORCE_ALL_AGENTS: ${{ inputs.force_all_agents }} + run: | + SCRIPT_PATH="${{ github.action_path }}/scripts/determine-agents.js" + + if [[ ! -f "$SCRIPT_PATH" ]]; then + echo "::warning::Determine agents script not found, spawning all agents" + echo "AGENTS_TO_RUN=bug,security,patterns" >> $GITHUB_ENV + echo "AGENT_REASON=Script not found (fallback)" >> $GITHUB_ENV + exit 0 + fi + + RESULT=$(node "$SCRIPT_PATH") + + AGENTS=$(echo "$RESULT" | jq -r '.agents | join(",")') + REASON=$(echo "$RESULT" | jq -r '.reason') + SKIPPED=$(echo "$RESULT" | jq -r '.skipped | join(",")') + + echo "AGENTS_TO_RUN=$AGENTS" >> $GITHUB_ENV + echo "AGENT_REASON=$REASON" >> $GITHUB_ENV + echo "AGENTS_SKIPPED=$SKIPPED" >> $GITHUB_ENV + + # Set skip flag if no agents to spawn + if [[ -z "$AGENTS" ]]; then + echo "SKIP_REVIEW=true" >> $GITHUB_ENV + else + echo "SKIP_REVIEW=false" >> $GITHUB_ENV + fi + + echo "::group::Agent Selection" + echo "Agents to spawn: ${AGENTS:-none}" + echo "Reason: $REASON" + if [[ -n "$SKIPPED" ]]; then + echo "Skipped agents: $SKIPPED" + fi + if [[ -z "$AGENTS" ]]; then + echo "Review will be skipped" + fi + echo "::endgroup::" + - name: Set up review prompt shell: bash run: | @@ -83,33 +130,74 @@ runs: --- - ## AUTOMATED CHECKS + ## MULTI-AGENT REVIEW" + + # Check if we have any agents to spawn + if [[ -z "$AGENTS_TO_RUN" ]]; then + # No agents to spawn - skip review + PROMPT="$PROMPT + + **Review skipped:** $AGENT_REASON + + Respond with: \\\"✅ No code review needed - $AGENT_REASON\\\"" + else + # Build agent list dynamically + AGENT_COUNT=0 + AGENT_LIST="" + + if [[ "$AGENTS_TO_RUN" == *"bug"* ]]; then + AGENT_COUNT=$((AGENT_COUNT + 1)) + AGENT_LIST="$AGENT_LIST + $AGENT_COUNT. **Bug Agent** - Spec: ${{ github.action_path }}/agents/review-bugs.md" + fi + + if [[ "$AGENTS_TO_RUN" == *"security"* ]]; then + AGENT_COUNT=$((AGENT_COUNT + 1)) + AGENT_LIST="$AGENT_LIST + $AGENT_COUNT. **Security Agent** - Spec: ${{ github.action_path }}/agents/review-security.md" + fi + + if [[ "$AGENTS_TO_RUN" == *"patterns"* ]]; then + AGENT_COUNT=$((AGENT_COUNT + 1)) + AGENT_LIST="$AGENT_LIST + $AGENT_COUNT. **Patterns Agent** - Spec: ${{ github.action_path }}/agents/review-patterns.md" + fi + + if [[ $AGENT_COUNT -eq 1 ]]; then + AGENT_WORD="subagent" + else + AGENT_WORD="subagents" + fi + + PROMPT="$PROMPT + + Based on PR analysis: $AGENT_REASON + + Conduct review using $AGENT_COUNT specialized $AGENT_WORD IN PARALLEL. + + **Agent specs:** ${{ github.action_path }}/agents/ + + ### Step 1: Spawn Agents Simultaneously - **Only report if violations found. Skip check if none detected.** + Use Task tool to launch agents in a SINGLE message. Use subagent_type=\\\"general-purpose\\\". - ### External Domain URLs - Flag URLs to domains other than reown.com, walletconnect.com, walletconnect.org: - 🔒 **External Domain URL** (Non-blocking) **URL:** [url] **File:** [path:line] - Verify intentional, review security implications. + **For EACH agent prompt include:** + 1. \\\"Read your spec file at [path] and follow its instructions.\\\" + 2. PR number, repository, list of changed files - ### Static Resource Cache-Control - Flag static files (.woff, .woff2, .ttf, .jpg, .png, .css, .js, .mp4, etc.) with max-age < 31536000 or missing explicit Cache-Control: - ⚠️ **Cache-Control Issue** **Resource:** [url] **File:** [path:line] **Current:** [value] **Recommendation:** \"Cache-Control: public, max-age=31536000, immutable\" + **Agents:**$AGENT_LIST - ### GitHub Actions Workflow Security - Scan .github/workflows/*.y*ml for: - - **CRITICAL:** pull_request_target + PR head checkout (github.event.pull_request.head.*) = arbitrary code execution - - **HIGH:** pull_request_target + script execution - - **MEDIUM:** Any pull_request_target usage (runs with secrets) - Format: 🚨 **GitHub Actions Security Risk** **Severity:** [level] **File:** [path:line] **Pattern:** [issue] **Recommendation:** [fix] + ### Step 2: Consolidate Findings - ### WalletConnect Pay Architecture - Flag anti-patterns in payment/wallet/transaction code: - 1. **CRITICAL:** Cross-service DB access (imports, queries, connections) → 🚨 Services must use APIs - 2. **HIGH:** Missing idempotency keys in POST/PUT/PATCH/DELETE → ⚠️ Extract key, check store, return cached response - 3. **HIGH:** External calls without timeout/retry → ⚠️ Add timeout, retry+backoff, circuit breaker - 4. **HIGH:** Event consumers (SQS/SNS/Kafka) without message deduplication → ⚠️ Check message ID before mutations - 5. **MEDIUM:** Multi-step workflows without saga compensation → ⚠️ Add rollback/compensating events - 6. **MEDIUM:** State transitions without trace context → ⚠️ Add structured logging with traceId/correlationId" + After agents complete: + 1. Collect all issues + 2. **Deduplicate** (same file + same/adjacent line + similar description): keep higher severity, merge context, prefer sec- > bug- > pat- + 3. Sort by severity: CRITICAL > HIGH > MEDIUM > LOW + + ### Step 3: Output + + Produce SINGLE consolidated summary in collapsed \\\`
\\\` format. No separate agent sections." + fi # Add project context @@ -195,17 +283,33 @@ runs: echo "::warning::The 'timeout_minutes' input is deprecated and has no effect in claude-code-action@v1. Please use job-level 'timeout-minutes' instead. See: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes" fi + - name: Skip review notification + if: env.SKIP_REVIEW == 'true' + shell: bash + env: + GH_TOKEN: ${{ github.token }} + run: | + echo "::notice::Skipping review: $AGENT_REASON" + + # Post a comment to the PR + gh pr comment "${{ github.event.pull_request.number }}" \ + --repo "${{ github.repository }}" \ + --body "✅ **Auto Review Skipped**: $AGENT_REASON + + _No code review needed for this PR. Add the \`full-review\` label to force a full review._" + - name: Automatic PR Review + if: env.SKIP_REVIEW != 'true' uses: anthropics/claude-code-action@v1 with: anthropic_api_key: ${{ inputs.anthropic_api_key }} prompt: ${{ env.REVIEW_PROMPT }} track_progress: true - claude_args: --model ${{ inputs.model }} + claude_args: --model ${{ inputs.model }} --allowedTools "Read,Glob,Grep,Task,WebFetch" allowed_bots: devin-ai-integration[bot] - name: Extract findings from Claude's comment - if: inputs.comment_pr_findings == 'true' + if: inputs.comment_pr_findings == 'true' && env.SKIP_REVIEW != 'true' shell: bash env: GH_TOKEN: ${{ github.token }} @@ -225,7 +329,7 @@ runs: node "$SCRIPT_PATH" - name: Ensure GitHub CLI and jq for inline comments - if: inputs.comment_pr_findings == 'true' + if: inputs.comment_pr_findings == 'true' && env.SKIP_REVIEW != 'true' shell: bash run: | # Install GitHub CLI if needed @@ -259,7 +363,7 @@ runs: fi - name: Post inline findings comments - if: inputs.comment_pr_findings == 'true' + if: inputs.comment_pr_findings == 'true' && env.SKIP_REVIEW != 'true' shell: bash env: GH_TOKEN: ${{ github.token }} diff --git a/claude/auto-review/agents/review-bugs.md b/claude/auto-review/agents/review-bugs.md new file mode 100644 index 0000000..334eb24 --- /dev/null +++ b/claude/auto-review/agents/review-bugs.md @@ -0,0 +1,106 @@ +# Bug Review Agent + +You are a code reviewer. Provide actionable feedback on code changes. + +**Diffs alone are not enough.** Read the full file(s) being modified to understand context. Code that looks wrong in isolation may be correct given surrounding logic. + +Your specialization: finding bugs and functional issues. + +**Scope:** Your review is not limited strictly to the focus areas below. Within the bug/correctness domain, also proactively recommend missing best practices - even when no explicit bug exists. + +## Your Focus Areas + +Analyze the PR changes for: + +1. **Logic Errors** + - Incorrect conditionals or boolean logic + - Off-by-one errors in loops or array access + - Wrong comparison operators (< vs <=, == vs ===) + - Inverted conditions or negation mistakes + +2. **Null/Undefined Handling** + - Missing null checks before dereferencing + - Optional chaining gaps + - Unhandled undefined returns from functions + - Incorrect default values + +3. **Race Conditions & Concurrency** + - Shared state modifications without synchronization + - Missing await on async operations + - Promise handling issues (unhandled rejections) + - State updates that could be overwritten + +4. **Error Handling** + - Missing try-catch blocks around fallible operations + - Swallowed errors (empty catch blocks) + - Incorrect error propagation + - Resource cleanup in error paths + +5. **Resource Leaks** + - Unclosed file handles, connections, or streams + - Missing cleanup in finally blocks + - Event listeners not removed + - Timers/intervals not cleared + +6. **Type Mismatches** + - Implicit type coercion issues + - Incorrect type assertions + - Function signature mismatches + - Incompatible type assignments + +## Best Practices to Recommend + +Beyond finding bugs, suggest improvements when you notice: +- Missing defensive coding patterns (guard clauses, assertions) +- Opportunities for better error handling structure +- Input validation that could prevent future bugs +- Null safety improvements (optional chaining, nullish coalescing) +- Missing type narrowing or runtime checks + +Use **LOW** severity for best practice recommendations (vs actual bugs). + +## Review Process + +1. Read the full file content for each changed file to understand context +2. Focus on the changed lines but consider how they interact with surrounding code +3. Look for edge cases the code doesn't handle +4. Verify error paths are properly handled + +## Before Flagging Anything + +- **Be certain** - Don't flag something as a bug if you're unsure. Investigate first. +- **Don't invent hypothetical problems** - If an edge case matters, explain the realistic scenario where it occurs. +- **Only review the changes** - Don't flag pre-existing code that wasn't modified in this PR. +- **Communicate severity honestly** - Don't overstate. A minor issue is not HIGH severity. + +## Severity Levels + +- **CRITICAL**: Will cause crashes, data corruption, or major functionality breakage +- **HIGH**: Likely to cause bugs in production under normal usage +- **MEDIUM**: Could cause issues in edge cases or specific conditions +- **LOW**: Minor issues that are unlikely to cause problems + +## Output Format + +Report each issue using this exact format: + +``` +#### Issue N: Brief description +**ID:** bug-{filename}-{2-4-key-terms}-{hash} +**File:** path/to/file.ext:lineNumber +**Severity:** CRITICAL/HIGH/MEDIUM/LOW +**Category:** bug + +**Context:** +- **Pattern:** What the problematic code pattern is +- **Risk:** Why it's a problem technically +- **Impact:** Potential consequences (crash, data corruption, etc.) +- **Trigger:** Under what conditions this bug manifests + +**Recommendation:** Fix with minimal code snippet (1-10 lines). +``` + +**ID Generation:** bug-{filename}-{2-4-key-terms}-{SHA256(path+desc).substr(0,4)} +Example: bug-cache-race-condition-a1b2 + +If no bugs found: "No bug issues found." diff --git a/claude/auto-review/agents/review-patterns.md b/claude/auto-review/agents/review-patterns.md new file mode 100644 index 0000000..68842a0 --- /dev/null +++ b/claude/auto-review/agents/review-patterns.md @@ -0,0 +1,128 @@ +# Patterns Review Agent + +You are a code reviewer. Provide actionable feedback on code changes. + +**Diffs alone are not enough.** Read the full file(s) being modified to understand context. Code that looks wrong in isolation may be correct given surrounding logic. + +Your specialization: code quality, architecture, and domain-specific compliance. + +**Scope:** Your review is not limited strictly to the focus areas below. Within the code quality domain, also proactively recommend missing best practices - even when no explicit anti-pattern exists. + +## Your Focus Areas + +### Code Quality & Architecture + +1. **Anti-Patterns** + - God objects/classes with too many responsibilities + - Spaghetti code with tangled dependencies + - Copy-paste code (DRY violations) + - Magic numbers/strings without constants + - Deep nesting (arrow anti-pattern) + +2. **Code Smells** + - Long methods/functions (> 50 lines) + - Long parameter lists (> 4 parameters) + - Feature envy (method uses other object's data more than its own) + - Inappropriate intimacy (classes too coupled) + - Dead code + +3. **Performance Issues** + - N+1 query patterns + - Missing database indexes for queried fields + - Unnecessary re-renders in React components + - Memory leaks (unbounded caches, retained references) + - Blocking operations in async contexts + - Inefficient algorithms (O(n^2) when O(n) is possible) + +4. **Missing Abstractions** + - Repeated logic that should be extracted + - Missing interfaces/protocols for polymorphism + - Hardcoded values that should be configurable + - Missing error types for domain errors + +### Domain-Specific Checks + +**Only report if violations found. Skip check if none detected.** + +5. **External Domain URLs** + Flag URLs to domains other than reown.com, walletconnect.com, walletconnect.org: + 🔒 **External Domain URL** (Non-blocking) **URL:** [url] **File:** [path:line] - Verify intentional. + +6. **Static Resource Cache-Control** + Flag static files (.woff, .woff2, .ttf, .jpg, .png, .css, .js, .mp4) with max-age < 31536000 or missing Cache-Control: + ⚠️ **Cache-Control Issue** **Resource:** [url] **File:** [path:line] **Current:** [value] **Recommendation:** "Cache-Control: public, max-age=31536000, immutable" + +7. **GitHub Actions Workflow Security** + Scan .github/workflows/*.y*ml for: + - **CRITICAL:** pull_request_target + PR head checkout = arbitrary code execution + - **HIGH:** pull_request_target + script execution + - **MEDIUM:** Any pull_request_target usage (runs with secrets) + +8. **WalletConnect Pay Architecture** + Flag anti-patterns in payment/wallet/transaction code: + - **CRITICAL:** Cross-service DB access → Services must use APIs + - **HIGH:** Missing idempotency keys in mutations → Extract key, check store, return cached + - **HIGH:** External calls without timeout/retry → Add timeout, retry+backoff, circuit breaker + - **HIGH:** Event consumers without deduplication → Check message ID before mutations + - **MEDIUM:** Multi-step workflows without saga compensation → Add rollback/compensating events + - **MEDIUM:** State transitions without trace context → Add traceId/correlationId logging + +## Best Practices to Recommend + +Beyond finding anti-patterns, suggest improvements when you notice: +- Opportunities for better code organization or structure +- Missing abstractions that would improve maintainability +- Testability improvements (dependency injection, pure functions) +- Documentation needs for complex logic +- Consistency improvements with existing codebase patterns +- Opportunities for better naming or self-documenting code + +Use **LOW** severity for best practice recommendations (vs actual issues). + +## Review Process + +1. Read the full file content for each changed file to understand context +2. Identify patterns and anti-patterns in the code structure +3. Check for performance implications of the changes +4. Verify domain-specific compliance rules +5. Consider maintainability impact + +## Before Flagging Anything + +- **Be certain** - Don't flag something as an anti-pattern if you're unsure. Investigate first. +- **Don't invent hypothetical problems** - If a pattern issue matters, explain the realistic impact. +- **Only review the changes** - Don't flag pre-existing code that wasn't modified in this PR. +- **Don't be a zealot about style** - Some "violations" are acceptable when they're the simplest option. +- **Communicate severity honestly** - Don't overstate. A minor style issue is not HIGH severity. + +## Severity Levels + +- **CRITICAL**: Severe issue (e.g., workflow security vulnerabilities, cross-service DB access) +- **HIGH**: Significant maintainability or performance issue +- **MEDIUM**: Code quality issue that should be addressed +- **LOW**: Minor improvement opportunity + +## Output Format + +Report each issue using this exact format: + +``` +#### Issue N: Brief description +**ID:** pat-{filename}-{2-4-key-terms}-{hash} +**File:** path/to/file.ext:lineNumber +**Severity:** CRITICAL/HIGH/MEDIUM/LOW +**Category:** patterns/performance/architecture/domain-compliance + +**Context:** +- **Pattern:** What the problematic code pattern is +- **Risk:** Why it's a problem technically +- **Impact:** Potential consequences (performance, maintainability, etc.) +- **Trigger:** Under what conditions this becomes problematic + +**Recommendation:** Fix with minimal code snippet (1-10 lines). +``` + +**ID Generation:** pat-{filename}-{2-4-key-terms}-{SHA256(path+desc).substr(0,4)} +Example: pat-userservi-n-plus-one-c3d4 + +If no pattern issues found: "No pattern issues found." diff --git a/claude/auto-review/agents/review-security.md b/claude/auto-review/agents/review-security.md new file mode 100644 index 0000000..70ae30b --- /dev/null +++ b/claude/auto-review/agents/review-security.md @@ -0,0 +1,120 @@ +# Security Review Agent + +You are a code reviewer. Provide actionable feedback on code changes. + +**Diffs alone are not enough.** Read the full file(s) being modified to understand context. Code that looks wrong in isolation may be correct given surrounding logic. + +Your specialization: finding security vulnerabilities. + +**Scope:** Your review is not limited strictly to the focus areas below. Within the security domain, also proactively recommend missing best practices and security hardening - even when no explicit vulnerability exists. + +## Your Focus Areas + +Analyze the PR changes for: + +1. **Injection Vulnerabilities** + - SQL injection (string concatenation in queries) + - Command injection (shell command construction) + - NoSQL injection (MongoDB query construction) + - Template injection (user input in templates) + - LDAP injection + +2. **Authentication & Authorization** + - Missing authentication checks + - Broken access control (IDOR vulnerabilities) + - Insecure session management + - Hardcoded credentials or API keys + - Weak password handling + - Missing CSRF protection + +3. **Data Exposure** + - Sensitive data in logs + - PII exposure in responses + - Credentials in error messages + - Secrets in version control + - Insecure data storage + +4. **Cryptographic Weaknesses** + - Weak hashing algorithms (MD5, SHA1 for passwords) + - Insecure random number generation + - Hardcoded cryptographic keys + - Missing encryption for sensitive data + - Improper certificate validation + +5. **Network Security** + - SSRF vulnerabilities (user-controlled URLs) + - Open redirects + - Missing TLS enforcement + - Insecure CORS configuration + - Unsafe deserialization + +6. **Path Traversal & File Handling** + - Path traversal in file operations + - Arbitrary file read/write + - Unsafe file uploads + - Symlink attacks + +7. **Input Validation** + - Missing input sanitization + - Prototype pollution + - ReDoS vulnerabilities (unsafe regex) + - Integer overflow/underflow + +## Best Practices to Recommend + +Beyond finding vulnerabilities, suggest security hardening when you notice: +- Missing input validation/sanitization (even if not currently exploitable) +- Opportunities for defense-in-depth (multiple security layers) +- Missing security headers or secure defaults +- Logging improvements for security audit trails +- Rate limiting or abuse prevention opportunities +- Secure coding patterns that could prevent future vulnerabilities + +Use **LOW** severity for best practice recommendations (vs actual vulnerabilities). + +## Review Process + +1. Read the full file content for each changed file to understand context +2. Trace data flow from untrusted sources to sensitive sinks +3. Check authorization boundaries at all entry points +4. Verify cryptographic implementations follow best practices +5. Look for OWASP Top 10 vulnerabilities + +## Before Flagging Anything + +- **Be certain** - Don't flag something as a vulnerability if you're unsure. Investigate first. +- **Don't invent hypothetical problems** - If an attack vector matters, explain the realistic scenario where it's exploitable. +- **Only review the changes** - Don't flag pre-existing code that wasn't modified in this PR. +- **Communicate severity honestly** - Don't overstate. A theoretical issue with no clear exploit path is not CRITICAL. + +## Severity Levels + +- **CRITICAL**: Exploitable vulnerability allowing RCE, auth bypass, or full data breach +- **HIGH**: Significant vulnerability requiring immediate remediation +- **MEDIUM**: Security weakness with limited exploitability +- **LOW**: Minor security improvement opportunity + +## Output Format + +Report each issue using this exact format: + +``` +#### Issue N: Brief description +**ID:** sec-{filename}-{2-4-key-terms}-{hash} +**File:** path/to/file.ext:lineNumber +**Severity:** CRITICAL/HIGH/MEDIUM/LOW +**Category:** security + +**Context:** +- **Pattern:** What the vulnerable code pattern is +- **Risk:** Why it's exploitable technically +- **Impact:** Potential consequences (RCE, data breach, auth bypass, etc.) +- **Trigger:** Under what conditions this becomes exploitable + +**Recommendation:** Fix with minimal code snippet (1-10 lines). +``` + +**ID Generation:** sec-{filename}-{2-4-key-terms}-{SHA256(path+desc).substr(0,4)} +Example: sec-users-sql-injection-f3a2 + +If no security issues found: "No security issues found." diff --git a/claude/auto-review/scripts/__tests__/determine-agents.test.js b/claude/auto-review/scripts/__tests__/determine-agents.test.js new file mode 100644 index 0000000..1260586 --- /dev/null +++ b/claude/auto-review/scripts/__tests__/determine-agents.test.js @@ -0,0 +1,602 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { + determineAgents, + categorizeFiles, + fetchPrFiles, + fetchPrLabels, +} from "../determine-agents.js"; +import { ghApi } from "../lib/github-utils.js"; + +// Mock ghApi +vi.mock("../lib/github-utils.js", async () => { + const actual = await vi.importActual("../lib/github-utils.js"); + return { + ...actual, + ghApi: vi.fn(), + }; +}); + +describe("categorizeFiles", () => { + it("should count files and lines correctly", () => { + const files = [ + { filename: "src/app.ts", additions: 10, deletions: 5 }, + { filename: "src/utils.ts", additions: 20, deletions: 10 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.totalFiles).toBe(2); + expect(stats.totalLines).toBe(45); + expect(stats.maxSingleFileLines).toBe(30); + }); + + it("should identify docs-only PRs", () => { + const files = [ + { filename: "README.md", additions: 10, deletions: 0 }, + { filename: "docs/guide.rst", additions: 5, deletions: 2 }, + { filename: "CHANGELOG.txt", additions: 3, deletions: 1 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.docsOnly).toBe(true); + expect(stats.testOnly).toBe(false); + }); + + it("should identify test-only PRs", () => { + const files = [ + { filename: "src/__tests__/app.test.ts", additions: 50, deletions: 10 }, + { filename: "tests/utils.spec.js", additions: 30, deletions: 5 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.testOnly).toBe(true); + expect(stats.docsOnly).toBe(false); + }); + + it("should identify rename-only PRs", () => { + const files = [ + { filename: "src/newName.ts", status: "renamed", changes: 0, additions: 0, deletions: 0 }, + { filename: "src/another.ts", status: "renamed", changes: 0, additions: 0, deletions: 0 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.renameOnly).toBe(true); + }); + + it("should identify lockfile-only PRs", () => { + const files = [ + { filename: "package-lock.json", additions: 100, deletions: 50 }, + { filename: "yarn.lock", additions: 200, deletions: 100 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.lockfileOnly).toBe(true); + }); + + it("should detect workflow files", () => { + const files = [ + { filename: ".github/workflows/ci.yml", additions: 10, deletions: 0 }, + { filename: "src/app.ts", additions: 5, deletions: 2 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.hasWorkflowFiles).toBe(true); + }); + + it("should detect auth-related files", () => { + const files = [ + { filename: "src/auth/login.ts", additions: 20, deletions: 5 }, + { filename: "src/session.ts", additions: 10, deletions: 3 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.hasAuthFiles).toBe(true); + }); + + it("should detect SQL/migration files", () => { + const files = [ + { filename: "db/migrations/001_create_users.sql", additions: 30, deletions: 0 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.hasSqlFiles).toBe(true); + }); + + it("should detect infrastructure files", () => { + const files = [ + { filename: "Dockerfile", additions: 15, deletions: 5 }, + { filename: "terraform/main.tf", additions: 50, deletions: 10 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.hasInfraFiles).toBe(true); + }); + + it("should detect dependency files", () => { + const files = [ + { filename: "package.json", additions: 3, deletions: 1 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.hasDepFiles).toBe(true); + }); + + it("should find security keywords in patches", () => { + const files = [ + { + filename: "src/api.ts", + additions: 10, + deletions: 5, + patch: `+const token = process.env.API_TOKEN;\n+const result = await fetch(url);`, + }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.securityKeywords).toContain("token"); + expect(stats.securityKeywords).toContain("fetch"); + }); + + it("should find patterns keywords in patches", () => { + const files = [ + { + filename: "src/component.tsx", + additions: 20, + deletions: 5, + patch: `+useEffect(() => {\n+ // Connect to walletconnect.com\n+}, []);`, + }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.patternsKeywords).toContain("useEffect"); + expect(stats.patternsKeywords).toContain("walletconnect\\.com"); + }); + + it("should exclude removed files from code file count", () => { + const files = [ + { filename: "src/old.ts", status: "removed", additions: 0, deletions: 50 }, + { filename: "src/new.ts", status: "added", additions: 30, deletions: 0 }, + ]; + + const stats = categorizeFiles(files); + + expect(stats.codeFiles).toBe(1); + }); +}); + +describe("determineAgents", () => { + describe("label overrides", () => { + it("should skip all agents with skip-review label", () => { + const files = [{ filename: "src/app.ts", additions: 100, deletions: 50 }]; + + const result = determineAgents(files, { labels: ["skip-review"] }); + + expect(result.agents).toEqual([]); + expect(result.skipped).toEqual(["bug", "security", "patterns"]); + expect(result.reason).toContain("skip-review"); + }); + + it("should spawn all agents with full-review label", () => { + const files = [{ filename: "README.md", additions: 5, deletions: 0 }]; + + const result = determineAgents(files, { labels: ["full-review"] }); + + expect(result.agents).toEqual(["bug", "security", "patterns"]); + expect(result.skipped).toEqual([]); + expect(result.reason).toContain("full-review"); + }); + + it("should be case-insensitive for labels", () => { + const files = [{ filename: "src/app.ts", additions: 10, deletions: 5 }]; + + const result = determineAgents(files, { labels: ["FULL-REVIEW"] }); + + expect(result.agents).toEqual(["bug", "security", "patterns"]); + }); + }); + + describe("forceAllAgents flag", () => { + it("should spawn all agents when forceAllAgents is true", () => { + const files = [{ filename: "README.md", additions: 5, deletions: 0 }]; + + const result = determineAgents(files, { forceAllAgents: true }); + + expect(result.agents).toEqual(["bug", "security", "patterns"]); + expect(result.reason).toContain("Force all agents"); + }); + + it("should override skip-review label when forceAllAgents is true", () => { + const files = [{ filename: "src/app.ts", additions: 10, deletions: 5 }]; + + const result = determineAgents(files, { + labels: ["skip-review"], + forceAllAgents: true, + }); + + expect(result.agents).toEqual(["bug", "security", "patterns"]); + }); + }); + + describe("edge cases", () => { + it("should skip all agents for docs-only changes", () => { + const files = [ + { filename: "README.md", additions: 10, deletions: 5 }, + { filename: "docs/guide.md", additions: 20, deletions: 10 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toEqual([]); + expect(result.reason).toContain("Docs-only"); + }); + + it("should skip all agents for rename-only changes", () => { + const files = [ + { filename: "src/newName.ts", status: "renamed", changes: 0, additions: 0, deletions: 0 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toEqual([]); + expect(result.reason).toContain("Rename-only"); + }); + + it("should only spawn security for lockfile-only changes", () => { + const files = [ + { filename: "package-lock.json", additions: 500, deletions: 200 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toEqual(["security"]); + expect(result.reason).toContain("Lockfile-only"); + expect(result.skipped).toContain("bug"); + expect(result.skipped).toContain("patterns"); + }); + + it("should only spawn bug for test-only changes", () => { + const files = [ + { filename: "src/__tests__/app.test.ts", additions: 50, deletions: 10 }, + { filename: "tests/utils.spec.js", additions: 30, deletions: 5 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toEqual(["bug"]); + expect(result.reason).toContain("Test-only"); + expect(result.skipped).toContain("security"); + expect(result.skipped).toContain("patterns"); + }); + }); + + describe("large PRs", () => { + it("should spawn all agents for PRs with >500 lines", () => { + const files = [ + { filename: "src/big.ts", additions: 300, deletions: 250 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toEqual(["bug", "security", "patterns"]); + expect(result.reason).toContain("Large PR"); + expect(result.reason).toContain("550 lines"); + }); + + it("should spawn all agents for PRs with >15 files", () => { + const files = Array.from({ length: 16 }, (_, i) => ({ + filename: `src/file${i}.ts`, + additions: 5, + deletions: 2, + })); + + const result = determineAgents(files); + + expect(result.agents).toEqual(["bug", "security", "patterns"]); + expect(result.reason).toContain("Large PR"); + expect(result.reason).toContain("16 files"); + }); + }); + + describe("security agent triggers", () => { + it("should spawn security for workflow files", () => { + const files = [ + { filename: ".github/workflows/ci.yml", additions: 10, deletions: 5 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("security"); + expect(result.reason).toContain("workflow files"); + }); + + it("should spawn security for auth-related files", () => { + const files = [ + { filename: "src/auth/login.ts", additions: 20, deletions: 5 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("security"); + expect(result.reason).toContain("auth files"); + }); + + it("should spawn security for SQL files", () => { + const files = [ + { filename: "db/migrations/001.sql", additions: 30, deletions: 0 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("security"); + expect(result.reason).toContain("SQL/migration files"); + }); + + it("should spawn security for infrastructure files", () => { + const files = [ + { filename: "Dockerfile", additions: 15, deletions: 5 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("security"); + expect(result.reason).toContain("infrastructure files"); + }); + + it("should spawn security for dependency files", () => { + const files = [ + { filename: "package.json", additions: 3, deletions: 1 }, + { filename: "src/app.ts", additions: 10, deletions: 5 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("security"); + expect(result.reason).toContain("dependency files"); + }); + + it("should spawn security when security keywords found in patch", () => { + const files = [ + { + filename: "src/api.ts", + additions: 10, + deletions: 5, + patch: `+const secret = getSecret();\n+const token = generateToken();`, + }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("security"); + expect(result.reason).toContain("security keywords"); + }); + }); + + describe("patterns agent triggers", () => { + it("should spawn patterns for workflow files", () => { + const files = [ + { filename: ".github/workflows/ci.yml", additions: 10, deletions: 5 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("patterns"); + expect(result.reason).toContain("workflow files"); + }); + + it("should spawn patterns for large single files (>300 lines)", () => { + const files = [ + { filename: "src/bigfile.ts", additions: 200, deletions: 150 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("patterns"); + expect(result.reason).toContain("large file"); + expect(result.reason).toContain("350 lines"); + }); + + it("should spawn patterns for >5 code files", () => { + const files = Array.from({ length: 6 }, (_, i) => ({ + filename: `src/file${i}.ts`, + additions: 10, + deletions: 5, + })); + + const result = determineAgents(files); + + expect(result.agents).toContain("patterns"); + expect(result.reason).toContain("6 code files"); + }); + + it("should spawn patterns when patterns keywords found", () => { + const files = [ + { + filename: "src/component.tsx", + additions: 10, + deletions: 5, + patch: `+useEffect(() => { console.log('mounted'); }, []);`, + }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("patterns"); + expect(result.reason).toContain("patterns keywords"); + }); + }); + + describe("small code PRs", () => { + it("should only spawn bug for trivial code changes", () => { + const files = [ + { filename: "src/app.ts", additions: 5, deletions: 2 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toEqual(["bug"]); + expect(result.skipped).toContain("security"); + expect(result.skipped).toContain("patterns"); + }); + + it("should include bug but not patterns for medium PR without triggers", () => { + const files = [ + { filename: "src/app.ts", additions: 50, deletions: 20 }, + { filename: "src/utils.ts", additions: 30, deletions: 10 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("bug"); + expect(result.skipped).toContain("patterns"); + }); + }); + + describe("mixed file types", () => { + it("should apply heuristic to code files when mixed with docs", () => { + const files = [ + { filename: "README.md", additions: 50, deletions: 20 }, + { filename: "src/auth.ts", additions: 15, deletions: 5 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toContain("bug"); + expect(result.agents).toContain("security"); + expect(result.docsOnly).toBeUndefined(); // Mixed, not docs-only + }); + + it("should detect both security and patterns triggers", () => { + const files = [ + { filename: ".github/workflows/ci.yml", additions: 10, deletions: 0 }, + { filename: "src/auth/login.ts", additions: 200, deletions: 150 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toEqual(["bug", "security", "patterns"]); + }); + }); + + describe("empty or minimal PRs", () => { + it("should handle empty file list", () => { + const result = determineAgents([]); + + expect(result.agents).toEqual([]); + expect(result.reason).toContain("Empty PR"); + expect(result.skipped).toEqual(["bug", "security", "patterns"]); + }); + + it("should handle files with zero changes", () => { + const files = [ + { filename: "src/app.ts", additions: 0, deletions: 0 }, + ]; + + const result = determineAgents(files); + + expect(result.agents).toEqual(["bug"]); + }); + }); +}); + +describe("fetchPrFiles", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + const mockContext = { + repo: { owner: "walletconnect", repo: "test-repo" }, + issue: { number: 123 }, + }; + + it("should fetch files from GitHub API", async () => { + const mockFiles = [ + { filename: "src/app.ts", additions: 10, deletions: 5 }, + ]; + ghApi.mockReturnValue(mockFiles); + + const files = await fetchPrFiles(mockContext); + + expect(ghApi).toHaveBeenCalledWith( + "/repos/walletconnect/test-repo/pulls/123/files" + ); + expect(files).toEqual(mockFiles); + }); + + it("should return empty array on error", async () => { + ghApi.mockImplementation(() => { + throw new Error("API error"); + }); + + const files = await fetchPrFiles(mockContext); + + expect(files).toEqual([]); + }); + + it("should return empty array when API returns null", async () => { + ghApi.mockReturnValue(null); + + const files = await fetchPrFiles(mockContext); + + expect(files).toEqual([]); + }); +}); + +describe("fetchPrLabels", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + const mockContext = { + repo: { owner: "walletconnect", repo: "test-repo" }, + issue: { number: 123 }, + }; + + it("should fetch labels from GitHub API", async () => { + ghApi.mockReturnValue({ + labels: [{ name: "bug" }, { name: "full-review" }], + }); + + const labels = await fetchPrLabels(mockContext); + + expect(ghApi).toHaveBeenCalledWith( + "/repos/walletconnect/test-repo/pulls/123" + ); + expect(labels).toEqual(["bug", "full-review"]); + }); + + it("should return empty array on error", async () => { + ghApi.mockImplementation(() => { + throw new Error("API error"); + }); + + const labels = await fetchPrLabels(mockContext); + + expect(labels).toEqual([]); + }); + + it("should handle PR with no labels", async () => { + ghApi.mockReturnValue({ labels: [] }); + + const labels = await fetchPrLabels(mockContext); + + expect(labels).toEqual([]); + }); + + it("should handle missing labels field", async () => { + ghApi.mockReturnValue({}); + + const labels = await fetchPrLabels(mockContext); + + expect(labels).toEqual([]); + }); +}); diff --git a/claude/auto-review/scripts/__tests__/extract-findings-from-comment.test.js b/claude/auto-review/scripts/__tests__/extract-findings-from-comment.test.js index a46ab51..f895d3d 100644 --- a/claude/auto-review/scripts/__tests__/extract-findings-from-comment.test.js +++ b/claude/auto-review/scripts/__tests__/extract-findings-from-comment.test.js @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import fs from 'fs'; import path from 'path'; import { fileURLToPath } from 'url'; @@ -222,6 +222,51 @@ This is the issue with no file specified.`; expect(findings[0].file).toBeUndefined(); expect(findings[0].description).toBe('Missing file info'); }); + + it('should parse multi-agent output and extract agent from ID prefix', () => { + const comment = fs.readFileSync( + path.join(fixturesDir, 'claude-comments', 'multi-agent-output.md'), + 'utf8' + ); + + const findings = parseClaudeComment(comment); + + expect(findings).toHaveLength(4); + + // Security agent issue (sec- prefix) + expect(findings[0].id).toBe('sec-users-sql-injection-f3a2'); + expect(findings[0].agent).toBe('review-security'); + expect(findings[0].description).toBe('SQL injection vulnerability in user query'); + + // Bug agent issue (bug- prefix) + expect(findings[1].id).toBe('bug-cache-race-condition-a1b2'); + expect(findings[1].agent).toBe('review-bugs'); + expect(findings[1].description).toBe('Race condition in cache update'); + + // Patterns agent issue (pat- prefix) + expect(findings[2].id).toBe('pat-userservi-n-plus-one-c3d4'); + expect(findings[2].agent).toBe('review-patterns'); + expect(findings[2].description).toBe('N+1 query pattern in user fetch'); + + // Another bug agent issue + expect(findings[3].id).toBe('bug-auth-missing-validation-8b91'); + expect(findings[3].agent).toBe('review-bugs'); + }); + + it('should not set agent for IDs without recognized prefix', () => { + const comment = `#### Issue 1: Test issue +**ID:** users-sql-injection-f3a2 +**File:** test.ts:1 +**Severity:** HIGH + +Problem.`; + + const findings = parseClaudeComment(comment); + + expect(findings).toHaveLength(1); + expect(findings[0].id).toBe('users-sql-injection-f3a2'); + expect(findings[0].agent).toBeUndefined(); + }); }); describe('getLatestClaudeComment', () => { diff --git a/claude/auto-review/scripts/__tests__/fixtures/claude-comments/multi-agent-output.md b/claude/auto-review/scripts/__tests__/fixtures/claude-comments/multi-agent-output.md new file mode 100644 index 0000000..12e60ff --- /dev/null +++ b/claude/auto-review/scripts/__tests__/fixtures/claude-comments/multi-agent-output.md @@ -0,0 +1,74 @@ +
+Found 4 issue(s) + +#### Issue 1: SQL injection vulnerability in user query +**ID:** sec-users-sql-injection-f3a2 +**File:** src/database/users.ts:45 +**Severity:** HIGH +**Category:** security + +**Context:** +- **Pattern:** Query at line 45 builds SQL via string concatenation with user-provided `userId` +- **Risk:** Allows arbitrary SQL injection via crafted input +- **Impact:** Unauthorized data access, modification, or database destruction +- **Trigger:** Any endpoint accepting user input that reaches this query + +**Recommendation:** Use parameterized queries: +```typescript +const result = await db.query('SELECT * FROM users WHERE id = $1', [userId]); +``` + +#### Issue 2: Race condition in cache update +**ID:** bug-cache-race-condition-a1b2 +**File:** src/services/cache.ts:89 +**Severity:** HIGH +**Category:** bug + +**Context:** +- **Pattern:** Cache read at line 85 and write at line 89 are separate non-atomic operations +- **Risk:** Concurrent requests can read stale data and overwrite each other's updates +- **Impact:** Data inconsistency, lost updates, stale cache serving +- **Trigger:** Multiple simultaneous requests updating the same cache key + +**Recommendation:** Use atomic operations: +```typescript +await cache.setNX(key, value, { EX: ttl }); +``` + +#### Issue 3: N+1 query pattern in user fetch +**ID:** pat-userservi-n-plus-one-c3d4 +**File:** src/services/userService.ts:156 +**Severity:** MEDIUM +**Category:** performance + +**Context:** +- **Pattern:** Loop at line 156 executes individual DB query per user ID +- **Risk:** O(n) database round-trips instead of O(1) batch query +- **Impact:** Degraded performance, database connection exhaustion under load +- **Trigger:** Fetching multiple users (e.g., listing team members) + +**Recommendation:** Use batch fetching: +```typescript +const users = await db.query('SELECT * FROM users WHERE id = ANY($1)', [userIds]); +``` + +#### Issue 4: Missing input validation +**ID:** bug-auth-missing-validation-8b91 +**File:** src/auth/login.ts:23 +**Severity:** MEDIUM +**Category:** bug + +**Context:** +- **Pattern:** Login function accepts email parameter without format validation +- **Risk:** Invalid email formats passed to downstream services +- **Impact:** Unexpected errors, failed lookups, or security bypass attempts +- **Trigger:** User submits malformed email in login form + +**Recommendation:** Add email validation: +```typescript +if (!isValidEmail(email)) { + throw new Error('Invalid email format'); +} +``` + +
diff --git a/claude/auto-review/scripts/determine-agents.js b/claude/auto-review/scripts/determine-agents.js new file mode 100644 index 0000000..63eed6f --- /dev/null +++ b/claude/auto-review/scripts/determine-agents.js @@ -0,0 +1,396 @@ +#!/usr/bin/env node + +/** + * Determines which review subagents to spawn based on PR characteristics. + * Implements a conservative heuristic that errs on the side of spawning agents. + */ + +import { ghApi, loadGitHubContext, createLogger } from "./lib/github-utils.js"; + +const logger = createLogger("determine-agents"); + +// All available agents +const ALL_AGENTS = ["bug", "security", "patterns"]; + +// File pattern matchers +const PATTERNS = { + docs: /\.(md|txt|rst|mdx)$/i, + tests: /\.(test|spec)\.[jt]sx?$|__tests__\//i, + workflows: /\.github\/workflows\/.*\.ya?ml$/, + auth: /auth|login|session/i, + sql: /\.sql$|\/migrations\//i, + secrets: /\.env|secrets?|config\//i, + infra: /Dockerfile|\.tf$|\.tfvars$/, + lockfiles: /\.lock$|package-lock\.json$|yarn\.lock$|pnpm-lock\.yaml$|go\.sum$/, + deps: /package\.json$|go\.mod$|requirements\.txt$|Gemfile$/, +}; + +// Keywords that trigger security agent +const SECURITY_KEYWORDS = [ + "password", + "secret", + "token", + "api_key", + "apikey", + "credential", + "jwt", + "bearer", + "crypto", + "hash", + "encrypt", + "decrypt", + "md5", + "sha1", + "exec", + "spawn", + "shell", + "eval", + "query", + "sql", + "fetch", + "axios", + "http", + "redirect", + "cors", + "readFile", + "writeFile", + "fs\\.", + "path\\.join", +]; + +// Keywords that trigger patterns agent +const PATTERNS_KEYWORDS = [ + "walletconnect\\.com", + "reown\\.com", + "Cache-Control", + "max-age", + "useEffect", + "useMemo", + "useCallback", +]; + +/** + * Check if a file matches a pattern category + * @param {string} filename + * @param {string} category - Key from PATTERNS + * @returns {boolean} + */ +function matchesPattern(filename, category) { + return PATTERNS[category]?.test(filename) ?? false; +} + +/** + * Check if patch content contains any keywords from a list + * @param {string} patch - Diff patch content + * @param {string[]} keywords + * @returns {string[]} - Matching keywords found + */ +function findKeywords(patch, keywords) { + if (!patch) return []; + const found = []; + for (const keyword of keywords) { + const regex = new RegExp(keyword, "i"); + if (regex.test(patch)) { + found.push(keyword); + } + } + return found; +} + +/** + * Categorize PR files and compute statistics + * @param {Array<{filename: string, status: string, additions: number, deletions: number, changes: number, patch?: string}>} files + * @returns {Object} + */ +export function categorizeFiles(files) { + const codeFiles = files.filter( + (f) => !matchesPattern(f.filename, "docs") && f.status !== "removed" + ); + + const stats = { + totalFiles: files.length, + codeFiles: codeFiles.length, + totalLines: files.reduce((sum, f) => sum + (f.additions || 0) + (f.deletions || 0), 0), + maxSingleFileLines: Math.max( + 0, + ...files.map((f) => (f.additions || 0) + (f.deletions || 0)) + ), + + // Edge case flags + isEmpty: files.length === 0, + docsOnly: files.length > 0 && files.every((f) => matchesPattern(f.filename, "docs")), + testOnly: + codeFiles.length > 0 && + codeFiles.every((f) => matchesPattern(f.filename, "tests")), + renameOnly: + files.length > 0 && + files.every((f) => f.status === "renamed" && (f.changes || 0) === 0), + lockfileOnly: + files.length > 0 && files.every((f) => matchesPattern(f.filename, "lockfiles")), + + // Signal flags + hasWorkflowFiles: files.some((f) => matchesPattern(f.filename, "workflows")), + hasAuthFiles: files.some((f) => matchesPattern(f.filename, "auth")), + hasSqlFiles: files.some((f) => matchesPattern(f.filename, "sql")), + hasSecretFiles: files.some((f) => matchesPattern(f.filename, "secrets")), + hasInfraFiles: files.some((f) => matchesPattern(f.filename, "infra")), + hasDepFiles: files.some((f) => matchesPattern(f.filename, "deps")), + }; + + // Aggregate patches for keyword search + const allPatches = files + .map((f) => f.patch || "") + .filter(Boolean) + .join("\n"); + + stats.securityKeywords = findKeywords(allPatches, SECURITY_KEYWORDS); + stats.patternsKeywords = findKeywords(allPatches, PATTERNS_KEYWORDS); + + return stats; +} + +/** + * Determine which agents to spawn based on PR characteristics + * @param {Array<{filename: string, status: string, additions: number, deletions: number, changes: number, patch?: string}>} prFiles + * @param {{labels?: string[], forceAllAgents?: boolean}} metadata + * @returns {{agents: string[], reason: string, skipped: string[]}} + */ +export function determineAgents(prFiles, metadata = {}) { + const { labels = [], forceAllAgents = false } = metadata; + + // Force all agents if requested + if (forceAllAgents) { + return { + agents: [...ALL_AGENTS], + reason: "Force all agents flag set", + skipped: [], + }; + } + + // Check override labels first + const labelSet = new Set(labels.map((l) => l.toLowerCase())); + + if (labelSet.has("skip-review")) { + return { + agents: [], + reason: "skip-review label present", + skipped: [...ALL_AGENTS], + }; + } + + if (labelSet.has("full-review")) { + return { + agents: [...ALL_AGENTS], + reason: "full-review label override", + skipped: [], + }; + } + + // Categorize files + const stats = categorizeFiles(prFiles); + + logger.log( + `Analyzing PR (${stats.totalFiles} files, ${stats.totalLines} lines changed)` + ); + logger.log( + `File signals: hasWorkflowFiles=${stats.hasWorkflowFiles}, hasAuthFiles=${stats.hasAuthFiles}, hasInfraFiles=${stats.hasInfraFiles}` + ); + logger.log( + `Content signals: securityKeywords=[${stats.securityKeywords.slice(0, 5).join(", ")}${stats.securityKeywords.length > 5 ? "..." : ""}]` + ); + + // Edge cases - no code to review + if (stats.isEmpty) { + return { + agents: [], + reason: "Empty PR (no files)", + skipped: [...ALL_AGENTS], + }; + } + + if (stats.docsOnly) { + return { + agents: [], + reason: "Docs-only change", + skipped: [...ALL_AGENTS], + }; + } + + if (stats.renameOnly) { + return { + agents: [], + reason: "Rename-only change (no content modifications)", + skipped: [...ALL_AGENTS], + }; + } + + // Edge cases - limited review + if (stats.lockfileOnly) { + return { + agents: ["security"], + reason: "Lockfile-only change (supply chain review)", + skipped: ["bug", "patterns"], + }; + } + + if (stats.testOnly) { + return { + agents: ["bug"], + reason: "Test-only change", + skipped: ["security", "patterns"], + }; + } + + // Large PR → all agents + if (stats.totalLines > 500 || stats.totalFiles > 15) { + return { + agents: [...ALL_AGENTS], + reason: `Large PR (${stats.totalFiles} files, ${stats.totalLines} lines)`, + skipped: [], + }; + } + + // Build agent list based on signals + const agents = ["bug"]; // Always include bug for code changes + const reasons = ["Code changes present"]; + const skipped = []; + + // Security agent triggers + const needsSecurity = + stats.hasWorkflowFiles || + stats.hasAuthFiles || + stats.hasSqlFiles || + stats.hasSecretFiles || + stats.hasInfraFiles || + stats.hasDepFiles || + stats.securityKeywords.length > 0; + + if (needsSecurity) { + agents.push("security"); + const secReasons = []; + if (stats.hasWorkflowFiles) secReasons.push("workflow files"); + if (stats.hasAuthFiles) secReasons.push("auth files"); + if (stats.hasSqlFiles) secReasons.push("SQL/migration files"); + if (stats.hasSecretFiles) secReasons.push("config/secret files"); + if (stats.hasInfraFiles) secReasons.push("infrastructure files"); + if (stats.hasDepFiles) secReasons.push("dependency files"); + if (stats.securityKeywords.length > 0) secReasons.push("security keywords"); + reasons.push(`Security: ${secReasons.join(", ")}`); + } else { + skipped.push("security"); + } + + // Patterns agent triggers (conservative: >300 lines single file OR >5 files OR workflow files) + const needsPatterns = + stats.hasWorkflowFiles || + stats.maxSingleFileLines > 300 || + stats.codeFiles > 5 || + stats.patternsKeywords.length > 0; + + if (needsPatterns) { + agents.push("patterns"); + const patReasons = []; + if (stats.hasWorkflowFiles) patReasons.push("workflow files"); + if (stats.maxSingleFileLines > 300) + patReasons.push(`large file (${stats.maxSingleFileLines} lines)`); + if (stats.codeFiles > 5) patReasons.push(`${stats.codeFiles} code files`); + if (stats.patternsKeywords.length > 0) patReasons.push("patterns keywords"); + reasons.push(`Patterns: ${patReasons.join(", ")}`); + } else { + skipped.push("patterns"); + } + + const result = { + agents, + reason: reasons.join("; "), + skipped, + }; + + logger.log(`Decision: Spawning [${agents.join(", ")}]`); + logger.log(`Reason: ${result.reason}`); + if (skipped.length > 0) { + logger.log(`Skipped: [${skipped.join(", ")}]`); + } + + return result; +} + +/** + * Fetch PR files from GitHub API + * @param {{repo: {owner: string, repo: string}, issue: {number: number}}} context + * @returns {Promise} + */ +export async function fetchPrFiles(context) { + const { owner, repo } = context.repo; + const prNumber = context.issue.number; + const endpoint = `/repos/${owner}/${repo}/pulls/${prNumber}/files`; + + try { + const files = ghApi(endpoint); + return files || []; + } catch (error) { + logger.error(`Failed to fetch PR files: ${error.message}`); + return []; + } +} + +/** + * Fetch PR labels from GitHub API + * @param {{repo: {owner: string, repo: string}, issue: {number: number}}} context + * @returns {Promise} + */ +export async function fetchPrLabels(context) { + const { owner, repo } = context.repo; + const prNumber = context.issue.number; + const endpoint = `/repos/${owner}/${repo}/pulls/${prNumber}`; + + try { + const pr = ghApi(endpoint); + return (pr?.labels || []).map((l) => l.name); + } catch (error) { + logger.error(`Failed to fetch PR labels: ${error.message}`); + return []; + } +} + +/** + * Main entry point when run as script + */ +async function main() { + const context = loadGitHubContext(); + + if (!context.issue.number) { + logger.error("No PR number found in context"); + // Return all agents as fallback + console.log(JSON.stringify({ agents: ALL_AGENTS, reason: "No PR context", skipped: [] })); + process.exit(0); + } + + // Check for force flag from environment + const forceAllAgents = process.env.FORCE_ALL_AGENTS === "true"; + + const [files, labels] = await Promise.all([ + fetchPrFiles(context), + fetchPrLabels(context), + ]); + + const result = determineAgents(files, { labels, forceAllAgents }); + + // Output as JSON for consumption by action.yml + console.log(JSON.stringify(result)); +} + +// Execution guard for direct invocation +if ( + import.meta.url === `file://${process.argv[1]}` || + process.argv[1]?.endsWith("determine-agents.js") +) { + main().catch((error) => { + logger.error(`Fatal error: ${error.message}`); + // Return all agents as fallback on error + console.log( + JSON.stringify({ agents: ALL_AGENTS, reason: "Error fallback", skipped: [] }) + ); + process.exit(0); + }); +} diff --git a/claude/auto-review/scripts/extract-findings-from-comment.js b/claude/auto-review/scripts/extract-findings-from-comment.js index 3b5e310..23ee400 100644 --- a/claude/auto-review/scripts/extract-findings-from-comment.js +++ b/claude/auto-review/scripts/extract-findings-from-comment.js @@ -53,9 +53,17 @@ export function parseClaudeComment(commentBody) { // Extract ID if present // Format: **ID:** file-slug-semantic-slug-hash + // Multi-agent format: **ID:** {agent-prefix}-file-slug-semantic-slug-hash (bug-, sec-, pat-) const idMatch = content.match(/\*\*ID:\*\*\s+([a-z0-9\-]+)/i); if (idMatch) { finding.id = idMatch[1].trim().toLowerCase(); + + // Extract agent from ID prefix (bug-, sec-, pat-) + const agentPrefixMatch = finding.id.match(/^(bug|sec|pat)-/); + if (agentPrefixMatch) { + const agentMap = { 'bug': 'review-bugs', 'sec': 'review-security', 'pat': 'review-patterns' }; + finding.agent = agentMap[agentPrefixMatch[1]]; + } } // Extract file path and line number