test(cli,wizard): A-2 sweep — encrypted-domain residue fix + integration coverage#785
Open
tobyhede wants to merge 8 commits into
Open
test(cli,wizard): A-2 sweep — encrypted-domain residue fix + integration coverage#785tobyhede wants to merge 8 commits into
tobyhede wants to merge 8 commits into
Conversation
|
| 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 |
…crypted
The corpus index recognised an encrypted column only in the mangled forms
drizzle-kit emits and the literal `public` schema. A domain installed into
another schema ("app"."eql_v3_text_search") or an array of a domain
(public.eql_v3_text_search[]) fell to the plaintext residue, so an ALTER on
such a column was rewritten into ADD+DROP+RENAME — dropping ciphertext the
corpus itself showed was encrypted.
ENCRYPTED_TYPE_REF now admits any quoted schema and a trailing '[', closing
both. It feeds only the encrypted index, never the rewrite matcher, so the
worst case is a flagged statement, not a new rewrite. Applied to both copies
of the sweep; changeset guarantee updated to match.
describeSkipReason had zero coverage repo-wide — replacing its body with a constant left every suite green. Its output is the guidance a user acts on when a statement is skipped, and source-unknown's is new. Pin each reason to a distinctive substring so a mis-mapped case is caught.
The sweep's unit tests are thorough; everything above them was thin. - The clack log mock in migration.test.ts and generate-drizzle-migration.test.ts omitted `step`, so the per-statement report threw and landed in the command's catch — no test ever saw the report block. Add `step`. - eql migration: assert source-unknown guidance reaches the user and the closing warning fires for an undeclared column. - db install (generateDrizzleMigration): its step-5 sweep block had zero tests — every other test's out dir held only the skipped generated file. Add a genuine rewrite and a source-unknown skip. - wizard post-agent: harden the clean default-to-Yes arm to assert it carries no warning phrase, and add two multi-directory tests that put the actionable migration in a non-first candidate, so shrinking DRIZZLE_OUT_DIRS or short-circuiting the aggregation loop fails.
tobyhede
force-pushed
the
fix/a2-coverage-followup
branch
from
July 25, 2026 01:43
0508abc to
301f7e0
Compare
…icit schema Two ways a corpus could still lose ciphertext to the ADD+DROP+RENAME, both found by the #772 review. Neither is caught by the fail-closed `declared` rule, because in both the column really is declared — as plaintext, by the original CREATE TABLE. Finding 3 — chained conversions. `indexColumnDeclarations` reads CREATE TABLE, ADD COLUMN and RENAME, but never the strict matcher's own target. A corpus carrying an earlier `SET DATA TYPE eql_v2_encrypted` (a stack version that predates this sweep) left the column looking plaintext, so a later domain change dropped its ciphertext. Record the conversion as the sweep walks, in file-sorted order — not in the corpus-wide index, which has no order and would flag the legitimate first conversion too. Finding 4 — `"users"` and `"public"."users"` are the same table; Postgres resolves the unqualified name through `search_path`. drizzle-kit emits unqualified, while hand-written SQL and this sweep's own renderSafeAlter output are qualified, so a corpus mixing the two split one column across two keys: the already-encrypted guard missed, and the ALTER dropped ciphertext. Collapse the implicit schema in `columnKey`. Only that one — `"app"."users"` stays distinct, and there is no case folding, since TABLE_REF matches quoted identifiers only. Both fixes land in both copies of the rewriter. 7 tests each: the two destructive cases, the wrong-reason case finding 4 also produced (`source-unknown` for a column declared two files up), and four guards for the ways a naive fix breaks — the first conversion in a chain, a chain across different columns, a commented-out earlier ALTER, and a real non-public schema.
…ning for comments Quote parity is a whole-file property. Anything that makes the scanner disagree with Postgres about where a literal ENDS shifts every token after it — so a commented-out ALTER downstream reads as live and is rewritten into a live DROP COLUMN. `isInsideCommentOrString` left two such gaps. A `$$ … $$` body was untracked on the stated grounds that mis-reading one can only make us SKIP a rewrite; that holds for an unterminated literal but not for an over-terminated one, and an odd apostrophe count inside the body (`$$ don't $$` — a function body, a comment, any prose) ends a literal EARLY. And `endOfQuoted` knew only the doubled-delimiter escape, so `E'a\'b'` read as closing at the escaped quote. Skip dollar-quoted bodies whole, and give `endOfQuoted` a backslashEscapes mode selected when the opening quote is an `E''` prefix (guarded so an identifier merely ending in `e` does not trigger it). Both copies. Four tests each. The two destructive shapes, a tagged `$fn$` variant, and a guard that a live ALTER *following* a dollar-quoted body is still rewritten — that one failed before this change too, in the over-skip direction the old docblock had assumed was the only risk.
…ssing it CREATE_TABLE_RE carried a lazy `([\s\S]*?)\)\s*;` body, so it stopped at the first `);` anywhere in the file. That can sit inside a `--` comment or a string DEFAULT — `"id" text, -- pk (uuid);` is legal and unremarkable — and the body was then truncated at that point. Every column declared after it disappeared from BOTH indexes. For an encrypted column that means the already-encrypted guard never sees it. On its own that yields a wrong reason (`source-unknown` for a column declared two lines up). Composed with a `declared` record from elsewhere in the corpus — a table dropped and recreated in a later migration — the fail-closed rule is satisfied too, and the ADD+DROP+RENAME drops a column full of ciphertext. Match only `CREATE TABLE … (` and locate the closing `)` by scanning candidate `)\s*;` positions for the first one `isInsideCommentOrString` says is live. Nested parens (`numeric(10,2)`, `CHECK (x > 0)`) are not followed by `;`, so requiring the terminator still excludes them. An unclosed statement is skipped rather than indexed to end-of-file. Three tests each: the comment truncation, the DEFAULT-string truncation, and the composed case that actually destroys data.
…ls CI The rewriter lives twice — packages/wizard/src/lib and packages/cli/src/commands/db — at ~99% similarity, and neither package depends on the other. Four #772 review findings each needed the identical fix in both places; a fix landing in one still passes that package's own suite, and this code emits DROP COLUMN. Extraction is not available this cycle: packages/utils has no package.json, @cipherstash/test-kit is private and build-less, and both bin entries set skipNodeModulesBundle, so a shared workspace package would survive into dist as an unresolvable bare specifier. That leaves publishing a new package into a fixed mid-RC release group or adding noExternal to two CLI bundles — both worse than the drift they would prevent, for now. So compare instead. Everything outside an explicit `#region wizard-only` marker must match byte-for-byte modulo comments, imports and the tool name in the emitted header. Verified the guard actually fires: perturbing either copy fails with a message naming both files and pointing at the region marker.
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.
freshtonic
approved these changes
Jul 25, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Approve.
The encrypted-domain residue fix is correct and provably safe: the widening is confined to the detection index (ENCRYPTED_TYPE_REF) and never touches the rewrite matcher (ALTER_COLUMN_TO_ENCRYPTED_RE still uses MANGLED_TYPE_FORMS), so the worst case is a column being flagged "already encrypted" rather than rewritten — the safe direction for a tool that emits DROP COLUMN on ciphertext. Both new correctness tests fail without their production fix; the tests write real .sql files and invoke the real sweep functions (no assertions on mock behavior), and the clack.log.step mock addition fixes a real incomplete-mock blindness. No test-only methods added to production. Changeset satisfied (the existing rewriter-never-drops-ciphertext.md is strengthened).
Non-blocking: the two rewriter copies remain duplicated, held together only by the new sync test — honestly acknowledged and deferred to #786 with a sound rationale. The describeSkipReason substring assertions are brittle to copy edits (justified, since the wording is the data-loss-remediation contract). Nice mutation-testing discipline surfacing the blind spots.
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
2 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.
What
Follow-up to #783 (the A-2 fail-closed sweep, merged into
remove-v2). Two things:Correctness fix — non-public-schema and array EQL domains
The corpus index recognised a column as encrypted only in the mangled forms drizzle-kit emits and the literal
publicschema. Two shapes fell through to the plaintext residue and got rewritten into a data-destroying ADD+DROP+RENAME — aDROP COLUMNon ciphertext the corpus itself declared:publicschema —"email" "app"."eql_v3_text_search""email" public.eql_v3_text_search[](the(?=[\s,;)]|$)tail rejected[)ENCRYPTED_TYPE_REFnow admits any quoted schema and a trailing[:It feeds only the encrypted index, never the rewrite matcher (the ALTER matcher uses
MANGLED_TYPE_FORMSdirectly), so widening can only move a column from "rewritten" to "flagged as already-encrypted" — it can never cause a new rewrite. Applied to both copies of the sweep; the.changeset/rewriter-never-drops-ciphertext.mdguarantee is updated to match rather than weakened, because the gap is actually closed. Two TDD tests per copy, red before the fix.Coverage
Reviews (CLI command level, wizard integration, outer tiers) mutation-tested rather than eyeballed. The load-bearing findings, all closed here:
describeSkipReasonwas untested repo-wide — replacing its body with a constant left every suite green. It's the guidance a user acts on when a statement is skipped, andsource-unknown's text is new. Pinned each of the three reasons to a distinctive substring in both copies.logmock inmigration.test.tsandgenerate-drizzle-migration.test.tsomittedstep, which the real clack exports — so the per-statement report threw on its first iteration and was swallowed by the command's catch. Production output was fine; the tests were blind. Addedstep, which also strengthens the existing near-miss test (it no longer passes via the catch path).source-unknownend to end foreql migration --drizzle: the guidance reaches the user vialog.stepand the closing "did not fully complete" warning fires.db install's step-5 sweep block had zero tests — every other test's out dir held only the skipped generated file, so the block never executed. Added a genuine rewrite and asource-unknownskip againstgenerateDrizzleMigration.drizzle/. Added two tests that put the actionable migration in a non-first candidate directory, so shrinkingDRIZZLE_OUT_DIRSto['drizzle']or short-circuiting the aggregation loop now fails.Deferred (tracked in #786)
Not in this PR, with reasons:
DROP COLUMNsits on disk — a production behaviour change, not just a test, so it wants its own PR.drizzle-kit pullbaseline) at any tier — larger, andexamples/**has no drizzle migration history to build on.packages/cli/AGENTS.md's own triggers the sweep meets none, and it sits behind a project-localdrizzle-kit generatethat isn't a repo dependency. The recommendation is to first add a read-only--dry-runsweep seam, then one e2e case can assert the report renders legibly; that seam is its own change.Test plan
pnpm --filter stash test— 880 passedpnpm --filter @cipherstash/wizard test— 313 passed, 5 skippedpnpm run code:check— 0 errorsPart of #707