review · diff
git:20260924.e9e9cf4 to git:20260925.6dde440
56 added, 27 removed. Audit A to A.
---
name: review
description: Multi-lens code review (parallel lenses, a separate scorer, only findings ≥80 confidence reach the report). - Use when the user asks to "review this", "review the diff", "code review", "check my changes", "review the PR", "simplify this", or before shipping substantial work.
user-invocable: true
---
# Review: parallel lenses, separate scorer, one report
+ Finder lenses read the diff in parallel, a separate scorer rates every finding, and one report carries what clears the bar.
+
## 1. Scope
- Default: uncommitted changes (else the last commit); the user may name a range, PR, or files. Collect the diff + the task context (what was ASKED: from the conversation, the issue, or its `## Spec`) so Spec-faithfulness is judgeable.
+ - Default: uncommitted changes, else the last commit. The user may name a range, PR, or files.
+ - Collect the diff and the task context: what was ASKED, from the conversation, the issue, or its `## Spec`. Spec-faithfulness is judged against it.
## 1b. Tier: full by default, light by criteria
- Two tiers. **Default is FULL.** An explicit invocation arg wins both ways: `/workkit:review light` forces light, `/workkit:review full` forces full.
-
+ - **Default is FULL.** An explicit invocation arg wins both ways: `/workkit:review light` forces light, `/workkit:review full` forces full.
- **Full tier**: every applicable lens in §2, in parallel, plus the §3 scorer.
- - **Light tier**: ONE `workkit:verifier` agent carrying the combined finder mandate (bugs + compliance + spec-faithfulness + simplification, plus one line of the Parity mandate below: name each changed file's siblings and report where the new code differs; same brief discipline, still execution-verifies its claims), plus the §3 scorer. The scorer is never skipped. Finder-never-scores is the integrity core of both tiers.
+ - **Light tier**: ONE `workkit:verifier` agent carrying the combined finder mandate, plus the §3 scorer:
+ - bugs, compliance, spec-faithfulness, and simplification;
+ - one line of the Parity mandate (§2): name each changed file's siblings and report where the new code differs;
+ - the same brief discipline, and it still execution-verifies its claims.
+ - Both tiers run the §3 scorer: it is never skipped.
Downgrade to light ONLY when ALL of these hold (any miss = stay full):
+
1. Small diff: under ~150 changed lines AND ~5 files.
- 2. No guard surface touched: nothing under `.claude/` (hooks, skills, agents, `.claude/settings.json`) and, in the dotfiles repo, nothing under `setup/`, the surfaces where a one-liner is catastrophic. Other repos' own config files are ordinary code, judged by criteria 1 and 3.
- 3. Fix-scope: a bug fix or tweak to existing behavior, not a new feature or new file of source.
+ 2. No guard surface touched: nothing under `.claude/` (hooks, skills, agents, `.claude/settings.json`), and in the dotfiles repo nothing under `setup/`. Those are the surfaces where a one-liner is catastrophic. Other repos' own config files are ordinary code, judged by criteria 1 and 3.
+ 3. Fix-scope: a bug fix or tweak to existing behavior, not a new feature or a new source file.
- The light reviewer may return `NEEDS_FULL` (with one line of why) instead of a report; the skill then upgrades to the full panel immediately. A wrong downgrade costs one escalation; never argue with the escape hatch.
+ - The light reviewer may return `NEEDS_FULL`, with one line of why, instead of a report. The skill then upgrades to the full panel immediately.
+ - A wrong downgrade costs one escalation, so never argue with the escape hatch.
## 2. Lenses: parallel subagents
- Dispatch per the file-handoff convention: the brief goes to a file in the session scratchpad, and each lens returns its full findings INLINE. Launch in ONE message:
-
- | Lens | Agent | Mandate |
- |---|---|---|
- | Compliance + spec | a user-level agent named `reviewer` if one exists (personal preloads), else `workkit:reviewer` | Derive checklist from live docs; Spec-faithfulness vs the task context |
- | Bugs | `workkit:scout` agent | Trace the diff for defects: logic, edge states, silent fallbacks. Read surrounding code, not just the diff |
- | Simplification | `workkit:scout` agent | Run the deletion test over the diff's ADDITIONS (`js:patterns` `resources/code-design.md`): wrappers that add nothing, options with one caller, defensive branches for impossible states, needless indirection. Clarity over brevity. Clearer sometimes means more lines, and an abstraction serving a NAMED second consumer is not clutter (global §3). Findings name the collapse, never apply it |
- | History | `workkit:scout` agent | `git log`/`blame` on touched files: does the diff fight a past fix, revert intent, or repeat a reverted approach? |
- | Firestore rules | `workkit:scout` agent | ONLY when the diff touches BEM/Firestore work: reads vs rules coverage both ways |
- | Parity | `workkit:scout` agent | Full tier only. For every file the diff adds or changes, name its siblings (the same kind of thing on another surface, target, command, or package) from the repo's docs and directory shape, then report where the new code's shape, naming, entry point, logging, or call form differs from them. The principle is the global AGENTS.md parity rule (like things use like systems); the lens quotes it, never owns it. This row is the KIT's one home of the mandate's wording: the light tier above and the per-issue drift question in `agents/verifier.md` both quote it. Findings name the sibling and the mismatch, never apply a fix |
-
- Never tell a lens what NOT to flag and never pre-rate severity in the brief. That manufactures false negatives. The `manager/resolver` hook supplies each class agent's model per spawn, so never pass a `model` param.
+ - Dispatch per the file-handoff convention: the brief goes to a file in the session scratchpad, and each lens returns its full findings INLINE.
+ - Launch every lens in ONE message:
+ - **Compliance + spec**: a user-level agent named `reviewer` if one exists (personal preloads), else `workkit:reviewer`. It derives its checklist from live docs and judges Spec-faithfulness against the task context.
+ - **Bugs**: `workkit:scout` agent. Trace the diff for defects: logic, edge states, silent fallbacks. Read the surrounding code, not just the diff.
+ - **Simplification**: `workkit:scout` agent. Run the deletion test over the diff's ADDITIONS (`js:patterns` `resources/code-design.md`): wrappers that add nothing, options with one caller, defensive branches for impossible states, needless indirection.
+ - Clarity over brevity. Clearer sometimes means more lines, and an abstraction serving a NAMED second consumer is not clutter (global §3).
+ - Findings name the collapse, never apply it.
+ - **History**: `workkit:scout` agent. `git log`/`blame` on touched files: does the diff fight a past fix, revert intent, or repeat a reverted approach?
+ - **Firestore rules**: `workkit:scout` agent, ONLY when the diff touches BEM/Firestore work. Check reads against rules coverage, both ways.
+ - **Parity**: `workkit:scout` agent, full tier only. This entry is the kit's one home of the mandate's wording; the light tier and the per-issue drift question in `agents/verifier.md` both quote it.
+ - For every file the diff adds or changes, name its siblings from the repo's docs and directory shape: the same kind of thing on another surface, target, command, or package.
+ - Report where the new code's shape, naming, entry point, logging, or call form differs from them.
+ - The principle is the global AGENTS.md parity rule (like things use like systems). The lens quotes it, never owns it.
+ - Findings name the sibling and the mismatch, never apply a fix.
+ - Never tell a lens what NOT to flag, and never pre-rate severity in the brief. That manufactures false negatives.
+ - The `manager/resolver` hook supplies each class agent's model per spawn, so never pass a `model` param.
## 3. Scorer: separate pass
- The `workkit:verifier` agent (never a finder in the same pass) scores every collected finding 0–100: "how certain is this a real issue a maintainer would fix?" It gets the finding + the relevant code, applies the false-positive list (linter-catchable, unmodified lines, pre-existing, no written rule, unreachable hypotheticals → score 0) and re-checks the claim against the actual file before scoring. Where a lens attached its own confidence, the scorer's number wins. A Parity finding whose file has no sibling scores 0. The final ship/fix/rework verdict stays with the dispatching session, frontier-or-session judgment by construction.
+ - The `workkit:verifier` agent scores every collected finding 0–100: "how certain is this a real issue a maintainer would fix?"
+ - It is never a finder in the same pass. Finder-never-scores is the integrity core of both tiers.
+ - It gets the finding and the relevant code, and re-checks the claim against the actual file before scoring.
+ - The false-positive list scores 0: linter-catchable, unmodified lines, pre-existing, no written rule, unreachable hypotheticals. So does a Parity finding whose file has no sibling.
+ - Where a lens attached its own confidence, the scorer's number wins.
+ - The final ship/fix/rework verdict stays with the dispatching session: frontier-or-session judgment by construction.
## 4. Report
- One consolidated report: findings **≥80** as actionable items (file:line, issue, fix); 40–79 compressed into a "lower confidence" note; below 40 dropped silently. End with a verdict: ship / fix-then-ship / rework. Every item in the report, and every issue one of them names, reads in the cold-reader line (`docs/project-state.md` § Restating an issue). Done-criteria: every ≥80 finding names its file:line and concrete fix; no lens output pasted raw into chat.
-
- A finding that gets FILED rather than fixed now passes the filing litmus test first: *would closing an open issue automatically mean this is done too?* Yes → it attaches there, never as a sibling issue; polish-grade findings batch as checklist lines onto the surface's rolling `polish: <surface>` issue. The rules: `docs/project-state.md` § How big is one issue.
+ - One consolidated report:
+ - findings **≥80**: actionable items (file:line, issue, fix);
+ - 40–79: compressed into a "lower confidence" note;
+ - below 40: dropped silently.
+ - End with a verdict: ship / fix-then-ship / rework.
+ - Every item in the report, and every issue one names, reads in the cold-reader line (`docs/project-state.md` § Restating an issue).
+ - Done-criteria: every ≥80 finding names its file:line and a concrete fix; no lens output pasted raw into chat.
+ - A finding that gets FILED rather than fixed passes the filing litmus test first: *would closing an open issue automatically mean this is done too?*
+ - Yes: it attaches to that issue, never as a sibling issue.
+ - Polish-grade findings batch as checklist lines onto the surface's rolling `polish: <surface>` issue.
+ - The rules: `docs/project-state.md` § How big is one issue.
## 5. Marker (feeds the commit gate)
- After the report, record that review ran, from inside the repo under review. The `safety/commit-gate` hook checks this marker before allowing a code commit, and the script names it through the same helper the hook reads it with, so the two can never drift:
+ - After the report, record that review ran, from inside the repo under review.
+ - The `safety/commit-gate` hook checks this marker before it allows a code commit. The script names the marker through the same helper the hook reads it with, so the two cannot drift.
```sh
bash "${CLAUDE_PLUGIN_ROOT:-$HOME/.claude/workkit/..}/scripts/review-marker.sh"
```
- (`CLAUDE_PLUGIN_ROOT` is set only inside hook commands, so the fallback is the plugin root reached through the engine's stable address: `~/.claude/workkit` is the engine folder INSIDE the plugin, and its parent is the plugin itself.)
+ - `CLAUDE_PLUGIN_ROOT` is set only inside hook commands. The fallback reaches the plugin root through the engine's stable address: `~/.claude/workkit` is the engine folder INSIDE the plugin, and its parent is the plugin itself.
## Gotchas
- - The inline return IS the convention. The reviewer, `workkit:scout`, and `workkit:verifier` toolsets have no Write anyway. A report FILE is the explicit-ask exception: name the path in the brief only when the output is a large artifact meant to be read selectively, and never ask a lens without Write for one.
- - Do not re-run a full panel over edits that merely implement findings the scorer already judged this session. That reviews the review's own output. A light verification pass ("does each edit implement its finding without contradictions?") is the honest check. The `workkit:ship` panel is the one exception: it runs full over the whole ship diff every time (its step 3.2b).
+ - The inline return IS the convention. The reviewer, `workkit:scout`, and `workkit:verifier` toolsets have no Write anyway.
+ - A report FILE is the explicit-ask exception. Name its path in the brief only for a large artifact meant to be read selectively, and never ask a lens without Write for one.
+ - Do not re-run a full panel over edits that merely implement findings the scorer already judged this session: that reviews the review's own output.
+ - The honest check is a light verification pass: "does each edit implement its finding without contradictions?"
+ - The one exception is the `workkit:ship` panel: it runs full over the whole ship diff every time (its step 3.2b).