Repository navigation
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 907 files, which is 807 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files.
 |
🦋 Changeset detectedLatest commit: ec5fd0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 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 |
Contributor
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 436 | 0 | 203 | 0 |
cargo-mutants exited with no surviving mutants recorded. The run may not have completed cleanly; check the workflow run logs.
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a05affaaa5
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
freshtonic
previously requested changes
Oct 23, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
I request changes for two release problems on the crates.io side. The code itself is in good shape, and every check I ran passes.
How I reviewed this
The diff is large because generated code is committed. I reviewed the generators and their templates, then I ran the drift checks to prove that the committed output matches them. I did not read each generated file.
- EQL codegen:
eql-codegenrenders the version-neutral SQL source two times (eql_v3andeql_v4) from theeql-domainscatalogue. It also generates the Rust payload types andeql_gen.go. - Go:
stashgengenerates the*_stash.gofiles and the goldens.
Checks run (in a worktree at a05affaaa)
- EQL:
cargo test -p eql-codegen(includes the Go parity test): pass.mise run types:check,codegen:parity,test:schemas:parity: pass.mise run build, with the installer completeness gate and the v4 isolation gate: pass.git statuswas clean after all of these.
- Rust and Go:
cargo fmt --check: pass.cargo clippy --workspace --all-features --all-targets -D warnings: pass.cargo test -p stack-guest-abi -p stack-encrypt --all-features: pass.
- Go guests and SDK:
- All five WASI guests build, and the import-surface checks pass.
go vet ./...andgo test ./...inlanguages/golang: pass.go generate ./...leaves no diff.
- I built the v3 bundle and compared it with the
@cipherstash/eql@3.0.6tarball. Onlylints()and comments changed, so the claim "v3 SQL unchanged" holds. git merge-treeagainst currentmain: no conflicts.cargo publish --dry-run -p stack-encrypt-derive -p stack-encryptagainst crates.io: fails (see below).
Blocking
- stack-encrypt 0.3.0 cannot publish. The root release-plz line publishes every stack-* version that crates.io does not have, so merging this starts a publish of stack-encrypt 0.3.0. stack-encrypt requires
stack-kms0.1.0, and that is the only stack-kms version on crates.io. Butsrc/lib.rsre-exportsstack_kms::{diagnostic, ErrorPayload}, and these were added after the 0.1.0 release with no version bump. The packaged build fails withE0432: unresolved imports stack_kms::diagnostic, stack_kms::ErrorPayload.- Fix: bump stack-kms (for example to 0.2.0, with a CHANGELOG entry), and make stack-encrypt require that version.
- Note that the publish dry run on
main(tests-crates.yml) packages all five crates together, so stack-encrypt resolves the tree's stack-kms there. That step does not catch a stack-kms that crates.io lacks under the same version number.
- The stack-encrypt CHANGELOG has no 0.3.0 section.
Cargo.tomlsays 0.3.0, but about 850 lines, including a### Breakingsection, are still under## [Unreleased]. The root release-plz line only publishes and never rewrites the changelog, so 0.3.0 would ship under "Unreleased", as 0.1.0 did. Please rename the section to## [0.3.0] - <date>in this PR.
Non-blocking (see inline)
SchemaVersionis now generic. Rust does not apply a default const parameter in an expression, soSchemaVersion::CURRENTandSchemaVersion::default()stop compiling (E0284). The changeset ships this as a minor bump and says "plainSchemaVersionstill means v3".- The stored and single-bound query domain templates accept
"v": null. The range template already guards against this. - stack-encrypt will publish with an exact pin on
cllw-ore =0.5.2-rc.1. - There are some small nits: stale vitaminc version numbers in comments, a Go wipe path, an ADR reference, and the position of a
use.
Before you merge
eql-bindings now requires stack-encrypt 0.3.0. Its crates.io publish (publish_all_features) succeeds only after release-crates has published stack-encrypt 0.3.0, so blocker 1 must be fixed first. Otherwise the EQL crate publish fails too.
Checked and fine
- SQL renderer: it refuses literal
eql_v3/eql_v4names, unknown placeholders and unmatched blocks. - Range domains and
between: correct, with STRICT/IMMUTABLE accessors and an inlinablebetween. On non-superuser installs, the ORE range domains are disabled with the same NOT VALID CHECK as the single-bound ORE domains. - v4
ore_block_v2SQL: the layout offsets and the GF(2^128) doubling are correct, and errors do not echo the input bytes. - stack-guest-abi
stepandvalue: no new raw reads of host lengths, handles are never reused,MAX_OPENis enforced, and secrets areZeroizingwithOpaqueDebug. - Go step loop: every early exit after a pause aborts the operation and wipes the buffers.
maxPausedis tied to the Rust constant by a test. - Go public surface:
QueryTerm,VerifyandTargetLimitfollow the existingSeal/Openpattern, andMatchQuerynever prints its text. - CI: actions are pinned by SHA, both copies of the path filters were updated, and no release workflow changed.
- Changesets: the EQL changesets are minor, which is correct apart from the
SchemaVersionpoint.chacha20-0-10-2.mdmatches the lockfile moves. - Skills: no skill update is needed.
stash eql installstill installs v3 only, and no skill covers the Go SDK or Stack Encrypt.
freshtonic
dismissed
their stale review
October 23, 2026 05:17
Not blocking. The points in this review are still open, but they need not stop the merge.
…akage-profile Brings in #1363's format (a self tag on every entry, ordinal-free member tokens, client-side wildcard expansion with a token list, no gates) so the leakage profile can be ported to it. The only conflict was the comment on stack-encrypt's proptest dev-dependency, which both branches use; one line, one comment naming both users. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
The leakage page's comparison table called this the one disclosure v4 makes that v3 does not. It is not new. cipherstash-client emits SteVec entries in document order and marks every array element's entry with the `a` flag (parent_is_array), so the leaves between two flagged entries belong to the element before them; with position selectors (array_index_mode position or all) each leaf's selector names its element outright. Only array_index_mode none, the stack's default, hides it, by indexing nothing inside an array. The design record's copy of the table said the same and is corrected too. Owner's finding. Refs #1363 Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…format
The profile was written against entries {s, c, p?, op?}, member tokens
carrying their ordinal, and a subtree read that sent one head token plus
plaintext gates. #1363 adds a self tag x to every entry, drops the
ordinal from member tokens, and has the client send the tokens of
exactly the nodes a read names, wildcards expanded to a bound with a
probe per expanded array. Run unchanged against it, the module did not
compile (a key now holds a list of tokens), and with that patched the
positive control failed on wire_fields, token_reach, token_derivability,
gated_walk_results and transcripts while every mutant still read as
caught, which is what a broken positive control looks like.
L now states the self tag (one fresh name per entry, so unlinkable
across rows and distinct within one), each read's token list with the
node each token names in each row and what it reaches below, the probe
flag, the budget, and the member or element rule each reached node's
token takes. The simulator programs x and p with the real hash only
where a sent token links them. Of the 27 distinguishers 26 are kept and
gated_walk_results becomes subtree_results (with the sealed-root budget
fallback); eight are new (stored_lengths, self_tag_equalities,
token_counts, bound_recovery, token_hits, wildcard_positions, hit_rules,
ancestor_links). The mutants go from 34 to 44: the two gate mutants are
gone, four are redefined for token lists, and twelve are new, among them
the pre-#1363 top-level token and member rule. All 44 are caught, the
positive control passes on the handcrafted columns and 48 random ones,
and string ordering terms are now modelled folded, as the code and the
page have them.
One finding against the page as #1363 published it: the database's
member-rule-then-element-rule test on every reached node tells it
whether each node below a token is an object member or an array
element, which nothing stored says. The page now says so, and
the_hit_rule_separates_it_from_the_real_scheme keeps the sentence
load-bearing. The SQL comment that the server cannot tell a probe from
any other token is not true of its place in the list or of what it
finds; a_read_gives_away_its_bound_and_its_probes shows it.
Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
… node token_derivability derived member and element children of every token a read sends, which for a wildcard at the default bound is 65 tokens, most of them naming positions no row has, and their children name nothing either. Deriving only from tokens that find a node in some row halves the calibration's time (14.8 s to 9.6 s) with the same 44 of 44 mutants caught by the same distinguishers. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
fix: review findings on the next-to-main release candidate (#1229)
Every line of the release diff arrived through a PR into next, which the gate already mutated. The release diff yields over 2,000 mutants, about 7.5 hours of runner time, and cannot finish inside the 120-minute timeout, so each push to next spent up to two hours and then failed.
BREAKING CHANGE: keyset encrypt/decrypt use opaque Ciphertext and separate context methods. Structural calls move to encrypt_tree/decrypt_tree; borrowed conversion helpers move to target::legacy. Declare owned conversions with EncryptTarget/DecryptSource and explicit context capabilities. Guard plaintext during planning and preserve the existing sealed-leaf bytes and Go/dynamic execution paths.
- Rename the structural entry points encrypt_node / decrypt_node to encrypt_tree / decrypt_tree. - Rename the sealed Scalar trait to OpaqueEncode / OpaqueDecode, the names the API plan accepted. Both stay sealed. - Rename Ciphertext::tag to key_tag, so it cannot be read as the AEAD tag. - Give Ciphertext Serde: padded Base64 in human-readable formats, raw bytes in binary ones. - Report a non-single tree as Error::OpenedType, not Error::Aead (ADR-0010). - Restore NonEmpty on the shared encrypt/open core. The scalar path reaches the core through its own entry that allows empty context, and ADR-0003 records the exception. - Open bytes written by the pre-scalar build through Ciphertext. - Kill the three mutants CI missed. The NoCallerContext conversion is equivalent and is excluded. - Add the CHANGELOG Breaking entries and drop roadmap prose from rustdoc.
Exercise release and deterministic guests through the host checker, preserving typed JSON numbers and sorted keys. Admit the supported ore_block output in host validation. Require fresh producer artifacts without repeating full CI suites. Refs #1460
Compare stored lengths after SQL-null omission, preserve structured source diagnostics at the Go boundary, and share output and refusal assertions. Refs #1461
feat(stack-encrypt): add owned scalar ciphertext API
test(stack-encrypt): add declaration grammar and shared conformance
An omitted SQL-null field can make an extra key leave the row count unchanged. Validate every source key before encoding so Seal cannot silently discard it. Add shared single-row and late-batch regressions.
…conformance test(stack-encrypt): add shared runtime conformance
…formance test(stack-encrypt): add stored-read and context conformance
…ery-conformance test(stack-encrypt): enroll value-kind and query fixtures
This branch was successfully deployed
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
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.
Summary
This is the integration branch for a release candidate of EQL v4 and of Stack Encrypt's Rust and Go support. EQL is the SQL that lets PostgreSQL store and search encrypted values; EQL v4 is a second copy of it for values written by Stack Encrypt, the Rust encryption engine behind the Go SDK. The open pull requests for both are merged here first, so they can be tested together before any of them reaches
main.The release version is not decided yet.
Why a branch, not main
Some of this work depends on vitaminc and ore.rs code that is not yet on crates.io, so it pins those crates to a git commit.
stack-encryptandeql-bindingsare published crates, andcargo publishdrops those git pins, so nothing on this branch can be released until vitaminc 0.6 is published (cipherstash/vitaminc#389, which waits on cipherstash/vitaminc#463). Once it is, the git pins become crates.io versions and this branch merges tomainas a normal release.What lands here, in order
feat/aead-remove-legacy-traitsto one commit on vitamincmain, and takes vitaminc's breaking changes since then.Rules for this branch
rev, neverbranch. A branch is deleted when its stack merges.packages/eqlandpackages/stack-encrypt/fuzz. Two commits mean two copies of vitaminc's traits, and the code stops compiling.Changes
Verification
nextwas created frommainat 2a70a47. Nothing has been built yet, because nothing has changed.Related