jakman85andClaude Opus 5
8ccfdab180
`required_approvals: 1` on `main` was a lock-out, not a protection. Gitea
refuses to let a PR's author approve their own PR, and `jakman85` is the only
collaborator — so no account in existence could approve a PR the operator
opened, including the PR that would fix that. Measured 2026-10-01 on PR #35:
gate green in 1m41s, reviews `[]`, `required_approvals: 1`, unmergeable.
It was never the documented design either: `ci/approval-policy.sh`'s header and
`ci/gate.sh:194` both already describe the model as "required_approvals drops
to 0 and the GATE decides", and `require-gate.sh`'s own closing note named this
failure and prescribed this remedy. 2026-09-30's entry flagged the 1-vs-0
disagreement as unresolved; this settles it at 0.
Layer 1 — branch protection. required_approvals 1 -> 0 in the declaration, plus
infra/gitea/allow-admin-self-merge.sh to apply it live (target read FROM the
declaration, so the script cannot drift from what the gate checks).
require-gate.sh moves to 0 too; left at 1 it would re-impose the lock-out on its
next run. The narrow choice is deliberate: the other way to unblock an owner is
block_admin_merge_override: false, which would re-open what
block-admin-override.sh was written to close (PRs #5-#9, zero approvals). This
drops the approval COUNT only — block_admin_merge_override and
enable_status_check stay true and are asserted after the PATCH, so a red gate
stays unmergeable by everyone, administrators included.
Layer 2 — ci/approval-policy.sh. Sensitive paths still need a non-author
approval the sole admin cannot get, so an administrator may waive it with an
`/approve-override <reason>` comment: directive must start a line, reason
mandatory, permission re-checked LIVE, invalidated by a new commit, and the
overrider + reason printed in the gate log and job summary. Candidate selection
is the pure, offline ci/approval-override.py (23 unit assertions, all passing);
authorisation stays in the fail-closed stage. Every new failure mode blocks.
The cost, stated not buried: for an administrator, "a change to the gate cannot
be gated by the gate" is now advisory rather than structural. Unavoidable with
one human; what is bought back is an attributed, commit-scoped record instead of
a silent bypass. And the uncomfortable part, accepted as an operator decision:
authorisation is by live permission alone, and the AGENT account `gitea-admin`
also holds `owner` — so an agent can clear its own PR this way. Documented in
approval-policy.json, the stage header, and CLAUDE.md (which tells agents not
to). Narrowing needs no code change: add `override_admins` and the stage already
requires membership.
Adjacent fix, in scope because `make -C ci test` is the only runner for the
suite added here: ci/tests/gate-decision.test.sh hand-maintained a stub list
that "must name EVERY stage gate.sh runs" and omitted `approval-policy` (added
to gate.sh by PR #17), so run_stage hit exit 127 and 15 of 50 assertions failed.
Verified pre-existing on pristine origin/main @ 0c7e8dd. The list is now derived
from gate.sh's own run_stage lines, so the omission cannot recur; the suite goes
15-failed -> 50 passing and `make -C ci test` goes FAILED -> all suites passed.
Toolbox gate: rc 3 UNDETERMINED — every stage green except `gitea-settings`
(no SETTINGS_API_TOKEN locally) and `conformance` (no cluster). NOT a green run.
The live rule is UNCHANGED and still reads 1 — mutating it is an operator
action. Until allow-admin-self-merge.sh is run, ci/gitea-settings.sh will
correctly report DRIFT and this PR will be red.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
186 lines
13 KiB
Markdown
186 lines
13 KiB
Markdown
---
|
||
date: 2026-10-01
|
||
branch: fix/admin-approval-override
|
||
title: Administrators can merge their own green PR — required_approvals 1 → 0, plus an attributed /approve-override for sensitive paths
|
||
---
|
||
|
||
## What changed
|
||
|
||
**The problem, measured first.** `required_approvals: 1` on `main` was a lock-out, not a
|
||
protection. Gitea refuses to let a pull request's author approve their own PR, and `jakman85` is
|
||
the **only** collaborator (permission `owner`). So no account in existence could approve a PR the
|
||
operator opened. Measured on the live API, 2026-10-01:
|
||
|
||
| | |
|
||
|---|---|
|
||
| PR #35 (`bugfix/arr-to-plex`, author `jakman85`) | `gate / gate (pull_request)` = **success, 1m41s** |
|
||
| its reviews | `[]` |
|
||
| live `required_approvals` | `1` |
|
||
| live `block_admin_merge_override` | `true` |
|
||
|
||
Green gate, zero reviews, no path to a merge — and the same was true of any PR that would have
|
||
fixed it.
|
||
|
||
It was also never the design this repo wrote down. `ci/approval-policy.sh`'s own header and
|
||
`ci/gate.sh:194` both already described the model as *"`required_approvals` drops to 0 and the
|
||
GATE decides"*, and `infra/gitea/require-gate.sh`'s closing note named this exact failure and
|
||
prescribed this exact remedy. `decisions/2026-09-30-flux-deploy-notification-inhibited.md` had
|
||
flagged the 1-vs-0 disagreement as unresolved and not that session's to settle. This settles it
|
||
at 0.
|
||
|
||
**Two layers were blocking, and both are addressed.**
|
||
|
||
*Layer 1 — Gitea branch protection.* `required_approvals: 1 → 0` in
|
||
`infra/gitea/expected-branch-protection.json`, with the rationale in the file, plus
|
||
`infra/gitea/allow-admin-self-merge.sh` to apply it to the live rule (it reads the target **from
|
||
the declaration**, so the script cannot drift from what the gate checks). `require-gate.sh` was
|
||
changed to 0 as well — left at 1 it would silently re-impose the lock-out on its next run.
|
||
|
||
The narrow choice matters: the *other* way to unblock an owner is
|
||
`block_admin_merge_override: false`, and that would have re-opened precisely what
|
||
`infra/gitea/block-admin-override.sh` was written to close (PRs #5–#9 merged with zero approvals
|
||
while the settings read as enforced). This change drops the approval **count** only.
|
||
`block_admin_merge_override` and `enable_status_check` both stay `true`, and the script asserts
|
||
both afterwards and fails loudly if the PATCH disturbed either. **A red gate remains unmergeable
|
||
by everyone, administrators included.**
|
||
|
||
*Layer 2 — `ci/approval-policy.sh`.* Sensitive paths (`ci/**`, `infra/gitea/**`, `CLAUDE.md`,
|
||
kyverno, rbac, sops, Flux wiring) still require a non-author `APPROVED` review, which the sole
|
||
administrator still cannot obtain. An administrator may now waive it by commenting on the PR:
|
||
|
||
/approve-override <reason>
|
||
|
||
- the directive must **start a line** (a quoted or mid-sentence mention does not count, or
|
||
repeating someone else's override would re-arm it);
|
||
- a **reason is mandatory** — a bare directive records that review was waived but not why;
|
||
- the commenter's permission is **re-checked live** against
|
||
`/collaborators/{login}/permission` when the gate runs, so an old comment from a demoted
|
||
account carries nothing;
|
||
- it is **invalidated by a new commit** (the comment must post-date the head commit), mirroring
|
||
Gitea's own `dismiss_stale_approvals`;
|
||
- the overrider, timestamp and reason are printed in the gate log and the job summary, and a
|
||
self-waiver by the PR's own author is called out as such.
|
||
|
||
Selection of the candidate comment is `ci/approval-override.py` — pure, offline, no authority of
|
||
its own — so it could be unit-tested; authorisation stays in the fail-closed stage.
|
||
`ci/tests/approval-override.test.sh` adds **23 assertions**, all passing, each constructing a
|
||
case the parser must refuse. Every new failure mode blocks: unreadable head-commit timestamp,
|
||
unreadable permission, parser error.
|
||
|
||
**The cost, stated rather than buried.** For an administrator, *"a change to the gate cannot be
|
||
gated by the gate"* is now **advisory, not structural**. That is unavoidable with one human — the
|
||
alternative is a repository its owner cannot change — and what is bought back is an attributed,
|
||
reasoned, commit-scoped record of each waiver instead of a silent bypass.
|
||
|
||
**The uncomfortable part, accepted with eyes open (operator decision).** Authorisation is by live
|
||
permission **alone**, with no allowlist of human accounts. The **agent account `gitea-admin` also
|
||
reports permission `owner`** on this repo (measured), so an agent *can* clear the sensitive-path
|
||
review on its own PR — and agents author most PRs here, so that is the common case, not an edge
|
||
case. I raised this before implementing; the operator chose permission-only. It is documented in
|
||
three places rather than left to be discovered: `approval-policy.json`
|
||
(`_known_consequence_agent_accounts`), the stage's header, and `CLAUDE.md`, which now tells
|
||
agents plainly **not** to use it on their own work. The narrowing path needs no code change —
|
||
add an `override_admins` list to `approval-policy.json` and the stage already requires membership
|
||
in addition to the permission check.
|
||
|
||
Also updated: `CLAUDE.md` §1 and the §2 enforcement table (the "approval required" row is now
|
||
explicitly ⬜ with the reason, rather than a stale ✅), and `infra/gitea/README.md` gains §6 and
|
||
marks its historical step 4 as reversed — do not re-apply.
|
||
|
||
**One adjacent fix, deliberately in scope: `ci/tests/gate-decision.test.sh` was already broken.**
|
||
It hand-maintains a list of stub stage scripts that "must name EVERY stage `gate.sh` runs", and it
|
||
did not name `approval-policy` — added to `gate.sh` by PR #17 and never added here. `run_stage`
|
||
therefore got exit 127 from a missing file and recorded a FAILED stage in every case, so **15 of
|
||
50 assertions failed**, including every one expecting a PASS/UNDETERMINED headline. Measured on
|
||
pristine `origin/main` (`0c7e8dd`) before touching it, so the attribution is not a guess.
|
||
|
||
I fixed it because `make -C ci test` is the *only* runner for the suite this change adds — leaving
|
||
it red would have made the new tests unverifiable in practice. The file's own comment already
|
||
narrated this exact failure for `gitea-settings` and did not prevent the repeat, which says the
|
||
comment was not the fix. So the stub list is now **derived from `gate.sh`'s own `run_stage` lines**
|
||
and asserts it parsed at least 8 of them: a stage added to the gate can no longer be silently
|
||
missing from its test. `make -C ci test` goes from **FAILED to all suites passed**.
|
||
|
||
## What I verified
|
||
|
||
- **The lock-out is real, and PR #35 is the proof.** Queried live: head sha
|
||
`8a094c46`, statuses `gate / gate (pull_request) = success :: Successful in 1m41s`,
|
||
`/pulls/35/reviews` → `[]`, `/branch_protections/main` → `required_approvals: 1`. I read the
|
||
**rule object**, not `/branches/main`, per §2b.
|
||
- **`jakman85` is the only collaborator**: `/collaborators` → one entry. Permission `owner`.
|
||
- **`gitea-admin` also reports `owner`** — this is why the agent-account consequence above is a
|
||
statement of fact and not a hypothetical.
|
||
- **PR #35 touches no sensitive paths** (`apps/wave-0/plex.yaml`, `apps/wave-1/{radarr,sonarr}.yaml`,
|
||
`catalog/{radarr,sonarr}.yaml`, its decisions entry) — so `ci/approval-policy.sh` was never its
|
||
blocker. Layer 1 alone unblocks it.
|
||
- **API shapes verified before depending on them**, rather than assumed: `/git/commits/{sha}`
|
||
returns `.commit.committer.date` as RFC3339 **with an offset** (`2026-10-01T23:09:08-05:00`),
|
||
not a trailing `Z` — a parser handling only `Z` would have silently dropped every real
|
||
override, so there is a test for the offset form. PR conversation comments are under
|
||
`/issues/{n}/comments`; `/pulls/{n}/comments` returns diff comments only and would never have
|
||
seen an override.
|
||
- **`ci/tests/approval-override.test.sh`: 23 passed, 0 failed.** Covers the happy path, bare
|
||
directive, mid-sentence and `>`-quoted directives, stale and equal-timestamp overrides,
|
||
unparseable timestamps, missing login, invalid JSON, non-list JSON, latest-wins, offset
|
||
timestamps, and a configurable directive.
|
||
- `bash -n` clean on `ci/approval-policy.sh`, `infra/gitea/allow-admin-self-merge.sh`,
|
||
`infra/gitea/require-gate.sh`; `jq -e .` clean on both JSON declarations.
|
||
- **`gate-decision.test.sh`'s 15 failures are pre-existing, measured not assumed.** Extracted
|
||
`origin/main`'s `ci/` at `0c7e8dd` into a throwaway directory and ran the suite from there:
|
||
**FAIL, 15 failed / 35 passed**. After deriving the stub list from `gate.sh`: **PASS, 50
|
||
assertions**.
|
||
- **`make -C ci test` in the pinned toolbox: `ci/tests: all suites passed`, rc 0** — approval-override
|
||
23, gate-decision 50, touched-requirements-base 31, test-attribution 15, touched-requirements 26,
|
||
decisions-log pass. It was rc 2 (`ci/tests: FAILED`) before the stub-list fix.
|
||
- **Toolbox merge gate** (`./infra/devcontainer/dev.sh run bash -c './ci/gate.sh'`), run on the
|
||
rebased branch: **rc 3, `MERGE GATE: UNDETERMINED`**. Green stages: spec-immutability,
|
||
decisions-log, lint, l0 (23 offline cases, 0 failures), render-golden including the new
|
||
`catalog/render.py --selftest`, l1 (3 cases), approval-policy (N/A offline). Skipped:
|
||
`gitea-settings` (no `SETTINGS_API_TOKEN` on this host) and `conformance` (no cluster).
|
||
**This is NOT a green run and I am not reporting it as one** — rc 3 is UNDETERMINED per the
|
||
CLAUDE.md §3 contract.
|
||
|
||
## What I did NOT verify
|
||
|
||
- **The live Gitea rule still says `required_approvals: 1`. I did not change it.** Mutating live
|
||
platform settings is an operator action; `infra/gitea/allow-admin-self-merge.sh` is written,
|
||
syntax-checked, and **unrun**. The declaration and the live rule must move together.
|
||
*Now measured, not predicted:* PR #38's CI run (run 202, job 0) fails at **1m44s** with
|
||
`required_approvals: DECLARED 0 but LIVE is 1` → `gitea-settings: FAIL`. Correct behaviour, and
|
||
the PR stays red until the script is run.
|
||
- **The override has never been ACCEPTED by a real run** — only offered. Measured in the same CI
|
||
log: the stage reached `approval-policy: FAIL` on the sensitive paths and printed the *new*
|
||
code's guidance, `Or, if you hold admin/owner on this repository, comment on the PR:
|
||
/approve-override <reason> ... NEWER than head commit 7b9f733c`. So the new stage **is** what
|
||
runs on this PR (self-bootstrapping confirmed by measurement, not by reasoning), and the
|
||
authorising branch — comment parse → live permission lookup → exit 0 — is still **unexercised**.
|
||
Posting the comment on PR #38 is its first real test. If it does not work, the recovery is to
|
||
PATCH `required_approvals` and read run 202's successor log.
|
||
- **I have not seen the gate still REFUSE a merge** now that approvals are 0. The claim that
|
||
`block_admin_merge_override: true` keeps a red gate unmergeable rests on Gitea's documented
|
||
behaviour and on the 2026-09-29 session, **not** on a provoked refusal in this one. That probe
|
||
is written out at the foot of `allow-admin-self-merge.sh` and should be done, exactly as the
|
||
push rejection was provoked rather than assumed.
|
||
- **Clock skew in the staleness check is unhandled.** Staleness compares the comment's
|
||
server-side `created_at` against the head commit's *committer* date, which comes from the
|
||
committer's machine. A commit dated in the future would reject legitimate overrides until real
|
||
time catches up (fail-closed, so loud, but annoying). Gitea's own `dismiss_stale_approvals`
|
||
compares SHAs instead; a comment cannot. Not hit here — PR #35's commit date is ~21 min behind
|
||
server time — and not fixed.
|
||
- **`ci/tests/approval-override.test.sh` does not run in the merge gate.** `ci/gate.sh` runs no
|
||
`ci/tests/*`; they run only via `make -C ci test`. So a regression in the parser would not be
|
||
caught by CI. PR #33 (merged while this branch was being written, and rebased onto here) fixed
|
||
the *same class* of gap for `catalog/render.py --selftest` by invoking it from
|
||
`ci/render-golden.sh` — it did **not** wire `ci/tests/*` in, which I confirmed after rebasing
|
||
rather than assuming. Wiring them is a one-line stage and I did not widen scope to it.
|
||
- **Authorisation itself is untested code.** `ci/approval-override.py` is unit-tested; the live
|
||
permission lookup, the `override_admins` allowlist branch, and every new BLOCK path in
|
||
`ci/approval-policy.sh` need Gitea and have never been exercised. The allowlist branch in
|
||
particular is implemented but **unused** — `override_admins` is absent by design.
|
||
- **The other open PRs will go red** once the live value changes, until they are rebased onto the
|
||
merged declaration: as of writing those are **#29** and **#37** (both `gitea-admin`), plus
|
||
**#35**. I did not rebase them. Merging them first avoids it entirely; the script's header gives
|
||
the order. PRs #33, #34 and #36 merged while this branch was being written — this branch is
|
||
rebased onto `0c7e8dd`, so the list was re-measured rather than carried forward.
|
||
- No cluster state was read or changed; nothing here touches Flux, Kyverno or any workload.
|