Repository navigation
feat(stack-encrypt)!: derive every order term over its ADR-0002 order key, and order floats - #1242
Conversation
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 28
Comment |
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 425 | 0 | 342 | 0 |
Every mutant in the changed lines was caught by a test.
This comment has been minimized.
This comment has been minimized.
|
Parked until new This PR takes #1235's workaround further. It builds every kind's vitaminc-ore order key inside Stack Encrypt and encrypts that key with No released or built version of either crate does that yet. Once one does, this PR gets reworked:
Most of the tests carry over: the float order checks, the |
54a204d to
f915cf5
Compare
91eb65e to
2e6be7c
Compare
|
Unparked, and reworked on top of #1235. #1235 now encrypts every order term through
The float tests, the |
a33c4ba to
2dbdf38
Compare
|
Rebased onto #1235, which removed the inverted-range check from |
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-review-findings
Reviewed head 2dbdf38 against base ccd4431, covering the engine (op::Ordered, the data-plan ordered()), the stashgen and enginetest changes, the test models and fixtures, and the docs.
Critical / blocking: none.
Non-blocking (1):
- The older
packages/stack-encrypt/CHANGELOG.mdentry still saysf64only and that anf32ordering does not compile. The new entry saysf32works (inline comment).
Cross-check:
- The
ordered!list, the data-planordered()and the stashgen/enginetestKindFloathandling agree. - The
f32path (f32widens to thef64it widens to,-0.0ties0.0, NaN sorts last) is covered by new tests on the typed path, the data path and the Rust and Go fixtures. - The diagnostics and UI snapshots are updated to name the floats.
CI at review time:
- Passed: Rust fmt, clippy, nextest and doctests, Mutants gate, CRAP, Go lint, fuzz regressions, and the TS and e2e suites.
- Still running: "WASI check, guests and Go (Linux)" and the Postgres shards. I could not check those results.
Source: automated review of the PR diff.
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: 2dbdf386d9
ℹ️ 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".
ccd4431 to
ec45e28
Compare
2dbdf38 to
fa4652a
Compare
|
Reworked onto the reworked #1235 (which is now on the current This PR is now one commit, What changed beyond the rebase:
Same caveat as #1235: Verification:
|
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Review: float ordering through cllw-ore's F64 target (head fa4652a, this PR's own diff against #1235's ec45e28)
Reviewed with the focus on the float ordering semantics, that stack-encrypt hands the value to the ORE crate rather than encoding it, key and domain separation, the dependency pin, Go parity, and the tests. The crypto path is sound; the findings are about the PR's framing, its CI state and fixture churn.
Findings, most severe first
-
The PR title still describes the draft this PR no longer is. The title says "derive every order term over its ADR-0002 order key, and order floats"; the one commit is "order floats through cllw-ore's F64 target", and the body itself says #1235 did the order-term rework. Whoever reads the merge commit, or
git log --oneline, will think this PR moved every order term. The!is in the same position: nothing in this commit's diff changes a stored byte or removes anything that shipped (TermError::NarrowFloatonly ever existed in the draft), so the breaking part the CHANGELOG entry points at ("Float order terms change bytes with every other order term (above)") is #1235's. Rename the PR to the commit's title; whether to keep the!is a judgement call, but the body should say which PR carries the byte change. -
Three Rust jobs are red at this head, all on one test, and the test is not this PR's.
fmt, clippy, nextest, doctests, rustdoc,CRAP (stack-encrypt)andMutants gateeach fail onstored_rows::every_frozen_row_opens_and_seals_as_it_did(tests/stored_rows.rs:238), which fails identically on #1235's head. I checked this PR's engine diff for anything byte-affecting:src/op/mod.rsonly addsf32, f64toordered!, andsrc/dynamic/query.rsonly adds the two kinds toordered();Orderable for f32/f64 => F64and the dynamicFloat::F32/F64dispatch are already in the base. So the failure is inherited, not introduced, but it means two things for this PR: it cannot go green until #1235 settles the frozen-row question (the file's own rule is that a deliberate format change addsstored_rows_v2.jsonrather than editing v1), and the "2 mutants, 2 caught" in the description is a local result only, since the gate exited 4 (baseline failed) and tested nothing. Worth a line in the body so a reader does not take the red checks as this change's. -
tests/fixtures/record_lowering.jsonis pure re-sealing churn. Everyeq,oreandmatchterm in that file is byte-for-byte unchanged; only the eightcciphertexts moved, becauseSTACK_ENCRYPT_UPDATE_FIXTURES=1re-seals them with fresh nonces whenever it runs. Nothing in that fixture depends on this PR (its only ordered field is auint32), and the test only asserts the terms and that the records open. Suggest restoring the file from the base so the diff does not read as a stored-format change in a PR about floats. (Thequery_operators.jsonregeneration is real: it adds thescorefield and its two queries.) -
Closes #1236closes an issue whose title still promises dates, timestamps and decimals. The issue's updated proposal defers those to #1144 and #1130, so closing on the proposal is defensible, but the title ("cannot order floats, dates, timestamps or decimals") will read as done once it is closed. Retitle the issue, or split the deferred kinds out before this merges. -
Minor test gap: no subnormal
f32on the Rust side.an_f32_is_ordered_as_the_f64_it_widens_to(tests/term_bytes.rs:344) coversMIN_POSITIVE, the smallest normal. The widening in vitaminc-normalize 0.6.0 is a hand-rolled integer path with a separate subnormal branch (widen_f32,top + 874exponent, mantissa realigned), and the only test that exercises it from this repo is Go's adjacency check withmath.SmallestNonzeroFloat32, which proves order among f32 terms but not that a subnormal f32 and its f64 are one term. vitaminc's ownwidening_matches_hardware_at_every_exponent_and_subnormal_bitcovers it upstream, so this is low risk; addingf32::from_bits(1)to that list would pin it from this side for free.
Verified as sound
- Stack Encrypt does not encode the float.
sem/mod.rsorderable!(f32 => F64, f64 => F64)callscolumn.encrypt(*self)with the bare scalar, and the dynamic path'sore_of/ope_ofdispatchFloat::F32(v)/Float::F64(v)to the sameore/opefunctions. No trace ofNarrowFloat,float_order_keyorOrderSourceremains insrc,testsorlanguages/golang. - The semantics live in the crate, and match ADR-0002. vitaminc-ore 0.6.0
order()routes(F64, Physical::Float(f))tovitaminc_normalize::float, which widens anf32exactly, thencanonical_f64folds-0.0to+0.0and every NaN (either sign, any payload, including the signalling0x7ff0_0000_0000_0001the test uses) to0x7ff8_0000_0000_0000, andorderable_f64's sign-flip encoding puts that above+Infinity. That is ADR-0002's baseline table row forFloat32/Float64and Postgresfloat8('NaN' = 'NaN','-0' = '0','NaN' > 'Infinity'). Write and query go through the sameOrderable::encrypt_in, so the canonicalisation cannot differ between them.Lt(NaN)matching every non-NaN andBetween(NaN, +Inf)matching nothing are both what Postgres answers. - The tests pin the right things.
-0.0 == 0.0and the NaN variants as one term;-Inf < MIN < -1.5 < -MIN_POSITIVE < 0 < MIN_POSITIVE < 1.5 < MAX < +Inf < NaNfor ORE (comparator) and OPE (bytes); exact ORE/OPE term bytes for1.5; the f32-equals-f64 check on the typed path, the dynamic path (dynamic/term.rsand theratiofloat32 field inquery.rs) and Go's property tests; andScore.Lt/Score.Betweenin Go checked byte-for-byte against the Rustquery_operators.json. - Key and domain separation. The ORE and OPE keys are
PRF(PAE([ORE_KEY_DOMAIN | OPE_KEY_DOMAIN, context])), unchanged here; vitaminc-ore's column salt binds the scheme name, the target domain (vitaminc/ore/value/float64/v1) and the context, so a float term and an integer term under the same field context are under different salts, and ORE and OPE never share a key. - Dependency. This PR changes neither
Cargo.tomlnorCargo.lock;cllw-ore = "=0.5.2-rc.1"with thevitamincfeature andvitaminc-ore 0.6.0are #1235's, and--lockedis unaffected. - Go parity. stashgen emits
Lt/Lte/Gt/Gte/Betweenonfloat32/float64withoreorope, the generated signatures take the field's own width,enginetest'sansweredBymirrorsordered(), the notice is narrowed to bytes, and CI'sgo generatedrift check passed at this head. - Changelogs. Rust crate and Go module only, both CHANGELOGs updated; no npm package is touched, so the changeset-bot warning is expected.
- The earlier CHANGELOG contradiction (f32 said not to compile) raised on
2dbdf38is fixed at this head, as is the Go README table.
Checks run locally (no credentials)
cargo nextest run -p stack-encrypt --all-features --no-fail-fast: 681 passed, 1 failed (stored_rows, see finding 2), 1 skipped.cargo clippy -p stack-encrypt --all-targets --all-features -- -D warnings: clean.cargo fmt --check: clean.go build ./...,go vet ./encrypt/... ./stashgen/...: clean.go test ./encrypt/... ./stashgen/...: stashgen and the fixture test pass; threeencrypttests fail only because the EQL guest is not built locally, andTestTermOrderIsPlaintextOrderskips without credentials (CI's live Go job passed at this head).- Read the vitaminc-ore 0.6.0, vitaminc-normalize 0.6.0 and cllw-ore 0.5.2-rc.1 sources from the registry for the float encoding, the widening and the salt.
Not run: the live PostgreSQL test and anything needing CipherStash credentials.
| }, | ||
| "text": { | ||
| "c": "0100000000000000000000000000000000cf1a45d1517a08c45d6ae1c989ae99de2000da620d1dbdbba9372d1439b6d44f7cc4169e9a255387aa180d6a95fb17926d7002feb70f92eddc6cf5ca7057e55c63c0ad86f563b5fc6db163691f66a21e23a7b7e089", | ||
| "c": "0100000000000000000000000000000000cf1a45d1517a08c45d6ae1c989ae99de2000da620d1dbdbba9372d1439b6d44f7cc4169e9a255387aa180d6a95fb17926d700294bddaed0bd2c45206be9407edb15f4c046eff417d18a00d6c033139e01770404e72", |
There was a problem hiding this comment.
Every eq, ore and match term in this file is unchanged; only the c ciphertexts moved, because the regeneration re-seals them with fresh nonces. Nothing here depends on this PR (its only ordered field is the uint32 age), so this hunk is noise that makes the diff look like a stored-format change. Suggest restoring the file from the base branch.
| async fn an_f32_is_ordered_as_the_f64_it_widens_to() { | ||
| let cipher = cipher().await; | ||
| let keyset = cipher.default_keyset(); | ||
| for value in [1.5f32, -0.0, f32::NAN, f32::MIN_POSITIVE, f32::INFINITY] { |
There was a problem hiding this comment.
f32::MIN_POSITIVE is the smallest normal f32. vitaminc-normalize's widen_f32 has a separate integer-only subnormal branch, and nothing on the Rust side here checks a subnormal f32 and its f64 are one term (Go's adjacency test with SmallestNonzeroFloat32 checks order among f32 terms, not equality with the f64's). Adding f32::from_bits(1) to this list pins it for free.
| operators are answered over `f32` and `f64` (`op::Ordered`) and over a | ||
| data plan's `float32` and `float64` fields, which they were not. An `f32` | ||
| is ordered as the `f64` it widens to: vitaminc widens it exactly, so an | ||
| `f32` and its `f64` are one term. Float order terms change bytes with |
There was a problem hiding this comment.
This sentence is right that the byte change is the one "(above)", from #1235. The PR title and the ! on this commit say otherwise: nothing in this PR's own diff changes a stored byte (op::Ordered and ordered() only widen which kinds answer the orderings). Worth aligning the title with the commit's.
tobyhede
left a comment
tobyhede
left a comment
There was a problem hiding this comment.
packages/stack-encrypt/src/dynamic/kind.rs:321minorexact_f64refuses every NaN, so now that the orderings reachread,Cipher.QueryTerm(ctx, plan, "f32", "lt", math.NaN())on afloat32field fails withErrEncodingwhile the same query on afloat64field succeeds. The stated reason (kind.rs:154-156, the test comment atkind.rs:863) is false now that every NaN is one value above +Inf. In the float arms, read any NaN asf32::NAN/f64::NAN, and rewriteread_refuses_a_nan_in_both_float_directions.languages/golang/stashgen/enginetest/enginetest.go:185minor TheansweredBycomment still says the orderings are answered "over an integer or text", but the guard at:202now admitsKindFloat. Change it to "over an integer, a float or text".
ec45e28 to
4392b57
Compare
fa4652a to
c1bdf66
Compare
4392b57 to
ba29e03
Compare
2d97f21 to
ea26f9d
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Re-review of the update to ea26f9d.
Against 2d97f21, the PR's single commit changed only in two Go tests:
order_test.go: the float64 and float32 order properties now draw values from uniformly random bit patterns (math.Float64frombits(r.Uint64()),Float32frombits(r.Uint32())). They previously usedtesting/quick's default generator, which scales byMaxFloat64and so draws almost only huge values.- Every exponent is now reachable, including subnormals, both zeros and NaN payloads. That covers the earlier "no subnormal f32 test" item probabilistically, alongside the fixed edge list in
floatValues(). floatOrderhandles the new inputs correctly: NaN sorts last and equal to NaN, andcmp.Comparetreats-0.0and0.0as equal. This matches the documented float8 order.
- Every exponent is now reachable, including subnormals, both zeros and NaN payloads. That covers the earlier "no subnormal f32 test" item probabilistically, alongside the fixed edge list in
query_test.go: theScore.Lt(-0.0)check now fails the test witht.Fatalfif building the-0.0query errors. Before, it went through amustTermhelper that turned the error intonil, which could compare equal to another failed term.mustTermis removed.
Both are improvements, and there are no new findings. CI is green at this head.
ba29e03 to
7a8c8ae
Compare
ea26f9d to
c634933
Compare
7a8c8ae to
e086fcf
Compare
c634933 to
72dc64c
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Incremental re-review of the rebase to 72dc64c (onto feat/stack-encrypt-text-ordering at e086fcf, after next gained #1262, #1264 and #1267).
Range-diff against ea26f9d. The PR's one commit changed in two hunks, both rebase adaptations to the base's reworded order-term entries; nothing substantive:
languages/golang/CHANGELOG.md: the float entry no longer repeats "every order term changes bytes from the alpha's; none was deployed", which #1235's own entry now carries at this head (lines 40–43).packages/stack-encrypt/tests/term_bytes.rsmodule doc: "None was deployed" became "Terms written by 0.1.0 and 0.2.0 do not compare with these (the CHANGELOG says so)", matching the stack-encrypt CHANGELOG at this head.
No FfiValue use is in this diff and no smallint surface is touched. No fixture or frozen file moved: query_operators.json and record_lowering.json are byte-identical to ea26f9d, and stored_rows_v1.json is not in the diff. Everything else in the commit (engine, stashgen, enginetest, tests, UI snapshots, docs) is identical to the reviewed head.
Earlier findings at this head (the rebase changed none of them):
- Still open, LOW (thread):
record_lowering.jsonstill carries the re-sealedcchurn (16 lines in this commit, every term unchanged). - Still open, nit (thread): the PR title still says "derive every order term over its ADR-0002 order key"; the commit is the float one.
- Still open, minor (review 5565461331):
exact_f64still refuses a NaN across float widths (read_refuses_a_nan_in_both_float_directions,kind.rs:878), so a dynamic query ofmath.NaN()against afloat32field is refused while the same against afloat64field is accepted; the rationale atkind.rs:154is unchanged. Andenginetest.go'sansweredBycomment still says "over an integer or text" while the guard admitsKindFloat. - Still open, optional (thread): no subnormal
f32in theterm_bytes.rs:345list; the Go property test now draws every bit pattern, so this is covered probabilistically. - Fixed since:
Closes #1236is nowRefs #1236; theorder_test.gogenerators and themustTermremoval were confirmed in review 5566619279.
Checks (no credentials): CI is green at this head (every job pass; review and the fuzz campaign skipped by design). mise run types:check at this head: no drift. At the top of the stack (#1280's 2189d5a, which contains this commit): mise x --env test -- cargo nextest run -p stack-encrypt --all-features 695 passed, 1 skipped, including stored_rows::every_frozen_row_opens_and_seals_as_it_did; go test ./stashgen/... ./cmd/stashgen/... ok; go vet clean.
72dc64c to
6fc0c6a
Compare
e086fcf to
b98bad2
Compare
b98bad2 to
21659d1
Compare
6fc0c6a to
2375eec
Compare
|
Rebased onto #1235's new head (6fc0c6a → 2375eec). Conflicts with the |
2375eec to
b4e9b91
Compare
21659d1 to
7840c3a
Compare
|
Rebased onto #1235's new head 7840c3a (2375eec → b4e9b91). One conflict, in |
An `f32` or `f64` order term is cllw-ore's vitaminc encryption of the value at the `F64` target, whose vitaminc-ore encoding folds `-0.0` into `0.0` and every NaN into one value above `+Infinity`: Postgres's `float8` order. So the ordering operators are now answered over floats, on the typed path (`op::Ordered` for `f32` and `f64`) and over a data plan's `float32` and `float64` fields, and stashgen writes `Lt`, `Lte`, `Gt`, `Gte` and `Between` on a float field with `ore` or `ope`. An `f32` is ordered as the `f64` it widens to: its `Orderable` target is `F64` and vitaminc widens it exactly, so an `f32` and its `f64` are one term and `f32` can join `op::Ordered`. A float `Between` is not refused when inverted (a range from NaN, which sorts last, matches nothing), as SQL's `BETWEEN` is not. The Go query profile gains a sealed `Score float64` with ORE, matching the Rust query fixture, and the live PostgreSQL test stores the score and its ORE term so the rows it reads back open. Float pins, the float ordering baseline, the integer-width lengths and the `score` field in the cross-path fixture are added; the fixtures and UI snapshots are re-recorded. Closes #1236.
7840c3a to
3004048
Compare
b4e9b91 to
36271cb
Compare
|
Rebased onto |
3 participants
Summary
Stack Encrypt (our Rust encryption engine, which the Go SDK runs as a WebAssembly guest) can now answer
<,>,BETWEENandORDER BYon floats. An order term is a small encrypted value stored next to the ciphertext so the database can compare rows without decrypting them. There are two schemes: ORE (order-revealing encryption) and OPE (order-preserving encryption, compared as plain bytes).Floats now order the way PostgreSQL's
float8does:-0.0ties0.0, and every NaN is one value that sorts after+Infinity.This builds on #1235, which made every order term cllw-ore's own encryption through
vitaminc-ore. Stack Encrypt passes a float through at theF64target, and cllw-ore encodes it. Stacked on #1235: review that PR first.Changes
Engine (
packages/stack-encrypt)op::Orderedcoversf32andf64.f32is ordered as thef64it widens to. ItsOrderabletarget isF64, and vitaminc widens it exactly, so anf32and itsf64are one term. The first draft refused a typedf32term withTermError::NarrowFloat. That variant is gone: the crate's widening settles the question.Betweenon floats. As on every other type since feat(stack-encrypt): order text over ADR-0002's folded order key #1235, an inverted range is not refused: a range from NaN, which sorts last, matches nothing, as SQL'sBETWEENwould.float32andfloat64fields.Go (
languages/golang)Lt,Lte,Gt,GteandBetweenon afloat32orfloat64field withoreorope. The "no generated method queries this index" notice now appears only for order indexes on bytes. The fake engine (enginetest) follows.QueryProfilegainsScore float64with ORE, matching the Rust query fixture.ProbegainsF64andF32fields.doc.go, the stashgen README and the CHANGELOG are updated.Verification
mise x --env test -- cargo nextest run --workspace --all-features: 1188 passed. That includes new tests:-0.0ties0.0, NaN payloads and signs are one term, and the order runs-Inf < MIN < -1.5 < … < MAX < +Inf < NaN, for both ORE and OPE;f32gives the same ORE and OPE terms as thef64it widens to, including-0.0, NaN and infinity;u8toi128has the expected term length and orders min < mid < max;Betweenon floats: NaN-last and-0.0tie cases;-0.0tie.mise run test:docpasses (stack-encrypt: 40 doctests).cargo clippy --workspace --all-targets --all-features -- -D warnings,cargo fmtandmise run doc: clean.cargo mutants --in-diffon stack-encrypt against feat(stack-encrypt): order text over ADR-0002's folded order key #1235: 2 mutants, 2 caught.tests/fixtures/query_operators.json) has afloat64fieldscore, queried withltandbetween. The data path derives the typed chain's bytes for both. Both fixtures are re-recorded.eql-bindingswith all features (197 passed) andpackages/eql/tests/encryption(37 passed, 3 ignored; those need a database).mise run wasm:guest:testpasses (68 and 71 tests).mise run go:testpasses, including the float ordering property tests and the generatedScore.Lt/Score.Betweenchecked byte for byte against the Rust fixture.mise run go:lintreports 0 issues, andgo generate ./...leaves no diff.DATABASE_URL).Related
IntegerOrdOpe,RealOrd, …) can now be produced. Producing them needs the EQL SQL checked against these terms first, then the types added toENCRYPTION_DOMAINS, as feat(eql-bindings): produce IntegerEq, BigintEq, RealEq and DoubleEq, and name each type's real blocker #1241 did for the equality types.int16is widened toInt32, and there's no way to encrypt a date, timestamp, decimal or 128-bit integer #1144: dates, timestamps and decimals still have no kind in the engine. Once they do, each gets itsvitaminc-oretarget throughOrderable.Review notes
op::Orderedgaining the floats and the data path answering float fields; feat(stack-encrypt): order text over ADR-0002's folded order key #1235 did the heavy lifting.f32changed. Allowingf32inOrderedis the one behaviour that differs from the first draft.https://claude.ai/code/session_01BZhkvKLpNtKuzj3HCfMXoL