fix(auto-approve): poll ci state with github_token, not the approving pat - #209
Merged
Conversation
… pat The v0.36.1 release cut stalled ~70 min and was cancelled by hand. The bump PR had every check green and was mergeable, but was never approved: wait-for-ci.sh failed its first check-runs call 5 times and default-denied. BOT_APPROVER_PAT is a fine-grained PAT, and fine-grained PATs cannot call the Checks API at all. There is no Checks permission to grant, so this was never a token-configuration fix. Reading CI state needs no distinct identity though; only the approval does, because GitHub forbids self-approval. Split the two: - new ci-read-token input, used as GH_TOKEN on the CI-wait step only, falling back to github.token. The PAT stays on check-pr-ready, approve, auto-merge. - grant checks: read and statuses: read in the reusable workflow, and expose ci-read-token as an optional secret so a caller that cannot grant those can pass a classic PAT or App token instead. Callers must grant checks: read and statuses: read themselves: a reusable workflow can only downgrade the caller's permissions, never elevate them, so the workflow-side grant is inert on its own. Documented in both READMEs, the generated workflow doc, and mirrored into the composite smoke test. Diagnosability, the reason a 70-minute stall was the first symptom: surface gh's stderr instead of discarding it, and escalate both give-up paths from ::notice:: to ::error::. Neither changes the exit code, so the never-hard-fail contract holds. That error text is API-controlled and goes into a workflow command, so it is sanitized rather than trusted: CR as well as LF is collapsed (CR terminates a log line for the runner, so a raw one could forge a ::error:: line), non-ASCII is dropped so the length cap cannot split a UTF-8 sequence, and % is escaped so %0A/%25 cannot be decoded out of it. Failure paths that must not become new failures: last_error is reset per poll so a malformed-response poll cannot report an earlier API error and send the operator after token permissions; mktemp failure degrades to 'capture unavailable' rather than exiting 1 under set -e; and the error reader cannot abort the script through pipefail. Closes DEVOPS-1254
Second adversarial review pass on the commit below. The sanitizer covered only gh's stderr. Check-run names and commit-status contexts are written by whoever posted the check, GitHub documents no character restriction on them, and they reached the failed/cancelled/pending log lines raw - so they were the wider injection channel, and the one left open. A CR in a check name forged an ::error:: line. Extracted sanitize_for_log and applied it to both channels. A non-numeric wait-* input reached sleep/seq and aborted under set -e with no ci_green emitted at all, which is a red job for a direct consumer of the composite (the documented usage has no continue-on-error). Inputs are coerced with a warning, and the contract wording now says precisely what cannot exit non-zero rather than claiming every failure mode. The bail reported only the last error, so four real 403s followed by one malformed response hid the actionable fault; it now carries the first as well. The length cap is applied after escaping too, so the emitted line is really bounded instead of growing threefold. Negative test assertions written as bare '! grep' were inert: bash does not abort on a !-inverted command under set -e, so they only had effect as a test's final line. Two were silently powerless. Replaced with assert_no_match, which does fail. All 15 DEVOPS-1254 tests are now verified failing against main. Docs: the caller permissions are no longer framed as private-repo-only (GITHUB_TOKEN is scoped by the permissions block whatever the repo's visibility), no longer stated unconditionally now that ci-read-token can satisfy them instead, and the reusable workflow no longer claims to mint an App token.
loft-bot
reviewed
Aug 3, 2026
…st gaps Round-3 adversarial review plus the AI reviewer's comments on the PR. Two defects that belong here rather than in a follow-up: - numeric_or_default had no upper bound, so the accept/reject line was an accident of int64 overflow in [ -gt ]. A plausible fat-finger like 1000000000 was accepted, and `for attempt in $(seq 1 N)` then had to materialise the whole list before the first poll: no output, no ci_green, a hang until GitHub's 6-hour timeout. That is a strictly worse version of the stall this PR exists to fix. Inputs are now range-checked (length first, so overflow never decides) and the loop is arithmetic, so nothing can be materialised. - first_error was set once and never cleared when the error streak resolved, so a 403 that had already recovered was reported as the first cause of a later, unrelated parse-failure streak. That is the same misattribution as F1, one variable over. It resets with consecutive_errors now. Log-channel and diagnostic fixes: - the rejected wait value was interpolated unsanitized; the sanitizer definitions move above the input coercion so it can be sanitized there too. - escaping now runs before truncation, so the cap bounds the line actually emitted instead of a pre-escape length that could triple, and truncation is marked rather than severing a name mid-word. - the pending/cancelled lists get a wider cap: 300 chars is right for one hostile string and wrong for a legitimately long list, and that line is how an operator answers 'why is this still running?'. - the first-error clause is suppressed when it equals the last, so the same 400-char blob is not printed twice. - a cancelled-but-never-replaced check is now ::error::. Unlike the failed-checks path it is not red anywhere else, so that annotation is the only signal. The annotation level now follows one stated rule instead of being ad hoc. Tests. assert_no_match failed open on a malformed regex (grep rc=2 read as 'no match') - the fourth instance of an assertion that cannot fail, in the guard written to prevent exactly that. It now branches on the return code. Added the gaps the AI reviewer found: commit-status .context injection on both the failed and pending paths (only check-run .name was covered, though both feed the same lines), positive assertions on the pending/cancelled name tests (an empty sanitizer result would have yielded 'pending: <unnamed>' and still passed), and non-numeric WAIT_MAX_ATTEMPTS / WAIT_MIN_ATTEMPTS coverage. 65 -> 74 tests, all mutation-verified. Docs: the ci-read-token description was 11 lines where every other input is one, and auto-doc pads the whole column to its widest cell, so it rendered every generated table ~1400 chars wide. Shortened, long form stays in the prose. The caller-permission requirement is now consistently scoped to 'unless you pass ci-read-token' in all four places, and the README no longer asserts the public-repo leniency as fact for GITHUB_TOKEN - that was only ever established for the fine-grained PAT.
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The incident
The
v0.36.1release cut stalled and was cancelled by hand after ~70 minutes(run).
The bump PR it was waiting on had all 26 checks green and was
MERGEABLE, butwas never approved.
v0.36.1was never tagged.The auto-approve job reported SUCCESS while doing nothing:
Root cause
wait-for-ci.shis default-deny, and its first call,GET /repos/{owner}/{repo}/commits/{sha}/check-runs, failed all 5 tries in ~0.4seach.
Fine-grained PATs cannot call the Checks API at all. There is no
Checksentry in the fine-grained repository permission list, and the fine-grained
permissions reference lists no
/check-runsendpoints. So this was never fixableby reconfiguring the token.
It had "worked" until now only because every prior successful run was in the
public
loft-sh/vcluster, where/check-runsreturns HTTP 200 with nocredentials at all. The token's Checks access was never exercised. Across a
200-run sample in
vcluster-pro, the failing run is the only one that has everreached
wait-for-ci.sh.The fix
Reading CI state needs no distinct identity. Only the approval does, because
GitHub forbids self-approval. The action was passing one token to every step.
ci-read-tokeninput, used asGH_TOKENon the CI-wait step only, fallingback to
github.token. The PAT stays oncheck-pr-ready, Approve PR andEnable auto-merge.
checks: read+statuses: readon the reusable workflow's job, plusci-read-tokenas an optional secret, so a caller that cannot grant those canpass a classic PAT or App token instead.
statusesis needed too:wait-for-ci.shalso reads/commits/{sha}/status,which never got a chance to fail because check-runs bailed first.
Callers must grant the permissions themselves
This is load-bearing and the reason the caller PRs land first. Per GitHub, "the
GITHUB_TOKENpermissions passed from the caller workflow can be only downgraded(not elevated) by the called workflow", and anything a caller omits defaults to
none. The workflow-side grant here is inert on its own.Merge order:
main, incl. the staging copy that mirrors to OSS)v0.34)v0.35)v0.36)auto-approve-bot-prs/v1tagNote the composite is checked out at
ref: main, soaction.ymlandsrc/go live on merge here, while the reusable workflow's permission block only
reaches callers at step 7.
Callers with no
permissions:block (hosted-platform,loft-prod,vcluster-docs) inherit the repo default and need no change.Diagnosability
A 70-minute release stall was the first symptom because this failure was
invisible: the script always exits 0, the job sets
continue-on-error, and theerror text was discarded via
2>/dev/null. A permanent 403 and a transient blipproduced byte-identical logs.
Now the captured stderr is reported, and both give-up paths are
::error::rather than
::notice::. Neither changes the exit code, so the never-hard-failcontract is intact and required checks stay green.
That text is API-controlled and goes into a workflow command, so it is sanitized
rather than trusted: CR as well as LF is collapsed (CR terminates a log line for
the runner, so a raw one could forge an
::error::line), non-ASCII is droppedso the length cap cannot split a UTF-8 sequence, and
%is escaped so%0A/%25cannot be decoded out of it.Tests
60bats tests, up from54. Every new test was verified to fail against thepre-fix script. Beyond the token split they cover the failure paths that must not
become new failures:
the operator after token permissions for an unrelated fault)
TMPDIRdegrades instead of exiting 1 underset -e::error::bats60/60,shellcheck,actionlint,zizmorandmake check-docsallclean.
Reviewed
Adversarially reviewed with Codex before opening. It found the stale-error
misdiagnosis, the CR injection vector, the fatal-
mktempcontract violation andthe error-reader
pipefailabort, all of which are fixed here, plus the callerordering hazard above. It also caught that the hand-edited
AUTO-DOCinput tableclaimed a default the action does not declare; the table is now generated.
Follow-ups, deliberately not here
check-pr-ready.shandenable-auto-merge.shstill use the same2>/dev/null || fallbackpattern one step earlier in the chain.addressed by fix(auto-approve-bot-prs): merge directly and surface skips that need action #208, which is open and independent: it escalates those skips and
fixes
--jq '.mergeable // "null"'collapsing a realmergeable: falseinto"null". This PR does not touch those two scripts.fix(auto-approve-bot-prs): merge directly and surface skips that need action #208 and this PR are complementary halves of the same failure class and
conflict textually in three files (
action.yml,README.md,test/gh_mock.bash), all in comment/prose blocks where both sides addedadjacent lines. No functional overlap: fix(auto-approve-bot-prs): merge directly and surface skips that need action #208 does not touch
wait-for-ci.shandthis PR does not touch
check-pr-ready.shorenable-auto-merge.sh.Suggested order is this PR first (it unblocks a stalled release and is the one
under adversarial review, so rebasing it would invalidate that baseline), then
rebase fix(auto-approve-bot-prs): merge directly and surface skips that need action #208.
The release cut should abort in seconds when the bump PR will never be
approved, rather than waiting out its ~100 min ceiling.
pull-requests: writeon the job token appears unused (pre-existing).Closes DEVOPS-1254