Delete Review Comment

[FrankRay78]

Why

The pre-specification review comment is working material. A numbered gap carries framing, cited evidence, a recommendation with its reason, and an answer slot — and ten of those compress to ten one-line decisions once /speckit.confirmissue has folded them in. After that the comment dwarfs the ## Confirmed decisions section that matters, and whoever picks the issue up has to work out which of the two halves is current. In practice it was already being deleted after every successful run; the prompt forbade the thing that actually happened.

Deleting it is the easy half. The comment's existence was the done-marker — docs/agentic-workflow-NetPace.md stated the contract, and two guards keyed on it — so the marker had to move first, or "confirmed" would have become indistinguishable from "never reviewed".

What changes

The marker moves to the ready label. /speckit.reviewissue and the speckit-reviewissue.yml guard both check it before falling back to the <!-- speckit:review --> sentinel, so issues confirmed under the old behaviour still read as reviewed with no migration.

/speckit.confirmissue deletes the review, but only after reading the ready label and the ## Confirmed decisions section back from the issue. Every hard-stop sits upstream of that point, so a run that stops early leaves the review answerable and re-runnable.

Re-running confirmation on a confirmed issue now says so, instead of reporting that there is no review to confirm — and repairs a missing ready label while it is there.

Reopening is removing the ready label. An issue whose scope has moved is by definition no longer ready; nobody has to hand-edit the body.

Docs: the state contract in docs/agentic-workflow-NetPace.md, a new CIR, and in-place supersede annotations on 2026-09-07-automated-prespec-review.md.

Non-obvious things a reviewer should know

The marker is the label alone, not "label OR body section". The issue's technical notes sketched the OR, but it cannot coexist with AC #8 — the only way to clear a body section is to hand-edit the body, which is exactly what AC #8 forbids of the reopen path. CIR decision 1 records this and both rejected alternatives. This is the one place the implementation deliberately departs from the issue's own technical sketch.

Deletion is gated on the label having landed, not merely on the body patch. The label step is never-fatal by design, so deleting after a label failure would leave decisions on the body with no label and no comment — indistinguishable from never-reviewed to both guards, and re-reviewed on top of its own decisions. That state is the one thing the design exists to prevent, and it is the only one reached through a door that cannot be reopened.

Three blockers were found in review and are fixed here — worth knowing because the first would have made the feature a silent no-op:

• The delete used the wrong ID namespace. gh issue view --json comments returns the GraphQL node id (IC_kwDONbNDk88AAAABTCYLgw); DELETE repos/.../issues/comments/<id> takes the numeric REST id (5572529027). Every run would have 404'd, and the "never fatal" rule would have reported that permanent failure as transient bad luck indefinitely. Step 1 now resolves over REST and selects by created_at — node ids do not sort chronologically, and that selection decides which comment is destroyed.
• The guard failed open. jq -e '[.labels[].name] | index("ready")' exits 5 on a missing or null labels key, and inside an if errexit is suppressed, so a jq error was indistinguishable from "not ready" and fell through to posting a duplicate. The label read is now live and assigned outside the if, so a gh/jq failure fails the job.
• Reading labels from the previous step's snapshot while reading comments live paired a stale half with a fresh one. A local /speckit.confirmissue is a second writer to the same state, so an issue confirmed between the two reads showed neither marker — and the post-condition check would still have passed the run green. Both reads are live now.

Deliberately not done, and recorded as residuals in the new CIR rather than silently dropped:

• No archive or tombstone. The issue's Out of scope settles this; the gap framing and recommendations are gone for good, and a decision whose reason lived only in the recommendation text is not recoverable.
• The reopen signal is indistinguishable from a lost label. "No ready, no comment, decisions on the body" is both the deliberate reopen state and what a repo-level label rename leaves behind. Telling them apart needs an explicit reopen signal, which reopens CIR decision 1.
• The deletion is enforced by a prompt instruction, not a gate — against this repo's own precedent (CIR 2026-09-07 decision 5). /speckit.confirmissue is a local command with no CI step to move it into; the read-back narrows the gap as far as the mechanism allows.

How to verify

Per Constitution Principle I's configuration/tooling carve-out there is no xUnit test here — the RED-GREEN evidence is the real tool failing before and passing after. The local half is done; the real-issue half is not, and should be captured before merge.

Done locally:

• Guard predicate, RED: against a confirmed-issue fixture (ready, no review comment) the pre-change guard did not fire — the job would have proceeded to post a second full review over ## Confirmed decisions.
• Guard predicate, GREEN: same fixture now skips; the reviewed-but-unconfirmed and never-reviewed fixtures keep their existing paths.
• Node-ID blocker verified live against this repo — node IC_kwDONbNDk88AAAABTCYLgw vs REST 5572529027 for the same comment.
• Guard fails closed: a failing gh in the label assignment exits non-zero under bash -e rather than falling through.
• dotnet build ./src clean (0 warnings), dotnet test ./src green — 648 passed, 0 failed, 0 skipped. No C# changed; this is the unconditional gate, not evidence for the change.

Still to do on a real issue:

• Dispatch speckit-reviewissue.yml against this branch ref (workflow_dispatch runs the ref it is dispatched against) for an issue carrying ready, and confirm it skips and clears the review label rather than posting.
• Run /speckit.confirmissue end-to-end on a throwaway issue with an answered review: decisions land, ready applies, the comment is deleted.
• Then request a review on that same issue and confirm nothing is posted; remove ready and confirm a fresh first review is.

Note the issues path runs the default branch's copy of both the workflow and the command file, so the label trigger only takes effect on merge — workflow_dispatch is the branch-testable path.

Related

Closes #280

• Rename Notes for SDD to Commentary, and give the section a boundary and a filter #279 (PR Commentary Section #282) — rewrote the review's Commentary section; had to land first, and the audience correction here applies to its text.
• Make a /speckit.reviewissue comment triageable, and stop the category sweep inflating it #281 — reshapes how gaps are presented and ordered; downstream of what this settles about the comment's lifetime.
• docs/change-intent-records/2026-09-11-confirmed-decisions-replace-the-review.md — new CIR: decisions, constraints, four known residuals.
• docs/change-intent-records/2026-09-07-automated-prespec-review.md — annotated in place where this supersedes it.
• docs/study/280.md — what surprised this work: 8 Execution, 1 Plan-spec, 1 Codebase.

[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 5m 24s —— View job

---

Review complete

• Gather context (PR body, changed files, diff)
• Review .claude/commands/speckit.confirmissue.md
• Review .claude/commands/speckit.reviewissue.md
• Review .github/workflows/speckit-reviewissue.yml
• Review docs changes (agentic-workflow, CIRs, study)
• Check for bugs, security concerns, CLAUDE.md/constitution adherence
• Check test coverage / RED-GREEN evidence
• Check spec/test-plan consistency
• Post final review with merge recommendation

---

Bugs / correctness

This is a well-defended design, and the PR's own docs/study/280.md already documents (and this branch already fixes, in 30acffd) the three real blockers a prior review pass found — node-id vs REST-id for deletion, the fail-open jq -e label guard, and the stale-snapshot/live-read mismatch. I independently re-read the current state of each fix in .claude/commands/speckit.confirmissue.md and .github/workflows/speckit-reviewissue.yml (via git diff origin/main...HEAD) and they hold up:

• Step 1 now resolves the comment over REST and selects by created_at, not node id.
• The workflow's ready=$(gh issue view ... --jq ...) assignment sits outside any if, so under bash's default -e a gh/jq failure aborts the step rather than silently reading as "not ready" (verified: an assignment whose command substitution fails triggers errexit at top level, unlike the same check written inside a conditional).
• Both ready and the sentinel/body checks are read live, in the same step, addressing the stale-snapshot pairing.
• Deletion (step 7) reads ready and ## Confirmed decisions back from the issue immediately before deleting, rather than trusting step 6's (deliberately non-fatal) result.

One latent edge case I don't see called out anywhere: gh api --paginate ... --jq 'max_by(.created_at)' in step 1. gh api --paginate combined with --jq evaluates the jq expression once per page, not once over the concatenated result — so max_by only finds the max within a page. In practice this is very low-risk here (the filter narrows to comments containing ``, and there's normally at most one live at a time since reruns edit in place), so it would only misfire on an issue with enough total comments to span multiple pages (default 30/page) and more than one marker comment landing on different pages. Given the emphasis this PR places on getting "most recent by timestamp" right (it's literally the fix for a previous bug), it's worth a one-line footnote acknowledging the boundary, even if not worth a code change today.

Security

No new secret handling; token usage (github.token/GH_TOKEN) and the sender.login == 'FrankRay78' trigger gate are unchanged. The one destructive action (comment deletion) is triple-gated (label + body read-back, done last, never touched on any hard-stop path) and scoped to a REST DELETE by numeric id resolved server-side, not by interpolating untrusted issue content into a shell command. Nothing flagged.

CLAUDE.md / constitution adherence

Good. This is a tooling/config change and correctly uses Principle I's carve-out rather than inventing a bespoke test — the PR body documents real tool failure→pass evidence. The new CIR follows the established format, in-place-annotates the superseded 2026-09-07 record rather than deleting history, and docs/agentic-workflow-NetPace.md was updated in lockstep (per this repo's own feedback_release_pipeline_doc-style convention of keeping docs in sync with mechanism changes). No stale specs/280-* reference left behind (checked — none exists), consistent with feedback_no_spec_references.

Test coverage

No C# changed; dotnet build/dotnet test green per the PR body. The new guard logic lives entirely in speckit-reviewissue.yml bash and the command markdown, and — unlike this repo's three committed hooks (no-skipped-tests.sh, green-gate.sh, traceability-gate.sh), each of which ships a *.tests.sh matrix — there's no repeatable, committed test for the new label-precedence guard. That said, this matches the project's own recorded preference (feedback_guard_outgrows_the_fix: don't build verification scaffolding bigger than the fix without discussion), and the PR is explicit about the gap: three "Still to do on a real issue" checklist items are left unchecked, including running /speckit.confirmissue end-to-end against a throwaway issue — i.e., the one live exercise of the irreversible delete path. That's the one item I'd want done (or at least strongly prioritized) before or immediately after merge, since deletion is silent and has no undo.

Spec/test-plan mismatches

None found — no active specs/280-* folder, and the issue's own technical sketch (which proposed "ready OR ## Confirmed decisions" as the marker) is explicitly and correctly overridden in CIR decision 1, with the conflict against AC #8 spelled out. That's a good example of catching a spec/implementation tension rather than silently building around it.

Note unrelated to the PR's content

While reviewing, this checkout's working tree had uncommitted local diffs on exactly .claude/commands/speckit.confirmissue.md and .claude/commands/speckit.reviewissue.md, reverting them to pre-55324ea content, even though git log and git diff origin/main...HEAD both show the full fix present at HEAD (30acffd). This looks like a checkout/caching artifact of the review sandbox rather than anything in the PR — my review is based on the committed diff, which is what would actually merge — but flagging it in case it indicates something worth a second look on your end.

Recommendation

Lean toward merge, conditional on completing the PR's own outstanding checklist — specifically the live /speckit.confirmissue run that exercises the delete path, since that's the one irreversible action in the whole design and it's the one thing not yet verified against a real issue. The design itself is sound, defensively ordered, and already incorporates fixes for every blocker a prior review round found. The gh api --paginate+--jq per-page caveat is worth a one-line doc footnote but isn't a blocker given how narrow the trigger condition is.