Replace wire Dataclass with ClassInstance carrying instance identity - #682
Replace wire Dataclass with ClassInstance carrying instance identity#682samuelcolvin wants to merge 4 commits into
Dataclass with ClassInstance carrying instance identity#682Conversation
Generalize the host-dataclass mechanism to arbitrary class instances:
- monty.proto: `ClassInstance { name, instance_id, type_id, attrs, frozen,
is_dataclass }` replaces `Dataclass` (no field_names); `FunctionCall`
swaps `method_call` for `optional uint64 instance_id` (the receiver is no
longer prepended to args); `NameLookup` gains `optional uint64
instance_id` for lazy attribute lookups on host instances.
- Interpreter: `types/host_class.rs` (`HostClass`, `Type::HostClass`)
replaces `types/dataclass.rs`; public missing attrs suspend as a new
`AttrLookup` exit (uncached; getattr/hasattr and `_`-names stay local);
an Undefined answer raises AttributeError naming the real class;
sandbox-defined instances cross out structurally with instance_id 0;
dump format bumped to 5.
- pydantic_monty: new pure-Python `ClassInstance` policy wrapper
(eager/lazy/method exposure, convert_value hook) and per-session
`InstanceStore` replace `DcRegistry`/`dataclass_registry`/
`UnknownDataclass`; returning a host-sent instance yields the original
object; unknown instances become the read-only `MontyClassInstance`
proxy; bare dataclasses are rejected with a pointer to the wrapper.
- @pydantic/monty: full parity — TS `ClassInstance`/`InstanceStore`/
`MontyClassInstance`, working method calls and lazy lookups over both
the napi and wasm transports, non-plain inputs rejected.
- datatest: `FixtureRegistry` routes fixture calls by instance id; lazy
class attrs (`Point.dimensions`) verified against CPython.
- limitations/ documents all divergences (HostClass type name, repr over
eager attrs, no fields()/asdict() on host values, store not dumped).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ha4b6Zj3JBo4X3ZEPqZRr
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewThis pull request replaces the wire-level One low-impact issue remains: instance identities expose parent-process heap addresses to a malicious worker. Two other issues have already been addressed, but the remaining identifier should be replaced with a session-local opaque value to avoid leaking address information. Open issues (1)
Fixed/addressed: 2 · PR risk: 3/10 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
Merging this PR will improve performance by 4.77%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | ext_call_rows |
9.1 ms | 8.7 ms | +5.08% |
| ⚡ | ext_calls_1000 |
2.4 ms | 2.3 ms | +4.47% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing wire-classes (05cc5a2) with main (27c9b8d)
Footnotes
-
16 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Codecov Results 📊✅ Patch coverage is 85.30%. Project has 9231 uncovered lines. Files with missing lines (19)Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 83.45% 83.73% +0.28%
==========================================
Files 243 243 —
Lines 56728 56748 +20
Branches 119594 119572 -22
==========================================
+ Hits 47341 47517 +176
- Misses 9387 9231 -156
- Partials 3289 3287 -2Generated by Codecov Action |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 79 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Error messages and type() no longer show the static HostClass placeholder:
- `Value::py_type_name` resolves the host instance's real class name via
the same heap pre-check named tuples already use, so `unhashable type:
'Point'`-style messages match CPython.
- `type(x)` on a host class instance materializes a new lightweight
`HostClassType` heap object (repr `<class 'Point'>`, `__name__`,
equality/hash by class identity) instead of the placeholder marker —
the safe counterpart of returning the class object for sandbox
instances, since `Type` cannot carry an untracked HeapId. Each call
allocates fresh, so `type(a) is type(b)` is False (documented); it is
not callable and not usable with isinstance.
- Sandbox→host, the type object crosses as the resolved-name
`MontyType::Instance` output shape; its host conversion error message
is generalized ("cannot convert class 'X' to a host type object").
Covered in dataclass__basic.py (CPython parity, incl. a memory-model-
checks run) and test_class_instance.py; divergences (no module
qualification in repr, per-call identity) recorded in
limitations/classes.md.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ha4b6Zj3JBo4X3ZEPqZRr
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- HostClass hash is now attr-order independent (matches order-insensitive eq) - repr no longer holds an unguarded value clone across fallible key formatting - JS prepare/restore walks are depth-capped; policies duck-type set-likes; wasm codec validates u64 ids and malformed attr pairs - JS NameLookupSnapshot.resumeValue answers lazy attr lookups by hand - object_bridge uses is_dataclass_class instead of building the field list - docs: instance-store retention, per-language naming, AttributeError vs NameError on instance lookups, JS README name/frozen options Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ha4b6Zj3JBo4X3ZEPqZRr
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ha4b6Zj3JBo4X3ZEPqZRr
| let instance_type = instance.get_type(); | ||
|
|
||
| let name: String = instance_type.getattr(intern!(py, "__name__"))?.extract()?; | ||
| let instance_id = instance.as_ptr() as u64; |
There was a problem hiding this comment.
Low: Parent heap address disclosure
as_ptr() exposes the wrapped object's parent-process address as instance_id, which is serialized to the worker. A malicious worker can therefore collect parent heap addresses; use a session-local opaque counter or random identifier on the wire, with the pointer-to-identifier mapping retained only in the parent.
There was a problem hiding this comment.
2 issues found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/monty-js/ts/session.ts">
<violation number="1" location="crates/monty-js/ts/session.ts:875">
P2: An unconvertible `resumeValue` argument rejects after consuming the snapshot but sends no response, leaving this session permanently suspended. Catch `prepare` failures and poison the session (matching lazy lookup handling), or convert the failure into a native resume before claiming the snapshot.</violation>
</file>
<file name="crates/monty-js/ts/classInstance.ts">
<violation number="1" location="crates/monty-js/ts/classInstance.ts:234">
P2: For outbound host values, the new uniform `MAX_INPUT_DEPTH = 48` cap does not actually mirror the receiver's depth budget for dicts and class instances. The wire decoder can handle 48 list levels but only ~32 dict levels and ~24 class-instance levels (each shape consumes a different number of proto message levels). Because `prepareInner` increments depth by 1 per container regardless of shape, a deeply nested plain object or a chain of `ClassInstance` eager attrs between ~25 and 48 levels deep passes `prepare` with a clean pass, then gets sent to the worker, where frame decoding fails — killing the checkout as a protocol failure instead of raising the promised catchable `'Max input depth exceeded'` error. This is exactly the failure your own `roundtrip.rs` test and this comment describe as needing a per-shape accounting. Consider charging each shape's real wire cost in the depth walk (or capping at the minimum shape capacity) so deep dicts/class instances fail here cleanly rather than crashing the worker.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| async resumeNameLookupValue(value: unknown): Promise<Snapshot> { | ||
| return this.advance( | ||
| (await this.native.resumeNameLookup(null, { value: prepare(value, this.instances) }, this.onPrint)) as NativeTurn, |
There was a problem hiding this comment.
P2: An unconvertible resumeValue argument rejects after consuming the snapshot but sends no response, leaving this session permanently suspended. Catch prepare failures and poison the session (matching lazy lookup handling), or convert the failure into a native resume before claiming the snapshot.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty-js/ts/session.ts, line 875:
<comment>An unconvertible `resumeValue` argument rejects after consuming the snapshot but sends no response, leaving this session permanently suspended. Catch `prepare` failures and poison the session (matching lazy lookup handling), or convert the failure into a native resume before claiming the snapshot.</comment>
<file context>
@@ -867,10 +867,15 @@ class SnapshotDriver {
+ async resumeNameLookupValue(value: unknown): Promise<Snapshot> {
+ return this.advance(
+ (await this.native.resumeNameLookup(null, { value: prepare(value, this.instances) }, this.onPrint)) as NativeTurn,
+ )
+ }
</file context>
| /** Nesting bound for outbound walks — mirrors the wire decoder's limit, so a | ||
| * too-deep value fails here with a catchable error instead of a stack | ||
| * overflow (`RangeError`) partway through the recursion. */ | ||
| const MAX_INPUT_DEPTH = 48 |
There was a problem hiding this comment.
P2: For outbound host values, the new uniform MAX_INPUT_DEPTH = 48 cap does not actually mirror the receiver's depth budget for dicts and class instances. The wire decoder can handle 48 list levels but only ~32 dict levels and ~24 class-instance levels (each shape consumes a different number of proto message levels). Because prepareInner increments depth by 1 per container regardless of shape, a deeply nested plain object or a chain of ClassInstance eager attrs between ~25 and 48 levels deep passes prepare with a clean pass, then gets sent to the worker, where frame decoding fails — killing the checkout as a protocol failure instead of raising the promised catchable 'Max input depth exceeded' error. This is exactly the failure your own roundtrip.rs test and this comment describe as needing a per-shape accounting. Consider charging each shape's real wire cost in the depth walk (or capping at the minimum shape capacity) so deep dicts/class instances fail here cleanly rather than crashing the worker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty-js/ts/classInstance.ts, line 234:
<comment>For outbound host values, the new uniform `MAX_INPUT_DEPTH = 48` cap does not actually mirror the receiver's depth budget for dicts and class instances. The wire decoder can handle 48 list levels but only ~32 dict levels and ~24 class-instance levels (each shape consumes a different number of proto message levels). Because `prepareInner` increments depth by 1 per container regardless of shape, a deeply nested plain object or a chain of `ClassInstance` eager attrs between ~25 and 48 levels deep passes `prepare` with a clean pass, then gets sent to the worker, where frame decoding fails — killing the checkout as a protocol failure instead of raising the promised catchable `'Max input depth exceeded'` error. This is exactly the failure your own `roundtrip.rs` test and this comment describe as needing a per-shape accounting. Consider charging each shape's real wire cost in the depth walk (or capping at the minimum shape capacity) so deep dicts/class instances fail here cleanly rather than crashing the worker.</comment>
<file context>
@@ -225,26 +225,40 @@ export function attributeErrorMessage(typeName: string, attrName: string): strin
+/** Nesting bound for outbound walks — mirrors the wire decoder's limit, so a
+ * too-deep value fails here with a catchable error instead of a stack
+ * overflow (`RangeError`) partway through the recursion. */
+const MAX_INPUT_DEPTH = 48
+/** Backstop for inbound walks; wire values are already bounded well below. */
+const MAX_OUTPUT_DEPTH = 200
</file context>
rewitt94
left a comment
rewitt94
left a comment
There was a problem hiding this comment.
This is cool!
I was trying to reason how far to consolidate the host and native dataclasses, but looks like host defined classes will always have a lot of distinct functionality.
Claude and I found a few issues that I think are worth looking at.
| let instance_type = instance.get_type(); | ||
|
|
||
| let name: String = instance_type.getattr(intern!(py, "__name__"))?.extract()?; | ||
| let instance_id = instance.as_ptr() as u64; |
There was a problem hiding this comment.
After loading a snapshot, a stale instance_id can persist in the sandbox heap? And the lookup could then access the wrong wrapper? Not a sandbox escape as it can only get a wrapper, could create some weird bugs though.
/// Looks up the wrapper registered for `instance_id`.
pub fn get(&self, py: Python<'_>, instance_id: u64) -> PyResult<Option<Py<PyAny>>> {
Ok(self.instances.bind(py).get_item(instance_id)?.map(Bound::unbind))
}
Fwiw, python will regularly re-use ids too.
| instance_id: 0, | ||
| type_id: 0, | ||
| attrs: pairs_to_objects(children, vm, visited).into(), | ||
| // native `@dataclass` does not support `frozen=True` |
There was a problem hiding this comment.
Soon to change in #650
Maybe we could add a TODO here referencing that PR
| value, | ||
| eager_attrs=self.eager_attrs, | ||
| lazy_attrs=self.lazy_attrs, | ||
| allowed_methods=self.allowed_methods, |
There was a problem hiding this comment.
What if we wanted to control the access on the constructed child rather than just giving the same access?
i.e.
@dataclass
class Config:
secret: str
def rotate(self): ... # dangerous
@dataclass
class App:
config: Config # ← holds the same Config object
def get_config(self): return self.config
session.feed_run(code, inputs={
'config': ClassInstance(cfg, eager_attrs=['name'], allowed_methods=set()), # locked down
'app': ClassInstance(app, allowed_methods='all'), # wide open
})
Here I wanted Config block access to methods, but via App it would allow all.
🤔
| Ok(Value::Ref(vm.heap.allocate(HeapData::Dataclass(Box::new(dc))))) | ||
| .map_err(|_| InvalidInputError::invalid_type("unhashable class instance attr keys"))?; | ||
| let hc = HostClass::new(name, instance_id, type_id, dict, frozen, is_dataclass); | ||
| Ok(Value::Ref(vm.heap.allocate(HeapData::HostClass(Box::new(hc))))) |
There was a problem hiding this comment.
Should we check if the instance_id already exists? or should we always allocate fresh? 🤔
There was a problem hiding this comment.
The same instance passed from the host would also fail foo is bar
There was a problem hiding this comment.
Also if we overwrite in the instance store? should we have some overwrite logic here too? 🤔
| // Builtins cannot suspend, so a lazy host attribute lookup reads as | ||
| // absent (documented divergence — only `obj.attr` syntax consults the | ||
| // host). | ||
| Ok(CallResult::AttrLookup { .. }) => false, |
There was a problem hiding this comment.
Would an error be better than a silent false?
| /// when two wrappers sent the same attrs in different orders. | ||
| /// | ||
| /// Mutable (non-frozen) instances return `None` (unhashable). | ||
| fn py_hash(&self, _self_id: HeapId, vm: &mut VM<'h>) -> RunResult<Option<HashValue>> { |
There was a problem hiding this comment.
Methods in here are also in my dataclasses PR #650 - I will try and de-dupe the logic if appropriate
| Dataclass, | ||
| /// The type of a host-backed class instance ([`MontyObject::ClassInstance`]). | ||
| /// A static placeholder — the real class name appears in error messages and | ||
| /// reprs, but `type(x)` renders as `HostClass`. |
There was a problem hiding this comment.
I think type(x) reprs as <class 'Point'>
| // round-tripped sandbox instance): the interpreter cannot rebuild | ||
| // the original class binding, so method calls / lazy lookups on an | ||
| // id-0 instance surface with instance_id 0 and hosts answer | ||
| // Undefined, which raises AttributeError. |
There was a problem hiding this comment.
method calls -> RuntimeError
lazy lookups -> AttributeError
| class_name: self.get(vm.heap).name(vm.interns).to_owned(), | ||
| instance_id: self.get(vm.heap).instance_id(), | ||
| })), | ||
| // we use name here, not `self.py_type(heap)` hence returning a Ok(None) |
| } | ||
|
|
||
| /// Writes `ClassName(f1=v1, ...)`, shared by the host-supplied [`HostClass`] and | ||
| /// native `@dataclass` instances so the two renderings cannot drift. |
There was a problem hiding this comment.
👍 - will try and consolidate what we can
2 participants
Summary by cubic
Replaced wire
DataclasswithClassInstancecarryinginstanceIdso method calls and lazy attribute lookups route back to the host object and identity is preserved end‑to‑end. Error messages andtype()now show the real class name for host-backed instances.New Features
ClassInstance { name, instance_id, type_id, attrs, frozen, is_dataclass };FunctionCall.instance_idandNameLookup.instance_id; interpreter emitsAttrLookupfor public missing attrs; new host-backed instance/type support (HostClass, lightweightHostClassTypefromtype(x)with repr<class 'Name'>, equality/hash by class id, per-call identity, not callable);HostClasshash ignores attr order; dump format v5.pydantic_monty):ClassInstancewrapper (eager/lazy/method policy), per-sessionInstanceStore, and read‑onlyMontyClassInstanceproxy; returning a host‑sent instance yields the original object; bare dataclasses rejected with a hint to wrap; error messages use real class names;HostClassTypeexposes only__name__; internal conversion/dispatch usesis_dataclass_classdirectly.@pydantic/monty): mirroredClassInstance/MontyClassInstanceand sessionInstanceStore; worker transport and value codec supportClassInstanceandinstanceIdfor method calls and lazy lookups; prepare/restore walks are depth‑capped; policies accept set‑likes (duck‑typed); wasm codec validatesu64ids and attr pairs;NameLookupSnapshot.resumeValueanswers lazy lookups.Point.dimensions); docs clarify instance‑store retention, AttributeError vs NameError for instance lookups, and JS README includesname/frozenoptions.Migration
Dataclassuses withClassInstance; update tests and expectations.dataclass_registry; wrap host objects withClassInstance(...)instead.instanceId(receiver is not inargs); name lookups may carryinstanceId.type()on host instances.Written for commit 05cc5a2. Summary will update on new commits.