Skip to content

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

Merged
Salil Das (sadlilas) merged 2 commits into
mainfrom
fix/background-no-bash-actionable-error
Aug 18, 2026
Merged

fix: close the background-path gap where missing bash silently ran commands with no shell#17
Salil Das (sadlilas) merged 2 commits into
mainfrom
fix/background-no-bash-actionable-error

Conversation

@sadlilas

@sadlilas Salil Das (sadlilas) commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Problem

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 on Windows, the code fell back to:

args = shlex.split(command)
process = subprocess.Popen(args, ...)

This created two failure modes:

  1. Bare OS error (first token NOT a real exe): For commands like echo hello or dir, Popen raised FileNotFoundError. Caught by execute()'s generic except handler, this became a bare [WinError 2] The system cannot find the file specified — no cause, no remedy.

  2. Misleading success (first token IS a real exe): For commands like python --version or git status, the first token happens to be a real executable on Windows. Popen succeeded, returned a PID, and execute() reported success=True. But bash never ran the command — pipes, &&, ~, and $VAR were silently passed as literal argv. This is worse than an error: a false positive that corrupts downstream logic.

Solution

Part 1: Extracted the foreground's actionable error message into a shared _WINDOWS_NO_BASH_ERROR constant. The user-visible text is byte-for-byte unchanged (existing foreground tests still pass). This makes it structurally impossible for the two paths to drift again.

Part 2: _run_command_background now raises RuntimeError(_WINDOWS_NO_BASH_ERROR) when no bash is found, launching nothing.

Raising rather than returning an error sentinel keeps the return contract of _run_command_background a plain {"pid": ...} with no optional keys callers must remember to check. execute() already wraps this call in a generic except Exception that produces ToolResult(success=False, output=str(e), error={"message": str(e)}) — exactly the shape the foreground path returns. No dedicated unwrapping branch is needed in execute().

Part 3: Removed the now-unused import shlex.

Test Coverage

Three new tests added (following the style of test_gap_bash_missing_actionable_error.py):

  • test_background_real_exe_command_does_not_launch_when_bash_missing — Verifies the misleading-success regression is fixed: subprocess.Popen is NEVER called when bash is missing, and the result is success=False.

  • test_background_plain_command_gets_actionable_error_when_bash_missing — Verifies non-exe commands get the actionable error, not a bare WinError.

  • test_background_launches_normally_when_bash_is_found — No regression: when bash exists, background path launches normally via Popen.

All three patch the Windows-only subprocess.DETACHED_PROCESS / CREATE_NEW_PROCESS_GROUP constants (which do not exist on Linux/macOS, where most of the CI matrix runs) and spy on the synchronous subprocess.Popen with MagicMock. Both details are load-bearing: without the constant patches, the pre-fix code raises AttributeError while evaluating creationflags= before Popen is reached, so assert not popen_spy.called would pass against the broken code for the wrong reason; and an async spy on a synchronous Popen makes the pre-fix path die on process.pid rather than producing the misleading success=True the test exists to catch.

Verified RED against the pre-fix source: the two regression tests fail on assert not popen_spy.called — Popen was called — which is the actual regression signature, with no AttributeError and no never-awaited warnings.

Verification

uv run pytest -q
114 passed, 12 skipped

Baseline was 111 passed, 12 skipped. The +3 is exactly these new tests. Zero regressions.

ruff check clean. ruff format --diff shows only a pre-existing hunk untouched by this PR. pyright output is identical to the base commit (no new type errors).

Scope

This fix deliberately does NOT touch:

  • WSL routing or path translation
  • The arbitration logic or _resolve_windows_bash

It closes only the background no-bash gap, leaving architecture and WSL integration for separate work.


Fixes the background-path case that PR #15 addressed for the foreground.

…mmands with no shell

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>
…tual regression

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>
@sadlilas
Salil Das (sadlilas) merged commit 85c6329 into main Aug 18, 2026
7 checks passed
@sadlilas
Salil Das (sadlilas) deleted the fix/background-no-bash-actionable-error branch August 18, 2026 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant