diff --git a/.changeset/rewriter-dollar-quoted-and-reconciliation.md b/.changeset/rewriter-dollar-quoted-and-reconciliation.md new file mode 100644 index 000000000..c89297908 --- /dev/null +++ b/.changeset/rewriter-dollar-quoted-and-reconciliation.md @@ -0,0 +1,52 @@ +--- +'stash': patch +'@cipherstash/wizard': patch +--- + +Close three correctness follow-ups in the Drizzle EQL migration rewriter, all of +which previously exited 0 while leaving the user in a wrong state. + +**Dollar-quoted DDL no longer bypasses the already-encrypted guard.** DDL inside +`DO $$ … END $$;` is executed SQL, but the corpus index skipped those bodies +whole, so an encrypted `ADD COLUMN` there never registered as encrypted. The +column fell through as "plaintext by residue" and the sweep staged an empty +`_encrypted` twin beside the real ciphertext, reporting success. The +index now reads dollar-quoted bodies for the *encrypted* side, so these are +flagged for staged re-encryption instead. A plaintext declaration inside such a +block still does not count as declaring the column — the block may be +conditional — so those statements remain flagged rather than rewritten. +`target-exists` also now recognises a twin that exists only inside a +dollar-quoted body, which previously produced a duplicate `ADD COLUMN` that +failed at migrate time. + +**A successful sweep now reports the artefact divergence it leaves behind.** The +rewrite repairs SQL only, so afterwards the database has both `email` (still +plaintext) and `email_encrypted`, while `schema.ts` and drizzle-kit's snapshot +both still declare `email` as the encrypted domain and know nothing about the +twin. `drizzle-kit generate` cannot surface this — it diffs the schema against +its snapshot and reads neither the `.sql` nor the database — so the divergence +was entirely silent: reads of the source column hand plaintext to a decrypt path +expecting an EQL envelope, and writes store an EQL envelope in a plaintext +column and succeed. `stash eql migration --drizzle` and the wizard now print the +divergence per column, naming the table, both columns, the domain and the +migration the twin was staged in, followed by the reconciliation: set the source +column back to its plaintext type, declare the twin under its own name, then run +`drizzle-kit generate` and delete the `ADD COLUMN` it regenerates for the twin. +That last step is load-bearing — the snapshot can only learn about the twin from +a `generate` that also emits SQL to create it, and the swept migration already +added that column, so leaving both fails with `column already exists`. The +`skills/stash-cli` and `skills/stash-drizzle` guides carry the same sequence. + +Twins are reported only once the migration file they were written into has been +saved, so a sweep that fails mid-write no longer names a column that never +reached disk. + +The new body scan is a single forward pass, so sweep time is unchanged on a real +drizzle output directory — which contains the ~2.6 MB EQL install migration, and +that file is itself thousands of `$$` PL/pgSQL bodies. + +**The wizard's per-directory sweep reporting no longer breaks on a non-`Error` +throw.** It read its partial result off an unchecked cast, so a `throw null` +raised a `TypeError` inside the very `catch` meant to report the failure — +skipping the error result and abandoning the remaining directories. It now +narrows, matching the CLI. diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 7bb16dcdc..c3ca5f5af 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -4,6 +4,7 @@ import path from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { describeSkipReason, + describeStagedReconciliation, rewriteEncryptedAlterColumns, } from '../commands/db/rewrite-migrations.js' @@ -356,6 +357,7 @@ describe('rewriteEncryptedAlterColumns', () => { const updated = fs.readFileSync(filePath, 'utf-8') expect(updated).toContain('source column "email" is deliberately preserved') expect(updated).toContain('staged `stash encrypt` lifecycle') + expect(updated).toContain('switch the application to the') expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) }) @@ -945,6 +947,18 @@ describe('rewriteEncryptedAlterColumns', () => { }) }) + /** + * #823 closed #811 by removing the blast radius — the rewrite became add-only + * — not by closing the MECHANISM. #836 item 1 closed the mechanism: the index + * now reads dollar-quoted bodies for the encrypted side, so these corpora are + * recognised as already-encrypted instead of being handed an empty twin. + * + * Two expectations here therefore tightened, both toward fail-closed: + * `already-encrypted` replaces `target-exists` on the rename corpora (after + * those renames `email` IS the ciphertext, and `target-exists`'s "review the + * existing encrypted twin" pointed at a column the rename had just consumed), + * and the `DO $$` ADD COLUMN case no longer rewrites at all. + */ describe('issue #811 dollar-quoted DDL regression', () => { const domainChange = 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "eql_v3_text_eq";' @@ -970,11 +984,14 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([]) expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) expect(skipped).toEqual([ - { file: change, statement: domainChange, reason: 'target-exists' }, + { file: change, statement: domainChange, reason: 'already-encrypted' }, ]) }) - it('does not emit destructive SQL when an encrypted ADD COLUMN is inside DO $$', async () => { + // The case #823's own test codified backwards (#836, item 1): the `DO $$` + // body really does leave `email` encrypted, so staging a twin beside the + // ciphertext was wrong. It is now recognised and flagged. + it('flags an encrypted ADD COLUMN inside DO $$ instead of staging a twin', async () => { fs.writeFileSync( path.join(tmpDir, '0000_setup.sql'), [ @@ -989,12 +1006,13 @@ describe('rewriteEncryptedAlterColumns', () => { const change = path.join(tmpDir, '0001_change_domain.sql') fs.writeFileSync(change, `${domainChange}\n`) - const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) - expect(rewritten).toEqual([change]) - const updated = fs.readFileSync(change, 'utf-8') - expect(updated).toContain('ADD COLUMN "email_encrypted"') - expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) + expect(rewritten).toEqual([]) + expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'already-encrypted' }, + ]) }) it('does not emit destructive SQL for a rename inside a custom dollar tag', async () => { @@ -1018,11 +1036,16 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([]) expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) expect(skipped).toEqual([ - { file: change, statement: domainChange, reason: 'target-exists' }, + { file: change, statement: domainChange, reason: 'already-encrypted' }, ]) }) - it('does not emit destructive SQL when an unterminated $$ hides a later encrypted declaration', async () => { + // An unterminated `$$` makes the whole file unparseable, so Postgres never + // ran any of it and nothing after the opener is proven. The index reads that + // remainder for the encrypted side anyway (see `dollarQuotedBodies`), which + // is why the twin is now seen and the statement flagged rather than + // rewritten — the fail-closed way to be wrong about a broken file. + it('flags rather than rewrites when an unterminated $$ hides an encrypted declaration', async () => { fs.writeFileSync( path.join(tmpDir, '0000_setup.sql'), [ @@ -1035,15 +1058,507 @@ describe('rewriteEncryptedAlterColumns', () => { const change = path.join(tmpDir, '0001_change_domain.sql') fs.writeFileSync(change, `${domainChange}\n`) - const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'target-exists' }, + ]) + }) + }) + + /** + * Issue #836, item 1. `isInsideCommentOrString` skips a dollar-quoted body + * WHOLE — correct for the rewrite pass, wrong for the index pass. DDL inside + * `DO $$ … END $$;` is executed SQL: the column really is encrypted in the + * database. Skipping it meant the column never entered `encrypted`, fell to + * "plaintext by residue", and the sweep added an empty `_encrypted` twin + * beside the real ciphertext — `rewritten` listing the file, `skipped` empty, + * exit code 0. + * + * The index now reads dollar-quoted bodies for the ENCRYPTED side only. The + * `declared` side stays gated: a `DO $$` body is conditional PL/pgSQL, so a + * plaintext declaration inside one may never have run, and over-detecting + * `declared` is the fail-OPEN direction. The rewrite pass is untouched — an + * ALTER inside a dollar body is still inert and still not rewritten. + */ + describe('encrypted DDL inside a dollar-quoted body', () => { + const domainChange = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "eql_v3_text_eq";' + + it('indexes an encrypted CREATE TABLE column inside DO $$', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'DO $$ BEGIN', + ' CREATE TABLE "users" ("email" "public"."eql_v3_text_search");', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'already-encrypted' }, + ]) + }) + + it('carries encryptedness through a RENAME inside DO $$', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'ALTER TABLE "users" ADD COLUMN "email_tmp" "eql_v3_text_search";', + 'DO $$ BEGIN', + ' ALTER TABLE "users" DROP COLUMN "email";', + ' ALTER TABLE "users" RENAME COLUMN "email_tmp" TO "email";', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'already-encrypted' }, + ]) + }) + + /** + * A rename CHAIN that crosses from a dollar-quoted body to a live statement + * inside the SAME file. The encryptedness has to survive both hops. + * + * This was accepted residue on the first cut, justified as unreachable + * because drizzle-kit never emits `RENAME COLUMN` inside `DO $$`. That is + * the wrong reachability test: the corpus #811 actually reported is + * hand-written and does exactly this — a human staging a manual column swap. + * The cost of missing it is the defect this whole change exists to close, an + * empty twin staged beside real ciphertext. + */ + it('carries encryptedness through a rename chain from DO $$ into a live statement', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'ALTER TABLE "users" ADD COLUMN "tmp" "eql_v3_text_search";', + 'DO $$ BEGIN', + ' ALTER TABLE "users" DROP COLUMN "email";', + ' ALTER TABLE "users" RENAME COLUMN "tmp" TO "mid";', + 'END $$;', + 'ALTER TABLE "users" RENAME COLUMN "mid" TO "email";', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'already-encrypted' }, + ]) + }) + + // And the reverse order, live -> dollar, which already worked. Pinned so a + // future reordering of the two rename passes cannot silently break one + // direction while the other keeps passing. + it('carries encryptedness through a rename chain from a live statement into DO $$', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'ALTER TABLE "users" ADD COLUMN "tmp" "eql_v3_text_search";', + 'ALTER TABLE "users" RENAME COLUMN "tmp" TO "mid";', + 'DO $$ BEGIN', + ' ALTER TABLE "users" DROP COLUMN "email";', + ' ALTER TABLE "users" RENAME COLUMN "mid" TO "email";', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'already-encrypted' }, + ]) + }) + + // The staged twin exists in the database but only inside a dollar body, so + // it is `encrypted` without ever being `declared`. Emitting another + // ADD COLUMN for it fails at migrate time with "column already exists". + it('treats an encrypted twin added inside DO $$ as an existing target', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'DO $$ BEGIN', + ' ALTER TABLE "users" ADD COLUMN "email_encrypted" "eql_v3_text_search";', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + const alter = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "eql_v3_text_search";' + fs.writeFileSync(change, `${alter}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: alter, reason: 'target-exists' }, + ]) + }) + + // The fail-OPEN direction, deliberately not taken. A `DO $$` body is + // conditional, so a PLAINTEXT declaration inside one is not proof the + // column exists — it stays undeclared and the statement stays flagged. + it('does not let a plaintext declaration inside DO $$ count as declared', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'DO $$ BEGIN', + ' ALTER TABLE "users" ADD COLUMN "email" text;', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'source-unknown' }, + ]) + }) + + // Reading dollar bodies must not resurrect INERT ones. A `DO $$` block + // sitting inside a `--` comment or a string literal never runs, so the + // encrypted declaration in it is not evidence of anything. + it('ignores a dollar-quoted body inside a line comment', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + '-- DO $$ BEGIN ALTER TABLE "users" ADD COLUMN "email" "eql_v3_text_search"; END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([change]) - const updated = fs.readFileSync(change, 'utf-8') - expect(updated).toContain('ADD COLUMN "email_encrypted"') - expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) + expect(skipped).toEqual([]) + expect(fs.readFileSync(change, 'utf-8')).toContain( + 'ADD COLUMN "email_encrypted"', + ) + }) + + it('ignores a dollar-quoted body inside a string literal', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + `INSERT INTO "audit" ("sql") VALUES ('DO $$ BEGIN ALTER TABLE "users" ADD COLUMN "email" "eql_v3_text_search"; END $$;');`, + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([change]) + expect(skipped).toEqual([]) + }) + + // The rewrite pass keeps treating a dollar body as inert: `renderSafeAlter` + // returns MULTIPLE lines, and splicing them into a PL/pgSQL body would + // rewrite code the sweep cannot reason about. + it('still refuses to rewrite an ALTER that sits inside DO $$', async () => { + const file = path.join(tmpDir, '0000_setup.sql') + const sql = [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'DO $$ BEGIN', + ` ${domainChange}`, + 'END $$;', + '', + ].join('\n') + fs.writeFileSync(file, sql) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) + expect(fs.readFileSync(file, 'utf-8')).toBe(sql) + }) + + // drizzle-kit's own enum idiom. It touches no table, so widening the index + // must not turn every corpus containing one into a wall of flagged + // statements — the reason a blanket "fail closed on any dollar-quoted body" + // was rejected in favour of indexing the encrypted side. + it('leaves the drizzle-kit CREATE TYPE enum idiom rewritable', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'DO $$ BEGIN CREATE TYPE "public"."role" AS ENUM(\'admin\'); EXCEPTION WHEN duplicate_object THEN null; END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([change]) + expect(skipped).toEqual([]) }) }) + /** + * #836, item 2. The sweep repairs SQL and nothing else, so a successful + * rewrite leaves schema.ts, the drizzle-kit snapshot and the database + * three-way divergent — and `drizzle-kit generate` cannot surface it, because + * it diffs schema.ts against the snapshot and those two still agree. The + * rewriter therefore reports exactly what it staged so the caller can name it. + */ + describe('staged reconciliation reporting', () => { + it('names the table, both columns and the domain it staged', async () => { + const create = path.join(tmpDir, '0000_create.sql') + fs.writeFileSync( + create, + 'CREATE TABLE "users" ("email" text NOT NULL);\n', + ) + const filePath = path.join(tmpDir, '0001_alter.sql') + fs.writeFileSync( + filePath, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { staged } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(staged).toEqual([ + { + file: filePath, + schema: undefined, + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ]) + }) + + it('keeps the schema qualifier for a pgSchema() table', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "app"."users" ("email" text NOT NULL);\n', + ) + const filePath = path.join(tmpDir, '0001_alter.sql') + fs.writeFileSync( + filePath, + 'ALTER TABLE "app"."users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { staged } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(staged).toEqual([ + { + file: filePath, + schema: 'app', + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ]) + }) + + // Finer-grained than `rewritten`, which lists FILES: one file can carry + // several ALTERs and each one stages its own twin. + it('records one entry per staged column, not per file', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "users" ("email" text, "phone" text);\n', + ) + const filePath = path.join(tmpDir, '0001_alter.sql') + fs.writeFileSync( + filePath, + [ + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + 'ALTER TABLE "users" ALTER COLUMN "phone" SET DATA TYPE eql_v3_text_eq;', + '', + ].join('\n'), + ) + + const { rewritten, staged } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([filePath]) + expect(staged.map((s) => s.encryptedColumn)).toEqual([ + 'email_encrypted', + 'phone_encrypted', + ]) + expect(staged.map((s) => s.domain)).toEqual([ + 'eql_v3_text_search', + 'eql_v3_text_eq', + ]) + }) + + // A statement the sweep refused to rewrite changed nothing on disk, so + // there is no divergence to reconcile and no notice to print. + it('stages nothing when every statement was skipped', async () => { + fs.writeFileSync( + path.join(tmpDir, '0001_alter.sql'), + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped, staged } = + await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(staged).toEqual([]) + }) + + it('describes the divergence, the silence, and the snapshot trap', async () => { + const lines = describeStagedReconciliation([ + { + file: '/tmp/0001_alter.sql', + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ]).join('\n') + + expect(lines).toContain('"email_encrypted" eql_v3_text_search') + expect(lines).toContain('users:') + // The three things a user cannot discover on their own. + expect(lines).toContain('drizzle-kit generate` will NOT warn you') + expect(lines).toContain('column already exists') + expect(lines).toContain('SUCCEED') + }) + + // Names the migration the twin was staged in, so the user can go read the + // statement rather than hunt the directory for it. + it('names the migration file each twin was staged in', async () => { + const lines = describeStagedReconciliation([ + { + file: '/tmp/drizzle/0001_alter.sql', + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ]).join('\n') + + expect(lines).toContain('/tmp/drizzle/0001_alter.sql') + }) + + /** + * The remediation has to be followed to a working end state, and the + * obvious reading of it does not get there. + * + * After a sweep, `schema.ts` declares the SOURCE column as the encrypted + * domain — that declaration is what made drizzle-kit emit the invalid + * ALTER — and the snapshot agrees with it. So the guidance must say to set + * the source column BACK to plaintext, not to "keep" it. + * + * And the snapshot has never seen the twin, so `drizzle-kit generate` + * ALWAYS emits an `ADD COLUMN` for it — the swept migration already adds + * that column, so applying both fails with "column already exists". That + * is not avoidable by editing the schema more carefully; the generated + * statement has to be removed. Guidance that stops at "run generate" walks + * the user into the very error it warns about. + */ + it('tells the user to revert the source column and drop the regenerated ADD COLUMN', async () => { + const lines = describeStagedReconciliation([ + { + file: '/tmp/0001_alter.sql', + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ]).join('\n') + + // Revert, not "keep": the schema currently declares it as the domain. + expect(lines).toMatch(/\bback to its plaintext type\b/i) + expect(lines).not.toContain('keep the source column as its plaintext') + // The step that makes the difference between working and failing. + expect(lines).toMatch(/\b(?:DELETE|delete|remove)\b[^\n]*ADD COLUMN/) + // And it must not blame partial editing for the duplicate. + expect(lines).not.toContain('Do not hand-edit only the schema') + }) + }) + + /** + * `dollarQuotedBodies` must track comment/string state itself. Asking + * `isInsideCommentOrString` about each `$` is the obvious implementation and + * is quadratic, because that predicate rescans from index 0 every call. + * + * This corpus is the shape that makes it bite — thousands of small `$$` + * PL/pgSQL bodies, which is exactly the ~2.6 MB EQL install migration sitting + * in a real drizzle output directory next to the ALTER being swept. So this is + * shipped-command latency, not a microbenchmark: on that corpus the whole + * sweep measures ~0.4 s in one pass and ~8.5 s per-opener. + * + * Sized so the two are unambiguous rather than marginal. Over this ~2.1 MB + * corpus the whole sweep measures ~70 ms in one pass and ~41 s per-opener, so + * the 15 s bound sits ~200x above the linear time and ~3x below the regressed + * one. + * + * That headroom is the answer to the usual objection that wall-clock + * assertions flake on loaded runners: tripping this one needs a machine ~200x + * slower at scanning a string than a developer laptop, where CI is 2-5x. It is + * a coarse order-of-magnitude gate, not a millisecond budget — if it ever does + * go off spuriously, raise the bound rather than delete the test, because the + * regression it guards is shipped-command latency. + */ + it('scans a dollar-quote-heavy corpus in a single pass', async () => { + const bodies = Array.from( + { length: 32_000 }, + (_, n) => + `DO $$ BEGIN PERFORM ${n}; EXCEPTION WHEN others THEN null; END $$;`, + ).join('\n') + fs.writeFileSync(path.join(tmpDir, '0000_install.sql'), bodies) + fs.writeFileSync( + path.join(tmpDir, '0001_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + const alter = path.join(tmpDir, '0002_alter.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const started = Date.now() + const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + const elapsed = Date.now() - started + + // Still correct: the dollar bodies declare nothing, so the ALTER rewrites. + expect(rewritten).toEqual([alter]) + expect(elapsed).toBeLessThan(15_000) + }, 180_000) + // A domain change on a column that is ALREADY encrypted needs staged // re-encryption; there is no plaintext source to backfill from. describe('columns that are already encrypted', () => { @@ -1669,12 +2184,22 @@ describe('rewriteEncryptedAlterColumns', () => { await rewriteEncryptedAlterColumns(tmpDir) throw new Error('expected rewriteEncryptedAlterColumns to throw') } catch (error) { - const partial = error as { rewritten?: string[]; skipped?: unknown[] } + const partial = error as { + rewritten?: string[] + skipped?: unknown[] + staged?: { column: string }[] + } expect(partial.rewritten).toEqual([first]) expect(partial.skipped).toEqual([]) expect(fs.readFileSync(first, 'utf-8')).toContain( 'ADD COLUMN "email_encrypted"', ) + // `staged` drives a notice telling the user the database now HAS these + // columns, so it must only list twins that reached disk. `name`'s twin was + // staged in memory during the string replace and then lost when the write + // threw — reporting it would send the user reconciling a column that + // exists nowhere. + expect(partial.staged?.map((s) => s.column)).toEqual(['email']) } finally { if (fs.statSync(failing).isDirectory()) fs.rmdirSync(failing) } diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index db82b4519..c8c60ca1f 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -270,6 +270,102 @@ function dollarQuoteDelimiter(sql: string, open: number): string | undefined { /** 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 +/** + * The INNER text of every dollar-quoted body (`$$ … $$`, `$fn$ … $fn$`) whose + * opener sits at a live position — one that {@link isInsideCommentOrString} + * does not already call inert. + * + * **Why this exists (#836, item 1):** {@link isInsideCommentOrString} skips a + * dollar-quoted body WHOLE, and that is right for the rewrite pass — the body + * is PL/pgSQL, {@link renderSafeAlter} returns multiple lines, and splicing + * them into code this sweep cannot reason about is not a repair. But it is + * wrong for {@link indexColumnDeclarations}: DDL inside `DO $$ … END $$;` is + * EXECUTED. An encrypted `ADD COLUMN` there means the column really does hold + * ciphertext, yet it never entered `encrypted`, so the column fell to + * "plaintext by residue" and the sweep added an empty `_encrypted` twin + * beside the real ciphertext — reported as a successful rewrite (#811). + * + * So the index reads these bodies, and only for the ENCRYPTED side. That is + * the same asymmetry the per-column `CREATE TABLE` loops already rely on: + * over-detecting "encrypted" costs a flagged statement, while over-detecting + * "declared" is what puts a column back on the fail-open path. A `DO $$` body + * is CONDITIONAL PL/pgSQL — an `IF … THEN` arm, an `EXCEPTION` handler — so a + * plaintext declaration inside one is not proof the column exists, and + * {@link indexEncryptedDeclarations} deliberately does not record one. + * + * An INERT body stays inert: a `DO $$ … $$;` inside a `--` comment or quoted + * inside an `INSERT … VALUES ('…')` never ran, and its DDL is evidence of + * nothing. + * + * An UNTERMINATED body yields its remainder to the end of the file. That is + * deliberately the opposite of what {@link isInsideCommentOrString} does with + * one, and the two are not in conflict: there, "inert to the end of the file" + * protects quote PARITY for the rewrite; here the goal is COVERAGE, and a file + * whose dollar quote never closes is one Postgres rejects outright — so + * everything it says is unproven, and over-detecting "encrypted" (a flagged + * statement) is the safe way to be wrong about it. + * + * **This tracks inertness itself rather than calling + * {@link isInsideCommentOrString} per opener, and that is a hard requirement, + * not a tidy-up.** That predicate rescans from index 0 on every call, so asking + * it about each `$` makes this quadratic — and the corpus this runs over + * routinely includes the ~2.6 MB EQL install migration, which is itself + * thousands of `$$` PL/pgSQL bodies. Measured on that corpus: ~370 ms as a + * single pass, ~8.5 s per-opener. Same token rules, same helpers, one traversal. + */ +function* dollarQuotedBodies(sql: string): Generator { + let i = 0 + while (i < sql.length) { + if (sql.startsWith('--', i)) { + const eol = sql.indexOf('\n', i) + // Runs to EOF, so nothing after it is live. + if (eol === -1) return + i = eol + 1 + } else if (sql.startsWith('/*', i)) { + // Nestable, exactly as in `isInsideCommentOrString`: stopping at the first + // `*/` would let the text after a nested close read as live SQL. + let depth = 1 + let j = i + 2 + while (j < sql.length && depth > 0) { + if (sql.startsWith('/*', j)) { + depth += 1 + j += 2 + } else if (sql.startsWith('*/', j)) { + depth -= 1 + j += 2 + } else { + j += 1 + } + } + i = j + } else if (sql[i] === '$') { + const delimiter = dollarQuoteDelimiter(sql, i) + if (delimiter === undefined) { + i += 1 + continue + } + const bodyStart = i + delimiter.length + const close = sql.indexOf(delimiter, bodyStart) + if (close === -1) { + yield sql.slice(bodyStart) + return + } + // The body is opaque: resuming past its close is what stops a `'` or `--` + // inside it from being read as SQL. + yield sql.slice(bodyStart, close) + i = close + delimiter.length + } else if (sql[i] === '"') { + // Before the `'` branch, so an apostrophe inside `"o'brien"` cannot open a + // phantom literal. Unterminated returns `sql.length` and ends the walk. + i = endOfQuoted(sql, i, '"') + } else if (sql[i] === "'") { + i = endOfQuoted(sql, i, "'", isEscapeStringOpener(sql, i)) + } else { + i += 1 + } + } +} + /** * Whether the `'` at `open` starts an `E''` escape-string, in which a backslash * escapes the following character. Plain literals give backslash no special @@ -558,64 +654,174 @@ function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { const declared = new Set() for (const sql of contents) { - 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 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 - // string literal was already skipped above. - // - // Asymmetric on purpose. ENCRYPTED does NOT re-check comments here: a - // commented-out encrypted column inside an otherwise live CREATE TABLE - // still counts, exactly as it did before this sweep learned to declare - // anything — over-detecting "encrypted" only costs a flagged statement. - // DECLARED DOES re-check: a commented-out plaintext line never ran, so - // it must not count as a declaration — over-detecting "declared" is - // what would put a truly-undeclared, possibly already-encrypted column - // back on the fail-open path. - for (const column of body.matchAll(CREATE_TABLE_ENCRYPTED_COLUMN_RE)) { - encrypted.add(columnKey(table, column[1], schema)) - } + // Materialised once: both passes below walk the same bodies, and the + // generator re-scans the file each time it is iterated. + const opaque = [...dollarQuotedBodies(sql)] - for (const column of body.matchAll(DECLARED_COLUMN_RE)) { - if (isInsideCommentOrString(body, column.index)) continue - declared.add(columnKey(table, column[1], schema)) - } + indexDeclarations(sql, encrypted, declared) + for (const body of opaque) indexEncryptedDeclarations(body, encrypted) + + // Renames run after EVERY declaration above, live and dollar-quoted: a + // rename carries the column's type with it, so the ADD it refers to has to + // be indexed first. + // + // Iterated to a FIXPOINT because a rename chain can alternate between live + // and dollar-quoted statements in either order within one file, and a single + // ordered pair of passes only follows one of those orders. Running the live + // pass first missed `ADD tmp ` → `RENAME tmp TO mid` inside `DO $$` + // → live `RENAME mid TO email`, leaving `email` looking like plaintext and + // staging an empty twin beside its ciphertext — the exact defect this change + // exists to close. Hand-written manual column swaps do this; the corpus #811 + // reported is one. + // + // Terminates: both sets only ever grow, and are bounded by the number of + // distinct column keys in the corpus. Converges in one extra iteration past + // the longest chain, so a corpus with no renames costs a second scan that + // finds nothing. + let indexed = -1 + while (indexed !== encrypted.size + declared.size) { + indexed = encrypted.size + declared.size + indexRenames(sql, encrypted, declared) + for (const body of opaque) indexEncryptedRenames(body, encrypted) } + } + + return { encrypted, declared } +} - for (const added of sql.matchAll(ADD_ENCRYPTED_COLUMN_RE)) { - if (isInsideCommentOrString(sql, added.index)) continue - const { schema, table } = tableOf(added[1], added[2]) - encrypted.add(columnKey(table, added[3], schema)) +/** Declarations at LIVE positions — both sides of the index. */ +function indexDeclarations( + sql: string, + encrypted: Set, + declared: Set, +): void { + 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 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 + // string literal was already skipped above. + // + // Asymmetric on purpose. ENCRYPTED does NOT re-check comments here: a + // commented-out encrypted column inside an otherwise live CREATE TABLE + // still counts, exactly as it did before this sweep learned to declare + // anything — over-detecting "encrypted" only costs a flagged statement. + // DECLARED DOES re-check: a commented-out plaintext line never ran, so + // it must not count as a declaration — over-detecting "declared" is + // what would put a truly-undeclared, possibly already-encrypted column + // back on the fail-open path. + for (const column of body.matchAll(CREATE_TABLE_ENCRYPTED_COLUMN_RE)) { + encrypted.add(columnKey(table, column[1], schema)) } - for (const added of sql.matchAll(ADD_COLUMN_RE)) { - if (isInsideCommentOrString(sql, added.index)) continue - const { schema, table } = tableOf(added[1], added[2]) - declared.add(columnKey(table, added[3], schema)) + for (const column of body.matchAll(DECLARED_COLUMN_RE)) { + if (isInsideCommentOrString(body, column.index)) continue + declared.add(columnKey(table, column[1], schema)) } + } + + for (const added of sql.matchAll(ADD_ENCRYPTED_COLUMN_RE)) { + if (isInsideCommentOrString(sql, added.index)) continue + const { schema, table } = tableOf(added[1], added[2]) + encrypted.add(columnKey(table, added[3], schema)) + } + + for (const added of sql.matchAll(ADD_COLUMN_RE)) { + if (isInsideCommentOrString(sql, added.index)) continue + const { schema, table } = tableOf(added[1], added[2]) + declared.add(columnKey(table, added[3], schema)) + } +} - // A rename carries the column's type with it — and `__cipherstash_tmp` - // renamed onto the real name is exactly what a previous sweep of this very - // directory emitted. Run after ADD so that tmp column is already indexed. - for (const renamed of sql.matchAll(RENAME_COLUMN_RE)) { - if (isInsideCommentOrString(sql, renamed.index)) continue - const { schema, table } = tableOf(renamed[1], renamed[2]) - const from = columnKey(table, renamed[3], schema) - const to = columnKey(table, renamed[4], schema) - if (encrypted.has(from)) encrypted.add(to) - if (declared.has(from)) declared.add(to) +/** + * The ENCRYPTED side only, over one {@link dollarQuotedBodies} body. + * + * `declared` is deliberately untouched — see {@link dollarQuotedBodies} for why + * a conditional PL/pgSQL body may not prove a plaintext column exists. The + * consequence is the safe one: a column whose ONLY declaration sits inside a + * dollar body stays `source-unknown` and is flagged rather than rewritten. + * + * Comments and literals INSIDE the body are not re-checked, matching the live + * encrypted `CREATE TABLE` loop: a commented-out encrypted column still counts, + * because over-detecting "encrypted" only ever costs a flagged statement. + */ +function indexEncryptedDeclarations( + body: string, + encrypted: Set, +): void { + for (const created of body.matchAll(CREATE_TABLE_HEAD_RE)) { + const { schema, table } = tableOf(created[1], created[2]) + const bodyStart = created.index + created[0].length + const bodyEnd = endOfCreateTableBody(body, bodyStart) + if (bodyEnd === undefined) continue + for (const column of body + .slice(bodyStart, bodyEnd) + .matchAll(CREATE_TABLE_ENCRYPTED_COLUMN_RE)) { + encrypted.add(columnKey(table, column[1], schema)) } } - return { encrypted, declared } + for (const added of body.matchAll(ADD_ENCRYPTED_COLUMN_RE)) { + const { schema, table } = tableOf(added[1], added[2]) + encrypted.add(columnKey(table, added[3], schema)) + } +} + +/** + * Renames at LIVE positions — both sides of the index. + * + * `__cipherstash_tmp` renamed onto the real name is exactly what a + * previous sweep of this very directory emitted, so a rename has to carry the + * source column's type onto its new name. + */ +function indexRenames( + sql: string, + encrypted: Set, + declared: Set, +): void { + for (const renamed of sql.matchAll(RENAME_COLUMN_RE)) { + if (isInsideCommentOrString(sql, renamed.index)) continue + const { schema, table } = tableOf(renamed[1], renamed[2]) + const from = columnKey(table, renamed[3], schema) + const to = columnKey(table, renamed[4], schema) + if (encrypted.has(from)) encrypted.add(to) + if (declared.has(from)) declared.add(to) + } +} + +/** + * Renames inside one {@link dollarQuotedBodies} body — ENCRYPTED side only. + * + * This is the shape #811 actually reported: `ADD COLUMN "email_encrypted" + * ` live, then `RENAME COLUMN "email_encrypted" TO "email"` inside + * `DO $$ … END $$;`. After that block `email` holds the ciphertext, so an + * `ALTER COLUMN "email" SET DATA TYPE …` needs staged RE-encryption, not a + * second twin. + * + * `declared` is not propagated, for the same reason + * {@link indexEncryptedDeclarations} does not record one: the rename may sit in + * a branch that never ran, and inventing a declaration from it is the + * fail-open direction. + * + * Chains that alternate between live and dollar-quoted statements are followed + * in EITHER order: {@link indexColumnDeclarations} iterates this pass and the + * live one to a fixpoint, so which runs first stops mattering. Crossing files + * works too, since files are indexed in sorted order. + */ +function indexEncryptedRenames(body: string, encrypted: Set): void { + for (const renamed of body.matchAll(RENAME_COLUMN_RE)) { + const { schema, table } = tableOf(renamed[1], renamed[2]) + if (encrypted.has(columnKey(table, renamed[3], schema))) { + encrypted.add(columnKey(table, renamed[4], schema)) + } + } } /** Why a recognised ALTER-to-encrypted statement was left alone. */ @@ -658,19 +864,149 @@ export function describeSkipReason(reason: SkipReason): string { } } +/** + * An encrypted twin this sweep added, named precisely enough for the caller to + * tell the user which ORM artefacts now disagree (#836, item 2). + * + * The rewrite is add-only, so after it the DATABASE has both `` (still + * its original plaintext type) and `` (the domain). Nothing + * updates the other two artefacts: `schema.ts` still declares `` AS the + * encrypted domain — that declaration is what made drizzle-kit emit the + * impossible `SET DATA TYPE` in the first place — and `meta/*_snapshot.json` + * still records the same thing. Neither knows `` exists. + */ +export interface StagedColumn { + /** Absolute path of the migration file the staged ADD COLUMN was written to. */ + file: string + /** Schema qualifier, when the table carried one (a `pgSchema()` table). */ + schema?: string + /** Table the column belongs to, without quotes. */ + table: string + /** The preserved source column, still its original plaintext type. */ + column: string + /** The twin that was added — conventionally `_encrypted`. */ + encryptedColumn: string + /** Bare EQL domain the twin was given, e.g. `eql_v3_text_search`. */ + domain: string +} + +/** + * The reconciliation the user must do by hand after a sweep staged twins, as + * lines ready to print (#836, item 2). + * + * **Why this has to be said out loud.** The sweep repairs SQL and nothing else, + * so a successful rewrite leaves three artefacts disagreeing: + * + * - the database gets `email text` plus `email_encrypted eql_v3_text_search` + * - `schema.ts` still declares `email` as the encrypted domain + * - `meta/*_snapshot.json` still records `email` as the encrypted domain + * - neither `schema.ts` nor the snapshot knows `email_encrypted` exists + * + * **`drizzle-kit generate` gives no signal.** It diffs `schema.ts` against the + * snapshot; it never reads `.sql` and never introspects the database. Both its + * inputs still agree, so the diff is empty and it emits nothing. It cannot + * propose a column that appears in neither input — that is `push`, which does + * introspect. So the divergence is invisible to the ORM's own tooling, and every + * consequence through it is silent: reads of `users.email` push plaintext into a + * `customType.fromDriver` expecting an EQL envelope, writes push an envelope + * into a `text` column and SUCCEED, and `email_encrypted` is unreachable because + * it is in no Drizzle schema. + * + * It only fails loudly much later: correcting `schema.ts` by hand makes + * `generate` diff against the stale snapshot and emit a duplicate ADD COLUMN, + * which errors at migrate time with "column already exists". + * + * **Why guidance rather than an automatic edit.** Editing `schema.ts` alone + * CREATES that duplicate-column failure — a correct automatic fix would have to + * rewrite drizzle-kit's snapshot in lockstep, in an internal format that is + * version-coupled and not ours. So the sweep names the divergence precisely and + * leaves the edit to the user, who is the one who knows whether the application + * is ready to read the twin. + */ +export function describeStagedReconciliation( + staged: readonly StagedColumn[], +): string[] { + const lines = [ + 'The sweep repaired SQL only. Your Drizzle schema and drizzle-kit snapshot still describe the OLD shape, so reconcile them before the application reads these columns:', + ] + for (const { + file, + schema, + table, + column, + encryptedColumn, + domain, + } of staged) { + const ref = schema ? `${schema}.${table}` : table + lines.push( + ` - ${ref}: the database now has "${column}" (unchanged, still plaintext) and "${encryptedColumn}" ${domain} (staged in ${file}), but your schema declares "${column}" as ${domain} and knows nothing about "${encryptedColumn}".`, + ) + } + lines.push( + 'Until you reconcile it: reads of the source column hand plaintext to a decrypt path expecting an EQL envelope, writes store an EQL envelope in a plaintext column and SUCCEED, and the new encrypted column is unreachable through the ORM.', + '`drizzle-kit generate` will NOT warn you — it diffs your schema against its snapshot and reads neither the .sql nor the database, and those two still agree.', + 'To reconcile, first fix your Drizzle schema: declare each encrypted column above under its own name, and set each source column BACK to its plaintext type — it is currently declared as the encrypted domain, and that declaration is what made drizzle-kit emit the invalid ALTER in the first place.', + 'Then run `drizzle-kit generate` to advance the snapshot, and DELETE the regenerated `ADD COLUMN` statement for each encrypted column from the migration it writes. The snapshot has never seen those columns, so `generate` always emits an ADD COLUMN for them — and the migration above already adds them, so applying both fails with "column already exists". Removing the statement keeps the snapshot advance, which is the whole point of the step. (`ADD COLUMN IF NOT EXISTS` works too.) Anything else `generate` emits, such as a no-op SET DATA TYPE on the source column, can stay.', + 'Then `drizzle-kit migrate`, and run the staged `stash encrypt` lifecycle to backfill before switching reads across. If you have NOT yet applied the migration above, you can instead revert your schema, remove that migration (its .sql, its meta/*_snapshot.json, and its meta/_journal.json entry) and start from the documented staged rollout, which generates the ADD COLUMN itself and leaves nothing to delete.', + ) + return lines +} + /** Outcome of a sweep: the files rewritten, and near-misses left for review. */ export interface RewriteResult { /** Absolute paths of files whose unsafe ALTER COLUMN(s) were rewritten. */ rewritten: string[] /** Near-miss statements the strict matcher passed over — flag, don't guess. */ skipped: SkippedAlter[] + /** + * Every encrypted twin staged, for the caller's reconciliation notice. One + * entry per rewritten statement, so this is finer-grained than `rewritten` + * (which lists files, and a file can carry several ALTERs). + */ + staged: StagedColumn[] } -interface RewriteSweepError extends Error { +/** + * What {@link rewriteEncryptedAlterColumns} attaches to the error when the + * sweep throws partway through: the files it had already rewritten, and the + * statements it had already flagged. Reported so a partial sweep names the work + * it did rather than looking like it never ran (#786). + */ +export interface PartialRewriteResult { rewritten?: string[] skipped?: SkippedAlter[] + staged?: StagedColumn[] } +/** + * Narrow a caught value to a {@link PartialRewriteResult}. + * + * The sweep can also fail with a non-`Error` throw — a string, `null`, anything + * — in which case there is no partial result to report. **Narrow rather than + * cast:** on `null` or `undefined` a property read raises a `TypeError` inside + * the very `catch` block that exists to report the failure, so the report never + * happens and the throw escapes the handler entirely. That defeats the + * fail-closed reporting outright, which is worse than the missing detail. + * + * Exported because both callers need it and the two copies of this file must + * stay identical: the wizard's `sweepMigrationDirs` uses it to build a + * per-directory error result, and the CLI's `eql migration --drizzle` uses it to + * report a partial sweep (#836, item 3). + */ +export function isPartialRewriteResult( + value: unknown, +): value is PartialRewriteResult { + if (typeof value !== 'object' || value === null) return false + const partial = value as Record + return ( + (partial.rewritten === undefined || Array.isArray(partial.rewritten)) && + (partial.skipped === undefined || Array.isArray(partial.skipped)) && + (partial.staged === undefined || Array.isArray(partial.staged)) + ) +} + +interface RewriteSweepError extends Error, PartialRewriteResult {} + /** * Replace in-place `ALTER COLUMN ... SET DATA TYPE ` * statements with a staged encrypted-column addition. @@ -718,6 +1054,7 @@ export async function rewriteEncryptedAlterColumns( ) const rewritten: string[] = [] const skipped: SkippedAlter[] = [] + const staged: StagedColumn[] = [] const seen = new Set() const stagedTargets = new Set() @@ -752,6 +1089,14 @@ export async function rewriteEncryptedAlterColumns( // Reset the regex's lastIndex — it's stateful on /g ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 + // Buffered per file, then committed to `staged` only once this file's + // write has SUCCEEDED. The entries are produced inside `.replace()`, which + // runs before the write — and `staged` drives a notice telling the user the + // database now HAS these columns. Pushing straight to `staged` reported + // twins that never reached disk when a later write threw, sending the user + // to reconcile a column that exists nowhere. + const fileStaged: StagedColumn[] = [] + const updated = original.replace( ALTER_COLUMN_TO_ENCRYPTED_RE, ( @@ -782,8 +1127,18 @@ export async function rewriteEncryptedAlterColumns( return match } + // `encryptedColumns` as well as `declaredColumns`: the two sets are + // not nested. A twin added inside a dollar-quoted body — or a + // commented-out encrypted column inside a live CREATE TABLE — is + // `encrypted` without ever being `declared`, and emitting a second + // ADD COLUMN for it fails at migrate time with "column already + // exists". Over-detecting here costs a flagged statement. const target = columnKey(table, `${column}_encrypted`, schema) - if (declaredColumns.has(target) || stagedTargets.has(target)) { + if ( + declaredColumns.has(target) || + encryptedColumns.has(target) || + stagedTargets.has(target) + ) { skip(filePath, match.trim(), 'target-exists') return match } @@ -793,7 +1148,18 @@ export async function rewriteEncryptedAlterColumns( // but leave the statement alone rather than emit a broken rewrite. if (!domain) return match + // `stagedTargets` is the in-run duplicate guard and is deliberately + // NOT rolled back with the buffer: if the write fails, treating the + // target as taken keeps a later file from staging it again. stagedTargets.add(target) + fileStaged.push({ + file: filePath, + schema, + table, + column, + encryptedColumn: `${column}_encrypted`, + domain, + }) return renderSafeAlter(table, column, domain, schema) }, ) @@ -801,6 +1167,7 @@ export async function rewriteEncryptedAlterColumns( if (updated !== original) { await writeFile(filePath, updated, 'utf-8') rewritten.push(filePath) + staged.push(...fileStaged) } // Broad secondary scan on the POST-rewrite content: anything still carrying @@ -828,11 +1195,12 @@ export async function rewriteEncryptedAlterColumns( const partial = error as RewriteSweepError partial.rewritten = rewritten partial.skipped = skipped + partial.staged = staged } throw error } - return { rewritten, skipped } + return { rewritten, skipped, staged } } /** diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index 6b21ffcfc..054755cf3 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -353,6 +353,92 @@ describe('eqlMigrationCommand — Drizzle', () => { ) }) + /** + * #836, item 2. A successful sweep used to be silent about the state it left + * behind: the database gains `email_encrypted`, while schema.ts and the + * drizzle-kit snapshot both still declare `email` as the domain and know + * nothing about the twin. `drizzle-kit generate` cannot surface that — it + * diffs schema.ts against the snapshot and those two still agree — so the + * command has to say it. + */ + it('warns that schema.ts and the snapshot diverged after staging a twin', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + writeFileSync( + join(out, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + writeFileSync( + join(out, '0001_encrypt-email.sql'), + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "undefined"."eql_v3_text_search";\n', + ) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0002_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + + await eqlMigrationCommand({ drizzle: true, out }) + + const warnings = clack.log.warn.mock.calls + .map((c) => String(c[0])) + .join('\n') + // Named precisely, not a generic "review your schema". + expect(warnings).toContain('users:') + expect(warnings).toContain('"email_encrypted" eql_v3_text_search') + // The three things the user cannot discover from the tooling. + expect(warnings).toContain('drizzle-kit generate` will NOT warn you') + expect(warnings).toContain('column already exists') + expect(warnings).toContain('SUCCEED') + }) + + // Warning, not a failure: the swept SQL is valid and additive, so the command + // must still succeed and still print its next-steps note. + it('does not exit non-zero merely because a twin was staged', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + writeFileSync( + join(out, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + writeFileSync( + join(out, '0001_encrypt-email.sql'), + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "undefined"."eql_v3_text_search";\n', + ) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0002_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + + await expect( + eqlMigrationCommand({ drizzle: true, out }), + ).resolves.toBeUndefined() + + expect(clack.log.error).not.toHaveBeenCalled() + expect(clack.log.success).toHaveBeenCalled() + }) + + // No rewrite means nothing diverged, so the notice must not fire — it would + // send the user editing a schema that is already consistent. + it('does not warn about reconciliation when nothing was staged', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + writeFileSync( + join(out, '0000_unrelated.sql'), + 'CREATE TABLE "widgets" ("id" integer);\n', + ) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0001_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + + await eqlMigrationCommand({ drizzle: true, out }) + + const warnings = clack.log.warn.mock.calls + .map((c) => String(c[0])) + .join('\n') + expect(warnings).not.toContain('drizzle-kit generate` will NOT warn you') + }) + it('does not rewrite the EQL install migration it just generated', async () => { const out = join(tmp, 'drizzle') mkdirSync(out, { recursive: true }) @@ -491,6 +577,47 @@ describe('eqlMigrationCommand — Drizzle', () => { ) }) + /** + * A sweep that threw PART WAY through has already written staged twins to + * disk, so the same three-way divergence exists for them and the + * reconciliation notice still has to fire. The command aborts either way — + * this pins that aborting does not swallow the guidance for work that did + * land. + */ + it('reports staged twins from a sweep that threw part way through', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + spawnMock.mockImplementation(() => { + writeFileSync(join(out, '0000_install-eql.sql'), '') + return { status: 0, stdout: '', stderr: '' } + }) + const partial = Object.assign(new Error('EISDIR'), { + rewritten: [join(out, '0001_encrypt.sql')], + skipped: [], + staged: [ + { + file: join(out, '0001_encrypt.sql'), + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ], + }) + rewriteMock.spy.mockRejectedValueOnce(partial) + + await expect( + eqlMigrationCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) + + const warnings = clack.log.warn.mock.calls + .map((c) => String(c[0])) + .join('\n') + expect(warnings).toContain('"email_encrypted" eql_v3_text_search') + expect(warnings).toContain('drizzle-kit generate` will NOT warn you') + expect(warnings).toContain('column already exists') + }) + it('aborts (exit 1) when drizzle-kit exits non-zero', async () => { spawnMock.mockReturnValue({ status: 1, stdout: '', stderr: 'boom' }) await expect( diff --git a/packages/cli/src/commands/eql/migration.ts b/packages/cli/src/commands/eql/migration.ts index ff0633bcc..5008a114e 100644 --- a/packages/cli/src/commands/eql/migration.ts +++ b/packages/cli/src/commands/eql/migration.ts @@ -8,8 +8,10 @@ import { CliExit } from '@/cli/exit.js' import { printNextSteps, SAFE_MIGRATION_NAME } from '@/commands/db/install.js' import { describeSkipReason, + describeStagedReconciliation, + isPartialRewriteResult, + type PartialRewriteResult, rewriteEncryptedAlterColumns, - type SkippedAlter, } from '@/commands/db/rewrite-migrations.js' import { detectPackageManager, @@ -22,32 +24,6 @@ import { messages } from '@/messages.js' const DEFAULT_MIGRATION_NAME = 'install-eql' const DEFAULT_DRIZZLE_OUT = 'drizzle' -/** - * What {@link rewriteEncryptedAlterColumns} attaches to the error when the - * sweep throws partway through: the files it had already rewritten, and the - * statements it had already flagged. Reported so a partial sweep names the - * work it did rather than looking like it never ran (#786). - */ -interface PartialRewriteResult { - rewritten?: string[] - skipped?: SkippedAlter[] -} - -/** - * The sweep can also fail with a non-`Error` throw — a string, `null`, anything - * — in which case there is no partial result to report. Narrow rather than cast - * so those cases fall through to the plain "could not sweep" message instead of - * crashing on a property read of a non-object. - */ -function isPartialRewriteResult(value: unknown): value is PartialRewriteResult { - if (typeof value !== 'object' || value === null) return false - const partial = value as Record - return ( - (partial.rewritten === undefined || Array.isArray(partial.rewritten)) && - (partial.skipped === undefined || Array.isArray(partial.skipped)) - ) -} - /** Find the most recently generated Drizzle migration matching the name. */ export async function findGeneratedMigration( outDir: string, @@ -288,15 +264,23 @@ async function generateDrizzleEqlMigration( // it again at the closing note (below) — not just inline here. let sweepIncomplete = false try { - const { rewritten, skipped } = await rewriteEncryptedAlterColumns(outDir, { - skip: migrationPath, - }) + const { rewritten, skipped, staged } = await rewriteEncryptedAlterColumns( + outDir, + { skip: migrationPath }, + ) if (rewritten.length > 0) { p.log.info( `Rewrote ${rewritten.length} migration file(s) to add staged encrypted columns while preserving the source columns:`, ) for (const file of rewritten) p.log.step(` - ${file}`) } + // The rewrite repaired SQL only, so schema.ts and the drizzle-kit snapshot + // now disagree with the database — and `drizzle-kit generate` cannot see it + // (#836, item 2). Warn, rather than exit non-zero: the swept SQL is valid + // and additive, and the reconciliation is the user's editorial call. + if (staged.length > 0) { + p.log.warn(describeStagedReconciliation(staged).join('\n')) + } if (skipped.length > 0) { sweepIncomplete = true p.log.warn( @@ -319,6 +303,11 @@ async function generateDrizzleEqlMigration( ) for (const file of partial.rewritten) p.log.step(` - ${file}`) } + // A partial sweep still staged real twins, so the same three-way divergence + // already exists for them. + if (partial.staged && partial.staged.length > 0) { + p.log.warn(describeStagedReconciliation(partial.staged).join('\n')) + } if (partial.skipped && partial.skipped.length > 0) { p.log.warn( `Found ${partial.skipped.length} ALTER-to-encrypted statement(s) the sweep left alone. Review and fix them before running your migrations:`, diff --git a/packages/wizard/src/__tests__/post-agent.test.ts b/packages/wizard/src/__tests__/post-agent.test.ts index 799eda070..ebd4d4f51 100644 --- a/packages/wizard/src/__tests__/post-agent.test.ts +++ b/packages/wizard/src/__tests__/post-agent.test.ts @@ -287,13 +287,62 @@ describe('drizzle migrate prompt after a staged rewrite', () => { // different hat. it('treats an empty error message as a failed sweep, not a clean one', async () => { vi.mocked(sweepMigrationDirs).mockResolvedValueOnce([ - { dir: 'drizzle', rewritten: [], skipped: [], error: '' }, + { dir: 'drizzle', rewritten: [], skipped: [], staged: [], error: '' }, ]) await expect(runDrizzle()).rejects.toThrow('unsafe or unverified SQL') expect(p.confirm).not.toHaveBeenCalled() }) + /** + * #836, item 2. The wizard's agent edited schema.ts to declare the column as + * the encrypted domain — that edit is what made drizzle-kit emit the + * impossible `SET DATA TYPE`. After the add-only sweep the database has both + * columns, while schema.ts and the snapshot still describe only the old one, + * and `drizzle-kit generate` shows nothing because those two agree with each + * other. The wizard must say so before it offers to run the migration. + */ + it('warns that schema.ts and the snapshot diverged after staging a twin', async () => { + const warn = vi.spyOn(p.log, 'warn').mockImplementation(() => {}) + makeDrizzleOut('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', + ) + + await runDrizzle() + + const warnings = warn.mock.calls.map(([m]) => String(m)).join('\n') + expect(warnings).toContain('users:') + expect(warnings).toContain('"email_encrypted" eql_v3_text_search') + expect(warnings).toContain('drizzle-kit generate` will NOT warn you') + expect(warnings).toContain('column already exists') + // Said BEFORE the migrate prompt, so the user decides with it in hand. + expect(p.confirm).toHaveBeenCalled() + warn.mockRestore() + }) + + // Nothing staged means nothing diverged: the notice must not fire, or it sends + // the user editing a schema that is already consistent. + it('does not warn about reconciliation when nothing was staged', async () => { + const warn = vi.spyOn(p.log, 'warn').mockImplementation(() => {}) + makeDrizzleOut('drizzle') + fs.writeFileSync( + path.join(cwd, 'drizzle', '0000_init.sql'), + 'CREATE TABLE "widgets" ("id" integer PRIMARY KEY);\n', + ) + + await runDrizzle() + + const warnings = warn.mock.calls.map(([m]) => String(m)).join('\n') + expect(warnings).not.toContain('drizzle-kit generate` will NOT warn you') + warn.mockRestore() + }) + // The wizard ships scanning drizzle/, migrations/ and src/db/migrations/ and // indexes each SEPARATELY — the per-directory index is the mechanism the // fail-closed rule exists to make safe. Every test above uses only drizzle/, diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index aaecaa957..6a31db3a6 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -4,6 +4,7 @@ import path from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { describeSkipReason, + describeStagedReconciliation, rewriteEncryptedAlterColumns, sweepMigrationDirs, } from '../lib/rewrite-migrations.js' @@ -886,6 +887,18 @@ describe('rewriteEncryptedAlterColumns', () => { }) }) + /** + * #823 closed #811 by removing the blast radius — the rewrite became add-only + * — not by closing the MECHANISM. #836 item 1 closed the mechanism: the index + * now reads dollar-quoted bodies for the encrypted side, so these corpora are + * recognised as already-encrypted instead of being handed an empty twin. + * + * Two expectations here therefore tightened, both toward fail-closed: + * `already-encrypted` replaces `target-exists` on the rename corpora (after + * those renames `email` IS the ciphertext, and `target-exists`'s "review the + * existing encrypted twin" pointed at a column the rename had just consumed), + * and the `DO $$` ADD COLUMN case no longer rewrites at all. + */ describe('issue #811 dollar-quoted DDL regression', () => { const domainChange = 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "eql_v3_text_eq";' @@ -911,11 +924,14 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([]) expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) expect(skipped).toEqual([ - { file: change, statement: domainChange, reason: 'target-exists' }, + { file: change, statement: domainChange, reason: 'already-encrypted' }, ]) }) - it('does not emit destructive SQL when an encrypted ADD COLUMN is inside DO $$', async () => { + // The case #823's own test codified backwards (#836, item 1): the `DO $$` + // body really does leave `email` encrypted, so staging a twin beside the + // ciphertext was wrong. It is now recognised and flagged. + it('flags an encrypted ADD COLUMN inside DO $$ instead of staging a twin', async () => { fs.writeFileSync( path.join(tmpDir, '0000_setup.sql'), [ @@ -930,12 +946,13 @@ describe('rewriteEncryptedAlterColumns', () => { const change = path.join(tmpDir, '0001_change_domain.sql') fs.writeFileSync(change, `${domainChange}\n`) - const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) - expect(rewritten).toEqual([change]) - const updated = fs.readFileSync(change, 'utf-8') - expect(updated).toContain('ADD COLUMN "email_encrypted"') - expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) + expect(rewritten).toEqual([]) + expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'already-encrypted' }, + ]) }) it('does not emit destructive SQL for a rename inside a custom dollar tag', async () => { @@ -959,11 +976,16 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([]) expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) expect(skipped).toEqual([ - { file: change, statement: domainChange, reason: 'target-exists' }, + { file: change, statement: domainChange, reason: 'already-encrypted' }, ]) }) - it('does not emit destructive SQL when an unterminated $$ hides a later encrypted declaration', async () => { + // An unterminated `$$` makes the whole file unparseable, so Postgres never + // ran any of it and nothing after the opener is proven. The index reads that + // remainder for the encrypted side anyway (see `dollarQuotedBodies`), which + // is why the twin is now seen and the statement flagged rather than + // rewritten — the fail-closed way to be wrong about a broken file. + it('flags rather than rewrites when an unterminated $$ hides an encrypted declaration', async () => { fs.writeFileSync( path.join(tmpDir, '0000_setup.sql'), [ @@ -976,15 +998,507 @@ describe('rewriteEncryptedAlterColumns', () => { const change = path.join(tmpDir, '0001_change_domain.sql') fs.writeFileSync(change, `${domainChange}\n`) - const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(fs.readFileSync(change, 'utf-8')).toBe(`${domainChange}\n`) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'target-exists' }, + ]) + }) + }) + + /** + * Issue #836, item 1. `isInsideCommentOrString` skips a dollar-quoted body + * WHOLE — correct for the rewrite pass, wrong for the index pass. DDL inside + * `DO $$ … END $$;` is executed SQL: the column really is encrypted in the + * database. Skipping it meant the column never entered `encrypted`, fell to + * "plaintext by residue", and the sweep added an empty `_encrypted` twin + * beside the real ciphertext — `rewritten` listing the file, `skipped` empty, + * exit code 0. + * + * The index now reads dollar-quoted bodies for the ENCRYPTED side only. The + * `declared` side stays gated: a `DO $$` body is conditional PL/pgSQL, so a + * plaintext declaration inside one may never have run, and over-detecting + * `declared` is the fail-OPEN direction. The rewrite pass is untouched — an + * ALTER inside a dollar body is still inert and still not rewritten. + */ + describe('encrypted DDL inside a dollar-quoted body', () => { + const domainChange = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "eql_v3_text_eq";' + + it('indexes an encrypted CREATE TABLE column inside DO $$', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'DO $$ BEGIN', + ' CREATE TABLE "users" ("email" "public"."eql_v3_text_search");', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'already-encrypted' }, + ]) + }) + + it('carries encryptedness through a RENAME inside DO $$', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'ALTER TABLE "users" ADD COLUMN "email_tmp" "eql_v3_text_search";', + 'DO $$ BEGIN', + ' ALTER TABLE "users" DROP COLUMN "email";', + ' ALTER TABLE "users" RENAME COLUMN "email_tmp" TO "email";', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'already-encrypted' }, + ]) + }) + + /** + * A rename CHAIN that crosses from a dollar-quoted body to a live statement + * inside the SAME file. The encryptedness has to survive both hops. + * + * This was accepted residue on the first cut, justified as unreachable + * because drizzle-kit never emits `RENAME COLUMN` inside `DO $$`. That is + * the wrong reachability test: the corpus #811 actually reported is + * hand-written and does exactly this — a human staging a manual column swap. + * The cost of missing it is the defect this whole change exists to close, an + * empty twin staged beside real ciphertext. + */ + it('carries encryptedness through a rename chain from DO $$ into a live statement', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'ALTER TABLE "users" ADD COLUMN "tmp" "eql_v3_text_search";', + 'DO $$ BEGIN', + ' ALTER TABLE "users" DROP COLUMN "email";', + ' ALTER TABLE "users" RENAME COLUMN "tmp" TO "mid";', + 'END $$;', + 'ALTER TABLE "users" RENAME COLUMN "mid" TO "email";', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'already-encrypted' }, + ]) + }) + + // And the reverse order, live -> dollar, which already worked. Pinned so a + // future reordering of the two rename passes cannot silently break one + // direction while the other keeps passing. + it('carries encryptedness through a rename chain from a live statement into DO $$', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'ALTER TABLE "users" ADD COLUMN "tmp" "eql_v3_text_search";', + 'ALTER TABLE "users" RENAME COLUMN "tmp" TO "mid";', + 'DO $$ BEGIN', + ' ALTER TABLE "users" DROP COLUMN "email";', + ' ALTER TABLE "users" RENAME COLUMN "mid" TO "email";', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'already-encrypted' }, + ]) + }) + + // The staged twin exists in the database but only inside a dollar body, so + // it is `encrypted` without ever being `declared`. Emitting another + // ADD COLUMN for it fails at migrate time with "column already exists". + it('treats an encrypted twin added inside DO $$ as an existing target', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'DO $$ BEGIN', + ' ALTER TABLE "users" ADD COLUMN "email_encrypted" "eql_v3_text_search";', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + const alter = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "eql_v3_text_search";' + fs.writeFileSync(change, `${alter}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: alter, reason: 'target-exists' }, + ]) + }) + + // The fail-OPEN direction, deliberately not taken. A `DO $$` body is + // conditional, so a PLAINTEXT declaration inside one is not proof the + // column exists — it stays undeclared and the statement stays flagged. + it('does not let a plaintext declaration inside DO $$ count as declared', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'DO $$ BEGIN', + ' ALTER TABLE "users" ADD COLUMN "email" text;', + 'END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: change, statement: domainChange, reason: 'source-unknown' }, + ]) + }) + + // Reading dollar bodies must not resurrect INERT ones. A `DO $$` block + // sitting inside a `--` comment or a string literal never runs, so the + // encrypted declaration in it is not evidence of anything. + it('ignores a dollar-quoted body inside a line comment', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + '-- DO $$ BEGIN ALTER TABLE "users" ADD COLUMN "email" "eql_v3_text_search"; END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([change]) - const updated = fs.readFileSync(change, 'utf-8') - expect(updated).toContain('ADD COLUMN "email_encrypted"') - expect(updated).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) + expect(skipped).toEqual([]) + expect(fs.readFileSync(change, 'utf-8')).toContain( + 'ADD COLUMN "email_encrypted"', + ) + }) + + it('ignores a dollar-quoted body inside a string literal', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + `INSERT INTO "audit" ("sql") VALUES ('DO $$ BEGIN ALTER TABLE "users" ADD COLUMN "email" "eql_v3_text_search"; END $$;');`, + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([change]) + expect(skipped).toEqual([]) + }) + + // The rewrite pass keeps treating a dollar body as inert: `renderSafeAlter` + // returns MULTIPLE lines, and splicing them into a PL/pgSQL body would + // rewrite code the sweep cannot reason about. + it('still refuses to rewrite an ALTER that sits inside DO $$', async () => { + const file = path.join(tmpDir, '0000_setup.sql') + const sql = [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'DO $$ BEGIN', + ` ${domainChange}`, + 'END $$;', + '', + ].join('\n') + fs.writeFileSync(file, sql) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) + expect(fs.readFileSync(file, 'utf-8')).toBe(sql) + }) + + // drizzle-kit's own enum idiom. It touches no table, so widening the index + // must not turn every corpus containing one into a wall of flagged + // statements — the reason a blanket "fail closed on any dollar-quoted body" + // was rejected in favour of indexing the encrypted side. + it('leaves the drizzle-kit CREATE TYPE enum idiom rewritable', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_setup.sql'), + [ + 'CREATE TABLE "users" ("email" text NOT NULL);', + 'DO $$ BEGIN CREATE TYPE "public"."role" AS ENUM(\'admin\'); EXCEPTION WHEN duplicate_object THEN null; END $$;', + '', + ].join('\n'), + ) + const change = path.join(tmpDir, '0001_change_domain.sql') + fs.writeFileSync(change, `${domainChange}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([change]) + expect(skipped).toEqual([]) + }) + }) + + /** + * #836, item 2. The sweep repairs SQL and nothing else, so a successful + * rewrite leaves schema.ts, the drizzle-kit snapshot and the database + * three-way divergent — and `drizzle-kit generate` cannot surface it, because + * it diffs schema.ts against the snapshot and those two still agree. The + * rewriter therefore reports exactly what it staged so the caller can name it. + */ + describe('staged reconciliation reporting', () => { + it('names the table, both columns and the domain it staged', async () => { + const create = path.join(tmpDir, '0000_create.sql') + fs.writeFileSync( + create, + 'CREATE TABLE "users" ("email" text NOT NULL);\n', + ) + const filePath = path.join(tmpDir, '0001_alter.sql') + fs.writeFileSync( + filePath, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { staged } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(staged).toEqual([ + { + file: filePath, + schema: undefined, + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ]) + }) + + it('keeps the schema qualifier for a pgSchema() table', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "app"."users" ("email" text NOT NULL);\n', + ) + const filePath = path.join(tmpDir, '0001_alter.sql') + fs.writeFileSync( + filePath, + 'ALTER TABLE "app"."users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { staged } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(staged).toEqual([ + { + file: filePath, + schema: 'app', + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ]) + }) + + // Finer-grained than `rewritten`, which lists FILES: one file can carry + // several ALTERs and each one stages its own twin. + it('records one entry per staged column, not per file', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "users" ("email" text, "phone" text);\n', + ) + const filePath = path.join(tmpDir, '0001_alter.sql') + fs.writeFileSync( + filePath, + [ + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + 'ALTER TABLE "users" ALTER COLUMN "phone" SET DATA TYPE eql_v3_text_eq;', + '', + ].join('\n'), + ) + + const { rewritten, staged } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([filePath]) + expect(staged.map((s) => s.encryptedColumn)).toEqual([ + 'email_encrypted', + 'phone_encrypted', + ]) + expect(staged.map((s) => s.domain)).toEqual([ + 'eql_v3_text_search', + 'eql_v3_text_eq', + ]) + }) + + // A statement the sweep refused to rewrite changed nothing on disk, so + // there is no divergence to reconcile and no notice to print. + it('stages nothing when every statement was skipped', async () => { + fs.writeFileSync( + path.join(tmpDir, '0001_alter.sql'), + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped, staged } = + await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(staged).toEqual([]) + }) + + it('describes the divergence, the silence, and the snapshot trap', async () => { + const lines = describeStagedReconciliation([ + { + file: '/tmp/0001_alter.sql', + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ]).join('\n') + + expect(lines).toContain('"email_encrypted" eql_v3_text_search') + expect(lines).toContain('users:') + // The three things a user cannot discover on their own. + expect(lines).toContain('drizzle-kit generate` will NOT warn you') + expect(lines).toContain('column already exists') + expect(lines).toContain('SUCCEED') + }) + + // Names the migration the twin was staged in, so the user can go read the + // statement rather than hunt the directory for it. + it('names the migration file each twin was staged in', async () => { + const lines = describeStagedReconciliation([ + { + file: '/tmp/drizzle/0001_alter.sql', + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ]).join('\n') + + expect(lines).toContain('/tmp/drizzle/0001_alter.sql') + }) + + /** + * The remediation has to be followed to a working end state, and the + * obvious reading of it does not get there. + * + * After a sweep, `schema.ts` declares the SOURCE column as the encrypted + * domain — that declaration is what made drizzle-kit emit the invalid + * ALTER — and the snapshot agrees with it. So the guidance must say to set + * the source column BACK to plaintext, not to "keep" it. + * + * And the snapshot has never seen the twin, so `drizzle-kit generate` + * ALWAYS emits an `ADD COLUMN` for it — the swept migration already adds + * that column, so applying both fails with "column already exists". That + * is not avoidable by editing the schema more carefully; the generated + * statement has to be removed. Guidance that stops at "run generate" walks + * the user into the very error it warns about. + */ + it('tells the user to revert the source column and drop the regenerated ADD COLUMN', async () => { + const lines = describeStagedReconciliation([ + { + file: '/tmp/0001_alter.sql', + table: 'users', + column: 'email', + encryptedColumn: 'email_encrypted', + domain: 'eql_v3_text_search', + }, + ]).join('\n') + + // Revert, not "keep": the schema currently declares it as the domain. + expect(lines).toMatch(/\bback to its plaintext type\b/i) + expect(lines).not.toContain('keep the source column as its plaintext') + // The step that makes the difference between working and failing. + expect(lines).toMatch(/\b(?:DELETE|delete|remove)\b[^\n]*ADD COLUMN/) + // And it must not blame partial editing for the duplicate. + expect(lines).not.toContain('Do not hand-edit only the schema') }) }) + /** + * `dollarQuotedBodies` must track comment/string state itself. Asking + * `isInsideCommentOrString` about each `$` is the obvious implementation and + * is quadratic, because that predicate rescans from index 0 every call. + * + * This corpus is the shape that makes it bite — thousands of small `$$` + * PL/pgSQL bodies, which is exactly the ~2.6 MB EQL install migration sitting + * in a real drizzle output directory next to the ALTER being swept. So this is + * shipped-command latency, not a microbenchmark: on that corpus the whole + * sweep measures ~0.4 s in one pass and ~8.5 s per-opener. + * + * Sized so the two are unambiguous rather than marginal. Over this ~2.1 MB + * corpus the whole sweep measures ~70 ms in one pass and ~41 s per-opener, so + * the 15 s bound sits ~200x above the linear time and ~3x below the regressed + * one. + * + * That headroom is the answer to the usual objection that wall-clock + * assertions flake on loaded runners: tripping this one needs a machine ~200x + * slower at scanning a string than a developer laptop, where CI is 2-5x. It is + * a coarse order-of-magnitude gate, not a millisecond budget — if it ever does + * go off spuriously, raise the bound rather than delete the test, because the + * regression it guards is shipped-command latency. + */ + it('scans a dollar-quote-heavy corpus in a single pass', async () => { + const bodies = Array.from( + { length: 32_000 }, + (_, n) => + `DO $$ BEGIN PERFORM ${n}; EXCEPTION WHEN others THEN null; END $$;`, + ).join('\n') + fs.writeFileSync(path.join(tmpDir, '0000_install.sql'), bodies) + fs.writeFileSync( + path.join(tmpDir, '0001_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + const alter = path.join(tmpDir, '0002_alter.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const started = Date.now() + const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + const elapsed = Date.now() - started + + // Still correct: the dollar bodies declare nothing, so the ALTER rewrites. + expect(rewritten).toEqual([alter]) + expect(elapsed).toBeLessThan(15_000) + }, 180_000) + // A domain change on a column that is ALREADY encrypted needs staged // re-encryption; there is no plaintext source to backfill from. describe('columns that are already encrypted', () => { @@ -1610,12 +2124,22 @@ describe('rewriteEncryptedAlterColumns', () => { await rewriteEncryptedAlterColumns(tmpDir) throw new Error('expected rewriteEncryptedAlterColumns to throw') } catch (error) { - const partial = error as { rewritten?: string[]; skipped?: unknown[] } + const partial = error as { + rewritten?: string[] + skipped?: unknown[] + staged?: { column: string }[] + } expect(partial.rewritten).toEqual([first]) expect(partial.skipped).toEqual([]) expect(fs.readFileSync(first, 'utf-8')).toContain( 'ADD COLUMN "email_encrypted"', ) + // `staged` drives a notice telling the user the database now HAS these + // columns, so it must only list twins that reached disk. `name`'s twin was + // staged in memory during the string replace and then lost when the write + // threw — reporting it would send the user reconciling a column that + // exists nowhere. + expect(partial.staged?.map((s) => s.column)).toEqual(['email']) } finally { if (fs.statSync(failing).isDirectory()) fs.rmdirSync(failing) } @@ -1916,6 +2440,10 @@ describe('sweepMigrationDirs', () => { beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'wizard-sweep-')) + // This describe shares the module-level writeFile spy with the one above, + // so restore the real implementation rather than inherit whatever the last + // test left behind. + fsPromisesWrite.spy.mockImplementation(fsPromisesWrite.real) }) afterEach(() => { @@ -1992,6 +2520,73 @@ describe('sweepMigrationDirs', () => { expect(results[1].rewritten).toEqual([path.join(abs, '0002_alter.sql')]) }) + /** + * #836, item 3. The catch block used to read `err.rewritten` off an unchecked + * `err as Partial`. For a non-object throw that property + * read raises a `TypeError` INSIDE the catch, so `results.push` never runs and + * the throw escapes `sweepMigrationDirs` — the per-directory fail-closed + * report the catch exists to produce simply does not happen, and the later + * directories are never swept either. + * + * `fs/promises` does not throw non-Errors, so this is reached by forcing it. + * The CLI path was hardened against exactly this; the wizard was not. + */ + it('still reports a directory whose sweep throws a non-object', async () => { + const broken = seedDir('drizzle', '0001_alter.sql') + const abs = seedDir('migrations', '0002_alter.sql') + fsPromisesWrite.spy.mockImplementation(async (file) => { + if (String(file).startsWith(broken)) throw null + return fsPromisesWrite.real(file, '') + }) + + const results = await sweepMigrationDirs(tmpDir, ['drizzle', 'migrations']) + + // Reported, not escaped: the directory appears with an error… + expect(results[0].dir).toBe('drizzle') + expect(results[0].error).toBe('null') + expect(results[0].rewritten).toEqual([]) + expect(results[0].skipped).toEqual([]) + expect(results[0].staged).toEqual([]) + // …and the sweep carried on to the remaining candidates. + expect(results[1].dir).toBe('migrations') + expect(results[1].rewritten).toEqual([path.join(abs, '0002_alter.sql')]) + }) + + /** + * A directory whose sweep threw part way through has real twins on disk, and + * `sweepMigrationDirs` has to carry them out so the caller can print the + * reconciliation notice for them. `partial.staged` was threaded but never + * asserted. + */ + it('carries staged twins out of a directory whose sweep threw', async () => { + const abs = seedDir('drizzle') + fs.writeFileSync( + path.join(abs, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text, "name" text);\n', + ) + const first = path.join(abs, '0001_email.sql') + const failing = path.join(abs, '0002_name.sql') + fs.writeFileSync( + first, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + fs.writeFileSync( + failing, + 'ALTER TABLE "users" ALTER COLUMN "name" SET DATA TYPE eql_v3_text_search;\n', + ) + fsPromisesWrite.spy.mockImplementation(async (file, data, options) => { + if (String(file) === failing) throw new Error('EISDIR') + return fsPromisesWrite.real(file, data, options) + }) + + const results = await sweepMigrationDirs(tmpDir, ['drizzle']) + + expect(results[0].error).toBeDefined() + // Only the twin that reached disk — `name`'s write threw. + expect(results[0].staged.map((s) => s.column)).toEqual(['email']) + expect(results[0].staged[0].encryptedColumn).toBe('email_encrypted') + }) + it('reports near-misses per directory', async () => { const abs = seedDir( 'drizzle', diff --git a/packages/wizard/src/lib/post-agent.ts b/packages/wizard/src/lib/post-agent.ts index bccfd6997..30201b77d 100644 --- a/packages/wizard/src/lib/post-agent.ts +++ b/packages/wizard/src/lib/post-agent.ts @@ -8,7 +8,12 @@ import { execSync } from 'node:child_process' import * as p from '@clack/prompts' import type { GatheredContext } from './gather.js' -import { describeSkipReason, sweepMigrationDirs } from './rewrite-migrations.js' +import { + describeSkipReason, + describeStagedReconciliation, + type StagedColumn, + sweepMigrationDirs, +} from './rewrite-migrations.js' import type { DetectedPackageManager, Integration } from './types.js' interface PostAgentOptions { @@ -69,15 +74,29 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { // text/numeric to an EQL domain). Covers the EQL v3 family the wizard now // scaffolds, and legacy eql_v2_encrypted. CIP-2991 + CIP-2994 + #693. const sweep = await rewriteEncryptedMigrations(cwd) - const staged = sweep.rewritten > 0 + const didStage = sweep.rewritten > 0 const skipped = sweep.skipped > 0 const unverified = sweep.failedDirs.length > 0 - if (staged) { + if (didStage) { p.log.info( `Rewrote ${sweep.rewritten} migration file(s) in the drizzle output to add staged encrypted columns while preserving the source columns.`, ) } + // The rewrite repaired SQL only: the agent's schema edit still declares the + // SOURCE column as the encrypted domain, and drizzle-kit's snapshot agrees + // with it, so `drizzle-kit generate` sees no diff and says nothing + // (#836, item 2). Name the divergence while the user is still here. + // + // Gated on `staged`, not on `didStage`, to match the CLI. The two are + // equivalent today — a rewritten file always yields at least one staged + // column, and both are recorded together only after the write succeeds — but + // this notice describes the staged columns, so it should be driven by + // whether there are any rather than by a file count that happens to track + // them. + if (sweep.staged.length > 0) { + p.log.warn(describeStagedReconciliation(sweep.staged).join('\n')) + } if (skipped || unverified) { throw new Error( `The ALTER COLUMN sweep found unsafe or unverified SQL. The generated migration remains in ${cwd}, but review the sibling migrations before running drizzle-kit migrate.`, @@ -85,7 +104,7 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { } const shouldMigrate = await p.confirm({ - message: staged + message: didStage ? `Run the migration now? (${runner} drizzle-kit migrate) — the generated migration adds staged encrypted columns and preserves the source column for the later backfill and application switch` : `Run the migration now? (${runner} drizzle-kit migrate)`, initialValue: true, @@ -131,14 +150,30 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { async function rewriteEncryptedMigrations(cwd: string): Promise<{ rewritten: number skipped: number + staged: StagedColumn[] failedDirs: string[] }> { const results = await sweepMigrationDirs(cwd, DRIZZLE_OUT_DIRS) - const totals = { rewritten: 0, skipped: 0, failedDirs: [] as string[] } + const totals = { + rewritten: 0, + skipped: 0, + staged: [] as StagedColumn[], + failedDirs: [] as string[], + } - for (const { dir, rewritten, skipped, error, notDrizzleOutput } of results) { + for (const { + dir, + rewritten, + skipped, + staged, + error, + notDrizzleOutput, + } of results) { totals.rewritten += rewritten.length totals.skipped += skipped.length + // Accumulated across every directory, including one whose sweep later + // threw: those twins are already on disk and already divergent. + totals.staged.push(...staged) // 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 diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 7a04f0e36..4d8e673aa 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -278,6 +278,102 @@ function dollarQuoteDelimiter(sql: string, open: number): string | undefined { /** 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 +/** + * The INNER text of every dollar-quoted body (`$$ … $$`, `$fn$ … $fn$`) whose + * opener sits at a live position — one that {@link isInsideCommentOrString} + * does not already call inert. + * + * **Why this exists (#836, item 1):** {@link isInsideCommentOrString} skips a + * dollar-quoted body WHOLE, and that is right for the rewrite pass — the body + * is PL/pgSQL, {@link renderSafeAlter} returns multiple lines, and splicing + * them into code this sweep cannot reason about is not a repair. But it is + * wrong for {@link indexColumnDeclarations}: DDL inside `DO $$ … END $$;` is + * EXECUTED. An encrypted `ADD COLUMN` there means the column really does hold + * ciphertext, yet it never entered `encrypted`, so the column fell to + * "plaintext by residue" and the sweep added an empty `_encrypted` twin + * beside the real ciphertext — reported as a successful rewrite (#811). + * + * So the index reads these bodies, and only for the ENCRYPTED side. That is + * the same asymmetry the per-column `CREATE TABLE` loops already rely on: + * over-detecting "encrypted" costs a flagged statement, while over-detecting + * "declared" is what puts a column back on the fail-open path. A `DO $$` body + * is CONDITIONAL PL/pgSQL — an `IF … THEN` arm, an `EXCEPTION` handler — so a + * plaintext declaration inside one is not proof the column exists, and + * {@link indexEncryptedDeclarations} deliberately does not record one. + * + * An INERT body stays inert: a `DO $$ … $$;` inside a `--` comment or quoted + * inside an `INSERT … VALUES ('…')` never ran, and its DDL is evidence of + * nothing. + * + * An UNTERMINATED body yields its remainder to the end of the file. That is + * deliberately the opposite of what {@link isInsideCommentOrString} does with + * one, and the two are not in conflict: there, "inert to the end of the file" + * protects quote PARITY for the rewrite; here the goal is COVERAGE, and a file + * whose dollar quote never closes is one Postgres rejects outright — so + * everything it says is unproven, and over-detecting "encrypted" (a flagged + * statement) is the safe way to be wrong about it. + * + * **This tracks inertness itself rather than calling + * {@link isInsideCommentOrString} per opener, and that is a hard requirement, + * not a tidy-up.** That predicate rescans from index 0 on every call, so asking + * it about each `$` makes this quadratic — and the corpus this runs over + * routinely includes the ~2.6 MB EQL install migration, which is itself + * thousands of `$$` PL/pgSQL bodies. Measured on that corpus: ~370 ms as a + * single pass, ~8.5 s per-opener. Same token rules, same helpers, one traversal. + */ +function* dollarQuotedBodies(sql: string): Generator { + let i = 0 + while (i < sql.length) { + if (sql.startsWith('--', i)) { + const eol = sql.indexOf('\n', i) + // Runs to EOF, so nothing after it is live. + if (eol === -1) return + i = eol + 1 + } else if (sql.startsWith('/*', i)) { + // Nestable, exactly as in `isInsideCommentOrString`: stopping at the first + // `*/` would let the text after a nested close read as live SQL. + let depth = 1 + let j = i + 2 + while (j < sql.length && depth > 0) { + if (sql.startsWith('/*', j)) { + depth += 1 + j += 2 + } else if (sql.startsWith('*/', j)) { + depth -= 1 + j += 2 + } else { + j += 1 + } + } + i = j + } else if (sql[i] === '$') { + const delimiter = dollarQuoteDelimiter(sql, i) + if (delimiter === undefined) { + i += 1 + continue + } + const bodyStart = i + delimiter.length + const close = sql.indexOf(delimiter, bodyStart) + if (close === -1) { + yield sql.slice(bodyStart) + return + } + // The body is opaque: resuming past its close is what stops a `'` or `--` + // inside it from being read as SQL. + yield sql.slice(bodyStart, close) + i = close + delimiter.length + } else if (sql[i] === '"') { + // Before the `'` branch, so an apostrophe inside `"o'brien"` cannot open a + // phantom literal. Unterminated returns `sql.length` and ends the walk. + i = endOfQuoted(sql, i, '"') + } else if (sql[i] === "'") { + i = endOfQuoted(sql, i, "'", isEscapeStringOpener(sql, i)) + } else { + i += 1 + } + } +} + /** * Whether the `'` at `open` starts an `E''` escape-string, in which a backslash * escapes the following character. Plain literals give backslash no special @@ -566,64 +662,174 @@ function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { const declared = new Set() for (const sql of contents) { - 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 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 - // string literal was already skipped above. - // - // Asymmetric on purpose. ENCRYPTED does NOT re-check comments here: a - // commented-out encrypted column inside an otherwise live CREATE TABLE - // still counts, exactly as it did before this sweep learned to declare - // anything — over-detecting "encrypted" only costs a flagged statement. - // DECLARED DOES re-check: a commented-out plaintext line never ran, so - // it must not count as a declaration — over-detecting "declared" is - // what would put a truly-undeclared, possibly already-encrypted column - // back on the fail-open path. - for (const column of body.matchAll(CREATE_TABLE_ENCRYPTED_COLUMN_RE)) { - encrypted.add(columnKey(table, column[1], schema)) - } + // Materialised once: both passes below walk the same bodies, and the + // generator re-scans the file each time it is iterated. + const opaque = [...dollarQuotedBodies(sql)] - for (const column of body.matchAll(DECLARED_COLUMN_RE)) { - if (isInsideCommentOrString(body, column.index)) continue - declared.add(columnKey(table, column[1], schema)) - } + indexDeclarations(sql, encrypted, declared) + for (const body of opaque) indexEncryptedDeclarations(body, encrypted) + + // Renames run after EVERY declaration above, live and dollar-quoted: a + // rename carries the column's type with it, so the ADD it refers to has to + // be indexed first. + // + // Iterated to a FIXPOINT because a rename chain can alternate between live + // and dollar-quoted statements in either order within one file, and a single + // ordered pair of passes only follows one of those orders. Running the live + // pass first missed `ADD tmp ` → `RENAME tmp TO mid` inside `DO $$` + // → live `RENAME mid TO email`, leaving `email` looking like plaintext and + // staging an empty twin beside its ciphertext — the exact defect this change + // exists to close. Hand-written manual column swaps do this; the corpus #811 + // reported is one. + // + // Terminates: both sets only ever grow, and are bounded by the number of + // distinct column keys in the corpus. Converges in one extra iteration past + // the longest chain, so a corpus with no renames costs a second scan that + // finds nothing. + let indexed = -1 + while (indexed !== encrypted.size + declared.size) { + indexed = encrypted.size + declared.size + indexRenames(sql, encrypted, declared) + for (const body of opaque) indexEncryptedRenames(body, encrypted) } + } - for (const added of sql.matchAll(ADD_ENCRYPTED_COLUMN_RE)) { - if (isInsideCommentOrString(sql, added.index)) continue - const { schema, table } = tableOf(added[1], added[2]) - encrypted.add(columnKey(table, added[3], schema)) + return { encrypted, declared } +} + +/** Declarations at LIVE positions — both sides of the index. */ +function indexDeclarations( + sql: string, + encrypted: Set, + declared: Set, +): void { + 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 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 + // string literal was already skipped above. + // + // Asymmetric on purpose. ENCRYPTED does NOT re-check comments here: a + // commented-out encrypted column inside an otherwise live CREATE TABLE + // still counts, exactly as it did before this sweep learned to declare + // anything — over-detecting "encrypted" only costs a flagged statement. + // DECLARED DOES re-check: a commented-out plaintext line never ran, so + // it must not count as a declaration — over-detecting "declared" is + // what would put a truly-undeclared, possibly already-encrypted column + // back on the fail-open path. + for (const column of body.matchAll(CREATE_TABLE_ENCRYPTED_COLUMN_RE)) { + encrypted.add(columnKey(table, column[1], schema)) } - for (const added of sql.matchAll(ADD_COLUMN_RE)) { - if (isInsideCommentOrString(sql, added.index)) continue - const { schema, table } = tableOf(added[1], added[2]) - declared.add(columnKey(table, added[3], schema)) + for (const column of body.matchAll(DECLARED_COLUMN_RE)) { + if (isInsideCommentOrString(body, column.index)) continue + declared.add(columnKey(table, column[1], schema)) } + } + + for (const added of sql.matchAll(ADD_ENCRYPTED_COLUMN_RE)) { + if (isInsideCommentOrString(sql, added.index)) continue + const { schema, table } = tableOf(added[1], added[2]) + encrypted.add(columnKey(table, added[3], schema)) + } + + for (const added of sql.matchAll(ADD_COLUMN_RE)) { + if (isInsideCommentOrString(sql, added.index)) continue + const { schema, table } = tableOf(added[1], added[2]) + declared.add(columnKey(table, added[3], schema)) + } +} - // A rename carries the column's type with it — and `__cipherstash_tmp` - // renamed onto the real name is exactly what a previous sweep of this very - // directory emitted. Run after ADD so that tmp column is already indexed. - for (const renamed of sql.matchAll(RENAME_COLUMN_RE)) { - if (isInsideCommentOrString(sql, renamed.index)) continue - const { schema, table } = tableOf(renamed[1], renamed[2]) - const from = columnKey(table, renamed[3], schema) - const to = columnKey(table, renamed[4], schema) - if (encrypted.has(from)) encrypted.add(to) - if (declared.has(from)) declared.add(to) +/** + * The ENCRYPTED side only, over one {@link dollarQuotedBodies} body. + * + * `declared` is deliberately untouched — see {@link dollarQuotedBodies} for why + * a conditional PL/pgSQL body may not prove a plaintext column exists. The + * consequence is the safe one: a column whose ONLY declaration sits inside a + * dollar body stays `source-unknown` and is flagged rather than rewritten. + * + * Comments and literals INSIDE the body are not re-checked, matching the live + * encrypted `CREATE TABLE` loop: a commented-out encrypted column still counts, + * because over-detecting "encrypted" only ever costs a flagged statement. + */ +function indexEncryptedDeclarations( + body: string, + encrypted: Set, +): void { + for (const created of body.matchAll(CREATE_TABLE_HEAD_RE)) { + const { schema, table } = tableOf(created[1], created[2]) + const bodyStart = created.index + created[0].length + const bodyEnd = endOfCreateTableBody(body, bodyStart) + if (bodyEnd === undefined) continue + for (const column of body + .slice(bodyStart, bodyEnd) + .matchAll(CREATE_TABLE_ENCRYPTED_COLUMN_RE)) { + encrypted.add(columnKey(table, column[1], schema)) } } - return { encrypted, declared } + for (const added of body.matchAll(ADD_ENCRYPTED_COLUMN_RE)) { + const { schema, table } = tableOf(added[1], added[2]) + encrypted.add(columnKey(table, added[3], schema)) + } +} + +/** + * Renames at LIVE positions — both sides of the index. + * + * `__cipherstash_tmp` renamed onto the real name is exactly what a + * previous sweep of this very directory emitted, so a rename has to carry the + * source column's type onto its new name. + */ +function indexRenames( + sql: string, + encrypted: Set, + declared: Set, +): void { + for (const renamed of sql.matchAll(RENAME_COLUMN_RE)) { + if (isInsideCommentOrString(sql, renamed.index)) continue + const { schema, table } = tableOf(renamed[1], renamed[2]) + const from = columnKey(table, renamed[3], schema) + const to = columnKey(table, renamed[4], schema) + if (encrypted.has(from)) encrypted.add(to) + if (declared.has(from)) declared.add(to) + } +} + +/** + * Renames inside one {@link dollarQuotedBodies} body — ENCRYPTED side only. + * + * This is the shape #811 actually reported: `ADD COLUMN "email_encrypted" + * ` live, then `RENAME COLUMN "email_encrypted" TO "email"` inside + * `DO $$ … END $$;`. After that block `email` holds the ciphertext, so an + * `ALTER COLUMN "email" SET DATA TYPE …` needs staged RE-encryption, not a + * second twin. + * + * `declared` is not propagated, for the same reason + * {@link indexEncryptedDeclarations} does not record one: the rename may sit in + * a branch that never ran, and inventing a declaration from it is the + * fail-open direction. + * + * Chains that alternate between live and dollar-quoted statements are followed + * in EITHER order: {@link indexColumnDeclarations} iterates this pass and the + * live one to a fixpoint, so which runs first stops mattering. Crossing files + * works too, since files are indexed in sorted order. + */ +function indexEncryptedRenames(body: string, encrypted: Set): void { + for (const renamed of body.matchAll(RENAME_COLUMN_RE)) { + const { schema, table } = tableOf(renamed[1], renamed[2]) + if (encrypted.has(columnKey(table, renamed[3], schema))) { + encrypted.add(columnKey(table, renamed[4], schema)) + } + } } /** Why a recognised ALTER-to-encrypted statement was left alone. */ @@ -666,19 +872,149 @@ export function describeSkipReason(reason: SkipReason): string { } } +/** + * An encrypted twin this sweep added, named precisely enough for the caller to + * tell the user which ORM artefacts now disagree (#836, item 2). + * + * The rewrite is add-only, so after it the DATABASE has both `` (still + * its original plaintext type) and `` (the domain). Nothing + * updates the other two artefacts: `schema.ts` still declares `` AS the + * encrypted domain — that declaration is what made drizzle-kit emit the + * impossible `SET DATA TYPE` in the first place — and `meta/*_snapshot.json` + * still records the same thing. Neither knows `` exists. + */ +export interface StagedColumn { + /** Absolute path of the migration file the staged ADD COLUMN was written to. */ + file: string + /** Schema qualifier, when the table carried one (a `pgSchema()` table). */ + schema?: string + /** Table the column belongs to, without quotes. */ + table: string + /** The preserved source column, still its original plaintext type. */ + column: string + /** The twin that was added — conventionally `_encrypted`. */ + encryptedColumn: string + /** Bare EQL domain the twin was given, e.g. `eql_v3_text_search`. */ + domain: string +} + +/** + * The reconciliation the user must do by hand after a sweep staged twins, as + * lines ready to print (#836, item 2). + * + * **Why this has to be said out loud.** The sweep repairs SQL and nothing else, + * so a successful rewrite leaves three artefacts disagreeing: + * + * - the database gets `email text` plus `email_encrypted eql_v3_text_search` + * - `schema.ts` still declares `email` as the encrypted domain + * - `meta/*_snapshot.json` still records `email` as the encrypted domain + * - neither `schema.ts` nor the snapshot knows `email_encrypted` exists + * + * **`drizzle-kit generate` gives no signal.** It diffs `schema.ts` against the + * snapshot; it never reads `.sql` and never introspects the database. Both its + * inputs still agree, so the diff is empty and it emits nothing. It cannot + * propose a column that appears in neither input — that is `push`, which does + * introspect. So the divergence is invisible to the ORM's own tooling, and every + * consequence through it is silent: reads of `users.email` push plaintext into a + * `customType.fromDriver` expecting an EQL envelope, writes push an envelope + * into a `text` column and SUCCEED, and `email_encrypted` is unreachable because + * it is in no Drizzle schema. + * + * It only fails loudly much later: correcting `schema.ts` by hand makes + * `generate` diff against the stale snapshot and emit a duplicate ADD COLUMN, + * which errors at migrate time with "column already exists". + * + * **Why guidance rather than an automatic edit.** Editing `schema.ts` alone + * CREATES that duplicate-column failure — a correct automatic fix would have to + * rewrite drizzle-kit's snapshot in lockstep, in an internal format that is + * version-coupled and not ours. So the sweep names the divergence precisely and + * leaves the edit to the user, who is the one who knows whether the application + * is ready to read the twin. + */ +export function describeStagedReconciliation( + staged: readonly StagedColumn[], +): string[] { + const lines = [ + 'The sweep repaired SQL only. Your Drizzle schema and drizzle-kit snapshot still describe the OLD shape, so reconcile them before the application reads these columns:', + ] + for (const { + file, + schema, + table, + column, + encryptedColumn, + domain, + } of staged) { + const ref = schema ? `${schema}.${table}` : table + lines.push( + ` - ${ref}: the database now has "${column}" (unchanged, still plaintext) and "${encryptedColumn}" ${domain} (staged in ${file}), but your schema declares "${column}" as ${domain} and knows nothing about "${encryptedColumn}".`, + ) + } + lines.push( + 'Until you reconcile it: reads of the source column hand plaintext to a decrypt path expecting an EQL envelope, writes store an EQL envelope in a plaintext column and SUCCEED, and the new encrypted column is unreachable through the ORM.', + '`drizzle-kit generate` will NOT warn you — it diffs your schema against its snapshot and reads neither the .sql nor the database, and those two still agree.', + 'To reconcile, first fix your Drizzle schema: declare each encrypted column above under its own name, and set each source column BACK to its plaintext type — it is currently declared as the encrypted domain, and that declaration is what made drizzle-kit emit the invalid ALTER in the first place.', + 'Then run `drizzle-kit generate` to advance the snapshot, and DELETE the regenerated `ADD COLUMN` statement for each encrypted column from the migration it writes. The snapshot has never seen those columns, so `generate` always emits an ADD COLUMN for them — and the migration above already adds them, so applying both fails with "column already exists". Removing the statement keeps the snapshot advance, which is the whole point of the step. (`ADD COLUMN IF NOT EXISTS` works too.) Anything else `generate` emits, such as a no-op SET DATA TYPE on the source column, can stay.', + 'Then `drizzle-kit migrate`, and run the staged `stash encrypt` lifecycle to backfill before switching reads across. If you have NOT yet applied the migration above, you can instead revert your schema, remove that migration (its .sql, its meta/*_snapshot.json, and its meta/_journal.json entry) and start from the documented staged rollout, which generates the ADD COLUMN itself and leaves nothing to delete.', + ) + return lines +} + /** Outcome of a sweep: the files rewritten, and near-misses left for review. */ export interface RewriteResult { /** Absolute paths of files whose unsafe ALTER COLUMN(s) were rewritten. */ rewritten: string[] /** Near-miss statements the strict matcher passed over — flag, don't guess. */ skipped: SkippedAlter[] + /** + * Every encrypted twin staged, for the caller's reconciliation notice. One + * entry per rewritten statement, so this is finer-grained than `rewritten` + * (which lists files, and a file can carry several ALTERs). + */ + staged: StagedColumn[] } -interface RewriteSweepError extends Error { +/** + * What {@link rewriteEncryptedAlterColumns} attaches to the error when the + * sweep throws partway through: the files it had already rewritten, and the + * statements it had already flagged. Reported so a partial sweep names the work + * it did rather than looking like it never ran (#786). + */ +export interface PartialRewriteResult { rewritten?: string[] skipped?: SkippedAlter[] + staged?: StagedColumn[] } +/** + * Narrow a caught value to a {@link PartialRewriteResult}. + * + * The sweep can also fail with a non-`Error` throw — a string, `null`, anything + * — in which case there is no partial result to report. **Narrow rather than + * cast:** on `null` or `undefined` a property read raises a `TypeError` inside + * the very `catch` block that exists to report the failure, so the report never + * happens and the throw escapes the handler entirely. That defeats the + * fail-closed reporting outright, which is worse than the missing detail. + * + * Exported because both callers need it and the two copies of this file must + * stay identical: the wizard's `sweepMigrationDirs` uses it to build a + * per-directory error result, and the CLI's `eql migration --drizzle` uses it to + * report a partial sweep (#836, item 3). + */ +export function isPartialRewriteResult( + value: unknown, +): value is PartialRewriteResult { + if (typeof value !== 'object' || value === null) return false + const partial = value as Record + return ( + (partial.rewritten === undefined || Array.isArray(partial.rewritten)) && + (partial.skipped === undefined || Array.isArray(partial.skipped)) && + (partial.staged === undefined || Array.isArray(partial.staged)) + ) +} + +interface RewriteSweepError extends Error, PartialRewriteResult {} + /** * Replace in-place `ALTER COLUMN ... SET DATA TYPE ` * statements with a staged encrypted-column addition. @@ -726,6 +1062,7 @@ export async function rewriteEncryptedAlterColumns( ) const rewritten: string[] = [] const skipped: SkippedAlter[] = [] + const staged: StagedColumn[] = [] const seen = new Set() const stagedTargets = new Set() @@ -760,6 +1097,14 @@ export async function rewriteEncryptedAlterColumns( // Reset the regex's lastIndex — it's stateful on /g ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 + // Buffered per file, then committed to `staged` only once this file's + // write has SUCCEEDED. The entries are produced inside `.replace()`, which + // runs before the write — and `staged` drives a notice telling the user the + // database now HAS these columns. Pushing straight to `staged` reported + // twins that never reached disk when a later write threw, sending the user + // to reconcile a column that exists nowhere. + const fileStaged: StagedColumn[] = [] + const updated = original.replace( ALTER_COLUMN_TO_ENCRYPTED_RE, ( @@ -790,8 +1135,18 @@ export async function rewriteEncryptedAlterColumns( return match } + // `encryptedColumns` as well as `declaredColumns`: the two sets are + // not nested. A twin added inside a dollar-quoted body — or a + // commented-out encrypted column inside a live CREATE TABLE — is + // `encrypted` without ever being `declared`, and emitting a second + // ADD COLUMN for it fails at migrate time with "column already + // exists". Over-detecting here costs a flagged statement. const target = columnKey(table, `${column}_encrypted`, schema) - if (declaredColumns.has(target) || stagedTargets.has(target)) { + if ( + declaredColumns.has(target) || + encryptedColumns.has(target) || + stagedTargets.has(target) + ) { skip(filePath, match.trim(), 'target-exists') return match } @@ -801,7 +1156,18 @@ export async function rewriteEncryptedAlterColumns( // but leave the statement alone rather than emit a broken rewrite. if (!domain) return match + // `stagedTargets` is the in-run duplicate guard and is deliberately + // NOT rolled back with the buffer: if the write fails, treating the + // target as taken keeps a later file from staging it again. stagedTargets.add(target) + fileStaged.push({ + file: filePath, + schema, + table, + column, + encryptedColumn: `${column}_encrypted`, + domain, + }) return renderSafeAlter(table, column, domain, schema) }, ) @@ -809,6 +1175,7 @@ export async function rewriteEncryptedAlterColumns( if (updated !== original) { await writeFile(filePath, updated, 'utf-8') rewritten.push(filePath) + staged.push(...fileStaged) } // Broad secondary scan on the POST-rewrite content: anything still carrying @@ -836,11 +1203,12 @@ export async function rewriteEncryptedAlterColumns( const partial = error as RewriteSweepError partial.rewritten = rewritten partial.skipped = skipped + partial.staged = staged } throw error } - return { rewritten, skipped } + return { rewritten, skipped, staged } } // #region wizard-only — deliberately has no counterpart in @@ -932,6 +1300,7 @@ export async function sweepMigrationDirs( dir, rewritten: [], skipped: [], + staged: [], notDrizzleOutput: true, }) } @@ -939,15 +1308,21 @@ export async function sweepMigrationDirs( } try { - const { rewritten, skipped } = await rewriteEncryptedAlterColumns(abs) - results.push({ dir, rewritten, skipped }) + const { rewritten, skipped, staged } = + await rewriteEncryptedAlterColumns(abs) + results.push({ dir, rewritten, skipped, staged }) } catch (err) { const message = err instanceof Error ? err.message : String(err) - const partial = err as Partial + // Narrowed, never cast: on `throw null` a property read here would raise + // a TypeError INSIDE this catch, so `results.push` never runs and the + // throw escapes `sweepMigrationDirs` — losing the per-directory + // fail-closed report this block exists to produce (#836, item 3). + const partial = isPartialRewriteResult(err) ? err : {} results.push({ dir, rewritten: partial.rewritten ?? [], skipped: partial.skipped ?? [], + staged: partial.staged ?? [], error: message, }) } diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 26f982302..a8c7d6db8 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -378,7 +378,9 @@ Pass exactly one of `--drizzle` / `--prisma`. The generated migration also insta After writing the migration, `--drizzle` sweeps the output directory for sibling migrations containing an in-place `ALTER COLUMN … SET DATA TYPE ` — drizzle-kit emits these when you change a plaintext column to an encrypted one, and Postgres rejects them (there is no cast from `text`/`numeric` to an EQL type). Each is rewritten into a staged `ADD COLUMN` for the encrypted twin, while preserving the source column, and the rewritten files are listed. The rewrite never emits `DROP COLUMN` or `RENAME COLUMN`. If the sweep cannot prove a column's source type, finds that the encrypted twin already exists, or encounters another unsafe form, it leaves that statement untouched and the command exits non-zero so you review the migration directory before running `drizzle-kit migrate`. Populated plaintext tables then take the staged EQL v3 rollout from there: dual-write, backfill, switch the application to the encrypted column by name, and drop plaintext only after verification. -The sweep is fail-closed: it rewrites a statement only when the same directory also contains the `CREATE TABLE` or `ADD COLUMN` that declared the column, and the column is not already encrypted **in the corpus**. That is a guarantee about what the migration files say, not about the live database — the sweep never queries the database, so a column that has drifted from its migration history (altered by hand via psql or the Supabase dashboard, say) can already hold ciphertext while the corpus still describes it as plaintext. A statement it cannot place — the declaration lives in another migration directory, or the history was squashed — is listed as needing review rather than rewritten, and the command exits non-zero if any such statement remains. Check the column's actual type in the database before applying a flagged or corpus-cleared rewrite by hand. +The sweep is fail-closed: it rewrites a statement only when the same directory also contains the `CREATE TABLE` or `ADD COLUMN` that declared the column, and the column is not already encrypted **in the corpus**. Declarations inside a `DO $$ … END $$;` block count toward "already encrypted" — that DDL really executes — but a *plaintext* declaration inside one does not count as declaring the column, since the block may be conditional. That is a guarantee about what the migration files say, not about the live database — the sweep never queries the database, so a column that has drifted from its migration history (altered by hand via psql or the Supabase dashboard, or changed by dynamic `EXECUTE` SQL the sweep cannot read) can already hold ciphertext while the corpus still describes it as plaintext. A statement it cannot place — the declaration lives in another migration directory, or the history was squashed — is listed as needing review rather than rewritten, and the command exits non-zero if any such statement remains. Check the column's actual type in the database before applying a flagged or corpus-cleared rewrite by hand. + +**After a successful sweep, reconcile your ORM schema.** The rewrite repairs SQL only, so the database ends up with both `email` (unchanged, still plaintext) and `email_encrypted`, while your `schema.ts` and drizzle-kit's `meta/*_snapshot.json` both still declare `email` as the encrypted domain and know nothing about the twin. `drizzle-kit generate` will **not** warn you: it diffs your schema against its snapshot, reads neither the `.sql` nor the database, and those two still agree. The command prints the divergence per column, naming the table, both columns and the domain. Until you reconcile it, reads of the source column hand plaintext to a decrypt path expecting an EQL envelope, writes store an EQL envelope in a plaintext column and *succeed*, and the new encrypted column is unreachable through the ORM. See `skills/stash-drizzle` for the reconciliation, including the step that is easy to miss: because the snapshot has never seen the encrypted twin, `drizzle-kit generate` **always** emits an `ADD COLUMN` for it, and the swept migration already adds that column — so the regenerated statement has to be deleted (or made `IF NOT EXISTS`) or migrate fails with `column already exists`. #### `eql upgrade` diff --git a/skills/stash-drizzle/SKILL.md b/skills/stash-drizzle/SKILL.md index 63d1c3ba8..bf0740424 100644 --- a/skills/stash-drizzle/SKILL.md +++ b/skills/stash-drizzle/SKILL.md @@ -61,7 +61,40 @@ stash eql migration --drizzle --supabase # also grants eql_v3 to anon/authenti The generated migration also installs the `cs_migrations` tracking schema, so a single `drizzle-kit migrate` covers everything `stash encrypt …` needs — no out-of-band `stash eql install`. EQL v3 ships one SQL bundle for every target including Supabase; `--supabase` only adds the PostgREST/RLS role grants (harmless when you connect directly as `postgres`). Requires `drizzle-kit` installed and configured. -**Changing an existing plaintext column to an encrypted one.** `drizzle-kit generate` emits an in-place `ALTER TABLE … ALTER COLUMN … SET DATA TYPE eql_v3_`, which Postgres rejects — there is no cast from `text`/`numeric` to an EQL domain. (On drizzle-kit 0.31.0 and later the emitted type is also mangled to `"undefined"."eql_v3_"`, since a `customType` has no `typeSchema`.) The `stash eql migration --drizzle` sweep repairs the invalid statement — the `stash-cli` skill covers what it rewrites and the rule that matters: the repair is data-destroying, so it is safe **only on an empty table**. It repairs only what it can place: the swept directory must also contain the migration that declared the column. If it does not, the statement is flagged for review instead of rewritten. For a table with live data, do **not** apply the swept migration; follow the staged flow in **Migrating an Existing Column to Encrypted** below instead. +**Changing an existing plaintext column to an encrypted one.** `drizzle-kit generate` emits an in-place `ALTER TABLE … ALTER COLUMN … SET DATA TYPE eql_v3_`, which Postgres rejects — there is no cast from `text`/`numeric` to an EQL domain. (On drizzle-kit 0.31.0 and later the emitted type is also mangled to `"undefined"."eql_v3_"`, since a `customType` has no `typeSchema`.) The `stash eql migration --drizzle` sweep repairs the invalid statement — the `stash-cli` skill covers what it rewrites. The repair is **add-only**: it adds a staged `_encrypted` column and leaves the source column in place, so it never emits `DROP COLUMN` or `RENAME COLUMN` and is safe to apply on a populated table. It repairs only what it can place: the swept directory must also contain the migration that declared the column. If the sweep cannot prove the source column's type, or the encrypted twin already exists, it leaves that statement untouched and the command exits non-zero so you review the directory before running `drizzle-kit migrate`. Applying the swept migration only *adds* the column — encrypting the data is the staged flow in **Migrating an Existing Column to Encrypted** below. + +**Reconcile your schema after a sweep — `drizzle-kit generate` will not tell you to.** The sweep repairs SQL and nothing else, so once you apply the swept migration three artefacts disagree: + +| | `email` | `email_encrypted` | +|---|---|---| +| database | `text` (unchanged) | `eql_v3_text_search` | +| `schema.ts` | declared as `eql_v3_text_search` | absent | +| `meta/*_snapshot.json` | declared as `eql_v3_text_search` | absent | + +`drizzle-kit generate` diffs `schema.ts` against the snapshot — it never reads `.sql` and never introspects the database. Those two still agree, so the diff is empty and it emits nothing. (Introspection is `drizzle-kit push`, a different command.) So nothing surfaces the divergence, and every consequence through the ORM is silent: + +- reads of `users.email` hand plaintext to the `customType.fromDriver` that expects an EQL envelope +- writes push an EQL envelope into a `text` column and **succeed**, storing ciphertext in a plaintext column +- `email_encrypted` is unreachable — it is in no Drizzle schema + +Reconciling is three steps, and the middle one is not optional. + +**1. Fix `schema.ts`:** declare the encrypted column under **its own name**, and set the source column **back** to its plaintext type. It is currently declared as the encrypted domain — that declaration is exactly what made `drizzle-kit generate` emit the invalid `ALTER` in the first place. + +```typescript +export const users = pgTable('users', { + email: text('email').notNull(), // back to plaintext + email_encrypted: types.TextSearch('email_encrypted'), // the twin the sweep added +}) +``` + +**2. Run `drizzle-kit generate`, then delete the regenerated `ADD COLUMN` from the migration it writes.** The snapshot has never seen `email_encrypted`, so `generate` **always** emits `ADD COLUMN "email_encrypted"` for it — and the swept migration already adds that column, so applying both fails at migrate time with `column "email_encrypted" already exists`. Deleting that one statement keeps the snapshot advance, which is the entire point of the step. (`ADD COLUMN IF NOT EXISTS` works too.) Anything else `generate` emits — such as a now no-op `SET DATA TYPE text` on the source column — can stay. + +This is not avoidable by editing the schema more carefully: the snapshot can only learn about the twin from a `generate` that also emits SQL to create it. `drizzle-kit push` would introspect and skip it, but that bypasses your migration history. + +**3. Run `drizzle-kit migrate`,** then backfill through the staged flow below before switching reads across. + +If you have **not** yet applied the swept migration, the cleaner option is to revert `schema.ts`, remove that migration entirely (its `.sql`, its `meta/*_snapshot.json`, and its entry in `meta/_journal.json`), and follow **Migrating an Existing Column to Encrypted** below from a clean start — `generate` then produces the `ADD COLUMN` itself and there is nothing to delete. ### Column Storage