pre-commit-review

name	pre-commit-review
description	ADVISORY pre-commit review that orchestrates parallel single-obsession rule hunters, an over-abstraction skeptic, and a comment critic against the diff. Spawns read-only agents (rule-hunter, overabstraction-skeptic, comment-critic); NEVER edits code. Invoked by @linter-driven-development (Phase 4), by @refactoring (after pattern application), or manually for standalone code review. Categorizes findings as Bugs, New Practice (when in Rome), Design Debt, Readability Debt, or Polish Opportunities. Does NOT block commits.
allowed-tools	["Read","Grep","Bash","Agent"]

Verify a finished diff against the plugin's rules (R1–R12) with evidence, by orchestrating parallel single-obsession `rule-hunter` agents, one `overabstraction-skeptic`, and one `comment-critic`. Pure orchestration and reporting: this skill may spawn agents but never edits code, never fixes findings, and never blocks a commit. Rule knowledge lives once in `../../rules/`; agents receive it as spawn-time payload — they do not invoke skills. Run pre-commit, per completed vertical slice — NEVER mid-implementation. GREEN-step TDD code is supposed to look under-designed; reviewing it produces false positives. The per-cycle detection greps in the REFACTOR step (see @refactoring) are the mid-implementation net; this pass is the verification net on finished work. - **Diff scope**: staged changes by default (`git diff --cached --name-only -- '*.go'`), or an explicit file list / diff range from the caller. - **Mode**: `FULL` (first run) or `INCREMENTAL` (re-run after fixes — requires the previous report's findings).

<step_1_grep_prefilter> In-context, cheap — no agents yet. For each rule below, read its rule file's Falsifying questions section and run the detection commands there against the diff scope (changed files only). The commands live in the rule files; never restate them here. A rule with zero hits is skipped — no hunter spawned for it.

Rule	File	Hunt focus
R1	../../rules/R1-primitive-obsession.md	domain concepts as raw primitives; sentinel returns; ceremony wrappers (inverse)
R2	../../rules/R2-self-validating-types.md	invalid-state construction; defensive re-checks; nil as a value
R3	../../rules/R3-storifying.md	mixed abstraction levels; comments naming unextracted blocks
R4	../../rules/R4-helper-placement.md	helper visibility/placement off the placement ladder
R5	../../rules/R5-vertical-slice.md	horizontal layering; role-named packages
R6	../../rules/R6-test-only-interfaces.md	interfaces whose only second implementer is a test double
R7	../../rules/R7-test-placement.md	internal test packages; wantErr conditionals; wrong-rung tests; sleeps
R8	../../rules/R8-no-globals.md	package-level state; context.Background() in library code
R9	../../rules/R9-repo-brain.md	orphan docs; broken doc edges (both directions); WHAT-comments on exported API; unwired root
R10	../../rules/R10-concurrency-safety.md	goroutines without exit paths or owners; unguarded shared-state writes; production sleeps; decorative mutexes
R11	../../rules/R11-conditional-dispatch.md	one discriminator switched in ≥2 places; type switches in domain logic; unknown-kind defaults away from the boundary; flag arguments; unearned dispatch abstractions (inverse)
R12	../../rules/R12-mutation-discipline.md	internal slices/maps returned by reference; constructors aliasing caller collections; query/modifier hybrids; setters around validating constructors; ceremony copies (inverse)

Also in-context: a new //nolint directive or .golangci.yaml exclusion in the diff is itself a finding — the change must justify, with evidence, that the rule genuinely does not apply.

Also in-context — the when-in-Rome check: a diff must arrive in the host repo's existing style, not import a new one. Flag anything the diff introduces that the repo does not already use: a new test mechanism (golden files, snapshot testing, a new assertion library), a new dependency in go.mod, a new tool or config file, edits to repo-level convention files (CLAUDE.md, coding standards, lint config) bundled into a feature diff, or a directory layout unlike its siblings. Detection is comparative: for each candidate, grep the repo outside the diff for prior use — zero prior use is the finding. These are not style crimes; they are adoption decisions that belong to the repo owner. The fix is a discussion or a separate PR, never silent inclusion. </step_1_grep_prefilter>

<step_2_spawn_hunters> For every rule with pre-filter hits, spawn one rule-hunter agent — all hunters in a single message, in parallel. Each spawn prompt MUST contain:

1. The rule file's FULL content, pasted — the hunter's entire rulebook and single obsession. Never a path reference alone; never more than one rule per hunter.
2. The diff scope — the changed-file list or git diff range.
3. That rule's pre-filter hits — as starting leads (the hunter re-runs the detection commands itself; leads are a starting point, not a limit).

If the rule cites a case file by plugin-relative path (e.g. ../examples/*.md), resolve it to an absolute path and include that path in the spawn prompt — the hunter runs in the reviewed project's cwd and cannot resolve plugin-relative paths on its own.

Each hunter returns one block per finding: rule | file:line | evidence (falsifying-question answers) | proposed fix pattern | effort (S/M/L) plus a final tally line (R<N>: <M> finding(s) or a hunted-clean line). </step_2_spawn_hunters>

<step_3_skeptic_pass> Collect ALL type/package-extraction findings — every R1/R2/R4 "create a type/package" proposal, R10 "Extract Synchronized Owner" proposals, and R11 "Interface Dispatch" / "Strategy Map" proposals — and spawn one overabstraction-skeptic. Its spawn prompt MUST contain:

1. The extraction findings under review — the hunter blocks pasted verbatim.
2. Payload: the Juiciness scoring and The over-abstraction trap sections of ../../rules/R1-primitive-obsession.md, pasted.
3. Payload: the FULL content of ../../examples/overabstraction-cidr.md, pasted.

Verdicts per finding: CONFIRMED (score + verified evidence) or REFUTED (score 0–1 + reason) → cheaper alternative. A refuted proposal does not ship; when its cheaper alternative (better naming, private fields + accessors, or R11's Keep the Single Exhaustive Switch) is still worth doing, report the alternative as 🟢 Polish. When R11 dispatch proposals are under review, additionally paste the FULL content of both R11 case files — ../../examples/anti-if-dispatch.md (Move 3 is the juiciness rejection: the switch stays, goes exhaustive) and ../../examples/switch-to-polymorphism.md (the dependency-direction rejection: the move is unavailable when the consumer owns the output format; the switch shrinks to pure dispatch). Only findings the skeptic cannot kill ship as extraction findings. Non-extraction findings (R3, R5–R9, and R1/R2/R10/R11 findings that propose no new type) skip the skeptic and go straight to the report — R9 findings (orphans, broken edges, WHAT-comments, unwired root) propose no type extractions. </step_3_skeptic_pass>

<step_3b_comment_critic> When the diff contains comment lines — prefilter: git diff --cached -- '*.go' | grep -E '^\+.*//' | grep -vE '//(go:|nolint| Output:)' (any hit qualifies; directives don't count) — spawn one comment-critic alongside the skeptic (same message when both run). Its spawn prompt MUST contain:

1. Payload: R9's Comment policy section (../../rules/R9-repo-brain.md, Design guidance) pasted verbatim — the Comment Value Toolbox kinds, the three-test standard, the tier table, budget accounting, and the visibility default.
2. Payload: the Comment Value Toolbox catalog section of ../documentation/reference.md (resolve to an absolute path) pasted verbatim.
3. The absolute path to ../../examples/private-comment-noise.md — the critic reads it when judging comments on unexported symbols.
4. The diff scope.

It judges every comment in the diff (godoc, in-body, test) against the three-test standard and returns per-comment verdicts (KEEP / TRIM / REWRITE / DELETE, or DELETE → route R3 for in-body extraction candidates) with evidence and proposed replacement text. Non-KEEP verdicts land in the report as 🟡 Readability Debt; DELETE → route R3 verdicts merge with any R3 hunter findings on the same lines (one finding, not two). The critic is advisory like everything else — accepted verdicts are fixed by @documentation (the rung-1 fixer), except R3 routes, which go to @refactoring. </step_3b_comment_critic>

<step_4_merged_report> Merge surviving findings into one report.

Cluster pass (before categorizing): group surviving findings by shared anchor — the same type, field/discriminator, or function named in ≥2 findings from different rules. Each hunter is single-obsession and blind to the others, so independent convergence on one anchor is evidence that a domain concept is missing there — the cluster is a juiciness scorecard that filled itself in (R1 hunter sees the raw primitive, R11 the duplicated switch, R2 the ownerless validation: one disease, four jurisdictions). Render each cluster as a first-class entry above the categories:

🔗 CLUSTER: Alert.Channel (4 findings: R1, R11, R2, R7)
   Hypothesis: missing domain concept — a Channel type wants to exist
   Routing: design-first — @code-designing (cluster-scoped), then @refactoring
   implements; do NOT fix members independently (partial fixes undo each other)

Member findings still appear under their categories below, tagged [cluster: <anchor>]. Clustering is reporting — this skill still never edits and never invokes fix skills; the caller routes.

Category mapping:

• 🐛 Bugs — will fail at runtime regardless of rule (nil returned as a value, cancellation swallowed by context.Background(), R10 goroutine leaks and unguarded concurrent writes): fix immediately.
• 🟠 New Practice (when in Rome) — the when-in-Rome check's findings: a mechanism, dependency, framework, or convention the host repo does not already use, introduced without discussion. Advisory like everything else, but flag it loudly: reviewers reject these threads hardest, and the fix (owner buy-in or a separate PR) is cheap before pushing and expensive after.
• 🔴 Design Debt — R1, R2, R4, R6, R7, R8, R10's non-crash findings (production sleeps, fire-and-forget ownership, mutex placement), R11 (duplicated discriminators, boundary leaks), R12 (leaked mutable internals, unvalidated setters), and R5 (advisory — never blocks; the user may have valid reasons): fix before commit recommended.
• 🟡 Readability Debt — R3, R9, unclear naming, and the comment-critic's non-KEEP verdicts (trash or over-budget or hard-to-read comments): improves maintainability.
• 🟢 Polish — minor idiomatic improvements, the skeptic's cheaper alternatives.

Every finding carries evidence — file:line plus the falsifying-question answer or command output — never a bare verdict. Effort carries over from the hunter (S/M/L). Fix routing is each rule file's Fix pattern section; cite it, don't restate it. Issues noticed outside the diff scope go in a BROADER CONTEXT section, not as findings. </step_4_merged_report>

**FULL (first run):** pre-filter all twelve rules over the whole diff scope; report every surviving finding.

INCREMENTAL (re-run after fixes): diff scope = only files changed since the last review. Run steps 1–3 on that scope, compare against the previous findings, and report a delta: ✅ Fixed (previous finding no longer reproducible — re-run its detection command to confirm), ⚠️ Remaining (still evidenced), 🆕 New (introduced by the fixes). Use after @refactoring applies fixes or whenever the caller iterates.

<report_example>

📊 CODE REVIEW REPORT
Scope: user/service.go, user/auth.go (+ tests) · Mode: FULL
Hunters: R1 (2 leads), R2 (1), R3 (1) · R4–R8 skipped (no pre-filter hits)
Skeptic: 1 extraction CONFIRMED, 1 REFUTED (score 1 → rename instead)
Critic: 14 comments reviewed — 11 KEEP · 2 REWRITE · 1 DELETE

🔴 DESIGN DEBT
user/service.go:67 | session token travels as raw string; emptiness check inline
  (R1 Q1: yes; Q2: same predicate at user/auth.go:41 — two owners) | Replace
  Primitive with Domain Type: SessionToken — skeptic CONFIRMED (score 5) | M
user/auth.go:34 | Authenticator.HashCost exported; methods re-check its range
  (R2 Q1: literal construction possible; Q2: re-check at auth.go:52) | validating
  constructor NewAuthenticator | S

🟡 READABILITY DEBT
user/auth.go:89 | Authenticate() mixes auth flow with bcrypt byte handling
  (R3 Q1: two abstraction levels in one body) | Extract Step: comparePassword | S
user/service.go:15 | godoc restates the name ("UserService provides user services")
  (critic: toolbox-value floor — no toolbox item delivered) | REWRITE → wider
  context: "Every user mutation flows through this service — auth, quota, and
  audit hooks attach here." | S

🟢 POLISH
user/auth.go:12 | ComparePasswordWithHash → PasswordMatches — skeptic's cheaper
  alternative to REFUTED PasswordHash wrapper (score 1: only method unwraps) | S

📝 BROADER CONTEXT
user/service.go:23 — email still a raw string (outside diff scope; same R1 pattern).

Caller decides: commit as-is · fix 🔴 first · fix all. Findings are advisory.

</report_example>

This skill MUST NOT: - Edit code, fix findings, or invoke fix skills (@refactoring, @code-designing, @testing) - Run the linter or tests — the caller does (see @linter-driven-development) - Block commits — every finding is advisory; the caller decides what to fix - Restate rule content — rules live once in `../../rules/`; paste them as spawn payload and cite them in findings - Spawn anything other than `rule-hunter`, `overabstraction-skeptic`, and `comment-critic`

<who_invokes>

1. @linter-driven-development — Phase 4, pre-commit / per completed vertical slice
2. @refactoring — after applying patterns, to validate design quality (INCREMENTAL)
3. User — manual standalone review before commit </who_invokes>