feat(2036): three org audits authenticate as the App; caller-drift keeps the PAT by decision by LukasWodka · Pull Request #278 · tracebloc/.github

LukasWodka · 2026-08-16T14:23:03Z

Refs #2036.

Decision: option B (Lukas, 2026-08-17). This started as four audits and ships three, because the fourth structurally cannot use a least-privilege identity.

audit	identity
`standards-sync`	tracebloc-release-train App
`merge-settings-drift`	tracebloc-release-train App
`bricked-prs`	tracebloc-release-train App
`caller-drift`	keeps `PROJECTS_KANBAN_TOKEN`, by decision

Why `caller-drift` can't move

It's the only audit asserting ruleset bypass allowlists, and GitHub returns bypass_actors only to a caller with write access to the ruleset — "to prevent leaking sensitive information."

Measured rather than assumed: administration: read was granted mid-review and fixed all 52 branch-protection reads while leaving every bypass_actors withheld. The only sufficient grant is administration: write, which would let an App invoked by ~14 workflows on every PR and push rewrite branch protection and every ruleset in the fleet — including the v* tag trust root whose bypass list this audit exists to police. Granting write over the trust root to read who may bypass it makes the auditor one of the actors it audits.

So #2036 closes with the PAT alive for exactly one consumer. That's the safest place for it: a weekly cron, never event-triggered, read-only. And the failure that filed #2036 — advance-deploy-env exhausting the quota mid-hop and stranding a card — was a hot-path problem, which is fixed.

The reasoning is recorded at the step, with an explicit "do not finish #2036 by pointing this at the App" and the condition under which it would become migratable.

The five correctness fixes — the real value here

All found by Bugbot in review, all independent of which credential runs the audit. Three were bugs I introduced fixing the previous one.

#	fix	sev
1	`merge-settings-drift` trusted a non-empty org listing as complete	High
2	absent `bypass_actors` became `[]`, silently satisfying `promotion_merge_commit_only` on all 32 promotion branches	High
3	a declared repo missing from the listing was reported as having left the org, advising removal of a legitimate entry	Med
4	listing gaps distorted `evaluated` and could trip the `die()` that discards the whole report	High
5	listing gaps counted as caller/copy read failures, so the headline named the wrong fix	Med

#2 is the one worth reading. It was a fail-open unreachable under the org-admin PAT — the PAT always saw the field, so folding an absent one into [] never mattered. Moving to a least-privilege identity made it reachable, and it would have passed the promotion-ruleset assertion without ever reading the allowlist.

Verification

Selftest 176 → 190, every new case mutation-anchored.

One first attempt was found vacuous by mutation — it passed under the exact change it existed to catch (a bucket dropped at the call site). The wiring is now asserted from the declarations, so a future bucket is covered when it's declared rather than when someone remembers to test it.

Note

Medium Risk
Changes org-wide audit authentication and fleet-coverage semantics; mistakes could fail audits or mis-report compliance, but caller-drift retains the privileged PAT only on a weekly cron and the PR deliberately closes several fail-open paths.

Overview
Credential change (backend#2036):bricked-prs, merge-settings-drift, and standards-sync mint an org-scopedtracebloc-release-train installation token (actions/create-github-app-token with owner:) and use it for all GH_TOKEN usage. There is no PAT fallback so a broken App path cannot stay green silently.

caller-drift stays on PROJECTS_KANBAN_TOKEN by design: it is the only audit that asserts ruleset bypass allowlists, and GitHub only returns bypass_actors to callers with write access to the ruleset—granting administration: write on the App would let hot-path workflows rewrite the same rulesets this job polices.

Fail-closed fixes exposed by narrower token visibility:merge-settings-drift now checks out repo-inventory.yml and refuses a non-empty org listing that does not cover every declared repo. In caller-drift.py, missing bypass_actors is treated as unreadable (bypass_present), not an empty allowlist; declared-but-unlisted repos are probed and recorded in a separate listing_unreadable bucket; coverage() and caller_read_failures() keep evaluated and watchdog headlines from blaming listing gaps as caller/copy failures. The workflow surfaces LISTING_UNREADABLE in the watchdog phrase.

Tests:caller-drift-selftest adds mutation-anchored cases and source-derived wiring checks so every *_unreadable output is passed to caller_read_failures and read by caller-drift.yml.

^{Reviewed by Cursor Bugbot for commit 6e4b67d. Bugbot is set up for automated code reviews on this repo. Configure here.}

Ten token sites across four workflows, all in this repo: caller-drift 4 standards-sync 3 merge-settings-drift 2 bricked-prs 1 None of these gates a promotion, so they are the safe half of what remains. `fr-gate` is deliberately NOT here: it is a required check on every promotion, so if its board read breaks you cannot promote the fix. It gets its own PR and a quiet window. WHY THIS ORDER. Of the six workflows still on PROJECTS_KANBAN_TOKEN, five live in this repo alone and one -- add-to-kanban.yml -- is a per-repo COPY in 19 repos. Doing the five single-repo ones first takes the migration from 8/14 to 13/14 for roughly two PRs, and leaves the expensive 19-repo sweep as the only remaining piece rather than mixing it in. THE TWO "token is present" GUARDS ARE KEPT, pointed at the minted token. A failed mint fails its own step, so they look redundant -- but what they actually guard is an EMPTY token reaching the audit, and empty is the fail-OPEN direction: the org listing returns nothing and every check passes against an empty scope. That reasoning does not change with the token's provenance, so the assertions stay. Deleting them because the new path "can't fail" is how a real guard becomes an inert one. NO REPO-ADMINISTRATION PERMISSION IS NEEDED, and merge-settings-drift is why that is worth stating. It contains `gh api -X PATCH repos/...` three times, which reads like it mutates repo settings. It does not: those lines are inside `printf`, and are remediation commands the report PRINTS for a human to run. Granting the App `administration: write` for a report-only cron would have been a serious over-grant, and the grep that suggested it was wrong. standards-sync's header planned a separate fine-grained STANDARDS_SYNC_TOKEN in case PROJECTS_KANBAN_TOKEN lacked contents-write. Moot: the App carries contents:write and pull_requests:write, and a second fleet-wide credential is the thing backend#2036 exists to stop. Header updated to say so. `owner:` on every mint makes the token ORG-scoped -- a repo-scoped token cannot enumerate the org, which is the whole job of these four. No fallback to the PAT anywhere: a fallback would let a broken App path keep reporting green, and an audit that cannot fail is the exact defect backend#1729 catalogued. After this, only `add-to-kanban.yml` (19-repo copy) and `fr-gate.yml` remain on the PAT. PROJECTS_KANBAN_TOKEN stays live and untouched until both land. Verified: actionlint clean; all four parse; caller-drift-selftest 176 pass / 0 fail. Refs backend#2036 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

saadqbal

Red for a real reason, and it's this PR — not a secret-missing flake. audit (caller-drift) failed with 6 findings, all tag_trust_root “bypass actors: missing [OrganizationAdmin, Team:18304481]” (backend, cli, client, data-ingestors, design-system, tracebloc-py-package). gate is just downstream — it requires caller-drift green on this SHA.

They're false positives from the token swap. evaluate_rulesets reads bypass actors from GET /repos/{o}/{r}/rulesets/{id}, and GitHub omits bypass_actors from that payload for a caller without repo administration — a 200, not a 403, so the audit reads them as genuinely missing rather than unreadable (exit 1, not exit 2). The org-admin PAT saw them; the release-train App installation carries no administration:read, so it can't. Same audit code was green on the PR merged ~36 min earlier and on develop/main — the only delta is PAT→App. The token minted fine (“Token revoked” at cleanup), so this isn't the self-referential “App secret absent on a PR run” case.

So caller-drift doesn't belong in the “safe, no-admin” batch: it's the one audit here that genuinely needs administration:read (for ruleset bypass actors). The other three don't touch that field — bricked-prs reads rules/branches + classic (bypass-free), merge-settings reads the repo object, standards-sync reads contents — so they're fine. Fix is org-side: grant the App administration:read and re-run. It can't be done in this YAML (you can't request a permission the installation lacks). Until then this fires red on every scheduled run too, against watchdog #1781.

Rest of the PR is clean: key only passed as an action input (masked, no set -x, gitleaks green, token not persisted), the empty-token guards correctly kept and repointed, all four fully converted, pin matches the repo's other 11 app-token uses. Smaller note: no permission-* on any mint, so each audit gets the installation's full set rather than a per-audit least-priv slice — matches existing convention and wouldn't have changed this outcome, but the per-audit “no admin needed” comments describe an intent the mint doesn't enforce. Not approving while red.

…one (Bugbot, #278) The zero-repo guard was sufficient while this ran as an org-member PAT, whose listing was the whole org by construction. An App installation token sees only what the installation covers, so narrowing the installation -- or adding a repo to the org and not to the App -- silently shrinks the audited set while every remaining repo passes and the job exits green. That is the exact fail-open this audit exists to refuse, reintroduced by the credential change rather than by any logic here. The other three migrated audits avoid it because they drive from repo-inventory.yml; this one enumerated and trusted the enumeration. So compare the listing against the DECLARED set: any repo repo-inventory.yml declares that the listing did not return means the listing is incomplete, not that the repo complies. Derived from the declaration, never from a second hand-maintained list. An extra repo in the listing is NOT a finding here -- a repo in the org but not yet in the inventory is caller-drift's business, and duplicating that assertion would put the same rule in two places to drift apart. A declared repo can also vanish from the listing by being archived, since the filter drops archives. Still a finding: an archived repo carrying an inventory entry is drift someone should look at, and refusing costs less than guessing which case it is. The message names both causes. Adds actions/checkout (for repo-inventory.yml) and actions/setup-python, same pins and version as the sibling audits. setup-python rather than the system interpreter because a bare pip install on the runner hits PEP 668. Verified by extracting the generated `run:` script from the YAML and checking it for real, rather than trusting that the heredoc survives block-scalar indentation stripping: `bash -n` clean, terminator resolves, actionlint clean. Guard proved in three directions -- complete listing passes, a listing missing 3 declared repos fails and names them, a listing with an extra repo passes. NOTE: this does not turn #278 green. The `audit` job still reports 6 findings from ruleset `bypass_actors` the App cannot see; that is the open question on saadqbal's thread and is not fixed here. Refs backend#2036 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…y allowlist (Bugbot, #278) TWO findings, and the first is a fail-open I missed while diagnosing the false-red beside it. 1. ABSENT IS NOT EMPTY. GitHub returns `bypass_actors` only to a caller with WRITE access to the ruleset ("to prevent leaking sensitive information"); everyone else gets a 200 with the field simply not there. `raw.get("bypass_actors") or []` folded that into an empty allowlist -- and `promotion_merge_commit_only` asserts exactly `bypass_actors: []`, so the assertion MATCHED without the field ever having been read. Fail-open on the allowlist this audit exists to enforce, on all 32 promotion branches. The tag rulesets fail the other way and go falsely red, which is the 6 findings on this PR -- same root cause, opposite symptom, and only the red one was visible. `read_rulesets` now refuses a ruleset whose `bypass_actors` was withheld, rather than each comparison having to remember to check. Same rule the docstring already stated for the cheaper endpoints, applied to the case where the RIGHT endpoint answers 200 and still withholds the field. Unreachable under the org-admin PAT, which always saw the field. backend#2036 moved this onto a least-privilege identity and made it reachable. 2. A DECLARED REPO MISSING FROM THE LISTING IS NOT PROOF IT LEFT THE ORG. Under the org-member PAT the listing WAS the org, so `inventory - active` could only mean archived/renamed/deleted and "remove the entry" was sound advice. An App token lists only what the installation covers, so that set now also holds repos that were never read -- and following the old advice would delete a legitimate entry and permanently shrink the audited fleet. Probes each one instead of guessing: readable-and-archived/fork stays a finding; readable-and-active means the LISTING was incomplete; unreadable means we cannot tell. The last two suppress an all-clear instead of manufacturing a finding. Bugbot correctly noted the previous commit applied this rule to merge-settings-drift's listing and left the identical flaw in its sibling. Three selftest cases, and the middle one is the mutation anchor: present-and-empty must stay a real assertable value, or the fix would make every legitimate empty allowlist unreadable and break the 32 branches it protects. MUTATION PROVED, not assumed. Forcing `bypass_present = True` (the old behaviour) reddens all three new cases, and the mutant emits `bypass actors: missing ['OrganizationAdmin', 'Team:18304481']` -- byte-for-byte the symptom seen on this PR. Restored: 179 pass / 0 fail (was 176). CONSEQUENCE, stated rather than discovered later: with this fix the App identity can no longer evaluate rulesets AT ALL -- the 6 findings become ~32 unreadable records. The run stays red, honestly now instead of half-silently. Whether caller-drift can run as the App is the open question on saadqbal's thread; this commit does not answer it, it just stops the answer being faked. Refs backend#2036 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…Bugbot, #278) Right, and it would have aborted exactly when the probe mattered most. `evaluated` drives a die() that discards the whole report, and it is computed as `len(audited) - len(names in unreadable)`. That arithmetic has one precondition, stated in the comment above it: every name in `unreadable` is an AUDITED one. The stale-inventory probe I added records repos from `inventory - active`, which are by definition not in `audited`, so each one subtracted a name that was never in the total. Enough listing gaps and `evaluated <= 0` fires on a run with plenty of verdicts -- and "enough" is the partial-installation case the probe exists to describe. Two changes: 1. Listing gaps get their OWN bucket, `listing_unreadable`, merged in only after `evaluated` is computed. Exactly the pattern protection and rulesets already use, for exactly the reason their comment gives. I should have followed it. 2. The arithmetic is extracted to `coverage()` and made tolerant rather than trusting: only names actually in `audited` count against it. The precondition stays the caller's to keep, but honouring it in one place means a future fourth bucket wired to the wrong list cannot silently abort every run. Extracted rather than fixed inline because of backend#1729 rule 9: a mutation check must call the code under test, not a copy. Asserting this sum inline in the selftest would have the test agree with its own arithmetic while production drifted. Now the selftest and the mutation both go through `coverage()`. Four cases, including both directions: a genuinely unreadable AUDITED repo must still reduce coverage to zero, or the fix would trade a false abort for a run that can never abort at all. MUTATION PROVED. Restoring `len(audited) - len(seen)` reddens the listing-gap case and the zero case, and the mutant returns -2 -- negative coverage, which is the `evaluated <= 0` that trips the die(). Restored: 183 pass / 0 fail (was 179). Refs backend#2036 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…r-read failure (Bugbot, #278) Right again, and the comment at the site I broke already explains the class: "a true count under a false name, pointing at the wrong problem. (Bugbot, #238.)" That decomposition exists precisely so a reader is sent to the right fix; adding a fourth bucket without updating the subtraction reintroduced it, and the wrong fix this one points at is "debug the caller reads" when the answer is "widen the App installation". Exit stayed fail-closed throughout, as Bugbot noted. Only the headline lied. - decide_exit() takes `listing_unreadable` and gives it its OWN clause, so all four causes are named separately instead of three named and one absorbed. - The caller/copy count is derived ONCE, by `caller_read_failures()`, and passed to both consumers. It was two inline subtractions, which is how a new bucket landed in one and not the other. - Variadic and clamped: buckets are passed positionally so a new one is either passed or visibly missing, and a double-counted bucket clamps at 0 rather than producing a negative headline. ON COVERAGE, because the first attempt was vacuous and saying so is the point. Breaking caller_read_failures() reddens its cases. Dropping the bucket AT THE CALL SITE did not -- 189 passed under that mutation, and a call-site omission is exactly what the bug was. A test that survives the mutation it is meant to catch is not coverage. So the wiring is asserted from the SOURCE: every `*_unreadable` bucket main() declares must appear in the call. Derived from the declarations, so a fifth bucket is covered the moment it is declared rather than when someone remembers to test it. Both mutations now redden -- the call-site one names the missing bucket. 190 pass / 0 fail (was 185). Refs backend#2036 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…the PAT by decision Decision: option B (Lukas, 2026-08-17). This PR started as four audits and ships three, because the fourth structurally cannot use a least-privilege identity. standards-sync, merge-settings-drift and bricked-prs authenticate as the tracebloc-release-train App. caller-drift does not, and that is now written down at the step where someone would otherwise "finish" the migration. WHY caller-drift CANNOT. It is the only audit asserting ruleset bypass allowlists, and GitHub returns `bypass_actors` only to a caller with WRITE access to the ruleset ("to prevent leaking sensitive information"). Measured, not assumed: `administration: read` was granted mid-review and fixed all 52 branch-protection reads while leaving every `bypass_actors` withheld. The only sufficient grant is `administration: write`, which would let an App invoked by ~14 workflows on every PR and push REWRITE branch protection and every ruleset in the fleet -- including the `v*` tag trust root whose bypass list this audit exists to police. Granting write over the trust root to read who may bypass it makes the auditor one of the actors it audits. So #2036 closes with the PAT alive for exactly one consumer, and that is a better end state than the alternative. It is also the safest place for a privileged credential to remain: a weekly cron, never event-triggered, so nothing an outside contributor submits can influence when it runs, and it never writes. The failure that filed #2036 -- advance-deploy-env exhausting the quota mid-hop and stranding a card -- was a hot-path, per-push problem, and that is fixed. THE FIVE CORRECTNESS FIXES STAY, and they are the real value here. All were found by Bugbot during review, all are independent of which credential runs the audit, and three were bugs I introduced while fixing the previous one: 1. merge-settings-drift trusted a non-empty org listing as complete 2. an absent `bypass_actors` became `[]`, silently satisfying the promotion_merge_commit_only assertion on all 32 promotion branches -- a fail-open that was unreachable under the org-admin PAT 3. a declared repo missing from the listing was reported as having left the org, advising removal of a legitimate inventory entry 4. listing gaps distorted `evaluated` and could trip the die() that discards the whole report, precisely in the partial-coverage case the probe describes 5. listing gaps were counted as caller/copy read failures, so the headline named the wrong fix Selftest 176 -> 190. Every new case mutation-anchored, and one first attempt was found VACUOUS by mutation -- it passed under the exact change it existed to catch, so the wiring is now asserted from the declarations rather than from memory. Refs backend#2036 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…(Bugbot, #278) Third consumer of the decomposed counts, third time I updated some and not all. decide_exit, the step outputs and the watchdog phrase were each edited separately, and each was missed once. That is not three mistakes so much as one missing guard. The phrase builder covered caller/protection/ruleset and fell through to the merged count otherwise -- but the fallback fires only when the sentence is ENTIRELY empty, so a listing gap MIXED with any other cause was dropped from the headline rather than mis-named. The reader is then told the wrong thing to go fix, which is exactly what the decomposition exists to prevent (Bugbot #238, and now twice more). Fixed: `listing_unreadable` is wired into the step env and gets its own clause, naming "fleet coverage UNKNOWN" -- whose fix is "widen the App installation", which no other clause would ever suggest. AND THE GUARD, because a fourth miss is otherwise a matter of time. Two assertions, both derived from what the script EMITS rather than from a list a human maintains: 1. every `*_unreadable` output the script writes is read by the workflow 2. every one of them gets its OWN clause in the watchdog phrase They catch different failures, which is why both exist: removing the env wiring trips (1) only; keeping the env and deleting its clause trips (2) only -- and (2) is the shape that just happened. Mutation-proved in both directions. A fifth bucket is now covered the moment it is written out, rather than when someone remembers three separate consumers. 192 pass / 0 fail (was 190). Refs backend#2036 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…tually uses (Bugbot, #278) The same class one more time, and this one I wrote INTO the fix for the previous one. Option B kept caller-drift on PROJECTS_KANBAN_TOKEN, but the diagnostics were written while it was being migrated and still named an App installation token this audit never mints: - the empty-token guard announced "The installation token is empty" while reading the PAT - listing-gap records told the operator to "widen the App installation" - decide_exit's clause comment said the same A real failure under the wrong fix -- exactly what the decomposition work in this PR keeps closing, aimed at itself. Fixed by role, not by name: - caller-drift.yml names PROJECTS_KANBAN_TOKEN, because that is what it reads and an operator grepping for the empty secret needs the real name - caller-drift.py's messages are CREDENTIAL-AGNOSTIC ("this audit's token", "the token's repo visibility"), because the script has now outlived one migration inside a single PR and will outlive others - merge-settings-drift is untouched: it IS on the App, so naming the App installation there is correct and remains No behaviour change; every assertion and exit code is identical. This is the sentence a human reads at 3am deciding what to go fix, which is the only reason the decomposition exists at all. 192 pass / 0 fail. Refs backend#2036 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 087f3a3. Configure here.}

…to protect (Bugbot, #278) The sharpest one in this PR: the guard added to stop a bucket being dropped could not see one of the four buckets, and it was the bucket that had already been dropped three times. `_emitted` matched `handle.write(f"x_unreadable=` on a single line. `caller_unreadable` is written across two, so it never entered the set. Measured: _emitted (old regex): listing, protection, ruleset <- 3 of 4 actual output writes: caller, listing, protection, ruleset So deleting CALLER_UNREADABLE from the workflow env left ALL THREE wiring assertions green. A guard blind to a quarter of what it guards is worse than no guard, because it reports coverage it does not have -- which is the whole subject of backend#1680 and the reason these assertions exist at all. Keyed on the EMITTED STRING LITERAL now (`"x_unreadable=`) rather than on the shape of the call wrapped around it. The literal is what actually reaches GITHUB_OUTPUT, so it is the thing the workflow consumes; matching the call shape was matching the formatting, and formatting is not the contract. FULL MUTATION MATRIX, 4 buckets x 2 directions, all 8 now caught: drop the env wiring caller/protection/ruleset/listing -> all FAIL drop the watchdog clause caller/protection/ruleset/listing -> all FAIL Before this commit, both caller_unreadable rows passed. 192 pass / 0 fail; the count is unchanged because this fixes what the existing assertions SEE, not how many there are -- which is precisely why it needed the mutation matrix to find rather than a passing suite to confirm. Refs backend#2036 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

LukasWodka · 2026-08-19T15:58:46Z

/fr-pass

Best-effort FR passed (triage; behavioral evidence limited while e2e journey red — backend#2206). Advancing to Ready for prod.

LukasWodka self-assigned this Aug 16, 2026

cursorBot reviewed Aug 16, 2026
View reviewed changes

Comment thread.github/workflows/merge-settings-drift.yml

LukasWodka requested a review from saadqbal August 16, 2026 19:01

saadqbal reviewed Aug 17, 2026
View reviewed changes

Comment thread.github/workflows/caller-drift.yml Outdated