From b68ce48fc0a0556ec3f7f33a069199fb40214384 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 20:57:26 +1000 Subject: [PATCH 01/11] docs(specs): fail-closed inversion for the encrypted ALTER COLUMN rewrite --- ...026-07-24-a2-fail-closed-rewrite-design.md | 203 ++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-24-a2-fail-closed-rewrite-design.md diff --git a/docs/superpowers/specs/2026-07-24-a2-fail-closed-rewrite-design.md b/docs/superpowers/specs/2026-07-24-a2-fail-closed-rewrite-design.md new file mode 100644 index 000000000..f44fa80f4 --- /dev/null +++ b/docs/superpowers/specs/2026-07-24-a2-fail-closed-rewrite-design.md @@ -0,0 +1,203 @@ +# A-2 — fail-closed inversion for the encrypted ALTER COLUMN rewrite + +**Date:** 2026-07-24 +**Addresses:** A-2 in `.work/2026-07-24-eql-v2-removal-verification.md` +**Base:** `05b7bf07` (A-1/A-3 tokenizer fix) +**Affects:** `packages/cli/src/commands/db/rewrite-migrations.ts`, `packages/wizard/src/lib/rewrite-migrations.ts` (the same file twice) + +## Problem + +`rewriteEncryptedAlterColumns` rewrites an in-place `ALTER COLUMN … SET DATA TYPE +` into an ADD + DROP + RENAME sequence. That sequence is +equivalent to DROP + ADD: it fixes the column's type but destroys its contents. + +The sweep decides whether to rewrite by consulting a corpus-wide index of columns +the migrations give an *encrypted* type (`indexEncryptedColumns`). A column found +there is skipped as `already-encrypted`, because dropping it would destroy +ciphertext with no plaintext left to backfill from. **Everything else is +rewritten** — including a column the corpus never mentions at all. + +That default is fail-open. A column absent from the index is not known to be +plaintext; it is simply unknown. Two reproduced triggers, both from the +verification doc: + +- **(a) Lone ALTER, no source declaration.** The corpus contains the ALTER and + nothing that declares the column. Rewritten today: 1 live `DROP COLUMN`. +- **(b) Cross-directory split.** The declaration lives in `drizzle/` and the + ALTER in `migrations/`. The index is built per directory, so the sweep of + `migrations/` never sees the declaration. Rewritten today: 1 live `DROP + COLUMN` — and the dropped column is already typed `eql_v3_text_eq`, i.e. + ciphertext. + +Trigger (b) is not contrived. `post-agent.ts:163` ships with +`DRIZZLE_OUT_DIRS = ['drizzle', 'migrations', 'src/db/migrations']`, so scanning +multiple directories with a per-directory index *is* the default configuration. + +## Decision + +Invert the default. Rewrite only a column the corpus positively declares and does +not give an encrypted type. Anything unknown is skipped and reported. + +Two alternatives were considered and rejected: + +- **Widen the index across directories** (build one corpus index in + `sweepMigrationDirs` and pass it into each per-directory rewrite). Fixes (b) + with a sharper `already-encrypted` message, but unioning unrelated migration + histories can over-detect *plaintext*, which is itself fail-open. Rejected: + fail-closed already makes (b) safe, and this trades a better message for a new + fail-open surface. +- **Require the declaration in the same file.** A column created in + `0001_init.sql` and altered in `0007_encrypt.sql` is the normal case, so this + would stop rewriting anything. + +## Mechanism + +The encrypted side is already precise: it matches against the known domain list +in `ENCRYPTED_DOMAIN`. So "plaintext" needs no type classification — it is the +residue. A column the corpus **declares** but that is not in the encrypted set is +plaintext. + +That turns "what type is this column?" (a SQL parse) into "does a declaration for +this column exist?" (a name match in a position the code already isolates). No +comma splitting, no paren-depth tracking, no quote state machine. + +`indexEncryptedColumns` becomes `indexColumnDeclarations`, one traversal +returning `{ encrypted, declared }`. `encrypted` is populated exactly as today — +the existing broad scan is deliberately over-detecting and must not regress. +`declared` is populated from two positions: + +1. **Inside a `CREATE TABLE` body**, already captured as group 3 of + `CREATE_TABLE_RE`: scan `"([^"]+)"\s+["a-z]` for declared names. +2. **`ALTER TABLE … ADD COLUMN "col" `**: generalise + `ADD_ENCRYPTED_COLUMN_RE` to accept any type with the same tail. + +`RENAME COLUMN "a" TO "b"` propagates `declared` the way it already propagates +`encrypted`. + +The `\s+["a-z]` tail is what makes the name match a declaration rather than a +mention. A type token always begins with a letter or a double quote, so +`"email" text` and `"email" "public"."eql_v3_text_search"` match, while these do +not — the name is followed by `)`, `,` or an operator: + +```sql +PRIMARY KEY ("id", "name") +FOREIGN KEY ("org_id") REFERENCES "orgs"("id") +CHECK ("age" > 0) +UNIQUE("email") +``` + +**The scan must stay anchored to those two positions.** A whole-file scan would +let `ALTER COLUMN "email" SET DATA TYPE …` match its own `"email" SET` and +declare the very column it is asking about — a self-fulfilling fail-open. + +**Known false positive, accepted.** In +`CONSTRAINT "users_email_unique" UNIQUE("email")` the constraint *name* is +followed by whitespace and a letter, so it registers as a declared column. It is +inert unless a constraint name exactly equals the name of a column that is also +encrypted and undeclared; drizzle's `__unique` convention makes that +contrived. Documented in the docstring rather than engineered around. + +**Corpus-wide, not migration-ordered.** As with the existing encrypted index, a +declaration in a *later* migration than the ALTER still counts. This is a +pre-existing property, not introduced here. + +### Decision order in the rewrite callback + +Unchanged first step, two steps after it: + +1. `isInsideCommentOrString(original, offset)` → return the match untouched. +2. In `encrypted` → `skip(…, 'already-encrypted')`. +3. Not in `declared` → `skip(…, 'source-unknown')`. +4. Otherwise rewrite. + +Step 2 must precede step 3 so a column that is both declared and encrypted — the +output of a previous sweep, which emits `ADD COLUMN ` plus +`RENAME TO ` — still reports `already-encrypted`, the more specific +and more actionable reason. + +`isInsideCommentOrString` is not touched: it is the subject of A-1/A-3, already +landed in the base commit. + +## Skip reason + +`SkipReason` gains `'source-unknown'`. `describeSkipReason` has no `default` arm, +so the compiler forces the new arm to be written. All three consumers +(`install.ts:660`, `eql/migration.ts:248`, `post-agent.ts:192`) render +`describeSkipReason(reason)` verbatim and need no edit. + +The text names both causes, because the user's remedy differs: + +> the sweep could not find where this column was declared in this migration +> directory, so it cannot tell a plaintext column (safe to rewrite) from one that +> already holds ciphertext (where the rewrite would DROP it). Usually the +> column's `CREATE TABLE` / `ADD COLUMN` lives in a different directory, or the +> migration history was squashed. Check the column's current type in the +> database: if it is plaintext and the table is empty, apply the +> ADD/DROP/RENAME by hand; if it already holds ciphertext, use the staged +> `stash encrypt` lifecycle instead + +## Blast radius + +This is a behaviour change for correct corpora, not only for the bug. Any project +whose swept directory does not contain the column's declaration — squashed +migrations, a baseline pulled with `drizzle-kit pull`, a schema bootstrapped from +a hand-written SQL file — stops being rewritten and starts being flagged for +review. That is the trade being bought, and it is the correct direction: the cost +of a false skip is a statement the user fixes by hand, and the cost of a false +rewrite is irrecoverable data. + +## Testing + +Most existing tests in both files use a bare ALTER with no `CREATE` anywhere and +assert a rewrite; under the inversion they would all become `source-unknown`. +Rather than rewrite each fixture, add a helper that writes a `0000_init.sql` +declaring the plaintext source column. It sorts first and is never rewritten, so +`rewritten`/`skipped` assertions stay as they are and the diff is one line per +test. + +New cases, in **both** test files: + +- Lone ALTER, no declaration anywhere → `skipped` with `source-unknown`, file + unchanged on disk, zero `DROP COLUMN`. +- Declaration in a sibling file in the same directory → rewritten (the + non-regression case). +- Declaration living in the `options.skip` file → still counts as declared. +- `RENAME COLUMN` propagates declared-ness from the original name. +- A column named only inside `PRIMARY KEY (…)` / `REFERENCES "t"("col")` → + **not** declared, so `source-unknown`. +- A column that is both declared and encrypted → still `already-encrypted`, not + `source-unknown` (pins the ordering of steps 2 and 3). + +Wizard file only: + +- The cross-directory split from the verification doc, driven through + `sweepMigrationDirs`: encrypted declaration in `drizzle/`, ALTER in + `migrations/` → `source-unknown`, zero `DROP COLUMN` in the output. + +## Dual-copy sync + +The two files are the same file twice, differing only in the wizard's +`sweepMigrationDirs` / `DirRewriteResult`, its attribution string +(`@cipherstash/wizard` vs `stash`), and a few comment blocks. Apply one fix +twice, then diff the two whitespace-normalised files to confirm nothing new +diverged. + +## Docs and changeset + +- `skills/stash-cli/SKILL.md:393` and `skills/stash-drizzle/SKILL.md:64` both + state that each such statement is rewritten. Both need the qualifier that the + rewrite now requires the column's declaration to be present in the swept + directory, and otherwise flags the statement for review. +- Extend `.changeset/rewriter-never-drops-ciphertext.md` rather than adding a + second changeset: it already ships the sibling `already-encrypted` behaviour + for the same component in the same release, and two changesets would describe + overlapping behaviour. Both `stash` and `@cipherstash/wizard` are already + listed. + +## Out of scope + +- Any change to `isInsideCommentOrString` (A-1/A-3, landed in the base commit). +- Sharing a corpus index across directories in `sweepMigrationDirs` (rejected + above). +- The near-miss scan (`NEAR_MISS_RE`) and its `unrecognised-form` reason, which + are unaffected. From d20a05281a795d54e1f2e5d72611116aa675683d Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 21:14:00 +1000 Subject: [PATCH 02/11] fix(cli): rewrite an encrypted ALTER only for a column the corpus declares A column missing from the encrypted index was treated as plaintext and rewritten into ADD+DROP+RENAME. Absence is not evidence: the declaration can live in a migration directory the sweep never reads, so the DROP COLUMN could land on a column already holding ciphertext. The corpus index now records declared columns as well as encrypted ones, and an ALTER whose column is declared nowhere is reported as source-unknown instead of rewritten. --- .../src/__tests__/rewrite-migrations.test.ts | 215 ++++++++++++++++++ .../cli/src/commands/db/rewrite-migrations.ts | 129 +++++++++-- 2 files changed, 323 insertions(+), 21 deletions(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 4f449965e..ac38926de 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -15,7 +15,28 @@ describe('rewriteEncryptedAlterColumns', () => { fs.rmSync(tmpDir, { recursive: true, force: true }) }) + /** + * Declare `columns` on `tableRef` as PLAINTEXT, in a migration that sorts + * before every fixture below. + * + * The sweep is fail-closed: it rewrites a column only when the corpus shows + * the column exists and is not already encrypted. A fixture that is just an + * ALTER declares nothing, so it is `source-unknown` by design — a test that + * exercises the REWRITE has to supply the `CREATE TABLE` a real drizzle + * corpus would carry. + * + * `tableRef` is written exactly as it appears in the ALTER, so a pgSchema() + * table passes `'"app"."users"'` and the declaration lands on the same key. + */ + const declarePlaintext = (tableRef: string, ...columns: string[]): void => { + const file = path.join(tmpDir, '0000_declare.sql') + const existing = fs.existsSync(file) ? fs.readFileSync(file, 'utf-8') : '' + const defs = columns.map((column) => `"${column}" text`).join(', ') + fs.writeFileSync(file, `${existing}CREATE TABLE ${tableRef} (${defs});\n`) + } + it('rewrites an in-place ALTER COLUMN with the bare type name', async () => { + declarePlaintext('"transactions"', 'amount') const original = `ALTER TABLE "transactions" ALTER COLUMN "amount" SET DATA TYPE eql_v2_encrypted;\n` const filePath = path.join(tmpDir, '0002_alter.sql') fs.writeFileSync(filePath, original) @@ -37,6 +58,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the schema-qualified form produced by drizzle-kit', async () => { + declarePlaintext('"users"', 'email') const original = 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "public"."eql_v2_encrypted";\n' const filePath = path.join(tmpDir, '0003_alter.sql') @@ -54,6 +76,7 @@ describe('rewriteEncryptedAlterColumns', () => { it('rewrites a schema-qualified table produced by pgSchema()', async () => { // drizzle-kit emits `"app"."users"` for a table declared in a pgSchema(); // the old `\s+` between the table and ALTER COLUMN could never cross the `.`. + declarePlaintext('"app"."users"', 'email') const original = 'ALTER TABLE "app"."users" ALTER COLUMN "email" SET DATA TYPE "undefined"."eql_v3_text_search";\n' const filePath = path.join(tmpDir, '0014_qualified.sql') @@ -75,6 +98,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the "undefined" schema form drizzle-kit emits for bare custom types', async () => { + declarePlaintext('"transactions"', 'amount') const original = 'ALTER TABLE "transactions" ALTER COLUMN "amount" SET DATA TYPE "undefined"."eql_v2_encrypted";\n' const filePath = path.join(tmpDir, '0005_undef.sql') @@ -90,6 +114,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the double-quoted form produced by stack 0.15.0', async () => { + declarePlaintext('"transactions"', 'description') const original = 'ALTER TABLE "transactions" ALTER COLUMN "description" SET DATA TYPE "undefined".""public"."eql_v2_encrypted"";\n' const filePath = path.join(tmpDir, '0006_double.sql') @@ -117,6 +142,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('skips the file passed in options.skip', async () => { + declarePlaintext('"t"', 'c') const install = path.join(tmpDir, '0000_install-eql.sql') const alter = path.join(tmpDir, '0002_alter.sql') fs.writeFileSync(install, 'CREATE SCHEMA eql_v2;\n') @@ -169,6 +195,7 @@ describe('rewriteEncryptedAlterColumns', () => { ] it.each(V3_DOMAINS)('rewrites an ALTER COLUMN to %s', async (domain) => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0007_v3.sql') fs.writeFileSync( filePath, @@ -196,6 +223,7 @@ describe('rewriteEncryptedAlterColumns', () => { ...V3_DOMAINS, 'eql_v2_encrypted', ])('extracts the bare domain %s from a mangled ALTER', async (domain) => { + declarePlaintext('"t"', 'c') const filePath = path.join(tmpDir, '0015_drift.sql') fs.writeFileSync( filePath, @@ -243,6 +271,7 @@ describe('rewriteEncryptedAlterColumns', () => { ] it.each(MANGLED_FORMS)('rewrites the v3 %s form', async (_label, emitted) => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0008_form.sql') fs.writeFileSync( filePath, @@ -265,6 +294,7 @@ describe('rewriteEncryptedAlterColumns', () => { '"undefined"."public.eql_v2_encrypted"', ], ])('rewrites the previously unmatched v2 %s form', async (_label, emitted) => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0009_v2form.sql') fs.writeFileSync( filePath, @@ -281,6 +311,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('names the target domain in the guidance comment', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0010_comment.sql') fs.writeFileSync( filePath, @@ -295,6 +326,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('notes that constraints/defaults/indexes are not carried over', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0016_constraints.sql') fs.writeFileSync( filePath, @@ -309,6 +341,7 @@ describe('rewriteEncryptedAlterColumns', () => { it('does not terminate the commented UPDATE placeholder with a semicolon', async () => { // A runner that naively splits on `;` must not cut mid-comment. + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0017_semicolon.sql') fs.writeFileSync( filePath, @@ -328,6 +361,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('separates ADD/DROP/RENAME with --> statement-breakpoint, one exec stmt per chunk', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0018_breakpoint.sql') fs.writeFileSync( filePath, @@ -353,6 +387,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites each statement to its own domain when v2 and v3 are mixed', async () => { + declarePlaintext('"a"', 'x', 'y') const filePath = path.join(tmpDir, '0011_mixed.sql') fs.writeFileSync( filePath, @@ -495,6 +530,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('reports no skipped statements when the strict rewrite fully handled the file', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0021_handled.sql') fs.writeFileSync( filePath, @@ -577,6 +613,7 @@ describe('rewriteEncryptedAlterColumns', () => { // The comment scan must not be fooled by `--` inside a string literal, or // it would skip a live statement and leave broken SQL to fail at migrate. it('still rewrites an ALTER that follows a "--" inside a string literal', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0030_literal.sql') fs.writeFileSync( filePath, @@ -793,6 +830,7 @@ describe('rewriteEncryptedAlterColumns', () => { // The scoping matters: encrypting `contacts.email` must not be blocked by // an unrelated `users.email` that happens to share a column name. it('scopes the check to the table', async () => { + declarePlaintext('"contacts"', 'email') fs.writeFileSync( path.join(tmpDir, '0000_add.sql'), 'ALTER TABLE "users" ADD COLUMN "email" eql_v3_text_eq;\n', @@ -830,6 +868,7 @@ describe('rewriteEncryptedAlterColumns', () => { // A commented-out ADD never ran, so it says nothing about the live schema. it('ignores an encrypted ADD COLUMN that is commented out', async () => { + declarePlaintext('"users"', 'email') fs.writeFileSync( path.join(tmpDir, '0000_add.sql'), '-- ALTER TABLE "users" ADD COLUMN "email" eql_v3_text_eq;\n', @@ -868,9 +907,42 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([]) expect(skipped[0]?.reason).toBe('already-encrypted') }) + + // Ordering pin: this column is BOTH declared plaintext (0000) and made + // encrypted by a previous sweep (0001). `already-encrypted` is the more + // specific reason and must win over `source-unknown`. + it('reports already-encrypted even when the column was also declared plaintext', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" text);\n', + ) + fs.writeFileSync( + path.join(tmpDir, '0001_swept.sql'), + [ + 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_eq";', + '--> statement-breakpoint', + 'ALTER TABLE "users" DROP COLUMN "email";', + '--> statement-breakpoint', + 'ALTER TABLE "users" RENAME COLUMN "email__cipherstash_tmp" TO "email";', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0002_domain-change.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) }) it('handles multiple ALTER statements in one file', async () => { + declarePlaintext('"a"', 'x', 'y') const original = [ 'ALTER TABLE "a" ALTER COLUMN "x" SET DATA TYPE eql_v2_encrypted;', 'ALTER TABLE "a" ALTER COLUMN "y" SET DATA TYPE eql_v2_encrypted;', @@ -891,6 +963,7 @@ describe('rewriteEncryptedAlterColumns', () => { // Regression pin, not a bug fix — the matchers carry `/gi`, so a // hand-lowercased migration is rewritten just like drizzle-kit's output. it('rewrites a lowercase alter table ... set data type', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0034_lowercase.sql') fs.writeFileSync( filePath, @@ -908,4 +981,146 @@ describe('rewriteEncryptedAlterColumns', () => { expect(updated).toContain('ALTER TABLE "users" DROP COLUMN "email";') expect(updated).not.toMatch(/set data type/i) }) + + // A-2: the sweep is FAIL-CLOSED. A column the corpus never declares is + // UNKNOWN, not plaintext — it may already hold ciphertext, with its + // declaration sitting in a migration directory this sweep never sees. + describe('columns the corpus does not declare', () => { + it('refuses to rewrite a column the corpus never declares', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(`${alterSql}\n`) + expect(updated).not.toContain('DROP COLUMN') + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + it('rewrites a column declared plaintext by an ADD COLUMN', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_add.sql'), + 'ALTER TABLE "users" ADD COLUMN "email" text;\n', + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + // The skipped file is still part of the corpus: a column's current type + // comes from the migrations that ran before this one, edit-eligible or not. + it('counts a declaration living in the file passed to options.skip', async () => { + const install = path.join(tmpDir, '0000_install-eql.sql') + fs.writeFileSync( + install, + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" text);\n', + ) + const alter = path.join(tmpDir, '0002_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns( + tmpDir, + { + skip: install, + }, + ) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + it('follows a RENAME when deciding a column is declared', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email_address" text);', + '--> statement-breakpoint', + 'ALTER TABLE "users" RENAME COLUMN "email_address" TO "email";', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + // A name inside a table/key constraint is a MENTION, not a declaration — + // it is followed by `)` or `,`, never by a type token. Counting one would + // put the rewrite back on the fail-open path. + it('does not treat a name inside PRIMARY KEY / REFERENCES as a declaration', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "sessions" (', + '\t"id" integer,', + '\t"user_id" integer,', + '\tPRIMARY KEY ("id", "email"),', + '\tFOREIGN KEY ("user_id") REFERENCES "users"("email")', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "sessions" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // A column line commented out INSIDE a live CREATE TABLE never ran, so it + // declares nothing. + it('does not count a column commented out inside a live CREATE TABLE', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\t-- "email" text,', + '\t"name" text', + ');', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('source-unknown') + }) + }) }) diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 550cb282a..789ca43a3 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -251,6 +251,40 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( 'gi', ) +/** + * A column name in a DECLARATION position — `"email" text`, + * `"email" "public"."eql_v3_text_search"`, `"amount" numeric(10, 2)`. + * + * The `\s+["a-z]` tail is the whole trick, and it is what separates a + * declaration from a MENTION. Every SQL type token begins with a letter or a + * double quote, so a name followed by `)`, `,` or an operator does not match: + * + * ```sql + * PRIMARY KEY ("id", "name") + * FOREIGN KEY ("org_id") REFERENCES "orgs"("id") + * CHECK ("age" > 0) + * ``` + * + * Deliberately NOT pinned to the encrypted domains. A column the corpus + * declares but does not give an encrypted type is, by residue, plaintext — so + * the fail-closed rule needs no type classification and no SQL parsing, only + * the known encrypted list that {@link ENCRYPTED_TYPE_REF} already provides. + * + * **Known false positive, accepted.** In + * `CONSTRAINT "users_email_unique" UNIQUE("email")` the constraint NAME is + * followed by whitespace and a letter, so it registers as a declared column. It + * is inert unless a constraint name exactly equals the name of a column that is + * also encrypted and undeclared, which drizzle's `
__unique` + * convention makes contrived. + */ +const DECLARED_COLUMN_RE = /"([^"]+)"\s+["a-z]/gi + +/** `ALTER TABLE … ADD COLUMN "col" ` — $1/$2 table, $3 column. */ +const ADD_COLUMN_RE = new RegExp( + String.raw`ALTER TABLE\s+${TABLE_REF}\s+ADD COLUMN\s+(?:IF NOT EXISTS\s+)?"([^"]+)"\s+["a-z]`, + 'gi', +) + /** Splits a `TABLE_REF` capture pair into its schema and table halves. */ function tableOf( first: string, @@ -263,42 +297,72 @@ function tableOf( : { schema: first, table: second } } -/** Identity of a column across the corpus, for {@link indexEncryptedColumns}. */ +/** Identity of a column across the corpus, for {@link indexColumnDeclarations}. */ function columnKey(table: string, column: string, schema?: string): string { return JSON.stringify([schema ?? '', table, column]) } +/** What the migration corpus says about the columns it mentions. */ +interface ColumnIndex { + /** Columns the corpus gives an ENCRYPTED type. Rewriting one drops ciphertext. */ + encrypted: Set + /** Columns the corpus DECLARES at all, whatever type it gives them. */ + declared: Set +} + /** - * Index every column the migration corpus gives an ENCRYPTED type, so the - * rewrite can tell the change it exists for (plaintext → encrypted) from one it - * must never touch (encrypted → encrypted). + * Index what the migration corpus knows about each column, so the rewrite can + * tell the change it exists for (plaintext → encrypted) from the two it must + * never make: encrypted → encrypted, and a change to a column it knows nothing + * about. * - * **Why (#772 review, W-3):** the strict matcher captures only the TARGET type. - * A column whose encrypted domain merely changes (`types.TextEq` → + * **Why `encrypted` (#772 review, W-3):** the strict matcher captures only the + * TARGET type. A column whose encrypted domain merely changes (`types.TextEq` → * `types.TextSearch`) matches just as well as a plaintext column, and the * ADD+DROP+RENAME then drops a column full of CIPHERTEXT — with no plaintext * left anywhere to backfill from, so unlike the plaintext case the data is not - * recoverable from the application at all. Changing an encrypted column's domain - * changes its index terms, so the data has to be re-encrypted through the client - * regardless; the sweep flags the statement and leaves it for the user. + * recoverable from the application at all. + * + * **Why `declared` (A-2):** absence from `encrypted` is not evidence of + * plaintext. It is evidence of nothing. The sweep runs per directory, and the + * shipped wizard default scans three of them, so a column's `CREATE TABLE` can + * simply live somewhere this sweep never reads — the column is then rewritten + * on an assumption, and the ADD+DROP+RENAME drops live ciphertext. Requiring a + * POSITIVE declaration makes the unknown case a flagged statement instead. + * + * Because the encrypted side is matched against the known domain list, the + * plaintext side needs no type classification: a column the corpus declares but + * does not give an encrypted type is plaintext by residue. * * The index is corpus-wide rather than ordered by migration: over-detecting - * "encrypted" costs a flagged statement the user must handle by hand, while - * under-detecting costs irrecoverable ciphertext. Only comment-free statements - * count, for the same reason {@link isInsideCommentOrString} exists. + * "encrypted" costs a flagged statement the user handles by hand, while + * under-detecting costs irrecoverable ciphertext. Only live statements count, + * for the same reason {@link isInsideCommentOrString} exists — inside a + * `CREATE TABLE` the check is applied per column line, so a commented-out + * column in an otherwise live table declares nothing. */ -function indexEncryptedColumns(contents: readonly string[]): Set { +function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { const encrypted = new Set() + const declared = new Set() for (const sql of contents) { for (const created of sql.matchAll(CREATE_TABLE_RE)) { if (isInsideCommentOrString(sql, created.index)) continue const { schema, table } = tableOf(created[1], created[2]) - for (const column of created[3].matchAll( - CREATE_TABLE_ENCRYPTED_COLUMN_RE, - )) { + const body = created[3] + + // 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. + for (const column of body.matchAll(CREATE_TABLE_ENCRYPTED_COLUMN_RE)) { + if (isInsideCommentOrString(body, column.index)) continue encrypted.add(columnKey(table, column[1], 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)) { @@ -307,19 +371,26 @@ function indexEncryptedColumns(contents: readonly string[]): Set { 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]) - if (encrypted.has(columnKey(table, renamed[3], schema))) { - encrypted.add(columnKey(table, renamed[4], schema)) - } + 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) } } - return encrypted + return { encrypted, declared } } /** Why a recognised ALTER-to-encrypted statement was left alone. */ @@ -328,6 +399,8 @@ export type SkipReason = | 'unrecognised-form' /** The column already holds an encrypted domain; rewriting drops ciphertext. */ | 'already-encrypted' + /** The corpus never declares the column, so its current type is unknown. */ + | 'source-unknown' /** A statement the sweep recognised as ALTER-to-encrypted but did NOT rewrite. */ export interface SkippedAlter { @@ -351,6 +424,8 @@ export function describeSkipReason(reason: SkipReason): string { return "the column is ALREADY encrypted, so the ADD+DROP+RENAME rewrite would DROP the ciphertext with no plaintext left to backfill from. Changing an encrypted column's domain changes its index terms, so the data must be re-encrypted through the staged `stash encrypt` lifecycle" case 'unrecognised-form': return 'it falls outside the strict matcher (a hand-authored `SET DATA TYPE ... USING ...`, or a drizzle-kit form the sweep does not recognise) and an in-place cast to an encrypted domain fails at migrate time' + case 'source-unknown': + return "the sweep could not find where this column was declared in this migration directory, so it cannot tell a plaintext column (safe to rewrite) from one that already holds ciphertext (where the rewrite would DROP it). Usually the column's `CREATE TABLE` / `ADD COLUMN` lives in a different directory, or the migration history was squashed. Check the column's current type in the database: if it is plaintext and the table is empty, apply the ADD/DROP/RENAME by hand; if it already holds ciphertext, use the staged `stash encrypt` lifecycle instead" } } @@ -432,7 +507,8 @@ export async function rewriteEncryptedAlterColumns( // Built from the WHOLE corpus, including `options.skip`: a column's current // type comes from the migrations that ran before this one, not just the files // we are allowed to edit. - const encryptedColumns = indexEncryptedColumns([...contents.values()]) + const { encrypted: encryptedColumns, declared: declaredColumns } = + indexColumnDeclarations([...contents.values()]) for (const [filePath, original] of contents) { if (options.skip && filePath === options.skip) continue @@ -463,6 +539,17 @@ export async function rewriteEncryptedAlterColumns( return match } + // Fail closed. Absence from `encryptedColumns` does not mean the column + // is plaintext — it means the corpus never said. The declaration can + // sit in a directory this sweep does not read (the wizard ships with + // three candidates and indexes each separately), so rewriting on the + // assumption is how a live `DROP COLUMN` reaches a populated — possibly + // already-encrypted — column. Flag it and let the user look. + if (!declaredColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'source-unknown') + return match + } + const domain = DOMAIN_RE.exec(mangledType)?.[0]?.toLowerCase() // Unreachable — the outer regex only matches when a domain is present — // but leave the statement alone rather than emit a broken rewrite. From baa9597c893b1be69d40977680c6c1979e69b592 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 21:35:53 +1000 Subject: [PATCH 03/11] fix(cli): close the keyword-mention gap and comment-check regression in the rewrite sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review of the fail-closed rewrite (3fedb33e) found two Important defects, both traced to the plan's own supplied code rather than its transcription: - DECLARED_COLUMN_RE matched any quoted name followed by whitespace and a letter, so a mention inside a predicate or constraint — e.g. CHECK ("email" IS NOT NULL), or a constraint named the same as a column — registered as a declaration with no real declaration ever existing, walking the fail-closed sweep back onto the fail-open path it exists to close. Fixed with a word-boundary-pinned negative lookahead over the SQL keywords reachable in that position, verified against every real type token that shares a keyword's leading letters (interval/inet/int vs IN, char/citext vs CHECK, eql_v2_encrypted/eql_v3_* vs EXCLUDE, etc). - The per-column comment check landed on BOTH body scans inside indexColumnDeclarations, but over-detecting "encrypted" (safe) and over-detecting "declared" (not safe) are not symmetric. Applying it to the encrypted scan turned a commented-out encrypted column inside an otherwise-live CREATE TABLE from over-detected (safe, pre-existing behaviour) to under-detected — exactly the "irrecoverable ciphertext" case the function's own docstring warns against. Removed the check from the encrypted scan only; kept it on the declared scan. Also corrected two doc comments that asserted the old (wrong) behaviour, and three minor JSDoc/comment inaccuracies (skip-reason count, missing source-unknown in the exported contract, a comment naming the wrong set). Added tests pinning both fixes: a CHECK predicate mention and a same-named constraint no longer count as declarations; a type name that merely starts with a blocked keyword's letters (interval/inet) still does; and a commented-out encrypted column beside a live redeclaration still reports already-encrypted rather than being rewritten. --- .../src/__tests__/rewrite-migrations.test.ts | 126 +++++++++++++++++- .../cli/src/commands/db/rewrite-migrations.ts | 89 +++++++++---- 2 files changed, 187 insertions(+), 28 deletions(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index ac38926de..c830b6001 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -939,6 +939,37 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped).toHaveLength(1) expect(skipped[0].reason).toBe('already-encrypted') }) + + // A commented-out encrypted column line inside an otherwise LIVE + // CREATE TABLE must still count as encrypted. The comment check applies + // only to the DECLARED scan (over-detecting plaintext costs data); the + // ENCRYPTED scan never re-checks comments inside a live CREATE TABLE, so + // this stays over-detecting — the safe direction — exactly as it was + // before the corpus learned to track declarations at all. + it('still counts a commented-out encrypted column inside a live CREATE TABLE as encrypted', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer,', + '\t-- "email" "public"."eql_v3_text_eq",', + '\t"email" text', + ');', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) }) it('handles multiple ALTER statements in one file', async () => { @@ -1067,9 +1098,12 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped).toEqual([]) }) - // A name inside a table/key constraint is a MENTION, not a declaration — - // it is followed by `)` or `,`, never by a type token. Counting one would - // put the rewrite back on the fail-open path. + // A name inside a table/key constraint is a MENTION, not a declaration. + // Here every mention is followed by `)` or `,`, which the declaration + // regex's tail alone already rejects. A mention followed by a SQL keyword + // instead (e.g. `CHECK ("email" IS NOT NULL)`) is a separate case, closed + // by the regex's keyword lookahead and pinned by its own tests below. + // Counting either would put the rewrite back on the fail-open path. it('does not treat a name inside PRIMARY KEY / REFERENCES as a declaration', async () => { fs.writeFileSync( path.join(tmpDir, '0000_create.sql'), @@ -1122,5 +1156,91 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped).toHaveLength(1) expect(skipped[0].reason).toBe('source-unknown') }) + + // A CHECK predicate mentioning a column has the same `"name" ` + // shape as a declaration — `"email" IS` — but IS is a predicate keyword, + // not a type token. Without the keyword lookahead this would read as a + // declaration and rewrite the column, dropping any ciphertext it already + // holds via a declaration this sweep never sees. + it('does not treat a CHECK predicate mention as a declaration', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\tCONSTRAINT "c1" CHECK ("email" IS NOT NULL)', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(`${alterSql}\n`) + expect(updated).not.toContain('DROP COLUMN') + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // A constraint's NAME can coincide with a column's name — drizzle's + // `
__unique` convention makes this contrived, but the keyword + // lookahead closes it regardless: a constraint name is always followed + // immediately by its constraint-type keyword (UNIQUE here), which the + // lookahead excludes. + it('does not let a same-named constraint declare the column', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\tCONSTRAINT "email" UNIQUE("id")', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // The keyword lookahead is pinned to a word boundary specifically so it + // does not eat real type names that merely START WITH a blocked + // keyword's letters: `interval`/`inet` both start "in" (colliding with + // IN) but must still count as genuine declarations. + it('still declares a column whose type name starts with a blocked keyword', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "events" ("email" interval, "note" inet);\n', + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + [ + 'ALTER TABLE "events" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + 'ALTER TABLE "events" ALTER COLUMN "note" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) }) }) diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 789ca43a3..7e1de20ec 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -255,29 +255,57 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( * A column name in a DECLARATION position — `"email" text`, * `"email" "public"."eql_v3_text_search"`, `"amount" numeric(10, 2)`. * - * The `\s+["a-z]` tail is the whole trick, and it is what separates a - * declaration from a MENTION. Every SQL type token begins with a letter or a - * double quote, so a name followed by `)`, `,` or an operator does not match: + * The `\s+["a-z]` tail is what separates a declaration from a MENTION. Most + * mentions are followed by `)`, `,`, or an operator, which the tail alone + * already rejects: * * ```sql * PRIMARY KEY ("id", "name") * FOREIGN KEY ("org_id") REFERENCES "orgs"("id") - * CHECK ("age" > 0) * ``` * + * But a mention inside a predicate or a table-level constraint is often + * followed by whitespace and a SQL KEYWORD — which has exactly the same + * `\s+[a-z]` shape as a real declaration, and drizzle-kit emits both forms + * inside a `CREATE TABLE` (a `CONSTRAINT … CHECK ()` body is + * user-authored, so the predicate form is reachable too): + * + * ```sql + * CHECK ("email" IS NOT NULL) + * CONSTRAINT "users_email_unique" UNIQUE ("email") + * ``` + * + * The negative lookahead rejects the keywords reachable this way: predicate + * keywords (`IS`, `IN`, `NOT`, `LIKE`, `ILIKE`, `BETWEEN`, `AND`, `OR`), + * ordering/collation (`COLLATE`, `ASC`, `DESC`, `NULLS`), and every + * table/column-constraint keyword (`UNIQUE`, `PRIMARY`, `FOREIGN`, `CHECK`, + * `EXCLUDE`, `REFERENCES`, `CONSTRAINT`, `USING`, `WITH`) — the last six also + * close the constraint-NAME case above, since Postgres always puts one of + * them immediately after a constraint's name. + * + * A real type token still matches because the lookahead is pinned to a word + * boundary (`\b`) right after each keyword, so it only rejects a keyword when + * that keyword is the WHOLE next word: `interval` and `inet` both start with + * `IN`'s letters, but their third character keeps them inside one word, so + * `\bIN\b` never matches there; the same reasoning clears `int`, + * `char`/`citext` against `CHECK`, and `eql_v2_encrypted`/`eql_v3_*` against + * `EXCLUDE`. + * * Deliberately NOT pinned to the encrypted domains. A column the corpus * declares but does not give an encrypted type is, by residue, plaintext — so * the fail-closed rule needs no type classification and no SQL parsing, only * the known encrypted list that {@link ENCRYPTED_TYPE_REF} already provides. * - * **Known false positive, accepted.** In - * `CONSTRAINT "users_email_unique" UNIQUE("email")` the constraint NAME is - * followed by whitespace and a letter, so it registers as a declared column. It - * is inert unless a constraint name exactly equals the name of a column that is - * also encrypted and undeclared, which drizzle's `
__unique` - * convention makes contrived. + * **Residue, accepted.** The lookahead is a fixed keyword list, not a parser: + * a predicate keyword it does not enumerate (`SIMILAR`, `ISNULL`, `NOTNULL`, + * `OVERLAPS`, …) still lets a bare mention through, e.g. + * `CHECK ("email" SIMILAR TO '...')`. This is inert unless that mention's name + * exactly matches a DIFFERENT column that is both encrypted and genuinely + * undeclared anywhere else in the corpus — contrived, and strictly narrower + * than the gap this replaces. */ -const DECLARED_COLUMN_RE = /"([^"]+)"\s+["a-z]/gi +const DECLARED_COLUMN_RE = + /"([^"]+)"\s+(?!(?:IS|IN|NOT|LIKE|ILIKE|BETWEEN|AND|OR|COLLATE|ASC|DESC|NULLS|UNIQUE|PRIMARY|FOREIGN|CHECK|EXCLUDE|REFERENCES|CONSTRAINT|USING|WITH)\b)["a-z]/gi /** `ALTER TABLE … ADD COLUMN "col" ` — $1/$2 table, $3 column. */ const ADD_COLUMN_RE = new RegExp( @@ -336,10 +364,10 @@ interface ColumnIndex { * * The index is corpus-wide rather than ordered by migration: over-detecting * "encrypted" costs a flagged statement the user handles by hand, while - * under-detecting costs irrecoverable ciphertext. Only live statements count, - * for the same reason {@link isInsideCommentOrString} exists — inside a - * `CREATE TABLE` the check is applied per column line, so a commented-out - * column in an otherwise live table declares nothing. + * under-detecting costs irrecoverable ciphertext. That asymmetry is why the + * two per-column scans inside a `CREATE TABLE` body are deliberately + * inconsistent about comments (see the comment at the loops themselves) — + * do not "fix" that inconsistency, it is the point. */ function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { const encrypted = new Set() @@ -354,8 +382,16 @@ function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { // 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)) { - if (isInsideCommentOrString(body, column.index)) continue encrypted.add(columnKey(table, column[1], schema)) } @@ -415,7 +451,7 @@ export interface SkippedAlter { /** * One-line explanation of a {@link SkipReason}, for the CLI/wizard to print * next to the statement. Lives here so every caller says the same thing — the - * two reasons need very different action from the user, and a single generic + * three reasons need very different action from the user, and a single generic * "could not rewrite automatically" hides that. */ export function describeSkipReason(reason: SkipReason): string { @@ -461,13 +497,16 @@ export interface RewriteResult { * * Returns {@link RewriteResult}: the files rewritten, plus `skipped` statements * left for a human — ones outside the strict matcher (a hand-authored - * `SET DATA TYPE … USING …;`, or a future drizzle-kit form), and ones targeting - * a column that is ALREADY encrypted, where the rewrite would drop ciphertext. - * Both are left untouched on disk and surfaced non-fatally so the caller can - * tell the user to review them, rather than silently shipping broken SQL or - * destroying data. Statements sitting inside a SQL comment — or inside a - * single-quoted string literal, where they are data rather than SQL — are inert - * and are neither rewritten nor reported. + * `SET DATA TYPE … USING …;`, or a future drizzle-kit form); ones targeting a + * column that is ALREADY encrypted, where the rewrite would drop ciphertext; + * and ones targeting a column the corpus never DECLARES anywhere, where the + * rewrite would be guessing at a type it cannot see (likely the most common + * skip on a real corpus, since the sweep only ever reads part of the + * migration history). All three are left untouched on disk and surfaced + * non-fatally so the caller can tell the user to review them, rather than + * silently shipping broken SQL or destroying data. Statements sitting inside a + * SQL comment — or inside a single-quoted string literal, where they are data + * rather than SQL — are inert and are neither rewritten nor reported. */ export async function rewriteEncryptedAlterColumns( outDir: string, @@ -539,7 +578,7 @@ export async function rewriteEncryptedAlterColumns( return match } - // Fail closed. Absence from `encryptedColumns` does not mean the column + // Fail closed. Absence from `declaredColumns` does not mean the column // is plaintext — it means the corpus never said. The declaration can // sit in a directory this sweep does not read (the wizard ships with // three candidates and indexes each separately), so rewriting on the From 84e9e672d4e91a2191cf4f8d339c9f0844d227ff Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 21:56:24 +1000 Subject: [PATCH 04/11] fix(wizard): rewrite an encrypted ALTER only for a column the corpus declares Mirrors the CLI fail-closed fix (3fedb33e, bcc573c6) into the wizard's copy of the sweep. This is where the fail-open default bit hardest: the wizard ships scanning drizzle/, migrations/ and src/db/migrations/ with a per-directory index, so a column declared in one directory and altered in another was rewritten on an assumption, and the ADD+DROP+ RENAME dropped live ciphertext with nothing anywhere to backfill from. Also documents two residual gaps found in Task 1's review, in both copies: a REFERENCES ... ON DELETE CASCADE mention can coincide with a column name (DECLARED_COLUMN_RE's keyword lookahead doesn't cover ON), and ADD_COLUMN_RE needs no such lookahead since the token after an ADD COLUMN's column name is always its type. Ports the CLI's full current test coverage (not just the original 7 tests, since a second review-fix commit added 4 more pinning the keyword lookahead and the comment-check asymmetry) plus a wizard-specific regression test for the per-directory index gap. --- .../cli/src/commands/db/rewrite-migrations.ts | 23 +- .../src/__tests__/rewrite-migrations.test.ts | 375 +++++++++++++++++- packages/wizard/src/lib/rewrite-migrations.ts | 195 +++++++-- 3 files changed, 556 insertions(+), 37 deletions(-) diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 7e1de20ec..e82e5cedf 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -298,16 +298,27 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( * * **Residue, accepted.** The lookahead is a fixed keyword list, not a parser: * a predicate keyword it does not enumerate (`SIMILAR`, `ISNULL`, `NOTNULL`, - * `OVERLAPS`, …) still lets a bare mention through, e.g. - * `CHECK ("email" SIMILAR TO '...')`. This is inert unless that mention's name - * exactly matches a DIFFERENT column that is both encrypted and genuinely - * undeclared anywhere else in the corpus — contrived, and strictly narrower - * than the gap this replaces. + * `OVERLAPS`, `ON`, …) still lets a bare mention through, e.g. + * `CHECK ("email" SIMILAR TO '...')`, or a `REFERENCES "email" ON DELETE + * CASCADE` where `"email"` names the referenced TABLE rather than the column + * being declared — `"id" integer REFERENCES "email" ON DELETE CASCADE` inside + * a `CREATE TABLE` body still registers `email` as declared. That one only + * bites when a column shares a referenced table's name, so like the rest of + * this residue it is inert unless that mention's name exactly matches a + * DIFFERENT column that is both encrypted and genuinely undeclared anywhere + * else in the corpus — contrived, and strictly narrower than the gap this + * replaces. */ const DECLARED_COLUMN_RE = /"([^"]+)"\s+(?!(?:IS|IN|NOT|LIKE|ILIKE|BETWEEN|AND|OR|COLLATE|ASC|DESC|NULLS|UNIQUE|PRIMARY|FOREIGN|CHECK|EXCLUDE|REFERENCES|CONSTRAINT|USING|WITH)\b)["a-z]/gi -/** `ALTER TABLE … ADD COLUMN "col" ` — $1/$2 table, $3 column. */ +/** + * `ALTER TABLE … ADD COLUMN "col" ` — $1/$2 table, $3 column. + * + * Needs no {@link DECLARED_COLUMN_RE}-style keyword lookahead: the token + * right after an ADD COLUMN's column name is always its type, so no SQL + * keyword can occupy that position. + */ const ADD_COLUMN_RE = new RegExp( String.raw`ALTER TABLE\s+${TABLE_REF}\s+ADD COLUMN\s+(?:IF NOT EXISTS\s+)?"([^"]+)"\s+["a-z]`, 'gi', diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index f5ef3ee45..9edc41bb3 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -18,7 +18,28 @@ describe('rewriteEncryptedAlterColumns', () => { fs.rmSync(tmpDir, { recursive: true, force: true }) }) + /** + * Declare `columns` on `tableRef` as PLAINTEXT, in a migration that sorts + * before every fixture below. + * + * The sweep is fail-closed: it rewrites a column only when the corpus shows + * the column exists and is not already encrypted. A fixture that is just an + * ALTER declares nothing, so it is `source-unknown` by design — a test that + * exercises the REWRITE has to supply the `CREATE TABLE` a real drizzle + * corpus would carry. + * + * `tableRef` is written exactly as it appears in the ALTER, so a pgSchema() + * table passes `'"app"."users"'` and the declaration lands on the same key. + */ + const declarePlaintext = (tableRef: string, ...columns: string[]): void => { + const file = path.join(tmpDir, '0000_declare.sql') + const existing = fs.existsSync(file) ? fs.readFileSync(file, 'utf-8') : '' + const defs = columns.map((column) => `"${column}" text`).join(', ') + fs.writeFileSync(file, `${existing}CREATE TABLE ${tableRef} (${defs});\n`) + } + it('rewrites an in-place ALTER COLUMN with the bare v2 type name', async () => { + declarePlaintext('"transactions"', 'amount') const original = `ALTER TABLE "transactions" ALTER COLUMN "amount" SET DATA TYPE eql_v2_encrypted;\n` const filePath = path.join(tmpDir, '0002_alter.sql') fs.writeFileSync(filePath, original) @@ -42,6 +63,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites a bare v3 domain (the generation the wizard now scaffolds)', async () => { + declarePlaintext('"users"', 'email') const original = `ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n` const filePath = path.join(tmpDir, '0002_v3.sql') fs.writeFileSync(filePath, original) @@ -57,6 +79,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the schema-qualified form produced by drizzle-kit', async () => { + declarePlaintext('"users"', 'email') const original = 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "public"."eql_v3_text_search";\n' const filePath = path.join(tmpDir, '0003_alter.sql') @@ -74,6 +97,7 @@ describe('rewriteEncryptedAlterColumns', () => { it('rewrites a schema-qualified table produced by pgSchema()', async () => { // drizzle-kit emits `"app"."users"` for a table declared in a pgSchema(); // the old `\s+` between the table and ALTER COLUMN could never cross the `.`. + declarePlaintext('"app"."users"', 'email') const original = 'ALTER TABLE "app"."users" ALTER COLUMN "email" SET DATA TYPE "undefined"."eql_v3_text_search";\n' const filePath = path.join(tmpDir, '0014_qualified.sql') @@ -95,6 +119,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the "undefined" schema form drizzle-kit emits for bare custom types', async () => { + declarePlaintext('"transactions"', 'amount') const original = 'ALTER TABLE "transactions" ALTER COLUMN "amount" SET DATA TYPE "undefined"."eql_v2_encrypted";\n' const filePath = path.join(tmpDir, '0005_undef.sql') @@ -110,6 +135,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites the double-quoted form produced by stack 0.15.0', async () => { + declarePlaintext('"transactions"', 'description') const original = 'ALTER TABLE "transactions" ALTER COLUMN "description" SET DATA TYPE "undefined".""public"."eql_v2_encrypted"";\n' const filePath = path.join(tmpDir, '0006_double.sql') @@ -137,6 +163,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('skips the file passed in options.skip', async () => { + declarePlaintext('"t"', 'c') const install = path.join(tmpDir, '0000_install-eql.sql') const alter = path.join(tmpDir, '0002_alter.sql') fs.writeFileSync(install, 'CREATE SCHEMA eql_v2;\n') @@ -190,6 +217,7 @@ describe('rewriteEncryptedAlterColumns', () => { ] it.each(V3_DOMAINS)('rewrites an ALTER COLUMN to %s', async (domain) => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0007_v3.sql') fs.writeFileSync( filePath, @@ -217,6 +245,7 @@ describe('rewriteEncryptedAlterColumns', () => { ...V3_DOMAINS, 'eql_v2_encrypted', ])('extracts the bare domain %s from a mangled ALTER', async (domain) => { + declarePlaintext('"t"', 'c') const filePath = path.join(tmpDir, '0015_drift.sql') fs.writeFileSync( filePath, @@ -255,6 +284,7 @@ describe('rewriteEncryptedAlterColumns', () => { ] it.each(MANGLED_FORMS)('rewrites the v3 %s form', async (_label, emitted) => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0008_form.sql') fs.writeFileSync( filePath, @@ -271,6 +301,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('names the target domain in the guidance comment', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0010_comment.sql') fs.writeFileSync( filePath, @@ -284,6 +315,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('warns that the rewrite is data-destroying / empty-table-only', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0016_warn.sql') fs.writeFileSync( filePath, @@ -299,6 +331,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('separates ADD/DROP/RENAME with --> statement-breakpoint', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0018_breakpoint.sql') fs.writeFileSync( filePath, @@ -316,6 +349,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('rewrites each statement to its own domain when v2 and v3 are mixed', async () => { + declarePlaintext('"a"', 'x', 'y') const filePath = path.join(tmpDir, '0011_mixed.sql') fs.writeFileSync( filePath, @@ -370,6 +404,7 @@ describe('rewriteEncryptedAlterColumns', () => { }) it('reports no skipped statements when the strict rewrite fully handled the file', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0021_handled.sql') fs.writeFileSync( filePath, @@ -508,6 +543,7 @@ describe('rewriteEncryptedAlterColumns', () => { // The comment scan must not be fooled by `--` inside a string literal, or // it would skip a live statement and leave broken SQL to fail at migrate. it('still rewrites an ALTER that follows a "--" inside a string literal', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0030_literal.sql') fs.writeFileSync( filePath, @@ -724,6 +760,7 @@ describe('rewriteEncryptedAlterColumns', () => { // The scoping matters: encrypting `contacts.email` must not be blocked by // an unrelated `users.email` that happens to share a column name. it('scopes the check to the table', async () => { + declarePlaintext('"contacts"', 'email') fs.writeFileSync( path.join(tmpDir, '0000_add.sql'), 'ALTER TABLE "users" ADD COLUMN "email" eql_v3_text_eq;\n', @@ -761,6 +798,7 @@ describe('rewriteEncryptedAlterColumns', () => { // A commented-out ADD never ran, so it says nothing about the live schema. it('ignores an encrypted ADD COLUMN that is commented out', async () => { + declarePlaintext('"users"', 'email') fs.writeFileSync( path.join(tmpDir, '0000_add.sql'), '-- ALTER TABLE "users" ADD COLUMN "email" eql_v3_text_eq;\n', @@ -799,9 +837,73 @@ describe('rewriteEncryptedAlterColumns', () => { expect(rewritten).toEqual([]) expect(skipped[0]?.reason).toBe('already-encrypted') }) + + // Ordering pin: this column is BOTH declared plaintext (0000) and made + // encrypted by a previous sweep (0001). `already-encrypted` is the more + // specific reason and must win over `source-unknown`. + it('reports already-encrypted even when the column was also declared plaintext', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" text);\n', + ) + fs.writeFileSync( + path.join(tmpDir, '0001_swept.sql'), + [ + 'ALTER TABLE "users" ADD COLUMN "email__cipherstash_tmp" "public"."eql_v3_text_eq";', + '--> statement-breakpoint', + 'ALTER TABLE "users" DROP COLUMN "email";', + '--> statement-breakpoint', + 'ALTER TABLE "users" RENAME COLUMN "email__cipherstash_tmp" TO "email";', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0002_domain-change.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) + + // A commented-out encrypted column line inside an otherwise LIVE + // CREATE TABLE must still count as encrypted. The comment check applies + // only to the DECLARED scan (over-detecting plaintext costs data); the + // ENCRYPTED scan never re-checks comments inside a live CREATE TABLE, so + // this stays over-detecting — the safe direction — exactly as it was + // before the corpus learned to track declarations at all. + it('still counts a commented-out encrypted column inside a live CREATE TABLE as encrypted', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer,', + '\t-- "email" "public"."eql_v3_text_eq",', + '\t"email" text', + ');', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('already-encrypted') + }) }) it('handles multiple ALTER statements in one file', async () => { + declarePlaintext('"a"', 'x', 'y') const original = [ 'ALTER TABLE "a" ALTER COLUMN "x" SET DATA TYPE eql_v3_text_search;', 'ALTER TABLE "a" ALTER COLUMN "y" SET DATA TYPE eql_v3_text_search;', @@ -822,6 +924,7 @@ describe('rewriteEncryptedAlterColumns', () => { // Regression pin, not a bug fix — the matchers carry `/gi`, so a // hand-lowercased migration is rewritten just like drizzle-kit's output. it('rewrites a lowercase alter table ... set data type', async () => { + declarePlaintext('"users"', 'email') const filePath = path.join(tmpDir, '0034_lowercase.sql') fs.writeFileSync( filePath, @@ -839,13 +942,250 @@ describe('rewriteEncryptedAlterColumns', () => { expect(updated).toContain('ALTER TABLE "users" DROP COLUMN "email";') expect(updated).not.toMatch(/set data type/i) }) + + // A-2: the sweep is FAIL-CLOSED. A column the corpus never declares is + // UNKNOWN, not plaintext — it may already hold ciphertext, with its + // declaration sitting in a migration directory this sweep never sees. + describe('columns the corpus does not declare', () => { + it('refuses to rewrite a column the corpus never declares', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(`${alterSql}\n`) + expect(updated).not.toContain('DROP COLUMN') + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + it('rewrites a column declared plaintext by an ADD COLUMN', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_add.sql'), + 'ALTER TABLE "users" ADD COLUMN "email" text;\n', + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + // The skipped file is still part of the corpus: a column's current type + // comes from the migrations that ran before this one, edit-eligible or not. + it('counts a declaration living in the file passed to options.skip', async () => { + const install = path.join(tmpDir, '0000_install-eql.sql') + fs.writeFileSync( + install, + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" text);\n', + ) + const alter = path.join(tmpDir, '0002_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns( + tmpDir, + { + skip: install, + }, + ) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + it('follows a RENAME when deciding a column is declared', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email_address" text);', + '--> statement-breakpoint', + 'ALTER TABLE "users" RENAME COLUMN "email_address" TO "email";', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + + // A name inside a table/key constraint is a MENTION, not a declaration. + // Here every mention is followed by `)` or `,`, which the declaration + // regex's tail alone already rejects. A mention followed by a SQL keyword + // instead (e.g. `CHECK ("email" IS NOT NULL)`) is a separate case, closed + // by the regex's keyword lookahead and pinned by its own tests below. + // Counting either would put the rewrite back on the fail-open path. + it('does not treat a name inside PRIMARY KEY / REFERENCES as a declaration', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "sessions" (', + '\t"id" integer,', + '\t"user_id" integer,', + '\tPRIMARY KEY ("id", "email"),', + '\tFOREIGN KEY ("user_id") REFERENCES "users"("email")', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "sessions" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // A column line commented out INSIDE a live CREATE TABLE never ran, so it + // declares nothing. + it('does not count a column commented out inside a live CREATE TABLE', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\t-- "email" text,', + '\t"name" text', + ');', + '', + ].join('\n'), + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n', + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toHaveLength(1) + expect(skipped[0].reason).toBe('source-unknown') + }) + + // A CHECK predicate mentioning a column has the same `"name" ` + // shape as a declaration — `"email" IS` — but IS is a predicate keyword, + // not a type token. Without the keyword lookahead this would read as a + // declaration and rewrite the column, dropping any ciphertext it already + // holds via a declaration this sweep never sees. + it('does not treat a CHECK predicate mention as a declaration', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\tCONSTRAINT "c1" CHECK ("email" IS NOT NULL)', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(`${alterSql}\n`) + expect(updated).not.toContain('DROP COLUMN') + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // A constraint's NAME can coincide with a column's name — drizzle's + // `
__unique` convention makes this contrived, but the keyword + // lookahead closes it regardless: a constraint name is always followed + // immediately by its constraint-type keyword (UNIQUE here), which the + // lookahead excludes. + it('does not let a same-named constraint declare the column', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + [ + 'CREATE TABLE "users" (', + '\t"id" integer PRIMARY KEY,', + '\tCONSTRAINT "email" UNIQUE("id")', + ');', + '', + ].join('\n'), + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync(alter, `${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // The keyword lookahead is pinned to a word boundary specifically so it + // does not eat real type names that merely START WITH a blocked + // keyword's letters: `interval`/`inet` both start "in" (colliding with + // IN) but must still count as genuine declarations. + it('still declares a column whose type name starts with a blocked keyword', async () => { + fs.writeFileSync( + path.join(tmpDir, '0000_create.sql'), + 'CREATE TABLE "events" ("email" interval, "note" inet);\n', + ) + const alter = path.join(tmpDir, '0001_encrypt.sql') + fs.writeFileSync( + alter, + [ + 'ALTER TABLE "events" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + 'ALTER TABLE "events" ALTER COLUMN "note" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([alter]) + expect(skipped).toEqual([]) + }) + }) }) describe('sweepMigrationDirs', () => { let tmpDir: string - const ALTER = - 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;\n' + const ALTER = [ + // Fail-closed: the sweep rewrites only a column the corpus DECLARES, and a + // real drizzle corpus carries the CREATE that declared it. + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" text);', + '--> statement-breakpoint', + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;', + '', + ].join('\n') /** Create `dir` under the sandbox, optionally seeding `name` with `sql`. */ const seedDir = (dir: string, name?: string, sql?: string): string => { @@ -945,4 +1285,35 @@ describe('sweepMigrationDirs', () => { expect(results[0].skipped).toHaveLength(1) expect(results[0].skipped[0].file).toBe(path.join(abs, '0001_using.sql')) }) + + // A-2 trigger (b). The wizard ships scanning three candidate directories and + // indexes each separately, so a declaration in `drizzle/` is invisible to the + // sweep of `migrations/`. That column is already ciphertext; rewriting it + // emits a live DROP COLUMN with nothing anywhere to backfill from. + it('does not rewrite an ALTER whose column is declared in a sibling directory', async () => { + seedDir( + 'drizzle', + '0000_create.sql', + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" "public"."eql_v3_text_eq");\n', + ) + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const migrations = seedDir( + 'migrations', + '0001_domain-change.sql', + `${alterSql}\n`, + ) + const alter = path.join(migrations, '0001_domain-change.sql') + + const results = await sweepMigrationDirs(tmpDir, ['drizzle', 'migrations']) + + const swept = results.find((result) => result.dir === 'migrations') + expect(swept?.rewritten).toEqual([]) + expect(swept?.skipped).toEqual([ + { file: alter, statement: alterSql, reason: 'source-unknown' }, + ]) + const updated = fs.readFileSync(alter, 'utf-8') + expect(updated).toBe(`${alterSql}\n`) + expect(updated).not.toContain('DROP COLUMN') + }) }) diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 73e1908b7..4fb879f89 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -259,6 +259,79 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( 'gi', ) +/** + * A column name in a DECLARATION position — `"email" text`, + * `"email" "public"."eql_v3_text_search"`, `"amount" numeric(10, 2)`. + * + * The `\s+["a-z]` tail is what separates a declaration from a MENTION. Most + * mentions are followed by `)`, `,`, or an operator, which the tail alone + * already rejects: + * + * ```sql + * PRIMARY KEY ("id", "name") + * FOREIGN KEY ("org_id") REFERENCES "orgs"("id") + * ``` + * + * But a mention inside a predicate or a table-level constraint is often + * followed by whitespace and a SQL KEYWORD — which has exactly the same + * `\s+[a-z]` shape as a real declaration, and drizzle-kit emits both forms + * inside a `CREATE TABLE` (a `CONSTRAINT … CHECK ()` body is + * user-authored, so the predicate form is reachable too): + * + * ```sql + * CHECK ("email" IS NOT NULL) + * CONSTRAINT "users_email_unique" UNIQUE ("email") + * ``` + * + * The negative lookahead rejects the keywords reachable this way: predicate + * keywords (`IS`, `IN`, `NOT`, `LIKE`, `ILIKE`, `BETWEEN`, `AND`, `OR`), + * ordering/collation (`COLLATE`, `ASC`, `DESC`, `NULLS`), and every + * table/column-constraint keyword (`UNIQUE`, `PRIMARY`, `FOREIGN`, `CHECK`, + * `EXCLUDE`, `REFERENCES`, `CONSTRAINT`, `USING`, `WITH`) — the last six also + * close the constraint-NAME case above, since Postgres always puts one of + * them immediately after a constraint's name. + * + * A real type token still matches because the lookahead is pinned to a word + * boundary (`\b`) right after each keyword, so it only rejects a keyword when + * that keyword is the WHOLE next word: `interval` and `inet` both start with + * `IN`'s letters, but their third character keeps them inside one word, so + * `\bIN\b` never matches there; the same reasoning clears `int`, + * `char`/`citext` against `CHECK`, and `eql_v2_encrypted`/`eql_v3_*` against + * `EXCLUDE`. + * + * Deliberately NOT pinned to the encrypted domains. A column the corpus + * declares but does not give an encrypted type is, by residue, plaintext — so + * the fail-closed rule needs no type classification and no SQL parsing, only + * the known encrypted list that {@link ENCRYPTED_TYPE_REF} already provides. + * + * **Residue, accepted.** The lookahead is a fixed keyword list, not a parser: + * a predicate keyword it does not enumerate (`SIMILAR`, `ISNULL`, `NOTNULL`, + * `OVERLAPS`, `ON`, …) still lets a bare mention through, e.g. + * `CHECK ("email" SIMILAR TO '...')`, or a `REFERENCES "email" ON DELETE + * CASCADE` where `"email"` names the referenced TABLE rather than the column + * being declared — `"id" integer REFERENCES "email" ON DELETE CASCADE` inside + * a `CREATE TABLE` body still registers `email` as declared. That one only + * bites when a column shares a referenced table's name, so like the rest of + * this residue it is inert unless that mention's name exactly matches a + * DIFFERENT column that is both encrypted and genuinely undeclared anywhere + * else in the corpus — contrived, and strictly narrower than the gap this + * replaces. + */ +const DECLARED_COLUMN_RE = + /"([^"]+)"\s+(?!(?:IS|IN|NOT|LIKE|ILIKE|BETWEEN|AND|OR|COLLATE|ASC|DESC|NULLS|UNIQUE|PRIMARY|FOREIGN|CHECK|EXCLUDE|REFERENCES|CONSTRAINT|USING|WITH)\b)["a-z]/gi + +/** + * `ALTER TABLE … ADD COLUMN "col" ` — $1/$2 table, $3 column. + * + * Needs no {@link DECLARED_COLUMN_RE}-style keyword lookahead: the token + * right after an ADD COLUMN's column name is always its type, so no SQL + * keyword can occupy that position. + */ +const ADD_COLUMN_RE = new RegExp( + String.raw`ALTER TABLE\s+${TABLE_REF}\s+ADD COLUMN\s+(?:IF NOT EXISTS\s+)?"([^"]+)"\s+["a-z]`, + 'gi', +) + /** Splits a `TABLE_REF` capture pair into its schema and table halves. */ function tableOf( first: string, @@ -271,42 +344,80 @@ function tableOf( : { schema: first, table: second } } -/** Identity of a column across the corpus, for {@link indexEncryptedColumns}. */ +/** Identity of a column across the corpus, for {@link indexColumnDeclarations}. */ function columnKey(table: string, column: string, schema?: string): string { return JSON.stringify([schema ?? '', table, column]) } +/** What the migration corpus says about the columns it mentions. */ +interface ColumnIndex { + /** Columns the corpus gives an ENCRYPTED type. Rewriting one drops ciphertext. */ + encrypted: Set + /** Columns the corpus DECLARES at all, whatever type it gives them. */ + declared: Set +} + /** - * Index every column the migration corpus gives an ENCRYPTED type, so the - * rewrite can tell the change it exists for (plaintext → encrypted) from one it - * must never touch (encrypted → encrypted). + * Index what the migration corpus knows about each column, so the rewrite can + * tell the change it exists for (plaintext → encrypted) from the two it must + * never make: encrypted → encrypted, and a change to a column it knows nothing + * about. * - * **Why (#772 review, W-3):** the strict matcher captures only the TARGET type. - * A column whose encrypted domain merely changes (`types.TextEq` → + * **Why `encrypted` (#772 review, W-3):** the strict matcher captures only the + * TARGET type. A column whose encrypted domain merely changes (`types.TextEq` → * `types.TextSearch`) matches just as well as a plaintext column, and the * ADD+DROP+RENAME then drops a column full of CIPHERTEXT — with no plaintext * left anywhere to backfill from, so unlike the plaintext case the data is not - * recoverable from the application at all. Changing an encrypted column's domain - * changes its index terms, so the data has to be re-encrypted through the client - * regardless; the sweep flags the statement and leaves it for the user. + * recoverable from the application at all. + * + * **Why `declared` (A-2):** absence from `encrypted` is not evidence of + * plaintext. It is evidence of nothing. The sweep runs per directory, and the + * shipped wizard default scans three of them, so a column's `CREATE TABLE` can + * simply live somewhere this sweep never reads — the column is then rewritten + * on an assumption, and the ADD+DROP+RENAME drops live ciphertext. Requiring a + * POSITIVE declaration makes the unknown case a flagged statement instead. + * + * Because the encrypted side is matched against the known domain list, the + * plaintext side needs no type classification: a column the corpus declares but + * does not give an encrypted type is plaintext by residue. * * The index is corpus-wide rather than ordered by migration: over-detecting - * "encrypted" costs a flagged statement the user must handle by hand, while - * under-detecting costs irrecoverable ciphertext. Only comment-free statements - * count, for the same reason {@link isInsideCommentOrString} exists. + * "encrypted" costs a flagged statement the user handles by hand, while + * under-detecting costs irrecoverable ciphertext. That asymmetry is why the + * two per-column scans inside a `CREATE TABLE` body are deliberately + * inconsistent about comments (see the comment at the loops themselves) — + * do not "fix" that inconsistency, it is the point. */ -function indexEncryptedColumns(contents: readonly string[]): Set { +function indexColumnDeclarations(contents: readonly string[]): ColumnIndex { const encrypted = new Set() + const declared = new Set() for (const sql of contents) { for (const created of sql.matchAll(CREATE_TABLE_RE)) { if (isInsideCommentOrString(sql, created.index)) continue const { schema, table } = tableOf(created[1], created[2]) - for (const column of created[3].matchAll( - CREATE_TABLE_ENCRYPTED_COLUMN_RE, - )) { + const body = created[3] + + // 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 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)) { @@ -315,19 +426,26 @@ function indexEncryptedColumns(contents: readonly string[]): Set { 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]) - if (encrypted.has(columnKey(table, renamed[3], schema))) { - encrypted.add(columnKey(table, renamed[4], schema)) - } + 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) } } - return encrypted + return { encrypted, declared } } /** Why a recognised ALTER-to-encrypted statement was left alone. */ @@ -336,6 +454,8 @@ export type SkipReason = | 'unrecognised-form' /** The column already holds an encrypted domain; rewriting drops ciphertext. */ | 'already-encrypted' + /** The corpus never declares the column, so its current type is unknown. */ + | 'source-unknown' /** A statement the sweep recognised as ALTER-to-encrypted but did NOT rewrite. */ export interface SkippedAlter { @@ -350,7 +470,7 @@ export interface SkippedAlter { /** * One-line explanation of a {@link SkipReason}, for the CLI/wizard to print * next to the statement. Lives here so every caller says the same thing — the - * two reasons need very different action from the user, and a single generic + * three reasons need very different action from the user, and a single generic * "could not rewrite automatically" hides that. */ export function describeSkipReason(reason: SkipReason): string { @@ -359,6 +479,8 @@ export function describeSkipReason(reason: SkipReason): string { return "the column is ALREADY encrypted, so the ADD+DROP+RENAME rewrite would DROP the ciphertext with no plaintext left to backfill from. Changing an encrypted column's domain changes its index terms, so the data must be re-encrypted through the staged `stash encrypt` lifecycle" case 'unrecognised-form': return 'it falls outside the strict matcher (a hand-authored `SET DATA TYPE ... USING ...`, or a drizzle-kit form the sweep does not recognise) and an in-place cast to an encrypted domain fails at migrate time' + case 'source-unknown': + return "the sweep could not find where this column was declared in this migration directory, so it cannot tell a plaintext column (safe to rewrite) from one that already holds ciphertext (where the rewrite would DROP it). Usually the column's `CREATE TABLE` / `ADD COLUMN` lives in a different directory, or the migration history was squashed. Check the column's current type in the database: if it is plaintext and the table is empty, apply the ADD/DROP/RENAME by hand; if it already holds ciphertext, use the staged `stash encrypt` lifecycle instead" } } @@ -394,13 +516,16 @@ export interface RewriteResult { * * Returns {@link RewriteResult}: the files rewritten, plus `skipped` statements * left for a human — ones outside the strict matcher (a hand-authored - * `SET DATA TYPE … USING …;`, or a future drizzle-kit form), and ones targeting - * a column that is ALREADY encrypted, where the rewrite would drop ciphertext. - * Both are left untouched on disk and surfaced non-fatally so the caller can - * tell the user to review them, rather than silently shipping broken SQL or - * destroying data. Statements sitting inside a SQL comment — or inside a - * single-quoted string literal, where they are data rather than SQL — are inert - * and are neither rewritten nor reported. + * `SET DATA TYPE … USING …;`, or a future drizzle-kit form); ones targeting a + * column that is ALREADY encrypted, where the rewrite would drop ciphertext; + * and ones targeting a column the corpus never DECLARES anywhere, where the + * rewrite would be guessing at a type it cannot see (likely the most common + * skip on a real corpus, since the sweep only ever reads part of the + * migration history). All three are left untouched on disk and surfaced + * non-fatally so the caller can tell the user to review them, rather than + * silently shipping broken SQL or destroying data. Statements sitting inside a + * SQL comment — or inside a single-quoted string literal, where they are data + * rather than SQL — are inert and are neither rewritten nor reported. */ export async function rewriteEncryptedAlterColumns( outDir: string, @@ -440,7 +565,8 @@ export async function rewriteEncryptedAlterColumns( // Built from the WHOLE corpus, including `options.skip`: a column's current // type comes from the migrations that ran before this one, not just the files // we are allowed to edit. - const encryptedColumns = indexEncryptedColumns([...contents.values()]) + const { encrypted: encryptedColumns, declared: declaredColumns } = + indexColumnDeclarations([...contents.values()]) for (const [filePath, original] of contents) { if (options.skip && filePath === options.skip) continue @@ -471,6 +597,17 @@ export async function rewriteEncryptedAlterColumns( return match } + // Fail closed. Absence from `declaredColumns` does not mean the column + // is plaintext — it means the corpus never said. The declaration can + // sit in a directory this sweep does not read (the wizard ships with + // three candidates and indexes each separately), so rewriting on the + // assumption is how a live `DROP COLUMN` reaches a populated — possibly + // already-encrypted — column. Flag it and let the user look. + if (!declaredColumns.has(columnKey(table, column, schema))) { + skip(filePath, match.trim(), 'source-unknown') + return match + } + const domain = DOMAIN_RE.exec(mangledType)?.[0]?.toLowerCase() // Unreachable — the outer regex only matches when a domain is present — // but leave the statement alone rather than emit a broken rewrite. From 94bdb45ca4d234eb1c4a5d482d64bcda006e29c5 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 22:00:09 +1000 Subject: [PATCH 05/11] fix(cli): declare the column the fail-closed sweep now requires in migration tests The Drizzle migration-sweep test fixtures wrote a bare ALTER COLUMN ... SET DATA TYPE with nothing declaring the target column anywhere in the corpus. Since rewriteEncryptedAlterColumns is now fail-closed, such a statement is correctly left as a source-unknown skip, which broke two assertions that expected the rewrite to happen. These tests exist to check the sweep's behaviour (that it walks sibling migrations and skips the just-generated install migration), not the fail-closed rule itself, so fix the fixtures rather than the assertions: each now writes a sibling CREATE TABLE declaring the column plaintext, matching the declarePlaintext pattern already used in rewrite-migrations.test.ts. --- .../commands/eql/__tests__/migration.test.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/eql/__tests__/migration.test.ts b/packages/cli/src/commands/eql/__tests__/migration.test.ts index ba3f8d7d1..9f0fa1f90 100644 --- a/packages/cli/src/commands/eql/__tests__/migration.test.ts +++ b/packages/cli/src/commands/eql/__tests__/migration.test.ts @@ -217,6 +217,14 @@ describe('eqlMigrationCommand — Drizzle', () => { it('rewrites a sibling migration with a broken v3 ALTER COLUMN', async () => { const out = join(tmp, 'drizzle') mkdirSync(out, { recursive: true }) + // The sweep is fail-closed: it rewrites a column only when the corpus + // positively declares it (and it isn't already encrypted). A real drizzle + // corpus carries this declaration in an earlier migration — supply it so + // the fixture matches what the sweep actually requires. + writeFileSync( + join(out, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) const sibling = join(out, '0001_encrypt-email.sql') writeFileSync( sibling, @@ -239,10 +247,17 @@ describe('eqlMigrationCommand — Drizzle', () => { it('does not rewrite the EQL install migration it just generated', async () => { const out = join(tmp, 'drizzle') mkdirSync(out, { recursive: true }) - const generated = join(out, '0000_install-eql.sql') + // Fail-closed requires the corpus to positively declare the column before + // the sweep will touch it — supply the declaration a real drizzle corpus + // would carry, same as the sibling-rewrite test above. + writeFileSync( + join(out, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + const generated = join(out, '0001_install-eql.sql') // A sibling carrying the SAME statement — the differential that proves the // sweep ran at all, rather than no-opping over the whole directory. - const sibling = join(out, '0001_encrypt-email.sql') + const sibling = join(out, '0002_encrypt-email.sql') const unsafeAlter = 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "undefined"."eql_v3_text_search";\n' writeFileSync(sibling, unsafeAlter) From 2632347d542e8a11db2365b8efbb30dda38e3823 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 22:11:00 +1000 Subject: [PATCH 06/11] fix(wizard): declare the column the fail-closed sweep now requires in post-agent test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'defaults to No, and says why, when a file was rewritten' fixture wrote a bare ALTER COLUMN ... SET DATA TYPE with nothing declaring the target column anywhere in the swept corpus. Since rewriteEncryptedAlterColumns is fail-closed, that statement was correctly left as a source-unknown skip — the test passed anyway only because post-agent.ts's `unsafe` check is `rewritten > 0 || skipped > 0`, so it silently became a duplicate of the skipped-path test below it, and the `rewritten > 0` half of that condition had zero coverage. Add a sibling migration declaring the column plaintext (matching the declarePlaintext pattern already used in rewrite-migrations.test.ts and the analogous cli/eql/migration.test.ts fix), so the sweep genuinely rewrites the column and the test exercises the branch its name claims. --- packages/wizard/src/__tests__/post-agent.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/wizard/src/__tests__/post-agent.test.ts b/packages/wizard/src/__tests__/post-agent.test.ts index c13f83cef..afbba81c5 100644 --- a/packages/wizard/src/__tests__/post-agent.test.ts +++ b/packages/wizard/src/__tests__/post-agent.test.ts @@ -159,6 +159,15 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { it('defaults to No, and says why, when a file was rewritten', async () => { fs.mkdirSync(path.join(cwd, 'drizzle')) + // The sweep is fail-closed: it rewrites a column only when the corpus + // positively declares it (and it isn't already encrypted). A real drizzle + // corpus carries this declaration in an earlier migration — supply it so + // the fixture matches what the sweep actually requires, and the ALTER + // below is genuinely rewritten rather than skipped as source-unknown. + 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', From 599c82692b1e6f71c419ec4f83fb8c46a8dbdebb Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 22:17:06 +1000 Subject: [PATCH 07/11] docs(skills,changesets): the ALTER sweep repairs only what it can place Both skills said every recognised statement is rewritten. It now requires the column's declaration to be present in the swept directory and flags what it cannot place. --- .changeset/rewriter-never-drops-ciphertext.md | 12 ++++++++++++ skills/stash-cli/SKILL.md | 2 ++ skills/stash-drizzle/SKILL.md | 2 +- 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/.changeset/rewriter-never-drops-ciphertext.md b/.changeset/rewriter-never-drops-ciphertext.md index b3577f8b4..116c48549 100644 --- a/.changeset/rewriter-never-drops-ciphertext.md +++ b/.changeset/rewriter-never-drops-ciphertext.md @@ -24,6 +24,18 @@ The sweep also refuses to rewrite a column the migration corpus already gives an encrypted type, so changing a column's encrypted domain no longer drops a column full of ciphertext. Skipped statements report why they were left alone. +The sweep is now fail-closed about the columns it does not recognise at all. +Previously a column missing from the corpus index was assumed to be plaintext +and rewritten; absence is not evidence, and the declaration can simply live in a +migration directory the sweep never reads — the wizard ships scanning three of +them and indexes each separately. Such a statement is now reported for review +rather than rewritten, so the ADD+DROP+RENAME can no longer drop a column that +already holds ciphertext. If your migration history is squashed, or the column's +`CREATE TABLE` lives outside the directory being swept, you will see the +statement flagged instead of repaired: check the column's current type and +either apply the rewrite by hand on an empty table, or use the staged +`stash encrypt` lifecycle. + An unreadable migration directory (`EACCES`) is reported rather than silently treated as empty, and the wizard's `Run the migration now?` prompt defaults to No whenever the sweep rewrote anything, flagged anything, or could not check a diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index b35c9be4f..39ea85498 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -392,6 +392,8 @@ 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 an `ADD COLUMN` + `DROP` + `RENAME` sequence and the rewritten files are listed. This is equivalent to DROP+ADD — it fixes the type but does **not** preserve data — so it is safe **only on an empty table**. On a populated table the new column starts NULL and the old one is dropped in the same migration, destroying the plaintext; do not run it there. Use the staged `stash encrypt` path (add → backfill → cutover → drop) instead. +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. 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, because a column of unknown type may already hold ciphertext that the rewrite's `DROP COLUMN` would destroy. + #### `eql upgrade` The install SQL is safe to re-run — columns and data survive — but it is not fully idempotent: it begins with `DROP SCHEMA IF EXISTS eql_v3 CASCADE`, which cascade-drops any **functional indexes** built on the `eql_v3` extractors (see `stash-indexing`). After an upgrade, recreate them: migration runners skip migrations already recorded as applied, so add a *new* migration that re-issues the `CREATE INDEX` statements (or run the DDL directly), then `ANALYZE` the affected tables. `upgrade` checks the current version, re-runs the install SQL, and reports the new one. If EQL isn't installed it points you at `eql install`. Same `--supabase`, `--exclude-operator-family`, `--eql-version`, `--latest`, `--dry-run`, `--database-url` flags. diff --git a/skills/stash-drizzle/SKILL.md b/skills/stash-drizzle/SKILL.md index f52025bb4..50ae36fd2 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**. 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 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. ### Column Storage From 5aebbd5791514ce025585fc20f5e57c96221b446 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 22:40:17 +1000 Subject: [PATCH 08/11] docs(changesets,skills): scope the drop-ciphertext guarantee to what the migration corpus can see The changeset and skills/stash-cli/SKILL.md both claimed the fail-closed sweep means the rewrite "can no longer drop a column that already holds ciphertext." That overclaims: the sweep requires a corpus *declaration*, not the database's *current* state, and the migration corpus is only a model of the database that can go stale. `stash encrypt cutover` renames columns directly in the database without writing drizzle SQL, so the corpus can still describe a cut-over column as plaintext; the same is true of any hand-authored psql or Supabase-dashboard change. Reworded both to say the guarantee holds for what the corpus shows, not for a database that has drifted from its migration history. --- .changeset/rewriter-never-drops-ciphertext.md | 18 +++++++++++++----- skills/stash-cli/SKILL.md | 2 +- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/.changeset/rewriter-never-drops-ciphertext.md b/.changeset/rewriter-never-drops-ciphertext.md index 116c48549..66ad0f209 100644 --- a/.changeset/rewriter-never-drops-ciphertext.md +++ b/.changeset/rewriter-never-drops-ciphertext.md @@ -29,11 +29,19 @@ Previously a column missing from the corpus index was assumed to be plaintext and rewritten; absence is not evidence, and the declaration can simply live in a migration directory the sweep never reads — the wizard ships scanning three of them and indexes each separately. Such a statement is now reported for review -rather than rewritten, so the ADD+DROP+RENAME can no longer drop a column that -already holds ciphertext. If your migration history is squashed, or the column's -`CREATE TABLE` lives outside the directory being swept, you will see the -statement flagged instead of repaired: check the column's current type and -either apply the rewrite by hand on an empty table, or use the staged +rather than rewritten, so the ADD+DROP+RENAME no longer drops a column that the +migration corpus itself shows already holds ciphertext. That is a guarantee +about what the corpus says, not about the database: the sweep reasons entirely +from migration files, and a database that has drifted from its migration +history is outside what it can see. `stash encrypt cutover` is the sharpest +example — it renames columns directly in the database and never writes drizzle +SQL, so the corpus can still describe a column as plaintext after cutover has +made it ciphertext; the same is true of any change made by hand via psql or the +Supabase dashboard. If your migration history is squashed, the column's +`CREATE TABLE` lives outside the directory being swept, or the database has +simply drifted from what the migrations describe, you will see the statement +flagged instead of repaired: check the column's current type in the database +and either apply the rewrite by hand on an empty table, or use the staged `stash encrypt` lifecycle. An unreadable migration directory (`EACCES`) is reported rather than silently diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 39ea85498..aa1c03c27 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -392,7 +392,7 @@ 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 an `ADD COLUMN` + `DROP` + `RENAME` sequence and the rewritten files are listed. This is equivalent to DROP+ADD — it fixes the type but does **not** preserve data — so it is safe **only on an empty table**. On a populated table the new column starts NULL and the old one is dropped in the same migration, destroying the plaintext; do not run it there. Use the staged `stash encrypt` path (add → backfill → cutover → drop) instead. -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. 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, because a column of unknown type may already hold ciphertext that the rewrite's `DROP COLUMN` would destroy. +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 (for example, renamed directly by `stash encrypt cutover`, or altered by hand via psql or the Supabase dashboard) 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, because a column of unknown type may already hold ciphertext that the rewrite's `DROP COLUMN` would destroy. Either way, check the column's actual type in the database before applying a flagged or corpus-cleared rewrite by hand. #### `eql upgrade` From 83bba450509362f7d94f462e2b1ee6ca1d984694 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 22:40:31 +1000 Subject: [PATCH 09/11] fix(cli,wizard): collapse duplicate skip reports for block-comment-prefixed near-misses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A statement the strict matcher matched but skipped (already-encrypted or source-unknown) is left on disk unchanged, so it still contains `SET DATA TYPE` and the broad near-miss scan finds it again. When a `/* ... */` block comment sat glued to the front of that statement, STATEMENT_PREAMBLE_RE (which only stripped blank lines and `--` line comments) left the near-miss pass reporting a different statement string than the strict pass's `match.trim()`, so the dedup key never matched: the same statement was reported twice, once with the correct reason and once as `unrecognised-form` — contradictory advice (look for a hand-authored `USING` clause) for a statement that has none, with the block comment quoted back to the user besides. Extend the preamble regex to also strip a leading block comment so both passes agree on the statement text and the second report collapses into the first. Also tightens the `leaves an ALTER inside a NESTED block comment untouched` regression test to assert `skipped` is empty, not just `rewritten` — this branch's fail-closed change made the fixture's column undeclared, so the statement is skipped as source-unknown regardless of whether nested block comments are handled, and the old assertion passed either way. Also names, in the DECLARED_COLUMN_RE docstring, the dependency the "declared but not encrypted is plaintext by residue" claim has on MANGLED_TYPE_FORMS covering every encrypted shape: a domain installed into a non-`public` schema and an array of the domain both escape ENCRYPTED_TYPE_REF and get rewritten as if plaintext. Neither is a supported EQL layout or a drizzle-kit output, and both behaved this way before this branch, so this is documentation, not a regression. Verified the nested-block-comment guard is not vacuous: replacing the depth-tracking loop in isInsideCommentOrString with a plain first-`*/` scan makes the strengthened test fail (rewritten stays empty, but skipped now surfaces a source-unknown entry), confirming it exercises actual nesting behaviour rather than only checking `rewritten`. --- .../src/__tests__/rewrite-migrations.test.ts | 26 +++++++++++++- .../cli/src/commands/db/rewrite-migrations.ts | 34 ++++++++++++++++--- .../src/__tests__/rewrite-migrations.test.ts | 26 +++++++++++++- packages/wizard/src/lib/rewrite-migrations.ts | 34 ++++++++++++++++--- 4 files changed, 108 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index c830b6001..80f235c1e 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -484,6 +484,29 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped[0].statement).toBe(statement) }) + // A statement the STRICT matcher already matched but skipped (here: + // source-unknown) is left on disk unchanged, so it still contains + // `SET DATA TYPE` and the broad near-miss scan finds it again. Before the + // preamble regex stripped a leading block comment, that second pass reported + // a DIFFERENT statement string (comment glued to the front) than the strict + // pass's `match.trim()`, so the dedup key never matched and the same + // statement came back twice: once correctly as `source-unknown`, once as + // `unrecognised-form` — contradictory advice (look for a hand-authored + // `USING` clause) for a statement that has none. + it('reports a block-comment-prefixed statement once, not twice', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0040_block-preamble.sql') + fs.writeFileSync(filePath, `/* note */\n${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + it('reports a near-miss without a preceding statement-breakpoint marker', async () => { const statement = 'ALTER TABLE "users" ALTER COLUMN "meta" SET DATA TYPE eql_v3_json USING (meta)::jsonb;' @@ -592,9 +615,10 @@ describe('rewriteEncryptedAlterColumns', () => { const filePath = path.join(tmpDir, '0030_nested.sql') fs.writeFileSync(filePath, original) - const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) expect(fs.readFileSync(filePath, 'utf-8')).toBe(original) }) diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index e82e5cedf..8042db3a8 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -96,8 +96,9 @@ const NEAR_MISS_RE = /[^;]*?\bSET\s+DATA\s+TYPE\b[^;]*?\beql_v[23][a-z0-9_]*[^;]*?;/gi /** - * Blank lines and `--` line comments (including drizzle-kit's - * `--> statement-breakpoint`) at the head of a {@link NEAR_MISS_RE} match. + * Blank lines, a leading `/* … *\/` block comment, and `--` line comments + * (including drizzle-kit's `--> statement-breakpoint`) at the head of a + * {@link NEAR_MISS_RE} match. * * That regex opens with a lazy `[^;]*?`, whose only left boundary is the * previous `;` — or the start of the file when there is no preceding statement. @@ -106,10 +107,21 @@ const NEAR_MISS_RE = * with that whole block glued to its front. Strip the preamble so the statement * we quote back reads as the offending statement alone. * - * Only line comments are handled — `/* … *\/` block comments are not something - * drizzle-kit emits, and a stray one costs cosmetics, not correctness. + * The leading block comment is NOT cosmetic: a statement the strict + * {@link ALTER_COLUMN_TO_ENCRYPTED_RE} matched but skipped (`already-encrypted` + * or `source-unknown`) is left on disk unchanged, so it still contains + * `SET DATA TYPE` and the broad scan below finds it again. Without stripping + * the block comment here, that second pass reports a DIFFERENT statement string + * (comment glued to the front) than the strict pass's `match.trim()`, so + * {@link rewriteEncryptedAlterColumns}'s dedup key never matches and the same + * statement is reported twice — once with the correct reason, once as + * `unrecognised-form`, whose guidance (look for a hand-authored `USING`) is + * wrong for a statement the strict matcher already matched. Stripping the + * comment here makes both passes agree on the statement text so the second + * report collapses into the first. */ -const STATEMENT_PREAMBLE_RE = /^(?:[^\S\n]*(?:--[^\n]*)?\n)+/ +const STATEMENT_PREAMBLE_RE = + /^(?:\/\*[\s\S]*?\*\/\s*)?(?:[^\S\n]*(?:--[^\n]*)?\n)*/ /** Drop the leading blank/comment lines a `[^;]*?`-anchored match dragged in. */ function trimStatementPreamble(statement: string): string { @@ -296,6 +308,18 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( * the fail-closed rule needs no type classification and no SQL parsing, only * the known encrypted list that {@link ENCRYPTED_TYPE_REF} already provides. * + * **The residue claim depends on {@link MANGLED_TYPE_FORMS} covering every + * encrypted shape.** "By residue, plaintext" is only as good as the encrypted + * side's coverage: a declaration {@link ENCRYPTED_TYPE_REF} fails to recognise + * falls to the plaintext residue and gets rewritten. Two forms do this: a + * domain installed into a non-`public` schema (`"email" "app"."eql_v3_text_search"`, + * since the mangled forms only special-case the literal `public` schema), and + * an array of the domain (`ADD COLUMN "email" public.eql_v3_text_search[]`, + * since {@link ENCRYPTED_TYPE_REF}'s trailing delimiter lookahead does not + * include `[`). Neither is a layout EQL installs into or a shape drizzle-kit + * emits, and both behaved identically before this branch — so this is a + * documentation gap, not a regression this branch introduced. + * * **Residue, accepted.** The lookahead is a fixed keyword list, not a parser: * a predicate keyword it does not enumerate (`SIMILAR`, `ISNULL`, `NOTNULL`, * `OVERLAPS`, `ON`, …) still lets a bare mention through, e.g. diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index 9edc41bb3..5c737da73 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -440,6 +440,29 @@ describe('rewriteEncryptedAlterColumns', () => { expect(skipped[0].statement).toBe(statement) }) + // A statement the STRICT matcher already matched but skipped (here: + // source-unknown) is left on disk unchanged, so it still contains + // `SET DATA TYPE` and the broad near-miss scan finds it again. Before the + // preamble regex stripped a leading block comment, that second pass reported + // a DIFFERENT statement string (comment glued to the front) than the strict + // pass's `match.trim()`, so the dedup key never matched and the same + // statement came back twice: once correctly as `source-unknown`, once as + // `unrecognised-form` — contradictory advice (look for a hand-authored + // `USING` clause) for a statement that has none. + it('reports a block-comment-prefixed statement once, not twice', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0040_block-preamble.sql') + fs.writeFileSync(filePath, `/* note */\n${alterSql}\n`) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + it('reports a near-miss without a preceding statement-breakpoint marker', async () => { const statement = 'ALTER TABLE "users" ALTER COLUMN "meta" SET DATA TYPE eql_v3_json USING (meta)::jsonb;' @@ -522,9 +545,10 @@ describe('rewriteEncryptedAlterColumns', () => { const filePath = path.join(tmpDir, '0030_nested.sql') fs.writeFileSync(filePath, original) - const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir) + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) expect(rewritten).toEqual([]) + expect(skipped).toEqual([]) expect(fs.readFileSync(filePath, 'utf-8')).toBe(original) }) diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 4fb879f89..abb6afab0 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -104,8 +104,9 @@ const NEAR_MISS_RE = /[^;]*?\bSET\s+DATA\s+TYPE\b[^;]*?\beql_v[23][a-z0-9_]*[^;]*?;/gi /** - * Blank lines and `--` line comments (including drizzle-kit's - * `--> statement-breakpoint`) at the head of a {@link NEAR_MISS_RE} match. + * Blank lines, a leading `/* … *\/` block comment, and `--` line comments + * (including drizzle-kit's `--> statement-breakpoint`) at the head of a + * {@link NEAR_MISS_RE} match. * * That regex opens with a lazy `[^;]*?`, whose only left boundary is the * previous `;` — or the start of the file when there is no preceding statement. @@ -114,10 +115,21 @@ const NEAR_MISS_RE = * with that whole block glued to its front. Strip the preamble so the statement * we quote back reads as the offending statement alone. * - * Only line comments are handled — `/* … *\/` block comments are not something - * drizzle-kit emits, and a stray one costs cosmetics, not correctness. + * The leading block comment is NOT cosmetic: a statement the strict + * {@link ALTER_COLUMN_TO_ENCRYPTED_RE} matched but skipped (`already-encrypted` + * or `source-unknown`) is left on disk unchanged, so it still contains + * `SET DATA TYPE` and the broad scan below finds it again. Without stripping + * the block comment here, that second pass reports a DIFFERENT statement string + * (comment glued to the front) than the strict pass's `match.trim()`, so + * {@link rewriteEncryptedAlterColumns}'s dedup key never matches and the same + * statement is reported twice — once with the correct reason, once as + * `unrecognised-form`, whose guidance (look for a hand-authored `USING`) is + * wrong for a statement the strict matcher already matched. Stripping the + * comment here makes both passes agree on the statement text so the second + * report collapses into the first. */ -const STATEMENT_PREAMBLE_RE = /^(?:[^\S\n]*(?:--[^\n]*)?\n)+/ +const STATEMENT_PREAMBLE_RE = + /^(?:\/\*[\s\S]*?\*\/\s*)?(?:[^\S\n]*(?:--[^\n]*)?\n)*/ /** Drop the leading blank/comment lines a `[^;]*?`-anchored match dragged in. */ function trimStatementPreamble(statement: string): string { @@ -304,6 +316,18 @@ const CREATE_TABLE_ENCRYPTED_COLUMN_RE = new RegExp( * the fail-closed rule needs no type classification and no SQL parsing, only * the known encrypted list that {@link ENCRYPTED_TYPE_REF} already provides. * + * **The residue claim depends on {@link MANGLED_TYPE_FORMS} covering every + * encrypted shape.** "By residue, plaintext" is only as good as the encrypted + * side's coverage: a declaration {@link ENCRYPTED_TYPE_REF} fails to recognise + * falls to the plaintext residue and gets rewritten. Two forms do this: a + * domain installed into a non-`public` schema (`"email" "app"."eql_v3_text_search"`, + * since the mangled forms only special-case the literal `public` schema), and + * an array of the domain (`ADD COLUMN "email" public.eql_v3_text_search[]`, + * since {@link ENCRYPTED_TYPE_REF}'s trailing delimiter lookahead does not + * include `[`). Neither is a layout EQL installs into or a shape drizzle-kit + * emits, and both behaved identically before this branch — so this is a + * documentation gap, not a regression this branch introduced. + * * **Residue, accepted.** The lookahead is a fixed keyword list, not a parser: * a predicate keyword it does not enumerate (`SIMILAR`, `ISNULL`, `NOTNULL`, * `OVERLAPS`, `ON`, …) still lets a bare mention through, e.g. From 56e18afdc15852bd8eaa7687ea7b454b64d60ee7 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 22:40:43 +1000 Subject: [PATCH 10/11] fix(wizard): stop claiming data destruction when the sweep only flagged, not rewrote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `unsafe = sweep.rewritten > 0 || sweep.skipped > 0` warned that the migration "DESTROYS data on any table that already holds rows" whenever anything was rewritten OR merely flagged. For a sweep that rewrote nothing and only flagged a source-unknown statement, that's false — nothing on disk changed, and the raw ALTER simply fails its cast at migrate time instead. This branch makes the pure-skip case the common one, so the overclaim needed its own message arm, matching the care already taken for the `unverified` (failed-sweep) case. Split `unsafe` into `destructive` (rewritten > 0) and `flaggedOnly` (nothing rewritten, but something was flagged), and give `flaggedOnly` a message that tells the truth: statements were flagged for review, nothing was rewritten, and the migration will fail at migrate time until they're resolved. The prompt still defaults to No in both cases. Also strengthens `defaults to No, and says why, when a file was rewritten` to assert the on-disk effect (the swept file contains DROP COLUMN, not SET DATA TYPE) rather than only the prompt defaulting to No and mentioning "DESTROYS data" — both of which the skipped-statement branch also produces, so the test could not tell a genuine rewrite from its `source-unknown` sibling. Confirmed by deleting the fixture's 0000_declare.sql (which makes the ALTER a source-unknown skip instead of a rewrite): the test now fails at the message assertion, since the flagged-only message added above no longer contains "DESTROYS data". Restored the fixture afterwards. --- .../wizard/src/__tests__/post-agent.test.ts | 19 ++++++++++++ packages/wizard/src/lib/post-agent.ts | 29 ++++++++++++------- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/packages/wizard/src/__tests__/post-agent.test.ts b/packages/wizard/src/__tests__/post-agent.test.ts index afbba81c5..8e7656d9f 100644 --- a/packages/wizard/src/__tests__/post-agent.test.ts +++ b/packages/wizard/src/__tests__/post-agent.test.ts @@ -178,6 +178,20 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] expect(options?.initialValue).toBe(false) expect(String(options?.message)).toContain('DESTROYS data') + + // Assert the REWRITE actually happened, not just that the prompt defaulted + // to No — both this test and its `source-unknown` sibling below produce + // `initialValue: false` and a message containing "DESTROYS data" is the + // only thing that used to distinguish them, and that came from the same + // skipped-statement branch too. Without the 0000_declare.sql fixture the + // ALTER is skipped as source-unknown rather than rewritten, so pin the + // on-disk effect a genuine rewrite leaves behind. + const swept = fs.readFileSync( + path.join(cwd, 'drizzle', '0001_encrypt.sql'), + 'utf-8', + ) + expect(swept).toContain('DROP COLUMN') + expect(swept).not.toContain('SET DATA TYPE') }) it('defaults to No when a statement was flagged rather than rewritten', async () => { @@ -191,6 +205,11 @@ describe('drizzle migrate prompt after a destructive rewrite', () => { const [options] = vi.mocked(p.confirm).mock.calls.at(-1) ?? [] expect(options?.initialValue).toBe(false) + // Nothing was rewritten here — the statement was left on disk and merely + // flagged — so the prompt must not claim data destruction the way the + // genuinely-rewritten case above does. + expect(String(options?.message)).not.toContain('DESTROYS data') + expect(String(options?.message)).toContain('flagged for review') }) // A directory whose sweep threw contributes 0 to both totals, so a failed diff --git a/packages/wizard/src/lib/post-agent.ts b/packages/wizard/src/lib/post-agent.ts index e4d4e2154..25c37b7da 100644 --- a/packages/wizard/src/lib/post-agent.ts +++ b/packages/wizard/src/lib/post-agent.ts @@ -82,12 +82,17 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { // scaffolds, and legacy eql_v2_encrypted. CIP-2991 + CIP-2994 + #693. const sweep = await rewriteEncryptedMigrations(cwd) - // A rewritten file is a DROP+ADD in disguise, and a flagged statement is one - // the sweep could not make safe at all. Either way the next keystroke can - // destroy data, so the prompt says so and defaults to NO — an - // `initialValue: true` immediately under a "do NOT run the migration" - // warning invites exactly the mistake the warning is about. - const unsafe = sweep.rewritten > 0 || sweep.skipped > 0 + // A rewritten file is a DROP+ADD in disguise — the next migrate destroys + // data on any table that already holds rows. A flagged statement never got + // that treatment: it is left on disk untouched, so nothing is destroyed by + // migrating, but a raw ALTER to an encrypted domain has no cast in + // Postgres and fails at migrate time until a human resolves it. Both + // default the prompt to NO — an `initialValue: true` immediately under + // either warning invites exactly the mistake the warning is about — but + // they need different words: claiming "DESTROYS data" for a migration + // that destroyed nothing is its own kind of wrong guidance. + const destructive = sweep.rewritten > 0 + const flaggedOnly = !destructive && sweep.skipped > 0 // A directory whose sweep threw contributes 0 to both totals, so on its own // it is indistinguishable from a clean sweep — except that it means the @@ -110,12 +115,14 @@ export async function runPostAgentSteps(opts: PostAgentOptions): Promise { } const shouldMigrate = await p.confirm({ - message: unsafe + message: destructive ? `Run the migration now? (${runner} drizzle-kit migrate) — see the warnings above: this migration DESTROYS data on any table that already holds rows` - : unverified - ? `Run the migration now? (${runner} drizzle-kit migrate) — the sweep could not check ${unverifiedCount} (${unverifiedList}); review those migrations before migrating, or you may apply broken/unsafe SQL` - : `Run the migration now? (${runner} drizzle-kit migrate)`, - initialValue: !unsafe && !unverified, + : flaggedOnly + ? `Run the migration now? (${runner} drizzle-kit migrate) — statement(s) were flagged for review above rather than rewritten; nothing was destroyed, but the raw ALTER will fail at migrate time until they're resolved` + : unverified + ? `Run the migration now? (${runner} drizzle-kit migrate) — the sweep could not check ${unverifiedCount} (${unverifiedList}); review those migrations before migrating, or you may apply broken/unsafe SQL` + : `Run the migration now? (${runner} drizzle-kit migrate)`, + initialValue: !destructive && !flaggedOnly && !unverified, }) if (!p.isCancel(shouldMigrate) && shouldMigrate) { From 692b9415407be7885bf80d2f09849cf50a4a8d4b Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 24 Jul 2026 23:37:25 +1000 Subject: [PATCH 11/11] fix(cli,wizard): fold the block-comment strip into the preamble loop STATEMENT_PREAMBLE_RE anchored its block-comment group to `^` ahead of the line-comment loop, so it only stripped a leading block comment when that comment sat at the very start of the FILE. In any file with a preceding statement, NEAR_MISS_RE's match starts at that statement's `;` -- i.e. with a newline, not the comment -- so the anchored group could never match past it. The same statement was then reported twice: once correctly (already- encrypted or source-unknown), and once as unrecognised-form with the block comment glued to its front and guidance pointing at a hand-authored USING clause that isn't there. Fold the block comment into the repeating loop as its own alternative instead of a group ahead of it, so it can match after any number of leading newlines/line comments, in any interleaving. Hardens the existing regression test to use a preceding statement (the one shape the broken regex happened to handle, which is why it passed before), and adds coverage for an indented block comment on the ALTER's own line and for the already-encrypted reason. Each asserts the full skipped array, so both count and reason are pinned. Also documents the one known residue: a nested closed block comment ahead of a live ALTER still double-reports, because the alternative's lazy `*?` stops at the first `*/`. Accepted, not fixed. Verified by reverting the regex to the anchored form: the three new/ hardened tests fail with the described double-report in both packages, confirming they are load-bearing. --- .../src/__tests__/rewrite-migrations.test.ts | 65 ++++++++++++++++++- .../cli/src/commands/db/rewrite-migrations.ts | 36 +++++++--- .../src/__tests__/rewrite-migrations.test.ts | 65 ++++++++++++++++++- packages/wizard/src/lib/rewrite-migrations.ts | 36 +++++++--- 4 files changed, 184 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/__tests__/rewrite-migrations.test.ts b/packages/cli/src/__tests__/rewrite-migrations.test.ts index 80f235c1e..275ee5852 100644 --- a/packages/cli/src/__tests__/rewrite-migrations.test.ts +++ b/packages/cli/src/__tests__/rewrite-migrations.test.ts @@ -493,11 +493,26 @@ describe('rewriteEncryptedAlterColumns', () => { // statement came back twice: once correctly as `source-unknown`, once as // `unrecognised-form` — contradictory advice (look for a hand-authored // `USING` clause) for a statement that has none. + // + // The block comment must NOT sit at the very start of the file — that is + // the one shape a previous, narrower version of the preamble regex happened + // to handle. A realistic migration file has a preceding statement, so + // NEAR_MISS_RE's match starts at THAT statement's `;`, dragging the newline + // before the comment in too; a regex that only strips a comment anchored to + // the very start of the match fails here. it('reports a block-comment-prefixed statement once, not twice', async () => { const alterSql = 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' const filePath = path.join(tmpDir, '0040_block-preamble.sql') - fs.writeFileSync(filePath, `/* note */\n${alterSql}\n`) + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY);', + '/* note */', + alterSql, + '', + ].join('\n'), + ) const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) @@ -507,6 +522,54 @@ describe('rewriteEncryptedAlterColumns', () => { ]) }) + // Same bug, but the comment sits on the ALTER's own line rather than its + // own — the preceding statement's `;` still starts the near-miss match + // before the (indented) comment, not at it. + it('reports an indented block-comment-prefixed statement once, not twice', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0041_block-preamble-indented.sql') + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY);', + ` /* note */ ${alterSql}`, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // Same bug again, this time on the OTHER correct reason a near-miss can + // carry: the column is already encrypted, not merely undeclared. + it('reports a block-comment-prefixed statement once, not twice, for an already-encrypted column', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0042_block-preamble-encrypted.sql') + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" "public"."eql_v3_text_eq");', + '/* note */', + alterSql, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + it('reports a near-miss without a preceding statement-breakpoint marker', async () => { const statement = 'ALTER TABLE "users" ALTER COLUMN "meta" SET DATA TYPE eql_v3_json USING (meta)::jsonb;' diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index 8042db3a8..cf3d0e0be 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -96,18 +96,30 @@ const NEAR_MISS_RE = /[^;]*?\bSET\s+DATA\s+TYPE\b[^;]*?\beql_v[23][a-z0-9_]*[^;]*?;/gi /** - * Blank lines, a leading `/* … *\/` block comment, and `--` line comments - * (including drizzle-kit's `--> statement-breakpoint`) at the head of a + * Strips any run of blank lines, `--` line comments (including drizzle-kit's + * `--> statement-breakpoint`), and `/* … *\/` block comments — in any order, + * repeated as many times as they occur — from the head of a * {@link NEAR_MISS_RE} match. * * That regex opens with a lazy `[^;]*?`, whose only left boundary is the * previous `;` — or the start of the file when there is no preceding statement. * So the raw match drags in every comment and blank line since then, and a - * near-miss in a file that opens with a comment block gets reported to the user - * with that whole block glued to its front. Strip the preamble so the statement - * we quote back reads as the offending statement alone. - * - * The leading block comment is NOT cosmetic: a statement the strict + * near-miss preceded by a comment block gets reported to the user with that + * whole block glued to its front. Strip the preamble so the statement we quote + * back reads as the offending statement alone. + * + * The block comment is matched as its OWN loop alternative rather than a + * single group anchored ahead of the line-comment loop, because the latter + * only works when the block comment sits at the very start of the file: in + * the far more common case — a preceding statement earlier in the same file — + * the match starts at THAT statement's `;`, so it opens with the newline + * after it, not with the comment. An anchored `^(?:/\*…\*\/\s*)?` can't match + * past that newline to reach the comment, so it silently matches nothing and + * leaves the comment attached to the reported statement. Folding the block + * comment into the repeating loop lets it match after any number of leading + * newlines/line-comments, in any interleaving. + * + * The block comment strip is NOT cosmetic: a statement the strict * {@link ALTER_COLUMN_TO_ENCRYPTED_RE} matched but skipped (`already-encrypted` * or `source-unknown`) is left on disk unchanged, so it still contains * `SET DATA TYPE` and the broad scan below finds it again. Without stripping @@ -119,9 +131,17 @@ const NEAR_MISS_RE = * wrong for a statement the strict matcher already matched. Stripping the * comment here makes both passes agree on the statement text so the second * report collapses into the first. + * + * Known residue, accepted rather than fixed: a NESTED closed block comment + * ahead of a live ALTER (`/* outer /* inner *\/ still *\/`) still + * double-reports. The block-comment alternative's `*?` is lazy, so it stops + * at the FIRST `*\/` — consuming only `/* outer /* inner *\/` — and leaves + * `` still *\/`` glued to the front of the next iteration, which the + * line-comment alternative doesn't recognise either. That residual text rides + * along into the `unrecognised-form` report. */ const STATEMENT_PREAMBLE_RE = - /^(?:\/\*[\s\S]*?\*\/\s*)?(?:[^\S\n]*(?:--[^\n]*)?\n)*/ + /^(?:\s*\/\*[\s\S]*?\*\/|[^\S\n]*(?:--[^\n]*)?\n)*/ /** Drop the leading blank/comment lines a `[^;]*?`-anchored match dragged in. */ function trimStatementPreamble(statement: string): string { diff --git a/packages/wizard/src/__tests__/rewrite-migrations.test.ts b/packages/wizard/src/__tests__/rewrite-migrations.test.ts index 5c737da73..01804dae6 100644 --- a/packages/wizard/src/__tests__/rewrite-migrations.test.ts +++ b/packages/wizard/src/__tests__/rewrite-migrations.test.ts @@ -449,11 +449,26 @@ describe('rewriteEncryptedAlterColumns', () => { // statement came back twice: once correctly as `source-unknown`, once as // `unrecognised-form` — contradictory advice (look for a hand-authored // `USING` clause) for a statement that has none. + // + // The block comment must NOT sit at the very start of the file — that is + // the one shape a previous, narrower version of the preamble regex happened + // to handle. A realistic migration file has a preceding statement, so + // NEAR_MISS_RE's match starts at THAT statement's `;`, dragging the newline + // before the comment in too; a regex that only strips a comment anchored to + // the very start of the match fails here. it('reports a block-comment-prefixed statement once, not twice', async () => { const alterSql = 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' const filePath = path.join(tmpDir, '0040_block-preamble.sql') - fs.writeFileSync(filePath, `/* note */\n${alterSql}\n`) + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY);', + '/* note */', + alterSql, + '', + ].join('\n'), + ) const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) @@ -463,6 +478,54 @@ describe('rewriteEncryptedAlterColumns', () => { ]) }) + // Same bug, but the comment sits on the ALTER's own line rather than its + // own — the preceding statement's `;` still starts the near-miss match + // before the (indented) comment, not at it. + it('reports an indented block-comment-prefixed statement once, not twice', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0041_block-preamble-indented.sql') + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY);', + ` /* note */ ${alterSql}`, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'source-unknown' }, + ]) + }) + + // Same bug again, this time on the OTHER correct reason a near-miss can + // carry: the column is already encrypted, not merely undeclared. + it('reports a block-comment-prefixed statement once, not twice, for an already-encrypted column', async () => { + const alterSql = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search;' + const filePath = path.join(tmpDir, '0042_block-preamble-encrypted.sql') + fs.writeFileSync( + filePath, + [ + 'CREATE TABLE "users" ("id" integer PRIMARY KEY, "email" "public"."eql_v3_text_eq");', + '/* note */', + alterSql, + '', + ].join('\n'), + ) + + const { rewritten, skipped } = await rewriteEncryptedAlterColumns(tmpDir) + + expect(rewritten).toEqual([]) + expect(skipped).toEqual([ + { file: filePath, statement: alterSql, reason: 'already-encrypted' }, + ]) + }) + it('reports a near-miss without a preceding statement-breakpoint marker', async () => { const statement = 'ALTER TABLE "users" ALTER COLUMN "meta" SET DATA TYPE eql_v3_json USING (meta)::jsonb;' diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index abb6afab0..0255df6b5 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -104,18 +104,30 @@ const NEAR_MISS_RE = /[^;]*?\bSET\s+DATA\s+TYPE\b[^;]*?\beql_v[23][a-z0-9_]*[^;]*?;/gi /** - * Blank lines, a leading `/* … *\/` block comment, and `--` line comments - * (including drizzle-kit's `--> statement-breakpoint`) at the head of a + * Strips any run of blank lines, `--` line comments (including drizzle-kit's + * `--> statement-breakpoint`), and `/* … *\/` block comments — in any order, + * repeated as many times as they occur — from the head of a * {@link NEAR_MISS_RE} match. * * That regex opens with a lazy `[^;]*?`, whose only left boundary is the * previous `;` — or the start of the file when there is no preceding statement. * So the raw match drags in every comment and blank line since then, and a - * near-miss in a file that opens with a comment block gets reported to the user - * with that whole block glued to its front. Strip the preamble so the statement - * we quote back reads as the offending statement alone. - * - * The leading block comment is NOT cosmetic: a statement the strict + * near-miss preceded by a comment block gets reported to the user with that + * whole block glued to its front. Strip the preamble so the statement we quote + * back reads as the offending statement alone. + * + * The block comment is matched as its OWN loop alternative rather than a + * single group anchored ahead of the line-comment loop, because the latter + * only works when the block comment sits at the very start of the file: in + * the far more common case — a preceding statement earlier in the same file — + * the match starts at THAT statement's `;`, so it opens with the newline + * after it, not with the comment. An anchored `^(?:/\*…\*\/\s*)?` can't match + * past that newline to reach the comment, so it silently matches nothing and + * leaves the comment attached to the reported statement. Folding the block + * comment into the repeating loop lets it match after any number of leading + * newlines/line-comments, in any interleaving. + * + * The block comment strip is NOT cosmetic: a statement the strict * {@link ALTER_COLUMN_TO_ENCRYPTED_RE} matched but skipped (`already-encrypted` * or `source-unknown`) is left on disk unchanged, so it still contains * `SET DATA TYPE` and the broad scan below finds it again. Without stripping @@ -127,9 +139,17 @@ const NEAR_MISS_RE = * wrong for a statement the strict matcher already matched. Stripping the * comment here makes both passes agree on the statement text so the second * report collapses into the first. + * + * Known residue, accepted rather than fixed: a NESTED closed block comment + * ahead of a live ALTER (`/* outer /* inner *\/ still *\/`) still + * double-reports. The block-comment alternative's `*?` is lazy, so it stops + * at the FIRST `*\/` — consuming only `/* outer /* inner *\/` — and leaves + * `` still *\/`` glued to the front of the next iteration, which the + * line-comment alternative doesn't recognise either. That residual text rides + * along into the `unrecognised-form` report. */ const STATEMENT_PREAMBLE_RE = - /^(?:\/\*[\s\S]*?\*\/\s*)?(?:[^\S\n]*(?:--[^\n]*)?\n)*/ + /^(?:\s*\/\*[\s\S]*?\*\/|[^\S\n]*(?:--[^\n]*)?\n)*/ /** Drop the leading blank/comment lines a `[^;]*?`-anchored match dragged in. */ function trimStatementPreamble(statement: string): string {