From a38cc69bcc23a37ca957528701cba328f677e525 Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Tue, 9 Jun 2026 02:21:12 -0700 Subject: [PATCH] fix(terminal): complete sane PATH entries on POSIX (salvage of #35614) (#42653) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminal): complete sane PATH entries on POSIX Fixes macOS gateway/launchd terminal sessions whose PATH already includes /usr/bin while omitting Apple Silicon Homebrew paths. LocalEnvironment._make_run_env() now appends each missing _SANE_PATH entry individually on POSIX, preserving caller precedence and avoiding duplicate sane entries. Root cause: the previous logic used /usr/bin as the sentinel for sane PATH injection. macOS launchd commonly provides /usr/bin while leaving out /opt/homebrew/bin and /opt/homebrew/sbin, so Homebrew-installed CLIs stayed unavailable in terminal tool calls. Salvaged from #35614 by @y0shua1ee. Fixes #35613. Co-authored-by: y0shua1ee <104712437+y0shua1ee@users.noreply.github.com> * test(terminal): harden sane PATH completion against dup/empty entries Follow-up to the #35613 fix. Strengthens _append_missing_sane_path_entries: - De-duplicate the caller-supplied PATH (first occurrence wins) so a PATH that already contains duplicate entries is collapsed rather than carried through. Previously only newly-appended sane entries were guarded against duplication; pre-existing caller duplicates were preserved verbatim. - Drop empty PATH entries (leading/trailing/double ':'), which POSIX shells interpret as the current working directory — a mild foot-gun in a default terminal environment. Behaviour for well-formed PATHs (no duplicates, no empty entries) is byte-identical to before; only malformed/duplicated inputs change. Adds regression tests for: the literal macOS launchd PATH (/usr/bin:/bin:/usr/sbin:/sbin), caller-duplicate collapsing with order preservation, and empty-entry stripping. * docs(terminal): clarify PATH normalisation semantics; drop dead set add Addresses review findings on the sane-PATH completion follow-up: - Sharpen the _append_missing_sane_path_entries docstring to state explicitly that on POSIX the caller PATH is rewritten (empty entries stripped, duplicates collapsed) rather than merely appended to, and that well-formed PATHs remain byte-identical bar the appended sane entries. This makes the intentional semantic change visible rather than buried under "hardening". - Document why _path_env_key is a deliberate second Windows guard distinct from the helper's early return (key-casing selection vs standalone safety), so neither is mistaken for redundant and removed. - Drop the dead `seen.add(entry)` in the sane-entry loop: _SANE_PATH is a static duplicate-free constant, so the membership check against the caller entries is sufficient and `seen` is never read afterwards. No behaviour change: verified byte-identical output across the launchd, minimal, empty, duplicate, empty-entry and already-full cases, and re-confirmed gh/brew resolve through the real LocalEnvironment.execute() path under a launchd-style PATH. 133 targeted tests pass. Intentionally NOT consolidating with tools/browser_tool._merge_browser_path: it prepends (vs append), filters on os.path.isdir, uses os.pathsep, and draws from a dynamic candidate set — a shared helper is a separate refactor, out of scope for this bugfix. --------- Co-authored-by: y0shua1ee <104712437+y0shua1ee@users.noreply.github.com> --- tests/tools/test_local_env_blocklist.py | 88 +++++++++++++++++++--- tests/tools/test_windows_native_support.py | 14 ++-- tools/environments/local.py | 80 +++++++++++++++++--- 3 files changed, 154 insertions(+), 28 deletions(-) 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)