feat(stack): make encryption authoring v3-only - #829
Merged
Conversation
🦋 Changeset detectedLatest commit: c745db7 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 skippedToo many files! This PR contains 159 files, which is 59 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (173)
You can disable this status message by setting the 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 |
tobyhede
added a commit
that referenced
this pull request
Jul 29, 2026
The v3-only fixture migration left __tests__/encrypt-query.test.ts behind: it still asserted EQL v2 payload shape and v2 validation semantics against v3 fixtures, which is the 21-test CI failure on #829. Four distinct causes, only two of which were stale assertions. Fixture defects (these broke the tests' premises, not just their expectations): - articles.content was ported to types.TextSearch, which derives unique + ope + match. `unique` outranks `match` in inferIndexType's priority order, so auto-inference stopped selecting `match` and the numeric-value guard in validateValueIndexCompatibility — which only fires for `match` — went silent. Two tests then failed on a protect-ffi cast error instead of the guard's message, and three more passed for the wrong reason. Restored to types.TextMatch, matching the fixture's documented "only freeTextSearch" contract. - products.price is now types.NumericOrdOre, so the suite keeps live coverage of the block-ORE ordering flavour (`ore`/`ob`) alongside users.age's CLLW-OPE one (`ope`/`op`). Previously both were `_ord` and the ORE path had no live coverage at all. Stale assertions: - v: 2 -> v: 3 (13 sites); authoring is v3-only now. - `ob` -> `op` at the seven users.age sites. `_ord` domains are CLLW-OPE per orderingForEqlType; only `_ord_ore` stays block-ORE. Obsolete test: - "provides descriptive error for queryType mismatch" asserted that equality against an order-only column throws. That is v2 behaviour: a v3 order-capable column answers equality through its ordering term (equalityOrderingIndex), which is pre-existing, deliberate, and already regression-tested in schema-v3.test.ts — including the v2-preservation case. Re-pointed at articles.content, which genuinely cannot answer equality, so the test keeps its intent instead of being deleted. New coverage: - fixtures-query-contract.test.ts pins every shared fixture to the index it resolves to. Credential-free, so a fixture that drifts fails on the PR that drifts it rather than on the first CI run holding credentials. This is the test that would have caught all of the above. - The live suite now asserts equality-through-the-ordering-term on users.age (with `hm` ABSENT — the load-bearing half) and the block-ORE term on products.price via the explicit-queryType path, where the ore->ope swap must not fire. - wasm-inline-query.test.ts gains an _ord_ore column covering the `ore` branches of that swap and of equalityOrderingIndex, neither of which had credential-free coverage. Why this reached CI: the suite had no credential gate, so a local run died in beforeAll and reported "Failed Suites 1 / Tests 44 skipped" — indistinguishable from ordinary credential gating. It now skips cleanly without credentials but FAILS when CI is set, so credential rot cannot leave the suite silently covering nothing while the build stays green. No production code changed.
tobyhede
added a commit
that referenced
this pull request
Jul 29, 2026
…ewing #829 A review of the PR — three agents over the diff, then two verifying the findings — turned up defects in both commits, including three in the review-fix commit itself. Each was reproduced before being acted on. **A type-breaking change was shipped as a patch.** Splitting `decryptModel` / `bulkDecryptModels` into arity-2 and arity-3 overloads dropped `undefined` from the third parameter, so `decryptModel(row, users, session?.lockContext)` — the ordinary shape for decrypting identity-bound rows only for signed-in users — stopped compiling. Proven against the built `.d.ts`, not just source. The positional overloads now take `LockContextInput | undefined`; `undefined` binds nothing, and binding twice is still a compile error. **Two tests were left type-broken by that same split**, with comments asserting the opposite of the new types, and CI could not see it: `vitest.config.ts` scopes typecheck to `*.test-d.ts`. They now model a plain-JavaScript caller explicitly — the runtime throw is the backstop for callers who never see these types, and chaining directly no longer compiles. **The exported wasm config type laundered an empty schema set.** Widening `WasmEncryptionConfig.schemas` to `readonly AnyV3Table[]` also made it unable to prove non-emptiness: `const cfg: WasmEncryptionConfig = { schemas: [], config }` compiled, and so did `Encryption(cfg)`, because `NonEmptyV3<readonly AnyV3Table[]>` resolves `S['length']` to `number` rather than `0`. Only the direct literal was rejected. Non-emptiness moves back onto the exported type, with the loose shape kept as an unexported implementation type; the factory still accepts widened arrays inline, which is what A-4 was about. **Nested EQL v2 DynamoDB reads returned raw base64 as a success.** A v2 grouped column registered its build key on the bare leaf, so a field in a group was written `<group>.<leaf>__source` while the schema knew it only as `<leaf>`. The v3 rewrite made matching exact-dotted-path only, for both generations, orphaning every such attribute behind a debug log that is invisible at the default level. The fallback is restored for `storedEqlVersion: 2` reads only — v3 registers full dotted paths precisely so a nested leaf cannot collide with a top-level column, and matching by bare leaf there once rewrote a plaintext sibling as an envelope and handed it to the FFI. **Tests that could not fail.** The two wasm-shaped DynamoDB cases set a capability flag nothing reads, making them byte-equivalent to the pair above them; their stub now throws when called table-less, verified by breaking the forwarding and watching three tests fail. A Drizzle type test asserted the same thing twice (`EncryptionClient<readonly AnyV3Table[]>` is definitionally the defaulted `EncryptionClient`) and now also covers a concrete tuple. A Supabase `@ts-expect-error` had degenerated to proving "an arbitrary object is rejected"; its stub is v2-table-shaped again, and the rejection now cites the v3 table's discriminating members. **Docs that overclaimed.** A shipped JSDoc and its changeset both said an adapter can name `EncryptionClient<readonly AnyV3Table[]>` and "keep the typed surface". This PR's own Supabase adapter carries a cast disproving it: with the schema parameter loose there is no per-column plaintext to resolve, so the model input stays untyped. Both now say so. `AGENTS.md` credited v2 decrypt to the native client alone, which stopped being true when the wasm entry gained it. Two findings were deliberately not acted on. The `assertClientTableVersionMatch` early return was settled by 8de757f with a second guard and a pinning test. Failing closed on an unmatched `*__source` would regress the passthrough that `helpers-v3.test.ts` pins on purpose — treating one as an envelope previously handed unrelated customer plaintext to the FFI. One instruction to a delegated agent was itself wrong: that a nominal `EncryptionClient<S>` parameter would reject a narrower schema tuple. Its members are declared method-style, so the instantiations relate bivariantly and both directions are accepted. The agent tested it, refused to write the false comment, and wrote the verified reason instead — that naming a ten-member interface nominally rejects anything supplying only the consumed capability. Verification: 760 stack unit tests, 51 type tests, dist declaration gate exit 0, 138 scripts tests, Supabase 489, Drizzle 371, Prisma Next 344, Biome error-free. The 11 failing stack files are 10 credential-gated suites and the pre-existing Clerk dependency-resolution issue this PR's description already notes.
Six review agents checked the branch against the issue's acceptance criteria.
AC2, AC3, AC6 and AC7 held; AC1, AC4 and AC5 had gaps. Each finding below was
independently verified before being acted on, and each fix landed test-first.
**AC4 — v2 decrypt was not preserved on the edge entry.**
`encryptedDynamoDB` refused a `{ storedEqlVersion: 2 }` read on a wasm-inline
client, so Deno/Bun/Workers/Supabase Edge customers could not read pre-migration
items at all, with no workaround. The refusal's stated rationale — "the v2 read
deliberately omits the table" — was false: `decrypt-model.ts` passes `this.table`
on that path, and `v2-table-forwarding.test.ts` already pinned it. `git show
a9d430b` shows the guard was written when the branch was `if (!isV3Table(table))`
and a v2 table object still existed; the v3-only rewrite removed the omission and
left the guard behind.
Both bindings are builds of the same protect-ffi crate, whose `decrypt` accepts
either wire generation regardless of the client's `eqlVersion` — confirmed in the
package README and by finding the v2 deserializer strings in both `index.node`
and `protect_ffi_bg.wasm`. The refusal is lifted, and a live wasm v2 read suite
now proves it in CI (no local credentials here, so that is where the executable
proof runs).
**AC5 — two types disagreed with their runtimes.**
- `wasm-inline.ts` still typed `schemas` as a mutable non-empty tuple long after
A-4 widened the native twin, so a `.map()` result or a `readonly` array
compiled against one entry and failed against the other while both runtimes
checked only `schemas.length`. Ported the native overload pair over.
- `decryptModel(row, table, lockContext).withLockContext(lockContext)`
type-checked and then threw. Split the operation interface the way the encrypt
path already does: binding returns `LockBoundDecryptModelOperation`, which
carries `.audit()` but not `.withLockContext()`. The runtime throw stays as the
plain-JavaScript backstop.
The dist declaration gate covered `encryptQuery` narrowing only, which is exactly
why the wasm drift survived — that entry gets its own tsup DTS pass no
source-level type test can see. It now covers schema-array shapes and config
typing on both entries. Verified it has teeth: reverting the wasm fix and
rebuilding makes it fail with TS2322/TS4104 against the built `.d.ts`.
**AC1 — coverage gaps.**
- No test covered the state a real migration leaves behind: one model carrying
both generations. Every existing case was all-v2, true only until the first v3
write.
- `packages/stack/src/dynamodb/**` appeared in no workflow `paths:` filter
(`grep -rn dynamodb .github/` returned nothing), so edits to the only live EQL
v2 read path — or a protect-ffi bump breaking v2 deserialization — never
triggered the suite covering them. `src/index.ts` and `src/types.ts` were
missing too. Pinned by a test that derives the requirement from what the
suites actually import, rather than hardcoding today's answer.
**AC6 — the removal was sound but unpinned.**
The stale-reconstructor-map hazard is closed structurally: config and
reconstructors derive from the same tuple in the same call, and no re-init
survives. Nothing asserted that, and the two tests that had exercised it were
deleted in a3830f0 — so re-adding an `init` passthrough would have failed no
test. Pinned, and verified the pin fails when `init` is reintroduced.
**Shipped guidance that was wrong.**
`skills/stash-indexing` told agents to use `types.TOrd`, which does not exist
(the table above it uses `types.NOrd` / `types.TextOrd`). Two skills stated v2
DynamoDB reads need the native entry. 23 TSDoc `@example` blocks across
`encryption/index.ts` and `schema/internal.ts` told readers to import removed
builders from `@cipherstash/stack/schema`; none would compile. A user-facing
error named `createEncryptionClient`, which users cannot import. Comments in
three files still cited `resolveEqlVersion` and `ClientConfig.eqlVersion`, both
removed.
Verification: `code:check` error-free; 744 stack unit tests pass (the 12 failing
files are the pre-existing credential-gated suites, confirmed identical at
baseline); 43 type tests; dist gate exit 0; 133 scripts tests; stack-drizzle,
stack-supabase and prisma-next all green.
The v3-only fixture migration left __tests__/encrypt-query.test.ts behind: it still asserted EQL v2 payload shape and v2 validation semantics against v3 fixtures, which is the 21-test CI failure on #829. Four distinct causes, only two of which were stale assertions. Fixture defects (these broke the tests' premises, not just their expectations): - articles.content was ported to types.TextSearch, which derives unique + ope + match. `unique` outranks `match` in inferIndexType's priority order, so auto-inference stopped selecting `match` and the numeric-value guard in validateValueIndexCompatibility — which only fires for `match` — went silent. Two tests then failed on a protect-ffi cast error instead of the guard's message, and three more passed for the wrong reason. Restored to types.TextMatch, matching the fixture's documented "only freeTextSearch" contract. - products.price is now types.NumericOrdOre, so the suite keeps live coverage of the block-ORE ordering flavour (`ore`/`ob`) alongside users.age's CLLW-OPE one (`ope`/`op`). Previously both were `_ord` and the ORE path had no live coverage at all. Stale assertions: - v: 2 -> v: 3 (13 sites); authoring is v3-only now. - `ob` -> `op` at the seven users.age sites. `_ord` domains are CLLW-OPE per orderingForEqlType; only `_ord_ore` stays block-ORE. Obsolete test: - "provides descriptive error for queryType mismatch" asserted that equality against an order-only column throws. That is v2 behaviour: a v3 order-capable column answers equality through its ordering term (equalityOrderingIndex), which is pre-existing, deliberate, and already regression-tested in schema-v3.test.ts — including the v2-preservation case. Re-pointed at articles.content, which genuinely cannot answer equality, so the test keeps its intent instead of being deleted. New coverage: - fixtures-query-contract.test.ts pins every shared fixture to the index it resolves to. Credential-free, so a fixture that drifts fails on the PR that drifts it rather than on the first CI run holding credentials. This is the test that would have caught all of the above. - The live suite now asserts equality-through-the-ordering-term on users.age (with `hm` ABSENT — the load-bearing half) and the block-ORE term on products.price via the explicit-queryType path, where the ore->ope swap must not fire. - wasm-inline-query.test.ts gains an _ord_ore column covering the `ore` branches of that swap and of equalityOrderingIndex, neither of which had credential-free coverage. Why this reached CI: the suite had no credential gate, so a local run died in beforeAll and reported "Failed Suites 1 / Tests 44 skipped" — indistinguishable from ordinary credential gating. It now skips cleanly without credentials but FAILS when CI is set, so credential rot cannot leave the suite silently covering nothing while the build stays green. No production code changed.
… criteria Reviewing the v3-only change against #815's acceptance criteria surfaced six gaps. All are verified against HEAD; each fix landed test-first. Config typing disagreed with runtime on both entries. `ClientConfig` and `WasmClientConfig` are all-optional, so excess-property checking was the only thing catching a leftover `config.eqlVersion` — and that fires on fresh object literals alone. A shared config const, the shape a v2 -> v3 migration actually holds, type-checked clean and threw at `Encryption()`. Both now declare `eqlVersion?: never`, pinned by hoisted-config directives in the dist gates. The runtime guards stay: JS and JSON callers bypass types entirely. The entries still disagreed about `config.eqlVersion` itself — native threw, wasm-inline accepted it silently. wasm-inline now mirrors the native guard byte-for-byte, which is the disagreement #815 exists to remove. Its non-v3-table error also stopped referring readers to the native entry for v2 authoring; that entry rejects it too, so the referral only bought a second rejection. Restore the `#1c` case the old suite explicitly asked to keep: v2 payloads read through a client configured for an unrelated table, proving decrypt never consults the encrypt config. Deleting it alongside the `eqlVersion: 2` minting hatch removed the detector for the invariant customers depend on. Fixtures still mint through protect-ffi directly, not the removed public API. Also drop two `id` fields from an `ffiEncryptBulk` call — protect-ffi's `EncryptPayload` has no such field and correlates positionally. Both integration workflows claimed a protect-ffi bump must run them while listing neither manifest that pins it. Add `packages/stack/package.json` and `pnpm-workspace.yaml` (which carries the lockstep `@cipherstash/auth` catalog pin) to all four filters; the lockfile stays out, since an exact pin cannot reach it without a manifest edit first, and both comments now say so. The guard discovers workflows by `CS_IT_SUITE` instead of hardcoding two names, and fails a one-sided edit of the duplicated push/pull_request blocks. Restore the `./types` nameability guard deleted with no replacement, and pin the v2 names out of it. Assert `root` lacks `encryptedTable` — the name the removed export line actually exported, which no test covered. Replace the re-init denylist with exact key equality against the client's ten members, so a future `withSchemas()` cannot slip past a four-name list. Correct docs the change invalidated: the README's `Encryption` sketch understated the accepted schema shapes, the encryption skill dropped two still-shipping subpaths, and `cipherstashFromStack`'s JSDoc described `config.eqlVersion` as a conditional escape hatch rather than an unconditional rejection.
"executes a single/bulk query bound to an identity claim" were the only tests in the repo that executed a lock-context ZeroKMS call under CI's service credentials. On the EQL v3 wire they fail with `ZeroKMS error 'Request forbidden due to insufficient permissions'`. They are invalid by construction, and have never asserted anything about lock contexts. Why they cannot be valid: a lock context binds a data key to an end user's identity claim, so it requires an OidcFederationStrategy-authenticated client. `skills/stash-encryption/SKILL.md` (added in 8bb698e) states that AccessKeyStrategy "authenticates a *service* and cannot be used with a lock context", and the same rule is in LockContext's TSDoc. This suite builds its client from the CS_* service credentials with no authStrategy, so the claim has no JWT to resolve from. The 403 is the correct response. Why they were never meaningful: - Introduced in fea303d with a mock that forwarded a literal `accessToken: 'mock-token'` to the live service as `serviceToken` — and passed. Had ZeroKMS been authorising the binding, that string would have been rejected. - Their assertions (`i`, `v`, `hm`, `op`) are identical to the non-lock-context tests earlier in the same file, so neither could distinguish a bound claim from an ignored one. - Per 8d707cc ("search terms are not identity-bound — decryption is the boundary"), query terms are not identity-bound at all: the `hm` term is workspace-scoped and matches with or without a lock context. There is no behaviour at this layer for them to assert. Deleting rather than gating or moving: - Gating on USER_JWT is not available: that secret was never provisioned (#530) and f143dfe retired it for CLERK_MACHINE_TOKEN, wired only into the integration workflows. - Moving them to integration/identity/ would add nothing — per 8d707cc a query-term assertion there is shape-only, and the decrypt-boundary coverage that matters already exists in integration/identity/matrix-identity.integration.test.ts and packages/stack-drizzle/integration/lock-context.integration.test.ts, both green on this branch via "Integration — Drizzle (EQL v3)". - No coverage is lost here: the two sibling tests still pin that `.withLockContext()` returns an executable operation, and lock-context-wiring.test.ts pins claim forwarding against a mocked FFI. Also stops what protect-ffi's README describes as invalid lock contexts firing security warnings in ZeroKMS on every CI run. Left open, tracked separately: nothing in this repo explains why the v3 wire reaches an authorization check the v2 wire did not. Answering that needs protect-ffi/ZeroKMS, and it bears on what v2 lock contexts on query terms actually guaranteed.
…sserting past it The restored `#1c` block added a DynamoDB case reading through a client that never registered the table, and it fails: the adapter forwards the table into `client.decryptModel(item, table)` — deliberately, to preserve Date reconstruction — and the table-aware overload rejects a table outside the schema tuple. Two independent registration checks sit on that path, and clearing the first does not clear the second. `assertClientTableVersionMatch` early-returns for `storedEqlVersion: 2` because a v2 payload says nothing about which v3 tables the client holds; the guard in `client-v3.ts` still fires. The restored case mistook the first for the only one. The pre-#815 case passed a v2 table descriptor and so never reached the v3 guard. That shape is unreproducible now the v2 builders are gone, and requiring the table to be declared is a coherent contract — the caller needs a descriptor to pass anyway. So the case is inverted to pin the boundary rather than deleted, and the file header now scopes the config-independence invariant to the table-less reads that actually hold it. The native and WASM cases through the registered client are untouched and still passing. `skills/stash-dynamodb` documented the descriptor a legacy read takes but not that it must be one of the tables given to `Encryption({ schemas })` — stated now, where the signature is.
…ewing #829 A review of the PR — three agents over the diff, then two verifying the findings — turned up defects in both commits, including three in the review-fix commit itself. Each was reproduced before being acted on. **A type-breaking change was shipped as a patch.** Splitting `decryptModel` / `bulkDecryptModels` into arity-2 and arity-3 overloads dropped `undefined` from the third parameter, so `decryptModel(row, users, session?.lockContext)` — the ordinary shape for decrypting identity-bound rows only for signed-in users — stopped compiling. Proven against the built `.d.ts`, not just source. The positional overloads now take `LockContextInput | undefined`; `undefined` binds nothing, and binding twice is still a compile error. **Two tests were left type-broken by that same split**, with comments asserting the opposite of the new types, and CI could not see it: `vitest.config.ts` scopes typecheck to `*.test-d.ts`. They now model a plain-JavaScript caller explicitly — the runtime throw is the backstop for callers who never see these types, and chaining directly no longer compiles. **The exported wasm config type laundered an empty schema set.** Widening `WasmEncryptionConfig.schemas` to `readonly AnyV3Table[]` also made it unable to prove non-emptiness: `const cfg: WasmEncryptionConfig = { schemas: [], config }` compiled, and so did `Encryption(cfg)`, because `NonEmptyV3<readonly AnyV3Table[]>` resolves `S['length']` to `number` rather than `0`. Only the direct literal was rejected. Non-emptiness moves back onto the exported type, with the loose shape kept as an unexported implementation type; the factory still accepts widened arrays inline, which is what A-4 was about. **Nested EQL v2 DynamoDB reads returned raw base64 as a success.** A v2 grouped column registered its build key on the bare leaf, so a field in a group was written `<group>.<leaf>__source` while the schema knew it only as `<leaf>`. The v3 rewrite made matching exact-dotted-path only, for both generations, orphaning every such attribute behind a debug log that is invisible at the default level. The fallback is restored for `storedEqlVersion: 2` reads only — v3 registers full dotted paths precisely so a nested leaf cannot collide with a top-level column, and matching by bare leaf there once rewrote a plaintext sibling as an envelope and handed it to the FFI. **Tests that could not fail.** The two wasm-shaped DynamoDB cases set a capability flag nothing reads, making them byte-equivalent to the pair above them; their stub now throws when called table-less, verified by breaking the forwarding and watching three tests fail. A Drizzle type test asserted the same thing twice (`EncryptionClient<readonly AnyV3Table[]>` is definitionally the defaulted `EncryptionClient`) and now also covers a concrete tuple. A Supabase `@ts-expect-error` had degenerated to proving "an arbitrary object is rejected"; its stub is v2-table-shaped again, and the rejection now cites the v3 table's discriminating members. **Docs that overclaimed.** A shipped JSDoc and its changeset both said an adapter can name `EncryptionClient<readonly AnyV3Table[]>` and "keep the typed surface". This PR's own Supabase adapter carries a cast disproving it: with the schema parameter loose there is no per-column plaintext to resolve, so the model input stays untyped. Both now say so. `AGENTS.md` credited v2 decrypt to the native client alone, which stopped being true when the wasm entry gained it. Two findings were deliberately not acted on. The `assertClientTableVersionMatch` early return was settled by 8de757f with a second guard and a pinning test. Failing closed on an unmatched `*__source` would regress the passthrough that `helpers-v3.test.ts` pins on purpose — treating one as an envelope previously handed unrelated customer plaintext to the FFI. One instruction to a delegated agent was itself wrong: that a nominal `EncryptionClient<S>` parameter would reject a narrower schema tuple. Its members are declared method-style, so the instantiations relate bivariantly and both directions are accepted. The agent tested it, refused to write the false comment, and wrote the verified reason instead — that naming a ten-member interface nominally rejects anything supplying only the consumed capability. Verification: 760 stack unit tests, 51 type tests, dist declaration gate exit 0, 138 scripts tests, Supabase 489, Drizzle 371, Prisma Next 344, Biome error-free. The 11 failing stack files are 10 credential-gated suites and the pre-existing Clerk dependency-resolution issue this PR's description already notes.
tobyhede
force-pushed
the
fix/issue-815-stack-v3-only
branch
from
July 29, 2026 07:02
991a9fc to
90c3873
Compare
The `Analyze v3 complexity` job failed: `query-encrypt.ts` scored 71.51 against the FTA cap of 71. 3aff6cb added 35 lines to the file, and a3d6abb had just ratcheted the cap 91 -> 71, so the file crossed a line the repo had only recently tightened. Raising the cap would undo that ratchet, so the file is split instead. `encryptFilterValues` was doing two unrelated jobs: a pure synchronous walk over `DbQuerySpace` deciding WHICH operands need encrypting, then the async FFI work of encrypting them. The walk moves to `./query-terms` — `collectQueryTerms`, `TermMapping`, `CollectedQueryTerm`, `queryTypeForRawOp`, `queryTypeForOrOp`, `assertEncryptedSearchOperand` — and `encryptFilterValues` becomes the three lines that join the halves. The name follows the existing `query-dbspace` / `query-filters` / `query-results` convention. Capability validation deliberately does NOT move: `assertTermQueryable` stays the single boundary for it, so every collected spelling is still checked identically rather than once per loop. Also updated: the module map in `query-builder.ts`'s header, and two `{@link}`s in `assertTermQueryable`'s doc that pointed at the moved functions. No changeset — `dist/index.d.ts` contains none of the moved symbols, so nothing here was ever public and behaviour is unchanged. Verification: all three complexity gates exit 0 (query-encrypt.ts 71.51 -> 60.60, new query-terms.ts 59.07 — both clear of the cap rather than back on it); 536 Supabase tests pass; build clean including DTS; Biome error-free. The 11 `tsc --noEmit` errors in `__tests__/supabase-v3-matrix.test.ts` are pre-existing — identical count on the unmodified tree — and invisible to CI because `vitest.config.ts` scopes typecheck to `*.test-d.ts`.
tobyhede
marked this pull request as ready for review
July 29, 2026 07:18
coderdan
self-requested a review
July 29, 2026 07:37
Contributor
|
The patch introduces two public type/runtime mismatches and returns the wrong plaintext type for some supported legacy DynamoDB reads.
|
coderdan
approved these changes
Jul 29, 2026
coderdan
left a comment
coderdan
left a comment
Contributor
There was a problem hiding this comment.
No blocking findings, but the P2s should be addressed before merging.
Reviewing the branch turned up seven sites where the `EncryptionV3` ->
`Encryption` and `TypedEncryptionClient` -> `NativeEncryptionClient` rename was
applied to prose that no longer parses, one orphaned doc block, one broken
shipped example, and a dead re-export that made two modules cyclic at runtime.
Each was confirmed against the code before being changed. No behaviour changes.
**The `./index` <-> `./client-v3` cycle is removed.** `client-v3.ts` imported
`Encryption` as a VALUE solely to feed a trailing `export { Encryption }` that
nothing imports — `encryption/v3.ts` re-exports it straight from `./index`.
Since `./index` imports `createEncryptionClient` as a value, that made the pair
genuinely cyclic at module eval. The whole `./index` import is now type-only,
so values flow one way. Verified acyclic rather than assumed: `client-v3`'s
remaining value imports are `@/eql/v3/{columns,date-reconstruction}`,
`@/errors` and `./operations/mapped-decrypt`, none of which reach back into
`@/encryption`.
The six-line comment in `index.ts` arguing the cycle was safe via function
hoisting goes with it. It was stale independently: `git log -S` traces it to
8832d35, when this file was `./v3` and contained `export const EncryptionV3 =
Encryption` — the module-eval assignment the argument was actually about, and
which no longer exists.
**The orphaned doc block is reattached.** ~30 lines documenting the deleted
`EncryptionClientFor` alias sat above the file's re-exports, documenting
nothing and invisible in the generated reference. Merged into the exported
`EncryptionClient` interface — the loose-`S` model-typing caveat and the
`Awaited<ReturnType<typeof Encryption>>` warning are the guidance callers most
need and were reachable by nobody.
**Two comments say something FALSE, not merely stale**, so they are rewritten
rather than renamed:
- `dynamodb/index.ts` claimed "both client shapes DO expose
`getEncryptConfig()`". `WasmEncryptionClient` has none — zero occurrences in
`wasm-inline.ts` — which is precisely the unreadable-config case the guard
early-returns on. Naming it makes the silence deliberate rather than luck.
- `dynamodb/types.ts` described "both NATIVE clients", their `DecryptModelOperation`
and `MappedDecryptOperation` as alternatives. There is one native client, and
its decrypt-model path returns a `MappedDecryptOperation` WRAPPING a
`DecryptModelOperation`.
**`skills/stash-dynamodb` shipped a destroyed sentence** — "whether the client
came from `Encryption` or the `Encryption` client". Line 491 of the same file
states the fact correctly; the changeset dynamodb-skill-wasm-caveat.md shows a
prior PR fixed that one and missed this. Now consistent, and it agrees with the
source comment in `dynamodb/types.ts` that wasm-inline drops audit metadata.
This ships into customer repos.
**The `encryptedDynamoDB` v2 example was not runnable**: an edit removed the
table and client construction but kept the imports, leaving `dynamo`, `users`
and `storedItem` undeclared and both imports unused. Rewritten complete, and
cross-checked against the skill.
Also: the `NativeEncryptionClient` name was a non-exported class in `index.ts`
AND an unrelated structural type in `client-v3.ts`. The type — 3 references, no
public surface — is now `UnderlyingNativeClient`, named for its role. The
"single import surface" comment moved to `encryption/v3.ts`, the entry it
describes. The one-arg `decryptModel` rationale was reworded, not dropped: it
still explains the legacy EQL v2 read path, and regains the provenance
("the arity the pre-v3 client exposed") that "mirroring the nominal client"
carried.
Two tests imported `encryptedTable`/`types` from the internal
`@/encryption/client-v3`, which is what kept its `export * from '@/eql/v3'`
alive; they now take them from `@/eql/v3`. `v3-only-public-surface.test.ts`
asserts nothing about `client-v3`'s own surface, so this does not weaken it.
No changeset: v3-only-encryption-client.md and
v3-only-config-typing-and-entry-parity.md already carry `stash: patch` and both
cover the bundled skills — the latter has a paragraph on stash-dynamodb — so
the bump and changelog entry exist. Nothing else here is public behaviour.
Verification: Biome, `@cipherstash/stack` build, test:types and test:types:dist
all exit 0. `packages/stack` vitest is 764 passed / 157 skipped / 11 failed —
the same 11 by cause as the unmodified tree (10 missing local credentials, 1
unresolvable `@clerk/shared/telemetry`), so no new failure. `pnpm run build`
fails at `@cipherstash/bench` both with and without these changes:
`packages/bench/node_modules` symlinks into another worktree, so bench resolves
`@cipherstash/stack` to a stale dist still exporting `EncryptionClientFor`,
which appears nowhere in this checkout.
v2 read coverage was broad across surfaces — scalar, model, bulk, mixed
v2/v3, unregistered-table — but text-only across types: every v2 fixture
in the repo used `types.TextEq`. Writes are proven for all 40 domains by
the v3 matrix; reads had exactly one.
Add an entry-agnostic v2 fixture plan (`packages/test-kit/src/v2-fixtures.ts`)
driven from `V3_MATRIX`, and consume it from all three surfaces that read
legacy payloads:
- native (`integration/shared`): 101 cases, 29 domains, 6 plaintext axes
- wasm-inline (`integration/wasm`): the same matrix plus a scalar bigint
pin — wasm reconstructs per value via `reconstructDateValue`, a separate
implementation from native's path-aware `reconstructDatePaths`, so it
needs its own proof rather than inheriting native's
- DynamoDB (`__tests__/dynamodb`): 116 cases including nested dotted-path
columns; unit-testable with a stub at the protect-ffi boundary, so this
surface runs without credentials
Every case asserts `{ v: 2, i: { t, c } }` on the fixture before asserting
any value: without that the matrices would pass just as happily against v3
payloads and prove nothing about legacy reads. The identifier is checked
too, since `encryptBulk` correlates positionally and a mis-zipped batch
would otherwise go green by coincidence.
29 of 40 domains are minted. The 11 deferrals carry written reasons: the
10 ope-indexed domains (a v2 payload from an ope column is data that never
existed — the removed v2 builder's `orderAndRange()` emitted `ore`) and
`json_search` (cipherstash-client 0.42 refuses to emit a v2 ste_vec, which
also makes `json` the one unreachable cast_as). No plaintext axis is lost:
each deferred domain shares its cast_as with a covered sibling.
The accounting guards live in `packages/stack/__tests__/test-kit-v2-fixtures.test.ts`
rather than only inside the integration suite that consumes the plan. They
are pure functions over `V3_MATRIX`, and a guard whose purpose is to fire
when someone adds a domain is worth little if it only fires for the subset
of contributors holding integration credentials.
Characterises one genuine defect found on the way: a v2 GROUPED date column
decrypts but is never reconstructed. `makeColumnMatcher`'s bare-leaf
fallback rebuilds the envelope at the nested position while
`rowReconstructor` keys date paths on the declared path, so
`reconstructDatePaths` finds nothing and the value returns as the FFI's
string — no throw, no data loss, simply not a `Date`. Pinned as
characterisation, not endorsement, with the caller-side workaround
(declare the dotted path) documented. Not fixed here.
Test-only; no changeset (`@cipherstash/test-kit` is private).
…eview
Grouped v2 date columns lost their `Date`. A legacy grouped field is stored as
`<group>.<leaf>__source` while the v2 schema knew it only as `<leaf>`, so a
`{ storedEqlVersion: 2 }` read matches it by bare leaf and the recursion re-nests
the rebuilt envelope, landing the plaintext at `details.placedAt`. Both clients
resolve date columns from the DECLARED paths — `rowReconstructor` on the native
entry, `dateFields` on wasm-inline — so neither found it and the value came back
as an ISO string, contradicting the guarantee stated in `dynamodb/index.ts` and
the DynamoDB skill.
Fixed in the adapter rather than either client: the read path is the only layer
that knows the alias fired, and reconstructing on the returned plaintext covers
both entries at once. `toItemWithEqlPayloads` reports the paths it actually wrote
to, and the decrypt operations reconstruct there — one set PER ITEM on the bulk
path, since `details.placedAt` can be an encrypted date column in one item and an
ordinary plaintext string in the next. The characterisation block in
`v2-type-coverage.test.ts` that pinned this as known behaviour now asserts the
fix.
`EncryptionClientConfig` stopped rejecting an empty schema set. Its default type
argument widened to `readonly AnyV3Table[]`, where `S['length']` resolves to
`number` and the non-empty conditional no longer fires, so a config built once
and passed around — the shape the type exists to serve — typechecked clean and
threw at `Encryption()`. Restores the non-empty tuple default, matching the fix
`90c38735` already applied to `WasmEncryptionConfig`, and gates it against the
BUILT declarations as well as source.
`config.eqlVersion: undefined` typechecked but threw. `eqlVersion?: never` has
declared type `undefined` without `exactOptionalPropertyTypes` (not enabled in
this repo) and no declaration can reject it, so a bare `Object.hasOwn` presence
check failed a config the published types accept. Both factories now tolerate
that one value — it names no version — and still reject every real one,
`eqlVersion: 3` included. Corrects the prisma-next comment that restated the
old "whatever its value" rule.
This was referenced Jul 29, 2026
Open
tobyhede
added a commit
that referenced
this pull request
Jul 30, 2026
Every item here has the same root cause: a change updated one copy of something and left the other. Closes #837. **`stash-encryption` contradicted itself in the customer's repo.** `skills/stash-encryption/SKILL.md:12` pointed at an EQL v2 schema surface "with chainable capability builders" that "still exists", while `:201` and `:1015` of the same file said the v2 builders are gone and v2 is a decrypt-only read path — which is what the code does after #829. The opening callout now says that. It is the first thing an agent reads, and `SKILL_MAP.drizzle` installs this skill into every Drizzle project. **`db push` residue.** `stash db push` was retired with the Proxy lifecycle (#814 / #825) and `post-agent.ts` says it "must never run", but the "Post-agent steps complete" changelog line still claimed it had, and three tests used it as their example of an allowed `stash db` command. The tests now use `db validate`, which is in the manifest. Also dropped "no db pushes" from the `--plan` help text. The `'stash db'` allowlist prefix is unchanged — it still serves `db validate`, `db migrate` and `db test-connection`. **Wizard sweep reporting gap.** When a directory's sweep threw, the `continue` in `rewriteEncryptedMigrations` skipped the per-directory report, so files it had already rewritten on disk went unnamed — `sweepMigrationDirs` propagates that partial set precisely so the caller can report it, and the CLI twin does. Now reported as "Rewrote N migration file(s) in <dir>/ before the sweep stopped", with the file list. Behaviour-neutral otherwise: the SQL is additive and the wizard still throws before the migrate prompt. Two tests pin it, including one that the clean path does not borrow the partial wording; the first fails without the fix. Dropped in the rebase: the `stash-drizzle` add-only correction and the CLI rewriter test assertion this commit originally carried both landed on main independently (91452c1), so the tree already has them. The `stash-drizzle` bullet is gone from the changeset for the same reason. The `err as Partial<RewriteSweepError>` cast at `rewrite-migrations.ts:946` is deliberately untouched — it is a fail-closed correctness defect tracked as item 3 of #836, not cleanup.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #815.
Based on
main. Theremove-v2base this branch was originally stacked on (3a6395c) has since merged, so the eleven commits below are the whole of this PR.Summary:
Encryptionthe sole native factory and always authors EQL v3EncryptionClient<S>Can a v3 client read existing v2 data?
Yes — but "read" means decrypt, not query, and it differs by surface. This is the question the previous description left ambiguous.
decrypt/decryptModel/bulkDecrypt){ storedEqlVersion: 2 }. Unlike the core path, the table must be declared inEncryption({ schemas }).public.eql_v3_*domains only, so aneql_v2_encryptedcolumn never enters the encrypt config and is returned as an untouched passthrough. Nothing is stranded — hand it to the core client — but the adapter will not do it for you.encryptQueryon v2 dataCore decrypt is generation-agnostic because
isEncryptedPayloadselects fields structurally and protect-ffi'sdecryptaccepts either wire generation regardless of the client's own mode — nothing consultsi.t/i.c. That is pinned byintegration/shared/v2-decrypt-compat.integration.test.tsand itsintegration/wasm/twin, which mint fixtures directly with protect-ffi in v2 mode and read them back through a client configured for an unrelated table, with a precondition test that fails if that client ever picks up the real one. Both suites run in the Drizzle integration job.The DynamoDB difference is deliberate and pinned by
refuses a stored v2 DynamoDB item whose table the client never registered: the adapter always forwards a table, and the table-aware overload does consult the schema tuple. This PR also fixed a real defect on that path — a nested v2 grouped column registered its build key on the bare leaf, so the v3 exact-dotted-path rewrite orphaned those attributes and returned raw base64 as a success.v2 read coverage spans the type catalog
The v2 read suites originally proved one type: every fixture used
types.TextEq. Writes were proven across all 40 domains by the v3 matrix; reads had one. That asymmetry is now closed — 339 v2-read cases across the three surfaces that read legacy payloads:integration/shared/v2-decrypt-compat(native)integration/wasm/v2-decrypt-compat(wasm-inline)__tests__/dynamodb/v2-type-coverageAll three drive from one shared, entry-agnostic fixture plan (
packages/test-kit/src/v2-fixtures.ts) derived fromV3_MATRIX, so there is a single domain list rather than three that drift. 29 of 40 domains are minted, spanning thebigint,boolean,date,number,stringandtimestampplaintext axes. Every case asserts{ v: 2, i: { t, c } }on the fixture before asserting any value — without that the matrices would pass just as happily against v3 payloads and prove nothing about legacy reads.The 11 deferrals carry written reasons and are themselves guarded by partition tests that run credential-free (
__tests__/test-kit-v2-fixtures.test.ts), so a newly added domain fails loudly rather than being silently skipped: the 10 ope-indexed domains (a v2 payload from an ope column is data that never existed — the removed v2 builder'sorderAndRange()emittedore) andjson_search(cipherstash-client 0.42 refuses to emit a v2 ste_vec).The wasm entry gets its own matrix rather than inheriting the native one because it reconstructs per value via
reconstructDateValue, a separate implementation from native's path-awarereconstructDatePaths.Previously a known gap, now fixed: a stored v2 grouped date column decrypted but was never reconstructed, returning a string where the declared type says
Date. The bare-leaf fallback re-nested the rebuilt envelope atdetails.placedAtwhile both clients resolved date columns from the DECLARED paths, so neither found it. Fixed inc745db71at the adapter layer rather than in either client — the read path is the only layer that knows the alias fired, so reconstructing on the returned plaintext covers the native and wasm-inline entries at once, with the path set computed PER ITEM on the bulk path. The characterisation test that pinned the old behaviour now asserts the fix, and__tests__/dynamodb/v2-grouped-date.test.tscovers it directly. Tracked and closed as #833.Review rounds since the first commit, each defect reproduced before being acted on:
decryptModel/bulkDecryptModelspositional overloads acceptLockContextInput | undefinedagain — the arity split had droppedundefined, breakingdecryptModel(row, users, session?.lockContext). Binding twice is still a compile errorWasmEncryptionConfig.schemasproves non-emptiness again; widening it toreadonly AnyV3Table[]had let{ schemas: [] }through, becauseNonEmptyV3<readonly AnyV3Table[]>resolvesS['length']tonumberrather than0@ts-expect-error)EncryptionClient<readonly AnyV3Table[]>and keep the typed surface — this PR's own Supabase adapter carries a cast disproving itpackages/stack-supabase/src/query-encrypt.tscrossed the FTA cap of 71 (71.51), whicha3d6abb8had just ratcheted down from 91. Rather than raise the cap,encryptFilterValueswas split at its natural seam — the pure synchronous walk overDbQuerySpacedeciding which operands need encrypting moved to a new./query-terms, leaving the async FFI half behind. Capability validation deliberately stayed put, soassertTermQueryableremains the single boundary for it.query-encrypt.tsis now 60.60 andquery-terms.ts59.07. No symbol involved appears in the builtdist/index.d.ts, so the public surface is unchanged and the change carries no changeset.Verification:
d5391279;c745db71is queued, including Run Tests (Node 22, Node 24, Bun), Analyze v3 complexity, Lint (Biome), the Drizzle / Supabase / prisma-next v3 integration suites against both postgres and supabase, the WASM Deno E2E, and the Prisma README walkthroughCS_*credentials, so the credential-gated encryption suites genuinely execute there. The Drizzle integration job log confirms the v2 read matrices run live rather than skipping:✓ integration/shared/v2-decrypt-compat.integration.test.ts (116 tests)and✓ integration/wasm/v2-decrypt-compat.integration.test.ts (107 tests). This settles the one contract that source reading could not: protect-ffi returns a nativebigintfor a stored v2 int8 payload, and date/timestamp values reconstruct from v2 wire, on both the native and WASM pathsReviewer note: CodeRabbit skipped this PR ("153 files exceed the limit of 100"), so no automated review has run over the diff.