fix(v3): resolve column domains when the client is built, not per row

[coderdan]

Follow-up to #132, addressing findings 2, 4 and 5 from the re-review.

Findings 2 and 5 turned out to have one shared fix, so they land together.

The docs were describing behaviour that didn't exist (#2)

target_domain_for_column was only reached from storage_output / query_output, so the EQL v3 column-domain check ran at encrypt time. But CHANGELOG.md:91, README.md:98 and docs/jsonb-api-reference.md:319 all said it happened "at configuration time", and the comment on the ste_vec guard said it fired "instead of at encrypt time" while sitting on the encrypt path.

The repo's own test proved the docs wrong — newClient accepted an invalid v3 config and only encrypt rejected it:

const client = await newClient({ encryptConfig: config, eqlVersion: 3 })   // succeeded
const attempt = encrypt(client, { plaintext: true, column: 'flagged', ... })
await expect(attempt).rejects.toMatchObject({ code: 'EQL_V3_UNSUPPORTED_COLUMN' })

Rather than reword three documents, this makes the claim true. newClient now resolves every configured column onto its eql_v3 domain up front — before any network I/O, alongside the existing eqlVersion fail-fast — and stores the result on the client.

This is a behaviour change. An unrepresentable v3 column now throws EQL_V3_UNSUPPORTED_COLUMN from newClient instead of on the first encrypt to that column. That closes a real gap: a column that was configured but never written to previously never errored at all. v2 clients are untouched — their payloads pass through unconverted, so no domain is needed, and the same configs stay valid there.

The two tests at eql-v3.test.ts:696-740 are updated to assert against newClient, and three Rust tests pin the new seam directly (v2 resolves nothing; v3 resolves every column; v3 rejects and names the offending one).

Per-row domain re-derivation on the bulk path (#5)

Falls out of the above. storage_output ran once per payload inside both bulk loops (lib.rs:1408, wasm.rs:515), and each call allocated the domain String and then called TargetDomain::parse, which rebuilds a ~52-element Vec<Box<dyn DomainType>> and linear-scans it comparing strings. The resolved TargetDomain is Copy and depends only on the column config, so it now comes from a map built once at client construction.

Two supporting types keep this honest rather than bolted on:

• OutputTarget pairs the wire version with the resolved domain (V2 | V3(TargetDomain)), so the v2/v3 split is a single exhaustive match at each output seam instead of a version check followed by a fallible lookup that "can't fail".
• ColumnResolver bundles the three client fields the encrypt entry points need. The Neon and wasm clients share no type but hand exactly these three to the same seams. It also keeps prepare_query_plaintext under clippy's argument limit (it would otherwise have hit 8) and removes the duplicated .ok_or_else(|| Error::UnknownColumn(...)) blocks from four call sites.

Columns resolve in identifier order. HashMap iteration order varies per map instance, so a config with two unrepresentable columns would otherwise name a different one on each run — an error that moves between identical runs reads like a flake. There's a test for it, and it's a real guard: removing the sort fails it on every run.

The missing inventory case (#4)

every_selected_domain_resolves_in_the_v3_inventory never exercised unique + ope + match → eql_v3_text_search, the only case reaching the _search arm added in #132, and the only domain name the _search_ore case didn't already cover. A typo in that arm's suffix would have slipped past the one test written to catch exactly that, and surfaced at encrypt time as an InvariantViolation. One line.

Not included

Findings 6, 7 and 8 (the per-arm v3_domain prefix obligation, the redundant sv / sv_mode state, and the double allocation at eql_v3.rs:374) are untouched — pure internal quality, no user-visible behaviour, and better kept out of a diff that already changes when an error fires.

Verification

cargo fmt --check, cargo clippy --no-deps --tests --all-features --all-targets -- -D warnings (the CI invocation), 211 Rust unit tests, the wasm32-unknown-unknown target, npm test (typecheck + 56 unit + lint + format), and a separate tsc --noEmit over integration-tests/, which reports the same three pre-existing @cipherstash/auth/wasm-inline module-resolution errors as main and nothing new.

The integration tests themselves were not run — they need Docker, Postgres and CipherStash credentials. The eql-v3.test.ts changes here are exactly the two tests whose behaviour this PR moves, so they are unproven end to end.

[coderabbitai]

Warning

Review limit reached

@coderdan, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 59 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d12dbc66-a951-41a7-a16a-25035874adda

📥 Commits

Reviewing files that changed from the base of the PR and between d21a87a and 9bcec22.

📒 Files selected for processing (6)

• CHANGELOG.md
• crates/protect-ffi/src/eql_v3.rs
• crates/protect-ffi/src/lib.rs
• crates/protect-ffi/src/wasm.rs
• integration-tests/tests/eql-v3.test.ts
• integration-tests/tests/wasm-round-trip.test.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/v3-domain-resolution-at-client-build

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderdan]

Rebased onto latest main and addressed a full review pass. Summary of what changed since the last push:

Behaviour decision (the load-bearing one)

The review confirmed that resolving domains at newClient breaks v3 clients that worked before: one unrepresentable column in the config now kills the whole client, even for an app that only decrypts, queries by selector, or never touches that column. (v2 clients are unaffected — resolve_all never attempts resolution under v2.)

Decision: keep the fail-fast rejection. A config declaring a column v3 cannot store is a configuration error, and reporting it at build — once, naming the column — is the point of the change. But it is a breaking change, so it is now documented as one under the repo's Breaking heading, with the remedy: a read-only service can build with eqlVersion: 2, since decrypt is version-agnostic and reads v3 payloads regardless of the setting.

Fixes

Issue	Fix
Errors named only the bare column	Now table.column — a whole-config sweep has no encrypt(table, column) call site to disambiguate, and two tables may share a column name
Neon parsed config before auth; wasm validated strategy first	Both build the strategy first, then the config, then network — same failure on each target
CHANGELOG claimed "v2 clients are unaffected"	Config parsing now precedes auth/network for all clients; documented under Changed
Two parallel maps kept in sync by doc comment; split column_config/output_target	One ResolvedEncryptConfig: each column's config + target in one entry, one resolve() lookup, drift impossible by construction
Both clients hand-assembled the same 3 fields + identical resolver()	One shared owned struct; each client holds one Arc, and the validate → parse → resolve sequence lives once
encrypt_bulk re-resolved per row after encryption	Carries the Copy target from loop 1, as encrypt_query_bulk already did — drops a per-row lookup and an unreachable error arm
target() test helper's doc claimed a seam its body didn't use	Resolves through ResolvedEncryptConfig itself; 28 conversion tests now exercise the real seam
Nothing asserted the resolved target's variant	New tests: variant + domain pairing, unknown-column, table disambiguation
wasm rejection path untested	New credential-free tests — resolution runs before the clientKey decode and any I/O

Also fixed a changelog misfiling the rebase introduced: the entry had context-matched into the released [0.29.0] section instead of [Unreleased].

Not changed: no integration test covers an encrypt-time v3 conversion error, because no user input can reach those paths on a validly-built client — they're fail-closed invariant guards, unit-tested in eql_v3.rs and errors.test.ts. Flagging it as a conscious call rather than an oversight.

218 unit tests pass (+2), both targets compile, fmt/lint/typecheck clean.

[Copilot]

Pull request overview

This PR moves EQL v3 column-domain resolution from per-encrypted-row (and per-query payload) to client construction time, so invalid v3 column configurations fail fast in newClient and bulk encryption avoids repeated domain parsing/allocation work.

Changes:

• Introduces a pre-resolved encrypt configuration (ResolvedEncryptConfig) and per-column OutputTarget (v2 vs v3 + resolved TargetDomain) and threads this through Neon + wasm encrypt/query output seams.
• Updates integration tests to assert the new error timing (v3 config rejection at newClient, while v2 still accepts the same config) and adds wasm wiring tests for the same seam.
• Documents the behavior change and performance improvement in CHANGELOG.md.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
integration-tests/tests/wasm-round-trip.test.ts	Adds wasm-side validation tests to ensure v3 domain resolution (and its failures) happen during newClient without requiring valid credentials.
integration-tests/tests/eql-v3.test.ts	Updates v3 configuration error expectations to occur at newClient rather than first encrypt.
crates/protect-ffi/src/wasm.rs	Switches wasm client to store a resolved config and passes OutputTarget into output shaping to avoid per-row domain derivation.
crates/protect-ffi/src/lib.rs	Switches Neon client to store a resolved config; updates encrypt/query entry points to use OutputTarget and resolves config before ZeroKMS setup.
crates/protect-ffi/src/eql_v3.rs	Adds OutputTarget + ResolvedEncryptConfig, resolves v3 TargetDomain once at build time, and updates storage/query output helpers and tests accordingly.
CHANGELOG.md	Adds breaking-change note and documents domain-resolution timing/performance change.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.