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

crueber commented

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

Owner

What's requested

A per-repo "allow self-approval" setting (default: permissive / allowed) that replaces today's hard server-side block on PR authors approving their own pull requests.

Today an author who tries to Approve (or Request Changes on) their own PR gets a 422:

internal/review/service.go:344-347 — SubmitReview rejects when normPrincipal(h.Author) == who && in.State != StateCommented with "author cannot approve their own pull request".
The rule is spec-encoded in docs/features/04_code_review.md (route table line ~203: "author self-approve/request-changes → 422"; constraints bullet line ~269: "no author self-approval, enforced server-side"). This ticket amends that stated design rule — flagged for the user's call, not silently changed.

Proposed design

New boolean on the repo settings doc (internal/api/settings.go, WAL-published TOML, GET AuthRead / PUT AuthAdmin, ≤16 KiB), e.g. allow_self_approval (or grouped under a review section per the doc's existing shape). Default permissive — on — so repos behave like GitHub out of the box.
SubmitReview consults the setting instead of the unconditional author check: when allowed, an author may submit APPROVED / CHANGES_REQUESTED on their own PR; when disabled, the current 422 behavior stands. COMMENTED is always allowed either way.
Settings UI: a toggle in the repo settings surface (reuse the existing toggle component; Tailwind-only, no ad-hoc CSS).
API: the setting rides the settings doc; no new endpoint. Three-twin route registration is untouched (no new top-level route).

Decision points (planner/user's call — pick one and note it)

Merge-gate counting (the big one). The required-reviews merge gate (Service.EvaluateGate / CheckRequiredReviews in internal/review/service.go ~line 698+, wired into merges via pulls' ReviewGate seam in internal/pulls/review.go) counts EVERY latest StateApproved review toward min_approvals, with no author exclusion — the current code never needs one because the author can't produce an approval. Once self-approval is allowed, an author's own approval WILL count toward min_approvals by default. Options:
- (a) Count it (GitHub-like in spirit: self-approvals satisfy gates unless policy says otherwise) — simplest, no gate change.
- (b) Never count author self-approvals toward min_approvals, regardless of the setting (self-approval then only affects the review summary/decision display, not the gate) — safest for protection semantics.
- (c) Make it a separate per-rule knob on the required-reviews policy effect (internal/pulls/policy.go), e.g. count_self: false default. Most flexible, most surface.
  The ticket should land the setting first with a choice made and stated in code/tests; do not leave the gate counting ambiguous.
Does "self-approval allowed" also permit CHANGES_REQUESTED by the author, or approvals only? The current block covers both non-COMMENTED states; the setting should keep them coupled (one toggle) unless a reason to split emerges.
dismiss_stale interaction: an author self-approval on an old commit_sha already fails the stale check — no special handling expected; confirm in tests.

Architecture notes

The block lives in exactly one place server-side (SubmitReview); review.go's PRKey comment and model.go:27 doc comments reference the self-approve rule and should be updated alongside.
The merge gate reads reviews via scanReviews → latestOf (authoritative scan); no caching/ETag concern here (the setting itself is on the settings doc — if it's projected into any SWR-cached summary, it must be ETag-covered per the mutability law).
Spec amendment: docs/features/04_code_review.md lines ~203 (route table) and ~269 (constraints bullet) must be amended to describe the conditional behavior; this is a deliberate design-rule change, presented for user ratification.

Acceptance criteria

New per-repo allow_self_approval setting on the settings doc, default on, editable via PUT AuthAdmin, rendered in the settings UI with an existing toggle component.
With the setting on: the PR author can APPROVE (and CHANGES_REQUESTED) their own PR; no 422; review summary and events reflect it.
With the setting off: the current 422 "author cannot approve their own pull request" behavior is preserved.
The merge-gate counting question (Decision 1) is answered with an explicit choice, implemented, and covered by tests (author approval counted vs not, per the choice).
docs/features/04_code_review.md amended (route table + constraints bullet) to describe the conditional rule.
Related doc comments (internal/review/model.go, internal/review/review.go, internal/review/service.go header) updated — no stale "never self-approve" claims.
Tests: SubmitReview allowed/denied paths, gate counting behavior, settings default (permissive) on a fresh repo.

## What's requested A per-repo **"allow self-approval"** setting (default: **permissive / allowed**) that replaces today's hard server-side block on PR authors approving their own pull requests. Today an author who tries to Approve (or Request Changes on) their own PR gets a 422: - `internal/review/service.go:344-347` — `SubmitReview` rejects when `normPrincipal(h.Author) == who && in.State != StateCommented` with "author cannot approve their own pull request". - The rule is spec-encoded in `docs/features/04_code_review.md` (route table line ~203: "author self-approve/request-changes → 422"; constraints bullet line ~269: "no author self-approval, enforced server-side"). **This ticket amends that stated design rule — flagged for the user's call, not silently changed.** ## Proposed design - New boolean on the repo settings doc (`internal/api/settings.go`, WAL-published TOML, GET AuthRead / PUT AuthAdmin, ≤16 KiB), e.g. `allow_self_approval` (or grouped under a review section per the doc's existing shape). Default permissive — **on** — so repos behave like GitHub out of the box. - `SubmitReview` consults the setting instead of the unconditional author check: when allowed, an author may submit APPROVED / CHANGES_REQUESTED on their own PR; when disabled, the current 422 behavior stands. COMMENTED is always allowed either way. - Settings UI: a toggle in the repo settings surface (reuse the existing toggle component; Tailwind-only, no ad-hoc CSS). - API: the setting rides the settings doc; no new endpoint. Three-twin route registration is untouched (no new top-level route). ## Decision points (planner/user's call — pick one and note it) 1. **Merge-gate counting (the big one).** The required-reviews merge gate (`Service.EvaluateGate` / `CheckRequiredReviews` in `internal/review/service.go` ~line 698+, wired into merges via pulls' `ReviewGate` seam in `internal/pulls/review.go`) counts EVERY latest `StateApproved` review toward `min_approvals`, with no author exclusion — the current code never needs one because the author can't produce an approval. Once self-approval is allowed, an author's own approval WILL count toward `min_approvals` by default. Options: - (a) **Count it** (GitHub-like in spirit: self-approvals satisfy gates unless policy says otherwise) — simplest, no gate change. - (b) **Never count author self-approvals toward `min_approvals`**, regardless of the setting (self-approval then only affects the review summary/decision display, not the gate) — safest for protection semantics. - (c) Make it a separate per-rule knob on the `required-reviews` policy effect (`internal/pulls/policy.go`), e.g. `count_self: false` default. Most flexible, most surface. The ticket should land the setting first with a choice made and stated in code/tests; do not leave the gate counting ambiguous. 2. **Does "self-approval allowed" also permit CHANGES_REQUESTED by the author, or approvals only?** The current block covers both non-COMMENTED states; the setting should keep them coupled (one toggle) unless a reason to split emerges. 3. **dismiss_stale interaction:** an author self-approval on an old commit_sha already fails the stale check — no special handling expected; confirm in tests. ## Architecture notes - The block lives in exactly one place server-side (`SubmitReview`); `review.go`'s `PRKey` comment and `model.go:27` doc comments reference the self-approve rule and should be updated alongside. - The merge gate reads reviews via `scanReviews` → `latestOf` (authoritative scan); no caching/ETag concern here (the setting itself is on the settings doc — if it's projected into any SWR-cached summary, it must be ETag-covered per the mutability law). - Spec amendment: `docs/features/04_code_review.md` lines ~203 (route table) and ~269 (constraints bullet) must be amended to describe the conditional behavior; this is a deliberate design-rule change, presented for user ratification. ## Acceptance criteria - [ ] New per-repo `allow_self_approval` setting on the settings doc, default **on**, editable via PUT AuthAdmin, rendered in the settings UI with an existing toggle component. - [ ] With the setting on: the PR author can APPROVE (and CHANGES_REQUESTED) their own PR; no 422; review summary and events reflect it. - [ ] With the setting off: the current 422 "author cannot approve their own pull request" behavior is preserved. - [ ] The merge-gate counting question (Decision 1) is answered with an explicit choice, implemented, and covered by tests (author approval counted vs not, per the choice). - [ ] `docs/features/04_code_review.md` amended (route table + constraints bullet) to describe the conditional rule. - [ ] Related doc comments (`internal/review/model.go`, `internal/review/review.go`, `internal/review/service.go` header) updated — no stale "never self-approve" claims. - [ ] Tests: SubmitReview allowed/denied paths, gate counting behavior, settings default (permissive) on a fresh repo.

crueber added the

enhancement

pr/merge protection rules

pull requests

labels

2026-09-15 17:03:03 +00:00

crueber added this to the v1 milestone

2026-09-15 17:03:03 +00:00

crueber added the

issues

label

2026-09-15 17:03:39 +00:00

crueber referenced this issue from a commit

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

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

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

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

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

crueber referenced this issue

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

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

crueber referenced this issue 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 closed this issue

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

crueber commented

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

Author

Owner

Fixed by #589 (merged): per-repo [review] allow_self_approval (default ON, zero migration) on the settings doc with a General-tab toggle; SubmitReview consults it (OFF keeps the 422); merge gate never counts author self-approvals toward min_approvals (Decision b), author's CHANGES_REQUESTED still blocks; dismiss_stale needs no special handling. Spec amended in 04_code_review.md + 11_config_cli.md as a deliberate rule change. Verified: review/config coverage ≥95%, go -race green, 1456 web unit green (smoke excluded, pre-existing), vite/esbuild green, independent review APPROVE.

crueber referenced this issue

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

Lock commenting (comments, replies, reactions, inline threads) on closed issues and merged/closed PRs #594

crueber referenced this issue

2026-09-15 21:49:11 +00:00

Code review section on Settings General tab sticks on 'loading…' forever (unset self-approval knob never leaves the prefill sentinel) #605

crueber referenced this issue

2026-09-15 22:35:45 +00:00

Fix #605: Code-review toggle stuck on loading for unset docs #610

crueber referenced this issue

2026-09-15 23:05:17 +00:00

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