Distinguish a cancelled PR-required run from a real failure - #1409
Conversation
`cancel-in-progress` supersedes an in-flight run on every push, and the aggregate's require_* helpers lumped the resulting `cancelled` in with `failure`. The red was therefore indistinguishable from a genuine break at a glance. Measured cost: one 2026-07-30 session spent four separate investigations on `::error::changes result was cancelled` / `static-pr result was cancelled`, and PR #1401 merged straight through an unrelated red the repo had learned to ignore — which is the worse half, because a signal that is red for reasons unrelated to the diff trains everyone to click through the one time it is real. The aggregate now wires RUN_CANCELLED: ${{ cancelled() }}, reports a workflow-level cancellation once through a shared cancelled_error helper, and labels a cancelled job result as cancelled rather than as a plain failure. The message is actionable rather than merely accurate: it names the supersession, points at the newest run for the current head, and tells a reader who finds no newer run that the run was hand-cancelled, verified nothing, and must be re-run rather than merged past. The obvious fix was rejected as unsafe. Treating cancelled as neutral, or skipping the aggregate with `if: !cancelled()`, makes the red disappear — but GitHub counts a SKIPPED required check as PASSING, so a hand-cancelled run on the current head would become mergeable with nothing verified. That is also what #95's own stop rule forbids. So the result stays RED; only the diagnosis cost is removed, and `if: always()` is now pinned by a test with the reason recorded. Seven cases in tests/ci-cache-safety.test.ts EXECUTE the extracted aggregate script under synthetic job results rather than grepping the YAML, because the defect was behavioural and a structural assertion passed against it (#94). They cover the green baseline, workflow-level cancellation, a single cancelled job, a genuine failure keeping its plain message with no cancellation excuse, and the invariant that no cancelled required job can ever exit 0. Mutation-proven: three fail against the pre-fix aggregate, and one asserts the extraction is non-empty so a YAML restructure cannot make the group vacuously green. Verified: verify:cheap — Test Files 432 passed (432), Tests 4477 passed | 4 skipped (4481); check:github-actions, check:ci-scope, check:ci-triage, prettier all pass. Ledger #95 records the fix and the rejected approach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (3)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
My previous commit wired the workflow-level cancelled status function through an
`env:` value. That is invalid: GitHub allows the status-check functions
(success/failure/cancelled/always) only in `if:` conditions. The whole workflow
file therefore failed to parse — the run was named `.github/workflows/ci.yml`
instead of `CI`, created ZERO jobs, and reported a bare failure with no logs to
read.
Nothing local caught it, and that is the part worth fixing. It is valid YAML and
an invalid Actions schema, so prettier, lint, typecheck, check:github-actions and
all 432 unit files passed the broken version. It was visible only on hosted CI —
exactly the gap I flagged as unverifiable when the first commit went up.
The env expression was never needed. Reading each job's own `cancelled` result
does the same work, because a supersession cancels the upstream jobs, so the
first require_* call reports it. So the fix is a deletion, not a workaround.
A new case in tests/ci-cache-safety.test.ts scans every file under
.github/workflows/** and fails on a status-check function outside an `if:`. It
caught a second instance immediately: the comment I had written to warn about the
first mistake contained the offending expression, and `${{ }}` is interpolated
inside `run:` blocks too, so that comment alone would have kept the file
unparseable. The comment now describes the trap without spelling it.
Verified: verify:cheap — Test Files 432 passed (432), Tests 4478 passed | 4
skipped (4482); check:github-actions passes; the guard is mutation-proven by
reintroducing the env expression and watching it fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
…cd8a' into claude/test-coverage-analysis-2vcd8a
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a15f0751b
ℹ️ 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".
|
@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. 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. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. 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". |
Codex found a real defect in the previous commit. The helpers exited on the first non-success, so a run that was cancelled AND broken — `safety` cancelled while `build` had already failed — exited at `safety`, announced "not a real failure" and "nothing here describes the diff", and hid the genuine build failure completely. That is worse than the ambiguity this change set out to remove: the old message was merely uninformative, the new one was actively wrong in the direction of ignoring a break. It also asserted that a newer push exists, which is false for a run cancelled by hand. Now two-pass. Every requirement is recorded into `failures` or `cancellations` before anything is reported, and genuine failures win: all of them are printed, and a concurrent cancellation is demoted to a `::warning::` context line that cannot read as an excuse. The cancelled-only headline states both possible causes — supersession or a hand cancellation — instead of asserting one, since a hand-cancelled run on the current head has no newer run to look at. Collecting before reporting also fixes an older annoyance for free: the aggregate now lists every failing job rather than stopping at the first. Two new cases cover the mixed run and the multiple-failure listing. Mutation-proven: four of the thirteen fail against the first-exit version. Verified: verify:cheap — Test Files 431 passed (431), Tests 4490 passed | 4 skipped (4494). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
Two things in one commit because the merge and the fix are on the same file and the PR's CI cancels in-progress runs on every push. Merge: main advanced 28 commits to b1f5718 and had again edited docs/outstanding-issues.md, so git merge-tree confirmed a real conflict rather than staleness. Resolved as before by taking main's ledger wholesale and re-applying this branch's five-row archive move; all five were verified still open on main first, so the move is still the correct change. 125 rows (56 open, 69 archived), marker next-id=128. Correction (Codex P2 on PR #1428, docs/outstanding-issues.md:224): the #95 archive record described the fix inaccurately, and the claim was verified against .github/workflows/ci.yml before being accepted: - It named "a shared cancelled_error helper". No such helper exists at any commit on this branch - grep finds nothing at be27fbb or at HEAD. The real implementation is a single record() collector (ci.yml:739) that reads each job's own result into failures/cancellations arrays, with require_success and require_skipped_or_success as thin wrappers passing skipped_ok false/true. - It claimed the error "names the newest run for the head". It does not; it points the reader at a newer PR required run on the current head SHA and says that if there is none the run was hand-cancelled and must be re-run. Both were carried over from an obsolete sentence in the open row, which described an earlier revision; the row's later text described the shipped two-pass behaviour, and condensing it for the archive kept the stale half and dropped the accurate one. An archive record that misdescribes its own fix defeats the purpose of archiving it. The rewritten record also fixes an inherited count: the row said "seven cases" in tests/ci-cache-safety.test.ts, but ten of that file's thirteen tests execute the extracted aggregate (lines 95-200); the first three cover caching and Playwright deps. It additionally records the failures-win refinement from PR #1409, which the original condensation omitted entirely. Verified: npm run verify:cheap -> EXIT=0; "Test Files 434 passed (434)"; "Tests 4562 passed | 4 skipped (4566)"; "Outstanding-issues guard passed: 125 rows (56 open, 69 archived), unique ids, next-id=128 above the highest". npx prettier --check . -> "All matched files use Prettier code style!" Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YdPS2KhKqz2buzsUgmX3c
3 participants
Summary
Closes ledger
#095.cancel-in-progresssupersedes an in-flight run on every push, and thepr-requiredaggregate'srequire_*helpers lumped the resultingcancelledin withfailure, so the red was indistinguishable from a genuine break at a glance.Measured cost, this session: four separate investigations of
::error::changes result was cancelled/::error::static-pr result was cancelledbefore recognising the pattern — and PR #1401 merged straight through an unrelated red the repo had learned to ignore. That second half is the worse one: a gate that goes red for reasons unrelated to the diff spends attention every time and trains everyone to click through the one time it's real.What the aggregate does now
Two-pass. Every requirement records into
failuresorcancellationsbefore anything is reported:::warning::also cancelled: …context line that cannot read as an excuse.CANCELLED with no failing job: …, which states both possible causes rather than asserting one: usually supersession (look for a newer run on the current head SHA), and if there is no newer run, it was cancelled by hand and must be re-run rather than merged past.The obvious fix is unsafe, and this deliberately doesn't do it
Treating
cancelledas neutral was my first instinct. It's wrong here:if: !cancelled()— the idiomatic way to make the red vanish — would make a hand-cancelled run on the current head mergeable with nothing verified.#095's own Stop rule forbids: "do not relaxrequire_successfor genuine failures while doing so."if: always()is pinned by a test with the reason recorded beside it, so the unsafe version can't be reapplied later as a cleanup.Two defects found in review, both real, both mine
This PR's history is worth reading, because both were the same mistake — verifying the state I had in mind and not the one I hadn't:
ci.ymlunparseable. It passed the workflow-level cancelled status function through anenv:value; GitHub allows those functions only inif:conditions. The run was named.github/workflows/ci.ymlinstead ofCI, created zero jobs, and reported a bare failure with no logs. Valid YAML and invalid Actions schema, so prettier, lint, typecheck,check:github-actionsand all 432 unit files passed the broken file — hosted CI was the only thing that caught it. Reading each job's owncancelledresult needs no such expression, so the fix was a deletion. A new guard now fails locally on any status-check function outside anif:across.github/workflows/**, and it immediately caught a second instance in the comment I'd written to warn about the first, because expressions are interpolated insiderun:blocks too.safetycancelled +buildfailed announced "not a real failure" and suppressed the build break entirely — worse than the ambiguity this PR set out to remove, because the old message was merely uninformative while that one was actively wrong in the direction of ignoring a break. It also asserted supersession as fact, which is false for a hand-cancelled run. Hence the two-pass structure above: it no longer depends on my having enumerated the states correctly.Verification
npm run verify:cheap—Test Files 431 passed (431),Tests 4490 passed | 4 skipped (4494).npm run check:github-actions—GitHub Actions pin check passed.npm run check:ci-scope—CI change scope self-test passed.npm run check:ci-triage—[ci-triage] self-test passednpx prettier --checkon all three files —All matched files use Prettier code style!Change scoperuns, which is the first jobci.ymlschedules; the two broken runs created zero jobs.set -euo pipefailchecked explicitly rather than assumed, since the rewrite introduced bash arrays:${#failures[@]}on an empty array is safe on bash5.2.21, whichubuntu-24.04ships, and the all-green case exits 0 with both arrays empty.safetyis required wheneverDOCS_ONLY=false, and I'd set itskipped). Caught by running it; fixed the fixture, not the assertion.npm run check:production-readinessnot run: no clinical workflow, privacy, environment, Supabase, source-governance or deployment change.The tests execute the aggregate rather than grepping the YAML
The cases extract the aggregate's
run:block and run it underbashwith synthetic job results. That's deliberate: both defects were behavioural, and a structural assertion would have passed against both — the exact complaint in#094(design-system gates assert structure, not rendered effect). Coverage: green baseline; supersession; a single cancelled job; a genuine failure keeping its plain message with no cancellation excuse; the mixed cancelled-and-failed run; every failure listed rather than the first; the invariant that no cancelled required job can exit 0; theif: always()pin; and the workflow-expression rule. One further case asserts the extraction is non-empty, so a YAML restructure can't make the group vacuously green.Risk and rollout
cancelledfailed before and fails now. The realistic failure mode is the test's script extraction going stale against a YAML restructure, which is why the non-empty guard exists.pr-requiredreturns to describing superseded runs as plain failures, one at a time.Clinical Governance Preflight
Not applicable to this diff. The changed paths are
.github/workflows/ci.yml,tests/ci-cache-safety.test.ts, anddocs/outstanding-issues.md. No ingestion, answer generation, search/ranking, source rendering, document access, privacy or clinical output behaviour changed — the aggregate only reads other jobs' results and reports. Note this does not weaken any required check: the set of states that pass is unchanged.Notes
RAG impact: no retrieval behaviour change — nothing under
src/lib/rag/**, clinical-search, retrieval-selection, released-search-order, ranking-config, answer-ranking, the eval harness, the golden fixture, or the retrieval RPCs is touched.This description was rewritten at head
1d415729; the original described the first attempt'sRUN_CANCELLEDwiring andcancelled_errorhelper, neither of which still exists. A stale body on a PR whose whole subject is "make CI signals honest" was worth correcting.Previously listed here as related-and-unfixed: the
ci/circleci: verifyred on docs-only PRs. Moot — CircleCI was removed from the repo in #1412.🤖 Generated with Claude Code
https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b