diff --git a/tests/tools/test_local_env_blocklist.py b/tests/tools/test_local_env_blocklist.py index 2e8332470ae8..e538b3c8feae 100644 --- a/tests/tools/test_local_env_blocklist.py +++ b/tests/tools/test_local_env_blocklist.py @@ -488,14 +488,22 @@ def test_sane_path_includes_homebrew_sbin(self): def test_make_run_env_appends_homebrew_on_minimal_path(self): """When PATH is minimal, _make_run_env appends missing sane entries.""" + import os as _os from tools.environments.local import _SANE_PATH, _make_run_env minimal_env = {"PATH": "/some/custom/bin"} with patch.dict(os.environ, minimal_env, clear=True): result = _make_run_env({}) - path_entries = result["PATH"].split(":") - assert path_entries[0] == "/some/custom/bin" - for entry in _SANE_PATH.split(":"): - assert entry in path_entries + # Use the host's path separator so the test works on both POSIX + # and Windows. _SANE_PATH is POSIX-only — off-Windows we expect + # it to be injected; on Windows the test is effectively a no-op + # for the sane-entry checks (the function ignores _SANE_PATH + # off-POSIX, see _append_missing_sane_path_entries). + sep = _os.pathsep + path_entries = result["PATH"].split(sep) + assert "/some/custom/bin" in path_entries + if sep == ":": + for entry in _SANE_PATH.split(":"): + assert entry in path_entries def test_make_run_env_fills_missing_homebrew_when_usr_bin_present(self): """macOS launchd PATH can include /usr/bin while missing Homebrew.""" @@ -650,6 +658,176 @@ def test_make_run_env_injects_hermes_bin_dir(self, monkeypatch): assert "/usr/bin" in entries +class TestPythonBinDirOnPath: + """The active Python's bin dir anchors the Hermes venv on PATH. + + Replaces the VIRTUAL_ENV-marker prelude that the original #66642 fix + relied on. The prelude failed because (a) ``_prepend_shell_init`` is + only called when ``init_files`` is non-empty, and (b) ``_make_run_env`` + intentionally strips VIRTUAL_ENV/CONDA_PREFIX before ``Popen`` to + prevent cross-project clobber (#23473, dbbf102b8). The shell condition + was therefore always false on the terminal path. + + The new contract uses ``sys.executable``'s parent dir as the trusted + anchor — the running interpreter is by definition inside the Hermes + venv, so its bin is exactly the dir that needs to stay on PATH. + """ + + def _reset_cache(self): + from tools.environments import local as local_mod + local_mod._PYTHON_BIN_DIR_CACHE = local_mod._SENTINEL + + def test_resolves_via_sys_executable(self, monkeypatch, tmp_path): + from tools.environments import local as local_mod + self._reset_cache() + venv_bin = tmp_path / "venv" / "bin" + venv_bin.mkdir(parents=True) + (venv_bin / "python").write_text("#!/bin/sh\n") + monkeypatch.setattr(local_mod.sys, "executable", str(venv_bin / "python")) + assert local_mod._resolve_python_bin_dir() == str(venv_bin) + + def test_returns_none_when_executable_unset(self, monkeypatch): + from tools.environments import local as local_mod + self._reset_cache() + monkeypatch.setattr(local_mod.sys, "executable", "") + assert local_mod._resolve_python_bin_dir() is None + + def test_returns_none_when_parent_not_a_dir(self, monkeypatch): + from tools.environments import local as local_mod + self._reset_cache() + # Use a path whose parent does not exist on disk. + monkeypatch.setattr( + local_mod.sys, "executable", "/nonexistent/parent/python" + ) + assert local_mod._resolve_python_bin_dir() is None + + def test_prepend_adds_missing_dir_at_front(self, monkeypatch): + from tools.environments import local as local_mod + self._reset_cache() + local_mod._PYTHON_BIN_DIR_CACHE = "/opt/hermes/venv/bin" + # The helper is platform-agnostic — it uses os.pathsep, which on + # Windows is ``;`` and on POSIX is ``:``. Simulate the host's + # separator so the test runs cross-platform. + sep = os.pathsep + original_path = sep.join(["/usr/bin", "/bin"]) + out = local_mod._prepend_python_bin_dir(original_path) + entries = out.split(sep) + assert entries[0] == "/opt/hermes/venv/bin" + assert "/usr/bin" in entries + + def test_prepend_is_idempotent(self, monkeypatch): + from tools.environments import local as local_mod + self._reset_cache() + local_mod._PYTHON_BIN_DIR_CACHE = "/opt/hermes/venv/bin" + sep = os.pathsep + original_path = sep.join(["/usr/bin", "/bin"]) + once = local_mod._prepend_python_bin_dir(original_path) + twice = local_mod._prepend_python_bin_dir(once) + assert twice == once + assert once.split(sep).count("/opt/hermes/venv/bin") == 1 + + def test_prepend_noop_when_unresolved(self, monkeypatch): + from tools.environments import local as local_mod + self._reset_cache() + local_mod._PYTHON_BIN_DIR_CACHE = None + sep = os.pathsep + original_path = sep.join(["/usr/bin", "/bin"]) + assert local_mod._prepend_python_bin_dir(original_path) == original_path + + def test_make_run_env_injects_python_bin_dir(self, monkeypatch): + """The active Python's bin is anchored on the terminal subshell PATH. + + Reproduces the original #66642 repro on bare containers: PATH + collapses to ``/usr/local/bin:/usr/bin:/bin`` after a login shell + with empty profile files, dropping the Hermes venv. The fix must + re-anchor ``python`` to the venv interpreter regardless of the + caller's PATH or whether VIRTUAL_ENV is set. + + Verifies the cross-platform contract: the venv bin dir must be + present in the resulting PATH and must outrank every entry the + caller originally supplied. The exact position depends on the + host's ``os.pathsep`` and the surrounding helpers — see + ``test_prepend_adds_missing_dir_at_front`` for the ordering unit. + """ + from tools.environments import local as local_mod + from tools.environments.local import _make_run_env + self._reset_cache() + local_mod._PYTHON_BIN_DIR_CACHE = "/opt/hermes/venv/bin" + monkeypatch.setattr(local_mod, "_IS_WINDOWS", False) + sep = os.pathsep + with patch.dict( + os.environ, + {"PATH": sep.join(["/usr/local/bin", "/usr/bin", "/bin"])}, + clear=True, + ): + result = _make_run_env({}) + # The venv bin must appear before any of the caller's PATH + # entries — that is the whole point of the prepend. Find its + # position and assert it outranks the caller's bin dirs. + entries = result["PATH"].split(sep) + venv_idx = entries.index("/opt/hermes/venv/bin") + assert venv_idx <= entries.index("/usr/local/bin") + assert "/usr/local/bin" in entries + assert "/usr/bin" in entries + + def test_make_run_env_does_not_require_virtual_env_marker(self, monkeypatch): + """The fix must work even when VIRTUAL_ENV is stripped from the env. + + ``_make_run_env`` removes VIRTUAL_ENV/CONDA_PREFIX to prevent + cross-project clobber (#23473). The original #66642 prelude was + therefore dead code on the terminal path; this test fails any + fix that re-introduces that dependency. + """ + from tools.environments import local as local_mod + from tools.environments.local import _make_run_env + self._reset_cache() + local_mod._PYTHON_BIN_DIR_CACHE = "/opt/hermes/venv/bin" + monkeypatch.setattr(local_mod, "_IS_WINDOWS", False) + sep = os.pathsep + # No VIRTUAL_ENV in os.environ — the fix must still inject the + # venv bin because it derives from sys.executable, not from env. + with patch.dict( + os.environ, + {"PATH": sep.join(["/usr/local/bin", "/usr/bin", "/bin"])}, + clear=True, + ): + result = _make_run_env({}) + entries = result["PATH"].split(sep) + assert "/opt/hermes/venv/bin" in entries + + def test_make_run_env_works_with_empty_init_files(self, monkeypatch): + """The fix must NOT depend on init_files being non-empty. + + The original #66642 prelude lived inside ``_prepend_shell_init``, + which is only called when ``init_files`` is non-empty + (``tools/environments/local.py:1360-1363``). On bare containers + with no shell rc files, the prelude never ran — the bug it was + supposed to fix was unreachable through its own code path. + + The new contract lives in ``_make_run_env`` and is unconditional. + """ + from tools.environments import local as local_mod + from tools.environments.local import _make_run_env + self._reset_cache() + local_mod._PYTHON_BIN_DIR_CACHE = "/opt/hermes/venv/bin" + monkeypatch.setattr(local_mod, "_IS_WINDOWS", False) + sep = os.pathsep + # Force the empty-init-files path explicitly. + monkeypatch.setattr(local_mod, "_resolve_shell_init_files", lambda: []) + with patch.dict( + os.environ, + {"PATH": sep.join(["/usr/local/bin", "/usr/bin", "/bin"])}, + clear=True, + ): + result = _make_run_env({}) + entries = result["PATH"].split(sep) + # Even with init_files=[] (the original bug's trigger), the + # venv bin is on PATH — the fix lives in _make_run_env, not in + # _prepend_shell_init. + venv_idx = entries.index("/opt/hermes/venv/bin") + assert venv_idx <= entries.index("/usr/local/bin") + + class TestHermesInternalDynamicSecrets: """Dynamically-named Hermes secrets injected at gateway/CLI startup must not leak into terminal subprocesses. diff --git a/tools/environments/local.py b/tools/environments/local.py index 8b4450c72010..078a23c17b0a 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -1036,6 +1036,69 @@ def _prepend_hermes_bin_dir(existing_path: str) -> str: return sep.join([bin_dir, *entries]) +# Cached bin dir of the running interpreter (``sys.executable``'s parent). +# Lets the login-shell snapshot keep the Hermes venv on PATH even when the +# user's profile is empty and the active-venv markers are scrubbed. +# Distinct from ``_HERMES_BIN_DIR`` (which locates the ``hermes`` console +# script, possibly via a PATH shim rather than the venv). +_PYTHON_BIN_DIR_CACHE: "str | None | object" = _SENTINEL + + +def _resolve_python_bin_dir() -> str | None: + """Return the directory holding ``sys.executable`` — the active venv's bin. + + This is the directory where the running Python interpreter's ``pip``, + ``python``, and any console-scripts the venv installed live. On a uv / + virtualenv layout it is ``/bin`` (POSIX) or ``/Scripts`` + (Windows). On a system install it is e.g. ``/usr/bin`` — in which case + the path is already on PATH and prepending it is harmless. + + Used as a trusted, Python-derived anchor for PATH when the login-shell + snapshot would otherwise lose the active venv. Cannot rely on + ``$VIRTUAL_ENV`` because ``_make_run_env`` strips it before ``Popen`` to + prevent cross-project clobber (commit ``dbbf102b8``, + ``tests/tools/test_local_env_blocklist.py:277-303``). + """ + global _PYTHON_BIN_DIR_CACHE + if _PYTHON_BIN_DIR_CACHE is not _SENTINEL: + return _PYTHON_BIN_DIR_CACHE # type: ignore[return-value] + + candidate: str | None = None + exe = sys.executable + if exe: + parent = os.path.dirname(os.path.abspath(exe)) + # ``sys.executable`` is the venv's python on every supported layout. + # The parent dir always exists; verify it is non-empty so a bogus + # config (empty executable path) does not poison the cache. + if parent and os.path.isdir(parent): + candidate = parent + + _PYTHON_BIN_DIR_CACHE = candidate + return candidate + + +def _prepend_python_bin_dir(existing_path: str) -> str: + """Prepend the active Python bin dir to ``existing_path`` if it's missing. + + Counterpart to ``_prepend_hermes_bin_dir`` for the interpreter's bin: + ensures the Hermes venv stays reachable on PATH even when the login + shell's profile sourcing (or lack thereof) would reset PATH to bash's + compiled default (``/usr/local/bin:/usr/bin:/bin``), silently dropping + ``python`` and every console-script in the active venv (#66642). + + Cross-platform (uses ``os.pathsep``). First-occurrence wins; no-op when + the dir can't be resolved. + """ + bin_dir = _resolve_python_bin_dir() + if not bin_dir: + return existing_path + sep = os.pathsep + entries = [e for e in existing_path.split(sep) if e] if existing_path else [] + if bin_dir in entries: + return existing_path + return sep.join([bin_dir, *entries]) + + def _append_missing_sane_path_entries(existing_path: str) -> str: """Return a normalised POSIX PATH with missing sane entries appended. @@ -1147,6 +1210,15 @@ def _make_run_env(env: dict) -> dict: path_key = _path_env_key(run_env) if path_key is not None: new_path = _append_missing_sane_path_entries(run_env.get(path_key, "")) + # Anchor the Hermes venv on PATH before any of the platform-specific + # fallbacks. Without this, a bare-container login shell whose + # ``/etc/profile`` is empty resets PATH to bash's compiled default + # (``/usr/local/bin:/usr/bin:/bin``) and silently drops the active + # venv — ``python`` then resolves to the system interpreter, not the + # Hermes one (#66642). We derive the bin dir from ``sys.executable`` + # rather than the ``$VIRTUAL_ENV`` env var because that marker is + # scrubbed further down to prevent cross-project clobber. + new_path = _prepend_python_bin_dir(new_path) # On Windows, ensure Git Bash's coreutils dirs (…\usr\bin etc.) are on # PATH. A non-login ``bash -c`` fallback (used when ``bash -l`` is # broken) never sources /etc/profile, so without this cat/mktemp/mv and