Skip to content

fix(acp): pass agent.disabled_toolsets to AIAgent — config-disabled tools stay executable over ACP - #57421

Open
5uck1ess wants to merge 2 commits into
NousResearch:mainfrom
5uck1ess:fix/acp-disabled-toolsets
Open

fix(acp): pass agent.disabled_toolsets to AIAgent — config-disabled tools stay executable over ACP#57421
5uck1ess wants to merge 2 commits into
NousResearch:mainfrom
5uck1ess:fix/acp-disabled-toolsets

Conversation

@5uck1ess

@5uck1ess 5uck1ess commented Jul 3, 2026

Copy link
Copy Markdown

Problem

agent.disabled_toolsets in config.yaml is honored by the CLI (cli.py: CLI_CONFIG['agent'].get('disabled_toolsets')) and the gateway (gateway/run.py:12782) — both pass it to AIAgent. The ACP adapter's _make_agent never does.

Consequence: state.agent.disabled_toolsets is always None on the ACP path, so the tool-registry rebuild in acp_adapter/server.py calls get_tool_definitions(enabled_toolsets=..., disabled_toolsets=None) and every tool in the enabled toolsets stays registered and executable — including toolsets the user explicitly disabled in config.

Observed live driving hermes over ACP: a todo tool call executed from a profile whose config lists todo in agent.disabled_toolsets.

Related family of reports on other surfaces: #36729 (WebUI), #54433 (desktop). This PR fixes the ACP surface. Sibling PR #57418 fixes the same construction gap for fallback_model (#18452) — _make_agent builds its kwargs from scratch and has drifted from what the CLI/gateway pass.

Fix

Read agent.disabled_toolsets from the loaded config in _make_agent and pass it to AIAgent, mirroring the CLI and gateway. Guarded by an isinstance check consistent with the surrounding config handling.

Verification

Two new tests in tests/acp/test_session.py (chain passes through when configured; key absent otherwise). Full file: 47 passed.

https://claude.ai/code/session_01YNvCUipheR7yx4VorUL2jW

…ools stayed executable over ACP

The CLI (cli.py: CLI_CONFIG['agent'].get('disabled_toolsets')) and the
gateway (gateway/run.py) both read agent.disabled_toolsets from config
and pass it to AIAgent. The ACP adapter's _make_agent never did, so
state.agent.disabled_toolsets was always None and the ACP tool-registry
rebuild (acp_adapter/server.py -> get_tool_definitions) included every
tool in the enabled toolsets — a toolset the user disabled in config
(todo, browser, ...) remained fully executable in editor/ACP sessions.

Observed live: a 'todo' tool call executed from a profile whose config
lists todo in agent.disabled_toolsets.

Read agent.disabled_toolsets in _make_agent and pass it through,
mirroring the CLI and gateway paths.

Claude-Session: https://claude.ai/code/session_01YNvCUipheR7yx4VorUL2jW
@alt-glitch alt-glitch added type/bug Something isn't working comp/acp Agent Communication Protocol adapter P4 Best-effort: we will get to it when we get to it (no commitment) labels Jul 3, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing this ACP-specific construction gap. The premise is confirmed on current main: acp_adapter/session.py:601-645 loads config and builds AIAgent kwargs without agent.disabled_toolsets; agent/agent_init.py:625-626 stores that value, and model_tools.py:395-427 enforces it when supplied.

Problems

  • acp_adapter/server.py:1793-1796 still rebuilds ACP /tools output without state.agent.disabled_toolsets. After this patch, execution is filtered, but /tools can still advertise disabled tools.
  • The new tests assert only fake-agent kwargs, not the filtered tool surface.

Suggested changes

  • Pass getattr(state.agent, "disabled_toolsets", None) to the /tools get_tool_definitions call, matching acp_adapter/server.py:838-842.
  • Add a behavioral assertion that a configured disabled toolset is absent from ACP-generated definitions.

This is an automated hermes-sweeper review.

…l coverage

Review follow-up: _cmd_tools rebuilt its listing without
state.agent.disabled_toolsets, so a config-disabled toolset was filtered
from execution but still advertised by /tools. Pass it through, matching
the session tool-surface rebuild.

New tests exercise the real get_tool_definitions (no patching) and assert
a disabled toolset is absent from both the /tools listing and the rebuilt
valid_tool_names — with a baseline assertion that the toolset is present
when nothing is disabled, so the check cannot pass vacuously.
@5uck1ess

Copy link
Copy Markdown
Author

Both points addressed in 040b08c:

  • _cmd_tools now passes getattr(state.agent, "disabled_toolsets", None) to its get_tool_definitions call, matching the session tool-surface rebuild, so /tools no longer advertises config-disabled tools it won't execute.
  • Added behavioral coverage that runs the real get_tool_definitions (no patching): one test asserts a disabled toolset is absent from the /tools listing, one asserts it is absent from the rebuilt valid_tool_names after MCP registration. The /tools test includes a baseline assertion that the toolset IS present when nothing is disabled, so it can't pass vacuously — and it fails when the server.py change is reverted.

tests/acp/test_server.py + tests/acp/test_session.py: 129 passed.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@ScaleLeanChris

Copy link
Copy Markdown
Contributor

Independent current-main validation supports keeping this original PR rather than the newer duplicate #74617.

  • On main at ba7d214, a constructor-boundary regression test fails because agent.disabled_toolsets never reaches AIAgent.
  • Applying the session pass-through makes 54 focused ACP tests pass across the command, session, server, and MCP paths.
  • A real SessionManager built from a temporary HERMES_HOME config retained terminal, browser, computer_use, and code_execution as disabled. Its generated tool surface exposed none of those toolsets.
  • Ruff, the Windows-footgun scan, and git diff --check pass.

This PR is broader than #74617 because it also filters the ACP /tools listing and already has non-vacuous behavioral coverage for both advertised and executable tools. GitHub currently reports it mergeable and rebaseable. I recommend refreshing its base and closing the duplicate.

In short, this is the original branch worth “Stayin' Alive.”

@alt-glitch alt-glitch added area/config Config system, migrations, profiles and removed sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/acp Agent Communication Protocol adapter P4 Best-effort: we will get to it when we get to it (no commitment) sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants