Make the Drizzle EQL migration rewriter non-destructive and fail closed - #823
Conversation
🦋 Changeset detectedLatest commit: 143a99d 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 |
📝 WalkthroughWalkthroughThe Drizzle migration rewriters now add staged ChangesDrizzle migration rewriter
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/cli/src/commands/db/rewrite-migrations.ts (1)
831-854: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the
"public"rationale to both rewriters or centralize the assumption.
packages/wizard/src/lib/rewrite-migrations.tsdocuments that"public"."<domain>"is an install-schema assumption, butpackages/cli/src/commands/db/rewrite-migrations.tsemits the same unconditional qualifier with only a short comment. Keep the contract in one shared rationale that both copies inherit; otherwise this kind of fix is easily applied to one tool and not the other.🤖 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/rewrite-migrations.ts` around lines 831 - 854, Centralize the rationale for the unconditional "public" domain qualifier used by renderSafeAlter and the corresponding wizard rewriter, or add the same install-schema explanation to both implementations. Ensure both generated ALTER statements retain the "public" qualifier while inheriting the shared/documented contract rather than relying on only the CLI’s brief comment.packages/cli/src/commands/db/install.ts (1)
672-675: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the partial-result carrier before reading it.
A caught value may be
null/undefined; this assertion does not change runtime behavior, sopartial.rewrittencan throw and mask the intendedCliExit(1)path. Narrowerrorto a non-null object before reading optional fields. As per coding guidelines, source code must avoid type-erasing assertions unless deliberately suppressed with a reason.Proposed fix
- const partial = error as Partial<{ - rewritten: string[] - skipped: SkippedAlter[] - }> + const partial = + error !== null && typeof error === 'object' + ? (error as Partial<{ + rewritten: string[] + skipped: SkippedAlter[] + }>) + : {}[recommendation: use a small type guard to avoid retaining the assertion]
🤖 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 672 - 675, Update the error handling around the partial-result carrier to safely narrow the caught value to a non-null object before accessing rewritten or skipped. Replace the type assertion with a small type guard, preserving the intended CliExit(1) path for null, undefined, and non-object errors.Source: Coding guidelines
🤖 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.
Inline comments:
In `@packages/cli/src/__tests__/rewrite-migrations.test.ts`:
- Around line 1611-1638: Replace the UID-dependent read-only-file setup in the
partial-progress test for rewriteEncryptedAlterColumns with a failure mechanism
that works when running as root, preferably creating 0002_name.sql as a
directory so the rewrite produces EISDIR; apply the identical change in
packages/cli/src/__tests__/rewrite-migrations.test.ts lines 1611-1638 and
packages/wizard/src/__tests__/rewrite-migrations.test.ts lines 1551-1578,
including matching cleanup.
In `@packages/cli/src/commands/eql/migration.ts`:
- Around line 279-284: Update the sweepIncomplete abort path in the migration
function to emit the same conditional standalone outro, `if (!embedded)
p.outro('Migration aborted.')`, before throwing `new CliExit(1)`, matching the
other non-zero abort paths while preserving the existing error log.
In `@packages/wizard/src/lib/post-agent.ts`:
- Around line 95-104: Remove the aggregate warning blocks guarded by skipped and
unverified in the post-agent flow, leaving rewriteEncryptedMigrations as the
sole source of these per-directory warnings and their accompanying details.
---
Nitpick comments:
In `@packages/cli/src/commands/db/install.ts`:
- Around line 672-675: Update the error handling around the partial-result
carrier to safely narrow the caught value to a non-null object before accessing
rewritten or skipped. Replace the type assertion with a small type guard,
preserving the intended CliExit(1) path for null, undefined, and non-object
errors.
In `@packages/cli/src/commands/db/rewrite-migrations.ts`:
- Around line 831-854: Centralize the rationale for the unconditional "public"
domain qualifier used by renderSafeAlter and the corresponding wizard rewriter,
or add the same install-schema explanation to both implementations. Ensure both
generated ALTER statements retain the "public" qualifier while inheriting the
shared/documented contract rather than relying on only the CLI’s brief comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 80a16ecb-b08f-4e52-981e-ebd45a0fec9e
📒 Files selected for processing (14)
.changeset/staged-add-only-rewriter.mdpackages/cli/src/__tests__/rewrite-migrations.test.tspackages/cli/src/commands/db/__tests__/generate-drizzle-migration.test.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/db/rewrite-migrations.tspackages/cli/src/commands/db/supabase-migration.tspackages/cli/src/commands/eql/__tests__/migration.test.tspackages/cli/src/commands/eql/migration.tspackages/wizard/src/__tests__/post-agent.test.tspackages/wizard/src/__tests__/rewrite-migrations.test.tspackages/wizard/src/lib/post-agent.tspackages/wizard/src/lib/rewrite-migrations.tsscripts/__tests__/rewriter-copies-in-sync.test.mjsskills/stash-cli/SKILL.md
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approve
Reviewed the full diff at e3ee5e6a (4 commits on remove-v2), reading the rewriter's complete lexer myself and running two focused audits — copy parity + caller fail-closed, and a non-destructive-output check. No blocking defects; all 7 CI checks green. This is a careful, well-tested conversion of the Drizzle EQL rewriter from destructive (ADD+DROP+RENAME) to additive-and-fail-closed.
What I verified
Additive-only is actually guaranteed. renderSafeAlter emits exactly one statement shape — ALTER TABLE … ADD COLUMN "<col>_encrypted" "public"."<domain>"; — preceded only by comment lines. DROP COLUMN survives solely in JSDoc explaining why a commented-out fragment must not be mistaken for live SQL; RENAME_COLUMN_RE is read-only, feeding column identity into the encrypted/declared fail-closed sets (never emitted). The source column is preserved in every branch.
Every caller fails closed. install.ts and eql/migration.ts set sweepIncomplete on a non-empty skipped (success path) and in the catch (surfacing partial rewritten/skipped), then throw new CliExit(1). The wizard's post-agent.ts throws on skipped || unverified, and — nicely — keys directory failure on error !== undefined (presence, not truthiness) so an empty-message sweep error still counts as unverified. The old wizard "warn and default the prompt to NO" path is replaced by a hard throw, and the now-safe additive rewrite lets the migrate prompt default to true legitimately.
The #811 dollar-quote regression is fixed at the lexer and covered by tests. dollarQuoteDelimiter refuses to open a $…$ body when the preceding char is an identifier char (matching Postgres's separator rule), so price$usd$cents / price$$cents no longer open phantom bodies that hide later live SQL; $$…$$ and $tag$…$tag$ bodies are skipped whole. The over-termination reasoning from #772-finding-1 (an early-closed literal is what flips a commented-out ALTER to "live") is applied correctly across the ", ', E'', and $$ branches, with " tokenized before '. Tests exercise odd-apostrophe dollar bodies, tagged bodies, both identifier-$ shapes, and a "still rewrites a live ALTER after a dollar body" case that proves the skip doesn't over-reach.
Copy discipline holds. The two rewriter copies share byte-identical core logic; they diverge only in imports, prose comments, the emitted tool-name header, and an explicit #region wizard-only block (the multi-directory sweep the wizard alone needs). rewriter-copies-in-sync.test.mjs strips comments/imports/blanks, canonicalizes the header, excludes the region, and asserts exact equality — with guard tests against both over-normalization and logic sneaking under the region marker. Real logic drift would fail it.
Fail-closed reasons are precise and well-explained. already-encrypted / source-unknown / target-exists / unrecognised-form each get distinct, actionable describeSkipReason guidance, and the corpus index deliberately over-detects "encrypted" (costs a flagged statement) rather than risk under-detecting (costs irrecoverable ciphertext) — the asymmetry is documented and correct. Changeset (stash + @cipherstash/wizard patch) and the shipped stash-cli skill are both updated accurately: the destructive "ADD+DROP+RENAME / safe only on empty table" and "DROP COLUMN would destroy" language is gone, replaced with the staged add-only + exits-non-zero description.
Non-blocking
- Unrelated import churn.
install.ts,eql/migration.ts, andsupabase-migration.tsswitch@/…→ relative paths, but 16 otherpackages/cli/srcfiles keep@/. It's behavior-neutral (green CI) but unrelated to this PR and now inconsistent — worth reverting to keep the security-focused diff tight, or applying repo-wide if there's a bundling reason. - Merge ordering with #825. This PR touches
install.tsand makes a trivial import change tosupabase-migration.ts, both of which sibling PR #825 (also onremove-v2) rewrites/deletes —supabase-migration.tsis deleted there. Whichever lands second will need conflict resolution; the rewriter-caller logic ineql/migration.tssurvives #825 (which keepsstash eql migration --drizzle), so nothing is lost, just a heads-up. - The author's documented "accepted residue" (nested-block-comment double-report as
unrecognised-form; theDECLARED_COLUMN_REkeyword-list edge cases likeSIMILAR/REFERENCES <table>) are all in the over-flag direction — a manual-review statement, never a fail-open — and strictly narrower than the gaps they replace. Correct calls; no action needed.
Excellent, rigorous work — the lexer comments alone document the exact failure mode each guard prevents. LGTM.
…base Rebasing onto remove-v2 drops `eql install --drizzle` and `generate-drizzle-migration.test.ts` with it, so the fail-closed work that lived on that path moves to the surviving one: - port the `isPartialRewriteResult` narrowing from the deleted `generateDrizzleMigration` into `eql migration --drizzle`, replacing the unchecked cast on the thrown sweep result - port the non-object sweep-failure coverage (`null` / `undefined` / a bare string) into `commands/eql/__tests__/migration.test.ts` - say "switch the application to the encrypted column by name, then drop plaintext" wherever the rewriter, the wizard prompt, and the shipped skill used to say "cutover" — `stash encrypt cutover` no longer exists
Both found by audit, neither caught by CI:
- `isPartialRewriteResult` used an annotation where the branch's original
used a cast, producing TS2322 ("Type 'object' is not assignable to type
'Record<string, unknown>'"). No runtime effect — packages/cli has no
typecheck job — but it breaks `tsc -p packages/cli` and shows in editors.
Restored the branch's own form.
- The emitted migration header lost the word "dual-write", which upstream
had deliberately added to it. Dual-write is the precondition
`--confirm-dual-writes-deployed` exists to enforce, and this text ships
into customer migration files. Restored in both rewriter copies; parity
test still byte-identical.
…oval `remove-v2` deleted `db/__tests__/generate-drizzle-migration.test.ts` along with `eql install --drizzle`. Seven of its blocks tested behaviour that still exists line-for-line in `generateDrizzleEqlMigration`, and two survivors here asserted less than the deleted originals did. Re-homed against `eql migration --drizzle`: - `--name` rejection for command substitution, backticks, a path separator, and `''` — the input where "empty" and "absent" diverge, since `''` is not nullish and so slips past `?? DEFAULT_MIGRATION_NAME` into the regex - name validation precedes the dry-run preview (ordering invariant) - `--out` appears in the dry-run preview - `--out` defaults to an absolute `drizzle/`; because `--out` is always appended to drizzle-kit's argv, a flagless run overrides the project's drizzle.config.ts `out`, and no test exercised that arm - the second and third arms of the spawn-failure chain (ENOENT, and a non-zero status with no stderr and no spawn error) - the remediation hint when drizzle-kit writes somewhere we don't look Strengthened: the argv assertion is `toEqual` on the full array plus the command, so dropping the `exec` prefix (regressing to download-and-run, which resolves a different drizzle.config.ts) now fails; `detectPackageManager` is pinned so that assertion is not tautological. The sibling-rewrite test regains the command-level add-only invariant and the "Rewrote N" success line.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/wizard/src/lib/rewrite-migrations.ts`:
- Around line 944-952: Update the catch block in sweepMigrationDirs to read
partial rewritten/skipped results only when err is an Error instance, matching
the existing isPartialRewriteResult narrowing behavior; for non-object throws
such as null or undefined, report empty arrays while preserving the extracted
error message and per-directory result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cdae8e5c-47a3-4082-8542-369fc0d2cd1e
📒 Files selected for processing (11)
.changeset/staged-add-only-rewriter.mdpackages/cli/src/__tests__/rewrite-migrations.test.tspackages/cli/src/commands/db/rewrite-migrations.tspackages/cli/src/commands/eql/__tests__/migration.test.tspackages/cli/src/commands/eql/migration.tspackages/wizard/src/__tests__/post-agent.test.tspackages/wizard/src/__tests__/rewrite-migrations.test.tspackages/wizard/src/lib/post-agent.tspackages/wizard/src/lib/rewrite-migrations.tsscripts/__tests__/rewriter-copies-in-sync.test.mjsskills/stash-cli/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/staged-add-only-rewriter.md
- packages/wizard/src/lib/post-agent.ts
- packages/wizard/src/tests/post-agent.test.ts
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err) | ||
| results.push({ dir, rewritten: [], skipped: [], error: message }) | ||
| const partial = err as Partial<RewriteSweepError> | ||
| results.push({ | ||
| dir, | ||
| rewritten: partial.rewritten ?? [], | ||
| skipped: partial.skipped ?? [], | ||
| error: message, | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the partial-result read against non-object throws.
err as Partial<RewriteSweepError> is an unchecked cast: for throw null / throw undefined, partial.rewritten raises a TypeError inside the catch and escapes sweepMigrationDirs, defeating the per-directory fail-closed reporting. The CLI caller narrows explicitly (isPartialRewriteResult in packages/cli/src/commands/eql/migration.ts); mirror that here. The rewriter only attaches partials to Error instances, so an instanceof check is sufficient.
🛡️ Proposed fix
} catch (err) {
const message = err instanceof Error ? err.message : String(err)
- const partial = err as Partial<RewriteSweepError>
+ const partial: Partial<RewriteSweepError> =
+ err instanceof Error ? (err as RewriteSweepError) : {}
results.push({
dir,
rewritten: partial.rewritten ?? [],
skipped: partial.skipped ?? [],
error: message,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err) | |
| results.push({ dir, rewritten: [], skipped: [], error: message }) | |
| const partial = err as Partial<RewriteSweepError> | |
| results.push({ | |
| dir, | |
| rewritten: partial.rewritten ?? [], | |
| skipped: partial.skipped ?? [], | |
| error: message, | |
| }) | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err) | |
| const partial: Partial<RewriteSweepError> = | |
| err instanceof Error ? (err as RewriteSweepError) : {} | |
| results.push({ | |
| dir, | |
| rewritten: partial.rewritten ?? [], | |
| skipped: partial.skipped ?? [], | |
| error: message, | |
| }) |
🤖 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/wizard/src/lib/rewrite-migrations.ts` around lines 944 - 952, Update
the catch block in sweepMigrationDirs to read partial rewritten/skipped results
only when err is an Error instance, matching the existing isPartialRewriteResult
narrowing behavior; for non-object throws such as null or undefined, report
empty arrays while preserving the extracted error message and per-directory
result.
2 participants
Summary
Verification
Fixes #813
Fixes #811
Summary by CodeRabbit
New Features
Bug Fixes
Documentation