review-code · diff
v1.1.0 to v2.0.0
95 added, 118 removed. Audit A to A.
---
name: review-code
- description: Runs a comprehensive multi-agent code review of a PR, commit, or the
- whole codebase across six dimensions (correctness, performance, code style, test
- coverage, error handling, and simplicity/over-engineering) and returns a severity-ranked
- report with file:line findings and fix suggestions. Use when the user wants a thorough
- code review, asks to review a PR or diff, or wants over-engineered code flagged
- for simplification. Analysis only, identifying issues without modifying code, committing,
- or running tests. Not for a security-focused audit (use review-security), a visual/UX
- design critique (use review-design), or a deep performance-only investigation with
- profiling and query-level analysis (use review-perf).
+ description: >-
+ Runs one adaptive multi-agent code review of a PR, commit, or the whole codebase.
+ Always covers six core dimensions (correctness, performance, code style, test coverage,
+ error handling, simplicity/over-engineering) plus architecture and design patterns, then
+ adds security, dependency, deep-performance, and UI/motion lanes when the change touches
+ those areas. Critical and Major findings are adversarially verified before the severity-ranked
+ report with file:line findings and fixes goes out in chat. Use when the user wants a thorough
+ review, asks to review a PR or diff, or wants over-engineered code flagged. Analysis only:
+ never edits code, commits, or applies fixes, and posts to GitHub only when explicitly asked.
+ Not for a standalone security audit (use review-security), a standalone dependency audit
+ (use review-deps), a standalone performance investigation (use review-perf), or a standalone
+ UX/design critique (use review-design).
metadata:
- summary: "Multi-agent code review across six dimensions, from correctness and performance to tests and error handling"
+ summary: "Adaptive multi-agent code review: core dimensions plus architecture, security, deps, perf and design lanes picked from the diff, with adversarial verification"
author: mgiovani
- version: 1.1.0
+ version: 2.0.0
---
# Code Review
- Multi-agent code review spanning six dimensions: correctness, performance, code style, test coverage gaps, error handling, and simplicity/over-engineering. This skill performs analysis only. It surfaces issues, explains why they matter, and suggests fixes, but never touches the code itself.
+ One review, adaptive lanes. The six core dimensions and the architecture lane always run. Security, deps, deep-performance and design/motion lanes join when the diff touches their area. Critical and Major findings are then attacked by a verifier before they reach the report.
- Every finding must cite a `file:line` you actually read: no hypothetical issues, no estimated counts. Only review files within the determined scope, and only flag style deviations from the project's own conventions, not personal preference.
+ Analysis only. Never edit code, commit, apply fixes, or run anything that mutates the repo. The report goes to chat. Run `gh` write commands (reviews, comments) only when the user explicitly asks to post.
- ## Review Workflow
+ ## Core rules
- ### Phase 0: Determine Review Scope
+ - Every finding cites a `file:line` you read, pointing at the offending statement itself, not the block around it. No hypothetical issues, no estimated counts.
+ - Review the diff, not the uncommitted working tree. Findings on changed lines are in-diff; findings on untouched lines of touched files are `preexisting`.
+ - Project rules (AGENTS.md, CLAUDE.md, `.claude/skills/*`) are the style standard. A violation is a finding. Personal preference is not.
+ - User context such as "another session is fixing X" is information, not an exclusion, unless the user says to exclude it.
+ - Strict by default: report every smell and rule violation, Nit included. Nothing is silently passed.
+ - Every subagent call sets `model` explicitly: review, research and verify use `sonnet`, planning uses `opus`. Never leave it inherited, never use `haiku`.
- Parse arguments to determine what to review:
+ ## Workflow
+ Copy this checklist and tick each step. Do not start a step before the previous gate holds.
+
```
- Arguments:
- - <pr_number>: Review only files changed in PR (e.g., "123", "#123")
- - <commit_sha>: Review only files changed in commit (e.g., "abc123")
- - "--all" or no args: Review entire codebase
- - "--focus [correctness|performance|style|tests|errors]": Focus on specific review dimension
+ - [ ] 0. Scope: diff retrieved, file list and hunk ranges known (gate: non-empty diff)
+ - [ ] 1. Discovery: stack and project rules extracted into a numbered list (gate: list written)
+ - [ ] 2. Lane selection: lanes chosen with a one-line reason each (gate: printed)
+ - [ ] 3. Fan-out: all lane agents launched in ONE parallel batch (gate: all findings files written)
+ - [ ] 4. Consolidate: merged, deduplicated, routed to the right dimension
+ - [ ] 5. Verify: Critical/Major findings attacked, REJECTED dropped (gate: every survivor has a verdict)
+ - [ ] 6. Report: written in chat from the template
+ - [ ] 7. Post: ONLY if the user explicitly asked
```
- If PR or commit specified, use Bash to get changed files and diff context:
+ ### 0. Scope
- ```bash
- # For PR - get files and full diff
- gh pr view <pr_number> --json files --jq '.files[].path'
- gh pr diff <pr_number>
+ Parse arguments: a PR number (`123`, `#123`), a commit SHA, `--all` or none (whole codebase), and `--focus <value>`.
- # For commit
- git diff-tree --no-commit-id --name-only -r <commit_sha>
- git show <commit_sha>
+ ```bash
+ gh pr diff <pr> # PR
+ git fetch origin <base> && git diff origin/<base>...HEAD # fallback when gh fails (e.g. diff too large)
+ git show <sha> # commit
```
- When reviewing a PR or commit, always retrieve the full diff: the diff context is essential for understanding what changed versus what was already there. Agents should focus findings on the changed lines while using surrounding code for context.
-
- ### Phase 1: Project Discovery
-
- Explore the codebase to understand its technology stack and conventions, plus whatever quality standards it already holds itself to:
-
- ### Phase 2: Initialize Progress Tracking (optional)
+ Create a scratch dir with `mktemp -d` (never inside the repo) and save the diff to `<scratch>/pr.diff`. Take the changed-file list from the same diff. From each hunk header `@@ -a,b +c,d @@`, the new-side range is lines `c` to `c+d-1`; anything outside those ranges is pre-existing. Re-reviewing after fixes is just a rerun: the diff is re-derived.
- If TodoWrite is available, use it to track review progress across the specialist dimensions and report generation. Skip it for a small scoped review or in an environment without it (it's a convenience, not a requirement).
+ ### 1. Discovery
- ### Phase 3: Parallel Specialist Review
+ Read the root and nested AGENTS.md / CLAUDE.md and `.claude/skills/*`. Turn every rule into a numbered checklist (comment policy, strict typing, file-size limits, empty `__init__`, no manual memo under React Compiler, and so on). This list goes into every agent prompt. Note the languages, frameworks and test setup.
- Spawn 6 parallel Explore agents for comprehensive code review (5 below, plus Agent 6 from the simplicity/over-engineering module later in this file). Each agent specializes in a specific review dimension. For detailed agent prompts and patterns, see [references/agent-prompts.md](references/agent-prompts.md).
+ ### 2. Lane selection
- Agent assignments:
+ | Lane | Findings file | Runs when the change touches | Prefix |
+ |------|---------------|------------------------------|--------|
+ | core (agents 1-6) | `core-<prefix>.json` per agent (`core-cl`, `core-pf`, `core-cs`, `core-tc`, `core-eh`, `core-oe`) | always | CL PF CS TC EH OE |
+ | architecture (agent 7) | `architecture.json` | always | AP |
+ | security-fe | `security-fe.json` | UI, client auth, storage, cookies, CSP | SEC |
+ | security-be | `security-be.json` | server code, auth, config, IaC, CI | SEC |
+ | deps | `deps.json` | a manifest or lockfile | DEP |
+ | perf-deep | `perf-deep.json` | DB, queries, hot loops, bundle or render paths | PF |
+ | design-motion | `design-motion.json` | UI, CSS, component files | DM |
- | Agent | Dimension | Focus |
- |-------|-----------|-------|
- | 1 | Correctness & Logic | bugs, race conditions, off-by-one errors, null safety, type mismatches |
- | 2 | Performance | algorithmic complexity, unnecessary allocations, N+1 queries, missing caching, memory leaks |
- | 3 | Code Style & Patterns | naming, structure, DRY violations, SOLID adherence, framework idioms |
- | 4 | Test Coverage Gaps | untested code paths, missing edge case tests, weak assertions, test quality |
- | 5 | Error Handling & Edge Cases | unhandled exceptions, missing validation, boundary conditions, graceful degradation |
+ With `--all`, run every lane that applies to the repo. With `--focus`, run only the matching lane. Print the chosen lanes with a one-line reason each, and a reason for each skipped lane. Lane prompts, sibling composition and installed-skill detection: [references/lanes.md](references/lanes.md). Architecture lens: [references/architecture-patterns.md](references/architecture-patterns.md).
- No Task/Explore tool available: run the same six specialist prompts (Agents 1-6, full text in [references/agent-prompts.md](references/agent-prompts.md)) as sequential Grep+Read passes instead of parallel subagents. Take one dimension at a time, in the same order, each following the same steps below, then merge all six dimensions' findings into one list before Phase 4.
+ ### 3. Fan-out
- Each agent must:
- 1. Grep for issue patterns across files in scope
- 2. Read each match to verify context and confirm it is a genuine issue
- 3. Extract exact code snippets (5-10 lines) with file:line references
- 4. Explain why the code is problematic
- 5. Classify severity (Critical/Major/Minor/Nit)
- 6. Provide a concrete fix suggestion with code example
+ Launch every selected lane in one parallel batch, explicit `model` on each call. Each agent gets the rule checklist, the diff scope, and the finding contract, and writes `<scratch>/findings/<lane>.json` as `{"lane": "...", "findings": [...]}`. Core agent prompts, grep patterns and the finding shape: [references/agent-prompts.md](references/agent-prompts.md).
- Severity definitions:
+ Each agent: grep for patterns in scope, read each match to confirm it is real, quote 5-10 lines with `file:line`, explain why it matters, classify severity, give a concrete fix.
| Severity | Meaning |
|----------|---------|
- | Critical | Bugs that cause data loss, crashes, security holes, or incorrect business logic |
- | Major | Significant issues that hurt reliability or performance, or create a maintainability risk |
- | Minor | Improvements to readability and consistency, plus minor inefficiencies worth flagging |
- | Nit | Style preferences, cosmetic suggestions, optional improvements |
-
- ### Phase 4: Consolidate & Analyze Findings
+ | Critical | Data loss, crashes, security holes, incorrect business logic |
+ | Major | Significant reliability or performance harm, real maintainability risk |
+ | Minor | Readability, consistency, small inefficiencies |
+ | Nit | Cosmetic or optional |
- After all agents complete:
+ Gate: a lane whose findings file is missing or invalid is re-run once. If it fails again, the report names the lane as failed instead of silently omitting it.
- 1. Collect all findings from the 6 parallel agents.
- 2. Deduplicate: remove duplicate findings across agents (e.g., the same function flagged by both correctness and error handling agents).
- 3. Prioritize by severity, using the same scale as above: Critical (data corruption, crashes, security implications, broken business logic), Major (performance bottlenecks, reliability issues, test gaps for critical paths), Minor (code readability, minor inefficiencies, style inconsistencies), Nit (naming preferences, optional simplifications, cosmetic changes).
- 4. Categorize by dimension: group findings under the 6 specialist categories.
- 5. Cross-reference findings that span multiple dimensions (e.g., a missing null check is both a correctness and error handling issue).
- 6. Compute statistics: total findings by severity, by dimension, files reviewed vs. files with issues.
+ Sequential fallback: with no subagent tool, run the same lanes as sequential Grep+Read passes in table order, the verification pass last. Same prompts, same findings files, same gates.
- ### Phase 5: Generate Review Report
+ ### 4. Consolidate
- Generate a comprehensive markdown report following the template in [references/report-template.md](references/report-template.md).
+ Run `uv run <skill-dir>/scripts/merge_findings.py --findings <scratch>/findings --diff <scratch>/pr.diff --out <scratch>` to dedupe (same path, lines within 2, same dimension or same title). If it is unavailable, merge by hand with the same rules: one primary finding, the other agents' versions in a collapsed `<details>` block. Move praise and "resolved" notes out of findings into Positive Observations. Route a finding to its true dimension: an over-engineered function that is also buggy is a `CL-` finding, not `OE-`. If Agent 6 and Agent 7 disagree on the same code, keep the OE removal and drop the pattern suggestion.
- Report sections:
- 1. Executive summary with overall code quality assessment
- 2. Severity breakdown with counts
- 3. Findings organized by dimension: each one carries a file:line, a code snippet, an explanation, and a fix suggestion
- 4. Prioritized action items (Critical first, then Major)
- 5. Positive observations - highlight well-written code, good patterns, thorough tests
+ ### 5. Verify
- To re-review after fixes, just run the skill again on the same PR/commit: Phase 0's scoping naturally re-derives the current diff, so it re-scopes to what's actually still there without a separate workflow.
+ Launch one adversarial verifier (`sonnet`) per batch of Critical/Major findings. It tries to disprove each, may run targeted read-only checks, and returns CONFIRMED / PLAUSIBLE / REJECTED with a corrected severity and a reason. Write each verdict, corrected severity and a new `reason` field back into the matching findings in `<scratch>/findings/*.json`, then re-run the merge script; the report and step 7 use that output (it drops REJECTED and renders `Verification: <VERDICT> (<reason>)`). A bug the user says they observed keeps its severity. Prompt, schema and rules: [references/verification.md](references/verification.md).
- ## Usage
+ ### 6. Report
- ```bash
- # Review a specific PR
- review-code 123
- review-code #456
+ Write the report in chat following [references/report-template.md](references/report-template.md): lane-selection line, executive summary, severity and verdict statistics, findings by dimension, pre-existing bucket, prioritized actions, positive observations. Zero surviving findings still produce the report (lanes line, statistics, positive observations); with posting requested, post a body-only review.
- # Review a specific commit
- review-code abc123def
+ ### 7. Post (explicit request only)
- # Review entire codebase
- review-code --all
- review-code
+ Only when the user asks ("post it on the PR", `--comment`). Run the merge script, show a preview with the count of inline comments versus body items, then post ONE review. Flow, anchoring rules and comment style: [references/github-review.md](references/github-review.md). Without an explicit request, run no `gh` write command.
- # Focus on a specific dimension
- review-code 123 --focus performance
- review-code --all --focus tests
+ ## Usage
- # Re-review after fixes — just run it again on the same PR/commit
- review-code 123
+ ```bash
+ review-code 123 # PR, lanes chosen from the diff
+ review-code abc123def # commit
+ review-code --all # whole codebase, every applicable lane
+ review-code 123 --focus security # narrow to one lane
+ review-code 123 --comment # also post the review to the PR
```
- ## Focus Options
-
- - `correctness`: Focus on bugs, logic errors, type safety, race conditions
- - `performance`: Focus on algorithmic complexity, resource usage, caching, queries
- - `style`: Focus on naming, structure, patterns, framework idioms, DRY/SOLID
- - `tests`: Focus on test coverage gaps, assertion quality, edge case testing
- - `errors`: Focus on error handling, validation, boundary conditions, graceful degradation
-
- If no focus specified, perform comprehensive review across all dimensions.
-
- ## Additional Resources
-
- - [references/agent-prompts.md](references/agent-prompts.md) - Detailed grep patterns and agent prompts for each review dimension
- - [references/report-template.md](references/report-template.md) - Full markdown report template with all sections
-
- ## Limitations
+ ## Focus options
- - Static, pattern-based analysis: cannot measure actual runtime/performance impact or detect runtime-only issues; some findings may turn out to be intentional design choices
- - Language support: grep patterns in [references/agent-prompts.md](references/agent-prompts.md) are written for C-like and Python syntax, so adapt them for other languages before relying on pattern coverage
- - Does not modify code or run tests/benchmarks, and skips security-specific analysis entirely (use review-security for that)
+ `correctness`, `performance`, `style`, `tests` and `errors` run the matching core agent, while `architecture`, `deps` and `design` (design-motion) run their own lane. `security` runs security-fe and security-be as applicable, and without `--focus` the lanes stay adaptive.
- ## Simplicity & Over-engineering lens (Claude Code enhancement)
+ ## Simplicity lens (Agent 6)
- LLM-written code tends to over-engineer: interfaces built for one implementation, factories for one product, wrapper layers that just forward a call. None of that shows up as a bug, so the five specialists in Phase 3 don't catch it: it needs its own lens. This module adds a 6th parallel specialist and a matching report dimension.
+ LLM-written code over-engineers: interfaces with one implementation, factories for one product, forwarding wrappers. None is a bug, so the other agents miss it. Agent 6 flags it as `OE-` (Minor/Nit by default). Prompt and tags: [references/agent-prompts.md](references/agent-prompts.md#agent-6---simplicity--over-engineering).
- ### Agent 6: Simplicity & Over-engineering
+ ## Reference files
- Spawn this agent alongside Agents 1-5 in Phase 3, in the same parallel batch. Full prompt and tag definitions: [references/agent-prompts.md](references/agent-prompts.md#agent-6---simplicity--over-engineering).
+ - [references/agent-prompts.md](references/agent-prompts.md): core agents 1-6, grep patterns, finding shape
+ - [references/architecture-patterns.md](references/architecture-patterns.md): agent 7, pain-to-pattern table, over-engineering counter-check
+ - [references/lanes.md](references/lanes.md): conditional lanes and the sibling skills they compose
+ - [references/verification.md](references/verification.md): verifier prompt, verdicts, drop rules
+ - [references/report-template.md](references/report-template.md): report layout
+ - [references/github-review.md](references/github-review.md): opt-in posting flow and comment style
+ - `scripts/merge_findings.py`: dedupe, drop REJECTED, validate RIGHT-side anchors, emit `merged.json` and `review-payload.json` (`--self-test` available)
- Routing out of scope: when Agent 6 flags something that Phase 4 consolidation determines is actually a correctness, security, or performance issue, move it into the matching dimension (`CL-`, `PF-`, or `EH-` prefix) instead of reporting it as an OE finding. An over-engineered function that also happens to be buggy is a bug first.
+ ## Limitations
- Add its dimension (`OE-` prefix, Minor/Nit by default) to Phase 4/5 output: full report-addendum spec in [references/agent-prompts.md](references/agent-prompts.md#agent-6---simplicity--over-engineering).
+ - Static, pattern-based analysis: it cannot measure runtime impact, and some findings may be intentional design.
+ - Grep patterns lean toward C-like and Python syntax; adapt them for other languages.
+ - Verification may run read-only checks but never the full test suite or benchmarks.
+ - Does not modify code. For a deep standalone security, dependency, performance or design audit, use the matching sibling skill.