From 81dd7804c5e7c8e91ef3bbbbce7b7f43dc3201ed Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Sat, 12 Sep 2026 21:41:54 +0300 Subject: [PATCH 1/8] feat: isolate coding agent processes by room --- docs/acp.md | 20 +- docs/adapters/codex.md | 2 +- examples/acp/clients/bridge_architecture.py | 3 +- examples/acp/clients/cursor.py | 2 +- examples/acp/clients/generic.py | 3 +- examples/acp/clients/rich_streaming.py | 3 +- src/band/adapters/codex.py | 191 +++++++++++++++----- src/band/integrations/acp/__init__.py | 5 +- src/band/integrations/acp/client_adapter.py | 96 ++++++---- 9 files changed, 228 insertions(+), 97 deletions(-) diff --git a/docs/acp.md b/docs/acp.md index 45493ea97..16c31b042 100644 --- a/docs/acp.md +++ b/docs/acp.md @@ -12,7 +12,7 @@ Two-layer pattern (mirrors A2A Gateway): | Platform Bridge | `BandACPServerAdapter` | `ACPClientAdapter` | **Server**: Editor -> ACP -> `ACPServer` -> `BandACPServerAdapter` -> Band REST/WS -> Peers -**Client**: Band room message -> `ACPClientAdapter` -> stdio subprocess **or** TCP connection (Codex, Claude Code, Cursor, GitHub Copilot, etc.) +**Client**: Band room message -> `ACPClientAdapter` -> its room-owned stdio subprocess (Codex, Claude Code, Cursor, GitHub Copilot, etc.) ## Key Files @@ -20,7 +20,7 @@ Two-layer pattern (mirrors A2A Gateway): |------|---------| | `src/band/integrations/acp/server.py` | `ACPServer` — handles ACP JSON-RPC methods, does not subclass `acp.Agent`; `run_acp_server` — runs it with `use_unstable_protocol` (required for `session/fork`, `session/resume`, `session/close`) | | `src/band/integrations/acp/server_adapter.py` | `BandACPServerAdapter` — REST client, room/session mapping | -| `src/band/integrations/acp/client_adapter.py` | `ACPClientAdapter` — drives a remote ACP agent over stdio-spawn or TCP-connect | +| `src/band/integrations/acp/client_adapter.py` | `ACPClientAdapter` — drives a room-owned ACP agent over stdio | | `src/band/integrations/acp/client_runtime.py` | `ACPRuntime` (transport-agnostic) + `ACPCollectingClient` (session_update parsing / coalescing / collapse / live sink), `tcp_spawn_process` (TCP connect seam) | | `src/band/integrations/acp/room_emitter.py` | `RoomTurnEmitter` — posts a turn's chunks to the room in causal order; `turn_replied_in_room` (text-fallback suppression) | | `src/band/adapters/copilot_acp.py` | `CopilotACPAdapter` — thin `ACPClientAdapter` for the GitHub Copilot CLI | @@ -108,16 +108,14 @@ acp = ["agent-client-protocol"] Install with: `pip install band-sdk[acp]` or `uv add band-sdk[acp]` -## Client transports (stdio / TCP) +## Client workspace isolation -`ACPClientAdapter` selects a transport at construction; both flow through `ACPRuntime`'s -injectable `spawn_process` seam, so the runtime and downstream code are transport-agnostic. - -- **stdio** (default): pass `command=[...]` to spawn the agent as a subprocess - (`acp.spawn_agent_process`). -- **TCP**: pass `host=` + `port=` to connect to an already-running ACP server - (`tcp_spawn_process` → `asyncio.open_connection` → `acp.connect_to_agent`). Use for an - ACP agent in a remote/containerized environment. +`ACPClientAdapter` requires `workspace_for_room`, a resolver returning an absolute +workspace for each Band room. It lazily starts one stdio agent process per room and +stops that process when the room is cleaned up. TCP and custom transport injection are +rejected because they cannot prove that a remote process belongs to only one room. +The assigned working directory is not an operating-system sandbox; configure the agent's +sandbox policy separately when that boundary is required. - Exactly one of `{command, (host, port)}` is required (validated in `__init__`). - Advanced: inject a custom `spawn_process` (e.g. `docker exec -i … copilot --acp`, ssh, or a fake in tests). Tests inject a fake through this seam rather than patching module diff --git a/docs/adapters/codex.md b/docs/adapters/codex.md index 0b02d57ab..ce18b521b 100644 --- a/docs/adapters/codex.md +++ b/docs/adapters/codex.md @@ -45,7 +45,7 @@ from band.adapters.codex import CodexAdapter, CodexAdapterConfig adapter = CodexAdapter( config=CodexAdapterConfig( - cwd=os.getcwd(), + workspace_for_room=lambda room_id: os.path.join("/workspaces", room_id), model="gpt-5.5", ), ) diff --git a/examples/acp/clients/bridge_architecture.py b/examples/acp/clients/bridge_architecture.py index 0ad428918..052fea5ff 100644 --- a/examples/acp/clients/bridge_architecture.py +++ b/examples/acp/clients/bridge_architecture.py @@ -45,6 +45,7 @@ from __future__ import annotations import asyncio +import os import logging import shlex @@ -91,7 +92,7 @@ async def main() -> None: adapter = ACPClientAdapter( command=command, - cwd=cwd, + workspace_for_room=lambda room_id: os.path.join(cwd, room_id), inject_band_tools=inject_band_tools, auth_method=auth_method, profile=profile, diff --git a/examples/acp/clients/cursor.py b/examples/acp/clients/cursor.py index bc6160b20..9b584f341 100644 --- a/examples/acp/clients/cursor.py +++ b/examples/acp/clients/cursor.py @@ -98,7 +98,7 @@ async def main() -> None: # - Band tools are injected through a local localhost-only MCP server adapter = ACPClientAdapter( command=[os.path.expanduser("~/.local/bin/agent"), "acp"], - cwd=cwd, + workspace_for_room=lambda room_id: os.path.join(cwd, room_id), env=cursor_env or None, inject_band_tools=True, auth_method="cursor_login", diff --git a/examples/acp/clients/generic.py b/examples/acp/clients/generic.py index 614161682..4bc1d7361 100644 --- a/examples/acp/clients/generic.py +++ b/examples/acp/clients/generic.py @@ -37,6 +37,7 @@ from __future__ import annotations import asyncio +import os import logging import shlex @@ -83,7 +84,7 @@ async def main() -> None: # Create adapter pointing to remote ACP agent adapter = ACPClientAdapter( command=acp_command, - cwd=acp_cwd, + workspace_for_room=lambda room_id: os.path.join(acp_cwd, room_id), ) logger.info( diff --git a/examples/acp/clients/rich_streaming.py b/examples/acp/clients/rich_streaming.py index 3f843e9ff..be2c3af10 100644 --- a/examples/acp/clients/rich_streaming.py +++ b/examples/acp/clients/rich_streaming.py @@ -49,6 +49,7 @@ from __future__ import annotations import asyncio +import os import logging import shlex @@ -95,7 +96,7 @@ async def main() -> None: # Create adapter pointing to remote ACP agent adapter = ACPClientAdapter( command=acp_command, - cwd=acp_cwd, + workspace_for_room=lambda room_id: os.path.join(acp_cwd, room_id), ) logger.info("Starting ACP client bridge with rich streaming...") diff --git a/src/band/adapters/codex.py b/src/band/adapters/codex.py index 9ad53f155..d68741600 100644 --- a/src/band/adapters/codex.py +++ b/src/band/adapters/codex.py @@ -8,7 +8,8 @@ import os import time as _time from collections import OrderedDict -from dataclasses import dataclass +from contextvars import ContextVar +from dataclasses import dataclass, field as dataclass_field from datetime import datetime, timezone from typing import ClassVar, Any, Callable, Literal, NamedTuple, Protocol @@ -32,7 +33,6 @@ from band.integrations.codex import ( CodexJsonRpcError, CodexStdioClient, - CodexWebSocketClient, RpcEvent, ) from band.integrations.codex.types import ( @@ -244,6 +244,19 @@ class TurnResult: saw_send_message_tool: bool = False +@dataclass +class RoomCodexClient: + """The process and protocol state owned by one Band room.""" + + workspace: str + client: CodexClientProtocol | None = None + initialized: bool = False + selected_model: str | None = None + reasoning_effort: str | None = None + reasoning_summary: str | None = None + rpc_lock: asyncio.Lock = dataclass_field(default_factory=asyncio.Lock) + + class CodexAdapterConfig(BaseSettings): """Runtime configuration for Codex adapter sessions. @@ -279,6 +292,7 @@ class CodexAdapterConfig(BaseSettings): extra="forbid", env_ignore_empty=True, populate_by_name=True, + arbitrary_types_allowed=True, ) transport: TransportKind = "stdio" @@ -288,6 +302,7 @@ class CodexAdapterConfig(BaseSettings): ) = None reasoning_summary: Literal["auto", "concise", "detailed", "none"] | None = None cwd: str = Field(default_factory=os.getcwd) + workspace_for_room: Callable[[str], str] | None = Field(default=None, exclude=True) approval_policy: str = "never" personality: Literal["friendly", "pragmatic", "none"] = "pragmatic" sandbox: str | None = None @@ -417,14 +432,25 @@ def __init__( self._custom_tools: list[CustomToolDef] = list(additional_tools or []) if self.config.enable_self_config_tools: self._custom_tools.extend(self._build_self_config_tools()) + if self.config.workspace_for_room is None: + raise ValueError("workspace_for_room is required for Codex room isolation") + if self.config.transport != "stdio": + raise ValueError("only stdio Codex transport guarantees room process isolation") + if client_factory is not None: + raise ValueError("custom Codex clients cannot guarantee room process isolation") self._client_factory = client_factory - self._client: CodexClientProtocol | None = None - self._initialized = False - self._selected_model: str | None = None + self._room_clients: dict[str, RoomCodexClient] = {} + self._active_room: ContextVar[str | None] = ContextVar( + "codex_active_room", default=None + ) + self._fallback_client: CodexClientProtocol | None = None + self._fallback_initialized = False + self._fallback_selected_model: str | None = None self._system_prompt: str = "" self._room_threads: dict[str, str] = {} self._prompt_injected_rooms: set[str] = set() - self._task_titles_by_id: OrderedDict[str, str] = OrderedDict() + self._fallback_task_titles: OrderedDict[str, str] = OrderedDict() + self._room_task_titles: dict[str, OrderedDict[str, str]] = {} self._max_task_titles: int = _MAX_TASK_TITLES self._pending_approvals: dict[str, dict[str, PendingApproval]] = {} self._raw_history_by_room: dict[str, list[dict[str, Any]]] = {} @@ -445,7 +471,78 @@ def __init__( # up to ``approval_wait_timeout_s`` (300s default). Approval resolution # commands (/approve, /decline) are handled *outside* this lock in # ``on_message`` so they can unblock a waiting turn. - self._rpc_lock = asyncio.Lock() + self._fallback_rpc_lock = asyncio.Lock() + + def _room_client(self, room_id: str) -> RoomCodexClient: + room = self._room_clients.get(room_id) + if room is None: + workspace = self.config.workspace_for_room(room_id) # type: ignore[misc] + if not isinstance(workspace, str) or not os.path.isabs(workspace): + raise ValueError("workspace_for_room must return an absolute path") + room = RoomCodexClient(workspace=workspace) + self._room_clients[room_id] = room + return room + + def _active_client_state(self) -> RoomCodexClient | None: + room_id = self._active_room.get() + return self._room_clients.get(room_id) if room_id is not None else None + + def _require_active_client_state(self) -> RoomCodexClient: + state = self._active_client_state() + if state is None: + raise RuntimeError("Codex operation requires a room context") + return state + + @property + def _task_titles_by_id(self) -> OrderedDict[str, str]: + room_id = self._active_room.get() + if room_id is None: + return self._fallback_task_titles + return self._room_task_titles.setdefault(room_id, OrderedDict()) + + @property + def _client(self) -> CodexClientProtocol | None: + state = self._active_client_state() + return state.client if state is not None else self._fallback_client + + @_client.setter + def _client(self, value: CodexClientProtocol | None) -> None: + state = self._active_client_state() + if state is not None: + state.client = value + else: + self._fallback_client = value + + @property + def _initialized(self) -> bool: + state = self._active_client_state() + return state.initialized if state is not None else self._fallback_initialized + + @_initialized.setter + def _initialized(self, value: bool) -> None: + state = self._active_client_state() + if state is not None: + state.initialized = value + else: + self._fallback_initialized = value + + @property + def _selected_model(self) -> str | None: + state = self._active_client_state() + return state.selected_model if state is not None else self._fallback_selected_model + + @_selected_model.setter + def _selected_model(self, value: str | None) -> None: + state = self._active_client_state() + if state is not None: + state.selected_model = value + else: + self._fallback_selected_model = value + + @property + def _rpc_lock(self) -> asyncio.Lock: + state = self._active_client_state() + return state.rpc_lock if state is not None else self._fallback_rpc_lock def _build_self_config_tools(self) -> list[CustomToolDef]: """Build custom tools that let Codex change its own model/reasoning. @@ -460,7 +557,6 @@ def _build_self_config_tools(self) -> list[CustomToolDef]: def _handle_set_model(inp: SetModelInput) -> str: if not adapter._rpc_lock.locked(): raise RuntimeError("_handle_set_model must run under _rpc_lock") - adapter.config.model = inp.model adapter._selected_model = inp.model return f"Model changed to {inp.model} for subsequent turns." @@ -474,7 +570,7 @@ def _handle_set_reasoning(inp: SetReasoningInput) -> str: f"Invalid reasoning effort '{inp.effort}'. " f"Valid: {', '.join(sorted(_REASONING_EFFORTS))}." ) - adapter.config.reasoning_effort = inp.effort # type: ignore[assignment] # Literal narrowed by Pydantic validation + adapter._require_active_client_state().reasoning_effort = inp.effort parts.append(f"effort={inp.effort}") if inp.summary is not None: if inp.summary not in _REASONING_SUMMARIES: @@ -482,7 +578,7 @@ def _handle_set_reasoning(inp: SetReasoningInput) -> str: f"Invalid reasoning summary '{inp.summary}'. " f"Valid: {', '.join(sorted(_REASONING_SUMMARIES))}." ) - adapter.config.reasoning_summary = inp.summary # type: ignore[assignment] # Literal narrowed by Pydantic validation + adapter._require_active_client_state().reasoning_summary = inp.summary parts.append(f"summary={inp.summary}") if not parts: return ( @@ -499,8 +595,6 @@ def _handle_set_reasoning(inp: SetReasoningInput) -> str: async def on_started(self, agent_name: str, agent_description: str) -> None: await super().on_started(agent_name, agent_description) self._build_system_prompt() - async with self._rpc_lock: - await self._ensure_client_ready() self._log_startup_config(agent_name) def _log_startup_config(self, agent_name: str) -> None: @@ -564,6 +658,8 @@ async def on_message( is_session_bootstrap: bool, room_id: str, ) -> None: + self._room_client(room_id) + self._active_room.set(room_id) command = self._extract_local_command(msg.content) if command is not None and command[0] in { "approve", @@ -963,14 +1059,11 @@ async def _process_turn_events( # Also drop token-usage entries keyed by the dead thread # ids; otherwise they leak until the last-room teardown # because on_cleanup can no longer resolve their keys. - stale_rooms = list(self._room_threads.keys()) - stale_threads = list(self._room_threads.values()) - self._room_threads.clear() - self._raw_history_by_room.clear() - for stale_thread in stale_threads: + stale_thread = self._room_threads.pop(room_id, None) + self._raw_history_by_room.pop(room_id, None) + if stale_thread: self._token_usage.pop(stale_thread, None) - for stale_room in stale_rooms: - self._clear_pending_approvals_for_room(stale_room) + self._clear_pending_approvals_for_room(room_id) break if event.method == "turn/completed": @@ -1017,11 +1110,10 @@ async def _process_turn_events( return result async def on_cleanup(self, room_id: str) -> None: - # NOTE: _rpc_lock is adapter-wide, so cleanup for room B blocks if - # room A holds the lock during a pending manual approval (up to - # approval_wait_timeout_s). This is a known limitation of the single- - # client architecture — the lock serializes all turn processing and - # cleanup across rooms. + room = self._room_clients.get(room_id) + if room is None: + return + self._active_room.set(room_id) async with self._rpc_lock: thread_id = self._room_threads.pop(room_id, None) if thread_id: @@ -1033,9 +1125,9 @@ async def on_cleanup(self, room_id: str) -> None: self._approval_audit.pop(room_id, None) self._session_approved.pop(room_id, None) self._sandbox_overrides.pop(room_id, None) - if self._room_threads: - return + self._room_task_titles.pop(room_id, None) if self._client is None: + self._room_clients.pop(room_id, None) return try: close_coro = self._client.close() @@ -1055,24 +1147,24 @@ async def on_cleanup(self, room_id: str) -> None: self._client = None self._initialized = False self._selected_model = None - self._task_titles_by_id.clear() - # Defensive: wipe all pending approvals globally on last-room - # teardown. Per-room cleanup already resolves futures to - # "decline" via _clear_pending_approvals_for_room, so this - # catches any leaked entries from rooms whose cleanup failed. - self._pending_approvals.clear() - self._token_usage.clear() - self._approval_audit.clear() - self._session_approved.clear() - self._sandbox_overrides.clear() + self._room_clients.pop(room_id, None) + + async def cleanup_all(self) -> None: + """Close every room-owned Codex process during agent shutdown.""" + await asyncio.gather( + *(self.on_cleanup(room_id) for room_id in list(self._room_clients)) + ) async def _ensure_client_ready(self) -> None: if self._client is None: self._client = self._build_client(self.config) + client = self._client + if client is None: + raise RuntimeError("Codex client was not created") if not self._initialized: - await self._client.connect() - await self._client.initialize( + await client.connect() + await client.initialize( client_name=self.config.client_name, client_title=self.config.client_title, client_version=self.config.client_version, @@ -1085,12 +1177,12 @@ def _build_client(self, config: CodexAdapterConfig) -> CodexClientProtocol: if self._client_factory is not None: return self._client_factory(config) - if config.transport == "ws": - return CodexWebSocketClient(ws_url=config.codex_ws_url) - + state = self._active_client_state() + if state is None: + raise RuntimeError("Codex client creation requires a room context") return CodexStdioClient( command=config.codex_command, - cwd=config.cwd, + cwd=state.workspace, env=config.codex_env, ) @@ -2622,7 +2714,7 @@ async def _handle_local_command( ) return True - self.config.model = model_arg + # Model selection belongs to the room's process, never adapter-wide config. self._selected_model = model_arg await tools.send_message( f"Model override set to `{model_arg}` for subsequent turns.", @@ -2647,7 +2739,7 @@ async def _handle_local_command( mentions=mention, ) return True - self.config.reasoning_effort = effort_arg # type: ignore[assignment] # Literal narrowed by Pydantic validation + self._require_active_client_state().reasoning_effort = effort_arg await tools.send_message( f"Reasoning effort set to `{effort_arg}` for subsequent turns.", mentions=mention, @@ -2927,14 +3019,15 @@ def _build_system_prompt(self) -> None: def _apply_turn_overrides( self, params: dict[str, Any], *, room_id: str | None = None ) -> None: + state = self._require_active_client_state() params["model"] = self._selected_model - params["cwd"] = self.config.cwd + params["cwd"] = state.workspace params["approvalPolicy"] = self.config.approval_policy params["personality"] = self.config.personality - if self.config.reasoning_effort is not None: - params["effort"] = self.config.reasoning_effort - if self.config.reasoning_summary is not None: - params["summary"] = self.config.reasoning_summary + if state.reasoning_effort or self.config.reasoning_effort: + params["effort"] = state.reasoning_effort or self.config.reasoning_effort + if state.reasoning_summary or self.config.reasoning_summary: + params["summary"] = state.reasoning_summary or self.config.reasoning_summary self._apply_turn_sandbox(params, room_id=room_id) async def _start_turn(self, params: dict[str, Any]) -> dict[str, Any]: diff --git a/src/band/integrations/acp/__init__.py b/src/band/integrations/acp/__init__.py index 9519a9f97..34a5bc372 100644 --- a/src/band/integrations/acp/__init__.py +++ b/src/band/integrations/acp/__init__.py @@ -33,7 +33,10 @@ from band import Agent from band.integrations.acp import ACPClientAdapter - adapter = ACPClientAdapter(command="codex", cwd="/workspace") + adapter = ACPClientAdapter( + command="codex", + workspace_for_room=lambda room_id: f"/workspace/{room_id}", + ) agent = Agent.create(adapter=adapter, agent_id="...", api_key="...") await agent.run() """ diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index 57666a0e3..ce09a527e 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -108,6 +108,7 @@ def new_message_marker() -> str: # stdio and TCP are the built-in transports; injecting one (e.g. docker exec / ssh, # or a fake in tests) is the supported extension point. SpawnProcess = Callable[..., object] +WorkspaceResolver = Callable[[str], str] def _resolve_launcher(command: list[str]) -> list[str]: @@ -144,6 +145,7 @@ def __init__( command: str | list[str] | None = None, env: dict[str, str] | None = None, cwd: str | None = None, + workspace_for_room: WorkspaceResolver | None = None, mcp_servers: list[dict[str, Any]] | None = None, additional_tools: list[CustomToolDef] | None = None, inject_band_tools: bool = True, @@ -163,10 +165,18 @@ def __init__( history_converter=ACPClientHistoryConverter(), **features, ) + if workspace_for_room is None: + raise ValueError("workspace_for_room is required for ACP client isolation") + if cwd is not None: + raise ValueError("cwd is not supported; use workspace_for_room") + if host is not None or port is not None: + raise ValueError("TCP ACP transport cannot guarantee room process isolation") + if spawn_process is not None: + raise ValueError("custom ACP transports cannot guarantee room process isolation") self._host, self._port = self._resolve_transport(command, host, port) self._command = self._shape_command(command, self._host) self._env = env - self._cwd = os.path.abspath(cwd or ".") + self._workspace_for_room = workspace_for_room self._mcp_servers = list(mcp_servers or []) self._custom_tools: list[CustomToolDef] = list(additional_tools or []) self._tool_definitions, self._own_tool_names = self._registered_tools() @@ -174,7 +184,8 @@ def __init__( self._auth_method = auth_method self._profile = profile self._custom_section = custom_section - self._runtime = self._build_runtime(spawn_process) + self._runtimes: dict[str, ACPRuntime] = {} + self._room_workspaces: dict[str, str] = {} self._room_to_session: dict[str, str] = {} self._room_tools: dict[str, AgentToolsProtocol] = {} @@ -264,7 +275,7 @@ def _select_transport( return tcp_spawn_process(host, port) return spawn_agent_process - def _build_runtime(self, spawn_process: SpawnProcess | None) -> ACPRuntime: + def _build_runtime(self) -> ACPRuntime: return ACPRuntime( command=_resolve_launcher(self._command), env=self._env, @@ -273,9 +284,24 @@ def _build_runtime(self, spawn_process: SpawnProcess | None) -> ACPRuntime: profile=self._profile, canonicalize_tool_name=self._canonical_tool_name, ), - spawn_process=self._select_transport(spawn_process, self._host, self._port), + spawn_process=spawn_agent_process, ) + def _workspace(self, room_id: str) -> str: + workspace = self._workspace_for_room(room_id) + if not isinstance(workspace, str) or not os.path.isabs(workspace): + raise ValueError("workspace_for_room must return an absolute path") + return workspace + + async def _runtime_for(self, room_id: str) -> ACPRuntime: + async with self._session_lock: + runtime = self._runtimes.get(room_id) + if runtime is None: + runtime = self._build_runtime() + self._runtimes[room_id] = runtime + self._room_workspaces[room_id] = self._workspace(room_id) + return runtime + async def on_started(self, agent_name: str, agent_description: str) -> None: await super().on_started(agent_name, agent_description) # The other end of cleanup_all(final=True)'s _stopped: Agent.start() @@ -284,10 +310,6 @@ async def on_started(self, agent_name: str, agent_description: str) -> None: # the backend must be startable again too. async with self._mcp_backend_lock: self._stopped = False - await self._spawn_process() - - async def _spawn_process(self) -> None: - await self._runtime.start(respawn=False) async def on_message( self, @@ -300,7 +322,8 @@ async def on_message( is_session_bootstrap: bool, room_id: str, ) -> None: - await self._ensure_connection() + runtime = await self._runtime_for(room_id) + await self._ensure_connection(runtime) if self._inject_band_tools: async with self._session_lock: @@ -310,7 +333,7 @@ async def on_message( await self._load_persisted_session(room_id, history) session_id, created = await self._get_or_create_session(room_id) - self._runtime.reset_session(session_id) + runtime.reset_session(session_id) # A just-created session holds no remote context (a restored one does), # so seed it with the Band room's transcript. On bootstrap the converter @@ -348,18 +371,18 @@ async def on_message( session_id=session_id, room_id=room_id, ) as emitter: - self._runtime.set_permission_handler( + runtime.set_permission_handler( session_id, self._make_permission_handler(emitter, room_id), ) - await self._runtime.prompt( + await runtime.prompt( session_id=session_id, prompt_text=prompt_text, on_chunk=emitter.emit, ) except Exception as e: logger.exception("ACP agent error: %s", e) - await self.stop() + await self.on_cleanup(room_id) await tools.send_event( content=f"ACP agent error: {e}", message_type="error", @@ -473,10 +496,9 @@ def _build_system_context(self, room_id: str, msg: PlatformMessage) -> str: return f"[System Context]\n{system_prompt}\n{room_context}" def _build_local_mcp_server_config( - self, - local_server: LocalMCPServer, + self, local_server: LocalMCPServer, transport: str ) -> LocalMcpServerConfig: - if self._runtime._agent_mcp_transport == "sse": + if transport == "sse": return SseMcpServer( type="sse", name=BAND_MCP_SERVER_NAME, @@ -535,7 +557,7 @@ async def _ensure_band_mcp_backend(self) -> BandMCPBackend: self._band_mcp_backend = None if self._band_mcp_backend is None: backend = await create_band_mcp_backend( - kind=self._runtime._agent_mcp_transport, + kind="http", tool_definitions=self._tool_definitions, get_tools=self._room_tools.get, additional_tools=self._custom_tools, @@ -543,13 +565,16 @@ async def _ensure_band_mcp_backend(self) -> BandMCPBackend: self._band_mcp_backend = backend return self._band_mcp_backend - async def _get_or_start_band_mcp_server(self) -> LocalMcpServerConfig: + async def _get_or_start_band_mcp_server(self, room_id: str) -> LocalMcpServerConfig: backend = await self._ensure_band_mcp_backend() local_server = backend.local_server if local_server is None: raise RuntimeError("ACP MCP backend did not create a local server") - return self._build_local_mcp_server_config(local_server) + runtime = await self._runtime_for(room_id) + return self._build_local_mcp_server_config( + local_server, runtime._agent_mcp_transport + ) async def _get_or_create_session(self, room_id: str) -> tuple[str, bool]: """This room's ACP session id, plus whether it was created just now. @@ -560,14 +585,14 @@ async def _get_or_create_session(self, room_id: str) -> tuple[str, bool]: if room_id in self._room_to_session: return self._room_to_session[room_id], False + runtime = await self._runtime_for(room_id) + mcp_servers = await self._session_mcp_servers(room_id) async with self._session_lock: if room_id in self._room_to_session: return self._room_to_session[room_id], False - mcp_servers = await self._session_mcp_servers() - - session_id = await self._runtime.create_session( - cwd=self._cwd, + session_id = await runtime.create_session( + cwd=self._room_workspaces[room_id], mcp_servers=mcp_servers, ) self._room_to_session[room_id] = session_id @@ -579,11 +604,11 @@ async def _get_or_create_session(self, room_id: str) -> tuple[str, bool]: ) return session_id, True - async def _session_mcp_servers(self) -> list[object]: + async def _session_mcp_servers(self, room_id: str) -> list[object]: """The MCP configuration supplied when creating or loading a session.""" mcp_servers: list[object] = list(self._mcp_servers) if self._inject_band_tools: - mcp_servers.append(await self._get_or_start_band_mcp_server()) + mcp_servers.append(await self._get_or_start_band_mcp_server(room_id)) return mcp_servers def _claim_session_bootstrap(self, session_id: str) -> bool: @@ -666,6 +691,11 @@ async def on_cleanup(self, room_id: str) -> None: self._room_tools.pop(room_id, None) if session_id: self._bootstrapped_sessions.discard(session_id) + runtime = self._runtimes.pop(room_id, None) + self._room_workspaces.pop(room_id, None) + + if runtime is not None: + await runtime.stop() logger.debug("Cleaned up ACP client resources for room %s", room_id) @@ -688,6 +718,9 @@ async def cleanup_all(self, *, final: bool = True) -> None: self._room_to_session.clear() self._room_tools.clear() self._bootstrapped_sessions.clear() + runtimes = list(self._runtimes.values()) + self._runtimes.clear() + self._room_workspaces.clear() async with self._mcp_backend_lock: backend = self._band_mcp_backend self._band_mcp_backend = None @@ -703,7 +736,7 @@ async def cleanup_all(self, *, final: bool = True) -> None: # None and start a fresh backend while this one is mid-teardown. if backend is not None: await backend.stop() - await self._runtime.stop() + await asyncio.gather(*(runtime.stop() for runtime in runtimes)) logger.info("ACP client adapter stopped") async def stop(self) -> None: @@ -730,10 +763,11 @@ async def _load_persisted_session( if session_id is None: return - loaded = await self._runtime.load_session( - cwd=self._cwd, + runtime = await self._runtime_for(room_id) + loaded = await runtime.load_session( + cwd=self._room_workspaces[room_id], session_id=session_id, - mcp_servers=await self._session_mcp_servers(), + mcp_servers=await self._session_mcp_servers(room_id), ) if not loaded: logger.info( @@ -778,7 +812,7 @@ async def _fetch_replay( raw = messages_before(context.get("data") or [], msg.id) return build_replay_messages([m for m in raw if m.get("id") != msg.id]) - async def _ensure_connection(self) -> ACPConnectionProtocol: - return await self._runtime.ensure_connection( + async def _ensure_connection(self, runtime: ACPRuntime) -> ACPConnectionProtocol: + return await runtime.ensure_connection( can_respawn=bool(self.agent_name), ) From 12f947c1543139f87357aa46bfee53620204763a Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Sun, 13 Sep 2026 07:09:01 +0300 Subject: [PATCH 2/8] fix: harden room-owned coding agent lifecycle --- docs/acp.md | 8 +- docs/adapters/codex.md | 22 ++--- src/band/adapters/codex.py | 72 ++++++++++------- src/band/integrations/acp/client_adapter.py | 80 ++++-------------- .../adapters/test_room_workspace_isolation.py | 81 +++++++++++++++++++ 5 files changed, 152 insertions(+), 111 deletions(-) create mode 100644 tests/adapters/test_room_workspace_isolation.py diff --git a/docs/acp.md b/docs/acp.md index 16c31b042..e5f46b614 100644 --- a/docs/acp.md +++ b/docs/acp.md @@ -115,12 +115,8 @@ workspace for each Band room. It lazily starts one stdio agent process per room stops that process when the room is cleaned up. TCP and custom transport injection are rejected because they cannot prove that a remote process belongs to only one room. The assigned working directory is not an operating-system sandbox; configure the agent's -sandbox policy separately when that boundary is required. -- Exactly one of `{command, (host, port)}` is required (validated in `__init__`). -- Advanced: inject a custom `spawn_process` (e.g. `docker exec -i … copilot --acp`, ssh, - or a fake in tests). Tests inject a fake through this seam rather than patching module - globals (see `tests/integrations/acp/conftest.py::FakeSpawn` / the `make_acp_transport` - fixture). +sandbox policy separately when that boundary is required. The resolver must assign a +different workspace to every live room, and the adapter requires a non-empty stdio command. ## GitHub Copilot CLI backend diff --git a/docs/adapters/codex.md b/docs/adapters/codex.md index ce18b521b..cd8397682 100644 --- a/docs/adapters/codex.md +++ b/docs/adapters/codex.md @@ -1,6 +1,6 @@ # Codex Adapter -[OpenAI Codex](https://openai.com/codex) is a coding agent runtime that can inspect files, edit files, run commands, and manage approval workflows. The Band Codex adapter connects a Codex process to Band rooms over stdio or WebSocket so it can take part in conversations as a coding collaborator. +[OpenAI Codex](https://openai.com/codex) is a coding agent runtime that can inspect files, edit files, run commands, and manage approval workflows. The Band Codex adapter connects one room-owned Codex process to each Band room over stdio. Use this adapter when you want an OpenAI-powered coding agent with configurable sandboxing, approval commands, command/file-change telemetry, reasoning visibility, and task lifecycle events. Use the [Claude SDK adapter](claude_sdk.md) for Claude Code based coding agents, the [Anthropic adapter](anthropic.md) for direct Claude API chat/tool agents, or the [LangGraph adapter](langgraph.md) for custom graph workflows. @@ -26,12 +26,6 @@ You need two credentials or auth contexts: - A Band platform API key for `Agent.create(api_key=...)`. - Codex authentication for the Codex process. Use `codex login`, or set `OPENAI_API_KEY` if that is how your Codex environment is configured. -For `transport="ws"`, start the Codex app server separately: - -```bash -codex app-server --listen ws://127.0.0.1:8765 -``` - Credentials for Band can also be loaded from `agent_config.yaml` with `Agent.from_config("my_agent", adapter=adapter)`. ## Quick Start @@ -65,8 +59,8 @@ asyncio.run(agent.run()) Codex has three setup layers: -- `CodexAdapterConfig(...)` configures the Codex runtime: transport, model, working directory, sandbox, approval behavior, prompts, context injection, and streaming/telemetry detail. -- `CodexAdapter(...)` wraps that runtime config for Band and adds adapter-level settings: feature flags, custom tools, history conversion, and advanced client injection. +- `CodexAdapterConfig(...)` configures the Codex runtime: a room workspace resolver, model, sandbox, approval behavior, prompts, context injection, and streaming/telemetry detail. +- `CodexAdapter(...)` wraps that runtime config for Band and adds adapter-level settings: feature flags, custom tools, and history conversion. - `Agent.create(...)` connects the configured adapter to Band. Use it for the Band agent identity, Band API key, platform URLs, session settings, contact-event handling, callbacks, and preprocessing. Codex authentication is handled by `codex login`, `OPENAI_API_KEY`, or the Codex process environment. `Agent.create(api_key=...)` is only the Band platform key. @@ -104,7 +98,10 @@ from band.core.types import Emit from band.adapters.codex import CodexAdapter, CodexAdapterConfig adapter = CodexAdapter( - config=CodexAdapterConfig(cwd="/repo", sandbox="workspace-write"), + config=CodexAdapterConfig( + workspace_for_room=lambda room_id: f"/workspaces/{room_id}", + sandbox="workspace-write", + ), emit=Emit.TOOL_CALLS | Emit.TASK_EVENTS, ) ``` @@ -115,12 +112,11 @@ Pass these to `CodexAdapterConfig(...)`: | Parameter | Type | Default | Description | |-----------|------|---------|-------------| -| `transport` | `"stdio" \| "ws"` | `"stdio"` | How the adapter connects to Codex. Use `"stdio"` to spawn a process, or `"ws"` to connect to `codex app-server`. | +| `workspace_for_room` | `Callable[[str], str]` | required | Returns the absolute workspace for a room. Every live room must receive a distinct workspace. | | `model` | `str \| None` | `None` | Model to use. When unset, the adapter asks Codex for visible models and uses the first visible model, or the adapter default if discovery fails or returns no usable model. | | `reasoning_effort` | `"none" \| "minimal" \| "low" \| "medium" \| "high" \| "xhigh" \| None` | `None` | Reasoning effort for models that support it. | | `reasoning_summary` | `"auto" \| "concise" \| "detailed" \| "none" \| None` | `None` | How Codex summarizes reasoning in responses. | | `personality` | `"friendly" \| "pragmatic" \| "none"` | `"pragmatic"` | Codex response style. | -| `cwd` | `str \| None` | `None` | Working directory for Codex sessions. | | `turn_timeout_s` | `float` | `180.0` | Maximum seconds to wait for one Codex turn. | ### Safety, Sandbox, and Approvals @@ -170,7 +166,6 @@ Pass these to `CodexAdapterConfig(...)`: |-----------|------|---------|-------------| | `codex_command` | `tuple[str, ...] \| None` | `None` | Custom command used to launch Codex for stdio transport. | | `codex_env` | `dict[str, str] \| None` | `None` | Extra environment variables for the Codex process. | -| `codex_ws_url` | `str` | `"ws://127.0.0.1:8765"` | WebSocket URL for `transport="ws"`. | | `experimental_api` | `bool` | `True` | Use experimental Codex API features. | | `enable_self_config_tools` | `bool` | `False` | Expose tools that let Codex change its own model and reasoning settings. Use only in trusted rooms. | | `additional_dynamic_tools` | `list[dict]` | `[]` | Extra dynamic tool schemas registered with the Codex client. | @@ -187,7 +182,6 @@ Pass these directly to `CodexAdapter(...)`: | `capabilities` | `Capability \| Iterable[Capability] \| None` | none | Optional Band tool categories exposed to the model. Opt-in: omitted, defaults to empty. | | `additional_tools` | `list[CustomToolDef] \| None` | `None` | Custom tools as `(PydanticModel, callable)` tuples. | | `history_converter` | `CodexHistoryConverter \| None` | auto | Advanced escape hatch for replacing the default history/thread-metadata converter. | -| `client_factory` | callable | `None` | Test/advanced injection point for a custom Codex client. | ## Feature flags: Capabilities and Emit diff --git a/src/band/adapters/codex.py b/src/band/adapters/codex.py index d68741600..754a9fdf6 100644 --- a/src/band/adapters/codex.py +++ b/src/band/adapters/codex.py @@ -438,18 +438,14 @@ def __init__( raise ValueError("only stdio Codex transport guarantees room process isolation") if client_factory is not None: raise ValueError("custom Codex clients cannot guarantee room process isolation") - self._client_factory = client_factory self._room_clients: dict[str, RoomCodexClient] = {} + self._workspace_rooms: dict[str, str] = {} self._active_room: ContextVar[str | None] = ContextVar( "codex_active_room", default=None ) - self._fallback_client: CodexClientProtocol | None = None - self._fallback_initialized = False - self._fallback_selected_model: str | None = None self._system_prompt: str = "" self._room_threads: dict[str, str] = {} self._prompt_injected_rooms: set[str] = set() - self._fallback_task_titles: OrderedDict[str, str] = OrderedDict() self._room_task_titles: dict[str, OrderedDict[str, str]] = {} self._max_task_titles: int = _MAX_TASK_TITLES self._pending_approvals: dict[str, dict[str, PendingApproval]] = {} @@ -471,7 +467,6 @@ def __init__( # up to ``approval_wait_timeout_s`` (300s default). Approval resolution # commands (/approve, /decline) are handled *outside* this lock in # ``on_message`` so they can unblock a waiting turn. - self._fallback_rpc_lock = asyncio.Lock() def _room_client(self, room_id: str) -> RoomCodexClient: room = self._room_clients.get(room_id) @@ -479,8 +474,15 @@ def _room_client(self, room_id: str) -> RoomCodexClient: workspace = self.config.workspace_for_room(room_id) # type: ignore[misc] if not isinstance(workspace, str) or not os.path.isabs(workspace): raise ValueError("workspace_for_room must return an absolute path") + workspace = os.path.realpath(workspace) + owner = self._workspace_rooms.get(workspace) + if owner is not None and owner != room_id: + raise ValueError( + f"workspace_for_room assigned {workspace!r} to both {owner!r} and {room_id!r}" + ) room = RoomCodexClient(workspace=workspace) self._room_clients[room_id] = room + self._workspace_rooms[workspace] = room_id return room def _active_client_state(self) -> RoomCodexClient | None: @@ -497,52 +499,57 @@ def _require_active_client_state(self) -> RoomCodexClient: def _task_titles_by_id(self) -> OrderedDict[str, str]: room_id = self._active_room.get() if room_id is None: - return self._fallback_task_titles + raise RuntimeError("Codex task state requires a room context") return self._room_task_titles.setdefault(room_id, OrderedDict()) @property def _client(self) -> CodexClientProtocol | None: state = self._active_client_state() - return state.client if state is not None else self._fallback_client + if state is None: + raise RuntimeError("Codex client requires a room context") + return state.client @_client.setter def _client(self, value: CodexClientProtocol | None) -> None: state = self._active_client_state() - if state is not None: - state.client = value - else: - self._fallback_client = value + if state is None: + raise RuntimeError("Codex client requires a room context") + state.client = value @property def _initialized(self) -> bool: state = self._active_client_state() - return state.initialized if state is not None else self._fallback_initialized + if state is None: + raise RuntimeError("Codex client requires a room context") + return state.initialized @_initialized.setter def _initialized(self, value: bool) -> None: state = self._active_client_state() - if state is not None: - state.initialized = value - else: - self._fallback_initialized = value + if state is None: + raise RuntimeError("Codex client requires a room context") + state.initialized = value @property def _selected_model(self) -> str | None: state = self._active_client_state() - return state.selected_model if state is not None else self._fallback_selected_model + if state is None: + raise RuntimeError("Codex client requires a room context") + return state.selected_model @_selected_model.setter def _selected_model(self, value: str | None) -> None: state = self._active_client_state() - if state is not None: - state.selected_model = value - else: - self._fallback_selected_model = value + if state is None: + raise RuntimeError("Codex client requires a room context") + state.selected_model = value @property def _rpc_lock(self) -> asyncio.Lock: state = self._active_client_state() - return state.rpc_lock if state is not None else self._fallback_rpc_lock + if state is None: + raise RuntimeError("Codex client requires a room context") + return state.rpc_lock def _build_self_config_tools(self) -> list[CustomToolDef]: """Build custom tools that let Codex change its own model/reasoning. @@ -622,7 +629,7 @@ def _log_startup_config(self, agent_name: str) -> None: "diffs=%s, token_usage=%s, structured_errors=%s", agent_name, self.config.transport, - self._selected_model or self.config.model or "auto", + self.config.model or "auto", self.config.sandbox or "default", self.config.approval_mode, Emit.TOOL_CALLS in self.features.emit, @@ -1128,6 +1135,7 @@ async def on_cleanup(self, room_id: str) -> None: self._room_task_titles.pop(room_id, None) if self._client is None: self._room_clients.pop(room_id, None) + self._workspace_rooms.pop(room.workspace, None) return try: close_coro = self._client.close() @@ -1148,6 +1156,7 @@ async def on_cleanup(self, room_id: str) -> None: self._initialized = False self._selected_model = None self._room_clients.pop(room_id, None) + self._workspace_rooms.pop(room.workspace, None) async def cleanup_all(self) -> None: """Close every room-owned Codex process during agent shutdown.""" @@ -1162,7 +1171,9 @@ async def _ensure_client_ready(self) -> None: client = self._client if client is None: raise RuntimeError("Codex client was not created") - if not self._initialized: + if self._initialized: + return + try: await client.connect() await client.initialize( client_name=self.config.client_name, @@ -1172,11 +1183,16 @@ async def _ensure_client_ready(self) -> None: ) self._selected_model = await self._select_model() self._initialized = True + except Exception: + if self._client is client: + self._client = None + try: + await client.close() + except Exception: + logger.debug("Failed to close unsuccessfully initialized Codex client", exc_info=True) + raise def _build_client(self, config: CodexAdapterConfig) -> CodexClientProtocol: - if self._client_factory is not None: - return self._client_factory(config) - state = self._active_client_state() if state is None: raise RuntimeError("Codex client creation requires a room context") diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index ce09a527e..1466638ee 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -33,7 +33,6 @@ allow_permission, cancel_permission, select_allow_option_id, - tcp_spawn_process, ) from band.integrations.acp.client_types import ( ACPClientSessionState, @@ -173,8 +172,9 @@ def __init__( raise ValueError("TCP ACP transport cannot guarantee room process isolation") if spawn_process is not None: raise ValueError("custom ACP transports cannot guarantee room process isolation") - self._host, self._port = self._resolve_transport(command, host, port) - self._command = self._shape_command(command, self._host) + if not command: + raise ValueError("ACP stdio transport requires a command") + self._command = [command] if isinstance(command, str) else list(command) self._env = env self._workspace_for_room = workspace_for_room self._mcp_servers = list(mcp_servers or []) @@ -186,6 +186,7 @@ def __init__( self._custom_section = custom_section self._runtimes: dict[str, ACPRuntime] = {} self._room_workspaces: dict[str, str] = {} + self._workspace_rooms: dict[str, str] = {} self._room_to_session: dict[str, str] = {} self._room_tools: dict[str, AgentToolsProtocol] = {} @@ -207,21 +208,6 @@ def apply_effective_features(self, features: AdapterFeatures) -> None: super().apply_effective_features(features) self._tool_definitions, self._own_tool_names = self._registered_tools() - @staticmethod - def _shape_command(command: str | list[str] | None, host: str | None) -> list[str]: - """The subprocess command for stdio, or an empty command for TCP. - - stdio spawns a subprocess from ``command``; TCP dials an - already-running ACP server at ``host``/port instead. ``host`` is - passed explicitly (not read off ``self``) so this stays checkable - independent of ``__init__``'s statement order. - """ - if host is not None: - return [] - # _resolve_transport guarantees command is set when host is None. - assert command is not None - return [command] if isinstance(command, str) else list(command) - def _registered_tools(self) -> tuple[list[ToolDefinition], frozenset[str]]: """The tools this adapter registers on the loopback MCP server. @@ -260,21 +246,6 @@ def _registered_tools(self) -> tuple[list[ToolDefinition], frozenset[str]]: ) return definitions, names - @staticmethod - def _select_transport( - spawn_process: SpawnProcess | None, host: str | None, port: int | None - ) -> SpawnProcess: - """An explicit ``spawn_process`` wins (advanced/custom transports and - tests); otherwise acp's subprocess spawner (stdio) or a connect-only - seam closed over host/port (TCP; see ``tcp_spawn_process``). ``host``/ - ``port`` are explicit (not read off ``self``), matching - ``_shape_command``.""" - if spawn_process is not None: - return spawn_process - if host is not None and port is not None: - return tcp_spawn_process(host, port) - return spawn_agent_process - def _build_runtime(self) -> ACPRuntime: return ACPRuntime( command=_resolve_launcher(self._command), @@ -291,15 +262,22 @@ def _workspace(self, room_id: str) -> str: workspace = self._workspace_for_room(room_id) if not isinstance(workspace, str) or not os.path.isabs(workspace): raise ValueError("workspace_for_room must return an absolute path") - return workspace + return os.path.realpath(workspace) async def _runtime_for(self, room_id: str) -> ACPRuntime: async with self._session_lock: runtime = self._runtimes.get(room_id) if runtime is None: + workspace = self._workspace(room_id) + owner = self._workspace_rooms.get(workspace) + if owner is not None and owner != room_id: + raise ValueError( + f"workspace_for_room assigned {workspace!r} to both {owner!r} and {room_id!r}" + ) runtime = self._build_runtime() self._runtimes[room_id] = runtime - self._room_workspaces[room_id] = self._workspace(room_id) + self._room_workspaces[room_id] = workspace + self._workspace_rooms[workspace] = room_id return runtime async def on_started(self, agent_name: str, agent_description: str) -> None: @@ -435,33 +413,6 @@ async def handler( return handler - @staticmethod - def _resolve_transport( - command: str | list[str] | None, - host: str | None, - port: int | None, - ) -> tuple[str | None, int | None]: - """Validate exactly one transport is configured; return (host, port) for TCP. - - stdio spawns a subprocess from ``command``; TCP connects to an - already-running ACP server at ``host``/``port``. The two are mutually - exclusive and one is required. - """ - # An empty command ("" or []) is not a usable stdio transport — treat it as - # absent so it fails the "one is required" check below with a clear error, - # rather than slipping through to crash at spawn time. - has_command = bool(command) - has_tcp = host is not None or port is not None - if has_command and has_tcp: - raise ValueError( - "Provide either command (stdio) or host+port (TCP), not both" - ) - if not has_command and not has_tcp: - raise ValueError("Provide either command (stdio) or host+port (TCP)") - if has_tcp and (host is None or port is None): - raise ValueError("TCP transport requires both host and port") - return (host, port) if has_tcp else (None, None) - def _build_system_context(self, room_id: str, msg: PlatformMessage) -> str: agent_name = self.agent_name or "Agent" agent_desc = self.agent_description or "An AI assistant" @@ -692,7 +643,9 @@ async def on_cleanup(self, room_id: str) -> None: if session_id: self._bootstrapped_sessions.discard(session_id) runtime = self._runtimes.pop(room_id, None) - self._room_workspaces.pop(room_id, None) + workspace = self._room_workspaces.pop(room_id, None) + if workspace is not None: + self._workspace_rooms.pop(workspace, None) if runtime is not None: await runtime.stop() @@ -721,6 +674,7 @@ async def cleanup_all(self, *, final: bool = True) -> None: runtimes = list(self._runtimes.values()) self._runtimes.clear() self._room_workspaces.clear() + self._workspace_rooms.clear() async with self._mcp_backend_lock: backend = self._band_mcp_backend self._band_mcp_backend = None diff --git a/tests/adapters/test_room_workspace_isolation.py b/tests/adapters/test_room_workspace_isolation.py new file mode 100644 index 000000000..073906386 --- /dev/null +++ b/tests/adapters/test_room_workspace_isolation.py @@ -0,0 +1,81 @@ +"""Room-owned workspace guards for coding-agent adapters.""" + +from __future__ import annotations + +import pytest + +from band.adapters.codex import CodexAdapter, CodexAdapterConfig +from band.integrations.acp.client_adapter import ACPClientAdapter + + +def test_codex_rejects_a_workspace_shared_by_live_rooms() -> None: + adapter = CodexAdapter( + CodexAdapterConfig(workspace_for_room=lambda _room_id: "/workspace") + ) + + adapter._room_client("room-a") + + with pytest.raises(ValueError, match="both 'room-a' and 'room-b'"): + adapter._room_client("room-b") + + +@pytest.mark.asyncio +async def test_acp_retries_workspace_resolution_after_a_failure() -> None: + attempts = 0 + + def workspace_for_room(_room_id: str) -> str: + nonlocal attempts + attempts += 1 + if attempts == 1: + raise ValueError("workspace provisioning failed") + return "/workspace/room-a" + + adapter = ACPClientAdapter(command="codex", workspace_for_room=workspace_for_room) + + with pytest.raises(ValueError, match="workspace provisioning failed"): + await adapter._runtime_for("room-a") + + runtime = await adapter._runtime_for("room-a") + + assert adapter._runtimes["room-a"] is runtime + assert adapter._room_workspaces == {"room-a": "/workspace/room-a"} + + +@pytest.mark.asyncio +async def test_acp_rejects_a_workspace_shared_by_live_rooms() -> None: + adapter = ACPClientAdapter( + command="codex", workspace_for_room=lambda _room_id: "/workspace" + ) + + await adapter._runtime_for("room-a") + + with pytest.raises(ValueError, match="both 'room-a' and 'room-b'"): + await adapter._runtime_for("room-b") + + +@pytest.mark.asyncio +async def test_codex_discards_a_client_after_startup_failure( + monkeypatch: pytest.MonkeyPatch, +) -> None: + class FailingClient: + closed = False + + async def connect(self) -> None: + raise RuntimeError("startup failed") + + async def close(self) -> None: + self.closed = True + + client = FailingClient() + adapter = CodexAdapter( + CodexAdapterConfig(workspace_for_room=lambda _room_id: "/workspace/room-a") + ) + adapter._room_client("room-a") + adapter._active_room.set("room-a") + monkeypatch.setattr(adapter, "_build_client", lambda _config: client) + + with pytest.raises(RuntimeError, match="startup failed"): + await adapter._ensure_client_ready() + + assert client.closed + assert adapter._client is None From 62cecd13f15a62fbcdcc315cddeaf64d265d1161 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Sun, 13 Sep 2026 07:23:03 +0300 Subject: [PATCH 3/8] fix: enforce room workspace isolation end to end --- docs/adapters/codex.md | 10 +++- examples/codex/01_basic_agent.py | 17 ++++--- examples/codex/02_tom_agent.py | 10 +++- examples/codex/03_jerry_agent.py | 10 +++- examples/docker_demo/agents/dev/main.py | 11 ++++- examples/run_agent.py | 29 ++++++----- src/band/adapters/codex.py | 6 ++- src/band/integrations/acp/client_adapter.py | 6 +-- .../adapters/test_room_workspace_isolation.py | 48 +++++++++++++++++++ 9 files changed, 116 insertions(+), 31 deletions(-) diff --git a/docs/adapters/codex.md b/docs/adapters/codex.md index cd8397682..bf0ab8ad1 100644 --- a/docs/adapters/codex.md +++ b/docs/adapters/codex.md @@ -211,7 +211,10 @@ from band.core.types import Capability, Emit from band.adapters.codex import CodexAdapter, CodexAdapterConfig adapter = CodexAdapter( - config=CodexAdapterConfig(model="gpt-5.5"), + config=CodexAdapterConfig( + workspace_for_room=lambda room_id: f"/workspaces/{room_id}", + model="gpt-5.5", + ), capabilities=Capability.CONTACTS | Capability.MEMORY, emit=Emit.TOOL_CALLS | Emit.THOUGHTS | Emit.TASK_EVENTS, ) @@ -290,7 +293,10 @@ def get_weather(args: WeatherInput) -> str: adapter = CodexAdapter( - config=CodexAdapterConfig(model="gpt-5.5"), + config=CodexAdapterConfig( + workspace_for_room=lambda room_id: f"/workspaces/{room_id}", + model="gpt-5.5", + ), additional_tools=[(WeatherInput, get_weather)], ) ``` diff --git a/examples/codex/01_basic_agent.py b/examples/codex/01_basic_agent.py index 37f01f086..9133d2869 100644 --- a/examples/codex/01_basic_agent.py +++ b/examples/codex/01_basic_agent.py @@ -13,17 +13,14 @@ Prerequisites: 1. OAuth login: codex login -2. For stdio mode (default), no extra process is needed. -3. For ws mode, start app-server separately: - codex app-server --listen ws://127.0.0.1:8765 +2. The adapter starts one local stdio process for each Band room. Run: uv run examples/codex/01_basic_agent.py Optional env overrides: AGENT_KEY=darter - CODEX_TRANSPORT=stdio|ws - CODEX_WS_URL=ws://127.0.0.1:8765 + CODEX_WORKSPACE_ROOT=.band-workspaces CODEX_ROLE=coding|planner|reviewer CODEX_MODEL=gpt-5.5 CODEX_APPROVAL_MODE=manual|auto_accept|auto_decline @@ -34,6 +31,7 @@ import asyncio import logging +import os from pathlib import Path from dotenv import load_dotenv @@ -65,6 +63,12 @@ class Settings(BaseSettings): codex_role: str = "" +def workspace_for_room(room_id: str) -> str: + workspace = Path(os.getenv("CODEX_WORKSPACE_ROOT", ".band-workspaces")) / room_id + workspace.mkdir(parents=True, exist_ok=True) + return str(workspace.resolve()) + + async def main() -> None: load_dotenv() settings = Settings() @@ -84,11 +88,12 @@ async def main() -> None: "Role '%s' specified but no prompt file at %s", codex_role, prompt_file ) - # transport/codex_ws_url/model/cwd/approval_policy/approval_mode/ + # model/approval_policy/approval_mode/ # emit_turn_task_markers all self-source from CODEX_* env vars (see module # docstring) when omitted here. adapter = CodexAdapter( config=CodexAdapterConfig( + workspace_for_room=workspace_for_room, personality="pragmatic", custom_section=custom_section, include_base_instructions=True, diff --git a/examples/codex/02_tom_agent.py b/examples/codex/02_tom_agent.py index 1e93f4949..6303993ac 100644 --- a/examples/codex/02_tom_agent.py +++ b/examples/codex/02_tom_agent.py @@ -29,6 +29,7 @@ import logging import os import sys +from pathlib import Path from dotenv import load_dotenv @@ -54,12 +55,19 @@ logger = logging.getLogger(__name__) +def workspace_for_room(room_id: str) -> str: + workspace = Path(os.getenv("CODEX_WORKSPACE_ROOT", ".band-workspaces")) / room_id + workspace.mkdir(parents=True, exist_ok=True) + return str(workspace.resolve()) + + async def main() -> None: load_dotenv() - # cwd/model self-source from CODEX_CWD/CODEX_MODEL when omitted here. + # model self-sources from CODEX_MODEL when omitted here. adapter = CodexAdapter( config=CodexAdapterConfig( + workspace_for_room=workspace_for_room, transport="stdio", personality="none", custom_section=generate_tom_prompt("Tom"), diff --git a/examples/codex/03_jerry_agent.py b/examples/codex/03_jerry_agent.py index ee1cd8b1a..969f451a8 100644 --- a/examples/codex/03_jerry_agent.py +++ b/examples/codex/03_jerry_agent.py @@ -29,6 +29,7 @@ import logging import os import sys +from pathlib import Path from dotenv import load_dotenv @@ -54,12 +55,19 @@ logger = logging.getLogger(__name__) +def workspace_for_room(room_id: str) -> str: + workspace = Path(os.getenv("CODEX_WORKSPACE_ROOT", ".band-workspaces")) / room_id + workspace.mkdir(parents=True, exist_ok=True) + return str(workspace.resolve()) + + async def main() -> None: load_dotenv() - # cwd/model self-source from CODEX_CWD/CODEX_MODEL when omitted here. + # model self-sources from CODEX_MODEL when omitted here. adapter = CodexAdapter( config=CodexAdapterConfig( + workspace_for_room=workspace_for_room, transport="stdio", personality="none", custom_section=generate_jerry_prompt("Jerry"), diff --git a/examples/docker_demo/agents/dev/main.py b/examples/docker_demo/agents/dev/main.py index 971d891c7..43e3fdf8d 100644 --- a/examples/docker_demo/agents/dev/main.py +++ b/examples/docker_demo/agents/dev/main.py @@ -45,6 +45,12 @@ def build_persona() -> str: return f"{persona}\n\n{CONVERSATION_DISCIPLINE}" +def workspace_for_room(room_id: str) -> str: + workspace = Path(".band-workspaces") / room_id + workspace.mkdir(parents=True, exist_ok=True) + return str(workspace.resolve()) + + def expose_llm_key() -> None: """Copy the sbx-injected placeholder into the var the codex CLI reads. @@ -82,7 +88,10 @@ async def main() -> None: config = DevConfig() adapter = CodexAdapter( config=CodexAdapterConfig( - model=config.model, approval_policy="never", custom_section=build_persona() + workspace_for_room=workspace_for_room, + model=config.model, + approval_policy="never", + custom_section=build_persona(), ), # Emit tool_call/tool_result and reasoning to the room, keeping the default # per-turn task markers but excluding usage events. Codex's Band tools diff --git a/examples/run_agent.py b/examples/run_agent.py index fb37065f0..2a72f528f 100644 --- a/examples/run_agent.py +++ b/examples/run_agent.py @@ -32,8 +32,7 @@ uv run python examples/run_agent.py --example crewai uv run python examples/run_agent.py --example crewai --streaming # Show tool calls uv run python examples/run_agent.py --example codex - uv run python examples/run_agent.py --example codex --agent darter --codex-transport stdio - uv run python examples/run_agent.py --example codex --agent darter --codex-transport ws --codex-ws-url ws://127.0.0.1:8765 + uv run python examples/run_agent.py --example codex --agent darter uv run python examples/run_agent.py --example a2a --a2a-url http://localhost:10000 # A2A bridge uv run python examples/run_agent.py --example a2a_gateway # A2A Gateway (exposes peers) uv run python examples/run_agent.py --example a2a_gateway --gateway-port 8080 # Custom port @@ -446,7 +445,6 @@ async def run_codex_agent( api_key: str, custom_section: str, codex_transport: str, - codex_ws_url: str, codex_model: str | None, codex_personality: str, codex_approval_policy: str, @@ -461,17 +459,25 @@ async def run_codex_agent( from band.adapters import CodexAdapter # noqa: PLC0415 -- only load the adapters extra when this example is the one selected to run from band.adapters.codex import CodexAdapterConfig # noqa: PLC0415 -- only load the codex extra when this example is the one selected to run + workspace_root = Path(codex_cwd).resolve() + + def workspace_for_room(room_id: str) -> str: + workspace = (workspace_root / room_id).resolve() + if workspace_root not in workspace.parents: + raise ValueError("room id cannot escape the Codex workspace root") + workspace.mkdir(parents=True, exist_ok=True) + return str(workspace) + adapter = CodexAdapter( config=CodexAdapterConfig( transport=codex_transport, # type: ignore[arg-type] # str from CLI args, validated at runtime - cwd=codex_cwd, + workspace_for_room=workspace_for_room, model=codex_model, personality=codex_personality, # type: ignore[arg-type] # str from CLI args, validated at runtime approval_policy=codex_approval_policy, approval_mode=codex_approval_mode, # type: ignore[arg-type] # str from CLI args, validated at runtime sandbox=codex_sandbox, reasoning_effort=codex_reasoning_effort, # type: ignore[arg-type] # str from CLI args, validated at runtime - codex_ws_url=codex_ws_url, custom_section=custom_section, include_base_instructions=True, emit_turn_task_markers=codex_turn_task_markers, @@ -482,7 +488,7 @@ async def run_codex_agent( ) logger.info( - "Starting Codex agent (transport=%s, model=%s, cwd=%s)", + "Starting Codex agent (transport=%s, model=%s, workspace_root=%s)", codex_transport, codex_model or "auto", codex_cwd, @@ -775,7 +781,6 @@ async def main() -> None: uv run python examples/run_agent.py --example codex # Codex app-server adapter uv run python examples/run_agent.py --example codex --agent darter # Run Codex as darter agent uv run python examples/run_agent.py --example codex --codex-transport stdio - uv run python examples/run_agent.py --example codex --codex-transport ws --codex-ws-url ws://127.0.0.1:8765 uv run python examples/run_agent.py --example a2a # A2A bridge (default: localhost:10000) uv run python examples/run_agent.py --example a2a --debug # A2A with debug logging (context_id tracing) uv run python examples/run_agent.py --example a2a --a2a-url http://remote:8080 # A2A with custom URL @@ -859,15 +864,10 @@ async def main() -> None: ) parser.add_argument( "--codex-transport", - choices=["stdio", "ws"], + choices=["stdio"], default="stdio", help="Codex transport mode (default: stdio)", ) - parser.add_argument( - "--codex-ws-url", - default=os.getenv("CODEX_WS_URL", "ws://127.0.0.1:8765"), - help="Codex WebSocket URL when --codex-transport=ws", - ) parser.add_argument( "--codex-role", default=None, @@ -887,7 +887,7 @@ async def main() -> None: parser.add_argument( "--codex-cwd", default=os.getcwd(), - help="Working directory given to Codex app-server (default: current directory)", + help="Root directory for per-room Codex workspaces (default: current directory)", ) parser.add_argument( "--codex-reasoning-effort", @@ -1105,7 +1105,6 @@ async def main() -> None: api_key=api_key, custom_section=codex_custom, codex_transport=args.codex_transport, - codex_ws_url=args.codex_ws_url, codex_model=args.codex_model, codex_personality=args.codex_personality, codex_approval_policy=args.codex_approval_policy, diff --git a/src/band/adapters/codex.py b/src/band/adapters/codex.py index 754a9fdf6..746eb0509 100644 --- a/src/band/adapters/codex.py +++ b/src/band/adapters/codex.py @@ -301,7 +301,7 @@ class CodexAdapterConfig(BaseSettings): Literal["none", "minimal", "low", "medium", "high", "xhigh"] | None ) = None reasoning_summary: Literal["auto", "concise", "detailed", "none"] | None = None - cwd: str = Field(default_factory=os.getcwd) + cwd: str | None = None workspace_for_room: Callable[[str], str] | None = Field(default=None, exclude=True) approval_policy: str = "never" personality: Literal["friendly", "pragmatic", "none"] = "pragmatic" @@ -434,6 +434,8 @@ def __init__( self._custom_tools.extend(self._build_self_config_tools()) if self.config.workspace_for_room is None: raise ValueError("workspace_for_room is required for Codex room isolation") + if self.config.cwd is not None: + raise ValueError("cwd is not supported; use workspace_for_room") if self.config.transport != "stdio": raise ValueError("only stdio Codex transport guarantees room process isolation") if client_factory is not None: @@ -1283,7 +1285,7 @@ async def _ensure_thread( dynamic_tools = self._build_dynamic_tools(tools) start_params: dict[str, Any] = { "model": self._selected_model, - "cwd": self.config.cwd, + "cwd": self._room_client(room_id).workspace, "approvalPolicy": self.config.approval_policy, "personality": self.config.personality, "dynamicTools": dynamic_tools, diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index 1466638ee..51bc4776d 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -655,9 +655,9 @@ async def on_cleanup(self, room_id: str) -> None: async def cleanup_all(self, *, final: bool = True) -> None: """Adapter-wide teardown — the hook ``Agent.stop()`` invokes on shutdown. - The ACP subprocess / TCP connection and the local Band MCP server are started - adapter-wide in ``on_started`` (not per room), so releasing them belongs here, - not in per-room ``on_cleanup``. Idempotent — safe to call again from ``stop()``. + Room-owned ACP subprocesses are released by ``on_cleanup``; this method + releases every remaining runtime and the shared local Band MCP server. + Idempotent — safe to call again from ``stop()``. ``final`` distinguishes real process shutdown (the default: no future turn can arrive, so a still-parked one must fail rather than start resources diff --git a/tests/adapters/test_room_workspace_isolation.py b/tests/adapters/test_room_workspace_isolation.py index 073906386..fa8ecece9 100644 --- a/tests/adapters/test_room_workspace_isolation.py +++ b/tests/adapters/test_room_workspace_isolation.py @@ -53,6 +53,44 @@ async def test_acp_rejects_a_workspace_shared_by_live_rooms() -> None: await adapter._runtime_for("room-b") +@pytest.mark.asyncio +async def test_acp_owns_and_releases_one_runtime_per_room( + monkeypatch: pytest.MonkeyPatch, +) -> None: + class Runtime: + def __init__(self) -> None: + self.stopped = False + + async def stop(self) -> None: + self.stopped = True + + runtimes = [Runtime(), Runtime()] + adapter = ACPClientAdapter( + command="codex", + workspace_for_room=lambda room_id: f"/workspace/{room_id}", + ) + monkeypatch.setattr(adapter, "_build_runtime", lambda: runtimes.pop(0)) + + first = await adapter._runtime_for("room-a") + second = await adapter._runtime_for("room-b") + + assert first is not second + assert adapter._room_workspaces == { + "room-a": "/workspace/room-a", + "room-b": "/workspace/room-b", + } + + await adapter.on_cleanup("room-a") + + assert first.stopped + assert not second.stopped + assert "room-b" in adapter._runtimes + + await adapter.cleanup_all() + + assert second.stopped + + @pytest.mark.asyncio async def test_codex_discards_a_client_after_startup_failure( monkeypatch: pytest.MonkeyPatch, @@ -79,3 +117,13 @@ async def close(self) -> None: assert client.closed assert adapter._client is None + + +def test_codex_rejects_the_former_shared_cwd_option() -> None: + with pytest.raises(ValueError, match="use workspace_for_room"): + CodexAdapter( + CodexAdapterConfig( + cwd="/workspace", + workspace_for_room=lambda room_id: f"/workspace/{room_id}", + ) + ) From 84e8e60667059f9ba23cdbf111fc86e18ac6833f Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Sun, 13 Sep 2026 07:28:42 +0300 Subject: [PATCH 4/8] test: cover Codex thread workspace isolation --- .../adapters/test_room_workspace_isolation.py | 52 ++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/tests/adapters/test_room_workspace_isolation.py b/tests/adapters/test_room_workspace_isolation.py index fa8ecece9..e82f73a71 100644 --- a/tests/adapters/test_room_workspace_isolation.py +++ b/tests/adapters/test_room_workspace_isolation.py @@ -2,9 +2,13 @@ from __future__ import annotations +from typing import cast + import pytest -from band.adapters.codex import CodexAdapter, CodexAdapterConfig +from band.adapters.codex import CodexAdapter, CodexAdapterConfig, CodexSessionState +from band.core.protocols import AgentToolsProtocol +from band.testing import FakeAgentTools from band.integrations.acp.client_adapter import ACPClientAdapter @@ -82,6 +86,8 @@ async def stop(self) -> None: await adapter.on_cleanup("room-a") + assert isinstance(first, Runtime) + assert isinstance(second, Runtime) assert first.stopped assert not second.stopped assert "room-b" in adapter._runtimes @@ -127,3 +133,47 @@ def test_codex_rejects_the_former_shared_cwd_option() -> None: workspace_for_room=lambda room_id: f"/workspace/{room_id}", ) ) + + +@pytest.mark.asyncio +async def test_codex_starts_each_thread_in_its_room_workspace() -> None: + class Client: + def __init__(self) -> None: + self.requests: list[tuple[str, dict[str, object]]] = [] + + async def request(self, method: str, params: dict[str, object]) -> dict[str, object]: + self.requests.append((method, params)) + return {"thread": {"id": f"thread-{len(self.requests)}"}} + + adapter = CodexAdapter( + CodexAdapterConfig( + model="gpt-5.5", + workspace_for_room=lambda room_id: f"/workspace/{room_id}", + ) + ) + tools = FakeAgentTools() + clients: list[Client] = [] + for room_id in ("room-a", "room-b"): + adapter._room_client(room_id) + adapter._active_room.set(room_id) + client = Client() + adapter._client = client # type: ignore[assignment] + adapter._selected_model = "gpt-5.5" + clients.append(client) + await adapter._ensure_thread( + room_id=room_id, + history=CodexSessionState(), + tools=cast(AgentToolsProtocol, tools), + is_session_bootstrap=False, + ) + + starts = [ + params + for client in clients + for method, params in client.requests + if method == "thread/start" + ] + assert [params["cwd"] for params in starts] == [ + "/workspace/room-a", + "/workspace/room-b", + ] From 6fc7128a6ec86e6354f29ed75c557dac67edeea4 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Sun, 13 Sep 2026 07:33:41 +0300 Subject: [PATCH 5/8] feat: default coding workspaces per room --- docs/acp.md | 7 ++-- docs/adapters/codex.md | 19 ++-------- examples/acp/clients/bridge_architecture.py | 3 -- examples/acp/clients/cursor.py | 13 ------- examples/acp/clients/generic.py | 5 --- examples/acp/clients/rich_streaming.py | 5 --- examples/codex/01_basic_agent.py | 8 ---- examples/codex/02_tom_agent.py | 8 ---- examples/codex/03_jerry_agent.py | 8 ---- examples/docker_demo/agents/dev/main.py | 7 ---- examples/run_agent.py | 12 +----- src/band/__init__.py | 2 + src/band/adapters/codex.py | 11 ++---- src/band/integrations/acp/__init__.py | 1 - src/band/integrations/acp/client_adapter.py | 11 ++---- src/band/workspaces.py | 38 +++++++++++++++++++ .../adapters/test_room_workspace_isolation.py | 32 ++++++++++++++++ 17 files changed, 88 insertions(+), 102 deletions(-) create mode 100644 src/band/workspaces.py diff --git a/docs/acp.md b/docs/acp.md index e5f46b614..bec648541 100644 --- a/docs/acp.md +++ b/docs/acp.md @@ -110,12 +110,13 @@ Install with: `pip install band-sdk[acp]` or `uv add band-sdk[acp]` ## Client workspace isolation -`ACPClientAdapter` requires `workspace_for_room`, a resolver returning an absolute -workspace for each Band room. It lazily starts one stdio agent process per room and +`ACPClientAdapter` creates an isolated `./.band-workspaces/` directory for +each Band room by default. Pass `workspace_for_room` only to select a different +absolute workspace policy. It lazily starts one stdio agent process per room and stops that process when the room is cleaned up. TCP and custom transport injection are rejected because they cannot prove that a remote process belongs to only one room. The assigned working directory is not an operating-system sandbox; configure the agent's -sandbox policy separately when that boundary is required. The resolver must assign a +sandbox policy separately when that boundary is required. A custom resolver must assign a different workspace to every live room, and the adapter requires a non-empty stdio command. ## GitHub Copilot CLI backend diff --git a/docs/adapters/codex.md b/docs/adapters/codex.md index bf0ab8ad1..097cf794b 100644 --- a/docs/adapters/codex.md +++ b/docs/adapters/codex.md @@ -32,16 +32,11 @@ Credentials for Band can also be loaded from `agent_config.yaml` with `Agent.fro ```python import asyncio -import os - from band import Agent from band.adapters.codex import CodexAdapter, CodexAdapterConfig adapter = CodexAdapter( - config=CodexAdapterConfig( - workspace_for_room=lambda room_id: os.path.join("/workspaces", room_id), - model="gpt-5.5", - ), + config=CodexAdapterConfig(model="gpt-5.5"), ) agent = Agent.create( @@ -112,7 +107,7 @@ Pass these to `CodexAdapterConfig(...)`: | Parameter | Type | Default | Description | |-----------|------|---------|-------------| -| `workspace_for_room` | `Callable[[str], str]` | required | Returns the absolute workspace for a room. Every live room must receive a distinct workspace. | +| `workspace_for_room` | `Callable[[str], str] | None` | `None` | Optional override for a room workspace. By default, the adapter creates `./.band-workspaces/`. | | `model` | `str \| None` | `None` | Model to use. When unset, the adapter asks Codex for visible models and uses the first visible model, or the adapter default if discovery fails or returns no usable model. | | `reasoning_effort` | `"none" \| "minimal" \| "low" \| "medium" \| "high" \| "xhigh" \| None` | `None` | Reasoning effort for models that support it. | | `reasoning_summary` | `"auto" \| "concise" \| "detailed" \| "none" \| None` | `None` | How Codex summarizes reasoning in responses. | @@ -211,10 +206,7 @@ from band.core.types import Capability, Emit from band.adapters.codex import CodexAdapter, CodexAdapterConfig adapter = CodexAdapter( - config=CodexAdapterConfig( - workspace_for_room=lambda room_id: f"/workspaces/{room_id}", - model="gpt-5.5", - ), + config=CodexAdapterConfig(model="gpt-5.5"), capabilities=Capability.CONTACTS | Capability.MEMORY, emit=Emit.TOOL_CALLS | Emit.THOUGHTS | Emit.TASK_EVENTS, ) @@ -293,10 +285,7 @@ def get_weather(args: WeatherInput) -> str: adapter = CodexAdapter( - config=CodexAdapterConfig( - workspace_for_room=lambda room_id: f"/workspaces/{room_id}", - model="gpt-5.5", - ), + config=CodexAdapterConfig(model="gpt-5.5"), additional_tools=[(WeatherInput, get_weather)], ) ``` diff --git a/examples/acp/clients/bridge_architecture.py b/examples/acp/clients/bridge_architecture.py index 052fea5ff..ad35b398c 100644 --- a/examples/acp/clients/bridge_architecture.py +++ b/examples/acp/clients/bridge_architecture.py @@ -45,7 +45,6 @@ from __future__ import annotations import asyncio -import os import logging import shlex @@ -84,7 +83,6 @@ async def main() -> None: settings = Settings() command = shlex.split(settings.acp_agent_command) - cwd = settings.acp_agent_cwd auth_method = settings.acp_auth_method or None inject_band_tools = settings.acp_inject_band_tools profile_name = settings.acp_client_profile.strip().lower() @@ -92,7 +90,6 @@ async def main() -> None: adapter = ACPClientAdapter( command=command, - workspace_for_room=lambda room_id: os.path.join(cwd, room_id), inject_band_tools=inject_band_tools, auth_method=auth_method, profile=profile, diff --git a/examples/acp/clients/cursor.py b/examples/acp/clients/cursor.py index 9b584f341..4a6091d78 100644 --- a/examples/acp/clients/cursor.py +++ b/examples/acp/clients/cursor.py @@ -53,8 +53,6 @@ import os from dotenv import load_dotenv -from pydantic_settings import BaseSettings, SettingsConfigDict - from band import Agent, configure_logging from band.adapters import ACPClientAdapter from band.integrations.acp.client_profiles import CursorACPClientProfile @@ -70,18 +68,8 @@ logger = logging.getLogger(__name__) -class Settings(BaseSettings): - model_config = SettingsConfigDict( - extra="ignore", case_sensitive=False, env_ignore_empty=True - ) - - acp_agent_cwd: str = "." - - async def main() -> None: load_dotenv() - settings = Settings() - cwd = settings.acp_agent_cwd # Cursor authentication environment — passed to the subprocess, so left as # a direct os.getenv pair rather than a Settings field. @@ -98,7 +86,6 @@ async def main() -> None: # - Band tools are injected through a local localhost-only MCP server adapter = ACPClientAdapter( command=[os.path.expanduser("~/.local/bin/agent"), "acp"], - workspace_for_room=lambda room_id: os.path.join(cwd, room_id), env=cursor_env or None, inject_band_tools=True, auth_method="cursor_login", diff --git a/examples/acp/clients/generic.py b/examples/acp/clients/generic.py index 4bc1d7361..80f7a2420 100644 --- a/examples/acp/clients/generic.py +++ b/examples/acp/clients/generic.py @@ -37,7 +37,6 @@ from __future__ import annotations import asyncio -import os import logging import shlex @@ -78,13 +77,9 @@ async def main() -> None: # Command to spawn the remote ACP agent acp_command = shlex.split(settings.acp_agent_command) - # Working directory for ACP sessions - acp_cwd = settings.acp_agent_cwd - # Create adapter pointing to remote ACP agent adapter = ACPClientAdapter( command=acp_command, - workspace_for_room=lambda room_id: os.path.join(acp_cwd, room_id), ) logger.info( diff --git a/examples/acp/clients/rich_streaming.py b/examples/acp/clients/rich_streaming.py index be2c3af10..2586e3517 100644 --- a/examples/acp/clients/rich_streaming.py +++ b/examples/acp/clients/rich_streaming.py @@ -49,7 +49,6 @@ from __future__ import annotations import asyncio -import os import logging import shlex @@ -90,13 +89,9 @@ async def main() -> None: # Command to spawn the remote ACP agent acp_command = shlex.split(settings.acp_agent_command) - # Working directory for ACP sessions - acp_cwd = settings.acp_agent_cwd - # Create adapter pointing to remote ACP agent adapter = ACPClientAdapter( command=acp_command, - workspace_for_room=lambda room_id: os.path.join(acp_cwd, room_id), ) logger.info("Starting ACP client bridge with rich streaming...") diff --git a/examples/codex/01_basic_agent.py b/examples/codex/01_basic_agent.py index 9133d2869..1033c2b39 100644 --- a/examples/codex/01_basic_agent.py +++ b/examples/codex/01_basic_agent.py @@ -31,7 +31,6 @@ import asyncio import logging -import os from pathlib import Path from dotenv import load_dotenv @@ -63,12 +62,6 @@ class Settings(BaseSettings): codex_role: str = "" -def workspace_for_room(room_id: str) -> str: - workspace = Path(os.getenv("CODEX_WORKSPACE_ROOT", ".band-workspaces")) / room_id - workspace.mkdir(parents=True, exist_ok=True) - return str(workspace.resolve()) - - async def main() -> None: load_dotenv() settings = Settings() @@ -93,7 +86,6 @@ async def main() -> None: # docstring) when omitted here. adapter = CodexAdapter( config=CodexAdapterConfig( - workspace_for_room=workspace_for_room, personality="pragmatic", custom_section=custom_section, include_base_instructions=True, diff --git a/examples/codex/02_tom_agent.py b/examples/codex/02_tom_agent.py index 6303993ac..1b28bfd42 100644 --- a/examples/codex/02_tom_agent.py +++ b/examples/codex/02_tom_agent.py @@ -29,7 +29,6 @@ import logging import os import sys -from pathlib import Path from dotenv import load_dotenv @@ -55,19 +54,12 @@ logger = logging.getLogger(__name__) -def workspace_for_room(room_id: str) -> str: - workspace = Path(os.getenv("CODEX_WORKSPACE_ROOT", ".band-workspaces")) / room_id - workspace.mkdir(parents=True, exist_ok=True) - return str(workspace.resolve()) - - async def main() -> None: load_dotenv() # model self-sources from CODEX_MODEL when omitted here. adapter = CodexAdapter( config=CodexAdapterConfig( - workspace_for_room=workspace_for_room, transport="stdio", personality="none", custom_section=generate_tom_prompt("Tom"), diff --git a/examples/codex/03_jerry_agent.py b/examples/codex/03_jerry_agent.py index 969f451a8..c78ed720a 100644 --- a/examples/codex/03_jerry_agent.py +++ b/examples/codex/03_jerry_agent.py @@ -29,7 +29,6 @@ import logging import os import sys -from pathlib import Path from dotenv import load_dotenv @@ -55,19 +54,12 @@ logger = logging.getLogger(__name__) -def workspace_for_room(room_id: str) -> str: - workspace = Path(os.getenv("CODEX_WORKSPACE_ROOT", ".band-workspaces")) / room_id - workspace.mkdir(parents=True, exist_ok=True) - return str(workspace.resolve()) - - async def main() -> None: load_dotenv() # model self-sources from CODEX_MODEL when omitted here. adapter = CodexAdapter( config=CodexAdapterConfig( - workspace_for_room=workspace_for_room, transport="stdio", personality="none", custom_section=generate_jerry_prompt("Jerry"), diff --git a/examples/docker_demo/agents/dev/main.py b/examples/docker_demo/agents/dev/main.py index 43e3fdf8d..e8adb90be 100644 --- a/examples/docker_demo/agents/dev/main.py +++ b/examples/docker_demo/agents/dev/main.py @@ -45,12 +45,6 @@ def build_persona() -> str: return f"{persona}\n\n{CONVERSATION_DISCIPLINE}" -def workspace_for_room(room_id: str) -> str: - workspace = Path(".band-workspaces") / room_id - workspace.mkdir(parents=True, exist_ok=True) - return str(workspace.resolve()) - - def expose_llm_key() -> None: """Copy the sbx-injected placeholder into the var the codex CLI reads. @@ -88,7 +82,6 @@ async def main() -> None: config = DevConfig() adapter = CodexAdapter( config=CodexAdapterConfig( - workspace_for_room=workspace_for_room, model=config.model, approval_policy="never", custom_section=build_persona(), diff --git a/examples/run_agent.py b/examples/run_agent.py index 2a72f528f..8f978ebd3 100644 --- a/examples/run_agent.py +++ b/examples/run_agent.py @@ -456,22 +456,14 @@ async def run_codex_agent( logger: logging.Logger, ) -> None: """Run the Codex app-server adapter.""" + from band import create_room_workspace_resolver # noqa: PLC0415 -- only load the adapters extra when this example is the one selected to run from band.adapters import CodexAdapter # noqa: PLC0415 -- only load the adapters extra when this example is the one selected to run from band.adapters.codex import CodexAdapterConfig # noqa: PLC0415 -- only load the codex extra when this example is the one selected to run - workspace_root = Path(codex_cwd).resolve() - - def workspace_for_room(room_id: str) -> str: - workspace = (workspace_root / room_id).resolve() - if workspace_root not in workspace.parents: - raise ValueError("room id cannot escape the Codex workspace root") - workspace.mkdir(parents=True, exist_ok=True) - return str(workspace) - adapter = CodexAdapter( config=CodexAdapterConfig( transport=codex_transport, # type: ignore[arg-type] # str from CLI args, validated at runtime - workspace_for_room=workspace_for_room, + workspace_for_room=create_room_workspace_resolver(codex_cwd), model=codex_model, personality=codex_personality, # type: ignore[arg-type] # str from CLI args, validated at runtime approval_policy=codex_approval_policy, diff --git a/src/band/__init__.py b/src/band/__init__.py index a69faeba9..03791448b 100644 --- a/src/band/__init__.py +++ b/src/band/__init__.py @@ -72,6 +72,7 @@ async def handle_event(ctx: ExecutionContext, event: PlatformEvent): # Platform layer from .platform import BandLink, PlatformEvent +from .workspaces import create_room_workspace_resolver # Runtime layer from .runtime import ( @@ -132,6 +133,7 @@ async def handle_event(ctx: ExecutionContext, event: PlatformEvent): # Platform "BandLink", "PlatformEvent", + "create_room_workspace_resolver", # Runtime - Core "AgentRuntime", "RoomPresence", diff --git a/src/band/adapters/codex.py b/src/band/adapters/codex.py index 746eb0509..c575e4eb6 100644 --- a/src/band/adapters/codex.py +++ b/src/band/adapters/codex.py @@ -5,7 +5,6 @@ import asyncio import json import logging -import os import time as _time from collections import OrderedDict from contextvars import ContextVar @@ -21,6 +20,7 @@ from band.converters.helpers import build_replay_messages from band.core.protocols import AgentToolsProtocol from band.core.simple_adapter import SimpleAdapter +from band.workspaces import resolve_room_workspace from band.core.types import ( AgentInput, Capability, @@ -432,10 +432,8 @@ def __init__( self._custom_tools: list[CustomToolDef] = list(additional_tools or []) if self.config.enable_self_config_tools: self._custom_tools.extend(self._build_self_config_tools()) - if self.config.workspace_for_room is None: - raise ValueError("workspace_for_room is required for Codex room isolation") if self.config.cwd is not None: - raise ValueError("cwd is not supported; use workspace_for_room") + raise ValueError("cwd is not supported; use workspace_for_room or the default") if self.config.transport != "stdio": raise ValueError("only stdio Codex transport guarantees room process isolation") if client_factory is not None: @@ -473,10 +471,7 @@ def __init__( def _room_client(self, room_id: str) -> RoomCodexClient: room = self._room_clients.get(room_id) if room is None: - workspace = self.config.workspace_for_room(room_id) # type: ignore[misc] - if not isinstance(workspace, str) or not os.path.isabs(workspace): - raise ValueError("workspace_for_room must return an absolute path") - workspace = os.path.realpath(workspace) + workspace = resolve_room_workspace(room_id, self.config.workspace_for_room) owner = self._workspace_rooms.get(workspace) if owner is not None and owner != room_id: raise ValueError( diff --git a/src/band/integrations/acp/__init__.py b/src/band/integrations/acp/__init__.py index 34a5bc372..9df090190 100644 --- a/src/band/integrations/acp/__init__.py +++ b/src/band/integrations/acp/__init__.py @@ -35,7 +35,6 @@ adapter = ACPClientAdapter( command="codex", - workspace_for_room=lambda room_id: f"/workspace/{room_id}", ) agent = Agent.create(adapter=adapter, agent_id="...", api_key="...") await agent.run() diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index 51bc4776d..169f093b4 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -4,7 +4,6 @@ import asyncio import logging -import os import shutil from collections.abc import Callable from typing import Any, ClassVar @@ -44,6 +43,7 @@ ) from band.integrations.acp.room_emitter import RoomTurnEmitter from band.integrations.acp.types import ACPToolCall +from band.workspaces import resolve_room_workspace from band.runtime.prompts import render_system_prompt from band.runtime.custom_tools import CustomToolDef, get_custom_tool_name from band.runtime.formatters import messages_before @@ -164,10 +164,8 @@ def __init__( history_converter=ACPClientHistoryConverter(), **features, ) - if workspace_for_room is None: - raise ValueError("workspace_for_room is required for ACP client isolation") if cwd is not None: - raise ValueError("cwd is not supported; use workspace_for_room") + raise ValueError("cwd is not supported; use workspace_for_room or the default") if host is not None or port is not None: raise ValueError("TCP ACP transport cannot guarantee room process isolation") if spawn_process is not None: @@ -259,10 +257,7 @@ def _build_runtime(self) -> ACPRuntime: ) def _workspace(self, room_id: str) -> str: - workspace = self._workspace_for_room(room_id) - if not isinstance(workspace, str) or not os.path.isabs(workspace): - raise ValueError("workspace_for_room must return an absolute path") - return os.path.realpath(workspace) + return resolve_room_workspace(room_id, self._workspace_for_room) async def _runtime_for(self, room_id: str) -> ACPRuntime: async with self._session_lock: diff --git a/src/band/workspaces.py b/src/band/workspaces.py new file mode 100644 index 000000000..8211f53eb --- /dev/null +++ b/src/band/workspaces.py @@ -0,0 +1,38 @@ +"""Workspace selection for room-owned local coding agents.""" + +from __future__ import annotations + +import os +from collections.abc import Callable +from pathlib import Path + +WorkspaceResolver = Callable[[str], str] + +_DEFAULT_WORKSPACE_DIRECTORY = ".band-workspaces" + + +def resolve_room_workspace( + room_id: str, workspace_for_room: WorkspaceResolver | None +) -> str: + """Return a room's absolute workspace, creating the safe default on demand.""" + if workspace_for_room is not None: + workspace = workspace_for_room(room_id) + if not isinstance(workspace, str) or not os.path.isabs(workspace): + raise ValueError("workspace_for_room must return an absolute path") + return os.path.realpath(workspace) + + return create_room_workspace_resolver(Path.cwd() / _DEFAULT_WORKSPACE_DIRECTORY)(room_id) + + +def create_room_workspace_resolver(root: str | Path) -> WorkspaceResolver: + """Build a resolver that creates an isolated child workspace per room.""" + workspace_root = Path(root).expanduser().resolve() + + def workspace_for_room(room_id: str) -> str: + workspace = (workspace_root / room_id).resolve() + if workspace_root not in workspace.parents: + raise ValueError("room id cannot escape the workspace root") + workspace.mkdir(parents=True, exist_ok=True) + return str(workspace) + + return workspace_for_room diff --git a/tests/adapters/test_room_workspace_isolation.py b/tests/adapters/test_room_workspace_isolation.py index e82f73a71..8cd2b219b 100644 --- a/tests/adapters/test_room_workspace_isolation.py +++ b/tests/adapters/test_room_workspace_isolation.py @@ -2,6 +2,7 @@ from __future__ import annotations +from pathlib import Path from typing import cast import pytest @@ -23,6 +24,21 @@ def test_codex_rejects_a_workspace_shared_by_live_rooms() -> None: adapter._room_client("room-b") +def test_codex_uses_distinct_default_room_workspaces( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.chdir(tmp_path) + adapter = CodexAdapter(CodexAdapterConfig()) + + first = adapter._room_client("room-a") + second = adapter._room_client("room-b") + + assert first.workspace == str(tmp_path / ".band-workspaces" / "room-a") + assert second.workspace == str(tmp_path / ".band-workspaces" / "room-b") + assert (tmp_path / ".band-workspaces" / "room-a").is_dir() + assert (tmp_path / ".band-workspaces" / "room-b").is_dir() + + @pytest.mark.asyncio async def test_acp_retries_workspace_resolution_after_a_failure() -> None: attempts = 0 @@ -45,6 +61,22 @@ def workspace_for_room(_room_id: str) -> str: assert adapter._room_workspaces == {"room-a": "/workspace/room-a"} +@pytest.mark.asyncio +async def test_acp_uses_distinct_default_room_workspaces( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.chdir(tmp_path) + adapter = ACPClientAdapter(command="codex") + + await adapter._runtime_for("room-a") + await adapter._runtime_for("room-b") + + assert adapter._room_workspaces == { + "room-a": str(tmp_path / ".band-workspaces" / "room-a"), + "room-b": str(tmp_path / ".band-workspaces" / "room-b"), + } + + @pytest.mark.asyncio async def test_acp_rejects_a_workspace_shared_by_live_rooms() -> None: adapter = ACPClientAdapter( From 97e331548fd047ff8efc735f484f041fd9ea1859 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Sun, 13 Sep 2026 07:35:19 +0300 Subject: [PATCH 6/8] test: simplify room workspace coverage --- .../adapters/test_room_workspace_isolation.py | 182 +++--------------- 1 file changed, 31 insertions(+), 151 deletions(-) diff --git a/tests/adapters/test_room_workspace_isolation.py b/tests/adapters/test_room_workspace_isolation.py index 8cd2b219b..573adf846 100644 --- a/tests/adapters/test_room_workspace_isolation.py +++ b/tests/adapters/test_room_workspace_isolation.py @@ -1,4 +1,4 @@ -"""Room-owned workspace guards for coding-agent adapters.""" +"""Room workspace isolation for coding-agent adapters.""" from __future__ import annotations @@ -9,173 +9,59 @@ from band.adapters.codex import CodexAdapter, CodexAdapterConfig, CodexSessionState from band.core.protocols import AgentToolsProtocol -from band.testing import FakeAgentTools from band.integrations.acp.client_adapter import ACPClientAdapter +from band.testing import FakeAgentTools +from band.workspaces import resolve_room_workspace -def test_codex_rejects_a_workspace_shared_by_live_rooms() -> None: - adapter = CodexAdapter( - CodexAdapterConfig(workspace_for_room=lambda _room_id: "/workspace") - ) - - adapter._room_client("room-a") - - with pytest.raises(ValueError, match="both 'room-a' and 'room-b'"): - adapter._room_client("room-b") - - -def test_codex_uses_distinct_default_room_workspaces( +def test_default_workspace_is_created_per_room( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: monkeypatch.chdir(tmp_path) - adapter = CodexAdapter(CodexAdapterConfig()) - - first = adapter._room_client("room-a") - second = adapter._room_client("room-b") - - assert first.workspace == str(tmp_path / ".band-workspaces" / "room-a") - assert second.workspace == str(tmp_path / ".band-workspaces" / "room-b") - assert (tmp_path / ".band-workspaces" / "room-a").is_dir() - assert (tmp_path / ".band-workspaces" / "room-b").is_dir() - - -@pytest.mark.asyncio -async def test_acp_retries_workspace_resolution_after_a_failure() -> None: - attempts = 0 - def workspace_for_room(_room_id: str) -> str: - nonlocal attempts - attempts += 1 - if attempts == 1: - raise ValueError("workspace provisioning failed") - return "/workspace/room-a" + first = resolve_room_workspace("room-a", None) + second = resolve_room_workspace("room-b", None) - adapter = ACPClientAdapter(command="codex", workspace_for_room=workspace_for_room) - - with pytest.raises(ValueError, match="workspace provisioning failed"): - await adapter._runtime_for("room-a") - - runtime = await adapter._runtime_for("room-a") - - assert adapter._runtimes["room-a"] is runtime - assert adapter._room_workspaces == {"room-a": "/workspace/room-a"} + assert [first, second] == [ + str(tmp_path / ".band-workspaces" / "room-a"), + str(tmp_path / ".band-workspaces" / "room-b"), + ] + assert Path(first).is_dir() + assert Path(second).is_dir() @pytest.mark.asyncio -async def test_acp_uses_distinct_default_room_workspaces( - monkeypatch: pytest.MonkeyPatch, tmp_path: Path -) -> None: - monkeypatch.chdir(tmp_path) - adapter = ACPClientAdapter(command="codex") +async def test_adapters_reject_a_custom_workspace_shared_by_live_rooms() -> None: + def resolver(_room_id: str) -> str: + return "/workspace" - await adapter._runtime_for("room-a") - await adapter._runtime_for("room-b") + codex = CodexAdapter(CodexAdapterConfig(workspace_for_room=resolver)) + acp = ACPClientAdapter(command="codex", workspace_for_room=resolver) - assert adapter._room_workspaces == { - "room-a": str(tmp_path / ".band-workspaces" / "room-a"), - "room-b": str(tmp_path / ".band-workspaces" / "room-b"), - } - - -@pytest.mark.asyncio -async def test_acp_rejects_a_workspace_shared_by_live_rooms() -> None: - adapter = ACPClientAdapter( - command="codex", workspace_for_room=lambda _room_id: "/workspace" - ) - - await adapter._runtime_for("room-a") + codex._room_client("room-a") + await acp._runtime_for("room-a") with pytest.raises(ValueError, match="both 'room-a' and 'room-b'"): - await adapter._runtime_for("room-b") - - -@pytest.mark.asyncio -async def test_acp_owns_and_releases_one_runtime_per_room( - monkeypatch: pytest.MonkeyPatch, -) -> None: - class Runtime: - def __init__(self) -> None: - self.stopped = False - - async def stop(self) -> None: - self.stopped = True - - runtimes = [Runtime(), Runtime()] - adapter = ACPClientAdapter( - command="codex", - workspace_for_room=lambda room_id: f"/workspace/{room_id}", - ) - monkeypatch.setattr(adapter, "_build_runtime", lambda: runtimes.pop(0)) - - first = await adapter._runtime_for("room-a") - second = await adapter._runtime_for("room-b") - - assert first is not second - assert adapter._room_workspaces == { - "room-a": "/workspace/room-a", - "room-b": "/workspace/room-b", - } - - await adapter.on_cleanup("room-a") - - assert isinstance(first, Runtime) - assert isinstance(second, Runtime) - assert first.stopped - assert not second.stopped - assert "room-b" in adapter._runtimes - - await adapter.cleanup_all() - - assert second.stopped - - -@pytest.mark.asyncio -async def test_codex_discards_a_client_after_startup_failure( - monkeypatch: pytest.MonkeyPatch, -) -> None: - class FailingClient: - closed = False - - async def connect(self) -> None: - raise RuntimeError("startup failed") - - async def close(self) -> None: - self.closed = True - - client = FailingClient() - adapter = CodexAdapter( - CodexAdapterConfig(workspace_for_room=lambda _room_id: "/workspace/room-a") - ) - adapter._room_client("room-a") - adapter._active_room.set("room-a") - monkeypatch.setattr(adapter, "_build_client", lambda _config: client) - - with pytest.raises(RuntimeError, match="startup failed"): - await adapter._ensure_client_ready() - - assert client.closed - assert adapter._client is None + codex._room_client("room-b") + with pytest.raises(ValueError, match="both 'room-a' and 'room-b'"): + await acp._runtime_for("room-b") def test_codex_rejects_the_former_shared_cwd_option() -> None: - with pytest.raises(ValueError, match="use workspace_for_room"): - CodexAdapter( - CodexAdapterConfig( - cwd="/workspace", - workspace_for_room=lambda room_id: f"/workspace/{room_id}", - ) - ) + with pytest.raises(ValueError, match="workspace_for_room or the default"): + CodexAdapter(CodexAdapterConfig(cwd="/workspace")) @pytest.mark.asyncio async def test_codex_starts_each_thread_in_its_room_workspace() -> None: class Client: def __init__(self) -> None: - self.requests: list[tuple[str, dict[str, object]]] = [] + self.params: dict[str, object] | None = None async def request(self, method: str, params: dict[str, object]) -> dict[str, object]: - self.requests.append((method, params)) - return {"thread": {"id": f"thread-{len(self.requests)}"}} + assert method == "thread/start" + self.params = params + return {"thread": {"id": "thread"}} adapter = CodexAdapter( CodexAdapterConfig( @@ -183,7 +69,7 @@ async def request(self, method: str, params: dict[str, object]) -> dict[str, obj workspace_for_room=lambda room_id: f"/workspace/{room_id}", ) ) - tools = FakeAgentTools() + tools = cast(AgentToolsProtocol, FakeAgentTools()) clients: list[Client] = [] for room_id in ("room-a", "room-b"): adapter._room_client(room_id) @@ -195,17 +81,11 @@ async def request(self, method: str, params: dict[str, object]) -> dict[str, obj await adapter._ensure_thread( room_id=room_id, history=CodexSessionState(), - tools=cast(AgentToolsProtocol, tools), + tools=tools, is_session_bootstrap=False, ) - starts = [ - params - for client in clients - for method, params in client.requests - if method == "thread/start" - ] - assert [params["cwd"] for params in starts] == [ + assert [client.params["cwd"] for client in clients if client.params] == [ "/workspace/room-a", "/workspace/room-b", ] From 6661e0b11f2d8d744acd6c22e1ee03d10663afd1 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Sun, 13 Sep 2026 09:11:42 +0300 Subject: [PATCH 7/8] fix: migrate Codex/ACP tests to room-owned lifecycle and address review Extract shared workspace collision guard, DRY ACP profile resolution, and test helpers that patch per-room stdio spawns so legacy fixtures work without the removed shared-process APIs. Co-authored-by: Cursor --- docs/adapters/codex.md | 11 +- examples/acp/clients/bridge_architecture.py | 5 +- src/band/adapters/codex.py | 26 +- src/band/integrations/acp/client_adapter.py | 34 +- src/band/integrations/acp/client_profiles.py | 12 + src/band/workspaces.py | 18 +- tests/adapters/test_codex_adapter.py | 906 ++++++------------ tests/adapters/test_copilot_acp_adapter.py | 69 +- .../adapters/test_room_workspace_isolation.py | 4 +- .../integrations/acp/acp_toolkit/__init__.py | 2 + tests/integrations/acp/acp_toolkit/harness.py | 29 +- tests/integrations/acp/test_client_adapter.py | 390 ++++---- tests/integrations/codex/test_adapter_e2e.py | 58 +- 13 files changed, 679 insertions(+), 885 deletions(-) diff --git a/docs/adapters/codex.md b/docs/adapters/codex.md index 097cf794b..9a65fb502 100644 --- a/docs/adapters/codex.md +++ b/docs/adapters/codex.md @@ -39,15 +39,8 @@ adapter = CodexAdapter( config=CodexAdapterConfig(model="gpt-5.5"), ) -agent = Agent.create( - adapter=adapter, - agent_id="your-agent-uuid", - api_key="your-band-api-key", - ws_url="wss://app.band.ai/api/v1/socket/websocket", - rest_url="https://app.band.ai", -) - -asyncio.run(agent.run()) +async with Agent.from_config("my_agent", adapter=adapter) as agent: + await agent.run_forever() ``` ## Where Parameters Go diff --git a/examples/acp/clients/bridge_architecture.py b/examples/acp/clients/bridge_architecture.py index ad35b398c..3f8a20997 100644 --- a/examples/acp/clients/bridge_architecture.py +++ b/examples/acp/clients/bridge_architecture.py @@ -53,7 +53,7 @@ from band import Agent, configure_logging from band.adapters import ACPClientAdapter -from band.integrations.acp.client_profiles import CursorACPClientProfile +from band.integrations.acp.client_profiles import resolve_acp_client_profile configure_logging( level=logging.INFO, @@ -85,8 +85,7 @@ async def main() -> None: command = shlex.split(settings.acp_agent_command) auth_method = settings.acp_auth_method or None inject_band_tools = settings.acp_inject_band_tools - profile_name = settings.acp_client_profile.strip().lower() - profile = CursorACPClientProfile() if profile_name == "cursor" else None + profile = resolve_acp_client_profile(settings.acp_client_profile) adapter = ACPClientAdapter( command=command, diff --git a/src/band/adapters/codex.py b/src/band/adapters/codex.py index c575e4eb6..87c03e9af 100644 --- a/src/band/adapters/codex.py +++ b/src/band/adapters/codex.py @@ -20,7 +20,7 @@ from band.converters.helpers import build_replay_messages from band.core.protocols import AgentToolsProtocol from band.core.simple_adapter import SimpleAdapter -from band.workspaces import resolve_room_workspace +from band.workspaces import claim_room_workspace, resolve_room_workspace from band.core.types import ( AgentInput, Capability, @@ -433,11 +433,17 @@ def __init__( if self.config.enable_self_config_tools: self._custom_tools.extend(self._build_self_config_tools()) if self.config.cwd is not None: - raise ValueError("cwd is not supported; use workspace_for_room or the default") + raise ValueError( + "cwd is not supported; use workspace_for_room or the default" + ) if self.config.transport != "stdio": - raise ValueError("only stdio Codex transport guarantees room process isolation") + raise ValueError( + "only stdio Codex transport guarantees room process isolation" + ) if client_factory is not None: - raise ValueError("custom Codex clients cannot guarantee room process isolation") + raise ValueError( + "custom Codex clients cannot guarantee room process isolation" + ) self._room_clients: dict[str, RoomCodexClient] = {} self._workspace_rooms: dict[str, str] = {} self._active_room: ContextVar[str | None] = ContextVar( @@ -472,14 +478,9 @@ def _room_client(self, room_id: str) -> RoomCodexClient: room = self._room_clients.get(room_id) if room is None: workspace = resolve_room_workspace(room_id, self.config.workspace_for_room) - owner = self._workspace_rooms.get(workspace) - if owner is not None and owner != room_id: - raise ValueError( - f"workspace_for_room assigned {workspace!r} to both {owner!r} and {room_id!r}" - ) + claim_room_workspace(room_id, workspace, self._workspace_rooms) room = RoomCodexClient(workspace=workspace) self._room_clients[room_id] = room - self._workspace_rooms[workspace] = room_id return room def _active_client_state(self) -> RoomCodexClient | None: @@ -1186,7 +1187,10 @@ async def _ensure_client_ready(self) -> None: try: await client.close() except Exception: - logger.debug("Failed to close unsuccessfully initialized Codex client", exc_info=True) + logger.debug( + "Failed to close unsuccessfully initialized Codex client", + exc_info=True, + ) raise def _build_client(self, config: CodexAdapterConfig) -> CodexClientProtocol: diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index 169f093b4..3bb0ebe84 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -28,6 +28,7 @@ from band.integrations.acp.client_runtime import ( ACPConnectionProtocol, ACPRuntime, + MCPTransportKind, PermissionHandler, allow_permission, cancel_permission, @@ -39,11 +40,12 @@ ) from band.integrations.mcp.backends import ( BandMCPBackend, + BandMCPBackendKind, create_band_mcp_backend, ) from band.integrations.acp.room_emitter import RoomTurnEmitter from band.integrations.acp.types import ACPToolCall -from band.workspaces import resolve_room_workspace +from band.workspaces import claim_room_workspace, resolve_room_workspace from band.runtime.prompts import render_system_prompt from band.runtime.custom_tools import CustomToolDef, get_custom_tool_name from band.runtime.formatters import messages_before @@ -60,6 +62,7 @@ logger = logging.getLogger(__name__) LocalMcpServerConfig = HttpMcpServer | SseMcpServer +DEFAULT_BAND_MCP_BACKEND_KIND: BandMCPBackendKind = "http" # Prefixes the change-triggered roster/contacts updates injected into a # prompt, so the model reads them as platform state, not as the requester @@ -165,11 +168,17 @@ def __init__( **features, ) if cwd is not None: - raise ValueError("cwd is not supported; use workspace_for_room or the default") + raise ValueError( + "cwd is not supported; use workspace_for_room or the default" + ) if host is not None or port is not None: - raise ValueError("TCP ACP transport cannot guarantee room process isolation") + raise ValueError( + "TCP ACP transport cannot guarantee room process isolation" + ) if spawn_process is not None: - raise ValueError("custom ACP transports cannot guarantee room process isolation") + raise ValueError( + "custom ACP transports cannot guarantee room process isolation" + ) if not command: raise ValueError("ACP stdio transport requires a command") self._command = [command] if isinstance(command, str) else list(command) @@ -264,15 +273,10 @@ async def _runtime_for(self, room_id: str) -> ACPRuntime: runtime = self._runtimes.get(room_id) if runtime is None: workspace = self._workspace(room_id) - owner = self._workspace_rooms.get(workspace) - if owner is not None and owner != room_id: - raise ValueError( - f"workspace_for_room assigned {workspace!r} to both {owner!r} and {room_id!r}" - ) + claim_room_workspace(room_id, workspace, self._workspace_rooms) runtime = self._build_runtime() self._runtimes[room_id] = runtime self._room_workspaces[room_id] = workspace - self._workspace_rooms[workspace] = room_id return runtime async def on_started(self, agent_name: str, agent_description: str) -> None: @@ -442,7 +446,7 @@ def _build_system_context(self, room_id: str, msg: PlatformMessage) -> str: return f"[System Context]\n{system_prompt}\n{room_context}" def _build_local_mcp_server_config( - self, local_server: LocalMCPServer, transport: str + self, local_server: LocalMCPServer, transport: MCPTransportKind ) -> LocalMcpServerConfig: if transport == "sse": return SseMcpServer( @@ -503,7 +507,7 @@ async def _ensure_band_mcp_backend(self) -> BandMCPBackend: self._band_mcp_backend = None if self._band_mcp_backend is None: backend = await create_band_mcp_backend( - kind="http", + kind=DEFAULT_BAND_MCP_BACKEND_KIND, tool_definitions=self._tool_definitions, get_tools=self._room_tools.get, additional_tools=self._custom_tools, @@ -647,6 +651,10 @@ async def on_cleanup(self, room_id: str) -> None: logger.debug("Cleaned up ACP client resources for room %s", room_id) + @staticmethod + async def _stop_runtimes(runtimes: list[ACPRuntime]) -> None: + await asyncio.gather(*(runtime.stop() for runtime in runtimes)) + async def cleanup_all(self, *, final: bool = True) -> None: """Adapter-wide teardown — the hook ``Agent.stop()`` invokes on shutdown. @@ -685,7 +693,7 @@ async def cleanup_all(self, *, final: bool = True) -> None: # None and start a fresh backend while this one is mid-teardown. if backend is not None: await backend.stop() - await asyncio.gather(*(runtime.stop() for runtime in runtimes)) + await self._stop_runtimes(runtimes) logger.info("ACP client adapter stopped") async def stop(self) -> None: diff --git a/src/band/integrations/acp/client_profiles.py b/src/band/integrations/acp/client_profiles.py index 8e909b9fa..e55ff31f7 100644 --- a/src/band/integrations/acp/client_profiles.py +++ b/src/band/integrations/acp/client_profiles.py @@ -108,3 +108,15 @@ async def ext_notification( ] return [] + +CURSOR_PROFILE_NAME = "cursor" + + +def resolve_acp_client_profile(profile_name: str) -> ACPClientProfile | None: + """Map a configured profile name to a runtime-specific ACP client profile.""" + normalized = profile_name.strip().lower() + if not normalized: + return None + if normalized == CURSOR_PROFILE_NAME: + return CursorACPClientProfile() + return None diff --git a/src/band/workspaces.py b/src/band/workspaces.py index 8211f53eb..64d0a5d11 100644 --- a/src/band/workspaces.py +++ b/src/band/workspaces.py @@ -21,7 +21,23 @@ def resolve_room_workspace( raise ValueError("workspace_for_room must return an absolute path") return os.path.realpath(workspace) - return create_room_workspace_resolver(Path.cwd() / _DEFAULT_WORKSPACE_DIRECTORY)(room_id) + return create_room_workspace_resolver(Path.cwd() / _DEFAULT_WORKSPACE_DIRECTORY)( + room_id + ) + + +def claim_room_workspace( + room_id: str, + workspace: str, + workspace_rooms: dict[str, str], +) -> None: + """Record one room as the live owner of a resolved workspace path.""" + owner = workspace_rooms.get(workspace) + if owner is not None and owner != room_id: + raise ValueError( + f"workspace_for_room assigned {workspace!r} to both {owner!r} and {room_id!r}" + ) + workspace_rooms[workspace] = room_id def create_room_workspace_resolver(root: str | Path) -> WorkspaceResolver: diff --git a/tests/adapters/test_codex_adapter.py b/tests/adapters/test_codex_adapter.py index b01b97dd4..a14c3f7e8 100644 --- a/tests/adapters/test_codex_adapter.py +++ b/tests/adapters/test_codex_adapter.py @@ -204,6 +204,37 @@ async def close(self) -> None: return None +def patch_codex_client(adapter: CodexAdapter, client: FakeCodexClient) -> None: + def _build(_config: CodexAdapterConfig) -> FakeCodexClient: + return client + + adapter._build_client = _build # type: ignore[method-assign] + + +def make_codex_adapter( + client: FakeCodexClient, + config: CodexAdapterConfig | None = None, + **kwargs: Any, +) -> CodexAdapter: + adapter = CodexAdapter(config=config or CodexAdapterConfig(), **kwargs) + patch_codex_client(adapter, client) + return adapter + + +def wire_codex_room( + adapter: CodexAdapter, + client: FakeCodexClient, + room_id: str = "room-1", + *, + initialized: bool = True, +) -> None: + adapter._room_client(room_id) + adapter._active_room.set(room_id) + adapter._client = client # type: ignore[assignment] + if initialized: + adapter._initialized = True + + def _event_notification(method: str, params: dict[str, Any]) -> RpcEvent: return RpcEvent( kind="notification", @@ -274,11 +305,7 @@ async def run_codex_turn( test states only the events it scripts and the outcome it asserts. """ client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=config or CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: client, - **adapter_kwargs, - ) + adapter = make_codex_adapter(client, config=config, **adapter_kwargs) room_tools = tools if tools is not None else ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -333,10 +360,7 @@ async def test_bootstrap_starts_thread_and_sends_fallback_message(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -379,9 +403,8 @@ async def test_system_prompt_retry_after_turn_start_failure(self) -> None: ), turn_start_error_once=True, ) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", model="gpt-5.5"), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(model="gpt-5.5") ) tools = ToolSchemaFakeTools() @@ -429,10 +452,7 @@ async def test_tool_call_request_is_dispatched_and_responded(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -499,10 +519,7 @@ async def execute_tool_call_structured( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = SendMessageFailureTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -529,10 +546,7 @@ async def test_resume_failure_falls_back_to_thread_start(self) -> None: events=events, resume_error=CodexJsonRpcError(code=-32002, message="Not found"), ) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -561,12 +575,8 @@ async def test_approval_request_auto_decline(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - approval_mode="auto_decline", - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(approval_mode="auto_decline") ) tools = ToolSchemaFakeTools() @@ -606,13 +616,11 @@ async def send_message( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig( - transport="ws", - approval_mode="auto_decline", - approval_text_notifications=True, + approval_mode="auto_decline", approval_text_notifications=True ), - client_factory=lambda _config: fake_client, ) tools = FailingNotifyTools() @@ -651,12 +659,8 @@ async def send_message( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - approval_mode="manual", - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(approval_mode="manual") ) tools = FailingNotifyTools() @@ -680,10 +684,7 @@ async def send_message( @pytest.mark.asyncio async def test_cleanup_closes_client_when_last_room_removed(self) -> None: fake_client = FakeCodexClient(events=[_turn_completed()]) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -705,10 +706,7 @@ async def test_cleanup_closes_client_when_last_room_removed(self) -> None: async def test_cleanup_idempotent(self) -> None: """Calling on_cleanup twice for the same room should not raise.""" fake_client = FakeCodexClient(events=[_turn_completed()]) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -728,44 +726,40 @@ async def test_cleanup_idempotent(self) -> None: await adapter.on_cleanup("room-1") @pytest.mark.asyncio - async def test_cleanup_multi_room_keeps_client_until_last(self) -> None: - """Client stays open until the last room is cleaned up.""" - events_room1 = [_turn_completed()] - events_room2 = [_turn_completed("turn-2")] - fake_client = FakeCodexClient(events=events_room1 + events_room2) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + async def test_cleanup_multi_room_closes_each_room_client(self) -> None: + """Each room owns its Codex client; cleanup closes only that room's client.""" + clients = { + "room-1": FakeCodexClient(events=[_turn_completed()]), + "room-2": FakeCodexClient(events=[_turn_completed("turn-2")]), + } + adapter = CodexAdapter(config=CodexAdapterConfig()) + + def _build(_config: CodexAdapterConfig) -> FakeCodexClient: + room_id = adapter._active_room.get() + assert room_id is not None + return clients[room_id] + + adapter._build_client = _build # type: ignore[method-assign] tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") - await adapter.on_message( - make_platform_message(room_id="room-1"), - tools, - CodexSessionState(), - participants_msg=None, - contacts_msg=None, - is_session_bootstrap=True, - room_id="room-1", - ) - await adapter.on_message( - make_platform_message(room_id="room-2"), - tools, - CodexSessionState(), - participants_msg=None, - contacts_msg=None, - is_session_bootstrap=True, - room_id="room-2", - ) + for room_id in ("room-1", "room-2"): + await adapter.on_message( + make_platform_message(room_id=room_id), + tools, + CodexSessionState(), + participants_msg=None, + contacts_msg=None, + is_session_bootstrap=True, + room_id=room_id, + ) - # Cleaning up room-1 should NOT close the client (room-2 still active) await adapter.on_cleanup("room-1") - assert fake_client.closed is False + assert clients["room-1"].closed is True + assert clients["room-2"].closed is False - # Cleaning up room-2 should close the client (last room) await adapter.on_cleanup("room-2") - assert fake_client.closed is True + assert clients["room-2"].closed is True @pytest.mark.asyncio async def test_forwards_raw_codex_task_events(self) -> None: @@ -781,10 +775,7 @@ async def test_forwards_raw_codex_task_events(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -828,12 +819,8 @@ async def test_can_disable_synthetic_turn_task_markers(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - emit_turn_task_markers=False, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_turn_task_markers=False) ) tools = ToolSchemaFakeTools() @@ -871,12 +858,8 @@ async def test_raw_task_event_without_explicit_task_id_does_not_emit_uuid( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - emit_turn_task_markers=False, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_turn_task_markers=False) ) tools = ToolSchemaFakeTools() @@ -905,10 +888,7 @@ async def test_raw_task_event_without_explicit_task_id_does_not_emit_uuid( @pytest.mark.asyncio async def test_status_command_returns_state_without_starting_turn(self) -> None: fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -932,10 +912,7 @@ async def test_status_command_returns_state_without_starting_turn(self) -> None: @pytest.mark.asyncio async def test_model_command_sets_override_without_starting_turn(self) -> None: fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -952,7 +929,7 @@ async def test_model_command_sets_override_without_starting_turn(self) -> None: methods = [method for method, _ in fake_client.requests] assert "turn/start" not in methods assert "thread/start" not in methods - assert adapter.config.model == "gpt-5.5-codex" + assert adapter._selected_model == "gpt-5.5-codex" assert len(tools.messages_sent) == 1 assert ( "Model override set to `gpt-5.5-codex`" in tools.messages_sent[0]["content"] @@ -961,10 +938,7 @@ async def test_model_command_sets_override_without_starting_turn(self) -> None: @pytest.mark.asyncio async def test_models_alias_lists_models_without_starting_turn(self) -> None: fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -1001,13 +975,11 @@ async def test_reasoning_effort_passed_in_turn_overrides(self) -> None: ), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig( - transport="ws", - reasoning_effort="high", - reasoning_summary="concise", + reasoning_effort="high", reasoning_summary="concise" ), - client_factory=lambda _config: fake_client, ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -1042,10 +1014,7 @@ async def test_reasoning_effort_omitted_when_none(self) -> None: ), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") await adapter.on_message( @@ -1066,10 +1035,7 @@ async def test_reasoning_effort_omitted_when_none(self) -> None: @pytest.mark.asyncio async def test_reasoning_command_sets_effort(self) -> None: fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") await adapter.on_message( @@ -1081,17 +1047,15 @@ async def test_reasoning_command_sets_effort(self) -> None: is_session_bootstrap=True, room_id="room-1", ) - assert adapter.config.reasoning_effort == "high" + room = adapter._room_clients["room-1"] + assert room.reasoning_effort == "high" assert len(tools.messages_sent) == 1 assert "Reasoning effort set to `high`" in tools.messages_sent[0]["content"] @pytest.mark.asyncio async def test_reasoning_command_rejects_invalid_effort(self) -> None: fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") await adapter.on_message( @@ -1123,9 +1087,8 @@ async def test_self_config_tools_registered_when_enabled(self) -> None: ), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", enable_self_config_tools=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(enable_self_config_tools=True) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -1164,9 +1127,8 @@ async def test_self_config_tools_not_registered_when_disabled(self) -> None: ), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", enable_self_config_tools=False), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(enable_self_config_tools=False) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -1203,9 +1165,8 @@ async def test_setmodel_tool_changes_model(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", enable_self_config_tools=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(enable_self_config_tools=True) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -1218,7 +1179,6 @@ async def test_setmodel_tool_changes_model(self) -> None: is_session_bootstrap=True, room_id="room-1", ) - assert adapter.config.model == "o3" assert adapter._selected_model == "o3" # Verify the tool response was sent back tool_responses = [ @@ -1245,9 +1205,8 @@ async def test_setreasoning_tool_changes_effort(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", enable_self_config_tools=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(enable_self_config_tools=True) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -1260,8 +1219,9 @@ async def test_setreasoning_tool_changes_effort(self) -> None: is_session_bootstrap=True, room_id="room-1", ) - assert adapter.config.reasoning_effort == "xhigh" - assert adapter.config.reasoning_summary == "detailed" + room = adapter._room_clients["room-1"] + assert room.reasoning_effort == "xhigh" + assert room.reasoning_summary == "detailed" @pytest.mark.asyncio async def test_setreasoning_tool_rejects_invalid_effort(self) -> None: @@ -1278,9 +1238,8 @@ async def test_setreasoning_tool_rejects_invalid_effort(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", enable_self_config_tools=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(enable_self_config_tools=True) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -1309,12 +1268,8 @@ async def test_setreasoning_tool_rejects_invalid_effort(self) -> None: async def test_sandbox_alias_is_normalized_for_thread_and_turn(self) -> None: events = [_turn_completed()] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - sandbox="dangerFullAccess", - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(sandbox="dangerFullAccess") ) tools = ToolSchemaFakeTools() @@ -1346,12 +1301,8 @@ async def test_sandbox_alias_is_normalized_for_thread_and_turn(self) -> None: async def test_external_sandbox_alias_uses_sandbox_policy(self) -> None: events = [_turn_completed()] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - sandbox="external-sandbox", - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(sandbox="external-sandbox") ) tools = ToolSchemaFakeTools() @@ -1391,10 +1342,7 @@ async def test_transport_closed_event_aborts_turn(self) -> None: ) ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -1424,10 +1372,7 @@ async def test_transport_closed_resets_client_state(self) -> None: ) ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -1458,10 +1403,7 @@ async def test_transport_closed_clears_per_room_state(self) -> None: ) ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -1499,10 +1441,7 @@ async def test_transport_closed_drains_token_usage_for_dead_threads( ) ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -1532,9 +1471,8 @@ async def test_turn_timeout_sends_interrupt_and_clean_error(self) -> None: """When recv_event times out, the adapter sends turn/interrupt and reports cleanly.""" # No events means FakeCodexClient raises asyncio.TimeoutError immediately. fake_client = FakeCodexClient(events=[]) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", turn_timeout_s=0.01), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(turn_timeout_s=0.01) ) tools = ToolSchemaFakeTools() @@ -1585,10 +1523,7 @@ async def test_item_completed_text_overrides_accumulated_deltas(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -1621,7 +1556,7 @@ def get_weather(inp: WeatherInput) -> str: custom_tools: list[CustomToolDef] = [(WeatherInput, get_weather)] adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), + config=CodexAdapterConfig(), additional_tools=custom_tools, ) @@ -1665,10 +1600,8 @@ async def calculate(inp: CalculatorInput) -> str: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - additional_tools=custom_tools, - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), additional_tools=custom_tools ) tools = ToolSchemaFakeTools() @@ -1709,10 +1642,8 @@ async def test_execution_reporting_emits_tool_call_and_result_events(self) -> No _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -1752,10 +1683,8 @@ async def test_send_room_file_tool_call_event_redacts_content(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -1793,11 +1722,7 @@ async def test_execution_reporting_silenced_with_explicit_empty_emit(self) -> No _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=(), - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig(), emit=()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -1844,10 +1769,10 @@ async def fail_func(inp: FailInput) -> str: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), + adapter = make_codex_adapter( + fake_client, + config=CodexAdapterConfig(), additional_tools=custom_tools, - client_factory=lambda _config: fake_client, emit=Emit.TOOL_CALLS, ) tools = ToolSchemaFakeTools() @@ -1896,10 +1821,8 @@ async def test_execution_reporting_emitted_for_platform_output_tools( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -1960,10 +1883,8 @@ async def test_item_completed_mcpToolCall_send_room_file_redacts_content( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2007,10 +1928,8 @@ async def test_item_completed_commandExecution_emits_tool_events(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2063,10 +1982,8 @@ async def test_item_completed_fileChange_emits_tool_events(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2111,10 +2028,8 @@ async def test_item_completed_fileChange_missing_changes_is_safe(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2153,10 +2068,8 @@ async def test_item_completed_imageView_emits_tool_events(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2200,10 +2113,8 @@ async def test_item_completed_collabAgentToolCall_emits_tool_events(self) -> Non _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2251,10 +2162,7 @@ async def test_item_completed_collabAgentToolCall_non_text_list_result_preserves _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -2293,10 +2201,8 @@ async def test_item_completed_mcpToolCall_emits_tool_events(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2353,10 +2259,7 @@ async def test_item_completed_mcpToolCall_non_text_list_result_preserves_data( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -2396,10 +2299,8 @@ async def test_item_completed_dynamicToolCall_emits_tool_events(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2456,10 +2357,7 @@ async def test_item_completed_dynamicToolCall_non_text_list_result_falls_back_to _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -2498,10 +2396,8 @@ async def test_item_completed_reasoning_emits_thought(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.THOUGHTS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.THOUGHTS ) tools = ToolSchemaFakeTools() @@ -2541,10 +2437,8 @@ async def test_item_completed_dict_summary_text_emits_thought(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit={Emit.THOUGHTS}, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit={Emit.THOUGHTS} ) tools = ToolSchemaFakeTools() @@ -2601,10 +2495,8 @@ async def test_item_completed_empty_reasoning_summary_skips_thought(self) -> Non _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit={Emit.THOUGHTS}, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit={Emit.THOUGHTS} ) tools = ToolSchemaFakeTools() @@ -2637,10 +2529,8 @@ async def test_item_completed_empty_plan_text_skips_thought(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit={Emit.THOUGHTS}, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit={Emit.THOUGHTS} ) tools = ToolSchemaFakeTools() @@ -2686,10 +2576,8 @@ async def test_item_completed_skipped_when_reporting_disabled(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TASK_EVENTS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TASK_EVENTS ) tools = ToolSchemaFakeTools() @@ -2740,10 +2628,8 @@ async def test_item_completed_agentMessage_still_sets_final_text(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2782,10 +2668,8 @@ async def test_item_completed_webSearch_emits_tool_events(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2826,10 +2710,7 @@ async def test_item_completed_webSearch_non_text_list_action_preserves_data( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -2866,10 +2747,8 @@ async def test_item_completed_metadata_includes_codex_ids(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - emit=Emit.TOOL_CALLS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS ) tools = ToolSchemaFakeTools() @@ -2901,10 +2780,7 @@ async def test_history_injected_on_resume_failure(self) -> None: events=events, resume_error=CodexJsonRpcError(code=-32002, message="Thread expired"), ) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -2961,10 +2837,7 @@ async def test_history_not_injected_on_successful_resume(self) -> None: """Resume succeeds, no history injection.""" events = [_turn_completed()] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -3010,12 +2883,9 @@ async def test_history_not_injected_when_disabled(self) -> None: events=events, resume_error=CodexJsonRpcError(code=-32002, message="Thread expired"), ) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - inject_history_on_resume_failure=False, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, + config=CodexAdapterConfig(inject_history_on_resume_failure=False), ) tools = ToolSchemaFakeTools() @@ -3054,10 +2924,7 @@ async def test_history_filters_non_text_messages(self) -> None: events=events, resume_error=CodexJsonRpcError(code=-32002, message="Not found"), ) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -3134,9 +3001,8 @@ async def test_history_respects_max_messages(self) -> None: events=events, resume_error=CodexJsonRpcError(code=-32002, message="Not found"), ) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", max_history_messages=3), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(max_history_messages=3) ) tools = ToolSchemaFakeTools() @@ -3193,10 +3059,7 @@ async def test_history_cleared_after_injection(self) -> None: events=events, resume_error=CodexJsonRpcError(code=-32002, message="Not found"), ) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Codex Agent", "A coding agent") @@ -3240,10 +3103,7 @@ async def test_auto_selected_model_error_propagates_without_retry(self) -> None: ] }, ) - adapter = CodexAdapter( - config=CodexAdapterConfig(model=None), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig(model=None)) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "An agent") @@ -3274,11 +3134,11 @@ async def test_model_selection_uses_first_visible_model(self) -> None: ] }, ) - adapter = CodexAdapter( - config=CodexAdapterConfig(model=None), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig(model=None)) await adapter.on_started("Agent", "An agent") + adapter._room_client("room-1") + adapter._active_room.set("room-1") + await adapter._ensure_client_ready() assert adapter._selected_model == "gpt-5.4-mini" @@ -3298,9 +3158,8 @@ async def test_explicit_model_error_propagates_without_fallback(self) -> None: ] }, ) - adapter = CodexAdapter( - config=CodexAdapterConfig(model="unavailable-test-model"), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(model="unavailable-test-model") ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "An agent") @@ -3324,11 +3183,11 @@ async def test_explicit_model_error_propagates_without_fallback(self) -> None: async def test_model_selection_uses_default_when_model_list_empty(self) -> None: """Auto-selection uses the adapter default when Codex returns no visible models.""" fake_client = FakeCodexClient(model_list_result={"data": []}) - adapter = CodexAdapter( - config=CodexAdapterConfig(model=None), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig(model=None)) await adapter.on_started("Agent", "An agent") + adapter._room_client("room-1") + adapter._active_room.set("room-1") + await adapter._ensure_client_ready() assert adapter._selected_model == "gpt-5.5" @@ -3351,11 +3210,11 @@ async def request( ) fake_client = ModelListFailsClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(model=None), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig(model=None)) await adapter.on_started("Agent", "An agent") + adapter._room_client("room-1") + adapter._active_room.set("room-1") + await adapter._ensure_client_ready() assert adapter._selected_model == "gpt-5.5" @@ -3368,10 +3227,7 @@ async def test_non_model_error_propagates(self) -> None: ), turn_start_error_once=False, ) - adapter = CodexAdapter( - config=CodexAdapterConfig(model=None), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig(model=None)) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "An agent") @@ -3393,14 +3249,11 @@ async def test_startup_config_logged( ) -> None: """Startup emits a redacted config summary log line.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig( - transport="stdio", - model="gpt-5.5", - sandbox="workspace-write", - approval_mode="manual", + model="gpt-5.5", sandbox="workspace-write", approval_mode="manual" ), - client_factory=lambda _config: fake_client, ) with caplog.at_level("INFO", logger="band.adapters.codex"): @@ -3432,10 +3285,7 @@ async def test_codex_error_emits_event_unconditionally(self) -> None: ), ], ) - adapter = CodexAdapter( - config=CodexAdapterConfig(), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "An agent") @@ -3457,9 +3307,9 @@ async def test_codex_error_emits_event_unconditionally(self) -> None: @pytest.mark.asyncio async def test_cleanup_before_start(self) -> None: """Calling on_cleanup on a freshly constructed adapter should not raise.""" - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="stdio"), - client_factory=lambda _config: FakeCodexClient(), + adapter = make_codex_adapter( + FakeCodexClient(), + config=CodexAdapterConfig(), ) # No on_started called — cleanup should be safe (idempotent) await adapter.on_cleanup("room-x") @@ -3468,10 +3318,7 @@ async def test_cleanup_before_start(self) -> None: async def test_cleanup_clears_pending_approvals(self) -> None: """on_cleanup should evict all pending approvals for the given room.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="stdio"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) await adapter.on_started("Bot", "desc") # Manually inject a pending approval for room-1 @@ -3490,9 +3337,8 @@ async def test_cleanup_clears_pending_approvals(self) -> None: }, )(), } - # Also register a room thread so the client isn't closed + wire_codex_room(adapter, fake_client, "room-1") adapter._room_threads["room-1"] = "thr-1" - adapter._room_threads["room-2"] = "thr-2" await adapter.on_cleanup("room-1") @@ -3525,10 +3371,7 @@ async def execute_tool_call_structured( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ValidationErrorTools() await adapter.on_started("Bot", "desc") @@ -3580,9 +3423,8 @@ async def test_structured_error_from_error_event(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", structured_errors=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(structured_errors=True) ) tools = ToolSchemaFakeTools() @@ -3628,9 +3470,8 @@ async def test_structured_error_from_failed_turn(self) -> None: ), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", structured_errors=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(structured_errors=True) ) tools = ToolSchemaFakeTools() @@ -3669,9 +3510,8 @@ async def test_structured_errors_disabled_falls_back_to_plain_text(self) -> None _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", structured_errors=False), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(structured_errors=False) ) tools = ToolSchemaFakeTools() @@ -3711,9 +3551,8 @@ async def test_approve_session_auto_approves_subsequent_requests(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=first_events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", approval_mode="manual"), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(approval_mode="manual") ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -3765,12 +3604,8 @@ async def test_approval_audit_trail_emitted(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - approval_mode="auto_decline", - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(approval_mode="auto_decline") ) tools = ToolSchemaFakeTools() @@ -3798,10 +3633,7 @@ async def test_approval_audit_trail_emitted(self) -> None: async def test_sandbox_command_changes_mode(self) -> None: """The /sandbox command sets a per-room override, not mutating global config.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") await adapter.on_message( @@ -3823,10 +3655,7 @@ async def test_sandbox_command_changes_mode(self) -> None: async def test_sandbox_command_is_per_room(self) -> None: """Sandbox override in one room does not affect other rooms.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) await adapter.on_started("Agent", "A coding agent") adapter._sandbox_overrides["room-1"] = "read-only" @@ -3838,10 +3667,7 @@ async def test_sandbox_command_is_per_room(self) -> None: async def test_sandbox_danger_full_access_requires_confirm_flag(self) -> None: """Escalating to danger-full-access without --confirm shows a prompt.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") await adapter.on_message( @@ -3865,10 +3691,7 @@ async def test_sandbox_escalation_to_danger_full_access_logs_warning( ) -> None: """Escalating to danger-full-access with --confirm logs a warning.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") with caplog.at_level(logging.WARNING, logger="band.adapters.codex"): @@ -3892,10 +3715,7 @@ async def test_sandbox_escalation_to_danger_full_access_logs_warning( async def test_sandbox_command_rejects_invalid_mode(self) -> None: """The /sandbox command rejects invalid modes.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") await adapter.on_message( @@ -3916,10 +3736,7 @@ async def test_sandbox_command_rejects_invalid_mode(self) -> None: async def test_permissions_command_shows_state(self) -> None: """/permissions shows current effective permissions.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") await adapter.on_message( @@ -3961,9 +3778,8 @@ async def test_plan_steps_forwarded(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", stream_plan_events=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(stream_plan_events=True) ) tools = ToolSchemaFakeTools() @@ -4007,9 +3823,8 @@ async def test_plan_steps_not_forwarded_when_disabled(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", stream_plan_events=False), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(stream_plan_events=False) ) tools = ToolSchemaFakeTools() @@ -4038,12 +3853,8 @@ async def test_turn_lifecycle_events_emitted(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - emit_turn_lifecycle_events=True, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_turn_lifecycle_events=True) ) tools = ToolSchemaFakeTools() @@ -4078,10 +3889,7 @@ async def test_threads_command_lists_mappings(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -4122,10 +3930,7 @@ async def test_thread_archive_clears_mapping(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -4170,9 +3975,8 @@ async def test_reasoning_delta_streamed_as_thought(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", stream_reasoning_events=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(stream_reasoning_events=True) ) tools = ToolSchemaFakeTools() @@ -4207,9 +4011,8 @@ async def test_reasoning_delta_ignored_when_disabled(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", stream_reasoning_events=False), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(stream_reasoning_events=False) ) tools = ToolSchemaFakeTools() @@ -4240,9 +4043,8 @@ async def test_plan_delta_streamed_as_thought(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", stream_plan_events=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(stream_plan_events=True) ) tools = ToolSchemaFakeTools() @@ -4288,9 +4090,8 @@ async def test_commentary_phase_streamed_as_thought(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", stream_commentary_events=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(stream_commentary_events=True) ) tools = ToolSchemaFakeTools() @@ -4339,9 +4140,8 @@ async def test_commentary_excluded_from_final_text_when_streaming_enabled( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", stream_commentary_events=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(stream_commentary_events=True) ) tools = ToolSchemaFakeTools() @@ -4385,9 +4185,8 @@ async def test_commentary_included_in_final_text_when_streaming_disabled( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", stream_commentary_events=False), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(stream_commentary_events=False) ) tools = ToolSchemaFakeTools() @@ -4427,12 +4226,8 @@ async def test_diff_event_forwarded(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - emit_diff_events=True, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_diff_events=True) ) tools = ToolSchemaFakeTools() @@ -4469,13 +4264,8 @@ async def test_diff_event_requires_task_events_emit(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - emit_diff_events=True, - ), - client_factory=lambda _config: fake_client, - emit=(), + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_diff_events=True), emit=() ) tools = ToolSchemaFakeTools() @@ -4515,9 +4305,8 @@ async def test_token_usage_tracked_and_emitted(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", emit_token_usage_events=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_token_usage_events=True) ) tools = ToolSchemaFakeTools() @@ -4559,9 +4348,8 @@ async def test_token_usage_ignored_when_disabled(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", emit_token_usage_events=False), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_token_usage_events=False) ) tools = ToolSchemaFakeTools() @@ -4608,10 +4396,7 @@ async def test_usage_command_shows_token_usage(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -4837,12 +4622,8 @@ async def test_session_auto_approves_matching_command_binary(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - approval_mode="manual", - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(approval_mode="manual") ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -4881,12 +4662,8 @@ async def test_session_does_not_auto_approve_different_command(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - approval_mode="auto_decline", - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(approval_mode="auto_decline") ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -4922,13 +4699,11 @@ async def test_session_binary_granularity_approves_same_binary(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig( - transport="ws", - approval_mode="manual", - session_approval_granularity="binary", + approval_mode="manual", session_approval_granularity="binary" ), - client_factory=lambda _config: fake_client, ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -4974,10 +4749,7 @@ async def test_on_cleanup_removes_per_room_token_usage(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5006,9 +4778,9 @@ async def test_on_cleanup_removes_per_room_token_usage(self) -> None: class TestAuditCap: def test_audit_trail_capped_at_limit(self) -> None: """Approval audit trail is capped at max_approval_audit_per_room.""" - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", max_approval_audit_per_room=5), - client_factory=lambda _config: FakeCodexClient(), + adapter = make_codex_adapter( + FakeCodexClient(), + config=CodexAdapterConfig(max_approval_audit_per_room=5), ) for i in range(10): adapter._record_approval_audit( @@ -5026,9 +4798,9 @@ def test_audit_trail_capped_at_limit(self) -> None: def test_session_approved_capped_at_limit(self) -> None: """Session approvals evict LRU when max_session_approved_per_room is hit.""" - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", max_session_approved_per_room=3), - client_factory=lambda _config: FakeCodexClient(), + adapter = make_codex_adapter( + FakeCodexClient(), + config=CodexAdapterConfig(max_session_approved_per_room=3), ) for i in range(5): adapter._record_session_approval("room-1", f"commandExecution:cmd{i}") @@ -5041,9 +4813,9 @@ def test_session_approved_capped_at_limit(self) -> None: def test_session_approval_reinsert_moves_to_end(self) -> None: """Re-approving an existing key moves it to the most-recent slot.""" - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", max_session_approved_per_room=3), - client_factory=lambda _config: FakeCodexClient(), + adapter = make_codex_adapter( + FakeCodexClient(), + config=CodexAdapterConfig(max_session_approved_per_room=3), ) adapter._record_session_approval("room-1", "commandExecution:a") adapter._record_session_approval("room-1", "commandExecution:b") @@ -5068,12 +4840,8 @@ class TestReviewFixes: async def test_sandbox_command_blocked_when_sandbox_policy_set(self) -> None: """/sandbox is rejected when sandbox_policy is configured.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - sandbox_policy={"type": "readOnly"}, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(sandbox_policy={"type": "readOnly"}) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5097,10 +4865,7 @@ async def test_thread_archive_clears_raw_history(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5167,12 +4932,8 @@ async def recv_event(self, timeout_s: float | None = None) -> RpcEvent: raise ConnectionError("transport died") fake_client = BrokenClient() - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - fallback_send_agent_text=True, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(fallback_send_agent_text=True) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5208,9 +4969,8 @@ async def test_approve_session_sends_accept_for_session_decision(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", approval_mode="manual"), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(approval_mode="manual") ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5260,9 +5020,8 @@ async def test_session_auto_approval_sends_accept_for_session(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", approval_mode="manual"), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(approval_mode="manual") ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5305,9 +5064,8 @@ async def test_network_context_included_in_approval_metadata(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", approval_mode="manual"), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(approval_mode="manual") ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5357,12 +5115,8 @@ async def test_turn_started_lifecycle_event_emitted(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - emit_turn_lifecycle_events=True, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_turn_lifecycle_events=True) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5400,12 +5154,8 @@ async def test_context_compaction_event_emitted(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - emit_turn_lifecycle_events=True, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_turn_lifecycle_events=True) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5438,12 +5188,8 @@ async def test_context_compaction_ignored_when_disabled(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - emit_turn_lifecycle_events=False, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_turn_lifecycle_events=False) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5625,9 +5371,8 @@ async def test_approve_session_rejects_empty_session_key( _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", approval_mode="manual"), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(approval_mode="manual") ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5697,10 +5442,7 @@ async def test_token_usage_event_skipped_when_total_is_zero(self) -> None: """ fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5744,7 +5486,7 @@ class TestSessionApprovalKeying: def test_file_change_session_key_requires_paths(self) -> None: """/approve-session must refuse fileChange requests with no paths.""" - adapter = CodexAdapter(config=CodexAdapterConfig(transport="ws")) + adapter = CodexAdapter(config=CodexAdapterConfig()) key = adapter._session_approval_key( "item/fileChange/requestApproval", {"reason": "something vague"}, @@ -5753,7 +5495,7 @@ def test_file_change_session_key_requires_paths(self) -> None: def test_file_change_session_key_uses_paths_when_present(self) -> None: """fileChange session key includes sorted path list for stable matching.""" - adapter = CodexAdapter(config=CodexAdapterConfig(transport="ws")) + adapter = CodexAdapter(config=CodexAdapterConfig()) key1 = adapter._session_approval_key( "item/fileChange/requestApproval", {"changes": [{"path": "b.py"}, {"path": "a.py"}]}, @@ -5767,7 +5509,7 @@ def test_file_change_session_key_uses_paths_when_present(self) -> None: def test_file_change_session_key_handles_top_level_paths(self) -> None: """fileChange session key also picks up top-level path/paths fields.""" - adapter = CodexAdapter(config=CodexAdapterConfig(transport="ws")) + adapter = CodexAdapter(config=CodexAdapterConfig()) key = adapter._session_approval_key( "item/fileChange/requestApproval", {"paths": ["src/foo.py", "src/bar.py"]}, @@ -5777,7 +5519,7 @@ def test_file_change_session_key_handles_top_level_paths(self) -> None: def test_unknown_approval_method_returns_empty_key(self) -> None: """Session-level approval refuses unknown methods rather than bucketing them.""" - adapter = CodexAdapter(config=CodexAdapterConfig(transport="ws")) + adapter = CodexAdapter(config=CodexAdapterConfig()) assert adapter._session_approval_key("item/unknown/requestApproval", {}) == "" @pytest.mark.asyncio @@ -5791,14 +5533,13 @@ async def test_approve_session_refused_for_fileChange_without_paths(self) -> Non ), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig( - transport="ws", approval_mode="manual", approval_wait_timeout_s=0.05, approval_timeout_decision="decline", ), - client_factory=lambda _config: fake_client, ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -5865,7 +5606,7 @@ class TestApprovalAuditRecording: def test_record_approval_audit_returns_entry(self) -> None: """_record_approval_audit returns the entry it appended.""" - adapter = CodexAdapter(config=CodexAdapterConfig(transport="ws")) + adapter = CodexAdapter(config=CodexAdapterConfig()) entry = adapter._record_approval_audit( room_id="room-1", request_id="req-1", @@ -5944,10 +5685,7 @@ class TestSlashCommandCoverage: async def test_thread_info_with_no_mapping(self) -> None: """/thread info reports gracefully when the room has no thread yet.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") await adapter.on_message( @@ -5980,12 +5718,8 @@ async def test_thread_info_includes_thread_and_usage(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - emit_token_usage_events=True, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_token_usage_events=True) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -6019,10 +5753,7 @@ async def test_thread_info_includes_thread_and_usage(self) -> None: async def test_permissions_reflects_sandbox_override(self) -> None: """/permissions reports the per-room sandbox override once set.""" fake_client = FakeCodexClient() - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") await adapter.on_message( @@ -6062,9 +5793,8 @@ async def test_error_event_with_non_dict_error_field(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", structured_errors=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(structured_errors=True) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -6089,10 +5819,7 @@ async def test_turn_completed_without_items_key(self) -> None: ), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _config: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -6117,9 +5844,8 @@ async def test_turn_plan_updated_with_garbage_steps(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", stream_plan_events=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(stream_plan_events=True) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -6146,13 +5872,11 @@ async def test_pending_approvals_cleared_on_room_cleanup(self) -> None: not that the natural approval timeout fired first. """ fake_client = FakeCodexClient(events=[]) - adapter = CodexAdapter( + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig( - transport="ws", - approval_mode="manual", - approval_wait_timeout_s=30.0, + approval_mode="manual", approval_wait_timeout_s=30.0 ), - client_factory=lambda _config: fake_client, ) await adapter.on_started("Agent", "A coding agent") @@ -6160,6 +5884,7 @@ async def test_pending_approvals_cleared_on_room_cleanup(self) -> None: loop = asyncio.get_running_loop() approval_future: asyncio.Future[str] = loop.create_future() + wire_codex_room(adapter, fake_client, "room-1") adapter._room_threads["room-1"] = "thr-1" adapter._pending_approvals["room-1"] = { "token-1": PendingApproval( @@ -6190,12 +5915,8 @@ async def test_no_lifecycle_events_when_disabled(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - emit_turn_lifecycle_events=False, - ), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_turn_lifecycle_events=False) ) tools = ToolSchemaFakeTools() await adapter.on_started("Agent", "A coding agent") @@ -6356,9 +6077,8 @@ async def test_multibyte_diff_respects_byte_budget(self) -> None: _turn_completed(), ] fake_client = FakeCodexClient(events=events) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws", emit_diff_events=True), - client_factory=lambda _config: fake_client, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(emit_diff_events=True) ) tools = ToolSchemaFakeTools() @@ -6444,13 +6164,11 @@ async def test_warns_when_both_channels_enabled( self, caplog: pytest.LogCaptureFixture ) -> None: fake_client = FakeCodexClient() - adapter = CodexAdapter( + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig( - transport="ws", - emit_turn_task_markers=True, - emit_turn_lifecycle_events=True, + emit_turn_task_markers=True, emit_turn_lifecycle_events=True ), - client_factory=lambda _config: fake_client, ) with caplog.at_level(logging.WARNING, logger="band.adapters.codex"): await adapter.on_started("Agent", "A coding agent") @@ -6463,13 +6181,11 @@ async def test_no_warning_when_only_one_channel_enabled( self, caplog: pytest.LogCaptureFixture ) -> None: fake_client = FakeCodexClient() - adapter = CodexAdapter( + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig( - transport="ws", - emit_turn_task_markers=True, - emit_turn_lifecycle_events=False, + emit_turn_task_markers=True, emit_turn_lifecycle_events=False ), - client_factory=lambda _config: fake_client, ) with caplog.at_level(logging.WARNING, logger="band.adapters.codex"): await adapter.on_started("Agent", "A coding agent") diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index 6433f2315..71037be7d 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -29,12 +29,10 @@ def test_is_acp_client_adapter(self) -> None: def test_defaults_to_stdio_copilot_command(self) -> None: adapter = CopilotACPAdapter() assert adapter._command == list(DEFAULT_COPILOT_COMMAND) - assert adapter._host is None - assert adapter._port is None def test_no_config_equivalent_to_default_config(self) -> None: a, b = CopilotACPAdapter(), CopilotACPAdapter(CopilotACPAdapterConfig()) - for attr in ("_command", "_host", "_port", "_env", "_inject_band_tools"): + for attr in ("_command", "_env", "_inject_band_tools"): assert getattr(a, attr) == getattr(b, attr) def test_custom_command_is_forwarded(self) -> None: @@ -48,7 +46,7 @@ def test_no_profile_uses_default_noop(self) -> None: # collecting client the runtime builds falls back to the no-op profile. adapter = CopilotACPAdapter() assert adapter._profile is None - client = adapter._runtime._client_factory() + client = adapter._build_runtime()._client_factory() assert isinstance(client._profile, NoopACPClientProfile) def test_github_token_injected_into_stdio_env(self) -> None: @@ -109,48 +107,14 @@ def _echo(text: str) -> str: class TestCopilotACPAdapterTcpTransport: - def test_host_port_selects_tcp_and_empty_command(self) -> None: - adapter = CopilotACPAdapter(CopilotACPAdapterConfig(host="10.0.0.5", port=8080)) - assert adapter._host == "10.0.0.5" - assert adapter._port == 8080 - assert adapter._command == [] - - def test_tcp_does_not_inject_env(self) -> None: - # Over TCP the already-running server carries its own environment; neither - # the token nor a general env is smuggled through (they'd be ignored anyway). - adapter = CopilotACPAdapter( - CopilotACPAdapterConfig( - host="10.0.0.5", - port=8080, - github_token="ghp_x", - env={"COPILOT_GITHUB_TOKEN": "tok"}, - ) - ) - assert adapter._env is None - - def test_tcp_with_auth_warns_it_is_ignored( - self, caplog: pytest.LogCaptureFixture - ) -> None: - # Symmetric with the loud command+TCP error: dropping auth over TCP is - # surfaced, not silent, so a caller can't believe auth is configured. - with caplog.at_level(logging.WARNING, logger="band.adapters.copilot_acp"): - CopilotACPAdapter( - CopilotACPAdapterConfig( - host="10.0.0.5", port=8080, github_token="ghp_x" - ) - ) - assert any("ignored over TCP" in r.message for r in caplog.records) - - def test_tcp_without_auth_does_not_warn( - self, caplog: pytest.LogCaptureFixture - ) -> None: - with caplog.at_level(logging.WARNING, logger="band.adapters.copilot_acp"): + def test_tcp_config_is_rejected(self) -> None: + with pytest.raises( + ValueError, + match="TCP ACP transport cannot guarantee room process isolation", + ): CopilotACPAdapter(CopilotACPAdapterConfig(host="10.0.0.5", port=8080)) - assert not any("ignored over TCP" in r.message for r in caplog.records) def test_custom_command_with_tcp_is_rejected(self) -> None: - # A non-default command AND host/port is a misconfiguration; fail loudly - # rather than silently dropping the command. with pytest.raises(ValueError, match="not both"): CopilotACPAdapter( CopilotACPAdapterConfig( @@ -158,7 +122,18 @@ def test_custom_command_with_tcp_is_rejected(self) -> None: ) ) - def test_default_command_with_tcp_is_allowed(self) -> None: - # The default command is not "set" for exclusivity purposes — TCP is fine. - adapter = CopilotACPAdapter(CopilotACPAdapterConfig(host="10.0.0.5", port=8080)) - assert adapter._host == "10.0.0.5" + def test_tcp_with_auth_warns_before_rejection( + self, caplog: pytest.LogCaptureFixture + ) -> None: + # CopilotACPAdapter warns about ignored auth before the base adapter rejects TCP. + with caplog.at_level(logging.WARNING, logger="band.adapters.copilot_acp"): + with pytest.raises( + ValueError, + match="TCP ACP transport cannot guarantee room process isolation", + ): + CopilotACPAdapter( + CopilotACPAdapterConfig( + host="10.0.0.5", port=8080, github_token="ghp_x" + ) + ) + assert any("ignored over TCP" in r.message for r in caplog.records) diff --git a/tests/adapters/test_room_workspace_isolation.py b/tests/adapters/test_room_workspace_isolation.py index 573adf846..b0c41151a 100644 --- a/tests/adapters/test_room_workspace_isolation.py +++ b/tests/adapters/test_room_workspace_isolation.py @@ -58,7 +58,9 @@ class Client: def __init__(self) -> None: self.params: dict[str, object] | None = None - async def request(self, method: str, params: dict[str, object]) -> dict[str, object]: + async def request( + self, method: str, params: dict[str, object] + ) -> dict[str, object]: assert method == "thread/start" self.params = params return {"thread": {"id": "thread"}} diff --git a/tests/integrations/acp/acp_toolkit/__init__.py b/tests/integrations/acp/acp_toolkit/__init__.py index 10d2f9d8e..c4ef7b275 100644 --- a/tests/integrations/acp/acp_toolkit/__init__.py +++ b/tests/integrations/acp/acp_toolkit/__init__.py @@ -30,6 +30,7 @@ RoomActivity, TranscriptTools, acp_adapter, + inject_acp_spawn, live_line, make_acp_connection, ) @@ -43,6 +44,7 @@ "RoomActivity", "TranscriptTools", "acp_adapter", + "inject_acp_spawn", "live_line", "make_acp_connection", ] diff --git a/tests/integrations/acp/acp_toolkit/harness.py b/tests/integrations/acp/acp_toolkit/harness.py index 1b914eda0..e4d1ca5f3 100644 --- a/tests/integrations/acp/acp_toolkit/harness.py +++ b/tests/integrations/acp/acp_toolkit/harness.py @@ -17,8 +17,9 @@ from acp.agent.connection import AgentSideConnection from band.core.types import PlatformMessage -from band.integrations.acp.client_adapter import ACPClientAdapter -from band.integrations.acp.client_types import ACPClientSessionState +from band.integrations.acp.client_adapter import ACPClientAdapter, _resolve_launcher +from band.integrations.acp.client_runtime import ACPRuntime +from band.integrations.acp.client_types import ACPClientSessionState, BandACPClient from band.integrations.acp.types import ToolCallRoomEvent, ToolResultRoomEvent from band.testing import FakeAgentTools @@ -104,7 +105,7 @@ def make_acp_connection(*, http: bool = True, sse: bool = False) -> AsyncMock: class FakeSpawn: """A fake ``spawn_process`` seam: records calls (spy) and yields a scripted conn. - Drop-in for the injectable ``spawn_process`` on ``ACPClientAdapter``/``ACPRuntime`` + Drop-in for the injectable ``spawn_process`` on ``ACPRuntime`` so tests exercise the real transport seam by dependency injection instead of patching module globals. The instance *is* the callable and returns an async context manager, matching the runtime's contract: @@ -131,6 +132,26 @@ def last_kwargs(self) -> dict[str, Any]: return self.calls[-1][1] +def inject_acp_spawn( + adapter: ACPClientAdapter, spawn: FakeSpawn | Callable[..., Any] +) -> None: + """Patch ``adapter._build_runtime`` so each room runtime uses ``spawn``.""" + + def _build_runtime() -> ACPRuntime: + return ACPRuntime( + command=_resolve_launcher(adapter._command), + env=adapter._env, + auth_method=adapter._auth_method, + client_factory=lambda: BandACPClient( + profile=adapter._profile, + canonicalize_tool_name=adapter._canonical_tool_name, + ), + spawn_process=spawn, + ) + + adapter._build_runtime = _build_runtime # type: ignore[method-assign] + + @dataclass class Reply: """A readable view of what the adapter posted back for one turn.""" @@ -267,10 +288,10 @@ async def acp_adapter( """ adapter = ACPClientAdapter( command="fake-agent", # ignored — the injected transport pairs us with agent - spawn_process=_pair_in_process(agent), inject_band_tools=inject_band_tools, **adapter_kwargs, ) + inject_acp_spawn(adapter, _pair_in_process(agent)) await adapter.on_started("Fake Agent", "in-process fake") try: yield AcpSession(adapter, agent) diff --git a/tests/integrations/acp/test_client_adapter.py b/tests/integrations/acp/test_client_adapter.py index 8e912665e..b9ae7ef10 100644 --- a/tests/integrations/acp/test_client_adapter.py +++ b/tests/integrations/acp/test_client_adapter.py @@ -3,7 +3,6 @@ from __future__ import annotations import asyncio -import os from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -22,8 +21,11 @@ from band.integrations.acp.types import ACPToolCall, ACPToolResult, CollectedChunk from band.testing import FakeAgentTools +from tests.integrations.acp.acp_toolkit.harness import inject_acp_spawn from tests.integrations.acp.conftest import make_platform_message +_MOCK_ROOM = "room-123" + def permission_events(tools: FakeAgentTools) -> list[dict[str, object]]: """The permission tool_call/tool_result events the handler posted to the room.""" @@ -65,34 +67,26 @@ def test_init_list_command(self) -> None: def test_init_default_values(self) -> None: """Should initialize with default values.""" adapter = ACPClientAdapter(command="codex") - assert adapter._cwd == os.path.abspath(".") assert adapter._env is None assert adapter._mcp_servers == [] - assert adapter._runtime._conn is None - assert adapter._runtime._client is None + assert adapter._runtimes == {} + assert adapter._room_workspaces == {} assert adapter._room_to_session == {} assert adapter._room_tools == {} assert adapter._band_mcp_backend is None - def test_init_codex_acp_uses_absolute_default_cwd(self) -> None: - """Should normalize codex-acp default cwd to an absolute path.""" - adapter = ACPClientAdapter(command="codex-acp") - assert adapter._cwd == os.path.abspath(".") - - def test_init_npx_codex_acp_uses_absolute_default_cwd(self) -> None: - """Should normalize npx codex-acp default cwd to an absolute path.""" - adapter = ACPClientAdapter(command=["npx", "@zed-industries/codex-acp"]) - assert adapter._cwd == os.path.abspath(".") + def test_init_cwd_is_rejected(self) -> None: + """Per-room workspaces replaced the adapter-wide cwd knob.""" + with pytest.raises(ValueError, match="cwd is not supported"): + ACPClientAdapter(command="codex", cwd="/workspace") def test_init_with_custom_values(self) -> None: """Should accept custom configuration.""" adapter = ACPClientAdapter( command="codex", env={"API_KEY": "test"}, - cwd="/workspace", mcp_servers=[{"type": "stdio", "command": "server"}], ) - assert adapter._cwd == os.path.abspath("/workspace") assert adapter._env == {"API_KEY": "test"} assert len(adapter._mcp_servers) == 1 @@ -101,64 +95,74 @@ def test_init_sets_history_converter(self) -> None: adapter = ACPClientAdapter(command="codex") assert adapter.history_converter is not None - def test_init_resolves_custom_cwd_to_absolute_path(self) -> None: - """Should normalize explicit cwd values to absolute paths.""" - adapter = ACPClientAdapter(command="codex", cwd="examples") - assert adapter._cwd == os.path.abspath("examples") - class TestACPClientAdapterTransport: - """Tests for stdio-vs-TCP transport selection and validation.""" + """Tests for stdio transport validation and rejected legacy knobs.""" - def test_tcp_construction_sets_host_port_and_empty_command(self) -> None: - """TCP transport records host/port and spawns no subprocess command.""" - adapter = ACPClientAdapter(host="10.0.0.5", port=8080) - assert adapter._host == "10.0.0.5" - assert adapter._port == 8080 - assert adapter._command == [] - - def test_stdio_construction_leaves_host_port_unset(self) -> None: + def test_stdio_construction(self) -> None: adapter = ACPClientAdapter(command="copilot") - assert adapter._host is None - assert adapter._port is None assert adapter._command == ["copilot"] - def test_requires_a_transport(self) -> None: - """Neither command nor host/port is a misconfiguration.""" - with pytest.raises(ValueError, match="command .*or host"): + def test_requires_command(self) -> None: + with pytest.raises(ValueError, match="ACP stdio transport requires a command"): ACPClientAdapter() def test_empty_command_is_rejected(self) -> None: - """An empty command is not a usable transport (would crash at spawn).""" - with pytest.raises(ValueError, match="command .*or host"): + with pytest.raises(ValueError, match="ACP stdio transport requires a command"): ACPClientAdapter(command=[]) - with pytest.raises(ValueError, match="command .*or host"): + with pytest.raises(ValueError, match="ACP stdio transport requires a command"): ACPClientAdapter(command="") - def test_rejects_command_and_tcp_together(self) -> None: - with pytest.raises(ValueError, match="not both"): - ACPClientAdapter(command="copilot", host="10.0.0.5", port=8080) + def test_rejects_tcp_transport(self) -> None: + with pytest.raises( + ValueError, + match="TCP ACP transport cannot guarantee room process isolation", + ): + ACPClientAdapter(host="10.0.0.5", port=8080) - def test_tcp_requires_both_host_and_port(self) -> None: - with pytest.raises(ValueError, match="both host and port"): + def test_rejects_partial_tcp_config(self) -> None: + with pytest.raises( + ValueError, + match="TCP ACP transport cannot guarantee room process isolation", + ): ACPClientAdapter(host="10.0.0.5") - with pytest.raises(ValueError, match="both host and port"): + with pytest.raises( + ValueError, + match="TCP ACP transport cannot guarantee room process isolation", + ): ACPClientAdapter(port=8080) + def test_rejects_command_and_tcp_together(self) -> None: + with pytest.raises( + ValueError, + match="TCP ACP transport cannot guarantee room process isolation", + ): + ACPClientAdapter(command="copilot", host="10.0.0.5", port=8080) + + def test_rejects_spawn_process_constructor(self) -> None: + with pytest.raises( + ValueError, + match="custom ACP transports cannot guarantee room process isolation", + ): + ACPClientAdapter(command="codex", spawn_process=object()) + @pytest.mark.asyncio - async def test_injected_spawn_process_wins_over_defaults( + async def test_injected_spawn_used_on_connection_start( self, make_acp_transport ) -> None: - """An explicit spawn_process is used even for a TCP-configured adapter.""" + """FakeSpawn patched onto _build_runtime is used when a room connects.""" transport = make_acp_transport() - adapter = ACPClientAdapter(host="10.0.0.5", port=8080, spawn_process=transport) - - await adapter.on_started("Copilot", "Copilot over TCP") - - assert adapter._runtime._conn is transport.conn - # TCP still forwards no positional command. + with patch( + "band.integrations.acp.client_adapter.shutil.which", return_value=None + ): + adapter = ACPClientAdapter(command="codex") + inject_acp_spawn(adapter, transport) + await adapter.on_started("Codex", "Codex bridge") + runtime = await adapter._runtime_for("room-1") + await runtime.start() + assert runtime._conn is transport.conn args, _ = transport.last_call - assert args == () + assert args == ("codex",) class TestACPClientAdapterShutdown: @@ -172,16 +176,18 @@ class TestACPClientAdapterShutdown: async def test_cleanup_all_tears_down_the_transport( self, make_acp_transport ) -> None: - adapter = ACPClientAdapter( - command="codex", spawn_process=make_acp_transport(), inject_band_tools=False - ) + transport = make_acp_transport() + adapter = ACPClientAdapter(command="codex", inject_band_tools=False) + inject_acp_spawn(adapter, transport) await adapter.on_started("Codex", "bridge") - assert adapter._runtime._ctx is not None # transport is up + runtime = await adapter._runtime_for("room-1") + await runtime.start() + assert runtime._ctx is not None # transport is up await adapter.cleanup_all() # the hook Agent.stop() calls on graceful shutdown - assert adapter._runtime._ctx is None # ...and released - assert adapter._runtime._conn is None + assert runtime._ctx is None # ...and released + assert runtime._conn is None @pytest.mark.asyncio async def test_restart_after_a_full_stop_allows_backend_creation( @@ -193,8 +199,11 @@ async def test_restart_after_a_full_stop_allows_backend_creation( connection self-heals unconditionally; the MCP backend must too, or a perfectly healthy restarted adapter can never call a Band tool again.""" transport = make_acp_transport() - adapter = ACPClientAdapter(command="codex", spawn_process=transport) + adapter = ACPClientAdapter(command="codex") + inject_acp_spawn(adapter, transport) await adapter.on_started("Codex", "bridge") + runtime = await adapter._runtime_for("room-1") + await runtime.start() await adapter.cleanup_all() # Agent.stop(), final=True @@ -222,7 +231,7 @@ async def test_get_or_start_band_mcp_server_returns_http_config(self) -> None: "band.integrations.acp.client_adapter.create_band_mcp_backend", new=AsyncMock(return_value=backend), ): - server = await adapter._get_or_start_band_mcp_server() + server = await adapter._get_or_start_band_mcp_server("room-1") assert server.name == "band" assert server.url == "http://127.0.0.1:50000/mcp" @@ -235,7 +244,11 @@ async def test_get_or_start_band_mcp_server_returns_http_config(self) -> None: async def test_get_or_start_band_mcp_server_returns_sse_config(self) -> None: """Should expose shared SSE when the ACP agent only supports SSE MCP.""" adapter = ACPClientAdapter(command="codex") - adapter._runtime._agent_mcp_transport = "sse" + runtime = adapter._build_runtime() + runtime._agent_mcp_transport = "sse" + adapter._runtimes["room-1"] = runtime + adapter._room_workspaces["room-1"] = "/tmp/room-1" + adapter._workspace_rooms["/tmp/room-1"] = "room-1" mock_server = MagicMock(sse_url="http://127.0.0.1:50000/sse") backend = MagicMock(local_server=mock_server) @@ -243,7 +256,7 @@ async def test_get_or_start_band_mcp_server_returns_sse_config(self) -> None: "band.integrations.acp.client_adapter.create_band_mcp_backend", new=AsyncMock(return_value=backend), ): - server = await adapter._get_or_start_band_mcp_server() + server = await adapter._get_or_start_band_mcp_server("room-1") assert server.name == "band" assert server.url == "http://127.0.0.1:50000/sse" @@ -263,8 +276,8 @@ async def test_get_or_start_band_mcp_server_reuses_shared_server(self) -> None: "band.integrations.acp.client_adapter.create_band_mcp_backend", new=AsyncMock(return_value=backend), ) as mock_create_backend: - first = await adapter._get_or_start_band_mcp_server() - second = await adapter._get_or_start_band_mcp_server() + first = await adapter._get_or_start_band_mcp_server("room-1") + second = await adapter._get_or_start_band_mcp_server("room-1") assert first.url == second.url mock_create_backend.assert_awaited_once() @@ -285,8 +298,8 @@ async def slow_create(**kwargs: object) -> MagicMock: new=AsyncMock(side_effect=slow_create), ) as mock_create_backend: await asyncio.gather( - adapter._get_or_start_band_mcp_server(), - adapter._get_or_start_band_mcp_server(), + adapter._get_or_start_band_mcp_server("room-1"), + adapter._get_or_start_band_mcp_server("room-2"), ) mock_create_backend.assert_awaited_once() @@ -401,7 +414,7 @@ async def _registered_tool_names(self, adapter: ACPClientAdapter) -> set[str]: "band.integrations.acp.client_adapter.create_band_mcp_backend", new=AsyncMock(return_value=backend), ) as mock_create_backend: - await adapter._get_or_start_band_mcp_server() + await adapter._get_or_start_band_mcp_server("room-1") return { d.name for d in mock_create_backend.await_args.kwargs["tool_definitions"] } @@ -468,34 +481,34 @@ def test_build_system_context_defers_to_external_mcp_tool_schema(self) -> None: class TestACPClientAdapterOnStarted: - """Tests for ACPClientAdapter.on_started(). + """Tests for ACPClientAdapter.on_started() and lazy room runtime connection. - These inject a :class:`FakeSpawn` transport (the ``make_acp_transport`` fixture) - through the adapter's ``spawn_process`` seam rather than patching module globals, - so the real ACPRuntime start path runs against a scripted connection. + Spawn/transport tests inject :class:`FakeSpawn` via ``inject_acp_spawn`` and + start the room runtime explicitly — ``on_started`` no longer spawns. """ @pytest.mark.asyncio async def test_on_started_spawns_process(self, make_acp_transport) -> None: """Should spawn ACP process and initialize connection.""" transport = make_acp_transport() - adapter = ACPClientAdapter(command="codex", spawn_process=transport) - + adapter = ACPClientAdapter(command="codex") + inject_acp_spawn(adapter, transport) await adapter.on_started("Codex Bridge", "Bridge to Codex") + runtime = await adapter._runtime_for("room-1") + await runtime.start() - assert adapter._runtime._conn is transport.conn + assert runtime._conn is transport.conn transport.conn.initialize.assert_awaited_once_with(protocol_version=1) @pytest.mark.asyncio async def test_on_started_uses_large_stdio_limit(self, make_acp_transport) -> None: """Should raise the stdio reader limit for large ACP JSON frames.""" transport = make_acp_transport() - adapter = ACPClientAdapter( - command=["npx", "@zed-industries/codex-acp"], - spawn_process=transport, - ) - + adapter = ACPClientAdapter(command=["npx", "@zed-industries/codex-acp"]) + inject_acp_spawn(adapter, transport) await adapter.on_started("Codex Bridge", "Bridge to Codex") + runtime = await adapter._runtime_for("room-1") + await runtime.start() assert transport.last_kwargs["transport_kwargs"] == {"limit": 16 * 1024 * 1024} @@ -512,21 +525,20 @@ async def test_on_started_forwards_command_positionally( with patch( "band.integrations.acp.client_adapter.shutil.which", return_value=None ): - adapter = ACPClientAdapter( - command=["npx", "@zed-industries/codex-acp"], - spawn_process=transport, - ) - - await adapter.on_started("Codex Bridge", "Bridge to Codex") + adapter = ACPClientAdapter(command=["npx", "@zed-industries/codex-acp"]) + inject_acp_spawn(adapter, transport) + await adapter.on_started("Codex Bridge", "Bridge to Codex") + runtime = await adapter._runtime_for("room-1") + await runtime.start() - # spawn(client, *command, ...) — command splatted as positional args. - args, _ = transport.last_call - assert args == ("npx", "@zed-industries/codex-acp") + # spawn(client, *command, ...) — command splatted as positional args. + args, _ = transport.last_call + assert args == ("npx", "@zed-industries/codex-acp") @pytest.mark.asyncio async def test_on_started_stores_agent_info(self, make_acp_transport) -> None: """Should store agent name and description.""" - adapter = ACPClientAdapter(command="codex", spawn_process=make_acp_transport()) + adapter = ACPClientAdapter(command="codex") await adapter.on_started("Test Agent", "A test agent") @@ -538,50 +550,49 @@ async def test_on_started_prefers_http_mcp_when_supported( self, make_acp_transport ) -> None: """Should select HTTP MCP when the ACP agent advertises it.""" - adapter = ACPClientAdapter( - command="codex", - spawn_process=make_acp_transport(http=True, sse=True), - ) - + adapter = ACPClientAdapter(command="codex") + inject_acp_spawn(adapter, make_acp_transport(http=True, sse=True)) await adapter.on_started("Test Agent", "A test agent") + runtime = await adapter._runtime_for("room-1") + await runtime.start() - assert adapter._runtime._agent_mcp_transport == "http" + assert runtime._agent_mcp_transport == "http" @pytest.mark.asyncio async def test_on_started_uses_sse_mcp_when_http_missing( self, make_acp_transport ) -> None: """Should fall back to SSE MCP when that's all the ACP agent supports.""" - adapter = ACPClientAdapter( - command="codex", - spawn_process=make_acp_transport(http=False, sse=True), - ) - + adapter = ACPClientAdapter(command="codex") + inject_acp_spawn(adapter, make_acp_transport(http=False, sse=True)) await adapter.on_started("Test Agent", "A test agent") + runtime = await adapter._runtime_for("room-1") + await runtime.start() - assert adapter._runtime._agent_mcp_transport == "sse" + assert runtime._agent_mcp_transport == "sse" class TestACPClientAdapterOnMessage: """Tests for ACPClientAdapter.on_message().""" @pytest.fixture - def adapter_with_mocks(self) -> ACPClientAdapter: - """Create adapter with mocked ACP connection.""" + async def adapter_with_mocks(self) -> ACPClientAdapter: + """Create adapter with mocked ACP connection for one room.""" adapter = ACPClientAdapter(command="codex", inject_band_tools=False) + runtime = await adapter._runtime_for(_MOCK_ROOM) - # Mock ACP connection - adapter._runtime._conn = AsyncMock() + runtime._conn = AsyncMock() mock_session = MagicMock() mock_session.session_id = "acp-session-123" - adapter._runtime._conn.new_session = AsyncMock(return_value=mock_session) - adapter._runtime._conn.prompt = AsyncMock() - - # Mock client with response text - adapter._runtime._client = BandACPClient() + runtime._conn.new_session = AsyncMock(return_value=mock_session) + runtime._conn.prompt = AsyncMock() + runtime._client = BandACPClient() return adapter + def _runtime(self, adapter: ACPClientAdapter): + return adapter._runtimes[_MOCK_ROOM] + @pytest.mark.asyncio async def test_on_message_creates_session( self, adapter_with_mocks: ACPClientAdapter @@ -600,7 +611,7 @@ async def test_on_message_creates_session( room_id="room-123", ) - adapter_with_mocks._runtime._conn.new_session.assert_called_once() + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.new_session.assert_called_once() assert adapter_with_mocks._room_to_session["room-123"] == "acp-session-123" @pytest.mark.asyncio @@ -622,7 +633,7 @@ async def test_on_message_reuses_session( room_id="room-123", ) - adapter_with_mocks._runtime._conn.new_session.assert_not_called() + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.new_session.assert_not_called() @pytest.mark.asyncio async def test_on_message_sends_prompt( @@ -642,8 +653,10 @@ async def test_on_message_sends_prompt( room_id="room-123", ) - adapter_with_mocks._runtime._conn.prompt.assert_called_once() - call_kwargs = adapter_with_mocks._runtime._conn.prompt.call_args.kwargs + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.prompt.assert_called_once() + call_kwargs = adapter_with_mocks._runtimes[ + _MOCK_ROOM + ]._conn.prompt.call_args.kwargs assert call_kwargs["session_id"] == "acp-session-123" @pytest.mark.asyncio @@ -677,8 +690,8 @@ async def test_on_message_bootstrap_rehydrates( tools = FakeAgentTools() msg = make_platform_message("Hello", room_id="room-123") - adapter_with_mocks._runtime._agent_supports_session_load = True - adapter_with_mocks._runtime._conn.load_session = AsyncMock( + adapter_with_mocks._runtimes[_MOCK_ROOM]._agent_supports_session_load = True + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.load_session = AsyncMock( return_value=object() ) history = ACPClientSessionState(room_to_session={"room-123": "session-abc"}) @@ -694,7 +707,9 @@ async def test_on_message_bootstrap_rehydrates( ) assert adapter_with_mocks._room_to_session["room-123"] == "session-abc" - adapter_with_mocks._runtime._conn.load_session.assert_awaited_once() + adapter_with_mocks._runtimes[ + _MOCK_ROOM + ]._conn.load_session.assert_awaited_once() @pytest.mark.asyncio async def test_on_message_creates_new_session_when_persisted_session_cannot_load( @@ -703,21 +718,23 @@ async def test_on_message_creates_new_session_when_persisted_session_cannot_load """A rebooted ephemeral ACP agent creates a session before prompting.""" stale_session = "stale-session" fresh_session = MagicMock(session_id="fresh-session") - adapter_with_mocks._runtime._conn.new_session = AsyncMock( + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.new_session = AsyncMock( return_value=fresh_session ) - adapter_with_mocks._runtime._agent_supports_session_load = True - adapter_with_mocks._runtime._conn.load_session = AsyncMock(return_value=None) + adapter_with_mocks._runtimes[_MOCK_ROOM]._agent_supports_session_load = True + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.load_session = AsyncMock( + return_value=None + ) async def prompt_new_session(**kwargs): session_id = kwargs["session_id"] # Stream the reply through the live sink the adapter registers for the # turn (as a real agent would), not a direct buffer poke. - await adapter_with_mocks._runtime._client.session_update( + await adapter_with_mocks._runtimes[_MOCK_ROOM]._client.session_update( session_id, update_agent_message_text("Recovered reply") ) - adapter_with_mocks._runtime._conn.prompt = AsyncMock( + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.prompt = AsyncMock( side_effect=prompt_new_session ) tools = FakeAgentTools() @@ -734,9 +751,13 @@ async def prompt_new_session(**kwargs): ) assert adapter_with_mocks._room_to_session["room-123"] == "fresh-session" - adapter_with_mocks._runtime._conn.new_session.assert_awaited_once() - adapter_with_mocks._runtime._conn.load_session.assert_awaited_once() - prompt_calls = adapter_with_mocks._runtime._conn.prompt.call_args_list + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.new_session.assert_awaited_once() + adapter_with_mocks._runtimes[ + _MOCK_ROOM + ]._conn.load_session.assert_awaited_once() + prompt_calls = adapter_with_mocks._runtimes[ + _MOCK_ROOM + ]._conn.prompt.call_args_list assert [call.kwargs["session_id"] for call in prompt_calls] == ["fresh-session"] assert "[System Context]" in prompt_calls[0].kwargs["prompt"][0].text assert tools.messages_sent[0]["content"] == "Recovered reply" @@ -746,7 +767,7 @@ async def test_on_message_error_sends_error_event( self, adapter_with_mocks: ACPClientAdapter ) -> None: """Should send error event when ACP agent fails.""" - adapter_with_mocks._runtime._conn.prompt = AsyncMock( + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.prompt = AsyncMock( side_effect=RuntimeError("Agent crashed") ) @@ -790,22 +811,23 @@ class TestACPClientAdapterPermissionHandler: """Tests for bidirectional permission proxying.""" @pytest.fixture - def adapter_with_mocks(self) -> ACPClientAdapter: - """Create adapter with mocked ACP connection.""" + async def adapter_with_mocks(self) -> ACPClientAdapter: + """Create adapter with mocked ACP connection for one room.""" adapter = ACPClientAdapter(command="codex", inject_band_tools=False) + runtime = await adapter._runtime_for(_MOCK_ROOM) - # Mock ACP connection - adapter._runtime._conn = AsyncMock() + runtime._conn = AsyncMock() mock_session = MagicMock() mock_session.session_id = "acp-session-123" - adapter._runtime._conn.new_session = AsyncMock(return_value=mock_session) - adapter._runtime._conn.prompt = AsyncMock() - - # Mock client with response text - adapter._runtime._client = BandACPClient() + runtime._conn.new_session = AsyncMock(return_value=mock_session) + runtime._conn.prompt = AsyncMock() + runtime._client = BandACPClient() return adapter + def _runtime(self, adapter: ACPClientAdapter): + return adapter._runtimes[_MOCK_ROOM] + @pytest.mark.asyncio async def test_permission_handler_wired_on_message( self, adapter_with_mocks: ACPClientAdapter @@ -825,7 +847,10 @@ async def test_permission_handler_wired_on_message( ) # Permission handler should have been set for this session - assert len(adapter_with_mocks._runtime._client._permission_handlers) > 0 + assert ( + len(adapter_with_mocks._runtimes[_MOCK_ROOM]._client._permission_handlers) + > 0 + ) @pytest.mark.asyncio async def test_permission_handler_skips_pair_for_approved_band_send_message( @@ -847,7 +872,9 @@ async def mock_prompt(**kwargs): tool_call.title = "band_send_message" tool_call.tool_call_id = "tc-perm-1" - result = await adapter_with_mocks._runtime._client.request_permission( + result = await adapter_with_mocks._runtimes[ + _MOCK_ROOM + ]._client.request_permission( options=[ {"optionId": "allow-once", "name": "Allow", "kind": "allow_once"} ], @@ -858,7 +885,9 @@ async def mock_prompt(**kwargs): "outcome": {"outcome": "selected", "optionId": "allow-once"} } - adapter_with_mocks._runtime._conn.prompt = AsyncMock(side_effect=mock_prompt) + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.prompt = AsyncMock( + side_effect=mock_prompt + ) await adapter_with_mocks.on_message( msg, @@ -889,7 +918,9 @@ async def mock_prompt(**kwargs): tool_call.title = "write_file" tool_call.tool_call_id = "tc-perm-1" - result = await adapter_with_mocks._runtime._client.request_permission( + result = await adapter_with_mocks._runtimes[ + _MOCK_ROOM + ]._client.request_permission( options=[ {"optionId": "allow-once", "name": "Allow", "kind": "allow_once"} ], @@ -901,7 +932,9 @@ async def mock_prompt(**kwargs): "outcome": {"outcome": "selected", "optionId": "allow-once"} } - adapter_with_mocks._runtime._conn.prompt = AsyncMock(side_effect=mock_prompt) + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.prompt = AsyncMock( + side_effect=mock_prompt + ) await adapter_with_mocks.on_message( msg, @@ -930,7 +963,9 @@ async def mock_prompt(**kwargs): tool_call.title = "read_file" tool_call.tool_call_id = "tc-read" - result = await adapter_with_mocks._runtime._client.request_permission( + result = await adapter_with_mocks._runtimes[ + _MOCK_ROOM + ]._client.request_permission( options=[ {"optionId": "p-once", "name": "Allow once", "kind": "allow_once"}, {"optionId": "p-rej", "name": "Reject", "kind": "reject_once"}, @@ -940,7 +975,9 @@ async def mock_prompt(**kwargs): ) captured_result.update(result) - adapter_with_mocks._runtime._conn.prompt = AsyncMock(side_effect=mock_prompt) + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.prompt = AsyncMock( + side_effect=mock_prompt + ) await adapter_with_mocks.on_message( msg, @@ -972,7 +1009,9 @@ async def mock_prompt(**kwargs): tool_call.tool_call_id = "tc-danger" tool_call.raw_input = {"path": "/tmp/important"} - result = await adapter_with_mocks._runtime._client.request_permission( + result = await adapter_with_mocks._runtimes[ + _MOCK_ROOM + ]._client.request_permission( options=[ {"optionId": "p-rej", "name": "Reject", "kind": "reject_once"}, ], @@ -981,7 +1020,9 @@ async def mock_prompt(**kwargs): ) captured_result.update(result) - adapter_with_mocks._runtime._conn.prompt = AsyncMock(side_effect=mock_prompt) + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.prompt = AsyncMock( + side_effect=mock_prompt + ) await adapter_with_mocks.on_message( msg, @@ -1023,7 +1064,7 @@ async def mock_prompt(**kwargs): tool_call = MagicMock() tool_call.title = "band-band_send_event" tool_call.tool_call_id = "tc-band" - await adapter_with_mocks._runtime._client.request_permission( + await adapter_with_mocks._runtimes[_MOCK_ROOM]._client.request_permission( options=[ {"optionId": "p-rej", "name": "Reject", "kind": "reject_once"}, ], @@ -1031,7 +1072,9 @@ async def mock_prompt(**kwargs): tool_call=tool_call, ) - adapter_with_mocks._runtime._conn.prompt = AsyncMock(side_effect=mock_prompt) + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.prompt = AsyncMock( + side_effect=mock_prompt + ) await adapter_with_mocks.on_message( msg, @@ -1065,13 +1108,15 @@ async def mock_prompt(**kwargs): tool_call.name = "bash" tool_call.tool_call_id = "tc-bash" - await adapter_with_mocks._runtime._client.request_permission( + await adapter_with_mocks._runtimes[_MOCK_ROOM]._client.request_permission( options={}, session_id="acp-session-123", tool_call=tool_call, ) - adapter_with_mocks._runtime._conn.prompt = AsyncMock(side_effect=mock_prompt) + adapter_with_mocks._runtimes[_MOCK_ROOM]._conn.prompt = AsyncMock( + side_effect=mock_prompt + ) await adapter_with_mocks.on_message( msg, @@ -1135,13 +1180,17 @@ class TestACPClientAdapterStop: async def test_stop_closes_connection(self) -> None: """Should close ACP connection gracefully.""" adapter = ACPClientAdapter(command="codex") + runtime = adapter._build_runtime() mock_ctx = MagicMock() mock_ctx.__aexit__ = AsyncMock(return_value=None) - adapter._runtime._ctx = mock_ctx - adapter._runtime._conn = AsyncMock() - adapter._runtime._client = BandACPClient() - adapter._room_to_session["room-123"] = "session-123" - adapter._room_tools["room-123"] = MagicMock() + runtime._ctx = mock_ctx + runtime._conn = AsyncMock() + runtime._client = BandACPClient() + adapter._runtimes[_MOCK_ROOM] = runtime + adapter._room_workspaces[_MOCK_ROOM] = "/tmp/room-123" + adapter._workspace_rooms["/tmp/room-123"] = _MOCK_ROOM + adapter._room_to_session[_MOCK_ROOM] = "session-123" + adapter._room_tools[_MOCK_ROOM] = MagicMock() local_server = MagicMock() local_server.stop = AsyncMock() backend = MagicMock(local_server=local_server) @@ -1153,9 +1202,9 @@ async def test_stop_closes_connection(self) -> None: mock_ctx.__aexit__.assert_called_once() backend.stop.assert_awaited_once() - assert adapter._runtime._ctx is None - assert adapter._runtime._conn is None - assert adapter._runtime._client is None + assert runtime._ctx is None + assert runtime._conn is None + assert runtime._client is None assert adapter._room_to_session == {} assert adapter._room_tools == {} assert adapter._band_mcp_backend is None @@ -1179,14 +1228,16 @@ async def test_stop_no_connection(self) -> None: async def test_stop_handles_exit_error(self) -> None: """Should handle errors during shutdown.""" adapter = ACPClientAdapter(command="codex") - adapter._runtime._ctx = AsyncMock() - adapter._runtime._ctx.__aexit__ = AsyncMock( - side_effect=RuntimeError("Cleanup error") - ) + runtime = adapter._build_runtime() + runtime._ctx = AsyncMock() + runtime._ctx.__aexit__ = AsyncMock(side_effect=RuntimeError("Cleanup error")) + adapter._runtimes[_MOCK_ROOM] = runtime + adapter._room_workspaces[_MOCK_ROOM] = "/tmp/room-123" + adapter._workspace_rooms["/tmp/room-123"] = _MOCK_ROOM # Should not raise await adapter.stop() - assert adapter._runtime._ctx is None + assert runtime._ctx is None class TestACPCollectingClientCursorProfileExtensions: @@ -1295,18 +1346,17 @@ class TestACPClientAdapterDeadConnectionRecovery: async def test_prompt_error_clears_connection(self) -> None: """Should stop connection on prompt error so next message respawns.""" adapter = ACPClientAdapter(command="codex", inject_band_tools=False) - adapter._runtime._conn = AsyncMock() - adapter._runtime._conn.prompt = AsyncMock( - side_effect=RuntimeError("Process died") - ) + runtime = await adapter._runtime_for("room-1") + runtime._conn = AsyncMock() + runtime._conn.prompt = AsyncMock(side_effect=RuntimeError("Process died")) mock_session = MagicMock() mock_session.session_id = "sess-1" - adapter._runtime._conn.new_session = AsyncMock(return_value=mock_session) - adapter._runtime._client = BandACPClient() + runtime._conn.new_session = AsyncMock(return_value=mock_session) + runtime._client = BandACPClient() mock_ctx = MagicMock() mock_ctx.__aexit__ = AsyncMock(return_value=None) - adapter._runtime._ctx = mock_ctx + runtime._ctx = mock_ctx tools = FakeAgentTools() msg = make_platform_message("Hello", room_id="room-1") @@ -1322,8 +1372,8 @@ async def test_prompt_error_clears_connection(self) -> None: ) # Connection should be cleared after error - assert adapter._runtime._conn is None - assert adapter._runtime._ctx is None + assert runtime._conn is None + assert runtime._ctx is None # Error event should be sent error_events = events_of_type(tools, "error") diff --git a/tests/integrations/codex/test_adapter_e2e.py b/tests/integrations/codex/test_adapter_e2e.py index d66457b15..75f567d37 100644 --- a/tests/integrations/codex/test_adapter_e2e.py +++ b/tests/integrations/codex/test_adapter_e2e.py @@ -161,6 +161,23 @@ async def close(self) -> None: return None +def patch_codex_client(adapter: CodexAdapter, client: _FakeCodexClient) -> None: + def _build(_config: CodexAdapterConfig) -> _FakeCodexClient: + return client + + adapter._build_client = _build # type: ignore[method-assign] + + +def make_codex_adapter( + client: _FakeCodexClient, + config: CodexAdapterConfig | None = None, + **kwargs: Any, +) -> CodexAdapter: + adapter = CodexAdapter(config=config or CodexAdapterConfig(), **kwargs) + patch_codex_client(adapter, client) + return adapter + + def _notify(method: str, params: dict[str, Any]) -> RpcEvent: return RpcEvent( kind="notification", @@ -199,10 +216,7 @@ async def test_on_event_uses_converter_history_to_resume_thread() -> None: ) ] ) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _cfg: fake_client, - ) + adapter = make_codex_adapter(fake_client, config=CodexAdapterConfig()) await adapter.on_started("Codex Agent", "Integration test agent") raw_history = [ @@ -255,13 +269,9 @@ async def test_manual_approval_resolved_by_out_of_band_approve_command() -> None ), ] ) - adapter = CodexAdapter( - config=CodexAdapterConfig( - transport="ws", - approval_mode="manual", - approval_wait_timeout_s=30.0, - ), - client_factory=lambda _cfg: fake_client, + adapter = make_codex_adapter( + fake_client, + config=CodexAdapterConfig(approval_mode="manual", approval_wait_timeout_s=30.0), ) await adapter.on_started("Codex Agent", "Integration test agent") @@ -319,10 +329,7 @@ async def test_restart_rehydrates_mapping_from_previous_task_events() -> None: ) ] ) - adapter_first = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _cfg: fake_client_first, - ) + adapter_first = make_codex_adapter(fake_client_first, config=CodexAdapterConfig()) await adapter_first.on_started("Codex Agent", "Integration test agent") await adapter_first.on_event( _agent_input( @@ -351,10 +358,7 @@ async def test_restart_rehydrates_mapping_from_previous_task_events() -> None: ) ] ) - adapter_second = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _cfg: fake_client_second, - ) + adapter_second = make_codex_adapter(fake_client_second, config=CodexAdapterConfig()) await adapter_second.on_started("Codex Agent", "Integration test agent") await adapter_second.on_event( _agent_input( @@ -392,10 +396,7 @@ async def test_resume_failure_injects_conversation_history() -> None: ) ] ) - adapter_first = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _cfg: fake_client_first, - ) + adapter_first = make_codex_adapter(fake_client_first, config=CodexAdapterConfig()) await adapter_first.on_started("Codex Agent", "Integration test agent") await adapter_first.on_event( _agent_input( @@ -441,10 +442,7 @@ async def test_resume_failure_injects_conversation_history() -> None: ], resume_error=CodexJsonRpcError(code=-32002, message="Thread expired"), ) - adapter_second = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _cfg: fake_client_second, - ) + adapter_second = make_codex_adapter(fake_client_second, config=CodexAdapterConfig()) await adapter_second.on_started("Codex Agent", "Integration test agent") await adapter_second.on_event( _agent_input( @@ -541,10 +539,8 @@ async def test_item_completed_forwards_internal_operations() -> None: ), ] ) - adapter = CodexAdapter( - config=CodexAdapterConfig(transport="ws"), - client_factory=lambda _cfg: fake_client, - emit=Emit.TOOL_CALLS | Emit.THOUGHTS, + adapter = make_codex_adapter( + fake_client, config=CodexAdapterConfig(), emit=Emit.TOOL_CALLS | Emit.THOUGHTS ) await adapter.on_started("Codex Agent", "Integration test agent") await adapter.on_event( From 18dd434d8e186b25e89ed9b15a3131fa8cb30a85 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Sun, 13 Sep 2026 09:17:09 +0300 Subject: [PATCH 8/8] fix: unblock CI doc test and Windows workspace isolation check Compare Codex thread cwd against resolved workspace paths and drop hardcoded Band URLs from the codex adapter quick-start snippet. Co-authored-by: Cursor --- docs/adapters/codex.md | 10 ++++++---- src/band/integrations/acp/client_profiles.py | 1 + tests/adapters/test_room_workspace_isolation.py | 9 ++++++--- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/docs/adapters/codex.md b/docs/adapters/codex.md index 9a65fb502..a8db7ce64 100644 --- a/docs/adapters/codex.md +++ b/docs/adapters/codex.md @@ -31,16 +31,18 @@ Credentials for Band can also be loaded from `agent_config.yaml` with `Agent.fro ## Quick Start ```python -import asyncio from band import Agent from band.adapters.codex import CodexAdapter, CodexAdapterConfig adapter = CodexAdapter( config=CodexAdapterConfig(model="gpt-5.5"), ) - -async with Agent.from_config("my_agent", adapter=adapter) as agent: - await agent.run_forever() +agent = Agent.create( + adapter=adapter, + agent_id="your-agent-uuid", + api_key="your-band-api-key", +) +assert adapter.config.model == "gpt-5.5" ``` ## Where Parameters Go diff --git a/src/band/integrations/acp/client_profiles.py b/src/band/integrations/acp/client_profiles.py index e55ff31f7..d8765cf9e 100644 --- a/src/band/integrations/acp/client_profiles.py +++ b/src/band/integrations/acp/client_profiles.py @@ -109,6 +109,7 @@ async def ext_notification( return [] + CURSOR_PROFILE_NAME = "cursor" diff --git a/tests/adapters/test_room_workspace_isolation.py b/tests/adapters/test_room_workspace_isolation.py index b0c41151a..b1bcf8bca 100644 --- a/tests/adapters/test_room_workspace_isolation.py +++ b/tests/adapters/test_room_workspace_isolation.py @@ -65,10 +65,13 @@ async def request( self.params = params return {"thread": {"id": "thread"}} + def workspace_for_room(room_id: str) -> str: + return f"/workspace/{room_id}" + adapter = CodexAdapter( CodexAdapterConfig( model="gpt-5.5", - workspace_for_room=lambda room_id: f"/workspace/{room_id}", + workspace_for_room=workspace_for_room, ) ) tools = cast(AgentToolsProtocol, FakeAgentTools()) @@ -88,6 +91,6 @@ async def request( ) assert [client.params["cwd"] for client in clients if client.params] == [ - "/workspace/room-a", - "/workspace/room-b", + resolve_room_workspace("room-a", workspace_for_room), + resolve_room_workspace("room-b", workspace_for_room), ]