mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-08 03:01:47 +00:00
fix(mcp): give 'mcp add --command' a distinct argparse dest
The --command flag of `hermes mcp add` shared its argparse dest with the top-level subparser (`dest="command"` in `hermes_cli/_parser.py`). When the flag was omitted, argparse still wrote `args.command = None`, clobbering the top-level value of `"mcp"`. The dispatcher then saw `args.command is None` and fell through to interactive chat, so `hermes mcp add ...` silently launched chat instead of registering the server. `cmd_mcp_add` was never reached. Use `dest="mcp_command"` on the flag and read it from `cmd_mcp_add`. The user-facing CLI flag `--command` is unchanged; only the in-memory namespace attribute moves. Also updates the `_make_args` helper in `tests/hermes_cli/test_mcp_config.py` to populate the new dest, and adds `tests/hermes_cli/test_mcp_add_command_dest.py` with a parser- level regression test. Closes #19785. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
parent
333598cb0e
commit
4f364c4e99
4 changed files with 105 additions and 7 deletions
|
|
@ -10007,7 +10007,15 @@ Examples:
|
|||
)
|
||||
mcp_add_p.add_argument("name", help="Server name (used as config key)")
|
||||
mcp_add_p.add_argument("--url", help="HTTP/SSE endpoint URL")
|
||||
mcp_add_p.add_argument("--command", help="Stdio command (e.g. npx)")
|
||||
# dest="mcp_command" so this flag does not clobber the top-level
|
||||
# subparser's args.command attribute, which the dispatcher reads to
|
||||
# route to cmd_mcp. Without an explicit dest, argparse derives
|
||||
# dest="command" from the flag name and sets it to None when the
|
||||
# flag is omitted, causing `hermes mcp add ...` to fall through to
|
||||
# interactive chat.
|
||||
mcp_add_p.add_argument(
|
||||
"--command", dest="mcp_command", help="Stdio command (e.g. npx)"
|
||||
)
|
||||
mcp_add_p.add_argument(
|
||||
"--args", nargs="*", default=[], help="Arguments for stdio command"
|
||||
)
|
||||
|
|
|
|||
|
|
@ -221,7 +221,10 @@ def cmd_mcp_add(args):
|
|||
"""Add a new MCP server with discovery-first tool selection."""
|
||||
name = args.name
|
||||
url = getattr(args, "url", None)
|
||||
command = getattr(args, "command", None)
|
||||
# Read from `mcp_command` (set by --command via explicit dest) — see
|
||||
# mcp_add_p.add_argument("--command", dest="mcp_command", ...) in
|
||||
# hermes_cli/main.py for why the dest is renamed.
|
||||
command = getattr(args, "mcp_command", None)
|
||||
cmd_args = getattr(args, "args", None) or []
|
||||
auth_type = getattr(args, "auth", None)
|
||||
preset_name = getattr(args, "preset", None)
|
||||
|
|
|
|||
87
tests/hermes_cli/test_mcp_add_command_dest.py
Normal file
87
tests/hermes_cli/test_mcp_add_command_dest.py
Normal file
|
|
@ -0,0 +1,87 @@
|
|||
"""Regression test: ``hermes mcp add --command`` must not clobber the
|
||||
top-level ``args.command`` subparser dest.
|
||||
|
||||
The top-level argparse parser uses ``dest="command"`` for its subparsers
|
||||
(``hermes_cli/_parser.py``). The dispatcher in ``hermes_cli/main.py``
|
||||
reads ``args.command`` to decide which command to run; if it is ``None``
|
||||
it falls through to interactive chat.
|
||||
|
||||
The ``mcp add`` subparser exposes a ``--command`` flag (the stdio command
|
||||
for an MCP server, e.g. ``npx``). Without an explicit ``dest=``, argparse
|
||||
derives the dest from the flag name and writes ``args.command = None``
|
||||
when the flag is omitted, overwriting the top-level ``"mcp"`` value. As a
|
||||
result, ``hermes mcp add foo --url ...`` silently launches chat instead
|
||||
of registering an MCP server.
|
||||
|
||||
The fix: declare the flag with ``dest="mcp_command"``. The CLI flag name
|
||||
is unchanged; only the in-memory attribute moves.
|
||||
|
||||
We replicate the relevant parser shape here rather than importing the
|
||||
real builder, mirroring ``test_argparse_flag_propagation.py`` and
|
||||
``test_subparser_routing_fallback.py``.
|
||||
"""
|
||||
|
||||
import argparse
|
||||
|
||||
|
||||
def _build_parser():
|
||||
"""Minimal replica of the slice of the hermes parser that exhibits
|
||||
the bug: top-level subparsers (dest="command") and ``mcp add`` with
|
||||
its ``--command`` flag.
|
||||
"""
|
||||
parser = argparse.ArgumentParser(prog="hermes")
|
||||
subparsers = parser.add_subparsers(dest="command")
|
||||
|
||||
subparsers.add_parser("chat")
|
||||
|
||||
mcp_p = subparsers.add_parser("mcp")
|
||||
mcp_sub = mcp_p.add_subparsers(dest="mcp_action")
|
||||
|
||||
mcp_add = mcp_sub.add_parser("add")
|
||||
mcp_add.add_argument("name")
|
||||
mcp_add.add_argument("--url")
|
||||
mcp_add.add_argument("--command", dest="mcp_command")
|
||||
|
||||
return parser
|
||||
|
||||
|
||||
class TestMcpAddCommandDest:
|
||||
def test_url_invocation_preserves_top_level_command(self):
|
||||
"""`hermes mcp add foo --url ...` must keep args.command == "mcp".
|
||||
|
||||
Before the dest fix this was clobbered to None, sending the
|
||||
dispatcher into the chat fallback.
|
||||
"""
|
||||
parser = _build_parser()
|
||||
args = parser.parse_args(
|
||||
["mcp", "add", "foo", "--url", "https://example.com/mcp"]
|
||||
)
|
||||
|
||||
assert args.command == "mcp"
|
||||
assert args.mcp_action == "add"
|
||||
assert args.name == "foo"
|
||||
assert args.url == "https://example.com/mcp"
|
||||
assert args.mcp_command is None
|
||||
|
||||
def test_command_flag_writes_to_mcp_command_dest(self):
|
||||
"""`--command npx` must populate args.mcp_command, not args.command."""
|
||||
parser = _build_parser()
|
||||
args = parser.parse_args(
|
||||
["mcp", "add", "github", "--command", "npx"]
|
||||
)
|
||||
|
||||
assert args.command == "mcp"
|
||||
assert args.mcp_command == "npx"
|
||||
|
||||
def test_bare_mcp_add_does_not_clobber_command(self):
|
||||
"""Even without --url or --command, args.command stays "mcp".
|
||||
|
||||
Catches the regression at the parser layer regardless of which
|
||||
transport flag the user passes.
|
||||
"""
|
||||
parser = _build_parser()
|
||||
args = parser.parse_args(["mcp", "add", "foo"])
|
||||
|
||||
assert args.command == "mcp"
|
||||
assert args.mcp_command is None
|
||||
assert args.url is None
|
||||
|
|
@ -43,7 +43,7 @@ def _make_args(**kwargs):
|
|||
defaults = {
|
||||
"name": "test-server",
|
||||
"url": None,
|
||||
"command": None,
|
||||
"mcp_command": None,
|
||||
"args": None,
|
||||
"auth": None,
|
||||
"preset": None,
|
||||
|
|
@ -233,7 +233,7 @@ class TestMcpAdd:
|
|||
|
||||
cmd_mcp_add(_make_args(
|
||||
name="github",
|
||||
command="npx",
|
||||
mcp_command="npx",
|
||||
args=["@mcp/github"],
|
||||
))
|
||||
out = capsys.readouterr().out
|
||||
|
|
@ -291,7 +291,7 @@ class TestMcpAdd:
|
|||
|
||||
cmd_mcp_add(_make_args(
|
||||
name="github",
|
||||
command="npx",
|
||||
mcp_command="npx",
|
||||
args=["@mcp/github"],
|
||||
env=["MY_API_KEY=secret123", "DEBUG=true"],
|
||||
))
|
||||
|
|
@ -313,7 +313,7 @@ class TestMcpAdd:
|
|||
|
||||
cmd_mcp_add(_make_args(
|
||||
name="github",
|
||||
command="npx",
|
||||
mcp_command="npx",
|
||||
args=["@mcp/github"],
|
||||
env=["BAD-NAME=value"],
|
||||
))
|
||||
|
|
@ -390,7 +390,7 @@ class TestMcpAdd:
|
|||
cmd_mcp_add(_make_args(
|
||||
name="custom",
|
||||
preset="testmcp",
|
||||
command="uvx",
|
||||
mcp_command="uvx",
|
||||
args=["custom-server"],
|
||||
))
|
||||
out = capsys.readouterr().out
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue