fix: Windows Job Object process linkage for tool subprocess isolation - #15
Merged
Conversation
GAP-013, GAP-024, GAP-028: Windows has no parent-death signal (no PR_SET_PDEATHSIG equivalent), so a tool subprocess spawned here survives a parent killed outright — crash, taskkill /F on one PID, a supervisor killing only the parent. Proven with live bash-tool subprocess in flight: killing *only* the top-level amplifier.exe PID left the wsl.exe → wsl.exe → wslhost.exe chain alive at all 18 polls from t+3s through t+60.8s. Initial Job Object implementation only captured the immediate wsl.exe launcher, not its descendants (proven via IsProcessInJob). Fixes: - Lazily-created Windows Job Object (CreateJobObjectW + JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, via ctypes, no new dependency) assigned to every foreground tool subprocess. Kernel tears the job down the instant the handle closes for *any* reason — crash, signal, external kill, etc. - CreateToolhelp32Snapshot process-tree walk assigning every descendant to the job, not just the launcher. Proof: before → target PIDs alive at all 18 checks; after → zero alive at first check (t+3s). Verified against genuine uncaught exception too: top-level process crashed with real traceback, all three wsl-family PIDs dead at first check. Regression test (test_gap013_windows_job_object_orphans.py) proven to have teeth: no-op'ing _assign_to_windows_job → fails with "grandchild process survived"; restore → 3/3 pass. Regression: Linux 59 passed / 10 skipped (all Windows-only guards); macOS 58 passed / 1 pre-existing unrelated failure. Pure no-op off Windows, confirmed by code inspection and all skips. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Brian Krabach (bkrabach)
marked this pull request as draft
August 11, 2026 19:13
Remove local editable path to amplifier-core in favour of canonical git+https source — the local path rendered the repo uninstallable for any user or system outside the authoring workspace. Add missing pytest and pytest-asyncio to dev dependency group. The suite was silently falling through to a globally-installed pytest outside the venv, masking 6 collection errors. Proof: Clean clone at /tmp with no ../amplifier-core sibling: Before: uv build fails with 'Distribution not found' After: uv build succeeds, 59 passed, 10 skipped 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…r sweeps Findings from independent adversarial review of PR #15: 1. CRITICAL — protection could silently fail with zero signal. All three call sites discarded _assign_to_windows_job()'s return value, and failures logged at debug only. AssignProcessToJobObject fails in GitHub Actions, Windows containers, and under some EDR agents — the normal state in many environments. Protection never engaged but commands still succeeded. Now reported once per process at warning level via _report_windows_job_failure(), plainly stating protection is NOT active and why. Environmental cause is constant, so once per process avoids training people to ignore repeated messages. 2. HIGH — no test drove actual call sites. Existing GAP-013 test called _assign_to_windows_job directly, bypassing _run_command, and was Windows- gated in a CI-less repo (ran nowhere automatically). Refactor dropping one call site would have kept suite green. Added tests/test_windows_job_call_sites.py which patches sys.platform and both helpers, driving real _run_command and asserting it job-assigns the spawned PID. Runs on every platform, needs no Windows kernel. 3. MEDIUM — fire-and-forget asyncio.create_task held no reference. Stdlib docs explicit: unreferenced tasks may garbage-collect before done. These sweeps are the entire GAP-013/GAP-028 protection for WSL descendants. Collected task looks identical to silent assignment failure. Now via _spawn_descendant_sweep() holding refs in module-level set with done-callback for cleanup. 4. MEDIUM — no restype/argtypes on HANDLE-returning Win32 calls. ctypes defaults undeclared return to c_int (32-bit), truncating 64-bit HANDLE. Win32 guarantees handles are 32-bit-significant so it worked, but relying on unstated guarantee is how corruption bugs get in. Declared on CreateJobObjectW, SetInformationJobObject, OpenProcess, AssignProcessToJobObject, CloseHandle. Error logs now use ctypes.WinError(...) instead of bare integer. 5. LOW — lazy threading.Lock() creation was itself unsynchronised. if _lock is None: _lock = Lock() lets two threads build two locks. Moved to module import. Benign vs environmental failure now distinguished: OpenProcess failure usually means process already exited (debug level), while AssignProcessToJobObject failure is environmental (warning once). Test results: 62 passed, 10 skipped (baseline 59 passed, 10 skipped). Delta is exactly 3 new contract tests. ruff check passed both before and after. POSIX safety via test assertion: with sys.platform patched to 'linux', neither _assign_to_windows_job nor _spawn_descendant_sweep is called — platform guard now test-enforced. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…n Windows The test file imports pty at module scope — a POSIX-only stdlib module with no Windows equivalent. On Windows, import raises during collection, surfacing as a hard ERROR. pytest.skip(..., allow_module_level=True) placed before the POSIX import prevents this. Previously, pytestmark guards were too late (evaluated after module body executes). Verified on POSIX (Linux aarch64, macOS arm64): - Full suite: 62 passed (Linux), 2 passed (macOS) Windows claim (intended: 1 error → 0) NOT verified — test machine went offline. Change is correct by inspection and matches pytest's documented mechanism. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
With no bash on PATH, simple commands failed with a bare OS error naming neither cause nor fix: [WinError 2] The system cannot find the file specified. Root cause: The no-bash branch returned the actionable error only when the command contained a shell metacharacter. Otherwise it fell through to shlex.split() + create_subprocess_exec with no shell at all, so every cmd.exe builtin failed silently with WinError 2. A docstring also claimed the tool 'falls back to cmd.exe' — it never touched cmd.exe. Commands like 'echo hello' and 'ls' had no signal why they failed. A tool named 'bash' whose entire contract is POSIX shell semantics should not silently run a subset of commands through something else — that is a degraded state pretending to work, and the user's mental model breaks the first time quoting or a builtin differs, with no signal why. Fixed by making the actionable error unconditional when bash is missing, correcting the docstring to match, and adding regression tests that verify the error fires for both shell-free and shell-requiring commands. Linux: 62 → 65 passed (+3), 10 skipped. Ruff clean A/B. Regression test shown failing before the fix (old code reaches create_subprocess_exec for a plain command) and passing after. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Three related Windows shell-resolution changes:
1. Git Bash was undiscoverable even when installed. On Windows with WSL enabled,
shutil.which('bash') always returns System32 launcher first. Git for Windows'
default install puts Git\bin on no PATH. Fixed by probing well-known install
locations directly when PATH resolves nothing or user opts in via config/env.
Preserves WSL default when both present; default unchanged.
2. Resolved shell was invisible until failure. Now logged at INFO and named in
tool description with its path conventions (/mnt/c/... vs /c/...). Field
research shows silent shell divergence is the dominant failure mode across
Claude Code, Cursor, Codex CLI, and others.
3. Latent disagreement between foreground and background shell selection fixed
by awaiting single cached-once-per-instance async resolution rather than
defaulting on cache miss.
Evidence: End-to-end on Windows 11 build 26200 with Git Bash 2.55.0.3:
- auto mode resolves to WSL (/mnt/c/ paths)
- AMPLIFIER_BASH_WINDOWS_SHELL=gitbash overrides to Git Bash (/c/ paths)
- Unit coverage: 62 → 88 passed (+26 new tests covering all three changes)
- Linux: ruff A/B identical (zero new issues)
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
SetInformationJobObject had a restype but no argtypes, so the argument list still defaulted to C int, which is exactly the truncation this class of bug produces on 64-bit Windows. Declared HANDLE, c_int, LPVOID, DWORD to match the real SetInformationJobObject signature. _enumerate_child_pids_windows was worse: none of its four kernel32 calls (CreateToolhelp32Snapshot, Process32First, Process32Next, CloseHandle) had any signature declared at all. CreateToolhelp32Snapshot returns a HANDLE, so an undeclared restype truncates it to 32 bits on a 64-bit process -- silent corruption, not a clean failure. Declared all four explicitly using wintypes.HANDLE/DWORD/BOOL and POINTER(PROCESSENTRY32), same pattern already used for the job-object calls a few functions above. Added tests/test_windows_ctypes_signatures.py to actually exercise this. Patching sys.platform to "win32" and ctypes.WinDLL to a MagicMock lets _get_windows_job_object, _assign_to_windows_job, and _enumerate_child_pids_windows run for real -- the DLL handle is the only stand-in, the signature-declaration code executes as written -- and the test asserts every kernel32 function touched has both argtypes and restype set. Runs on Linux/macOS/CI today; no Windows box required.
Salil Das (sadlilas)
force-pushed
the
fix/gap-013-024-028
branch
from
August 12, 2026 22:20
377e894 to
0555d61
Compare
… signature Fix a surviving instance of the handle-sentinel bug class: CreateToolhelp32Snapshot signals failure with INVALID_HANDLE_VALUE = (HANDLE)-1, not NULL. With wintypes.HANDLE restype, ctypes converts NULL to None and the -1 bit pattern to a large positive int (18446744073709551615 on 64-bit). The guard 'if snap in (-1, 0):' could never match a real failure, allowing execution to fall through to Process32First and CloseHandle on invalid handles — flagged as fatal under Application Verifier / gflags. Changes: - Added _INVALID_HANDLE_VALUE = ctypes.c_void_p(-1).value and updated guard to 'if snap is None or snap == _INVALID_HANDLE_VALUE:'. Restype left unchanged (was already correct; only the comparison was wrong). - Corrected PROCESSENTRY32.th32DefaultHeapID from ctypes.POINTER(ctypes.c_ulong) to ctypes.c_size_t. Real Win32 type is ULONG_PTR (pointer-width integer), not pointer-to-ulong. Layout was accidentally correct on 64-bit; type is now semantically right. - Added test_snapshot_invalid_handle_value_is_treated_as_failure in tests/test_windows_ctypes_signatures.py, verifying real failure value returns empty result and neither Process32First nor CloseHandle are called. Pre-existing test used bare MagicMock with zero coverage of sentinel comparison; new test verified to genuinely fail against old guard. Investigation confirmed Win32 job-object containment is NOT a no-op on WSL path — it contains the Win32 launcher processes (wsl.exe/wslhost.exe); only the process inside WSL2 VM is unreachable, already documented in _protect_windows_descendants. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Two tests mocked shutil.which returning None and expected the actionable bash-missing error. But the filesystem-probe fix on this branch queries well-known install locations directly (/Program Files/Git, etc.) precisely because Git for Windows never adds Git\bin to PATH. On a real Windows box with Git installed, the filesystem probe succeeds, bash is found, the actionable error never fires, and execution reaches create_subprocess_exec() — surfacing as 'ValueError: not enough values to unpack' when the mocked subprocess returns nothing. This was invisible on Linux where these tests are platform-guarded. The tests now properly neutralize both _find_git_bash_executable and _find_wsl_bash_executable alongside shutil.which. **Worth noting: this was a genuine interaction between two fixes on the same branch** — the filesystem-probe fix silently invalidated the actionable-error test on native Windows. This is exactly the kind of cross-platform gap that has no CI visibility until tests run where they're actually exercised. Evidence — native Windows 11, this test file: before: 2 failed, 94 passed after: 96 passed Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
This module had no CI. The entire Windows surface is shell resolution (Git Bash vs WSL vs neither), and every test guarding that path is POSIX-skipped, so platform-specific failures are invisible today. New workflow with a three-OS matrix [ubuntu-latest, macos-latest, windows-latest]. Includes fail-fast: false to preserve POSIX results if Windows fails. Honest limit: GitHub's Windows runner has no WSL, so the `wsl --exec` path cannot be exercised there. It does have Git Bash, so the Git-Bash and no-bash paths are covered. WSL behaviour still needs a real machine. This is CI-only and deliberately scoped to be independent. Expect the first Windows run to be red — those failures already exist. We simply cannot see them on POSIX CI today. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
The test test_setsid_detached_child_is_killed_on_timeout was vacuous on macOS: it shelled out to setsid(1) from util-linux, which macOS does not ship. The subshell failed instantly, the marker was never written, and the test died at the marker assertion without ever exercising the cleanup hazard it claims to guard. Additionally, _find_descendant_pids walked /proc (Linux-only), meaning that even if the test had succeeded on macOS, it would still fail to find and kill a session-detached descendant on that platform -- a real product gap, not a test artifact. Fixed by two changes: 1. Detach the grandchild via os.setsid() (a POSIX syscall exposed by Python on every Unix, including macOS) instead of shelling out to the setsid binary. This is portable and exercises the identical hazard. 2. Add _find_descendant_pids_via_ps as a portable fallback using 'ps -A -o pid=,ppid=' (portable across GNU/BSD ps implementations) for POSIX systems without /proc. Refactored the pid/ppid tree walk into a shared _descendants_from_pid_ppid_pairs helper so both /proc and ps paths share a single implementation. Verified on Linux via monkey-patching os.listdir to raise on /proc, simulating macOS's absence of it. The fallback located and killed the detached descendant via ps alone. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
The git source pin for amplifier-core was added to resolve test-collection errors in a clean checkout. However, it forced uv to compile amplifier-core from source on every CI run, causing Windows legs to timeout before any tests executed. This is a documentation of a verification failure: CI infrastructure was added and reported as passing (5 checks completed), but the Windows legs never actually executed. The check counts were accurate; the check coverage was not. amplifier-core publishes prebuilt wheels (amplifier_core-1.6.1-cp311-abi3-win*.whl) that satisfy the dependency. Removing the [tool.uv.sources] block lets uv resolve from PyPI while maintaining the dependency. Verified: - Linux suites pass unchanged (92 passed, 10 skipped) - uv.lock resolves to amplifier-core v1.6.1 from PyPI registry - No regression in test collection with PyPI-sourced build Fixes: #15 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Brian Krabach (bkrabach)
marked this pull request as ready for review
August 18, 2026 02:03
Salil Das (sadlilas)
added a commit
that referenced
this pull request
Aug 18, 2026
…mmands with no shell (#17) PR #15 fixed the foreground no-bash case on Windows but missed the background path. With run_in_background=True and no bash present, the code fell back to shlex.split + Popen, producing either a bare [WinError 2] (first token not a real exe) or -- worse -- a misleading success=True with a live PID (first token IS a real exe, e.g. `python --version`), where bash never ran and pipes, &&, ~ and $VAR were passed through as literal argv. Extract the foreground's actionable message into a shared _WINDOWS_NO_BASH_ERROR constant (user-visible text byte-for-byte unchanged) so the two paths cannot drift again, and have _run_command_background raise RuntimeError(_WINDOWS_NO_BASH_ERROR) instead of launching anything. Raising rather than returning an error sentinel keeps the method's return contract a plain {"pid": ...} with no optional keys callers must remember to check -- execute() already wraps the call and turns any exception into ToolResult(success=False, output=str(e), error={"message": str(e)}), exactly the shape the foreground path returns. Drop the now-unused shlex import. Three regression tests cover both failure modes plus the no-regression case when bash is present. All three patch the Windows-only subprocess.DETACHED_PROCESS / CREATE_NEW_PROCESS_GROUP constants 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. Verified RED against the pre-fix source: the two regression tests fail on `assert not popen_spy.called` -- Popen was called -- the real regression signature, with no AttributeError and no never-awaited warnings. Full suite 114 passed, 12 skipped (baseline 111 + these 3). CI green across macOS/ubuntu/windows x py3.11/3.12. ruff clean; pyright identical to base.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GAP-013, GAP-024, GAP-028: Windows subprocess orphaning
Windows has no parent-death signal (no
PR_SET_PDEATHSIGequivalent), so a tool subprocess spawned here survives a parent killed outright — crash,taskkill /Fon one PID, a supervisor killing only the parent.Symptom
Proven with a live bash-tool subprocess in flight: killing only the top-level
amplifier.exePID left thewsl.exe → wsl.exe → wslhost.exechain alive at all 18 polls from t+3s through t+60.8s.Root cause investigation
Initial Job Object implementation only captured the immediate
wsl.exelauncher, not its descendants (proven via Win32IsProcessInJob). The earlier absence of visible orphans was coincidental — WSL's own connection teardown, not the job object doing its job.Fixes
CreateJobObjectW+JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, viactypes, no new dependency) assigned to every foreground tool subprocess. The kernel tears the job down the instant the handle closes for any reason — crash, signal, external kill, etc.CreateToolhelp32Snapshotto assign every descendant to the job, not just the launcher PID.Proof (native Windows)
Before: target PIDs alive at all 18 checks
After: zero alive at first check (t+3s)
Also verified against a genuine uncaught exception: top-level process crashed with real traceback, all three wsl-family PIDs dead at first check.
Regression testing
Included:
tests/test_gap013_windows_job_object_orphans.py(3 tests) proven to have teeth — no-op'ing_assign_to_windows_job→ fails with "grandchild process survived"; restore → 3/3 pass.All platforms:
Scope and limitations
Windows evidence is n=1. All Windows proof comes from a single machine —
alienware-r13, Windows NT 10.0.26200, Python 3.14. The code-logic fixes here generalize (they are platform-conditional logic bugs, not environment-specific). Where a fix's trigger was environmental, that is called out inline above.Cross-platform validation. Linux (aarch64), macOS (Darwin arm64), and WSL2 were all validated — full regression suites on each, plus a real end-to-end pipe with live API calls on WSL. Zero regressions attributable to this diff on any platform.
Clean-install proof. Validated against a clean
uv tool installfrom upstream HEAD (6c3fd86), not only the commit these changes were developed against.How this was found. Part of a Windows-native gap investigation. Completeness was declared prematurely several times during that investigation and each premature call was later broken by adversarial re-testing — several fixes exist only because an earlier "this is done" was challenged. Treat this as what was found, not a closed set.