From f551a723c0e7e5bcc9d0baf4d9eeddbd65e4543a Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 10:35:21 +1000 Subject: [PATCH 1/8] fix(cli,wizard): detect non-public-schema and array EQL domains as encrypted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The corpus index recognised an encrypted column only in the mangled forms drizzle-kit emits and the literal `public` schema. A domain installed into another schema ("app"."eql_v3_text_search") or an array of a domain (public.eql_v3_text_search[]) fell to the plaintext residue, so an ALTER on such a column was rewritten into ADD+DROP+RENAME — dropping ciphertext the corpus itself showed was encrypted. ENCRYPTED_TYPE_REF now admits any quoted schema and a trailing '[', closing both. It feeds only the encrypted index, never the rewrite matcher, so the worst case is a flagged statement, not a new rewrite. Applied to both copies of the sweep; changeset guarantee updated to match. --- .changeset/rewriter-never-drops-ciphertext.md | 5 +- .../src/__tests__/rewrite-migrations.test.ts | 48 +++++++++++++++++++ .../cli/src/commands/db/rewrite-migrations.ts | 29 +++++++---- .../src/__tests__/rewrite-migrations.test.ts | 48 +++++++++++++++++++ packages/wizard/src/lib/rewrite-migrations.ts | 29 +++++++---- 5 files changed, 138 insertions(+), 21 deletions(-) diff --git a/.changeset/rewriter-never-drops-ciphertext.md b/.changeset/rewriter-never-drops-ciphertext.md index 66ad0f20..ec82f4b6 100644 --- a/.changeset/rewriter-never-drops-ciphertext.md +++ b/.changeset/rewriter-never-drops-ciphertext.md @@ -22,7 +22,10 @@ either kind makes the rest of the file inert rather than live. The sweep also refuses to rewrite a column the migration corpus already gives an encrypted type, so changing a column's encrypted domain no longer drops a column -full of ciphertext. Skipped statements report why they were left alone. +full of ciphertext. Skipped statements report why they were left alone. This +recognises the encrypted forms drizzle-kit emits, a domain installed into a +non-`public` schema, and an array of a domain — so a corpus that shows a column +as encrypted in any of those shapes is flagged, not rewritten. The sweep is now fail-closed about the columns it does not recognise at all. Previously a column missing from the corpus index was assumed to be plaintext diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 275ee585..d6e357ea 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -865,6 +865,54 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped[0].reason).toBe('already-encrypted') }) + // An EQL domain installed into a NON-`public` schema is still ciphertext. + // The mangled forms special-case the literal `public`, so without the + // extra alternative this column falls to the plaintext residue and the + // ALTER drops a column full of ciphertext. + it('refuses to rewrite a domain change on a column encrypted in a non-public schema', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\t"email" "app"."eql_v3_text_eq"', + ');', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_domain-change.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) + + // An ARRAY of an EQL domain is still ciphertext. The trailing delimiter + // lookahead must admit `[` or the column falls to the plaintext residue. + it('refuses to rewrite a domain change on a column encrypted as a domain array', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_add.sql'), + 'ALTER TABLE "users" ADD COLUMN "email" public.eql_v3_text_eq[];\n', + ) + const alter = path.join(tmpDir, '0001_domain-change.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) + // A previous sweep of this directory leaves ADD tmp + RENAME behind. The // column it renamed onto is encrypted, so a later domain change on it is // just as destructive. diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index cf3d0e0b..61acc600 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -256,8 +256,17 @@ const TABLE_REF = String.raw`"([^"]+)"(?:\."([^"]+)")?` /** * An encrypted type in any of the {@link MANGLED_TYPE_FORMS}, pinned to end at a * delimiter so a bare domain cannot match a prefix of a longer identifier. + * + * Two shapes {@link MANGLED_TYPE_FORMS} does not enumerate are folded in here so + * the encrypted index recognises them and the ADD+DROP+RENAME cannot drop their + * ciphertext: a domain in ANY quoted schema (`"app"."eql_v3_text_search"`, not + * just the literal `public` the mangled forms special-case), and a `[` in the + * trailing lookahead so an ARRAY of the domain (`public.eql_v3_text_search[]`) + * ends at a delimiter too. Both feed only the encrypted index — never the + * rewrite matcher — so the worst case of admitting one is a flagged statement, + * the same safe asymmetry the fail-closed rule already relies on. */ -const ENCRYPTED_TYPE_REF = String.raw`(?:${MANGLED_TYPE_FORMS})(?=[\s,;)]|$)` +const ENCRYPTED_TYPE_REF = String.raw`(?:${MANGLED_TYPE_FORMS}|"[^"]+"\."(?:${ENCRYPTED_DOMAIN})")(?=[\s,;)[]|$)` /** `ALTER TABLE … ADD COLUMN "col" ` — $1/$2 table, $3 column. */ const ADD_ENCRYPTED_COLUMN_RE = new RegExp( @@ -328,17 +337,17 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( * the fail-closed rule needs no type classification and no SQL parsing, only * the known encrypted list that {@link ENCRYPTED_TYPE_REF} already provides. * - * **The residue claim depends on {@link MANGLED_TYPE_FORMS} covering every + * **The residue claim depends on {@link ENCRYPTED_TYPE_REF} recognising every * encrypted shape.** "By residue, plaintext" is only as good as the encrypted * side's coverage: a declaration {@link ENCRYPTED_TYPE_REF} fails to recognise - * falls to the plaintext residue and gets rewritten. Two forms do this: a - * domain installed into a non-`public` schema (`"email" "app"."eql_v3_text_search"`, - * since the mangled forms only special-case the literal `public` schema), and - * an array of the domain (`ADD COLUMN "email" public.eql_v3_text_search[]`, - * since {@link ENCRYPTED_TYPE_REF}'s trailing delimiter lookahead does not - * include `[`). Neither is a layout EQL installs into or a shape drizzle-kit - * emits, and both behaved identically before this branch — so this is a - * documentation gap, not a regression this branch introduced. + * falls to the plaintext residue and gets rewritten. Two such shapes are now + * covered explicitly by {@link ENCRYPTED_TYPE_REF} — a domain in a non-`public` + * schema (`"email" "app"."eql_v3_text_search"`) and an array of the domain + * (`"email" public.eql_v3_text_search[]`) — because both name ciphertext the + * corpus can see, so rewriting them is the exact drop this rule exists to + * prevent. The dependency itself remains: any encrypted shape a future EQL + * install introduces must be added to {@link ENCRYPTED_TYPE_REF} or it too will + * fall to the plaintext residue. * * **Residue, accepted.** The lookahead is a fixed keyword list, not a parser: * a predicate keyword it does not enumerate (`SIMILAR`, `ISNULL`, `NOTNULL`, diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index 01804dae..fb9e08de 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -795,6 +795,54 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped[0].reason).toBe('already-encrypted') }) + // An EQL domain installed into a NON-`public` schema is still ciphertext. + // The mangled forms special-case the literal `public`, so without the + // extra alternative this column falls to the plaintext residue and the + // ALTER drops a column full of ciphertext. + it('refuses to rewrite a domain change on a column encrypted in a non-public schema', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\t"email" "app"."eql_v3_text_eq"', + ');', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_domain-change.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) + + // An ARRAY of an EQL domain is still ciphertext. The trailing delimiter + // lookahead must admit `[` or the column falls to the plaintext residue. + it('refuses to rewrite a domain change on a column encrypted as a domain array', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_add.sql'), + 'ALTER TABLE "users" ADD COLUMN "email" public.eql_v3_text_eq[];\n', + ) + const alter = path.join(tmpDir, '0001_domain-change.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) + // A previous sweep of this directory leaves ADD tmp + RENAME behind. The // column it renamed onto is encrypted, so a later domain change on it is // just as destructive. diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 0255df6b..a8afed6e 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -264,8 +264,17 @@ const TABLE_REF = String.raw`"([^"]+)"(?:\."([^"]+)")?` /** * An encrypted type in any of the {@link MANGLED_TYPE_FORMS}, pinned to end at a * delimiter so a bare domain cannot match a prefix of a longer identifier. + * + * Two shapes {@link MANGLED_TYPE_FORMS} does not enumerate are folded in here so + * the encrypted index recognises them and the ADD+DROP+RENAME cannot drop their + * ciphertext: a domain in ANY quoted schema (`"app"."eql_v3_text_search"`, not + * just the literal `public` the mangled forms special-case), and a `[` in the + * trailing lookahead so an ARRAY of the domain (`public.eql_v3_text_search[]`) + * ends at a delimiter too. Both feed only the encrypted index — never the + * rewrite matcher — so the worst case of admitting one is a flagged statement, + * the same safe asymmetry the fail-closed rule already relies on. */ -const ENCRYPTED_TYPE_REF = String.raw`(?:${MANGLED_TYPE_FORMS})(?=[\s,;)]|$)` +const ENCRYPTED_TYPE_REF = String.raw`(?:${MANGLED_TYPE_FORMS}|"[^"]+"\."(?:${ENCRYPTED_DOMAIN})")(?=[\s,;)[]|$)` /** `ALTER TABLE … ADD COLUMN "col" ` — $1/$2 table, $3 column. */ const ADD_ENCRYPTED_COLUMN_RE = new RegExp( @@ -336,17 +345,17 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( * the fail-closed rule needs no type classification and no SQL parsing, only * the known encrypted list that {@link ENCRYPTED_TYPE_REF} already provides. * - * **The residue claim depends on {@link MANGLED_TYPE_FORMS} covering every + * **The residue claim depends on {@link ENCRYPTED_TYPE_REF} recognising every * encrypted shape.** "By residue, plaintext" is only as good as the encrypted * side's coverage: a declaration {@link ENCRYPTED_TYPE_REF} fails to recognise - * falls to the plaintext residue and gets rewritten. Two forms do this: a - * domain installed into a non-`public` schema (`"email" "app"."eql_v3_text_search"`, - * since the mangled forms only special-case the literal `public` schema), and - * an array of the domain (`ADD COLUMN "email" public.eql_v3_text_search[]`, - * since {@link ENCRYPTED_TYPE_REF}'s trailing delimiter lookahead does not - * include `[`). Neither is a layout EQL installs into or a shape drizzle-kit - * emits, and both behaved identically before this branch — so this is a - * documentation gap, not a regression this branch introduced. + * falls to the plaintext residue and gets rewritten. Two such shapes are now + * covered explicitly by {@link ENCRYPTED_TYPE_REF} — a domain in a non-`public` + * schema (`"email" "app"."eql_v3_text_search"`) and an array of the domain + * (`"email" public.eql_v3_text_search[]`) — because both name ciphertext the + * corpus can see, so rewriting them is the exact drop this rule exists to + * prevent. The dependency itself remains: any encrypted shape a future EQL + * install introduces must be added to {@link ENCRYPTED_TYPE_REF} or it too will + * fall to the plaintext residue. * * **Residue, accepted.** The lookahead is a fixed keyword list, not a parser: * a predicate keyword it does not enumerate (`SIMILAR`, `ISNULL`, `NOTNULL`, From 0cfd135aeeffd7fe35472c04af6ad4fb1da72783 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 10:43:10 +1000 Subject: [PATCH 2/8] test(cli,wizard): pin describeSkipReason's three reason mappings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit describeSkipReason had zero coverage repo-wide — replacing its body with a constant left every suite green. Its output is the guidance a user acts on when a statement is skipped, and source-unknown's is new. Pin each reason to a distinctive substring so a mis-mapped case is caught. --- .../src/__tests__/rewrite-migrations.test.ts | 42 ++++++++++++++++++- .../src/__tests__/rewrite-migrations.test.ts | 38 +++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index d6e357ea..f014cfec 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -2,7 +2,10 @@ import fs from 'node:fs' import os from 'node:os' import path from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' -import { rewriteEncryptedAlterColumns } from '../commands/db/rewrite-migrations.js' +import { + describeSkipReason, + rewriteEncryptedAlterColumns, +} from '../commands/db/rewrite-migrations.js' describe('rewriteEncryptedAlterColumns', () => { let tmpDir: string @@ -1379,3 +1382,40 @@ describe('rewriteEncryptedAlterColumns', () => { }) }) }) + +// 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 +// mis-MAPPED case (wiring `source-unknown` to the `already-encrypted` string) +// compiles fine and ships wrong remediation into a data-loss decision. Pin the +// mapping so that swap is caught. +describe('describeSkipReason', () => { + it('describes already-encrypted as a re-encrypt-through-the-lifecycle action', () => { + const text = describeSkipReason('already-encrypted') + expect(text).toContain('ALREADY encrypted') + expect(text).toContain('DROP the ciphertext') + expect(text).toContain('`stash encrypt` lifecycle') + }) + + it('describes unrecognised-form as a hand-authored / unknown cast', () => { + const text = describeSkipReason('unrecognised-form') + expect(text).toContain('SET DATA TYPE ... USING') + expect(text).toContain('fails at migrate time') + }) + + it('describes source-unknown as a go-check-the-database action', () => { + const text = describeSkipReason('source-unknown') + expect(text).toContain('could not find where this column was declared') + expect(text).toContain("Check the column's current type in the database") + }) + + it('gives each reason a distinct description', () => { + const reasons = [ + 'already-encrypted', + 'unrecognised-form', + 'source-unknown', + ] as const + const described = reasons.map(describeSkipReason) + expect(new Set(described).size).toBe(reasons.length) + }) +}) diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index fb9e08de..22d10692 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -3,6 +3,7 @@ import os from 'node:os' import path from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { + describeSkipReason, rewriteEncryptedAlterColumns, sweepMigrationDirs, } from '../lib/rewrite-migrations.js' @@ -1452,3 +1453,40 @@ describe('sweepMigrationDirs', () => { expect(updated).not.toContain('DROP COLUMN') }) }) + +// 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 +// mis-MAPPED case (wiring `source-unknown` to the `already-encrypted` string) +// compiles fine and ships wrong remediation into a data-loss decision. Pin the +// mapping so that swap is caught. +describe('describeSkipReason', () => { + it('describes already-encrypted as a re-encrypt-through-the-lifecycle action', () => { + const text = describeSkipReason('already-encrypted') + expect(text).toContain('ALREADY encrypted') + expect(text).toContain('DROP the ciphertext') + expect(text).toContain('`stash encrypt` lifecycle') + }) + + it('describes unrecognised-form as a hand-authored / unknown cast', () => { + const text = describeSkipReason('unrecognised-form') + expect(text).toContain('SET DATA TYPE ... USING') + expect(text).toContain('fails at migrate time') + }) + + it('describes source-unknown as a go-check-the-database action', () => { + const text = describeSkipReason('source-unknown') + expect(text).toContain('could not find where this column was declared') + expect(text).toContain("Check the column's current type in the database") + }) + + it('gives each reason a distinct description', () => { + const reasons = [ + 'already-encrypted', + 'unrecognised-form', + 'source-unknown', + ] as const + const described = reasons.map(describeSkipReason) + expect(new Set(described).size).toBe(reasons.length) + }) +}) From 301f7e0fde49cca1e3399ad55333746e40357935 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 10:52:27 +1000 Subject: [PATCH 3/8] test(cli,wizard): cover the sweep's command and multi-directory layers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep's unit tests are thorough; everything above them was thin. - The clack log mock in migration.test.ts and generate-drizzle-migration.test.ts omitted `step`, so the per-statement report threw and landed in the command's catch — no test ever saw the report block. Add `step`. - eql migration: assert source-unknown guidance reaches the user and the closing warning fires for an undeclared column. - db install (generateDrizzleMigration): its step-5 sweep block had zero tests — every other test's out dir held only the skipped generated file. Add a genuine rewrite and a source-unknown skip. - wizard post-agent: harden the clean default-to-Yes arm to assert it carries no warning phrase, and add two multi-directory tests that put the actionable migration in a non-first candidate, so shrinking DRIZZLE_OUT_DIRS or short-circuiting the aggregation loop fails. --- .../generate-drizzle-migration.test.ts | 75 ++++++++++++++++- .../commands/eql/__tests__/migration.test.ts | 49 ++++++++++- .../wizard/src/__tests__/post-agent.test.ts | 82 +++++++++++++++++++ 3 files changed, 204 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/db/__tests__/generate-drizzle-migration.test.ts b/packages/cli/src/commands/db/__tests__/generate-drizzle-migration.test.ts index 3bfc0380..cf4f085e 100644 --- a/packages/cli/src/commands/db/__tests__/generate-drizzle-migration.test.ts +++ b/packages/cli/src/commands/db/__tests__/generate-drizzle-migration.test.ts @@ -17,7 +17,15 @@ import { messages } from '../../../messages.js' // through. The spinner instance doubles as the `s` argument. const clack = vi.hoisted(() => ({ spinnerInstance: { start: vi.fn(), stop: vi.fn() }, - log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), success: vi.fn() }, + // `step` is on the real clack `log`; the sweep's per-statement report calls + // it, so it must be present or the report throws into the catch unseen. + log: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + step: vi.fn(), + }, intro: vi.fn(), note: vi.fn(), outro: vi.fn(), @@ -165,6 +173,71 @@ describe('generateDrizzleMigration', () => { expect(written).toContain('cs_migrations') }) + // Step 5 of the generator sweeps sibling migrations. This block had no test at + // all: every other test's out dir contains only the file the generator wrote + // (which is `skip`ped), so the sweep never had anything to act on. A sibling + // ALTER on a plaintext column the corpus declares must be rewritten. + it('rewrites a sibling ALTER on a declared plaintext column', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + writeFileSync( + join(out, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + const sibling = join(out, '0001_encrypt-email.sql') + writeFileSync( + sibling, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "public"."eql_v3_text_search";\n', + ) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0002_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + + await generateDrizzleMigration(spinner, { out }) + + const rewritten = readFileSync(sibling, 'utf-8') + expect(rewritten).toContain( + 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_search";', + ) + expect(rewritten).not.toContain('SET DATA TYPE') + const info = clack.log.info.mock.calls.map((c) => String(c[0])) + expect(info.some((msg) => msg.includes('Rewrote 1 migration file'))).toBe( + true, + ) + }) + + // A sibling ALTER on a column the corpus never declares is fail-closed to + // `source-unknown`: left on disk and reported with its remediation. Exercises + // the skip branch of the step-5 report, which no db-install test reached. + it('reports source-unknown for a sibling ALTER on an undeclared column', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + const sibling = join(out, '0001_encrypt-email.sql') + const unsafeAlter = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "public"."eql_v3_text_search";\n' + writeFileSync(sibling, unsafeAlter) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0002_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + + await generateDrizzleMigration(spinner, { out }) + + // Never rewritten — its current type is unknown and could be ciphertext. + expect(readFileSync(sibling, 'utf-8')).toBe(unsafeAlter) + const stepped = clack.log.step.mock.calls.map((c) => String(c[0])) + expect( + stepped.some((msg) => + msg.includes("Check the column's current type in the database"), + ), + ).toBe(true) + const warned = clack.log.warn.mock.calls.map((c) => String(c[0])) + expect(warned.some((msg) => msg.includes('did not fully complete'))).toBe( + true, + ) + }) + it('includes --out in the dry-run preview', async () => { const out = join(tmp, 'custom-out') await generateDrizzleMigration(spinner, { dryRun: true, out }) diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index 9f0fa1f9..c0193b66 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -18,7 +18,16 @@ import { buildEqlV3MigrationSql, eqlMigrationCommand } from '../migration.js' // reports through. const clack = vi.hoisted(() => ({ spinnerInstance: { start: vi.fn(), stop: vi.fn() }, - log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), success: vi.fn() }, + // `step` is on the real clack `log`; omitting it made the sweep's + // per-statement report throw and land in the command's catch block, so no + // test ever saw the report. Keep it here. + log: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + step: vi.fn(), + }, intro: vi.fn(), note: vi.fn(), outro: vi.fn(), @@ -313,6 +322,44 @@ describe('eqlMigrationCommand — Drizzle', () => { ) }) + // A column the corpus never declares is fail-closed to `source-unknown`: left + // on disk, and reported so the user goes and checks its type. This is the most + // common skip on a real (e.g. squashed) corpus, and it renders through the + // `p.log.step` path that a missing mock method used to make throw — so assert + // the guidance text actually reaches the user, not just that a warning fired. + it('reports source-unknown guidance for an undeclared column and warns at the close', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + // No CREATE TABLE / ADD COLUMN anywhere declares "users"."email". + const sibling = join(out, '0001_encrypt-email.sql') + const unsafeAlter = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n' + writeFileSync(sibling, unsafeAlter) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0002_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + + await eqlMigrationCommand({ drizzle: true, out }) + + // Left exactly as written — a source-unknown statement is never rewritten. + expect(readFileSync(sibling, 'utf-8')).toBe(unsafeAlter) + // The per-statement report reached the user with the source-unknown + // remediation (the whole point of the reason), not a crash into the catch. + const stepped = clack.log.step.mock.calls.map((c) => String(c[0])) + expect(stepped.some((msg) => msg.includes(sibling))).toBe(true) + expect( + stepped.some((msg) => + msg.includes("Check the column's current type in the database"), + ), + ).toBe(true) + // And the closing note warns the sweep did not fully complete. + const warned = clack.log.warn.mock.calls.map((c) => String(c[0])) + 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/wizard/src/__tests__/post-agent.test.ts b/packages/wizard/src/__tests__/post-agent.test.ts index 8e7656d9..f8f87a79 100644 --- a/packages/wizard/src/__tests__/post-agent.test.ts +++ b/packages/wizard/src/__tests__/post-agent.test.ts @@ -155,6 +155,15 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] expect(options?.initialValue).toBe(true) + // The clean arm is the last of a 4-way nested ternary; the negative + // assertions that guard the other arms' wording never run against this one. + // A mis-nested arm here would ship a destruction/flag/unchecked warning to a + // user with nothing to sweep, and asserting only `initialValue` would miss + // it — so pin that the clean message carries none of those phrases. + const message = String(options?.message) + expect(message).not.toContain('DESTROYS data') + expect(message).not.toContain('flagged for review') + expect(message).not.toContain('could not check') }) it('defaults to No, and says why, when a file was rewritten', async () => { @@ -247,4 +256,77 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { expect(options?.initialValue).toBe(false) 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/, + // so shrinking the shipped constant to ['drizzle'], or short-circuiting the + // aggregation loop after the first directory, leaves them all green. These two + // put the actionable content in a NON-first directory so those regressions + // fail. + it('sweeps a candidate directory other than the first', async () => { + // drizzle/ exists but has nothing to do; the rewrite lives in migrations/. + fs.mkdirSync(path.join(cwd, 'drizzle')) + fs.writeFileSync( + path.join(cwd, 'drizzle', '0000_init.sql'), + 'CREATE TABLE "widgets" ("id" integer PRIMARY KEY);\n', + ) + fs.mkdirSync(path.join(cwd, 'migrations')) + fs.writeFileSync( + path.join(cwd, 'migrations', '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + fs.writeFileSync( + path.join(cwd, 'migrations', '0001_encrypt.sql'), + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + await runDrizzle() + + // The migrations/ ALTER was rewritten — proof the second directory was + // swept, not just drizzle/. + const swept = fs.readFileSync( + path.join(cwd, 'migrations', '0001_encrypt.sql'), + 'utf-8', + ) + expect(swept).toContain('DROP COLUMN') + expect(swept).not.toContain('SET DATA TYPE') + const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] + expect(options?.initialValue).toBe(false) + expect(String(options?.message)).toContain('DESTROYS data') + }) + + it('aggregates a rewrite in one directory with a flag in another', async () => { + // drizzle/ declares email, so its ALTER is rewritten. + fs.mkdirSync(path.join(cwd, 'drizzle')) + fs.writeFileSync( + path.join(cwd, 'drizzle', '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + fs.writeFileSync( + path.join(cwd, 'drizzle', '0001_encrypt.sql'), + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + // migrations/ never declares its column, so its ALTER is source-unknown. + fs.mkdirSync(path.join(cwd, 'migrations')) + const flagged = path.join(cwd, 'migrations', '0001_encrypt.sql') + const flaggedSql = + 'ALTER TABLE "orders" ALTER COLUMN "total" SET DATA TYPE eql_v3_text_search;\n' + fs.writeFileSync(flagged, flaggedSql) + + await runDrizzle() + + // drizzle/ was rewritten... + const rewritten = fs.readFileSync( + path.join(cwd, 'drizzle', '0001_encrypt.sql'), + 'utf-8', + ) + expect(rewritten).toContain('DROP COLUMN') + // ...and migrations/ was left untouched, flagged rather than rewritten. + expect(fs.readFileSync(flagged, 'utf-8')).toBe(flaggedSql) + // A rewrite anywhere makes the prompt destructive and defaults it to No. + const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] + expect(options?.initialValue).toBe(false) + expect(String(options?.message)).toContain('DESTROYS data') + }) }) From 8ebb174acac96b69ff2500ec89c49b77070d76c6 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 12:06:15 +1000 Subject: [PATCH 4/8] fix(cli,wizard): index the rewrite's own target, and resolve the implicit schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways a corpus could still lose ciphertext to the ADD+DROP+RENAME, both found by the #772 review. Neither is caught by the fail-closed `declared` rule, because in both the column really is declared — as plaintext, by the original CREATE TABLE. Finding 3 — chained conversions. `indexColumnDeclarations` reads CREATE TABLE, ADD COLUMN and RENAME, but never the strict matcher's own target. A corpus carrying an earlier `SET DATA TYPE eql_v2_encrypted` (a stack version that predates this sweep) left the column looking plaintext, so a later domain change dropped its ciphertext. Record the conversion as the sweep walks, in file-sorted order — not in the corpus-wide index, which has no order and would flag the legitimate first conversion too. Finding 4 — `"users"` and `"public"."users"` are the same table; Postgres resolves the unqualified name through `search_path`. drizzle-kit emits unqualified, while hand-written SQL and this sweep's own renderSafeAlter output are qualified, so a corpus mixing the two split one column across two keys: the already-encrypted guard missed, and the ALTER dropped ciphertext. Collapse the implicit schema in `columnKey`. Only that one — `"app"."users"` stays distinct, and there is no case folding, since TABLE_REF matches quoted identifiers only. Both fixes land in both copies of the rewriter. 7 tests each: the two destructive cases, the wrong-reason case finding 4 also produced (`source-unknown` for a column declared two files up), and four guards for the ways a naive fix breaks — the first conversion in a chain, a chain across different columns, a commented-out earlier ALTER, and a real non-public schema. --- .../src/__tests__/rewrite-migrations.test.ts | 182 ++++++++++++++++++ .../cli/src/commands/db/rewrite-migrations.ts | 42 +++- .../src/__tests__/rewrite-migrations.test.ts | 182 ++++++++++++++++++ packages/wizard/src/lib/rewrite-migrations.ts | 42 +++- 4 files changed, 444 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index f014cfec..8f999f85 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -1108,6 +1108,188 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped).toHaveLength(1) expect(skipped[0].reason).toBe('already-encrypted') }) + + // #772 review, finding 3. The index reads CREATE TABLE, ADD COLUMN and + // RENAME, but never the strict matcher's OWN target. So a corpus where an + // earlier migration already converted the column — realistic wherever a + // previous stack version's ALTER ran before this sweep existed — leaves + // that column looking plaintext, and the SECOND conversion rewrites a + // column that now holds ciphertext. `declared` cannot catch it: the column + // IS declared, as plaintext, by the original CREATE TABLE. + it('rewrites only the first conversion when two ALTERs chain on one column', async () => { + declarePlaintext('"users"', 'email') + const first = path.join(tmpDir, '0002_v2.sql') + fs.writeFileSync( + first, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v2_encrypted;\n', + ) + const secondSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const second = path.join(tmpDir, '0003_v3.sql') + fs.writeFileSync(second, `${secondSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + // The first conversion is the legitimate plaintext -> encrypted one and + // must still happen — flagging it too would be the naive fix. + expect(rewritten).toEqual([first]) + expect(fs.readFileSync(first, 'utf-8')).toContain('DROP COLUMN') + // The second targets ciphertext. Left byte-identical on disk. + expect(fs.readFileSync(second, 'utf-8')).toBe(`${secondSql}\n`) + expect(skipped).toEqual([ + { file: second, statement: secondSql, reason: 'already-encrypted' }, + ]) + }) + + it('flags the second of two chained ALTERs inside a single file', async () => { + declarePlaintext('"users"', 'email') + const secondSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0002_chain.sql') + fs.writeFileSync( + filePath, + [ + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v2_encrypted;', + secondSql, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([filePath]) + const updated = fs.readFileSync(filePath, 'utf-8') + // Exactly one conversion was applied, and the v3 statement survives verbatim. + expect(updated.match(/DROP COLUMN/g)?.length).toBe(1) + expect(updated).toContain(secondSql) + expect(skipped).toEqual([ + { file: filePath, statement: secondSql, reason: 'already-encrypted' }, + ]) + }) + + // A chain on DIFFERENT columns is not a chain — each is its own first + // conversion and both must be rewritten. + it('rewrites both when chained ALTERs target different columns', async () => { + declarePlaintext('"users"', 'email', 'name') + const first = path.join(tmpDir, '0002_email.sql') + fs.writeFileSync( + first, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + const second = path.join(tmpDir, '0003_name.sql') + fs.writeFileSync( + second, + 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([first, second]) + expect(skipped).toEqual([]) + }) + + // A commented-out first conversion never ran, so the column is still + // plaintext and the live second conversion is the legitimate one. + it('does not treat a commented-out earlier ALTER as having encrypted the column', async () => { + declarePlaintext('"users"', 'email') + fs.writeFileSync( + path.join(tmpDir, '0002_v2.sql'), + '-- ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v2_encrypted;\n', + ) + const live = path.join(tmpDir, '0003_v3.sql') + fs.writeFileSync( + live, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([live]) + expect(fs.readFileSync(live, 'utf-8')).toContain('DROP COLUMN') + }) + + // #772 review, finding 4. `columnKey` keys on the schema exactly as written, + // but Postgres resolves an unqualified name through `search_path` — so + // `"users"` and `"public"."users"` are the SAME table. drizzle-kit emits + // unqualified; hand-written SQL and this sweep's own output are qualified. + // A corpus mixing the two split the index across two keys. + it('treats "public"."users" and "users" as the same table when checking encryption', async () => { + // 0000 declares the column unqualified — drizzle-kit's default output. + fs.writeFileSync( + path.join(tmpDir, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + // 0001 is a previous sweep's output, schema-qualified. The RENAME leaves + // `email` holding ciphertext, recorded under the QUALIFIED key. + fs.writeFileSync( + path.join(tmpDir, '0001_encrypt.sql'), + [ + 'ALTER TABLE "public"."users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_eq";', + 'ALTER TABLE "public"."users" DROP COLUMN "email";', + 'ALTER TABLE "public"."users" RENAME COLUMN "email__cipherstash_tmp" TO "email";', + '', + ].join('\n'), + ) + // 0002 changes the domain, written unqualified. + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0002_domain-change.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(fs.readFileSync(alter, 'utf-8')).toBe(`${alterSql}\n`) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + + // The mirror direction: declared/encrypted unqualified, altered qualified. + // Before the fix this reported `source-unknown` — a WRONG reason, telling + // the user the corpus never declares a column it declares two files up. + it('resolves an unqualified declaration against a schema-qualified ALTER', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "users" ("email" "public"."eql_v3_text_eq");\n', + ) + const alterSql = + 'ALTER TABLE "public"."users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_domain-change.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + + // Only the IMPLICIT schema collapses. A real non-public schema is a + // genuinely different table and must not be conflated with the bare name. + it('does not conflate a non-public schema with the unqualified table', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_app.sql'), + 'CREATE TABLE "app"."users" ("email" "public"."eql_v3_text_eq");\n', + ) + fs.writeFileSync( + path.join(tmpDir, '0001_public.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + const alter = path.join(tmpDir, '0002_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + // public.users.email is plaintext — app.users.email being encrypted is + // about a different table entirely. + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) }) it('handles multiple ALTER statements in one file', async () => { diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 61acc600..2d027f0d 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -389,9 +389,29 @@ function tableOf( : { schema: first, table: second } } -/** Identity of a column across the corpus, for {@link indexColumnDeclarations}. */ +/** + * Identity of a column across the corpus, for {@link indexColumnDeclarations}. + * + * `public` and no schema at all are the SAME table: Postgres resolves an + * unqualified name through `search_path`, which starts at `public`. The two + * spellings coexist in one corpus routinely — drizzle-kit emits unqualified, + * while hand-written SQL and this sweep's own {@link renderSafeAlter} output + * are qualified. Keying on the literal text split one column across two keys, + * so a column already encrypted under one spelling looked plaintext when + * altered under the other and the ADD+DROP+RENAME dropped its ciphertext + * (#772 review, finding 4). + * + * Only the IMPLICIT schema collapses. `"app"."users"` is a genuinely different + * table from `"users"` and keeps its own key. No case folding either: + * `TABLE_REF` matches quoted identifiers only, and those are case-sensitive in + * Postgres — `"PUBLIC"` really is a different schema from `"public"`. + */ function columnKey(table: string, column: string, schema?: string): string { - return JSON.stringify([schema ?? '', table, column]) + return JSON.stringify([ + schema === 'public' ? '' : (schema ?? ''), + table, + column, + ]) } /** What the migration corpus says about the columns it mentions. */ @@ -657,6 +677,24 @@ export async function rewriteEncryptedAlterColumns( // 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) }, ) diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index 22d10692..3a1b189c 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -1036,6 +1036,188 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped).toHaveLength(1) expect(skipped[0].reason).toBe('already-encrypted') }) + + // #772 review, finding 3. The index reads CREATE TABLE, ADD COLUMN and + // RENAME, but never the strict matcher's OWN target. So a corpus where an + // earlier migration already converted the column — realistic wherever a + // previous stack version's ALTER ran before this sweep existed — leaves + // that column looking plaintext, and the SECOND conversion rewrites a + // column that now holds ciphertext. `declared` cannot catch it: the column + // IS declared, as plaintext, by the original CREATE TABLE. + it('rewrites only the first conversion when two ALTERs chain on one column', async () => { + declarePlaintext('"users"', 'email') + const first = path.join(tmpDir, '0002_v2.sql') + fs.writeFileSync( + first, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v2_encrypted;\n', + ) + const secondSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const second = path.join(tmpDir, '0003_v3.sql') + fs.writeFileSync(second, `${secondSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + // The first conversion is the legitimate plaintext -> encrypted one and + // must still happen — flagging it too would be the naive fix. + expect(rewritten).toEqual([first]) + expect(fs.readFileSync(first, 'utf-8')).toContain('DROP COLUMN') + // The second targets ciphertext. Left byte-identical on disk. + expect(fs.readFileSync(second, 'utf-8')).toBe(`${secondSql}\n`) + expect(skipped).toEqual([ + { file: second, statement: secondSql, reason: 'already-encrypted' }, + ]) + }) + + it('flags the second of two chained ALTERs inside a single file', async () => { + declarePlaintext('"users"', 'email') + const secondSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0002_chain.sql') + fs.writeFileSync( + filePath, + [ + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v2_encrypted;', + secondSql, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([filePath]) + const updated = fs.readFileSync(filePath, 'utf-8') + // Exactly one conversion was applied, and the v3 statement survives verbatim. + expect(updated.match(/DROP COLUMN/g)?.length).toBe(1) + expect(updated).toContain(secondSql) + expect(skipped).toEqual([ + { file: filePath, statement: secondSql, reason: 'already-encrypted' }, + ]) + }) + + // A chain on DIFFERENT columns is not a chain — each is its own first + // conversion and both must be rewritten. + it('rewrites both when chained ALTERs target different columns', async () => { + declarePlaintext('"users"', 'email', 'name') + const first = path.join(tmpDir, '0002_email.sql') + fs.writeFileSync( + first, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + const second = path.join(tmpDir, '0003_name.sql') + fs.writeFileSync( + second, + 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([first, second]) + expect(skipped).toEqual([]) + }) + + // A commented-out first conversion never ran, so the column is still + // plaintext and the live second conversion is the legitimate one. + it('does not treat a commented-out earlier ALTER as having encrypted the column', async () => { + declarePlaintext('"users"', 'email') + fs.writeFileSync( + path.join(tmpDir, '0002_v2.sql'), + '-- ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v2_encrypted;\n', + ) + const live = path.join(tmpDir, '0003_v3.sql') + fs.writeFileSync( + live, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([live]) + expect(fs.readFileSync(live, 'utf-8')).toContain('DROP COLUMN') + }) + + // #772 review, finding 4. `columnKey` keys on the schema exactly as written, + // but Postgres resolves an unqualified name through `search_path` — so + // `"users"` and `"public"."users"` are the SAME table. drizzle-kit emits + // unqualified; hand-written SQL and this sweep's own output are qualified. + // A corpus mixing the two split the index across two keys. + it('treats "public"."users" and "users" as the same table when checking encryption', async () => { + // 0000 declares the column unqualified — drizzle-kit's default output. + fs.writeFileSync( + path.join(tmpDir, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + // 0001 is a previous sweep's output, schema-qualified. The RENAME leaves + // `email` holding ciphertext, recorded under the QUALIFIED key. + fs.writeFileSync( + path.join(tmpDir, '0001_encrypt.sql'), + [ + 'ALTER TABLE "public"."users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_eq";', + 'ALTER TABLE "public"."users" DROP COLUMN "email";', + 'ALTER TABLE "public"."users" RENAME COLUMN "email__cipherstash_tmp" TO "email";', + '', + ].join('\n'), + ) + // 0002 changes the domain, written unqualified. + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0002_domain-change.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(fs.readFileSync(alter, 'utf-8')).toBe(`${alterSql}\n`) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + + // The mirror direction: declared/encrypted unqualified, altered qualified. + // Before the fix this reported `source-unknown` — a WRONG reason, telling + // the user the corpus never declares a column it declares two files up. + it('resolves an unqualified declaration against a schema-qualified ALTER', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "users" ("email" "public"."eql_v3_text_eq");\n', + ) + const alterSql = + 'ALTER TABLE "public"."users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_domain-change.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + + // Only the IMPLICIT schema collapses. A real non-public schema is a + // genuinely different table and must not be conflated with the bare name. + it('does not conflate a non-public schema with the unqualified table', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_app.sql'), + 'CREATE TABLE "app"."users" ("email" "public"."eql_v3_text_eq");\n', + ) + fs.writeFileSync( + path.join(tmpDir, '0001_public.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + const alter = path.join(tmpDir, '0002_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + // public.users.email is plaintext — app.users.email being encrypted is + // about a different table entirely. + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) }) it('handles multiple ALTER statements in one file', async () => { diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index a8afed6e..6cea3886 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -397,9 +397,29 @@ function tableOf( : { schema: first, table: second } } -/** Identity of a column across the corpus, for {@link indexColumnDeclarations}. */ +/** + * Identity of a column across the corpus, for {@link indexColumnDeclarations}. + * + * `public` and no schema at all are the SAME table: Postgres resolves an + * unqualified name through `search_path`, which starts at `public`. The two + * spellings coexist in one corpus routinely — drizzle-kit emits unqualified, + * while hand-written SQL and this sweep's own {@link renderSafeAlter} output + * are qualified. Keying on the literal text split one column across two keys, + * so a column already encrypted under one spelling looked plaintext when + * altered under the other and the ADD+DROP+RENAME dropped its ciphertext + * (#772 review, finding 4). + * + * Only the IMPLICIT schema collapses. `"app"."users"` is a genuinely different + * table from `"users"` and keeps its own key. No case folding either: + * `TABLE_REF` matches quoted identifiers only, and those are case-sensitive in + * Postgres — `"PUBLIC"` really is a different schema from `"public"`. + */ function columnKey(table: string, column: string, schema?: string): string { - return JSON.stringify([schema ?? '', table, column]) + return JSON.stringify([ + schema === 'public' ? '' : (schema ?? ''), + table, + column, + ]) } /** What the migration corpus says about the columns it mentions. */ @@ -665,6 +685,24 @@ export async function rewriteEncryptedAlterColumns( // 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) }, ) From 333edf2f1232045a8481cd4f6cb8585822951489 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:05:59 +1000 Subject: [PATCH 5/8] fix(cli,wizard): track dollar-quoted bodies and E'' escapes when scanning for comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quote parity is a whole-file property. Anything that makes the scanner disagree with Postgres about where a literal ENDS shifts every token after it — so a commented-out ALTER downstream reads as live and is rewritten into a live DROP COLUMN. `isInsideCommentOrString` left two such gaps. A `$$ … $$` body was untracked on the stated grounds that mis-reading one can only make us SKIP a rewrite; that holds for an unterminated literal but not for an over-terminated one, and an odd apostrophe count inside the body (`$$ don't $$` — a function body, a comment, any prose) ends a literal EARLY. And `endOfQuoted` knew only the doubled-delimiter escape, so `E'a\'b'` read as closing at the escaped quote. Skip dollar-quoted bodies whole, and give `endOfQuoted` a backslashEscapes mode selected when the opening quote is an `E''` prefix (guarded so an identifier merely ending in `e` does not trigger it). Both copies. Four tests each. The two destructive shapes, a tagged `$fn$` variant, and a guard that a live ALTER *following* a dollar-quoted body is still rewritten — that one failed before this change too, in the over-skip direction the old docblock had assumed was the only risk. --- .../src/__tests__/rewrite-migrations.test.ts | 92 +++++++++++++++++++ .../cli/src/commands/db/rewrite-migrations.ts | 73 +++++++++++++-- .../src/__tests__/rewrite-migrations.test.ts | 92 +++++++++++++++++++ packages/wizard/src/lib/rewrite-migrations.ts | 73 +++++++++++++-- 4 files changed, 318 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 8f999f85..c8090b21 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -819,6 +819,98 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped).toEqual([]) expect(fs.readFileSync(filePath, 'utf-8')).toBe(original) }) + + // #772 review, finding 1. Quote parity is a whole-file property: anything + // that makes the scanner disagree with Postgres about where a literal ENDS + // shifts every following token, so a commented-out ALTER downstream reads + // as live and is rewritten into a live DROP COLUMN. + // + // The two shapes below both do that. A `$$ … $$` body was documented as + // safe on the grounds that mis-reading one can only make us SKIP — that + // holds for an unterminated literal, but an odd apostrophe count inside the + // body ends a literal EARLY, which is the opposite direction. + it('leaves a commented-out ALTER alone after a dollar-quoted body with an odd apostrophe count', async () => { + declarePlaintext('"users"', 'email') + const original = [ + "CREATE FUNCTION note() RETURNS text AS $$ don't $$ LANGUAGE sql;", + '--> statement-breakpoint', + '-- it\'s ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n') + const filePath = path.join(tmpDir, '0033_dollar-quoted.sql') + fs.writeFileSync(filePath, original) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) + const updated = fs.readFileSync(filePath, 'utf-8') + expect(updated).toBe(original) + expect(updated).not.toContain('DROP COLUMN') + }) + + it('leaves a commented-out ALTER alone after a tagged dollar-quoted body', async () => { + declarePlaintext('"users"', 'email') + const original = [ + "CREATE FUNCTION note() RETURNS text AS $fn$ don't $fn$ LANGUAGE sql;", + '--> statement-breakpoint', + '-- it\'s ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n') + const filePath = path.join(tmpDir, '0033_tagged-dollar.sql') + fs.writeFileSync(filePath, original) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) + expect(fs.readFileSync(filePath, 'utf-8')).toBe(original) + }) + + // A backslash-escaped quote inside an E'' string does not close it. Reading + // it as a close shifts parity for the rest of the file the same way. + it('leaves a commented-out ALTER alone after an E-string with a backslash-escaped quote', async () => { + declarePlaintext('"users"', 'email') + const original = [ + 'CREATE TABLE "notes" ("body" text);', + '--> statement-breakpoint', + "INSERT INTO \"notes\" VALUES (E'a\\'b');", + '--> statement-breakpoint', + '-- it\'s ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n') + const filePath = path.join(tmpDir, '0033_estring.sql') + fs.writeFileSync(filePath, original) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) + const updated = fs.readFileSync(filePath, 'utf-8') + expect(updated).toBe(original) + expect(updated).not.toContain('DROP COLUMN') + }) + + // The dollar-quote skip must not swallow live SQL: a `$$` body sitting + // BEFORE a genuine plaintext ALTER still leaves that ALTER rewritable. + it('still rewrites a live ALTER that follows a dollar-quoted body', async () => { + declarePlaintext('"users"', 'email') + const filePath = path.join(tmpDir, '0033_dollar-then-live.sql') + fs.writeFileSync( + filePath, + [ + "CREATE FUNCTION note() RETURNS text AS $$ don't $$ LANGUAGE sql;", + '--> statement-breakpoint', + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n'), + ) + + const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([filePath]) + expect(fs.readFileSync(filePath, 'utf-8')).toContain('DROP COLUMN') + }) }) // ADD+DROP+RENAME on a column that is ALREADY encrypted drops CIPHERTEXT, and diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 2d027f0d..dae19ab8 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -176,9 +176,15 @@ function trimStatementPreamble(statement: string): string { * it into a real `DROP COLUMN`. A doubled delimiter (`''` in a literal, `""` in * an identifier) is an escape and does not close the token. * - * Dollar-quoted bodies are NOT tracked: a `--` or `'` inside one reads as a - * comment/literal here, which can only make us skip a rewrite (the statement - * then fails loudly at migrate time), never perform a destructive one. + * Dollar-quoted bodies (`$$ … $$`, `$fn$ … $fn$`) are skipped whole, and an + * `E''` literal gives backslash its escape meaning. Both are quote-PARITY + * concerns, not merely "we might skip a rewrite": an apostrophe inside an + * untracked `$$` body, or a `\'` read as a close, ends a literal EARLY, and + * every token after it is then misread — including a commented-out ALTER, which + * reads as live and gets rewritten into a live DROP COLUMN. That is the same + * failure mode as the apostrophe-in-identifier case below, and it is why the + * earlier reasoning here ("can only make us skip") was wrong: it holds for an + * UNDER-terminated literal, not an over-terminated one (#772 review, finding 1). */ function isInsideCommentOrString(sql: string, index: number): boolean { let i = 0 @@ -206,6 +212,20 @@ function isInsideCommentOrString(sql: string, index: number): boolean { } if (j > index) return true i = j + } else if (sql[i] === '$') { + // A `$tag$ … $tag$` body is opaque: its content is not SQL, so a `'` or + // `--` inside it means nothing. Skipping the whole body is what keeps the + // rest of the file's quote parity aligned with Postgres. + const delimiter = dollarQuoteDelimiter(sql, i) + if (delimiter === undefined) { + i += 1 + } else { + const close = sql.indexOf(delimiter, i + delimiter.length) + // Unterminated: inert to the end of the file, like the cases below. + const end = close === -1 ? sql.length : close + delimiter.length + if (end > index) return true + i = end + } } else if (sql[i] === '"') { // A quoted identifier is live SQL, but its body is not: consuming it here // — before the `'` branch below — is what stops an apostrophe inside one @@ -219,7 +239,7 @@ function isInsideCommentOrString(sql: string, index: number): boolean { if (end > index) return true i = end } else if (sql[i] === "'") { - const end = endOfQuoted(sql, i, "'") + const end = endOfQuoted(sql, i, "'", isEscapeStringOpener(sql, i)) // Unterminated, or the literal runs past `index`: `index` is inside a // string literal, which is every bit as inert as a comment. if (end > index) return true @@ -231,15 +251,56 @@ function isInsideCommentOrString(sql: string, index: number): boolean { return false } +/** + * The `$tag$` delimiter opening at `open`, or `undefined` when what sits there + * is just a `$`. The tag is empty (`$$`) or an SQL identifier (`$fn$`); a digit + * cannot start one, so a positional parameter (`$1`) is never mistaken for a + * dollar-quote opener. + */ +function dollarQuoteDelimiter(sql: string, open: number): string | undefined { + DOLLAR_QUOTE_OPEN_RE.lastIndex = open + return DOLLAR_QUOTE_OPEN_RE.exec(sql)?.[0] +} + +/** Sticky, so the scan never has to slice the file to test one position. */ +const DOLLAR_QUOTE_OPEN_RE = /\$(?:[A-Za-z_][A-Za-z0-9_]*)?\$/y + +/** + * Whether the `'` at `open` starts an `E''` escape-string, in which a backslash + * escapes the following character. Plain literals give backslash no special + * meaning under the default `standard_conforming_strings = on`. + * + * The character before the `E` must not be part of an identifier, so a column + * or type name that merely ends in `e` does not turn the literal after it into + * an escape-string. + */ +function isEscapeStringOpener(sql: string, open: number): boolean { + const prefix = sql[open - 1] + if (prefix !== 'E' && prefix !== 'e') return false + return !/[A-Za-z0-9_]/.test(sql[open - 2] ?? '') +} + /** * The index just past the `quote`-delimited token that opens at `open`, or * `sql.length` when it is never closed. A doubled delimiter inside the token is * an escaped one (`''`, `""`) and does not end it. + * + * With `backslashEscapes`, a `\` also escapes the next character — the `E''` + * form. Getting this wrong ends the literal EARLY, which shifts quote parity + * for the whole rest of the file and can make a commented-out ALTER downstream + * read as live (#772 review, finding 1). */ -function endOfQuoted(sql: string, open: number, quote: "'" | '"'): number { +function endOfQuoted( + sql: string, + open: number, + quote: "'" | '"', + backslashEscapes = false, +): number { let i = open + 1 while (i < sql.length) { - if (sql[i] !== quote) { + if (backslashEscapes && sql[i] === '\\') { + i += 2 + } else if (sql[i] !== quote) { i += 1 } else if (sql[i + 1] === quote) { i += 2 diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index 3a1b189c..4d9bfea8 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -747,6 +747,98 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped).toEqual([]) expect(fs.readFileSync(filePath, 'utf-8')).toBe(original) }) + + // #772 review, finding 1. Quote parity is a whole-file property: anything + // that makes the scanner disagree with Postgres about where a literal ENDS + // shifts every following token, so a commented-out ALTER downstream reads + // as live and is rewritten into a live DROP COLUMN. + // + // The two shapes below both do that. A `$$ … $$` body was documented as + // safe on the grounds that mis-reading one can only make us SKIP — that + // holds for an unterminated literal, but an odd apostrophe count inside the + // body ends a literal EARLY, which is the opposite direction. + it('leaves a commented-out ALTER alone after a dollar-quoted body with an odd apostrophe count', async () => { + declarePlaintext('"users"', 'email') + const original = [ + "CREATE FUNCTION note() RETURNS text AS $$ don't $$ LANGUAGE sql;", + '--> statement-breakpoint', + '-- it\'s ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n') + const filePath = path.join(tmpDir, '0033_dollar-quoted.sql') + fs.writeFileSync(filePath, original) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) + const updated = fs.readFileSync(filePath, 'utf-8') + expect(updated).toBe(original) + expect(updated).not.toContain('DROP COLUMN') + }) + + it('leaves a commented-out ALTER alone after a tagged dollar-quoted body', async () => { + declarePlaintext('"users"', 'email') + const original = [ + "CREATE FUNCTION note() RETURNS text AS $fn$ don't $fn$ LANGUAGE sql;", + '--> statement-breakpoint', + '-- it\'s ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n') + const filePath = path.join(tmpDir, '0033_tagged-dollar.sql') + fs.writeFileSync(filePath, original) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) + expect(fs.readFileSync(filePath, 'utf-8')).toBe(original) + }) + + // A backslash-escaped quote inside an E'' string does not close it. Reading + // it as a close shifts parity for the rest of the file the same way. + it('leaves a commented-out ALTER alone after an E-string with a backslash-escaped quote', async () => { + declarePlaintext('"users"', 'email') + const original = [ + 'CREATE TABLE "notes" ("body" text);', + '--> statement-breakpoint', + "INSERT INTO \"notes\" VALUES (E'a\\'b');", + '--> statement-breakpoint', + '-- it\'s ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n') + const filePath = path.join(tmpDir, '0033_estring.sql') + fs.writeFileSync(filePath, original) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) + const updated = fs.readFileSync(filePath, 'utf-8') + expect(updated).toBe(original) + expect(updated).not.toContain('DROP COLUMN') + }) + + // The dollar-quote skip must not swallow live SQL: a `$$` body sitting + // BEFORE a genuine plaintext ALTER still leaves that ALTER rewritable. + it('still rewrites a live ALTER that follows a dollar-quoted body', async () => { + declarePlaintext('"users"', 'email') + const filePath = path.join(tmpDir, '0033_dollar-then-live.sql') + fs.writeFileSync( + filePath, + [ + "CREATE FUNCTION note() RETURNS text AS $$ don't $$ LANGUAGE sql;", + '--> statement-breakpoint', + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n'), + ) + + const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([filePath]) + expect(fs.readFileSync(filePath, 'utf-8')).toContain('DROP COLUMN') + }) }) // ADD+DROP+RENAME on a column that is ALREADY encrypted drops CIPHERTEXT, and diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 6cea3886..e0cec019 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -184,9 +184,15 @@ function trimStatementPreamble(statement: string): string { * it into a real `DROP COLUMN`. A doubled delimiter (`''` in a literal, `""` in * an identifier) is an escape and does not close the token. * - * Dollar-quoted bodies are NOT tracked: a `--` or `'` inside one reads as a - * comment/literal here, which can only make us skip a rewrite (the statement - * then fails loudly at migrate time), never perform a destructive one. + * Dollar-quoted bodies (`$$ … $$`, `$fn$ … $fn$`) are skipped whole, and an + * `E''` literal gives backslash its escape meaning. Both are quote-PARITY + * concerns, not merely "we might skip a rewrite": an apostrophe inside an + * untracked `$$` body, or a `\'` read as a close, ends a literal EARLY, and + * every token after it is then misread — including a commented-out ALTER, which + * reads as live and gets rewritten into a live DROP COLUMN. That is the same + * failure mode as the apostrophe-in-identifier case below, and it is why the + * earlier reasoning here ("can only make us skip") was wrong: it holds for an + * UNDER-terminated literal, not an over-terminated one (#772 review, finding 1). */ function isInsideCommentOrString(sql: string, index: number): boolean { let i = 0 @@ -214,6 +220,20 @@ function isInsideCommentOrString(sql: string, index: number): boolean { } if (j > index) return true i = j + } else if (sql[i] === '$') { + // A `$tag$ … $tag$` body is opaque: its content is not SQL, so a `'` or + // `--` inside it means nothing. Skipping the whole body is what keeps the + // rest of the file's quote parity aligned with Postgres. + const delimiter = dollarQuoteDelimiter(sql, i) + if (delimiter === undefined) { + i += 1 + } else { + const close = sql.indexOf(delimiter, i + delimiter.length) + // Unterminated: inert to the end of the file, like the cases below. + const end = close === -1 ? sql.length : close + delimiter.length + if (end > index) return true + i = end + } } else if (sql[i] === '"') { // A quoted identifier is live SQL, but its body is not: consuming it here // — before the `'` branch below — is what stops an apostrophe inside one @@ -227,7 +247,7 @@ function isInsideCommentOrString(sql: string, index: number): boolean { if (end > index) return true i = end } else if (sql[i] === "'") { - const end = endOfQuoted(sql, i, "'") + const end = endOfQuoted(sql, i, "'", isEscapeStringOpener(sql, i)) // Unterminated, or the literal runs past `index`: `index` is inside a // string literal, which is every bit as inert as a comment. if (end > index) return true @@ -239,15 +259,56 @@ function isInsideCommentOrString(sql: string, index: number): boolean { return false } +/** + * The `$tag$` delimiter opening at `open`, or `undefined` when what sits there + * is just a `$`. The tag is empty (`$$`) or an SQL identifier (`$fn$`); a digit + * cannot start one, so a positional parameter (`$1`) is never mistaken for a + * dollar-quote opener. + */ +function dollarQuoteDelimiter(sql: string, open: number): string | undefined { + DOLLAR_QUOTE_OPEN_RE.lastIndex = open + return DOLLAR_QUOTE_OPEN_RE.exec(sql)?.[0] +} + +/** Sticky, so the scan never has to slice the file to test one position. */ +const DOLLAR_QUOTE_OPEN_RE = /\$(?:[A-Za-z_][A-Za-z0-9_]*)?\$/y + +/** + * Whether the `'` at `open` starts an `E''` escape-string, in which a backslash + * escapes the following character. Plain literals give backslash no special + * meaning under the default `standard_conforming_strings = on`. + * + * The character before the `E` must not be part of an identifier, so a column + * or type name that merely ends in `e` does not turn the literal after it into + * an escape-string. + */ +function isEscapeStringOpener(sql: string, open: number): boolean { + const prefix = sql[open - 1] + if (prefix !== 'E' && prefix !== 'e') return false + return !/[A-Za-z0-9_]/.test(sql[open - 2] ?? '') +} + /** * The index just past the `quote`-delimited token that opens at `open`, or * `sql.length` when it is never closed. A doubled delimiter inside the token is * an escaped one (`''`, `""`) and does not end it. + * + * With `backslashEscapes`, a `\` also escapes the next character — the `E''` + * form. Getting this wrong ends the literal EARLY, which shifts quote parity + * for the whole rest of the file and can make a commented-out ALTER downstream + * read as live (#772 review, finding 1). */ -function endOfQuoted(sql: string, open: number, quote: "'" | '"'): number { +function endOfQuoted( + sql: string, + open: number, + quote: "'" | '"', + backslashEscapes = false, +): number { let i = open + 1 while (i < sql.length) { - if (sql[i] !== quote) { + if (backslashEscapes && sql[i] === '\\') { + i += 2 + } else if (sql[i] !== quote) { i += 1 } else if (sql[i + 1] === quote) { i += 2 From 03579b2281ef1bdb2a42d25c49d5aa94e85268ce Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:08:41 +1000 Subject: [PATCH 6/8] fix(cli,wizard): find the CREATE TABLE body terminator instead of guessing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CREATE_TABLE_RE carried a lazy `([\s\S]*?)\)\s*;` body, so it stopped at the first `);` anywhere in the file. That can sit inside a `--` comment or a string DEFAULT — `"id" text, -- pk (uuid);` is legal and unremarkable — and the body was then truncated at that point. Every column declared after it disappeared from BOTH indexes. For an encrypted column that means the already-encrypted guard never sees it. On its own that yields a wrong reason (`source-unknown` for a column declared two lines up). Composed with a `declared` record from elsewhere in the corpus — a table dropped and recreated in a later migration — the fail-closed rule is satisfied too, and the ADD+DROP+RENAME drops a column full of ciphertext. Match only `CREATE TABLE … (` and locate the closing `)` by scanning candidate `)\s*;` positions for the first one `isInsideCommentOrString` says is live. Nested parens (`numeric(10,2)`, `CHECK (x > 0)`) are not followed by `;`, so requiring the terminator still excludes them. An unclosed statement is skipped rather than indexed to end-of-file. Three tests each: the comment truncation, the DEFAULT-string truncation, and the composed case that actually destroys data. --- .../src/__tests__/rewrite-migrations.test.ts | 87 +++++++++++++++++++ .../cli/src/commands/db/rewrite-migrations.ts | 50 +++++++++-- .../src/__tests__/rewrite-migrations.test.ts | 87 +++++++++++++++++++ packages/wizard/src/lib/rewrite-migrations.ts | 50 +++++++++-- 4 files changed, 264 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index c8090b21..4a0d231b 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -1358,6 +1358,93 @@ describe('rewriteEncryptedAlterColumns', () => { ]) }) + // #772 review, finding 2. CREATE_TABLE_RE's body is lazy up to the first + // `)\s*;`, which can sit inside a `--` comment or a string DEFAULT. The + // body is then truncated and every column declared after that point is + // lost from BOTH indexes — so an encrypted column reads as undeclared. + it('indexes columns declared after a ");" inside a comment in the CREATE TABLE body', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" text, -- pk (uuid);', + '\t"email" "public"."eql_v3_text_eq"', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_domain-change.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + + it('indexes columns declared after a ");" inside a string DEFAULT', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"note" text DEFAULT \'see (ticket);\',', + '\t"email" "public"."eql_v3_text_eq"', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_domain-change.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + + // The destructive composition: the truncated body loses the encrypted + // column, but a LATER migration re-declares the table so `declared` is + // satisfied from elsewhere — the fail-closed rule passes and the ALTER + // drops a column full of ciphertext. + it('does not drop ciphertext when a truncated CREATE TABLE body hides the encrypted column', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + fs.writeFileSync( + path.join(tmpDir, '0001_recreate.sql'), + [ + 'DROP TABLE "users";', + '--> statement-breakpoint', + 'CREATE TABLE "users" (', + '\t"id" text, -- pk (uuid);', + '\t"email" "public"."eql_v3_text_eq"', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0002_domain-change.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(fs.readFileSync(alter, 'utf-8')).toBe(`${alterSql}\n`) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + // Only the IMPLICIT schema collapses. A real non-public schema is a // genuinely different table and must not be conflated with the bare name. it('does not conflate a non-public schema with the unqualified table', async () => { diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index dae19ab8..3852853f 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -341,12 +341,47 @@ const RENAME_COLUMN_RE = new RegExp( 'gi', ) -/** `CREATE TABLE … ( … );` — $1/$2 table, $3 the column-definition body. */ -const CREATE_TABLE_RE = new RegExp( - String.raw`CREATE TABLE\s+(?:IF NOT EXISTS\s+)?${TABLE_REF}\s*\(([\s\S]*?)\)\s*;`, +/** + * `CREATE TABLE … (` — $1/$2 table. The body's END is found by + * {@link endOfCreateTableBody} rather than by the matcher. + * + * This used to carry a lazy `([\s\S]*?)\)\s*;` body, which stopped at the FIRST + * `);` in the file — and that can sit inside a `--` comment or a string DEFAULT + * (`"id" text, -- pk (uuid);`). The body was then truncated and every column + * declared after that point vanished from both indexes, including an ENCRYPTED + * one — which is how a later domain change on a ciphertext column got past the + * already-encrypted guard (#772 review, finding 2). + */ +const CREATE_TABLE_HEAD_RE = new RegExp( + String.raw`CREATE TABLE\s+(?:IF NOT EXISTS\s+)?${TABLE_REF}\s*\(`, 'gi', ) +/** Candidate body terminators, filtered by {@link isInsideCommentOrString}. */ +const CREATE_TABLE_BODY_END_RE = /\)\s*;/g + +/** + * The offset of the `)` closing the CREATE TABLE body that opens at + * `bodyStart`, or `undefined` when the statement is never closed. + * + * Parens nested inside the body (`numeric(10,2)`, `CHECK (x > 0)`) are not + * followed by `;`, so requiring the terminator keeps them from matching. + */ +function endOfCreateTableBody( + sql: string, + bodyStart: number, +): number | undefined { + CREATE_TABLE_BODY_END_RE.lastIndex = bodyStart + for ( + let end = CREATE_TABLE_BODY_END_RE.exec(sql); + end !== null; + end = CREATE_TABLE_BODY_END_RE.exec(sql) + ) { + if (!isInsideCommentOrString(sql, end.index)) return end.index + } + return undefined +} + /** `"col" ` inside a CREATE TABLE body — $1 column. */ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( String.raw`"([^"]+)"\s+${ENCRYPTED_TYPE_REF}`, @@ -519,10 +554,15 @@ function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { const declared = new Set() for (const sql of contents) { - for (const created of sql.matchAll(CREATE_TABLE_RE)) { + for (const created of sql.matchAll(CREATE_TABLE_HEAD_RE)) { if (isInsideCommentOrString(sql, created.index)) continue const { schema, table } = tableOf(created[1], created[2]) - const body = created[3] + const bodyStart = created.index + created[0].length + const bodyEnd = endOfCreateTableBody(sql, bodyStart) + // Never closed — treat the statement as absent rather than index a body + // that runs to the end of the file. + if (bodyEnd === undefined) continue + const body = sql.slice(bodyStart, bodyEnd) // The body is scanned as its own document: a `--` earlier in it comments // out the rest of that line, and a CREATE inside a block comment or a diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index 4d9bfea8..5ebfbf36 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -1286,6 +1286,93 @@ describe('rewriteEncryptedAlterColumns', () => { ]) }) + // #772 review, finding 2. CREATE_TABLE_RE's body is lazy up to the first + // `)\s*;`, which can sit inside a `--` comment or a string DEFAULT. The + // body is then truncated and every column declared after that point is + // lost from BOTH indexes — so an encrypted column reads as undeclared. + it('indexes columns declared after a ");" inside a comment in the CREATE TABLE body', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" text, -- pk (uuid);', + '\t"email" "public"."eql_v3_text_eq"', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_domain-change.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + + it('indexes columns declared after a ");" inside a string DEFAULT', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"note" text DEFAULT \'see (ticket);\',', + '\t"email" "public"."eql_v3_text_eq"', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_domain-change.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + + // The destructive composition: the truncated body loses the encrypted + // column, but a LATER migration re-declares the table so `declared` is + // satisfied from elsewhere — the fail-closed rule passes and the ALTER + // drops a column full of ciphertext. + it('does not drop ciphertext when a truncated CREATE TABLE body hides the encrypted column', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + fs.writeFileSync( + path.join(tmpDir, '0001_recreate.sql'), + [ + 'DROP TABLE "users";', + '--> statement-breakpoint', + 'CREATE TABLE "users" (', + '\t"id" text, -- pk (uuid);', + '\t"email" "public"."eql_v3_text_eq"', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0002_domain-change.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(fs.readFileSync(alter, 'utf-8')).toBe(`${alterSql}\n`) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + // Only the IMPLICIT schema collapses. A real non-public schema is a // genuinely different table and must not be conflated with the bare name. it('does not conflate a non-public schema with the unqualified table', async () => { diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index e0cec019..1f1d3118 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -349,12 +349,47 @@ const RENAME_COLUMN_RE = new RegExp( 'gi', ) -/** `CREATE TABLE … ( … );` — $1/$2 table, $3 the column-definition body. */ -const CREATE_TABLE_RE = new RegExp( - String.raw`CREATE TABLE\s+(?:IF NOT EXISTS\s+)?${TABLE_REF}\s*\(([\s\S]*?)\)\s*;`, +/** + * `CREATE TABLE … (` — $1/$2 table. The body's END is found by + * {@link endOfCreateTableBody} rather than by the matcher. + * + * This used to carry a lazy `([\s\S]*?)\)\s*;` body, which stopped at the FIRST + * `);` in the file — and that can sit inside a `--` comment or a string DEFAULT + * (`"id" text, -- pk (uuid);`). The body was then truncated and every column + * declared after that point vanished from both indexes, including an ENCRYPTED + * one — which is how a later domain change on a ciphertext column got past the + * already-encrypted guard (#772 review, finding 2). + */ +const CREATE_TABLE_HEAD_RE = new RegExp( + String.raw`CREATE TABLE\s+(?:IF NOT EXISTS\s+)?${TABLE_REF}\s*\(`, 'gi', ) +/** Candidate body terminators, filtered by {@link isInsideCommentOrString}. */ +const CREATE_TABLE_BODY_END_RE = /\)\s*;/g + +/** + * The offset of the `)` closing the CREATE TABLE body that opens at + * `bodyStart`, or `undefined` when the statement is never closed. + * + * Parens nested inside the body (`numeric(10,2)`, `CHECK (x > 0)`) are not + * followed by `;`, so requiring the terminator keeps them from matching. + */ +function endOfCreateTableBody( + sql: string, + bodyStart: number, +): number | undefined { + CREATE_TABLE_BODY_END_RE.lastIndex = bodyStart + for ( + let end = CREATE_TABLE_BODY_END_RE.exec(sql); + end !== null; + end = CREATE_TABLE_BODY_END_RE.exec(sql) + ) { + if (!isInsideCommentOrString(sql, end.index)) return end.index + } + return undefined +} + /** `"col" ` inside a CREATE TABLE body — $1 column. */ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( String.raw`"([^"]+)"\s+${ENCRYPTED_TYPE_REF}`, @@ -527,10 +562,15 @@ function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { const declared = new Set() for (const sql of contents) { - for (const created of sql.matchAll(CREATE_TABLE_RE)) { + for (const created of sql.matchAll(CREATE_TABLE_HEAD_RE)) { if (isInsideCommentOrString(sql, created.index)) continue const { schema, table } = tableOf(created[1], created[2]) - const body = created[3] + const bodyStart = created.index + created[0].length + const bodyEnd = endOfCreateTableBody(sql, bodyStart) + // Never closed — treat the statement as absent rather than index a body + // that runs to the end of the file. + if (bodyEnd === undefined) continue + const body = sql.slice(bodyStart, bodyEnd) // The body is scanned as its own document: a `--` earlier in it comments // out the rest of that line, and a CREATE inside a block comment or a From c12e3555dc6173a785c2de2a1b66e3e773f06022 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:11:35 +1000 Subject: [PATCH 7/8] test(scripts): compare the two rewriter copies so a one-sided fix fails CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rewriter lives twice — packages/wizard/src/lib and packages/cli/src/commands/db — at ~99% similarity, and neither package depends on the other. Four #772 review findings each needed the identical fix in both places; a fix landing in one still passes that package's own suite, and this code emits DROP COLUMN. Extraction is not available this cycle: packages/utils has no package.json, @cipherstash/test-kit is private and build-less, and both bin entries set skipNodeModulesBundle, so a shared workspace package would survive into dist as an unresolvable bare specifier. That leaves publishing a new package into a fixed mid-RC release group or adding noExternal to two CLI bundles — both worse than the drift they would prevent, for now. So compare instead. Everything outside an explicit `#region wizard-only` marker must match byte-for-byte modulo comments, imports and the tool name in the emitted header. Verified the guard actually fires: perturbing either copy fails with a message naming both files and pointing at the region marker. --- .changeset/rewriter-never-drops-ciphertext.md | 28 +++++ packages/wizard/src/lib/rewrite-migrations.ts | 6 + .../rewriter-copies-in-sync.test.mjs | 111 ++++++++++++++++++ 3 files changed, 145 insertions(+) create mode 100644 scripts/__tests__/rewriter-copies-in-sync.test.mjs diff --git a/.changeset/rewriter-never-drops-ciphertext.md b/.changeset/rewriter-never-drops-ciphertext.md index ec82f4b6..0815b61d 100644 --- a/.changeset/rewriter-never-drops-ciphertext.md +++ b/.changeset/rewriter-never-drops-ciphertext.md @@ -52,3 +52,31 @@ treated as empty, and the wizard's `Run the migration now?` prompt defaults to N whenever the sweep rewrote anything, flagged anything, or could not check a directory at all — naming the directories that went unchecked, and making no claim about data destruction for a directory nothing is known about. + +Four further ways the sweep could still reach a ciphertext column are closed: + +- **Chained conversions.** The corpus index read `CREATE TABLE`, `ADD COLUMN` + and `RENAME`, but never the sweep's own target. A directory where an earlier + migration already ran `SET DATA TYPE eql_v2_encrypted` — generated by a stack + version predating this sweep — left the column looking plaintext, so a later + domain change dropped its ciphertext. Conversions are now recorded as the + sweep walks the corpus in order, so the first conversion still applies and + only the ones after it are flagged. +- **Schema qualification.** `"users"` and `"public"."users"` are the same table + — Postgres resolves the unqualified name through `search_path` — but they were + indexed under different keys. drizzle-kit emits unqualified while hand-written + SQL and this sweep's own output are qualified, so a corpus mixing the two hid + an encrypted column from the guard. A non-`public` schema stays distinct. +- **Quote parity.** A `$$ … $$` body containing an odd number of apostrophes, or + an `E'a\'b'` literal, ended a string literal earlier than Postgres does. Every + token after it was then misread — including a commented-out `ALTER`, which + read as live and was rewritten into a live `DROP COLUMN`. Dollar-quoted bodies + are skipped whole and `E''` backslash escapes are honoured. +- **Truncated `CREATE TABLE` bodies.** The body was matched up to the first + `);`, which can sit inside a `--` comment or a string `DEFAULT`. Columns + declared after that point vanished from the index entirely. The closing paren + is now located by skipping candidates that are inside a comment or literal. + +The two copies of this rewriter — one in `stash`, one in `@cipherstash/wizard` — +are now compared by a repo test, so a fix can no longer land in one and silently +miss the other. diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 1f1d3118..93ea0e36 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -834,6 +834,11 @@ export async function rewriteEncryptedAlterColumns( return { rewritten, skipped } } +// #region wizard-only — deliberately has no counterpart in +// packages/cli/src/commands/db/rewrite-migrations.ts. Everything OUTSIDE this +// region is mirrored there byte-for-byte and is checked by +// scripts/__tests__/rewriter-copies-in-sync.test.mjs. Only the wizard sweeps +// several candidate directories; both CLI call sites pass one explicit --out. /** One candidate migration directory's outcome from {@link sweepMigrationDirs}. */ export interface DirRewriteResult extends RewriteResult { /** The candidate directory as supplied (relative to `cwd`), for reporting. */ @@ -884,6 +889,7 @@ export async function sweepMigrationDirs( return results } +// #endregion wizard-only /** * The rewrite is identical for v2 and v3, and the ADD+DROP+RENAME sequence is diff --git a/scripts/__tests__/rewriter-copies-in-sync.test.mjs b/scripts/__tests__/rewriter-copies-in-sync.test.mjs new file mode 100644 index 00000000..213b1ffc --- /dev/null +++ b/scripts/__tests__/rewriter-copies-in-sync.test.mjs @@ -0,0 +1,111 @@ +import { readFileSync } from 'node:fs' +import { resolve } from 'node:path' +import { fileURLToPath } from 'node:url' +import { describe, expect, it } from 'vitest' + +const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') + +/** + * The destructive ALTER-COLUMN rewriter exists twice: `@cipherstash/wizard` + * runs it after its agent edits a schema, and `stash eql migration --drizzle` + * runs it over an explicit `--out`. Both are published, neither depends on the + * other, and `packages/utils` is not a package while `@cipherstash/test-kit` is + * private and build-less — so there is nowhere to put shared runtime code that + * both npm tarballs could resolve. Extracting one means either publishing a new + * package into a fixed release group or adding `noExternal` to two CLI bundles. + * + * Until that happens the two files are near-clones, and drift between them is + * the failure mode: four separate #772 review findings each needed the same fix + * applied in both places, and a fix landing in only one still passes that + * package's suite. Nothing else in CI compares them. + * + * So: everything outside the wizard's `#region wizard-only` must match the CLI + * copy exactly, modulo comments and the tool name in the emitted header. + */ +const WIZARD = 'packages/wizard/src/lib/rewrite-migrations.ts' +const CLI = 'packages/cli/src/commands/db/rewrite-migrations.ts' + +const REGION_OPEN = '// #region wizard-only' +const REGION_CLOSE = '// #endregion wizard-only' + +/** Drop the wizard-only region, delimited by the markers in the source. */ +function stripWizardOnly(source) { + const open = source.indexOf(REGION_OPEN) + if (open === -1) return source + const close = source.indexOf(REGION_CLOSE, open) + if (close === -1) return source + return source.slice(0, open) + source.slice(close + REGION_CLOSE.length) +} + +/** + * Comments, imports and blank lines removed; the emitted header's tool name + * canonicalised. Both files are Biome-formatted identically, so a pure comment + * line is reliably one starting with `//`, `/*` or `*` — no line of code in + * either file starts that way (the regex literals begin `/[`). + */ +function comparableCode(source) { + return source + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.length > 0) + .filter( + (line) => + !line.startsWith('//') && + !line.startsWith('*') && + !line.startsWith('/*'), + ) + .filter((line) => !line.startsWith('import ')) + .map((line) => + line.replace(/'-- Rewritten by [^:]+:/, "'-- Rewritten by :"), + ) +} + +const read = (file) => readFileSync(resolve(REPO_ROOT, file), 'utf8') + +describe('the wizard and cli rewriter copies stay in sync', () => { + const wizardSource = read(WIZARD) + const cliSource = read(CLI) + + it('marks the wizard-only region so the comparison knows what to exclude', () => { + expect(wizardSource).toContain(REGION_OPEN) + expect(wizardSource).toContain(REGION_CLOSE) + // The CLI copy has no counterpart, so it must not carry the markers. + expect(cliSource).not.toContain(REGION_OPEN) + }) + + it('finds real code in both (guards against a normaliser that strips everything)', () => { + expect( + comparableCode(stripWizardOnly(wizardSource)).length, + ).toBeGreaterThan(200) + expect(comparableCode(cliSource).length).toBeGreaterThan(200) + }) + + it('has identical shared logic', () => { + const wizard = comparableCode(stripWizardOnly(wizardSource)) + const cli = comparableCode(cliSource) + expect( + wizard.join('\n'), + `${WIZARD} and ${CLI} have drifted. Every fix to this rewriter must land ` + + "in BOTH copies — a one-sided fix still passes that package's own suite, " + + 'and this rewriter emits DROP COLUMN. If the difference is intentional and ' + + `wizard-only, move it inside the ${REGION_OPEN} region.`, + ).toBe(cli.join('\n')) + }) + + it('keeps sweepMigrationDirs as the only wizard-only export', () => { + // A second wizard-only export would mean shared logic had started to + // diverge under cover of the region marker. + const region = wizardSource.slice( + wizardSource.indexOf(REGION_OPEN), + wizardSource.indexOf(REGION_CLOSE), + ) + const exported = [ + ...region.matchAll( + /^export (?:async function|interface|const|type) (\w+)/gm, + ), + ] + .map((match) => match[1]) + .sort() + expect(exported).toEqual(['DirRewriteResult', 'sweepMigrationDirs']) + }) +}) From 9c96ff125835b3e8c7bbba40cf3c95dbe28f6ee5 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:16:23 +1000 Subject: [PATCH 8/8] fix(wizard): sweep only drizzle-kit output directories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wizard cannot discover a project's configured drizzle-kit `out`, so it tries drizzle/, migrations/ and src/db/migrations/. Two of those are generic names — Knex, node-pg-migrate, Flyway and hand-rolled psql all use them — and this sweep emits DROP COLUMN. So a project whose out is drizzle/ but which also keeps a hand-maintained migrations/ had that second directory rewritten into ADD+DROP+RENAME, in a directory the wizard was never pointed at. #783's fail-closed rule does not help: it indexes per directory, and a real migration history declares its own columns, so the guard passes and the rewrite proceeds. Worse, the emitted `--> statement-breakpoint` is a valid SQL line comment, so the mangled file stays runnable by whichever tool actually owns it. Gate on meta/_journal.json, which drizzle-kit writes on first generate and maintains after, and which none of the others produce. A directory holding .sql files but no journal is reported rather than silently passed over — a genuine drizzle output whose meta/ went missing must not just look clean. Both CLI call sites are unaffected; they pass one explicit --out and never sweep. The multi-directory tests added by the previous commit keep their intent — their fixtures are now journal-bearing, so shrinking DRIZZLE_OUT_DIRS or short-circuiting the aggregation loop still fails them — and a new pair covers the foreign-directory case at both the sweep and command layers. Also adds the afterEach that post-agent.test.ts never had: all five tests in that describe leaked a temp tree per run. --- .changeset/rewriter-never-drops-ciphertext.md | 14 ++++ .../wizard/src/__tests__/post-agent.test.ts | 81 ++++++++++++++++-- .../src/__tests__/rewrite-migrations.test.ts | 84 ++++++++++++++++++- packages/wizard/src/lib/post-agent.ts | 14 +++- packages/wizard/src/lib/rewrite-migrations.ts | 51 +++++++++++ 5 files changed, 233 insertions(+), 11 deletions(-) diff --git a/.changeset/rewriter-never-drops-ciphertext.md b/.changeset/rewriter-never-drops-ciphertext.md index 0815b61d..e32890e4 100644 --- a/.changeset/rewriter-never-drops-ciphertext.md +++ b/.changeset/rewriter-never-drops-ciphertext.md @@ -80,3 +80,17 @@ Four further ways the sweep could still reach a ciphertext column are closed: The two copies of this rewriter — one in `stash`, one in `@cipherstash/wizard` — are now compared by a repo test, so a fix can no longer land in one and silently miss the other. + +**The wizard now sweeps only drizzle-kit output directories.** It cannot +discover a project's configured `out`, so it tries `drizzle/`, `migrations/` and +`src/db/migrations/` — but the last two are generic names that Knex, +node-pg-migrate, Flyway and hand-rolled psql also use. A project whose drizzle +`out` is `drizzle/` and which also keeps a hand-maintained `migrations/` had +that second directory rewritten into ADD+DROP+RENAME, in a directory the wizard +was never pointed at. The fail-closed rule is no defence there: a real migration +history declares its own columns, so the rewrite proceeds. A candidate is now +swept only if it carries the `meta/_journal.json` drizzle-kit maintains. A +directory that holds `.sql` files but no journal is reported rather than passed +over in silence, so a genuine drizzle output whose `meta/` went missing is +visible instead of looking clean. `stash eql migration` and `stash eql install` +are unaffected — both already take a single explicit `--out`. diff --git a/packages/wizard/src/__tests__/post-agent.test.ts b/packages/wizard/src/__tests__/post-agent.test.ts index f8f87a79..e104be5a 100644 --- a/packages/wizard/src/__tests__/post-agent.test.ts +++ b/packages/wizard/src/__tests__/post-agent.test.ts @@ -1,7 +1,7 @@ import fs from 'node:fs' import os from 'node:os' import path from 'node:path' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { runPostAgentSteps } from '../lib/post-agent.js' import type { DetectedPackageManager } from '../lib/types.js' @@ -139,13 +139,35 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { } as never, }) + /** + * Make `dir` a drizzle-kit OUTPUT directory. The sweep now requires the + * `meta/_journal.json` drizzle-kit maintains, because `migrations/` and + * `src/db/migrations/` are generic names other tools use and this sweep + * emits `DROP COLUMN`. + */ + const makeDrizzleOut = (dir: string): string => { + const abs = path.join(cwd, dir) + fs.mkdirSync(path.join(abs, 'meta'), { recursive: true }) + fs.writeFileSync( + path.join(abs, 'meta', '_journal.json'), + JSON.stringify({ version: '7', dialect: 'postgresql', entries: [] }), + ) + return abs + } + beforeEach(() => { cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'wizard-post-agent-')) vi.mocked(p.confirm).mockClear() }) + afterEach(() => { + // Every test here writes a real temp tree; without this they accumulate in + // os.tmpdir() for the life of the machine. + fs.rmSync(cwd, { recursive: true, force: true }) + }) + it('defaults to Yes when the sweep changed nothing', async () => { - fs.mkdirSync(path.join(cwd, 'drizzle')) + makeDrizzleOut('drizzle') fs.writeFileSync( path.join(cwd, 'drizzle', '0000_init.sql'), 'CREATE TABLE "users" ("id" integer PRIMARY KEY);\n', @@ -167,7 +189,7 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { }) it('defaults to No, and says why, when a file was rewritten', async () => { - fs.mkdirSync(path.join(cwd, 'drizzle')) + makeDrizzleOut('drizzle') // The sweep is fail-closed: it rewrites a column only when the corpus // positively declares it (and it isn't already encrypted). A real drizzle // corpus carries this declaration in an earlier migration — supply it so @@ -204,7 +226,7 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { }) it('defaults to No when a statement was flagged rather than rewritten', async () => { - fs.mkdirSync(path.join(cwd, 'drizzle')) + makeDrizzleOut('drizzle') fs.writeFileSync( path.join(cwd, 'drizzle', '0001_using.sql'), 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search USING (email)::eql_v3_text_search;\n', @@ -226,7 +248,7 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { // Yes over migrations nobody checked. Unknown is not the same as safe. it('defaults to No when a directory could not be swept at all', async () => { // A directory named `*.sql` makes readFile throw EISDIR mid-sweep. - fs.mkdirSync(path.join(cwd, 'drizzle')) + makeDrizzleOut('drizzle') fs.mkdirSync(path.join(cwd, 'drizzle', '0001_alter.sql')) await runDrizzle() @@ -266,12 +288,12 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { // fail. it('sweeps a candidate directory other than the first', async () => { // drizzle/ exists but has nothing to do; the rewrite lives in migrations/. - fs.mkdirSync(path.join(cwd, 'drizzle')) + makeDrizzleOut('drizzle') fs.writeFileSync( path.join(cwd, 'drizzle', '0000_init.sql'), 'CREATE TABLE "widgets" ("id" integer PRIMARY KEY);\n', ) - fs.mkdirSync(path.join(cwd, 'migrations')) + makeDrizzleOut('migrations') fs.writeFileSync( path.join(cwd, 'migrations', '0000_declare.sql'), 'CREATE TABLE "users" ("email" text);\n', @@ -296,9 +318,50 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { expect(String(options?.message)).toContain('DESTROYS data') }) + // The other half of the test above. `migrations/` is swept when it is a + // drizzle output directory — and must NOT be when it belongs to Knex, + // node-pg-migrate, Flyway or hand-rolled psql, all of which use that name. + // The wizard was never pointed at such a directory, and this sweep emits + // DROP COLUMN (#772 review, finding 5). + it('leaves a migrations/ directory that is not a drizzle output untouched', async () => { + makeDrizzleOut('drizzle') + fs.writeFileSync( + path.join(cwd, 'drizzle', '0000_init.sql'), + 'CREATE TABLE "widgets" ("id" integer PRIMARY KEY);\n', + ) + // A foreign migration history: self-contained, so the fail-closed + // `declared` rule would happily pass it. + fs.mkdirSync(path.join(cwd, 'migrations'), { recursive: true }) + const foreign = path.join(cwd, 'migrations', '002_encrypt.sql') + const foreignSql = [ + 'CREATE TABLE "patients" ("ssn" text);', + 'ALTER TABLE "patients" ALTER COLUMN "ssn" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n') + fs.writeFileSync(foreign, foreignSql) + + // Only `confirm` is mocked at module level; spy on the log so the + // "passed over" notice can be asserted. + const info = vi.spyOn(p.log, 'info').mockImplementation(() => {}) + + await runDrizzle() + + expect(fs.readFileSync(foreign, 'utf-8')).toBe(foreignSql) + expect(fs.readFileSync(foreign, 'utf-8')).not.toContain('DROP COLUMN') + // Nothing was rewritten or flagged, so the prompt stays on its clean arm. + const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] + expect(options?.initialValue).toBe(true) + // But the user is told the directory was passed over, so a genuine drizzle + // output whose meta/ went missing does not just look clean. + const logged = info.mock.calls.flat().join('\n') + expect(logged).toContain('migrations/') + expect(logged).toContain('meta/_journal.json') + info.mockRestore() + }) + it('aggregates a rewrite in one directory with a flag in another', async () => { // drizzle/ declares email, so its ALTER is rewritten. - fs.mkdirSync(path.join(cwd, 'drizzle')) + makeDrizzleOut('drizzle') fs.writeFileSync( path.join(cwd, 'drizzle', '0000_declare.sql'), 'CREATE TABLE "users" ("email" text);\n', @@ -308,7 +371,7 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', ) // migrations/ never declares its column, so its ALTER is source-unknown. - fs.mkdirSync(path.join(cwd, 'migrations')) + makeDrizzleOut('migrations') const flagged = path.join(cwd, 'migrations', '0001_encrypt.sql') const flaggedSql = 'ALTER TABLE "orders" ALTER COLUMN "total" SET DATA TYPE eql_v3_text_search;\n' diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index 5ebfbf36..1914febc 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -1684,8 +1684,27 @@ describe('sweepMigrationDirs', () => { '', ].join('\n') - /** Create `dir` under the sandbox, optionally seeding `name` with `sql`. */ + /** + * Create a drizzle-kit OUTPUT directory under the sandbox, optionally seeding + * `name` with `sql`. + * + * The `meta/_journal.json` is what makes it drizzle-kit's: the sweep now + * requires it, because `migrations/` and `src/db/migrations/` are generic + * names that Knex, Flyway, node-pg-migrate and raw psql also use, and this + * sweep emits `DROP COLUMN`. Use {@link seedForeignDir} for the other case. + */ const seedDir = (dir: string, name?: string, sql?: string): string => { + const abs = seedForeignDir(dir, name, sql) + fs.mkdirSync(path.join(abs, 'meta'), { recursive: true }) + fs.writeFileSync( + path.join(abs, 'meta', '_journal.json'), + JSON.stringify({ version: '7', dialect: 'postgresql', entries: [] }), + ) + return abs + } + + /** A migration directory belonging to some OTHER tool — no drizzle journal. */ + const seedForeignDir = (dir: string, name?: string, sql?: string): string => { const abs = path.join(tmpDir, dir) fs.mkdirSync(abs, { recursive: true }) if (name) fs.writeFileSync(path.join(abs, name), sql ?? ALTER) @@ -1813,6 +1832,69 @@ describe('sweepMigrationDirs', () => { expect(updated).toBe(`${alterSql}\n`) expect(updated).not.toContain('DROP COLUMN') }) + + // #772 review, finding 5. `migrations/` and `src/db/migrations/` are generic + // names — Knex, node-pg-migrate, Flyway and hand-rolled psql all use them. + // Sweeping every candidate that merely EXISTS meant a project whose drizzle + // `out` is `drizzle/` but which also keeps a hand-maintained `migrations/` + // had that second directory rewritten into ADD+DROP+RENAME, in a directory + // this tool was never pointed at. The fail-closed `declared` rule does not + // help: a real migration history declares its own columns. + // + // drizzle-kit always writes `meta/_journal.json` into its output directory; + // none of the other tools do. That is the discriminator. + it('does not sweep a directory that is not a drizzle-kit output', async () => { + const foreign = seedForeignDir('migrations', '0001_alter.sql') + const alter = path.join(foreign, '0001_alter.sql') + + const results = await sweepMigrationDirs(tmpDir, ['migrations']) + + expect(results.find((r) => r.dir === 'migrations')?.rewritten).toEqual([]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(ALTER) + expect(updated).not.toContain('DROP COLUMN') + }) + + // Silence would be its own failure: a genuine drizzle directory whose meta/ + // was deleted must not simply do nothing without saying so. + it('reports a skipped directory that holds SQL but no drizzle journal', async () => { + seedForeignDir('migrations', '0001_alter.sql') + + const results = await sweepMigrationDirs(tmpDir, ['migrations']) + + expect(results.find((r) => r.dir === 'migrations')?.notDrizzleOutput).toBe( + true, + ) + }) + + // An empty foreign directory is not worth mentioning — there is nothing in it + // the sweep could have repaired. + it('does not report a journal-less directory that holds no SQL', async () => { + seedForeignDir('migrations') + + const results = await sweepMigrationDirs(tmpDir, ['migrations']) + + expect(results.find((r) => r.dir === 'migrations')?.notDrizzleOutput).toBe( + undefined, + ) + }) + + // The blast-radius fix must not shrink the legitimate reach: a real drizzle + // output directory that is NOT the first candidate is still swept. + it('still sweeps a drizzle output directory that is not the first candidate', async () => { + seedDir( + 'drizzle', + '0000_noop.sql', + 'CREATE TABLE "widgets" ("id" integer);\n', + ) + const migrations = seedDir('migrations', '0001_alter.sql') + + const results = await sweepMigrationDirs(tmpDir, ['drizzle', 'migrations']) + + expect(results.find((r) => r.dir === 'migrations')?.rewritten).toEqual([ + path.join(migrations, '0001_alter.sql'), + ]) + }) }) // The three reasons drive very different user action — re-encrypt through the diff --git a/packages/wizard/src/lib/post-agent.ts b/packages/wizard/src/lib/post-agent.ts index 25c37b7d..df8f7ec0 100644 --- a/packages/wizard/src/lib/post-agent.ts +++ b/packages/wizard/src/lib/post-agent.ts @@ -170,10 +170,22 @@ async function rewriteEncryptedMigrations(cwd: string): Promise<{ const results = await sweepMigrationDirs(cwd, DRIZZLE_OUT_DIRS) const totals = { rewritten: 0, skipped: 0, failedDirs: [] as string[] } - for (const { dir, rewritten, skipped, error } of results) { + for (const { dir, rewritten, skipped, error, notDrizzleOutput } of results) { totals.rewritten += rewritten.length totals.skipped += skipped.length + // Not a failure and not a risk — the directory belongs to some other tool, + // so `drizzle-kit migrate` will not run it and the prompt below is + // unaffected. Said out loud anyway, so a user whose drizzle output really + // does live here (meta/ deleted, or a hand-assembled directory) can see why + // nothing was repaired instead of assuming it was clean. + if (notDrizzleOutput) { + p.log.info( + `Left ${dir}/ alone — it holds .sql files but no drizzle-kit journal (meta/_journal.json), so it is not a drizzle output directory. If it IS your drizzle \`out\`, run \`drizzle-kit generate\` once to create the journal, then re-run the wizard.`, + ) + continue + } + // Presence, not truthiness: `error` is `err.message` for a thrown `Error`, // 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 diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 93ea0e36..74393eac 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -845,6 +845,34 @@ export interface DirRewriteResult extends RewriteResult { dir: string /** Set when this directory's sweep threw; the sweep continues regardless. */ error?: string + /** + * Set when the directory holds `.sql` files but no drizzle-kit journal, so + * nothing was swept. Reported rather than silent: a genuine drizzle output + * directory whose `meta/` was deleted would otherwise look clean. + */ + notDrizzleOutput?: true +} + +/** + * Whether `abs` is a drizzle-kit output directory. + * + * drizzle-kit writes `meta/_journal.json` there on the first `generate` and + * maintains it for every migration after; nothing else in the candidate list's + * ecosystem does. Knex, node-pg-migrate and Flyway all track state in the + * database, not in a sibling file. + */ +function isDrizzleOutputDir(abs: string): boolean { + return existsSync(join(abs, 'meta', '_journal.json')) +} + +/** Whether the directory holds anything this sweep could have acted on. */ +async function holdsSqlFiles(abs: string): Promise { + try { + const entries = await readdir(abs) + return entries.some((entry) => entry.endsWith('.sql')) + } catch { + return false + } } /** @@ -867,6 +895,15 @@ export interface DirRewriteResult extends RewriteResult { * * A directory that throws mid-sweep is reported via `error` rather than * aborting — one unreadable candidate must not strand the others. + * + * **Why only drizzle-kit output directories:** the candidate list is a guess, + * and two of its three entries are generic names that Knex, node-pg-migrate, + * Flyway and hand-rolled psql also use. This sweep emits `DROP COLUMN`, so + * rewriting a directory it was never pointed at is the worst thing it can do — + * and the fail-closed `declared` rule is no defence there, because a real + * migration history declares its own columns. Requiring the `meta/_journal.json` + * that drizzle-kit maintains keeps the reach to directories drizzle-kit owns + * (#772 review, finding 5). */ export async function sweepMigrationDirs( cwd: string, @@ -878,6 +915,20 @@ export async function sweepMigrationDirs( const abs = resolve(cwd, dir) if (!existsSync(abs)) continue + if (!isDrizzleOutputDir(abs)) { + // Only worth surfacing when there was something here to act on; an empty + // or non-SQL directory is noise. + if (await holdsSqlFiles(abs)) { + results.push({ + dir, + rewritten: [], + skipped: [], + notDrizzleOutput: true, + }) + } + continue + } + try { const { rewritten, skipped } = await rewriteEncryptedAlterColumns(abs) results.push({ dir, rewritten, skipped })