diff --git a/acp_adapter/entry.py b/acp_adapter/entry.py index 9ce6281824c..5048b702598 100644 --- a/acp_adapter/entry.py +++ b/acp_adapter/entry.py @@ -23,6 +23,11 @@ except ModuleNotFoundError: # new code but ``uv pip install -e .`` didn't finish. Missing bootstrap # means UTF-8 stdio setup is skipped on Windows; POSIX is unaffected. pass +else: + # Stop a ``utils/``/``proxy/``/``ui/`` package in the launch directory from + # shadowing Hermes's own modules — ``hermes acp`` can be started from any + # cwd, including a project that has same-named packages on its path. + hermes_bootstrap.harden_import_path() import argparse import asyncio diff --git a/hermes_bootstrap.py b/hermes_bootstrap.py index 890336c3448..e43d9db80b9 100644 --- a/hermes_bootstrap.py +++ b/hermes_bootstrap.py @@ -122,6 +122,42 @@ def apply_windows_utf8_bootstrap() -> bool: return True +def harden_import_path(src_root: str | None = None) -> None: + """Stop a package in the current directory from shadowing Hermes modules. + + Hermes ships top-level modules with common names (``utils``, ``proxy``, + ``ui``). Python always seeds ``sys.path`` with the current directory, so + launching an entry point from a project that has its own ``utils/`` package + makes ``from utils import ...`` resolve to the *user's* package and crash + with an ImportError before the gateway can even start. + + The current directory reaches ``sys.path`` two ways, and a complete guard + has to handle both: + + - As the empty string ``""`` (or ``"."``) that Python inserts at + ``sys.path[0]`` for ``-m`` / script launches. + - As its own *absolute* path, when a venv activation or a project that + adds itself to ``PYTHONPATH`` puts the directory there explicitly. + + We drop the relative forms outright, then force the real Hermes source root + to the front — relocating it ahead of any absolute cwd entry rather than + only inserting when absent, so an absolute cwd path can't keep winning. + + ``src_root`` defaults to the directory this module lives in, which is the + repository root for every shipped entry point, so the guard is + self-sufficient and does not depend on the spawner exporting an env var. + """ + root = src_root or os.environ.get("HERMES_PYTHON_SRC_ROOT") or os.path.dirname( + os.path.abspath(__file__) + ) + + sys.path[:] = [p for p in sys.path if p not in ("", ".")] + + root_abs = os.path.abspath(root) + sys.path[:] = [p for p in sys.path if os.path.abspath(p) != root_abs] + sys.path.insert(0, root) + + # Apply on import — entry points just need ``import hermes_bootstrap`` # (or ``from hermes_bootstrap import apply_windows_utf8_bootstrap``) at # the very top of their module, before importing anything else. The diff --git a/tests/test_hermes_bootstrap.py b/tests/test_hermes_bootstrap.py index 69f3c6b7c03..50a582bf998 100644 --- a/tests/test_hermes_bootstrap.py +++ b/tests/test_hermes_bootstrap.py @@ -311,3 +311,88 @@ class TestEntryPointsImportBootstrap: f"configured before anything else initializes. Move the " f"'import hermes_bootstrap' line to be the first import." ) + + +class TestHardenImportPath: + """harden_import_path() must keep a same-named package in the launch + directory from shadowing Hermes's own top-level modules — covering both + the relative ('' / '.') and absolute-path forms the cwd can take on + sys.path (issue #51286).""" + + def _run(self, hb, path_seed, env=None): + original = sys.path[:] + original_env = os.environ.get("HERMES_PYTHON_SRC_ROOT") + try: + sys.path[:] = path_seed + if env is not None: + os.environ["HERMES_PYTHON_SRC_ROOT"] = env + elif "HERMES_PYTHON_SRC_ROOT" in os.environ: + del os.environ["HERMES_PYTHON_SRC_ROOT"] + hb.harden_import_path(src_root="/opt/hermes") + return sys.path[:] + finally: + sys.path[:] = original + if original_env is None: + os.environ.pop("HERMES_PYTHON_SRC_ROOT", None) + else: + os.environ["HERMES_PYTHON_SRC_ROOT"] = original_env + + def test_relative_cwd_forms_removed(self): + hb = _fresh_import() + result = self._run(hb, ["", ".", "/opt/hermes", "/usr/lib/python"]) + assert "" not in result + assert "." not in result + + def test_src_root_forced_to_front(self): + hb = _fresh_import() + result = self._run(hb, ["", "/opt/hermes", "/usr/lib/python"]) + assert result[0] == "/opt/hermes" + + def test_absolute_cwd_path_loses_to_src_root(self): + # The real #51286 bug: the launch dir is present as its own absolute + # path (venv activation / a project on PYTHONPATH), ahead of the + # Hermes root. The guard must relocate Hermes to the front. + hb = _fresh_import() + result = self._run(hb, ["/home/user/tg-ws-proxy", "/opt/hermes"]) + assert result[0] == "/opt/hermes" + # The cwd absolute path may still appear (it can hold legit deps), + # but only AFTER the Hermes root. + assert result.index("/opt/hermes") < result.index("/home/user/tg-ws-proxy") + + def test_src_root_not_duplicated(self): + hb = _fresh_import() + result = self._run(hb, ["/opt/hermes", "/opt/hermes", ""]) + assert result.count("/opt/hermes") == 1 + + def test_env_var_used_when_no_arg(self): + hb = _fresh_import() + original = sys.path[:] + original_env = os.environ.get("HERMES_PYTHON_SRC_ROOT") + try: + sys.path[:] = ["", "/cwd/proj", "/usr/lib"] + os.environ["HERMES_PYTHON_SRC_ROOT"] = "/env/hermes" + hb.harden_import_path() + assert sys.path[0] == "/env/hermes" + finally: + sys.path[:] = original + if original_env is None: + os.environ.pop("HERMES_PYTHON_SRC_ROOT", None) + else: + os.environ["HERMES_PYTHON_SRC_ROOT"] = original_env + + def test_defaults_to_module_dir(self): + # With neither arg nor env var, the helper anchors on the bootstrap + # module's own directory — the repo root for shipped entry points. + hb = _fresh_import() + original = sys.path[:] + original_env = os.environ.get("HERMES_PYTHON_SRC_ROOT") + try: + sys.path[:] = ["", "/somewhere/else"] + os.environ.pop("HERMES_PYTHON_SRC_ROOT", None) + hb.harden_import_path() + expected = os.path.dirname(os.path.abspath(hb.__file__)) + assert sys.path[0] == expected + finally: + sys.path[:] = original + if original_env is not None: + os.environ["HERMES_PYTHON_SRC_ROOT"] = original_env diff --git a/tests/tui_gateway/test_entry_sys_path.py b/tests/tui_gateway/test_entry_sys_path.py index 15619d2a9fc..7f67e7ba006 100644 --- a/tests/tui_gateway/test_entry_sys_path.py +++ b/tests/tui_gateway/test_entry_sys_path.py @@ -1,100 +1,81 @@ -"""Tests for tui_gateway/entry.py sys.path hardening (issue #15989). +"""Tests for tui_gateway/entry.py sys.path hardening (issues #15989, #51286). -When the TUI backend is spawned by Node.js, the Python interpreter may have -'' or '.' at the front of sys.path, allowing a local utils/ directory in CWD -to shadow the installed utils module. entry.py must sanitize sys.path before -any non-stdlib import is resolved. +When the TUI backend is spawned by Node.js, the launch directory may shadow +Hermes's own top-level modules (``utils``, ``proxy``, ``ui``). entry.py must +neutralize this before any non-stdlib import is resolved, by delegating to the +shared ``hermes_bootstrap.harden_import_path`` guard. + +These tests assert the entry point wires up the real guard (rather than +re-implementing it inline) and that the guard's behavior covers both the +relative-cwd form and the absolute-cwd-path form that was the actual #51286 +failure. """ -import os -import sys -from unittest.mock import patch +import ast +import pathlib + +import hermes_bootstrap -def _reload_entry_with_env(env_overrides: dict) -> None: - """Re-execute entry.py's module-level path setup under a controlled env.""" - # We only want to exercise the sys.path fixup block, not the signal/import - # machinery that follows. We do this by running the fixup code verbatim in - # a fresh copy of sys.path rather than importing the real module (which - # would trigger tui_gateway.server imports requiring heavy mocks). - original_path = sys.path[:] - original_env = {k: os.environ.get(k) for k in env_overrides} - try: - with patch.dict(os.environ, env_overrides, clear=False): - _src_root = os.environ.get("HERMES_PYTHON_SRC_ROOT", "") - if _src_root and _src_root not in sys.path: - sys.path.insert(0, _src_root) - sys.path = [p for p in sys.path if p not in {"", "."}] - return sys.path[:] - finally: - sys.path = original_path - for k, v in original_env.items(): - if v is None: - os.environ.pop(k, None) - else: - os.environ[k] = v +def _entry_source() -> str: + here = pathlib.Path(__file__).resolve() + repo_root = here.parent.parent.parent # tests/tui_gateway/ -> repo root + return (repo_root / "tui_gateway" / "entry.py").read_text(encoding="utf-8") -def test_empty_string_and_dot_removed_from_sys_path(): +def test_entry_calls_shared_harden_guard_before_heavy_imports(): + """entry.py must call hermes_bootstrap.harden_import_path() before it + imports tui_gateway.server (which pulls ``from utils import ...``).""" + source = _entry_source() + tree = ast.parse(source) + + harden_call_line = None + server_import_line = None + for node in ast.walk(tree): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "harden_import_path" + ): + harden_call_line = node.lineno + if isinstance(node, ast.ImportFrom) and (node.module or "").startswith( + "tui_gateway" + ): + if server_import_line is None: + server_import_line = node.lineno + + assert harden_call_line is not None, ( + "entry.py must call hermes_bootstrap.harden_import_path()" + ) + assert server_import_line is not None, "entry.py must import from tui_gateway" + assert harden_call_line < server_import_line, ( + "harden_import_path() must run before tui_gateway.server is imported" + ) + + +def test_entry_does_not_reimplement_guard_inline(): + """The old inline ``{'', '.'}`` strip lived in entry.py; the dedicated + helper now owns it. Guard against the inline logic creeping back.""" + source = _entry_source() + assert '{"", "."}' not in source and "{'', '.'}" not in source, ( + "entry.py should delegate to hermes_bootstrap.harden_import_path, " + "not re-implement the sys.path strip inline" + ) + + +def test_guard_handles_absolute_cwd_path(): + """The #51286 case: the launch dir is on sys.path as its own absolute + path, ahead of the Hermes root. harden_import_path must relocate the + Hermes root to the front so ``from utils import ...`` resolves to Hermes.""" + import sys + original = sys.path[:] try: - sys.path.insert(0, "") - sys.path.insert(0, ".") - assert "" in sys.path - assert "." in sys.path - - # Run the entry.py fixup logic directly - sys.path = [p for p in sys.path if p not in {"", "."}] - - assert "" not in sys.path - assert "." not in sys.path + sys.path[:] = ["/home/user/tg-ws-proxy", "/opt/hermes", "/usr/lib"] + hermes_bootstrap.harden_import_path(src_root="/opt/hermes") + assert sys.path[0] == "/opt/hermes" + assert sys.path.index("/opt/hermes") < sys.path.index( + "/home/user/tg-ws-proxy" + ) finally: - sys.path = original - - -def test_hermes_src_root_inserted_at_front(): - original = sys.path[:] - try: - fake_root = "/fake/hermes/src" - with patch.dict(os.environ, {"HERMES_PYTHON_SRC_ROOT": fake_root}): - _src_root = os.environ.get("HERMES_PYTHON_SRC_ROOT", "") - if _src_root and _src_root not in sys.path: - sys.path.insert(0, _src_root) - sys.path = [p for p in sys.path if p not in {"", "."}] - - assert sys.path[0] == fake_root - finally: - sys.path = original - - -def test_src_root_not_duplicated_if_already_present(): - original = sys.path[:] - try: - fake_root = "/already/present" - sys.path.insert(0, fake_root) - count_before = sys.path.count(fake_root) - - with patch.dict(os.environ, {"HERMES_PYTHON_SRC_ROOT": fake_root}): - _src_root = os.environ.get("HERMES_PYTHON_SRC_ROOT", "") - if _src_root and _src_root not in sys.path: - sys.path.insert(0, _src_root) - sys.path = [p for p in sys.path if p not in {"", "."}] - - assert sys.path.count(fake_root) == count_before - finally: - sys.path = original - - -def test_no_src_root_env_does_not_crash(): - original = sys.path[:] - try: - env = {k: v for k, v in os.environ.items() if k != "HERMES_PYTHON_SRC_ROOT"} - with patch.dict(os.environ, {}, clear=True): - os.environ.update(env) - _src_root = os.environ.get("HERMES_PYTHON_SRC_ROOT", "") - if _src_root and _src_root not in sys.path: - sys.path.insert(0, _src_root) - sys.path = [p for p in sys.path if p not in {"", "."}] - # No exception raised - finally: - sys.path = original + sys.path[:] = original diff --git a/tui_gateway/entry.py b/tui_gateway/entry.py index 0993a263c30..89b30b17322 100644 --- a/tui_gateway/entry.py +++ b/tui_gateway/entry.py @@ -1,15 +1,14 @@ import os import sys -# Guard against a local utils/ (or other package) in CWD shadowing installed -# hermes modules. hermes_cli sets HERMES_PYTHON_SRC_ROOT before spawning this -# subprocess; inserting it first ensures the installed packages win. -_src_root = os.environ.get("HERMES_PYTHON_SRC_ROOT", "") -if _src_root and _src_root not in sys.path: - sys.path.insert(0, _src_root) -# Strip '' and '.' — both resolve to CWD at import time and can let a local -# directory shadow installed packages. -sys.path = [p for p in sys.path if p not in {"", "."}] +# Stop a ``utils/`` (or ``proxy/``, ``ui/``) package in the launch directory +# from shadowing Hermes's own top-level modules. ``hermes_bootstrap`` lives at +# the repo root next to this package, so importing it is safe before the guard +# runs (its name won't collide with a user package), and it owns the canonical +# path-hardening logic shared with the other entry points. +import hermes_bootstrap + +hermes_bootstrap.harden_import_path() import json import logging diff --git a/ui-tui/src/gatewayClient.ts b/ui-tui/src/gatewayClient.ts index 88ddc0fcdc3..a2008374acf 100644 --- a/ui-tui/src/gatewayClient.ts +++ b/ui-tui/src/gatewayClient.ts @@ -344,6 +344,9 @@ export class GatewayClient extends EventEmitter { const pyPath = env.PYTHONPATH?.trim() env.PYTHONPATH = pyPath ? `${root}${delimiter}${pyPath}` : root + // Tell the gateway child where the Hermes source root is so its import + // guard can force it ahead of any same-named package in the launch cwd. + env.HERMES_PYTHON_SRC_ROOT = root this.startReadyTimer(python, cwd) this.proc = spawn(python, ['-m', 'tui_gateway.entry'], { cwd, env, stdio: ['pipe', 'pipe', 'pipe'] }) this.lifecycle(`[lifecycle] spawned gateway child ${describeChild(this.proc)} python=${python} cwd=${cwd}`)