Repository navigation
Conversation
Contributor
WalkthroughWarning Review details and warnings were omitted to fit the comment limit. |
🦋 Changeset detectedLatest commit: c8a9c80 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.
… Planned A data plan was lowered by building a typed plan from it and running that, so the plan builder sat in the dynamic path and could not be removed. `compose` and `opening` in `dynamic::planned` now build each field's combinator from the parsed plan, as ADR-0007's amendment of 9 November 2026 decided, and `encrypt_with`, `decrypt_with`, `check_source`, `check_record` and `Plan::contexts` run them. The whole-plan rules the builder checked for a data plan (names once, no shared identity, plain labels) are checked where the plan is parsed. No stored byte changes: the combinators, labels and order are the builder's, and the conformance vectors and the equivalence suite pin them. The builder, the chain and the old traits are removed in the pull requests that follow. Closes #1120. Refs #1488.
The review of #1597 found five in-diff mutants surviving. Three were equivalent: the "keys nothing" distinction in `compose` under a label context, where a passthrough ignores its context either way, and the context-field disjunct of the predicate, dead because a context field is always a passthrough. The distinction is removed rather than excluded. Two were checks nothing reached: `check_stored` and `check_names` are now reached by unit tests in `planned.rs`. `validate` is deleted. Its duplicate-field and label rules were refused by the parser first; the one live rule, shared identity between keyed fields, is in `Plan::build`'s loop beside the target rule. The builder's `join`, `field_label`, `FROM_PLAN` and `FROM_CALL` are shared with the lowering rather than copied. A target field is skipped by `operation_of` returning `None`, so no `unreachable!` sits in library code. Docs that still described lowering through the builder, or that said the checks run the lowering, are reworded: the checks share `check_names`, `check_stored` and `opening_base` with it. Tests added: a context field declared last, a batch with absent fields under a context field opened with an expected context, the stored and value checks refusing each kind of misfit, and the shared-identity table with exact reasons. Refs #1488.
…ule's docs The crate's rustdoc is what a user reads; a design record is for the people who maintain the crate. The two sentences state what the code does and leave the record out. Refs #1598.
Each sealed field's closure read the record's context field by copying the frame and parsing the copy, once per field per row. The frame is now read in place through `IntoLabel for &ValueFrame`, so what each field does per row is one parse of a short text and no copy. Parsing once per row would need a slot the field closures share, which the description has no place for: each field's `under_by` sees the row alone. Refs #1488.
…nators The ADR listed the two check functions among the callers of `compose` and `opening`. They run no combinator: they state the same rules through the checks the lowering shares, as the CHANGELOG says. Refs #1488.
…field markers A field marker had operator methods only where the field's type was one index. A field of several indexes, a nullable field and a field that is a record had none, so those fields could be queried only through the plan chain that #1488 removes. A tuple of terms now answers each operator through the one term that answers it. Rust's coherence rules allow no single impl that picks that term, so each term states its answer to each scalar operator as a type (op::OperatorAnswer, Yes or No), and op::AnsweredBy picks the one Yes. An operator no term answers, or two answer, stays a compile error. Option<T> answers what a present value answers; a test shows a present value's term is a bare value's. Marker::field reaches an inner record's markers under the outer field's context. The marker now names where its field sits (marker::Root<R> or marker::Nested<W>), so field refuses a marker of another record and the marker stays Copy with no depth limit. A query wraps the caller's scope in each outer field's context, outermost first, as the derive seals a nested record. (marker::Takes<T>), checked by the derive against the plaintext field, so an equality marker no longer takes a value of any type. Closes #1596
…ord field markers OreBlockTerm had no LabelledTarget impl, so a block ORE field, alone or beside an equality index, had a marker with no operator method. It is sealed under the caller's context like the other order terms. field() now bounds the inner record's scope with NestedScope, so an inner record with a scope of its own is refused with a message that says why, rather than a bare type mismatch. QueryValue is sealed for every type parameter. Root's outer labels are pushed by an empty method, so no equivalent mutant is left for the mutants gate. The docs say that an untyped nullable marker takes an Option, and that None derives a term no row holds; query = .. refuses it, and a ui case pins that. The tests add a caller binding and stored bindings around a nested record, an outer field under another identity, a nullable field of two indexes, an empty nested scope, block ORE, and the chain's tuple query without .via. The CHANGELOG describes the final Marker. Refs #1596
…text feat(stack-encrypt): show the context a declaration or a call uses, and pin it in tests
…nd-bindings feat(stack-encrypt): split the context into scopes and bindings
…y-preparation feat(stack-encrypt)!: declare each operator once and prepare a query once
…rkers feat(stack-encrypt): field markers and a value under a plan that is data
…lowering refactor(stack-encrypt): lower a data plan to the combinators in dynamic::planned
…ields feat(stack-encrypt): operator methods for tuple, nullable and record field markers
The rustdoc of stack-encrypt and stack-encrypt-derive is what a user reads on docs.rs and in an editor. It named the crates' design records (the ADRs under packages/stack-encrypt/docs/adr and the plans under docs/plans) in 47 places. Those records are not published with the crate, so a user could not follow the reference. Each sentence now states the rule or the mechanism itself. Where a maintainer benefits from the pointer, it moved to a plain // comment. Three history references go too: a closed issue cited twice on NullMode, and a closed tracking issue in the read docs, which now say that query_term reads its input as the field's kind. tests/doc_hygiene.rs reads every source file of both crates and fails, naming file and line, on a /// or //! line that names an ADR number, docs/adr or docs/plans. Closes #1598.
The derive crate's attribute reference, pulled into its rustdoc with include_str!, still named the API plan, so the guard now scans every file a doc attribute includes. It also flags an ADR that ends a line, whose number sits on the next one, and matches case-insensitively. The JSON module's docs compared the code with its plan; they now describe the behaviour, and the comparison is a maintainer comment. The query docs said a type answers with one query domain per operator; EQL v4 has one per type family and term, as the code binds it. A few more history phrases go, and the row-binding sentence says what naming the row costs. Refs #1598.
… inner record with its own scope A field marker is the typed way to query a field of a derived record. Two kinds of record field had no marker query. The plan chain was the only typed way to query them, and #1488 removes the chain. Marker::field now takes a marker of the record a field holds or holds an Option of (marker::Holds). The inner record may list a scope of its own. The result's scope type is the outer record's scope joined with the inner one (marker::JoinScope), and MarkedQuery::scope takes them as one tuple, outermost first. The query extends the field's label as the derive seals it: each record's scope goes before the context of the field that holds the record. marker::NestedScope is removed. It was added in this unreleased version. Closes #1600
Clarify the explanation regarding row binding and its impact on ciphertext protection. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
JsonSelector::open was corrected in the previous commit. The same claim, that a row binding makes selectors and terms that row's alone, stood in Error::JsonWrongEntry. A binding protects the ciphertext; selectors and terms derive under the query scopes alone, as tests/scopes_and_bindings.rs pins.
A fourth record with a scope of its own now fails with the message about the three-scope limit. Before, the three-scope tuple had one `JoinScope` implementation, so rustc decided the inner marker's scope was `NoScope` and reported a type mismatch that said the inner record must list no scope. A second implementation stops that inference and carries the message. Its bound is a trait whose one implementation is for a type no caller can name. A new trybuild case pins it. Tests now cover a binding beside a nested scope: an inner record that stores a scope and a binding, under a record with a caller scope and a caller binding, as a field and as a nullable field, and a nullable record that stores a binding and no scope. A nullable record inside a nullable record is covered too. `contexts()` is checked for the nullable record with a scope and for three scopes. The empty-scope tests match `PlanError::EmptyPart`, and the absent-record test compares the query with a present record's term. `extension` checks in debug builds that it has one scope for each record that lists one. `GivenScopes` is implemented for flat tuples of `Scope` only, the same types as `ScopeInput`. The docs no longer say that a marker's scope is always the record's caller scope: a stored scope gives the same type, and a nested marker can take a tuple. The CHANGELOG no longer says a nested marker takes only the outer record's scope.
…no-adrs docs(stack-encrypt): state the rules the rustdoc cited records for
…in plain language The rustdoc for a field that is a record, and for a query that needs more than one scope, now says who does what in short sentences, and lists the context parts and the mistakes that do not compile. The three compiler messages state one thing each. The message for a fourth scope names the limit, its label names the call that adds the scope, and its note names the module to use instead. The docs now also say that two scopes of the same type, passed in the wrong order, compile and match no row.
A query on a field of a nested record took its scopes as a tuple: .scope((Scope::new(outer), Scope::new(inner))). The caller input everywhere else is a chain that starts at Scope::new, such as Scope::new(tenant).bind(row). A query now takes the same form: .scope(Scope::new(outer).scope(inner)). Scope::scope returns the new target::Scopes<Outer, Inner>, and Scopes::scope adds the scope of a record one level further in. Scopes nests, so a query no longer has a limit of three scopes. The FourthScope trait and its compile error, which only reported that limit, are removed. Refs #1600
Stack Encrypt's dynamic interface runs a plan that arrives as data. A field of that plan can name an EQL type, and a program gives the engine the EQL types through the TargetResolver trait. Nobody shipped one, so the Go guest and three EQL encryption tests each wrote the same resolver over encryption::targets, and the copies had drifted: two test copies named fewer of a JSON document's query domains than the guest. encryption::EqlTargets is the guest's resolver, moved into eql-bindings behind the targets feature. The row-to-descriptor and refusal conversions are From impls beside it. The guest and the tests use it and their copies are gone, so a change to the trait's signature is made once. Closes #1571
- Name the three refusals that convert to `Other` (`DocumentQuery`, `NoIndex`, `Options`) instead of a wildcard, so a variant added later must be placed by hand. A test shows the engine refuses options on a type with no JSON index at plan time, before the resolver is called. - Move the explanation and the example onto `EqlTargets`. The `resolver` module is private, so rustdoc did not show its module doc. - Test the `Unproducible` and `Json` conversions and the `Other` arms. - Drop the braces around a single `self as eql_targets` import. - The API plan no longer says each program writes its own resolver.
The docs for EqlTargets and its two conversions, and the module docs of the Go guest's resolver, now use short sentences that name who does what. They say Stack Encrypt where they said the engine.
…cope-markers feat(stack-encrypt): typed queries for a nullable record field and an inner record with its own scope
…solver feat(eql-bindings): ship one TargetResolver for the EQL types
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