Gate client CHANGES_REQUESTED merge-block on required-reviews rule (fixes #612) #615

Merged

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

2026-09-15 23:20:25 +00:00

crueber commented

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

Owner

Problem

UI treated any CHANGES_REQUESTED as merge-blocking (mergeState review arm, disabled button, blocked tooltip, amber line) while the server merges it when no required-reviews rule exists (verified live). Server behavior (GitHub-like, no rule → no block) stays.

Fix (client-only)

New pure requiredReviewsApplies(policy, baseRef) in web/src/lib/pull-state.js (rule presence: effect carries required-reviews + match.refs empty/exact-including base).
Pull.jsx derives requiresReviews from the fetched policy (same list as requiredChecks()) and passes it to MergeBox.
mergeState + amber blockers() + tooltip follow the gate (requiresReviews === false = known-no-rule → advisory; otherwise fail-closed block).
#588 "Changes requested" headline keeps the raw decision (informational either way). Server untouched; server-blocks-always stays rejected per #586 Decision-1b.
Simplification noted in docs: presence only (no min_approvals/stale/bypass exactness), exact ref match client-side; server authoritative.

Verification

New web/test/unit/merge-review-gate-612.test.js (15 subtests: gate matrix, mergeState/arm/blocker/tooltip pins, page wiring, headline-raw, deps/CSS/law-12).
Related suites (588/592/586/pull-state/sha-link/strategy/sentinel/pr-composer) green; full-minus-smoke 1579/1579 green (smoke excluded: needs live server, pre-existing).
vite build + esbuild SDK green; web/dist/.keep restored; go vet ./internal/... clean (no Go change).
390px reasoned (same button/line, conditional text only — no layout change).

Docs: FIXED (Forgejo #612) amendment in docs/go/12_web_ui.md same commit (law 12).

## Problem UI treated any CHANGES_REQUESTED as merge-blocking (mergeState review arm, disabled button, blocked tooltip, amber line) while the server merges it when no required-reviews rule exists (verified live). Server behavior (GitHub-like, no rule → no block) stays. ## Fix (client-only) - New pure `requiredReviewsApplies(policy, baseRef)` in `web/src/lib/pull-state.js` (rule presence: effect carries `required-reviews` + match.refs empty/exact-including base). - `Pull.jsx` derives `requiresReviews` from the fetched policy (same list as `requiredChecks()`) and passes it to `MergeBox`. - `mergeState` + amber `blockers()` + tooltip follow the gate (`requiresReviews === false` = known-no-rule → advisory; otherwise fail-closed block). - #588 "Changes requested" headline keeps the raw decision (informational either way). Server untouched; server-blocks-always stays rejected per #586 Decision-1b. - Simplification noted in docs: presence only (no min_approvals/stale/bypass exactness), exact ref match client-side; server authoritative. ## Verification - New `web/test/unit/merge-review-gate-612.test.js` (15 subtests: gate matrix, mergeState/arm/blocker/tooltip pins, page wiring, headline-raw, deps/CSS/law-12). - Related suites (588/592/586/pull-state/sha-link/strategy/sentinel/pr-composer) green; full-minus-smoke 1579/1579 green (smoke excluded: needs live server, pre-existing). - `vite build` + esbuild SDK green; `web/dist/.keep` restored; `go vet ./internal/...` clean (no Go change). - 390px reasoned (same button/line, conditional text only — no layout change). Docs: FIXED (Forgejo #612) amendment in `docs/go/12_web_ui.md` same commit (law 12).

crueber added 1 commit

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

Gate client CHANGES_REQUESTED merge-block on required-reviews rule presence (Forgejo #612 ) 5af73d8968

mergeState() blocked on any reviewDecision === CHANGES_REQUESTED while
the server merges when no required-reviews rule exists (GitHub-like).
New pure requiredReviewsApplies(policy, baseRef) in pull-state.js;
Pull.jsx derives requiresReviews from the fetched policy and passes it
to MergeBox; mergeState + amber blockers() + tooltip follow the gate.
The #588 headline stays informational. Server behavior untouched.

Docs: FIXED (Forgejo #612) amendment in docs/go/12_web_ui.md.
Tests: web/test/unit/merge-review-gate-612.test.js; full-minus-smoke
1579/1579 green; vite build + esbuild green; go vet clean.

crueber commented

2026-09-15 23:20:10 +00:00

Author

Owner

APPROVE — independent review of fix/issue-612 (5af73d8) vs #612 acceptance.

Acceptance: MET. No-rule → red phrase stays, button enables, merge succeeds (server unchanged, merges as before); with-rule → disabled + blocked tooltip + server refuses (server gate untouched). Unit tests pin both halves (merge-review-gate-612.test.js, 15 subtests green; full-minus-smoke 1579/1579 green verified in worktree, smoke excluded — needs live server, pre-existing).

requiredReviewsApplies semantics: helper checks effect-carries-required-reviews + match.refs empty/exact-includes base. Server (service.go EvaluateGate + policy.MatchingRules) uses Principal/Op match law + most-restrictive combination (max min_approvals, stale-OR, bypass-only-if-EVERY-rule). Divergence direction checked: no-rule case can no longer client-block-while-server-allows (old bug gone — helper false → gate advisory). Residual gaps are the safe directions: client-enables-while-server-refuses (min_approvals shortfall, glob-vs-exact miss, null-effect) → server authoritative, task error surfaces; client-blocks-while-server-allows only in the with-rule bypass edge → fail-closed, acceptable. Simplification is documented, not silent.
Tri-state: one nuance noted, not blocking. mergeState-level undefined → !== false → blocked (fail-closed as claimed), BUT the page path never emits undefined while loading: getPolicy() starts undefined → requiredReviewsApplies(undefined)→false → unwrapped false → enables during the policy fetch (fail-open with brief enable→disable flicker in the with-rule case). This matches the existing requiredChecks pattern (empty during load → no blockers) and fails toward the server-authoritative refusal, so I leave it as-is; the helper contract (null/undefined/empty→false) is pinned by test. Fetch-error null→false→enables and loaded-empty→enables both verified.
Purity/seams: mergeState stays pure (props only, no fetch). MergeBox has no pull-state import (#592 holds — verified). Tooltip derives from blockers() alone (no CHANGES_REQUESTED branch — verified by slice pin). Amber entry gated with the identical predicate (requiresReviews() === v!==false). Headline-vs-line split is coherent per the issue: #588 phrase always red/informational, amber line+tooltip only when actually blocking.
Sidebar #588 headline unchanged: mergeabilityDisplay untouched (raw decision → Changes requested red either way); Pull.jsx passes summary()?.decision ungated. Verified.
Server untouched (diff names: docs/go/12_web_ui.md + 3 web src + 1 new test; no Go, no package.json, no ui.css). Law 12 amendment present (#612 entry). Law 1 holds (4 runtime deps asserted, no inline style). Pre-fix fails: main has unconditional review arm (MergeBox.jsx:44) + no helper (grep 0 hits) → new gate/tooltip/amber pins fail on main. Good.
Bypass/min_approvals/stale simplification note exists in both pull-state.js docstring and the 12_web_ui.md amendment (presence-only, exact-ref, server authoritative). Verified.

No fix commit — no defect rising above note-level. The loading fail-open nuance above is the only observation; changing it would trade the current checks-consistent behavior for disable→enable flicker on every PR load, so deliberately not changed.

APPROVE — independent review of fix/issue-612 (5af73d8) vs #612 acceptance. Acceptance: MET. No-rule → red phrase stays, button enables, merge succeeds (server unchanged, merges as before); with-rule → disabled + blocked tooltip + server refuses (server gate untouched). Unit tests pin both halves (merge-review-gate-612.test.js, 15 subtests green; full-minus-smoke 1579/1579 green verified in worktree, smoke excluded — needs live server, pre-existing). 1. requiredReviewsApplies semantics: helper checks effect-carries-required-reviews + match.refs empty/exact-includes base. Server (service.go EvaluateGate + policy.MatchingRules) uses Principal/Op match law + most-restrictive combination (max min_approvals, stale-OR, bypass-only-if-EVERY-rule). Divergence direction checked: no-rule case can no longer client-block-while-server-allows (old bug gone — helper false → gate advisory). Residual gaps are the safe directions: client-enables-while-server-refuses (min_approvals shortfall, glob-vs-exact miss, null-effect) → server authoritative, task error surfaces; client-blocks-while-server-allows only in the with-rule bypass edge → fail-closed, acceptable. Simplification is documented, not silent. 2. Tri-state: one nuance noted, not blocking. mergeState-level undefined → !== false → blocked (fail-closed as claimed), BUT the page path never emits undefined while loading: getPolicy() starts undefined → requiredReviewsApplies(undefined)→false → unwrapped false → enables during the policy fetch (fail-open with brief enable→disable flicker in the with-rule case). This matches the existing requiredChecks pattern (empty during load → no blockers) and fails toward the server-authoritative refusal, so I leave it as-is; the helper contract (null/undefined/empty→false) is pinned by test. Fetch-error null→false→enables and loaded-empty→enables both verified. 3. Purity/seams: mergeState stays pure (props only, no fetch). MergeBox has no pull-state import (#592 holds — verified). Tooltip derives from blockers() alone (no CHANGES_REQUESTED branch — verified by slice pin). Amber entry gated with the identical predicate (requiresReviews() === v!==false). Headline-vs-line split is coherent per the issue: #588 phrase always red/informational, amber line+tooltip only when actually blocking. 4. Sidebar #588 headline unchanged: mergeabilityDisplay untouched (raw decision → Changes requested red either way); Pull.jsx passes summary()?.decision ungated. Verified. 5. Server untouched (diff names: docs/go/12_web_ui.md + 3 web src + 1 new test; no Go, no package.json, no ui.css). Law 12 amendment present (#612 entry). Law 1 holds (4 runtime deps asserted, no inline style). Pre-fix fails: main has unconditional review arm (MergeBox.jsx:44) + no helper (grep 0 hits) → new gate/tooltip/amber pins fail on main. Good. 6. Bypass/min_approvals/stale simplification note exists in both pull-state.js docstring and the 12_web_ui.md amendment (presence-only, exact-ref, server authoritative). Verified. No fix commit — no defect rising above note-level. The loading fail-open nuance above is the only observation; changing it would trade the current checks-consistent behavior for disable→enable flicker on every PR load, so deliberately not changed.

crueber merged commit 756dd87a7e into main

2026-09-15 23:20:25 +00:00

crueber referenced this pull request from a commit

2026-09-15 23:20:26 +00:00

Merge pull request 'Gate client CHANGES_REQUESTED merge-block on required-reviews rule (fixes #612)' (#615) from fix/issue-612 into main

crueber referenced this pull request

2026-09-15 23:20:26 +00:00

Merge UI treats CHANGES_REQUESTED as blocking with no protection rule while the server merges it #612