fix(cli,wizard): rewrite an encrypted ALTER only for a column the corpus declares#783
Merged
Conversation
…lares 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.
…in the rewrite sweep
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.
…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.
…gration 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.
… post-agent test 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.
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.
…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.
…efixed near-misses 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`.
…ed, not rewrote `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.
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.
|
| Name | Type |
|---|---|
| @cipherstash/wizard | Patch |
| stash | Patch |
| @cipherstash/e2e | Patch |
| @cipherstash/basic-example | Patch |
| @cipherstash/stack | Patch |
| @cipherstash/stack-drizzle | Patch |
| @cipherstash/stack-supabase | Patch |
| @cipherstash/prisma-next | Patch |
| @cipherstash/bench | Patch |
| @cipherstash/test-kit | Patch |
| @cipherstash/prisma-next-example | Patch |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a 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 |
This was referenced Jul 24, 2026
Closed
tobyhede
added a commit
that referenced
this pull request
Jul 25, 2026
The wizard cannot discover a project's configured drizzle-kit `out`, so it tries drizzle/, migrations/ and src/db/migrations/. Two of those are generic names — Knex, node-pg-migrate, Flyway and hand-rolled psql all use them — and this sweep emits DROP COLUMN. So a project whose out is drizzle/ but which also keeps a hand-maintained migrations/ had that second directory rewritten into ADD+DROP+RENAME, in a directory the wizard was never pointed at. #783's fail-closed rule does not help: it indexes per directory, and a real migration history declares its own columns, so the guard passes and the rewrite proceeds. Worse, the emitted `--> statement-breakpoint` is a valid SQL line comment, so the mangled file stays runnable by whichever tool actually owns it. Gate on meta/_journal.json, which drizzle-kit writes on first generate and maintains after, and which none of the others produce. A directory holding .sql files but no journal is reported rather than silently passed over — a genuine drizzle output whose meta/ went missing must not just look clean. Both CLI call sites are unaffected; they pass one explicit --out and never sweep. The multi-directory tests added by the previous commit keep their intent — their fixtures are now journal-bearing, so shrinking DRIZZLE_OUT_DIRS or short-circuiting the aggregation loop still fails them — and a new pair covers the foreign-directory case at both the sweep and command layers. Also adds the afterEach that post-agent.test.ts never had: all five tests in that describe leaked a temp tree per run.
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
1 participant
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.
What
Inverts the Drizzle migration sweep's default from fail-open to fail-closed.
stash eql migration --drizzleand the wizard's post-agent step scan a project's migration directories for in-placeALTER TABLE … ALTER COLUMN … SET DATA TYPE <encrypted domain>statements — Postgres has no cast fromtext/numericto an EQL domain, so drizzle-kit's output fails at migrate time — and rewrite each into ADD COLUMN + DROP COLUMN + RENAME. That sequence is equivalent to DROP+ADD: it fixes the type but destroys the column's contents, and is safe only on an empty table.The sweep decided what to rewrite from a corpus-wide index of columns the migrations give an encrypted type, skipping those and rewriting everything else — including a column the corpus never mentions. Absence from that index is not evidence of plaintext; it is evidence of nothing.
Two triggers, both reproduced:
DROP COLUMN.drizzle/, ALTER inmigrations/→ rewritten. This is not contrived:post-agent.tsships with['drizzle', 'migrations', 'src/db/migrations']and indexes each directory separately, so scanning several with a per-directory index is the default configuration. In the repro the dropped column is already typedeql_v3_text_eq— ciphertext, with nothing anywhere to backfill from.How
indexEncryptedColumnsbecomesindexColumnDeclarations, one traversal returning{ encrypted, declared }. The rewrite callback now checksencryptedfirst (skipalready-encrypted), thendeclared(skip the newsource-unknown), and rewrites only what survives both.Because the encrypted side already matches against the known domain list, the plaintext side needs no type classification and no SQL parsing — a column the corpus declares but does not give an encrypted type is plaintext by residue.
declaredis a name match scoped to two positions the code already isolates:CREATE TABLEbodies andADD COLUMN. Scoping matters — a whole-file scan would letALTER COLUMN "email" SET DATA TYPE …match its own"email" SETand declare the very column it is asking about.Two deliberate asymmetries, both load-bearing and both pinned by tests:
DECLARED_COLUMN_REcarries a word-boundary-pinned negative lookahead over SQL keywords, so a mention does not read as a declaration. Without it,CHECK ("email" IS NOT NULL)declaresemailand the destructive rewrite fires.Applied to both copies of the sweep —
packages/cli/src/commands/db/rewrite-migrations.tsandpackages/wizard/src/lib/rewrite-migrations.ts— which the wizard copy's own docstring mandates keeping in sync. Verified with a diff: no fail-closed identifier appears in the delta between them.Also fixed en route: a block-comment-prefixed statement was reported twice with contradictory reasons (the correct one, plus
unrecognised-form, whose guidance points at a hand-authoredUSINGclause that is not there), and the wizard'sRun the migration now?prompt warned about data destruction for a sweep that rewrote nothing and merely flagged.Behaviour change, not only a bug fix
A correct corpus whose swept directory does not contain the column's declaration — squashed history, a
drizzle-kit pullbaseline, a schema bootstrapped from hand-written SQL — is now flagged for review instead of repaired. That is the trade: a false skip costs a statement the user fixes by hand, a false rewrite costs irrecoverable data.The guarantee is scoped honestly in the changeset and the skill: the sweep reasons from the migration files, not the database. A database that has drifted from its migration history — after
stash encrypt cutover, which renames in-database and writes no drizzle SQL, or after a psql/dashboard change — is outside what it can see.describeSkipReason('source-unknown')names both causes and tells the user to check the column's real type before acting.Test plan
pnpm --filter stash test— 871 passedpnpm --filter @cipherstash/wizard test— 305 passed, 5 skippedpnpm --filter stash test:e2e— 76 passedpnpm run code:check— 0 errorsNew tests cover: the undeclared column, declaration by
ADD COLUMN, a declaration in theoptions.skipfile,RENAMEpropagating declared-ness, mentions insidePRIMARY KEY/REFERENCES/CHECKpredicates, a same-named constraint, types whose names start with a blocked keyword (interval,inet), a column commented out inside a liveCREATE TABLE, thealready-encrypted-before-source-unknownordering, and the cross-directory split throughsweepMigrationDirs.Existing tests that assert a rewrite were given the
CREATE TABLEdeclaration a real drizzle corpus carries. No assertion, fixture string, or test name was changed and no test was removed.Four review rounds found six defects the tests alone would not have, including two fail-open holes in the original design and two tests that passed while proving nothing. Each fix was confirmed by mutation — reverting it makes exactly the intended test fail.
isInsideCommentOrStringandendOfQuotedare untouched; the diff adds call sites only.Docs
skills/stash-cli/SKILL.mdandskills/stash-drizzle/SKILL.mdsaid every recognised statement is rewritten. Both now state the requirement and the scope of the guarantee. These ship to customers inside thestashtarball.Extends the existing
.changeset/rewriter-never-drops-ciphertext.mdrather than adding a second changeset describing overlapping behaviour in the same release.Part of #707