review-code

# Reviewing code This skill reviews a diff against a fixed set of quality gates, plus any repo-specific conventions the target project declares for itself. It is the orchestrator: it gathers the diff, checks scope, dispatches the gate agents, and aggregates their verdicts. The rules each gate grades against live in the `conventions` plugin's topic skills; the gates themselves are this plugin's agents. The correctness gate hunts bugs on every diff. The other gates catch what slips through a regular review: convention drift, code placed in the wrong layer, duplicated logic that already exists somewhere else, and code that takes three readings to understand. ## Hard rules - **Failing even ONE checklist item is worth raising.** Many rules are expressed as a checklist of failure modes (e.g. a ternary rule listing nested / long / multi-line / non-trivial-branch as independent failure modes). A code item that trips a *single* box is a finding. Do NOT wait for two or three boxes to fail, and do NOT decide "it's only one of the conditions, that's fine." Walk each box independently and raise on the first failure. (Severity still calibrates per the rubric below; "worth raising" can be a 🟡, but it gets raised.) The recurring miss is pattern-matching a rule to its example domain, e.g. reading a "ternary" rule as "JSX ternaries only" and skipping a multi-line *value* ternary, so apply each checklist to every matching construct, not just the ones that look like the example. - **Apply DRY, YAGNI, and KISS as a standing lens on every diff, not just the specific checklists.** Evaluate every change against three questions and surface a finding whenever the answer is "no", with the same any-finding-fails weight as any other rule: - **DRY**: is any logic, value, literal, or markup duplicated that should be a single source? A copied block, a re-declared constant, a re-implemented helper. - **YAGNI**: is there speculative or unused code? Unused params/props, an abstraction with a single caller built for a hypothetical second one, config options nothing passes, dead branches, a generality the diff doesn't use. - **KISS**: is there a materially simpler equivalent? A lookup object beating nested conditionals, an early return beating nesting, an existing util/stdlib call beating a hand-roll, fewer moving parts for the same behavior. These are first-class smells, not stylistic extras. Surface them with evidence and let the user judge (per the confirmed-findings rule); never withhold a confirmed one because you decided the complexity was "probably needed". - **Never post anything to GitHub on your own.** No `gh pr review`, no `gh pr comment`, no `gh api` writes. Output stays in chat. If the user wants comments drafted or posted, they run `/code-review:post-review`. The plugin's hook blocks unapproved GitHub review writes. - **Verify conventions against the codebase before flagging.** Do not cite a "convention" from memory. Before saying "this codebase does it like X", grep the repo and confirm there are at least 2-3 existing examples of X. Cite the example file paths in your finding. - **Before flagging, confirm it's actually a deviation, not a misread.** Some things look wrong in isolation but are the house style when you grep. Grep to confirm the thing genuinely deviates from how the codebase does it, and cite the evidence. This check is about ACCURACY (don't raise a non-issue based on a misunderstanding); it is NOT a license to suppress. Once something genuinely deviates from a rule, you surface it; see the next two rules. - **Report what you can confirm; drop what you cannot.** A finding you cannot confirm (a possible bug you could not trace, a judgment call you cannot back with evidence from the code or docs) is dropped, not softened into a "maybe". A confirmed violation of a checklist box is never dropped or softened: whether it is acceptable is the user's (and the PR author's) call, not yours, so "intentional", "defensible", "matches the neighbors" or "low-value" is still never a reason to withhold one. Those words may be added as descriptions for the human's benefit. - **Any finding fails its gate. There is no finding that "still passes."** A gate is PASS only when nothing is found in it; the moment one thing is found, that gate is FAIL and the overall verdict is FAILED. Severity labels (🔴/🟠/🟡, issue/suggestion/nitpick) calibrate only the *weight communicated to the author*; they do NOT affect whether the finding is reported or whether the gate passes. A single 🟡 nitpick fails its gate and therefore the review. Report FAILED plainly; never round up to "looks good with minor nits." - (Self-review parallel: when reviewing *my own* work, precedent never excuses a violation either; the same confirmed-findings bar applies before I call work done.) - **When a rule lists examples, examples are illustrations, never an exhaustive boundary.** State the principle first, examples second, explicitly labeled as illustrations. Apply the rule to *every* item in the diff that matches the principle, including items not in the example list. If you find yourself thinking "this item isn't in the listed examples so the rule doesn't apply," you're misreading the rule. This applies BOTH when reading rules (don't narrow to the listed examples) AND when writing rules (don't frame a rule around a specific domain when the principle is general). - **Brevity is a hard rule on every surface: chat findings, drafted comments, and replies.** Length is the most consistently violated rule in this skill. Every unit of output has a budget, and a sentence past it must earn its place: **a chat finding is one line**, **a drafted comment is one to three sentences**, **a reply accepting feedback is one sentence plus the commit SHA**. A drafted `suggestion` carries one brief reason inside its budget (see `post-review`). Prose that restates the rule, justifies a change the reader already asked for, re-describes what a linked commit shows, or over-justifies a finding past that one reason does NOT earn its place; cut it. If the reader needs more of the why, they will ask. Count before you show: an over-budget unit is a rewrite *before* the user sees it, never after they ask you to shorten it. - **No em dashes (U+2014) in any output.** Not in the chat report, verdict blocks, drafted comments or replies. Use a period, comma, colon or parentheses instead. - **A pre-existing problem is flagged at most once, and only when it intersects the change.** Code the diff did not touch is not the author's to fix in this PR. When an untouched line interacts with a changed one (the change calls into it, extends it, or depends on its behavior), raise it once, briefly, marked as pre-existing; otherwise leave it out. A violation the diff copies or moves is not pre-existing: it is the diff's own (see "Matches the existing pattern" below). - **Read the actual changed code yourself** with the `Read` tool, after fetching the diff. Gate verdicts are a starting point: the specific issues live in specific lines and you need to see them to call them out usefully. - **A gate finding is a lead, not a fact. Verify every one before it reaches the user.** Gates apply a real rule to the wrong line, cite a file that does not hold what they claim, miscount, or assert behavior nobody ran. Before a finding enters the report, tick all four: (a) re-read the cited `file:line` at the head SHA and confirm it holds what the finding says; (b) when the finding names several files, check each one, not only the first; (c) when it asserts behavior, measure it rather than relaying the gate's reasoning; (d) confirm the measurement ran against the real module, not a global test mock or stub. A finding failing any box is dropped and listed on the report's `Dropped:` line with a one-clause reason, never omitted silently. The same bar applies to a finding you produced yourself. - **A sweep is complete or it did not happen.** Every hit a sweep prints gets its own `file:line` verdict. Never pipe a sweep through `head` or `tail`, take the "first N", sample, or stop reading at the first find: the hit count must equal the verdict count. A sampled sweep fails its box exactly like a sweep never run, because the line nobody read is where the miss is. - **Any fix proposed must itself pass every rule, of every gate.** Before a suggested rewrite goes into a finding (yours or a gate's), walk it against all the checklists, not only the rule that surfaced it. A replacement that still holds a multi-line ternary, a conditional spread, a hand-rolled helper or a subject-less name like `isOn` is worse than no suggestion, because it launders the violation as the reviewer's own recommendation. - **Cite line numbers, file paths, and head SHA** for every finding. Vague feedback is useless. - **Locale diffs require a per-key audit table, never an aggregate grep, never your memory.** This is enforced by the i18n gate agent: if the diff touches translation/locale files, that agent must produce a per-key audit table (one row per new key, checking namespace placement, pluralization, naming, and duplication; see the `conventions:i18n` skill's §I1) as its evidence, and a missing/incomplete table is a gate FAIL. You may NOT write any locale verdict, including "no new keys", "no issues this round", or silently dropping i18n from the checklist, without the table. An aggregate report ("greped N keys, all unique") is an automatic incomplete review: a direct-duplicate grep alone cannot catch a generic noun that belongs in a shared strings file, a singular-only label that should be a plural-aware key, or a key whose name doesn't match its own value. "It matches the neighbors" / "low-value" / "no new keys" are the three rationalizations that have caused this miss; none of them is valid without the table. ## Workflow: agent-driven gates The review is NOT a single inline pass. It is a fan-out of independent **gate agents**, one per convention topic, aggregated into a single PASS/FAIL verdict. The whole fan-out runs **every time the skill is invoked, including every re-review after the author has addressed previous comments**. There is no delta-only, "I already looked at this", or "only the changed files since last round" shortcut: a re-review re-runs every gate against the new head SHA from scratch. A previously-passed gate can fail on a later round (the fix introduced a new violation), so each round earns its PASS independently. **Why agents instead of inline:** an inline reviewer silently skips checklist items ("low-value", "matches the neighbors", "no new keys"); this has been the repeated failure mode. A dedicated agent that owns exactly one topic and must return a per-box PASS/FAIL verdict has no other job to crowd it out, so it cannot quietly drop a box. ### Step 1: gather context (you, the orchestrator) Run the bundled scripts from the target repo's working directory; each prints what it gathered, so read its output rather than re-deriving it. - **Diff, head SHA, changed files:** `bash "${CLAUDE_PLUGIN_ROOT}/scripts/gather-review.sh" <num>` for a PR, or `bash "${CLAUDE_PLUGIN_ROOT}/scripts/gather-review.sh" --branch [<base>]` for a local branch (base: `origin/HEAD`, then `main`, then `master`). It saves `review.diff` and `changed-files.txt` under `${TMPDIR:-/tmp}/kc-review-<sha>/` and prints `meta.env`: `HEAD_SHA`, `BASE`, `TITLE`, `DIFF_FILE`, `CHANGED_FILES`, and `PR` in PR mode. Every gate gets `DIFF_FILE` and `HEAD_SHA` from it; the sweep scripts run against `DIFF_FILE`. - **Head checkout:** when `meta.env` has a `FETCH=` line, the working tree is not on the head commit. Run that command so the gate agents can read files at the exact head SHA via `git show <sha>:<path>`. - **Scope facts (PRs):** `bash "${CLAUDE_PLUGIN_ROOT}/scripts/scope-facts.sh" <num>` prints the author, base, commit and file counts, the author's other open PRs, and each linked issue, saving the PR description (`pr-body.md`) and each issue body (`issue-<n>.md`) beside the diff. The §G1-§G3 boxes below are judged from it. - **On a re-review, also read the reply threads on your previous comments:** `bash "${CLAUDE_PLUGIN_ROOT}/scripts/review-threads.sh" <num> --mine <your-login>` (login: `gh api user --jq .login`) prints every thread, root then replies, and marks the threads you started and whether the author replied after your last comment. For every carried-over finding, classify the author's reply before re-raising it: - **A commit SHA** = "fixed in `<sha>`". Verify it against head, and verify the WHOLE class, not just the examples you named. A common pattern: the author fixes the exact items you listed but leaves the rest of the same class; the finding is still open, but re-frame it as "the named ones are done; the same applies to the rest", not as if nothing changed. - **Reasoned push-back** (e.g. "this matches the existing pattern, keeping it") = do NOT silently re-post a fresh duplicate comment on a thread they already answered; that reads as ignoring them. Surface their reasoning to the user, and either drop the finding or reply in the existing thread with the counter-point; let the user decide. - **No reply / unaddressed** = re-raise, noting it is still open from the prior round. Carry the author's responses into Step 4 so the report and any re-drafted comments reflect what was fixed, what was declined-with-reason, and what is genuinely still open. Posting a re-review that ignores the author's replies is a documented trust failure. If a script fails, relay its message; fall back to the equivalent `gh`/`git` command by hand only when the script itself is unavailable, and say so in the report. **Scope checks (you, before the fan-out).** These are about the target as a whole, so no single gate owns them. Walk each box yourself and record the result as a `SCOPE:` block in the same `BOXES:` shape the gate agents return (Step 3); the verification gate audits it (its §V7). A scope finding goes into the report like any gate finding and fails the review the same way. - [ ] §G1 The target is not stacked: the author's other open PRs were listed (`scope-facts.sh`), and when the diff is large (30+ files) or the PR's base or commits belong to another open branch (compare its head, base and commit count with the listed PRs), the review stops and reports the stacking before any line-level finding, suggesting a rebase onto the default branch. A stacked diff cannot be reviewed in isolation. (N/A: local diff, or the author has no other open PR) - [ ] §G2 The diff does only what its title and description say. Out-of-scope work (an unrelated refactor, removed public surface, a new abstraction, a bundled feature or option) is one finding asking to split the PR, naming which commits belong in which PR; it is not reviewed silently as if it were in scope. (N/A: never; every target has a stated purpose, even if it is only a branch name) - [ ] §G3 Every linked issue was read in full (the `issue-<n>.md` files `scope-facts.sh` saved, and its `pr-body.md`), and each constraint it states ("no public API change", "must work offline") is checked against the diff; a violated constraint is a finding that cites the issue. (N/A: no linked issue) - [ ] §G4 A diff too large to read in one pass (roughly 35KB or more) was read from the saved file in offset/limit chunks, every chunk; no hunk was skimmed. The gate agents get the saved path, not a truncated paste. (N/A: the diff was read whole) ### Step 2: dispatch the gate agents in two phases (mandatory: ALL gates, EVERY round) Dispatch each gate with the Agent tool, `subagent_type` set to the agent name below. Phase 1 runs in parallel: issue all ten Agent calls in a single message. Phase 2 (verification) runs only after every phase-1 agent has returned, because it audits their returned verdict blocks rather than re-deriving findings. Each topic gate preloads its `conventions:<topic>` skill, so it already holds that topic's `## Review checklist`, `## Review detail` and `## Sweeps`. You do not paste rules into the prompt. **Phase 1: dispatch these ten in parallel.** | Gate | Agent (`subagent_type`) | Dispatch note | |------|-------------------------|---------------| | naming | `code-review:naming-gate` | always | | clarity | `code-review:clarity-gate` | always | | structure | `code-review:structure-gate` | always | | simplicity | `code-review:simplicity-gate` | always | | datetime | `code-review:datetime-gate` | agent returns PASS (N/A) if no date/time logic in diff | | react | `code-review:react-gate` | agent returns PASS (N/A) if no React/JSX UI code in diff | | i18n | `code-review:i18n-gate` | agent returns PASS (N/A) if no locale files in diff | | testing | `code-review:testing-gate` | agent returns PASS (N/A) if the diff changes no behavior and touches no test file | | correctness | `code-review:correctness-gate` | always | | project-conventions | `code-review:project-conventions-gate` | agent returns PASS (N/A) if target repo has no `.claude/review-conventions.md` | **Phase 2: dispatch only after every phase-1 agent has returned.** | Gate | Agent (`subagent_type`) | Dispatch note | |------|-------------------------|---------------| | verification | `code-review:verification-gate` | always; its prompt additionally includes every phase-1 gate's returned `GATE:`/`STATUS:`/`BOXES:`/`FINDINGS:` block verbatim, your `SCOPE:` block, and on a re-review your classification of each carried-over finding (Step 1) | Every gate is dispatched even when it looks inapplicable. A gate whose rules don't apply returns `PASS (N/A)` **with the reason**; you do NOT skip dispatching it. "No hooks changed, so react is N/A" is a verdict the gate agent must produce after reading the diff, never an assumption you make on its behalf. The `project-conventions` gate is how this skill stays repo-agnostic while still enforcing house rules: it reads the target repo's own `.claude/review-conventions.md` (if one exists) and grades the diff against whatever that file declares. A repo with no such file is legitimately N/A; it is not this skill's job to invent conventions for a repo that hasn't written any down. When a repo rule contradicts a built-in convention, the repo rule wins for that repo: the gate lists the override (its §X3), and you report the built-in box as overridden, citing the repo rule, instead of failing it. **Fallback when a gate agent is unavailable.** If an Agent call fails because the `code-review:<gate>` agent type does not exist (for example, the `conventions` dependency failed to load, or the plugin's agents are not registered), dispatch a `general-purpose` agent for that gate instead. Tell it to invoke the `conventions:<topic>` skill with the Skill tool first, then follow the gate-agent contract in Step 3 exactly, returning the same verdict block. For project-conventions or verification, tell it to read `${CLAUDE_PLUGIN_ROOT}/agents/<gate>.md` and follow it. If the `conventions:<topic>` skill cannot be invoked either, the gate is FAIL with the evidence "topic skill unavailable"; never grade a topic from memory. Report every fallback in the output (see Output format). ### Step 3: the gate-agent contract The agents carry this contract in their own definitions; the dispatch prompt supplies the inputs. Give each phase-1 agent: - the head SHA (`HEAD_SHA` from `meta.env`); - the path to the saved unified diff file (`DIFF_FILE` from `meta.env`); - the changed-file list (`CHANGED_FILES` from `meta.env`); - the target repo root. Each agent must: 1. Read its topic's `## Review checklist` and `## Review detail` in full. 2. **Sweep the greppable constructs FIRST, before reading the diff for meaning.** Some boxes cover constructs a script can enumerate exactly, and those boxes are graded on whether the hits were **listed**, not on whether they were noticed. The agent runs every script under its topic's `## Sweeps` against the diff file, then gives every hit a `file:line` verdict in the evidence, with the hit count. `0 hits` must be stated explicitly; a box with no receipt is FAIL by default, because silence is indistinguishable from never having looked. 3. Evaluate EVERY checklist box against every matching construct in the diff. Before failing a box on a convention, grep the target repo to confirm the convention is real (cite 2-3 example paths in the evidence). 4. Return ONLY this structured verdict: ``` GATE: <name> STATUS: PASS | PASS (N/A) | FAIL BOXES: - [PASS|FAIL|N/A] <box id>: <evidence: file:line, grep result, or "no matching construct in diff"> FINDINGS: - <severity 🔴|🟠|🟡> <file>:<line>: <issue>. <fix> ``` 5. Failure semantics: a box is **FAIL** if ANY matching construct in the diff violates it; a sweep whose hit count differs from its verdict count is FAIL too (never truncated or sampled); the gate STATUS is **FAIL** if ANY box is FAIL. Do not average, do not "mostly pass". 6. Drop a finding it cannot confirm, never a confirmed one, and write no em dash (U+2014) in the block. 7. Walk every fix it proposes against every gate's checklist, not only its own, before writing it into a finding. A verdict that does not have this shape, or that has a box with no evidence, is rejected: send it back to the same agent for the per-box `BOXES:` breakdown before aggregating. ### Step 4: aggregate into one verdict (you, the orchestrator) - Overall verdict is **PASSED** only when every gate returned `PASS` or `PASS (N/A)`, and the `SCOPE:` block has no FAIL box. - **A single FAIL box in any one gate means overall FAILED.** State plainly that the review has not passed and the author needs further changes; do not soften a FAILED into "looks mostly good" or "just nits". A nitpick-severity finding still fails its box and therefore the review; severity calibrates the *comment*, not whether the gate passed. - **A gate PASSES only when it found nothing. Any finding = that gate FAILED.** Do not let a gate agent (or yourself) return PASS while also listing a finding; that is contradictory and means FAIL. Aggregate by surfacing every finding from every gate; never drop or merge-away a confirmed finding because you judged it intentional/acceptable (that call is the user's; see Hard rules). The report lists all findings found, and the verdict is FAILED if the list is non-empty. - A built-in box the project-conventions gate lists as overridden (§X3) is reported as overridden, citing the repo rule, and does not fail its gate. - Emit the gate-status table first, then the deduped findings, then the explicit `PASSED`/`FAILED` verdict (see Output format). - On a re-review, run Steps 1-4 again in full, and fold in the author's replies to your prior comments (Step 1): mark each carried-over item as fixed (verify the whole class), declined-with-reason (surface it, don't re-post over their reply), or still-open. Report the new verdict; do not carry forward a prior round's PASS. **Self-review trigger:** when *I* am the one implementing changes (not reviewing a teammate's PR), the gate fan-out runs in two cases only: when the user asks for a review, or once when a big piece of work is finished and a PR is about to be opened. It does not run after each fix or for small changes; the fan-out costs too much time and too many tokens for that. When it runs, lint and types passing is not the same as gate-clean, and every gate's checklist is walked box by box to a PASS. Separately, UI work is looked at rendered in a browser before I call it done (see the verification gate's self-review group), since tests lay nothing out. **"Matches the existing pattern" is a yellow flag, not a green light.** A gate agent must not pass a box because the diff copied the shape of nearby code. If the surrounding pattern violates the rule, the new code that extends it violates it too. Every copied block, every matched helper, every duplicated key style is evaluated against the rule on its own; nearby precedent is not the evaluation. ## Output format Report directly in chat. No file output. **Be terse.** The reader is the user, not the PR author; they want to scan, not read prose. **Rules:** - One finding = one line. Two short lines max if a fix snippet is genuinely needed. This is a budget, not a target to fill; most findings fit in one line and should stay there.