refactor(claude): make /check spawn one Opus subagent

.claude/skills/check/SKILL.md

@@ -1,11 +1,11 @@
 ---
 name: check
-description: Daily-driver review of in-flight changes (or specified files) for reuse, simplification, efficiency, and high-confidence bugs. Flags code that duplicates existing helpers, overengineering, dead code, hot-path bloat, premature abstraction, real correctness issues, and unnecessary comments or tests. Default mode diffs the working tree and staged changes against the main branch. Pass paths (`/check src/foo.rs`) to review file state instead. Use as the fast pre-PR review pass. For a deeper whole-tree retrospective, use /audit.
+description: Daily-driver review of in-flight changes (or specified files) for reuse, simplification, efficiency, and high-confidence bugs. Spawns one Opus subagent to keep file reads off the main thread. Default mode diffs the working tree and staged changes against the main branch. Pass paths (`/check src/foo.rs`) to review file state instead. Use as the fast pre-PR review pass. For a deeper whole-tree retrospective, use /audit.
 ---
 
 # /check
 
-Pre-PR review pass over work-in-progress. Default runs against the current diff. Explicit paths switch to file-state review.
+Pre-PR review pass over work-in-progress. The main thread spawns one Opus subagent to do the analysis in isolation, then receives a structured finding list to present and apply.
 
 ## Invocation
 
@@ -19,41 +19,77 @@ Pre-PR review pass over work-in-progress. Default runs against the current diff.
 
 Paths win: if any are given, ignore the diff and review file state.
 
-## Gathering context
+## Step 1: Determine scope
 
-Diff mode:
+Diff mode (no paths):
 
 1. Resolve the base branch. Try `git symbolic-ref refs/remotes/origin/HEAD`, then `main`, then `master`. Ask the user if neither exists.
 2. With `--branch <name>`, use that base instead.
-3. With `--staged`, use `git diff --cached <base>`. Otherwise include working tree (`git diff <base>...HEAD` plus uncommitted).
+3. With `--staged`, scope to staged changes only (`git diff --cached <base>`). Otherwise include working tree (`git diff <base>...HEAD` plus uncommitted).
-4. Read each touched file fully, not just the hunks. Context outside the diff is needed to spot duplication and existing helpers.
+4. Enumerate touched files (`git diff --name-only ...`).
 
-File mode: read each passed path (recurse into directories, skip generated and vendored trees).
+File mode (paths passed): take the paths verbatim. The subagent recurses into directories.
 
-Always: read `CLAUDE.md` (root and any in touched dirs) and `TODO.md` / `BACKLOG.md` if present. Honor local conventions and do not re-raise deferred items.
+Do not read the in-scope source files in the main thread. The subagent owns review-time reads.
 
-## What to flag
+## Step 2: Spawn the review subagent
 
-### Reuse (look here first)
+One call to the `Agent` tool with:
+
+- `subagent_type: general-purpose`
+- `model: opus` (regardless of session model)
+- `description`: `"/check review pass"`
+- `prompt`: composed as `<scope block>\n\n<review instructions, verbatim from Step 3>`
+
+The scope block tells the subagent what to review:
+
+```
+# Scope
+
+Mode: <diff|file>
+Base branch: <name>           # diff mode only
+Staged only: <yes|no>         # diff mode only
+Touched files:                # diff mode: from `git diff --name-only`
+  - path/to/file.rs
+  - ...
+Paths to review:              # file mode
+  - src/foo.rs
+  - src/bar/
+Repo root: <absolute path>
+```
+
+## Step 3: Review instructions (passed verbatim to the subagent)
+
+You are reviewing the in-flight work (or specified paths) described in the scope block above. Surface concrete, high-confidence improvements before this work ships. Be useful, not exhaustive.
+
+### Context
+
+Read each in-scope file fully, not just the diff hunks. Reuse and efficiency findings need surrounding context. Also read `CLAUDE.md` at the repo root and in any touched subdirectories, plus `TODO.md` / `BACKLOG.md` if present. Honor local conventions. Do not re-raise anything already deferred.
+
+When writing finding text, do not use em-dashes, en-dashes, semicolons, or `--` as em-dash substitutes. Use periods or commas instead.
+
+### What to flag
+
+#### Reuse (look here first)
 
 - New helpers or inline logic that duplicates an existing utility in this codebase. Scan the file's directory and shared locations (`utils/`, `lib/`, `helpers/`, modules adjacent to the changed ones) before concluding nothing exists.
 - Reinvented standard library or framework primitives (manual path joining, ad-hoc env var parsing, custom type guards, hand-rolled string manipulation, bespoke deep-equals, etc.) where the language or an in-repo helper already covers it.
 - Inconsistent import paths for the same thing (some sites use a top-level re-export, others reach into deep paths). Pick the path the rest of the codebase uses.
-- For each duplication finding: name the existing helper with `file:line` so the fix is concrete, not abstract.
+- For each duplication finding, name the existing helper with `file:line` so the fix is concrete.
 
-### Overengineering
+#### Overengineering
 
 - New helpers, traits, generics, or indirection introduced for one caller. Inline.
 - Flags, parameters, or fields with only one realistic value or use site.
 - Layers added speculatively without a current second caller.
 
-### Dead code in the same change
+#### Dead code in the same change
 
 - Functions added but uncalled elsewhere in the diff.
 - Branches that cannot be reached given the rest of the change.
 - New types, fields, or variants that nothing constructs.
 
-### Bugs (high-confidence only)
+#### Bugs (high-confidence only)
 
 Flag only when the issue is clearly wrong on inspection. "This might fail under some inputs" is not a finding.
 
@@ -65,11 +101,11 @@ Flag only when the issue is clearly wrong on inspection. "This might fail under
 - Rust borrow or lifetime patterns that compile but violate the intended invariant (lifetimes too permissive, `Send`/`Sync` boundaries crossed by accident).
 - Resource leaks: opened but never closed, listeners attached but never removed on the failure path.
 
-### Efficiency
+#### Efficiency
 
 Focus on hot paths (per-render, per-request, per-frame, per-record) and on patterns that get worse with scale.
 
-- Redundant work: duplicate computations, repeated file reads, redundant API or network calls, N-plus-one query patterns (one query per item where one batch query would do).
+- Redundant work: duplicate computations, repeated file reads, redundant API or network calls, N+1 query patterns.
 - Independent operations run sequentially that could run in parallel.
 - New blocking work added to startup or per-render/per-request paths that did not have it before.
 - State updates inside polling loops or event handlers that fire even when nothing changed. Add change-detection so downstream consumers are not woken for no-ops. If a setter takes an updater callback, verify same-value returns are honored so callers' own no-op guards are not silently defeated.
@@ -77,36 +113,36 @@ Focus on hot paths (per-render, per-request, per-frame, per-record) and on patte
 - Unbounded data structures, missing cleanup, leaked event listeners or timers.
 - Operations broader than they need to be (reading entire files when only a portion is needed, loading all rows when filtering for one, scanning a whole collection to find a single item where an index already exists).
 
-### Premature flexibility
+#### Premature flexibility
 
 - `Option<T>` where the value is always `Some`.
 - Trait bounds or generic parameters wider than the actual use.
 - Enum variants for cases that do not yet exist.
 - `impl Trait` where the concrete type would read just as clearly.
 
-### Comments to delete
+#### Comments to delete
 
 - Narration of what the code does (`// increments counter`).
 - Section dividers (`// ---`, `// === FOO ===`).
 - References to the task or commit that prompted the change ("added for X", "used by Y", "fixes #123").
 - Stale docs where the code has moved past them.
 
-### Tests not pulling weight
+#### Tests not pulling weight
 
 - Tests that assert the language or framework's behavior rather than the new logic.
 - Tests duplicated across files with no extra coverage.
 - Fixtures wrapping a one-liner for a single test.
 - Mocked tests where an integration test already covers the same path.
 
-### Scope creep
+#### Scope creep
 
 - Unrelated formatting changes mixed in with functional changes.
 - Drive-by refactors in files outside the stated goal.
 - "While I'm here" cleanup that belongs in a separate commit.
 
-### Style-rule violations (from the user's global CLAUDE.md)
+#### Style-rule violations
 
-Flag these whenever they appear in the diff or files, no preamble required:
+Flag these whenever they appear in the diff or files:
 
 - Em-dashes, en-dashes, `--` as em-dash substitutes, and semicolons in prose (comments, docs, commit messages, PR descriptions).
 - Programming-culture acronyms (YAGNI, DRY, KISS, SOLID, TDD, BDD, DDD, MVP, NBO, TOCTOU) in any prose. Replace with what they actually mean.
@@ -116,7 +152,7 @@ Flag these whenever they appear in the diff or files, no preamble required:
 
 Fold in any project-local `CLAUDE.md` conventions on top of these.
 
-## What NOT to flag
+### What NOT to flag
 
 - Low-confidence speculation. "This might break under some inputs" is not a bug finding. "This could be faster in theory" is not an efficiency finding. If you cannot point at a concrete failure mode or a concrete cost, leave it out.
 - Issues a linter or formatter already catches.
@@ -125,41 +161,58 @@ Fold in any project-local `CLAUDE.md` conventions on top of these.
 - Items already in `TODO.md` / `BACKLOG.md`.
 - Performance theatre: micro-optimizations outside hot paths.
 
-## Reporting
+### Output format
 
-Single pass, all findings together. No tiered ceremony.
+Return a single markdown block with this exact structure. No preamble or commentary outside the block.
 
 ```
-## Review pass
+## Findings
 
-<N> findings on <scope>.
+<N> findings on <scope description>.
 
-1. path/to/file.rs:42-58 - <issue, one line>.
+1. path/to/file.rs:42-58
-   Fix: <one-line change>.
+   Issue: <one line, concrete>.
-2. path/to/file.rs:88 - <issue>.
+   Fix: <one line, specific enough that the orchestrator can apply without re-reading the file>.
-   Fix: <one-line change>.
 
-Apply all? "Go ahead", or tell me which to skip.
+2. path/to/file.rs:88
+   Issue: <one line>.
+   Fix: <one line>.
 ```
 
-If a finding touches public API or changes behavior beyond cleanup, surface it as **Needs discussion** with two options rather than dropping it in the auto-apply list.
+For findings that touch public API surface or change behavior beyond cleanup, replace the `Fix:` line with:
 
-If nothing is worth flagging, say "Nothing flagged" in one line and stop. Do not pad.
+```
+   Needs discussion: <reason>.
+   Option A: <option>.
+   Option B: <option>.
+   Tradeoff: <one line>.
+```
 
-## Applying
+If nothing is worth flagging, return exactly `Nothing flagged` and stop. Do not pad.
 
-After approval:
+End of review instructions.
 
-1. Apply only approved findings.
+## Step 4: Present findings
-2. Diff mode: fold edits into the working tree. Do not commit unless the user asks, since they are mid-PR.
+
-3. File mode (no in-flight diff): default to one commit per theme using Conventional Commits.
+Display the subagent's findings to the user as-is. If any are marked `Needs discussion`, present those after the auto-apply list as a separate block.
-4. Run the project gate (`scripts/prepare.sh`, `pnpm check`, `cargo test`, equivalent) when applying multiple substantive changes.
+
+End with: `Apply all? "Go ahead", or tell me which numbers to skip.`
+
+## Step 5: Apply approved findings
+
+Only after the user approves:
+
+1. Read each touched file once.
+2. Apply the approved findings with the `Edit` tool. The subagent's `Fix:` line should be specific enough to apply without ambiguity. If a fix is too vague, surface that and ask the user.
+3. Diff mode: do not commit. The user is mid-PR.
+4. File mode (no in-flight diff): default to one commit per theme using Conventional Commits.
+5. Run the project gate (`scripts/prepare.sh`, `pnpm check`, `cargo test`, equivalent) when applying multiple substantive changes.
 
 ## Do not
 
+- Read files under review in the main thread before Step 5. The subagent owns review-time reads.
 - Rewrite history. The user is preparing a PR. Do not squash or amend existing commits.
-- Auto-apply changes to public API surface. Lift those into **Needs discussion**.
+- Auto-apply changes to public API surface. Lift those into `Needs discussion`.
-- Touch files outside the diff (diff mode) or outside the passed paths (file mode).
+- Reference the check run in commit messages ("from /check", "review cleanup"). Describe the change.
-- Reference the check pass in commit messages ("from /check", "review cleanup"). Describe the change, not how it surfaced.
+- Surface a "bug" or "perf" finding the subagent was not confident about. The signal value comes from the user trusting flagged bugs are real.
-- Surface a "bug" or "perf" finding you are not confident about. The signal value of this skill comes from the user trusting flagged bugs are real. False alarms erode that fast.
+- Pad. If the subagent returns `Nothing flagged`, that is the right answer.
-- Pad. If the diff is clean, "Nothing flagged" is the right answer.