From 007f51e9b513f322b82e6784b51f851e06b8082e Mon Sep 17 00:00:00 2001 From: Lukas Geiger Date: Sat, 26 Sep 2026 14:59:54 +0200 Subject: [PATCH 1/4] feat: add optional, Windows-gated zombie-killer-tray integration T-20260926-368033290 (c) / T-20260926-212716751: adds an OPTIONAL integration with zombie-killer-tray (conservative cleanup of orphaned MCP and language-server processes), mirroring the pattern already used by dev-bricks/CareCenter-for-Codex for this same tool -- a git-pinned pip dependency, launched as its own subprocess via `python -m zombie_killer_tray watch ...`, coordinated through the shared zombie_events.jsonl audit log, never an in-process import. Safe Start for Codex is explicitly cross-platform (Windows/Linux/macOS, zero required runtime dependencies); zombie-killer-tray is Win32-only. This integration is therefore doubly gated to keep that intact: - New `[project.optional-dependencies].zombie-killer` extra (never installed implicitly), itself carrying a PEP 508 `sys_platform == 'win32'` marker so `pip install .[zombie-killer]` can't even attempt to resolve zombie-killer-tray's own Win32 dependencies on Linux/macOS. - `launch_zombie_killer_watch()`/`build_zombie_killer_status()` check a small `_is_windows()` seam and refuse/report cleanly on any other platform rather than attempting a doomed subprocess spawn. (Deliberately NOT monkeypatching `os.name` directly in tests for this -- doing so on an actual Windows test runner breaks pathlib's own class dispatch for any Path() constructed afterwards; verified this the hard way before introducing the `_is_windows()` seam.) New `zombie_killer_integration.py`: target resolution (local sibling checkout override via `SAFE_START_ZOMBIE_KILLER_SOURCE`, else the pinned GitHub spec), install/launch/status functions. Three new CLI subcommands in cli.py mirroring the rest of its subparser style: `zombie-killer-report` / `-install` / `-watch`. Updated THIRD_PARTY_LICENSES.txt/.md and tests/test_third_party_licenses.py's exact-dependency-set assertion to cover the new optional dependency. Pinned to dev-bricks/zombie-killer-tray@6c8eb2c (that repo's PR #4, src/-packaging, not yet merged -- move this pin to the merge commit once it lands, same as the CareCenter-for-Codex integration PR). 139/139 tests green (128 baseline + 11 new), ruff clean. Not merging -- per the two-model rule, a different model/session should review before merge. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01PTbvD41MCVmnQWaobvHfCk --- THIRD_PARTY_LICENSES.md | 1 + THIRD_PARTY_LICENSES.txt | 4 + pyproject.toml | 8 + src/safe_start_for_codex/cli.py | 67 ++++ .../zombie_killer_integration.py | 367 ++++++++++++++++++ tests/test_third_party_licenses.py | 1 + tests/test_zombie_killer_integration.py | 158 ++++++++ 7 files changed, 606 insertions(+) create mode 100644 src/safe_start_for_codex/zombie_killer_integration.py create mode 100644 tests/test_zombie_killer_integration.py diff --git a/THIRD_PARTY_LICENSES.md b/THIRD_PARTY_LICENSES.md index 9319778..ee019fd 100644 --- a/THIRD_PARTY_LICENSES.md +++ b/THIRD_PARTY_LICENSES.md @@ -57,6 +57,7 @@ The core runtime of `safe-start-for-codex` has **zero external runtime dependenc | **pytest** | `>=9.1.1` | Automated test runner, contract verification suites, mock fixtures | [MIT](https://github.com/pytest-dev/pytest/blob/main/LICENSE) | [pytest-dev/pytest](https://github.com/pytest-dev/pytest) | | **ruff** | `>=0.5.0` | High-performance Python linter and code formatting enforcement | [MIT / Apache-2.0](https://github.com/astral-sh/ruff/blob/main/LICENSE-MIT) | [astral-sh/ruff](https://github.com/astral-sh/ruff) | | **PyInstaller** | `>=6.0` | Optional standalone Windows executable builder (`build_exe.bat`) | [GPLv2-or-later with Special Exception](https://github.com/pyinstaller/pyinstaller/blob/develop/COPYING.txt) | [pyinstaller/pyinstaller](https://github.com/pyinstaller/pyinstaller) | +| **zombie-killer-tray** | commit `6c8eb2c` | Optional, Windows-only companion for orphaned MCP/language-server process cleanup, launched as its own subprocess | [MIT](https://github.com/dev-bricks/zombie-killer-tray/blob/main/LICENSE) | [dev-bricks/zombie-killer-tray](https://github.com/dev-bricks/zombie-killer-tray) | --- diff --git a/THIRD_PARTY_LICENSES.txt b/THIRD_PARTY_LICENSES.txt index 3a075f7..e8c0531 100644 --- a/THIRD_PARTY_LICENSES.txt +++ b/THIRD_PARTY_LICENSES.txt @@ -27,6 +27,7 @@ The base package declares no external runtime dependencies in | pytest | Development tests | `>=9.1.1` | 9.1.1 | MIT | https://pypi.org/project/pytest/ | | ruff | Development linter | `>=0.5.0` | 0.6.2 | MIT / Apache-2.0 | https://pypi.org/project/ruff/ | | PyInstaller | Optional Windows EXE build | `>=6.0` | 6.21.0 | GPLv2-or-later with PyInstaller's special exception | https://pypi.org/project/pyinstaller/ | +| zombie-killer-tray | Optional, Windows-only integration | commit `6c8eb2cc8773e662c65e7a5c39d060b974805c3d` | 0.1.0 | MIT | https://github.com/dev-bricks/zombie-killer-tray | ## Transitive Build & Test Inventory @@ -49,5 +50,8 @@ The base package declares no external runtime dependencies in - The optional `build` extra uses PyInstaller. PyInstaller metadata states a GPLv2-or-later license with a special exception that allows building and distributing non-free applications. +- The optional `zombie-killer` extra installs `zombie-killer-tray`, guarded by + a `sys_platform == 'win32'` environment marker so it is never attempted on + Linux/macOS even if that extra is requested there. - Binary release preparation should still inspect the actual bundled wheel set and include the license files from bundled distributions. diff --git a/pyproject.toml b/pyproject.toml index ba4b9c3..e1e12b3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -69,6 +69,14 @@ dev = [ build = [ "pyinstaller>=6.0" ] +# T-20260926-212716751: optional, Windows-only companion tool (conservative +# cleanup of orphaned MCP/language-server processes). Never installed +# implicitly -- only pulled in if a caller explicitly requests this extra. +# The environment marker also protects `pip install .[zombie-killer]` on a +# non-Windows machine from failing on zombie-killer-tray's own Win32 deps. +zombie-killer = [ + "zombie-killer-tray @ git+https://github.com/dev-bricks/zombie-killer-tray.git@6c8eb2cc8773e662c65e7a5c39d060b974805c3d ; sys_platform == 'win32'" +] [project.scripts] safe-start-for-codex = "safe_start_for_codex.cli:main" diff --git a/src/safe_start_for_codex/cli.py b/src/safe_start_for_codex/cli.py index c5d349c..0386da7 100644 --- a/src/safe_start_for_codex/cli.py +++ b/src/safe_start_for_codex/cli.py @@ -1730,6 +1730,46 @@ def setup(_icon: pystray.Icon) -> None: return 0 +def command_zombie_killer_report(args: argparse.Namespace) -> int: + from .zombie_killer_integration import build_zombie_killer_status + + status = build_zombie_killer_status() + if args.json: + print(json.dumps(status.to_dict(), ensure_ascii=False, indent=2)) + else: + print(status.to_text()) + return 0 + + +def command_zombie_killer_install(args: argparse.Namespace) -> int: + from .zombie_killer_integration import install_zombie_killer_package + + result = install_zombie_killer_package(target=args.target) + if args.json: + print(json.dumps(result.to_dict(), ensure_ascii=False, indent=2)) + else: + print(result.to_text()) + return 0 if result.status == "ok" else 1 + + +def command_zombie_killer_watch(args: argparse.Namespace) -> int: + from .zombie_killer_integration import ( + DEFAULT_MIN_AGE_SECONDS, + DEFAULT_WATCH_INTERVAL_SECONDS, + launch_zombie_killer_watch, + ) + + result = launch_zombie_killer_watch( + interval_seconds=args.interval or DEFAULT_WATCH_INTERVAL_SECONDS, + min_age_seconds=args.min_age or DEFAULT_MIN_AGE_SECONDS, + ) + if args.json: + print(json.dumps(result.to_dict(), ensure_ascii=False, indent=2)) + else: + print(result.to_text()) + return 0 if result.status == "ok" else 1 + + def command_status(_: argparse.Namespace) -> int: latest = state_dir() / "latest.json" if not latest.exists(): @@ -1923,6 +1963,33 @@ def build_parser() -> argparse.ArgumentParser: backup = sub.add_parser("backup", help="Create a manual backup of automation TOML files.") backup.set_defaults(func=command_backup) + + zombie_killer_report = sub.add_parser( + "zombie-killer-report", + help="Check zombie-killer-tray status (last cleanup cycle). Optional, Windows-only integration.", + ) + zombie_killer_report.add_argument("--json", action="store_true") + zombie_killer_report.set_defaults(func=command_zombie_killer_report) + + zombie_killer_install = sub.add_parser( + "zombie-killer-install", + help="Install or upgrade zombie-killer-tray.", + ) + zombie_killer_install.add_argument( + "--target", default=None, + help="Optional pip target. Default: local sibling checkout, else the commit-pinned GitHub source.", + ) + zombie_killer_install.add_argument("--json", action="store_true") + zombie_killer_install.set_defaults(func=command_zombie_killer_install) + + zombie_killer_watch = sub.add_parser( + "zombie-killer-watch", + help="Launch zombie-killer-tray as its own watch subprocess (orphaned MCP/language-server processes). Windows-only.", + ) + zombie_killer_watch.add_argument("--interval", type=int, default=None) + zombie_killer_watch.add_argument("--min-age", type=int, default=None) + zombie_killer_watch.add_argument("--json", action="store_true") + zombie_killer_watch.set_defaults(func=command_zombie_killer_watch) return parser diff --git a/src/safe_start_for_codex/zombie_killer_integration.py b/src/safe_start_for_codex/zombie_killer_integration.py new file mode 100644 index 0000000..e845d7c --- /dev/null +++ b/src/safe_start_for_codex/zombie_killer_integration.py @@ -0,0 +1,367 @@ +"""Optional zombie-killer-tray integration for Safe Start (T-20260926-212716751). + +Safe Start for Codex is cross-platform (Windows/Linux/macOS) with zero +required runtime dependencies. zombie-killer-tray is Windows-only (Win32 +process APIs). This integration therefore stays entirely optional and +Windows-gated: nothing here is imported or run unless a caller explicitly +asks for it, and `launch_zombie_killer_watch()` refuses cleanly on any +non-Windows platform instead of attempting anything. + +Same pattern as `dev-bricks/CareCenter-for-Codex`'s existing +`safe_start_integration.py`/`zombie_killer_integration.py`: a git-pinned pip +dependency, launched as its own subprocess via +`python -m zombie_killer_tray watch ...`, coordinated through the shared +zombie_events.jsonl audit log -- never an in-process import. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +from collections.abc import Callable +from dataclasses import asdict, dataclass +from pathlib import Path + +from .cli import codex_home, no_window_kwargs + +# T-20260926-212716751: zombie-killer-tray#4 (src/-Paketierung) is pushed but +# not yet merged -- this pin points at the branch head. Move it to the merge +# commit once #4 lands (same verification step as any other pinned dep here). +ZOMBIE_KILLER_PACKAGE_SPEC = ( + "zombie-killer-tray @ " + "git+https://github.com/dev-bricks/zombie-killer-tray.git" + "@6c8eb2cc8773e662c65e7a5c39d060b974805c3d" +) +ZOMBIE_KILLER_SOURCE_ENV = "SAFE_START_ZOMBIE_KILLER_SOURCE" +DEFAULT_WATCH_INTERVAL_SECONDS = 600 +DEFAULT_MIN_AGE_SECONDS = 1800 + + +@dataclass(slots=True) +class ZombieKillerInstallResult: + status: str + target: str + command: list[str] + message: str + stdout: str = "" + stderr: str = "" + + def to_dict(self) -> dict[str, object]: + return asdict(self) + + def to_text(self) -> str: + lines = [ + f"Status: {self.status}", + f"Target: {self.target}", + "Command: " + " ".join(self.command), + self.message, + ] + if self.stdout.strip(): + lines.append("Output:") + lines.append(self.stdout.strip()) + if self.stderr.strip(): + lines.append("Error output:") + lines.append(self.stderr.strip()) + return "\n".join(lines) + + +@dataclass(slots=True) +class ZombieKillerLaunchResult: + status: str + command: list[str] + message: str + state_dir: str + pid: int | None = None + + def to_dict(self) -> dict[str, object]: + return asdict(self) + + def to_text(self) -> str: + lines = [f"Status: {self.status}", "Command: " + " ".join(self.command), self.message, + f"State dir: {self.state_dir}"] + if self.pid is not None: + lines.append(f"PID: {self.pid}") + return "\n".join(lines) + + +@dataclass(slots=True) +class ZombieKillerStatus: + available: bool + supported_platform: bool + state_dir: str + last_cycle_at: float | None + last_cycle_count: int | None + notes: list[str] + + def to_dict(self) -> dict[str, object]: + return asdict(self) + + def to_text(self) -> str: + availability = "installed" if self.available else "not installed" + lines = [f"zombie-killer-tray: {availability}", f"State dir: {self.state_dir}"] + if self.last_cycle_at is not None: + lines.append(f"Last cycle: {self.last_cycle_at} (reaped: {self.last_cycle_count})") + if self.notes: + lines.append("Notes:") + lines.extend(f"- {note}" for note in self.notes) + return "\n".join(lines) + + +def _is_windows() -> bool: + """Testable seam for the platform gate -- monkeypatching `os.name` itself + would break pathlib's own Windows/Posix class dispatch mid-test.""" + return os.name == "nt" + + +def zombie_killer_state_dir() -> Path: + """zombie-killer-tray's own working directory, under CODEX_HOME. + + zombie-killer-tray writes its runtime state (`zombie_events.jsonl`, + `zombie_worker_errors.log`) to its current working directory, not to a + hardcoded path -- this is passed as `cwd=` to the watch subprocess + (see `launch_zombie_killer_watch`). + """ + path = codex_home() / "zombie-killer-tray" + path.mkdir(parents=True, exist_ok=True) + return path + + +def _zombie_killer_importable() -> bool: + try: + __import__("zombie_killer_tray.killer") + except Exception: + return False + return True + + +def _local_zombie_killer_source() -> Path | None: + env_path = os.environ.get(ZOMBIE_KILLER_SOURCE_ENV) + if env_path: + candidate = Path(env_path).expanduser() + if (candidate / "pyproject.toml").exists(): + return candidate + + project_root = Path(__file__).resolve().parents[2] + sibling = project_root.parent / "REL-PUB_zombie-killer-tray" + if (sibling / "pyproject.toml").exists(): + return sibling + return None + + +def zombie_killer_install_target() -> str: + """Prefer a local sibling checkout, otherwise the commit-pinned GitHub source.""" + local_source = _local_zombie_killer_source() + if local_source is not None: + return str(local_source) + return ZOMBIE_KILLER_PACKAGE_SPEC + + +def _pip_command_candidates() -> list[list[str]]: + candidates: list[list[str]] = [] + if not getattr(sys, "frozen", False): + candidates.append([sys.executable, "-m", "pip"]) + candidates.extend((["py", "-3", "-m", "pip"], ["python3", "-m", "pip"], ["python", "-m", "pip"])) + + unique: list[list[str]] = [] + seen: set[tuple[str, ...]] = set() + for candidate in candidates: + key = tuple(candidate) + if key not in seen: + unique.append(candidate) + seen.add(key) + return unique + + +def _python_command_candidates() -> list[list[str]]: + candidates: list[list[str]] = [] + if not getattr(sys, "frozen", False): + candidates.append([sys.executable]) + candidates.extend((["py", "-3"], ["python3"], ["python"])) + + unique: list[list[str]] = [] + seen: set[tuple[str, ...]] = set() + for candidate in candidates: + key = tuple(candidate) + if key not in seen: + unique.append(candidate) + seen.add(key) + return unique + + +def _run_install_command(command: list[str]) -> subprocess.CompletedProcess[str]: + return subprocess.run( + command, + check=False, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + ) + + +def install_zombie_killer_package( + *, + target: str | None = None, + runner: Callable[[list[str]], subprocess.CompletedProcess[str]] | None = None, +) -> ZombieKillerInstallResult: + """Install or upgrade zombie-killer-tray via pip -- an explicit user action. + + Callable on any platform (pip will simply fail on a non-Windows box, + since zombie-killer-tray itself requires Win32); `launch_zombie_killer_watch` + is where the platform gate that actually matters lives. + """ + chosen_target = target or zombie_killer_install_target() + run = runner or _run_install_command + attempts: list[ZombieKillerInstallResult] = [] + for pip_command in _pip_command_candidates(): + command = [*pip_command, "install", "--upgrade", chosen_target] + try: + completed = run(command) + except OSError as exc: + attempts.append( + ZombieKillerInstallResult( + status="failed", target=chosen_target, command=command, message=str(exc) + ) + ) + continue + status = "ok" if completed.returncode == 0 else "failed" + result = ZombieKillerInstallResult( + status=status, + target=chosen_target, + command=command, + message=( + "zombie-killer-tray was installed or upgraded." + if status == "ok" + else f"pip exited with code {completed.returncode}." + ), + stdout=completed.stdout or "", + stderr=completed.stderr or "", + ) + if status == "ok": + return result + attempts.append(result) + + if attempts: + last = attempts[-1] + return ZombieKillerInstallResult( + status="failed", + target=chosen_target, + command=last.command, + message="zombie-killer-tray could not be installed.", + stdout=last.stdout, + stderr=last.stderr or last.message, + ) + return ZombieKillerInstallResult( + status="failed", target=chosen_target, command=[], message="No Python/pip command found." + ) + + +def _zombie_killer_env() -> dict[str, str]: + env = os.environ.copy() + local_source = _local_zombie_killer_source() + if local_source is not None: + src = str(local_source / "src") + old_pythonpath = env.get("PYTHONPATH") + env["PYTHONPATH"] = src if not old_pythonpath else src + os.pathsep + old_pythonpath + return env + + +def launch_zombie_killer_watch( + *, + interval_seconds: int = DEFAULT_WATCH_INTERVAL_SECONDS, + min_age_seconds: int = DEFAULT_MIN_AGE_SECONDS, + popen: Callable[..., subprocess.Popen[str]] | None = None, +) -> ZombieKillerLaunchResult: + """Start `python -m zombie_killer_tray watch` as its own subprocess. + + Refuses cleanly on any non-Windows platform (zombie-killer-tray is + Win32-only) rather than attempting a doomed subprocess spawn. + """ + state_dir = zombie_killer_state_dir() + if not _is_windows(): + return ZombieKillerLaunchResult( + status="unsupported-platform", + command=[], + message="zombie-killer-tray is Windows-only; skipped on this platform.", + state_dir=str(state_dir), + ) + + args = [ + "watch", "--yes", + "--interval", str(interval_seconds), + "--min-age", str(min_age_seconds), + "--parent-pid", str(os.getpid()), + ] + env = _zombie_killer_env() + run = popen or subprocess.Popen + last_error = "" + last_command: list[str] = [] + + for python_command in _python_command_candidates(): + command = [*python_command, "-m", "zombie_killer_tray", *args] + last_command = command + try: + process = run(command, cwd=str(state_dir), env=env, close_fds=True, **no_window_kwargs()) + except OSError as exc: + last_error = str(exc) + continue + pid = getattr(process, "pid", None) + return ZombieKillerLaunchResult( + status="ok", + command=command, + message="zombie-killer-tray was started as its own watch subprocess.", + state_dir=str(state_dir), + pid=int(pid) if isinstance(pid, int) else None, + ) + + return ZombieKillerLaunchResult( + status="failed", + command=last_command, + message=last_error or "No Python command found for zombie-killer-tray.", + state_dir=str(state_dir), + ) + + +def _last_cycle_event(state_dir: Path) -> dict[str, object] | None: + events_path = state_dir / "zombie_events.jsonl" + if not events_path.exists(): + return None + last: dict[str, object] | None = None + try: + with events_path.open("r", encoding="utf-8", errors="replace") as handle: + for line in handle: + line = line.strip() + if not line: + continue + try: + record = json.loads(line) + except json.JSONDecodeError: + continue + if isinstance(record, dict) and "cycle_at" in record: + last = record + except OSError: + return None + return last + + +def build_zombie_killer_status() -> ZombieKillerStatus: + state_dir = zombie_killer_state_dir() + notes: list[str] = [] + supported = _is_windows() + if not supported: + notes.append("zombie-killer-tray is Windows-only; not applicable on this platform.") + available = supported and _zombie_killer_importable() + if supported and not available: + notes.append("zombie-killer-tray package not importable; showing prior audit log if any.") + + last_cycle = _last_cycle_event(state_dir) + return ZombieKillerStatus( + available=available, + supported_platform=supported, + state_dir=str(state_dir), + last_cycle_at=float(last_cycle["cycle_at"]) if last_cycle else None, + last_cycle_count=int(last_cycle["count"]) if last_cycle and "count" in last_cycle else None, + notes=notes, + ) diff --git a/tests/test_third_party_licenses.py b/tests/test_third_party_licenses.py index 19f4497..113f6a1 100644 --- a/tests/test_third_party_licenses.py +++ b/tests/test_third_party_licenses.py @@ -37,6 +37,7 @@ def test_third_party_license_inventory_covers_direct_dependencies() -> None: "pystray", "pytest", "ruff", + "zombie-killer-tray", } for package_name in _declared_direct_dependencies(): assert package_name in text diff --git a/tests/test_zombie_killer_integration.py b/tests/test_zombie_killer_integration.py new file mode 100644 index 0000000..97e3226 --- /dev/null +++ b/tests/test_zombie_killer_integration.py @@ -0,0 +1,158 @@ +from __future__ import annotations + +import json +import subprocess +from pathlib import Path +from types import SimpleNamespace + +from safe_start_for_codex.zombie_killer_integration import ( + ZOMBIE_KILLER_PACKAGE_SPEC, + ZOMBIE_KILLER_SOURCE_ENV, + build_zombie_killer_status, + install_zombie_killer_package, + launch_zombie_killer_watch, + zombie_killer_install_target, + zombie_killer_state_dir, +) + + +def test_state_dir_is_under_codex_home(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + state_dir = zombie_killer_state_dir() + assert state_dir == tmp_path / ".codex" / "zombie-killer-tray" + assert state_dir.is_dir(), "must create the directory" + + +def test_install_target_falls_back_to_pinned_github_spec_without_local_source( + monkeypatch, +) -> None: + monkeypatch.delenv(ZOMBIE_KILLER_SOURCE_ENV, raising=False) + monkeypatch.setattr( + "safe_start_for_codex.zombie_killer_integration._local_zombie_killer_source", + lambda: None, + ) + assert zombie_killer_install_target() == ZOMBIE_KILLER_PACKAGE_SPEC + + +def test_install_target_prefers_local_source_env_override(tmp_path: Path, monkeypatch) -> None: + local = tmp_path / "local-zkt" + local.mkdir() + (local / "pyproject.toml").write_text("[project]\nname='x'\n", encoding="utf-8") + monkeypatch.setenv(ZOMBIE_KILLER_SOURCE_ENV, str(local)) + assert zombie_killer_install_target() == str(local) + + +def test_install_zombie_killer_package_reports_ok_on_zero_exit() -> None: + calls: list[list[str]] = [] + + def fake_runner(command: list[str]) -> subprocess.CompletedProcess[str]: + calls.append(command) + return subprocess.CompletedProcess(command, returncode=0, stdout="installed", stderr="") + + result = install_zombie_killer_package(target="some-target", runner=fake_runner) + + assert result.status == "ok" + assert calls[0][-3:] == ["install", "--upgrade", "some-target"] + + +def test_install_zombie_killer_package_reports_failed_on_nonzero_exit() -> None: + def fake_runner(command: list[str]) -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess(command, returncode=1, stdout="", stderr="boom") + + result = install_zombie_killer_package(target="some-target", runner=fake_runner) + + assert result.status == "failed" + assert "boom" in result.stderr + + +def test_launch_refuses_cleanly_on_non_windows(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + monkeypatch.setattr( + "safe_start_for_codex.zombie_killer_integration._is_windows", lambda: False + ) + + called = False + + def fake_popen(command, **kwargs): + nonlocal called + called = True + return SimpleNamespace(pid=1) + + result = launch_zombie_killer_watch(popen=fake_popen) + + assert result.status == "unsupported-platform" + assert called is False, "must never attempt to spawn on a non-Windows platform" + + +def test_launch_invokes_module_with_expected_args_on_windows(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + captured: dict[str, object] = {} + + def fake_popen(command, **kwargs): + captured["command"] = command + captured["cwd"] = kwargs.get("cwd") + return SimpleNamespace(pid=4242) + + result = launch_zombie_killer_watch(interval_seconds=42, min_age_seconds=99, popen=fake_popen) + + assert result.status == "ok" + assert result.pid == 4242 + command = captured["command"] + assert "-m" in command + assert "zombie_killer_tray" in command + assert "watch" in command + assert "--interval" in command and "42" in command + assert "--min-age" in command and "99" in command + assert "--parent-pid" in command + assert "--yes" in command + assert captured["cwd"] == str(tmp_path / ".codex" / "zombie-killer-tray") + + +def test_launch_reports_failure_when_no_python_found(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + + def failing_popen(command, **kwargs): + raise OSError("no interpreter") + + result = launch_zombie_killer_watch(popen=failing_popen) + + assert result.status == "failed" + assert "no interpreter" in result.message + + +def test_build_status_reads_last_cycle_from_events_log(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + state_dir = zombie_killer_state_dir() + events = state_dir / "zombie_events.jsonl" + with events.open("w", encoding="utf-8") as handle: + handle.write(json.dumps({"cycle_at": 111.0, "apply": False, "count": 0}) + "\n") + handle.write(json.dumps({"cycle_at": 222.0, "apply": True, "count": 3}) + "\n") + handle.write(json.dumps({"event": "broker-superseded-idle", "root_pid": 5}) + "\n") + + status = build_zombie_killer_status() + + assert status.last_cycle_at == 222.0 + assert status.last_cycle_count == 3 + assert status.state_dir == str(state_dir) + + +def test_build_status_handles_missing_log(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + + status = build_zombie_killer_status() + + assert status.last_cycle_at is None + assert status.last_cycle_count is None + + +def test_build_status_marks_non_windows_as_unsupported(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + monkeypatch.setattr( + "safe_start_for_codex.zombie_killer_integration._is_windows", lambda: False + ) + + status = build_zombie_killer_status() + + assert status.supported_platform is False + assert status.available is False + assert any("Windows-only" in note for note in status.notes) From f39a504a0850746be807a58673c4b705396cf238 Mon Sep 17 00:00:00 2001 From: Lukas Geiger Date: Sat, 26 Sep 2026 15:25:42 +0200 Subject: [PATCH 2/4] chore: move zombie-killer-tray pin to its merged commit zombie-killer-tray#4 (src/-Paketierung) merged as 039b4f2. Updates the pinned dependency in pyproject.toml's zombie-killer extra, the module-level ZOMBIE_KILLER_PACKAGE_SPEC constant, and both THIRD_PARTY_LICENSES entries from the pre-merge branch head (6c8eb2c) to the actual merge commit. Verified: `import zombie_killer_tray.killer` resolves against this pin in a clean venv. 139/139 tests green, ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01PTbvD41MCVmnQWaobvHfCk --- THIRD_PARTY_LICENSES.md | 2 +- THIRD_PARTY_LICENSES.txt | 2 +- pyproject.toml | 2 +- src/safe_start_for_codex/zombie_killer_integration.py | 6 ++---- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/THIRD_PARTY_LICENSES.md b/THIRD_PARTY_LICENSES.md index ee019fd..2b4443c 100644 --- a/THIRD_PARTY_LICENSES.md +++ b/THIRD_PARTY_LICENSES.md @@ -57,7 +57,7 @@ The core runtime of `safe-start-for-codex` has **zero external runtime dependenc | **pytest** | `>=9.1.1` | Automated test runner, contract verification suites, mock fixtures | [MIT](https://github.com/pytest-dev/pytest/blob/main/LICENSE) | [pytest-dev/pytest](https://github.com/pytest-dev/pytest) | | **ruff** | `>=0.5.0` | High-performance Python linter and code formatting enforcement | [MIT / Apache-2.0](https://github.com/astral-sh/ruff/blob/main/LICENSE-MIT) | [astral-sh/ruff](https://github.com/astral-sh/ruff) | | **PyInstaller** | `>=6.0` | Optional standalone Windows executable builder (`build_exe.bat`) | [GPLv2-or-later with Special Exception](https://github.com/pyinstaller/pyinstaller/blob/develop/COPYING.txt) | [pyinstaller/pyinstaller](https://github.com/pyinstaller/pyinstaller) | -| **zombie-killer-tray** | commit `6c8eb2c` | Optional, Windows-only companion for orphaned MCP/language-server process cleanup, launched as its own subprocess | [MIT](https://github.com/dev-bricks/zombie-killer-tray/blob/main/LICENSE) | [dev-bricks/zombie-killer-tray](https://github.com/dev-bricks/zombie-killer-tray) | +| **zombie-killer-tray** | commit `039b4f2` | Optional, Windows-only companion for orphaned MCP/language-server process cleanup, launched as its own subprocess | [MIT](https://github.com/dev-bricks/zombie-killer-tray/blob/main/LICENSE) | [dev-bricks/zombie-killer-tray](https://github.com/dev-bricks/zombie-killer-tray) | --- diff --git a/THIRD_PARTY_LICENSES.txt b/THIRD_PARTY_LICENSES.txt index e8c0531..35d4ef9 100644 --- a/THIRD_PARTY_LICENSES.txt +++ b/THIRD_PARTY_LICENSES.txt @@ -27,7 +27,7 @@ The base package declares no external runtime dependencies in | pytest | Development tests | `>=9.1.1` | 9.1.1 | MIT | https://pypi.org/project/pytest/ | | ruff | Development linter | `>=0.5.0` | 0.6.2 | MIT / Apache-2.0 | https://pypi.org/project/ruff/ | | PyInstaller | Optional Windows EXE build | `>=6.0` | 6.21.0 | GPLv2-or-later with PyInstaller's special exception | https://pypi.org/project/pyinstaller/ | -| zombie-killer-tray | Optional, Windows-only integration | commit `6c8eb2cc8773e662c65e7a5c39d060b974805c3d` | 0.1.0 | MIT | https://github.com/dev-bricks/zombie-killer-tray | +| zombie-killer-tray | Optional, Windows-only integration | commit `039b4f2c7acd69063b39d6168c757b0b2fca2438` | 0.1.0 | MIT | https://github.com/dev-bricks/zombie-killer-tray | ## Transitive Build & Test Inventory diff --git a/pyproject.toml b/pyproject.toml index e1e12b3..48509b9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -75,7 +75,7 @@ build = [ # The environment marker also protects `pip install .[zombie-killer]` on a # non-Windows machine from failing on zombie-killer-tray's own Win32 deps. zombie-killer = [ - "zombie-killer-tray @ git+https://github.com/dev-bricks/zombie-killer-tray.git@6c8eb2cc8773e662c65e7a5c39d060b974805c3d ; sys_platform == 'win32'" + "zombie-killer-tray @ git+https://github.com/dev-bricks/zombie-killer-tray.git@039b4f2c7acd69063b39d6168c757b0b2fca2438 ; sys_platform == 'win32'" ] [project.scripts] diff --git a/src/safe_start_for_codex/zombie_killer_integration.py b/src/safe_start_for_codex/zombie_killer_integration.py index e845d7c..a40c7e4 100644 --- a/src/safe_start_for_codex/zombie_killer_integration.py +++ b/src/safe_start_for_codex/zombie_killer_integration.py @@ -26,13 +26,11 @@ from .cli import codex_home, no_window_kwargs -# T-20260926-212716751: zombie-killer-tray#4 (src/-Paketierung) is pushed but -# not yet merged -- this pin points at the branch head. Move it to the merge -# commit once #4 lands (same verification step as any other pinned dep here). +# T-20260926-212716751: zombie-killer-tray#4 (src/-Paketierung) merged as 039b4f2. ZOMBIE_KILLER_PACKAGE_SPEC = ( "zombie-killer-tray @ " "git+https://github.com/dev-bricks/zombie-killer-tray.git" - "@6c8eb2cc8773e662c65e7a5c39d060b974805c3d" + "@039b4f2c7acd69063b39d6168c757b0b2fca2438" ) ZOMBIE_KILLER_SOURCE_ENV = "SAFE_START_ZOMBIE_KILLER_SOURCE" DEFAULT_WATCH_INTERVAL_SECONDS = 600 From 3ca665268aa7d99cfe665dd85eb3199b146fba4d Mon Sep 17 00:00:00 2001 From: Lukas Geiger Date: Sat, 26 Sep 2026 15:55:40 +0200 Subject: [PATCH 3/4] fix: watcher lifecycle bug and hatch direct-reference metadata Review findings on this PR: (1) LIFECYCLE BUG (blocking): launch_zombie_killer_watch() passed --parent-pid . zombie-killer-tray's watch_parent thread kills the watch process as soon as whatever PID it was given for --parent-pid exits -- and the CLI process that spawns the watcher exits immediately after Popen returns, so the watcher died within ~1s of being started, before doing any real work. Mocked-Popen tests never caught it because they don't run the real subprocess lifecycle at all. Fix: drop --parent-pid entirely. The watcher is now a genuinely detached, long-lived process; its lifecycle is governed by its own PID file (/watch.pid) rather than any parent/child relationship. Added stop_zombie_killer_watch() (reads the PID file, verifies via psutil that the PID still belongs to zombie-killer-tray before signalling it) and a new `zombie-killer-stop` CLI subcommand. Added a REAL integration test (no mocked Popen, skipped on non-Windows) that spawns the actual zombie-killer-tray package as a subprocess exactly the way production code does, asserts it is still running 2s after the launching call returns (the old bug would already have killed it by then), then asserts zombie-killer-stop actually terminates it. Requires the package installed; added the same pip-install step to ci.yml (windows-latest only, matching zombie-killer-tray's own platform requirement -- the cross-platform source-platform-smoke.yml workflow only runs tests/source_platform_smoke.py and never collects this file). (3) [tool.hatch.metadata] allow-direct-references (blocking): the new `zombie-killer` optional extra declares a direct `git+https://...` reference, which hatchling refuses to build without this explicit opt-in -- `pip install -e .` failed with a ValueError during metadata generation. Reproduced the exact failure in a clean venv before adding the fix, then reproduced the fix working the same way. (2) is CareCenter-for-Codex-specific (taskkill /T, broad MCP markers) -- addressed in that repo's own PR, not this one; safe-start-for-codex has no equivalent code path. 143/143 tests green (128 pre-existing/unrelated-to-this-PR + this PR's own 15), ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01PTbvD41MCVmnQWaobvHfCk --- .github/workflows/ci.yml | 4 +- pyproject.toml | 6 ++ src/safe_start_for_codex/cli.py | 18 ++++ .../zombie_killer_integration.py | 84 ++++++++++++++++- tests/test_zombie_killer_integration.py | 91 ++++++++++++++++++- 5 files changed, 197 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 90e50d2..6e310b4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,7 +25,9 @@ jobs: python-version: ${{ matrix.python-version }} cache: 'pip' - name: Install - run: python -m pip install -e ".[dev,tray]" + run: | + python -m pip install -e ".[dev,tray]" + python -m pip install "zombie-killer-tray @ git+https://github.com/dev-bricks/zombie-killer-tray.git@039b4f2c7acd69063b39d6168c757b0b2fca2438" - name: Lint run: ruff check . - name: Bytecode compilation check diff --git a/pyproject.toml b/pyproject.toml index 48509b9..32cd607 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -97,6 +97,12 @@ Notice = "https://github.com/dev-bricks/safe-start-for-codex/blob/main/NOTICE" "LLM Ready" = "https://github.com/dev-bricks/safe-start-for-codex/blob/main/llms.txt" +[tool.hatch.metadata] +# T-20260926-212716751: the optional `zombie-killer` extra declares a direct +# `git+https://...` reference (zombie-killer-tray), which hatchling refuses +# to build without this explicit opt-in (broke `pip install -e .` in CI). +allow-direct-references = true + [tool.hatch.build.targets.wheel] packages = ["src/safe_start_for_codex"] diff --git a/src/safe_start_for_codex/cli.py b/src/safe_start_for_codex/cli.py index 0386da7..2f54755 100644 --- a/src/safe_start_for_codex/cli.py +++ b/src/safe_start_for_codex/cli.py @@ -1770,6 +1770,17 @@ def command_zombie_killer_watch(args: argparse.Namespace) -> int: return 0 if result.status == "ok" else 1 +def command_zombie_killer_stop(args: argparse.Namespace) -> int: + from .zombie_killer_integration import stop_zombie_killer_watch + + result = stop_zombie_killer_watch() + if args.json: + print(json.dumps(result.to_dict(), ensure_ascii=False, indent=2)) + else: + print(result.to_text()) + return 0 if result.status in ("ok", "not-running") else 1 + + def command_status(_: argparse.Namespace) -> int: latest = state_dir() / "latest.json" if not latest.exists(): @@ -1990,6 +2001,13 @@ def build_parser() -> argparse.ArgumentParser: zombie_killer_watch.add_argument("--min-age", type=int, default=None) zombie_killer_watch.add_argument("--json", action="store_true") zombie_killer_watch.set_defaults(func=command_zombie_killer_watch) + + zombie_killer_stop = sub.add_parser( + "zombie-killer-stop", + help="Stop a zombie-killer-tray watch subprocess started via zombie-killer-watch.", + ) + zombie_killer_stop.add_argument("--json", action="store_true") + zombie_killer_stop.set_defaults(func=command_zombie_killer_stop) return parser diff --git a/src/safe_start_for_codex/zombie_killer_integration.py b/src/safe_start_for_codex/zombie_killer_integration.py index a40c7e4..decbdc7 100644 --- a/src/safe_start_for_codex/zombie_killer_integration.py +++ b/src/safe_start_for_codex/zombie_killer_integration.py @@ -18,6 +18,7 @@ import json import os +import signal import subprocess import sys from collections.abc import Callable @@ -84,6 +85,22 @@ def to_text(self) -> str: return "\n".join(lines) +@dataclass(slots=True) +class ZombieKillerStopResult: + status: str + message: str + pid: int | None = None + + def to_dict(self) -> dict[str, object]: + return asdict(self) + + def to_text(self) -> str: + lines = [f"Status: {self.status}", self.message] + if self.pid is not None: + lines.append(f"PID: {self.pid}") + return "\n".join(lines) + + @dataclass(slots=True) class ZombieKillerStatus: available: bool @@ -266,16 +283,30 @@ def _zombie_killer_env() -> dict[str, str]: return env +def _watch_pid_file(state_dir: Path) -> Path: + return state_dir / "watch.pid" + + def launch_zombie_killer_watch( *, interval_seconds: int = DEFAULT_WATCH_INTERVAL_SECONDS, min_age_seconds: int = DEFAULT_MIN_AGE_SECONDS, popen: Callable[..., subprocess.Popen[str]] | None = None, ) -> ZombieKillerLaunchResult: - """Start `python -m zombie_killer_tray watch` as its own subprocess. + """Start `python -m zombie_killer_tray watch` as its own, long-lived + subprocess. Refuses cleanly on any non-Windows platform (zombie-killer-tray is Win32-only) rather than attempting a doomed subprocess spawn. + + NO `--parent-pid`: review finding (T-20260926-212716751) -- zombie- + killer-tray kills the watch process as soon as whatever PID was passed + via `--parent-pid` exits. A one-shot CLI call like `zombie-killer-watch` + exits right after this function returns, so passing its own (thus + already-doomed) PID there killed the watcher within ~1s of starting it, + before it could do any real work. The watcher's lifecycle is instead + tracked via its own PID file, independent of whoever launched it (see + `stop_zombie_killer_watch`). """ state_dir = zombie_killer_state_dir() if not _is_windows(): @@ -290,7 +321,6 @@ def launch_zombie_killer_watch( "watch", "--yes", "--interval", str(interval_seconds), "--min-age", str(min_age_seconds), - "--parent-pid", str(os.getpid()), ] env = _zombie_killer_env() run = popen or subprocess.Popen @@ -306,12 +336,15 @@ def launch_zombie_killer_watch( last_error = str(exc) continue pid = getattr(process, "pid", None) + pid_int = int(pid) if isinstance(pid, int) else None + if pid_int is not None: + _watch_pid_file(state_dir).write_text(str(pid_int), encoding="utf-8") return ZombieKillerLaunchResult( status="ok", command=command, - message="zombie-killer-tray was started as its own watch subprocess.", + message="zombie-killer-tray was started as its own, long-lived watch subprocess.", state_dir=str(state_dir), - pid=int(pid) if isinstance(pid, int) else None, + pid=pid_int, ) return ZombieKillerLaunchResult( @@ -322,6 +355,49 @@ def launch_zombie_killer_watch( ) +def _is_our_watch_process(pid: int) -> bool | None: + """True/False if verifiable, None if psutil is unavailable (best-effort: + zombie-killer-tray always pulls psutil in as its own dependency, so this + is only missing if the extra itself was never installed).""" + try: + import psutil + except ImportError: + return None + try: + return "zombie_killer_tray" in " ".join(psutil.Process(pid).cmdline()) + except psutil.Error: + return False + + +def stop_zombie_killer_watch() -> ZombieKillerStopResult: + """Stop the watch subprocess started by `launch_zombie_killer_watch`, + identified via its PID file rather than any parent/child relationship.""" + pid_file = _watch_pid_file(zombie_killer_state_dir()) + if not pid_file.exists(): + return ZombieKillerStopResult(status="not-running", message="No PID file found.") + try: + pid = int(pid_file.read_text(encoding="utf-8").strip()) + except (OSError, ValueError): + pid_file.unlink(missing_ok=True) + return ZombieKillerStopResult(status="not-running", message="PID file unreadable; removed.") + + verified = _is_our_watch_process(pid) + if verified is False: + pid_file.unlink(missing_ok=True) + return ZombieKillerStopResult( + status="not-found", message="PID no longer belongs to zombie-killer-tray.", pid=pid + ) + + try: + os.kill(pid, signal.SIGTERM) + except OSError: + pid_file.unlink(missing_ok=True) + return ZombieKillerStopResult(status="already-stopped", message="Process was not running.", pid=pid) + + pid_file.unlink(missing_ok=True) + return ZombieKillerStopResult(status="ok", message="zombie-killer-tray was stopped.", pid=pid) + + def _last_cycle_event(state_dir: Path) -> dict[str, object] | None: events_path = state_dir / "zombie_events.jsonl" if not events_path.exists(): diff --git a/tests/test_zombie_killer_integration.py b/tests/test_zombie_killer_integration.py index 97e3226..d56f954 100644 --- a/tests/test_zombie_killer_integration.py +++ b/tests/test_zombie_killer_integration.py @@ -1,16 +1,23 @@ from __future__ import annotations +import contextlib import json +import os +import signal import subprocess +import time from pathlib import Path from types import SimpleNamespace +import pytest + from safe_start_for_codex.zombie_killer_integration import ( ZOMBIE_KILLER_PACKAGE_SPEC, ZOMBIE_KILLER_SOURCE_ENV, build_zombie_killer_status, install_zombie_killer_package, launch_zombie_killer_watch, + stop_zombie_killer_watch, zombie_killer_install_target, zombie_killer_state_dir, ) @@ -103,8 +110,12 @@ def fake_popen(command, **kwargs): assert "watch" in command assert "--interval" in command and "42" in command assert "--min-age" in command and "99" in command - assert "--parent-pid" in command assert "--yes" in command + assert "--parent-pid" not in command, ( + "must NOT tie the watcher's lifetime to this (necessarily short-lived) " + "caller's own PID -- it would die within ~1s of being spawned (review finding)" + ) + assert (zombie_killer_state_dir() / "watch.pid").read_text(encoding="utf-8") == "4242" assert captured["cwd"] == str(tmp_path / ".codex" / "zombie-killer-tray") @@ -156,3 +167,81 @@ def test_build_status_marks_non_windows_as_unsupported(tmp_path: Path, monkeypat assert status.supported_platform is False assert status.available is False assert any("Windows-only" in note for note in status.notes) + + +def test_stop_reports_not_running_without_a_pid_file(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + result = stop_zombie_killer_watch() + assert result.status == "not-running" + + +def test_stop_reports_not_found_for_a_stale_pid_reused_by_something_else( + tmp_path: Path, monkeypatch +) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + state_dir = zombie_killer_state_dir() + # PID of the current test process itself -- definitely alive, definitely + # NOT a zombie-killer-tray process, so this exercises the cmdline check + # refusing to kill an unrelated process that happens to have reused the pid. + (state_dir / "watch.pid").write_text(str(os.getpid()), encoding="utf-8") + + result = stop_zombie_killer_watch() + + assert result.status == "not-found" + assert not (state_dir / "watch.pid").exists(), "stale/wrong pid file must be cleaned up" + + +def test_stop_reports_already_stopped_for_a_dead_pid(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + state_dir = zombie_killer_state_dir() + (state_dir / "watch.pid").write_text("999999999", encoding="utf-8") + monkeypatch.setattr( + "safe_start_for_codex.zombie_killer_integration._is_our_watch_process", + lambda pid: None, # simulate psutil unavailable/inconclusive -- still must not crash + ) + + result = stop_zombie_killer_watch() + + assert result.status == "already-stopped" + assert not (state_dir / "watch.pid").exists() + + +@pytest.mark.skipif(os.name != "nt", reason="zombie-killer-tray is Windows-only") +@pytest.mark.timeout(30) +def test_real_watch_subprocess_outlives_its_launcher_and_stop_terminates_it( + tmp_path: Path, monkeypatch +) -> None: + """No mocked Popen -- a genuinely real subprocess, spawned exactly the way + production code does it. Direct regression test for the review finding: + the old design passed --parent-pid , and + zombie-killer-tray's watch_parent thread kills the watcher within ~1s of + whatever PID it was given exiting. + """ + psutil = pytest.importorskip("psutil") + pytest.importorskip("zombie_killer_tray") + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + + result = launch_zombie_killer_watch(interval_seconds=3, min_age_seconds=30) + assert result.status == "ok", result.message + pid = result.pid + assert pid is not None + + try: + time.sleep(2.0) + assert psutil.pid_exists(pid), ( + "the watcher must still be running 2s after launch returned -- " + "with the old --parent-pid design it would already be dead" + ) + + stop_result = stop_zombie_killer_watch() + assert stop_result.status == "ok" + assert stop_result.pid == pid + + deadline = time.monotonic() + 10 + while time.monotonic() < deadline and psutil.pid_exists(pid): + time.sleep(0.2) + assert not psutil.pid_exists(pid), "the watcher must actually exit after being stopped" + finally: + if psutil.pid_exists(pid): + with contextlib.suppress(OSError): + os.kill(pid, signal.SIGTERM) From 99fc3e7bc9b18f64cb71a2f4166a959764ef5927 Mon Sep 17 00:00:00 2001 From: Lukas Geiger Date: Sat, 26 Sep 2026 16:14:06 +0200 Subject: [PATCH 4/4] fix: fail-closed stop, double-start guard, real watcher PID, py311 marker Re-review findings, all 4 addressed (mirrors CareCenter-for-Codex#2): (1) stop was fail-open on unverifiable PIDs -- replaced _is_our_watch_process with _verify_watch_process(pid, expected_create_time); stop now refuses (status="verification-unavailable", process untouched) whenever verification returns None instead of falling through to os.kill(). The PID file now stores {"pid", "create_time"} (from the real watcher's own psutil.Process(pid).create_time() right after spawn) for a create_time cross-check on top of the cmdline check -- catches PID reuse the cmdline check alone might miss. psutil declared explicitly in the `zombie-killer` extra (was only relying on it arriving transitively). (2) A second `zombie-killer-watch` silently overwrote watch.pid, orphaning the first watcher. launch_zombie_killer_watch() now verifies any existing PID file before spawning: confirmed-alive refuses a second start (status="already-running"); unverifiable refuses too, fail-closed, same direction as (1) (status="verification-unavailable"); only a confirmed- dead/stale entry is cleared and a fresh watcher started. (3) N/A for this repo in the strict sense (safe-start's launch function never took a MaintenanceConfig, so there was no separate "test vs production" config split) -- added the same apply: bool = True parameter anyway for parity with CareCenter and switched the integration test to apply=False, since the real subprocess it spawns could otherwise reap real, qualifying orphans on whatever machine runs it (including CI). (4) In a frozen build, sys.executable is the packaged host app and `py -3` spawns py.exe as a WRAPPER that itself spawns the real interpreter as ITS OWN child (Windows has no exec()) -- the PID Popen returned was the launcher's, not the worker's, so stop never reached the actual watcher. Replaced the multi-candidate launch loop with _resolve_watch_python_executable(): not frozen -> sys.executable directly; frozen -> resolve the real path once via each launcher candidate's own `-c "import sys; print(sys.executable)"` stdout, then spawn directly against that resolved path. Added a unit test simulating the frozen case. Safe-start-specific: the `zombie-killer` extra's git dependency now also carries a `python_version >= '3.12'` marker (zombie-killer-tray's own floor; this project's is >=3.11) alongside the existing `sys_platform == 'win32'` marker, so `pip install .[zombie-killer]` is a correct no-op on 3.11 instead of failing. ci.yml's separate zombie-killer-tray install step (needed for the real integration test) is now gated with `if: matrix.python-version != '3.11'` for the same reason -- verified the marker syntax by actually installing `.[zombie-killer]` in a clean venv (psutil + zombie-killer-tray both resolved correctly on 3.12/win32). 150/150 tests green (128 pre-existing + this PR's own 22), ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01PTbvD41MCVmnQWaobvHfCk --- .github/workflows/ci.yml | 7 +- THIRD_PARTY_LICENSES.md | 1 + THIRD_PARTY_LICENSES.txt | 1 + pyproject.toml | 14 +- .../zombie_killer_integration.py | 232 +++++++++++++----- tests/test_third_party_licenses.py | 1 + tests/test_zombie_killer_integration.py | 170 ++++++++++++- 7 files changed, 345 insertions(+), 81 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6e310b4..ece3ca6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,9 +25,10 @@ jobs: python-version: ${{ matrix.python-version }} cache: 'pip' - name: Install - run: | - python -m pip install -e ".[dev,tray]" - python -m pip install "zombie-killer-tray @ git+https://github.com/dev-bricks/zombie-killer-tray.git@039b4f2c7acd69063b39d6168c757b0b2fca2438" + run: python -m pip install -e ".[dev,tray]" + - name: Install zombie-killer-tray (Python 3.12+ only; it doesn't support 3.11) + if: matrix.python-version != '3.11' + run: python -m pip install "zombie-killer-tray @ git+https://github.com/dev-bricks/zombie-killer-tray.git@039b4f2c7acd69063b39d6168c757b0b2fca2438" - name: Lint run: ruff check . - name: Bytecode compilation check diff --git a/THIRD_PARTY_LICENSES.md b/THIRD_PARTY_LICENSES.md index 2b4443c..5145655 100644 --- a/THIRD_PARTY_LICENSES.md +++ b/THIRD_PARTY_LICENSES.md @@ -58,6 +58,7 @@ The core runtime of `safe-start-for-codex` has **zero external runtime dependenc | **ruff** | `>=0.5.0` | High-performance Python linter and code formatting enforcement | [MIT / Apache-2.0](https://github.com/astral-sh/ruff/blob/main/LICENSE-MIT) | [astral-sh/ruff](https://github.com/astral-sh/ruff) | | **PyInstaller** | `>=6.0` | Optional standalone Windows executable builder (`build_exe.bat`) | [GPLv2-or-later with Special Exception](https://github.com/pyinstaller/pyinstaller/blob/develop/COPYING.txt) | [pyinstaller/pyinstaller](https://github.com/pyinstaller/pyinstaller) | | **zombie-killer-tray** | commit `039b4f2` | Optional, Windows-only companion for orphaned MCP/language-server process cleanup, launched as its own subprocess | [MIT](https://github.com/dev-bricks/zombie-killer-tray/blob/main/LICENSE) | [dev-bricks/zombie-killer-tray](https://github.com/dev-bricks/zombie-killer-tray) | +| **psutil** | `>=7.2,<8` | Watcher PID/create_time verification (Windows + Python>=3.12 only) | [BSD-3-Clause](https://github.com/giampaolo/psutil/blob/master/LICENSE) | [giampaolo/psutil](https://github.com/giampaolo/psutil) | --- diff --git a/THIRD_PARTY_LICENSES.txt b/THIRD_PARTY_LICENSES.txt index 35d4ef9..7ff77b3 100644 --- a/THIRD_PARTY_LICENSES.txt +++ b/THIRD_PARTY_LICENSES.txt @@ -28,6 +28,7 @@ The base package declares no external runtime dependencies in | ruff | Development linter | `>=0.5.0` | 0.6.2 | MIT / Apache-2.0 | https://pypi.org/project/ruff/ | | PyInstaller | Optional Windows EXE build | `>=6.0` | 6.21.0 | GPLv2-or-later with PyInstaller's special exception | https://pypi.org/project/pyinstaller/ | | zombie-killer-tray | Optional, Windows-only integration | commit `039b4f2c7acd69063b39d6168c757b0b2fca2438` | 0.1.0 | MIT | https://github.com/dev-bricks/zombie-killer-tray | +| psutil | Watcher PID/create_time verification, Windows/Python>=3.12 only | `>=7.2,<8` | 7.2.0 | BSD-3-Clause | https://pypi.org/project/psutil/ | ## Transitive Build & Test Inventory diff --git a/pyproject.toml b/pyproject.toml index 32cd607..0e4b728 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -72,10 +72,18 @@ build = [ # T-20260926-212716751: optional, Windows-only companion tool (conservative # cleanup of orphaned MCP/language-server processes). Never installed # implicitly -- only pulled in if a caller explicitly requests this extra. -# The environment marker also protects `pip install .[zombie-killer]` on a -# non-Windows machine from failing on zombie-killer-tray's own Win32 deps. +# Two environment markers, combined: `sys_platform` protects `pip install +# .[zombie-killer]` on a non-Windows machine from failing on +# zombie-killer-tray's own Win32 deps; `python_version` does the same for +# Python 3.11 (this project's own floor is >=3.11, but zombie-killer-tray +# itself requires >=3.12 -- review finding: without this marker, the CI +# matrix's 3.11 leg would try and fail to install it). zombie-killer = [ - "zombie-killer-tray @ git+https://github.com/dev-bricks/zombie-killer-tray.git@039b4f2c7acd69063b39d6168c757b0b2fca2438 ; sys_platform == 'win32'" + "zombie-killer-tray @ git+https://github.com/dev-bricks/zombie-killer-tray.git@039b4f2c7acd69063b39d6168c757b0b2fca2438 ; sys_platform == 'win32' and python_version >= '3.12'", + # Declared explicitly (not just hoped-for transitively via + # zombie-killer-tray) -- stop_zombie_killer_watch()/launch's double-start + # guard fail-closed without it, so it must not be an implicit assumption. + "psutil>=7.2,<8 ; sys_platform == 'win32' and python_version >= '3.12'", ] [project.scripts] diff --git a/src/safe_start_for_codex/zombie_killer_integration.py b/src/safe_start_for_codex/zombie_killer_integration.py index decbdc7..c56af05 100644 --- a/src/safe_start_for_codex/zombie_killer_integration.py +++ b/src/safe_start_for_codex/zombie_killer_integration.py @@ -189,20 +189,37 @@ def _pip_command_candidates() -> list[list[str]]: return unique -def _python_command_candidates() -> list[list[str]]: - candidates: list[list[str]] = [] +def _resolve_watch_python_executable( + *, runner: Callable[[list[str]], subprocess.CompletedProcess[str]] | None = None +) -> str | None: + """Resolve a REAL python.exe path -- never `py.exe`, the Python Launcher. + + Review finding (T-20260926-212716751): Windows has no exec(), so + launching the watcher via `["py", "-3", ...]` spawns py.exe as a WRAPPER + process that itself spawns the real interpreter as ITS OWN child. The + PID `subprocess.Popen` hands back is the launcher's, not the worker's -- + in a frozen build (`sys.executable` is the packaged host app, unusable + as an interpreter, so `py -3` was the only remaining candidate), + `stop_zombie_killer_watch` was signalling the launcher while the actual + watcher kept running, unreachable, as an orphan. + + Not frozen: `sys.executable` already IS a real interpreter, used + directly. Frozen: resolve the real path once via `-c "import sys; + print(sys.executable)"` through each launcher candidate, so the actual + watch subprocess is spawned directly against that resolved path. + """ if not getattr(sys, "frozen", False): - candidates.append([sys.executable]) - candidates.extend((["py", "-3"], ["python3"], ["python"])) - - unique: list[list[str]] = [] - seen: set[tuple[str, ...]] = set() - for candidate in candidates: - key = tuple(candidate) - if key not in seen: - unique.append(candidate) - seen.add(key) - return unique + return sys.executable + run = runner or _run_install_command + for candidate in (["py", "-3"], ["python3"], ["python"]): + try: + completed = run([*candidate, "-c", "import sys; print(sys.executable)"]) + except OSError: + continue + path = completed.stdout.strip() if completed.stdout else "" + if completed.returncode == 0 and path and Path(path).is_file(): + return path + return None def _run_install_command(command: list[str]) -> subprocess.CompletedProcess[str]: @@ -287,10 +304,64 @@ def _watch_pid_file(state_dir: Path) -> Path: return state_dir / "watch.pid" +def _write_watch_pid_file(state_dir: Path, pid: int, create_time: float | None) -> None: + payload = {"pid": pid, "create_time": create_time} + _watch_pid_file(state_dir).write_text(json.dumps(payload), encoding="utf-8") + + +def _read_watch_pid_file(state_dir: Path) -> tuple[int, float | None] | None: + pid_file = _watch_pid_file(state_dir) + if not pid_file.exists(): + return None + try: + payload = json.loads(pid_file.read_text(encoding="utf-8")) + raw_create_time = payload.get("create_time") + return ( + int(payload["pid"]), + float(raw_create_time) if raw_create_time is not None else None, + ) + except (OSError, ValueError, KeyError, TypeError, json.JSONDecodeError): + return None + + +def _process_create_time(pid: int) -> float | None: + try: + import psutil + except ImportError: + return None + try: + return psutil.Process(pid).create_time() + except psutil.Error: + return None + + +def _verify_watch_process(pid: int, expected_create_time: float | None) -> bool | None: + """True: `pid` is verifiably the same zombie-killer-tray watcher incarnation + that was recorded. False: gone, or the PID was reused by something else. + None: cannot verify at all (psutil unavailable or inaccessible) -- + callers MUST treat this as "do not touch" (review finding: `stop` used + to fail-open on None, so a stale, verification-less PID file could + terminate an unrelated process that happened to reuse the PID).""" + try: + import psutil + except ImportError: + return None + try: + proc = psutil.Process(pid) + actual_create_time = proc.create_time() + cmdline = " ".join(proc.cmdline()) + except psutil.Error: + return False + if expected_create_time is not None and abs(actual_create_time - expected_create_time) > 2.0: + return False # a different incarnation -- the pid was reused + return "zombie_killer_tray" in cmdline + + def launch_zombie_killer_watch( *, interval_seconds: int = DEFAULT_WATCH_INTERVAL_SECONDS, min_age_seconds: int = DEFAULT_MIN_AGE_SECONDS, + apply: bool = True, popen: Callable[..., subprocess.Popen[str]] | None = None, ) -> ZombieKillerLaunchResult: """Start `python -m zombie_killer_tray watch` as its own, long-lived @@ -299,14 +370,24 @@ def launch_zombie_killer_watch( Refuses cleanly on any non-Windows platform (zombie-killer-tray is Win32-only) rather than attempting a doomed subprocess spawn. - NO `--parent-pid`: review finding (T-20260926-212716751) -- zombie- - killer-tray kills the watch process as soon as whatever PID was passed - via `--parent-pid` exits. A one-shot CLI call like `zombie-killer-watch` - exits right after this function returns, so passing its own (thus - already-doomed) PID there killed the watcher within ~1s of starting it, - before it could do any real work. The watcher's lifecycle is instead + `apply=False` starts in preview mode (no `--yes`): the watcher logs + candidates but never terminates a real process. Defaults to `True` for + production use; a caller that only wants to exercise the lifecycle + (does the watcher survive, does stop work) MUST pass `apply=False` -- + otherwise the real subprocess can actually reap real, qualifying + orphans on whatever machine runs it (review finding). + + NO `--parent-pid`: zombie-killer-tray kills the watch process as soon + as whatever PID was passed via `--parent-pid` exits. A one-shot CLI + call like `zombie-killer-watch` exits right after this function + returns, so passing its own (thus already-doomed) PID there killed the + watcher within ~1s of starting it. The watcher's lifecycle is instead tracked via its own PID file, independent of whoever launched it (see `stop_zombie_killer_watch`). + + Refuses a second start while the existing PID file describes a still- + running (or unverifiable) watcher -- otherwise a second call overwrites + `watch.pid` and the first watcher becomes unreachable (review finding). """ state_dir = zombie_killer_state_dir() if not _is_windows(): @@ -317,73 +398,92 @@ def launch_zombie_killer_watch( state_dir=str(state_dir), ) + existing = _read_watch_pid_file(state_dir) + if existing is not None: + existing_pid, existing_create_time = existing + verified = _verify_watch_process(existing_pid, existing_create_time) + if verified is not False: + return ZombieKillerLaunchResult( + status="already-running" if verified else "verification-unavailable", + command=[], + message=( + f"A watcher is already running (PID {existing_pid}); " + "use zombie-killer-stop to stop it." + if verified + else "Existing PID file cannot be verified (psutil missing?); " + "check before starting another one." + ), + state_dir=str(state_dir), + pid=existing_pid, + ) + _watch_pid_file(state_dir).unlink(missing_ok=True) # stale/dead -- safe to clear + + python_executable = _resolve_watch_python_executable() + if python_executable is None: + return ZombieKillerLaunchResult( + status="failed", + command=[], + message="No real Python interpreter found (the py.exe launcher is deliberately " + "never used directly as the watch process, see _resolve_watch_python_executable).", + state_dir=str(state_dir), + ) + args = [ - "watch", "--yes", + "watch", + *(["--yes"] if apply else []), "--interval", str(interval_seconds), "--min-age", str(min_age_seconds), ] + command = [python_executable, "-m", "zombie_killer_tray", *args] env = _zombie_killer_env() run = popen or subprocess.Popen - last_error = "" - last_command: list[str] = [] + try: + process = run(command, cwd=str(state_dir), env=env, close_fds=True, **no_window_kwargs()) + except OSError as exc: + return ZombieKillerLaunchResult( + status="failed", command=command, message=str(exc), state_dir=str(state_dir) + ) - for python_command in _python_command_candidates(): - command = [*python_command, "-m", "zombie_killer_tray", *args] - last_command = command - try: - process = run(command, cwd=str(state_dir), env=env, close_fds=True, **no_window_kwargs()) - except OSError as exc: - last_error = str(exc) - continue - pid = getattr(process, "pid", None) - pid_int = int(pid) if isinstance(pid, int) else None - if pid_int is not None: - _watch_pid_file(state_dir).write_text(str(pid_int), encoding="utf-8") + pid = getattr(process, "pid", None) + pid_int = int(pid) if isinstance(pid, int) else None + if pid_int is None: return ZombieKillerLaunchResult( - status="ok", + status="failed", command=command, - message="zombie-killer-tray was started as its own, long-lived watch subprocess.", + message="Subprocess returned no PID.", state_dir=str(state_dir), - pid=pid_int, ) - + _write_watch_pid_file(state_dir, pid_int, _process_create_time(pid_int)) return ZombieKillerLaunchResult( - status="failed", - command=last_command, - message=last_error or "No Python command found for zombie-killer-tray.", + status="ok", + command=command, + message="zombie-killer-tray was started as its own, long-lived watch subprocess.", state_dir=str(state_dir), + pid=pid_int, ) -def _is_our_watch_process(pid: int) -> bool | None: - """True/False if verifiable, None if psutil is unavailable (best-effort: - zombie-killer-tray always pulls psutil in as its own dependency, so this - is only missing if the extra itself was never installed).""" - try: - import psutil - except ImportError: - return None - try: - return "zombie_killer_tray" in " ".join(psutil.Process(pid).cmdline()) - except psutil.Error: - return False - - def stop_zombie_killer_watch() -> ZombieKillerStopResult: """Stop the watch subprocess started by `launch_zombie_killer_watch`, identified via its PID file rather than any parent/child relationship.""" - pid_file = _watch_pid_file(zombie_killer_state_dir()) - if not pid_file.exists(): + state_dir = zombie_killer_state_dir() + existing = _read_watch_pid_file(state_dir) + if existing is None: return ZombieKillerStopResult(status="not-running", message="No PID file found.") - try: - pid = int(pid_file.read_text(encoding="utf-8").strip()) - except (OSError, ValueError): - pid_file.unlink(missing_ok=True) - return ZombieKillerStopResult(status="not-running", message="PID file unreadable; removed.") + pid, create_time = existing - verified = _is_our_watch_process(pid) + verified = _verify_watch_process(pid, create_time) + if verified is None: + # FAIL-CLOSED (review finding): cannot verify this PID is really our + # watcher (psutil missing/inaccessible) -- refuse to touch it rather + # than blindly signalling a possibly-unrelated, reused PID. + return ZombieKillerStopResult( + status="verification-unavailable", + message="Cannot verify PID (psutil missing?); process was NOT stopped.", + pid=pid, + ) if verified is False: - pid_file.unlink(missing_ok=True) + _watch_pid_file(state_dir).unlink(missing_ok=True) return ZombieKillerStopResult( status="not-found", message="PID no longer belongs to zombie-killer-tray.", pid=pid ) @@ -391,10 +491,10 @@ def stop_zombie_killer_watch() -> ZombieKillerStopResult: try: os.kill(pid, signal.SIGTERM) except OSError: - pid_file.unlink(missing_ok=True) + _watch_pid_file(state_dir).unlink(missing_ok=True) return ZombieKillerStopResult(status="already-stopped", message="Process was not running.", pid=pid) - pid_file.unlink(missing_ok=True) + _watch_pid_file(state_dir).unlink(missing_ok=True) return ZombieKillerStopResult(status="ok", message="zombie-killer-tray was stopped.", pid=pid) diff --git a/tests/test_third_party_licenses.py b/tests/test_third_party_licenses.py index 113f6a1..c9bff9a 100644 --- a/tests/test_third_party_licenses.py +++ b/tests/test_third_party_licenses.py @@ -38,6 +38,7 @@ def test_third_party_license_inventory_covers_direct_dependencies() -> None: "pytest", "ruff", "zombie-killer-tray", + "psutil", } for package_name in _declared_direct_dependencies(): assert package_name in text diff --git a/tests/test_zombie_killer_integration.py b/tests/test_zombie_killer_integration.py index d56f954..9e84824 100644 --- a/tests/test_zombie_killer_integration.py +++ b/tests/test_zombie_killer_integration.py @@ -5,6 +5,7 @@ import os import signal import subprocess +import sys import time from pathlib import Path from types import SimpleNamespace @@ -115,10 +116,125 @@ def fake_popen(command, **kwargs): "must NOT tie the watcher's lifetime to this (necessarily short-lived) " "caller's own PID -- it would die within ~1s of being spawned (review finding)" ) - assert (zombie_killer_state_dir() / "watch.pid").read_text(encoding="utf-8") == "4242" + pid_payload = json.loads((zombie_killer_state_dir() / "watch.pid").read_text(encoding="utf-8")) + assert pid_payload["pid"] == 4242 assert captured["cwd"] == str(tmp_path / ".codex" / "zombie-killer-tray") +def test_launch_uses_preview_mode_without_apply(tmp_path: Path, monkeypatch) -> None: + """Review finding: a caller that only wants to exercise the watcher's + lifecycle, not its actual reap behaviour, must be able to omit --yes -- + otherwise a real watch subprocess can terminate real, qualifying orphans + on whatever machine runs it.""" + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + + def fake_popen(command, **kwargs): + return SimpleNamespace(pid=1) + + result = launch_zombie_killer_watch(apply=False, popen=fake_popen) + assert result.status == "ok" + assert "--yes" not in result.command + + +def test_launch_refuses_a_second_start_while_the_first_watcher_is_verified_alive( + tmp_path: Path, monkeypatch +) -> None: + """Review finding: a second `watch` used to silently overwrite watch.pid, + orphaning the first watcher (still running, now unreachable via stop).""" + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + state_dir = zombie_killer_state_dir() + (state_dir / "watch.pid").write_text( + json.dumps({"pid": 424242, "create_time": 123.0}), encoding="utf-8" + ) + monkeypatch.setattr( + "safe_start_for_codex.zombie_killer_integration._verify_watch_process", + lambda pid, create_time: True, + ) + called = False + + def fake_popen(command, **kwargs): + nonlocal called + called = True + return SimpleNamespace(pid=1) + + result = launch_zombie_killer_watch(popen=fake_popen) + + assert result.status == "already-running" + assert result.pid == 424242 + assert called is False, "must never spawn a second watcher over a verified-alive one" + assert json.loads((state_dir / "watch.pid").read_text(encoding="utf-8"))["pid"] == 424242 + + +def test_launch_refuses_a_second_start_when_verification_is_unavailable( + tmp_path: Path, monkeypatch +) -> None: + """Fail-closed applies to the double-start guard too.""" + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + state_dir = zombie_killer_state_dir() + (state_dir / "watch.pid").write_text( + json.dumps({"pid": 424242, "create_time": 123.0}), encoding="utf-8" + ) + monkeypatch.setattr( + "safe_start_for_codex.zombie_killer_integration._verify_watch_process", + lambda pid, create_time: None, + ) + called = False + + def fake_popen(command, **kwargs): + nonlocal called + called = True + return SimpleNamespace(pid=1) + + result = launch_zombie_killer_watch(popen=fake_popen) + + assert result.status == "verification-unavailable" + assert called is False + + +def test_launch_proceeds_when_the_existing_pid_file_is_stale(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + state_dir = zombie_killer_state_dir() + (state_dir / "watch.pid").write_text( + json.dumps({"pid": 424242, "create_time": 123.0}), encoding="utf-8" + ) + monkeypatch.setattr( + "safe_start_for_codex.zombie_killer_integration._verify_watch_process", + lambda pid, create_time: False, + ) + + def fake_popen(command, **kwargs): + return SimpleNamespace(pid=99999) + + result = launch_zombie_killer_watch(popen=fake_popen) + + assert result.status == "ok" + assert result.pid == 99999 + assert json.loads((state_dir / "watch.pid").read_text(encoding="utf-8"))["pid"] == 99999 + + +def test_resolve_python_executable_uses_sys_executable_when_not_frozen() -> None: + from safe_start_for_codex.zombie_killer_integration import _resolve_watch_python_executable + + assert _resolve_watch_python_executable() == sys.executable + + +def test_resolve_python_executable_resolves_through_the_launcher_when_frozen(monkeypatch) -> None: + """Review finding: in a frozen build, `sys.executable` is the packaged + host app, not an interpreter, and launching via the bare `py -3` + launcher would hand back the LAUNCHER's pid, not the worker's.""" + from safe_start_for_codex.zombie_killer_integration import _resolve_watch_python_executable + + monkeypatch.setattr(sys, "frozen", True, raising=False) + real_path = sys.executable + + def fake_runner(command: list[str]) -> subprocess.CompletedProcess[str]: + assert command[:2] == ["py", "-3"] + return subprocess.CompletedProcess(command, returncode=0, stdout=real_path + "\n", stderr="") + + resolved = _resolve_watch_python_executable(runner=fake_runner) + assert resolved == real_path + + def test_launch_reports_failure_when_no_python_found(tmp_path: Path, monkeypatch) -> None: monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) @@ -178,12 +294,16 @@ def test_stop_reports_not_running_without_a_pid_file(tmp_path: Path, monkeypatch def test_stop_reports_not_found_for_a_stale_pid_reused_by_something_else( tmp_path: Path, monkeypatch ) -> None: + pytest.importorskip("psutil") monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) state_dir = zombie_killer_state_dir() # PID of the current test process itself -- definitely alive, definitely - # NOT a zombie-killer-tray process, so this exercises the cmdline check - # refusing to kill an unrelated process that happens to have reused the pid. - (state_dir / "watch.pid").write_text(str(os.getpid()), encoding="utf-8") + # NOT a zombie-killer-tray process (wrong cmdline AND, deliberately, a + # create_time far from its real one), so this exercises the reuse check + # refusing to kill an unrelated process that happens to reuse the pid. + (state_dir / "watch.pid").write_text( + json.dumps({"pid": os.getpid(), "create_time": 1.0}), encoding="utf-8" + ) result = stop_zombie_killer_watch() @@ -191,14 +311,42 @@ def test_stop_reports_not_found_for_a_stale_pid_reused_by_something_else( assert not (state_dir / "watch.pid").exists(), "stale/wrong pid file must be cleaned up" -def test_stop_reports_already_stopped_for_a_dead_pid(tmp_path: Path, monkeypatch) -> None: +def test_stop_fails_closed_when_verification_is_unavailable(tmp_path: Path, monkeypatch) -> None: + """Review finding (blocking): stop used to fail-OPEN when verification + returned None (psutil unavailable) -- it killed the PID anyway. It must + instead refuse and leave the process untouched.""" + monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) + state_dir = zombie_killer_state_dir() + (state_dir / "watch.pid").write_text( + json.dumps({"pid": 999999999, "create_time": 123.0}), encoding="utf-8" + ) + killed: list[int] = [] + monkeypatch.setattr( + "safe_start_for_codex.zombie_killer_integration._verify_watch_process", + lambda pid, create_time: None, + ) + monkeypatch.setattr(os, "kill", lambda pid, sig: killed.append(pid)) + + result = stop_zombie_killer_watch() + + assert result.status == "verification-unavailable" + assert killed == [], "must NOT signal the pid when verification is unavailable (fail-closed)" + assert (state_dir / "watch.pid").exists(), "an unresolved pid file is left in place, not deleted" + + +def test_stop_reports_already_stopped_for_a_verified_but_dead_pid( + tmp_path: Path, monkeypatch +) -> None: monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) state_dir = zombie_killer_state_dir() - (state_dir / "watch.pid").write_text("999999999", encoding="utf-8") + (state_dir / "watch.pid").write_text( + json.dumps({"pid": 999999999, "create_time": 123.0}), encoding="utf-8" + ) monkeypatch.setattr( - "safe_start_for_codex.zombie_killer_integration._is_our_watch_process", - lambda pid: None, # simulate psutil unavailable/inconclusive -- still must not crash + "safe_start_for_codex.zombie_killer_integration._verify_watch_process", + lambda pid, create_time: True, ) + monkeypatch.setattr(os, "kill", lambda pid, sig: (_ for _ in ()).throw(OSError("gone"))) result = stop_zombie_killer_watch() @@ -221,8 +369,12 @@ def test_real_watch_subprocess_outlives_its_launcher_and_stop_terminates_it( pytest.importorskip("zombie_killer_tray") monkeypatch.setenv("CODEX_HOME", str(tmp_path / ".codex")) - result = launch_zombie_killer_watch(interval_seconds=3, min_age_seconds=30) + # apply=False (preview/no --yes): review finding -- with --yes, this real + # subprocess would actually reap real, qualifying orphan processes on + # whatever machine runs this test (including CI). + result = launch_zombie_killer_watch(interval_seconds=3, min_age_seconds=30, apply=False) assert result.status == "ok", result.message + assert "--yes" not in result.command pid = result.pid assert pid is not None