Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 15e9c56 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
|
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
 |
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 26 | 0 | 9 | 0 |
Every mutant in the changed lines was caught by a test.
5528f4c to
8c14324
Compare
353cb56 to
0883fb1
Compare
…ee document An equality and a range at two paths under one array could only be asked as `@>` plus an ordering over `json_tree_path_query`, which two different elements can answer: a superset of the plaintext jsonpath `exists($.a[*] ? (@.x == $v && @.y > $w))`. `json_tree_subtree` already computed `k`, the place in `tokens` of the token that found each row, and dropped it. `json_tree_subtree_indexed` returns it, and `json_tree_subtree` is now its projection, so the two reads have one implementation. A client expands a wildcard in element order to one bound, so reads of two fields of one array agree on `k` for one element. `eql_v4.json_tree_element_match(col, p eql_v4.query_json_elements)` takes one operand: per field, its read tokens (probe included) and its bounds as `query_json_leaf` operands. It opens the document once, finds the node each token names by its self tag (the node the subtree read returns at `d = 0`, with nothing beneath it read), pairs the members' nodes by `k`, and compares with the existing entry/leaf operators. A bound is checked by the same validator as the `query_json_leaf` CHECK. A token that names a node at another selector than its member's bounds is the probe's hit on an array longer than the bound, and raises `invalid_parameter_value` instead of answering over part of the array. The domain joins the catalog as the json_tree family's ElementsQuery role, so the eql-bindings inventory and the Go `eql` package pick it up from the generator (`eql.JSONSearchElementsQuery`). Refs #1440
…ment field
`element_match` takes `{"path", "wildcard_bound"?, "predicates"}`: an
array's elements by a path with a wildcard, and predicates on fields of
an element by a member path relative to it. Its operand is
`eql_v4.query_json_elements`: per predicate, the `subtree` tokens of the
member's full path at the bound, and the leaf operands the orderings
derive for its values. `eq` is one leaf as both `gte` and `lte`, so its
equality is the ordering term's (folded text, numbers as float64), and
`between` is `between_at`'s two. The elements' read is derived once and
each member's extends its tokens by the member's steps
(`JsonSubtreeKey::below`, the member and element rules the full path's
key applies). No new derivation.
Refused before anything is derived: a path with no wildcard, a
`wildcard_bound` of 0, a position in the path or a member, a wildcard in
a member, an empty predicate list, an unknown `op`, a value no ordering
takes, and a member whose full path the field does not order. The operator is listed only on a
field that orders a path and whose type names the elements domain
(`TargetDescriptor::with_elements_query_sql_domain`), so a host that
does not name it, such as the Go guest today, sees no new operator.
The heap hygiene suite's `json_open (subtree)` watch fails on this
branch, and passes on the base, with `load_keysets`' 256-byte future box
freed holding a retrieved data key and stack addresses (macOS arm64
debug build). Boxing the keyset loads' `join_all` makes the residue go
away; which write copies it in is not identified, so this is a
workaround, recorded as an observation at the call site.
Refs #1440
…PostgreSQL Seals contact lists with the JSON index, derives `element_match` through a data plan over eql-bindings' target table, and queries the column: two contacts each holding half of the predicate are returned by the superset query (`@>` and an ordering over `json_tree_path_query`) and not by `json_tree_element_match`; one contact holding both is returned by both; a folded spelling ties under the element predicate and not under exact containment; a bound below the longest array raises; and `json_tree_subtree` equals the indexed read without `k` under every budget, the sealed-root fallback at `k = 0`. The rest of the SQL's contract is pinned over the same documents and operands: the operand domain's CHECK member by member, over a derived operand and its parts; one element must satisfy every member under the sealed root and the open one; each bound, `between` and `eq` compare as the entry operators do, and a field with no ordering term or no field satisfies none; a probe's hit raises, including a token whose node was moved to the element's selector; and `k` is the place of the token whose node a row sits in. EQL tests run over real encrypted data, and the SQLx suite has no way to produce a JSON tree document, so these live here. Refs #1440
8c14324 to
15e9c56
Compare
|
Rebased onto #1444's new head (0883fb1); new head 15e9c56. Why:
|
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.
Review attestation: code-review (medium) + cross-check against existing feedback
Reviewed commit: 15e9c56 (base fix/eql-v4-json-function-blockers @ 0883fb1)
Result: no critical or blocking issues. No new findings.
Read in full:
elements.sql- the
types.sqlandfunctions.sqldiffs - the
json_query.rsoperator, input and operand derivation JsonSubtreeKey::belowinderive.rs- the
query.rsandpending.rsdiffs - the catalog, binding and changeset changes
Not read in depth: the large new test files (elements.rs, json_operators.rs). I did not run the test suites; I relied on the PR's stated results.
Checked:
kpairing and the probe.- The count-per-
kpairing is sound. The CHECK requires equal token counts, so onekis one element, including with nested wildcards. - Probe detection by selector holds. A non-probe token's node carries the member's field selector, and the probe's carries the array's.
- A
22023raise on a probe hit is deterministic per row.
- The count-per-
json_tree_subtreeas a projection of_indexed. The wrapper dropskand has one implementation, so the two reads cannot disagree.- Leaf-validator reuse.
query_json_leafand the new elements CHECK sharejson_tree_is_valid_leaf_query_payload. - Existing domain on upgrade. The
IF NOT EXISTSguard keeps an existingquery_json_leafdomain on its old CHECK. The validation is equivalent, so I found no behavioural difference. - Rust refusals and offsets. The refusal ordering is consistent.
member_pathoffset shifting is correct and saturating.belowkeeps probes as they are and refuses wildcards. - Fidelity.
ElementMatchfidelity matches the orderings. - Skills and changesets.
- No
skills/*/SKILL.mdreferences EQL v4json_treeor its operands, so there is no skill drift. - The
@cipherstash/eqlminor changeset is present. - The
stack-encryptandeql-bindingscrates are published through release-plz, so they need no changeset.
- No
Cross-check: all 9 existing review threads (coderdan's efficiency, fidelity, refusal, offset, reuse, double-sort, per-member re-derivation and Box::pin items) are resolved and addressed at head, and I did not re-raise any of them.
Open for the author (already flagged in the PR notes, not a new finding): the pending.rs Box::pin is a workaround with an unidentified mechanism and adds one allocation per dispatch. The author asked for a decision. A future layout change could re-expose the heap-hygiene watch failure.
Still running: the Codex code and security reviews on this commit.
Generated by Claude Code
![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: 15e9c56039
ℹ️ 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".
| pub fn elements_query_sql_domain(&self) -> Option<&'static str> { | ||
| use crate::v4::DomainType; | ||
| document(self.name) | ||
| .map(|_| crate::v4::json_tree::JsonTreeElementsQuery::sql_domain_static()) | ||
| } |
There was a problem hiding this comment.
For Go callers using JsonSearch, the new element_match operation remains unavailable: languages/golang/encrypt/guest/src/targets.rs::describe copies leaf_query_sql_domain() but never copies this new domain into TargetDescriptor. Consequently, dynamic/query.rs::answers filters out ElementMatch, and attempting to derive its operand returns operator_not_answered, despite the newly generated JSONSearchElementsQuery type. Propagate the domain through the production guest descriptor and update the Go checker's operator/input readers to accept element_match and path_predicates; the new Rust tests currently supply the descriptor metadata themselves and therefore bypass this integration gap.
Useful? React with 👍 / 👎.
Summary
EQL v4 stores an encrypted JSON document in a
public.eql_v4_json_searchcolumn. Until now a filter such as "some emergency contact is a Sibling and has more than five calls" could only be written as two separate tests, an equality (@>) and a range (an ordering overeql_v4.json_tree_path_query), and two different contacts could satisfy them. So the query returned more rows than the plaintext jsonpathexists($.contacts[*] ? (@.relationship == "Sibling" && @.calls > 5))does.This PR adds
eql_v4.json_tree_element_match(col, $1::eql_v4.query_json_elements), which is true only when one array element satisfies every part of the predicate, and the Stack Encrypt query operatorelement_matchthat derives its one parameter. No stored format changes: existing documents answer the new query as they are.Changes
EQL (
packages/eql)src/v3/json_tree/functions.sql: neweql_v4.json_tree_subtree_indexed(e, tokens, budget DEFAULT 64) RETURNS TABLE (k integer, entry public.eql_v4_json_entry). It is the existing subtree read body, now returningk, the 1-based place intokensof the token whose node each row sits in (0for the sealed-root fallback past the budget).json_tree_subtreeis now a projection of it, so there is one implementation.src/v3/json_tree/types.sql: new domaineql_v4.query_json_elements({"m": [{"t": [token, …], "gt"|"gte": leaf, "lt"|"lte": leaf}, …]}), with a CHECK that validates each member's tokens and its bounds asquery_json_leafoperands.src/v3/json_tree/elements.sql(new, v4 only):eql_v4.json_tree_element_match. It opens the row's document once, finds the node each member's tokens name by its self tag (the nodejson_tree_subtree_indexedreturns atd = 0, and nothing beneath it), pairs the members' nodes byk, and compares with the existing entry-against-leaf operator functions. Each bound is checked by the same validator thequery_json_leafCHECK now calls (eql_v4_internal.json_tree_is_valid_leaf_query_payload). It raisesinvalid_parameter_value(SQLSTATE22023) when a probe token finds a node (see "How the probe is detected" below).eql_domains::JSON_TREEgains theElementsQueryrole (JsonSearchElementsQuery,eql_v4.query_json_elements). Generated from it: the eql-bindings v4 inventory and the Go typeeql.JSONSearchElementsQueryinlanguages/golang/encrypt/eql/eql_gen.go. Hand-written: theJsonTreeElementsQuerybinding struct, andTarget::elements_query_sql_domain().docs/reference/eql-functions.md(the function, the operand, the folded-equality caveat, the probe error, the planner cost, the hand-writtenEXISTS … JOIN … ON c.k = r.kform), and a bullet injson-tree-leakage.mdon what the query reveals.packages/eql/AGENTS.mdlists the new file and domain.@cipherstash/eqlminor.Engine (
packages/stack-encrypt)dynamic/json_query.rs: a new operator,element_match. Its input is{"path", "wildcard_bound"?, "predicates": [{"member", "op", "json" | "low"+"high"}]}, and it derives theeql_v4.query_json_elementsoperand. It reuses thesubtreetoken derivation forpath + member, and the leaf-operand derivationgt_at/between_atuse.eqis one leaf operand used as bothgteandlte. There is no new cryptographic derivation. The module's operator table has the new row and a section on element predicates.dynamic/query.rs,target.rs,mod.rs:OpSpec::ElementMatch(inputpath_predicates),TargetDescriptor::with_elements_query_sql_domain, and four new refusal reasons:json_path_no_wildcard,json_no_predicates,json_wildcard_bound_zeroandjson_unknown_predicate_op.OpSpec::fidelity's rustdoc says the orderings andelement_matchare exact on the ordering terms, soelement_match'seqis an equality of folded values.json/derive.rs:JsonSubtreeKey::below(steps), which extends a derived read by member and element steps, soelement_matchderives the elements' read once and extends it per member. A test holds it equal to the full path's key under both root options, through nested wildcards.target/pending.rs: oneBox::pinaround the keyset loads'join_all. See the review notes.Verification
Everything ran locally on macOS against a private PostgreSQL 17 container (port 7541), after
mise run buildinpackages/eql.mise x --env test -- cargo nextest run -p stack-encrypt --all-features: 1008 passed. That includes 8 newelement_matchtests intests/json_operators.rs:opderives the ordering's leaf operand, andeqhas lo = hi;subtreeread of its full path, and every member ends with the same probe;wildcard_bound: 0, an unknownop, and a member path error whose offset lands in the member's own text;The heap-hygiene test now covers the operator too.
Root workspace:
cargo clippy --workspace --all-targets -D warningsunder default features and under--all-features,mise run wasm:wasi-check,mise run wasm:no-http-test,mise run wasm:guest:test(76 passed),cargo fmt --all --check, doctests (91) andmise run doc:stack-encrypt: clean.JsonSubtreeKey::belowand its test carry#[cfg(feature = "dynamic")], the feature of its only caller, as the crate gatesMAX_DEPTHandJsonPath::of; without that, every build withoutdynamicreported it as dead code: the WASI check, and also the--all-featuresworkspace clippy, because that builds stack-encrypt a second time as a dev-dependency of the proc-macro crate stack-encrypt-derive, with that crate's features, which do not includedynamic.cargo-mutants,
cargo mutants -p stack-encrypt --in-diff <diff against fix/eql-v4-json-function-blockers>: 35 mutants, 26 caught, 9 unviable, 0 missed (re-run after the rebase onto the new fix(eql): block native jsonb functions on the v4 JSON tree domains #1444 head). The unviable ones areDefault::default()replacements of types that have noDefault.SQLx
lint_tests(12),build_validation_tests(5),selector_literal_guard(10) andv3_public_surface_tests(7): passed.EQL_TEST_DATABASE_URL=… mise run test:encryption:postgreson a fresh database: all suites passed,json_tree28: the 14 existing json_tree PostgreSQL tests (which runjson_tree_subtreeunchanged), the 8 function-blocker tests from fix(eql): block native jsonb functions on the v4 JSON tree domains #1444, and six new tests intests/json_tree/elements.rs. Every one seals real documents with Stack Encrypt under the fake key source and derives its operands through a data plan over eql-bindings' target table; EQL tests must run over real encrypted data, and the SQLx suite has no way to produce a JSON tree document, so the SQL's contract is pinned here rather than in a SQLx file with hand-built documents. Five of them cover:gt,gte,lt,lte,between,eq, and two bounds on one member) compares as the entry operators do, and a field with no ordering term or no field satisfies none;json_tree_subtreeequals the indexed read withoutkunder budgets 0, 1, 2, 3 and 64,kis the place of the token whose node a row sits in, and the sealed root falls back tok = 0.The sixth,
postgres_an_element_predicate_holds_of_one_element, checks:@>plusjson_tree_path_query, and not byjson_tree_element_match;"sibling"ties with"Sibling"under the element predicate but not under exact@>;22023;k, and akjoin pairs the fields of one contact;mise run test:crates(498),test:encryption(541),types:check,typescript:checkandcodegen:parity: passed.The engine also has a
derive.rstest that holdsJsonSubtreeKey::belowequal to the full path's key, under both root options and through nested wildcards.docs:validate:required-tags,docs:validate:coverage,docs:validate:documented-sql(against my own container),test:doc-anchorsandtest:docs_v3_grep: passed.Go:
wasm:guest:build,wasm:auth-guest:build,wasm:guest:build:deterministic,wasm:guest:build:eqlandwasm:guest:build:eql:deterministicbuilt, thenmise run go:testpassed for every package.Not run:
v3_v4_coexistence_tests,v3_privilege_testsandv3_uninstall_testsfail to compile here: they read the credential-generatedfixtures/eql_v3_integer.sqlandv3_ste_vec.sql, and this environment has no CipherStash credentials. They test v3 and integer surfaces. This PR does not change the v3 bundle, because every file it touches is-- ONLY EQL VERSION: 4.pnpm run code:check: no JS or TS changed.go:test:postgres: not run.Related
Closes #1440
Stacked on #1444. Refs #1441 (this operand becomes one leaf kind of that predicate tree) and #1442 (the Go call that wraps it).
Review notes
{"m": [member, …]}, each member{"t": [tokens…], "gt"|"gte": leaf, "lt"|"lte": leaf}.lo/hi, becausegtandltare strict. A plain lo/hi pair could not express that without extra flags.query_json's group validator already does, so the single operand works.query_json_leaf's inline CHECK. Sharing it would mean a new SQL helper, whichpin_search_paththen pins, or aneql-inline-criticalallowlist entry.$.contacts[i].relationship), whose selector is the one the member's bounds carry. The probe names the element itself ($.contacts[N]), at the array's selector. So ad = 0row whose selector differs from its member's raises.truewould be correct in that case, but the error stays deterministic per row rather than depending on which element matched first.Matchon top of #1440 #1442 mirrors:json_path_no_wildcard,json_wildcard_bound_zero,json_no_predicates,json_unknown_predicate_op,json_input(keys that do not fit theop),json_path_wildcard(a wildcard in a member), and the index'sJsonOrderingAtPosition,JsonPathNotOrdered,JsonLeafNotOrdered,JsonPath(at an offset into the member's own text) andJsonTooDeep.subtreestill acceptswildcard_bound: 0: a probe-only read still answers whether an array is empty, where a probe-only predicate can never be true.TermError::JsonPathNotOrderedthe orderings give. The crate keeps path text out of errors, because a key name can be customer data (seeJsonPathError), so the error names neither the path nor which predicate failed. Go SDK: no way to test an equality and a range on the same array element of a JsonSearch field — add an element-scopedMatchon top of #1440 #1442'sstashgencheck runs at generate time, where the path is schema, and that is the place to name it. If you want the engine error to carry the predicate's index, it is a small follow-up.languages/golang/encrypt/guest/src/targets.rs) does not name it yet, so the Go SDK sees no new operator and its pinned operator lists stay unchanged. Go SDK: no way to test an equality and a range on the same array element of a JsonSearch field — add an element-scopedMatchon top of #1440 #1442 needs:with_elements_query_sql_domain(target.elements_query_sql_domain())line in that guest;element_matchadded toparseOperatorName(encrypt/checker.go) and to stashgen'sjsonOperators.Without those, a catalog listing it is refused as an unknown operator.
pending.rsis a workaround with an unidentified mechanism; please decide. Without it,tests/heap_hygiene.rs'sjson_open (subtree)watch fails on this branch every time (macOS arm64, debug build), and passes on the base.load_keysets' future box (256 bytes), holding the retrieved data key at offset 64, the key's first 16 bytes again after it, and two stack addresses, in a pattern repeated later in the block. That looks like an image of a stack frame.Recorded::record). Rewriting that copy to go through no stack temporary left the failure unchanged. It is also not residue from before the watch: scrubbing 1 MiB of stack instead of 256 KiB left it unchanged too.join_all, so the outer future holds the loads as a pointer.json_tree_subtreeand derive per-element reads in the engine #1440 states. Exact per-element equality would need a stored-format addition and is out of scope."json": "\"Sibling\"", not"json": "Sibling".https://claude.ai/code/session_01MrxG724qYKcAWJdcABBFNY