Repository navigation
feat(python): decrypt rows back into models - #1509
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.decrypt takes the rows psycopg read with dict_row and returns instances of the declared class, built with its constructor. A sealed column may be EQL JSON as a str, as UTF-8 bytes, or as a dict. Other columns are ignored. Every row is checked before any key request: its shape, its declared columns, its EQL JSON, and the table and column the JSON's "i" names. Each passthrough value is checked by exact type, as on encrypt. A refused row raises the new DecryptError or InvalidValueError, and the call returns no instance. The core crate decrypts with the engine's fake key source in its tests, so the round trip is tested without ZeroKMS. The engine does not say which field a kind mismatch was in. So decrypt_kind_mismatch names the field only when the plan has one sealed field. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Open rows under the client scope, as Go's Client.Open does. A row sealed under another keyset of the same client now decrypts. Map stack_encrypt_foreign_keyset and stack_encrypt_context_mismatch to AuthenticationError. Name the row in a malformed-value error, and check for a duplicate typed null as the scalar path does. Borrow str and bytes column values, so the EQL text is copied once, into the engine's frame. Share the frame helpers between encrypt and decrypt, and move the credential fixtures into conftest.py. A kind mismatch names its field only when the model has one sealed field. The engine does not name the field yet. #1508 tracks that. The README says that a passthrough datetime needs a timestamptz column, because psycopg reads timestamp as a naive datetime. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Encode a str column value with encode_utf8, and use the bytes it returns. Under abi3-py39, PyString::to_cow encodes the text and then copies it again, on every Python version. Borrowing the UTF-8 of a str is in the limited API only from Python 3.10. Add a test that the engine names the field of a value with only an identifier, in a model with two sealed fields. So the field of decrypt_malformed does not depend on #1508. Move the untouched fixture into conftest.py, so the decrypt and encrypt tests share one copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1390) SQLAlchemy finds a relationship's target in the type directly inside Mapped[...]. It does not look inside Annotated. So a bare relationship() on Mapped[Annotated[List["Post"], Ignore]] took List["Post"] as the target, and the first instance failed to configure the mapper. The live tests on SQLAlchemyUser failed on that. Naming the target, relationship("Post"), lets the mapper configure. posts is still not a list under this form. Spec #1341 records that, and leaves the fix to the SQLAlchemy layer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tion (#1390) SQLAlchemy reads a relationship's target and collection type from the type directly inside Mapped[...]. It does not look inside Annotated. So Ignore works on a relationship to one object, and a collection marked Ignore maps as a single object. ADR-0001 records this in an amendment. The README shows a collection left without an annotation, one object marked Ignore, and the Optional form, which must name its target. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e0ddde6 to
9d5188b
Compare
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed 9d5188b · posted from Claude Code · model ID claude-sonnet-5-5 · effort medium
Sources: code-review · model unstated
Skills: code-review, review-cross-check, review-post
Findings (4): 0 critical, 1 major, 3 minor — 3 inline, 1 below.
languages/python/tests/test_encrypt_live.py:170minor The SQLAlchemy test model declaresposts: Mapped[Annotated[List["Post"], Ignore]], which the README and the ADR amendment in this PR say not to do (SQLAlchemy then maps one object, not a list). This is likely behind the two failing live tests in the Blacksmith comment (comment). Use the documented unannotatedposts = relationship("Post")and test that form.
Not verified at head, so not posted: partial construction when a model constructor raises on row N (all-or-nothing docstring), the double JSON parse of EQL payloads, and duplicated Refusal construction.
Source: code-review · model unstated (Claude Code)
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 (2 of 8 review job(s) failed)
infracode — claude (claude-opus-5-5)
This pull request has no infrastructure-as-code changes. It changes no Terraform, CloudFormation, mise task, workflow or observability config, so this review has no findings. The one manifest change, in languages/python/crates/core/Cargo.toml, moves serde_json from a dev dependency to a normal dependency and enables the test-support feature of stack-kms. That feature has no dependencies, so Cargo.lock does not need to change.
python — claude (claude-opus-5-5)
Nothing must change before merging. The two points below can wait. The earlier review already covers SQL NULL handling, the field name on decrypt_kind_mismatch, conversion errors in python_value, and the failing SQLAlchemy live tests, so this review does not repeat them.
| Severity | Finding | Location |
|---|---|---|
| Fix in a follow-up | Every exception from json.dumps and from the passthrough read becomes decrypt_malformed |
crates/engine/src/decrypt.rs:158 |
| Optional | A single dict passed to decrypt gives a misleading dict_row message |
python/stackencrypt/_cipher.py:90 |
| Optional | encrypt takes Sequence[object] though Cipher is generic over T |
python/stackencrypt/_cipher.py:18 |
I did not run mypy or pytest. This checkout has no built .venv.
rust — claude (claude-opus-5-5)
No Rust finding needs to change before merging. The read path has no unsafe, no unwrap on caller input in library code, and the GIL is released only around the engine call. The three comments below are about error reporting and can wait for a follow-up.
The discussion already covers SQL NULL, the unnamed field of decrypt_kind_mismatch, and python_value errors. This review does not repeat them. cargo is not installed here, so clippy and the tests did not run.
| Severity | Finding | Location |
|---|---|---|
| follow-up | Engine-found decrypt_malformed and decrypt_kind_mismatch do not name the row |
languages/python/crates/core/src/decrypt.rs:221 |
| minor | A str with a lone surrogate raises a raw UnicodeEncodeError |
languages/python/crates/engine/src/decrypt.rs:146 |
| minor | KeyboardInterrupt and MemoryError become decrypt_malformed |
languages/python/crates/engine/src/decrypt.rs:157 |
test-gap — claude (claude-opus-5-5)
Nothing here needs to change before merging. The four gaps below are refusal or error branches in the new decrypt code that no test runs. You can add these tests now or in a follow-up.
I checked the later pull requests in the stack (#1510 to #1582). #1513 adds the tests for decrypt_not_representable, and #1511 adds the round trip for every producible sealed type. So this review does not report those. It also does not repeat what the earlier review raised: NULL handling, the docstring for decrypt_kind_mismatch, and the python_value error wrapping.
infracode — codex (gpt-5.6-terra)
This PR has no infrastructure-as-code changes to review.
rust — codex (gpt-5.6-terra)
I found no new Rust issues in this diff. The existing discussion already covers the current decrypt-path concerns.
Other findings not posted as comments
languages/python/crates/core/src/decrypt.rs:231— Optional: no test coversdecrypt_kind_mismatchon a plan with two or more sealed fields, wherefieldisNone.languages/python/crates/engine/src/decrypt.rs:66— Optional: no test checks that a row that is aMappingbut not adictis accepted.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 4 found, 2 posted |
| claude | claude-opus-5-5 | rust | 3 found, 3 posted |
| claude | claude-opus-5-5 | python | 3 found, 3 posted |
| claude | claude-opus-5-5 | infracode | 0 found, 0 posted |
| codex | gpt-5.6-terra | test-gap | failed |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
| codex | gpt-5.6-terra | python | failed |
| codex | gpt-5.6-terra | infracode | 0 found, 0 posted |
Plain language: the reader test did not run; only the word-and-shape check was applied.
Stack: position 9 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 7 discussion entries.
Description:
Summary
Cipher.decryptnow turns rows read from PostgreSQL back into instances of the declared model. EQL (Encrypt Query Language) is CipherStash's set of PostgreSQL types for encrypted values. Each row is a mapping of column names to values, as psycopg'sdict_rowreturns it. All the rows are opened with one key request per 500 sealed values, and the model's own constructor builds each instance.Changes
Cipher.decrypt(rows)returns a list of instances of the model class. The newDecryptErroris aStashError. The README has a new section, "Read the rows back", with adict_rowexample.decrypt_row_shape. A missing declared column isdecrypt_missing_column. A column value that is not EQL JSON isdecrypt_malformed. A value stored under another table or column isdecrypt_foreign_row. A passthrough value of the wrong type is anInvalidValueError. Each error names the row. Each error names the field too, exceptdecrypt_row_shape.decrypt_kind_mismatch. An opened date or timestamp that Python cannot hold isdecrypt_not_representable. No instance is built when any value fails.str, UTF-8bytes, or adictthat the driver already parsed. Columns the model does not declare are ignored.Client.Opendoes the same.stack_encrypt_foreign_keysetandstack_encrypt_context_mismatchare nowAuthenticationError.postgres:17for dataclasses, attrs, Pydantic,NamedTuple, and SQLAlchemy models. The credential fixtures are in a newtests/conftest.py.Verification
gate-probetests.mise run test:cargo: 24 core tests and 5 engine tests passed.cargo fmt --check, and mypy: no findings.pnpm run test:scripts: 83 of 83 files pass.livejob on this PR.Related
Closes #1390. Refs #1341 and #1508.
Review notes
decrypt_kind_mismatchnames its field only when the model has one sealed field. The engine error does not name the field. Stack Encrypt: a type mismatch in a batch decrypt does not name the field #1508 tracks that engine change.reasonofdecrypt_kind_mismatchisNone. The spec asked for the stored kind. The engine never names what a value held, so Python SDK: core call over psycopg, on EQL v4 and Stack Encrypt #1341 needs an amendment.datetimeneeds atimestamptzcolumn. psycopg readstimestampas a naivedatetime, anddecryptrefuses it. The README says so.Ignorecomes back asNone. The later SQLAlchemy layer will handle relationships. Python SDK: core call over psycopg, on EQL v4 and Stack Encrypt #1341 has a note on this.🤖 Generated with Claude Code