From d9f1f1a1de42de2ff1f15e0cd1cdf8ff61234ec6 Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Fri, 26 Jun 2026 20:20:28 +0530 Subject: [PATCH] fix(terminal): prefer $SHELL over bash for background process spawning (#42203) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On macOS, terminal(background=true) silently failed: the process returned a session_id and exit_code=0 but the command never ran (empty stdout, no side effects). Root cause is two interacting issues: 1. _find_shell was aliased to _find_bash, which prefers `shutil.which("bash")` → /bin/bash (GNU bash 3.2, still shipped on macOS) over $SHELL (/bin/zsh). 2. process_registry.spawn_local runs [shell, "-lic", "set +m; "] with stdin=/dev/null. bash 3.2 as a login shell sources ~/.bash_profile, which on many macOS setups contains `exec /bin/zsh -l`; that exec replaces bash but drops the -c argument, so the command is swallowed (exit 0, no output). Decouple _find_shell from _find_bash: _find_shell now prefers the user's configured $SHELL on POSIX (the shell they actually log in with), falling back to _find_bash when $SHELL is unset/missing. _find_bash is unchanged, so callers that genuinely need bash (e.g. the _run_bash login-shell snapshot) keep bash semantics. zsh handles -lic correctly even with redirected stdin. Salvaged from #42219 by @liuhao1024 (authorship preserved via cherry-pick). On top of the original (8 unit tests covering $SHELL-set/unset/missing/empty, Windows-ignores-$SHELL, _find_bash-unchanged), added an E2E regression test that reproduces the real bash-3.2 login-shell swallow (exit 0 / no file) and asserts the shell _find_shell selects actually executes a -lic background command. Mutation-verified: reverting _find_shell to the bash alias fails the $SHELL-preference test. Bug reproduced directly: /bin/bash 3.2 -lic with a .bash_profile->exec-zsh creates no file; zsh -lic does. Closes #42203. Supersedes #42290. --- tests/tools/test_find_shell.py | 188 +++++++++++++++++++++++++++++++++ tools/environments/local.py | 49 ++++++++- 2 files changed, 235 insertions(+), 2 deletions(-) create mode 100644 tests/tools/test_find_shell.py diff --git a/tests/tools/test_find_shell.py b/tests/tools/test_find_shell.py new file mode 100644 index 00000000000..6de3b2594f9 --- /dev/null +++ b/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; "] + # 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" diff --git a/tools/environments/local.py b/tools/environments/local.py index 3b07b539752..0a1651a758b 100644 --- a/tools/environments/local.py +++ b/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; "]``, 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.