feat: Interactive Workspace Standard — typed port, executor + environment seams (#225)#226
Merged
Merged
Conversation
…phase 1) Promote the interactive-tmux send_message/await_completion/capture_response trio out of Workspace._handle: Any into a structural InteractiveSession Protocol plus a promoted AwaitResult dataclass, both defined fresh in agentic_isolation.providers.base (no dependency on the standalone driver, which stays stdlib-only). BaseProvider gains a concrete interactive_session() defaulting to None; InteractiveTmuxProvider overrides it to return workspace._handle, which already satisfies the protocol structurally. Zero behavior change.
… (issue #225 phase 2) Introduce CommandExecutor/ExecResult/DockerExecExecutor in the driver and route docker exec calls through it (default DockerExecExecutor(container) when no executor is injected, so all existing call sites are unchanged). Replace host bind-mount credential staging with exec-based transfer: start_workspace now runs the container without -v mounts and pushes each staged credential file's bytes into the container over the executor via base64-chunked docker exec writes, then chowns/chmods them in-container. This removes the host-tmpdir/bind-mount dependency so the driver works when the caller itself runs inside a container (docker-out-of-docker). Adds InteractiveTmuxWorkspace.executor, defaulting to DockerExecExecutor(self.container) via __post_init__.
…ching (issue #225 phase 3) Every subprocess.run/docker-exec call in the interactive-tmux driver now carries a bounded timeout_s (DEFAULT_EXEC_TIMEOUT_S for docker-exec/tmux ops, DEFAULT_RUN_TIMEOUT_S for docker run/rm) instead of blocking forever on a wedged container. DockerExecExecutor.exec() catches subprocess.TimeoutExpired and returns a timed_out ExecResult rather than propagating. send_message/await_completion/capture_response each take an explicit per-call timeout distinct from await_completion's overall deadline; a single failed/timed-out poll no longer aborts await_completion or wait_for_started, it's logged and retried until the overall deadline. tmux send-keys payloads over ~12KB (TMUX_SEND_KEYS_MAX_BYTES) are staged via a container-side temp file + tmux load-buffer/paste-buffer instead of raw send-keys -l, avoiding tmux's ~16KB send-keys ceiling. Small payloads keep the existing raw path unchanged. No behavior change to the sync/blocking subprocess model (deferred to a future async pass if needed).
…225 phase 4) Pull the generic tmux window operations (send-keys, capture-pane, incremental output diffing, is_alive, session/window start+stop) out of InteractiveTmuxWorkspace into a standalone TmuxSession class that knows nothing about claude/codex/gemini and routes everything through the Phase 2 CommandExecutor seam. InteractiveTmuxWorkspace now builds one TmuxSession per enabled agent in __post_init__ and delegates capture_response/await_completion/the startup wait to it instead of re-deriving (container, window, executor) inline. start_workspace enables agents by iterating the _ADAPTERS registry rather than the closed AGENTS literal tuple, so a 4th agent joins by registering an adapter object, not by editing TmuxSession or the enablement loop. Pure refactor: public API (start_workspace/send_message/await_completion/ capture_response/stop) and all existing tests are unchanged; per-agent adapter static methods keep their existing (container, executor) call signatures for backward compatibility with direct callers/tests. Adds test_interactive_tmux_session.py covering TmuxSession in isolation (no docker/tmux required) plus the registry-driven enablement.
#225 async follow-up) InteractiveTmuxProvider.create()/.destroy() called the blocking interactive-tmux driver (subprocess.run-backed start_workspace/stop) directly on the caller's event loop, freezing all other traffic on that loop for the full container startup/teardown window. Offload both through asyncio.to_thread, serialized via a dedicated asyncio.Lock (_blocking_call_lock) held for the duration of each to_thread call. This directly addresses the documented concern in the prior code comment: an executor thread's subprocess.run racing asyncio's child watcher and returning spurious exit codes from docker exec. Serializing guarantees at most one such offloaded docker run/exec/rm is ever in flight, regardless of how many create/destroy calls run concurrently. Adds regression tests proving create()/destroy() no longer block the loop (a concurrently scheduled asyncio.sleep ticker keeps advancing during the call) and that the lock actually serializes concurrent create() calls (a re-entrancy-detecting driver stub never observes overlap).
…ecutor Add an `Environment` Protocol (start()->CommandExecutor, stop()->None) and extract the existing docker-run/docker-rm logic in start_workspace()/stop() into a `DockerEnvironment`. start_workspace() gains an optional `environment=` kwarg; when omitted it builds a DockerEnvironment exactly as before (no behavior change for existing callers). Rename the `container` field/param threaded through TmuxSession, _docker_exec, _tmux_send_keys, _tmux_send_literal, _tmux_capture, and DockerExecExecutor to the backend-neutral `target` (pure rename, docker exec command shape unchanged), clearing the way for Local/SSH-VPS Environment backends. Issue #225.
…ning (issue #225) Adds LocalExecutor (runs argv directly, no docker prefix, mirrors DockerExecExecutor's timeout handling) and LocalEnvironment (no-op start-up beyond a fail-fast `tmux` PATH check via shutil.which, no-op teardown since it doesn't own the host). Opt-in only via the existing `environment=` param on start_workspace; no default-path wiring.
…oning (issue #225) Adds SSHExecutor (CommandExecutor) and SSHEnvironment (Environment) as a third opt-in provisioning backend alongside Docker and Local. start() fails fast with a `ssh ... true` reachability check before any tmux session is attempted; exec() joins argv via shlex.join, optionally cd-prefixed with workdir, mirroring the timeout/ExecResult handling of DockerExecExecutor/LocalExecutor exactly. stop() is a documented no-op (no persistent ControlMaster connection in this version). Not wired into any default path -- opt-in only via environment=.
Narrow workspace._handle (typed Any) to InteractiveSession via an explicit isinstance check instead of returning Any, raising TypeError on a malformed handle rather than silently trusting the escape hatch.
Line-length wraps from `just qa-fix`, no behavior change.
There was a problem hiding this comment.
Pull request overview
This PR promotes the interactive-tmux workspace from a spike into a standardized “interactive workspace” boundary across the codebase by introducing a typed interactive session port, a command-executor/environment seam for backend-agnostic operation, and reliability hardening (timeouts + large payload handling).
Changes:
- Introduces a typed
InteractiveSessionprotocol (andAwaitResult) inagentic_isolation.providers.base, plusBaseProvider.interactive_session()/ provider override. - Refactors the interactive-tmux driver to route operations through a
CommandExecutorand provisions targets via anEnvironmentabstraction (Docker/Local/SSH), including exec-based credential transfer (no bind mounts). - Adds reliability hardening (bounded subprocess timeouts, tmux large-payload staging) and unit tests covering the new seams and behaviors.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| providers/workspaces/interactive-tmux/driver/interactive_tmux.py | Adds executor/environment seams, bounded timeouts, tmux payload staging, and session extraction. |
| lib/python/agentic_isolation/agentic_isolation/providers/base.py | Adds AwaitResult and InteractiveSession protocol; adds default interactive_session() hook on providers. |
| lib/python/agentic_isolation/agentic_isolation/providers/interactive_tmux/init.py | Updates provider create/destroy to avoid blocking the event loop; exposes interactive_session(). |
| lib/python/agentic_isolation/agentic_isolation/providers/init.py | Re-exports InteractiveSession and AwaitResult from the base provider module. |
| lib/python/agentic_isolation/agentic_isolation/init.py | Re-exports InteractiveSession and AwaitResult at package top-level. |
| lib/python/agentic_isolation/tests/test_interactive_session_protocol.py | Adds tests for AwaitResult + InteractiveSession protocol and provider defaults/override behavior. |
| lib/python/agentic_isolation/tests/test_interactive_tmux_executor.py | Adds tests for executor seam and exec-based credential seeding (no bind mounts). |
| lib/python/agentic_isolation/tests/test_interactive_tmux_reliability.py | Adds tests for bounded timeouts and tmux payload staging behavior. |
| lib/python/agentic_isolation/tests/test_interactive_tmux_session.py | Adds tests for TmuxSession extraction and registry-driven agent enablement. |
| lib/python/agentic_isolation/tests/test_interactive_tmux_environment.py | Adds tests for Environment provisioning seam (Docker/Local/SSH). |
| lib/python/agentic_isolation/tests/test_interactive_tmux_adapter_nonblocking.py | Adds tests ensuring provider create/destroy don’t block the event loop and are serialized. |
| lib/python/agentic_isolation/tests/test_interactive_tmux_pane_tail.py | Updates monkeypatch signature to match _tmux_capture(..., **kwargs) changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1282
to
+1288
| result = executor.exec(command, timeout_s=timeout_s) | ||
| if result.exit_code != 0: | ||
| raise RuntimeError( | ||
| f"container command failed (exit {result.exit_code}): {command!r}\n" | ||
| f"stdout: {result.stdout}\nstderr: {result.stderr}" | ||
| ) | ||
| return result |
Comment on lines
+390
to
+394
| The underlying driver's `InteractiveTmuxWorkspace` handle already | ||
| exposes `send_message`/`await_completion`/`capture_response` with | ||
| signatures compatible with `InteractiveSession`; structural typing | ||
| means it satisfies the protocol as-is, so no wrapper is needed. | ||
| Returns `None` if the workspace has no live handle (e.g. already |
…H argv, honor Docker run timeout, guard create() cancellation Codex cross-review of PR #226 surfaced four real bugs: - _ClaudeAdapter/_CodexAdapter/_GeminiAdapter.launch_in_window() still called the module-level tmux helpers without an executor, so bootstrap bypassed LocalEnvironment/SSHEnvironment entirely and always shelled out via docker exec regardless of which Environment was injected. - SSHEnvironment's base ssh argv had no `--` before `user@host`, letting a hostile host/user string be parsed as an ssh option. - DockerEnvironment.start() ignored the caller's run_timeout_s and used the module default instead. - create() awaited the asyncio.to_thread(start_workspace) call directly; if the caller cancelled create() while the thread was still running, the thread could finish later and leak a live, credentialed container with no owner. Now shielded, with a done-callback that stops any workspace that finishes starting after cancellation. 314 tests passing (up from 310), ruff clean, no new mypy --strict errors on touched lines.
…ce standard Security: - credential seeding pipes the base64 payload via STDIN to in-container base64 -d instead of embedding it in the docker-exec argv (was visible in ps / /proc/*/cmdline to any host user); whole file in one exec - _run_exec_checked takes a redacted label so credential-seeding failures no longer interpolate the payload/command into the raised RuntimeError Correctness / reliability: - add workspace_dir to Workspace.metadata (#225 spec) so downstream artifact collection resolves a path instead of silently collecting none - paste-buffer uses -p (bracketed paste) so multiline prompts >send-keys limit are pasted as one unit instead of newlines submitting early - readiness polls (await_completion / _wait_for_started) detect a dead container and return reason=container_dead immediately instead of spinning the full startup timeout and masking the docker error Concurrency: - narrow the provider lock to only the _workspaces registry dict; the blocking start/stop no longer holds a cross-workspace lock, so multi- agent provisions run concurrently (the guarded child-watcher race cannot occur with subprocess.run in a worker thread) - shield cleanup stop() and catch BaseException so a second cancellation cannot leak a credential-seeded container Types: - replace the duplicated base.AwaitResult dataclass (never the object actually returned) with a runtime_checkable Protocol; drops dict[str, Any] Executor protocol: CommandExecutor.exec gains optional stdin: bytes; docker adds -i only when stdin is supplied. Refs #225
…er death Codex review of the fix commit: _container_death_reason matched 'cannot connect to the Docker daemon' and 'error connecting to', which signal a transient daemon/socket outage while the container is very likely still running - not a dead container. Classifying them as death aborted a live workspace on a blip. Drop both markers so they stay on the retry path (bounded by the overall deadline); keep the unambiguous death markers (no such container / is not running / tmux server+session gone). Adds a regression test: a daemon-outage capture error retries and recovers instead of returning reason=container_dead. Refs #225
Comment on lines
+622
to
+625
| cmd = ["docker", "exec", target, *args] | ||
| logger.debug("exec: %s", _redact_cmd(cmd)) | ||
| exec_ = executor or DockerExecExecutor(target) | ||
| result = exec_.exec(list(args), timeout_s=timeout_s) |
Comment on lines
+1433
to
+1436
| raise RuntimeError( | ||
| f"container command failed (exit {result.exit_code}): {described}\n" | ||
| f"stdout: {result.stdout}\nstderr: {result.stderr}" | ||
| ) |
Comment on lines
+62
to
+68
| Exposes only the read surface consumers need: | ||
|
|
||
| - ready=True -> adapter is_ready() held stable | ||
| - timed_out=True, ready=False -> deadline hit before idle | ||
| - ready=False, reason="never_ready" -> never reached even one ready frame | ||
| - ready=False, reason="unstable" -> ready frames seen but pane kept changing | ||
|
|
Comment on lines
+408
to
+409
| workdir: str | Path | ||
| require_tools: Sequence[str] = ("tmux",) |
Comment on lines
1778
to
+1812
| @@ -843,28 +1790,36 @@ def start_workspace( | |||
| if not enabled: | |||
| raise ValueError("start_workspace called with no enabled agents (host_auth empty)") | |||
|
|
|||
| # Run the container with bind mounts (each mount is a -v arg). | |||
| run_cmd = [ | |||
| "docker", | |||
| "run", | |||
| "-d", | |||
| "--name", | |||
| container, | |||
| "--workdir", | |||
| workdir, | |||
| ] | |||
| # Provision the workspace's backing target WITHOUT credential | |||
| # bind mounts (Phase 2: docker-out-of-docker fix). Bind-mounting | |||
| # `-v host:container` requires the *outer* docker daemon to | |||
| # resolve `host` on its own filesystem — that breaks when this | |||
| # driver itself runs inside a container, where the throwaway | |||
| # staging dir is only visible to the driver's own mount | |||
| # namespace, not the sibling daemon's. Instead, credentials are | |||
| # pushed into the running target below via the executor | |||
| # `Environment.start()` returns (see `_transfer_path_to_ | |||
| # container`), which always targets the right backend | |||
| # regardless of where the driver process lives. | |||
| if environment is None: | |||
| environment = DockerEnvironment(name=container, image=image, workdir=workdir) | |||
| executor: CommandExecutor = environment.start() | |||
|
|
|||
| # Target is up; transfer each prepared credential file/dir | |||
| # into it over the executor seam instead of bind-mounting. | |||
| for host_path, container_path in all_mounts: | |||
| run_cmd.extend(["-v", f"{host_path}:{container_path}"]) | |||
| run_cmd.extend([image, "sleep", "infinity"]) | |||
| _run(run_cmd) | |||
| _transfer_path_to_container(executor, host_path, container_path) | |||
| _secure_container_path(executor, container_path, is_dir=host_path.is_dir()) | |||
Comment on lines
+622
to
+630
| cmd = ["docker", "exec", target, *args] | ||
| logger.debug("exec: %s", _redact_cmd(cmd)) | ||
| exec_ = executor or DockerExecExecutor(target) | ||
| result = exec_.exec(list(args), timeout_s=timeout_s) | ||
| if check and result.exit_code != 0: | ||
| raise subprocess.CalledProcessError( | ||
| result.exit_code, cmd, result.stdout, result.stderr | ||
| ) | ||
| return subprocess.CompletedProcess(cmd, result.exit_code, result.stdout, result.stderr) |
Comment on lines
+1676
to
+1683
| def __post_init__(self) -> None: | ||
| if self.executor is None: | ||
| self.executor = DockerExecExecutor(self.container) | ||
| self._sessions = { | ||
| agent: TmuxSession(target=self.container, window=_ADAPTERS[agent].window, executor=self.executor) | ||
| for agent in self.enabled_agents | ||
| if agent in _ADAPTERS | ||
| } |
Comment on lines
+1433
to
+1436
| raise RuntimeError( | ||
| f"container command failed (exit {result.exit_code}): {described}\n" | ||
| f"stdout: {result.stdout}\nstderr: {result.stderr}" | ||
| ) |
Comment on lines
+62
to
+68
| Exposes only the read surface consumers need: | ||
|
|
||
| - ready=True -> adapter is_ready() held stable | ||
| - timed_out=True, ready=False -> deadline hit before idle | ||
| - ready=False, reason="never_ready" -> never reached even one ready frame | ||
| - ready=False, reason="unstable" -> ready frames seen but pane kept changing | ||
|
|
Comment on lines
116
to
120
| - timed_out=True, ready=False → deadline hit before idle | ||
| - ready=False, reason="never_ready" → never reached even one ready frame | ||
| - ready=False, reason="unstable" → ready frames seen but pane kept changing | ||
| - ready=False, reason="container_dead"→ target died mid-poll (not a timeout) | ||
| - ready=True → adapter is_ready() held stable |
Live integration surfaced a shutil.copytree failure staging ~/.codex: 'Operation not supported on socket' on vendor_imports/.../.git/fsmonitor--daemon.ipc, a Unix socket left by a running git fsmonitor daemon. copy2 cannot copy sockets/FIFOs/devices and aborts the whole credential stage. - add _ignore_uncopyable: a copytree ignore filter that skips non-regular, non-dir, non-symlink entries (sockets, FIFOs, devices) - apply it to the codex and gemini auth-dir copytree calls (claude copies only .credentials.json, no tree) Tests: a FIFO in a staged dir is skipped; a normal tree copies unchanged. Refs #225
…dirs
Live integration surfaced a ~4-minute codex auth stage that overloaded the
docker daemon ('Cannot connect to the Docker daemon' mid-mkdir): the codex
stage was copying ~/.codex/plugins/cache/.../node_modules/... - thousands of
dirs - one 'docker exec mkdir' per directory. Those are plugin/dependency
caches, never auth material.
- _ignore_uncopyable also skips .git and node_modules dirs at any depth
(defense in depth; .git is also where the fsmonitor socket lived)
- codex top-level skip set adds 'plugins'
Auth staging now copies only real auth (auth.json/config.toml/sessions),
staying fast and daemon-safe. Refs #225
Comment on lines
117
to
121
| - timed_out=True, ready=False → deadline hit before idle | ||
| - ready=False, reason="never_ready" → never reached even one ready frame | ||
| - ready=False, reason="unstable" → ready frames seen but pane kept changing | ||
| - ready=False, reason="container_dead"→ target died mid-poll (not a timeout) | ||
| - ready=True → adapter is_ready() held stable |
| ready: bool | ||
| timed_out: bool | ||
| reason: str # "ready" | "timeout_never_ready" | "timeout_unstable" | "error" | ||
| reason: str # "ready" | "timeout_never_ready" | "timeout_unstable" | "container_dead" | "error" |
Comment on lines
+623
to
+626
| cmd = ["docker", "exec", target, *args] | ||
| logger.debug("exec: %s", _redact_cmd(cmd)) | ||
| exec_ = executor or DockerExecExecutor(target) | ||
| result = exec_.exec(list(args), timeout_s=timeout_s) |
| if result.exit_code != 0: | ||
| described = label if label is not None else repr(command) | ||
| raise RuntimeError( | ||
| f"container command failed (exit {result.exit_code}): {described}\n" |
Comment on lines
+64
to
+67
| - ready=True -> adapter is_ready() held stable | ||
| - timed_out=True, ready=False -> deadline hit before idle | ||
| - ready=False, reason="never_ready" -> never reached even one ready frame | ||
| - ready=False, reason="unstable" -> ready frames seen but pane kept changing |
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
Closes #225
Promotes the interactive-tmux workspace from a spike to the standard interactive workspace boundary, per the spec in #225, plus two follow-ups identified during review.
InteractiveSessionprotocol + promotedAwaitResultinagentic_isolation;BaseProvider.interactive_session()(defaultsNone) /InteractiveTmuxProvideroverride replacesWorkspace._handle: Anyas the documented consumer-facing seam.CommandExecutorprotocol +DockerExecExecutor; all tmux ops and credential seeding now route through it. Host bind-mount credential staging replaced with exec-based, base64-chunked transfer — the docker-out-of-docker fix.send-keys -l.TmuxSessionextracted from the driver; per-agent adapters (claude/codex/gemini) now sit on a registry above it instead of being baked into the driver.create()/destroy()no longer block the event loop (offloaded viaasyncio.to_thread, serialized behind a lock to guard the previously-documented docker subprocess/child-watcher race).Environmentprotocol (start()/stop()) aboveCommandExecutorso provisioning itself is pluggable, not just command execution.DockerEnvironment(default, zero behavior change),LocalEnvironment(host-native, no docker), andSSHEnvironment(remote VPS, fail-fast reachability check) ship as the first three backends.containerrenamed totargetthroughout the affected helpers to stop implying Docker-only.mypy --strictgap surfaced during review:interactive_session()now narrowsworkspace._handlevia an explicitisinstancecheck instead of returningAny.Acceptance criteria (from #225)
import agentic_isolationsucceeds with no driver tree present (lazy driver load, already true pre-PR, re-verified).Anyin the consumer-facing seam — one narrow pre-existingAnyremains onWorkspace._handleitself at the base-class level (intentional escape hatch); the protocol surface is fully typed.docker execsurfaces as a bounded timeout, with dedicated test coverage.agentic-workspace-interactive-tmux:latestround-trip) was not run in this environment (needs a built image + authenticated CLIs) — code review + the full unit/executor/reliability test suite give confidence, but this should be re-run in CI or manually before merge if that gate exists.Test plan
cd lib/python/agentic_isolation && uv run pytest -q --ignore=tests/integration— 310 passed, 0 failedmypy --strictclean on all files touched this PR (pre-existing unrelated errors in untouched files, e.g.claude_cli/*,workspace_files.py, left as-is — out of scope)ruff check/ruff formatclean on all touched filesEnvironmentsmoke test against a real box (unit-tested with mocks only so far)