feat(stack): EQL v3 typed schema + strongly-typed client (@cipherstash/stack/schema/v3, /v3)#535
feat(stack): EQL v3 typed schema + strongly-typed client (@cipherstash/stack/schema/v3, /v3)#535tobyhede wants to merge 59 commits into
Conversation
🦋 Changeset detectedLatest commit: 63fe076 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 |
|
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:
📝 WalkthroughWalkthroughThis PR adds a v3 ChangesEQL v3 text_search schema DSL and client type widening
CLI dotenv loading and non-PTY test helper
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/stack/src/types.ts (1)
111-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePossible redundant union arm.
If
EncryptedColumnalready has a publicgetEqlType()method and no private/protected members forcing nominal typing, the explicitEncryptedColumnarm is structurally subsumed byBuildableColumn & { getEqlType(): string }and could be dropped for simplicity. Not a functional issue either way; only worth simplifying if confirmed redundant.🤖 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/src/types.ts` around lines 111 - 113, The BuildableQueryColumn union appears to have a redundant arm if EncryptedColumn already satisfies BuildableColumn & { getEqlType(): string } structurally. Check the EncryptedColumn type and, if it does not rely on nominal typing via private/protected members, simplify the BuildableQueryColumn alias in types.ts by removing the explicit EncryptedColumn union member and keeping only the shared structural form.
🤖 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.
Nitpick comments:
In `@packages/stack/src/types.ts`:
- Around line 111-113: The BuildableQueryColumn union appears to have a
redundant arm if EncryptedColumn already satisfies BuildableColumn & {
getEqlType(): string } structurally. Check the EncryptedColumn type and, if it
does not rely on nominal typing via private/protected members, simplify the
BuildableQueryColumn alias in types.ts by removing the explicit EncryptedColumn
union member and keeping only the shared structural form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01405e99-c5e9-4153-8af3-10759fd8ebbd
📒 Files selected for processing (16)
.changeset/eql-v3-text-search.md.github/workflows/tests.ymldocs/superpowers/plans/2026-06-30-eql-v3-text-search-schema-plan.mddocs/superpowers/specs/2026-06-30-eql-v3-text-search-schema-design.mdpackages/stack/__tests__/schema-v3.test-d.tspackages/stack/__tests__/schema-v3.test.tspackages/stack/package.jsonpackages/stack/src/encryption/helpers/infer-index-type.tspackages/stack/src/encryption/operations/bulk-encrypt.tspackages/stack/src/encryption/operations/encrypt.tspackages/stack/src/schema/index.tspackages/stack/src/schema/v3/index.tspackages/stack/src/types.tspackages/stack/tsconfig.typecheck.jsonpackages/stack/tsup.config.tspackages/stack/vitest.config.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/tests/helpers/run.ts (1)
44-87: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo timeout/kill safeguard for a hung child.
If the spawned CLI process hangs (e.g., waiting on unexpected input despite
stdio: ['ignore', ...]), nothing here kills it — the test will eventually time out via vitest, but the orphaned child process keeps running. Consider an optional timeout that callschild.kill()and rejects.🤖 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/cli/tests/helpers/run.ts` around lines 44 - 87, The run helper currently waits indefinitely for the spawned CLI in run() and only resolves on close, so a hung child can outlive the test. Update packages/cli/tests/helpers/run.ts by adding an optional timeout to RunOptions and wiring it in run() to call child.kill() and reject if the process does not exit in time, while preserving the existing stdout/stderr capture and cleanup in the child.on('close') path.
🤖 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.
Nitpick comments:
In `@packages/cli/tests/helpers/run.ts`:
- Around line 44-87: The run helper currently waits indefinitely for the spawned
CLI in run() and only resolves on close, so a hung child can outlive the test.
Update packages/cli/tests/helpers/run.ts by adding an optional timeout to
RunOptions and wiring it in run() to call child.kill() and reject if the process
does not exit in time, while preserving the existing stdout/stderr capture and
cleanup in the child.on('close') path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8faee085-09ef-4411-822e-50addd54c10c
📒 Files selected for processing (5)
packages/cli/src/bin/main.tspackages/cli/tests/e2e/runner-aware-help.e2e.test.tspackages/cli/tests/e2e/smoke.e2e.test.tspackages/cli/tests/helpers/run.tspackages/stack/src/schema/v3/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/tests/e2e/smoke.e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stack/src/schema/v3/index.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stack/src/schema/v3/index.ts (1)
460-470: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSnapshot nested
freeTextSearch()options when you store them.Lines 465-466 keep
opts.tokenizerandopts.token_filtersby reference, so mutating the caller’s options object after configuration silently changes this builder’s laterbuild()output. The rest of this class is explicitly avoiding shared nested state, so this should clone on write too.Suggested fix
freeTextSearch(opts?: MatchIndexOpts): this { // A fresh defaults object per call supplies the `?? ` fallbacks, so no // nested default object is ever shared into `this.matchOpts` by reference. const defaults = defaultMatchOpts() this.matchOpts = { - tokenizer: opts?.tokenizer ?? defaults.tokenizer, - token_filters: opts?.token_filters ?? defaults.token_filters, + tokenizer: opts?.tokenizer + ? { ...opts.tokenizer } + : { ...defaults.tokenizer }, + token_filters: opts?.token_filters + ? opts.token_filters.map((f) => ({ ...f })) + : defaults.token_filters.map((f) => ({ ...f })), k: opts?.k ?? defaults.k, m: opts?.m ?? defaults.m, include_original: opts?.include_original ?? defaults.include_original, } return this🤖 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/src/schema/v3/index.ts` around lines 460 - 470, The freeTextSearch method in the v3 schema builder is still storing nested options by reference, so later mutations to the caller’s MatchIndexOpts can leak into build() output. Update freeTextSearch in packages/stack/src/schema/v3/index.ts to clone the tokenizer and token_filters values when assigning this.matchOpts, matching the class’s existing clone-on-write approach and avoiding shared nested state.
🧹 Nitpick comments (6)
docs/superpowers/plans/2026-07-01-eql-v3-typed-schema.md (3)
510-510: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix redundant phrasing.
"Repeat the same exact pattern" → "Repeat the same pattern" or "Repeat this exact pattern".
- Repeat the same exact pattern for: + Repeat the same pattern for:🤖 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 `@docs/superpowers/plans/2026-07-01-eql-v3-typed-schema.md` at line 510, Update the phrasing in the plans document to remove redundancy: change the “Repeat the same exact pattern for:” text to either “Repeat the same pattern for:” or “Repeat this exact pattern for:”. Locate the sentence in the document section containing that exact phrase and keep the rest unchanged.Source: Linters/SAST tools
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider rewording for readability.
Three successive list items begin with "Schemas with". While this is a list format where parallelism is expected, consider varying the structure if the static analysis tool flagged it as an issue.
- - Schemas with required `hm` support equality. - - Schemas with required `ob` support order/range. - - Schemas with required `bf` support free-text search. + - `hm` required → equality support. + - `ob` required → order/range support. + - `bf` required → free-text search support.🤖 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 `@docs/superpowers/plans/2026-07-01-eql-v3-typed-schema.md` at line 117, Reword the list item about storage-only schemas to avoid repeating the same “Schemas with” sentence pattern as the surrounding bullets. Update the wording in the schema documentation section so it still conveys that schemas containing only v, i, and c are storage-only, but uses a different sentence structure for readability and to satisfy the static analysis warning.Source: Linters/SAST tools
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider rewording for readability.
Three successive sentences beginning with "In" - though this appears to be in the file structure list where parallelism is intentional. Given the context is a structured plan document, this is acceptable but could be tightened.
🤖 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 `@docs/superpowers/plans/2026-07-01-eql-v3-typed-schema.md` at line 27, The plan document wording is a bit repetitive in the file-structure list, where several consecutive bullets/sentences start with the same “In” pattern. Rephrase the affected entries in the schema-plan section to improve readability while preserving the parallel structure, keeping the “BuildableTable” bullet clear and concise.Source: Linters/SAST tools
packages/stack/scripts/install-eql-v3.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBare
dotenv/configre-introduces the banner noise this cohort suppresses elsewhere.The CLI entrypoint now loads env files with
config({ path: '.env.local', quiet: true })specifically to avoid dotenv v17's injected-env banner. This script importsdotenv/configdirectly with no options, so it will still print that banner whenever it runs (e.g. in CI logs).♻️ Suggested fix
-import 'dotenv/config' +import { config } from 'dotenv' import postgres from 'postgres' import { installEqlV3IfNeeded } from '../__tests__/helpers/eql-v3' + +config({ quiet: true })🤖 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/scripts/install-eql-v3.ts` at line 1, The install-eql-v3 script is importing dotenv in a way that re-enables the noisy injected-env banner. Update the startup env loading in this script to use the same explicit dotenv config pattern as the CLI entrypoint, with a fixed .env.local path and quiet enabled, and remove the bare dotenv/config import so the script stays silent in CI. Reference the script’s top-level env bootstrap in install-eql-v3.ts.packages/stack/__tests__/schema-v3.test.ts (1)
2-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise this failure through a public API instead of
resolveIndexType.This test now imports
@/encryption/helpers/infer-index-typedirectly and asserts the helper’s internal error strings, which makes the suite brittle to refactors inside the implementation rather than the supported contract. Please move this misuse coverage to the public entry point that surfaces the same runtime failure. As per coding guidelines, "Prefer testing via public API; avoid reaching into private internals in tests".Also applies to: 661-677
🤖 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 2 - 3, The test is reaching into the private `resolveIndexType` helper and asserting its internal error strings, which makes it brittle. Update `packages/stack/__tests__/schema-v3.test.ts` to remove the direct `@/encryption/helpers/infer-index-type` import and exercise the same failure through the public `encryptConfigSchema`/`encryptedColumn` API instead. Keep the coverage for the misuse case, but assert the runtime failure surfaced by the supported schema entry point rather than helper internals.Source: Coding guidelines
packages/stack/__tests__/schema-v3.test-d.ts (1)
229-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a model-inference case with aliased column names.
This file already proves v3 domain inference for aliased builders like
createdAt→created_at, but theencryptModel/bulkEncryptModelsacceptance cases only cover same-name keys. One typed assertion for an aliasedencryptedTimestamptzColumn('created_at')model field would protect the exact v3 field mapping this PR is widening.🤖 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-d.ts` around lines 229 - 301, Add a typed model-inference test for aliased v3 encrypted columns, since the current `encryptModel` and `bulkEncryptModels` cases only cover same-name fields. Update the `schema-v3.test-d.ts` assertions near the existing `encryptModel`/`bulkEncryptModels` checks to include a model using `encryptedTimestamptzColumn('created_at')` with an aliased property name. Verify the inferred `EncryptionClient.encryptModel` and/or `bulkEncryptModels` result type maps the aliased field correctly while still preserving unrelated fields.
🤖 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 `@docs/superpowers/plans/2026-07-01-eql-v3-typed-schema.md`:
- Around line 63-65: The task steps currently hardcode a developer-specific
absolute path in the read-only references, which will not work for other
environments. Update the instructions in the plan to use repository-relative
paths or an environment-agnostic placeholder, and if needed mention that the
base path must be configured by the user; keep the references to the
inventory.rs and schema/v3/*.json locations clear without embedding a local
machine path.
In `@packages/stack/__tests__/helpers/eql-v3.ts`:
- Around line 28-41: The advisory lock handling in installEqlV3IfNeeded is using
separate sql calls, so the lock and unlock may run on different pooled
connections. Update the function to run the entire check/install/unlock flow on
a reserved connection via sql.reserve(), or replace
pg_advisory_lock/pg_advisory_unlock with pg_advisory_xact_lock if the EQL v3
install path is transaction-safe, and keep the existing hasEqlV3TextSearch and
eqlV3Sql execution logic inside that reserved scope.
In `@packages/stack/__tests__/schema-v3-pg.test.ts`:
- Around line 133-149: The cleanup hooks in the schema-v3-pg test only remove
rows from protect_ci_v3_text_search, so the typed-domain fixture data in
protect_ci_v3_typed_domains is left behind. Update the existing beforeEach and
afterAll hooks in schema-v3-pg.test.ts to also delete rows for the typed-domain
table using the same TEST_RUN_ID guard, alongside the current cleanup logic. Use
the existing beforeEach, afterAll, and sql cleanup blocks as the place to add
the matching protect_ci_v3_typed_domains deletion.
In `@packages/stack/src/types.ts`:
- Around line 151-183: The public BuildableTable shape is too weak for the
encryption inference used by encryptModel() and bulkEncryptModels(), so
structurally accepted tables lose the literal column keys needed by
EncryptedFromBuildableTable. Fix this by either adding the column map
brand/_columnType to the BuildableTable contract and keeping
BuildableTableColumns aligned with it, or by narrowing the affected APIs/types
back to the branded table builder type so the return type reflects encrypted
fields correctly.
---
Outside diff comments:
In `@packages/stack/src/schema/v3/index.ts`:
- Around line 460-470: The freeTextSearch method in the v3 schema builder is
still storing nested options by reference, so later mutations to the caller’s
MatchIndexOpts can leak into build() output. Update freeTextSearch in
packages/stack/src/schema/v3/index.ts to clone the tokenizer and token_filters
values when assigning this.matchOpts, matching the class’s existing
clone-on-write approach and avoiding shared nested state.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-07-01-eql-v3-typed-schema.md`:
- Line 510: Update the phrasing in the plans document to remove redundancy:
change the “Repeat the same exact pattern for:” text to either “Repeat the same
pattern for:” or “Repeat this exact pattern for:”. Locate the sentence in the
document section containing that exact phrase and keep the rest unchanged.
- Line 117: Reword the list item about storage-only schemas to avoid repeating
the same “Schemas with” sentence pattern as the surrounding bullets. Update the
wording in the schema documentation section so it still conveys that schemas
containing only v, i, and c are storage-only, but uses a different sentence
structure for readability and to satisfy the static analysis warning.
- Line 27: The plan document wording is a bit repetitive in the file-structure
list, where several consecutive bullets/sentences start with the same “In”
pattern. Rephrase the affected entries in the schema-plan section to improve
readability while preserving the parallel structure, keeping the
“BuildableTable” bullet clear and concise.
In `@packages/stack/__tests__/schema-v3.test-d.ts`:
- Around line 229-301: Add a typed model-inference test for aliased v3 encrypted
columns, since the current `encryptModel` and `bulkEncryptModels` cases only
cover same-name fields. Update the `schema-v3.test-d.ts` assertions near the
existing `encryptModel`/`bulkEncryptModels` checks to include a model using
`encryptedTimestamptzColumn('created_at')` with an aliased property name. Verify
the inferred `EncryptionClient.encryptModel` and/or `bulkEncryptModels` result
type maps the aliased field correctly while still preserving unrelated fields.
In `@packages/stack/__tests__/schema-v3.test.ts`:
- Around line 2-3: The test is reaching into the private `resolveIndexType`
helper and asserting its internal error strings, which makes it brittle. Update
`packages/stack/__tests__/schema-v3.test.ts` to remove the direct
`@/encryption/helpers/infer-index-type` import and exercise the same failure
through the public `encryptConfigSchema`/`encryptedColumn` API instead. Keep the
coverage for the misuse case, but assert the runtime failure surfaced by the
supported schema entry point rather than helper internals.
In `@packages/stack/scripts/install-eql-v3.ts`:
- Line 1: The install-eql-v3 script is importing dotenv in a way that re-enables
the noisy injected-env banner. Update the startup env loading in this script to
use the same explicit dotenv config pattern as the CLI entrypoint, with a fixed
.env.local path and quiet enabled, and remove the bare dotenv/config import so
the script stays silent in CI. Reference the script’s top-level env bootstrap in
install-eql-v3.ts.
🪄 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: 4aa02b84-db93-4a9b-aa84-d185e2c884d9
📒 Files selected for processing (25)
.changeset/eql-v3-typed-schema.mddocs/query-api-walkthrough.mddocs/superpowers/plans/2026-07-01-eql-v3-typed-schema.mdpackages/stack/__tests__/cjs-require.test.tspackages/stack/__tests__/fixtures/eql-v3/cipherstash-encrypt-v3.sqlpackages/stack/__tests__/helpers/eql-v3.tspackages/stack/__tests__/helpers/stub-auth-wasm-inline.tspackages/stack/__tests__/helpers/stub-protect-ffi-wasm-inline.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__/wasm-inline-column-name.test.tspackages/stack/package.jsonpackages/stack/scripts/install-eql-v3.tspackages/stack/src/encryption/helpers/infer-index-type.tspackages/stack/src/encryption/helpers/model-helpers.tspackages/stack/src/encryption/index.tspackages/stack/src/encryption/operations/bulk-encrypt-models.tspackages/stack/src/encryption/operations/encrypt-model.tspackages/stack/src/encryption/operations/encrypt-query.tspackages/stack/src/encryption/operations/encrypt.tspackages/stack/src/schema/v3/index.tspackages/stack/src/types.tspackages/stack/vitest.config.ts
✅ Files skipped from review due to trivial changes (3)
- docs/query-api-walkthrough.md
- packages/stack/tests/helpers/stub-protect-ffi-wasm-inline.ts
- .changeset/eql-v3-typed-schema.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/stack/tests/wasm-inline-column-name.test.ts
- packages/stack/src/encryption/helpers/infer-index-type.ts
- packages/stack/package.json
- packages/stack/src/encryption/operations/encrypt.ts
tobyhede
force-pushed
the
feat/eql-v3-text-search-schema
branch
from
b54e6d4 to
ed78233
Compare
tobyhede
changed the title
Column builders are copied onto the EncryptedTable instance for accessor access (users.email). A column named build/tableName/columnBuilders/ _columnType would silently overwrite that member — worst case a 'build' column breaks buildEncryptConfig's tb.build() call at runtime. Throw a clear error at table-definition time instead. Scoped to v3; v2 retains its existing behavior. Found by CodeRabbit review.
…ntry The wasm-inline encrypt entry typed opts.column as the widened structural BuildableColumn, but getColumnName still gated on instanceof EncryptedColumn || EncryptedField and threw for a v3 EncryptedTextSearchColumn — a runtime break the type promise hid. Resolve the name structurally (typeof getName) so v3 columns round-trip through WasmEncryptionClient.encrypt(); still throws for non-builder JS input. getColumnName is the only instanceof gate on this path; the rest reads table.tableName structurally. Adds wasm-inline-column-name.test.ts exercising the seam (v2 column/field + v3 column + non-builder). Like its sibling wasm-inline-normalize.test.ts the suite cannot load in environments missing the @cipherstash/protect-ffi /wasm-inline dep subpath.
Config tables are keyed by name, so two tables with the same tableName silently dropped the earlier one. Add a v3-only additive guard that throws on a duplicate (Object.hasOwn). v2's buildEncryptConfig keeps its existing silent-overwrite behavior (no-v2-change constraint).
The RESERVED_TABLE_KEYS guard only covered own members (build, tableName, columnBuilders, _columnType), so a column named constructor/toString/valueOf/ hasOwnProperty was assigned as an own property, shadowing the Object.prototype member. Add an `in` check (isReservedTableKey) so any prototype-chain member is also rejected, keeping the table object well-behaved for reflection.
…freeTextSearch' for match A v3 text_search column emits unique+ore+match, and shared index inference picks by priority unique > match > ore. So encryptQuery without an explicit queryType builds an EQUALITY term (via unique) — a substring matches nothing. Document on EncryptedTextSearchColumn + encryptedTextSearchColumn that callers must pass queryType:'freeTextSearch' (FFI 'match') for free-text queries. Addresses review finding #2 (naming footgun; doc-only, no runtime change).
Add table-driven runtime tests for all 40 EQL v3 domain builders (name, eqlType, capabilities, config, queryability) plus type-level tests for nominal domain distinctness, InferPlaintext mapping, queryability of BuildableQueryColumn, and v3/v2 model inference.
test(stack): eql v3 domain test-matrix + equality-via-ORE fix
The structural builder contracts (BuildableColumn, BuildableQueryColumn, BuildableV3QueryableColumn, BuildableTable, BuildableTableColumns) and the encryptModel/bulkEncryptModels return-type mapper (EncryptedFromBuildableTable) appear in public return positions but were not re-exported from `@cipherstash/stack/types`, so consumers could not name them — an inconsistency with the already-exposed `EncryptedFromSchema`. No build breakage (the mapped types were emitted inline); this closes the nameability gap. Regression guard: types-public-surface.test-d.ts imports each contract from the public `@/types-public` entrypoint (a missing re-export fails typecheck). Note: these types are inherited from the base branch (feat/eql-v3-text-search-schema, PR #535); the export is added here in response to review feedback on the stacked PR.
Bug:
|
Review findings (10)Multi-angle review of this branch vs main; every finding below was independently re-verified against the code (several with tsc/runtime repros). Ordered most-severe first. The 1.
|
|
Follow-up on the Domain { name: "ord", terms: &[Term::Hm, Term::Ore] },
Domain { name: "ord_ore", terms: &[Term::Hm, Term::Ore] },
Domain { name: "ord_ope", terms: &[Term::Hm, Term::Ope] },
Domain { name: "search", terms: &[Term::Hm, Term::Ore, Term::Bloom] },with this design rationale in the doc comment:
Three consequences for this PR:
All three are the same lesson: the capability→term mapping has a single Rust source of truth with generated JSON/TS bindings, and every hand-written re-derivation of it (here, and the one previously flagged on protectjs-ffi#104) has drifted in a way the bindings would have made unrepresentable. Until |
The structural builder contracts (BuildableColumn, BuildableQueryColumn, BuildableV3QueryableColumn, BuildableTable, BuildableTableColumns) and the encryptModel/bulkEncryptModels return-type mapper (EncryptedFromBuildableTable) appear in public return positions but were not re-exported from `@cipherstash/stack/types`, so consumers could not name them — an inconsistency with the already-exposed `EncryptedFromSchema`. No build breakage (the mapped types were emitted inline); this closes the nameability gap. Regression guard: types-public-surface.test-d.ts imports each contract from the public `@/types-public` entrypoint (a missing re-export fails typecheck). Note: these types are inherited from the base branch (feat/eql-v3-text-search-schema, PR #535); the export is added here in response to review feedback on the stacked PR.
The structural builder contracts (BuildableColumn, BuildableQueryColumn, BuildableV3QueryableColumn, BuildableTable, BuildableTableColumns) and the encryptModel/bulkEncryptModels return-type mapper (EncryptedFromBuildableTable) appear in public return positions but were not re-exported from `@cipherstash/stack/types`, so consumers could not name them — an inconsistency with the already-exposed `EncryptedFromSchema`. No build breakage (the mapped types were emitted inline); this closes the nameability gap. Regression guard: types-public-surface.test-d.ts imports each contract from the public `@/types-public` entrypoint (a missing re-export fails typecheck). Note: these types are inherited from the base branch (feat/eql-v3-text-search-schema, PR #535); the export is added here in response to review feedback on the stacked PR.
Replace the 35 verbose `encrypted<Domain>Column` factories with a single `types` namespace whose members mirror the underlying `eql_v3.<name>` domains 1:1 (`types.TextEq`, `types.Int4Ord`, `types.Timestamptz`, …), and split the 992-line `src/schema/v3/index.ts` into a cohesive module under `src/eql/v3/` (`columns.ts`, `types.ts`, `table.ts`, curated `index.ts`). The authoring subpath is renamed `@cipherstash/stack/schema/v3` -> `@cipherstash/stack/eql/v3`; the `./v3` typed-client surface now re-exports the `types` namespace instead of the standalone factories. Behaviour is preserved: same classes, same nominal-typing mechanism, same `build()` output. - Rewire tsup entry, package.json exports/typesVersions/analyze:complexity, the `@/eql/v3` re-export, `[eql/v3]` error prefix, and fta-v3.yml paths. - Migrate all v3 tests + CJS smoke test to `types.*` and the new subpath. - Reconcile the three unreleased changesets and refresh tracked v3 design docs (supersede banners on completed-work records; correct the not-yet-built Stryker gate spec's single-file premise for the 4-file split). Verified: build emits dist/eql/v3; schema/v3 subpath and factories are gone (ERR_PACKAGE_PATH_NOT_EXPORTED, undefined on both subpaths); 101 v3 runtime + 50 type tests pass; FTA scores all 4 files (max 68.68 < 72); e2e authoring config byte-matches expected.
Two JS properties whose builders resolve to the same DB name (getName()) silently overwrote in the built config — the later column won and the first's config was dropped. Throw instead, matching the existing duplicate-tableName guard in buildEncryptConfig and the reserved-key guard in encryptedTable. Regression tests: `EncryptedTable.build()` and `buildEncryptConfig` both throw on a duplicate DB name (schema-v3.test.ts, eql_v3 encryptedTable block).
The structural builder contracts (BuildableColumn, BuildableQueryColumn, BuildableV3QueryableColumn, BuildableTable, BuildableTableColumns) and the encryptModel/bulkEncryptModels return-type mapper (EncryptedFromBuildableTable) appear in public return positions but were not re-exported from `@cipherstash/stack/types`, so consumers could not name them — an inconsistency with the already-exposed `EncryptedFromSchema`. No build breakage (the mapped types were emitted inline); this closes the nameability gap. Regression guard: types-public-surface.test-d.ts imports each contract from the public `@/types-public` entrypoint (a missing re-export fails typecheck). Note: these types are inherited from the base branch (feat/eql-v3-text-search-schema, PR #535); the export is added here in response to review feedback on the stacked PR.
The v3-matrix domain suite (catalog.ts + matrix tests) landed on the base branch via PR #540 after this branch was cut, and used the pre-refactor `@/schema/v3` path and `encrypted<Domain>Column` factories. Retarget it to `@/eql/v3` and the `types.*` namespace so the base's matrix coverage keeps working on top of the refactor. `EqlTypeForColumn` (which #540's catalog.ts consumes) is preserved — ported into eql/v3/columns.ts and re-exported from the barrel during the rebase. Post-rebase reconciliation only; no behavior change.
Close two coverage gaps on the eql/v3 branch that only live/e2e tests
touched:
- encrypt-lock-context-guards: assert NaN/+Inf/-Inf are rejected on the
`encrypt(...).withLockContext(...)` path and short-circuit before the
FFI call. The non-lock guards run only under the live number-protect
suite; the lock-context arm (encrypt.ts:163-168) had no coverage.
- wasm-inline-new-client: assert the protect-ffi 0.25 single-object
`newClient({ strategy, encryptConfig, clientId, clientKey })` shape,
incl. cast_as normalisation. Previously exercised only by the
secret-gated Deno e2e, so a regression to the 0.24 two-arg form would
pass normal CI.
Both run offline (mocked FFI).
The all-35-domain live Postgres suite was force-skipped (describe.skip, not credential-gated) after `beforeAll`'s dynamic INSERT crashed with `invalid input syntax for type json` (PR #540). That crash was a postgres.js serialization gap — a bare ciphertext object stringified to "[object Object]" — and was fixed 32 minutes later by wrapping every INSERT param in `sql.json(...)` (commit 53cf854). The force-skip was simply left stale; it is not an FFI limitation. Restore the credential-gated form (`LIVE_EQL_V3_PG_ENABLED ? describe : describe.skip`) as the file's own comment instructed, so the 35-domain SQL round-trip runs in CI (which supplies DATABASE_URL + CS_* creds) and self-skips locally. The genuine FFI-level skip — timestamptz `cast_as:'date'` time-of-day truncation in schema-v3-client.test.ts — stays skipped (needs a native 'timestamp' cast_as variant). timestamptz matrix cases are unaffected (midnight samples, no truncation).
…equirement)
The `eql_v3.text_ord` and `eql_v3.text_ord_ore` Postgres domains require BOTH
`hm` (HMAC) and `ob` (ORE) in the stored ciphertext — text equality is
HMAC-based (their `eql_v3.eq_term` extracts `hm`), unlike numeric/date order
domains which answer equality via `ob` and need only ORE. The SDK's
`indexesForCapabilities` treated every order/range domain identically, emitting
`ore` only, so text-order ciphertexts lacked `hm` and a real INSERT failed with
`value for domain eql_v3.text_ord_ore violates check constraint`. (Surfaced by
re-enabling matrix-live-pg; masked before by the suite skip.)
Make index derivation castAs-aware: emit `unique` (hm) when equality is
answered via HMAC — equality-only domains of any type, AND text order domains
(`string` + order/range). Numeric/date order domains are unchanged (`ore` only).
Query path follows automatically: `resolvesEqualityViaOre` only fires when
`unique` is absent, so text-order equality now resolves to the `hm` index
(eq_term) while numeric/date order equality still resolves to `ore`.
TDD: text_ord/text_ord_ore build() now emits { unique, ore }; numeric order
stays { ore }; text-order equality resolves to unique. Catalog + matrix build()
assertions updated (TEXT_ORD_IDX). Verified against the eql_v3 domain checks in
the fixture; live SQL runs in CI.
… guards
Test-only additions (separated from the in-flight EQL v3 bundle upgrade so they
land on this branch, not the bundle branch):
- encrypt-lock-context-guards.test.ts: run every non-finite-number guard case
against BOTH a v2 fluent-builder column and a v3 domain column, since the
guard lives on the shared EncryptOperationWithLockContext.
- schema-v3.test.ts: `.freeTextSearch()` no-arg is a no-op (pins the
opts===undefined branch); a text_match mutable-state aliasing guard (base-class
match-clone path, which the text_search-only test can't cover); and
buildEncryptConfig() with zero tables yields { v: 1, tables: {} }.
- wasm-inline-strategy.test.ts: Biome line-wrap formatting only.
- encryption/v3: reconstructRow → rowReconstructor factory — the table config (build() + buildColumnKeyMap()) is row-invariant but was rebuilt per row on the bulk decrypt path; it is now derived once per call site, with date columns resolved up front - encrypt operations: replace the two inline NaN/Infinity guard copies with the existing assertValidNumericValue helper (validation.ts) - schema/match-defaults: single source of truth for the default match index parameters (previously duplicated between the v2 freeTextSearch builder and the v3 domain builders) plus a shared cloneMatchOpts deep-clone used at all three v3 clone sites - tests: one shared live-gate helper (LIVE_CIPHERSTASH_ENABLED / LIVE_EQL_V3_PG_ENABLED + describeLive/describeLivePg) replaces the gate blocks copy-pasted across seven live suites No behavioral changes: emitted encrypt configs are byte-identical (schema-v3 fixture tests unchanged), guard error messages unchanged, gating semantics unchanged.
The structural builder contracts (BuildableColumn, BuildableQueryColumn, BuildableV3QueryableColumn, BuildableTable, BuildableTableColumns) and the encryptModel/bulkEncryptModels return-type mapper (EncryptedFromBuildableTable) appear in public return positions but were not re-exported from `@cipherstash/stack/types`, so consumers could not name them — an inconsistency with the already-exposed `EncryptedFromSchema`. No build breakage (the mapped types were emitted inline); this closes the nameability gap. Regression guard: types-public-surface.test-d.ts imports each contract from the public `@/types-public` entrypoint (a missing re-export fails typecheck). Note: these types are inherited from the base branch (feat/eql-v3-text-search-schema, PR #535); the export is added here in response to review feedback on the stacked PR.
refactor(stack): EQL v3 `types` namespace on @cipherstash/stack/eql/v3
Resolves pending review feedback from #541 (merged). Seven findings: - test: restore the live ORE proof for text order domains. text_ord / text_ord_ore carry both hm (unique) and ob (ore), so the single-kind classifier ran only the eq_term/hmac_256 proof and silently skipped ord_term/ore_block_256 -- a wrong-valued ob would pass the matrix green. Run both proofs; build the ob term with queryType:'orderAndRange' since equality resolves to hm on these domains. - schema: extract a shared resolveMatchOpts() merge+clone helper used by both the v2 and v3 freeTextSearch builders, giving v2 the clone-on-write protection it lacked (it stored caller opts by reference). - eql/v3: derive EncryptedTextSearchColumn.build()'s unique/ore blocks via indexesForCapabilities (override only match) so the emitted index shape cannot drift from the shared capability mapping. - encryption/v3: precompute row reconstructors per schema table at typedClient construction and return a DecryptionError failure for unknown tables, so table.build()'s duplicate-column throw can no longer escape the decrypt Result contract as a promise rejection. - eql/v3: alias V3DecryptedModel to V3ModelInput (character-identical) to stop the input/output model shapes silently drifting. - ci: add src/schema/match-defaults.ts to the fta-v3 path scope -- it shapes every emitted v3 match block but sat outside the gate. - ci: re-baseline FTA --score-cap 72 -> 69 after the eql/v3 file split and refresh the spec doc's stale 71.08 baseline claim.
fix(stack): address EQL v3 types-module review follow-ups
3 participants
EQL v3 typed schema + strongly-typed client (
@cipherstash/stack/eql/v3,@cipherstash/stack/v3)Expands the EQL v3 authoring surface from the single
text_searchbuilder to every generated EQL v3 SQL domain — exposed through a singletypesnamespace whose members mirror the underlyingeql_v3.<name>domains — and adds a strongly-typed v3 client (@cipherstash/stack/v3) that derives input/output types from the schema and rejects misuse at compile time. v2 is unchanged; pick the model by import path.Usage
Mix any v3 domains in one table — each column declares its own type and query capabilities. The
types.*member name maps 1:1 to theeql_v3.<name>domain (stripeql_v3., PascalCase each segment):Plaintext types are inferred per domain, and
Datevalues work directly:Queryability is enforced at compile time — storage-only columns can't be queried:
typesmembers & capabilitiesOne member per generated EQL v3 domain (PascalCase of the
eql_v3.<name>). Each maps to an EQL v3 SQL type and exposesgetQueryCapabilities()/isQueryable().types.Int4int4Eqtypes.TextEqtext_eqOrd,OrdOretypes.Int4Ordint4_ordTextMatchtypes.TextMatchtext_matchTextSearchtypes.TextSearchtext_searchCovered domains:
int2/int4,float4/float8,numeric,date,timestamptz,text*,bool→ inferred asnumber/Date/string/boolean.int8/bigintis intentionally omitted pending lossless FFI round-tripping (a JSbigintcannot be marshalled by the native@cipherstash/protect-ffibuild, andnumberloses precision above 2^53).types.TextSearchkeeps the chainable.freeTextSearch(opts)tuner — the only capability-bearing chain; every other domain is fully described by its type. Each factory returns the CONCRETE branded column class (never the widenedAnyEncryptedV3Column), so per-column plaintext / query-capability inference stays precise.Strongly-typed client (
@cipherstash/stack/v3)A dedicated, definitively-typed client surface for v3 schemas.
EncryptionV3mirrorsEncryption;typedClientretypes an existing client. Both re-export the v3typesnamespace and table API, so a single import provides everything needed to author and use a schema.Every method derives its types from the concrete
table/columnarguments:encrypt/encryptQuerypin the plaintext to the column's domain type (text → string,timestamptz → Date, …).encryptQueryadditionally constrainsqueryTypeto the column's capabilities and rejects storage-only columns at compile time.encryptModel/bulkEncryptModelsvalidate schema-column fields against their inferred plaintext type (passthrough fields likeidare untouched) and return a precise encrypted model.decryptModel/bulkDecryptModelsreturn the precise plaintext model, reconstructingDatevalues from the encrypt-configcast_as.Because the typed methods bind to the concrete branded v3 classes, a hand-rolled structural table/column — or a column borrowed from a different table's domain — is rejected. This closes the soundness gap where a non-branded structural table could be encrypted at runtime while typed as plaintext.
Also
encryptModel/bulkEncryptModelsaccept any v3 table (structuralBuildableTable); v2 unchanged.encrypt/encryptQueryvalue type widened withDate(newPlaintexttype) — the FFI declaresDateas a cast target but omits it from itsJsPlaintextinput union.bigintis intentionally excluded until the FFI supports lossless bigint I/O.text_searchstays byte-identical to a v2equality().orderAndRange().freeTextSearch()column. The only runtime addition is the per-columnDatereconstruction on the typed client's model-decrypt paths.Structure
src/eql/v3/{columns,types,table,index}.ts, exported on the@cipherstash/stack/eql/v3subpath.types.*is the single authoring API — there are no standaloneencrypted<Domain>Columnfactories.src/encryption/v3.ts, exported on@cipherstash/stack/v3, and re-exports the fulleql/v3surface (types, table API, inference aliases).Notes
Pre-existing on this branch, not introduced here:
pnpm build's dts step may fail onsrc/wasm-inline.ts(missing@cipherstash/authdep) in some environments, and live client/pg tests require CipherStash credentials. v3 unit + type tests are green, and v3 dist artifacts build (dist/encryption/v3.*,dist/eql/v3/*).The typed client's decrypt reconstruction is gated on a live round-trip check (needs credentials): if the FFI already returns
Date, the reconstruction can be dropped as a pure-type optimization — it is idempotent and safe either way.