From d189ace966ae65ea37349c911e3c18b3fbe3a21b Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Wed, 19 Aug 2026 22:50:25 +1000 Subject: [PATCH 1/3] fix(cli): make `eql migration --drizzle` actually run drizzle-kit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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/eql-migration-drizzle-kit-spawn.md | 9 + .../cli/src/__tests__/database-url.test.ts | 67 +++++- packages/cli/src/cli/registry.ts | 2 +- .../commands/eql/__tests__/migration.test.ts | 197 +++++++++++++++--- packages/cli/src/commands/eql/migration.ts | 137 +++++++++--- .../src/commands/init/steps/install-eql.ts | 5 +- packages/cli/src/config/database-url.ts | 34 +++ packages/cli/src/messages.ts | 38 ++++ skills/stash-cli/SKILL.md | 4 +- skills/stash-drizzle/SKILL.md | 2 + 10 files changed, 439 insertions(+), 56 deletions(-) create mode 100644 .changeset/eql-migration-drizzle-kit-spawn.md diff --git a/.changeset/eql-migration-drizzle-kit-spawn.md b/.changeset/eql-migration-drizzle-kit-spawn.md new file mode 100644 index 000000000..a29b63cca --- /dev/null +++ b/.changeset/eql-migration-drizzle-kit-spawn.md @@ -0,0 +1,9 @@ +--- +'stash': patch +--- + +Fix `stash eql migration --drizzle`, which aborted for every project with a `drizzle.config.ts` (#924). + +- **Stop passing `--out` to `drizzle-kit generate`.** drizzle-kit 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`). Verified against drizzle-kit 0.28.5, 0.30.6 and 0.31.4 — this was never version-specific. Your `drizzle.config.ts` now decides the output directory and stash follows the path drizzle-kit reports, warning when it differs from a `--out` you passed. `--out` remains the fallback directory to search. +- **Pass the resolved `DATABASE_URL` into the drizzle-kit child process.** A `drizzle.config.ts` that reads `process.env.DATABASE_URL` (and often throws when it is missing) previously saw nothing, because the project's usual `dotenv -e .env.local -- drizzle-kit …` wrapper never runs when stash invokes drizzle-kit directly. stash already loads `.env`/`.env.local` at startup; it now also threads down a URL only the CLI can find, such as a running local Supabase. +- **Report the actual failure.** drizzle-kit writes its errors to stdout, not stderr, so the abort printed nothing but "Make sure drizzle-kit is installed and configured" — the one thing that was never wrong. Both streams are now surfaced, and a config that could not read `DATABASE_URL` gets a follow-up naming that instead. diff --git a/packages/cli/src/__tests__/database-url.test.ts b/packages/cli/src/__tests__/database-url.test.ts index 885ce42d0..e5afb2f58 100644 --- a/packages/cli/src/__tests__/database-url.test.ts +++ b/packages/cli/src/__tests__/database-url.test.ts @@ -29,9 +29,8 @@ vi.mock('@clack/prompts', () => ({ note: clack.note, })) -const { resolveDatabaseUrl, withResolverContext } = await import( - '../config/database-url.js' -) +const { resolveDatabaseUrl, tryResolveDatabaseUrl, withResolverContext } = + await import('../config/database-url.js') const VALID_URL = 'postgresql://postgres:postgres@127.0.0.1:54322/postgres' @@ -317,3 +316,65 @@ describe('withResolverContext — concurrent isolation', () => { expect(b).toBe(URL_B) }) }) + +/** + * The non-blocking variant, used to decorate a spawned `drizzle-kit`'s + * environment (#924). Same sources as {@link resolveDatabaseUrl} minus the + * prompt and the hard exit — a missing URL is an ordinary answer here, because + * the child may not need one. + */ +describe('tryResolveDatabaseUrl', () => { + it('prefers the flag from the resolver context', async () => { + process.env.DATABASE_URL = 'postgresql://env@localhost:5432/env' + const result = await withResolverContext( + { databaseUrlFlag: VALID_URL }, + async () => tryResolveDatabaseUrl(), + ) + expect(result).toBe(VALID_URL) + }) + + it('falls back to process.env — the dotenv files bin/main.ts loaded', () => { + process.env.DATABASE_URL = VALID_URL + expect(tryResolveDatabaseUrl()).toBe(VALID_URL) + }) + + it('reaches supabase status when the project has a config.toml', () => { + detect.detectSupabaseProject.mockReturnValue({ + hasMigrationsDir: true, + hasConfigToml: true, + migrationsDir: path.join(tmpDir, 'supabase/migrations'), + }) + supabase.execSync.mockReturnValue(`DB_URL="${VALID_URL}"\n`) + expect(tryResolveDatabaseUrl({ cwd: tmpDir })).toBe(VALID_URL) + }) + + it('returns undefined instead of prompting or exiting', () => { + Object.defineProperty(process.stdin, 'isTTY', { + value: true, + configurable: true, + }) + const exit = vi.spyOn(process, 'exit').mockImplementation((() => { + throw new Error('process.exit called') + }) as never) + + expect(tryResolveDatabaseUrl({ cwd: tmpDir })).toBeUndefined() + expect(clack.text).not.toHaveBeenCalled() + expect(exit).not.toHaveBeenCalled() + }) + + it('ignores a malformed flag rather than exiting on it', async () => { + const exit = vi.spyOn(process, 'exit').mockImplementation((() => { + throw new Error('process.exit called') + }) as never) + process.env.DATABASE_URL = VALID_URL + + const result = await withResolverContext( + { databaseUrlFlag: 'not a url' }, + async () => tryResolveDatabaseUrl(), + ) + // resolveDatabaseUrl exits here; this one is advisory, so it moves on to + // the next source and the caller's own resolution reports the bad flag. + expect(result).toBe(VALID_URL) + expect(exit).not.toHaveBeenCalled() + }) +}) diff --git a/packages/cli/src/cli/registry.ts b/packages/cli/src/cli/registry.ts index 2770c3a8a..655e3b027 100644 --- a/packages/cli/src/cli/registry.ts +++ b/packages/cli/src/cli/registry.ts @@ -439,7 +439,7 @@ export const registry: CommandGroup[] = [ name: '--out', value: '', description: - 'Where the migration is written. Drizzle: passed to `drizzle-kit generate --out`, defaults to `drizzle` — set it to match your drizzle.config.ts. Supabase: leave it alone. The Supabase CLI replays `supabase/migrations` and has no setting to move it, so pointing elsewhere means `supabase db reset` / `db push` never apply the install; the command warns when you do.', + 'Where the migration is written. Drizzle: your drizzle.config.ts `out` decides that, and stash follows the path drizzle-kit reports — this is only the fallback directory to look in (defaults to `drizzle`) if it reports none. Supabase: leave it alone. The Supabase CLI replays `supabase/migrations` and has no setting to move it, so pointing elsewhere means `supabase db reset` / `db push` never apply the install; the command warns when you do.', }, { name: '--force', diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index faf76b318..0d017d1ba 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -41,6 +41,15 @@ vi.mock('@clack/prompts', () => ({ outro: clack.outro, })) +// The DATABASE_URL resolver reaches for `supabase status` on a Supabase +// project, which is a real subprocess — stub it so the env-threading tests can +// pin what the command passes down without one. +const resolveUrlMock = vi.hoisted(() => vi.fn()) +vi.mock('../../../config/database-url.js', async (importOriginal) => ({ + ...(await importOriginal()), + tryResolveDatabaseUrl: resolveUrlMock, +})) + // Stub the drizzle-kit scaffold — only the child process is faked. const spawnMock = vi.hoisted(() => vi.fn()) vi.mock('node:child_process', () => ({ spawnSync: spawnMock })) @@ -107,6 +116,7 @@ vi.mock('../../db/install.js', async (importOriginal) => { beforeEach(() => { fsWrite.spy.mockImplementation(fsWrite.real) rewriteMock.spy.mockImplementation(rewriteMock.real) + resolveUrlMock.mockReturnValue(undefined) }) afterEach(() => { vi.clearAllMocks() @@ -828,26 +838,25 @@ describe('eqlMigrationCommand — Drizzle', () => { ) }) - it('includes --out in the dry-run preview', async () => { + // #924: `--out` must NEVER reach drizzle-kit's argv, in the preview or the + // real run. drizzle-kit treats any of --schema/--out/--dialect as "ignore the + // config file" and then aborts on the two we cannot supply, so passing it + // broke the command for every project with a drizzle.config.ts. The dry run + // is where a user checks what we will run, so it has to show the truth. + it('names --out as the lookup directory, never as a drizzle-kit flag', async () => { const out = join(tmp, 'custom-out') await eqlMigrationCommand({ drizzle: true, dryRun: true, out }) expect(spawnMock).not.toHaveBeenCalled() - expect(clack.note).toHaveBeenCalledWith( - expect.stringContaining(`--out=${out}`), - 'Dry Run', - ) + const [note] = vi.mocked(clack.note).mock.calls.at(-1) ?? [] + expect(String(note)).not.toContain('--out=') + expect(String(note)).toContain(out) }) - // Widest blast radius of the flag handling: because `--out` is ALWAYS - // appended to drizzle-kit's argv, a flag-less invocation silently overrides - // the project's drizzle.config.ts `out` with `/drizzle`. The dry-run - // preview reaches that arm without spawning or touching the filesystem. - it('defaults --out to an absolute drizzle/ when the flag is omitted', async () => { + it('falls back to an absolute drizzle/ when the --out flag is omitted', async () => { await eqlMigrationCommand({ drizzle: true, dryRun: true }) - expect(clack.note).toHaveBeenCalledWith( - expect.stringContaining(`--out=${resolve('drizzle')}`), - 'Dry Run', - ) + const [note] = vi.mocked(clack.note).mock.calls.at(-1) ?? [] + expect(String(note)).not.toContain('--out=') + expect(String(note)).toContain(resolve('drizzle')) }) it('dry run says the grants would be included under --supabase', async () => { @@ -878,11 +887,12 @@ describe('eqlMigrationCommand — Drizzle', () => { const [command, argv] = spawnMock.mock.calls[0] // The whole argv, exactly — not `toContain` checks, which would still pass // if the runner prefix (`exec`) were dropped and drizzle-kit ran under the - // wrong resolver. Three things at once: name and out are discrete inert - // tokens in an array, never interpolated into a shell string; `--out` is - // actually passed, so drizzle-kit writes where step 2 then looks; and the - // project-local `exec` form (not `dlx`) is asserted, so a regression back to - // download-and-run — which resolves a different drizzle.config.ts — fails. + // wrong resolver. Three things at once: `--name` is a discrete inert token + // in an array, never interpolated into a shell string; `--out` is absent + // (#924 — it puts drizzle-kit into CLI-config mode, which then aborts for + // want of --schema/--dialect); and the project-local `exec` form (not + // `dlx`) is asserted, so a regression back to download-and-run — which + // resolves a different drizzle.config.ts — fails. expect(command).toBe('pnpm') expect(argv).toEqual([ 'exec', @@ -890,7 +900,6 @@ describe('eqlMigrationCommand — Drizzle', () => { 'generate', '--custom', '--name=add-eql', - `--out=${out}`, ]) const written = readFileSync(join(out, '0000_add-eql.sql'), 'utf-8') @@ -1207,6 +1216,142 @@ describe('eqlMigrationCommand — Drizzle', () => { expect(warnings).toContain('column already exists') }) + /** + * #924, the half that made the real failure invisible. drizzle-kit writes its + * argument-validation and config-resolution errors to STDOUT, not stderr — + * verified against 0.28.5, 0.30.6 and 0.31.4 — so a reporter that only read + * `result.stderr` printed nothing but a generic hint and sent users looking + * at their drizzle-kit install, which was never the problem. + */ + it("surfaces drizzle-kit's stdout, which is where it prints its errors", async () => { + spawnMock.mockReturnValue({ + status: 1, + stdout: + 'Error Please provide required params:\n [x] dialect: undefined', + stderr: '', + }) + await expect( + eqlMigrationCommand({ drizzle: true, out: join(tmp, 'drizzle') }), + ).rejects.toBeInstanceOf(CliExit) + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('Please provide required params'), + ) + }) + + it('joins both streams so neither half of the failure is dropped', async () => { + spawnMock.mockReturnValue({ + status: 1, + stdout: "Reading config file 'drizzle.config.ts'", + stderr: 'Error: DATABASE_URL is not set', + }) + await expect( + eqlMigrationCommand({ drizzle: true, out: join(tmp, 'drizzle') }), + ).rejects.toBeInstanceOf(CliExit) + const reported = String(vi.mocked(clack.log.error).mock.calls.at(-1)?.[0]) + expect(reported).toContain("Reading config file 'drizzle.config.ts'") + expect(reported).toContain('DATABASE_URL is not set') + // A config that throws on a missing URL is the dominant .env.local shape, + // so the follow-up names that rather than the generic reproduce-it line. + expect(clack.log.info).toHaveBeenCalledWith( + expect.stringContaining('could not read DATABASE_URL'), + ) + }) + + /** + * The project's own npm script is usually `dotenv -e .env.local -- + * drizzle-kit …`; invoked by us that wrapper never runs, so the config sees + * `process.env.DATABASE_URL` and finds nothing. We pass the URL we resolved + * down explicitly. + */ + it('threads the resolved DATABASE_URL into the drizzle-kit child env', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + // Nothing in the parent env: the URL comes from a source only this CLI + // knows about (a running local Supabase), so inheritance alone would leave + // the config with `undefined` — the #924 abort. + const previous = process.env.DATABASE_URL + delete process.env.DATABASE_URL + resolveUrlMock.mockReturnValue( + 'postgres://postgres@127.0.0.1:54322/postgres', + ) + try { + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0000_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + await eqlMigrationCommand({ drizzle: true, out }) + const [, , opts] = spawnMock.mock.calls[0] + expect(opts.env.DATABASE_URL).toBe( + 'postgres://postgres@127.0.0.1:54322/postgres', + ) + // The rest of the environment still has to reach drizzle-kit — PATH above + // all, or the runner cannot even find it. + expect(opts.env.PATH).toBe(process.env.PATH) + } finally { + if (previous !== undefined) process.env.DATABASE_URL = previous + } + }) + + it('leaves the child env alone when no URL can be resolved', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + resolveUrlMock.mockReturnValue(undefined) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0000_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + await eqlMigrationCommand({ drizzle: true, out }) + const [, , opts] = spawnMock.mock.calls[0] + expect(opts.env).toBe(process.env) + }) + + /** + * With `--out` gone from the argv, `drizzle.config.ts` decides the output + * directory — so the path drizzle-kit reports is the only authority on where + * the file went. Using it (rather than scanning our guess) is what keeps the + * command working for a project whose config writes somewhere else. + */ + it('follows the path drizzle-kit reports, even outside --out', async () => { + const configured = join(tmp, 'db', 'migrations') + mkdirSync(configured, { recursive: true }) + const requested = join(tmp, 'drizzle') + const written = join(configured, '0000_install-eql.sql') + spawnMock.mockImplementation(() => { + writeFileSync(written, '') + return { + status: 0, + stdout: `[✓] Your SQL migration file ➜ ${written} 🚀`, + stderr: '', + } + }) + + await eqlMigrationCommand({ drizzle: true, out: requested }) + + expect(readFileSync(written, 'utf-8')).toContain('EQL v3 schema creation') + // `--out` looking honoured while the file lands elsewhere is exactly the + // silent divergence the old always-pass-`--out` code was trying to avoid. + const warnings = clack.log.warn.mock.calls + .map((c) => String(c[0])) + .join('\n') + expect(warnings).toContain(configured) + expect(warnings).toContain(requested) + }) + + it('falls back to scanning --out when drizzle-kit reports no path', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0000_install-eql.sql'), '') + // An unrecognised banner: the scan is what keeps a future drizzle-kit + // (or a wrapper that swallows stdout) working. + return { status: 0, stdout: 'done', stderr: '' } + }) + await eqlMigrationCommand({ drizzle: true, out }) + expect(readFileSync(join(out, '0000_install-eql.sql'), 'utf-8')).toContain( + 'EQL v3 schema creation', + ) + }) + it('aborts (exit 1) when drizzle-kit exits non-zero', async () => { spawnMock.mockReturnValue({ status: 1, stdout: '', stderr: 'boom' }) await expect( @@ -1216,11 +1361,11 @@ describe('eqlMigrationCommand — Drizzle', () => { }) it('reports the spawn error when drizzle-kit cannot be launched', async () => { - // spawnSync's ENOENT shape: null status, no captured stderr, `error` set. - // `result.stderr?.trim()` is undefined, so the message falls through to the + // spawnSync's ENOENT shape: null status, neither stream captured, `error` + // set. Both streams trim to nothing, so the message falls through to the // second arm (`result.error?.message`) — the realistic "drizzle-kit isn't - // installed" case. If the `?.` on stderr were dropped, this shape would - // throw a TypeError instead of reporting. + // installed" case. If the `?.` on either stream were dropped, this shape + // would throw a TypeError instead of reporting. spawnMock.mockReturnValue({ status: null, stdout: null, @@ -1235,7 +1380,9 @@ describe('eqlMigrationCommand — Drizzle', () => { ).rejects.toBeInstanceOf(CliExit) expect(clack.log.error).toHaveBeenCalledWith('spawnSync pnpm ENOENT') expect(clack.log.info).toHaveBeenCalledWith( - expect.stringContaining('Make sure drizzle-kit is installed'), + expect.stringContaining( + 'drizzle-kit generate --custom --name=install-eql', + ), ) }) diff --git a/packages/cli/src/commands/eql/migration.ts b/packages/cli/src/commands/eql/migration.ts index efae6d2c7..0e2ed232b 100644 --- a/packages/cli/src/commands/eql/migration.ts +++ b/packages/cli/src/commands/eql/migration.ts @@ -1,7 +1,7 @@ import { spawnSync } from 'node:child_process' import { existsSync, unlinkSync, writeFileSync } from 'node:fs' import { readdir } from 'node:fs/promises' -import { join, resolve } from 'node:path' +import { dirname, isAbsolute, join, resolve } from 'node:path' import { MIGRATIONS_SCHEMA_SQL } from '@cipherstash/migrate' import * as p from '@clack/prompts' import { CliExit } from '@/cli/exit.js' @@ -22,6 +22,10 @@ import { execArgv, execCommand, } from '@/commands/init/utils.js' +import { + detectDotenvFile, + tryResolveDatabaseUrl, +} from '@/config/database-url.js' import { loadBundledEqlSql, SUPABASE_MIGRATION_GRANTS_SQL_V3, @@ -53,6 +57,35 @@ export async function findGeneratedMigration( return join(outDir, matchingFiles[matchingFiles.length - 1]) } +/** + * Pull the migration path out of drizzle-kit's own success line: + * + * [✓] Your SQL migration file ➜ drizzle/0000_install-eql.sql 🚀 + * + * Since we no longer pass `--out` (drizzle-kit rejects the run when we do — + * see `generateDrizzleEqlMigration`), `drizzle.config.ts` owns the output + * directory and this line is how we learn it. The path is printed relative to + * the cwd drizzle-kit ran in, which is ours. + * + * Returns `undefined` unless the file is actually there, so an unrecognised + * banner or a moved file falls through to the directory scan rather than + * pointing the SQL write at a path that does not exist. + */ +export function parseReportedMigrationPath( + stdout: string | undefined, +): string | undefined { + if (!stdout) return undefined + // 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)) { + const candidate = match[1] + const abs = isAbsolute(candidate) ? candidate : resolve(candidate) + if (existsSync(abs)) found = abs + } + return found +} + function cleanupMigrationFile(filePath: string | undefined): void { if (!filePath) return try { @@ -370,10 +403,25 @@ async function generateDrizzleEqlMigration( // download-and-run form — it must resolve this project's drizzle.config.ts and // schema. Invoke via spawnSync with an argv array (no shell), so a `--name` // carrying spaces or shell metacharacters is one inert token, never word-split - // or executed. `--out` is always passed so drizzle-kit WRITES where we then - // LOOK — otherwise a project whose drizzle.config.ts points elsewhere would - // have drizzle-kit write there while we search `drizzle/` and fail in step 2. - // It defaults to `drizzle/`; override with `--out` to match your config. + // or executed. + // + // `--out` is deliberately NOT passed (#924). drizzle-kit's `generate` has two + // mutually exclusive configuration modes, and passing ANY of --schema/--out/ + // --dialect switches it out of config-file mode into CLI mode — where it then + // demands all three and aborts: + // + // Error Please provide required params: + // [x] schema: undefined + // [x] dialect: undefined + // [✓] out: '/abs/path/drizzle' + // + // We know neither the schema glob nor the dialect, and `--config` cannot be + // combined with CLI options either ("You can't use both --config and other + // cli options for generate command"). Verified against drizzle-kit 0.28.5, + // 0.30.6 and 0.31.4 — this was never version-specific, so every project with + // a drizzle.config.ts hit it. So drizzle.config.ts decides the output + // directory; step 2 reads back the path drizzle-kit reports, and `--out` is + // only the fallback place to look. const { command, prefixArgs } = execArgv(pm) const drizzleArgs = [ ...prefixArgs, @@ -381,7 +429,6 @@ async function generateDrizzleEqlMigration( 'generate', '--custom', `--name=${migrationName}`, - `--out=${outDir}`, ] const displayCmd = `${execCommand(pm)} ${drizzleArgs.slice(prefixArgs.length).join(' ')}` @@ -401,7 +448,7 @@ async function generateDrizzleEqlMigration( if (options.dryRun) { p.note( - `Would run: ${displayCmd}\nWould write the EQL v3 install SQL${options.supabase ? ' (with Supabase grants)' : ''} into the generated migration in ${outDir}`, + `Would run: ${displayCmd}\nWould write the EQL v3 install SQL${options.supabase ? ' (with Supabase grants)' : ''} into the migration drizzle-kit generates (your drizzle.config.ts \`out\` decides the directory; ${outDir} is the fallback if drizzle-kit does not report the path)`, 'Dry Run', ) if (!embedded) p.outro('Dry run complete.') @@ -412,42 +459,84 @@ async function generateDrizzleEqlMigration( // Step 1 — scaffold an empty custom migration (drizzle-kit owns the journal // + sequence numbering; hand-rolling that is fragile). + // + // The child inherits our env, which already carries the dotenv files + // `bin/main.ts` loads at startup — that is what a `drizzle.config.ts` reading + // `process.env.DATABASE_URL` needs, and what the project's usual + // `dotenv -e .env.local -- drizzle-kit …` npm-script wrapper would have + // supplied had we gone through it. On top of that we thread down a URL only + // 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 }) const result = spawnSync(command, drizzleArgs, { stdio: 'pipe', encoding: 'utf-8', + env: databaseUrl + ? { ...process.env, DATABASE_URL: databaseUrl } + : process.env, }) if (result.status !== 0) { s.stop('Failed to generate migration.') - const stderr = result.stderr?.trim() + // drizzle-kit writes its argument-validation and config errors to STDOUT + // and its thrown-config errors to stderr, so reporting one stream alone + // loses the whole message half the time — which is how #924 stayed + // invisible behind "make sure drizzle-kit is installed". + const output = [result.stdout, result.stderr] + .map((stream) => stream?.trim()) + .filter((stream): stream is string => Boolean(stream)) + .join('\n') p.log.error( - stderr || + output || result.error?.message || `drizzle-kit exited with status ${result.status ?? 'unknown'}.`, ) p.log.info( - `Make sure drizzle-kit is installed and configured: ${execCommand(pm)} drizzle-kit --version`, + /DATABASE_URL/.test(output) + ? messages.eql.migrationDrizzleKitNoDatabaseUrl(detectDotenvFile()) + : messages.eql.migrationDrizzleKitFailed( + `${execCommand(pm)} drizzle-kit generate --custom --name=${migrationName}`, + ), ) if (!embedded) p.outro('Migration aborted.') throw new CliExit(1) } s.stop('Custom Drizzle migration generated.') - // Step 2 — locate the file drizzle-kit just wrote. + // Step 2 — locate the file drizzle-kit just wrote. It prints the path itself + // ("[✓] Your SQL migration file ➜ drizzle/0000_install-eql.sql 🚀"), which is + // the only authority on where its config sent the file now that we no longer + // pass `--out` (see step 1's note). Falling back to a scan of `outDir` keeps + // the old behaviour when that line is absent or points at something that + // vanished — a drizzle-kit whose banner we do not recognise still works. let migrationPath: string s.start('Locating generated migration file...') - try { - migrationPath = await findGeneratedMigration(outDir, migrationName) + const reported = parseReportedMigrationPath(result.stdout) + if (reported) { + migrationPath = reported s.stop(`Found migration: ${migrationPath}`) - } catch (error) { - s.stop('Failed to locate migration file.') - p.log.error(error instanceof Error ? error.message : String(error)) - p.log.info( - `If your drizzle.config.ts writes elsewhere, pass --out so it matches.`, - ) - if (!embedded) p.outro('Migration aborted.') - throw new CliExit(1) + const writtenDir = dirname(migrationPath) + if (options.out !== undefined && writtenDir !== outDir) { + p.log.warn(messages.eql.migrationDrizzleOutOverridden(writtenDir, outDir)) + } + } else { + try { + migrationPath = await findGeneratedMigration(outDir, migrationName) + s.stop(`Found migration: ${migrationPath}`) + } catch (error) { + s.stop('Failed to locate migration file.') + p.log.error(error instanceof Error ? error.message : String(error)) + p.log.info( + `If your drizzle.config.ts writes elsewhere, pass --out so it matches.`, + ) + if (!embedded) p.outro('Migration aborted.') + throw new CliExit(1) + } } + // Everything downstream (the SQL write, the ALTER COLUMN sweep, the closing + // note) works on the directory the file actually landed in, not the one we + // guessed. + const migrationDir = dirname(migrationPath) // Step 3 — write the EQL v3 install SQL into it. s.start('Writing EQL v3 install SQL into migration file...') @@ -472,17 +561,17 @@ async function generateDrizzleEqlMigration( let sweepIncomplete = false try { sweepIncomplete = reportSweepResult( - await rewriteEncryptedAlterColumns(outDir, { skip: migrationPath }), + await rewriteEncryptedAlterColumns(migrationDir, { skip: migrationPath }), ) } catch (error) { // Advisory: the install migration itself is already written and valid. sweepIncomplete = true - reportSweepFailure(outDir, error) + reportSweepFailure(migrationDir, error) } if (sweepIncomplete) { p.log.error( - `The ALTER COLUMN sweep found unsafe or unverified SQL. The generated migration remains at ${migrationPath}, but review the sibling migrations in ${outDir} and use the staged stash encrypt flow before running drizzle-kit migrate.`, + `The ALTER COLUMN sweep found unsafe or unverified SQL. The generated migration remains at ${migrationPath}, but review the sibling migrations in ${migrationDir} and use the staged stash encrypt flow before running drizzle-kit migrate.`, ) if (!embedded) p.outro('Migration aborted.') throw new CliExit(1) diff --git a/packages/cli/src/commands/init/steps/install-eql.ts b/packages/cli/src/commands/init/steps/install-eql.ts index dc5308c69..389160182 100644 --- a/packages/cli/src/commands/init/steps/install-eql.ts +++ b/packages/cli/src/commands/init/steps/install-eql.ts @@ -128,7 +128,10 @@ function resolveMigrationRoute( options: { drizzle: true, supabase: supabase || undefined }, retryCommand: 'stash eql migration --drizzle', failureHint: - 'Could not generate the EQL migration — check that drizzle-kit is installed and configured.', + // Not "check that drizzle-kit is installed" (#924): that was almost + // never the cause, and drizzle-kit's own output — now printed above + // this line, from both streams — says what actually went wrong. + 'Could not generate the EQL migration. drizzle-kit reported the failure above; the usual cause is a drizzle.config.ts that cannot read DATABASE_URL.', } } if (supabase && hasLocalSupabaseScaffolding()) { diff --git a/packages/cli/src/config/database-url.ts b/packages/cli/src/config/database-url.ts index 5d5b91a18..c13467f35 100644 --- a/packages/cli/src/config/database-url.ts +++ b/packages/cli/src/config/database-url.ts @@ -254,3 +254,37 @@ export async function resolveDatabaseUrl( } process.exit(1) } + +/** + * Best-effort sibling of {@link resolveDatabaseUrl}: same source order, minus + * the two tiers that take over the terminal. It never prompts, never prints, + * and never exits — it returns `undefined` when nothing is configured. + * + * That is what makes it usable for *decorating a child process's environment* + * rather than for connecting ourselves. `eql migration --drizzle` spawns the + * project's `drizzle-kit`, whose `drizzle.config.ts` typically reads + * `process.env.DATABASE_URL` (and often throws when it is missing). We inherit + * the parent env, so a `.env.local` value is already there — `bin/main.ts` + * loads the dotenv files at startup. What is NOT there is a URL that only this + * CLI knows how to find: `supabase status --output env` on a local Supabase + * project, or a `--database-url` flag threaded through the resolver context. + * Passing that down turns a hard abort into a working scaffold, and a failure + * to find one is not an error here — drizzle-kit may not need a URL at all. + */ +export function tryResolveDatabaseUrl( + opts: ResolveDatabaseUrlOptions = {}, +): string | undefined { + const ctx: ResolveDatabaseUrlOptions = { ...als.getStore(), ...opts } + const cwd = ctx.cwd ?? process.cwd() + + const flag = ctx.databaseUrlFlag?.trim() + if (flag && isUrlParseable(flag)) return flag + + const fromEnv = process.env.DATABASE_URL?.trim() + if (fromEnv) return fromEnv + + if (ctx.supabase || detectSupabaseProject(cwd).hasConfigToml) { + return trySupabaseStatus() + } + return undefined +} diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index 8dc5db157..40ddc1626 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -116,6 +116,44 @@ export const messages = { /** `--name` carried characters outside `[A-Za-z0-9_-]`. */ migrationBadName: 'Migration name must contain only letters, numbers, dashes, and underscores.', + /** + * Generic follow-up when the spawned `drizzle-kit generate` exits non-zero + * and we cannot name a more specific cause. + * + * Deliberately no longer says "make sure drizzle-kit is installed and + * configured": that was the one thing almost never wrong, and it sent + * people looking in the wrong place (#924). drizzle-kit prints its own + * failures — including the argument validation ones — to STDOUT, not + * stderr, so the caller must surface both streams for this hint to be the + * fallback rather than the whole message. + */ + migrationDrizzleKitFailed: (versionCmd: string) => + `drizzle-kit's own output is above. Reproduce it directly with \`${versionCmd}\` to see the failure without stash in the way.`, + /** + * drizzle-kit ran, read `drizzle.config.ts`, and the config itself blew up + * on a missing `DATABASE_URL`. + * + * The dominant Drizzle layout keeps the URL in `.env.local` and wraps + * drizzle-kit in `dotenv -e .env.local -- drizzle-kit …`; invoked by us + * that wrapper never runs. We load the dotenv files at startup and thread a + * resolved URL into the child env, so reaching this message means neither + * found one — say where to put it rather than blaming the install. + */ + migrationDrizzleKitNoDatabaseUrl: (dotenvFile: string) => + `Your drizzle.config.ts could not read DATABASE_URL. stash loads ${dotenvFile} and passes the URL it resolves down to drizzle-kit, so it is not set in either — export DATABASE_URL, or add it to ${dotenvFile}, and re-run.`, + /** + * drizzle-kit wrote somewhere other than the `--out` we were given. + * + * We no longer pass `--out` down (drizzle-kit treats ANY of + * schema/out/dialect on the command line as "ignore the config file + * entirely", then rejects the run for the two we cannot supply — see + * `generateDrizzleEqlMigration`), so `drizzle.config.ts` decides the + * directory and `--out` is only where we look. When the two disagree, the + * config wins and the user needs to know which one their file landed in — + * silently using the real one would leave `--out` looking honoured. + */ + migrationDrizzleOutOverridden: (configured: string, requested: string) => + `drizzle-kit wrote into ${configured}, not the --out you passed (${requested}) — the \`out\` in your drizzle.config.ts decides where migrations go, and stash follows it so the journal stays consistent. Change it there if you want them elsewhere.`, /** * `--name` with `--supabase`. The Supabase filename is fixed because * duplicate detection matches on the `_cipherstash_eql.sql` suffix, so the diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index a63e998a7..36f5e474f 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -422,11 +422,11 @@ stash eql migration --supabase # supabase/migrations/_cip | Flag | Description | |---|---| -| `--drizzle` | Emit a Drizzle custom migration (via `drizzle-kit generate --custom`, then inject the SQL). Requires `drizzle-kit`. | +| `--drizzle` | Emit a Drizzle custom migration (via `drizzle-kit generate --custom`, then inject the SQL). Requires `drizzle-kit`, and a `drizzle.config.ts` that can read `DATABASE_URL` — stash loads your `.env`/`.env.local` and passes the URL it resolves down to the child, so the usual `dotenv -e .env.local -- drizzle-kit …` wrapper is not needed. | | `--prisma` | **Not needed** — Prisma Next installs the EQL bundle through its own migration framework (the extension pack's `migrations/cipherstash/` contract space; run `prisma-next migrate`). The flag exists only to say so and point you there. | | `--supabase` | Alone: write the install into `supabase/migrations/`, so it survives `supabase db reset`. With `--drizzle`: append the Supabase role grants (`eql_v3` + `eql_v3_internal` → `anon`, `authenticated`, `service_role`) instead. Harmless when you connect directly as `postgres`; needed when the same tables are reached via PostgREST/RLS. | | `--name ` | Migration name (Drizzle). Default `install-eql`. Letters, numbers, `-`, and `_` only — anything else is rejected. | -| `--out ` | Where the migration is written. Drizzle: default `drizzle`, passed straight to `drizzle-kit --out`, so set it to match your `drizzle.config.ts` if that writes elsewhere. Supabase: leave it alone — see below. | +| `--out ` | Where the migration is written. Drizzle: your `drizzle.config.ts` `out` decides that — stash follows the path drizzle-kit reports and warns if it differs from this flag. `--out` is only the fallback directory to search (default `drizzle`) when drizzle-kit reports no path. Supabase: leave it alone — see below. | | `--force` | Regenerate the Supabase install migration in place when one already exists (keeping its version, so an applied ledger stays consistent). Without it, a second run exits 1. Re-applying the replaced file takes a specific recipe — see "Re-applying after `--force`" below. Not needed for `--drizzle` — drizzle-kit numbers each generated migration. | | `--dry-run` | Show what would happen without writing anything. | diff --git a/skills/stash-drizzle/SKILL.md b/skills/stash-drizzle/SKILL.md index 65a30d81b..99a418376 100644 --- a/skills/stash-drizzle/SKILL.md +++ b/skills/stash-drizzle/SKILL.md @@ -61,6 +61,8 @@ stash eql migration --drizzle --supabase # also grants eql_v3 to anon/authenti The generated migration also installs the `cs_migrations` tracking schema, so a single `drizzle-kit migrate` covers everything `stash encrypt …` needs — no out-of-band `stash eql install`. EQL v3 ships one SQL bundle for every target including Supabase; `--supabase` only adds the PostgREST/RLS role grants (harmless when you connect directly as `postgres`). Requires `drizzle-kit` installed and configured. +Your `drizzle.config.ts` decides the output directory: stash runs `drizzle-kit generate --custom` with no `--out` (drizzle-kit reads its config file *or* command-line options, never both — passing `--out` makes it demand `--schema` and `--dialect` too and abort), then follows the path drizzle-kit prints. `--out` is only the fallback directory to search. If your config reads `DATABASE_URL` — the usual `dotenv -e .env.local -- drizzle-kit …` shape — you do not need that wrapper here: stash loads `.env`/`.env.local` itself and passes the URL it resolves into the child process. + **Changing an existing plaintext column to an encrypted one.** `drizzle-kit generate` emits an in-place `ALTER TABLE … ALTER COLUMN … SET DATA TYPE eql_v3_`, which Postgres rejects — there is no cast from `text`/`numeric` to an EQL domain. (On drizzle-kit 0.31.0 and later the emitted type is also mangled to `"undefined"."eql_v3_"`, since a `customType` has no `typeSchema`.) Repair it with: ```bash From e945d5cc2dc30d0fd402db87710052be5a4394d6 Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Wed, 19 Aug 2026 23:03:14 +1000 Subject: [PATCH 2/3] fix(cli): address review findings on the drizzle-kit spawn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../commands/eql/__tests__/migration.test.ts | 71 +++++++++++++++++++ packages/cli/src/commands/eql/migration.ts | 65 +++++++++++++++-- .../src/commands/init/steps/install-eql.ts | 11 ++- 3 files changed, 139 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index 0d017d1ba..0bdfeaca7 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -1352,6 +1352,77 @@ describe('eqlMigrationCommand — Drizzle', () => { ) }) + /** + * Review finding, #924 follow-up. `resolveDatabaseUrl` never writes to + * `process.env`, so a URL init got from its own prompt or `--database-url` + * lives in `InitState.databaseUrl` and nowhere the child could see it. The + * option is the seam init passes it through — without it the embedded + * Drizzle route still aborts on a config that reads `DATABASE_URL`. + */ + it("prefers a caller-supplied databaseUrl over the resolver's", async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + resolveUrlMock.mockReturnValue('postgres://resolver@localhost:5432/db') + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0000_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + + await eqlMigrationCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://from-init@localhost:5432/db', + }) + + const [, , opts] = spawnMock.mock.calls[0] + expect(opts.env.DATABASE_URL).toBe('postgres://from-init@localhost:5432/db') + }) + + /** + * Review finding. `\S+` truncated a reported path at the first space, so a + * project whose drizzle.config.ts writes into a directory with a space in it + * failed the existence check and fell through to scanning an unrelated + * `--out` — an abort, or worse, an older same-named migration. + */ + it('reads a reported path that contains spaces', async () => { + const configured = join(tmp, 'My Project', 'db migrations') + mkdirSync(configured, { recursive: true }) + const written = join(configured, '0000_install-eql.sql') + spawnMock.mockImplementation(() => { + writeFileSync(written, '') + return { + status: 0, + stdout: `[✓] Your SQL migration file ➜ ${written} 🚀`, + stderr: '', + } + }) + + await eqlMigrationCommand({ drizzle: true, out: join(tmp, 'drizzle') }) + + expect(readFileSync(written, 'utf-8')).toContain('EQL v3 schema creation') + }) + + /** + * Review finding. Every failure naming DATABASE_URL used to be reported as + * "it is not set", which contradicts drizzle-kit whenever the variable is + * present and merely wrong. The generic follow-up is never wrong, so an + * unrecognised phrasing has to fall through to it. + */ + it.each([ + ['Error: DATABASE_URL is not set', true], + ['Missing environment variable: DATABASE_URL', true], + ['DATABASE_URL is required', true], + ['invalid connection string for DATABASE_URL: undefined scheme', false], + ['DATABASE_URL: password authentication failed for user "app"', false], + ])('classifies %j as missing-URL=%s', async (stderr, missing) => { + spawnMock.mockReturnValue({ status: 1, stdout: '', stderr }) + await expect( + eqlMigrationCommand({ drizzle: true, out: join(tmp, 'drizzle') }), + ).rejects.toBeInstanceOf(CliExit) + const info = String(vi.mocked(clack.log.info).mock.calls.at(-1)?.[0]) + expect(info.includes('could not read DATABASE_URL')).toBe(missing) + }) + it('aborts (exit 1) when drizzle-kit exits non-zero', async () => { spawnMock.mockReturnValue({ status: 1, stdout: '', stderr: 'boom' }) await expect( diff --git a/packages/cli/src/commands/eql/migration.ts b/packages/cli/src/commands/eql/migration.ts index 0e2ed232b..d06a987f2 100644 --- a/packages/cli/src/commands/eql/migration.ts +++ b/packages/cli/src/commands/eql/migration.ts @@ -75,17 +75,52 @@ export function parseReportedMigrationPath( stdout: string | undefined, ): string | undefined { if (!stdout) return undefined - // Last match wins: one invocation writes one file, but a wrapper that echoes + // Scanned per line, taking everything between the arrow and the LAST `.sql` + // on it, because an output directory may contain spaces — a `\S+` pattern + // truncates such a path, fails the existence check, and drops us into the + // fallback scan of an unrelated directory. + // + // Last line 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)) { - const candidate = match[1] + for (const line of stdout.split(/\r?\n/)) { + const arrow = line.indexOf('➜') + if (arrow === -1) continue + const rest = line.slice(arrow + 1) + const end = rest.lastIndexOf('.sql') + if (end === -1) continue + const candidate = rest.slice(0, end + '.sql'.length).trim() + if (!candidate) continue const abs = isAbsolute(candidate) ? candidate : resolve(candidate) if (existsSync(abs)) found = abs } return found } +/** + * Does drizzle-kit's output say `DATABASE_URL` is *absent*, as opposed to + * present and wrong? + * + * Only the absent case earns the "add it to your dotenv file" follow-up. A + * malformed URL, an unsupported scheme, or a refused connection all mention + * `DATABASE_URL` too, and answering those with "it is not set" replaces + * drizzle-kit's accurate diagnosis with remediation that contradicts it. + * + * Deliberately conservative: an unrecognised phrasing falls through to the + * generic follow-up, which is never wrong — drizzle-kit's own output is + * printed directly above either way. Note that bare `undefined` is NOT a + * trigger; `invalid connection string for DATABASE_URL: undefined scheme` + * is a malformed URL, not a missing one. + */ +export function looksLikeMissingDatabaseUrl(output: string): boolean { + return ( + /DATABASE_URL\W{0,20}(?:is |was )?(?:not set|unset|not defined|is undefined|not provided|missing|empty|required)\b/i.test( + output, + ) || + /\b(?:missing|unset|undefined|no)\b[^\n]{0,40}?DATABASE_URL/i.test(output) + ) +} + function cleanupMigrationFile(filePath: string | undefined): void { if (!filePath) return try { @@ -135,6 +170,19 @@ export interface EqlMigrationOptions { * so a re-run never collides. */ force?: boolean + /** + * A database URL the caller already resolved, threaded into the spawned + * `drizzle-kit`'s environment so a `drizzle.config.ts` reading + * `process.env.DATABASE_URL` sees one. + * + * `resolveDatabaseUrl` deliberately never writes to `process.env`, so a URL + * that came from init's prompt (or its `--database-url` flag) lives only in + * `InitState.databaseUrl` — inheritance alone would leave the child with + * nothing and abort the scaffold, which is half of #924. There is no + * `--database-url` flag on this command; this is the programmatic seam init + * uses, matching the one it already passes to `installCommand`. + */ + databaseUrl?: string /** Describe what would happen without writing anything. */ dryRun?: boolean /** @@ -465,10 +513,13 @@ async function generateDrizzleEqlMigration( // `process.env.DATABASE_URL` needs, and what the project's usual // `dotenv -e .env.local -- drizzle-kit …` npm-script wrapper would have // supplied had we gone through it. On top of that we thread down a URL only - // this CLI can find (a running local Supabase), so the config sees one in - // that case too (#924). + // this CLI can find: one the caller already resolved (init's prompt, which + // lands in `InitState.databaseUrl` and nowhere else — `resolveDatabaseUrl` + // never writes to `process.env`), or a running local Supabase. Either way + // the config sees one where inheritance alone would leave it empty (#924). s.start('Generating custom Drizzle migration...') - const databaseUrl = tryResolveDatabaseUrl({ supabase: options.supabase }) + const databaseUrl = + options.databaseUrl ?? tryResolveDatabaseUrl({ supabase: options.supabase }) const result = spawnSync(command, drizzleArgs, { stdio: 'pipe', encoding: 'utf-8', @@ -492,7 +543,7 @@ async function generateDrizzleEqlMigration( `drizzle-kit exited with status ${result.status ?? 'unknown'}.`, ) p.log.info( - /DATABASE_URL/.test(output) + looksLikeMissingDatabaseUrl(output) ? messages.eql.migrationDrizzleKitNoDatabaseUrl(detectDotenvFile()) : messages.eql.migrationDrizzleKitFailed( `${execCommand(pm)} drizzle-kit generate --custom --name=${migrationName}`, diff --git a/packages/cli/src/commands/init/steps/install-eql.ts b/packages/cli/src/commands/init/steps/install-eql.ts index 389160182..2f218c104 100644 --- a/packages/cli/src/commands/init/steps/install-eql.ts +++ b/packages/cli/src/commands/init/steps/install-eql.ts @@ -162,7 +162,16 @@ async function generateEqlMigration( await scaffoldConfigAndClient(state) try { - await eqlMigrationCommand({ ...route.options, embedded: true }) + // The URL init resolved at the start of the run. It lives only in + // `InitState` — `resolveDatabaseUrl` never writes to `process.env` — so + // the Drizzle route's `drizzle-kit` child would otherwise see nothing and + // abort on a config that reads `DATABASE_URL` (#924). Same reason the + // direct-install route below passes it to `installCommand`. + await eqlMigrationCommand({ + ...route.options, + databaseUrl: state.databaseUrl, + embedded: true, + }) } catch { p.log.error(route.failureHint) p.note(`Re-run with: ${route.retryCommand}`, 'You can retry manually') From eead8b09470ea35595e2fb1d786ecd3106cbfa31 Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Thu, 20 Aug 2026 15:19:44 +0930 Subject: [PATCH 3/3] fix(cli): correct the drizzle-kit failure follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two blocking findings from @tobyhede's review of #927: - A spawn failure (`result.error`, e.g. ENOENT) leaves both streams empty, so the reporter fell through to "drizzle-kit's own output is above. Reproduce it directly with …" — pointing at output that does not exist and a command that cannot run. The user re-ran it and got the same ENOENT with no hint that anything was missing. There is now a third, narrowest branch on `result.error` carrying the install advice the other two arms deliberately dropped, and it names the runner: `pnpm exec drizzle-kit` fails with ENOENT when *pnpm* is missing, not drizzle-kit. The test that locked in the old text asserts the new behaviour, plus a case proving the not-launched branch outranks the missing-URL classifier when stderr happens to carry both. - `looksLikeMissingDatabaseUrl` listed `no` in its alternation, which reads as strongly as `missing` to a regex but is ordinary prose: "No other issues found, DATABASE_URL looks valid" classified as an unset variable, inverting the diagnosis. Dropped, with `not set` / `not defined` added in its place, and two false-positive rows added to the table-driven test. Checked against nine phrasings including a zod `received undefined` shape, a malformed URL, an auth failure, and a refused connection. Also takes the optional dedup: both resolvers now share one `shouldTrySupabase` helper rather than repeating the detection line. Verified live: `stash eql migration --drizzle` under a stripped PATH now reports "`npx` could not be started … Check that npx and drizzle-kit are both installed and on PATH". --- .../commands/eql/__tests__/migration.test.ts | 39 ++++++++++++++++--- packages/cli/src/commands/eql/migration.ts | 36 ++++++++++++----- packages/cli/src/config/database-url.ts | 17 ++++++-- packages/cli/src/messages.ts | 14 +++++++ 4 files changed, 88 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index 0bdfeaca7..d46fda8aa 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -1414,6 +1414,10 @@ describe('eqlMigrationCommand — Drizzle', () => { ['DATABASE_URL is required', true], ['invalid connection string for DATABASE_URL: undefined scheme', false], ['DATABASE_URL: password authentication failed for user "app"', false], + // `no` reads as strongly as `missing` to a regex, and appears constantly in + // ordinary prose — a classifier that fires on it inverts the diagnosis. + ['No other issues found, DATABASE_URL looks valid', false], + ['No schema changes detected for DATABASE_URL', false], ])('classifies %j as missing-URL=%s', async (stderr, missing) => { spawnMock.mockReturnValue({ status: 1, stdout: '', stderr }) await expect( @@ -1450,11 +1454,36 @@ describe('eqlMigrationCommand — Drizzle', () => { eqlMigrationCommand({ drizzle: true, out: join(tmp, 'drizzle') }), ).rejects.toBeInstanceOf(CliExit) expect(clack.log.error).toHaveBeenCalledWith('spawnSync pnpm ENOENT') - expect(clack.log.info).toHaveBeenCalledWith( - expect.stringContaining( - 'drizzle-kit generate --custom --name=install-eql', - ), - ) + // The one case where "check that it is installed" is the right advice: + // nothing ran, so there is no drizzle-kit output to look at and nothing to + // reproduce. Pointing at either would send the user in a circle. + const info = String(vi.mocked(clack.log.info).mock.calls.at(-1)?.[0]) + expect(info).toContain('could not be started') + expect(info).toContain('drizzle-kit --version') + expect(info).not.toContain('output is above') + }) + + /** + * A spawn failure that DID capture output is still a spawn failure: the + * runner never handed off to drizzle-kit, so the install guidance wins over + * both the reproduce-it line and the missing-URL classifier. + */ + it('prefers the not-launched guidance over anything in the streams', async () => { + spawnMock.mockReturnValue({ + status: null, + stdout: '', + stderr: 'DATABASE_URL is not set', + error: Object.assign(new Error('spawnSync pnpm ENOENT'), { + code: 'ENOENT', + }), + }) + + await expect( + eqlMigrationCommand({ drizzle: true, out: join(tmp, 'drizzle') }), + ).rejects.toBeInstanceOf(CliExit) + const info = String(vi.mocked(clack.log.info).mock.calls.at(-1)?.[0]) + expect(info).toContain('could not be started') + expect(info).not.toContain('could not read DATABASE_URL') }) it('falls back to the exit status when there is no stderr or spawn error', async () => { diff --git a/packages/cli/src/commands/eql/migration.ts b/packages/cli/src/commands/eql/migration.ts index d06a987f2..0e9cf3e69 100644 --- a/packages/cli/src/commands/eql/migration.ts +++ b/packages/cli/src/commands/eql/migration.ts @@ -107,17 +107,23 @@ export function parseReportedMigrationPath( * drizzle-kit's accurate diagnosis with remediation that contradicts it. * * Deliberately conservative: an unrecognised phrasing falls through to the - * generic follow-up, which is never wrong — drizzle-kit's own output is - * printed directly above either way. Note that bare `undefined` is NOT a - * trigger; `invalid connection string for DATABASE_URL: undefined scheme` - * is a malformed URL, not a missing one. + * generic follow-up, which stays safe because drizzle-kit's own output is + * printed directly above either way. Two things that are NOT triggers: + * + * - bare `undefined` AFTER the variable name — `invalid connection string for + * DATABASE_URL: undefined scheme` is a malformed URL, not a missing one; + * - the word `no`, which reads as strongly as `missing` to a regex but is far + * more common in ordinary prose (`No other issues found, DATABASE_URL looks + * valid`). Only unambiguous absence phrasings belong in that alternation. */ export function looksLikeMissingDatabaseUrl(output: string): boolean { return ( /DATABASE_URL\W{0,20}(?:is |was )?(?:not set|unset|not defined|is undefined|not provided|missing|empty|required)\b/i.test( output, ) || - /\b(?:missing|unset|undefined|no)\b[^\n]{0,40}?DATABASE_URL/i.test(output) + /\b(?:missing|unset|undefined|not set|not defined)\b[^\n]{0,40}?DATABASE_URL/i.test( + output, + ) ) } @@ -542,12 +548,22 @@ async function generateDrizzleEqlMigration( result.error?.message || `drizzle-kit exited with status ${result.status ?? 'unknown'}.`, ) + // Three follow-ups, narrowest first. `result.error` means the runner never + // started, so there IS no drizzle-kit output — telling the user to look + // above at output that does not exist, or to reproduce a command that + // cannot run, sends them in a circle. That case keeps the install advice + // the other two arms deliberately dropped. p.log.info( - looksLikeMissingDatabaseUrl(output) - ? messages.eql.migrationDrizzleKitNoDatabaseUrl(detectDotenvFile()) - : messages.eql.migrationDrizzleKitFailed( - `${execCommand(pm)} drizzle-kit generate --custom --name=${migrationName}`, - ), + result.error + ? messages.eql.migrationDrizzleKitNotLaunched( + command, + `${execCommand(pm)} drizzle-kit --version`, + ) + : looksLikeMissingDatabaseUrl(output) + ? messages.eql.migrationDrizzleKitNoDatabaseUrl(detectDotenvFile()) + : messages.eql.migrationDrizzleKitFailed( + `${execCommand(pm)} drizzle-kit generate --custom --name=${migrationName}`, + ), ) if (!embedded) p.outro('Migration aborted.') throw new CliExit(1) diff --git a/packages/cli/src/config/database-url.ts b/packages/cli/src/config/database-url.ts index c13467f35..0663bcc80 100644 --- a/packages/cli/src/config/database-url.ts +++ b/packages/cli/src/config/database-url.ts @@ -144,6 +144,18 @@ function trySupabaseStatus(): string | undefined { return undefined } +/** + * Is the Supabase tier in play — opted into with `--supabase`, or a project + * that clearly is one? Shared by both resolvers so the two cannot drift into + * disagreeing about when to shell out to `supabase status`. + */ +function shouldTrySupabase( + ctx: ResolveDatabaseUrlOptions, + cwd: string, +): boolean { + return Boolean(ctx.supabase) || detectSupabaseProject(cwd).hasConfigToml +} + async function promptForUrl(cwd: string): Promise { // Surface the alternative paths before prompting so users don't feel // like they're stuck in an interactive flow when a flag or env var @@ -219,8 +231,7 @@ export async function resolveDatabaseUrl( } // 3. Supabase fallback — opted-in, or the project clearly is one. - const supabaseProject = detectSupabaseProject(cwd) - if (ctx.supabase || supabaseProject.hasConfigToml) { + if (shouldTrySupabase(ctx, cwd)) { const fromSupabase = trySupabaseStatus() if (fromSupabase) { if (!ctx.quiet) p.log.info(messages.db.urlResolvedFromSupabase) @@ -283,7 +294,7 @@ export function tryResolveDatabaseUrl( const fromEnv = process.env.DATABASE_URL?.trim() if (fromEnv) return fromEnv - if (ctx.supabase || detectSupabaseProject(cwd).hasConfigToml) { + if (shouldTrySupabase(ctx, cwd)) { return trySupabaseStatus() } return undefined diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index 40ddc1626..b79509625 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -129,6 +129,20 @@ export const messages = { */ migrationDrizzleKitFailed: (versionCmd: string) => `drizzle-kit's own output is above. Reproduce it directly with \`${versionCmd}\` to see the failure without stash in the way.`, + /** + * The runner never started — `spawnSync` set `error` (ENOENT and friends), + * so neither stream carries anything and no drizzle-kit output exists to + * point at. + * + * This is the one case where "check that it is installed" was the right + * advice all along, so it comes back here rather than being lost with the + * blanket version of it (#924). It names the runner too: `pnpm exec + * drizzle-kit` fails with ENOENT when *pnpm* is missing, not drizzle-kit — + * a missing drizzle-kit gets as far as running and exits non-zero with its + * own message, which the other arms report. + */ + migrationDrizzleKitNotLaunched: (command: string, versionCmd: string) => + `\`${command}\` could not be started, so drizzle-kit never ran and printed nothing. Check that ${command} and drizzle-kit are both installed and on PATH: \`${versionCmd}\`.`, /** * drizzle-kit ran, read `drizzle.config.ts`, and the config itself blew up * on a missing `DATABASE_URL`.