refactor(migrate): drop EQL v2 from the domain-type classifier#761
Conversation
- Note the intentional @cipherstash/nextjs patch (package.json description edit) - Use PCRE negative lookahead so the stale-reference check excludes protect-ffi
Remove the `eql_v2_encrypted → 2` branch from `classifyEqlDomain`, so the migrate domain-type resolution (`detectColumnEqlVersion`, `listEncryptedColumns`, `resolveEncryptedColumn`) recognises only the self-describing `eql_v3_*` domains. v3 is the sole generation this workspace authors and backfills; a legacy v2 column's version is carried by the manifest's recorded `eqlVersion` (the CLI status renderers already fall back to it), so v2 status output is unchanged. This drops v2 *classification*, not the v2 read path — existing v2 ciphertext stays decryptable via `@cipherstash/stack`. `EqlVersion` keeps its `2` member for manifest-sourced legacy values and the exported function signatures are unchanged. Tests in `version.test.ts` are updated to assert v2 domains are no longer classified (excluded from `listEncryptedColumns`, `null` from the detectors). Decision 6 guard: `classifyEqlDomain` is a source-column classifier for backfill planning and read-only CLI status display — no decrypt/round-trip consumes its `2` result — so dropping v2 here is safe. NOTE: `packages/migrate/src/eql.ts` (the `eql_v2.*` config-lifecycle wrappers) is NOT deleted in this PR. Although it carries no decrypt path, it is hard- imported by the CLI's v2 cut-over/config commands (`encrypt cutover`, `db activate`, `db push`); deleting it would break the CLI build, which is out of this migrate-scoped PR. Its removal must be sequenced with the CLI v2-command removal. PR 2 of the EQL v2 final removal (#707).
🦋 Changeset detectedLatest commit: b3a3499 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:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
27a0915 to
994e5b2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.changeset/remove-eql-v2-migrate-classifier.md:
- Around line 5-12: Update the release-note wording around classifyEqlDomain so
“solely” qualifies only determination from self-describing eql_v3_* domain
types, while explicitly preserving manifest-recorded eqlVersion as the source
for legacy v2 columns.
🪄 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
Run ID: 1a7445f7-65a4-4093-acf1-eca44939995a
📒 Files selected for processing (3)
.changeset/remove-eql-v2-migrate-classifier.mdpackages/migrate/src/__tests__/version.test.tspackages/migrate/src/version.ts
| Drop EQL v2 from the domain-type classifier. `classifyEqlDomain` (and the | ||
| `detectColumnEqlVersion` / `listEncryptedColumns` / `resolveEncryptedColumn` | ||
| resolution built on it) no longer recognise the legacy `eql_v2_encrypted` | ||
| domain — v3 is the sole generation this workspace authors and backfills, so a | ||
| column's version is now determined solely from its self-describing `eql_v3_*` | ||
| domain type. A legacy v2 column's version is carried by the manifest's recorded | ||
| `eqlVersion` instead (the CLI's `encrypt status` / `status` renderers already | ||
| fall back to it), so status output is unchanged for v2 columns. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that legacy v2 versions still come from the manifest.
Line 9 says a column’s version is determined solely from its eql_v3_* domain, but lines 10–12 correctly state that legacy v2 uses the manifest’s recorded eqlVersion. Reword the sentence so “solely” applies only to v3 domain classification; the current release note is internally contradictory.
🤖 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 @.changeset/remove-eql-v2-migrate-classifier.md around lines 5 - 12, Update
the release-note wording around classifyEqlDomain so “solely” qualifies only
determination from self-describing eql_v3_* domain types, while explicitly
preserving manifest-recorded eqlVersion as the source for legacy v2 columns.
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed by Claude Code on behalf of James Sadler.
Verdict: APPROVE
The core refactor is correct. Removing the eql_v2_encrypted → 2 arm from classifyEqlDomain leaves the v3 path (startsWith('eql_v3_')) and the null default untouched, so every v3 domain is still classified exactly as before and v2 domains now fall through to null — the intended change. EqlVersion correctly keeps its 2 member for manifest-sourced legacy values, so nothing that reads a recorded eqlVersion breaks type-wise. I traced every downstream consumer (detectColumnEqlVersion, listEncryptedColumns, resolveEncryptedColumn, and the CLI status/backfill/drop/cutover callers) and found no destructive misclassification: no v2 column is now treated as plaintext-and-dropped, because the v2 drop path is gated by the cut-over phase state machine (unchanged) and always targets <col>_plaintext, never a resolved ciphertext column.
Verification notes
- Status paths preserved.
packages/cli/src/commands/status/index.ts:143andpackages/cli/src/commands/encrypt/status.ts:161both resolve version asclassifyEqlDomain(domain) ?? <manifest eqlVersion>. For a v2 column the classifier now returnsnulland the manifest's recorded2supplies the value — so v2 status output is genuinely unchanged for existing manifests, as claimed. - Backfill lifecycle preserved. In
backfill.ts,buildManifestEntrysetstargetPhase: eqlVersion === 3 ? 'dropped' : 'cut-over'; a v2 column'snullstill yields'cut-over', so the v2 ladder is intact. - Drop path is safe.
drop.tsfalls through to the v2 branch (isV3 = info?.version === 3isfalse) and gates onrequiredPhase = 'cut-over'; the v3-only coverage check is correctly skipped andinfois not used to redirect what gets dropped. The documented "post-cutover v2 state falls through to the v2 path" (drop.ts:84-88) still holds because a table whose only encrypted column is now-unclassified v2 yields emptycandidates, andexplainUnresolvedreturnsnulloncandidates.length === 0. - Tests.
version.test.tsupdates are accurate — thepickEncryptedColumncase at line 159 now usescol('email', 'eql_v3_encrypted')(default version 3) and still validates the "never resolves the plaintext column to itself" invariant.
Non-blocking observations (follow-up, not required to merge)
- Dead branch left behind —
resolve-eql.ts:88.candidates.some((c) => c.column === column && c.version === 2)is now unreachable:listEncryptedColumnscan never emitversion: 2after this change. It's harmless (the single-column post-cutover case still short-circuits on thecandidates.length === 0check above it), but the branch and the surrounding v2 comments inresolve-eql.ts/drop.tsare now stale. Worth sweeping when the CLI v2 commands are removed — the same sequencing you already call out foreql.ts. - Manifest recording gap for new v2 backfills.
buildManifestEntryonly recordseqlVersionwhen truthy, anddetectColumnEqlVersionnow returnsnullfor a v2 column — so a v2 column backfilled after this PR would record noeqlVersion, and since its domain also classifies tonull, status would show a blank version. This only bites if v2 backfill is still exercised (the whole epic is removing v2 authoring, so likely moot), but the "readers fall back to the domain type in the database" comment at backfill.ts:563 is now slightly misleading for v2, since that fallback also yieldsnull. Existing manifests already carryeqlVersion: 2, so pre-existing v2 columns are unaffected. - Doc artifact.
docs/plans/2026-07-22-eql-v2-removal-pr1-plan.mdis a PR-1 plan landing in PR-2's diff — a stacking artifact, not a code concern.
None of the above changes correctness for the v3 generation this workspace authors, and the v2 read/decrypt path is untouched. Good, well-commented, well-tested change.
…change `classifyEqlDomain` now returns `3 | null`, so `listEncryptedColumns` can never emit `version: 2` and the `c.version === 2` exemption in `explainUnresolved` is dead. Removing it is provably behaviour-preserving: a post-cutover v2 table (`<col>` carrying the v2 domain) now reaches `explainUnresolved` with an EMPTY candidate list, which the `candidates.length === 0` guard above already falls through on. - Remove the branch; rewrite the doc comment and the stale v2 parentheticals in `drop.ts` / `cutover.ts` to say why the post-cutover state now arrives as "no EQL columns". - Rewrite the drop test that hand-built a `version: 2` candidate — a state resolution can no longer produce, so it only exercised the dead branch — to the state that actually occurs. - Add unit tests pinning `explainUnresolved`'s contract, including that a candidate sharing the plaintext column's name still fails closed (the removed branch's only behaviour, and wrong at v3, which has no cut-over rename). - Correct the backfill manifest comments: `null` now also means a legacy `eql_v2_encrypted` domain, so a v2 column backfilled from here on records no `eqlVersion` and reports no version in `encrypt status`. The live-domain fallback yields null for that case too. Existing manifests are unaffected. Noted in the changeset. - Drop the PR-1 plan doc that landed in this PR's diff.
2 participants
Stacked on #760 (PR 1). Base branch is
worktree-feat+remove-eql-v2-pr1-delete-packages; review that PR first.PR 2 of the EQL v2 final removal (#707).
What
Drops EQL v2 from the migrate domain-type classifier. The
eql_v2_encrypted → 2branch is removed fromclassifyEqlDomain, so the migrate resolution path (detectColumnEqlVersion,listEncryptedColumns,resolveEncryptedColumn) recognises only the self-describingeql_v3_*domains.Why it's safe
eqlVersion(the CLI status renderers already fall back to it), so v2 status output is unchanged.@cipherstash/stack.EqlVersionkeeps its2member for manifest-sourced legacy values; exported function signatures are unchanged.classifyEqlDomainis a source-column classifier for backfill planning and read-only CLI status — no decrypt/round-trip consumes its2result.Deliberately not in this PR
packages/migrate/src/eql.ts(theeql_v2.*config-lifecycle wrappers) is not deleted here. It carries no decrypt path, but it's hard-imported by the CLI's v2 cut-over/config commands (encrypt cutover,db activate,db push); deleting it would break the CLI build, which is out of this migrate-scoped PR. Its removal must be sequenced with the CLI v2-command removal.Tests
version.test.tsupdated to assert v2 domains are no longer classified (excluded fromlistEncryptedColumns,nullfrom the detectors).Changeset:
@cipherstash/migratepatch.Summary by CodeRabbit
Bug Fixes
Documentation