Automated Issue Review

[FrankRay78]

Why

/speckit.reviewissue is the pre-specification gate, and it is the one step in the loop whose cost is the wait — it carries the codebase-grounding pass. Running it only from a developer machine means the first step of a three-step loop gates the other two: an issue raised from a phone sits untouched until the author sits down. Labelling an issue review now runs it unattended, so the gap analysis is waiting rather than requested.

What changes

• Applying the review label to an issue produces a pre-specification gap analysis as a comment on that issue — numbered gaps, a recommendation per gap, an inline answer slot — grounded in this codebase's real paths and conventions. The author answers inline and runs /speckit.confirmissue exactly as before.
• The label is the state. A green run always ends with the label removed, so labelled = review pending, unlabelled + a <!-- speckit:review --> comment = done, still labelled = the run did not complete. There is no failure comment; GitHub's failed-run notification is the alert.
• Labelling an issue that already carries a review posts nothing and clears the label, rather than posting a second review that would orphan answers already written against the first.
• claude.yml is untouched, so @claude on issues and PRs behaves exactly as before.

The review logic itself is not duplicated — the workflow prompt reads .claude/commands/speckit.reviewissue.md and applies it, so the analysis stays single-sourced and a change to the command file takes effect without touching the workflow.

Non-obvious things a reviewer should know

Three deliberate deviations from the issue's Confirmed decisions, each because the decision as written was not implementable or did not hold:

1. The workflow snapshots the issue with gh instead of reading the context the action injects. The confirmed decision said injection made fetching unnecessary. It does not: the action sanitises injected content by stripping HTML comments, which would destroy the <!-- speckit:review --> sentinel that the guard, /speckit.confirmissue, and the entire design key on. Separately, workflow_dispatch carries no issue payload to inject at all.
2. The if: gained a workflow_dispatch disjunct. The confirmed expression alone would have made the dispatch path — which the same decision mandates — unreachable.
3. --allowedTools gained Read,Glob,Grep,Write. The prompt requires the Write tool, and the whole codebase-grounding pass requires reads. The confirmed decision was about which writes to GitHub Claude gets; both Bash(gh …) grants are unchanged.

Two defects found in review that broke the contract the confirmed decisions describe, both fixed in b1d6d00:

• claude-code-action exits green whenever the model finishes its turn — a failed gh issue comment is tool output the model reads, not a step failure. A prompt instruction saying "let the job fail" cannot be honoured. So the run that posted nothing went green and notified nobody, which is precisely the state "GitHub's failed-run notification is the alert" was meant to cover. There is now a verification step that re-reads the issue and fails the job unless the sentinel is present and the label is gone.
• The already-reviewed guard originally left the label attached, producing labelled + review comment — a state the doc's own model reads as "pending", permanently, and reachable by the ordinary gesture of reviewing locally then labelling. The guard is now a run: step (a model can skip a prompt instruction) and clears the label.

Known residuals, all recorded in the CIR:

• The tool allowlist is not a boundary. --allowedTools adds to the permission rules Claude Code loads from the checked-out .claude/settings.json, which already grants Edit(**), Bash(gh:*) and defaultMode: acceptEdits. The real grant is every gh subcommand, and the prompt's "do not edit the issue body" is a request, not an enforced limit. Narrowing it properly means changing settings that govern every local session — separate work.
• The snapshot bypasses the action's sanitiser. Keeping the sentinel intact necessarily means the raw, attacker-authorable issue body reaches the model unsanitised while it holds an issues: write token. The label gate is the mitigation: untrusted text only reaches the model if a maintainer deliberately labels it.
• A label applied by any account other than FrankRay78 is silent. The job-level if: means GitHub creates no run, so nothing appears in the Actions tab and no notification fires — yet the resulting state (labelled, no review) is the one reserved for a failed run. The gate expression is pinned by the confirmed decisions, so this is flagged rather than changed.

fetch-depth: 0 and id-token: write are both unused here and were flagged in review. Kept deliberately for parity with the known-working claude.yml: this workflow cannot be triggered before merge, so changes were confined to correctness rather than untested tidying.

RED–GREEN evidence (Constitution Principle I, configuration/tooling carve-out)

The gate is the real trigger, not a hand-rolled test. No bespoke test was written to stand in for GitHub Actions.

RED, before the change — parsing every workflow's on: block:

Workflows triggered by the issues event: NONE
$ gh run list --limit 50 --jq '[.[]|select(.event=="issues")]|length'
0

GREEN, structural, after:

.github/workflows/speckit-reviewissue.yml: on = ['issues', 'workflow_dispatch']
issues trigger types: ['labeled']

GREEN, end-to-end, is merge-then-verify and is still outstanding — no event can trigger a new workflow file from a feature branch. Per the issue's own confirmed decision, the completing evidence is the real label trigger observed working on main against a throwaway issue. I will link that run here after merge.

How to verify

• .claude/commands/speckit.reviewissue.md is unmodified — the analysis must stay single-sourced, and the prompt delegates rather than restating it.
• The prompt's three declared deltas match the command file: step 1 (fetch/mode detection, but not its "don't re-raise a resolved gap" rule, which is explicitly retained), step 4's gh repo view default-branch resolution, step 5's heredoc.
• claude.yml is untouched; mention @claude on any issue or PR and confirm unchanged behaviour.
• After merge: label a throwaway issue review. Confirm a review comment appears citing at least three repo-specific paths, and that the review label is removed.
• After merge: re-apply review to that same issue. Confirm no second comment is posted and the label is cleared again.
• After merge: confirm the run that produced the review is green, and that workflow_dispatch with an issue_number input works against a branch ref.

Related

Closes #271

• .claude/commands/speckit.reviewissue.md — the review logic this automates.
• .claude/commands/speckit.confirmissue.md — the downstream step that consumes the review comment; it finds the review by sentinel and does not filter on author, which is what lets a CI-posted comment be folded in unchanged.
• docs/change-intent-records/2026-09-07-automated-prespec-review.md — decisions and residuals.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YaRuiUQHdPMeGugFuvGfeK

[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 3m 17s —— View job

---

Review of #276 — Automated Issue Review

• Gather context (diff, changed files, CLAUDE.md/constitution)
• Review .github/workflows/speckit-reviewissue.yml for bugs/security
• Check adherence to project conventions
• Check test coverage for new behaviour
• Check spec/CIR consistency
• Post final review with merge recommendation

I read all three changed files (speckit-reviewissue.yml, the doc update, the CIR), the unmodified command files the workflow delegates to (speckit.reviewissue.md, speckit.confirmissue.md), .claude/settings.json, claude.yml, and the b1d6d00 fix commit that preceded this comment (already folds in a prior clean-context review's findings).

Bugs / correctness

None found. I traced the control flow carefully:

• The if: gate (workflow_dispatch || (label == 'review' && sender == 'FrankRay78')) is correct — sender is the label-applier, not the issue author, matching the public-repo rationale.
• env.ISSUE_NUMBER and the concurrency.group both use github.event.issue.number || inputs.issue_number; the inputs context is valid for workflow_dispatch outside step scope, so this resolves correctly on both trigger paths.
• The guard step (id: guard) and the two conditioned steps downstream (if: steps.guard.outputs.already_reviewed != 'true') are correctly wired, and all the if grep -qF ... / if jq -e ... constructs are safe under the runner's default bash -e since they sit inside if conditionals.
• The two defects from the prior review round (green run on a failed post; guard leaving the label on) are genuinely fixed here — the "Verify the review posted and the label cleared" step re-reads state rather than trusting the model's narration, and the guard step is a run: step that clears the label itself.

Security

• Token scope is minimal and correct: contents: read, issues: write (id-token: write is an acknowledged unused carry-over from claude.yml parity, already flagged in the PR body).
• The real bypass — --allowedTools narrows nothing because .claude/settings.json already grants Bash(gh:*)/Edit(**)/acceptEdits — is disclosed as a known residual, and I confirmed both claims directly against settings.json. The actual boundary is the GITHUB_TOKEN's job-level permissions, not the tool allowlist; worth being precise that the allowlist is cosmetic today, which the PR already is.
• The label gate (single named account) is the real mitigation for the unsanitised issue body reaching a token-bearing model — reasonable given the alternative (losing the sentinel) isn't viable, and it's called out rather than quietly accepted.

CLAUDE.md / constitution adherence

• Principle I carve-out (config/tooling RED-GREEN) is applied correctly, not faked: no bespoke test stands in for GitHub Actions, and the PR body gives real RED (gh run list showing zero issues-triggered runs) and structural GREEN evidence from parsing the merged on: block.
• Correctly flags that true end-to-end GREEN is merge-then-verify and still outstanding, per this project's own [[feedback_rerun_tests_before_done]]-style discipline — nothing here is claimed as proven that isn't yet.
• claude.yml is confirmed byte-for-byte untouched.
• Doc updates land in docs/agentic-workflow-NetPace.md and a CIR, consistent with how this repo documents workflow-level decisions.

Test coverage

Adequate for what this change is. There's no C# production code, so xUnit coverage doesn't apply. The Principle I carve-out is the right lens, and it's satisfied per above. The one honest gap — real label-triggered E2E on main — is explicitly called "outstanding" in the PR body's own checklist rather than glossed over, which is the right way to leave it for a workflow file (this branch cannot trigger it).

Spec/CIR consistency

Checked the three declared prompt deltas against speckit.reviewissue.md line-by-line — all three match (step 1's fetch/mode-detection override while retaining the "don't re-raise a resolved gap" rule at line 55-56; step 4's gh repo view override at line 193; step 5's heredoc→Write-tool override at line 202). The CIR's "Behaviour" and "Decisions" sections match the shipped YAML for every item I checked, including the two items that were previously wrong and are now corrected (decision 1's rationale, the "author-gated" → "labeller-gated" terminology fix).

Recommendation

Approve / merge. No blockers found. The three deliberate deviations from the issue's confirmed decisions are well-argued and necessary (HTML-comment stripping would have destroyed the sentinel either way), and the two previously-found defects are genuinely fixed rather than papered over. The remaining items (allowlist being cosmetic, non-FrankRay78 labels being silent, E2E verification pending) are all disclosed as known residuals rather than surprises, which is the right way to ship a workflow file that can't be tested pre-merge.
· branch feature/271-automated-issue-review