Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 4 files. (6 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the main change: Go tests validate EQL v4 types against Rust and PostgreSQL.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 4 files. (6 skipped: 6 unsupported.)
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
f7f2724 to
6b9b42c
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Review of this PR's own diff (feat/eql-v4-text-match...test/golang-eql-v4). Summary: the Go-to-Rust fixture check is sound and passes locally, but no committed test touches PostgreSQL, and CI has not yet run any of the new tests.
What I ran locally on 6b9b42c: mise run wasm:guest:build:eql:deterministic and wasm:guest:build:eql. Then go test ./encrypt -run 'TestV4|TestTextEq|EQL', where both new tests pass. cargo test -p eql-encryption-tests --test v4_types passes 7, with 1 ignored (the Postgres test). Finally go generate ./encrypt/internal/testusers/, which left the tree unchanged, so profile_stash.go is current.
Findings
1. Major: the title says "against PostgreSQL", but no committed test checks Go output against PostgreSQL
languages/golang/encrypt/eql_v4_test.go (whole file), PR title
The two new Go tests are hermetic. One checks top-level JSON keys and a seal/open round trip. The other compares query operands byte for byte with v4_queries.json. The PostgreSQL check in the description was done by hand and is not in the repo. You can make a chain argument: Go operands equal Rust operands, and the ignored Rust test postgres_stores_and_queries_every_producible_v4_type (packages/eql/tests/encryption/tests/v4_types.rs:327) runs in CI through test:encryption:postgres. But that chain covers query operands only. That Rust test uses its own label (users/age) and its own values. Stored values sealed by the generated Go code never reach an eql_v4_* domain check or an operator in any committed test. If a Go-sealed value had the right top-level keys and a malformed body (a wrong i, say, or a bf the domain rejects), every test here would still pass.
Why it matters: the commit message goes into history saying PostgreSQL proved this. A later reader will assume a regression gate exists, and there isn't one.
Suggested fix (either one):
- Narrow the title and commit message to the Go/Rust parity this PR actually proves, and keep the PostgreSQL step as a manual note.
- Or reverse the fixture direction, as stack-encrypt's
record_lowering.jsonalready does. The deterministic guest seals reproducibly undertestSeed, so a Go test can writev4_rows.json(Go-sealedProfilerows) behind an update flag and assert committed == produced. The Rust Postgres test, which already has a driver and already runs in CI, then inserts those rows intoeql_v4_*columns, queries them with thev4_queries.jsonoperands (=,@@), and decrypts withDeterministicSourceunder the same seed. That adds no PostgreSQL dependency to the Go module, which was the stated reason for leaving the check out.
2. Major: CI has not run any of this PR's new tests
.github/workflows/tests-golang.yml (wasi-check job), .github/workflows/test-eql.yml:606-612
- Go: in "WASI check, guests and Go (Linux)", the "stack-encrypt guest lint and tests" step fails (
targets::tests::the_eql_build_lists_the_catalog_and_resolves_text_eqandnative_ops::targets_lists_what_the_build_holds). Everything after it is skipped, including the "Go binding" step that runsgo:test.Go binding (macos/windows)is skipped because itneeds: wasi-check. Soeql_v4_test.gohas run on no platform, and neither has thego generatedrift gate. - Rust:
test:encryptionrunscargo test -p eql-bindingsbeforecargo test -p eql-encryption-testsunderset -e. The first command fails (a_producible_type_without_a_query_twin_answers_no_query,refusals_name_the_type_and_its_reason), sothe_v4_query_fixture_is_what_the_dispatch_derivesnever ran. Neither did thetest:encryption:postgresstep that follows it.
Those failures are the stale producibility assertions from lower in the stack (#1164 shows the same failures), and this PR doesn't touch guest/src/targets.rs, guest/tests/native_ops.rs or eql-bindings/src/encryption/targets.rs. The consequence for this PR is that "All Go packages pass" in the description has only been shown locally. This PR should not merge on the strength of CI until the stack below is green and these jobs actually reach the new tests.
3. Minor: the Go workflow does not trigger on a change to the fixture it reads
.github/workflows/tests-golang.yml:28-50 and :52-72
TestV4QueriesMatchTheRustFixture reads packages/eql/tests/encryption/fixtures/v4_queries.json (eql_v4_test.go:88). Neither paths: list includes it, and neither includes packages/eql/Cargo.lock. A PR that regenerates only the fixture skips the Go job, as does a PR that bumps a crate in EQL's lockfile that the Rust side derives under. Examples: a vitaminc bump in the EQL workspace, or a hand edit. The Go/Rust disagreement then surfaces on the next unrelated Go PR. text_eq_query.json has the same gap today, but this PR adds a second file that depends on it.
Fix: add packages/eql/tests/encryption/fixtures/** to both pull_request and push paths, before the !**.md excludes.
4. Minor: on the Go side, only one stored value is compared with its query, and the column label of stored values is never checked
languages/golang/encrypt/eql_v4_test.go:52-68, :138-154
- The stored-vs-query check covers
Age'shmonly.Balance'shmisn't compared, and forTextMatchnothing checks that the query'sbfpositions are a subset of the stored value'sbf. That subset relation is what@@depends on. Rust checks it (text_match_filters_contain_their_substrings), but for values the Go-generated code produced, nothing does. keysOfchecksvand the set of key names but neveri. For the stored values inTestV4FieldsSealToTheirEQLTypesAndOpenBack, a wrong table or column label wouldn't be caught: decryption uses the same declaration on both sides, so the round trip still succeeds. (The query operands do pinithrough the fixture.)
Fix: assert i == {"t":"profiles","c":<column>} for every stored field. Compare hm for Balance too. For Name, assert that the query's bf is a subset of the stored bf, plus a non-match such as "zzz".
IntegerEq / BigintEq being held back: what this PR would need to change
Once those two types are withdrawn (made unproducible), stashgen will refuse encrypt_into=IntegerEq/BigintEq, the Rust dispatch will return Unproducible, and these places break:
languages/golang/encrypt/internal/testusers/profiles.go:12(Age),:14(Balance): drop the fields or move them to a producible type. Otherwisego generatefails and so does the drift gate.languages/golang/encrypt/internal/testusers/profile_stash.go: regenerate. The references are at:22,:24,:58,:60,:90,:92, and theProfileAgeField/ProfileBalanceFieldmethods at:184-214.languages/golang/encrypt/eql_v4_test.go:21-22(row values),:57,:59(expected keys),:94-101(fixture struct),:116-129(Age/Balance query comparisons),:138-154. The last range is the only stored-vs-query check in the file, and it usesAge.packages/eql/tests/encryption/fixtures/v4_queries.json:3-10(age,balanceentries).packages/eql/tests/encryption/tests/v4_types.rs:564-568:targets::query("IntegerEq"|"BigintEq", …).unwrap()would panic. The doc comment at:544also namesencrypt_into=IntegerEqas its example.languages/golang/encrypt/README.md:288,:290: theIntegerEqandBigintEqtable rows.
Without them, do the tests still prove the claim? Mostly, with one gap:
- The seal/open round trip still covers
Integer,Boolean,Real,Double,TextandTextMatch. Each value still has to reach a plan that accepts its value kind. - The cross-language fixture drops to
TextMatchalone.TextEqkeeps its own fixture and stored-vs-query check ineql_test.go:151-190. - The stored-vs-query check at
eql_v4_test.go:138-154has nothing left to check. So for the one query type this PR still adds, no test would show that a stored value matches its query. Thebfsubset check suggested in finding 4 should replace it rather than be optional. - Finding 1 is unchanged: PostgreSQL still sees no Go-sealed value.
This PR doesn't fix the stale producibility assertions from #1163 (guest/src/targets.rs, guest/tests/native_ops.rs), and doesn't touch them.
3aaeddd to
8bf6a10
Compare
|
Rebased onto the updated #1164. 8bf6a10 answers the review above, with the withdrawal applied. 1. PostgreSQL. I took the reverse-fixture route. Go's side:
Rust's side:
So the title's "against PostgreSQL" is now true in CI, not just by hand. 2. CI never reached these tests. The stale producibility assertions are fixed in #1163, so these jobs can reach the new tests now. I have not seen a CI run on the new heads yet. 3. Paths. 4. Go-side checks.
Withdrawal.
Verified locally:
|
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: automated review by Claude Code (/code-review medium plus cross-check of findings) at head 8bf6a10. Static read of the full diff (10 files). I did not build or run the Go, Rust or PostgreSQL tests.
Critical or blocking issues: none found.
Findings: no correctness bugs found. Checked:
- The fixture paths resolve (
languages/golang/encrypt→ repo root →packages/eql/tests/encryption/fixtures). v4_rows.jsondrift check stripsc(random AEAD nonce) and compares everything else, then opens the committedcvalues. That is sound.- Rust side:
stack-kms(test-support),postgres,serde_jsonare already dev-deps. Thego_rowsmodule only uses names already imported inv4_types.rs. The PG test is#[ignore]d behindEQL_TEST_DATABASE_URL. - The
tests-golang.ymlpath filters now cover the fixtures andpackages/eql/Cargo.lock.biome.jsonexcludes the generatedv4_rows.json.
Non-blocking notes:
- The PR description says the PostgreSQL check is "checked by hand, not committed", but the diff adds
postgres_stores_and_queries_rows_the_go_sdk_sealed(ignored by default) and a second fixture,v4_rows.json. Worth updating the description so it matches. - The
@@assertions inTestV4QueriesMatchTheRustFixture("bob"→ false) rely on the Bloom filter not producing a false positive. That is deterministic under the fixed seed, so it is stable, but a seed or parameter change could flip it with no code bug. - No changeset or skills change appears needed: the diff is tests, fixtures, READMEs and a workflow filter, with no published package surface change. Please confirm the README claim that
TextMatchrejects text shorter than 3 characters matches the engine, since I did not run it.
Merge state is blocked (base is the stacked feat/eql-v4-text-match); CI results were not assessed.
Generated by Claude Code
tobyhede
left a comment
tobyhede
left a comment
There was a problem hiding this comment.
Review attestation (high effort, 8 inline angles, dedup only, no verify).
Files reviewed: .github/workflows/tests-golang.yml, biome.json, languages/golang/{cmd/stashgen/README.md, encrypt/README.md, encrypt/eql_v4_test.go, encrypt/internal/testusers/profiles.go, encrypt/internal/testusers/profile_stash.go}, packages/eql/tests/encryption/{tests/v4_types.rs, fixtures/v4_queries.json, fixtures/v4_rows.json}.
Models involved: Claude Sonnet 5.5 (claude-sonnet-5-5).
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: 8bf6a108c8
ℹ️ 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".
8bf6a10 to
d280ccf
Compare
GitHub Actions
Failure
d280ccf to
49005c7
Compare
49005c7 to
8ca5cc1
Compare
f0c0425 to
70748c3
Compare
…eSQL stashgen needed no change: it maps int32, int64, bool, float32, float64 and string to their kinds and accepts any type the engine's se_targets table marks producible, so every type the previous commits made producible already generates typed Encrypt and Query methods. A Profile fixture stores one field in each new type. The hermetic test seals it through generated code over the deterministic EQL guest, checks every value is a v4 envelope with the type's keys, and opens it back. A cross-language fixture, v4_queries.json, holds the IntegerEq, BigintEq and TextMatch query operands the Rust dispatch derives under the shared test index key; the Go fields derive them byte for byte. Checked by hand against PostgreSQL: rows sealed by this Go code, loaded into an eql_v4 install, answer =, = and @@ with exactly the expected ids. The Go module has no PostgreSQL driver, and adding one for a test would change every consumer's dependency graph, so that check is not committed. The encrypt and stashgen READMEs name eql_v4 and list the producible types with the Go field type each takes. Refs #1144
… withdrawn integer equality types Nothing in the repository put a value the generated Go code sealed into an eql_v4 domain or operator; the PostgreSQL check was manual. The Go test now writes v4_rows.json, the Profile rows the generated code seals over the deterministic EQL guest. On every later run it checks that file against today's output, envelopes and match terms byte for byte and ciphertexts by opening them (their AEAD nonce is random). The EQL suite opens those rows with stack-kms's DeterministicSource over the same seed, and its PostgreSQL test, which test:encryption:postgres runs in CI, inserts them into eql_v4_* columns and queries name @@ with the Go query operand from v4_queries.json. With IntegerEq and BigintEq held back, Profile drops Age and stores Balance as Bigint, and v4_queries.json carries TextMatch only. The one stored-versus-query check had used Age, so it is replaced by a TextMatch check that a contained needle's filter bits are a subset of the stored filter and an absent one's are not. Every stored field's i is checked against its table and column. The Go workflow now runs when the fixtures or EQL's lockfile change, and biome leaves the generated rows file alone.
…re stable - TestV4RowsMatchTheCommittedFixture writes the rows it seals to EQL_V4_ROWS_OUT, and the Rust suite opens the file EQL_V4_ROWS names. The Go job runs both, so the ciphertexts the Go SDK seals in that run are opened by native Rust, not only the committed ones. - EQL_UPDATE_FIXTURES rewrites v4_rows.json only when something besides the ciphertexts moved, so a regeneration that changes nothing leaves no diff of fresh nonces. - The Go and Rust TextMatch checks say that their negative cases hold for one seed and configuration, the PostgreSQL expectation folds case as `@@` does, and a needle shorter than three characters is asserted to be refused. - tests-golang.yml runs on any change under packages/eql/tests/encryption. Claude-Session: https://claude.ai/code/session_01VYKAWbd361xDLmqBdLoUcB
70748c3 to
25606d2
Compare
This branch was successfully deployed
3 participants
Summary
This proves the new EQL v4 types work end to end from the Go SDK.
stashgenneeded no change: it maps Go field types to their value kinds and accepts any type the engine's target table marks producible, so the previous PRs already give Go typedEncryptandQuerymethods for every new type.Changes
testusers.Profile: a fixture struct with one field in each new type, and its generated code.eql_v4_test.go: seals a batch through generated code over the deterministic EQL guest, checks every value is a v4 envelope with its type's keys, and opens it back.tests/encryption/fixtures/v4_queries.json: theIntegerEq,BigintEqandTextMatchquery operands the Rust dispatch derives under the shared test index key. The Go fields derive them byte for byte.encryptandstashgenREADMEs nameeql_v4and list each producible type with the Go field type it takes.Verification
mise run go:test,go:lint).database/sqlwith pgx into aneql_v4install, answer=(ids 1,3) and@@(ids 1,2), scan back into the encrypted type, and decrypt to the original row. The Go module has no PostgreSQL driver, and adding one for a test would change every consumer's dependency graph, so that check lives outside the repo.Related
Closes #1133. #1163 makes
Textproducible, and this PR covers the rest of #1133:Profile.Noteis aTextfield written by the generated Go code, stored in aneql_v4_textcolumn in CI's PostgreSQL test (v4_rows.json), and read back and decrypted.TextEqwas proven end to end before stashgen refuses the storage-only Text type — it is held back until TextEq is proven end to end #1133's hold-back lifted.text_eq.rs's PostgreSQL test stores, queries and decrypts it oneql_v4, and the Go SDK derivestext_eq_query.json's query operand byte for byte.This does not address #1144 (Go integer widths, dates, decimals), which waits for vitaminc 0.6. Stacked on #1164.
https://claude.ai/code/session_01CA4pFfaN4DPNSTLnJVNxsu
Summary by CodeRabbit