From b946c475c663149c95f9c4db19a1d3e9e667e736 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Fri, 2 Oct 2026 00:44:31 -0500 Subject: [PATCH] fix(ci): accept the override from a PR review, and stop misdiagnosing an unreadable comments API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CLAUDE.md | 14 +- ci/approval-override.py | 155 ++++++++++++------ ci/approval-policy.sh | 42 ++++- ci/tests/approval-override.test.sh | 91 +++++++++- .../2026-10-01-admin-approval-override.md | 94 +++++++++-- infra/gitea/README.md | 13 +- infra/gitea/approval-policy.json | 3 +- infra/gitea/set-ci-token.sh | 22 ++- 8 files changed, 350 insertions(+), 84 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4f707aa..8489d19 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,13 +40,21 @@ missing credential is a lock-out, here a skip would be a bypass. **The administrator override (2026-10-01).** The rule above was unsatisfiable for the one human who runs this repo: Gitea refuses to let an author approve their own PR, and `jakman85` is the only collaborator — so a sensitive-path PR the operator opened could never be merged, *including -the PR that would fix that.* An administrator may therefore waive the review by commenting on +the PR that would fix that.* An administrator may therefore waive the review by posting this on the PR: /approve-override -The directive must **start a line**, the **reason is mandatory**, the commenter's `admin`/`owner` -permission is re-checked **live** when the gate runs, and the comment must be **newer than the +**Post it as a PR *review* of type "Comment", not in the plain comment box.** CI's +`SETTINGS_API_TOKEN` is minted `[read:repository, read:admin]`, and `/issues/{n}/comments` sits +behind Gitea's *issue* scope — so an override typed into the comment box is **invisible to CI**, +and the stage reports it as a missing approval rather than as a permission problem (measured on +PR #38, 2026-10-02). `/pulls/{n}/reviews` is readable, so a review body works today. Both sources +are accepted and the gate names which it used; the comment box starts working once +`infra/gitea/set-ci-token.sh` is re-run to pick up `read:issue`. + +The directive must **start a line**, the **reason is mandatory**, the poster's `admin`/`owner` +permission is re-checked **live** when the gate runs, and it must be **newer than the head commit** — push again and you must override again. The overrider, the timestamp and the reason are printed in the gate log and the job summary. For an administrator, "a change to the gate cannot be gated by the gate" is therefore **advisory, not structural**; what is kept is an diff --git a/ci/approval-override.py b/ci/approval-override.py index 913ccfd..400221a 100755 --- a/ci/approval-override.py +++ b/ci/approval-override.py @@ -1,39 +1,54 @@ #!/usr/bin/env python3 -"""ci/approval-override.py — select the admin-override comment on a PR, if there is a valid one. +"""ci/approval-override.py — select the admin-override on a PR, if there is a valid one. -This script decides NOTHING about authority. It parses a PR's comment list and returns the -single override *candidate* that ci/approval-policy.sh should then try to authorise against +This script decides NOTHING about authority. It parses a PR's comments and reviews and returns +the single override *candidate* that ci/approval-policy.sh should then try to authorise against the live Gitea permission API. Keeping the two apart is the point: * parsing is pure, offline, and therefore TESTABLE (ci/tests/approval-override.test.sh); * authorisation needs the API and must stay in the stage that fails closed. -A candidate is a comment whose body contains, AT THE START OF A LINE, the directive followed -by a non-empty reason: +A candidate is a comment OR a review whose body contains, AT THE START OF A LINE, the directive +followed by a non-empty reason: /approve-override the gate cannot gate itself and I am the only human reviewer +TWO SOURCES, AND WHY THAT IS NOT BELT-AND-BRACES. Measured 2026-10-02 on PR #38: the override was +posted as an ordinary PR comment, was well-formed, was newer than the head commit — and the stage +still blocked, because CI could not READ it. `SETTINGS_API_TOKEN` is minted with scopes +`[read:repository, read:admin]`, and Gitea puts /issues/{n}/comments behind the *issue* scope, so +the request came back as an error OBJECT rather than an array. `/pulls/{n}/reviews` is under +read:repository and was readable in the same run. So: + + * a REVIEW body works with the token CI has today (leave a review of type "Comment"); + * a COMMENT body works once the token carries read:issue (see infra/gitea/set-ci-token.sh). + +Accepting both means the hatch is not hostage to one API scope. A source that cannot be read +contributes no candidates and is reported on stderr; it can only cause a false BLOCK, never a +false pass, which is why degrading it is safe and blocking on it was not necessary. + WHY EACH RULE IS HERE * START OF A LINE, not "anywhere in the body". Otherwise quoting someone else's override - ("you could just /approve-override this") re-arms it. Markdown quotes are stripped of a - leading '>' before matching for the same reason -- a quoted directive must NOT count, so - lines beginning with '>' are DROPPED, not unquoted. - * A REASON IS MANDATORY. A bare directive records that a review was waived but not why, - which is the difference between an audit trail and a rubber stamp. - * STALE OVERRIDES ARE DISCARDED. An override earned at commit A must not silently cover - commit B. Any candidate created at or before the head commit's timestamp is dropped -- - the same semantics Gitea's own `dismiss_stale_approvals` applies to reviews. Push new - commits and you must re-override. - * THE LATEST CANDIDATE WINS, so a second override comment supersedes the first rather than - both being live. + ("you could just /approve-override this") re-arms it. Lines beginning with '>' are DROPPED + rather than unquoted, for exactly that reason. + * A REASON IS MANDATORY. A bare directive records that a review was waived but not why, which + is the difference between an audit trail and a rubber stamp. + * STALE OVERRIDES ARE DISCARDED. An override earned at commit A must not silently cover commit + B. Any candidate created at or before the head commit's timestamp is dropped — the same + semantics Gitea's own `dismiss_stale_approvals` applies to approvals. Push new commits and + you must re-override. + * A PENDING REVIEW IS NOT SUBMITTED. It is a private draft visible only to its author, so it is + not an act of anything and is dropped. + * THE LATEST CANDIDATE WINS, across both sources, so a later override supersedes an earlier one. -Output: one TSV line (login, created_at, reason) on stdout, or nothing at all when there is no -valid candidate. Never exits non-zero for "no override" -- absence is a normal answer, and the +Output: one TSV line (login, timestamp, source, reason) on stdout, or nothing at all when there is +no valid candidate. Never exits non-zero for "no override" — absence is a normal answer and the caller must not be able to confuse it with a parse crash. Input, all via env so nothing lands in `ps`: - COMMENTS_JSON the Gitea /issues/{n}/comments array + COMMENTS_JSON the Gitea /issues/{n}/comments array (or an error object; or absent) + REVIEWS_JSON the Gitea /pulls/{n}/reviews array (or an error object; or absent) HEAD_TIME ISO-8601 timestamp of the PR head commit DIRECTIVE the override directive, e.g. /approve-override """ @@ -81,57 +96,97 @@ def extract_reason(body, directive): return None -def main(): - directive = os.environ.get("DIRECTIVE", "/approve-override").strip() - head_time = parse_ts(os.environ.get("HEAD_TIME", "")) - +def load_source(env_name, label): + """Return a list of items for one source, or [] with a stderr note if it is unusable.""" + raw = os.environ.get(env_name) + if not raw or not raw.strip(): + return [] try: - comments = json.loads(os.environ.get("COMMENTS_JSON") or "[]") + parsed = json.loads(raw) except (ValueError, TypeError): - # The caller validated this JSON already; if it is broken here, say so loudly on - # stderr and emit no candidate. Silence on stdout means "no override". - print("approval-override: COMMENTS_JSON is not valid JSON", file=sys.stderr) - return 0 - if not isinstance(comments, list): - print("approval-override: COMMENTS_JSON is not a list", file=sys.stderr) - return 0 + print( + "approval-override: %s is not valid JSON — ignoring that source" % label, + file=sys.stderr, + ) + return [] + if isinstance(parsed, list): + return parsed + # An error object, typically {"message": "token does not have ... scope"}. Say which, + # because "no override found" and "could not read the overrides" look identical from + # the outside and the first wasted a real debugging session. + detail = "" + if isinstance(parsed, dict): + detail = str(parsed.get("message") or parsed.get("errors") or "")[:200] + print( + "approval-override: %s is not a list (Gitea returned %s%s) — ignoring that source" + % (label, type(parsed).__name__, ": " + detail if detail else ""), + file=sys.stderr, + ) + return [] - candidates = [] - for comment in comments: - if not isinstance(comment, dict): + +def candidates_from(items, source, directive, head_time): + """Yield (when, login, reason, raw_when, source) for every valid override in one source.""" + found = [] + for item in items: + if not isinstance(item, dict): continue - login = ((comment.get("user") or {}).get("login") or "").strip() - reason = extract_reason(comment.get("body"), directive) + login = ((item.get("user") or {}).get("login") or "").strip() + reason = extract_reason(item.get("body"), directive) if not login or not reason: continue - created_raw = comment.get("created_at") or comment.get("updated_at") or "" - created = parse_ts(created_raw) - if created is None: + # A PENDING review is an unsubmitted private draft — not an act of anything. + state = str(item.get("state") or "").upper() + if source == "review" and state == "PENDING": print( - "approval-override: dropping override by '%s' — unreadable timestamp %r" - % (login, created_raw), + "approval-override: dropping PENDING (unsubmitted) review by '%s'" % login, + file=sys.stderr, + ) + continue + raw_when = ( + item.get("submitted_at") or item.get("created_at") or item.get("updated_at") or "" + ) + when = parse_ts(raw_when) + if when is None: + print( + "approval-override: dropping %s override by '%s' — unreadable timestamp %r" + % (source, login, raw_when), file=sys.stderr, ) continue # An override must post-date the commit it covers. - if head_time is not None and created <= head_time: + if head_time is not None and when <= head_time: print( - "approval-override: dropping STALE override by '%s' (%s) — not newer than " - "head commit (%s). Re-comment to override the current commit." - % (login, created.isoformat(), head_time.isoformat()), + "approval-override: dropping STALE %s override by '%s' (%s) — not newer than " + "head commit (%s). Re-post it to override the current commit." + % (source, login, when.isoformat(), head_time.isoformat()), file=sys.stderr, ) continue - candidates.append((created, login, reason, created_raw)) + found.append((when, login, reason, raw_when, source)) + return found - if not candidates: + +def main(): + directive = os.environ.get("DIRECTIVE", "/approve-override").strip() + head_time = parse_ts(os.environ.get("HEAD_TIME", "")) + + found = [] + found += candidates_from( + load_source("COMMENTS_JSON", "COMMENTS_JSON"), "comment", directive, head_time + ) + found += candidates_from( + load_source("REVIEWS_JSON", "REVIEWS_JSON"), "review", directive, head_time + ) + + if not found: return 0 - # Latest wins: a later override supersedes an earlier one. - created, login, reason, created_raw = max(candidates, key=lambda c: c[0]) + # Latest wins, across both sources: a later override supersedes an earlier one. + when, login, reason, raw_when, source = max(found, key=lambda c: c[0]) # Keep the reason on one line — this becomes a TSV field. reason = " ".join(reason.split()) - print("%s\t%s\t%s" % (login, created_raw, reason)) + print("%s\t%s\t%s\t%s" % (login, raw_when, source, reason)) return 0 diff --git a/ci/approval-policy.sh b/ci/approval-policy.sh index f541871..9855ebb 100755 --- a/ci/approval-policy.sh +++ b/ci/approval-policy.sh @@ -103,8 +103,15 @@ REVIEWS_JSON="$(api "${GITEA_URL}/api/v1/repos/${GITEA_REPO}/pulls/${PR}/reviews # PR conversation comments live under the ISSUES endpoint. `/pulls/{n}/comments` returns review # (diff) comments ONLY, so an override posted in the normal comment box would never appear in it. # Verified against this Gitea (1.24.3) rather than assumed. +# +# This one is NOT in the must-parse loop below, and that is deliberate. /issues/{n}/comments sits +# behind Gitea's *issue* token scope, which `SETTINGS_API_TOKEN` does not carry (it is minted +# `[read:repository, read:admin]`) — so it returns an error OBJECT, not an array. Blocking on that +# turned "CI cannot read your override" into a message that said "you did not post one", and cost a +# real debugging session on PR #38. It is handled explicitly, and loudly, further down: an +# unreadable source can only cause a false BLOCK, never a false pass. COMMENTS_JSON="$(api "${GITEA_URL}/api/v1/repos/${GITEA_REPO}/issues/${PR}/comments?limit=1000" || true)" -for v in FILES_JSON PR_JSON REVIEWS_JSON COMMENTS_JSON; do +for v in FILES_JSON PR_JSON REVIEWS_JSON; do jq -e . >/dev/null 2>&1 <<<"${!v}" || { err "could not read ${v} from Gitea for PR #${PR} — BLOCKING rather than assuming." summary "- approval-policy: **BLOCKED — Gitea unreadable**" @@ -170,8 +177,23 @@ if [ "$OVERRIDE_ENABLED" = "true" ]; then exit 1 fi - # Pure, offline, unit-tested selection: which comment, if any, is a live override? - OVERRIDE_TSV="$(COMMENTS_JSON="$COMMENTS_JSON" HEAD_TIME="$HEAD_TIME" DIRECTIVE="$DIRECTIVE" \ + # Is the comments source actually readable? Say so plainly if not — a scope problem and "you + # never posted an override" are indistinguishable from the outside, and conflating them is what + # made PR #38 look like a code bug. + if ! jq -e 'type == "array"' >/dev/null 2>&1 <<<"${COMMENTS_JSON:-}"; then + SCOPE_MSG="$(jq -r '.message // empty' <<<"${COMMENTS_JSON:-null}" 2>/dev/null || true)" + warn "cannot read PR COMMENTS from Gitea${SCOPE_MSG:+ — ${SCOPE_MSG}}." + warn " /issues/${PR}/comments needs the *issue* token scope; SETTINGS_API_TOKEN is minted" + warn " [read:repository, read:admin] (infra/gitea/set-ci-token.sh). An override posted as an" + warn " ordinary PR COMMENT is therefore invisible to CI — post it as a REVIEW of type" + warn " 'Comment' instead, which /pulls/${PR}/reviews exposes under read:repository." + summary "- approval-policy: note — PR comments unreadable (token lacks the issue scope); use a **review** to override" + COMMENTS_JSON='[]' + fi + + # Pure, offline, unit-tested selection: which comment or review, if any, is a live override? + OVERRIDE_TSV="$(COMMENTS_JSON="$COMMENTS_JSON" REVIEWS_JSON="$REVIEWS_JSON" \ + HEAD_TIME="$HEAD_TIME" DIRECTIVE="$DIRECTIVE" \ python3 "${CI_DIR}/approval-override.py")" || { err "ci/approval-override.py failed — BLOCKING rather than assuming there is no override." summary "- approval-policy: **BLOCKED — override parser failed**" @@ -180,7 +202,8 @@ if [ "$OVERRIDE_ENABLED" = "true" ]; then if [ -n "$OVERRIDE_TSV" ]; then OV_USER="$(cut -f1 <<<"$OVERRIDE_TSV")" OV_WHEN="$(cut -f2 <<<"$OVERRIDE_TSV")" - OV_WHY="$(cut -f3- <<<"$OVERRIDE_TSV")" + OV_SRC="$(cut -f3 <<<"$OVERRIDE_TSV")" + OV_WHY="$(cut -f4- <<<"$OVERRIDE_TSV")" # AUTHORITY IS MEASURED LIVE, never inferred from the comment or from a name in git. PERM_JSON="$(api "${GITEA_URL}/api/v1/repos/${GITEA_REPO}/collaborators/${OV_USER}/permission" || true)" @@ -214,12 +237,13 @@ print("yes" if (ok_perm and ok_list) else "no") else warn "ADMIN OVERRIDE by '${OV_USER}' (permission '${OV_PERM}'); author is '${AUTHOR}'." fi + warn " via: PR ${OV_SRC}" warn " when: ${OV_WHEN} (head commit ${HEAD_SHA:0:8}, ${HEAD_TIME})" warn " reason: ${OV_WHY}" warn " review waived on: $(printf '%s ' $SENSITIVE)" ok "sensitive paths cleared by administrator override — this stage does not block." SELFNOTE=""; [ "$OV_USER" = "$AUTHOR" ] && SELFNOTE=", **self-waived as author**" - summary "- approval-policy: **OVERRIDDEN by \`${OV_USER}\`** (permission \`${OV_PERM}\`${SELFNOTE})" + summary "- approval-policy: **OVERRIDDEN by \`${OV_USER}\`** (permission \`${OV_PERM}\`, via PR ${OV_SRC}${SELFNOTE})" summary " - reason: ${OV_WHY}" summary " - review waived on: $(printf '\`%s\` ' $SENSITIVE)" exit 0 @@ -239,10 +263,12 @@ err " honestly approve for itself. Get a human review, or move the change out o printf ' %s\n' $SENSITIVE >&2 err " The list lives in infra/gitea/approval-policy.json, with a reason per entry." if [ "$OVERRIDE_ENABLED" = "true" ]; then - err " Or, if you hold admin/owner on this repository, comment on the PR:" + err " Or, if you hold admin/owner on this repository, post this on the PR:" err " ${DIRECTIVE} " - err " The reason is mandatory, the directive must START a line, and the comment must be" - err " NEWER than head commit ${HEAD_SHA:0:8} — push again and you must override again." + err " as a REVIEW of type 'Comment' (not the plain comment box — CI's token cannot read" + err " /issues/${PR}/comments without the issue scope; see infra/gitea/set-ci-token.sh)." + err " The reason is mandatory, the directive must START a line, and it must be NEWER than" + err " head commit ${HEAD_SHA:0:8} — push again and you must override again." fi summary "- approval-policy: **BLOCKED — needs a non-author approval, or an admin \`${DIRECTIVE}\` comment**" exit 1 diff --git a/ci/tests/approval-override.test.sh b/ci/tests/approval-override.test.sh index d792d4b..4d3364e 100755 --- a/ci/tests/approval-override.test.sh +++ b/ci/tests/approval-override.test.sh @@ -26,7 +26,13 @@ BEFORE="2026-10-01T11:30:00Z" # run -> stdout of the parser (stderr is diagnostic only) run() { - COMMENTS_JSON="$1" HEAD_TIME="${2:-$HEAD_TIME}" DIRECTIVE="${3:-/approve-override}" \ + COMMENTS_JSON="$1" REVIEWS_JSON="" HEAD_TIME="${2:-$HEAD_TIME}" DIRECTIVE="${3:-/approve-override}" \ + python3 "$SUT" 2>/dev/null +} + +# run_rev — the same, but through the REVIEWS source. +run_rev() { + COMMENTS_JSON="" REVIEWS_JSON="$1" HEAD_TIME="${2:-$HEAD_TIME}" DIRECTIVE="${3:-/approve-override}" \ python3 "$SUT" 2>/dev/null } @@ -54,10 +60,24 @@ expect_user() { expect_reason() { local label="$1" json="$2" want="$3" out got out="$(run "$json")" - got="$(cut -f3- <<<"$out")" + got="$(cut -f4- <<<"$out")" if [ "$got" = "$want" ]; then ok "$label"; else bad "$label (wanted '${want}', got '${got}')"; fi } +# check