From c7deeb3efeb7966031ba293f021f869865087918 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 28 Jul 2026 11:20:41 +1000 Subject: [PATCH 1/5] fix(wizard,cli): report a part-way-failed ALTER COLUMN sweep as destructive (#786) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Drizzle migration rewriter writes one file at a time. When it threw *after* rewriting a file — ENOSPC, a read-only file, a lock held by an editor or drizzle-kit — the accumulated `rewritten` array died with the stack frame. `sweepMigrationDirs` then hard-coded `rewritten: []` on the error result, and `rewriteEncryptedMigrations` `continue`d past the whole reporting block, so the wizard printed "the sweep could not check 1 directory (drizzle/)" over a prompt that never mentioned data loss — while a live DROP COLUMN sat on disk. The last fail-open path in the A-2 sweep, in exactly the subsystem where it costs the most. `rewriteEncryptedAlterColumns` now rejects with a `PartialRewriteError` carrying `rewritten` and `skipped` whenever it fails part way through a directory it has already changed, preserving the underlying message verbatim and keeping the original as `cause`. A failure with no work behind it still rejects with the original error untouched, so the "nothing is known about this directory" arm is byte-for-byte unchanged. `sweepMigrationDirs` propagates those arrays onto the error result, and the wizard's report no longer skips a directory that errored: the rewritten files are listed with the existing data-destroying warning, the "did not fully complete" warning still fires, and the prompt takes the destructive arm (initialValue: false, "DESTROYS data"). Both facts are true; the destructive one wins because it is the one the user cannot afford to miss. `stash eql migration --drizzle` and `stash eql install --drizzle` likewise name any files the sweep rewrote before it failed, instead of pointing at the directory and leaving the user to guess. The rewriter change lands identically in both mirrored copies (scripts/__tests__/rewriter-copies-in-sync.test.mjs is green). Tests use a delegating `node:fs/promises` writeFile spy that fails the Nth call — deterministic, unlike chmod, which passes silently as root. Coverage spans the rewriter (both copies), the directory sweep, the wizard report driven end to end through a real failing write, and both CLI callers. Skill updated: skills/stash-cli documents what a part-way failure now reports. --- .../sweep-partial-rewrite-destructive.md | 33 +++ .../src/__tests__/rewrite-migrations.test.ts | 154 +++++++++++- .../cli/src/commands/db/rewrite-migrations.ts | 213 +++++++++------- .../commands/eql/__tests__/migration.test.ts | 78 ++++++ packages/cli/src/commands/eql/migration.ts | 51 ++-- .../wizard/src/__tests__/post-agent.test.ts | 142 +++++++++++ .../src/__tests__/rewrite-migrations.test.ts | 195 ++++++++++++++- packages/wizard/src/lib/post-agent.ts | 41 ++- packages/wizard/src/lib/rewrite-migrations.ts | 234 +++++++++++------- skills/stash-cli/SKILL.md | 2 + 10 files changed, 945 insertions(+), 198 deletions(-) create mode 100644 .changeset/sweep-partial-rewrite-destructive.md diff --git a/.changeset/sweep-partial-rewrite-destructive.md b/.changeset/sweep-partial-rewrite-destructive.md new file mode 100644 index 000000000..e7ca2f944 --- /dev/null +++ b/.changeset/sweep-partial-rewrite-destructive.md @@ -0,0 +1,33 @@ +--- +'@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. + +The work already done now travels with the failure. `rewriteEncryptedAlterColumns` +rejects with a `PartialRewriteError` carrying `rewritten` and `skipped` whenever +it fails part way through a directory it has already changed, and the wizard's +directory sweep reports those arrays alongside the error instead of zeros. A +directory in that state is reported as **both**: the rewritten files 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. + +A sweep that fails before changing anything is unchanged: it rejects with the +original error, reports zeros, and keeps the softer "nothing is known about this +directory" wording, because claiming data destruction there would be a guess. diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 4a0d231bd..89ffc456c 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -1,12 +1,53 @@ 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() + 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) => { + calls += 1 + if (calls === nth) return Promise.reject(new Error(message)) + return fsWrite.real(...args) + }, + ) +} + describe('rewriteEncryptedAlterColumns', () => { let tmpDir: string @@ -1744,6 +1785,117 @@ 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 the files it had already rewritten', 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]) + // 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('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', + ) + fs.writeFileSync( + path.join(tmpDir, '0002_encrypt_name.sql'), + '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([]) + 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.writeFileSync( + path.join(tmpDir, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + fs.writeFileSync( + path.join(tmpDir, '0001_encrypt_email.sql'), + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + failWriteNumber(1, 'EROFS: read-only file system') + + const error = await rewriteEncryptedAlterColumns(tmpDir).catch( + (err: unknown) => err, + ) + + expect(error).toBeInstanceOf(Error) + expect(error).not.toBeInstanceOf(PartialRewriteError) + expect((error as Error).message).toBe('EROFS: read-only file system') + }) +}) + // 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 diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 2e0c33c8b..7297f453d 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -658,6 +658,38 @@ export interface RewriteResult { skipped: SkippedAlter[] } +/** + * Thrown by {@link rewriteEncryptedAlterColumns} when it fails PART WAY through + * a directory it has already changed. + * + * The sweep writes one file at a time, so a failure on the third file leaves + * the first two rewritten on disk — each holding a live `DROP COLUMN`. A bare + * rejection discards that fact: the accumulated `rewritten` array dies with the + * stack frame, and every caller then reports the directory as merely + * *unchecked*, telling the user to review those migrations without saying that + * some of them now destroy data. That is the fail-open inversion this rewriter + * exists to prevent, so the work already done travels with the failure (#786). + * + * `message` is the underlying failure's, verbatim — callers render it straight + * to the user — and the original is kept as `cause`. Thrown ONLY when there is + * partial work to report: a sweep that fails before changing or flagging + * anything rejects with the original error untouched, because "nothing is known + * about this directory" is a different state, and not a destructive one. + */ +export class PartialRewriteError extends Error { + /** Absolute paths of the files already rewritten when the failure hit. */ + readonly rewritten: string[] + /** Near-miss statements already flagged when the failure hit. */ + readonly skipped: SkippedAlter[] + + constructor(cause: unknown, rewritten: string[], skipped: SkippedAlter[]) { + super(cause instanceof Error ? cause.message : String(cause), { cause }) + this.name = 'PartialRewriteError' + this.rewritten = [...rewritten] + this.skipped = [...skipped] + } +} + /** * Replace in-place `ALTER COLUMN ... SET DATA TYPE ` * statements with an ADD + DROP + RENAME sequence. @@ -735,93 +767,108 @@ export async function rewriteEncryptedAlterColumns( const { encrypted: encryptedColumns, declared: declaredColumns } = indexColumnDeclarations([...contents.values()]) - for (const [filePath, original] of contents) { - if (options.skip && filePath === options.skip) continue - - // Reset the regex's lastIndex — it's stateful on /g - ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 - - const updated = original.replace( - ALTER_COLUMN_TO_ENCRYPTED_RE, - ( - match: string, - first: string, - second: string | undefined, - column: string, - mangledType: string, - offset: number, - ) => { - // Commented-out SQL never runs, and a multi-line replacement would only - // inherit the `-- ` on its first line — leaving the rest live. - if (isInsideCommentOrString(original, offset)) return match - - const { schema, table } = tableOf(first, second) - - // Already encrypted: the ADD+DROP+RENAME would drop the ciphertext and - // there is no plaintext left to backfill from. Flag, never guess. - if (encryptedColumns.has(columnKey(table, column, schema))) { - skip(filePath, match.trim(), 'already-encrypted') - return match - } + // Wrapped, not left to reject bare: from the first `writeFile` on, a failure + // anywhere in this loop leaves the directory PART rewritten. See + // {@link PartialRewriteError} — the reads above it cannot have changed + // anything, so they still reject with their own error. + try { + for (const [filePath, original] of contents) { + if (options.skip && filePath === options.skip) continue + + // Reset the regex's lastIndex — it's stateful on /g + ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 + + const updated = original.replace( + ALTER_COLUMN_TO_ENCRYPTED_RE, + ( + match: string, + first: string, + second: string | undefined, + column: string, + mangledType: string, + offset: number, + ) => { + // Commented-out SQL never runs, and a multi-line replacement would only + // inherit the `-- ` on its first line — leaving the rest live. + if (isInsideCommentOrString(original, offset)) return match + + const { schema, table } = tableOf(first, second) + + // Already encrypted: the ADD+DROP+RENAME would drop the ciphertext and + // there is no plaintext left to backfill from. Flag, never guess. + if (encryptedColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'already-encrypted') + return match + } + + // Fail closed. Absence from `declaredColumns` does not mean the column + // is plaintext — it means the corpus never said. The declaration can + // sit in a directory this sweep does not read (the wizard ships with + // three candidates and indexes each separately), so rewriting on the + // assumption is how a live `DROP COLUMN` reaches a populated — possibly + // already-encrypted — column. Flag it and let the user look. + if (!declaredColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'source-unknown') + return match + } + + const domain = DOMAIN_RE.exec(mangledType)?.[0]?.toLowerCase() + // Unreachable — the outer regex only matches when a domain is present — + // but leave the statement alone rather than emit a broken rewrite. + if (!domain) return match + + // This statement converts the column, so from here on in the corpus it + // holds CIPHERTEXT. `indexColumnDeclarations` cannot know that: it + // reads CREATE TABLE, ADD COLUMN and RENAME, never the strict matcher's + // own target. Without this, a corpus carrying an earlier conversion + // (`... SET DATA TYPE eql_v2_encrypted` from a stack version that + // predates this sweep) leaves the column looking plaintext, and the + // NEXT domain change drops the ciphertext — `declared` cannot catch it, + // because the column really is declared, as plaintext, by the original + // CREATE TABLE. + // + // Recorded here rather than in the corpus-wide index on purpose: the + // index has no order, so it would flag THIS conversion — the legitimate + // plaintext -> encrypted one — as already-encrypted too. Files are + // walked in sorted order and matches within a file in source order, so + // "already converted" means "converted by a statement that runs before + // this one". + encryptedColumns.add(columnKey(table, column, schema)) + return renderSafeAlter(table, column, domain, schema) + }, + ) + + if (updated !== original) { + await writeFile(filePath, updated, 'utf-8') + rewritten.push(filePath) + } - // Fail closed. Absence from `declaredColumns` does not mean the column - // is plaintext — it means the corpus never said. The declaration can - // sit in a directory this sweep does not read (the wizard ships with - // three candidates and indexes each separately), so rewriting on the - // assumption is how a live `DROP COLUMN` reaches a populated — possibly - // already-encrypted — column. Flag it and let the user look. - if (!declaredColumns.has(columnKey(table, column, schema))) { - skip(filePath, match.trim(), 'source-unknown') - return match + // Broad secondary scan on the POST-rewrite content: anything still carrying + // `SET DATA TYPE` near an eql_v2/eql_v3 token slipped past the strict + // matcher. Flag it — non-fatally — rather than leave the user shipping SQL + // that fails at migrate time. + for (const nearMiss of updated.matchAll(NEAR_MISS_RE)) { + const statement = trimStatementPreamble(nearMiss[0]) + // Anchor the comment test on the `SET DATA TYPE` itself: the match starts + // at the previous `;`, so its own offset sits before any preamble. + const keyword = nearMiss[0].search(/\bSET\s+DATA\s+TYPE\b/i) + if ( + isInsideCommentOrString( + updated, + nearMiss.index + Math.max(keyword, 0), + ) + ) { + continue } - - const domain = DOMAIN_RE.exec(mangledType)?.[0]?.toLowerCase() - // Unreachable — the outer regex only matches when a domain is present — - // but leave the statement alone rather than emit a broken rewrite. - if (!domain) return match - - // This statement converts the column, so from here on in the corpus it - // holds CIPHERTEXT. `indexColumnDeclarations` cannot know that: it - // reads CREATE TABLE, ADD COLUMN and RENAME, never the strict matcher's - // own target. Without this, a corpus carrying an earlier conversion - // (`... SET DATA TYPE eql_v2_encrypted` from a stack version that - // predates this sweep) leaves the column looking plaintext, and the - // NEXT domain change drops the ciphertext — `declared` cannot catch it, - // because the column really is declared, as plaintext, by the original - // CREATE TABLE. - // - // Recorded here rather than in the corpus-wide index on purpose: the - // index has no order, so it would flag THIS conversion — the legitimate - // plaintext -> encrypted one — as already-encrypted too. Files are - // walked in sorted order and matches within a file in source order, so - // "already converted" means "converted by a statement that runs before - // this one". - encryptedColumns.add(columnKey(table, column, schema)) - return renderSafeAlter(table, column, domain, schema) - }, - ) - - if (updated !== original) { - await writeFile(filePath, updated, 'utf-8') - rewritten.push(filePath) - } - - // Broad secondary scan on the POST-rewrite content: anything still carrying - // `SET DATA TYPE` near an eql_v2/eql_v3 token slipped past the strict - // matcher. Flag it — non-fatally — rather than leave the user shipping SQL - // that fails at migrate time. - for (const nearMiss of updated.matchAll(NEAR_MISS_RE)) { - const statement = trimStatementPreamble(nearMiss[0]) - // Anchor the comment test on the `SET DATA TYPE` itself: the match starts - // at the previous `;`, so its own offset sits before any preamble. - const keyword = nearMiss[0].search(/\bSET\s+DATA\s+TYPE\b/i) - if ( - isInsideCommentOrString(updated, nearMiss.index + Math.max(keyword, 0)) - ) { - continue + skip(filePath, statement, 'unrecognised-form') } - skip(filePath, statement, 'unrecognised-form') } + } catch (error) { + // Nothing done yet means nothing to add: rethrow the original, `code` and + // identity intact, so the caller's "this directory went unchecked" wording + // stays exactly as it was. + if (rewritten.length === 0 && skipped.length === 0) throw error + throw new PartialRewriteError(error, rewritten, skipped) } return { rewritten, skipped } diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index c0193b665..0dc4266c1 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -66,6 +66,24 @@ vi.mock('node:fs', async (importOriginal) => { return { ...actual, default: actual, writeFileSync: fsWrite.spy } }) +// Same seam on the async side. The ALTER-COLUMN sweep is the only thing in this +// command that writes through `node:fs/promises`, so a call-counting spy here +// 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: 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() + fsWriteAsync.real = actual.writeFile + return { ...actual, default: actual, writeFile: fsWriteAsync.spy } +}) + // `printNextSteps` lives in the install module, which drags in `pg`. Stub it; // the two helpers we reuse (`findGeneratedMigration`, `cleanupMigrationFile`) // stay real and act on the tmpdir. @@ -76,6 +94,7 @@ vi.mock('../../db/install.js', async (importOriginal) => { beforeEach(() => { fsWrite.spy.mockImplementation(fsWrite.real) + fsWriteAsync.spy.mockImplementation(fsWriteAsync.real) }) afterEach(() => { vi.clearAllMocks() @@ -360,6 +379,65 @@ describe('eqlMigrationCommand — Drizzle', () => { ) }) + // 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('names the files it already rewrote 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 rewritten = join(out, '0001_encrypt-email.sql') + writeFileSync( + rewritten, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + writeFileSync( + join(out, '0002_encrypt-name.sql'), + 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', + ) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0003_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + // The generated migration is `skip`ped, so the only writes through + // `node:fs/promises` are the sweep's: #1 rewrites 0001, #2 fails on 0002. + let asyncWrites = 0 + fsWriteAsync.spy.mockImplementation( + ( + ...args: Parameters + ): Promise => { + asyncWrites += 1 + if (asyncWrites === 2) { + return Promise.reject(new Error('ENOSPC: no space left on device')) + } + return fsWriteAsync.real(...args) + }, + ) + + await eqlMigrationCommand({ drizzle: true, out }) + + // The first file really was rewritten and is sitting there destructive. + expect(readFileSync(rewritten, 'utf-8')).toContain('DROP COLUMN') + // So it must be named, not just counted away by a directory-level warning. + const stepped = clack.log.step.mock.calls.map((c) => String(c[0])) + expect(stepped.some((msg) => msg.includes(rewritten))).toBe(true) + const infos = clack.log.info.mock.calls.map((c) => String(c[0])) + expect(infos.some((msg) => msg.includes('Rewrote 1 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('did not fully complete'))).toBe( + true, + ) + }) + 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 e68cb0b44..d698c71e6 100644 --- a/packages/cli/src/commands/eql/migration.ts +++ b/packages/cli/src/commands/eql/migration.ts @@ -8,7 +8,9 @@ import { CliExit } from '@/cli/exit.js' import { printNextSteps, SAFE_MIGRATION_NAME } from '@/commands/db/install.js' import { describeSkipReason, + PartialRewriteError, rewriteEncryptedAlterColumns, + type SkippedAlter, } from '@/commands/db/rewrite-migrations.js' import { detectPackageManager, @@ -265,29 +267,25 @@ async function generateDrizzleEqlMigration( // Either way the user must review sibling migrations before running migrate, // so surface it again at the closing note (below) — not just inline here. let sweepIncomplete = false + // Reported AFTER the try/catch rather than inside it, because a failed sweep + // is not an empty one: it writes a file at a time, so a throw part way + // through leaves earlier files already rewritten and holding a live + // `DROP COLUMN`. Naming them is the whole point of the report — "review the + // sibling migrations in " does not tell a user which ones are already + // destructive (#786). + let rewritten: string[] = [] + let skipped: SkippedAlter[] = [] try { - const { rewritten, skipped } = await rewriteEncryptedAlterColumns(outDir, { + ;({ rewritten, skipped } = await rewriteEncryptedAlterColumns(outDir, { skip: migrationPath, - }) - if (rewritten.length > 0) { - p.log.info( - `Rewrote ${rewritten.length} migration file(s) into a runnable ADD+DROP+RENAME for encrypted columns (safe on empty tables; see each file's header before running against populated data):`, - ) - for (const file of rewritten) p.log.step(` - ${file}`) - } - if (skipped.length > 0) { - sweepIncomplete = true - p.log.warn( - `Found ${skipped.length} ALTER-to-encrypted statement(s) the sweep left alone. Review and fix them before running your migrations:`, - ) - for (const { file, statement, reason } of skipped) { - p.log.step(` - ${file}: ${statement}`) - p.log.step(` ${describeSkipReason(reason)}`) - } - } + })) } catch (error) { // Advisory: the install migration itself is already written and valid. sweepIncomplete = true + if (error instanceof PartialRewriteError) { + rewritten = error.rewritten + skipped = error.skipped + } p.log.warn( `Could not sweep ${outDir} for unsafe ALTER COLUMN statements: ${ error instanceof Error ? error.message : String(error) @@ -295,6 +293,23 @@ async function generateDrizzleEqlMigration( ) } + if (rewritten.length > 0) { + p.log.info( + `Rewrote ${rewritten.length} migration file(s) into a runnable ADD+DROP+RENAME for encrypted columns (safe on empty tables; see each file's header before running against populated data):`, + ) + for (const file of rewritten) p.log.step(` - ${file}`) + } + if (skipped.length > 0) { + sweepIncomplete = true + p.log.warn( + `Found ${skipped.length} ALTER-to-encrypted statement(s) the sweep left alone. Review and fix them before running your migrations:`, + ) + for (const { file, statement, reason } of skipped) { + p.log.step(` - ${file}: ${statement}`) + p.log.step(` ${describeSkipReason(reason)}`) + } + } + p.log.success(`Migration created: ${migrationPath}`) if (sweepIncomplete) { p.log.warn( diff --git a/packages/wizard/src/__tests__/post-agent.test.ts b/packages/wizard/src/__tests__/post-agent.test.ts index 0816129a6..5e373d1a4 100644 --- a/packages/wizard/src/__tests__/post-agent.test.ts +++ b/packages/wizard/src/__tests__/post-agent.test.ts @@ -24,6 +24,28 @@ vi.mock('../lib/rewrite-migrations.js', async (importOriginal) => { return { ...actual, sweepMigrationDirs: vi.fn(actual.sweepMigrationDirs) } }) +// `node:fs/promises` stays REAL by default — the sweep's own reads and writes +// need it. Only `writeFile` is routed through a delegating spy, so one test can +// fail the Nth write and drive the whole chain (rewriter → sweep → report) +// through a genuine mid-directory failure rather than a mocked sweep result. +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() + fsWrite.real = actual.writeFile + return { ...actual, default: actual, writeFile: fsWrite.spy } +}) + +beforeEach(() => { + fsWrite.spy.mockImplementation(fsWrite.real) +}) + import * as childProcess from 'node:child_process' import * as p from '@clack/prompts' import { sweepMigrationDirs } from '../lib/rewrite-migrations.js' @@ -300,6 +322,126 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { expect(String(options?.message)).toContain('could not check 1 directory') }) + // A sweep that threw AFTER rewriting a file is both things at once: those + // migrations went unchecked from the failure point on, AND a live DROP COLUMN + // is already on disk. The reporting loop used to `continue` past the whole + // block on any error, so neither the file list nor the data-destruction + // warning printed, and the prompt took the softer "could not check" arm. The + // destructive fact is the one that must win the prompt (#786). + it('reports a directory that threw after rewriting as destructive', async () => { + const rewrittenFile = path.join(cwd, 'drizzle', '0001_encrypt.sql') + vi.mocked(sweepMigrationDirs).mockResolvedValueOnce([ + { + dir: 'drizzle', + rewritten: [rewrittenFile], + skipped: [], + error: 'EACCES: permission denied, open ...0002_encrypt.sql', + }, + ]) + const warn = vi.spyOn(p.log, 'warn').mockImplementation(() => {}) + const info = vi.spyOn(p.log, 'info').mockImplementation(() => {}) + const step = vi.spyOn(p.log, 'step').mockImplementation(() => {}) + + await runDrizzle() + + const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] + expect(options?.initialValue).toBe(false) + expect(String(options?.message)).toContain('DESTROYS data') + + // The user is told WHICH file now holds the DROP COLUMN... + const stepped = step.mock.calls.map((c) => String(c[0])) + expect(stepped.some((msg) => msg.includes(rewrittenFile))).toBe(true) + const logged = info.mock.calls.map((c) => String(c[0])) + expect(logged.some((msg) => msg.includes('Rewrote 1 migration file'))).toBe( + true, + ) + + const warned = warn.mock.calls.map((c) => String(c[0])) + // ...that it is data-destroying... + expect(warned.some((msg) => msg.includes('data-destroying'))).toBe(true) + // ...and that the sweep stopped early, so the rest went unchecked. Both + // facts are true; neither replaces the other. + expect(warned.some((msg) => msg.includes('did not fully complete'))).toBe( + true, + ) + expect(warned.some((msg) => msg.includes('EACCES'))).toBe(true) + + warn.mockRestore() + info.mockRestore() + step.mockRestore() + }) + + // The same outcome driven end to end — real files, a real failing write, the + // real sweep — rather than a hand-written sweep result. It is the seam + // between the rewriter, the directory sweep and this report that broke, so + // one test has to cross all three: a mocked `sweepMigrationDirs` would stay + // green even if the rewriter went back to swallowing its partial work. + it('reports a real mid-sweep write failure as destructive', async () => { + makeDrizzleOut('drizzle') + fs.writeFileSync( + path.join(cwd, 'drizzle', '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text, "name" text);\n', + ) + const first = path.join(cwd, 'drizzle', '0001_encrypt_email.sql') + fs.writeFileSync( + first, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + fs.writeFileSync( + path.join(cwd, 'drizzle', '0002_encrypt_name.sql'), + 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', + ) + // Sorted order, and 0000 needs no write: write #1 is 0001, #2 is 0002. + let writes = 0 + fsWrite.spy.mockImplementation( + (...args: Parameters) => { + writes += 1 + if (writes === 2) { + return Promise.reject(new Error('ENOSPC: no space left on device')) + } + return fsWrite.real(...args) + }, + ) + const warn = vi.spyOn(p.log, 'warn').mockImplementation(() => {}) + const step = vi.spyOn(p.log, 'step').mockImplementation(() => {}) + + await runDrizzle() + + // 0001 really is destructive now — this is the on-disk fact the report + // used to lose. + expect(fs.readFileSync(first, 'utf-8')).toContain('DROP COLUMN') + const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] + expect(options?.initialValue).toBe(false) + expect(String(options?.message)).toContain('DESTROYS data') + expect(step.mock.calls.map((c) => String(c[0])).join('\n')).toContain(first) + const warned = warn.mock.calls.map((c) => String(c[0])) + expect(warned.some((msg) => msg.includes('data-destroying'))).toBe(true) + expect(warned.some((msg) => msg.includes('did not fully complete'))).toBe( + true, + ) + + warn.mockRestore() + step.mockRestore() + }) + + // The sibling of the case above, and the reason `destructive` cannot simply + // be "there was an error": a directory that threw with nothing rewritten + // knows nothing about its migrations, so claiming they destroy data would be + // a guess. Kept green by the existing EISDIR test too, but pinned here + // through the mocked shape so the two arms are visibly distinguished. + it('keeps an error with no rewritten files on the unverified arm', async () => { + vi.mocked(sweepMigrationDirs).mockResolvedValueOnce([ + { dir: 'drizzle', rewritten: [], skipped: [], error: 'EISDIR' }, + ]) + + await runDrizzle() + + const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] + expect(options?.initialValue).toBe(false) + expect(String(options?.message)).not.toContain('DESTROYS data') + expect(String(options?.message)).toContain('could not check 1 directory') + }) + // The wizard ships scanning drizzle/, migrations/ and src/db/migrations/ and // indexes each SEPARATELY — the per-directory index is the mechanism the // fail-closed rule exists to make safe. Every test above uses only drizzle/, diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index 5cefe86e2..da4329c56 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -1,13 +1,54 @@ 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() + fsWrite.real = actual.writeFile + return { ...actual, default: actual, writeFile: fsWrite.spy } +}) + import { describeSkipReason, + PartialRewriteError, rewriteEncryptedAlterColumns, sweepMigrationDirs, } from '../lib/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) => { + calls += 1 + if (calls === nth) return Promise.reject(new Error(message)) + return fsWrite.real(...args) + }, + ) +} + describe('rewriteEncryptedAlterColumns', () => { let tmpDir: string @@ -1674,6 +1715,116 @@ 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 test in this file uses the EISDIR-on-read +// trick, which 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 the caller reported zeros (#786). +describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () => { + let tmpDir: string + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'wizard-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 the files it had already rewritten', 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]) + // 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('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', + ) + fs.writeFileSync( + path.join(tmpDir, '0002_encrypt_name.sql'), + '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([]) + 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 + // "nothing is known about this directory" path stays exactly as it was. + it('rethrows the original error when nothing had been done yet', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + fs.writeFileSync( + path.join(tmpDir, '0001_encrypt_email.sql'), + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + failWriteNumber(1, 'EROFS: read-only file system') + + const error = await rewriteEncryptedAlterColumns(tmpDir).catch( + (err: unknown) => err, + ) + + expect(error).toBeInstanceOf(Error) + expect(error).not.toBeInstanceOf(PartialRewriteError) + expect((error as Error).message).toBe('EROFS: read-only file system') + }) +}) + describe('sweepMigrationDirs', () => { let tmpDir: string @@ -1791,6 +1942,48 @@ describe('sweepMigrationDirs', () => { expect(results[1].rewritten).toEqual([path.join(abs, '0002_alter.sql')]) }) + // The error result used to be hard-coded to `rewritten: []`, which is a claim + // — "nothing was changed here" — and it is false whenever the throw landed + // after a write. The caller reads those totals to decide whether to warn + // about data destruction, so emptying them turns a destructive outcome into + // an "unchecked" one (#786). + it('reports the files a failing directory had already rewritten', async () => { + const abs = seedDir('drizzle') + fs.writeFileSync( + path.join(abs, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text, "name" text);\n', + ) + const first = path.join(abs, '0001_encrypt_email.sql') + fs.writeFileSync( + first, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + fs.writeFileSync( + path.join(abs, '0002_encrypt_name.sql'), + 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', + ) + failWriteNumber(2, 'ENOSPC: no space left on device') + + const results = await sweepMigrationDirs(tmpDir, ['drizzle']) + + expect(results[0].error).toBe('ENOSPC: no space left on device') + expect(results[0].rewritten).toEqual([first]) + }) + + // Unchanged behaviour, pinned so the fix above cannot bleed into it: a + // directory that threw before touching anything still reports zeros, because + // that is the truth for it. + it('still reports zeros for a directory that threw before any write', async () => { + const broken = seedDir('drizzle') + fs.mkdirSync(path.join(broken, '0001_alter.sql')) + + const results = await sweepMigrationDirs(tmpDir, ['drizzle']) + + expect(results[0].error).toBeDefined() + expect(results[0].rewritten).toEqual([]) + expect(results[0].skipped).toEqual([]) + }) + it('reports near-misses per directory', async () => { const abs = seedDir( 'drizzle', diff --git a/packages/wizard/src/lib/post-agent.ts b/packages/wizard/src/lib/post-agent.ts index f34ff1c38..a2d9792f8 100644 --- a/packages/wizard/src/lib/post-agent.ts +++ b/packages/wizard/src/lib/post-agent.ts @@ -82,14 +82,22 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { const destructive = sweep.rewritten > 0 const flaggedOnly = !destructive && sweep.skipped > 0 - // A directory whose sweep threw contributes 0 to both totals, so on its own - // it is indistinguishable from a clean sweep — except that it means the - // opposite: those migrations may still hold unrepaired `SET DATA TYPE` - // statements and nobody has looked. `stash eql migration` / `db install` - // treat "sweep failed outright" and "sweep left near-misses" as the same - // state for the same reason; unknown is not safe, so the default is NO here - // too. The wording differs from the destructive case on purpose: nothing is - // known about that directory, so claiming it destroys data would be a guess. + // A directory whose sweep threw before doing anything contributes 0 to both + // totals, so on its own it is indistinguishable from a clean sweep — except + // that it means the opposite: those migrations may still hold unrepaired + // `SET DATA TYPE` statements and nobody has looked. `stash eql migration` / + // `db install` treat "sweep failed outright" and "sweep left near-misses" as + // the same state for the same reason; unknown is not safe, so the default is + // NO here too. The wording differs from the destructive case on purpose: + // nothing is known about that directory, so claiming it destroys data would + // be a guess. + // + // This is orthogonal to `destructive`, not an alternative to it. A sweep + // that threw AFTER rewriting a file is both — those files hold a live + // `DROP COLUMN`, and the rest of the directory went unchecked — so it lands + // in both totals and both warnings fire. The prompt's ternary puts + // `destructive` first, which is the right precedence: data destruction is + // the fact the user cannot afford to miss (#786). const unverifiedDirs = sweep.failedDirs const unverified = unverifiedDirs.length > 0 const unverifiedList = unverifiedDirs.map((dir) => `${dir}/`).join(', ') @@ -147,8 +155,13 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { * `failedDirs` names the directories that exist but whose sweep threw. It is a * third state, not a variant of "nothing to do": those migrations may still * contain unrepaired `SET DATA TYPE` statements and went unchecked, which the - * `rewritten`/`skipped` counts cannot express — both stay 0 for such a - * directory, exactly as they do for a clean one. + * `rewritten`/`skipped` counts cannot express — for a directory that threw + * before touching anything they stay 0, exactly as they do for a clean one. + * + * It is not exclusive with those counts either. A sweep that threw part way + * through reports what it had already rewritten, so such a directory lands in + * `failedDirs` AND contributes to `rewritten` — it is both destructive and + * incompletely checked, and the caller must say both (#786). */ async function rewriteEncryptedMigrations(cwd: string): Promise<{ rewritten: number @@ -178,12 +191,18 @@ async function rewriteEncryptedMigrations(cwd: string): Promise<{ // and `new Error()` has an empty message. Testing `if (error)` would put a // blank-message failure back on the fail-open path this whole branch exists // to close. + // + // Deliberately NOT a `continue`: the sweep writes one file at a time, so a + // directory that threw part way through can have rewritten files already on + // disk, each holding a live `DROP COLUMN`. Skipping the report below hid + // exactly those — the user was told the directory went unchecked and never + // that part of it was already data-destroying (#786). Both blocks below + // no-op when the failure came before any work, which is the common case. if (error !== undefined) { totals.failedDirs.push(dir) p.log.warn( `Could not rewrite migrations in ${dir}: ${error || 'unknown error'}`, ) - continue } if (rewritten.length > 0) { diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index a050b4d05..76fc4f512 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -666,6 +666,38 @@ export interface RewriteResult { skipped: SkippedAlter[] } +/** + * Thrown by {@link rewriteEncryptedAlterColumns} when it fails PART WAY through + * a directory it has already changed. + * + * The sweep writes one file at a time, so a failure on the third file leaves + * the first two rewritten on disk — each holding a live `DROP COLUMN`. A bare + * rejection discards that fact: the accumulated `rewritten` array dies with the + * stack frame, and every caller then reports the directory as merely + * *unchecked*, telling the user to review those migrations without saying that + * some of them now destroy data. That is the fail-open inversion this rewriter + * exists to prevent, so the work already done travels with the failure (#786). + * + * `message` is the underlying failure's, verbatim — callers render it straight + * to the user — and the original is kept as `cause`. Thrown ONLY when there is + * partial work to report: a sweep that fails before changing or flagging + * anything rejects with the original error untouched, because "nothing is known + * about this directory" is a different state, and not a destructive one. + */ +export class PartialRewriteError extends Error { + /** Absolute paths of the files already rewritten when the failure hit. */ + readonly rewritten: string[] + /** Near-miss statements already flagged when the failure hit. */ + readonly skipped: SkippedAlter[] + + constructor(cause: unknown, rewritten: string[], skipped: SkippedAlter[]) { + super(cause instanceof Error ? cause.message : String(cause), { cause }) + this.name = 'PartialRewriteError' + this.rewritten = [...rewritten] + this.skipped = [...skipped] + } +} + /** * Replace in-place `ALTER COLUMN ... SET DATA TYPE ` * statements with an ADD + DROP + RENAME sequence. @@ -743,93 +775,108 @@ export async function rewriteEncryptedAlterColumns( const { encrypted: encryptedColumns, declared: declaredColumns } = indexColumnDeclarations([...contents.values()]) - for (const [filePath, original] of contents) { - if (options.skip && filePath === options.skip) continue - - // Reset the regex's lastIndex — it's stateful on /g - ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 - - const updated = original.replace( - ALTER_COLUMN_TO_ENCRYPTED_RE, - ( - match: string, - first: string, - second: string | undefined, - column: string, - mangledType: string, - offset: number, - ) => { - // Commented-out SQL never runs, and a multi-line replacement would only - // inherit the `-- ` on its first line — leaving the rest live. - if (isInsideCommentOrString(original, offset)) return match - - const { schema, table } = tableOf(first, second) - - // Already encrypted: the ADD+DROP+RENAME would drop the ciphertext and - // there is no plaintext left to backfill from. Flag, never guess. - if (encryptedColumns.has(columnKey(table, column, schema))) { - skip(filePath, match.trim(), 'already-encrypted') - return match - } + // Wrapped, not left to reject bare: from the first `writeFile` on, a failure + // anywhere in this loop leaves the directory PART rewritten. See + // {@link PartialRewriteError} — the reads above it cannot have changed + // anything, so they still reject with their own error. + try { + for (const [filePath, original] of contents) { + if (options.skip && filePath === options.skip) continue - // Fail closed. Absence from `declaredColumns` does not mean the column - // is plaintext — it means the corpus never said. The declaration can - // sit in a directory this sweep does not read (the wizard ships with - // three candidates and indexes each separately), so rewriting on the - // assumption is how a live `DROP COLUMN` reaches a populated — possibly - // already-encrypted — column. Flag it and let the user look. - if (!declaredColumns.has(columnKey(table, column, schema))) { - skip(filePath, match.trim(), 'source-unknown') - return match - } + // Reset the regex's lastIndex — it's stateful on /g + ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 - const domain = DOMAIN_RE.exec(mangledType)?.[0]?.toLowerCase() - // Unreachable — the outer regex only matches when a domain is present — - // but leave the statement alone rather than emit a broken rewrite. - if (!domain) return match - - // This statement converts the column, so from here on in the corpus it - // holds CIPHERTEXT. `indexColumnDeclarations` cannot know that: it - // reads CREATE TABLE, ADD COLUMN and RENAME, never the strict matcher's - // own target. Without this, a corpus carrying an earlier conversion - // (`... SET DATA TYPE eql_v2_encrypted` from a stack version that - // predates this sweep) leaves the column looking plaintext, and the - // NEXT domain change drops the ciphertext — `declared` cannot catch it, - // because the column really is declared, as plaintext, by the original - // CREATE TABLE. - // - // Recorded here rather than in the corpus-wide index on purpose: the - // index has no order, so it would flag THIS conversion — the legitimate - // plaintext -> encrypted one — as already-encrypted too. Files are - // walked in sorted order and matches within a file in source order, so - // "already converted" means "converted by a statement that runs before - // this one". - encryptedColumns.add(columnKey(table, column, schema)) - return renderSafeAlter(table, column, domain, schema) - }, - ) - - if (updated !== original) { - await writeFile(filePath, updated, 'utf-8') - rewritten.push(filePath) - } + const updated = original.replace( + ALTER_COLUMN_TO_ENCRYPTED_RE, + ( + match: string, + first: string, + second: string | undefined, + column: string, + mangledType: string, + offset: number, + ) => { + // Commented-out SQL never runs, and a multi-line replacement would only + // inherit the `-- ` on its first line — leaving the rest live. + if (isInsideCommentOrString(original, offset)) return match + + const { schema, table } = tableOf(first, second) + + // Already encrypted: the ADD+DROP+RENAME would drop the ciphertext and + // there is no plaintext left to backfill from. Flag, never guess. + if (encryptedColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'already-encrypted') + return match + } + + // Fail closed. Absence from `declaredColumns` does not mean the column + // is plaintext — it means the corpus never said. The declaration can + // sit in a directory this sweep does not read (the wizard ships with + // three candidates and indexes each separately), so rewriting on the + // assumption is how a live `DROP COLUMN` reaches a populated — possibly + // already-encrypted — column. Flag it and let the user look. + if (!declaredColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'source-unknown') + return match + } + + const domain = DOMAIN_RE.exec(mangledType)?.[0]?.toLowerCase() + // Unreachable — the outer regex only matches when a domain is present — + // but leave the statement alone rather than emit a broken rewrite. + if (!domain) return match + + // This statement converts the column, so from here on in the corpus it + // holds CIPHERTEXT. `indexColumnDeclarations` cannot know that: it + // reads CREATE TABLE, ADD COLUMN and RENAME, never the strict matcher's + // own target. Without this, a corpus carrying an earlier conversion + // (`... SET DATA TYPE eql_v2_encrypted` from a stack version that + // predates this sweep) leaves the column looking plaintext, and the + // NEXT domain change drops the ciphertext — `declared` cannot catch it, + // because the column really is declared, as plaintext, by the original + // CREATE TABLE. + // + // Recorded here rather than in the corpus-wide index on purpose: the + // index has no order, so it would flag THIS conversion — the legitimate + // plaintext -> encrypted one — as already-encrypted too. Files are + // walked in sorted order and matches within a file in source order, so + // "already converted" means "converted by a statement that runs before + // this one". + encryptedColumns.add(columnKey(table, column, schema)) + return renderSafeAlter(table, column, domain, schema) + }, + ) - // Broad secondary scan on the POST-rewrite content: anything still carrying - // `SET DATA TYPE` near an eql_v2/eql_v3 token slipped past the strict - // matcher. Flag it — non-fatally — rather than leave the user shipping SQL - // that fails at migrate time. - for (const nearMiss of updated.matchAll(NEAR_MISS_RE)) { - const statement = trimStatementPreamble(nearMiss[0]) - // Anchor the comment test on the `SET DATA TYPE` itself: the match starts - // at the previous `;`, so its own offset sits before any preamble. - const keyword = nearMiss[0].search(/\bSET\s+DATA\s+TYPE\b/i) - if ( - isInsideCommentOrString(updated, nearMiss.index + Math.max(keyword, 0)) - ) { - continue + if (updated !== original) { + await writeFile(filePath, updated, 'utf-8') + rewritten.push(filePath) + } + + // Broad secondary scan on the POST-rewrite content: anything still carrying + // `SET DATA TYPE` near an eql_v2/eql_v3 token slipped past the strict + // matcher. Flag it — non-fatally — rather than leave the user shipping SQL + // that fails at migrate time. + for (const nearMiss of updated.matchAll(NEAR_MISS_RE)) { + const statement = trimStatementPreamble(nearMiss[0]) + // Anchor the comment test on the `SET DATA TYPE` itself: the match starts + // at the previous `;`, so its own offset sits before any preamble. + const keyword = nearMiss[0].search(/\bSET\s+DATA\s+TYPE\b/i) + if ( + isInsideCommentOrString( + updated, + nearMiss.index + Math.max(keyword, 0), + ) + ) { + continue + } + skip(filePath, statement, 'unrecognised-form') } - skip(filePath, statement, 'unrecognised-form') } + } catch (error) { + // Nothing done yet means nothing to add: rethrow the original, `code` and + // identity intact, so the caller's "this directory went unchecked" wording + // stays exactly as it was. + if (rewritten.length === 0 && skipped.length === 0) throw error + throw new PartialRewriteError(error, rewritten, skipped) } return { rewritten, skipped } @@ -844,7 +891,14 @@ export async function rewriteEncryptedAlterColumns( export interface DirRewriteResult extends RewriteResult { /** The candidate directory as supplied (relative to `cwd`), for reporting. */ dir: string - /** Set when this directory's sweep threw; the sweep continues regardless. */ + /** + * Set when this directory's sweep threw; the sweep continues regardless. + * + * It does NOT imply `rewritten` and `skipped` are empty — a sweep can fail + * after it has already rewritten files, and those files are reported here + * alongside the error. Both facts hold at once, so a caller must report the + * rewrites (data-destroying) as well as the failure (unchecked remainder). + */ error?: string /** * Set when the directory holds `.sql` files but no drizzle-kit journal, so @@ -935,7 +989,19 @@ export async function sweepMigrationDirs( results.push({ dir, rewritten, skipped }) } catch (err) { const message = err instanceof Error ? err.message : String(err) - results.push({ dir, rewritten: [], skipped: [], error: message }) + // Hard-coding zeros here would be a CLAIM — "this directory was not + // changed" — and it is false whenever the sweep threw after a write. The + // caller reads these arrays to decide whether to warn about data + // destruction, so emptying them downgrades a destructive outcome to a + // merely unchecked one (#786). A failure with nothing behind it still + // reports zeros, because for that directory they are true. + const partial = err instanceof PartialRewriteError + results.push({ + dir, + rewritten: partial ? err.rewritten : [], + skipped: partial ? err.skipped : [], + error: message, + }) } } diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 44784cc2f..47c6eeea3 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -378,6 +378,8 @@ Pass exactly one of `--drizzle` / `--prisma`. The generated migration also insta After writing the migration, `--drizzle` sweeps sibling migrations containing an in-place `ALTER COLUMN … SET DATA TYPE `. When the source declaration is provably plaintext, it replaces the ALTER with an `ADD COLUMN` + `DROP` + `RENAME` sequence and lists the rewritten files. This is equivalent to DROP+ADD: it is safe only on an empty table and does not preserve data, constraints, defaults, or indexes. On a populated table, do not run that rewrite; use the staged EQL v3 rollout (add an encrypted twin, dual-write, backfill, switch the application to the encrypted column by name, then drop plaintext). If the source type cannot be proven, the statement remains unchanged and the command warns that the migration directory needs review. +If the sweep itself fails part way through — an unwritable file, a full disk, a lock held by an editor or `drizzle-kit` — it still lists the files it rewrote before the failure, *and* warns that it did not fully complete. Both matter: the listed files already contain the `DROP COLUMN`, and the rest of the directory went unchecked. Treat that directory as needing review before you run `drizzle-kit migrate`. + #### `eql upgrade` The install SQL is safe to re-run — columns and data survive — but it cascade-drops functional indexes that depend on `eql_v3`; recreate them afterward. `upgrade` is v3-only and accepts `--supabase`, `--dry-run`, and `--database-url`. From de81ac51511d79b22159214ae8a19c2a6678dfbf Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 28 Jul 2026 12:02:09 +1000 Subject: [PATCH 2/5] fix(wizard,cli): report attempted migration writes --- .../sweep-partial-rewrite-destructive.md | 29 +++++---- .../src/__tests__/rewrite-migrations.test.ts | 55 ++++++++++++----- .../cli/src/commands/db/rewrite-migrations.ts | 29 +++++---- .../commands/eql/__tests__/migration.test.ts | 36 +++++++---- .../wizard/src/__tests__/post-agent.test.ts | 40 ++++++++++--- .../src/__tests__/rewrite-migrations.test.ts | 60 ++++++++++++++----- packages/wizard/src/lib/rewrite-migrations.ts | 38 +++++++----- 7 files changed, 196 insertions(+), 91 deletions(-) diff --git a/.changeset/sweep-partial-rewrite-destructive.md b/.changeset/sweep-partial-rewrite-destructive.md index e7ca2f944..7a792bab6 100644 --- a/.changeset/sweep-partial-rewrite-destructive.md +++ b/.changeset/sweep-partial-rewrite-destructive.md @@ -17,17 +17,20 @@ 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. -The work already done now travels with the failure. `rewriteEncryptedAlterColumns` -rejects with a `PartialRewriteError` carrying `rewritten` and `skipped` whenever -it fails part way through a directory it has already changed, and the wizard's -directory sweep reports those arrays alongside the error instead of zeros. A -directory in that state is reported as **both**: the rewritten files 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. +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. -A sweep that fails before changing anything is unchanged: it rejects with the -original error, reports zeros, and keeps the softer "nothing is known about this -directory" wording, because claiming data destruction there would be a guess. +A sweep that fails before attempting any write is unchanged: it rejects with +the original error, reports zeros, and keeps the softer "nothing is known about +this directory" wording, because claiming data destruction there would be a +guess. diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 89ffc456c..d6d8a1faa 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -48,6 +48,26 @@ const failWriteNumber = (nth: number, message: string): void => { ) } +/** + * 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 + ) => { + 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 @@ -1822,7 +1842,7 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () return { first, second } } - it('rejects with the files it had already rewritten', async () => { + 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. @@ -1838,12 +1858,26 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () expect((error as PartialRewriteError).message).toBe( 'ENOSPC: no space left on device', ) - expect((error as PartialRewriteError).rewritten).toEqual([first]) + 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'), @@ -1854,8 +1888,9 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () 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( - path.join(tmpDir, '0002_encrypt_name.sql'), + 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. @@ -1866,7 +1901,7 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () ) expect(error).toBeInstanceOf(PartialRewriteError) - expect((error as PartialRewriteError).rewritten).toEqual([]) + expect((error as PartialRewriteError).rewritten).toEqual([attempted]) expect((error as PartialRewriteError).skipped).toEqual([ expect.objectContaining({ file: nearMiss, reason: 'unrecognised-form' }), ]) @@ -1876,15 +1911,7 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () // 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.writeFileSync( - path.join(tmpDir, '0000_declare.sql'), - 'CREATE TABLE "users" ("email" text);\n', - ) - fs.writeFileSync( - path.join(tmpDir, '0001_encrypt_email.sql'), - 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', - ) - failWriteNumber(1, 'EROFS: read-only file system') + fs.mkdirSync(path.join(tmpDir, '0001_broken.sql')) const error = await rewriteEncryptedAlterColumns(tmpDir).catch( (err: unknown) => err, @@ -1892,7 +1919,7 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () expect(error).toBeInstanceOf(Error) expect(error).not.toBeInstanceOf(PartialRewriteError) - expect((error as Error).message).toBe('EROFS: read-only file system') + expect((error as NodeJS.ErrnoException).code).toBe('EISDIR') }) }) diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 7297f453d..2488375aa 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -660,24 +660,29 @@ export interface RewriteResult { /** * Thrown by {@link rewriteEncryptedAlterColumns} when it fails PART WAY through - * a directory it has already changed. + * a directory where it has attempted a write. * * The sweep writes one file at a time, so a failure on the third file leaves - * the first two rewritten on disk — each holding a live `DROP COLUMN`. A bare - * rejection discards that fact: the accumulated `rewritten` array dies with the - * stack frame, and every caller then reports the directory as merely + * the first two rewritten on disk — each holding a live `DROP COLUMN` — and may + * truncate or partially replace the third before rejecting. A bare rejection + * discards that fact: the accumulated `rewritten` array dies with the stack + * frame, and every caller then reports the directory as merely * *unchecked*, telling the user to review those migrations without saying that * some of them now destroy data. That is the fail-open inversion this rewriter * exists to prevent, so the work already done travels with the failure (#786). * * `message` is the underlying failure's, verbatim — callers render it straight * to the user — and the original is kept as `cause`. Thrown ONLY when there is - * partial work to report: a sweep that fails before changing or flagging - * anything rejects with the original error untouched, because "nothing is known - * about this directory" is a different state, and not a destructive one. + * partial work to report: a sweep that fails before attempting a write or + * flagging anything rejects with the original error untouched, because + * "nothing is known about this directory" is a different state, and not a + * destructive one. */ export class PartialRewriteError extends Error { - /** Absolute paths of the files already rewritten when the failure hit. */ + /** + * Absolute paths of completed and attempted rewrites. A rejected write may + * already have mutated its destination, so its path is included. + */ readonly rewritten: string[] /** Near-miss statements already flagged when the failure hit. */ readonly skipped: SkippedAlter[] @@ -839,8 +844,8 @@ export async function rewriteEncryptedAlterColumns( ) if (updated !== original) { - await writeFile(filePath, updated, 'utf-8') rewritten.push(filePath) + await writeFile(filePath, updated, 'utf-8') } // Broad secondary scan on the POST-rewrite content: anything still carrying @@ -864,9 +869,9 @@ export async function rewriteEncryptedAlterColumns( } } } catch (error) { - // Nothing done yet means nothing to add: rethrow the original, `code` and - // identity intact, so the caller's "this directory went unchecked" wording - // stays exactly as it was. + // No write attempted and nothing flagged means nothing to add: rethrow the + // original, `code` and identity intact, so the caller's "this directory went + // unchecked" wording stays exactly as it was. if (rewritten.length === 0 && skipped.length === 0) throw error throw new PartialRewriteError(error, rewritten, skipped) } diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index 0dc4266c1..924014660 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -384,38 +384,45 @@ describe('eqlMigrationCommand — Drizzle', () => { // 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('names the files it already rewrote when the sweep fails part way', async () => { + 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 rewritten = join(out, '0001_encrypt-email.sql') + const skipped = join(out, '0001_using-email.sql') + writeFileSync( + skipped, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE 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 eql_v3_text_search;\n', ) + const attempted = join(out, '0003_encrypt-name.sql') writeFileSync( - join(out, '0002_encrypt-name.sql'), + attempted, 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', ) spawnMock.mockImplementation(() => { - writeFileSync(join(out, '0003_install-eql.sql'), '') + writeFileSync(join(out, '0004_install-eql.sql'), '') return { status: 0, stdout: '', stderr: '' } }) - // The generated migration is `skip`ped, so the only writes through - // `node:fs/promises` are the sweep's: #1 rewrites 0001, #2 fails on 0002. + // 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 ): Promise => { asyncWrites += 1 if (asyncWrites === 2) { - return Promise.reject(new Error('ENOSPC: no space left on device')) + await fsWriteAsync.real(args[0], '', 'utf-8') + throw new Error('ENOSPC: no space left on device') } - return fsWriteAsync.real(...args) + await fsWriteAsync.real(...args) }, ) @@ -423,16 +430,23 @@ describe('eqlMigrationCommand — Drizzle', () => { // The first file really was rewritten and is sitting there destructive. expect(readFileSync(rewritten, 'utf-8')).toContain('DROP COLUMN') - // So it must be named, not just counted away by a directory-level warning. + // 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 1 migration file'))).toBe( + 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, ) diff --git a/packages/wizard/src/__tests__/post-agent.test.ts b/packages/wizard/src/__tests__/post-agent.test.ts index 5e373d1a4..fd6821dab 100644 --- a/packages/wizard/src/__tests__/post-agent.test.ts +++ b/packages/wizard/src/__tests__/post-agent.test.ts @@ -376,51 +376,73 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { // between the rewriter, the directory sweep and this report that broke, so // one test has to cross all three: a mocked `sweepMigrationDirs` would stay // green even if the rewriter went back to swallowing its partial work. - it('reports a real mid-sweep write failure as destructive', async () => { + it('reports completed, attempted, and skipped files after a real mid-sweep failure', async () => { makeDrizzleOut('drizzle') fs.writeFileSync( path.join(cwd, 'drizzle', '0000_declare.sql'), 'CREATE TABLE "users" ("email" text, "name" text);\n', ) - const first = path.join(cwd, 'drizzle', '0001_encrypt_email.sql') + const skipped = path.join(cwd, 'drizzle', '0001_using_email.sql') + fs.writeFileSync( + skipped, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search USING (email)::eql_v3_text_search;\n', + ) + const first = path.join(cwd, 'drizzle', '0002_encrypt_email.sql') fs.writeFileSync( first, 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', ) + const attempted = path.join(cwd, 'drizzle', '0003_encrypt_name.sql') fs.writeFileSync( - path.join(cwd, 'drizzle', '0002_encrypt_name.sql'), + attempted, 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', ) - // Sorted order, and 0000 needs no write: write #1 is 0001, #2 is 0002. + // Sorted order: 0001 is only flagged, write #1 is 0002, and the rejected + // write #2 is 0003. let writes = 0 fsWrite.spy.mockImplementation( - (...args: Parameters) => { + async ( + ...args: Parameters + ) => { writes += 1 if (writes === 2) { - return Promise.reject(new Error('ENOSPC: no space left on device')) + await fsWrite.real(args[0], '', 'utf-8') + throw new Error('ENOSPC: no space left on device') } - return fsWrite.real(...args) + await fsWrite.real(...args) }, ) const warn = vi.spyOn(p.log, 'warn').mockImplementation(() => {}) + const info = vi.spyOn(p.log, 'info').mockImplementation(() => {}) const step = vi.spyOn(p.log, 'step').mockImplementation(() => {}) await runDrizzle() - // 0001 really is destructive now — this is the on-disk fact the report + // 0002 really is destructive now — this is the on-disk fact the report // used to lose. expect(fs.readFileSync(first, 'utf-8')).toContain('DROP COLUMN') + expect(fs.readFileSync(attempted, 'utf-8')).toBe('') const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] expect(options?.initialValue).toBe(false) expect(String(options?.message)).toContain('DESTROYS data') - expect(step.mock.calls.map((c) => String(c[0])).join('\n')).toContain(first) + const stepped = step.mock.calls.map((c) => String(c[0])).join('\n') + expect(stepped).toContain(first) + expect(stepped).toContain(attempted) + expect(stepped).toContain(skipped) + expect(info.mock.calls.flat().join('\n')).toContain( + 'Rewrote 2 migration file', + ) const warned = warn.mock.calls.map((c) => String(c[0])) expect(warned.some((msg) => msg.includes('data-destroying'))).toBe(true) + expect(warned.some((msg) => msg.includes('1 statement(s) look like'))).toBe( + true, + ) expect(warned.some((msg) => msg.includes('did not fully complete'))).toBe( true, ) warn.mockRestore() + info.mockRestore() step.mockRestore() }) diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index da4329c56..6fa339d64 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -49,6 +49,26 @@ const failWriteNumber = (nth: number, message: string): void => { ) } +/** + * 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 + ) => { + 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 @@ -1751,7 +1771,7 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () return { first, second } } - it('rejects with the files it had already rewritten', async () => { + 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. @@ -1767,12 +1787,26 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () expect((error as PartialRewriteError).message).toBe( 'ENOSPC: no space left on device', ) - expect((error as PartialRewriteError).rewritten).toEqual([first]) + 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'), @@ -1783,8 +1817,9 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () 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( - path.join(tmpDir, '0002_encrypt_name.sql'), + 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. @@ -1795,7 +1830,7 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () ) expect(error).toBeInstanceOf(PartialRewriteError) - expect((error as PartialRewriteError).rewritten).toEqual([]) + expect((error as PartialRewriteError).rewritten).toEqual([attempted]) expect((error as PartialRewriteError).skipped).toEqual([ expect.objectContaining({ file: nearMiss, reason: 'unrecognised-form' }), ]) @@ -1805,15 +1840,7 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () // rejecting with the ORIGINAL error — its `code` and identity intact — so the // "nothing is known about this directory" path stays exactly as it was. it('rethrows the original error when nothing had been done yet', async () => { - fs.writeFileSync( - path.join(tmpDir, '0000_declare.sql'), - 'CREATE TABLE "users" ("email" text);\n', - ) - fs.writeFileSync( - path.join(tmpDir, '0001_encrypt_email.sql'), - 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', - ) - failWriteNumber(1, 'EROFS: read-only file system') + fs.mkdirSync(path.join(tmpDir, '0001_broken.sql')) const error = await rewriteEncryptedAlterColumns(tmpDir).catch( (err: unknown) => err, @@ -1821,7 +1848,7 @@ describe('rewriteEncryptedAlterColumns — a write that fails mid-directory', () expect(error).toBeInstanceOf(Error) expect(error).not.toBeInstanceOf(PartialRewriteError) - expect((error as Error).message).toBe('EROFS: read-only file system') + expect((error as NodeJS.ErrnoException).code).toBe('EISDIR') }) }) @@ -1958,8 +1985,9 @@ describe('sweepMigrationDirs', () => { first, 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', ) + const attempted = path.join(abs, '0002_encrypt_name.sql') fs.writeFileSync( - path.join(abs, '0002_encrypt_name.sql'), + attempted, 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', ) failWriteNumber(2, 'ENOSPC: no space left on device') @@ -1967,7 +1995,7 @@ describe('sweepMigrationDirs', () => { const results = await sweepMigrationDirs(tmpDir, ['drizzle']) expect(results[0].error).toBe('ENOSPC: no space left on device') - expect(results[0].rewritten).toEqual([first]) + expect(results[0].rewritten).toEqual([first, attempted]) }) // Unchanged behaviour, pinned so the fix above cannot bleed into it: a diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 76fc4f512..ed05a27f0 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -668,24 +668,29 @@ export interface RewriteResult { /** * Thrown by {@link rewriteEncryptedAlterColumns} when it fails PART WAY through - * a directory it has already changed. + * a directory where it has attempted a write. * * The sweep writes one file at a time, so a failure on the third file leaves - * the first two rewritten on disk — each holding a live `DROP COLUMN`. A bare - * rejection discards that fact: the accumulated `rewritten` array dies with the - * stack frame, and every caller then reports the directory as merely + * the first two rewritten on disk — each holding a live `DROP COLUMN` — and may + * truncate or partially replace the third before rejecting. A bare rejection + * discards that fact: the accumulated `rewritten` array dies with the stack + * frame, and every caller then reports the directory as merely * *unchecked*, telling the user to review those migrations without saying that * some of them now destroy data. That is the fail-open inversion this rewriter * exists to prevent, so the work already done travels with the failure (#786). * * `message` is the underlying failure's, verbatim — callers render it straight * to the user — and the original is kept as `cause`. Thrown ONLY when there is - * partial work to report: a sweep that fails before changing or flagging - * anything rejects with the original error untouched, because "nothing is known - * about this directory" is a different state, and not a destructive one. + * partial work to report: a sweep that fails before attempting a write or + * flagging anything rejects with the original error untouched, because + * "nothing is known about this directory" is a different state, and not a + * destructive one. */ export class PartialRewriteError extends Error { - /** Absolute paths of the files already rewritten when the failure hit. */ + /** + * Absolute paths of completed and attempted rewrites. A rejected write may + * already have mutated its destination, so its path is included. + */ readonly rewritten: string[] /** Near-miss statements already flagged when the failure hit. */ readonly skipped: SkippedAlter[] @@ -847,8 +852,8 @@ export async function rewriteEncryptedAlterColumns( ) if (updated !== original) { - await writeFile(filePath, updated, 'utf-8') rewritten.push(filePath) + await writeFile(filePath, updated, 'utf-8') } // Broad secondary scan on the POST-rewrite content: anything still carrying @@ -872,9 +877,9 @@ export async function rewriteEncryptedAlterColumns( } } } catch (error) { - // Nothing done yet means nothing to add: rethrow the original, `code` and - // identity intact, so the caller's "this directory went unchecked" wording - // stays exactly as it was. + // No write attempted and nothing flagged means nothing to add: rethrow the + // original, `code` and identity intact, so the caller's "this directory went + // unchecked" wording stays exactly as it was. if (rewritten.length === 0 && skipped.length === 0) throw error throw new PartialRewriteError(error, rewritten, skipped) } @@ -895,9 +900,10 @@ export interface DirRewriteResult extends RewriteResult { * Set when this directory's sweep threw; the sweep continues regardless. * * It does NOT imply `rewritten` and `skipped` are empty — a sweep can fail - * after it has already rewritten files, and those files are reported here + * after it has attempted file rewrites, and those paths are reported here * alongside the error. Both facts hold at once, so a caller must report the - * rewrites (data-destroying) as well as the failure (unchecked remainder). + * possible rewrites (data-destroying) as well as the failure (unchecked + * remainder). */ error?: string /** @@ -990,8 +996,8 @@ export async function sweepMigrationDirs( } catch (err) { const message = err instanceof Error ? err.message : String(err) // Hard-coding zeros here would be a CLAIM — "this directory was not - // changed" — and it is false whenever the sweep threw after a write. The - // caller reads these arrays to decide whether to warn about data + // changed" — and it may be false whenever the sweep threw during a write. + // The caller reads these arrays to decide whether to warn about data // destruction, so emptying them downgrades a destructive outcome to a // merely unchecked one (#786). A failure with nothing behind it still // reports zeros, because for that directory they are true. From c5ffd59b8c04e8ee788970369c1f882db7bb4e10 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 28 Jul 2026 12:45:49 +1000 Subject: [PATCH 3/5] fix(wizard,cli): preserve partial sweep warnings --- .../sweep-partial-rewrite-destructive.md | 8 ++++- .../commands/eql/__tests__/migration.test.ts | 30 +++++++++++++++++++ packages/cli/src/commands/eql/migration.ts | 10 +++---- .../wizard/src/__tests__/post-agent.test.ts | 30 +++++++++++++++++++ packages/wizard/src/lib/post-agent.ts | 2 +- 5 files changed, 73 insertions(+), 7 deletions(-) diff --git a/.changeset/sweep-partial-rewrite-destructive.md b/.changeset/sweep-partial-rewrite-destructive.md index 7a792bab6..f818aeb28 100644 --- a/.changeset/sweep-partial-rewrite-destructive.md +++ b/.changeset/sweep-partial-rewrite-destructive.md @@ -15,7 +15,9 @@ 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. +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 @@ -30,6 +32,10 @@ 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 attempting any write is unchanged: it rejects with the original error, reports zeros, and keeps the softer "nothing is known about this directory" wording, because claiming data destruction there would be a diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index 924014660..6cfd16c52 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -450,6 +450,36 @@ describe('eqlMigrationCommand — Drizzle', () => { 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('aborts (exit 1) when drizzle-kit exits non-zero', async () => { diff --git a/packages/cli/src/commands/eql/migration.ts b/packages/cli/src/commands/eql/migration.ts index d698c71e6..d3ed995ec 100644 --- a/packages/cli/src/commands/eql/migration.ts +++ b/packages/cli/src/commands/eql/migration.ts @@ -267,6 +267,7 @@ async function generateDrizzleEqlMigration( // Either way the user must review sibling migrations before running migrate, // so surface it again at the closing note (below) — not just inline here. let sweepIncomplete = false + let sweepFailure: string | undefined // Reported AFTER the try/catch rather than inside it, because a failed sweep // is not an empty one: it writes a file at a time, so a throw part way // through leaves earlier files already rewritten and holding a live @@ -286,11 +287,9 @@ async function generateDrizzleEqlMigration( rewritten = error.rewritten skipped = error.skipped } - p.log.warn( - `Could not sweep ${outDir} for unsafe ALTER COLUMN statements: ${ - error instanceof Error ? error.message : String(error) - }`, - ) + sweepFailure = `Could not sweep ${outDir} for unsafe ALTER COLUMN statements: ${ + error instanceof Error ? error.message : String(error) + }` } if (rewritten.length > 0) { @@ -309,6 +308,7 @@ async function generateDrizzleEqlMigration( p.log.step(` ${describeSkipReason(reason)}`) } } + if (sweepFailure) p.log.warn(sweepFailure) p.log.success(`Migration created: ${migrationPath}`) if (sweepIncomplete) { diff --git a/packages/wizard/src/__tests__/post-agent.test.ts b/packages/wizard/src/__tests__/post-agent.test.ts index fd6821dab..d14dd0270 100644 --- a/packages/wizard/src/__tests__/post-agent.test.ts +++ b/packages/wizard/src/__tests__/post-agent.test.ts @@ -464,6 +464,36 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { expect(String(options?.message)).toContain('could not check 1 directory') }) + // A failed directory can still carry near-misses found before the failure. + // Keep both safety facts in the prompt: the flagged ALTER remains broken, + // and the rest of that named directory was not verified. + it('keeps flagged guidance and names the unchecked directory when both states apply', async () => { + vi.mocked(sweepMigrationDirs).mockResolvedValueOnce([ + { + dir: 'drizzle', + rewritten: [], + skipped: [ + { + file: '/tmp/fake/drizzle/0001_using.sql', + statement: + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search USING email::eql_v3_text_search;', + reason: 'unrecognised-form', + }, + ], + error: 'EIO', + }, + ]) + + await runDrizzle() + + const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] + expect(options?.initialValue).toBe(false) + expect(String(options?.message)).toContain('flagged for review') + expect(String(options?.message)).toContain('nothing was destroyed') + expect(String(options?.message)).toContain('could not check 1 directory') + expect(String(options?.message)).toContain('drizzle/') + }) + // The wizard ships scanning drizzle/, migrations/ and src/db/migrations/ and // indexes each SEPARATELY — the per-directory index is the mechanism the // fail-closed rule exists to make safe. Every test above uses only drizzle/, diff --git a/packages/wizard/src/lib/post-agent.ts b/packages/wizard/src/lib/post-agent.ts index a2d9792f8..9be863f81 100644 --- a/packages/wizard/src/lib/post-agent.ts +++ b/packages/wizard/src/lib/post-agent.ts @@ -114,7 +114,7 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { message: destructive ? `Run the migration now? (${runner} drizzle-kit migrate) — see the warnings above: this migration DESTROYS data on any table that already holds rows` : flaggedOnly - ? `Run the migration now? (${runner} drizzle-kit migrate) — statement(s) were flagged for review above rather than rewritten; nothing was destroyed, but the raw ALTER will fail at migrate time until they're resolved` + ? `Run the migration now? (${runner} drizzle-kit migrate) — statement(s) were flagged for review above rather than rewritten; nothing was destroyed, but the raw ALTER will fail at migrate time until they're resolved${unverified ? `; the sweep also could not check ${unverifiedCount} (${unverifiedList}), so review those migrations before migrating` : ''}` : unverified ? `Run the migration now? (${runner} drizzle-kit migrate) — the sweep could not check ${unverifiedCount} (${unverifiedList}); review those migrations before migrating, or you may apply broken/unsafe SQL` : `Run the migration now? (${runner} drizzle-kit migrate)`, From 35570a4129e3f8adde518f404110df9c4348d53c Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 28 Jul 2026 13:39:42 +1000 Subject: [PATCH 4/5] docs(cli): clarify partial sweep results --- .changeset/sweep-partial-rewrite-destructive.md | 8 ++++---- skills/stash-cli/SKILL.md | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.changeset/sweep-partial-rewrite-destructive.md b/.changeset/sweep-partial-rewrite-destructive.md index f818aeb28..0c8b82857 100644 --- a/.changeset/sweep-partial-rewrite-destructive.md +++ b/.changeset/sweep-partial-rewrite-destructive.md @@ -36,7 +36,7 @@ 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 attempting any write is unchanged: it rejects with -the original error, reports zeros, and keeps the softer "nothing is known about -this directory" wording, because claiming data destruction there would be a -guess. +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. diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 47c6eeea3..25dfc8dab 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -378,7 +378,7 @@ Pass exactly one of `--drizzle` / `--prisma`. The generated migration also insta After writing the migration, `--drizzle` sweeps sibling migrations containing an in-place `ALTER COLUMN … SET DATA TYPE `. When the source declaration is provably plaintext, it replaces the ALTER with an `ADD COLUMN` + `DROP` + `RENAME` sequence and lists the rewritten files. This is equivalent to DROP+ADD: it is safe only on an empty table and does not preserve data, constraints, defaults, or indexes. On a populated table, do not run that rewrite; use the staged EQL v3 rollout (add an encrypted twin, dual-write, backfill, switch the application to the encrypted column by name, then drop plaintext). If the source type cannot be proven, the statement remains unchanged and the command warns that the migration directory needs review. -If the sweep itself fails part way through — an unwritable file, a full disk, a lock held by an editor or `drizzle-kit` — it still lists the files it rewrote before the failure, *and* warns that it did not fully complete. Both matter: the listed files already contain the `DROP COLUMN`, and the rest of the directory went unchecked. Treat that directory as needing review before you run `drizzle-kit migrate`. +If the sweep itself fails part way through — an unwritable file, a full disk, a lock held by an editor or `drizzle-kit` — it still lists files it rewrote or attempted before the failure, *and* warns that it did not fully complete. An attempted write may already have partially modified, truncated, or left its destination unchanged, so inspect every listed file and the rest of the unchecked directory before you run `drizzle-kit migrate`. #### `eql upgrade` From 8115efc7eb1d6316f1a9d3640dcd657fac9ecf9b Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Wed, 29 Jul 2026 16:08:55 +1000 Subject: [PATCH 5/5] docs(cli),test(cli): mirror the public-qualifier rationale, pin the abort outro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #812. Five of the six findings did not survive verification against the branch; two changes came out of the two that had something real behind them. The wizard's `renderSafeAlter` carries a nine-line explanation of why the EQL domain is emitted as `"public".""` unconditionally — an assumption about where `eql install` puts its domains, not something read back from the matched SQL. The CLI twin emitted the identical string with no explanation, and its only nearby comment is about the TABLE's schema qualifier, which is the confusion the wizard's comment exists to head off. Mirror it, with each side naming the other, so the pair stays in step. Centralising was the reviewer's first suggestion, but `@cipherstash/wizard` does not depend on `stash` and nothing in the repo does — sharing ~1000 lines of pre-existing duplicated rewriter needs a new workspace package, which does not belong in this PR. The abort-outro behaviour was pinned only negatively: one test asserts `embedded` SUPPRESSES `p.outro('Migration aborted.')`. A regression that dropped the outro from the abort paths entirely would still satisfy that. Add the positive half — standalone, every post-intro abort closes the banner it opened, or clack renders a half-drawn frame. Verified by mutation: deleting the outro at the drizzle-kit-failure path fails the new test and nothing else. No changeset: comment-only plus test-only, no observable behaviour change, and the sweep fix's own changeset already covers the PR. --- .../cli/src/commands/db/rewrite-migrations.ts | 9 +++++++++ .../src/commands/eql/__tests__/migration.test.ts | 16 ++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 2488375aa..047448b0c 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -918,6 +918,15 @@ function renderSafeAlter( '-- NOTE: constraints, defaults, and indexes on the original column are NOT', '-- carried over by this ADD/DROP/RENAME — re-add any NOT NULL, DEFAULT,', '-- UNIQUE, or index definitions manually.', + // The domain is emitted as `"public".""` unconditionally: EQL + // installs its domains into `public` (both `stash eql install` and the + // adapters' baseline migrations do), and the qualifier makes the rewrite + // independent of the session `search_path`. It is an ASSUMPTION, not + // something read back from the matched SQL — the `schema` capture above is + // the TABLE's schema (from a pgSchema() table) and says nothing about where + // the domain lives. If EQL ever supports installing into a non-`public` + // schema, this needs the install schema threaded in, here and in the + // sibling `packages/wizard/src/lib/rewrite-migrations.ts`. `ALTER TABLE ${qualifiedTable} ADD COLUMN "${tmp}" "public"."${domain}";`, `-- UPDATE ${qualifiedTable} SET "${tmp}" = /* encrypted value for ${column} */ NULL`, '--> statement-breakpoint', diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index 6cfd16c52..57d888864 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -490,6 +490,22 @@ describe('eqlMigrationCommand — Drizzle', () => { expect(clack.log.error).toHaveBeenCalledWith('boom') }) + // The positive half of the `embedded` contract asserted below: standalone, + // every post-intro abort must CLOSE the banner it opened. `p.intro` without a + // matching `p.outro` leaves clack's box unterminated, so the abort renders as + // a half-drawn frame. Only the negative (embedded suppresses it) was pinned, + // which a regression dropping the outro entirely would still satisfy. + it('closes the intro banner with the abort outro when standalone', async () => { + spawnMock.mockReturnValue({ status: 1, stdout: '', stderr: 'boom' }) + + await expect( + eqlMigrationCommand({ drizzle: true, out: join(tmp, 'drizzle') }), + ).rejects.toBeInstanceOf(CliExit) + + expect(clack.intro).toHaveBeenCalled() + expect(clack.outro).toHaveBeenCalledWith('Migration aborted.') + }) + it('removes the scaffolded migration when writing the SQL fails', async () => { const out = join(tmp, 'drizzle') mkdirSync(out, { recursive: true })