EQL v2 removal: review remediation for #772

[tobyhede]

Tracks review remediation for #772 (remove-v2).

Scoping principle: nothing v2-related is deferred. Everything below is either about v2 or was created/exposed by removing it. Items are sequenced by which PR lands them, not by whether they block the merge.

In-flight branch: fix/remove-v2-review-remediation (head c8c15121, 5 commits, +1,729/−225) on top of remove-v2 @ a16302fc.

Verification method: three independent agents re-derived every verdict by executing the branch's real exported functions and compiling probes — with control runs of the pre-fix code on identical fixtures to separate "fixed" from "was always like that". Baseline extracted via git archive from origin/remove-v2. Nothing was trusted from commit messages or changeset prose. Prior verified findings: .work/2026-07-24-eql-v2-removal-verified-findings.md.

---

✅ Fixed and verified on the remediation branch

Migration rewriter — a fully -- commented-out ALTER is now inert in both the wizard and CLI copies (rewritten: [], skipped: [], file byte-identical, no DROP COLUMN). Control run on pre-fix code emitted a live ALTER TABLE "users" DROP COLUMN "email";, so the fixture is valid. The legitimate text → eql_v3_text_search rewrite still works — no behavioural regression. W-4 resolved: initialValue: !destructive, so the prompt defaults to No after any rewrite or flag, and the warning now states it "DESTROYS data on any table that already holds rows".

Type surface — S-4 (overload/runtime agreement, via V3ClientConfig) and S-6 (Encryption({ schemas: [] }) now a compile error) both confirmed against baseline. test:types → 99 passed, in the CI glob.

stack-supabase — SB-1, SB-2, SB-3, SB-4, SB-6 all fixed, and more thoroughly than asked: returns<U>() is type and runtime backed, withLockContext/audit after .single() are now allowed rather than merely documented, and the changeset was rewritten to disclose the break it previously hid. Zero consumers of the deleted helpers. test:types 40 passed, suite 466 passed.

CI — X-4 (packages/stack/src/encryption/** added to both push and PR trigger lists) and X-3 (test-kit/install alias, real consumer at bench/src/harness/global-setup.ts:5).

Changesets — D-2 and DDB-1 corrected.

Skills/docs — stash-drizzle:426, stash-encryption:810,185,862, stash-supabase:603,718, stash-cli:369,436,454, stash-dynamodb:519, AGENTS-doctrine:10,21, setup-prompt:280,306, stack-drizzle README (ops.matches), docs/reference/supabase-sdk.md:137.

S-7 partial — a genuine v3-reads-v2 test now exists (#1c, 4 tests) using a v3 client built from an unrelated table, with { v: 2 } assertions blocking a false pass, plus DynamoDB cross-generation coverage. Requires live credentials, fails loudly rather than skipping, runs in CI via CS_IT_SUITE.

---

🔴 Must fix before the remediation branch merges

1. W-1 is still reachable — data loss

An odd apostrophe inside a double-quoted identifier ("o'brien_data") opens a phantom string, isInsideComment returns false, and a fully commented-out ALTER becomes a live DROP COLUMN. Exact original signature, reproduced on the branch.

2. Rewriter must never emit destructive SQL — DECIDED: fail closed

The current fix is a per-directory corpus heuristic, not a source-type check. It correctly skips when the column's CREATE/ADD is in the same directory (reason: 'already-encrypted'), but with the ALTER alone it still emits DROP COLUMN over ciphertext, silently. Triggers: drizzle-kit push users, squashed history, baseline installed outside the migration dir, and cross-directory splits since sweepMigrationDirs indexes per-directory.

Decision (2026-07-24): the rewriter must never emit SQL that can destroy data. This is wider than the unknown-source case. The rewriter's own emitted comment concedes that the ADD+DROP+RENAME shape "is safe ONLY if the table is empty. On a populated table it DESTROYS existing data" — so even the legitimate plaintext→encrypted rewrite is destructive on a populated table, and the only safeguard is a comment inside generated SQL that W-1 proved can be silently mangled.

Required shape:

1. Never emit an executable DROP COLUMN / RENAME COLUMN. Emit the non-destructive ADD COLUMN only and direct the user to the staged stash encrypt path (add → backfill → cutover → drop), which preserves the data. A generated migration must be safe to run on a populated table by someone who never read the comments.
2. Fail closed when the source type cannot be proven — refuse, report with a distinct SkipReason, exit non-zero.
3. Leave the statement untouched rather than writing partial output.

Accepted cost: more statements need human attention, concentrated on users whose baseline was installed by stash eql install directly against the database, who adopted Drizzle late, or who squashed history.

Introspection — checked, cannot be the primary mechanism. Whether the column's live type could be introspected instead of inferred was checked at all three call sites: stash eql install (db/install.ts:648) has a connection (databaseUrl at :243, pg.Client at :396); stash eql migration (eql/migration.ts:236) has none at all — no pg import, no databaseUrl, by design, since its docstring says it emits a migration "rather than running SQL directly against the database"; the wizard (post-agent.ts:118) doesn't receive one but packages/wizard depends on pg, so it could be threaded through.

Since the gap is widest in eql migration — the migration-first path the docs recommend, and the one most likely to run against a schema whose history isn't in the corpus — the corpus check and fail-closed default must be correct with no connection. Introspection is an optional noise-reducer where a connection happens to exist.

Implement no-destructive-SQL + fail-closed first, then measure whether the flagging is actually annoying before adding introspection. The decision above removes most of the pressure for it: once the rewriter never emits DROP COLUMN, an unproven source costs an extra manual step rather than silent data loss.

3. Rewriter test gaps

The new tests are genuine (139 wizard / 130 CLI passing) but don't cover: the apostrophe case above, W-3's no-source-in-dir case, the cross-directory case, case-insensitivity, or CRLF. Also unfixed: an ALTER inside a '…' string literal is rewritten and the injected apostrophe turns the migration into a syntax error (not destructive, but wrong).

DECIDED 2026-07-24 — items 4, 6 and 7 are one piece of work. Policy: maximise type safety, minimise customer effort, absorb the cost here. Restructure Encryption to a single signature with a conditional return type rather than documenting the EncryptionClientFor<S> idiom-tax. One signature makes ReturnType resolve correctly (item 6); S extends readonly AnyV3Table[] accepts non-tuple arrays again so the breaking change in item 4 disappears entirely rather than being documented; folding the config into the conditional makes types and runtime agree on eqlVersion: 2 (item 7). Must not regress S-6 (schemas: [] stays a compile error) — reject the empty tuple at the parameter. Reordering the overloads is NOT a fix: resolution takes the first match and ReturnType the last, so they pull in opposite directions. Full target shape, risks and acceptance criteria in .work/2026-07-24-eql-v2-removal-work-orders.md §A-6.

4. New breaking change, undocumented

The overload rework means a non-tuple const schemas: AnyV3Table[] — a shared schema module, a dynamic list, a spread — now matches neither overload. It compiled on remove-v2. Array literals are unaffected.

error TS2769: No overload matches this call.
  Overload 1 ... Type 'AnyV3Table[]' is not assignable to type 'readonly [AnyV3Table, ...AnyV3Table[]]'.
  Overload 2 ... Type 'AnyV3Table[]' is not assignable to type 'AtLeastOneCsTable<BuildableTable>'.

prisma-next had to destructure-and-respread to cope; customers will hit the same. Needs a changeset and a migration note.

5. @cipherstash/prisma-next public option narrowed, no changeset

CipherstashFromStackV3Options.encryptionConfig narrowed from ClientConfig to V3ClientConfig, so { eqlVersion: 2 } no longer compiles. Correct change, but there is no @cipherstash/prisma-next changeset on this branch.

6. S-2 was pinned, not fixed

Awaited<ReturnType<typeof EncryptionV3>> still resolves to the nominal client — probe output byte-identical to baseline. A workaround type EncryptionClientFor<S> was added and the new test-d file asserts the broken resolution is expected. The five real call sites were never converted:

integration/adapter.ts(160,7): error TS2739 ...
integration/json-adapter.ts(75,7): error TS2739 ...
integration/lock-context.integration.test.ts(158,3): error TS2739 ...
integration/null-persistence.integration.test.ts(110,3): error TS2739 ...
integration/relational.integration.test.ts(215,3): error TS2739 ...

Decide explicitly: convert the call sites, or accept EncryptionClientFor<S> as the documented idiom and say so in a changeset. Either way stack-drizzle's integration/** is in tsconfig include but typechecked by nothing in CI — see §7.

7. S-4 residual — direction flipped, not closed

A ClientConfig-typed variable now selects the nominal overload while the runtime returns the typed client, so c.init compiles and is undefined at runtime. The baseline errored here; the failure moved rather than disappeared. Nothing in the test-d file covers it.

8. Still shipping wrong to customers

• packages/stack/README.md:446-451 — the headline Supabase example still teaches the deprecated encryptedSupabaseV3, contradicting :766/:785 in the same file. The commit fixed :754 and :795-808 but missed this. Ships in the @cipherstash/stack tarball.
• The auto-detect sweep is incomplete — the same false claim survives at skills/stash-supabase/SKILL.md:735,749,785, and (not previously reported) packages/migrate/README.md:9, which cites detectColumnEqlVersion as auto-detecting the generation when it returns 3|null.

9. Changeset sweep incomplete

• .changeset/eql-v3-sole-docs.md:9-13 untouched — still says "DynamoDB still requires the v2 schema surface", directly contradicting stack-dynamodb-v2-write-removal.md on the same release; still claims rollout tooling "targets v2 columns"; still advertises @cipherstash/stack-drizzle/v3 and encryptedSupabaseV3.
• 17 stale references across 9 files remain (down from 29/12; 4 further hits in remove-eql-v2-drizzle-root.md are legitimately historical). None of the 9 files were touched. Worst pair: init-placeholder-eql-v3.md:17-18 says init emits extractEncryptionSchemaV3 from /v3 while remove-eql-v2-drizzle-root.md:25 says it emits the collapsed root import — both ship in the same CHANGELOG.

---

Now, regardless of PR ordering

Add the examples/e2e typecheck CI job. turbo filters to ./packages/*; no root script invokes the examples' typecheck. This is the mechanism that let examples/basic sit broken on remove-v2 — it imported encryptedType (deleted by #770) and called the v2 encryptedSupabase (deleted by #769) while CI stayed green. PRs 8–12 all touch the same surfaces, so leaving it unfixed guarantees a repeat. Extend the same job to packages/stack-drizzle's integration/** (§6).

---

Lands with PR 8 — the CLI v2 SQL leaf

• packages/cli/src/commands/init/lib/introspect.ts:88 matches only eql_v2_encrypted, so stash init pre-selects nothing on a v3 database and :233 announces the wrong domain. Byte-identical on main, and PR 8 opens the file anyway.
• Delete the vendored v2 SQL (22,512 lines; the Supabase and no-operator-family bundles are byte-identical duplicates), encrypt/cutover.ts, db/push.ts, db/activate.ts, db/supabase-migration.ts, migrate/src/eql.ts, and the --eql-version / --latest flags. eql upgrade survives — db/upgrade.ts:29 already defaults to v3.
• Must-survive (verified reachable): db/status.ts:32-62 legacy detection, the manifest eqlVersion fallbacks in encrypt/status.ts and status/index.ts, and eql_v2_encrypted in rewrite-migrations.ts:11.
• Same-PR mitigation: deleting the vendored SQL removes the only offline way to recreate public.eql_v2_encrypted when restoring a v2 dump. Surface installer/index.ts:38-40's upstream eql-2.3.1 URL in the --eql-version 2 rejection.

Lands with PRs 9–12 — the stack authoring surface

src/schema/index.ts is 754 lines, ~511 of them v2-only authoring; all three root re-exports at src/index.ts:9 are v2-only. resolveEqlVersion returns undefined for an all-v2 set and protect-ffi reads that as v2, so Encryption({ schemas: [v2Table] }) still writes v2 wire with no flag. Removing the DSL does not remove that branch — the loose BuildableTable path keeps emitting v2 silently unless the wire contract is decided in the same PR.

Before the rc.5 publish

Everything under §8 above, plus a re-sweep of skills/ and the package READMEs once PRs 8–12 land. These ship inside the stash tarball and are copied into customer repos by installSkills(); nothing type-checks them.

---

🚧 Hard gate on PRs 9–12

The v2-decrypt fixture is still minted through the emission path being deleted. v2-decrypt-compat.integration.test.ts:70-76 builds v2Client = await Encryption({ schemas: [usersV2], config: { eqlVersion: 2 } }), and all four #1c tests encrypt through it first. There is no static ciphertext in the file. Deleting the eqlVersion: 2 emission path kills beforeAll and therefore #1a, #1b and the new #1c together — the file's own header concedes this.

The fixture replacement must land in the same PR as the removal, not after. A purely static fixture is not viable alone: ZeroKMS ciphertexts are keyset-bound and packages/test-kit/src/env.ts sources credentials from a developer profile, so a fixture minted against CI's workspace won't decrypt locally.

---

Also on the branch, verified benign

packages/cli/src/commands/db/install.ts and eql/migration.ts are output-only (print describeSkipReason(reason) per skipped statement; reword "could not rewrite" → "left alone") with no change to what gets rewritten. The wizard/CLI rewriter copies remain behaviourally identical — both gave identical verdicts across all 21 probe fixtures; only a wizard-only sweepMigrationDirs and the banner string differ.