From e6f23d59e07e2229c087a09f1fdb96f137f47a3c Mon Sep 17 00:00:00 2001 From: Oscar Wallberg Date: Tue, 26 May 2026 22:57:15 +0200 Subject: [PATCH] refactor(claude): make /check spawn one Opus subagent --- .claude/skills/check/SKILL.md | 143 +++++++++++++++++++++++----------- 1 file changed, 98 insertions(+), 45 deletions(-) diff --git a/.claude/skills/check/SKILL.md b/.claude/skills/check/SKILL.md index ebd730b..5c6caaf 100644 --- a/.claude/skills/check/SKILL.md +++ b/.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 `, use that base instead. -3. With `--staged`, use `git diff --cached `. Otherwise include working tree (`git diff ...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. +3. With `--staged`, scope to staged changes only (`git diff --cached `). Otherwise include working tree (`git diff ...HEAD` plus uncommitted). +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 `\n\n` + +The scope block tells the subagent what to review: + +``` +# Scope + +Mode: +Base branch: # diff mode only +Staged only: # 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: +``` + +## 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` 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 - findings on . + findings on . -1. path/to/file.rs:42-58 - . - Fix: . -2. path/to/file.rs:88 - . - Fix: . +1. path/to/file.rs:42-58 + Issue: . + Fix: . -Apply all? "Go ahead", or tell me which to skip. +2. path/to/file.rs:88 + Issue: . + Fix: . ``` -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: . + Option A: