feat(cli): remove the EQL v2 migration leaf - #825
Conversation
🦋 Changeset detectedLatest commit: 487dc9b The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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:
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 |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approve
Reviewed the full non-SQL diff at 7d988c3f (7 commits on remove-v2), plus two focused audits over the whole PR tree — one hunting dangling references to every removed module/symbol, one verifying the installer/fail-closed/read-diagnostics claims hold together. No blocking defects; all 14 CI checks green. This is a clean, complete, correctly-classified (major) retirement of the EQL v2 migration leaf.
What I verified
The removal is complete and has no dangling edges. db push / db activate / encrypt cutover are gone from registry.ts, and push.ts / activate.ts / supabase-migration.ts / cutover.ts / resolve-proxy-choice.ts / migrate/src/eql.ts are deleted with zero surviving importers across packages/** and e2e/**. The migrate barrel no longer re-exports the eight v2 config-lifecycle primitives, and a positive guard test asserts their absence.
It fails closed everywhere, with defense in depth. Retired flags are rejected at the argv layer (rejectRetiredEqlFlags → validateInstallFlags) and again inside installCommand — both --flag value and --flag=value forms are covered, because parseArgs now splits on = first (closing the prior bypass where --eql-version=2 was mis-parsed as a boolean and silently ignored) and present() checks both flags[k]===true and Object.hasOwn(values,k). Removed subcommands hard-error with CliExit(1) before any requireStack. backfill throws BackfillConfigError on a non-v3 domain; drop now has an explicit if (!info) fail-closed branch — the one that matters, since info===null previously fell through to the now-deleted v2 <col>_plaintext / cut-over path. Every refusal points dump-recovery users at the upstream eql-2.3.1 release.
Read-only v2 diagnostics are genuinely retained — the fail-closed is scoped to mutation only. db status still reports v2 presence, quest.ts returns descriptive strings for legacy v2 rollout state (never throws), and migrate keeps EqlVersion = 2 | 3 plus the v2 manifest/IndexKind/cut-over schema so existing manifests stay parseable.
No ENOENT risk from the deleted SQL bundle. The installer now reads via readInstallSql() from @cipherstash/eql/sql; tsup.config.ts drops the cpSync('src/sql','dist/sql'); --eql-version 2 is intercepted by validateInstallFlags before any FS/DB I/O. eql upgrade is cleanly reduced to the pinned v3 bundle.
drop.ts is hardened, not just ported. buildV3DropSql now quotes identifiers through qualifyTable/quoteIdent (the old naive "${...}" interpolation is gone), and buildMigrationStem sanitizes untrusted identifiers for the generated filename — a net security improvement over the code it replaces.
Tests assert behaviour, not strings. The e2e suite checks exitCode === 1 for the spaced forms, a 9-case it.each of --flag=value forms, and the removed-subcommand routes; unit tests assert manifest absence + the validateInstallFlags recovery URL; the migrate test guards the removed exports. Changesets are correct (stash/@cipherstash/migrate major; skills refreshed).
Non-blocking nits (follow-up, not gating)
backfill.ts: theeqlVersion !== 3throw wording ("… not an EQL v3 domain. stash no longer backfills legacy EQL v2 columns …") also fires when the<col>_encryptedcolumn is simply absent (not v2), sincedetectColumnEqlVersionreturnsnullfor both. The remedy it gives ("migrate the schema to an eql_v3_* domain first") is still correct, so this is purely a message-precision nit.- Stale internal design docs:
docs/plans/encryption-migrations.mdanddocs/plans/cli-help-and-manifest.mdstill referencestash db push/encrypt cutoverand evenpackages/cli/src/commands/db/push.ts. These don't compile or ship to customers (per AGENTS.md), so nothing breaks — worth a cleanup pass whenever those plans are next touched.
Nice work — the fail-closed-for-mutation / stay-readable-for-diagnostics split is exactly right, and the parseArgs = fix + drop.ts quoting are real hardening beyond the stated scope. LGTM.
`detectColumnEqlVersion` returns null both for a column whose domain isn't `eql_v3_*` and for a column that isn't there at all, so backfill's single error accused a user who had merely not added `<col>_encrypted` yet of running legacy EQL v2 — and handed them a remedy (migrate the domain) for a column that does not exist. Extract the guard into `assertEqlV3Target` and probe `columnExists` on the failure path only; a v3 answer already proves existence, so the happy path still makes one catalog query. Also mark the retired surface in two internal design plans (`stash db push`, `stash encrypt cutover`, the `eql_v2.*` wrappers, `commands/db/push.ts`) so a future reader doesn't chase files that were deleted with EQL v2. The historical narrative is left intact.
2 participants
Summary
stash--flag=valueformsWhy
The command registry removed the EQL v2 mutation surface, but several callers and published instructions still invoked retired flags and commands. That could make integration setup fail, let obsolete flags silently select the v3 flow, or hand customers commands that immediately error.
This change removes the migration leaf consistently and fails closed for legacy mutation requests while retaining read-only compatibility diagnostics and dump-recovery guidance.
Validation
pnpm --filter stash test— 868 passedpnpm --filter @cipherstash/wizard test— 333 passed, 5 skippedpnpm --filter @cipherstash/stack-supabase test— 489 passedpnpm --filter @cipherstash/test-kit exec vitest run src/__tests__/install.test.ts— 3 passedpnpm exec vitest run --config scripts/vitest.config.mjs scripts/__tests__/no-removed-eql-version-flag.test.mjs— 28 passedpnpm --filter stash exec vitest run --config vitest.integration.config.ts tests/e2e/v2-retirement.e2e.test.ts— 18 passedstash,@cipherstash/wizard,@cipherstash/stack-supabase@cipherstash/test-kittypecheckpnpm run code:check(error-free; existing warnings remain)Stack
This PR is stacked on
remove-v2and intentionally targets that branch.