diff --git a/.claude/commands/speckit.reviewissue.md b/.claude/commands/speckit.reviewissue.md index 2df4bb2..c79eef0 100644 --- a/.claude/commands/speckit.reviewissue.md +++ b/.claude/commands/speckit.reviewissue.md @@ -84,6 +84,7 @@ Each gap (in either group) must: - be answerable with a short written response (not "go figure it out") - cite concrete evidence from the issue or codebase where relevant +- state its **consequence** — which of the two kinds below it is, and in one clause what a different answer would change - end with a **Recommendation:** line — your best judgement call with a one-sentence *Reason*. This is mandatory, not optional. The author should be able to read the recommendation and either accept it (record a short @@ -91,9 +92,28 @@ Each gap (in either group) must: gap without a recommendation forces the author to originate the answer from scratch, which is exactly the work this command is meant to front-load. -**Number gaps contiguously across both groups** (1, 2, 3, … not 1a, 1b). This keeps `/speckit.confirmissue` parsing simple and lets the author refer to questions by a single number in chat. +**Raise a gap only where the issue leaves something open.** A gap exists because the issue does not determine the answer — someone taking this into SDD would have to invent it or guess. Test every candidate against the issue as it stands: if the body, its acceptance criteria, its out-of-scope list, or an existing non-review comment (step 1) already settles the point, there is no gap, however squarely a category below invites one. A category you considered and found settled produces **nothing** — no gap, no placeholder, and no commentary bullet announcing that it is fine. -**Requirements gaps** — probe these before any technical question. Cover at least these categories when applicable: +Do **not** additionally ask whether the author would contest your recommendation — that is a judgement about a person rather than about the issue, and a wrong call settles a real decision silently. Every open point stays a numbered gap with its own answer slot, however confident the recommendation is. + +**The review is sized by what the issue leaves open, not by the number of categories below.** The category lists are a checklist of what to *consider*, never a quota to fill. An issue that commits to little should attract a visibly shorter review than one that leaves much open — so if a tightly-scoped issue is producing a long review, the sweep is likely manufacturing gaps; re-test each against the issue and drop the ones nothing is actually open in. + +**Consequence — every gap says what a different answer would change.** Each gap is exactly one of two kinds: + +- **Changes what gets built** — answering against the recommendation would change something the issue commits to building: a scenario, an acceptance criterion, the scope boundary, a user-visible behaviour. +- **Settles a detail** — what gets built is fixed either way, and the answer settles a detail within it: a name, a path, a value, where something is documented. + +Name the kind on the gap and say in one clause what would change. Where the call is genuinely borderline, take *Settles a detail* — an inflated marker costs the reader exactly the attention the marker exists to save. + +**Order the gaps, then number them.** Settle the order first; numbers are assigned to the ordered list, never the reverse: + +1. **By group** — all Requirements gaps, then all Technical gaps, because requirements are probed before any technical question. What is load-bearing here is the *grouping*, not the sequence: `/speckit.confirmissue` routes each folded decision by the heading its gap sat under (its step 4), so consequence ordering operates *within* a group and never moves a gap across the two. +2. **By consequence within the group** — every *Changes what gets built* gap comes before every *Settles a detail* gap in the same group. +3. **Number contiguously across both groups** (1, 2, 3, … not 1a, 1b), following that order. Contiguous numbering lets the author refer to a gap by a single number in chat. + +Numbers are assigned once, when the comment is first composed. A refine run never reorders and never renumbers — see step 6. + +**Requirements gaps** — probe these before any technical question. Consider each category below and raise a gap only where the issue leaves it open: - **User & persona** — who uses this, in what context. The issue may name a feature without naming the person. - **Job-to-be-done** — the user-visible outcome that means "done"; contradictions between the stated outcome and the proposed mechanism. @@ -103,7 +123,7 @@ Each gap (in either group) must: - **Acceptance criteria from outside** — whether existing ACs are observable from outside the implementation by a user or external test; flag project-housekeeping items (project exists, sln updated, test scaffolding) — they belong in `/speckit.tasks`. - **User-visible failure modes** — what the user sees when a dependency is unreachable, slow, or rejects them; fail-open vs fail-closed *from the user's viewpoint*. -**Technical gaps** — surface gaps suggested by Step 2 (codebase grounding) plus any tech shape the issue itself already commits to. Let the author set the depth: they may want extensive tech review or none. +**Technical gaps** — surface gaps suggested by Step 2 (codebase grounding) plus any tech shape the issue itself already commits to. Let the author set the depth: they may want extensive tech review or none. Same test as above — consider each category, raise a gap only where something is genuinely unsettled. - **Where it lives** — existing endpoint/service/flow extended vs. new; who calls whom; which org(s) are involved. - **Integration** — interface contracts with existing components, event/data flow, ordering. @@ -129,15 +149,14 @@ Write what survives as bullets, not questions: ### 4. Draft the comment -Structure the comment body as follows. Each gap gets an inline answer slot -(`> _Answer:_`) so the issue author can respond beneath it in a single edit. +Structure the comment body as follows. A table lists every gap up front so the author can triage the review before reading into it, and each gap then gets an inline answer slot (`> _Answer:_`) so they can respond beneath it in a single edit. ```markdown ## Pre-specification review — gaps & clarifications -Before taking this into SDD, the following points need answers. Please record responses inline. +Before taking this into SDD, the following points need answers. The table lists every gap and what turns on it — use it to decide where to spend your attention, then record responses inline beneath each gap. > If an answer slot says `not sure`, `idk`, `tbd`, `more options`, `help me`, or similar hedge (anything that means "I want help, not a decision"), re-run `/speckit.reviewissue #N` and that question will be re-framed with extra options, a worked example, and a revised recommendation. Iterate as many times as you need. > @@ -145,9 +164,18 @@ Before taking this into SDD, the following points need answers. Please record re > > When all answers are concrete, run `/speckit.confirmissue #N` to fold them into the issue body as **Confirmed decisions**. That deletes this comment — the decisions are the record from then on, and you revise one by editing its bullet. +### Gaps at a glance + +| # | Gap | If answered against the recommendation | +|---|---|---| +| 1 | | Changes what gets built — | +| 2 | | Settles a detail — | +| 3 | | Changes what gets built — | + ### Requirements gaps **1. ** +_Changes what gets built:_ - - @@ -157,6 +185,7 @@ Before taking this into SDD, the following points need answers. Please record re > _Answer:_ **2. ** +_Settles a detail:_ ... > _**Recommendation:**_ ... Reason: ... @@ -168,6 +197,7 @@ Before taking this into SDD, the following points need answers. Please record re > Omit this section entirely if no technical gaps were identified — do not emit an empty heading. Gap numbering continues from the Requirements section (3, 4, …), not restarting at 1. **N. ** +_:_ ... > _**Recommendation:**_ ... Reason: ... @@ -182,8 +212,15 @@ Before taking this into SDD, the following points need answers. Please record re - ... ``` -Keep each gap tight. If a gap has more than ~3 sub-bullets, consider whether -it is actually two gaps. +**The at-a-glance table.** One row per gap, in the same order as the gaps themselves, spanning both groups — so the consequence column is not sorted globally: it restarts at *Changes what gets built* where the Technical group begins. Titles in the `Gap` column match each gap's own title verbatim, and the consequence cell is a compression of the gap's own consequence line, so a row and its gap are unmistakably the same thing and never say different ones. Always emit the table, even for a single gap: the author should never have to check whether it is there. + +**Never use the `**N. **` form in the table, and never put a `> _Answer:_` line above the first group.** `/speckit.confirmissue` parses every `**N. <title>**` block in the comment as a gap, ending at its `> _Answer:_` line (its step 2). A row imitating that shape carries no answer slot of its own, so it either hard-stops the fold — step 2 refuses to fold anything while a parsed gap looks unanswered — or takes the first real gap's answer slot as its own and corrupts the decisions that do land. Table cells carry a bare number and plain text, which matches nothing the parser looks for. + +**Length bound — 120 words per gap.** Count everything from the `**N. <title>**` line through to its `> _Answer:_` slot: the consequence line, the framing, every sub-bullet, and the recommendation with its reason. Count whitespace-separated words of the prose, taking a markdown link as its link text rather than its URL. The bound applies to the gap as a whole rather than to any one part of it, and to the comment as first composed — a refine run's expansion (step 6) may exceed it, where keeping the re-framing tight is the goal rather than the ceiling. + +**Splitting is not how you meet the bound.** A 200-word gap broken into two 100-word gaps satisfies nothing — the reader faces the same prose and one more decision. Cut instead: drop the restatement of what the issue already says, keep the evidence that makes the gap specific, and let the recommendation carry the detail rather than the framing. Split only where the gap is genuinely two independent questions needing two separate answers — and then each half must meet the bound on its own. + +**Check the draft before posting.** Composing to a bound is not the same as meeting one — count, do not estimate. Before step 5 posts, verify against the draft: one table row per gap, its title matching the gap verbatim and its consequence cell agreeing with the gap's consequence line; every gap carrying a consequence line, a recommendation with its one-sentence reason, and an answer slot; every gap within the bound, counted rather than judged; *Changes what gets built* ahead of *Settles a detail* within each group; and no line above the first group heading matching either `**N. <title>**` or `> _Answer:_`. Fix what fails and re-check. This applies again to a refine run's edit (step 6), minus the bound. **Recommendation quality bar:** the recommendation must be a concrete, actionable default (a value, a library, a field name, an HTTP status, an @@ -251,7 +288,9 @@ Re-framing rules (hedging case only): - **Expand the question body** with 2–4 concrete options laid out as a sub-list, each with a one-line trade-off. Add a worked example or a pointer to a comparable existing pattern in the codebase (read the codebase again if needed — surface defaults they may not have known existed: existing constants, sibling service patterns, port allocations, etc.). - **Revise the `> _**Recommendation:**_` line** if the new framing changes your call. Keep the `Reason:` cite tied to evidence. - **After ~2 hedging iterations on the same question** with no commitment, add a final option *"This may be out of scope for the current issue — answer `out of scope: <reason>` to drop it"* and call it out in the recommendation. Do not edit the gap out yourself — leave that to the author + `/speckit.confirmissue`. -- **Do not touch any other gap.** Substantive, out-of-scope, and empty answers must come through byte-for-byte. The Commentary section is also untouched. +- **Keep the gap's consequence line and its table row in step with the re-framing.** If the new framing changes that gap's title or its consequence, update both places that state it — the `_<kind>:_` line on the gap body, and its row's title and consequence cells — and nothing else in the table. Never add, remove, reorder or renumber rows: the table mirrors the posted gap order, fixed when the comment was first composed. A consequence that changes after posting can therefore leave a *Changes what gets built* gap sitting below a *Settles a detail* one; that is the accepted cost of never renumbering a review the author already refers to by number. +- **Do not retrofit the table onto an older comment.** A comment posted before the at-a-glance table and the consequence line existed has no row to update and no kind to restate — leave it that way. A refine run re-frames the hedging gap and nothing else; it never adds a table to a comment that has none. +- **Do not touch any other gap.** Substantive, out-of-scope, and empty answers must come through byte-for-byte, and so must every table row but the one you changed. The Commentary section is also untouched. If no gap qualifies for re-framing, **make no edit** and report that in chat (the author either still has un-answered questions, or is ready for `/speckit.confirmissue`). @@ -286,7 +325,7 @@ Keep your own chat response short. Tailor it to the run mode: **First run:** - confirm the issue reviewed (number + title) -- state how many gaps were raised, and whether the review carries commentary +- state how many gaps were raised, split by consequence (e.g. *5 gaps — 2 change what gets built, 3 settle a detail*), and whether the review carries commentary - return the comment URL **Refine run:** diff --git a/docs/change-intent-records/2026-09-12-triageable-review-comment.md b/docs/change-intent-records/2026-09-12-triageable-review-comment.md new file mode 100644 index 0000000..aa95d93 --- /dev/null +++ b/docs/change-intent-records/2026-09-12-triageable-review-comment.md @@ -0,0 +1,41 @@ +# A Triageable Pre-Specification Review + +**Intent:** Let the author of an issue spend attention on a `/speckit.reviewissue` comment in proportion to what is at stake, and stop the command's fourteen-category sweep inflating the review beyond what the issue actually leaves open — without dropping, demoting or capping a single gap on merit. + +**Behaviour:** + +- Given a review raising several gaps, When the author opens the comment, Then an at-a-glance table gives every gap's number, title and consequence before any gap body is read. +- Given gaps that differ in consequence, When the comment is composed, Then within each group the gaps that change what the issue commits to building are numbered ahead of those that settle a detail, and each gap names its own kind. +- Given an issue that leaves little open, When the review runs, Then it raises gaps only where something is genuinely unsettled, and is visibly shorter than the review of an issue that leaves much open. +- Given a gap of any consequence, When it is composed, Then it still carries a recommendation with a one-sentence reason and an inline answer slot, in the form `/speckit.confirmissue` already parses. + +**Constraints:** + +- `/speckit.confirmissue`'s prompt is out of scope, so its parser is fixed: it folds every `**N. <title>**` block ending in a `> _Answer:_` line into the issue body. Anything added above the gaps must match neither shape. +- What that command's routing depends on is the *grouping* — it files each folded decision by the heading its gap sat under — not the sequence of the two sections, which is byte-identical in its output either way. Consequence ordering therefore operates strictly *within* a group. Requirements-before-Technical is retained on its own merits (requirements are probed first, and #281 puts the groups' order out of scope), not because the parser needs it. +- The author refers to gaps by number in chat, and folding strips the numbers, so ordering must be settled before numbering and a posted review is never renumbered. +- Prompt-only change, so Principle I's configuration-and-tooling carve-out governs: no xUnit test, and explicitly no bespoke checker standing in for a tool. The evidence is the command's own output before and after, recorded in the PR body. The carve-out's second clause — make the check repeatable — is discharged by a self-check inside the command rather than by CI, for the reason under *Known residuals*. + +**Decisions:** + +1. **Consequence is two values, not a severity scale.** *Changes what gets built* versus *Settles a detail*, decided by a question about the artefact — would a different answer change something the issue commits to building? Rejected: a three-tier priority, which offers a middle bucket everything drifts into and still does not say *what* would change. Borderline calls take the lower kind, because an inflated marker costs the reader precisely the attention the marker exists to save. + +2. **The up-front view is a table, and a downstream parser chose its shape.** The obvious index mirrors the gaps — a bold-numbered list — and that is exactly the shape `/speckit.confirmissue` parses as a gap. The damage is not a tidy extra bullet: a row has no answer slot of its own, so that command's step 2 either hard-stops the whole fold as unanswered, or resolves the row's end to the first real gap's answer slot and corrupts the decisions that do land. A table row carries a bare number and plain text and matches nothing the parser looks for. Also rejected: a collapsed `<details>` block, which leaves the reader an action to perform before they can see the set, which is the behaviour being removed. + +3. **The table is always emitted, even for a single gap.** Rejected: a threshold below which it is omitted. A threshold makes the reader establish whether the table is missing or merely absent before trusting it, and gives the refine run a mode in which a row may or may not exist. + +4. **The sweep is narrowed at source by an open-ness test, not by a cap.** A category considered and found settled produces nothing. Rejected: a numeric cap on gaps, which forces out a genuine gap for arriving last; and the superseded #278 proposal of demoting the gaps the reviewer predicts the author would not contest, which asks the model to judge a person rather than the issue and settles a real decision silently when it judges wrong. + +5. **The length bound is per-gap words, and splitting is explicitly not the remedy.** The rule it replaces bounded sub-bullets and offered "consider whether it is actually two gaps" as the fix — which raises the gap count while leaving the word count where it was. 120 words was chosen against real output: counted from each `**N. <title>**` line through to its own `> _Answer:_` slot, the four gaps of the review on #278 ran 184, 192, 196 and 223 words, so the bound bites on every one of them while still leaving room for framing, cited evidence, two or three sub-questions and a reasoned recommendation. (An earlier draft of this record said 317 for the last gap; that was a counting script running past the answer slot to the end of the comment. The spread is 184–223 and the conclusion is unchanged.) + +6. **A refine run does not retrofit the shape, and keeps both statements of a consequence in step.** A comment posted before the table and the consequence line existed keeps neither. Re-framing a hedging gap updates that gap's consequence line *and* its row — the two places a consequence is stated — and nothing else. Because rows are never reordered and numbers never reassigned, a consequence that changes after posting can leave the order no longer sorted; that is the accepted cost of not renumbering a review the author already cites by number. + +**Known residuals:** + +- **The consequence call is an unverified model judgement.** #281's own open questions name the fallback if it proves badly calibrated: order by consequence without publishing the marker, which keeps the benefit that survives a wrong call and drops the one that does not. +- **Enforcement is a self-check inside the command, not a gate** — against this repo's precedent of preferring a gate (CIR [`2026-09-07-automated-prespec-review`](2026-09-07-automated-prespec-review.md), decision 5). The two halves differ and should not be conflated. Whether a gap was manufactured, and whether a review is proportionate, are judgements no exit code decides. But the table's shape, the parser-collision rule, the word bound, the per-gap consequence line and the ordering *are* mechanically decidable, and [`.claude/hooks/traceability-gate.sh`](../../.claude/hooks/traceability-gate.sh) is this repo's precedent for gating a markdown-shape invariant. What blocks a gate here is that the invariant lives in a GitHub comment composed at runtime: a first run pipes the body straight into `gh issue comment`, so there is no committed artefact for a hook or a CI step to read. Making it gateable means having the command always write the body to a file first and checking that — a change to how the command posts, which #281 does not scope. Until then the check is repeatable but self-administered, which is weaker than the precedent. +- **`/speckit.draftissue` is a third consumer of this comment.** It ingests the whole body as brief material in migration mode and reads it as prose rather than parsing its shape, so the table breaks nothing — but each gap title now reaches it twice. Enumerated here deliberately: the same consumer was missed on each of the two preceding issues — by a review of the Commentary section's own description (`docs/study/279.md`), then by the enumeration of consumers for the comment's deletion (`docs/study/280.md`). +- **The two kinds are framed around built behaviour, and some gaps are about neither.** A gap asking which release a change ships under, or what its RED step is, concerns a process artefact rather than something built, and neither kind fits it cleanly. A controlled trial hit this on two of five gaps; the "when borderline, take *Settles a detail*" tie-break absorbed it without a wrong call, which is the tie-break doing its job rather than evidence that the taxonomy is complete. +- **Nothing verifies that a recommendation is a good default, or that its *Reason* survives being checked against the file it cites.** A shorter review makes each recommendation carry more weight, so this change raises the cost of that gap without addressing it. #281 records it as separate and more consequential than length. + +**Date:** 2026-09-12