diff --git a/tests/tools/test_local_env_blocklist.py b/tests/tools/test_local_env_blocklist.py index 570ad9e90d..0f081be9e3 100644 --- a/tests/tools/test_local_env_blocklist.py +++ b/tests/tools/test_local_env_blocklist.py @@ -9,6 +9,7 @@ See: https://github.com/NousResearch/hermes-agent/issues/1264 """ import os +import sys import threading from pathlib import Path from unittest.mock import MagicMock, patch @@ -321,7 +322,7 @@ class TestPythonpathSelectiveStrip: venv's site-packages (Python 3.11) into PYTHONPATH. When this leaks into subprocesses running a different Python (e.g. 3.13), 3.11 C extensions appear on sys.path and crash with ImportError. - ``_strip_mismatched_site_packages`` surgically removes only the + ``_strip_hermes_owned_pythonpath`` surgically removes only the entries Hermes itself owns (repo root, own venv site-packages), preserving user paths — including user paths whose names merely contain another Python version. @@ -329,7 +330,7 @@ class TestPythonpathSelectiveStrip: def test_hermes_venv_site_packages_stripped(self): """A site-packages entry under the Hermes venv is removed.""" - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath import sys # Construct a path that looks like the Hermes venv site-packages. @@ -342,7 +343,7 @@ class TestPythonpathSelectiveStrip: env = { "PYTHONPATH": os.pathsep.join([venv_sp, "/home/user/my-lib"]), } - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) assert "PYTHONPATH" in env entries = env["PYTHONPATH"].split(os.pathsep) assert venv_sp not in entries @@ -350,10 +351,10 @@ class TestPythonpathSelectiveStrip: def test_user_pythonpath_preserved(self): """User PYTHONPATH entries pass through untouched.""" - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath user_pp = os.pathsep.join(["/opt/my-lib", "/another/path"]) env = {"PYTHONPATH": user_pp} - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) assert env.get("PYTHONPATH") == user_pp def test_other_version_site_packages_preserved(self): @@ -365,7 +366,7 @@ class TestPythonpathSelectiveStrip: judged against the BACKEND's interpreter version (P2, #74817 follow-up). Regression: prior Check 1 stripped these. """ - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath import sys # Use a version different from the running interpreter. @@ -378,7 +379,7 @@ class TestPythonpathSelectiveStrip: env = { "PYTHONPATH": os.pathsep.join([other_sp, "/home/user/my-lib"]), } - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) assert "PYTHONPATH" in env entries = env["PYTHONPATH"].split(os.pathsep) assert other_sp in entries @@ -388,11 +389,11 @@ class TestPythonpathSelectiveStrip: """A user's python2.7/site-packages entry is preserved — path ownership, not version, decides stripping (P2, #74817 follow-up). """ - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath env = { "PYTHONPATH": "/old/lib/python2.7/site-packages:/home/user/lib", } - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) assert "PYTHONPATH" in env entries = env["PYTHONPATH"].split(os.pathsep) assert "/old/lib/python2.7/site-packages" in entries @@ -403,14 +404,14 @@ class TestPythonpathSelectiveStrip: must never be stripped (P1, #74817 follow-up). Regression: prior Check 1 deleted these because it keyed on the version component alone. """ - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath user_pp = os.pathsep.join([ "/opt/tools/python3.13/bin", "/opt/downloads/python3.13", "/custom/python3.13", ]) env = {"PYTHONPATH": user_pp} - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) assert env.get("PYTHONPATH") == user_pp def test_windows_backslash_paths(self): @@ -423,9 +424,10 @@ class TestPythonpathSelectiveStrip: (including site-packages paths for another Python version) are never destroyed. On a real Windows host, Path splits on backslashes and Hermes venv site-packages entries are stripped - by the same Hermes-owned check. + by the same Hermes-owned check (covered by the Windows-only test + below). """ - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath import sys pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" @@ -436,7 +438,7 @@ class TestPythonpathSelectiveStrip: } # Mock os.pathsep to ';' (Windows) just for the strip call. with patch("os.pathsep", ";"): - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) assert "PYTHONPATH" in env entries = env["PYTHONPATH"].split(";") # Both survive on POSIX: user paths must always be preserved, and @@ -444,26 +446,106 @@ class TestPythonpathSelectiveStrip: assert hermes_win in entries assert user_win in entries + @pytest.mark.windows_only + def test_windows_hermes_owned_paths_stripped(self): + """On Windows, a Hermes venv site-packages entry written with + backslashes is stripped by the same Hermes-owned check, while a + user Windows path is preserved. Windows-only: POSIX ``Path`` does + not split on backslashes, so this cannot be meaningfully simulated + on a POSIX host.""" + from tools.environments.local import _strip_hermes_owned_pythonpath + import sys + + pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" + venv_sp = str( + __import__("pathlib").Path(sys.prefix) / "Lib" / "site-packages" + ) + # Windows form: C:\...\venv\Lib\site-packages (backslashes) + hermes_win = venv_sp.replace("\\", "\\\\").replace("/", "\\\\") + user_win = "D:\\\\user\\\\lib" + env = { + "PYTHONPATH": ";".join([hermes_win, user_win]), + } + _strip_hermes_owned_pythonpath(env) + entries = env["PYTHONPATH"].split(";") + assert hermes_win not in entries + assert user_win in entries + def test_empty_pythonpath_unchanged(self): """An empty PYTHONPATH is a no-op (falsy -> early return).""" - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath env = {"PYTHONPATH": ""} - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) # Empty string is falsy, so the function returns early without # modifying the dict. The key stays as-is (empty string). assert env.get("PYTHONPATH") == "" + def test_mixed_ordering_user_and_hermes_preserves_user_order(self): + """Mixed user/Hermes/user entries: user entries survive in their + original relative order after Hermes-owned entries are removed.""" + from tools.environments.local import _strip_hermes_owned_pythonpath + import sys + + pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" + venv_sp = str( + __import__("pathlib").Path(sys.prefix) / "lib" / pyver / "site-packages" + ) + local_file = Path( + __import__("tools.environments.local", fromlist=["__file__"]).__file__ + ).resolve() + repo_root = str(local_file.parents[2]) + env = { + "PYTHONPATH": os.pathsep.join([ + "/first/user/lib", + repo_root, + "/second/user/lib", + venv_sp, + "/third/user/lib", + ]), + } + _strip_hermes_owned_pythonpath(env) + pp = env.get("PYTHONPATH", "") + entries = pp.split(os.pathsep) if pp else [] + assert entries == ["/first/user/lib", "/second/user/lib", "/third/user/lib"] + + def test_duplicate_hermes_entries_all_stripped(self): + """Every duplicate Hermes-owned entry is removed; user duplicates + follow the existing contract (no unrelated dedup).""" + from tools.environments.local import _strip_hermes_owned_pythonpath + import sys + + pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" + venv_sp = str( + __import__("pathlib").Path(sys.prefix) / "lib" / pyver / "site-packages" + ) + local_file = Path( + __import__("tools.environments.local", fromlist=["__file__"]).__file__ + ).resolve() + repo_root = str(local_file.parents[2]) + env = { + "PYTHONPATH": os.pathsep.join([ + venv_sp, + "/user/lib", + venv_sp, # duplicate Hermes entry + "/user/lib", # duplicate user entry — preserved as-is + ]), + } + _strip_hermes_owned_pythonpath(env) + pp = env.get("PYTHONPATH", "") + entries = pp.split(os.pathsep) if pp else [] + assert entries == ["/user/lib", "/user/lib"] + def test_no_pythonpath_key(self): """Missing PYTHONPATH key is a no-op.""" - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath env = {"PATH": "/usr/bin"} - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) assert "PYTHONPATH" not in env def test_all_entries_stripped_removes_key(self): """If all entries are Hermes-owned and stripped, PYTHONPATH key is removed entirely.""" - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath import sys pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" @@ -472,7 +554,7 @@ class TestPythonpathSelectiveStrip: ) env = {"PYTHONPATH": venv_sp} - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) assert "PYTHONPATH" not in env def test_make_run_env_strips_hermes_venv_pythonpath(self): @@ -537,11 +619,11 @@ class TestPythonpathSelectiveStrip: def test_scrub_child_env_strips_hermes_venv_pythonpath(self): """execute_code's _scrub_child_env path: after scrubbing, Hermes venv site-packages entries should be stripped when - _strip_mismatched_site_packages is applied (as the spawn path does), + _strip_hermes_owned_pythonpath is applied (as the spawn path does), while user entries (even for another Python version) are preserved. """ from tools.code_execution_tool import _scrub_child_env - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath import sys pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" @@ -558,7 +640,7 @@ class TestPythonpathSelectiveStrip: # The scrubber passes PYTHONPATH through (it's in _SAFE_ENV_PREFIXES). assert "PYTHONPATH" in scrubbed # Now apply the selective strip (as the spawn path does). - _strip_mismatched_site_packages(scrubbed) + _strip_hermes_owned_pythonpath(scrubbed) pp = scrubbed.get("PYTHONPATH", "") entries = pp.split(os.pathsep) if pp else [] assert venv_sp not in entries @@ -577,7 +659,7 @@ class TestPythonpathSelectiveStrip: (e.g. ``parents[1]`` resolving to ``tools/``) would cause this test to fail instead of silently passing. """ - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath # Independently compute the real repo root: local.py lives at # tools/environments/local.py, so the repo root is parents[2]. @@ -587,22 +669,25 @@ class TestPythonpathSelectiveStrip: env = { "PYTHONPATH": os.pathsep.join([real_repo_root, "/home/user/my-lib"]), } - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) pp = env.get("PYTHONPATH", "") entries = pp.split(os.pathsep) if pp else [] assert real_repo_root not in entries assert "/home/user/my-lib" in entries - def test_repo_root_direct_child_stripped(self): - """A direct child of the repo root (depth=1) is stripped. + def test_repo_root_direct_child_preserved(self): + """A direct child of the repo root (depth=1) is PRESERVED. - Check 3's depth rule is ``depth <= 1``: the repo root itself is - depth=0 (covered above), and a top-level package directory like - ``/tools`` is depth=1. Both are stripped because Electron - prepends exactly these shallow paths so ``import tools`` works in - the backend, and they shadow user packages of the same name. + Independent audit of every real launcher producer (Electron + ``electron-main.mjs``, ``gateway/run.py::_ensure_windows_gateway_venv_imports``, + ``cron/scheduler.py::_windows_cron_python_invocation``, + ``tui_gateway/host_supervisor.py``) shows they all inject the exact + repo root and/or the venv site-packages — none injects + ``/tools`` or another direct child as an independent + PYTHONPATH entry. A user path that merely happens to live under + the repo directory must therefore be preserved. """ - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath local_file = Path(__import__("tools.environments.local", fromlist=["__file__"]).__file__).resolve() real_repo_root = local_file.parents[2] @@ -611,20 +696,63 @@ class TestPythonpathSelectiveStrip: env = { "PYTHONPATH": os.pathsep.join([direct_child, "/home/user/my-lib"]), } - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) pp = env.get("PYTHONPATH", "") entries = pp.split(os.pathsep) if pp else [] - assert direct_child not in entries + assert direct_child in entries assert "/home/user/my-lib" in entries + def test_repo_root_junction_alias_stripped(self): + """A PYTHONPATH entry using the unresolved (HERMES_HOME/junction) + spelling of the repo root is stripped. + + On Windows the gateway launcher renders Hermes-owned paths under the + configured HERMES_HOME spelling, which may be a symlink/junction to + another drive (``_preserve_hermes_home_path`` in + ``hermes_cli/gateway_windows.py``). ``_hermes_repo_root`` is the + RESOLVED physical path, so the launcher's spelling differs lexically; + the alias set must still recognize it as Hermes-owned. This test + monkeypatches the aliases to a pair whose lexical forms differ, the + way a junction would, and verifies the launcher's spelling is + stripped while a nested user path under the same prefix is not. + """ + from tools.environments.local import ( + _strip_hermes_owned_pythonpath, + _hermes_repo_root_aliases, + ) + from pathlib import Path as _Path + + resolved_root = _Path("/data/hermes/hermes-agent") + junction_root = _Path("/home/u/AppData/Local/hermes/hermes-agent") + with patch( + "tools.environments.local._hermes_repo_root_aliases", + (resolved_root, junction_root), + ): + # Launcher spelling (junction form) of the exact repo root. + env = {"PYTHONPATH": str(junction_root)} + _strip_hermes_owned_pythonpath(env) + assert "PYTHONPATH" not in env + + # Nested user path under the same junction prefix survives. + env = {"PYTHONPATH": os.pathsep.join([ + str(junction_root / "user-data"), + "/home/user/my-lib", + ])} + _strip_hermes_owned_pythonpath(env) + pp = env.get("PYTHONPATH", "") + entries = pp.split(os.pathsep) if pp else [] + assert str(junction_root / "user-data") in entries + assert "/home/user/my-lib" in entries + def test_deep_path_under_repo_root_preserved(self): """A deeper path under the repo root (depth=2) is preserved. ``/tools/environments`` is depth=2, past the ``depth <= 1`` cutoff. Such a path is not something Electron injects and may be - a legitimate user library path, so it must survive Check 3. + a legitimate user library path, so it must survive the repo-root + ownership check. """ - from tools.environments.local import _strip_mismatched_site_packages + from tools.environments.local import _strip_hermes_owned_pythonpath local_file = Path(__import__("tools.environments.local", fromlist=["__file__"]).__file__).resolve() real_repo_root = local_file.parents[2] @@ -633,7 +761,7 @@ class TestPythonpathSelectiveStrip: env = { "PYTHONPATH": os.pathsep.join([deep_path, "/home/user/my-lib"]), } - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) pp = env.get("PYTHONPATH", "") entries = pp.split(os.pathsep) if pp else [] assert deep_path in entries @@ -696,6 +824,28 @@ class TestPythonhomeSanitized: from tools.environments.local import _ACTIVE_VENV_MARKER_VARS assert "PYTHONHOME" in _ACTIVE_VENV_MARKER_VARS + def test_build_subprocess_env_no_scrub_preserves_pythonhome(self): + """``build_subprocess_env(scrub_secrets=False)`` is the documented + byte-for-byte escape hatch: no key is removed, so PYTHONHOME (and + everything else) survives there by contract, not by omission. + + Callers that explicitly opt out of scrubbing (git credential flows, + secret CLIs) must not have their environment silently altered — this + test pins that exception as intentional. + """ + from tools.environments.local import build_subprocess_env + base = { + "PATH": "/usr/bin:/bin", + "HOME": "/home/user", + "PYTHONHOME": "/opt/hermes-venv", + "VIRTUAL_ENV": "/opt/hermes-venv", + "SERVICE_TOKEN": "s3cr3t", + } + result = build_subprocess_env(base, scrub_secrets=False) + assert result.get("PYTHONHOME") == "/opt/hermes-venv" + assert result.get("VIRTUAL_ENV") == "/opt/hermes-venv" + assert result.get("SERVICE_TOKEN") == "s3cr3t" + class TestProfileScopedPassthrough: def test_make_run_env_uses_active_profile_for_passthrough(self, monkeypatch): diff --git a/tools/code_execution_tool.py b/tools/code_execution_tool.py index c5749c8c92..9b550f86fb 100644 --- a/tools/code_execution_tool.py +++ b/tools/code_execution_tool.py @@ -1483,16 +1483,15 @@ def execute_code( # external venv; exposing Hermes's site-packages to that interpreter # can mix incompatible compiled extensions (for example, Python 3.12 # NumPy with a Python 3.9 project interpreter). - # Before re-injecting PYTHONPATH, strip any mismatched site-packages - # entries that leaked through _scrub_child_env (PYTHONPATH is in - # _SAFE_ENV_PREFIXES so it passes the scrub). Cross-version entries - # (e.g. python3.11 site-packages injected by systemd/Electron) would - # poison the sandbox's sys.path with ABI-incompatible C extensions - # (#74817); Hermes venv/repo-root entries are redundant because the - # correct ones are re-added below, gated on the child interpreter - # actually being the Hermes environment. - from tools.environments.local import _strip_mismatched_site_packages - _strip_mismatched_site_packages(child_env) + # + # Before re-injecting PYTHONPATH, strip Hermes-owned entries that + # leaked through _scrub_child_env (PYTHONPATH is in _SAFE_ENV_PREFIXES + # so it passes the scrub). The sandbox runs the SAME Python as + # Hermes, so the Hermes venv entries are redundant — and if they + # came from a different Hermes venv they would poison the sandbox's + # sys.path with ABI-incompatible C extensions (#74817). + from tools.environments.local import _strip_hermes_owned_pythonpath + _strip_hermes_owned_pythonpath(child_env) _hermes_root = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) _existing_pp = child_env.get("PYTHONPATH", "") _pp_parts = [tmpdir] diff --git a/tools/environments/local.py b/tools/environments/local.py index 04d91e0b8e..4ea5f53c38 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -357,7 +357,7 @@ _HERMES_PROVIDER_ENV_BLOCKLIST = _build_provider_env_blocklist() # child can set it explicitly in the command. # # PYTHONPATH is NOT included here — it's handled by -# _strip_mismatched_site_packages() which removes only Hermes-owned entries, +# _strip_hermes_owned_pythonpath() which removes only Hermes-owned entries, # preserving user-set paths. _ACTIVE_VENV_MARKER_VARS = ("VIRTUAL_ENV", "CONDA_PREFIX", "PYTHONHOME") @@ -519,7 +519,7 @@ def _sanitize_subprocess_env(base_env: dict | None, extra_env: dict | None = Non for _marker in _ACTIVE_VENV_MARKER_VARS: sanitized.pop(_marker, None) - _strip_mismatched_site_packages(sanitized) + _strip_hermes_owned_pythonpath(sanitized) _apply_windows_msys_bash_env_defaults(sanitized) @@ -650,7 +650,7 @@ def hermes_subprocess_env(*, inherit_credentials: bool = False) -> dict[str, str for _marker in _ACTIVE_VENV_MARKER_VARS: env.pop(_marker, None) - _strip_mismatched_site_packages(env) + _strip_hermes_owned_pythonpath(env) _apply_windows_msys_bash_env_defaults(env) @@ -1338,7 +1338,7 @@ def _make_run_env(env: dict) -> dict: for _marker in _ACTIVE_VENV_MARKER_VARS: run_env.pop(_marker, None) - _strip_mismatched_site_packages(run_env) + _strip_hermes_owned_pythonpath(run_env) _apply_windows_msys_bash_env_defaults(run_env) @@ -1381,6 +1381,19 @@ _hermes_venv_root: Path = Path(sys.prefix) #: can shadow local packages. _hermes_repo_root: Path = Path(__file__).resolve().parents[2] +#: Alternate spellings of the repo root that Hermes launchers may emit. +#: ``Path(__file__).resolve()`` canonicalizes symlinks/junctions, but the +#: Windows gateway launcher deliberately renders Hermes-owned paths under +#: the configured HERMES_HOME spelling (which may be a junction to another +#: drive — see ``hermes_cli/gateway_windows.py::_preserve_hermes_home_path``). +#: ``Path(__file__)`` (unresolved) keeps that spelling, so a PYTHONPATH +#: entry written by the launcher still matches even though it differs +#: lexically from the resolved root. +_hermes_repo_root_aliases: tuple[Path, ...] = ( + _hermes_repo_root, + Path(__file__).parents[2], +) + #: Whether the current interpreter is running inside a venv. On Python 3.3+ #: ``sys.base_prefix != sys.prefix`` indicates a venv (or virtualenv). #: ``sys.real_prefix`` is the old virtualenv (<20) marker. @@ -1435,7 +1448,7 @@ def _get_hermes_site_packages() -> list[Path]: _SITE_PACKAGES_RE = re.compile(r"(?:^|[\\/])site-packages(?:[\\/]|$)") -def _strip_mismatched_site_packages(env: dict) -> None: +def _strip_hermes_owned_pythonpath(env: dict) -> None: """Remove Hermes-owned PYTHONPATH entries from subprocess environments. The Desktop Electron process (and other Hermes launchers) prepend the @@ -1454,7 +1467,9 @@ def _strip_mismatched_site_packages(env: dict) -> None: 1. **Hermes repo root** - the path the Electron app prepends so the backend can ``import tools``. Subprocesses don't need it and it can - shadow local packages of the same name. + shadow local packages of the same name. Only the exact root is + stripped; direct children (``/tools`` etc.) are never injected + by any launcher and are treated as user paths. 2. **Hermes venv site-packages** - entries under the running interpreter's own venv site-packages directory. Redundant for @@ -1506,18 +1521,20 @@ def _strip_mismatched_site_packages(env: dict) -> None: # --- Check 2: Hermes repo root --- # The Electron app prepends the repo root so ``import tools`` works # in the backend. Subprocesses don't need it and it can shadow - # local packages of the same name. - if not should_strip and _is_path_under(entry_path, _hermes_repo_root): - # Only strip if the entry IS the repo root or a direct package - # dir under it (e.g. ``.../hermes-agent/tools``). Don't strip - # arbitrary user paths that happen to be nested deeper. - rel = entry_path.resolve() if entry_path.exists() else entry_path - try: - depth = len(rel.relative_to(_hermes_repo_root).parts) - except (ValueError, OSError): - depth = -1 - if depth <= 1: - should_strip = True + # local packages of the same name. Only the EXACT root is stripped: + # no launcher injects a direct child (``/tools`` etc.) as an + # independent PYTHONPATH entry, and user paths that merely happen to + # live under the repo directory must be preserved. Both the + # resolved and unresolved (HERMES_HOME/junction) spellings count as + # Hermes-owned. + if not should_strip: + for repo_root in _hermes_repo_root_aliases: + if _is_path_under(entry_path, repo_root): + # Exact root only (same path), not children. + rel = entry_path.relative_to(repo_root) + if len(rel.parts) == 0: + should_strip = True + break if should_strip: stripped.append(entry)