Requesting Code Review

Request review early to catch issues before they spread.

When

• After completing a plan task or batch
• After major refactor/feature work
• Before merge or PR finalization

How

1. Determine review range (BASE_SHA -> HEAD_SHA). Getting this wrong silently reviews the wrong thing:
   • Whole branch: BASE_SHA=$(git merge-base origin/main HEAD) (substitute your default branch). Not a bare origin/main — once main moves past your branch point, main's newer files appear in the diff as phantom deletions your branch never made, and the reviewer spends its pass on them.
   • A single task or batch: the commit recorded before the work started. Not HEAD~1, which silently drops all but the last commit of a multi-commit task.
   • No remote, or origin/main not fetched: fall back to git merge-base main HEAD.
   • Confirm the range is non-empty before dispatching — git diff --stat BASE..HEAD. An empty range produces a confident "no issues found" review of nothing.

2. The review scope is always git diff --name-only BASE_SHA..HEAD_SHA. Never take it from context-snapshot.json: its changed_files are the commits since the last session start (or the last commit), not your review range.

   • The snapshot may add callers. If it exists and its git_hash equals git rev-parse HEAD, then for each file in the review range that has a blast_radius entry, add those callers to the reviewer prompt: "Changed files: [range list]. Also referenced by: [blast_radius callers]." If the hashes differ, ignore blast_radius.

   What blast_radius means. Each entry lists files containing a reference that resolves to the changed file's path — a relative import, or the repo path written out. It is deliberately incomplete: references that cannot be resolved (package-style imports, dynamic paths, aliases from tsconfig) are dropped rather than guessed, so an empty list means "nothing was proven," not "nothing depends on this." Never widen review scope on a name match alone; if you need dependents the snapshot does not list, grep for them and say you did.

   blast_radius_method records how the edges were derived. If it is missing, the snapshot came from a version that matched basenames as words — ignore blast_radius entirely and scope from the diff.

3. If a plan is in play, write the diff to a file first: bash ../subagent-driven-development/scripts/review-package PLAN_FILE BASE HEAD. It refuses empty and non-descendant ranges outright, so the reviewer never receives a range that cannot mean what it claims. Pass the printed path to the reviewer.
4. Dispatch superpowers-optimized:code-reviewer using requesting-code-review/code-reviewer.md.
5. Provide:
   • What changed (from context snapshot or git diff)
   • Scoped file list (changed files + blast radius callers if fresh snapshot available, or broad if not)
   • Requirement or plan reference
   • SHA range
   • Short summary

Security Review (Built-In)

When changes touch security-relevant areas, the code review must include a security pass. This is not a separate step — it's part of every review where applicable.

Triggers automatically when changes touch:

• Authentication or authorization flows
• Input validation or output encoding
• API endpoints handling user data
• Secrets management or credential handling
• Cryptography, key management, or token generation
• Infrastructure, deployment, or CI/CD configs

Security checklist:

• OWASP Top 10 and CWE vulnerability scan
• OWASP API Security Top 10: broken object/function-level authorization, unrestricted resource consumption, SSRF, mass assignment, improper inventory management
• Input validation and injection risk (SQL, XSS, CSRF, command injection)
• Auth flow correctness (session handling, token expiry, privilege escalation, rate limiting on auth endpoints)
• Secrets handling (no hardcoded credentials, proper env var usage)
• Dependency vulnerabilities (known CVEs in imported packages)
• API hardening (security headers, CORS configuration, error message sanitization, rate limiting)
• Logging hygiene (no secrets in logs, adequate audit trail)

Severity enforcement:

• Critical/High security findings block merge until addressed or the user explicitly accepts the risk with documented rationale.
• Medium security findings should be fixed before merge unless explicitly deferred.

Adversarial Red Team (Optional)

For changes involving complex logic, concurrency, state management, or critical data paths, dispatch superpowers-optimized:red-team in parallel with the code reviewer.

Triggers when changes touch:

• State machines or multi-step workflows
• Concurrent access to shared resources
• Complex business logic with branching conditions
• Data transformation pipelines
• Retry/recovery/rollback logic
• Performance-critical paths handling large inputs

The red team agent finds concrete failure scenarios (specific inputs, race conditions, state corruption, resource exhaustion) that checklist-based review misses. It does NOT duplicate the security review — its focus is adversarial logic analysis, not OWASP/CWE compliance.

Red team critical findings block merge alongside security critical findings.

Auto-Fix Pipeline

When the red team report contains Critical or High findings, run the auto-fix pipeline. The pipeline is ASI-guided and iterative — fix one finding at a time, starting from the red team's designated ASI, then re-assess before proceeding. This prevents fixes from conflicting with each other when findings touch shared code.

Iteration loop:

1. Identify the entry point. Start with the finding marked ASI in the red team summary. If no ASI is marked, start with the highest-severity finding.
2. Write the failing test. Flesh out the test skeleton from the red team report into a real test and run it. It MUST fail — this proves the scenario is real. If the test passes, the finding was a false positive; skip it and note it in the triage, then re-identify the next ASI.
3. Fix the code. Make the minimum change to pass the test. Do not refactor or improve surrounding code.
4. Run a targeted re-check. Re-read only the files touched by the fix and check whether: (a) the fix introduced any new issues, and (b) any previously reported findings are now resolved as a side effect.
5. Re-assess the remaining findings. Update your list — remove resolved findings, re-prioritize if the fix changed the risk landscape. Identify the new ASI.
6. Repeat from step 2 until no Critical or High findings remain.

After the loop completes:

• Run one final regression check covering everything the fixes touched, scoped per verification-before-completion. Fixes confined to one module: that module's suite plus its dependents. Fixes spanning modules or touching a shared contract: the full suite.
• Report: findings fixed, false positives skipped, any regressions introduced and resolved.

Skip conditions:

• If the red team report has zero Critical/High findings, skip the pipeline entirely.
• Medium findings are tracked for later, not auto-fixed.
• If the user explicitly says to skip auto-fix, respect that.

Triage Rules

• Fix all Critical issues before proceeding.
• Fix Important issues unless user explicitly defers.
• Track Minor issues for later.
• Push back with evidence when feedback is incorrect.

Output Requirement

Review must include severity, file references, security findings (if applicable), and merge readiness verdict.