From 91452c14c7cdf525184db2d69977b85964ae202b Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Wed, 29 Jul 2026 17:14:55 +1000 Subject: [PATCH 1/6] docs(skills): correct stash-drizzle for the add-only rewriter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `skills/stash-drizzle/SKILL.md` still told Drizzle users the sweep's repair is "data-destroying", safe "only on an empty table", and that a populated table must **not** apply the swept migration. That was true on remove-v2 and this branch makes it false — the repair is add-only and preserves the source column. It matters more than a stale sentence: SKILL_MAP.drizzle installs both stash-drizzle and stash-cli into every Drizzle project, so the two skills contradicted each other in the customer's repo, with the wrong one telling their agent not to apply a migration that is now safe. Per the AGENTS.md skill map, a Drizzle-integration change must check skills/stash-drizzle as well as skills/stash-cli. Missed in the original PR and carried through the rebase. (skills/ ships in the stash tarball; the existing changeset already scopes stash as a patch.) Also pins 'switch the application to the' in the CLI rewriter test, which the wizard twin already asserted — the parity guard compares the two source files, not the two test files, so the drift was invisible to CI. --- packages/cli/src/__tests__/rewrite-migrations.test.ts | 1 + skills/stash-drizzle/SKILL.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 7bb16dcdc..4584ef0a1 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -356,6 +356,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) }) diff --git a/skills/stash-drizzle/SKILL.md b/skills/stash-drizzle/SKILL.md index 63d1c3ba8..90fe2e255 100644 --- a/skills/stash-drizzle/SKILL.md +++ b/skills/stash-drizzle/SKILL.md @@ -61,7 +61,7 @@ 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. ### Column Storage From 54f364d01208db8ab8743827fdd329c900e0f9db Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 30 Jul 2026 09:59:59 +1000 Subject: [PATCH 2/6] fix(cli,wizard): index dollar-quoted DDL so the already-encrypted guard holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #836, item 1. `isInsideCommentOrString` skips a dollar-quoted body WHOLE. That is correct for the rewrite pass — the body is PL/pgSQL and `renderSafeAlter` returns multiple lines — but wrong for `indexColumnDeclarations`: DDL inside `DO $$ … END $$;` is EXECUTED SQL, so an encrypted `ADD COLUMN` there means the column really does hold ciphertext. It never entered the `encrypted` set, so the column fell to "plaintext by residue" and the sweep staged an empty `_encrypted` twin beside the real ciphertext — `rewritten` listing the file, `skipped` empty, exit code 0. This is the mechanism #811 reported; #823 closed it by removing the blast radius (add-only emission), not by closing the mechanism. The index now reads dollar-quoted bodies via `dollarQuotedBodies`, for the ENCRYPTED side only. That is the 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 is deliberately not recorded. An inert body (inside a `--` comment, or quoted in an `INSERT … VALUES`) stays inert; the rewrite pass is untouched. `target-exists` now also consults `encrypted`. The two sets are not nested: a twin added inside a dollar body is `encrypted` without ever being `declared`, and emitting a second ADD COLUMN for it fails with "column already exists". `dollarQuotedBodies` tracks comment/string state itself rather than asking `isInsideCommentOrString` about each `$`, and that is load-bearing, not a stylistic choice: the predicate rescans from index 0 on every call, so the per-opener form is quadratic. The corpus this sweeps is the drizzle output directory holding the ~2.6 MB EQL install migration the command has just written, and that file is itself thousands of small `$$` PL/pgSQL bodies — measured over it, the whole sweep runs ~0.4 s as a single pass versus ~8.5 s per-opener. It reuses the same token helpers (`dollarQuoteDelimiter`, `endOfQuoted`, `isEscapeStringOpener`) so the tokenisation rules stay identical: `"` consumed before `'`, nested block comments tracked by depth, and an unterminated comment, literal or dollar body ending the walk. A regression guard sweeps a 2.1 MB dollar-quote-heavy corpus with a 15 s bound — ~30x above the linear time so a slow shared runner stays comfortable, ~3x below the quadratic time; it measures ~70 ms as written and 41 s against the per-opener form. The alternative in the issue — fail closed on any table touched inside a dollar-quoted body — was rejected. It would trip on drizzle-kit's own `DO $$ … CREATE TYPE … EXCEPTION` enum idiom and flag every ALTER in such a corpus, for a residual (dynamic `EXECUTE` SQL) that is the same corpus-invisible class as the psql/dashboard drift `skills/stash-cli` already documents. A test pins that idiom as still rewritable. Three pre-existing expectations tightened, all toward fail-closed: - the `DO $$` ADD COLUMN corpus no longer rewrites at all. #823's own test codified the wrong outcome here (`rewrite-migrations.test.ts:977-998`). - both rename corpora report `already-encrypted` rather than `target-exists`. After those renames `email` IS the ciphertext, and `target-exists`'s "review the existing encrypted twin" pointed at a column the rename had consumed. - an unterminated `$$` now flags rather than rewrites. The file is unparseable, so Postgres ran none of it and nothing after the opener is proven. --- .../src/__tests__/rewrite-migrations.test.ts | 293 ++++++++++++++++- .../cli/src/commands/db/rewrite-migrations.ts | 298 +++++++++++++++--- .../src/__tests__/rewrite-migrations.test.ts | 293 ++++++++++++++++- packages/wizard/src/lib/rewrite-migrations.ts | 298 +++++++++++++++--- 4 files changed, 1056 insertions(+), 126 deletions(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 4584ef0a1..76f2c9f8e 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -946,6 +946,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";' @@ -971,11 +983,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'), [ @@ -990,12 +1005,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 () => { @@ -1019,11 +1035,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'), [ @@ -1036,15 +1057,261 @@ 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' }, + ]) + }) + + // 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([]) }) }) + /** + * `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 body scan alone measures ~3 ms in one pass and ~41 s per-opener, + * and the whole sweep runs in well under a second when linear. The 15 s bound + * therefore sits ~30x above the linear time (so a slow shared runner is still + * comfortable) and ~3x below the regressed time — it catches an + * order-of-magnitude regression and does not police milliseconds. + */ + 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', () => { diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index db82b4519..b1e8e42b7 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,156 @@ 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. + indexRenames(sql, encrypted, declared) + for (const body of opaque) indexEncryptedRenames(body, encrypted) + } + + 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) - 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)) + // 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. + * + * **Residue, accepted.** A rename CHAIN that alternates between live and + * dollar-quoted statements within a single file, in the order dollar→live, is + * not followed: the live pass above has already run by then. Crossing files + * works, 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. */ @@ -782,8 +970,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 } diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index aaecaa957..376dcd1be 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -886,6 +886,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 +923,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 +945,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 +975,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 +997,261 @@ 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' }, + ]) + }) + + // 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([]) }) }) + /** + * `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 body scan alone measures ~3 ms in one pass and ~41 s per-opener, + * and the whole sweep runs in well under a second when linear. The 15 s bound + * therefore sits ~30x above the linear time (so a slow shared runner is still + * comfortable) and ~3x below the regressed time — it catches an + * order-of-magnitude regression and does not police milliseconds. + */ + 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', () => { diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 7a04f0e36..95b11ae78 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,156 @@ 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. + indexRenames(sql, encrypted, declared) + for (const body of opaque) indexEncryptedRenames(body, encrypted) + } + + 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) - 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)) + // 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. + * + * **Residue, accepted.** A rename CHAIN that alternates between live and + * dollar-quoted statements within a single file, in the order dollar→live, is + * not followed: the live pass above has already run by then. Crossing files + * works, 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. */ @@ -790,8 +978,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 } From 5e3463c9db4def7cffe30831a2dc30a1f41e5f51 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 30 Jul 2026 10:46:32 +1000 Subject: [PATCH 3/6] fix(cli,wizard): report the artefact divergence a sweep leaves behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #836, item 2. `renderSafeAlter` is add-only, so after a sweep the database has `email text` PLUS `email_encrypted eql_v3_text_search`, while `schema.ts` still declares `email` 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` records the same. Neither knows the twin exists. `drizzle-kit generate` gives no signal: it diffs `schema.ts` against the snapshot, never reads `.sql`, and never introspects the database. Both inputs still agree, so the diff is empty. Every consequence through the ORM is therefore silent — reads of `users.email` hand plaintext to 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 with "column already exists". `RewriteResult` now carries `staged: StagedColumn[]` — one entry per rewritten statement, so it is finer-grained than `rewritten`, which lists files — and `describeStagedReconciliation` turns it into guidance naming the table, both columns and the domain. `stash eql migration --drizzle` and the wizard's post-agent step both print it, the wizard before its migrate prompt so the user decides with it in hand. The partial-sweep path reports it too: those twins are already on disk and already divergent. Warn rather than exit non-zero — the swept SQL is valid and additive, and which way to reconcile is the user's call. Writing the `schema.ts` edit was rejected: editing it alone CREATES the duplicate-column failure above, so 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. Detecting it in `stash encrypt plan` was rejected as too late — it runs later, and may never run. Item 3, same subsystem. `sweepMigrationDirs` read its partial result off an unchecked `err as Partial`. For a non-object throw (`throw null`) that property read raises a TypeError INSIDE the catch, so the per-directory error result is never pushed, the throw escapes `sweepMigrationDirs` entirely, and the remaining directories go unswept — the fail-closed reporting the catch exists to provide simply does not happen. The CLI had been hardened against exactly this; the wizard had not, and it was the only unresolved review thread on merged #823. Rather than duplicate the predicate, `PartialRewriteResult`/`isPartialRewriteResult` move into the shared part of both rewriter copies — so the parity guard now polices them, which it could not do while the wizard's version sat inside `#region wizard-only` — and `eql/migration.ts` imports instead of defining its own. A test forces `throw null` through the writeFile spy and pins that the directory is still reported and the sweep still continues; it fails with the exact "Cannot read properties of null (reading 'rewritten')" against the old cast. --- .../src/__tests__/rewrite-migrations.test.ts | 126 ++++++++++++++ .../cli/src/commands/db/rewrite-migrations.ts | 135 ++++++++++++++- .../commands/eql/__tests__/migration.test.ts | 86 ++++++++++ packages/cli/src/commands/eql/migration.ts | 49 +++--- .../wizard/src/__tests__/post-agent.test.ts | 51 +++++- .../src/__tests__/rewrite-migrations.test.ts | 161 ++++++++++++++++++ packages/wizard/src/lib/post-agent.ts | 38 ++++- packages/wizard/src/lib/rewrite-migrations.ts | 148 +++++++++++++++- 8 files changed, 750 insertions(+), 44 deletions(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 76f2c9f8e..211dc9d46 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' @@ -1268,6 +1269,131 @@ describe('rewriteEncryptedAlterColumns', () => { }) }) + /** + * #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') + }) + }) + /** * `dollarQuotedBodies` must track comment/string state itself. Asking * `isInsideCommentOrString` about each `$` is the obvious implementation and diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index b1e8e42b7..e6b2b7cd1 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -846,19 +846,140 @@ 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 { 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}, 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.', + 'Declare the encrypted column in your Drizzle schema under its own name, keep the source column as its plaintext type, then run `drizzle-kit generate` to bring the snapshot forward. Do not hand-edit only the schema and re-generate against the stale snapshot: that emits a duplicate ADD COLUMN which fails with "column already exists". Then run the staged `stash encrypt` lifecycle to backfill before switching reads across.', + ) + 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. @@ -906,6 +1027,7 @@ export async function rewriteEncryptedAlterColumns( ) const rewritten: string[] = [] const skipped: SkippedAlter[] = [] + const staged: StagedColumn[] = [] const seen = new Set() const stagedTargets = new Set() @@ -992,6 +1114,14 @@ export async function rewriteEncryptedAlterColumns( if (!domain) return match stagedTargets.add(target) + staged.push({ + file: filePath, + schema, + table, + column, + encryptedColumn: `${column}_encrypted`, + domain, + }) return renderSafeAlter(table, column, domain, schema) }, ) @@ -1026,11 +1156,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..c6f2840f0 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 }) 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 376dcd1be..5268aed44 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' @@ -1208,6 +1209,131 @@ describe('rewriteEncryptedAlterColumns', () => { }) }) + /** + * #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') + }) + }) + /** * `dollarQuotedBodies` must track comment/string state itself. Asking * `isInsideCommentOrString` about each `$` is the obvious implementation and @@ -2183,6 +2309,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(() => { @@ -2259,6 +2389,37 @@ 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([]) + // …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')]) + }) + 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..fa278f890 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,14 +74,19 @@ 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. + p.log.warn(describeStagedReconciliation(sweep.staged).join('\n')) } if (skipped || unverified) { throw new Error( @@ -85,7 +95,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 +141,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 95b11ae78..863525622 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -854,19 +854,140 @@ 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 { 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}, 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.', + 'Declare the encrypted column in your Drizzle schema under its own name, keep the source column as its plaintext type, then run `drizzle-kit generate` to bring the snapshot forward. Do not hand-edit only the schema and re-generate against the stale snapshot: that emits a duplicate ADD COLUMN which fails with "column already exists". Then run the staged `stash encrypt` lifecycle to backfill before switching reads across.', + ) + 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. @@ -914,6 +1035,7 @@ export async function rewriteEncryptedAlterColumns( ) const rewritten: string[] = [] const skipped: SkippedAlter[] = [] + const staged: StagedColumn[] = [] const seen = new Set() const stagedTargets = new Set() @@ -1000,6 +1122,14 @@ export async function rewriteEncryptedAlterColumns( if (!domain) return match stagedTargets.add(target) + staged.push({ + file: filePath, + schema, + table, + column, + encryptedColumn: `${column}_encrypted`, + domain, + }) return renderSafeAlter(table, column, domain, schema) }, ) @@ -1034,11 +1164,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 @@ -1130,6 +1261,7 @@ export async function sweepMigrationDirs( dir, rewritten: [], skipped: [], + staged: [], notDrizzleOutput: true, }) } @@ -1137,15 +1269,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, }) } From d84ebacfe44b850af60f77f348977ed4b381ec14 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 30 Jul 2026 10:46:46 +1000 Subject: [PATCH 4/6] docs(skills,changeset): document post-sweep reconciliation and the DO $$ rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per AGENTS.md, a change to the CLI command surface or a user-facing workflow must update the affected shipped skills in the same PR — `skills/` ships inside the `stash` tarball and is copied into customer repos, so drift here becomes wrong guidance in someone else's codebase. `skills/stash-drizzle` gains the reconciliation step for #836 item 2: a table of what the database, `schema.ts` and `meta/*_snapshot.json` each say after a sweep, why `drizzle-kit generate` reports nothing (it diffs schema against snapshot and reads neither the `.sql` nor the database — introspection is `push`), the three silent ORM consequences, and the corrected `schema.ts` with both columns declared. It also warns against the half-fix: changing only the source column's type and re-generating against the stale snapshot emits a duplicate ADD COLUMN that fails with `column already exists`. This lands on the paragraph the preceding commit corrected for #837, which is why that correction had to come first — writing this on top of the old "data-destroying / safe only on an empty table" text would have silently reinstated it. `skills/stash-cli` gains the same reconciliation note, scoped to what the command prints, plus the fail-closed rule's new detail: a declaration inside a `DO $$ … END $$;` block counts toward "already encrypted", while a plaintext one inside such a block does not count as declaring the column. Dynamic `EXECUTE` SQL joins hand-psql and dashboard edits in the list of corpus-invisible drift. Verified against the registry per AGENTS.md — every `--flag` the `eql migration` section names resolves against `stash manifest --json` (`--eql-version` appears only in the sentence saying it does not exist). Changeset: `stash` patch, `@cipherstash/wizard` patch. --- ...writer-dollar-quoted-and-reconciliation.md | 44 +++++++++++++++++++ skills/stash-cli/SKILL.md | 4 +- skills/stash-drizzle/SKILL.md | 25 +++++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 .changeset/rewriter-dollar-quoted-and-reconciliation.md diff --git a/.changeset/rewriter-dollar-quoted-and-reconciliation.md b/.changeset/rewriter-dollar-quoted-and-reconciliation.md new file mode 100644 index 000000000..3c47bc504 --- /dev/null +++ b/.changeset/rewriter-dollar-quoted-and-reconciliation.md @@ -0,0 +1,44 @@ +--- +'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 and the domain, along with +the reconciling schema edit and the stale-snapshot trap that turns a partial fix +into `column already exists`. The `skills/stash-cli` and `skills/stash-drizzle` +guides document it. + +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/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 26f982302..09d5807b3 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 schema edit — and note that changing only the schema and re-generating against the stale snapshot emits a duplicate `ADD COLUMN` that fails with `column already exists`. #### `eql upgrade` diff --git a/skills/stash-drizzle/SKILL.md b/skills/stash-drizzle/SKILL.md index 90fe2e255..c56b976e2 100644 --- a/skills/stash-drizzle/SKILL.md +++ b/skills/stash-drizzle/SKILL.md @@ -63,6 +63,31 @@ The generated migration also installs the `cs_migrations` tracking schema, so a **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 + +The fix: declare the encrypted column in `schema.ts` under **its own name**, keep the source column as its plaintext type, then run `drizzle-kit generate` to bring the snapshot forward. + +```typescript +export const users = pgTable('users', { + email: text('email').notNull(), // source column, still plaintext + email_encrypted: types.TextSearch('email_encrypted'), // the twin the sweep added +}) +``` + +Do **not** just change `email`'s type and re-generate: that diffs against the stale snapshot and emits a duplicate `ADD COLUMN "email_encrypted"`, which fails at migrate time with `column "email_encrypted" already exists`. Then backfill through the staged flow below before switching reads across. + ### Column Storage Each encrypted column is a concrete Postgres domain named `public.eql_v3_`: From f33f21bd4fa90171fdc06b6bbdcc86f9a167d149 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 30 Jul 2026 11:00:27 +1000 Subject: [PATCH 5/6] fix(cli,wizard): make the post-sweep reconciliation guidance actually work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #839 found the remediation text walked the user into the very failure it warned about, and that the notice could name a column that was never written. **The guidance was wrong.** It said to declare the twin under its own name, "keep the source column as its plaintext type", then run `drizzle-kit generate` to bring the snapshot forward — and warned separately against hand-editing "only" the schema. Both halves were off: - After a sweep `schema.ts` declares the SOURCE column as the encrypted domain; that declaration is what produced the invalid ALTER. So it has to be set BACK to plaintext — "keep" describes a state the schema is not in. - The duplicate `ADD COLUMN` is not caused by editing partially. The snapshot has never seen the twin, so `generate` ALWAYS emits an `ADD COLUMN` for it, and the swept migration already added that column — applying both fails with "column already exists". Following the old text produced exactly that error, whether or not the source column was reverted. The notice and both skills now give the sequence that reaches a working state: fix the schema (twin under its own name, source back to plaintext), run `generate` to advance the snapshot, then DELETE the regenerated `ADD COLUMN` for the twin from the migration it wrote — keeping the snapshot advance, which is the point of the step. `IF NOT EXISTS` is noted as an equivalent. Anything else `generate` emits, such as the now no-op `SET DATA TYPE` on the source column, can stay. `skills/stash-drizzle` also documents the cleaner alternative when the swept migration has not been applied yet: revert the schema, remove that migration (`.sql`, `meta/*_snapshot.json`, and its `meta/_journal.json` entry) and follow the canonical staged rollout, which generates the ADD COLUMN itself. **`staged` could name a column that was never persisted.** The entries are produced inside `.replace()`, which runs before `writeFile`. A sweep that threw on a later file therefore attached twins from a file whose write had failed, and the notice claims "the database now has" those columns. They are now buffered per file and committed only after that file's write succeeds. `stagedTargets`, the in-run duplicate guard, is deliberately not rolled back: if a write fails, treating the target as taken keeps a later file from staging it again. Found by writing the test for the previously uncovered partial-sweep reporting path — it reproduced as `['email', 'name']` where only `email` reached disk. Also surfaces `StagedColumn.file`, which was populated and documented but never read: the per-column line now names the migration the twin was staged in, so the user can go read the statement instead of searching the directory. Coverage for the three paths this PR added but never exercised: - `describeStagedReconciliation` must say "back to its plaintext type" and must name a delete-the-ADD-COLUMN step; it must not say "keep the source column" or blame partial editing. Prose assertions, but they pin the two claims that make the difference between guidance that works and guidance that fails. - `eql migration --drizzle` reports staged twins from a sweep that threw part way through — fails if the `partial.staged` branch is removed. - `sweepMigrationDirs` carries staged twins out of a directory whose sweep threw, and carries only the ones that reached disk — fails if the `partial.staged ?? []` carry is dropped. stash 915 passed (60 files, default timeouts), wizard 362 passed, parity guard 4 passed, build 9/9, code:check 0 errors. --- ...writer-dollar-quoted-and-reconciliation.md | 16 ++- .../src/__tests__/rewrite-migrations.test.ts | 64 ++++++++++- .../cli/src/commands/db/rewrite-migrations.ts | 29 ++++- .../commands/eql/__tests__/migration.test.ts | 41 +++++++ .../src/__tests__/rewrite-migrations.test.ts | 100 +++++++++++++++++- packages/wizard/src/lib/rewrite-migrations.ts | 29 ++++- skills/stash-cli/SKILL.md | 2 +- skills/stash-drizzle/SKILL.md | 14 ++- 8 files changed, 277 insertions(+), 18 deletions(-) diff --git a/.changeset/rewriter-dollar-quoted-and-reconciliation.md b/.changeset/rewriter-dollar-quoted-and-reconciliation.md index 3c47bc504..c89297908 100644 --- a/.changeset/rewriter-dollar-quoted-and-reconciliation.md +++ b/.changeset/rewriter-dollar-quoted-and-reconciliation.md @@ -28,10 +28,18 @@ 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 and the domain, along with -the reconciling schema edit and the stale-snapshot trap that turns a partial fix -into `column already exists`. The `skills/stash-cli` and `skills/stash-drizzle` -guides document it. +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 diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 211dc9d46..6d996dd47 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -1392,6 +1392,58 @@ describe('rewriteEncryptedAlterColumns', () => { 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') + }) }) /** @@ -2063,12 +2115,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 e6b2b7cd1..c082a7b44 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -911,16 +911,25 @@ export function describeStagedReconciliation( 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 { schema, table, column, encryptedColumn, domain } of staged) { + 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}, but your schema declares "${column}" as ${domain} and knows nothing about "${encryptedColumn}".`, + ` - ${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.', - 'Declare the encrypted column in your Drizzle schema under its own name, keep the source column as its plaintext type, then run `drizzle-kit generate` to bring the snapshot forward. Do not hand-edit only the schema and re-generate against the stale snapshot: that emits a duplicate ADD COLUMN which fails with "column already exists". Then run the staged `stash encrypt` lifecycle to backfill before switching reads across.', + '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 } @@ -1062,6 +1071,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, ( @@ -1113,8 +1130,11 @@ 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) - staged.push({ + fileStaged.push({ file: filePath, schema, table, @@ -1129,6 +1149,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 diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index c6f2840f0..054755cf3 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -577,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/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index 5268aed44..ccf37706a 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -1332,6 +1332,58 @@ describe('rewriteEncryptedAlterColumns', () => { 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') + }) }) /** @@ -2003,12 +2055,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) } @@ -2415,11 +2477,47 @@ describe('sweepMigrationDirs', () => { 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/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 863525622..b8d8902a6 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -919,16 +919,25 @@ export function describeStagedReconciliation( 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 { schema, table, column, encryptedColumn, domain } of staged) { + 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}, but your schema declares "${column}" as ${domain} and knows nothing about "${encryptedColumn}".`, + ` - ${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.', - 'Declare the encrypted column in your Drizzle schema under its own name, keep the source column as its plaintext type, then run `drizzle-kit generate` to bring the snapshot forward. Do not hand-edit only the schema and re-generate against the stale snapshot: that emits a duplicate ADD COLUMN which fails with "column already exists". Then run the staged `stash encrypt` lifecycle to backfill before switching reads across.', + '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 } @@ -1070,6 +1079,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, ( @@ -1121,8 +1138,11 @@ 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) - staged.push({ + fileStaged.push({ file: filePath, schema, table, @@ -1137,6 +1157,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 diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 09d5807b3..a8c7d6db8 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -380,7 +380,7 @@ After writing the migration, `--drizzle` sweeps the output directory for sibling 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 schema edit — and note that changing only the schema and re-generating against the stale snapshot emits a duplicate `ADD COLUMN` that fails with `column already exists`. +**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 c56b976e2..bf0740424 100644 --- a/skills/stash-drizzle/SKILL.md +++ b/skills/stash-drizzle/SKILL.md @@ -77,16 +77,24 @@ The generated migration also installs the `cs_migrations` tracking schema, so a - 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 -The fix: declare the encrypted column in `schema.ts` under **its own name**, keep the source column as its plaintext type, then run `drizzle-kit generate` to bring the snapshot forward. +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(), // source column, still plaintext + email: text('email').notNull(), // back to plaintext email_encrypted: types.TextSearch('email_encrypted'), // the twin the sweep added }) ``` -Do **not** just change `email`'s type and re-generate: that diffs against the stale snapshot and emits a duplicate `ADD COLUMN "email_encrypted"`, which fails at migrate time with `column "email_encrypted" already exists`. Then backfill through the staged flow below before switching reads across. +**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 From 3e11af6aa61050b39da06403af8dd6e9c5a78bc7 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 30 Jul 2026 12:46:20 +1000 Subject: [PATCH 6/6] fix(cli,wizard): follow rename chains across the dollar-quoted boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 3. One accepted-residue verdict overturned, two addressed as raised. **The rename-chain residue is reachable, so it is now fixed rather than documented.** `indexColumnDeclarations` ran the live rename pass before the dollar-quoted one, so a chain crossing dollar→live inside one file lost the type: `ADD COLUMN "tmp" ` live, `RENAME tmp TO mid` inside `DO $$`, then a live `RENAME mid TO email` left `email` looking like plaintext and staged an empty twin beside its ciphertext — the exact defect this branch exists to close. It was accepted on the grounds that drizzle-kit never emits `RENAME COLUMN` inside `DO $$`. True, and the wrong reachability test: the corpus #811 actually reported is hand-written and does precisely this, a human staging a manual column swap. `rewrite-migrations.test.ts`'s own "reported two-file DO $$ rename corpus" fixture is that shape. Reachability here is about what people write by hand, not what the generator emits. Both rename passes now iterate to a fixpoint, so which one runs first stops mattering and chains are followed in either direction. It terminates because both sets only grow and are bounded by the distinct column keys in the corpus, and it converges one iteration past the longest chain — a corpus with no renames pays a second scan that finds nothing. The dollar-quote-heavy perf guard is unmoved at ~70 ms. Tests cover both directions, so a future reordering of the passes cannot break one while the other stays green. **The wizard's reconciliation notice is now gated on `staged`, matching the CLI.** The two conditions are equivalent today — a rewritten file always yields at least one staged column, and both are recorded together only after the write succeeds — so this is consistency, not a behaviour change, and no test can distinguish them. The 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. **The perf guard's documented margin was understated, which is what made it look marginal.** The comment claimed ~30x headroom, derived from a hand-waved "well under a second". Measured, the whole sweep over that corpus is ~67-71 ms against a 15 s bound: ~200x. Corrected, with the flake question answered concretely — tripping it needs a runner ~200x slower at scanning a string than a developer laptop, where CI is 2-5x — and a note to raise the bound rather than delete the test if it ever does fire, since what it guards is shipped-command latency. Also drops the now-stale "Residue, accepted" paragraph from `indexEncryptedRenames`. stash 917 passed (60 files, default timeouts), wizard 364 passed, parity guard 4 passed, build 9/9, code:check 0 errors. --- .../src/__tests__/rewrite-migrations.test.ts | 79 +++++++++++++++++-- .../cli/src/commands/db/rewrite-migrations.ts | 30 +++++-- .../src/__tests__/rewrite-migrations.test.ts | 79 +++++++++++++++++-- packages/wizard/src/lib/post-agent.ts | 17 +++- packages/wizard/src/lib/rewrite-migrations.ts | 30 +++++-- 5 files changed, 209 insertions(+), 26 deletions(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 6d996dd47..c3ca5f5af 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -1132,6 +1132,70 @@ describe('rewriteEncryptedAlterColumns', () => { ]) }) + /** + * 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". @@ -1458,11 +1522,16 @@ describe('rewriteEncryptedAlterColumns', () => { * 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 body scan alone measures ~3 ms in one pass and ~41 s per-opener, - * and the whole sweep runs in well under a second when linear. The 15 s bound - * therefore sits ~30x above the linear time (so a slow shared runner is still - * comfortable) and ~3x below the regressed time — it catches an - * order-of-magnitude regression and does not police milliseconds. + * 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( diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index c082a7b44..c8c60ca1f 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -664,8 +664,26 @@ function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { // 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. - indexRenames(sql, encrypted, declared) - for (const body of opaque) indexEncryptedRenames(body, encrypted) + // + // 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 } @@ -792,10 +810,10 @@ function indexRenames( * a branch that never ran, and inventing a declaration from it is the * fail-open direction. * - * **Residue, accepted.** A rename CHAIN that alternates between live and - * dollar-quoted statements within a single file, in the order dollar→live, is - * not followed: the live pass above has already run by then. Crossing files - * works, since files are indexed in sorted order. + * 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)) { diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index ccf37706a..6a31db3a6 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -1072,6 +1072,70 @@ describe('rewriteEncryptedAlterColumns', () => { ]) }) + /** + * 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". @@ -1398,11 +1462,16 @@ describe('rewriteEncryptedAlterColumns', () => { * 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 body scan alone measures ~3 ms in one pass and ~41 s per-opener, - * and the whole sweep runs in well under a second when linear. The 15 s bound - * therefore sits ~30x above the linear time (so a slow shared runner is still - * comfortable) and ~3x below the regressed time — it catches an - * order-of-magnitude regression and does not police milliseconds. + * 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( diff --git a/packages/wizard/src/lib/post-agent.ts b/packages/wizard/src/lib/post-agent.ts index fa278f890..30201b77d 100644 --- a/packages/wizard/src/lib/post-agent.ts +++ b/packages/wizard/src/lib/post-agent.ts @@ -82,10 +82,19 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { 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. + } + // 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) { diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index b8d8902a6..4d8e673aa 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -672,8 +672,26 @@ function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { // 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. - indexRenames(sql, encrypted, declared) - for (const body of opaque) indexEncryptedRenames(body, encrypted) + // + // 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 } @@ -800,10 +818,10 @@ function indexRenames( * a branch that never ran, and inventing a declaration from it is the * fail-open direction. * - * **Residue, accepted.** A rename CHAIN that alternates between live and - * dollar-quoted statements within a single file, in the order dollar→live, is - * not followed: the live pass above has already run by then. Crossing files - * works, since files are indexed in sorted order. + * 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)) {