Add Bonk break-glass command

[ndisidore]

Allow authorized collaborators to skip Bonk by commenting bonk break glass.

[ask-bonk]

Review: Add Bonk break-glass command

I focused on the actual bypass mechanics, since this workflow gates a required merge check and the job now holds checks: write. A note up front: the current implementation is materially different from what my earlier comment on this PR assumed — the break-glass job doesn't just add a label and hope a re-run turns the check green, it directly mints a bonk check-run with conclusion=success via the Checks API. That's the real bypass, and it changes which concerns actually matter. Re-reviewing on that basis:

1. Blocking — the synthesized check-run must match the required status check context, or this bypass does nothing

The break-glass job creates a check-run named bonk (line 70). But the normal bonk job's status context is produced by the Actions runner and is typically reported as Bonk PR Review / bonk (workflow name + job id), not bare bonk. GitHub branch protection required checks match on the context string. If the required check is registered as bonk (a check-run created directly by the action, e.g. ask-bonk posts its own bonk check-run) this works; if the required context is the job status Bonk PR Review / bonk, then POSTing a check-run named bonk creates an unrelated green check and the required job check stays red/absent — merge stays blocked. Please confirm the exact required-context string in branch protection and make the -f name= value match it verbatim. This is the load-bearing line of the whole feature and it's currently an unstated assumption.

2. Blocking — cancelling the in-flight bonk run can leave the required check permanently pending

The job cancels non-completed runs (lines 52–55) and then synthesizes a success check. But if branch protection requires the job-level status context, a cancelled run reports that context as cancelled/failing, and your separately-created bonk check-run does not overwrite it (different check-run id/context). Net result can be worse than doing nothing: you've killed the in-flight review and not satisfied the required context. This is the same root issue as #1 — the fix is to stop relying on a name collision and instead target the exact required context. Until #1 is pinned down, the cancel step is risky.

3. Blocking — bonk break glass bypasses review of this very file

Because issue_comment and non-fork pull_request events run the workflow definition from the default branch, this feature only activates after merge to main. Fine. But once merged, any OWNER/MEMBER/COLLABORATOR can comment bonk break glass on a PR that further weakens or removes these guards and force the required check green without Bonk ever reading the diff. Given this is the kernel's merge gate, please document the threat model in the PR body: the control reduces to "trust every collaborator with the allowlisted association," and the escape hatch can be used to merge changes to the escape hatch itself. That may be acceptable, but it should be an explicit decision, not implicit.

4. The label path and the check-run path are redundant and can disagree

Two independent mechanisms now suppress Bonk: (a) the bonk-break-glass label makes the bonk job no-op on the next pull_request event, and (b) the directly-POSTed success check-run. On the current head SHA, (b) is what unblocks merge immediately; (a) only matters on a subsequent synchronize. But a new push creates a fresh head SHA, so the label-driven no-op job produces a skipped/neutral bonk job — which, if that's the required context, is not success and re-blocks merge. So after break-glass, the next push may silently re-block. Please decide on one authoritative mechanism and document the push-after-break-glass behavior; right now the two paths interact in a way that's easy to get wrong.

5. unlabeled re-arm produces a failing/pending check, not a clean slate

Subscribing to unlabeled (line 5) re-runs bonk when the label is removed — good for reversibility. But removing the label does not delete the synthetic success check-run you POSTed earlier for that SHA. If the re-run's real conclusion is failing, you now have two bonk-ish checks (the stale synthetic success + the new real result) on the same SHA, and which one branch protection honors again comes down to context/id semantics from #1. Reversibility isn't clean.

6. sender.type != 'Bot' on the label-driven bonk job is doing subtle double duty

Line 82–83 now runs the bonk job for Bot senders if the label is present. That's how the labeled no-op re-runs after a bot push, but it also means a bot-authored synchronize on a break-glassed PR will spin a runner just to echo the skip message. Minor, but worth a comment since it inverts the original bot-exclusion intent.

Non-blocking / confirmations

• Fork guard (lines 34–37) correctly refuses break-glass on forks — good, since checks: write on a fork head would be a real escalation.
• Exact-string comment match (== 'bonk break glass', line 15) still fails silently on whitespace/case, and the job posts no acknowledgement or error comment. With a Checks-API bypass this is more important, not less: a collaborator can't tell whether the bypass fired. Recommend trim()+lowercase normalization and a confirmation comment/reaction.
• bonk.yml also fires on this comment; it no-ops (no /bonk/@ask-bonk mention) but spins a runner. Acceptable.
• --force on gh label create (line 42) is idempotent — fine.

Bottom line: the security posture (fork guard + author_association allowlist + minimal per-job permissions) is sound and not a regression. The blocking items are all correctness, and they converge on one question: what exact status-check context does branch protection require, and does the synthesized bonk check-run satisfy it? Please answer that in the PR (ideally with a one-line comment in the workflow), and reconcile the label-vs-check-run duality in #4 before merge.

[ask-bonk]

Review posted to PR #150.

Summary

I re-reviewed the workflow change in .github/workflows/bonk-pr.yml on the basis of its actual mechanism, which differs from what the earlier ask-bonk review assumed. The prior review treated break-glass as "add a label and let a re-run turn the check green." In reality the break-glass job holds checks: write and directly POSTs a bonk check-run with conclusion=success — that direct minting is the real bypass, so the prior blocking items (#1/#2 about re-run timing) are largely moot, while a different, sharper set of concerns applies.

Blocking items I raised (all correctness, converging on one question):

1. Required-context match — The synthesized check-run is named bare bonk (line 70). Branch protection matches on the exact status-check context string, which for a job status is usually Bonk PR Review / bonk. If the required context isn't literally bonk, the synthetic check is unrelated and merge stays blocked. This is the load-bearing line and is an unstated assumption.
2. Cancel can strand the required check — Cancelling the in-flight run (lines 52–55) reports the job context as cancelled; the separately-created check-run won't overwrite it if contexts differ. Can be worse than a no-op.
3. Bypasses review of this file itself — Once merged to the default branch, any allowlisted collaborator can break-glass a PR that weakens the guard. The threat model ("trust every collaborator") should be documented explicitly.
4. Label path vs. check-run path redundancy — The two suppression mechanisms interact; a push after break-glass creates a new SHA whose labeled no-op job may report skipped/neutral (not success) and silently re-block.

Plus advisories on unlabeled reversibility leaving stale synthetic checks, the inverted bot-exclusion, brittle exact-string match with no acknowledgement, and confirmations on the fork guard and bonk.yml interaction.

The security posture itself (fork guard + author_association allowlist + minimal permissions) is sound and not a regression.

github run

[ndisidore]

@ask-bonk

1. Invalid. The active ruleset requires exactly bonk from GitHub Actions app ID 15368. The normal job produces that exact name/app, and the synthetic check will too.
2. Invalid. Cancellation is followed by polling, then the matching bonk check is created. The canceled result cannot arrive afterward and supersede it.
3. Valid policy concern. Break-glass authority includes changes to this workflow itself. That is inherent to the feature.
4. Invalid. After a push, the bonk job runs but skips only the Bonk steps; the job concludes successfully. The direct check handles the current SHA, while the label handles future SHAs.
5. Invalid. Removing the label creates a newer real bonk check with the same app/name, which governs just like a normal workflow rerun.
6. Partly valid. Bot-triggered synchronization must produce a successful bypass check while the label remains. The review incorrectly references a labeled rerun; that trigger was removed.

[ask-bonk]

Re: break-glass review points

Author's responses checked against the actual workflow (.github/workflows/bonk-pr.yml). Verdicts below, with the load-bearing fact stated once up front:

Key fact: The normal review posts its result as a check-run via the ask-bonk/ask-bonk/github action (line 113) — a check-run literally named bonk from the GitHub Actions app — not a job-level status context Bonk PR Review / bonk. The break-glass job POSTs a check-run with -f name=bonk from the same app (lines 77–84). Same app, same name → same required-check identity. That single fact resolves most of the earlier concerns.

1. Invalid — agreed. The required context is the bonk check-run (app 15368), and both the normal action and the synthetic POST produce that exact name/app. No Bonk PR Review / bonk job-context mismatch exists. The earlier review's central assumption was wrong.

2. Invalid — agreed. The sequence is cancel (60–63) → poll until completed (65–75) → then POST the success check (77–84). Because the success check is only minted after every in-flight run has reached completed, a late cancelled conclusion cannot arrive afterward and supersede it. The "can strand the required check" concern doesn't hold given the ordering.

3. Valid — acknowledged. Break-glass authority does include weakening this very file, and that's inherent to a collaborator-trust escape hatch. Worth an explicit line in the PR body documenting the threat model ("trust every allowlisted collaborator, including for changes to the hatch itself"), but it's a policy decision, not a bug.

4. Invalid — agreed. On a synchronize with the label present, the bonk job runs and only skips the Bonk steps (lines 103–105 run; 107–113 are gated off) — the job still concludes success. So there's no skipped/neutral re-block. The direct check covers the current SHA; the label covers future SHAs. No redundancy conflict.

5. Invalid — agreed. Removing the label fires unlabeled, re-running bonk for real, which posts a newer bonk check (same app/name). Branch protection honors the latest check-run for that context, exactly like any normal rerun — the stale synthetic success doesn't win.

6. Partly valid — agreed. A bot-triggered synchronize on a labeled PR must still yield a successful bypass check while the label remains, and line 96 / lines 87–91 handle that. The earlier review's framing referenced a labeled rerun trigger that isn't present (the workflow subscribes to unlabeled, not labeled); the mechanism that actually matters is the label-gated no-op success on synchronize, which is correct.

Bottom line: items 1, 2, 4, 5 are invalid — they stem from assuming a job-level status context rather than the bonk check-run the action actually emits, and from misreading the cancel→poll→create ordering. Item 3 is a legitimate policy note to document. Item 6 is correct in spirit but the trigger it cites was removed.

[ask-bonk]

Posted to PR #150.

I verified each point against the actual workflow file. The findings:

The load-bearing fact: The ask-bonk/ask-bonk/github action (line 113) emits a check-run named bonk from the GitHub Actions app — not a job-level status context Bonk PR Review / bonk. The break-glass job POSTs -f name=bonk (lines 77–84) from the same app. Same app, same name, same required-check identity.

Verdicts:

1. Invalid — required context is the bonk check-run; both paths produce it. No context mismatch.
2. Invalid — code does cancel → poll-until-completed (65–75) → then POST success. A late cancelled cannot supersede.
3. Valid policy concern — break-glass can weaken this file; inherent to the feature, worth documenting.
4. Invalid — labeled synchronize runs the bonk job which only skips the Bonk steps (107–113) but still concludes success.
5. Invalid — label removal produces a newer real bonk check that governs like a normal rerun.
6. Partly valid — the bypass-success-on-synchronize requirement holds; the earlier review referenced a labeled trigger that isn't present (workflow uses unlabeled).

Your responses are consistent with the code.

github run

[ask-bonk]

Re-review: Add Bonk break-glass command

Thanks for the point-by-point responses. I re-read the current workflow (both commits, including "Require write access for Bonk bypass") against your replies. I agree with your dispositions on the correctness items, and I want to record why they hold so the reasoning is captured for the next reader — several of them turn on non-obvious details of the current code.

Agreeing with your rebuttals

#1 required-context match / #2 cancel ordering — These both reduce to: the required context is a check-run named bonk from the GitHub Actions app (id 15368), not the job status context Bonk PR Review / bonk. Given that, the earlier "job status gets stranded as cancelled" worry doesn't apply, because branch protection never keys on the job context. And the ordering in the script is correct for this: it cancels in-flight runs (lines 60-63), waits for them to actually reach completed (the polling loop, lines 65-75), and only then POSTs the synthetic bonk check-run (lines 77-84). So a late-arriving cancelled result can't supersede the success. Both invalid, agreed — conditional on that app-id/context assumption, which is the single load-bearing fact of the whole feature.

#4 label vs check-run duality — Agreed, and the reason is easy to miss: on a later synchronize the bonk job still runs, but the "Break glass active" step (lines 103-105) executes while Checkout/Run Bonk are skipped. Because at least one step runs and succeeds, the job concludes success — not skipped/neutral. That echo step is doing real work: it's what keeps a fully-suppressed run from concluding neutral and re-blocking. Worth keeping in mind that if someone later "cleans up" that no-op step, the push-after-break-glass path silently regresses.

#5 unlabeled re-arm / #6 bot sync — Both consistent with the code. unlabeled (line 5) re-runs Bonk for real on the current SHA and the newest same-named check governs; and lines 90-91 deliberately let a bot synchronize run the labeled no-op so the bypass check is re-minted while the label stays. The reviewer's reference to a removed labeled-rerun trigger was stale — your correction is right.

The one thing I'd still ask for: write it down

#3 is accepted as inherent, and that's exactly why the assumptions behind #1/#2/#4 need to be documented in the file itself. Every non-blocking conclusion above rests on facts that live outside the diff:

• the required status check is the check-run bonk from app id 15368 (not the job context);
• the "Break glass active" echo step exists specifically to force a success (not neutral) conclusion on the suppressed path;
• break-glass authority intentionally extends to changes to this workflow itself.

None of these are visible to a future editor reading bonk-pr.yml. Since this is the kernel's merge gate and the escape hatch can be used on a PR that edits the escape hatch, please add a short comment block at the top of the break-glass/bonk jobs capturing those three invariants. That converts "correct because the author happens to know the ruleset" into "correct and self-documenting," and it's the cheapest possible insurance against a well-meaning refactor quietly breaking the gate.

Remaining advisory (non-blocking)

The exact-string comment match (== 'bonk break glass', line 15) still fails silently on whitespace/case, and the job posts no acknowledgement or error. With checks: write + direct check minting, a collaborator has no signal whether the bypass fired. A trim()/lowercase normalization plus a confirmation reaction or comment would make this debuggable. Not a blocker.

Bottom line: no security regression, and the correctness objections are resolved by the current mechanics. My only ask before merge is documenting the three load-bearing invariants (#1/#4/#3) in the workflow, since the whole feature's correctness rests on facts that aren't in the diff.

[ask-bonk]

Review posted to PR #150.

Summary

I re-reviewed the current state of .github/workflows/bonk-pr.yml against the author's point-by-point rebuttals. I agree with the author's dispositions and posted a review documenting why each holds against the actual code:

• Bump vitest from 3.2.4 to 4.1.0 #1/Bump vitest from 3.2.4 to 3.2.6 #2 (invalid) — Both reduce to the required context being a check-run named bonk from the Actions app (id 15368), not the job status context. The script's cancel → poll-to-completion → POST-success ordering (lines 60-84) is correct for that model.
• Sync GitLab main to GitHub #4 (invalid) — The "Break glass active" echo step (lines 103-105) is load-bearing: because one step runs and succeeds while Bonk steps skip, the job concludes success, not neutral, so a later push doesn't re-block.
• Trim suggested model list #5/Seed default Workers AI models for new users on deploy-wizard instances #6 — Consistent with unlabeled re-run + bot-sync handling in lines 5 and 90-91.
• Bump vite from 7.3.2 to 7.3.5 #3 (valid/inherent) — Break-glass authority extending to this file is accepted.

My one remaining ask (non-blocking): document the three load-bearing invariants inline in the workflow — the required-context/app-id assumption, the purpose of the no-op echo step, and the self-modification threat model — because the feature's correctness rests entirely on facts that don't appear in the diff and would silently break under a well-meaning refactor. Plus an advisory to normalize the comment match and add an acknowledgement.

No security regression identified.

github run

[ask-bonk]

remote: Permission to cloudflare/cloudflare-os.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/cloudflare/cloudflare-os.git/': The requested URL returned error: 403

github run

[ask-bonk]

@ndisidore Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.