feat: upgrade protect-ffi to 0.23.0#473
Open
calvinbrewer wants to merge 6 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: 1bed6f7 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 |
📝 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Comment on lines
307
to
+310
| // Forward compatibility: new encryptions should NOT have k field | ||
| expect(encryptedData.data[0].data).not.toHaveProperty('k') | ||
| expect(encryptedData.data[1].data).not.toHaveProperty('k') | ||
| expect(encryptedData.data[2].data).not.toHaveProperty('k') | ||
| expect(encryptedData.data[0].data).toHaveProperty('k') | ||
| expect(encryptedData.data[1].data).toHaveProperty('k') | ||
| expect(encryptedData.data[2].data).toHaveProperty('k') |
There was a problem hiding this comment.
Fix contradictory forward-compat note and make the check explicit.
Line 307 says new payloads should not have k, but Lines 308-310 assert the opposite. Update the comment and assert the expected discriminator value.
Suggested diff
- // 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'
+ expect(encryptedData.data[0].data).toHaveProperty('k', 'ct')
+ expect(encryptedData.data[1].data).toHaveProperty('k', 'ct')
+ expect(encryptedData.data[2].data).toHaveProperty('k', 'ct')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Forward compatibility: new encryptions should NOT have k field | |
| expect(encryptedData.data[0].data).not.toHaveProperty('k') | |
| expect(encryptedData.data[1].data).not.toHaveProperty('k') | |
| expect(encryptedData.data[2].data).not.toHaveProperty('k') | |
| 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' | |
| 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/protect/__tests__/number-protect.test.ts` around lines 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.
| ): EncryptConfig { | ||
| const config: EncryptConfig = { | ||
| v: 2, | ||
| v: 1, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Tighten the version contract in schema/type to v: 1 as well.
buildEncryptConfig now emits only 1, but encryptConfigSchema still accepts any number. Please align validation/type inference so invalid config versions fail at schema-validation time, not later in FFI.
Suggested patch
export const encryptConfigSchema = z.object({
- v: z.number(),
+ v: z.literal(1),
tables: tablesSchema,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| v: 1, | |
| export const encryptConfigSchema = z.object({ | |
| v: z.literal(1), | |
| tables: tablesSchema, | |
| }) |
🤖 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/schema/src/index.ts` at 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.
Contributor
There was a problem hiding this comment.
@coderdan I think the version contract regression from the config is my fault. It should be 2. The fix needs to be made in cipherstash client too.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
288
to
+291
| // Forward compatibility: new encryptions should NOT have k field | ||
| expect(encryptedData.data[0].data).not.toHaveProperty('k') | ||
| expect(encryptedData.data[1].data).not.toHaveProperty('k') | ||
| expect(encryptedData.data[2].data).not.toHaveProperty('k') | ||
| expect(encryptedData.data[0].data).toHaveProperty('k') | ||
| expect(encryptedData.data[1].data).toHaveProperty('k') | ||
| expect(encryptedData.data[2].data).toHaveProperty('k') |
There was a problem hiding this comment.
Update stale comment and assert the expected discriminator value.
The comment on Line 288 contradicts the assertions on Lines 289-291. Align the wording and validate k as 'ct'.
Suggested diff
- // 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'
+ expect(encryptedData.data[0].data).toHaveProperty('k', 'ct')
+ expect(encryptedData.data[1].data).toHaveProperty('k', 'ct')
+ expect(encryptedData.data[2].data).toHaveProperty('k', 'ct')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Forward compatibility: new encryptions should NOT have k field | |
| expect(encryptedData.data[0].data).not.toHaveProperty('k') | |
| expect(encryptedData.data[1].data).not.toHaveProperty('k') | |
| expect(encryptedData.data[2].data).not.toHaveProperty('k') | |
| 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' | |
| 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, 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).
coderdan
reviewed
May 21, 2026
| } | ||
|
|
||
| if (!term?.hm) { | ||
| if (term?.k !== 'ct' || !term.hm) { |
Contributor
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.
coderdan
added a commit
that referenced
this pull request
May 21, 2026
…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).
3 tasks
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>
calvinbrewer
force-pushed
the
protect-ffi-0-22-0
branch
from
18875b5 to
1bed6f7
Compare
calvinbrewer
changed the title
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
Comment on lines
16
to
19
| # Postgres + EQL for the integration tests. Official EQL image — | ||
| # PostgreSQL 17 with EQL pre-installed via /docker-entrypoint-initdb.d. | ||
| # Pinned to eql-2.2.1 to match the EQL payload format the code emits | ||
| # Pinned to eql-2.3.1 to match the EQL payload format the code emits | ||
| # (protect-ffi 0.21.x); bump in lockstep with the protect-ffi upgrade. |
There was a problem hiding this comment.
Stale comment: references protect-ffi 0.21.x after the 0.23.0 bump.
The justification block says the EQL image is pinned to match protect-ffi 0.21.x, but this PR moves @cipherstash/protect-ffi to 0.23.0 and the EQL image to 17-2.3.1. Refresh the comment so future maintainers don't read a misleading lockstep reference.
📝 Suggested fix
- # Postgres + EQL for the integration tests. Official EQL image —
- # PostgreSQL 17 with EQL pre-installed via /docker-entrypoint-initdb.d.
- # Pinned to eql-2.3.1 to match the EQL payload format the code emits
- # (protect-ffi 0.21.x); bump in lockstep with the protect-ffi upgrade.
+ # Postgres + EQL for the integration tests. Official EQL image —
+ # PostgreSQL 17 with EQL pre-installed via /docker-entrypoint-initdb.d.
+ # Pinned to eql-2.3.1 to match the EQL payload format the code emits
+ # (protect-ffi 0.23.x); bump in lockstep with the protect-ffi upgrade.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Postgres + EQL for the integration tests. Official EQL image — | |
| # PostgreSQL 17 with EQL pre-installed via /docker-entrypoint-initdb.d. | |
| # Pinned to eql-2.2.1 to match the EQL payload format the code emits | |
| # Pinned to eql-2.3.1 to match the EQL payload format the code emits | |
| # (protect-ffi 0.21.x); bump in lockstep with the protect-ffi upgrade. | |
| # Postgres + EQL for the integration tests. Official EQL image — | |
| # PostgreSQL 17 with EQL pre-installed via /docker-entrypoint-initdb.d. | |
| # Pinned to eql-2.3.1 to match the EQL payload format the code emits | |
| # (protect-ffi 0.23.x); bump in lockstep with the protect-ffi upgrade. |
🤖 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 @.github/workflows/tests.yml around lines 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.
| "dependencies": { | ||
| "@byteslice/result": "0.2.0", | ||
| "@cipherstash/protect-ffi": "0.21.4", | ||
| "@cipherstash/protect-ffi": "0.23.0", |
There was a problem hiding this comment.
PR title is out of sync with the actual version bump.
The PR title says feat: upgrade protect-ffi to 0.22.0 but this diff bumps @cipherstash/protect-ffi to 0.23.0 (and the stack context/changeset describe a 0.23.0 upgrade). Update the PR title (and source branch description if it's autogenerated from protect-ffi-0-22-0) so changelog/release notes are accurate.
🤖 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/package.json` at 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.
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
3 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.
Summary by CodeRabbit
Chores
Breaking Changes