Repository navigation
feat(python): encrypt models into EQL v4 rows - #1499
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
 |
This comment has been minimized.
This comment has been minimized.
Cipher.encrypt returns one dict per model. A sealed field is a str of EQL JSON, which psycopg inserts with a plain %s and no cast. A passthrough field is the value itself, echoed by the engine. Every value is checked by exact type and range while the models are written into the engine's input frame. A bad value raises the new InvalidValueError with the field's name. No request is made, and the engine client is not built. A property that raises propagates unchanged. One engine client is shared per process, with one multi-thread tokio runtime. It is built on the first encrypt of any Cipher, and the GIL is released around each engine call. Its ZeroKMS connection counts requests, so the tests check the 500-keys-per-request rule. The http feature is on, and every new crate matches the root lock's version. A live CI job in tests-python.yml encrypts against ZeroKMS and inserts a row into Postgres with the EQL v4 bundle from the tree. It skips on a fork's pull request. The EQL cache test now accepts a second workspace's cache step in the same job. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A Decimal passthrough now comes back as a Decimal. Its digits go to Decimal as a tuple, not as bytes. A zero Decimal with a huge exponent no longer hangs the process with the GIL held. A zero with a positive exponent returns at once. An exponent above 38 is refused as value_not_representable. The shared client now reads CS_ZEROKMS_HOST, then CS_VITUR_HOST, as the other SDKs do. A bad value is refused as stack_kms_invalid_endpoint, before the credentials are read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
encrypt failed on every model with a Passthrough field. The extension wrote the value as a passthrough node. The plan's "passthrough" output already carries the field's frame as it is, so the engine returned the value nested in a second passthrough, and reading it failed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f4a374a to
d1cf50d
Compare
There was a problem hiding this comment.
Review attestation
Reviewed d1cf50d · posted from Claude Code · claude-sonnet-5-5 · effort medium
Sources: code-review · model unknown
Skills: code-review, review-cross-check, review-post
Findings (9): 0 critical, 4 major, 5 minor — 9 inline, 0 below. Each was checked against d1cf50d by reading the code; none was reproduced. No existing review feedback overlapped.
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 (1 of 8 review job(s) failed)
Nothing blocks this merge. All findings are test gaps or small CI and documentation fixes, and they can wait for a follow-up. The earlier review threads already cover the shared-client concurrency, fork safety and utcoffset() points, so they are not repeated.
Other findings not posted as comments
languages/python/crates/core/src/values.rs:162: theInt::I128branch (a passthroughintwider than 64 bits) has no test. Addi64::MAX + 1,i128::MINandi128::MAXtoa_scalar_written_as_a_passthrough_reads_back.languages/python/tests/test_encrypt_live.py:101: live tests check the returned passthrough forintandDecimalonly. Add a parametrized test forstr,bytes,float,bool,dateanddatetime.languages/python/tests/test_encrypt.py:203: no test coversCS_VITUR_HOST, or thatCS_ZEROKMS_HOSTis read first.languages/python/crates/engine/src/values.rs:174: anintwider than 128 bits says "does not fit 128 bits", while other too-wide values name their kind (for exampleint16). Use one message that names the declared kind.languages/python/crates/core/src/encrypt.rs:106: thestack_encrypt_unknownbranches inread_rowsandread_rowhave no test. They need a forged engine output.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 5 found, 2 posted |
| claude | claude-opus-5-5 | rust | 2 found, 1 posted |
| claude | claude-opus-5-5 | python | 1 found, 1 posted |
| claude | claude-opus-5-5 | infracode | 3 found, 3 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 1 posted |
| codex | gpt-5.6-terra | rust | failed |
| codex | gpt-5.6-terra | python | 0 found, 0 posted |
| codex | gpt-5.6-terra | infracode | 0 found, 0 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: the reader test did not run; only the word-and-shape check was applied.
Stack: position 8 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, 3 linked issue(s) and 13 discussion entries.
Summary
Cipher.encryptnow turns a list of declared models into rows for PostgreSQL with EQL v4. EQL is CipherStash's set of PostgreSQL types for encrypted values. A sealed field becomes astrof EQL JSON. psycopg inserts it with a plain%sand no cast. A passthrough field is the value itself.Changes
Cipher.encrypt(models)returns one dict per model. The newInvalidValueErroris aStashErrorand aValueError. A README stub shows theCREATE TABLE, the encrypt call, and the insert.InvalidValueErrorwith the field's name. The error never contains the value. The codes arevalue_none,value_wrong_type,value_out_of_range,value_not_representable, andvalue_naive_datetime.encryptof anyCipher. The Python global interpreter lock (GIL) is released during each engine call. The client readsCS_ZEROKMS_HOST. The client's ZeroKMS connection counts requests for the tests.httpfeature is on.stack-kms,tokio, andzerokms-protocolare new. Every new crate in the PythonCargo.lockhas the root lock's version.livejob intests-python.ymlrunsmise run test:live. The job encrypts against ZeroKMS and inserts rows into apostgres:17service with the EQL v4 bundle from this tree. The job skips on a pull request from a fork.Verification
gate-probetests.cargo test --locked, clippy, andcargo fmt --check: no failures or warnings.pnpm run test:scripts: 83 of 83 files pass.livejob on this PR.Related
Closes #1389. Refs #1341.
Review notes
"type"key in the output. python:queryan equality term #1391 can add it.KmsErroris not built yet. A ZeroKMS failure is aStashError.eql-suite-ci.test.mjsnow accepts a Rust cache step for a second workspace in the same job. Thelivejob needs one forlanguages/pythonand one forpackages/eql.🤖 Generated with Claude Code