From 643b94bf9cef98969affa6de386202e0707a4379 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sat, 19 Sep 2026 00:00:00 -0700 Subject: [PATCH] fix(acp): rejected session/set_model is an invalid-params error and a failed rebuild reports its real cause set_session_model already validates through hermes_cli.model_switch.switch_model (11576390fec), so an unadvertised modelId is refused before the session mutates (#72439's main atom). The rejection surfaced as JSON-RPC -32603 "Internal error" though, which clients attribute to the agent rather than to the request; it is now RequestError.invalid_params (-32602) carrying the switch_model reason. _switch_model also assigned state.model before the rebuild, so an agent-build failure left the session persisted on a model the live agent did not run; the assignment now follows the successful build. _make_agent swallowed a resolve_runtime_provider failure at debug and built a bare AIAgent, which dies with the first-run "No LLM provider configured. Run `hermes setup`" text on a configured machine (#91090's residual ask). The fallback stays, but when the bare build fails the swallowed resolution error (revoked OAuth, disabled provider, ...) is raised instead, chained to the fallback failure. Direction credited to @z0zero (#72579: -32602 + atomic session state) and @webtecnica (#91100: do not swallow the resolution failure). Co-authored-by: z0zero Co-authored-by: webtecnica --- acp_adapter/server.py | 16 +++++++--- acp_adapter/session.py | 14 +++++++-- ...t_acp_dashboard_model_switch_validation.py | 30 +++++++++++++++++++ tests/acp_adapter/test_session.py | 29 ++++++++++++++++++ 4 files changed, 83 insertions(+), 6 deletions(-) diff --git a/acp_adapter/server.py b/acp_adapter/server.py index 719766c1da..15ddc56cbe 100644 --- a/acp_adapter/server.py +++ b/acp_adapter/server.py @@ -335,7 +335,6 @@ class HermesACPAgent(SlashCommandsMixin, acp.Agent): if not result.success: raise ValueError(result.error_message or f"Cannot switch to {raw_model}") target_provider, new_model = result.target_provider, result.new_model - state.model = new_model endpoint: dict[str, Any] = {} if keep_endpoint and not (current_provider and target_provider != current_provider): endpoint = { @@ -343,12 +342,15 @@ class HermesACPAgent(SlashCommandsMixin, acp.Agent): } # ACP-provided MCP servers live only on the running agent's toolsets (``_register_session_mcp_servers``); # a rebuild that re-derived them from config would silently drop every session MCP tool (#42719). - state.agent = self.session_manager._make_agent( + agent = self.session_manager._make_agent( session_id=state.session_id, cwd=state.cwd, model=new_model, requested_provider=target_provider, **endpoint, enabled_toolsets=getattr(state.agent, "enabled_toolsets", None), disabled_toolsets=getattr(state.agent, "disabled_toolsets", None), ) + # Assign only after the rebuild succeeded so a failed switch leaves the session on its + # working model instead of a model/agent mismatch that persists via save_session. + state.agent, state.model = agent, new_model self.session_manager.save_session(state.session_id) return current_provider, target_provider, new_model @@ -998,8 +1000,14 @@ class HermesACPAgent(SlashCommandsMixin, acp.Agent): if state: # switch_model() does synchronous network I/O (models.dev, custom-endpoint probes, # ~10 s cold) — off the loop, like the gateway, so other ACP sessions keep flowing. - _old, requested_provider, resolved_model = await asyncio.to_thread( - self._switch_model, state, model_id, keep_endpoint=True) + try: + _old, requested_provider, resolved_model = await asyncio.to_thread( + self._switch_model, state, model_id, keep_endpoint=True) + except ValueError as exc: + # A model no provider can serve is a bad ``modelId`` param (-32602), not an agent + # internal error (-32603): the client attributes it to the request, not to Hermes (#72439). + from acp.exceptions import RequestError + raise RequestError.invalid_params({"details": str(exc)}) from exc logger.info( "Session %s: model switched to %s via provider %s", session_id, resolved_model, requested_provider ) diff --git a/acp_adapter/session.py b/acp_adapter/session.py index 30863edfef..2d44a4335e 100644 --- a/acp_adapter/session.py +++ b/acp_adapter/session.py @@ -404,6 +404,7 @@ class SessionManager: "model": model or default_model, "cwd": cwd, } + resolve_error: Exception | None = None try: runtime = resolve_runtime_provider( requested=requested_provider or config_provider, target_model=(model or default_model) or None) @@ -412,7 +413,8 @@ class SessionManager: "base_url": base_url or runtime.get("base_url"), "api_key": runtime.get("api_key"), "command": runtime.get("command"), "args": list(runtime.get("args") or []), }) - except Exception: + except Exception as exc: + resolve_error = exc logger.debug("ACP session falling back to default provider resolution", exc_info=True) _register_task_cwd(session_id, cwd) @@ -430,7 +432,15 @@ class SessionManager: except Exception: logger.debug("ACP: bounded MCP discovery wait failed", exc_info=True) - agent = AIAgent(**kwargs) + try: + agent = AIAgent(**kwargs) + except Exception as exc: + # The bare-AIAgent fallback dies with "No LLM provider configured. Run `hermes setup`" on a + # machine that is configured and was working a call earlier; the swallowed resolution + # failure (revoked OAuth, disabled provider, ...) is the actionable error (#91090). + if resolve_error is not None: + raise resolve_error from exc + raise # ACP stdio: stdout is protocol-only JSON-RPC; agent chatter goes to stderr. agent._print_fn = _acp_stderr_print return agent diff --git a/tests/acp_adapter/test_acp_dashboard_model_switch_validation.py b/tests/acp_adapter/test_acp_dashboard_model_switch_validation.py index 4ceb569dea..3d332f86f1 100644 --- a/tests/acp_adapter/test_acp_dashboard_model_switch_validation.py +++ b/tests/acp_adapter/test_acp_dashboard_model_switch_validation.py @@ -113,3 +113,33 @@ def test_acp_switch_model_carries_the_live_agent_toolsets_into_the_rebuild(monke assert made["enabled_toolsets"] == ["hermes-acp", "mcp-demo-search"] assert made["disabled_toolsets"] == ["browser"] + + +def test_acp_set_session_model_rejection_is_invalid_params_and_leaves_session_untouched(monkeypatch): + """#72439: a ``modelId`` no provider can serve is a bad param (-32602 with the switch_model + reason), not a -32603 internal error; and a rebuild that blows up after switch_model accepted + the model must not leave ``state.model`` pointing at a model the live agent does not run.""" + import asyncio + + from acp.exceptions import RequestError + + monkeypatch.setattr("hermes_cli.model_switch.switch_model", + lambda **_kw: ModelSwitchResult(success=False, error_message="`nope` is not a model")) + agent, _made = _acp_agent() + state = _state() + agent.session_manager.get_session = lambda sid: state + with pytest.raises(RequestError) as exc: + asyncio.run(agent.set_session_model("nope", "s1")) + assert exc.value.code == -32602 and exc.value.data == {"details": "`nope` is not a model"} + + monkeypatch.setattr("hermes_cli.model_switch.switch_model", + lambda **_kw: ModelSwitchResult(success=True, new_model="other", target_provider="anthropic")) + + def _boom(**_kw): + raise RuntimeError("No Codex credentials stored") + + agent.session_manager._make_agent = _boom + old_agent = state.agent + with pytest.raises(RuntimeError, match="No Codex credentials"): + agent._switch_model(state, "other") + assert state.model == "claude-sonnet-5" and state.agent is old_agent diff --git a/tests/acp_adapter/test_session.py b/tests/acp_adapter/test_session.py index 07ce76751d..fe28a7cdff 100644 --- a/tests/acp_adapter/test_session.py +++ b/tests/acp_adapter/test_session.py @@ -142,6 +142,35 @@ class TestCreateSession: assert (seen[0]["enabled_toolsets"], seen[0]["disabled_toolsets"]) == (["hermes-acp", "mcp-cfg-server"], None) assert (seen[1]["enabled_toolsets"], seen[1]["disabled_toolsets"]) == (["hermes-acp", "mcp-acp-server"], ["browser"]) + def test_make_agent_surfaces_the_provider_resolution_failure(self, monkeypatch): + """#91090: when ``resolve_runtime_provider`` fails, the bare-AIAgent fallback dies with the + first-run "No LLM provider configured" text; the operator must get the swallowed cause + instead. The fallback still stands when the bare build succeeds.""" + def _no_creds(**_kw): + raise RuntimeError("No Codex credentials stored. Run `hermes auth add openai-codex`") + + class BareFails: + def __init__(self, **kwargs): + raise RuntimeError("No LLM provider configured. Run `hermes setup`") + + class BareWorks: + def __init__(self, **kwargs): + self.kwargs = kwargs + + monkeypatch.setattr("hermes_cli.config.load_config", lambda: {"model": {"default": "m", "provider": "openai-codex"}}) + monkeypatch.setattr("hermes_cli.runtime_provider.resolve_runtime_provider", _no_creds) + monkeypatch.setattr("hermes_cli.mcp_startup.ensure_mcp_discovery_before_agent_build", lambda **_kw: None) + monkeypatch.setattr("acp_adapter.session._register_task_cwd", lambda task_id, cwd: None) + manager = SessionManager(db=None) + + monkeypatch.setattr("run_agent.AIAgent", BareFails) + with pytest.raises(RuntimeError, match="No Codex credentials stored") as exc: + manager._make_agent(session_id="rebuilt", cwd=".", requested_provider="openai-codex") + assert "No LLM provider configured" in str(exc.value.__cause__) + + monkeypatch.setattr("run_agent.AIAgent", BareWorks) + assert "provider" not in manager._make_agent(session_id="fresh", cwd=".").kwargs +