Extract runtime facade compatibility layer from PR 188 - #213
Conversation
Re-cut the runtime facade compatibility work from the old stacked PR onto the current main branch after PR 211 merged. Key changes: - restore public facade exports for checkpoint, embedding, event, hook, plan, security, and transport through defaults/adapters modules instead of direct _internal imports - preserve permissive security boundary compatibility and deep-copy checkpoint payload snapshots for stable round-trips - bring over the facade-focused regression coverage and min-surface review note - intentionally exclude the old stacked PR's compression/context baseline implementation changes Rationale: The previous PR 212 was stacked on an outdated pre-merge PR 211 base and still carried obsolete compression/context commits. This commit rebuilds the intended facade/checkpoint scope directly on top of main so the PR can be reviewed and merged independently.
|
@codex review |
![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: 8c82ca754c
ℹ️ 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".
| class DefaultSecurityBoundary(LegacyDefaultSecurityBoundary): | ||
| """Permissive default boundary with legacy `from_config()` compatibility.""" | ||
|
|
||
| @classmethod | ||
| def from_config(cls, config: dict[str, Any] | None) -> PolicySecurityBoundary: |
There was a problem hiding this comment.
Changing dare_framework.security.impl.DefaultSecurityBoundary from an alias of PolicySecurityBoundary to a subclass of the permissive legacy boundary is a behavioral regression for existing impl imports: DefaultSecurityBoundary() now allows high-risk actions by default and no longer accepts policy constructor kwargs (it raises TypeError), whereas before it inherited full policy-gate behavior and constructor compatibility. This can silently disable approval/deny enforcement in deployments that instantiate this symbol from security.impl.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already covered by 2c0cbe6 on this branch. dare_framework.security.impl.DefaultSecurityBoundary is back to the PolicySecurityBoundary alias, the permissive public baseline stays behind the public facade, and tests/unit/test_security_boundary.py now exercises both the from_config(...) and direct constructor paths for the impl import.
Fresh verification:
/Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/python -m pytest -q tests/unit/test_security_boundary.py tests/unit/test_package_initializers_facade_pattern.py->9 passed
Address the PR 213 review finding around semantics. Key changes: - restore as the policy-boundary alias so existing impl imports keep constructor kwargs and approval/deny behavior - add to expose the permissive public baseline boundary without routing the package initializer through - update the public security facade to import from the new defaults module - add regression coverage proving the impl constructor path still behaves like Rationale: The previous refactor conflated the public permissive facade with the historical alias and changed behavior for deployments that import from . This split preserves both compatibility layers with their original semantics.
|
Handled in This preserves the old two-layer contract instead of conflating them:
Verification:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Re-checked the inline security-boundary comment against the current PR head. The branch already carries the Fresh verification:
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Resolve the remaining package initializer conflicts after merging origin/main into codex/pr212-runtime-facades-main.\n\nKeep the facade/defaults exports introduced by this branch in the affected package __init__ modules, retain the incoming mainline updates across the rest of the tree, and verify the combined result with the full pytest suite.
|
Merged the latest Verification:
Conflict-resolution merge commit: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 727b587284
ℹ️ 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() includes task_id and run_id in the checkpoint payload, but deserialize_and_apply() only writes back current_milestone_idx. When a checkpoint is restored into a new or reset session object, the execution resumes with stale identifiers, which can break continuity for metadata keyed by task/run IDs even though those IDs were captured in the snapshot.
Useful? React with 👍 / 👎.
Restore task_id and run_id when SessionStateContributor reapplies serialized session state during checkpoint recovery.\n\nThis preserves the identifiers emitted by serialize(), avoids stale session metadata after restore, and adds a regression test that exercises the direct deserialize path alongside the existing checkpoint defaults coverage.
|
Addressed the latest checkpoint review note from #213 (comment). Fixed in
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51554c8e58
ℹ️ 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"), | ||
| kind=item.get("kind", "chat"), | ||
| text=item.get("text") or item.get("content", ""), |
There was a problem hiding this comment.
StmContributor.serialize() writes the original Message.text value (which can validly be None), but restore uses item.get("text") or item.get("content", ""), so any saved None is coerced to "". This mutates checkpoint round-trips (e.g., tool/result messages created with text=None) and can change downstream behavior that distinguishes missing text from empty text.
Useful? React with 👍 / 👎.
Preserve explicit None message text when StmContributor restores serialized STM payloads during checkpoint recovery.\n\nDifferentiate between checkpoints that intentionally saved text=None and legacy payloads that only exposed content, so tool/result messages round-trip without being coerced into empty strings. Add regression coverage for the null-text checkpoint case alongside the existing checkpoint defaults tests.
|
Addressed the latest STM checkpoint review note from #213 (comment). Fixed in
Verification:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
maindefaults/adaptersmodulesVerification
/Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/python -m pytest -q tests/unit/test_package_initializers_facade_pattern.py tests/unit/test_checkpoint_defaults.py tests/unit/test_security_boundary.py tests/unit/test_transport_typed_payloads.py tests/unit/test_transport_channel.py tests/unit/test_react_agent_gateway_injection.py/Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/python -m pytest -qNotes
main.