fix(cli,wizard): correctness follow-ups to the Drizzle EQL migration rewriter - #839
Merged
Conversation
`skills/stash-drizzle/SKILL.md` still told Drizzle users the sweep's repair is "data-destroying", safe "only on an empty table", and that a populated table must **not** apply the swept migration. That was true on remove-v2 and this branch makes it false — the repair is add-only and preserves the source column. It matters more than a stale sentence: SKILL_MAP.drizzle installs both stash-drizzle and stash-cli into every Drizzle project, so the two skills contradicted each other in the customer's repo, with the wrong one telling their agent not to apply a migration that is now safe. Per the AGENTS.md skill map, a Drizzle-integration change must check skills/stash-drizzle as well as skills/stash-cli. Missed in the original PR and carried through the rebase. (skills/ ships in the stash tarball; the existing changeset already scopes stash as a patch.) Also pins 'switch the application to the' in the CLI rewriter test, which the wizard twin already asserted — the parity guard compares the two source files, not the two test files, so the drift was invisible to CI.
🦋 Changeset detectedLatest commit: 3e11af6 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 migration rewriters now detect encrypted DDL inside dollar-quoted bodies, stage encrypted twin metadata only after successful writes, and expose reconciliation guidance through the CLI and wizard. Tests and documentation cover fail-closed behavior, partial sweeps, error narrowing, and Drizzle schema reconciliation. ChangesEncrypted migration rewriting
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 |
…rd holds Issue #836, item 1. `isInsideCommentOrString` skips a dollar-quoted body WHOLE. That is correct for the rewrite pass — the body is PL/pgSQL and `renderSafeAlter` returns multiple lines — but wrong for `indexColumnDeclarations`: DDL inside `DO $$ … END $$;` is EXECUTED SQL, so an encrypted `ADD COLUMN` there means the column really does hold ciphertext. It never entered the `encrypted` set, so the column fell to "plaintext by residue" and the sweep staged an empty `<col>_encrypted` twin beside the real ciphertext — `rewritten` listing the file, `skipped` empty, exit code 0. This is the mechanism #811 reported; #823 closed it by removing the blast radius (add-only emission), not by closing the mechanism. The index now reads dollar-quoted bodies via `dollarQuotedBodies`, for the ENCRYPTED side only. That is the asymmetry the per-column `CREATE TABLE` loops already rely on: over-detecting "encrypted" costs a flagged statement, while over-detecting "declared" is what puts a column back on the fail-open path. A `DO $$` body is conditional PL/pgSQL — an `IF … THEN` arm, an `EXCEPTION` handler — so a plaintext declaration inside one is not proof the column exists and is deliberately not recorded. An inert body (inside a `--` comment, or quoted in an `INSERT … VALUES`) stays inert; the rewrite pass is untouched. `target-exists` now also consults `encrypted`. The two sets are not nested: a twin added inside a dollar body is `encrypted` without ever being `declared`, and emitting a second ADD COLUMN for it fails with "column already exists". `dollarQuotedBodies` tracks comment/string state itself rather than asking `isInsideCommentOrString` about each `$`, and that is load-bearing, not a stylistic choice: the predicate rescans from index 0 on every call, so the per-opener form is quadratic. The corpus this sweeps is the drizzle output directory holding the ~2.6 MB EQL install migration the command has just written, and that file is itself thousands of small `$$` PL/pgSQL bodies — measured over it, the whole sweep runs ~0.4 s as a single pass versus ~8.5 s per-opener. It reuses the same token helpers (`dollarQuoteDelimiter`, `endOfQuoted`, `isEscapeStringOpener`) so the tokenisation rules stay identical: `"` consumed before `'`, nested block comments tracked by depth, and an unterminated comment, literal or dollar body ending the walk. A regression guard sweeps a 2.1 MB dollar-quote-heavy corpus with a 15 s bound — ~30x above the linear time so a slow shared runner stays comfortable, ~3x below the quadratic time; it measures ~70 ms as written and 41 s against the per-opener form. The alternative in the issue — fail closed on any table touched inside a dollar-quoted body — was rejected. It would trip on drizzle-kit's own `DO $$ … CREATE TYPE … EXCEPTION` enum idiom and flag every ALTER in such a corpus, for a residual (dynamic `EXECUTE` SQL) that is the same corpus-invisible class as the psql/dashboard drift `skills/stash-cli` already documents. A test pins that idiom as still rewritable. Three pre-existing expectations tightened, all toward fail-closed: - the `DO $$` ADD COLUMN corpus no longer rewrites at all. #823's own test codified the wrong outcome here (`rewrite-migrations.test.ts:977-998`). - both rename corpora report `already-encrypted` rather than `target-exists`. After those renames `email` IS the ciphertext, and `target-exists`'s "review the existing encrypted twin" pointed at a column the rename had consumed. - an unterminated `$$` now flags rather than rewrites. The file is unparseable, so Postgres ran none of it and nothing after the opener is proven.
Issue #836, item 2. `renderSafeAlter` is add-only, so after a sweep the database has `email text` PLUS `email_encrypted eql_v3_text_search`, while `schema.ts` still declares `email` as the encrypted domain — that declaration is what made drizzle-kit emit the impossible `SET DATA TYPE` in the first place — and `meta/*_snapshot.json` records the same. Neither knows the twin exists. `drizzle-kit generate` gives no signal: it diffs `schema.ts` against the snapshot, never reads `.sql`, and never introspects the database. Both inputs still agree, so the diff is empty. Every consequence through the ORM is therefore silent — reads of `users.email` hand plaintext to a `customType.fromDriver` expecting an EQL envelope, writes push an envelope into a `text` column and SUCCEED, and `email_encrypted` is unreachable because it is in no Drizzle schema. It only fails loudly much later: correcting `schema.ts` by hand makes `generate` diff against the stale snapshot and emit a duplicate ADD COLUMN, which errors with "column already exists". `RewriteResult` now carries `staged: StagedColumn[]` — one entry per rewritten statement, so it is finer-grained than `rewritten`, which lists files — and `describeStagedReconciliation` turns it into guidance naming the table, both columns and the domain. `stash eql migration --drizzle` and the wizard's post-agent step both print it, the wizard before its migrate prompt so the user decides with it in hand. The partial-sweep path reports it too: those twins are already on disk and already divergent. Warn rather than exit non-zero — the swept SQL is valid and additive, and which way to reconcile is the user's call. Writing the `schema.ts` edit was rejected: editing it alone CREATES the duplicate-column failure above, so a correct automatic fix would have to rewrite drizzle-kit's snapshot in lockstep, in an internal format that is version-coupled and not ours. Detecting it in `stash encrypt plan` was rejected as too late — it runs later, and may never run. Item 3, same subsystem. `sweepMigrationDirs` read its partial result off an unchecked `err as Partial<RewriteSweepError>`. For a non-object throw (`throw null`) that property read raises a TypeError INSIDE the catch, so the per-directory error result is never pushed, the throw escapes `sweepMigrationDirs` entirely, and the remaining directories go unswept — the fail-closed reporting the catch exists to provide simply does not happen. The CLI had been hardened against exactly this; the wizard had not, and it was the only unresolved review thread on merged #823. Rather than duplicate the predicate, `PartialRewriteResult`/`isPartialRewriteResult` move into the shared part of both rewriter copies — so the parity guard now polices them, which it could not do while the wizard's version sat inside `#region wizard-only` — and `eql/migration.ts` imports instead of defining its own. A test forces `throw null` through the writeFile spy and pins that the directory is still reported and the sweep still continues; it fails with the exact "Cannot read properties of null (reading 'rewritten')" against the old cast.
… $$ rule Per AGENTS.md, a change to the CLI command surface or a user-facing workflow must update the affected shipped skills in the same PR — `skills/` ships inside the `stash` tarball and is copied into customer repos, so drift here becomes wrong guidance in someone else's codebase. `skills/stash-drizzle` gains the reconciliation step for #836 item 2: a table of what the database, `schema.ts` and `meta/*_snapshot.json` each say after a sweep, why `drizzle-kit generate` reports nothing (it diffs schema against snapshot and reads neither the `.sql` nor the database — introspection is `push`), the three silent ORM consequences, and the corrected `schema.ts` with both columns declared. It also warns against the half-fix: changing only the source column's type and re-generating against the stale snapshot emits a duplicate ADD COLUMN that fails with `column already exists`. This lands on the paragraph the preceding commit corrected for #837, which is why that correction had to come first — writing this on top of the old "data-destroying / safe only on an empty table" text would have silently reinstated it. `skills/stash-cli` gains the same reconciliation note, scoped to what the command prints, plus the fail-closed rule's new detail: a declaration inside a `DO $$ … END $$;` block counts toward "already encrypted", while a plaintext one inside such a block does not count as declaring the column. Dynamic `EXECUTE` SQL joins hand-psql and dashboard edits in the list of corpus-invisible drift. Verified against the registry per AGENTS.md — every `--flag` the `eql migration` section names resolves against `stash manifest --json` (`--eql-version` appears only in the sentence saying it does not exist). Changeset: `stash` patch, `@cipherstash/wizard` patch.
… work Review of #839 found the remediation text walked the user into the very failure it warned about, and that the notice could name a column that was never written. **The guidance was wrong.** It said to declare the twin under its own name, "keep the source column as its plaintext type", then run `drizzle-kit generate` to bring the snapshot forward — and warned separately against hand-editing "only" the schema. Both halves were off: - After a sweep `schema.ts` declares the SOURCE column as the encrypted domain; that declaration is what produced the invalid ALTER. So it has to be set BACK to plaintext — "keep" describes a state the schema is not in. - The duplicate `ADD COLUMN` is not caused by editing partially. The snapshot has never seen the twin, so `generate` ALWAYS emits an `ADD COLUMN` for it, and the swept migration already added that column — applying both fails with "column already exists". Following the old text produced exactly that error, whether or not the source column was reverted. The notice and both skills now give the sequence that reaches a working state: fix the schema (twin under its own name, source back to plaintext), run `generate` to advance the snapshot, then DELETE the regenerated `ADD COLUMN` for the twin from the migration it wrote — keeping the snapshot advance, which is the point of the step. `IF NOT EXISTS` is noted as an equivalent. Anything else `generate` emits, such as the now no-op `SET DATA TYPE` on the source column, can stay. `skills/stash-drizzle` also documents the cleaner alternative when the swept migration has not been applied yet: revert the schema, remove that migration (`.sql`, `meta/*_snapshot.json`, and its `meta/_journal.json` entry) and follow the canonical staged rollout, which generates the ADD COLUMN itself. **`staged` could name a column that was never persisted.** The entries are produced inside `.replace()`, which runs before `writeFile`. A sweep that threw on a later file therefore attached twins from a file whose write had failed, and the notice claims "the database now has" those columns. They are now buffered per file and committed only after that file's write succeeds. `stagedTargets`, the in-run duplicate guard, is deliberately not rolled back: if a write fails, treating the target as taken keeps a later file from staging it again. Found by writing the test for the previously uncovered partial-sweep reporting path — it reproduced as `['email', 'name']` where only `email` reached disk. Also surfaces `StagedColumn.file`, which was populated and documented but never read: the per-column line now names the migration the twin was staged in, so the user can go read the statement instead of searching the directory. Coverage for the three paths this PR added but never exercised: - `describeStagedReconciliation` must say "back to its plaintext type" and must name a delete-the-ADD-COLUMN step; it must not say "keep the source column" or blame partial editing. Prose assertions, but they pin the two claims that make the difference between guidance that works and guidance that fails. - `eql migration --drizzle` reports staged twins from a sweep that threw part way through — fails if the `partial.staged` branch is removed. - `sweepMigrationDirs` carries staged twins out of a directory whose sweep threw, and carries only the ones that reached disk — fails if the `partial.staged ?? []` carry is dropped. stash 915 passed (60 files, default timeouts), wizard 362 passed, parity guard 4 passed, build 9/9, code:check 0 errors.
freshtonic
approved these changes
Jul 30, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Reviewed in an isolated worktree (at f33f21b), running the rewriter + parity test suites myself.
Approving. A careful, correctness-focused follow-up that closes three silent-wrong-state defects in the add-only Drizzle EQL migration rewriter — and every fix moves in the fail-closed direction.
Verified
- Fix 1: the corpus index now reads dollar-quoted (
DO $$ … $$) bodies for the encrypted side only — the safe asymmetry, since over-detecting "encrypted" merely flags a statement for manual review, while over-detecting "declared" is the fail-open path they deliberately avoid. Inert bodies (comment/string) stay inert; unterminated bodies over-read toward flagging. - Fix 2: surfaces the schema/snapshot/database three-way divergence with corrected guidance (revert source column + delete the regenerated
ADD COLUMN), and only reports twins that actually reached disk (buffered per-file, committed after write). - Fix 3: narrows instead of casting so a non-
Errorthrow can'tTypeErrorinside the reporting catch. - Both rewriter copies got identical changes (parity guard passes), and the shared predicates were moved out of the wizard-only region so the guard polices them. Tests are genuine behavioral tests against real SQL. Ran: CLI rewrite-migrations (194) + eql migration (38), wizard rewrite-migrations (203) + post-agent (15), copies-in-sync parity (4);
code:checkexits 0. Changeset (stash + wizard patch) present;stash-cli+stash-drizzleskills updated — and the stash-drizzle change correctly drops the previously-wrong "data-destroying / safe only on an empty table" claim.
Non-blocking findings
rewrite-migrations.ts(indexEncryptedRenames, minor): a rename chain dollar-body→live within one file isn't followed (live rename pass runs before the dollar rename pass), so a column made encrypted via a rename insideDO $$then renamed again by a live statement could stage an empty twin beside real ciphertext. Documented as accepted residue; unreachable in real drizzle-kit output (never emitsRENAME COLUMNinsideDO $$) and add-only, so no data loss. Acceptable.rewrite-migrations.test.ts:567-591(+ wizard mirror :1969) (minor): the perf guard asserts wall-clockelapsed < 15_000— timing assertions are flake-prone on loaded runners. Mitigated by a ~30x margin over ~0.4s linear time and it catches a genuine shippable-latency quadratic regression, but it's a latent source of CI noise.post-agent.ts:~2143(nit): warns gated ondidStage(rewritten>0) whereas the CLI gates onstaged.length > 0. The invariant holds, so consistency-only; guarding onsweep.staged.lengthwould match the CLI and be resilient if the invariant ever changes.
Caveat: didn't run a live end-to-end drizzle-kit generate/migrate cycle (reconciliation guidance verified via unit assertions on emitted text) or credentialed integration suites — the column already exists mechanism is well-reasoned regardless.
coderdan
approved these changes
Jul 30, 2026
Review round 3. One accepted-residue verdict overturned, two addressed as raised. **The rename-chain residue is reachable, so it is now fixed rather than documented.** `indexColumnDeclarations` ran the live rename pass before the dollar-quoted one, so a chain crossing dollar→live inside one file lost the type: `ADD COLUMN "tmp" <domain>` live, `RENAME tmp TO mid` inside `DO $$`, then a live `RENAME mid TO email` left `email` looking like plaintext and staged an empty twin beside its ciphertext — the exact defect this branch exists to close. It was accepted on the grounds that drizzle-kit never emits `RENAME COLUMN` inside `DO $$`. True, and the wrong reachability test: the corpus #811 actually reported is hand-written and does precisely this, a human staging a manual column swap. `rewrite-migrations.test.ts`'s own "reported two-file DO $$ rename corpus" fixture is that shape. Reachability here is about what people write by hand, not what the generator emits. Both rename passes now iterate to a fixpoint, so which one runs first stops mattering and chains are followed in either direction. It terminates because both sets only grow and are bounded by the distinct column keys in the corpus, and it converges one iteration past the longest chain — a corpus with no renames pays a second scan that finds nothing. The dollar-quote-heavy perf guard is unmoved at ~70 ms. Tests cover both directions, so a future reordering of the passes cannot break one while the other stays green. **The wizard's reconciliation notice is now gated on `staged`, matching the CLI.** The two conditions are equivalent today — a rewritten file always yields at least one staged column, and both are recorded together only after the write succeeds — so this is consistency, not a behaviour change, and no test can distinguish them. The notice describes the staged columns, so it should be driven by whether there are any rather than by a file count that happens to track them. **The perf guard's documented margin was understated, which is what made it look marginal.** The comment claimed ~30x headroom, derived from a hand-waved "well under a second". Measured, the whole sweep over that corpus is ~67-71 ms against a 15 s bound: ~200x. Corrected, with the flake question answered concretely — tripping it needs a runner ~200x slower at scanning a string than a developer laptop, where CI is 2-5x — and a note to raise the bound rather than delete the test if it ever does fire, since what it guards is shipped-command latency. Also drops the now-stale "Residue, accepted" paragraph from `indexEncryptedRenames`. stash 917 passed (60 files, default timeouts), wizard 364 passed, parity guard 4 passed, build 9/9, code:check 0 errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #836.
Three silent-wrongness defects in the Drizzle EQL migration rewriter, all of which exited 0 while leaving the user in a wrong state. Neither of the first two was data loss — the rewrite never emits
DROP COLUMNorRENAME COLUMN— but neither was visible either.Sequencing note
#836 says to land #837's
skills/stash-drizzle/SKILL.md:64correction first, because item 2 adds guidance to that same paragraph and editing in the other order silently reinstates the false "data-destroying / safe only on an empty table" claim. That correction already existed as an unpushed local commit (e3695929), so it is cherry-picked as the first commit here rather than waiting on #837. The remaining #837 items are untouched and still belong to that issue.1. Dollar-quoted DDL no longer bypasses the already-encrypted guard
isInsideCommentOrStringskips a dollar-quoted body whole. Correct for the rewrite pass — the body is PL/pgSQL andrenderSafeAlterreturns multiple lines — but wrong for the index pass: DDL insideDO $$ … END $$;is executed SQL, so an encryptedADD COLUMNthere means the column really holds ciphertext. It never enteredencrypted, fell to "plaintext by residue", and the sweep staged an empty twin beside the real ciphertext.The index now reads dollar-quoted bodies for the encrypted side only. That is the asymmetry the file already relies on: over-detecting
encryptedcosts a flagged statement, over-detectingdeclaredis the fail-open direction. ADO $$body is conditional PL/pgSQL, so a plaintext declaration inside one still does not count as declaring the column.target-existsnow also consultsencrypted— the sets are not nested, and a twin added inside a dollar body was previously invisible to it, producing a duplicateADD COLUMNthat fails at migrate time.Decision taken (the issue left this open): index the bodies, rather than fail closed on any table touched inside one. A blanket fail-closed would trip on drizzle-kit's own
DO $$ … CREATE TYPE … EXCEPTIONenum idiom and flag every ALTER in such a corpus — for a residual (dynamicEXECUTESQL) that is the same corpus-invisible class as the psql/dashboard driftskills/stash-clialready documents. A test pins that idiom as still rewritable.Three pre-existing expectations tightened, all toward fail-closed — including the one #823 codified backwards at
rewrite-migrations.test.ts:977-998. Both rename corpora now reportalready-encryptedrather thantarget-exists: after those renamesemailis the ciphertext, andtarget-exists's "review the existing encrypted twin" pointed at a column the rename had consumed.One implementation constraint is load-bearing and worth a reviewer's eye.
dollarQuotedBodiestracks comment/string state itself instead of askingisInsideCommentOrStringabout each$. That predicate rescans from index 0 on every call, so the per-opener form is quadratic — and the corpus this sweeps is the drizzle output directory holding the ~2.6 MB EQL install migration the command has just written, which is itself thousands of small$$PL/pgSQL bodies. Measured there: ~0.4 s single-pass vs ~8.5 s per-opener for the whole sweep. It reuses the same token helpers so tokenisation is unchanged ("before', nested block comments by depth, unterminated tokens ending the walk), and a regression guard sweeps a 2.1 MB dollar-quote-heavy corpus with a 15 s bound — ~30x above linear so a slow runner stays comfortable, ~3x below quadratic. It measures ~70 ms as written and 41 s against the per-opener form.2. A successful sweep now reports the artefact divergence it leaves
After a sweep the database has
email textplusemail_encrypted eql_v3_text_search, whileschema.tsstill declaresemailas the domain — that declaration is what made drizzle-kit emit the impossibleSET DATA TYPE— andmeta/*_snapshot.jsonrecords the same. Neither knows the twin exists.drizzle-kit generategives no signal: it diffs schema against snapshot, reads neither the.sqlnor the database, and both inputs still agree. So every consequence was silent — reads hand plaintext to afromDriverexpecting an EQL envelope, writes push an envelope into atextcolumn and succeed, and the twin is unreachable.RewriteResultnow carriesstaged: StagedColumn[], anddescribeStagedReconciliationturns it into guidance naming the table, both columns and the domain. Both callers print it; the wizard prints it before its migrate prompt. The partial-sweep path reports it too.Decision taken: warn, not write the
schema.tsedit. Editing it alone creates the duplicate-column failure, so a correct automatic fix would have to rewrite drizzle-kit's snapshot in lockstep, in a version-coupled internal format that is not ours.stash encrypt planwas rejected as too late — it runs later and may never run.3. Wizard sweep reporting survives a non-
ErrorthrowsweepMigrationDirsread its partial result off an unchecked cast, sothrow nullraised aTypeErrorinside the catch meant to report the failure — skipping the error result and abandoning the remaining directories. It now narrows. Rather than duplicate the CLI's predicate,PartialRewriteResult/isPartialRewriteResultmove into the shared part of both copies, so the parity guard polices them — which it could not while the wizard's version sat inside#region wizard-only.This was the only unresolved review thread on merged #823.
Review round 2 — fixes applied
Self-review found the reconciliation guidance was wrong in a way that undercut item 2's whole purpose, plus a real bug behind it. Fixed in
f33f21b, TDD with regression tests.The guidance walked the user into the error it warned about. It said to keep the source column "as its plaintext type" and run
generateto advance the snapshot, warning separately against editing "only" the schema. Both halves were off: after a sweepschema.tsdeclares the source column as the domain (that declaration is what produced the invalid ALTER), so it must be set back to plaintext; and the duplicateADD COLUMNisn't caused by partial editing — the snapshot has never seen the twin, sogeneratealways emits anADD COLUMNfor it, and the swept migration already added that column. The old text producedcolumn already existseither way.The notice and both skills now give the sequence that reaches a working state: fix the schema, run
generate, then delete the regeneratedADD COLUMNfor the twin (keeping the snapshot advance, which is the point).IF NOT EXISTSnoted as equivalent.skills/stash-drizzlealso documents the cleaner path when the swept migration hasn't been applied yet — revert, remove the migration and its journal/snapshot entries, and use the canonical rollout.stagedcould name a column that was never written. Entries are produced inside.replace(), which runs beforewriteFile, so a sweep throwing on a later file attached twins from a failed write — and the notice claims "the database now has" them. Now buffered per file and committed only after that file's write succeeds.stagedTargets(the in-run duplicate guard) is deliberately not rolled back. Surfaced by writing the test for the previously uncovered partial-sweep path: it reproduced as['email', 'name']where onlyemailreached disk.StagedColumn.fileis now read — the per-column line names the migration the twin was staged in, rather than carrying a field nothing consumed.Coverage for the three paths this PR added but never exercised, each verified to fail when the code it covers is removed:
describeStagedReconciliationmust say "back to its plaintext type" and name a delete-the-ADD COLUMNstep; must not say "keep the source column" or blame partial editingeql migration --drizzlereports staged twins from a sweep that threw part way throughsweepMigrationDirscarries staged twins out of a failed directory — and only those that reached diskReview round 3 — fixes applied
Three findings raised as minor/nit. Two addressed as described; one verdict overturned.
3e11af6a.The rename-chain residue was reachable, so it is fixed rather than documented. The live rename pass ran before the dollar-quoted one, so a chain crossing dollar→live inside one file lost the type —
ADD COLUMN "tmp" <domain>live,RENAME tmp TO midinsideDO $$, then liveRENAME mid TO email— leavingemaillooking like plaintext and staging an empty twin beside its ciphertext.It had been accepted on the grounds that drizzle-kit never emits
RENAME COLUMNinsideDO $$. That is true but the wrong reachability test: the corpus #811 actually reported is hand-written and does exactly this, a human staging a manual column swap — it is thereported two-file DO $$ rename corpusfixture in this very test file. What matters here is what people write by hand, not what the generator emits.Both rename passes now iterate to a fixpoint, so pass order stops mattering and chains follow in either direction. Terminates (both sets only grow, bounded by distinct column keys) and converges one iteration past the longest chain. The perf guard is unmoved at ~70 ms. Tests cover both directions so a future reordering can't break one while the other stays green.
The wizard notice now gates on
staged.length > 0, matching the CLI. Equivalent today — a rewritten file always yields ≥1 staged column, and both are recorded only after the write succeeds — so consistency rather than behaviour, and no test can distinguish them. Correct on the merits: the notice describes staged columns, so it should be driven by whether there are any.The perf guard's documented margin was understated, which is what made it look marginal. The comment claimed ~30x, derived from a hand-waved "well under a second". Measured, the sweep over that corpus is 67-71 ms against a 15 s bound — ~200x. Corrected in the comment, with the flake question answered concretely: tripping it needs a machine ~200x slower at scanning a string than a developer laptop, where CI is 2-5x. Kept rather than restructured, with a note to raise the bound rather than delete the test if it ever fires, since it guards shipped-command latency. Happy to swap it for a deterministic static check that
dollarQuotedBodiesdoesn't call the per-index predicate, if you'd rather have zero wall-clock assertions — that trades "catches any quadratic implementation" for "catches this one deterministically".Verification
stash917 passed / 60 files, at the default 5 s timeout@cipherstash/wizard364 passed, 5 skippedpnpm run build: 9/9pnpm run code:check: 0 errors (178 warnings, as on main)--flagtheeql migrationskill section names resolves againststash manifest --json@cipherstash/stackand@cipherstash/prisma-nexttests fail locally for want ofCS_*credentials — verified identical on pristinec8b1325a, and no code in either package is touched.Changeset:
stashpatch,@cipherstash/wizardpatch (skills ship in thestashtarball).Summary by CodeRabbit
Bug Fixes
DO $$ ... END $$blocks.New Features
Documentation