diff --git a/CLAUDE.md b/CLAUDE.md index f9403a3..a3ddd47 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,7 +81,7 @@ Before answering architecture questions or starting non-trivial work in an unfam ### 1. Plan Mode Default - Enter plan mode for ANY non-trivial task (3+ steps or architectural decisions). If something goes sideways, STOP and re-plan. -- **Plan format**: atomic tasks with explicit file paths, each independently verifiable. State what changes, where, and how to prove it works. +- **Plan format**: atomic tasks with explicit file paths, each independently verifiable. State what changes, where, and how to prove it works. For any item whose necessity isn't self-evident, also state **what breaks without it** — a task that can't answer that is a task to cut, and the answer becomes the deletion probe the reviewer runs later. - **Plan the smallest thing that satisfies the request.** Over-engineering is far cheaper to prevent here than to prune later, and the plan is where speculative machinery gets legitimised: once "add a `--strategy` flag" is a numbered task, it reads as a requirement rather than a guess. Name the caller for every parameter, option, and abstraction the plan introduces — if that caller is hypothetical, cut the item. Don't plan extensibility nobody asked for, and don't plan a helper you'd write exactly one call to. Detail in `coding-standards.md` ("Simplicity & Scope (YAGNI)"). - **User checkpoint**: for multi-commit plans, cross-cutting refactors, or anything touching shared infrastructure (`infra-ops.md`), share the plan before implementing. - **Plan review loop — MANDATORY gate before implementation starts**: review the plan, fix every issue, re-review. Repeat until **three consecutive passes find nothing** (any finding restarts the count at zero). Do NOT create the §1b worktree, enter ExitPlanMode, or write code until this passes. Each pass covers: the six review dimensions (see below); Reuse (§1a); scope discipline (only what was asked?); blast radius (callers, tests, migrations, downstream consumers all listed?); unknowns (verify "verify-first" items NOW, not at implementation time). Per-pass findings go in the plan as a short "review pass N" note. @@ -94,7 +94,7 @@ Before answering architecture questions or starting non-trivial work in an unfam - **Security**: injection, auth bypass, secrets exposure, OWASP top 10, input validation at boundaries? - **Bugs**: race conditions, null derefs, edge cases, error-handling gaps, resource leaks? - **Duplication**: re-invents anything already in the project? If yes, reuse/refactor per §1a. -- **Over-engineering**: is every parameter set by a real caller, every abstraction used by more than one consumer, every guard protecting a reachable state, every comment earning its line? Prune what fails, per `coding-standards.md` ("Simplicity & Scope (YAGNI)"), and remove it safely per that file's "Verifying a Refactor". Review this dimension **adversarially**: the author's local justification for a piece of machinery almost always holds up, so ask instead what the calling system actually does and what would break if the machinery were deleted. Correct, well-tested code guarding an unreachable state still comes out. +- **Over-engineering**: is every parameter set by a real caller, every abstraction used by more than one consumer, every guard protecting a reachable state, every comment earning its line? Prune what fails, per `coding-standards.md` ("Simplicity & Scope (YAGNI)"), and remove it safely per that file's "Verifying a Refactor". Review this dimension **adversarially**: the author's local justification for a piece of machinery almost always holds up, so ask instead what the calling system actually does and what would break if the machinery were deleted. Correct, well-tested code guarding an unreachable state still comes out. Where the answer is genuinely arguable, **don't argue it — run a deletion probe** (`coding-standards.md`, "Deletion probes"): delete the candidate, run the verification, and let the result decide. A "nothing broke" that turns out to be a coverage gap rather than dead code is the most valuable finding this dimension produces. ### 1a. Reuse Before Writing — Avoid Duplication @@ -123,7 +123,7 @@ Non-trivial work happens in a dedicated git worktree branched off the current br Every code change the implementer (Sonnet for simpler changes, Opus for non-trivial code, per §2) produces during the implementation phase is reviewed locally by Opus before it counts as done. Local analog of the post-PR CodeRabbit loop (`git-workflow.md`): catch issues in the worktree before the diff is pushed. Does NOT replace the §1 post-implementation review or the §1b merge gate; it runs inside the implementation phase, upstream of both. The loop: -1. **The implementer (Sonnet or Opus per §2) implements** an atomic task (or one logically complete chunk) per the approved plan. Write the plan's task, not a generalised version of it: no parameter without a caller in this changeset, no abstraction with one consumer, no guard against a state the callers cannot produce, no comment restating the line below it. If the task as written seems to need machinery the plan didn't call for, that is a signal to re-plan rather than to improvise it (§1). +1. **The implementer (Sonnet or Opus per §2) implements** an atomic task (or one logically complete chunk) per the approved plan. Write the plan's task, not a generalised version of it: no parameter without a caller in this changeset, no abstraction with one consumer, no guard against a state the callers cannot produce, no comment restating the line below it. If the task as written seems to need machinery the plan didn't call for, that is a signal to re-plan rather than to improvise it (§1). Before handing the diff to review, **probe your own additions** (`coding-standards.md`, "Deletion probes"): delete each piece whose necessity you couldn't state in one sentence and see whether anything actually fails. Cheaper to find here than in review, and what survives arrives with evidence attached. 2. **Opus reviews the diff locally** across the six review dimensions plus Reuse (§1a) and scope discipline. Spawn a dedicated Opus reviewer subagent (set via `model`) to keep the implementer's context clean; escalate the review to Fable only for the hardest money-path / architecture calls where peak intelligence matters. Emit a concrete findings list (`file:line` + what's wrong + suggested fix), or an explicit "no actionable findings". 3. **The implementer addresses** every finding. Mechanical, decided fixes stay with the implementer; a finding needing a design call escalates that item to Opus (or Fable for a peak call, §2 carve-out), then the decided fix goes back to the implementer. 4. **Opus re-reviews** the updated diff. diff --git a/coding-standards.md b/coding-standards.md index 65926e9..2233b9c 100644 --- a/coding-standards.md +++ b/coding-standards.md @@ -73,6 +73,21 @@ A cleanup that changes behaviour is a bug wearing a tidy diff. "Delete aggressiv If you can't prove an edit is behaviour-preserving, don't make it. A small confident diff beats a large clever one. +### Deletion probes — test necessity, don't argue it + +Whether a piece of machinery is needed is an empirical question, so answer it empirically instead of by reasoning about the code. For each candidate: **delete it, run the full verification, and read the result.** + +- **Something fails** → it is load-bearing. Restore it, and record *what* failed: that failure is the evidence it earns its place, and it belongs in the PR description or the protected list. +- **Nothing fails** → exactly one of two things is true, and you must say which: + 1. the code was genuinely unnecessary — delete it for real; or + 2. **the verification is too weak to notice** — restore the code and report a coverage gap. + +**Never read "nothing broke" as automatic permission to delete.** If removing a guard that protects a real invariant breaks no test, the honest finding is "this guard is untested", not "this guard is unnecessary". Those cases are usually worth more than the deletions, because they point at the hole that let the risk in. + +Restore between probes so they can't interact, and probe one thing at a time — two simultaneous deletions that cancel out will read as a clean run. + +This is the cheap way to settle the arguments that stall a cleanup, and it cuts both directions: it deletes machinery whose justification was only ever theoretical, and it produces hard evidence for the pieces that survive. A parameter proven necessary by a failing build is no longer a matter of opinion. + ## Preferred Stack - **Language**: Go for new backend/CLI projects; TypeScript/Node for frontend, lightweight CLIs, or when the ecosystem fit is strong; match the existing language for additions to existing projects