fix(wizard,cli): report a part-way-failed ALTER COLUMN sweep as destructive (#786)

.changeset/sweep-partial-rewrite-destructive.md

@@ -0,0 +1,42 @@
+---
+'@cipherstash/wizard': patch
+'stash': patch
+---
+
+Close the last fail-open path in the Drizzle ALTER COLUMN sweep: a sweep that
+failed **after** it had already rewritten a file reported that directory as
+merely *unverified* instead of *destructive*.
+
+The sweep writes one migration file at a time. If the write of the second file
+failed — ENOSPC, a read-only file, an editor or `drizzle-kit` holding a lock —
+the whole call rejected and the list of files it had already rewritten was
+discarded with the stack frame. The wizard then reported zero rewrites for that
+directory and printed "the sweep could not check 1 directory (drizzle/)" over a
+prompt that made no mention of data loss, while a live `DROP COLUMN` sat on
+disk. The CLI's `stash eql migration --drizzle` and `stash eql install
+--drizzle` had the milder form: they warned about the directory but never named
+the files that had already become data-destroying. Those CLI reports now put
+the completed and attempted file list before the filesystem failure, so users
+see the possible damage before the reason the sweep stopped.
+
+The work already attempted now travels with the failure.
+`rewriteEncryptedAlterColumns` rejects with a `PartialRewriteError` carrying
+`rewritten` and `skipped` whenever it fails part way through a directory. The
+attempted file is included because a rejected filesystem write may already have
+truncated or partially replaced its destination. The wizard's directory sweep
+reports those arrays alongside the error instead of zeros. A directory in that
+state is reported as **both**: the rewrite paths are listed with the existing
+data-destroying warning, *and* the "sweep did not fully complete — review the
+sibling migrations" warning still fires, because both are true. The `Run the
+migration now?` prompt takes the destructive arm — defaulting to No and saying
+the migration DESTROYS data on a populated table — since that is the fact a user
+cannot afford to miss.
+
+If a wizard sweep flags a raw ALTER and then fails before rewriting anything,
+the prompt now preserves both facts: it keeps the flagged-statement guidance
+and names the migration directory that the sweep could not finish checking.
+
+A sweep that fails before recording any attempted rewrite or skipped statement
+is unchanged: it rejects with the original error, reports zero rewrite/skip
+results, and keeps the softer "nothing is known about this directory" wording,
+because claiming data destruction there would be a guess.

packages/cli/src/__tests__/rewrite-migrations.test.ts

@@ -1,12 +1,73 @@
 import fs from 'node:fs'
 import os from 'node:os'
 import path from 'node:path'
-import { afterEach, beforeEach, describe, expect, it } from 'vitest'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+
+// `node:fs/promises` stays REAL by default — every fixture below relies on the
+// genuine readdir/readFile/writeFile. Only `writeFile` is routed through a spy
+// that delegates to the real implementation, so the partial-write tests can
+// make the Nth write fail. A deterministic call-counting spy is used rather
+// than chmod: chmod-based simulation silently passes as root and behaves
+// differently across platforms.
+const fsWrite = vi.hoisted(() => ({
+  real: (() => {
+    throw new Error(
+      'fsWrite.real not initialised: node:fs/promises mock factory did not run',
+    )
+  }) as typeof import('node:fs/promises').writeFile,
+  spy: vi.fn(),
+}))
+vi.mock('node:fs/promises', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('node:fs/promises')>()
+  fsWrite.real = actual.writeFile
+  return { ...actual, default: actual, writeFile: fsWrite.spy }
+})
+
 import {
   describeSkipReason,
+  PartialRewriteError,
   rewriteEncryptedAlterColumns,
 } from '../commands/db/rewrite-migrations.js'
 
+beforeEach(() => {
+  fsWrite.spy.mockImplementation(fsWrite.real)
+})
+
+/**
+ * Arm the `writeFile` spy to throw on the `nth` (1-based) call, letting every
+ * other write through to the real implementation.
+ */
+const failWriteNumber = (nth: number, message: string): void => {
+  let calls = 0
+  fsWrite.spy.mockImplementation(
+    (...args: Parameters<typeof import('node:fs/promises').writeFile>) => {
+      calls += 1
+      if (calls === nth) return Promise.reject(new Error(message))
+      return fsWrite.real(...args)
+    },
+  )
+}
+
+/**
+ * Simulate a filesystem write that opens/truncates its destination before the
+ * operation rejects, as can happen when the device runs out of space.
+ */
+const failWriteAfterTruncatingNumber = (nth: number, message: string): void => {
+  let calls = 0
+  fsWrite.spy.mockImplementation(
+    async (
+      ...args: Parameters<typeof import('node:fs/promises').writeFile>
+    ) => {
+      calls += 1
+      if (calls === nth) {
+        await fsWrite.real(args[0], '', 'utf-8')
+        throw new Error(message)
+      }
+      await fsWrite.real(...args)
+    },
+  )
+}
+
 describe('rewriteEncryptedAlterColumns', () => {
   let tmpDir: string
 
@@ -1744,6 +1805,124 @@ describe('rewriteEncryptedAlterColumns', () => {
   })
 })
 
+// The sweep writes one file at a time. A throw on the SECOND file leaves the
+// first already rewritten on disk — holding a live `DROP COLUMN` — while the
+// call rejects. Every other failure path exercised here throws during the read
+// pass, BEFORE any write, so the partial case was invisible: the accumulated
+// `rewritten` array was discarded with the stack frame and both CLI callers
+// told the user to review the directory without naming the files that had
+// already become data-destroying (#786).
+describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () => {
+  let tmpDir: string
+
+  beforeEach(() => {
+    tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'stash-partial-'))
+  })
+
+  afterEach(() => {
+    fs.rmSync(tmpDir, { recursive: true, force: true })
+  })
+
+  /** Two independently rewritable files, plus the declaration both need. */
+  const seedTwoRewritable = (): { first: string; second: string } => {
+    fs.writeFileSync(
+      path.join(tmpDir, '0000_declare.sql'),
+      'CREATE TABLE "users" ("email" text, "name" text);\n',
+    )
+    const first = path.join(tmpDir, '0001_encrypt_email.sql')
+    fs.writeFileSync(
+      first,
+      'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n',
+    )
+    const second = path.join(tmpDir, '0002_encrypt_name.sql')
+    fs.writeFileSync(
+      second,
+      'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n',
+    )
+    return { first, second }
+  }
+
+  it('rejects with completed and attempted rewrite files', async () => {
+    const { first, second } = seedTwoRewritable()
+    // Files are swept in sorted order and 0000 needs no write, so write #1 is
+    // 0001 and write #2 — the one that fails — is 0002.
+    failWriteNumber(2, 'ENOSPC: no space left on device')
+
+    const error = await rewriteEncryptedAlterColumns(tmpDir).catch(
+      (err: unknown) => err,
+    )
+
+    expect(error).toBeInstanceOf(PartialRewriteError)
+    // The original failure must survive the wrapping verbatim — the callers
+    // render `err.message` straight to the user.
+    expect((error as PartialRewriteError).message).toBe(
+      'ENOSPC: no space left on device',
+    )
+    expect((error as PartialRewriteError).rewritten).toEqual([first, second])
+    // Not a bookkeeping claim: that file really does hold a live DROP COLUMN.
+    expect(fs.readFileSync(first, 'utf-8')).toContain('DROP COLUMN')
+    expect(fs.readFileSync(second, 'utf-8')).toContain('SET DATA TYPE')
+  })
+
+  it('reports an attempted file when its rejected write already truncated it', async () => {
+    const { first, second } = seedTwoRewritable()
+    failWriteAfterTruncatingNumber(1, 'ENOSPC: no space left on device')
+
+    const error = await rewriteEncryptedAlterColumns(tmpDir).catch(
+      (err: unknown) => err,
+    )
+
+    expect(error).toBeInstanceOf(PartialRewriteError)
+    expect((error as PartialRewriteError).rewritten).toEqual([first])
+    expect(fs.readFileSync(first, 'utf-8')).toBe('')
+    expect(fs.readFileSync(second, 'utf-8')).toContain('SET DATA TYPE')
+  })
+
+  it('carries the near-misses it had already flagged', async () => {
+    fs.writeFileSync(
+      path.join(tmpDir, '0000_declare.sql'),
+      'CREATE TABLE "users" ("email" text, "name" text);\n',
+    )
+    const nearMiss = path.join(tmpDir, '0001_using.sql')
+    fs.writeFileSync(
+      nearMiss,
+      'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search USING (email)::eql_v3_text_search;\n',
+    )
+    const attempted = path.join(tmpDir, '0002_encrypt_name.sql')
+    fs.writeFileSync(
+      attempted,
+      'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n',
+    )
+    // 0001 is flagged, never written, so the FIRST write is 0002's.
+    failWriteNumber(1, 'EACCES: permission denied')
+
+    const error = await rewriteEncryptedAlterColumns(tmpDir).catch(
+      (err: unknown) => err,
+    )
+
+    expect(error).toBeInstanceOf(PartialRewriteError)
+    expect((error as PartialRewriteError).rewritten).toEqual([attempted])
+    expect((error as PartialRewriteError).skipped).toEqual([
+      expect.objectContaining({ file: nearMiss, reason: 'unrecognised-form' }),
+    ])
+  })
+
+  // The other half of the contract. A throw with no work behind it must keep
+  // rejecting with the ORIGINAL error — its `code` and identity intact — so the
+  // "this directory went unchecked" path stays exactly as it was.
+  it('rethrows the original error when nothing had been done yet', async () => {
+    fs.mkdirSync(path.join(tmpDir, '0001_broken.sql'))
+
+    const error = await rewriteEncryptedAlterColumns(tmpDir).catch(
+      (err: unknown) => err,
+    )
+
+    expect(error).toBeInstanceOf(Error)
+    expect(error).not.toBeInstanceOf(PartialRewriteError)
+    expect((error as NodeJS.ErrnoException).code).toBe('EISDIR')
+  })
+})
+
 // The three reasons drive very different user action — re-encrypt through the
 // staged lifecycle, fix a hand-authored cast, or go check the database. A
 // switch with no `default` arm means a missing case fails the build, but a

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

@@ -59,6 +59,22 @@ vi.mock('node:fs', async (importOriginal) => {
   return { ...actual, default: actual, writeFileSync: fsWrite.spy }
 })
 
+// Same seam on the async side. Step 5's ALTER-COLUMN sweep is the only thing
+// here that writes through `node:fs/promises`, so a call-counting spy makes a
+// mid-sweep write failure deterministic — no chmod, which passes silently as
+// root and varies by platform.
+const fsWriteAsync = vi.hoisted(() => ({
+  real: (() => {
+    throw new Error('fsWriteAsync.real not initialised')
+  }) as typeof import('node:fs/promises').writeFile,
+  spy: vi.fn(),
+}))
+vi.mock('node:fs/promises', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('node:fs/promises')>()
+  fsWriteAsync.real = actual.writeFile
+  return { ...actual, default: actual, writeFile: fsWriteAsync.spy }
+})
+
 // The `--latest` path calls `downloadEqlSql`; stub just that export so we can
 // make the network fetch reject. Everything else in the installer module
 // (including the real `loadBundledEqlSql` the other tests rely on) stays real.
@@ -85,6 +101,7 @@ beforeEach(() => {
   // Default: `writeFileSync` behaves exactly like the real thing. Tests that
   // need it to fail override the implementation for their own scope.
   fsWrite.spy.mockImplementation(fsWrite.real)
+  fsWriteAsync.spy.mockImplementation(fsWriteAsync.real)
 })
 
 afterEach(() => {
@@ -238,6 +255,109 @@ describe('generateDrizzleMigration', () => {
     )
   })
 
+  // The sweep writes one file at a time, so a failure part way through leaves
+  // earlier files already rewritten — each holding a live `DROP COLUMN`. The
+  // catch used to warn about the directory without naming them, so the user was
+  // sent to "review the sibling migrations" with no idea which ones had already
+  // become data-destroying (#786).
+  it('reports completed, attempted, and skipped files when the sweep fails part way', async () => {
+    const out = join(tmp, 'drizzle')
+    mkdirSync(out, { recursive: true })
+    writeFileSync(
+      join(out, '0000_declare.sql'),
+      'CREATE TABLE "users" ("email" text, "name" text);\n',
+    )
+    const skipped = join(out, '0001_using-email.sql')
+    writeFileSync(
+      skipped,
+      'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "public"."eql_v3_text_search" USING (email)::eql_v3_text_search;\n',
+    )
+    const rewritten = join(out, '0002_encrypt-email.sql')
+    writeFileSync(
+      rewritten,
+      'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "public"."eql_v3_text_search";\n',
+    )
+    const attempted = join(out, '0003_encrypt-name.sql')
+    writeFileSync(
+      attempted,
+      'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE "public"."eql_v3_text_search";\n',
+    )
+    spawnMock.mockImplementation(() => {
+      writeFileSync(join(out, '0004_install-eql.sql'), '')
+      return { status: 0, stdout: '', stderr: '' }
+    })
+    // The generated migration is `skip`ped and 0001 is only flagged, so the
+    // writes are #1 for 0002 and #2 — the rejected one — for 0003.
+    let asyncWrites = 0
+    fsWriteAsync.spy.mockImplementation(
+      async (
+        ...args: Parameters<typeof import('node:fs/promises').writeFile>
+      ): Promise<void> => {
+        asyncWrites += 1
+        if (asyncWrites === 2) {
+          await fsWriteAsync.real(args[0], '', 'utf-8')
+          throw new Error('ENOSPC: no space left on device')
+        }
+        await fsWriteAsync.real(...args)
+      },
+    )
+
+    await generateDrizzleMigration(spinner, { out })
+
+    // The first file really was rewritten and is sitting there destructive.
+    expect(readFileSync(rewritten, 'utf-8')).toContain('DROP COLUMN')
+    // The rejected write really did mutate its destination too.
+    expect(readFileSync(attempted, 'utf-8')).toBe('')
+    // Both possible rewrites and the earlier near-miss must survive the catch.
+    const stepped = clack.log.step.mock.calls.map((c) => String(c[0]))
+    expect(stepped.some((msg) => msg.includes(rewritten))).toBe(true)
+    expect(stepped.some((msg) => msg.includes(attempted))).toBe(true)
+    expect(stepped.some((msg) => msg.includes(skipped))).toBe(true)
+    const infos = clack.log.info.mock.calls.map((c) => String(c[0]))
+    expect(infos.some((msg) => msg.includes('Rewrote 2 migration file'))).toBe(
+      true,
+    )
+    // And the failure itself is still reported, with the closing warning.
+    const warned = clack.log.warn.mock.calls.map((c) => String(c[0]))
+    expect(warned.some((msg) => msg.includes('ENOSPC'))).toBe(true)
+    expect(
+      warned.some((msg) => msg.includes('1 ALTER-to-encrypted statement')),
+    ).toBe(true)
+    expect(warned.some((msg) => msg.includes('did not fully complete'))).toBe(
+      true,
+    )
+
+    // Put the potentially damaged paths and destructive context before the
+    // write failure, then close with the incomplete-sweep warning.
+    const rewriteInfoIndex = infos.findIndex((msg) =>
+      msg.includes('Rewrote 2 migration file'),
+    )
+    const rewrittenStepIndex = stepped.findIndex((msg) =>
+      msg.includes(rewritten),
+    )
+    const attemptedStepIndex = stepped.findIndex((msg) =>
+      msg.includes(attempted),
+    )
+    const failureWarnIndex = warned.findIndex((msg) => msg.includes('ENOSPC'))
+    const closingWarnIndex = warned.findIndex((msg) =>
+      msg.includes('did not fully complete'),
+    )
+    const failureOrder =
+      clack.log.warn.mock.invocationCallOrder[failureWarnIndex]
+    expect(
+      clack.log.info.mock.invocationCallOrder[rewriteInfoIndex],
+    ).toBeLessThan(failureOrder)
+    expect(
+      clack.log.step.mock.invocationCallOrder[rewrittenStepIndex],
+    ).toBeLessThan(failureOrder)
+    expect(
+      clack.log.step.mock.invocationCallOrder[attemptedStepIndex],
+    ).toBeLessThan(failureOrder)
+    expect(failureOrder).toBeLessThan(
+      clack.log.warn.mock.invocationCallOrder[closingWarnIndex],
+    )
+  })
+
   it('includes --out in the dry-run preview', async () => {
     const out = join(tmp, 'custom-out')
     await generateDrizzleMigration(spinner, { dryRun: true, out })