/code-critic

name: code-critic
description: >
  Code review checklist and coding standards, extended per project by
  whatever file AGENTS.md's Review & Planning Guidance section names
  (defaulting to docs/agent-rules/code-critic.md). Invoked as /code-critic
  for an ad-hoc review, or applied by the code-critic sub-agent in the
  /feature workflow. (Named code-critic so it does not shadow Claude Code's
  bundled code-review skill.)

/code-critic — Code Review

Apply this skill to review code changes against project standards.

---

Stance

Treat the implementation as a hypothesis under attack. Your default assumption is that something is wrong; your job is to find what.

Confirmation bias is the dominant failure mode of AI code review — guard against it by actively trying to disconfirm the implementation rather than verifying that it looks reasonable. If you cannot find a fault after genuine effort, that is itself a finding worth stating explicitly (see the Output Format section).

---

Before Reviewing

Selecting the diff

In the `/feature` workflow the implementation is committed before review, so a bare `git diff` (working tree) shows nothing — review the branch's committed changes against its base, the default branch named in `AGENTS.md` → *Commands*: `git diff <default-branch>...HEAD` (or `git log -p <default-branch>..HEAD`). Invoked ad-hoc on uncommitted work, review the working-tree diff instead. If unsure what changed, check `git status` and `git log --oneline` first.

Note for ad-hoc use: the push gate records a commit SHA (`scripts/review-ok.sh`), so a review meant to unlock a push must cover the committed state — commit first, then review.

Architecture Decision Records

Check if a `docs/adr/` directory exists. If it does, list the ADRs you read by number in the review output. If none seemed relevant to the diff, say so explicitly — silence is not acceptable, since it is indistinguishable from skipping the step.

If the diff contradicts an ADR, quote the contradicting ADR clause and the offending diff line, and flag it as `FAIL`.

Implementation Plan

If plan text was provided (inline in the prompt, or via a file path), use it before reviewing. Use it as follows:

• **Approval Summary / Acceptance Criteria**: the human-approved contract.

Verify every `AC-<slug>-n` has a committed test (via the `[AC-<slug>-n]` tags in Test Strategy) that would fail if the criterion were broken — a criterion without one is `FAIL`.

• **Contract** section (if present): cross-check the diff's routes, form

fields/params, response shapes, error rendering, and schema changes against it. An undiscussed deviation from the approved Contract is `FAIL`.

• **Requirements** section: the source of truth for what should have been built —

used to verify Plan Compliance and that all specified edge cases are handled.

• **Approach** section: the agreed implementation strategy — used to verify the

code follows the intended design rather than an ad-hoc alternative.

• **Edge Cases** section: the enumerated scenarios that must be handled.

Verify every `EDGE-<slug>-n` has a committed test (via the `[EDGE-<slug>-n]` tags in Test Strategy) that would fail if the handling strategy were broken — an edge case without one is `FAIL`.

• **Test Strategy** section: the agreed test coverage — cross-reference against

the actual tests written.

• **Files** section: the planned file manifest — cross-check against the diff. A

file in the diff that is not listed here (or listed but left untouched) is an undiscussed change; flag the mismatch and judge whether it is in scope.

• **Out of Scope** section: the explicit boundary — do NOT flag as missing

anything listed here, and DO flag as scope creep any code that strays into it.

If no plan was provided, skip the Plan Compliance checklist section entirely.

**Test evidence.** The review verifies coverage statically; whether the suite actually ran and passed on the reviewed state is separate evidence. In the `/feature` workflow that evidence is passed in (the summary of the latest full test-suite run). Treat that summary as a record of the run, not independent proof — it is produced by the implementing agent, and this review has no independent way to confirm it (whether the project's own CI provides that is outside this skill's scope). If no evidence was provided and you cannot (or may not) run the suite yourself, do not assume it is green — raise an Open Question: "no evidence the test suite ran on the reviewed state".

When flagging a plan compliance issue, **quote the exact line from the plan** that the diff violates, alongside the diff line that violates it. Paraphrasing the plan is not enough — the developer needs to see the literal mismatch.

Reading Beyond the Diff

You may read any file in the repository. The diff is the unit under review, but surrounding code, tests, configuration, and migrations are fair context — and often necessary to judge whether the change is correct. Some project-specific rules below may explicitly require it (cross-referencing a sibling file, checking that a registration or annotation is present).

---

Coding Standards

Single Responsibility

**Functions:** A function should do one thing. If a function contains a conditional branch that handles a fundamentally different concern (e.g. an admin path bolted onto a regular-user path, or a parsing path inside a persistence call), flag it. Severity:

• New function introduced with mixed concerns → `FAIL`
• Existing function extended with a clearly unrelated branch → `FAIL`
• Borderline case where extraction would hurt readability → `NEEDS_DECISION`

**Classes:** A class has too many responsibilities if you can identify more than one independent reason it would need to change.

When reviewing new code that touches an existing class:

• If the new code **introduces** a class with too many responsibilities → `FAIL`
• If the new code **significantly worsens** an ex