diff --git a/tests/tools/test_local_env_blocklist.py b/tests/tools/test_local_env_blocklist.py index 7d7545fa8c5..875b8a15ccb 100644 --- a/tests/tools/test_local_env_blocklist.py +++ b/tests/tools/test_local_env_blocklist.py @@ -388,20 +388,86 @@ class TestSanePathIncludesHomebrew: assert "/opt/homebrew/sbin" in _SANE_PATH def test_make_run_env_appends_homebrew_on_minimal_path(self): - """When PATH is minimal (no /usr/bin), _make_run_env should append - _SANE_PATH which now includes Homebrew dirs.""" - from tools.environments.local import _make_run_env + """When PATH is minimal, _make_run_env appends missing sane entries.""" + 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({}) - assert "/opt/homebrew/bin" in result["PATH"] - assert "/opt/homebrew/sbin" in result["PATH"] + path_entries = result["PATH"].split(":") + assert path_entries[0] == "/some/custom/bin" + for entry in _SANE_PATH.split(":"): + assert entry in path_entries - def test_make_run_env_does_not_duplicate_on_full_path(self): - """When PATH already has /usr/bin, _make_run_env should not append.""" + def test_make_run_env_fills_missing_homebrew_when_usr_bin_present(self): + """macOS launchd PATH can include /usr/bin while missing Homebrew.""" from tools.environments.local import _make_run_env - full_env = {"PATH": "/usr/bin:/bin"} - with patch.dict(os.environ, full_env, clear=True): + launchd_env = {"PATH": "/usr/local/bin:/usr/bin:/bin"} + with patch.dict(os.environ, launchd_env, clear=True): result = _make_run_env({}) - # Should keep existing PATH unchanged - assert result["PATH"] == "/usr/bin:/bin" + path_entries = result["PATH"].split(":") + assert "/opt/homebrew/bin" in path_entries + assert "/opt/homebrew/sbin" in path_entries + + def test_make_run_env_does_not_duplicate_existing_sane_entries(self): + from tools.environments.local import _make_run_env + existing_env = {"PATH": "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin"} + with patch.dict(os.environ, existing_env, clear=True): + result = _make_run_env({}) + path_entries = result["PATH"].split(":") + assert path_entries.count("/opt/homebrew/bin") == 1 + assert path_entries.count("/usr/local/bin") == 1 + assert path_entries.count("/usr/bin") == 1 + + def test_make_run_env_real_launchd_path_gains_homebrew(self): + """The literal macOS launchd PATH is the production trigger for #35613.""" + from tools.environments.local import _make_run_env + launchd_env = {"PATH": "/usr/bin:/bin:/usr/sbin:/sbin"} + with patch.dict(os.environ, launchd_env, clear=True): + result = _make_run_env({}) + path_entries = result["PATH"].split(":") + assert "/opt/homebrew/bin" in path_entries + assert "/opt/homebrew/sbin" in path_entries + # Original entries keep their leading precedence. + assert path_entries[:4] == ["/usr/bin", "/bin", "/usr/sbin", "/sbin"] + + def test_make_run_env_collapses_duplicate_caller_entries(self): + """Duplicates already present in the caller PATH are de-duplicated.""" + from tools.environments.local import _make_run_env + dup_env = {"PATH": "/usr/bin:/usr/bin:/custom/bin:/custom/bin:/bin"} + with patch.dict(os.environ, dup_env, clear=True): + result = _make_run_env({}) + path_entries = result["PATH"].split(":") + assert path_entries.count("/usr/bin") == 1 + assert path_entries.count("/custom/bin") == 1 + # First-occurrence order is preserved for the caller entries. + assert path_entries[:3] == ["/usr/bin", "/custom/bin", "/bin"] + + def test_make_run_env_strips_empty_path_entries(self): + """Leading/trailing/double colons (== CWD on POSIX) are dropped.""" + from tools.environments.local import _make_run_env + empty_env = {"PATH": "/usr/bin::/bin:"} + with patch.dict(os.environ, empty_env, clear=True): + result = _make_run_env({}) + path_entries = result["PATH"].split(":") + assert "" not in path_entries + assert "/usr/bin" in path_entries + assert "/opt/homebrew/bin" in path_entries + + def test_make_run_env_leaves_windows_path_unchanged(self, monkeypatch): + from tools.environments import local as local_mod + from tools.environments.local import _make_run_env + windows_env = {"PATH": r"C:\Windows\System32;C:\Program Files\Git\bin"} + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + with patch.dict(os.environ, windows_env, clear=True): + result = _make_run_env({}) + assert result["PATH"] == windows_env["PATH"] + + def test_make_run_env_preserves_windows_mixed_case_path_key(self, monkeypatch): + from tools.environments import local as local_mod + from tools.environments.local import _make_run_env + windows_env = {"Path": r"C:\Windows\System32;C:\Program Files\Git\bin"} + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + with patch.object(local_mod.os, "environ", windows_env): + result = _make_run_env({}) + assert result["Path"] == windows_env["Path"] + assert "PATH" not in result diff --git a/tests/tools/test_windows_native_support.py b/tests/tools/test_windows_native_support.py index 661fefb6555..3abf5bf80f2 100644 --- a/tests/tools/test_windows_native_support.py +++ b/tests/tools/test_windows_native_support.py @@ -841,13 +841,15 @@ class TestLocalEnvironmentWindowsTempDir: class TestLocalEnvironmentPathInjectionGated: - """The /usr/bin PATH injection in _make_run_env must be POSIX-only.""" + """Sane PATH completion must stay POSIX-only.""" - def test_source_gates_path_injection(self): - root = Path(__file__).resolve().parents[2] - source = (root / "tools" / "environments" / "local.py").read_text(encoding="utf-8") - # The fix wraps the injection in `if not _IS_WINDOWS`. - assert 'not _IS_WINDOWS and "/usr/bin" not in existing_path.split(":")' in source + def test_windows_path_is_left_unchanged(self, monkeypatch): + from tools.environments import local as local_mod + from tools.environments.local import _append_missing_sane_path_entries + + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + path = r"C:\Windows\System32;C:\Program Files\Git\bin" + assert _append_missing_sane_path_entries(path) == path # --------------------------------------------------------------------------- diff --git a/tools/environments/local.py b/tools/environments/local.py index 68ff2577473..32c6897fe5f 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -300,6 +300,72 @@ _SANE_PATH = ( ) +def _append_missing_sane_path_entries(existing_path: str) -> str: + """Return a normalised POSIX PATH with missing sane entries appended. + + On POSIX the caller-supplied PATH is rewritten (not merely appended to): + empty entries and duplicate entries are dropped, preserving + first-occurrence order, then each missing ``_SANE_PATH`` entry is appended + once at the end so existing entries keep their precedence. + + Two intentional normalisations beyond the bare "add Homebrew dirs" fix: + + - **Empty entries are stripped.** A leading/trailing/double ``:`` encodes + an empty PATH element, which POSIX shells interpret as the current + working directory — a mild foot-gun in a default terminal environment. + We drop these rather than carry them through. + - **Duplicates are collapsed** (first occurrence wins), so a caller PATH + that already contains repeats is not propagated verbatim. + + For a well-formed PATH (no empties, no duplicates) the leading segment is + byte-identical to the input and ordering is preserved; only the missing + sane entries are appended. On Windows this is a no-op passthrough (the + separator is ``;`` and the native PATH must not be touched). + """ + if _IS_WINDOWS: + return existing_path + + sane_entries = [entry for entry in _SANE_PATH.split(":") if entry] + if not existing_path: + return ":".join(sane_entries) + + # De-duplicate the caller PATH (first occurrence wins) and drop empty + # entries before merging in the sane fallbacks. + seen: set[str] = set() + ordered_entries: list[str] = [] + for entry in existing_path.split(":"): + if not entry or entry in seen: + continue + seen.add(entry) + ordered_entries.append(entry) + + # _SANE_PATH is a static, duplicate-free constant, so a membership check + # against the caller entries is sufficient — no need to track `seen` here. + for entry in sane_entries: + if entry not in seen: + ordered_entries.append(entry) + + return ":".join(ordered_entries) + + +def _path_env_key(run_env: dict) -> str | None: + """Return the PATH env key to update without altering Windows casing. + + Note: this is deliberately a *second* Windows guard, distinct from the + early-return in ``_append_missing_sane_path_entries``. Its job is to pick + the correctly-cased key (``Path`` vs ``PATH``) so completion writes back to + the key the caller already used; the helper's guard makes that helper safe + to call standalone (it is, e.g. in the Windows unit tests). Both are + intentional. + """ + if not _IS_WINDOWS: + return "PATH" + for key in run_env: + if key.upper() == "PATH": + return key + return None + + def _make_run_env(env: dict) -> dict: """Build a run environment with a sane PATH and provider-var stripping.""" try: @@ -315,17 +381,9 @@ def _make_run_env(env: dict) -> dict: run_env[real_key] = v elif k not in _HERMES_PROVIDER_ENV_BLOCKLIST or _is_passthrough(k): run_env[k] = v - existing_path = run_env.get("PATH", "") - # The "/usr/bin not already present → inject sane POSIX path" heuristic - # only makes sense on POSIX. On Windows the PATH separator is ";" - # (the split(":") above turns a full Windows PATH into a single - # unrecognisable chunk, which then triggers prepending POSIX paths - # to a Windows PATH — completely wrong). Skip the injection entirely - # on Windows; the native PATH already points at whatever shell - # Hermes is driving via _find_bash (Git Bash), and Git Bash itself - # prepends its MSYS2 /usr/bin equivalent via the shell-init files. - if not _IS_WINDOWS and "/usr/bin" not in existing_path.split(":"): - run_env["PATH"] = f"{existing_path}:{_SANE_PATH}" if existing_path else _SANE_PATH + path_key = _path_env_key(run_env) + if path_key is not None: + run_env[path_key] = _append_missing_sane_path_entries(run_env.get(path_key, "")) _inject_context_hermes_home(run_env)