Update EQL v3 concrete type names via FFI 0.28#570
Conversation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🦋 Changeset detectedLatest commit: 175b1ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
tobyhede
force-pushed
the
chore/eql-v3-ffi-0-27
branch
from
da69a6b to
1fdb1f5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.changeset/eql-v3-ffi-0-27-concrete-types.md:
- Around line 1-5: This changeset is marked as patch but the EQL v3
factory/domain rename is a breaking API change, so update the release type to
minor. Adjust the changeset entry for `@cipherstash/stack` to reflect the new
version bump, keeping the note about upgrading `@cipherstash/protect-ffi` and the
renamed concrete type factories/domains (Integer, Smallint, Boolean, Real,
Double, Bigint) as the reason for the higher bump.
In `@packages/stack/__tests__/schema-v3.test.ts`:
- Line 436: The parameterized schema-v3 test labels are stale and still
reference the old `int4_ord` naming, which no longer matches the renamed domain
builders. Update the case labels in `schema-v3.test.ts` to use the new
`IntegerOrd` and `IntegerOrdOre` names (matching `types.IntegerOrd` and
`types.IntegerOrdOre` / their emitted `eql_v3.integer_ord` and
`eql_v3.integer_ord_ore` output) so the test output stays consistent with the
renamed symbols.
🪄 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: 96442036-7c23-4cd7-abf6-647e5b3bdbe8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.changeset/eql-v3-ffi-0-27-concrete-types.md.changeset/eql-v3-typed-schema.mdpackages/stack/__tests__/cjs-require.test.tspackages/stack/__tests__/encrypt-lock-context-guards.test.tspackages/stack/__tests__/schema-v3-client.test.tspackages/stack/__tests__/schema-v3-pg.test.tspackages/stack/__tests__/schema-v3.test-d.tspackages/stack/__tests__/schema-v3.test.tspackages/stack/__tests__/typed-client-v3.test-d.tspackages/stack/__tests__/v3-matrix/catalog.tspackages/stack/__tests__/v3-matrix/matrix-bulk.test.tspackages/stack/__tests__/v3-matrix/matrix-live-pg.test.tspackages/stack/__tests__/v3-matrix/matrix-live.test.tspackages/stack/__tests__/v3-matrix/matrix.test-d.tspackages/stack/package.jsonpackages/stack/src/encryption/helpers/index.tspackages/stack/src/encryption/operations/batch-encrypt-query.tspackages/stack/src/eql/v3/columns.tspackages/stack/src/eql/v3/index.tspackages/stack/src/eql/v3/types.tspackages/stack/src/types.ts
tobyhede
force-pushed
the
chore/eql-v3-ffi-0-27
branch
from
f67d08d to
15e63a8
Compare
- Bump stack dependency and the e2e Deno wasm-inline pin to 0.28.0 - Regenerate lockfile - 0.28 widens JsPlaintext to include bigint (native i64 round-tripping); update the v3 type test to assert bigint plaintext is now accepted - Rename/refresh the FFI changeset for 0.28
…ngeset) The prior commit f84e4c1 captured only the changeset rename; the actual content edits were left unstaged. This commit carries them: - packages/stack/package.json: protect-ffi 0.27.0 → 0.28.0 - e2e/wasm/deno.json: wasm-inline pin → 0.28.0 - pnpm-lock.yaml: regenerated for 0.28.0 - schema-v3.test-d.ts: assert bigint plaintext now accepted (JsPlaintext widened) - changeset body: 0.27.0 → 0.28.0
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Test coverage review
Verdict: well covered, with two gaps. This PR is largely a mechanical rename of the EQL v3 concrete domains (int4→integer, int2→smallint, float4→real, float8→double, bool factory → Boolean) to match protect-ffi 0.28 / the regenerated SQL bundle, plus type-widening (Encrypted, EncryptedQuery unions) with no new runtime logic. The rename surface is thoroughly exercised: the runtime matrix.test.ts asserts getEqlType(), getQueryCapabilities(), isQueryable() and build() (incl. cast_as) for every renamed domain from the catalog, and the new schema-v3.test.ts "concrete type names" block explicitly pins each factory (including the Boolean → eql_v3.bool name divergence). The bigint-omission and per-item bulkDecrypt behaviors have new tests.
Two gaps worth closing are posted inline:
- Nothing cross-checks the domain names the SDK emits against the checked-in SQL bundle. The
types.tsdocstring explicitly warns that name drift is "a version mismatch, not a harmless naming drift" — butmatrix.test.tsonly proves the builder and the catalog agree with each other, not with the fixture. Since this PR's whole purpose is to keep those in lockstep, a cheap non-live guard is worth adding. - The rewritten
bulkDecrypterror-code test only exercises an all-malformed batch; the defining property of the fallible per-item API (a valid item still returns{ data }when a malformed sibling returns{ error }) is untested. This mirrors the known "one axis tested, the discriminating axis not" anti-pattern.
Out of scope (noted, not blocking)
- The widened
EncryptedQuery = CipherStashEncryptedQuery | CipherStashEncryptedV3QueryandEncrypted = CipherStashEncryptedPayloadare type-only changes with no explicit.test-d.tsassertion that a v3jsonb_queryneedle is assignable toEncryptedQuery. A one-lineexpectTypeOfregression guard would be cheap, but such an assertion is close to tautological, so I did not post it inline.
| 'eql_v3.int2_ord_ore': { builder: types.Int2OrdOre, ColumnClass: EncryptedInt2OrdOreColumn, castAs: 'number', capabilities: ORD, indexes: ORE_IDX, samples: INT2_S, errorSamples: NUM_ERR }, | ||
| 'eql_v3.int2_ord': { builder: types.Int2Ord, ColumnClass: EncryptedInt2OrdColumn, castAs: 'number', capabilities: ORD, indexes: ORE_IDX, samples: INT2_S, errorSamples: NUM_ERR }, | ||
| // integer | ||
| 'eql_v3.integer': { builder: types.Integer, ColumnClass: EncryptedIntegerColumn, castAs: 'number', capabilities: STORAGE, indexes: NONE, samples: INTEGER_S, errorSamples: NUM_ERR }, |
There was a problem hiding this comment.
Gap: The renamed domain names (eql_v3.integer, eql_v3.smallint, eql_v3.real, eql_v3.double, …) are only checked for internal consistency — matrix.test.ts asserts the builder emits what this catalog says, but nothing asserts those names actually exist as domains in the checked-in cipherstash-encrypt-v3.sql bundle. The types.ts docstring warns that a name mismatch here is "a version mismatch, not a harmless naming drift," yet the only thing that would catch such drift today is the live Postgres suite (gated). A cheap, non-live guard reading the fixture closes exactly the invariant this PR exists to maintain.
// packages/stack/__tests__/v3-matrix/domain-names-in-sync.test.ts
import { readFile } from 'node:fs/promises'
import { dirname, resolve } from 'node:path'
import { fileURLToPath } from 'node:url'
import { describe, expect, it } from 'vitest'
import { V3_MATRIX } from './catalog'
const fixture = resolve(
dirname(fileURLToPath(import.meta.url)),
'../fixtures/eql-v3/cipherstash-encrypt-v3.sql',
)
describe('v3 domain names stay in sync with the SQL bundle', () => {
it('every emitted eqlType exists as a CREATE DOMAIN in the fixture', async () => {
const sql = await readFile(fixture, 'utf8')
const declared = new Set(
[...sql.matchAll(/CREATE DOMAIN (eql_v3\.[a-z0-9_]+) AS jsonb/g)].map(
(m) => m[1],
),
)
// Subset, not equality: the bundle also declares int8_* domains the SDK
// intentionally omits pending lossless bigint round-tripping.
for (const eqlType of Object.keys(V3_MATRIX)) {
expect(declared.has(eqlType), `${eqlType} missing from SQL bundle`).toBe(
true,
)
}
})
})Expected: passes on this branch; fails the moment an emitted name drifts from the regenerated bundle (e.g. a stray eql_v3.int4 left behind, or a fixture regen that renames a domain without updating the SDK).
| } | ||
|
|
||
| expect(result.data).toHaveLength(2) | ||
| expect(result.data.every((item) => 'error' in item)).toBe(true) |
There was a problem hiding this comment.
Gap: This rewrite now documents the fallible per-item behavior, but only for an all-malformed batch. The property that actually changed — a valid ciphertext still decrypts to { data } while a malformed sibling is isolated as { error } in the same call — is untested. This is the "one axis tested, discriminating axis not" anti-pattern: the all-invalid case would also pass under a hypothetical regression that fails the whole batch as soon as one item is bad.
it(
'isolates malformed items without failing valid siblings',
async () => {
const good = await protectClient.encrypt('valid@example.com', {
column: testSchema.email,
table: testSchema,
})
if (good.failure) throw new Error(good.failure.message)
const mixed = [
{ data: good.data },
{ data: { i: { t: 'test_table', c: 'email' }, v: 2, c: 'invalid' } },
] as unknown as BulkDecryptPayload
const result = await protectClient.bulkDecrypt(mixed)
if (result.failure) {
throw new Error(`Expected per-item errors, got ${result.failure.type}`)
}
expect(result.data).toHaveLength(2)
expect('data' in result.data[0]).toBe(true) // valid item survives
expect('error' in result.data[1]).toBe(true) // malformed item isolated
},
FFI_TEST_TIMEOUT,
)Expected: the valid entry round-trips to { data: 'valid@example.com' } and the malformed entry carries a per-item { error }, proving failure isolation rather than the all-or-nothing case the current test covers.
tobyhede
changed the title
|
Superseded by #575 — branch renamed |
Code review — EQL v3 concrete type renameAutomated multi-angle review (xhigh effort): 10 finder angles → verification → sweep. Scope: the PR diff vs Verdict: the rename is clean and safe to ship. The mechanical rename ( Findings (ranked)1. 2. 3. 4. 5. 6. 7. 8. 9. Cleared (checked, not findings)Efficiency; conventions/changeset-format; the 🤖 Generated with automated multi-agent code review via Claude Code. |
2 participants
Summary
@cipherstash/protect-ffito0.27.0.Integer,Smallint,Boolean,Real,Double, andBigint.Validation
pnpm --filter @cipherstash/stack buildpnpm exec vitest run __tests__/schema-v3.test.ts __tests__/v3-matrix/matrix.test.tspnpm exec vitest --run --typecheck.only __tests__/schema-v3.test-d.ts __tests__/typed-client-v3.test-d.ts __tests__/v3-matrix/matrix.test-d.tsNotes
Summary by CodeRabbit
New Features
Datevalues in v3 encryption and query encryption flows.Bug Fixes