fix(ci): accept the override from a PR review, and stop misdiagnosing an unreadable comments API
The override did not work on its first real test, and the reason was invisible from the outside. Run 203 on PR #38: a well-formed `/approve-override` posted by jakman85 in the PR comment box, 29 minutes newer than the head commit, and the stage still blocked with: approval-override: COMMENTS_JSON is not a list SETTINGS_API_TOKEN is minted [read:repository, read:admin], and Gitea puts /issues/{n}/comments behind the *issue* scope — so Gitea answered with an error OBJECT, `jq -e .` accepted it as valid JSON, and the stage fell through to 'no approving review'. A permission failure wearing the costume of a missing override. /pulls/{n}/files and /pulls/{n}/reviews are under read:repository, which is why every other part of the same stage worked. 1. The diagnosis no longer lies. COMMENTS_JSON leaves the must-parse-or-block loop and is checked for being an ARRAY; when it is not, the stage prints Gitea's own .message, names the missing scope and says where to post instead. An unreadable source can only cause a false BLOCK, never a false pass, so a loud warning is the honest behaviour and blocking was not. 2. An override is accepted from a PR REVIEW body as well as a comment, so the hatch is not hostage to one API scope — a review of type 'Comment' works on the token CI has today. The gate names which source it used; PENDING (unsubmitted, author-only) reviews are dropped. 3. set-ci-token.sh requests read:issue, making the ordinary comment box work — on a RE-RUN only, since an existing token's scopes cannot be widened. ci/tests/approval-override.test.sh: 23 -> 38 assertions, covering the reviews source, PENDING reviews, an error object from the comments API, 'an unreadable comments source must not suppress a valid review override', latest-wins across both sources, and that the diagnostic quotes Gitea's message. Also measured and recorded: run 203 shows gitea-settings PASS after the operator ran the live PATCH, with block_admin_merge_override still true — the narrow change held, the approval count moved and the gate's teeth did not. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
1 parent
8ccfdab180
commit
b946c475c6
8 files changed
+350
-84
No files matched your search
@@ -87,6 +87,43 @@ Also updated: `CLAUDE.md` §1 and the §2 enforcement table (the "approval requi
|
||||
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.
|
||||
|
||||
**Second pass, 2026-10-02 — the override did not work the first time, and why.** The operator ran
|
||||
the live PATCH (`required_approvals: 0` confirmed live, with `block_admin_merge_override` and
|
||||
`enable_status_check` still `true`), merged PR #35, and posted `/approve-override Need to ensure
|
||||
approval override work as expected` on PR #38. The gate re-ran (run 203) and **still blocked.**
|
||||
|
||||
The comment was not at fault: posted by `jakman85`, directive at the start of the line, reason
|
||||
present, and 29 minutes **newer** than head commit `616a497`, so it cleared the staleness rule.
|
||||
What failed was that **CI could not read it.** Run 203's log:
|
||||
|
||||
approval-override: COMMENTS_JSON is not a list
|
||||
|
||||
`SETTINGS_API_TOKEN` is minted `["read:repository", "read:admin"]` by
|
||||
`infra/gitea/set-ci-token.sh`, and Gitea puts `/issues/{n}/comments` behind the **issue** scope. So
|
||||
Gitea returned an error *object*, `jq -e .` accepted it as valid JSON, and the stage fell through
|
||||
to "no approving review" — **a permission failure wearing the costume of a missing override.**
|
||||
`/pulls/{n}/files` and `/pulls/{n}/reviews` are under `read:repository`, which is exactly why every
|
||||
other part of the same stage worked and this one did not.
|
||||
|
||||
Three fixes, and the first is the one that mattered most:
|
||||
|
||||
1. **The diagnosis is no longer misleading.** `COMMENTS_JSON` is out of the must-parse-or-block
|
||||
loop and is explicitly checked for being an *array*; when it is not, the stage prints Gitea's
|
||||
own `.message`, names the missing scope, and says where to post the override instead. An
|
||||
unreadable source can only cause a false BLOCK, never a false pass, so degrading it to a loud
|
||||
warning is safe — and blocking on it was never the honest behaviour.
|
||||
2. **The override is accepted from a PR REVIEW body as well as a comment**, so the hatch is not
|
||||
hostage to one API scope. A review of type "Comment" works on the token CI has *today*. The
|
||||
gate names which source it used. `PENDING` (unsubmitted, author-only draft) reviews are dropped.
|
||||
3. **`set-ci-token.sh` now requests `read:issue`**, which makes the ordinary comment box work —
|
||||
the thing anybody reaches for first. It takes effect only on a **re-run**: an existing token's
|
||||
scopes cannot be widened in place.
|
||||
|
||||
`ci/tests/approval-override.test.sh` grows from 23 to **38 assertions**, covering the reviews
|
||||
source, `PENDING` reviews, an error *object* from the comments API, "an unreadable comments source
|
||||
must not suppress a valid review override", latest-wins across both sources, and that the
|
||||
diagnostic actually quotes Gitea's message.
|
||||
|
||||
**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`
|
||||
@@ -130,8 +167,24 @@ missing from its test. `make -C ci test` goes from **FAILED to all suites passed
|
||||
**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,
|
||||
**38**, 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.
|
||||
- **The live flip worked, and the drift detector proved it.** Run 202 (before): `gitea-settings:
|
||||
FAIL`, `required_approvals: DECLARED 0 but LIVE is 1`. Run 203 (after the operator ran the
|
||||
PATCH): `gitea-settings: pass`, with `block_admin_merge_override = true` among the keys it
|
||||
checked. So the narrow change held — the approval count moved and the gate's teeth did not.
|
||||
- **The first override attempt failed for a measured reason, not a guessed one.** Run 203 log:
|
||||
`approval-override: COMMENTS_JSON is not a list`. Token scopes read from `set-ci-token.sh`:
|
||||
`[read:repository, read:admin]`. The same run successfully read `/pulls/38` and
|
||||
`/pulls/38/files` (author and sensitive paths both resolved) and got past the reviews
|
||||
validation, which is how I know `/pulls/{n}/reviews` IS readable with that token — from
|
||||
behaviour, not from documentation.
|
||||
- **The comment itself was valid.** `created_at` 2026-10-02T05:31:24Z vs head commit `616a497`
|
||||
committer date 2026-10-02T00:02:20-05:00 → the comment is 29m04s newer, so it passed the
|
||||
staleness rule. Ruling the comment out is what left the token scope as the only candidate.
|
||||
- **`?limit=1000` is not the culprit.** Queried `/issues/38/comments` with `limit=1000`, `limit=50`
|
||||
and no limit using my own credentials: `type=array` in all three. So the difference is the
|
||||
credential, not the query.
|
||||
- **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
|
||||
@@ -142,20 +195,31 @@ missing from its test. `make -C ci test` goes from **FAILED to all suites passed
|
||||
|
||||
## 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.
|
||||
- **The override has still never been ACCEPTED by a real run.** This is the second attempt at
|
||||
this bullet, which is itself the point: the first pass called the hatch unverified, and the
|
||||
first real test found a defect that no amount of local testing would have (an API scope CI has
|
||||
and I did not). What is now measured is that the parse → *reject* path works end to end and
|
||||
diagnoses itself. The **accepting** branch — directive found → live permission lookup → exit 0 —
|
||||
has never executed. Posting the override as a **review** on PR #38 is its first real test, and
|
||||
I would not be surprised by a third round.
|
||||
- **The `read:issue` scope name is unverified.** `set-ci-token.sh` now requests it, but I did not
|
||||
mint a token to confirm Gitea 1.24.3 accepts that exact string — the script's own header warns
|
||||
that "Gitea token scopes are coarse; grant the least that works on your version and confirm the
|
||||
check goes green rather than assuming the scope name", and this repo has already been burned by
|
||||
a *name* that could not exist (`GITEA_SETTINGS_TOKEN`). **If the mint fails, the scope string is
|
||||
the first suspect.** The review route does not depend on it.
|
||||
- **I did not verify that Gitea lets a PR's own author submit a `COMMENT`-type review.** Gitea
|
||||
blocks author self-*approval*; "Comment" is expected to be allowed, and that expectation is now
|
||||
load-bearing for the recommended route. It is one click to find out, and I deliberately did not
|
||||
test it by posting an override myself: doing so as `gitea-admin` is precisely the agent
|
||||
self-override that `CLAUDE.md` now forbids.
|
||||
- **The whole re-run mechanism is manual.** `gate.yml` triggers on `pull_request` and `push: main`
|
||||
only — there is no `issue_comment` trigger, so posting an override does **not** re-run the gate.
|
||||
Re-run the job from the Actions UI, or close and reopen the PR (which fires `pull_request` and
|
||||
leaves the head SHA alone). **Do not push an empty commit to re-trigger it:** that moves the head
|
||||
commit and makes the override stale by the rule above, so it would have to be posted again. A
|
||||
comment trigger was considered and not built — it would need to check out PR head code while the
|
||||
job holds `SETTINGS_API_TOKEN`, which is the classic pwn-request shape and not worth it here.
|
||||
- **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
|
||||
|
||||
Reference in new issue
Block a user