From 327ca7264a1a11f710f2c48c75448ab39db6e152 Mon Sep 17 00:00:00 2001 From: Suhani Date: Wed, 15 Jul 2026 15:47:06 +0530 Subject: [PATCH 1/2] fix: resolve Windows npm CLI shims and process trees Codex/Claude readiness used bare create_subprocess_exec names, which miss npm .CMD shims on Windows and left hung auth probes uncleared. Resolve executables via shutil.which, kill process trees with taskkill, and keep provider path serialization POSIX-stable. --- pyproject.toml | 2 +- src/yoke/models.py | 2 +- src/yoke/process.py | 134 +++++++++++++++++++++++ src/yoke/providers/claude.py | 18 ++- src/yoke/providers/codex_agents.py | 8 +- src/yoke/providers/codex_app/process.py | 15 ++- src/yoke/providers/codex_app_server.py | 9 +- src/yoke/providers/codex_cli.py | 6 +- src/yoke/providers/codex_sdk.py | 9 +- src/yoke/providers/runtime_deployment.py | 13 +-- src/yoke/readiness.py | 22 +++- tests/test_claude_options.py | 2 +- tests/test_executable_resolution.py | 134 +++++++++++++++++++++++ tests/test_runtime_deployments.py | 13 ++- 14 files changed, 347 insertions(+), 40 deletions(-) create mode 100644 src/yoke/process.py create mode 100644 tests/test_executable_resolution.py diff --git a/pyproject.toml b/pyproject.toml index 5923b59..d68f97d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "almanac-yoke" -version = "0.1.7" +version = "0.1.8" description = "A provider-neutral harness SDK for agent systems on Claude and Codex." readme = "README.md" requires-python = ">=3.11" diff --git a/src/yoke/models.py b/src/yoke/models.py index abb5d34..9dda41a 100644 --- a/src/yoke/models.py +++ b/src/yoke/models.py @@ -893,7 +893,7 @@ def native_input(self) -> dict[str, Any]: if self.native_name is not None: data["name"] = self.native_name if self.script_path is not None: - data["scriptPath"] = str(self.script_path) + data["scriptPath"] = self.script_path.as_posix() if self.args is not None: data["args"] = self.args if self.resume_from_run_id is not None: diff --git a/src/yoke/process.py b/src/yoke/process.py new file mode 100644 index 0000000..a521e07 --- /dev/null +++ b/src/yoke/process.py @@ -0,0 +1,134 @@ +"""Cross-platform process helpers for provider CLIs.""" + +from __future__ import annotations + +import asyncio +import os +import shutil +import subprocess +import sys +from pathlib import Path + + +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. + """ + + path = Path(command) + if path.is_file(): + return str(path) + return shutil.which(command) + + +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 os.name != "nt" + + +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 sys.platform == "win32": + 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 sys.platform == "win32": + creationflags = getattr(subprocess, "CREATE_NO_WINDOW", 0) + subprocess.run( + ["taskkill", "/PID", str(pid), "/T", "/F"], + check=False, + capture_output=True, + creationflags=creationflags, + ) + return + try: + os.kill(pid, 9) + except ProcessLookupError: + return + + +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 sys.platform == "win32": + 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 _suppress_timeout(): + process.wait(timeout=0.5) + + +class _suppress_timeout: + def __enter__(self) -> None: + return None + + def __exit__(self, exc_type: object, exc: object, traceback: object) -> bool: + return isinstance(exc, subprocess.TimeoutExpired) diff --git a/src/yoke/providers/claude.py b/src/yoke/providers/claude.py index f2dae95..3047a94 100644 --- a/src/yoke/providers/claude.py +++ b/src/yoke/providers/claude.py @@ -60,6 +60,7 @@ SessionOptions, WorkflowOptions, ) +from yoke.process import path_for_provider from yoke.providers.claude_plugins import ( is_plugin_skill_path, plugin_paths, @@ -145,6 +146,11 @@ async def check(self, harness: Harness) -> Readiness: surface=self.surface, available=False, message="claude auth status timed out", + fix=( + "Claude CLI is installed, but `claude auth status` did not " + "return. Run it in an interactive terminal, or set " + f"{ANTHROPIC_API_KEY}." + ), ) if result.code != 0: return Readiness( @@ -354,7 +360,7 @@ async def read_session( from claude_agent_sdk import get_session_info, get_session_messages except ImportError as exc: raise YokeError(f"Claude support requires `{CLAUDE_INSTALL}`.") from exc - directory = str(harness.cwd) if harness.cwd else None + directory = path_for_provider(harness.cwd) if harness.cwd else None info = await asyncio.to_thread( get_session_info, session_id, @@ -396,7 +402,7 @@ async def rename(self, session: Session, title: str) -> SessionSummary: except ImportError as exc: raise YokeError(f"Claude support requires `{CLAUDE_INSTALL}`.") from exc session_id = session.provider_session_id or session.id - directory = str(session.cwd) if session.cwd else None + directory = path_for_provider(session.cwd) if session.cwd else None await asyncio.to_thread( rename_session, session_id, @@ -409,7 +415,7 @@ async def rename(self, session: Session, title: str) -> SessionSummary: id=session_id, provider_session_id=session_id, title=title, - cwd=str(session.cwd) if session.cwd else None, + cwd=path_for_provider(session.cwd) if session.cwd else None, ) async def tag(self, session: Session, tag: str | None) -> SessionSummary: @@ -418,7 +424,7 @@ async def tag(self, session: Session, tag: str | None) -> SessionSummary: except ImportError as exc: raise YokeError(f"Claude support requires `{CLAUDE_INSTALL}`.") from exc session_id = session.provider_session_id or session.id - directory = str(session.cwd) if session.cwd else None + directory = path_for_provider(session.cwd) if session.cwd else None await asyncio.to_thread( tag_session, session_id, @@ -431,7 +437,7 @@ async def tag(self, session: Session, tag: str | None) -> SessionSummary: id=session_id, provider_session_id=session_id, tag=tag, - cwd=str(session.cwd) if session.cwd else None, + cwd=path_for_provider(session.cwd) if session.cwd else None, ) async def start(self, harness: Harness, options: SessionOptions) -> Session: @@ -716,7 +722,7 @@ def claude_options( goal, compile_inline=deployment is None, ), - "cwd": str(harness.cwd), + "cwd": path_for_provider(harness.cwd), "model": options.model or agent.model, "effort": options.effort or agent.effort, "max_turns": options.max_turns, diff --git a/src/yoke/providers/codex_agents.py b/src/yoke/providers/codex_agents.py index 775aac6..9b44a60 100644 --- a/src/yoke/providers/codex_agents.py +++ b/src/yoke/providers/codex_agents.py @@ -161,7 +161,7 @@ def codex_agent_toml( continue lines.append("") lines.append("[[skills.config]]") - lines.append(f"path = {toml_string(str(path))}") + lines.append(f"path = {toml_string(path_string(path))}") lines.append(f"enabled = {'true' if enabled else 'false'}") return "\n".join(lines) + "\n" @@ -225,6 +225,12 @@ def slug(value: str) -> str: return normalized or "agent" +def path_string(path: Path | str) -> str: + """Serialize filesystem paths with forward slashes for Codex agent TOML.""" + + return Path(path).as_posix() + + def toml_string(value: str) -> str: return json.dumps(value) diff --git a/src/yoke/providers/codex_app/process.py b/src/yoke/providers/codex_app/process.py index 0e5e593..a5be8f4 100644 --- a/src/yoke/providers/codex_app/process.py +++ b/src/yoke/providers/codex_app/process.py @@ -16,6 +16,11 @@ from pydantic import JsonValue, TypeAdapter, ValidationError from yoke.errors import YokeError +from yoke.process import ( + 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 +44,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 +60,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 +95,9 @@ def read_until(self, deadline: float, timeout_label: str) -> JsonObject: def terminate(self) -> None: if self.child.poll() is not None: return + if os.name == "nt": + 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_app_server.py b/src/yoke/providers/codex_app_server.py index 1850866..a22b57d 100644 --- a/src/yoke/providers/codex_app_server.py +++ b/src/yoke/providers/codex_app_server.py @@ -50,6 +50,7 @@ SessionOptions, WorkflowOptions, ) +from yoke.process import path_for_provider from yoke.providers.codex_app.events import TurnResult, read_turn, read_turn_step from yoke.providers.codex_app.fields import as_record, string_field from yoke.providers.codex_app.goals import app_goal_status, yoke_goal @@ -999,9 +1000,11 @@ def codex_runtime_config(deployment: RuntimeDeployment) -> dict[str, Any]: } } if deployment.codex_parent_skill_settings: + from yoke.providers.codex_agents import path_string + config["skills"] = { "config": [ - {"path": str(path), "enabled": enabled} + {"path": path_string(path), "enabled": enabled} for path, enabled in deployment.codex_parent_skill_settings ] } @@ -1105,7 +1108,7 @@ def thread_params( ) -> dict[str, Any]: permission_profile = codex_app_server_option(provider_options, "permissions") params: dict[str, Any] = { - "cwd": str(harness.cwd), + "cwd": path_for_provider(harness.cwd), "model": model or harness.agent.model, "approvalPolicy": approval_policy(permissions, provider_options), "developerInstructions": developer_instructions( @@ -1164,7 +1167,7 @@ def turn_params( permission_profile = codex_app_server_option(effective_options, "permissions") params: dict[str, Any] = { "threadId": thread.thread_id, - "cwd": str(thread.cwd), + "cwd": path_for_provider(thread.cwd), "input": [ { "type": "text", 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/codex_sdk.py b/src/yoke/providers/codex_sdk.py index e5069e0..8791d9e 100644 --- a/src/yoke/providers/codex_sdk.py +++ b/src/yoke/providers/codex_sdk.py @@ -46,6 +46,7 @@ SessionOptions, WorkflowOptions, ) +from yoke.process import path_for_provider from yoke.providers.codex_app.events import TurnResult, map_notification from yoke.providers.codex_app.prompts import developer_instructions from yoke.readiness import run_command @@ -353,7 +354,7 @@ async def stream(self, session: Session, turn: Turn, options: RunOptions): permissions = options.permissions or session.permissions handle = await sdk_session.thread.turn( prompt_with_goal(turn.prompt, options.resolve_goal(session.goal)), - cwd=str(session.cwd) if session.cwd else None, + cwd=path_for_provider(session.cwd) if session.cwd else None, effort=str(options.effort or session.agent.effort) if options.effort or session.agent.effort else None, @@ -398,7 +399,7 @@ async def fork(self, session: Session, options: ForkOptions) -> Session: thread = await sdk_session.process.client.thread_fork( session.id, approval_mode=approval_mode(sdk, permissions), - cwd=str(session.cwd) if session.cwd else None, + cwd=path_for_provider(session.cwd) if session.cwd else None, developer_instructions=developer_instructions(session.agent), ephemeral=options.ephemeral, model=sdk_session.model, @@ -437,7 +438,7 @@ async def _thread( kwargs = { "approval_mode": approval_mode(sdk, permissions), "config": codex_config(options), - "cwd": str(harness.cwd), + "cwd": path_for_provider(harness.cwd), "developer_instructions": developer_instructions(harness.agent), "model": options.model or harness.agent.model, "sandbox": sandbox(sdk, permissions), @@ -466,7 +467,7 @@ async def _send( try: result = await sdk_session.thread.run( prompt_with_goal(turn.prompt, run_options.resolve_goal(session.goal)), - cwd=str(session.cwd) if session.cwd else None, + cwd=path_for_provider(session.cwd) if session.cwd else None, effort=str(run_options.effort or session.agent.effort) if run_options.effort or session.agent.effort else None, 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..04f3a57 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 "" + + +# Re-export for callers that already import from readiness. +__all__ = [ + "CommandCheck", + "first_line", + "resolve_executable", + "run_command", +] diff --git a/tests/test_claude_options.py b/tests/test_claude_options.py index 8415489..7eb4d51 100644 --- a/tests/test_claude_options.py +++ b/tests/test_claude_options.py @@ -85,7 +85,7 @@ def test_claude_provider_options_reach_sdk_options( assert options.kwargs["include_partial_messages"] is True assert options.kwargs["include_hook_events"] is True assert options.kwargs["max_budget_usd"] == 1.5 - assert options.kwargs["cwd"] == str(Path.cwd()) + assert options.kwargs["cwd"] == Path.cwd().as_posix() assert options.kwargs["append_system_prompt"] == "extra" assert options.kwargs["model"] == "sonnet" diff --git a/tests/test_executable_resolution.py b/tests/test_executable_resolution.py new file mode 100644 index 0000000..8cd2768 --- /dev/null +++ b/tests/test_executable_resolution.py @@ -0,0 +1,134 @@ +from __future__ import annotations + +import asyncio +import os +import sys +import time +from pathlib import Path + +import pytest + +from yoke import Agent, Harness +from yoke.process import kill_process_tree, 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", reason="Windows npm shim integration") +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", reason="Windows npm shim integration") +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", reason="Windows npm shim integration") +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", reason="Windows npm shim integration") +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) diff --git a/tests/test_runtime_deployments.py b/tests/test_runtime_deployments.py index 5f371a8..a424e2d 100644 --- a/tests/test_runtime_deployments.py +++ b/tests/test_runtime_deployments.py @@ -207,14 +207,17 @@ 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"] + Path(item["path"]).as_posix(): 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() From 1ae88254d4dc2f1490100661828cc07f3aa4595d Mon Sep 17 00:00:00 2001 From: Suhani Date: Sun, 2 Aug 2026 16:42:11 +0530 Subject: [PATCH 2/2] fix: harden process-tree kills and executable resolution Follow-up review of the Windows shim fix. kill_process_tree only signalled the single pid on POSIX despite promising descendants, orphaning every child of a session leader. It now signals the child's process group, and skips the group kill when the target shares this process's group so a caller can never signal itself. resolve_executable treated any command naming an existing file as a path, including bare names, so a file in the working directory could shadow a real PATH entry while Yoke operated on an untrusted checkout. Bare names now always go through PATH. Path-like commands are returned verbatim, because normalising through Path strips a leading "./" and turns an explicit relative path back into a PATH lookup. Also collapses three spellings of the Windows check onto one IS_WINDOWS constant, replaces a hand-rolled context manager with contextlib.suppress, and drops a misleading comment on readiness.__all__ that claimed to control direct imports. Adds a Windows CI matrix, since the code this branch changes is exactly the code that behaves differently there, and a changelog for the 0.1.8 release. --- .github/workflows/ci.yml | 4 +- CHANGELOG.md | 40 ++++++++++++++ src/yoke/process.py | 70 +++++++++++++++++------- src/yoke/providers/codex_app/process.py | 3 +- src/yoke/readiness.py | 2 +- tests/test_executable_resolution.py | 73 ++++++++++++++++++++++++- 6 files changed, 168 insertions(+), 24 deletions(-) create mode 100644 CHANGELOG.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 15ab794..a3cf4b6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,9 +10,11 @@ permissions: jobs: test: - runs-on: ubuntu-latest + runs-on: ${{ matrix.os }} strategy: + fail-fast: false matrix: + os: [ubuntu-latest, windows-latest] python-version: ["3.11", "3.12", "3.13"] steps: - uses: actions/checkout@v4 diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..a66b03a --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,40 @@ +# Changelog + +All notable changes to this project are documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [0.1.8] - 2026-08-02 + +First release with supported Windows hosts. + +### Added + +- `yoke.process`, a cross-platform module for resolving provider executables, + serializing paths for provider APIs, and terminating process trees. +- CI now runs the test suite on `windows-latest` as well as `ubuntu-latest`, + across Python 3.11, 3.12, and 3.13. + +### Fixed + +- Provider CLIs installed through npm are found on Windows. Commands are + resolved with `shutil.which`, which locates the `.CMD`/`.BAT` shims that + `asyncio.create_subprocess_exec` cannot launch by bare name. Previously + Codex and Claude were reported as "not found on PATH" on every Windows host. +- `claude auth status` no longer hangs forever when the Windows CLI stalls with + a piped stdout. The readiness check times out and reports a repair hint + instead of blocking the caller. +- Terminating a provider process now kills its descendants rather than only the + wrapper process, so npm shim trees are not left running. Windows uses + `taskkill /T /F`; POSIX signals the child's process group. +- Paths sent to provider APIs use forward slashes, so Windows drive paths + round-trip through providers that expect POSIX separators. + +### Security + +- A bare command name is always resolved through `PATH`. A file named, for + example, `codex` in the working directory can no longer shadow the real + executable when Yoke operates on an untrusted checkout. + +[0.1.8]: https://github.com/AlmanacCode/Yoke/releases/tag/v0.1.8 diff --git a/src/yoke/process.py b/src/yoke/process.py index a521e07..cdf0919 100644 --- a/src/yoke/process.py +++ b/src/yoke/process.py @@ -3,12 +3,15 @@ from __future__ import annotations import asyncio +import contextlib import os import shutil +import signal import subprocess -import sys 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.""" @@ -22,20 +25,33 @@ def resolve_executable(command: str) -> str | None: 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. """ - path = Path(command) - if path.is_file(): - return str(path) + 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 os.name != "nt" + return not IS_WINDOWS def process_is_alive(pid: int) -> bool: @@ -45,7 +61,7 @@ def process_is_alive(pid: int) -> bool: return False if pid == os.getpid(): return True - if sys.platform == "win32": + if IS_WINDOWS: return _windows_process_is_alive(pid) try: os.kill(pid, 0) @@ -80,7 +96,7 @@ def kill_process_tree(pid: int) -> None: if pid <= 0: return - if sys.platform == "win32": + if IS_WINDOWS: creationflags = getattr(subprocess, "CREATE_NO_WINDOW", 0) subprocess.run( ["taskkill", "/PID", str(pid), "/T", "/F"], @@ -89,10 +105,32 @@ def kill_process_tree(pid: int) -> None: creationflags=creationflags, ) return - try: - os.kill(pid, 9) - except ProcessLookupError: + 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: @@ -100,7 +138,7 @@ async def terminate_asyncio_process(process: asyncio.subprocess.Process) -> None if process.returncode is not None or process.pid is None: return - if sys.platform == "win32": + if IS_WINDOWS: await asyncio.to_thread(kill_process_tree, process.pid) try: await asyncio.wait_for(process.wait(), timeout=2) @@ -122,13 +160,5 @@ def terminate_popen(process: subprocess.Popen[str]) -> None: process.wait(timeout=2) except subprocess.TimeoutExpired: process.kill() - with _suppress_timeout(): + with contextlib.suppress(subprocess.TimeoutExpired): process.wait(timeout=0.5) - - -class _suppress_timeout: - def __enter__(self) -> None: - return None - - def __exit__(self, exc_type: object, exc: object, traceback: object) -> bool: - return isinstance(exc, subprocess.TimeoutExpired) diff --git a/src/yoke/providers/codex_app/process.py b/src/yoke/providers/codex_app/process.py index a5be8f4..229ddb7 100644 --- a/src/yoke/providers/codex_app/process.py +++ b/src/yoke/providers/codex_app/process.py @@ -17,6 +17,7 @@ from yoke.errors import YokeError from yoke.process import ( + IS_WINDOWS, popen_start_new_session, resolve_executable, terminate_popen, @@ -95,7 +96,7 @@ def read_until(self, deadline: float, timeout_label: str) -> JsonObject: def terminate(self) -> None: if self.child.poll() is not None: return - if os.name == "nt": + if IS_WINDOWS: terminate_popen(self.child) return try: diff --git a/src/yoke/readiness.py b/src/yoke/readiness.py index 04f3a57..7251f2e 100644 --- a/src/yoke/readiness.py +++ b/src/yoke/readiness.py @@ -68,10 +68,10 @@ def first_line(value: str) -> str: return lines[0] if lines else "" -# Re-export for callers that already import from readiness. __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 index 8cd2768..9324342 100644 --- a/tests/test_executable_resolution.py +++ b/tests/test_executable_resolution.py @@ -2,6 +2,7 @@ import asyncio import os +import subprocess import sys import time from pathlib import Path @@ -9,7 +10,7 @@ import pytest from yoke import Agent, Harness -from yoke.process import kill_process_tree, resolve_executable +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 @@ -132,3 +133,73 @@ def test_claude_readiness_does_not_hang_when_auth_status_stalls( 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)