diff --git a/openhands-agent-server/openhands/agent_server/conversation_router.py b/openhands-agent-server/openhands/agent_server/conversation_router.py index afafa4beb38..aeaca118d49 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_router.py +++ b/openhands-agent-server/openhands/agent_server/conversation_router.py @@ -405,7 +405,6 @@ async def switch_conversation_llm( responses={ 400: {"description": "Agent is not ACP, or provider can't switch models"}, 404: {"description": "Conversation not found"}, - 409: {"description": "ACP session not initialized yet"}, 504: {"description": "ACP server did not answer the model switch in time"}, }, ) @@ -414,11 +413,14 @@ async def switch_conversation_acp_model( model: str = Body(..., embed=True), conversation_service: ConversationService = Depends(get_conversation_service), ) -> Success: - """Switch the model of a running ACP conversation, mid-conversation. - - Issues a protocol-level ``session/set_model`` call to the ACP subprocess - so the new model applies to subsequent turns without losing context. Only - valid for ACP conversations whose provider supports runtime switching. + """Switch the model of an ACP conversation. + + For a conversation that has already started, issues a protocol-level + ``session/set_model`` call to the ACP subprocess so the new model applies to + subsequent turns without losing context. For one created but not yet run, + the value is persisted and applied when the first session starts (returns + ``200`` either way). Only valid for ACP conversations whose provider + supports model switching. """ event_service = await conversation_service.get_event_service(conversation_id) if event_service is None: @@ -438,11 +440,6 @@ async def switch_conversation_acp_model( status_code=status.HTTP_504_GATEWAY_TIMEOUT, detail=str(e), ) - except RuntimeError as e: - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=str(e), - ) return Success() diff --git a/openhands-agent-server/openhands/agent_server/event_service.py b/openhands-agent-server/openhands/agent_server/event_service.py index 86d119e10a9..e8fa2fa17ca 100644 --- a/openhands-agent-server/openhands/agent_server/event_service.py +++ b/openhands-agent-server/openhands/agent_server/event_service.py @@ -1079,21 +1079,24 @@ async def set_security_analyzer( ) async def switch_acp_model(self, model: str) -> None: - """Switch the model on a running ACP conversation, mid-conversation. - - Runs the (blocking) protocol-level ``session/set_model`` round-trip in a - worker thread, then mirrors the new model into ``meta.json`` so the - switch survives an agent-server restart: ``start()`` rebuilds the agent - from ``self.stored.agent`` and ``ConversationState.create()`` copies - that over the persisted base_state.json on resume. Only ``acp_model`` - needs updating — ``model_post_init`` re-derives the sentinel - ``llm.model`` on reload. + """Switch the model on an ACP conversation. + + For a conversation that has already started, runs the (blocking) + protocol-level ``session/set_model`` round-trip in a worker thread; for + one not yet run, the SDK defers the switch (persist-only). Either way it + mirrors the new model into ``meta.json`` so the switch survives an + agent-server restart: ``start()`` rebuilds the agent from + ``self.stored.agent`` and ``ConversationState.create()`` copies that over + the persisted base_state.json on resume. Only ``acp_model`` needs + updating — ``model_post_init`` re-derives the sentinel ``llm.model`` on + reload. """ if self._conversation is None: - raise RuntimeError( - "Conversation is not active; it has not been started or has " - "been closed." - ) + # Match the inactive-service convention of the other event-service + # methods (the conversation router maps it to 400). The SDK no + # longer raises for a created-but-not-yet-run conversation, so a + # pre-first-run switch is a normal 200 deferral, not an error. + raise ValueError("inactive_service") loop = asyncio.get_running_loop() await loop.run_in_executor(None, self._conversation.switch_acp_model, model) self.stored = self.stored.model_copy( diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 7c024f4f8a1..b5d233768ec 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1654,6 +1654,13 @@ def current_model_id(self) -> str | None: through ``model_dump()``. Consumers that need to read it across the API boundary should look at ``ConversationInfo.current_model_id``, which the agent-server lifts off the agent into the response. + + This (the protocol-reported session model) is the authoritative model + id — trust it over what the model says about itself. Provider models + introspect their own identity unreliably: gemini-cli reports a stale + ``gemini-1.5-flash-latest`` regardless of the active model, and codex + won't name its tier. Verified across all three providers; the switch is + honored even when the model's self-description disagrees. """ return self._current_model_id @@ -1697,6 +1704,29 @@ def supports_runtime_model_switch(self) -> bool: provider = detect_acp_provider_by_agent_name(self._agent_name) return provider is not None and provider.supports_runtime_model_switch + @property + def has_live_acp_session(self) -> bool: + """Whether a live ACP session exists to act on right now. + + ``True`` only once the subprocess-backed session is fully wired — + connection, session id, and executor all present — which happens during + the first ``run()`` (see :meth:`init_state`). ``False`` before that + (created but not yet run) and after teardown. + + Lets callers branch on session state instead of catching the + ``RuntimeError`` that :meth:`set_acp_model` raises pre-session or + reaching into the ``_conn`` / ``_session_id`` / ``_executor`` + PrivateAttrs. In particular + :meth:`~openhands.sdk.conversation.impl.local_conversation.LocalConversation.switch_acp_model` + uses it to decide between a live in-place switch and a deferred persist + that the next session start applies. + """ + return ( + self._conn is not None + and self._session_id is not None + and self._executor is not None + ) + def get_all_llms(self) -> Generator[LLM]: yield self.llm @@ -2418,16 +2448,23 @@ async def _init() -> tuple[ ) session_id = response.session_id reported_model_id, available_models = _extract_session_models(response) - # Initial-selection protocol call for providers that use it - # (codex-acp, gemini-cli); no-op for claude, which selected its - # model via the _meta above. + # Initial-model protocol call for every built-in provider + # (codex, gemini, claude-code). The pinned claude CLI ignores + # the _meta above, so this set_session_model call is what + # actually applies the model (#3654); _meta is kept only as + # backward-compat for older claude CLIs that still honor it. applied_via_call = await _maybe_set_session_model( conn, agent_name, session_id, self.acp_model, ) - override_applied = bool(session_meta) or applied_via_call + # set_session_model is the authoritative signal that acp_model + # reached the server. _meta is a no-op on the pinned claude CLI, + # so it must not by itself claim the override took effect. (For + # claude+acp_model the two always agree: the call returns True or + # raises before we reach here.) + override_applied = applied_via_call else: # Resumed session. load_session() does not carry model _meta, so # reapply the persisted (possibly runtime-switched) acp_model via @@ -3448,10 +3485,17 @@ def set_acp_model(self, model: str) -> None: keeps cost/token accounting on the previously-known model and self-heals on the next successful switch; the agent itself always runs whatever model the live ACP session holds. + + claude-agent-acp caveat: a live switch changes the session model + and the inference target, but the model-naming system context is + injected at session *start* and is not refreshed here, so the + agent's self-reported identity can lag (e.g. still says "haiku" + after switching to sonnet) until a new session. The switch itself + is applied; only the model's self-description is stale. """ if not model or not model.strip(): raise ValueError("model must be a non-empty string") - if self._conn is None or self._session_id is None or self._executor is None: + if not self.has_live_acp_session: raise RuntimeError( "ACP session is not initialized; the model can only be switched " "after the conversation has started (first run())." diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 52996ccacc0..5605f8c8f81 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -990,33 +990,46 @@ def switch_profile(self, profile_name: str) -> None: self.switch_llm(cached) def switch_acp_model(self, model: str) -> None: - """Switch the model on a running ACP conversation (mid-conversation). + """Switch the model on an ACP conversation. Unlike :meth:`switch_llm`, which swaps OpenHands' own LLM object, this - issues a protocol-level ``session/set_model`` call to the ACP - subprocess so the new model applies to subsequent turns of the *same* - session, preserving conversation context. ``switch_llm`` would not + targets the model the ACP subprocess runs. ``switch_llm`` would not affect an ACP conversation, since the subprocess owns its own model. + Behaves differently depending on whether a live session exists yet: + + * **Live session** (after the first ``run()``): issues a protocol-level + ``session/set_model`` call so the new model applies to subsequent + turns of the *same* session, preserving conversation context. + * **No live session yet** (created but not yet run): there is nothing to + switch live, so the new value is only persisted. Session creation on + the first ``run()`` then honors it — ``_maybe_set_session_model`` + issues a one-shot ``set_session_model`` for every built-in provider + (codex, gemini, and claude-code) — so the first turn runs on the + switched model rather than silently using the construction-time one. + Args: model: Provider-specific model id to switch to. Raises: ValueError: If the conversation's agent is not an :class:`ACPAgent`, - or the provider does not support runtime model switching, or - the ACP server rejects the switch. - RuntimeError: If the ACP session is not yet initialized. - TimeoutError: If the ACP server does not respond within - ``acp_prompt_timeout`` seconds. + or (for a live switch) the provider does not support runtime + model switching, or the ACP server rejects the switch. + TimeoutError: If a live switch's ``session/set_model`` round-trip + exceeds ``acp_prompt_timeout`` seconds. """ if not isinstance(self.agent, ACPAgent): raise ValueError( "switch_acp_model is only supported for ACP conversations." ) with self._state: - # Perform the live protocol switch first; if it fails we leave the - # persisted state untouched. - self.agent.set_acp_model(model) + # With a live session, perform the protocol switch first; if it + # fails we leave the persisted state untouched. Without one (pre + # first run()), there is nothing to switch live — skip the call and + # just persist; session creation applies the value (see docstring). + live = self.agent.has_live_acp_session + if live: + self.agent.set_acp_model(model) # Persist the switched model as the authoritative value. ``acp_model`` # is frozen, so we replace the agent with a copy carrying the new # value. This matters on two counts the in-place mutation missed: @@ -1027,14 +1040,17 @@ def switch_acp_model(self, model: str) -> None: # and the resumed session model from ``acp_model`` on reload, so # it must hold the switched value, not the construction-time one. # - # model_copy is shallow, so the copy shares the live ACP runtime + # model_copy is shallow, so a live copy shares the ACP runtime # (_conn/_executor/_process) with the old agent. Disarm the old # agent's finalizer before dropping it: otherwise ACPAgent.__del__ # -> close() on the discarded agent would tear down the session the # copy now owns, leaving the next turn pointing at a dead connection. + # Pre-session there is no runtime to hand off, so release_runtime is + # unnecessary (the discarded agent's close() is already a no-op). old_agent = self.agent new_agent = old_agent.model_copy(update={"acp_model": model}) - old_agent.release_runtime() + if live: + old_agent.release_runtime() # ``self.agent`` is the live reference used by subsequent ``step()`` # calls; ``self._state.agent`` is what the autosave path serializes # to base_state.json. Update both so the running conversation and the diff --git a/tests/agent_server/test_conversation_router.py b/tests/agent_server/test_conversation_router.py index fd86b04ef6b..62355456d7e 100644 --- a/tests/agent_server/test_conversation_router.py +++ b/tests/agent_server/test_conversation_router.py @@ -1156,14 +1156,44 @@ def test_switch_acp_model_non_acp_returns_400( client.app.dependency_overrides.clear() -def test_switch_acp_model_uninitialized_returns_409( +def test_switch_acp_model_pre_session_returns_200( client, mock_conversation_service, mock_event_service, sample_conversation_id ): - """A RuntimeError (session not initialized yet) maps to 409.""" + """A switch before the first run() is a 200, not a 409. + + Regression for #3763: the SDK now persists (defers) a pre-session switch + instead of raising, so the route no longer maps it to 409 "ACP session not + initialized yet". The event service returns normally (the deferral is + applied when the first session starts). + """ mock_conversation_service.get_event_service.return_value = mock_event_service - mock_event_service.switch_acp_model.side_effect = RuntimeError( - "ACP session is not initialized" + mock_event_service.switch_acp_model.return_value = None + + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service ) + try: + response = client.post( + f"/api/conversations/{sample_conversation_id}/switch_acp_model", + json={"model": "haiku"}, + ) + assert response.status_code == 200 + mock_event_service.switch_acp_model.assert_awaited_once_with("haiku") + finally: + client.app.dependency_overrides.clear() + + +def test_switch_acp_model_inactive_service_returns_400( + client, mock_conversation_service, mock_event_service, sample_conversation_id +): + """An inactive service (closed/never-started) maps to 400. + + The event service raises the shared ``inactive_service`` ValueError for a + missing live conversation, consistent with its other methods; the router's + generic ValueError handler turns it into a 400. + """ + mock_conversation_service.get_event_service.return_value = mock_event_service + mock_event_service.switch_acp_model.side_effect = ValueError("inactive_service") client.app.dependency_overrides[get_conversation_service] = ( lambda: mock_conversation_service @@ -1173,7 +1203,7 @@ def test_switch_acp_model_uninitialized_returns_409( f"/api/conversations/{sample_conversation_id}/switch_acp_model", json={"model": "haiku"}, ) - assert response.status_code == 409 + assert response.status_code == 400 finally: client.app.dependency_overrides.clear() diff --git a/tests/agent_server/test_event_service.py b/tests/agent_server/test_event_service.py index 519ef88561d..2d19c25075c 100644 --- a/tests/agent_server/test_event_service.py +++ b/tests/agent_server/test_event_service.py @@ -1690,6 +1690,32 @@ async def test_switch_acp_model_persists_to_meta(self, tmp_path): assert isinstance(loaded.agent, ACPAgent) assert loaded.agent.acp_model == "new-model" + @pytest.mark.asyncio + async def test_switch_acp_model_inactive_service_raises_value_error(self, tmp_path): + """An inactive service (no live conversation) raises the shared + ``inactive_service`` ValueError — consistent with the other + event-service methods — which the router maps to 400. It is no longer a + RuntimeError: the SDK now defers (rather than rejects) a switch before + the first run(), so the only failure mode here is a closed/never-started + service. + """ + from openhands.sdk.agent import ACPAgent + + stored = StoredConversation( + id=uuid4(), + agent=ACPAgent(acp_command=["echo", "test"], acp_model="old-model"), + workspace=LocalWorkspace(working_dir=str(tmp_path)), + confirmation_policy=NeverConfirm(), + initial_message=None, + metrics=None, + ) + service = EventService(stored=stored, conversations_dir=tmp_path) + # _conversation defaults to None (service not started). + assert service._conversation is None + + with pytest.raises(ValueError, match="inactive_service"): + await service.switch_acp_model("new-model") + class TestEventServiceStartWithRunningStatus: """Test cases for EventService.start handling of RUNNING execution status.""" diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 934e43842f0..34eba1e6933 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -4607,6 +4607,48 @@ async def test_any_request_error_is_swallowed_on_resume(self): assert applied is False +class TestHasLiveACPSession: + """The public predicate LocalConversation branches on for model switching.""" + + def test_false_before_session(self): + # Freshly constructed: no connection / session / executor wired yet. + agent = _make_agent() + assert agent.has_live_acp_session is False + + def test_true_when_fully_wired(self): + agent = _make_agent() + agent._conn = MagicMock() + agent._session_id = "sess-1" + agent._executor = MagicMock() + assert agent.has_live_acp_session is True + + @pytest.mark.parametrize( + "conn,session_id,executor", + [ + (None, "sess-1", "exec"), + ("conn", None, "exec"), + ("conn", "sess-1", None), + ], + ) + def test_false_when_partially_wired(self, conn, session_id, executor): + # Every one of the three runtime handles must be present; a partial + # teardown / bootstrap must not read as a live session. + agent = _make_agent() + agent._conn = MagicMock() if conn else None + agent._session_id = session_id + agent._executor = MagicMock() if executor else None + assert agent.has_live_acp_session is False + + def test_set_acp_model_raises_when_no_live_session(self): + # The low-level primitive still rejects a pre-session call (the deferral + # lives in LocalConversation.switch_acp_model, which branches on the + # predicate); guards that the two stay in sync. + agent = _make_agent() + assert not agent.has_live_acp_session + with pytest.raises(RuntimeError, match="not initialized"): + agent.set_acp_model("gpt-5.4") + + class TestSetACPModel: """Runtime (mid-conversation) model switching via set_session_model.""" diff --git a/tests/sdk/conversation/test_switch_model.py b/tests/sdk/conversation/test_switch_model.py index 9f8340a08ca..1998f1687a5 100644 --- a/tests/sdk/conversation/test_switch_model.py +++ b/tests/sdk/conversation/test_switch_model.py @@ -90,6 +90,57 @@ def _make_acp_conversation(tmp_path) -> tuple[LocalConversation, ACPAgent]: return conv, agent +def _make_pre_session_acp_conversation(tmp_path) -> tuple[LocalConversation, ACPAgent]: + """An ACP conversation created but not yet ``run()``. + + No ``_conn`` / ``_session_id`` / ``_executor`` is wired, so there is no live + session — ``switch_acp_model`` must defer rather than issue a protocol call. + """ + agent = ACPAgent(acp_command=["echo", "test"], acp_model="model-a") + agent._agent_name = "codex-acp" + conv = LocalConversation( + agent=agent, + workspace=tmp_path, + persistence_dir=str(tmp_path / "persist"), + ) + return conv, agent + + +def test_switch_acp_model_before_session_defers_and_persists(tmp_path): + """A pre-``run()`` switch persists the new model without raising. + + Regression for #3763: ``switch_acp_model`` used to call ``set_acp_model`` + first, which raised ``RuntimeError`` before the first ``run()``, so the + persist block never ran and the new model was silently lost — the first + session kept the construction-time ``acp_model``. With no live session the + live call must be skipped and the value persisted, so session creation on + the first ``run()`` honors the switched model. + """ + conv, agent = _make_pre_session_acp_conversation(tmp_path) + assert not agent.has_live_acp_session + + conv.switch_acp_model("model-b") + + # The authoritative model moved even though no live session was touched. + switched = conv.agent + assert isinstance(switched, ACPAgent) + assert switched.acp_model == "model-b" + assert not switched.has_live_acp_session + assert isinstance(conv.state.agent, ACPAgent) + assert conv.state.agent.acp_model == "model-b" + # Cold-read hint updated for the chip/picker before any session exists. + assert conv.state.agent_state["acp_current_model_id"] == "model-b" + + # Survives a restart before the first run: base_state.json carries the + # switched model, and model_post_init re-derives the sentinel LLM from it, + # so the first session starts on model-b (acceptance criterion #3). + base_text = conv.state._fs.read(BASE_STATE) + reloaded = ConversationState.model_validate(json.loads(base_text)) + assert isinstance(reloaded.agent, ACPAgent) + assert reloaded.agent.acp_model == "model-b" + assert reloaded.agent.llm.model == "model-b" + + def test_switch_acp_model_persists_authoritative_model(tmp_path): """A runtime switch persists as the authoritative ``acp_model``.