refactor(stack): restore erased EQL v3 envelope + Result types#618
Conversation
The refactor the integration harness (#616) was built to make safe. Both v3 adapters typed their operand-encryption paths as `unknown` and dropped the `Result` wrapper, so the payload type and the failure channel were invisible. - eql/v3/drizzle/operators.ts: `encrypt`/`bulkEncrypt` were typed to return `unknown`; the operation `Result` collapsed to `{ data?: unknown; failure? }`; the bulk response was cast `as Array<{ data: unknown }>`. Now the client surface returns a typed `ChainableOperation<Encrypted>` / `ChainableOperation<BulkEncryptedData>` whose `then` resolves `Result<T, EncryptionError>`, with the casts gone. - supabase/query-builder-v3.ts (and the base query-builder.ts): `encryptCollectedTerms` / `bulkEncryptGroup` / `encryptGroupPerTerm` returned `Promise<unknown[]>`. They now return `EncryptedQueryResult[]` / `Array<Encrypted | null>` / `Encrypted[]`. Types come from protect-ffi via stack's existing re-exports (`Encrypted`, `BulkEncryptedData`, `EncryptedQueryResult`) — NOT from @cipherstash/eql, which is only a devDependency; importing it into src would emit unresolvable .d.ts references. See the PR thread for why per-column precision isn't threadable through the dynamic operator API and the domain union is the right granularity. Un-erasing surfaced two things the `unknown` hid: - The Supabase base/v3 divergence: the v2 path yields `encryptQuery` composite literals, the v3 path yields `JSON.stringify`'d envelope strings. Both are `EncryptedQueryResult` (its `string` arm), now stated in the type. - A latent bug in `ChainableOperation`: it required `withLockContext` to return something re-lockable, but the real `EncryptOperationWithLockContext` isn't. Now two-tier (`ChainableOperation` -> `AuditableOperation`), which is what lets the real client satisfy the surface with no cast — the M1 regression the .test-d.ts guards. Strengthened that test: a `{ encrypt }` returning `unknown` no longer typechecks. Type-only: no runtime logic changed (same JSON bytes, same length check). Src typechecks with 0 errors; type tests pass; 703 adapter unit tests + 1561 total unit tests pass. Real-crypto integration is covered by #616's suites on CI — local re-run was blocked by an OrbStack daemon outage, not by this change.
🦋 Changeset detectedLatest commit: 311dfc6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 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 |
There was a problem hiding this comment.
Pull request overview
This PR restores the canonical EQL v3 envelope unions and Result<T, EncryptionError> typing through the v3 Drizzle and Supabase adapter operand-encryption paths, reversing prior unknown/type-erasure that hid both payload shape and failure handling from the compiler.
Changes:
- Retypes Supabase query-term encryption to return
EncryptedQueryResult[](v2 and v3 builders), making the operand encoding and nullability explicit at the type level. - Retypes Drizzle v3 operator operand encryption to carry
Encrypted/BulkEncryptedDatathrough a properly modelled thenable operation chain (ChainableOperation<T>→AuditableOperation<T>), aligning with the real SDK surface. - Updates the Drizzle
.test-d.tsregression guard and adds a patch changeset documenting the restoration.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/stack/src/supabase/query-builder.ts | Returns typed EncryptedQueryResult[] from v2 batch term encryption (encryptQuery(terms)), instead of unknown[]. |
| packages/stack/src/supabase/query-builder-v3.ts | Returns typed EncryptedQueryResult[] and narrows bulk/per-term encryption helpers to EQL v3 envelope types. |
| packages/stack/src/eql/v3/drizzle/operators.ts | Replaces unknown operand encryption + ad-hoc result shape with typed Result<T, EncryptionError> and typed envelope payloads. |
| packages/stack/tests/drizzle-v3/operators.test-d.ts | Updates the structural-double test to match the now-typed operation surface (Result<Encrypted, EncryptionError>). |
| .changeset/eql-v3-adapter-type-robustness.md | Patch changeset describing the adapter type un-erasure and its guarantees. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- operators.test-d.ts: add the negative assertion the changeset claimed but
never made — a `{ encrypt }` double returning `unknown` must be REJECTED by
createEncryptionOperatorsV3. The prior positive `toBeCallableWith(double)`
could not catch a re-erasure (a correct value is assignable to `unknown`), so
the "pinned" guarantee was toothless. The `@ts-expect-error` now goes unused
and fails the moment the client contract regresses to `unknown`.
- query-builder.ts: type `EncryptedFilterState.encryptedValues` as
`EncryptedQueryResult[]`, not `unknown[]`. `encryptCollectedTerms` was restored
to return that type but the value widened straight back to `unknown` at this
field, so the use site (`encryptedValues[i]`) still saw `unknown` — the
restoration never reached the consumer for the base builder. Now it does.
- query-builder-v3.ts: guard the `null` arm in `bulkEncryptGroup`. The restored
`Encrypted | null` type makes null reachable; an unguarded null envelope would
be `JSON.stringify`'d to the literal `"null"` and sent as a filter operand
(matching the wrong rows). Treat it as a contract violation.
- changeset: `patch` → `minor`. Tightening the public `createEncryptionOperatorsV3`
client contract is a compile-time breaking change for loosely-typed downstream
clients; corrected the "no runtime change" line to note the new null guard.
Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Drives `bulkEncrypt` to return a length-matched response with a null envelope at position 0 and asserts the query fails 500 with "null envelope at position 0" rather than sending the literal "null" as the `in` operand. Sibling to the existing length-mismatch guard test. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
The refactor the harness was built to make safe. #616 gave the v3 adapters real-crypto coverage; this leans on it to undo the type erasure they were shipping.
The problem
Reviewing #535 turned up that the canonical EQL v3 payload types are erased in both adapters.
@cipherstash/eql(and the copy@cipherstash/protect-ffivendors) defines a precise type per domain —IntegerOrd = { v, i, c, op },IntegerOrdQuery = { v, i, op }, ~130 of them — and stack already re-exports the storage union asEncrypted. But the operand-encryption paths threw all of it away:eql/v3/drizzle/operators.tstyped the client'sencrypt/bulkEncryptas returningunknown, collapsed the operation'sResultto{ data?: unknown; failure?: { message } }, and cast the bulk response toArray<{ data: unknown }>.supabase/query-builder-v3.tsreturnedPromise<unknown[]>fromencryptCollectedTerms,bulkEncryptGroupandencryptGroupPerTerm; the basequery-builder.tsdid the same.So an operand's type, and whether encryption failed, were both invisible to the compiler. The
eql_v3.query_*typing survived only as a runtime SQL cast.What changed
The paths now carry the SDK's real types, threaded through a properly-typed operation surface that resolves
Result<T, EncryptionError>:Encrypted— the storage-envelope union, which already includes every v3 per-domain payload.BulkEncryptedData—{ id?, data: Encrypted | null }[].EncryptedQueryResult—Encrypted | EncryptedQuery | string | null.No new dependency.
@cipherstash/eqlis only a devDependency of stack, so importing its types intosrc/would emit.d.tsreferences consumers can't resolve.protect-ffi— a real dependency — vendors the same generated types, and stack already re-exports them. See the decision note in the thread for why per-column precision (IntegerOrdfor this column) isn't threadable through the dynamic operator API, and why the domain-unionEncryptedis the right granularity.What the erasure was hiding
Un-erasing the Supabase path surfaced a genuine divergence between v2 and v3 that
unknown[]had flattened: the baseencryptCollectedTermsyieldsencryptQuerycomposite literals (objects); the v3 override yieldsJSON.stringify'd envelope strings. Both areEncryptedQueryResult— that union has astringarm precisely for the composite/envelope forms — so the divergence is now stated in the type rather than concealed by it.Modelling the operation chain
ChainableOperation<T>had a subtle bug theunknownhid: it requiredwithLockContextto return something also re-lockable, but the realEncryptOperationWithLockContextdropswithLockContext(you can't lock-context twice). It's now two-tier —ChainableOperation<T>→AuditableOperation<T>— which is what lets the realTypedEncryptionClientsatisfy the structural surface with no cast. That was the M1 regression the.test-d.tsalready guarded, and it still passes.Verification
operators.test-d.tsasserts (via@ts-expect-error) that a{ encrypt }double returningunknownis REJECTED bycreateEncryptionOperatorsV3. A positive "correctly-typed double is accepted" check cannot catch a re-erasure — a correct value is assignable tounknown— so the negative assertion is the one with teeth; it fails the moment the client contract regresses. The real client is still accepted with no cast (M1).nullenvelope frombulkEncrypt(the restoredEncrypted | nulltype makes that arm reachable; an unguarded null would beJSON.stringify'd to the literal"null"and sent as a filter operand). Unit-tested.src/typechecks with zero errors; the unit suite passes. The changeset is aminor: tightening the publiccreateEncryptionOperatorsV3client contract fromunknownto a typed operation surface is a compile-time breaking change for loosely-typed downstream clients.Post-review fixes (round 2)
A
/code-reviewpass over this PR found the un-erasure was only partly realised, and closed the gaps:encryptCollectedTermswas retyped toEncryptedQueryResult[], but its value was stored intoEncryptedFilterState.encryptedValues, still typedunknown[], so the use site (encryptedValues[i]) widened straight back tounknown. That field is nowEncryptedQueryResult[], so the restored type actually reaches the consumer..test-d.tsonly asserted a correctly-typed double is accepted — which keeps passing even if the contract re-erases tounknown. Added the negative assertion described in Verification.nullarm the restored type newly names was unguarded in the Supabase bulk path — fixed with a guard + unit test (see Verification).patch→minor(breaking type-surface change) and its "no runtime change" line corrected.Still to come
Two more adapter type-robustness tasks and the follow-ups queued in the plan doc (removing the
LIVE_*unit-suite skips; porting the remaining live suites onto the harness).