corvus-review-r2

Pass 4 runs AFTER 1-3 because it needs to see what the other passes found to avoid duplicate findings and to calibrate its nit sensitivity.

Pass Toggling

Check PR_CONTEXT.config.passes before launching each pass:

if config.passes.architecture == false → Skip Pass 1, set status: "skipped"
if config.passes.correctness == false  → Skip Pass 2, set status: "skipped"
if config.passes.security == false     → Skip Pass 3, set status: "skipped"
if config.passes.conventions == false  → Skip Pass 4, set status: "skipped"

If ALL passes are skipped → Produce empty REVIEW_FINDINGS and proceed to R3.

Path-Specific Pass Skipping

Check PR_CONTEXT.config.path_rules for pass-level skipping:

path_rules:
  - pattern: "vendor/**"
    skip_passes: ["conventions"]

When a path rule specifies skip_passes, exclude matching files from those passes. Pass the excluded files list to the relevant pass delegation.

SHARED CONTEXT BLOCK

Every pass delegation includes this shared context block. Prepare it ONCE, reuse across all delegations:

**PR IDENTITY**:
- PR #[pr_number]: [title]
- Author: @[author]
- Branch: [head_branch] → [base_branch]
- Changes: +[additions] / -[deletions] across [files_changed] files

**CHANGED FILES**:
[List all files with language and diff size]

**CODEBASE CONVENTIONS** (from R1):
- Naming: [conventions.naming]
- File structure: [conventions.file_structure]
- Error handling: [conventions.error_handling]
- Test patterns: [conventions.test_patterns]
- Import order: [conventions.import_order]

**DEPENDENCY GRAPH** (key relationships):
[Summarize key dependency relationships from REVIEW_CONTEXT.dependency_graph]

**TEST COVERAGE**:
- Files with tests: [list]
- Files without tests: [list]

**LINKED ISSUES**:
[Summarize linked issues with acceptance criteria if available]

**CI STATUS**: [ci_status]
[If failing: summarize CI failure analysis from REVIEW_CONTEXT]

**TRIAGE FLAGS**:
[List active flags: is_large_pr, missing_description, has_ci_failures, etc.]

PASS 1: ARCHITECTURE & DESIGN

DELEGATE TO: @ux-dx-quality

Condition: config.passes.architecture == true

**TASK**: Architecture & Design review for PR #[pr_number]

**REVIEW PASS**: architecture (Pass 1 of 4)
**REVIEW SCOPE**: Broad structural view — evaluate design decisions, not line-level correctness.

[SHARED CONTEXT BLOCK]

**FILE CONTENTS AND DIFFS**:
[For each changed file, include:
  - Full file content (from REVIEW_CONTEXT.file_map[file].full_content)
  - Diff hunks (from REVIEW_CONTEXT.file_map[file].diff_hunks)
  - Callers list (from REVIEW_CONTEXT.file_map[file].callers)
]

**EXCLUDE FROM THIS PASS**:
[List any files excluded by path_rules with skip_passes including "architecture"]

**REVIEW CHECKLIST**:
1. **Abstraction quality**: Are new abstractions at the right level? Too many layers? Too few?
2. **Responsibility placement**: Is new code in the right module/file? Does it follow existing boundaries?
3. **API design**: Are new public interfaces intuitive? Consistent with existing APIs?
4. **Coupling**: Does this increase coupling between modules? Are dependencies going the right direction?
5. **Complexity**: Is the approach proportional to the problem? Over-engineered or under-engineered?
6. **Breaking changes**: Any backward-incompatible changes? Are they documented/flagged?
7. **Scalability concerns**: Will this approach work at 10x scale? Any obvious bottlenecks?
8. **Pattern consistency**: Does this follow or diverge from established codebase patterns?

**FINDING FORMAT**:
Each finding MUST use this exact structure:

```yaml
- id: "arch-NNN"
  pass: "architecture"
  label: "<blocker|critical|major|minor|nitpick|praise|thought|note>"
  severity: <0-5>
  file: "<file_path>"
  line_start: <number>
  line_end: <number|null>
  title: "<short title, max 80 chars, imperative mood>"
  body: "<markdown explanation>"
  suggestion: null
  confidence: <0.0-1.0>
  related_to: []
  suppressed: false

MUST DO:

• Review ALL changed files (not just the largest ones)
• Consider the changes holistically — how do they fit together?
• Include at least one praise finding if there's genuinely good design work
• Set confidence honestly (0.5-0.7 for "I think", 0.8-0.9 for "I'm fairly sure", 1.0 for "definitely")
• Cross-reference with linked issue acceptance criteria when available

MUST NOT DO:

• Review line-level correctness (that's Pass 2)
• Review security (that's Pass 3)
• Review naming/style conventions (that's Pass 4)
• Modify any files
• Produce findings for files in the exclude list

REPORT FORMAT:

### Pass 1: Architecture & Design — Summary

[2-3 sentence high-level assessment]

### Findings

[YAML array of all findings]

### Pass Summary
- Total findings: [N]
- By severity: [breakdown]
- Key concern: [one-sentence summary of most important finding, or "none"]

---

## PASS 2: LOGIC & CORRECTNESS

**DELEGATE TO**: @code-quality

**Condition**: `config.passes.correctness == true`

```markdown
**TASK**: Logic & Correctness review for PR #[pr_number]

**REVIEW PASS**: correctness (Pass 2 of 4)
**REVIEW SCOPE**: Line-by-line analysis — evaluate correctness, edge cases, error handling.

[SHARED CONTEXT BLOCK]

**FILE CONTENTS AND DIFFS**:
[For each changed file, include:
  - Full file content
  - Diff hunks
  - Callers list
  - Associated test files (from REVIEW_CONTEXT.file_map[file].test_files)
]

**EXCLUDE FROM THIS PASS**:
[List any files excluded by path_rules with skip_passes including "correctness"]

**REVIEW CHECKLIST**:
1. **Logic errors**: Off-by-one, wrong comparisons, incorrect boolean logic, null/undefined handling
2. **Edge cases**: Empty inputs, boundary values, concurrent access, error paths
3. **Error handling**: Are errors caught? Propagated correctly? Are error messages helpful?
4. **Type safety**: Are types correct? Any unsafe casts? Any `any` types in TypeScript?
5. **Resource management**: Are resources (connections, handles, streams) properly cleaned up?
6. **Race conditions**: Any shared mutable state? Async issues? Missing await?
7. **Test coverage**: Are new code paths tested? Are edge cases covered? Are tests meaningful?
8. **Regression risk**: Could these changes break existing functionality? Are callers updated?
9. **Data validation**: Are inputs validated? Are assumptions documented?
10. **Performance gotchas**: O(n^2) in loops, unnecessary allocations, missing pagination

**FINDING FORMAT**:
Each finding MUST use this exact structure:

```yaml
- id: "logic-NNN"
  pass: "correctness"
  label: "<blocker|critical|major|minor|nitpick|praise|thought|note>"
  severity: <0-5>
  file: "<file_path>"
  line_start: <number>
  line_end: <number|null>
  title: "<short title, max 80 chars, imperative mood>"
  body: "<markdown explanation with concrete example of failure scenario>"
  suggestion: "<suggested fix code or null>"
  confidence: <0.0-1.0>
  related_to: []
  suppressed: false

MUST DO:

• Review EVERY changed line, not just new code (modifications matter too)
• For each logic issue, describe a CONCRETE scenario where it fails
• Provide suggestion code for fixable issues (using GitHub suggestion format)
• Check that test files actually test the changed behavior (not just exist)
• Cross-reference callers: if a function signature changed, are all callers updated?
• Set confidence: 1.0 for demonstrable bugs, 0.7-0.9 for likely issues, 0.5-0.6 for suspicions

MUST NOT DO:

• Review architecture or design (that's Pass 1)
• Review security (that's Pass 3)
• Review naming/style (that's Pass 4)
• Modify any files
• Produce findings for files in the exclude list
• Flag "missing tests" as a blocker (it's a major at most)

REPORT FORMAT:

### Pass 2: Logic & Correctness — Summary

[2-3 sentence assessment of code correctness]

### Findings

[YAML array of all findings]

### Pass Summary
- Total findings: [N]
- By severity: [breakdown]
- Key concern: [one-sentence summary or "none"]

---

## PASS 3: SECURITY

**DELEGATE TO**: @security-reviewer

**Condition**: `config.passes.security == true`

```markdown
**TASK**: Security review for PR #[pr_number]

**REVIEW PASS**: security (Pass 3 of 4)
**REVIEW SCOPE**: Security-focused analysis — vulnerabilities, auth, data protection.

[SHARED CONTEXT BLOCK]

**FILE CONTENTS AND DIFFS**:
[For each changed file, include:
  - Full file content
  - Diff hunks
]

**DEPENDENCY ADVISORIES** (from R1):
[List any dependency advisories from REVIEW_CONTEXT.dependency_advisories]

**PATH SECURITY ELEVATION**:
[List any files matching path_rules with elevate_security: true]

**EXCLUDE FROM THIS PASS**:
[List any files excluded by path_rules with skip_passes including "security"]

**REVIEW CHECKLIST (OWASP-aligned)**:
1. **Injection**: SQL injection, command injection, LDAP injection, XSS (reflected/stored/DOM)
2. **Broken Authentication**: Weak password handling, session management, token validation
3. **Sensitive Data Exposure**: Secrets in code, PII logging, insecure storage, missing encryption
4. **Broken Access Control**: Missing authorization checks, IDOR, privilege escalation
5. **Security Misconfiguration**: Insecure defaults, verbose errors in production, CORS misconfiguration
6. **Insecure Deserialization**: Untrusted data deserialization, prototype pollution
7. **Known Vulnerable Components**: Dependencies with known CVEs (cross-ref with advisories)
8. **Insufficient Logging**: Missing audit trail for security-sensitive operations
9. **Input Validation**: Missing validation, regex DoS (ReDoS), path traversal
10. **Secrets Management**: Hardcoded credentials, API keys, tokens, connection strings

**SECURITY-ELEVATED PATHS**:
For files matching `elevate_security: true` path rules, LOWER the threshold for flagging:
- What would normally be `minor` becomes `major`
- What would normally be `major` becomes `critical`

**FINDING FORMAT**:
Each finding MUST use this exact structure:

```yaml
- id: "sec-NNN"
  pass: "security"
  label: "<blocker|critical|major|minor|nitpick|praise|thought|note>"
  severity: <0-5>
  file: "<file_path>"
  line_start: <number>
  line_end: <number|null>
  title: "<short title, max 80 chars, imperative mood>"
  body: "<markdown explanation with attack scenario>"
  suggestion: "<suggested fix code or null>"
  confidence: <0.0-1.0>
  related_to: []
  suppressed: false

MUST DO:

• Check EVERY changed file for security implications (even seemingly innocent changes)
• For each security finding, describe a CONCRETE attack scenario
• Include CWE reference where applicable (e.g., "CWE-79: Cross-site Scripting")
• Check for secrets/credentials in both code AND configuration files
• Cross-reference with dependency advisories from R1
• Use HIGH confidence only for demonstrable vulnerabilities
• Include praise for good security practices (input validation, proper auth checks)

MUST NOT DO:

• Review logic correctness (that's Pass 2)
• Review architecture (that's Pass 1)
• Review style (that's Pass 4)
• Modify any files
• Flag theoretical issues with confidence > 0.5 (use thought label for speculative concerns)
• Produce findings for files in the exclude list

REPORT FORMAT:

### Pass 3: Security — Summary

[2-3 sentence security assessment]

### Findings

[YAML array of all findings]

### Pass Summary
- Total findings: [N]
- By severity: [breakdown]
- Key concern: [one-sentence summary or "No security issues found"]

---

## PASS 4: CONVENTIONS & POLISH (Corvus-Review Direct)

**Executor**: Corvus-Review direct (no subagent delegation).

**Condition**: `config.passes.conventions == true`

**Timing**: AFTER Passes 1-3 complete.

<critical_rule priority="9999">
  Pass 4 is AGGRESSIVELY FILTERED. Maximum output: 3 nit findings.
  
  Purpose: catch only the most impactful style/convention violations.
  This pass should NOT produce blockers or criticals — if something is
  that severe, it belongs in Pass 1 (architecture) or Pass 2 (correctness).
  
  Maximum severity for Pass 4 findings: minor (severity 2).
  If you think it's severity 3+, it belongs in another pass.
</critical_rule>

### Pass 4 Review Scope

Using REVIEW_CONTEXT.conventions as the baseline, check the changed code for:

1. **Naming consistency**: Do new names follow the detected conventions?
2. **Import ordering**: Do new imports follow the detected pattern?
3. **Documentation**: Are new public APIs/functions documented? (Only flag if existing code IS documented)
4. **Code style**: Consistent formatting, no mixed styles within a file
5. **Dead code**: Commented-out code, unused imports, unreachable branches
6. **Custom rules**: Apply `PR_CONTEXT.config.custom_rules` regex patterns

### Custom Rules Execution

For each custom rule in config:

```yaml
custom_rules:
  - id: "todo-no-issue"
    pattern: "TODO(?!.*#\\d+)"
    severity: "minor"
    message: "TODO comment without linked issue"
    include: ["*.ts", "*.js"]
    exclude: ["*.test.*"]

1. Filter changed files by include/exclude patterns
2. Search each matching file for the pattern regex
3. If found, produce a finding with the specified severity and message
4. Custom rule findings count toward the nit budget

Pass 4 Finding Format

- id: "conv-NNN"
  pass: "conventions"
  label: "<minor|nitpick|praise|thought|note>"  # NO blocker/critical/major allowed
  severity: <0-2>                        # Maximum severity: 2 (minor)
  file: "<file_path>"
  line_start: <number>
  line_end: <number|null>
  title: "<short title>"
  body: "<brief explanation>"
  suggestion: "<suggested fix or null>"
  confidence: <0.0-1.0>
  related_to: []
  suppressed: false

Pass 4 Deduplication Against Passes 1-3

Before finalizing Pass 4 findings:

1. Check each Pass 4 finding against all Pass 1-3 findings
2. If a Pass 4 finding overlaps with a finding from Passes 1-3 (same file, overlapping lines, similar concern), DROP the Pass 4 finding
3. Log dropped findings with reason "duplicate of [other_finding_id]"

Pass 4 Budget Enforcement

After deduplication:

1. Count remaining findings (excluding praise and note)
2. If count > config.max_nits (default: 3):
   • Sort by confidence (ascending)
   • Drop lowest-confidence findings until count == max_nits
   • Log dropped findings with reason "nit_budget_exceeded"

ASSEMBLE REVIEW_FINDINGS

After all passes complete (or are skipped), assemble the REVIEW_FINDINGS object:

Assembly Steps

1. Collect findings from all completed passes
2. Apply path-rule suppressions: For each finding, check if its file matches a path_rules entry with suppress_below. If finding severity is below the threshold, set suppressed: true
3. Apply suppression rules: Check each finding against config.suppressions (by ID and message pattern). If matched, set suppressed: true
4. Count totals: Aggregate counts by label (excluding suppressed findings)
5. Set pass statuses: "completed", "skipped", or "error" for each pass

Error Handling for Individual Passes

If a pass subagent fails:

1. Set that pass's status to "error"
2. Set its findings to []
3. Add an error summary: "Pass [N] ([name]) failed: [error description]"
4. PROCEED with remaining passes — do NOT abort the review
5. R3 will note the failed pass in the review summary

GATE ENFORCEMENT

[R2 COMPLETE] Passes: [N] completed, [N] skipped, [N] errored
Findings: [blocker]B [critical]C [major]M [minor]m [nit]n [praise]p
→ Proceeding to R3 (Comment Synthesis)