Repository navigation
fix: seed E2E fixtures in-process with per-run isolation - #583
Merged
Merged
Conversation
The Web E2E credentialed specs (auth-shell.spec.ts, profile-completion.spec.ts, and six sibling files) called the full e2e-fixtures reset from test.beforeEach, so every single test paid for a fresh GoTrue admin round trip (password rehash included) plus a full accounts/tenants/invitation re-seed against the e2e-fixtures Supabase Edge Function. Measured against the live project this call takes 5.7 to 12.4 seconds per invocation (edge function cold starts and plain network latency), and Playwright's default per-test timeout (30s) is shared between a test's beforeEach hooks and its body. auth-shell.spec.ts's own signIn() helper already budgets 25s for the post-login redirect wait, so any nonzero reset time on top of that risked exhausting the test's entire timeout before the page had a chance to render, surfacing as an element-not- found or page.fill timeout with no reset failure logged, since the reset itself had already succeeded. Move the full reset to test.beforeAll: each of these files runs its tests in mode: "serial" against a fixed, shared set of e2e-fixtures accounts, and within a file only one reset is needed before the first test starts. Files that separately reset a single mutable field between specific tests (for example profile-completion.spec.ts's resetProfileBetweenSpecs for profile_setup_complete) keep that narrower reset in its own nested beforeEach; only the full identity/account/tenant/invitation reset moves. Also add a deterministic ORDER BY to ListMembershipsByUserID. Without one, Postgres does not guarantee row order across calls, and EnsureViewerContext picks memberships[0] as the default workspace whenever the console does not send a specific account id, which is exactly the case on first navigation after sign-in. The e2e-fixtures verified test user is the one identity in the suite with two memberships (owner in one workspace, member in another), so this file is also the one place the missing order was ever observable. Verification: could not bring up a fresh full stack in this environment tonight (control-plane's DB ping deadline was consistently exceeded against the pooler from this host, and GitHub/Go-proxy connectivity independently timed out), so the affected specs were not re-run end to end here. Verified instead by: gofmt/parse of the changed Go file; a manual audit of every test in all eight changed spec files confirming no test depends on another test's mutation being reset first within the same file; and 10 consecutive live calls to the real e2e-fixtures reset endpoint plus an immediate password- grant login, which measured the 5.7-12.4s latency this fix removes from the per-test budget and confirmed no JWT tenant-claim staleness on any call.
… BY to its own PR The missing ORDER BY is a real, user-facing production defect (any signed-in user with more than one workspace membership gets a nondeterministic default workspace), not an E2E-fixtures test-support concern. Splitting it into its own PR so it can land, be reviewed, and be reverted independently of this file's Playwright timeout-budget fix.
|
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: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…create seedAccountsAndMemberships deleted six (account_id, user_id) pairs from account_memberships, then upserted five of them back in a second, separate Supabase call. Five of those six pairs were always going to be rewritten by the upsert anyway, so deleting them first bought nothing but a window, per reset call, where each of those rows had zero memberships. A read landing in that window sees an empty membership list for that user and account, and control-plane's EnsureViewerContext responds by calling provisionDefaultWorkspace, which hands the signed-in fixture user a brand new, unrelated workspace instead of the one e2e-fixtures actually seeded. This is a real server error, not stale data: the affected page's Promise.all of getViewer and getAccountProfile has nothing to catch it, so it surfaces as the console error boundary or, if the accidental workspace happens to lack the expected fields, a UI element that never renders. This explains both originally reported failures directly. auth-shell.spec.ts "members page redirects unverified users to profile settings" hit the error boundary (confirmed from the CI run's page snapshot artifact: "Something went wrong on this page", matching app/console/error.tsx exactly). profile-completion.spec.ts "setup saves profile" timed out on page.fill waiting for a form field that never appeared, the same symptom on a different page, with a 30s action timeout instead of the 5s expect timeout the other test hit. Both users affected, verifiedUser (two real fixture memberships) and unverifiedUser (one), were in the deleted set. Fix: delete only the one pair that must NOT be recreated, verifiedUser's membership in the invited workspace, which needs to start every reset unaccepted so the invitation-acceptance spec has something to accept. The other five pairs are upserted with onConflict on (account_id, user_id), which already updates role and status in place with no window where the row is briefly absent, so they never needed a delete at all. Known adjacent risk, not fixed here: resetProfilesAndInvitation has the same delete-then-separate-upsert shape for the fixture invitation row (delete by account_id and email, upsert by id). Same class of window, different table, not implicated in either failure this run reproduced, left alone to keep this diff to the confirmed defect.
…stic failure beforeAll made every test in a file share one reset instead of getting its own. That reintroduced exactly the shared-mutable-state property flagged as a separate defect earlier: auth-shell.spec.ts and profile-completion.spec.ts both failed on their initial attempt and both retries once this landed, which is deterministic, not the intermittent timing symptom this change was meant to fix. A change that trades a latency win for a real, reproducible test failure is not a trade worth making. Reverting to beforeEach; every file in this diff is now byte-identical to origin/main again. The actual fix for both those failures lives in the next two commits: seedAccountsAndMemberships no longer opens a window where a fixture user has zero memberships (already landed), and this branch now also gives every CI job its own namespaced fixture identity so concurrent jobs against the same live Supabase project can no longer collide on shared rows at all.
sakibsadmanshajib
changed the title
Owner
Author
|
Merge held. This branch is now up to date with main (which carries the session mode pooler fix), all six required contexts are green, and there are zero unresolved review threads, but Web E2E fails deterministically on two specs:
Both time out in the Root cause is the split between the two halves of this change. Unblocking needs the edge function deployed before merge ( |
Owner
Author
|
Blocking this from merge, and correcting the inert-until-deployed banner in the body: this change is not inert. It actively breaks the Web E2E suite until the edge function is deployed, so the ordering matters. EvidenceWeb E2E fails on this branch across all three attempts, on two tests:
Both fail the same way, `page.waitForURL` exceeding the 25000ms sign-in wait. The same job passes on `main` at `bd29a5ff` and `be5132ab`, and this branch already contains `bd29a5ff`, so neither a stale base nor the pooler DSN regression explains it. Mechanism`e2e-auth-fixtures.mjs` now sends `runKey`, `verifiedEmail`, `unverifiedEmail` and `invitationToken` in the reset body, and relies on the edge function seeding exactly that identity rather than deriving its own. The version of `e2e-fixtures` actually deployed is the old one. It ignores those fields and seeds the single shared fixture identity instead. CI sets `E2E_RUN_KEY` per job attempt, so the spec derives a run-scoped email, asks the old function to seed it, receives a shared-identity seed instead, then tries to sign in as a user that was never created. The sign-in never resolves and the wait times out. That is the failure, and it is deterministic rather than flaky, which matches three failed attempts rather than an intermittent pass. What unblocks itDeploy `supabase/functions/e2e-fixtures` first, then re-run this job. The two must land in that order. Deployment is currently blocked on repository configuration rather than on code: there is no `SUPABASE_ACCESS_TOKEN` available to CI, no linked Supabase CLI in this environment, and the MCP server is unauthenticated. The durable fix is to add that secret and give CI a deploy step for edge functions, since functions being outside CI entirely is what let a client and its server drift apart in the first place. Not proposing a client-side fallback that tolerates the old function. That would mask the version skew rather than fix it, and it would quietly restore the shared-fixture collision surface this PR exists to remove. |
…function The Web E2E job seeded its fixture identity by POSTing to a deployed `e2e-fixtures` Supabase Edge Function. That put the caller and the seeder on independent release cycles: the caller shipped with its pull request, while the seeder only changed when somebody remembered to run `supabase functions deploy`. This branch's caller started sending the exact run-scoped identity it wanted seeded, the deployed copy still derived its own, and the specs signed in as a user that was never created. `auth-shell.spec.ts` and `profile-completion.spec.ts` then timed out waiting for a URL they could never reach. Seeding now runs in the Node process the specs already spawn from `beforeEach`, against the Supabase admin API with the service-role key. The client versus deployed-server skew class is gone: there is one seeding implementation, in the repository, versioned with its caller. Changes: - `apps/web-console/tests/e2e/support/e2e-fixture-seed.mjs` (new) holds all service-role work, ported from the edge function: the three fixture users, tenants, accounts, memberships, profiles, the pending invitation, and the stale-run sweep. It also keeps the run-key derivation this branch added, now in the only place that derives it. - `e2e-auth-fixtures.mjs` resolves the credentials and passes them straight to the seeder, so one process decides the identity and writes it. It also gained a `reset-profile <email>` action so the CLI is the single entrypoint. - `reset-profile.ts` shells out to that action instead of POSTing to the edge function. Shelling out rather than importing avoids Playwright's CommonJS transform choking on a .mjs import, and keeps the key out of the Playwright worker's module graph. - Seeding without `SUPABASE_URL` and `SUPABASE_SERVICE_ROLE_KEY` now fails and names the missing variable. It used to silently no-op, which is what let the skew hide until a 25 second sign-in timeout. - `supabase/functions/e2e-fixtures/` is deleted. Nothing else called it, and leaving a second seeding implementation in the tree is what drifted. - `ci.yml` drops `E2E_FIXTURE_URL` and `E2E_FIXTURE_SECRET` from the Playwright step and scopes `SUPABASE_SERVICE_ROLE_KEY` to it instead. The key stays out of job-level env, matching the other two steps that need admin auth. The repository secrets themselves are left in place. - Passwords now come from the same `E2E_VERIFIED_PASSWORD` and `E2E_UNVERIFIED_PASSWORD` the specs read. The edge function read a separate `E2E_DEFAULT_*` pair that CI never set, which was a second way for the two sides to disagree. - `tests/unit/e2e-fixture-ids.test.ts` (new) covers the run-key derivation and the secret redaction that were previously unreachable from any test in this repository.
sakibsadmanshajib
changed the title
3 of 4 tasks
sakibsadmanshajib
added a commit
that referenced
this pull request
Jul 30, 2026
resetProfilesAndInvitation deletes account_billing_profiles rows in a loop with no paired upsert right after, the same shape of delete-then-write window PR #583 fixed for account_memberships and account_invitations. This one was left open in #592 for its own look. Verified it is not a bug: GetBillingProfile LEFT JOINs account_billing_profiles and COALESCEs every column, so a missing row reads back blank rather than ErrNotFound. There is also no concurrent reader of the window: every spec's beforeEach shells out to the fixture CLI via execFileSync, which blocks synchronously until the child exits, and playwright.config.ts pins workers to 1. Account ids are namespaced per E2E_RUN_KEY so two CI jobs never touch the same row either. supabase/functions/e2e-fixtures/index.ts, the other file the issue asked to audit, no longer exists: seeding moved in-process in PR #583. Documented the reasoning as an inline comment so the dismissal survives instead of living only in a PR body, and logged it to .wolf/buglog.jsonl.
sakibsadmanshajib
added a commit
that referenced
this pull request
Jul 30, 2026
…#607) ## Summary Closes #592. The issue asked for an audit of `apps/web-console/tests/e2e/support` and `supabase/functions/e2e-fixtures/index.ts` for any remaining un-paired delete (the same shape of bug PR #583 fixed for `account_memberships` and `account_invitations`), and a decision on whether the `account_billing_profiles` delete in `resetProfilesAndInvitation` needs a fix. Audit result: it does not need a fix. - `supabase/functions/e2e-fixtures/index.ts` no longer exists. Seeding moved in-process in PR #583, so there is nothing left to audit there. - The only remaining un-paired delete in `apps/web-console/tests/e2e/support` is the `account_billing_profiles` loop. `GetBillingProfile` (`apps/control-plane/internal/profiles/repository.go`) LEFT JOINs both `account_profiles` and `account_billing_profiles` and COALESCEs every selected column, so a missing row after the delete reads back as blank fields, never `ErrNotFound`. There is nothing here for a read to observe as broken. - There is also no concurrent reader of the window in the first place: every spec's `beforeEach` calls the fixture CLI through `execFileSync` (confirmed in `auth-shell.spec.ts`, `profile-completion.spec.ts`, and five other specs), which blocks the calling test synchronously until the child process exits, and `playwright.config.ts` pins `workers: 1`. No page load can land inside `resetProfilesAndInvitation`'s execution window, in this run or a concurrent one, and `buildIds(runKey)` namespaces every account id per `E2E_RUN_KEY` so two CI jobs never touch the same row anyway. Change is a documentation-only PR: added an inline comment above the delete loop recording this reasoning and referencing #592 directly, so it does not get forgotten by living only in a PR body, and logged the same conclusion to `.wolf/buglog.jsonl`. No functional code was touched. An unrelated, separately real bug was found while reading this code during the audit (`account_memberships.created_at` ties within a single bulk upsert make `EnsureViewerContext`'s default-workspace pick vary from reseed to reseed, since the Go-side tiebreak added by PR #583 sorts on a fresh random `id` per run rather than the array's intended priority order). That is a different bug class (a same-timestamp sort tie, not an un-paired delete) and outside what #592 asked for, so it was deliberately left out of this PR to keep the diff scoped to the confirmed defect, matching this issue's own precedent. Flagging it for a follow-up issue rather than bundling it here. ## Test plan - [x] `node --check apps/web-console/tests/e2e/support/e2e-fixture-seed.mjs` passes. - [x] Verified `GetBillingProfile`'s LEFT JOIN + COALESCE behavior by reading `apps/control-plane/internal/profiles/repository.go` directly. - [x] Verified `playwright.config.ts` pins `workers: 1` and every credentialed spec's `beforeEach` calls the fixture CLI via `execFileSync`. - [ ] Full E2E suite not run: no Hive core stack (control-plane/edge-api/Supabase) was up in this environment, and this repo shares one 15-client session-mode Supabase pool across CI, agents, and the live chat surface, so a stack was not started for a documentation-only change with no functional diff.
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 12, 2026
#880) Refs #879. **Deliberately not `Closes`**: that issue tracks the rotation of the exposed credentials, which has not happened, and an auto-close on merge would mark an open exposure as resolved. ## This does not un-expose anything The three values removed here remain in this repository's git history, and this repository is public. They must be treated as compromised regardless of what HEAD looks like. **Rotation is the actual remedy**, it is tracked separately in #879, and it is deliberately not done in this pull request: several agents are running live tests against these accounts right now and a rotation would revoke every one of their sessions. Nothing below closes the exposure. It stops the exposure from being recreated, and stops the code that kept writing those values back onto live accounts. ## What was exposed `apps/web-console/tests/e2e/support/e2e-auth-defaults.json` carried `verifiedPassword`, `unverifiedPassword` and `invitationToken` in plaintext, on `main`, for `e2e-verified@scubed.com.bd` and `e2e-unverified@scubed.com.bd`. Both accounts hold a tenant OWNER role and a billing account on the live Supabase project, and the fixture seeder writes those exact values back onto them on every run with no environment overrides. No value appears anywhere in this pull request. ## Changes **1. No credential has a committed default.** `e2e-auth-defaults.json` now holds the two addresses and the two length limits only. Both readers, `e2e-auth-creds.ts` and `e2e-auth-fixtures.mjs`, take the three credentials from the environment through a `requiredSecretEnv` helper that throws, names the variable, and points at `docs/live-test-auth.md`. No fallback, no silent skip, since a silent skip is how a committed value survives unnoticed. In the CLI the three are resolved inside `prepareE2EAuthFixtures` rather than at module scope, so the `reset-profile` action, which needs no credential, still runs without them. **1b. No fixture address is a shared one.** Removing the committed defaults was not sufficient, and on its own it made things worse. The seeder fell back to the two shared live addresses whenever no run key was set, and `ensureUser` sends `password:` on both update paths, so a developer following the README and running the documented command would overwrite a shared tenant-OWNER account's password and revoke every concurrent session. The committed constant had at least made that write idempotent. `runScopedEmail` now throws without `E2E_RUN_KEY` and otherwise derives this run's own address from the shared base, idempotently, at both resolution points, so the account a spec signs in as and the account the seeder writes are the same one and it belongs to this run. `DEFAULT_EMAILS` is deleted. This is also the only route by which the newly minted CI secret values could have reached the two exposed accounts. `E2E_VERIFIED_PASSWORD` and `E2E_UNVERIFIED_PASSWORD` did not exist as repository secrets, which is why CI was silently using the committed values for 109 days, and for roughly the first 95 of those, before run-scoped addresses landed in #583, every CI run wrote the published password back onto the shared live accounts. Both secrets are now set. Their values were generated randomly, never displayed, and are only ever written to the run-scoped throwaway users that `ci.yml` already creates per job attempt, so no existing account was touched. **2. No script rotates a shared account any more.** `scripts/seed-owui-e2e-user.py` gets `password_to_set`, the same helper and semantics as the one already in `scripts/seed-demo-owner.py`: an existing account is left alone unless the caller explicitly supplies a password. It prints a `PASSWORD` line only for a password it actually set, and names on stderr why a line is missing. To keep the nightly working under that rule without a hand-managed secret, `OWUI_E2E_RUN_KEY` namespaces both fixture addresses per job attempt, so every run provisions its own users and shares no credential with any other run. Same shape as `E2E_RUN_KEY` in `ci.yml`. The workflow passes `github.run_id`-`github.run_attempt`. `scripts/verify-rag-roundtrip.py` had the same unconditional rotation against its own hardcoded shared address. It now signs in with `RAG_VERIFY_PASSWORD` and refuses to rotate, failing by name instead. Only a first run, which creates the account, generates a password. Both guards are pinned by unit tests in `scripts/test_seed_owui_e2e_user.py`, which already runs in `make test-scripts`. A first version of this claimed a run "shares no credential with any other run". That was false in two places, both now fixed. The run key namespaced the GoTrue users but not the shim billing account, so every run still issued `DELETE /api_keys` against that shared account for every key but its own, and two overlapping runs revoked each other mid-flight (the outage in `.wolf/cerebrum.md`). That delete is now bounded by age, so a key minted minutes ago survives. The account stays shared deliberately: a tenant bills to exactly one account, so a per-run account would need a per-run tenant and leave a permanent row behind every night. Second, run-scoped users leaked two per nightly, one an OWNER of the `owui-e2e` tenant; `sweep_stale_fixture_users` now clears what earlier runs left, matching on both halves of the address, never touching the shared base address or this run's own users, and never failing a run. `verify-rag-roundtrip.py` generated a password on the create path and never printed it, which would have left every later run demanding a value no operator could obtain, and `DEMO.md`'s RAG proof dead on every environment it had already run against. It prints it once now, on stderr. **3. Artifacts no longer carry live session material for 90 days.** Five upload steps, not two. `playwright-report-api` and `owui-playwright-report` exclude `*.zip`, `*.webm` and `index.html`, and set `retention-days: 5`. `index.html` had to go for the same reason as the traces: the HTML reporter base64-inlines the whole report payload into it, stdout, stderr and error text included, and a `waitForURL` timeout enumerates every URL it navigated, fragment and all. The three container-log artifacts (`compose-logs`, `compose-logs-web-e2e-api`, `owui-compose-logs`) now pipe through `scripts/redact-log-credentials.py` and retain for five days; Open WebUI's uvicorn access lines print `GET /oauth/oidc/callback?code=...` verbatim. That redactor mirrors `redactSecrets`, understands fragments as well as query strings, and carries a self-check wired into `make test-scripts`. Traces hold every request header and cookie, and for the OWUI suite the OAuth callback URLs with `code=` and `access_token=` in both the query string and the fragment. A trace is a zip of binary resources, so no text linter can scrub it and exclusion is the only reliable answer. Screenshots still upload, and the failing test names and error text are already in the job log. Review caught that the screenshot half of that was false when first written: `playwright.config.ts` never set `screenshot`, so it kept Playwright's default of `off` and the three exclusions left the `playwright-report-api` artifact empty while `if-no-files-found: ignore` kept it quiet. It now sets `screenshot: "only-on-failure"`, which captures the viewport only, so no URL bar, no request headers and no cookies. The `ci.yml` change stays confined to the upload steps, the log dumps, the token generation step and comments, to rebase cleanly against #805, #811 and #822. **4. The sweeper's three `console.error` calls now go through `redactSecrets`.** They relayed raw client-library messages to stderr, which is exactly where the service-role key ends up embedded, while the module header two hundred lines above declared the opposite contract. **5. `docs/live-test-auth.md` is now true.** It named only an out-of-repo scratch script while three in-tree paths broke the rule it stated. It now has a "no credential is ever committed" rule with the environment-variable pattern as the sanctioned alternative, a table of what each of the three scripts does after this change, a note on why the automatically-invoked one matters most, and the artifact rule. `README.md` no longer advertises fallbacks that do not exist. ## How absence fails now | path | with the credential unset | | --- | --- | | Playwright specs | Throws at import: `E2E_VERIFIED_PASSWORD is required and has no default...` | | Fixture seeding CLI | Same error before any admin call; `reset-profile` still works | | `seed-owui-e2e-user.py` | Existing account keeps its password, no `PASSWORD` line, stderr names `OWUI_E2E_RUN_KEY` and `OWUI_E2E_PASSWORD`; the nightly's own guard then fails the job loudly | | `verify-rag-roundtrip.py` | Exits non-zero naming `RAG_VERIFY_PASSWORD` | ## Verification The first push of this branch left a required check RED while this section claimed otherwise. That is corrected here, and the check now passes. * `node scripts/verify-spec-collection.mjs` in the web-console container: OK, 36 files across 2 configs. It failed ten times over at head `dec64e84` because it runs `playwright test --list` in a job holding no E2E secrets. * `docker compose run --build web-console npm run test:unit`: 488 passed, 1 skipped, and one suite fails: `tests/unit/control-plane-host.test.ts` on `ENOENT /app/.env.example`. That one is pre-existing and unrelated. The web-console image contains only `apps`, so that suite cannot read a repository-root file in this container at all, on this branch or on main. * New guards in `tests/unit/e2e-auth-creds.test.ts`, 4 passed: the defaults file must hold only the four non-secret fields, every string in it must look like an address rather than a secret, a missing credential must throw by name, and `runScopedEmail` must refuse an empty run key while namespacing idempotently. * `python3 scripts/test_seed_owui_e2e_user.py`, `python3 scripts/test_seed_demo_owner.py`, `python3 scripts/redact-log-credentials.py --selfcheck`: all pass, including the five new cases. * Both workflow files parse, and the rewritten nightly seed step passes `bash -n` plus a simulation proving the guard is reached rather than pre-empted by `set -e`. * `Web E2E (full stack)` now passes at this head: 33 collected, 27 executed, 0 failed, 0 passed only on retry. That is the first end-to-end exercise of the new shape, and it covers the required secrets, the run-scoped addresses and the randomly generated invitation token against the live Supabase project. Its first attempt failed, and not on anything in this change: the stack never started, because control-plane could not reach the database (`EMAXCONNSESSION ... max clients are limited to pool_size: 15`), the known shared session-pool ceiling. A re-run of the same commit passed. All 14 checks are green. ## One deliberate deviation from the review Review item 1 asked for the three credentials to resolve lazily. They still resolve at module scope, and the required check is green anyway, because `scripts/verify-spec-collection.mjs` now supplies placeholder values for its `--list` pass. That guard already does exactly this for the OWUI config, for the same reason: listing loads spec modules and never dials out, so it needs the variables present but not real. The reason for keeping module scope is that lazy resolution weakens the guarantee. Every gated spec computes `HAS_CREDS` at module scope, so a lazily resolved empty value turns a missing credential into `test.skip`, which is the silent skip this whole change exists to remove. Throwing at load keeps the failure loud and immediate. If the reviewer still prefers lazy resolution, the change is small, but it needs a run-level assertion to replace what the throw currently guarantees. ## Stage 6 adversarial review, run on this PR Streams: CodeRabbit CLI (**SKIPPED**, `Review failed: A valid organization session is required`, HTTP 401 `UNAUTHORIZED`, on both the default and `--light` paths; its absence is not a pass, and the CodeRabbit GitHub App check is not a substitute since it reported `Review rate limited` earlier in this PR's life), `ecc:code-review`, a plain adversarial pass, `typescript-reviewer`, `python-reviewer`, `security-reviewer` (mandatory, credential and auth path) and `/codex:adversarial-review`. Eight inline comments posted, seven fixed in `190d2479`, one rebutted and upheld, all threads resolved. The claim gating the credential rotation was the primary target, and it now has a runnable proof rather than an argument. `seedFixtures` is driven in a unit test by an admin client that throws on any property access, so an empty, whitespace, `undefined` or non-string run key must reject with the `E2E_RUN_KEY` error rather than the proxy's, which is what proves no admin call happens before the guard. The same stream found the one shared-account write the claim did not cover: `reset-profile` takes an address off argv, and it now refuses anything not scoped to this run. Other fixes: both `runScopedEmail` implementations are pinned as equivalent by test, the log redactor was blind to environment-variable spelling (`\b` treats `_` as a word character, so `RAG_VERIFY_PASSWORD=` did not match), the user sweep's single-page ceiling is now stated, and the invitation token's length floor is documented where it is generated. ## Review round one An independent review returned do-not-merge on the first push, with nine findings. All nine are addressed above and in the commits on this branch: the red required check (item 1), the shared-account write that this change had made worse (item 2), the auto-closing issue reference (item 3), `index.html` (item 4), the three missed upload steps (item 5), the unobtainable RAG password (item 6), the shim key revocation (item 7), the derivable invitation token (item 8), and the fixture user leak plus the nightly guard that could never fire (item 9). The reviewer's sweep of all 1898 tracked files found no committed secret material other than item 8. ## Buglog entry To be appended to `.wolf/buglog.jsonl` on `main` in a separate buglog-only pull request once this merges, per `.claude/rules/openwolf.md`: ```json {"id":"BUG-2026-08-11-committed-e2e-credentials","date":"2026-08-11","title":"Live E2E credentials committed in plaintext in a public repo, and re-seeded onto the accounts on every run","error_message":"No error. The suite passed, which is the problem: e2e-auth-defaults.json carried verifiedPassword, unverifiedPassword and invitationToken for two live tenant-OWNER accounts, and CI referenced E2E_VERIFIED_PASSWORD and E2E_UNVERIFIED_PASSWORD secrets that did not exist, so the empty string fell through to the committed values.","root_cause":"envOrDefault treated an unset credential as a request for the committed fallback. A fallback makes the absence of a secret invisible, so nothing ever failed and the values stayed live for months while the seeder wrote them back onto the accounts through the admin API on every credential-less run. Two sibling scripts copied a related pattern, rotating hardcoded shared accounts unconditionally, one of them from a scheduled workflow whose concurrency group does not join a labelled-PR run to the scheduled run.","fix":"Removed the three fields; both readers now use requiredSecretEnv, which throws and names the variable with no fallback and no skip. Set the two missing CI secrets. seed-owui-e2e-user.py takes password_to_set from seed-demo-owner.py and gains OWUI_E2E_RUN_KEY so the nightly provisions its own users instead of rotating shared ones; verify-rag-roundtrip.py takes RAG_VERIFY_PASSWORD and refuses to rotate. Both Playwright artifact uploads exclude traces and videos and retain for 5 days instead of 90. The fixture sweeper's three console.error calls now go through redactSecrets. docs/live-test-auth.md and README.md corrected. Review of the first push found the removal alone had made the shared-account hazard worse: the seeder still fell back to the two shared addresses without a run key, and ensureUser writes a password on both update paths, so each operator would now write a DIFFERENT value where the committed constant had at least been idempotent. runScopedEmail closes that by refusing an empty run key and namespacing every fixture address. Same round: index.html base64-inlines the report payload so excluding traces alone did not stop the leak, three container-log artifacts were missed, the shim-key delete still revoked concurrent runs until bounded by age, and the invitation token was a committed literal plus a public run id.","tags":["security","credentials","e2e","ci","public-repo","gotrue","artifacts"],"pr":880,"issue":879} ``` Note: the values remain in git history. This entry is not a record of a closed exposure. Rotation is tracked in #879.
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.
Summary
Web E2E (full stack)flaked, then began failing deterministically on this branch. Two separate defects, both now fixed, plus the structural reason the second one was able to hide.1. Non-atomic seed (root cause of the original flake)
seedAccountsAndMembershipsdeleted six(account_id, user_id)pairs fromaccount_memberships, then upserted five of them back in a second, separate call. Every one of those five had a real window with zero memberships. A read landing in that window (this job's own, or a concurrent job's, since every job used the same hardcoded ids) madeEnsureViewerContextcallprovisionDefaultWorkspace, handing the signed-in user an unrelated workspace. Confirmed from the failing run's page-snapshot artifact:app/console/error.tsxrendered, an exact text match, meaninggetViewerorgetAccountProfilethrew server side.Fix: delete only the one pair that must stay absent (the unconsumed invitation) and upsert the other five with no delete at all. The same delete-then-upsert shape in the invitation reset is fixed the same way.
2. Cross-run collision
Seeding used one hardcoded set of account, user, tenant, and invitation ids for every caller. Multiple CI jobs run concurrently against the same live Supabase project and were proven by timestamp to overlap the failing run's execution window. Two jobs sharing rows can stomp each other regardless of how atomic each job's own seed is.
Fix: every id and email is derived from an optional run key.
buildIds("")returns the original hardcoded identity unchanged, so local and manual runs are unaffected. With a run key, account, tenant, and invitation ids are deterministically hashed and emails get a+runKeytag so GoTrue creates genuinely distinct users.accounts.slugandtenants.slugare bothUNIQUE, so both get a short deterministic suffix too. CI sets the run key to${{ github.run_id }}-${{ github.run_attempt }}, withrun_attemptincluded on purpose: a rerun of the same run id, which is exactly how a flake gets verified, must not reuse a spent identity.Cleanup rides along on the seed call, sweeping namespaced rows older than three hours, so no separate CI step or schedule is needed. It is scoped to accounts with an
e2e-slug and a+in the profile's login email, both required, so it cannot touch a real customer account (a+Gmail-style address is common enough in the wild that the email check alone would not be safe) or the shared local identity, which has no run key and can be arbitrarily old. The account is deleted before its owner becauseaccounts.owner_user_idhas noON DELETE CASCADE.3. The structural problem: a deployed seeder the repository could not update
Seeding used to POST to a deployed
e2e-fixturesSupabase Edge Function. That put the caller and the seeder on independent release cycles. The caller shipped with its pull request; the seeder only changed when somebody remembered to runsupabase functions deploy. Supabase functions are not deployed by CI anywhere in this repository.That is what turned fix 2 into a hard failure. This branch's caller began sending the exact run-scoped identity it wanted seeded, the deployed copy still derived its own, and the specs signed in as a user that was never created.
auth-shell.spec.ts:50andprofile-completion.spec.ts:52timed out at 25 seconds waiting for a URL they could never reach, on all three attempts, while the same job passed onmain.Seeding now runs in the Node process the specs already spawn from
beforeEach, against the Supabase admin API. There is one seeding implementation, in the repository, versioned with its caller. The deploy step is off the critical path of every pull request that touches fixtures, and this whole skew class is gone rather than worked around.What changed
apps/web-console/tests/e2e/support/e2e-fixture-seed.mjs(new) holds every service-role mutation, ported from the edge function: the three fixture users, tenants, accounts, memberships, profiles, the pending invitation, and the stale-run sweep. The run-key derivation lives here, in the only place that derives it.e2e-auth-fixtures.mjsresolves the credentials and hands them straight to the seeder, so one process decides the identity and writes it. It gained areset-profile <email>action so the CLI is the single entrypoint.reset-profile.tsshells out to that action. Shelling out rather than importing avoids Playwright's CommonJS transform choking on a.mjsimport, and keeps the service-role key out of the Playwright worker's own module graph.SUPABASE_URLandSUPABASE_SERVICE_ROLE_KEYnow fails and names the missing variable. The old code quietly no-opped when the edge function's URL and secret were absent, which is precisely what let the skew hide until a sign-in timeout.supabase/functions/e2e-fixtures/is deleted. Nothing else called it (verified by grep across the repository: only its own README referenced it). Leaving a second seeding implementation in the tree is what drifted in the first place.ci.ymldropsE2E_FIXTURE_URLandE2E_FIXTURE_SECRETfrom the Playwright step and scopesSUPABASE_SERVICE_ROLE_KEYto that step instead. The key stays out of job-level env, matching the two other steps in this job that need admin auth. The repository secrets themselves are untouched.E2E_VERIFIED_PASSWORDandE2E_UNVERIFIED_PASSWORDthe specs read. The edge function read a separateE2E_DEFAULT_*pair that CI never set, a second way for the two sides to disagree.tests/unit/e2e-fixture-ids.test.ts(new) covers the run-key derivation and the secret redaction, neither of which any test in this repository could reach while they lived in a Deno function.Service-role key handling
ci.yml, never job-level, so no other step in the job inherits it.redactSecretsfirst, which replaces the key with<redacted>. Unit tested.Verification
npx vitest run tests/unit/e2e-fixture-ids.test.ts: 7 passed. Covers local versus run-key identity, determinism, full disjointness between two run keys, uuid shape, email tagging, and redaction.npx tsc --noEmit --listFilesconfirms all four changed or added TypeScript and.mjsfiles are in the type-check program, with no errors from any of them.SUPABASE_URLandSUPABASE_SERVICE_ROLE_KEYunset, bothresetandreset-profileexit 1 and name both missing variables. An unknown action exits 1.ci.ymlparsed withyaml.safe_load; assertedSUPABASE_SERVICE_ROLE_KEYis present on the Playwright step'senvand absent from the job'senv.Web E2E (full stack)on this branch, which is the job the deployed-function skew was breaking.Test plan
Web E2E (full stack)green on this branch: run 30391716754, 14 passed and 2 skipped, first attempt, no retries.auth-shell.spec.ts:50andprofile-completion.spec.ts:52, the two tests this PR was failing on all three attempts, both pass.ed69e4f3: Go tests (edge-api, control-plane, storage, agent-engine), Repo policy lints (tenant + audit), Web console (type + unit + build).Desktop app (Rust + TS)andRust tests (desktop-sandbox)also passed. The 2 skipped specs areconsole-platform-admin, gated onE2E_PLATFORM_ADMIN_*secrets, unchanged by this PR.