Triageable Review - #284
Conversation
Four changes to the pre-specification review, all in its own prompt: - An at-a-glance table above the first group gives every gap's number, title and consequence, so the review can be triaged before it is read. Its shape is forced by /speckit.confirmissue, which folds every **N. <title>** block ending in `> _Answer:_` into the issue body — a bold-numbered index would have been folded as phantom decisions. - Every gap names one of two kinds — changes what gets built, or settles a detail — and says in one clause what a different answer would change. - Gaps are ordered by consequence within each group, then numbered. Requirements still precedes Technical, since that grouping is what /speckit.confirmissue routes folded decisions by. - A 120-word bound per gap replaces "keep each gap tight", which bounded sub-bullets and offered splitting as the remedy — raising the gap count while leaving the word count alone. The category lists become things to consider rather than a sweep to complete: a category with nothing open produces no gap. Yield is unchanged — no gap is dropped, demoted, or capped on merit, and every gap keeps its recommendation and answer slot in the form the downstream parser already reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six decisions a future maintainer would reasonably question, and the constraint none of them make obvious: the up-front view is a table because /speckit.confirmissue folds anything shaped like a numbered gap into the issue body, so the obvious bold-numbered index would have posted phantom decisions the author never made. Also records why consequence is two values rather than a severity scale, why the sweep is narrowed at source rather than capped, why splitting is not how a gap meets the length bound, and where 120 came from — the four gaps of the review on #278 ran 184 to 317 words. Four residuals, including the third consumer of this comment (/speckit.draftissue), enumerated here because reviews of this same section missed it on both preceding issues. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One line per paragraph, per CLAUDE.md. The four paragraphs added around the comment template were hard-wrapped to match the older text beside them, which reflows a whole block on a one-word edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four corrections where the new text asserted something untrue or contradicted a rule elsewhere in the file: - The 120-word bound was stated as an unscoped absolute, contradicting step 6's mandate that a re-framed gap gain 2-4 options with trade-offs and a worked example. Scoped to first composition. - A refine run was told to update the table row but never the gap's own consequence line, so the up-front view could drift from the gap it describes — the one thing AC10 exists to prevent. - "This order is load-bearing" was false. /speckit.confirmissue routes by the heading a gap sat under, not by the sequence of the two sections; swap them and its output is byte-identical. Reattributed to grouping. - The phantom-decision mechanism was wrong. That command hard-stops in step 2 when a parsed gap looks unanswered, so a table row in gap shape either blocks the fold or takes the first real gap's answer slot. Also: narrowed an absolute that excluded step 1's existing-comments rule; dropped the zero-gap table exception, which had no template support (the empty-group path belongs to #278's successor); dropped an unsupported claim that contiguous numbering helps the parser, which strips numbers; named "Settles a detail" rather than "the lower of the two", since the design denies there is a scale; softened "the sweep is manufacturing gaps" to a likelihood, per #281's own open question; and fixed the example table to show the consequence column restarting at the group boundary. Added a pre-post self-check step. Composing to a bound is not meeting one: the first draft written under this prompt overshot on 3 of 3 gaps unaided, which no instruction caught. CIR: the 184/192/196/317 figure was wrong — the counting script ran past the answer slot to end-of-file, adding 94 words of trailing heading and commentary. Two independent counts put the fourth gap at 223. Corrected, with the counting method stated. Also split the gating residual, which conflated the two judgement-only criteria with nine that are mechanically decidable, and recorded why a hook cannot reach them: a first run pipes the body straight to gh, leaving no committed artefact to check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@claude Review this pull request. Analyse the code changes and provide feedback covering:
End the review with a recommendation whether to merge the PR, and if not, what you suggest needs addressing first. |
|
Claude finished @FrankRay78's task in 3m 2s —— View job Review complete
SummaryThis is a prompt-only change to I checked the diff ( Correctness
One small robustness gap I didn't see addressed: the table is Markdown, and cells are delimited by CLAUDE.md / constitution adherence
Test coverage — the main thing worth flaggingFor a prompt file there's no xUnit suite; the real "tests" here are the manual verification checklist in the PR body. That checklist is honest about its own gaps, and three items are still unchecked:
Given the parser-collision risk is the single most consequential thing this PR is trying to avoid, I'd want AC9 exercised for real (not just shape-inspected) before merging — that's the one scenario where a mistake silently corrupts confirmed decisions on a real issue rather than just producing a worse review. Spec/test-plan mismatchesNone found — there is no SecurityNo concerns. No new shell/eval patterns, no new secrets handling; existing RecommendationHold merge until the AC9 live check is run. Everything else — the corrected parser-routing claims, the word-bound/refine-run interaction, the CIR's honesty about residuals (including the self-administered vs. gated enforcement gap, which is well-argued and appropriately scoped out of this PR) — is sound and I have no correctness objections. But this PR's entire value proposition rests on the table never being mis-parsed by Branch: |
1 participant
Why
The review comment offered two options: read all of it, or read none of it. Every gap arrived at identical visual weight — same bold numbered heading, same framing, same recommendation, same answer slot — so nothing distinguished a gap that would change an acceptance criterion from one that settled a file name. Finding out which was which meant reading both. Skipping is the rational response to that when the review is long.
Four properties of the prompt produced it: no summary, no length bound, no ordering by consequence, and a mandated sweep of fourteen categories where breadth was instructed but proportion to the issue in hand was not.
What changes
Yield is unchanged, which is the constraint the issue was most insistent about. No gap is dropped, demoted or capped on merit, and every gap keeps its recommendation and answer slot in exactly the form
/speckit.confirmissuealready parses.Non-obvious things a reviewer should know
A downstream parser chose the table's shape. The obvious index mirrors the gaps — a bold-numbered list — and that is precisely what
/speckit.confirmissueparses as a gap. #281 puts that command's prompt out of scope, so the constraint had to be designed around rather than fixed. A table row carries a bare number and plain text and matches nothing the parser looks for.The failure mode that rules this out is not the obvious one. The first draft of this branch said a table row would be "folded as a phantom decision the author never made". It would not:
/speckit.confirmissuehard-stops in step 2 whenever a parsed gap looks unanswered, so a row in gap shape either blocks the whole fold, or its block-end resolves to the first real gap's answer slot and corrupts the decisions that do land. Corrected here, and it is a stronger argument for the rule than the phantom was.What is load-bearing is the grouping, not the section order. The first draft claimed "this order is load-bearing". It is not — that command routes each folded decision by the heading its gap sat under, so swapping the two sections leaves its output byte-identical. Requirements-before-Technical is retained on its own merits (requirements are probed first; #281 puts the groups' order out of scope), not because the parser needs it. This matters because labelling a false claim "load-bearing" invites a future editor to test it, find it false, and relax the genuine constraint next to it.
The bound is scoped to first composition, deliberately. Step 6 requires a re-framed hedging gap to gain 2–4 options with trade-offs plus a worked example. That cannot fit in 120 words, and an unscoped bound would have forced a refine run to silently break one of two instructions.
A pre-post self-check was added because composing to a bound is not meeting one. The first draft written under this prompt overshot on 3 of 3 gaps, unaided, and no instruction caught it. The check is self-administered rather than gated — see the CIR's residual for why a hook cannot reach this invariant: a first run pipes the comment body straight into
gh issue comment, leaving no committed artefact to check.The zero-gap case is deliberately left alone. An earlier draft added a "no gaps at all → omit the table" exception. The template has conditional-omit directives for Technical gaps and Commentary but none for Requirements gaps, and the intro sentence promises points that would not exist — so the exception had no support and half-fixing it was worse than not touching it. That path was gap 3 of #278's own review and belongs with #278's successor.
The taxonomy does not cover process artefacts. A gap asking which release a change ships under, or what its RED step is, is about neither built behaviour nor a detail within it. The "when borderline, take Settles a detail" tie-break absorbed both cases in trial without a wrong call, but the two kinds are not a complete partition. Recorded as a residual.
How to verify
Per Constitution Principle I's configuration-and-tooling carve-out there is no xUnit test here — the evidence is the command's own output before and after. The controlled half is done; two live checks are not, and should happen before merge.
A blind A/B was run to avoid grading my own work: two clean-context agents each composed a review of the same issue (#242), one given the pre-change prompt and one the new prompt, neither told a comparison was happening or what was being tested.
> _Answer:_lines above the first group, checked by script against the composed body.dotnet build ./srcclean (0 warnings),dotnet test ./srcgreen — 648 passed, 0 failed, 0 skipped. No C# changed; this is the unconditional gate, not evidence for the change./speckit.confirmissueend to end. Confirm the table contributes no bullet to## Confirmed decisions. The mechanical check above is inspection, not execution./speckit.reviewissue, and confirm the re-framed gap's consequence line and its table row both move, every other gap and row comes through byte-for-byte, and nothing is renumbered.Two caveats a reviewer should weigh rather than take on trust. AC3's ordering rule was only trivially exercised — all three Requirements gaps came out Changes what gets built and both Technical ones Settles a detail, so nothing actually needed sorting within a group. And the pre-change run produced more informative gap titles: declarative findings ("The CSV writer evaluates the placeholder before discarding it, so the obvious null conversion breaks the build") against this branch's questions ("How do the writers hold a result that may not exist?"). It also found two things the new run missed. I cannot attribute that to the prompt — that agent compiled the change to find it and the other did not — but it is the one signal pointing against this change, and it deserves a human eye.
Related
Closes #281
docs/change-intent-records/2026-09-12-triageable-review-comment.md— six decisions, five residuals, including the un-gated enforcement and the/speckit.draftissuethird consumer that reviews of the two preceding issues both missed.c5f4eed's message cites the Stop /speckit.reviewissue turning settled calls into questions #278 gaps as "184 to 317 words". The 317 was a counting script running past the answer slot to end-of-file, adding 94 words of trailing heading and commentary. The real spread is 184–223; the CIR is corrected, but that commit message is immutable.