Repository navigation
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
 |
0ebe128 to
d777757
Compare
A model declares each field with a marker named after its EQL type, such as TextEq. The spec (#1341) wants the markers generated from the EQL catalog, beside the Go package, so Python and Go cannot disagree about a type. eql-codegen gains a py-eql emitter over the same rows as go-eql. It writes stackencrypt/_eql.py: one Marker subclass per row with the ten catalog attributes, a TYPES tuple, and an __all__ of the producible markers that the package re-exports. types:generate runs it and types:check diffs it. Two parity tests hold it: the committed file must match the render, and every marker must match its Go Types row. The file is not added to the EQL workflow path filters, the same as eql_gen.go. A path outside packages/eql/ would need a new exception in the filter guard, and the catalog changes that matter already start the EQL workflow. Refs #1385 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`go-eql` and `py-eql` write outside the EQL subtree, so an `EQL_CODEGEN_OUT_ROOT` override sent them outside the throwaway tree. Both now refuse the override with exit 2, and a CLI test covers it. `types:generate` now also snapshots `eql_gen.go` and `_eql.py`. A failed run restores them with the bindings, so the tree is never left mixed. `Marker` declares the ten attributes with types, so mypy and pyright accept them on a `type[Marker]`. The Go and Python emitters now take their shared wording from two methods on `Row` in `targets.rs`: `summary` and `status`. Both generated files are unchanged. The parity test skips each docstring before it reads attributes, so a catalog reason containing ` = ` cannot be read as one. The eql-bindings README and AGENTS.md now say that `_eql.py` is generated. Refs #1385 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d777757 to
240921c
Compare
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed 240921c · posted from Claude Code · claude-sonnet-5-5 · effort medium
Sources: manual review (code-review not invoked as a skill) · claude-sonnet-5-5
Skills: none
No findings.
Checked: eql-codegen py_eql_parity and CLI tests pass on this commit; _eql.py has 51 classes matching the catalog. Two eql-codegen lib tests (generate_type_failure_does_not_delete_before_writing, write_failure_preserves_existing_target_and_leaves_no_temp) failed when run as root and were not compared against the base. CI and mypy/pyright were not run.
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 (1 of 8 review job(s) failed)
Nothing must change before merging. The four inline comments are optional hardening and test gaps; they can wait for a follow-up.
Other findings not posted as comments
languages/python/python/stackencrypt/_markers.py:14— Optional:Markeris a plain class. Code can assign to a marker's catalog attributes, or writeAnnotated[str, TextEq()](an instance), and the declaration reader in #1386 finds markers by identity. A metaclass that refuses assignment and instantiation would give clear errors..github/workflows/tests-python.yml:173— Optional: thepy_eql_paritystep runs in both Python matrix legs and gives the same result. Addingif: matrix.python == '3.14'would save build time.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 2 found, 2 posted |
| claude | claude-opus-5-5 | rust | 1 found, 1 posted |
| claude | claude-opus-5-5 | python | 1 found, 0 posted |
| claude | claude-opus-5-5 | infracode | 1 found, 0 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 1 posted |
| 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 |
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 4 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 3 discussion entries.
| # root Rust toolchain; tests-golang.yml compiles that workspace the | ||
| # same way. The target directory is not cached: the build is seconds. | ||
| - name: stackencrypt._eql matches the EQL catalog | ||
| run: cargo test --locked --manifest-path ../../packages/eql/Cargo.toml -p eql-codegen --test py_eql_parity |
There was a problem hiding this comment.
Optional: no repo test checks that tests-python.yml still runs py_eql_parity.
Impact: This step is the only check that runs when someone edits _eql.py by hand. test-eql.yml does not start on a change under languages/python. If a later edit removes or renames this step, the Python workflow still passes. A hand edit to _eql.py then merges with no failing check.
Evidence: scripts/__tests__/python-ci.test.mjs checks the path filter and the matrices. No test under scripts/__tests__ names py_eql_parity.
Fix: Add a case to python-ci.test.mjs that reads the workflow and expects exactly one run step containing cargo test, --locked, -p eql-codegen and --test py_eql_parity.
Found by 1 model: claude
| /// A Python string literal. Catalog text is printable, so only the quote | ||
| /// and the backslash need escaping; anything else is refused rather than | ||
| /// written in an escape Python would read differently. | ||
| fn py_str(s: &str) -> String { |
There was a problem hiding this comment.
Optional: the input guards in py_str and docstring have no test.
Impact: Today no catalog text has a control character, """, or a backslash, so the parity test never runs these branches. If a later edit removes a guard, nothing fails until the catalog gets such text. Then the generator writes a _eql.py that Python reads differently or cannot parse.
Fix: Add unit tests in this module: py_str("a\nb") must panic with control character; py_opt(None) must give None; a docstring with a backslash must double it; a docstring with """ must panic.
Found by 1 model: claude
| }; | ||
| let _ = write!( | ||
| out, | ||
| "\n\nclass {name}(Marker):\n {doc}\n\n \ |
There was a problem hiding this comment.
Optional: the catalog name is written as a Python class name with no check that it is a valid identifier.
Impact: A catalog name that is a Python keyword (None, True) gives a SyntaxError on import. The name Marker replaces the imported base class, so later classes inherit from the wrong class with no error. cargo test -p eql-codegen passes in both cases, so the failure shows only in the Python job, far from the cause.
Evidence: class() puts row.name straight into class {name}(Marker):. The TYPES tuple uses it the same way.
Fix: Add a py_ident helper that asserts the name starts with an uppercase ASCII letter, has only ASCII letters and digits, and is not Marker, None, True, False or TYPES. Call it from class() and from the TYPES writer.
Found by 1 model: claude
| for command in ["go-eql", "py-eql"] { | ||
| let out = Command::new(bin()) | ||
| .arg(command) | ||
| .env("EQL_CODEGEN_OUT_ROOT", out_root.0.as_os_str()) |
There was a problem hiding this comment.
Fix in a follow-up: the override-rejection test does not check that go-eql and py-eql write no file.
Impact: A regression could exit with code 2 after an emitter has already written outside the throwaway tree. This test would still pass.
Evidence: The test checks only the exit code, stdout and stderr. Its doc comment promises refusal before any write.
Fix: Use a nested output root, such as one/two under the temporary directory. After each command, assert that out_root.join(GO_EQL_PATH) and out_root.join(PY_EQL_PATH) do not exist.
Found by 1 model: codex
Summary
A Python developer will declare each encrypted field with a marker named after its EQL type, for example
Annotated[str, TextEq]. EQL (Encrypt Query Language) is the set of PostgreSQL types that store and query encrypted values. This PR adds those markers, generated from the EQL catalog by the same generator that writes the Go package. So Python and Go cannot disagree about a type.Nothing reads the markers yet. The
modeldecorator that reads them comes next, in #1386.Changes
eql-codegenhas a newpy-eqlcommand. It writeslanguages/python/python/stackencrypt/_eql.pyfrom the same catalog rows as the Goencrypt/eqlpackage.sql_domain(public.eql_v4_text_eq) andquery_sql_domain. ATYPEStuple lists them all._markers.pyhas theMarkerbase class,Passthrough(stored unencrypted), andIgnore(not a column).Markerdeclares the ten attributes with types, for mypy and pyright.from stackencrypt import TextEqworks for the 50 types the engine produces today. The one it cannot produce,Json, givesImportError.mise run types:generaterunspy-eql, andtypes:checkdiffs_eql.py. A failedtypes:generaterun restores_eql.pyand the Go file, as it already did for the bindings.Row::summaryandRow::statusintargets.rs. The Go output is unchanged.go-eqlandpy-eqlwrite outsidepackages/eql/, so they now refuseEQL_CODEGEN_OUT_ROOTinstead of writing outside the throwaway tree.AGENTS.mdsay that_eql.pyis generated.Verification
cargo test -p eql-codegen: all pass. The newpy_eql_parity.rschecks the committed file against the catalog and every marker against its GoTypesrow. I planted an error for each check, and each one failed with a clear message.cargo fmt --checkandcargo clippy -D warningsforeql-codegen: both pass.mise run testinlanguages/python: 61 passed and 2 skipped. The skipped tests are the probe tests from test(python): verify the two engine facts the spec depends on (#1384) #1414. They run only in a special build.--strictand pyright: both acceptm.query_sql_domainon atype[Marker]. Both rejectfrom stackencrypt import Json.mise run types:generate: exits 0 and changes no other generated file. With its last step forced to fail, it restored both cross-tree files.pnpm run test:scriptson Node 22: 83 of 83 test files pass.Not tested:
Related
Closes #1385. Refs #1341 (the spec). Stacked on #1414.
Review notes
Start with
packages/eql/crates/eql-codegen/src/py_eql.rs, then the generated_eql.py.The EQL workflow path filters do not list
_eql.py, the same as the Go fileeql_gen.go. A path outsidepackages/eql/would need a new exception in the guardeql-suite-ci.test.mjs. Instead,tests-python.ymlruns the parity tests, so a hand edit to_eql.pyfails the Python workflow.PassthroughandIgnoreinherit the attribute declarations fromMarker, but set no values. A type checker acceptsPassthrough.sql_domain, but it raisesAttributeErrorat run time.#1385 says 11 catalog types are unproducible. On the current base, only
Jsonis. The tests compare the Python and Go lists and assume no count.🤖 Generated with Claude Code