Files
houren Antony d4f1fbd110 ci: add windows-latest to test matrix + fix 11 cross-platform test bugs (#271)
* ci: add windows-latest to test matrix + fix 11 cross-platform test bugs

The test workflow ran on ``ubuntu-latest`` only. Per the issue's
first bullet — the maintainer's explicit #1 priority — add
``windows-latest`` to the matrix so the manager and related
modules are exercised on Windows on every PR.

The matrix addition surfaces 18 pre-existing Windows-only test
failures. Without fixes the new leg would be 18+ reds from
day one and the matrix would just produce a wall of
``fail-fast`` noise. This PR fixes 11 of them; each fix is
a real (cross-platform) bug, not a Windows-specific hack —
most were already flagged by CodeRabbit on PR #236 but never
acted on. The remaining 4 failures need code refactors
(``os.killpg`` → ``psutil`` in ``background.py``,
``convert_virtual_paths_in_command`` Windows-aware quoting,
tilde expansion) that are documented as out-of-scope
follow-ups below.

## What changed

* ``.github/workflows/test.yml``
  - ``os: [ubuntu-latest, windows-latest]`` → 2 OS × 2 Python
    = 4 cells.
  - ``fail-fast: false`` so one bad cell doesn't cancel the
    rest while the Windows leg is being brought up. Removable
    in a future PR once the suite is fully green.

* ``tests/test_backends.py``
  - Hard-coded ``"python3"`` → ``{sys.executable}`` in 7
    test commands. Windows has no ``python3`` on PATH; using
    ``sys.executable`` is portable and matches what CodeRabbit
    flagged on PR #236.
  - Strict string comparisons → ``shlex.split`` round-trip in
    5 resolver tests. ``shlex.quote`` adds single quotes
    around backslash paths on Windows, which broke the
    direct ``==`` compare.
  - Cross-platform suffix checks in 2 path-resolution tests
    (``Path(resolved).parts[-2:]`` instead of
    ``str(resolved).endswith("src/main.py")``).
  - ``mkdir -p`` → ``sys.executable -c "import os;
    os.makedirs(...)"`` in the cwd-sanitization test.
  - ``skipif(sys.platform == "win32")`` on 3 e2e tests that
    hit the underlying ``shlex.quote`` + ``cmd.exe`` quoting
    bug (real, separate issue).

* ``tests/test_sessions.py``
  - ``test_uses_data_dir``: check ``.evoscientist`` in the
    long path form (via ``Path.resolve()``) rather than the
    short-path form ``get_db_path`` returns on Windows.

* ``tests/test_mcp_client.py``
  - ``endswith("python")`` → ``Path(result).stem.lower()`` so
    ``python.EXE`` matches on Windows.
  - ``endswith("npx")`` also accepts ``npx.cmd`` so the npm
    shim on Windows matches.

## Out of scope (follow-up issues to file)

* ``os.killpg`` doesn't exist on Windows
  (``EvoScientist/background.py:248``) — 3 background tests
  fail. Real fix is the same ``psutil`` walk pattern PR #200
  shipped in ``langgraph_dev/manager.py``.
* Tilde expansion in file mentions.
* Windows-aware shell quoting in
  ``convert_virtual_paths_in_command``.
* Path conventions (``~/.config/evoscientist/`` vs
  ``%APPDATA%\EvoScientist``) — needs design discussion +
  ``platformdirs`` migration.
* Cross-module audit of
  ``EvoScientist/tools/execute.py``,
  ``EvoScientist/ccproxy_manager.py``,
  ``EvoScientist/config/onboard.py``.

Closes #207 (step 1 only — CI matrix + the easy test
fixes; remaining bullets tracked separately).

* fix: cross-platform compatibility for Windows CI runners

- background.py: replace POSIX-only os.killpg/os.getpgid with
  cross-platform _kill_process_tree() helper. On Windows falls back
  to Popen.terminate()/Popen.kill() (TerminateProcess); on POSIX
  keeps existing os.killpg logic.

- test_backends.py: replace mkdir -p shell execution in
  test_literal_workspace_path_replaced with preprocessing-boundary
  assertion (patch LocalShellBackend.execute, capture command,
  assert workspace path was rewritten to ./). Avoids POSIX-only
  mkdir -p on Windows runners.

- test_file_mentions.py: monkeypatch USERPROFILE on Windows so
  ntpath.expanduser() resolves ~ to tmp_path even when HOME is
  unset on CI runners.

* fix(test): cross-platform sleep/true commands for Windows CI

Replace POSIX-only sleep/true with module-level helpers that use
ping -n / cmd /c on Windows. Also fix python3 -> sys.executable
in the non-timeout recovery test.

- test_background.py: 7 sleep/true fixes
- test_background_middleware.py: 6 sleep/true fixes
- test_backends.py: 4 sleep fixes + 1 python3 fix

2318 passed, 0 failed on Windows.

* fix(test): use shell-portable double quotes for python -c on Windows

cmd.exe does not treat single quotes as string delimiters, so
-c 'raise SystemExit(1)' was passed with literal quotes on Windows.
Switch to double quotes which work on both cmd.exe and POSIX sh.

* fix: use psutil for Windows process tree kill + avoid sys.executable under uv

- background.py: replace Popen.terminate()/kill() with psutil-based
  process tree walking on Windows. TerminateProcess does NOT cascade
  to grandchildren; psutil.Process.children(recursive=True) ensures
  the entire tree is signaled.

- test_backends.py: replace sys.executable with 'python' in sandbox
  execute() calls. Under uv, sys.executable is under the workspace
  and gets rewritten to ./ by prepare_sandbox_command, breaking
  Linux CI. The plain 'python' command resolves correctly in any
  activated venv.

* fix: broaden try/except in _kill_process_tree to cover proc.children()

If the process exits between Process(popen.pid) and children(recursive=True),
the children call raises an uncaught exception escaping stop(). Move it inside
the existing try/except block.

* fix: narrow exception to ProcessLookupError in POSIX _kill_process_tree

OSError is too broad — would silently swallow EPERM on SIGKILL, leaving
the process alive when we report it as stopped. Match original behavior
which only caught ProcessLookupError (process already gone).

* style: ruff format test_backends.py

* ci: trigger re-run for flaky prompt_toolkit test

* style: fix ruff check (import order + RUF005 unpacking)

---------

Co-authored-by: Xi Zhang <106144707+X-iZhang@users.noreply.github.com>
2026-06-10 15:43:18 +01:00

335 lines
12 KiB
Python

"""Background OS-process execution for the sandbox.
A *process* here is a single detached OS process launched via ``run_in_background``
(distinct from an async sub-agent *task* and a future cron *schedule* — the word
"job" is intentionally never used).
The registry is **module-global (process-level)**: processes survive ``/new`` and
``/resume`` within the same CLI process, but are not persisted across a CLI restart.
The live ``Popen`` handle is held so ``poll()`` / ``returncode`` stay authoritative
(no PID-reuse risk).
Command validation and cwd resolution happen at the tool layer
(``middleware/background.py``); this module is the pure execution + tracking mechanism
and is safe to unit-test on its own. A future scheduler (cron) would reuse ``launch``.
"""
from __future__ import annotations
import logging
import os
import signal
import subprocess
import threading
import time
import uuid
from collections.abc import Callable
from dataclasses import dataclass, field
from datetime import UTC, datetime
from pathlib import Path
import psutil
logger = logging.getLogger(__name__)
_BG_DIRNAME = ".bg_processes"
_KILL_GRACE_SECONDS = 2.0
@dataclass
class BgProcess:
"""A tracked background OS process."""
process_id: str
name: str
command: str
popen: subprocess.Popen
pid: int
log_path: Path
started_at: str # ISO-8601 UTC (record/display)
started_ts: float # epoch seconds (elapsed computation)
origin_thread_id: str | None = None # CLI thread/session that launched it
returncode: int | None = None
finished_at: str | None = None
finished_ts: float | None = None # epoch at exit; freezes elapsed once done
stopped: bool = False # set by stop(); suppresses the completion notification
# epoch each thread last checked this process (status/list); keyed by thread_id
# so a check from one session can't dedup another session's completion ping.
last_checked_by_thread: dict[str | None, float] = field(default_factory=dict)
_PROCESSES: dict[str, BgProcess] = {}
_LOCK = threading.Lock()
def _now_iso() -> str:
return datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%SZ")
def _record_exit(proc: BgProcess) -> None:
"""Record terminal state on first observed exit. Caller MUST hold ``_LOCK``.
``finished_ts`` is set when the exit is first observed. The per-process daemon
watcher (:func:`_watch`) calls this right after ``popen.wait()`` returns, so in
practice ``finished_ts`` ≈ the real exit time. Calls from ``status`` / ``list_all`` /
``stop`` are a fallback for the brief window before the watcher runs.
"""
rc = proc.popen.poll()
if rc is not None and proc.returncode is None:
proc.returncode = rc
proc.finished_at = _now_iso()
proc.finished_ts = time.time()
def _elapsed(proc: BgProcess) -> int:
"""Seconds the process has run — frozen at first-observed exit once it has exited."""
end = proc.finished_ts if proc.finished_ts is not None else time.time()
return int(end - proc.started_ts)
def was_observed_done(process_id: str, origin_thread_id: str | None = None) -> bool:
"""True if ``origin_thread_id`` already saw this process's completion itself.
i.e. the process has exited AND was checked (``status``/``list_all``) from that thread
at or after it finished. Used to dedup the completion notification (routed to the
launching thread), so a check from a *different* session can't suppress it.
"""
with _LOCK:
proc = _PROCESSES.get(process_id)
if proc is None or proc.finished_ts is None:
return False
seen_ts = proc.last_checked_by_thread.get(origin_thread_id)
return seen_ts is not None and seen_ts >= proc.finished_ts
def _read_tail(log_path: Path, tail_bytes: int) -> str:
# Seek from the end so a huge log isn't fully read into memory on each status check.
try:
with log_path.open("rb") as f:
f.seek(0, os.SEEK_END)
size = f.tell()
if size == 0:
return "(no output yet)"
if size > tail_bytes:
f.seek(-tail_bytes, os.SEEK_END)
return "...(truncated)...\n" + f.read().decode("utf-8", "replace")
f.seek(0)
data = f.read()
except OSError:
return "(no output captured yet)"
return data.decode("utf-8", "replace")
def _watch(proc: BgProcess, on_exit: Callable[[BgProcess], None] | None) -> None:
"""Block until ``proc`` exits, record the exit promptly, then fire ``on_exit``.
Running in a daemon thread, ``popen.wait()`` lets us record ``finished_ts`` at (very
close to) the real exit time — fixing the observation-time inflation — and gives a
hook the CLI layer wires to a completion notification, without ``background.py``
importing the notifier (kept decoupled via the callback).
"""
try:
proc.popen.wait()
except Exception:
pass
with _LOCK:
_record_exit(proc)
if on_exit is not None:
try:
on_exit(proc)
except Exception:
logger.warning("background on_exit callback failed", exc_info=True)
def launch(
command: str,
cwd: str,
name: str | None = None,
*,
origin_thread_id: str | None = None,
on_exit: Callable[[BgProcess], None] | None = None,
) -> str:
"""Launch ``command`` detached in ``cwd``; return a short ``process_id``.
The command is run via ``shell=True`` with output redirected to a per-process log
file under ``<cwd>/.bg_processes/`` and ``start_new_session=True`` so the child is a
process-group leader (survives this call's return and can be killed as a group).
The caller is responsible for validating ``command`` first.
``origin_thread_id`` records the launching CLI session so ``list_all`` can scope to it.
``on_exit`` (optional) is called with the ``BgProcess`` from a daemon watcher thread
once the process exits — used by the CLI layer to emit a completion notification.
"""
process_id = uuid.uuid4().hex[:8]
log_dir = Path(cwd) / _BG_DIRNAME
log_dir.mkdir(parents=True, exist_ok=True)
log_path = log_dir / f"{process_id}.log"
log_file = open(log_path, "w")
try:
popen = subprocess.Popen(
command,
shell=True,
cwd=cwd,
stdout=log_file,
stderr=subprocess.STDOUT,
stdin=subprocess.DEVNULL,
start_new_session=True,
)
finally:
# The child inherited its own dup of the fd during spawn; the parent's copy
# is no longer needed (and must be closed so the pipe/file isn't held open).
log_file.close()
proc = BgProcess(
process_id=process_id,
name=name or command[:40],
command=command,
popen=popen,
pid=popen.pid,
log_path=log_path,
started_at=_now_iso(),
started_ts=time.time(),
origin_thread_id=origin_thread_id,
)
with _LOCK:
_PROCESSES[process_id] = proc
# Daemon watcher: records the precise exit time and fires on_exit when done.
threading.Thread(target=_watch, args=(proc, on_exit), daemon=True).start()
return process_id
def status(
process_id: str, *, thread_id: str | None = None, tail_bytes: int = 16_000
) -> str:
"""Return a human-readable status + recent output tail for ``process_id``."""
with _LOCK:
proc = _PROCESSES.get(process_id)
if proc is None:
return (
f"No such background process: {process_id!r}. "
"Use list_processes to see tracked processes."
)
_record_exit(proc)
proc.last_checked_by_thread[thread_id] = time.time() # this thread observed it
running = proc.returncode is None
elapsed = _elapsed(proc)
name, pid, command, returncode, log_path = (
proc.name,
proc.pid,
proc.command,
proc.returncode,
proc.log_path,
)
if running:
head = f"Process {process_id} (name={name!r}) RUNNING — {elapsed}s elapsed, pid {pid}."
else:
head = f"Process {process_id} (name={name!r}) EXITED code {returncode} after ~{elapsed}s."
tail = _read_tail(log_path, tail_bytes) # file IO outside the lock
return (
f"{head}\nCommand: {command}\n--- output (last {tail_bytes} bytes) ---\n{tail}"
)
def _kill_process_tree(popen: subprocess.Popen, *, forceful: bool) -> None:
"""Kill the process group/tree in a cross-platform way.
On POSIX ``start_new_session=True`` makes the child a process-group
leader; ``os.killpg`` terminates the entire group (shell + any
grandchildren). On Windows ``TerminateProcess`` (used by
``Popen.terminate()`` / ``Popen.kill()``) only kills the direct
child — it does *not* cascade to grandchildren. We use ``psutil``
to walk the process tree and signal every descendant.
"""
if os.name == "nt":
try:
proc = psutil.Process(popen.pid)
targets = [proc, *proc.children(recursive=True)]
except (psutil.NoSuchProcess, psutil.AccessDenied):
return
for p in targets:
try:
if forceful:
p.kill()
else:
p.terminate()
except (psutil.NoSuchProcess, psutil.AccessDenied):
pass
else:
sig = signal.SIGKILL if forceful else signal.SIGTERM
try:
os.killpg(os.getpgid(popen.pid), sig)
except ProcessLookupError:
pass
def stop(process_id: str) -> str:
"""Terminate ``process_id`` and its process group (SIGTERM, then SIGKILL)."""
with _LOCK:
proc = _PROCESSES.get(process_id)
if proc is None:
return f"No such background process: {process_id!r}."
if proc.popen.poll() is not None:
_record_exit(proc)
return f"Process {process_id} already finished (code {proc.returncode})."
# Mark as user-stopped so the watcher's on_exit suppresses the completion
# notification (the user already knows — no need to ping them).
proc.stopped = True
# The watcher's popen.wait() reaps without the lock, so a tiny PID-reuse race
# remains (getpgid on a recycled pid). On POSIX ProcessLookupError covers the
# common case; on Windows ``Popen.terminate()`` is a no-op on a dead handle
# so we poll after the call instead.
_kill_process_tree(proc.popen, forceful=False)
if proc.popen.poll() is not None:
_record_exit(proc)
return f"Process {process_id} is no longer running."
deadline = time.time() + _KILL_GRACE_SECONDS
while time.time() < deadline:
with _LOCK:
if proc.popen.poll() is not None:
_record_exit(proc)
break
time.sleep(0.1)
else:
with _LOCK:
if proc.popen.poll() is None:
_kill_process_tree(proc.popen, forceful=True)
_record_exit(proc)
with _LOCK:
_record_exit(proc)
name = proc.name
return f"Stopped background process {process_id} (name={name!r})."
def list_all(thread_id: str | None = None, *, include_all: bool = False) -> str:
"""List tracked background processes with live statuses.
Scoped to the launching session (``thread_id``) unless ``include_all`` is set.
"""
with _LOCK:
all_procs = list(_PROCESSES.values())
procs = (
all_procs
if include_all
else [p for p in all_procs if p.origin_thread_id == thread_id]
)
if not procs:
if all_procs and not include_all:
return (
"No background processes in this session "
f"({len(all_procs)} in other sessions — pass all_threads=True to see them)."
)
return "No background processes tracked."
lines = []
now = time.time()
for p in procs:
_record_exit(p)
p.last_checked_by_thread[thread_id] = now # this thread observed it
state = "RUNNING" if p.returncode is None else f"exited({p.returncode})"
lines.append(
f" {p.process_id} {state:12} {_elapsed(p)}s name={p.name!r}"
)
return f"{len(procs)} background process(es):\n" + "\n".join(lines)