Repository navigation
feat(eql-bindings): put the by-name dispatch behind a targets feature, and keep dynamic out of stack-encrypt - #1260
Conversation
|
| Name | Type |
|---|---|
| @cipherstash/eql | Minor |
| stash | Patch |
| @cipherstash/stack-prisma | Patch |
| @cipherstash/basic-example | Patch |
| @cipherstash/e2e | Patch |
| @cipherstash/prisma-example | Patch |
| @cipherstash/stack-drizzle | Patch |
| @cipherstash/stack-supabase | Patch |
| @cipherstash/stack | Patch |
| @cipherstash/wizard | Patch |
| @cipherstash/bench | Patch |
| @cipherstash/test-kit | Patch |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a 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 configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Automated high-effort review by a Claude Code sub-agent. I reviewed only this PR's diff on top of #1259.
What I ran (fresh worktree at 27db82f1f, separate CARGO_TARGET_DIR):
cargo check -p eql-bindings --all-targetswith--features stack-encrypt,--features targetsand--no-default-features: all pass.cargo clippy -p eql-bindings --all-targets -D warningswith--features stack-encryptand with no features: clean.RUSTDOCFLAGS="-D warnings" cargo doc -p eql-bindings --no-depswith--features stack-encryptand with--features targets: clean.- Stack Encrypt's features from cargo's JSON output:
[]for bothcargo buildandcargo test --no-rununder--features stack-encrypt. So thestack-encryptline intest:encryptionreally builds withoutdynamic, and no dev-dependency turns it back on. mise run check:encryption:wasiandmise run test:encryption: pass (108 unit tests understack-encrypt, 115 undertargets;targets16,v4_types12 + 3 ignored,heap_hygiene2).- Go guest:
cargo check --locked --features eqlpasses, both natively (--all-targets) and forwasm32-wasip1. cargo publish -p eql-bindings --dry-run --all-features --allow-dirtyfails because stack-encrypt 0.3.0 is not on crates.io yet (only 0.1.0 and 0.2.0). That is expected and is not a finding.
Consumers and gating: no problems.
- Nothing outside
encryption::targetsandv4::targetsuseszeroize,thiserrororstack_encrypt::dynamic. So thestack-encryptset builds without them, and I found notargets-gated item that a typedencrypt_asuser needs.Identifier::from_labelmoves behindtargets, but it returnsTargetError, and the CHANGELOG already lists it under the dispatch entry. - Every user of the dispatch now enables
targets: the Go guest (languages/golang/encrypt/guest/src/targets.rs) andeql-encryption-tests. protect-ffi andtests/sqlxdepend on eql-bindings with no features and don't use the dispatch. - release-plz (
publish_all_features) and thetest-eql.ymldry run both use all features, so both pick uptargets. - The CHANGELOG entries are all under
[Unreleased], and the three changesets are consistent with the code.
Findings (all low; none blocks):
-
CI now checks the
stack-encrypt-only feature set less than before. Before this PR,stack-encryptwas the only feature, so every check covered it. Now three checks cover only the fulltargetsset:- clippy in
test:encryption(mise.toml:318,--all-features); - the wasm32 check (
mise.toml:354,--features targets); - the registry builds: the
--all-featuresdry run intest-eql.ymlandpublish_all_featuresin release-plz.
The typed-only set is built only by the in-tree
cargo test --features stack-encrypt, without-D warningsand only on the host. So nothing would catch these:- an unused import or dead code that shows up only without
targets; - a wasm32 break in the typed path;
- a crates.io stack-encrypt that puts something the typed path uses behind
dynamic.
Today all of these are clean (I ran clippy and the docs build on the
stack-encryptset by hand). Inline comments suggest small additions. - clippy in
-
The docs don't say the dispatch needs
targets. docs.rs builds withtargets, and the crate has nodoc(cfg). So a reader on docs.rs seesencryption::targetswith no hint that it needs a feature, and thesrc/encryption/targets.rsmodule docs never mention it. Also,encryption.rs:48drops the[targets]intra-doc link even when the module exists. See the inline comment.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Automated review attestation — Claude Code /code-review medium + /cross-check-review-findings, head 27db82f.
Result: no findings. No critical or blocking issues.
Checked against the PR diff and a local checkout:
cargo check --locked -p eql-bindings --features stack-encryptand--features targetsboth build.cargo test --no-run --features stack-encryptcompiles every test target withoutzeroize/thiserror, so moving them behindtargetsleaves no dangling use.RUSTDOCFLAGS="-D warnings" cargo doc --features stack-encryptis clean.- Every user of
encryption::targets/v4::targetsin the tree (Go guesteqlbuild,tests/encryption) now enablestargets. protect-ffidepends oneql-bindingswith no features, so it is unaffected.- The
//(not///) on thetargetsmodule declarations is correct for the reason given in the PR. - Changesets and CHANGELOG match the new feature split.
Not run here: Go tests, wasm32 check, and the mise tasks. CI covers those.
Generated by Claude Code
…, and keep dynamic out of stack-encrypt eql-bindings' stack-encrypt feature turned on Stack Encrypt's dynamic layer for every user, although only the by-name dispatch a language binding needs uses it: encryption::targets and the generated v4::targets take a ValueFrame and name an EQL type at run time. A Rust program that encrypts an IntegerEq through encrypt_as compiled the whole dynamic layer for nothing. stack-encrypt now gives the typed EQL v4 types alone, with Stack Encrypt's default features off and no dynamic. A new targets feature turns on stack-encrypt, Stack Encrypt's dynamic feature, and zeroize and thiserror, which only the dispatch uses, and compiles the two targets modules. The Go guest's eql build and the encryption tests enable targets. test:encryption tests eql-bindings under both feature sets, and the wasm32 check builds targets, as the guest does. docs.rs documents targets. The comment on the encryption::targets declaration is a plain comment: a doc comment there would resolve the module's own intra-doc links from the parent scope. Refs #1252.
Before the targets split, stack-encrypt was the only feature, so clippy with -D warnings and the wasm32-wasip1 check covered the typed path. After it, both ran only with targets on, and the typed set was built only by a host cargo test without -D warnings. A lint that fires only without targets, or a wasm32 break in the typed path, would have passed CI. test:encryption now also runs clippy on eql-bindings with --features stack-encrypt alone, and check:encryption:wasi also checks that set for wasm32-wasip1. Both are clean. Refs #1252
The encryption module docs lost their link to `targets` when the module went behind the feature, because a link to a missing module fails the doc build. Link it with cfg_attr when the feature is on, as lib.rs does, and keep the plain text otherwise. docs.rs builds with targets and the crate has no docsrs cfg, so no feature badge shows. Say in the targets module docs that it needs the feature. Refs #1252
27db82f to
e43dfaf
Compare
|
Reply to the review body. Both findings have inline threads; this covers the one part that only the body raised.
Checks on e43dfaf, from This branch is rebased onto #1259's new head 75336d3. I updated the |
2 participants
Summary
eql-bindings'
stack-encryptfeature turned on Stack Encrypt'sdynamiclayer for everyone who used it. Only the by-name dispatch a language binding needs usesdynamic; it names an EQL type at run time and takes aValueFrame. So a Rust program that encrypts anIntegerEqthrough the typedencrypt_ascompiled the whole dynamic layer for nothing. Nowstack-encryptgives the typed EQL v4 types alone, and a newtargetsfeature adds the dispatch. This is step 2 of #1252.Changes
packages/eql/crates/eql-bindings/Cargo.toml:stack-encryptbuilds Stack Encrypt with no default features and nodynamic.targetsturns onstack-encrypt,stack-encrypt/dynamic, andzeroizeandthiserror, which only the dispatch uses.targets.src/encryption.rs,src/v4/mod.rs:encryption::targetsand the generatedv4::targetscompile undertargetsonly. Two doc links that pointed at them now resolve under either feature set.eqlbuild (languages/golang/encrypt/guest/Cargo.toml) and the encryption test crate enabletargets.packages/eql/mise.toml:test:encryptiontests eql-bindings under both feature sets and runs clippy with-D warningson each.check:encryption:wasibuilds bothstack-encryptalone andtargets(as the guest does) for wasm32.targetsfeature" entry, and the dispatch entries now nametargets;Verification
[]under--features stack-encryptand["dynamic"]under--features targets.mise run types:check,test:encryptionandtest:cratespass. Clippy is clean, andcargo doc -p eql-bindingswith-D warningsis clean under both features.eqlbuilds (deterministic and not) build, and the guest's owncargo test --locked --features eqlpasses.go test ./encrypt/...passes, and so doescargo check -p eql-bindings --features targets --target wasm32-wasip1.cargo metadata --lockedpasses for the EQL, Go guest and protect-ffi workspaces, and no lockfile changed.Related
Refs #1252 (proposal step 2). Stacked on #1259.
Review notes
encryption::targetsorv4::targetsmust now enabletargets. Every entry that describes it is unreleased, so this changes no released version's surface. The changesets and the CHANGELOG are edited in place.targetsdeclaration has a plain//comment, not///: a doc comment on a module declaration merges with the module's own docs, and rustdoc then resolves the module's intra-doc links from the parent scope. The same thing happened on feat(eql-bindings): produce IntegerEq, BigintEq, RealEq and DoubleEq, and name each type's real blocker #1241.Plaintext. It goes with stack-encrypt: dynamic control messages (plan, options, query results) are parsed from an untyped FfiValue tree — give each its own type, and stop eql-bindings forcingdynamic#1252's step 3, which changes how the dispatch works. This PR only moves the dispatch behind the feature.