From 79dbb1450ec404a2240529f5554526da4cfec498 Mon Sep 17 00:00:00 2001 From: Austin Pickett Date: Sun, 27 Sep 2026 00:38:21 -0400 Subject: [PATCH] fix(tools): make managed Node authoritative in the MCP stdio PATH `_prepend_path` inserted the resolved command's directory only when it was absent from the child's PATH. The Hermes installer appends its managed Node dir to the user PATH, so for anyone with a system Node (<22.12) earlier on PATH the check no-oped and the managed dir stayed behind it. npm lifecycle children (`node install.js`) then resolved the older system Node and failed with ERR_REQUIRE_ESM even though Hermes had provisioned a compatible runtime. Strip every existing case/trailing-separator variant of the directory first, then prepend it, so the canonical entry is the one that wins and PATH does not grow duplicates. Fixes #82309 --- tests/tools/test_mcp_tool_issue_948.py | 64 ++++++++++++++++++++++++++ tools/mcp_tool_common.py | 25 ++++++++-- 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/tests/tools/test_mcp_tool_issue_948.py b/tests/tools/test_mcp_tool_issue_948.py index ced0dda4e5..4544c06fdc 100644 --- a/tests/tools/test_mcp_tool_issue_948.py +++ b/tests/tools/test_mcp_tool_issue_948.py @@ -7,6 +7,7 @@ from unittest.mock import AsyncMock, MagicMock, patch from tools.mcp_tool import MCPServerTask, _MCP_AVAILABLE from tools.mcp_tool_errors import _format_connect_error +from tools.mcp_tool_common import _prepend_path from tools.mcp_tool_config import _node_fallback, _resolve_stdio_command from tools.mcp_tool_config import _which_with_config_pathext @@ -340,3 +341,66 @@ def test_run_stdio_malware_check_times_out_fail_open(): assert elapsed < 1.0, f"startup did not fail-open promptly ({elapsed:.1f}s)" asyncio.run(_test()) + + +# --------------------------------------------------------------------------- +# #82309: a managed dir that is ALREADY on the child's PATH (the Hermes +# installer appends its managed Node dir) must still end up FIRST. "Prepend +# only when absent" left the older system Node ahead of it, so npm lifecycle +# children (`node install.js`) resolved the system Node and died with +# ERR_REQUIRE_ESM even though Hermes had provisioned a compatible runtime. +# --------------------------------------------------------------------------- + + +def test_prepend_path_makes_an_already_present_dir_first(): + """The reported layout — system Node first, managed dir already present + behind it — must come out with the managed dir first.""" + managed = os.path.join(os.sep, "managed", "node", "bin") + system = os.path.join(os.sep, "system", "node") + env = _prepend_path({"PATH": os.pathsep.join([system, "mid-dir", managed]), "KEEP": "v"}, managed) + + assert env["PATH"].split(os.pathsep) == [managed, system, "mid-dir"] + assert env["KEEP"] == "v" + + +def test_prepend_path_collapses_duplicate_entries(): + """Prepending an already-present dir must not grow PATH a duplicate.""" + managed = "/managed/node/bin" + env = _prepend_path({"PATH": os.pathsep.join([managed, "/usr/bin", managed])}, managed) + + assert env["PATH"].split(os.pathsep) == [managed, "/usr/bin"] + + +def test_prepend_path_collapses_windows_case_and_separator_variants(monkeypatch): + """Windows PATH lookup is case- and separator-insensitive, so every variant + of the managed dir has to be removed for the canonical entry to win.""" + monkeypatch.setattr(os, "pathsep", ";") + monkeypatch.setattr(sys, "platform", "win32") + managed = r"C:\Users\x\AppData\Local\hermes\node" + variant = "c:\\users\\x\\appdata\\local\\hermes\\node" + "\\" + env = _prepend_path( + {"PATH": ";".join([r"C:\Program Files\nodejs", variant, r"C:\tools"])}, managed + ) + + assert env["PATH"].split(";") == [managed, r"C:\Program Files\nodejs", r"C:\tools"] + + +def test_resolve_stdio_command_displaces_a_system_node_already_on_path(tmp_path, monkeypatch): + """End-to-end: with the managed dir already on the child PATH behind a system + Node dir, the resolved env must put the managed dir first so the spawned + launcher's shebang/children (`/usr/bin/env node`) get the managed Node.""" + node_bin = tmp_path / "node" / "bin" + node_bin.mkdir(parents=True) + npx_path = node_bin / "npx" + npx_path.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + npx_path.chmod(0o755) + system_bin = tmp_path / "system-node" + system_bin.mkdir() + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + inherited = os.pathsep.join([str(system_bin), "/usr/bin", str(node_bin)]) + + with patch("tools.mcp_tool_config.shutil.which", return_value=None): + command, env = _resolve_stdio_command("npx", {"PATH": inherited}) + + assert command == str(npx_path) + assert env["PATH"].split(os.pathsep) == [str(node_bin), str(system_bin), "/usr/bin"] diff --git a/tools/mcp_tool_common.py b/tools/mcp_tool_common.py index 8104ee628c..24a9cd7a60 100644 --- a/tools/mcp_tool_common.py +++ b/tools/mcp_tool_common.py @@ -3,9 +3,11 @@ error-text sanitising, numeric/bool coercion, timeouts and jitter. No origin sta import logging import math +import ntpath import os import random import re +import sys from typing import Any, Optional logger = logging.getLogger("tools.mcp_tool") @@ -94,14 +96,29 @@ def _exc_str(exc: BaseException) -> str: return text or repr(exc) +def _path_comparison_key(entry: str) -> str: + """Normalize a PATH entry under the active platform's path semantics, so + ``C:\\Node\\`` and ``c:\\node`` compare equal on Windows and stay distinct on POSIX.""" + path_module = ntpath if sys.platform == "win32" else os.path + return path_module.normcase(path_module.normpath(entry)) + + def _prepend_path(env: dict, directory: str) -> dict: - """Prepend *directory* to env PATH if it is not already present.""" + """Make *directory* the FIRST PATH entry, collapsing existing variants of it. + + Prepending only when *directory* was absent left a directory that is already + on PATH — the Hermes installer appends its managed Node dir — behind an older + system Node. npm lifecycle children (`node install.js`) then resolve the + system Node through PATH and die with ERR_REQUIRE_ESM (#82309). Every + existing case/trailing-separator variant is stripped first, so this entry is + the one that wins and PATH does not grow duplicates. + """ updated = dict(env or {}) if directory: parts = [part for part in updated.get("PATH", "").split(os.pathsep) if part] - if directory not in parts: - parts = [directory, *parts] - updated["PATH"] = os.pathsep.join(parts) if parts else directory + key = _path_comparison_key(directory) + remaining = [part for part in parts if _path_comparison_key(part) != key] + updated["PATH"] = os.pathsep.join([directory, *remaining]) return updated