test(eql-bindings): build test values as frames of Rust values, not FfiValue by coderdan · Pull Request #1259 · cipherstash/stack

coderdan · 2026-10-18T07:21:54Z

Summary

eql-bindings maps EQL's Postgres types to Rust. Its by-name dispatch, which the Go SDK uses, has taken a ValueFrame since #1245: one plaintext value in vitaminc's transport encoding. But its tests still built every input as an FfiValue, Stack Encrypt's untyped value tree, and read every opened value back into one. That made FfiValue look like a plaintext type eql-bindings depends on, which it no longer is. It's also the first step of #1252, which removes FfiValue altogether.

Changes

Tests (tests/encryption/tests/{v4_types,targets,heap_hygiene}.rs, and the unit tests in src/encryption/targets.rs):
- Inputs are written straight from Rust values: ValueFrame::of(-7i32), ValueFrame::of("café"), and None::<i32> for a null.
- An opened frame is compared byte for byte with the frame of the expected value, which also checks its kind and a float's bit pattern.
- Helpers are renamed to match (as_frame, sealed_from_frame).
- No test changed what it asserts.
Docs that named FfiValue as the dispatch's plaintext, wrong since feat(stack-encrypt)!: hold a dynamic plaintext as its frame, not a value tree #1245:
- the README's dispatch example;
- the stack-encrypt dependency comment in Cargo.toml;
- an unreleased CHANGELOG line;
- the test crate's Cargo.toml comment.

Verification

mise run test:encryption and mise run test:crates: pass. The per-file counts match next: v4_types has 12 passing and 3 Postgres-only ignored, targets 16, heap_hygiene 2, and the encryption::targets unit tests 7.
cargo clippy -p eql-bindings -p eql-encryption-tests --all-features --all-targets -D warnings: clean.
git grep FfiValue -- packages/eql: no matches.

…fiValue eql-bindings' API takes a ValueFrame since the plaintext moved to frames, but its tests still built every input as an FfiValue and read every opened value back into one. That kept FfiValue looking like a plaintext type eql-bindings depends on, which it no longer is. The tests now write a Rust value straight into a frame (ValueFrame::of(-7i32), ValueFrame::of("café")) and compare an opened frame with the frame of the value expected, byte for byte, which checks the kind and a float's bit pattern too. A null is the frame of None::<i32>. The README's dispatch example, a Cargo.toml comment and an unreleased CHANGELOG line still named FfiValue as the dispatch's plaintext and now say ValueFrame. No test changed what it asserts. eql-bindings and its encryption tests no longer name FfiValue anywhere. Refs #1252.

changeset-bot · 2026-10-18T07:21:58Z

⚠️ No Changeset found

Latest commit: 75336d3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

coderabbitai · 2026-10-18T07:22:03Z

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: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: fdc543fe-f967-4f52-8326-35b629636ceb

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

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

coderdan

Automated high-effort review by a Claude Code sub-agent.

What I ran (in a fresh worktree of the stack, #1260 on top of this PR, since #1260 changes no test code):

mise run test:encryption: passes. v4_types 12 passed and 3 ignored, targets 16, heap_hygiene 2, and the encryption::targets unit tests 7 (115 unit tests with targets against 108 without). These match the counts in the description.
cargo clippy -p eql-bindings --features stack-encrypt --all-targets -D warnings and --all-features: clean.
I compared every changed assertion with the one it replaces.

Assertions: nothing got weaker.

same() now compares frame bytes. The frame keeps the integer width (Int32 and Int64 stay apart, as found: Some(ValueKind::Int64) in the unit test shows) and a float's bit pattern. So the check is at least as strict as the old matches! / to_bits() comparison.
opened() now reads through String::sink(). vitaminc's FromScalar for String accepts only (None, Physical::Text), which is the same as the old FfiValue::String match.
None::<i32> encodes as sink.scalar(None, Physical::Null) in vitaminc's Option impl, which is the same null FfiValue::Null produced.
In heap_hygiene, the frame is still built outside watched(), so the spy covers the same calls.

Findings (all low):

The README's interop matrix still says "value enum". packages/eql/crates/eql-bindings/README.md:195: "...in both directions between the typed plaintexts and the value enum." This PR renamed that direction to a frame in v4_types.rs (a_leaf_sealed_from_a_frame_opens_as_the_typed_plaintext), and the module docs there now say "frame". git grep FfiValue misses this line because it never names the type. I'm putting it in this body because the line is outside the diff.
One test frames a frame twice. See the inline comment on v4_types.rs:532.
The README sentence on line 121 reads badly. See the inline comment.

chatgpt-codex-connector · 2026-10-18T07:32:42Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-18T07:35:26.404792Z	`36aadd6`	Draft marked ready
🔒 Security Review	✅ Completed 2026-10-18T07:34:29.983372Z	`36aadd6`	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

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

Review attestation: Claude Code `/code-review medium` + `/cross-check-review-findings`

Reviewed: head 36aadd6, all 8 changed files (full diff).

Critical / blocking: none.
Findings: none.

What I checked:

Every FfiValue use in v4_types.rs, targets.rs, heap_hygiene.rs and the src/encryption/targets.rs unit tests was replaced with the equivalent ValueFrame built from the same Rust value. The kinds are kept: frame::<i32> vs frame::<i64> still drives the "another kind is refused" cases, and None::<i32> still stands in for null.
same() now compares frame bytes. That checks kind and value, and a float's bit pattern, so the -0.0 and NaN assertions are no weaker than the old to_bits() match. The assertions are unchanged.
The README example, Cargo.toml comments and CHANGELOG line now match what the code does.
The Rust CI jobs (cargo test + clippy + rustfmt, Rust workspace crates, Build test archive) are green on this head. Go macOS/Windows, the Go live tests and the Postgres shards were still running when I looked.
No changeset is needed: this is tests and docs only, and the bot's "No Changeset" notice is expected. The CHANGELOG edit rewords an unreleased line.

Nothing to fix before merge. This is a review of the diff and CI state, not a local build or test run.

Generated by Claude Code

In a_value_of_another_kind_is_refused_not_converted each case is already built as a ValueFrame. Wrapping it in frame() again ran ValueFrame::of over a frame and wrote the same bytes, which read as if the value were still an FfiValue. Pass the frame straight to targets::encrypt. Refs #1252

…c frame The typed-path paragraph named stack_encrypt::dynamic::ValueFrame to say which leaf each type seals. A typed-only build does not build Stack Encrypt's dynamic module, so point at the Go SDK instead, which is the compatibility the sentence is about. The sentence also read badly. The interoperability matrix still called the other direction "the value enum". The tests now build value frames, so say that. Refs #1252

coderdan · 2026-10-18T07:50:25Z

Reply to the review body's finding 1 (outside the diff, so no inline thread):

README.md:195 still said "the value enum". Fixed in 75336d3. The interop matrix now says the tests run "in both directions between the typed plaintexts and the by-name dispatch's value frames."

Findings 2 and 3 are answered on their inline threads (458d37a and 75336d3).

Checks on 75336d3, from packages/eql: cargo fmt --all --check, mise run test:encryption (clippy included), mise run check:encryption:wasi, RUSTDOCFLAGS="-D warnings" cargo doc -p eql-bindings --no-deps --features stack-encrypt, cargo check -p eql-bindings --no-default-features and mise run types:check all pass. (targets does not exist on this branch; #1260 adds it.) #1260 is rebased onto this head.

…pes compiles on next #1257 added a TextEq query to the v4 query fixture with an FfiValue input, and #1259 removed FfiValue from v4_types.rs. Each passed on its own; merged together on next, the test file does not compile. The input is now frame("alice@example.com"), as every other query there is, and the fixture's bytes are unchanged. (cherry picked from commit cd6d7c5)

coderdan mentioned this pull request Oct 18, 2026

feat(eql-bindings): put the by-name dispatch behind a targets feature, and keep dynamic out of stack-encrypt #1260

Merged

coderdan commented Oct 18, 2026

View reviewed changes

Comment thread packages/eql/tests/encryption/tests/v4_types.rs Outdated

Comment thread packages/eql/crates/eql-bindings/README.md Outdated

coderdan marked this pull request as ready for review October 18, 2026 07:32

coderdan requested a review from a team as a code owner October 18, 2026 07:32

cipherstash-agent reviewed Oct 18, 2026

View reviewed changes

coderdan added 2 commits October 18, 2026 18:44

coderdan removed the request for review from a team October 18, 2026 07:59

coderdan merged commit d9bd121 into next Oct 18, 2026
42 checks passed

coderdan deleted the test/eql-bindings-no-ffivalue branch October 18, 2026 08:15

coderdan mentioned this pull request Oct 18, 2026

fix(eql): build the TextEq query fixture's input as a frame, so v4_types compiles on next #1261

Merged

coderdan mentioned this pull request Oct 18, 2026

feat(eql-bindings): query EQL v4 types through Stack Encrypt's typed query chain #1244

Merged

Navigation Menu

Repository navigation

test(eql-bindings): build test values as frames of Rust values, not FfiValue - #1259

test(eql-bindings): build test values as frames of Rust values, not FfiValue#1259
coderdan merged 3 commits into
nextfrom
test/eql-bindings-no-ffivalue

coderdan commented Oct 18, 2026 •

edited

Loading

Uh oh!

changeset-bot Bot commented Oct 18, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 18, 2026 •

edited

Loading

Review skipped

Uh oh!

coderdan left a comment

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Oct 18, 2026 •

edited

Loading

Uh oh!

cipherstash-agent left a comment

Uh oh!

coderdan commented Oct 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

coderdan commented Oct 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Verification

Related

Uh oh!

changeset-bot Bot commented Oct 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

coderabbitai Bot commented Oct 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

coderdan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Oct 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

cipherstash-agent left a comment

Choose a reason for hiding this comment

Review attestation: Claude Code /code-review medium + /cross-check-review-findings

Uh oh!

coderdan commented Oct 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderdan commented Oct 18, 2026 •

edited

Loading

changeset-bot Bot commented Oct 18, 2026 •

edited

Loading

coderabbitai Bot commented Oct 18, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Oct 18, 2026 •

edited

Loading

Review attestation: Claude Code `/code-review medium` + `/cross-check-review-findings`