From 933de6fb652f5e8ae485c3bf3a05a0c537028c72 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Mon, 14 Sep 2026 18:09:30 +0300 Subject: [PATCH 1/2] feat: Add E2E tests for the coding-agent adapters (claude_sdk, codex) Restores test_codex.py's regression-guard smoke (deleted in fc62742c) and adds a new claude_sdk manual-approval smoke, both proving the coding-agent adapters' live-only behavior end-to-end. Fixes two real bugs the new claude_sdk smoke uncovered: - _pre_tool_use_continue_hook returned {"continue_": True}, which carries no permission decision, so native tool calls (Bash/Write/Edit) never actually reached can_use_tool under the adapter's default permission_mode -- manual approval silently never gated them. Fixed to return hookSpecificOutput.permissionDecision="ask", the SDK's real delegation mechanism. - Even with that fixed, approval_mode="manual" deadlocked: Band's runtime processes one room-message cycle at a time, but on_message blocked synchronously inside that cycle waiting on a chat reply only a later cycle could deliver. on_message now runs a turn as a detached task and only awaits its early release (mirroring OpencodeAdapter's release_turn_wait/turn_future split), so a manual approval can free the room to dispatch the reply that resolves it. Also fixes _build_claude_sdk's cwd hermeticity (no explicit cwd meant every matrix cell ran Claude Code against this repo's own checkout) and extracts the adapter's approval prompt/resolution strings into named constants so the new smoke anchors on them instead of duplicating the wording. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01PycgygL3XZiSheL4CpquPM --- src/band/adapters/claude_sdk.py | 169 +++++++++++++++--- tests/adapters/test_claude_sdk_adapter.py | 9 +- .../smoke/adapters/test_claude_sdk.py | 134 ++++++++++++++ .../e2e/baseline/smoke/adapters/test_codex.py | 95 ++++++++++ .../baseline/smoke/samples/sample_agents.py | 26 +++ tests/e2e/baseline/toolkit/builders.py | 6 + 6 files changed, 408 insertions(+), 31 deletions(-) create mode 100644 tests/e2e/baseline/smoke/adapters/test_claude_sdk.py create mode 100644 tests/e2e/baseline/smoke/adapters/test_codex.py diff --git a/src/band/adapters/claude_sdk.py b/src/band/adapters/claude_sdk.py index 14a1af986..3310cc4f0 100644 --- a/src/band/adapters/claude_sdk.py +++ b/src/band/adapters/claude_sdk.py @@ -132,6 +132,16 @@ ApprovalMode = Literal["auto_accept", "auto_decline", "manual"] ApprovalDecision = Literal["accept", "decline"] +# Chat-facing approval prompt/resolution text (mirrors +# band.adapters.opencode.approvals's constant style) -- named so callers +# (e.g. E2E smokes) can anchor on the exact wording instead of re-typing it. +APPROVAL_REQUESTED_TEMPLATE = ( + "Approval requested ({summary}). Token: `{token}`.\n" + "Reply `/approve {token}` or `/decline {token}`.\n" + "Use `/approvals` to list pending approvals." +) +APPROVAL_RESOLVED_TEMPLATE = "Approval `{token}` resolved as **{decision}**." + # Commands recognised as local (not forwarded to Claude) _APPROVAL_CMDS = frozenset({"approve", "decline", "approvals"}) _LOCAL_CMDS = _APPROVAL_CMDS | frozenset({"status"}) @@ -180,12 +190,23 @@ async def _pre_tool_use_continue_hook( _tool_name: str | None, _context: HookContext, ) -> HookJSONOutput: - """PreToolUse hook that delegates every tool to ``can_use_tool``. - - Returning ``{"continue_": True}`` tells the SDK to skip its built-in - permission resolution and call the ``can_use_tool`` callback instead. + """PreToolUse hook that forces every native tool call to ``can_use_tool``. + + ``hookSpecificOutput.permissionDecision: "ask"`` is what actually routes a + tool call to the ``can_use_tool`` callback (verified against + ``claude_agent_sdk.types.ToolPermissionContext.decision_reason``'s own + docstring). A bare ``continue_: True`` carries no permission decision, so + the CLI falls back to its own ``permission_mode``-driven default instead + of ever consulting ``can_use_tool`` -- silently skipping chat-based manual + approval for native tools (Bash/Write/Edit) under the adapter's default + ``permission_mode="acceptEdits"``. """ - return {"continue_": True} + return { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "ask", + } + } @dataclass @@ -399,6 +420,16 @@ def __init__( # as results arrive and the room's map is dropped in on_cleanup. self._pending_tool_names: dict[str, dict[str, str]] = {} + # A turn runs as a detached task so a manual approval mid-turn can + # release on_message early (see _run_turn) -- Band's runtime processes + # one room-message cycle at a time, so on_message must return before + # the room can be dispatched the reply that resolves the approval. + # {room_id: the current turn's release future} + self._turn_release: dict[str, asyncio.Future[None]] = {} + # {room_id: the current turn's background task}, so on_message can + # refuse to start a second concurrent turn on the same client. + self._turn_tasks: dict[str, asyncio.Task[None]] = {} + # --- Adapted from BandClaudeSDKAgent._on_started --- async def on_started(self, agent_name: str, agent_description: str) -> None: """Create MCP server and session manager after agent metadata is fetched.""" @@ -591,6 +622,19 @@ async def on_message( ) return + # A prior turn's background task (see _run_turn) may still be running + # this room's ClaudeSDKClient -- most commonly while it's parked on a + # manual approval. A second concurrent client.query() on the same + # session is unsafe, so refuse rather than race it (mirrors + # OpencodeAdapter's own "still processing" guard). + running_turn = self._turn_tasks.get(room_id) + if running_turn is not None and not running_turn.done(): + await tools.send_message( + "Still processing the previous request in this room.", + mentions=[msg.sender_id] if msg.sender_id else None, + ) + return + # Determine session_id for resume: prefer history (persisted) then # in-memory cache. Only used on bootstrap/reconnect. stored_session_id: str | None = None @@ -678,32 +722,94 @@ async def on_message( len(messages_to_send), ) + # Run the turn as a detached task and only await its *release* -- + # not its completion. A manual approval mid-turn resolves the release + # early (see _resolve_manual_approval / _release_turn) so on_message + # can return and Band's strictly-sequential per-room message loop can + # dispatch the reply that will eventually resolve the approval. When + # nothing needs a human, the turn finishes before release fires and + # this is equivalent to the previous synchronous await. + release_future: asyncio.Future[None] = ( + asyncio.get_running_loop().create_future() + ) + self._turn_release[room_id] = release_future + turn_task = asyncio.create_task( + self._run_turn(client, room_id, tools, full_message, msg.id, release_future) + ) + self._turn_tasks[room_id] = turn_task + turn_task.add_done_callback(self._log_turn_task_exception) + try: + await release_future + finally: + if self._turn_release.get(room_id) is release_future: + del self._turn_release[room_id] + if turn_task.done(): + # The turn finished before release fired (the common, no-approval + # case) -- await it so a failure still propagates through + # on_message exactly as it did before this turn ran detached. + await turn_task + + async def _run_turn( + self, + client: ClaudeSDKClient, + room_id: str, + tools: AgentToolsProtocol, + full_message: str, + msg_id: str, + release_future: asyncio.Future[None], + ) -> None: + """Run one turn to completion; always releases ``release_future``.""" try: - # Send query to Claude - await client.query(full_message) + try: + # Send query to Claude + await client.query(full_message) - # Process streaming response (MCP tools handle execution) - await self._process_response(client, room_id, tools) + # Process streaming response (MCP tools handle execution) + await self._process_response(client, room_id, tools) - except CLIConnectionError as e: - # CLI process is dead — evict the cached session so the next - # message creates a fresh one instead of reusing the corpse. - logger.error( - "Room %s: CLI process terminated: %s — invalidating session", - room_id, - e, - ) - await self._invalidate_session(room_id) + except CLIConnectionError as e: + # CLI process is dead — evict the cached session so the next + # message creates a fresh one instead of reusing the corpse. + logger.error( + "Room %s: CLI process terminated: %s — invalidating session", + room_id, + e, + ) + await self._invalidate_session(room_id) - await self._report_error(tools, str(e)) - raise + await self._report_error(tools, str(e)) + raise - except Exception as e: - logger.exception("Error processing message: %s", e) - await self._report_error(tools, str(e)) - raise + except Exception as e: + logger.exception("Error processing message: %s", e) + await self._report_error(tools, str(e)) + raise + + logger.debug("Message %s processed successfully", msg_id) + finally: + self._release_turn(room_id) - logger.debug("Message %s processed successfully", msg.id) + def _release_turn(self, room_id: str) -> None: + """Resolve the current turn's release future, if still pending. + + Idempotent: a turn with several gated tool calls only needs the + first manual approval to release on_message, and the turn's own + completion (see _run_turn's finally) must release it too when + nothing ever blocked on a human. + """ + release_future = self._turn_release.get(room_id) + if release_future is not None and not release_future.done(): + release_future.set_result(None) + + def _log_turn_task_exception(self, task: asyncio.Task[None]) -> None: + """Retrieve a turn task's exception so asyncio doesn't log it as + "never retrieved" -- _run_turn already reported it to the room. Only + matters for a turn that fails *after* on_message already returned + (post-approval-release); on_message's own ``await turn_task`` still + re-raises normally for a turn that fails before release. + """ + if not task.cancelled(): + task.exception() async def _invalidate_session(self, room_id: str) -> None: """Evict the cached session and client so the next message for this @@ -1125,6 +1231,8 @@ async def on_cleanup(self, room_id: str) -> None: self._room_last_sender.pop(room_id, None) self._notified_declines.pop(room_id, None) self._pending_tool_names.pop(room_id, None) + self._turn_release.pop(room_id, None) + self._turn_tasks.pop(room_id, None) logger.debug("Room %s: Cleaned up Claude SDK session", room_id) # --- Copied from BaseFrameworkAgent._report_error --- @@ -1328,9 +1436,7 @@ async def _resolve_manual_approval( if tools: try: await tools.send_message( - f"Approval requested ({summary}). Token: `{token}`.\n" - f"Reply `/approve {token}` or `/decline {token}`.\n" - "Use `/approvals` to list pending approvals.", + APPROVAL_REQUESTED_TEMPLATE.format(summary=summary, token=token), mentions=mention, ) except Exception: @@ -1346,6 +1452,11 @@ async def _resolve_manual_approval( message="Could not deliver approval prompt, tool use declined" ) + # The request has been posted (or there was nowhere to post it) -- + # either way, on_message must return now so Band's room loop can + # dispatch the reply that will resolve this wait. + self._release_turn(room_id) + # Wait for decision or timeout try: decision_raw = await asyncio.wait_for( @@ -1477,7 +1588,7 @@ async def _handle_approval_command( decision: ApprovalDecision = "accept" if command == "approve" else "decline" notified = await self._send_best_effort( tools, - f"Approval `{token}` resolved as **{decision}**.", + APPROVAL_RESOLVED_TEMPLATE.format(token=token, decision=decision), mention, room_id=room_id, failure_note=f"Failed to send approval resolution notice for token {token}", diff --git a/tests/adapters/test_claude_sdk_adapter.py b/tests/adapters/test_claude_sdk_adapter.py index fa88d5c3d..c783d40bd 100644 --- a/tests/adapters/test_claude_sdk_adapter.py +++ b/tests/adapters/test_claude_sdk_adapter.py @@ -2489,9 +2489,14 @@ class TestPreToolUseHook: """Tests for the PreToolUse hook that enables can_use_tool delegation.""" @pytest.mark.asyncio - async def test_hook_returns_continue_true(self): + async def test_hook_forces_permission_decision_ask(self): result = await _pre_tool_use_continue_hook(None, None, None) - assert result == {"continue_": True} + assert result == { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "ask", + } + } class TestApprovalCleanup: diff --git a/tests/e2e/baseline/smoke/adapters/test_claude_sdk.py b/tests/e2e/baseline/smoke/adapters/test_claude_sdk.py new file mode 100644 index 000000000..ba60fb215 --- /dev/null +++ b/tests/e2e/baseline/smoke/adapters/test_claude_sdk.py @@ -0,0 +1,134 @@ +"""Claude SDK showcase smoke — the manual chat-based approval round trip. + +The generic matrix runs claude_sdk with no ``approval_mode`` set (the SDK's own +``permission_mode`` governs instead; see ``toolkit/builders.py``), so the chat-based +manual relay -- Claude Code's native ``Bash``/``Write``/``Edit`` tools gated through +``can_use_tool``, the adapter posting an ``/approve `` prompt to the room, a +human replying, the turn resuming -- is otherwise never exercised live. Band's own +MCP tools (and any caller ``additional_tools``) are swept into ``allowed_tools`` and +so bypass ``can_use_tool`` entirely; only the CLI's native tools are actually gated, +which is why this smoke compels a ``Bash`` use specifically. + +Construction is bespoke (the matrix builder never sets ``approval_mode`` and its +``prompt``/``features``/``tools`` contract can't express it either), so — like +``test_copilot_sdk.py`` -- there is no ``@with_adapters``/``@per_adapter`` binding; +gating is explicit (``@requires``) and the home lane is pinned with +``@lane(Lane.CORE)`` (claude_sdk is core-lane, not backends). + +Run with: + E2E_TESTS_ENABLED=true BAND_E2E_LANE=core uv run pytest \\ + tests/e2e/baseline/smoke/adapters/test_claude_sdk.py -v -s --no-cov +""" + +from __future__ import annotations + +import re +import tempfile + +import pytest + +from band.adapters.claude_sdk import APPROVAL_RESOLVED_TEMPLATE, ClaudeSDKAdapter +from band.client.streaming import MessageCreatedPayload + +from tests.e2e.baseline.agents import Lane, lane +from tests.e2e.baseline.flaky import flaky_infra +from tests.e2e.baseline.requires import Dep, requires +from tests.e2e.baseline.settings import BaselineSettings +from tests.e2e.baseline.timeouts import slow_turn_budget +from tests.e2e.baseline.toolkit.capture import CaptureFactory +from tests.e2e.baseline.toolkit.provisioning import ( + ResourceManager, + running_provisioned_agent, +) +from tests.e2e.baseline.toolkit.user_ops import UserOps + +# Anchored on the literal text wrapping "{token}" in APPROVAL_REQUESTED_TEMPLATE +# ("Token: `{token}`.") -- not derived from the template itself (same tradeoff +# test_opencode.py accepts for its room-command syntax), so a reworded prompt +# fails this test loudly rather than silently drifting. +TOKEN_RE = re.compile(r"Token: `(\S+?)`\.") + +# Two sequential live turns: the gated tool use, then the resumed turn. +BUDGET = slow_turn_budget(BaselineSettings().e2e_timeout, barriers=2) + + +def _requested_token(messages: list[MessageCreatedPayload]) -> str | None: + """The approval token from the adapter's request prompt, if it posted one.""" + matches = (TOKEN_RE.search(m.content or "") for m in messages) + return next((m.group(1) for m in matches if m), None) + + +def _resolved(messages: list[MessageCreatedPayload], token: str) -> bool: + """Whether the adapter confirmed ``token`` was resolved as accepted.""" + expected = APPROVAL_RESOLVED_TEMPLATE.format(token=token, decision="accept") + return any(expected in (m.content or "") for m in messages) + + +@lane(Lane.CORE) # bespoke build exposes no framework; pin scheduling to core +@requires(Dep.ANTHROPIC) +@flaky_infra("one live Claude Code turn to trigger a Bash tool use can time out") +@pytest.mark.timeout(extra=BUDGET.extra_s) +@pytest.mark.asyncio(loop_scope="session") +async def test_manual_bash_approval_resolved_from_a_mentioned_reply( + baseline_settings: BaselineSettings, + resource_manager: ResourceManager, + user_ops: UserOps, + reply_capture: CaptureFactory, +) -> None: + """A gated ``Bash`` use pauses the turn; a mentioned ``/approve `` resolves it. + + ``approval_mode="manual"`` routes every native-tool ``can_use_tool`` call + through the chat relay, so compelling a ``Bash`` use raises a real approval + request and the adapter posts ``APPROVAL_REQUESTED_TEMPLATE``. The reply is + delivered with the platform's leading ``@handle`` mention block, and + ``_extract_command`` strips that before matching ``/approve`` -- so the + adapter actually *recognizing* the reply and posting + ``APPROVAL_RESOLVED_TEMPLATE`` is the end-to-end guard, not just that a + message was sent. + + The resumed tool output is deliberately not asserted: whether the model + re-runs the command and relays it is model-dependent, whereas recognizing + the reply and resolving the approval is the relay's actual guarantee (same + rationale as ``test_opencode.py``'s manual-approval smoke). + """ + sandbox = tempfile.mkdtemp(prefix="band-e2e-claude-sdk-manual-approval-") + adapter = ClaudeSDKAdapter( + model=baseline_settings.llm_models.anthropic_model, + custom_section="Keep responses short. Use your Bash tool when asked.", + cwd=sandbox, + approval_mode="manual", + ) + + async with running_provisioned_agent( + adapter, resource_manager, label="claude-sdk-manual-approval" + ) as agent: + room_id = await resource_manager.provision_room( + title="e2e-claude-sdk-manual-approval", participants=[agent.id] + ) + async with reply_capture(room_id) as capture: + # Turn 1: compel a Bash tool use -> gated by can_use_tool -> approval prompt. + await user_ops.send_message( + room_id, + "Use your Bash tool to run exactly `echo ok`. You must execute " + "it with the tool, not answer from memory.", + mention_id=agent.id, + mention_name=agent.name, + ) + asked = await capture.wait_until( + lambda msgs: _requested_token(msgs) is not None, + deadline_s=BUDGET.deadline_s, + ) + token = _requested_token(asked) + assert token is not None # the predicate guarantees one + + # Turn 2: the mentioned `/approve ` reply must be RECOGNIZED. + await user_ops.send_message( + room_id, + f"/approve {token}", + mention_id=agent.id, + mention_name=agent.name, + ) + await capture.wait_until( + lambda msgs: _resolved(msgs, token), + deadline_s=BUDGET.deadline_s, + ) diff --git a/tests/e2e/baseline/smoke/adapters/test_codex.py b/tests/e2e/baseline/smoke/adapters/test_codex.py new file mode 100644 index 000000000..2d2e3baf9 --- /dev/null +++ b/tests/e2e/baseline/smoke/adapters/test_codex.py @@ -0,0 +1,95 @@ +"""Codex showcase smokes — adapter-native thought emission on the live backends lane. + +The generic matrix runs Codex without ``Emit.THOUGHTS`` (builder features default +to ``None``, so only config-boolean ``TASK_EVENTS`` lands). These smokes turn +thoughts on and assert the room never receives placeholder noise from empty +reasoning/plan items — the live symptom of empty-summary fallbacks posting +``(reasoning)`` / ``(plan)``. + +Run with: + E2E_TESTS_ENABLED=true BAND_E2E_LANE=backends uv run pytest \\ + tests/e2e/baseline/smoke/adapters/test_codex.py -v -s --no-cov +""" + +from __future__ import annotations + +import pytest + +from band.adapters.codex import CodexAdapter, CodexAdapterConfig +from band.core.types import Emit + +from tests.e2e.baseline.agents import Lane, lane +from tests.e2e.baseline.flaky import flaky_infra +from tests.e2e.baseline.requires import Dep, requires +from tests.e2e.baseline.settings import BaselineSettings +from tests.e2e.baseline.smoke.samples.sample_agents import ( + REPLY_PROMPT, + reasoning_joke_instruction, + unique_marker, +) +from tests.e2e.baseline.toolkit.builders import codex_config_kwargs +from tests.e2e.baseline.toolkit.capture import CaptureFactory +from tests.e2e.baseline.toolkit.provisioning import ResourceManager, running_agent +from tests.e2e.baseline.toolkit.user_ops import UserOps + +PLACEHOLDER_THOUGHTS = ("(reasoning)", "(plan)") + + +@lane(Lane.BACKENDS) # bespoke config exposes no framework; pin scheduling to backends +@requires(Dep.CODEX_CLI, Dep.CODEX_CWD) +@flaky_infra("retry a transient live-turn timeout; assertion failures fail loud") +@pytest.mark.timeout(extra=180) # Codex cold start + one reasoning turn +@pytest.mark.asyncio(loop_scope="session") +async def test_codex_thoughts_are_not_placeholders( + baseline_settings: BaselineSettings, + resource_manager: ResourceManager, + user_ops: UserOps, + reply_capture: CaptureFactory, +) -> None: + """With ``Emit.THOUGHTS`` on and a reasoning summary actually requested, empty + reasoning/plan items must not spam the room. + + Bespoke construction (bypassing the shared registry builder) because the + matrix has no per-test hook for ``reasoning_summary``. Reuses the + builder's own ``codex_config_kwargs`` for cwd/model/command so this test + can't drift from the matrix's env-driven settings, adding only the one + field the builder doesn't expose. Codex only returns a reasoning summary + when one is explicitly requested -- leaving it unset (the matrix default) + means every summary comes back empty and the adapter correctly drops + every one rather than posting a placeholder, so this test would fail + vacuously without requesting one itself. + + A completed reply proves the turn ran. At least one thought event must have + landed — otherwise the placeholder assertion below would pass vacuously + without ever exercising the fix — and none of them may carry the literal + ``(reasoning)`` / ``(plan)`` placeholders the adapter used to emit for empty + summaries. The user message uses ``reasoning_joke_instruction`` so + ``name == marker`` and the ask itself invites reasoning (how a joke might be + badly interpreted). + """ + name = unique_marker("Sam") + config_kwargs = codex_config_kwargs(baseline_settings, prompt=REPLY_PROMPT) + config_kwargs["reasoning_summary"] = "auto" + adapter = CodexAdapter( + config=CodexAdapterConfig(**config_kwargs), + emit=Emit.THOUGHTS, + ) + + identity = await resource_manager.provision_agent("codex-thoughts") + room_id = await resource_manager.provision_room( + title="e2e-codex-thoughts", participants=[identity.id] + ) + async with running_agent(identity, adapter, baseline_settings): + async with reply_capture(room_id) as capture: + mid = await user_ops.send_message( + room_id, + reasoning_joke_instruction(name), + mention_id=identity.id, + mention_name=identity.name, + ) + replies = await capture.wait_for_reply(mid, identity.id) + thoughts = await capture.thoughts(sender_id=identity.id) + + replies.assert_contains_any([name]) + thoughts.assert_at_least(1) + thoughts.assert_contains_none(PLACEHOLDER_THOUGHTS) diff --git a/tests/e2e/baseline/smoke/samples/sample_agents.py b/tests/e2e/baseline/smoke/samples/sample_agents.py index e4736de70..a8a37796c 100644 --- a/tests/e2e/baseline/smoke/samples/sample_agents.py +++ b/tests/e2e/baseline/smoke/samples/sample_agents.py @@ -172,6 +172,32 @@ def unique_marker(prefix: str) -> str: return f"{prefix}-{uuid.uuid4().hex[:8]}" +def reasoning_joke_instruction(name: str) -> str: + """Drive a turn that needs visible reasoning, with ``name`` as the assert token. + + ``name`` is both the person in the joke and the verbatim marker (name == + marker — same pattern as add-band's liveness joke probe). The joke itself + is asked to be harmless (a plain pun) — asking the model to produce + something "that might be badly interpreted" risks a safety refusal instead + of a reply, an unrelated false failure. The reasoning hook is a separate + ask: weigh several distinct ways even a harmless pun could still land + wrong, which pushes multi-step deliberation (and therefore a populated + reasoning summary) without needing the joke to be edgy. This is what lets + Emit.THOUGHTS smokes observe thought events without forcing + ``band_send_event``. + """ + return ( + "We're building a short lesson for kids on being more PC. First, write " + f'one short, harmless pun-based joke using the exact name "{name}" — ' + "nothing edgy, just wordplay. Then, as the actual lesson: think through " + f'at least three distinct ways someone (in this case "{name}") could ' + "still misread or take offense at even a harmless pun like this, " + "weigh how serious each risk " + "is, and explain that reasoning to the kids. " + f'In your final reply, include the exact name "{name}".' + ) + + def file_round_trip_instruction(marker: str) -> str: """Drive one real text-file send, discovery, and read-back in a room. diff --git a/tests/e2e/baseline/toolkit/builders.py b/tests/e2e/baseline/toolkit/builders.py index d04820c1d..4e584587e 100644 --- a/tests/e2e/baseline/toolkit/builders.py +++ b/tests/e2e/baseline/toolkit/builders.py @@ -76,9 +76,15 @@ def _build_claude_sdk( ) -> SimpleAdapter[Any]: from band.adapters.claude_sdk import ClaudeSDKAdapter # noqa: PLC0415 -- isolates the claude_sdk extra from the other frameworks this file builds + # Claude Code gets real Bash/filesystem tools; an unset cwd falls back to + # the process cwd (this repo's own checkout). Mirrors _build_copilot_acp's + # per-cell disposable sandbox. + sandbox = tempfile.mkdtemp(prefix="band-e2e-claude-sdk-") + return ClaudeSDKAdapter( model=s.llm_models.anthropic_model, custom_section=prompt, + cwd=sandbox, additional_tools=_custom_tool_defs(tools), **feature_kwargs(features), ) From b060ba1968caa5c97037efb046e9cddffda24919 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Mon, 14 Sep 2026 18:18:38 +0300 Subject: [PATCH 2/2] fix: close coding agent E2E lifecycle gaps --- src/band/adapters/claude_sdk.py | 22 +++-- tests/adapters/test_claude_sdk_adapter.py | 36 +++++++++ .../smoke/adapters/test_claude_sdk.py | 80 ++++++++++--------- tests/e2e/baseline/toolkit/builders.py | 10 ++- 4 files changed, 99 insertions(+), 49 deletions(-) diff --git a/src/band/adapters/claude_sdk.py b/src/band/adapters/claude_sdk.py index 3310cc4f0..356976d93 100644 --- a/src/band/adapters/claude_sdk.py +++ b/src/band/adapters/claude_sdk.py @@ -787,9 +787,11 @@ async def _run_turn( logger.debug("Message %s processed successfully", msg_id) finally: - self._release_turn(room_id) + self._release_turn(room_id, release_future) - def _release_turn(self, room_id: str) -> None: + def _release_turn( + self, room_id: str, release_future: asyncio.Future[None] | None = None + ) -> None: """Resolve the current turn's release future, if still pending. Idempotent: a turn with several gated tool calls only needs the @@ -797,9 +799,11 @@ def _release_turn(self, room_id: str) -> None: completion (see _run_turn's finally) must release it too when nothing ever blocked on a human. """ - release_future = self._turn_release.get(room_id) - if release_future is not None and not release_future.done(): - release_future.set_result(None) + current_release = self._turn_release.get(room_id) + if current_release is None or current_release.done(): + return + if release_future is None or current_release is release_future: + current_release.set_result(None) def _log_turn_task_exception(self, task: asyncio.Task[None]) -> None: """Retrieve a turn task's exception so asyncio doesn't log it as @@ -1232,7 +1236,9 @@ async def on_cleanup(self, room_id: str) -> None: self._notified_declines.pop(room_id, None) self._pending_tool_names.pop(room_id, None) self._turn_release.pop(room_id, None) - self._turn_tasks.pop(room_id, None) + turn_task = self._turn_tasks.pop(room_id, None) + if turn_task is not None: + turn_task.cancel() logger.debug("Room %s: Cleaned up Claude SDK session", room_id) # --- Copied from BaseFrameworkAgent._report_error --- @@ -1260,6 +1266,10 @@ async def cleanup_all(self) -> None: self._room_last_sender.clear() self._notified_declines.clear() self._pending_tool_names.clear() + for turn_task in self._turn_tasks.values(): + turn_task.cancel() + self._turn_release.clear() + self._turn_tasks.clear() # ------------------------------------------------------------------ # Chat-based approval flow diff --git a/tests/adapters/test_claude_sdk_adapter.py b/tests/adapters/test_claude_sdk_adapter.py index c783d40bd..f048287d0 100644 --- a/tests/adapters/test_claude_sdk_adapter.py +++ b/tests/adapters/test_claude_sdk_adapter.py @@ -697,6 +697,42 @@ async def test_cleanup_without_session_manager_is_safe(self): assert "room-123" not in adapter._room_tools + @pytest.mark.asyncio + async def test_old_turn_cannot_release_a_rejoined_turn(self, mock_tools): + """A turn surviving cleanup must not release a later turn in the same room.""" + adapter = ClaudeSDKAdapter() + old_release = asyncio.get_running_loop().create_future() + adapter._turn_release["room-123"] = old_release + response_started = asyncio.Event() + release_response = asyncio.Event() + + async def wait_for_response(*_args): + response_started.set() + await release_response.wait() + + client = MagicMock() + client.query = AsyncMock() + with patch.object(adapter, "_process_response", side_effect=wait_for_response): + old_turn = asyncio.create_task( + adapter._run_turn( + client, + "room-123", + mock_tools, + "old message", + "old-message-id", + old_release, + ) + ) + await response_started.wait() + adapter._turn_release.pop("room-123") + rejoined_release = asyncio.get_running_loop().create_future() + adapter._turn_release["room-123"] = rejoined_release + + release_response.set() + await old_turn + + assert not rejoined_release.done() + class TestCleanupAll: """Tests for cleanup_all() method.""" diff --git a/tests/e2e/baseline/smoke/adapters/test_claude_sdk.py b/tests/e2e/baseline/smoke/adapters/test_claude_sdk.py index ba60fb215..40c609deb 100644 --- a/tests/e2e/baseline/smoke/adapters/test_claude_sdk.py +++ b/tests/e2e/baseline/smoke/adapters/test_claude_sdk.py @@ -91,44 +91,46 @@ async def test_manual_bash_approval_resolved_from_a_mentioned_reply( the reply and resolving the approval is the relay's actual guarantee (same rationale as ``test_opencode.py``'s manual-approval smoke). """ - sandbox = tempfile.mkdtemp(prefix="band-e2e-claude-sdk-manual-approval-") - adapter = ClaudeSDKAdapter( - model=baseline_settings.llm_models.anthropic_model, - custom_section="Keep responses short. Use your Bash tool when asked.", - cwd=sandbox, - approval_mode="manual", - ) - - async with running_provisioned_agent( - adapter, resource_manager, label="claude-sdk-manual-approval" - ) as agent: - room_id = await resource_manager.provision_room( - title="e2e-claude-sdk-manual-approval", participants=[agent.id] + with tempfile.TemporaryDirectory( + prefix="band-e2e-claude-sdk-manual-approval-" + ) as sandbox: + adapter = ClaudeSDKAdapter( + model=baseline_settings.llm_models.anthropic_model, + custom_section="Keep responses short. Use your Bash tool when asked.", + cwd=sandbox, + approval_mode="manual", ) - async with reply_capture(room_id) as capture: - # Turn 1: compel a Bash tool use -> gated by can_use_tool -> approval prompt. - await user_ops.send_message( - room_id, - "Use your Bash tool to run exactly `echo ok`. You must execute " - "it with the tool, not answer from memory.", - mention_id=agent.id, - mention_name=agent.name, - ) - asked = await capture.wait_until( - lambda msgs: _requested_token(msgs) is not None, - deadline_s=BUDGET.deadline_s, - ) - token = _requested_token(asked) - assert token is not None # the predicate guarantees one - - # Turn 2: the mentioned `/approve ` reply must be RECOGNIZED. - await user_ops.send_message( - room_id, - f"/approve {token}", - mention_id=agent.id, - mention_name=agent.name, - ) - await capture.wait_until( - lambda msgs: _resolved(msgs, token), - deadline_s=BUDGET.deadline_s, + + async with running_provisioned_agent( + adapter, resource_manager, label="claude-sdk-manual-approval" + ) as agent: + room_id = await resource_manager.provision_room( + title="e2e-claude-sdk-manual-approval", participants=[agent.id] ) + async with reply_capture(room_id) as capture: + # Turn 1: compel a Bash tool use -> gated by can_use_tool -> approval prompt. + await user_ops.send_message( + room_id, + "Use your Bash tool to run exactly `echo ok`. You must execute " + "it with the tool, not answer from memory.", + mention_id=agent.id, + mention_name=agent.name, + ) + asked = await capture.wait_until( + lambda msgs: _requested_token(msgs) is not None, + deadline_s=BUDGET.deadline_s, + ) + token = _requested_token(asked) + assert token is not None # the predicate guarantees one + + # Turn 2: the mentioned `/approve ` reply must be RECOGNIZED. + await user_ops.send_message( + room_id, + f"/approve {token}", + mention_id=agent.id, + mention_name=agent.name, + ) + await capture.wait_until( + lambda msgs: _resolved(msgs, token), + deadline_s=BUDGET.deadline_s, + ) diff --git a/tests/e2e/baseline/toolkit/builders.py b/tests/e2e/baseline/toolkit/builders.py index 4e584587e..e769606de 100644 --- a/tests/e2e/baseline/toolkit/builders.py +++ b/tests/e2e/baseline/toolkit/builders.py @@ -19,6 +19,7 @@ import os import tempfile +import weakref from typing import Any from band.core.simple_adapter import SimpleAdapter @@ -79,15 +80,16 @@ def _build_claude_sdk( # Claude Code gets real Bash/filesystem tools; an unset cwd falls back to # the process cwd (this repo's own checkout). Mirrors _build_copilot_acp's # per-cell disposable sandbox. - sandbox = tempfile.mkdtemp(prefix="band-e2e-claude-sdk-") - - return ClaudeSDKAdapter( + sandbox = tempfile.TemporaryDirectory(prefix="band-e2e-claude-sdk-") + adapter = ClaudeSDKAdapter( model=s.llm_models.anthropic_model, custom_section=prompt, - cwd=sandbox, + cwd=sandbox.name, additional_tools=_custom_tool_defs(tools), **feature_kwargs(features), ) + weakref.finalize(adapter, sandbox.cleanup) + return adapter @adapter(