# Code Review

> Staged code review — runs tests, extracts intent, dispatches 3 specialist agents (state-enumeration bug detection, project rule compliance, history + regression detection), then verifies findings with type-stratified confidence thresholds. Works on git diff (pre-commit) or PR diff. Use when user says 'code review', 'review this code', 'review changed code', 'review for regressions', 'check for bugs', 'is this good code', 'review what I just wrote'. Do NOT use for plan review (use /plan-review). Do NOT use for security/dead code scanning (use /code-security). Do NOT use for edge case auditing (use /code-edges). Do NOT use for pre-refactor impact analysis (use /code-blast-radius). Do NOT use for style cleanup or simplification (use /code-review).

- Skill: `gridlock-nyc/code-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add gridlock-nyc/code-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/gridlock-nyc/code-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Security
- Author: GRIDLOCK-NYC (https://skillmd.com/u/gridlock-nyc)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/gridlock-nyc/code-review

---


# Code Review

Staged pipeline: test → scope → intent → rules → analysis → verification → report.
Finds bugs through state enumeration, not pattern matching. Catches regressions through
git history, not guessing. Kills false positives through intent-anchored verification.

**Scope argument:** "$ARGUMENTS"

## Important

- False positives are the enemy. Every finding must survive a verification pass with
  type-stratified confidence thresholds. Findings below threshold are discarded.
- This skill finds BUGS, not style issues.
- Read CLAUDE.md, REVIEW.md, and all @-imported rules files BEFORE launching review agents.
- Never invent data. If you need to reference a function's output or API response, read the
  actual source — do not estimate or fabricate.
- This skill reviews code, not plans. For plan review use `/plan-review`.

## Instructions

### Step 0: Test Baseline

Auto-detect and run the project's test suite and linter/analyzer before reviewing any code.
This gives agents factual ground truth about whether the code under review is broken.

**Detection order** (stop at first match):
1. `pubspec.yaml` exists → run `flutter analyze` then `flutter test`
2. `package.json` exists with a `scripts.test` key that is NOT the default npm placeholder
   (`echo "Error: no test specified" && exit 1`) → run `npm test` (or `yarn test` if
   `yarn.lock` present, or `bun test` if `bun.lockb` present)
3. `Cargo.toml` exists → run `cargo clippy` then `cargo test`
4. `go.mod` exists → run `go vet ./...` then `go test ./...`
5. `pyproject.toml` or `setup.py` exists → run `pytest`
6. Check REVIEW.md for a `test_command:` directive → run that command
7. No runner found → skip with note "No test runner detected — skipping baseline"

**Capture output and exit codes.**

- If tests FAIL: record the failure output. This becomes mandatory CRITICAL context injected
  into every agent in Step 4. The test failure itself is a finding — include it in the report.
- If tests PASS: record `Tests: PASS` for the report header.
- If linter/analyzer has errors (not warnings): record as context for agents.

Do NOT abort the review if tests fail — the diff may contain the fix.

### Step 1: Determine Review Scope

**If `$ARGUMENTS` is a PR number or "PR":**
- Run `gh pr diff` to get the changes
- Run `gh pr view` to get the PR description and title
- Extract the PR description as raw text for Step 2

**If `$ARGUMENTS` is a file path or glob:**
- Use that as the scope
- Extract the most recent commit message touching those files via `git log -1 -- <file>`

**If `$ARGUMENTS` is empty:**
- Run `git diff` for unstaged changes
- If empty, run `git diff --cached` for staged changes
- If empty, run `git diff HEAD~1` for the most recent commit
- If all empty, ask the user what to review
- Extract the commit message via `git log -1 --format=%B`

State what you found: "Reviewing N changed files: [list]."

### Step 2: Intent Extraction

Launch a **single agent** (not parallel) to determine what the change is trying to accomplish.

**Input:** the diff from Step 1 + the commit message or PR description.

**Agent prompt:**
> Analyze this diff and its commit message / PR description. Answer:
>
> 1. **Intent**: What is this change trying to accomplish? State the goal in one paragraph.
> 2. **Scope**: List the files being modified and their roles.
> 3. **Constraints**: List any constraints mentioned in the commit message, PR description,
>    or comments in the diff (e.g., "must not change public API", "backwards compatible").
> 4. **Confidence**: Rate your confidence in understanding the intent:
>    - `clear` — the commit message explains the goal and the diff matches
>    - `ambiguous` — the diff does multiple things or the message is vague
>    - `missing` — no commit message, or the message doesn't match the diff
>
> Return a structured response with these four fields.

**Store the intent artifact.** It is passed to every subsequent agent and to the verification
step. If confidence is `missing`, all downstream findings receive a -15 confidence penalty
(they cannot be as confidently judged without knowing the author's goal).

### Step 3: Load Project Rules

Read CLAUDE.md in the project root. If it contains @-imports (lines like `@~/.claude/rules/foo.md`),
read each imported file. Collect all explicit rules into a rules set.

Read REVIEW.md in the project root if it exists. REVIEW.md contains review-specific rules and
causal bug patterns ("when you see X, check for Y because Z"). These are injected as
highest-priority context to all agents — they take precedence over default behavior.

Identify which CLAUDE.md files exist in directories whose files were changed — these are also
relevant and should be loaded.

This step is mandatory. Skipping this produces opinions, not compliance checks.

### Step 4: Domain Analysis

Launch **3 agents in parallel**. Each agent receives:
- The diff from Step 1
- The test baseline results from Step 0
- The intent artifact from Step 2
- The project rules from Step 3

Each agent returns a list of findings. Each finding must include: file path, line number(s),
description, category, and the evidence (specific code, state combination, or historical
pattern that supports the finding).

---

**Agent A — State Enumeration Bug Detection**

You detect bugs by systematically tracking every possible value of every variable at every
program point. You do not pattern-match against bug categories. You trace state.

For each changed function or method in the diff, execute the following phases IN ORDER.
Show your work at each phase. Do not skip to conclusions.

**Phase 1 — State Inventory**

Build a state table for the function:

| Variable/Field | Type | Possible Values | Read/Written | Lifecycle |
|---------------|------|----------------|--------------|-----------|
| ... | ... | ... | ... | ... |

"Possible Values" means the ACTUAL set, not just the type:
- `bool`: {true, false}
- `String?` / `string | null`: {null, empty, non-empty}
- `List<T>` / `T[]`: {null, empty, single-element, many}
- Controller/resource: {uninitialized, initialized, disposed/closed}
- Promise/Future state: {pending, resolved, rejected}

"Lifecycle": when created, when it can become invalid, who owns disposal.

Include: all locals, all fields/properties touched, all parameters, any global/singleton
state, any async state (Futures, Streams, Promises, pending callbacks), external state
(DB, network, file system).

**Phase 2 — Transition Tracing**

For each execution path through the function:

1. ENTRY STATE: what combinations of variable values can exist at entry?
2. Walk each branch. At each decision point, fork:
   - Condition-true path: what does the state table look like now?
   - Condition-false path: what does the state table look like now?
3. At each WRITE: is the new value consistent with what other variables expect?
4. At each AWAIT / ASYNC BOUNDARY: any state read before the await may have changed
   after it. Re-examine the state table post-await.
5. At EXIT: does the final state satisfy the caller's expectations?

For each path, check:
- Can this path be reached? (dead code)
- Is state ever partially updated? (field A changed, dependent field B not yet updated)
- Is there a window where invariants don't hold? (between two writes)

**Phase 3 — Invariant Checking**

State invariants that must hold for this function:

ENTRY (preconditions):
- Is the object/widget still alive (mounted, not disposed)?
- Are required fields non-null / initialized?
- Are resources in the expected state?

MAINTAINED (during execution):
- If state A and state B must be consistent, is there a window where they aren't?
- If a resource is held, is it held across the entire critical section?
- Across async boundaries, is re-validation performed?

EXIT (postconditions):
- Are all acquired resources released on every path (including error paths)?
- Is the return value in the expected range/type on every path?
- Are listeners/subscriptions properly managed?

CROSS-FUNCTION:
- If function A must be called before function B, is that enforced?
- Does this function's output match its callers' expectations?

For each invariant: does every traced path maintain it?

**Output rules:**
- Do NOT report style issues, naming preferences, or simplification opportunities.
- Only report bugs traceable to a specific state violation through a specific path.
- Each finding must reference: the state variable(s), the path, and the invariant violated.
- If Phases 1-3 found no violations, report "CLEAN — no state-based bugs found."

---

**Agent B — Project Rules + Compliance**

- Audit the changes against every rule in the loaded CLAUDE.md rules set
- For each violation, quote the specific rule and its source file
- Focus on rules that constrain the CODE itself (naming, patterns, forbidden practices),
  not rules about workflow or process
- If REVIEW.md exists and contains causal bug patterns ("when you see X, check for Y
  because Z"), actively check for Y in the diff — these are the highest-priority checks
- Note: CLAUDE.md is guidance for Claude writing code — not all rules apply during review

---

**Agent C — History + Regression Detection**

This agent holds both git history context AND prior PR patterns in a single context,
enabling cross-cutting findings that isolated agents would miss.

**Git history analysis:**
- Run `git log --oneline -20 -- <file>` and `git blame` on the modified files
- Check: was this code recently fixed for the same kind of issue?
- Check: does the blame show this area has been a repeated source of bugs?
- Check: are the changes reverting or contradicting recent intentional fixes?

**Prior PR comment replay:**
- Find previous PRs that touched the same files: `gh pr list --state merged --search "file:path"`
  or `git log --oneline --follow -- <file>` then `gh pr list` to find associated PRs
- Read comments on those PRs (use `gh api repos/{owner}/{repo}/pulls/{number}/comments`)
- Check if any prior review comments apply to the current changes — is the author making
  the same mistake that was flagged before?

**Regression detection:**
- Cross-reference: does this diff re-introduce a pattern that was previously fixed
  (visible in git log) AND flagged in a prior PR comment? That's a high-confidence regression.
- Are historically buggy files being modified without corresponding test changes?

If `gh` CLI is not available, skip the PR comment analysis and note it in the output.
If no prior PRs touched these files, return only git history findings (or empty if none).

### Step 5: Verification

Launch a **single sequential agent** that receives ALL findings from Step 4 plus the
intent artifact from Step 2. This agent verifies each finding and assigns a confidence score.

**For each finding, the verifier checks:**

1. **Intent relevance**: "Does this finding affect the stated intent from Step 2?"
   - If irrelevant to the change's goal → cap at 50
2. **Traceability**: "Can I trace this to a specific line, path, or state combination?"
   - If untraceable or speculative → cap at 50
3. **Scope**: "Is this pre-existing (on lines the user didn't modify) or introduced by this change?"
   - If pre-existing → cap at 50

**Confidence rubric** (pass verbatim to the verifier):

- **0**: False positive. Does not stand up to scrutiny, or is a pre-existing issue unrelated
  to the change.
- **25**: Might be real, but could be a false positive. Unable to verify from the diff.
- **50**: Verified as real, but irrelevant to the stated intent, or rarely hit in practice.
- **75**: Real issue that will be hit in practice. Important and will directly impact
  functionality, OR directly violates a CLAUDE.md / REVIEW.md rule.
- **100**: Definitely real, directly impacts the stated intent, evidence confirms it.
  Test failure from Step 0 that correlates with the diff is automatic 100.

**Type-stratified thresholds** — findings below these are discarded:

| Source | Threshold | Rationale |
|--------|-----------|-----------|
| Agent A (state bugs) | **65+** | Real bugs are hard to express with high confidence from static analysis — a 70-confidence state violation is more valuable than a 95-confidence naming nit |
| Agent B (compliance) | **80+** | Rules are deterministic — if the rule exists and applies, confidence should be high |
| Agent C (regression/history) | **70+** | Historical patterns are probabilistic — a reintroduced anti-pattern is likely but not certain to be a bug |

**These are always false positives — discard regardless of score:**
- Things a linter, typechecker, or compiler would catch (imports, types, formatting)
- Pedantic nitpicks a senior engineer wouldn't flag
- General code quality concerns (test coverage, documentation) unless required in CLAUDE.md
- Issues silenced in code (lint ignore comments, type: ignore)
- Style preferences, naming opinions, simplification suggestions
- Runtime-state claims the verifier cannot trace from the diff — if verification would
  require reading code outside the diff and the verifier can't do so in 1-2 reads,
  the finding is speculative

**Cap at 50 (never escalate above) when:**
- The only evidence is a deleted comment or removed block from a prior fix commit.
  Removals of prior-fix code are often revised tradeoffs. Report at 50 as "UNCERTAIN —
  verify on-device before fixing," never higher. Only escalates if reproduced on a
  running build.
- Claims that depend on controller/animation resting values without reading the
  underlying Tween/animation definition
- "Missing dismiss/cleanup" claims when the parent controller manages lifecycle

### Step 6: Report

```
CODE REVIEW — [N files reviewed, 3 agents dispatched, tests: PASS/FAIL/SKIPPED]

CRITICAL (confidence 90-100) — [count]
  * [file:line] [description]
    Source: [Agent A|B|C] | Type: [state-bug | compliance | regression]
    Path: [execution path or state combination that triggers this]
    Fix: [specific change]

IMPORTANT (confidence 65-89) — [count]
  * [file:line] [description]
    Source: [Agent A|B|C] | Type: [state-bug | compliance | regression]
    Fix: [specific change]

CLEAN AREAS: [agents that returned no findings above threshold]

SUMMARY: [1-sentence assessment anchored to the stated intent from Step 2]
```

If no findings survived verification:
"No issues found. Reviewed [N] files against stated intent: [one-line intent summary].
Checked for state-based bugs, project rule compliance, and regressions. Tests: [PASS/FAIL/SKIPPED]."

### Step 7: Apply Fixes

For CRITICAL findings (90+): apply fixes immediately unless the user said "report only."

For IMPORTANT findings (65-89): offer to apply: "Want me to apply these fixes?"

Never apply fixes to code outside the reviewed scope.

## Error Handling

1. **No changes found**: "No changed files found. Provide a file path, PR number, or run after
   making changes. Example: `/code-review src/bot.py` or `/code-review 42`"
2. **Scope argument doesn't exist**: "File `$ARGUMENTS` not found. Check the path."
3. **CLAUDE.md not found**: Run Agents A and C only. Note: "No CLAUDE.md found — skipping
   compliance check (Agent B)."
4. **REVIEW.md not found**: Normal — most projects won't have one. No note needed.
5. **No prior PRs for Agent C**: Agent C reports only git history findings. Note in report:
   "No prior PR comments found for these files."
6. **gh CLI not available**: Agent C skips PR comment analysis. Note in report.
7. **Binary or generated files in diff**: Skip silently. Note excluded files in summary.
8. **Test runner not detected**: Skip Step 0 with "Tests: SKIPPED (no runner detected)."
9. **Tests timeout or error**: Record as test infrastructure issue, not a code finding.
   Note in report header and proceed with review.

## Examples

### Example 1: State bug caught via enumeration
**Input**: `/code-review`
**Process**: `git diff` shows changes to an auth guard. Step 0 runs tests (pass). Step 2
extracts intent: "Fix null dereference when anonymous session expires." Agent A builds
the state table, traces the session token through the guard, and finds that `ref.read(authProvider)`
is used post-await without re-read — the session can change across the async boundary.
Verifier scores 85 (state bug, traceable path, relevant to intent). Reports as IMPORTANT.

### Example 2: Regression caught via merged history agent
**Input**: `/code-review 42`
**Process**: Fetches PR #42 diff. Agent C finds via git blame that line 47 was fixed for an
off-by-one last month. A prior PR comment on PR #38 says "watch the boundary condition here."
The current diff reintroduces the same pattern. Cross-referencing history + prior comment
gives high confidence. Verifier scores 90. Reports as CRITICAL, auto-fixes.

### Example 3: Clean review
**Input**: `/code-review`
**Process**: Step 0 tests pass. 3 agents review 2 changed files. No findings survive verification.
**Output**: "No issues found. Reviewed 2 files against stated intent: 'Add retry logic to API client.'
Checked for state-based bugs, project rule compliance, and regressions. Tests: PASS."