From a41e3afb380421bc69deb071b8dfae816a88df4f Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Fri, 12 Jun 2026 17:50:11 -0400 Subject: [PATCH 1/8] fix ACP model selection methods Co-authored-by: openhands --- .../agent_server/conversation_router.py | 13 +- .../agent_server/conversation_service.py | 2 +- .../openhands/agent_server/docker/Dockerfile | 2 +- .../openhands/agent_server/event_service.py | 4 +- .../openhands/agent_server/models.py | 10 +- .../openhands/sdk/agent/acp_agent.py | 164 ++++++++++-------- .../openhands/sdk/agent/acp_models.py | 2 +- .../conversation/impl/local_conversation.py | 2 +- .../openhands/sdk/settings/acp_providers.py | 60 ++++--- openhands-sdk/openhands/sdk/settings/model.py | 4 +- openhands-sdk/pyproject.toml | 2 +- .../test_conversation_info_model.py | 2 +- .../agent_server/test_conversation_router.py | 12 +- tests/sdk/agent/test_acp_agent.py | 92 ++++++---- tests/sdk/settings/test_acp_providers.py | 11 +- tests/sdk/test_settings.py | 4 +- uv.lock | 8 +- 17 files changed, 227 insertions(+), 167 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/conversation_router.py b/openhands-agent-server/openhands/agent_server/conversation_router.py index afafa4beb3..b4d3081774 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_router.py +++ b/openhands-agent-server/openhands/agent_server/conversation_router.py @@ -416,9 +416,10 @@ async def switch_conversation_acp_model( ) -> 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. + Issues the provider's protocol-level model-selection 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. """ event_service = await conversation_service.get_event_service(conversation_id) if event_service is None: @@ -431,9 +432,9 @@ async def switch_conversation_acp_model( detail=str(e), ) except TimeoutError as e: - # The bounded session/set_model round-trip expired. The ACP server is - # wedged/slow rather than rejecting the request, so surface a 504 - # instead of an opaque 500. + # The bounded model-selection round-trip expired. The ACP server is + # wedged/slow rather than rejecting the request, so surface a 504 instead + # of an opaque 500. raise HTTPException( status_code=status.HTTP_504_GATEWAY_TIMEOUT, detail=str(e), diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index b588377769..a44e77b635 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -264,7 +264,7 @@ def _compose_conversation_info( # The ``acp_model`` fallback is gated on the agent NOT being a live, # initialized one. Once ``init_state`` has fired, ``current_model_id`` is the # authoritative resolved value — including ``None`` when an override couldn't - # be applied (unknown provider, or a resume whose ``set_session_model`` the + # be applied (unknown provider, or a resume whose model-selection call the # server rejected) — so falling back to ``acp_model`` there would re-assert an # override the live session isn't actually running. The fallback is only for # *cold* reads (``init_state`` hasn't fired, PrivateAttrs still empty), where diff --git a/openhands-agent-server/openhands/agent_server/docker/Dockerfile b/openhands-agent-server/openhands/agent_server/docker/Dockerfile index 8751c22628..ab3442609c 100644 --- a/openhands-agent-server/openhands/agent_server/docker/Dockerfile +++ b/openhands-agent-server/openhands/agent_server/docker/Dockerfile @@ -172,7 +172,7 @@ RUN set -ux; \ PATH="$ACP_NODE_DIR/bin:$PATH"; \ if "$ACP_NODE_DIR/bin/npm" install -g \ @agentclientprotocol/claude-agent-acp@0.30.0 \ - @zed-industries/codex-acp@0.15.0 \ + @zed-industries/codex-acp@0.16.0 \ @google/gemini-cli@0.38.0; then \ # Create wrappers in /usr/local/bin that prepend ACP's Node 22 to PATH. # This ensures the ACP binary's #!/usr/bin/env node shebang resolves diff --git a/openhands-agent-server/openhands/agent_server/event_service.py b/openhands-agent-server/openhands/agent_server/event_service.py index 86d119e10a..49d9d001ea 100644 --- a/openhands-agent-server/openhands/agent_server/event_service.py +++ b/openhands-agent-server/openhands/agent_server/event_service.py @@ -1081,8 +1081,8 @@ 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 + Runs the blocking provider model-selection 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`` diff --git a/openhands-agent-server/openhands/agent_server/models.py b/openhands-agent-server/openhands/agent_server/models.py index 35da059e8f..d621a3e166 100644 --- a/openhands-agent-server/openhands/agent_server/models.py +++ b/openhands-agent-server/openhands/agent_server/models.py @@ -223,13 +223,13 @@ class _ConversationInfoBase(BaseModel): supports_runtime_model_switch: bool = Field( default=False, description=( - "Whether a live, mid-conversation model switch (via " - "``session/set_model``) will be attempted for this conversation — " + "Whether a live, mid-conversation model switch will be attempted " + "for this conversation — " "tells the inline picker whether to offer a live-switch control. " "Mirrors the SDK's switch gate: ``True`` for known switch-capable " - "providers; ``True`` for unknown/custom ACP servers too, since " - "OpenHands attempts the switch optimistically rather than refusing " - "(a rejection then surfaces as an error). ``False`` for native " + "providers; ``False`` for unknown/custom ACP servers because their " + "generic config writes are not guaranteed live-switch primitives. " + "``False`` for native " "OpenHands agents, for a known provider that declares no support, " "and before the conversation has started a session." ), diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 8382946c4f..6e5f1dae95 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -75,6 +75,7 @@ from openhands.sdk.observability.laminar import maybe_init_laminar, observe from openhands.sdk.settings.acp_providers import ( ACPFileSecretSpec, + ACPProviderInfo, build_session_model_meta, default_acp_file_secrets, detect_acp_provider_by_agent_name, @@ -336,7 +337,7 @@ def _codex_base_url_overrides( corporate egress, etc.) via ``OPENAI_BASE_URL`` alone would have every turn hit the real OpenAI API with the wrong key and fail ``401 invalid_api_key`` — surfaced opaquely as ACP ``-32603 Internal error``. (codex-acp 0.11.1 - happened to honour the env var; 0.15.0 does not, so the eval/canvas/cloud + happened to honour the env var; current releases do not, so the eval/canvas/cloud codex-via-proxy flows broke on the bump.) The documented one-liner is ``openai_base_url`` — it overrides the built-in @@ -403,7 +404,7 @@ def _extract_session_models( current = current if isinstance(current, str) and current else None raw = getattr(models, "available_models", None) or [] # Drop entries without a usable id: an empty/missing ``model_id`` is an - # invalid picker option and an unusable ``set_session_model`` target, so we + # invalid picker option and an unusable model-selection target, so we # filter it out rather than surfacing ``model_id=""``. available = [ info for info in (ACPModelInfo.from_protocol(m) for m in raw) if info.model_id @@ -535,21 +536,18 @@ async def _maybe_set_session_model( ) -> bool: """Apply the *initial* session model right after session creation. - This is the session-creation path only, gated on - :attr:`~openhands.sdk.settings.acp_providers.ACPProviderInfo.supports_set_session_model`. - All built-in providers get a one-shot ``set_session_model`` call here. - claude-agent-acp used to be skipped on the assumption that it already - received the model via ``new_session()`` ``_meta``, but 0.30.0 silently - ignores that payload (#3654) — the protocol call is the only path that - both validates and applies the model. + This is the session-creation path only, gated on each provider's configured + model-selection method. claude-agent-acp used to be skipped on the + assumption that it already received the model via ``new_session()`` ``_meta``, + but 0.30.0 silently ignores that payload (#3654) — a protocol call is the + only path that both validates and applies the model. For unknown/custom providers (e.g. Devin CLI), we fall back to the generic ``set_config_option`` method with configId="model", which is a standard ACP method that many custom ACP servers support. Runtime, mid-conversation switches go through - :meth:`ACPAgent.set_acp_model` instead, which always uses - ``set_session_model`` and is gated on the separate + :meth:`ACPAgent.set_acp_model` instead, which is gated on the separate ``supports_runtime_model_switch`` capability flag. Returns ``True`` only when this issued a model-setting call that succeeded — i.e. @@ -561,33 +559,62 @@ async def _maybe_set_session_model( if not acp_model: return False provider = detect_acp_provider_by_agent_name(agent_name) - if provider is not None and provider.supports_set_session_model: - await conn.set_session_model(model_id=acp_model, session_id=session_id) - return True - # For unknown/custom providers, try the generic set_config_option method - # which is a standard ACP protocol method for setting configuration options - if provider is None: - try: + try: + return await _apply_acp_model_selection( + conn, provider, agent_name, session_id, acp_model + ) + except ACPRequestError as e: + if provider is not None: + raise + logger.warning( + "Could not set model %r on unknown/custom ACP server %s via " + "set_config_option (%s); the session will use the server default", + acp_model, + agent_name, + e, + ) + return False + + +async def _apply_acp_model_selection( + conn: ClientSideConnection, + provider: ACPProviderInfo | None, + agent_name: str, + session_id: str, + acp_model: str, +) -> bool: + """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, ) logger.info( - "Set model %r on unknown/custom ACP server %s via set_config_option", + "Set model %r on ACP provider %s via set_config_option", acp_model, - agent_name, + provider.key, ) return True - except ACPRequestError as e: - logger.warning( - "Could not set model %r on unknown/custom ACP server %s via " - "set_config_option (%s); the session will use the server default", - acp_model, - agent_name, - e, - ) - return False + if provider.supports_set_session_model: + await conn.set_session_model(model_id=acp_model, session_id=session_id) + return True + return False + + # For unknown/custom providers, try the generic set_config_option method + # which is a standard ACP protocol method for setting configuration options. + await conn.set_config_option( + config_id="model", + value=acp_model, + session_id=session_id, + ) + logger.info( + "Set model %r on unknown/custom ACP server %s via set_config_option", + acp_model, + agent_name, + ) + return True async def _reapply_session_model_on_resume( @@ -600,8 +627,8 @@ async def _reapply_session_model_on_resume( ``load_session()`` carries no model ``_meta``, so a session resumed after a runtime switch (or with any persisted ``acp_model``) would otherwise run on - the ACP server's default. This issues ``set_session_model`` so the resumed - live session matches the serialized ``acp_model``. + the ACP server's default. This issues the provider's model-selection method + so the resumed live session matches the serialized ``acp_model``. For unknown/custom providers (e.g. Devin CLI), we fall back to the generic ``set_config_option`` method with configId="model", which is a standard ACP @@ -610,8 +637,8 @@ async def _reapply_session_model_on_resume( The gating mirrors :meth:`ACPAgent.set_acp_model` (attempt for custom/unknown servers and known providers that support runtime switching; skip only known providers that don't), deliberately differing from the initial-selection - gate: claude-agent-acp selects its initial model via ``_meta`` yet supports - ``set_session_model`` for later switches. A server that rejects the call is + gate: claude-agent-acp sends initial model ``_meta`` yet still needs a + protocol-level model-selection call. A server that rejects the call is tolerated (logged) — like the ``load_session`` fallback above — so resume can't break; the session keeps the server default until the next switch. @@ -628,23 +655,9 @@ async def _reapply_session_model_on_resume( if provider is not None and not provider.supports_runtime_model_switch: return False try: - if provider is not None: - # Known provider: use set_session_model - await conn.set_session_model(model_id=acp_model, session_id=session_id) - else: - # Unknown/custom provider: try set_config_option as fallback - await conn.set_config_option( - config_id="model", - value=acp_model, - session_id=session_id, - ) - logger.info( - "Reapplied model %r on unknown/custom ACP server %s " - "via set_config_option", - acp_model, - agent_name, - ) - return True + return await _apply_acp_model_selection( + conn, provider, agent_name, session_id, acp_model + ) except ACPRequestError as e: logger.warning( "Could not reapply model %r on resumed session %s (%s); the live " @@ -1341,8 +1354,8 @@ def _serialize_acp_env(self, value: dict[str, str], info): default=None, description=( "Model for the ACP server to use (e.g. 'claude-opus-4-6' or " - "'gpt-5.4'). For Claude ACP, passed via session _meta. For Codex " - "ACP, applied via the protocol-level set_session_model call. " + "'gpt-5.4'). For Claude ACP, also passed via session _meta. For " + "Codex ACP, applied via session/set_config_option configId='model'. " "If None, the server picks its default." ), ) @@ -1468,7 +1481,7 @@ def model_post_init(self, __context: object) -> None: # The model the ACP server reported as active for this session, captured # from ``models.currentModelId`` on the new_session / load_session # response. Overridden by ``self.acp_model`` when the caller explicitly - # chose one (either via ``set_session_model`` or via session ``_meta``). + # chose one (via the provider's model-selection method or session ``_meta``). # ``None`` when the server doesn't surface model state — the field is # marked UNSTABLE in the ACP spec, so older agents may omit it. # @@ -1488,7 +1501,7 @@ def model_post_init(self, __context: object) -> None: # list on resume. The public ``available_models`` property coerces to ``[]``. _available_models: list[ACPModelInfo] | None = PrivateAttr(default=None) # Whether the caller's ``acp_model`` was actually pushed to the server in - # the most recent session init (via session ``_meta`` or ``set_session_model``). + # the most recent session init (via session ``_meta`` or a model-selection call). # ``False`` when there's no override, the provider can't apply it (unknown # server on a fresh session), or the server rejected the call on resume — in # those cases the live session runs its own default, so neither @@ -1654,7 +1667,7 @@ def available_models(self) -> list[ACPModelInfo]: enough for a client to render a model picker and resolve ``current_model_id`` to a display label without any server-side curation. ``current_model_id`` is the value to pass to - ``set_session_model`` to switch. + the provider's model-selection method to switch. Same lifecycle and serialization caveats as ``current_model_id``: in-process runtime state, lifted onto @@ -1669,10 +1682,10 @@ def supports_runtime_model_switch(self) -> bool: """Whether a live, mid-conversation model switch will be attempted. Tells a client whether to offer the inline picker's live-switch control. - ``True`` only for known providers that explicitly declare support for - ``session/set_model``. Unknown/custom providers use ``set_config_option`` - for *initial* model selection but that RPC is a generic config write, not - a guaranteed live-switch primitive, so the picker is hidden for them. + ``True`` only for known providers that explicitly declare runtime model + switching support. Unknown/custom providers use ``set_config_option`` for + *initial* model selection but that RPC is a generic config write, not a + guaranteed live-switch primitive, so the picker is hidden for them. ``False`` before a session exists (nothing to switch yet). See @@ -2388,7 +2401,7 @@ async def _init() -> tuple[ # Track whether ``acp_model`` was actually pushed to the server so # ``current_model_id`` below can stay honest: a caller override that # never reached the server (unknown provider on a fresh session, or - # a resume whose ``set_session_model`` the server rejected) must not + # a resume whose model-selection call the server rejected) must not # be surfaced as the live model. override_applied = False if session_id is None: @@ -3395,7 +3408,7 @@ async def _fork_and_prompt() -> str: def set_acp_model(self, model: str) -> None: """Switch the model on the running ACP session (mid-conversation). - Issues a protocol-level ``session/set_model`` call on the live + Issues the provider's protocol-level model-selection call on the live connection so the new model takes effect for subsequent turns in the *same* session — no subprocess restart, no loss of conversation context. Verified against claude-agent-acp and codex-acp. @@ -3417,7 +3430,7 @@ def set_acp_model(self, model: str) -> None: Raises: ValueError: If ``model`` is empty or whitespace-only, if the detected provider does not support runtime model switching, or - if the ACP server rejects the ``session/set_model`` call (e.g. + if the ACP server rejects the model-selection call (e.g. method-not-found on a custom server, or an invalid model id). RuntimeError: If the ACP session has not been initialized yet (i.e. before the first ``run()``). @@ -3426,10 +3439,10 @@ def set_acp_model(self, model: str) -> None: Note: A timeout means the client stopped waiting, not that the switch was - rejected: the ``session/set_model`` request may already have been - written and could still be applied server-side. The connection and - session stay alive and the local sentinel model is intentionally - left unchanged, so a timed-out switch leaves the server-side model + rejected: the model-selection request may already have been written + and could still be applied server-side. The connection and session + stay alive and the local sentinel model is intentionally left + unchanged, so a timed-out switch leaves the server-side model indeterminate. The conservative choice (treat it as failed locally) keeps cost/token accounting on the previously-known model and self-heals on the next successful switch; the agent itself always @@ -3446,18 +3459,27 @@ def set_acp_model(self, model: str) -> None: if provider is not None and not provider.supports_runtime_model_switch: raise ValueError( f"ACP provider '{provider.key}' does not support runtime model " - "switching via set_session_model." + "switching." ) # Bounded round-trip: this runs while LocalConversation.switch_acp_model # holds the state lock, so a server that accepts the call but never # answers must not wedge the lock indefinitely. On timeout / protocol # error we propagate *before* mutating any local state, so the sentinel # LLM is only updated once the live session has actually switched. + if ( + 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 + ) + else: + model_selection_call = self._conn.set_session_model( + model_id=model, session_id=self._session_id + ) try: self._executor.run_async( - self._conn.set_session_model( - model_id=model, session_id=self._session_id - ), + model_selection_call, timeout=self.acp_prompt_timeout, ) except ACPRequestError as e: @@ -3473,7 +3495,7 @@ def set_acp_model(self, model: str) -> None: # and the agent-server route — treat it as a 400-class client error # rather than an opaque 500. raise ValueError( - f"ACP server rejected set_session_model(model={model!r}): {e}" + f"ACP server rejected model switch to {model!r}: {e}" ) from e # Reflect the live model on the sentinel LLM + metrics so cost/token # accounting and serialized state show the model actually in use diff --git a/openhands-sdk/openhands/sdk/agent/acp_models.py b/openhands-sdk/openhands/sdk/agent/acp_models.py index 4e7d79b340..f50dbbd988 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_models.py +++ b/openhands-sdk/openhands/sdk/agent/acp_models.py @@ -36,7 +36,7 @@ class ACPModelInfo(BaseModel): "Server-assigned model identifier. May be concrete " '(e.g. ``"gpt-5.5/xhigh"``) or an opaque alias ' '(e.g. ``"default"``, ``"auto"``). This is the value to pass to ' - "``set_session_model`` to switch to this model." + "the provider's model-selection method to switch to this model." ), ) name: str | None = Field( diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 1fa6adad05..615f8db371 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -964,7 +964,7 @@ def switch_acp_model(self, model: str) -> None: """Switch the model on a running ACP conversation (mid-conversation). Unlike :meth:`switch_llm`, which swaps OpenHands' own LLM object, this - issues a protocol-level ``session/set_model`` call to the ACP + issues the provider's protocol-level model-selection call to the ACP subprocess so the new model applies to subsequent turns of the *same* session, preserving conversation context. ``switch_llm`` would not affect an ACP conversation, since the subprocess owns its own model. diff --git a/openhands-sdk/openhands/sdk/settings/acp_providers.py b/openhands-sdk/openhands/sdk/settings/acp_providers.py index 39e1ba4ab8..3938b575bf 100644 --- a/openhands-sdk/openhands/sdk/settings/acp_providers.py +++ b/openhands-sdk/openhands/sdk/settings/acp_providers.py @@ -11,12 +11,12 @@ - ``default_session_mode`` ACP mode ID that disables permission prompts - ``agent_name_patterns`` lowercase substrings in the runtime agent name; used by ``ACPAgent`` to auto-detect mode / protocol -- ``supports_set_session_model`` whether the provider applies its *initial* - model via the ``set_session_model`` protocol - call at session creation +- ``supports_set_session_model`` whether the provider supports the literal + ``session/set_model`` protocol call - ``supports_runtime_model_switch`` whether the server supports the - ``session/set_model`` protocol call for + configured model-selection method for runtime, mid-conversation model switching +- ``model_selection_method`` protocol method used to apply model changes - ``session_meta_key`` top-level ``_meta`` key for model selection (or ``None``) - ``available_models`` curated list of selectable models for the provider's model picker (``acp_model`` candidates) @@ -173,14 +173,14 @@ class ACPProviderInfo: """ supports_set_session_model: bool - """``True`` if this provider selects its *initial* model via the - ``set_session_model`` protocol call (rather than session ``_meta``). - - This governs the **session-creation** path only. ``True`` for all three - built-in providers, which get a one-shot ``set_session_model`` call right - after the session is created. claude-agent-acp was ``False`` until 0.30.0 - was found to silently ignore the session-``_meta`` selection it relied on - (#3654); its ``_meta`` payload is still sent alongside (see + """``True`` if this provider supports the literal ``session/set_model`` call. + + ``session/set_model`` is not the only protocol-level model-selection path: + current ACP servers may expose ``session/set_config_option`` with + ``configId="model"`` instead. Use :attr:`model_selection_method` to decide + which call to issue. claude-agent-acp was ``False`` until 0.30.0 was found + to silently ignore the session-``_meta`` selection it relied on (#3654); + its ``_meta`` payload is still sent alongside (see :attr:`session_meta_key`). This is **independent of** runtime switching capability — see @@ -197,10 +197,7 @@ class ACPProviderInfo: ``{session_meta_key: {"options": {"model": }}}`` passed to ``new_session()``. This is best-effort only: claude-agent-acp 0.30.0 ignores it (#3654), so the authoritative initial selection is the - ``set_session_model`` call gated on :attr:`supports_set_session_model`. - - Runtime switches always use ``set_session_model`` (gated on - :attr:`supports_runtime_model_switch`). + :attr:`model_selection_method` call when supported. - ``"claudeCode"`` — claude-agent-acp - ``None`` — codex-acp, gemini-cli @@ -224,8 +221,7 @@ class ACPProviderInfo: """ supports_runtime_model_switch: bool = False - """``True`` if the server supports the ``session/set_model`` protocol call - for **runtime, mid-conversation model switching**. + """``True`` if the server supports runtime, mid-conversation model switching. The call applies to the live session, so subsequent turns use the new model without restarting the subprocess or losing context. All three @@ -234,14 +230,25 @@ class ACPProviderInfo: Unlike :attr:`supports_set_session_model`, this is about switching the model of an *already-running* session, not the initial selection. A - provider may select its initial model via ``_meta`` (claude-agent-acp) - yet still support ``set_session_model`` for later switches. + provider may select its initial model via ``_meta`` (claude-agent-acp) yet + still support a protocol-level model-selection method for later switches. Defaults to ``False`` so forward-compat providers — and any external caller constructing this dataclass positionally — keep working without a signature break; the built-in providers set it explicitly. """ + model_selection_method: Literal["set_session_model", "set_config_option"] = ( + "set_session_model" + ) + """Protocol method used to apply model selection for this provider. + + ``"set_session_model"`` issues ACP ``session/set_model``. ``"set_config_option"`` + issues ACP ``session/set_config_option`` with ``configId="model"``. The + default preserves the historical behavior for external callers constructing + this dataclass positionally. + """ + file_secrets: tuple[ACPFileSecretSpec, ...] = field(default=(), compare=False) """Reserved file-content credential secrets this provider authenticates from. @@ -379,7 +386,7 @@ class ACPProviderInfo: # `ACPAgentSettings.resolve_acp_command` runs the pinned `binary_name` instead, # so the `@version` suffix is a no-op there. CLAUDE_AGENT_ACP_VERSION = "0.30.0" -CODEX_ACP_VERSION = "0.15.0" +CODEX_ACP_VERSION = "0.16.0" GEMINI_CLI_VERSION = "0.38.0" @@ -400,11 +407,12 @@ class ACPProviderInfo: # claude-agent-acp 0.30.0 silently ignores the session-_meta model # selection (the requested model only becomes a picker option; the # session keeps running "default"), so the init path must push the - # model via session/set_model like codex/gemini (#3654). The _meta + # model via session/set_config_option configId="model". The _meta # payload (session_meta_key below) is still sent — harmless, and # picks up the same model if a future CLI honours it. supports_set_session_model=True, supports_runtime_model_switch=True, + model_selection_method="set_config_option", session_meta_key="claudeCode", available_models=_CLAUDE_MODELS, default_model="claude-opus-4-8", @@ -423,8 +431,12 @@ class ACPProviderInfo: base_url_env_var="OPENAI_BASE_URL", default_session_mode="full-access", agent_name_patterns=("codex-acp",), - supports_set_session_model=True, + # Current codex-acp exposes model selection as the standard + # session/set_config_option configId="model" method, not the older + # session/set_model RPC. + supports_set_session_model=False, supports_runtime_model_switch=True, + model_selection_method="set_config_option", session_meta_key=None, available_models=_CODEX_MODELS, default_model="gpt-5.5/medium", @@ -547,7 +559,7 @@ def build_session_model_meta(agent_name: str, acp_model: str | None) -> dict[str :attr:`~ACPProviderInfo.session_meta_key` is not ``None``). Returns an empty dict when *acp_model* is ``None`` or when the detected - provider uses the ``set_session_model`` protocol call instead. + provider uses a protocol call instead. """ if not acp_model: return {} diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index d390cd8071..2052e30035 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -1323,8 +1323,8 @@ def _serialize_acp_env(self, value: dict[str, str], info): default=None, description=( "Model identifier for the ACP server to use (e.g. " - "``'claude-opus-4-6'``). claude-agent-acp receives it via session " - "_meta; codex-acp and gemini-cli via ``set_session_model``. " + "``'claude-opus-4-6'``). Providers apply it through their " + "configured ACP model-selection method. " "Leave blank to let the server pick its default." ), json_schema_extra={ diff --git a/openhands-sdk/pyproject.toml b/openhands-sdk/pyproject.toml index 38713baa44..1e88bf80ee 100644 --- a/openhands-sdk/pyproject.toml +++ b/openhands-sdk/pyproject.toml @@ -5,7 +5,7 @@ description = "OpenHands SDK - Core functionality for building AI agents" requires-python = ">=3.12" dependencies = [ - "agent-client-protocol>=0.8.1", + "agent-client-protocol>=0.10.1", "deprecation>=2.1.0", "fakeredis[lua]>=2.32.1", # Explicit dependency for docket/fastmcp background tasks "fastmcp>=3.0.0", diff --git a/tests/agent_server/test_conversation_info_model.py b/tests/agent_server/test_conversation_info_model.py index eb55c65a10..97a8688e00 100644 --- a/tests/agent_server/test_conversation_info_model.py +++ b/tests/agent_server/test_conversation_info_model.py @@ -156,7 +156,7 @@ def test_cold_read_falls_back_to_acp_model_override(): def test_live_agent_does_not_fall_back_to_unapplied_override(): """Live initialized agent whose override was NOT applied (e.g. a resume whose - ``set_session_model`` the server rejected): ``current_model_id`` is the + model-selection call the server rejected): ``current_model_id`` is the authoritative ``None`` and must NOT fall back to ``acp_model`` — that would re-assert an override the live session isn't running. """ diff --git a/tests/agent_server/test_conversation_router.py b/tests/agent_server/test_conversation_router.py index fd86b04ef6..90a9162f82 100644 --- a/tests/agent_server/test_conversation_router.py +++ b/tests/agent_server/test_conversation_router.py @@ -1181,7 +1181,7 @@ def test_switch_acp_model_uninitialized_returns_409( def test_switch_acp_model_protocol_error_returns_400( client, mock_conversation_service, mock_event_service, sample_conversation_id ): - """A rejected ACP ``session/set_model`` call maps to 400, not 500. + """A rejected ACP model-selection call maps to 400, not 500. ``ACPAgent.set_acp_model`` translates ``acp.exceptions.RequestError`` (e.g. method-not-found on a custom server, or an invalid model id) into a @@ -1190,7 +1190,7 @@ def test_switch_acp_model_protocol_error_returns_400( """ mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.switch_acp_model.side_effect = ValueError( - "ACP server rejected set_session_model(model='bogus'): method not found" + "ACP server rejected model switch to 'bogus': method not found" ) client.app.dependency_overrides[get_conversation_service] = ( @@ -1211,13 +1211,13 @@ def test_switch_acp_model_timeout_returns_504( ): """A TimeoutError (wedged/slow ACP server) maps to 504, not 500. - ``ACPAgent.set_acp_model`` bounds the ``session/set_model`` round-trip with - ``acp_prompt_timeout``; an expired call raises ``TimeoutError``, which the - route surfaces as a Gateway Timeout rather than an opaque 500. + ``ACPAgent.set_acp_model`` bounds the provider model-selection round-trip + with ``acp_prompt_timeout``; an expired call raises ``TimeoutError``, which + the route surfaces as a Gateway Timeout rather than an opaque 500. """ mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.switch_acp_model.side_effect = TimeoutError( - "ACP server did not answer set_session_model within 600s" + "ACP server did not answer model switch within 600s" ) client.app.dependency_overrides[get_conversation_service] = ( diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 934e43842f..8efc00dcbe 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -4384,7 +4384,7 @@ def test_detects_codex_in_any_token(self): # e.g. launched via npx with the scoped package name ov = _codex_base_url_overrides( "npx", - ["-y", "@zed-industries/codex-acp@0.15.0"], + ["-y", "@zed-industries/codex-acp@0.16.0"], {"OPENAI_BASE_URL": "https://p"}, ) assert ov == ["-c", 'openai_base_url="https://p"'] @@ -4426,23 +4426,25 @@ def test_noop_when_caller_already_set_provider(self): class TestMaybeSetSessionModel: @pytest.mark.asyncio - async def test_codex_agent_uses_protocol_model_override(self): + async def test_codex_agent_uses_config_option_model_override(self): conn = AsyncMock() applied = await _maybe_set_session_model( conn, "codex-acp", "session-1", "gpt-5.4" ) - conn.set_session_model.assert_awaited_once_with( - model_id="gpt-5.4", + 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", ) # The override was actually pushed to the server via the protocol call. assert applied is True @pytest.mark.asyncio - async def test_claude_agent_uses_protocol_model_override(self): + async def test_claude_agent_uses_config_option_model_override(self): # claude-agent-acp 0.30.0 silently ignores the session-_meta model - # selection (#3654), so the init path pushes the model via the same - # one-shot set_session_model call as codex/gemini. + # selection (#3654), so the init path pushes the model via + # session/set_config_option configId="model". conn = AsyncMock() applied = await _maybe_set_session_model( conn, @@ -4450,8 +4452,10 @@ async def test_claude_agent_uses_protocol_model_override(self): "session-1", "claude-opus-4-8", ) - conn.set_session_model.assert_awaited_once_with( - model_id="claude-opus-4-8", + conn.set_session_model.assert_not_called() + conn.set_config_option.assert_awaited_once_with( + config_id="model", + value="claude-opus-4-8", session_id="session-1", ) assert applied is True @@ -4506,8 +4510,11 @@ async def test_claude_reapplies_persisted_model_on_resume(self): applied = await _reapply_session_model_on_resume( conn, "claude-agent-acp", "sess-1", "claude-haiku-4-5-20251001" ) - conn.set_session_model.assert_awaited_once_with( - model_id="claude-haiku-4-5-20251001", session_id="sess-1" + conn.set_session_model.assert_not_called() + conn.set_config_option.assert_awaited_once_with( + config_id="model", + value="claude-haiku-4-5-20251001", + session_id="sess-1", ) assert applied is True @@ -4517,8 +4524,11 @@ async def test_codex_reapplies_persisted_model_on_resume(self): applied = await _reapply_session_model_on_resume( conn, "codex-acp", "sess-1", "gpt-5.4/low" ) - conn.set_session_model.assert_awaited_once_with( - model_id="gpt-5.4/low", session_id="sess-1" + 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", ) assert applied is True @@ -4597,18 +4607,18 @@ async def test_any_request_error_is_swallowed_on_resume(self): # resume — like the load_session fallback — so a flaky/stale server # can't break session startup; the session keeps the server default. conn = AsyncMock() - conn.set_session_model.side_effect = ACPRequestError( + conn.set_config_option.side_effect = ACPRequestError( code=-32603, message="internal error" ) applied = await _reapply_session_model_on_resume( conn, "codex-acp", "sess-1", "gpt-5.4/low" ) - conn.set_session_model.assert_awaited_once() + conn.set_config_option.assert_awaited_once() assert applied is False class TestSetACPModel: - """Runtime (mid-conversation) model switching via set_session_model.""" + """Runtime (mid-conversation) model switching.""" @staticmethod def _wire(agent: ACPAgent, agent_name: str) -> ACPAgent: @@ -4623,8 +4633,11 @@ def _wire(agent: ACPAgent, agent_name: str) -> ACPAgent: 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_called_once_with( - model_id="gpt-5.4/low", session_id="sess-1" + 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._executor.run_async.assert_called_once() # Sentinel LLM + metrics reflect the live model for cost/token tracking. @@ -4634,8 +4647,11 @@ def test_switches_model_on_live_codex_session(self): 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_called_once_with( - model_id="claude-haiku-4-5-20251001", session_id="sess-1" + agent._conn.set_session_model.assert_not_called() + agent._conn.set_config_option.assert_called_once_with( + config_id="model", + value="claude-haiku-4-5-20251001", + session_id="sess-1", ) def test_unknown_provider_still_attempts_switch(self): @@ -4688,7 +4704,7 @@ def test_translates_acp_request_error_to_value_error(self): agent._executor.run_async.side_effect = ACPRequestError( code=-32601, message="method not found" ) - with pytest.raises(ValueError, match="rejected set_session_model"): + 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. assert agent.llm.model != "bogus-model" @@ -5211,6 +5227,7 @@ def _make_conn( conn.set_session_mode = AsyncMock() conn.set_session_model = AsyncMock() + conn.set_config_option = AsyncMock() conn.authenticate = AsyncMock() conn.close = AsyncMock() return conn @@ -5779,7 +5796,7 @@ def test_resume_with_reported_models_but_no_current_clears_stale_id(self, tmp_pa def test_resume_rejected_override_with_absent_models_clears_stale_id( self, tmp_path ): - """Resume where ``set_session_model`` is rejected AND ``load_session`` + """Resume where model reapply is rejected AND ``load_session`` omits the ``models`` block must CLEAR the stale persisted current id. This is the case the preserve-on-resume rule would otherwise keep: @@ -5807,7 +5824,7 @@ def test_resume_rejected_override_with_absent_models_clears_stale_id( conn.initialize.return_value.auth_methods = [] load_response = MagicMock(spec=[]) # no .models block conn.load_session = AsyncMock(return_value=load_response) - conn.set_session_model = AsyncMock( + conn.set_config_option = AsyncMock( side_effect=ACPRequestError(code=-32601, message="method not found") ) @@ -5958,7 +5975,7 @@ def test_fallback_replacement_clears_suffix_marker(self, tmp_path): assert agent._suffix_install_state == "pending_first_prompt" def test_resume_path_still_applies_session_mode_and_model(self, tmp_path): - """load_session must be followed by the same set_session_model and + """load_session must be followed by the same model-selection and set_session_mode calls as new_session, so a resumed session honours acp_model overrides and the bypass-permissions mode. """ @@ -5969,8 +5986,7 @@ def test_resume_path_still_applies_session_mode_and_model(self, tmp_path): "acp_session_id": "stored-sess", "acp_session_cwd": str(tmp_path), } - # Named "codex-acp"; any built-in provider routes acp_model through - # conn.set_session_model on this path. + # Named "codex-acp"; Codex routes acp_model through config options. conn = self._make_conn() conn.initialize.return_value.agent_info.name = "codex-acp" conn.initialize.return_value.auth_methods = [] @@ -5979,8 +5995,10 @@ def test_resume_path_still_applies_session_mode_and_model(self, tmp_path): conn.load_session.assert_awaited_once() conn.new_session.assert_not_awaited() - conn.set_session_model.assert_awaited_once_with( - model_id="claude-opus-4-6", + conn.set_session_model.assert_not_awaited() + conn.set_config_option.assert_awaited_once_with( + config_id="model", + value="claude-opus-4-6", session_id="stored-sess", ) conn.set_session_mode.assert_awaited_once_with( @@ -6045,13 +6063,14 @@ def test_known_provider_surfaces_applied_override(self, tmp_path): self._patched_start_acp_server(agent, state, conn=conn) - conn.set_session_model.assert_awaited_once_with( - model_id="caller-model", session_id="fresh-sess" + conn.set_session_model.assert_not_awaited() + conn.set_config_option.assert_awaited_once_with( + config_id="model", value="caller-model", session_id="fresh-sess" ) assert agent.current_model_id == "caller-model" def test_resume_rejected_override_surfaces_server_model(self, tmp_path): - """Resume where ``set_session_model`` is rejected: the live session keeps + """Resume where model reapply is rejected: the live session keeps the server default, so ``current_model_id`` must fall back to what the server reported on ``load_session`` rather than claiming the override. """ @@ -6069,7 +6088,7 @@ def test_resume_rejected_override_surfaces_server_model(self, tmp_path): conn.initialize.return_value.auth_methods = [] conn.load_session = AsyncMock(return_value=load_response) # Server rejects the reapply — swallowed, session keeps its own model. - conn.set_session_model = AsyncMock( + conn.set_config_option = AsyncMock( side_effect=ACPRequestError(code=-32601, message="method not found") ) @@ -6172,6 +6191,7 @@ def _make_conn(): conn.load_session = AsyncMock(return_value=MagicMock()) conn.set_session_mode = AsyncMock() conn.set_session_model = AsyncMock() + conn.set_config_option = AsyncMock() conn.authenticate = AsyncMock() conn.close = AsyncMock() return conn @@ -6646,6 +6666,7 @@ def _make_conn(): conn.load_session = AsyncMock(return_value=MagicMock()) conn.set_session_mode = AsyncMock() conn.set_session_model = AsyncMock() + conn.set_config_option = AsyncMock() conn.authenticate = AsyncMock() conn.close = AsyncMock() return conn @@ -6851,8 +6872,8 @@ def test_acp_model_override_wins_over_server_report(self): Mirrors the resolution logic in ``_init``: a caller-provided ``acp_model`` takes precedence over whatever the server happens to - report — both for the ``set_session_model`` path (Codex / Gemini) - and the ``session _meta`` path (Claude Code). + report — both for protocol model-selection calls and the session _meta + path (Claude Code). """ agent = _make_agent(acp_model="gpt-5") agent._current_model_id = agent.acp_model or "fallback-from-server" @@ -7027,7 +7048,7 @@ def test_returns_a_copy(self): class TestACPAgentSupportsRuntimeModelSwitch: """``supports_runtime_model_switch`` gates the live-switch picker. - ``True`` only for known providers that declare ``session/set_model`` support. + ``True`` only for known providers that declare runtime switch support. Unknown/custom providers use ``set_config_option`` for initial model selection but that is a generic config write, not a guaranteed live-switch primitive, so the picker is hidden for them. ``False`` before a session exists. @@ -7284,6 +7305,7 @@ def _make_conn(*, agent_name: str = "codex-acp", auth_method: str | None = None) conn.load_session = AsyncMock(return_value=MagicMock()) conn.set_session_mode = AsyncMock() conn.set_session_model = AsyncMock() + conn.set_config_option = AsyncMock() conn.authenticate = AsyncMock() conn.close = AsyncMock() return conn diff --git a/tests/sdk/settings/test_acp_providers.py b/tests/sdk/settings/test_acp_providers.py index 6ab36b4bc7..267f85b143 100644 --- a/tests/sdk/settings/test_acp_providers.py +++ b/tests/sdk/settings/test_acp_providers.py @@ -35,11 +35,12 @@ def test_claude_code_metadata(self): assert info.base_url_env_var == "ANTHROPIC_BASE_URL" assert info.default_session_mode == "bypassPermissions" assert "claude-agent" in info.agent_name_patterns - # Initial selection rides session/set_model — claude-agent-acp 0.30.0 - # silently ignores the session-_meta payload (#3654), which is still - # sent as best-effort (session_meta_key below). + # claude-agent-acp 0.30.0 silently ignores the session-_meta payload + # (#3654), which is still sent as best-effort (session_meta_key below). + # The authoritative path is session/set_config_option configId="model". assert info.supports_set_session_model is True assert info.supports_runtime_model_switch is True + assert info.model_selection_method == "set_config_option" assert info.session_meta_key == "claudeCode" assert info.default_model == "claude-opus-4-8" assert any(m.id == "claude-opus-4-8" for m in info.available_models) @@ -56,8 +57,9 @@ def test_codex_metadata(self): assert info.base_url_env_var == "OPENAI_BASE_URL" assert info.default_session_mode == "full-access" assert "codex-acp" in info.agent_name_patterns - assert info.supports_set_session_model is True + assert info.supports_set_session_model is False assert info.supports_runtime_model_switch is True + assert info.model_selection_method == "set_config_option" assert info.session_meta_key is None assert info.default_model == "gpt-5.5/medium" assert any(m.id == "gpt-5.5/medium" for m in info.available_models) @@ -75,6 +77,7 @@ def test_gemini_cli_metadata(self): assert "gemini-cli" in info.agent_name_patterns assert info.supports_set_session_model is True assert info.supports_runtime_model_switch is True + assert info.model_selection_method == "set_session_model" assert info.session_meta_key is None assert info.default_model == "auto-gemini-2.5" assert any(m.id == "auto-gemini-2.5" for m in info.available_models) diff --git a/tests/sdk/test_settings.py b/tests/sdk/test_settings.py index 45b7c9068d..67b1765e99 100644 --- a/tests/sdk/test_settings.py +++ b/tests/sdk/test_settings.py @@ -1009,7 +1009,7 @@ def test_acp_resolve_command_rewrites_versioned_npx_to_pinned_binary( monkeypatch.setattr(shutil, "which", _which_returning("codex-acp")) for pkg in ( "@zed-industries/codex-acp", - "@zed-industries/codex-acp@0.15.0", + "@zed-industries/codex-acp@0.16.0", "@zed-industries/codex-acp@0.11.1", ): settings = ACPAgentSettings( @@ -1032,7 +1032,7 @@ def test_acp_resolve_command_keeps_npx_when_binary_absent( assert settings.resolve_acp_command() == [ "npx", "-y", - "@zed-industries/codex-acp@0.15.0", + "@zed-industries/codex-acp@0.16.0", ] diff --git a/uv.lock b/uv.lock index 01a3129c0d..cb02664147 100644 --- a/uv.lock +++ b/uv.lock @@ -56,14 +56,14 @@ dev = [ [[package]] name = "agent-client-protocol" -version = "0.8.1" +version = "0.10.1" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "pydantic" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/1b/7b/7cdac86db388809d9e3bc58cac88cc7dfa49b7615b98fab304a828cd7f8a/agent_client_protocol-0.8.1.tar.gz", hash = "sha256:1bbf15663bf51f64942597f638e32a6284c5da918055d9672d3510e965143dbd", size = 68866, upload-time = "2026-02-13T15:34:54.567Z" } +sdist = { url = "https://files.pythonhosted.org/packages/88/a0/3b96cd8374725c69bc3dae9fcc2082f3f6cafec1be35d24d7af0f8c3265f/agent_client_protocol-0.10.1.tar.gz", hash = "sha256:355c65ca19f0568344aafc2c1552b7066a8fc491df23ab28e7e253c6c9a85a25", size = 81924, upload-time = "2026-05-24T18:46:44.444Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/4b/f3/219eeca0ad4a20843d4b9eaac5532f87018b9d25730a62a16f54f6c52d1a/agent_client_protocol-0.8.1-py3-none-any.whl", hash = "sha256:9421a11fd435b4831660272d169c3812d553bb7247049c138c3ca127e4b8af8e", size = 54529, upload-time = "2026-02-13T15:34:53.344Z" }, + { url = "https://files.pythonhosted.org/packages/7b/18/d8c7ff337cf621ea79a84006a7252ff057bfb5767549bb102cc6649f4ec2/agent_client_protocol-0.10.1-py3-none-any.whl", hash = "sha256:a03d3198f4d772f2e0ec012c00ac1cce131b4710220a3dc9fae3c991d047c750", size = 65401, upload-time = "2026-05-24T18:46:43.202Z" }, ] [[package]] @@ -2520,7 +2520,7 @@ boto3 = [ [package.metadata] requires-dist = [ - { name = "agent-client-protocol", specifier = ">=0.8.1" }, + { name = "agent-client-protocol", specifier = ">=0.10.1" }, { name = "boto3", marker = "extra == 'boto3'", specifier = ">=1.35.0" }, { name = "deprecation", specifier = ">=2.1.0" }, { name = "fakeredis", extras = ["lua"], specifier = ">=2.32.1" }, From b04967515174e0d8df2420f77cdf8382b5981940 Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Fri, 12 Jun 2026 19:05:02 -0400 Subject: [PATCH 2/8] Preserve ACP provider info constructor order --- .../openhands/sdk/settings/acp_providers.py | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/openhands-sdk/openhands/sdk/settings/acp_providers.py b/openhands-sdk/openhands/sdk/settings/acp_providers.py index 3938b575bf..2ed7d3bd2a 100644 --- a/openhands-sdk/openhands/sdk/settings/acp_providers.py +++ b/openhands-sdk/openhands/sdk/settings/acp_providers.py @@ -238,17 +238,6 @@ class ACPProviderInfo: signature break; the built-in providers set it explicitly. """ - model_selection_method: Literal["set_session_model", "set_config_option"] = ( - "set_session_model" - ) - """Protocol method used to apply model selection for this provider. - - ``"set_session_model"`` issues ACP ``session/set_model``. ``"set_config_option"`` - issues ACP ``session/set_config_option`` with ``configId="model"``. The - default preserves the historical behavior for external callers constructing - this dataclass positionally. - """ - file_secrets: tuple[ACPFileSecretSpec, ...] = field(default=(), compare=False) """Reserved file-content credential secrets this provider authenticates from. @@ -290,6 +279,17 @@ class ACPProviderInfo: :attr:`~openhands.sdk.agent.ACPAgent.acp_isolate_data_dir`. """ + model_selection_method: Literal["set_session_model", "set_config_option"] = ( + "set_session_model" + ) + """Protocol method used to apply model selection for this provider. + + ``"set_session_model"`` issues ACP ``session/set_model``. ``"set_config_option"`` + issues ACP ``session/set_config_option`` with ``configId="model"``. The + default preserves the historical behavior for external callers constructing + this dataclass positionally. + """ + # --------------------------------------------------------------------------- # Curated ``acp_model`` candidate lists for the built-in providers. From 0c099b246a3a0998562b777bff48c856b531633e Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Fri, 12 Jun 2026 19:13:09 -0400 Subject: [PATCH 3/8] Clarify ACP config option fallback --- openhands-sdk/openhands/sdk/agent/acp_agent.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 6e5f1dae95..689b140c12 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -602,8 +602,8 @@ async def _apply_acp_model_selection( return True return False - # For unknown/custom providers, try the generic set_config_option method - # which is a standard ACP protocol method for setting configuration options. + # For unknown/custom providers, try set_config_option as a best-effort + # standard ACP configuration method. Some custom servers may still reject it. await conn.set_config_option( config_id="model", value=acp_model, From 9b1bccaad02774d8bbc7166ce8d96cfe32228f41 Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Fri, 12 Jun 2026 19:20:09 -0400 Subject: [PATCH 4/8] Document ACP model selection metadata --- openhands-sdk/openhands/sdk/agent/acp_agent.py | 10 +++------- openhands-sdk/openhands/sdk/settings/acp_providers.py | 9 +++++---- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 689b140c12..8356e15576 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -560,9 +560,7 @@ async def _maybe_set_session_model( return False provider = detect_acp_provider_by_agent_name(agent_name) try: - return await _apply_acp_model_selection( - conn, provider, agent_name, session_id, acp_model - ) + return await _apply_acp_model(conn, provider, agent_name, session_id, acp_model) except ACPRequestError as e: if provider is not None: raise @@ -576,7 +574,7 @@ async def _maybe_set_session_model( return False -async def _apply_acp_model_selection( +async def _apply_acp_model( conn: ClientSideConnection, provider: ACPProviderInfo | None, agent_name: str, @@ -655,9 +653,7 @@ async def _reapply_session_model_on_resume( if provider is not None and not provider.supports_runtime_model_switch: return False try: - return await _apply_acp_model_selection( - conn, provider, agent_name, session_id, acp_model - ) + return await _apply_acp_model(conn, provider, agent_name, session_id, acp_model) except ACPRequestError as e: logger.warning( "Could not reapply model %r on resumed session %s (%s); the live " diff --git a/openhands-sdk/openhands/sdk/settings/acp_providers.py b/openhands-sdk/openhands/sdk/settings/acp_providers.py index 2ed7d3bd2a..865169f264 100644 --- a/openhands-sdk/openhands/sdk/settings/acp_providers.py +++ b/openhands-sdk/openhands/sdk/settings/acp_providers.py @@ -177,10 +177,11 @@ class ACPProviderInfo: ``session/set_model`` is not the only protocol-level model-selection path: current ACP servers may expose ``session/set_config_option`` with - ``configId="model"`` instead. Use :attr:`model_selection_method` to decide - which call to issue. claude-agent-acp was ``False`` until 0.30.0 was found - to silently ignore the session-``_meta`` selection it relied on (#3654); - its ``_meta`` payload is still sent alongside (see + ``configId="model"`` instead. This field is legacy capability metadata, not + the model-selection dispatch gate; use :attr:`model_selection_method` to + decide which call to issue. claude-agent-acp was ``False`` until 0.30.0 was + found to silently ignore the session-``_meta`` selection it relied on + (#3654); its ``_meta`` payload is still sent alongside (see :attr:`session_meta_key`). This is **independent of** runtime switching capability — see From 08e9c4774c5f395c7a826a639c151447acad7c4d Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Fri, 12 Jun 2026 19:32:15 -0400 Subject: [PATCH 5/8] Clarify ACP provider metadata deprecation --- openhands-sdk/openhands/sdk/settings/acp_providers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openhands-sdk/openhands/sdk/settings/acp_providers.py b/openhands-sdk/openhands/sdk/settings/acp_providers.py index 865169f264..937035e2e6 100644 --- a/openhands-sdk/openhands/sdk/settings/acp_providers.py +++ b/openhands-sdk/openhands/sdk/settings/acp_providers.py @@ -178,7 +178,8 @@ class ACPProviderInfo: ``session/set_model`` is not the only protocol-level model-selection path: current ACP servers may expose ``session/set_config_option`` with ``configId="model"`` instead. This field is legacy capability metadata, not - the model-selection dispatch gate; use :attr:`model_selection_method` to + the model-selection dispatch gate. Deprecated for dispatch decisions: + external consumers should migrate to :attr:`model_selection_method` to decide which call to issue. claude-agent-acp was ``False`` until 0.30.0 was found to silently ignore the session-``_meta`` selection it relied on (#3654); its ``_meta`` payload is still sent alongside (see From 5fec2c0173787b66c287c31a0c53f3dd01676434 Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Fri, 12 Jun 2026 19:49:22 -0400 Subject: [PATCH 6/8] Add ACP model selection method alias --- openhands-sdk/openhands/sdk/settings/acp_providers.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/openhands-sdk/openhands/sdk/settings/acp_providers.py b/openhands-sdk/openhands/sdk/settings/acp_providers.py index 937035e2e6..beb0d48384 100644 --- a/openhands-sdk/openhands/sdk/settings/acp_providers.py +++ b/openhands-sdk/openhands/sdk/settings/acp_providers.py @@ -42,6 +42,9 @@ from pydantic import BaseModel, ConfigDict, Field, field_validator +ModelSelectionMethod = Literal["set_session_model", "set_config_option"] + + @dataclass(frozen=True) class ACPModelOption: """One selectable model for a built-in ACP provider's model picker.""" @@ -281,9 +284,7 @@ class ACPProviderInfo: :attr:`~openhands.sdk.agent.ACPAgent.acp_isolate_data_dir`. """ - model_selection_method: Literal["set_session_model", "set_config_option"] = ( - "set_session_model" - ) + model_selection_method: ModelSelectionMethod = "set_session_model" """Protocol method used to apply model selection for this provider. ``"set_session_model"`` issues ACP ``session/set_model``. ``"set_config_option"`` From 263a782f5c3442d2a26e20669c9d0b8b745b47e4 Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Fri, 12 Jun 2026 20:15:15 -0400 Subject: [PATCH 7/8] Clarify ACP model dispatch fallback --- openhands-sdk/openhands/sdk/agent/acp_agent.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 8356e15576..39b40a222d 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -595,10 +595,11 @@ async def _apply_acp_model( provider.key, ) return True - if provider.supports_set_session_model: + elif provider.supports_set_session_model: await conn.set_session_model(model_id=acp_model, session_id=session_id) return True - return False + else: + return False # For unknown/custom providers, try set_config_option as a best-effort # standard ACP configuration method. Some custom servers may still reject it. From a4f7924f9cd7aaf80a461310b5f51971984fdbc5 Mon Sep 17 00:00:00 2001 From: Graham Neubig <398875+neubig@users.noreply.github.com> Date: Fri, 12 Jun 2026 23:40:46 -0400 Subject: [PATCH 8/8] fix(sdk): split Codex ACP model config options --- .../openhands/sdk/agent/acp_agent.py | 53 ++++++++-- tests/sdk/agent/test_acp_agent.py | 97 ++++++++++++++----- 2 files changed, 118 insertions(+), 32 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 39b40a222d..0ace27b531 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/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 diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 8efc00dcbe..6357e2b003 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/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,6 +4420,20 @@ 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 # --------------------------------------------------------------------------- @@ -4426,17 +4441,23 @@ def test_noop_when_caller_already_set_provider(self): 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,10 +4650,18 @@ 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 @@ -4634,11 +4669,17 @@ 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.