diff --git a/amplifier_module_tool_bash/__init__.py b/amplifier_module_tool_bash/__init__.py index 1180fb0..bdaeae7 100644 --- a/amplifier_module_tool_bash/__init__.py +++ b/amplifier_module_tool_bash/__init__.py @@ -9,7 +9,6 @@ import asyncio import logging import os -import shlex import shutil import signal import subprocess @@ -654,6 +653,27 @@ async def _protect_windows_descendants(root_pid: int) -> None: _WINDOWS_SHELL_PREFERENCE_ENV_VAR = "AMPLIFIER_BASH_WINDOWS_SHELL" _VALID_WINDOWS_SHELL_PREFERENCES = ("auto", "wsl", "gitbash") +# Actionable "no bash found on Windows" message, shared verbatim by both the +# foreground (`_run_command`) and background (`_run_command_background`) +# no-bash branches -- see each call site for why there is no degraded +# fallback (e.g. cmd.exe, or exec-with-no-shell-at-all) for "simple" +# commands. +_WINDOWS_NO_BASH_ERROR = ( + "Bash not found in PATH.\n" + "\n" + "This tool requires bash for POSIX shell semantics " + "(quoting, tilde expansion, pipes, redirects, " + "command substitution). Without it, even simple " + "commands cannot be run with correct, predictable " + "behavior.\n" + "\n" + "Install Git for Windows (includes Git Bash):\n" + " https://git-scm.com/download/win\n" + "\n" + "Or install WSL:\n" + " https://learn.microsoft.com/en-us/windows/wsl/install" +) + def _find_git_bash_executable() -> str | None: """Probe well-known Git-for-Windows install locations for bash.exe, @@ -1493,20 +1513,22 @@ async def _run_command_background(self, command: str) -> dict[str, Any]: | subprocess.CREATE_NEW_PROCESS_GROUP, ) else: - try: - args = shlex.split(command) - except ValueError as e: - raise ValueError(f"Invalid command syntax: {e}") - - process = subprocess.Popen( - args, - stdout=devnull, - stderr=devnull, - stdin=devnull, - cwd=self.working_dir, - creationflags=subprocess.DETACHED_PROCESS - | subprocess.CREATE_NEW_PROCESS_GROUP, - ) + # No bash found on Windows. Same contract as the + # foreground path (`_run_command`): a tool named + # `bash` silently running a command with no shell at + # all (or raising a bare OS error for anything else) + # is a degraded state pretending to be a working one. + # Surface the same actionable error instead of + # attempting to run anything. + # + # Raising (rather than returning a sentinel) keeps the + # return contract of this method a plain `{"pid": ...}` + # with no optional keys for callers to remember to + # check. `execute()` already wraps this call and turns + # any exception into ToolResult(success=False, + # output=str(e), error={"message": str(e)}) -- exactly + # the shape the foreground path returns. + raise RuntimeError(_WINDOWS_NO_BASH_ERROR) else: # Unix-like: Use start_new_session to create new session, fully detached process = subprocess.Popen( @@ -1608,21 +1630,7 @@ async def _run_command( # for every command, with the real cause and the fix. return { "stdout": "", - "stderr": ( - "Bash not found in PATH.\n" - "\n" - "This tool requires bash for POSIX shell semantics " - "(quoting, tilde expansion, pipes, redirects, " - "command substitution). Without it, even simple " - "commands cannot be run with correct, predictable " - "behavior.\n" - "\n" - "Install Git for Windows (includes Git Bash):\n" - " https://git-scm.com/download/win\n" - "\n" - "Or install WSL:\n" - " https://learn.microsoft.com/en-us/windows/wsl/install" - ), + "stderr": _WINDOWS_NO_BASH_ERROR, "returncode": 1, } else: diff --git a/tests/test_gap_background_bash_missing.py b/tests/test_gap_background_bash_missing.py new file mode 100644 index 0000000..7c02e01 --- /dev/null +++ b/tests/test_gap_background_bash_missing.py @@ -0,0 +1,195 @@ +"""Regression test: bash-not-found on Windows must give an actionable error +for the BACKGROUND (`run_in_background=True`) path too, not a misleading +success or a bare OS error. + +## Why this test exists + +The foreground path (`_run_command`, covered by +``test_gap_bash_missing_actionable_error.py``) was fixed to return the +actionable "bash not found" error unconditionally when no bash is +discoverable on Windows. The background path (`_run_command_background`) +was not fixed and still contained the pre-fix fallback: + +```python +else: + try: + args = shlex.split(command) + except ValueError as e: + raise ValueError(f"Invalid command syntax: {e}") + process = subprocess.Popen(args, ...) +``` + +Two distinct failure modes resulted: + +- If the command's first token is not a real ``.exe`` (``echo hello``, + ``dir``): ``Popen`` raises ``FileNotFoundError`` -> caught by + ``execute()``'s generic ``except Exception`` -> the model sees a bare + ``[WinError 2] The system cannot find the file specified`` with no cause + and no remedy. +- If the first token IS a real exe (``python --version``, ``git status``): + it launches with **no shell at all** and returns a PID -> ``execute()`` + reports ``success=True``. ``&&``, pipes, ``~``, ``$VAR`` are silently + passed as literal argv. This is the worse case: a **misleading success**. + +The fix makes ``_run_command_background`` return the same actionable error +as the foreground path (via the shared ``_WINDOWS_NO_BASH_ERROR`` constant) +without launching anything, and wires ``execute()`` to surface that as +``ToolResult(success=False, ...)`` instead of wrapping a (nonexistent) PID +into a success result. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +import amplifier_module_tool_bash as mod +from amplifier_module_tool_bash import BashTool + + +@pytest.mark.asyncio +async def test_background_real_exe_command_does_not_launch_when_bash_missing() -> None: + """`python --version` (first token IS a real executable) must NOT be + launched with no shell when bash is missing on Windows -- this is the + misleading-success regression: a PID gets returned and `execute()` + reports success=True even though no real shell ran the command. + """ + tool = BashTool({}) + + # `subprocess.Popen` is synchronous -- spy with MagicMock, not + # AsyncMock. An AsyncMock returns a coroutine, so the pre-fix code + # would die on `process.pid` instead of producing the misleading + # success this test exists to guard against. Returning a usable + # fake process is what lets the pre-fix path reach `success=True` + # with a PID -- i.e. actually reproduce the regression. + class _FakeProcess: + pid = 1234 + + popen_spy = MagicMock(return_value=_FakeProcess()) + + with ( + patch("amplifier_module_tool_bash.sys.platform", "win32"), + patch("amplifier_module_tool_bash.shutil.which", return_value=None), + patch( + "amplifier_module_tool_bash._find_git_bash_executable", return_value=None + ), + patch( + "amplifier_module_tool_bash._find_wsl_bash_executable", return_value=None + ), + patch("amplifier_module_tool_bash.subprocess.Popen", popen_spy), + # Windows-only constants absent from `subprocess` on Linux/macOS. + # Without these, the PRE-FIX code raises AttributeError while + # evaluating `creationflags=` -- BEFORE Popen is ever reached -- + # so `assert not popen_spy.called` below would pass against the + # broken code for entirely the wrong reason, and the + # misleading-success regression would never be exercised at all. + # Supplying them makes this test model real Windows. + patch.object(mod.subprocess, "DETACHED_PROCESS", 0x00000008, create=True), + patch.object( + mod.subprocess, "CREATE_NEW_PROCESS_GROUP", 0x00000200, create=True + ), + ): + result = await tool.execute( + {"command": "python --version", "run_in_background": True} + ) + + assert not popen_spy.called, ( + "no-bash-on-Windows background path called subprocess.Popen with no " + "shell at all -- this is the exact regression the fix removes: a " + "command whose first token happens to be a real executable must " + "never be launched with no shell and reported as a misleading " + "success, it must get the actionable bash-missing error" + ) + assert result.success is False + assert "bash" in str(result.output).lower() + assert ( + "git-scm.com" in str(result.output) + or "git for windows" in str(result.output).lower() + ) + assert "wsl" in str(result.output).lower() + + +@pytest.mark.asyncio +async def test_background_plain_command_gets_actionable_error_when_bash_missing() -> ( + None +): + """`echo hello` (no shell metacharacters, first token is not a real + exe) must get the actionable "bash not found" error in the background + path too -- not a bare FileNotFoundError/WinError from Popen. + """ + tool = BashTool({}) + + # Synchronous spy -- see the note in the test above. + class _FakeProcess: + pid = 1234 + + popen_spy = MagicMock(return_value=_FakeProcess()) + + with ( + patch("amplifier_module_tool_bash.sys.platform", "win32"), + patch("amplifier_module_tool_bash.shutil.which", return_value=None), + patch( + "amplifier_module_tool_bash._find_git_bash_executable", return_value=None + ), + patch( + "amplifier_module_tool_bash._find_wsl_bash_executable", return_value=None + ), + patch("amplifier_module_tool_bash.subprocess.Popen", popen_spy), + # See the note in the test above: without these Windows-only + # constants the pre-fix code dies on AttributeError before + # reaching Popen, so this test would not exercise the real + # bare-WinError regression it is written to guard. + patch.object(mod.subprocess, "DETACHED_PROCESS", 0x00000008, create=True), + patch.object( + mod.subprocess, "CREATE_NEW_PROCESS_GROUP", 0x00000200, create=True + ), + ): + result = await tool.execute( + {"command": "echo hello", "run_in_background": True} + ) + + assert not popen_spy.called + assert result.success is False + assert "WinError" not in str(result.output), ( + f"got a bare OS error instead of the actionable message: {result.output!r}" + ) + assert "bash" in str(result.output).lower() + + +@pytest.mark.asyncio +async def test_background_launches_normally_when_bash_is_found() -> None: + """No regression: when bash IS found on Windows, the background path + must still launch via subprocess.Popen exactly as before. + """ + tool = BashTool({}) + + class _FakeProcess: + pid = 4242 + + popen_spy = MagicMock(return_value=_FakeProcess()) + + with ( + patch("amplifier_module_tool_bash.sys.platform", "win32"), + patch( + "amplifier_module_tool_bash.shutil.which", + return_value="C:\\fake\\Git\\bin\\bash.exe", + ), + patch.object(BashTool, "_is_wsl_bash", AsyncMock(return_value=False)), + patch("amplifier_module_tool_bash.subprocess.Popen", popen_spy), + # Windows-only constants that don't exist on the real `subprocess` + # module on Linux/macOS -- the test suite runs there, so these must + # be supplied for the (mocked-anyway) Popen call's keyword + # arguments to even evaluate. Matches the pattern used in + # test_windows_shell_resolution.py. + patch.object(mod.subprocess, "DETACHED_PROCESS", 0x00000008, create=True), + patch.object( + mod.subprocess, "CREATE_NEW_PROCESS_GROUP", 0x00000200, create=True + ), + ): + result = await tool.execute( + {"command": "echo hello", "run_in_background": True} + ) + + assert popen_spy.called, "bash was found but subprocess.Popen was never called" + assert result.success is True + assert result.output["pid"] == 4242