Narrow the browser gate to the specs a change can actually break - #2553
Conversation
…ly break `npm run verify:ui` is the most expensive run in this repository — 646 Chromium tests, ~25 minutes — and CI repeats it wholesale: `Production UI critical` and the three `Production UI` shards are guarded on `ui_changed`, so any change touching a browser surface gets the full suite on GitHub whether or not it ran locally first. Measured on two consecutive changes 2026-09-01/02: 25.0m and 20.5m locally, while the focused selection for the same diffs took 37s and 6.1s and reached the same verdict. `gate-receipts.mjs` removed local-versus-local duplication and `gate-arbiter.mjs` weighs local-versus-CI for the static gates, but neither can touch this one. The arbiter's lever is DEFERRAL, and `ui` is in `NEVER_DEFER_CLASSES` — correctly, because pushing a UI change with no browser evidence at all is not a bet this repository takes. So the lever here is a different one: run the part of the suite the diff can break. Narrowing is strictly safer than deferring; something always runs. `scripts/browser-test-plan.mjs` picks one of four levels — `none` (no browser surface changed, and CI skips Production UI for that scope too), `changed` (an edited spec, run complete rather than grepped), `focused` (changed UI source attributed to its owning specs), `full`. Attribution is evidence, not a hand-maintained table that rots: a spec owns a changed file when both contain the same literal — a `data-testid` the source renders and the spec asks for, or a route the source defines and the spec navigates to. Component names are deliberately not matched, because a spec never names a component, so a match there would be a comment. It fails closed, the opposite of the arbiter's fail-open contract: an unattributable UI file, a shared foundation, an unclassifiable browser-lane path, or a deleted file all escalate on their own. The arbiter's bug costs a redundant run; this one's would cost an unrun journey. `isBrowserLanePath` restates `uiPatterns` from `ci-change-scope.mjs` because that script answers for a CHANGE, not a FILE — without a per-file answer every doc in a mixed change looks unclassified and forces the full suite, which is nearly every real change. A restated rule drifts, so it is not left to inspection: the test cross-checks the predicate against the real classifier path by path, and a `ui_changed` this mirror does not recognise escalates rather than narrows. The arbiter now names the browser gates as narrowed rather than answering "nothing to weigh", which read as "no saving available" for the gate with the largest saving on offer. `.claude/hooks/testing-policy.sh` states the policy to cloud sessions at SessionStart, including the reporting rules that keep it honest — a narrowed run is reported as "focused browser proof, full suite left to CI", never as `verify:ui` passing. CI is untouched. GitHub runs exactly what it ran before, and no required check is weakened to save local time. Verification: `npm run test` 948 files, 12104 passed | 1 skipped; `npm run lint` and `npm run typecheck` exit 0; `check:gate-manifest`, `check:ci-scope`, `check:verification-plan`, `check:skills`, `check:knip`, `docs:check-links`, `docs:check-scripts` and `docs:check-inventory` all pass. `npm run plan:browser` on this commit's own files returns level `none` and CI agrees, so no browser run was needed — the tool's first use is its own change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkKHznd5E5KLnKbowRTLd8
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ffd3aeec-4ffc-4a60-880b-593e19acb695) |
![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: 767869879f
ℹ️ 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. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch claude/smart-test-selection-q4rt7m at starting commit 7678698; 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/smart-test-selection-q4rt7m, 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. |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #14836 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 767869879f
ℹ️ 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".
…nd honour both flag spellings Three Codex P2s on #2553, all confirmed against the code before acting. 1. Playwright project. `chromium` carries `grepInvert: /@mockup/` and `chromium-mockups` carries `grep: /@mockup/`, so `--project=chromium` on a mockup-only spec collects NOTHING and the run exits 0 having executed no test — the worst possible outcome for a tool whose job is to prove a narrowed selection is real. The two `testMatch` patterns are now read out of `playwright.config.ts` rather than copied (a copied regex drifts), and each selected spec is routed by them; `tests/ui-tools.spec.ts` matches both and gets both projects. A spec neither project collects escalates to the full suite instead of emitting a command that matches nothing. If the patterns cannot be extracted, both projects run — too many tests, never none. 2. CI preconditions. `deriveCiCoverage` returns guards it cannot evaluate from a worktree in `assumed` and still reports `covered: true`. Collapsing that to a boolean printed "CI runs the complete suite" on a draft PR, where `ui-critical-fast` is guarded on `github.event.pull_request.draft != true` and is skipped — the opposite of the truth, about the very job that makes narrowing safe. The assumptions are now printed with the verdict. 3. Selector flags. The documented syntax is `--files a.tsx,b.ts` and `--diff HEAD~1`, and only the `=` spelling was parsed. That did not error: it fell through to the `origin/main` diff and planned — or with `--run` EXECUTED — tests for a different change. Verified by reproduction: `--files docs/testing.md` reported 9 changed files before the fix and 1 after. Both spellings are now accepted, and a following flag is treated as a missing value rather than as the value. The first cut of fix 1 had a bug of its own, caught by its test: a greedy `/.*/` ran from the first slash in the config to the last, so every spec came back unroutable and every spec-only change escalated. The extraction now matches one literal. `tests/browser-test-plan.test.ts` joins `test:ci-workflows` because it now reads `.github/workflows/ci.yml`, which `tests/ci-cache-safety.test.ts` requires of any workflow-reading suite — that gate caught the omission. Verification: `npm run test` 948 files, 12114 passed | 1 skipped (the `check:function-grants: FAIL` lines in the log are that suite's own negative fixtures); `npm run lint` and `npm run typecheck` exit 0; `check:gate-manifest`, `docs:check-links` and `docs:check-inventory` pass. `npm run plan:browser` returns level `none` for this change and CI agrees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkKHznd5E5KLnKbowRTLd8
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3e6c0d4c-4162-49b9-a6d1-f5a4572111a8) |
main's #2553 (smart test selection) added tests/browser-test-plan.test.ts to test:ci-workflows on the same line this branch added tests/branch-review-index.test.ts, so package.json conflicted on that one script. Both suites load a committed workflow file and both are required there by ci-cache-safety.test.ts, so the resolution is the union, not a choice: main's list with branch-review-index re-inserted. Verified the union rather than assuming it: npx vitest run tests/ci-cache-safety.test.ts tests/branch-review-index.test.ts tests/browser-test-plan.test.ts -> Test Files 3 passed (3), Tests 134 passed (134). docs/branch-review-index.md, data/repo-awareness-snapshot.json and docs/scripts-index.md regenerated with their own generators. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AxPVZhAvzE2YupMiDz8sqF
PR #2553 landed and added scripts, so docs/scripts-index.md conflicted — a generated file, resolved by regenerating with npm run docs:update rather than by hand. package.json auto-merged cleanly; verified check:diff-integrity is still registered in verify:cheap:internal after that merge, since a silently dropped registration is exactly the kind of thing this branch exists to stop. Gates: docs:check-inventory, docs:check-scripts, docs:check-index, docs:check-links, check:gate-manifest, check:verification-plan, check:diff-integrity, check:repo-awareness-snapshot, check:outstanding-issues, check:pr-policy and check:ci-scope all pass; typecheck clean; unit suite 949 files / 12,158 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0193brbqFWHbpVnLfgDBkVNP
2 participants
Summary
scripts/browser-test-plan.mjs(npm run plan:browser): given a diff, it picks the smallest browser gate that still covers it.npm run verify:uiis the most expensive run in this repository — 646 Chromium tests, ~25 minutes — and CI repeats it wholesale, becauseProduction UI criticaland the threeProduction UIshards are guarded onui_changed. Measured on two consecutive changes 2026-09-01/02: 25.0m and 20.5m locally, while the focused selection for the same diffs took 37s and 6.1s and reached the same verdict.gate-receipts.mjsremoved local-versus-local duplication andgate-arbiter.mjsweighs local-versus-CI for the static gates, but neither can touch this one. The arbiter's lever is deferral, anduiis inNEVER_DEFER_CLASSES— correctly, because pushing a UI change with no browser evidence at all is not a bet this repository takes. The lever here is a different one: run the part of the suite the diff can break. Narrowing is strictly safer than deferring; something always runs locally.none(no browser surface changed — CI skips Production UI for that scope too),changed(an edited spec, run complete rather than grepped, since the diff rewrote its own assertions),focused(changed UI source attributed to its owning specs),full.data-testidthe source renders and the spec asks for, or a route the source defines and the spec navigates to. Component names are deliberately not matched: a spec never names a component, so a match there would be a comment, and a comment is not proof a journey covers the code.globals.css, the Playwright config, the runner, the shell/chrome coordinator set), an unclassifiable browser-lane path, or a deleted file all escalate to the full suite on their own. The arbiter's bug costs a redundant run; this one's would cost an unrun journey, so the defaults point the other way.isBrowserLanePathrestatesuiPatternsfromci-change-scope.mjs, because that script answers for a change, not a file — without a per-file answer, every doc in a mixed change looks unclassified and forces the full suite, which is nearly every real change. A restated rule drifts, so it is not left to inspection: the test cross-checks the predicate against the real classifier path by path (19 cases), and aui_changedthis mirror does not recognise escalates rather than narrows.gate-arbiter.mjsnow names the browser gates as narrowed rather than deferred and points at the planner. Asking it aboutverify:uipreviously returned "nothing to weigh", which reads as "no saving available" for the gate with the largest saving on offer..claude/hooks/testing-policy.shstates the policy to cloud sessions at SessionStart, including the reporting rules that keep it honest. Read-only, exits 0 on every path including a malformed payload, registered asbash "$CLAUDE_PROJECT_DIR/…"with an explicit timeout so the executable bit is never load-bearing.CI is untouched. GitHub runs exactly what it ran before, and no required check is weakened to save local time.
Verification
npm run test(full offline unit suite) —Test Files 948 passed (948),Tests 12104 passed | 1 skipped (12105), including the 44 new cases intests/browser-test-plan.test.ts.npm run lint— exit 0 ([gate-receipts] recorded a pass for "lint:internal" (6013 input files)).npm run typecheck— exit 0.check:gate-manifest,check:ci-scope,check:verification-plan,check:skills,check:knip,docs:check-links,docs:check-scripts,docs:check-inventory— all pass.npm run check:browser-test-plan— the planner's own offline self-test passes.Browser gate:
npm run plan:browseron this commit's own files returns levelnone, and the planner's derived reading ofci.ymlagrees that Production UI is skipped for this scope — so no browser run was needed. The tool's first use is its own change. Noui_changedpath is touched by this diff.npm run verify:pr-localnot run: its heavy scope is lint + typecheck + the full unit suite + the build-backed gates, each run directly on this tree and reported above with its own output. Provider-backed gates (verify:release,eval:*,check:production-readiness) were not authorized and no retrieval, ranking, generation, ingestion or environment behaviour changed here.Risk and rollout
--run; the arbiter change alters one message; the hook is read-only and cloud-only. The material risk is a planner that narrows when it should not, which is why every uncertain path escalates and why the mirrored CI rule is cross-checked against the real classifier rather than trusted.Notes
tests/browser-test-plan.test.tspins the escalations first and the narrowing second, and pins the reporting wording in the runner itself: a narrowed pass printsthis is focused browser proof, not the full UI gate.NEVER_DEFER_CLASSES, no new CI-side selection, and no automatic substitution of the planner forverify:uiin any existing script. A human or agent still chooses the gate; this only makes the smallest correct one visible, with its evidence.🤖 Generated with Claude Code
https://claude.ai/code/session_01NkKHznd5E5KLnKbowRTLd8
Generated by Claude Code
Note
Low Risk
Local advisory tooling and docs only; CI merge gates are untouched. Residual risk is incorrect narrowing, mitigated by fail-closed escalation and cross-checks against
ci-change-scope.mjs.Overview
Adds
npm run plan:browser(scripts/browser-test-plan.mjs) so local work can run a smaller Chromium slice than fullverify:ui(~25 min) while CI still runs the complete Production UI suite onui_changed. The planner picks one of four levels (none,changed,focused,full) from the diff, attributes UI files to specs via shared literals (data-testid, routes—not component names), routes specs to the right Playwright projects, and fails closed to the full suite on shared foundations, unattributable UI, drift vsci-change-scope, or unroutable specs. Dry-run by default;--runexecutes.gate-arbiter.mjsnow treatsverify:uiand related e2e gates as narrowed, not deferred, and points atplan:browserinstead of implying no local saving. Cloud SessionStart gets.claude/hooks/testing-policy.sh(read-only policy + honest reporting). Docs (verification-gates.md,AGENTS.md), Claude allowlists,check:browser-test-plan, andtests/browser-test-plan.test.ts(intest:ci-workflows) lock the contract. CI workflows are unchanged.Reviewed by Cursor Bugbot for commit 2626ff7. Configure here.