ci: gate the advisory UI lane, drop dead outputs, and record what the rest measured - #1453
Conversation
#137 — the advisory lane covers `@quarantine` + `@mockup`. With an empty flake ledger and no `@quarantine` tag anywhere in the suite, it was spending ~3m14 on every UI pull request to run five mockup tests. It now also requires `advisory_ui_changed`: a mockup surface changed, or the flake ledger is non-empty. Reading the ledger rather than hard-coding "no quarantines exist" is the point — the moment a test is quarantined the lane comes back on every UI PR without anyone remembering to re-enable it. An unreadable ledger fails OPEN, because a lane that silently never runs is the failure mode worth guarding against. Five self-test cases cover mockup source, mockup component, a populated ledger, the unreadable-ledger path, and the negative case. #139 — `source_changed`, `workflow_changed` and `changed_files` were job outputs consumed by nothing, which reads as live wiring and invites a future job to be gated on a value no one maintains. They are no longer exported. The script still computes all three; `docs_only` derives from the first two. The separate coverage-breadth half of #139 is untouched and stays open.
Three of the five outstanding CI items resolved to "already done" or "measured and rejected" rather than to code, and the measurements are worth more than the changes would have been. #135 (ui_changed over-fires on src/app/api) — already implemented on main, and now validated. A naive grep suggested a coverage hole: 16 of 17 API route groups appear in Playwright specs. But 92 of those references are `page.route(` / `context.route(` stubs — the journeys mock the API layer, so an API handler change cannot move them. The exclusion is correct. Recorded with the stop rule, because the grep that suggested otherwise is the obvious thing to run again. #136 (cold Next build cache) — built, benchmarked, rejected. `<distDir>/cache` symlinked outside the run root keeps the runner's always-delete cleanup contract intact, and that mechanism is proven. But cold compile 97s vs warm 63s is 34s saved for a 804 MB cache, and a per-SHA key writes a fresh multi-hundred-MB entry per commit against a 10 GB budget with LRU eviction — it would evict the browser caches that save more. The symlink trick is reusable if the cache ever shrinks; the arithmetic is what fails. Shard imbalance — measured rather than guessed. `ui-phone-scroll.spec.ts` is 65% of shard 1 (267s of 409s) at 4.77s/test, the worst rate of any large spec in it, and it is also the file behind both #127 and #141. Splitting it is the only lever that rebalances, since `--shard` cannot divide one file. #127 — no recurrence across four post-#1427 runs, including the one whose shard 1 failed on a different test. That is the evidence its own next step asked for.
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:17 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 (4)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
BigSimmo
commented
Jul 30, 2026
@cursoragent resolve merge conflicts |
Merge conflict on PR #1453 is resolved and pushed. Conflict: only
PR: #1453   |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:920616e881
ℹ️ 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.
Codex found a real hole in #137's gate, and it is wider than the single file it named. `mockupPatterns` matched `-mockups.tsx` only, so eight singular `-mockup.tsx` components missed it, and three of the five specs the advisory project runs carry `@mockup` without "mockup" in the filename. The failure mode is silent and total. Editing `src/components/tools-page-mockups/task-directory-mockup.tsx` on its own gave `ui_changed=true, advisory_ui_changed=false`: the production projects `grepInvert` its `@mockup` tag, the advisory lane never starts, and `ui-tools-task-directory.spec.ts` — that component's only browser coverage — runs nowhere, on a green pull request. Matched now by mockup component directory as well as singular/plural filename, plus the three name-less advisory specs. A second list is how this happened, so the second list is now checked against the first. `assertMockupSpecParity` reads `mockupSpecPattern` out of playwright.config.ts, expands the alternation, and asserts every advisory spec matches `mockupPatterns`. It fails CLOSED on a lost anchor rather than quietly becoming a guard that checks nothing. Both directions proven by mutation: dropping the new spec pattern reports `tests/ui-tools.spec.ts` unrun, and renaming the constant in playwright.config.ts reports the missing anchor. Six self-test cases cover the singular filename, the mockup directory, both name-less specs, and the negative case that the directory rule does not swallow ordinary component paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JHLPEV4o1rzipPDqshCSHY
Uh oh!
There was an error while loading. Please reload this page.
Resolves the docs/outstanding-issues.md conflict with PR #1453. That file deliberately carries no merge driver (#133), so overlapping appends conflict loudly rather than being silently concatenated. Resolved as the issues skill requires: rebuilt the file from origin/main and re-applied only the two rows this branch owns (the #86 in-place update and the new #145), so none of #1453's rows were dropped. Verified #140-#144 all still present and #144's cell content byte-identical to main. Re-checked that #145 was still free on main before reusing the id — main's next-id marker was untouched at 145, so there was no id collision to reallocate around. docs/branch-review-ledger.md auto-resolved through its merge=ledger driver. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGEBHp4Seoh1jK1vGTNtYS
Summary
Works the five outstanding CI items from the
#1427review. Two became code; three resolved to measurements that say the change should not be made, which is a more useful result than the change would have been.#137— gate the advisory UI lane. It covers@quarantine+@mockup. With an empty flake ledger and zero@quarantinetags in the suite, it was spending ~3m14 on every UI PR to run five mockup tests. It now also requiresadvisory_ui_changed: a mockup surface changed, or the flake ledger is non-empty. Reading the ledger rather than hard-coding "no quarantines exist" makes it self-correcting — quarantining a test brings the lane straight back with nobody remembering to re-enable it. Fails open on an unreadable ledger, because a lane that silently never runs is the failure mode worth guarding.#139— stop exporting outputs nothing reads.source_changed,workflow_changed,changed_fileswere job outputs with zero consumers, which reads as live wiring and invites a future job to gate on a value nobody maintains. The script still computes all three (docs_onlyderives from two of them); they are simply no longer exported.#139's separate coverage-breadth half is untouched and stays open.#135,#136, and the shard imbalance — documented with the measurements, no behaviour change.RAG impact: no retrieval behaviour change — nothing under
src/lib/rag/**, clinical-search, retrieval-selection, ranking-config, answer-ranking, the eval harness, or the golden fixture is touched.Correction: the first cut of
#137's gate had a real holeCodex found it, and it was wider than the single file it named.
mockupPatternsmatched-mockups.tsxonly, so eight singular-mockup.tsxcomponents missed it, and three of the five specs the advisory project actually runs —ui-tools,ui-tools-collapse,ui-tools-task-directory— carry@mockupwith no "mockup" in the filename and missed it too.The failure mode was silent and total. Editing
src/components/tools-page-mockups/task-directory-mockup.tsxon its own gaveui_changed=true, advisory_ui_changed=false: the production projectsgrepInvertits@mockuptag, the advisory lane never starts, and that component's only browser coverage runs nowhere — on a green PR. So the gate I added to save 3m14 could have cost real coverage.Fixed in
6b9b79b: matched by mockup component directory as well as singular/plural filename, plus the three name-less specs.The root cause is that
mockupPatternswas a second hand-maintained copy of a listplaywright.config.tsalready owns, so the second list is now checked against the first.assertMockupSpecParityreadsmockupSpecPatternout of the config, expands the alternation, and asserts every advisory spec matchesmockupPatterns. It fails closed on a lost anchor rather than degrading into a guard that checks nothing.The three that measured out
#135(ui_changedover-fires onsrc/app/api/**) — already implemented, and now validated.isUiChangedPathalready excludes it. I nearly reported a coverage hole: 16 of 17 API route groups appear in the Playwright specs. But 92 of those references arepage.route(/context.route(stubs — the journeys mock the API layer, so an API handler change genuinely cannot move them. The exclusion is correct. Recorded with a stop rule, since the grep that suggested otherwise is the obvious thing to run again.#136(cold Next build cache) — built, benchmarked, rejected. The mechanism works and is proven: symlinking/cacheto a directory outside the run root leaves the runner's always-delete cleanup contract completely untouched, becausermSyncremoves the symlink and not the target. The arithmetic is what fails:34s for 804 MB does not pay. Worse, my first cut keyed on
github.sha, which writes a fresh multi-hundred-MB entry per commit against a 10 GB repo-wide budget with LRU eviction — it would evict the Playwright browser caches that save considerably more. Reverted in full; the symlink trick is recorded as reusable if the cache ever shrinks.Shard imbalance — measured. Running
--shard=1/3locally and summing per-test durations (121 passed, 6.9 min):ui-phone-scroll.spec.tsui-chrome-scroll.spec.tsui-accessibility.spec.tsui-phone-scroll.spec.tsis 65% of shard 1 (267s of 409s), at the worst per-test rate of any large spec in it, and it is also the file behind both#127and#141. Splitting it is the only lever that rebalances, since--shardcannot divide a single file.#127— no recurrence across four post-#1427runs (30530618838,30532582652,30534158395,30535651470), including the one whose shard 1 failed on a different test. That is the evidence its own next step asked for; recorded, held open only for a wider sample onmain.Not done
#138(CI Triage inert) needs theCI_TRIAGE_ENABLEDrepository variable set. That is a GitHub settings action, not a code change — it can only be done by a maintainer.Verification
npm run verify:cheapat head6b9b79b—Test Files 436 passed (436),Tests 4574 passed | 4 skipped (4578), exit 0. (An earlier run before this branch was rebased onto main read435 (435)/4569 passed; the delta is main's newtests/rag-round-trip-budget.test.ts, not this diff.)npm run check:ci-scope— self-test passes, now includingMockup spec parity: 5 advisory specs all match mockupPatternsand eleven advisory-lane casestests/ui-tools.spec.tsunrun (exit 1); renamingmockupSpecPatterninplaywright.config.tsreports the missing anchor (exit 1)npm run check:github-actions,check:outstanding-issues,check:branch-review-ledgernpx vitest run tests/ci-cache-safety.test.ts tests/container-ci-contract.test.ts— 18 passednpx prettier --check .—All matched files use Prettier code style!#136benchmarked end-to-end with real builds before being reverted; symlink/cleanup interaction proven directlyUI verification not run: no UI source changes. This PR gates the advisory lane rather than the required
Production UIjob, so the shards are unaffected. Confirmed on this PR's own runs thatAdvisory UIskips — before and after the widened patterns, sincescripts/ci-change-scope.mjsis a workflow path and not a mockup surface.Not run, deliberately:
verify:release,eval:*,check:supabase-projectare provider-backed and not authorised here.Risk and rollout
continue-on-errorand outsidepr-required, so gating it cannot make a PR red; the realistic downside is it not running when it should — which is exactly what Codex caught, and what the parity guard now blocks. Dropping three unread outputs cannot change any job's behaviour — nothing consumed them.git reverteither of the first two commits independently. The first is the only behavioural one; the third is its correction and should be reverted with it.Notes
The two rejected items are the point of the exercise rather than a shortfall:
#135would have been a silent coverage hole if the grep had been taken at face value, and#136would have traded 34s for a cache-budget eviction cascade. Both stop rules are in the ledger so the next pass starts from the measurement.The Codex finding belongs in the same column. A cost-saving CI gate whose miss is invisible is worse than the cost it saves, and the fix worth keeping is not the widened regex — it is that the duplicated list is now checked against its source.
Generated by Claude Code