CI on the escape for every job, the nightly cold clone, and e2e through the lookup - #1227
Conversation
…gh the lookup The `test` job's `DATABASE_URL` moves from four individual steps to the job, so the browser-test step and every other step that runs tests carry it too and an escape-only suite can no longer read as coverage while silently skipping. The `e2e` job's copies are hoisted the same way. Nightly gains a cold-clone job: `create` then `dev` on a runner with no database installed, deliberately without `DATABASE_URL` so the Dev database sidecar is what the app is served by. The published-npm and examples-build jobs are gated to `main`, whose published Prisma 7 scaffold they exercise. The e2e global setup and its seed helper now reach the database through core's lookup: `DATABASE_URL` (the CI container) or, with nothing set, a Dev database started under the project's Generated bundle, which `generate`, `prisma db update` and the Next server all read back from the state file. Implements #1160. Part of #1125. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Coverage Report for Core Package Coverage (./packages/core)
File CoverageNo changed files found. |
Coverage Report for UI Package Coverage (./packages/ui)
File CoverageNo changed files found. |
Coverage Report for CLI Package Coverage (./packages/cli)
File CoverageNo changed files found. |
Coverage Report for Auth Package Coverage (./packages/auth)
File CoverageNo changed files found. |
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Coverage Report for RAG Package Coverage (./packages/rag)
File CoverageNo changed files found. |
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
| const existing = findDatabaseConnection({ cwd: projectDir }) | ||
| if (existing === undefined) { | ||
| started = await startDevDatabase({ | ||
| dataDir: path.join(projectDir, '.opensaas', 'dev-db'), | ||
| extensions: ['vector'], | ||
| cwd: projectDir, | ||
| }) | ||
| } catch (error) { | ||
| console.error('Failed to generate schema') | ||
| throw error | ||
| } |
There was a problem hiding this comment.
The Generated bundle directory is not created before PGlite opens its data directory.
startDevDatabase({ dataDir: <projectDir>/.opensaas/dev-db }) is called here as the first thing setupDatabase does — before generate runs. packages/cli/src/commands/dev.ts guards exactly this case immediately before its own call:
// PGlite's own `mkdir` of the data directory is not recursive, so the
// Generated bundle directory has to exist before it runs.
const dataDir = path.join(cwd, DEV_DATABASE_DIR)
fs.mkdirSync(path.dirname(dataDir), { recursive: true })This caller has no such guard. .opensaas/ is gitignored, so on a clean checkout examples/starter-auth/.opensaas does not exist, and a local pnpm test:e2e with no DATABASE_URL set dies in globalSetup with PGlite's ENOENT before generate ever gets a chance to create the bundle directory.
| const existing = findDatabaseConnection({ cwd: projectDir }) | |
| if (existing === undefined) { | |
| started = await startDevDatabase({ | |
| dataDir: path.join(projectDir, '.opensaas', 'dev-db'), | |
| extensions: ['vector'], | |
| cwd: projectDir, | |
| }) | |
| } catch (error) { | |
| console.error('Failed to generate schema') | |
| throw error | |
| } | |
| const existing = findDatabaseConnection({ cwd: projectDir }) | |
| if (existing === undefined) { | |
| const dataDir = path.join(projectDir, '.opensaas', 'dev-db') | |
| // PGlite's own `mkdir` of the data directory is not recursive. | |
| fs.mkdirSync(path.dirname(dataDir), { recursive: true }) | |
| started = await startDevDatabase({ | |
| dataDir, | |
| extensions: ['vector'], | |
| cwd: projectDir, | |
| }) | |
| } |
(plus import * as fs from 'node:fs')
| /** Stops the Dev database, if this process started one. */ | ||
| export async function cleanupDatabase(): Promise<void> { | ||
| await started?.stop() | ||
| started = undefined | ||
| } |
There was a problem hiding this comment.
The e2e run no longer starts from a clean database, and nothing ever removes the persisted one.
The old helper deleted dev.db and re-pushed the schema on every run; cleanupDatabase deleted it again afterwards. The replacement resets nothing: setupDatabase only reconciles the schema, and cleanupDatabase stops the sidecar but leaves examples/starter-auth/.opensaas/dev-db (a persistent dataDir) on disk with all its rows.
Concrete failure: Post.slug is isIndexed: 'unique' in examples/starter-auth/opensaas.config.ts, and the specs create posts under fixed slugs — apples/bananas/draft-one/published-one (05-filter.spec.ts, 06-filter-builder.spec.ts), unique-slug (02-posts-access-control.spec.ts), test-post/full-post/edit-test-post (03-admin-ui.spec.ts). On the second local pnpm test:e2e every one of those creates hits a unique violation against rows the previous run left behind. Because waitForURL(/admin\/post/) also matches /admin/post/create, the failed create does not fail the test — the assertions then pass against the stale rows, so these specs stop testing what they claim to and only break later, opaquely.
CI is masked from this because the service container is fresh per job; only local runs accumulate. Either rmSync the dev-db data dir in cleanupDatabase, or drop/recreate the schema in setupDatabase.
| # published, so the workspace is the only source of the toolchain. | ||
| cold-clone: | ||
| name: Cold clone — create then dev | ||
| if: github.ref_name != 'main' |
There was a problem hiding this comment.
github.ref_name != 'main' means the nightly cold clone never runs on the nightly.
A schedule trigger always fires on the repository's default branch, so for the 03:17 cron github.ref_name is main and this job is skipped every single night. The only way it executes is a manual workflow_dispatch explicitly targeting prisma-8 — and once prisma-8 merges into main, ref_name is main for every trigger and the job is dead permanently. The two jobs above it (published-create-e2e, examples-build) are gated == 'main', so they do run nightly; this one is the odd one out.
Since the workflow file on prisma-8 is only ever used when dispatched on prisma-8, the branch discrimination isn't buying anything here — the job can just run unconditionally on this branch, and the gate re-added (as an == 'main'-compatible condition) when the branch lands.
| env: | ||
| DATABASE_URL: 'file:./dev.db' | ||
| BETTER_AUTH_URL: 'http://localhost:3000' | ||
| BETTER_AUTH_SECRET: 'secret-for-teating-in-github-actions-with-numbers1234' | ||
| NEXT_PUBLIC_APP_URL: 'http://localhost:3000' |
There was a problem hiding this comment.
This DATABASE_URL is now incompatible with the harness the same PR rewrote, and nothing in CI catches it.
e2e/utils/db.ts now calls findDatabaseConnection(), startDevDatabase() (PGlite) and prisma db update. file:./dev.db is a set value, so the lookup returns provenance: 'env' and the harness:
- never starts the Dev database (so the
'dev-database'branch the rewrite exists to exercise is unreachable in CI), and - runs
prisma db updateagainst a SQLite URL, which the Prisma 8 toolchain cannot use —packages/core/src/testing/escape.tscalls out this exact value ("a stale SQLiteDATABASE_URLin CI") as the misconfiguration it refuses.
Combined with if: github.base_ref == 'main' on line 149, the e2e job is skipped for every PR into prisma-8, so none of the rewritten e2e/utils/db.ts, global-setup.ts or global-teardown.ts is executed by any CI job on this branch — the rewrite lands entirely unverified, and the first run that does exercise it (spec 9, or the merge to main) will hit both problems at once. Worth at least dropping DATABASE_URL from this block now so the harness takes its intended branch, rather than leaving a value that is wrong for the code it feeds.
| function writeEnvFile(exampleDir: string): void { | ||
| fs.writeFileSync( | ||
| path.join(exampleDir, '.env'), | ||
| [ | ||
| 'BETTER_AUTH_SECRET="test-secret-key-for-e2e-tests-only-not-for-production-use"', | ||
| 'BETTER_AUTH_URL="http://localhost:3000"', | ||
| 'NEXT_PUBLIC_APP_URL="http://localhost:3000"', | ||
| '', | ||
| ].join('\n'), | ||
| 'utf8', | ||
| ) | ||
| } |
There was a problem hiding this comment.
This now clobbers a developer's local .env unconditionally.
The old code was guarded by if (!fs.existsSync(envPath)) — it only ever created a missing .env. This version writes over whatever is there on every pnpm test:e2e, with no backup and no warning. A contributor who put real OAuth client secrets, an OPENAI_API_KEY, or a DATABASE_URL pointing at their own Postgres into examples/starter-auth/.env loses all of it the first time they run the e2e suite.
The DATABASE_URL case is the sharpest: the harness deliberately wants that variable absent, so a developer who set it to their own server gets it silently deleted and gets a PGlite sidecar started instead — the opposite of the escape the file's own docblock describes. Reading the existing file and stripping/merging keys (or refusing to overwrite a .env that has content the harness didn't write) would keep the same guarantee without the data loss.
| sleep 2 | ||
| done | ||
| curl -sf http://localhost:3000 > /dev/null | ||
| test -f .opensaas/dev-db.json |
There was a problem hiding this comment.
As written this assertion cannot pass, because the scaffolder hands dev the escape it is supposed to avoid.
create-opensaas-app still calls writeEnvFile(...) unconditionally (packages/create-opensaas-app/src/index.ts), which emits DATABASE_URL="file:./dev.db" into myapp/.env. devCommand calls loadProjectEnvFile(cwd) before deciding the branch, so findDatabaseConnection({ cwd })?.provenance === 'env' is true, it prints "DATABASE_URL is set: using it, and starting no dev database", and .opensaas/dev-db.json is never written. It will also then try to reconcile a Prisma 8 contract against a SQLite URL.
The PR body notes this is blocked on #1159, which is fair — but flagging it here because the job is red-by-construction rather than merely unverified, and (given the ref_name != 'main' gate above) that redness won't surface anywhere until someone dispatches it by hand. A short-lived unset/sed of DATABASE_URL from the scaffolded .env in the scaffold step would let the job actually prove the sidecar today.
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
High-effort review of the CI env hoisting, the nightly cold-clone job, and the e2e harness rewrite. Six findings are posted as inline comments; this is the summary plus the checks that came back clean.
Blocking
1. cold-clone never runs on the nightly schedule — .github/workflows/nightly.yml:148
if: github.ref_name != 'main'. This workflow fires only on schedule and workflow_dispatch, and a scheduled run always fires on the default branch, so ref_name is always main on the cron. The job is reachable only by a manual dispatch on prisma-8, and once this branch merges to main it is dead permanently — the acceptance criterion it exists to satisfy would then be silently uncovered rather than red.
This also corrects the PR body's framing. The job does not "currently fail" on the nightly; it does not run there at all. Failing loudly is the right posture to merge in — a cold start that cannot start is exactly what the job should report, and disabling it would be worse — but that argument only holds once the gate actually lets the cron reach it. Gate on the content (e.g. run always, or github.event_name == 'workflow_dispatch' || <branch condition on the checked-out ref>), not on ref_name.
2. e2e harness: PGlite data dir is not created — e2e/utils/db.ts:52
startDevDatabase({ dataDir: <projectDir>/.opensaas/dev-db }) runs before generate, with no mkdirSync. packages/cli/src/commands/dev.ts:293 guards this exact case ("PGlite's own mkdir of the data directory is not recursive"), and .opensaas/ is gitignored — a local pnpm test:e2e on a clean checkout with no DATABASE_URL dies in globalSetup with ENOENT.
3. e2e harness: no reset, persistent data dir — e2e/utils/db.ts:77
cleanupDatabase leaves the dev-db data dir on disk and the harness no longer resets. Post.slug is isIndexed: 'unique' and the specs use fixed slugs (apples, unique-slug, test-post), so a second local run hits unique violations — which waitForURL(/admin\/post/) swallows, since it also matches /admin/post/create. The specs then assert against stale rows instead of failing.
Non-blocking but should be addressed
4. The e2e job's DATABASE_URL defeats the new harness — .github/workflows/test.yml:157. Pinning file:./dev.db at job level makes the rewritten harness take the 'env' branch and never exercise the Dev database, running prisma db update against a SQLite URL — the misconfiguration packages/core/src/testing/escape.ts refuses by name. Combined with the job's pre-existing if: github.base_ref == 'main', no CI job on this branch executes the new e2e/ code, so the rewrite lands unverified.
5. writeEnvFile now clobbers — e2e/global-setup.ts:12. Previously create-if-missing; now unconditional overwrite of examples/starter-auth/.env, silently destroying a contributor's local secrets.
6. test -f .opensaas/dev-db.json cannot pass today — .github/workflows/nightly.yml:193. The scaffolder writes DATABASE_URL="file:./dev.db" into the project .env and devCommand loads it before branching, so no sidecar starts. Acknowledged as blocked on #1159 — worth naming in the job comment so the red is legible to whoever sees it first.
Checked and cleared
- Branch discrimination.
test.ymlfires onpull_requestonly (branches: [main, prisma-8]), sogithub.base_refis always populated and both arms of theDATABASE_URLternary are correct. Nopushtrigger, so the "a PR run's ref is not the target branch" trap does not apply here. - Nothing silently changed by inheritance. The three auth guard steps (rate-limit, MCP OAuth cascade, credential read-deny) previously ran with no
DATABASE_URLand now inherit one — but each assignsprocess.env.DATABASE_URLto its own absolute temp path before use and sets its ownBETTER_AUTH_SECRET/BETTER_AUTH_URL, so the hoisted values are overridden, not consumed. No step needed a different value than the one it was flattened to. The fixture-regeneration step does not readDATABASE_URL, andpackages/uibrowser tests do not use the DB harness. - No
DATABASE_URLleaks intocold-clone. No workflow-levelenv, no job-levelenv, novars/secretsreference. The only leak vector is the scaffolder's own.env(finding 6) — which is the failure, not a hidden pass. - The success condition is genuine. The poll breaks only on a successful
curl -sf, exits early with the log if the dev process dies, and re-runscurl -sfunguarded after the loop, so a 120-iteration timeout fails rather than falling through. It proves the app served, not merely that a process started. - Gating
published-create-e2e/examples-buildtomainremoves no coverage. Both are nightly-only, and the cron always runs asmain. They are skipped only on a manual dispatch from another branch, where they could not pass anyway. - Root dependency versions match the CLI's pins exactly —
@electric-sql/pglite0.5.8,pglite-pgvector0.0.9,pglite-socket0.2.11, identical inpackages/cli/package.jsonandpackages/core/package.json.@opensaas/stack-core/@opensaas/stack-cliasworkspace:*root devDeps is the standard pnpm form for root-level tooling; it does not shadow workspace resolution and creates no cycle (the root package is not itself a dependency of any workspace package). - No changeset needed. The diff touches only
.github/workflows/{nightly,test}.yml,e2e/{global-setup,global-teardown,utils/db}.ts,package.jsonandpnpm-lock.yaml. Nopackages/*source file is modified — the author's claim holds. - No scope collision with #1228. No
examples/**file,README, or.env.exampleis touched. - Repo rules. No
anyand no type casting in the changed TypeScript. The YAML comments carry why the gates and pins exist (ADR references, issue numbers) rather than restating the lines — they earn their place. - Teardown. Ordering does stop the sidecar even when
globalSetupthrows; no leaked in-process PGlite on the failure path.
…non-destructive .env - `cold-clone` loses its `github.ref_name != 'main'` gate. A scheduled run always fires on the default branch, so that gate skipped the job on every cron and would have been permanently dead once this lands on `main`. It is expected to fail until #1129 converts `examples/starter`; that redness is the point. - The e2e harness creates the Generated bundle directory before PGlite opens its data directory, matching `dev.ts`, and discards the persistent data directory first so a second local run does not inherit the previous run's rows. - Post-submit `waitForURL(/admin\/post/)` is anchored to `/\/admin\/post$/`, so a create that failed and stayed on `/admin/post/create` fails the wait instead of passing against stale rows. - The `e2e` job no longer pins a SQLite `DATABASE_URL`, so the harness takes the Dev database branch it exists to exercise. - `writeEnvFile` appends only the variables that are missing, leaving a contributor's own `.env` intact. - The nightly's `pnpm db:push` / `test -f dev.db` are dropped: neither exists under Prisma 8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review findings addressed in f7374d6. 1 (blocking) — Proven with a real dispatch on this branch: https://github.com/OpenSaasAU/stack/actions/runs/34060337801 — 2 (blocking) — PGlite data dir. 3 (blocking) — no reset on a persistent data dir. The URL assertion is tightened too: every post-submit 4 — the You are right that no CI job on this branch executes the new 5 — 6 — stale Not touched, per scope:
|
1 participant
Implements #1160. Part of #1125.
Already in place before this PR
The
testjob's service container was already a pgvector-capable Postgres 17 (pgvector/pgvector@sha256:cf134a…, pinned by digest), and the four integration-branch test steps already setDATABASE_URLat it — both landed with #1157. The plain-Node anchor already imports.opensaas/context.ts(packages/cli/tests/bundle-node-load.test.ts, from #1138); nothing to re-target.What changed
.github/workflows/test.yml—DATABASE_URLmoves from four steps to thetestjob, so the step that was missing it (Run browser tests with coverage) and every future step carry it. The value stays branch-discriminated:file:./dev.dbfor PRs intomain, the service container for the integration branch. Thee2ejob's two step-level copies (and itsBETTER_AUTH_*/NEXT_PUBLIC_APP_URL) are hoisted to the job the same way..github/workflows/nightly.yml— acold-clonejob:createthendevon a runner with no database installed, then a poll on the served app and an assertion that the Dev database's state file exists. It sets noDATABASE_URLon purpose — one would be the Database escape and no sidecar would start, which is exactly the property under test. The scaffolded project borrowsexamples/starter's installednode_modules(the PR-gate scaffold guard's trick) because the integration branch's packages are not published.published-create-e2eandexamples-buildare gated tomain, whose Prisma 7 scaffold and SQLite examples they exercise.e2e/utils/db.ts,e2e/global-setup.ts,e2e/global-teardown.ts— the seed helper no longer deletes a SQLite file and runsdb:push. It calls core's lookup: an already-resolvable connection (CI's container) is used as-is; with nothing set,startDevDatabase()runs under the project's.opensaas/, andgenerate,prisma db updateand the Next server Playwright starts all read it back from the state file. Nothing is injected into the app, so provenance stays'dev-database'locally and'env'in CI. Teardown stops the sidecar. The.envthe setup writes no longer carries aDATABASE_URL.package.json—@opensaas/stack-core,@opensaas/stack-cliand the three PGlite packages (at the versions the CLI already pins) become root devDependencies so the e2e harness can call the lookup and the sidecar.Scope
No
packages/*source changed, so no changeset. No example script, README or.env.exampletouched — those are #1159's.Known dependencies
The
cold-clonejob cannot go green until #1159 convertsexamples/starter's config and drops the scaffolder'sdb:pushpost-step and its.envDATABASE_URL; it fails loudly rather than skipping until then. Likewise thee2ejob stays gated tomainuntil #1129 convertsstarter-auth.🤖 Generated with Claude Code