From d6112d909532fac1ce408e763eeb2fc2331fdd9e Mon Sep 17 00:00:00 2001 From: sadlilas <11658960+sadlilas@users.noreply.github.com> Date: Tue, 18 Aug 2026 12:28:52 -0700 Subject: [PATCH 1/2] fix: close the background-path gap where missing bash silently ran commands with no shell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #15 fixed the foreground no-bash case on Windows, but missed the background path entirely. When run_in_background=True and no bash was found, the code fell back to shlex.split + subprocess.Popen with no shell at all. Two failure modes resulted: 1. For commands with no shell metacharacters whose first token is NOT a real exe (echo, dir, etc.): Popen raised FileNotFoundError, caught as a bare WinError with neither cause nor remedy. 2. For commands whose first token IS a real exe (python, git, etc.): they launched successfully with no shell and returned a PID, so execute() reported success=True. Pipes, &&, ~, and $VAR were silently passed as literal argv. This misleading success is worse than the error case. The fix: - Extracted the foreground's actionable error message into a shared _WINDOWS_NO_BASH_ERROR constant (byte-for-byte unchanged, foreground tests still pass). - _run_command_background now returns {"pid": None, "error": message} when no bash is found, launching nothing. - execute()'s background branch now checks for the "error" key and returns ToolResult(success=False, ...) with the actionable text instead of a misleading success or bare OS error. - Removed the now-unused shlex import. Three new tests added (matching the style of test_gap_bash_missing_actionable_error.py): - test_background_real_exe_command_does_not_launch_when_bash_missing: asserts Popen is never called and success=False when a real exe command is given with no bash (the misleading-success regression). - test_background_plain_command_gets_actionable_error_when_bash_missing: verifies a non-exe command gets the actionable error, not a bare WinError. - test_background_launches_normally_when_bash_is_found: no regression when bash exists. Verification: uv run pytest -q → 114 passed, 12 skipped (was 111 passed; +3 are the new tests, zero regressions). ruff clean on touched files. Scope: deliberately does NOT touch WSL routing, path translation, or _resolve_windows_bash — closes only the background no-bash gap. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_module_tool_bash/__init__.py | 69 +++++---- tests/test_gap_background_bash_missing.py | 163 ++++++++++++++++++++++ 2 files changed, 202 insertions(+), 30 deletions(-) create mode 100644 tests/test_gap_background_bash_missing.py diff --git a/amplifier_module_tool_bash/__init__.py b/amplifier_module_tool_bash/__init__.py index 1180fb0..07aad83 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, @@ -1173,6 +1193,15 @@ async def execute(self, input: dict[str, Any]) -> ToolResult: if run_in_background: # Execute command in background and return immediately result = await self._run_command_background(command) + if "error" in result: + # No bash on Windows: nothing was launched (no PID), + # surface the same actionable error the foreground + # path returns instead of a misleading success. + return ToolResult( + success=False, + output=result["error"], + error={"message": result["error"]}, + ) return ToolResult( success=True, output={ @@ -1493,20 +1522,14 @@ 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. + return {"pid": None, "error": _WINDOWS_NO_BASH_ERROR} else: # Unix-like: Use start_new_session to create new session, fully detached process = subprocess.Popen( @@ -1608,21 +1631,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..249926d --- /dev/null +++ b/tests/test_gap_background_bash_missing.py @@ -0,0 +1,163 @@ +"""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({}) + + popen_spy = AsyncMock() + + 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), + ): + 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({}) + + popen_spy = AsyncMock() + + 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), + ): + 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 From 4bb67103c828c8397805460e7fe12b1f9ed6398b Mon Sep 17 00:00:00 2001 From: sadlilas <11658960+sadlilas@users.noreply.github.com> Date: Tue, 18 Aug 2026 12:52:29 -0700 Subject: [PATCH 2/2] fix: replace error-sentinel pattern with raise; fix tests to guard actual regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FIXES (3 total): 1. SOURCE: Replace error-sentinel return → raise _run_command_background returned {"pid": None, "error": ...} when bash missing on Windows, plus ~9-line unwrapping block in execute(). But execute() already wraps this in generic except Exception that produces identical ToolResult. The sentinel turned the contract into "dict that might have error key". Now raises RuntimeError directly. Return contract stays plain {"pid": ...}. Net -9 lines, identical behavior. 2. TESTS: Add missing Windows constant patches Tests 1 & 2 patched sys.platform="win32" but not subprocess.DETACHED_PROCESS / CREATE_NEW_PROCESS_GROUP (don't exist on Linux/macOS where CI runs). Against pre-fix code, Python raised AttributeError before Popen was reached, so "assert not popen_spy.called" passed for wrong reason. Tests could never catch the regression they guard. Test 3 already had the right pattern; tests 1 & 2 didn't. Added patches. 3. TESTS: Fix AsyncMock spy on synchronous Popen Tests 1 & 2 used AsyncMock() to spy on subprocess.Popen (sync function). AsyncMock returns coroutine, so pre-fix path died on 'coroutine' object has no attribute 'pid' + never-awaited RuntimeWarning, instead of silent success=True + PID that test docstring claims to guard. Swapped to MagicMock(return_value=_FakeProcess()), matching test 3 pattern. VERIFICATION: - RED verified against pre-fix (bf96523) with fixed tests: 2 failed / 1 passed, failing on "assert not popen_spy.called" (Popen WAS called) — real regression signature, no AttributeError, no warnings - GREEN: full suite 114 passed, 12 skipped; test file 3 passed - ruff: all checks passed (identical to baseline) - pyright: 5 errors, identical to baseline (shifted line numbers only); all 4 pre-date this PR 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_module_tool_bash/__init__.py | 19 ++++++------ tests/test_gap_background_bash_missing.py | 36 +++++++++++++++++++++-- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/amplifier_module_tool_bash/__init__.py b/amplifier_module_tool_bash/__init__.py index 07aad83..bdaeae7 100644 --- a/amplifier_module_tool_bash/__init__.py +++ b/amplifier_module_tool_bash/__init__.py @@ -1193,15 +1193,6 @@ async def execute(self, input: dict[str, Any]) -> ToolResult: if run_in_background: # Execute command in background and return immediately result = await self._run_command_background(command) - if "error" in result: - # No bash on Windows: nothing was launched (no PID), - # surface the same actionable error the foreground - # path returns instead of a misleading success. - return ToolResult( - success=False, - output=result["error"], - error={"message": result["error"]}, - ) return ToolResult( success=True, output={ @@ -1529,7 +1520,15 @@ async def _run_command_background(self, command: str) -> dict[str, Any]: # is a degraded state pretending to be a working one. # Surface the same actionable error instead of # attempting to run anything. - return {"pid": None, "error": _WINDOWS_NO_BASH_ERROR} + # + # 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( diff --git a/tests/test_gap_background_bash_missing.py b/tests/test_gap_background_bash_missing.py index 249926d..7c02e01 100644 --- a/tests/test_gap_background_bash_missing.py +++ b/tests/test_gap_background_bash_missing.py @@ -56,7 +56,16 @@ async def test_background_real_exe_command_does_not_launch_when_bash_missing() - """ tool = BashTool({}) - popen_spy = AsyncMock() + # `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"), @@ -68,6 +77,17 @@ async def test_background_real_exe_command_does_not_launch_when_bash_missing() - "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} @@ -99,7 +119,11 @@ async def test_background_plain_command_gets_actionable_error_when_bash_missing( """ tool = BashTool({}) - popen_spy = AsyncMock() + # 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"), @@ -111,6 +135,14 @@ async def test_background_plain_command_gets_actionable_error_when_bash_missing( "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}