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
 |
The decorator now checks each field's type against its marker, the constructor, and the plan with the engine's own reader. The EQL resolver is the Go guest's targets.rs, compiled in place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A constructor that lacks a field is now its own error, apart from the decorator order. Pydantic aliases are read from Pydantic. A test covers a relationship to a class defined later. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2dc6d3d to
89830a6
Compare
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed 89830a6 · posted from Claude Code · claude-sonnet-5-5 · effort medium
Sources: code-review (medium, manual diff read) · claude-sonnet-5-5
Skills: code-review, review-cross-check, review-post
Findings (1): 0 critical, 0 major, 1 minor — 1 inline, 0 below.
Existing feedback on the PR (two bot comments, no review threads) raised nothing that overlaps. Findings were checked by reading the code at head; the Pydantic alias case was not executed.
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)
Two constructor-check cases must change before merging: a Pydantic dataclass init=False field, and a required attrs Ignore field with a private name. Both let a wrong model pass the class statement. The other items are optional and can wait.
A Codex finding said python-ci.test.mjs resolves #[path] from the wrong directory. I dropped it. The recursive walk passes the current directory as dir, so the path resolves from the file's own directory.
Other findings not posted as comments
- Optional: add tests for three untested branches in
_declaration.py. They are a positional-only constructor parameter, the Pydanticvalidate_by_nameandvalidation_aliasbranches, andOptional[T]on a Passthrough field. - Optional: no test covers the
inspect.signature(cls)failure fallback (_declaration.py:404), orRefusal.fieldinplan.rs. - Optional: if the frame encoder in
plan.rs:48returns an error,checkpanics. pyo3 raisesPanicException, whichexcept Exceptiondoes not catch. Return aRefusalinstead. - Optional:
#[allow(dead_code)]inlib.rs:25covers all oftargets.rs. OnlyHOLDS_EQLis unused. UseHOLDS_EQLin a const assertion, or use#[expect(dead_code)]. - Fix in a follow-up: the re-seed procedure in
AGENTS.md:96(copy the rootCargo.lock, thencargo metadata --offline) no longer gives this lock. The root lock has noeql-bindingsdependencies such asschemars. Document that those crates followpackages/eql/Cargo.lock. - Optional:
lazy_staticis 1.5.0 inlanguages/python/Cargo.lock, but 1.5.1 in the root lock. Pick one rule and write it down. - Optional:
_takesrefuses a Pydanticvalidation_aliasofAliasChoices("email", "Email"), although the constructor takesemailby name. - Optional: update the comments in
.github/dependabot.yml:267and the header oftests-python.ymlto mentioneql-bindingsand the#[path]check.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 3 found, 0 posted |
| claude | claude-opus-5-5 | rust | 2 found, 0 posted |
| claude | claude-opus-5-5 | python | 2 found, 2 posted |
| claude | claude-opus-5-5 | infracode | 2 found, 0 posted |
| codex | gpt-5.6-terra | test-gap | 0 found, 0 posted |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
| codex | gpt-5.6-terra | python | failed |
| codex | gpt-5.6-terra | infracode | 1 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 6 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 4 discussion entries.
Summary
@stackencrypt.modelnow checks what a declaration means, at the class statement. It refuses a field type that does not match its marker, a constructordecryptcould not call, and a plan the engine refuses. So a wrong model fails at import, before any database call.A marker such as
TextEqnames an EQL type. EQL is CipherStash's encrypted column format in PostgreSQL.Changes
Decorator checks (
_declaration.py), after rule A:declaration_unproducible: the engine cannot produce the marker yet.declaration_type_mismatch: the value type is not exactly the kind's Python type, such asstrforTextEq.declaration_not_outermost: the decorator ran below@dataclass, so the class had no constructor yet.declaration_constructor_missing_field: the constructor does not take a field's name, such as withinit=Falseor a Pydantic alias.declaration_ignore_required: anIgnorefield has no default.DeclarationErrorwith the engine's code, such asstack_encrypt_dynamic_plan.Engine check.
crates/core/src/plan.rsbuilds the plan frame and runs the engine'splan_with. The EQL resolver is the Go guest'stargets.rs, compiled in place through#[path], so both SDKs run one resolver.eql-bindingsis a new path dependency. Its lock entries match the EQL workspace's versions.CI.
tests-python.ymlalso runs on changes toeql-bindingsandtargets.rs.python-ci.test.mjschecks that the filter selects every#[path]file.Verification
cargo test, clippy,cargo fmt --check, mypy strict, and pyright are clean.pnpm run test:scripts: 83 of 83 files pass.python-cichecks fail when the new filter entries are removed.eqlfeature stops with the intendedcompile_error!.Related
Closes #1387. Part of #1341.
Review notes
_check_value_typeand_check_constructor, thenplan.rs.#[path]means the Python crate must list every dependency oftargets.rs. A Go change that adds one failstests-python.ymlat once.JsonSearchhas no Python type in v1, so it fails withdeclaration_type_mismatch."type"key. The encrypt ticket can add one.🤖 Generated with Claude Code