diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 4451704b1b..15bf312e0a 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -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" ) diff --git a/hermes_cli/mcp_config.py b/hermes_cli/mcp_config.py index 0e01f558dd..5bc30aaa0c 100644 --- a/hermes_cli/mcp_config.py +++ b/hermes_cli/mcp_config.py @@ -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) diff --git a/tests/hermes_cli/test_mcp_add_command_dest.py b/tests/hermes_cli/test_mcp_add_command_dest.py new file mode 100644 index 0000000000..09e47df95a --- /dev/null +++ b/tests/hermes_cli/test_mcp_add_command_dest.py @@ -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 diff --git a/tests/hermes_cli/test_mcp_config.py b/tests/hermes_cli/test_mcp_config.py index 979108a951..e136f1b3c0 100644 --- a/tests/hermes_cli/test_mcp_config.py +++ b/tests/hermes_cli/test_mcp_config.py @@ -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