From dca11b66502bf13625621795b546a71a5adb68d5 Mon Sep 17 00:00:00 2001 From: helix4u <4317663+helix4u@users.noreply.github.com> Date: Wed, 10 Jun 2026 15:23:48 -0600 Subject: [PATCH] fix(mcp): preserve stdio argv passthrough --- hermes_cli/main.py | 67 ++++++++++++---- hermes_cli/mcp_config.py | 2 + hermes_cli/subcommands/mcp.py | 6 +- .../hermes_cli/test_apply_profile_override.py | 77 +++++++++++++++++++ tests/hermes_cli/test_mcp_add_command_dest.py | 24 ++++++ tests/tools/test_mcp_tool.py | 27 +++++++ tools/mcp_tool.py | 38 ++++++++- website/docs/reference/cli-commands.md | 2 +- 8 files changed, 226 insertions(+), 17 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 719a181bc2..c7d15dddf5 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -334,21 +334,66 @@ sys.path.insert(0, str(PROJECT_ROOT)) # Falls back to ~/.hermes/active_profile for sticky default. # --------------------------------------------------------------------------- def _apply_profile_override() -> None: - """Pre-parse --profile/-p and set HERMES_HOME before module imports.""" + """Pre-parse --profile/-p and set HERMES_HOME before imports.""" argv = sys.argv[1:] profile_name = None consume = 0 + profile_index = None - # 1. Check for explicit -p / --profile flag - for i, arg in enumerate(argv): + def _inside_mcp_add_args(index: int) -> bool: + """True once argv reaches `hermes mcp add ... --args `. + + ``mcp add --args`` is command-argv passthrough. Flags after that point + belong to the child MCP command (for example Docker MCP Toolkit's + ``--profile``), not to Hermes' own profile selector. + """ + try: + mcp_index = argv.index("mcp", 0, index) + argv.index("add", mcp_index + 1, index) + except ValueError: + return False + return True + + # 1. Check for explicit -p / --profile flag. Historically this worked even + # after the subcommand (`hermes chat -p coder`), so keep scanning broadly. + # The exception is command-argv passthrough regions such as `mcp add --args`. + value_flags = { + "-z", "--oneshot", + "-m", "--model", + "--provider", + "-t", "--toolsets", + "-r", "--resume", + "-s", "--skills", + } + optional_value_flags = {"-c", "--continue"} + i = 0 + while i < len(argv): + arg = argv[i] + if arg == "--": + break + if arg == "--args" and _inside_mcp_add_args(i): + break if arg in {"--profile", "-p"} and i + 1 < len(argv): profile_name = argv[i + 1] consume = 2 + profile_index = i break - elif arg.startswith("--profile="): + if arg.startswith("--profile="): profile_name = arg.split("=", 1)[1] consume = 1 + profile_index = i break + if "=" not in arg and arg in value_flags and i + 1 < len(argv): + i += 2 + elif ( + "=" not in arg + and arg in optional_value_flags + and i + 1 < len(argv) + and not argv[i + 1].startswith("-") + ): + i += 2 + else: + i += 1 # 1b. Reject values that can't be valid profile names (e.g. pytest's # "-p no:xdist" would be misread as profile "no:xdist" otherwise). @@ -360,6 +405,7 @@ def _apply_profile_override() -> None: if not _re.match(r"^[a-z0-9][a-z0-9_-]{0,63}$", profile_name): profile_name = None consume = 0 + profile_index = None # 1.5 If HERMES_HOME is already set and no explicit flag was given, trust it # only when it already points to a specific profile directory. The @@ -407,16 +453,9 @@ def _apply_profile_override() -> None: return os.environ["HERMES_HOME"] = hermes_home # Strip the flag from argv so argparse doesn't choke - if consume > 0: - for i, arg in enumerate(argv): - if arg in {"--profile", "-p"}: - start = i + 1 # +1 because argv is sys.argv[1:] - sys.argv = sys.argv[:start] + sys.argv[start + consume :] - break - elif arg.startswith("--profile="): - start = i + 1 - sys.argv = sys.argv[:start] + sys.argv[start + 1 :] - break + if consume > 0 and profile_index is not None: + start = profile_index + 1 # +1 because argv is sys.argv[1:] + sys.argv = sys.argv[:start] + sys.argv[start + consume :] _apply_profile_override() diff --git a/hermes_cli/mcp_config.py b/hermes_cli/mcp_config.py index bb8f894875..17b6c0329e 100644 --- a/hermes_cli/mcp_config.py +++ b/hermes_cli/mcp_config.py @@ -288,6 +288,8 @@ def cmd_mcp_add(args): # hermes_cli/main.py for why the dest is renamed. command = getattr(args, "mcp_command", None) cmd_args = getattr(args, "args", None) or [] + if cmd_args and cmd_args[0] == "--": + cmd_args = cmd_args[1:] auth_type = getattr(args, "auth", None) preset_name = getattr(args, "preset", None) raw_env = getattr(args, "env", None) diff --git a/hermes_cli/subcommands/mcp.py b/hermes_cli/subcommands/mcp.py index ec17b8ed98..405a984578 100644 --- a/hermes_cli/subcommands/mcp.py +++ b/hermes_cli/subcommands/mcp.py @@ -6,6 +6,7 @@ Handler injected to avoid importing ``main``. from __future__ import annotations +import argparse from typing import Callable from hermes_cli.subcommands._shared import add_accept_hooks_flag @@ -52,7 +53,10 @@ def build_mcp_parser(subparsers, *, cmd_mcp: Callable) -> None: "--command", dest="mcp_command", help="Stdio command (e.g. npx)" ) mcp_add_p.add_argument( - "--args", nargs="*", default=[], help="Arguments for stdio command" + "--args", + nargs=argparse.REMAINDER, + default=[], + help="Arguments for stdio command; must be the last option", ) mcp_add_p.add_argument("--auth", choices=["oauth", "header"], help="Auth method") mcp_add_p.add_argument("--preset", help="Known MCP preset name") diff --git a/tests/hermes_cli/test_apply_profile_override.py b/tests/hermes_cli/test_apply_profile_override.py index 6396faabd2..9159969d3b 100644 --- a/tests/hermes_cli/test_apply_profile_override.py +++ b/tests/hermes_cli/test_apply_profile_override.py @@ -138,3 +138,80 @@ class TestApplyProfileOverrideHermesHomeGuard: _apply_profile_override() assert os.environ.get("HERMES_HOME") is None + + def test_subcommand_profile_flag_is_not_consumed(self, tmp_path, monkeypatch): + """Command argv flags named --profile must stay with that command. + + Docker Desktop's MCP Toolkit uses `docker mcp gateway run --profile ...`. + When that argv is passed through `hermes mcp add --args`, the early + profile pre-parser must not interpret the Docker profile as a Hermes + profile. + """ + hermes_root = tmp_path / ".hermes" + hermes_root.mkdir(parents=True, exist_ok=True) + argv = [ + "hermes", + "mcp", + "add", + "docker-research", + "--command", + "docker", + "--args", + "mcp", + "gateway", + "run", + "--profile", + "research", + ] + + monkeypatch.setattr(Path, "home", lambda: tmp_path) + monkeypatch.delenv("HERMES_HOME", raising=False) + monkeypatch.setattr(sys, "argv", list(argv)) + + from hermes_cli.main import _apply_profile_override + _apply_profile_override() + + assert os.environ.get("HERMES_HOME") is None + assert sys.argv == argv + + def test_profile_after_chat_subcommand_is_still_consumed(self, tmp_path, monkeypatch): + """Profile flags historically work after normal Hermes subcommands.""" + result = _run_apply_profile_override( + tmp_path, + monkeypatch, + hermes_home=None, + active_profile="coder", + argv=["hermes", "chat", "-p", "coder", "-q", "hello"], + ) + + assert result is not None + assert result.endswith("coder") + assert sys.argv == ["hermes", "chat", "-q", "hello"] + + def test_top_level_profile_after_value_flag_is_consumed(self, tmp_path, monkeypatch): + """Top-level --profile still works after other top-level value flags.""" + result = _run_apply_profile_override( + tmp_path, + monkeypatch, + hermes_home=None, + active_profile="coder", + argv=["hermes", "-m", "gpt-5", "--profile", "coder", "chat"], + ) + + assert result is not None + assert result.endswith("coder") + assert sys.argv == ["hermes", "-m", "gpt-5", "chat"] + + def test_top_level_profile_after_continue_flag_is_consumed(self, tmp_path, monkeypatch): + """--continue has an optional value, so a following --profile is a flag.""" + result = _run_apply_profile_override( + tmp_path, + monkeypatch, + hermes_home=None, + active_profile="coder", + argv=["hermes", "--continue", "--profile", "coder"], + ) + + assert result is not None + assert result.endswith("coder") + assert sys.argv == ["hermes", "--continue"] diff --git a/tests/hermes_cli/test_mcp_add_command_dest.py b/tests/hermes_cli/test_mcp_add_command_dest.py index 09e47df95a..2e7a3f2de0 100644 --- a/tests/hermes_cli/test_mcp_add_command_dest.py +++ b/tests/hermes_cli/test_mcp_add_command_dest.py @@ -41,6 +41,7 @@ def _build_parser(): mcp_add.add_argument("name") mcp_add.add_argument("--url") mcp_add.add_argument("--command", dest="mcp_command") + mcp_add.add_argument("--args", nargs=argparse.REMAINDER, default=[]) return parser @@ -85,3 +86,26 @@ class TestMcpAddCommandDest: assert args.command == "mcp" assert args.mcp_command is None assert args.url is None + + def test_args_passthrough_keeps_nested_option_flags(self): + """`--args` must keep command flags like Docker MCP's --profile.""" + parser = _build_parser() + args = parser.parse_args( + [ + "mcp", + "add", + "docker-research", + "--command", + "docker", + "--args", + "mcp", + "gateway", + "run", + "--profile", + "research", + ] + ) + + assert args.command == "mcp" + assert args.mcp_command == "docker" + assert args.args == ["mcp", "gateway", "run", "--profile", "research"] diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index f7a19f4d92..777e2e5884 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -1378,6 +1378,33 @@ class TestBuildSafeEnv: assert "DATABASE_URL" not in result assert "API_SECRET" not in result + def test_windows_location_vars_passed_without_secrets(self): + """Windows launcher tools need location vars, but secrets stay filtered.""" + from tools.mcp_tool import _build_safe_env + + fake_env = { + "PATH": r"C:\Windows\System32", + "ProgramFiles": r"C:\Program Files", + "ProgramData": r"C:\ProgramData", + "ProgramW6432": r"C:\Program Files", + "LOCALAPPDATA": r"C:\Users\alice\AppData\Local", + "APPDATA": r"C:\Users\alice\AppData\Roaming", + "USERPROFILE": r"C:\Users\alice", + "GITHUB_TOKEN": "ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + "OPENAI_API_KEY": "sk-proj-abc123", + } + with patch.dict("os.environ", fake_env, clear=True): + result = _build_safe_env(None) + + assert result["ProgramFiles"] == r"C:\Program Files" + assert result["ProgramData"] == r"C:\ProgramData" + assert result["ProgramW6432"] == r"C:\Program Files" + assert result["LOCALAPPDATA"].endswith("Local") + assert result["APPDATA"].endswith("Roaming") + assert result["USERPROFILE"] == r"C:\Users\alice" + assert "GITHUB_TOKEN" not in result + assert "OPENAI_API_KEY" not in result + # --------------------------------------------------------------------------- # _sanitize_error diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 7287a45dbe..a95739fe81 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -268,6 +268,38 @@ _SAFE_ENV_KEYS = frozenset({ "PATH", "HOME", "USER", "LANG", "LC_ALL", "TERM", "SHELL", "TMPDIR", }) +_SAFE_ENV_KEYS_CASE_INSENSITIVE = frozenset({ + # Windows process/location vars. These are needed by launcher-style tools + # such as Docker Desktop's MCP plugin discovery, and do not carry secrets. + "ALLUSERSPROFILE", + "APPDATA", + "COMMONPROGRAMFILES", + "COMMONPROGRAMFILES(X86)", + "COMMONPROGRAMW6432", + "COMPUTERNAME", + "COMSPEC", + "HOMEDRIVE", + "HOMEPATH", + "LOCALAPPDATA", + "NUMBER_OF_PROCESSORS", + "OS", + "PATHEXT", + "PROCESSOR_ARCHITECTURE", + "PROGRAMDATA", + "PROGRAMFILES", + "PROGRAMFILES(X86)", + "PROGRAMW6432", + "PUBLIC", + "SYSTEMDRIVE", + "SYSTEMROOT", + "TEMP", + "TMP", + "USERDOMAIN", + "USERNAME", + "USERPROFILE", + "WINDIR", +}) + # Regex for credential patterns to strip from error messages _CREDENTIAL_PATTERN = re.compile( r"(?:" @@ -305,7 +337,11 @@ def _build_safe_env(user_env: Optional[dict]) -> dict: """ env = {} for key, value in os.environ.items(): - if key in _SAFE_ENV_KEYS or key.startswith("XDG_"): + if ( + key in _SAFE_ENV_KEYS + or key.upper() in _SAFE_ENV_KEYS_CASE_INSENSITIVE + or key.startswith("XDG_") + ): env[key] = value if user_env: env.update(user_env) diff --git a/website/docs/reference/cli-commands.md b/website/docs/reference/cli-commands.md index abf172e9fa..03708422f6 100644 --- a/website/docs/reference/cli-commands.md +++ b/website/docs/reference/cli-commands.md @@ -1180,7 +1180,7 @@ Manage MCP (Model Context Protocol) server configurations and run Hermes as an M | `catalog` | List Nous-approved MCPs (plain text, scriptable). | | `install ` | Install a catalog entry (e.g. `hermes mcp install n8n`). | | `serve [-v\|--verbose]` | Run Hermes as an MCP server — expose conversations to other agents. | -| `add [--url URL] [--command CMD] [--args ...] [--auth oauth\|header]` | Add a custom MCP server with automatic tool discovery. | +| `add [--url URL] [--command CMD] [--auth oauth\|header] [--args ...]` | Add a custom MCP server with automatic tool discovery. `--args` passes the remaining argv to the stdio command, so put it last. | | `remove ` (alias: `rm`) | Remove an MCP server from config. | | `list` (alias: `ls`) | List configured MCP servers. | | `test ` | Test connection to an MCP server. |