feat(multi-agent-review): spin sub-agents to review specific aspects

claude/auto-review/action.yml

@@ -100,145 +100,83 @@ runs:
 
         ---
 
-        ## REVIEW FOCUS AREAS
+        ## MULTI-AGENT REVIEW
 
-        Analyze code changes for issues in these areas:
-        - **Code quality and best practices** for the technologies used in this project
-        - **Potential bugs or issues** especially in critical code paths and async operations
-        - **Performance considerations** for both frontend and backend code
-        - **Security implications** particularly for authentication, API endpoints, and data handling
-        - **Test coverage** and quality of test implementations
-        - **Documentation updates** if needed for significant changes
-        - **Type safety** and proper usage of type systems
-        - **Error handling** and edge cases
-        - **Code maintainability** and readability
+        You will conduct a comprehensive code review using THREE specialized subagents running IN PARALLEL.
 
-        ---
+        ### Step 1: Spawn All Review Agents Simultaneously
 
-        ## AUTOMATED CHECKS
+        Use the Task tool to launch ALL THREE agents in a SINGLE message with multiple tool calls. This is critical for parallel execution.
 
-        ### External Domain URL Detection
+        **CRITICAL: Use subagent_type=\"general-purpose\" for all agents** and include the specialized instructions in each prompt.
 
-        Scan all changed files for URLs matching the pattern 'https?://(?:www\.)?([a-zA-Z0-9.-]+\.[a-zA-Z]{2,})'. **ONLY report if URLs are found** pointing to domains other than the approved company domains (reown.com, walletconnect.com, walletconnect.org). **If no external domain URLs are detected, do not mention this check at all.**
+        **Agents to spawn (all with subagent_type=\"general-purpose\"):**
 
-        If external domain URLs are found, report them using this exact format:
+        1. **Bug Review Agent** - Include in prompt:
+           - Focus: Logic errors, null handling, race conditions, error handling, resource leaks, type mismatches
+           - Severity: CRITICAL (crashes/data corruption), HIGH (production bugs), MEDIUM (edge cases), LOW (minor)
+           - ID prefix: bug-
+           - Output format: \\\"#### Issue N: description\\\" with **ID:** bug-{file-slug}-{semantic-slug}-{hash}
 
-        <example-snippet>
-        🔒 **External Domain URL Detected** (Non-blocking)
-        **URL:** [detected_url]
-        **File:** [file_path:line_number]
+        2. **Security Review Agent** - Include in prompt:
+           - Focus: Injection, auth/authz, data exposure, crypto weaknesses, SSRF, path traversal, input validation
+           - Severity: CRITICAL (RCE/auth bypass), HIGH (immediate fix), MEDIUM (limited exploit), LOW (improvement)
+           - ID prefix: sec-
+           - Include **Exploit Scenario:** for each finding
+           - Output format: \\\"#### Issue N: description\\\" with **ID:** sec-{file-slug}-{semantic-slug}-{hash}
 
-        This change introduces URLs pointing to external domains. Please verify that these external dependencies are intentional and review for potential security, privacy, or compliance implications. Approved company domains are: reown.com, walletconnect.com, walletconnect.org
-        </example-snippet>
+        3. **Patterns Review Agent** - Include in prompt:
+           - Focus: Anti-patterns, code smells, performance (N+1, memory leaks), missing abstractions
+           - Domain checks: External URLs (only flag non-reown.com/walletconnect.com/walletconnect.org), Cache-Control, GitHub Actions pull_request_target security, WalletConnect Pay architecture (cross-service DB, idempotency, resilience, saga compensation, trace context)
+           - Severity: HIGH (significant issue), MEDIUM (should address), LOW (improvement)
+           - ID prefix: pat-
+           - Output format: \\\"#### Issue N: description\\\" with **ID:** pat-{file-slug}-{semantic-slug}-{hash}
 
-        ### Static Resource Cache-Control Validation
+        **For EACH agent prompt, include:**
+        - This preamble: \\\"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.\\\"
+        - PR number and repository
+        - List of changed files
+        - The specific focus areas and severity levels for that agent type
+        - The ID format with the appropriate prefix (bug-, sec-, pat-)
+        - \\\"If no issues found, state: No [type] issues found.\\\"
+        - \\\"Wrap all issues in a collapsed \`<details>\` block.\\\"
 
-        Scan all changed files for static immutable resources (fonts, images, CSS, JS, media files) and their Cache-Control header configurations. **ONLY report if issues are found.** **If no cache-control issues are detected, do not mention this check at all.**
+        **CRITICAL - Before flagging anything, agents MUST:**
+        - **Be certain** - Don't flag something as a bug if 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.
 
-        Flag any issues using these guidelines:
+        ### Step 2: Consolidate Findings
 
-        1. **Identify static resources**: Look for URLs or file references to static assets including:
-           - Font files: .woff, .woff2, .ttf, .otf, .eot
-           - Images: .jpg, .jpeg, .png, .gif, .svg, .webp, .ico
-           - Stylesheets: .css
-           - Scripts: .js (when served as static files)
-           - Media: .mp4, .webm, .mp3, .wav
+        After ALL agents complete, consolidate their findings:
+        1. Collect all issues from each agent's response
+        2. **Deduplicate**: If multiple agents report the same issue (same file + same/adjacent line + similar description):
+           - Keep the finding with higher severity
+           - Merge unique context from duplicates
+           - Preserve the ID from the more specific agent (sec- > bug- > pat-)
+        3. Sort final list by severity: CRITICAL > HIGH > MEDIUM > LOW
 
-        2. **Check Cache-Control headers**: Examine if Cache-Control headers are explicitly set for these resources. Flag in these cases:
-           - **Explicit headers with insufficient max-age**: If Cache-Control is set with max-age < 31536000 (1 year) for static immutable resources
-           - **Missing or implicit headers**: If Cache-Control is not explicitly set in the code and may rely on server defaults
+        ### Step 3: Output Consolidated Review
 
-        3. **Report format for insufficient cache-control**:
-        <example-snippet>
-        ⚠️ **Static Resource Cache-Control Issue**
-        **Resource:** [URL or file path]
-        **File:** [file_path:line_number]
-        **Current Cache-Control:** [value or \"Not explicitly set (implicit)\"]
-        **Recommendation:** Static immutable resources should have \"Cache-Control: public, max-age=31536000, immutable\" (1 year)
-        **Rationale:** Improves performance by allowing browsers to cache static resources for extended periods. Use cache-busting techniques (versioned URLs/filenames) for updates.
-        </example-snippet>
+        Produce a SINGLE consolidated summary containing all unique issues in the standard collapsed \`<details>\` format. Do NOT include separate sections per agent - merge everything into one unified list.
+
+        ---
+
+        ## REVIEW FOCUS AREAS (Reference for Agents)
+
+        Analyze code changes for issues in these areas:
+        - **Code quality and best practices** for the technologies used in this project
+        - **Potential bugs or issues** especially in critical code paths and async operations
+        - **Performance considerations** for both frontend and backend code
+        - **Security implications** particularly for authentication, API endpoints, and data handling
+        - **Test coverage** and quality of test implementations
+        - **Documentation updates** if needed for significant changes
+        - **Type safety** and proper usage of type systems
+        - **Error handling** and edge cases
+        - **Code maintainability** and readability
 
-        4. **When to flag**:
-           - Flag: Static resource URLs being added/modified without proper cache headers
-           - Flag: Cache-Control headers set to less than 1 year for static immutable resources
-           - Flag: Static resources where caching configuration is not visible in the changed code (ask reviewer to verify server/CDN configuration)
-           - Don't flag: Dynamic/API endpoints or resources that change frequently
-           - Don't flag: Resources already configured with max-age >= 31536000
-
-        ### GitHub Actions Workflow Security (Supply Chain Attack Prevention)
-
-        Scan all changed .github/workflows/*.yml or .github/workflows/*.yaml files for dangerous patterns that could enable supply chain attacks. **ONLY report if issues are found.** **If no workflow security issues are detected, do not mention this check at all.**
-
-        Flag using these guidelines:
-
-        1. **CRITICAL: pull_request_target with PR head checkout** - Flag ANY workflow that:
-           - Uses on: pull_request_target trigger AND
-           - Checks out the PR head using patterns like:
-             - ref: github.event.pull_request.head.sha
-             - ref: github.event.pull_request.head.ref
-             - ref: refs/pull/github.event.pull_request.number/merge
-             - Any checkout that references the pull request head rather than base
-           - **Severity: CRITICAL** - This combination allows attackers to run arbitrary code by modifying repository scripts in their PR
-
-        2. **HIGH: pull_request_target with script execution** - Flag workflows that:
-           - Use on: pull_request_target AND
-           - Execute repository scripts (.js, .sh, .py, .ts, etc.) after checkout
-           - **Severity: HIGH** - Could be exploited if checkout configuration changes
-
-        3. **MEDIUM: pull_request_target usage** - Flag ANY use of pull_request_target as requiring security review:
-           - This trigger runs with repository secrets access
-           - External contributors can trigger it via PRs
-           - **Severity: MEDIUM** - Requires careful review even without obvious vulnerabilities
-
-        Report format:
-        🚨 **GitHub Actions Security Risk** (Blocking)
-        **Severity:** [CRITICAL/HIGH/MEDIUM]
-        **File:** [file_path:line_number]
-        **Pattern:** [description of the dangerous pattern found]
-        **Risk:** [specific attack vector]
-        **Recommendation:** [specific fix - e.g., use pull_request trigger instead, or avoid checking out PR head, or use workflow_run pattern for external PRs]
-
-        ### WalletConnect Pay Architecture Compliance
-
-        Scan changed files for SOA and reliability pattern violations. **ONLY report if violations found.** **If no violations detected, do not mention this check.**
-
-        1. **CRITICAL: Cross-Service Database Access**
-           - Imports of other services' DB models/DAOs/repositories
-           - SQL queries to tables outside service boundary
-           - Shared DB connections across services
-           **Report:** 🚨 **Architecture Violation: Cross-Service DB Access** (Blocking) - Services must expose data via APIs, not direct DB access
-
-        2. **HIGH: Missing Idempotency Key Validation**
-           - POST/PUT/PATCH/DELETE handlers without idempotency key extraction
-           - Payment/wallet/transaction mutations lacking idempotency store checks
-           - No duplicate request detection or payload fingerprint validation
-           **Report:** ⚠️ **Missing Idempotency Protection** - Recommend extracting idempotency key from request headers, checking against a store for duplicates, and returning cached response if already processed
-
-        3. **HIGH: External Calls Without Timeout/Retry**
-           - fetch()/axios/http.request() without explicit timeout
-           - External API calls without retry + exponential backoff
-           - Critical dependencies without circuit breaker
-           **Report:** ⚠️ **Missing Resilience Patterns** - Add timeout, retry with backoff, circuit breaker for cascading failure prevention
-
-        4. **HIGH: Non-Idempotent Event Consumers**
-           - Message queue consumers (SQS/SNS/Kafka) without message ID deduplication
-           - Event handlers mutating state without checking if already processed
-           - Webhook handlers lacking replay protection
-           **Report:** ⚠️ **Non-Idempotent Event Consumer** - Recommend checking event/message ID against processed events store before performing mutations to prevent duplicate processing
-
-        5. **MEDIUM: Missing Compensating Actions**
-           - Sequential cross-service calls without rollback logic
-           - Multi-step workflows (hold → settle → notify) without saga pattern
-           - Try-catch blocks not emitting compensating events on failure
-           **Report:** ⚠️ **Missing Saga Compensation** - Recommend adding catch blocks that release holds and emit compensating/reversal events on failure
-
-        6. **MEDIUM: State Transitions Without Trace Context**
-           - Payment/wallet state changes without structured logs + correlation IDs
-           - Cross-service calls missing trace ID propagation (X-Trace-Id header)
-           - State events not linked to originating operation
-           **Report:** ⚠️ **Missing Trace Context** - Recommend adding structured logging with traceId and correlationId for state changes, and propagating trace headers across service calls
-
-        **Scope:** Flag new code or modifications introducing anti-patterns in payment/wallet/transaction operations. Ignore read-only ops, utilities, tests."
+        **Note:** Domain-specific checks (External URLs, Cache-Control, GitHub Actions Security, WalletConnect Pay Architecture) are handled by the review-patterns agent."
 
 
           # Add project context
@@ -410,7 +348,7 @@ runs:
         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

claude/auto-review/agents/review-bugs.md

@@ -0,0 +1,97 @@
+---
+name: review-bugs
+description: Reviews code for bugs, logic errors, race conditions, and runtime issues. Use when analyzing PR changes for functional correctness.
+tools: Read, Glob, Grep
+model: sonnet
+---
+
+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.
+
+## 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
+
+## 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-{file-slug}-{semantic-slug}-{hash}
+**File:** path/to/file.ext:lineNumber
+**Severity:** CRITICAL|HIGH|MEDIUM|LOW
+**Category:** bug
+**Context:** Detailed explanation of the bug and why it's problematic.
+**Recommendation:** Specific fix with code snippet if applicable.
+```
+
+**ID Format Rules:**
+- Always prefix with `bug-`
+- file-slug: filename without extension, first 12 chars, lowercase
+- semantic-slug: 2-4 key terms from issue description
+- hash: first 4 chars of SHA256(file_path + description)
+
+If no bugs are found, state: "No bug issues found."
+
+Wrap all issues in a collapsed `<details>` block with summary showing the count.