diff --git a/src/yoke/process.py b/src/yoke/process.py new file mode 100644 index 0000000..fc30b1f --- /dev/null +++ b/src/yoke/process.py @@ -0,0 +1,155 @@ +"""Cross-platform process helpers for provider CLIs.""" + +from __future__ import annotations + +import asyncio +import contextlib +import os +import shutil +import signal +import subprocess +from pathlib import Path + +IS_WINDOWS = os.name == "nt" + + +def path_for_provider(path: str | Path) -> str: + """Serialize a filesystem path for provider APIs with stable separators.""" + return Path(path).as_posix() + + +def resolve_executable(command: str) -> str | None: + """Resolve a command name to an executable path on the current platform. + + On Windows this finds npm ``.CMD``/``.BAT`` shims that + ``asyncio.create_subprocess_exec("codex", ...)`` cannot launch by bare + name. + + A bare name always goes through ``PATH``. Only a command written as a path + is read from the filesystem, so a file named ``codex`` in the working + directory cannot shadow the real executable. + """ + if _is_path_like(command): + # Returned verbatim: normalising through Path would strip a leading + # ``./``, turning an explicit relative path back into a bare name that + # the exec call would then look up on PATH. + return command if Path(command).is_file() else None + return shutil.which(command) + + +def _is_path_like(command: str) -> bool: + """Return whether a command names a filesystem path rather than a bare name.""" + separators = (os.sep, os.altsep) if os.altsep else (os.sep,) + return any(separator in command for separator in separators) + + +def popen_start_new_session() -> bool: + """Return whether new process groups are safe for this platform.""" + # Windows process groups do not match POSIX killpg semantics and can leave + # npm shim trees behind when only the wrapper pid is terminated. + return not IS_WINDOWS + + +def process_is_alive(pid: int) -> bool: + """Return whether a PID currently refers to a live process.""" + if pid <= 0: + return False + if pid == os.getpid(): + return True + if IS_WINDOWS: + return _windows_process_is_alive(pid) + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except PermissionError: + return True + return True + + +def _windows_process_is_alive(pid: int) -> bool: + import ctypes + from ctypes import wintypes + + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + process_query_limited_information = 0x1000 + still_active = 259 + handle = kernel32.OpenProcess(process_query_limited_information, 0, pid) + if not handle: + return False + try: + exit_code = wintypes.DWORD() + if kernel32.GetExitCodeProcess(handle, ctypes.byref(exit_code)) == 0: + return False + return int(exit_code.value) == still_active + finally: + kernel32.CloseHandle(handle) + + +def kill_process_tree(pid: int) -> None: + """Force-terminate a process and its descendants.""" + if pid <= 0: + return + if IS_WINDOWS: + creationflags = getattr(subprocess, "CREATE_NO_WINDOW", 0) + subprocess.run( + ["taskkill", "/PID", str(pid), "/T", "/F"], + check=False, + capture_output=True, + creationflags=creationflags, + ) + return + if _kill_process_group(pid): + return + with contextlib.suppress(ProcessLookupError, PermissionError): + os.kill(pid, signal.SIGKILL) + + +def _kill_process_group(pid: int) -> bool: + """Kill the process group led by ``pid``, if it is safe to do so. + + Children started with ``start_new_session=True`` lead their own group, so + signalling the group reaches descendants that a bare ``os.kill`` would + orphan. A child sharing this process's group is skipped: signalling that + group would kill the caller too. + """ + try: + group = os.getpgid(pid) + except (ProcessLookupError, PermissionError, OSError): + return False + if group in (os.getpgid(0), 0): + return False + try: + os.killpg(group, signal.SIGKILL) + except (ProcessLookupError, PermissionError, OSError): + return False + return True + + +async def terminate_asyncio_process(process: asyncio.subprocess.Process) -> None: + """Terminate an asyncio subprocess, including Windows npm shim trees.""" + if process.returncode is not None or process.pid is None: + return + if IS_WINDOWS: + await asyncio.to_thread(kill_process_tree, process.pid) + try: + await asyncio.wait_for(process.wait(), timeout=2) + except TimeoutError: + process.kill() + await process.wait() + return + process.kill() + await process.wait() + + +def terminate_popen(process: subprocess.Popen[str]) -> None: + """Terminate a ``subprocess.Popen`` process tree.""" + if process.poll() is not None or process.pid is None: + return + kill_process_tree(process.pid) + try: + process.wait(timeout=2) + except subprocess.TimeoutExpired: + process.kill() + with contextlib.suppress(subprocess.TimeoutExpired): + process.wait(timeout=0.5) diff --git a/src/yoke/providers/codex_app/process.py b/src/yoke/providers/codex_app/process.py index 0e5e593..229ddb7 100644 --- a/src/yoke/providers/codex_app/process.py +++ b/src/yoke/providers/codex_app/process.py @@ -16,6 +16,12 @@ from pydantic import JsonValue, TypeAdapter, ValidationError from yoke.errors import YokeError +from yoke.process import ( + IS_WINDOWS, + popen_start_new_session, + resolve_executable, + terminate_popen, +) from yoke.providers.codex_app.fields import JsonObject, as_record, string_field JSON_VALUE = TypeAdapter(JsonValue) @@ -39,12 +45,15 @@ def start( cwd: Path, env: dict[str, str] | None, ) -> JsonRpcLineProcess: + executable = resolve_executable(command) + if executable is None: + raise FileNotFoundError(command) process_env = dict(os.environ) process_env.setdefault("YOKE_INTERNAL_SESSION", "1") if env is not None: process_env.update(env) child = subprocess.Popen( - (command, *args), + (executable, *args), cwd=cwd, env=process_env, text=True, @@ -52,7 +61,7 @@ def start( stdout=subprocess.PIPE, stderr=subprocess.PIPE, bufsize=1, - start_new_session=True, + start_new_session=popen_start_new_session(), ) return cls(child) @@ -87,6 +96,9 @@ def read_until(self, deadline: float, timeout_label: str) -> JsonObject: def terminate(self) -> None: if self.child.poll() is not None: return + if IS_WINDOWS: + terminate_popen(self.child) + return try: os.killpg(os.getpgid(self.child.pid), signal.SIGTERM) except (AttributeError, ProcessLookupError, PermissionError, OSError): diff --git a/src/yoke/providers/codex_cli.py b/src/yoke/providers/codex_cli.py index a405985..cf3712e 100644 --- a/src/yoke/providers/codex_cli.py +++ b/src/yoke/providers/codex_cli.py @@ -11,6 +11,7 @@ from typing import Any from yoke.errors import YokeError +from yoke.process import resolve_executable ORIGINATOR_ENV = "CODEX_INTERNAL_ORIGINATOR_OVERRIDE" YOKE_ORIGINATOR = "yoke_python" @@ -47,7 +48,10 @@ async def run( ) -> AsyncIterator[dict[str, Any]]: schema_path: Path | None = None try: - args = [self.executable, "exec", "--json", "--cd", str(cwd)] + executable = resolve_executable(self.executable) + if executable is None: + raise FileNotFoundError(self.executable) + args = [executable, "exec", "--json", "--cd", str(cwd)] if model: args.extend(["--model", model]) if sandbox: diff --git a/src/yoke/providers/runtime_deployment.py b/src/yoke/providers/runtime_deployment.py index c97285e..bb29985 100644 --- a/src/yoke/providers/runtime_deployment.py +++ b/src/yoke/providers/runtime_deployment.py @@ -11,6 +11,7 @@ from yoke.errors import YokeError from yoke.models import Agent, Provider, Skill +from yoke.process import process_is_alive from yoke.providers.codex_agents import ( codex_agent_name, codex_agent_toml, @@ -117,18 +118,6 @@ def runtime_owner_pid(name: str) -> int | None: return pid if pid > 0 else None -def process_is_alive(pid: int) -> bool: - if pid == os.getpid(): - return True - try: - os.kill(pid, 0) - except ProcessLookupError: - return False - except PermissionError: - return True - return True - - def _write_codex(agent: Agent, deployment: RuntimeDeployment) -> None: agents_dir = deployment.root / "agents" entries, role_maps = _codex_roles(agent) diff --git a/src/yoke/readiness.py b/src/yoke/readiness.py index 4f3a0c5..7251f2e 100644 --- a/src/yoke/readiness.py +++ b/src/yoke/readiness.py @@ -6,6 +6,8 @@ import os from dataclasses import dataclass +from yoke.process import resolve_executable, terminate_asyncio_process + @dataclass(frozen=True) class CommandCheck: @@ -29,11 +31,17 @@ async def run_command( ) -> CommandCheck: """Run one local readiness command.""" + if not args: + raise ValueError("run_command requires at least one argument") + executable = resolve_executable(args[0]) + if executable is None: + raise FileNotFoundError(args[0]) process_env = dict(os.environ) if env is not None: process_env.update(env) process = await asyncio.create_subprocess_exec( - *args, + executable, + *args[1:], stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, env=process_env, @@ -44,8 +52,7 @@ async def run_command( timeout=timeout_seconds, ) except TimeoutError: - process.kill() - await process.wait() + await terminate_asyncio_process(process) raise return CommandCheck( code=process.returncode or 0, @@ -59,3 +66,12 @@ def first_line(value: str) -> str: lines = value.splitlines() return lines[0] if lines else "" + + +__all__ = [ + "CommandCheck", + "first_line", + "resolve_executable", + "run_command", + "terminate_asyncio_process", +] diff --git a/tests/test_executable_resolution.py b/tests/test_executable_resolution.py new file mode 100644 index 0000000..2fa4160 --- /dev/null +++ b/tests/test_executable_resolution.py @@ -0,0 +1,205 @@ +from __future__ import annotations + +import asyncio +import os +import subprocess +import sys +import time +from pathlib import Path + +import pytest + +from yoke import Agent, Harness +from yoke.process import kill_process_tree, process_is_alive, resolve_executable +from yoke.providers.claude import Claude +from yoke.providers.codex import Codex +from yoke.providers.codex_app_server import CodexAppServer +from yoke.readiness import run_command + + +def test_resolve_executable_returns_existing_file(tmp_path: Path) -> None: + executable = tmp_path / "codex" + executable.write_text("", encoding="utf-8") + assert resolve_executable(str(executable)) == str(executable) + + +def test_resolve_executable_finds_pathext_shim(tmp_path: Path, monkeypatch) -> None: + shim = tmp_path / "codex.cmd" + shim.write_text("@echo off\r\necho codex-cli 0.0.0-test\r\n", encoding="utf-8") + monkeypatch.setenv("PATH", str(tmp_path)) + resolved = resolve_executable("codex") + assert resolved is not None + assert Path(resolved).resolve() == shim.resolve() + + +def test_run_command_times_out_without_hanging(tmp_path: Path, monkeypatch) -> None: + script = tmp_path / ("hang.cmd" if sys.platform == "win32" else "hang") + if sys.platform == "win32": + # Invoke this interpreter by absolute path so the shim can hang even + # when PATH is restricted to the temporary directory + System32. + system32 = Path(os.environ.get("SystemRoot", r"C:\Windows")) / "System32" + script.write_text( + f'@echo off\r\n"{sys.executable}" -c "import time; time.sleep(30)"\r\n', + encoding="utf-8", + ) + monkeypatch.setenv("PATH", os.pathsep.join((str(tmp_path), str(system32)))) + else: + script.write_text("#!/bin/sh\nsleep 30\n", encoding="utf-8") + script.chmod(0o755) + monkeypatch.setenv("PATH", str(tmp_path)) + + started = time.monotonic() + with pytest.raises(TimeoutError): + asyncio.run(run_command("hang", timeout_seconds=1)) + elapsed = time.monotonic() - started + assert elapsed < 8 + + +@pytest.mark.skipif( + sys.platform != "win32" or resolve_executable("codex") is None, + reason="Windows npm shim integration (requires codex on PATH)", +) +def test_run_command_resolves_npm_codex_shim() -> None: + resolved = resolve_executable("codex") + assert resolved is not None + assert resolved.lower().endswith((".cmd", ".exe", ".bat")) + result = asyncio.run(run_command("codex", "--version")) + assert result.code == 0 + assert "codex" in result.stdout.lower() + + +@pytest.mark.skipif( + sys.platform != "win32" or resolve_executable("codex") is None, + reason="Windows npm shim integration (requires codex on PATH)", +) +def test_codex_readiness_does_not_report_missing_when_shim_is_on_path() -> None: + harness = Harness( + provider="codex", + surface="codex_cli", + agent=Agent(instructions="test"), + cwd=Path.cwd(), + ) + readiness = asyncio.run(Codex(executable="codex").check(harness)) + assert readiness.message != "codex not found on PATH" + assert readiness.available is True + assert "ChatGPT" in (readiness.message or "") or readiness.available + + +@pytest.mark.skipif( + sys.platform != "win32" or resolve_executable("codex") is None, + reason="Windows npm shim integration (requires codex on PATH)", +) +def test_codex_app_server_readiness_does_not_report_missing_when_shim_is_on_path() -> ( + None +): + harness = Harness( + provider="codex", + surface="codex_app_server", + agent=Agent(instructions="test"), + cwd=Path.cwd(), + ) + readiness = asyncio.run(CodexAppServer(executable="codex").check(harness)) + assert readiness.message != "codex not found on PATH" + assert readiness.available is True + + +@pytest.mark.skipif( + sys.platform != "win32" or resolve_executable("claude") is None, + reason="Windows npm shim integration (requires claude on PATH)", +) +def test_claude_readiness_does_not_hang_when_auth_status_stalls( + monkeypatch, +) -> None: + monkeypatch.setitem(sys.modules, "claude_agent_sdk", object()) + harness = Harness( + provider="claude", + surface="claude_python_sdk", + agent=Agent(instructions="test"), + cwd=Path.cwd(), + ) + started = time.monotonic() + readiness = asyncio.run(Claude(executable="claude").check(harness)) + elapsed = time.monotonic() - started + assert readiness.message != "claude not found on PATH" + assert elapsed < 20 + # Auth may succeed, fail with a CLI auth error, or time out when the + # Windows Claude CLI stalls with a piped stdout. Never hang forever. + assert ( + readiness.available is True + or "timed out" in (readiness.message or "") + or "login" in (readiness.message or "").lower() + or "auth" in (readiness.message or "").lower() + ) + + +def test_kill_process_tree_is_safe_for_missing_pid() -> None: + kill_process_tree(-1) + + +def test_resolve_executable_ignores_working_directory_shadow( + tmp_path: Path, monkeypatch +) -> None: + """A bare name resolves through PATH, never through the working directory.""" + shadow = tmp_path / "cwd" + shadow.mkdir() + decoy = shadow / "codex" + decoy.write_text("", encoding="utf-8") + if os.name != "nt": + decoy.chmod(0o755) + + empty_path = tmp_path / "bin" + empty_path.mkdir() + + monkeypatch.chdir(shadow) + monkeypatch.setenv("PATH", str(empty_path)) + assert resolve_executable("codex") is None + + # An explicit relative path is still honoured. + assert resolve_executable(f".{os.sep}codex") == f".{os.sep}codex" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX process-group semantics") +def test_kill_process_tree_kills_posix_descendants(tmp_path: Path) -> None: + """Killing a session leader must reap its children, not orphan them.""" + child_pid_file = tmp_path / "child.pid" + script = tmp_path / "spawn.sh" + script.write_text( + f'#!/bin/sh\nsleep 60 &\necho $! > "{child_pid_file}"\nwait\n', + encoding="utf-8", + ) + script.chmod(0o755) + + parent = subprocess.Popen([str(script)], start_new_session=True) + try: + deadline = time.monotonic() + 10 + while time.monotonic() < deadline and not child_pid_file.exists(): + time.sleep(0.05) + assert child_pid_file.exists(), "grandchild never reported its pid" + child_pid = int(child_pid_file.read_text(encoding="utf-8").strip()) + assert process_is_alive(child_pid) + + kill_process_tree(parent.pid) + + deadline = time.monotonic() + 10 + while time.monotonic() < deadline and process_is_alive(child_pid): + time.sleep(0.05) + assert not process_is_alive(child_pid), "grandchild survived the tree kill" + finally: + parent.kill() + parent.wait(timeout=10) + + +def test_kill_process_tree_does_not_kill_the_caller() -> None: + """A pid sharing this process's group must never trigger a group kill.""" + child = subprocess.Popen( + [sys.executable, "-c", "import time; time.sleep(60)"], + start_new_session=False, + ) + try: + kill_process_tree(child.pid) + child.wait(timeout=10) + assert process_is_alive(os.getpid()) + finally: + if child.poll() is None: + child.kill() + child.wait(timeout=10) diff --git a/tests/test_runtime_deployments.py b/tests/test_runtime_deployments.py index 5f371a8..49af21f 100644 --- a/tests/test_runtime_deployments.py +++ b/tests/test_runtime_deployments.py @@ -207,14 +207,16 @@ def capture(_process, method, params, _timeout): assert str(root_skill.parent.resolve()) in roots assert str(child_skill.parent.resolve()) in roots - assert str(child_skill.resolve()) in reviewer - assert f'path = "{child_skill.resolve()}"\nenabled = true' in reviewer + child_path = child_skill.resolve().as_posix() + root_path = root_skill.resolve().as_posix() + assert child_path in reviewer + assert f'path = "{child_path}"\nenabled = true' in reviewer config = codex_runtime_config(deployment) settings = { item["path"]: item["enabled"] for item in config["skills"]["config"] } - assert settings[str(root_skill.resolve())] is True - assert settings[str(child_skill.resolve())] is False + assert settings[root_path] is True + assert settings[child_path] is False finally: deployment.cleanup()