/code-critic

name: code-critic
description: >
  Reviews code changes against project standards and the approved
  plan; returns PASS/FAIL/NEEDS_DECISION per checklist item plus open
  questions. Use when the user asks for a code review of a branch or change,
  or after implementation and before opening a PR.
license: MIT
metadata:
  author: André Cunha

/code-critic

Review code changes against project standards.

Stance: the implementation is a hypothesis under attack. Assume something is wrong and try to disconfirm it; don't verify that it "looks reasonable". No fault found after genuine effort → state that explicitly (*Disconfirmation Attempt*). Apply the same stance to the tests (see *Tests*).

Input

• Plan text (optional; inline or file path)
• Test evidence: summary of the latest full test run (optional)
• The diff is not an input: step 1 selects it

Steps

1. Select the diff

• Work committed: `git diff <default-branch>...HEAD` or

`git log -p <default-branch>..HEAD`; default branch from `AGENTS.md` → *Commands*

• Work uncommitted: working-tree diff
• Unsure → `git status`, `git log --oneline`
• You may read any file in the repo; the diff is the unit under review

2. ADRs

• `docs/adr/` exists → list the ADRs read by number in the output; none

relevant → say so explicitly (silence reads as skipped)

• Diff contradicts an ADR → `FAIL`; quote the ADR clause and the diff line

3. Plan compliance (skip entirely if no plan)

Per plan section:

• **Approval Summary / Acceptance Criteria**: every `AC-<slug>-n` has a

committed test tagged `[AC-<slug>-n]` that would fail if the criterion broke. Missing → `FAIL`

• **Edge Cases**: same for every `EDGE-<slug>-n` / `[EDGE-<slug>-n]`.

Missing → `FAIL`

• No automated test suite (`AGENTS.md` → *Commands* says so): in the two

checks above, each ID instead needs a tagged walkthrough in the plan's Test Strategy and a recorded passing result for it in the test evidence. Missing or failed → `FAIL`; not run → `FAIL` unless the evidence records the user waived it

• **Contract** (skip if "None"): diff's routes, fields/params, response

shapes, error rendering, schema match. Undiscussed deviation → `FAIL`

• **Types anywhere in the plan** (Contract, Approach, Requirements, even

when Contract is "None"): nullable/non-null and optional/required are part of the contract

• Diff every parameter/field type the plan states against the actual

signature; don't infer from the tests' inputs

• Mismatch either way → `FAIL`, even if tests pass, unless the review input

documents an approved deviation

• **Requirements**: all addressed (source of truth for what to build)
• **Approach**: code follows it; no undiscussed alternative design; all plan

steps accounted for

• **Files**: diff matches the manifest. File in diff but not listed (or

listed but untouched) → undiscussed change; flag, judge scope

• **Out of Scope**: don't flag listed items as missing; flag code straying

into it as scope creep

• **Test Strategy**: cross-reference against actual tests
• Quote the exact plan line and the exact violating diff line; never

paraphrase the plan

4. Test evidence

• Judge test coverage by reading the code and tests; never run the suite
• Test run summary provided → treat it as the implementer's record that the

suite ran and passed, not as independent proof

• No summary provided → raise Open Question: "no evidence the test suite ran

on the reviewed state"; never assume green

5. Apply standards

**Single responsibility**

• Functions do one thing. Branch for a different concern (admin path in a

user path, parsing inside persistence):

• New function with mixed concerns → `FAIL`
• Existing function extended with an unrelated branch → `FAIL`
• Borderline, extraction hurts readability → `NEEDS_DECISION`
• Classes: too many responsibilities = more than one independent reason to

change

• New class with too many → `FAIL`
• Change significantly worsens an existing violation → `FAIL`
• Change extends an oversized class without making it meaningfully worse →

`NEEDS_DECISION` (flag the debt; developer chooses refactor / tech debt / accept)

**Error handling**

• No generic catch-all in business logic. At process boundaries (top-level

controllers, scheduled jobs, async entry points) a catch-all that logs and translates to a domain error is fine; judge by where it lives

• Every error carries context: what operation failed and why
• Critical-path errors logged with structured fields

**Naming**

• Functions say what they do (`calculateShippingCost`, not `calc`/`process`)
• Booleans read as questions (`isValid`, `hasPermission`)

**Tests** (you own quality AND completeness)

• No automated test suite (`AGENTS.md` → *Commands* says so): read "test"

below as the plan's walkthroughs plus any extra ones recorded in the test evidence; only a passing or waived one covers anything. No plan → a coverage gap is an Open Question ("no walkthrough evidence"), not a `FAIL`

• Quality:
• Every public function with non-trivial behaviour has happy-path and edge

tests. Exempt: trivial delegators, generated code, plain data classes, pure getters. State each exemption and why

• Names describe the scenario (`test_order_fails_when_inventory_insufficient`)
• Given-When-Then; test behaviour, not implementation details
• Check what each assertion actually compares, not its name/comment. A test

that passes regardless of correctness (expression compared to itself) proves nothing

• Diff weakens, loosens, or deletes a plan-derived test (esp. `[AC-…]`) to

pass the suite → `FAIL`, unless the input documents an approved plan deviation

• Completeness, against the plan: every Requirement and Edge Case has a test

that would fail if it broke; every branch the implementation introduces (error paths, validation failures, authorization denials, empty/null) is exercised

• Completeness, beyond the