Repository navigation
Conversation
TestEQLTypesRoundTripAndQueryInPostgres runs <, <=, > and >= over four fixed values per type. A comparator that is only partly wrong (one that reads only a block ORE term's first blocks, mishandles a sign bit, or orders a decimal by its scale) still agrees with plaintext order on most pairs, so four values say little. The Go property tests compare terms in Go, and the SQLx property suite covers EQL v3, so nothing ran random Go SDK values through EQL v4's operators. TestEQLOrderMatchesPlaintextInPostgres stores about 360 values in a temporary table for every producible ordered EQL v4 type (29 today): 300 random draws, the edges of the type's order, and thirty duplicates. Each of 20 query values goes through each ordering operator and must find exactly the rows plaintext comparison selects, and ORDER BY the type's order term must return the rows in plaintext order, both ways. Plaintext order is the engine's: text by its fold, -0.0 equal to 0.0 and NaN above +Inf, timestamps to the microsecond, decimals by value. The seed is fixed and logged, EQL_ORDER_SEED replaces it, and each type draws from its own generator so one subtest replays alone. A failure names the type, the operator, the seed and the value pair. The connection and EQL-installed check move into a helper both tests share, and go:test:postgres runs and requires the new test. Closes #1343 Claude-Session: https://claude.ai/code/session_01XQN1QpaWVDGjHEg39KEJF3
|
|
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 ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Mutation testing (cargo-mutants,
|
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Review of #1344 at medium effort.
Head: 1cdfb943a544133b83503a2b3bdd6563380941d8
Read: the whole diff (eql_order_postgres_test.go, eql_postgres_test.go, mise.toml, tests-golang.yml), eql/eql_gen.go (the 29 ordered producible types and their index names), order_test.go (floatOrder, floatValues, foldedAlphabet), and the order rules in packages/stack-encrypt/src/value.rs. The plaintext model matches those rules: floor truncation of timestamps, decimals by value, -0.0 == 0.0, NaN above +Inf.
Ran: mise run go:test:postgres against a PostgreSQL 17 container. Everything passed, and the order test took 46 s. I also ran the order test with a deliberately wrong plaintext model, to see what it catches. It caught timestamp truncation toward zero, -0.0 < 0.0 in the model, and 0 < 0.00 in the model. It missed a tie the engine splits (finding 1).
Not covered: the Rust side of the order terms, the cross-language fixtures, and other PostgreSQL versions.
Findings: 2 (1 Major, 1 Minor).
Dropped candidates:
- Risk of the 10-minute
go testtimeout: dropped. Measured 46 s locally, and the CI job took 1m58s in total. field.Termis derived once per operator rather than once per query value: speed only, not correctness.oreis listed ineqlOrderIndexwith no extractor: it fails loudly, which is the right behaviour. It is covered by finding 2's fix.- The decimal
cmpre-parses throughbig.Raton every call: speed only. eqlFloat32OrderduplicateseqlFloat64Order: style.%#08x/%#016xprint too few digits: wrong. Go does not count the0xprefix in the width (checked:0x00000001).- The fixed default seed means CI never sees new values: deliberate, so runs do not vary, and
EQL_ORDER_SEEDcovers exploration.
Generated by Claude Code
The edges hold the ties the order rules make: -0.0 and 0.0, 1.5 and 1.50, two instants in one microsecond. An engine that splits one of those ties orders the tied rows one way every time, and the ORDER BY check accepts tied rows in either order, so only a query on one of the tied values can show it. Four edges were picked at random as query values, and with the default seed four of the six float types passed with a plaintext model that tied two values the engine keeps apart. Every edge is now a query value, and all six fail on that model. Refs #1343. Claude-Session: https://claude.ai/code/session_01XQN1QpaWVDGjHEg39KEJF3
eqlOrderIndex kept its own list of order index names, and a type whose index was not on it was skipped, with only a zero-types guard behind it. The round-trip test fails on an index missing from eqlIndexOperators, so a new order index would be added there and this test would then pass without ever running the new types. An order index is now any index whose operators in eqlIndexOperators include lt, so a new one reaches runEQLOrder and fails at the missing extractor until it has one. Refs #1343. Claude-Session: https://claude.ai/code/session_01XQN1QpaWVDGjHEg39KEJF3
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 (Claude Code), head ccc7bdc
Result: no critical or blocking issues. No new findings.
Scope: the full diff (eql_order_postgres_test.go, eql_postgres_test.go, mise.toml, tests-golang.yml), checked against the PR head.
Candidates raised (7), all dropped after checking against head:
- ORDER BY cannot detect split ties. This is already raised in the earlier review, and fixed in
5532dc935by running every edge as a query value (thread resolved). Only the comment ateql_order_postgres_test.go:310, "so equal values come out next to each other", still overstates what the ORDER BY loop checks. That is wording only. - Extractor names
eql_v4.ord_term/ord_term_oreunverified. Dropped. The PR reports the run passing against the bundle built from the tree. go test10-minute timeout risk. Dropped. The test took 46–60 s locally, and theeql-postgresjob took about 2 minutes in CI.- "thirty" in a comment vs
eqlOrderRandom / 10. Dropped. It equals 30 today, which is a style point only. - Seed replay depends on draw order. Dropped. A seed replays on the same file, and the PR does not claim replay across edits.
foldedAlphabettype and fold assumptions. Refuted.foldedAlphabetis a[]rune(order_test.go:133), and its non-ASCII characters (ж,ω,中) are already lowercase, so ASCII lowercasing is the correct fold.- Timestamp full-range draw relies on unsigned wraparound. Dropped. The arithmetic is correct and the seeds listed in the PR description pass.
Not covered: the Rust side of the order terms, other PostgreSQL versions, and a local run of the suite.
Note: pr_feedback.py could not run here because GraphQL is blocked, so existing feedback was read through the REST/MCP tools instead.
Generated by Claude Code
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟢 merge as it is
Nothing must change before merging. This PR adds only tests, and the new property test covers every ordered EQL v4 type with random values, edges and ties. The three comments are extra edge values the test does not store today. All can wait for a follow-up.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 2 found, 2 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 1 posted |
Synthesis: claude-sonnet-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-sonnet-5-5 read every comment as a new reader would. No comment had a problem that stopped the reader acting, so none was rewritten.
Stack: 1 earlier and 0 later pull request(s) (#1229, *️⃣ #1344). *️⃣ marks this pull request.
Context loaded: the description, 1 linked issue(s) and 10 discussion entries.
| // the folds above. The edges add strings either side of block ORE's padding | ||
| // buckets (16, 32 and 64 bytes) that differ only in their last byte. Block | ||
| // ORE's cost grows with the padded length, about 0.5 ms a byte under the | ||
| // test build, so nothing here is near its 1023-byte limit. |
There was a problem hiding this comment.
Fix in a follow-up: block ORE text ordering has no test near its 1023-byte input limit.
Impact: The longest stored text is 65 bytes. A comparator bug in a later block, or at the last valid byte, can pass CI.
Evidence: The comment above eqlTextOrder says no value is near the 1023-byte limit. TextOrdOre and TextSearchOre use block ORE.
Fix: Add two ASCII values of exactly 1023 bytes that differ only in the last byte to the text edges:
max := strings.Repeat("m", 1022)
edges = append(edges, max+"a", max+"b")At about 0.5 ms per byte, the extra encryption cost is small. The existing query and ORDER BY checks then cover the final block.
Found by 1 model: codex
| edges = append(edges, float32(v)) | ||
| } | ||
| edges = append(edges, math.SmallestNonzeroFloat32, -math.SmallestNonzeroFloat32, math.MaxFloat32, -math.MaxFloat32, | ||
| -1, 1, math.Nextafter32(1, 0), math.Nextafter32(1, 2)) |
There was a problem hiding this comment.
Optional: the float32 edges do not include a NaN with a non-default payload or a signalling NaN.
Impact: The rule is that every NaN is one value, above +Inf. An engine that recognises only the canonical float32 NaN (0x7fc00000) would order other NaNs wrongly, and this test could pass.
Evidence: The float32 edges come from float32(v) over floatValues(). The conversion drops the low payload bits, so no edge has a signalling NaN.
Fix: Add NaNs as bit patterns, so the conversion cannot change them:
math.Float32frombits(0x7fc0_beef), math.Float32frombits(0x7f80_0001), math.Float32frombits(0xff80_0001)The test expects each of these to tie with every other NaN.
Found by 1 model: claude
| eqlDecimalText(eqlDecimalMaxMantissa, 28, false), eqlDecimalText(eqlDecimalMaxMantissa, 28, true), | ||
| eqlDecimalText(new(big.Int).Sub(eqlDecimalMaxMantissa, big.NewInt(1)), 0, false), | ||
| "0.0000000000000000000000000001", "0.0000000000000000000000000002", "-0.0000000000000000000000000001", | ||
| } { |
There was a problem hiding this comment.
Optional: no decimal edge ties a value at scale 0 with the same value at scale 28.
Impact: Decimals must order by value whatever their scale. The scale ties in the test are small (1.5/1.50/1.500, and at most 5 extra digits in the random draws). A scale bug that shows only at a large scale difference would pass.
Evidence: 1 at scale 28 has the mantissa 10^28, which is below 2^96 - 1, so it is valid. No edge or draw stores it.
Fix: Add these edges:
"1", "1.0000000000000000000000000000", "-1", "-1.0000000000000000000000000000",
"7", "7.0000000000000000000000000000",The test expects each pair to tie.
Found by 1 model: claude
Summary
EQL is the SQL library CipherStash installs into a customer's PostgreSQL. An order term is a value derived from a plaintext so that comparing two terms gives the same answer as comparing the plaintexts. EQL v4 compares them with SQL operators such as
<and>=, using one of three schemes: OPE, CLLW ORE or block ORE. Until now the Go SDK's PostgreSQL test checked order with four fixed values per type. A comparator that is only partly wrong would pass that.This adds a property test. For every ordered EQL v4 type, the Go SDK stores a few hundred random values plus edge values. Random queries through
<,<=,>and>=in PostgreSQL must return exactly the rows that plaintext comparison selects, andORDER BYmust return the rows in plaintext order. A broken comparator, term derivation or SQL operator now fails CI, with a seed that replays the failure.Changes
languages/golang/encrypt/eql_order_postgres_test.go): newTestEQLOrderMatchesPlaintextInPostgres.ope,oreorore_blockindex, read fromeql.Types(29 today), over int16, int32, int64, float32, float64, text, date, timestamp and decimal.1.5/1.50, and instants less than a microsecond apart.ORDER BY eql_v4.ord_term(v)oreql_v4.ord_term_ore(v)is checked ASC and DESC. These domains have no operator class, so a bareORDER BY vwould sort the jsonb.EQL_ORDER_SEEDreplaces it. Each type has its own generator, seeded from the seed and the type's name, so one subtest replays on its own. A failure names the type, operator, seed, and the stored and query values that disagree. It stops after five disagreements per type.eql_postgres_test.go): the connection and the "EQL v4 is installed" check move intoeqlPostgresConn, which both tests share. Without EQL v4, the new test fails rather than skips.go:test:postgres): the task runs the new test, and its--- PASS:check requires it. Theeql-postgresCI job runs this task, so the job needs no change. Its comment intests-golang.ymlnow describes the new check.Verification
mise run go:test:postgresagainst a throwaway PostgreSQL 17 container, after rebasing ontonext: all three tests pass. The task took 58 s in all, including the bundle build. The new test took 46.8 s. Block ORE text costs the most:TextOrdOreandTextSearchOretake about 11 s each, almost all of it encrypting in the WASM guest (about 10 ms per value at 16 bytes, plus about 0.5 ms per extra byte). Every other type takes 4.4 s or less.EQL_ORDER_SEED) also pass.<flipped to<=:BigintOrdOre: v < q missed row 205, but in plaintext -93 < -93 is true (seed 1343; replay with EQL_ORDER_SEED=1343).NumericOrdfailed the same way.op) replaced in SQL by row 2's:TextOrd: v < q returned row 1, but in plaintext "ß" < "Élan" is false (seed 1343; …)andNumericOrd: v < q returned row 1, but in plaintext 441323043518896555732877.30309 < 0.875 is false (…).EQL v4 is not installed in DATABASE_URL: …(FAIL, not SKIP).go vet ./...andgofmt -l .underlanguages/golang: clean.EQL_DIST_REQUIRED=1 pnpm run test:scripts: 81 files passed.Related
Closes #1343
Refs #1291 (merged into
next; this builds on its fixed-value test and itseql-postgresCI job)Review notes
foldedAlphabetplus A–Z), and the strings with accents or ß come from a fixed table with their folds. So the test needs no Unicode normalisation library.https://claude.ai/code/session_01XQN1QpaWVDGjHEg39KEJF3