nested subparser dispatch bug causing chat fallback

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #21204 fix(mcp): give 'mcp add --command' a distinct argparse dest

  merged to main

  files: hermes_cli/main.py, hermes_cli/mcp_config.py, scripts/release.py, tests/hermes_cli/test_mcp_add_command_dest.py, tests/hermes_cli/test_mcp_config.py

  Show the diff

  • hermes_cli/main.py

    @@ -10007,7 +10007,15 @@ def cmd_tools(args):
         )
         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"
         )

  • 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)

  • scripts/release.py

    @@ -41,6 +41,7 @@
     AUTHOR_MAP = {
         # teknium (multiple emails)
         "[redacted:email]": "teknium1",
    +    "[redacted:email]": "discodirector",
         "[redacted:email]": "mikeyobrien",
         "[redacted:email]": "qiyin-code",
         "[redacted:email]": "olisikh",

  • 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

  • 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 @@ def mock_probe(name, config, **kw):
     
             cmd_mcp_add(_make_args(
                 name="github",
    -            command="npx",
    +            mcp_command="npx",
                 args=["@mcp/github"],
             ))
             out = capsys.readouterr().out
    @@ -291,7 +291,7 @@ def mock_probe(name, config, **kw):
     
             cmd_mcp_add(_make_args(
                 name="github",
    -            command="npx",
    +            mcp_command="npx",
                 args=["@mcp/github"],
                 env=["MY_API_KEY=[redacted:secret-assignment]", "DEBUG=true"],
             ))
    @@ -313,7 +313,7 @@ def test_add_stdio_server_rejects_invalid_env_name(self, capsys):
     
             cmd_mcp_add(_make_args(
                 name="github",
    -            command="npx",
    +            mcp_command="npx",
                 args=["@mcp/github"],
                 env=["BAD-NAME=value"],
             ))
    @@ -390,7 +390,7 @@ def mock_probe(name, config, **kw):
             cmd_mcp_add(_make_args(
                 name="custom",
                 preset="testmcp",
    -            command="uvx",
    +            mcp_command="uvx",
                 args=["custom-server"],
             ))
             out = capsys.readouterr().out