From 2442944f2a219ddc61a8811e1f4adf2d7be20475 Mon Sep 17 00:00:00 2001 From: Natan Zalkin Date: Tue, 29 Sep 2026 14:35:21 +0300 Subject: [PATCH 01/17] feat(acp): pass Copilot model and reasoning effort as CLI flags Copilot's ACP session has no model option. Append --model and --reasoning-effort once on the stdio command, and refuse a flag that is already in command or a TCP transport that cannot take CLI flags. --- src/band/adapters/copilot_acp.py | 33 ++++++++++++++++- .../adapters/test_copilot_acp_model_flags.py | 37 +++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 tests/adapters/test_copilot_acp_model_flags.py diff --git a/src/band/adapters/copilot_acp.py b/src/band/adapters/copilot_acp.py index 074117bf4..89da2e411 100644 --- a/src/band/adapters/copilot_acp.py +++ b/src/band/adapters/copilot_acp.py @@ -44,6 +44,8 @@ logger = logging.getLogger(__name__) DEFAULT_COPILOT_COMMAND: tuple[str, ...] = ("copilot", "--acp") +_MODEL_FLAG = "--model" +_REASONING_EFFORT_FLAG = "--reasoning-effort" @dataclass(frozen=True) @@ -70,6 +72,11 @@ class CopilotACPAdapterConfig: resolve_session_config: SessionConfigResolver | None = None resolve_permission: PermissionResolver | None = None turn_timeout_s: float = DEFAULT_TURN_TIMEOUT_SECONDS + # Appended to the stdio command as --model/--reasoning-effort. Copilot's + # ACP session exposes no model option, and the CLI accepts an unknown + # model at session/new. + model: str | None = None + reasoning_effort: str | None = None class CopilotACPAdapter(ACPClientAdapter): @@ -94,6 +101,11 @@ def __init__( # is misconfigured — fail loudly rather than silently dropping the command. if use_tcp and tuple(config.command) != DEFAULT_COPILOT_COMMAND: raise ValueError("set either command (stdio) or host/port (TCP), not both") + if use_tcp and (config.model or config.reasoning_effort): + raise ValueError( + "model/reasoning_effort are CLI flags and need the stdio transport; " + "configure them on the TCP server instead" + ) # A rejected remote transport owns its environment, so any auth supplied # with it is ignored after this warning. @@ -133,7 +145,26 @@ def __init__( if use_tcp: super().__init__(host=config.host, port=config.port, **common, **features) else: - super().__init__(command=list(config.command), **common, **features) + super().__init__( + command=_command_with_model_flags(config), **common, **features + ) + + +def _command_with_model_flags(config: CopilotACPAdapterConfig) -> list[str]: + """``command`` plus the typed model flags, each given exactly once.""" + command = list(config.command) + for flag, value in ( + (_MODEL_FLAG, config.model), + (_REASONING_EFFORT_FLAG, config.reasoning_effort), + ): + if value is None: + continue + if any(arg == flag or arg.startswith(f"{flag}=") for arg in command): + raise ValueError( + f"{flag} is set both in command and as a typed config field" + ) + command.extend((flag, value)) + return command __all__ = [ diff --git a/tests/adapters/test_copilot_acp_model_flags.py b/tests/adapters/test_copilot_acp_model_flags.py new file mode 100644 index 000000000..992a824ff --- /dev/null +++ b/tests/adapters/test_copilot_acp_model_flags.py @@ -0,0 +1,37 @@ +"""Typed ``model`` / ``reasoning_effort`` flags on the Copilot ACP command.""" + +from __future__ import annotations + +import pytest + +from band.adapters.copilot_acp import CopilotACPAdapter, CopilotACPAdapterConfig + + +def test_copilot_model_flags_are_appended_once() -> None: + adapter = CopilotACPAdapter( + CopilotACPAdapterConfig(model="claude-sonnet-5", reasoning_effort="high") + ) + assert adapter._command[-4:] == [ + "--model", + "claude-sonnet-5", + "--reasoning-effort", + "high", + ] + assert adapter._command.count("--model") == 1 + + +@pytest.mark.parametrize("spliced", ["--model", "--model=gpt-6"]) +def test_copilot_model_flag_in_both_places_is_refused(spliced: str) -> None: + with pytest.raises(ValueError, match="--model is set both"): + CopilotACPAdapter( + CopilotACPAdapterConfig( + command=("copilot", "--acp", spliced), model="claude-sonnet-5" + ) + ) + + +def test_copilot_model_flags_need_stdio() -> None: + with pytest.raises(ValueError, match="need the stdio transport"): + CopilotACPAdapter( + CopilotACPAdapterConfig(host="10.0.0.5", port=8080, model="gpt-6") + ) From c7447024ce357c2082239dc74f51fad209e03d58 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 12:52:47 +0300 Subject: [PATCH 02/17] feat(sdk): add a model selection contract to every adapter SimpleAdapter gains overridable hooks so each adapter can adopt harness- advertised model and reasoning-effort validation on its own schedule: model_selection (what it is configured to use), list_models() (what its harness advertises), and apply_model_selection(selection, room_id=) (switch one room at runtime). The defaults select and advertise nothing, and a runtime switch is refused rather than silently ignored. check_model_selection is the single rule and error wording: an unknown model or an effort the chosen model lacks is rejected naming what is advertised, and a harness that reports nothing is left unchecked. ModelChoice.efforts separates "not reported" (None) from "offers none" (()). SimpleAdapter.startup() now owns the start sequence both Agent.start() and OneShotInvoker.startup() ran by hand (feature negotiation, on_started), and validates the selection after on_started, since adapters start their own harness client there. A rejected selection releases the adapter before the error propagates. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- src/band/agent.py | 19 ++-- src/band/core/model_catalog.py | 155 +++++++++++++++++++++++++++++ src/band/core/simple_adapter.py | 69 +++++++++++++ src/band/runtime/oneshot.py | 16 ++- src/band/runtime/startup.py | 32 ++++++ tests/catalogs.py | 43 ++++++++ tests/core/test_agent_lifecycle.py | 33 +++++- tests/core/test_model_catalog.py | 71 +++++++++++++ tests/core/test_simple_adapter.py | 10 ++ tests/runtime/test_oneshot.py | 11 ++ tests/test_agent.py | 24 +++-- 11 files changed, 449 insertions(+), 34 deletions(-) create mode 100644 src/band/core/model_catalog.py create mode 100644 src/band/runtime/startup.py create mode 100644 tests/catalogs.py create mode 100644 tests/core/test_model_catalog.py diff --git a/src/band/agent.py b/src/band/agent.py index 8c732388c..39c541dbb 100644 --- a/src/band/agent.py +++ b/src/band/agent.py @@ -13,8 +13,8 @@ from band.core.protocols import FrameworkAdapter, Preprocessor from band.core.simple_adapter import SimpleAdapter from band.preprocessing.default import DefaultPreprocessor -from band.runtime.capabilities import prune_unsupported from band.runtime.platform_runtime import PlatformRuntime +from band.runtime.startup import start_adapter from band.runtime.types import ( AgentConfig, ContactEventConfig, @@ -257,18 +257,11 @@ async def start(self) -> None: # setattr rather than assignment: FrameworkAdapter is a Protocol, so # a duck-typed adapter may not declare the attribute. setattr(self._adapter, "platform", self._runtime.connection) # noqa: B010 - if isinstance(self._adapter, SimpleAdapter): - # A bare FrameworkAdapter has no SUPPORTED_CAPABILITIES, so it - # cannot request a gated capability in the first place and - # takes no part in negotiation. - self._adapter.apply_effective_features( - prune_unsupported( - self._adapter.features, self._runtime.feature_flags - ) - ) - await self._adapter.on_started( - self._runtime.agent_name, - self._runtime.agent_description, + await start_adapter( + self._adapter, + agent_name=self._runtime.agent_name, + agent_description=self._runtime.agent_description, + feature_flags=self._runtime.feature_flags, ) # 3. NOW start message processing (connects WebSocket) diff --git a/src/band/core/model_catalog.py b/src/band/core/model_catalog.py new file mode 100644 index 000000000..b8474238c --- /dev/null +++ b/src/band/core/model_catalog.py @@ -0,0 +1,155 @@ +"""Model and reasoning-effort selection, checked against what a harness offers. + +Every adapter speaks this vocabulary: ``SimpleAdapter.model_selection`` is what +it is configured to use, ``SimpleAdapter.list_models()`` is what its harness +advertises, and ``check_model_selection`` is the one rule deciding whether the +two agree. Harnesses report their own catalogs; nothing here hardcodes a model +or effort name. +""" + +from __future__ import annotations + +from collections.abc import Sequence +from enum import StrEnum + +from pydantic import BaseModel, ConfigDict + +from band.core.exceptions import BandConfigError + + +class ModelSetting(StrEnum): + """A selectable model setting.""" + + MODEL = "model" + REASONING_EFFORT = "reasoning_effort" + + +class ModelSelection(BaseModel): + """The model and reasoning effort an adapter is configured to use. + + ``None`` leaves the choice to the harness. + """ + + model_config = ConfigDict(frozen=True) + + model: str | None = None + reasoning_effort: str | None = None + + @property + def is_empty(self) -> bool: + return self.model is None and self.reasoning_effort is None + + +class ModelChoice(BaseModel): + """One model a harness advertises. + + ``efforts`` is ``None`` when the harness does not report them for this + model (the effort goes unchecked), and ``()`` when the model offers none. + """ + + model_config = ConfigDict(frozen=True) + + id: str + label: str | None = None + efforts: tuple[str, ...] | None = None + default_effort: str | None = None + + +class ModelCatalog(BaseModel): + """The models a harness advertises, and the one it currently uses.""" + + model_config = ConfigDict(frozen=True) + + models: tuple[ModelChoice, ...] + current_model: str | None = None + + @property + def model_ids(self) -> tuple[str, ...]: + return tuple(choice.id for choice in self.models) + + def choice(self, model_id: str) -> ModelChoice | None: + return next((choice for choice in self.models if choice.id == model_id), None) + + +class ModelSelectionError(BandConfigError): + """A configured model or effort is not what the harness advertises.""" + + def __init__( + self, + message: str, + *, + setting: ModelSetting, + value: str, + advertised: tuple[str, ...], + ) -> None: + super().__init__(message) + self.setting = setting + self.value = value + self.advertised = advertised + + +def check_model_selection( + selection: ModelSelection, catalog: ModelCatalog | None +) -> None: + """Raise ``ModelSelectionError`` when ``catalog`` rejects ``selection``. + + A ``None`` catalog means the harness advertises nothing, so nothing is + checked. The effort is checked against the selected model, or the + catalog's current model when none is selected. + """ + if catalog is None: + return + if selection.model is not None: + _check_model(selection.model, catalog) + if selection.reasoning_effort is not None: + _check_effort( + selection.reasoning_effort, + model_id=selection.model or catalog.current_model, + catalog=catalog, + ) + + +def _check_model(model_id: str, catalog: ModelCatalog) -> None: + if catalog.choice(model_id) is not None: + return + raise ModelSelectionError( + f'model "{model_id}" is not advertised; available: ' + f"{advertised_listing(catalog.model_ids)}", + setting=ModelSetting.MODEL, + value=model_id, + advertised=catalog.model_ids, + ) + + +def _check_effort(effort: str, *, model_id: str | None, catalog: ModelCatalog) -> None: + choice = catalog.choice(model_id) if model_id is not None else None + if choice is None or choice.efforts is None or effort in choice.efforts: + return + message = ( + f'reasoning effort "{effort}" is not advertised for model "{model_id}"; ' + f"available: {advertised_listing(choice.efforts)}" + if choice.efforts + else f'model "{model_id}" offers no reasoning effort' + ) + raise ModelSelectionError( + message, + setting=ModelSetting.REASONING_EFFORT, + value=effort, + advertised=choice.efforts, + ) + + +def advertised_listing(values: Sequence[str]) -> str: + """Advertised values as shown in a rejection message.""" + return ", ".join(values) or "(none)" + + +__all__ = [ + "ModelCatalog", + "ModelChoice", + "ModelSelection", + "ModelSelectionError", + "ModelSetting", + "advertised_listing", + "check_model_selection", +] diff --git a/src/band/core/simple_adapter.py b/src/band/core/simple_adapter.py index 06dd708cf..2243928f6 100644 --- a/src/band/core/simple_adapter.py +++ b/src/band/core/simple_adapter.py @@ -13,6 +13,11 @@ from band.client.rest import AsyncRestClient from band.client.streaming import ControlMode from band.core.exceptions import BandConfigError +from band.core.model_catalog import ( + ModelCatalog, + ModelSelection, + check_model_selection, +) from band.core.protocols import AgentToolsProtocol, HistoryConverter from band.core.types import ( USAGE_EVENT_TYPE, @@ -299,6 +304,70 @@ async def cleanup_all(self) -> None: release here. """ + @property + def model_selection(self) -> ModelSelection: + """The model and reasoning effort this adapter is configured to use. + + Adapters with typed model settings override this to project them. The + default selects nothing, leaving the choice to the harness. + """ + return ModelSelection() + + async def list_models(self) -> ModelCatalog | None: + """The models this adapter's harness advertises, or ``None``. + + Adapters whose harness reports a catalog override this; ``None`` means + it cannot, so the configured selection goes unchecked. It runs after + ``on_started``, so it may use a client started there. Harnesses whose + catalog belongs to a live session (ACP) keep ``None`` and check the + selection per session instead. + """ + return None + + async def apply_model_selection( + self, selection: ModelSelection, *, room_id: str + ) -> None: + """Switch ``room_id``'s live conversation to ``selection``. + + Adapters whose harness can change model mid-conversation override + this. The default refuses, so a requested switch is never silently + ignored. + """ + del selection, room_id + raise BandConfigError( + f"{type(self).__name__} does not support switching models at runtime" + ) + + async def startup( + self, agent_name: str, agent_description: str, *, features: AdapterFeatures + ) -> None: + """Start for an agent: negotiated ``features``, ``on_started``, then the + model-selection check. + + The runtime calls this once per start; adapters override the hooks it + runs, not this. A rejected selection releases what ``on_started`` + acquired before the ``ModelSelectionError`` propagates. + """ + self.apply_effective_features(features) + await self.on_started(agent_name, agent_description) + try: + await self._validate_model_selection() + except BaseException: + await self._release_after_failed_start() + raise + + async def _validate_model_selection(self) -> None: + selection = self.model_selection + if selection.is_empty: + return + check_model_selection(selection, await self.list_models()) + + async def _release_after_failed_start(self) -> None: + try: + await self.cleanup_all() + except Exception: + logger.exception("Adapter cleanup_all failed after a rejected start") + async def on_started(self, agent_name: str, agent_description: str) -> None: """Override for post-start setup.""" self.agent_name = agent_name diff --git a/src/band/runtime/oneshot.py b/src/band/runtime/oneshot.py index 3da775ca9..9f8889535 100644 --- a/src/band/runtime/oneshot.py +++ b/src/band/runtime/oneshot.py @@ -63,7 +63,6 @@ ) from band.logging_config import current_traceparent from band.platform.link import BandLink -from band.runtime.capabilities import prune_unsupported from band.runtime.context_serialization import context_item_to_dict from band.runtime.formatters import ( build_participants_message, @@ -71,6 +70,7 @@ replace_uuid_mentions, ) from band.runtime.participants import participant_snapshot +from band.runtime.startup import start_adapter from band.runtime.tools import AgentTools # BandLink.get_next_message returns this dataclass, not band.core.types' @@ -197,14 +197,12 @@ async def startup(self) -> None: "platform", self._link.to_platform_connection(self._agent_id), ) - if isinstance(self._adapter, SimpleAdapter): - # A bare FrameworkAdapter has no SUPPORTED_CAPABILITIES, so it - # cannot request a gated capability in the first place and takes - # no part in negotiation. - self._adapter.apply_effective_features( - prune_unsupported(self._adapter.features, self._feature_flags) - ) - await self._adapter.on_started(self._agent_name, self._agent_description) + await start_adapter( + self._adapter, + agent_name=self._agent_name, + agent_description=self._agent_description, + feature_flags=self._feature_flags, + ) self._started = True logger.info( "OneShotInvoker ready: agent_id=%s name=%s", diff --git a/src/band/runtime/startup.py b/src/band/runtime/startup.py new file mode 100644 index 000000000..1a56ece0b --- /dev/null +++ b/src/band/runtime/startup.py @@ -0,0 +1,32 @@ +"""Bring an adapter up for a started agent, the same way from every entry point.""" + +from __future__ import annotations + +from band.core.protocols import FrameworkAdapter +from band.core.simple_adapter import SimpleAdapter +from band.runtime.capabilities import prune_unsupported + + +async def start_adapter( + adapter: FrameworkAdapter | SimpleAdapter, + *, + agent_name: str, + agent_description: str, + feature_flags: dict[str, bool] | None, +) -> None: + """Start ``adapter`` for an agent whose deployment serves ``feature_flags``. + + A bare ``FrameworkAdapter`` has no ``SUPPORTED_CAPABILITIES`` to negotiate + and no model selection to check, so it only receives ``on_started``. + """ + if isinstance(adapter, SimpleAdapter): + await adapter.startup( + agent_name, + agent_description, + features=prune_unsupported(adapter.features, feature_flags), + ) + return + await adapter.on_started(agent_name, agent_description) + + +__all__ = ["start_adapter"] diff --git a/tests/catalogs.py b/tests/catalogs.py new file mode 100644 index 000000000..6ca79eb10 --- /dev/null +++ b/tests/catalogs.py @@ -0,0 +1,43 @@ +"""An adapter whose harness advertises a model catalog, for start-up checks. + +The catalog is only reachable after ``on_started``, like a client that +``on_started`` launches. +""" + +from __future__ import annotations + +from band.core.model_catalog import ModelCatalog, ModelChoice, ModelSelection +from band.core.simple_adapter import SimpleAdapter + +SONNET_CATALOG = ModelCatalog(models=(ModelChoice(id="sonnet", efforts=("high",)),)) + + +class CatalogAdapter(SimpleAdapter[None]): + """Lists ``catalog``, but only once ``on_started`` has run.""" + + def __init__( + self, selection: ModelSelection, catalog: ModelCatalog = SONNET_CATALOG + ) -> None: + super().__init__() + self._selection = selection + self._catalog = catalog + self._started_catalog: ModelCatalog | None = None + self.cleaned_up = False + + async def on_started(self, agent_name: str, agent_description: str) -> None: + await super().on_started(agent_name, agent_description) + self._started_catalog = self._catalog + + @property + def model_selection(self) -> ModelSelection: + return self._selection + + async def list_models(self) -> ModelCatalog | None: + assert self._started_catalog is not None, "listed before on_started" + return self._started_catalog + + async def on_message(self, *args: object, **kwargs: object) -> None: + del args, kwargs + + async def cleanup_all(self) -> None: + self.cleaned_up = True diff --git a/tests/core/test_agent_lifecycle.py b/tests/core/test_agent_lifecycle.py index e284b1754..7497ce9ff 100644 --- a/tests/core/test_agent_lifecycle.py +++ b/tests/core/test_agent_lifecycle.py @@ -12,13 +12,16 @@ import pytest from band.agent import Agent +from band.core.model_catalog import ModelSelection, ModelSelectionError +from tests.catalogs import CatalogAdapter -def make_agent(adapter: object) -> Agent: +def make_agent(adapter: object, *, started: bool = True) -> Agent: runtime = AsyncMock() runtime.stop.return_value = True + runtime.feature_flags = None agent = Agent(runtime=runtime, adapter=adapter) # type: ignore[arg-type] - agent._started = True + agent._started = started return agent @@ -37,6 +40,32 @@ async def test_runtime_start_failure_rolls_back_adapter(self): adapter.cleanup_all.assert_awaited_once() +class TestStartValidatesModelSelection: + @pytest.mark.asyncio + async def test_an_advertised_selection_starts_after_on_started_lists_it( + self, + ) -> None: + agent = make_agent( + CatalogAdapter(ModelSelection(model="sonnet", reasoning_effort="high")), + started=False, + ) + + await agent.start() + + agent._runtime.start.assert_awaited_once() + + @pytest.mark.asyncio + async def test_a_rejected_selection_fails_start_before_connecting(self) -> None: + adapter = CatalogAdapter(ModelSelection(model="gpt-9")) + agent = make_agent(adapter, started=False) + + with pytest.raises(ModelSelectionError, match='model "gpt-9"'): + await agent.start() + + agent._runtime.start.assert_not_awaited() + assert adapter.cleaned_up + + class TestStopCleansUpAdapter: @pytest.mark.asyncio async def test_stop_calls_adapter_cleanup_all(self): diff --git a/tests/core/test_model_catalog.py b/tests/core/test_model_catalog.py new file mode 100644 index 000000000..a598ddc99 --- /dev/null +++ b/tests/core/test_model_catalog.py @@ -0,0 +1,71 @@ +"""The one rule deciding whether a harness accepts a model/effort selection.""" + +from __future__ import annotations + +import pytest + +from band.core.model_catalog import ( + ModelCatalog, + ModelChoice, + ModelSelection, + ModelSelectionError, + ModelSetting, + check_model_selection, +) + +CATALOG = ModelCatalog( + models=( + ModelChoice(id="sonnet", efforts=("low", "high", "max")), + ModelChoice(id="haiku", efforts=()), + ModelChoice(id="gpt", efforts=None), + ), + current_model="sonnet", +) + + +def rejection(selection: ModelSelection) -> ModelSelectionError: + with pytest.raises(ModelSelectionError) as caught: + check_model_selection(selection, CATALOG) + return caught.value + + +@pytest.mark.parametrize( + "selection", + [ + ModelSelection(model="haiku"), + ModelSelection(model="sonnet", reasoning_effort="max"), + ModelSelection(reasoning_effort="low"), # against the current model + ModelSelection(model="gpt", reasoning_effort="anything"), # not reported + ], +) +def test_advertised_selections_pass(selection: ModelSelection) -> None: + check_model_selection(selection, CATALOG) + + +def test_anything_passes_when_the_harness_advertises_no_catalog() -> None: + check_model_selection(ModelSelection(model="x", reasoning_effort="y"), None) + + +def test_unknown_model_names_what_is_advertised() -> None: + error = rejection(ModelSelection(model="gpt-9")) + + assert (error.setting, error.value) == (ModelSetting.MODEL, "gpt-9") + assert ( + str(error) == 'model "gpt-9" is not advertised; available: sonnet, haiku, gpt' + ) + + +def test_effort_is_checked_against_the_selected_model_not_the_current_one() -> None: + error = rejection(ModelSelection(model="haiku", reasoning_effort="high")) + + assert (error.setting, error.value) == (ModelSetting.REASONING_EFFORT, "high") + assert str(error) == 'model "haiku" offers no reasoning effort' + + +def test_unadvertised_effort_names_the_models_efforts() -> None: + error = rejection(ModelSelection(reasoning_effort="xhigh")) + + assert str(error) == ( + 'reasoning effort "xhigh" is not advertised for model "sonnet"; ' + "available: low, high, max" + ) diff --git a/tests/core/test_simple_adapter.py b/tests/core/test_simple_adapter.py index 82f02267e..241560e39 100644 --- a/tests/core/test_simple_adapter.py +++ b/tests/core/test_simple_adapter.py @@ -5,6 +5,8 @@ import pytest +from band.core.exceptions import BandConfigError +from band.core.model_catalog import ModelSelection from band.core.protocols import FrameworkAdapter, HistoryConverter from band.core.simple_adapter import SimpleAdapter from band.core.types import AgentInput, HistoryProvider, PlatformMessage @@ -254,6 +256,14 @@ async def test_defaults_empty(self): assert adapter.agent_description == "" +class TestApplyModelSelection: + async def test_an_adapter_that_cannot_switch_models_refuses(self) -> None: + with pytest.raises(BandConfigError, match="does not support switching"): + await RecordingAdapter().apply_model_selection( + ModelSelection(model="sonnet"), room_id="room-1" + ) + + class TestOnCleanup: """Tests for on_cleanup() lifecycle hook.""" diff --git a/tests/runtime/test_oneshot.py b/tests/runtime/test_oneshot.py index 9dbfa874f..9029966e7 100644 --- a/tests/runtime/test_oneshot.py +++ b/tests/runtime/test_oneshot.py @@ -23,6 +23,7 @@ from pydantic import BaseModel, Field from band.adapters.anthropic import AnthropicAdapter +from band.core.model_catalog import ModelSelection, ModelSelectionError from band.core.simple_adapter import SimpleAdapter from band.core.types import Capability from band.runtime.capabilities import FeatureFlag @@ -34,6 +35,7 @@ _lookup_sender_name, _parse_inserted_at, ) +from tests.catalogs import CatalogAdapter from tests.runtime.conftest import ctx_item, make_link_mock, platform_msg @@ -232,6 +234,15 @@ async def test_keeps_files_capability_when_flag_on(self) -> None: assert Capability.FILES in adapter.features.capabilities + async def test_startup_rejects_a_model_the_harness_does_not_offer(self) -> None: + adapter = CatalogAdapter(ModelSelection(model="gpt-9")) + invoker = OneShotInvoker(link=make_link_mock(), adapter=adapter, agent_id="a") + + with pytest.raises(ModelSelectionError, match='model "gpt-9"'): + await invoker.startup() + + assert adapter.cleaned_up + async def test_startup_is_idempotent(self) -> None: link = make_link_mock() adapter = _make_adapter_mock() diff --git a/tests/test_agent.py b/tests/test_agent.py index 86abd6f68..8e6156ea6 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -13,7 +13,7 @@ RoomAddedPayload, ) from band.core.simple_adapter import SimpleAdapter -from band.core.types import AdapterFeatures, AgentInput, Capability +from band.core.types import AgentInput, Capability from band.platform.event import MessageEvent, ParticipantAddedEvent, RoomAddedEvent from band.preprocessing.default import DefaultPreprocessor from band.runtime.capabilities import FeatureFlag @@ -423,19 +423,23 @@ class TestSimpleAdapterIntegration: @pytest.mark.asyncio async def test_works_with_simple_adapter(self, mock_runtime): - """Should work with SimpleAdapter subclass.""" - # Create a mock SimpleAdapter - adapter = MagicMock(spec=SimpleAdapter) - adapter.on_started = AsyncMock() - adapter.on_cleanup = AsyncMock() - adapter.on_event = AsyncMock() - adapter.features = AdapterFeatures() - + """A SimpleAdapter subclass is started with the agent's identity.""" + adapter = MinimalAdapter() agent = Agent(runtime=mock_runtime, adapter=adapter) await agent.start() - adapter.on_started.assert_awaited_once() + assert (adapter.agent_name, adapter.agent_description) == ( + "TestBot", + "A test bot", + ) + + +class MinimalAdapter(SimpleAdapter): + """A SimpleAdapter that keeps every default hook.""" + + async def on_message(self, *args, **kwargs) -> None: + pass class FilesAdapter(SimpleAdapter): From 602e78a54435624c923617da08e05b11889d235c Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 12:52:56 +0300 Subject: [PATCH 03/17] fix(acp): select Copilot model and effort from the live session catalog Hosted Copilot's ACP session/new advertises a model select and a per-model reasoning_effort select, and silently ignores an unknown --model or an effort the model lacks. The typed model/reasoning_effort now go through session/set_config_option instead of CLI flags. Under BYOK the session advertises neither, so a typed value fails loudly and the provider env (COPILOT_MODEL) picks the model. ACPClientAdapter implements the adapter contract per session: it selects the model, finds the effort select again in the catalog the agent returns (a model switch can add, replace, or drop it), then selects the effort. Anything not advertised fails the room turn before the first prompt, naming what is offered. apply_model_selection switches one live room against the catalog that room's session last returned. Options are found by the spec-reserved model/thought_level categories; agents that publish them differently override locate_model_options. A typed selection and resolve_session_config are exclusive. Session-config rejections now list the advertised options and values. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- src/band/adapters/copilot_acp.py | 39 +--- src/band/integrations/acp/client_adapter.py | 147 +++++++++++++-- src/band/integrations/acp/model_selection.py | 175 ++++++++++++++++++ src/band/integrations/acp/session_config.py | 24 ++- tests/adapters/test_copilot_acp_adapter.py | 163 +++++++++++++++- .../adapters/test_copilot_acp_model_flags.py | 37 ---- .../integrations/acp/acp_toolkit/__init__.py | 9 +- tests/integrations/acp/acp_toolkit/agent.py | 71 ++++++- tests/integrations/acp/acp_toolkit/harness.py | 16 ++ tests/integrations/acp/test_session_config.py | 115 ++++++++++-- 10 files changed, 682 insertions(+), 114 deletions(-) create mode 100644 src/band/integrations/acp/model_selection.py delete mode 100644 tests/adapters/test_copilot_acp_model_flags.py diff --git a/src/band/adapters/copilot_acp.py b/src/band/adapters/copilot_acp.py index 89da2e411..e391ae3a2 100644 --- a/src/band/adapters/copilot_acp.py +++ b/src/band/adapters/copilot_acp.py @@ -31,6 +31,7 @@ from typing_extensions import Unpack +from band.core.model_catalog import ModelSelection from band.core.types import FeatureKwargs from band.integrations.acp.client_adapter import ( DEFAULT_TURN_TIMEOUT_SECONDS, @@ -44,8 +45,6 @@ logger = logging.getLogger(__name__) DEFAULT_COPILOT_COMMAND: tuple[str, ...] = ("copilot", "--acp") -_MODEL_FLAG = "--model" -_REASONING_EFFORT_FLAG = "--reasoning-effort" @dataclass(frozen=True) @@ -72,9 +71,10 @@ class CopilotACPAdapterConfig: resolve_session_config: SessionConfigResolver | None = None resolve_permission: PermissionResolver | None = None turn_timeout_s: float = DEFAULT_TURN_TIMEOUT_SECONDS - # Appended to the stdio command as --model/--reasoning-effort. Copilot's - # ACP session exposes no model option, and the CLI accepts an unknown - # model at session/new. + # Selected from each session's advertised catalog; a value it does not + # offer fails the turn. Only GitHub-hosted sessions advertise these; under + # BYOK the provider env (COPILOT_MODEL) picks the model. Exclusive with + # resolve_session_config. model: str | None = None reasoning_effort: str | None = None @@ -101,11 +101,6 @@ def __init__( # is misconfigured — fail loudly rather than silently dropping the command. if use_tcp and tuple(config.command) != DEFAULT_COPILOT_COMMAND: raise ValueError("set either command (stdio) or host/port (TCP), not both") - if use_tcp and (config.model or config.reasoning_effort): - raise ValueError( - "model/reasoning_effort are CLI flags and need the stdio transport; " - "configure them on the TCP server instead" - ) # A rejected remote transport owns its environment, so any auth supplied # with it is ignored after this warning. @@ -138,6 +133,9 @@ def __init__( "inject_band_tools": config.inject_band_tools, "custom_section": config.custom_section, "resolve_session_config": config.resolve_session_config, + "model_selection": ModelSelection( + model=config.model, reasoning_effort=config.reasoning_effort + ), "resolve_permission": config.resolve_permission, "turn_timeout_s": config.turn_timeout_s, } @@ -145,26 +143,7 @@ def __init__( if use_tcp: super().__init__(host=config.host, port=config.port, **common, **features) else: - super().__init__( - command=_command_with_model_flags(config), **common, **features - ) - - -def _command_with_model_flags(config: CopilotACPAdapterConfig) -> list[str]: - """``command`` plus the typed model flags, each given exactly once.""" - command = list(config.command) - for flag, value in ( - (_MODEL_FLAG, config.model), - (_REASONING_EFFORT_FLAG, config.reasoning_effort), - ): - if value is None: - continue - if any(arg == flag or arg.startswith(f"{flag}=") for arg in command): - raise ValueError( - f"{flag} is set both in command and as a typed config field" - ) - command.extend((flag, value)) - return command + super().__init__(command=list(config.command), **common, **features) __all__ = [ diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index 46c19e69d..90524298e 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -5,7 +5,14 @@ import asyncio import logging import shutil -from collections.abc import AsyncIterator, Awaitable, Callable, Coroutine, Mapping +from collections.abc import ( + AsyncIterator, + Awaitable, + Callable, + Coroutine, + Mapping, + Sequence, +) from contextlib import asynccontextmanager, suppress from dataclasses import dataclass from typing import Any, ClassVar, TypeAlias @@ -18,6 +25,7 @@ HttpMcpServer, NewSessionResponse, PermissionOption, + SetSessionConfigOptionResponse, SseMcpServer, ) from band_sdk_core import AgentFailure @@ -26,6 +34,8 @@ from band.converters.acp_client import ACPClientHistoryConverter from band.converters.helpers import build_replay_messages from band.core.delivery import DeliveryFailedError, reraise_delivery_cause +from band.core.exceptions import BandConfigError +from band.core.model_catalog import ModelSelection from band.core.protocols import ( FAILURE_CODE_TIMEOUT, GENERIC_PROVIDER_FAILURE_MESSAGE, @@ -58,6 +68,11 @@ ACPClientSessionState, BandACPClient, ) +from band.integrations.acp.model_selection import ( + ACPModelOptions, + apply_model_selection, + locate_model_options, +) from band.integrations.acp.room_emitter import RoomTurnEmitter from band.integrations.acp.session_config import ( RESOLVER_CONFIG_OPTION_ID, @@ -65,6 +80,7 @@ ACPConfigRequest, SessionConfigOption, SessionConfigResolver, + SessionConfigSetter, apply_session_config_selections, session_config_options, ) @@ -198,6 +214,19 @@ def _resolve_launcher(command: list[str]) -> list[str]: return [resolved, *command[1:]] if resolved else list(command) +def _config_setter(runtime: ACPRuntime) -> SessionConfigSetter: + """``session/set_config_option`` on ``runtime``, in the applier's shape.""" + + async def set_option( + session_id: str, option_id: str, value: str + ) -> SetSessionConfigOptionResponse | None: + return await runtime.set_config_option( + session_id=session_id, config_id=option_id, value=value + ) + + return set_option + + def _to_agent_failure(exc: Exception) -> AgentFailure: """Parse a turn-ending exception into the shared provider-failure shape. @@ -250,12 +279,18 @@ def __init__( client_capabilities: ClientCapabilities | None = None, use_unstable_protocol: bool = False, turn_timeout_s: float = DEFAULT_TURN_TIMEOUT_SECONDS, + model_selection: ModelSelection | None = None, **features: Unpack[FeatureKwargs], ) -> None: super().__init__( history_converter=ACPClientHistoryConverter(), **features, ) + model_selection = model_selection or ModelSelection() + if not model_selection.is_empty and resolve_session_config is not None: + raise ValueError( + "set either model_selection or resolve_session_config, not both" + ) if cwd is not None: raise ValueError( "cwd is not supported; use workspace_for_room or the default" @@ -281,6 +316,7 @@ def __init__( self._auth_method = auth_method self._profile = profile self._resolve_session_config = resolve_session_config + self._model_selection = model_selection self._resolve_permission = resolve_permission self._client_capabilities = client_capabilities self._use_unstable_protocol = use_unstable_protocol @@ -292,6 +328,9 @@ def __init__( self._turn_timeout_s = turn_timeout_s self._room_to_session: dict[str, str] = {} + # Each live session's config catalog as of its last change, so a runtime + # model switch is checked against what the session now advertises. + self._room_catalogs: dict[str, tuple[SessionConfigOption, ...]] = {} self._session_initializers: dict[str, SessionInitializer] = {} self._room_tools: dict[str, AgentToolsProtocol] = {} self._background_tasks: set[asyncio.Task[None]] = set() @@ -308,6 +347,46 @@ def __init__( # and is never stopped -- a real leaked server, not just a failed turn. self._stopped = False + @property + def model_selection(self) -> ModelSelection: + """Applied per session from its live catalog, not checked at start.""" + return self._model_selection + + async def apply_model_selection( + self, selection: ModelSelection, *, room_id: str + ) -> None: + """Switch ``room_id``'s live ACP session, checked against its catalog. + + Raises ``ACPConfigError`` naming what the session offers when it + rejects ``selection``; new rooms keep the configured selection. + """ + async with self._session_lock: + session_id = self._room_to_session.get(room_id) + runtime = self._runtimes.get(room_id) + catalog = self._room_catalogs.get(room_id, ()) + if session_id is None or runtime is None: + raise BandConfigError(f"room {room_id} has no live ACP session to switch") + catalog = await apply_model_selection( + session_id=session_id, + config_options=catalog, + selection=selection, + locate=self.locate_model_options, + set_option=_config_setter(runtime), + ) + async with self._session_lock: + if self._room_to_session.get(room_id) == session_id: + self._room_catalogs[room_id] = catalog + + def locate_model_options( + self, options: Sequence[SessionConfigOption] + ) -> ACPModelOptions: + """Find the session's model and effort selects. + + The default reads the spec-reserved categories; agents that publish + them under other ids or categories override this. + """ + return locate_model_options(options) + def apply_effective_features(self, features: AdapterFeatures) -> None: """Rebuild the lazy MCP registration after capability negotiation.""" super().apply_effective_features(features) @@ -893,7 +972,7 @@ async def _restore_session( return None try: - await self._configure_session( + catalog = await self._configure_session( runtime, room_id, session_id, @@ -902,7 +981,7 @@ async def _restore_session( except BaseException: await self._close_fresh_session(runtime, session_id) raise - await self._record_session(room_id, session_id) + await self._record_session(room_id, session_id, catalog) logger.debug("Loaded ACP session mapping: %s -> %s", room_id, session_id) return session_id @@ -911,13 +990,13 @@ async def _create_session( ) -> str: """Create, configure, and publish a session for one room.""" async with self._fresh_session(runtime, room_id, mcp_servers) as session: - await self._configure_session( + catalog = await self._configure_session( runtime, room_id, session.session_id, session_config_options(session), ) - await self._record_session(room_id, session.session_id) + await self._record_session(room_id, session.session_id, catalog) logger.info( "Created ACP session %s for room %s (mcp_servers=%d)", @@ -950,10 +1029,16 @@ async def _fresh_session( await self._close_fresh_session(runtime, session.session_id) raise - async def _record_session(self, room_id: str, session_id: str) -> None: + async def _record_session( + self, + room_id: str, + session_id: str, + catalog: tuple[SessionConfigOption, ...], + ) -> None: """Publish a fully initialized session to its room.""" async with self._session_lock: self._room_to_session[room_id] = session_id + self._room_catalogs[room_id] = catalog async def _close_fresh_session(self, runtime: ACPRuntime, session_id: str) -> None: """Best-effort cleanup when configuration prevented first use.""" @@ -1025,14 +1110,40 @@ async def _configure_session( room_id: str, session_id: str, config_options: tuple[SessionConfigOption, ...] | None, - ) -> None: - """Apply caller-selected values from the session's live ACP catalog.""" - if self._resolve_session_config is None: - return - + ) -> tuple[SessionConfigOption, ...]: + """Apply caller-selected values; return the session's resulting catalog.""" catalog = tuple(config_options or ()) + selection = self.model_selection + if not selection.is_empty: + return await apply_model_selection( + session_id=session_id, + config_options=catalog, + selection=selection, + locate=self.locate_model_options, + set_option=_config_setter(runtime), + ) + if self._resolve_session_config is not None: + return await self._apply_resolved_config( + resolver=self._resolve_session_config, + room_id=room_id, + session_id=session_id, + catalog=catalog, + set_option=_config_setter(runtime), + ) + return catalog + + async def _apply_resolved_config( + self, + *, + resolver: SessionConfigResolver, + room_id: str, + session_id: str, + catalog: tuple[SessionConfigOption, ...], + set_option: SessionConfigSetter, + ) -> tuple[SessionConfigOption, ...]: + """Apply what the caller's ``resolve_session_config`` selects.""" try: - selections = await self._resolve_session_config( + selections = await resolver( ACPConfigRequest( room_id=room_id, session_id=session_id, @@ -1049,17 +1160,13 @@ async def _configure_session( message=f"ACP session configuration resolver failed: {error}", ) from error if selections is None: - return + return catalog - await apply_session_config_selections( + return await apply_session_config_selections( session_id=session_id, config_options=catalog, selections=selections, - set_option=lambda session_id, option_id, value: runtime.set_config_option( - session_id=session_id, - config_id=option_id, - value=value, - ), + set_option=set_option, ) async def _report_config_error( @@ -1158,6 +1265,7 @@ def _build_prompt_text( async def on_cleanup(self, room_id: str) -> None: async with self._session_lock: session_id = self._room_to_session.pop(room_id, None) + self._room_catalogs.pop(room_id, None) initializer = self._session_initializers.pop(room_id, None) self._room_tools.pop(room_id, None) if session_id: @@ -1196,6 +1304,7 @@ async def cleanup_all(self, *, final: bool = True) -> None: initializers = tuple(self._session_initializers.values()) self._session_initializers.clear() self._room_to_session.clear() + self._room_catalogs.clear() self._room_tools.clear() self._bootstrapped_sessions.clear() runtimes = list(self._runtimes.values()) diff --git a/src/band/integrations/acp/model_selection.py b/src/band/integrations/acp/model_selection.py new file mode 100644 index 000000000..c2e7a8549 --- /dev/null +++ b/src/band/integrations/acp/model_selection.py @@ -0,0 +1,175 @@ +"""Apply a typed ``ModelSelection`` to an ACP session's live config catalog.""" + +from __future__ import annotations + +from collections.abc import Callable, Sequence +from dataclasses import dataclass + +from acp.schema import SessionConfigOptionSelect, SessionConfigSelectOption + +from band.core.model_catalog import ( + ModelCatalog, + ModelChoice, + ModelSelection, + ModelSelectionError, + ModelSetting, + check_model_selection, +) +from band.integrations.acp.session_config import ( + ACPConfigError, + SessionConfigOption, + SessionConfigSetter, + apply_session_config_selections, + flatten_select_options, + select_values, +) + +# Category ids the ACP spec reserves for these selectors. +MODEL_CATEGORY = "model" +THOUGHT_LEVEL_CATEGORY = "thought_level" + + +@dataclass(frozen=True) +class ACPModelOptions: + """The selects a session uses for its model and reasoning effort.""" + + model: SessionConfigOptionSelect | None + effort: SessionConfigOptionSelect | None + + def option_for(self, setting: ModelSetting) -> SessionConfigOptionSelect | None: + match setting: + case ModelSetting.MODEL: + return self.model + case ModelSetting.REASONING_EFFORT: + return self.effort + + def model_catalog(self) -> ModelCatalog | None: + """The advertised models; efforts are known only for the current one.""" + if self.model is None: + return None + return ModelCatalog( + models=tuple( + self._choice(entry, current=self.model.current_value) + for entry in flatten_select_options(self.model.options) + ), + current_model=self.model.current_value, + ) + + def _choice(self, entry: SessionConfigSelectOption, *, current: str) -> ModelChoice: + if entry.value != current: + return ModelChoice(id=entry.value, label=entry.name) + if self.effort is None: + return ModelChoice(id=entry.value, label=entry.name, efforts=()) + return ModelChoice( + id=entry.value, + label=entry.name, + efforts=select_values(self.effort), + default_effort=self.effort.current_value, + ) + + +def locate_model_options(options: Sequence[SessionConfigOption]) -> ACPModelOptions: + """Find the model and effort selects by their spec-reserved categories.""" + return ACPModelOptions( + model=_select_in_category(options, MODEL_CATEGORY), + effort=_select_in_category(options, THOUGHT_LEVEL_CATEGORY), + ) + + +async def apply_model_selection( + *, + session_id: str, + config_options: Sequence[SessionConfigOption], + selection: ModelSelection, + locate: Callable[[Sequence[SessionConfigOption]], ACPModelOptions], + set_option: SessionConfigSetter, +) -> tuple[SessionConfigOption, ...]: + """Select the model, then the effort from the catalog that model returns. + + Choosing a model can add, remove, or replace the effort select, so the + effort is located and checked only after the model has been applied. + Returns the catalog after the last change. + """ + catalog = tuple(config_options) + for setting, value in ( + (ModelSetting.MODEL, selection.model), + (ModelSetting.REASONING_EFFORT, selection.reasoning_effort), + ): + if value is None: + continue + catalog = await _apply_setting( + session_id=session_id, + catalog=catalog, + setting=setting, + value=value, + locate=locate, + set_option=set_option, + ) + return catalog + + +async def _apply_setting( + *, + session_id: str, + catalog: tuple[SessionConfigOption, ...], + setting: ModelSetting, + value: str, + locate: Callable[[Sequence[SessionConfigOption]], ACPModelOptions], + set_option: SessionConfigSetter, +) -> tuple[SessionConfigOption, ...]: + located = locate(catalog) + option = located.option_for(setting) + option_id = option.id if option is not None else setting + try: + check_model_selection(_selection_of(setting, value), located.model_catalog()) + except ModelSelectionError as error: + raise ACPConfigError( + session_id=session_id, + option_id=option_id, + selected_value=value, + message=str(error), + ) from error + if option is None: + raise ACPConfigError( + session_id=session_id, + option_id=option_id, + selected_value=value, + message=f"ACP session advertises no {setting} option.", + ) + return await apply_session_config_selections( + session_id=session_id, + config_options=catalog, + selections={option.id: value}, + set_option=set_option, + ) + + +def _selection_of(setting: ModelSetting, value: str) -> ModelSelection: + match setting: + case ModelSetting.MODEL: + return ModelSelection(model=value) + case ModelSetting.REASONING_EFFORT: + return ModelSelection(reasoning_effort=value) + + +def _select_in_category( + options: Sequence[SessionConfigOption], category: str +) -> SessionConfigOptionSelect | None: + return next( + ( + option + for option in options + if isinstance(option, SessionConfigOptionSelect) + and option.category == category + ), + None, + ) + + +__all__ = [ + "MODEL_CATEGORY", + "THOUGHT_LEVEL_CATEGORY", + "ACPModelOptions", + "apply_model_selection", + "locate_model_options", +] diff --git a/src/band/integrations/acp/session_config.py b/src/band/integrations/acp/session_config.py index 01852e051..013719d17 100644 --- a/src/band/integrations/acp/session_config.py +++ b/src/band/integrations/acp/session_config.py @@ -15,6 +15,8 @@ SetSessionConfigOptionResponse, ) +from band.core.model_catalog import advertised_listing + SessionConfigOption: TypeAlias = SessionConfigOptionSelect | SessionConfigOptionBoolean SessionConfigSelections: TypeAlias = Mapping[str, str | None] SessionConfigSetter: TypeAlias = Callable[ @@ -67,6 +69,11 @@ def flatten_select_options( return tuple(flattened) +def select_values(option: SessionConfigOptionSelect) -> tuple[str, ...]: + """The values an ACP select option advertises, groups flattened.""" + return tuple(entry.value for entry in flatten_select_options(option.options)) + + def session_config_options(response: object) -> tuple[SessionConfigOption, ...] | None: """Return a response's complete, typed ACP configuration catalog.""" options = getattr(response, "config_options", None) @@ -108,19 +115,25 @@ async def apply_session_config_selections( option = next((entry for entry in catalog if entry.id == option_id), None) if not isinstance(option, SessionConfigOptionSelect): + select_ids = tuple( + entry.id + for entry in catalog + if isinstance(entry, SessionConfigOptionSelect) + ) raise ACPConfigError( session_id=session_id, option_id=option_id, selected_value=selected_value, - message=f'ACP config option "{option_id}" is not available.', + message=( + f'ACP config option "{option_id}" is not available; advertised ' + f"options: {advertised_listing(select_ids)}." + ), ) if selected_value == option.current_value: continue - available_values = { - entry.value for entry in flatten_select_options(option.options) - } + available_values = select_values(option) if selected_value not in available_values: raise ACPConfigError( session_id=session_id, @@ -128,7 +141,8 @@ async def apply_session_config_selections( selected_value=selected_value, message=( f'ACP config value "{selected_value}" is not advertised ' - f'for option "{option_id}".' + f'for option "{option_id}"; advertised values: ' + f"{advertised_listing(available_values)}." ), ) diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index ec4b00d2a..84e70ad6e 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -3,13 +3,17 @@ CopilotACPAdapter is a thin specialization of ACPClientAdapter — its contract is how a CopilotACPAdapterConfig maps onto the base adapter's transport, auth, and system-context wiring. Runtime/on_started behavior is covered by the generic ACP -client suite (tests/integrations/acp/), so these are construction-level tests. +client suite (tests/integrations/acp/); the model-selection tests drive the +adapter against an in-process fake shaped like Copilot's live catalog. """ from __future__ import annotations import logging +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager from pathlib import Path +from typing import Any import pytest from pydantic import BaseModel @@ -19,9 +23,14 @@ CopilotACPAdapter, CopilotACPAdapterConfig, ) +from band.core.exceptions import BandConfigError +from band.core.model_catalog import ModelSelection from band.integrations.acp.client_adapter import ACPClientAdapter from band.integrations.acp.client_profiles import NoopACPClientProfile -from band.integrations.acp.session_config import ACPConfigRequest +from band.integrations.acp.client_types import ACPClientSessionState +from band.integrations.acp.session_config import ACPConfigError, ACPConfigRequest +from tests.integrations.acp.acp_toolkit.agent import FakeACPAgent +from tests.integrations.acp.acp_toolkit.harness import AcpSession, started_acp_adapter class TestCopilotACPAdapterConstruction: @@ -157,3 +166,153 @@ def test_tcp_with_auth_warns_before_rejection( ) ) assert any("ignored over TCP" in r.message for r in caplog.records) + + +# Copilot CLI 1.0.89's efforts per model (probed live): selecting a model +# replaces the effort select, or drops it for a model without efforts. +COPILOT_EFFORTS = { + "claude-sonnet-5": ("low", "medium", "high", "xhigh", "max"), + "gpt-5.4": ("none", "low", "medium", "high", "xhigh"), + "claude-haiku-4.5": (), +} +CONFIG_FAILED = "ACP session configuration failed: " + + +def copilot(current: str = "claude-sonnet-5") -> FakeACPAgent: + """A fake ``copilot --acp`` advertising Copilot's model catalog.""" + return ( + FakeACPAgent(supports_session_load=True) + .advertises_models(COPILOT_EFFORTS, current=current) + .knows_session("persisted-session") + .will_say("Configured") + ) + + +@asynccontextmanager +async def copilot_room(agent: FakeACPAgent, **config: Any) -> AsyncIterator[AcpSession]: + adapter = CopilotACPAdapter( + CopilotACPAdapterConfig(inject_band_tools=False, **config) + ) + async with started_acp_adapter(adapter, agent) as session: + yield session + + +class TestCopilotACPModelSelection: + @pytest.mark.asyncio + @pytest.mark.parametrize("current", ["claude-sonnet-5", "claude-haiku-4.5"]) + async def test_model_then_effort_are_set_before_the_first_prompt( + self, current: str + ) -> None: + # From Haiku, the effort select only exists once gpt-5.4 is chosen. + agent = copilot(current) + + async with copilot_room( + agent, model="gpt-5.4", reasoning_effort="high" + ) as session: + reply = await session.send("Hello") + + assert reply.texts == ["Configured"] + assert agent.config_selections() == [ + ("model", "gpt-5.4"), + ("reasoning_effort", "high"), + ] + + @pytest.mark.asyncio + async def test_a_restored_session_is_configured_too(self) -> None: + agent = copilot() + + async with copilot_room(agent, model="gpt-5.4") as session: + reply = await session.send( + "Resume", + bootstrap=True, + history=ACPClientSessionState( + room_to_session={"room-1": "persisted-session"} + ), + ) + + assert reply.texts == ["Configured"] + assert agent.config_option_requests == [ + ("persisted-session", "model", "gpt-5.4") + ] + + @pytest.mark.asyncio + @pytest.mark.parametrize( + ("config", "set_first", "error"), + [ + pytest.param( + {"model": "gpt-9"}, + [], + 'model "gpt-9" is not advertised; available: ' + "claude-sonnet-5, gpt-5.4, claude-haiku-4.5", + id="unknown-model", + ), + pytest.param( + {"model": "claude-haiku-4.5", "reasoning_effort": "high"}, + [("model", "claude-haiku-4.5")], + 'model "claude-haiku-4.5" offers no reasoning effort', + id="model-without-efforts", + ), + pytest.param( + {"model": "gpt-5.4", "reasoning_effort": "max"}, + [("model", "gpt-5.4")], + 'reasoning effort "max" is not advertised for model "gpt-5.4"; ' + "available: none, low, medium, high, xhigh", + id="effort-the-model-lacks", + ), + ], + ) + async def test_an_unadvertised_selection_fails_the_turn_naming_the_options( + self, + config: dict[str, str], + set_first: list[tuple[str, str]], + error: str, + ) -> None: + agent = copilot() + + async with copilot_room(agent, **config) as session: + reply = await session.send("Hello") + + assert reply.errors == [CONFIG_FAILED + error] + assert agent.config_selections() == set_first + assert agent.prompt_texts() == [] + + @pytest.mark.asyncio + async def test_a_live_room_switches_against_its_current_catalog(self) -> None: + # Sonnet offers "high"; after switching to Haiku the room's catalog has + # no effort select, so a stale catalog would wrongly accept "high". + agent = copilot() + + async with copilot_room(agent) as session: + await session.send("Hello") + await session.adapter.apply_model_selection( + ModelSelection(model="claude-haiku-4.5"), room_id="room-1" + ) + with pytest.raises(ACPConfigError) as rejected: + await session.adapter.apply_model_selection( + ModelSelection(reasoning_effort="high"), room_id="room-1" + ) + + assert str(rejected.value) == ( + 'model "claude-haiku-4.5" offers no reasoning effort' + ) + assert agent.config_selections() == [("model", "claude-haiku-4.5")] + + @pytest.mark.asyncio + async def test_switching_a_room_without_a_live_session_is_refused(self) -> None: + async with copilot_room(copilot()) as session: + with pytest.raises(BandConfigError, match="no live ACP session"): + await session.adapter.apply_model_selection( + ModelSelection(model="gpt-5.4"), room_id="room-1" + ) + + def test_typed_selection_and_a_resolver_are_exclusive(self) -> None: + async def resolver(request: ACPConfigRequest) -> dict[str, str]: + del request + return {} + + with pytest.raises(ValueError, match="not both"): + CopilotACPAdapter( + CopilotACPAdapterConfig( + model="gpt-5.4", resolve_session_config=resolver + ) + ) diff --git a/tests/adapters/test_copilot_acp_model_flags.py b/tests/adapters/test_copilot_acp_model_flags.py deleted file mode 100644 index 992a824ff..000000000 --- a/tests/adapters/test_copilot_acp_model_flags.py +++ /dev/null @@ -1,37 +0,0 @@ -"""Typed ``model`` / ``reasoning_effort`` flags on the Copilot ACP command.""" - -from __future__ import annotations - -import pytest - -from band.adapters.copilot_acp import CopilotACPAdapter, CopilotACPAdapterConfig - - -def test_copilot_model_flags_are_appended_once() -> None: - adapter = CopilotACPAdapter( - CopilotACPAdapterConfig(model="claude-sonnet-5", reasoning_effort="high") - ) - assert adapter._command[-4:] == [ - "--model", - "claude-sonnet-5", - "--reasoning-effort", - "high", - ] - assert adapter._command.count("--model") == 1 - - -@pytest.mark.parametrize("spliced", ["--model", "--model=gpt-6"]) -def test_copilot_model_flag_in_both_places_is_refused(spliced: str) -> None: - with pytest.raises(ValueError, match="--model is set both"): - CopilotACPAdapter( - CopilotACPAdapterConfig( - command=("copilot", "--acp", spliced), model="claude-sonnet-5" - ) - ) - - -def test_copilot_model_flags_need_stdio() -> None: - with pytest.raises(ValueError, match="need the stdio transport"): - CopilotACPAdapter( - CopilotACPAdapterConfig(host="10.0.0.5", port=8080, model="gpt-6") - ) diff --git a/tests/integrations/acp/acp_toolkit/__init__.py b/tests/integrations/acp/acp_toolkit/__init__.py index c4ef7b275..de855d2c9 100644 --- a/tests/integrations/acp/acp_toolkit/__init__.py +++ b/tests/integrations/acp/acp_toolkit/__init__.py @@ -22,7 +22,11 @@ from __future__ import annotations -from tests.integrations.acp.acp_toolkit.agent import FakeACPAgent, PromptHandler +from tests.integrations.acp.acp_toolkit.agent import ( + FakeACPAgent, + PromptHandler, + select_option, +) from tests.integrations.acp.acp_toolkit.harness import ( AcpSession, FakeSpawn, @@ -33,6 +37,7 @@ inject_acp_spawn, live_line, make_acp_connection, + started_acp_adapter, ) __all__ = [ @@ -47,4 +52,6 @@ "inject_acp_spawn", "live_line", "make_acp_connection", + "select_option", + "started_acp_adapter", ] diff --git a/tests/integrations/acp/acp_toolkit/agent.py b/tests/integrations/acp/acp_toolkit/agent.py index ca554847f..c533eae6f 100644 --- a/tests/integrations/acp/acp_toolkit/agent.py +++ b/tests/integrations/acp/acp_toolkit/agent.py @@ -2,7 +2,7 @@ from __future__ import annotations -from collections.abc import Awaitable, Callable, Sequence +from collections.abc import Awaitable, Callable, Iterable, Mapping, Sequence from typing import Any from acp import RequestError @@ -28,12 +28,17 @@ SessionCapabilities, SessionCloseCapabilities, SessionConfigOptionSelect, + SessionConfigSelectOption, SetSessionConfigOptionResponse, ToolCallUpdate, ) from mcp import ClientSession from mcp.client.streamable_http import streamable_http_client +from band.integrations.acp.model_selection import ( + MODEL_CATEGORY, + THOUGHT_LEVEL_CATEGORY, +) from band.integrations.acp.session_config import SessionConfigOption PromptHandler = Callable[["FakeACPAgent", str], Awaitable[None]] @@ -103,6 +108,45 @@ def on_config_option(self, handler: ConfigOptionHandler) -> ConfigOptionHandler: self._config_option_handler = handler return handler + def advertises_models( + self, efforts_by_model: Mapping[str, Sequence[str]], *, current: str + ) -> FakeACPAgent: + """Advertise a model select whose effort select follows the chosen model. + + Mirrors Copilot CLI: selecting a model replaces the effort select with + that model's levels (current = its first), or drops it for a model + with none. + """ + active = {"model": current} + + def catalog(model: str, effort: str | None = None) -> list[SessionConfigOption]: + options: list[SessionConfigOption] = [ + select_option("model", model, efforts_by_model, category=MODEL_CATEGORY) + ] + if efforts := efforts_by_model[model]: + options.append( + select_option( + "reasoning_effort", + effort or efforts[0], + efforts, + category=THOUGHT_LEVEL_CATEGORY, + ) + ) + return options + + @self.on_config_option + async def select( + fake: FakeACPAgent, session_id: str, option_id: str, value: str + ) -> list[SessionConfigOption]: + del fake, session_id + if option_id == "model": + active["model"] = value + return catalog(value) + return catalog(active["model"], effort=value) + + self._config_options = catalog(current) + return self + def will_say(self, text: str) -> FakeACPAgent: self._script.append(lambda a, sid: a.say(sid, text)) return self @@ -409,6 +453,10 @@ async def close_session(self, session_id: str, **kwargs: Any) -> None: del kwargs self.closed_sessions.append(session_id) + def config_selections(self) -> list[tuple[str, str]]: + """Each ``(option, value)`` the client set, in order, across sessions.""" + return [(option, value) for _sid, option, value in self.config_option_requests] + def prompt_texts(self) -> list[str]: """Each received prompt's text, one string per prompt, in arrival order.""" return [ @@ -435,3 +483,24 @@ async def prompt( for action in self._script: await action(self, session_id) return PromptResponse(stop_reason="end_turn") + + +def select_option( + option_id: str, + current_value: str, + values: Iterable[str], + *, + category: str | None = None, +) -> SessionConfigOptionSelect: + """A concise ACP select catalog entry, optionally in a spec category.""" + return SessionConfigOptionSelect( + id=option_id, + name=option_id.replace("_", " ").title(), + type="select", + category=category, + current_value=current_value, + options=[ + SessionConfigSelectOption(value=value, name=value.title()) + for value in values + ], + ) diff --git a/tests/integrations/acp/acp_toolkit/harness.py b/tests/integrations/acp/acp_toolkit/harness.py index 27a8cbd9e..186b2ed8c 100644 --- a/tests/integrations/acp/acp_toolkit/harness.py +++ b/tests/integrations/acp/acp_toolkit/harness.py @@ -182,6 +182,10 @@ def texts(self) -> list[str]: def thoughts(self) -> list[str]: return [e["content"] for e in self._events_of("thought")] + @property + def errors(self) -> list[str]: + return [e["content"] for e in self._events_of("error")] + @property def tool_calls(self) -> list[dict[str, Any]]: return [ @@ -310,6 +314,18 @@ async def acp_adapter( inject_band_tools=inject_band_tools, **adapter_kwargs, ) + async with started_acp_adapter(adapter, agent) as session: + yield session + + +@asynccontextmanager +async def started_acp_adapter( + adapter: ACPClientAdapter, agent: FakeACPAgent +) -> AsyncIterator[AcpSession]: + """Start any ``ACPClientAdapter`` (or subclass) wired to ``agent`` in process. + + Yields an :class:`AcpSession`; tears the adapter down on exit. + """ pair_in_process(adapter, agent) await adapter.on_started("Fake Agent", "in-process fake") try: diff --git a/tests/integrations/acp/test_session_config.py b/tests/integrations/acp/test_session_config.py index 3aa6f64d3..90b0b7be5 100644 --- a/tests/integrations/acp/test_session_config.py +++ b/tests/integrations/acp/test_session_config.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +from collections.abc import Sequence from types import SimpleNamespace from unittest.mock import AsyncMock @@ -14,7 +15,10 @@ SetSessionConfigOptionResponse, ) +from band.core.model_catalog import ModelCatalog, ModelChoice, ModelSelection +from band.integrations.acp.client_adapter import ACPClientAdapter from band.integrations.acp.client_types import ACPClientSessionState +from band.integrations.acp.model_selection import ACPModelOptions from band.integrations.acp.session_config import ( RESOLVER_CONFIG_OPTION_ID, ACPConfigError, @@ -22,25 +26,13 @@ SessionConfigOption, apply_session_config_selections, ) -from tests.integrations.acp.acp_toolkit import FakeACPAgent, Reply, acp_adapter - - -def select_option( - option_id: str, - current_value: str, - values: list[str], -) -> SessionConfigOptionSelect: - """A concise ACP select catalog entry for one test.""" - return SessionConfigOptionSelect( - id=option_id, - name=option_id.replace("_", " ").title(), - type="select", - current_value=current_value, - options=[ - SessionConfigSelectOption(value=value, name=value.title()) - for value in values - ], - ) +from tests.integrations.acp.acp_toolkit import ( + FakeACPAgent, + Reply, + acp_adapter, + select_option, + started_acp_adapter, +) def malformed_catalog_response() -> SimpleNamespace: @@ -239,6 +231,13 @@ async def resolve_config(request: ACPConfigRequest) -> dict[str, str]: "selected_value": "unsupported", }, ) + assert reply.errors == [ + ( + "ACP session configuration failed: ACP config value " + '"unsupported" is not advertised for option "reasoning_effort"; ' + "advertised values: medium, high." + ) + ] assert agent.prompt_texts() == [] assert agent.closed_sessions == ["fake-session-1"] @@ -457,3 +456,81 @@ async def resolve_config(request: ACPConfigRequest) -> dict[str, str]: }, ) assert agent.closed_sessions == ["persisted-session"] + + +class ThinkingIdAdapter(ACPClientAdapter): + """An ACP adapter for an agent that publishes uncategorized ``model`` and + ``thinking`` selects, the way OMP does.""" + + def locate_model_options( + self, options: Sequence[SessionConfigOption] + ) -> ACPModelOptions: + selects = { + option.id: option + for option in options + if isinstance(option, SessionConfigOptionSelect) + } + return ACPModelOptions( + model=selects.get("model"), effort=selects.get("thinking") + ) + + +class TestTypedModelSelection: + def test_the_catalog_knows_efforts_only_for_the_current_model(self) -> None: + # ACP advertises efforts for the active model alone: the others are + # unknown (None), which a host must not show as "offers none" (()). + located = ACPModelOptions( + model=select_option("model", "large", ["small", "large"]), + effort=select_option("thinking", "low", ["low", "high"]), + ) + + assert located.model_catalog() == ModelCatalog( + models=( + ModelChoice(id="small", label="Small", efforts=None), + ModelChoice( + id="large", + label="Large", + efforts=("low", "high"), + default_effort="low", + ), + ), + current_model="large", + ) + + @pytest.mark.asyncio + async def test_an_adapter_can_locate_selects_that_carry_no_category( + self, + ) -> None: + agent = FakeACPAgent( + config_options=[ + select_option("model", "small", ["small", "large"]), + select_option("thinking", "low", ["low", "high"]), + ] + ).will_say("Configured") + adapter = ThinkingIdAdapter( + command="fake-agent", + inject_band_tools=False, + model_selection=ModelSelection(model="large", reasoning_effort="high"), + ) + + async with started_acp_adapter(adapter, agent) as session: + reply = await session.send("Hello") + + assert reply.texts == ["Configured"] + assert agent.config_selections() == [("model", "large"), ("thinking", "high")] + + @pytest.mark.asyncio + async def test_a_model_is_refused_when_the_agent_advertises_no_model_option( + self, + ) -> None: + agent = FakeACPAgent().will_say("unreachable") + + async with acp_adapter( + agent, model_selection=ModelSelection(model="large") + ) as session: + reply = await session.send("Hello") + + assert reply.errors == [ + "ACP session configuration failed: ACP session advertises no model option." + ] + assert agent.prompt_texts() == [] From 20209ead56c3360c6fb5fcb0ef103bc67f7838a6 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 13:11:00 +0300 Subject: [PATCH 04/17] test(e2e): cover Copilot's typed model selection live The hosted-auth smoke gains a typed-effort cell: reasoning_effort="high" differs from the hosted model's advertised default, so the selection is really sent through session/set_config_option and the turn must still reply. A BYOK smoke proves a typed model fails the turn with a visible error, since BYOK sessions advertise no model select, rather than being silently ignored as the old --model flag was. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- AGENTS.md | 2 +- tests/e2e/baseline/README.md | 2 +- tests/e2e/baseline/settings.py | 2 +- .../smoke/adapters/test_copilot_acp.py | 86 +++++++++++++++++-- 4 files changed, 82 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b80ddab9e..d9eadcae7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -223,7 +223,7 @@ agent keys and platform URLs should stay aligned with `.env.test` / - `GOOGLE_API_KEY`: Google API key for Gemini Developer API (for Gemini/Google ADK examples) - `GOOGLE_GENAI_USE_VERTEXAI`: Set to `true` to use Vertex AI instead of Gemini Developer API - `GOOGLE_CLOUD_PROJECT`: Google Cloud project ID (required when using Vertex AI) -- `GITHUB_TOKEN`: A Copilot-entitled GitHub token. The baseline `copilot_sdk` and `copilot_acp` builders use Anthropic BYOK and never read it; the only baseline reader is the single Copilot-hosted auth smoke (`test_copilot_hosted_auth_replies`, skips when unset). Also used by Copilot-hosted examples outside the baseline; optional when a stored `copilot login` is present. +- `GITHUB_TOKEN`: A Copilot-entitled GitHub token. The baseline `copilot_sdk` and `copilot_acp` builders use Anthropic BYOK and never read it; the only baseline reader is the Copilot-hosted auth smoke (`test_copilot_hosted_auth_replies`, default and typed-effort cells, skips when unset). Also used by Copilot-hosted examples outside the baseline; optional when a stored `copilot login` is present. - `E2E_TESTS_ENABLED`: Set to `true` to enable E2E tests (default: disabled) - `E2E_LLM_MODEL`: OpenAI model for E2E tests (default: `gpt-5.4-mini`) - `E2E_ANTHROPIC_MODEL`: Anthropic model for E2E tests (default: `claude-haiku-4-5` — the baseline judge uses structured outputs, which older Haiku models do not support) diff --git a/tests/e2e/baseline/README.md b/tests/e2e/baseline/README.md index 288b80cb1..6d9ab35a1 100644 --- a/tests/e2e/baseline/README.md +++ b/tests/e2e/baseline/README.md @@ -573,7 +573,7 @@ the `dev` extra but are split out for isolation. | `core` | `dev` | anthropic, claude_sdk, agno, langgraph, pydantic_ai, copilot_sdk | provider keys (secrets); copilot_sdk self-downloads its CLI runtime and uses Anthropic BYOK without GitHub auth | | `crewai` | `dev-crewai` | crewai, crewai_flow | provider keys; isolated venv (crewai conflicts with `dev`'s deps — `pyproject.toml [tool.uv] conflicts`) | | `google` | `dev` | gemini, google_adk | provider keys; split from `core` so Google free-tier rate-limit flakiness is isolated | -| `backends` | `dev` | codex, opencode, copilot_acp | the CLI/server coding agents in one job: the `codex` CLI + login + a disposable `CODEX_CWD` (+ the codex-acp e2e), a running `opencode serve` (`OPENCODE_BASE_URL`, gating `bash` to `ask` → `E2E_OPENCODE_BASH_ASKS`), and the `copilot` CLI (`Dep.COPILOT_CLI` + `Dep.ANTHROPIC` — Anthropic BYOK; optional `GITHUB_TOKEN` for the single Copilot-hosted auth smoke) | +| `backends` | `dev` | codex, opencode, copilot_acp | the CLI/server coding agents in one job: the `codex` CLI + login + a disposable `CODEX_CWD` (+ the codex-acp e2e), a running `opencode serve` (`OPENCODE_BASE_URL`, gating `bash` to `ask` → `E2E_OPENCODE_BASH_ASKS`), and the `copilot` CLI (`Dep.COPILOT_CLI` + `Dep.ANTHROPIC` — Anthropic BYOK; optional `GITHUB_TOKEN` for the Copilot-hosted auth smoke) | | `letta` | `dev` | letta | a self-hosted Letta server (docker — `.github/scripts/setup-letta.sh`); the adapter self-hosts its Band MCP server inside pytest (see "Letta lane" below). **Linux-only** (`LINUX_ONLY_LANES`) — no Windows cells | | `parlant` | `dev-parlant` | *(none — parlant is a bespoke smoke, not a registered matrix adapter; pinned via `@lane(Lane.PARLANT)`)* | provider keys; isolated venv (parlant's `griffe`/`griffelib` transitive deps collide with pydantic_ai's — `pyproject.toml [tool.uv] conflicts`); no server setup — the smoke spins up its own in-process Parlant server | diff --git a/tests/e2e/baseline/settings.py b/tests/e2e/baseline/settings.py index 7e71e1cf2..6382c6f9b 100644 --- a/tests/e2e/baseline/settings.py +++ b/tests/e2e/baseline/settings.py @@ -174,7 +174,7 @@ class Backends(BaseSettings): omp_command: str = "" # OMP_COMMAND omp_model: str = "google/gemini-2.5-flash" # OMP_MODEL - # Copilot-hosted auth for the single non-BYOK smoke + # Copilot-hosted auth for the non-BYOK smoke # (test_copilot_acp.py::test_copilot_hosted_auth_replies); the BYOK matrix # cells never read it. The smoke skips when unset. github_token: str = "" # GITHUB_TOKEN diff --git a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py index 25ec880c2..0d6711764 100644 --- a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py +++ b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py @@ -37,6 +37,10 @@ # -- that vocabulary answers a different question ("is this tool observational, # not terminal work, for no-reply detection"), which only coincides with this one today. BAND_EVENT_TOOL_NAME = "band_send_event" +# What a BYOK turn posts for a typed model: the session advertises no model select. +BYOK_TYPED_MODEL_FAILURE = ( + "ACP session configuration failed: ACP session advertises no model option." +) @with_adapters(Adapter.COPILOT_ACP, **TOOL_AGENT) @@ -130,7 +134,12 @@ async def test_acp_band_tool_result_is_a_single_clean_payload( def hermetic_copilot_config( - settings: BaselineSettings, work_dir: Path, *, hosted: bool = False + settings: BaselineSettings, + work_dir: Path, + *, + hosted: bool = False, + model: str | None = None, + reasoning_effort: str | None = None, ) -> Any: """A per-test ``CopilotACPAdapterConfig`` with a fresh cwd + ``COPILOT_HOME``. @@ -141,8 +150,9 @@ def hermetic_copilot_config( ``hosted=True`` omits it and authenticates with ``github_token`` — the Copilot-hosted path production users run. The hosted env also pins ``COPILOT_MODEL`` (``settings.backends.copilot_hosted_model``) so this - smoke's one billed turn uses a cheap, deterministic model instead of - Copilot's ``auto`` picker. + smoke's billed turns use a cheap, deterministic model instead of + Copilot's ``auto`` picker. ``model`` / ``reasoning_effort`` set the typed + selection the adapter applies from each session's advertised catalog. """ from band.adapters.copilot_acp import ( # noqa: PLC0415 -- copilot_acp imports the acp (agent-client-protocol) extra at its own top level; not installed in every lane's venv CopilotACPAdapterConfig, @@ -160,6 +170,10 @@ def hermetic_copilot_config( } if hosted: kwargs["github_token"] = settings.backends.github_token + if model is not None: + kwargs["model"] = model + if reasoning_effort is not None: + kwargs["reasoning_effort"] = reasoning_effort if settings.backends.copilot_command.strip(): kwargs["command"] = tuple(settings.backends.copilot_command.split()) return CopilotACPAdapterConfig(**kwargs) @@ -169,19 +183,28 @@ def hermetic_copilot_config( @requires(Dep.COPILOT_CLI) @pytest.mark.timeout(extra=180) # Copilot CLI cold boot + hosted-auth handshake @pytest.mark.asyncio(loop_scope="session") +@pytest.mark.parametrize( + "reasoning_effort", + # "high" differs from the hosted model's advertised default ("medium"), + # so the typed selection is really sent through session/set_config_option. + [pytest.param(None, id="default"), pytest.param("high", id="typed-effort")], +) async def test_copilot_hosted_auth_replies( baseline_settings: BaselineSettings, resource_manager: ResourceManager, user_ops: UserOps, reply_capture: CaptureFactory, tmp_path: Any, + reasoning_effort: str | None, ) -> None: """One reply turn on Copilot-hosted auth (GITHUB_TOKEN, no BYOK). The matrix cells run Anthropic BYOK to spare the monthly Copilot-hosted - quota, but the hosted path is the one production users run — this single - cheap turn keeps it proven. Skips (not fails) without a token: hosted - auth is optional extra coverage, the BYOK cells are the lane's bar. + quota, but the hosted path is the one production users run — one cheap + turn per cell keeps it proven. Skips (not fails) without a token: hosted + auth is optional extra coverage, the BYOK cells are the lane's bar. Only + hosted sessions advertise model/effort selects, so the typed-effort cell + proves a typed selection is accepted against Copilot's live catalog. """ from band.adapters.copilot_acp import ( # noqa: PLC0415 -- copilot_acp imports the acp (agent-client-protocol) extra at its own top level; not installed in every lane's venv CopilotACPAdapter, @@ -197,7 +220,12 @@ async def test_copilot_hosted_auth_replies( ) adapter = CopilotACPAdapter( - hermetic_copilot_config(baseline_settings, tmp_path / "hosted", hosted=True) + hermetic_copilot_config( + baseline_settings, + tmp_path / "hosted", + hosted=True, + reasoning_effort=reasoning_effort, + ) ) async with ( running_agent(identity, adapter, baseline_settings), @@ -215,6 +243,50 @@ async def test_copilot_hosted_auth_replies( replies.assert_contains_any([marker]) +@lane(Lane.BACKENDS) # bespoke build exposes no framework; pin scheduling to backends +@requires(Dep.COPILOT_CLI, Dep.ANTHROPIC) +@pytest.mark.timeout(extra=180) # Copilot CLI cold boot +@pytest.mark.asyncio(loop_scope="session") +async def test_copilot_byok_turn_fails_loudly_on_a_typed_model( + baseline_settings: BaselineSettings, + resource_manager: ResourceManager, + user_ops: UserOps, + reply_capture: CaptureFactory, + tmp_path: Any, +) -> None: + """Under BYOK Copilot advertises no model select (the provider env picks the + model), so a typed ``model`` must fail the turn with a visible error rather + than be silently ignored, as the old ``--model`` flag was.""" + from band.adapters.copilot_acp import ( # noqa: PLC0415 -- copilot_acp imports the acp (agent-client-protocol) extra at its own top level; not installed in every lane's venv + CopilotACPAdapter, + ) + + identity = await resource_manager.provision_agent("copilot-byok-typed-model") + room_id = await resource_manager.provision_room( + title="e2e-copilot-byok-typed-model", participants=[identity.id] + ) + + adapter = CopilotACPAdapter( + hermetic_copilot_config( + baseline_settings, tmp_path / "byok", model=unique_marker("model") + ) + ) + async with ( + running_agent(identity, adapter, baseline_settings), + reply_capture(room_id) as capture, + ): + mid = await user_ops.send_message( + room_id, + "Reply with one short sentence.", + mention_id=identity.id, + mention_name=identity.name, + ) + await capture.wait_for_processed(mid, identity.id) + errors = await capture.errors(sender_id=identity.id) + + errors.assert_contains_any([BYOK_TYPED_MODEL_FAILURE]) + + @lane(Lane.BACKENDS) # bespoke build exposes no framework; pin scheduling to backends @requires(Dep.COPILOT_CLI, Dep.ANTHROPIC) # Deliberately no flaky marker: two clean runs so far, and a rerun here would From da711b9037822f747a0273721858b1331b3726fe Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 13:36:19 +0300 Subject: [PATCH 05/17] fix(acp): check runtime model switches against the session's live catalog The ACP runtime now owns each session's advertised config catalog. It records the catalog from session/new, session/load, every set_config_option reply, and agent-pushed config_option_update. A switch that is only partly applied, or an agent-initiated model change, therefore no longer leaves a stale adapter-side copy that silently skips the next switch. Multi-step config changes on a session hold a per-session lock, so concurrent switches apply in call order. ACPConfigError is now also a BandConfigError, so every model-selection refusal shares one family; it stays a RuntimeError for existing handlers. Also from review: - the failed-start release lives in start_adapter/release_adapter, not in both SimpleAdapter and Agent; - rejection listings and the config-failure prefix each have one source; - ModelSelection owns its setting-to-field mapping. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- src/band/agent.py | 15 +-- src/band/core/model_catalog.py | 25 +++-- src/band/core/simple_adapter.py | 18 +--- src/band/core/validation.py | 10 +- src/band/integrations/acp/client_adapter.py | 99 +++++++------------ src/band/integrations/acp/client_runtime.py | 50 +++++++++- src/band/integrations/acp/model_selection.py | 18 +--- src/band/integrations/acp/session_config.py | 17 ++-- src/band/runtime/startup.py | 39 ++++++-- tests/adapters/test_copilot_acp_adapter.py | 76 +++++++++++--- tests/catalogs.py | 10 +- tests/core/test_agent_lifecycle.py | 15 ++- tests/integrations/acp/acp_toolkit/agent.py | 72 +++++++++++--- tests/integrations/acp/test_session_config.py | 26 ++++- 14 files changed, 324 insertions(+), 166 deletions(-) diff --git a/src/band/agent.py b/src/band/agent.py index 39c541dbb..d63cee50c 100644 --- a/src/band/agent.py +++ b/src/band/agent.py @@ -14,7 +14,7 @@ from band.core.simple_adapter import SimpleAdapter from band.preprocessing.default import DefaultPreprocessor from band.runtime.platform_runtime import PlatformRuntime -from band.runtime.startup import start_adapter +from band.runtime.startup import release_adapter, start_adapter from band.runtime.types import ( AgentConfig, ContactEventConfig, @@ -278,7 +278,7 @@ async def start(self) -> None: # on_started may have acquired resources (e.g. a CLI runtime # subprocess); a failed start must release them — stop() won't # run for an agent that never started. - await self._cleanup_adapter() + await release_adapter(self._adapter) raise except BaseException: # Idempotent: a failure inside runtime.start() already released. @@ -316,7 +316,7 @@ async def stop(self, timeout: float | None = None) -> bool: # Always release adapter-wide resources (e.g. a CLI runtime # subprocess, a self-hosted MCP server, an external registration), # even when the runtime fails to stop cleanly. - await self._cleanup_adapter() + await release_adapter(self._adapter) self._started = False _running_agents.discard(self) logger.info( @@ -324,15 +324,6 @@ async def stop(self, timeout: float | None = None) -> bool: ) return graceful - async def _cleanup_adapter(self) -> None: - """Release adapter-wide resources, best-effort.""" - cleanup_all = getattr(self._adapter, "cleanup_all", None) - if cleanup_all is not None: - try: - await cleanup_all() - except Exception: - logger.exception("Adapter cleanup_all failed") - async def run( self, shutdown_timeout: float | None = DEFAULT_SHUTDOWN_TIMEOUT ) -> None: diff --git a/src/band/core/model_catalog.py b/src/band/core/model_catalog.py index b8474238c..f899f7cce 100644 --- a/src/band/core/model_catalog.py +++ b/src/band/core/model_catalog.py @@ -9,16 +9,19 @@ from __future__ import annotations -from collections.abc import Sequence from enum import StrEnum from pydantic import BaseModel, ConfigDict from band.core.exceptions import BandConfigError +from band.core.validation import listing class ModelSetting(StrEnum): - """A selectable model setting.""" + """A selectable model setting, valued as its ``ModelSelection`` field. + + Declared in apply order: the model decides which efforts exist. + """ MODEL = "model" REASONING_EFFORT = "reasoning_effort" @@ -35,6 +38,14 @@ class ModelSelection(BaseModel): model: str | None = None reasoning_effort: str | None = None + @classmethod + def only(cls, setting: ModelSetting, value: str) -> ModelSelection: + """A selection of ``value`` for ``setting`` alone.""" + return cls.model_validate({setting: value}) + + def value_of(self, setting: ModelSetting) -> str | None: + return getattr(self, setting) + @property def is_empty(self) -> bool: return self.model is None and self.reasoning_effort is None @@ -114,7 +125,7 @@ def _check_model(model_id: str, catalog: ModelCatalog) -> None: return raise ModelSelectionError( f'model "{model_id}" is not advertised; available: ' - f"{advertised_listing(catalog.model_ids)}", + f"{listing(catalog.model_ids)}", setting=ModelSetting.MODEL, value=model_id, advertised=catalog.model_ids, @@ -127,7 +138,7 @@ def _check_effort(effort: str, *, model_id: str | None, catalog: ModelCatalog) - return message = ( f'reasoning effort "{effort}" is not advertised for model "{model_id}"; ' - f"available: {advertised_listing(choice.efforts)}" + f"available: {listing(choice.efforts)}" if choice.efforts else f'model "{model_id}" offers no reasoning effort' ) @@ -139,17 +150,11 @@ def _check_effort(effort: str, *, model_id: str | None, catalog: ModelCatalog) - ) -def advertised_listing(values: Sequence[str]) -> str: - """Advertised values as shown in a rejection message.""" - return ", ".join(values) or "(none)" - - __all__ = [ "ModelCatalog", "ModelChoice", "ModelSelection", "ModelSelectionError", "ModelSetting", - "advertised_listing", "check_model_selection", ] diff --git a/src/band/core/simple_adapter.py b/src/band/core/simple_adapter.py index 2243928f6..085318a31 100644 --- a/src/band/core/simple_adapter.py +++ b/src/band/core/simple_adapter.py @@ -31,6 +31,7 @@ PlatformMessage, TurnUsage, ) +from band.core.validation import listing from band.logging_config import trace_context_scope logger = logging.getLogger(__name__) @@ -63,7 +64,7 @@ def _normalize_flags( def _describe(values: Iterable[Emit] | Iterable[Capability]) -> str: - return ", ".join(sorted(v.value for v in values)) or "(none)" + return listing(sorted(v.value for v in values)) class SimpleAdapter(ABC, Generic[H]): @@ -345,16 +346,11 @@ async def startup( model-selection check. The runtime calls this once per start; adapters override the hooks it - runs, not this. A rejected selection releases what ``on_started`` - acquired before the ``ModelSelectionError`` propagates. + runs, not this. """ self.apply_effective_features(features) await self.on_started(agent_name, agent_description) - try: - await self._validate_model_selection() - except BaseException: - await self._release_after_failed_start() - raise + await self._validate_model_selection() async def _validate_model_selection(self) -> None: selection = self.model_selection @@ -362,12 +358,6 @@ async def _validate_model_selection(self) -> None: return check_model_selection(selection, await self.list_models()) - async def _release_after_failed_start(self) -> None: - try: - await self.cleanup_all() - except Exception: - logger.exception("Adapter cleanup_all failed after a rejected start") - async def on_started(self, agent_name: str, agent_description: str) -> None: """Override for post-start setup.""" self.agent_name = agent_name diff --git a/src/band/core/validation.py b/src/band/core/validation.py index 06af1169f..3b052a499 100644 --- a/src/band/core/validation.py +++ b/src/band/core/validation.py @@ -1,9 +1,17 @@ """Shared cross-field validation rules used by tool input models and their ``AgentTools``-level backstops (some adapters hand-register tools as plain -functions and never construct/validate the input model).""" +functions and never construct/validate the input model), and the shared +wording of rejection messages.""" from __future__ import annotations +from collections.abc import Iterable + + +def listing(values: Iterable[str]) -> str: + """The accepted values as a rejection message shows them.""" + return ", ".join(values) or "(none)" + def _join_with_or(names: list[str]) -> str: if len(names) == 1: diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index 90524298e..d7eac85b1 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -75,6 +75,7 @@ ) from band.integrations.acp.room_emitter import RoomTurnEmitter from band.integrations.acp.session_config import ( + CONFIG_FAILURE_PREFIX, RESOLVER_CONFIG_OPTION_ID, ACPConfigError, ACPConfigRequest, @@ -82,7 +83,6 @@ SessionConfigResolver, SessionConfigSetter, apply_session_config_selections, - session_config_options, ) from band.integrations.acp.types import ACPToolCall from band.integrations.mcp.backends import ( @@ -328,9 +328,6 @@ def __init__( self._turn_timeout_s = turn_timeout_s self._room_to_session: dict[str, str] = {} - # Each live session's config catalog as of its last change, so a runtime - # model switch is checked against what the session now advertises. - self._room_catalogs: dict[str, tuple[SessionConfigOption, ...]] = {} self._session_initializers: dict[str, SessionInitializer] = {} self._room_tools: dict[str, AgentToolsProtocol] = {} self._background_tasks: set[asyncio.Task[None]] = set() @@ -363,19 +360,12 @@ async def apply_model_selection( async with self._session_lock: session_id = self._room_to_session.get(room_id) runtime = self._runtimes.get(room_id) - catalog = self._room_catalogs.get(room_id, ()) if session_id is None or runtime is None: raise BandConfigError(f"room {room_id} has no live ACP session to switch") - catalog = await apply_model_selection( - session_id=session_id, - config_options=catalog, - selection=selection, - locate=self.locate_model_options, - set_option=_config_setter(runtime), + await self._apply_model_selection(runtime, session_id, selection) + logger.info( + "Switched ACP session %s for room %s to %s", session_id, room_id, selection ) - async with self._session_lock: - if self._room_to_session.get(room_id) == session_id: - self._room_catalogs[room_id] = catalog def locate_model_options( self, options: Sequence[SessionConfigOption] @@ -972,16 +962,11 @@ async def _restore_session( return None try: - catalog = await self._configure_session( - runtime, - room_id, - session_id, - session_config_options(loaded), - ) + await self._configure_session(runtime, room_id, session_id) except BaseException: await self._close_fresh_session(runtime, session_id) raise - await self._record_session(room_id, session_id, catalog) + await self._record_session(room_id, session_id) logger.debug("Loaded ACP session mapping: %s -> %s", room_id, session_id) return session_id @@ -990,13 +975,8 @@ async def _create_session( ) -> str: """Create, configure, and publish a session for one room.""" async with self._fresh_session(runtime, room_id, mcp_servers) as session: - catalog = await self._configure_session( - runtime, - room_id, - session.session_id, - session_config_options(session), - ) - await self._record_session(room_id, session.session_id, catalog) + await self._configure_session(runtime, room_id, session.session_id) + await self._record_session(room_id, session.session_id) logger.info( "Created ACP session %s for room %s (mcp_servers=%d)", @@ -1029,16 +1009,10 @@ async def _fresh_session( await self._close_fresh_session(runtime, session.session_id) raise - async def _record_session( - self, - room_id: str, - session_id: str, - catalog: tuple[SessionConfigOption, ...], - ) -> None: + async def _record_session(self, room_id: str, session_id: str) -> None: """Publish a fully initialized session to its room.""" async with self._session_lock: self._room_to_session[room_id] = session_id - self._room_catalogs[room_id] = catalog async def _close_fresh_session(self, runtime: ACPRuntime, session_id: str) -> None: """Best-effort cleanup when configuration prevented first use.""" @@ -1105,32 +1079,31 @@ async def _session_mcp_servers(self, room_id: str) -> list[object]: return mcp_servers async def _configure_session( - self, - runtime: ACPRuntime, - room_id: str, - session_id: str, - config_options: tuple[SessionConfigOption, ...] | None, - ) -> tuple[SessionConfigOption, ...]: - """Apply caller-selected values; return the session's resulting catalog.""" - catalog = tuple(config_options or ()) + self, runtime: ACPRuntime, room_id: str, session_id: str + ) -> None: + """Apply caller-selected values from the session's live ACP catalog.""" selection = self.model_selection if not selection.is_empty: - return await apply_model_selection( - session_id=session_id, - config_options=catalog, - selection=selection, - locate=self.locate_model_options, - set_option=_config_setter(runtime), - ) - if self._resolve_session_config is not None: - return await self._apply_resolved_config( + await self._apply_model_selection(runtime, session_id, selection) + elif self._resolve_session_config is not None: + await self._apply_resolved_config( resolver=self._resolve_session_config, room_id=room_id, session_id=session_id, - catalog=catalog, + runtime=runtime, + ) + + async def _apply_model_selection( + self, runtime: ACPRuntime, session_id: str, selection: ModelSelection + ) -> None: + async with runtime.config_lock(session_id): + await apply_model_selection( + session_id=session_id, + config_options=runtime.config_options(session_id), + selection=selection, + locate=self.locate_model_options, set_option=_config_setter(runtime), ) - return catalog async def _apply_resolved_config( self, @@ -1138,10 +1111,10 @@ async def _apply_resolved_config( resolver: SessionConfigResolver, room_id: str, session_id: str, - catalog: tuple[SessionConfigOption, ...], - set_option: SessionConfigSetter, - ) -> tuple[SessionConfigOption, ...]: + runtime: ACPRuntime, + ) -> None: """Apply what the caller's ``resolve_session_config`` selects.""" + catalog = runtime.config_options(session_id) try: selections = await resolver( ACPConfigRequest( @@ -1160,13 +1133,13 @@ async def _apply_resolved_config( message=f"ACP session configuration resolver failed: {error}", ) from error if selections is None: - return catalog + return - return await apply_session_config_selections( + await apply_session_config_selections( session_id=session_id, config_options=catalog, selections=selections, - set_option=set_option, + set_option=_config_setter(runtime), ) async def _report_config_error( @@ -1174,11 +1147,11 @@ async def _report_config_error( tools: AgentToolsProtocol, error: ACPConfigError, ) -> None: - logger.warning("ACP session configuration failed: %s", error) + logger.warning("%s%s", CONFIG_FAILURE_PREFIX, error) await tools.send_failure( AgentFailure( _PROVIDER, - f"ACP session configuration failed: {error}", + f"{CONFIG_FAILURE_PREFIX}{error}", "acp_session_config", { "session_id": error.session_id, @@ -1265,7 +1238,6 @@ def _build_prompt_text( async def on_cleanup(self, room_id: str) -> None: async with self._session_lock: session_id = self._room_to_session.pop(room_id, None) - self._room_catalogs.pop(room_id, None) initializer = self._session_initializers.pop(room_id, None) self._room_tools.pop(room_id, None) if session_id: @@ -1304,7 +1276,6 @@ async def cleanup_all(self, *, final: bool = True) -> None: initializers = tuple(self._session_initializers.values()) self._session_initializers.clear() self._room_to_session.clear() - self._room_catalogs.clear() self._room_tools.clear() self._bootstrapped_sessions.clear() runtimes = list(self._runtimes.values()) diff --git a/src/band/integrations/acp/client_runtime.py b/src/band/integrations/acp/client_runtime.py index 8db65feed..9a62e7ce1 100644 --- a/src/band/integrations/acp/client_runtime.py +++ b/src/band/integrations/acp/client_runtime.py @@ -5,7 +5,7 @@ import asyncio import json import logging -from collections.abc import AsyncIterator, Awaitable, Callable, Mapping +from collections.abc import AsyncIterator, Awaitable, Callable, Mapping, Sequence from contextlib import AbstractAsyncContextManager, asynccontextmanager from typing import Any, Literal, Protocol, cast @@ -14,6 +14,7 @@ from acp.interfaces import Client from acp.schema import ( ClientCapabilities, + ConfigOptionUpdate, DeclineElicitationResponse, LoadSessionResponse, NewSessionResponse, @@ -24,6 +25,10 @@ ACPClientProfile, NoopACPClientProfile, ) +from band.integrations.acp.session_config import ( + SessionConfigOption, + session_config_options, +) from band.integrations.acp.types import ( ACPToolCall, ACPToolResult, @@ -352,6 +357,9 @@ def __init__( # Never popped in reset_session: replacing a lock a straggler task still # holds would let two tasks into the session's critical section. self._session_locks: dict[str, asyncio.Lock] = {} + # Each session's latest advertised config catalog; like the locks, + # it outlives reset_session, which runs every turn. + self._config_options: dict[str, tuple[SessionConfigOption, ...]] = {} def _session_lock(self, session_id: str) -> asyncio.Lock: return self._session_locks.setdefault(session_id, asyncio.Lock()) @@ -371,6 +379,9 @@ async def session_update( self, session_id: str, update: object, **kwargs: object ) -> None: del kwargs + if isinstance(update, ConfigOptionUpdate): + self.record_config_options(session_id, update.config_options) + return async with self._session_lock(session_id): chunk = self._chunk_from_update(update) if chunk is not None: @@ -699,6 +710,14 @@ def set_permission_handler( else: self._permission_handlers[session_id] = handler + def record_config_options( + self, session_id: str, options: Sequence[SessionConfigOption] + ) -> None: + self._config_options[session_id] = tuple(options) + + def config_options(self, session_id: str) -> tuple[SessionConfigOption, ...]: + return self._config_options.get(session_id, ()) + def reset_session(self, session_id: str) -> None: self._session_chunks.pop(session_id, None) self._permission_handlers.pop(session_id, None) @@ -827,6 +846,7 @@ def __init__( self._agent_mcp_transport: MCPTransportKind = "http" self._agent_supports_session_load = False self._agent_supports_session_close = False + self._config_locks: dict[str, asyncio.Lock] = {} async def start(self, *, respawn: bool = False) -> None: """Spawn or respawn the ACP agent subprocess.""" @@ -906,10 +926,12 @@ async def create_session_response( self, *, cwd: str, mcp_servers: list[object] ) -> NewSessionResponse: conn = await self.ensure_connection(can_respawn=False) - return cast( + response = cast( NewSessionResponse, await conn.new_session(cwd=cwd, mcp_servers=mcp_servers), ) + self._record_config_options(response.session_id, response) + return response async def load_session( self, @@ -974,6 +996,7 @@ async def load_session_response( error, ) return None + self._record_config_options(session_id, response) return response async def set_config_option( @@ -985,11 +1008,32 @@ async def set_config_option( ) -> SetSessionConfigOptionResponse | None: """Set one advertised select option and return the refreshed catalog.""" conn = await self.ensure_connection(can_respawn=False) - return await conn.set_config_option( + response = await conn.set_config_option( session_id=session_id, config_id=config_id, value=value, ) + self._record_config_options(session_id, response) + return response + + def config_options(self, session_id: str) -> tuple[SessionConfigOption, ...]: + """The session's live catalog: its setup response, then every change.""" + if self._client is None: + return () + return self._client.config_options(session_id) + + def config_lock(self, session_id: str) -> asyncio.Lock: + """Held across a multi-step config change on ``session_id``. + + Each step is checked against the catalog the previous one returned, so + an interleaved change would validate against a model no longer current. + """ + return self._config_locks.setdefault(session_id, asyncio.Lock()) + + def _record_config_options(self, session_id: str, response: object) -> None: + options = session_config_options(response) + if options is not None and self._client is not None: + self._client.record_config_options(session_id, options) async def close_session(self, session_id: str) -> None: """Close a session when the agent advertised lifecycle support.""" diff --git a/src/band/integrations/acp/model_selection.py b/src/band/integrations/acp/model_selection.py index c2e7a8549..31797126b 100644 --- a/src/band/integrations/acp/model_selection.py +++ b/src/band/integrations/acp/model_selection.py @@ -91,10 +91,8 @@ async def apply_model_selection( Returns the catalog after the last change. """ catalog = tuple(config_options) - for setting, value in ( - (ModelSetting.MODEL, selection.model), - (ModelSetting.REASONING_EFFORT, selection.reasoning_effort), - ): + for setting in ModelSetting: + value = selection.value_of(setting) if value is None: continue catalog = await _apply_setting( @@ -121,7 +119,9 @@ async def _apply_setting( option = located.option_for(setting) option_id = option.id if option is not None else setting try: - check_model_selection(_selection_of(setting, value), located.model_catalog()) + check_model_selection( + ModelSelection.only(setting, value), located.model_catalog() + ) except ModelSelectionError as error: raise ACPConfigError( session_id=session_id, @@ -144,14 +144,6 @@ async def _apply_setting( ) -def _selection_of(setting: ModelSetting, value: str) -> ModelSelection: - match setting: - case ModelSetting.MODEL: - return ModelSelection(model=value) - case ModelSetting.REASONING_EFFORT: - return ModelSelection(reasoning_effort=value) - - def _select_in_category( options: Sequence[SessionConfigOption], category: str ) -> SessionConfigOptionSelect | None: diff --git a/src/band/integrations/acp/session_config.py b/src/band/integrations/acp/session_config.py index 013719d17..0f8740be0 100644 --- a/src/band/integrations/acp/session_config.py +++ b/src/band/integrations/acp/session_config.py @@ -15,7 +15,8 @@ SetSessionConfigOptionResponse, ) -from band.core.model_catalog import advertised_listing +from band.core.exceptions import BandConfigError +from band.core.validation import listing SessionConfigOption: TypeAlias = SessionConfigOptionSelect | SessionConfigOptionBoolean SessionConfigSelections: TypeAlias = Mapping[str, str | None] @@ -28,6 +29,8 @@ SESSION_CONFIG_TIMEOUT_SECONDS = 10.0 RESOLVER_CONFIG_OPTION_ID = "resolver" +# How the room reads a failure to apply a session configuration. +CONFIG_FAILURE_PREFIX = "ACP session configuration failed: " @dataclass(frozen=True) @@ -39,7 +42,9 @@ class ACPConfigRequest: config_options: tuple[SessionConfigOption, ...] -class ACPConfigError(RuntimeError): +# RuntimeError stays a base so handlers written against earlier releases +# still catch it. +class ACPConfigError(BandConfigError, RuntimeError): """A requested ACP session configuration could not be applied.""" def __init__( @@ -125,8 +130,8 @@ async def apply_session_config_selections( option_id=option_id, selected_value=selected_value, message=( - f'ACP config option "{option_id}" is not available; advertised ' - f"options: {advertised_listing(select_ids)}." + f'ACP session offers no config option "{option_id}"; ' + f"available: {listing(select_ids)}." ), ) @@ -141,8 +146,8 @@ async def apply_session_config_selections( selected_value=selected_value, message=( f'ACP config value "{selected_value}" is not advertised ' - f'for option "{option_id}"; advertised values: ' - f"{advertised_listing(available_values)}." + f'for option "{option_id}"; available: ' + f"{listing(available_values)}." ), ) diff --git a/src/band/runtime/startup.py b/src/band/runtime/startup.py index 1a56ece0b..a66185146 100644 --- a/src/band/runtime/startup.py +++ b/src/band/runtime/startup.py @@ -2,10 +2,14 @@ from __future__ import annotations +import logging + from band.core.protocols import FrameworkAdapter from band.core.simple_adapter import SimpleAdapter from band.runtime.capabilities import prune_unsupported +logger = logging.getLogger(__name__) + async def start_adapter( adapter: FrameworkAdapter | SimpleAdapter, @@ -17,16 +21,33 @@ async def start_adapter( """Start ``adapter`` for an agent whose deployment serves ``feature_flags``. A bare ``FrameworkAdapter`` has no ``SUPPORTED_CAPABILITIES`` to negotiate - and no model selection to check, so it only receives ``on_started``. + and no model selection to check, so it only receives ``on_started``. A + failed start releases what the adapter acquired before re-raising. """ - if isinstance(adapter, SimpleAdapter): - await adapter.startup( - agent_name, - agent_description, - features=prune_unsupported(adapter.features, feature_flags), - ) + try: + if isinstance(adapter, SimpleAdapter): + await adapter.startup( + agent_name, + agent_description, + features=prune_unsupported(adapter.features, feature_flags), + ) + else: + await adapter.on_started(agent_name, agent_description) + except BaseException: + await release_adapter(adapter) + raise + + +async def release_adapter(adapter: FrameworkAdapter | SimpleAdapter) -> None: + """Best-effort ``cleanup_all``: a failure is logged, never raised, so it + cannot replace the error that made the caller release.""" + cleanup_all = getattr(adapter, "cleanup_all", None) + if cleanup_all is None: return - await adapter.on_started(agent_name, agent_description) + try: + await cleanup_all() + except Exception: + logger.exception("Adapter cleanup_all failed") -__all__ = ["start_adapter"] +__all__ = ["release_adapter", "start_adapter"] diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index 84e70ad6e..8ab8a1e12 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -9,6 +9,7 @@ from __future__ import annotations +import asyncio import logging from collections.abc import AsyncIterator from contextlib import asynccontextmanager @@ -28,7 +29,10 @@ from band.integrations.acp.client_adapter import ACPClientAdapter from band.integrations.acp.client_profiles import NoopACPClientProfile from band.integrations.acp.client_types import ACPClientSessionState -from band.integrations.acp.session_config import ACPConfigError, ACPConfigRequest +from band.integrations.acp.session_config import ( + CONFIG_FAILURE_PREFIX, + ACPConfigRequest, +) from tests.integrations.acp.acp_toolkit.agent import FakeACPAgent from tests.integrations.acp.acp_toolkit.harness import AcpSession, started_acp_adapter @@ -175,7 +179,6 @@ def test_tcp_with_auth_warns_before_rejection( "gpt-5.4": ("none", "low", "medium", "high", "xhigh"), "claude-haiku-4.5": (), } -CONFIG_FAILED = "ACP session configuration failed: " def copilot(current: str = "claude-sonnet-5") -> FakeACPAgent: @@ -197,6 +200,13 @@ async def copilot_room(agent: FakeACPAgent, **config: Any) -> AsyncIterator[AcpS yield session +async def switch_room(session: AcpSession, **selection: str) -> None: + """Switch the live session of the room ``session.send`` talks to.""" + await session.adapter.apply_model_selection( + ModelSelection(**selection), room_id="room-1" + ) + + class TestCopilotACPModelSelection: @pytest.mark.asyncio @pytest.mark.parametrize("current", ["claude-sonnet-5", "claude-haiku-4.5"]) @@ -272,7 +282,7 @@ async def test_an_unadvertised_selection_fails_the_turn_naming_the_options( async with copilot_room(agent, **config) as session: reply = await session.send("Hello") - assert reply.errors == [CONFIG_FAILED + error] + assert reply.errors == [CONFIG_FAILURE_PREFIX + error] assert agent.config_selections() == set_first assert agent.prompt_texts() == [] @@ -284,26 +294,66 @@ async def test_a_live_room_switches_against_its_current_catalog(self) -> None: async with copilot_room(agent) as session: await session.send("Hello") - await session.adapter.apply_model_selection( - ModelSelection(model="claude-haiku-4.5"), room_id="room-1" - ) - with pytest.raises(ACPConfigError) as rejected: - await session.adapter.apply_model_selection( - ModelSelection(reasoning_effort="high"), room_id="room-1" - ) + await switch_room(session, model="claude-haiku-4.5") + with pytest.raises(BandConfigError) as rejected: + await switch_room(session, reasoning_effort="high") assert str(rejected.value) == ( 'model "claude-haiku-4.5" offers no reasoning effort' ) assert agent.config_selections() == [("model", "claude-haiku-4.5")] + @pytest.mark.asyncio + async def test_a_partly_applied_switch_leaves_the_room_on_what_the_agent_runs( + self, + ) -> None: + # The model lands before the effort is refused, so switching back + # must reach the agent rather than match the pre-switch catalog. + agent = copilot() + + async with copilot_room(agent) as session: + await session.send("Hello") + with pytest.raises(BandConfigError): + await switch_room(session, model="gpt-5.4", reasoning_effort="max") + await switch_room(session, model="claude-sonnet-5") + + assert agent.current_value("model") == "claude-sonnet-5" + + @pytest.mark.asyncio + async def test_a_switch_follows_a_model_the_agent_chose_itself(self) -> None: + agent = copilot() + + @agent.on_prompt + async def pick_gpt(fake: FakeACPAgent, session_id: str) -> None: + await fake.selects_on_its_own(session_id, "model", "gpt-5.4") + await fake.say(session_id, "Switched") + + async with copilot_room(agent) as session: + await session.send("Use gpt") + await switch_room(session, model="claude-sonnet-5") + + assert agent.current_value("model") == "claude-sonnet-5" + + @pytest.mark.asyncio + async def test_concurrent_switches_on_a_room_apply_in_call_order(self) -> None: + agent = copilot() + + async with copilot_room(agent) as session: + await session.send("Hello") + gate = agent.holds_config_replies() + first = asyncio.create_task(switch_room(session, model="gpt-5.4")) + await gate.received.wait() + second = asyncio.create_task(switch_room(session, model="claude-sonnet-5")) + gate.release.set() + await asyncio.gather(first, second) + + assert agent.current_value("model") == "claude-sonnet-5" + @pytest.mark.asyncio async def test_switching_a_room_without_a_live_session_is_refused(self) -> None: async with copilot_room(copilot()) as session: with pytest.raises(BandConfigError, match="no live ACP session"): - await session.adapter.apply_model_selection( - ModelSelection(model="gpt-5.4"), room_id="room-1" - ) + await switch_room(session, model="gpt-5.4") def test_typed_selection_and_a_resolver_are_exclusive(self) -> None: async def resolver(request: ACPConfigRequest) -> dict[str, str]: diff --git a/tests/catalogs.py b/tests/catalogs.py index 6ca79eb10..d25abc4db 100644 --- a/tests/catalogs.py +++ b/tests/catalogs.py @@ -13,20 +13,20 @@ class CatalogAdapter(SimpleAdapter[None]): - """Lists ``catalog``, but only once ``on_started`` has run.""" + """Lists ``SONNET_CATALOG``, but only once ``on_started`` has run.""" def __init__( - self, selection: ModelSelection, catalog: ModelCatalog = SONNET_CATALOG + self, selection: ModelSelection, *, cleanup_fails: bool = False ) -> None: super().__init__() self._selection = selection - self._catalog = catalog + self._cleanup_fails = cleanup_fails self._started_catalog: ModelCatalog | None = None self.cleaned_up = False async def on_started(self, agent_name: str, agent_description: str) -> None: await super().on_started(agent_name, agent_description) - self._started_catalog = self._catalog + self._started_catalog = SONNET_CATALOG @property def model_selection(self) -> ModelSelection: @@ -41,3 +41,5 @@ async def on_message(self, *args: object, **kwargs: object) -> None: async def cleanup_all(self) -> None: self.cleaned_up = True + if self._cleanup_fails: + raise RuntimeError("cleanup failed") diff --git a/tests/core/test_agent_lifecycle.py b/tests/core/test_agent_lifecycle.py index 7497ce9ff..7971191b4 100644 --- a/tests/core/test_agent_lifecycle.py +++ b/tests/core/test_agent_lifecycle.py @@ -7,7 +7,7 @@ from __future__ import annotations -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, MagicMock import pytest @@ -20,6 +20,8 @@ def make_agent(adapter: object, *, started: bool = True) -> Agent: runtime = AsyncMock() runtime.stop.return_value = True runtime.feature_flags = None + runtime.claim_single_instance = MagicMock() + runtime.release_single_instance = MagicMock() agent = Agent(runtime=runtime, adapter=adapter) # type: ignore[arg-type] agent._started = started return agent @@ -65,6 +67,17 @@ async def test_a_rejected_selection_fails_start_before_connecting(self) -> None: agent._runtime.start.assert_not_awaited() assert adapter.cleaned_up + @pytest.mark.asyncio + async def test_a_failing_release_does_not_mask_the_rejection( + self, caplog: pytest.LogCaptureFixture + ) -> None: + adapter = CatalogAdapter(ModelSelection(model="gpt-9"), cleanup_fails=True) + + with pytest.raises(ModelSelectionError, match='model "gpt-9"'): + await make_agent(adapter, started=False).start() + + assert "Adapter cleanup_all failed" in caplog.text + class TestStopCleansUpAdapter: @pytest.mark.asyncio diff --git a/tests/integrations/acp/acp_toolkit/agent.py b/tests/integrations/acp/acp_toolkit/agent.py index c533eae6f..6ec5c6553 100644 --- a/tests/integrations/acp/acp_toolkit/agent.py +++ b/tests/integrations/acp/acp_toolkit/agent.py @@ -2,7 +2,9 @@ from __future__ import annotations +import asyncio from collections.abc import Awaitable, Callable, Iterable, Mapping, Sequence +from dataclasses import dataclass, field from typing import Any from acp import RequestError @@ -19,6 +21,7 @@ ) from acp.schema import ( AgentCapabilities, + ConfigOptionUpdate, InitializeResponse, LoadSessionResponse, McpCapabilities, @@ -47,6 +50,14 @@ ] +@dataclass +class ReplyGate: + """Holds ``set_config_option`` replies: each is applied, then waits.""" + + received: asyncio.Event = field(default_factory=asyncio.Event) + release: asyncio.Event = field(default_factory=asyncio.Event) + + class FakeACPAgent: """A scripted, in-process ACP agent (the peer an ACP client drives). @@ -81,6 +92,7 @@ def __init__( self._custom: PromptHandler | None = None self._config_options = list(config_options) self._config_option_handler: ConfigOptionHandler | None = None + self._reply_gate: ReplyGate | None = None # Observability for assertions: self.sessions: list[dict[str, Any]] = [] self._mcp_servers_by_session: dict[str, list[Any]] = {} @@ -108,6 +120,11 @@ def on_config_option(self, handler: ConfigOptionHandler) -> ConfigOptionHandler: self._config_option_handler = handler return handler + def holds_config_replies(self) -> ReplyGate: + """Apply each ``set_config_option`` at once but reply only on release.""" + self._reply_gate = ReplyGate() + return self._reply_gate + def advertises_models( self, efforts_by_model: Mapping[str, Sequence[str]], *, current: str ) -> FakeACPAgent: @@ -328,6 +345,20 @@ def _conn_for(self, session_id: str) -> AgentSideConnection: async def emit(self, session_id: str, update: Any) -> None: await self._conn_for(session_id).session_update(session_id, update) + async def selects_on_its_own( + self, session_id: str, option_id: str, value: str + ) -> None: + """Change an option agent-side and push ``config_option_update``, as + Copilot's in-session ``/model`` does.""" + self._config_options = await self._select(session_id, option_id, value) + await self.emit( + session_id, + ConfigOptionUpdate( + session_update="config_option_update", + config_options=self._config_options, + ), + ) + async def ask_permission( self, session_id: str, tool_call: Any, options: list[Any] ) -> Any: @@ -433,20 +464,37 @@ async def set_config_option( """Apply one advertised select option and return the full live catalog.""" del kwargs self.config_option_requests.append((session_id, config_id, value)) + self._config_options = await self._select(session_id, config_id, value) + if self._reply_gate is not None: + self._reply_gate.received.set() + await self._reply_gate.release.wait() + return SetSessionConfigOptionResponse(config_options=self._config_options) + + async def _select( + self, session_id: str, option_id: str, value: str + ) -> list[SessionConfigOption]: if self._config_option_handler is not None: - self._config_options = list( - await self._config_option_handler(self, session_id, config_id, value) + return list( + await self._config_option_handler(self, session_id, option_id, value) ) - return SetSessionConfigOptionResponse(config_options=self._config_options) - - updated: list[SessionConfigOption] = [] - for option in self._config_options: - if option.id == config_id and isinstance(option, SessionConfigOptionSelect): - updated.append(option.model_copy(update={"current_value": value})) - else: - updated.append(option) - self._config_options = updated - return SetSessionConfigOptionResponse(config_options=self._config_options) + return [ + option.model_copy(update={"current_value": value}) + if option.id == option_id and isinstance(option, SessionConfigOptionSelect) + else option + for option in self._config_options + ] + + def current_value(self, option_id: str) -> str | None: + """The value the agent currently has selected for ``option_id``.""" + return next( + ( + option.current_value + for option in self._config_options + if option.id == option_id + and isinstance(option, SessionConfigOptionSelect) + ), + None, + ) async def close_session(self, session_id: str, **kwargs: Any) -> None: """Record that the client closed a session before prompting it.""" diff --git a/tests/integrations/acp/test_session_config.py b/tests/integrations/acp/test_session_config.py index 90b0b7be5..3954c6a1e 100644 --- a/tests/integrations/acp/test_session_config.py +++ b/tests/integrations/acp/test_session_config.py @@ -20,6 +20,7 @@ from band.integrations.acp.client_types import ACPClientSessionState from band.integrations.acp.model_selection import ACPModelOptions from band.integrations.acp.session_config import ( + CONFIG_FAILURE_PREFIX, RESOLVER_CONFIG_OPTION_ID, ACPConfigError, ACPConfigRequest, @@ -128,7 +129,7 @@ async def test_fails_when_a_prior_selection_removes_a_later_effort_option( ) ) - with pytest.raises(ACPConfigError, match='"reasoning_effort" is not available'): + with pytest.raises(ACPConfigError) as rejected: await apply_session_config_selections( session_id="session-1", config_options=[model, effort], @@ -136,8 +137,25 @@ async def test_fails_when_a_prior_selection_removes_a_later_effort_option( set_option=set_option, ) + assert str(rejected.value) == ( + 'ACP session offers no config option "reasoning_effort"; available: model.' + ) set_option.assert_awaited_once_with("session-1", "model", "auto") + @pytest.mark.asyncio + async def test_an_option_on_an_empty_catalog_is_refused_naming_none(self) -> None: + with pytest.raises(ACPConfigError) as rejected: + await apply_session_config_selections( + session_id="session-1", + config_options=[], + selections={"model": "auto"}, + set_option=AsyncMock(), + ) + + assert str(rejected.value) == ( + 'ACP session offers no config option "model"; available: (none).' + ) + @pytest.mark.asyncio async def test_rejects_a_malformed_refreshed_catalog(self) -> None: effort = select_option("reasoning_effort", "medium", ["medium", "high"]) @@ -233,9 +251,9 @@ async def resolve_config(request: ACPConfigRequest) -> dict[str, str]: ) assert reply.errors == [ ( - "ACP session configuration failed: ACP config value " + f"{CONFIG_FAILURE_PREFIX}ACP config value " '"unsupported" is not advertised for option "reasoning_effort"; ' - "advertised values: medium, high." + "available: medium, high." ) ] assert agent.prompt_texts() == [] @@ -531,6 +549,6 @@ async def test_a_model_is_refused_when_the_agent_advertises_no_model_option( reply = await session.send("Hello") assert reply.errors == [ - "ACP session configuration failed: ACP session advertises no model option." + f"{CONFIG_FAILURE_PREFIX}ACP session advertises no model option." ] assert agent.prompt_texts() == [] From 40767bb6cc1d8bb88d7c8b2a5785cb807c3be954 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 13:36:19 +0300 Subject: [PATCH 06/17] test(e2e): refuse an unadvertised Copilot selection under BYOK and hosted auth The BYOK typed-model smoke becomes one parametrized test with a hosted cell: an effort the hosted model does not advertise fails the turn before any prompt, naming what the live catalog offers. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- AGENTS.md | 2 +- tests/e2e/baseline/settings.py | 5 +- .../smoke/adapters/test_copilot_acp.py | 48 ++++++++++++++----- 3 files changed, 39 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d9eadcae7..29eb45537 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -223,7 +223,7 @@ agent keys and platform URLs should stay aligned with `.env.test` / - `GOOGLE_API_KEY`: Google API key for Gemini Developer API (for Gemini/Google ADK examples) - `GOOGLE_GENAI_USE_VERTEXAI`: Set to `true` to use Vertex AI instead of Gemini Developer API - `GOOGLE_CLOUD_PROJECT`: Google Cloud project ID (required when using Vertex AI) -- `GITHUB_TOKEN`: A Copilot-entitled GitHub token. The baseline `copilot_sdk` and `copilot_acp` builders use Anthropic BYOK and never read it; the only baseline reader is the Copilot-hosted auth smoke (`test_copilot_hosted_auth_replies`, default and typed-effort cells, skips when unset). Also used by Copilot-hosted examples outside the baseline; optional when a stored `copilot login` is present. +- `GITHUB_TOKEN`: A Copilot-entitled GitHub token. The baseline `copilot_sdk` and `copilot_acp` builders use Anthropic BYOK and never read it; the only baseline readers are the Copilot-hosted smokes (`test_copilot_hosted_auth_replies` and the `hosted-effort` cell of `test_copilot_turn_fails_loudly_on_an_unadvertised_selection`, which skip when unset). Also used by Copilot-hosted examples outside the baseline; optional when a stored `copilot login` is present. - `E2E_TESTS_ENABLED`: Set to `true` to enable E2E tests (default: disabled) - `E2E_LLM_MODEL`: OpenAI model for E2E tests (default: `gpt-5.4-mini`) - `E2E_ANTHROPIC_MODEL`: Anthropic model for E2E tests (default: `claude-haiku-4-5` — the baseline judge uses structured outputs, which older Haiku models do not support) diff --git a/tests/e2e/baseline/settings.py b/tests/e2e/baseline/settings.py index 6382c6f9b..5236fa834 100644 --- a/tests/e2e/baseline/settings.py +++ b/tests/e2e/baseline/settings.py @@ -174,9 +174,8 @@ class Backends(BaseSettings): omp_command: str = "" # OMP_COMMAND omp_model: str = "google/gemini-2.5-flash" # OMP_MODEL - # Copilot-hosted auth for the non-BYOK smoke - # (test_copilot_acp.py::test_copilot_hosted_auth_replies); the BYOK matrix - # cells never read it. The smoke skips when unset. + # Copilot-hosted auth for the non-BYOK smokes in test_copilot_acp.py; the + # BYOK matrix cells never read it. Those smokes skip when unset. github_token: str = "" # GITHUB_TOKEN # Pins that same smoke's model instead of leaving it to Copilot's `auto` # picker, which can land on an expensive reasoning-tier model (observed: diff --git a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py index 0d6711764..0db253f5c 100644 --- a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py +++ b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py @@ -37,10 +37,9 @@ # -- that vocabulary answers a different question ("is this tool observational, # not terminal work, for no-reply detection"), which only coincides with this one today. BAND_EVENT_TOOL_NAME = "band_send_event" -# What a BYOK turn posts for a typed model: the session advertises no model select. -BYOK_TYPED_MODEL_FAILURE = ( - "ACP session configuration failed: ACP session advertises no model option." -) +# Room text for a refused typed selection. Literal: importing the SDK's +# constant would need the acp extra at collection time. +CONFIG_FAILURE_PREFIX = "ACP session configuration failed: " @with_adapters(Adapter.COPILOT_ACP, **TOOL_AGENT) @@ -247,28 +246,53 @@ async def test_copilot_hosted_auth_replies( @requires(Dep.COPILOT_CLI, Dep.ANTHROPIC) @pytest.mark.timeout(extra=180) # Copilot CLI cold boot @pytest.mark.asyncio(loop_scope="session") -async def test_copilot_byok_turn_fails_loudly_on_a_typed_model( +@pytest.mark.parametrize( + ("hosted", "selection", "failure"), + [ + # Under BYOK the provider env picks the model; no model select exists. + pytest.param( + False, + {"model": "not-a-model"}, + "ACP session advertises no model option.", + id="byok-model", + ), + # Hosted sessions advertise per-model efforts; the refusal proves the + # selection was checked against Copilot's live catalog. + pytest.param( + True, + {"reasoning_effort": "not-an-effort"}, + 'reasoning effort "not-an-effort" is not advertised for model', + id="hosted-effort", + ), + ], +) +async def test_copilot_turn_fails_loudly_on_an_unadvertised_selection( baseline_settings: BaselineSettings, resource_manager: ResourceManager, user_ops: UserOps, reply_capture: CaptureFactory, tmp_path: Any, + hosted: bool, + selection: dict[str, str], + failure: str, ) -> None: - """Under BYOK Copilot advertises no model select (the provider env picks the - model), so a typed ``model`` must fail the turn with a visible error rather - than be silently ignored, as the old ``--model`` flag was.""" + """A typed selection the session does not advertise fails the turn with a + visible error naming what it offers, rather than being ignored.""" from band.adapters.copilot_acp import ( # noqa: PLC0415 -- copilot_acp imports the acp (agent-client-protocol) extra at its own top level; not installed in every lane's venv CopilotACPAdapter, ) - identity = await resource_manager.provision_agent("copilot-byok-typed-model") + if hosted and not baseline_settings.backends.github_token: + pytest.skip("GITHUB_TOKEN unset — the Copilot-hosted auth smoke needs one") + + identity = await resource_manager.provision_agent("copilot-unadvertised") room_id = await resource_manager.provision_room( - title="e2e-copilot-byok-typed-model", participants=[identity.id] + title="e2e-copilot-unadvertised", participants=[identity.id] ) adapter = CopilotACPAdapter( hermetic_copilot_config( - baseline_settings, tmp_path / "byok", model=unique_marker("model") + baseline_settings, tmp_path / "copilot", hosted=hosted, **selection ) ) async with ( @@ -284,7 +308,7 @@ async def test_copilot_byok_turn_fails_loudly_on_a_typed_model( await capture.wait_for_processed(mid, identity.id) errors = await capture.errors(sender_id=identity.id) - errors.assert_contains_any([BYOK_TYPED_MODEL_FAILURE]) + errors.assert_contains_any([CONFIG_FAILURE_PREFIX + failure]) @lane(Lane.BACKENDS) # bespoke build exposes no framework; pin scheduling to backends From 3ab4b5b2c6e666e3dbcfc21fc47a366683b03f19 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 13:43:51 +0300 Subject: [PATCH 07/17] test(acp): model the fake Copilot on its probed catalog behavior A live probe of Copilot CLI 1.0.89 (hosted) showed the fake was wrong in three ways: the effort defaults to "medium" rather than a model's first level, a still-offered effort carries over a model switch, and every change is also pushed as config_option_update. The fake now does all three. A plain spec agent that only returns catalogs in its set replies gets its own switch-back test, since those replies are its only record of the change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- tests/integrations/acp/acp_toolkit/agent.py | 45 +++++++++++++------ tests/integrations/acp/test_session_config.py | 21 +++++++++ 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/tests/integrations/acp/acp_toolkit/agent.py b/tests/integrations/acp/acp_toolkit/agent.py index 6ec5c6553..e96500efd 100644 --- a/tests/integrations/acp/acp_toolkit/agent.py +++ b/tests/integrations/acp/acp_toolkit/agent.py @@ -126,25 +126,37 @@ def holds_config_replies(self) -> ReplyGate: return self._reply_gate def advertises_models( - self, efforts_by_model: Mapping[str, Sequence[str]], *, current: str + self, + efforts_by_model: Mapping[str, Sequence[str]], + *, + current: str, + default_effort: str = "medium", ) -> FakeACPAgent: """Advertise a model select whose effort select follows the chosen model. - Mirrors Copilot CLI: selecting a model replaces the effort select with - that model's levels (current = its first), or drops it for a model - with none. + Mirrors Copilot CLI 1.0.89 (probed live): selecting a model replaces + the effort select with that model's levels, or drops it for a model + with none; the current effort carries over when the new model offers + it, else falls back to ``default_effort``; an unoffered effort is + silently ignored; and every change is also pushed as a + ``config_option_update``. """ - active = {"model": current} + active = {"model": current, "effort": default_effort} - def catalog(model: str, effort: str | None = None) -> list[SessionConfigOption]: + def catalog() -> list[SessionConfigOption]: options: list[SessionConfigOption] = [ - select_option("model", model, efforts_by_model, category=MODEL_CATEGORY) + select_option( + "model", active["model"], efforts_by_model, category=MODEL_CATEGORY + ) ] - if efforts := efforts_by_model[model]: + efforts = efforts_by_model[active["model"]] + if efforts: + if active["effort"] not in efforts: + active["effort"] = default_effort options.append( select_option( "reasoning_effort", - effort or efforts[0], + active["effort"], efforts, category=THOUGHT_LEVEL_CATEGORY, ) @@ -155,13 +167,20 @@ def catalog(model: str, effort: str | None = None) -> list[SessionConfigOption]: async def select( fake: FakeACPAgent, session_id: str, option_id: str, value: str ) -> list[SessionConfigOption]: - del fake, session_id if option_id == "model": active["model"] = value - return catalog(value) - return catalog(active["model"], effort=value) + elif value in efforts_by_model[active["model"]]: + active["effort"] = value + options = catalog() + await fake.emit( + session_id, + ConfigOptionUpdate( + session_update="config_option_update", config_options=options + ), + ) + return options - self._config_options = catalog(current) + self._config_options = catalog() return self def will_say(self, text: str) -> FakeACPAgent: diff --git a/tests/integrations/acp/test_session_config.py b/tests/integrations/acp/test_session_config.py index 3954c6a1e..c2d468334 100644 --- a/tests/integrations/acp/test_session_config.py +++ b/tests/integrations/acp/test_session_config.py @@ -537,6 +537,27 @@ async def test_an_adapter_can_locate_selects_that_carry_no_category( assert reply.texts == ["Configured"] assert agent.config_selections() == [("model", "large"), ("thinking", "high")] + @pytest.mark.asyncio + async def test_a_switch_back_reaches_an_agent_that_only_replies_with_catalogs( + self, + ) -> None: + # No config_option_update is pushed, so the set replies are the only + # record that the session moved off "small". + agent = FakeACPAgent( + config_options=[ + select_option("model", "small", ["small", "large"], category="model") + ] + ).will_say("ok") + + async with acp_adapter(agent) as session: + await session.send("Hello") + for model in ("large", "small"): + await session.adapter.apply_model_selection( + ModelSelection(model=model), room_id="room-1" + ) + + assert agent.config_selections() == [("model", "large"), ("model", "small")] + @pytest.mark.asyncio async def test_a_model_is_refused_when_the_agent_advertises_no_model_option( self, From 079de57fc6666d200f7ddb9f903847ba999c7e4d Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 13:48:39 +0300 Subject: [PATCH 08/17] fix(acp): let the agent's reply, not a cached catalog, confirm each set apply_session_config_selections no longer skips a value the cached catalog already shows as current. A set whose reply timed out leaves that catalog stale, so the skip could drop a later switch back while reporting success. Every requested value is now sent and confirmed by the agent's refreshed catalog. Copilot accepts re-setting a current value (probed live). A runtime switch now re-checks, once it holds the session's config lock, that the room still has the same live session. A switch queued behind a room cleanup therefore fails with "no live ACP session" rather than a misleading "advertises no model option". Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- src/band/integrations/acp/client_adapter.py | 37 ++++++++++++------- src/band/integrations/acp/client_runtime.py | 5 ++- src/band/integrations/acp/session_config.py | 5 +-- tests/adapters/test_copilot_acp_adapter.py | 19 ++++++++++ tests/integrations/acp/test_session_config.py | 29 +++++++++++++++ 5 files changed, 76 insertions(+), 19 deletions(-) diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index d7eac85b1..305165f97 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -354,18 +354,28 @@ async def apply_model_selection( ) -> None: """Switch ``room_id``'s live ACP session, checked against its catalog. - Raises ``ACPConfigError`` naming what the session offers when it - rejects ``selection``; new rooms keep the configured selection. + Raises ``BandConfigError`` (an ``ACPConfigError`` naming what the + session offers, when it rejects ``selection``); new rooms keep the + configured selection. """ + session = await self._live_session(room_id) + session_id, runtime = session + async with runtime.config_lock(session_id): + # A cleanup may have ended the session while this switch queued. + if await self._live_session(room_id) != session: + raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") + await self._apply_model_selection(runtime, session_id, selection) + logger.info( + "Switched ACP session %s for room %s to %s", session_id, room_id, selection + ) + + async def _live_session(self, room_id: str) -> tuple[str, ACPRuntime]: async with self._session_lock: session_id = self._room_to_session.get(room_id) runtime = self._runtimes.get(room_id) if session_id is None or runtime is None: raise BandConfigError(f"room {room_id} has no live ACP session to switch") - await self._apply_model_selection(runtime, session_id, selection) - logger.info( - "Switched ACP session %s for room %s to %s", session_id, room_id, selection - ) + return session_id, runtime def locate_model_options( self, options: Sequence[SessionConfigOption] @@ -1096,14 +1106,13 @@ async def _configure_session( async def _apply_model_selection( self, runtime: ACPRuntime, session_id: str, selection: ModelSelection ) -> None: - async with runtime.config_lock(session_id): - await apply_model_selection( - session_id=session_id, - config_options=runtime.config_options(session_id), - selection=selection, - locate=self.locate_model_options, - set_option=_config_setter(runtime), - ) + await apply_model_selection( + session_id=session_id, + config_options=runtime.config_options(session_id), + selection=selection, + locate=self.locate_model_options, + set_option=_config_setter(runtime), + ) async def _apply_resolved_config( self, diff --git a/src/band/integrations/acp/client_runtime.py b/src/band/integrations/acp/client_runtime.py index 9a62e7ce1..89ed5a344 100644 --- a/src/band/integrations/acp/client_runtime.py +++ b/src/band/integrations/acp/client_runtime.py @@ -1023,10 +1023,11 @@ def config_options(self, session_id: str) -> tuple[SessionConfigOption, ...]: return self._client.config_options(session_id) def config_lock(self, session_id: str) -> asyncio.Lock: - """Held across a multi-step config change on ``session_id``. + """Held across a runtime switch on ``session_id``. Each step is checked against the catalog the previous one returned, so - an interleaved change would validate against a model no longer current. + an interleaved switch would validate against a model no longer current. + Session setup needs no lock: its room is not yet published to switch. """ return self._config_locks.setdefault(session_id, asyncio.Lock()) diff --git a/src/band/integrations/acp/session_config.py b/src/band/integrations/acp/session_config.py index 0f8740be0..69ebdd565 100644 --- a/src/band/integrations/acp/session_config.py +++ b/src/band/integrations/acp/session_config.py @@ -135,9 +135,8 @@ async def apply_session_config_selections( ), ) - if selected_value == option.current_value: - continue - + # Sent even when the catalog already shows it: a set whose reply was + # lost leaves that catalog stale, and only the agent's reply is proof. available_values = select_values(option) if selected_value not in available_values: raise ACPConfigError( diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index 8ab8a1e12..04e1f87ec 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -349,6 +349,25 @@ async def test_concurrent_switches_on_a_room_apply_in_call_order(self) -> None: assert agent.current_value("model") == "claude-sonnet-5" + @pytest.mark.asyncio + async def test_a_switch_queued_behind_a_room_cleanup_names_the_ended_session( + self, + ) -> None: + agent = copilot() + + async with copilot_room(agent) as session: + await session.send("Hello") + gate = agent.holds_config_replies() + first = asyncio.create_task(switch_room(session, model="gpt-5.4")) + await gate.received.wait() + queued = asyncio.create_task(switch_room(session, model="claude-haiku-4.5")) + await asyncio.sleep(0) # one tick: the queued switch parks on the lock + await session.adapter.on_cleanup("room-1") + results = await asyncio.gather(first, queued, return_exceptions=True) + + assert isinstance(results[1], BandConfigError) + assert "no live ACP session" in str(results[1]) + @pytest.mark.asyncio async def test_switching_a_room_without_a_live_session_is_refused(self) -> None: async with copilot_room(copilot()) as session: diff --git a/tests/integrations/acp/test_session_config.py b/tests/integrations/acp/test_session_config.py index c2d468334..3931006b3 100644 --- a/tests/integrations/acp/test_session_config.py +++ b/tests/integrations/acp/test_session_config.py @@ -15,7 +15,9 @@ SetSessionConfigOptionResponse, ) +from band.core.exceptions import BandConfigError from band.core.model_catalog import ModelCatalog, ModelChoice, ModelSelection +from band.integrations.acp import session_config from band.integrations.acp.client_adapter import ACPClientAdapter from band.integrations.acp.client_types import ACPClientSessionState from band.integrations.acp.model_selection import ACPModelOptions @@ -558,6 +560,33 @@ async def test_a_switch_back_reaches_an_agent_that_only_replies_with_catalogs( assert agent.config_selections() == [("model", "large"), ("model", "small")] + @pytest.mark.asyncio + async def test_a_switch_back_after_a_timed_out_switch_reaches_the_agent( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # The agent applies "large" but its reply misses the deadline, so the + # client never sees the session leave "small". + monkeypatch.setattr(session_config, "SESSION_CONFIG_TIMEOUT_SECONDS", 0.05) + agent = FakeACPAgent( + config_options=[ + select_option("model", "small", ["small", "large"], category="model") + ] + ).will_say("ok") + + async with acp_adapter(agent) as session: + await session.send("Hello") + gate = agent.holds_config_replies() + with pytest.raises(BandConfigError, match="did not respond"): + await session.adapter.apply_model_selection( + ModelSelection(model="large"), room_id="room-1" + ) + gate.release.set() + await session.adapter.apply_model_selection( + ModelSelection(model="small"), room_id="room-1" + ) + + assert agent.current_value("model") == "small" + @pytest.mark.asyncio async def test_a_model_is_refused_when_the_agent_advertises_no_model_option( self, From a50dab16c4d232d17834c57776ce109d0d4966d0 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 13:50:02 +0300 Subject: [PATCH 09/17] refactor: trim model selection startup and ACP apply plumbing Inline adapter startup validation, clarify naming and docs, and drop ModelSelection.only in favor of a single validate path. Co-authored-by: Cursor --- src/band/agent.py | 6 +++--- src/band/core/model_catalog.py | 14 +------------- src/band/core/simple_adapter.py | 19 +++++++------------ src/band/integrations/acp/client_adapter.py | 4 ++-- src/band/integrations/acp/model_selection.py | 17 +++++++++-------- 5 files changed, 22 insertions(+), 38 deletions(-) diff --git a/src/band/agent.py b/src/band/agent.py index d63cee50c..c618556ee 100644 --- a/src/band/agent.py +++ b/src/band/agent.py @@ -275,9 +275,9 @@ async def start(self) -> None: on_control=getattr(self._adapter, "on_interrupt", None), ) except BaseException: - # on_started may have acquired resources (e.g. a CLI runtime - # subprocess); a failed start must release them — stop() won't - # run for an agent that never started. + # Adapter startup may have acquired resources (e.g. a CLI + # runtime subprocess); a failed start must release them — + # stop() won't run for an agent that never started. await release_adapter(self._adapter) raise except BaseException: diff --git a/src/band/core/model_catalog.py b/src/band/core/model_catalog.py index f899f7cce..8381b96b7 100644 --- a/src/band/core/model_catalog.py +++ b/src/band/core/model_catalog.py @@ -1,11 +1,4 @@ -"""Model and reasoning-effort selection, checked against what a harness offers. - -Every adapter speaks this vocabulary: ``SimpleAdapter.model_selection`` is what -it is configured to use, ``SimpleAdapter.list_models()`` is what its harness -advertises, and ``check_model_selection`` is the one rule deciding whether the -two agree. Harnesses report their own catalogs; nothing here hardcodes a model -or effort name. -""" +"""Model and reasoning-effort selection checked against a harness catalog.""" from __future__ import annotations @@ -38,11 +31,6 @@ class ModelSelection(BaseModel): model: str | None = None reasoning_effort: str | None = None - @classmethod - def only(cls, setting: ModelSetting, value: str) -> ModelSelection: - """A selection of ``value`` for ``setting`` alone.""" - return cls.model_validate({setting: value}) - def value_of(self, setting: ModelSetting) -> str | None: return getattr(self, setting) diff --git a/src/band/core/simple_adapter.py b/src/band/core/simple_adapter.py index 085318a31..fc32a5d1e 100644 --- a/src/band/core/simple_adapter.py +++ b/src/band/core/simple_adapter.py @@ -176,12 +176,11 @@ def _resolve_features( def apply_effective_features(self, features: AdapterFeatures) -> None: """Adopt ``features`` as the post-negotiation feature set. - Called by ``Agent.start()``/``OneShotInvoker.startup()`` after pruning - capabilities the deployment doesn't serve (see - ``runtime.capabilities.prune_unsupported``). Override if the adapter - caches something derived from ``self.features`` at construction (e.g. - a tool-definition list built in ``__init__``) so that cache rebuilds - too. + Called from ``runtime.startup.start_adapter`` after pruning capabilities + the deployment doesn't serve (see ``runtime.capabilities.prune_unsupported``). + Override if the adapter caches something derived from ``self.features`` at + construction (e.g. a tool-definition list built in ``__init__``) so that + cache rebuilds too. """ self.features = features @@ -350,13 +349,9 @@ async def startup( """ self.apply_effective_features(features) await self.on_started(agent_name, agent_description) - await self._validate_model_selection() - - async def _validate_model_selection(self) -> None: selection = self.model_selection - if selection.is_empty: - return - check_model_selection(selection, await self.list_models()) + if not selection.is_empty: + check_model_selection(selection, await self.list_models()) async def on_started(self, agent_name: str, agent_description: str) -> None: """Override for post-start setup.""" diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index 305165f97..e3fae5c57 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -169,6 +169,7 @@ class ACPTurnTimeoutError(TimeoutError): NEW_MESSAGE_MARKER_PREFIX = "[New Message" SESSION_CLOSE_TIMEOUT_SECONDS = 5.0 DEFAULT_TURN_TIMEOUT_SECONDS = 300.0 +_EMPTY_MODEL_SELECTION = ModelSelection() def new_message_marker() -> str: @@ -279,14 +280,13 @@ def __init__( client_capabilities: ClientCapabilities | None = None, use_unstable_protocol: bool = False, turn_timeout_s: float = DEFAULT_TURN_TIMEOUT_SECONDS, - model_selection: ModelSelection | None = None, + model_selection: ModelSelection = _EMPTY_MODEL_SELECTION, **features: Unpack[FeatureKwargs], ) -> None: super().__init__( history_converter=ACPClientHistoryConverter(), **features, ) - model_selection = model_selection or ModelSelection() if not model_selection.is_empty and resolve_session_config is not None: raise ValueError( "set either model_selection or resolve_session_config, not both" diff --git a/src/band/integrations/acp/model_selection.py b/src/band/integrations/acp/model_selection.py index 31797126b..8d971b409 100644 --- a/src/band/integrations/acp/model_selection.py +++ b/src/band/integrations/acp/model_selection.py @@ -90,37 +90,38 @@ async def apply_model_selection( effort is located and checked only after the model has been applied. Returns the catalog after the last change. """ - catalog = tuple(config_options) + options = tuple(config_options) for setting in ModelSetting: value = selection.value_of(setting) if value is None: continue - catalog = await _apply_setting( + options = await _apply_setting( session_id=session_id, - catalog=catalog, + config_options=options, setting=setting, value=value, locate=locate, set_option=set_option, ) - return catalog + return options async def _apply_setting( *, session_id: str, - catalog: tuple[SessionConfigOption, ...], + config_options: tuple[SessionConfigOption, ...], setting: ModelSetting, value: str, locate: Callable[[Sequence[SessionConfigOption]], ACPModelOptions], set_option: SessionConfigSetter, ) -> tuple[SessionConfigOption, ...]: - located = locate(catalog) + located = locate(config_options) option = located.option_for(setting) option_id = option.id if option is not None else setting try: check_model_selection( - ModelSelection.only(setting, value), located.model_catalog() + ModelSelection.model_validate({setting: value}), + located.model_catalog(), ) except ModelSelectionError as error: raise ACPConfigError( @@ -138,7 +139,7 @@ async def _apply_setting( ) return await apply_session_config_selections( session_id=session_id, - config_options=catalog, + config_options=config_options, selections={option.id: value}, set_option=set_option, ) From 68b84041660eecf43b01f520dbc57bab645a1368 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 13:59:25 +0300 Subject: [PATCH 10/17] feat(acp): keep a room's model switch across its later sessions A runtime switch changed only the room's live session. A session that recovery recreated, or that session/load restored, was configured from the startup selection again, so the switch was silently undone. The adapter now keeps each room's selection, updated by each switch, for its whole lifetime, and every later session for that room starts from it. Settings a switch leaves unset keep their configured values. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- src/band/core/model_catalog.py | 4 +++ src/band/integrations/acp/client_adapter.py | 17 +++++++++--- tests/adapters/test_copilot_acp_adapter.py | 29 +++++++++++++++++++++ tests/integrations/acp/acp_toolkit/agent.py | 11 +++++--- 4 files changed, 55 insertions(+), 6 deletions(-) diff --git a/src/band/core/model_catalog.py b/src/band/core/model_catalog.py index 8381b96b7..797de4468 100644 --- a/src/band/core/model_catalog.py +++ b/src/band/core/model_catalog.py @@ -34,6 +34,10 @@ class ModelSelection(BaseModel): def value_of(self, setting: ModelSetting) -> str | None: return getattr(self, setting) + def updated_by(self, change: ModelSelection) -> ModelSelection: + """This selection with every setting ``change`` names replaced.""" + return self.model_copy(update=change.model_dump(exclude_none=True)) + @property def is_empty(self) -> bool: return self.model is None and self.reasoning_effort is None diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index e3fae5c57..ddace5e22 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -328,6 +328,10 @@ def __init__( self._turn_timeout_s = turn_timeout_s self._room_to_session: dict[str, str] = {} + # Each room's selection after its runtime switches. Kept for the + # adapter's lifetime, not its sessions', so a room's recreated or + # restored session starts on the model the room last switched to. + self._room_selections: dict[str, ModelSelection] = {} self._session_initializers: dict[str, SessionInitializer] = {} self._room_tools: dict[str, AgentToolsProtocol] = {} self._background_tasks: set[asyncio.Task[None]] = set() @@ -355,8 +359,9 @@ async def apply_model_selection( """Switch ``room_id``'s live ACP session, checked against its catalog. Raises ``BandConfigError`` (an ``ACPConfigError`` naming what the - session offers, when it rejects ``selection``); new rooms keep the - configured selection. + session offers, when it rejects ``selection``). The switch stays with + the room: every later session for it starts from the configured + selection updated by each switch. Other rooms keep the configured one. """ session = await self._live_session(room_id) session_id, runtime = session @@ -365,10 +370,16 @@ async def apply_model_selection( if await self._live_session(room_id) != session: raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") await self._apply_model_selection(runtime, session_id, selection) + self._room_selections[room_id] = self._room_selection(room_id).updated_by( + selection + ) logger.info( "Switched ACP session %s for room %s to %s", session_id, room_id, selection ) + def _room_selection(self, room_id: str) -> ModelSelection: + return self._room_selections.get(room_id, self.model_selection) + async def _live_session(self, room_id: str) -> tuple[str, ACPRuntime]: async with self._session_lock: session_id = self._room_to_session.get(room_id) @@ -1092,7 +1103,7 @@ async def _configure_session( self, runtime: ACPRuntime, room_id: str, session_id: str ) -> None: """Apply caller-selected values from the session's live ACP catalog.""" - selection = self.model_selection + selection = self._room_selection(room_id) if not selection.is_empty: await self._apply_model_selection(runtime, session_id, selection) elif self._resolve_session_config is not None: diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index 04e1f87ec..19796c3b3 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -349,6 +349,35 @@ async def test_concurrent_switches_on_a_room_apply_in_call_order(self) -> None: assert agent.current_value("model") == "claude-sonnet-5" + @pytest.mark.asyncio + @pytest.mark.parametrize( + ("history", "next_session"), + [ + pytest.param(None, "fake-session-2", id="recreated"), + pytest.param( + ACPClientSessionState(room_to_session={"room-1": "persisted-session"}), + "persisted-session", + id="restored", + ), + ], + ) + async def test_a_switch_outlives_the_session_it_was_made_on( + self, history: ACPClientSessionState | None, next_session: str + ) -> None: + # The switch changes the model only, so the configured effort stays. + agent = copilot() + + async with copilot_room(agent, reasoning_effort="high") as session: + await session.send("Hello") + await switch_room(session, model="gpt-5.4") + await session.adapter.on_cleanup("room-1") + await session.send("Again", bootstrap=history is not None, history=history) + + assert agent.config_selections(next_session) == [ + ("model", "gpt-5.4"), + ("reasoning_effort", "high"), + ] + @pytest.mark.asyncio async def test_a_switch_queued_behind_a_room_cleanup_names_the_ended_session( self, diff --git a/tests/integrations/acp/acp_toolkit/agent.py b/tests/integrations/acp/acp_toolkit/agent.py index e96500efd..1a672f875 100644 --- a/tests/integrations/acp/acp_toolkit/agent.py +++ b/tests/integrations/acp/acp_toolkit/agent.py @@ -520,9 +520,14 @@ async def close_session(self, session_id: str, **kwargs: Any) -> None: del kwargs self.closed_sessions.append(session_id) - def config_selections(self) -> list[tuple[str, str]]: - """Each ``(option, value)`` the client set, in order, across sessions.""" - return [(option, value) for _sid, option, value in self.config_option_requests] + def config_selections(self, session_id: str | None = None) -> list[tuple[str, str]]: + """Each ``(option, value)`` the client set, in order, on ``session_id`` + or across sessions.""" + return [ + (option, value) + for sid, option, value in self.config_option_requests + if session_id in (None, sid) + ] def prompt_texts(self) -> list[str]: """Each received prompt's text, one string per prompt, in arrival order.""" From b92eebb4fefb85c5a3f24ce93a20f5a5ec33b727 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 14:07:13 +0300 Subject: [PATCH 11/17] fix(acp): carry forward what a switched session runs, not a merged selection Found live against Copilot. A room configured with gpt-5.4/high and switched to claude-haiku-4.5 remembered (haiku, high), because the switch was merged into the configured selection. Haiku offers no effort, so the room's recreated session failed configuration. The room now keeps the model and effort its switched session reports in the refreshed catalog, so a later session starts exactly where the switch left it. Branch coverage of the changed code: - a harness with no catalog starts on any selection; - a switch whose room's session ended while it queued names that, and the gone and replaced cases now share one check; - a stopped runtime keeps no session catalog. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- src/band/core/model_catalog.py | 4 --- src/band/integrations/acp/client_adapter.py | 22 ++++++++------- src/band/integrations/acp/model_selection.py | 7 +++++ tests/adapters/test_copilot_acp_adapter.py | 23 ++++++++++++++- tests/core/test_simple_adapter.py | 17 +++++++++++ tests/integrations/acp/test_client_runtime.py | 28 ++++++++++++++++++- 6 files changed, 85 insertions(+), 16 deletions(-) diff --git a/src/band/core/model_catalog.py b/src/band/core/model_catalog.py index 797de4468..8381b96b7 100644 --- a/src/band/core/model_catalog.py +++ b/src/band/core/model_catalog.py @@ -34,10 +34,6 @@ class ModelSelection(BaseModel): def value_of(self, setting: ModelSetting) -> str | None: return getattr(self, setting) - def updated_by(self, change: ModelSelection) -> ModelSelection: - """This selection with every setting ``change`` names replaced.""" - return self.model_copy(update=change.model_dump(exclude_none=True)) - @property def is_empty(self) -> bool: return self.model is None and self.reasoning_effort is None diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index ddace5e22..c5f2fe9c8 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -328,9 +328,9 @@ def __init__( self._turn_timeout_s = turn_timeout_s self._room_to_session: dict[str, str] = {} - # Each room's selection after its runtime switches. Kept for the - # adapter's lifetime, not its sessions', so a room's recreated or - # restored session starts on the model the room last switched to. + # What each switched room's session ran after its last runtime switch. + # Kept for the adapter's lifetime, not its sessions', so the room's + # recreated or restored session starts where the switch left it. self._room_selections: dict[str, ModelSelection] = {} self._session_initializers: dict[str, SessionInitializer] = {} self._room_tools: dict[str, AgentToolsProtocol] = {} @@ -360,19 +360,21 @@ async def apply_model_selection( Raises ``BandConfigError`` (an ``ACPConfigError`` naming what the session offers, when it rejects ``selection``). The switch stays with - the room: every later session for it starts from the configured - selection updated by each switch. Other rooms keep the configured one. + the room: every later session for it starts on the model and effort + the switched session reported. Other rooms keep the configured one. """ session = await self._live_session(room_id) + if session is None: + raise BandConfigError(f"room {room_id} has no live ACP session to switch") session_id, runtime = session async with runtime.config_lock(session_id): # A cleanup may have ended the session while this switch queued. if await self._live_session(room_id) != session: raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") await self._apply_model_selection(runtime, session_id, selection) - self._room_selections[room_id] = self._room_selection(room_id).updated_by( - selection - ) + self._room_selections[room_id] = self.locate_model_options( + runtime.config_options(session_id) + ).current_selection() logger.info( "Switched ACP session %s for room %s to %s", session_id, room_id, selection ) @@ -380,12 +382,12 @@ async def apply_model_selection( def _room_selection(self, room_id: str) -> ModelSelection: return self._room_selections.get(room_id, self.model_selection) - async def _live_session(self, room_id: str) -> tuple[str, ACPRuntime]: + async def _live_session(self, room_id: str) -> tuple[str, ACPRuntime] | None: async with self._session_lock: session_id = self._room_to_session.get(room_id) runtime = self._runtimes.get(room_id) if session_id is None or runtime is None: - raise BandConfigError(f"room {room_id} has no live ACP session to switch") + return None return session_id, runtime def locate_model_options( diff --git a/src/band/integrations/acp/model_selection.py b/src/band/integrations/acp/model_selection.py index 8d971b409..6f65c10ce 100644 --- a/src/band/integrations/acp/model_selection.py +++ b/src/band/integrations/acp/model_selection.py @@ -43,6 +43,13 @@ def option_for(self, setting: ModelSetting) -> SessionConfigOptionSelect | None: case ModelSetting.REASONING_EFFORT: return self.effort + def current_selection(self) -> ModelSelection: + """What the session currently runs; no effort when it offers none.""" + return ModelSelection( + model=self.model.current_value if self.model else None, + reasoning_effort=self.effort.current_value if self.effort else None, + ) + def model_catalog(self) -> ModelCatalog | None: """The advertised models; efforts are known only for the current one.""" if self.model is None: diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index 19796c3b3..1309440d7 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -378,6 +378,27 @@ async def test_a_switch_outlives_the_session_it_was_made_on( ("reasoning_effort", "high"), ] + @pytest.mark.asyncio + async def test_a_switch_to_a_model_without_efforts_drops_the_configured_effort( + self, + ) -> None: + # Haiku offers no effort, so the room's next session must not ask for + # the configured "high" (found live: it failed configuration). + agent = copilot() + + async with copilot_room( + agent, model="gpt-5.4", reasoning_effort="high" + ) as session: + await session.send("Hello") + await switch_room(session, model="claude-haiku-4.5") + await session.adapter.on_cleanup("room-1") + reply = await session.send("Again") + + assert reply.errors == [] + assert agent.config_selections("fake-session-2") == [ + ("model", "claude-haiku-4.5") + ] + @pytest.mark.asyncio async def test_a_switch_queued_behind_a_room_cleanup_names_the_ended_session( self, @@ -395,7 +416,7 @@ async def test_a_switch_queued_behind_a_room_cleanup_names_the_ended_session( results = await asyncio.gather(first, queued, return_exceptions=True) assert isinstance(results[1], BandConfigError) - assert "no live ACP session" in str(results[1]) + assert str(results[1]) == "room room-1's ACP session ended mid-switch" @pytest.mark.asyncio async def test_switching_a_room_without_a_live_session_is_refused(self) -> None: diff --git a/tests/core/test_simple_adapter.py b/tests/core/test_simple_adapter.py index 241560e39..0aeacd9d7 100644 --- a/tests/core/test_simple_adapter.py +++ b/tests/core/test_simple_adapter.py @@ -256,6 +256,23 @@ async def test_defaults_empty(self): assert adapter.agent_description == "" +class SelectingAdapter(RecordingAdapter): + """Configured with a model, but its harness lists no catalog.""" + + @property + def model_selection(self) -> ModelSelection: + return ModelSelection(model="any-model", reasoning_effort="any-effort") + + +class TestStartupModelCheck: + async def test_a_harness_without_a_catalog_starts_on_any_selection(self) -> None: + adapter = SelectingAdapter() + + await adapter.startup("Agent", "desc", features=adapter.features) + + assert adapter.agent_name == "Agent" + + class TestApplyModelSelection: async def test_an_adapter_that_cannot_switch_models_refuses(self) -> None: with pytest.raises(BandConfigError, match="does not support switching"): diff --git a/tests/integrations/acp/test_client_runtime.py b/tests/integrations/acp/test_client_runtime.py index 8ec9eb972..d7f97f694 100644 --- a/tests/integrations/acp/test_client_runtime.py +++ b/tests/integrations/acp/test_client_runtime.py @@ -10,7 +10,11 @@ import pytest from acp.client.connection import ClientSideConnection from acp.exceptions import RequestError -from acp.schema import ClientCapabilities, DeclineElicitationResponse +from acp.schema import ( + ClientCapabilities, + DeclineElicitationResponse, + NewSessionResponse, +) from band.integrations.acp.client_profiles import ( CursorACPClientProfile, @@ -24,6 +28,7 @@ tcp_spawn_process, ) from band.integrations.acp.types import ChunkType, CollectedChunk +from tests.integrations.acp.acp_toolkit import FakeSpawn, select_option class TestSelectAllowOptionId: @@ -835,6 +840,27 @@ async def test_create_session_and_prompt_use_active_connection(self) -> None: mock_conn.new_session.assert_awaited_once_with(cwd="/tmp", mcp_servers=[]) mock_conn.prompt.assert_awaited_once() + @pytest.mark.asyncio + async def test_a_stopped_runtime_keeps_no_session_catalog(self) -> None: + # Session ids belong to the agent process; a respawn must not reuse + # what the stopped process advertised. + spawn = FakeSpawn() + spawn.conn.new_session = AsyncMock( + return_value=NewSessionResponse( + session_id="sess-1", + config_options=[select_option("model", "small", ["small", "large"])], + ) + ) + runtime = ACPRuntime(command=["codex"], spawn_process=spawn) + await runtime.start() + await runtime.create_session_response(cwd="/tmp", mcp_servers=[]) + advertised = runtime.config_options("sess-1") + + await runtime.stop() + + assert [option.id for option in advertised] == ["model"] + assert runtime.config_options("sess-1") == () + @pytest.mark.asyncio async def test_close_session_uses_the_active_connection_when_supported( self, From 87ea6f8ad0171e3ecfc565264c9e89608d90a3c6 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 14:23:24 +0300 Subject: [PATCH 12/17] fix(acp): make a room's remembered switch safe to carry and to drop Fixes from the second review of the per-room model switch: - The room remembers what its session runs even when a switch fails partway. If the model landed and the effort was refused, the next session no longer goes back to the pre-switch model. - A remembered switch that a later session refuses fails that turn only. The room then goes back to the configured selection, where it used to stay broken until the process restarted. - A runtime switch is refused beside resolve_session_config, so it can no longer silently replace the resolver for the room's later sessions. - A switch whose session ended while its set was in flight records nothing and reports that the session ended. - The runtime holds one config lock (it serves one room) and forgets a closed session's catalog. Tests: the agent-initiated and set-reply-only cases now fail when that catalog source is not recorded, which always sending every value had hidden. Duplicated E2E skip messages and the ConfigOptionUpdate push each have one helper, and the hosted cell no longer requires Anthropic. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- src/band/integrations/acp/client_adapter.py | 62 +++++++++++++++---- src/band/integrations/acp/client_runtime.py | 14 +++-- tests/adapters/test_copilot_acp_adapter.py | 50 ++++++++++++--- tests/e2e/baseline/settings.py | 6 +- .../smoke/adapters/test_copilot_acp.py | 19 +++--- tests/integrations/acp/acp_toolkit/agent.py | 20 +++--- tests/integrations/acp/test_session_config.py | 49 ++++++++++++--- 7 files changed, 164 insertions(+), 56 deletions(-) diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index c5f2fe9c8..905e5cd0c 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -328,9 +328,7 @@ def __init__( self._turn_timeout_s = turn_timeout_s self._room_to_session: dict[str, str] = {} - # What each switched room's session ran after its last runtime switch. - # Kept for the adapter's lifetime, not its sessions', so the room's - # recreated or restored session starts where the switch left it. + # Outlives the room's sessions; see apply_model_selection. self._room_selections: dict[str, ModelSelection] = {} self._session_initializers: dict[str, SessionInitializer] = {} self._room_tools: dict[str, AgentToolsProtocol] = {} @@ -359,24 +357,42 @@ async def apply_model_selection( """Switch ``room_id``'s live ACP session, checked against its catalog. Raises ``BandConfigError`` (an ``ACPConfigError`` naming what the - session offers, when it rejects ``selection``). The switch stays with - the room: every later session for it starts on the model and effort - the switched session reported. Other rooms keep the configured one. + session offers, when it rejects ``selection``). The room keeps what + its session then runs, even after a partial failure, for the + adapter's lifetime: every later session for it starts there, until + one refuses it. Other rooms keep the configured selection. Refused + when ``resolve_session_config`` configures sessions instead. """ + if self._resolve_session_config is not None: + raise BandConfigError( + "a runtime model switch cannot be combined with resolve_session_config" + ) session = await self._live_session(room_id) if session is None: raise BandConfigError(f"room {room_id} has no live ACP session to switch") session_id, runtime = session - async with runtime.config_lock(session_id): + async with runtime.config_lock: # A cleanup may have ended the session while this switch queued. if await self._live_session(room_id) != session: raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") - await self._apply_model_selection(runtime, session_id, selection) - self._room_selections[room_id] = self.locate_model_options( - runtime.config_options(session_id) - ).current_selection() + try: + await self._apply_model_selection(runtime, session_id, selection) + finally: + still_live = await self._live_session(room_id) == session + if still_live: + self._remember_room_selection(room_id, runtime, session_id) + if not still_live: + raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") + + def _remember_room_selection( + self, room_id: str, runtime: ACPRuntime, session_id: str + ) -> None: + running = self.locate_model_options( + runtime.config_options(session_id) + ).current_selection() + self._room_selections[room_id] = running logger.info( - "Switched ACP session %s for room %s to %s", session_id, room_id, selection + "ACP session %s for room %s now runs %s", session_id, room_id, running ) def _room_selection(self, room_id: str) -> ModelSelection: @@ -1107,7 +1123,7 @@ async def _configure_session( """Apply caller-selected values from the session's live ACP catalog.""" selection = self._room_selection(room_id) if not selection.is_empty: - await self._apply_model_selection(runtime, session_id, selection) + await self._apply_room_selection(runtime, room_id, session_id, selection) elif self._resolve_session_config is not None: await self._apply_resolved_config( resolver=self._resolve_session_config, @@ -1116,6 +1132,26 @@ async def _configure_session( runtime=runtime, ) + async def _apply_room_selection( + self, + runtime: ACPRuntime, + room_id: str, + session_id: str, + selection: ModelSelection, + ) -> None: + try: + await self._apply_model_selection(runtime, session_id, selection) + except ACPConfigError: + # A remembered switch the agent no longer accepts must fail this + # turn only; later sessions go back to the configured selection. + if self._room_selections.pop(room_id, None) is not None: + logger.warning( + "Room %s's switched model was refused; reverting to %s", + room_id, + self.model_selection, + ) + raise + async def _apply_model_selection( self, runtime: ACPRuntime, session_id: str, selection: ModelSelection ) -> None: diff --git a/src/band/integrations/acp/client_runtime.py b/src/band/integrations/acp/client_runtime.py index 89ed5a344..3d23be08f 100644 --- a/src/band/integrations/acp/client_runtime.py +++ b/src/band/integrations/acp/client_runtime.py @@ -718,6 +718,9 @@ def record_config_options( def config_options(self, session_id: str) -> tuple[SessionConfigOption, ...]: return self._config_options.get(session_id, ()) + def forget_config_options(self, session_id: str) -> None: + self._config_options.pop(session_id, None) + def reset_session(self, session_id: str) -> None: self._session_chunks.pop(session_id, None) self._permission_handlers.pop(session_id, None) @@ -846,7 +849,7 @@ def __init__( self._agent_mcp_transport: MCPTransportKind = "http" self._agent_supports_session_load = False self._agent_supports_session_close = False - self._config_locks: dict[str, asyncio.Lock] = {} + self._config_lock = asyncio.Lock() async def start(self, *, respawn: bool = False) -> None: """Spawn or respawn the ACP agent subprocess.""" @@ -1022,14 +1025,15 @@ def config_options(self, session_id: str) -> tuple[SessionConfigOption, ...]: return () return self._client.config_options(session_id) - def config_lock(self, session_id: str) -> asyncio.Lock: - """Held across a runtime switch on ``session_id``. + @property + def config_lock(self) -> asyncio.Lock: + """Held across a runtime switch. Each step is checked against the catalog the previous one returned, so an interleaved switch would validate against a model no longer current. Session setup needs no lock: its room is not yet published to switch. """ - return self._config_locks.setdefault(session_id, asyncio.Lock()) + return self._config_lock def _record_config_options(self, session_id: str, response: object) -> None: options = session_config_options(response) @@ -1038,6 +1042,8 @@ def _record_config_options(self, session_id: str, response: object) -> None: async def close_session(self, session_id: str) -> None: """Close a session when the agent advertised lifecycle support.""" + if self._client is not None: + self._client.forget_config_options(session_id) if not self._agent_supports_session_close: return conn = await self.ensure_connection(can_respawn=False) diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index 1309440d7..99c443aea 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -307,32 +307,39 @@ async def test_a_live_room_switches_against_its_current_catalog(self) -> None: async def test_a_partly_applied_switch_leaves_the_room_on_what_the_agent_runs( self, ) -> None: - # The model lands before the effort is refused, so switching back - # must reach the agent rather than match the pre-switch catalog. + # The model lands before the effort is refused; the room's next + # session must start on it, not on the pre-switch model. agent = copilot() async with copilot_room(agent) as session: await session.send("Hello") with pytest.raises(BandConfigError): await switch_room(session, model="gpt-5.4", reasoning_effort="max") - await switch_room(session, model="claude-sonnet-5") + await session.adapter.on_cleanup("room-1") + await session.send("Again") - assert agent.current_value("model") == "claude-sonnet-5" + assert agent.config_selections("fake-session-2") == [ + ("model", "gpt-5.4"), + ("reasoning_effort", "medium"), + ] @pytest.mark.asyncio async def test_a_switch_follows_a_model_the_agent_chose_itself(self) -> None: agent = copilot() @agent.on_prompt - async def pick_gpt(fake: FakeACPAgent, session_id: str) -> None: - await fake.selects_on_its_own(session_id, "model", "gpt-5.4") + async def pick_haiku(fake: FakeACPAgent, session_id: str) -> None: + await fake.selects_on_its_own(session_id, "model", "claude-haiku-4.5") await fake.say(session_id, "Switched") async with copilot_room(agent) as session: - await session.send("Use gpt") - await switch_room(session, model="claude-sonnet-5") + await session.send("Use haiku") + with pytest.raises(BandConfigError) as rejected: + await switch_room(session, reasoning_effort="high") - assert agent.current_value("model") == "claude-sonnet-5" + assert str(rejected.value) == ( + 'model "claude-haiku-4.5" offers no reasoning effort' + ) @pytest.mark.asyncio async def test_concurrent_switches_on_a_room_apply_in_call_order(self) -> None: @@ -383,7 +390,7 @@ async def test_a_switch_to_a_model_without_efforts_drops_the_configured_effort( self, ) -> None: # Haiku offers no effort, so the room's next session must not ask for - # the configured "high" (found live: it failed configuration). + # the configured "high". agent = copilot() async with copilot_room( @@ -399,6 +406,29 @@ async def test_a_switch_to_a_model_without_efforts_drops_the_configured_effort( ("model", "claude-haiku-4.5") ] + @pytest.mark.asyncio + async def test_a_remembered_switch_the_agent_refuses_fails_one_turn_only( + self, + ) -> None: + agent = copilot() + + async with copilot_room(agent) as session: + await session.send("Hello") + await switch_room(session, model="gpt-5.4") + await session.adapter.on_cleanup("room-1") + agent.advertises_models( + {"claude-sonnet-5": COPILOT_EFFORTS["claude-sonnet-5"]}, + current="claude-sonnet-5", + ) + refused = await session.send("Again") + recovered = await session.send("And again") + + assert refused.errors == [ + CONFIG_FAILURE_PREFIX + + 'model "gpt-5.4" is not advertised; available: claude-sonnet-5' + ] + assert recovered.texts == ["Configured"] + @pytest.mark.asyncio async def test_a_switch_queued_behind_a_room_cleanup_names_the_ended_session( self, diff --git a/tests/e2e/baseline/settings.py b/tests/e2e/baseline/settings.py index 5236fa834..c34474674 100644 --- a/tests/e2e/baseline/settings.py +++ b/tests/e2e/baseline/settings.py @@ -177,11 +177,11 @@ class Backends(BaseSettings): # Copilot-hosted auth for the non-BYOK smokes in test_copilot_acp.py; the # BYOK matrix cells never read it. Those smokes skip when unset. github_token: str = "" # GITHUB_TOKEN - # Pins that same smoke's model instead of leaving it to Copilot's `auto` + # Pins those smokes' model instead of leaving it to Copilot's `auto` # picker, which can land on an expensive reasoning-tier model (observed: # gpt-5.6-terra). gpt-5.6-luna is the cheapest model in the GPT-5.6 family - # while still agentic/tool-calling, so the one billed reply turn stays - # cheap and deterministic across runs. + # while still agentic/tool-calling, so their billed turns stay cheap and + # deterministic across runs. copilot_hosted_model: str = "gpt-5.6-luna" # COPILOT_HOSTED_MODEL diff --git a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py index 0db253f5c..5c4489cd8 100644 --- a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py +++ b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py @@ -132,6 +132,12 @@ async def test_acp_band_tool_result_is_a_single_clean_payload( band_results.assert_json_output() +def skip_without_github_token(settings: BaselineSettings) -> None: + """Hosted-auth coverage is optional; the BYOK cells are the lane's bar.""" + if not settings.backends.github_token: + pytest.skip("GITHUB_TOKEN unset — the Copilot-hosted smokes need one") + + def hermetic_copilot_config( settings: BaselineSettings, work_dir: Path, @@ -184,8 +190,7 @@ def hermetic_copilot_config( @pytest.mark.asyncio(loop_scope="session") @pytest.mark.parametrize( "reasoning_effort", - # "high" differs from the hosted model's advertised default ("medium"), - # so the typed selection is really sent through session/set_config_option. + # A non-default effort the hosted model advertises (its default: "medium"). [pytest.param(None, id="default"), pytest.param("high", id="typed-effort")], ) async def test_copilot_hosted_auth_replies( @@ -209,8 +214,7 @@ async def test_copilot_hosted_auth_replies( CopilotACPAdapter, ) - if not baseline_settings.backends.github_token: - pytest.skip("GITHUB_TOKEN unset — the Copilot-hosted auth smoke needs one") + skip_without_github_token(baseline_settings) marker = unique_marker("hosted") identity = await resource_manager.provision_agent("copilot-hosted-auth") @@ -243,7 +247,7 @@ async def test_copilot_hosted_auth_replies( @lane(Lane.BACKENDS) # bespoke build exposes no framework; pin scheduling to backends -@requires(Dep.COPILOT_CLI, Dep.ANTHROPIC) +@requires(Dep.COPILOT_CLI) @pytest.mark.timeout(extra=180) # Copilot CLI cold boot @pytest.mark.asyncio(loop_scope="session") @pytest.mark.parametrize( @@ -254,6 +258,7 @@ async def test_copilot_hosted_auth_replies( False, {"model": "not-a-model"}, "ACP session advertises no model option.", + marks=requires(Dep.ANTHROPIC), id="byok-model", ), # Hosted sessions advertise per-model efforts; the refusal proves the @@ -282,8 +287,8 @@ async def test_copilot_turn_fails_loudly_on_an_unadvertised_selection( CopilotACPAdapter, ) - if hosted and not baseline_settings.backends.github_token: - pytest.skip("GITHUB_TOKEN unset — the Copilot-hosted auth smoke needs one") + if hosted: + skip_without_github_token(baseline_settings) identity = await resource_manager.provision_agent("copilot-unadvertised") room_id = await resource_manager.provision_room( diff --git a/tests/integrations/acp/acp_toolkit/agent.py b/tests/integrations/acp/acp_toolkit/agent.py index 1a672f875..ba3365ade 100644 --- a/tests/integrations/acp/acp_toolkit/agent.py +++ b/tests/integrations/acp/acp_toolkit/agent.py @@ -131,6 +131,7 @@ def advertises_models( *, current: str, default_effort: str = "medium", + pushes_updates: bool = True, ) -> FakeACPAgent: """Advertise a model select whose effort select follows the chosen model. @@ -139,7 +140,8 @@ def advertises_models( with none; the current effort carries over when the new model offers it, else falls back to ``default_effort``; an unoffered effort is silently ignored; and every change is also pushed as a - ``config_option_update``. + ``config_option_update``. ``pushes_updates=False`` models a spec agent + whose set replies are the only record of a change. """ active = {"model": current, "effort": default_effort} @@ -172,12 +174,8 @@ async def select( elif value in efforts_by_model[active["model"]]: active["effort"] = value options = catalog() - await fake.emit( - session_id, - ConfigOptionUpdate( - session_update="config_option_update", config_options=options - ), - ) + if pushes_updates: + await fake.push_config_options(session_id, options) return options self._config_options = catalog() @@ -370,11 +368,15 @@ async def selects_on_its_own( """Change an option agent-side and push ``config_option_update``, as Copilot's in-session ``/model`` does.""" self._config_options = await self._select(session_id, option_id, value) + await self.push_config_options(session_id, self._config_options) + + async def push_config_options( + self, session_id: str, options: Sequence[SessionConfigOption] + ) -> None: await self.emit( session_id, ConfigOptionUpdate( - session_update="config_option_update", - config_options=self._config_options, + session_update="config_option_update", config_options=list(options) ), ) diff --git a/tests/integrations/acp/test_session_config.py b/tests/integrations/acp/test_session_config.py index 3931006b3..6f58818d3 100644 --- a/tests/integrations/acp/test_session_config.py +++ b/tests/integrations/acp/test_session_config.py @@ -20,7 +20,7 @@ from band.integrations.acp import session_config from band.integrations.acp.client_adapter import ACPClientAdapter from band.integrations.acp.client_types import ACPClientSessionState -from band.integrations.acp.model_selection import ACPModelOptions +from band.integrations.acp.model_selection import MODEL_CATEGORY, ACPModelOptions from band.integrations.acp.session_config import ( CONFIG_FAILURE_PREFIX, RESOLVER_CONFIG_OPTION_ID, @@ -540,25 +540,52 @@ async def test_an_adapter_can_locate_selects_that_carry_no_category( assert agent.config_selections() == [("model", "large"), ("thinking", "high")] @pytest.mark.asyncio - async def test_a_switch_back_reaches_an_agent_that_only_replies_with_catalogs( + async def test_a_switch_is_checked_against_the_catalog_a_set_reply_returned( self, ) -> None: - # No config_option_update is pushed, so the set replies are the only - # record that the session moved off "small". + # No config_option_update is pushed, so only the model set's reply + # tells the client that "large" brings a "max" effort. + agent = ( + FakeACPAgent() + .advertises_models( + {"small": (), "large": ("medium", "max")}, + current="small", + pushes_updates=False, + ) + .will_say("ok") + ) + + async with acp_adapter(agent) as session: + await session.send("Hello") + for selection in ( + ModelSelection(model="large"), + ModelSelection(reasoning_effort="max"), + ): + await session.adapter.apply_model_selection(selection, room_id="room-1") + + assert agent.current_value("reasoning_effort") == "max" + + @pytest.mark.asyncio + async def test_a_runtime_switch_is_refused_beside_a_resolver(self) -> None: + async def resolve_config(request: ACPConfigRequest) -> None: + del request + agent = FakeACPAgent( config_options=[ - select_option("model", "small", ["small", "large"], category="model") + select_option( + "model", "small", ["small", "large"], category=MODEL_CATEGORY + ) ] ).will_say("ok") - async with acp_adapter(agent) as session: + async with acp_adapter(agent, resolve_session_config=resolve_config) as session: await session.send("Hello") - for model in ("large", "small"): + with pytest.raises(BandConfigError, match="resolve_session_config"): await session.adapter.apply_model_selection( - ModelSelection(model=model), room_id="room-1" + ModelSelection(model="large"), room_id="room-1" ) - assert agent.config_selections() == [("model", "large"), ("model", "small")] + assert agent.config_selections() == [] @pytest.mark.asyncio async def test_a_switch_back_after_a_timed_out_switch_reaches_the_agent( @@ -569,7 +596,9 @@ async def test_a_switch_back_after_a_timed_out_switch_reaches_the_agent( monkeypatch.setattr(session_config, "SESSION_CONFIG_TIMEOUT_SECONDS", 0.05) agent = FakeACPAgent( config_options=[ - select_option("model", "small", ["small", "large"], category="model") + select_option( + "model", "small", ["small", "large"], category=MODEL_CATEGORY + ) ] ).will_say("ok") From c6ab462330f105ee200cf75a0b04eb567ad8d5ec Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 14:30:47 +0300 Subject: [PATCH 13/17] fix(acp): forget a room's switch only when the agent refuses it A config set that timed out or lost its connection used to erase the room's remembered switch for the adapter's lifetime, just like a real refusal. Such failures now raise ACPConfigUnreachableError. It is an ACPConfigError, so the turn still fails visibly, but it leaves the switch in place. A JSON-RPC error is still the agent's refusal. A switch refused before it changed anything no longer pins the room to whatever its session was running. A failed switch is remembered only as far as it actually moved the session. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- src/band/integrations/acp/client_adapter.py | 34 ++++++++++++------ src/band/integrations/acp/session_config.py | 16 +++++++-- tests/adapters/test_copilot_acp_adapter.py | 36 +++++++++++++++++++ tests/integrations/acp/test_session_config.py | 27 ++++++++++++++ 4 files changed, 101 insertions(+), 12 deletions(-) diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index 905e5cd0c..dcd38c47b 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -79,6 +79,7 @@ RESOLVER_CONFIG_OPTION_ID, ACPConfigError, ACPConfigRequest, + ACPConfigUnreachableError, SessionConfigOption, SessionConfigResolver, SessionConfigSetter, @@ -375,21 +376,32 @@ async def apply_model_selection( # A cleanup may have ended the session while this switch queued. if await self._live_session(room_id) != session: raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") + before = self._running_selection(runtime, session_id) try: await self._apply_model_selection(runtime, session_id, selection) - finally: - still_live = await self._live_session(room_id) == session - if still_live: - self._remember_room_selection(room_id, runtime, session_id) - if not still_live: - raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") + except BaseException: + # A failed switch still counts as far as it moved the session. + if await self._live_session(room_id) == session: + running = self._running_selection(runtime, session_id) + if running != before: + self._remember_room_selection(room_id, session_id, running) + raise + if await self._live_session(room_id) != session: + raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") + self._remember_room_selection( + room_id, session_id, self._running_selection(runtime, session_id) + ) - def _remember_room_selection( - self, room_id: str, runtime: ACPRuntime, session_id: str - ) -> None: - running = self.locate_model_options( + def _running_selection( + self, runtime: ACPRuntime, session_id: str + ) -> ModelSelection: + return self.locate_model_options( runtime.config_options(session_id) ).current_selection() + + def _remember_room_selection( + self, room_id: str, session_id: str, running: ModelSelection + ) -> None: self._room_selections[room_id] = running logger.info( "ACP session %s for room %s now runs %s", session_id, room_id, running @@ -1141,6 +1153,8 @@ async def _apply_room_selection( ) -> None: try: await self._apply_model_selection(runtime, session_id, selection) + except ACPConfigUnreachableError: + raise except ACPConfigError: # A remembered switch the agent no longer accepts must fail this # turn only; later sessions go back to the configured selection. diff --git a/src/band/integrations/acp/session_config.py b/src/band/integrations/acp/session_config.py index 69ebdd565..01ea655b5 100644 --- a/src/band/integrations/acp/session_config.py +++ b/src/band/integrations/acp/session_config.py @@ -7,6 +7,7 @@ from dataclasses import dataclass from typing import TypeAlias +from acp.exceptions import RequestError from acp.schema import ( SessionConfigOptionBoolean, SessionConfigOptionSelect, @@ -61,6 +62,11 @@ def __init__( self.selected_value = selected_value +class ACPConfigUnreachableError(ACPConfigError): + """The agent never answered a config request (a timeout or a lost + connection), so nothing says it refused the value.""" + + def flatten_select_options( options: Sequence[SessionConfigSelectOption | SessionConfigSelectGroup], ) -> tuple[SessionConfigSelectOption, ...]: @@ -158,7 +164,7 @@ async def apply_session_config_selections( except (asyncio.CancelledError, KeyboardInterrupt): raise except TimeoutError as error: - raise ACPConfigError( + raise ACPConfigUnreachableError( session_id=session_id, option_id=option_id, selected_value=selected_value, @@ -168,7 +174,13 @@ async def apply_session_config_selections( ), ) from error except Exception as error: - raise ACPConfigError( + # A JSON-RPC error is the agent's answer; anything else never got one. + error_type = ( + ACPConfigError + if isinstance(error, RequestError) + else ACPConfigUnreachableError + ) + raise error_type( session_id=session_id, option_id=option_id, selected_value=selected_value, diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index 99c443aea..9f444ef28 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -26,6 +26,7 @@ ) from band.core.exceptions import BandConfigError from band.core.model_catalog import ModelSelection +from band.integrations.acp import session_config from band.integrations.acp.client_adapter import ACPClientAdapter from band.integrations.acp.client_profiles import NoopACPClientProfile from band.integrations.acp.client_types import ACPClientSessionState @@ -429,6 +430,41 @@ async def test_a_remembered_switch_the_agent_refuses_fails_one_turn_only( ] assert recovered.texts == ["Configured"] + @pytest.mark.asyncio + async def test_a_remembered_switch_survives_a_set_the_agent_never_answered( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr(session_config, "SESSION_CONFIG_TIMEOUT_SECONDS", 0.05) + agent = copilot() + + async with copilot_room(agent) as session: + await session.send("Hello") + await switch_room(session, model="gpt-5.4") + await session.adapter.on_cleanup("room-1") + gate = agent.holds_config_replies() + timed_out = await session.send("Again") + gate.release.set() + await session.adapter.on_cleanup("room-1") + await session.send("And again") + + assert "did not respond" in timed_out.errors[0] + assert agent.config_selections("fake-session-3")[0] == ("model", "gpt-5.4") + + @pytest.mark.asyncio + async def test_a_switch_refused_before_any_change_leaves_the_room_unpinned( + self, + ) -> None: + agent = copilot() + + async with copilot_room(agent) as session: + await session.send("Hello") + with pytest.raises(BandConfigError): + await switch_room(session, model="gpt-9") + await session.adapter.on_cleanup("room-1") + await session.send("Again") + + assert agent.config_selections("fake-session-2") == [] + @pytest.mark.asyncio async def test_a_switch_queued_behind_a_room_cleanup_names_the_ended_session( self, diff --git a/tests/integrations/acp/test_session_config.py b/tests/integrations/acp/test_session_config.py index 6f58818d3..f60eebb17 100644 --- a/tests/integrations/acp/test_session_config.py +++ b/tests/integrations/acp/test_session_config.py @@ -8,6 +8,7 @@ from unittest.mock import AsyncMock import pytest +from acp.exceptions import RequestError from acp.schema import ( SessionConfigOptionSelect, SessionConfigSelectGroup, @@ -26,6 +27,7 @@ RESOLVER_CONFIG_OPTION_ID, ACPConfigError, ACPConfigRequest, + ACPConfigUnreachableError, SessionConfigOption, apply_session_config_selections, ) @@ -144,6 +146,31 @@ async def test_fails_when_a_prior_selection_removes_a_later_effort_option( ) set_option.assert_awaited_once_with("session-1", "model", "auto") + @pytest.mark.asyncio + @pytest.mark.parametrize( + ("failure", "raised"), + [ + pytest.param(RequestError.invalid_params(), ACPConfigError, id="refused"), + pytest.param( + RuntimeError("Connection closed"), + ACPConfigUnreachableError, + id="unanswered", + ), + ], + ) + async def test_a_failed_set_says_whether_the_agent_answered( + self, failure: Exception, raised: type[ACPConfigError] + ) -> None: + with pytest.raises(ACPConfigError) as rejected: + await apply_session_config_selections( + session_id="session-1", + config_options=[select_option("model", "small", ["small", "large"])], + selections={"model": "large"}, + set_option=AsyncMock(side_effect=failure), + ) + + assert type(rejected.value) is raised + @pytest.mark.asyncio async def test_an_option_on_an_empty_catalog_is_refused_naming_none(self) -> None: with pytest.raises(ACPConfigError) as rejected: From 3033c63afe2a4c150e0c29198343c0a3c69b1988 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 14:54:20 +0300 Subject: [PATCH 14/17] fix(acp): remember a room's switch only when it succeeds The runtime switch now follows one written transition table instead of per-case patches: - Only a switch that returns writes the room's memory, as what its session then runs. A switch that raises, partly applied or not, leaves later sessions where they would have started without it. This drops the before/after snapshot and the failure-path liveness check. - An empty switch is a no-op, so it can no longer pin a room to the agent's defaults or hide the configured selection. - A reply that fails validation is the agent's answer, not a missing one, so a remembered switch it refuses is forgotten. - "Advertises no model option" now lists the session's select ids, like the other config errors, through one select_ids helper. - Config failures log their session, option and value; a remembered switch kept after an unanswered set logs that. Every table row has a test that fails when its rule is reverted. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- src/band/integrations/acp/client_adapter.py | 66 ++++++++++--------- src/band/integrations/acp/model_selection.py | 7 +- src/band/integrations/acp/session_config.py | 23 ++++--- tests/adapters/test_copilot_acp_adapter.py | 40 ++++++++--- tests/e2e/baseline/README.md | 2 +- .../smoke/adapters/test_copilot_acp.py | 6 +- tests/integrations/acp/acp_toolkit/agent.py | 13 +++- tests/integrations/acp/test_client_runtime.py | 9 ++- tests/integrations/acp/test_session_config.py | 10 ++- 9 files changed, 113 insertions(+), 63 deletions(-) diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index dcd38c47b..7b8df5885 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -358,50 +358,44 @@ async def apply_model_selection( """Switch ``room_id``'s live ACP session, checked against its catalog. Raises ``BandConfigError`` (an ``ACPConfigError`` naming what the - session offers, when it rejects ``selection``). The room keeps what - its session then runs, even after a partial failure, for the - adapter's lifetime: every later session for it starts there, until - one refuses it. Other rooms keep the configured selection. Refused - when ``resolve_session_config`` configures sessions instead. + session offers, when it rejects ``selection``). Only a switch that + returns counts: the room then keeps what its session runs for the + adapter's lifetime, and every later session for it starts there + until one refuses it. A switch that raises may leave the live session + partly switched, but later sessions start where they would have + before it. Other rooms keep the configured selection. Refused when + ``resolve_session_config`` configures sessions instead. An empty + ``selection`` changes nothing. """ if self._resolve_session_config is not None: raise BandConfigError( "a runtime model switch cannot be combined with resolve_session_config" ) + if selection.is_empty: + return session = await self._live_session(room_id) if session is None: raise BandConfigError(f"room {room_id} has no live ACP session to switch") session_id, runtime = session async with runtime.config_lock: # A cleanup may have ended the session while this switch queued. - if await self._live_session(room_id) != session: - raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") - before = self._running_selection(runtime, session_id) - try: - await self._apply_model_selection(runtime, session_id, selection) - except BaseException: - # A failed switch still counts as far as it moved the session. - if await self._live_session(room_id) == session: - running = self._running_selection(runtime, session_id) - if running != before: - self._remember_room_selection(room_id, session_id, running) - raise - if await self._live_session(room_id) != session: - raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") - self._remember_room_selection( - room_id, session_id, self._running_selection(runtime, session_id) - ) + await self._ensure_session_live(room_id, session) + await self._apply_model_selection(runtime, session_id, selection) + await self._ensure_session_live(room_id, session) + self._remember_room_selection(room_id, session_id, runtime) - def _running_selection( - self, runtime: ACPRuntime, session_id: str - ) -> ModelSelection: - return self.locate_model_options( - runtime.config_options(session_id) - ).current_selection() + async def _ensure_session_live( + self, room_id: str, session: tuple[str, ACPRuntime] + ) -> None: + if await self._live_session(room_id) != session: + raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") def _remember_room_selection( - self, room_id: str, session_id: str, running: ModelSelection + self, room_id: str, session_id: str, runtime: ACPRuntime ) -> None: + running = self.locate_model_options( + runtime.config_options(session_id) + ).current_selection() self._room_selections[room_id] = running logger.info( "ACP session %s for room %s now runs %s", session_id, room_id, running @@ -1154,6 +1148,11 @@ async def _apply_room_selection( try: await self._apply_model_selection(runtime, session_id, selection) except ACPConfigUnreachableError: + logger.warning( + "Room %s's switched model %s got no answer; keeping it", + room_id, + selection, + ) raise except ACPConfigError: # A remembered switch the agent no longer accepts must fail this @@ -1219,7 +1218,14 @@ async def _report_config_error( tools: AgentToolsProtocol, error: ACPConfigError, ) -> None: - logger.warning("%s%s", CONFIG_FAILURE_PREFIX, error) + logger.warning( + "%s%s (session %s, option %s, value %s)", + CONFIG_FAILURE_PREFIX, + error, + error.session_id, + error.option_id, + error.selected_value, + ) await tools.send_failure( AgentFailure( _PROVIDER, diff --git a/src/band/integrations/acp/model_selection.py b/src/band/integrations/acp/model_selection.py index 6f65c10ce..c730bda2e 100644 --- a/src/band/integrations/acp/model_selection.py +++ b/src/band/integrations/acp/model_selection.py @@ -15,12 +15,14 @@ ModelSetting, check_model_selection, ) +from band.core.validation import listing from band.integrations.acp.session_config import ( ACPConfigError, SessionConfigOption, SessionConfigSetter, apply_session_config_selections, flatten_select_options, + select_ids, select_values, ) @@ -142,7 +144,10 @@ async def _apply_setting( session_id=session_id, option_id=option_id, selected_value=value, - message=f"ACP session advertises no {setting} option.", + message=( + f"ACP session advertises no {setting} option; available: " + f"{listing(select_ids(config_options))}." + ), ) return await apply_session_config_selections( session_id=session_id, diff --git a/src/band/integrations/acp/session_config.py b/src/band/integrations/acp/session_config.py index 01ea655b5..40b3d904e 100644 --- a/src/band/integrations/acp/session_config.py +++ b/src/band/integrations/acp/session_config.py @@ -15,6 +15,7 @@ SessionConfigSelectOption, SetSessionConfigOptionResponse, ) +from pydantic import ValidationError from band.core.exceptions import BandConfigError from band.core.validation import listing @@ -63,8 +64,8 @@ def __init__( class ACPConfigUnreachableError(ACPConfigError): - """The agent never answered a config request (a timeout or a lost - connection), so nothing says it refused the value.""" + """No reply ever came to a config request (a timeout or a lost + connection), so nothing says the agent refused the value.""" def flatten_select_options( @@ -85,6 +86,13 @@ def select_values(option: SessionConfigOptionSelect) -> tuple[str, ...]: return tuple(entry.value for entry in flatten_select_options(option.options)) +def select_ids(options: Sequence[SessionConfigOption]) -> tuple[str, ...]: + """The ids of the select options in an ACP catalog.""" + return tuple( + option.id for option in options if isinstance(option, SessionConfigOptionSelect) + ) + + def session_config_options(response: object) -> tuple[SessionConfigOption, ...] | None: """Return a response's complete, typed ACP configuration catalog.""" options = getattr(response, "config_options", None) @@ -126,18 +134,13 @@ async def apply_session_config_selections( option = next((entry for entry in catalog if entry.id == option_id), None) if not isinstance(option, SessionConfigOptionSelect): - select_ids = tuple( - entry.id - for entry in catalog - if isinstance(entry, SessionConfigOptionSelect) - ) raise ACPConfigError( session_id=session_id, option_id=option_id, selected_value=selected_value, message=( f'ACP session offers no config option "{option_id}"; ' - f"available: {listing(select_ids)}." + f"available: {listing(select_ids(catalog))}." ), ) @@ -174,10 +177,10 @@ async def apply_session_config_selections( ), ) from error except Exception as error: - # A JSON-RPC error is the agent's answer; anything else never got one. + # A JSON-RPC error or a malformed reply is still the agent's answer. error_type = ( ACPConfigError - if isinstance(error, RequestError) + if isinstance(error, (RequestError, ValidationError)) else ACPConfigUnreachableError ) raise error_type( diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index 9f444ef28..ba2ce6e1f 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -173,8 +173,7 @@ def test_tcp_with_auth_warns_before_rejection( assert any("ignored over TCP" in r.message for r in caplog.records) -# Copilot CLI 1.0.89's efforts per model (probed live): selecting a model -# replaces the effort select, or drops it for a model without efforts. +# Copilot CLI 1.0.89's efforts per model (probed live). COPILOT_EFFORTS = { "claude-sonnet-5": ("low", "medium", "high", "xhigh", "max"), "gpt-5.4": ("none", "low", "medium", "high", "xhigh"), @@ -305,14 +304,14 @@ async def test_a_live_room_switches_against_its_current_catalog(self) -> None: assert agent.config_selections() == [("model", "claude-haiku-4.5")] @pytest.mark.asyncio - async def test_a_partly_applied_switch_leaves_the_room_on_what_the_agent_runs( + async def test_a_partly_applied_switch_does_not_carry_to_later_sessions( self, ) -> None: - # The model lands before the effort is refused; the room's next - # session must start on it, not on the pre-switch model. + # The model lands before the effort is refused; the switch raised, so + # the room's next session starts where it would have without it. agent = copilot() - async with copilot_room(agent) as session: + async with copilot_room(agent, reasoning_effort="high") as session: await session.send("Hello") with pytest.raises(BandConfigError): await switch_room(session, model="gpt-5.4", reasoning_effort="max") @@ -320,10 +319,26 @@ async def test_a_partly_applied_switch_leaves_the_room_on_what_the_agent_runs( await session.send("Again") assert agent.config_selections("fake-session-2") == [ - ("model", "gpt-5.4"), - ("reasoning_effort", "medium"), + ("reasoning_effort", "high") ] + @pytest.mark.asyncio + async def test_an_empty_switch_leaves_the_room_on_its_configured_selection( + self, + ) -> None: + agent = copilot() + + async with copilot_room(agent, model="gpt-5.4") as session: + await session.send("Hello") + await agent.selects_on_its_own( + agent.session_ids()[0], "model", "claude-haiku-4.5" + ) + await switch_room(session) + await session.adapter.on_cleanup("room-1") + await session.send("Again") + + assert agent.config_selections("fake-session-2") == [("model", "gpt-5.4")] + @pytest.mark.asyncio async def test_a_switch_follows_a_model_the_agent_chose_itself(self) -> None: agent = copilot() @@ -344,6 +359,8 @@ async def pick_haiku(fake: FakeACPAgent, session_id: str) -> None: @pytest.mark.asyncio async def test_concurrent_switches_on_a_room_apply_in_call_order(self) -> None: + # Only gpt-5.4 offers "none", so the second switch is accepted only + # when checked against the catalog the first one left. agent = copilot() async with copilot_room(agent) as session: @@ -351,11 +368,14 @@ async def test_concurrent_switches_on_a_room_apply_in_call_order(self) -> None: gate = agent.holds_config_replies() first = asyncio.create_task(switch_room(session, model="gpt-5.4")) await gate.received.wait() - second = asyncio.create_task(switch_room(session, model="claude-sonnet-5")) + second = asyncio.create_task(switch_room(session, reasoning_effort="none")) gate.release.set() await asyncio.gather(first, second) - assert agent.current_value("model") == "claude-sonnet-5" + assert agent.config_selections() == [ + ("model", "gpt-5.4"), + ("reasoning_effort", "none"), + ] @pytest.mark.asyncio @pytest.mark.parametrize( diff --git a/tests/e2e/baseline/README.md b/tests/e2e/baseline/README.md index 6d9ab35a1..36861ec1b 100644 --- a/tests/e2e/baseline/README.md +++ b/tests/e2e/baseline/README.md @@ -573,7 +573,7 @@ the `dev` extra but are split out for isolation. | `core` | `dev` | anthropic, claude_sdk, agno, langgraph, pydantic_ai, copilot_sdk | provider keys (secrets); copilot_sdk self-downloads its CLI runtime and uses Anthropic BYOK without GitHub auth | | `crewai` | `dev-crewai` | crewai, crewai_flow | provider keys; isolated venv (crewai conflicts with `dev`'s deps — `pyproject.toml [tool.uv] conflicts`) | | `google` | `dev` | gemini, google_adk | provider keys; split from `core` so Google free-tier rate-limit flakiness is isolated | -| `backends` | `dev` | codex, opencode, copilot_acp | the CLI/server coding agents in one job: the `codex` CLI + login + a disposable `CODEX_CWD` (+ the codex-acp e2e), a running `opencode serve` (`OPENCODE_BASE_URL`, gating `bash` to `ask` → `E2E_OPENCODE_BASH_ASKS`), and the `copilot` CLI (`Dep.COPILOT_CLI` + `Dep.ANTHROPIC` — Anthropic BYOK; optional `GITHUB_TOKEN` for the Copilot-hosted auth smoke) | +| `backends` | `dev` | codex, opencode, copilot_acp | the CLI/server coding agents in one job: the `codex` CLI + login + a disposable `CODEX_CWD` (+ the codex-acp e2e), a running `opencode serve` (`OPENCODE_BASE_URL`, gating `bash` to `ask` → `E2E_OPENCODE_BASH_ASKS`), and the `copilot` CLI (`Dep.COPILOT_CLI` + `Dep.ANTHROPIC` — Anthropic BYOK; optional `GITHUB_TOKEN` for the Copilot-hosted smokes) | | `letta` | `dev` | letta | a self-hosted Letta server (docker — `.github/scripts/setup-letta.sh`); the adapter self-hosts its Band MCP server inside pytest (see "Letta lane" below). **Linux-only** (`LINUX_ONLY_LANES`) — no Windows cells | | `parlant` | `dev-parlant` | *(none — parlant is a bespoke smoke, not a registered matrix adapter; pinned via `@lane(Lane.PARLANT)`)* | provider keys; isolated venv (parlant's `griffe`/`griffelib` transitive deps collide with pydantic_ai's — `pyproject.toml [tool.uv] conflicts`); no server setup — the smoke spins up its own in-process Parlant server | diff --git a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py index 5c4489cd8..1a445c305 100644 --- a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py +++ b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py @@ -205,9 +205,7 @@ async def test_copilot_hosted_auth_replies( The matrix cells run Anthropic BYOK to spare the monthly Copilot-hosted quota, but the hosted path is the one production users run — one cheap - turn per cell keeps it proven. Skips (not fails) without a token: hosted - auth is optional extra coverage, the BYOK cells are the lane's bar. Only - hosted sessions advertise model/effort selects, so the typed-effort cell + turn per cell keeps it proven. Only hosted sessions advertise model/effort selects, so the typed-effort cell proves a typed selection is accepted against Copilot's live catalog. """ from band.adapters.copilot_acp import ( # noqa: PLC0415 -- copilot_acp imports the acp (agent-client-protocol) extra at its own top level; not installed in every lane's venv @@ -257,7 +255,7 @@ async def test_copilot_hosted_auth_replies( pytest.param( False, {"model": "not-a-model"}, - "ACP session advertises no model option.", + "ACP session advertises no model option; available:", marks=requires(Dep.ANTHROPIC), id="byok-model", ), diff --git a/tests/integrations/acp/acp_toolkit/agent.py b/tests/integrations/acp/acp_toolkit/agent.py index ba3365ade..5fd84a306 100644 --- a/tests/integrations/acp/acp_toolkit/agent.py +++ b/tests/integrations/acp/acp_toolkit/agent.py @@ -49,6 +49,10 @@ ["FakeACPAgent", str, str, str], Awaitable[Sequence[SessionConfigOption]] ] +# The option ids Copilot CLI publishes its model and effort selects under. +MODEL_OPTION_ID = "model" +EFFORT_OPTION_ID = "reasoning_effort" + @dataclass class ReplyGate: @@ -148,7 +152,10 @@ def advertises_models( def catalog() -> list[SessionConfigOption]: options: list[SessionConfigOption] = [ select_option( - "model", active["model"], efforts_by_model, category=MODEL_CATEGORY + MODEL_OPTION_ID, + active["model"], + efforts_by_model, + category=MODEL_CATEGORY, ) ] efforts = efforts_by_model[active["model"]] @@ -157,7 +164,7 @@ def catalog() -> list[SessionConfigOption]: active["effort"] = default_effort options.append( select_option( - "reasoning_effort", + EFFORT_OPTION_ID, active["effort"], efforts, category=THOUGHT_LEVEL_CATEGORY, @@ -169,7 +176,7 @@ def catalog() -> list[SessionConfigOption]: async def select( fake: FakeACPAgent, session_id: str, option_id: str, value: str ) -> list[SessionConfigOption]: - if option_id == "model": + if option_id == MODEL_OPTION_ID: active["model"] = value elif value in efforts_by_model[active["model"]]: active["effort"] = value diff --git a/tests/integrations/acp/test_client_runtime.py b/tests/integrations/acp/test_client_runtime.py index d7f97f694..ea2d73b41 100644 --- a/tests/integrations/acp/test_client_runtime.py +++ b/tests/integrations/acp/test_client_runtime.py @@ -841,7 +841,8 @@ async def test_create_session_and_prompt_use_active_connection(self) -> None: mock_conn.prompt.assert_awaited_once() @pytest.mark.asyncio - async def test_a_stopped_runtime_keeps_no_session_catalog(self) -> None: + @pytest.mark.parametrize("ended_by", ["stop", "close_session"]) + async def test_an_ended_session_keeps_no_catalog(self, ended_by: str) -> None: # Session ids belong to the agent process; a respawn must not reuse # what the stopped process advertised. spawn = FakeSpawn() @@ -856,7 +857,11 @@ async def test_a_stopped_runtime_keeps_no_session_catalog(self) -> None: await runtime.create_session_response(cwd="/tmp", mcp_servers=[]) advertised = runtime.config_options("sess-1") - await runtime.stop() + match ended_by: + case "stop": + await runtime.stop() + case "close_session": + await runtime.close_session("sess-1") assert [option.id for option in advertised] == ["model"] assert runtime.config_options("sess-1") == () diff --git a/tests/integrations/acp/test_session_config.py b/tests/integrations/acp/test_session_config.py index f60eebb17..40838249d 100644 --- a/tests/integrations/acp/test_session_config.py +++ b/tests/integrations/acp/test_session_config.py @@ -151,6 +151,12 @@ async def test_fails_when_a_prior_selection_removes_a_later_effort_option( ("failure", "raised"), [ pytest.param(RequestError.invalid_params(), ACPConfigError, id="refused"), + # The acp client validates each reply; a null one fails that. + pytest.param( + lambda *_: SetSessionConfigOptionResponse.model_validate({}), + ACPConfigError, + id="malformed-reply", + ), pytest.param( RuntimeError("Connection closed"), ACPConfigUnreachableError, @@ -159,7 +165,7 @@ async def test_fails_when_a_prior_selection_removes_a_later_effort_option( ], ) async def test_a_failed_set_says_whether_the_agent_answered( - self, failure: Exception, raised: type[ACPConfigError] + self, failure: object, raised: type[ACPConfigError] ) -> None: with pytest.raises(ACPConfigError) as rejected: await apply_session_config_selections( @@ -655,6 +661,6 @@ async def test_a_model_is_refused_when_the_agent_advertises_no_model_option( reply = await session.send("Hello") assert reply.errors == [ - f"{CONFIG_FAILURE_PREFIX}ACP session advertises no model option." + f"{CONFIG_FAILURE_PREFIX}ACP session advertises no model option; available: (none)." ] assert agent.prompt_texts() == [] From 0b76aeb2d877f4383e72f3c1ed6051457453a187 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 15:04:47 +0300 Subject: [PATCH 15/17] refactor(acp): read each model step from the runtime's catalog - apply_model_selection reads the runtime's live catalog before each setting instead of threading its own copy, and returns nothing: the runtime is the one record, including updates pushed between steps. - find_select replaces four copies of the select-by-id lookup. - A config set with no reply is logged once, by the turn's failure report, not a second time as a "switched model" that may never have been switched. - Tests reuse the toolkit: the SDK's CONFIG_FAILURE_PREFIX in the E2E smoke, the fake's option-id constants, AcpSession.session_id, and tests/test_agent.py's mock_runtime for the start-time selection check. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- src/band/integrations/acp/client_adapter.py | 8 +--- src/band/integrations/acp/model_selection.py | 18 ++++--- src/band/integrations/acp/session_config.py | 22 +++++++-- tests/adapters/test_copilot_acp_adapter.py | 47 ++++++++++-------- tests/core/test_agent_lifecycle.py | 48 ++----------------- .../smoke/adapters/test_copilot_acp.py | 12 ++--- tests/integrations/acp/acp_toolkit/agent.py | 13 ++--- tests/integrations/acp/test_client_runtime.py | 3 +- tests/integrations/acp/test_session_config.py | 8 +--- tests/test_agent.py | 39 +++++++++++++++ 10 files changed, 110 insertions(+), 108 deletions(-) diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index 7b8df5885..bd07722c2 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -15,6 +15,7 @@ ) from contextlib import asynccontextmanager, suppress from dataclasses import dataclass +from functools import partial from typing import Any, ClassVar, TypeAlias from uuid import uuid4 @@ -1148,11 +1149,6 @@ async def _apply_room_selection( try: await self._apply_model_selection(runtime, session_id, selection) except ACPConfigUnreachableError: - logger.warning( - "Room %s's switched model %s got no answer; keeping it", - room_id, - selection, - ) raise except ACPConfigError: # A remembered switch the agent no longer accepts must fail this @@ -1170,7 +1166,7 @@ async def _apply_model_selection( ) -> None: await apply_model_selection( session_id=session_id, - config_options=runtime.config_options(session_id), + catalog=partial(runtime.config_options, session_id), selection=selection, locate=self.locate_model_options, set_option=_config_setter(runtime), diff --git a/src/band/integrations/acp/model_selection.py b/src/band/integrations/acp/model_selection.py index c730bda2e..a598ba679 100644 --- a/src/band/integrations/acp/model_selection.py +++ b/src/band/integrations/acp/model_selection.py @@ -88,31 +88,29 @@ def locate_model_options(options: Sequence[SessionConfigOption]) -> ACPModelOpti async def apply_model_selection( *, session_id: str, - config_options: Sequence[SessionConfigOption], + catalog: Callable[[], Sequence[SessionConfigOption]], selection: ModelSelection, locate: Callable[[Sequence[SessionConfigOption]], ACPModelOptions], set_option: SessionConfigSetter, -) -> tuple[SessionConfigOption, ...]: - """Select the model, then the effort from the catalog that model returns. +) -> None: + """Select the model, then the effort, each against the session's catalog + as it stands then. Choosing a model can add, remove, or replace the effort select, so the effort is located and checked only after the model has been applied. - Returns the catalog after the last change. """ - options = tuple(config_options) for setting in ModelSetting: value = selection.value_of(setting) if value is None: continue - options = await _apply_setting( + await _apply_setting( session_id=session_id, - config_options=options, + config_options=tuple(catalog()), setting=setting, value=value, locate=locate, set_option=set_option, ) - return options async def _apply_setting( @@ -123,7 +121,7 @@ async def _apply_setting( value: str, locate: Callable[[Sequence[SessionConfigOption]], ACPModelOptions], set_option: SessionConfigSetter, -) -> tuple[SessionConfigOption, ...]: +) -> None: located = locate(config_options) option = located.option_for(setting) option_id = option.id if option is not None else setting @@ -149,7 +147,7 @@ async def _apply_setting( f"{listing(select_ids(config_options))}." ), ) - return await apply_session_config_selections( + await apply_session_config_selections( session_id=session_id, config_options=config_options, selections={option.id: value}, diff --git a/src/band/integrations/acp/session_config.py b/src/band/integrations/acp/session_config.py index 40b3d904e..a39bef14c 100644 --- a/src/band/integrations/acp/session_config.py +++ b/src/band/integrations/acp/session_config.py @@ -86,6 +86,20 @@ def select_values(option: SessionConfigOptionSelect) -> tuple[str, ...]: return tuple(entry.value for entry in flatten_select_options(option.options)) +def find_select( + options: Sequence[SessionConfigOption], option_id: str +) -> SessionConfigOptionSelect | None: + """The select option with ``option_id`` in an ACP catalog, if any.""" + return next( + ( + option + for option in options + if isinstance(option, SessionConfigOptionSelect) and option.id == option_id + ), + None, + ) + + def select_ids(options: Sequence[SessionConfigOption]) -> tuple[str, ...]: """The ids of the select options in an ACP catalog.""" return tuple( @@ -132,8 +146,8 @@ async def apply_session_config_selections( message=f'ACP config value for option "{option_id}" must be a string.', ) - option = next((entry for entry in catalog if entry.id == option_id), None) - if not isinstance(option, SessionConfigOptionSelect): + option = find_select(catalog, option_id) + if option is None: raise ACPConfigError( session_id=session_id, option_id=option_id, @@ -225,8 +239,8 @@ def refreshed_catalog( selected_value=selected_value, message=f'ACP config option "{option_id}" returned a malformed catalog.', ) - selected_option = next((entry for entry in catalog if entry.id == option_id), None) - if not isinstance(selected_option, SessionConfigOptionSelect): + selected_option = find_select(catalog, option_id) + if selected_option is None: raise ACPConfigError( session_id=session_id, option_id=option_id, diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index ba2ce6e1f..00cc3b3f5 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -34,7 +34,11 @@ CONFIG_FAILURE_PREFIX, ACPConfigRequest, ) -from tests.integrations.acp.acp_toolkit.agent import FakeACPAgent +from tests.integrations.acp.acp_toolkit.agent import ( + EFFORT_OPTION_ID, + MODEL_OPTION_ID, + FakeACPAgent, +) from tests.integrations.acp.acp_toolkit.harness import AcpSession, started_acp_adapter @@ -223,8 +227,8 @@ async def test_model_then_effort_are_set_before_the_first_prompt( assert reply.texts == ["Configured"] assert agent.config_selections() == [ - ("model", "gpt-5.4"), - ("reasoning_effort", "high"), + (MODEL_OPTION_ID, "gpt-5.4"), + (EFFORT_OPTION_ID, "high"), ] @pytest.mark.asyncio @@ -242,7 +246,7 @@ async def test_a_restored_session_is_configured_too(self) -> None: assert reply.texts == ["Configured"] assert agent.config_option_requests == [ - ("persisted-session", "model", "gpt-5.4") + ("persisted-session", MODEL_OPTION_ID, "gpt-5.4") ] @pytest.mark.asyncio @@ -258,13 +262,13 @@ async def test_a_restored_session_is_configured_too(self) -> None: ), pytest.param( {"model": "claude-haiku-4.5", "reasoning_effort": "high"}, - [("model", "claude-haiku-4.5")], + [(MODEL_OPTION_ID, "claude-haiku-4.5")], 'model "claude-haiku-4.5" offers no reasoning effort', id="model-without-efforts", ), pytest.param( {"model": "gpt-5.4", "reasoning_effort": "max"}, - [("model", "gpt-5.4")], + [(MODEL_OPTION_ID, "gpt-5.4")], 'reasoning effort "max" is not advertised for model "gpt-5.4"; ' "available: none, low, medium, high, xhigh", id="effort-the-model-lacks", @@ -301,7 +305,7 @@ async def test_a_live_room_switches_against_its_current_catalog(self) -> None: assert str(rejected.value) == ( 'model "claude-haiku-4.5" offers no reasoning effort' ) - assert agent.config_selections() == [("model", "claude-haiku-4.5")] + assert agent.config_selections() == [(MODEL_OPTION_ID, "claude-haiku-4.5")] @pytest.mark.asyncio async def test_a_partly_applied_switch_does_not_carry_to_later_sessions( @@ -318,9 +322,7 @@ async def test_a_partly_applied_switch_does_not_carry_to_later_sessions( await session.adapter.on_cleanup("room-1") await session.send("Again") - assert agent.config_selections("fake-session-2") == [ - ("reasoning_effort", "high") - ] + assert agent.config_selections("fake-session-2") == [(EFFORT_OPTION_ID, "high")] @pytest.mark.asyncio async def test_an_empty_switch_leaves_the_room_on_its_configured_selection( @@ -331,13 +333,15 @@ async def test_an_empty_switch_leaves_the_room_on_its_configured_selection( async with copilot_room(agent, model="gpt-5.4") as session: await session.send("Hello") await agent.selects_on_its_own( - agent.session_ids()[0], "model", "claude-haiku-4.5" + session.session_id("room-1"), MODEL_OPTION_ID, "claude-haiku-4.5" ) await switch_room(session) await session.adapter.on_cleanup("room-1") await session.send("Again") - assert agent.config_selections("fake-session-2") == [("model", "gpt-5.4")] + assert agent.config_selections("fake-session-2") == [ + (MODEL_OPTION_ID, "gpt-5.4") + ] @pytest.mark.asyncio async def test_a_switch_follows_a_model_the_agent_chose_itself(self) -> None: @@ -345,7 +349,9 @@ async def test_a_switch_follows_a_model_the_agent_chose_itself(self) -> None: @agent.on_prompt async def pick_haiku(fake: FakeACPAgent, session_id: str) -> None: - await fake.selects_on_its_own(session_id, "model", "claude-haiku-4.5") + await fake.selects_on_its_own( + session_id, MODEL_OPTION_ID, "claude-haiku-4.5" + ) await fake.say(session_id, "Switched") async with copilot_room(agent) as session: @@ -373,8 +379,8 @@ async def test_concurrent_switches_on_a_room_apply_in_call_order(self) -> None: await asyncio.gather(first, second) assert agent.config_selections() == [ - ("model", "gpt-5.4"), - ("reasoning_effort", "none"), + (MODEL_OPTION_ID, "gpt-5.4"), + (EFFORT_OPTION_ID, "none"), ] @pytest.mark.asyncio @@ -402,8 +408,8 @@ async def test_a_switch_outlives_the_session_it_was_made_on( await session.send("Again", bootstrap=history is not None, history=history) assert agent.config_selections(next_session) == [ - ("model", "gpt-5.4"), - ("reasoning_effort", "high"), + (MODEL_OPTION_ID, "gpt-5.4"), + (EFFORT_OPTION_ID, "high"), ] @pytest.mark.asyncio @@ -424,7 +430,7 @@ async def test_a_switch_to_a_model_without_efforts_drops_the_configured_effort( assert reply.errors == [] assert agent.config_selections("fake-session-2") == [ - ("model", "claude-haiku-4.5") + (MODEL_OPTION_ID, "claude-haiku-4.5") ] @pytest.mark.asyncio @@ -468,7 +474,10 @@ async def test_a_remembered_switch_survives_a_set_the_agent_never_answered( await session.send("And again") assert "did not respond" in timed_out.errors[0] - assert agent.config_selections("fake-session-3")[0] == ("model", "gpt-5.4") + assert agent.config_selections("fake-session-3")[0] == ( + MODEL_OPTION_ID, + "gpt-5.4", + ) @pytest.mark.asyncio async def test_a_switch_refused_before_any_change_leaves_the_room_unpinned( diff --git a/tests/core/test_agent_lifecycle.py b/tests/core/test_agent_lifecycle.py index 7971191b4..e284b1754 100644 --- a/tests/core/test_agent_lifecycle.py +++ b/tests/core/test_agent_lifecycle.py @@ -7,23 +7,18 @@ from __future__ import annotations -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import AsyncMock import pytest from band.agent import Agent -from band.core.model_catalog import ModelSelection, ModelSelectionError -from tests.catalogs import CatalogAdapter -def make_agent(adapter: object, *, started: bool = True) -> Agent: +def make_agent(adapter: object) -> Agent: runtime = AsyncMock() runtime.stop.return_value = True - runtime.feature_flags = None - runtime.claim_single_instance = MagicMock() - runtime.release_single_instance = MagicMock() agent = Agent(runtime=runtime, adapter=adapter) # type: ignore[arg-type] - agent._started = started + agent._started = True return agent @@ -42,43 +37,6 @@ async def test_runtime_start_failure_rolls_back_adapter(self): adapter.cleanup_all.assert_awaited_once() -class TestStartValidatesModelSelection: - @pytest.mark.asyncio - async def test_an_advertised_selection_starts_after_on_started_lists_it( - self, - ) -> None: - agent = make_agent( - CatalogAdapter(ModelSelection(model="sonnet", reasoning_effort="high")), - started=False, - ) - - await agent.start() - - agent._runtime.start.assert_awaited_once() - - @pytest.mark.asyncio - async def test_a_rejected_selection_fails_start_before_connecting(self) -> None: - adapter = CatalogAdapter(ModelSelection(model="gpt-9")) - agent = make_agent(adapter, started=False) - - with pytest.raises(ModelSelectionError, match='model "gpt-9"'): - await agent.start() - - agent._runtime.start.assert_not_awaited() - assert adapter.cleaned_up - - @pytest.mark.asyncio - async def test_a_failing_release_does_not_mask_the_rejection( - self, caplog: pytest.LogCaptureFixture - ) -> None: - adapter = CatalogAdapter(ModelSelection(model="gpt-9"), cleanup_fails=True) - - with pytest.raises(ModelSelectionError, match='model "gpt-9"'): - await make_agent(adapter, started=False).start() - - assert "Adapter cleanup_all failed" in caplog.text - - class TestStopCleansUpAdapter: @pytest.mark.asyncio async def test_stop_calls_adapter_cleanup_all(self): diff --git a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py index 1a445c305..e28152033 100644 --- a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py +++ b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py @@ -37,9 +37,6 @@ # -- that vocabulary answers a different question ("is this tool observational, # not terminal work, for no-reply detection"), which only coincides with this one today. BAND_EVENT_TOOL_NAME = "band_send_event" -# Room text for a refused typed selection. Literal: importing the SDK's -# constant would need the acp extra at collection time. -CONFIG_FAILURE_PREFIX = "ACP session configuration failed: " @with_adapters(Adapter.COPILOT_ACP, **TOOL_AGENT) @@ -172,13 +169,11 @@ def hermetic_copilot_config( "cwd": str(work_dir), "custom_section": "Keep responses short and concise.", "env": (hosted_env if hosted else copilot_acp_env(settings, home)), + "model": model, + "reasoning_effort": reasoning_effort, } if hosted: kwargs["github_token"] = settings.backends.github_token - if model is not None: - kwargs["model"] = model - if reasoning_effort is not None: - kwargs["reasoning_effort"] = reasoning_effort if settings.backends.copilot_command.strip(): kwargs["command"] = tuple(settings.backends.copilot_command.split()) return CopilotACPAdapterConfig(**kwargs) @@ -284,6 +279,9 @@ async def test_copilot_turn_fails_loudly_on_an_unadvertised_selection( from band.adapters.copilot_acp import ( # noqa: PLC0415 -- copilot_acp imports the acp (agent-client-protocol) extra at its own top level; not installed in every lane's venv CopilotACPAdapter, ) + from band.integrations.acp.session_config import ( # noqa: PLC0415 -- same acp extra as above + CONFIG_FAILURE_PREFIX, + ) if hosted: skip_without_github_token(baseline_settings) diff --git a/tests/integrations/acp/acp_toolkit/agent.py b/tests/integrations/acp/acp_toolkit/agent.py index 5fd84a306..61549ed45 100644 --- a/tests/integrations/acp/acp_toolkit/agent.py +++ b/tests/integrations/acp/acp_toolkit/agent.py @@ -42,7 +42,7 @@ MODEL_CATEGORY, THOUGHT_LEVEL_CATEGORY, ) -from band.integrations.acp.session_config import SessionConfigOption +from band.integrations.acp.session_config import SessionConfigOption, find_select PromptHandler = Callable[["FakeACPAgent", str], Awaitable[None]] ConfigOptionHandler = Callable[ @@ -514,15 +514,8 @@ async def _select( def current_value(self, option_id: str) -> str | None: """The value the agent currently has selected for ``option_id``.""" - return next( - ( - option.current_value - for option in self._config_options - if option.id == option_id - and isinstance(option, SessionConfigOptionSelect) - ), - None, - ) + option = find_select(self._config_options, option_id) + return option.current_value if option is not None else None async def close_session(self, session_id: str, **kwargs: Any) -> None: """Record that the client closed a session before prompting it.""" diff --git a/tests/integrations/acp/test_client_runtime.py b/tests/integrations/acp/test_client_runtime.py index ea2d73b41..12fae5db6 100644 --- a/tests/integrations/acp/test_client_runtime.py +++ b/tests/integrations/acp/test_client_runtime.py @@ -27,6 +27,7 @@ select_allow_option_id, tcp_spawn_process, ) +from band.integrations.acp.session_config import select_ids from band.integrations.acp.types import ChunkType, CollectedChunk from tests.integrations.acp.acp_toolkit import FakeSpawn, select_option @@ -863,7 +864,7 @@ async def test_an_ended_session_keeps_no_catalog(self, ended_by: str) -> None: case "close_session": await runtime.close_session("sess-1") - assert [option.id for option in advertised] == ["model"] + assert select_ids(advertised) == ("model",) assert runtime.config_options("sess-1") == () @pytest.mark.asyncio diff --git a/tests/integrations/acp/test_session_config.py b/tests/integrations/acp/test_session_config.py index 40838249d..0c858d79d 100644 --- a/tests/integrations/acp/test_session_config.py +++ b/tests/integrations/acp/test_session_config.py @@ -30,6 +30,7 @@ ACPConfigUnreachableError, SessionConfigOption, apply_session_config_selections, + find_select, ) from tests.integrations.acp.acp_toolkit import ( FakeACPAgent, @@ -518,13 +519,8 @@ class ThinkingIdAdapter(ACPClientAdapter): def locate_model_options( self, options: Sequence[SessionConfigOption] ) -> ACPModelOptions: - selects = { - option.id: option - for option in options - if isinstance(option, SessionConfigOptionSelect) - } return ACPModelOptions( - model=selects.get("model"), effort=selects.get("thinking") + model=find_select(options, "model"), effort=find_select(options, "thinking") ) diff --git a/tests/test_agent.py b/tests/test_agent.py index 8e6156ea6..23e62d78b 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -12,6 +12,7 @@ ParticipantAddedPayload, RoomAddedPayload, ) +from band.core.model_catalog import ModelSelection, ModelSelectionError from band.core.simple_adapter import SimpleAdapter from band.core.types import AgentInput, Capability from band.platform.event import MessageEvent, ParticipantAddedEvent, RoomAddedEvent @@ -19,6 +20,7 @@ from band.runtime.capabilities import FeatureFlag from band.runtime.types import AgentConfig, ConversationContext, SessionConfig from band.testing.platform import platform_connection_stub +from tests.catalogs import CatalogAdapter @pytest.fixture @@ -478,6 +480,43 @@ async def test_files_capability_kept_when_flag_on(self, mock_runtime): assert Capability.FILES in adapter.features.capabilities +class TestModelSelectionCheckOnStart: + """Agent.start() refuses a model selection the adapter's catalog lacks.""" + + @pytest.mark.asyncio + async def test_an_advertised_selection_starts(self, mock_runtime): + adapter = CatalogAdapter( + ModelSelection(model="sonnet", reasoning_effort="high") + ) + + await Agent(runtime=mock_runtime, adapter=adapter).start() + + mock_runtime.start.assert_awaited_once() + + @pytest.mark.asyncio + async def test_a_rejected_selection_fails_start_before_connecting( + self, mock_runtime + ): + adapter = CatalogAdapter(ModelSelection(model="gpt-9")) + + with pytest.raises(ModelSelectionError, match='model "gpt-9"'): + await Agent(runtime=mock_runtime, adapter=adapter).start() + + mock_runtime.start.assert_not_awaited() + assert adapter.cleaned_up + + @pytest.mark.asyncio + async def test_a_failing_release_does_not_mask_the_rejection( + self, mock_runtime, caplog: pytest.LogCaptureFixture + ): + adapter = CatalogAdapter(ModelSelection(model="gpt-9"), cleanup_fails=True) + + with pytest.raises(ModelSelectionError, match='model "gpt-9"'): + await Agent(runtime=mock_runtime, adapter=adapter).start() + + assert "Adapter cleanup_all failed" in caplog.text + + class TestDefaultPreprocessorIntegration: """Tests for DefaultPreprocessor behavior.""" From 11a4577980268bce388d97fbb0225f2bb565d45d Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 15:19:23 +0300 Subject: [PATCH 16/17] test(e2e): prove a live Copilot model and effort switch A room's live session is switched to gpt-5.4 / low with apply_model_selection and must keep replying. A fresh copilot process then loads the session and must report exactly those values, so the check is Copilot's own record: a switch that only updated the adapter, or an effort Copilot silently dropped, fails (verified by skipping the effort send). Band tools stay off because a hosted turn with them keeps calling band_send_message and never ends. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- AGENTS.md | 2 +- .../smoke/adapters/test_copilot_acp.py | 131 +++++++++++++++++- 2 files changed, 130 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 29eb45537..f2a355f55 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -223,7 +223,7 @@ agent keys and platform URLs should stay aligned with `.env.test` / - `GOOGLE_API_KEY`: Google API key for Gemini Developer API (for Gemini/Google ADK examples) - `GOOGLE_GENAI_USE_VERTEXAI`: Set to `true` to use Vertex AI instead of Gemini Developer API - `GOOGLE_CLOUD_PROJECT`: Google Cloud project ID (required when using Vertex AI) -- `GITHUB_TOKEN`: A Copilot-entitled GitHub token. The baseline `copilot_sdk` and `copilot_acp` builders use Anthropic BYOK and never read it; the only baseline readers are the Copilot-hosted smokes (`test_copilot_hosted_auth_replies` and the `hosted-effort` cell of `test_copilot_turn_fails_loudly_on_an_unadvertised_selection`, which skip when unset). Also used by Copilot-hosted examples outside the baseline; optional when a stored `copilot login` is present. +- `GITHUB_TOKEN`: A Copilot-entitled GitHub token. The baseline `copilot_sdk` and `copilot_acp` builders use Anthropic BYOK and never read it; the only baseline readers are the Copilot-hosted smokes (`test_copilot_hosted_auth_replies`, `test_copilot_switches_model_and_effort_in_a_live_room`, and the `hosted-effort` cell of `test_copilot_turn_fails_loudly_on_an_unadvertised_selection`, which skip when unset). Also used by Copilot-hosted examples outside the baseline; optional when a stored `copilot login` is present. - `E2E_TESTS_ENABLED`: Set to `true` to enable E2E tests (default: disabled) - `E2E_LLM_MODEL`: OpenAI model for E2E tests (default: `gpt-5.4-mini`) - `E2E_ANTHROPIC_MODEL`: Anthropic model for E2E tests (default: `claude-haiku-4-5` — the baseline judge uses structured outputs, which older Haiku models do not support) diff --git a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py index e28152033..d89085687 100644 --- a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py +++ b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py @@ -13,7 +13,9 @@ import pytest -from band.core.types import MessageType +from band.core.model_catalog import ModelSelection +from band.core.types import MessageType, metadata_to_dict +from band.workspaces import create_room_workspace_resolver from tests.e2e.baseline.agents import Adapter, Lane, lane, with_adapters from tests.e2e.baseline.flaky import flaky_model from tests.e2e.baseline.requires import Dep, requires @@ -24,7 +26,8 @@ unique_marker, ) from tests.e2e.baseline.toolkit.builders import copilot_acp_env, copilot_home_dir -from tests.e2e.baseline.toolkit.capture import CaptureFactory +from tests.e2e.baseline.toolkit.capture import CaptureFactory, ReplyCapture +from tests.e2e.baseline.toolkit.observations.events import Tasks from tests.e2e.baseline.toolkit.provisioning import ( ProvisionedAgent, ResourceManager, @@ -142,6 +145,7 @@ def hermetic_copilot_config( hosted: bool = False, model: str | None = None, reasoning_effort: str | None = None, + inject_band_tools: bool = True, ) -> Any: """A per-test ``CopilotACPAdapterConfig`` with a fresh cwd + ``COPILOT_HOME``. @@ -155,6 +159,8 @@ def hermetic_copilot_config( smoke's billed turns use a cheap, deterministic model instead of Copilot's ``auto`` picker. ``model`` / ``reasoning_effort`` set the typed selection the adapter applies from each session's advertised catalog. + ``inject_band_tools=False`` leaves the agent without Band tools, so the + adapter posts its text as the room reply. """ from band.adapters.copilot_acp import ( # noqa: PLC0415 -- copilot_acp imports the acp (agent-client-protocol) extra at its own top level; not installed in every lane's venv CopilotACPAdapterConfig, @@ -171,6 +177,7 @@ def hermetic_copilot_config( "env": (hosted_env if hosted else copilot_acp_env(settings, home)), "model": model, "reasoning_effort": reasoning_effort, + "inject_band_tools": inject_band_tools, } if hosted: kwargs["github_token"] = settings.backends.github_token @@ -312,6 +319,126 @@ async def test_copilot_turn_fails_loudly_on_an_unadvertised_selection( errors.assert_contains_any([CONFIG_FAILURE_PREFIX + failure]) +def room_session_id(tasks: Tasks, room_id: str) -> str: + """The ACP session id the adapter last reported for ``room_id``.""" + reported = [ + metadata["acp_client_session_id"] + for metadata in (metadata_to_dict(task.metadata) for task in tasks) + if metadata.get("acp_client_room_id") == room_id + ] + assert reported, f"no ACP session reported for room {room_id}" + return reported[-1] + + +async def copilot_persisted_selection( + config: Any, *, room_id: str, session_id: str +) -> ModelSelection: + """What a fresh ``copilot --acp`` reports ``session_id`` runs after + ``session/load``: Copilot's own record, not the adapter's.""" + from band.integrations.acp.client_runtime import ( # noqa: PLC0415 -- the acp extra, as above + ACPRuntime, + ) + from band.integrations.acp.model_selection import ( # noqa: PLC0415 -- the acp extra, as above + locate_model_options, + ) + from band.integrations.acp.session_config import ( # noqa: PLC0415 -- the acp extra, as above + session_config_options, + ) + + workspace = create_room_workspace_resolver(config.cwd)(room_id) + runtime = ACPRuntime( + command=list(config.command), + env={**config.env, "GITHUB_TOKEN": config.github_token}, + cwd=workspace, + ) + await runtime.start() + try: + loaded = await runtime.load_session_response( + cwd=workspace, session_id=session_id, mcp_servers=[] + ) + finally: + await runtime.stop() + assert loaded is not None, f"Copilot could not load session {session_id}" + return locate_model_options( + session_config_options(loaded) or () + ).current_selection() + + +# Both differ from what a hosted ACP session starts on (claude-sonnet-5 / +# medium: the env's COPILOT_MODEL does not pick an ACP session's model), so +# Copilot reporting them proves each was applied. Probed as advertised; an +# account without them fails the switch naming what it offers. +SWITCHED = ModelSelection(model="gpt-5.4", reasoning_effort="low") + + +@lane(Lane.BACKENDS) # bespoke build exposes no framework; pin scheduling to backends +@requires(Dep.COPILOT_CLI) +# Two hosted turns plus a second Copilot CLI boot for the session/load check. +@pytest.mark.timeout(extra=300) +@pytest.mark.asyncio(loop_scope="session") +async def test_copilot_switches_model_and_effort_in_a_live_room( + baseline_settings: BaselineSettings, + resource_manager: ResourceManager, + user_ops: UserOps, + reply_capture: CaptureFactory, + tmp_path: Any, +) -> None: + """A runtime switch changes what Copilot itself runs for the room, and the + room keeps replying on it. + + The check is Copilot's persisted session, read by a fresh process, so a + switch that only updated the adapter's own record, or an effort Copilot + silently ignored, fails. Band tools stay off: with them, a hosted turn + keeps calling band_send_message after its reply and never ends. + """ + from band.adapters.copilot_acp import ( # noqa: PLC0415 -- copilot_acp imports the acp (agent-client-protocol) extra at its own top level; not installed in every lane's venv + CopilotACPAdapter, + ) + + skip_without_github_token(baseline_settings) + + identity = await resource_manager.provision_agent("copilot-live-switch") + room_id = await resource_manager.provision_room( + title="e2e-copilot-live-switch", participants=[identity.id] + ) + config = hermetic_copilot_config( + baseline_settings, tmp_path / "copilot", hosted=True, inject_band_tools=False + ) + adapter = CopilotACPAdapter(config) + + async def reply_with(capture: ReplyCapture, marker: str) -> None: + mark = capture.messages.snapshot() + mid = await user_ops.send_message( + room_id, + f"Reply with one short sentence that includes the marker {marker}.", + mention_id=identity.id, + mention_name=identity.name, + ) + replies = await capture.wait_for_reply( + mid, identity.id, since=mark, deadline_s=baseline_settings.e2e_timeout + ) + replies.assert_contains_any([marker]) + + async with ( + running_agent(identity, adapter, baseline_settings), + reply_capture(room_id) as capture, + ): + await reply_with(capture, unique_marker("before-switch")) + await adapter.apply_model_selection(SWITCHED, room_id=room_id) + # Also the prompt Copilot persists the switched session on. + await reply_with(capture, unique_marker("after-switch")) + session_id = room_session_id( + await capture.tasks(sender_id=identity.id), room_id + ) + + assert ( + await copilot_persisted_selection( + config, room_id=room_id, session_id=session_id + ) + == SWITCHED + ) + + @lane(Lane.BACKENDS) # bespoke build exposes no framework; pin scheduling to backends @requires(Dep.COPILOT_CLI, Dep.ANTHROPIC) # Deliberately no flaky marker: two clean runs so far, and a rerun here would From 41d59d245d0d939e4298c7f4cc2bda5504c93274 Mon Sep 17 00:00:00 2001 From: Alexander Zaikman Date: Wed, 30 Sep 2026 15:54:36 +0300 Subject: [PATCH 17/17] fix(acp): name an ended session as a switch's cause and heal a lost setup - A runtime switch that fails once a cleanup has ended its session raises "ended mid-switch", however the dead runtime answered: a stopped runtime's empty catalog no longer reads as the agent refusing the effort. The same check covers a switch queued behind the cleanup, so the separate pre-check goes. A failed switch logs what its session now runs. - A session-start set that got no reply releases the room's runtime, so a lost connection is replaced on the next turn and the remembered switch is retried instead of every turn failing on the dead one. - Tests: other rooms keep the configured selection, a switch survives an adapter restart, a cleanup at any point of a switch is named, and a lost connection heals (the fake agent can now hang up). - Live smokes: the hosted model is pinned through the typed selection (COPILOT_MODEL does not pick an ACP session's model); hosted auth cells run without Band tools, which kept their turn from ending; the session/load check spawns Copilot through the adapter's own wiring. - CI installs Copilot CLI 1.0.89, the version these facts were probed on. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017o8axcpdExtcLcGfH8UDA3 --- .github/scripts/setup-copilot.sh | 2 +- src/band/integrations/acp/client_adapter.py | 31 +++- src/band/integrations/acp/client_runtime.py | 1 + src/band/integrations/acp/model_selection.py | 22 ++- src/band/integrations/acp/session_config.py | 27 ++-- tests/adapters/test_copilot_acp_adapter.py | 86 ++++++++++- tests/e2e/baseline/settings.py | 10 +- .../smoke/adapters/test_copilot_acp.py | 135 ++++++++++-------- .../integrations/acp/acp_toolkit/__init__.py | 2 + tests/integrations/acp/acp_toolkit/agent.py | 12 ++ tests/integrations/acp/acp_toolkit/harness.py | 6 +- 11 files changed, 231 insertions(+), 103 deletions(-) diff --git a/.github/scripts/setup-copilot.sh b/.github/scripts/setup-copilot.sh index 95fa4f578..7c9ca02aa 100755 --- a/.github/scripts/setup-copilot.sh +++ b/.github/scripts/setup-copilot.sh @@ -16,7 +16,7 @@ set -euo pipefail # Pinned for the same reason as codex: an unpinned global install lets the CLI # float between runs, so a CLI change lands as an unrelated-looking lane # failure. Bump deliberately. -COPILOT_CLI_VERSION="${COPILOT_CLI_VERSION:-1.0.80}" +COPILOT_CLI_VERSION="${COPILOT_CLI_VERSION:-1.0.89}" npm install -g "@github/copilot@${COPILOT_CLI_VERSION}" copilot --version diff --git a/src/band/integrations/acp/client_adapter.py b/src/band/integrations/acp/client_adapter.py index bd07722c2..3f95c9318 100644 --- a/src/band/integrations/acp/client_adapter.py +++ b/src/band/integrations/acp/client_adapter.py @@ -379,9 +379,19 @@ async def apply_model_selection( raise BandConfigError(f"room {room_id} has no live ACP session to switch") session_id, runtime = session async with runtime.config_lock: - # A cleanup may have ended the session while this switch queued. - await self._ensure_session_live(room_id, session) - await self._apply_model_selection(runtime, session_id, selection) + try: + await self._apply_model_selection(runtime, session_id, selection) + except ACPConfigError: + # A cleanup while this switch queued or ran fails it however + # the dead runtime answered; the ended session is the cause. + await self._ensure_session_live(room_id, session) + logger.warning( + "Switching room %s to %s failed; its session runs %s", + room_id, + selection, + self._running_selection(runtime, session_id), + ) + raise await self._ensure_session_live(room_id, session) self._remember_room_selection(room_id, session_id, runtime) @@ -391,12 +401,17 @@ async def _ensure_session_live( if await self._live_session(room_id) != session: raise BandConfigError(f"room {room_id}'s ACP session ended mid-switch") + def _running_selection( + self, runtime: ACPRuntime, session_id: str + ) -> ModelSelection: + return self.locate_model_options( + runtime.config_options(session_id) + ).current_selection() + def _remember_room_selection( self, room_id: str, session_id: str, runtime: ACPRuntime ) -> None: - running = self.locate_model_options( - runtime.config_options(session_id) - ).current_selection() + running = self._running_selection(runtime, session_id) self._room_selections[room_id] = running logger.info( "ACP session %s for room %s now runs %s", session_id, room_id, running @@ -542,6 +557,10 @@ async def on_message( history if is_session_bootstrap else None, ) except ACPConfigError as error: + # An unanswered set may have left a dead connection the runtime + # never replaces; only a fresh runtime lets the next turn retry. + if isinstance(error, ACPConfigUnreachableError): + await self.on_cleanup(room_id) await self._report_config_error(tools, error) return runtime.reset_session(session_id) diff --git a/src/band/integrations/acp/client_runtime.py b/src/band/integrations/acp/client_runtime.py index 3d23be08f..219c0ca1d 100644 --- a/src/band/integrations/acp/client_runtime.py +++ b/src/band/integrations/acp/client_runtime.py @@ -380,6 +380,7 @@ async def session_update( ) -> None: del kwargs if isinstance(update, ConfigOptionUpdate): + logger.debug("ACP session %s pushed new config options", session_id) self.record_config_options(session_id, update.config_options) return async with self._session_lock(session_id): diff --git a/src/band/integrations/acp/model_selection.py b/src/band/integrations/acp/model_selection.py index a598ba679..9dbeb1c13 100644 --- a/src/band/integrations/acp/model_selection.py +++ b/src/band/integrations/acp/model_selection.py @@ -24,6 +24,7 @@ flatten_select_options, select_ids, select_values, + selects, ) # Category ids the ACP spec reserves for these selectors. @@ -65,15 +66,16 @@ def model_catalog(self) -> ModelCatalog | None: ) def _choice(self, entry: SessionConfigSelectOption, *, current: str) -> ModelChoice: - if entry.value != current: - return ModelChoice(id=entry.value, label=entry.name) - if self.effort is None: - return ModelChoice(id=entry.value, label=entry.name, efforts=()) + efforts: tuple[str, ...] | None = None + default_effort: str | None = None + if entry.value == current: + efforts = select_values(self.effort) if self.effort else () + default_effort = self.effort.current_value if self.effort else None return ModelChoice( id=entry.value, label=entry.name, - efforts=select_values(self.effort), - default_effort=self.effort.current_value, + efforts=efforts, + default_effort=default_effort, ) @@ -159,13 +161,7 @@ def _select_in_category( options: Sequence[SessionConfigOption], category: str ) -> SessionConfigOptionSelect | None: return next( - ( - option - for option in options - if isinstance(option, SessionConfigOptionSelect) - and option.category == category - ), - None, + (option for option in selects(options) if option.category == category), None ) diff --git a/src/band/integrations/acp/session_config.py b/src/band/integrations/acp/session_config.py index a39bef14c..88e543aae 100644 --- a/src/band/integrations/acp/session_config.py +++ b/src/band/integrations/acp/session_config.py @@ -3,7 +3,7 @@ from __future__ import annotations import asyncio -from collections.abc import Awaitable, Callable, Mapping, Sequence +from collections.abc import Awaitable, Callable, Iterator, Mapping, Sequence from dataclasses import dataclass from typing import TypeAlias @@ -44,8 +44,7 @@ class ACPConfigRequest: config_options: tuple[SessionConfigOption, ...] -# RuntimeError stays a base so handlers written against earlier releases -# still catch it. +# Also a RuntimeError, so existing ``except RuntimeError`` handlers catch it. class ACPConfigError(BandConfigError, RuntimeError): """A requested ACP session configuration could not be applied.""" @@ -86,25 +85,25 @@ def select_values(option: SessionConfigOptionSelect) -> tuple[str, ...]: return tuple(entry.value for entry in flatten_select_options(option.options)) +def selects( + options: Sequence[SessionConfigOption], +) -> Iterator[SessionConfigOptionSelect]: + """The select options in an ACP catalog.""" + return ( + option for option in options if isinstance(option, SessionConfigOptionSelect) + ) + + def find_select( options: Sequence[SessionConfigOption], option_id: str ) -> SessionConfigOptionSelect | None: """The select option with ``option_id`` in an ACP catalog, if any.""" - return next( - ( - option - for option in options - if isinstance(option, SessionConfigOptionSelect) and option.id == option_id - ), - None, - ) + return next((option for option in selects(options) if option.id == option_id), None) def select_ids(options: Sequence[SessionConfigOption]) -> tuple[str, ...]: """The ids of the select options in an ACP catalog.""" - return tuple( - option.id for option in options if isinstance(option, SessionConfigOptionSelect) - ) + return tuple(option.id for option in selects(options)) def session_config_options(response: object) -> tuple[SessionConfigOption, ...] | None: diff --git a/tests/adapters/test_copilot_acp_adapter.py b/tests/adapters/test_copilot_acp_adapter.py index 00cc3b3f5..02de7ca3b 100644 --- a/tests/adapters/test_copilot_acp_adapter.py +++ b/tests/adapters/test_copilot_acp_adapter.py @@ -39,7 +39,11 @@ MODEL_OPTION_ID, FakeACPAgent, ) -from tests.integrations.acp.acp_toolkit.harness import AcpSession, started_acp_adapter +from tests.integrations.acp.acp_toolkit.harness import ( + DEFAULT_ROOM, + AcpSession, + started_acp_adapter, +) class TestCopilotACPAdapterConstruction: @@ -207,7 +211,7 @@ async def copilot_room(agent: FakeACPAgent, **config: Any) -> AsyncIterator[AcpS async def switch_room(session: AcpSession, **selection: str) -> None: """Switch the live session of the room ``session.send`` talks to.""" await session.adapter.apply_model_selection( - ModelSelection(**selection), room_id="room-1" + ModelSelection(**selection), room_id=DEFAULT_ROOM ) @@ -513,6 +517,84 @@ async def test_a_switch_queued_behind_a_room_cleanup_names_the_ended_session( assert isinstance(results[1], BandConfigError) assert str(results[1]) == "room room-1's ACP session ended mid-switch" + @pytest.mark.asyncio + async def test_a_cleanup_at_any_point_of_a_switch_names_the_ended_session( + self, + ) -> None: + # Sweeps where the cleanup lands: before the model reply, between the + # model and effort steps, and after the switch completes. + outcomes = {await self._switch_cleaned_up_after(ticks) for ticks in range(12)} + + assert outcomes <= {"ok", "room room-1's ACP session ended mid-switch"} + assert "room room-1's ACP session ended mid-switch" in outcomes + + @staticmethod + async def _switch_cleaned_up_after(ticks: int) -> str: + agent = copilot() + async with copilot_room(agent) as session: + await session.send("Hello") + gate = agent.holds_config_replies() + switch = asyncio.create_task( + switch_room(session, model="gpt-5.4", reasoning_effort="high") + ) + await gate.received.wait() + gate.release.set() + for _ in range(ticks): + await asyncio.sleep(0) + await session.adapter.on_cleanup(DEFAULT_ROOM) + (result,) = await asyncio.gather(switch, return_exceptions=True) + return "ok" if result is None else str(result) + + @pytest.mark.asyncio + async def test_a_setup_that_loses_its_connection_retries_on_the_next_turn( + self, + ) -> None: + agent = copilot() + + async with copilot_room(agent) as session: + await session.send("Hello") + await switch_room(session, model="gpt-5.4") + await session.adapter.on_cleanup(DEFAULT_ROOM) + agent.hangs_up_on_next_config_option() + lost = await session.send("Again") + healed = await session.send("And again") + + assert lost.errors == [CONFIG_FAILURE_PREFIX + "Connection closed"] + assert healed.texts == ["Configured"] + assert agent.config_selections("fake-session-3") == [ + (MODEL_OPTION_ID, "gpt-5.4"), + (EFFORT_OPTION_ID, "medium"), + ] + + @pytest.mark.asyncio + async def test_a_switch_leaves_other_rooms_on_the_configured_selection( + self, + ) -> None: + agent = copilot() + + async with copilot_room(agent) as session: + await session.send("Hello") + await switch_room(session, model="gpt-5.4") + await session.send("Hi", room="room-2") + + assert agent.config_selections("fake-session-2") == [] + + @pytest.mark.asyncio + async def test_a_switch_survives_an_adapter_restart(self) -> None: + agent = copilot() + + async with copilot_room(agent) as session: + await session.send("Hello") + await switch_room(session, model="gpt-5.4") + await session.adapter.cleanup_all() + await session.adapter.on_started("Fake Agent", "restarted") + await session.send("Again") + + assert agent.config_selections("fake-session-2") == [ + (MODEL_OPTION_ID, "gpt-5.4"), + (EFFORT_OPTION_ID, "medium"), + ] + @pytest.mark.asyncio async def test_switching_a_room_without_a_live_session_is_refused(self) -> None: async with copilot_room(copilot()) as session: diff --git a/tests/e2e/baseline/settings.py b/tests/e2e/baseline/settings.py index c34474674..589490f04 100644 --- a/tests/e2e/baseline/settings.py +++ b/tests/e2e/baseline/settings.py @@ -177,11 +177,11 @@ class Backends(BaseSettings): # Copilot-hosted auth for the non-BYOK smokes in test_copilot_acp.py; the # BYOK matrix cells never read it. Those smokes skip when unset. github_token: str = "" # GITHUB_TOKEN - # Pins those smokes' model instead of leaving it to Copilot's `auto` - # picker, which can land on an expensive reasoning-tier model (observed: - # gpt-5.6-terra). gpt-5.6-luna is the cheapest model in the GPT-5.6 family - # while still agentic/tool-calling, so their billed turns stay cheap and - # deterministic across runs. + # Those smokes' typed model (see hermetic_copilot_config) rather than + # Copilot's own pick, which can be an expensive reasoning-tier model + # (observed: gpt-5.6-terra). gpt-5.6-luna is the cheapest model in the + # GPT-5.6 family while still agentic/tool-calling, so their billed turns + # stay cheap and deterministic across runs. copilot_hosted_model: str = "gpt-5.6-luna" # COPILOT_HOSTED_MODEL diff --git a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py index d89085687..e1d092cce 100644 --- a/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py +++ b/tests/e2e/baseline/smoke/adapters/test_copilot_acp.py @@ -8,14 +8,15 @@ from __future__ import annotations +from functools import partial from pathlib import Path from typing import Any import pytest +from band.converters.acp_client import ACPClientHistoryConverter from band.core.model_catalog import ModelSelection from band.core.types import MessageType, metadata_to_dict -from band.workspaces import create_room_workspace_resolver from tests.e2e.baseline.agents import Adapter, Lane, lane, with_adapters from tests.e2e.baseline.flaky import flaky_model from tests.e2e.baseline.requires import Dep, requires @@ -154,11 +155,11 @@ def hermetic_copilot_config( ``session/load`` deterministically miss. ``hosted=False`` (default) mirrors the registry builder's Anthropic BYOK env (``copilot_acp_env``); ``hosted=True`` omits it and authenticates with ``github_token`` — the - Copilot-hosted path production users run. The hosted env also pins - ``COPILOT_MODEL`` (``settings.backends.copilot_hosted_model``) so this - smoke's billed turns use a cheap, deterministic model instead of - Copilot's ``auto`` picker. ``model`` / ``reasoning_effort`` set the typed - selection the adapter applies from each session's advertised catalog. + Copilot-hosted path production users run. ``model`` / ``reasoning_effort`` + set the typed selection the adapter applies from each session's + advertised catalog; a hosted ``model`` defaults to + ``settings.backends.copilot_hosted_model``, since the env's + ``COPILOT_MODEL`` does not pick an ACP session's model. ``inject_band_tools=False`` leaves the agent without Band tools, so the adapter posts its text as the room reply. """ @@ -167,14 +168,12 @@ def hermetic_copilot_config( ) home = copilot_home_dir(str(work_dir)) - hosted_env = { - "COPILOT_HOME": home, - "COPILOT_MODEL": settings.backends.copilot_hosted_model, - } + if hosted and model is None: + model = settings.backends.copilot_hosted_model kwargs: dict[str, Any] = { "cwd": str(work_dir), "custom_section": "Keep responses short and concise.", - "env": (hosted_env if hosted else copilot_acp_env(settings, home)), + "env": ({"COPILOT_HOME": home} if hosted else copilot_acp_env(settings, home)), "model": model, "reasoning_effort": reasoning_effort, "inject_band_tools": inject_band_tools, @@ -186,13 +185,35 @@ def hermetic_copilot_config( return CopilotACPAdapterConfig(**kwargs) +async def reply_with( + capture: ReplyCapture, + *, + marker: str, + user_ops: UserOps, + identity: ProvisionedAgent, + settings: BaselineSettings, +) -> None: + """One room turn whose reply must carry ``marker``.""" + mark = capture.messages.snapshot() + mid = await user_ops.send_message( + capture.room_id, + f"Reply with one short sentence that includes the marker {marker}.", + mention_id=identity.id, + mention_name=identity.name, + ) + replies = await capture.wait_for_reply( + mid, identity.id, since=mark, deadline_s=settings.e2e_timeout + ) + replies.assert_contains_any([marker]) + + @lane(Lane.BACKENDS) # bespoke build exposes no framework; pin scheduling to backends @requires(Dep.COPILOT_CLI) @pytest.mark.timeout(extra=180) # Copilot CLI cold boot + hosted-auth handshake @pytest.mark.asyncio(loop_scope="session") @pytest.mark.parametrize( "reasoning_effort", - # A non-default effort the hosted model advertises (its default: "medium"). + # A non-default effort copilot_hosted_model advertises (its default: "medium"). [pytest.param(None, id="default"), pytest.param("high", id="typed-effort")], ) async def test_copilot_hosted_auth_replies( @@ -207,8 +228,10 @@ async def test_copilot_hosted_auth_replies( The matrix cells run Anthropic BYOK to spare the monthly Copilot-hosted quota, but the hosted path is the one production users run — one cheap - turn per cell keeps it proven. Only hosted sessions advertise model/effort selects, so the typed-effort cell - proves a typed selection is accepted against Copilot's live catalog. + turn per cell keeps it proven. Only hosted sessions advertise model/effort + selects, so the typed-effort cell proves a typed selection is accepted + against Copilot's live catalog. Band tools stay off: with them, a hosted + turn keeps calling band_send_message after its reply and never ends. """ from band.adapters.copilot_acp import ( # noqa: PLC0415 -- copilot_acp imports the acp (agent-client-protocol) extra at its own top level; not installed in every lane's venv CopilotACPAdapter, @@ -216,7 +239,6 @@ async def test_copilot_hosted_auth_replies( skip_without_github_token(baseline_settings) - marker = unique_marker("hosted") identity = await resource_manager.provision_agent("copilot-hosted-auth") room_id = await resource_manager.provision_room( title="e2e-copilot-hosted-auth", participants=[identity.id] @@ -228,22 +250,20 @@ async def test_copilot_hosted_auth_replies( tmp_path / "hosted", hosted=True, reasoning_effort=reasoning_effort, + inject_band_tools=False, ) ) async with ( running_agent(identity, adapter, baseline_settings), reply_capture(room_id) as capture, ): - mid = await user_ops.send_message( - room_id, - f"Reply with one short sentence that includes the marker {marker}.", - mention_id=identity.id, - mention_name=identity.name, - ) - replies = await capture.wait_for_reply( - mid, identity.id, deadline_s=baseline_settings.e2e_timeout + await reply_with( + capture, + marker=unique_marker("hosted"), + user_ops=user_ops, + identity=identity, + settings=baseline_settings, ) - replies.assert_contains_any([marker]) @lane(Lane.BACKENDS) # bespoke build exposes no framework; pin scheduling to backends @@ -320,23 +340,25 @@ async def test_copilot_turn_fails_loudly_on_an_unadvertised_selection( def room_session_id(tasks: Tasks, room_id: str) -> str: - """The ACP session id the adapter last reported for ``room_id``.""" - reported = [ - metadata["acp_client_session_id"] - for metadata in (metadata_to_dict(task.metadata) for task in tasks) - if metadata.get("acp_client_room_id") == room_id - ] - assert reported, f"no ACP session reported for room {room_id}" - return reported[-1] + """The ACP session the adapter last reported for ``room_id``, read the way + it reads one back to restore it.""" + reported = ( + ACPClientHistoryConverter() + .convert([{"metadata": metadata_to_dict(task.metadata)} for task in tasks]) + .room_to_session + ) + assert room_id in reported, f"no ACP session reported for room {room_id}" + return reported[room_id] async def copilot_persisted_selection( config: Any, *, room_id: str, session_id: str ) -> ModelSelection: """What a fresh ``copilot --acp`` reports ``session_id`` runs after - ``session/load``: Copilot's own record, not the adapter's.""" - from band.integrations.acp.client_runtime import ( # noqa: PLC0415 -- the acp extra, as above - ACPRuntime, + ``session/load``: Copilot's own record, not the adapter's. It is spawned + by a fresh adapter's own runtime wiring (launcher, env, workspace).""" + from band.adapters.copilot_acp import ( # noqa: PLC0415 -- the acp extra, as above + CopilotACPAdapter, ) from band.integrations.acp.model_selection import ( # noqa: PLC0415 -- the acp extra, as above locate_model_options, @@ -345,12 +367,9 @@ async def copilot_persisted_selection( session_config_options, ) - workspace = create_room_workspace_resolver(config.cwd)(room_id) - runtime = ACPRuntime( - command=list(config.command), - env={**config.env, "GITHUB_TOKEN": config.github_token}, - cwd=workspace, - ) + spawner = CopilotACPAdapter(config) + workspace = spawner._workspace(room_id) + runtime = spawner._build_runtime(workspace) await runtime.start() try: loaded = await runtime.load_session_response( @@ -364,10 +383,8 @@ async def copilot_persisted_selection( ).current_selection() -# Both differ from what a hosted ACP session starts on (claude-sonnet-5 / -# medium: the env's COPILOT_MODEL does not pick an ACP session's model), so -# Copilot reporting them proves each was applied. Probed as advertised; an -# account without them fails the switch naming what it offers. +# Probed as advertised; an account without them fails the switch naming +# what it offers. SWITCHED = ModelSelection(model="gpt-5.4", reasoning_effort="low") @@ -402,31 +419,27 @@ async def test_copilot_switches_model_and_effort_in_a_live_room( title="e2e-copilot-live-switch", participants=[identity.id] ) config = hermetic_copilot_config( - baseline_settings, tmp_path / "copilot", hosted=True, inject_band_tools=False + baseline_settings, + tmp_path / "copilot", + hosted=True, + reasoning_effort="high", + inject_band_tools=False, ) + # Only a switch away from what the session starts on proves anything. + assert config.model != SWITCHED.model, "COPILOT_HOSTED_MODEL is the switch target" adapter = CopilotACPAdapter(config) - - async def reply_with(capture: ReplyCapture, marker: str) -> None: - mark = capture.messages.snapshot() - mid = await user_ops.send_message( - room_id, - f"Reply with one short sentence that includes the marker {marker}.", - mention_id=identity.id, - mention_name=identity.name, - ) - replies = await capture.wait_for_reply( - mid, identity.id, since=mark, deadline_s=baseline_settings.e2e_timeout - ) - replies.assert_contains_any([marker]) + turn = partial( + reply_with, user_ops=user_ops, identity=identity, settings=baseline_settings + ) async with ( running_agent(identity, adapter, baseline_settings), reply_capture(room_id) as capture, ): - await reply_with(capture, unique_marker("before-switch")) + await turn(capture, marker=unique_marker("before-switch")) await adapter.apply_model_selection(SWITCHED, room_id=room_id) # Also the prompt Copilot persists the switched session on. - await reply_with(capture, unique_marker("after-switch")) + await turn(capture, marker=unique_marker("after-switch")) session_id = room_session_id( await capture.tasks(sender_id=identity.id), room_id ) diff --git a/tests/integrations/acp/acp_toolkit/__init__.py b/tests/integrations/acp/acp_toolkit/__init__.py index de855d2c9..1ddaaf7cb 100644 --- a/tests/integrations/acp/acp_toolkit/__init__.py +++ b/tests/integrations/acp/acp_toolkit/__init__.py @@ -28,6 +28,7 @@ select_option, ) from tests.integrations.acp.acp_toolkit.harness import ( + DEFAULT_ROOM, AcpSession, FakeSpawn, Reply, @@ -41,6 +42,7 @@ ) __all__ = [ + "DEFAULT_ROOM", "AcpSession", "FakeACPAgent", "FakeSpawn", diff --git a/tests/integrations/acp/acp_toolkit/agent.py b/tests/integrations/acp/acp_toolkit/agent.py index 61549ed45..688f5ff5c 100644 --- a/tests/integrations/acp/acp_toolkit/agent.py +++ b/tests/integrations/acp/acp_toolkit/agent.py @@ -97,6 +97,9 @@ def __init__( self._config_options = list(config_options) self._config_option_handler: ConfigOptionHandler | None = None self._reply_gate: ReplyGate | None = None + self._hangs_up_on_config = False + # Closes this agent's end of the transport; the harness binds it. + self.hang_up: Callable[[], None] = lambda: None # Observability for assertions: self.sessions: list[dict[str, Any]] = [] self._mcp_servers_by_session: dict[str, list[Any]] = {} @@ -124,6 +127,11 @@ def on_config_option(self, handler: ConfigOptionHandler) -> ConfigOptionHandler: self._config_option_handler = handler return handler + def hangs_up_on_next_config_option(self) -> None: + """Drop the connection on the next ``set_config_option``, unanswered, + as a crashed agent process does.""" + self._hangs_up_on_config = True + def holds_config_replies(self) -> ReplyGate: """Apply each ``set_config_option`` at once but reply only on release.""" self._reply_gate = ReplyGate() @@ -492,6 +500,10 @@ async def set_config_option( """Apply one advertised select option and return the full live catalog.""" del kwargs self.config_option_requests.append((session_id, config_id, value)) + if self._hangs_up_on_config: + self._hangs_up_on_config = False + self.hang_up() + await asyncio.Event().wait() self._config_options = await self._select(session_id, config_id, value) if self._reply_gate is not None: self._reply_gate.received.set() diff --git a/tests/integrations/acp/acp_toolkit/harness.py b/tests/integrations/acp/acp_toolkit/harness.py index 186b2ed8c..6c54e3e1a 100644 --- a/tests/integrations/acp/acp_toolkit/harness.py +++ b/tests/integrations/acp/acp_toolkit/harness.py @@ -24,6 +24,9 @@ from band.testing import FakeAgentTools from tests.integrations.acp.acp_toolkit.agent import FakeACPAgent +# The room ``AcpSession.send`` talks to unless told otherwise. +DEFAULT_ROOM = "room-1" + _SESSION_EVENT_MARKER = "acp_client_session_id" # the adapter's trailing task event @@ -262,7 +265,7 @@ async def send( self, content: str, *, - room: str = "room-1", + room: str = DEFAULT_ROOM, history: ACPClientSessionState | None = None, bootstrap: bool = False, room_context: list[dict[str, Any]] | None = None, @@ -363,6 +366,7 @@ async def _spawn( use_unstable_protocol=True, ) conn = connect_to_agent(client, writer_c, reader_c) + agent.hang_up = writer_a.close try: yield conn, agent_conn finally: