fix(stack): repair the published v3 typed client and gate the surfaces that compiled nowhere - #782
Conversation
🦋 Changeset detectedLatest commit: 124048d The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fe7938e to
1fbbd71
Compare
freshtonic
left a comment
There was a problem hiding this comment.
Approve.
Verified the Result contract, EncryptionErrorTypes strings, .withLockContext() availability, encrypted-payload shape, and ESM/CJS exports are all untouched. The declare readonly __domain?: D phantom carrier in eql/v3/columns.ts is the right minimal fix for the typed encryptQuery-resolves-to-never bug, and shipping it with a dist-types typecheck gate (plus @ts-expect-error guards so the assertions can't pass vacuously) closes the exact gap that let that regression survive the rc series. The turbo.json outputs: ["dist/**"] fix is a genuine correctness issue (cache hits were restoring no files). Semver targeting is correct — the train is already major, and the incremental changesets here are appropriately additive.
Non-blocking nits (no change required to merge):
- The A-4
schemaswidening is genuinely non-breaking (it only broadens what compiles;Encryption({ schemas: [] })is still a compile error), yet commitee455f15ais taggedfix(stack)!. Cosmetic labeling inconsistency, no semver risk. packages/stack/package.json— the newtest:types:distscript line is space-indented while siblings use tabs. Biome passed (warnings only), so purely cosmetic.
…gates Covers A-4, A-6, A-7 and C-1 from the EQL v2 removal verification, and records what is deliberately excluded: the single generic signature and ClientFor<S, C> machinery belong with the v2 removal (#637), not here.
`Encryption` picks its return value with two discriminators that can disagree — overload resolution reads the `config` argument, the runtime reads the `schemas` argument. Hoisting a config into a `ClientConfig`-typed variable splits them: `ClientConfig.eqlVersion` is `2 | 3`, which the v3 overload's `eqlVersion?: 3` rejects, so the call resolves to the nominal client while the runtime still returns the typed one. `init` was the only member of `EncryptionClient` absent from the typed client, so anything holding that client through its declared type hit `TypeError: client.init is not a function`. Delegate it, and assert the parity so a future member cannot reopen the hole. The remaining half — the silent loss of the typed surface — is not closable at the type level (the runtime inspects values, the type inspects an erasable static type). It ends with the EQL v2 removal (#637).
`Encryption({ schemas })` only accepted an array LITERAL. Every indirect
form failed with TS2769: a shared `export const all: AnyV3Table[]`, the
`ReadonlyArray<AnyV3Table>` prisma-next exposes publicly, anything
push-built or spread, an unannotated `const`.
The cause was how the empty-schema hole was closed — by constraining the
type parameter to a non-empty TUPLE, which also excluded every non-literal.
Move non-emptiness onto the `schemas` property via `NonEmptyV3<S>` and let
the parameter be the array. `Encryption({ schemas: [] })` is still a compile
error, and the literal path keeps its per-column plaintext typing — the
guard has to sit on the property, since wrapping the config in a
conditional alias defeats `const` inference.
`EncryptionClientFor` carried the same tuple guard and is widened in step.
Left alone it fell through to the nominal client for a loose `AnyV3Table[]`
— handing the wrong type to exactly the callers this enables.
Prisma-next's destructure-and-respread workaround can now be removed.
…For (A-6)
`ReturnType` reads the LAST overload, which is the nominal one, so
`Awaited<ReturnType<typeof Encryption>>` yielded `EncryptionClient` even for
an all-v3 schema set and every assignment of the real client to it failed
with TS2739. 15 sites carried that error — 10 in stack, 5 in
stack-drizzle — and none was visible to CI, because the only typecheck step
in either package is scoped to `__tests__/**/*.test-d.ts`.
Overload order cannot fix it (putting the nominal signature first
mis-resolves v3 schemas instead, since a v3 table structurally satisfies
BuildableTable), so name the client instead — which is what every
comparable library does: z.infer, arktype's typeof T.infer, hono's
Client<T>.
Sweeps the idiom out entirely, not just the erroring sites: nominal
callers now say `EncryptionClient`, v3 callers `EncryptionClientFor<S>`.
packages/bench drops the single-signature helper it wrapped Encryption in
for exactly this reason.
Typing these clients correctly unmasked call sites the wrong type had been
hiding. Most were `as never` casts that are simply no longer needed, and
dropping them means the calls are now genuinely checked. Two are not:
encrypt-query-stevec and encrypt-query-searchable-json exercise
`encryptQuery` query types the typed client mis-models — it derives the
plaintext from the column's domain, so every query type on a types.Json()
column is typed JsonDocument, but the SteVec types take a JSONPath string,
a { path, value } pair, or a bare scalar. Those two suites hold the client
through the nominal surface with the gap named at the cast, rather than
casting at every call and hiding it.
packages/stack tsconfig: 168 -> 147 errors, none new.
packages/stack-drizzle tsconfig: 69 -> 63 errors, none new.
Four packages had a `tsconfig.json` covering their src and tests that no CI
step ever ran, and were already clean — gate them before they drift:
migrate, nextjs, examples/prisma and e2e. `packages/nextjs` needed two
fixes first: `vi.Mock` was used as a TYPE, which needs `import type { Mock }`.
Also declare `outputs: ["dist/**"]` on turbo's `build`. It declared none, so
a cache hit replayed logs and restored no files — and the examples/basic
gate added by 5fab1cf typechecks against `packages/stack/dist/*.d.ts`. It
held only because fresh CI runners have no cache. Verified by deleting
packages/stack/dist and re-running the gate: cache hit, dist restored,
still green.
Deliberately NOT gated, with counts recorded in the workflow so the gap is
visible rather than silent: @cipherstash/stack (147), stack-drizzle (63),
stash (21), stack-supabase (11). Their `test:types` scripts are scoped to
`__tests__/**/*.test-d.ts`, so src, the runtime suites and integration/**
compile nowhere. Most of the drizzle and supabase count is a single root
cause in @cipherstash/test-kit's V3_MATRIX.
- Two new changesets: the schema-array widening (stack minor, prisma-next patch) and the typed-client init parity (stack patch). - stack-audit-on-decrypt.md dropped the paragraph telling callers to narrow AnyV3Table[] to readonly [AnyV3Table, ...AnyV3Table[]]. That advice is obsolete, and it never worked for ReadonlyArray<AnyV3Table> — the type prisma-next exposes publicly — since readonly also failed the old tuple constraint. - prisma-next drops its destructure-and-respread workaround. Its comment claimed 'Encryption requires a non-empty schema tuple', which is no longer true; the runtime emptiness check and its error message stay. - skills/stash-encryption documents EncryptionClientFor as the way to name the client, and that schemas need not be an array literal.
`packages/bench`'s build is `tsc --noEmit` — a typecheck, not a bundle — so the repo-wide `outputs: ["dist/**"]` added alongside it warned 'no output files found' on every run.
CodeRabbit review of this branch. Two guidance defects that would have shipped into customer repos: - `skills/stash-encryption` claimed reads of the promoted column return "decrypted ciphertext transparently" after `stash encrypt cutover`. Only CipherStash Proxy decrypts on the wire; SDK/ORM reads still return ciphertext, which is what the very next table row says. An agent following the table would return raw `eql_v2_encrypted` composites to end users. - `stash init`'s setup prompt told agents to declare an EQL v2 cutover target with a `types.*` domain. `types.*` is the v3 domain family; a v2 column is an `eql_v2_encrypted` composite. v2 is a read path now, so the branch points at the deprecated `@cipherstash/stack/schema` builders, decrypting through `@cipherstash/stack`. Also: the `EncryptedSingleQueryBuilder` doc comment said a method omitted from the interface "is absent at runtime too". `single()` returns `this` (query-builder.ts:479), so every filter and transform remains a property of the object — the interface narrows the static API only. Skipped three markdownlint findings (MD040 aside): this repo runs no markdownlint, and MD028 on the supabase skill and the code-span whitespace in the rewriter changeset are both correct as written under CommonMark.
The existing changeset already states that the bundled skills described the v2 lifecycle as the default and that the guidance ships into customer repos. Enumerating each individual correction adds nothing for a reader of the changelog.
`client.encryptQuery(value, { table, column })` did not typecheck for ANY
column when imported from the published package — every plaintext resolved
to `never`. Reproduced identically against the rc.4 tarball on npm,
released main @ b48494c, origin/remove-v2 and this branch, so it predates
the v2 removal and shipped in every release carrying the v3 typed client.
`PlaintextForColumn` / `QueryTypesForColumn` recover the domain via
`C extends EncryptedV3Column<infer D>`, which needs `D` bare in the instance
type. Its only bare site was `private readonly definition: D`, and tsc
strips private member types on declaration emit — leaving only
`D['eqlType']`, `D['capabilities']` and `QueryableFlag<D>`, none of which
TypeScript can invert. So `infer D` fell back to the V3DomainDefinition
constraint: QueryTypesForColumn -> never -> QueryableColumnsOf -> never ->
plaintext never. `encrypt` escaped it by resolving through `ColumnsOf`.
Fix is a type-only `declare readonly __domain?: D`: nothing emitted at
runtime, no constructor or call-site change, but it survives declaration
emit and restores the inference site.
Adds `packages/stack/dist-types` and a `test:types:dist` script that
typechecks the EMITTED declarations rather than source, wired into CI.
That gap is why this survived the whole rc series — every other type test
imports `@/…`. Verified red without the fix (three errors, including the
original `never`) and green with it.
Found while converting the A-6 call sites: the same tests that would have
caught it were holding the client through the broken `ReturnType` idiom.
Two corrections to the gate added in the previous commit: - Exclude `dist-types` from `packages/stack/tsconfig.json`. That config maps `@/*` and the `@cipherstash/stack/*` subpaths to SOURCE, so checking these files there resolved their imports to the very thing they exist to avoid and reported two failures that said nothing about what ships. - Move the `@ts-expect-error` directives onto the exact lines the errors are reported at. A directive only covers the following line, and the formatter is free to split a call across several — which it did, silently turning one negative into 'unused directive' and letting the other through as a real error. Re-proved red/green by deleting the phantom field and rebuilding: 4 errors without it, including the original `never`; clean with it.
EncryptionV3 used to force `eqlVersion: 3` over whatever the caller passed, and
its comment said that override was the whole point: a v2-mode client cannot
author v3 concrete-domain columns. Collapsing EncryptionV3 into a deprecated
alias of Encryption deleted the override, and nothing else caught the
combination — resolveEqlVersion throws for a mixed set and for legacy v2
searchableJson, then returns an explicit version unchanged.
So `EncryptionV3({ schemas: [v3Table], config: { eqlVersion: 2 } })` went from
silently auto-corrected-and-working to silently building a v2-wire client, with
no diagnostic anywhere. Verified against the FFI boundary: newClient received
eqlVersion 2, and the encryptConfig and the args reaching ffi.encrypt were
byte-identical to the v3 case — only the flag differed. The failure surfaces
later, as an eql_v3_* domain CHECK rejecting the write, or as v2 wire landing
in a v3 column wherever the check is looser.
Throw instead, keyed on `v3Count === schemas.length` so the hatch survives
exactly where it is used: minting v2 wire from a V2 schema set, which is what
integration/shared/v2-decrypt-compat.integration.test.ts does. Nothing in the
repo mints v2 wire from a v3 table — the unit test that asserted that shape was
pure, with a rationale ("for minting v2 wire during a migration") no consumer
ever exercised. Retargeted, along with the two init-strategy tests that pinned
the same behaviour with comments already hedging about it.
Also widens stack-drizzle's typecheck gate to the two integration adapters that
this branch made zero-error (A-6). The remaining 63 errors are still ungated and
still recorded in the workflow; most are one root cause in test-kit's V3_MATRIX
(#778). Verified the widened gate actually fails on an injected error rather
than silently including nothing.
936efe0 to
ded4921
Compare
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed at the true PR head (ded4921d, 59 files, +1104/-161), verifying contracts against source and running the checks/tests myself.
Approving. A disciplined, well-reasoned change. Runtime impact is limited to two things — an init passthrough on the typed client (closes a real TypeError crash) and a new setup-time throw that fails safer (refusing v2 wire over an all-v3 schema set) — everything else is type-level or CI plumbing.
Verified
- Public contracts preserved: Result shape
{ data } | { failure }unchanged;EncryptionErrorTypesuntouched (the new guard throws a plain setup-timeErrorconsistent with sibling throws, beforeinit);.withLockContext()preserved (A-6 changes are type-annotation-only); noexportsmaps changed. pnpm run build(9/9), the newtest:types:distgate, the C-1 typecheck gates (migrate/nextjs/examples/prisma/e2e), 62/62 mocked unit tests, 102/102.test-d, and 7/7 stack-drizzle type tests all pass. (Live credentialed suites not run — orthogonal to these type/CI changes.)
Highlights
typed-client-nominal-parity.test.tsreflectively asserts everyEncryptionClient.prototypemember exists on the typed client — a durable invariant guard, not a point fix.- Typechecking the emitted
.d.ts(dist-types/, wired into CI) closes the exact blind spot that let theencryptQuery-is-neverbug ship through the rc series. Strong systemic thinking. turbo.jsonoutputs: ["dist/**"]fixes a latent cache-correctness bug.
Non-blocking findings
packages/stack/src/encryption/index.ts(~L92-94 and ~L919-921): two comments still cite "writing v2 wire from a v3 schema set during a migration" as the retained escape hatch — but that's exactly what the new A-8 throw (~L108) now rejects. Contradicts the code just below; update both to the surviving example (expliciteqlVersion: 2over a v2 schema set).- PR scope drift: the title/body enumerate only A-4/A-6/A-7/C-1, but the head also lands A-8, a runtime behaviour change (
.changeset/reject-v2-wire-over-v3-schemas.md, stack minor). The changeset exists so nothing ships invisibly, but the description undersells the contract change — add A-8 to it. - Nit: the added
test:types:dist/typechecklines inpackages/stack/package.jsonandpackages/nextjs/package.jsonuse space indentation in otherwise tab-indented files.
…hemas, stop shipping stale skills Review remediation for #782. Six agents cross-checked the review findings; three of them were overturned and three defects the PR itself introduced were found in the process. Each fix has a regression test proven to fail without it. Defects introduced by this PR ----------------------------- `init` on the typed v3 client was a way around the guard A-8 had just added. `EncryptionClient.init` takes its own `eqlVersion`, forwards `undefined` to the FFI — whose default is EQL v2 — and never consults `resolveEqlVersion`. So the A-7 passthrough let a typed v3 client be re-initialised into v2 wire while keeping its v3 surface: `eql_v2_encrypted` payloads into `eql_v3_*` columns, the same contradiction refused at construction, one method call later. The existing parity test could not see it — it stubs `init` out and asserts only that it was called. `init` now pins `eqlVersion: 3`, refuses an explicit `2` with a failure `Result` (its declared shape), and resolves to the typed client so `client = (await client.init(cfg)).data` does not silently drop to nominal. `NonEmptyV3<S>` on the `schemas` property broke every caller generic over its schemas — a shape that compiled before A-4, and the one the `EncryptionClientFor` docs point generic code at. A type parameter is assignable to a deferred conditional only if it is assignable to both branches, and one branch is `never`. Deleting the guard is not the fix (overload 1 then accepts `[]`); the v3 signature is split in two, with the tuple overload carrying non-emptiness in its constraint, where a generic `S` can satisfy it. `outputs: ["dist/**"]` could publish stale skills. `cli` and `wizard` copy the repo-root `skills/` into `dist/skills`, outside their own input scope, so a skills-only edit never invalidated the build — and once outputs were declared, a cache hit began actively restoring the previous copy. Since `stash init` installs those into customer repos, an edited skill could ship with its pre-edit text while the source on disk was correct and CI green. Reproduced end to end before fixing. Review findings --------------- Nine comments described the `eqlVersion: 2` escape hatch as still permitted, including two documenting live guards: the `isV3Only && eqlVersion === 3` branch in `Encryption`, and the `V3ClientConfig` JSDoc the prisma-next texts paraphrase. `skills/stash-encryption` claimed an empty array is a compile error. That holds only when the argument's type has a statically known length of 0 — a spread of an empty array is a literal at the call site and still compiles. Corrected, and the runtime half now has coverage it never had. Four typecheck gates compiled their own `dist/` alongside source, so they compiled a different program in CI than locally, and `migrate`'s file set depended on CI step order. `wizard` and `examples/basic` had the same defect. The declaration gate reached `dist/` by relative path under bundler resolution, so it never consulted the `exports` map: the `.d.cts` set and `wasm-inline.d.ts` — each with its own inlined copy of the column class — were ungated. The wasm entry is the documented target for Workers, Deno, Bun and Supabase Edge. `__domain` was reachable from consumer code and appeared in autocomplete on every column, typed `D | undefined` and always `undefined`; there is no `stripInternal`, so `@internal` was documentation, not enforcement. Now keyed by a non-exported `unique symbol` — as inferrable, unnameable. New tests --------- - `scripts/lint-typecheck-scope.mjs` + self-tests + CI step: a gated tsconfig must scope away its build output. - `dist-types/node16/`: `.cts`/`.mts`/wasm probes resolving by package name under node16, so the conditions a customer walks are the ones under test. - `turbo-skills-inputs.test.mjs`: a build that copies `skills/` must declare it. - `empty-schemas-boundary.test.ts`, `typed-client-init-wire-version.test.ts`. Left alone ---------- `eqlVersion: 3` over an all-v2 schema set is still accepted, writing v3 payloads into `eql_v2_encrypted` columns — the mirror of what A-8 refuses. It predates this branch and `resolve-eql-version.test.ts:115` pins it deliberately, so closing it is a product decision rather than review remediation.
…exposed The reviewer's two findings were both already fixed at bbade2c — this adds the guards that would have caught them, plus two accuracy fixes. `typecheck:scaffold` declares `dependsOn: ["^build"]`, so turbo has to be the thing invoking it. #787 first added it as a bare `pnpm --filter stash run typecheck:scaffold` and CI went green anyway: an earlier step in the same job happened to build the workspace via its own `^build`. Those earlier steps read as independent guards for other packages, so they look freely removable — delete one and the scaffold step fails `TS2307`, which reads as "the scaffold is broken" rather than "you skipped the build". Nothing caught that. scripts/__tests__/workflow-turbo-build-deps.test.mjs derives the protected set from turbo.json (every task declaring `^build`) rather than hardcoding names, and asserts tests.yml routes them through turbo. Mutation-tested against three regressions: reverting the step to the bare form, deleting the step outright, and adding a new bare build-dependent step — each fails with a message naming the step and the fix. Two pre-existing bare `typecheck` invocations (prisma-next, wizard) carry the same latent trap; they are recorded in KNOWN_BARE rather than silently skipped, and a fourth test fails if an entry goes stale so the list gets worked down instead of accumulating. turbo.json is JSONC, so reading it needs the string-aware comment stripper #782 wrote for `turbo-skills-inputs.test.mjs` — a naive regex eats the `//` in `"$schema": "https://turbo.build/schema.json"`. Rather than hand-copy 30 lines of parser into an adjacent file, that function moves to `scripts/__tests__/lib/read-jsonc.mjs` and both guards import it. Two accuracy fixes: - vitest.config.ts claimed the suite's residual `@cipherstash/stack` coupling runs through `migrate/src/backfill.ts`. That is only one of two routes: `init/lib/__tests__/introspect.test.ts:1` imports `@cipherstash/stack/eql/v3` directly, never touching migrate. As written, someone could decouple backfill.ts and expect the suite to go standalone. It would not. - cli-vitest-alias.test.mjs called `.endsWith()` on every alias value, so Vite's array form (`[{find, replacement}]`) would throw an opaque TypeError from a helper — reading as "the guard is broken" rather than "the config changed shape". It now fails cleanly, naming the offender, and says to update the guard rather than delete it. No changeset: repo tooling and a comment, no published surface touched.
…exposed The reviewer's two findings were both already fixed at bbade2c — this adds the guards that would have caught them, plus two accuracy fixes. `typecheck:scaffold` declares `dependsOn: ["^build"]`, so turbo has to be the thing invoking it. #787 first added it as a bare `pnpm --filter stash run typecheck:scaffold` and CI went green anyway: an earlier step in the same job happened to build the workspace via its own `^build`. Those earlier steps read as independent guards for other packages, so they look freely removable — delete one and the scaffold step fails `TS2307`, which reads as "the scaffold is broken" rather than "you skipped the build". Nothing caught that. scripts/__tests__/workflow-turbo-build-deps.test.mjs derives the protected set from turbo.json (every task declaring `^build`) rather than hardcoding names, and asserts tests.yml routes them through turbo. Mutation-tested against three regressions: reverting the step to the bare form, deleting the step outright, and adding a new bare build-dependent step — each fails with a message naming the step and the fix. Two pre-existing bare `typecheck` invocations (prisma-next, wizard) carry the same latent trap; they are recorded in KNOWN_BARE rather than silently skipped, and a fourth test fails if an entry goes stale so the list gets worked down instead of accumulating. turbo.json is JSONC, so reading it needs the string-aware comment stripper #782 wrote for `turbo-skills-inputs.test.mjs` — a naive regex eats the `//` in `"$schema": "https://turbo.build/schema.json"`. Rather than hand-copy 30 lines of parser into an adjacent file, that function moves to `scripts/__tests__/lib/read-jsonc.mjs` and both guards import it. Two accuracy fixes: - vitest.config.ts claimed the suite's residual `@cipherstash/stack` coupling runs through `migrate/src/backfill.ts`. That is only one of two routes: `init/lib/__tests__/introspect.test.ts:1` imports `@cipherstash/stack/eql/v3` directly, never touching migrate. As written, someone could decouple backfill.ts and expect the suite to go standalone. It would not. - cli-vitest-alias.test.mjs called `.endsWith()` on every alias value, so Vite's array form (`[{find, replacement}]`) would throw an opaque TypeError from a helper — reading as "the guard is broken" rather than "the config changed shape". It now fails cleanly, naming the offender, and says to update the guard rather than delete it. No changeset: repo tooling and a comment, no published surface touched.
…hemas, stop shipping stale skills Review remediation for #782. Six agents cross-checked the review findings; three of them were overturned and three defects the PR itself introduced were found in the process. Each fix has a regression test proven to fail without it. Defects introduced by this PR ----------------------------- `init` on the typed v3 client was a way around the guard A-8 had just added. `EncryptionClient.init` takes its own `eqlVersion`, forwards `undefined` to the FFI — whose default is EQL v2 — and never consults `resolveEqlVersion`. So the A-7 passthrough let a typed v3 client be re-initialised into v2 wire while keeping its v3 surface: `eql_v2_encrypted` payloads into `eql_v3_*` columns, the same contradiction refused at construction, one method call later. The existing parity test could not see it — it stubs `init` out and asserts only that it was called. `init` now pins `eqlVersion: 3`, refuses an explicit `2` with a failure `Result` (its declared shape), and resolves to the typed client so `client = (await client.init(cfg)).data` does not silently drop to nominal. `NonEmptyV3<S>` on the `schemas` property broke every caller generic over its schemas — a shape that compiled before A-4, and the one the `EncryptionClientFor` docs point generic code at. A type parameter is assignable to a deferred conditional only if it is assignable to both branches, and one branch is `never`. Deleting the guard is not the fix (overload 1 then accepts `[]`); the v3 signature is split in two, with the tuple overload carrying non-emptiness in its constraint, where a generic `S` can satisfy it. `outputs: ["dist/**"]` could publish stale skills. `cli` and `wizard` copy the repo-root `skills/` into `dist/skills`, outside their own input scope, so a skills-only edit never invalidated the build — and once outputs were declared, a cache hit began actively restoring the previous copy. Since `stash init` installs those into customer repos, an edited skill could ship with its pre-edit text while the source on disk was correct and CI green. Reproduced end to end before fixing. Review findings --------------- Nine comments described the `eqlVersion: 2` escape hatch as still permitted, including two documenting live guards: the `isV3Only && eqlVersion === 3` branch in `Encryption`, and the `V3ClientConfig` JSDoc the prisma-next texts paraphrase. `skills/stash-encryption` claimed an empty array is a compile error. That holds only when the argument's type has a statically known length of 0 — a spread of an empty array is a literal at the call site and still compiles. Corrected, and the runtime half now has coverage it never had. Four typecheck gates compiled their own `dist/` alongside source, so they compiled a different program in CI than locally, and `migrate`'s file set depended on CI step order. `wizard` and `examples/basic` had the same defect. The declaration gate reached `dist/` by relative path under bundler resolution, so it never consulted the `exports` map: the `.d.cts` set and `wasm-inline.d.ts` — each with its own inlined copy of the column class — were ungated. The wasm entry is the documented target for Workers, Deno, Bun and Supabase Edge. `__domain` was reachable from consumer code and appeared in autocomplete on every column, typed `D | undefined` and always `undefined`; there is no `stripInternal`, so `@internal` was documentation, not enforcement. Now keyed by a non-exported `unique symbol` — as inferrable, unnameable. New tests --------- - `scripts/lint-typecheck-scope.mjs` + self-tests + CI step: a gated tsconfig must scope away its build output. - `dist-types/node16/`: `.cts`/`.mts`/wasm probes resolving by package name under node16, so the conditions a customer walks are the ones under test. - `turbo-skills-inputs.test.mjs`: a build that copies `skills/` must declare it. - `empty-schemas-boundary.test.ts`, `typed-client-init-wire-version.test.ts`. Left alone ---------- `eqlVersion: 3` over an all-v2 schema set is still accepted, writing v3 payloads into `eql_v2_encrypted` columns — the mirror of what A-8 refuses. It predates this branch and `resolve-eql-version.test.ts:115` pins it deliberately, so closing it is a product decision rather than review remediation.
…exposed The reviewer's two findings were both already fixed at bbade2c — this adds the guards that would have caught them, plus two accuracy fixes. `typecheck:scaffold` declares `dependsOn: ["^build"]`, so turbo has to be the thing invoking it. #787 first added it as a bare `pnpm --filter stash run typecheck:scaffold` and CI went green anyway: an earlier step in the same job happened to build the workspace via its own `^build`. Those earlier steps read as independent guards for other packages, so they look freely removable — delete one and the scaffold step fails `TS2307`, which reads as "the scaffold is broken" rather than "you skipped the build". Nothing caught that. scripts/__tests__/workflow-turbo-build-deps.test.mjs derives the protected set from turbo.json (every task declaring `^build`) rather than hardcoding names, and asserts tests.yml routes them through turbo. Mutation-tested against three regressions: reverting the step to the bare form, deleting the step outright, and adding a new bare build-dependent step — each fails with a message naming the step and the fix. Two pre-existing bare `typecheck` invocations (prisma-next, wizard) carry the same latent trap; they are recorded in KNOWN_BARE rather than silently skipped, and a fourth test fails if an entry goes stale so the list gets worked down instead of accumulating. turbo.json is JSONC, so reading it needs the string-aware comment stripper #782 wrote for `turbo-skills-inputs.test.mjs` — a naive regex eats the `//` in `"$schema": "https://turbo.build/schema.json"`. Rather than hand-copy 30 lines of parser into an adjacent file, that function moves to `scripts/__tests__/lib/read-jsonc.mjs` and both guards import it. Two accuracy fixes: - vitest.config.ts claimed the suite's residual `@cipherstash/stack` coupling runs through `migrate/src/backfill.ts`. That is only one of two routes: `init/lib/__tests__/introspect.test.ts:1` imports `@cipherstash/stack/eql/v3` directly, never touching migrate. As written, someone could decouple backfill.ts and expect the suite to go standalone. It would not. - cli-vitest-alias.test.mjs called `.endsWith()` on every alias value, so Vite's array form (`[{find, replacement}]`) would throw an opaque TypeError from a helper — reading as "the guard is broken" rather than "the config changed shape". It now fails cleanly, naming the offender, and says to update the guard rather than delete it. No changeset: repo tooling and a comment, no published surface touched.
Review remediation for #772, addressing A-4, A-6, A-7, A-8 and C-1 from the verification pass (#778).
Stacked on #780 (
fix/remove-v2-review-remediation), which is itself stacked onremove-v2(#772). Branched at8b3f0b15; the 13 commits here are this PR's own. Design doc:docs/superpowers/specs/2026-07-24-encryption-signature-and-typecheck-gates-design.md.Scope, and what is deliberately out of it
A-6 and A-7 exist because
Encryptionis overloaded, and it is overloaded because there are two clients: the typed EQL v3 one and the nominal one that v2 and loose introspection-derived schemas need. Two overloads means two discriminators that can disagree — overload resolution readsconfig, the runtime readsschemas. Remove EQL v2 and both defects delete themselves.So this does not attempt the type-level repair the verification doc explores (a single generic signature returning
ClientFor<S, C>, a discriminated-unionWireConfig). That machinery exists only to let two clients coexist and would be deleted by the v2 removal that motivates it — it is #637, and it belongs with PRs 9–12. What is here is the subset that is either independent of the removal (A-4, C-1) or cheap and non-throwaway (A-6, A-7).A-8 is a different shape from the rest and is the reason to read this PR as more than type plumbing: it is a guard the v2 removal deleted by accident, restored at the runtime layer.
EncryptionV3was silently correcting a wrong wire format for callers; collapsing it into an alias removed that correction and nothing noticed. It is closed here rather than deferred because the failure it leaves open is a write of v2 payloads intoeql_v3_*columns.A-4 — schema arrays that are not literals
Encryption({ schemas })accepted only an array literal. Every indirect form failedTS2769: a sharedAnyV3Table[]module export,ReadonlyArray<AnyV3Table>(the typeprisma-nextexposes publicly), push-built arrays, spreads, unannotatedconst. Widened to accept any non-empty array.prisma-nextdrops its destructure-and-respread workaround; the runtime emptiness check and its error message stay.A note on the commit tag: A-4's commit is marked
fix(stack)!, and that was accurate when it landed — moving non-emptiness onto the property broke callers generic over their schemas, a shape that compiled on its parent.124048derepaired that by splitting the v3 overload, so the PR's net effect is additive: every form that compiled at the base still compiles. The shipped bump is governed by.changeset/encryption-schema-arrays.md, which says minor — correctly.A-6 — name the client with
EncryptionClientFor<S>ReturnTypereads the last overload — the nominal one — soAwaited<ReturnType<typeof Encryption>>yieldedEncryptionClienteven for an all-v3 schema set, and every assignment of the real client to it failedTS2739. 15 sites carried that error (10 instack, 5 instack-drizzle), none visible to CI because the only typecheck step in either package is scoped to__tests__/**/*.test-d.ts.Overload order cannot fix it — putting the nominal signature first mis-resolves v3 schemas instead, since a v3 table structurally satisfies
BuildableTable— so the client gets a name, which is what comparable libraries do (z.infer, arktype'stypeof T.infer, hono'sClient<T>). The idiom is swept out entirely, not just at the erroring sites.Typing these clients correctly unmasked call sites the wrong type had been hiding. Most were
as nevercasts that are simply no longer needed, so those calls are now genuinely checked. Two are not:encrypt-query-stevecandencrypt-query-searchable-jsonexerciseencryptQueryquery types the typed client mis-models (it derives the plaintext from the column's domain, so every query type on atypes.Json()column is typedJsonDocument, but the SteVec types take a JSONPath string, a{ path, value }pair, or a bare scalar). Those two hold the client through the nominal surface with the gap named at the cast, rather than casting at every call and hiding it.A-7 —
initpassthrough on the typed clientThe two discriminators disagreeing has a concrete crash behind it. Hoisting a config into a
ClientConfig-typed variable is enough to split them —ClientConfig.eqlVersionis2 | 3, which the v3 overload'seqlVersion?: 3rejects — so the call types as the nominalEncryptionClientwhile the runtime still returns the typed one.initwas the only member ofEncryptionClientabsent from the typed client, so anything holding it through its declared type hitTypeError: client.init is not a function. Now present, with the parity asserted so a future member cannot reopen the hole — and pinned toeqlVersion: 3rather than delegated verbatim, because a bare passthrough turned out to be a way around A-8. See the second review pass below.The other half — the silent loss of the typed surface, with no diagnostic — is not closable at the type level (the runtime inspects values, the type inspects an erasable static type). It ends with the v2 removal (#637).
A-8 — refuse EQL v2 wire over an all-v3 schema set
This is the one runtime contract change in the PR.
Encryption({ schemas, config: { eqlVersion: 2 } })now throws at setup when every table inschemasis EQL v3, instead of building a client that writeseql_v2_encryptedpayloads intoeql_v3_*columns.It is the third case of a guard that already existed:
resolveEqlVersionthrew for a mixed v2/v3 set and for legacy v2searchableJson(), but returned an explicit version unchanged.EncryptionV3used to cover this case by forcingeqlVersion: 3over whatever the caller passed — its comment said that was why it existed. CollapsingEncryptionV3into an alias ofEncryptionremoved the override and nothing replaced it, so the combination became silently wrong rather than corrected.Two populations are affected, and one of them is a genuine break:
EncryptionV3({ schemas: [v3Table], config: { eqlVersion: 2 } })worked on releasedmain— the override corrected it to 3. It now throws.Encryption({ schemas: [v3Table], config: { eqlVersion: 2 } })silently built a v2-wire client with no diagnostic at any layer. It now throws.The escape hatch is unchanged where it is actually used: an explicit
eqlVersion: 2over an EQL v2 schema set still emits v2 wire, which is howintegration/shared/v2-decrypt-compat.integration.test.tsmints the fixtures proving v2 payloads still decrypt. Reading v2 payloads is unaffected —decrypt/decryptModelread both generations regardless of the client's wire version.There is no compile-time diagnostic, by construction:
V3ClientConfig.eqlVersionis?: 3, so a config carrying2falls to the nominal overload and the call still type-checks. The refusal is a setup-timeErrorthrown from the factory beforeinit, matching the four sibling throws around it — the{ data } | { failure }Result contract andEncryptionErrorTypesare untouched.Changeset:
reject-v2-wire-over-v3-schemas(stack minor). Covered by negative tests in__tests__/resolve-eql-version.test.tsand, through the public factory,__tests__/init-strategy.test.ts(which also asserts the FFI client is never constructed).C-1 — typecheck the surfaces that compiled nowhere
Four packages had a
tsconfig.jsoncovering src and tests that no CI step ever ran, and were already clean — now gated before they drift:migrate,nextjs,examples/prisma,e2e.packages/nextjsneededvi.Mockimported as a type first.Also declares
outputs: ["dist/**"]on turbo'sbuild. It declared none, so a cache hit replayed logs and restored no files — and theexamples/basicgate added by 5fab1cf typechecks againstpackages/stack/dist/*.d.ts. It held only because fresh CI runners have no cache. Verified by deletingpackages/stack/distand re-running: cache hit, dist restored, still green.Deliberately not gated, with counts recorded in the workflow so the gap is visible rather than silent:
@cipherstash/stack(147),stack-drizzle(63),stash(21),stack-supabase(11). Theirtest:typesscripts are scoped to__tests__/**/*.test-d.ts, so src, the runtime suites andintegration/**compile nowhere. Most of the drizzle and supabase count is a single root cause in@cipherstash/test-kit'sV3_MATRIX.Found along the way: typed
encryptQuerywasneveragainst the built packageclient.encryptQuery(value, { table, column })did not typecheck for any column when imported from the published package — every plaintext resolved tonever. Reproduced identically against the rc.4 tarball on npm, releasedmain@ b48494c,origin/remove-v2and this branch, so it predates the v2 removal and shipped in every release carrying the v3 typed client.PlaintextForColumn/QueryTypesForColumnrecover the domain viaC extends EncryptedV3Column<infer D>, which needsDbare in the instance type. Its only bare site wasprivate readonly definition: D, and tsc strips private member types on declaration emit — leaving nothing TypeScript can invert, soinfer Dfell back to the constraint and cascaded tonever.encryptescaped it by resolving throughColumnsOf.Fixed with a type-only
declare readonly __domain?: D— nothing emitted at runtime, no call-site change, but it survives declaration emit. Addspackages/stack/dist-typesand atest:types:distscript that typechecks the emitted declarations rather than source, wired into CI. That gap is why this survived the whole rc series: every other type test imports@/….A follow-up (74b6755) scopes that gate correctly:
packages/stack's owntsconfig.jsonwas mapping the new directory's@cipherstash/stack/*imports back to source — the exact thing the gate exists to avoid — and the formatter's line-splitting had moved one@ts-expect-erroroff the line its error is reported on, so the gate as first written was red. Both are fixed there, with the red/green re-proved by deleting the phantom field.Review
CodeRabbit reviewed the branch — 6 findings, 0 critical. The two majors were real and are fixed in 58bff6f (the branch has since been rebased; this landed as 35b8858 at review time):
skills/stash-encryptionclaimed reads of the promoted column decrypt transparently afterstash encrypt cutover(true only through CipherStash Proxy, and contradicted by the very next table row), andstash init's setup prompt told agents to declare an EQL v2 cutover target with atypes.*domain (v3-only; a v2 column is aneql_v2_encryptedcomposite). The same commit also took a third finding — a stale runtime-surface doc comment inpackages/stack-supabase/src/types.ts— and the MD040 fenced-block language on the design doc. The two markdownlint findings that remain were skipped deliberately: this repo runs no markdownlint, and both suggestions are wrong under CommonMark (MD038's code span does not begin and end with a space, so no stripping occurs; MD028's blank-separated call-outs already render as the distinct blockquotes they are meant to be).Second review pass — three defects this PR introduced
124048deis remediation for the review of this branch. Six agents cross-checked the findings; three were overturned, and three defects the PR itself had introduced surfaced in the process. Each fix carries a regression test proven to fail without it.initwas a way around the guard A-8 had just added. A-7 handed the typed v3 clientEncryptionClient.init— which takes its owneqlVersion, forwardsundefinedto the FFI (whose default is EQL v2), and never consultsresolveEqlVersion. So a typed v3 client could be re-initialised into v2 wire while keeping its v3 surface:eql_v2_encryptedpayloads intoeql_v3_*columns, the same contradiction refused at construction, one method call later. The parity test could not see it — it stubsinitout and asserts only that it was called.initnow pinseqlVersion: 3, refuses an explicit2with a failureResult(its declared shape), and resolves to the typed client, soclient = (await client.init(cfg)).datadoes not silently drop to nominal.NonEmptyV3<S>broke every caller generic over its schemas — a shape that compiled before A-4, and the one theEncryptionClientFordocs point generic code at. A type parameter is assignable to a deferred conditional only if it is assignable to both branches, and one branch isnever. Deleting the guard is not the fix (overload 1 then accepts[]), so the v3 signature is split in two, with the tuple overload carrying non-emptiness in its constraint, where a genericScan satisfy it.The
outputs: ["dist/**"]declaration added by C-1 could publish stale skills. Declaringdist/**is right forpackages/stack, butstashand@cipherstash/wizardcpSyncthe repo-rootskills/intodist/skills— a directory outside their own input scope. A skills-only edit never invalidated the build, and once outputs were declared a cache hit stopped being merely stale and began actively restoring the previous copy over the tree. Sincestash initinstalls those into customer repos, an edited skill could ship with its pre-edit text while the source on disk was correct and CI green. Reproduced end to end before fixing; both builds now declare$TURBO_ROOT$/skills/**as an input, withscripts/__tests__/turbo-skills-inputs.test.mjspinning it.The findings that stood were narrower. Nine comments described the
eqlVersion: 2escape hatch as still permitted, two of them documenting live guards.skills/stash-encryptionclaimed an empty schema array is a compile error — true only when the argument's type has a statically known length of 0; a spread of an empty array is a literal at the call site and still compiles. And C-1's own gates needed two corrections: four of them (migrate,nextjs,wizard,examples/basic) globbed their package'sdist/into the program, so they compiled a different program in CI than locally andmigrate's file set depended on step order; and the declaration gate reacheddist/by relative path under bundler resolution, never consulting theexportsmap, so the.d.ctsset andwasm-inline.d.ts— each carrying its own inlined copy of the column class theencryptQuerybug was about — were ungated.scripts/lint-typecheck-scope.mjsand a CI step now assert that a gated tsconfig scopes away its build output, anddist-types/node16/resolves@cipherstash/stack/*by package name, so the conditions a customer walks are the ones under test.One tightening rode along: the phantom domain carrier that makes typed
encryptQuerysurvive declaration emit was a plainly named__domainproperty, and this build has nostripInternal, so@internalwas documentation rather than enforcement — it appeared in autocomplete on every column, typedD | undefinedand alwaysundefinedat runtime. It is now keyed by a non-exportedunique symbol: as inferrable, unnameable.Left alone, but not because it is justified.
eqlVersion: 3over an all-v2 schema set is still accepted, writing v3 payloads intoeql_v2_encryptedcolumns — the mirror of what A-8 refuses. Archaeology says this is residue, not design:return explicitwas written unconditionally when the parameter was introduced (ba91f7c8, #547), and every rationale ever attached to it — the original docstring,ClientConfig.eqlVersion's@deprecatednote, all 30 "escape hatch" commits — describes only the2-over-v3 direction. It has no consumer:stack-supabasesynthesises v3 tables so auto-detection already returns 3, andwasm-inline.tsrefuses a v2 table outright rather than pin v3 wire to it. Only two unit assertions exercise the shape. It is also incoherent — v2 authoring cannot emit theopeindex term every v3*Orddomain needs, and eight v3 domain families are unreachable from any v2 builder — so the flag flips the wire format without flipping the index configuration. Closing it is a contract change and belongs to whoever owns the v2 removal, not to this review pass.Changesets
encryption-schema-arrays(stack minor, prisma-next patch),reject-v2-wire-over-v3-schemas(stack minor),typed-client-init-parity(stackpatch),
typed-encrypt-query-dist(stack patch),typecheck-gates-scope-source(stack patch),
domain-carrier-not-public(stack patch),skills-encryption-client-naming(stash patch),cli-v2-cutover-prompt-correction(stash patch),skills-ship-current-copy(stash patch, wizard patch).
Two are edited in place, both because A-8 made their existing text wrong:
stack-audit-on-decrypt(stack major) saidconfig: { eqlVersion: 2 }merelyselects the nominal overload — over an all-v3 schema set it now throws at setup;
and
prisma-next-v3-client-config(prisma-next major) explained the removedeqlVersionfield as a client/type mismatch rather than a combination the clientrefuses outright.