mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-14 09:11:54 +00:00
fix(mcp): preserve stdio argv passthrough
This commit is contained in:
parent
ee1a744ace
commit
dca11b6650
8 changed files with 226 additions and 17 deletions
|
|
@ -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 <command argv>`.
|
||||
|
||||
``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()
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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"]
|
||||
|
|
|
|||
|
|
@ -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"]
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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 <name>` | 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 <name> [--url URL] [--command CMD] [--args ...] [--auth oauth\|header]` | Add a custom MCP server with automatic tool discovery. |
|
||||
| `add <name> [--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 <name>` (alias: `rm`) | Remove an MCP server from config. |
|
||||
| `list` (alias: `ls`) | List configured MCP servers. |
|
||||
| `test <name>` | Test connection to an MCP server. |
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue