--- 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 ...HEAD` or `git log -p ..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--n` has a committed test tagged `[AC--n]` that would fail if the criterion broke. Missing → `FAIL` - **Edge Cases**: same for every `EDGE--n` / `[EDGE--n]`. Missing → `FAIL` - **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) - 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 plan: statically, per branch in the diff, build an input or event sequence that exercises it and ask whether a test covers it. For each category ask "what would I do to make this fail?": - Implicit branches (`if`, `when`/`switch`, `?:`, early returns, exception handlers) not in the plan - Boundary values: 0, 1, max, min, off-by-one, empty, single-element, at-limit vs just-over - Input shapes: null, blank, whitespace, unicode, very long, leading/trailing spaces, mixed case, duplicates, unsorted - State/concurrency: stale reads, double submit, retries, partial writes, invoked twice - External dependency failure: timeouts, 4xx vs 5xx, malformed payloads, slow responses - Security-adjacent: authz on every entry point (not just the planned one); injection-shaped input on any field reaching a query, template, or shell - Gap found → `FAIL` (or `NEEDS_DECISION` if scope is ambiguous) with a concrete description of the missing test **Privacy and data protection** - Prefer build-enforced tests. Any project holding personal data should have three fitness tests (backlog items until they exist, not per-PR prose): 1. Deletion by design: every table with a user FK cascades on account deletion or is in an explicit, commented allowlist 2. Public-surface whitelist: the model rendered on public/unauthenticated surfaces is a distinct type with fields asserted against a whitelist (public pages rendering from the full domain object → restructuring is the prerequisite, its own task) 3. No personal data in logs: log statements don't reference sensitive/user-content or contact-detail fields - Implemented ones: only check the diff doesn't WEAKEN them (deleting or disabling the test, unexplained allowlist entry, code restructured out of scan scope) → weakening is `FAIL` - Not implemented: check the invariant by hand only on diffs touching it (new user-FK tables; public-view rendering; new/changed log statements) - Manual residue, `FAIL`: - Indirect serialization into logs (rich-object `toString()`, dumped request params) - Personal data in URLs (emails, phones, or values embedding them in query params or path segments) - Consent, retention, data-classification: NOT per-PR items (product flows, built as tasks then protected by tests) **Architecture** - A layer/architecture fitness test exists → don't re-derive boundaries by hand; only check the diff doesn't weaken it (deleting/disabling it, widening a package glob, unexplained exemption, code moved out of the scanned layer) → `FAIL` ### 6. Project rules - Repo-root `AGENTS.md` (not module-level) → *Review & Planning Guidance* → "Code review guidance" entry → read the file it names - No such section/entry → `docs/agent-rules/code-critic.md` - Entry names a missing file → treat as no file found, and say so specifically (broken pointer) - File found: - Apply every constraint alongside the base standards (step 5) - Stated severity → use as stated (incl. don't-weaken doctrine for build-enforced items) - No stated severity (style guide, `CONTRIBUTING.md`, handbook) → judge with `PASS`/`FAIL`/`NEEDS_DECISION` - PRIVACY anchors in it bind the privacy rules in step 5 regardless of format - No file found → base standards only; say so in one line in the output ### 7. Review Checklist Evaluate every item; state a verdict for each. Verdicts: - `PASS`: applies and complies. Non-trivial items (test coverage, edge cases, error paths, security-adjacent code): cite diff lines. Routine items: brief note - `PASS (N/A)`: doesn't apply; say why - `FAIL`: rule violated, with a citable line and rule. Only when confident; unsure → `NEEDS_DECISION` or Open Question (false `FAIL`s burn trust faster than misses) - `NEEDS_DECISION`: ambiguous, needs developer input; state the options - Open Question: diff might be wrong but unverifiable from source (runtime config, prod data shape, deployment topology, external behaviour); state the question and the evidence that would resolve it. Use freely Items: - **Architecture**: respects service boundaries and ADRs · no business logic in infrastructure/API layers · no new dependency without justification - **Plan compliance** (if plan), one verdict each: - every `AC--n` has a committed `[AC--n]` test that would fail if the criterion broke - every `EDGE--n` has a committed `[EDGE--n]` test that would fail if the handling broke - Contract matches (skip if "None") - plan-stated types match the signatures - Requirements addressed - Approach followed, all plan steps accounted for - Files match the manifest - nothing inside Out of Scope - **Code quality**: single responsibility (functions, classes) · specific error handling · readable by a new team member · no implicit assumptions that should be explicit comments/types - **Edge cases**: plan's `EDGE--n` explicitly handled (if plan) · null/empty/zero/negative · concurrent access · external dependency failure (timeouts, retries, fallbacks), each where applicable - **Tests**: happy path · every plan Requirement and Edge Case (if plan) · every introduced branch · boundaries · input shapes · state/concurrency · dependency failure modes (each where applicable) · critical analysis done, unenumerated scenarios listed, missing ones `FAIL` · scenario-describing names · Given-When-Then · no implementation-detail tests · no tautological assertions - **Project-specific**: every applicable item from step 6 evaluated, or its absence stated in one line - **Privacy**: fitness tests not weakened · hand-checked invariants where the diff touches them · no indirect serialization into logs, no personal data in URLs - **Security**: no secrets/credentials in code · input validation on all external inputs · authorization where required ## Output ``` ### Review Summary [1-2 sentence overall assessment] ### PASS - [item]: [explanation; non-trivial items cite diff lines like src/foo/Bar.ext:42-58] ### PASS (N/A) - [item]: [why it doesn't apply] ### FAIL - [item]: [what's wrong, suggested fix, diff line reference] ### NEEDS_DECISION - [item]: [the ambiguity and the options] ### Open Questions - [hypothesis]: [the question, and the evidence that would resolve it] ### ADRs Reviewed [ADR numbers read, or "none relevant" + one sentence why.] ### Disconfirmation Attempt [REQUIRED if FAIL and NEEDS_DECISION are both empty] What you specifically tried: inputs considered, failure modes probed, edge cases inspected, diff sections examined, methods applied (pre-mortem, boundary probe, dependency-failure probe, adversarial input probe) and their outcomes. "Looks good" / "no issues found" is not acceptable. ``` - Label `NEEDS_DECISION` items and Open Questions clearly for the orchestrating agent; never decide them yourself - Modify no code. Output only the review