fix(cli): rewrite ALTER COLUMN to EQL v3 domains, not just eql_v2_encrypted#728
fix(cli): rewrite ALTER COLUMN to EQL v3 domains, not just eql_v2_encrypted#728tobyhede wants to merge 6 commits into
Conversation
…rypted drizzle-kit emits an in-place `ALTER TABLE ... ALTER COLUMN ... SET DATA TYPE` when a plaintext column is changed to an encrypted one. Postgres rejects it (no cast from text/numeric to an EQL type), and on drizzle-kit 0.31.0+ the type name is additionally mangled to `"undefined"."<type>"` because a customType has no `typeSchema`. `rewriteEncryptedAlterColumns` repaired this only for the v2 type, so an EQL v3 user got an un-runnable migration with nothing to fix it (#693). - Match the whole `eql_v3_*` domain family alongside `eql_v2_encrypted`, by shape rather than an enumerated list, so a new domain needs no edit here. - Capture the mangled type blob and thread the extracted domain through `renderSafeAlter`, which no longer hardcodes the v2 type. - Cover all six mangled forms, verified empirically against drizzle-kit 0.24.2 / 0.28.1 / 0.30.6 / 0.31.0 / 0.31.1 / 0.31.10 via `drizzle-kit/api`. The `"undefined".` prefix is a 0.31.0-AND-LATER behaviour; #693 and #688 both describe it as an older-drizzle-kit artifact, which is backwards. Two forms this adds (`public.eql_vN_...` and `"undefined"."public.eql_vN_..."`) were unmatched for v2 as well, and the latter is what stack emitted for v3 before #688. - Run the sweep from `stash eql migration --drizzle` too. That is the v3 migration-first path and it never called the rewriter, so the regex fix alone would not have reached a single v3 user. Backfill guidance: v3 stores the ZeroKMS-produced EQL jsonb envelope rather than v2's composite, but it is still not expressible in SQL, so the UPDATE stays a guidance comment on both surfaces. What is worth adding is that this sequence DROPS the plaintext column in the same migration, whereas the staged `stash encrypt` lifecycle keeps both alive across deploys — so the comment now points there for populated tables. The wizard's copy of the rewriter stays v2-only (it only scaffolds v2 columns); its cross-reference comment records the divergence. Closes #693
The test asserted only that the generated migration still contained the EQL bundle. The bundle carries no ALTER COLUMN of its own, so the sweep would never have rewritten it and the test passed with `skip` removed. Append an unsafe ALTER to what the command writes, and put the identical statement in a sibling file: the generated migration must keep it verbatim while the sibling gets rewritten. Verified it fails when `skip` is dropped. Found by CodeRabbit.
🦋 Changeset detectedLatest commit: 8dcbf05 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe migration rewriter now recognizes EQL v2 and v3 encrypted domains, rewrites eligible Drizzle ALTER statements, reports skipped near-misses, and preserves schema qualifiers. Both migration workflows invoke the sweep, with tests and documentation covering its behavior and data-handling guidance. ChangesEQL migration rewrite
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant eqlMigrationCommand
participant rewriteEncryptedAlterColumns
participant SiblingMigrations
participant MigrationLogger
eqlMigrationCommand->>rewriteEncryptedAlterColumns: sweep output directory and skip generated migration
rewriteEncryptedAlterColumns->>SiblingMigrations: rewrite eligible sibling ALTER statements
rewriteEncryptedAlterColumns->>eqlMigrationCommand: return rewritten and skipped results
eqlMigrationCommand->>MigrationLogger: log rewrites and incomplete-sweep warnings
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Two review findings on the rewrite: - The guidance comment told the user to "backfill the new column BEFORE running this migration" — impossible, since the temp column is created BY this migration's ADD COLUMN and the original is dropped in the same run. The sequence is equivalent to DROP+ADD: it fixes the type but does not preserve data, so it is safe ONLY on an empty table. Reworded the comment (and the two skills) to say exactly that and to point a populated table at the staged `stash encrypt` path instead. - Narrow the match tail from `[^;]*;` to `\s*;` so a hand-authored `SET DATA TYPE <type> USING <expr>;` — a runnable statement the user wrote deliberately — is left untouched rather than silently rewritten with its USING clause discarded. drizzle-kit's own output has no trailing clause, so this changes nothing for the real broken forms. Added a regression test (fails under the old `[^;]*;`). Found by CodeRabbit.
The Step 4/5 comment and `p.log.info` on both sweep call sites (`eql migration --drizzle` and `eql install --drizzle`) still said the rewrite "works on empty and populated tables alike" / was a "safe ADD+migrate+DROP" — the same overclaim just corrected in the rewriter and skills. The sequence is equivalent to DROP+ADD: runnable, safe on an empty table, data-destroying on a populated one. Reword both to say so and to point at each file's header comment (which steers populated tables to the staged `stash encrypt` path). Found by CodeRabbit. Two further findings on the same review — the rewrite not preserving column constraints, and the regex not being SQL-context-aware — are limitations of the shared v2 rewriter this change inherits rather than defects in it; deferred to the standalone repair-command follow-up (#710) rather than redesigning shipped v2 behaviour inside a v3 bugfix.
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Review — stash migration rewriter (v3 domain support)
Deep pass over the rewriter. The core change is right: matching the eql_v3_* family by shape, carrying the matched domain into the replacement, and wiring the sweep into eql migration --drizzle all check out, and the 42-domain list in the test matches packages/stack/src/eql/v3/ exactly. 80/80 tests pass locally.
I verified the regex empirically against realistic drizzle-kit output rather than by reading. Results: --> statement-breakpoint, lowercase SQL, a newline before the ;, and mixed v2/v3 in one file all match correctly. Two inputs do not match — details below and inline.
Nine findings; six are inline. The three below sit outside the diff hunks, so GitHub won't anchor them.
1. The exported function's docblock still claims populated-table safety — rewrite-migrations.ts:78-90
This is the same falsehood a71b9a8 removed from the user-facing messages, surviving in the API documentation:
"The fix that works on both empty and non-empty tables is to add a new encrypted column, backfill it, drop the original..."
"Running this migration against a populated table leaves the new column NULL until the app backfills it."
That reads as recoverable — as though the plaintext is still there to backfill from. It isn't: renderSafeAlter emits DROP COLUMN in the same migration, and its own docblock at line 131 now says the sequence DESTROYS the data. The two docblocks in this one file directly contradict each other, and the wrong one is on the exported symbol.
Worth fixing in this PR precisely because it's the class of drift the PR set out to correct. (It also still describes only eql_v2_encrypted.)
2. No diagnostic when an encrypted ALTER is detected but not rewritten — rewrite-migrations.ts:105
The sweep returns only files it changed; callers log Rewrote N file(s) or stay silent. Anything recognisably an ALTER-to-encrypted that falls outside the pattern — a schema-qualified table (see inline on line 70), any trailing clause now that the tail is \s*;, a form a future drizzle-kit adds — is passed over with zero output. The user believes the sweep ran and ships a migration that dies at drizzle-kit migrate.
Given the tail was deliberately tightened this PR, the silent-miss surface grew. A cheap second pass — match SET DATA TYPE near an eql_v[23] token, warn on anything the strict regex didn't rewrite — turns every one of these into an actionable message and costs one extra scan.
3. Four statements share one --> statement-breakpoint chunk — rewrite-migrations.ts:165
drizzle-kit splits on --> statement-breakpoint and sends each chunk as one query. The rewrite replaces one statement with four joined by \n and inserts no breakpoints, so all four arrive as a single multi-statement string. node-postgres tolerates that in simple-query mode; the Neon HTTP driver and postgres.js in prepared mode reject multiple commands per statement — failing exactly the projects this unblocks. Pre-existing, but this PR extends it to every v3 user.
Respond to review feedback on the migration rewriter:
- Match schema-qualified tables (app.users from pgSchema()); thread the
schema through renderSafeAlter so every emitted statement stays qualified.
- Derive DOMAIN_RE from ENCRYPTED_DOMAIN so the domain alternation has one
source of truth (silent-miss if they drift).
- Warn in the emitted guidance that NOT NULL, DEFAULT, UNIQUE, and indexes are
not carried over by the ADD/DROP/RENAME.
- Drop the trailing ; from the commented UPDATE placeholder so naive
semicolon-splitting runners can't cut mid-comment.
- Rewrite the exported docblock: the sequence destroys data and is empty-table
only, matching renderSafeAlter; covers both v2 and the v3 domain family.
- Report near-miss encrypted ALTERs the strict regex didn't rewrite (skipped)
instead of passing them over silently; return { rewritten, skipped }.
- Insert --> statement-breakpoint between emitted statements so Neon HTTP /
postgres.js prepared-mode drivers don't reject multi-command chunks.
- Surface an incomplete sweep in the closing next-steps note, not just an
inline warning sandwiched between success messages.
- Reduce the duplicated plaintext->encrypted warning in stash-drizzle to the
drizzle-specific detail plus a cross-reference; stash-cli owns the full
safety explanation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/commands/db/install.ts (1)
616-630: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSkipped near-misses aren't re-surfaced at the closing note (inconsistent with
migration.ts).When
skipped.length > 0, the warning is logged inline at Line 617, but execution then falls through top.log.success('Migration created')and the "rundrizzle-kit migrate" note. The warning scrolls past between success output, and the user is steered to migrate against sibling SQL that still holds un-runnableALTER COLUMNstatements.
generateDrizzleEqlMigrationinpackages/cli/src/commands/eql/migration.tsalready solved this by trackingsweepIncompleteand re-warning at the closing note. This v2 install path — the one that "has always done this" — should get the same treatment for parity.♻️ Mirror the sweepIncomplete closing-note warning
+ let sweepIncomplete = false try { const { rewritten, skipped } = await rewriteEncryptedAlterColumns(outDir, { skip: generatedMigrationPath, }) if (rewritten.length > 0) { p.log.info( `Rewrote ${rewritten.length} migration file(s) into a runnable ADD+DROP+RENAME for encrypted columns (safe on empty tables; see each file's header before running against populated data):`, ) for (const file of rewritten) p.log.step(` - ${file}`) } if (skipped.length > 0) { + sweepIncomplete = true p.log.warn( `Found ${skipped.length} ALTER-to-encrypted statement(s) the sweep could not rewrite automatically. Review and fix them before running your migrations:`, ) for (const { file, statement } of skipped) { p.log.step(` - ${file}: ${statement}`) } } } catch (error) { + sweepIncomplete = true p.log.warn( `Could not rewrite ALTER COLUMN migrations: ${error instanceof Error ? error.message : String(error)}`, ) } p.log.success(`Migration created: ${generatedMigrationPath}`) + if (sweepIncomplete) { + p.log.warn( + `The ALTER COLUMN sweep did not fully complete — review the sibling migrations in ${outDir} before running drizzle-kit migrate, or you may apply broken/unsafe SQL.`, + ) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/db/install.ts` around lines 616 - 630, Track whether the ALTER-to-encrypted sweep was incomplete when skipped statements are found, using the existing skipped handling in the install migration flow. At the closing output near p.log.success, re-emit the warning and migration guidance when that flag is set, matching generateDrizzleEqlMigration’s sweepIncomplete behavior so users are not directed to run migrations containing unrewriteable statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/cli/src/commands/db/install.ts`:
- Around line 616-630: Track whether the ALTER-to-encrypted sweep was incomplete
when skipped statements are found, using the existing skipped handling in the
install migration flow. At the closing output near p.log.success, re-emit the
warning and migration guidance when that flag is set, matching
generateDrizzleEqlMigration’s sweepIncomplete behavior so users are not directed
to run migrations containing unrewriteable statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b29e3fda-4b0f-441c-b591-22156d9756c7
📒 Files selected for processing (9)
.changeset/wild-donkeys-shave.mdpackages/cli/src/__tests__/rewrite-migrations.test.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/db/rewrite-migrations.tspackages/cli/src/commands/eql/__tests__/migration.test.tspackages/cli/src/commands/eql/migration.tspackages/wizard/src/lib/rewrite-migrations.tsskills/stash-cli/SKILL.mdskills/stash-drizzle/SKILL.md
The v2 install path (generateDrizzleMigration) warned inline on a skipped statement or a thrown sweep, then fell through to the 'Migration created' success line and the 'run drizzle-kit migrate' note with no re-surfacing — so the warning scrolled past and the user was steered to migrate against un-swept, broken sibling SQL. Mirror the sweepIncomplete flag + closing-note warning already used in the v3 stash eql migration --drizzle path (migration.ts), giving the two migration surfaces parity. Addresses the CodeRabbit parity nitpick on #728.
|
@coderabbitai Applied in 8dcbf05.
No test added: |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
I thought the plan was to remove V2 entirely but I'm okay leaving it, so we get this one out of the line, if that is removed as a follow-up. |
2 participants
What
drizzle-kit generateemits an in-placeALTER TABLE … ALTER COLUMN … SET DATA TYPEwhen a plaintext column is changed to an encrypted one. Postgres rejects it — there is no cast fromtext/numericto an EQL domain — and on drizzle-kit 0.31.0+ the type name is additionally mangled to"undefined"."<type>"because acustomTypehas notypeSchema.rewriteEncryptedAlterColumnsrepaired this only for the v2 typeeql_v2_encrypted. An EQL v3 user who changed a column's type got an un-runnable migration with nothing to fix it. Closes #693.Fix
eql_v3_*domain family alongsideeql_v2_encrypted, by shape (eql_v3_[a-z0-9_]+) rather than an enumerated list, so a newly added domain needs no edit here.renderSafeAlter, which previously hardcoded the v2 type in its comment and itsADD COLUMN.stash eql migration --drizzletoo. This is the v3 migration-first path and it never called the rewriter — the regex fix alone would not have reached a single v3 user.Version-dependence (issue and #688 had it backwards)
Reproduced against six drizzle-kit versions via
drizzle-kit/api'sgenerateMigration:dataType()returnseql_v3_text_searcheql_v3_text_search"undefined"."eql_v3_text_search"public.eql_v3_text_searchpublic.eql_v3_text_search"undefined"."public.eql_v3_text_search""public"."eql_v3_text_search""public"."eql_v3_text_search""undefined".""public"."eql_v3_text_search""The
"undefined".prefix is a 0.31.0-and-later behaviour, not an older-drizzle-kit artifact as #693 and #688 both state. That yields six mangled forms, all matched. Two of them (public.eql_vN_…and"undefined"."public.eql_vN_…") were unmatched for v2 as well; the latter is exactly what stack emitted for v3 before #688, so pre-#688 RC users have those files on disk today.Backfill guidance
Kept as a comment on both surfaces. v3 stores the ZeroKMS-produced EQL jsonb envelope rather than v2's composite, but it is still not expressible in SQL, so the UPDATE cannot become real SQL. What is worth adding — and now is — is that this sequence drops the plaintext column in the same migration, whereas the staged
stash encryptlifecycle (add → backfill → cutover → drop) keeps both alive across deploys; the comment now points there for populated tables.Not in scope (filed as #710)
The sweep is bolted onto
eql migration, which runs before the broken migration exists in the intended flow, so it only helps the user who re-runs the command as a recovery step. The complete fix is a standalone repair command (stash eql repair --drizzle, or a check instash encrypt plan) that can also consult the drizzle journal to avoid rewriting an already-appliedjsonb→v3 migration. Filed as #710 rather than expanding this PR's surface.Tests
text,jsonb,eql_v4_*,not_eql_v3_*).eql migration --drizzletests: sweeps a broken sibling; does not rewrite the install migration it just generated (the skip is verified load-bearing — the test fails ifskipis dropped).pnpm --filter stash test→ 640 passed / 51 files.pnpm --filter @cipherstash/wizard test→ 145 passed.pnpm run code:check→ exit 0.Skills / changeset
skills/stash-cliandskills/stash-drizzledocument the new sweep behaviour and the plaintext→encrypted column-change hazard. The wizard's copy of the rewriter stays v2-only (it only scaffolds v2 columns); its cross-reference comment records the divergence. Changeset:stashpatch.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
ALTER COLUMN ... SET DATA TYPEwhen switching from plaintext to CipherStash EQL encrypted domains (EQL v2 and full EQL v3 family, including schema-qualified and version-mangled forms).SET DATA TYPEmigrations by rewriting eligible statements into a safe multi-step ADD/DROP/RENAME flow.Documentation