fix(acp): persist pre-session model switch instead of raising

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()
 
 

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(

openhands-sdk/openhands/sdk/agent/acp_agent.py

@@ -1697,6 +1697,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
 
@@ -3451,7 +3474,7 @@ def set_acp_model(self, model: str) -> None:
         """
         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())."

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

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()
 

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."""

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."""
 

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``.