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..d46fda8aa 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,217 @@ 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', + ) + }) + + /** + * 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], + // `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( + 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( @@ -1216,11 +1436,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, @@ -1234,9 +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('Make sure drizzle-kit is installed'), - ) + // 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 efae6d2c7..0e9cf3e69 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,76 @@ 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 + // 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 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 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|not set|not defined)\b[^\n]{0,40}?DATABASE_URL/i.test( + output, + ) + ) +} + function cleanupMigrationFile(filePath: string | undefined): void { if (!filePath) return try { @@ -102,6 +176,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 /** @@ -370,10 +457,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 +483,6 @@ async function generateDrizzleEqlMigration( 'generate', '--custom', `--name=${migrationName}`, - `--out=${outDir}`, ] const displayCmd = `${execCommand(pm)} ${drizzleArgs.slice(prefixArgs.length).join(' ')}` @@ -401,7 +502,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 +513,97 @@ 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: 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 = + options.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'}.`, ) + // 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( - `Make sure drizzle-kit is installed and configured: ${execCommand(pm)} drizzle-kit --version`, + 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) } 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 +628,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..2f218c104 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()) { @@ -159,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') diff --git a/packages/cli/src/config/database-url.ts b/packages/cli/src/config/database-url.ts index 5d5b91a18..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) @@ -254,3 +265,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 (shouldTrySupabase(ctx, cwd)) { + return trySupabaseStatus() + } + return undefined +} diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index 8dc5db157..b79509625 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -116,6 +116,58 @@ 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.`, + /** + * 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`. + * + * 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