fix(delegate): reject acp_command values the transport can't drive by ssiweifnag · Pull Request #39462 · NousResearch/hermes-agent

ssiweifnag · 2026-06-05T01:35:15Z

Summary

Fixes #38662 — the gateway's /claude command set acp_command='claude' with args=['--acp', '--stdio']. Claude Code CLI doesn't implement --acp, so every child agent died with unknown option '--acp' before sending any prompt, retried three times, then fell back to a different model and surfaced a confusing 401 to the user.

The ACP subprocess transport in agent/copilot_acp_client.py is wired specifically to GitHub Copilot CLI. This PR validates acp_command at the delegate_task entry point so a bad value fails fast with a clear message that names the supported CLI and points at the claude-code skill for Claude users.

Changes

tools/delegate_tool.py — add _validate_acp_command allowlist helper, call it in delegate_task for the top-level kwarg and inside the per-task loop. Allowlist currently contains only copilot (matching what agent/copilot_acp_client.py supports); absolute paths ending in /copilot are accepted so users with the CLI in a non-default location (nvm, asdf) aren't tripped up.
tests/tools/test_delegate.py — update the existing test_acp_args_forwarded test (it was pinning the broken behavior with acp_command="claude"), and add 12 new tests covering the allowlist unit (TestValidateAcpCommand) and the end-to-end rejection in delegate_task (TestDelegateAcpCommandRejection).

Why not a Claude → ACP bridge?

The issue's "Proposed Fix" section suggests building a lightweight ACP bridge that translates claude -p "prompt" into ACP JSON-RPC. That's a much larger surface — the user would be responsible for keeping the bridge in sync with the ACP spec, and we'd be adding a new code path to maintain. The transport's docstring explicitly states it is built for copilot --acp --stdio, so failing fast on the unsupported value is the more conservative fix and matches what the existing tool schema description already promises ("Requires an ACP-compatible CLI (currently GitHub Copilot CLI via 'copilot --acp --stdio')"). If a real Claude→ACP bridge gets built later, expanding the allowlist is a one-line change.

Test plan

pytest tests/tools/test_delegate.py -v — 147 passed
New TestValidateAcpCommand covers None, "", "copilot", "/opt/copilot/bin/copilot", "claude", "claude-code", "aider", etc.
New TestDelegateAcpCommandRejection covers top-level rejection, per-task rejection, and the positive path (copilot still works, unset still works)
Updated TestDispatchDelegateTask::test_acp_args_forwarded now uses acp_command="copilot" so it asserts the supported path

🤖 Generated with Claude Code

…esent _check_via_local_git() hard-coded 'origin' for both the fetch and the rev-list ref, so users on a fork (where 'origin' is their own clone) saw a 'behind' count measured against their fork's main rather than the canonical source. The fork itself was always ahead of local by 0-3 commits, so the message was permanently misleading. Add a small _detect_canonical_remote() helper that returns 'upstream' when that remote exists, else 'origin', and use it in _check_via_local_git. This mirrors the convention already used by cmd_update (see _has_upstream_remote, _sync_with_upstream_if_needed) so the update-check banner and the 'hermes update' flow now agree on what 'behind' means for fork users. Also update the docstring of check_for_updates() to say 'canonical remote' instead of 'origin/main'.

Adds regression coverage for the canonical-remote detection in _check_via_local_git. Without these tests, a future refactor that re-hardcodes "origin" would silently break fork users (the "behind" banner would measure against the fork's main, which is normally 0-3 commits ahead, producing permanent false positives). Two test groups: - TestDetectCanonicalRemote: exercises the detector against real git repos in tmp_path (4 scenarios: upstream present, no upstream, not a git repo, only upstream). - TestCheckViaLocalGitUsesCanonicalRemote: pins the contract that the checker fetches+rev-lists whatever the detector returns, by mocking _detect_canonical_remote directly so subprocess.run only sees the intended fetch/rev-list calls.

The error returned by _validate_cron_script_path for absolute / ~ / Windows paths told users to "Place scripts in ~/.hermes/scripts/" — but the actual resolution uses get_hermes_home() / "scripts", so on Docker or any non-default HERMES_HOME setup the error message points users (and the agent) at the wrong directory. The fix computes scripts_dir once and uses it in both the rejection message and the (unchanged) containment check. Closes NousResearch#38693

…Exit The memory plugin loader used to glob every *.py in a user-installed provider directory and exec_module it, with a bare `except Exception` around the call. Two compounding problems: 1. Files that are not real submodules — `setup.py`, `conftest.py`, `pyproject.py`, `test_*.py`, `*_test.py` — get executed. A `setup.py` next to a plugin will call setuptools, which parses sys.argv and `sys.exit()`s on bad subcommand. 2. `sys.exit()` raises SystemExit, which inherits from BaseException — not Exception. The bare `except Exception` does not catch it, so SystemExit propagates and crashes the whole Hermes process. Fix: skip the known non-submodule files in the glob, and explicitly re-raise KeyboardInterrupt/SystemExit in the load path so they cannot be swallowed by future `except` clauses either. Closes NousResearch#38674

The conversation loop's recovery path uses parse_available_output_tokens_from_error() to detect when the request failed because max_tokens (the *output* cap) is too large relative to the context window, vs. the *input* itself being too long. The two require different recovery strategies — output-cap errors reduce max_tokens for the next call, while input-overflow errors compress history. The parser previously recognized only Anthropic's "... = available_tokens: N" shape. OpenRouter and Nous Research return the same condition in a different shape: "maximum context length is 256000 tokens. However, you requested about 281093 tokens (5683 of text input, 13410 of tool input, 262000 in the output)." For OpenRouter/Nous users, the parser returned None, so Hermes classified the error as a prompt-overflow and tried to compress history. On a fresh session with 1 message there's nothing to compress, so the gateway auto-reset the session — and on the next message the same max_tokens config value produced the same error, looping forever. The new guard recognizes the OpenRouter shape by its three structural anchors (text input, tool input, "in the output") and extracts the available output as context_length - text_input - tool_input. All existing Anthropic-format tests still pass. Closes NousResearch#38652

Two related bugs in NousResearch#38650: 1. ``hermes dump`` reported ``mcp_servers: 0`` even when servers were configured. ``_count_mcp_servers`` read the wrong config key (``config["mcp"]["servers"]`` — a path that never existed in real configs). The real Hermes config uses a top-level ``mcp_servers`` key. Fix reads the right key, and additionally prefers the live ``_servers`` registry over the config snapshot — the registry is the reliable truth at dump time because MCP discovery runs asynchronously in the background. 2. The welcome banner labeled every not-yet-connected server as "failed" (red text) on fresh launch. The MCP registry is empty for a few seconds while background discovery is in flight, so every server showed as failed. ``get_mcp_status()`` now distinguishes "pending" (discovery hasn't reached this server yet) from "failed" (the genuine case), and the banner renders pending as a neutral "connecting…". The original "failed" rendering is preserved for the case where discovery has actually attempted and given up on a server — the "pending" flag is only set when the live registry has no record of that server *and* discovery either hasn't started or hasn't reached it yet. Closes NousResearch#38650

…ousResearch#38662) The gateway's /claude command was setting acp_command='claude' with args=['--acp', '--stdio']. Claude Code CLI doesn't implement --acp, so every child agent died with "unknown option '--acp'" before any prompt was sent, retried three times, then fell back to a different model and surfaced a confusing 401. The ACP subprocess transport in agent/copilot_acp_client.py is wired specifically to GitHub Copilot CLI. Validate at the delegate_task entry point so a bad acp_command fails fast with a clear message that names the supported CLI and points at the claude-code skill for Claude users. Update the existing test that pinned the broken behavior and add coverage for the new allowlist. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

@nikshepsvn

…orted ACP transport delegate_task exposes acp_command / acp_args as free-form, model-controllable tool-call arguments. They propagate unvalidated through child construction into subprocess.Popen([acp_command] + acp_args, ...) (agent/copilot_acp_client.py), so a prompt-injected delegate_task call with acp_command="python" (or powershell/bash + inline code in acp_args) is arbitrary host-process execution that bypasses the terminal backend's sandboxing and command approvals — a model-reachable RCE crossing the OS boundary. Add a copilot-only allowlist (_validate_acp_command / _SUPPORTED_ACP_COMMANDS) enforced on BOTH the top-level and per-task model-supplied overrides, before any child agent is built. A bare `copilot` or an absolute path ending in `/copilot` (custom install location) is accepted; everything else returns a clear tool error. Operator-configured `delegation.command` (creds["command"]) is trusted and is not routed through the check. Schema descriptions updated to state the restriction. Supersedes NousResearch#39462 (stale / merge-conflicted), which first proposed the copilot-only allowlist; this carries that approach forward, rebased on current main, with top-level + per-task enforcement and regression tests. Credit also to @nikshepsvn (NousResearch#27426) for the earlier acp_command crash-hardening attempt. The companion credential-inheritance half of this finding (the ACP subprocess inheriting os.environ secrets) is addressed separately in the "sanitize agent subprocess env" change. Co-authored-by: ssiweifnag <120658181+ssiweifnag@users.noreply.github.com> Co-authored-by: nikshepsvn <23241247+nikshepsvn@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

teknium1 · 2026-07-01T12:36:26Z

Thanks for surfacing this — you were first to propose restricting the model-supplied acp_command (the finding is real: it reaches subprocess.Popen as a host binary).

We're closing it along with the later carry-forward (#52346), because the fix direction we want isn't an allowlist. Filtering the value keeps acp_command as a model-facing tool argument; the better fix is to remove it from the model schema entirely, since choosing the subprocess binary is operator/infrastructure config, not a model decision. We just did exactly this for the sibling toolsets argument in #56386, and acp_command / acp_args will follow the same path (resolved only from trusted operator config + parent inheritance).

Your work here is credited as the earliest proposal of this restriction. Thanks!

Source: #52346 Related prior work: #39462 Related prior work: #27426 Maintainer direction: #52346 (comment) Remove acp_command and acp_args from the model-facing delegate_task schema and dispatch paths. Child agents can still use ACP subprocess transport when it comes from trusted delegation config or parent inheritance, but a model tool call can no longer choose the command or arguments that reach child construction. This is salvageable because the risky boundary is model control over child ACP transport, not ACP itself. The patch follows the maintainer direction from the source discussion by preserving trusted ACP configuration and prior integration work while removing the untrusted tool-call fields from both top-level and per-task delegate inputs. Reproduced on main by passing acp_command through delegate_task and observing it reach _build_child_agent. Verified after the fix that model dispatch strips the hidden top-level fields and per-task hidden fields are ignored before child construction. Co-authored-by: Carlosian <claudlos@agentmail.to> Co-authored-by: ssiweifnag <120658181+ssiweifnag@users.noreply.github.com> Co-authored-by: nikshepsvn <23241247+nikshepsvn@users.noreply.github.com>

Source: NousResearch#52346 Related prior work: NousResearch#39462 Related prior work: NousResearch#27426 Maintainer direction: NousResearch#52346 (comment) Remove acp_command and acp_args from the model-facing delegate_task schema and dispatch paths. Child agents can still use ACP subprocess transport when it comes from trusted delegation config or parent inheritance, but a model tool call can no longer choose the command or arguments that reach child construction. This is salvageable because the risky boundary is model control over child ACP transport, not ACP itself. The patch follows the maintainer direction from the source discussion by preserving trusted ACP configuration and prior integration work while removing the untrusted tool-call fields from both top-level and per-task delegate inputs. Reproduced on main by passing acp_command through delegate_task and observing it reach _build_child_agent. Verified after the fix that model dispatch strips the hidden top-level fields and per-task hidden fields are ignored before child construction. Co-authored-by: Carlosian <claudlos@agentmail.to> Co-authored-by: ssiweifnag <120658181+ssiweifnag@users.noreply.github.com> Co-authored-by: nikshepsvn <23241247+nikshepsvn@users.noreply.github.com>

ssiweifnag and others added 7 commits June 4, 2026 00:16

alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/delegate Subagent delegation labels Jun 5, 2026

claudlos mentioned this pull request Jun 25, 2026

security(delegate): restrict model-controlled acp_command to the supported ACP transport #52346

Closed

teknium1 closed this Jul 1, 2026

egilewski mentioned this pull request Jul 3, 2026

fix(security): remove model-controlled delegate ACP transport #57573

Closed

19 tasks

teknium1 mentioned this pull request Jul 14, 2026

bug: /claude gateway command passes --acp --stdio to Claude Code CLI which doesn't support these flags #38662

Closed

Navigation Menu

Uh oh!

fix(delegate): reject acp_command values the transport can't drive - #39462

fix(delegate): reject acp_command values the transport can't drive#39462
ssiweifnag wants to merge 7 commits into
NousResearch:mainfrom
ssiweifnag:fix/gateway-claude-acp-flags

ssiweifnag commented Jun 5, 2026

Uh oh!

teknium1 commented Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ssiweifnag commented Jun 5, 2026

Summary

Changes

Why not a Claude → ACP bridge?

Test plan

Uh oh!

teknium1 commented Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants