Extract runtime facade compatibility layer from PR 188 - #212
Conversation
Split the non-governance portion of PR 188 into a standalone change that keeps public package facades off direct _internal imports. Add explicit defaults/adapters modules for embedding, event, hook, plan, and transport, restore the historical permissive DefaultSecurityBoundary export, and document the min-surface facade rule. Also add checkpoint facade compatibility helpers plus regression tests for checkpoint message round-trips, security boundary behavior, and package initializer import hygiene, while aligning the transport facade exports with the current main-branch transport API.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bb47069ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Message( | ||
| role=item.get("role", "user"), | ||
| text=item.get("text") or item.get("content", ""), | ||
| name=item.get("name"), | ||
| metadata=dict(item.get("metadata") or {}), |
There was a problem hiding this comment.
The new StmContributor restore path reconstructs each message with only role, text, name, and metadata, so any checkpoint containing non-chat STM entries (for example tool_call/tool_result messages with structured data or attachments) is silently downgraded to default chat messages and loses payload fields on restore. This corrupts checkpoint round-trips for realistic agent sessions that rely on typed message state after resume.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in 22cb1da.
StmContributor now serializes/restores kind, attachments, data, mark, and id in addition to the legacy text/name/metadata fields, so typed STM entries survive checkpoint round-trips instead of being downgraded to default chat messages. Added a regression in tests/unit/test_checkpoint_defaults.py that round-trips attachments plus a TOOL_CALL message with structured payload.
| # Preserve historical facade behavior where `DefaultSecurityBoundary` is permissive. | ||
| DefaultSecurityBoundary = LegacyDefaultSecurityBoundary |
There was a problem hiding this comment.
Rebinding dare_framework.security.impl.DefaultSecurityBoundary to the legacy permissive class removes the from_config(...) classmethod that existed when this symbol aliased PolicySecurityBoundary, so callers that instantiate via DefaultSecurityBoundary.from_config(...) now fail with AttributeError. Because this commit positions impl as the public export layer for facade imports, this is a backward-incompatible API regression for existing consumers of that module path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in 22cb1da.
dare_framework.security.impl.DefaultSecurityBoundary is now a permissive compatibility class with a from_config(...) classmethod that delegates to PolicySecurityBoundary.from_config(...), preserving the old constructor contract for callers that instantiate through the impl module path. Added coverage in tests/unit/test_security_boundary.py to assert from_config(...) still returns a policy-configured boundary.
Preserve structured tool-call payloads during deduplication by incorporating message kind, name, attachments, and data into the compression identity key, so auto-compression no longer drops distinct assistant tool-call history with empty text. Also re-append the transient SmartContext reflection prompt after re-assembling messages for auto-compression, and add regression coverage for both review findings.
Preserve typed STM checkpoint round-trips by serializing message kind, attachments, data, mark, and id in the compatibility contributor instead of downgrading everything to plain chat messages on restore. Also restore the config-driven constructor contract on dare_framework.security.impl.DefaultSecurityBoundary so callers using from_config(...) still receive a policy-configured boundary, and add regression coverage for both compatibility paths.
|
Addressed the Codex review findings in Handled:
Verification:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22cb1da13e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| value.filename, | ||
| _freeze_value(getattr(value, "metadata", {})), | ||
| ) | ||
| return value |
There was a problem hiding this comment.
In the dedup_then_truncate path, _dedup_messages stores keys containing _freeze_value(msg.data) in a set, but _freeze_value falls through to return value for unsupported types. If a message payload contains an unhashable value (for example a set or an unhashable dataclass instance), compression raises TypeError during set membership and aborts the run. This is reachable in production when auto-compression is enabled with the default dedup_then_truncate strategy and tool outputs include non-JSON-native Python values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in f32e736.
_freeze_value() now normalizes sets/frozensets, dataclass instances, and other unhashable objects before they participate in the dedup identity key, so dedup_then_truncate no longer raises TypeError on non-JSON-native payload values. Added a regression in tests/unit/test_context_compression.py that deduplicates messages containing set payloads.
| ], | ||
| "data": _clone_payload(m.data), | ||
| "name": m.name, | ||
| "metadata": dict(getattr(m, "metadata", {}) or {}), |
There was a problem hiding this comment.
StmContributor.serialize deep-copies data and attachment metadata but only shallow-copies message metadata via dict(...). Nested metadata objects therefore stay aliased to live context objects, so mutating metadata after save() can silently mutate stored checkpoint contents and break point-in-time restore semantics. Checkpoint payload construction should deep-copy metadata the same way as other nested fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in f32e736.
StmContributor.serialize() now deep-copies message metadata the same way it already deep-copies attachment metadata and data, so saved checkpoint payloads are no longer aliased to live nested metadata objects. Added a regression in tests/unit/test_checkpoint_defaults.py that mutates the source message metadata after save() and verifies the restored checkpoint still contains the original nested value.
Make the default in-memory STM treat max_messages=0 as full eviction instead of relying on Python's -0 slice behavior, which previously retained the entire history. Add a Context-level regression test so the canonical basic compression path guarantees explicit zero-message requests clear the default STM.
Normalize non-JSON-native payload values such as sets and unhashable dataclass-like objects before deduplication so auto-compression cannot raise TypeError while building message identity keys. Also deep-copy nested checkpoint metadata during STM serialization to preserve point-in-time snapshot semantics, and add regressions for both behaviors.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f32e736e55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def get(self, checkpoint_id: str) -> dict[str, Any] | None: | ||
| if checkpoint_id not in self._store: | ||
| return None | ||
| return dict(self._store[checkpoint_id]) |
There was a problem hiding this comment.
MemoryCheckpointStore.get() returns only a shallow dict(...) copy, so nested structures remain shared with _store. In practice this corrupts checkpoints: after restore(), mutating nested message metadata in the restored context can mutate the stored snapshot, and a second restore returns the mutated state instead of the original checkpoint. Checkpoint snapshots should be immutable once saved, so this path needs a deep clone on read (and ideally on write as well).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in 5362ee5.
MemoryCheckpointStore now deep-copies payloads on both put() and get(), so nested checkpoint state cannot be mutated through restored objects or repeated reads. Added a regression in tests/unit/test_checkpoint_defaults.py that restores the same checkpoint twice and verifies mutating the first restored context does not affect the second restore.
Make budget_remaining treat zero-valued limits as finite instead of falling back to infinity, so strict zero-token runs correctly degrade retrieval instead of fetching LTM/knowledge under an exhausted budget. Also make compression dedup keys robust for unhashable payload values, and add regressions for both the zero-budget retrieval path and non-JSON-native payload deduplication.
Clone payloads on both write and read in MemoryCheckpointStore so nested checkpoint state cannot be mutated through restored objects or repeated reads. Add a regression that restores the same checkpoint twice and verifies mutating the first restored context does not affect the second restore.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5362ee5a8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if "current_milestone_idx" in payload and hasattr(state, "current_milestone_idx"): | ||
| state.current_milestone_idx = payload["current_milestone_idx"] |
There was a problem hiding this comment.
SessionStateContributor.serialize() persists task_id and run_id, but deserialize_and_apply() only reapplies current_milestone_idx, so checkpoint restore loses the saved session identity fields. In any resume flow that expects full session-state round-tripping (for example, restoring into a fresh context object), the restored state will keep whatever task_id/run_id happened to be on the target object, which can desynchronize traceability and subsequent orchestration logic from the checkpoint that was actually loaded.
Useful? React with 👍 / 👎.
|
Superseded by #213.
The replacement PR has been re-cut directly on top of |
1 participant
Summary
defaults/adapters/implmodules instead of direct._internalimportsDefaultSecurityBoundaryexport and add checkpoint facade compatibility helpersVerification
.venv/bin/pytest -q.venv/bin/pytest -q tests/unit/test_package_initializers_facade_pattern.py tests/unit/test_checkpoint_defaults.py tests/unit/test_security_boundary.py.venv/bin/pytest -q tests/unit/test_transport_typed_payloads.py tests/unit/test_transport_channel.py tests/unit/test_react_agent_gateway_injection.pyNotes
codex/main-baseline-fixesso the runtime-facade extraction stays isolated from the main-branch baseline repair.