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
| for field in read: | ||
| parameter = parameters.get(field.name) | ||
| if field.marker is Ignore: | ||
| if parameter is not None and _required(parameter): |
There was a problem hiding this comment.
minor A required Ignore field on a Pydantic model with an alias (Field(alias="X")) is not refused: the signature lists the alias, so parameters.get(field.name) is None and declaration_ignore_required is skipped. Decrypt then omits the field and Pydantic fails at construction. Look the Ignore field up through _takes' alias logic, or check info.is_required() for Pydantic fields.
Source: code-review (medium) · 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 (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.
| # says whether the name is taken too. With extra="allow" the signature | ||
| # has **kwargs, and a name that is not taken lands in the extras. | ||
| alias = getattr(info, "validation_alias", None) or getattr(info, "alias", None) | ||
| if alias is None or alias == name: |
There was a problem hiding this comment.
Change before merge: _takes accepts a Pydantic dataclass field declared with init=False, but the constructor does not take that field.
Impact: A Pydantic dataclass sets __pydantic_fields__, so _takes uses the Pydantic branch. The field has no alias, so the function returns True and never reads the signature. The class declares with no error. Then decrypt passes the field by name, and construction fails or drops the value. The spec wants declaration_constructor_missing_field at the class statement.
Evidence: _takes returns at the alias is None line without reading parameter. Pydantic's generated signature leaves out init=False fields. A plain-dataclass init=False case is tested. The Pydantic dataclass case is not. A Pydantic dataclass also keeps its config in __pydantic_config__, not model_config.
Fix: When the field has no alias, use the signature as the answer. Read the config from either attribute. Add a Pydantic dataclass case to the init=False tests.
if alias is None or alias == name:
return _keyword(parameter) or any_keyword
config = getattr(cls, "model_config", None) or getattr(cls, "__pydantic_config__", None) or {}
return bool(config.get("populate_by_name") or config.get("validate_by_name"))Found by 1 model: claude
| if base is not object | ||
| ) | ||
| for field in read: | ||
| parameter = parameters.get(field.name) |
There was a problem hiding this comment.
Change before merge: a required Ignore field with an attrs private name is not refused.
Impact: attrs names the constructor argument without the leading underscore. For _cache: Annotated[dict, Ignore] with no default, the parameter is cache. So parameters.get("_cache") returns None, and the declaration_ignore_required check does not run. The class declares with no error. The first decrypt then fails with TypeError: missing 1 required argument.
Evidence: The Ignore branch uses parameter = parameters.get(field.name). The existing test passes only because it uses a public name. The plan-field path handles private names through _takes, so only the Ignore path misses it.
Fix: Look up the parameter by the attrs init name, and add a private-name case to the Ignore test.
def _init_name(cls: type, name: str) -> str:
for attribute in getattr(cls, "__attrs_attrs__", ()):
if attribute.name == name:
return getattr(attribute, "alias", None) or name.lstrip("_")
return name
parameter = parameters.get(_init_name(cls, field.name))Found by 1 model: claude
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