fix(sdk): split Codex ACP model config options

Author: neubig

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

@@ -357,6 +357,33 @@ def _codex_base_url_overrides(
     return ["-c", f'openai_base_url="{base_url}"']
 
 
+_CODEX_REASONING_EFFORTS: Final[frozenset[str]] = frozenset(
+    {"low", "medium", "high", "xhigh"}
+)
+
+
+def _codex_model_config_options(acp_model: str) -> tuple[tuple[str, str], ...]:
+    """Map Canvas Codex model IDs to codex-acp config options.
+
+    codex-acp 0.16 exposes base model and reasoning effort as separate config
+    options. Agent Canvas keeps a single stable model id such as
+    ``gpt-5.5/high``. Sending that combined id as the raw ``model`` option is
+    accepted at config time but fails the next prompt for ChatGPT-auth sessions.
+    """
+    base_model, sep, effort = acp_model.rpartition("/")
+    if sep and base_model and effort in _CODEX_REASONING_EFFORTS:
+        return (("model", base_model), ("reasoning_effort", effort))
+    return (("model", acp_model),)
+
+
+def _model_config_options(
+    provider: ACPProviderInfo | None, acp_model: str
+) -> tuple[tuple[str, str], ...]:
+    if provider is not None and provider.key == "codex":
+        return _codex_model_config_options(acp_model)
+    return (("model", acp_model),)
+
+
 def _write_secret_file(path: Path, value: str) -> None:
     """Write ``value`` to ``path`` as a ``0600`` file.
 
@@ -584,11 +611,12 @@ async def _apply_acp_model(
     """Apply a model with the provider's configured ACP model-selection method."""
     if provider is not None:
         if provider.model_selection_method == "set_config_option":
-            await conn.set_config_option(
-                config_id="model",
-                value=acp_model,
-                session_id=session_id,
-            )
+            for config_id, value in _model_config_options(provider, acp_model):
+                await conn.set_config_option(
+                    config_id=config_id,
+                    value=value,
+                    session_id=session_id,
+                )
             logger.info(
                 "Set model %r on ACP provider %s via set_config_option",
                 acp_model,
@@ -3467,9 +3495,18 @@ def set_acp_model(self, model: str) -> None:
             provider is not None
             and provider.model_selection_method == "set_config_option"
         ):
-            model_selection_call = self._conn.set_config_option(
-                config_id="model", value=model, session_id=self._session_id
-            )
+
+            async def _set_config_options() -> None:
+                assert self._conn is not None
+                assert self._session_id is not None
+                for config_id, value in _model_config_options(provider, model):
+                    await self._conn.set_config_option(
+                        config_id=config_id,
+                        value=value,
+                        session_id=self._session_id,
+                    )
+
+            model_selection_call = _set_config_options()
         else:
             model_selection_call = self._conn.set_session_model(
                 model_id=model, session_id=self._session_id

tests/sdk/agent/test_acp_agent.py

@@ -10,7 +10,7 @@
 from concurrent.futures import Future
 from pathlib import Path
 from typing import Any
-from unittest.mock import AsyncMock, MagicMock, patch
+from unittest.mock import AsyncMock, MagicMock, call, patch
 
 import pytest
 from acp.exceptions import RequestError as ACPRequestError
@@ -22,6 +22,7 @@
     _classify_acp_init_error,
     _codex_auth_file,
     _codex_base_url_overrides,
+    _codex_model_config_options,
     _estimate_cost_from_tokens,
     _extract_session_models,
     _extract_token_usage,
@@ -4419,24 +4420,44 @@ def test_noop_when_caller_already_set_provider(self):
         )
 
 
+class TestCodexModelConfigOptions:
+    def test_splits_combined_model_and_reasoning_effort(self):
+        assert _codex_model_config_options("gpt-5.5/high") == (
+            ("model", "gpt-5.5"),
+            ("reasoning_effort", "high"),
+        )
+
+    def test_leaves_base_or_custom_model_id_unchanged(self):
+        assert _codex_model_config_options("gpt-5.5") == (("model", "gpt-5.5"),)
+        assert _codex_model_config_options("custom/provider/model") == (
+            ("model", "custom/provider/model"),
+        )
+
+
 # ---------------------------------------------------------------------------
 # ACP model overrides
 # ---------------------------------------------------------------------------
 
 
 class TestMaybeSetSessionModel:
     @pytest.mark.asyncio
-    async def test_codex_agent_uses_config_option_model_override(self):
+    async def test_codex_agent_splits_model_and_reasoning_effort(self):
         conn = AsyncMock()
         applied = await _maybe_set_session_model(
-            conn, "codex-acp", "session-1", "gpt-5.4"
+            conn, "codex-acp", "session-1", "gpt-5.4/low"
         )
         conn.set_session_model.assert_not_called()
-        conn.set_config_option.assert_awaited_once_with(
-            config_id="model",
-            value="gpt-5.4",
-            session_id="session-1",
+        conn.set_config_option.assert_has_awaits(
+            [
+                call(config_id="model", value="gpt-5.4", session_id="session-1"),
+                call(
+                    config_id="reasoning_effort",
+                    value="low",
+                    session_id="session-1",
+                ),
+            ]
         )
+        assert conn.set_config_option.await_count == 2
         # The override was actually pushed to the server via the protocol call.
         assert applied is True
 
@@ -4525,11 +4546,17 @@ async def test_codex_reapplies_persisted_model_on_resume(self):
             conn, "codex-acp", "sess-1", "gpt-5.4/low"
         )
         conn.set_session_model.assert_not_called()
-        conn.set_config_option.assert_awaited_once_with(
-            config_id="model",
-            value="gpt-5.4/low",
-            session_id="sess-1",
+        conn.set_config_option.assert_has_awaits(
+            [
+                call(config_id="model", value="gpt-5.4", session_id="sess-1"),
+                call(
+                    config_id="reasoning_effort",
+                    value="low",
+                    session_id="sess-1",
+                ),
+            ]
         )
+        assert conn.set_config_option.await_count == 2
         assert applied is True
 
     @pytest.mark.asyncio
@@ -4623,22 +4650,36 @@ class TestSetACPModel:
     @staticmethod
     def _wire(agent: ACPAgent, agent_name: str) -> ACPAgent:
         agent._conn = MagicMock()
+        agent._conn.set_config_option = AsyncMock()
+        agent._conn.set_session_model = AsyncMock()
         agent._session_id = "sess-1"
         agent._agent_name = agent_name
+
+        def run_async(awaitable, timeout=None):
+            if asyncio.iscoroutine(awaitable):
+                return asyncio.run(awaitable)
+            return awaitable
+
         executor = MagicMock()
-        executor.run_async = MagicMock()
+        executor.run_async = MagicMock(side_effect=run_async)
         agent._executor = executor
         return agent
 
     def test_switches_model_on_live_codex_session(self):
         agent = self._wire(_make_agent(), "codex-acp")
         agent.set_acp_model("gpt-5.4/low")
         agent._conn.set_session_model.assert_not_called()
-        agent._conn.set_config_option.assert_called_once_with(
-            config_id="model",
-            value="gpt-5.4/low",
-            session_id="sess-1",
+        agent._conn.set_config_option.assert_has_awaits(
+            [
+                call(config_id="model", value="gpt-5.4", session_id="sess-1"),
+                call(
+                    config_id="reasoning_effort",
+                    value="low",
+                    session_id="sess-1",
+                ),
+            ]
         )
+        assert agent._conn.set_config_option.await_count == 2
         agent._executor.run_async.assert_called_once()
         # Sentinel LLM + metrics reflect the live model for cost/token tracking.
         assert agent.llm.model == "gpt-5.4/low"
@@ -4648,7 +4689,7 @@ def test_claude_provider_supports_runtime_switch(self):
         agent = self._wire(_make_agent(), "claude-agent-acp")
         agent.set_acp_model("claude-haiku-4-5-20251001")
         agent._conn.set_session_model.assert_not_called()
-        agent._conn.set_config_option.assert_called_once_with(
+        agent._conn.set_config_option.assert_awaited_once_with(
             config_id="model",
             value="claude-haiku-4-5-20251001",
             session_id="sess-1",
@@ -4659,7 +4700,7 @@ def test_unknown_provider_still_attempts_switch(self):
         # the call; the ACP layer errors if it isn't actually supported.
         agent = self._wire(_make_agent(), "some-custom-acp")
         agent.set_acp_model("whatever")
-        agent._conn.set_session_model.assert_called_once()
+        agent._conn.set_session_model.assert_awaited_once()
 
     def test_rejects_empty_model(self):
         agent = self._wire(_make_agent(), "codex-acp")
@@ -4701,9 +4742,13 @@ def test_translates_acp_request_error_to_value_error(self):
         # or an invalid model id) must surface as a ValueError — not leak as a
         # raw acp.exceptions.RequestError — so the agent-server maps it to 400.
         agent = self._wire(_make_agent(), "codex-acp")
-        agent._executor.run_async.side_effect = ACPRequestError(
-            code=-32601, message="method not found"
-        )
+
+        def reject(awaitable, timeout=None):
+            if asyncio.iscoroutine(awaitable):
+                awaitable.close()
+            raise ACPRequestError(code=-32601, message="method not found")
+
+        agent._executor.run_async.side_effect = reject
         with pytest.raises(ValueError, match="rejected model switch"):
             agent.set_acp_model("bogus-model")
         # The sentinel LLM must not be mutated when the switch fails.
@@ -4715,9 +4760,13 @@ def test_propagates_server_internal_error(self):
         # than be mislabeled as a 400-class ValueError, mirroring the retriable
         # handling on the prompt path.
         agent = self._wire(_make_agent(), "codex-acp")
-        agent._executor.run_async.side_effect = ACPRequestError(
-            code=-32603, message="internal error"
-        )
+
+        def fail_internal(awaitable, timeout=None):
+            if asyncio.iscoroutine(awaitable):
+                awaitable.close()
+            raise ACPRequestError(code=-32603, message="internal error")
+
+        agent._executor.run_async.side_effect = fail_internal
         with pytest.raises(ACPRequestError):
             agent.set_acp_model("some-model")
         # The sentinel LLM must not be mutated when the switch fails.