fix(tests): remove hardcoded GNU coreutils dependencies on Windows (#293)

[dracic]

Summary

Two tests depended on GNU coreutils being resolvable on Windows PATH:

• tests/test_runs.py::test_stop_run_signals_live_process spawned subprocess.Popen(["sleep", "30"]).
• tests/test_cleanup.py::_dead_pid() spawned subprocess.Popen(["true"]).

On stock Windows environments without Git\usr\bin on PATH, these failed with FileNotFoundError or exit code 9009 ('true' is not recognized as an internal or external command).

Changes

• Updated test_stop_run_signals_live_process to use sys.executable with import time; time.sleep(30) and added process cleanup in a finally block.
• Updated _dead_pid in tests/test_cleanup.py to spawn sys.executable with "".

The tests/test_verify.py half of the original diff (true/false verify commands) landed on main via #308, so this branch was rebased onto it and now carries only the two Popen call sites.

Fixes #293

Summary by CodeRabbit

• Tests
  • Improved process-related test reliability by using the active Python interpreter instead of platform-specific external commands.
  • Added cleanup safeguards to ensure test processes are terminated and reaped properly.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fd0a59df-bedb-4ec0-a3a1-b37026fb93f2

📥 Commits

Reviewing files that changed from the base of the PR and between 7cdef07 and 6454a6a.

📒 Files selected for processing (2)

• tests/test_cleanup.py
• tests/test_runs.py

💤 Files with no reviewable changes (1)

• tests/test_cleanup.py

---

Walkthrough

Tests replace platform-specific true and sleep commands with Python interpreter subprocesses and add guaranteed cleanup for the live process test.

Changes

Cross-platform test portability

Layer / File(s)	Summary
Portable process lifecycle tests tests/test_cleanup.py, tests/test_runs.py	Cleanup and run-stop tests launch subprocesses through sys.executable; the live-process test force-terminates and waits for any remaining child process.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: pbean

Poem

I’m a rabbit who hops through the test-run night,
With Python-born processes behaving just right.
No shell command can send me astray,
I clean up my burrows before I play.
Cross-platform carrots—hip hip hooray!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the Windows test portability fix and matches the change set.
Linked Issues check	✅ Passed	The PR replaces the GNU sleep/true dependency with sys.executable-based processes and preserves the intended test behavior.
Out of Scope Changes check	✅ Passed	The changes stay focused on the linked Windows test fix and do not introduce unrelated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR replaces GNU coreutils commands (sleep 30 and true) in two test files with cross-platform Python equivalents using sys.executable, fixing FileNotFoundError failures on stock Windows. No production code is changed.

• test_stop_run_signals_live_process in test_runs.py now spawns sys.executable -c "import time; time.sleep(30)" and wraps the body in a try/finally to ensure the subprocess is killed if the test assertion fails or times out.
• _dead_pid() and its now-unused import subprocess are removed from test_cleanup.py; the function already exists in test_runs.py (line 42) with the equivalent sys.executable -c "" form.

Confidence Score: 5/5

Safe to merge — only test helpers are touched; no production code is changed.

Both changes are straightforward cross-platform substitutions. The sleep/true GNU coreutils calls are replaced with sys.executable invocations that work identically on all platforms. The added finally block is a net improvement that prevents zombie test processes. The removed _dead_pid() in test_cleanup.py was genuinely dead code with no callers in that file.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename	Overview
tests/test_runs.py	Replaces subprocess.Popen(["sleep", "30"]) with a cross-platform Python-based sleep subprocess and adds a finally block to clean up the process on test failure
tests/test_cleanup.py	Removes the unused _dead_pid() helper (which relied on true) and its import subprocess statement; the function is not called anywhere in this file

_{Reviews (3): Last reviewed commit: "fix(tests): delete the uncalled _dead_pi..." | Re-trigger Greptile}

[pbean]

Reviewed — approach is right, and I pushed one follow-up commit (6454a6a).

The rebase landed well. You beat me to it; your bb7bfef resolved the tests/test_verify.py conflict correctly — the file is byte-identical to main and #308's _FAIL constant survived. Worth flagging for anyone hitting this shape later: that conflict was the one real hazard here. This branch forked at 5432c81, before ed77c6b ("classify verify env faults per shell", #302) landed the same fix with a named _FAIL in conftest.py plus the comment explaining why POSIX false isn't cmd's failure verb. Resolving it the reflex way — keeping the branch's inline "exit 1" — would have quietly reverted that. It didn't, so no action needed; just noting why that hunk is gone from your commit now.

What I changed: tests/test_cleanup.py::_dead_pid is dead code — it has never had a call site. 62818ed created that file by copying the helper out of test_runs.py, Popen(["true"]) body and all, but never wrote a call; the occurrence count has been 1 (the bare def) from that commit through HEAD, with no cross-file import.

That means it was never part of #293's failure set — an uncalled helper can't fail to resolve true on any PATH. It's also why #31 fixed only the live copy when it added the native-Windows lane, and why three weeks of that lane never flagged this one. Porting it would re-apply #31's fix to code that never runs and re-sync a duplicate of test_runs.py:42, recreating exactly the duplication that let it drift. So I deleted it and its now-unused import subprocess instead — the coreutils dependency goes away either way, and the PR body's claim that this spot failed with FileNotFoundError/9009 doesn't hold for it.

Also bounded the proc.wait() in your new finally to timeout=10, matching every other wait in that file and in test_process_host.py. The finally itself is a good addition — before it, a failed assertion leaked the child for 30s.

The test_runs.py sleeper fix is the real one and is exactly right: sys.executable is the pattern already established at test_runs.py:42, and it's more faithful than sleep since a real engine pid is a Python process.

One thing left, and CI can't do it. All checks green is not evidence for this issue — GitHub's windows-latest ships C:\Program Files\Git\usr\bin on PATH, which is precisely why #293 was invisible to the Windows lane to begin with. Could you post a run of

uv run pytest tests/test_runs.py::test_stop_run_signals_live_process -q

from a shell without Git\usr\bin on PATH, including the duration? Two things I'd like confirmed: that it passes, and how long it takes. WindowsProcessHost.terminate is taskkill /PID without /F and swallows failure (check=False); a console child with no message loop may well refuse it, in which case stop_run burns the full _STOP_WAIT_S = 10.0 grace window before reaching taskkill /F /T. Should still pass — sleep.exe had the same shape, so it's not a regression — but worth knowing before we call the Windows story closed.

Green here: full suite 3111 passed, 30 skipped, trunk check clean.

[pbean]

Verified on native Windows, stock PATH. @pbean ran this on a Windows VM (PowerShell, CPython 3.13 via uv, no Git\usr\bin on PATH) as a before/after ablation — the evidence CI structurally can't produce, since windows-latest carries Git\usr\bin.

Before — main, reproduces #293:

> uv run pytest tests/test_runs.py::test_stop_run_signals_live_process -q
tests\test_runs.py:408:
    proc = subprocess.Popen(["sleep", "30"])
E   FileNotFoundError: [WinError 2] The system cannot find the file specified
1 failed in 0.55s

CreateProcess failing to resolve sleep is also a stronger proof of the stock-PATH precondition than a where.exe check.

After — this branch (6454a6a):

> uv run pytest tests/test_runs.py::test_stop_run_signals_live_process -q --durations=0
1 passed in 11.19s
   10.26s call     tests/test_runs.py::test_stop_run_signals_live_process

Full #293 surface, same shell: 302 passed, 1 skipped in 354.72s
(tests/test_runs.py tests/test_cleanup.py tests/test_verify.py).

So #293 is genuinely closed. 👍

---

One confirmed side-observation, for a follow-up — not this PR.

That 10.26s is not noise; it's _STOP_WAIT_S = 10.0 being burned in full. It means host.is_alive(pid) stayed true for the entire grace window after terminate(), which empirically confirms that WindowsProcessHost.terminate — taskkill /PID without /F, whose failure is swallowed by check=False — cannot close a console child with no message loop. Every stop on native Windows therefore waits 10s and then force-kills via taskkill /F /T.

Not a regression (sleep.exe had the identical shape, so this cost predates the PR) and functionally correct, since stop_run's fallback still marks stopped and kill_session still backstops the agent window. But it does mean the path stop_run's own docstring says it prefers — "the engine's own SIGTERM handler so the engine stays the single writer of stopped" — never actually runs on native Windows; every stop takes the force-kill branch. Worth an issue against the native-Windows tier rather than widening this PR.

[pbean]

Filed the follow-up as #319 (bug, area:engine, needs-design) — native-Windows stop_run always force-killing after the full 10s wait, with the 10.26s measurement as evidence. Out of scope here; this PR stays the two-file test fix.