Repository navigation
feat(python): value rules for every producible type - #1511
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Mutation testing (cargo-mutants,
|
|
Found 2 test failures on Blacksmith runners: Failures
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Run vitaminc's shared value corpus against the SDK's value conversion. crates/core/testdata/value-conformance.json is a copy at vitaminc 06568ba93, the commit Go uses, because the published crates do not include it. Each vector of a kind the SDK writes must give its exact bytes and read back as its value. Every malformed vector is refused. A new vector of an unknown kind fails the test until it is handled. Test every InvalidValueError code through encrypt and through query, with no request. A float NaN, infinity or -0.0 is accepted: the engine and PostgreSQL both keep them, and a live test round-trips them. Add a cargo-fuzz crate, crates/core/fuzz, over the conversion both ways. An accepted value must frame, read back as itself, and match the bytes ValueFrame::of gives the plain Rust type. Arbitrary bytes read as a passthrough must never panic. The root task fuzz:python-values runs it, and fuzz.yml replays its seeds on a pull request. A live test round-trips one field of each of the 49 producible types through PostgreSQL. JsonSearch is out of scope: the json kind has no Python type yet, and the decorator refuses it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Run fuzz:python-values and the campaign's cmin step from the root, naming the fuzz crate with --fuzz-dir. From a folder under languages/python, mise also loads that folder's mise.toml, and installs its Python and uv, which the fuzz jobs do not cache. Compare each field of the 49-type live round trip exactly: a Decimal by as_tuple(), so its scale counts, and a float by its bytes, so the sign of zero counts. Python's == treats 1.50 as 1.5 and -0.0 as 0.0. Check the normalized vectors against the corpus's reencode_value, not against a second read of the same bytes. The canonical value must write back as the reencode bytes, and a vector whose two byte strings are equal fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fuzz.yml's pull_request filter missed four paths that a fuzz target compiles: stack-profile, stack-encrypt-derive, eql-bindings, and the Go guest's targets.rs, which the Python core crate compiles by #[path]. A change to any of them could break a harness with no job to notice. The stack-* fuzz crates already compiled the first two. Add the four paths. The new fuzz-workflow-paths.test.mjs reads each fuzz crate's Cargo.lock and fails when the filter misses a crate a target compiles, or a file one compiles by #[path]. Move the frame writer into values::Typed, and the scalar-to-value conversion into Scalar::value(). They replace three Written copies, Carried in decrypt.rs, Typed in query.rs, and two value_of copies. Correct the fuzz comment: values::read range-checks a timestamp but not a date or a decimal, so those are the cases read accepts and write refuses. The testdata README now names the two counts the corpus test checks, which a corpus update may change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lk (#1392) fuzz-workflow-paths.test.mjs scanned #[path] with its own regex over raw source. It read only each crate's src/, and counted a #[path] quoted in a comment. It now uses compiledCrateFiles from lib/rust-compiled-files.mjs, which strips comments and follows mod, The lockfile walk moves to lib/cargo-lock.mjs, and python-ci.test.mjs uses it too. The corpus test now reads a typed null's reencode bytes, which must be a plain null. The fuzz.yml header no longer counts six targets, and docs/fuzzing.md lists mise.test.toml in the path filter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
10bdd53 to
ee7d870
Compare
Biome failed on testdata/value-conformance.json, a verbatim copy of vitaminc's value corpus. The testdata README records the copy's git blob hash. A reformat would change that hash. Then the copy would no longer match its source. biome.json now leaves the file out. The README says why, because biome.json cannot hold a comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed 96ca56a · posted from Claude Code · claude-sonnet-5-5 · effort medium
Sources: code-review · claude-sonnet-5-5
Skills: code-review, review-cross-check, review-post
Findings (2): 2 minor — 1 inline, 1 below.
.github/dependabot.ymlminor The new detached fuzz cratelanguages/python/crates/core/fuzzhas its ownCargo.lockbut is not in the cargodirectorieslist, unlike the three stack-* fuzz crates. Its lockfile never gets Dependabot updates. Add/languages/python/crates/core/fuzz;AGENTS.md("None of them has a Dependabot entry") is also already inaccurate.
Source: code-review · claude-sonnet-5-5
Generated by Claude Code
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
Change before merging: fix the fuzz seed corpus, which is in the wrong format, so the per-PR replay does not read any valid frame. The test-strength points and the lock note can wait for follow-ups.
Other findings not posted as comments
languages/python/tests/test_values.py:10— the module docstring claims a live round trip for every edge value, but only one value per kind goes through PostgreSQL. Fix the docstring or runACCEPTEDthrough a live test.languages/python/tests/test_encrypt_live.py:351— the offline check ofPRODUCIBLEagainstKIND_VALUESruns only in thelivejob, because of the module-level skip. Move it to an offline module.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 3 found, 3 posted |
| claude | claude-opus-5-5 | rust | 1 found, 1 posted |
| claude | claude-opus-5-5 | python | 3 found, 1 posted |
| claude | claude-opus-5-5 | infracode | 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. 2 comment(s) had a problem that stopped the reader acting; it rewrote 2.
Stack: position 11 of 15 (#1405, #1406, #1414, #1415, #1433, #1437, #1453, #1499, #1509, #1510, *️⃣ #1511, #1513, #1516, #1517, #1582). *️⃣ marks this pull request.
Context loaded: the description, 2 linked issue(s) and 6 discussion entries.
Summary
The Python SDK's value rules are now tested for every EQL type it can produce, through the public API. EQL (Encrypt Query Language) is CipherStash's set of PostgreSQL types for encrypted values. A value of the wrong type, out of range, or naive is refused before any request, from
encryptand fromqueryalike. The conversion is also checked against vitaminc's shared value corpus, and fuzzed.Changes
crates/core/testdata/value-conformance.jsonis a copy of vitaminc's shared corpus, at the commit the Go module uses. The published vitaminc crates do not include it. AREADME.mdbeside it records the source and how to update it.crates/core/tests/value_conformance.rschecks three things:tests/test_values.pyruns 18 refused values and 19 accepted edge values throughencrypt, and throughquerywhere the marker has aneqindex. EveryInvalidValueErrorcode is covered with no request.-0.0are accepted onRealandDoublefields. The engine and PostgreSQL both keep them, and a live test round-trips them.crates/core/fuzzis a detached cargo-fuzz crate with one target,value_frame.ValueFrame::ofgives the plain Rust type.fuzz:python-valuesruns it, andfuzz.ymlreplays its seeds on a pull request.fuzz.ymlnow also runs forstack-profile,stack-encrypt-derive,eql-bindings, and the Go guest'stargets.rs. A fuzz target compiles all four. The newscripts/__tests__/fuzz-workflow-paths.test.mjsderives the filter from each fuzz crate'sCargo.lock.Decimal's scale and a float's sign count.values::TypedandScalar::value()replace five copies of a frame writer and two of a value conversion.Verification
gate-probetests.mise run test:cargo: all core, corpus, and extension tests pass.cargo fmt --check: no findings, on the workspace and on the fuzz crate.pnpm run test:scripts: 84 of 84 files pass.fuzz-regressionjob on this PR is its first run.livejob on this PR is their first run.Related
Closes #1392. Refs #1341.
Review notes
JsonSearch. The catalog marks it producible, but thejsonkind has no Python type yet, and the decorator refuses it. Supporting it needs its own design: the Python types a JSON field takes, the rules for the values inside, and JSON queries.fuzz:python-valuesis in the rootmise.toml, not inlanguages/python/mise.toml. A script test requires every task there to run fromtests-python.yml, and the fuzz replay runs fromfuzz.yml.🤖 Generated with Claude Code