fix(cli): make eql migration --drizzle actually run drizzle-kit - #927
fix(cli): make eql migration --drizzle actually run drizzle-kit#927coderdan wants to merge 2 commits into
eql migration --drizzle actually run drizzle-kit#927Conversation
`stash eql migration --drizzle` aborted for every project with a
drizzle.config.ts, and the abort blamed the one thing that was never
wrong ("Make sure drizzle-kit is installed and configured"). Three
independent defects, all on the spawn (#924):
1. We always passed `--out`. drizzle-kit's `generate` reads its config
file OR its command-line options, never both: any of --schema/--out/
--dialect switches it into CLI mode, where it then aborts demanding
the two we cannot supply ("Please provide required params: [x] schema
[x] dialect"). `--config` cannot be combined with CLI options either.
Verified against drizzle-kit 0.28.5, 0.30.6 and 0.31.4 — this was
never version-specific.
drizzle.config.ts now decides the output directory, and we follow the
path drizzle-kit reports on stdout, warning when it differs from a
`--out` the user passed. `--out` stays the fallback directory to scan
when no path is reported, so an unrecognised banner still works. The
sweep and the closing note now use the directory the file actually
landed in.
2. A drizzle.config.ts reading `process.env.DATABASE_URL` — the
dominant layout, wrapped in `dotenv -e .env.local -- drizzle-kit …`
npm scripts we bypass — could see nothing. We already load .env/
.env.local at startup, and now also thread down a URL only the CLI
can find (a running local Supabase) via a new non-blocking
`tryResolveDatabaseUrl`: same sources as `resolveDatabaseUrl` minus
the prompt and the hard exit.
3. drizzle-kit writes its errors to stdout, not stderr, so the reporter
printed an empty message. Both streams are now surfaced, and a config
that could not read DATABASE_URL gets a follow-up naming that.
Verified end-to-end against real drizzle-kit 0.31.4 with a config that
throws on a missing DATABASE_URL supplied only via .env.local.
🦋 Changeset detectedLatest commit: e945d5c The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // this CLI can find (a running local Supabase), so the config sees one in | ||
| // that case too (#924). | ||
| s.start('Generating custom Drizzle migration...') | ||
| const databaseUrl = tryResolveDatabaseUrl({ supabase: options.supabase }) |
There was a problem hiding this comment.
[P1] Thread init's resolved URL into drizzle-kit
When stash init --drizzle obtains the URL from its prompt, it stores it only in state.databaseUrl; resolveDatabaseUrl deliberately does not mutate process.env. This resolver therefore returns nothing, so a drizzle.config.ts reading DATABASE_URL still aborts. Pass the init-resolved URL through EqlMigrationOptions and use it in the child environment.
There was a problem hiding this comment.
Confirmed and fixed in e945d5c. EqlMigrationOptions gains a databaseUrl seam — the same one installCommand already takes — and generateEqlMigration threads state.databaseUrl through it. It takes precedence over the resolver, since a caller-supplied URL is more specific than anything we could rediscover.
| // Last match wins: one invocation writes one file, but a wrapper that echoes | ||
| // its own banner first should not shadow drizzle-kit's. | ||
| let found: string | undefined | ||
| for (const match of stdout.matchAll(/➜\s*(\S+\.sql)/g)) { |
There was a problem hiding this comment.
[P2] Parse reported migration paths containing spaces
When drizzle.config.ts uses an output path containing spaces, \S+ truncates the reported filename and rejects it as nonexistent. The fallback then scans the unrelated --out directory, causing an abort or potentially selecting an older same-name migration. Capture the complete path up to the drizzle-kit banner suffix instead.
There was a problem hiding this comment.
Confirmed and fixed in e945d5c. Now scans per line and takes everything between the arrow and the last .sql on it, so spaces survive; the existence check still gates the result. Verified live against drizzle-kit 0.31.4 with an out of ./my migrations — found and written correctly.
| ) | ||
| p.log.info( | ||
| `Make sure drizzle-kit is installed and configured: ${execCommand(pm)} drizzle-kit --version`, | ||
| /DATABASE_URL/.test(output) |
There was a problem hiding this comment.
[P2] Do not classify every DATABASE_URL error as missing
Any failure mentioning DATABASE_URL, including malformed URLs or unsupported schemes, is labeled as an unset variable. This replaces drizzle-kit's accurate diagnosis with contradictory remediation. Match only known missing-variable messages or retain the generic follow-up.
There was a problem hiding this comment.
Confirmed and fixed in e945d5c. New looksLikeMissingDatabaseUrl matches only absence phrasings, and anything unrecognised falls through to the generic follow-up — never wrong, since drizzle-kit output prints directly above it. Bare undefined is deliberately not a trigger, so invalid connection string for DATABASE_URL: undefined scheme stays generic. Table-driven test covers both directions.
Three defects from the PR #927 review, all on the same path: - P1: `stash init --drizzle` resolves a database URL at the start of the run and keeps it in `InitState` — `resolveDatabaseUrl` deliberately never writes to `process.env`. The embedded migration route therefore passed nothing down, and a drizzle.config.ts reading DATABASE_URL still aborted. `EqlMigrationOptions` gains a `databaseUrl` seam (the one `installCommand` already has) and init threads its URL through it. - P2: `parseReportedMigrationPath` matched `\S+\.sql`, truncating a reported path at the first space. A drizzle.config.ts writing into a directory with a space in its name failed the existence check and fell through to scanning an unrelated `--out` — an abort, or an older same-named migration. It now scans per line, taking everything between the arrow and the last `.sql` on it. Verified live against drizzle-kit 0.31.4 with an `out` of `./my migrations`. - P3: every failure mentioning DATABASE_URL was reported as "it is not set", which contradicts drizzle-kit whenever the variable is present and merely wrong (malformed URL, unsupported scheme, auth failure). `looksLikeMissingDatabaseUrl` matches only absence phrasings, and an unrecognised one falls through to the generic follow-up — never wrong, since drizzle-kit's own output prints directly above it.
1 participant
Fixes #924.
The defect is broader than the issue reports
stash eql migration --drizzlehas never worked for any project with adrizzle.config.ts— the--outfailure is not drizzle-kit 0.31.x-specific. Reproduced identically on drizzle-kit 0.28.5, 0.30.6 and 0.31.4:generatereads its config file or its command-line options, never both. Passing any of--schema/--out/--dialectswitches it out of config-file mode, and it then demands all three.--configcannot be combined with them either (You can't use both --config and other cli options for generate command).And drizzle-kit writes those errors to stdout, not stderr. The reporter only read
result.stderr, so the user got an empty error line followed by "Make sure drizzle-kit is installed and configured" — the one thing that was never wrong. That is why the real cause stayed invisible in the skilltester run.What changed
1.
--outno longer reaches drizzle-kit.drizzle.config.tsdecides the output directory; we read back the path drizzle-kit reports on stdout ([✓] Your SQL migration file ➜ drizzle/0000_install-eql.sql 🚀) and warn when it differs from an--outthe user passed.--outremains the fallback directory to scan when no path is reported, so a drizzle-kit whose banner we do not recognise still works. The ALTER COLUMN sweep and the closing note now operate on the directory the file actually landed in, not the one we guessed.2. The resolved
DATABASE_URLis threaded into the child env. Adrizzle.config.tsreadingprocess.env.DATABASE_URL(and often throwing when it is missing) is the dominant layout, wrapped indotenv -e .env.local -- drizzle-kit …npm scripts that never run when stash invokes drizzle-kit directly.bin/main.tsalready loads.env/.env.localat startup; on top of that we now pass down a URL only this CLI can find — a running local Supabase — via a newtryResolveDatabaseUrl(): same sources asresolveDatabaseUrl, minus the interactive prompt and the hard exit, because a missing URL is an ordinary answer when decorating a child's environment.3. Both streams are reported. A config that could not read
DATABASE_URLgets a follow-up naming the detected dotenv file; anything else gets a reproduce-it-directly line instead of the install advice.stash init --drizzle's equivalent failure hint was fixed the same way.Verification
End-to-end against real drizzle-kit 0.31.4, in a project whose
drizzle.config.tsthrows on a missingDATABASE_URLsupplied only via.env.local— the exact reported scenario. Generates and writes the install SQL; the--outdivergence warning fires when the config writes elsewhere.Unit coverage added for each defect:
--outabsent from the argv, stdout surfaced on failure, both streams joined, the URL threaded (withprocess.env.DATABASE_URLdeleted, so inheritance alone would fail),envleft untouched when nothing resolves, the reported path followed outside--out, and the scan fallback. Plus five cases fortryResolveDatabaseUrl.packages/cliunit suite: 1355 passing. Twodoctore2e tests fail — pre-existing, confirmed identical on a clean tree (@cipherstash/auth0.42 platform binary), untouched by this PR.Proposal 3 in the issue is not implementable as written
The issue's third item asks for a same-name, cast-preserving
ALTER … SET DATA TYPE public.eql_v3_* USING <cast>recipe. No such cast can exist: EQL values are encrypted client-side through ZeroKMS, and the bundle ships exactly one cast (eql_v3_json_search AS eql_v3.query_json) — nothing fromtext/numeric. There is no SQL expression that converts plaintext into an EQL payload. The existing add-twin rewrite plus the stagedstash encryptbackfill is the correct shape, and that bullet is worth closing rather than leaving open as work.Skills
skills/stash-cli(--outand--drizzleflag rows) andskills/stash-drizzle(a paragraph on who owns the output directory and why the dotenv wrapper is not needed). Command surface is unchanged —stash manifest --jsonstill matches.