Commentary Section

[FrankRay78]

Why

/speckit.reviewissue emitted a section called Notes for SDD — a name that describes a pipeline stage rather than its content, and that only resolves for a reader who already knows the answer. A "Speckit Docs Tidy" commit (e2996b0) had also given it a second flavour: items "deferred to /speckit.specify". That hand-off never worked and never could — /speckit.specify is stock spec-kit taking a natural-language description via $ARGUMENTS, with no notion of a GitHub issue, a comment or a review. The same commit stated the deferral rule twice, in two different wordings, two paragraphs apart.

The section also had no written boundary and no bar for entry, so it had already absorbed content outside its own definition once and was carrying bullets of the right kind and no value ("three hooks will fire, all of them no-ops here").

What changes

• The section is Commentary, everywhere it appears including the emitted comment template.
• The /speckit.specify hand-off is gone, along with the duplicate routing rule that expressed the same drift without naming a destination.
• The section now states what it carries: remarks that inform the spec author and require no answer, and nothing else.
• A boundary: the source of a constraint decides, not its force. A fact already true of the codebase is commentary however binding it proves; a choice only the author can make, or an obligation this issue would newly impose, is a numbered gap.
• A filter: only what the spec author would otherwise miss or get wrong. A bullet restating a rule they already hold, or amounting to "nothing to do here", does not appear — and the template now omits the heading entirely rather than emitting it empty.
• The example list was checked against the new filter and the two examples it bans were removed.

RED–GREEN evidence

Constitution Principle I's configuration/tooling carve-out applies: this is a prompt, so the evidence is the real check failing and then passing, not a bespoke test. No hand-rolled checker was written.

RED — before the change:

$ grep -n "Notes for SDD" .claude/commands/speckit.reviewissue.md
74:  ... plus a **Notes for SDD** section for non-question observations.
99:  ... Items the author wants tracked but not answered now belong in **Notes for SDD**.
109: **Notes for SDD** — ... items the author chose to defer to `/speckit.specify` ...
166: ### Notes for SDD
241: ... The "Notes for SDD" section is also untouched.

$ grep -nEi "\bnotes?\b" .claude/commands/speckit.reviewissue.md
277: - state how many gaps + how many notes were raised

GREEN — after:

$ grep -nEi "\bnotes?\b|Notes for SDD|tracked but not answered|acceptance scenario naming" .claude/commands/speckit.reviewissue.md
(no matches)

Full suite green throughout: 648 passed, 0 failed, 0 skipped, 0 warnings. dotnet format style and dotnet format whitespace both exit 0 with no changes.

Non-obvious things a reviewer should know

• A Blocker was caught in review and fixed in 6c5cbc3, not in the original commit. The first draft added an example bullet asserting "frontmatter in this repo is a single description: line". That is true of 6 of the 31 files under .claude/commands/ and .claude/skills/ — and speckit.reviewissue.md itself carries no frontmatter at all, so the file asserted a convention it violated. It matters beyond tidiness: this command posts to a public issue, and .github/workflows/speckit-reviewissue.yml:106 instructs the CI agent that "the review must cite real paths and real conventions from this repository". The example is now an illustration that asserts nothing about this repo.
• "No downstream command parses it" is deliberately narrower than "hands nothing on". The first draft claimed the latter; speckit.draftissue.md:37 ingests the whole <!-- speckit:review --> comment as brief material in migration mode, so only the parsing claim is true.
• The boundary carries an extra clause the issue's AC did not spell out: a convention that already governs an area stays commentary even when this issue is the first work to trigger it. Without it both halves of the rule fire on the same input, and anything wrongly pushed out of commentary becomes a numbered gap — which must then manufacture a question and an answer for /speckit.confirmissue to fold into the issue body as a Confirmed decision.
• /speckit.confirmissue is untouched and unaffected. It keys on **N. <title>** blocks ending in > _Answer:_ and on the two gap headings; Commentary is unnumbered plain bullets with no answer slot, under a heading it never reads.
• Deferred, out of scope — worth its own issue. The authoring blockquotes at :172 (Commentary) and :161 (Technical gaps) sit inside the fenced block that defines the posted comment body, where sibling blockquotes are literal published output. Fixing it properly means touching the pre-existing Technical-gaps guard, which is a second mission.
• docs/study/279.md records six findings from this branch — five Execution, one Plan-spec.

How to verify

• grep -nEi "\bnotes?\b|Notes for SDD" .claude/commands/speckit.reviewissue.md returns nothing.
• git diff main...HEAD --name-only lists only .claude/commands/speckit.reviewissue.md and docs/study/279.md — no other command file changed.
• The example list under Commentary contains no bullet that restates a rule already in CLAUDE.md or the constitution.
• The refine-run rule still guarantees the section is left untouched.
• Read the boundary and filter paragraphs as if executing them: is each decidable against a concrete bullet?

Related

Closes #279

• Unblocks Delete the review comment once /speckit.confirmissue has folded it into the issue body #280, and through it Make a /speckit.reviewissue comment triageable, and stop the category sweep inflating it #281 — both change /speckit.reviewissue and should land on the corrected name and the stated boundary.
• .claude/commands/speckit.confirmissue.md — the downstream consumer, unchanged, whose parser constrains the template.

[FrankRay78]

@claude Review this pull request. Analyse the code changes and provide feedback covering:

• Bugs or correctness issues
• Security concerns (including any flagged by static analysis)
• Adherence to the project conventions in CLAUDE.md
• Test coverage — do the tests adequately cover the new behaviour?
• Any spec/test-plan mismatches

End the review with a recommendation whether to merge the PR, and if not, what you suggest needs addressing first.

[claude]

Claude finished @FrankRay78's task in 2m 47s —— View job

---

Review

• Gather context (PR body, diff, CLAUDE.md conventions)
• Review diff for bugs/correctness
• Check security concerns
• Check CLAUDE.md / constitution adherence
• Check test coverage (RED-GREEN evidence for prompt/tooling changes)
• Check for spec/test-plan mismatches

Scope: .claude/commands/speckit.reviewissue.md (+16/-10) and docs/study/279.md (+10, new). No production code (.cs) touched.

Bugs / correctness

None found. I independently re-verified the claims in the PR description rather than taking them on trust:

• Rename completeness — grepped the whole repo for Notes for SDD; only hit is the historical mention inside docs/study/279.md (documenting the old name, correctly not a live reference). No stray occurrence anywhere else, and no other command file references the old /speckit.specify hand-off either.
• "No downstream command parses it" (line 109) — checked both consumers: speckit.confirmissue.md keys only on the **N. <title>** / > _Answer:_ gap format and never mentions Commentary/Notes; speckit.draftissue.md:37 does ingest the whole `` comment as migration-mode brief material, so the claim is correctly scoped to "parses" rather than the stronger (and false) "hands nothing on" from the first draft.
• The boundary clause ("a convention that already governs this area stays commentary even when this issue is the first work to trigger it") resolves the exact non-decidability the study doc flags as a code-reviewer Blocker on the prior commit — checked it against a few hypothetical bullets and it holds.
• The two example bullets removed (Testing conventions…, Project-specific authoring rules … from CLAUDE.md) are exactly the two that fail the new filter (restate a rule the author already holds); the reworded Documentation files that cover this area, listed by path survives because it's informational, not a restated rule.
• git diff origin/main...HEAD --name-only → only the two files listed above, matching the PR's own verification checklist.

One pre-existing wart, correctly not fixed here and explicitly called out as deferred in the PR body: the > Omit this section entirely... instruction blockquotes for both Technical gaps and Commentary sit inside the fenced block that defines the literal posted comment, so on a run with no gaps at all they'd render as literal text in the GitHub comment. This is pre-existing behavior for Technical gaps (unchanged by this PR) and the author has scoped fixing it to a separate issue — reasonable, since it touches a different guard than this rename/boundary/filter change.

Security

None applicable — markdown/prompt-only change, no code execution paths, no secrets.

CLAUDE.md / constitution adherence

• Correctly invokes Constitution Principle I's config/tooling carve-out: the RED/GREEN evidence is real grep output (before: matches; after: none), not a bespoke test standing in for a tool. No hand-rolled checker was introduced.
• Markdown soft-wrap convention (feedback_markdown_soft_wrap.md) is followed — the three new prose paragraphs and all new/changed bullets are single-line; the one bullet that was hard-wrapped (Project-specific authoring rules … from CLAUDE.md) is removed rather than carried forward.
• docs/study/279.md follows the /study skill's table format and its five commit-hash references (87d239f, 6c5cbc3) match the actual commit log.
• No NetPace.Core public API touched, so the XML-docs / dependency / semver rules don't apply.

Test coverage

Adequate for the change's nature. Since this is a prompt file, not production code, "tests" are the grep-based RED/GREEN evidence — and it's real: I reproduced the GREEN grep independently against the actual committed HEAD content (not just trusting the pasted output) and it returns no matches. No dotnet test re-run was needed since no .cs file changed; I did not re-verify the PR's "648 passed" / dotnet format claims myself since they're unaffected by a docs-only diff, so take those as author-reported rather than independently confirmed here.

Spec/test-plan mismatches

None — this issue has no spec.md/test-plan.md (doc/prompt-only issue closed directly), so Principle VIII/IX traceability doesn't apply.

Recommendation

Merge. The change is small, self-contained, already went through one review-and-fix cycle (6c5cbc3, documented with five findings in docs/study/279.md), and every claim in the PR description checks out against the actual repository state. No blockers.