Uh oh!
There was an error while loading. Please reload this page.
fix: sync PRs are opened as the PAT owner, so Bugbot can review them - #351
fix: sync PRs are opened as the PAT owner, so Bugbot can review them#351LukasWodka wants to merge 2 commits into
Conversation
… them Every open org-standards sync PR is unmergeable, fleet-wide, and has been since bugbot-gate became a required context. MEASURED 2026-08-26 on .github#344 and client#848: Bugbot review gate: FAIL No Bugbot check run on head 4bc5e08. 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. Cursor Bugbot reviews only HUMAN-authored PRs. `standards-sync.yml` opens its PRs with the App token, so no review ever arrives, and the gate -- correctly -- will not read an absence as approval. Posting `bugbot run` does not help either: tried on #344, nine minutes, no check appeared. So the two mechanisms were mutually unsatisfiable and the sync had quietly stopped being able to land anywhere. THIS IS NOT A REVERSAL OF backend#2036. That decision was about the AUDIT: an org-scoped installation token can read the whole fleet where a repo-scoped PAT cannot, and a silent PAT fallback there would hide a broken App path. Both still hold -- the fleet reads and the pushes are unchanged and still the App's. Only `pr create` moves, because AUTHORSHIP and FLEET READS are different questions and only the first decides whether a review can ever arrive. `promote-repo.sh:1223` already splits them exactly this way, and has since promotions started getting reviewed. THE FALLBACK IS LOUD. If the PAT is absent or cannot see the repo -- what happened to design-system-v2, whose entry was missing from the fine-grained token's repository list -- the sync still opens via the App so something lands, and warns that the PR cannot pass the gate and names the fix. A silent fallback would produce a PR that looks fine and can never merge, which is precisely the state being repaired. EVIDENCE. Selftest 33 checks, 0 failed. Two new cases, and both mutation-proved rather than merely passing: revert create to the app token -> REDDENS silence the fallback warning -> REDDENS The first is the one that matters: the change is invisible in the PR's title, body and diff, so a test that did not assert WHICH helper opens it would pass against the bug. `actionlint` clean; both files parse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka
commented
Aug 26, 2026
Flagging an overlap before this goes further, because two PRs are currently solving the same problem in this repo and whichever merges second will conflict hard. This duplicates #348Both make the sync open its PRs as a human so Bugbot will review them. They are independent designs, not one built on the other:
Different secret names is the part that makes this more than untidy — whichever lands second needs the workflow secret renamed to match, so they cannot both be half-right. The ticket reference looks wrongThe title says The Bugbot finding here is already handled in #348
#348 refuses an empty PAT rather than falling back, and does it before the first write rather than at PR-creation time, so a missing token cannot push a branch to every drifted repo and open a PR on none. It also carries SuggestionClose this in favour of #348 and keep backend#2590 as the ticket — #348 is further along, has the reviewer's re-review, and already answers this PR's own Bugbot. If there is something in the Not closing it myself — it isn't mine to close, and the call is @LukasWodka's. |
LukasWodka
commented
Aug 26, 2026
Duplicate of #348 — please read before reviewing@aptracebloc: hold off on this one. This PR and #348 are two independent fixes for the same defect, opened by two parallel sessions on my account. They touch the same three files and will conflict. #348 is strictly ahead, measured just now:
More importantly, the High on this PR is a bug #348 exists to prevent. Bugbot flags that an empty #348 also covers two things this PR does not, both found by Bugbot on it:
I have deliberately not fixed the High here and not closed this PR — closing someone's PR is a human call, and this one was opened by a different session than the one writing this. My recommendation is to close this in favour of #348. If instead you prefer this smaller diff, say so on #348 and I will withdraw that one instead — but the High and the two gaps above would need porting over first. Cross-posted to #348. |
LukasWodka
commented
Aug 26, 2026
Follow-up: #348 merged as |
LukasWodka
commented
Aug 26, 2026
Superseded by #348 — recommend closing rather than merging
Three things make this a close rather than a conflict-resolve:
The title's I did write and mutation-prove a fix for the Bugbot High below (the absent-PAT fallback being silent) before finding #348. I am not pushing it: on the surviving design there is nothing to make loud, because there is no fallback. Discarded deliberately rather than merged. Not closing this myself — closing a PR is a human call. @LukasWodka, this one wants closing. 🤖 Generated with Claude Code |
Bugbot High on #351, and it is this change reproducing the state it exists to end. `gh_as_pr_author` falls back to the caller token when PR_AUTHOR_TOKEN is unset, so the create SUCCEEDS as the App and `code == 0`. The loud branch never fired: it requires `code != 0` AND the token present, and the absent case has neither. So a missing PAT opened an app-authored PR -- one that cannot pass bugbot-gate and will never merge -- with no annotation anywhere naming the fix. The workflow `env:` comment already PROMISED otherwise: "absent, the script falls back to the App and says so loudly rather than opening a PR that can never merge". That was a claim in prose standing where the check belonged. Now it is true. `::error::` rather than `::warning::` for this one, because the blast radius differs: a missing PAT makes EVERY PR in the sweep unmergeable, where a PAT that cannot see one repo costs one. Still non-fatal, so the sweep lands something rather than nothing -- the same trade the existing branch makes. Both paths now state the consequence through ONE string, `_APP_AUTHORED_CANNOT_MERGE`, so they cannot drift into saying different things about the same dead end. Two selftests, both mutation-proven: restoring the silent path reddens them, and so does dropping the shared sentence. The absent-PAT test leaves `gh_as_pr_author` REAL rather than stubbed, so the fallback it exercises is the real one. Verified: selftest 35 checks 0 failed, make lint clean, make selftests 28 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 26, 2026
bugbot run |
LukasWodka
commented
Aug 26, 2026
Update after #348 merged ( 1. The premise is false — no bugbot context is required anywhereThis PR's rationale, in both the workflow comment and the
Measured across 16 repos' default branches just now — not one has any bugbot context in
So app-authored sync PRs were never blocked from merging by a Bugbot gate. The evidence is that all 19 sync PRs landed, including client#848 which merged today with The check does fail on app-authored PRs — that part is real, and #348 states it accurately (" 2. The work is already on develop#348 landed the whole mechanism: The conflict here isn't incidental. Both PRs rewrite the same three files to do the same thing, so 3. The secret it names doesn't exist
Recommendation, unchangedClose in favour of #348. There is nothing here to port: the mechanism is merged, the premise is wrong, and the secret is absent. The one thing worth carrying forward is the measurement above, which I've recorded rather than lost. Still not closing it myself — @LukasWodka's call. |
LukasWodka
commented
Aug 26, 2026
|
There was a problem hiding this comment.
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 1b2f040. Configure here.
| "🤖 Generated with [Claude Code](https://claude.com/claude-code)\n" | ||
| ) | ||
| code, out, err = gh("pr", "create", "-R", full, "--base", base, "--head", head, | ||
| # OPENED AS THE PAT OWNER so Bugbot will review it, falling back to the caller's |
There was a problem hiding this comment.
Open bot PR skips author check
High Severity
_ensure_pr returns success whenever any open PR already tracks the head, without checking author. After an App fallback opens an unmergeable PR (one warning), later create-prs runs hit this early return, stay silent and green, and never re-open as the PAT owner.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1b2f040. Configure here.
…356) * fix(2284): the gate tolerates a review that never came, and says so Two absences were one verdict, and they mean opposite things. `PENDING` covered both "Bugbot is still running" and "Bugbot never showed up", and the timeout failed both identically. A check that STARTED and never finished is a review that BROKE -- worth blocking on. A check that never appeared is Bugbot dropping the PR, which this repo cannot fix, retry, or wait out. Measured 2026-08-26 on human-authored PRs, well past p50 164s / max 635s: six of nine never got a check at all -- .github#349 (57 min), #350 (55), #352 (40), #353 (37), #354 (32), e2e-test-agent#273 (2h+) -- while #351, opened BETWEEN two of them, was reviewed in three minutes. Not latency, not the seat limit, not the author. backend#2114 closed COMPLETED saying "no discriminator survives the data"; the drop is still live. `bugbot run` cannot recover it: Cursor refuses on a seat limit and the App will not be given one (decision, 2026-08-26). So requiring this context while failing UNCLAIMED would block roughly two thirds of PRs for the full wait and then fail them with no remedy -- the gate would look broken while behaving exactly as written. WHAT THIS IS NOT: it is not a pass. UNCLAIMED exits 0 so the context can be required, and every other word says the head is UNREVIEWED -- its own banner, its own summary line, and an explicit "read the diff yourself". The honest report is "nothing looked at this", not "this is clean". PENDING still blocks, and that asymmetry is the whole change. Also corrected: the vocabulary comment said "Only PENDING is waitable", which this makes false. `main` now READS `WAITABLE` rather than restating it. 79 selftest assertions (4 new through `main`, since the exit code is the behaviour and `evaluate` alone cannot show it). Registry: 1 stale marker fixed, 5 mutations added -- the collapse in both directions, the tolerance leaking onto PENDING, the tolerance removed, and the banner reading as a pass. Part of tracebloc/backend#2284 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(2284): pin the UNREVIEWED banner, which no assertion covered The mutation `the UNREVIEWED banner reads as a pass` came back UNCAUGHT. Every other assertion in this suite checks a verdict or an exit code, and the banner is neither -- it is the one line a human skimming the summary actually reads, and the only thing standing between "exits 0" and "looks clean". Three cases: the UNCLAIMED headline says UNREVIEWED, does not contain the word pass, and a genuine pass still does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style: hoist the selftest imports ruff flagged E402/E401 from the banner test added in the previous commit. `make lint` is where I should have run it before pushing, not after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(2284): the threshold is applied before the head is classified Bugbot on #356, High: `evaluate` returned UNCLAIMED as soon as the head had no Bugbot check, without applying the open-finding threshold to the threads it had ALREADY loaded one block earlier. So the tolerance this PR adds for a dropped review would launder a finding that had already come -- review head A, get a High, push head B, Bugbot never claims B, and the gate reports UNREVIEWED-but-not-blocked over an open High at exit 0. The same hole was in PENDING, and fixing only the branch Bugbot named would have left its twin. So `blocking` is now computed before `bugbot_check` is consulted at all, and an absence of either kind with a blocking finding is a FAIL that names the finding and does NOT wait -- the answer would not change. Both absence verdicts now also state that the threshold was checked, so the report says which question was answered. The tolerance itself is intact and asserted: a Low on an unreviewed head is still UNCLAIMED, exit 0, and so is a resolved High -- otherwise the remedy the FAIL message names would not clear it. Not a shipped bug: `required_conversation_resolution` is true on every train branch, so the merge was blocked anyway. It was a gate naming the wrong reason, which is the failure mode this file exists to prevent. One renderer (`_finding_lines`) feeds both paths, because two would drift and one of them would quietly stop saying OPEN (rule 9). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Every open org-standards sync PR is unmergeable, fleet-wide, and has been since
bugbot-gatebecame a required context.Measured
On
.github#344andclient#848:Cursor Bugbot reviews only human-authored PRs.
standards-sync.ymlopens its PRs with the App token, so no review ever arrives — and the gate, correctly, will not read an absence as approval. Postingbugbot rundoes not help either: tried on #344, nine minutes, no check appeared.So the two mechanisms were mutually unsatisfiable, and the sync had quietly stopped being able to land in any repo.
This is not a reversal of backend#2036
That decision was about the audit: an org-scoped installation token can read the whole fleet where a repo-scoped PAT cannot, and a silent PAT fallback there would hide a broken App path. Both still hold — the fleet reads and the pushes are unchanged and still the App's.
Only
pr createmoves, because authorship and fleet reads are different questions, and only the first decides whether a review can ever arrive.promote-repo.sh:1223already splits them exactly this way, and has since promotions started getting reviewed.The fallback is loud
If the PAT is absent or cannot see the repo — what happened to
design-system-v2, missing from the fine-grained token's repository list — the sync still opens via the App so something lands, and warns that the PR cannot pass the gate, naming the fix. A silent fallback would produce a PR that looks fine and can never merge, which is the state being repaired.Evidence
Selftest: 33 checks, 0 failed. Two new cases, both mutation-proved rather than merely passing:
createto the app tokenThe first is the one that matters: this change is invisible in the PR's title, body and diff, so a test that did not assert which helper opens it would pass against the bug.
actionlintclean; both files parse.After this merges
The 16 open sync PRs still need one recut/re-run each to be re-opened under the PAT — closing and re-opening is what changes authorship, since
gh pr createrefuses a duplicate on an existing head. Worth doing as one sweep rather than piecemeal.Type
fix
Checklist
developNote
Medium Risk
Touches release automation credentials and PR creation paths; behavior change is intentional but misconfigured secrets or PAT repo scope could still yield unmergeable sync PRs until re-run.
Overview
Org-standards remediation PRs were stuck behind
bugbot-gatebecause they were opened with the GitHub App token; Bugbot only reviews human-authored PRs, so checks never appeared and the gate timed out after 900s fleet-wide.This PR changes only
pr createauthorship: the workflow passesPR_AUTHOR_TOKEN(RELEASE_TRAIN_PR_TOKEN) into the audit step, andstandards-sync.pyintroducesgh_as_pr_authorso sync PRs are created under the PAT owner while fleet reads and branch pushes still use the App installation token (unchanged from backend#2036).When the PAT is missing or cannot access a repo, the script still falls back to App-authored creates so a sweep can land something, but it now emits
::error::/::warning::messages namingbugbot-gateand the fix (set the secret or add the repo to the PAT list). A shared_APP_AUTHORED_CANNOT_MERGEstring keeps both fallback paths aligned.The selftest adds cases that assert which helper opens the PR and that silent app-authored creates are no longer allowed.
Reviewed by Cursor Bugbot for commit 1b2f040. Bugbot is set up for automated code reviews on this repo. Configure here.