Per-repo allow_self_approval setting replacing the hard author-cannot-approve block (Fix #586) #589

Merged

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

2026-09-15 19:00:09 +00:00

crueber commented

2026-09-15 18:54:59 +00:00

Owner

Deliberate design-rule change for user ratification: the unconditional author-cannot-approve block (422) becomes conditional on the per-repo [review] allow_self_approval knob, default ON so fresh repos behave like GitHub. OFF keeps the historical 422; COMMENTED always allowed.

Decisions (all implemented + tested, see docs/features/04_code_review.md Decisions):

Decision 1 (b): author self-approvals NEVER count toward min_approvals in the merge gate. The submit toggle governs who may record a verdict; the gate governs what protects the ref. Rejected (a) count-them (an author could single-handedly satisfy a protection rule) and (c) a second count_self knob (unasked-for complexity). An author's surviving CHANGES_REQUESTED still blocks, like any reviewer's.
Decision 2: one coupled toggle covers APPROVED + CHANGES_REQUESTED (no reason to split emerged).
Decision 3: dismiss_stale needs no special handling — a stale self-approval already fails the commit_sha freshness check, and the author exclusion covers the fresh case too.

Seam/cost (laws 6+8): review.Service.Settings (nil = default) wired in composition via reg.Open + manifest snapshot + config.AllowSelfApprovalOf; warm path in-memory, no lock held across the call; submits are control-plane-sized, off the push/sync budgets (same cost class as the gate's policy.json read). No summary/ETag projection — enforced read-time, never cached. No proto change (TOML settings-doc field only). Settings UI toggle reuses ToggleSwitch (Tailwind-only); rides the existing settings PUT, no new endpoint/route.

Verification: go test ./internal/review/... ./internal/pulls/... ./internal/api/... + ./internal/config/... ./cmd/walhub/... all -race green; per-package cover review 95.9% / config 96.0% (new code 100%); gofmt/vet clean; web unit 1456/1456 (incl. 2 new files, 10 tests); vite build + esbuild SDK bundle green; dist/.keep restored.

Deliberate design-rule change for user ratification: the unconditional author-cannot-approve block (422) becomes conditional on the per-repo `[review] allow_self_approval` knob, default ON so fresh repos behave like GitHub. OFF keeps the historical 422; COMMENTED always allowed. Decisions (all implemented + tested, see docs/features/04_code_review.md Decisions): - Decision 1 (b): author self-approvals NEVER count toward min_approvals in the merge gate. The submit toggle governs who may record a verdict; the gate governs what protects the ref. Rejected (a) count-them (an author could single-handedly satisfy a protection rule) and (c) a second count_self knob (unasked-for complexity). An author's surviving CHANGES_REQUESTED still blocks, like any reviewer's. - Decision 2: one coupled toggle covers APPROVED + CHANGES_REQUESTED (no reason to split emerged). - Decision 3: dismiss_stale needs no special handling — a stale self-approval already fails the commit_sha freshness check, and the author exclusion covers the fresh case too. Seam/cost (laws 6+8): review.Service.Settings (nil = default) wired in composition via reg.Open + manifest snapshot + config.AllowSelfApprovalOf; warm path in-memory, no lock held across the call; submits are control-plane-sized, off the push/sync budgets (same cost class as the gate's policy.json read). No summary/ETag projection — enforced read-time, never cached. No proto change (TOML settings-doc field only). Settings UI toggle reuses ToggleSwitch (Tailwind-only); rides the existing settings PUT, no new endpoint/route. Verification: go test ./internal/review/... ./internal/pulls/... ./internal/api/... + ./internal/config/... ./cmd/walhub/... all -race green; per-package cover review 95.9% / config 96.0% (new code 100%); gofmt/vet clean; web unit 1456/1456 (incl. 2 new files, 10 tests); vite build + esbuild SDK bundle green; dist/.keep restored.

crueber added 1 commit

2026-09-15 18:55:00 +00:00

Per-repo allow_self_approval knob replacing the hard author-cannot-approve block (Forgejo #586 ) ae42bd6e88

SubmitReview consults the WAL-published [review] allow_self_approval
(default ON: fresh repos behave like GitHub) via a narrow SettingsResolver
seam wired in composition (reg.Open + manifest snapshot + AllowSelfApprovalOf;
warm path in-memory, no lock held, off the hot-path budgets); OFF keeps the
historical 422, COMMENTED always allowed, resolver failure fails closed (503).

Decision 1 (b): author self-approvals NEVER count toward min_approvals in
EvaluateGate (submit governs recording, the gate governs protection).
Decision 2: one coupled toggle covers APPROVED + CHANGES_REQUESTED.
Decision 3: dismiss_stale needs no special handling (stale self-approval
already fails the freshness check; exclusion covers the fresh case too).

Settings UI: Code-review toggle on the General tab (shared ToggleSwitch,
Tailwind-only) composing into the existing settings PUT. No new endpoint,
no summary/ETag projection (enforced read-time, never cached).

Amends docs/features/04_code_review.md (route table, gate rule, constraints
bullet, Decisions) + docs/go/11_config_cli.md (shape, rules, Decisions;
also records the previously undocumented [features] section).

crueber commented

2026-09-15 18:59:54 +00:00

Author

Owner

Independent review of ae42bd6 against #586 — verdict: APPROVE (no fixes needed, worktree clean, nothing pushed).

Acceptance, all checked:

Setting seam: [review] allow_self_approval as *bool (unset distinct from false); absent section/key resolves allowed — fresh repos AND zero-migration for pre-#586 repos. Rides the existing settings doc (GET AuthRead / PUT AuthAdmin, central 16 KiB + ParseRepoSettings validation untouched) — no new endpoint/route. SettingsResolver wired in composition only (cmd/walhub/review.go: reg.Open + ManifestSnapshot + config.AllowSelfApprovalOf); internal/review imports nothing upward (identity, policy, server/auth, store only). SubmitReview holds no lock; one manifest-snapshot read per author submit (control-plane cost class, same as the gate policy.json read) — no hot-path budget impact, no sim/budget files touched.
Gate Decision 1(b): EvaluateGate excludes author self-approvals from min_approvals via normPrincipal on both sides (event keys normalized at submit, header author normalized at gate); author's CHANGES_REQUESTED still blocks (no exclusion on that branch). Resolver failure fails closed: ErrUnavailable → 503, and only the author path consults the seam (non-authors unaffected) — both tested.
Docs: 04_code_review.md route table + merge-gate rule + constraints bullet all describe the conditional rule, framed as a deliberate rule change for ratification with Decisions 1–3 explicit; model.go/review.go/service.go comments updated (verified no stale unconditional claims remain); 11_config_cli.md §4.1/§4.2 + Decisions accurate — verified the [features]/§522 section truly was undocumented in that doc on main, so the bundled amendment is correct.
UI: shared ToggleSwitch, existing settings PUT path (withSelfApproval preserves all other sections), Tailwind-only, #533 row pattern; label 'Allow self-approval' confirmed present in built bundle web/dist/assets/index-CmYiphbH.js.
Tests: allowed/denied × on/off/nil-seam × APPROVED/CHANGES_REQUESTED/COMMENTED/non-author matrix, verbatim 422 text, 503 fail-closed, gate counting (fresh self alone 0/1, self+1 real = count 1, author block text, case-insensitive exclusion), stale self + stale control + re-approve-new-head, fresh-default/stale-default, config parse/reject/merge-ignored/fail-open. service_test.go 422-row removal is relocation into selfapprove586_test.go (knob-off cases), not weakening. New tests fail pre-fix by inspection (pre-fix SubmitReview 422s author APPROVE unconditionally; new default-ON rows expect success).
Laws: no proto change; no new deps (go.mod/web package.json untouched; SDK stays dependency-free).

Verification run in /tmp/walhub-586: review + config + pulls + api + cmd/walhub all -race green; cover review 95.9% / config 96.0% (claims match); gofmt/vet clean; web unit 1456/1456 green (full-glob run showed 1459 with 1 failure in smoke.test.js /setup 403, but that is environmental — a stray listener on :8080 serving 403, unrelated to this PR which touches no serving code; with WALHUB_TEST_WEB_BASE_URL pointed unroutable the 3 server-dependent tests skip and the suite is 1456 pass / 0 fail, exactly the claimed count).

Independent review of ae42bd6 against #586 — verdict: APPROVE (no fixes needed, worktree clean, nothing pushed). Acceptance, all checked: - Setting seam: [review] allow_self_approval as *bool (unset distinct from false); absent section/key resolves allowed — fresh repos AND zero-migration for pre-#586 repos. Rides the existing settings doc (GET AuthRead / PUT AuthAdmin, central 16 KiB + ParseRepoSettings validation untouched) — no new endpoint/route. SettingsResolver wired in composition only (cmd/walhub/review.go: reg.Open + ManifestSnapshot + config.AllowSelfApprovalOf); internal/review imports nothing upward (identity, policy, server/auth, store only). SubmitReview holds no lock; one manifest-snapshot read per author submit (control-plane cost class, same as the gate policy.json read) — no hot-path budget impact, no sim/budget files touched. - Gate Decision 1(b): EvaluateGate excludes author self-approvals from min_approvals via normPrincipal on both sides (event keys normalized at submit, header author normalized at gate); author's CHANGES_REQUESTED still blocks (no exclusion on that branch). Resolver failure fails closed: ErrUnavailable → 503, and only the author path consults the seam (non-authors unaffected) — both tested. - Docs: 04_code_review.md route table + merge-gate rule + constraints bullet all describe the conditional rule, framed as a deliberate rule change for ratification with Decisions 1–3 explicit; model.go/review.go/service.go comments updated (verified no stale unconditional claims remain); 11_config_cli.md §4.1/§4.2 + Decisions accurate — verified the [features]/§522 section truly was undocumented in that doc on main, so the bundled amendment is correct. - UI: shared ToggleSwitch, existing settings PUT path (withSelfApproval preserves all other sections), Tailwind-only, #533 row pattern; label 'Allow self-approval' confirmed present in built bundle web/dist/assets/index-CmYiphbH.js. - Tests: allowed/denied × on/off/nil-seam × APPROVED/CHANGES_REQUESTED/COMMENTED/non-author matrix, verbatim 422 text, 503 fail-closed, gate counting (fresh self alone 0/1, self+1 real = count 1, author block text, case-insensitive exclusion), stale self + stale control + re-approve-new-head, fresh-default/stale-default, config parse/reject/merge-ignored/fail-open. service_test.go 422-row removal is relocation into selfapprove586_test.go (knob-off cases), not weakening. New tests fail pre-fix by inspection (pre-fix SubmitReview 422s author APPROVE unconditionally; new default-ON rows expect success). - Laws: no proto change; no new deps (go.mod/web package.json untouched; SDK stays dependency-free). Verification run in /tmp/walhub-586: review + config + pulls + api + cmd/walhub all -race green; cover review 95.9% / config 96.0% (claims match); gofmt/vet clean; web unit 1456/1456 green (full-glob run showed 1459 with 1 failure in smoke.test.js /setup 403, but that is environmental — a stray listener on :8080 serving 403, unrelated to this PR which touches no serving code; with WALHUB_TEST_WEB_BASE_URL pointed unroutable the 3 server-dependent tests skip and the suite is 1456 pass / 0 fail, exactly the claimed count).

crueber merged commit 820f41a8e5 into main

2026-09-15 19:00:09 +00:00

crueber referenced this pull request from a commit

2026-09-15 19:00:10 +00:00

Merge pull request 'Per-repo allow_self_approval setting replacing the hard author-cannot-approve block (Fix #586)' (#589) from fix/issue-586 into main

crueber referenced this pull request

2026-09-15 19:00:11 +00:00

Per-repo 'allow self-approval' setting (default on) replacing the hard author-cannot-approve block #586