fix(terminal): complete sane PATH entries on POSIX - #35614
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Reviewed Changes
- tools/environments/local.py — Refactored PATH injection into
_append_missing_sane_path_entries()and_path_env_key()helpers. PATH completion now: (1) skips Windows entirely, (2) checks each POSIX sane entry individually rather than using a/usr/binpresence heuristic, (3) deduplicates entries, (4) handles WindowsPathkey casing. - tests/tools/test_local_env_blocklist.py — Expanded tests: macOS launchd minimal PATH, Homebrew completion, no duplication, Windows PATH unchanged, Windows mixed-case Path key.
- tests/tools/test_windows_native_support.py — Replaced source-string assertion with actual runtime test.
✅ Looks Good
- Correctness: The old code used
/usr/bin not in existing_path.split(":")which would match a/usr/binanywhere in the path — but then appended the entire_SANE_PATH. Worse, on Windows with;separator, the split produced one huge chunk which would always match the heuristic and skip injection entirely (wrong behavior in both directions). The new code checks each sane entry individually and appends only what's missing. Windows is completely skipped. - Edge cases: Empty PATH, mixed-case Windows Path key, macOS Homebrew paths, duplicate entry prevention — all handled.
- Test coverage: The old source-string assertion (
'not _IS_WINDOWS and "/usr/bin" not in existing_path.split(":")' in source) was brittle and tested the implementation detail, not the behavior. The new tests actually exercise the function.
Reviewed by Hermes Agent
|
Adding a concrete real-world repro to help push this over the line. Reproduction: Hermes Desktop (.app) on macOS Launching Hermes from the macOS Desktop app ( With the unfixed guard, This also breaks Verified fix: the approach in this PR (append missing entries per-item rather than all-or-nothing) resolves it completely. Applied the same logic locally, 7/7 regression tests pass including the new Merging this would fix the Desktop app for all macOS users. The PR is already approved — flagging in case it just needs a final review pass to land. |
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 NousResearch#35614 by @y0shua1ee. Fixes NousResearch#35613. Co-authored-by: y0shua1ee <104712437+y0shua1ee@users.noreply.github.com>
#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>
|
Thanks for this @y0shua1ee — the root-cause analysis was spot on. The This landed via #42653 (merged just now): your fix is the first commit, cherry-picked unchanged and credited via Closing in favour of #42653. Appreciate the clean diagnosis and tests! |
…search#35614) (NousResearch#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 NousResearch#35614 by @y0shua1ee. Fixes NousResearch#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 NousResearch#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>
#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>
…search#35614) (NousResearch#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 NousResearch#35614 by @y0shua1ee. Fixes NousResearch#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 NousResearch#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>
…search#35614) (NousResearch#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 NousResearch#35614 by @y0shua1ee. Fixes NousResearch#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 NousResearch#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>
…search#35614) (NousResearch#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 NousResearch#35614 by @y0shua1ee. Fixes NousResearch#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 NousResearch#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>
…search#35614) (NousResearch#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 NousResearch#35614 by @y0shua1ee. Fixes NousResearch#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 NousResearch#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>
…search#35614) (NousResearch#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 NousResearch#35614 by @y0shua1ee. Fixes NousResearch#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 NousResearch#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>
…search#35614) (NousResearch#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 NousResearch#35614 by @y0shua1ee. Fixes NousResearch#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 NousResearch#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>
…search#35614) (NousResearch#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 NousResearch#35614 by @y0shua1ee. Fixes NousResearch#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 NousResearch#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>
…search#35614) (NousResearch#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 NousResearch#35614 by @y0shua1ee. Fixes NousResearch#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 NousResearch#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>
What does this PR do?
Fixes macOS gateway/launchd terminal sessions whose
PATHalready includes/usr/binwhile omitting Apple Silicon Homebrew paths.LocalEnvironment._make_run_env()now appends each missing_SANE_PATHentry individually on POSIX, preserving caller precedence and avoiding duplicate sane entries.Root cause: the previous logic used
/usr/binas the sentinel for sanePATHinjection. macOS launchd commonly provides/usr/binwhile leaving out/opt/homebrew/binand/opt/homebrew/sbin, so Homebrew-installed CLIs stayed unavailable in terminal tool calls.User-visible impact: terminal sessions launched from the gateway can resolve Homebrew CLIs such as
gh,brew, andxurlthrough the default environment fallback, reducing absolute-path workarounds on macOS.Related Issue
Fixes #35613
Type of Change
Changes Made
tools/environments/local.py_append_missing_sane_path_entries()to append missing_SANE_PATHentries per item on POSIX._path_env_key()so Windows keeps the existingPATH/Pathkey casing._make_run_env()to use the helper instead of the/usr/binsentinel.tests/tools/test_local_env_blocklist.pyPATH=/usr/local/bin:/usr/bin:/binmissing Homebrew entries.Pathcasing preservation.tests/tools/test_windows_native_support.pyHow to Test
Reproduce the old failure with the new regression test against the previous implementation:
PYTHONPATH=. python -m pytest \ tests/tools/test_local_env_blocklist.py::TestSanePathIncludesHomebrew::test_make_run_env_fills_missing_homebrew_when_usr_bin_present \ -o 'addopts=' -qOld behavior failed with:
Run the targeted cross-platform coverage:
PYTHONPATH=. python -m pytest \ tests/tools/test_local_env_blocklist.py \ tests/tools/test_env_passthrough.py \ tests/test_subprocess_home_isolation.py \ tests/tools/test_windows_native_support.py::TestLocalEnvironmentPathInjectionGated \ tests/tools/test_local_env_windows_msys.py \ -o 'addopts=' -qResult:
69 passed.Run formatting/lint checks:
Result:
All checks passed!.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs