fix(cli): validate --name and pass --out in the v2 Drizzle migration generator

.changeset/khaki-pugs-play.md

@@ -0,0 +1,10 @@
+---
+'stash': patch
+---
+
+Fix two defects in the Drizzle migration generator used by `stash eql install --drizzle` (EQL v2):
+
+- **`--name` is now validated and no longer reaches a shell.** The migration name was interpolated into a shell command string, so a name containing shell metacharacters (e.g. `--name 'x; rm -rf ~'`) was executed. `--name` is now restricted to letters, numbers, dashes, and underscores, and drizzle-kit is invoked with an argv array instead of a shell string.
+- **`--out` is now actually passed to drizzle-kit.** The flag was used to search for the generated migration but never handed to `drizzle-kit generate`, so any project whose `drizzle.config.ts` writes migrations outside `drizzle/` had the file written in one place and searched for in another, failing with "migration file not found".
+
+`stash eql migration --drizzle` (EQL v3) already had both fixes and is unchanged.

packages/cli/src/commands/db/__tests__/generate-drizzle-migration.test.ts

@@ -0,0 +1,146 @@
+import {
+  mkdirSync,
+  mkdtempSync,
+  readFileSync,
+  rmSync,
+  writeFileSync,
+} from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import * as p from '@clack/prompts'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { CliExit } from '../../../cli/exit.js'
+import { messages } from '../../../messages.js'
+
+// clack is chrome — silence it and spy on the channels the generator reports
+// through. The spinner instance doubles as the `s` argument.
+const clack = vi.hoisted(() => ({
+  spinnerInstance: { start: vi.fn(), stop: vi.fn() },
+  log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), success: vi.fn() },
+  intro: vi.fn(),
+  note: vi.fn(),
+  outro: vi.fn(),
+}))
+vi.mock('@clack/prompts', () => ({
+  spinner: vi.fn(() => clack.spinnerInstance),
+  log: clack.log,
+  intro: clack.intro,
+  note: clack.note,
+  outro: clack.outro,
+}))
+
+// Only the child process is faked — everything else (fs, bundled SQL) is real.
+const spawnMock = vi.hoisted(() => vi.fn())
+vi.mock('node:child_process', () => ({ spawnSync: spawnMock }))
+
+// Pin the package manager so the argv assertion below is exact. Detection reads
+// the lockfile in cwd and npm_config_user_agent, both of which vary by how the
+// suite was launched. The runner MAPPING stays real — `pnpm` + `['dlx', …]` is
+// part of what's being asserted, so mocking it would defeat the test.
+vi.mock('@/commands/init/utils.js', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('@/commands/init/utils.js')>()),
+  detectPackageManager: () => 'pnpm',
+}))
+
+const { generateDrizzleMigration } = await import('../install.js')
+
+const spinner = p.spinner()
+
+afterEach(() => {
+  vi.clearAllMocks()
+})
+
+/**
+ * The v2 (`eql install --drizzle`) generator. Both regressions pinned here are
+ * invocation-level: an unvalidated `--name` reaching a shell string, and
+ * `--out` being computed for the search but never handed to drizzle-kit.
+ */
+describe('generateDrizzleMigration', () => {
+  let tmp: string
+  beforeEach(() => {
+    tmp = mkdtempSync(join(tmpdir(), 'stash-v2-drizzle-migration-'))
+  })
+  afterEach(() => {
+    rmSync(tmp, { recursive: true, force: true })
+  })
+
+  it('rejects a migration name with unsafe characters before spawning', async () => {
+    await expect(
+      generateDrizzleMigration(spinner, {
+        name: 'x; rm -rf ~',
+        out: join(tmp, 'drizzle'),
+      }),
+    ).rejects.toBeInstanceOf(CliExit)
+    expect(clack.log.error).toHaveBeenCalledWith(messages.eql.migrationBadName)
+    expect(spawnMock).not.toHaveBeenCalled()
+  })
+
+  it.each([
+    ['command substitution', 'a$(whoami)'],
+    ['backticks', 'a`id`'],
+    ['a space', 'add eql'],
+    ['a path separator', '../escape'],
+  ])('rejects %s in --name', async (_label, name) => {
+    await expect(
+      generateDrizzleMigration(spinner, { name, out: join(tmp, 'drizzle') }),
+    ).rejects.toBeInstanceOf(CliExit)
+    expect(spawnMock).not.toHaveBeenCalled()
+  })
+
+  it('rejects an unsafe name in a dry run too (validation precedes the preview)', async () => {
+    await expect(
+      generateDrizzleMigration(spinner, { name: 'x; ls', dryRun: true }),
+    ).rejects.toBeInstanceOf(CliExit)
+    expect(clack.note).not.toHaveBeenCalled()
+  })
+
+  it('passes --name and --out to drizzle-kit as argv (no shell) and writes the SQL', async () => {
+    const out = join(tmp, 'db', 'migrations')
+    mkdirSync(out, { recursive: true })
+    // Stand in for drizzle-kit scaffolding an empty custom migration.
+    spawnMock.mockImplementation(() => {
+      writeFileSync(join(out, '0000_add-eql.sql'), '')
+      return { status: 0, stdout: '', stderr: '' }
+    })
+
+    await generateDrizzleMigration(spinner, { name: 'add-eql', out })
+
+    expect(spawnMock).toHaveBeenCalledTimes(1)
+    const [command, argv] = spawnMock.mock.calls[0]
+    // The whole argv, exactly — not `toContain` checks, which would still pass
+    // if the runner prefix (`dlx`) were dropped and drizzle-kit ran under the
+    // wrong resolver. DEFECT 1: name and out are discrete inert tokens in an
+    // array, never interpolated into a shell string. DEFECT 2: `--out` is
+    // actually passed, so drizzle-kit writes where step 2 then looks.
+    expect(command).toBe('pnpm')
+    expect(argv).toEqual([
+      'dlx',
+      'drizzle-kit',
+      'generate',
+      '--custom',
+      '--name=add-eql',
+      `--out=${out}`,
+    ])
+
+    const written = readFileSync(join(out, '0000_add-eql.sql'), 'utf-8')
+    expect(written).toContain('cs_migrations')
+  })
+
+  it('includes --out in the dry-run preview', async () => {
+    const out = join(tmp, 'custom-out')
+    await generateDrizzleMigration(spinner, { dryRun: true, out })
+    expect(spawnMock).not.toHaveBeenCalled()
+    expect(clack.note).toHaveBeenCalledWith(
+      expect.stringContaining(`--out=${out}`),
+      'Dry Run',
+    )
+  })
+
+  it('aborts with CliExit when drizzle-kit exits non-zero', async () => {
+    spawnMock.mockReturnValue({ status: 1, stdout: '', stderr: 'boom' })
+    await expect(
+      generateDrizzleMigration(spinner, { out: join(tmp, 'drizzle') }),
+    ).rejects.toBeInstanceOf(CliExit)
+    expect(clack.log.error).toHaveBeenCalledWith('boom')
+  })
+})

packages/cli/src/commands/db/install.ts

@@ -1,4 +1,4 @@
-import { execSync } from 'node:child_process'
+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'
@@ -8,7 +8,12 @@ import {
 } from '@cipherstash/migrate'
 import * as p from '@clack/prompts'
 import pg from 'pg'
-import { detectPackageManager, runnerCommand } from '@/commands/init/utils.js'
+import { CliExit } from '@/cli/exit.js'
+import {
+  detectPackageManager,
+  runnerArgv,
+  runnerCommand,
+} from '@/commands/init/utils.js'
 import { resolveDatabaseUrl } from '@/config/database-url.js'
 import { findConfigFile, loadStashConfig } from '@/config/index.js'
 import { isInteractive } from '@/config/tty.js'
@@ -37,6 +42,16 @@ import {
 const DEFAULT_MIGRATION_NAME = 'install-eql'
 const DEFAULT_DRIZZLE_OUT = 'drizzle'
 
+/**
+ * File-system-safe migration name: what drizzle-kit accepts, and shell-inert.
+ *
+ * Lives here rather than in `commands/eql/migration.ts` (the v3 generator) only
+ * because that module already imports from this one — keeping the constant on
+ * this side of the existing edge avoids an import cycle. Both generators share
+ * it; there must not be a second copy.
+ */
+export const SAFE_MIGRATION_NAME = /^[\w-]+$/
+
 export interface InstallOptions {
   force?: boolean
   dryRun?: boolean
@@ -467,8 +482,12 @@ export function printNextSteps(): void {
  * Uses `drizzle-kit generate --custom` to scaffold an empty migration,
  * then loads the EQL install SQL (bundled by default, or from GitHub with
  * `--latest`) and writes it into the file.
+ *
+ * Exported for unit tests; not part of the CLI's public surface. Failures
+ * throw {@link CliExit} rather than calling `process.exit` so the telemetry
+ * `finally` in `main.ts` still runs (and the branches stay testable).
  */
-async function generateDrizzleMigration(
+export async function generateDrizzleMigration(
   s: ReturnType<typeof p.spinner>,
   options: {
     name?: string
@@ -480,8 +499,29 @@ async function generateDrizzleMigration(
   },
 ) {
   const migrationName = options.name ?? DEFAULT_MIGRATION_NAME
+  if (!SAFE_MIGRATION_NAME.test(migrationName)) {
+    p.log.error(messages.eql.migrationBadName)
+    p.outro('Migration aborted.')
+    throw new CliExit(1)
+  }
   const outDir = resolve(options.out ?? DEFAULT_DRIZZLE_OUT)
-  const drizzleCmd = `${runnerCommand(detectPackageManager(), '').trim()} drizzle-kit generate --custom --name=${migrationName}`
+
+  // 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.
+  const pm = detectPackageManager()
+  const { command, prefixArgs } = runnerArgv(pm)
+  const drizzleArgs = [
+    ...prefixArgs,
+    'drizzle-kit',
+    'generate',
+    '--custom',
+    `--name=${migrationName}`,
+    `--out=${outDir}`,
+  ]
+  const drizzleCmd = `${runnerCommand(pm, '').trim()} ${drizzleArgs.slice(prefixArgs.length).join(' ')}`
 
   if (options.dryRun) {
     p.log.info('Dry run — no changes will be made.')
@@ -501,32 +541,23 @@ async function generateDrizzleMigration(
   // Step 1: Generate a custom Drizzle migration
   s.start('Generating custom Drizzle migration...')
 
-  try {
-    execSync(drizzleCmd, {
-      stdio: 'pipe',
-      encoding: 'utf-8',
-    })
-    s.stop('Custom Drizzle migration generated.')
-  } catch (error) {
+  const result = spawnSync(command, drizzleArgs, {
+    stdio: 'pipe',
+    encoding: 'utf-8',
+  })
+  if (result.status !== 0) {
     s.stop('Failed to generate migration.')
-    const stderr =
-      error !== null &&
-      typeof error === 'object' &&
-      'stderr' in error &&
-      typeof error.stderr === 'string'
-        ? error.stderr.trim()
-        : undefined
-    if (stderr) {
-      p.log.error(stderr)
-    } else {
-      p.log.error(
-        error instanceof Error ? error.message : 'Unknown error occurred.',
-      )
-    }
+    const stderr = result.stderr?.trim()
+    p.log.error(
+      stderr ||
+        result.error?.message ||
+        `drizzle-kit exited with status ${result.status ?? 'unknown'}.`,
+    )
     p.log.info('Make sure drizzle-kit is installed: npm install -D drizzle-kit')
     p.outro('Migration aborted.')
-    process.exit(1)
+    throw new CliExit(1)
   }
+  s.stop('Custom Drizzle migration generated.')
 
   // Step 2: Find the generated migration file
   s.start('Locating generated migration file...')
@@ -537,8 +568,11 @@ async function generateDrizzleMigration(
   } 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 <dir> so it matches.',
+    )
     p.outro('Migration aborted.')
-    process.exit(1)
+    throw new CliExit(1)
   }
 
   // Step 3: Load the EQL SQL (bundled or from GitHub). Thread supabase /
@@ -560,7 +594,7 @@ async function generateDrizzleMigration(
       p.log.error(error instanceof Error ? error.message : String(error))
       cleanupMigrationFile(generatedMigrationPath)
       p.outro('Migration aborted.')
-      process.exit(1)
+      throw new CliExit(1)
     }
   } else {
     s.start('Loading bundled EQL install script...')
@@ -572,7 +606,7 @@ async function generateDrizzleMigration(
       p.log.error(error instanceof Error ? error.message : String(error))
       cleanupMigrationFile(generatedMigrationPath)
       p.outro('Migration aborted.')
-      process.exit(1)
+      throw new CliExit(1)
     }
   }
 
@@ -592,7 +626,7 @@ async function generateDrizzleMigration(
     p.log.error(error instanceof Error ? error.message : String(error))
     cleanupMigrationFile(generatedMigrationPath)
     p.outro('Migration aborted.')
-    process.exit(1)
+    throw new CliExit(1)
   }
 
   // Step 5: Sweep for sibling migrations that drizzle-kit may have emitted

packages/cli/src/commands/eql/migration.ts

@@ -8,6 +8,7 @@ import {
   cleanupMigrationFile,
   findGeneratedMigration,
   printNextSteps,
+  SAFE_MIGRATION_NAME,
 } from '@/commands/db/install.js'
 import {
   detectPackageManager,
@@ -20,9 +21,6 @@ import { messages } from '@/messages.js'
 const DEFAULT_MIGRATION_NAME = 'install-eql'
 const DEFAULT_DRIZZLE_OUT = 'drizzle'
 
-/** File-system-safe migration name: what drizzle-kit accepts, and shell-inert. */
-const SAFE_MIGRATION_NAME = /^[\w-]+$/
-
 export interface EqlMigrationOptions {
   /** Emit a Drizzle custom migration. */
   drizzle?: boolean

packages/cli/src/commands/init/steps/__tests__/install-eql.test.ts

@@ -19,6 +19,7 @@ vi.mock('@clack/prompts', () => ({
 }))
 
 import * as p from '@clack/prompts'
+import { CliExit } from '../../../../cli/exit.js'
 import { isInteractive } from '../../../../config/tty.js'
 import { installCommand } from '../../../db/install.js'
 import { installEqlStep } from '../install-eql.js'
@@ -103,4 +104,36 @@ describe('installEqlStep', () => {
     expect(result.eqlInstalled).toBe(false)
     expect(result.eqlMigrationPending).toBe(true)
   })
+
+  it('re-throws CliExit instead of reframing it as a connection failure', async () => {
+    // `installCommand` throws CliExit for hard stops it has ALREADY reported on
+    // with its own actionable error (e.g. an unsafe `--name`). The broad catch
+    // below it must not swallow that: doing so prints "check your database
+    // connection" for a problem that has nothing to do with the database, and
+    // lets init continue past a hard stop. Re-throwing unwinds to `run()`,
+    // which records the outcome and exits with the carried code.
+    vi.mocked(installCommand).mockRejectedValueOnce(new CliExit(1))
+
+    await expect(
+      installEqlStep.run(baseState, provider),
+    ).rejects.toBeInstanceOf(CliExit)
+    expect(p.log.error).not.toHaveBeenCalled()
+  })
+
+  it('still swallows a non-CliExit failure and lets init continue', async () => {
+    // The contrast that gives the test above its meaning: an ordinary throw is
+    // reported generically and init carries on. The message is deliberately
+    // generic — Postgres client errors routinely carry the connection string,
+    // credentials included, so the underlying error is never echoed.
+    vi.mocked(installCommand).mockRejectedValueOnce(
+      new Error('connect ECONNREFUSED'),
+    )
+
+    const result = await installEqlStep.run(baseState, provider)
+
+    expect(result.eqlInstalled).toBe(false)
+    expect(p.log.error).toHaveBeenCalledWith(
+      'EQL install failed — check your database connection and try again.',
+    )
+  })
 })