feat(wizard): port migration rewriter to the EQL v3 domain family#771
feat(wizard): port migration rewriter to the EQL v3 domain family#771tobyhede wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 8466300 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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
81ed000 to
a1bfe1a
Compare
bdabbac to
80e93e1
Compare
a1bfe1a to
36b099b
Compare
27c028f to
3e88398
Compare
2a1dca6 to
30e7b55
Compare
The wizard now scaffolds EQL v3 columns, so `drizzle-kit generate` emits `ALTER COLUMN ... SET DATA TYPE eql_v3_<name>` — which Postgres rejects (no cast from text/numeric to an EQL domain). The post-agent rewriter matched only the single `eql_v2_encrypted` type, so those v3 statements slipped through unrepaired and failed at migrate time. Port the rewriter to the whole `eql_v3_*` concrete-domain family alongside legacy `eql_v2_encrypted`, mirroring the sibling CLI fix (#693): every mangled form drizzle-kit emits (incl. the 0.31.0+ `"undefined".` prefix and schema-qualified pgSchema tables), near-miss flagging for `SET DATA TYPE ... USING ...` it cannot safely repair, statement-breakpoints, and a clearer data-destroying / empty-table-only warning that points populated tables at the staged `stash encrypt` flow. Database introspection (`isEqlEncrypted`) now recognises BOTH `eql_v2_encrypted` and the `eql_v3_*` family as already-encrypted, matching migrate's `classifyEqlDomain` v3 convention — so the agent won't scaffold over existing encrypted data of either generation (v2 ciphertext stays valid and detected). Add a rewrite-migrations test suite (adapted from the CLI's).
3e88398 to
cd75ef4
Compare
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed by Claude Code on behalf of James Sadler.
Verdict: Approve
This is a clean, well-reasoned port of the wizard's migration rewriter from the single eql_v2_encrypted type to the full eql_v2/eql_v3_* domain family, re-converging it with the CLI sibling (packages/cli/src/commands/db/rewrite-migrations.ts) landed in #693. I fetched both files at the PR head and confirmed the regex machinery is now identical to the CLI reference. The change is a detection/repair transform, not a crypto change, and I found no blocking correctness, security, or data-loss defects.
What I checked carefully
- Regex family (
ENCRYPTED_DOMAIN→DOMAIN_RE/MANGLED_TYPE_FORMS/ALTER_COLUMN_TO_ENCRYPTED_RE). Derived-from-one-source design (DOMAIN_REbuilt fromENCRYPTED_DOMAIN) is good — it structurally prevents matcher/extractor drift, and theit.each(V3_DOMAINS)"extracts the bare domain" test enforces it. Longest-first alternation ordering is correct; I traced the plain,public.,"public"."...","undefined"."...", and double-quoted forms and none premature-match a prefix and strand a fragment before\s*;. Capture-group indexing in the replace callback (first/second/column/mangledType) is right because everyMANGLED_TYPE_FORMSbranch uses non-capturing groups. - The
\s*;tightening. Switching the tail from[^;]*;to(...)\s*;is the load-bearing safety fix: a hand-authoredSET DATA TYPE ... USING <expr>;no longer strict-matches (itsUSINGwould otherwise have been silently discarded). Correctly backstopped byNEAR_MISS_RE, which flags-but-does-not-rewrite. Verified by the0013_usingtest. NEAR_MISS_REordering. Running the broad scan on the post-rewrite content is correct: genuinely-rewritten statements no longer containSET DATA TYPE, so they can't self-flag. The "reports no skipped statements when the strict rewrite fully handled the file" test pins this, and the emitted guidance/-- UPDATE ... SET ...comment lines contain noSET DATA TYPE, so they don't trip it either.[^;]bounding keeps matches within a single statement even across newlines.- Data-loss surface. ADD+DROP+RENAME is data-destroying — but that's unchanged from the prior v2 rewriter (deliberate, per design Decision 6). The new header comment and the
post-agent.tswarning are a genuine improvement over the old "backfill ... Empty tables are safe as-is" text, which understated the footgun. The added note that constraints/defaults/indexes are not carried over is a good catch. - Detection (
isEqlEncryptedDomain).=== 'eql_v2_encrypted' || startsWith('eql_v3_')errs conservative (treats-as-encrypted), so the failure mode is "won't scaffold over a column" rather than "clobbers ciphertext" — the safe direction. Trailing-underscore prefix correctly avoids a hypotheticaleql_v30_*. post-agent.tswiring. Destructures the newRewriteResult, surfacesskippednon-fatally, per-dirtry/catch. Fine.
Non-blocking notes (no change required)
- Hardcoded
"public"."<domain>"in the emitted ADD COLUMN. The rewrite always targets the EQL domain inpublic, regardless of where it's actually installed. This matches both the prior v2 code and the CLI sibling, andpublicis the EQL install convention, so it's not a regression — just a latent assumption worth keeping in mind if EQL ever supports non-publicdomain installs. rewriteEncryptedMigrationsreturns after the first existing candidate dir, even if that dir contained zero matches, so a later candidate dir with real EQL alters would be skipped. Pre-existing behavior, not introduced here — flagging only for awareness.skipped[].statementcan include leading content back to the previous;(e.g. a leading comment block at file top) becauseNEAR_MISS_REstarts with lazy[^;]*?. Cosmetic only — the warning is still actionable.- PR notes 3 pre-existing
@cipherstash/authAutoStrategytscerrors in untouched files; agreed those are out of scope here but would bite a future strict type gate.
Test coverage is strong (108 new tests, including the full generated v3 domain cross-product and every mangled drizzle-kit form). Good work.
Address code-review feedback on the v3 migration-rewriter port.
**Sweep every candidate migration directory.** `rewriteEncryptedMigrations`
returned after the FIRST candidate that merely existed, even when that
directory contained zero matches — so an empty or already-rewritten
`drizzle/` sitting next to a project's real `migrations/` left those
migrations unrepaired, and they then failed at migrate time with the very
`cannot cast type ...` error the rewriter exists to prevent.
The comment justifying the early return ("running again on a different
candidate would double-transform already-rewritten SQL") was wrong on both
counts: distinct directories hold distinct files, and the rewrite is
idempotent anyway — a rewritten statement no longer contains `SET DATA
TYPE`, so neither the strict matcher nor the near-miss scan can match it a
second time.
Extract the sweep into an exported, log-free `sweepMigrationDirs(cwd, dirs)`
so it is directly testable without mocking @clack/prompts. It sweeps every
existing candidate and reports a directory that throws via `error` rather
than stranding the ones after it; post-agent keeps the reporting.
**Trim the near-miss statement preamble.** `NEAR_MISS_RE` opens with a lazy
`[^;]*?` whose only left boundary is the previous `;` — or the start of file
when there is none. The reported statement therefore dragged in every
comment and blank line since then, so a near-miss in a file opening with a
comment block was quoted back to the user with the whole header glued to its
front. Strip leading blank/comment lines (incl. `--> statement-breakpoint`)
so the statement reads as the offending statement alone. Detection is
unchanged; only the text shown to the user differs.
Applied to the `stash` CLI sibling too — it carried the identical defect and
its header mandates keeping the two in sync. The directory sweep does not
apply there: the CLI takes a single explicit `outDir`.
**Document the hardcoded `"public"."<domain>"`.** Behaviour unchanged and
not a regression, but worth stating: the domain qualifier is an assumption
(EQL installs into `public`), not something read back from the matched SQL —
the `schema` capture is the TABLE's schema and says nothing about where the
domain lives. Non-public domain installs would need it threaded in here and
in the CLI sibling. Already pinned by the existing pgSchema() test.
9 new tests across the two packages, each watched failing first.
Part of the EQL v2 removal effort (#707). PR 6 of the linear stack — stacked on #770 (PR 5, stack-drizzle).
What this does
Removes the last live EQL v2 code from
@cipherstash/wizard, per the design §"PR 6".src/lib/rewrite-migrations.tsfrom theeql_v2_encrypted-only rewriter to the fulleql_v3_*family (mirroring the CLI's drizzle-kit v3: ALTER COLUMN to an eql_v3 domain emits invalid DDL (no v3 rewriter) #693 fix, re-converging the two long-coupled copies). Port, not delete — it's still live:post-agent.tscalls it afterdrizzle-kit generate, and since the wizard now scaffolds v3, the generatedALTER COLUMN … eql_v3_*statements would otherwise slip through unrepaired and fail at migrate time. Adds near-miss flagging (RewriteResult) and a stronger data-destroying warning.isEqlEncryptedDomainhelper (eql_v2_encrypted || startsWith('eql_v3_'), matching migrate'sclassifyEqlDomainconvention) inwizard-tools.ts.wizard: minor). No skill change (the wizard's internal post-agent step isn't documented in any skill).Decision 6 (deliberate)
Detection recognizes both v2 and v3 as "already encrypted" so the agent won't clobber existing v2 ciphertext, while the wizard only emits v3. No decrypt/read path removed.
Green gate
build ✓ ·
tsc --noEmitclean for touched files (3 pre-existing@cipherstash/authAutoStrategyerrors in untouched files) · test 256 pass / 5 env-skipped (+108 new tests) ·code:check0 errors.Note for reviewer
Pre-existing (not from this PR): the wizard package has 3
tscerrors about@cipherstash/auth'sAutoStrategytype resolution in untouched files — would bite if a strict type gate is added here.🤖 Generated with Claude Code