Reviewer requests must be blocked (server + UI) on merged/closed PRs #599

New issue

Closed

opened 2026-09-15 21:17:51 +00:00 by crueber · 1 comment

crueber commented

2026-09-15 21:17:51 +00:00

Owner

Reviewer requests must be blocked (server + UI) on merged/closed PRs

What's requested

Adding and removing requested reviewers must be refused — server-side with a 4xx, and in the UI by hiding/disabling the reviewers picker — once a pull request is closed or merged.

Evidence (static analysis, current tree; no local repro per standing rule)

Server — no state check at all. internal/review/threads.go:

AddRequests (POST …/api/pulls/{num}/review-requests, line 507) validates auth, read access, author-or-triage+ role, and reviewer list shape — but never inspects the PR's state. It calls s.prHeadOf(...) (line 514) which returns the PRHeader and PRSidecar, both of which carry exactly the fields needed: PRHeader.State (internal/review/model.go:183) and PRSidecar.Merged (internal/review/model.go:213). The check is missing, not hard.
RemoveRequests (DELETE, line 570) has the same gap (line 577).
On merge the header state goes to closed and the sidecar's Merged flag is set (internal/pulls/merge.go step 6, "merged event (state → closed …)"), so today a merged PR happily accepts POST/DELETE on review-requests.json, mutating the CAS'd current-state index and emitting review_requested notifications for a PR that can never be reviewed again.

UI — role-gated only. web/src/pages/Pull.jsx:

canReview = () => roleAtLeast(role(), "write") (line 1061) is the only gate; the reviewers panel is rendered with canEdit={canReview()} (line 1415) regardless of PR state, so the "request a reviewer…" input stays live on merged/closed PRs.
The file already has the state-gating idiom to copy: line 1441 <Show when={thread()?.state === "open" || pr()?.merged}>.

Spec note: docs/features/04_code_review.md §5 ("Review requests") specifies auth and dedup rules but is silent on PR lifecycle state — this ticket adds the rule rather than amending a stated one.

Architecture notes

The refusal belongs in AddRequests/RemoveRequests right after the prHeadOf call: if h.State != StateOpen || side.Merged → 422 ErrUnprocessable ("review requests are only accepted on open pull requests"), consistent with how the review surface words other 422s. GET stays allowed (history is still viewable).
Decide and note whether self-removal (the selfOnly branch, threads.go:597) is also blocked — recommend yes, uniformly.
The summary refresh + notify emit after mutation must be skipped on the refusal path (natural if the check precedes them).
UI: gate the picker's editable affordance on the PR being open (state is already on the page), and keep the requested-reviewer chips visible read-only. Follow the existing state-gating <Show> pattern in the same file.

Acceptance criteria

POST …/review-requests on a closed PR returns 422 (message names the reason); on a merged PR likewise.
DELETE …/review-requests on a closed/merged PR returns 422, including the self-removal path (per the decision noted above).
GET …/review-requests still works on closed/merged PRs.
Service tests cover add/remove on open vs closed vs merged PRs.
The reviewers picker input no longer renders (or renders disabled with a reason) on closed/merged PRs in the SPA; requested-reviewer chips remain visible.
No changes to the review-requests.json shape or the notify event classes for open PRs.

# Reviewer requests must be blocked (server + UI) on merged/closed PRs ## What's requested Adding and removing requested reviewers must be refused — server-side with a 4xx, and in the UI by hiding/disabling the reviewers picker — once a pull request is closed or merged. ## Evidence (static analysis, current tree; no local repro per standing rule) **Server — no state check at all.** `internal/review/threads.go`: - `AddRequests` (POST `…/api/pulls/{num}/review-requests`, line 507) validates auth, read access, author-or-triage+ role, and reviewer list shape — but never inspects the PR's state. It calls `s.prHeadOf(...)` (line 514) which returns the `PRHeader` and `PRSidecar`, both of which carry exactly the fields needed: `PRHeader.State` (`internal/review/model.go:183`) and `PRSidecar.Merged` (`internal/review/model.go:213`). The check is missing, not hard. - `RemoveRequests` (DELETE, line 570) has the same gap (line 577). - On merge the header state goes to `closed` and the sidecar's `Merged` flag is set (`internal/pulls/merge.go` step 6, "merged event (state → closed …)"), so today a merged PR happily accepts `POST`/`DELETE` on `review-requests.json`, mutating the CAS'd current-state index and emitting `review_requested` notifications for a PR that can never be reviewed again. **UI — role-gated only.** `web/src/pages/Pull.jsx`: - `canReview = () => roleAtLeast(role(), "write")` (line 1061) is the only gate; the reviewers panel is rendered with `canEdit={canReview()}` (line 1415) regardless of PR state, so the "request a reviewer…" input stays live on merged/closed PRs. - The file already has the state-gating idiom to copy: line 1441 `<Show when={thread()?.state === "open" || pr()?.merged}>`. **Spec note:** `docs/features/04_code_review.md` §5 ("Review requests") specifies auth and dedup rules but is silent on PR lifecycle state — this ticket adds the rule rather than amending a stated one. ## Architecture notes - The refusal belongs in `AddRequests`/`RemoveRequests` right after the `prHeadOf` call: `if h.State != StateOpen || side.Merged → 422 ErrUnprocessable` ("review requests are only accepted on open pull requests"), consistent with how the review surface words other 422s. `GET` stays allowed (history is still viewable). - Decide and note whether self-removal (the `selfOnly` branch, threads.go:597) is also blocked — recommend yes, uniformly. - The summary refresh + notify emit after mutation must be skipped on the refusal path (natural if the check precedes them). - UI: gate the picker's editable affordance on the PR being open (state is already on the page), and keep the requested-reviewer chips visible read-only. Follow the existing state-gating `<Show>` pattern in the same file. ## Acceptance criteria - [ ] `POST …/review-requests` on a closed PR returns 422 (message names the reason); on a merged PR likewise. - [ ] `DELETE …/review-requests` on a closed/merged PR returns 422, including the self-removal path (per the decision noted above). - [ ] `GET …/review-requests` still works on closed/merged PRs. - [ ] Service tests cover add/remove on open vs closed vs merged PRs. - [ ] The reviewers picker input no longer renders (or renders disabled with a reason) on closed/merged PRs in the SPA; requested-reviewer chips remain visible. - [ ] No changes to the `review-requests.json` shape or the notify event classes for open PRs.

crueber added the

bug

pull requests

labels

2026-09-15 21:17:57 +00:00

crueber added this to the v1 milestone

2026-09-15 21:17:57 +00:00

crueber added the

issues

label

2026-09-15 21:18:23 +00:00

crueber referenced this issue

2026-09-15 21:29:00 +00:00

Terminal PRs (merged/closed) show a merged/closed status in the sidebar instead of the Review Summary, and hide the moot Mergeability section #602

crueber referenced this issue from a commit

2026-09-15 22:05:01 +00:00

Block review requests on merged/closed PRs (Fix #599)

crueber referenced this issue from a pull request that will close it,

2026-09-15 22:05:07 +00:00

Block review requests on merged/closed PRs (Fix #599) #607

crueber referenced this issue

2026-09-15 22:06:57 +00:00

Block review requests on merged/closed PRs (Fix #599) #607

crueber closed this issue

2026-09-15 22:07:27 +00:00

crueber referenced this issue from a commit

2026-09-15 22:07:27 +00:00

Merge pull request 'Block review requests on merged/closed PRs (Fix #599)' (#607) from fix/issue-599 into main

crueber commented

2026-09-15 22:07:28 +00:00

Author

Owner

Fixed by #607 (merged): AddRequests/RemoveRequests refuse 422 on closed/merged PRs (incl. uniform self-removal) via the shared threadLocked predicate — no new reads, before summary/notify; GET stays open. Picker gated on live lock (chips stay visible); spec §5 rule added. Verified: review 96.0% cover, -race green, 1538 web unit green (smoke excluded, pre-existing), vite/esbuild green, independent review APPROVE.