diff --git a/.claude/commands/finish.md b/.claude/commands/finish.md index 6a69867..c3a4cad 100644 --- a/.claude/commands/finish.md +++ b/.claude/commands/finish.md @@ -20,9 +20,10 @@ Verify: ## Step 2: Parse the body section structure -The issue body should have these seven sections, in this order, with heading names preserved exactly: +The issue body should have these eight sections, in this order, with heading names preserved exactly: - `## Context` — orientation for where this issue sits in the cascade +- `## Assumptions` — explicit calibration surface for ambiguity the rough-in author filled in (per Addy Osmani's "good spec for AI agents" pattern); each gap tagged `[ASSUMPTION:]` so plan mode can enumerate them. Valid contents include `- None — all parameters explicit from the framing intent and acceptance criteria.` when no assumptions were made; the section must be present even when empty. - `## Implementation` — the load-bearing section you will use as the primary plan-mode anchor - `## Acceptance criteria` — observable, verifiable outcomes the work must satisfy - `## Test plan` — named tests the implementer writes red-first; one per acceptance criterion for logic-regime modules (see `.claude/rules/testing.md` for regime classification) @@ -59,6 +60,7 @@ This isn't comprehensive — Claude Code can't detect every in-progress state. B Construct a plan-mode prompt using the issue body. The primary anchor is the `## Implementation` section. The other sections are supporting context: - `## Context` tells plan mode why this issue exists and where it sits in the cascade +- `## Assumptions` lists `[ASSUMPTION:]`-tagged calibration points the rough-in author surfaced. Plan mode **must enumerate each tagged item** in the plan output as a "Confirm or correct" line so the user can revise during plan iteration. **Do not silently resolve assumptions at plan time** — even when plan mode has a strong default, surface it so the user can push back. Resolved assumptions (those the user confirms or corrects) inline into the plan's relevant Implementation step rather than persisting as a separate "Resolved" subsection. If the section says `- None — ...` or contains no `[ASSUMPTION:]` lines, proceed silently. - `## Acceptance criteria` become the contract the plan must satisfy - `## Test plan` names the tests the plan must scaffold red-first (Step 6) before implementing — these are the executable form of the acceptance criteria - `## Done signal` is the verification command the plan must produce as its final step @@ -113,13 +115,13 @@ Once the implementation is complete and `mise run check` passes: **Quick summary of the rubric for orientation** (the rules file is authoritative when in doubt): - - **Apply** — behavior-preserving fix with concrete evidence of need: defects with reproducible failing path, factually wrong comments / rot, dead code / unused imports, existing-doc clarity improvements, defensive additions with concrete evidence, local-symbol renames when fact-based, missing docstrings on entirely-undocumented surfaces. One focused commit per finding with a Conventional Commits message that names it. + - **Apply** — behavior-preserving fix with concrete evidence of need (defects with reproducible failing path, factually wrong comments / rot, dead code / unused imports, existing-doc clarity improvements, defensive additions with concrete evidence, local-symbol renames when fact-based, missing docstrings on entirely-undocumented surfaces) **OR low-risk small changes** (≤ ~15 LOC, confined, no design call required, plausibly an improvement — typo fixes, redundant assertion drops, inlining one-shot helpers, extracting constants the agent already named, simplifying obvious-once-named expressions). One focused commit per finding with a Conventional Commits message that names it. - **Apply with care** — execute as its own commit but flag in the hand-off summary: cross-file refactors >~50 LOC, new module outside named files, materially altering changes, non-obvious correctness fixes. - - **Surface** — do not apply, note with agent's rationale verbatim: stylistic disagreement, structural refactors, speculative defensive guards (no concrete failing scenario), doc *expansions* of existing-but-thin sections, rule-of-three not hit on suggested abstractions, alternative approaches the agent prefers but the existing one is also fine. + - **Surface** — do not apply, note with agent's rationale verbatim. **Genuine design or taste calls only**: structural refactors that change surface area, speculative defensive guards (no concrete failing scenario), doc *expansions* of existing-but-thin sections, rule-of-three not hit on suggested abstractions, alternative approaches the agent prefers but the existing one is also fine. **NOT Surface**: small low-risk improvements (those go to Apply); style nits without strong rationale (those go to Reject with a one-line dismissal — surfacing them inflates the user's review surface). - **Defer** — conflicts with an ADR or out-of-scope. - - **Reject** — agent factually misunderstood. + - **Reject** — agent factually misunderstood, OR style nit suggested without strong rationale (one-line dismissal is enough). - **When uncertain, Surface — don't Apply.** The Apply gate is **behavior-preservation + concrete evidence**, not "the agent's reasoning is sound." Speculative findings without a demonstrated path go to Surface. Auto-applying speculative changes trains future humans (and agents) to ignore the bot. + **When uncertain about a small low-risk change, Apply. When uncertain about a design or taste call, Surface.** Atomic-commit history makes any wrong Apply call cheap to revert during draft review, and the user-side cost of triaging Surface items routinely exceeds applying-and-showing the diff. Reserve Surface for genuine taste calls and design decisions where reasonable people disagree. **Medium-confidence findings follow the same triage as high-confidence findings** — confidence is about whether the finding is real, not whether to apply it. **Don't bias away from being defensive**: defensive hardening (race guards, missing cleanups, future-edit foot-guns) is a correctness concern even when the test passes today — those go in Apply, not Surface. **Pre-filter generated files, lock files, vendored deps, and build artifacts** before the agents read them — see `pr-review.md` § Pre-filters. diff --git a/.claude/rules/pr-review.md b/.claude/rules/pr-review.md index d7f1fbc..aa55173 100644 --- a/.claude/rules/pr-review.md +++ b/.claude/rules/pr-review.md @@ -41,13 +41,17 @@ Every finding goes into exactly one of four classes. The Apply class is narrow a | Class | Action | Triggers (see § Apply/Surface calibration below for category-by-category) | |---|---|---| -| **Apply** | Execute the fix; commit as its own focused commit on the branch with a Conventional Commits message that names the finding. | Behavior-preserving fix with concrete evidence of need. Includes: defects with reproducible failing path; factually wrong comments / rot; dead code / unused imports; existing-doc improvements (clarity, precision, accuracy); defensive additions with concrete evidence; local-symbol renames when fact-based; missing docstrings on entirely-undocumented surfaces. | +| **Apply** | Execute the fix; commit as its own focused commit on the branch with a Conventional Commits message that names the finding. | Behavior-preserving fix with concrete evidence of need. Includes: defects with reproducible failing path; factually wrong comments / rot; dead code / unused imports; existing-doc improvements (clarity, precision, accuracy); defensive additions with concrete evidence; local-symbol renames when fact-based; missing docstrings on entirely-undocumented surfaces. **Also includes low-risk small changes**: ≤ ~15 LOC delta, confined to one file or symbol, no design call required, plausibly an improvement per the agent's rationale (e.g. typo fixes, redundant assertion drops, inlining a one-shot helper, extracting a constant the agent already named, simplifying an obvious-once-named expression). The user-side cost of triaging 5 small Surface items routinely exceeds applying them and presenting the diff. | | **Apply with care** | Execute as its own focused commit, but flag in the hand-off summary so the user knows to scrutinize. | Cross-file refactors >~50 LOC; introducing a new module/file outside the issue's named files; changes that materially alter test or implementation strategy; correctness fix whose implementation is non-obvious. | -| **Surface** | Do **not** apply. Note in hand-off summary with the agent's rationale verbatim. | Stylistic disagreement; structural refactors that change surface area; speculative defensive guards (no concrete failing scenario); doc *expansions* of existing-but-thin sections; rule-of-three not hit on suggested abstractions; alternative implementation approaches the agent prefers but the existing one is also fine. | +| **Surface** | Do **not** apply. Note in hand-off summary with the agent's rationale verbatim. | **Genuine design or taste calls only**: stylistic disagreement where the existing form is also fine; structural refactors that change surface area; speculative defensive guards (no concrete failing scenario); doc *expansions* of existing-but-thin sections; suggested abstractions where the existing duplication is small (rule-of-three not yet hit); alternative implementation approaches the agent prefers but the existing one is also fine. **NOT Surface**: small low-risk improvements (those go to Apply); style nits the agent suggested without strong rationale (those go to Reject with a one-line dismissal — don't surface them with verbatim rationale, that just inflates the user's review surface). | | **Defer** | Note in hand-off summary; do not modify the issue body or open follow-up issues without user direction. | Conflicts with an ADR or with the issue's intentional design; finding is explicitly listed under the issue's `## Out of scope`. | | **Reject** | Note in hand-off summary with one-line justification. | Agent factually misunderstood the codebase or the spec; finding contradicts project rules or an existing ADR. | -The Apply gate is **behavior-preservation + concrete evidence**, adapted from Sourcery's auto-apply split (refactorings that don't change behavior auto-apply; suggestions that may change behavior never auto-apply). The "concrete evidence" half adopts PhotoStructure's "prove it or discard it" rule — speculative findings without a demonstrated failing path go to Surface. +The Apply gate is **behavior-preservation + concrete evidence**, adapted from Sourcery's auto-apply split (refactorings that don't change behavior auto-apply; suggestions that may change behavior never auto-apply). The "concrete evidence" half adopts PhotoStructure's "prove it or discard it" rule — speculative findings without a demonstrated failing path go to Surface. The Apply class also extends to low-risk small changes that are plausibly improvements (≤ ~15 LOC, confined, no design call) — the calibration learning here is that user-side triage cost on small Surface items routinely exceeds the cost of applying-and-showing the diff, and atomic-commit history makes any wrong call cheap to revert. + +**When uncertain about a small low-risk change, Apply. When uncertain about a design or taste call, Surface.** The split: if the agent's suggestion is a small, confined, no-design-call edit and the rationale is plausible, prefer Apply — atomic-commit history makes the fix easy to revert and the user-side cost of triaging Surface items routinely exceeds applying-and-showing. Reserve Surface for genuine taste calls and design decisions where reasonable people disagree. **Medium-confidence findings from review agents follow the same triage as high-confidence findings** — confidence is about whether the finding is real, not whether to apply it. Don't default mediums to Surface just because they're medium-confidence; that converts the bot's "I'm not sure this is real" into the user's "now you have to triage 5 items." The Apply bar is "would a future maintainer be wrong without this fix, OR is the fix small + low-risk + plausibly an improvement?" — defects, ADR/standards/logging-contract violations, defensive hardening, factual rot, and small low-risk improvements meet that bar; genuine design or taste calls do not. + +**Don't bias away from being defensive.** Defensive hardening (e.g., adding a race guard, a missing cleanup, a setup that prevents a future edit from silently breaking a test) is a correctness concern even when the test passes today — those go in Apply, not Surface. Conservatism here means *defending the codebase against future drift*, not *being conservative about applying defenses*. ## Apply / Surface calibration @@ -93,7 +97,8 @@ The calibration that distinguishes this rubric from a generic "Apply correctness | Sub-category | Class | Why | |---|---|---| | Formatter / linter violations not auto-caught by `mise run check` | **Apply** | If the project's check task should have caught it, fixing the lint is part of finishing. | -| Stylistic preference (one-liner vs multi-line, function vs method, etc.) | **Surface** | Genuine taste; not the bot's call. | +| Stylistic preference with a substantive rationale (e.g., consistency with a project pattern in `STANDARDS.md`, readability impact the agent can articulate) | **Surface** | Genuine taste call; human's judgment. | +| Stylistic nit the agent suggested without a strong rationale (one-liner vs multi-line, function vs method, naming flavor) | **Reject** | One-line dismissal in the hand-off. Surfacing these with verbatim rationale just inflates the user's review surface — that's the exact pattern that trains humans to ignore the bot. | ## What NOT to flag — the exclusion list diff --git a/.claude/skills/rough-in/SKILL.md b/.claude/skills/rough-in/SKILL.md index 9f426e9..090d403 100644 --- a/.claude/skills/rough-in/SKILL.md +++ b/.claude/skills/rough-in/SKILL.md @@ -56,7 +56,7 @@ Rough-in's full flow (target identification → context gathering → research **Detect-then-confirm** at session start: propose a mode in one sentence based on the user's opening message and any prior rough-in runs visible in `README.md` index. Let the user override in one word. If the opening message gives no signal, default to **standard mode** for first-time users. -**What rough-in must NOT skip even in light mode**: reading all required inputs in full, the deferred meta-issues pre-flight check, at least one HITL gate on the final sub-sub-issue set before commit, and the planning-backend commit atomic transition discipline. +**What rough-in must NOT skip even in light mode**: reading all required inputs in full, the deferred meta-issues pre-flight check, at least one HITL gate on the final sub-sub-issue set before commit, the planning-backend commit atomic transition discipline, and **framing-invited judgment calls** — if the framing's milestone notes explicitly invite a rough-in-time decision (e.g. "may compress 1+2", "rough-in shapes the final boundaries", "decision deferred to rough-in"), the issue plan gate is non-collapsible regardless of rigor mode. These are the calls light mode is wrong about by default — the framing flagged that judgment lives at this layer. ## Step 1 — Inheritance and pre-flight checks @@ -129,6 +129,8 @@ For each approved issue plan entry, draft the full sub-sub-issue spec using the 7. **Dependencies** — explicit list of prior R-issues that must be complete before this one can start 8. **Done signal** — a specific, observable outcome that means this issue is done. Usually the same as the acceptance criteria's top-line check but stated as a verification command the user can run. +**Section names + ordering on disk are project-specific.** The 8 properties above are conceptual — *what* every spec must express, not *what headings* to use. The literal heading names and ordering are parser-driven by the project's executor (`/finish` slash command or equivalent — see the project's `cbk-conventions.md` for the canonical list). Before drafting spec bodies, check the executor's body-parser section to confirm the required headings; mismatched headings will fail the parser even when the content is correct. Concepts like "Technical detail" and "Plan-mode prompt" typically merge into a single `## Implementation` section in the executor's template; "Intent" typically maps to `## Context`. Express the 8 properties in whatever heading shape the project's executor expects. + **The Claude Code plan-mode prompt is the most important part of each spec.** It's the thing that makes rough-in valuable over just "framing plus a wish list." A plan-mode prompt has these properties: - Written in second person ("Implement the Verifier trait...") so Claude Code reads it as an instruction