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
(11576390fe), 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 <z0zero@users.noreply.github.com>
Co-authored-by: webtecnica <webtecnica@users.noreply.github.com>
This commit is contained in:
@@ -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
|
||||
)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user