Skip to content

feat(protect-ffi): vendor the package and run its own checks - #862

Open
tobyhede wants to merge 563 commits into
feat/protect-ffi-2-supply-chain-gatefrom
feat/protect-ffi-3-vendor
Open

feat(protect-ffi): vendor the package and run its own checks#862
tobyhede wants to merge 563 commits into
feat/protect-ffi-2-supply-chain-gatefrom
feat/protect-ffi-3-vendor

Conversation

@tobyhede

@tobyhede tobyhede commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Stack 3 of 4 — splitting #858. Base: #861.

PR What it does
1 #860 Consume the published protect-ffi 0.31.0
2 #861 Close three fail-open holes in the no-caching gate
3 this one Vendor packages/protect-ffi and wire its own CI
4 Link consumers to the workspace copy

Reviewing this one

The first commit is a git subtree add of cipherstash/protectjs-ffi at 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/stack and 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 test reaches this package, so a cargo process on that path would be a Rust toolchain on every contributor's machine. test is the JS chain, build is tsc; cargo lives behind test:cargo, mise run lint:rust and build:native. src/lintWiring.test.ts enforces the split from the manifest side.

Six per-platform binary packages linked with workspace:* and globbed explicitly (packages/* only reaches one level). turbo.json gains a build override declaring outputs: ["lib/**"] — without it Turbo caches the repo-wide dist/** and a cache hit restores nothing while reporting success. Three WASM .d.ts are tracked so stack's declaration build resolves without Rust.

Publishing has not movedscripts/lint-no-ffi-changeset.mjs fails 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.yml and integration-protect-ffi.yml fix that; src/integrationSuiteCi.test.ts stops 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 running pnpm install must have node-gyp on PATH first, flattening composites so the four jobs that install via integration-setup are checked where they actually install.
  • ffi-binding-action — pins the mise-action SHA and the cache-key inputs.

Dependabot gains the cargo ecosystem; 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.

Verification

protect-ffi's JS chain passing; scripts suite 285 passing; supply-chain e2e 21 passing; biome 0 errors; lockfile in sync. assertNativeBindingAvailable needs a compiled index.node (Rust build) — verified passing locally with the binary present, which CI produces.

yujiyokoo and others added 30 commits April 16, 2026 14:55
On AWS, we had issues where GLIBC_2.36 was not supported. As the
currently supported Node version is 20+, and it requires 2.28, we set
our GLIBC version to 2.28 here too
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).
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.
coderdan and others added 24 commits July 27, 2026 12:56
`queryOp` travelled as a bare `String` from the options struct all the
way to `prepare_query_plaintext`, where a `match` turned it into
cipherstash-client's `QueryOp` and an unrecognised value became
`Error::UnknownQueryOp`. So the field was validated late, after the
column and index had already been resolved, and every function between
the boundary and that `match` took a `&str` that might be anything.

`QueryOpName` is now the type on the wire, in `query_op.rs` with the
tests that define it. A value that is not one of the four spellings
cannot be constructed, so `parse_query_op`, `default_query_op` and the
`UnknownQueryOp` variant are all gone — `to_query_op` is infallible
because the value was already checked.

The mapping to `QueryOp` stays explicit rather than deriving or
re-exporting the library's enum: that type is cipherstash-client's
internal vocabulary and can grow variants this binding has no wire
spelling for.

Two things that constrained the shape:

- `inferErrorCode` in `src/errors.ts` matches `Unknown query operation:`
  to produce the public `UNKNOWN_QUERY_OP` code. A derived `Deserialize`
  would emit serde's own "unknown variant" wording and that code would
  quietly become unreachable, so `Deserialize` is hand-written around a
  shared `UNKNOWN_QUERY_OP_PREFIX`, and the coupling is pinned from both
  ends. Verified the message survives the trip: neon's `Json` extractor
  Displays the serde error verbatim into the thrown `JsError`, and the
  wasm entry does the same. The message now also lists what IS accepted,
  which it did not before.
- `QueryOpKind` is renamed `SteVecQueryOpKind`, variants losing the
  `SteVec` prefix. All 17 uses are on `ste_vec` paths — the other index
  types have no shape for the input to be wrong in — so the prefix was
  redundant, which is what `clippy::enum_variant_names` was pointing at.
  Its `Display` now goes through `QueryOpName::as_str`, so an error
  cannot name an operation with a spelling the caller could not pass
  back. `Default` keeps its parenthetical, being inferred rather than
  written.

`lib.rs` is 70 lines shorter. `cargo clippy --no-deps --tests
--all-features --all-targets -- -D warnings` clean, 279 Rust tests
(8 new), 50 vitest, wasm32 checks, wasm type tests green.

Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
`prune_nulls` treats a null-valued key as an absent field, which is what
makes `{cast_as: cfg.castAs}` with an undefined `castAs` work on wasm.
It also, unavoidably, does the same to NaN and ±Infinity:
`serde_json::Value` cannot hold a non-finite float and
`JSON.stringify(Infinity)` is literally `null`, so by the time a config
reaches this function the two are the same value on both bindings.

So `{match: {m: Infinity}}` used to fail with `invalid type: null,
expected usize` and now silently takes `m`'s default. That is worse for
that input, and it is not fixable here — there is nothing left to
distinguish. Accepted because the frequencies are not comparable: an
undefined property is ordinary JavaScript that hard-errored on wasm, a
non-finite bloom filter parameter is a typo nobody has written.

Documented on `prune_nulls` and pinned by
`a_non_finite_number_is_indistinguishable_from_undefined`, so it reads
as a known trade rather than something nobody noticed. Catching it would
have to happen on the JS side, before the `Value` hop.

Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
Every options struct now carries `deny_unknown_fields`, so a misspelling,
a stale key, or a value in the wrong place fails by name rather than being
discarded on the way in.

The issue proposed the serde attribute and flagged two obstacles. Both
turned out differently:

- `deny_unknown_fields` alongside `#[serde(flatten)]` works. serde_derive
  generates a leftover check for exactly that combination, so `ClientOpts`
  and `EnsureKeysetOpts` needed nothing beyond the attribute — no custom
  Deserialize, no unflattening `CredentialOpts`.
- The attribute alone is a no-op on wasm, which the issue did not
  anticipate. serde-wasm-bindgen's `deserialize_struct` looks up each
  expected field with `Reflect::get` and never enumerates the object, so an
  undeclared key is invisible to serde and there is nothing to reject. The
  boundary that silently dropped the credentials was the one the fix would
  have missed.

`DenyUnknown` closes that: an empty flattened marker puts every options
struct on serde's `deserialize_map` path, which goes through
`Object::entries` and does enumerate. The two structs that already flatten
are on that path and don't carry it.

Also:

- The wasm `newClient` strips `authStrategy` / `strategy` from a shallow
  copy before deserializing. They're read with `Reflect` beforehand — a JS
  function can't survive serde — and a struct that denies unknown fields
  would otherwise reject them. The copy is so a config reused across calls
  keeps its strategy.
- The Neon `newClient` rebuilt the native options object field by field,
  dropping unrecognised top-level keys before the Rust could see them. It
  now forwards the rest verbatim.

Breaking: input that was previously accepted and ignored is now rejected.
That is the point — `lockContext` at the top level of a bulk call used to
encrypt every value unbound while the caller believed it was
identity-bound, with nothing in the output to tell the two apart.
…m a doc

The `lockContext` narrative belongs in the PR, not on the struct field.
Removed; the field documents what it does.

The comment on the strip made two claims. One was wrong and the other was
missing:

- "Non-object input passes through untouched" described a path nothing takes.
  A primitive never reaches the guard — the `Reflect::get` above it has
  already failed with "called on non-object". Arrays and functions DO reach
  it, get flattened by `Object::assign`, and are rejected by serde for their
  missing fields. Checked against the built wasm rather than reasoned about.
- Nothing said why the deletes can't fail. They can't because `Object::assign`
  writes through [[Set]] onto a fresh object, so every copied property is a
  plain configurable data property regardless of the descriptor it had on the
  caller's object. That is the sort of guarantee a later refactor breaks
  silently, so it is written down now.

Both descriptor cases that could plausibly defeat the strip now have tests: a
frozen options object, and a non-enumerable strategy (which `Object::assign`
never copies — it is read off the original beforehand, so it is used rather
than lost).
`js_sys::Object::assign` is declared without `catch` — `pub fn assign<T>(target,
source) -> Object<T>`, no `Result`. Every clone in `wasm.rs` copies an object
the caller built, and a copy reads each own enumerable property, so any of them
can meet a getter that throws. That throw would travel straight out of wasm,
skipping the destructors of the zeroizing values these paths carry. Every other
JS call in the module is `Reflect::*` with `map_err`; js-sys points at
`try_assign` for exactly this.

All four clone sites now go through one `shallow_clone` helper, which names what
it was copying — the failure is the caller's object misbehaving and they have to
be able to find it. `new_client`'s strip was the site under review, but
`encode_plaintext` and `encode_plaintext_list` read caller objects on every
encrypt path and had the same hole.

Also records the other direction of the [[Set]] semantics the strip already
relies on: a setter for a copied key on `Object.prototype` swallows the value
and the key never arrives. A silent drop inside the function that exists to stop
silent drops, but reaching it means a poisoned `Object.prototype`, which breaks
far more than this.

Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
Seven of thirteen structs carrying `deny_unknown_fields` were asserted; six
were not — `PlaintextPayload`, `QueryPayload`, `EncryptQueryOptions`,
`EncryptQueryBulkOptions`, `DecryptOptions`, `BulkDecryptPayload`. All six
reject correctly, so this is what the tests were missing, not the code.

The bulk-payload case matters most: a misspelled `lockContext` on a payload item
is the per-item form of the top-level one this change exists for, and the
likelier of the two. Both spellings encrypt UNBOUND with nothing in the output
to tell them apart. Its mirror image is covered too — `unverifiedContext` lives
on the container and `lockContext` on the item, and each is now rejected in the
other's place rather than dropped, which is the trap for anyone copying the
scalar call shape.

The `queryOpp` assertion also pins what the marker costs: the message is
`unknown field \`queryOpp\`` and nothing else, because serde's flatten path
buffers the map and reports at its closing brace, dropping the `expected one of
...` list. Left unasserted that would go quietly.

On the wasm side, none of the existing cases actually depended on the marker:
`ClientOpts` reaches the map path through its own flattened credentials, so
only the `clientId` test needed `DenyUnknown` at all — the other ten
marker-carrying structs had no wasm coverage and dropping the marker from all of
them would have failed one test. A misspelled `eqlVersion` on `newClient` covers
it without credentials; the encrypt / encryptBulk / decrypt cases cover the
structs that actually carry a lock context.

Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
Forwarding what the JS layer doesn't handle is what lets Rust reject an unknown
key by name — but it also means an unknown key now reaches neon's `Json`
extractor, which is `JSON.stringify`. A circular value or a `bigint` throws
there, before serde runs:

    newClient({...appConfig, logger})
      -> TypeError: Converting circular structure to JSON

`normalizeError` / `inferErrorCode` match neither, so the caller gets a raw
`TypeError` — not a `ProtectError`, and not the name of the key they got wrong.
On a change whose whole point is naming the offender, that is the wrong failure.

Only the forwarded keys are checked, so this covers exactly what forwarding
newly exposed: `clientOpts` and the auth strategy never reach it, and a circular
`encryptConfig` fails the way it always has. The cost is one `JSON.stringify` of
the config, once, at client construction.

A function- or symbol-valued key is deliberately not caught — `JSON.stringify`
drops those without throwing, so they stay a silent drop on this binding while
wasm reports them. That asymmetry is in the CHANGELOG rather than fixed here.

Interim: #150 replaces this error layer with codes derived in Rust.

Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
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)
…068d2d0c72a4'

git-subtree-dir: packages/protect-ffi
git-subtree-mainline: 05f84bc
git-subtree-split: ce820bb
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.
@tobyhede
tobyhede requested a review from a team as a code owner August 6, 2026 00:58
@changeset-bot

changeset-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9ad87f2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
stash Patch
@cipherstash/basic-example Patch
@cipherstash/e2e Patch
@cipherstash/stack Patch
@cipherstash/stack-drizzle Patch
@cipherstash/stack-supabase Patch
@cipherstash/stack-prisma Patch
@cipherstash/wizard Patch
@cipherstash/bench Patch
@cipherstash/test-kit Patch
@cipherstash/prisma-example Patch

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

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3c93952f-06bf-4df4-ab8e-b051e5e248e2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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 freshtonic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 compiled index.node from 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)

  1. assertNativeBindingAvailable negative 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 as MODULE_NOT_FOUND with the same code/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. Pointing createRequire at a fixture entry whose load.cjs proxy has no platform match would cover it. Low urgency (new, not yet consumed), but it's the important half of the contract.

Nits

  1. The new public export + lazy-load behavior change land with no changeset — deliberately, since scripts/lint-no-ffi-changeset.mjs blocks 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.
  2. skills/stash-supply-chain-security describes the cargo/npm cooldown as "7 days minor/patch" and drops the 14-day semver-major window — consistent with majors being ignored entirely, which makes semver-major-days: 14 effectively 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_dispatch evaluator 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/migrate can 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.yml comments thoroughly explain the why (Turbo empty-dist/ cache trap, platforms/* nesting glob, cargo directory: /packages/protect-ffi not 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants