fix(2590): the sync opens its PRs as a human, so Bugbot reviews them

[LukasWodka]

Summary

The sync opened its per-repo PRs with the tracebloc-release-train App token, so
the author was tracebloc-release-train[bot] (type: Bot). Cursor Bugbot keys
its review on the author's Cursor seat and a Bot has none — so Bugbot
reviewed 0 of 14 open sync PRs and bugbot / review failed closed on all of
them.

gh() now takes an explicit identity, and PR creation alone uses it
(SYNC_PR_AUTHOR_TOKEN, a PAT owned by a seated user). Fleet reads and the
branch push keep the App token — owner:-scoped, short-lived, not tied to one
account, which is backend#2036 and stays intact.

Evidence

Measured 2026-08-26, all 14 open sync PRs — zero Cursor Bugbot check runs:

No Bugbot check run on head 4bc5e085de02.
Waited 900s over 46 attempt(s) and Bugbot never reported a terminal verdict on
this head. Measured Bugbot latency is p50 164s / max 635s over 40 runs, so this
is not slowness.

Bugbot itself is healthy — every human-authored PR the same day got a verdict
(.github#347 success, backend#2568 neutral, backend#2569 pass,
model-zoo#188 pass). A manual bugbot run names the cause, because Cursor
attributes the request to the author and not the commenter: "Bugbot is not
enabled for your user on this team."

Reviewer and assignee

Both become SYNC_REVIEWER (saqlainsyed007) instead of GITHUB_ACTOR. The
dispatcher is now the author, and GitHub refuses an approving review from a
PR's own author — keeping GITHUB_ACTOR would deadlock every sync PR with a
required review nobody eligible could give. Four of the 14 were already in that
shape (docs#143, release-train#130, .github#344, claude-skills#39) and
were reassigned by hand.

The two roles are deliberately collapsed (against D31's usual split): nobody
"does the work" on a machine-generated prose sync.

Fails closed

An empty PAT is a hard per-repo refusal, not a fallback to the App token. The
fallback is the bug — it opens a bot-authored PR that looks identical to a
working one and that Bugbot silently skips. standards-sync.yml already refuses
a fallback in the other direction for the same reason.

Test plan

make selftests — 29 passed, 0 failed. The existing meta-selftest picked up the
new runner unprompted: "standards-sync-mutations.py calls its baseline guard
before its first write".

make lint / make check — both PASS.

make mutation-standards-sync — 5 mutations, each caught by the assertion
named for it, 0 stale / 0 malformed / 0 uncaught:

mutation	reddened
pr create loses token=, reverting to the App identity	pr create runs as the author PAT
an empty PAT silently falls back to the App token	an empty PAT opens NO PR
the PAT leaks onto the fleet-read path	only PR creation changes identity
reviewer reverts to GITHUB_ACTOR, who is now the author	requests review from SYNC_REVIEWER
SYNC_REVIEWER becomes the PR author	the reviewer is not the account that authors

selftests-cover initially refused this PR — "standards-sync-mutations.py
is not run by any mutation target" — because MUTATION_TARGETS is an explicit
list, not the glob. Wired, and it now reports all 19 selftests and 9 mutation
runners covered.

Two harness defects worth naming

Writing the mutations surfaced two ways a mutation tier can look like coverage
and be none. Both are fixed here and are not guarded in the sibling runners:

1. A mutation can apply cleanly and not compile. The first pr list case
   spliced token= mid-call → positional argument follows keyword argument.
   The suite could not import the file, crashed, printed no FAIL: line, and the
   run recorded UNCAUGHT — indistinguishable from a vacuous test. Every mutation
   is now compile()d first and reported as MALFORMED rather than counted.
2. A crash is not a catch (rule 10). The empty-PAT case first scripted only
   gh pr list, so a fallback over-ran the stub and raised out of the suite: red,
   but with no assertion naming the behaviour. The selftest now scripts the whole
   happy path so the assertion is what reddens, and the runner reports CRASHED
   separately so the distinction cannot be silently re-lost.

Before this can be dispatched

SYNC_PR_AUTHOR_TOKEN must exist as an org Actions secret — a repo-scoped PAT
owned by a seated Cursor user. Until it does, --create-prs refuses per repo
with a message naming the variable, which is the intended failure: no PRs beats
bot-authored PRs nothing reviews.

Out of scope

Dependabot.bugbot-gate-caller.yml:22-32 already records the first
instance of this class — "BUGBOT DOES NOT REVIEW DEPENDABOT PRs", sampled
2026-08-25 — and it is why bugbot / review is still advisory rather than
required. Dependabot's PRs cannot be re-authored, so this PR does not close that
half and step 3 of backend#2284 stays blocked on it.

Closes tracebloc/backend#2590

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes fleet-wide remediation auth and PR authorship for org standards sync; misconfigured SYNC_PR_AUTHOR_TOKEN disables remediation or aborts mid-run, but read-only audit still runs and the script fails closed rather than silently opening bot PRs.

Overview
Fixes org-standards sync PRs that Bugbot never reviewed because gh pr create used the release-train App token and made the author a bot with no Cursor seat.

The sync now splits credentials: the App token still does fleet reads, branch pushes, and CLAUDE.md writes; SYNC_PR_AUTHOR_TOKEN (a seated human PAT) is used only for gh pr create and reviewer/assignee edits. Reviewer and assignee are both SYNC_REVIEWER instead of GITHUB_ACTOR, so the author and required reviewer are not the same person.

Fail-closed behavior moved ahead of any writes: check_author_identity() validates the PAT (present, resolvable, not equal to SYNC_REVIEWER) in main() before the first branch push. A bad credential disarms remediation but still runs the read-only fleet audit, reports NOT REMEDIATED / REMEDIATION DISABLED, and exits 2. Existing open PRs with bot authors are errors (close and reopen as human); AuthorUnusable aborts the fleet on the first failed pr create to limit half-rollouts.

Workflow comments document the two-token model; Makefile wires mutation-standards-sync. standards-sync-selftest.py and new standards-sync-mutations.py pin identity split, gates, and bot-author handling.

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

[saqlainsyed007]

Really solid core — I verified the credential handling end to end and it's clean: the gh() helper passes the token via env=dict(os.environ, GH_TOKEN=token, GITHUB_TOKEN=token) to subprocess.run([...], env=…) with no shell=True, so the PAT is never in argv, never spliced into a shell string, never written to $GITHUB_ENV/output or the PR body; fleet reads/branch push/commit stay on the App token and only gh pr create/pr edit carry the PAT. The mutation-harness tests are genuinely rigorous. But a few things to resolve before I approve — this is the .github repo and the change is about fleet-wide credentials, so the bar is high.

Blockers

• F5 (same-PR doc obligation) — the workflow's own TOKEN header is now false. Lines 25-34 still say create-prs's Pull requests: RW need is "satisfied by the tracebloc-release-train App installation … it carries contents:write and pull_requests:write." After this PR the App token no longer opens the PRs — your new step comment (117-123) says exactly that. Same-file contradiction; please update the header in this PR. It also still asserts "a second fleet-wide credential is the thing backend#2036 exists to stop" — which is precisely what SYNC_PR_AUTHOR_TOKEN now is, so that paragraph needs reconciling with the new reality too.

• F4 (config-coupling deadlock) — SYNC_REVIEWER = "saqlainsyed007" is a hardcoded literal and nothing reads the PAT owner at runtime. The reviewer≠author invariant rests on that literal plus a selftest that hardcodes != "LukasWodka". If SYNC_PR_AUTHOR_TOKEN is ever re-provisioned to saqlainsyed007's PAT, author==reviewer, every --add-reviewer 422s, and the #2590 deadlock this PR fixes silently returns — with tests still green. Please assert reviewer ≠ (token owner) at runtime (resolve the PAT's login once via gh api user and compare), rather than trusting a literal.

Strong recommendation (security)

• Least privilege — use a fine-grained PAT, not a classic repo one.SYNC_PR_AUTHOR_TOKEN is a classic repo-scoped, long-lived, human-owned, fleet-wide credential — full code RW + issues + settings + webhooks on every repo the owner can reach — to do work that needs only pull_requests: write + contents: read. A fine-grained PAT scoped to those on the target repos is dramatically tighter and honors backend#2036's intent. The in-code rationale ("Needs repo scope … contents:write alone cannot open a PR") conflates "the App's contents:write can't" with "a fine-grained pull_requests PAT can't" — the latter can.

Should-fix (non-blocking)

• F1 — _ensure_pr early-returns before the reviewer/assignee block when an open PR already tracks the branch, so a PR that failed to get its (now merge-blocking) reviewer on an earlier run is never repaired on later runs. Re-request the reviewer on the existing-PR path too.
• F3 — the fail-closed PAT-empty guard lives inside _ensure_pr, after remediate() has already pushed the branch and committed CLAUDE.md fleet-wide; move the emptiness check into main() before any write so a missing token can't leave a partial rollout.
• F2 — a failed --add-reviewer being a non-fatal ::warning:: is a defensible call, but combined with F1 (won't self-heal) it means a reviewer-less, un-mergeable PR can ship green — the same class this PR exists to kill. Worth making the reviewer add fatal.

Great work on the token split and tests; holding on F5 + F4 (and please weigh the fine-grained PAT).

[LukasWodka]

All five addressed in 2c0b40b, and F4 turned out to be load-bearing in a way I didn't expect — chasing it exposed two defects in the test file itself.

F4 — the invariant is now asked of the token

You're exactly right that it rested on two literals agreeing with each other and with nothing else. author_login() asks GitHub who the credential is — as the PAT, not as the ambient App identity, or it would resolve the App's login and compare the wrong pair — and check_author_identity() refuses when that equals SYNC_REVIEWER. Case-insensitively, since GitHub logins are. An unresolvable token is its own refusal, not a guess.

I extracted the gate into a function rather than leaving it inline in main(), because inline it was unreachable from the selftest — and the mutation harness said so out loud: with the old literal check retired, "SYNC_REVIEWER becomes the account that authors the PRs" came back UNCAUGHT.

That also changes what the right mutation is. Flipping the SYNC_REVIEWER literal is now survivable — the runtime gate refuses the run — so the mutation that matters is dropping the comparison, not changing one of its operands. Re-aimed accordingly.

F3 — the guard moved ahead of the writes

This was the one with teeth. The check ran inside _ensure_pr, i.e. afterremediate() had created the branch and pushed CLAUDE.md. A missing token therefore produced a fleet-wide half rollout: branches pushed to every drifted repo, PRs opened nowhere. Now validated in main() before the first repo is touched; the in-function check stays as belt-and-braces and says that's what it is.

F1 + F2 — together they were the silent shape

Role assignment runs on both paths now, so a PR whose --add-reviewer failed once gets repaired on the next run instead of sitting un-mergeable for ever. And the reviewer half is fatal — branch protection needs one, so a warning meant an un-mergeable PR shipping on a green run. The assignee half stays cosmetic on purpose: it blocks no merge, and failing the run over it would be the opposite error.

F5 — header reconciled

Including the paragraph arguing "a second fleet-wide credential is the thing backend#2036 exists to stop." That argument was right about the risk and is overtaken by a harder constraint — no App token can author a PR Bugbot will look at — so I reconciled it rather than deleting it.

The security point

You're right and I corrected the rationale in place: "needs repo scope; contents: write alone cannot open a PR" conflates two claims. The true one is that the App's contents:write cannot open a PR Bugbot will review — an authorship problem, not a scope one. A fine-grained PAT with pull_requests: write + contents: read opens PRs perfectly well and is dramatically tighter.

The re-provisioning itself is a decision, not a code change, so I recorded it rather than asserting it done. It needs a new token minted and the secret swapped — worth doing before this is relied on fleet-wide.

Two defects in my own test edits, both the local class

1. The selftest could not go red below line 486.failed = [...] was computed a hundred lines above the last checks, so anything appended below it printed its FAIL and did not count — the summary read 0 failed while two checks had failed, and the script exited 0. The mutation harness surfaced it: a gate mutation came back UNCAUGHT because the suite measuring it was structurally incapable of failing there. A test file that cannot go red is a guard that cannot fire, one level up.

2. Stubs scripted (0, "[]", "") for "no open PR". The real call carries --jq '.[0].number // empty', so no-PR is an empty string and "[]" is truthy. Now that the existing-PR path also makes two edit calls, a "[]" stub consumes the same number of entries down the opposite branch — passing while testing the case it is named against. Three corrected.

Verification

• selftest 47 checks, 0 failed (was 37; the literal check retired, 11 added)
• mutation harness 6 mutations, 0 stale, 0 malformed, 0 uncaught
• py_compile, ruff and actionlint clean

[LukasWodka]

bugbot run

[saqlainsyed007]

Re-review of 2c0b40be — every code finding is addressed, and well. Verified each against the new source:

• F4 (runtime identity) —author_login(token) now asks GitHub who the PAT is (gh api user --jq .login, None when unresolvable), and check_author_identity(token) compares it to SYNC_REVIEWER. The invariant is derived on both sides now, not literal-vs-literal, and the mutation harness catches "SYNC_REVIEWER becomes the PR author" (it was UNCAUGHT before). Exactly the fix.
• F3 (guard before writes) — the identity gate is extracted and called before remediate() touches any repo, ordered emptiness → resolution → identity, each with its own refusal. No more branch/commit fleet-wide then refuse-to-open.
• F5 (stale header) — the TOKEN header is rewritten to "TOKENS, PLURAL … the split is the point," correctly describing App(Contents:RW, branch push) + human PAT(PR author with a Cursor seat). No longer claims the App satisfies PR-creation.
• F1 (existing-PR reviewer) — the existing-PR path now returns _assign_roles(...) instead of early-returning, so a PR whose reviewer add failed on an earlier run gets repaired. F2 is addressed as a bonus: a failed --add-reviewer now returns an error string (fatal) rather than a non-fatal warning.

All my threads are resolved and verified. The only thing left is CI (pending) — once it's green I'll approve.

One non-code note (won't hold the approval): the least-privilege point stands at the provisioning layer — SYNC_PR_AUTHOR_TOKEN should be a fine-grained PAT scoped to pull_requests:write + contents:read on the target repos, not a classic repo-scoped one. The code accepts whichever, so this is a secret-setup choice; worth doing so the fleet-wide human credential is as narrow as the job it does.

[LukasWodka]

bugbot run

[saqlainsyed007]

Re-review of fa9c0d1a — the Bugbot follow-up is a sound fix and doesn't regress anything I flagged.

The AuthorUnusable addition is exactly right: check_author_identity proves the token exists, resolves, and isn't the reviewer, but none of that proves it can open a PR (wrong fine-grained permission, or never SSO-authorized). Raising on the first failed pr create — rather than returning per-repo — aborts the fleet loop so at most one repo is left branch-without-PR instead of all of them, and the docstring is honest that "the only proof is opening one," so the guarantee is bounded rather than claimed. That's consistent with the guard-before-writes reasoning from the earlier round.

All my findings (F1–F5) remain addressed, and the Bugbot thread is resolved. Only CI is left (pending) — I'll approve once it's green.

[LukasWodka]

bugbot run

[LukasWodka]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

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

[LukasWodka]

Green on 1962556. Both Bugbot rounds since your last re-review are addressed and every thread is resolved:

• Existing bot PRs skip human authorship (Medium) — real and live: tracebloc/client#848 is an open sync PR authored by app/tracebloc-release-train. The existing-PR path repaired its roles and returned None, so the first --create-prs run after this merged would have reported it ensured while leaving it exactly as unreviewable as before. _ensure_pr now reads the author (--json number,author, is_bot asked of GitHub rather than pattern-matched off the login) and refuses per-repo, naming the remedy.
• Token refusal skips the fleet audit (Medium) — the moved gate was written as die(), which is the same abort standards-sync.yml argues against twenty lines above the secret. It disarms remediation now instead: fleet still audited and reported, nothing pushed, exit 2.

All 14 checks green (Cursor Bugbot is SUCCESS, not merely neutral), 0 unresolved threads, MERGEABLE. The only thing left is your CHANGES_REQUESTED from the first round — re-requested above.

make check green; standards-sync-selftest.py 60 checks / 0 failed; standards-sync-mutations.py 11 mutations, 0 stale, 0 malformed, 0 uncaught.

One operational consequence worth flagging before this merges: there are open bot-authored sync PRs on the fleet today (client#848 confirmed). Once this lands, the next --create-prs run reports those repos as failed writes until someone closes those PRs — deliberately, since a PR's author cannot be reassigned and the next run reopens them as the PAT's owner. That is a one-time migration, not a regression, but it should not be a surprise on the first red run.

[saqlainsyed007]

Approving 1962556a — CI green, mergeable, no open threads, and I re-verified the net change through the Bugbot rounds: all my original findings still hold (author_login + check_author_identity gate the PAT before any write, AuthorUnusable aborts the fleet on a failed create, and the TOKENS-PLURAL header correctly describes the App-vs-PAT split), and the three follow-up commits are all on-topic identity/refusal hardening from Bugbot's own review — exactly what this PR set out to enable: a refused PAT now disarms the writes rather than half-rolling-out, and an open bot-authored sync PR is refused rather than treated as ensured.

Nice piece of work — this closes the loop that had all 14 sync PRs skipping Bugbot. (Non-code, for provisioning: keep SYNC_PR_AUTHOR_TOKEN a fine-grained PAT scoped to pull_requests:write + contents:read.)

[LukasWodka]

⚠️Duplicate alert:#351 ("fix: sync PRs are opened as the PAT owner, so Bugbot can review them", fix/2594-sync-pr-pat-author) was opened 40 minutes ago by a parallel session on my account and fixes the same defect as this PR, touching the same three files. The two will conflict.

This PR is ahead on every axis — 7 commits vs 1, two human re-reviews and three Bugbot rounds all resolved vs none, 14/14 green vs bugbot / review FAILURE, and a mutation harness the other has no counterpart for. #351 also carries an open High for a silent App-token fallback on an empty PAT, which is precisely the case this PR turns into a hard pre-write refusal (and pins with the an empty PAT silently falls back to the App token mutation).

I have not closed #351 — that is a human call. Recommendation is to close it in favour of this one. Full comparison posted on #351.