fix: close the background-path gap where missing bash silently ran commands with no shell

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:

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