fix(cli,migrate): harden EQL column resolution per #649 review

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

Author: coderdan

.changeset/migrate-eql-v3.md

@@ -26,19 +26,29 @@ right lifecycle, no new flags:
   **verifies live coverage** (refuses to generate the migration while any row
   still has the plaintext set and the encrypted column NULL — the
   `countUnencrypted` check), and drops the ORIGINAL plaintext column (there
-  is no `<col>_plaintext` under v3); v2 behaviour is unchanged.
+  is no `<col>_plaintext` under v3); v2 behaviour is unchanged. The generated
+  v3 migration **re-verifies coverage at apply time** — it locks the table,
+  re-counts, and aborts without dropping if plaintext-only rows appeared
+  after generation. And because dropping is the one irreversible step, it
+  requires a positively asserted plaintext↔ciphertext pairing (the
+  manifest's recorded `encryptedColumn` or the naming convention): a match
+  found only by being the table's sole EQL column is refused with
+  instructions, and an ambiguous table (several EQL columns, none
+  identifiable) fails closed listing the candidates — as does `cutover`.
 - **`encrypt status`** classifies each column from the observed domain type
   (manifest as fallback), shows `v3` in the EQL column, and no longer raises
   the v2-only `not-registered` / `plaintext-col-missing` drift flags for v3
   columns. `stash status`'s quest ladder and the `stash init` agent handoff
   prompt teach the version-appropriate next step (no more "run cutover" on
   v3 columns).
 - New `@cipherstash/migrate` exports: `classifyEqlDomain`,
-  `resolveEncryptedColumn`, `listEncryptedColumns` (domain-type resolution —
-  case-exact for quoted/mixed-case table names), `countEncrypted` /
-  `countUnencrypted` (coverage counts), and manifest `eqlVersion` +
-  `encryptedColumn` fields. `EqlVersion` is numeric (`2 | 3`), matching the
-  manifest and the installer.
+  `resolveEncryptedColumn`, `pickEncryptedColumn`, `listEncryptedColumns`
+  (domain-type resolution — case-exact for quoted/mixed-case table names),
+  `countEncrypted` / `countUnencrypted` (coverage counts), and manifest
+  `eqlVersion` + `encryptedColumn` fields. `EqlVersion` is numeric (`2 | 3`),
+  matching the manifest and the installer. Resolved columns carry `via:
+  'hint' | 'convention' | 'sole'` so callers can tell a positively asserted
+  pairing from a by-elimination guess.
 - Fixed: `encrypt cutover`/`encrypt drop` precondition failures now actually
   exit 1 — the early-return guards previously skipped the exit-code path
   entirely, so failed preconditions exited 0. (This also applies to v2

packages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.ts

@@ -5,9 +5,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
 // v2 config machine, and `encrypt drop` must target the ORIGINAL plaintext
 // column (there is no `<col>_plaintext`) gated on `backfilled` AND a live
 // coverage check. Version + encrypted-column NAME come from the domain types
-// via `resolveEncryptedColumn` — the `<col>_encrypted` naming is a
-// convention, never relied upon. The v2 paths are pinned alongside as
-// regression guards.
+// via `resolveColumnLifecycle` — the `<col>_encrypted` naming is a
+// convention, never relied upon — and both commands FAIL CLOSED when
+// resolution is ambiguous instead of guessing a lifecycle. The v2 paths are
+// pinned alongside as regression guards.
 
 const queryMock = vi.hoisted(() =>
   vi.fn(async (_sql: string) => ({ rows: [] as unknown[] })),
@@ -23,16 +24,12 @@ vi.mock('pg', () => ({
 }))
 
 type ColumnInfo = { column: string; domain: string; version: 2 | 3 }
+type ResolvedInfo = ColumnInfo & { via: 'hint' | 'convention' | 'sole' }
+type Lifecycle = { info: ResolvedInfo | null; candidates: ColumnInfo[] }
 
 const migrateMocks = vi.hoisted(() => ({
-  resolveEncryptedColumn: vi.fn(
-    async (): Promise<ColumnInfo | null> => ({
-      column: 'email_encrypted',
-      domain: 'eql_v3_text_search',
-      version: 3,
-    }),
-  ),
   listEncryptedColumns: vi.fn(async (): Promise<ColumnInfo[]> => []),
+  pickEncryptedColumn: vi.fn(() => null),
   readManifest: vi.fn(async () => null),
   countUnencrypted: vi.fn(async () => 0),
   progress: vi.fn(
@@ -47,6 +44,22 @@ const migrateMocks = vi.hoisted(() => ({
 }))
 vi.mock('@cipherstash/migrate', () => migrateMocks)
 
+// Mock the lifecycle RESOLUTION (each test states its scenario directly)
+// but keep the real `explainUnresolved` — the fail-closed messaging is part
+// of what these tests pin.
+const lifecycleMock = vi.hoisted(() =>
+  vi.fn(
+    async (): Promise<{
+      info: (ColumnInfo & { via: string }) | null
+      candidates: ColumnInfo[]
+    }> => ({ info: null, candidates: [] }),
+  ),
+)
+vi.mock('../lib/resolve-eql.js', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('../lib/resolve-eql.js')>()
+  return { ...actual, resolveColumnLifecycle: lifecycleMock }
+})
+
 vi.mock('@clack/prompts', () => ({
   intro: vi.fn(),
   outro: vi.fn(),
@@ -104,6 +117,13 @@ const V2_INFO: ColumnInfo = {
   version: 2,
 }
 
+function resolved(
+  info: ColumnInfo,
+  via: ResolvedInfo['via'] = 'convention',
+): Lifecycle {
+  return { info: { ...info, via }, candidates: [info] }
+}
+
 /** v2 config machine present + a pending config row. */
 function mockV2ConfigQueries() {
   queryMock.mockImplementation(async (sql: string) => {
@@ -129,7 +149,7 @@ describe('encrypt cutover — EQL version awareness', () => {
     vi.clearAllMocks()
     queryMock.mockResolvedValue({ rows: [] })
     migrateMocks.progress.mockResolvedValue({ phase: 'backfilled' })
-    migrateMocks.resolveEncryptedColumn.mockResolvedValue(V3_INFO)
+    lifecycleMock.mockResolvedValue(resolved(V3_INFO))
   })
 
   it('short-circuits on a backfilled v3 column: guidance, no rename, no config machine, exit 0', async () => {
@@ -169,8 +189,22 @@ describe('encrypt cutover — EQL version awareness', () => {
     exitSpy.mockRestore()
   })
 
+  it('v3 already dropped: terminal phase is "nothing to do", not "finish the backfill"; exit 0', async () => {
+    migrateMocks.progress.mockResolvedValue({ phase: 'dropped' })
+    const exitSpy = spyExit()
+
+    await cutoverCommand({ table: 'users', column: 'email' })
+
+    expect(p.log.info).toHaveBeenCalledWith(
+      expect.stringContaining('already completed'),
+    )
+    expect(p.log.error).not.toHaveBeenCalled()
+    expect(exitSpy).not.toHaveBeenCalled()
+    exitSpy.mockRestore()
+  })
+
   it('uses the RESOLVED encrypted column name, not the naming convention', async () => {
-    migrateMocks.resolveEncryptedColumn.mockResolvedValue(V3_CUSTOM_INFO)
+    lifecycleMock.mockResolvedValue(resolved(V3_CUSTOM_INFO, 'hint'))
 
     await cutoverCommand({ table: 'users', column: 'email' })
 
@@ -182,8 +216,29 @@ describe('encrypt cutover — EQL version awareness', () => {
     )
   })
 
+  it('fails closed when EQL columns exist but none is identifiable', async () => {
+    lifecycleMock.mockResolvedValue({
+      info: null,
+      candidates: [
+        { column: 'a_enc', domain: 'eql_v3_text_eq', version: 3 },
+        { column: 'b_enc', domain: 'eql_v3_text_eq', version: 3 },
+      ],
+    })
+    const exitSpy = spyExit()
+
+    await cutoverCommand({ table: 'users', column: 'email' })
+
+    expect(p.log.error).toHaveBeenCalledWith(
+      expect.stringContaining('Cannot identify which encrypted column'),
+    )
+    expect(p.log.error).toHaveBeenCalledWith(expect.stringContaining('a_enc'))
+    expect(migrateMocks.renameEncryptedColumns).not.toHaveBeenCalled()
+    expect(exitSpy).toHaveBeenCalledWith(1)
+    exitSpy.mockRestore()
+  })
+
   it('still runs the v2 flow for a v2 column (regression pin)', async () => {
-    migrateMocks.resolveEncryptedColumn.mockResolvedValue(V2_INFO)
+    lifecycleMock.mockResolvedValue(resolved(V2_INFO))
     mockV2ConfigQueries()
 
     await cutoverCommand({ table: 'users', column: 'email' })
@@ -196,8 +251,7 @@ describe('encrypt cutover — EQL version awareness', () => {
   it('explains a v3-only database instead of a raw relation-does-not-exist error', async () => {
     // Detection missed (e.g. no EQL columns visible) → v2 path — but the
     // config table doesn't exist on this database.
-    migrateMocks.resolveEncryptedColumn.mockResolvedValue(null)
-    migrateMocks.listEncryptedColumns.mockResolvedValue([])
+    lifecycleMock.mockResolvedValue({ info: null, candidates: [] })
     queryMock.mockImplementation(async (sql: string) =>
       typeof sql === 'string' && sql.includes('to_regclass')
         ? { rows: [{ exists: null }] }
@@ -220,7 +274,7 @@ describe('encrypt drop — EQL version awareness', () => {
   beforeEach(() => {
     vi.clearAllMocks()
     queryMock.mockResolvedValue({ rows: [] })
-    migrateMocks.resolveEncryptedColumn.mockResolvedValue(V3_INFO)
+    lifecycleMock.mockResolvedValue(resolved(V3_INFO))
     migrateMocks.countUnencrypted.mockResolvedValue(0)
   })
 
@@ -246,6 +300,24 @@ describe('encrypt drop — EQL version awareness', () => {
     )
   })
 
+  it('v3: the generated migration re-verifies coverage at APPLY time, atomically', async () => {
+    migrateMocks.progress.mockResolvedValueOnce({ phase: 'backfilled' })
+
+    await dropCommand({ table: 'users', column: 'email' })
+
+    // The CLI-side count goes stale the moment the file is written; the
+    // migration must lock, re-count, and abort without dropping if any
+    // plaintext-only row appeared between generation and application.
+    const sql = writeFileMock.mock.calls[0]?.[1] as string
+    expect(sql).toContain('LOCK TABLE "users" IN ACCESS EXCLUSIVE MODE')
+    expect(sql).toContain('RAISE EXCEPTION')
+    expect(sql).toContain('"email" IS NOT NULL')
+    expect(sql).toContain('"email_encrypted" IS NULL')
+    // The DROP itself runs inside the same DO block (EXECUTE), so
+    // check-and-drop stay atomic even under non-transactional runners.
+    expect(sql).toMatch(/EXECUTE 'ALTER TABLE "users" DROP COLUMN "email"'/)
+  })
+
   it('v3: refuses to generate when rows are still plaintext-only', async () => {
     migrateMocks.progress.mockResolvedValueOnce({ phase: 'backfilled' })
     migrateMocks.countUnencrypted.mockResolvedValueOnce(7)
@@ -266,7 +338,7 @@ describe('encrypt drop — EQL version awareness', () => {
   })
 
   it('v3: gates coverage on the RESOLVED encrypted column name', async () => {
-    migrateMocks.resolveEncryptedColumn.mockResolvedValue(V3_CUSTOM_INFO)
+    lifecycleMock.mockResolvedValue(resolved(V3_CUSTOM_INFO, 'hint'))
     migrateMocks.progress.mockResolvedValueOnce({ phase: 'backfilled' })
 
     await dropCommand({ table: 'users', column: 'email' })
@@ -281,6 +353,54 @@ describe('encrypt drop — EQL version awareness', () => {
     expect(sql).toContain('DROP COLUMN "email"')
   })
 
+  it("v3: refuses a by-elimination ('sole') match — uniqueness cannot prove the pairing", async () => {
+    // The table's ONE EQL column may encrypt a DIFFERENT field; gating
+    // coverage on it and dropping `email` could destroy the only copy.
+    lifecycleMock.mockResolvedValue(
+      resolved(
+        { column: 'secret_blob', domain: 'eql_v3_text_search', version: 3 },
+        'sole',
+      ),
+    )
+    // No progress stub: the sole-match guard fires before the phase gate
+    // ever consults cs_migrations. (Queuing an unconsumed
+    // mockResolvedValueOnce here would leak into the next test.)
+    const exitSpy = spyExit()
+
+    await dropCommand({ table: 'users', column: 'email' })
+
+    expect(p.log.error).toHaveBeenCalledWith(
+      expect.stringContaining('nothing confirms it encrypts "email"'),
+    )
+    expect(p.log.error).toHaveBeenCalledWith(
+      expect.stringContaining('--encrypted-column secret_blob'),
+    )
+    expect(migrateMocks.countUnencrypted).not.toHaveBeenCalled()
+    expect(writeFileMock).not.toHaveBeenCalled()
+    expect(exitSpy).toHaveBeenCalledWith(1)
+    exitSpy.mockRestore()
+  })
+
+  it('fails closed when EQL columns exist but none is identifiable', async () => {
+    lifecycleMock.mockResolvedValue({
+      info: null,
+      candidates: [
+        { column: 'a_enc', domain: 'eql_v3_text_eq', version: 3 },
+        { column: 'b_enc', domain: 'eql_v3_text_eq', version: 3 },
+      ],
+    })
+    const exitSpy = spyExit()
+
+    await dropCommand({ table: 'users', column: 'email' })
+
+    expect(p.log.error).toHaveBeenCalledWith(
+      expect.stringContaining('Cannot identify which encrypted column'),
+    )
+    expect(writeFileMock).not.toHaveBeenCalled()
+    expect(exitSpy).toHaveBeenCalledWith(1)
+    exitSpy.mockRestore()
+  })
+
   it('v3: rejects when not yet backfilled', async () => {
     migrateMocks.progress.mockResolvedValueOnce({ phase: 'backfilling' })
     const exitSpy = spyExit()
@@ -296,7 +416,24 @@ describe('encrypt drop — EQL version awareness', () => {
   })
 
   it('v2: unchanged — requires cut-over, no coverage gate, drops <col>_plaintext (regression pin)', async () => {
-    migrateMocks.resolveEncryptedColumn.mockResolvedValue(V2_INFO)
+    lifecycleMock.mockResolvedValue(resolved(V2_INFO))
+    migrateMocks.progress.mockResolvedValueOnce({ phase: 'cut-over' })
+
+    await dropCommand({ table: 'users', column: 'email' })
+
+    const sql = writeFileMock.mock.calls[0]?.[1] as string
+    expect(sql).toContain('DROP COLUMN "email_plaintext"')
+    expect(migrateMocks.countUnencrypted).not.toHaveBeenCalled()
+  })
+
+  it('v2 post-cutover: `email` itself carrying the v2 domain is NOT ambiguity — proceeds down the v2 path', async () => {
+    // After cutover renamed the ciphertext onto `email`, no counterpart is
+    // resolvable BY DESIGN. The fail-closed guard must recognize this state
+    // rather than blocking the one drop the lifecycle actually wants.
+    lifecycleMock.mockResolvedValue({
+      info: null,
+      candidates: [{ column: 'email', domain: 'eql_v2_encrypted', version: 2 }],
+    })
     migrateMocks.progress.mockResolvedValueOnce({ phase: 'cut-over' })
 
     await dropCommand({ table: 'users', column: 'email' })

packages/cli/src/commands/encrypt/cutover.ts

@@ -12,7 +12,7 @@ import { detectDrizzle } from '@/commands/db/detect.js'
 import { detectPackageManager, runnerCommand } from '@/commands/init/utils.js'
 import { loadStashConfig } from '@/config/index.js'
 import { scaffoldDrizzleMigration } from './drizzle-helper.js'
-import { resolveColumnLifecycle } from './lib/resolve-eql.js'
+import { explainUnresolved, resolveColumnLifecycle } from './lib/resolve-eql.js'
 
 /**
  * Options accepted by `stash encrypt cutover`. Swaps the plaintext and
@@ -70,15 +70,39 @@ export async function cutoverCommand(options: CutoverCommandOptions) {
     // DOMAIN TYPES (manifest name as a hint; the `<col>_encrypted` naming is
     // a convention, never relied upon) before any phase/config checks so v3
     // users get the real answer, not a confusing precondition error.
-    const { info } = await resolveColumnLifecycle(
+    const { info, candidates } = await resolveColumnLifecycle(
       client,
       options.table,
       options.column,
     )
+    // Fail closed on ambiguity: `info === null` with EQL columns present
+    // means we can't tell WHICH lifecycle applies — running the v2 config
+    // machine against (possibly) v3 columns would only produce a misleading
+    // downstream error. (No EQL columns at all, or the post-cutover v2
+    // same-name state, still falls through to the v2 preconditions below.)
+    const unresolved = explainUnresolved(
+      options.table,
+      options.column,
+      candidates,
+    )
+    if (!info && unresolved) {
+      p.log.error(unresolved)
+      exitCode = 1
+      return
+    }
     const state = await progress(client, options.table, options.column)
 
     if (info?.version === 3) {
       const encryptedColumn = info.column
+      if (state?.phase === 'dropped') {
+        // Terminal phase — the lifecycle already finished. Not an error and
+        // not "finish the backfill": there is nothing left to backfill.
+        p.log.info(
+          `${options.table}.${options.column} has already completed the EQL v3 lifecycle (plaintext dropped). Nothing to cut over.`,
+        )
+        p.outro('Nothing to do for EQL v3.')
+        return
+      }
       if (state?.phase !== 'backfilled') {
         // Not a "nothing to do" — the user isn't ready for ANY next step
         // yet. Exit 1 so scripted pipelines gating on cutover don't read