fix(terminal): complete sane PATH entries on POSIX (salvage of #35614) (#42653)

* 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

@@ -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

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
 
 
 # ---------------------------------------------------------------------------

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)