feat(migrate): EQL v3 support#649
Conversation
📝 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: 8153419 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
af92d80 to
3d81993
Compare
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
![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
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.
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
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
| const { info } = await resolveColumnLifecycle( | ||
| client, | ||
| options.table, | ||
| options.column, | ||
| ) | ||
| const state = await progress(client, options.table, options.column) | ||
|
|
||
| if (info?.version === 3) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail closed when EQL lifecycle resolution returns no match. Both commands currently interpret info === null as v2, even though it also represents ambiguity or a missing encrypted column.
packages/cli/src/commands/encrypt/cutover.ts#L73-L80: reject unresolved or ambiguous targets before entering the v2 configuration transaction.packages/cli/src/commands/encrypt/drop.ts#L70-L80: reject unresolved targets and explicitly recognize the valid post-cutover v2 same-name-domain case.
📍 Affects 2 files
packages/cli/src/commands/encrypt/cutover.ts#L73-L80(this comment)packages/cli/src/commands/encrypt/drop.ts#L70-L80
🤖 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 `@packages/cli/src/commands/encrypt/cutover.ts` around lines 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.
| if (isV3) { | ||
| // The phase gate above proves a backfill FINISHED at some point; it | ||
| // says nothing about rows written since (a bulk import or a service | ||
| // that isn't dual-writing leaves plaintext-only rows). Dropping the | ||
| // original column is the one irreversible step in the v3 ladder, so | ||
| // verify live coverage before generating the migration. | ||
| const unencrypted = await countUnencrypted( | ||
| client, | ||
| options.table, | ||
| options.column, | ||
| encryptedColumn, | ||
| ) | ||
| if (unencrypted > 0) { | ||
| p.log.error( | ||
| `Refusing to generate the drop migration: ${unencrypted} row(s) in ${options.table} have "${options.column}" set but "${encryptedColumn}" NULL — dropping "${options.column}" would permanently destroy that data. Likely rows written without dual-writes since the backfill. Re-run:\n stash encrypt backfill --table ${options.table} --column ${options.column}\nthen generate the drop again.`, | ||
| ) | ||
| exitCode = 1 | ||
| return | ||
| } | ||
| p.log.success( | ||
| `Verified: no rows with "${options.column}" set and "${encryptedColumn}" NULL.`, | ||
| ) | ||
| p.log.info( | ||
| `${options.table}.${encryptedColumn} is EQL v3 (${info?.domain}) — the drop targets the original plaintext column "${options.column}" (v3 has no rename, so there is no "${options.column}_plaintext"). Make sure your application reads/writes ${encryptedColumn} before applying this migration.`, | ||
| ) | ||
| } | ||
|
|
||
| const dot = options.table.indexOf('.') | ||
| const qualifiedTable = | ||
| dot >= 0 | ||
| ? `"${options.table.slice(0, dot)}"."${options.table.slice(dot + 1)}"` | ||
| : `"${options.table}"` | ||
| const dropSql = `-- Generated by stash encrypt drop\n-- Drops the plaintext column now that ${options.table}.${options.column} is encrypted.\n\nALTER TABLE ${qualifiedTable} DROP COLUMN "${options.column}_plaintext";\n` | ||
| const dropSql = `-- Generated by stash encrypt drop\n-- Drops the plaintext column now that ${options.table}.${isV3 ? encryptedColumn : options.column} is encrypted.\n\nALTER TABLE ${qualifiedTable} DROP COLUMN "${plaintextToDrop}";\n` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Revalidate ciphertext coverage inside the generated drop migration.
This check becomes stale immediately after generation. A later plaintext-only write can occur before migration application, after which the generated SQL drops the plaintext without rechecking. Make the migration lock the table, verify coverage, and perform the drop atomically.
🤖 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 `@packages/cli/src/commands/encrypt/drop.ts` around lines 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.
| const manifest = await readManifest().catch(() => null) | ||
| const hint = manifest?.tables[table]?.find( | ||
| (entry) => entry.column === column, | ||
| )?.encryptedColumn |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not silently discard manifest validation or I/O failures.
readManifest already returns null for ENOENT. This catch also hides malformed JSON, schema failures, and permission errors, allowing destructive commands to continue without the recorded column hint.
Proposed fix
- const manifest = await readManifest().catch(() => null)
+ const manifest = await readManifest()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const manifest = await readManifest().catch(() => null) | |
| const hint = manifest?.tables[table]?.find( | |
| (entry) => entry.column === column, | |
| )?.encryptedColumn | |
| const manifest = await readManifest() | |
| const hint = manifest?.tables[table]?.find( | |
| (entry) => entry.column === column, | |
| )?.encryptedColumn |
🤖 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 `@packages/cli/src/commands/encrypt/lib/resolve-eql.ts` around lines 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.
| Each column walks through these phases: | ||
| Each column walks through these phases — the ladder depends on the column's EQL version (auto-detected from its Postgres domain type via `detectColumnEqlVersion`): | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced-block language.
Add a language identifier such as text to satisfy the reported MD040 lint warning.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@packages/migrate/README.md` at 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.
Source: Linters/SAST tools
| function mockClient(rows: Array<Record<string, unknown>>) { | ||
| const query = vi.fn().mockResolvedValue({ rows }) | ||
| return { client: { query } as unknown as ClientBase, query } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== version.test.ts ==\n'
sed -n '1,120p' packages/migrate/src/__tests__/version.test.ts
printf '\n== backfill-v3.integration.test.ts (relevant slices) ==\n'
sed -n '220,290p' packages/migrate/src/__tests__/backfill-v3.integration.test.ts
printf '\n== search for query-client helpers/types ==\n'
rg -n "ClientBase|query.*mock|as unknown as pg\\.ClientBase|as unknown as ClientBase|Pool|Client" packages/migrate/src/__tests__ packages/migrate/src -g '!**/dist/**'Repository: cipherstash/stack
Length of output: 21970
🏁 Script executed:
set -euo pipefail
printf '\n== version.test.ts ==\n'
cat -n packages/migrate/src/__tests__/version.test.ts | sed -n '1,120p'
printf '\n== backfill-v3.integration.test.ts (relevant slices) ==\n'
cat -n packages/migrate/src/__tests__/backfill-v3.integration.test.ts | sed -n '220,290p'
printf '\n== search for query-client helpers/types ==\n'
rg -n "ClientBase|query.*mock|as unknown as pg\\.ClientBase|as unknown as ClientBase|Pool|Client" packages/migrate/src/__tests__ packages/migrate/src -g '!**/dist/**'Repository: cipherstash/stack
Length of output: 23244
🏁 Script executed:
set -euo pipefail
printf '\n== state.test.ts helper ==\n'
cat -n packages/migrate/src/__tests__/state.test.ts | sed -n '1,90p'
printf '\n== version.ts signatures ==\n'
cat -n packages/migrate/src/version.ts | sed -n '1,220p'Repository: cipherstash/stack
Length of output: 10720
Replace the as unknown as test client casts with typed fixtures.
packages/migrate/src/__tests__/version.test.ts#L10-L12: return a minimal typed{ query }client instead of casting throughunknown.packages/migrate/src/__tests__/state.test.ts#L18-L40: apply the same pattern increateMockClient.packages/migrate/src/__tests__/backfill-v3.integration.test.ts#L238-L274: pass a small query-capable adapter instead of castingpoolat each call site.
📍 Affects 2 files
packages/migrate/src/__tests__/version.test.ts#L10-L12(this comment)packages/migrate/src/__tests__/backfill-v3.integration.test.ts#L238-L240packages/migrate/src/__tests__/backfill-v3.integration.test.ts#L248-L250packages/migrate/src/__tests__/backfill-v3.integration.test.ts#L258-L260packages/migrate/src/__tests__/backfill-v3.integration.test.ts#L272-L274
🤖 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 `@packages/migrate/src/__tests__/version.test.ts` around lines 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.
Source: Coding guidelines
| const conventional = candidates.find( | ||
| (c) => c.column === `${plaintextColumn}_encrypted`, | ||
| ) | ||
| if (conventional) return conventional | ||
|
|
||
| // The plaintext column itself can't be its own encrypted counterpart. | ||
| const others = candidates.filter((c) => c.column !== plaintextColumn) | ||
| return others.length === 1 ? (others[0] ?? null) : null |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not infer a plaintext relationship from table-wide uniqueness.
A sole EQL column may encrypt an unrelated field. Resolving email to the only EQL column, such as ssn_cipher, lets dropCommand validate the wrong ciphertext and generate a migration that destroys email. Require a manifest/hint or validated naming match; otherwise return null.
Proposed safe fallback
- // The plaintext column itself can't be its own encrypted counterpart.
- const others = candidates.filter((c) => c.column !== plaintextColumn)
- return others.length === 1 ? (others[0] ?? null) : null
+ return null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const conventional = candidates.find( | |
| (c) => c.column === `${plaintextColumn}_encrypted`, | |
| ) | |
| if (conventional) return conventional | |
| // The plaintext column itself can't be its own encrypted counterpart. | |
| const others = candidates.filter((c) => c.column !== plaintextColumn) | |
| return others.length === 1 ? (others[0] ?? null) : null | |
| const conventional = candidates.find( | |
| (c) => c.column === `${plaintextColumn}_encrypted`, | |
| ) | |
| if (conventional) return conventional | |
| return null |
🤖 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 `@packages/migrate/src/version.ts` around lines 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.
| **EQL v2 only** — v3 has no cut-over: the application switches to `<col>_encrypted` by name, and running this command on a v3 column reports "not applicable" (exit 0) with the next step. For v2, the preconditions are: the column is in the `backfilled` phase, **and** a pending EQL configuration exists. | ||
|
|
||
| In one transaction it renames `<col>` → `<col>_plaintext` and `<col>_encrypted` → `<col>`, advances the pending config to `encrypting`, activates it, and appends a `cut_over` event. With a Proxy URL configured (`--proxy-url` or `CIPHERSTASH_PROXY_URL`) it then calls `eql_v2.reload_config()` so Proxy picks up the new shape. | ||
|
|
||
| > **After cutover, `<col>` holds ciphertext — the read path is not automatic.** Wire reads through the encryption client (`decryptModel(row, table)` for Drizzle, the `encryptedSupabaseV3` wrapper for Supabase — `encryptedSupabase` on the legacy v2 surface, otherwise `decrypt` / `bulkDecryptModels`) before returning values to callers. Skip this and your read paths hand raw EQL payloads to end users. The integration skill has the exact API. **CipherStash Proxy is the one exception** — it decrypts on the wire, so Proxy users need no application change. The cutover plan written by `stash plan` includes this read-path switch as an explicit step. | ||
|
|
||
| > **Known gap.** The pending-configuration precondition is satisfied by `stash db push`. SDK-only users (who otherwise never need `db push`) must therefore run it once before `encrypt cutover`. Decoupling this — under EQL v3 there is no configuration table at all — is tracked in [issue #585](https://github.com/cipherstash/stack/issues/585). | ||
| > **Known gap (v2).** The pending-configuration precondition is satisfied by `stash db push`. SDK-only users (who otherwise never need `db push`) must therefore run it once before `encrypt cutover`. (EQL v3 columns sidestep this entirely — no configuration table, no cutover; see above.) Tracked in [issue #585](https://github.com/cipherstash/stack/issues/585). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the blank line inside the blockquote.
The blank line between the v2 cutover guidance and the “Known gap” paragraph triggers the reported MD028 Markdown lint warning. Keep the related blockquote content contiguous or close and reopen the quote intentionally.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 472-472: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🪛 SkillSpector (2.3.11)
[warning] 35: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 36: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 43: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 44: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 45: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 51: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 52: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 64: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 65: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 73: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 163: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 165: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 166: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 328: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 347: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 560: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 293: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 12: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
🤖 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 `@skills/stash-cli/SKILL.md` around lines 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.
Source: Linters/SAST tools
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
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