feat(migrate): EQL v3 support#649
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe migration library and ChangesEQL v3 encryption lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant EncryptCLI
participant MigrateLibrary
participant Postgres
Operator->>EncryptCLI: encrypt backfill
EncryptCLI->>MigrateLibrary: resolveColumnLifecycle
MigrateLibrary->>Postgres: inspect encrypted column domain
Postgres-->>MigrateLibrary: EQL version and column metadata
EncryptCLI->>MigrateLibrary: run version-aware backfill
MigrateLibrary->>Postgres: write encrypted payloads
Operator->>EncryptCLI: encrypt drop
EncryptCLI->>MigrateLibrary: countUnencrypted
MigrateLibrary->>Postgres: verify plaintext/encrypted coverage
EncryptCLI-->>Operator: generate version-specific drop migration
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
🦋 Changeset detectedLatest commit: 605c40a 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 |
coderdan
force-pushed
the
feat/migrate-eql-v3
branch
from
July 16, 2026 07:58
af92d80 to
3d81993
Compare
coderdan
added a commit
that referenced
this pull request
Jul 16, 2026
Completes #649. The backfill engine was already version-agnostic (it never touches the v2 config machine in eql.ts — that's CLI-called only), and isEncryptedPayload already accepts both v3 wire shapes, so the work lands where the v2 coupling actually lived: migrate: - countEncrypted(client, table, column) — the v3 verification primitive (v3 has no eql_v2.count_encrypted_with_active_config; a plain count of the populated domain column is the equivalent, the domain CHECK already guarantees validity). - Manifest gains an optional eqlVersion (2|3) field, recorded at backfill time, so status/plan branch without a DB round-trip. - backfill-v3.integration.test.ts pins the v3 contract against a real Postgres: flat-scalar and SteVec payloads through the leak guard, the $N::jsonb write, countEncrypted, idempotency. vitest fileParallelism disabled for the package — the two integration suites share the singleton cipherstash.cs_migrations schema and raced when parallel. cli (all auto-detected via detectColumnEqlVersion, no new flags): - encrypt backfill: detects the version, logs the lifecycle, records eqlVersion + the v3 target phase ('dropped' — no cut-over) in the manifest, and prints v3 next steps (switch-by-name, then drop). - encrypt cutover: v3 short-circuits with "not applicable" guidance (exit 0) before any v2 config-machine call. - encrypt drop: v3 runs from 'backfilled' and drops the ORIGINAL column (no <col>_plaintext exists under v3); v2 unchanged, both pinned by tests. - encrypt status: v2-only drift flags (not-registered, plaintext-col-missing) suppressed for v3 columns; EQL column shows 'v3'. - Fixed a pre-existing exit-code bug the new tests exposed: cutover/drop precondition guards `return` from inside `try`, so the trailing `if (exitCode) process.exit(...)` was unreachable — precondition failures exited 0. The exit now lives in `finally`. Verified live against postgres-eql + EQL v3 installed by this branch's CLI: real EncryptionV3 ciphertext backfilled into a concrete eql_v3_text domain column (CHECK satisfied), detectColumnEqlVersion → v3, countEncrypted exact, decrypt round-trip exact. Docs: migrate README rewritten around the two lifecycles; stash-cli and stash-encryption skills updated (the #648 v2-only notes are gone). Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
coderdan
marked this pull request as ready for review
July 16, 2026 08:24
coderdan
marked this pull request as draft
July 16, 2026 08:27
![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 `@packages/migrate/src/manifest.ts`:
- Around line 63-71: Update the `eqlVersion` field in `ManifestColumnSchema` to
validate the string literals `'v2'` and `'v3'`, matching the `EqlVersion` values
returned by `detectColumnEqlVersion`, while keeping the field optional for older
manifests.
🪄 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: ea91f446-d211-4211-bdd1-e4288e3d212d
📒 Files selected for processing (18)
.changeset/migrate-eql-v3.mdpackages/cli/src/bin/main.tspackages/cli/src/cli/registry.tspackages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.tspackages/cli/src/commands/encrypt/backfill.tspackages/cli/src/commands/encrypt/cutover.tspackages/cli/src/commands/encrypt/drop.tspackages/cli/src/commands/encrypt/status.tspackages/migrate/README.mdpackages/migrate/src/__tests__/backfill-v3.integration.test.tspackages/migrate/src/__tests__/version.test.tspackages/migrate/src/cursor.tspackages/migrate/src/index.tspackages/migrate/src/manifest.tspackages/migrate/src/version.tspackages/migrate/vitest.config.tsskills/stash-cli/SKILL.mdskills/stash-encryption/SKILL.md
Contributor
There was a problem hiding this comment.
Pull request overview
Adds EQL v3 support to the @cipherstash/migrate primitives and the stash encrypt * CLI lifecycle by auto-detecting a column’s EQL version from its Postgres domain type and routing version-specific lifecycle steps accordingly.
Changes:
- Added
detectColumnEqlVersion(domain-type based EQL v2/v3 detection) plus v3 verification primitivecountEncrypted, and recorded EQL version in the migration manifest. - Made
stash encrypt backfill/cutover/drop/statusversion-aware (v3: no cut-over; drop targets original plaintext column; status suppresses v2-only drift flags). - Updated shipped skills + migrate README + changeset to document the v2 vs v3 rollout lifecycles, and serialized migrate’s integration tests to avoid DB/schema races.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/stash-encryption/SKILL.md | Updates rollout guidance to describe both v2 and v3 lifecycles and v3 “switch-by-name” behavior. |
| skills/stash-cli/SKILL.md | Documents version-aware encrypt * behavior (v3: no cutover; drop semantics differ). |
| packages/migrate/vitest.config.ts | Serializes test files to avoid shared-DB schema/state races. |
| packages/migrate/src/version.ts | Introduces detectColumnEqlVersion for domain-based EQL v2/v3 detection. |
| packages/migrate/src/manifest.ts | Adds optional eqlVersion field to persisted manifest column entries. |
| packages/migrate/src/index.ts | Exports new helpers (countEncrypted, detectColumnEqlVersion, EqlVersion). |
| packages/migrate/src/cursor.ts | Adds countEncrypted as v3’s backfill verification primitive. |
| packages/migrate/src/tests/version.test.ts | Unit tests for detectColumnEqlVersion behavior. |
| packages/migrate/src/tests/backfill-v3.integration.test.ts | New v3 integration coverage for backfill leak-guard acceptance + countEncrypted + idempotency. |
| packages/migrate/README.md | Rewrites lifecycle documentation to clearly distinguish v2 vs v3. |
| packages/cli/src/commands/encrypt/status.ts | Renders v3 status correctly and suppresses v2-only drift flags for v3 columns (manifest-driven). |
| packages/cli/src/commands/encrypt/drop.ts | Makes drop version-aware (v3: drop original plaintext column; v2: drop <col>_plaintext) and fixes exit-code handling. |
| packages/cli/src/commands/encrypt/cutover.ts | Short-circuits on v3 with guidance (no-op) and fixes exit-code handling for guard failures. |
| packages/cli/src/commands/encrypt/backfill.ts | Detects and records EQL version at backfill time; sets version-appropriate target phase; prints v3 next steps. |
| packages/cli/src/commands/encrypt/tests/encrypt-v3.test.ts | Unit tests asserting v3/v2 branching for encrypt cutover and encrypt drop. |
| packages/cli/src/cli/registry.ts | Marks encrypt cutover as “EQL v2 only” in the registry summary. |
| packages/cli/src/bin/main.ts | Updates CLI help text for encrypt cutover to indicate v2-only behavior. |
| .changeset/migrate-eql-v3.md | Changeset documenting new v3 support and lifecycle behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
coderdan
marked this pull request as ready for review
July 16, 2026 11:27
First slice of EQL v3 support (#648): detectColumnEqlVersion() inspects an encrypted column's Postgres domain type — public.eql_v2_encrypted -> v2, a concrete eql_v3_* domain -> v3, anything else (plaintext / not found) -> null. This is the keystone the version-aware lifecycle branches on. Unit-tested with a mocked pg client; no behaviour change to existing v2 paths. Refs #648 Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Completes #649. The backfill engine was already version-agnostic (it never touches the v2 config machine in eql.ts — that's CLI-called only), and isEncryptedPayload already accepts both v3 wire shapes, so the work lands where the v2 coupling actually lived: migrate: - countEncrypted(client, table, column) — the v3 verification primitive (v3 has no eql_v2.count_encrypted_with_active_config; a plain count of the populated domain column is the equivalent, the domain CHECK already guarantees validity). - Manifest gains an optional eqlVersion (2|3) field, recorded at backfill time, so status/plan branch without a DB round-trip. - backfill-v3.integration.test.ts pins the v3 contract against a real Postgres: flat-scalar and SteVec payloads through the leak guard, the $N::jsonb write, countEncrypted, idempotency. vitest fileParallelism disabled for the package — the two integration suites share the singleton cipherstash.cs_migrations schema and raced when parallel. cli (all auto-detected via detectColumnEqlVersion, no new flags): - encrypt backfill: detects the version, logs the lifecycle, records eqlVersion + the v3 target phase ('dropped' — no cut-over) in the manifest, and prints v3 next steps (switch-by-name, then drop). - encrypt cutover: v3 short-circuits with "not applicable" guidance (exit 0) before any v2 config-machine call. - encrypt drop: v3 runs from 'backfilled' and drops the ORIGINAL column (no <col>_plaintext exists under v3); v2 unchanged, both pinned by tests. - encrypt status: v2-only drift flags (not-registered, plaintext-col-missing) suppressed for v3 columns; EQL column shows 'v3'. - Fixed a pre-existing exit-code bug the new tests exposed: cutover/drop precondition guards `return` from inside `try`, so the trailing `if (exitCode) process.exit(...)` was unreachable — precondition failures exited 0. The exit now lives in `finally`. Verified live against postgres-eql + EQL v3 installed by this branch's CLI: real EncryptionV3 ciphertext backfilled into a concrete eql_v3_text domain column (CHECK satisfied), detectColumnEqlVersion → v3, countEncrypted exact, decrypt round-trip exact. Docs: migrate README rewritten around the two lifecycles; stash-cli and stash-encryption skills updated (the #648 v2-only notes are gone). Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
coderdan
force-pushed
the
feat/migrate-eql-v3
branch
from
July 16, 2026 11:31
897372b to
17b46e7
Compare
…e drop Code-review round on #649. The EQL v3 types are self-describing — that was the point of v3 — so version and encrypted-column resolution now come from the Postgres domain types, with the <col>_encrypted naming demoted to an unenforced convention: - @cipherstash/migrate: classifyEqlDomain (the one home for the rule), listEncryptedColumns, resolveEncryptedColumn (hint → convention → sole-EQL-column). detectColumnEqlVersion resolves tables case-exactly (format('%I') before to_regclass) — a bare to_regclass case-folded Prisma-style "User" tables to 'user' and silently wedged v3 columns into the v2 lifecycle. EqlVersion is numeric (2|3), matching the manifest and installer; no more string↔number translation. - The manifest records encryptedColumn at backfill time; cutover and drop resolve manifest-hint-first instead of guessing the name, so a custom --encrypted-column no longer deadlocks the v3 lifecycle. - encrypt drop (v3) now verifies live coverage before generating the migration: refuses while any row has plaintext set and ciphertext NULL (countUnencrypted) — the verification the README promised but nothing performed. Dropping the original column is the irreversible step; the phase gate alone only proved a backfill once finished. - encrypt cutover (v3) is phase-aware: mid-backfill it exits 1 telling the user to finish the backfill, instead of exit-0 guidance to switch the app onto a half-populated column. The v2 path guards the eql_v2_configuration query so v3-only databases get an explanation, not a raw relation-does-not-exist error. - encrypt status classifies from the observed domain type (manifest as fallback) — a v3 column whose manifest entry predates detection no longer collects permanent false not-registered flags. stash status's quest ladder and the init agent handoff teach the v3 next step (4-rung ladder, switch-by-name) instead of steering v3 users into the no-op cutover. - Docs trued up: manifest docstrings, migrate README (drop's built-in coverage gate), skills' phase-ladder intros (both lifecycles), and the stale workflow comments. Unit + real-catalog integration coverage for the resolver (mixed-case tables, custom names, ambiguity); CLI tests pin the coverage gate, the phase-aware cutover, and resolved-name usage. 507 CLI + 49 migrate + 58 pty-e2e green. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@packages/cli/src/commands/encrypt/cutover.ts`:
- Around line 73-80: Update resolveColumnLifecycle handling in
packages/cli/src/commands/encrypt/cutover.ts at lines 73-80 to reject null or
otherwise unresolved/ambiguous info before entering the v2 configuration
transaction. In packages/cli/src/commands/encrypt/drop.ts at lines 70-80, reject
unresolved targets while explicitly allowing the valid post-cutover v2
same-name-domain case; preserve existing handling for resolved lifecycle states.
In `@packages/cli/src/commands/encrypt/drop.ts`:
- Around line 91-123: Update the generated dropSql migration around the existing
plaintextToDrop logic to lock the target table, revalidate that no rows have the
plaintext column set while encryptedColumn is NULL, and only then drop the
column within the same transaction. Ensure the migration aborts without dropping
anything when coverage validation fails, preserving the existing safety message
or equivalent.
In `@packages/cli/src/commands/encrypt/lib/resolve-eql.ts`:
- Around line 44-47: Update the manifest lookup around readManifest so it no
longer catches and converts every failure to null. Preserve readManifest’s
existing ENOENT-to-null behavior, while allowing malformed JSON, schema
validation, permission, and other I/O errors to propagate before accessing
manifest?.tables in the hint calculation.
In `@packages/migrate/README.md`:
- Line 11: Update the fenced code block in the migration README to include an
explicit language identifier, such as text, while preserving its existing
content.
In `@packages/migrate/src/__tests__/version.test.ts`:
- Around line 10-12: Replace unsafe unknown-based client casts with minimal
typed query-capable fixtures. In packages/migrate/src/__tests__/version.test.ts
lines 10-12, update mockClient to return a typed { query } client; apply the
same approach in createMockClient at
packages/migrate/src/__tests__/state.test.ts lines 18-40. In
packages/migrate/src/__tests__/backfill-v3.integration.test.ts lines 238-240,
248-250, 258-260, and 272-274, create and pass one small query-capable adapter
instead of casting pool at each call site.
In `@packages/migrate/src/version.ts`:
- Around line 159-166: Update the candidate resolution logic after the
`${plaintextColumn}_encrypted` lookup so it no longer selects an unrelated
column based solely on table-wide uniqueness. When no validated naming match or
manifest/hint identifies the encrypted counterpart, return null; preserve the
direct conventional match behavior.
In `@skills/stash-cli/SKILL.md`:
- Around line 467-473: Remove the blank line separating the v2 cutover guidance
from the “Known gap (v2)” paragraph in the blockquote. Keep both paragraphs
contiguous within the same blockquote so the Markdown content passes MD028.
🪄 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: 68b64239-3991-43d3-989d-a226793f5979
📒 Files selected for processing (24)
.changeset/migrate-eql-v3.mdpackages/cli/src/bin/main.tspackages/cli/src/cli/registry.tspackages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.tspackages/cli/src/commands/encrypt/backfill.tspackages/cli/src/commands/encrypt/cutover.tspackages/cli/src/commands/encrypt/drop.tspackages/cli/src/commands/encrypt/lib/db-readers.tspackages/cli/src/commands/encrypt/lib/resolve-eql.tspackages/cli/src/commands/encrypt/status.tspackages/cli/src/commands/init/lib/__tests__/setup-prompt.test.tspackages/cli/src/commands/init/lib/setup-prompt.tspackages/cli/src/commands/status/index.tspackages/cli/src/commands/status/quest.tspackages/migrate/README.mdpackages/migrate/src/__tests__/backfill-v3.integration.test.tspackages/migrate/src/__tests__/version.test.tspackages/migrate/src/cursor.tspackages/migrate/src/index.tspackages/migrate/src/manifest.tspackages/migrate/src/version.tspackages/migrate/vitest.config.tsskills/stash-cli/SKILL.mdskills/stash-encryption/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/cli/src/cli/registry.ts
- packages/migrate/src/cursor.ts
- packages/cli/src/bin/main.ts
- packages/migrate/vitest.config.ts
- .changeset/migrate-eql-v3.md
- packages/cli/src/commands/encrypt/tests/encrypt-v3.test.ts
- packages/cli/src/commands/encrypt/backfill.ts
Two review-thread fixes on #649: the v3 backfill integration table now types email_encrypted with a domain carrying the real eql_v3_* storage CHECK shape (both scalar and SteVec arms), so the $N::jsonb write exercises the implicit jsonb→domain cast + CHECK enforcement instead of plain jsonb; and classifyEqlDomain matches the 'eql_v3_' prefix with the underscore so a hypothetical eql_v30_* generation can't classify as v3 (negative pins added). Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
freshtonic
approved these changes
Jul 16, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Overview
Solid, well-motivated work. Makes @cipherstash/migrate and stash encrypt * version-aware for EQL v3:
- Domain types are the source of truth —
version.tsclassifies the EQL generation from the Postgres domain type (classifyEqlDomain/detectColumnEqlVersion/listEncryptedColumns/resolveEncryptedColumn); the<col>_encryptedname is a hint only, never relied upon. - v3 has no cut-over — lifecycle routing is version-aware (
backfill → switch-by-name → drop), landing CLI-side sincerunBackfillwas already version-agnostic. dropgains a live coverage gate for v3 (countUnencrypted) since dropping the original plaintext column is irreversible.- A genuine exit-code bug fix rides along (guards
returned from insidetry, making the trailingprocess.exitunreachable).
Reasoning is captured in comments, tests are thorough (unit + real-catalog integration + the live-crypto proof in the PR body), the changeset is present, and both affected skills + the migrate README are updated in-PR. I verified the exports the docs now reference (@cipherstash/stack/v3, EncryptionV3, encryptedSupabaseV3) all exist on the branch.
Correctness — strengths
- The exit-code fix is real and correctly done. Moving
if (exitCode) process.exit(exitCode)intofinallyfixes precondition failures that previously exited0; the success path (exitCode === 0) still avoids the exit. Well pinned by thespyExittests. classifyEqlDomainguards the prefix boundary (eql_v3_with the underscore), with an explicit test thateql_v30_text/eql_v3do not classify as v3.- Case-exact table resolution via
format('%I', …)beforeto_regclass— the fix for Prisma-style"User"tables being case-folded and misclassified as v2 — is the subtle correct choice, tested against both a mock (asserting SQL shape) and a real quoted-table catalog. - The v3 coverage gate counts the right thing (
plaintext IS NOT NULL AND encrypted IS NULL) against the resolved encrypted column name, and refuses before generating.
Issues & suggestions — all minor, none blocking
1. (Low, UX) Misleading cutover message on an already-migrated v3 column. cutover.ts:80 — the v3 branch treats every non-backfilled phase as "hasn't finished backfilling", so a column already in dropped gets told to "finish the backfill first". Consider special-casing terminal phases (dropped) or softening the wording for phases past backfilled.
2. (Low, efficiency) resolveColumnLifecycle can issue up to three identical listEncryptedColumns queries on the stale/absent-hint path (each a fresh catalog scan). Failure-path only and tables are small, so cosmetic — but you could resolve candidates once and derive info in-process.
3. (Low, consistency) drop/cutover decide the version from live resolution only, while status falls back to the manifest's cached eqlVersion. If the encrypted column is ambiguous/missing, resolveColumnLifecycle returns info: null and a v3 column silently takes the v2 path (drop errors Must be 'cut-over'; cutover hits the "no EQL v2 configuration table" guard). Defensible (these commands have a DB connection, so live truth is preferable, and the manifest encryptedColumn hint normally resolves it), but worth a one-line comment on the divergence. ResolvedLifecycle.candidates is plumbed through but not yet used to produce a friendlier "which column did you mean?" error — natural follow-up.
4. (Nit, advisory only) The coverage gate is inherently TOCTOU — counts, then writes a migration applied later; new plaintext-only rows can appear in between, and stale ciphertext (both set, plaintext edited post-backfill) is undetectable without decryption. Same limitations v2 has, already acknowledged in a comment. No action needed.
Conventions, tests, security
- Conventions: Result/exit-code contracts preserved, ESM
.jsspecifiers,EqlVersionkept numeric (2 | 3) to match the manifest zod schema and installer; manifest fields added as optional (backward-compatible with pre-v3 manifests). - Tests: Strong —
version.test.tscovers classification + all fourresolveEncryptedColumnbranches incl. ambiguity →null;encrypt-v3.test.tspins v3 and v2 (regression) cutover/drop, resolved-name-vs-convention, and the coverage gate; the integration suite exercises a real domain-typed column with a CHECK mirroringeql_v3_*. The newvitest.config.ts(fileParallelism: false) correctly prevents the two integration suites racing on the sharedcs_migrationsschema. - Security: No plaintext logging introduced; identifiers quoted (
quoteIdent/format('%I')), values parameterised or routed through existing quoting helpers; the irreversible v3 drop is gated behind both a phase check and a live coverage count.
Verdict
Approve. Correct, well-tested, and unusually well-documented in-code. The four items are low-severity polish. Worth a quick look at #1 (one-line message tweak for terminal phases); consider wiring ResolvedLifecycle.candidates into a friendlier ambiguity error in a follow-up.
tobyhede
approved these changes
Jul 16, 2026
Review follow-ups from Toby, James, and CodeRabbit: - Resolution provenance: pickEncryptedColumn (new pure core of resolveEncryptedColumn) tags every match `via: hint | convention | sole`. A sole-EQL-column match only proves uniqueness, not the plaintext<->ciphertext pairing, so `encrypt drop` — the one irreversible step — now refuses it with instructions instead of gating coverage on a possibly unrelated column. Display paths keep using it, so convention-free resolution still works everywhere else. - Fail closed on ambiguity: cutover and drop error listing the candidate EQL columns when none is identifiable, instead of falling through to the v2 machinery; the post-cutover v2 same-name state is recognised and still falls through to the v2 preconditions. - The generated v3 drop migration re-verifies coverage at APPLY time: LOCK TABLE, re-count plaintext-only rows, RAISE EXCEPTION without dropping if any appeared since generation; the DROP runs via EXECUTE in the same DO block so check-and-drop stay atomic under non-transactional runners. - resolveColumnLifecycle no longer swallows manifest read failures (ENOENT stays null via readManifest; malformed JSON/schema/permission errors propagate) and fetches the catalog once instead of up to three times. - cutover on an already-dropped v3 column says "lifecycle complete" (exit 0) instead of "finish the backfill". - stash status derives eqlVersion from the encrypted column's domain type (manifest encryptedColumn hint over the naming convention), falling back to the manifest only when the DB isn't visible — matching encrypt status. - Stale drop.ts JSDoc rewritten version-aware; migrate README fence language (MD040); stash-cli skill blockquote joined (MD028). Test-cast note: the `as unknown as ClientBase` factories in migrate tests stay (ClientBase's overloaded query signature can't be met by a structural fixture) but most resolution tests now target the pure pickEncryptedColumn and need no client at all. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Contributor
Author
|
Review follow-ups landed in 605c40a — thanks Toby, James (@freshtonic), and CodeRabbit. All nine inline threads are resolved with replies; James's four review-body items are also in:
One judgement call worth eyes: CodeRabbit flagged that the sole-EQL-column fallback can pair unrelated columns (resolve Also: manifest read failures no longer downgrade to "no hint" on the destructive paths (ENOENT still fine, malformed/permission errors propagate), |
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
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.
Makes
@cipherstash/migrate(and thestash encrypt *CLI it backs) work with EQL v3 columns. Closes #648.Design (from #648, sharpened in review)
encrypt drop. No v3 rename step.classifyEqlDomain/resolveEncryptedColumn/listEncryptedColumns). The<col>_encryptednaming is a convention only — neither enforced nor relied upon: an explicit name (manifest-recorded--encrypted-column) wins, then the convention is validated against the domain, then a table's sole EQL column resolves with no convention at all. No new flags.What landed
Version + column resolution (
@cipherstash/migrate):classifyEqlDomain(the one home for the domain→version rule;eql_v3_prefix, underscore included),detectColumnEqlVersion,listEncryptedColumns,resolveEncryptedColumn. Table resolution is case-exact (format('%I')beforeto_regclass) — a bareto_regclasscase-folded Prisma-style"User"tables and silently misclassified v3 columns as v2.EqlVersionis numeric (2 | 3), matching the manifest and installer.encryptedColumnat backfill time (including any--encrypted-columnoverride);cutover/dropresolve manifest-hint-first, so custom-named columns can't deadlock the v3 lifecycle.countEncrypted/countUnencryptedexported as coverage primitives.runBackfillwas already version-agnostic — the v2 config machine lives ineql.tsand is only called by the CLI, so lifecycle routing lands CLI-side.Version-aware CLI (auto-detected, no flags):
encrypt backfill: logs the detected lifecycle, recordseqlVersion+encryptedColumn+ the v3 target phase (dropped) in.cipherstash/migrations.json, prints v3 next steps.encrypt cutover: phase-aware v3 short-circuit — backfilled → "not applicable" + the actual next step, exit 0; mid-backfill → exit 1 with "finish the backfill" (never switch-now guidance onto a half-populated column). The v2 path guards theeql_v2_configurationquery, so v3-only databases get an explanation instead of a raw relation-does-not-exist error.encrypt drop: v3 runs frombackfilled, verifies live coverage before generating — refuses (with count + re-backfill guidance) while any row has plaintext set and the encrypted column NULL. Dropping the original column is the irreversible v3 step; the phase gate alone only proves a backfill once finished. v2 unchanged.encrypt status: classifies from the observed domain type (manifest as fallback) — no permanent falsenot-registeredflags when the manifest predates detection; showsv3.stash statusquest ladder + thestash initagent handoff teach the version-appropriate ladder (4 rungs for v3, switch-by-name) instead of steering v3 users into the no-op cutover.returned from insidetry, making the trailingprocess.exitunreachable) — exit now lives infinally. Note this also corrects v2 behaviour to the documented contract.Verification:
postgres-eqlwith EQL v3 installed by this branch's CLI): realEncryptionV3ciphertext backfilled into a concreteeql_v3_textdomain column — domain CHECK satisfied — detection → v3,countEncryptedexact (25/25), decrypt round-trip exact.PG_TEST_URLharness, no credentials): flat + SteVec payloads through the leak guard and a domain-typed target column (implicit jsonb→domain cast + CHECK enforcement),countEncrypted, idempotency, and the resolver against a real catalog — mixed-case quoted tables and name-free resolution included.encrypt-v3suite pins the coverage gate, phase-aware cutover, resolved-name usage, and both versions' branches) + 58 pty e2e.Docs: migrate README rewritten around the two lifecycles and the domain-resolution primitives;
stash-cli+stash-encryptionskills document the dual ladder (v2: backfill → cutover → drop; v3: backfill → switch-by-name → drop) and drop's coverage gate; changeset:@cipherstash/migrate+stashminor.https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Summary by CodeRabbit
encrypt backfillnow detects lifecycle versions and provides version-specific next steps.encrypt dropsafely removes plaintext columns with coverage checks before and during migration.