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

Merged

crueber merged 1 commit from fix/issue-599 into main

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

crueber commented

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

Owner

Summary

Reviewer requests (POST/DELETE review-requests) plus the PR-page picker stayed live on merged/closed PRs. This change gates both ends on open-PR state.

Server (`internal/review/threads.go`)

AddRequests / RemoveRequests refuse 422 ErrUnprocessable (review requests are only accepted on open pull requests) when threadLocked(h, side) on the already-loaded prHeadOf header + sidecar — zero new store reads (law 6), placed before summary refresh + notify emit. GET/suggest stay allowed.
Self-removal blocked uniformly (named decision, spec §5: a terminal PR's request list is frozen curation, not an inbox the requestee still owns).

Client (`Pull.jsx` + `pull-state.js`)

New headless reviewRequestsEditable(canEdit, thread, pr) (role first, then the #594 live lock — reopen restores with no reload).
ReviewersPanel: picker <Show> reads editable(); chips <ul> moved outside the gate, × follows the same gate (terminal PRs read read-only).

Spec (law 12, same commit)

docs/features/04_code_review.md: §5 lifecycle rule (addition, not amendment), §7 table 422 notes, Decisions entry.
docs/go/12_web_ui.md: Decisions entry.

Verification

go test ./internal/review/... -race: pass (new requests599_test.go: open vs closed vs merged incl. self-removal + GET-allowed + reopen; open-path wire-shape + notify-class pins).
review coverage 96.0% (≥95% gate).
gofmt clean, go vet clean.
web unit full-minus-smoke: 1538/1538 pass (new review-requests-599.test.js, 4 tests).
vite build green, esbuild SDK bundle green, web/dist/.keep restored.

## Summary Reviewer requests (POST/DELETE review-requests) plus the PR-page picker stayed live on merged/closed PRs. This change gates both ends on open-PR state. ## Server (`internal/review/threads.go`) - `AddRequests` / `RemoveRequests` refuse `422 ErrUnprocessable` (`review requests are only accepted on open pull requests`) when `threadLocked(h, side)` on the already-loaded `prHeadOf` header + sidecar — zero new store reads (law 6), placed before summary refresh + notify emit. GET/suggest stay allowed. - Self-removal blocked uniformly (named decision, spec §5: a terminal PR's request list is frozen curation, not an inbox the requestee still owns). ## Client (`Pull.jsx` + `pull-state.js`) - New headless `reviewRequestsEditable(canEdit, thread, pr)` (role first, then the #594 live lock — reopen restores with no reload). - `ReviewersPanel`: picker `<Show>` reads `editable()`; chips `<ul>` moved outside the gate, × follows the same gate (terminal PRs read read-only). ## Spec (law 12, same commit) - `docs/features/04_code_review.md`: §5 lifecycle rule (addition, not amendment), §7 table 422 notes, Decisions entry. - `docs/go/12_web_ui.md`: Decisions entry. ## Verification - `go test ./internal/review/... -race`: pass (new `requests599_test.go`: open vs closed vs merged incl. self-removal + GET-allowed + reopen; open-path wire-shape + notify-class pins). - review coverage 96.0% (≥95% gate). - gofmt clean, `go vet` clean. - web unit full-minus-smoke: 1538/1538 pass (new `review-requests-599.test.js`, 4 tests). - `vite build` green, esbuild SDK bundle green, `web/dist/.keep` restored.

crueber added 1 commit

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

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

Server: AddRequests/RemoveRequests refuse 422 ErrUnprocessable
('review requests are only accepted on open pull requests') on
closed/merged PRs, gated on the prHeadOf-loaded header+sidecar
via threadLocked — zero new store reads, before summary refresh
+ notify emit; GET/suggest stay allowed. Self-removal blocked
uniformly (named decision: terminal request list is frozen
curation). Client: ReviewersPanel picker gated by headless
reviewRequestsEditable(role + live thread/pr lock); chips stay
read-only. Docs: 04 §5/§7 + Decisions, 12_web_ui Decisions.

crueber commented

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

Author

Owner

APPROVED — independent review of fix/issue-599 (0679f4d) vs #599.

Verified in /tmp/walhub-599:

Gate placement: threadLocked(h, side) immediately after prHeadOf in BOTH AddRequests (threads.go:530) and RemoveRequests (:603) — before the selfOnly branch (:622), before refreshSummary + notify emit (:576/:579, :652/:655). Refusal stores nothing and emits nothing (pinned: notified count unchanged + GET shows unmutated list).
Predicate reuse: threadLocked from #594 (service.go:116) — h.State != open OR (side != nil && side.Merged). Covers closed, merged, and stamp-in-flight (merged=true/header open, tested); no over-block (only other PR states are open/closed; GET/Suggest never call it; implicit removeRequester on submit ungated, correct).
422 mapping: fmt.Errorf %w ErrUnprocessable → statusFor 422 (model.go:59); HTTP layer writePlain(w, statusFor(err)) (http.go:135, :629/:647) — not 500. Helper asserts errors.Is + status 422 + message names open-PR rule.
GET/suggest ungated: GetRequests (:459) and Suggest (:697) have no gate; tests assert GET works on closed and merged.
Self-removal uniformly blocked: gate precedes selfOnly branch; closed + merged self-removal both assert 422 (decision coherent, spec §5 rationale recorded).
Client: reviewRequestsEditable(canEdit, thread, pr) = role first, then !pullCommentLock().locked (pull-state.js:238). Panel passes live thread()/pr() props fed by useCollabStream([pull,review,thread,check]) — same source as commentLock() — so reopen restores picker with no reload. gateNote: role note when !canEdit, else live lockReason.
Chips:
- Open-PR pins: wire shape (Principal/By/At) + notify classes review_requested/review_request_removed asserted; no wire/notify change.
- Docs (law 12): features/04 §5 lifecycle rule (addition, noted as such) + §7 table 422 notes + Decisions entry; go/12_web_ui Decisions entry. No contradiction of stated rules.
- Coverage: go test ./internal/review/ -cover = 96.0% (≥95%). -race pass. web: 4/4 new + 1538/1538 full-minus-smoke pass. gofmt/vet clean (Go files).
- Pre-fix check: new tests fail on origin/main threads.go (closed add + merged add: want 422, got nil) — true regression tests.
- No new deps: diff touches 8 files only; no go.mod/package.json/pnpm changes.
One non-blocking nit: merged-PR picker fallback reuses the #594 reason ('Merged — commenting is locked') for a reviewer picker — slightly off-noun but consistent shared lock language; left as-is, no churn.

No fix commits — worktree clean. Verdict: APPROVED.

APPROVED — independent review of fix/issue-599 (0679f4d) vs #599. Verified in /tmp/walhub-599: - Gate placement: threadLocked(h, side) immediately after prHeadOf in BOTH AddRequests (threads.go:530) and RemoveRequests (:603) — before the selfOnly branch (:622), before refreshSummary + notify emit (:576/:579, :652/:655). Refusal stores nothing and emits nothing (pinned: notified count unchanged + GET shows unmutated list). - Predicate reuse: threadLocked from #594 (service.go:116) — h.State != open OR (side != nil && side.Merged). Covers closed, merged, and stamp-in-flight (merged=true/header open, tested); no over-block (only other PR states are open/closed; GET/Suggest never call it; implicit removeRequester on submit ungated, correct). - 422 mapping: fmt.Errorf %w ErrUnprocessable → statusFor 422 (model.go:59); HTTP layer writePlain(w, statusFor(err)) (http.go:135, :629/:647) — not 500. Helper asserts errors.Is + status 422 + message names open-PR rule. - GET/suggest ungated: GetRequests (:459) and Suggest (:697) have no gate; tests assert GET works on closed and merged. - Self-removal uniformly blocked: gate precedes selfOnly branch; closed + merged self-removal both assert 422 (decision coherent, spec §5 rationale recorded). - Client: reviewRequestsEditable(canEdit, thread, pr) = role first, then !pullCommentLock().locked (pull-state.js:238). Panel passes live thread()/pr() props fed by useCollabStream([pull,review,thread,check]) — same source as commentLock() — so reopen restores picker with no reload. gateNote: role note when !canEdit, else live lockReason. - Chips: <ul> moved outside picker <Show> (Pull.jsx:284), × follows editable() (:289) — terminal PRs read read-only, request list stays visible. - Open-PR pins: wire shape (Principal/By/At) + notify classes review_requested/review_request_removed asserted; no wire/notify change. - Docs (law 12): features/04 §5 lifecycle rule (addition, noted as such) + §7 table 422 notes + Decisions entry; go/12_web_ui Decisions entry. No contradiction of stated rules. - Coverage: go test ./internal/review/ -cover = 96.0% (≥95%). -race pass. web: 4/4 new + 1538/1538 full-minus-smoke pass. gofmt/vet clean (Go files). - Pre-fix check: new tests fail on origin/main threads.go (closed add + merged add: want 422, got nil) — true regression tests. - No new deps: diff touches 8 files only; no go.mod/package.json/pnpm changes. One non-blocking nit: merged-PR picker fallback reuses the #594 reason ('Merged — commenting is locked') for a reviewer picker — slightly off-noun but consistent shared lock language; left as-is, no churn. No fix commits — worktree clean. Verdict: APPROVED.

crueber merged commit 1c21d2c004 into main

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

crueber referenced this pull request 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 referenced this pull request

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

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