diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0b9e7622cc..861d6c5347 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,10 +23,27 @@ on: concurrency: # Deliberate one-off runs (workflow_dispatch, weekly schedule) get a per-run group so a # push to the same ref cannot cancel them mid-flight — release-browser-matrix takes up to - # 70 minutes and was repeatedly killed by main churn. Push/PR runs keep the shared ref - # group with cancel-in-progress so superseded heads still stop early. + # 70 minutes and was repeatedly killed by main churn. PR runs keep the shared ref group + # with cancel-in-progress so superseded heads still stop early. group: ${{ github.workflow }}-${{ (github.event_name == 'workflow_dispatch' || github.event_name == 'schedule') && github.run_id || github.ref }} - cancel-in-progress: true + # Base-branch pushes are EXEMPT from supersession. `on.push.branches` is [main, release/**], + # so `event_name == 'push'` is always a base-branch push — a commit that is already merged and + # can never be superseded by a "newer head" the way a PR branch can. Cancelling those runs + # does not save redundant work; it destroys the only verification `main` ever gets. + # + # Measured 2026-08-18 over the last 30 pushes to main: 23 cancelled (77%), 6 success, 1 failure. + # main was therefore unverified four times out of five, and three separate defects reached a + # feature branch as first detection rather than being caught on the branch that introduced them: + # - the Lighthouse baseline drifted ~163ms on desktop /therapy-compass with nobody alerted, + # so the next PR to complete a Lighthouse run inherited it as its own "regression"; + # - two @mockup assertions in ui-tools-search-mode-mockup.spec.ts broke on main and stayed + # broken (later repaired by #2115) after surfacing on an unrelated PR; + # - ci-triage could not baseline a single PR failure, reporting "Compared with main CI run + # #12038/#12062/#12087 (cancelled)" three times in one afternoon. + # + # The cost of this exemption is bounded: concurrent main runs, one per merge, each already + # scoped by the `changes` job. That is strictly cheaper than a defect landing unnoticed. + cancel-in-progress: ${{ github.event_name != 'push' }} permissions: contents: read diff --git a/AGENTS.md b/AGENTS.md index 81a7952bc5..e9d76e06dc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -649,7 +649,7 @@ Durable mitigations in this repo: merely behind and the merge tree is clean, let that run settle and sync once, late, after review/fix work is assembled. Preempt an in-flight run only when the branch is genuinely blocking-conflicted or the user explicitly asks for - an immediate sync; do not disable `cancel-in-progress`. + an immediate sync; do not disable `cancel-in-progress` for PR branches. - The historical review table is frozen during normal PR work. Write a new review with `ledger:append`, which creates an immutable record; never resolve a review conflict by editing the historical table. The repository deliberately leaves its @@ -748,8 +748,8 @@ Before opening a new branch, check whether the task can ride an **already-open P still own** or be bundled with **other currently-queued low-risk work** instead of minting a new one. If the target PR's CI is already running, either wait for it to settle before pushing the addition or assemble every commit before that PR's first push — -`.github/workflows/ci.yml` sets `cancel-in-progress: true`, so a push mid-run cancels and -restarts CI rather than saving an invocation, reproducing the exact cancellation waste +`.github/workflows/ci.yml` cancels in-progress runs for pull requests, so a push mid-run +cancels and restarts CI rather than saving an invocation, reproducing the exact cancellation waste this rule exists to cut (reproduced 2026-07-30 pushing a second commit to PR #1406: the in-flight `static-pr` run was cancelled, failing `pr-required` on the now-stale head). A settle-then-push addition also lands after this repo's one automatic Codex review may @@ -845,8 +845,13 @@ named PR). Future process only. to settle before pushing again. Apply the same settle-first rule to branch syncs: for a behind-but-clean PR with required CI in flight, wait, then perform at most one late `update-branch` / `git merge origin/main` after review and fix work is assembled. - `cancel-in-progress: true` should remain enabled, but every superseding push or sync cancels - Production UI mid-flight (~40% of recent PR CI runs were cancellations). + Cancel-in-progress should remain enabled **for pull requests**, where a newer head genuinely + supersedes the work in flight, but every superseding push or sync cancels Production UI + mid-flight (~40% of recent PR CI runs were cancellations). It is deliberately NOT enabled for + base-branch pushes: a merged commit cannot be superseded, so cancelling there destroyed the + only verification `main` received (23 of the last 30 main pushes cancelled, measured + 2026-08-18). Do not "simplify" that exemption back to a blanket `true` — it is pinned by + `tests/ci-cache-safety.test.ts`. - Before push: `npm run format` **and commit the result**, then `npm run verify:pr-local` (or the smallest gate that covers the change). Format is in `static-pr` but not in `verify:cheap`; an uncommitted format leaves CI red on the pushed diff --git a/docs/branch-review-records/b1814fea1580f18252cda21f16744ba5373c7933d894c80cba5e425c53430b23.record.md b/docs/branch-review-records/b1814fea1580f18252cda21f16744ba5373c7933d894c80cba5e425c53430b23.record.md new file mode 100644 index 0000000000..9c76c906a4 --- /dev/null +++ b/docs/branch-review-records/b1814fea1580f18252cda21f16744ba5373c7933d894c80cba5e425c53430b23.record.md @@ -0,0 +1 @@ +| 2026-08-18 | claude/ci-main-verification-blindspot | 63fb976bf9862160a04b9b784cbdec79e4f3f11f | CI concurrency: exempt base-branch pushes from cancel-in-progress | shipped | test:ci-workflows 326 passed; ci-cache-safety 50 passed; check:github-actions passed; check:ci-scope passed; prettier clean | diff --git a/tests/ci-cache-safety.test.ts b/tests/ci-cache-safety.test.ts index 1cefe98aad..622ba574b3 100644 --- a/tests/ci-cache-safety.test.ts +++ b/tests/ci-cache-safety.test.ts @@ -203,6 +203,37 @@ describe("CI cache safety", () => { expect(releaseHeader).toContain("needs.changes.outputs.lockfile_changed == 'true'"); expect(releaseHeader).toContain("startsWith(github.ref, 'refs/heads/release/')"); }); + + /* + * Base-branch pushes must never be cancelled by a later merge. + * + * `cancel-in-progress: true` is correct for a PR branch, where a newer head genuinely + * supersedes the work in flight. It is wrong for `main`: that commit is already merged and + * nothing supersedes it, so cancelling does not skip redundant work — it throws away the only + * verification `main` receives. Measured 2026-08-18 across the last 30 pushes to main, 23 were + * cancelled (77%) and only 6 completed, which is how a ~163ms Lighthouse drift on + * desktop /therapy-compass and two broken @mockup assertions both reached feature branches as + * first detection, and why ci-triage kept reporting a cancelled main run as its baseline. + * + * A blanket `true` here reads as a harmless cost control and is not one, so it is pinned with + * its own case rather than left to review. + */ + it("never cancels an in-flight run for a base-branch push", () => { + const concurrency = sourceSegment(workflow, "concurrency:", "permissions:", { + label: "workflow concurrency block", + }); + + expect(concurrency).toContain("cancel-in-progress: ${{ github.event_name != 'push' }}"); + expect(concurrency).not.toContain("cancel-in-progress: true"); + + // `on.push.branches` is what makes `event_name == 'push'` mean "base branch" — if a push + // trigger is ever widened to feature branches, this exemption silently stops being scoped + // and every branch keeps its superseded runs alive. + const pushTrigger = sourceSegment(workflow, " push:", " pull_request:", { + label: "workflow push trigger", + }); + expect(pushTrigger).toContain('branches: [main, "release/**"]'); + }); }); /*