Skip to content

feat(acp): honor platform_toolsets.acp config for ACP tool surface - #64045

Open
israellot wants to merge 2 commits into
NousResearch:mainfrom
israellot:feat/acp-platform-toolsets
Open

feat(acp): honor platform_toolsets.acp config for ACP tool surface#64045
israellot wants to merge 2 commits into
NousResearch:mainfrom
israellot:feat/acp-platform-toolsets

Conversation

@israellot

Copy link
Copy Markdown
Contributor

Problem

ACP (editor integration) sessions hard-code the hermes-acp composite toolset in three places (acp_adapter/session.py agent construction, acp_adapter/server.py MCP-registration tool refresh and /tools command). As a result, platform_toolsets in config.yaml — the mechanism every other platform uses — is silently ignored for ACP, and users cannot trim the ACP tool surface (e.g. drop browser automation to save ~10 tool schemas of context per request in VS Code/Zed sessions).

Change

  • New _acp_base_toolsets() helper in acp_adapter/session.py: reads platform_toolsets.acp from config; an explicit non-empty list wins, anything else (missing key, non-list, empty) falls back to the hermes-acp composite.
  • _expand_acp_enabled_toolsets(None, ...) now resolves through the helper; the three hard-coded ["hermes-acp"] call sites pass None instead.
  • Backward compatible: installs without the config key behave exactly as before.

Usage

platform_toolsets:
  acp: [web, terminal, file, code_execution, vision, skills, todo, memory, session_search, delegation]

Testing

  • New: test_create_session_honors_platform_toolsets_acp (config list replaces composite, MCP toolsets still appended) and test_acp_base_toolsets_fallback_on_empty_or_missing (5 fallback shapes).
  • pytest tests/acp tests/acp_adapter → 327 passed.
  • Verified live: an ACP session under a profile with the config key resolves 15 tools with zero browser_*; a profile without it falls back to hermes-acp unchanged.

ACP sessions previously hard-coded the hermes-acp composite toolset,
so 'hermes tools disable <toolset>' (which edits platform_toolsets)
had no effect on editor-integration sessions — users could not trim
the ACP tool surface (e.g. drop browser automation) the way they can
for cli and messaging platforms.

Resolve the base toolsets through a new _acp_base_toolsets() helper:
an explicit platform_toolsets.acp list in config.yaml wins; missing,
non-list, or empty values fall back to the hermes-acp composite, so
existing installs are unaffected.
@alt-glitch alt-glitch added type/feature New feature or request comp/acp Agent Communication Protocol adapter P3 Low — cosmetic, nice to have labels Jul 14, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

This PR adds platform_toolsets.acp config support for ACP tool surface. Small, targeted addition.

Please verify:

  • The config key is properly read and applied
  • ACP tools work correctly with the new config

Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

Changes

ACP adapter: default toolsets changed from ["hermes-acp"] to None when agent has no enabled_toolsets. Allows the ACP session to inherit the agent's full toolset instead of being restricted to hermes-acp.

Assessment

  • Correctness: Without this, the ACP session would always get hermes-acp even if the agent had a broader toolset. Now it inherits properly.
  • No debug artifacts.

Reviewed by Hermes Agent

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for targeting a real ACP consistency gap: current main still hard-codes hermes-acp in acp_adapter/session.py:619-622.

Problems

  • acp_adapter/session.py:140-147 reimplements raw platform_toolsets.acp lookup instead of using the shared resolver. That bypasses the global agent.disabled_toolsets subtraction in hermes_cli/tools_config.py:1904-1912. ACP construction does not pass disabled_toolsets independently, so a toolset disabled globally but listed under platform_toolsets.acp would be exposed.

Suggested changes

  • Route ACP through _get_platform_tools(config, "acp"), preserving its hermes-acp fallback and shared explicit-selection/MCP semantics, then retain the ACP-client MCP append path.
  • Add a test for an ACP list containing a globally disabled toolset.

Automated hermes-sweeper review.

Comment thread acp_adapter/session.py Outdated
try:
from hermes_cli.config import load_config

raw = (load_config().get("platform_toolsets") or {}).get("acp")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the shared _get_platform_tools(config, "acp") resolver rather than read this mapping directly. It applies agent.disabled_toolsets last (hermes_cli/tools_config.py:1904-1912); ACP _make_agent() does not pass that list separately, so this path can re-enable a globally disabled toolset.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fbb39d2_acp_base_toolsets() now routes explicit platform_toolsets.acp lists through the shared _get_platform_tools(config, "acp", include_default_mcp_servers=False) resolver, so the global agent.disabled_toolsets subtraction (applied last in hermes_cli/tools_config.py) is honored instead of bypassed. The hermes-acp fallback for missing/non-list/empty values is preserved, and config-level MCP servers stay excluded from the base resolution so the existing per-session append path in _expand_acp_enabled_toolsets remains the single place MCP toolsets are added.

Added the regression test you asked for: test_create_session_strips_globally_disabled_toolsets (tests/acp/test_session.py) asserts that a toolset listed under platform_toolsets.acp but also in agent.disabled_toolsets is excluded from the session's enabled_toolsets.

Verified with scripts/run_tests.sh tests/acp -q — 14 files, 309 tests passed, 0 failed.

@teknium1 teknium1 added 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 16, 2026
…ols resolver

The hand-rolled config lookup in _acp_base_toolsets() bypassed the
global agent.disabled_toolsets subtraction that _get_platform_tools
applies last, so a toolset disabled globally but listed under
platform_toolsets.acp would be re-exposed on the ACP surface.

Resolve explicit lists through _get_platform_tools(config, "acp",
include_default_mcp_servers=False) instead, keeping the hermes-acp
fallback for missing/non-list/empty values and leaving MCP servers to
the existing per-session append path in _expand_acp_enabled_toolsets.

Adds a regression test: a toolset in platform_toolsets.acp that is
also in agent.disabled_toolsets is excluded from the session's
enabled_toolsets.
@israellot

Copy link
Copy Markdown
Contributor Author

Addressed the hermes-sweeper review finding in fbb39d2.

Finding → fix

  • acp_adapter/session.py:140-147 reimplements the raw platform_toolsets.acp lookup, bypassing the global agent.disabled_toolsets subtraction_acp_base_toolsets() now resolves explicit lists through the shared _get_platform_tools(config, "acp", include_default_mcp_servers=False) resolver, which applies agent.disabled_toolsets last. Behavior intentionally preserved from the original PR: missing/non-list/empty platform_toolsets.acp values still fall back to the hermes-acp composite, and the ACP-client MCP append path in _expand_acp_enabled_toolsets is untouched (MCP servers are excluded from base resolution so per-session appending remains the single source).

Tests

  • New: TestPersistence::test_create_session_strips_globally_disabled_toolsets — config with web in both platform_toolsets.acp and agent.disabled_toolsets → session enabled_toolsets excludes web.
  • Updated: test_create_session_honors_platform_toolsets_acp — expected order now matches the resolver's sorted output.

Verification

scripts/run_tests.sh tests/acp -q → 14 files, 309 tests passed, 0 failed.

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Summary

Two PRs address the ACP configuration gap. Both replace the hard-coded ACP toolset with platform_toolsets.acp, but #64045 covers all three ACP resolution paths and preserves shared disabled-toolset and MCP semantics, while #13167 changes only agent construction and platform registration.

Related pull requests

  • #13167 [closed] duplicate — (+152/-2) — superseded duplicate: The closed PR replaces the hard-coded enabled_toolsets value during ACP agent construction and adds ACP to the platform registry, but it does not update the MCP-registration or /tools refresh paths and performs raw config normalization rather than shared resolver handling; it remains relevant as the narrower predecessor to #64045.
  • #64045 related — (+109/-4) — preferred consolidation target: The diff routes explicit platform_toolsets.acp values through the shared resolver, updates agent construction, MCP registration, and /tools resolution, preserves per-session MCP appending, and tests global disabled-toolset subtraction. The contributor keep_open review on #64045 identified that disabled_toolsets could be bypassed; the current diff explicitly addresses that concern with _get_platform_tools(..., include_default_mcp_servers=False) and a regression test, although no separate recorded verify verdict is available.

Duplicates

#13167 and #64045 implement substantially the same core configuration support, but #64045 supersedes #13167 with broader call-site coverage and shared resolver semantics.

Suggested consolidation

Merge #64045 after normal maintainer validation — its current diff addresses the contributor review’s blocking resolver concern and covers the full ACP tool-surface lifecycle. Keep #13167 closed as superseded by #64045; neither PR has a recorded verify verdict.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup13167 ["PRs duplicating each other"]
        P13167["PR #13167 (closed)"]
        P64045["PR #64045 (open)"]
    end
    class P13167 closed
    class P64045 open
    class P64045 target
    click P13167 "https://github.com/NousResearch/hermes-agent/pull/13167"
    click P64045 "https://github.com/NousResearch/hermes-agent/pull/64045"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 17 kB of PR diffs, 6 kB of issue/PR text, 4 kB of discussion (5 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter P3 Low — cosmetic, nice to have 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/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants