feat(protect-ffi): vendor the package and run its own checks - #862
Merged
Conversation
Adds end-to-end handling for JS Date values. Previously, `cast_as: 'date'` was advertised in the config but the plaintext conversion layer had no path to or from date types — encrypt and decrypt both failed. - New `cast_as: 'timestamp'` -> `ColumnType::Timestamp` alongside the existing 'date' -> `ColumnType::Date` (day precision). - `JsPlaintext::Date(DateTime<Utc>)` variant maps to `Plaintext::Timestamp`; string arms in `to_plaintext_with_type` parse ISO 8601 / YYYY-MM-DD for callers who pass a JS Date via `d.toISOString()`. - Decrypt returns a plain RFC 3339 string; callers wrap with `new Date(...)` if they want a Date object (JSON has no native Date type, so this keeps the wire format honest). - Error messages no longer echo the user's input string, preventing a secret that gets mistakenly routed to a date column from appearing in the error that propagates to the caller.
feat: support date and timestamp plaintexts
Bumps aws-lc-rs 1.16.1 -> 1.16.3, which in turn bumps aws-lc-sys 0.38.0 -> 0.40.0 (>= 0.39.0). Addresses GHSA-9f94-5g5w-gf6r and GHSA-394x-vwmw-crm3.
Addresses GHSA-82j2-j2ch-gfr8 (out-of-bounds panic in bit_string_flags via the issuingDistributionPoint CRL extension).
fix(deps): patch aws-lc-sys to 0.40.0
fix(deps): patch vite to 8.0.10
fix(deps): patch rustls-webpki to 0.103.13
Set GLIBC version using cargo-zigbuild
Bump cipherstash-client, cts-common, and stack-profile from 0.34.1-alpha.2 to 0.34.1-alpha.4 and adapt protect-ffi to the breaking API changes: - ColumnType::Utf8Str -> ColumnType::Text, ColumnType::JsonB -> ColumnType::Json - Plaintext::Utf8Str -> Plaintext::Text, Plaintext::JsonB -> Plaintext::Json - EqlEncryptOpts gained a required decryption_policy field - IndexType gained an Ope variant; handle it alongside Ore in the index-name helpers
…ent-0.34.1-alpha.4 chore(deps): upgrade cipherstash-client to 0.34.1-alpha.4
Bumps cipherstash-client, cts-common, and stack-profile from 0.34.1-alpha.4 to 0.34.1-alpha.5 to pick up shared schema and config types ahead of the CanonicalEncryptionConfig migration. `IndexType::SteVec` gained a required `mode: SteVecMode` field in this release; the FFI sets it to `SteVecMode::default()` (`Compat` at alpha.5) so existing behaviour is preserved. No public TypeScript API change.
Surfaces the `mode` option on SteVec indexes through the config API, letting callers choose between `compat` and `standard` encoding. Previously the FFI hard-coded `SteVecMode::default()`, giving callers no way to opt in to the newer encoding. How: - Add the `SteVecMode` TypeScript type (`'compat' | 'standard'`). - Add a `mode` field to the Rust `SteVecIndexOpts` struct with `#[serde(default)]` so configs that omit it continue to parse. - Thread the parsed value through to `IndexType::SteVec` instead of always falling back to `SteVecMode::default()`. - Document the option in the JSONB API reference. Backwards compatibility: configs that omit `mode` keep using the upstream library default, which at this commit (cipherstash-client alpha.5) is still `Compat`. The follow-up alpha.7 bump flips the upstream default to `Standard` — see the CanonicalEncryptionConfig refactor commit for the user-visible breaking change that introduces.
Adds `.worktrees/` to .gitignore so scratch worktrees created under the repo root don't show up as untracked. Personal workflow convention; not used by CI or other contributors.
Bumps cipherstash-client, cipherstash-config, cipherstash-core, cts-common, stack-auth, stack-profile, and zerokms-protocol from alpha.5 to alpha.7. Required so the FFI can deserialize encrypt configs directly into `CanonicalEncryptionConfig` and reuse upstream validation (config version check, ste_vec/match plaintext-type rules). BREAKING (latent at this commit, becomes user-visible once the follow-up CanonicalEncryptionConfig refactor swaps the FFI types over): - `SteVecMode::default()` flips from `Compat` to `Standard` at the library level. Because the FFI currently parses `mode` through its own struct that falls back to `SteVecMode::default()` when the field is omitted, any ste_vec index that omits `mode` will now resolve to `Standard` instead of `Compat`. The two encodings are NOT cross-compatible — stored data indexed under `Compat` cannot be queried under `Standard`. Callers that need to preserve the previous behaviour must pin `mode: 'compat'` explicitly. The migration docs commit later in this branch records the full set of breaking changes and the caller-facing migration recipes.
Adds a TypeScript translation layer that converts the public, JS-friendly EncryptConfig vocabulary into the canonical vocabulary that cipherstash-config's CanonicalEncryptionConfig expects. Not yet wired into `newClient`; the wiring lands in the follow-up "normalize encrypt config vocabulary at the FFI boundary" commit. Why translate in TypeScript rather than rename in Rust: - Keeps the public TypeScript API stable for existing callers (`cast_as: 'string' | 'number' | 'bigint' | ...`). - Lets the native config adopt upstream's canonical names (`text`, `float`, `big_int`) without leaking them into the JS interface. - Future vocabulary tweaks can ship as TS-only changes without another Rust release. How: - Remap `cast_as` values that have no canonical equivalent: `string` → `text`, `number` → `float`, `bigint` → `big_int`. All other values pass through unchanged. - Inject `array_index_mode: 'none'` on any `ste_vec` index that omits the field. The upstream library defaults to `'all'`, so without this we would silently change array-indexing behaviour for existing configs (see the migration design doc earlier in this branch). - Leave `mode` untouched. Omitted `mode` follows the upstream default (`Standard` at alpha.7) — this is a documented breaking change, surfaced once the wiring lands. - Never mutate the caller's config object; build a fresh `NativeEncryptConfig` and return it. Includes unit tests covering each remapped value, the `array_index_mode` default injection, pass-through of canonical values, immutability of the input, and the no-op cases.
…Config
Removes the 688-line `encrypt_config.rs` module and deserializes
the encrypt config directly into cipherstash-config's
`CanonicalEncryptionConfig`. Eliminates duplicate type definitions
across the FFI and the shared schema crate, picks up upstream
validation (version check, ste_vec/match plaintext-type rules)
for free, and lets future schema changes propagate without an FFI
patch.
How:
- Drop `mod encrypt_config` and import `CanonicalEncryptionConfig`
and `Identifier` from `cipherstash_client::schema` directly.
- Change `NewClientOptions::encrypt_config` to
`CanonicalEncryptionConfig`.
- Replace the bespoke `Error::SteVecRequiresJsonCastAs` and
`Error::Config(String)` variants with
`Error::Config(#[from] ConfigError)`, surfacing the upstream
error verbatim.
- Rename the old `Error::Config(String)` to
`Error::Credentials(String)` to reflect its actual usage
(only emitted from SecretKey hex parsing). Note: the
`#[error("Configuration error: {0}")]` display template is
left unchanged for this variant — a follow-up to tighten the
wording is captured in the migration doc.
BREAKING CHANGES (visible to TS callers — see
docs/canonical-encryption-config-migration.md for migration
recipes):
1. SteVec `mode` default: `Compat` → `Standard`. Any ste_vec
config that omits `mode` now indexes new writes under
`Standard` encoding. The two encodings are NOT
cross-compatible: data indexed under `Compat` cannot be
queried under `Standard`, and vice versa. Pin `mode: 'compat'`
explicitly to preserve the pre-migration behaviour for stored
data, or plan a re-encryption of affected columns.
2. `match` index now requires a text-family `cast_as` (`'text'`
or `'string'`). Previously unvalidated; now fails at
`newClient` (mapped to `MATCH_REQUIRES_TEXT` by the follow-up
FFI-boundary wiring commit).
3. Config `v` must equal `1`. Previously unchecked; other values
now fail at `newClient` (mapped to `UNSUPPORTED_CONFIG_VERSION`
by the follow-up FFI-boundary wiring commit).
4. Config-validation error message text now comes from upstream
`ConfigError` and is worded differently. `ProtectError.code`
values are preserved, so consumers branching on `code` are
unaffected; consumers string-matching on `err.message` for
config-validation errors must update.
BREAKING CHANGE: ste_vec indexes that omit `mode` now use
`Standard` encoding instead of `Compat`. Pin `mode: 'compat'` or
plan re-encryption of stored data.
BREAKING CHANGE: `match` indexes now require a text-family
`cast_as` (`'text'` or `'string'`); previously unvalidated configs
will fail at `newClient`.
BREAKING CHANGE: encrypt config `v` must equal `1`; other values
fail at `newClient` instead of being silently accepted.
BREAKING CHANGE: config-validation error message wording changed
(error codes preserved); consumers string-matching on
`err.message` must update.
Wires the `normalizeEncryptConfig` helper (added earlier in this
branch) into the public `newClient` entry point so callers keep
using the JS-friendly `EncryptConfig` vocabulary while the native
side receives the canonical `CanonicalEncryptionConfig` shape
produced by the preceding refactor.
How:
- Pipe `opts.encryptConfig` through `normalizeEncryptConfig`
before passing it to `native.newClient`.
- Introduce an internal `NativeNewClientOptions` type so the
native module declaration reflects the post-normalization shape
(`NativeEncryptConfig`) without leaking it from the public API.
Error-code mapping:
- `inferErrorCode` now recognises three message fragments produced
by upstream `ConfigError`:
- `'requires plaintext_type: json'` → `STE_VEC_REQUIRES_JSON_CAST_AS`
(existing code; substring updated to match the new wording).
- `'requires plaintext_type: text'` → `MATCH_REQUIRES_TEXT`
(new code).
- `'unsupported config version'` → `UNSUPPORTED_CONFIG_VERSION`
(new code).
- Adds `MATCH_REQUIRES_TEXT` and `UNSUPPORTED_CONFIG_VERSION` to
the exported `ProtectErrorCode` union.
BREAKING:
- `ProtectError` messages for config-validation errors are now
worded as `ConfigError` emits them. Consumers branching on
`ProtectError.code` are unaffected; consumers string-matching
on `err.message` must update their match strings.
- The exported `ProtectErrorCode` union gains two new values
(`MATCH_REQUIRES_TEXT`, `UNSUPPORTED_CONFIG_VERSION`).
Exhaustive switches over `ProtectErrorCode` will need
additional cases to stay exhaustive (TS will flag missing
cases when `--strict` is on).
BREAKING CHANGE: two new `ProtectErrorCode` values exist
(`MATCH_REQUIRES_TEXT`, `UNSUPPORTED_CONFIG_VERSION`); exhaustive
switches over the union need additional cases.
BREAKING CHANGE: config-validation error message text is now
sourced from upstream `ConfigError`; consumers string-matching on
`err.message` must update.
Adds `docs/canonical-encryption-config-migration.md` describing the four breaking changes that ship with the CanonicalEncryptionConfig migration, with migration recipes for each: 1. SteVec `mode` default: `Compat` → `Standard` (most impactful; existing data must be re-encrypted or `mode: 'compat'` pinned). Calls out explicitly that the two encodings are not cross-compatible. 2. `match` index now requires text-family `cast_as`. 3. Config `v` must equal `1`. 4. ConfigError message text differs from the old hand-rolled error wording. `ProtectError.code` values are preserved. Also expands `docs/jsonb-api-reference.md` with: - The full `cast_as` vocabulary table showing the public ↔ canonical mapping (so callers can debug error messages that reference canonical names). - Validation rules for `v`, `ste_vec`, and `match` together with the error codes they emit. - A SteVec `mode` section warning that re-encryption is required when changing modes. - The two new `ProtectErrorCode` values (`MATCH_REQUIRES_TEXT`, `UNSUPPORTED_CONFIG_VERSION`). Includes a "explicitly not changed" section calling out that `array_index_mode` still defaults to `'none'` at the FFI boundary (the TS helper injects it), and a "follow-ups" section recording two non-blocking polish items (unit tests for `inferErrorCode` and the `Error::Credentials` display template). Documentation only.
Fixes two doc-drift issues caught after the migration docs landed: - The JSDoc on `SteVecMode` (src/index.cts) still claimed `compat` was the default. After the alpha.7 bump the runtime default is `standard`. Update the comment to match runtime behaviour and add a hint that callers should pin `compat` explicitly to preserve pre-alpha.7 encoding for stored data. - The `cast_as` union snippet in docs/jsonb-api-reference.md was missing `'text'` and `'timestamp'` (added by the migration). Update the snippet to mirror the public `CastAs` union exported from src/index.cts. Documentation only.
Adds an integration test exercising newClient with legacy cast_as values plus an ste_vec config without mode, and three negative cases asserting the ProtectError codes MATCH_REQUIRES_TEXT, UNSUPPORTED_CONFIG_VERSION, and STE_VEC_REQUIRES_JSON_CAST_AS — replacing the manual verification steps from the migration PR.
The cipherstash-client 0.34.1-alpha.7 default flipped SteVec encoding from `Compat` to `Standard`, which collapses the old `b3`/`ocf`/`ocv` SteVec entry fields into two: - Scalar strings and numbers share a single orderable field `oc` (CLLW ORE with tagged-plaintext domain separation). - Booleans, null, arrays, and objects produce an `hm` HMAC-SHA256. Update the integration tests to assert against `oc`/`hm` instead of the retired `b3`/`ocf`/`ocv` names. Restructure the `unique index field (b3)` block as `HMAC index field (hm)` and exercise the non-orderable types (root object, booleans) that actually produce HMAC entries under Standard mode.
The cipherstash-client 0.34.1-alpha.7 default flipped SteVec encoding from `Compat` to `Standard`, collapsing the old `b3`/`ocf`/`ocv` fields. Update the JSONB API reference to match the runtime: - Replace `b3`/`ocf`/`ocv` in the EqlCiphertext / EqlCiphertextBody type snippets with the current fields (`oc` for Standard SteVec, `op` for Compat SteVec, `opf`/`opv` for non-SteVec OPE indexes). - Note that `hm` now also covers SteVec MAC entries (objects, arrays, booleans, null), not just the standalone `unique` index. - Add a small table summarising which entry field each JSON value type produces, plus a mention of the Compat-mode `op` variant. - Update the storage and term-query example outputs accordingly. Documentation only.
Root-level `hm` is HMAC-SHA256 for unique (exact) indexes; SteVec MAC entries live under `sv`, not at the root.
Collapses three near-identical remap tests and the canonical-values loop into table-driven `it.each` blocks.
…ipherstash-client refactor: use OPE & consistent config types from `cipherstash-client`
Adopts the EQL v2.3 storage schema: EqlCiphertext is now a discriminated
union keyed on `k` ("ct" for scalar payloads, "sv" for SteVec), and
encrypt_eql returns a Vec<EqlOutput> separating storage ciphertexts from
query payloads. Storage SteVec payloads now place the root ciphertext at
`sv[0].c` rather than at the root.
`encrypt` / `encrypt_bulk` unwrap `EqlOutput::Store` via a new
`into_store_ciphertext` helper (they always run with `EqlOperation::Store`).
`encrypt_query` / `encrypt_query_bulk` return `EqlOutput` directly —
upstream now derives `Serialize` / `Deserialize` and is `#[serde(untagged)]`,
so the wire shape is determined by the inner `EqlCiphertext` / `EqlQueryPayload`
`k` tag.
Updates the FFI call sites, the TS Encrypted type, integration tests, and
the JSONB API reference to match the new wire format.
cipherstash-client 0.35.0 moved the SteVec storage root ciphertext from the top-level `c` to `sv[0].c`. Update the three remaining assertions in json-containment.test.ts (and the structural-comparison log lines) so they match the new wire format.
Three things the note got wrong or left out, all checked against the pinned serde-wasm-bindgen 0.6.5 rather than reasoned about. **The lookup is not `Reflect::get`.** `deserialize_struct` reads each expected field with `ObjectExt::get_with_ref_key`, a wasm-bindgen `indexing_getter` — plain `obj[key]`. Same prototype semantics, wrong name. **`deserialize_map` tries `js_sys::try_iter` FIRST**; `Object::entries` is only the fallback, with no `Map`-only guard on the iterator arm — unlike `deserialize_any`. So an options object carrying `Symbol.iterator` is read through the iterator and its own properties are ignored entirely, which is the silent-drop class this marker closes, reopened on a shape almost nobody passes. `Object::assign` does not close it: an own enumerable `[Symbol.iterator]` survives the copy. An array of `[k, v]` pairs and a JS `Map` also become accepted where `deserialize_struct` rejected them, and the array form bypasses `encode_plaintext`, so a `bigint` plaintext loses precision above 2^53. **Two diagnostics regressions were undocumented.** A misspelled REQUIRED field now reports `missing field \`indexType\`` and never names `indexTyp`; the `expected one of ...` list is gone from every rejection. Neon-only — the wasm path had no error to lose. Both are now asserted, not just described. Also corrects the scope of the narrowing: the clones in `wasm.rs` are shallow, so a nested `LockContext` is read from the caller's own object on every entry point, and `encode_plaintext_list` returns `opts` untouched when nothing needed encoding — which includes a legitimate empty `plaintexts` — so the bulk entries' top-level bag is narrowed too. Own non-enumerable properties are dropped as well, not only inherited ones. And the per-key allocation cost the map path adds for valid input is written down, roughly 5N `String`s on an N-payload `encryptBulk` where there were none. The CHANGELOG gains the two boundary asymmetries it was short: function- and symbol-valued unknown keys, and the `JSON.stringify` throw on a circular or `bigint` value. Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
feat: converge the wasm and Neon interfaces, and declare real types on the wasm build (#142)
fix!: reject unknown option keys instead of dropping them (#144)
Clippy errors on wasm32, all pre-existing, none of them a bug — the point is that nothing was telling us. `--all-targets` in the lint task means all target *kinds* (lib, bins, tests, benches), not all platform targets, so wasm32 has never been linted. Unused imports: Neon-only pieces of `cipherstash_client` that `lib.rs` imported ungated. `wasm.rs` imports its own copies of the ones it needs straight from the crate, so gating them costs the wasm build nothing. `once_cell::OnceCell` holds the Tokio runtime the Neon exports block on, and `BTreeMap` is used only by gated code. One duplicated attribute: `mod wasm;` in lib.rs already carries `#[cfg(target_arch = "wasm32")]`, so the inner `#![cfg(...)]` in wasm.rs restated it. Dropped, with the reason recorded in the module docs so it does not come back. The dead-code errors this originally also fixed — `EnsureKeysetOpts` / `EnsureKeysetResult` — arrived on main with #147, which moved them into `client_options.rs` already gated. What that comment did not carry is why: the gate keeps the lint honest, it is not an endorsement of the split. `ensureKeyset` is missing from the wasm surface by oversight — the module docs used to call it a deliberate boundary (provisioning belongs on your server), and that reads well but does not hold up: wasm ships to servers too. Corrected in both places, because the reason it went unnoticed generalises: `ensureKeyset`'s only caller in this repo is an integration test, and one of the eighteen integration files loads the wasm build. A missing export is invisible when no test on that target would have called it. Filed as #149; taking those gates off is what marks it done. Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
The two gaps this closes are the same gap: a check nothing invokes reads exactly like a check that passes. `mise run lint:rust` is now an aggregate over three arms — clippy for the host, clippy for wasm32, and `cargo fmt --check`. It keeps the name CI already called, so the step gets strictly more coverage without a rename. The wasm arm lints the lib only; the unit tests are host-run. `npm test` now reaches `test:format:rust`, which has sat in package.json with no caller. That also makes the README's claim about `npm test` true again — it said it formatted and linted Rust, and it did neither. Drops the `cargo check --target wasm32-unknown-unknown` step from test.yml: clippy checks as it lints, so it was doing that work twice. `src/lintWiring.test.ts` guards the call graph rather than the checks. Its general form — no `test:*` script unreachable from `npm test`, no `lint:rust:*` task the aggregate skips — is what catches the next orphan, not just this one. Exemptions have to name a reason. Verified it fails on each regression it claims to catch: re-orphaning `test:format:rust`, dropping the wasm arm from `depends`, and CI calling clippy directly instead of the entry point. No changelog entry: nothing here changes the published surface. Claude-Session: https://claude.ai/code/session_01YJekcEBAsUg8qJoBcqyzBx
`Error` is a 14-variant enum, several carrying structured fields. All of it was discarded at the FFI boundary: Neon exports returned `extract::Error`, whose `TryIntoJs` is `cx.error(cause.to_string())`, and `wasm.rs` did the same via `js_error(&e.to_string())`. Only the message crossed. Each variant that JS can act on now carries `#[diagnostic(code(..))]`, and both boundaries read it onto `err.code`. Values are unchanged, so this half is additive on its own — the JS side that stops inferring them is the next commit. Notes on the shape, since two parts of the issue's proposal did not survive contact: - `#[diagnostic(transparent)]` on the `#[error(transparent)]` variants buys nothing. cipherstash-client, stack-auth, cipherstash-config and eql-bindings contain zero `#[diagnostic(code(..))]` and no manual `code` impls between them — they use `Diagnostic` for `help()` text only — so inheriting would inherit `None`. Six of the eleven wrapped types do not implement `Diagnostic` at all. The codes are therefore ours, which also settles the issue's worry about coupling to upstream naming: there is nothing to couple to. - `Error::Config` is split into four variants. Three published codes (`STE_VEC_REQUIRES_JSON_CAST_AS`, `MATCH_REQUIRES_TEXT`, `UNSUPPORTED_CONFIG_VERSION`) are sub-variants of one upstream `ConfigError`, and the derive cannot compute a code from inner state. `From<ConfigError> for Error` routes them by variant, so an upstream rename is a compile error where the substring match it replaces would have silently degraded to `UNKNOWN`. `#[error(transparent)]` on all four keeps the message identical. The Neon exports had to move their bodies into `do_*` helpers returning `Result<_, Error>`, mirroring what `wasm.rs` already does. `TryIntoJs` is sealed behind a private module, so no type declared here can implement it, and `extract::with` — which defers conversion until the JS thread and hands it a `Cx` — is the only hook for setting a property on the thrown error. That opaque return type is not something `?` can convert into, hence the split. Two things the wasm entry gains beyond the code itself: - `newClient` routed `into_config_map`, `ZeroKMSBuilder::build` and `ScopedCipher::init` through `js_error` rather than `error_to_js`. The divergence was invisible while no code was being carried; it meant the three config codes arrived bare on this entry. - `WasmDecryptResult` is gone. It existed only to describe the missing `code` — the field was synthesised by the Neon JS wrapper, which this build has no equivalent of. Both entries now name one `DecryptResult`. `UNKNOWN_QUERY_OP` is the one code that could not be derived from the variant the error was built as. #143 moved `queryOp` parsing into `query_op.rs`, where an unknown value is rejected inside `Deserialize` — which is what makes the failure name the field rather than surfacing later from query preparation — and serde's `de::Error::custom` takes a `Display`, so nothing typed reaches the boundary. `Error::unknown_query_op` recovers it from the message prefix, and `From<serde_json::Error>` / `wasm::from_js_value` route both entries through it. That is the same prefix match `src/errors.ts` was doing, moved rather than removed, and worth being explicit about in a commit whose point is that codes stop coming from prose. What moving it buys: it sits beside `UNKNOWN_QUERY_OP_PREFIX`, the constant that defines the message, in the same crate and the same review diff, and the prefix is pinned from both sides — `query_op`'s `an_unknown_value_keeps_the_prefix_the_error_routing_matches` and `error_codes::an_unknown_query_op_is_routed_off_the_serde_message`. A change that breaks the mapping fails `cargo test` instead of silently degrading a caller's `code` to `UNKNOWN`. `other_deserialization_failures_stay_uncoded` pins the other side, since a prefix match that over-captured would be worse than none. Rebase note: this series was written against a tree where `UnknownQueryOp` was still a plain `Error` variant, and #143 landed on main in between. `integration-tests/tests/wasm-error-codes.test.ts` covers the wasm entry. It needs no credentials, unlike the round-trip suite (#149) — every case is config validation, which fails before any network I/O. Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
`inferErrorCode` is deleted. It matched the message against fourteen
prefixes and substrings to recover what Rust had just thrown away — the
same process serialising structure to prose and then parsing the prose
back.
It worked, and it was fragile in a way nothing tested. Three of those
patterns matched wording owned by cipherstash-config, not this repo:
if (message.includes('requires plaintext_type: json'))
return 'STE_VEC_REQUIRES_JSON_CAST_AS'
if (message.includes('unsupported config version'))
return 'UNSUPPORTED_CONFIG_VERSION'
An upstream reword would silently downgrade a caller's error to
`UNKNOWN` — the call still fails, just less usefully, and nothing here
would have failed to say so. Three of fourteen understates it, because
the table gave no way to tell which three: `' index configured'` reads
exactly like an upstream phrase and is this repo's own `MissingIndex`.
`docs/canonical-encryption-config-migration.md` had already flagged the
gap as a follow-up, proposing tests for the substrings. This closes it by
removing them instead.
`normalizeError` reads `err.code` and validates it against the declared
set. Validation is the point: Node puts a `code` on its own errors, so a
bare structural read would let an `ECONNRESET` through as a
`ProtectErrorCode`. `isProtectErrorCode` exports that check, for callers
who cannot rely on `instanceof ProtectError` — the wasm entry has no JS
wrapper to construct one.
`PROTECT_ERROR_CODES` is now the single declaration, with the union
derived from it, and `errorCodes.test.ts` reads the Rust attributes and
proves the two sets agree. That is the one remaining way for this to go
wrong, and it is silent: a code TypeScript does not declare still arrives
at runtime and still fails the predicate.
BREAKING CHANGE: a failed `decryptBulkFallible` item with no code omits
`code` rather than setting `'UNKNOWN'`. The declared type has always been
`code?: ProtectErrorCode`, but the field was in practice always present on
the Neon entry, because the wrapper stored whatever the inference returned.
Test for absence instead.
The api reference stopped restating the union — its copy was already
missing `SHORT_MATCH_NEEDLE`, which is the argument.
Claude-Session: https://claude.ai/code/session_01YJekcEBAsUg8qJoBcqyzBx
The previous commit stopped `src/errors.ts` inferring a code from the message, but left the layer that existed to carry it: every export ran through `wrapAsync`/`wrapSync`, which caught each failure and re-threw it as a `ProtectError`. Once Rust sets `code` on the error it builds, that layer adds nothing. It was not free. It made the two bindings throw different things — wasm has no JS wrapper, so its errors stayed plain. It re-based the stack trace onto the wrapper, demoting the real one to `cause`. And the check it existed to provide, `instanceof`, is false across duplicate copies of a package, which is the failure the issue's payoff section already called out. So the exports return what the binding threw. `newClient` on a bad config now produces a byte-identical error object on Neon and wasm, which is what this PR series has been converging on since #142. No replacement guard is shipped, and the `isProtectError` I first reached for is not there. Narrowing is not a neon limitation — TypeScript types every `catch` variable as `unknown` (TS18046), so a caller narrows once no matter what Rust throws, and neon has no class API, so Rust could not throw an `instanceof`-able class unless JS handed it one. Branching needs nothing from this package: if (err instanceof Error && 'code' in err && err.code === 'MISSING_INDEX') and `isProtectErrorCode`, already exported for validating a code value, narrows `err.code` to a typed `ProtectErrorCode` for callers that want to store it. A second predicate would have been API replacing API. Every export that returns a promise is now `async`, and that keyword is load-bearing rather than stylistic: neon extracts arguments SYNCHRONOUSLY. A bad client handle, an options object serde rejects (every unknown-key rejection from #144), or an out-of-range bigint threw from the call itself, and `wrapAsync` was quietly converting those into rejections. Verified against the built addon that all four still reject rather than throw, and that an out-of-range bigint is still a `RangeError` — which the README and the `JsPlaintext` JSDoc promise, and which previously survived only because the inference table happened not to match its message. BREAKING CHANGE: `ProtectError` is no longer exported and nothing throws it. Both entries throw an ordinary `Error` with a `code` property. Replace `err instanceof ProtectError && err.code === X` with `err instanceof Error && 'code' in err && err.code === X`, or use `isProtectErrorCode` where the code is wanted as a typed value. Claude-Session: https://claude.ai/code/session_01YJekcEBAsUg8qJoBcqyzBx
ci: lint the wasm32 target, and give the Rust checks one entry point
feat!: derive error codes in Rust instead of string-matching Display output (#146)
The commit before this one is a `git subtree add` of `cipherstash/protectjs-ffi` at its v0.31.0 tag, tree-identical to upstream and carrying its full history. This makes it a member of the monorepo. **Scripts are split so the repo stays Rust-free by default.** Root `pnpm test` runs `turbo test --filter './packages/*'`, which now reaches this package — so a cargo process on that path is a Rust toolchain on every contributor's machine. `test` is the JS chain and `build` is `tsc`; `cargo test` and `cargo fmt --check` live behind `test:cargo`, clippy behind `mise run lint:rust`, and `cargo build --release` behind `build:native`. `src/lintWiring.test.ts` enforces the split: no `test:*` script may be unreachable from both entry points, nothing cargo may be reachable from `test`, and every cargo check must be reachable from `test:cargo`. `test:typecheck:wasm` is deliberately NOT here. It needs `dist/wasm`, so it cannot hang off the default `test`, and the exemption list that would carve it out requires a root workflow to name it — the job that does arrives with the workspace link, in the last PR of this stack. Adding the script now would be exactly the laundering the exemption list exists to prevent: a carve-out whose "some other job runs it" reason is prose, and prose does not fail. **Six per-platform binary packages** under `platforms/*` are linked with `workspace:*` and globbed in `pnpm-workspace.yaml` — `packages/*` only reaches one level, so they need their own entry. `.changeset/config.json` gains the matching fixed group. **Publishing has not moved.** All seven packages are still published from `cipherstash/protectjs-ffi` until npm trusted publishing is repointed, so `scripts/lint-no-ffi-changeset.mjs` fails CI on a changeset naming any of them. The package can be changed freely; its changeset waits for the cutover. **`lib/` is the package `main` and is generated**, so a workspace consumer resolves an empty package until `build` has run. `turbo.json` carries a `@cipherstash/protect-ffi#build` override declaring `outputs: ["lib/**"]`; without it Turbo caches the repo-wide `dist/**` and a cache hit restores nothing while reporting success. **Three WASM declaration files are tracked** (`dist/wasm/*.d.ts`) so stack's declaration build resolves `@cipherstash/protect-ffi/wasm-inline` without Rust. Everything else under `dist/` stays ignored; the re-inclusion chain spans the root `.gitignore`, the package's own, and one wasm-pack generates. Consumers are untouched: `@cipherstash/stack` and the two adapters still resolve the published 0.31.0 from npm. Vendoring and switching to the workspace copy are separate steps, and this is the first.
The absorption deposited upstream's workflows under `packages/protect-ffi/.github/`, a directory GitHub never reads — it takes workflows from the repository root alone. So from the day the package landed, its Rust checks and its 19-file live integration suite ran NOWHERE, and a suite that never starts reads exactly like a suite that passes. **`tests-rust.yml`** runs `test:cargo` (cargo test + rustfmt) and `mise run lint:rust` (clippy, host and wasm32), path-filtered to the package. It runs `lint:rust` by name rather than its arms, because an arm reachable only by name is an arm nobody runs; `lintWiring.test.ts` asserts every `lint:rust:*` task is in its `depends` list. **`integration-protect-ffi.yml`** runs the integration suite against live ZeroKMS and a real Postgres. Two things there are deliberately not copies of upstream. It builds the binding with `.github/actions/build-ffi-binding` rather than `mise run build:debug`, because a RELEASE `index.node` at the package root satisfies `src/load.cts`'s `debug:` fallback and that action caches it on a content hash of the Rust inputs; and it invokes vitest directly, since the mise task would recompile in the debug profile over the artifact CI already paid for. It does not use `.github/actions/integration-db` — the EQL installs pipe SQL through `docker exec -i protect-ffi-postgres`, which only this suite's own compose file produces, and that action provides no EQL at all. Nothing else in the repo installs EQL v2, which half the suite needs. `src/integrationSuiteCi.test.ts` asserts a ROOT workflow still invokes the suite, and deliberately scans only the repo-root workflow directory. That is what stops it going quiet again. **Three guards land with the workflows they guard**, each written against a defect that was live rather than hypothetical: - `workflow-dispatch-job-conditions.test.mjs` evaluates every job-level `if:` against a synthetic context per event. Six workflows gated on `github.event_name == 'push' || <same-repo check>` — an ALLOWLIST of events, which fails shut on the one nobody enumerated. On a manual dispatch the event name is neither, and the payload carries no `pull_request` object, so GitHub coerces null to 0 against a string that is NaN and both operands are false: the run is created, the only job is skipped, and it reports success having executed nothing. All six now say "not a fork pull request" instead. The evaluator THROWS on an expression outside its grammar, so a condition it cannot reason about fails loudly rather than being waved through. - `workflow-node-gyp.test.mjs` asserts every job running `pnpm install` puts node-gyp on PATH first, flattening local composite actions so the four jobs that reach their install through `integration-setup` are checked where they actually install. node-pty is the repo's one `onlyBuiltDependencies` entry and ships no linux prebuild, so its `node-gyp rebuild` fallback is unconditional on a Linux runner. - `ffi-binding-action.test.mjs` pins the mise-action SHA and the cache-key inputs of `build-ffi-binding`. Dependabot gains the `cargo` ecosystem for the in-tree workspace — monthly, and ignoring the exact-pinned CipherStash crates that share a release train with the `@cipherstash/auth` catalog. The supply-chain e2e suite now derives ecosystem coverage from the filesystem, so a lockfile for a new language fails the suite until `dependabot.yml` covers it, and checks each entry's `directory` actually contains the manifest its ecosystem reads — load-bearing for cargo, whose workspace root is `packages/protect-ffi`, not the repo root.
🦋 Changeset detectedLatest commit: d348f84 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Vendoring the package puts it inside `turbo test --filter './packages/*'`, so root `pnpm test` now runs protect-ffi's own suite — and `src/nativeLoading.test.ts` asserts the platform binary loads. Nothing in the `run-tests` job produced `index.node`, so it failed with `Cannot find module '.../protect-ffi-linux-x64-gnu/index.node'` on Node 22 and 24. This is a consequence of vendoring alone, not of linking consumers to the workspace copy, which is why the step belongs in this PR rather than the next one. It reproduces only in CI: a local checkout that has ever built the binding keeps `index.node` on disk, gitignored, and the test passes on the stale artifact.
freshtonic
approved these changes
Aug 6, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Review — APPROVE
Reviewed all four first-parent commits. The subtree add (aa69b3ea) is byte-identical to upstream protectjs-ffi v0.31.0 and not reviewed beyond "right commit"; the reconcile + CI-wiring commits are the actual work.
Verified locally
pnpm run code:check(biome): 0 errors (warnings/infos pre-existing, CI gates on errors only)pnpm run test:scripts: 285/285 pass- supply-chain e2e: 21/21 pass
- protect-ffi JS unit: 83 pass; the one failure (
assertNativeBindingAvailable > succeeds when the binding is present) needs a compiledindex.nodefrom the Rust build — exactly as the PR's verification note documents. All emit-shape guards (lazy load, no__importStar, body reaches native) pass. - Rust checks + live ZeroKMS/Postgres integration not runnable here (no toolchain/creds), consistent with the PR.
Blocking
None.
Should-fix (non-blocking)
assertNativeBindingAvailablenegative path is untested (packages/protect-ffi/src/index.cts,nativeLoading.test.ts). The function exists to detect a missing binary, and its doc comment promises the error propagates asMODULE_NOT_FOUNDwith the samecode/message— but nothing asserts that. The test defers the negative case to "the CLI's missing-binary fixture," which doesn't exist yet, so the promised contract ships unverified. PointingcreateRequireat a fixture entry whoseload.cjsproxy has no platform match would cover it. Low urgency (new, not yet consumed), but it's the important half of the contract.
Nits
- The new public export + lazy-load behavior change land with no changeset — deliberately, since
scripts/lint-no-ffi-changeset.mjsblocks a protect-ffi changeset until trusted publishing is repointed. The script says the "phase-2 laziness changeset lands too" in the cutover PR; nothing enforces that it isn't forgotten there, so worth a checklist item on phase 4. skills/stash-supply-chain-securitydescribes the cargo/npm cooldown as "7 days minor/patch" and drops the 14-day semver-major window — consistent with majors being ignored entirely, which makessemver-major-days: 14effectively dead config for npm and cargo. Harmless.
Highlights
- Every CI guard is written against a real reproduced defect and asserts the property not a string proxy — the
workflow_dispatchevaluator models GitHub's coercion (null→0, string→NaN, case-insensitive compare) and throws on unknown grammar so "didn't understand it" ≠ "job runs". Guards defend their own vacuity (matched > 0,EXPECTED_*floors). - The lazy native load is a genuine correctness win (pure-JS consumers like
@cipherstash/migratecan import without a binary), verified against emitted JS since the difference is invisible on a machine that has a binary. turbo.json/pnpm-workspace.yaml/dependabot.ymlcomments thoroughly explain the why (Turbo empty-dist/cache trap,platforms/*nesting glob, cargodirectory: /packages/protect-ffinot root).- The filesystem-derived ecosystem-coverage e2e test turns "someone adds a new-language lockfile" into a CI failure instead of a silent monitoring gap.
- Vendored-source reconciliation is pure Biome reformatting — no behavior change.
AGENTS.md compliance
Skill updated in-PR with a matching stash patch changeset; protect-ffi changeset correctly deferred; only the root run-tests job (which now builds the binding) invokes protect-ffi's native suite — tests-bench is scoped to packages/bench. All good.
…ngAvailable
The function exists to detect a missing platform binary, and its doc comment
promises the loader's error propagates unwrapped — same `code`, same `message`.
Nothing asserted that. The suite deferred the negative case to a CLI
missing-binary fixture that does not exist (an unchecked phase-5 item), so the
promise was verified by prose alone.
Reproduced by swapping the loader rather than the environment: the real emitted
`lib/index.cjs` is re-required with a stand-in for `./load.cjs` whose platform
arm requires a specifier Node genuinely cannot resolve. Both halves are real —
the shipped function, and Node's own resolver error.
Two shapes that look right and are not, recorded in the test because both are
what you reach for first:
- A loader with no entry for the current platform fails EAGERLY inside
`@neon-rs/load`'s `proxy()`, at require time, with a plain `Error` and no
`code`. It never reaches the function under test. A missing binary is the
opposite: the platform key is present and the lazy `load()` closure raises
`MODULE_NOT_FOUND` on first property access.
- Requiring the real platform package from a directory with no `node_modules`
above it does not make it unresolvable. pnpm's bin shims export `NODE_PATH`
covering the flat store, and Node appends it for every bare specifier, so the
fixture silently loads the very binary it stands in for.
Asserts object identity, not `code`/`message`, because the failure mode is a
wrapper that looks correct — `new Error(msg, { cause })` preserves the message
and `Object.assign(new Error(msg), { code })` preserves the code too.
`scripts/lint-no-ffi-changeset.mjs` blocks a changeset naming any of the seven
FFI packages until npm trusted publishing is repointed. It told contributors
their changeset waits for the cutover PR without giving them anywhere to put
it, so the prose for the laziness change and `assertNativeBindingAvailable()`
would have had to be reconstructed from the git log months later — which is how
a user-visible behaviour change ships with an empty changelog.
Park it as `.changeset/<name>.md.deferred`. The cutover PR becomes a `git mv`
rather than an act of memory.
The suffix order is the entire safety property, and it was verified before
being relied on. `@changesets/read` selects with `!f.startsWith('.') &&
f.endsWith('.md') && !/^README\.md$/i`; `getOldChangesets` descends only into
directories; `removeEmptyFolders` swallows the `ENOTDIR` a file raises; and
`applyReleasePlan` deletes strictly `${id}.md` for ids in the plan. Empirically:
with `protect-ffi-lazy-load.md.deferred` in place `changeset status` queues no
FFI package, and renaming it to `.deferred.md` queues all seven.
The guard's failure message now prints the `git mv` recipe at the moment someone
needs it, and its self-test pins that a parked file exists, names a guarded
package, and would not be read as a changeset. Those assertions guard the
window, not the cutover — the cutover PR deletes them along with the guard.
The `npm` and `cargo` entries set `semver-major-days: 14`, and nothing could ever reach it. Both entries ignore `version-update:semver-major` for `*`, so Dependabot never proposes a major version update for the key to delay, and it cannot apply on the security path instead — "the cooldown option is only available for version updates, not security updates" (Dependabot options reference). The `github-actions` entry already omitted it, so the three were inconsistent as well as inert. Removed rather than documented, following the precedent the cargo entry already records for the `day:` key it leaves out of a monthly schedule: left out so it does not read as configuration that does nothing. Dead config reads as policy — this one said majors arrive after 14 days when in fact they never arrive. The e2e assertion pins both ends of the relationship, not just the conditional "if it ignores majors then no window", which would pass vacuously on exactly the drift that makes a window live again: dropping the ignore. Asserting the ignore too also gives the skill's "majors are reviewed and applied by hand" claim its first test.
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Synthesis review — protect-ffi monorepo absorption
Verdict: ship with follow-ups. No blockers. The CI/build surface is careful work; the substantive findings are one image-pin drift (corroborated by both models) and a set of narrow test-coverage gaps on newly-added branches. Everything below was verified against the diff; unsubstantiated items were dropped.
The only cross-model finding — postgres:latest — is real: the repo pins its DB image everywhere else (ghcr.io/cipherstash/postgres-eql:17-2.3.1 in .github/workflows/tests.yml:62 and :402), and this credentialed suite installs version-specific EQL SQL, so a silent Postgres major bump can break it with no diff behind it.
Review stats
| Source | Raw | Survived |
|---|---|---|
| claude (opus-4-8) [infracode] | 1 | 1 |
| claude (opus-4-8) [test-gap] | 3 | 3 (2 inline, 1 overflow) |
| codex (gpt-5.5) [infracode] | 3 | 3 |
| codex (gpt-5.5) [test-gap] | 3 | 3 |
- Cross-model overlap: 2 kept findings corroborated by 2+ models —
postgres:latestunpinned image, andrestart: alwayson the ephemeral CI DB. All others are single-source but verified against the branch they touch. - Dropped: none as hallucinations; every raw finding grounded in the diff.
Additional findings not posted inline
integration-tests/tests/error-stack.test.ts:16— lopsided negative (single-source: claude). Covers a stackless native rejection getting the JS call site grafted, but not its complement: a JS-side failure that already has frames (e.g. an out-of-range bigint fromencodeBigIntPlaintext, whichbigintWire.test.ts:35confirms throws onBIGINT_MAX + 1n) must makerestoreCallSiteearly-return and leave the original stack intact. That early-return guard is unasserted.scripts/inline-wasm.mjs— no test (single-source: claude). Its one guard (if (!exportMatch) throw) is untested; lower priority than the changelog extractor because it's a side-effecting build step needing a fixture dir rather than a pure call.src/index.ctsrestoreCallSite— two unexercised arms (single-source: claude). The synthesized${name}: ${message}header on empty-string stack, and the try/catch around a read-onlystack, are only reachable through the credentialed integration suite.- Dead-but-live-looking workflows under
packages/protect-ffi/.github/**(single-source: claude). GitHub reads only root workflows, sobuild.yml/release.yml/test.yml/.envthere never run (intentional per the cutover story, guarded bysrc/integrationSuiteCi.test.ts). Buttest.ymlhardcodes staleap-southeast-2hosts and.envpinsNODE_VERSION=20.x. No leak, no regression — a maintainer just can't tell dead from live at a glance. Add a# NOT EXECUTEDheader or delete at cutover. push:path-filter asymmetry (single-source: claude).tests-rust.ymlruns cargo on every push tomain;integration-protect-ffi.yml's push trigger is path-filtered. Both defensible; noted only for consistency.
| @@ -0,0 +1,16 @@ | |||
| services: | |||
| postgres: | |||
| image: postgres:latest | |||
There was a problem hiding this comment.
[2 models: claude, codex] postgres:latest is unpinned — the one unpinned DB image in the repo. This credentialed suite installs version-specific EQL SQL (the eql-2.2.1 v2 bundle plus the eql_v3_* domains), so a Postgres major/minor bump can silently break the install/suite with no diff behind it — exactly the drift the repo's pinning norm exists to prevent (.github/workflows/tests.yml:62/:402 pin ghcr.io/cipherstash/postgres-eql:17-2.3.1). Pin to the version this suite is validated against, e.g. image: postgres:17 (or a digest, or the postgres-eql image the sibling harnesses use). Codex rated this request-changes; claude rated it follow-up — either way it's a wrong default for CI infra, not shipping-broken today but an unforced future flake.
| - POSTGRES_PASSWORD=${PGPASSWORD:-password} | ||
| ports: | ||
| - 5436:5432 | ||
| restart: always |
There was a problem hiding this comment.
[2 models: claude, codex] restart: always on an ephemeral CI/test DB. Combined with the fixed container_name: protect-ffi-postgres and fixed host port 5436, a hard-cancelled run can leave a container blocking the next local/warm-runner run, and docker compose up --wait can silently restart a Postgres that crashed mid-suite — turning a hard failure into a confusing flake. The sibling integration-db action avoids this with isolated project names + teardown. Drop the policy (compose default "no") and let --wait own lifecycle. Follow-up severity — the DB is throwaway.
| ["eql:download"] | ||
| description = "Download CipherStash encrypt SQL" | ||
| dir = "{{ config_root }}/integration-tests" | ||
| run = "curl -sLo sql/cipherstash-encrypt.sql https://github.com/cipherstash/encrypt-query-language/releases/download/eql-2.2.1/cipherstash-encrypt.sql" |
There was a problem hiding this comment.
[single-source: codex] eql:download fetches SQL with no failure handling or integrity check, then eql:install pipes it into Postgres. curl -sLo is silent-on-error, so a replaced/bad release asset or a CDN hiccup becomes trusted database setup (or an empty file that fails later, further from the cause). Fail loudly and verify a pinned checksum before install, e.g. curl --fail --location --show-error --silent -o sql/cipherstash-encrypt.sql <url> && echo '<sha256> sql/cipherstash-encrypt.sql' | sha256sum -c -. Follow-up — ephemeral DB, but it executes third-party-hosted SQL in credentialed CI.
| // *inside* a section body (never at EOF) is left untouched. This is stricter | ||
| // than treating any column-0 `[x]:` line as the boundary, which would truncate | ||
| // notes that use reference-style Markdown links. | ||
| const withoutLinks = text.replace( |
There was a problem hiding this comment.
[single-source: claude] New release-notes extractor with real branching logic and no test, unlike every sibling .mjs helper (scripts/__tests__/ has dozens). Two non-obvious claims rot silently: that the trailing compare-link block is stripped while an in-body reference link is preserved (this regex, L33-36), and that semver metacharacters (+, .) are escaped so the heading matches literally (L40) — drop that escape and + reads as a quantifier. The exit-code arms (missing arg → 2, unreadable file → 1, missing section → 1) are also unverified. A small execFileSync-driven test over a temp CHANGELOG covers all of it.
| envClientId !== undefined && envClientKey !== undefined | ||
|
|
||
| const creds: CredentialOpts = { | ||
| clientId: opts?.clientId ?? (hasEnvClientKey ? envClientId : undefined), |
There was a problem hiding this comment.
[single-source: claude] The atomic-keypair rule guards the env side but not a half-specified opts side. Precedence is per-field, so withEnvCredentials({ clientId: 'opt-id' }, envWithBothKeys) yields clientId: 'opt-id' + clientKey: 'env-key' — a mismatched pair. withEnvCredentials.test.ts's "partial opts filled from env" case only fills the independent accessKey, never half of the pair, so nothing pins this behaviour either way. Add a test asserting the intended outcome (keep the env key, or take neither half) so a future refactor is a visible test edit, not a silent flip.
| }); | ||
|
|
||
| match (requested, has_ste_vec) { | ||
| (Some(version), true) if version == EqlVersion::V2 as u8 => Err(Error::SteVecRequiresV3), |
There was a problem hiding this comment.
[single-source: codex] SteVecRequiresV3 branch is uncovered. (Some(V2), true) => Err(Error::SteVecRequiresV3) rejects forcing eqlVersion: 2 on a ste_vec config; the integration tests cover scalar eqlVersion handling and ste_vec-without-v2 (encrypt-config.test.ts), but not this specific rejection. Add a live case constructing a ste_vec config with eqlVersion: 2 and asserting it rejects at client construction — so a removal/reorder of this arm surfaces as a test failure rather than silently emitting a v2 payload.
| hint: JsonPathHint::TryPrefix(path.to_string()), | ||
| }); | ||
| } | ||
| if !path.starts_with('$') { |
There was a problem hiding this comment.
[single-source: codex] Malformed ste_vec_selector path branches untested. query.test.ts covers valid selectors ($.name, $.email, $.profile.address) but neither the empty-path (JsonPathReason::Empty) nor the missing-$ (MissingDollar) rejection here. Without a negative test, '' and 'name' would silently encrypt as selector strings if this validation regressed. Add a case asserting both reject with INVALID_JSON_PATH.
| } | ||
| }; | ||
|
|
||
| let valid = value |
There was a problem hiding this comment.
[single-source: codex] ste_vec_value_selector shape validator only exercised on the happy path. The validator requires a 2-key object with a scalar value (object.len() == 2 and value not object/array). json-containment.test.ts:691 covers the valid shape; malformed inputs (missing value, or a nested object/array value) that should reject with INVALID_QUERY_INPUT are untested. Add a negative case so a loosened/bypassed validator doesn't silently produce a value-selector needle.
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
7 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.
Stack 3 of 4 — splitting #858. Base: #861.
packages/protect-ffiand wire its own CIReviewing this one
The first commit is a
git subtree addofcipherstash/protectjs-ffiat its v0.31.0 tag — the vendored tree is byte-identical to upstream (verified by tree comparison) and the full upstream history is preserved and reachable. There is nothing to review in it beyond "is this the right commit". The two commits after it are the actual work.Consumers are untouched.
@cipherstash/stackand the adapters still resolve the published 0.31.0 from npm. Vendoring and switching are separate steps, and this is only the first.Commit 2 — reconcile with the monorepo
Scripts are split so the repo stays Rust-free by default. Root
pnpm testreaches this package, so a cargo process on that path would be a Rust toolchain on every contributor's machine.testis the JS chain,buildistsc; cargo lives behindtest:cargo,mise run lint:rustandbuild:native.src/lintWiring.test.tsenforces the split from the manifest side.Six per-platform binary packages linked with
workspace:*and globbed explicitly (packages/*only reaches one level).turbo.jsongains a build override declaringoutputs: ["lib/**"]— without it Turbo caches the repo-widedist/**and a cache hit restores nothing while reporting success. Three WASM.d.tsare tracked so stack's declaration build resolves without Rust.Publishing has not moved —
scripts/lint-no-ffi-changeset.mjsfails CI on a changeset naming any of the seven packages until trusted publishing is repointed.Commit 3 — run its checks from root workflows
The absorption deposited upstream's workflows under
packages/protect-ffi/.github/, a directory GitHub never reads. So from the day the package landed, its Rust checks and its 19-file live integration suite ran nowhere — and a suite that never starts reads exactly like a suite that passes.tests-rust.ymlandintegration-protect-ffi.ymlfix that;src/integrationSuiteCi.test.tsstops it going quiet again.Three guards land with the workflows they guard, each written against a live defect:
workflow-dispatch-job-conditions— six workflows gated on an allowlist of event names, so a manual dispatch created a run, skipped its only job, and reported success having executed nothing. All six now say "not a fork pull request". The evaluator throws on expressions outside its grammar rather than waving them through.workflow-node-gyp— every job runningpnpm installmust have node-gyp on PATH first, flattening composites so the four jobs that install viaintegration-setupare checked where they actually install.ffi-binding-action— pins the mise-action SHA and the cache-key inputs.Dependabot gains the
cargoecosystem; the supply-chain e2e suite now derives ecosystem coverage from the filesystem, so a lockfile for a new language fails the suite untildependabot.ymlcovers it.Verification
protect-ffi's JS chain passing; scripts suite 285 passing; supply-chain e2e 21 passing; biome 0 errors; lockfile in sync.
assertNativeBindingAvailableneeds a compiledindex.node(Rust build) — verified passing locally with the binary present, which CI produces.