Skip to content

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

Closed
discodirector wants to merge 1 commit into
NousResearch:mainfrom
discodirector:fix/mcp-add-command-dest-collision
Closed

fix(mcp): give 'mcp add --command' a distinct argparse dest#19787
discodirector wants to merge 1 commit into
NousResearch:mainfrom
discodirector:fix/mcp-add-command-dest-collision

Conversation

@discodirector

Copy link
Copy Markdown
Contributor

What does this PR do?

hermes mcp add <name> --url <url> silently launches an interactive chat session instead of registering an MCP server, because of an argparse dest collision. This PR fixes the collision and adds a regression test at the parser layer.

The top-level subparsers in hermes_cli/_parser.py use dest="command" so the dispatcher in main.py can route on args.command. The mcp add subparser registers a --command flag (the stdio command for an MCP server) without an explicit dest=, so argparse derives dest="command" from the flag name. Whenever --command is omitted (e.g. on the documented --url install path), the subparser still writes args.command = None, clobbering the top-level "mcp". The dispatcher then sees args.command is None and falls through to cmd_chat. cmd_mcp_add is never reached — there is no error, no warning, no log.

The fix gives the flag an explicit, non-colliding dest. The user-facing CLI flag --command is unchanged; only the in-memory namespace attribute moves.

Related Issue

Fixes #19785.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/main.py — declare mcp_add_p.add_argument("--command", dest="mcp_command", ...) so the flag no longer overwrites args.command. Comment explains the constraint so a future cleanup pass does not re-introduce the collision.
  • hermes_cli/mcp_config.pycmd_mcp_add reads getattr(args, "mcp_command", None) instead of args.command. Comment cross-references the parser.
  • tests/hermes_cli/test_mcp_config.py_make_args helper now defaults mcp_command=None (was command=None); the four call-sites that asserted stdio-server behavior pass mcp_command="npx" / mcp_command="uvx" accordingly. Pure mechanical rename, no logic change.
  • tests/hermes_cli/test_mcp_add_command_dest.py (new) — three parser-level regression tests modelled on test_argparse_flag_propagation.py and test_subparser_routing_fallback.py. They build a minimal replica of the parent + mcp add subparser, parse representative argv vectors, and assert (a) args.command stays "mcp", (b) --command npx populates args.mcp_command (not args.command), (c) the bare-mcp add form also preserves the top-level dest.

How to Test

  1. Reproduce on main at cfd86dc:

    hermes mcp add example --url 'https://example.com/mcp'
    

    Observe an interactive chat prompt opens; hermes mcp list shows no example entry.

  2. Apply this PR. Repeat the command. The MCP server is now registered; hermes mcp list shows example.

  3. Run the new regression test:

    pytest tests/hermes_cli/test_mcp_add_command_dest.py -v
    

    All three tests should pass. Reverting the dest="mcp_command" change makes test_url_invocation_preserves_top_level_command and test_bare_mcp_add_does_not_clobber_command fail with args.command == None.

  4. The existing tests/hermes_cli/test_mcp_config.py suite (covering stdio-add, env vars, presets, etc.) continues to pass after the helper rename.

Notes on scope

  • No fallback read on the old args.command attribute. A naïve fallback (getattr(args, "mcp_command", None) or getattr(args, "command", None)) would silently return "mcp" whenever cmd_mcp_add is invoked through the real dispatcher (because argparse sets args.command = "mcp" from the top-level subparser), making cmd_mcp_add think the user passed --command mcp. The clean rename is safer.
  • The --command CLI flag name is preserved; this is purely an internal namespace change. No config-file format changes, no behaviour change for any path other than the previously broken one.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs — no duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and all tests pass — I ran tests/hermes_cli/test_mcp_add_command_dest.py in isolation (3/3 pass) and verified Python syntax of the four touched files. I was not able to run the full pytest suite in my dev environment; happy to address any CI failures.
  • I've added tests for my changes
  • I've tested on my platform: Windows 11, Python 3.12

Documentation & Housekeeping

  • I've updated relevant documentation — N/A (no user-facing docs reference the internal dest name; the --command CLI flag is unchanged)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact — argparse behaviour is platform-independent
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

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 NousResearch#19785.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard tool/mcp MCP client and OAuth P1 High — major feature broken, no workaround labels May 4, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Note: #17294 is an earlier PR for the same argparse dest collision fix. This PR appears more complete (includes regression tests).

1 similar comment
@alt-glitch

Copy link
Copy Markdown
Collaborator

Note: #17294 is an earlier PR for the same argparse dest collision fix. This PR appears more complete (includes regression tests).

@teknium1

teknium1 commented May 7, 2026

Copy link
Copy Markdown
Contributor

Merged via #21204 — your commit was cherry-picked onto current main with your authorship preserved (commit 4f364c4). Thanks for the clean root-cause analysis and the parser-level regression test — made the salvage trivial. Sorry it bit the Sonoglyph onboarding; the fix is on main now and next hermes update will pull it in. https://sonoglyph.xyz is dope, by the way.

@teknium1

teknium1 commented May 7, 2026

Copy link
Copy Markdown
Contributor

Already merged via #21204 (commit 4f364c4 on main) — your commit landed with your authorship preserved via rebase-merge. E2E-verified just now that hermes mcp add <name> --url ... now dispatches to cmd_mcp_add correctly, writes the server to config.yaml, and hermes mcp list shows it. Thanks for the clean diagnosis, minimal repro, and regression test.

@discodirector

Copy link
Copy Markdown
Contributor Author

Merged via #21204 — your commit was cherry-picked onto current main with your authorship preserved (commit 4f364c4). Thanks for the clean root-cause analysis and the parser-level regression test — made the salvage trivial. Sorry it bit the Sonoglyph onboarding; the fix is on main now and next hermes update will pull it in. https://sonoglyph.xyz is dope, by the way.

Thanks ser for the trust, now onboarding really should go smooth as butter.
Waiting for your own Sonoglyph artifact!

Screenshot 2026-05-01 004742

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

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P1 High — major feature broken, no workaround tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: hermes mcp add silently launches chat instead of registering MCP server

3 participants