Remove request-review label at receiver review-start

[nsheaps]

Summary

• Reverses part of the 2026-05-23 redesign of the review-dispatch pipeline: the decider still never touches the request-review label (routing-only, unchanged), but the receiver now removes it right when a review actually starts (after posting the in_progress check, before dismissing prior approvals).
• Without this, a still-draft PR's label just stays "on" after the first review runs — there's no clear signal to distinguish "review already requested and handled" from "please look again." Removing it at review-start makes the label reusable: re-apply request-review after addressing feedback (fixing code, or just replying to justify a comment) to request another round.
• New script: .github/scripts/review-receiver/remove-request-review-label.sh (idempotent — no-op if the label is already absent).
• review-receiver.yaml: wires the removal step into the review job using the automation-App token (same identity that edits labels in the decider), updates header comments.
• plugins/claude-code/review-utils/specs/review-dispatch.md: documents the reversal with a dated note, updates the implementation map and the receiver stage-by-stage flow.

The consumer-side gate logic (dispatch-review.yaml, requiring the label only to force a review on a still-draft PR) is unchanged — this only affects what the receiver does with the label once a review starts.

Test plan

• python3 -c "import yaml; yaml.safe_load(open('.github/workflows/review-receiver.yaml'))" — YAML parses (done locally)
• End-to-end: open a draft PR in a consumer repo, apply request-review, confirm the review runs and the label is removed once the check flips to in_progress
• Re-apply request-review after pushing a fix, confirm a second review round fires and the label is removed again

---

Generated by Claude Code

[github-actions]

Plugin Version Preview

Preview only — plugin versions and marketplace.json are bumped automatically on merge to main, not in this PR. Manual bumps to higher versions are preserved. See the file annotations for the pending change on each plugin.json.

Plugin	Base	Current	Action
review-utils	0.2.3	0.2.4	Auto-bumped

[nsheaps]

CI red on lint (both failed runs) — failing during the checkout-as-app GitHub App auth step with tls: failed to verify certificate: x509: certificate is not valid for any names, but wanted to match api.github.com. That's a TLS handshake failure against api.github.com itself, unrelated to this PR's diff (one new shell script + two doc edits). Looks like a transient runner/network issue, not something introduced here. Will re-check on the next event; will re-request review once CI recovers.

---

Generated by Claude Code

[nsheaps]

🤖 The "AI Code Review" check failure on this PR (check run 93983045453) is a known systemic/intermittent pipeline bug — the review job completes without writing review-metrics.yaml, likely because the agent used a gh/git-based write instead of the required Write tool and got silently denied. Tracked in nsheaps/agents#337, which documents this same failure on three separate PRs (including this one). Re-applying request-review to retry.

---

Generated by Claude Code

Dismissed: new commits since last approval require re-review.

Dismissed: new commits since last approval require re-review.

Dismissed: new commits since last approval require re-review.

Dismissed: new commits since last approval require re-review.

Dismissed: new commits since last approval require re-review.

Dismissed: new commits since last approval require re-review.

[nsheaps]

Why are we writing that this is superseeded rather than just removing or am I misreading this... This feels more appropriate for a design doc, spec, decision log...anywhere but here

[nsheaps]

🤖 Agreed — you're not misreading it, and the "superseded" note was the wrong call. I checked plugins/claude-code/review-utils/specs/review-dispatch.md: its §Status already carries a dated decision log for exactly this (the 2026-05-23 redesign entry and a new 2026-08-12 partial-reversal entry I just added), so the workflow file didn't need to re-host that history at all.

Fixed in agents@286d248: removed the outdated "2026-05-23 redesign" bullet block and the "SUPERSEDED" patch-note, replaced with one short paragraph describing current behavior plus a pointer to the spec's §Status for the "why."

---

Generated by Claude Code

[nsheaps]

🤖 Correction to my reply above: the 2026-08-12 §Status entry in review-dispatch.md was already there from an earlier session — I read it, I didn't add it just now. Only review-receiver.yaml's header comment changed in 286d248. Doesn't change the fix, just correcting the inaccurate "I just added" claim.

---

Generated by Claude Code

Dismissed: new commits since last approval require re-review.

[henry-nsheaps]

Review Summary

Verdict: 🟢 APPROVE — re-verification against new HEAD 286d248. Since my last approval at cc29050, one commit landed (286d248) that addresses nsheaps's inline review on the header's SUPERSEDED annotation. The fix is clean: the "SUPERSEDED" patch-note and the old 2026-05-23 redesign bullet block are gone from review-receiver.yaml, replaced with a short paragraph describing current behavior plus a parenthetical pointer to plugins/claude-code/review-utils/specs/review-dispatch.md's §Status (which already carries the dated decision log). Design-decision history stays in the spec; the workflow file describes only current behavior. No script or spec drift; all prior inline threads remain resolved.

Re-verification

What I checked on the new commit (click to expand)

1. New commit. 286d248 — Trim SUPERSEDED comment block from review-receiver.yaml header. Only one file attributable to this commit per gh pr files: .github/workflows/review-receiver.yaml.
2. Header trim. Fetched the file at HEAD and read lines 1–50:
   • Gone: the 2026-05-23 redesign (Nate 18:08Z): bullet block and the SUPERSEDED (handler instruction, this session): block that nsheaps flagged.
   • Added: a (§Status has the dated design-decision log …) parenthetical under the Spec: line, and a one-paragraph description of current behavior of the skip job's non-touch of the label. No history, no dated notes, no "why we changed our mind" — all of that lives in the spec now, exactly as nsheaps requested.
   • Flow list is intact and re-numbered correctly (steps 1–9, with step 4 Remove the request-review label… and subsequent steps shifted down as expected).
3. Script + spec unchanged since prior approval. .github/scripts/review-receiver/remove-request-review-label.sh line 29 still >/dev/null || true (stderr surfaces for auth/rate-limit breakage, exit swallowed for 404 idempotence — the minimal observability fix from 7b9dac4). plugins/claude-code/review-utils/specs/review-dispatch.md still carries the 2026-08-12 partial reversal note in §Status, the implementation-map row, and the renumbered stage-by-stage list (bullets 4 = label removal, 5a = metrics hook merged in from main, cross-references to bullets 7 & 8 correct).
4. Inline threads. All three of my prior threads still isResolved: true (P2 swallowed failures, P2 URL encoding, P3 praise on ordering) with the author's acks. The SUPERSEDED thread is functionally addressed by 286d248 (the code the comment pointed at is gone, so the thread is naturally outdated) — leaving thread state to the author since it's his own thread.
5. PR description accuracy. PR body still accurately describes the change: decider untouched, receiver removes label at review-start, three files, script is idempotent. No drift from what's shipped.
6. Trigger source. This run was legitimately triggered by the new 286d248 commit — not another instance of the auto-dismissal loop I flagged in an earlier observation. Real re-review, not churn.

Strengths ✅ (carried over)

• Symmetric, single-purpose change. Receiver flips one behavior; decider still never touches the label; the spec's 2026-08-12 §Status entry narrates which half of the 2026-05-23 invariant still holds.
• Docs match code. Renumbered receiver flow (steps 4–9), updated skip: job comment, and the new implementation-map row all agree with what the workflow does. The 286d248 trim keeps the header lean without losing the "why" — the spec still has it, and there's a pointer.
• Right token, right place. steps.auth.outputs.token (automation App) — same identity that edits labels in the decider — keeps label edits a routing concern. Approval dismissal continues to use the separate review-App token.
• Minimal, correct observability fix. 7b9dac4's >/dev/null || true (dropping 2>&1) lets gh api's stderr surface for auth/rate-limit/API-shape failures while preserving 404 idempotence.
• Comment discipline (new). 286d248 is a good example of the "history in the spec, current behavior in the file" principle — the workflow header no longer accumulates dated redesign notes over time.

Follow-ups

None outstanding on this PR's scope. All prior inline threads remain resolved; the SUPERSEDED-annotation concern is addressed in code by 286d248.

Recommendation

APPROVE. Ship once CI recovers from the checkout-as-app TLS flake (author's note) and the metrics-file emission systemic bug (#337) doesn't bite again. Post-merge validation per the PR body: draft PR → apply request-review → confirm removal on in_progress → re-apply → confirm round 2 fires and clears.

_{🤖 Automated review by @henry-nsheaps · workflow run}