test(stack): eql v3 domain test-matrix + equality-via-ORE fix#540
Open
tobyhede wants to merge 7 commits into
Open
test(stack): eql v3 domain test-matrix + equality-via-ORE fix#540tobyhede wants to merge 7 commits into
tobyhede wants to merge 7 commits into
Conversation
Add a single declarative catalog that drives both a runtime `it.each`
matrix and type-level `expectTypeOf` assertions for every EQL v3 scalar
domain — the TypeScript analog of the Rust `eql_v3` `scalar_matrix!`
harness. Replaces the hand-rolled, per-domain test bodies with one
source of truth.
- add exported `EqlTypeForColumn<C>` helper beside `PlaintextForColumn`,
so the catalog keys off `EqlTypeForColumn<AnyEncryptedV3Column>` (the
full domain union) rather than a hand-copied list.
- __tests__/v3-matrix/catalog.ts: `V3_MATRIX` covering all 35 domains,
`as const satisfies Record<EqlV3TypeName, DomainSpec>`. Coverage is
MANDATORY — omitting a domain fails `tsc` and names the missing one,
the compile-time analog of (and stronger than) the Rust
`test:matrix:inventory` cross-check. Every field is consumed by a
test. `typedEntries` keeps the matrix key as `EqlV3TypeName`.
- matrix.test.ts: runtime matrix asserting `build()` toStrictEqual
`{ cast_as, indexes }` at full fidelity across all domains.
- matrix.test-d.ts: type-level matrix (plaintext axis, derived queryType
union, storage-only exclusion, exhaustiveness anchor), with the table
built from the catalog's own builders so one catalog drives both
surfaces.
- schema-v3.test.ts: remove the superseded `domainCases` array + its
it.each and the now-redundant basic text_search asserts; keep the
text_search-specific behavior (v2 parity, freeTextSearch tuning,
clone-on-write / no-alias). Prune now-unused imports.
`indexes` is stored per-row as data, not derived, because text_search
overrides build() to emit unique+ore+match.
Verified: test:types 54/54 (no type errors), runtime matrix 35/35,
schema-v3 26/26, tsup build + biome clean. No regressions — full-suite
failures are the 18 pre-existing FFI/env cases (identical with changes
stashed).
Fix an EQL v3 SDK bug and close the largest test-coverage gaps between v3
and v2, driven off the type-driven domain catalog.
SDK fix (Part A):
- resolveIndexType now resolves `equality` to the `ore` (`ob`) index on
order-capable v3 columns instead of throwing on the absent `unique`
index, matching the documented capability ("exact-match ... or
comparison via `ob`") and the type surface. Gated on getQueryCapabilities
(v3-only), so v2 columns keep their equality-without-unique throw
unchanged (no-v2-change constraint). No build()/wire change.
- Deterministic regressions (ord+equality resolves to ore per plaintext
axis; v2 order-only column still throws) plus a required live pg proof
that `ord_term(x) = ore_block_256(term)` selects the exact row.
Test coverage (Part B):
- catalog: add samples/errorSamples per domain (numeric split
integer-vs-fractional; NaN/±Infinity as error samples).
- matrix-live: live round-trip of all 35 domains x samples via batched
bulkEncryptModels/bulkDecryptModels, plus NaN/Infinity rejection.
- schema-v3: catalog-driven blocker sweep over every (domain, queryType)
pair, superseding the two hand-picked misuse cases.
- matrix-lock-context: offline wiring for the v3 typed client, incl. the
positional decryptModel lockContext path; matrix-identity-live: live
lock-context + audit round-trip; matrix-audit.test-d: pins that v3
decryptModel has no .audit() hook.
- matrix-keyset: invalid-UUID (deterministic) + live ensureKeyset.
- matrix-bulk: 100-item live round-trip through the v3 typed client.
- wire the previously-dead occurredAt timestamptz column into a
round-trip assertion.
190 deterministic tests pass, 56 type tests pass, tsc clean; live suites
soft-skip without credentials.
Defence in depth: the equality-via-ORE fix shows an SDK-side bug can hide behind a clean FFI round-trip and only surface against real SQL, so every domain gets a live query-correctness proof, not just the 4 already covered. - matrix-live-pg.test.ts (new): one mega Postgres table across all 35 domains, one proof per domain dispatched by capability tier (mirrors resolveIndexType's own priority — match > unique > ore > none): eq_term/hmac_256 for *_eq (8), ord_term/ore_block_256 equality-via-ORE for *_ord/*_ord_ore (16 — verified against the SQL fixture that non-text ord domains have no eq_term at all, so this is the only equality path that exists for them), match_term/bloom_filter for text_match/text_search (2), plain INSERT/SELECT round-trip for storage-only domains (9). Doubles as a canonical example per capability tier of how to query each v3 domain kind. - matrix-live.test.ts: fix 2 latent type errors (spec.errorSamples didn't resolve because `as const satisfies Record<...>` gives rows that omit the optional field a type lacking the key, not `undefined`) by pinning typedEntries's type arguments explicitly. Caught by running real tsc against the file — vitest run only transpiles .test.ts files, it never type-checks them, so this had shipped unnoticed in the prior commit. Both live suites soft-skip without credentials; verified via tsc, biome, and vitest in a sandbox with no live DB — SQL correctness itself is unverified beyond static checks against the real eql_v3 fixture.
Applies 4 of 6 findings from the CodeRabbit review of cfacc3b7 (equality-via-ORE fix + live v3 domain coverage). The other 2 findings are plan/design-doc feedback, not source changes. - matrix-lock-context.test.ts: restore CS_WORKSPACE_CRN after each test so it doesn't leak into other suites sharing the Vitest worker. - stub-auth-wasm-inline.ts: add an OidcFederationStrategy stub alongside AccessKeyStrategy — src/wasm-inline.ts re-exports both, so importing it under the Vitest alias could fail with only one stubbed. - identity/index.ts: omit ctsToken from getLockContext()'s return when unset, instead of returning it as an explicit `undefined`, so the shape matches the optional `ctsToken?` type callers check presence against. - tests.yml: fix a stale version comment (protect-ffi 0.25+/auth 0.38+ -> 0.26+/0.40+, matching the actual e2e/wasm deps). Verified: schema-v3/v3-matrix/lock-context suites pass (212/212, rest soft-skip without live creds), biome clean, build clean.
… order Address CodeRabbit review findings: - cjs-require: also assert encryptedTable and buildEncryptConfig are exported from the v3 CJS bundle so regressions in the primary /schema/v3 export surface are caught. - cli run() helper: build raw from interleaved chunks instead of stdout + stderr so the combined transcript preserves real ordering.
|
|
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 |
CI run 28569708268 (PR #540, Node 22) surfaced two real, distinct bugs against live credentials. Disabling both to unblock CI; root causes are identified but not fixed here. - schema-v3-client.test.ts: skip the occurredAt timestamptz round-trip test. Confirmed root cause: protect-ffi's native CastAs has a distinct 'timestamp' variant (full date+time) separate from 'date' (calendar-date only), but this SDK's CastAs/PlaintextKind types never included 'timestamp' - every timestamptz domain sets cast_as: 'date', identical to the plain date domain, so the native layer silently truncates time-of-day. Pre-existing SDK gap (predates this branch), not a test bug. - matrix-live-pg.test.ts: force-skip the whole suite. beforeAll crashes with `PostgresError: invalid input syntax for type json` on the dynamic 35-column INSERT, before any per-domain case runs. Root cause not yet pinned - CI's stack trace bottoms out in postgres.js's connection handler with no frame back to the offending parameter/domain, and the same ciphertext values round-trip fine via FFI-only in the sibling matrix-live.test.ts, so the break is specific to how this file hands them to Postgres. Needs live query/parameter logging or a local repro to isolate. Verified: 441 passing (18 pre-existing/unrelated failures, reproduced identically without these changes), test:types 56/56, build clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/stack/__tests__/schema-v3.test.ts (1)
271-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSweep is correct but re-implements production gating logic and hits an internal helper directly.
queryTypeAllowed()hand-derives the throw/allow outcome fromDomainSpec['indexes'], mirroringresolveIndexType's equality-via-ore branch rather than exercising it as a black box (e.g. viaresolvesEqualityViaOreif exported, or the publicencryptQuery/typed-client surface). This works today but two implementations of the same gating rule can silently drift if one changes without the other. Given the file's guideline to prefer public-API testing, consider whether this 105-case sweep could assert throughtypedClient(...).encryptQuery(...)instead ofresolveIndexTypedirectly — though the current whitebox approach is reasonable for fast, non-FFI coverage of a specific internal fix.As per coding guidelines for
packages/**/__tests__/**/*.test.{ts,tsx}: "Prefer testing via public API; avoid reaching into private internals in tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/__tests__/schema-v3.test.ts` around lines 271 - 330, The catalog-driven sweep in schema-v3.test.ts is duplicating production gating logic through queryTypeAllowed() and calling resolveIndexType() directly, which makes the test brittle against future drift. Refactor the sweep to exercise the public surface instead of the internal helper, ideally by asserting through typedClient(...).encryptQuery(...) or another exported API path, and remove the hand-derived index gating so the test validates behavior end-to-end. Keep the existing coverage intent and the specific match-only/storage-only spot checks, but route them through public-facing symbols rather than resolveIndexType.Source: Coding guidelines
packages/stack/__tests__/v3-matrix/matrix-live-pg.test.ts (1)
153-171: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPossible root cause of the documented crash: missing
sql.json()wrapping.
insertRowpassesenc[slug(t)](raw JS objects) directly as query parameter values before casting to::${t}. The siblingschema-v3-pg.test.tsregression test wraps the same kind of encrypted-payload parameter withsql.json(...)before casting (e.g.${sql.json(ageCt)}::eql_v3.int4_ord). This inconsistency looks like a plausible cause of the "invalid input syntax for type json" error noted in the file's comments as unresolved.🔧 Proposed fix: wrap parameter values with `sql.json(...)`
- const values = domains.map(([t]) => enc[slug(t)]) as never[] + const values = domains.map(([t]) => sql.json(enc[slug(t)] as never)) as never[]Want me to draft the fix and re-enable
describeLivePgfor a trial run to confirm this resolves the crash?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/__tests__/v3-matrix/matrix-live-pg.test.ts` around lines 153 - 171, The `insertRow` helper in `matrix-live-pg.test.ts` is sending raw encrypted payload objects from `enc[slug(t)]` directly into `sql.unsafe`, which can trigger the JSON parsing crash. Update `insertRow` to wrap each parameter value with `sql.json(...)` before casting to the target domain type, matching the pattern used in `schema-v3-pg.test.ts`, and keep the `casts`/`values` assembly aligned in that helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stack/__tests__/v3-matrix/matrix-live-pg.test.ts`:
- Around line 46-65: The live setup hooks are still running even though the
suite is meant to be skipped, because beforeAll/afterAll are defined at file
scope rather than inside the skipped describe block. Move the lifecycle hooks
and the live setup/teardown logic into the describeLivePg (or conditional
describe) suite so they are only registered when the suite actually runs, and
keep the LIVE_EQL_V3_PG_ENABLED gate aligned with that suite selection.
---
Nitpick comments:
In `@packages/stack/__tests__/schema-v3.test.ts`:
- Around line 271-330: The catalog-driven sweep in schema-v3.test.ts is
duplicating production gating logic through queryTypeAllowed() and calling
resolveIndexType() directly, which makes the test brittle against future drift.
Refactor the sweep to exercise the public surface instead of the internal
helper, ideally by asserting through typedClient(...).encryptQuery(...) or
another exported API path, and remove the hand-derived index gating so the test
validates behavior end-to-end. Keep the existing coverage intent and the
specific match-only/storage-only spot checks, but route them through
public-facing symbols rather than resolveIndexType.
In `@packages/stack/__tests__/v3-matrix/matrix-live-pg.test.ts`:
- Around line 153-171: The `insertRow` helper in `matrix-live-pg.test.ts` is
sending raw encrypted payload objects from `enc[slug(t)]` directly into
`sql.unsafe`, which can trigger the JSON parsing crash. Update `insertRow` to
wrap each parameter value with `sql.json(...)` before casting to the target
domain type, matching the pattern used in `schema-v3-pg.test.ts`, and keep the
`casts`/`values` assembly aligned in that helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4016226-054c-418d-8514-a2d2ddfd1ebe
📒 Files selected for processing (20)
.github/workflows/tests.ymlpackages/cli/tests/helpers/run.tspackages/stack/__tests__/cjs-require.test.tspackages/stack/__tests__/helpers/stub-auth-wasm-inline.tspackages/stack/__tests__/schema-v3-client.test.tspackages/stack/__tests__/schema-v3-pg.test.tspackages/stack/__tests__/schema-v3.test.tspackages/stack/__tests__/v3-matrix/catalog.tspackages/stack/__tests__/v3-matrix/matrix-audit.test-d.tspackages/stack/__tests__/v3-matrix/matrix-bulk.test.tspackages/stack/__tests__/v3-matrix/matrix-identity-live.test.tspackages/stack/__tests__/v3-matrix/matrix-keyset.test.tspackages/stack/__tests__/v3-matrix/matrix-live-pg.test.tspackages/stack/__tests__/v3-matrix/matrix-live.test.tspackages/stack/__tests__/v3-matrix/matrix-lock-context.test.tspackages/stack/__tests__/v3-matrix/matrix.test-d.tspackages/stack/__tests__/v3-matrix/matrix.test.tspackages/stack/src/encryption/helpers/infer-index-type.tspackages/stack/src/identity/index.tspackages/stack/src/schema/v3/index.ts
Addresses code review of the disabled matrix-live-pg suite (one finding confirmed invalid, one skipped as not worth the tradeoff, this one confirmed and fixed - see prior turn for full verification detail). Root cause of the original CI crash (PostgresError: invalid input syntax for type json), traced into postgres.js's Bind() in connection.js: a bare ciphertext object has no recognized wire type under inferType() (only Parameter/Date/Uint8Array/boolean/bigint are special-cased), so it falls back to `'' + x` - literal JS string coercion, producing "[object Object]" on the wire. sql.json(value) avoids this by returning a Parameter with an explicit type OID that has a registered serializer. Fixed both insertRow's values and the eqTerms/ordTerms/matchTerms references in the it.each blocks - all four pass raw ciphertext/query-term objects through sql.unsafe() the same way, so all four had the identical bug. schema-v3-pg.test.ts already uses this exact sql.json() pattern, confirming it's correct. Suite stays describe.skip'd - the underlying bug is fixed but unverified against live credentials in this sandbox, so re-enabling is a separate call. Verified: biome clean, tsc clean (no new errors), full suite unchanged at 441 passing / 18 pre-existing-unrelated failures, test:types 56/56, build clean.
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
1 participant
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.
Summary
Stacked on #535. Closes the test-coverage gap between the new EQL v3 typed
schema/client and the long-established v2 client: v3 had only structural
(unit) coverage for its 35 domains and no live-behavioral proof, while v2's
suite exercises live FFI/Postgres round-trips, blocker/error cases, lock
context, keysets, and bulk-at-scale throughout.
__tests__/v3-matrix/catalog.ts): a singleRecord<EqlV3TypeName, DomainSpec>,as const satisfies-checked so addinga domain to the SDK without a matching row fails
tsc(compile-timecoverage guarantee, stronger than the Rust
eql_v3harness's CIsnapshot-diff equivalent). Drives both a runtime
it.eachmatrix(
matrix.test.ts) and type-levelexpectTypeOfassertions(
matrix.test-d.ts).infer-index-type.ts): order-capable v3 columns(
*_ord/*_ord_ore) answer equality via theiroreindex instead ofthrowing on the absent
uniqueindex — matching the documented capabilitycontract and the type surface (
QueryTypesForColumnalready allowed'equality'on these columns; the runtime rejected it). Gated so v2columns are unaffected. Includes a live Postgres proof that the resulting
query selects the exact row via
eql_v3.ord_term(...) = eql_v3.ore_block_256(...).matrix-live.test.ts— every domain round-trips real FFI ciphertext,batched via
bulkEncryptModels/bulkDecryptModels(2 network calls, not~120), plus NaN/±Infinity rejection for every numeric domain.
matrix-live-pg.test.ts— every domain gets a real Postgres query proofdispatched by capability tier (
eq_term/hmac_256,ord_term/ore_block_256,match_term/bloom_filter, or a plainround-trip for storage-only domains) against a real installed
eql_v3extension.
matrix-lock-context.test.ts/matrix-identity-live.test.ts/matrix-audit.test-d.ts— lock-context and audit-metadata coverage forthe v3 typed client (offline-mocked + live), including the one real v3
limitation (
decryptModelhas no.audit()hook) pinned as anexecutable type invariant.
matrix-keyset.test.ts— keyset config (deterministic invalid-UUID +live).
matrix-bulk.test.ts— 100-item bulk round-trip through real FFI.schema-v3.test.ts): every(domain, queryType) pair, superseding two hand-picked cases.
matrix-lock-context.test.ts,missing
OidcFederationStrategyauth stub,ctsTokenoptional-fieldshape, a stale version comment in
tests.yml, and hardening the v3 CJSexport check +
run()helper's transcript ordering (merged against thisbranch's own
buildRunResultrefactor during the rebase onto feat(stack): EQL v3 typed schema + strongly-typed client (@cipherstash/stack/schema/v3, /v3) #535).No v2 behavior change — a parity-guard regression test locks this
(
encryptedColumn('x').orderAndRange()still throws on equality).Test plan
pnpm --filter @cipherstash/stack test— 441 passing (18 pre-existingFFI/env failures unrelated to this branch, reproduced identically on
feat/eql-v3-text-search-schema); live suites soft-skip withoutCS_*/DATABASE_URLcredentials.pnpm --filter @cipherstash/stack test:types— 56/56.pnpm --filter @cipherstash/cli test— 323/323.pnpm build(stack + cli) andbiome checkclean on every file thisbranch touches.
matrix-live,matrix-live-pg,matrix-bulk,matrix-identity-live, keyset/lock-context live cases, and theequality-via-ORE Postgres proof) need a run with real
CS_*+DATABASE_URLcredentials — unverified in this sandbox beyond staticchecks against the real
eql_v3SQL fixture.Summary by CodeRabbit
New Features
Bug Fixes