Merge pull request #53110 from NousResearch/salvage/42203-find-shell-prefer-usershell

fix(terminal): prefer $SHELL over bash for background process spawning (#42203)

tests/tools/test_find_shell.py

@@ -0,0 +1,188 @@
+"""Tests for _find_shell — user-login-shell preference on POSIX.
+
+Regression tests for #42203: on macOS, ``_find_shell`` used to return
+``/bin/bash`` (bash 3.2) which silently swallowed background commands
+when ``~/.bash_profile`` contained ``exec /bin/zsh -l``.
+"""
+
+import os
+import platform
+import subprocess
+import sys
+from unittest.mock import patch
+
+import pytest
+
+from tools.environments.local import _find_bash, _find_shell
+
+
+class TestFindShellPrefersUserShell:
+    """_find_shell should prefer $SHELL over bash on POSIX."""
+
+    def test_returns_shell_env_when_set_and_exists(self, tmp_path):
+        """When $SHELL points to an existing allowlisted executable, _find_shell returns it."""
+        fake_zsh = tmp_path / "zsh"
+        fake_zsh.touch()
+        fake_zsh.chmod(0o755)
+        with patch.dict(os.environ, {"SHELL": str(fake_zsh)}):
+            assert _find_shell() == str(fake_zsh)
+
+    def test_falls_back_when_shell_not_executable(self, tmp_path):
+        """$SHELL exists but lacks the execute bit -> fall back to _find_bash
+        (returning it would fail at spawn time)."""
+        fake = tmp_path / "zsh"
+        fake.touch()
+        fake.chmod(0o644)  # not executable
+        with patch.dict(os.environ, {"SHELL": str(fake)}):
+            assert _find_shell() == _find_bash()
+
+    def test_falls_back_for_incompatible_shell_fish(self, tmp_path):
+        """#42203 regression: $SHELL=fish must NOT be returned — spawn_local's
+        `-lic` / `set +m` syntax breaks fish, which would trade the bash-3.2
+        swallow for a parse error on every background command. Fall back to bash."""
+        fake_fish = tmp_path / "fish"
+        fake_fish.touch()
+        fake_fish.chmod(0o755)
+        with patch.dict(os.environ, {"SHELL": str(fake_fish)}):
+            assert _find_shell() == _find_bash()
+
+    def test_falls_back_for_incompatible_shell_csh(self, tmp_path):
+        """$SHELL=tcsh/csh is also not -lic/set+m compatible -> fall back."""
+        fake = tmp_path / "tcsh"
+        fake.touch()
+        fake.chmod(0o755)
+        with patch.dict(os.environ, {"SHELL": str(fake)}):
+            assert _find_shell() == _find_bash()
+
+    def test_honours_allowlisted_bash_and_dash(self, tmp_path):
+        """Every allowlisted POSIX-sh-family shell is honoured."""
+        for name in ("bash", "dash", "sh", "ksh"):
+            fake = tmp_path / name
+            fake.touch()
+            fake.chmod(0o755)
+            with patch.dict(os.environ, {"SHELL": str(fake)}):
+                assert _find_shell() == str(fake), name
+
+    def test_falls_back_to_find_bash_when_shell_unset(self):
+        """When $SHELL is unset, _find_shell delegates to _find_bash."""
+        env = {k: v for k, v in os.environ.items() if k != "SHELL"}
+        with patch.dict(os.environ, env, clear=True):
+            assert _find_shell() == _find_bash()
+
+    def test_falls_back_to_find_bash_when_shell_not_a_file(self, tmp_path):
+        """When $SHELL points to a non-existent path, _find_shell delegates."""
+        fake_path = str(tmp_path / "nonexistent_shell")
+        with patch.dict(os.environ, {"SHELL": fake_path}):
+            assert _find_shell() == _find_bash()
+
+    def test_falls_back_to_find_bash_when_shell_empty(self):
+        """When $SHELL is empty string, _find_shell delegates."""
+        with patch.dict(os.environ, {"SHELL": ""}):
+            assert _find_shell() == _find_bash()
+
+
+class TestFindShellWindowsBehavior:
+    """On Windows, _find_shell always delegates to _find_bash."""
+
+    def test_windows_ignores_shell_env(self):
+        """On Windows, $SHELL is ignored — _find_shell delegates to _find_bash."""
+        with patch("tools.environments.local._IS_WINDOWS", True):
+            # Even if SHELL is set, it should be ignored on Windows
+            with patch.dict(os.environ, {"SHELL": "/usr/bin/zsh"}):
+                result = _find_shell()
+                assert result == _find_bash()
+
+
+class TestFindShellReturnsString:
+    """_find_shell must return a string, never None."""
+
+    def test_returns_string(self):
+        """_find_shell always returns a non-empty string on any platform."""
+        result = _find_shell()
+        assert isinstance(result, str)
+        assert len(result) > 0
+
+
+class TestFindBashUnchanged:
+    """_find_bash should be unaffected by the _find_shell change."""
+
+    def test_find_bash_still_prefers_bash(self):
+        """_find_bash still returns bash (not $SHELL) on POSIX."""
+        result = _find_bash()
+        # On any system, _find_bash should return something containing "bash"
+        # or fall back to $SHELL or /bin/sh — but it should NOT prefer $SHELL
+        # over bash the way _find_shell does.
+        assert isinstance(result, str)
+        assert len(result) > 0
+
+
+@pytest.mark.skipif(
+    not os.path.isfile("/bin/bash") or sys.platform != "darwin",
+    reason="reproduces the macOS system-bash-3.2 login-shell swallow",
+)
+class TestMacosLoginShellSwallowRegression:
+    """E2E regression for #42203: the actual failure is that system bash 3.2,
+    invoked as a login shell (`-lic`) with stdin=/dev/null and a
+    ~/.bash_profile that `exec`s zsh, silently swallows the command (exit 0,
+    no output, no side effects). Prove (a) the bug exists with /bin/bash and
+    (b) the $SHELL (zsh) path _find_shell prefers does NOT swallow."""
+
+    def _spawn_like_registry(self, shell, command, home, tmp_path):
+        import subprocess
+        env = dict(os.environ)
+        env["HOME"] = str(home)
+        # Mirror process_registry.spawn_local: [shell, "-lic", "set +m; <cmd>"]
+        # with stdin redirected to /dev/null.
+        return subprocess.run(
+            [shell, "-lic", f"set +m; {command}"],
+            stdin=subprocess.DEVNULL,
+            capture_output=True,
+            text=True,
+            env=env,
+        )
+
+    def test_system_bash_swallows_but_zsh_does_not(self, tmp_path):
+        # A .bash_profile that exec's zsh — the reported macOS shape.
+        home = tmp_path / "home"
+        home.mkdir()
+        (home / ".bash_profile").write_text("exec /bin/zsh -l\n")
+
+        zsh = os.environ.get("SHELL") or "/bin/zsh"
+        if not os.path.isfile(zsh):
+            pytest.skip("no zsh available")
+
+        marker_bash = tmp_path / "bash_ran"
+        marker_zsh = tmp_path / "zsh_ran"
+
+        # /bin/bash login shell: command is swallowed (file NOT created).
+        self._spawn_like_registry("/bin/bash", f"echo x > {marker_bash}", home, tmp_path)
+        # zsh (the $SHELL _find_shell prefers): command runs (file created).
+        self._spawn_like_registry(zsh, f"echo x > {marker_zsh}", home, tmp_path)
+
+        # The FIX path (zsh) must run the command.
+        assert marker_zsh.exists(), "zsh ($SHELL) path must run the command"
+
+        # Differential: when /bin/bash is the swallow-prone 3.x (macOS system
+        # bash), the login-shell invocation must demonstrably FAIL to run the
+        # command — that's the bug this PR routes around. Only assert the
+        # negative when we've confirmed a 3.x bash, so the test stays valid on
+        # boxes/CI with a newer /bin/bash that doesn't swallow.
+        ver = subprocess.run(
+            ["/bin/bash", "--version"], capture_output=True, text=True
+        ).stdout
+        if "version 3." in ver:
+            assert not marker_bash.exists(), (
+                "system bash 3.x login shell should swallow the command "
+                "(the #42203 bug); _find_shell routes around it by preferring zsh"
+            )
+
+    def test_find_shell_selects_working_shell_on_this_box(self, tmp_path):
+        """_find_shell's choice must actually execute a background-style
+        command (regression against returning a swallow-prone shell)."""
+        shell = _find_shell()
+        marker = tmp_path / "ok_marker"
+        subprocess.run(
+            [shell, "-lic", f"set +m; echo ok > {marker}"],
+            stdin=subprocess.DEVNULL, capture_output=True, text=True,
+        )
+        assert marker.exists(), f"_find_shell()={shell} swallowed the command"

tools/environments/local.py

@@ -288,8 +288,53 @@ def _find_bash() -> str:
     )
 
 
-# Backward compat — process_registry.py imports this name
-_find_shell = _find_bash
+# POSIX-sh-family shells that understand the ``[shell, "-lic", "set +m; …"]``
+# invocation spawn_local uses. $SHELL values outside this set (fish, csh/tcsh,
+# nushell, elvish, xonsh, …) would error on that syntax, so _find_shell falls
+# back to bash for them rather than honouring $SHELL. (#42203)
+_SPAWN_COMPATIBLE_SHELLS = frozenset({"bash", "zsh", "sh", "dash", "ksh", "mksh"})
+
+
+def _find_shell() -> str:
+    """Find the user's login shell for background process spawning.
+
+    Unlike ``_find_bash`` (which always returns a bash binary for callers
+    that explicitly need bash), this function prefers the user's configured
+    ``$SHELL`` on POSIX so that ``spawn_local`` uses the shell the user
+    actually logs in with.
+
+    On macOS Catalina+ the default login shell is zsh, but
+    ``shutil.which("bash")`` still finds the system ``/bin/bash`` (GNU bash
+    3.2).  When bash 3.2 is invoked with ``-l`` (login) and stdin is
+    ``/dev/null``, it sources ``~/.bash_profile`` which on many macOS setups
+    contains ``exec /bin/zsh -l``.  That ``exec`` replaces bash with zsh but
+    drops the ``-c`` argument, so the background command never runs — the
+    subprocess exits 0 with no output and no side effects.
+
+    Preferring ``$SHELL`` (when it is a POSIX-``sh``-family shell) avoids this
+    because zsh/bash/sh/dash/ksh handle ``-lic`` correctly even with
+    redirected stdin.
+
+    Only POSIX-sh-family shells are honoured: ``spawn_local`` invokes the
+    shell as ``[shell, "-lic", "set +m; <cmd>"]``, and that ``-lic`` bundle +
+    ``set +m`` job-control syntax is NOT understood by fish, csh/tcsh,
+    nushell, elvish, xonsh, etc.  Returning such a ``$SHELL`` would trade the
+    bash-3.2 swallow for a parse error on every background command, so for any
+    non-allowlisted shell we fall back to ``_find_bash`` (the prior behaviour).
+
+    On Windows, ``$SHELL`` is typically bash (Git Bash), so behaviour is
+    unchanged — we fall through to ``_find_bash``.
+    """
+    if not _IS_WINDOWS:
+        user_shell = os.environ.get("SHELL")
+        if (
+            user_shell
+            and os.path.isfile(user_shell)
+            and os.access(user_shell, os.X_OK)
+            and Path(user_shell).name in _SPAWN_COMPATIBLE_SHELLS
+        ):
+            return user_shell
+    return _find_bash()
 
 
 # Standard PATH entries for environments with minimal PATH.