ci: stop main merges evicting each other's CI runs, and stop triage citing a baseline that never ran the job - #2209
Conversation
… cases can run `inFlightCiGuard` probes for the `gh` binary and fails open before either injected fetcher is consulted. The message-formatting case injected `prViewer` and `runFetcher` but could not inject that probe, so it only ever exercised the code under test on a machine that happened to have `gh` installed: green in CI, red in any bare container, for no product reason. `ghAvailable` now joins the other two as an injectable, defaulting to the real probe. The formatting case and the script's own `--self-test` pass it, and a new case pins the fail-open branch itself — asserting that neither fetcher is consulted when `gh` is absent, which nothing covered before. No behaviour change: production callers pass no options and get `ghIsAvailable`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HoyoNN7nCUVDLaE84k6e6c
…p evicting each other
`cancel-in-progress: ${{ github.event_name != 'push' }}` was added to stop a later
merge superseding an in-flight main run, and it does that. It does not stop the
other way a run dies: GitHub keeps at most ONE pending run per concurrency group,
so when merges land faster than a run completes, each newly queued main run
cancels the one already waiting. No supersession is involved, so the exemption
never sees it, and main kept landing unverified anyway.
Observed 2026-08-20: `a1c2ced`, `d745d15`, `97f6142` and `1cc0d29` were all
cancelled while a ~70-minute release-browser-matrix held the shared
`CI-refs/heads/main` group. A mobile-`/` CLS regression rode through that gap and
surfaced hours later on an unrelated PR (#2199), where it read as that PR's
failure. It is the same pattern the existing comment already documents from
2026-08-18 (23 of 30 main pushes cancelled) — the earlier fix simply treated the
wrong mechanism.
Pushes now key concurrency on `github.run_id`, exactly as workflow_dispatch and
schedule already do, so a merged commit is never queued behind or evicted by
another. `cancel-in-progress` stays: it is the line that states the intent, and it
remains the only protection if the group key ever changes.
Cost is the one the existing comment already accepts: concurrent main runs, one
per merge, each still scoped by the `changes` job.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HoyoNN7nCUVDLaE84k6e6c…a baseline CI is path-scoped, so a docs-only push to main reports `success` with Lighthouse, Production UI and Build all skipped. The triage comment nevertheless printed "Compared with main CI run #N (success)" and labelled the failure "needs investigation", which reads as "main is green for this job" when main had never measured it. That is how the mobile-`/` CLS regression was waved through on PR #2199 — the bot cited run #12662, a docs-only merge that skipped Lighthouse entirely — and it is the trap ledger #5DYBQQ already recorded from the other side. The baseline's jobs are now read whatever its conclusion, not only when it failed. A failing job the baseline did not execute is classified `unbaselined` and says so in its own words, and the comparison sentence names the jobs that run did not exercise. `classifyFailedJobs` takes the executed-name list as an optional fourth argument: omitted means "could not establish", which keeps the previous classification rather than inventing a verdict — so the `catch` path in the workflow degrades exactly as before. Cost is one extra paginated job listing per triage comment, on a workflow that only runs when a PR's CI has already failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HoyoNN7nCUVDLaE84k6e6c
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:5 minutes Limit details: You’ve used the included review currently available. Your 87 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:cf2865879c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 21, 2026
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch claude/pr-review-comments-yegd3q at starting commit cf28658; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:claude/pr-review-comments-yegd3q, then verify that the pull request head contains the published commit before reporting success. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
`executedJobNames` excluded `skipped` by denylist, so every other conclusion counted as evidence — including `cancelled`. A cancelled job completed no verification, so a PR's Build failure would still be reported as `needs-investigation` while citing a main run that never finished that check: the same wrong claim the previous commit set out to remove, arrived at from a different conclusion value. It is also the likeliest one here, since a cancelled main run is this repo's common failure mode rather than a rarity. Inverted to an allowlist — `success`, `failure`, `neutral` — because the two mistakes are not symmetric. Calling a job that ran "unbaselined" only says the comparison is silent there, which costs a reader nothing; calling a job that did NOT run "baselined" implies main covered the failure. An allowlist therefore fails in the harmless direction for `timed_out`, `stale`, `action_required` and any conclusion GitHub adds later, instead of silently treating each new state as evidence. Self-test covers cancelled, timed_out, stale, action_required, an unrecognised future conclusion, and the end-to-end case that a cancelled baseline job yields `unbaselined`. Raised by Codex review on this PR (P2). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HoyoNN7nCUVDLaE84k6e6c
Uh oh!
There was an error while loading. Please reload this page.
Resolves a conflict in the guard-push pair, where PR #2209 landed the same fix independently: `ghAvailable` injected into `inFlightCiGuard` so the fail-open `gh --version` probe stops short-circuiting the injected fetchers. Took main's version of both files wholesale rather than hand-merging two spellings of one change. It is a strict superset of the local one: it also injects the probe into `selfTest`'s `mockBlockedGuard`, and adds a fails-open-when-gh-is-unavailable case whose fetchers throw if consulted. Nothing local was lost in substance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014uzqrokJW55EN9vbZhFyXz
Summary
Three independent CI-signal defects found while reviewing PRs #2199 and #2201. Each is its own commit, and each is separately revertible.
mainmerges were evicting each other's CI runs (5f9b4a5).cancel-in-progress: ${{ github.event_name != 'push' }}stops a later merge superseding an in-flight main run, and it does that. It does not stop the other way a run dies: GitHub keeps at most one pending run per concurrency group, so when merges land faster than a run completes, each newly queued main run cancels the one already waiting. No supersession is involved, so the exemption never sees it. Base-branch pushes now key concurrency ongithub.run_id, exactly asworkflow_dispatchandschedulealready do.cancel-in-progressstays as the statement of intent and as the fallback if the group key ever changes.The CI triage bot cited a baseline that never ran the failing job (
0916c9b). CI is path-scoped, so a docs-only push tomainreportssuccesswith Lighthouse, Production UI and Build all skipped. The comment still printedCompared with main CI run #N (success)and labelled the failure "needs investigation", which reads as "main is green for this job" when main had never measured it. The baseline's jobs are now read whatever its conclusion; a failing job the baseline did not execute is classifiedunbaselinedand the comparison sentence names it.classifyFailedJobstakes the executed-name list as an optional fourth argument — omitted means "could not establish", so the workflow'scatchpath degrades exactly as before.tests/guard-push.test.tscould not run its own subject withoutghon PATH (2e535bd).inFlightCiGuardprobes for theghbinary and fails open before either injected fetcher is consulted, so the message-formatting case was green in CI and red in any bare container, for no product reason.ghAvailablenow joinsprViewer/runFetcheras an injectable defaulting to the real probe, and a new case pins the fail-open branch itself — asserting neither fetcher is consulted whenghis absent, which nothing covered before.These are not hypothetical. The concurrency hole let a mobile-
/CLS regression land onmainunmeasured on 2026-08-20; it then surfaced on unrelated PR #2199 as that PR's failure, and the triage bot pointed at run #12662 — a docs-only merge that skipped Lighthouse entirely — as the green baseline. Two of the three defects combined to misattribute one regression.Verification
npm run verify:pr-localFocused evidence on the changed contracts:
npx vitest run tests/guard-push.test.ts— 42/42 passed in a container with noghinstalled, which is the environment that previously failed.npx vitest run tests/ci-cache-safety.test.ts— 53/53 passed, including the new assertion that base-branch pushes key concurrency ongithub.run_id.node scripts/guard-push.mjs --self-test— passed; the same injection was applied there, since the script's own self-test had the identical coupling.node scripts/ci-triage.mjs --self-test— passed, extended with the skipped-baseline case (unbaselinedclassification, and the "did not exercise" sentence).npm run check:github-actions— pin check passed.npm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changedUI verification not run: no UI, routing, or styling surface is touched. The diff is two workflow files, two scripts and two test files.
npm run verify:releasebefore release or handoff confidence claimsNot run: release confidence was not requested.
Risk and rollout
main: previously a burst of merges produced one run plus one survivor, now it produces one run per merge. That is the cost the existing in-file comment already accepts ("concurrent main runs, one per merge, each already scoped by thechangesjob"), but it is a real increase in runner usage during merge bursts and should be watched. The triage change adds one paginated job listing per triage comment, on a workflow that only fires when a PR's CI has already failed. The guard-push change is inert in production — callers pass no options and get the real probe.5f9b4a5(concurrency),0916c9b(triage),2e535bd(guard-push). Reverting the concurrency commit restores the sharedCI-refs/heads/maingroup and the eviction behaviour; nothing else depends on it.Notes
AGENTS.md"PR bundling" excluding operational-risk items from bundling. Two reasons, both worth a reviewer's judgement: this session is constrained to a single designated branch, andscripts/pr-policy.mjsonly warns about bundling when operational risk mixes with clinical or UI changes, which this does not. Every item is its own commit so pre-merge revertibility is intact; say the word and I will split them.cancel-in-progressfalse for pushes, pending-eviction is the only remaining way GitHub cancels a run in a group. The observed timeline fits exactly — the four cancelled runs bracket the 16:24→17:27 window during which run32391849411's release-browser-matrix held the group.document-viewerLinux visual baseline needs re-adopting from thevisual-baselineartifact after PR Make document search on demand #2199 lands.Generated by Claude Code