feat: client 0.38.0 migration — drop vendored stack-auth + EQL v2.3 mapper support (CIP-3233)#409
Open
freshtonic wants to merge 10 commits into
Open
feat: client 0.38.0 migration — drop vendored stack-auth + EQL v2.3 mapper support (CIP-3233)#409freshtonic wants to merge 10 commits into
freshtonic wants to merge 10 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 16 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates proxy encryption and SQL rewriting to use ChangesProxy EQL and index-aware rewrite migration
Vendored stack-auth cleanup
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…tack-auth patch (CIP-3233) Moves Proxy off the `vendor/stack-auth` `[patch.crates-io]` workaround and onto the current released cipherstash-client group, built against the fixed stack-auth. Background: 2.2.4 (PR #408) shipped the CIP-3233 access-key token-refresh fix via a vendored stack-auth patched on top of the 0.34.1-alpha.4 source. cipherstash-client 0.38.0 links stack-auth 0.38.0, which carries the same fix from crates.io, so the vendored copy and patch are no longer needed. Changes: - cipherstash-client / cipherstash-config / cts-common: 0.34.1-alpha.4 -> 0.38.0 (carries the API migration from PR #406's 0.37.0 upgrade; 0.37 -> 0.38 needed no further source changes) - Remove `[patch.crates-io] stack-auth = { path = "vendor/stack-auth" }`, the `exclude = ["vendor/stack-auth"]` workspace entry, and the vendor/stack-auth tree - stack-auth now resolves from crates.io (0.38.0); single version of the cipherstash-client group in the lock (zerokms-protocol 0.12.19) Verified: `cargo check --workspace`, `cargo clippy --workspace --all-targets`, and `cargo test --workspace --lib` (111 proxy unit tests) all pass. Integration tests need a live DB/ZeroKMS and were not run here.
freshtonic
force-pushed
the
james/cip-3233-proxy-drop-vendor-patch
branch
from
256ad08 to
2e11f69
Compare
freshtonic
changed the title
cipherstash-client 0.38.0 emits structured JSONB (SteVec) encrypted
values as {"k":"sv",...} without a top-level `c` field. The pinned EQL
2.3.0-pre.3 enforced a top-level `c` on every encrypted value via
eql_v2._encrypted_check_c, rejecting these payloads with EP0001
("Encrypted column missing ciphertext (c) field") and failing all
JSONB/SteVec integration tests.
EQL 2.3.1 relaxes the check to `(val ? 'c') OR (val ? 'sv')`, accepting
the new SteVec format.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cipherstash-proxy/src/postgresql/messages/bind.rs (1)
84-96: 🗄️ Data Integrity & Integration | 🟠 MajorConfirm EqlOutput serialization format mismatch for EQL 2.3.1
The workspace is pinned to
cipherstash-clientversion 0.38.0, whereEqlOutputserializes viaserde_json::to_valueinto a JSON object containing a top-levelc(ciphertext) field. The required EQL 2.3.1 format expects a structure without this top-levelcwrapper. As written,rewrite()writes the legacy payload, which will fail or corrupt data ineql_v2_encryptedcolumns expecting the new format. Update the serialization logic to produce the schema-free format expected by the EQL 2.3.1 backend, likely requiring manual construction of the JSON payload from theEqlOutputcomponents or updating the dependency if a fix is available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cipherstash-proxy/src/postgresql/messages/bind.rs` around lines 84 - 96, The bind::rewrite method is serializing EqlOutput with serde_json::to_value, which preserves the legacy top-level c wrapper and does not match the EQL 2.3.1 payload shape. Update rewrite() to emit the schema-free JSON expected by eql_v2_encrypted columns, either by manually building the payload from EqlOutput’s fields or by switching to a dependency/version that already produces the new format. Keep the change localized to bind::rewrite and the EqlOutput serialization path it uses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 46-48: The Cargo.toml dependency update points cipherstash-client,
cipherstash-config, and cts-common at unpublished 0.38.0 versions, which will
break dependency resolution. Revert these entries to a published crates.io
version or restore the [patch.crates-io] override to a valid local/git source
for stack-auth until those crates are available. Keep the existing dependency
names and patch configuration aligned so Cargo can resolve them successfully.
---
Outside diff comments:
In `@packages/cipherstash-proxy/src/postgresql/messages/bind.rs`:
- Around line 84-96: The bind::rewrite method is serializing EqlOutput with
serde_json::to_value, which preserves the legacy top-level c wrapper and does
not match the EQL 2.3.1 payload shape. Update rewrite() to emit the schema-free
JSON expected by eql_v2_encrypted columns, either by manually building the
payload from EqlOutput’s fields or by switching to a dependency/version that
already produces the new format. Keep the change localized to bind::rewrite and
the EqlOutput serialization path it uses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25c7c19a-7daf-43a1-9720-a75b783fae42
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockvendor/stack-auth/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
Cargo.tomlmise.local.example.tomlmise.tomlpackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/lib.rspackages/cipherstash-proxy/src/postgresql/backend.rspackages/cipherstash-proxy/src/postgresql/context/mod.rspackages/cipherstash-proxy/src/postgresql/frontend.rspackages/cipherstash-proxy/src/postgresql/messages/bind.rspackages/cipherstash-proxy/src/postgresql/messages/data_row.rspackages/cipherstash-proxy/src/proxy/encrypt_config/manager.rspackages/cipherstash-proxy/src/proxy/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rsvendor/stack-auth/.gitignorevendor/stack-auth/Cargo.tomlvendor/stack-auth/LICENSEvendor/stack-auth/README.mdvendor/stack-auth/examples/auto_strategy.rsvendor/stack-auth/examples/device_code.rsvendor/stack-auth/src/access_key.rsvendor/stack-auth/src/access_key_refresher.rsvendor/stack-auth/src/access_key_strategy.rsvendor/stack-auth/src/auto_refresh.rsvendor/stack-auth/src/auto_strategy.rsvendor/stack-auth/src/device_client.rsvendor/stack-auth/src/device_code/mod.rsvendor/stack-auth/src/device_code/protocol.rsvendor/stack-auth/src/device_code/tests.rsvendor/stack-auth/src/lib.rsvendor/stack-auth/src/oauth_refresher.rsvendor/stack-auth/src/oauth_strategy.rsvendor/stack-auth/src/refresher.rsvendor/stack-auth/src/service_token.rsvendor/stack-auth/src/static_token_strategy.rsvendor/stack-auth/src/token.rs
💤 Files with no reviewable changes (22)
- vendor/stack-auth/src/static_token_strategy.rs
- vendor/stack-auth/src/device_code/tests.rs
- vendor/stack-auth/README.md
- vendor/stack-auth/src/auto_strategy.rs
- vendor/stack-auth/src/oauth_refresher.rs
- vendor/stack-auth/.gitignore
- vendor/stack-auth/src/access_key.rs
- vendor/stack-auth/Cargo.toml
- vendor/stack-auth/src/device_client.rs
- vendor/stack-auth/src/device_code/protocol.rs
- vendor/stack-auth/src/device_code/mod.rs
- vendor/stack-auth/src/refresher.rs
- vendor/stack-auth/src/service_token.rs
- vendor/stack-auth/src/oauth_strategy.rs
- vendor/stack-auth/src/access_key_refresher.rs
- vendor/stack-auth/src/access_key_strategy.rs
- vendor/stack-auth/examples/auto_strategy.rs
- vendor/stack-auth/LICENSE
- vendor/stack-auth/src/token.rs
- vendor/stack-auth/src/lib.rs
- vendor/stack-auth/src/auto_refresh.rs
- vendor/stack-auth/examples/device_code.rs
Comment on lines
+46
to
+48
| cipherstash-client = { version = "0.38.0" } | ||
| cipherstash-config = { version = "0.38.0" } | ||
| cts-common = { version = "0.38.0" } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm published versions and that no stale patch/exclude remains
for c in cipherstash-client cipherstash-config cts-common; do
echo "== $c =="
curl -s "https://crates.io/api/v1/crates/$c" | jq -r '.versions[].num' | rg -n '0\.38\.0' || echo "0.38.0 NOT found"
done
echo "== residual stack-auth references =="
rg -n 'stack-auth|vendor/stack-auth' Cargo.toml Cargo.lock 2>/dev/nullRepository: cipherstash/proxy
Length of output: 550
Critical Build Failure: Versions 0.38.0 are unpublished.
The versions 0.38.0 for cipherstash-client, cipherstash-config, and cts-common are not available on crates.io. Removing the [patch.crates-io] override with these versions will cause the build to fail immediately as the resolver cannot find these artifacts.
Required Action:
- Revert the
Cargo.tomlchanges to use a published version or restore the[patch.crates-io]entry with a valid local or git source forstack-authuntil the crates are published. Do not merge this PR in its current state.
[dangerous_changes]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` around lines 46 - 48, The Cargo.toml dependency update points
cipherstash-client, cipherstash-config, and cts-common at unpublished 0.38.0
versions, which will break dependency resolution. Revert these entries to a
published crates.io version or restore the [patch.crates-io] override to a valid
local/git source for stack-auth until those crates are available. Keep the
existing dependency names and patch configuration aligned so Cargo can resolve
them successfully.
…nt 0.38.0 (CIP-3279)
cipherstash-client 0.38.0 stores jsonb leaves as STE-vec elements carrying
CLLW ORE (`oc`), and EQL 2.3.1's `->` returns `eql_v2.ste_vec_entry`. Bare
comparison/order on the extracted entry routed to the root Block-ORE path
(`eql_v2.compare` → `ore_block_u64_8_256`, requires `ob`) and failed with
`Expected an ore index (ob) value`. Field projection returned the raw
encrypted sv element instead of decrypting it.
Group B (range/order): new RewriteJsonbSteVecOrdering rule rewrites
`(col -> sel) <op> $param` into
`eql_v2.ore_cllw((col -> sel)::JSONB) <op> eql_v2.ore_cllw($param::JSONB)`,
with a new EqlTerm::SteVecTerm variant (bound Ord) and post-inference RHS
reclassification so the param/literal is encrypted as a CLLW STE-vec query
term (`oc`) via QueryOp::SteVecTerm.
Group C (field projection): data_row reshapes a bare ste_vec_entry jsonb
(text `{` and binary `0x01`) into a decryptable EqlCiphertext; from_sql
parses sv-entry scalars (numbers as f64 to match storage's orderable
encoding).
Fixes 20 integration tests (8 range/order + 12 field projection), verified
end-to-end against live proxy + PostgreSQL + ZeroKMS. jsonb sv equality
(select_where_jsonb_eq, jsonb_term_filter) remains a separate follow-up.
…dering (CIP-3283)
eql-mapper previously saw only abstract EqlTraits for an encrypted column,
never the concrete index type, so transformation rules could not target
index-specific SQL. Introduce a side-channel IndexResolver (TableColumn ->
{IndexKind}) consulted only by transformation rules — inference, unification,
and the EqlValue/EqlTerm output types are deliberately untouched, and the
crate takes no dependency on cipherstash-config. The proxy populates the
resolver from EncryptConfig (already keyed by table+column); an
EmptyIndexResolver reproduces prior behaviour as the safe default.
First consumer: RewriteScalarOpeOrdering rewrites scalar (non-jsonb) OPE
columns so PostgreSQL compares the order-preserving `op` ciphertext directly
with built-ins only:
col <op> $p -> decode(col->>'op','hex') <op> decode($p->>'op','hex')
ORDER BY col -> ORDER BY decode(col->>'op','hex') (ASC/DESC/NULLS kept)
ORE columns, equality, and jsonb sv accessors are left untouched.
This lets scalar OPE ordering work Proxy-side with NO EQL change (potentially
retiring CIP-3282), gated on the client storing/query-encoding `op` (CIP-3280).
Verified: cargo test -p eql-mapper (94 pass, +9), proxy lib (new tests pass;
config::tandem failures are a pre-existing parallelism flake), mise run check
clean. NOT yet code-reviewed (the review pass did not complete). e2e blocked
on the unpublished CIP-3280 client.
freshtonic
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cipherstash-proxy/src/postgresql/data/from_sql.rs (1)
325-352: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
eql_termin the binaryTEXT/VARCHARfallback.The new
SteVecTermJSON path is skipped when a driver sends the RHS as binaryTEXT/VARCHAR: this fallback still hard-codesEqlTermVariant::Full, so jsonb STE-vec ordering values get converted toPlaintext::Jsoninstead of the scalarPlaintextthatQueryOp::SteVecTermexpects.Suggested fix
- (_, _, &Type::TEXT | &Type::VARCHAR) => parse_bytes_from_sql::<String>(bytes, pg_type) - .and_then(|val| text_from_sql(&val, EqlTermVariant::Full, col_type)), + (eql_term, _, &Type::TEXT | &Type::VARCHAR) => { + parse_bytes_from_sql::<String>(bytes, pg_type) + .and_then(|val| text_from_sql(&val, eql_term, col_type)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cipherstash-proxy/src/postgresql/data/from_sql.rs` around lines 325 - 352, The TEXT/VARCHAR fallback in from_sql::from_sql is dropping the original eql_term by hard-coding EqlTermVariant::Full, which breaks the SteVecTerm JSON scalar path. Update that fallback to pass through the incoming eql_term instead of forcing Full, so QueryOp::SteVecTerm still reaches text_from_sql/json_scalar_to_plaintext with the correct variant. Keep the existing JSON/JSONB/BYTEA handling intact and make sure the fallback in the final &Type::TEXT | &Type::VARCHAR arm preserves the caller’s term for both binary and text driver inputs.
🧹 Nitpick comments (1)
packages/eql-mapper/src/lib.rs (1)
2333-2335: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact
Partialvariant here.This guard says root-scalar RHS params must remain
EqlTerm::Partial, but!matches!(SteVecTerm)would still pass forFullor another wrong EQL variant.Proposed test tightening
- assert!( - !matches!(value, Value::Eql(EqlTerm::SteVecTerm(_))), - "root-scalar ordering RHS must NOT be reclassified to SteVecTerm, got {value}" - ); + assert!( + matches!(value, Value::Eql(EqlTerm::Partial(_, _))), + "root-scalar ordering RHS must remain EqlTerm::Partial, got {value}" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/eql-mapper/src/lib.rs` around lines 2333 - 2335, The guard in the root-scalar ordering check is too loose because it only excludes Value::Eql(EqlTerm::SteVecTerm(_)) and would still allow other incorrect variants. Update the assertion in the relevant ordering/RHS validation path to match the exact expected EqlTerm::Partial shape, using the same value being checked there, so the logic fails for any non-Partial EQL term. Tighten the corresponding test around this root-scalar ordering case to verify the RHS remains Partial and does not accept Full or other variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cipherstash-proxy/src/proxy/encrypt_config/index_resolver.rs`:
- Around line 53-56: The config lookup is using a bare table name in index
resolution, which breaks matches for schema-qualified EncryptConfig keys. Update
the identifier construction in `IndexResolver` so `Identifier::new(...)` uses
the fully qualified table name for `TableColumn` lookup, either by
preserving/retrieving the schema when building `TableColumn` or by resolving it
at lookup time before matching against config entries. Ensure the fix is applied
in the `IndexResolver` path that currently reads `table_column.table.value` so
schema-qualified tables no longer fall back to the empty-set rewrite path.
In `@packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs`:
- Around line 205-214: The SteVecTerm mapping in the zero-kms EqlTermVariant
match currently falls back to Store when no SteVec index is found, which can
silently drop the STE-vec query term. Update the EqlTermVariant::SteVecTerm
branch in the zerokms mapper to fail fast or otherwise prevent this path from
returning EqlOperation::Store; keep the operation as a query only when an
IndexType::SteVec is present, and use the surrounding EqlOperation::Query /
QueryOp::SteVecTerm handling as the place to enforce the invariant.
In
`@packages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_ordering.rs`:
- Around line 191-197: The rewrite in would_edit currently gates on
self.is_eql_typed(right), which is too broad and allows non-SteVec encrypted
expressions to be rewritten incorrectly. Update the condition in
rewrite_jsonb_ste_vec_ordering::would_edit to require the RHS to be classified
as a SteVecTerm (using the existing term/classification helpers in this rule)
instead of any EQL-typed expression, so only single STE-vec comparison terms
trigger the jsonb accessor ordering rewrite.
In `@packages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rs`:
- Around line 173-185: The `would_edit_comparison` check in
`rewrite_scalar_ope_ordering.rs` is too permissive because it only verifies that
the RHS is EQL-typed, not that it is OPE-backed. Update the guard in
`would_edit_comparison` so the rewrite only applies when the right-hand
expression is confirmed to carry the needed OPE backing/`op` term, using the
same kind of resolver-based validation already used for `is_scalar_ope_column`
on the left. Keep the binary-operator and left-column checks intact, but add the
missing RHS-specific predicate before allowing the rewrite.
In `@packages/eql-mapper/src/type_checked_statement.rs`:
- Around line 167-170: Gate the JSONB STE-vec rewrite behind IndexResolver
checks in RewriteJsonbSteVecOrdering and the type_checked_statement wiring.
Update RewriteJsonbSteVecOrdering to accept the resolver alongside node_types,
and use the same resolve(table_column).contains(&IndexKind::SteVec) predicate
before rewriting JsonLike columns or classifying RHS values as SteVecTerm, so
columns without a configured STE-vec index do not get rewritten to
ore_cllw(...).
---
Outside diff comments:
In `@packages/cipherstash-proxy/src/postgresql/data/from_sql.rs`:
- Around line 325-352: The TEXT/VARCHAR fallback in from_sql::from_sql is
dropping the original eql_term by hard-coding EqlTermVariant::Full, which breaks
the SteVecTerm JSON scalar path. Update that fallback to pass through the
incoming eql_term instead of forcing Full, so QueryOp::SteVecTerm still reaches
text_from_sql/json_scalar_to_plaintext with the correct variant. Keep the
existing JSON/JSONB/BYTEA handling intact and make sure the fallback in the
final &Type::TEXT | &Type::VARCHAR arm preserves the caller’s term for both
binary and text driver inputs.
---
Nitpick comments:
In `@packages/eql-mapper/src/lib.rs`:
- Around line 2333-2335: The guard in the root-scalar ordering check is too
loose because it only excludes Value::Eql(EqlTerm::SteVecTerm(_)) and would
still allow other incorrect variants. Update the assertion in the relevant
ordering/RHS validation path to match the exact expected EqlTerm::Partial shape,
using the same value being checked there, so the logic fails for any non-Partial
EQL term. Tighten the corresponding test around this root-scalar ordering case
to verify the RHS remains Partial and does not accept Full or other variants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0ef3fd4-1fb6-44ea-aac1-a8954e8b56b6
📒 Files selected for processing (19)
packages/cipherstash-proxy/src/postgresql/context/mod.rspackages/cipherstash-proxy/src/postgresql/data/from_sql.rspackages/cipherstash-proxy/src/postgresql/frontend.rspackages/cipherstash-proxy/src/postgresql/messages/data_row.rspackages/cipherstash-proxy/src/proxy/encrypt_config/index_resolver.rspackages/cipherstash-proxy/src/proxy/encrypt_config/manager.rspackages/cipherstash-proxy/src/proxy/encrypt_config/mod.rspackages/cipherstash-proxy/src/proxy/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rspackages/eql-mapper/src/eql_mapper.rspackages/eql-mapper/src/index_resolver.rspackages/eql-mapper/src/inference/unifier/eql_traits.rspackages/eql-mapper/src/inference/unifier/types.rspackages/eql-mapper/src/lib.rspackages/eql-mapper/src/ste_vec_ordering.rspackages/eql-mapper/src/transformation_rules/mod.rspackages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_ordering.rspackages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rspackages/eql-mapper/src/type_checked_statement.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cipherstash-proxy/src/proxy/encrypt_config/manager.rs
- packages/cipherstash-proxy/src/postgresql/messages/data_row.rs
Comment on lines
+53
to
+56
| let identifier = Identifier::new( | ||
| table_column.table.value.to_string(), | ||
| table_column.column.value.to_string(), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TableColumn definition / construction =="
rg -n -C4 'struct TableColumn|TableColumn\s*\{' packages/eql-mapper/src
rg -n -C4 'table:\s*Ident::new|column:\s*Ident::new|TableColumn \{' packages/eql-mapper/src packages/cipherstash-proxy/src
echo
echo "== EncryptConfig identifier derivation in the proxy =="
rg -n -C4 'Identifier::new|get_column_config' packages/cipherstash-proxy/src
echo
echo "== ColumnMapper lookup path =="
fd -i 'column_mapper*.rs' packages/cipherstash-proxy/src -x sed -n '1,240p' {}
echo
echo "== Existing schema-qualified config coverage =="
rg -n -C3 'my_schema\.users|Identifier::new\("my_schema\.users"' packages/cipherstash-proxy/srcRepository: cipherstash/proxy
Length of output: 50373
Config lookup fails for schema-qualified tables due to bare name usage.
The EncryptConfig uses schema-qualified keys (e.g., "my_schema.users"), but TableColumn stores only the bare table name. The schema is stripped when converting OverlayTable to Table (packages/eql-mapper/src/model/schema_delta.rs line 100), resulting in Identifier::new("users", ...) which cannot match the config entries.
This causes index-aware rewrites to silently fallback to the empty-set path for columns in schema-qualified tables.
Use TableColumn.table.value only if the configuration keys are guaranteed to use bare names. Otherwise, resolve the table identifier to include the schema prefix at the source of construction or retrieve the full qualified name during lookup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cipherstash-proxy/src/proxy/encrypt_config/index_resolver.rs` around
lines 53 - 56, The config lookup is using a bare table name in index resolution,
which breaks matches for schema-qualified EncryptConfig keys. Update the
identifier construction in `IndexResolver` so `Identifier::new(...)` uses the
fully qualified table name for `TableColumn` lookup, either by
preserving/retrieving the schema when building `TableColumn` or by resolving it
at lookup time before matching against config entries. Ensure the fix is applied
in the `IndexResolver` path that currently reads `table_column.table.value` so
schema-qualified tables no longer fall back to the empty-set rewrite path.
Comment on lines
+205
to
+214
| // SteVecTerm generates a CLLW ORE comparison term (`oc`) for the | ||
| // right-hand side of a jsonb sv ordering comparison | ||
| // (`col -> selector <op> $param`). | ||
| EqlTermVariant::SteVecTerm => col | ||
| .config | ||
| .indexes | ||
| .iter() | ||
| .find(|i| matches!(i.index_type, IndexType::SteVec { .. })) | ||
| .map(|index| EqlOperation::Query(&index.index_type, QueryOp::SteVecTerm)) | ||
| .unwrap_or(EqlOperation::Store), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't downgrade SteVecTerm to Store.
Once the mapper has classified a value as SteVecTerm, the SQL path is already committed to eql_v2.ore_cllw(...). Falling back to Store here emits a root payload without the STE-vec query term the rewritten SQL reads, so this should fail fast (or be prevented upstream) instead of silently changing the operation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs` around lines 205 -
214, The SteVecTerm mapping in the zero-kms EqlTermVariant match currently falls
back to Store when no SteVec index is found, which can silently drop the STE-vec
query term. Update the EqlTermVariant::SteVecTerm branch in the zerokms mapper
to fail fast or otherwise prevent this path from returning EqlOperation::Store;
keep the operation as a query only when an IndexType::SteVec is present, and use
the surrounding EqlOperation::Query / QueryOp::SteVecTerm handling as the place
to enforce the invariant.
Comment on lines
+191
to
+197
| fn would_edit<N: Visitable>(&mut self, node_path: &NodePath<'ast>, _target_node: &N) -> bool { | ||
| if let Some((Expr::BinaryOp { left, op, right },)) = node_path.last_1_as::<Expr>() { | ||
| if is_ordering_operator(op) | ||
| && is_ste_vec_accessor(left) | ||
| && self.is_eql_typed(left) | ||
| && self.is_eql_typed(right) | ||
| { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate this rewrite on SteVecTerm, not any EQL RHS.
self.is_eql_typed(right) also matches full/partial encrypted expressions, so jsonb_accessor <op> other_encrypted_expr will be rewritten to eql_v2.ore_cllw(...) even when the RHS was never classified as a single STE-vec comparison term. That compares the wrong payload shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_ordering.rs`
around lines 191 - 197, The rewrite in would_edit currently gates on
self.is_eql_typed(right), which is too broad and allows non-SteVec encrypted
expressions to be rewritten incorrectly. Update the condition in
rewrite_jsonb_ste_vec_ordering::would_edit to require the RHS to be classified
as a SteVecTerm (using the existing term/classification helpers in this rule)
instead of any EQL-typed expression, so only single STE-vec comparison terms
trigger the jsonb accessor ordering rewrite.
Comment on lines
+173
to
+185
| /// Returns `true` if the binary expression at the head of `node_path` is a | ||
| /// scalar OPE ordering comparison that this rule would rewrite. | ||
| fn would_edit_comparison(&self, node_path: &NodePath<'ast>) -> bool { | ||
| if let Some((Expr::BinaryOp { left, op, right },)) = node_path.last_1_as::<Expr>() { | ||
| return matches!( | ||
| op, | ||
| BinaryOperator::Lt | ||
| | BinaryOperator::LtEq | ||
| | BinaryOperator::Gt | ||
| | BinaryOperator::GtEq | ||
| ) && self.is_scalar_ope_column(left) | ||
| && self.is_eql_typed(right); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require an OPE-backed RHS before rewriting the comparison.
The left side is gated through the index resolver, but the right side is only checked for “some EQL type”. That means ope_col <op> rhs_expr can be rewritten to decode(rhs_expr ->> 'op', 'hex') even when rhs_expr is not OPE-backed and has no op term.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rs`
around lines 173 - 185, The `would_edit_comparison` check in
`rewrite_scalar_ope_ordering.rs` is too permissive because it only verifies that
the RHS is EQL-typed, not that it is OPE-backed. Update the guard in
`would_edit_comparison` so the rewrite only applies when the right-hand
expression is confirmed to carry the needed OPE backing/`op` term, using the
same kind of resolver-based validation already used for `is_scalar_ope_column`
on the left. Keep the binary-operator and left-column checks intact, but add the
missing RHS-specific predicate before allowing the rewrite.
Comment on lines
+167
to
+170
| RewriteJsonbSteVecOrdering::new(Arc::clone(&self.node_types)), | ||
| RewriteScalarOpeOrdering::new( | ||
| Arc::clone(&self.node_types), | ||
| Arc::clone(&self.index_resolver), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Gate the JSONB STE-vec rewrite on IndexKind::SteVec.
RewriteJsonbSteVecOrdering is wired with only node_types, so it cannot honor the IndexResolver empty-set/no-concrete-info contract. A JsonLike column without a configured STE-vec index can still be rewritten to ore_cllw(...), while eql_mapper.rs also reclassifies its RHS as SteVecTerm. Please pass the resolver into this rule and use the same resolve(table_column).contains(&IndexKind::SteVec) predicate for the RHS param/literal classification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/eql-mapper/src/type_checked_statement.rs` around lines 167 - 170,
Gate the JSONB STE-vec rewrite behind IndexResolver checks in
RewriteJsonbSteVecOrdering and the type_checked_statement wiring. Update
RewriteJsonbSteVecOrdering to accept the resolver alongside node_types, and use
the same resolve(table_column).contains(&IndexKind::SteVec) predicate before
rewriting JsonLike columns or classifying RHS values as SteVecTerm, so columns
without a configured STE-vec index do not get rewritten to ore_cllw(...).
…IP-3283)
Address non-blocking review nits on RewriteScalarOpeOrdering:
- Add tests pinning behaviour for shapes around the rule's match surface:
- col BETWEEN $a AND $b (Expr::Between) is not rewritten
- MIN(col)/MAX(col) route to eql_v2 aggregates, not decode(op)
- $p < col (column on the right) IS rewritten symmetrically -- a bare
comparison param unifies to the same eql column, so the rule fires for
either operand ordering (documented in the test; not a coverage gap)
- Add a multi-key ORDER BY test asserting only the OPE sort key is
rewritten to decode(col->>'op','hex') with directions/NULLS preserved
- Tighten the rule rustdoc: mem::replace is the ownership mechanism that
moves the RHS node intact (so downstream cast rules re-encrypt it), not
the reason the op slot is preserved
- Leave a TODO(CIP-3280) for the NULL-invariant scalar-OPE e2e test, which
is blocked on the unpublished client that can carry op end-to-end
…P-3281)
Rewrites jsonb STE-vec equality comparisons so they bind to EQL 2.3.1's
XOR-aware eql_v2.eq_term extractor instead of root eql_v2_encrypted
equality. The LHS sv-element accessor (col -> sel, already an
eql_v2.ste_vec_entry) is wrapped in eql_v2.eq_term; the jsonb_path_query_first
form (an eql_v2_encrypted) is cast to ::JSONB::eql_v2.ste_vec_entry first.
The RHS query payload is a root sv payload ({k:sv, ..., hm|oc}) that lacks
the s/c fields the ste_vec_entry DOMAIN CHECK requires, so it cannot use the
documented ::eql_v2.ste_vec_entry recipe. Instead the eq_term body is inlined
on the raw jsonb: decode(coalesce(rhs ->> 'hm', rhs ->> 'oc'), 'hex'). This
coalesces whichever deterministic term the column leaf carries (oc for CLLW
ORE leaves, hm for hmac/term-filter leaves), so no proxy-side query-op change
is needed: the existing QueryOp::SteVecTerm path already emits the matching
term.
The RHS param/literal is reclassified to EqlTerm::SteVecTerm via the
generalised ste_vec_term_rhs_keys, which now collects both ordering and
equality RHS operands. Both rewrite rules and the reclassification gate on the
same shared predicates plus the shared rhs_as_jsonb operand helper, so they
cannot drift apart.
Verified: cargo test -p eql-mapper (100 passed), proxy lib (121 passed),
fmt/clippy -D warnings/check --locked clean. e2e (select_where_jsonb_eq,
jsonb_term_filter) compiles but was NOT run -- no live PG/ZeroKMS in sandbox.
Points the proxy at the published cipherstash-client 0.38.1-alpha.1 — a pre-release of the suite `fix/scalar-ope-oc-term` branch (CIP-3280) that stores + query-encodes the scalar OPE `op` term. This lets CI exercise the full integration suite, including scalar OPE ordering (map_ope_*), against a client that carries the op term. Temporary: revert to a stable published cipherstash-client before merge (once CIP-3280 / suite PR #2061 lands and a release ships).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/eql-mapper/src/ste_vec_ordering.rs`:
- Around line 97-144: The rhs_as_jsonb helper is based on an outdated cast shape
and now always falls back because RewriteJsonbSteVecOrdering runs before
CastParamsAsEncrypted and CastLiteralsAsEncrypted. Update rhs_as_jsonb to match
the actual transformer order by removing the special-case logic for stripping
eql_v2_encrypted and simplifying it to return the expression unchanged when it
is already a JSONB cast, otherwise wrap it in a JSONB cast. Keep the behavior
localized in rhs_as_jsonb so the STE-vec ordering rewrite remains aligned with
make_transformer execution.
In
`@packages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_equality.rs`:
- Around line 146-154: The RHS handling in build_eq_term_rhs is cloning the
expression before extracting hm and oc, which can break NodeKey identity and
prevent CastLiteralsAsEncrypted from rewriting encrypted literals. Update
rewrite_jsonb_ste_vec_equality::build_eq_term_rhs to follow the same
single-consumption pattern used by the ordering rule: derive the hm/oc path
without passing cloned Expr values into field_text, or extract the needed fields
before any clone so the original rhs node remains intact for the transformer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5929dbcc-b3e9-4b2f-871e-904dac54d064
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
CHANGELOG.mdCargo.tomlpackages/cipherstash-proxy-integration/src/map_ope_index_order.rspackages/cipherstash-proxy/src/postgresql/data/from_sql.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rspackages/eql-mapper/src/eql_mapper.rspackages/eql-mapper/src/lib.rspackages/eql-mapper/src/ste_vec_ordering.rspackages/eql-mapper/src/transformation_rules/mod.rspackages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_equality.rspackages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_ordering.rspackages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rspackages/eql-mapper/src/type_checked_statement.rs
✅ Files skipped from review due to trivial changes (3)
- CHANGELOG.md
- packages/cipherstash-proxy-integration/src/map_ope_index_order.rs
- packages/eql-mapper/src/transformation_rules/mod.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_ordering.rs
- packages/eql-mapper/src/type_checked_statement.rs
- packages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rs
- packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
- packages/cipherstash-proxy/src/postgresql/data/from_sql.rs
- packages/eql-mapper/src/eql_mapper.rs
- Cargo.toml
- packages/eql-mapper/src/lib.rs
Comment on lines
+97
to
+144
| /// Reduces a STE-vec comparison's right-hand-side operand to a bare `::JSONB` | ||
| /// value. | ||
| /// | ||
| /// The casting rules wrap encrypted params/literals as | ||
| /// `<value>::JSONB::eql_v2_encrypted`. The STE-vec extractors | ||
| /// (`eql_v2.ore_cllw(jsonb)` for ordering, the inlined `eq_term` `->>` reads for | ||
| /// equality) operate on raw `jsonb`, so the outer `::eql_v2_encrypted` cast is | ||
| /// stripped, leaving `<value>::JSONB`. If the expression is not in the expected | ||
| /// double-cast shape it is wrapped in a `::JSONB` cast defensively. | ||
| /// | ||
| /// This cast-shape logic is encryption-binding-critical and shared by both | ||
| /// rewrite rules: a divergence would bind a value encrypted one way to SQL that | ||
| /// extracts it another way, silently producing wrong results. | ||
| pub(crate) fn rhs_as_jsonb(expr: Expr) -> Expr { | ||
| if let Expr::Cast { | ||
| kind: CastKind::DoubleColon, | ||
| expr: inner, | ||
| data_type: DataType::Custom(name, _), | ||
| .. | ||
| } = &expr | ||
| { | ||
| let is_encrypted = name.0.len() == 1 | ||
| && matches!( | ||
| &name.0[0], | ||
| ObjectNamePart::Identifier(ident) | ||
| if ident.value.eq_ignore_ascii_case("eql_v2_encrypted") | ||
| ); | ||
|
|
||
| if is_encrypted { | ||
| if let Expr::Cast { | ||
| data_type: DataType::JSONB, | ||
| .. | ||
| } = &**inner | ||
| { | ||
| // Strip the outer `::eql_v2_encrypted` cast, keeping `<value>::JSONB`. | ||
| return (**inner).clone(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Defensive fallback: cast whatever we have to JSONB. | ||
| Expr::Cast { | ||
| kind: CastKind::DoubleColon, | ||
| expr: Box::new(expr), | ||
| data_type: DataType::JSONB, | ||
| format: None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how transforms are driven (traversal direction) and rule ordering.
rg -nP "fn apply_transform|trait Transform\b|fn transform\b|\benter\b|\bexit\b" \
packages/eql-mapper/src/transformation_rules/mod.rs -C2
# Locate and outline the cast rules to see what shape they emit and when.
fd -t f -e rs . packages/eql-mapper/src/transformation_rules | rg -i "cast_(param|literal)"
fd -t f -e rs . packages/eql-mapper/src/transformation_rules \
| rg -i "cast_(param|literal)" \
| xargs -I{} sh -c 'echo "== {} =="; ast-grep outline {}'Repository: cipherstash/proxy
Length of output: 1251
Align rhs_as_jsonb with actual transform execution order
The make_transformer sequence applies RewriteJsonbSteVecOrdering before CastParamsAsEncrypted/CastLiteralsAsEncrypted. Consequently, rhs_as_jsonb receives a raw jsonb value rather than the documented ::JSONB::eql_v2_encrypted shape, causing it to always take the defensive fallback branch. Remove the obsolete logic attempting to strip a cast that has not yet been applied and simplify the function to return the value cast to JSONB.
// Simplified to match actual execution order
pub(crate) fn rhs_as_jsonb(expr: Expr) -> Expr {
if let Expr::Cast {
kind: CastKind::DoubleColon,
data_type: DataType::JSONB,
..
} = &expr {
return expr;
}
Expr::Cast {
kind: CastKind::DoubleColon,
expr: Box::new(expr),
data_type: DataType::JSONB,
format: None,
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/eql-mapper/src/ste_vec_ordering.rs` around lines 97 - 144, The
rhs_as_jsonb helper is based on an outdated cast shape and now always falls back
because RewriteJsonbSteVecOrdering runs before CastParamsAsEncrypted and
CastLiteralsAsEncrypted. Update rhs_as_jsonb to match the actual transformer
order by removing the special-case logic for stripping eql_v2_encrypted and
simplifying it to return the expression unchanged when it is already a JSONB
cast, otherwise wrap it in a JSONB cast. Keep the behavior localized in
rhs_as_jsonb so the STE-vec ordering rewrite remains aligned with
make_transformer execution.
Comment on lines
+146
to
+154
| fn build_eq_term_rhs(expr: Expr) -> Expr { | ||
| let rhs = rhs_as_jsonb(expr); | ||
|
|
||
| let hm = Self::field_text(rhs.clone(), "hm"); | ||
| let oc = Self::field_text(rhs, "oc"); | ||
| let coalesce = Self::call("", "coalesce", vec![hm, oc]); | ||
|
|
||
| Self::call("", "decode", vec![coalesce, Self::string_literal("hex")]) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How CastLiteralsAsEncrypted matches/replaces literal nodes.
fd -t f -e rs . packages/eql-mapper/src/transformation_rules | rg -i "cast_literal"
fd -t f -e rs . packages/eql-mapper/src/transformation_rules \
| rg -i "cast_literal" \
| xargs -I{} sh -c 'echo "== {} =="; rg -nP "NodeKey|encrypted_literals|downcast|replace" {} -A3 -B1'
# NodeKey identity semantics (pointer vs structural).
rg -nP "struct NodeKey|impl .* NodeKey|fn new\b|Hash for NodeKey|PartialEq for NodeKey" \
$(fd -t f node_key.rs) 2>/dev/nullRepository: cipherstash/proxy
Length of output: 1545
Cloning the RHS arguments risks bypassing literal encryption.
build_eq_term_rhs clones rhs to extract hm and oc fields. The CastLiteralsAsEncrypted transformer (packages/eql-mapper/src/transformation_rules/cast_literals_as_encrypted.rs) maps NodeKey identifiers directly to ast::Value nodes. Modifying the input to pass clones to field_text breaks the association with the original NodeKey, meaning encrypted literals in the hm or oc paths may fail to be replaced, leaking plaintext.
The ordering rule handles this by consuming rhs exactly once to preserve key identity. Use the same pattern here: resolve the hm or oc values into a single path without intermediate cloning, or extract them before the cloned call so the original Expr node remains the one visited by the transformer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/eql-mapper/src/transformation_rules/rewrite_jsonb_ste_vec_equality.rs`
around lines 146 - 154, The RHS handling in build_eq_term_rhs is cloning the
expression before extracting hm and oc, which can break NodeKey identity and
prevent CastLiteralsAsEncrypted from rewriting encrypted literals. Update
rewrite_jsonb_ste_vec_equality::build_eq_term_rhs to follow the same
single-consumption pattern used by the ordering rule: derive the hm/oc path
without passing cloned Expr values into field_text, or extract the needed fields
before any clone so the original rhs node remains intact for the transformer.
…CIP-3283) The scalar OPE ordering/range rewrite emitted `decode(col ->> 'op','hex')`, but `eql_v2_encrypted` defines its own `->>` operator (a selector accessor, alias of `->`), so the bare `->>` bound to that instead of native jsonb extraction — PostgreSQL then tried to parse 'op' as a composite literal and raised `EP0001/22P02 malformed record literal: "op"` at runtime. Cast the operand to its base `jsonb` type first (`decode((col)::jsonb ->> 'op','hex')`) so native jsonb field extraction is used, mirroring the jsonb STE-vec rules. Unit tests asserted the SQL string so they passed while the SQL was invalid; only e2e (CI, PG14-17) caught it. Updated the rewrite tests to expect the `::JSONB` cast.
A jsonb STE-vec *term* comparison with a bare numeric literal on the right-hand side (e.g. `jsonb_path_query_first(pii, '$.path') > 70`) failed at runtime with a fatal `SteVecTerm only supports scalar values` error. The eql-mapper correctly reclassifies the literal to `EqlTerm::SteVecTerm`, but `literal_from_sql` converted bare numeric literals via the generic `Value::Number` arm, which ignores `eql_term` and (for a Json cast type) produced a non-scalar `Plaintext::Json`. The client's `QueryOp::SteVecTerm` rejects non-scalars. Parameters and quoted string literals were unaffected because they route through `text_from_sql`/`binary_from_sql`, which already reduce sv-term values to scalars. Add a guarded arm that maps a bare numeric sv-term literal to `Plaintext::Float` (f64), matching how cipherstash-client encodes stored jsonb numeric leaves so ordering/equality comparisons stay consistent. Verified end-to-end: the showcase Test 4 (high cardiovascular risk, `> 70`) now passes and the full showcase completes successfully.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cipherstash-proxy/src/postgresql/data/from_sql.rs`:
- Around line 603-625: The SteVecTerm RHS collection in eql_mapper’s
reclassification loop only handles Expr::Value, so negative literals parsed as
Expr::UnaryOp are skipped. Update the matching logic to also recognize
Expr::UnaryOp with Minus and unwrap its inner Number or Placeholder before
inserting into the literals set. Keep the existing Expr::Value behavior intact,
and ensure the same path is used for both direct values and unary-negated RHS
operands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 476940ea-358a-4c14-b47c-ad8596083d00
📒 Files selected for processing (4)
CHANGELOG.mdpackages/cipherstash-proxy/src/postgresql/data/from_sql.rspackages/eql-mapper/src/lib.rspackages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/eql-mapper/src/transformation_rules/rewrite_scalar_ope_ordering.rs
- packages/eql-mapper/src/lib.rs
Comment on lines
+603
to
+625
| /// A *negative* numeric value reaching the sv-term literal arm must keep its | ||
| /// sign through the f64 reduction. | ||
| /// | ||
| /// Note: SQL `-70` is parsed by sqlparser as | ||
| /// `Expr::UnaryOp { op: Minus, expr: Value::Number("70") }`, so the | ||
| /// `Value::Number` seen here is the *positive* `70` and the sign lives in a | ||
| /// separate AST node — handling negation in the literal collection path is a | ||
| /// pre-existing concern outside this conversion. This test pins that the | ||
| /// conversion itself is sign-correct when it does receive a negative | ||
| /// `BigDecimal` (e.g. a directly-constructed value), so the f64 mapping is | ||
| /// not where a sign would be lost. | ||
| #[test] | ||
| pub fn ste_vec_term_negative_numeric_literal_keeps_sign() { | ||
| log::init(LogConfig::default()); | ||
|
|
||
| let literal = Value::Number(BigDecimal::from_str("-70").unwrap(), false); | ||
|
|
||
| let pt = literal_from_sql(&literal, EqlTermVariant::SteVecTerm, ColumnType::Json) | ||
| .unwrap() | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(pt, Plaintext::Float(Some(-70.0))); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Map the RHS-collection function
ast-grep outline packages/eql-mapper/src/eql_mapper.rs --match ste_vec_term_rhs_keys
# Show how the `right` operand is matched for reclassification
rg -nP -C4 'if let Expr::Value' packages/eql-mapper/src/eql_mapper.rs
# Look for any unary-minus / negative-literal handling in the sv-term path
rg -nP 'UnaryOp|Minus|negat' packages/eql-mapper/src/eql_mapper.rsRepository: cipherstash/proxy
Length of output: 572
Support UnaryOp for negative sv-term RHS literals
The reclassification loop in packages/eql-mapper/src/eql_mapper.rs (lines 322–332) collects values for SteVecTerm only if the right operand matches Expr::Value. Negative numbers like -70 parse as Expr::UnaryOp { op: Minus, expr: Value::Number("70") }, causing them to be skipped during collection.
This creates a mismatch: SQL rewrite rules transform the comparison assuming the RHS is a term, but the engine encrypts the RHS as a generic payload. The query will fail or return incorrect results for such negatives.
Extend the operand matching to handle Expr::UnaryOp (unwrapping Minus on Number or Placeholder).
Current strict matching
if let Expr::Value(value_with_span) = &**right {
match &value_with_span.value {
ast::Value::Placeholder(p) => {
// ...
}
other => {
literals.insert(NodeKey::new(other));
}
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cipherstash-proxy/src/postgresql/data/from_sql.rs` around lines 603
- 625, The SteVecTerm RHS collection in eql_mapper’s reclassification loop only
handles Expr::Value, so negative literals parsed as Expr::UnaryOp are skipped.
Update the matching logic to also recognize Expr::UnaryOp with Minus and unwrap
its inner Number or Placeholder before inserting into the literals set. Keep the
existing Expr::Value behavior intact, and ensure the same path is used for both
direct values and unary-negated RHS operands.
Reverts the CI-only pin to the suite PR #2061 prerelease, restoring the stable published cipherstash-client 0.38.0. The prerelease was used to validate scalar OPE end-to-end in CI (it carries the CIP-3280 `op`-store fix); that validation is complete (full suite green on PG14-17). Until CIP-3280 (suite PR #2061) is released and this is repointed to the new stable version, the 11 scalar `map_ope_*` integration tests will fail (0.38.0 does not store the OPE `op` term). All other tests remain green.
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
2 participants
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
Moves Proxy off the
vendor/stack-auth[patch.crates-io]workaround onto the releasedcipherstash-clientgroup (CIP-3233 follow-up) — and carries the proxy-side work needed to keep the EQL/encrypted-query path working against the new payload format thatcipherstash-client 0.38.0introduced.Warning
Work in progress — not merge-ready. CI is still red (see "Status / outstanding"). The
0.38.0upgrade changed the encrypted-payload format (v2.3), which broke 38 integration tests; this PR fixes the bulk of them but two pieces are still in flight and one commit is pending re-review.What's in this PR (by commit)
2e11f698cipherstash-client/config/cts-common0.34.1-alpha.4 → 0.38.0; remove vendoredstack-auth+[patch.crates-io]+exclude. stack-auth now resolves from crates.io 0.38.0.b38eeb0aeql-2.3.0-pre.3→eql-2.3.1—_encrypted_check_cnow accepts SteVec ((val ? 'c') OR (val ? 'sv')), fixing the storage-check failures the new client's JSONB format triggered.d1abdd2bRewriteJsonbSteVecOrderingrule rewrites(col -> sel) <op> $pto the CLLW extractor form (eql_v2.ore_cllw(...)); proxy decrypts/reshapes bareste_vec_entry. Fixes 20 jsonb tests (e2e-verified).1af99682IndexResolver(TableColumn → {IndexKind}) consulted only by transformation rules (inference/unifier untouched, nocipherstash-configdep);RewriteScalarOpeOrderingrewrites scalar OPE ordering/range todecode(col->>'op','hex')bytea comparison (built-ins only — no EQL change).Why the extra scope
cipherstash-client 0.38.0(v2.3 EQL payload) changed how encrypted values are stored/searched:oc) → bare>routed to the root Block-ORE path and failed → CIP-3279 rewrites to theste_vec_entry/ore_cllwextractor form.opbytea comparison (order-preserving), Proxy-side, with no EQL change.Cross-repo dependency
Scalar OPE end-to-end also needs the client to store + query-encode the
opterm — that's cipherstash-suite PR #2061 (CIP-3280). Until that ships, scalarmap_ope_*cannot pass e2e. (CIP-3283's Proxy-side approach means the EQL-side change CIP-3282 / U-005 reversal is likely not needed.)Verification
cargo test -p eql-mapper— 94 passcargo test -p cipherstash-proxy --lib— pass (the only failures are a pre-existingconfig::tandemparallelism flake, present on the clean base too)mise run check(fmt + clippy + compile) — cleanStatus / outstanding (why CI is red)
select_where_jsonb_eq,jsonb_term_filter, 9 tests) — not yet implemented (CIP-3281; Proxy-only, EQL already provideseq_term).map_ope_*, 11 tests) — gated on the clientop-store (suite PR #2061) landing/publishing.1af99682(CIP-3283) pending code review.Notes
Summary by CodeRabbit
New Features
ORDER BYhandling.EqlOutput) across query flows.Bug Fixes
Documentation