fix(acp): resolve ACP toolsets through the shared platform resolver
A fresh ACP agent hardcoded enabled_toolsets=["hermes-acp"], so platform_toolsets.acp never narrowed the editor tool surface, unlike the gateway, cron and api_server which all resolve via hermes_cli.tools_config._get_platform_tools. Resolve the ACP base the same way (ACP keeps appending its own mcp-<server> entries), and treat only None, not an explicit empty list, as "use the hermes-acp default" in the /tools and MCP-refresh rebuilds so a deny-all list cannot re-widen mid-session. With the unconfigured default the resolved tool definitions are byte-identical to the hermes-acp composite, so existing sessions keep the same tool list and prompt cache. The fresh-session assertion in test_make_agent_prefers_passed_toolsets_over_config_servers now checks membership of the config MCP entry: the resolver returns the expanded toolset keys rather than the bare composite name. Refs #74582, #79516. Credit: #64045 (@israellot), #80309 (@thatssoheil), #106834 (@nicolasramos) proposed the resolver routing.
This commit is contained in:
@@ -156,7 +156,7 @@ class SlashCommandsMixin:
|
||||
from types import SimpleNamespace
|
||||
from agent.memory_manager import inject_memory_provider_tools
|
||||
|
||||
toolsets = _expand_acp_enabled_toolsets(getattr(state.agent, "enabled_toolsets", None) or ["hermes-acp"])
|
||||
toolsets = _expand_acp_enabled_toolsets(getattr(state.agent, "enabled_toolsets", None))
|
||||
tools = get_tool_definitions(
|
||||
enabled_toolsets=toolsets,
|
||||
disabled_toolsets=getattr(state.agent, "disabled_toolsets", None),
|
||||
|
||||
@@ -447,7 +447,7 @@ class HermesACPAgent(SlashCommandsMixin, acp.Agent):
|
||||
|
||||
agent = state.agent
|
||||
agent.enabled_toolsets = _expand_acp_enabled_toolsets(
|
||||
getattr(agent, "enabled_toolsets", None) or ["hermes-acp"],
|
||||
getattr(agent, "enabled_toolsets", None),
|
||||
mcp_server_names=[s.name for s in mcp_servers],
|
||||
)
|
||||
agent.tools = get_tool_definitions(
|
||||
|
||||
@@ -103,7 +103,7 @@ def _register_task_cwd(task_id: str, cwd: str) -> None:
|
||||
def _expand_acp_enabled_toolsets(toolsets: List[str] | None = None,
|
||||
mcp_server_names: List[str] | None = None) -> List[str]:
|
||||
"""Return ACP toolsets plus explicit MCP server toolsets for this session."""
|
||||
names = [n for n in (toolsets or ["hermes-acp"]) if n]
|
||||
names = [n for n in (["hermes-acp"] if toolsets is None else toolsets) if n]
|
||||
names += [f"mcp-{s}" for s in (mcp_server_names or []) if s]
|
||||
return list(dict.fromkeys(names))
|
||||
|
||||
@@ -458,7 +458,7 @@ class SessionManager:
|
||||
requested_provider: str | None = None, base_url: str | None = None, api_mode: str | None = None,
|
||||
enabled_toolsets: list[str] | None = None, disabled_toolsets: list[str] | None = None):
|
||||
"""``enabled_toolsets``/``disabled_toolsets`` carry a live session's toolsets into a rebuild; ``None`` derives
|
||||
them from the config-declared MCP servers (fresh session)."""
|
||||
them from config (fresh session)."""
|
||||
if self._agent_factory is not None:
|
||||
return self._agent_factory()
|
||||
|
||||
@@ -466,6 +466,7 @@ class SessionManager:
|
||||
from agent.skill_utils import parse_config_string_list
|
||||
from hermes_cli.config import load_config
|
||||
from hermes_cli.runtime_provider import resolve_runtime_provider
|
||||
from hermes_cli.tools_config import _get_platform_tools
|
||||
from hermes_constants import resolve_reasoning_config
|
||||
|
||||
config = load_config()
|
||||
@@ -484,8 +485,10 @@ class SessionManager:
|
||||
]
|
||||
kwargs = {
|
||||
"platform": "acp", "quiet_mode": True, "session_id": session_id, "session_db": self._get_db(),
|
||||
"enabled_toolsets": (list(enabled_toolsets) if enabled_toolsets is not None
|
||||
else _expand_acp_enabled_toolsets(["hermes-acp"], mcp_server_names=configured_mcp_servers)),
|
||||
# The same per-platform resolver as the gateway/cron/api_server: platform_toolsets.acp wins, else hermes-acp.
|
||||
"enabled_toolsets": (list(enabled_toolsets) if enabled_toolsets is not None else _expand_acp_enabled_toolsets(
|
||||
sorted(_get_platform_tools(config, "acp", include_default_mcp_servers=False)),
|
||||
mcp_server_names=configured_mcp_servers)),
|
||||
# agent.disabled_toolsets is subtracted at tool granularity by the agent, as on the CLI/gateway/cron.
|
||||
"disabled_toolsets": (list(disabled_toolsets) if disabled_toolsets is not None
|
||||
else parse_config_string_list((config.get("agent") or {}).get("disabled_toolsets")) or None),
|
||||
|
||||
@@ -135,9 +135,44 @@ class TestCreateSession:
|
||||
session_id="rebuilt", cwd=".", enabled_toolsets=["hermes-acp", "mcp-acp-server"], disabled_toolsets=["browser"],
|
||||
)
|
||||
|
||||
assert (seen[0]["enabled_toolsets"], seen[0]["disabled_toolsets"]) == (["hermes-acp", "mcp-cfg-server"], None)
|
||||
assert "mcp-cfg-server" in seen[0]["enabled_toolsets"] and seen[0]["disabled_toolsets"] is None
|
||||
assert (seen[1]["enabled_toolsets"], seen[1]["disabled_toolsets"]) == (["hermes-acp", "mcp-acp-server"], ["browser"])
|
||||
|
||||
@pytest.mark.parametrize("config, offered, withheld", [
|
||||
# agent.disabled_toolsets, in the JSON-string shape `hermes config set` stores (#74582).
|
||||
({"agent": {"disabled_toolsets": "['code_execution']"}}, "file", "code_execution"),
|
||||
# platform_toolsets.acp narrows the surface like every other platform (#79516).
|
||||
({"platform_toolsets": {"acp": ["file"]}}, "file", "code_execution"),
|
||||
])
|
||||
def test_fresh_agent_tool_surface_honours_toolset_config(self, monkeypatch, config, offered, withheld):
|
||||
"""A fresh ACP agent resolves its tools like the gateway/cron: the real tool surface built from its
|
||||
kwargs carries the offered toolset and none of the withheld one."""
|
||||
from model_tools import get_tool_definitions
|
||||
from toolsets import resolve_toolset
|
||||
|
||||
seen: list[dict] = []
|
||||
|
||||
class FakeAgent:
|
||||
def __init__(self, **kwargs):
|
||||
seen.append(kwargs)
|
||||
|
||||
monkeypatch.setattr("run_agent.AIAgent", FakeAgent)
|
||||
monkeypatch.setattr("hermes_cli.config.load_config", lambda: {"model": {"default": "m"}, **config})
|
||||
monkeypatch.setattr("hermes_cli.runtime_provider.resolve_runtime_provider", lambda **_kw: {})
|
||||
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)
|
||||
|
||||
SessionManager(db=None)._make_agent(session_id="fresh", cwd=".")
|
||||
|
||||
def surface(enabled, disabled=None) -> set:
|
||||
return {t["function"]["name"] for t in get_tool_definitions(
|
||||
enabled_toolsets=enabled, disabled_toolsets=disabled, quiet_mode=True)}
|
||||
|
||||
assert set(resolve_toolset(withheld)) <= surface(["hermes-acp"]) # non-vacuous: offered by default
|
||||
names = surface(seen[0]["enabled_toolsets"], seen[0]["disabled_toolsets"])
|
||||
assert set(resolve_toolset(offered)) <= names
|
||||
assert not names & set(resolve_toolset(withheld))
|
||||
|
||||
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
|
||||
|
||||
@@ -119,7 +119,8 @@ Examples:
|
||||
```text
|
||||
new_session(cwd)
|
||||
-> create SessionState
|
||||
-> create AIAgent(platform="acp", enabled_toolsets=["hermes-acp"])
|
||||
-> create AIAgent(platform="acp", enabled_toolsets=<platform_toolsets.acp, default hermes-acp>,
|
||||
disabled_toolsets=<agent.disabled_toolsets>)
|
||||
-> bind task_id/session_id to cwd override
|
||||
|
||||
prompt(..., session_id)
|
||||
|
||||
@@ -35,6 +35,10 @@ Hermes runs with a curated `hermes-acp` toolset designed for editor workflows. I
|
||||
|
||||
It intentionally excludes things that do not fit typical editor UX, such as messaging delivery and cronjob management.
|
||||
|
||||
The toolset resolves like every other platform: `platform_toolsets.acp` in
|
||||
`config.yaml` (or `hermes tools`) replaces the `hermes-acp` default, and
|
||||
`agent.disabled_toolsets` removes toolsets from every ACP session.
|
||||
|
||||
## Installation
|
||||
|
||||
Install Hermes normally, then add the ACP extra from the install checkout:
|
||||
@@ -268,12 +272,10 @@ therefore runs shell commands on the host without prompting. I asked one to run
|
||||
Selecting `Anyone` hands that same shell access to every author who can reach
|
||||
the channel. Buzz does not warn when you pick it.
|
||||
|
||||
Neither of the obvious mitigations works today:
|
||||
|
||||
- `approvals.mode: manual` does make Hermes raise the permission request, but
|
||||
Buzz auto-approves it and the command still runs.
|
||||
- `platform_toolsets.acp` does not narrow the ACP toolset, so it cannot be used
|
||||
to drop `terminal`.
|
||||
`approvals.mode: manual` does not help: Hermes raises the permission request,
|
||||
but Buzz auto-approves it and the command still runs. To take the shell away,
|
||||
narrow the toolset instead: set `platform_toolsets.acp` to a list without
|
||||
`terminal` and `code_execution`, or add them to `agent.disabled_toolsets`.
|
||||
|
||||
`!shutdown` from the owner stops the agent in any mode, and Buzz ignores that
|
||||
command from everyone else.
|
||||
|
||||
Reference in New Issue
Block a user