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` had the milder form: it warned
+about the directory but never named the files that had already become
+data-destroying. That CLI report now puts 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