Summary
SDResult in buckaroo/jlisp/configure_utils.py is declared @dataclass(frozen=True), but sd_updates is a plain dict so callers retain a live reference and can mutate it after constructing the result.
@dataclass(frozen=True)
class SDResult:
df: Any
sd_updates: Dict[str, Dict[str, Any]]frozen=True only blocks reassignment of the field itself. result.sd_updates['x'] = ... still mutates the same dict the caller holds — and the same dict the interpreter is about to read from in apply-result!.
Why it's not a correctness bug today
The interpreter's _apply_result closure (configure_utils.py:82) only reads from result.sd_updates. No code in the SDResult write path mutates after construction. So in practice nothing breaks.
But the docstring says:
Frozen because the result is a one-shot return value — the interpreter only reads from it.
That overstates what frozen=True guarantees, and a future change inside _apply_result (or a clever command author chaining results) could rely on the framing.
Options
Pick one:
- Drop the framing. Edit the docstring to say the dataclass is frozen to prevent field reassignment, not to make the contents immutable. Cheapest fix.
- Make it actually immutable. Convert
sd_updates to a nested-immutable form in __post_init__ — wrap the outer dict in MappingProxyType, and either wrap each inner dict too or convert to frozenset of (col, key, value) tuples. The merge loop in _apply_result would need to handle the new shape.
- Validate shape on construction. Add a
__post_init__ that asserts sd_updates is a dict of dicts. Doesn't fix mutability but catches the related class of bug where a command returns SDResult(df, "not a dict") — currently surfaces as a confusing TypeError deep in the lisp dispatch.
Option 1 is the minimal honest fix. Option 2 mirrors the read-only MappingProxyType treatment already applied to sd in buckaroo_transform, and would be consistent with the design doc's read-only-by-default stance.
Refs
🤖 Generated with Claude Code
Summary
SDResultinbuckaroo/jlisp/configure_utils.pyis declared@dataclass(frozen=True), butsd_updatesis a plaindictso callers retain a live reference and can mutate it after constructing the result.frozen=Trueonly blocks reassignment of the field itself.result.sd_updates['x'] = ...still mutates the same dict the caller holds — and the same dict the interpreter is about to read from inapply-result!.Why it's not a correctness bug today
The interpreter's
_apply_resultclosure (configure_utils.py:82) only reads fromresult.sd_updates. No code in the SDResult write path mutates after construction. So in practice nothing breaks.But the docstring says:
That overstates what
frozen=Trueguarantees, and a future change inside_apply_result(or a clever command author chaining results) could rely on the framing.Options
Pick one:
sd_updatesto a nested-immutable form in__post_init__— wrap the outer dict inMappingProxyType, and either wrap each inner dict too or convert tofrozensetof(col, key, value)tuples. The merge loop in_apply_resultwould need to handle the new shape.__post_init__that assertssd_updatesis adictofdicts. Doesn't fix mutability but catches the related class of bug where a command returnsSDResult(df, "not a dict")— currently surfaces as a confusingTypeErrordeep in the lisp dispatch.Option 1 is the minimal honest fix. Option 2 mirrors the read-only
MappingProxyTypetreatment already applied tosdinbuckaroo_transform, and would be consistent with the design doc's read-only-by-default stance.Refs
feat(jlisp): sd channel).sdproxy view itself (see feat(jlisp): sd channel — SDResult return + sd-as-arg read-only view #755 (comment)) — same family of "MappingProxyType only freezes one level" issue. Worth resolving the two together.🤖 Generated with Claude Code