feat: upgrade protect-ffi to 0.23.0#473
Conversation
🦋 Changeset detectedLatest commit: 8fac8aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPR upgrades protect-ffi to 0.23.0 and EQL to 2.3.1. Introduces ChangesEQL 2.3.1 Upgrade & Discriminated Payload Adoption
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/stack/__tests__/json-protect.test.ts (1)
61-61: ⚡ Quick winStrengthen discriminator assertions to
k: 'ct'.
toHaveProperty('k')validates presence only. Using'ct'here better locks the ciphertext contract.Suggested diff
- expect(ciphertext.data).toHaveProperty('k') + expect(ciphertext.data).toHaveProperty('k', 'ct') - expect(encryptedModel.data.json).toHaveProperty('k') + expect(encryptedModel.data.json).toHaveProperty('k', 'ct')Also applies to: 113-113, 200-203, 312-318, 446-446
🤖 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__/json-protect.test.ts` at line 61, The tests currently assert only the presence of the discriminator key with expect(ciphertext.data).toHaveProperty('k'); update these assertions to assert the exact discriminator value 'ct' (e.g., use toHaveProperty('k', 'ct') or expect(ciphertext.data.k).toBe('ct')) so the ciphertext contract is locked; apply the same change for the other similar assertions in this file that reference ciphertext.data (and any ciphertext2/ciphertext3 usages).packages/protect/__tests__/json-protect.test.ts (1)
58-58: ⚡ Quick winAssert the discriminator value, not just key presence.
These checks pass even if
khas the wrong value. For ciphertext payloads, assertk: 'ct'explicitly.Suggested diff
- expect(ciphertext.data).toHaveProperty('k') + expect(ciphertext.data).toHaveProperty('k', 'ct') - expect(encryptedModel.data.email).toHaveProperty('k') + expect(encryptedModel.data.email).toHaveProperty('k', 'ct')Also applies to: 110-110, 217-219, 329-335, 514-514
🤖 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/protect/__tests__/json-protect.test.ts` at line 58, The test currently only checks presence of the discriminator key 'k' (expect(ciphertext.data).toHaveProperty('k')) but should assert its value equals 'ct'; update the assertions that inspect ciphertext payloads (e.g., where you reference ciphertext.data) to check expect(ciphertext.data.k).toBe('ct') (and similarly replace loose has-property checks at the other occurrences) so the tests verify the discriminator value, not just key presence.
🤖 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/protect/__tests__/number-protect.test.ts`:
- Around line 307-310: The comment is contradictory with the assertions: update
the comment to state new encryptions SHOULD have the discriminator 'k' (or
adjust to the intended expectation), and change the three expect lines on
encryptedData.data[i].data to assert the explicit discriminator value (e.g.,
expect(encryptedData.data[0].data.k).toBe('k') or
expect(encryptedData.data[0].data).toHaveProperty('k',
<expectedDiscriminator>)), doing the same for indices 1 and 2; ensure the test
checks the discriminator value explicitly rather than only presence, and make
the comment reflect that explicit check.
In `@packages/schema/src/index.ts`:
- Line 353: The schema currently allows any numeric version while
buildEncryptConfig emits only 1; update the encryptConfigSchema and the
TypeScript type in schema/type to require v: 1 (not number) so validation fails
for other versions, and adjust any dependent inference so buildEncryptConfig's
output type matches; then ensure EncryptionClient.init uses encryptConfigSchema
validation (or its parsed type) to reject invalid configs at schema-validation
time rather than later in the FFI by invoking the validator and handling
failures before continuing.
In `@packages/stack/__tests__/number-protect.test.ts`:
- Around line 288-291: The test comment is stale and the assertions contradict
it: update the comment to state that encrypted items should include a
discriminator 'k' with value 'ct', and change the assertions on
encryptedData.data[0].data, encryptedData.data[1].data, and
encryptedData.data[2].data to assert that the property 'k' === 'ct' (i.e.,
validate the discriminator value rather than only checking existence).
---
Nitpick comments:
In `@packages/protect/__tests__/json-protect.test.ts`:
- Line 58: The test currently only checks presence of the discriminator key 'k'
(expect(ciphertext.data).toHaveProperty('k')) but should assert its value equals
'ct'; update the assertions that inspect ciphertext payloads (e.g., where you
reference ciphertext.data) to check expect(ciphertext.data.k).toBe('ct') (and
similarly replace loose has-property checks at the other occurrences) so the
tests verify the discriminator value, not just key presence.
In `@packages/stack/__tests__/json-protect.test.ts`:
- Line 61: The tests currently assert only the presence of the discriminator key
with expect(ciphertext.data).toHaveProperty('k'); update these assertions to
assert the exact discriminator value 'ct' (e.g., use toHaveProperty('k', 'ct')
or expect(ciphertext.data.k).toBe('ct')) so the ciphertext contract is locked;
apply the same change for the other similar assertions in this file that
reference ciphertext.data (and any ciphertext2/ciphertext3 usages).
🪄 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: 969eecc3-fae8-4b80-9c9f-1a6c068ea869
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.changeset/protect-ffi-0-22-0.md.claude/skills/prisma-next-extension-upgradepackages/cli/src/sql/cipherstash-encrypt-no-operator-family.sqlpackages/cli/src/sql/cipherstash-encrypt-supabase.sqlpackages/cli/src/sql/cipherstash-encrypt.sqlpackages/drizzle/__tests__/integration-test-helpers.tspackages/prisma-next/migrations/20260601T0000_install_eql_bundle/ops.jsonpackages/prisma-next/src/migration/eql-install.generated.tspackages/protect-dynamodb/src/helpers.tspackages/protect-dynamodb/src/operations/search-terms.tspackages/protect/__tests__/backward-compat.test.tspackages/protect/__tests__/json-protect.test.tspackages/protect/__tests__/number-protect.test.tspackages/protect/__tests__/searchable-json-pg.test.tspackages/protect/package.jsonpackages/schema/__tests__/schema.test.tspackages/schema/src/index.tspackages/stack/__tests__/backward-compat.test.tspackages/stack/__tests__/json-protect.test.tspackages/stack/__tests__/number-protect.test.tspackages/stack/__tests__/schema-builders.test.tspackages/stack/package.jsonpackages/stack/src/dynamodb/helpers.tspackages/stack/src/schema/index.tspnpm-workspace.yamlskills-lock.jsonskills/prisma-next-extension-upgrade
| } | ||
|
|
||
| if (!term?.hm) { | ||
| if (term?.k !== 'ct' || !term.hm) { |
There was a problem hiding this comment.
This isn't correct but its in part because the types from protectjs-ffi aren't being exposed or used. Might need a follow-up there.
I'll investigate.
There was a problem hiding this comment.
I've got Claude on the case investigating this.
There was a problem hiding this comment.
Investigated — summary for when you pick this thread back up.
This is now resolvable. When you wrote the comment, protect-ffi was still 0.22.0 with the single conflated Encrypted type — there was no query-specific type to narrow to. protectjs-ffi#95 (shipped in protect-ffi@0.23.0, which this PR now pins) splits it: Encrypted is storage-only, EncryptedQuery / EncryptedScalarQuery are the query payloads. The current search-terms.ts narrowing (k === 'ct' → 'hm' in term) is correct against those types. I've also got a staged change adding an isEncryptedScalarQuery type guard to @cipherstash/protect, so this file narrows via protect-ffi's exact EncryptedScalarQuery type instead of hand-rolled property checks.
Bonus finding while tracing the createSearchTerms chain — there's a latent bug-shaped trap next door:
There are two SearchTermsOperation classes in @cipherstash/protect:
| File | FFI call | Marked | Used by |
|---|---|---|---|
ffi/operations/search-terms.ts |
encryptBulk |
not deprecated | nobody |
ffi/operations/deprecated/search-terms.ts |
encryptQueryBulk |
@deprecated |
createSearchTerms (ffi/index.ts) |
The wiring is inverted. The live, correct path is the deprecated file (encryptQueryBulk → genuine query terms). The non-deprecated file is dead code — imported by nobody — and uses encryptBulk (storage encryption), which is wrong for search terms: it produces full storage ciphertexts where hm is present only if the column happens to have a unique index. That's exactly the storage-vs-query confusion you flagged, frozen into a file. It stayed invisible because pre-#95 both FFI functions returned the same conflated type.
Same orphaned encryptBulk-based file exists in the stack mirror (packages/stack/src/encryption/operations/search-terms.ts), with no deprecated counterpart.
Suggest deleting both dead files in a small follow-up cleanup — out of scope for this upgrade PR, but worth an issue so nobody edits the dead file thinking it's live (their change would silently do nothing, or "fixing" it would reintroduce the bug).
…build The 4 'WHERE comparison: = equality' tests assert STE-vec entry equality behaviour and error wording that match no published EQL release (verified 2.1.x-2.3.x) — they passed only against an unreleased EQL build on the old shared CI database. With CI now pinned to a published EQL image, they fail. Skip them; re-calibrate as part of the EQL 2.3 upgrade (#473).
protect-ffi 0.23.0 is a types-only release that splits the previously-
conflated `Encrypted` type into:
- `Encrypted` — storage-only payload returned by encrypt/encryptBulk,
the only shape decrypt accepts. `c` is now required (was `c?`).
- `EncryptedQuery` (new) — query payload returned by encryptQuery /
encryptQueryBulk for scalar unique/match/ore lookups and ste_vec
selector path queries. Carries no ciphertext (`c?: never`) so the
union discriminates cleanly via `'c' in payload`. JSON containment
queries (ste_vec_term) still come back as a storage-shaped
`Encrypted`, hence the union return.
- `isEncrypted` arg widened to `unknown`.
- `EncryptedSteVecStorage` collapsed into `EncryptedSteVec`.
No runtime change; consumers that lied about the loose union now have
to narrow properly.
Stack-side updates:
- `EncryptedSearchTerm` and `EncryptedQueryResult` (in protect and
stack) widen to `Encrypted | EncryptedQuery | string [| null]`.
- `formatEncryptedResult`, `encryptedToCompositeLiteral`, and
`encryptedToEscapedCompositeLiteral` accept the union via a new
internal `EncryptedQueryTerm = CipherStashEncrypted |
CipherStashEncryptedQuery` alias.
- `BatchEncryptQueryOperation.assembleResults` takes the union for the
FFI's bulk return shape.
- `protect-dynamodb`'s `SearchTermsOperation` narrows via
`'hm' in term && typeof term.hm === 'string'` so the new
`EncryptedScalarQuery & { hm }` / `& { bf }` / `& { ob }` discriminator
unions narrow cleanly without reading off the loose union.
Verified: pnpm test (1876 passed | 20 skipped, 14/14 packages) and
prisma-next e2e (36/36 across the seven shape suites). No DB-side
changes needed since the runtime is unchanged from 0.22.0 → 0.23.0.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The branch reconstructing the SteVec storage payload from DynamoDB
attributes was gated on `cast_as === 'jsonb'`, but protect-ffi's
`CastAs` union has no `'jsonb'` member — the value the schema emits
for JSON columns is `'json'`. The branch was dead, so JSON / ste_vec
columns were silently rebuilt as a scalar `{k:'ct', c}` payload,
losing the `sv` array on the way back out.
The sibling `packages/stack/src/dynamodb/helpers.ts` already uses
`'json'` correctly — the two files had drifted.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #476 (merged before this branch's rebase) pinned every EQL install script and the postgres-eql CI container at eql-2.2.1. This branch upgrades the code to emit the EQL v2.3 payload format, so every pin must move in lockstep — otherwise CI runs the new bytes against the old EQL extension and JSON inserts hit `_encrypted_check_c`. Flipped: - `.github/workflows/tests.yml` — container image `ghcr.io/cipherstash/postgres-eql:17-2.2.1` → `17-2.3.1`. - `local/docker-compose.yml` — same tag; the bench workflow (`tests-bench.yml`) consumes this compose file via `docker compose up --wait`. - `packages/cli/src/installer/index.ts` and `packages/drizzle/src/bin/generate-eql-migration.ts` — `EQL_VERSION` constant. - The three docs the `EQL_VERSION` pin appears in (`packages/protect/README.md`, `packages/drizzle/README.md`, `packages/drizzle/GENERATE_EQL_MIGRATION_CLI.md`). The vendored SQL in `packages/cli/src/sql/*.sql` and the prisma-next install bundle were already eql-2.3.1 from earlier in this PR; the example app's `examples/prisma/migrations/cipherstash/install_eql_bundle/` copy was also synced. `@cipherstash/schema` no longer bundles any `.sql` (verified — no `*.sql` under `packages/schema/`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
18875b5 to
1bed6f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/stack/__tests__/number-protect.test.ts (1)
288-291:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale comment and validate discriminator value.
The comment contradicts the assertions and should reflect EQL v2.3 discriminator behavior. The assertions should also validate the discriminator value
'ct'rather than just checking for presence.📝 Proposed fix
- // Forward compatibility: new encryptions should NOT have k field - expect(encryptedData.data[0].data).toHaveProperty('k') - expect(encryptedData.data[1].data).toHaveProperty('k') - expect(encryptedData.data[2].data).toHaveProperty('k') + // Forward compatibility: new encryptions should include k: 'ct' discriminator + expect(encryptedData.data[0].data).toHaveProperty('k', 'ct') + expect(encryptedData.data[1].data).toHaveProperty('k', 'ct') + expect(encryptedData.data[2].data).toHaveProperty('k', 'ct')🤖 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__/number-protect.test.ts` around lines 288 - 291, Update the stale comment to state that EQL v2.3 uses a discriminator and then change the three assertions that currently check for presence of 'k' to explicitly assert the discriminator equals 'ct' for each entry (i.e., check encryptedData.data[0].data.discriminator === 'ct', encryptedData.data[1].data.discriminator === 'ct', encryptedData.data[2].data.discriminator === 'ct'). Ensure you reference the encryptedData variable and its .data array entries when making the replacement so the test validates the discriminator value rather than merely the presence of 'k'.
🤖 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 @.github/workflows/tests.yml:
- Around line 16-19: Update the stale justification comment that mentions
"protect-ffi 0.21.x": change the reference to reflect the current bump to
`@cipherstash/protect-ffi` 0.23.0 (or remove the specific minor-version phrasing
and say "protect-ffi 0.23.x") so it matches the EQL image pin "eql-2.3.1"
referenced in the same comment; edit the comment block that discusses the
Postgres + EQL test image and its lockstep relationship with protect-ffi to use
the correct package version string (`@cipherstash/protect-ffi` 0.23.0) or a
generalized "0.23.x" wording for future-proofing.
In `@packages/stack/package.json`:
- Line 206: The PR title and possibly the source branch name are out of sync
with the package.json change: package "`@cipherstash/protect-ffi`" was bumped to
0.23.0 but the PR title (and likely branch protect-ffi-0-22-0) still reference
0.22.0; update the PR title to reflect "upgrade protect-ffi to 0.23.0" and
rename or update the source branch/description if autogenerated so
changelog/release notes match the package.json change for
`@cipherstash/protect-ffi` v0.23.0.
---
Duplicate comments:
In `@packages/stack/__tests__/number-protect.test.ts`:
- Around line 288-291: Update the stale comment to state that EQL v2.3 uses a
discriminator and then change the three assertions that currently check for
presence of 'k' to explicitly assert the discriminator equals 'ct' for each
entry (i.e., check encryptedData.data[0].data.discriminator === 'ct',
encryptedData.data[1].data.discriminator === 'ct',
encryptedData.data[2].data.discriminator === 'ct'). Ensure you reference the
encryptedData variable and its .data array entries when making the replacement
so the test validates the discriminator value rather than merely the presence of
'k'.
🪄 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: e682f838-39d6-4b64-8f19-297f6bf9b5cf
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (44)
.changeset/protect-ffi-0-23-0.md.claude/skills/prisma-next-extension-upgrade.github/workflows/tests.ymlexamples/prisma/migrations/cipherstash/20260601T0000_install_eql_bundle/migration.jsonexamples/prisma/migrations/cipherstash/20260601T0000_install_eql_bundle/ops.jsonlocal/docker-compose.ymlpackages/cli/src/installer/index.tspackages/cli/src/sql/cipherstash-encrypt-no-operator-family.sqlpackages/cli/src/sql/cipherstash-encrypt-supabase.sqlpackages/cli/src/sql/cipherstash-encrypt.sqlpackages/drizzle/GENERATE_EQL_MIGRATION_CLI.mdpackages/drizzle/README.mdpackages/drizzle/__tests__/integration-test-helpers.tspackages/drizzle/src/bin/generate-eql-migration.tspackages/prisma-next/migrations/20260601T0000_install_eql_bundle/migration.jsonpackages/prisma-next/migrations/20260601T0000_install_eql_bundle/ops.jsonpackages/prisma-next/src/migration/eql-bundle.tspackages/prisma-next/src/migration/eql-install.generated.tspackages/protect-dynamodb/src/helpers.tspackages/protect-dynamodb/src/operations/search-terms.tspackages/protect/README.mdpackages/protect/__tests__/backward-compat.test.tspackages/protect/__tests__/json-protect.test.tspackages/protect/__tests__/number-protect.test.tspackages/protect/__tests__/searchable-json-pg.test.tspackages/protect/package.jsonpackages/protect/src/ffi/operations/batch-encrypt-query.tspackages/protect/src/helpers/index.tspackages/protect/src/types.tspackages/schema/__tests__/schema.test.tspackages/schema/src/index.tspackages/stack/__tests__/backward-compat.test.tspackages/stack/__tests__/json-protect.test.tspackages/stack/__tests__/number-protect.test.tspackages/stack/__tests__/schema-builders.test.tspackages/stack/package.jsonpackages/stack/src/dynamodb/helpers.tspackages/stack/src/encryption/helpers/index.tspackages/stack/src/encryption/operations/batch-encrypt-query.tspackages/stack/src/schema/index.tspackages/stack/src/types.tspnpm-workspace.yamlskills-lock.jsonskills/prisma-next-extension-upgrade
✅ Files skipped from review due to trivial changes (16)
- local/docker-compose.yml
- packages/drizzle/src/bin/generate-eql-migration.ts
- pnpm-workspace.yaml
- packages/drizzle/GENERATE_EQL_MIGRATION_CLI.md
- skills-lock.json
- .changeset/protect-ffi-0-23-0.md
- examples/prisma/migrations/cipherstash/20260601T0000_install_eql_bundle/migration.json
- packages/prisma-next/migrations/20260601T0000_install_eql_bundle/migration.json
- packages/protect/package.json
- packages/protect/README.md
- packages/prisma-next/src/migration/eql-bundle.ts
- .claude/skills/prisma-next-extension-upgrade
- packages/protect/tests/number-protect.test.ts
- skills/prisma-next-extension-upgrade
- packages/drizzle/README.md
- packages/protect/tests/json-protect.test.ts
…tack into protect-ffi-0-22-0
Fixes the actionable findings CodeRabbit raised on the rebased PR
state.
- `.github/workflows/tests.yml` — the comment justifying the
postgres-eql:17-2.3.1 image pin still said "protect-ffi 0.21.x".
After the 0.23.0 bump that's a stale lockstep reference; flip to
0.23.x so future maintainers don't read it backwards.
- `packages/schema/src/index.ts` — tighten `encryptConfigSchema.v`
from `z.number()` to `z.literal(1)`. `buildEncryptConfig` only
emits `1`, and protect-ffi 0.22.0+ rejects any other value at
`newClient` with `UNSUPPORTED_CONFIG_VERSION`; enforcing it at
the zod layer fails bad configs earlier (and surfaces a cleaner
error than crossing into the native module first).
- `packages/{protect,stack}/__tests__/number-protect.test.ts` —
the "// Forward compatibility: new encryptions should NOT have
k field" comment contradicted the asserting branch right below
(post-EQL-v2.3 the discriminator IS present). Reword to match
the actual contract and tighten `toHaveProperty('k')` to
`toHaveProperty('k', 'ct')`.
- `packages/{protect,stack}/__tests__/json-protect.test.ts` —
same tightening for all sites. These columns are typed
`dataType('json')` without `searchableJson()`, so no ste_vec
index is configured and the FFI emits a scalar payload — `'ct'`
is the correct discriminator at every assertion site.
Verified: pnpm build (10/10 packages, full TURBO), pnpm test
(14/14 packages, 1876 passed | 20 skipped).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add an `isEncryptedScalarQuery` type guard to `@cipherstash/protect` that narrows a value to protect-ffi's `EncryptedScalarQuery` — a scalar query term carrying no ciphertext and exactly one of `hm`/`bf`/`ob`. Use it in protect-dynamodb's `SearchTermsOperation` in place of the hand-rolled `k`/`c`/`hm` property checks, so the narrowing is centralised, self-documenting, and tied to protect-ffi's exact query type. No behaviour change.
Summary by CodeRabbit
Chores
New Features
Breaking Changes