Repository navigation
feat: add execution binding foundation - #75
Merged
Merged
Conversation
Record the Phase 2 boundary after Phase 1 PG persistence and add an implementation checklist for owner, lease, fencing, routing, and failover work in the HTTP session stack. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Keep the workspace resolver stable on SessionManager and cover the missing cloud binding failures. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughIntroduces a first-class ExecutionBinding abstraction (local/cloud) and a BindingResolver; persists bindings on Session with backward-compatible defaults; adds a PostgreSQL-backed session owner store with atomic acquire/renew/release semantics and enforces ownership in session runtime paths. Adds ADRs, implementation plan, and tests covering bindings and owner store behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant SM as SessionManager
participant SOS as SessionOwnerStore
participant BR as BindingResolver
participant CA as create_agent
SM->>SOS: _assert_owner(session_id)
SOS-->>SM: owner_id, fencing_token / success or conflict
SM->>BR: resolve_workspace_root(execution_binding)
BR-->>SM: workspace_root (Path)
SM->>BR: resolve_tool_config(execution_binding)
BR-->>SM: tool_config (dict)
SM->>CA: create_agent(workspace_root, tool_config)
CA-->>SM: agent instance
sequenceDiagram
participant SM as SessionManager
participant PG as PGSessionOwnerStore
participant DB as PostgreSQL
SM->>PG: acquire(session_id, owner_id, lease_ttl)
PG->>DB: INSERT/UPDATE session_owners (atomic)
DB-->>PG: fencing_token / result
PG-->>SM: success or conflict
SM->>PG: renew(session_id, owner_id, lease_ttl)
PG->>DB: conditional UPDATE lease_expires_at
DB-->>PG: renewed or stale
PG-->>SM: success or stale error
SM->>PG: release(session_id, owner_id)
PG->>DB: conditional DELETE
DB-->>PG: released
PG-->>SM: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a first-class “execution binding” abstraction for sessions, persists it in session metadata (with legacy fallback from repo_path), and routes runtime creation/rehydration/restore through a binding resolver with targeted regression tests.
Changes:
- Add
ExecutionBindingdataclasses (local + cloud skeleton) plusDefaultBindingResolver. - Persist
execution_bindinginSessionstore data and resolveworkspace_rootfrom bindings inrun_agent,ensure_session_runtime, and checkpoint restore. - Add/extend UI tests to cover binding round-trips, legacy fallback behavior, and runtime workspace root resolution.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
coding-agent/src/coding_agent/ui/session_manager.py |
Persists/rehydrates execution_binding, resolves workspace root via resolver in runtime paths. |
coding-agent/src/coding_agent/ui/execution_binding.py |
Adds typed binding models and (de)serialization helpers. |
coding-agent/src/coding_agent/ui/binding_resolver.py |
Adds resolver interface + default local resolver (cloud unimplemented). |
coding-agent/tests/ui/test_execution_binding.py |
New tests for binding (de)serialization and resolver behavior. |
coding-agent/tests/ui/test_session_manager_public_api.py |
Adds session metadata persistence tests for execution bindings + legacy fallback. |
coding-agent/tests/ui/test_session_manager_runtime.py |
Adds runtime/restore tests to ensure workspace_root comes from bindings; adjusts task cancellation assertions. |
coding-agent/docs/adr/0014-separate-session-execution-binding-from-runtime-ownership.md |
ADR documenting the binding abstraction decision and acceptance criteria. |
coding-agent/docs/adr/0013-define-phase2-multi-instance-session-ownership-for-pg-http-sessions.md |
ADR documenting Phase 2 ownership/lease/fencing direction. |
coding-agent/docs/pg-phase2-multi-instance-checklist.md |
Checklist for multi-instance ownership implementation. |
coding-agent/docs/superpowers/plans/2026-04-19-session-execution-binding-and-thin-phase2.md |
Detailed implementation plan for execution binding + Phase 2 substrate. |
coding-agent/.opencode/prompts/session-execution-binding-phase2-task-packet.md |
Task packet prompt for implementing this work incrementally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+187
to
+190
| workspace_root = ( | ||
| repo_path_raw if repo_path_raw is not None else str(Path.cwd()) | ||
| ) | ||
| execution_binding = LocalExecutionBinding(workspace_root=workspace_root) |
There was a problem hiding this comment.
When hydrating legacy session metadata without an explicit execution_binding, the fallback workspace_root uses repo_path_raw or str(Path.cwd()) without resolving to an absolute path. This can yield relative workspace roots (and inconsistent behavior vs create_session, which resolves repo_path/cwd). Consider resolving (e.g., Path(...).resolve()) for both repo_path_raw and cwd before constructing LocalExecutionBinding, and keep repo_path consistent if you still persist it for backward-compat.
Comment on lines
+26
to
+28
| - Add an `ExecutionBinding` dataclass/protocol in `coding_agent.ui.execution_binding` with at least two concrete variants: | ||
| - `LocalExecutionBinding(workspace_root: Path)` for local filesystem mode. | ||
| - `CloudWorkspaceBinding(workspace_url: str, workspace_id: str)` for remote workspace mode. |
There was a problem hiding this comment.
ADR-0014 states LocalExecutionBinding(workspace_root: Path), but the implementation uses workspace_root: str (and the binding resolver converts it to Path). Please align the ADR wording with the code (or change the code to store a Path) to avoid misleading future implementers about the binding type and serialization expectations.
Comment on lines
+352
to
+353
| fake_task = FakeTask() | ||
| session.task = cast(asyncio.Task[None], cast(object, fake_task)) |
There was a problem hiding this comment.
The double cast cast(asyncio.Task[None], cast(object, fake_task)) is redundant and makes the test harder to read. A single cast to asyncio.Task (or assigning to a typed variable + type: ignore[assignment]) should be sufficient while still keeping fake_task available for assertions.
Comment on lines
+382
to
+383
| fake_task = FakeTask() | ||
| session.task = cast(asyncio.Task[None], cast(object, fake_task)) |
There was a problem hiding this comment.
Same as above: the extra cast(object, fake_task) is redundant here and reduces readability. Prefer a single cast (or a targeted type ignore) while keeping the separate fake_task variable for assertions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
coding-agent/src/coding_agent/ui/session_manager.py (1)
181-190: Consider adding a warning when falling back toPath.cwd().When deserializing a session that has neither
execution_bindingnorrepo_path, the code falls back toPath.cwd(). This could silently produce unexpected behavior if the session is restored in a different working directory than where it was created.Consider logging a warning in this edge case to aid debugging:
💡 Proposed enhancement
else: + import logging + _logger = logging.getLogger(__name__) workspace_root = ( repo_path_raw if repo_path_raw is not None else str(Path.cwd()) ) + if repo_path_raw is None: + _logger.warning( + "Session %s has no execution_binding or repo_path; defaulting to cwd", + data.get("id"), + ) execution_binding = LocalExecutionBinding(workspace_root=workspace_root)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/ui/session_manager.py` around lines 181 - 190, When deserializing in session_manager, if both execution_binding is missing and repo_path_raw is None the code falls back to Path.cwd() without notice; update the branch that constructs LocalExecutionBinding(workspace_root=workspace_root) to emit a warning via the logger (e.g., use self.logger.warning or module logger) indicating we are falling back to Path.cwd() and include context (session id or user if available), referencing execution_binding, repo_path_raw, and LocalExecutionBinding so callers can locate and review the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/tests/ui/test_execution_binding.py`:
- Around line 13-52: Replace hardcoded "/tmp/repo" and the endswith check with
pytest's tmp_path fixture and assert exact resolved path: in
test_local_binding_round_trip and test_local_resolver_returns_absolute_path
create the LocalExecutionBinding using tmp_path / "repo" (cast to str), use that
same exact path when asserting restored.workspace_root and when comparing
resolver.resolve_workspace_root(binding) (convert to str) and
resolver.resolve_tool_config(binding) to ensure precise equality; also update
test_local_binding_requires_string_workspace_root to use tmp_path for
consistency if needed.
In `@coding-agent/tests/ui/test_session_manager_public_api.py`:
- Around line 161-204: Replace hardcoded "/tmp/..." paths with temporary
directories created at test runtime: in
test_session_metadata_round_trips_local_execution_binding, create a temp dir
(e.g., with tempfile.TemporaryDirectory() or pytest tmp_path) and pass its path
to LocalExecutionBinding(workspace_root=tmp_dir); in
test_session_metadata_defaults_missing_binding_to_local_from_repo_path, create a
temp dir and use its path for the stored "repo_path" value instead of
"/tmp/legacy-repo". Ensure the temp dir is cleaned up (use context manager or
pytest fixture) so tests remain deterministic and avoid Ruff S108.
In `@coding-agent/tests/ui/test_session_manager_runtime.py`:
- Around line 1032-1144: Replace hardcoded "/tmp" paths in the tests with
pytest's tmp_path (or a tempdir) so Ruff S108 is avoided: update
test_run_agent_uses_resolved_workspace_root_from_binding and
test_restore_checkpoint_preserves_execution_binding to accept a tmp_path fixture
and set session.repo_path and LocalExecutionBinding(workspace_root=...) to
str(tmp_path / "not-used-directly") / str(tmp_path / "bound-workspace")
(respectively), and update the assertions to compare
captured_kwargs["workspace_root"] and session.execution_binding.workspace_root
against Path(tmp_path / "...").resolve() or the string form as appropriate;
reference LocalExecutionBinding, SessionManager.run_agent, and
SessionManager._restore_checkpoint to locate changes.
---
Nitpick comments:
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 181-190: When deserializing in session_manager, if both
execution_binding is missing and repo_path_raw is None the code falls back to
Path.cwd() without notice; update the branch that constructs
LocalExecutionBinding(workspace_root=workspace_root) to emit a warning via the
logger (e.g., use self.logger.warning or module logger) indicating we are
falling back to Path.cwd() and include context (session id or user if
available), referencing execution_binding, repo_path_raw, and
LocalExecutionBinding so callers can locate and review the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f3cc185f-538e-4cd5-9570-62850016b4dc
📒 Files selected for processing (11)
coding-agent/.opencode/prompts/session-execution-binding-phase2-task-packet.mdcoding-agent/docs/adr/0013-define-phase2-multi-instance-session-ownership-for-pg-http-sessions.mdcoding-agent/docs/adr/0014-separate-session-execution-binding-from-runtime-ownership.mdcoding-agent/docs/pg-phase2-multi-instance-checklist.mdcoding-agent/docs/superpowers/plans/2026-04-19-session-execution-binding-and-thin-phase2.mdcoding-agent/src/coding_agent/ui/binding_resolver.pycoding-agent/src/coding_agent/ui/execution_binding.pycoding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/ui/test_execution_binding.pycoding-agent/tests/ui/test_session_manager_public_api.pycoding-agent/tests/ui/test_session_manager_runtime.py
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This was referenced Apr 21, 2026
This was referenced May 2, 2026
This was referenced May 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
repo_pathTest Plan
uv run pytest tests/ui/test_execution_binding.py -vuv run pytest tests/ui/test_session_manager_public_api.py -k "binding" -vuv run pytest tests/ui/test_session_manager_runtime.py -k "binding" -vSummary by CodeRabbit
New Features
Documentation
Tests