Repository navigation
test: remove the web-e2e flake at its cause, and fail any run that only passes on retry - #838
Merged
Merged
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
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 |
sakibsadmanshajib
force-pushed
the
test/e2e-flake-zero
branch
from
August 10, 2026 16:03
1fc632c to
e0c193c
Compare
Owner
Author
|
Note on measurement conditions, since pooler contention has been reddening this job on main and a starved pooler can masquerade as flakiness. These results are not contention artefacts. Three reasons.
What #837 will change is the base rate: a faster, less contended seeder makes the window narrower and the flake rarer. It does not close it, which is why the fix is to stop charging the hook to the product budget rather than to make the hook faster. Lab hygiene for this work: the |
…runs that only pass on retry Web E2E measured 7.7 percent flakiness in CI run 31361681115: 26 executed tests, 2 of which passed only on a retry. Both were in tests/e2e/profile-completion.spec.ts, identified from the run artifact's JSON rather than from names. Root cause, category test infrastructure, not a product race. Nine spec files copy-paste a beforeEach that reseeds the shared Supabase fixtures through a child process. Playwright charges beforeEach to the test's own timeout, and that reseed talks to a shared cloud project, so its wall time is set by conditions no spec controls. The same call took 2618 ms, 19226 ms, 19894 ms and 30248 ms across four attempts inside that one job. Every spec was therefore running against a random slice of its declared 30 second budget. The test that "passed on retry" had burned its entire budget inside the hook and never opened a browser page at all. A second defect sat underneath it. The reseed ran through execFileSync, which freezes the worker's event loop, and Playwright's timeout is an ordinary timer that cannot fire while the loop is blocked. As a result "setup saves profile" was reported as passed at 30194 ms under a 30000 ms timeout: a test that overran its deadline and went green anyway. The fix is one shared helper, tests/e2e/support/fixture-reset.ts. It runs the reseed as an awaited async child with its own explicit 120 second ceiling, so a wedged seeder dies with its own message instead of surfacing as a UI timeout, and it adds the hook's measured wall time back onto the deadline through testInfo.setTimeout. The product always gets exactly the budget the spec author declared, no more and no less. All nine specs now route through it, which removes 107 lines of duplication and leaves no sibling caller carrying the original defect. Retries are now visible. The list reporter folds a retry-pass into its "N passed" tail, so a run that needed two retries read identically to a clean one, and finding these two meant downloading a 5 MB artifact and unzipping the HTML report's embedded JSON by hand. flake-reporter writes a table of every retry-pass into the GitHub job summary and fails the run when there is one at all. The threshold is zero, argued in flake-report.ts: this check is being promoted to required, and a retry-pass is exactly the failure mode that promotion has to survive. Retries stay on, because they are what produce the second attempt and its trace; what changes is that a run which needed them no longer reports success. Nothing here raises retries, adds a blind wait, loosens an assertion or skips a spec. Measured, both arms on the same booted stack with the same seeder: setup saves profile before 0/10 after 10/10 profile settings stay reachable, CI latency before 0/10 after 9/10 The single remaining failure in each after arm was an upstream Supabase error (a schema cache retry, and a sign-in exceeding 25 seconds on a loaded box), now correctly attributed to the seeder and to sign-in instead of appearing as an unexplained UI timeout.
…locking the event loop Review follow-ups on the flake gate. The custom reporter was the only thing failing a run on a retry-pass, and `npx playwright test --reporter=list` replaces the configured reporters outright. Under that flag the reporter never ran at all and the run exited 0, so a single command line argument disabled the gate the reporter exists to enforce. Playwright's own `failOnFlakyTests` is now set alongside it and survives the override. The reporter stays, because the built-in flag gives an exit code and nothing to act on, while the reporter is what names the test and its attempt count in the job summary. live-auth.ts minted its session through execFileSync. That helper is the sanctioned way to sign a run in, and its `reauthenticate` is explicitly built for mid-test use to renew a session that issue #782 kills after about 55 minutes. Blocking the worker's event loop stops Playwright's timeout timer from firing, so the first mid-test caller would silently disable every timeout in that file, which is the defect this branch removes from the fixture reseed. It is awaited now. There are no callers yet, so nothing else changed. owui.setup.ts keeps its synchronous call, with a comment saying why: it runs once inside a setup project rather than mid-test, so the only deadline it can eat is its own, and `stdio: "inherit"` streams the installer's progress into the job log live where a buffered async child would withhold it until exit. Three smaller corrections. The flake table repeated the spec path in both columns, because `titlePath()` carries a leading empty root and slicing before filtering kept the file; it filters first now, and the comment matches what the code does. `fixture-reset.ts` promised a "reseed exceeded" message it never emitted, so the timeout kill now produces one instead of a bare signal. And the hook's elapsed time was logged only on failure, which is backwards: that number is what diagnoses this entire class, and it prints on every reseed together with the product budget being preserved. Proof, all observed: retry-pass, --reporter=list, flag present exit 1 retry-pass, --reporter=list, flag removed exit 0, reporter never ran clean run, --reporter=list, flag present exit 0 retry-pass, configured reporters exit 1, table names one spec reseed helper driven down its failure path logs elapsed and the relay, product budget kept at 30000
Scope added after review, during the audit that traced the Web E2E reds on main to this branch. `auth-shell.spec.ts` carried "workspace switcher persists selected account". It called `selectOption(value)` and then asserted `toHaveValue(value)` on the same element. `selectOption` sets that value client-side, so the assertion was true the instant it was reached, before any navigation could occur. It proved nothing about persistence while reading as though it did, which is camouflage shape 3 in docs/TESTING-STANDARD.md. Two lesser problems came with it: it skipped itself whenever the account had fewer than two workspaces, and it hand-rolled the switch rather than calling support/workspace-switch.ts. Deleted rather than repaired, because the repaired version already exists. console-workspace-switch.spec.ts, "switching workspace takes effect and survives a reload", switches by name, reloads, and asserts the value the server rendered from the cookie the switch handler set, in both directions. It passed in 14.7s in the very run where the deleted test failed, so no coverage is lost. A comment in auth-shell.spec.ts records what was removed, why, and where the coverage now lives. auth-shell.spec.ts goes from three tests to two. Every import it had is still used by the invitation test that remains.
sakibsadmanshajib
force-pushed
the
test/e2e-flake-zero
branch
from
August 10, 2026 19:09
7722e02 to
e93fafd
Compare
This was referenced Aug 10, 2026
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 11, 2026
…s the loop (#843) Two guards from an external adversarial review, both closing silent failure holes in the Playwright suite. Neither needs a live stack, a browser, or a credential. ## Guard 1: a spec can silently vanish from the run plan **Mechanism chosen: a committed manifest that pins identity, plus a hard fail on any listing error.** `apps/web-console/playwright-spec-manifest.json` maps every Playwright test module (36 files: 34 specs plus 2 setup files) to the exact set of projects that collect it. `apps/web-console/scripts/verify-spec-collection.mjs` lists both configs (`playwright.config.ts` and `e2e/phase-19/owui/playwright.owui.config.ts`) and fails on any of: 1. `playwright test --list` exits non zero, or its JSON report carries load errors. This is the syntax error and throwing top level import case. The failure message prints the Playwright error rather than the 4000 character resolved config that surrounds it in the JSON report. 2. `DROPPED`: a manifest entry still on disk that no project collects any more. This is the `testDir` or `testMatch` edit that quietly stops matching, which produces no error at all. 3. `DELETED`: a manifest entry whose file is gone. 4. `REWIRED`: a spec collected by a different set of projects than the manifest pins. 5. `UNDECLARED`: a spec that runs but is not in the manifest. 6. `DARK`: a file in the tree that no project collects, so it can never fail. **Why a manifest rather than an expected count.** A count is satisfiable by accident: add one spec and drop another in the same PR and the number is unchanged, so the guard stays green while a test goes dark. Pinning names also makes deliberate and accidental cases distinguishable, which was the requirement. A deliberate removal or rewiring is the one line manifest edit the failure message prints verbatim, made in the same PR. An accidental drop surfaces as a manifest line nobody meant to touch, in a diff a reviewer is already reading. **Why not only "fail if `--list` errors".** That covers case 1 and nothing else. Cases 2, 3 and 6 raise no error: Playwright simply never loads the file. It is included, but as one of six checks rather than the whole guard. Two further details worth knowing: - The owui config gates every project's `testMatch` on credentials being present, so listing it from a job with no OWUI secrets finds zero specs and calls that fine. The guard supplies obvious placeholders (`unused-collection-guard-placeholder`) for the listing only, which makes collection deterministic everywhere and puts the nightly owui specs under the same guard as the per push ones. Listing never dials out, so no real credential is involved. - This replaces `apps/web-console/scripts/verify-phase19-collection.sh`, which asked the same question for the `phase-19` project only. The new guard subsumes it: an owui spec leaking into the `phase-19` project now fails as `REWIRED`, which is what the old script's count comparison existed to catch. ### Red then green proof Two failure modes were exercised, one per hole. **A. Syntax error in a spec (the requested proof).** Appended an unterminated `const broken = (` to `apps/web-console/tests/e2e/unauth.spec.ts`: ``` spec collection guard FAILED: `playwright test --list` exited 1 for playwright.config.ts. A spec file did not load (syntax error, throwing top-level import, or a bad config block), so the run plan is incomplete. SyntaxError: /.../apps/web-console/tests/e2e/unauth.spec.ts: Unexpected token (115:0) 113 | 114 | const broken = ( > 115 | | ^ ``` Exit code 1. Restored the file, re run: ``` spec collection guard: OK, 36 Playwright test files collected across 2 configs, each into the projects the manifest pins it to. ``` Exit code 0. **B. Silent drop with no error at all.** Narrowed the `phase-19` `testMatch` to `/\/phase-19\/0[1-6][^/]+\.spec\.ts$/`, which drops one spec and raises no Playwright error: ``` spec collection guard FAILED: DROPPED e2e/phase-19/07-cross-tenant-attack.spec.ts is still on disk but no Playwright project collects it any more (manifest says ["phase-19"]). A testDir/testMatch edit removed it from the run plan. Restore the wiring, or drop its manifest line if that was deliberate. ``` Exit code 1. Reverted the `testMatch`, re run green as above. This is the case the previous state had no signal for. ## Guard 2: synchronous child process calls defeat every timeout in their file **Mechanism chosen: a structural lint with a self test, matching call shape and import clauses.** `tools/lint-no-sync-child-process-in-tests.mjs` bans `execFileSync`, `execSync` and `spawnSync` across `apps/web-console/e2e` and `apps/web-console/tests/e2e`, which is where both Playwright configs root their projects. Support modules are in scope because a spec importing a helper that blocks the loop is blocked just the same. Detection is call shape (`name(`) plus `child_process` import and require clauses, so an aliased import (`import { execSync as run }`) is caught even though no call site spells the banned name. It deliberately does **not** strip comments first: stripping `//` to end of line also eats anything after a URL on that line, which is a way to hide a real call from the scanner. A prose comment that spells a banned call with its parenthesis will trip the guard, which is a loud and harmless false positive; the existing prose in `tests/e2e/support/*.ts` that explains why the sync form is banned is covered by a `MUST_ALLOW` case so the explanation cannot trip the ban. Allowlist is one entry, `apps/web-console/e2e/phase-19/owui/owui.setup.ts`, carrying the reason PR #838 recorded: it runs once in a setup project rather than mid test so it can only eat its own deadline, and `stdio: "inherit"` streams installer progress that a buffered async child would withhold. The lint also fails if an allowlisted path stops existing, so a rename or delete forces the allowlist edit rather than leaving a stale exception nobody reads. A `MUST_CATCH` and `MUST_ALLOW` self test (17 assertions) runs as a preflight on every invocation, matching the pattern of `tools/lint-no-pgx-dsn-for-libpq.mjs`. `MUST_CATCH` includes the `#838` shape verbatim, a namespaced call, a spaced call, an aliased import, a bare specifier import, a multi line import and a require destructure. ### Red then green proof Added a banned call to a spec where it is not allowlisted, `apps/web-console/tests/e2e/unauth.spec.ts`: ``` lint-no-sync-child-process-in-tests: self-test ok (17 assertions) apps/web-console/tests/e2e/unauth.spec.ts:4: `execFileSync` blocks the Playwright worker's event loop, which stops every test timeout in this file from firing while the child runs. execFileSync("true", []); Use the async form (execFile + promisify, or spawn with an awaited close) and await it. If this genuinely cannot block a test deadline, add the file to ALLOWED in tools/lint-no-sync-child-process-in-tests.mjs with the reason. apps/web-console/tests/e2e/unauth.spec.ts:2: `execFileSync` blocks the Playwright worker's event loop, ... lint-no-sync-child-process-in-tests: a synchronous child process in the Playwright tree disarms the timeouts around it. PR #838 found a test recorded as passed at 30194 ms under a 30000 ms timeout for exactly this reason. ``` Exit code 1, and note it caught both the call and the import. Removed the call and the import, re run: ``` lint-no-sync-child-process-in-tests: self-test ok (17 assertions) lint-no-sync-child-process-in-tests: ok (46 files across apps/web-console/e2e, apps/web-console/tests/e2e; 2 allowlisted call(s) in 1 file(s)) ``` Exit code 0. The 2 allowlisted hits are the import and the call in `owui.setup.ts`. ## Wiring, verified by reading the `run:` blocks Not by grepping for filenames. Both jobs are required contexts in `.github/branch-protection-main.json`. `repo-policy-lints`, `name: Repo policy lints (tenant + audit)`: ``` run: node .github/ci/lint-workflow-check-names.mjs run: npm run lint:tenant-setting ... run: npm run lint:litellm-config run: npm run lint:sync-child-process <- guard 2 run: npm run lint:no-client-cost-fields <- drive by, see below run: make test-scripts ``` `web-unit`, `name: Web console (type + unit + build)`: ``` run: npm ci run: npx tsc --noEmit run: npm run e2e:verify-collection <- guard 1 run: npm run test:unit run: npm run build ``` The collection guard sits in `web-unit` because it needs the web console `node_modules` that job already installs. The lint sits in `repo-policy-lints` because it needs nothing but node and should fire on every PR. Both jobs run with `if: always()` and apply the docs only decision per step, so neither guard can be skipped into an absent check. `node .github/ci/lint-workflow-check-names.mjs` still passes after the workflow edit: 19 check names across 6 pull request workflows, 6 required contexts each published by exactly one always running job. ## Drive by `npm run lint:no-client-cost-fields` existed in the root `package.json` but no workflow ran it, so it had never executed in CI. That is the same silent absence one level up. It is now a step in the same required job and passes on main today. Nothing else in the diff depends on it. A follow up worth considering, not built here: a meta guard that fails when any `lint:*` script in the root `package.json` is not referenced by a `run:` block in a required job. That would make this class of dark check impossible rather than found by hand. ## Constraints honoured - Nothing was made to pass by adding `continue-on-error`, a skip, or a loosened check. Both guards are steps in existing required contexts, not new non required ones. - No live stack, no database, no pooler connection. `--list` loads spec modules and exits; the lint reads files. - No password, key, token, JWT or connection string is written, printed or rotated. The placeholders the owui listing uses are literal strings ending in `-placeholder`. - Local verification before push: `npx tsc --noEmit` clean, both guards green, `lint-workflow-check-names` green. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added comprehensive validation to ensure end-to-end test files are collected by the correct projects. * Added a manifest documenting test coverage and project assignments. * Added linting to detect synchronous child-process usage in end-to-end tests. * **Bug Fixes** * Improved reporting for missing, undeclared, incorrectly assigned, or uncollected tests. * Replaced a project-specific collection check with a broader validation across test configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Review follow up (baa2a6c) CodeRabbit found a real bypass in guard 2: `const run = require("node:child_process").execSync;` was invisible, because no line ever spelled a banned name followed by a parenthesis. Fixed more broadly than the reported shape. Property access to a banned name is now an offence in its own right, whether the result is called there or only aliased, which also covers `import cp from "node:child_process"; const run = cp.spawnSync;` and `cp["execFileSync"]`. Four more `MUST_CATCH` cases, self test now 21 assertions, verified red then green against the real tree with the reported snippet. A regex that was defined and never used was deleted in the same commit. Thread resolved.
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 25, 2026
## Revival, 2026-08-24: rebased and green on today's auth surface The stall is diagnosed and fixed. The project-ref mismatch was a theory about two repository secrets disagreeing; what actually happened is simpler and worse. This job read its five Supabase values from repository secrets while main had already moved Web E2E onto a throwaway in-job Supabase (Postgres + GoTrue + PostgREST behind one nginx gateway) whose five values are written to `$GITHUB_ENV` by `scripts/ci-supabase-stack.sh`. The secrets name a hosted project that no longer backs this repo's auth surface after the self-hosted cutover (PRs #982-#993), so the mint wrote a complete state file whose cookies the bundle never looked for. One source for all five values makes the mint's cookie and the bundle's expected cookie agree by construction. What changed: - **The CI arm boots its own throwaway Supabase**, the same arrangement Web E2E uses (`scripts/ci-supabase-stack.sh`), instead of repository secrets. - **The seeder's verbose progress line moved to stderr** so the seed-check step can JSON.parse its captured stdout; with both lines on stdout the check died on "Unexpected token" before ever comparing addresses. - **The unit half stays required**: 46 cases in `gate-integrity.test.ts` run inside the required web-unit job (green here, including after the 103-commit rebase). - **The sweep half stays advisory per-PR** until it has a green track record on main, then argue for required. It went green end to end against a throwaway stack built from this branch (own Postgres on 55433, own GoTrue and PostgREST gateway on 9001, compose core on 18081 and 18080, Next.js on 3000): ``` routes discovered 26 visited with proven controls 22 controls 400 enumerated, 396 proven, 2 declared, 2 disabled, 0 unproven COVERAGE 99.0% (distinct control identities) problems 0 ``` Core routes all at 100%: `/console/api-keys` 20/20, `/console/billing` 25/25, `/console/billing/alerts` 21/21, `/console/billing/budget` 20/20, `/console/billing/invoices` 17/17, `/console/settings/profile` 25/25, `/console/analytics` 30 proven plus one declared inert and one disabled. Session establishment is proven by the same run: the setup minted through the admin one-time-token flow (`live-auth.mjs`) and every authenticated route rendered instead of redirecting. And it is green in CI itself, not only locally: on head `ab74bc11d` the `Interaction coverage (console controls)` job passed with the same sweep shape (400 enumerated, 396 proven, 0 unproven, COVERAGE 99.0%, run 32807539583), alongside green Web E2E and the required web-unit job. The one extra fix that took was a Node 20 to Node 24 bump for this job, matching Web E2E: Node 20's npm rejects the current esbuild optional-dependency set with EBADPLATFORM before any test runs. ## Buglog entry ```json {"id":"interaction-gate-hosted-secret-precedence","date":"2026-08-24","area":"apps/web-console/tests/interaction","error_message":"every authenticated route redirected to /auth/sign-in while the storage state file was complete","root_cause":"The job read its five Supabase values from repository secrets after main had moved Web E2E onto a throwaway in-job Supabase; post cutover the secrets name a hosted project that no longer backs the console, so the minted cookies were never looked for. A job-level env entry also takes precedence over what a boot step writes to GITHUB_ENV, which would have kept the drift alive even after the throwaway stack existed.","fix":"Boot scripts/ci-supabase-stack.sh inside the job and derive all five values from its output, mirroring Web E2E. Also moves the fixture seeder's verbose progress line to stderr so the seed-check step can JSON.parse its captured stdout.","tags":["ci","supabase","test-infrastructure","session-establishment"]} ``` --- Enumerates every interactive control the console renders, activates each one, and requires observable evidence that it did something. A control with no proven effect fails the run. This is a rework. An adversarial review returned BLOCK on twelve findings, and the headline was correct: the gate had never measured a single control in CI. Everything below is what changed. ## 1. The gate never ran, and now it does `tests/interaction/auth.setup.ts` imported `writeStorageState` from `../e2e/support/live-auth`. Two modules answer to that name. The type checker resolved it to the synchronous `.ts` wrapper; Node resolved it to the asynchronous `.mjs`. The returned promise floated, the setup reported success in 13 milliseconds having written no storage state, and the sweep died reading that missing file (run 31445547804). The job had never enumerated a control, and the only symptom was an ENOENT that read like a harness bug. Naming the `.mjs` explicitly does not fix it either: Playwright compiles specs to CommonJS and evaluating that in ES module scope fails with `exports is not defined`, which takes the whole run plan down. The spec collection guard from #843 caught that attempt, which is a good advertisement for the guard. The setup now uses the documented command line form, fails loudly when the state file is absent, and deletes any state a previous run left behind. The job also never passed `SUPABASE_SERVICE_ROLE_KEY`, which the mint requires, so awaiting alone would only have moved the failure. It passes it now. `INTERACTION_PASSWORD` is gone: nothing read it, and a credential-shaped variable in a workflow is an invitation to wire it up. ## 2. The gate wrote to the surface it measures Destructive controls were clicked against whatever origin the run pointed at, with Escape pressed afterwards. Escape after a click is not a safeguard: the request has already gone. Submissions were pre-filled with probe values and sent for real. **What that actually did against the live demo console.** No delete and no revoke ever fired, and no control matching the destructive pattern did anything but navigate. That framing was too narrow, and a later review pass proved it: the pattern is a text match, so it says nothing about what a control does. `Change email` on `/console/settings/profile` calls `supabase.auth.updateUser`, matched nothing, and **was activated in both runs**, issuing a real `PUT /auth/v1/user` each time with the probe address in the field beside it. GoTrue records a pending change and mails a confirmation rather than switching the address, so the account most likely holds an unconfirmed pending change to a domain that cannot receive mail; `auth.users.email_change` and `email_change_token_new` want checking and clearing. Everything that reached `https://console-hive.scubed.co` on 2026-08-08 and 2026-08-10: | Route | Request | Effect | | --- | --- | --- | | /console/api-keys | `POST /api/v1/accounts/current/api-keys` | created an API key named after the probe value, on both runs | | /console/members | `POST /api/console/members` | sent a workspace invitation to `interaction-gate@example.invalid` | | /auth/sign-up | `POST /auth/v1/signup` | sign-up attempt for that same address | | /auth/forgot-password | `POST /auth/v1/recover` | password recovery request for that address | | /auth/sign-in | `POST /auth/v1/token` | failed sign-in attempt | | /console/settings/profile | `PUT /auth/v1/user` | requested an email change to the probe address, pending confirmation, on both runs | | billing budget, spend alerts, reset password | client side only | no request left the browser | No customer data was touched and nothing was deleted, but two probe API keys and at least one invitation are real writes on the demo tenant and want cleaning up by hand. The fix is one rule: **when the values in a request are the gate's own invention, or the control's label says delete, revoke or purchase, the request never leaves the browser.** A mutation guard on the browser context aborts it, the application still builds and issues it, and the interception is what proves the control is wired. The one deliberate exception is a toggle, which invents no value, whose whole proof is that the flip survives a reload, and which the gate flips back and now fails if it could not. ## 3. Floors that could never hold The old floor was a minimum control count per route, recorded against the demo tenant. The report says two files away that instance counts are not comparable between runs, and it is right: a CI account with an empty workspace renders fewer rows, so the floor was either red forever or regenerated in CI and thereafter below what the live console renders. Replaced by the set of control identities each route must render **and leave enabled**. The console shell renders its navigation from a static list on every route, and each page renders its own primary action, so the same list holds against an empty CI tenant and the live console while still failing on a blank page, a crashed route, or a permission regression that greys the surface out. There is no regenerate command any more: a bar a run can rewrite is a bar a run can lower. ## 4. Proof from markup - A disabled control counted as proven on any `title` attribute, so greying out a page kept the gate green. Disabled is now its own bucket, never the numerator. What fails an unexpected disable is the floor above, which is the only place this suite asserts a control must be usable. - A text field counted as proven on a `name` attribute alone. It now needs an observable consequence or a form to submit into, and the DOM signature tracks disabled state so that typing into a field which enables the Save button beside it registers as the effect it is. - A toggle whose restore failed warned and still returned proven, leaving the live setting flipped. That fails now. ## 5. Scoring nothing as everything `ratio(0, 0)` returned 1, so `/oauth/consent` (recorded floor: zero controls) and `/invitations/accept` reported 100% coverage of pages nothing had ever rendered on. It returns 0, a visited route that enumerates nothing is an integrity failure, and both routes are now declared skips with an owner and a reason: reaching either means holding a credential in a query string, which this gate must never hold or log. A run that visits no route, or enumerates no control, now fails on that fact alone. That is the exact state this gate shipped in. ## 6. Exclusions with an ending `expectRedirect` silently removed a route from measurement and was not treated as an exclusion at all, so it needed no owner, no issue and no expiry. It does now, like every skip and every registry entry. The expiry check itself used `it.runIf(token)` against a job that passed no token, so it had never run anywhere; the web-unit job passes `GITHUB_TOKEN` and the check fails rather than skipping when CI is set. A tracker that will not answer is now a failure too, instead of a silent continue. Two exclusions are new, both citing open issues, both expiring when those close: - #883, the console links Documentation to `https://hivegpt.io` from every route, and that host accepts no connection. This is the gate's first real finding and it is a product defect, not a test problem. - #885, `/console/api-keys/[id]/limits` needs a key to exist before anything links to it, and the gate now refuses to create one. ## 7. Rebased, with the guards from #813, #838 and #843 intact `failOnFlakyTests`, the flake reporter, the `probe` project and the `chromium` `testIgnore` are all present, and both interaction specs are pinned in `playwright-spec-manifest.json`. The two interaction projects additionally set `retries: 0` against the repository default of two: the sweep is one test that walks the whole console, so a retry re-walks all of it, and a control that only works on the second attempt is the defect this gate exists to report. No trace and no video for these projects either. The sweep types into password fields and a trace carries the `Authorization` header of every request the console made, the same exposure that already stops this job uploading the HTML report (#554). It is also expensive: a failing run spent over ten minutes finalizing artifacts, on a job capped at sixty, and finishes in ten seconds without them. ## 8. The CI arm cannot establish a session today, and that is the honest status **Do not read the `Interaction coverage (console controls)` job as a measurement of the console yet. It is not reaching the console.** On the latest run every authenticated route redirected to `/auth/sign-in`, including `/no-workspace`, which needs only a session and no workspace. So the browser carried no session the application would accept, and the sweep measured five anonymous routes out of twenty four. The run before it failed differently, redirecting to `/no-workspace`, because this job's `E2E_RUN_KEY` did not match what its addresses carried and `runScopedEmail` therefore seeded one account while the gate signed in as another. Fixing that changed which way it fails rather than fixing it. What this PR does about that, rather than papering over it: - **No `expectRedirect` declarations for those routes.** Declaring them would convert a broken session into a documented expectation and produce a green run that measures nothing, which is the precise failure this gate exists to detect. They stay as integrity problems and the job stays red. - **The failure is named once, at the seam.** The setup now opens `/console` with the storage state it just minted and fails there if it lands on a sign-in page, listing what to check in order: whether `SUPABASE_URL` and `NEXT_PUBLIC_SUPABASE_URL` name the same project, since the auth cookie's name is derived from the project ref and a mismatch yields a complete state file the app cannot see; whether the seeded account exists on that project; and whether `E2E_RUN_KEY` is exactly the string the addresses carry. One message with a cause beats fifteen identical redirects with none. **What the gate is worth in the meantime.** Its unit half, 46 cases in `gate-integrity.test.ts`, runs in the **required** `Web console (type + unit + build)` job and is green: the proof predicate, the floors, the registry, exclusion expiry against the live tracker, URL redaction, and the sign-in decisions that caused the original incident. Its sweep half is proven to measure and proven to go red on a broken control against a local build of this branch, and has measured 326 controls across 20 routes in CI when the session did work (run 31519164067). What is unproven today is only the CI arm's ability to sign in. **The expected-red list is deliberately not carried forward.** It described run 31519164067, and the coordinator is right that the list has to be re-derived from a run that genuinely reaches all twenty four routes. Three findings already have issues and self-expiring declarations: #905 (password reset answers a generic server error instead of naming an expired link), #883 (Documentation links point at an unreachable host), #885 (the per-key limits route is unreachable without a key the gate refuses to create). Whether those are the whole list is a question only a working run can answer. ## 9. Required or advisory **Advisory, and stated plainly rather than quietly.** It is a sweep of a whole application against a live-ish stack, so its failure modes include the stack, the network and the seeded account, not only the console. Making it required before it has a run history on main would block every merge on any of those. It graduates the way rust-tests and desktop-tests did: a track record first. Advisory does not mean silent. The job reports its own red, and the unit half (`gate-integrity.test.ts`, 40 tests) runs inside the **required** web-unit job, so a malformed registry, a stale exclusion, an entry naming a route that no longer exists, or a broken enumerator fails a required check whether or not the sweep runs. ## Proof that it measures, and that it can go red Run against a Next.js build of this branch, on the `/auth/sign-in` route, with the mutation guard active. Clean, then the same route with one control's handler neutered at the event layer (`INTERACTION_SABOTAGE`), markup and siblings untouched. Clean: ``` [route] /auth/sign-in -> 5 controls enumerated ok /auth/sign-in input|#email dom ok /auth/sign-in input|#password dom ok /auth/sign-in a|Forgot password? navigation ok /auth/sign-in button|Continue wired-write-blocked ok /auth/sign-in a|Create one navigation controls 5 enumerated, 5 proven, 0 declared, 0 disabled, 0 unproven COVERAGE 100.0% (distinct control identities) 1 passed (11.0s) EXIT=0 ``` `wired-write-blocked` on the submit is the guard doing its job: the page built its sign-in request and the gate stopped it inside the browser, so no credential attempt left the machine. Sabotaged (`INTERACTION_SABOTAGE=Continue`): ``` [sabotage] handlers blocked for: Continue ok /auth/sign-in input|#email dom ok /auth/sign-in input|#password dom ok /auth/sign-in a|Forgot password? navigation XX /auth/sign-in button|Continue unproven ok /auth/sign-in a|Create one navigation UNPROVEN CONTROLS (1) /auth/sign-in button|Continue unproven: activation produced no request, no navigation, and no change to the rendered output (form pre-filled: email, password) Error: controls with no proven effect (control surface coverage 80.0%) 1 failed EXIT=1 ``` Same page, same markup, one dead handler, and the gate says which control and why. Local verification: `npx tsc --noEmit` clean, `npm run test:unit` 531 tests across 44 files including 40 in `gate-integrity.test.ts`, `npm run e2e:verify-collection` clean at 38 collected files. The exclusion expiry check was additionally run both ways: it fails with `CI=1` and no token, and passes against the live tracker with one. ## Buglog entry ```json {"id":"interaction-gate-floating-mint","date":"2026-08-11","area":"apps/web-console/tests/interaction","error_message":"Error reading storage state from tests/interaction/.auth/user.json: ENOENT","root_cause":"An extensionless import of ../e2e/support/live-auth resolved to the synchronous .ts wrapper for tsc and to the asynchronous .mjs at run time. The unawaited promise floated, so the setup passed in 13ms without minting a session, and the sweep failed three hundred lines later on the missing file. The job also never passed SUPABASE_SERVICE_ROLE_KEY, so the mint would have thrown even once awaited.","fix":"Call the documented live-auth.mjs command line form from the setup, assert the state file exists before the sweep may run, and pass the service role key in the workflow. Naming the .mjs in an import is not an alternative: Playwright's CommonJS output fails with 'exports is not defined' in ES module scope.","tags":["playwright","test-infrastructure","silent-failure","module-resolution"]} ``` --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
sakibsadmanshajib
added a commit
that referenced
this pull request
Sep 2, 2026
lint-no-sync-child-process-in-tests flagged the new proof spec, correctly. A synchronous child blocks the Playwright worker's event loop, so the 360 second timeout the spec sets would never have fired while the capture ran: the exact shape PR #838 found recorded as passed at 30194 ms under a 30000 ms timeout. Awaited spawn instead, with stdio inherited so the capture log still reaches the run log live, and the child's exit code surfaced as the test failure. The per-child timeout is gone with the sync call, which is the point: Playwright's own timeout is now the one that actually applies.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gate 4 of promoting
Web E2E (full stack)to a required merge check: drive the measured 7.7 percent flake rate to zero by removing the cause, and make any future retry-pass impossible to miss.The two specs
Identified from the artifact of CI run 31361681115, by unzipping the report JSON embedded in the HTML report and reading each test's
outcomeandresults[]. Not guessed from names.That run collected 34 tests, skipped 6, failed 2 and executed 26. Two of the 26 passed only on a retry, which is the 7.7 percent. Both are in
apps/web-console/tests/e2e/profile-completion.spec.ts:profile-completion.spec.tssetup saves profileprofile-completion.spec.tsprofile settings stay reachable while unverifiedRoot cause
Category: test infrastructure. Neither is a product race. I looked for one and did not find it; the browser behaved correctly in every attempt I examined, including the failing ones.
Nine spec files copy-paste a
beforeEachthat reseeds the shared Supabase fixtures through a child process. Playwright chargesbeforeEachto the test's own timeout, and the reseed talks to a shared cloud project, so its wall time is set by conditions no spec controls. Step timings for that identical call in that one job:beforeEach hooksetup saves profilesetup saves profilesetup saves profileprofile settings stay reachable while unverifiedAgainst a declared 30 second timeout, every spec in the file was running with a random slice of its budget. The second test's failing attempt spent the entire 30 seconds inside the hook and never opened a page, which is why its error reads
Test timeout of 30000ms exceeded while setting up "context"and names no product surface at all.A second defect sat underneath. The reseed ran through
execFileSync, which freezes the worker's event loop, and Playwright's timeout is an ordinary timer that cannot fire while the loop is blocked.setup saves profileattempt 1 was therefore reported as passed at 30194 ms under a 30000 ms timeout: a test that overran its deadline and went green anyway. That is a camouflage shape in its own right, so it is fixed here too rather than left as a footnote.The fix
One shared helper,
apps/web-console/tests/e2e/support/fixture-reset.ts, replacing the copy-pasted block in all nine specs (107 lines deleted). It:testInfo.setTimeout, so the product always gets exactly the budget the spec author declared. It composes, so a second reseeding hook in the same test adds its own elapsed time on top.Fixing it in the shared helper rather than only in
profile-completion.spec.tsis deliberate: the other eight files carry the identical defect and are all in scope once this check is required.Nothing here raises
retries, adds a blindwaitForTimeout, loosens or deletes an assertion, or skips a spec.I also updated the long comment in
e2e-fixture-seed.mjsthat explicitly reasoned from "runs via execFileSync, fully synchronous". Its ordering guarantee never depended on the event loop being blocked, only on theawaitand onworkers: 1, and the comment now says so and names the new revisit condition.Retries are now visible
Today's job log ends with a
listreporter tail that folds retry-passes into its pass count, so a run that needed two retries reads identically to a clean one. Finding these two meant downloading a 5 MB artifact and unzipping the HTML report's embedded JSON by hand.flake-reporter.tsnow writes a table of every retry-pass into the GitHub job summary (and stdout locally), and fails the run when there is one at all.The threshold is zero, and that is argued in the code. This check is being promoted to required, and a retry-pass is precisely the failure mode that promotion has to survive: it blocks main at random and teaches everyone to press re-run until green, which is the same habit whether the rate is 8 percent or 4. Any non-zero percentage also silently loosens as the suite grows, since one flaky test scores better in a bigger denominator. Retries stay switched on, because they are what produce the second attempt, the trace and the video that identify the flake; what changes is that a run which needed them no longer reports success. Raising it is a one line diff a reviewer can see and argue with, which is why there is no environment variable for it.
Not applied to the OWUI nightly config, deliberately: those specs drive real free tier provider latency, and a zero flake gate there would fail nightly for reasons unrelated to this check.
Red proof
Per
docs/TESTING-STANDARD.md, everything added here was watched failing first.gateTripped = falseinbuildFlakeReportexecuted = entries.length(skipped back in the denominator)playwrightexit code 1, summary named the spec and "Attempts 2"The unit tests cover the pure
buildFlakeReport; the reporter's mapping from Playwright'sSuiteonto it is what the two end to end runs above prove, so neither half is only asserted against a hand copy of itself.Before and after, 10 runs each
Both arms ran against the same booted control-plane and console, the same live Supabase project and the same seeder, one attempt per trial (
--retries=0). Only the spec changed between arms, restored withgit stash.Natural seeder latency (measured 9.6 s to 20.3 s on this box, no injection):
setup saves profileprofile settings stay reachable while unverifiedThe second test did not reproduce in that window: this box's seeder happened to stay fast enough for it. So I reran it with the hook latency pinned to what the failing CI attempt actually showed (about 30 s total), which is the condition that broke it in CI:
profile settings stay reachable while unverified, CI hook latencyThe one remaining failure in each after arm was an upstream Supabase problem, not the budget: a
Could not query the database for the schema cacheretry in one, and a sign-in exceeding its own 25 s timeout on a heavily loaded box in the other. Both are now reported as what they are, a seeder failure and a sign-in timeout, rather than as an unexplained UI timeout. That is the fix working as intended.The mechanism is also visible directly in the run output: the same spec on the same box reports
Test timeout of 30000ms exceededbefore andTest timeout of 43103ms exceededafter, the difference being the 13103 ms the hook actually took.Residual flake rate, and what it means for promoting this check
Roughly 10 percent per trial on one test. Not zero, and it should not be read as zero.
Across the two after arms,
profile settings stay reachable while unverifiedfailed once in each set of ten, so 1/10 and 1/10. Neither failure was the timeout budget this PR fixes:accounts upsert failed: Could not query the database for the schema cachepage.waitForURL: Timeout 25000ms exceededinsignInThe attribution is sound in mechanism, both failures carry an explicit upstream error rather than an inferred one, but the evidence for each is a single occurrence and no artefact is linked, so treat the mechanism as established and the rate as a rough estimate rather than a measured constant.
The consequence matters for the promotion decision and should not be discovered afterwards. With
retries: 2and a threshold of zero, an upstream hiccup of this kind consumes a retry, and if it recurs on the retry the job goes red; if the retry rescues it, the run becomes a retry-pass, which this PR now deliberately fails. Either way a transient Supabase or sign-in stall becomes a red required check onceWeb E2E (full stack)is required. That is the intended behaviour, since the alternative is exactly the re-run-until-green habit this work exists to prevent, but it means promotion should account for a non-zero rate of reds that no code change in this repository can remove.Two things reduce it, neither in scope here: #837 makes the pooler less contended, which shortens the seeder and narrows the window; and the 25 second cap inside
signInis a hardcoded local timeout that does not scale with load, which is worth revisiting separately.Review follow-ups (second commit)
The gate had an escape hatch, now closed.
npx playwright test --reporter=listreplaces the configured reporters outright. Under that flag the custom reporter never ran and the run exited 0 on a retry-pass, so one command line argument disabled the whole gate.failOnFlakyTests: trueis now set alongside the reporter and survives the override. Both layers are kept on purpose: the built-in flag gives an exit code and nothing to act on, the reporter is what names the test and its attempt count.--reporter=list, flag present--reporter=list, flag removed--reporter=list, flag presentlive-auth.tsno longer blocks the event loop. It is the sanctioned auth helper and itsreauthenticateis built for mid-test use, so the first mid-test caller would have silently disabled every timeout in that file, which is precisely the defect this PR removes from the reseed. It has no callers yet, so the conversion touched nothing else.owui.setup.tskeepsexecFileSync, deliberately. It runs once inside a setup project rather than mid-test, so the only deadline it can eat is its own, andstdio: "inherit"streams the installer's progress into the job log live where a buffered async child would withhold it until exit. The reasoning is now a comment in the file so the next reader does not have to rediscover it, with the condition that would change the answer.Three corrections. The flake table repeated the spec path in both columns, because
titlePath()carries a leading empty root and slicing before filtering kept the file; it filters first now and the comment matches the code.fixture-reset.tspromised a "reseed exceeded" message it never emitted, so a timeout kill produces one instead of a bare signal. Hook elapsed time was logged only on failure, which is backwards for the one number that diagnoses this class, so it prints on every reseed alongside the preserved product budget.Scope added after review, third commit
Flagged explicitly because it was not part of what was approved, and an earlier approval should not be stretched over it silently.
An independent audit of the
Web E2Efailures on main traced them to this branch's fix, with a head to head control run on the same live Supabase three minutes apart: all threeauth-shell.spec.tstests green on first attempt at 0 percent flake here, against two of them failing on main at 36.3 s and 32.6 s under a 30000 ms timeout. A test overrunning its own declared timeout and being recorded at all is the event loop freeze this PR removes, so that is a direct confirmation of the mechanism from an independent run.That audit surfaced one more test of the same family, and this commit deletes it:
workspace switcher persists selected accountinauth-shell.spec.ts.Why it went rather than being repaired:
switcher.selectOption(secondValue)and then assertedexpect(select).toHaveValue(secondValue)on the same element.selectOptionsets that value client side, so the assertion was already true when it was reached, before any navigation. It read as a persistence check and was not one. That is camouflage shape 3 indocs/TESTING-STANDARD.md.console-workspace-switch.spec.ts,switching workspace takes effect and survives a reload, switches by workspace name, callspage.reload(), and asserts the value the server rendered from the cookie the switch handler set, in both directions. That is where the coverage lives, and it is unchanged by this PR. It passed in 14.7 s in the very run where the deleted test failed, so deleting the weaker one loses nothing.tests/e2e/support/workspace-switch.ts.The alternative was to rewrite it to call
switchToWorkspace, reload, and assert the server rendered value. That would have made it a second copy of the test named above, so deletion was the smaller and honest change. A comment inauth-shell.spec.tsrecords what was removed, why, and which test retains the coverage, so nobody rediscovers the gap and writes the weak version again.auth-shell.spec.tsgoes from three tests to two, confirmed with--list. Every import it had is still used by the invitation test that remains.Separate finding, not fixed here
profile-completion.spec.tscontains a wait that never waits, the same shape as #826:Measured in the CI artifact, that step resolves in 0 ms, and the following
toHaveValue("Acme Labs LLC")then passes in 12 ms by reading the value the test itself typed into the unsubmitted input.billing settings save partial business profiletherefore cannot distinguish a saved billing profile from a silently discarded one. It was not flaky and is not this PR's cause, so it is filed separately rather than changed here, because fixing the assertion honestly requires first establishing whether the save actually persists.Scope note for reviewers
console-budgets.spec.tsandconsole-workspace-switch.spec.tsare also touched by #829. The overlap is only thebeforeEachblock at the top of each file, well away from the test bodies #829 edits, so whichever merges second should resolve trivially.🤖 Generated with Claude Code