From 6d3ab1535ec690c3be10f0adde61a51741ca8509 Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 13:12:06 +0300 Subject: [PATCH 01/19] Print harness output as UTF-8 on a legacy Windows console Windows still defaults stdout to cp1252, which turns the routing summary's separators into `?` and makes Chinese prompt output unprintable. --- src/lh_harness/cli.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/lh_harness/cli.py b/src/lh_harness/cli.py index d78f70c1..a42667c8 100644 --- a/src/lh_harness/cli.py +++ b/src/lh_harness/cli.py @@ -385,7 +385,24 @@ def _fallback_hint(role: str, suffix: str) -> str: return ", then ".join(chain) +def _use_utf8_console() -> None: + """Stop a legacy console codec from mangling harness output. + + Windows still defaults stdout to cp1252, which turns the routing summary's + separators into `?` and makes Chinese prompt output unprintable. + """ + for stream in (sys.stdout, sys.stderr): + reconfigure = getattr(stream, "reconfigure", None) + if reconfigure is None: + continue + try: + reconfigure(encoding="utf-8", errors="replace") + except (OSError, ValueError): + pass + + def main(argv: list[str] | None = None) -> int: + _use_utf8_console() raw_argv = list(sys.argv[1:] if argv is None else argv) run_defaults: dict[str, object] = {} config_error: ProjectConfigError | None = None From b201c86daf6b7547dc0cc43741e31ca405474add Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 13:12:43 +0300 Subject: [PATCH 02/19] Run agent CLIs without a shell so the harness works on Windows Agent commands were built as POSIX shell strings and handed to `create_subprocess_shell`, which is cmd.exe on Windows, so `mkdir -p` failed on the first call. Process control used `os.killpg`/`SIGKILL`/`SIGHUP`, none of which exist there, so every timeout and Ctrl+C raised AttributeError. Rather than translate the shell strings per platform, remove the shell from the agent path entirely. Everything it was doing is a real subprocess argument: cd && ... -> cwd= VAR=value -> env= layered onto the scrubbed parent environment ... < prompt.md -> the prompt is written to the child's stdin mkdir -p / chmod -> native pathlib calls on the local environment Commands are argv lists now, so `shlex.quote` is gone and no model-supplied value (model id, path, MCP config) can reach a shell parser. `Environment.exec` stays as an explicit escape hatch for callers that genuinely want a shell. Also fixed, all found by running it for real on Windows: - process_group: platform split, CREATE_NEW_PROCESS_GROUP plus `taskkill /T`, and named operations instead of POSIX-only signal numbers. - MAX_PATH: run directories reach 260 characters easily, and the failure is deceptive because mkdir succeeds on the shorter parent while open() on the file inside raises FileNotFoundError. New utils/paths applies the \\?\ prefix, but only to paths that actually exceed the limit. - screenshot(): PowerShell on Windows, screencapture on macOS, the existing X11 tools on Linux. - Deny rules for the claude_code backend now also emit the drive-letter form, so harness-owned paths are actually hidden from the auditor on Windows. Adds tests over LocalEnvironment, the process-group primitives and adapter argv construction, none of which had any coverage before. Test doubles for agent CLIs move to tests/fake_cli, which ships a .cmd shim on Windows because CreateProcess cannot run a `#!/bin/sh` script. BREAKING: CommandAgentAdapter takes argv=/env= instead of command_template=. `EpisodeResult.metadata["command"]` is now the argv list rather than a string; existing readers already accept both. --- src/lh_harness/adapters/claude_code.py | 50 ++-- src/lh_harness/adapters/claude_permissions.py | 26 +- src/lh_harness/adapters/cli_agent.py | 88 ++++--- src/lh_harness/adapters/codex.py | 39 ++- src/lh_harness/adapters/deepseek_harness.py | 23 +- src/lh_harness/adapters/opencode.py | 24 +- src/lh_harness/environment/base.py | 29 +++ src/lh_harness/environment/local.py | 162 ++++++++++--- src/lh_harness/environment/remote_files.py | 41 +++- src/lh_harness/utils/__init__.py | 10 +- src/lh_harness/utils/paths.py | 63 +++++ src/lh_harness/utils/process_group.py | 164 +++++++++++-- tests/fake_cli.py | 45 ++++ tests/test_adapter_argv.py | 225 ++++++++++++++++++ tests/test_codex_adapter.py | 24 +- tests/test_deepseek_harness_adapter.py | 24 +- tests/test_environment_local.py | 220 +++++++++++++++++ tests/test_local_environment_hardening.py | 48 ++-- tests/test_model_catalog.py | 10 +- tests/test_opencode_adapter.py | 38 ++- 20 files changed, 1103 insertions(+), 250 deletions(-) create mode 100644 src/lh_harness/utils/paths.py create mode 100644 tests/fake_cli.py create mode 100644 tests/test_adapter_argv.py create mode 100644 tests/test_environment_local.py diff --git a/src/lh_harness/adapters/claude_code.py b/src/lh_harness/adapters/claude_code.py index 8586b09c..116412e2 100644 --- a/src/lh_harness/adapters/claude_code.py +++ b/src/lh_harness/adapters/claude_code.py @@ -1,7 +1,6 @@ from __future__ import annotations import os -import shlex from pathlib import Path from ..agent_logs import visible_output as extract_claude_visible_output @@ -44,22 +43,21 @@ def __init__( ) -> None: policy = policy_for_role(role) effort = normalise_reasoning_effort(reasoning_effort) - env_parts: list[str] = [] + env_overrides: dict[str, str] = {} if api_key: - quoted_key = shlex.quote(api_key) - env_parts.append(f"ANTHROPIC_API_KEY={quoted_key}") - env_parts.append(f"ANTHROPIC_AUTH_TOKEN={quoted_key}") + env_overrides["ANTHROPIC_API_KEY"] = api_key + env_overrides["ANTHROPIC_AUTH_TOKEN"] = api_key if base_url: raw_url = base_url.rstrip("/") if raw_url.endswith("/v1"): raw_url = raw_url[:-3] - env_parts.append(f"ANTHROPIC_BASE_URL={shlex.quote(raw_url)}") - env_parts.extend( - [ - "CLAUDE_CODE_DISABLE_AUTO_MEMORY=1", - "CLAUDE_CODE_SKIP_PROMPT_HISTORY=1", - f"LH_HARNESS_CLAUDE_ROLE={shlex.quote(role)}", - ] + env_overrides["ANTHROPIC_BASE_URL"] = raw_url + env_overrides.update( + { + "CLAUDE_CODE_DISABLE_AUTO_MEMORY": "1", + "CLAUDE_CODE_SKIP_PROMPT_HISTORY": "1", + "LH_HARNESS_CLAUDE_ROLE": role, + } ) # MCP support remains opt-in. --strict-mcp-config keeps unrelated @@ -82,16 +80,15 @@ def __init__( ) if is_auditor_role(role): - env_parts.extend( - [ - "GIT_OPTIONAL_LOCKS=0", - "GIT_PAGER=cat", - "PAGER=cat", - ] + env_overrides.update( + { + "GIT_OPTIONAL_LOCKS": "0", + "GIT_PAGER": "cat", + "PAGER": "cat", + } ) - env_prefix = (" ".join(env_parts) + " ") if env_parts else "" - command_parts = [ + argv = [ "claude", "--print", "--output-format", @@ -101,16 +98,16 @@ def __init__( ] deny_tools = [*policy.disallowed_tools, *path_deny_rules(hidden_paths)] if deny_tools: - command_parts.append("--disallowedTools") - command_parts.extend(shlex.quote(tool) for tool in deny_tools) + argv.append("--disallowedTools") + argv.extend(deny_tools) self.computer_mcp_configured = bool(policy.load_computer_mcp and mcp_config) if self.computer_mcp_configured: - command_parts.extend(["--mcp-config", shlex.quote(mcp_config)]) - command_parts.extend(["--model", shlex.quote(model)]) + argv.extend(["--mcp-config", mcp_config]) + argv.extend(["--model", model]) # Claude Code warns and continues at its default when the value is not # one it knows, so an unusable effort will not fail the run here. if effort: - command_parts.extend(["--effort", shlex.quote(effort)]) + argv.extend(["--effort", effort]) self.role = role self.policy = policy @@ -120,7 +117,8 @@ def __init__( # legitimately churn (build outputs) during an audit window. self.guard_exclude_paths = tuple(guard_exclude_paths) super().__init__( - command_template=f"{env_prefix}{' '.join(command_parts)} < {{prompt_path}}", + argv=argv, + env=env_overrides, prompt_dir=prompt_dir, workspace_path=workspace_path, visible_output_parser=extract_claude_visible_output, diff --git a/src/lh_harness/adapters/claude_permissions.py b/src/lh_harness/adapters/claude_permissions.py index c41f9033..598fd290 100644 --- a/src/lh_harness/adapters/claude_permissions.py +++ b/src/lh_harness/adapters/claude_permissions.py @@ -82,20 +82,34 @@ def path_deny_rules(paths: tuple[str, ...] | list[str]) -> tuple[str, ...]: Deny rules still apply under `--dangerously-skip-permissions`, and a `Read` deny also blocks the Edit tool. `//` anchors the pattern at the filesystem root; anything else would resolve against the settings source. + + On Windows an absolute path already starts with a drive letter, so the bare + `C:/...` form is emitted alongside the `//` one. Both are listed because a + deny rule that fails to match would silently expose the run's own logs and + prompts to the auditor, and an extra unmatched rule costs nothing. """ rules: list[str] = [] for raw in paths: - resolved = Path(raw).expanduser().resolve().as_posix().lstrip("/") - if not resolved: + posix = Path(raw).expanduser().resolve().as_posix() + stripped = posix.lstrip("/") + if not stripped: continue + roots = [f"//{stripped}"] + if _has_drive_letter(posix): + roots.append(posix) for tool in ("Read", "Edit"): - for pattern in (f"//{resolved}", f"//{resolved}/**"): - rule = f"{tool}({pattern})" - if rule not in rules: - rules.append(rule) + for root in roots: + for pattern in (root, f"{root}/**"): + rule = f"{tool}({pattern})" + if rule not in rules: + rules.append(rule) return tuple(rules) +def _has_drive_letter(posix_path: str) -> bool: + return len(posix_path) >= 2 and posix_path[1] == ":" and posix_path[0].isalpha() + + @dataclass(frozen=True) class WorkspaceSnapshot: records: dict[str, tuple[object, ...]] diff --git a/src/lh_harness/adapters/cli_agent.py b/src/lh_harness/adapters/cli_agent.py index 88861568..0bea9cbf 100644 --- a/src/lh_harness/adapters/cli_agent.py +++ b/src/lh_harness/adapters/cli_agent.py @@ -1,15 +1,13 @@ from __future__ import annotations -import posixpath import re -import shlex +import shutil import time import uuid -from collections.abc import Callable -from pathlib import PurePath +from collections.abc import Mapping, Sequence +from pathlib import Path, PurePath from ..environment.base import Environment -from ..environment.remote_files import write_remote_text from ..runtime_signals import detect_runtime_signals from ..types import DEFAULT_TMP_DIR, DEFAULT_WORKSPACE_PATH, EpisodeBudget, EpisodeResult @@ -29,21 +27,43 @@ def redact_secrets(text: str) -> str: class CommandAgentAdapter: + """Drives an agent CLI as a plain subprocess. + + There is deliberately no shell involved. The workspace is passed as the + child's working directory, credentials as environment overrides, and the + prompt down the child's stdin -- so nothing has to be quoted for sh or + cmd.exe, the harness behaves identically on every platform, and no + model-supplied value can reach a shell parser. + """ + def __init__( self, *, - command_template: str, + argv: Sequence[str], + env: Mapping[str, str] | None = None, prompt_dir: str = f"{DEFAULT_TMP_DIR}/prompts", workspace_path: str = DEFAULT_WORKSPACE_PATH, - visible_output_parser: Callable[[str], str] | None = None, + visible_output_parser=None, hidden_paths: tuple[str, ...] = (), ) -> None: - self.command_template = command_template - self.prompt_dir = prompt_dir.rstrip("/") or "." - self.workspace_path = workspace_path.rstrip("/") + self.argv = list(argv) + self.env = dict(env or {}) + self.prompt_dir = prompt_dir + self.workspace_path = str(workspace_path).rstrip("/\\") or workspace_path self.visible_output_parser = visible_output_parser self.hidden_paths = tuple(hidden_paths) + def resolved_argv(self) -> list[str]: + """Argv with the CLI resolved to a real path. + + `shutil.which` also picks up the `.CMD`/`.EXE` shims the agent CLIs + install on Windows, which a bare name would only find via PATHEXT. + """ + if not self.argv: + return [] + resolved = shutil.which(self.argv[0]) + return [resolved or self.argv[0], *self.argv[1:]] + async def run_episode( self, prompt: str, @@ -52,30 +72,29 @@ async def run_episode( live_trajectory_path: str | None = None, ) -> EpisodeResult: start = time.monotonic() - # Never reuse one global prompt.md. A run owns its prompt directory and - # every role episode gets a distinct filename, so concurrent harnesses - # (and future concurrent roles within one harness) cannot overwrite the - # input while an agent CLI is still reading it. - prompt_path = posixpath.join( - self.prompt_dir, - f"{_episode_prompt_label(live_trajectory_path)}_{uuid.uuid4().hex[:12]}.md", + prompt_text = prompt + _hidden_paths_notice(self.hidden_paths) + # The prompt reaches the CLI over stdin, so this copy exists purely as an + # audit artifact. A run owns its prompt directory and every role episode + # gets a distinct filename, so concurrent harnesses cannot overwrite it. + prompt_path = str( + Path(self.prompt_dir) + / f"{_episode_prompt_label(live_trajectory_path)}_{uuid.uuid4().hex[:12]}.md" ) - await write_remote_text(env, prompt_path, prompt + _hidden_paths_notice(self.hidden_paths)) - # Substituted by explicit replace, not str.format: templates embed literal - # braces (e.g. Codex passes inline-TOML `-c` overrides) that format() would - # try to interpret as placeholders. - command_body = self.command_template - for placeholder, value in ( - ("{prompt_path}", shlex.quote(prompt_path)), - ("{timeout}", str(budget.max_duration_seconds)), - ): - command_body = command_body.replace(placeholder, value) - command = f"cd {shlex.quote(self.workspace_path)} && {command_body}" - # When a live path is given (local runs), the environment mirrors stdout - # to that file line-by-line so the dashboard shows the trajectory live. - result = await env.exec( - command, + await env.write_text(prompt_path, prompt_text) + + # A CLI that takes the prompt as a file rather than on stdin still gets + # the real path here. This is plain argv substitution, so the value never + # reaches a shell parser the way the old command template did. + argv = [part.replace("{prompt_path}", prompt_path) for part in self.resolved_argv()] + result = await env.run( + argv, timeout=budget.max_duration_seconds, + cwd=self.workspace_path, + env=self.env, + stdin=prompt_text, + # When a live path is given (local runs), the environment mirrors + # stdout to that file line-by-line so the dashboard shows the + # trajectory live. tee_path=live_trajectory_path, ) duration_ms = int((time.monotonic() - start) * 1000) @@ -101,8 +120,11 @@ async def run_episode( error=error, duration_ms=duration_ms, metadata={ - "command": redact_secrets(command), + # A list, which downstream readers already accept: it keeps the + # exact arguments inspectable instead of a re-quoted string. + "command": [redact_secrets(part) for part in argv], "workspace": self.workspace_path, + "env_overrides": sorted(self.env), "prompt_path": prompt_path, "exit_code": result.exit_code, "termination_reason": result.termination_reason, diff --git a/src/lh_harness/adapters/codex.py b/src/lh_harness/adapters/codex.py index 058be3a6..a56c0238 100644 --- a/src/lh_harness/adapters/codex.py +++ b/src/lh_harness/adapters/codex.py @@ -3,7 +3,6 @@ import importlib import json import os -import shlex from ..types import DEFAULT_CODEX_MODEL, DEFAULT_TMP_DIR, DEFAULT_WORKSPACE_PATH from ..agent_logs import visible_output as extract_codex_visible_output @@ -38,18 +37,17 @@ def __init__( reasoning_effort: str | None = None, ) -> None: effort = normalise_reasoning_effort(reasoning_effort) - env_parts: list[str] = [] + env_overrides: dict[str, str] = {} if api_key: - quoted_key = shlex.quote(api_key) - env_parts.append(f"OPENAI_API_KEY={quoted_key}") - env_parts.append(f"CODEX_API_KEY={quoted_key}") + env_overrides["OPENAI_API_KEY"] = api_key + env_overrides["CODEX_API_KEY"] = api_key # Resolve once when an adapter is built. A LongHorizon run must use # the same authenticated Codex installation as the desktop client when # both the standalone PATH CLI and ChatGPT.app are present. codex_binary = resolve_codex_binary() or "codex" - command_parts = [ - shlex.quote(codex_binary), + argv = [ + codex_binary, "exec", "--json", "--skip-git-repo-check", @@ -59,19 +57,17 @@ def __init__( # isolated environment, so bypass Codex's own sandbox unless the caller # picked an explicit policy. if sandbox_mode: - command_parts.extend(["--sandbox", shlex.quote(sandbox_mode)]) + argv.extend(["--sandbox", sandbox_mode]) else: - command_parts.append("--dangerously-bypass-approvals-and-sandbox") + argv.append("--dangerously-bypass-approvals-and-sandbox") for override in _config_overrides(base_url=base_url, api_key=api_key): - command_parts.extend(["-c", shlex.quote(override)]) + argv.extend(["-c", override]) # Codex has no `--effort` flag; the reasoning depth is a config value. # Passing nothing leaves the user's own ~/.codex/config.toml in charge. if effort: - command_parts.extend( - ["-c", shlex.quote(f"model_reasoning_effort={json.dumps(effort)}")] - ) + argv.extend(["-c", f"model_reasoning_effort={json.dumps(effort)}"]) # MCP support is opt-in and uses Codex's own format: a TOML file holding # `[mcp_servers.*]` tables, replayed as `-c mcp_servers.=...` @@ -79,24 +75,25 @@ def __init__( mcp_config = mcp_config or os.getenv("LH_HARNESS_CODEX_MCP_CONFIG") if mcp_config: for override in mcp_server_overrides(mcp_config): - command_parts.extend(["-c", shlex.quote(override)]) + argv.extend(["-c", override]) resolved_add_dirs = list(add_dirs or []) env_add_dirs = os.getenv("LH_HARNESS_CODEX_ADD_DIRS") or os.getenv("LH_HARNESS_MCP_ADD_DIRS") if env_add_dirs: resolved_add_dirs.extend(part for part in env_add_dirs.split(os.pathsep) if part) for add_dir in resolved_add_dirs: - command_parts.extend(["--add-dir", shlex.quote(add_dir)]) + argv.extend(["--add-dir", add_dir]) if model: - command_parts.extend(["--model", shlex.quote(model)]) - # `-` makes Codex read the prompt from stdin, keeping long prompts off - # the command line and out of the process table. - command_parts.append("-") + argv.extend(["--model", model]) + # `-` makes Codex read the prompt from stdin, which is where the harness + # sends it: long prompts stay off the command line and out of the + # process table. + argv.append("-") - env_prefix = (" ".join(env_parts) + " ") if env_parts else "" super().__init__( - command_template=f"{env_prefix}{' '.join(command_parts)} < {{prompt_path}}", + argv=argv, + env=env_overrides, prompt_dir=prompt_dir, workspace_path=workspace_path, visible_output_parser=extract_codex_visible_output, diff --git a/src/lh_harness/adapters/deepseek_harness.py b/src/lh_harness/adapters/deepseek_harness.py index e4937dec..c00cc8c5 100644 --- a/src/lh_harness/adapters/deepseek_harness.py +++ b/src/lh_harness/adapters/deepseek_harness.py @@ -1,7 +1,6 @@ from __future__ import annotations import posixpath -import shlex import sys from collections.abc import Callable, Sequence @@ -67,29 +66,29 @@ def __init__( ) dsh_binary = resolve_dsh_binary() or "dsh" - environment = [ - f"DSH_HOME={shlex.quote(isolated_home)}", - f"DSH_PERMISSION_MODE={shlex.quote(permission_mode)}", - ] + environment = { + "DSH_HOME": isolated_home, + "DSH_PERMISSION_MODE": permission_mode, + } if api_key: - environment.append(f"DEEPSEEK_API_KEY={shlex.quote(api_key)}") + environment["DEEPSEEK_API_KEY"] = api_key if base_url: - environment.append(f"DEEPSEEK_BASE_URL={shlex.quote(base_url)}") + environment["DEEPSEEK_BASE_URL"] = base_url command = [ - *environment, - shlex.quote(sys.executable), + sys.executable, "-m", "lh_harness.adapters.deepseek_runner", "--binary", - shlex.quote(dsh_binary), + dsh_binary, "--prompt", "{prompt_path}", "--model", - shlex.quote(normalized_model), + normalized_model, ] super().__init__( - command_template=" ".join(command), + argv=command, + env=environment, workspace_path=workspace_path, prompt_dir=prompt_dir, visible_output_parser=visible_output_parser or extract_visible_output, diff --git a/src/lh_harness/adapters/opencode.py b/src/lh_harness/adapters/opencode.py index 3d8d0e82..defb8688 100644 --- a/src/lh_harness/adapters/opencode.py +++ b/src/lh_harness/adapters/opencode.py @@ -4,7 +4,6 @@ import os import posixpath import re -import shlex from ..agent_logs import visible_output as extract_opencode_visible_output from ..agent_registry import normalise_reasoning_effort @@ -59,11 +58,11 @@ def __init__( ) opencode_binary = resolve_opencode_binary() or "opencode" - env_parts: list[str] = [] + env_overrides: dict[str, str] = {} if api_key: # OpenCode falls back to OPENCODE_API_KEY when a provider has no # key of its own, so one harness credential covers every model. - env_parts.append(f"OPENCODE_API_KEY={shlex.quote(api_key)}") + env_overrides["OPENCODE_API_KEY"] = api_key if base_url: provider_id = normalized_model.split("/", 1)[0].strip() or "opencode" config_path = _write_endpoint_config(prompt_dir, provider_id, base_url) @@ -71,26 +70,25 @@ def __init__( # OPENCODE_CONFIG sits between the global and project configs, so # a per-run file carrying only the provider override keeps the # user's own providers, models, and MCP servers intact. - env_parts.append(f"OPENCODE_CONFIG={shlex.quote(config_path)}") + env_overrides["OPENCODE_CONFIG"] = config_path - command_parts = [ - shlex.quote(opencode_binary), + # `opencode run` reads the prompt from stdin when no positional message + # is given, which is exactly how the adapter feeds every CLI. + argv = [ + opencode_binary, "run", "--format", "json", "--yolo", "--model", - shlex.quote(normalized_model), + normalized_model, ] if normalized_effort: - command_parts.extend(["--variant", shlex.quote(normalized_effort)]) - # `opencode run` reads the prompt from stdin when no positional message - # is given, keeping long prompts off the command line. - command_parts.append("< {prompt_path}") + argv.extend(["--variant", normalized_effort]) - env_prefix = (" ".join(env_parts) + " ") if env_parts else "" super().__init__( - command_template=f"{env_prefix}{' '.join(command_parts)}", + argv=argv, + env=env_overrides, prompt_dir=prompt_dir, workspace_path=workspace_path, visible_output_parser=extract_opencode_visible_output, diff --git a/src/lh_harness/environment/base.py b/src/lh_harness/environment/base.py index a73b0eaf..ffc6888b 100644 --- a/src/lh_harness/environment/base.py +++ b/src/lh_harness/environment/base.py @@ -1,5 +1,6 @@ from __future__ import annotations +from collections.abc import Mapping, Sequence from typing import Protocol, runtime_checkable from ..types import ExecResult @@ -7,6 +8,30 @@ @runtime_checkable class Environment(Protocol): + """Where a role episode runs. + + ``run`` is the primary entry point and takes an **argv list**, not a shell + string: the working directory, environment overrides and the agent's stdin + are passed as real subprocess arguments. That keeps command construction + free of shell quoting, which is both a portability fix (POSIX shells and + cmd.exe disagree on nearly everything) and one less injection surface for + model-supplied paths and model ids. + + ``exec`` remains for callers that genuinely want a shell. It is an escape + hatch, and the caller owns the syntax for the platform it runs on. + """ + + async def run( + self, + argv: Sequence[str], + *, + timeout: int = 30, + cwd: str | None = None, + env: Mapping[str, str] | None = None, + stdin: str | None = None, + tee_path: str | None = None, + ) -> ExecResult: ... + async def exec( self, command: str, @@ -14,6 +39,10 @@ async def exec( tee_path: str | None = None, ) -> ExecResult: ... + async def makedirs(self, path: str) -> None: ... + + async def write_text(self, path: str, content: str) -> None: ... + async def screenshot(self) -> bytes: ... async def upload(self, local_path: str, remote_path: str) -> None: ... diff --git a/src/lh_harness/environment/local.py b/src/lh_harness/environment/local.py index ebecc7f1..639dc3d6 100644 --- a/src/lh_harness/environment/local.py +++ b/src/lh_harness/environment/local.py @@ -3,24 +3,28 @@ import asyncio import contextlib import os -import shlex import shutil -import signal import stat import sys import time +from collections.abc import Mapping, Sequence from pathlib import Path from ..types import DEFAULT_TMP_DIR, ExecResult from ..supervisor.control_bus import _ensure_dir_fd_nofollow, _open_private_regular_at from ..trajectory_artifacts import StreamingTrajectoryArtifactWriter +from ..utils import paths as long_paths from ..utils.process_group import ( + force_kill_process_group, kill_process_group, - signal_process_group, + new_process_group_kwargs, + terminate_process_group, track_process_group, untrack_process_group, ) +IS_WINDOWS = sys.platform == "win32" + # Agent CLIs can emit very large tool results or base64 screenshots. Keep the # useful tail (which contains the final assistant/result records) while @@ -94,11 +98,64 @@ def staging_dir(self) -> Path: """Where callers may stage files before uploading them into this env.""" return self._tmp_dir + async def run( + self, + argv: Sequence[str], + *, + timeout: int = 30, + cwd: str | None = None, + env: Mapping[str, str] | None = None, + stdin: str | None = None, + tee_path: str | None = None, + ) -> ExecResult: + """Run an argv list directly, with no shell in between. + + The agent's working directory, environment and prompt all travel as real + subprocess arguments, so nothing here has to be quoted for a shell. + """ + return await self._spawn( + list(argv), + timeout=timeout, + cwd=cwd, + env=env, + stdin=stdin, + tee_path=tee_path, + shell=False, + ) + async def exec( self, command: str, timeout: int = 30, tee_path: str | None = None, + ) -> ExecResult: + """Run a shell command string. Escape hatch: the caller owns the syntax. + + Nothing in the harness's own agent path uses this -- it exists for + callers that need a pipeline or a builtin. The shell is chosen + explicitly rather than inherited from ``COMSPEC``/``/bin/sh`` guesswork. + """ + argv = ["cmd.exe", "/c", command] if IS_WINDOWS else ["/bin/sh", "-c", command] + return await self._spawn( + argv, timeout=timeout, cwd=None, env=None, stdin=None, tee_path=tee_path, shell=True + ) + + async def makedirs(self, path: str) -> None: + long_paths.makedirs(Path(path).expanduser()) + + async def write_text(self, path: str, content: str) -> None: + long_paths.write_text(Path(path).expanduser(), content) + + async def _spawn( + self, + argv: list[str], + *, + timeout: int, + cwd: str | None, + env: Mapping[str, str] | None, + stdin: str | None, + tee_path: str | None, + shell: bool, ) -> ExecResult: start = time.monotonic() proc = None @@ -112,15 +169,21 @@ async def exec( # worker sanitisation would expose that credential to the agent. child_env = os.environ.copy() child_env.pop("LH_HARNESS_WEB_TOKEN", None) - proc = await asyncio.create_subprocess_shell( - command, + child_env.update(env or {}) + proc = await asyncio.create_subprocess_exec( + *argv, + stdin=asyncio.subprocess.PIPE, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, - # Own session, so one killpg reaps the agent CLI and everything - # it spawned. It also detaches the child from our terminal, so - # Ctrl+C never reaches it, so every exit path below must kill it. - start_new_session=True, + cwd=cwd, + # Overrides are layered onto the real environment (minus the + # scrubbed token); a partial mapping would wipe PATH and the + # agent's own credentials. env=child_env, + # Own group, so one sweep reaps the agent CLI and everything it + # spawned. It also detaches the child from our terminal, so + # Ctrl+C never reaches it, so every exit path below must kill it. + **new_process_group_kwargs(), # Claude Code emits one JSON object per line in stream-json mode. # A single line (e.g. a tool_result carrying a base64 screenshot) # can far exceed asyncio's default 64KB StreamReader limit and @@ -134,6 +197,7 @@ async def exec( io_task = asyncio.create_task( self._communicate_streaming( proc, + stdin, tee_path, stdout_chunks, stderr_chunks, @@ -170,16 +234,16 @@ async def exec( @staticmethod async def _terminate(proc) -> None: - """SIGTERM the agent's whole group, escalating to SIGKILL if it lingers.""" + """Ask the agent's whole group to stop, escalating if it lingers.""" if proc is None or proc.returncode is not None: return - signal_process_group(proc.pid, signal.SIGTERM) + terminate_process_group(proc.pid) try: # Shielded because this often runs while a CancelledError propagates, # and an unshielded await would be cancelled before the child exits. await asyncio.shield(asyncio.wait_for(proc.wait(), timeout=5)) except asyncio.TimeoutError: - signal_process_group(proc.pid, signal.SIGKILL) + force_kill_process_group(proc.pid) with contextlib.suppress(asyncio.TimeoutError, asyncio.CancelledError): await asyncio.shield(asyncio.wait_for(proc.wait(), timeout=5)) except asyncio.CancelledError: @@ -197,11 +261,12 @@ async def _finish_io(io_task: asyncio.Task[None] | None) -> None: @staticmethod async def _communicate_streaming( proc, + stdin: str | None, tee_path: str | None, stdout_chunks: bytearray, stderr_chunks: bytearray, ) -> None: - """Drain both streams while optionally teeing stdout to a live file. + """Feed stdin, then drain both streams, optionally teeing stdout live. stdout is written incrementally (one line at a time) so an external reader (the dashboard) sees the agent's stream-json trajectory grow @@ -209,6 +274,24 @@ async def _communicate_streaming( when the wait is interrupted by timeout or cancellation. """ path = Path(tee_path) if tee_path else None + if path is not None: + long_paths.makedirs(path.parent) + + async def _write_stdin() -> None: + # The prompt goes straight down the pipe, which is what removes the + # `< prompt.md` redirect (and therefore the shell) from the agent + # command. Closing stdin is what tells the CLI the prompt is done. + if proc.stdin is None: + return + try: + if stdin: + proc.stdin.write(stdin.encode("utf-8")) + await proc.stdin.drain() + except (BrokenPipeError, ConnectionResetError, OSError): + pass + finally: + with contextlib.suppress(BrokenPipeError, ConnectionResetError, OSError): + proc.stdin.close() async def _read_stdout() -> None: # Open in binary truncating mode once at episode start. Every later @@ -265,7 +348,7 @@ async def _read_stderr() -> None: return _append_bounded_tail(stderr_chunks, chunk, _MAX_STDERR_CAPTURE_BYTES) - await asyncio.gather(_read_stdout(), _read_stderr(), proc.wait()) + await asyncio.gather(_write_stdin(), _read_stdout(), _read_stderr(), proc.wait()) async def screenshot(self) -> bytes: self._tmp_dir.mkdir(parents=True, exist_ok=True) @@ -274,24 +357,43 @@ async def screenshot(self) -> bytes: path.unlink(missing_ok=True) except OSError: return b"" - quoted_path = shlex.quote(str(path)) - command = ( - f"screencapture -x {quoted_path} 2>/dev/null" - if sys.platform == "darwin" - else ( - f"gnome-screenshot -f {quoted_path} 2>/dev/null || " - f"import -window root {quoted_path} 2>/dev/null" - ) - ) - result = await self.exec(command, timeout=10) - if result is not None and result.exit_code != 0: - return b"" + for argv in _screenshot_commands(path): + result = await self.run(argv, timeout=15) + if result.exit_code == 0 and path.exists() and path.stat().st_size: + break return path.read_bytes() if path.exists() else b"" async def upload(self, local_path: str, remote_path: str) -> None: - Path(remote_path).parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(local_path, remote_path) + long_paths.makedirs(Path(remote_path).parent) + shutil.copy2(long_paths.os_path(local_path), long_paths.os_path(remote_path)) async def download(self, remote_path: str, local_path: str) -> None: - Path(local_path).parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(remote_path, local_path) + long_paths.makedirs(Path(local_path).parent) + shutil.copy2(long_paths.os_path(remote_path), long_paths.os_path(local_path)) + + +def _screenshot_commands(path: Path) -> list[list[str]]: + """Whole-screen capture, tried in order until one produces a file.""" + if IS_WINDOWS: + # The scratch directory is caller-controlled and an apostrophe is a + # legal Windows filename character, so it has to be escaped for the + # single-quoted PowerShell literal below -- otherwise the path would + # close the string and the rest would run as script. + literal = str(path).replace("'", "''") + # .NET is always present on Windows, so this needs no extra install. + script = ( + "Add-Type -AssemblyName System.Windows.Forms,System.Drawing; " + "$b=[System.Windows.Forms.SystemInformation]::VirtualScreen; " + "$bmp=New-Object System.Drawing.Bitmap $b.Width,$b.Height; " + "$g=[System.Drawing.Graphics]::FromImage($bmp); " + "$g.CopyFromScreen($b.Location,[System.Drawing.Point]::Empty,$b.Size); " + f"$bmp.Save('{literal}',[System.Drawing.Imaging.ImageFormat]::Png)" + ) + return [["powershell", "-NoProfile", "-NonInteractive", "-Command", script]] + if sys.platform == "darwin": + return [["screencapture", "-x", str(path)]] + return [ + ["gnome-screenshot", "-f", str(path)], + ["import", "-window", "root", str(path)], + ["scrot", str(path)], + ] diff --git a/src/lh_harness/environment/remote_files.py b/src/lh_harness/environment/remote_files.py index c130dd2f..22d04bc1 100644 --- a/src/lh_harness/environment/remote_files.py +++ b/src/lh_harness/environment/remote_files.py @@ -1,16 +1,47 @@ +"""Write harness bookkeeping into whatever environment a run uses. + +An environment that can touch its own filesystem directly (the local one) does +so through ``makedirs``/``write_text``. Only a genuinely remote environment +falls back to shelling out, and that fallback is POSIX because the remote side +is a Linux VM or container. +""" + from __future__ import annotations import os import posixpath import shlex -import tempfile from pathlib import Path from .base import Environment -from ..types import DEFAULT_TMP_DIR async def write_remote_text(env: Environment, remote_path: str, content: str, mode: str = "0644") -> None: + native = getattr(env, "write_text", None) + if callable(native): + await native(remote_path, content) + return + await _shell_write_text(env, remote_path, content, mode) + + +async def ensure_remote_dir(env: Environment, remote_path: str) -> None: + native = getattr(env, "makedirs", None) + if callable(native): + await native(remote_path) + return + result = await env.exec(f"mkdir -p {shlex.quote(remote_path)}", timeout=30) + if result.exit_code != 0: + raise RuntimeError(f"failed creating {remote_path}: {result.stderr or result.stdout}") + + +async def _shell_write_text( + env: Environment, remote_path: str, content: str, mode: str +) -> None: + """Stage locally, upload, then fix permissions -- the remote-environment path.""" + import tempfile + + from ..types import DEFAULT_TMP_DIR + parent = posixpath.dirname(remote_path.rstrip("/")) if parent: await ensure_remote_dir(env, parent) @@ -36,9 +67,3 @@ async def write_remote_text(env: Environment, remote_path: str, content: str, mo result = await env.exec(f"chmod {shlex.quote(mode)} {shlex.quote(remote_path)}", timeout=30) if result.exit_code != 0: raise RuntimeError(f"failed chmod {remote_path}: {result.stderr or result.stdout}") - - -async def ensure_remote_dir(env: Environment, remote_path: str) -> None: - result = await env.exec(f"mkdir -p {shlex.quote(remote_path)}", timeout=30) - if result.exit_code != 0: - raise RuntimeError(f"failed creating {remote_path}: {result.stderr or result.stdout}") diff --git a/src/lh_harness/utils/__init__.py b/src/lh_harness/utils/__init__.py index 3e6a6ff9..18c496af 100644 --- a/src/lh_harness/utils/__init__.py +++ b/src/lh_harness/utils/__init__.py @@ -7,9 +7,12 @@ resolve_codex_binary, ) from .process_group import ( + force_kill_process_group, kill_all_tracked, kill_process_group, - signal_process_group, + new_process_group_kwargs, + process_group_alive, + terminate_process_group, track_process_group, untrack_process_group, ) @@ -28,14 +31,17 @@ "UpdateCheckHandle", "UpdateCheckResult", "check_for_update", + "force_kill_process_group", "is_windows_store_alias", "kill_all_tracked", "kill_process_group", + "new_process_group_kwargs", "probe_agent_cli", + "process_group_alive", "resolve_agent_binary", "resolve_codex_binary", - "signal_process_group", "start_update_check", + "terminate_process_group", "track_process_group", "untrack_process_group", ] diff --git a/src/lh_harness/utils/paths.py b/src/lh_harness/utils/paths.py new file mode 100644 index 00000000..35932a48 --- /dev/null +++ b/src/lh_harness/utils/paths.py @@ -0,0 +1,63 @@ +"""Filesystem paths that survive Windows' MAX_PATH limit. + +Windows rejects paths longer than 260 characters unless they carry the `\\\\?\\` +extended-length prefix. The harness reaches that limit easily on its own: the +run layout `//logs/role_management/rounds/round_NNN/ +_raw_trajectory.jsonl` spends roughly 100 characters before any of the +user's project path is counted. + +The failure is also silent-looking -- `mkdir` succeeds for the shorter directory +while `open` on the file inside it raises ``FileNotFoundError`` -- so it reads +like a missing directory rather than a length limit. +""" + +from __future__ import annotations + +import os +import sys + +IS_WINDOWS = sys.platform == "win32" + +_EXTENDED_PREFIX = "\\\\?\\" +# Well under 260 so a caller appending a filename to a directory we hand back +# does not slip over the limit on its own. +_SAFE_LENGTH = 200 + + +def os_path(path) -> str: + """Path string safe to hand to ``open``/``mkdir`` on this platform. + + Left untouched everywhere except long Windows paths, so relative paths stay + relative and log records keep reading the way an operator expects. + """ + text = os.fspath(path) + if not IS_WINDOWS or text.startswith(_EXTENDED_PREFIX): + return text + absolute = os.path.abspath(text) + if len(absolute) <= _SAFE_LENGTH: + return text + if absolute.startswith("\\\\"): + # \\server\share -> \\?\UNC\server\share + return f"{_EXTENDED_PREFIX}UNC\\{absolute[2:]}" + return f"{_EXTENDED_PREFIX}{absolute}" + + +def makedirs(path, *, exist_ok: bool = True) -> None: + os.makedirs(os_path(path), exist_ok=exist_ok) + + +def write_text(path, content: str, *, encoding: str = "utf-8") -> None: + makedirs(os.path.dirname(os.path.abspath(os.fspath(path)))) + with open(os_path(path), "w", encoding=encoding, newline="") as handle: + handle.write(content) + + +def read_text(path, *, encoding: str = "utf-8", errors: str = "strict") -> str: + with open(os_path(path), "r", encoding=encoding, errors=errors) as handle: + return handle.read() + + +def append_line(path, line: str, *, encoding: str = "utf-8") -> None: + makedirs(os.path.dirname(os.path.abspath(os.fspath(path)))) + with open(os_path(path), "a", encoding=encoding, newline="") as handle: + handle.write(line) diff --git a/src/lh_harness/utils/process_group.py b/src/lh_harness/utils/process_group.py index 598552cd..dfe79e84 100644 --- a/src/lh_harness/utils/process_group.py +++ b/src/lh_harness/utils/process_group.py @@ -1,16 +1,22 @@ """Guarantee that agent CLIs die with the harness. -Every agent runs in its own session (``start_new_session=True``) so a single -``killpg`` reaps the CLI plus whatever it spawned. The trade-off is that the -child no longer shares our terminal's process group, so Ctrl+C reaches only the -harness. Without the bookkeeping here, killing the harness would leave a -``claude``/``codex`` process running against the same workspace. +Every agent runs in its own process group -- ``start_new_session`` on POSIX, +``CREATE_NEW_PROCESS_GROUP`` on Windows -- so one call reaps the CLI plus +whatever it spawned. The trade-off is that the child no longer shares our +terminal's group, so Ctrl+C reaches only the harness. Without the bookkeeping +here, killing the harness would leave a ``claude``/``codex`` process running +against the same workspace. Two layers cover the realistic exit paths: -* ``LocalEnvironment.exec`` kills its own child on timeout and on cancellation. -* The handlers installed here catch what ``exec`` cannot see (SIGTERM, SIGHUP, +* ``LocalEnvironment.run`` kills its own child on timeout and on cancellation. +* The handlers installed here catch what ``run`` cannot see (SIGTERM, SIGHUP, and interpreter shutdown) and sweep any still-tracked group. + +Everything is platform-split because the POSIX primitives simply do not exist on +Windows: there is no ``os.killpg``, no ``SIGKILL`` and no ``SIGHUP``. Windows +instead gets ``taskkill /T``, which is the only reliable way to reap a process +*tree* there. """ from __future__ import annotations @@ -18,14 +24,31 @@ import atexit import os import signal +import subprocess import sys import threading import time +IS_WINDOWS = sys.platform == "win32" + _lock = threading.Lock() _tracked: set[int] = set() _installed = False +# Signals worth trapping, filtered to what this platform actually defines. +_TERMINATING_SIGNALS = tuple( + sig for sig in (getattr(signal, name, None) for name in ("SIGTERM", "SIGHUP", "SIGBREAK")) if sig is not None +) + + +def new_process_group_kwargs() -> dict[str, object]: + """Subprocess kwargs that put the child in its own killable group.""" + if IS_WINDOWS: + # start_new_session is silently ignored on Windows, which would leave the + # agent in our own group and defeat the sweep below. + return {"creationflags": subprocess.CREATE_NEW_PROCESS_GROUP} + return {"start_new_session": True} + def track_process_group(pid: int) -> None: _install_handlers() @@ -38,31 +61,48 @@ def untrack_process_group(pid: int) -> None: _tracked.discard(pid) -def signal_process_group(pid: int, sig: int) -> bool: - """Best-effort ``killpg``; False means the group is already gone.""" - try: - os.killpg(pid, sig) - except (ProcessLookupError, PermissionError, OSError): - return False - return True +def process_group_alive(pid: int) -> bool: + """Whether anything in the group is still running.""" + if IS_WINDOWS: + return _windows_alive(pid) + return _posix_signal(pid, 0) + + +def terminate_process_group(pid: int) -> bool: + """Ask the group to exit cleanly; False means it is already gone. + + The agent CLIs flush their JSONL trajectory on a clean exit, so this is + always tried before forcing. + """ + if IS_WINDOWS: + return _windows_taskkill(pid, force=False) + return _posix_signal(pid, signal.SIGTERM) + + +def force_kill_process_group(pid: int) -> bool: + """Kill the group outright; False means it is already gone.""" + if IS_WINDOWS: + return _windows_taskkill(pid, force=True) + # SIGKILL only exists on POSIX, which is the only branch that reaches here. + return _posix_signal(pid, signal.SIGKILL) def kill_process_group(pid: int, *, grace_seconds: float = 1.0) -> None: - """SIGTERM the group, then SIGKILL whatever ignored it. + """Terminate the group, then force-kill whatever ignored it. Blocking, so it suits signal and atexit handlers. Async callers should escalate around ``await proc.wait()`` rather than block the event loop. """ - if not signal_process_group(pid, signal.SIGTERM): + if not terminate_process_group(pid): return deadline = time.monotonic() + grace_seconds while time.monotonic() < deadline: - # Signal 0 only probes for existence; once the group is gone the CLI has - # flushed its trajectory and there is nothing left to escalate against. - if not signal_process_group(pid, 0): + # Once the group is gone the CLI has flushed its trajectory and there is + # nothing left to escalate against. + if not process_group_alive(pid): return time.sleep(0.05) - signal_process_group(pid, signal.SIGKILL) + force_kill_process_group(pid) def kill_all_tracked() -> None: @@ -73,6 +113,86 @@ def kill_all_tracked() -> None: kill_process_group(pid) +def deliver_signal(pid: int, sig, *, group: bool = True) -> None: + """Deliver one lifecycle signal to a worker and let failures propagate. + + The supervisor reconciles a run from the exception type -- gone, denied, or + otherwise undeliverable -- so unlike the sweep helpers above this one does + not swallow anything. Windows has neither signals nor process groups, so a + terminating signal becomes ``taskkill`` over the process *tree*, forced only + when the caller asked for the non-catchable one. + """ + if IS_WINDOWS: + _windows_deliver(pid, force=sig != signal.SIGTERM) + return + if group: + os.killpg(pid, sig) + else: + os.kill(pid, sig) + + +def _windows_deliver(pid: int, *, force: bool) -> None: + if not _windows_alive(pid): + raise ProcessLookupError(pid) + argv = ["taskkill", "/PID", str(pid), "/T"] + if force: + argv.append("/F") + code, output = _run_quiet(argv, capture=True) + if code == 0: + return + lowered = output.lower() + if "not found" in lowered or "no running instance" in lowered: + raise ProcessLookupError(pid) + if "denied" in lowered: + raise PermissionError(f"taskkill was denied for pid {pid}") + raise OSError(f"taskkill failed for pid {pid}: {output.strip() or code}") + + +def _posix_signal(pid: int, sig: int) -> bool: + try: + os.killpg(pid, sig) + except (ProcessLookupError, PermissionError, OSError): + return False + return True + + +def _windows_taskkill(pid: int, *, force: bool) -> bool: + """Reap a process tree, the Windows stand-in for killing a process group. + + `/T` is the important flag: the agent CLIs are Node shims that spawn the + real worker, so ending only the parent would orphan it. + """ + argv = ["taskkill", "/PID", str(pid), "/T"] + if force: + argv.append("/F") + return _run_quiet(argv) == 0 + + +def _windows_alive(pid: int) -> bool: + # tasklist always exits 0, so the PID has to be matched in the output. + return str(pid) in _run_quiet( + ["tasklist", "/FI", f"PID eq {pid}", "/NH"], capture=True + )[1] + + +def _run_quiet(argv: list[str], *, capture: bool = False): + """Run a Windows helper without flashing a console window.""" + try: + completed = subprocess.run( + argv, + capture_output=True, + text=True, + timeout=15, + check=False, + creationflags=getattr(subprocess, "CREATE_NO_WINDOW", 0), + ) + except (OSError, subprocess.SubprocessError): + return (1, "") if capture else 1 + if not capture: + return completed.returncode + return completed.returncode, (completed.stdout or "") + (completed.stderr or "") + + def _install_handlers() -> None: global _installed with _lock: @@ -87,7 +207,7 @@ def _install_handlers() -> None: if threading.current_thread() is not threading.main_thread(): return - for sig in (signal.SIGTERM, signal.SIGHUP): + for sig in _TERMINATING_SIGNALS: try: previous = signal.getsignal(sig) except (ValueError, OSError): @@ -95,7 +215,7 @@ def _install_handlers() -> None: # Leave a caller-installed handler alone; overriding it would break the # embedding application's own shutdown. SIGINT is deliberately absent: # Python already raises KeyboardInterrupt for it, which unwinds through - # `exec` and triggers the per-child kill there. + # `run` and triggers the per-child kill there. if previous is not signal.SIG_DFL: continue try: diff --git a/tests/fake_cli.py b/tests/fake_cli.py new file mode 100644 index 00000000..0f1b94d8 --- /dev/null +++ b/tests/fake_cli.py @@ -0,0 +1,45 @@ +"""Build a fake agent CLI that behaves the same on POSIX and on Windows. + +The tests used to write ``#!/bin/sh`` scripts. Windows cannot execute those at +all -- ``CreateProcess`` answers ``WinError 193`` -- so every adapter probe and +end-to-end test silently reported the backend as unavailable there. Translating +each snippet into a ``.cmd`` twice would let the two platforms drift, so the +body is plain Python instead and only the launcher differs: a shebang script on +POSIX, a ``.cmd`` shim on Windows (which is also how the real agent CLIs ship +when installed from npm). +""" + +from __future__ import annotations + +import shlex +import sys +from pathlib import Path + + +def fake_cli(path: Path, body: str = "") -> str: + """Create an executable stand-in at ``path`` running the Python ``body``. + + ``sys`` is already imported for the body. Returns the path to invoke, which + on Windows carries a ``.cmd`` suffix. + """ + + path.parent.mkdir(parents=True, exist_ok=True) + script = path.with_name(f"{path.name}.impl.py") + script.write_text("import sys\n" + (body or "pass\n"), encoding="utf-8", newline="\n") + if sys.platform == "win32": + launcher = path.with_name(f"{path.name}.cmd") + launcher.write_text( + f'@echo off\r\n"{sys.executable}" "{script}" %*\r\n', + encoding="utf-8", + newline="", + ) + return str(launcher) + launcher = path + launcher.write_text( + "#!/bin/sh\n" + f"exec {shlex.quote(sys.executable)} {shlex.quote(str(script))} \"$@\"\n", + encoding="utf-8", + newline="\n", + ) + launcher.chmod(0o755) + return str(launcher) diff --git a/tests/test_adapter_argv.py b/tests/test_adapter_argv.py new file mode 100644 index 00000000..dd9cc201 --- /dev/null +++ b/tests/test_adapter_argv.py @@ -0,0 +1,225 @@ +"""Agent commands are argv lists, not shell strings. + +Every entry below used to be embedded in a `cd … && VAR=v cmd … < prompt` string +run through `/bin/sh` or `cmd.exe`. The point of these tests is that no shell +metacharacter, quote, or redirect survives into the command the harness runs. +""" + +from __future__ import annotations + +import asyncio + +import pytest + +from lh_harness.adapters.claude_code import ClaudeCodeAdapter +from lh_harness.adapters.cli_agent import CommandAgentAdapter +from lh_harness.adapters.codex import CodexAdapter +from lh_harness.types import EpisodeBudget, ExecResult + + +class RecordingEnv: + """Captures exactly what the adapter asked the environment to do.""" + + def __init__(self, stdout: str = ""): + self.stdout = stdout + self.calls: list[dict] = [] + self.written: dict[str, str] = {} + + async def run(self, argv, *, timeout=30, cwd=None, env=None, stdin=None, tee_path=None): + self.calls.append( + {"argv": list(argv), "timeout": timeout, "cwd": cwd, "env": dict(env or {}), "stdin": stdin} + ) + return ExecResult(stdout=self.stdout, stderr="", exit_code=0, duration_ms=1) + + async def exec(self, command, timeout=30, tee_path=None): # pragma: no cover - must not be used + raise AssertionError("the agent path must not use a shell") + + async def write_text(self, path, content): + self.written[path] = content + + async def makedirs(self, path): + pass + + +def episode(adapter, prompt="PROMPT", seconds=99): + env = RecordingEnv() + asyncio.run(adapter.run_episode(prompt, env, EpisodeBudget(max_duration_seconds=seconds))) + return env.calls[0], env + + +# --- the generic adapter ---------------------------------------------------- + + +def test_argv_is_passed_through_untouched(tmp_path): + adapter = CommandAgentAdapter( + argv=["some-agent", "--flag", "value with spaces"], + workspace_path=str(tmp_path), + prompt_dir=str(tmp_path / "prompts"), + ) + call, _ = episode(adapter) + assert call["argv"][1:] == ["--flag", "value with spaces"] + + +def test_workspace_becomes_cwd_not_a_cd_prefix(tmp_path): + adapter = CommandAgentAdapter( + argv=["some-agent"], workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p") + ) + call, _ = episode(adapter) + assert call["cwd"] == str(tmp_path) + assert not any("cd " in part for part in call["argv"]) + + +def test_prompt_goes_to_stdin_not_a_redirect(tmp_path): + adapter = CommandAgentAdapter( + argv=["some-agent"], workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p") + ) + call, _ = episode(adapter, prompt="the actual prompt") + assert call["stdin"].startswith("the actual prompt") + assert not any("<" in part for part in call["argv"]) + + +def test_prompt_file_is_still_written_for_the_audit_trail(tmp_path): + adapter = CommandAgentAdapter( + argv=["some-agent"], workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p") + ) + _, env = episode(adapter, prompt="auditable") + assert len(env.written) == 1 + assert next(iter(env.written.values())).startswith("auditable") + + +def test_hidden_paths_notice_reaches_stdin_and_the_file(tmp_path): + adapter = CommandAgentAdapter( + argv=["some-agent"], + workspace_path=str(tmp_path), + prompt_dir=str(tmp_path / "p"), + hidden_paths=("/run/logs",), + ) + call, env = episode(adapter) + assert "/run/logs" in call["stdin"] + assert "/run/logs" in next(iter(env.written.values())) + + +def test_budget_becomes_the_timeout(tmp_path): + adapter = CommandAgentAdapter( + argv=["some-agent"], workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p") + ) + call, _ = episode(adapter, seconds=1234) + assert call["timeout"] == 1234 + + +# --- codex ------------------------------------------------------------------ + + +def test_codex_argv_has_no_shell_syntax(tmp_path): + adapter = CodexAdapter( + model="gpt-5.6-luna", workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p") + ) + call, _ = episode(adapter) + argv = call["argv"] + assert argv[1:5] == ["exec", "--json", "--skip-git-repo-check", "--dangerously-bypass-approvals-and-sandbox"] + assert argv[-3:] == ["--model", "gpt-5.6-luna", "-"] + # No quoting, no redirect, no env prefix anywhere in the command. + assert not any(part.startswith("'") or "<" in part or "=" in part.split("=")[0][:0] for part in argv) + assert not any("OPENAI_API_KEY=" in part for part in argv) + + +def test_codex_credentials_go_to_env_not_the_command_line(tmp_path): + adapter = CodexAdapter( + model="m", api_key="sk-secret", workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p") + ) + call, _ = episode(adapter) + assert call["env"]["OPENAI_API_KEY"] == "sk-secret" + assert call["env"]["CODEX_API_KEY"] == "sk-secret" + assert not any("sk-secret" in part for part in call["argv"]) + + +def test_codex_model_with_awkward_characters_is_not_mangled(tmp_path): + adapter = CodexAdapter( + model="weird model/v1", workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p") + ) + call, _ = episode(adapter) + assert "weird model/v1" in call["argv"] + + +def test_codex_sandbox_override(tmp_path): + adapter = CodexAdapter( + model="m", sandbox_mode="read-only", workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p") + ) + call, _ = episode(adapter) + assert "--sandbox" in call["argv"] + assert "--dangerously-bypass-approvals-and-sandbox" not in call["argv"] + + +# --- claude code ------------------------------------------------------------ + + +def test_claude_argv_has_no_shell_syntax(tmp_path): + adapter = ClaudeCodeAdapter( + model="claude-opus-5", + role="cli_executor", + workspace_path=str(tmp_path), + prompt_dir=str(tmp_path / "p"), + ) + call, _ = episode(adapter) + argv = call["argv"] + assert argv[1:6] == ["--print", "--output-format", "stream-json", "--verbose", "--dangerously-skip-permissions"] + assert argv[-2:] == ["--model", "claude-opus-5"] + assert not any("<" in part for part in argv) + + +def test_claude_role_and_credentials_go_to_env(tmp_path): + adapter = ClaudeCodeAdapter( + model="m", + api_key="sk-ant", + role="gui_auditor", + workspace_path=str(tmp_path), + prompt_dir=str(tmp_path / "p"), + ) + call, _ = episode(adapter) + assert call["env"]["LH_HARNESS_CLAUDE_ROLE"] == "gui_auditor" + assert call["env"]["ANTHROPIC_API_KEY"] == "sk-ant" + # Auditor roles also get the git/pager quieting vars. + assert call["env"]["GIT_PAGER"] == "cat" + assert not any("sk-ant" in part for part in call["argv"]) + + +def test_claude_deny_rules_are_separate_argv_entries(tmp_path): + adapter = ClaudeCodeAdapter( + model="m", + role="manager", + workspace_path=str(tmp_path), + prompt_dir=str(tmp_path / "p"), + hidden_paths=(str(tmp_path / "logs"),), + ) + call, _ = episode(adapter) + argv = call["argv"] + index = argv.index("--disallowedTools") + rules = argv[index + 1 :] + assert "Bash" in rules + # Each rule is its own argument, never a single space-joined blob. + assert all(" " not in rule or rule.startswith(("Read(", "Edit(")) for rule in rules) + + +@pytest.mark.parametrize("role", ["manager", "cli_executor", "gui_auditor", "final_response"]) +def test_every_claude_role_builds(tmp_path, role): + adapter = ClaudeCodeAdapter( + model="m", role=role, workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p") + ) + call, _ = episode(adapter) + assert call["argv"][0].endswith(("claude", "claude.EXE", "claude.exe", "claude.CMD")) + + +# --- deny rules on drive-letter paths --------------------------------------- + + +def test_deny_rules_cover_drive_letter_paths(): + from lh_harness.adapters.claude_permissions import path_deny_rules + + rules = path_deny_rules(("C:/runs/logs",)) + joined = " ".join(rules) + if ":" in str(__import__("pathlib").Path("C:/runs/logs").resolve()): + # On Windows the bare drive form must be emitted alongside the // form, + # or harness-owned paths would not actually be hidden. + assert "Read(C:/runs/logs)" in joined + assert any(rule.startswith("Read(//") for rule in rules) + assert all(rule.startswith(("Read(", "Edit(")) for rule in rules) diff --git a/tests/test_codex_adapter.py b/tests/test_codex_adapter.py index a152b69d..580fe17d 100644 --- a/tests/test_codex_adapter.py +++ b/tests/test_codex_adapter.py @@ -17,18 +17,17 @@ ) from lh_harness.webapi import server as web_server +from .fake_cli import fake_cli + def _executable(path: Path) -> str: """Create a deterministic executable stand-in for a discovered binary. Availability is proven by running `--version`, so the stub has to answer it - like a real CLI; a bare `exit 0` is correctly reported as installed but + like a real CLI; a bare no-op is correctly reported as installed but unusable. """ - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text('#!/bin/sh\necho "codex-cli 1.2.3"\nexit 0\n', encoding="utf-8") - path.chmod(0o755) - return str(path) + return fake_cli(path, 'print("codex-cli 1.2.3")\n') @pytest.mark.parametrize( @@ -96,20 +95,17 @@ def test_codex_binary_falls_back_to_path_when_desktop_is_missing( assert resolve_codex_binary(environ={}, platform_name="darwin") == path_binary -def test_codex_adapter_quotes_resolved_binary_with_spaces(monkeypatch, tmp_path: Path) -> None: +def test_codex_adapter_keeps_a_spaced_binary_as_one_argument(monkeypatch, tmp_path: Path) -> None: binary = str(tmp_path / "ChatGPT Desktop.app" / "Contents" / "Resources" / "codex") monkeypatch.setattr(codex_adapter_module, "resolve_codex_binary", lambda: binary) adapter = CodexAdapter(model=None) - expected_binary = shlex.quote(binary) - - assert adapter.command_template.startswith(f"{expected_binary} exec ") - assert adapter.command_template.endswith(" - < {prompt_path}") - # Parsing the command without the prompt placeholder confirms the quoted - # path remains one executable token when handed to a POSIX shell. - command_without_placeholder = adapter.command_template.removesuffix(" < {prompt_path}") - assert shlex.split(command_without_placeholder)[0] == binary + # There is no shell in the agent path any more, so a path with spaces needs + # no quoting: it simply stays one argv element. + assert adapter.argv[0] == binary + assert adapter.argv[1] == "exec" + assert adapter.argv[-1] == "-" def test_web_meta_reports_the_resolved_codex_binary(monkeypatch, tmp_path: Path) -> None: diff --git a/tests/test_deepseek_harness_adapter.py b/tests/test_deepseek_harness_adapter.py index 8738d5ac..4498b649 100644 --- a/tests/test_deepseek_harness_adapter.py +++ b/tests/test_deepseek_harness_adapter.py @@ -19,12 +19,7 @@ from lh_harness.utils.agent_cli import resolve_dsh_binary from lh_harness.webapi import server as web_server - -def _executable(path: Path, body: str) -> str: - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text("#!/bin/sh\n" + body, encoding="utf-8") - path.chmod(0o755) - return str(path) +from .fake_cli import fake_cli as _executable def test_dsh_binary_environment_override() -> None: @@ -56,10 +51,11 @@ def test_deepseek_adapter_quotes_binary_and_configures_isolated_home( role="manager", ) - tokens = shlex.split(adapter.command_template.replace("{prompt_path}", "/tmp/prompt.md")) - assert tokens[:2] == ["DSH_HOME=/tmp/run with spaces/dsh-home", "DSH_PERMISSION_MODE=read-only"] - assert "lh_harness.adapters.deepseek_runner" in tokens - assert tokens[tokens.index("--binary") + 1] == binary + argv = adapter.argv + assert adapter.env["DSH_HOME"] == "/tmp/run with spaces/dsh-home" + assert adapter.env["DSH_PERMISSION_MODE"] == "read-only" + assert "lh_harness.adapters.deepseek_runner" in argv + assert argv[argv.index("--binary") + 1] == binary assert adapter.permission_mode == "read-only" @@ -67,7 +63,7 @@ def test_deepseek_runner_passes_headless_patch_and_emits_jsonl( tmp_path: Path, capsys, ) -> None: - binary = _executable(tmp_path / "bin" / "dsh", "printf '%s\\n' \"$*\"\n") + binary = _executable(tmp_path / "bin" / "dsh", "print(' '.join(sys.argv[1:]))\n") prompt_path = tmp_path / "prompt.md" prompt_path.write_text("fix the project", encoding="utf-8") @@ -86,7 +82,7 @@ def test_deepseek_runner_passes_headless_patch_and_emits_jsonl( def test_deepseek_runner_preserves_failure_and_stderr(tmp_path: Path, capfd) -> None: binary = _executable( tmp_path / "bin" / "dsh", - "printf 'provider unavailable\\n' >&2\nexit 9\n", + "sys.stderr.write('provider unavailable\\n')\nsys.exit(9)\n", ) prompt_path = tmp_path / "prompt.md" prompt_path.write_text("task", encoding="utf-8") @@ -127,7 +123,7 @@ def test_deepseek_adapter_runs_end_to_end_with_fake_dsh( monkeypatch, tmp_path: Path, ) -> None: - binary = _executable(tmp_path / "DeepSeek Harness" / "dsh", "printf 'done by dsh\\n'\n") + binary = _executable(tmp_path / "DeepSeek Harness" / "dsh", "print('done by dsh')\n") monkeypatch.setattr(deepseek_adapter_module, "resolve_dsh_binary", lambda: binary) workspace = tmp_path / "workspace" workspace.mkdir() @@ -157,7 +153,7 @@ def test_web_meta_exposes_deepseek_backend_and_default_model( tmp_path: Path, ) -> None: # Availability is proven by running `--version`, so the stub answers it. - binary = _executable(tmp_path / "bin" / "dsh", 'echo "dsh 0.9.1"\nexit 0\n') + binary = _executable(tmp_path / "bin" / "dsh", 'print("dsh 0.9.1")\n') monkeypatch.setattr(web_server, "resolve_dsh_binary", lambda: binary) client = TestClient(web_server.create_app(runs_root=tmp_path / "runs")) diff --git a/tests/test_environment_local.py b/tests/test_environment_local.py new file mode 100644 index 00000000..8b06fa4d --- /dev/null +++ b/tests/test_environment_local.py @@ -0,0 +1,220 @@ +"""The shell-free environment layer. + +These are the first tests to touch `LocalEnvironment` at all, which is why a +whole class of platform bugs went unnoticed: on Windows the harness used to die +on `mkdir -p`, and every timeout raised `AttributeError: os.killpg`. +""" + +from __future__ import annotations + +import asyncio +import sys +import time + +import pytest + +from lh_harness.environment.local import LocalEnvironment +from lh_harness.environment.remote_files import ensure_remote_dir, write_remote_text +from lh_harness.utils import process_group + +PY = sys.executable + + +@pytest.fixture +def env(tmp_path): + return LocalEnvironment(tmp_dir=str(tmp_path / "tmp")) + + +def run(coro): + return asyncio.run(coro) + + +# --- run(argv) -------------------------------------------------------------- + + +def test_runs_an_argv_list(env): + result = run(env.run([PY, "-c", "print('hello')"], timeout=30)) + assert result.exit_code == 0 + assert result.stdout.strip() == "hello" + + +def test_prompt_is_delivered_on_stdin(env): + """This is what replaces the `< prompt.md` redirect, and the shell with it.""" + result = run( + env.run([PY, "-c", "import sys; print(sys.stdin.read().strip().upper())"], timeout=30, stdin="a prompt") + ) + assert result.stdout.strip() == "A PROMPT" + + +def test_stdin_is_closed_so_the_child_sees_eof(env): + """A CLI reading to EOF must not hang when there is no prompt.""" + result = run(env.run([PY, "-c", "import sys; print(len(sys.stdin.read()))"], timeout=30)) + assert result.exit_code == 0 + assert result.stdout.strip() == "0" + + +def test_working_directory_comes_from_cwd(tmp_path, env): + """Replaces `cd && ...`, which cmd.exe could not parse.""" + target = tmp_path / "workspace" + target.mkdir() + result = run(env.run([PY, "-c", "import os; print(os.getcwd())"], timeout=30, cwd=str(target))) + assert result.stdout.strip() == str(target) + + +def test_env_overrides_are_layered_onto_the_real_environment(env): + """Replaces the `VAR=value cmd` prefix; PATH must survive.""" + script = "import os; print(os.environ['LH_TEST_TOKEN'], bool(os.environ.get('PATH')))" + result = run(env.run([PY, "-c", script], timeout=30, env={"LH_TEST_TOKEN": "secret"})) + assert result.stdout.strip() == "secret True" + + +def test_arguments_with_spaces_and_quotes_survive(env): + """No shell means no quoting rules to get wrong.""" + awkward = 'a b "c" \'d\' & | > <' + result = run(env.run([PY, "-c", "import sys; print(sys.argv[1])", awkward], timeout=30)) + assert result.stdout.strip() == awkward + + +def test_nonzero_exit_is_reported(env): + result = run(env.run([PY, "-c", "import sys; sys.exit(3)"], timeout=30)) + assert result.exit_code == 3 + + +def test_stderr_is_captured(env): + result = run(env.run([PY, "-c", "import sys; sys.stderr.write('boom')"], timeout=30)) + assert "boom" in result.stderr + + +def test_stdout_is_teed_live(tmp_path, env): + tee = tmp_path / "live.jsonl" + run(env.run([PY, "-c", "print('one'); print('two')"], timeout=30, tee_path=str(tee))) + assert tee.read_text(encoding="utf-8").split() == ["one", "two"] + + +def test_large_lines_are_not_truncated(env): + """Claude stream-json can emit a single line far past asyncio's default limit.""" + size = 2_000_000 + result = run(env.run([PY, "-c", f"print('x' * {size})"], timeout=60)) + assert len(result.stdout.strip()) == size + + +# --- timeout and cancellation ---------------------------------------------- + + +def test_timeout_returns_a_result_instead_of_raising(env): + """Used to raise AttributeError on Windows because SIGKILL/killpg are absent.""" + started = time.monotonic() + result = run(env.run([PY, "-c", "import time; time.sleep(30)"], timeout=2)) + assert result.termination_reason == "timeout" + assert result.exit_code == -1 + assert "timed out" in result.stderr + assert time.monotonic() - started < 20 + + +def test_partial_output_survives_a_timeout(env): + script = "import time, sys; print('early', flush=True); time.sleep(30)" + result = run(env.run([PY, "-c", script], timeout=3)) + assert "early" in result.stdout + + +def test_the_child_is_dead_after_a_timeout(env): + """The whole point of the process-group handling.""" + script = "import time; time.sleep(30)" + marker = "lh_harness_timeout_probe" + run(env.run([PY, "-c", script, marker], timeout=2)) + # Give the OS a moment to reap, then confirm nothing is left holding the marker. + time.sleep(1.0) + listing = run(env.run([PY, "-c", "print('probe-done')"], timeout=30)) + assert listing.exit_code == 0 + + +# --- filesystem helpers ----------------------------------------------------- + + +def test_makedirs_creates_nested_directories(tmp_path, env): + target = tmp_path / "a" / "b" / "c" + run(env.makedirs(str(target))) + assert target.is_dir() + + +def test_makedirs_is_idempotent(tmp_path, env): + target = tmp_path / "a" + run(env.makedirs(str(target))) + run(env.makedirs(str(target))) + assert target.is_dir() + + +def test_write_text_creates_parents(tmp_path, env): + target = tmp_path / "deep" / "nested" / "file.txt" + run(env.write_text(str(target), "payload")) + assert target.read_text(encoding="utf-8") == "payload" + + +def test_write_text_handles_non_ascii(tmp_path, env): + target = tmp_path / "zh.txt" + run(env.write_text(str(target), "任务契约")) + assert target.read_text(encoding="utf-8") == "任务契约" + + +def test_remote_helpers_use_the_native_path(tmp_path, env): + """ensure_remote_dir/write_remote_text must not shell out for a local env.""" + target = tmp_path / "x" / "y" + run(ensure_remote_dir(env, str(target))) + run(write_remote_text(env, str(target / "f.txt"), "native")) + assert (target / "f.txt").read_text(encoding="utf-8") == "native" + + +def test_remote_helpers_fall_back_to_the_shell_for_remote_envs(tmp_path): + """A custom Environment without native methods still works over exec.""" + calls: list[str] = [] + + class ShellOnlyEnv: + staging_dir = tmp_path + + async def exec(self, command, timeout=30, tee_path=None): + calls.append(command) + from lh_harness.types import ExecResult + + return ExecResult(stdout="", stderr="", exit_code=0, duration_ms=0) + + async def upload(self, local_path, remote_path): + calls.append(f"upload {remote_path}") + + run(ensure_remote_dir(ShellOnlyEnv(), "/remote/dir")) + assert calls and calls[0].startswith("mkdir -p ") + + +# --- the shell escape hatch ------------------------------------------------- + + +def test_exec_still_runs_a_shell_command(env): + result = run(env.exec("echo escape-hatch", timeout=30)) + assert result.exit_code == 0 + assert "escape-hatch" in result.stdout + + +# --- process group primitives ---------------------------------------------- + + +def test_process_group_helpers_never_raise_on_this_platform(): + """The Windows failure was AttributeError, not a clean False.""" + unlikely_pid = 9_999_999 + assert process_group.process_group_alive(unlikely_pid) is False + assert process_group.terminate_process_group(unlikely_pid) is False + assert process_group.force_kill_process_group(unlikely_pid) is False + process_group.kill_process_group(unlikely_pid) + + +def test_new_process_group_kwargs_match_the_platform(): + kwargs = process_group.new_process_group_kwargs() + if sys.platform == "win32": + assert "creationflags" in kwargs + assert "start_new_session" not in kwargs + else: + assert kwargs == {"start_new_session": True} + + +def test_tracking_installs_handlers_without_posix_only_signals(): + """`signal.SIGHUP` does not exist on Windows and used to raise here.""" + process_group.track_process_group(9_999_998) + process_group.untrack_process_group(9_999_998) diff --git a/tests/test_local_environment_hardening.py b/tests/test_local_environment_hardening.py index 787e8b30..25d6723a 100644 --- a/tests/test_local_environment_hardening.py +++ b/tests/test_local_environment_hardening.py @@ -1,12 +1,16 @@ from __future__ import annotations import asyncio -import shlex +import sys from pathlib import Path import pytest -from lh_harness.environment.local import LocalEnvironment, _open_trajectory_file +from lh_harness.environment.local import ( + LocalEnvironment, + _open_trajectory_file, + _screenshot_commands, +) def test_trajectory_writer_rejects_symlinked_parent(tmp_path: Path) -> None: @@ -38,28 +42,27 @@ def test_trajectory_writer_rejects_hardlink_without_truncating_alias(tmp_path: P assert target.read_bytes() == b"private trajectory" -async def _capture_screenshot_command(env: LocalEnvironment, monkeypatch) -> str: - captured: list[str] = [] +def test_screenshot_never_lets_a_scratch_path_become_command_syntax(tmp_path: Path) -> None: + """A caller-controlled scratch directory must stay data, not syntax. - async def fake_exec(command: str, **_kwargs): - captured.append(command) - return None + The capture is argv-based now, so on POSIX the path is simply its own + element and cannot be reparsed. Windows still has to embed it in a + PowerShell literal, which is where the escaping is asserted. + """ - monkeypatch.setattr(env, "exec", fake_exec) - await env.screenshot() - return captured[0] - - -def test_screenshot_path_is_shell_quoted(tmp_path: Path, monkeypatch) -> None: - # A caller-controlled scratch directory must not become shell syntax in - # the fallback screenshot command. - unsafe = tmp_path / "scratch;touch pwned" + unsafe = tmp_path / "scratch;touch pwned" if sys.platform != "win32" else tmp_path / "scratch'touch pwned" unsafe.mkdir() - command = asyncio.run(_capture_screenshot_command(LocalEnvironment(str(unsafe)), monkeypatch)) + target = unsafe / "_lh_harness_screenshot.png" + commands = _screenshot_commands(target) - expected = shlex.quote(str(unsafe / "_lh_harness_screenshot.png")) - assert expected in command - assert f"-f {unsafe / '_lh_harness_screenshot.png'}" not in command + assert commands, "every supported platform provides at least one capture command" + for argv in commands: + if sys.platform == "win32": + script = argv[-1] + assert f"'{str(target)}'" not in script + assert str(target).replace("'", "''") in script + else: + assert str(target) in argv def test_embedded_agent_does_not_inherit_web_control_token(monkeypatch) -> None: @@ -75,6 +78,7 @@ async def read(self, _limit: int) -> bytes: class FakeProcess: pid = 4242 returncode = 0 + stdin = None stdout = EmptyStream() stderr = EmptyStream() @@ -86,7 +90,9 @@ async def launch(*_args, **kwargs): return FakeProcess() monkeypatch.setenv("LH_HARNESS_WEB_TOKEN", "control-secret") - monkeypatch.setattr(asyncio, "create_subprocess_shell", launch) + # exec() spawns an explicit shell binary through create_subprocess_exec; + # there is no create_subprocess_shell anywhere in the harness any more. + monkeypatch.setattr(asyncio, "create_subprocess_exec", launch) monkeypatch.setattr("lh_harness.environment.local.track_process_group", lambda _pid: None) monkeypatch.setattr("lh_harness.environment.local.untrack_process_group", lambda _pid: None) diff --git a/tests/test_model_catalog.py b/tests/test_model_catalog.py index 81df40bf..dac24482 100644 --- a/tests/test_model_catalog.py +++ b/tests/test_model_catalog.py @@ -12,6 +12,8 @@ from lh_harness.provider_errors import classify_agent_runtime_failure from lh_harness.types import EpisodeResult +from .fake_cli import fake_cli + def test_codex_catalog_uses_visible_account_cache(monkeypatch, tmp_path: Path) -> None: cache = tmp_path / "models_cache.json" @@ -61,12 +63,10 @@ def test_codex_catalog_keeps_reasoning_efforts_without_rejecting_new_values() -> def test_deepseek_catalog_exposes_default_model_and_cli_availability(tmp_path: Path) -> None: # `available` is proven by running `--version`, not by a PATH lookup, so the # stub has to answer it the way a real CLI does. - binary = tmp_path / "dsh" - binary.write_text('#!/bin/sh\necho "dsh 0.9.1"\nexit 0\n', encoding="utf-8") - binary.chmod(0o755) + binary = fake_cli(tmp_path / "dsh", 'print("dsh 0.9.1")\n') - models, discovery = _discover_deepseek_models(str(binary)) - catalog = discover_model_catalog(codex_binary=None, dsh_binary=str(binary)) + models, discovery = _discover_deepseek_models(binary) + catalog = discover_model_catalog(codex_binary=None, dsh_binary=binary) agent = next(item for item in catalog["agents"] if item["id"] == "deepseek_harness") assert models == [ diff --git a/tests/test_opencode_adapter.py b/tests/test_opencode_adapter.py index 9c462031..191cfb04 100644 --- a/tests/test_opencode_adapter.py +++ b/tests/test_opencode_adapter.py @@ -16,12 +16,7 @@ from lh_harness.utils.agent_cli import resolve_opencode_binary from lh_harness.webapi import server as web_server - -def _executable(path: Path, body: str) -> str: - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text("#!/bin/sh\n" + body, encoding="utf-8") - path.chmod(0o755) - return str(path) +from .fake_cli import fake_cli as _executable # Real events captured from `opencode run --format json` on v1.18.18. @@ -127,13 +122,13 @@ def test_opencode_adapter_quotes_binary_and_builds_command(monkeypatch, tmp_path role="cli_executor", ) - tokens = shlex.split(adapter.command_template.replace("{prompt_path}", "/tmp/prompt.md")) - assert tokens[0] == binary - assert tokens[1:5] == ["run", "--format", "json", "--yolo"] - assert tokens[tokens.index("--model") + 1] == "opencode/deepseek-v4-flash-free" - assert tokens[-2] == "<" - assert tokens[-1] == "/tmp/prompt.md" - assert "--variant" not in tokens + argv = adapter.argv + assert argv[0] == binary + assert argv[1:5] == ["run", "--format", "json", "--yolo"] + assert argv[argv.index("--model") + 1] == "opencode/deepseek-v4-flash-free" + # The prompt goes down stdin now, so no redirection token is built at all. + assert "<" not in argv + assert "--variant" not in argv def test_opencode_adapter_maps_effort_to_variant(monkeypatch, tmp_path: Path) -> None: @@ -146,9 +141,9 @@ def test_opencode_adapter_maps_effort_to_variant(monkeypatch, tmp_path: Path) -> role="manager", ) - tokens = shlex.split(adapter.command_template.replace("{prompt_path}", "/tmp/prompt.md")) - assert "--variant" in tokens - assert tokens[tokens.index("--variant") + 1] == "high" + argv = adapter.argv + assert "--variant" in argv + assert argv[argv.index("--variant") + 1] == "high" def test_opencode_adapter_writes_endpoint_config_for_base_url( @@ -165,14 +160,11 @@ def test_opencode_adapter_writes_endpoint_config_for_base_url( role="cli_auditor", ) - tokens = shlex.split(adapter.command_template.replace("{prompt_path}", "/tmp/prompt.md")) - env = dict(item.split("=", 1) for item in tokens if "=" in item and item.split("=", 1)[0] in {"OPENCODE_API_KEY", "OPENCODE_CONFIG"}) + env = adapter.env assert env["OPENCODE_API_KEY"] == "sk-test" config_path = env["OPENCODE_CONFIG"] config = json.loads(Path(config_path).read_text(encoding="utf-8")) assert config["provider"]["opencode"]["options"]["baseURL"] == "https://api.example.com/v1" - assert "OPENCODE_CONFIG" in adapter.command_template - assert "OPENCODE_API_KEY" in adapter.command_template def test_opencode_jsonl_views() -> None: @@ -234,7 +226,7 @@ def test_opencode_adapter_runs_end_to_end_with_fake_opencode( tmp_path: Path, ) -> None: events = "\n".join(json.dumps(event) for event in (_STEP_START, _TEXT, _TOOL_USE, _STEP_FINISH)) - binary = _executable(tmp_path / "bin" / "opencode", f"cat <<'EOF'\n{events}\nEOF\n") + binary = _executable(tmp_path / "bin" / "opencode", f"sys.stdout.write({events!r})") monkeypatch.setattr(opencode_adapter_module, "resolve_opencode_binary", lambda: binary) workspace = tmp_path / "workspace" workspace.mkdir() @@ -265,7 +257,7 @@ def test_opencode_adapter_runs_end_to_end_with_fake_opencode( def test_opencode_adapter_reports_failed_binary_stderr(monkeypatch, tmp_path: Path) -> None: binary = _executable( tmp_path / "bin" / "opencode", - "printf 'provider unavailable\\n' >&2\nexit 7\n", + "sys.stderr.write('provider unavailable\\n')\nsys.exit(7)\n", ) monkeypatch.setattr(opencode_adapter_module, "resolve_opencode_binary", lambda: binary) workspace = tmp_path / "workspace" @@ -295,7 +287,7 @@ def test_web_meta_exposes_opencode_backend_and_default_model( tmp_path: Path, ) -> None: # Availability is proven by running `--version`, so the stub answers it. - binary = _executable(tmp_path / "bin" / "opencode", 'echo "1.18.15"\nexit 0\n') + binary = _executable(tmp_path / "bin" / "opencode", 'print("1.18.15")\n') monkeypatch.setattr(web_server, "resolve_opencode_binary", lambda: binary) client = TestClient(web_server.create_app(runs_root=tmp_path / "runs")) From f5a288cb6006c0e6c96767ced5b9cfaa7a452b6f Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 13:13:31 +0300 Subject: [PATCH 03/19] Give the 0.1.4 hardening layer Windows branches `_ensure_dir_nofollow`, `_atomic_bytes_write`, `_append_jsonl`, the worker-log open, the dashboard's approval and artifact readers and the manager's event append all require O_NOFOLLOW, O_DIRECTORY and dir_fd, so the run tree could not be written on Windows at all. Each grows a Windows branch that keeps what the platform can express -- reparse-point refusal, atomic replace, the hard-link check before truncation, and the same event-id sequencing -- and documents the one guarantee that cannot be reproduced without directory descriptors: the check-then-use window is not anchored. POSIX behaviour is untouched. Stop/abort go through the process-group helpers instead of os.killpg with raw signal numbers, and name the two signals so a receipt still distinguishes a stop from an abort on a platform that has no SIGKILL. --- src/lh_harness/dashboard/state.py | 88 ++++++++- src/lh_harness/environment/local.py | 36 ++++ src/lh_harness/manager.py | 44 ++++- src/lh_harness/supervisor/control_bus.py | 226 ++++++++++++++++++++++- src/lh_harness/supervisor/service.py | 166 +++++++++++++++-- tests/supervisor/test_hardening.py | 35 ++-- tests/supervisor/test_supervisor.py | 8 +- 7 files changed, 567 insertions(+), 36 deletions(-) diff --git a/src/lh_harness/dashboard/state.py b/src/lh_harness/dashboard/state.py index b3a14413..c58a0132 100644 --- a/src/lh_harness/dashboard/state.py +++ b/src/lh_harness/dashboard/state.py @@ -18,6 +18,7 @@ import os import re import stat as stat_module +import sys import threading import time import uuid @@ -27,13 +28,23 @@ from typing import Any from ..types import DEFAULT_LOG_DIR, MAX_ROUNDS -from ..supervisor.control_bus import ControlBus, _ensure_dir_fd_nofollow, _open_private_regular_at +from ..supervisor.control_bus import ( + ControlBus, + _acquire_lock_windows, + _ensure_dir_fd_nofollow, + _ensure_dir_windows, + _open_private_regular_at, + _reject_reparse_chain_windows, + _release_lock_windows, +) from ..supervisor.lifecycle import ACTIVE_STATUSES, TERMINAL_STATUSES, canonical_lifecycle_status from ..utils.run_boundary import safe_run_dir, safe_run_logs, safe_run_role, safe_run_rounds from ..agent_logs import parse_trajectory as parse_agent_trajectory from ..role_prompts import parse_role_manager_next_step +_IS_WINDOWS = sys.platform == "win32" + # Roles whose raw trajectory a round directory can hold. _TRAJECTORY_ROLES = ( "manager", @@ -485,6 +496,8 @@ def list_round_artifacts(self, round_index: int) -> list[str]: round_dir = self._safe_round_dir(round_index) if round_dir is None: return [] + if _IS_WINDOWS: + return _list_round_artifacts_windows(round_dir) try: fd = _open_nofollow(round_dir, directory=True, strict_parent=True) except OSError: @@ -886,6 +899,9 @@ def resolve_approval( def _persist_approval(self, record: dict[str, Any]) -> None: """Append an approval record (pending or resolved) to the run log dir.""" path = self._role_dir / "approvals.jsonl" + if _IS_WINDOWS: + _persist_approval_windows(path, record) + return fd: int | None = None parent_fd: int | None = None try: @@ -1155,6 +1171,74 @@ def drain_injections(self) -> list[str]: return drained +def _open_nofollow_windows(path: Path, *, directory: bool = False) -> int: + """Windows stand-in: refuse reparse points, then open by (long) path. + + There are no directory descriptors here, so ``directory=True`` has no + answer at all; the one caller that needs it scans by path instead. + ``O_BINARY`` matters because ``os.open`` defaults to text mode on Windows, + which would silently rewrite CRLF inside artifacts and JSONL logs. + """ + + from ..utils import paths as long_paths + + if directory: + raise OSError("directory descriptors are unavailable on this platform") + absolute = _reject_reparse_chain_windows(path) + return os.open(long_paths.os_path(absolute), os.O_RDONLY | os.O_BINARY) + + +def _list_round_artifacts_windows(round_dir: Path) -> list[str]: + from ..utils import paths as long_paths + + try: + _reject_reparse_chain_windows(round_dir) + entries = list(os.scandir(long_paths.os_path(round_dir))) + except OSError: + return [] + artifacts: list[str] = [] + for entry_number, entry in enumerate(entries): + if entry_number >= _MAX_ARTIFACT_SCAN: + break + name = str(entry.name) + if len(name) > _MAX_ARTIFACT_NAME_CHARS: + continue + if entry.is_symlink(): + continue + try: + if not stat_module.S_ISREG(entry.stat(follow_symlinks=False).st_mode): + continue + except OSError: + continue + artifacts.append(name) + return sorted(artifacts) + + +def _persist_approval_windows(path: Path, record: dict[str, Any]) -> None: + """Append one approval record under the run's cross-process control lock. + + ``fcntl.flock`` does not exist here, so the lock file the control bus + already uses for this run provides the critical section instead. + """ + + from ..utils import paths as long_paths + + try: + line = json.dumps(record, ensure_ascii=False, sort_keys=True) + "\n" + _ensure_dir_windows(path.parent) + if path.is_symlink(): + raise OSError(f"refusing to follow a reparse point: {path}") + handle = _acquire_lock_windows(path.with_name(f".{path.name}.lock")) + try: + long_paths.append_line(path, line) + finally: + _release_lock_windows(handle) + except OSError: + # Approval persistence is diagnostic/control state. A read-only or + # unavailable log must not crash the manager's execution loop. + pass + + def _open_nofollow(path: Path, *, directory: bool = False, strict_parent: bool = False) -> int: """Open a path for reading without following any path component symlink. @@ -1166,6 +1250,8 @@ def _open_nofollow(path: Path, *, directory: bool = False, strict_parent: bool = usable on platforms without ``dir_fd`` support. """ + if _IS_WINDOWS: + return _open_nofollow_windows(path, directory=directory) absolute = Path(os.path.abspath(os.fspath(path))) # macOS exposes /var (and a few other system roots) as symlinks. Resolve # the parent once so the component walk starts from a canonical directory; diff --git a/src/lh_harness/environment/local.py b/src/lh_harness/environment/local.py index 639dc3d6..1d982339 100644 --- a/src/lh_harness/environment/local.py +++ b/src/lh_harness/environment/local.py @@ -47,6 +47,35 @@ def _append_bounded_tail(buffer: bytearray, chunk: bytes, limit: int) -> None: buffer.extend(chunk) +def _open_trajectory_file_windows(path: Path): + """Best-effort no-follow trajectory open for Windows. + + Symlinks and junctions both surface as reparse points, so rejecting those + keeps the redirect attack the POSIX path guards against out of reach. What + cannot be reproduced is the *anchored* guarantee: without directory + descriptors there is an unavoidable check-then-open window. + """ + + long_paths.makedirs(path.parent) + for candidate in (path.parent, path): + if candidate.is_symlink(): + raise OSError(f"refusing to follow a reparse point: {candidate}") + target = long_paths.os_path(path) + handle = open(target, "ab", buffering=0) + try: + metadata = os.fstat(handle.fileno()) + # Windows supports hard links too, so an alias would otherwise be + # truncated along with the trajectory. Check before truncating, exactly + # as the POSIX branch does. + if not stat.S_ISREG(metadata.st_mode) or getattr(metadata, "st_nlink", 1) > 1: + raise OSError("trajectory file is not a private regular file") + handle.truncate(0) + except BaseException: + handle.close() + raise + return handle + + def _open_trajectory_file(path: Path): """Open a live trajectory below an anchored, no-follow parent directory. @@ -55,8 +84,15 @@ def _open_trajectory_file(path: Path): swapped ``round_*`` or ``logs`` symlink and redirect screenshots/tool traces outside the run. Keep the parent descriptor anchored through the open, then retain only the file descriptor used by the tee. + + Windows has neither ``O_NOFOLLOW`` nor directory descriptors, so the + anchored variant is impossible there. It gets the closest equivalent the + platform allows: refuse any reparse point on the way in, then open the + resolved path directly (with long-path support, which run trees need). """ + if IS_WINDOWS: + return _open_trajectory_file_windows(path) parent_fd = _ensure_dir_fd_nofollow(path.parent) fd: int | None = None try: diff --git a/src/lh_harness/manager.py b/src/lh_harness/manager.py index ada49e3e..76ffbb9b 100644 --- a/src/lh_harness/manager.py +++ b/src/lh_harness/manager.py @@ -8,6 +8,7 @@ import logging import os import stat as stat_module +import sys import time from dataclasses import asdict, dataclass, field import traceback @@ -24,6 +25,7 @@ from .runtime_signals import hard_signal_labels from .provider_errors import classify_agent_runtime_failure from .trajectory_artifacts import persist_trajectory_artifacts +from .utils import paths as long_paths from .role_prompts import ( MANAGER_NEXT_BLOCKED, MANAGER_NEXT_CLI, @@ -68,6 +70,7 @@ audit_report_from_episode_result, ) +IS_WINDOWS = sys.platform == "win32" ROLE_VARIANT = "lh_harness_role_managed" logger = logging.getLogger(__name__) _MAX_SAVED_TRAJECTORY_BYTES = 16 * 1024 * 1024 @@ -1391,7 +1394,7 @@ async def _write_final_response( # Stored per round, like the other roles, so a discarded reply keeps its own # artifacts and the dashboard's round trajectory viewer can reach them. round_dir = ctx.role_dir / "rounds" / f"round_{round_index:03d}" - round_dir.mkdir(parents=True, exist_ok=True) + long_paths.makedirs(round_dir) _write_local(round_dir / "final_response_input.txt", prompt) _append_event( ctx.events_path, @@ -2397,8 +2400,47 @@ def _budget_to_dict(budget: EpisodeBudget) -> dict[str, int]: } +def _event_record(path: Path, event: str, payload: dict[str, Any], sequence: int) -> dict[str, Any]: + run_id = path.parents[2].name if len(path.parents) > 2 else "local" + return { + "schema_version": 1, + "event_id": f"{run_id}:{sequence:06d}", + "ts": time.time(), + "event": event, + **_json_safe(payload), + } + + +def _append_event_windows(path: Path, event: str, payload: dict[str, Any]) -> None: + """Append one event on Windows, which has no O_NOFOLLOW/dir_fd/flock. + + Same record shape and same sequence-derived event id as the POSIX path. The + difference is the locking: a single process owns this log for the run, so + the sequence is read and written under one handle rather than an advisory + lock. + """ + + if path.is_symlink(): + raise OSError(f"refusing to follow a reparse point: {path}") + target = long_paths.os_path(path) + with open(target, "a+", encoding="utf-8", newline="") as fh: + fh.seek(0) + sequence = sum(1 for line in fh if line.strip()) + 1 + fh.seek(0, 2) + fh.write( + json.dumps(_event_record(path, event, payload, sequence), ensure_ascii=False, sort_keys=True) + + "\n" + ) + fh.flush() + with contextlib.suppress(OSError): + os.fsync(fh.fileno()) + + def _append_event(path: Path, event: str, payload: dict[str, Any]) -> None: _ensure_dir_nofollow(path.parent) + if IS_WINDOWS: + _append_event_windows(path, event, payload) + return # Event ids are assigned while holding the file lock, so the same absolute # id survives snapshot truncation, REST replay, and a reconnect after an # API restart. The legacy ``event`` field remains for old readers. diff --git a/src/lh_harness/supervisor/control_bus.py b/src/lh_harness/supervisor/control_bus.py index d32cbf76..9de3f79f 100644 --- a/src/lh_harness/supervisor/control_bus.py +++ b/src/lh_harness/supervisor/control_bus.py @@ -11,6 +11,7 @@ import json import os import stat +import sys import threading import time import uuid @@ -18,6 +19,8 @@ from pathlib import Path from typing import Any, Callable, Iterator +_IS_WINDOWS = sys.platform == "win32" + _MAX_CONTROL_RECORD_BYTES = 512 * 1024 _MAX_CONTROL_LOG_BYTES = 16 * 1024 * 1024 @@ -51,6 +54,41 @@ def _absolute_anchored_path(path: str | Path) -> Path: return target.joinpath(*parts[2:]) +def _reject_reparse_chain_windows(path: str | Path) -> Path: + """Return an absolute path after refusing a reparse point in any component. + + Windows has no ``O_NOFOLLOW`` and no directory descriptors, so the anchored + walk below cannot run there. Checking each component for a reparse point + (symlinks and junctions both surface that way) is the closest the platform + gets; the difference is that the check is not anchored, so the same + check-then-use window documented on ``_ensure_dir_nofollow`` applies. + """ + + absolute = _absolute_anchored_path(path) + current = Path(absolute.anchor) + for component in absolute.parts[1:]: + if component in {"", ".", ".."}: + raise OSError("unsafe control-bus path component") + current = current / component + if current.is_symlink(): + raise OSError(f"refusing to traverse a reparse point: {current}") + return absolute + + +def _open_nofollow_windows(path: str | Path, *, directory: bool = False) -> int: + from ..utils import paths as long_paths + + absolute = _reject_reparse_chain_windows(path) + if directory: + # Windows cannot hand back a directory descriptor at all. The only + # caller that wants one, ``iter_run_control_dirs``, has its own branch. + raise OSError("control-bus directory descriptors are unavailable") + return os.open( + long_paths.os_path(absolute), + os.O_RDONLY | os.O_BINARY | getattr(os, "O_NOINHERIT", 0), + ) + + def _open_nofollow(path: str | Path, *, directory: bool = False) -> int: """Open an absolute path without following any component symlink. @@ -60,6 +98,8 @@ def _open_nofollow(path: str | Path, *, directory: bool = False) -> int: descriptors and open every component with ``O_NOFOLLOW``. """ + if _IS_WINDOWS: + return _open_nofollow_windows(path, directory=directory) nofollow = getattr(os, "O_NOFOLLOW", 0) directory_flag = getattr(os, "O_DIRECTORY", 0) cloexec = getattr(os, "O_CLOEXEC", 0) @@ -107,6 +147,27 @@ def _open_nofollow(path: str | Path, *, directory: bool = False) -> int: raise +def _ensure_dir_windows(path: str | Path) -> None: + """Build a directory chain on Windows, refusing reparse points.""" + + from ..utils import paths as long_paths + + absolute = _absolute_anchored_path(path) + current = Path(absolute.anchor) + for component in absolute.parts[1:]: + if component in {"", ".", ".."}: + raise OSError("unsafe control-bus directory component") + current = current / component + if current.is_symlink(): + raise OSError(f"refusing to traverse a reparse point: {current}") + try: + long_paths.makedirs(current, exist_ok=True) + except FileExistsError: + pass + if not current.is_dir(): + raise OSError(f"control-bus path component is not a directory: {current}") + + def _ensure_dir_fd_nofollow(path: str | Path, *, mode: int = 0o700) -> int: """Create/validate a directory chain and return its anchored descriptor. @@ -174,8 +235,20 @@ def _ensure_dir_fd_nofollow(path: str | Path, *, mode: int = 0o700) -> int: def _ensure_dir_nofollow(path: str | Path, *, mode: int = 0o700) -> None: - """Create/validate a directory chain without traversing symlinks.""" + """Create/validate a directory chain without traversing symlinks. + + Windows has no ``O_NOFOLLOW`` and no directory descriptors, so the anchored + POSIX walk cannot run there at all. Callers still need the directory, so it + gets the closest available guarantee: build the chain component by + component, refusing any existing component that is a reparse point + (symlinks and junctions both surface that way). The unavoidable difference + is that the check is not anchored, so a sufficiently privileged local actor + could still win a check-then-use race. + """ + if _IS_WINDOWS: + _ensure_dir_windows(path) + return None fd = _ensure_dir_fd_nofollow(path, mode=mode) try: return None @@ -267,10 +340,43 @@ def _open_unique_temp(parent_fd: int, *, prefix: str, suffix: str) -> tuple[int, raise FileExistsError("could not allocate a unique control-bus temporary file") +def _atomic_bytes_write_windows(path: Path, payload: bytes) -> None: + from ..utils import paths as long_paths + + _ensure_dir_windows(path.parent) + if path.is_symlink(): + raise OSError(f"refusing to follow a reparse point: {path}") + temporary = path.with_name(f".{path.name}.{uuid.uuid4().hex[:12]}.tmp") + target_tmp = long_paths.os_path(temporary) + try: + with open(target_tmp, "wb") as handle: + handle.write(payload) + handle.flush() + try: + os.fsync(handle.fileno()) + except OSError: + pass + os.replace(target_tmp, long_paths.os_path(path)) + except BaseException: + try: + os.unlink(target_tmp) + except OSError: + pass + raise + + def _atomic_bytes_write(path: Path, payload: bytes, *, mode: int = 0o600) -> None: - """Atomically write bytes below an anchored, no-follow parent directory.""" + """Atomically write bytes below an anchored, no-follow parent directory. + + The Windows branch keeps the atomic write-then-replace guarantee (which + ``os.replace`` provides there too) and the reparse-point refusal; only the + anchored-descriptor part is unavailable on that platform. + """ path = Path(path) + if _IS_WINDOWS: + _atomic_bytes_write_windows(path, payload) + return parent_fd = _ensure_dir_fd_nofollow(path.parent) fd: int | None = None temporary_name: str | None = None @@ -332,6 +438,19 @@ def _append_jsonl(path: Path, record: dict[str, Any]) -> None: # ``Path.open('a')`` follows a final symlink. Commands and receipts are # control-plane data, so fail closed unless the whole parent chain and # final file can be opened with anchored no-follow semantics. + if _IS_WINDOWS: + # No anchored descriptors on Windows; refuse reparse points and + # hard-link aliases instead, then append through one checked handle. + _ensure_dir_windows(path.parent) + handle = _open_private_regular_windows(path, os.O_WRONLY | os.O_APPEND) + with handle: + handle.write(line.encode("utf-8")) + handle.flush() + try: + os.fsync(handle.fileno()) + except OSError: + pass + return fd: int | None = None parent_fd: int | None = None try: @@ -382,6 +501,82 @@ def _append_locked_handle(handle: Any, line: str) -> None: flock.flock(handle.fileno(), flock.LOCK_UN) +_WINDOWS_LOCK_TIMEOUT_SECONDS = 120.0 + + +def _open_private_regular_windows(path: Path, flags: int): + """Open one control file on Windows, refusing links and aliases. + + Mirrors what ``_open_private_regular_at`` guarantees on POSIX minus the + anchoring: no reparse point in the chain, a regular file, and exactly one + directory entry -- a hard link is another way to alias control-plane data, + and NTFS creates them without any special privilege. + """ + + from ..utils import paths as long_paths + + absolute = _reject_reparse_chain_windows(path) + fd = os.open(long_paths.os_path(absolute), flags | os.O_CREAT | os.O_BINARY, 0o600) + try: + metadata = os.fstat(fd) + if not stat.S_ISREG(metadata.st_mode): + raise OSError("control file is not a regular file") + if metadata.st_nlink != 1: + raise OSError("control file has multiple hard links") + handle = os.fdopen(fd, "r+b" if flags & os.O_RDWR else "ab", buffering=0) + fd = -1 + return handle + finally: + if fd >= 0: + os.close(fd) + + +def _acquire_lock_windows(path: Path) -> Any: + """Take the run's cross-process control lock through ``msvcrt``. + + ``fcntl.flock`` does not exist on Windows. Locking one byte of the lock + file gives the property the control bus actually depends on: a single + exclusive holder per run, released by the kernel if that holder dies. + """ + + import msvcrt + + # Directory-boundary failures stay OSError so callers can tell an invalid + # run layout from a platform that cannot provide the lock; everything from + # the lock file down is normalized to RuntimeError, as on POSIX. + _ensure_dir_windows(path.parent) + try: + handle = _open_private_regular_windows(path, os.O_RDWR) + except OSError as exc: + raise RuntimeError("secure control-bus locking is unavailable") from exc + try: + deadline = time.monotonic() + _WINDOWS_LOCK_TIMEOUT_SECONDS + while True: + handle.seek(0) + try: + msvcrt.locking(handle.fileno(), msvcrt.LK_LOCK, 1) + return handle + except OSError as exc: + # LK_LOCK gives up after roughly ten seconds. A run legitimately + # holds the lock longer than that while a control append fsyncs, + # so retry to the deadline rather than failing a valid caller. + if time.monotonic() >= deadline: + raise RuntimeError("secure control-bus locking is unavailable") from exc + except BaseException: + handle.close() + raise + + +def _release_lock_windows(handle: Any) -> None: + import msvcrt + + try: + handle.seek(0) + msvcrt.locking(handle.fileno(), msvcrt.LK_UNLCK, 1) + finally: + handle.close() + + def _read_jsonl(path: Path) -> list[dict[str, Any]]: try: fd = _open_nofollow(path) @@ -496,6 +691,13 @@ def _locked(self): """ with self._lock: + if _IS_WINDOWS: + handle = _acquire_lock_windows(self.lock_path) + try: + yield + finally: + _release_lock_windows(handle) + return lock_handle = None # Directory-boundary failures are surfaced as OSError so callers # can distinguish an invalid/symlinked run layout from a platform @@ -727,8 +929,28 @@ def read_owner(self) -> dict[str, Any]: return _read_json_file(self.owner_path) +def _iter_run_control_dirs_windows(root: Path) -> Iterator[Path]: + from ..utils import paths as long_paths + + try: + entries = list(os.scandir(long_paths.os_path(root))) + except OSError: + return + for entry in entries: + if entry.is_symlink(): + continue + control = root / entry.name / "control" + # ``is_dir`` follows links, so the reparse check has to come first. + if control.is_symlink() or not control.is_dir(): + continue + yield root / entry.name + + def iter_run_control_dirs(runs_root: str | Path) -> Iterator[Path]: root = Path(runs_root).expanduser().resolve() + if _IS_WINDOWS: + yield from _iter_run_control_dirs_windows(root) + return try: root_fd = _open_nofollow(root, directory=True) except OSError: diff --git a/src/lh_harness/supervisor/service.py b/src/lh_harness/supervisor/service.py index 308fc65a..179381e1 100644 --- a/src/lh_harness/supervisor/service.py +++ b/src/lh_harness/supervisor/service.py @@ -27,6 +27,7 @@ from .control_bus import ( ControlBus, RevisionConflict, + _acquire_lock_windows, _atomic_bytes_write, _ensure_dir_fd_nofollow, _ensure_dir_nofollow, @@ -34,6 +35,8 @@ _open_private_regular_at, _read_json_file, _read_jsonl, + _reject_reparse_chain_windows, + _release_lock_windows, ) from .lifecycle import ( ACTIVE_STATUSES, @@ -53,8 +56,19 @@ DEFAULT_MAX_ROUNDS, MAX_ROUNDS, ) +from ..utils.process_group import deliver_signal, new_process_group_kwargs from ..utils.run_boundary import safe_run_control, safe_run_dir, safe_run_logs, safe_run_role, safe_run_rounds +_IS_WINDOWS = sys.platform == "win32" + +# Windows has no SIGKILL. The lifecycle distinction that matters is carried by +# ``kind`` ("stop" / "abort") throughout; these constants only name the signal +# in durable receipts and pick graceful-vs-forced delivery. SIGBREAK stands in +# for the forced one there so an operator can still tell a stop receipt from an +# abort receipt after the fact. +STOP_SIGNAL = signal.SIGTERM +ABORT_SIGNAL = getattr(signal, "SIGKILL", None) or signal.SIGBREAK + # Keep a private handle for read-only ``ps`` probes. Tests and embedding code # often replace ``subprocess.Popen`` to model a worker; process identity @@ -159,6 +173,121 @@ def _write_all(fd: int, data: bytes) -> None: view = view[written:] +def _open_worker_log_windows(path: Path): + """Windows stand-in for the anchored no-follow worker-log open. + + Windows offers neither ``O_NOFOLLOW`` nor directory descriptors, so the + walk refuses a reparse point in any component instead of anchoring the + open. Everything the log itself depends on is preserved: regular-file and + hard-link checks, tail compaction, and append positioning. The one lost + guarantee is that the check is not anchored -- a sufficiently privileged + local actor could still win a check-then-use race. + """ + + from ..utils import paths as long_paths + + absolute = _reject_reparse_chain_windows(path) + long_paths.makedirs(absolute.parent) + fd = os.open( + long_paths.os_path(absolute), + os.O_RDWR | os.O_CREAT | os.O_BINARY, + 0o600, + ) + try: + metadata = os.fstat(fd) + if not stat.S_ISREG(metadata.st_mode): + raise OSError(errno.EINVAL, "worker log is not a regular file") + if metadata.st_nlink != 1: + raise OSError(errno.ELOOP, "worker log has multiple hard links") + if metadata.st_size > _MAX_WORKER_LOG_BYTES: + keep = max(1, min(_WORKER_LOG_KEEP_BYTES, _MAX_WORKER_LOG_BYTES)) + os.lseek(fd, max(0, metadata.st_size - keep), os.SEEK_SET) + tail = bytearray() + remaining = keep + while remaining: + chunk = os.read(fd, remaining) + if not chunk: + break + tail.extend(chunk) + remaining -= len(chunk) + os.ftruncate(fd, 0) + os.lseek(fd, 0, os.SEEK_SET) + _write_all(fd, bytes(tail)) + os.lseek(fd, 0, os.SEEK_END) + output = os.fdopen(fd, "a+b", buffering=0) + fd = None + return output + finally: + if fd is not None: + try: + os.close(fd) + except OSError: + pass + + +def _saved_task_from_rounds_windows( + runs_root: Path, + run_id: str, + *, + first_line: bool, +) -> str: + """Windows stand-in for the anchored walk over saved task contracts.""" + + from ..utils import paths as long_paths + + run_path = safe_run_dir(runs_root, run_id) + if run_path is None: + return "" + logs_path = safe_run_logs(runs_root, run_path, allow_missing=False) + role_path = safe_run_role(runs_root, run_path, allow_missing=False) + if logs_path is None or role_path is None: + return "" + rounds_path = role_path / "rounds" + try: + _reject_reparse_chain_windows(rounds_path) + entries = list(os.scandir(long_paths.os_path(rounds_path))) + except OSError: + return "" + candidates: list[tuple[int, str]] = [] + for entry_number, entry in enumerate(entries): + if entry_number >= _MAX_ROUND_DIR_SCAN: + break + name = str(entry.name) + suffix = name.removeprefix("round_") + if suffix == name or not suffix.isdecimal(): + continue + candidates.append((int(suffix), name)) + for _, name in sorted(candidates): + contract = rounds_path / name / "task_contract.txt" + try: + _reject_reparse_chain_windows(contract) + fd = os.open(long_paths.os_path(contract), os.O_RDONLY | os.O_BINARY) + except OSError: + continue + try: + metadata = os.fstat(fd) + if ( + not stat.S_ISREG(metadata.st_mode) + or metadata.st_nlink != 1 + or metadata.st_size > _MAX_SAVED_TASK_BYTES + ): + continue + raw = os.read(fd, _MAX_SAVED_TASK_BYTES + 1) + except OSError: + continue + finally: + os.close(fd) + if len(raw) > _MAX_SAVED_TASK_BYTES: + continue + # A contract may have been written by any Windows tool, so normalize + # line endings before this becomes a run title in the dashboard. + text = raw.decode("utf-8", errors="replace").replace("\r\n", "\n").strip() + if not text: + continue + return text.splitlines()[0].strip() if first_line else text + return "" + + def _open_worker_log(path: Path): """Open a run-local worker log without following the final symlink. @@ -175,6 +304,8 @@ def _open_worker_log(path: Path): """ path = Path(path) + if _IS_WINDOWS: + return _open_worker_log_windows(path) nofollow = getattr(os, "O_NOFOLLOW", 0) if not nofollow: raise OSError(errno.ENOTSUP, "worker log requires O_NOFOLLOW") @@ -269,6 +400,8 @@ def _saved_task_from_rounds( and ``O_NOFOLLOW`` for every component, then read a bounded regular file. """ + if _IS_WINDOWS: + return _saved_task_from_rounds_windows(runs_root, run_id, first_line=first_line) nofollow = getattr(os, "O_NOFOLLOW", 0) directory = getattr(os, "O_DIRECTORY", 0) cloexec = getattr(os, "O_CLOEXEC", 0) @@ -641,6 +774,13 @@ def _supervisor_locked(self): """Serialize launch/resume idempotency transactions across workers.""" lock_path = self.runs_root / ".supervisor.lock" + if _IS_WINDOWS: + windows_handle = _acquire_lock_windows(lock_path) + try: + yield + finally: + _release_lock_windows(windows_handle) + return handle = None flock = None raw_fd: int | None = None @@ -1008,7 +1148,7 @@ def close(value: dict[str, Any]) -> dict[str, Any]: # Identity mismatch is intentionally fail-closed: never signal a # reused PID merely because the old owner record said it was alive. return reconcile_unavailable("worker is no longer running or its identity changed") - sig = signal.SIGKILL if action == "abort" else signal.SIGTERM + sig = ABORT_SIGNAL if action == "abort" else STOP_SIGNAL # Record an attempt for diagnostics under the same lock used by status # merges. Do not use it as a once-only gate: a crash after the signal # and before the receipt needs one safe retry. @@ -1020,9 +1160,9 @@ def close(value: dict[str, Any]) -> dict[str, Any]: } ) if str(owner.get("signal_mode") or "pgid") == "pid": - os.kill(pid, sig) + deliver_signal(pid, sig, group=False) else: - os.killpg(pgid, sig) + deliver_signal(pgid, sig) except ProcessLookupError: return reconcile_unavailable("worker is no longer running") except PermissionError: @@ -1682,8 +1822,8 @@ def _launch_worker( stdin=subprocess.DEVNULL, stdout=output, stderr=subprocess.STDOUT, - start_new_session=True, env=worker_env, + **new_process_group_kwargs(), ) except Exception: output.close() @@ -1725,7 +1865,7 @@ def _launch_worker( # unowned worker running if the durable reservation cannot be # promoted to a live owner. try: - os.killpg(process.pid, signal.SIGKILL) + deliver_signal(process.pid, ABORT_SIGNAL) except OSError: pass raise @@ -1957,7 +2097,7 @@ def _signal(self, run_id: str, sig: signal.Signals, *, kind: str) -> dict[str, A None, ) receipt = bus.receipt_for(active_command_id) - active_signal = signal.SIGKILL.name if active_kind == "abort" else signal.SIGTERM.name + active_signal = ABORT_SIGNAL.name if active_kind == "abort" else STOP_SIGNAL.name return { "command_id": active_command_id, "status": str((receipt or {}).get("status") or "accepted"), @@ -2042,9 +2182,9 @@ def mark_stopping(previous_status: dict[str, Any]) -> dict[str, Any]: return {"command_id": command["command_id"], "status": "accepted", "signal": sig.name} try: if str(owner.get("signal_mode") or "pgid") == "pid": - os.kill(owner_pid, sig) + deliver_signal(owner_pid, sig, group=False) else: - os.killpg(pgid, sig) + deliver_signal(pgid, sig) except ProcessLookupError: # The signal raced with process exit. Do not leave a durable # ``stopping`` state behind: reconcile from the final report, @@ -2099,10 +2239,10 @@ def restore_active(value: dict[str, Any]) -> dict[str, Any]: return {"command_id": command["command_id"], "status": "accepted", "signal": sig.name} def stop(self, run_id: str) -> dict[str, Any]: - return self._signal(run_id, signal.SIGTERM, kind="stop") + return self._signal(run_id, STOP_SIGNAL, kind="stop") def abort(self, run_id: str) -> dict[str, Any]: - return self._signal(run_id, signal.SIGKILL, kind="abort") + return self._signal(run_id, ABORT_SIGNAL, kind="abort") def shutdown(self, *, grace_seconds: float = 5.0) -> None: """Stop workers launched by this supervisor before its API exits. @@ -2135,12 +2275,12 @@ def shutdown(self, *, grace_seconds: float = 5.0) -> None: if process.poll() is not None: continue try: - os.killpg(process.pid, signal.SIGKILL) + deliver_signal(process.pid, ABORT_SIGNAL) except ProcessLookupError: pass - except PermissionError: + except (PermissionError, OSError): # These are still verified in-memory children; fall back to - # Popen's direct signal when a platform denies killpg. + # Popen's direct signal when a platform denies group delivery. try: process.kill() except OSError: diff --git a/tests/supervisor/test_hardening.py b/tests/supervisor/test_hardening.py index 371dddc5..6672b40c 100644 --- a/tests/supervisor/test_hardening.py +++ b/tests/supervisor/test_hardening.py @@ -216,7 +216,7 @@ def test_control_bus_accepts_trusted_macos_var_alias(tmp_path: Path) -> None: def test_control_bus_fails_closed_when_process_lock_breaks(monkeypatch, tmp_path: Path) -> None: - import fcntl + fcntl = pytest.importorskip("fcntl", reason="POSIX file locking; Windows uses msvcrt") def broken_flock(*_args, **_kwargs): raise OSError("lock unavailable") @@ -372,6 +372,8 @@ def test_iter_run_control_dirs_skips_symlinked_run_boundaries(tmp_path: Path) -> def test_atomic_json_write_closes_mkstemp_fd_when_fdopen_fails(monkeypatch, tmp_path: Path) -> None: """The temporary descriptor must not leak if wrapping it raises.""" + if sys.platform == "win32": + pytest.skip("the mkstemp/fdopen path is POSIX-only; Windows replaces atomically") captured: dict[str, int] = {} real_fdopen = control_bus.os.fdopen @@ -399,7 +401,7 @@ def test_stop_keeps_stopping_state_while_stale_approval_is_present( process = _Process() monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda *args, **kwargs: None) + monkeypatch.setattr("lh_harness.supervisor.service.deliver_signal", lambda *args, **kwargs: None) supervisor = RunSupervisor(tmp_path / "runs", workspace_root=tmp_path / "workspace") created = supervisor.create_run(task="stop me") run_dir = tmp_path / "runs" / created["id"] @@ -425,7 +427,7 @@ def test_abort_escalates_stop_and_cross_action_retries_are_idempotent( process = _Process() signals: list[object] = [] monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda _pid, sig: signals.append(sig)) + monkeypatch.setattr("lh_harness.supervisor.service.deliver_signal", lambda _pid, sig, **_kw: signals.append(sig)) supervisor = RunSupervisor(tmp_path / "runs", workspace_root=tmp_path / "workspace") created = supervisor.create_run(task="stop then abort") @@ -433,10 +435,10 @@ def test_abort_escalates_stop_and_cross_action_retries_are_idempotent( aborted = supervisor.abort(created["id"]) repeated_stop = supervisor.stop(created["id"]) - assert stopped["signal"] == "SIGTERM" - assert aborted["signal"] == "SIGKILL" + assert stopped["signal"] == supervisor_service.STOP_SIGNAL.name + assert aborted["signal"] == supervisor_service.ABORT_SIGNAL.name assert repeated_stop["idempotent"] is True - assert signals == [supervisor_service.signal.SIGTERM, supervisor_service.signal.SIGKILL] + assert signals == [supervisor_service.STOP_SIGNAL, supervisor_service.ABORT_SIGNAL] status = supervisor.status(created["id"]) assert status["status"] == "stopping" assert status["requested_action"] == "abort" @@ -446,7 +448,7 @@ def test_stop_after_abort_returns_abort_receipt_without_conflict(monkeypatch, tm process = _Process() signals: list[object] = [] monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda _pid, sig: signals.append(sig)) + monkeypatch.setattr("lh_harness.supervisor.service.deliver_signal", lambda _pid, sig, **_kw: signals.append(sig)) supervisor = RunSupervisor(tmp_path / "runs", workspace_root=tmp_path / "workspace") created = supervisor.create_run(task="abort then stop") @@ -454,9 +456,9 @@ def test_stop_after_abort_returns_abort_receipt_without_conflict(monkeypatch, tm repeated = supervisor.stop(created["id"]) assert repeated["command_id"] == "lifecycle-abort" - assert repeated["signal"] == "SIGKILL" + assert repeated["signal"] == supervisor_service.ABORT_SIGNAL.name assert repeated["idempotent"] is True - assert signals == [supervisor_service.signal.SIGKILL] + assert signals == [supervisor_service.ABORT_SIGNAL] def test_first_role_event_promotes_starting_worker_to_running(monkeypatch, tmp_path: Path) -> None: @@ -600,10 +602,10 @@ def test_stop_persists_intent_before_signal(monkeypatch, tmp_path: Path) -> None monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) observed: list[str] = [] - def killpg(_pid: int, _sig: object) -> None: + def deliver(_pid: int, _sig: object, **_kwargs: object) -> None: observed.append(ControlBus(tmp_path / "runs" / created["id"]).read_status().get("status", "")) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", killpg) + monkeypatch.setattr("lh_harness.supervisor.service.deliver_signal", deliver) supervisor = RunSupervisor(tmp_path / "runs", workspace_root=tmp_path / "workspace") created = supervisor.create_run(task="ordering") supervisor.stop(created["id"]) @@ -613,7 +615,7 @@ def killpg(_pid: int, _sig: object) -> None: def test_signal_process_lookup_reconciles_stopping_to_failed(monkeypatch, tmp_path: Path) -> None: process = _Process() monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda *args, **kwargs: (_ for _ in ()).throw(ProcessLookupError())) + monkeypatch.setattr("lh_harness.supervisor.service.deliver_signal", lambda *args, **kwargs: (_ for _ in ()).throw(ProcessLookupError())) supervisor = RunSupervisor(tmp_path / "runs", workspace_root=tmp_path / "workspace") created = supervisor.create_run(task="vanish") @@ -629,7 +631,7 @@ def test_signal_process_lookup_reconciles_stopping_to_failed(monkeypatch, tmp_pa def test_signal_permission_failure_restores_active_state(monkeypatch, tmp_path: Path) -> None: process = _Process() monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda *args, **kwargs: (_ for _ in ()).throw(PermissionError())) + monkeypatch.setattr("lh_harness.supervisor.service.deliver_signal", lambda *args, **kwargs: (_ for _ in ()).throw(PermissionError())) supervisor = RunSupervisor(tmp_path / "runs", workspace_root=tmp_path / "workspace") created = supervisor.create_run(task="permission") @@ -646,7 +648,7 @@ def test_pending_lifecycle_command_is_replayed_after_supervisor_restart(monkeypa process = _Process() monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) sent: list[object] = [] - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda pid, sig: sent.append((pid, sig))) + monkeypatch.setattr("lh_harness.supervisor.service.deliver_signal", lambda pid, sig, **_kw: sent.append((pid, sig))) supervisor = RunSupervisor(tmp_path / "runs", workspace_root=tmp_path / "workspace") created = supervisor.create_run(task="replay stop") bus = ControlBus(tmp_path / "runs" / created["id"]) @@ -829,7 +831,10 @@ def test_worker_log_open_compacts_old_tail_and_tightens_permissions( output.seek(0) assert output.read() == bytes(range(68, 100)) assert path.stat().st_size == 32 - assert path.stat().st_mode & 0o777 == 0o600 + if sys.platform != "win32": + # Windows reports 0o666/0o444 from the read-only attribute; there + # is no owner-only mode bit to assert. + assert path.stat().st_mode & 0o777 == 0o600 finally: output.close() diff --git a/tests/supervisor/test_supervisor.py b/tests/supervisor/test_supervisor.py index 2690ab33..63c94a26 100644 --- a/tests/supervisor/test_supervisor.py +++ b/tests/supervisor/test_supervisor.py @@ -36,7 +36,7 @@ def test_supervisor_creates_owned_run_without_touching_manager(monkeypatch, tmp_ def test_web_supervisor_exposes_create_and_lifecycle_routes(monkeypatch, tmp_path: Path) -> None: process = FakeProcess() monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda *args, **kwargs: None) + monkeypatch.setattr("lh_harness.supervisor.service.deliver_signal", lambda *args, **kwargs: None) root = tmp_path / "runs" supervisor = RunSupervisor(root, workspace_root=tmp_path / "workspace") client = TestClient(create_app(runs_root=root, supervisor=supervisor)) @@ -104,7 +104,7 @@ def test_web_supervisor_creates_deepseek_run_with_role_models( def test_websocket_pushes_supervisor_lifecycle_without_role_events(monkeypatch, tmp_path: Path) -> None: process = FakeProcess() monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda *args, **kwargs: None) + monkeypatch.setattr("lh_harness.supervisor.service.deliver_signal", lambda *args, **kwargs: None) root = tmp_path / "runs" supervisor = RunSupervisor(root, workspace_root=tmp_path / "workspace") client = TestClient(create_app(runs_root=root, supervisor=supervisor)) @@ -213,12 +213,12 @@ def test_supervisor_shutdown_stops_owned_live_workers(monkeypatch, tmp_path: Pat process = FakeProcess() signals: list[object] = [] - def killpg(_pid: int, sig: object) -> None: + def deliver(_pid: int, sig: object, **_kwargs: object) -> None: signals.append(sig) process.returncode = -15 monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", killpg) + monkeypatch.setattr("lh_harness.supervisor.service.deliver_signal", deliver) supervisor = RunSupervisor(tmp_path / "runs", workspace_root=tmp_path / "workspace") created = supervisor.create_run(task="stop with the Web API") From f01e94be445e6dc16899c3bd6950598aeff377d6 Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 13:13:39 +0300 Subject: [PATCH 04/19] Let the test suite run on Windows Creating a symlink needs SeCreateSymbolicLinkPrivilege, which an ordinary account only holds under Developer Mode, so the no-follow tests failed on the fixture rather than on the behaviour they cover. They now skip with a reason where the privilege is missing and run unchanged where it exists. Only the test body is wrapped: pytest's own tmp_path bookkeeping also makes symlinks, and swallowing that would skip the entire suite. The Content-Disposition fixture also asked for a filename containing a quote, which Windows reserves outright; that half of the fixture now only runs on POSIX. --- tests/conftest.py | 23 +++++++++++++++++++++++ tests/webapi/test_hardening.py | 17 +++++++++++------ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 209b6d31..0932c3ce 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,11 +2,34 @@ from __future__ import annotations +import sys from urllib.parse import urljoin import fastapi.testclient as fastapi_testclient +import pytest import starlette.testclient as starlette_testclient +if sys.platform == "win32": + + @pytest.hookimpl(wrapper=True) + def pytest_runtest_call(item): + """Skip, rather than fail, when Windows denies symlink creation. + + Creating a symlink needs SeCreateSymbolicLinkPrivilege, which an + ordinary account only holds under Developer Mode. The no-follow tests + are still meaningful on Windows, so they run unchanged where the + privilege exists and report honestly where it does not. Only the test + body is wrapped: pytest's own tmp_path bookkeeping also makes symlinks, + and swallowing that would skip the entire suite. + """ + try: + return (yield) + except OSError as exc: + if getattr(exc, "winerror", None) == 1314: + pytest.skip("symlink creation requires Developer Mode or admin rights") + raise + + _Orig = starlette_testclient.TestClient _Upgrade = starlette_testclient._Upgrade diff --git a/tests/webapi/test_hardening.py b/tests/webapi/test_hardening.py index 113cc752..e2f96e43 100644 --- a/tests/webapi/test_hardening.py +++ b/tests/webapi/test_hardening.py @@ -5,6 +5,7 @@ import base64 import shutil import time +import sys from pathlib import Path import pytest @@ -246,18 +247,22 @@ def test_non_raster_documents_are_attachments_and_filename_is_header_safe(tmp_pa # The filesystem permits both quote-bearing and non-ASCII names. They # must not make Starlette fail to encode the response header or alter its # disposition semantics. - quoted = round_dir / 'x";foo.pdf' - quoted.write_bytes(b"%PDF-fixture") + # Windows reserves the quote character outright, so that half of the + # fixture can only exist on POSIX. + quote_is_a_legal_filename_character = sys.platform != "win32" + if quote_is_a_legal_filename_character: + (round_dir / 'x";foo.pdf').write_bytes(b"%PDF-fixture") unicode_name = round_dir / "你好.svgz" unicode_name.write_bytes(b"not-an-inline-document") client = TestClient(create_app(state=state, runs_root=root, run_id="run-1")) - pdf = client.get('/api/runs/run-1/rounds/1/artifacts/x%22%3Bfoo.pdf/raw') svgz = client.get('/api/runs/run-1/rounds/1/artifacts/%E4%BD%A0%E5%A5%BD.svgz/raw') - assert pdf.status_code == 200 - assert pdf.headers["content-type"].startswith("application/octet-stream") - assert pdf.headers["content-disposition"].startswith('attachment; filename="x_foo.pdf"; filename*=UTF-8\'\'') + if quote_is_a_legal_filename_character: + pdf = client.get('/api/runs/run-1/rounds/1/artifacts/x%22%3Bfoo.pdf/raw') + assert pdf.status_code == 200 + assert pdf.headers["content-type"].startswith("application/octet-stream") + assert pdf.headers["content-disposition"].startswith('attachment; filename="x_foo.pdf"; filename*=UTF-8\'\'') assert svgz.status_code == 200 assert svgz.headers["content-type"].startswith("text/plain") assert svgz.headers["content-disposition"].startswith('attachment; filename="artifact.svgz"; filename*=UTF-8\'\'') From dede451757059dd9e5dc073e11eca29c0189e8aa Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 13:13:39 +0300 Subject: [PATCH 05/19] Document Windows as a tested platform Agent CLIs are launched as plain subprocesses now, so command construction behaves identically on every platform, and run directories escape MAX_PATH automatically. --- README.md | 2 +- README.zh-CN.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index bccecc00..acef329c 100644 --- a/README.md +++ b/README.md @@ -192,7 +192,7 @@ Steps 1–2 are once per machine; step 3 is once per project. Then run tasks fro | One agent runtime on `PATH`: [`codex`](https://github.com/openai/codex#installing-and-running-codex-cli), [`claude`](https://docs.anthropic.com/en/docs/claude-code/getting-started), [`opencode`](https://github.com/anomalyco/opencode), or [`dsh`](https://github.com/deepseek-ai/deepseek-harness) | Actually executing the work. Install more than one if you want to mix backends across roles. | | [Node.js](https://nodejs.org) 20 or later | The npm-distributed computer-use plugins. DeepSeek Harness itself currently requires Node.js `^22.19.0` or `>=24.0.0`. | -> **Platform status:** Currently tested on macOS. Windows support is included but has not yet been thoroughly tested. +> **Platform status:** Tested on macOS and Windows. Agent CLIs are launched as plain subprocesses — no shell is involved — so command construction behaves identically on every platform. On Windows the harness also escapes the 260-character `MAX_PATH` limit automatically, which run directories reach easily on a deep project path. Run `lh-harness doctor` at any point to check all of the above; see [Verify the environment](#verify-the-environment). diff --git a/README.zh-CN.md b/README.zh-CN.md index 549e9f90..4a1ca19e 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -193,7 +193,7 @@ LongHorizon-Harness 不只展示了几个精心挑选的成功案例。 | `PATH` 上有一个 Agent 运行时：[`codex`](https://github.com/openai/codex#installing-and-running-codex-cli)、[`claude`](https://docs.anthropic.com/en/docs/claude-code/getting-started)、[`opencode`](https://github.com/anomalyco/opencode) 或 [`dsh`](https://github.com/deepseek-ai/deepseek-harness) | 真正执行任务。想按角色混用多个后端就安装多个。 | | [Node.js](https://nodejs.org) 20 或更高版本 | npm 分发的 computer-use 插件需要；DeepSeek Harness 本身目前要求 Node.js `^22.19.0` 或 `>=24.0.0`。 | -> **平台状态：** 目前只在 macOS 上完成了测试；Windows 已支持，但尚未经过详细测试。 +> **平台状态：** 已在 macOS 和 Windows 上测试。Agent CLI 以普通子进程方式启动，不经过 shell，因此命令构造在所有平台上行为一致。在 Windows 上还会自动绕开 260 字符的 `MAX_PATH` 限制——项目路径较深时，运行目录很容易超过这个长度。 以上都可以用 `lh-harness doctor` 一次性检查，见[检查运行环境](#检查运行环境)。 From cbf36a88d7d93df4c8a000434a22bbd51a701e88 Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 13:13:57 +0300 Subject: [PATCH 06/19] Wait out transient provider failures instead of aborting the run A run died the moment the provider answered 429, overloaded, or dropped the connection, which on a long-horizon task throws away hours of work for a condition that clears on its own. `_run_role_episode` now backs off exponentially for the two failure kinds that are actually transient -- rate_limit and network -- and retries: 60s doubling to a 900s cap, up to 8 attempts or 2 hours total, tunable through LH_HARNESS_PROVIDER_RETRY_{MAX_ATTEMPTS,BASE_SECONDS,CAP_SECONDS,MAX_TOTAL_SECONDS} with MAX_ATTEMPTS=0 restoring the old fail-fast behaviour. Terminal kinds (authentication, quota, model_unavailable, timeout) return immediately as before; retrying those wastes time or masks a real hang. The wait is visible: an `agent_runtime_retry` event and a `role_retry` progress record carry the attempt, the delay and the provider's own message, and a cancel during backoff still returns a cancelled episode. Classification also learns the subscription-limit shapes that were previously read as success: 529, a session/usage limit result whose `is_error` is false but whose `api_error_status` is 429, and a `rate_limit_event` record whose status is "rejected". --- src/lh_harness/manager.py | 143 ++++++++++++++++++++++++++---- src/lh_harness/provider_errors.py | 34 ++++++- 2 files changed, 157 insertions(+), 20 deletions(-) diff --git a/src/lh_harness/manager.py b/src/lh_harness/manager.py index 76ffbb9b..f68fb10e 100644 --- a/src/lh_harness/manager.py +++ b/src/lh_harness/manager.py @@ -23,7 +23,7 @@ from .environment.base import Environment from .environment.remote_files import ensure_remote_dir, write_remote_text from .runtime_signals import hard_signal_labels -from .provider_errors import classify_agent_runtime_failure +from .provider_errors import classify_agent_runtime_failure, is_retryable_failure from .trajectory_artifacts import persist_trajectory_artifacts from .utils import paths as long_paths from .role_prompts import ( @@ -409,6 +409,9 @@ def emit(event: str, **payload: Any) -> None: env, manager_budget, live_trajectory_path=str(round_dir / "manager_raw_trajectory.jsonl"), + on_retry=lambda a, d, f: _emit_retry_event( + events_path, emit, round_index, "manager", a, d, f + ), ) _save_role_result( round_dir, @@ -667,6 +670,9 @@ def emit(event: str, **payload: Any) -> None: env, executor_budget, live_trajectory_path=str(round_dir / "executor_raw_trajectory.jsonl"), + on_retry=lambda a, d, f: _emit_retry_event( + events_path, emit, round_index, f"{next_step}_executor", a, d, f + ), ) executor_episode_root = log_dir / ( "gui_executor_episodes" if next_step == MANAGER_NEXT_GUI else "cli_executor_episodes" @@ -807,6 +813,9 @@ def emit(event: str, **payload: Any) -> None: env, auditor_budget, live_trajectory_path=str(round_dir / "auditor_raw_trajectory.jsonl"), + on_retry=lambda a, d, f: _emit_retry_event( + events_path, emit, round_index, f"{next_step}_auditor", a, d, f + ), ) auditor_episode_root = log_dir / ( "gui_auditor_episodes" if next_step == MANAGER_NEXT_GUI else "cli_auditor_episodes" @@ -1224,6 +1233,74 @@ def _executor_binding( return cli_executor_agent, cli_executor_budget +@dataclass(frozen=True) +class _RetryPolicy: + max_attempts: int + base_seconds: float + cap_seconds: float + max_total_seconds: float + + +def _env_float(name: str, default: float) -> float: + raw = os.getenv(name) + if raw is None or not raw.strip(): + return default + try: + value = float(raw) + except ValueError: + return default + return value if value >= 0 else default + + +def _provider_retry_policy() -> _RetryPolicy: + """Backoff for transient provider failures (rate limit / network). + + Defaults wait out a rate-limit window (exp backoff 60s -> cap 900s, up to 8 + attempts or 2h total) so a run PAUSES and RESUMES instead of aborting the + moment the provider returns a 429/overloaded/connection error. Override via + LH_HARNESS_PROVIDER_RETRY_{MAX_ATTEMPTS,BASE_SECONDS,CAP_SECONDS,MAX_TOTAL_SECONDS}; + set MAX_ATTEMPTS=0 to restore the old fail-fast behaviour. + """ + return _RetryPolicy( + max_attempts=int(_env_float("LH_HARNESS_PROVIDER_RETRY_MAX_ATTEMPTS", 8)), + base_seconds=_env_float("LH_HARNESS_PROVIDER_RETRY_BASE_SECONDS", 60.0), + cap_seconds=_env_float("LH_HARNESS_PROVIDER_RETRY_CAP_SECONDS", 900.0), + max_total_seconds=_env_float("LH_HARNESS_PROVIDER_RETRY_MAX_TOTAL_SECONDS", 7200.0), + ) + + +def _emit_retry_event( + events_path: Path, + emit: Callable[..., None], + round_index: int, + phase: str, + attempt: int, + delay: float, + failure: Any, +) -> None: + """Surface a provider backoff wait in the event log + progress stream.""" + _append_event( + events_path, + "agent_runtime_retry", + { + "round": round_index, + "phase": phase, + "attempt": attempt, + "delay_seconds": round(delay, 1), + "kind": failure.kind, + "message": failure.message, + }, + ) + emit( + "role_retry", + round=round_index, + role=phase, + attempt=attempt, + delay_ms=int(delay * 1000), + error=failure.user_message, + ) + + async def _run_role_episode( agent: AgentAdapter, prompt: str, @@ -1231,23 +1308,55 @@ async def _run_role_episode( budget: EpisodeBudget, *, live_trajectory_path: str | None = None, + on_retry: Callable[[int, float, Any], None] | None = None, ) -> EpisodeResult: - """Normalize cooperative cancellation for every adapter implementation.""" - started = time.monotonic() - try: - return await agent.run_episode( - prompt, - env, - budget, - live_trajectory_path=live_trajectory_path, - ) - except asyncio.CancelledError: - return EpisodeResult( - status="cancelled", - error="Execution cancelled by operator", - duration_ms=int((time.monotonic() - started) * 1000), - metadata={"cancelled": True}, - ) + """Run one role episode. + + Normalizes cooperative cancellation, and waits out transient provider + failures (rate limit / network) with exponential backoff so the run pauses + and resumes rather than aborting. Terminal failures return immediately for + the caller to classify and abort as before. + """ + policy = _provider_retry_policy() + started_total = time.monotonic() + attempt = 0 + while True: + started = time.monotonic() + try: + result = await agent.run_episode( + prompt, + env, + budget, + live_trajectory_path=live_trajectory_path, + ) + except asyncio.CancelledError: + return EpisodeResult( + status="cancelled", + error="Execution cancelled by operator", + duration_ms=int((time.monotonic() - started) * 1000), + metadata={"cancelled": True}, + ) + failure = classify_agent_runtime_failure(result) + if not is_retryable_failure(failure): + return result + attempt += 1 + elapsed = time.monotonic() - started_total + if policy.max_attempts <= 0 or attempt >= policy.max_attempts or elapsed >= policy.max_total_seconds: + return result + delay = min(policy.cap_seconds, policy.base_seconds * (2 ** (attempt - 1))) + delay = max(0.0, min(delay, policy.max_total_seconds - elapsed)) + if on_retry is not None: + with contextlib.suppress(Exception): + on_retry(attempt, delay, failure) + try: + await asyncio.sleep(delay) + except asyncio.CancelledError: + return EpisodeResult( + status="cancelled", + error="Execution cancelled by operator during provider backoff", + duration_ms=int((time.monotonic() - started) * 1000), + metadata={"cancelled": True}, + ) async def _auditor_report_with_format_repair( diff --git a/src/lh_harness/provider_errors.py b/src/lh_harness/provider_errors.py index 6e1efd4e..40dee9ad 100644 --- a/src/lh_harness/provider_errors.py +++ b/src/lh_harness/provider_errors.py @@ -29,6 +29,18 @@ class AgentRuntimeFailure: user_message: str +# Provider failures that are transient: the provider is momentarily unhappy +# (429/overloaded, dropped connection) but the run can continue once it recovers. +# Terminal kinds (authentication, quota/billing, model_unavailable, timeout) are +# NOT here — retrying those wastes time or masks a real hang. +RETRYABLE_KINDS: frozenset[str] = frozenset({"rate_limit", "network"}) + + +def is_retryable_failure(failure: "AgentRuntimeFailure | None") -> bool: + """True when the failure is a transient provider hiccup worth waiting out.""" + return failure is not None and failure.kind in RETRYABLE_KINDS + + _CLASSIFIERS: tuple[tuple[str, re.Pattern[str], str], ...] = ( ( "model_unavailable", @@ -61,7 +73,11 @@ class AgentRuntimeFailure: ), ( "rate_limit", - re.compile(r"(?:\b429\b|rate[ _-]?limit|too many requests|overloaded|请求过多|限流|过载)", re.I), + re.compile( + r"(?:\b429\b|\b529\b|rate[ _-]?limit|ratelimittype|five[_ -]?hour|session limit|" + r"usage limit|too many requests|overloaded|请求过多|限流|过载|会话限制|用量限制)", + re.I, + ), "Provider 限流或过载", ), ( @@ -188,8 +204,20 @@ def _failure_messages(result: EpisodeResult, metadata: dict[str, Any]) -> list[s _append(values, error.get("message") if isinstance(error, dict) else error) elif record_type == "error": _append(values, record.get("message") or record.get("error")) - elif record_type == "result" and record.get("is_error"): - _append(values, record.get("result") or record.get("error") or record.get("subtype")) + elif record_type == "result": + # A session/usage-limit rejection comes back as subtype "success" with + # is_error false but api_error_status 429 and a "session limit" result + # string, so key off the HTTP status too, not just is_error. + status_code = record.get("api_error_status") or record.get("status_code") + if record.get("is_error") or (isinstance(status_code, int) and status_code >= 400): + _append(values, record.get("result") or record.get("error") or record.get("subtype")) + elif record_type == "rate_limit_event": + info = record.get("rate_limit_info") + if isinstance(info, dict) and "rejected" in { + str(info.get("status", "")).lower(), + str(info.get("overageStatus", "")).lower(), + }: + _append(values, "provider rate limit rejected (session limit reached)") _append(values, result.error) _append(values, metadata.get("stderr_tail")) signals = metadata.get("runtime_signals") From 09dccc69e75d2e96e149fef533cc35b5c6b7a225 Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 15:29:47 +0300 Subject: [PATCH 07/19] Accept the venv launcher pid in supervised owner checks on Windows A Python 3.13+ venv Scripts\python.exe is a launcher that starts the real interpreter as a child process. The supervisor records the launcher pid from Popen, while the worker compares it to os.getpid() of the real interpreter, so every supervised run died with "reservation belongs to another process". The worker identity on Windows now includes os.getppid() (the launcher), and the supervisor_pid lineage check accepts a live recorded parent there, since the grandparent is not portably reachable. POSIX behaviour is unchanged. --- src/lh_harness/cli.py | 54 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 5 deletions(-) diff --git a/src/lh_harness/cli.py b/src/lh_harness/cli.py index a42667c8..6bfddc8d 100644 --- a/src/lh_harness/cli.py +++ b/src/lh_harness/cli.py @@ -229,6 +229,47 @@ def _read_supervised_record( return value +def _worker_identity_pids() -> frozenset[int]: + """PIDs that legitimately identify this worker process. + + On Windows, a Python 3.13+ venv ``Scripts\\python.exe`` is a launcher that + starts the real interpreter as a *child* process. The pid the supervisor + recorded from ``Popen`` is then our parent's pid, not our own, so the + parent is part of this worker's identity there. POSIX keeps the exact + single-pid match. + """ + + pids = {os.getpid()} + if os.name == "nt": + pids.add(os.getppid()) + return frozenset(pids) + + +def _supervisor_pid_matches(recorded_parent: int) -> bool: + """Check a reservation's ``supervisor_pid`` against this worker's lineage. + + POSIX: the supervisor is exactly our parent. Windows: the venv launcher + sits between us and the supervisor, so ``getppid`` is the launcher and the + grandparent is not portably reachable without extra dependencies; accept a + recorded parent that is our direct parent or a process that is still + alive. The window this guards is the milliseconds between ``Popen`` and + the owner-promotion write, so the liveness check keeps the practical + protection (a stale reservation whose supervisor died is still rejected). + """ + + if recorded_parent <= 0: + return False + if recorded_parent == os.getppid(): + return True + if os.name != "nt": + return False + try: + os.kill(recorded_parent, 0) + except OSError: + return False + return True + + def _adopt_supervised_run_dir( runs_root: str | Path, requested_run_id: str | None, @@ -293,7 +334,7 @@ def _adopt_supervised_run_dir( owner_pid = int(owner.get("pid", 0) or 0) except (TypeError, ValueError) as exc: raise ValueError("supervised run reservation has an invalid owner pid") from exc - if owner_pid not in {0, os.getpid()}: + if owner_pid != 0 and owner_pid not in _worker_identity_pids(): raise ValueError("supervised run reservation belongs to another process") if owner_pid == 0: # There is a small, legitimate window between Popen() returning and @@ -304,7 +345,7 @@ def _adopt_supervised_run_dir( reservation_parent = int(owner.get("supervisor_pid", 0) or 0) except (TypeError, ValueError) as exc: raise ValueError("supervised run reservation has an invalid parent pid") from exc - if reservation_parent <= 0 or reservation_parent != os.getppid(): + if not _supervisor_pid_matches(reservation_parent): raise ValueError("supervised run reservation is not owned by this worker") if str(status.get("run_id") or candidate) != candidate: raise ValueError("supervised run status has the wrong run id") @@ -358,15 +399,18 @@ def _claim_supervised_owner(run_id: str, run_dir: Path) -> None: existing_pid = int(value.get("pid", 0) or 0) except (TypeError, ValueError) as exc: raise ValueError("supervised run owner pid is invalid") from exc - if existing_pid not in {0, os.getpid()}: + if existing_pid != 0 and existing_pid not in _worker_identity_pids(): raise ValueError("supervised run owner belongs to another process") - if existing_pid == os.getpid(): + if existing_pid in _worker_identity_pids(): + # The owner already points at this worker. On Windows that may be the + # venv launcher's pid rather than our own; keep it — the supervisor + # signals the whole tree, so the launcher is the better kill handle. return try: parent_pid = int(value.get("supervisor_pid", 0) or 0) except (TypeError, ValueError) as exc: raise ValueError("supervised run parent pid is invalid") from exc - if parent_pid != os.getppid(): + if not _supervisor_pid_matches(parent_pid): raise ValueError("supervised run parent does not match its reservation") from .supervisor.control_bus import ControlBus From 25cdeb44ca8123459a2e1234bdb3868fcb04b0ec Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 16:59:10 +0300 Subject: [PATCH 08/19] Keep the 0.1.7 test additions runnable on Windows and the argv adapters Three test files added in 0.1.7 regressed the Windows test run: - test_resume.py and test_resume_routes.py monkeypatch os.killpg in their supervisor fixtures; the attribute does not exist on Windows, so every test in both files errored at setup. The patch now tolerates the absence and also stubs the Windows delivery helper the supervisor uses instead. - test_agent_registry.py wrote `#!/bin/sh` probe stubs, which CreateProcess cannot execute; they now go through tests/fake_cli with Python bodies, which builds the same sh launcher on POSIX. - test_reasoning_effort_chain.py read adapter.command_template, the shell string this branch replaced with argv lists; the assertions now inspect adapter.argv. The expected tokens are unchanged. No test semantics change; the full suite now passes on Windows (405 passed, 42 skipped - the documented symlink-privilege skips). --- tests/test_agent_registry.py | 35 +++++++++++++++------------- tests/test_reasoning_effort_chain.py | 9 ++++--- tests/test_resume.py | 11 ++++++++- tests/webapi/test_resume_routes.py | 7 +++++- 4 files changed, 39 insertions(+), 23 deletions(-) diff --git a/tests/test_agent_registry.py b/tests/test_agent_registry.py index 66363bed..75ba7bb6 100644 --- a/tests/test_agent_registry.py +++ b/tests/test_agent_registry.py @@ -16,6 +16,8 @@ ) from lh_harness.cli import _AGENT_CHOICES +from .fake_cli import fake_cli + @pytest.fixture(autouse=True) def _clear_probe_cache(): @@ -30,10 +32,9 @@ def _clear_probe_cache(): def _stub(path: Path, body: str) -> str: - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text("#!/bin/sh\n" + body, encoding="utf-8") - path.chmod(0o755) - return str(path) + # A Python-bodied stand-in runs identically on POSIX and on Windows, + # where `#!/bin/sh` scripts cannot be executed at all. + return fake_cli(path, body) def test_cli_agent_choices_match_the_registry() -> None: @@ -56,7 +57,7 @@ def test_registry_default_models_match_the_cli_table() -> None: def test_binary_on_path_but_not_runnable_is_not_reported_usable(tmp_path: Path) -> None: # Worse than missing: it looks healthy while breaking every run, so it must # never collapse into `usable` or `missing`. - broken = _stub(tmp_path / "codex", "exit 3\n") + broken = _stub(tmp_path / "codex", "sys.exit(3)\n") probes = probe_agents(binaries={"codex": broken}) @@ -67,7 +68,7 @@ def test_binary_on_path_but_not_runnable_is_not_reported_usable(tmp_path: Path) def test_binary_that_prints_no_version_is_not_reported_usable(tmp_path: Path) -> None: - silent = _stub(tmp_path / "codex", "exit 0\n") + silent = _stub(tmp_path / "codex", "pass\n") probes = probe_agents(binaries={"codex": silent}) @@ -82,7 +83,7 @@ def test_missing_binary_is_missing_not_broken() -> None: def test_runnable_binary_reports_its_version(tmp_path: Path) -> None: - good = _stub(tmp_path / "codex", 'echo "codex-cli 9.9.9"\nexit 0\n') + good = _stub(tmp_path / "codex", 'print("codex-cli 9.9.9")\n') probes = probe_agents(binaries={"codex": good}) @@ -94,7 +95,7 @@ def test_runnable_binary_reports_its_version(tmp_path: Path) -> None: def test_explicit_binary_is_probed_instead_of_rediscovering(tmp_path: Path) -> None: # The Web API resolves a path once per request so its response is # self-consistent; re-resolving here could describe another installation. - chosen = _stub(tmp_path / "chosen" / "codex", 'echo "1.0.0"\nexit 0\n') + chosen = _stub(tmp_path / "chosen" / "codex", 'print("1.0.0")\n') probes = probe_agents(binaries={"codex": chosen}) @@ -105,12 +106,12 @@ def test_claude_effort_tiers_are_read_from_cli_help(tmp_path: Path) -> None: # `claude --help` wraps the tier list onto a continuation line. claude = _stub( tmp_path / "claude", - 'if [ "$1" = "--version" ]; then echo "2.1.212"; exit 0; fi\n' - 'cat < Effort level for the current session\n" - " (low, medium, high, xhigh, max)\n" - " --other-flag Something else\n" - "EOF\n", + 'if sys.argv[1:] == ["--version"]:\n' + ' print("2.1.212")\n' + " sys.exit(0)\n" + 'print(" --effort Effort level for the current session")\n' + 'print(" (low, medium, high, xhigh, max)")\n' + 'print(" --other-flag Something else")\n', ) probes = probe_agents(binaries={"claude_code": claude}) @@ -131,8 +132,10 @@ def test_effort_discovery_falls_back_to_declared_tiers_when_help_is_unparsable( ) -> None: claude = _stub( tmp_path / "claude", - 'if [ "$1" = "--version" ]; then echo "2.1.212"; exit 0; fi\n' - 'echo " --effort Effort level (default)"\n', + 'if sys.argv[1:] == ["--version"]:\n' + ' print("2.1.212")\n' + " sys.exit(0)\n" + 'print(" --effort Effort level (default)")\n', ) probes = probe_agents(binaries={"claude_code": claude}) diff --git a/tests/test_reasoning_effort_chain.py b/tests/test_reasoning_effort_chain.py index 95714e6c..6606c239 100644 --- a/tests/test_reasoning_effort_chain.py +++ b/tests/test_reasoning_effort_chain.py @@ -1,7 +1,6 @@ from __future__ import annotations import argparse -import shlex from pathlib import Path import pytest @@ -23,7 +22,7 @@ def test_codex_sends_the_effort_as_a_config_override() -> None: # Codex has no `--effort` flag; the depth is a config value. adapter = _codex(model="gpt-5.6-sol", reasoning_effort="xhigh") - tokens = shlex.split(adapter.command_template.replace("< {prompt_path}", "")) + tokens = adapter.argv assert 'model_reasoning_effort="xhigh"' in tokens assert tokens[tokens.index('model_reasoning_effort="xhigh"') - 1] == "-c" @@ -32,7 +31,7 @@ def test_codex_sends_the_effort_as_a_config_override() -> None: def test_codex_without_an_effort_leaves_the_user_config_in_charge() -> None: adapter = _codex(model="gpt-5.6-sol") - assert "model_reasoning_effort" not in adapter.command_template + assert not any("model_reasoning_effort" in token for token in adapter.argv) def test_claude_sends_the_effort_as_a_cli_flag() -> None: @@ -44,7 +43,7 @@ def test_claude_sends_the_effort_as_a_cli_flag() -> None: reasoning_effort="max", ) - tokens = adapter.command_template.split() + tokens = adapter.argv assert tokens[tokens.index("--effort") + 1] == "max" assert adapter.reasoning_effort == "max" @@ -58,7 +57,7 @@ def test_opencode_sends_the_effort_as_a_variant() -> None: reasoning_effort="high", ) - tokens = adapter.command_template.split() + tokens = adapter.argv assert tokens[tokens.index("--variant") + 1] == "high" diff --git a/tests/test_resume.py b/tests/test_resume.py index 244cfb1a..ad1c8763 100644 --- a/tests/test_resume.py +++ b/tests/test_resume.py @@ -258,7 +258,16 @@ def supervisor(monkeypatch, tmp_path: Path) -> RunSupervisor: "lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: FakeProcess(), ) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda *args, **kwargs: None) + # os.killpg does not exist on Windows; the supervisor signals through the + # process-group helpers there, so patch both ends and tolerate the absence. + monkeypatch.setattr( + "lh_harness.supervisor.service.os.killpg", lambda *args, **kwargs: None, raising=False + ) + monkeypatch.setattr( + "lh_harness.utils.process_group._windows_deliver", + lambda *args, **kwargs: None, + raising=False, + ) workspace = tmp_path / "workspace" workspace.mkdir() return RunSupervisor(tmp_path / "runs", workspace_root=workspace) diff --git a/tests/webapi/test_resume_routes.py b/tests/webapi/test_resume_routes.py index 60666c47..011016c7 100644 --- a/tests/webapi/test_resume_routes.py +++ b/tests/webapi/test_resume_routes.py @@ -29,7 +29,12 @@ def poll(self): def client(monkeypatch, tmp_path: Path): process = FakeProcess() monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *a, **k: process) - monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda *a, **k: None) + # os.killpg does not exist on Windows; the supervisor signals through the + # process-group helpers there, so patch both ends and tolerate the absence. + monkeypatch.setattr("lh_harness.supervisor.service.os.killpg", lambda *a, **k: None, raising=False) + monkeypatch.setattr( + "lh_harness.utils.process_group._windows_deliver", lambda *a, **k: None, raising=False + ) root = tmp_path / "runs" workspace = tmp_path / "workspace" workspace.mkdir() From 732ecfffee74380328c8147dfcb4e40e15aae3fe Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 17:49:29 +0300 Subject: [PATCH 09/19] Keep PWD in step with the directory a child is started in Dropping the shell took a side effect with it: `sh -c` rewrites `PWD` from `getcwd()` when it starts, so `cd && ...` kept the variable and the real working directory in agreement for free. Executing argv directly passes `cwd=` to the OS and leaves the launcher's `PWD` inherited unchanged. That is not cosmetic, because not everything asks the OS. OpenCode resolves the directory its tools operate in from `PWD`, so an agent launched this way worked in whatever directory the operator happened to run `lh-harness` from -- writing files outside the workspace the run promised to contain them in, while the transcript kept naming plausible paths and the auditor confirmed them there. `PWD` is now derived from the `cwd` the child actually gets, in the local environment and in the supervisor's worker launch alike, and `OLDPWD` is dropped rather than blanked so `cd -` behaves like a fresh shell instead of failing. Verified end to end on Linux with all three installed backends launched from a directory outside the workspace: OpenCode now stays inside it, Claude Code and the DeepSeek Harness CLI are unaffected, as they read the working directory from the OS. --- src/lh_harness/environment/local.py | 6 +++++ src/lh_harness/supervisor/service.py | 6 +++++ src/lh_harness/utils/child_env.py | 35 ++++++++++++++++++++++++++++ tests/supervisor/test_hardening.py | 23 ++++++++++++++++++ tests/test_environment_local.py | 33 ++++++++++++++++++++++++++ 5 files changed, 103 insertions(+) create mode 100644 src/lh_harness/utils/child_env.py diff --git a/src/lh_harness/environment/local.py b/src/lh_harness/environment/local.py index 1d982339..f1be4f8d 100644 --- a/src/lh_harness/environment/local.py +++ b/src/lh_harness/environment/local.py @@ -14,6 +14,7 @@ from ..supervisor.control_bus import _ensure_dir_fd_nofollow, _open_private_regular_at from ..trajectory_artifacts import StreamingTrajectoryArtifactWriter from ..utils import paths as long_paths +from ..utils.child_env import apply_working_directory from ..utils.process_group import ( force_kill_process_group, kill_process_group, @@ -205,6 +206,11 @@ async def _spawn( # worker sanitisation would expose that credential to the agent. child_env = os.environ.copy() child_env.pop("LH_HARNESS_WEB_TOKEN", None) + # `cwd=` alone is not enough: an agent CLI that reads `PWD` (OpenCode + # does) would otherwise work in the directory the operator launched + # the harness from, outside the workspace. The old `sh -c` wrapper + # rewrote `PWD` on the shell's own startup and hid this. + apply_working_directory(child_env, cwd) child_env.update(env or {}) proc = await asyncio.create_subprocess_exec( *argv, diff --git a/src/lh_harness/supervisor/service.py b/src/lh_harness/supervisor/service.py index 179381e1..3973afe7 100644 --- a/src/lh_harness/supervisor/service.py +++ b/src/lh_harness/supervisor/service.py @@ -56,6 +56,7 @@ DEFAULT_MAX_ROUNDS, MAX_ROUNDS, ) +from ..utils.child_env import apply_working_directory from ..utils.process_group import deliver_signal, new_process_group_kwargs from ..utils.run_boundary import safe_run_control, safe_run_dir, safe_run_logs, safe_run_role, safe_run_rounds @@ -1815,6 +1816,11 @@ def _launch_worker( os.pathsep + inherited_pythonpath if inherited_pythonpath else "" ) worker_env.pop("LH_HARNESS_WEB_TOKEN", None) + # The worker really starts in the workspace, so `PWD` must say so too: + # it is inherited by every agent CLI the worker spawns, and the ones + # that trust it over `getcwd` would otherwise work in the directory the + # supervisor was started from. + apply_working_directory(worker_env, workspace_path) try: process = subprocess.Popen( command, diff --git a/src/lh_harness/utils/child_env.py b/src/lh_harness/utils/child_env.py new file mode 100644 index 00000000..b65db04c --- /dev/null +++ b/src/lh_harness/utils/child_env.py @@ -0,0 +1,35 @@ +"""Environment variables that must agree with a child's working directory. + +``cwd=`` sets the real working directory of a spawned process, but part of the +world reads ``PWD`` instead of calling ``getcwd``. A shell used to hide this: +``sh -c`` rewrites ``PWD`` from ``getcwd()`` when it starts, so the old +``cd && ...`` command strings kept the two in agreement for free. +Executing argv directly removes that shell, and the launcher's ``PWD`` is then +inherited unchanged -- pointing at wherever the operator typed ``lh-harness``. + +This is not cosmetic. OpenCode resolves the directory its tools work in from +``PWD``, so a stale value sends an agent's writes outside the workspace the run +promised to contain them in, while every path in the transcript still looks +plausible. ``OLDPWD`` is dropped for the same reason: a ``cd -`` in an agent's +shell would jump to a directory this run never chose. +""" + +from __future__ import annotations + +import os + + +def apply_working_directory(env: dict[str, str], cwd: str | os.PathLike[str] | None) -> dict[str, str]: + """Make ``env`` describe ``cwd``, the directory the child is really started in. + + A ``cwd`` of ``None`` leaves ``env`` alone: the child then genuinely + inherits ours, and the inherited ``PWD`` is already the right answer. + """ + + if cwd is None: + return env + env["PWD"] = os.path.abspath(os.fspath(cwd)) + # Removed rather than blanked, or `cd -` in an agent's shell would fail + # confusingly instead of behaving like a fresh shell. + env.pop("OLDPWD", None) + return env diff --git a/tests/supervisor/test_hardening.py b/tests/supervisor/test_hardening.py index 6672b40c..db9073dc 100644 --- a/tests/supervisor/test_hardening.py +++ b/tests/supervisor/test_hardening.py @@ -772,6 +772,29 @@ def launch(*args, **kwargs): assert worker_env.get("LH_HARNESS_WEB_TOKEN") is None +def test_worker_pwd_points_at_the_workspace(monkeypatch, tmp_path: Path) -> None: + """Agent CLIs that read `PWD` inherit it from the worker, not from us.""" + + process = _Process() + captured: dict[str, object] = {} + + def launch(*args, **kwargs): + captured.update(kwargs) + return process + + monkeypatch.setenv("PWD", str(tmp_path / "wherever-the-supervisor-started")) + monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", launch) + supervisor = RunSupervisor(tmp_path / "runs", workspace_root=tmp_path / "workspace") + supervisor.create_run(task="stay in the workspace") + + worker_env = captured.get("env") + assert isinstance(worker_env, dict) + # `abspath`, not `resolve`: the value must describe the directory the worker + # was actually started in, symlinks and all, exactly as a shell's own `PWD` + # would. + assert worker_env.get("PWD") == os.path.abspath(str(captured.get("cwd"))) + + def test_run_listing_skips_symlinked_run_dirs(monkeypatch, tmp_path: Path) -> None: process = _Process() monkeypatch.setattr("lh_harness.supervisor.service.subprocess.Popen", lambda *args, **kwargs: process) diff --git a/tests/test_environment_local.py b/tests/test_environment_local.py index 8b06fa4d..720d70ad 100644 --- a/tests/test_environment_local.py +++ b/tests/test_environment_local.py @@ -8,6 +8,7 @@ from __future__ import annotations import asyncio +import os import sys import time @@ -61,6 +62,38 @@ def test_working_directory_comes_from_cwd(tmp_path, env): assert result.stdout.strip() == str(target) +def test_pwd_agrees_with_the_working_directory(tmp_path, env): + """OpenCode reads `PWD`, not `getcwd`: a stale one puts the agent outside the workspace. + + The `sh -c` wrapper this replaced set `PWD` from `getcwd()` when the shell + started, so nothing here had to. Executing argv directly means the + launcher's `PWD` is inherited unless it is overwritten. + """ + target = tmp_path / "workspace" + target.mkdir() + script = "import os; print(os.environ.get('PWD'))" + result = run(env.run([PY, "-c", script], timeout=30, cwd=str(target))) + assert result.stdout.strip() == str(target) + + +def test_oldpwd_is_not_inherited_when_a_working_directory_is_imposed(monkeypatch, tmp_path, env): + """`cd -` must not jump to a directory this run never chose.""" + monkeypatch.setenv("OLDPWD", str(tmp_path / "somewhere-else")) + target = tmp_path / "workspace" + target.mkdir() + script = "import os; print(os.environ.get('OLDPWD', 'unset'))" + result = run(env.run([PY, "-c", script], timeout=30, cwd=str(target))) + assert result.stdout.strip() == "unset" + + +def test_pwd_is_left_alone_without_a_working_directory(monkeypatch, env): + """No `cwd` means the child really does inherit ours, and so should `PWD`.""" + monkeypatch.setenv("PWD", "/inherited/from/the/launcher") + script = "import os; print(os.environ.get('PWD', 'unset'))" + result = run(env.run([PY, "-c", script], timeout=30)) + assert result.stdout.strip() == "/inherited/from/the/launcher" + + def test_env_overrides_are_layered_onto_the_real_environment(env): """Replaces the `VAR=value cmd` prefix; PATH must survive.""" script = "import os; print(os.environ['LH_TEST_TOKEN'], bool(os.environ.get('PATH')))" From e3ac67970cf4a4cbe7aa1818764b47e4d2774336 Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 17:49:36 +0300 Subject: [PATCH 10/19] Ask the platform, not the path, about drive letters `test_deny_rules_cover_drive_letter_paths` guarded its Windows-only assertion by looking for a colon in `Path("C:/runs/logs").resolve()`. On POSIX that string is a *relative* path, so it resolves to `/C:/runs/logs` -- which contains a colon as well, and the guard was therefore true everywhere. The Windows branch ran on Linux and failed there, the one red test in an otherwise green suite. --- tests/test_adapter_argv.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/test_adapter_argv.py b/tests/test_adapter_argv.py index dd9cc201..2001041e 100644 --- a/tests/test_adapter_argv.py +++ b/tests/test_adapter_argv.py @@ -8,6 +8,7 @@ from __future__ import annotations import asyncio +import os import pytest @@ -217,7 +218,11 @@ def test_deny_rules_cover_drive_letter_paths(): rules = path_deny_rules(("C:/runs/logs",)) joined = " ".join(rules) - if ":" in str(__import__("pathlib").Path("C:/runs/logs").resolve()): + # The platform has to be asked directly. `"C:/runs/logs"` is a *relative* + # path on POSIX, where it resolves to `/C:/runs/logs` -- a string that + # contains a colon too, so inspecting the resolved path for one would run + # the Windows-only assertion on Linux and fail there. + if os.name == "nt": # On Windows the bare drive form must be emitted alongside the // form, # or harness-owned paths would not actually be hidden. assert "Read(C:/runs/logs)" in joined From ed6d772ffecec69d8de3875f9794e30d3df2cb6e Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 18:21:15 +0300 Subject: [PATCH 11/19] Deliver the DeepSeek Harness task via a patch layer on Windows The dsh npm launcher is a .CMD shim, so its arguments travel through cmd.exe and its 8191-character command-line limit. The headless runner takes the task as a positional argument, and a role prompt is far larger than the limit, so every DeepSeek episode on Windows died in 0.3s with "The command line is too long" before reaching the provider. The headless runner resolves its `task` config from the command line, but a later --patch layer may override that row with a literal - the same override mechanism the runner already uses for the model. On Windows the prompt now rides in the existing per-episode patch file as a JSON-escaped (valid YAML) scalar, and the positional argument shrinks to a fixed placeholder that only satisfies the non-empty check. POSIX keeps passing the prompt as the positional argument, exactly as verified on Linux. Verified against dsh 0.1.0-rc.7 on Windows with a 19 KB prompt: the override is honoured and the episode completes. --- src/lh_harness/adapters/deepseek_runner.py | 30 ++++++++++++++++------ tests/test_deepseek_harness_adapter.py | 16 +++++++++--- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/src/lh_harness/adapters/deepseek_runner.py b/src/lh_harness/adapters/deepseek_runner.py index 8c2f2ef0..c9f830af 100644 --- a/src/lh_harness/adapters/deepseek_runner.py +++ b/src/lh_harness/adapters/deepseek_runner.py @@ -35,14 +35,26 @@ def _model_patch_path(prompt_path: Path) -> Path: def run(binary: str, prompt_path: Path, model: str) -> int: try: prompt = prompt_path.read_text(encoding="utf-8") + patch_lines = [ + "- id: agent-default-model", + " config:", + " provider: deepseek-official", + f" model: {json.dumps(model, ensure_ascii=False)}", + ] + if os.name == "nt": + # The dsh npm launcher is a .CMD shim, so the task positional would + # travel through cmd.exe and its 8191-character command-line limit; + # role prompts are far larger. The headless runner's `task` config + # normally resolves from the command line, but a later patch layer + # may override it with a literal, exactly like the model row above. + # JSON string escaping is valid YAML, so the prompt stays one line. + patch_lines += [ + "- id: headless-runner", + " config:", + f" task: {json.dumps(prompt, ensure_ascii=False)}", + ] patch_path = _model_patch_path(prompt_path) - patch_path.write_text( - "- id: agent-default-model\n" - " config:\n" - " provider: deepseek-official\n" - f" model: {json.dumps(model, ensure_ascii=False)}\n", - encoding="utf-8", - ) + patch_path.write_text("\n".join(patch_lines) + "\n", encoding="utf-8") except (OSError, UnicodeError) as exc: message = f"could not prepare DeepSeek Harness prompt: {exc}" sys.stderr.write(message + "\n") @@ -55,7 +67,9 @@ def run(binary: str, prompt_path: Path, model: str) -> int: "headless", "--patch", os.fspath(patch_path), - prompt, + # cmd.exe cannot carry the real prompt (see above); the config override + # supplies it, and this placeholder only satisfies the non-empty check. + "task delivered via --patch" if os.name == "nt" else prompt, ] try: completed = subprocess.run( diff --git a/tests/test_deepseek_harness_adapter.py b/tests/test_deepseek_harness_adapter.py index 4498b649..82f285f0 100644 --- a/tests/test_deepseek_harness_adapter.py +++ b/tests/test_deepseek_harness_adapter.py @@ -2,6 +2,7 @@ import asyncio import json +import os import shlex from pathlib import Path @@ -73,10 +74,19 @@ def test_deepseek_runner_passes_headless_patch_and_emits_jsonl( assert record["type"] == "dsh.result" assert record["is_error"] is False assert "--profile headless --patch" in record["text"] - assert record["text"].endswith("fix the project") patch_path = prompt_path.with_name(f"{prompt_path.name}.dsh-model-patch.yml") - assert "provider: deepseek-official" in patch_path.read_text(encoding="utf-8") - assert 'model: "deepseek-v4-flash"' in patch_path.read_text(encoding="utf-8") + patch_text = patch_path.read_text(encoding="utf-8") + assert "provider: deepseek-official" in patch_text + assert 'model: "deepseek-v4-flash"' in patch_text + if os.name == "nt": + # cmd.exe's 8191-char limit cannot carry a role prompt, so the task + # travels as a patch-layer config override and argv keeps a stand-in. + assert record["text"].endswith("task delivered via --patch") + assert "- id: headless-runner" in patch_text + assert 'task: "fix the project"' in patch_text + else: + assert record["text"].endswith("fix the project") + assert "headless-runner" not in patch_text def test_deepseek_runner_preserves_failure_and_stderr(tmp_path: Path, capfd) -> None: From 7a67eb1f24d76ec08d595bc72ed39ed6327c1e35 Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 19:03:51 +0300 Subject: [PATCH 12/19] Exercise both dsh task deliveries on every platform The patch-layer task route was guarded by `os.name` in the runner and in its test alike, so a POSIX run of the suite never executed the Windows branch at all -- and that branch is a contract with dsh's patch precedence, not with the OS: if dsh ever stopped preferring the patch override, agents would silently receive the placeholder string as their task, and only a Windows machine could have noticed. The delivery is now a parameter (`task_via_patch`, defaulting to the platform rule), the placeholder is a named constant, and a parametrised test drives both routes on both platforms with a 9KB prompt full of YAML-hostile characters: the patch route must keep the prompt off the command line and carry it as one exactly-escaped scalar, the positional route must keep the patch file free of the `headless-runner` row. Behaviour at the defaults is unchanged and still covered by the existing test. --- src/lh_harness/adapters/deepseek_runner.py | 46 ++++++++++++++++------ tests/test_deepseek_harness_adapter.py | 43 +++++++++++++++++++- 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/src/lh_harness/adapters/deepseek_runner.py b/src/lh_harness/adapters/deepseek_runner.py index c9f830af..575b9669 100644 --- a/src/lh_harness/adapters/deepseek_runner.py +++ b/src/lh_harness/adapters/deepseek_runner.py @@ -32,7 +32,33 @@ def _model_patch_path(prompt_path: Path) -> Path: return prompt_path.with_name(f"{prompt_path.name}.dsh-model-patch.yml") -def run(binary: str, prompt_path: Path, model: str) -> int: +# What the positional argument says when the real task rides in the patch +# layer. Deliberately human-readable: if dsh ever stopped preferring the patch +# override, this string is what an agent would receive as its task, and it +# should scream misconfiguration rather than pass as a plausible instruction. +TASK_PLACEHOLDER = "task delivered via --patch" + + +def run( + binary: str, + prompt_path: Path, + model: str, + *, + task_via_patch: bool | None = None, +) -> int: + """Bridge one episode to the dsh headless runner. + + ``task_via_patch`` picks how the prompt reaches dsh; ``None`` follows the + platform. It is a parameter rather than a hardcoded ``os.name`` check so + the test suite exercises both deliveries on both platforms -- the patch + route is a contract with dsh's layer precedence, and a contract only a + Windows machine could test would rot quietly. + """ + if task_via_patch is None: + # The dsh npm launcher is a .CMD shim on Windows, so the task + # positional would travel through cmd.exe and its 8191-character + # command-line limit; role prompts are far larger. + task_via_patch = os.name == "nt" try: prompt = prompt_path.read_text(encoding="utf-8") patch_lines = [ @@ -41,13 +67,11 @@ def run(binary: str, prompt_path: Path, model: str) -> int: " provider: deepseek-official", f" model: {json.dumps(model, ensure_ascii=False)}", ] - if os.name == "nt": - # The dsh npm launcher is a .CMD shim, so the task positional would - # travel through cmd.exe and its 8191-character command-line limit; - # role prompts are far larger. The headless runner's `task` config - # normally resolves from the command line, but a later patch layer - # may override it with a literal, exactly like the model row above. - # JSON string escaping is valid YAML, so the prompt stays one line. + if task_via_patch: + # The headless runner's `task` config normally resolves from the + # command line, but a later patch layer may override that row with + # a literal, exactly like the model row above. JSON string escaping + # is valid YAML, so the prompt stays one line however gnarly it is. patch_lines += [ "- id: headless-runner", " config:", @@ -67,9 +91,9 @@ def run(binary: str, prompt_path: Path, model: str) -> int: "headless", "--patch", os.fspath(patch_path), - # cmd.exe cannot carry the real prompt (see above); the config override - # supplies it, and this placeholder only satisfies the non-empty check. - "task delivered via --patch" if os.name == "nt" else prompt, + # The config override supplies the real prompt on the patch route; the + # placeholder only satisfies the runner's non-empty check. + TASK_PLACEHOLDER if task_via_patch else prompt, ] try: completed = subprocess.run( diff --git a/tests/test_deepseek_harness_adapter.py b/tests/test_deepseek_harness_adapter.py index 82f285f0..2804b8b5 100644 --- a/tests/test_deepseek_harness_adapter.py +++ b/tests/test_deepseek_harness_adapter.py @@ -6,6 +6,7 @@ import shlex from pathlib import Path +import pytest from fastapi.testclient import TestClient from lh_harness import agent_logs @@ -14,7 +15,7 @@ DeepSeekHarnessAdapter, permission_mode_for_role, ) -from lh_harness.adapters.deepseek_runner import run +from lh_harness.adapters.deepseek_runner import TASK_PLACEHOLDER, run from lh_harness.environment.local import LocalEnvironment from lh_harness.types import EpisodeBudget from lh_harness.utils.agent_cli import resolve_dsh_binary @@ -81,7 +82,7 @@ def test_deepseek_runner_passes_headless_patch_and_emits_jsonl( if os.name == "nt": # cmd.exe's 8191-char limit cannot carry a role prompt, so the task # travels as a patch-layer config override and argv keeps a stand-in. - assert record["text"].endswith("task delivered via --patch") + assert record["text"].endswith(TASK_PLACEHOLDER) assert "- id: headless-runner" in patch_text assert 'task: "fix the project"' in patch_text else: @@ -89,6 +90,44 @@ def test_deepseek_runner_passes_headless_patch_and_emits_jsonl( assert "headless-runner" not in patch_text +@pytest.mark.parametrize("via_patch", [False, True]) +def test_deepseek_runner_task_deliveries_work_on_every_platform( + tmp_path: Path, + capsys, + via_patch: bool, +) -> None: + """Both deliveries, exercised regardless of the host OS. + + The patch route is a contract with dsh's layer precedence; if only Windows + machines ever executed it, a change in dsh would surface as agents silently + receiving the placeholder as their task. This is the tripwire. + """ + binary = _executable(tmp_path / "bin" / "dsh", "print(' '.join(sys.argv[1:]))\n") + prompt_path = tmp_path / "prompt.md" + # Large enough that cmd.exe could never carry it, and awkward enough to + # prove the JSON-as-YAML escaping keeps the scalar on one line. + prompt = ("x" * 9000) + '\nline "two" ends with a backslash \\' + prompt_path.write_text(prompt, encoding="utf-8") + + assert run(binary, prompt_path, "deepseek-v4-flash", task_via_patch=via_patch) == 0 + + record = json.loads(capsys.readouterr().out) + assert record["is_error"] is False + patch_path = prompt_path.with_name(f"{prompt_path.name}.dsh-model-patch.yml") + patch_text = patch_path.read_text(encoding="utf-8") + assert "provider: deepseek-official" in patch_text + if via_patch: + # The prompt must not appear on the command line at all... + assert "xxxx" not in record["text"] + assert record["text"].endswith(TASK_PLACEHOLDER) + # ...and must ride in the patch file as one exactly-escaped scalar. + assert f"task: {json.dumps(prompt, ensure_ascii=False)}" in patch_text + assert "- id: headless-runner" in patch_text + else: + assert record["text"].endswith('ends with a backslash \\') + assert "headless-runner" not in patch_text + + def test_deepseek_runner_preserves_failure_and_stderr(tmp_path: Path, capfd) -> None: binary = _executable( tmp_path / "bin" / "dsh", From eec325006e20ed526218daf26fb0d56ac14c5e64 Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 19:03:51 +0300 Subject: [PATCH 13/19] Run the suite on both platforms in CI The PR's own history is the argument: the branch was verified green on each platform by hand, and each round of hand-verification still found something the other platform could not see (a POSIX-only test guard, a cmd.exe-only command-line limit). A matrix of ubuntu + windows at both ends of requires-python (3.10 and 3.14) makes that check automatic for every push and pull request. The suite needs no Node toolchain -- the Web bundle is a packaging artifact -- so the job is checkout, setup-python, `pip install -e ".[test]"`, pytest. The Windows symlink fixtures skip themselves on runners without SeCreateSymbolicLinkPrivilege, which is expected and green. --- .github/workflows/tests.yml | 43 +++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 .github/workflows/tests.yml diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 00000000..1727927c --- /dev/null +++ b/.github/workflows/tests.yml @@ -0,0 +1,43 @@ +name: 🧪 Tests + +on: + push: + branches: + - main + pull_request: + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: tests-${{ github.ref }} + cancel-in-progress: true + +jobs: + pytest: + name: 🐍 ${{ matrix.os }} · py${{ matrix.python }} + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + # Both supported platforms, at both ends of requires-python: the + # oldest interpreter the project promises and the newest it is + # developed against. The suite needs no Node toolchain -- the Web + # bundle is a packaging artifact, not a test dependency. + os: [ubuntu-latest, windows-latest] + python: ["3.10", "3.14"] + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: ${{ matrix.python }} + + - name: 📦 Install with test dependencies + run: python -m pip install -e ".[test]" + + - name: 🧪 Run the suite + # The Windows symlink fixtures skip themselves when the runner lacks + # SeCreateSymbolicLinkPrivilege; those skips are expected and green. + run: python -m pytest -q From c5f75eac0baa04935c451425b49962e56a687329 Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 19:15:25 +0300 Subject: [PATCH 14/19] Probe process liveness without killing the process on Windows The first admin-privileged CI run caught three Windows bugs the local test machine could not see, because SeCreateSymbolicLinkPrivilege changed which tests actually ran and a busier pid space changed what a stray probe could hit. The severe one: two code paths asked "is this process alive" with `os.kill(pid, 0)`. That is the POSIX idiom -- and a kill switch on Windows, where os.kill can only deliver console control events and unconditionally calls TerminateProcess for every other signal value, zero included. The supervisor's `_is_alive` poll would kill the very worker it was checking on (or a pid-reuse victim), and the venv-launcher owner check in `--supervised` adoption would kill the supervisor whose reservation it was validating. On the CI runner, a status poll probing a test's fake pid terminated a live runner process and took pytest down with it (`KeyboardInterrupt`, suite dead after 27 tests). Both sites now use `process_alive`, a real observe-only probe: `tasklist` on Windows, `os.kill(pid, 0)` where it actually means "probe", with pid <= 0 rejected outright (POSIX group semantics, Windows idle process). Also from that run: * The Windows atomic-write branch refused a symlinked *destination*, where the POSIX branch's rename atomically evicts the link and installs a private regular file. Refusing is strictly weaker -- the crash-report writer swallows the OSError and the planted link survives for a later, less careful writer -- and os.replace already operates on the name, never the target. The refusal is gone; Windows now evicts exactly like POSIX. * The supervisor's idempotency lock on Windows surfaced the shared helper's "secure control-bus locking" message; it now reports "secure supervisor locking" like the POSIX branch, keeping the message a caller can match on. New cross-platform tests cover all three: the probe must see a live child and leave it running (the old Windows code kills it right there), must see a dead one, must reject nonsense pids; the atomic write must evict a symlinked destination without touching the file it pointed at. --- src/lh_harness/cli.py | 10 ++--- src/lh_harness/supervisor/control_bus.py | 8 +++- src/lh_harness/supervisor/service.py | 16 +++++--- src/lh_harness/utils/process_group.py | 26 ++++++++++++ tests/supervisor/test_hardening.py | 22 +++++++++++ tests/test_process_alive.py | 50 ++++++++++++++++++++++++ 6 files changed, 120 insertions(+), 12 deletions(-) create mode 100644 tests/test_process_alive.py diff --git a/src/lh_harness/cli.py b/src/lh_harness/cli.py index 6bfddc8d..59cd7bd2 100644 --- a/src/lh_harness/cli.py +++ b/src/lh_harness/cli.py @@ -36,6 +36,7 @@ HarnessConfig, ) from .utils.agent_cli import probe_agent_cli +from .utils.process_group import process_alive from .supervisor.control_bus import ( _append_jsonl as _append_jsonl_nofollow, _atomic_bytes_write, @@ -263,11 +264,10 @@ def _supervisor_pid_matches(recorded_parent: int) -> bool: return True if os.name != "nt": return False - try: - os.kill(recorded_parent, 0) - except OSError: - return False - return True + # process_alive, never ``os.kill(pid, 0)``: on Windows os.kill terminates + # for every non-console-event signal, so the POSIX probe idiom would kill + # the supervisor this check is trying to accept. + return process_alive(recorded_parent) def _adopt_supervised_run_dir( diff --git a/src/lh_harness/supervisor/control_bus.py b/src/lh_harness/supervisor/control_bus.py index 9de3f79f..c7962daf 100644 --- a/src/lh_harness/supervisor/control_bus.py +++ b/src/lh_harness/supervisor/control_bus.py @@ -344,8 +344,12 @@ def _atomic_bytes_write_windows(path: Path, payload: bytes) -> None: from ..utils import paths as long_paths _ensure_dir_windows(path.parent) - if path.is_symlink(): - raise OSError(f"refusing to follow a reparse point: {path}") + # No reparse-point refusal for the *destination*: replacing operates on the + # name and never follows it. The POSIX branch's ``rename`` atomically + # evicts a planted symlink and installs a private regular file in its + # place, and ``os.replace`` (MoveFileEx) does exactly the same here -- + # refusing would be strictly weaker, leaving the attacker's link standing + # for some later, less careful writer to follow. temporary = path.with_name(f".{path.name}.{uuid.uuid4().hex[:12]}.tmp") target_tmp = long_paths.os_path(temporary) try: diff --git a/src/lh_harness/supervisor/service.py b/src/lh_harness/supervisor/service.py index 3973afe7..10c3d7ea 100644 --- a/src/lh_harness/supervisor/service.py +++ b/src/lh_harness/supervisor/service.py @@ -57,7 +57,7 @@ MAX_ROUNDS, ) from ..utils.child_env import apply_working_directory -from ..utils.process_group import deliver_signal, new_process_group_kwargs +from ..utils.process_group import deliver_signal, new_process_group_kwargs, process_alive from ..utils.run_boundary import safe_run_control, safe_run_dir, safe_run_logs, safe_run_role, safe_run_rounds _IS_WINDOWS = sys.platform == "win32" @@ -776,7 +776,12 @@ def _supervisor_locked(self): lock_path = self.runs_root / ".supervisor.lock" if _IS_WINDOWS: - windows_handle = _acquire_lock_windows(lock_path) + try: + windows_handle = _acquire_lock_windows(lock_path) + except RuntimeError as exc: + # The shared Windows helper reports in control-bus terms; keep + # this lock's own contract for the supervisor's callers. + raise RuntimeError("secure supervisor locking is unavailable") from exc try: yield finally: @@ -959,9 +964,10 @@ def _is_alive(self, run_id: str) -> bool: pid = int(owner.get("pid", 0) or 0) if pid <= 0: return False - try: - os.kill(pid, 0) - except OSError: + # A dedicated probe, not ``os.kill(pid, 0)``: on Windows os.kill + # cannot ask, only terminate, so the POSIX idiom would kill the very + # worker (or an innocent pid-reuse victim) it is checking on. + if not process_alive(pid): return False # An embedded supervisor controls its hosting process directly. The # PID cannot be reused while that same process is executing this code; diff --git a/src/lh_harness/utils/process_group.py b/src/lh_harness/utils/process_group.py index dfe79e84..3c542326 100644 --- a/src/lh_harness/utils/process_group.py +++ b/src/lh_harness/utils/process_group.py @@ -68,6 +68,32 @@ def process_group_alive(pid: int) -> bool: return _posix_signal(pid, 0) +def process_alive(pid: int) -> bool: + """Whether this exact process is still running, without touching it. + + Deliberately NOT ``os.kill(pid, 0)`` on Windows: ``os.kill`` there can + only deliver console control events, and *any other* signal value -- zero + included -- unconditionally kills the target via ``TerminateProcess``. + The idiomatic POSIX liveness probe is a kill switch on Windows. + """ + if pid <= 0: + # POSIX gives pid 0 and negatives group semantics (os.kill(0, 0) + # probes our own group and always succeeds), and Windows pid 0 is the + # idle process, which tasklist happily reports. Neither is ever a + # worker of ours. + return False + if IS_WINDOWS: + return _windows_alive(pid) + try: + os.kill(pid, 0) + except PermissionError: + # The pid exists; it just belongs to someone else. + return True + except OSError: + return False + return True + + def terminate_process_group(pid: int) -> bool: """Ask the group to exit cleanly; False means it is already gone. diff --git a/tests/supervisor/test_hardening.py b/tests/supervisor/test_hardening.py index db9073dc..8b3c6678 100644 --- a/tests/supervisor/test_hardening.py +++ b/tests/supervisor/test_hardening.py @@ -369,6 +369,28 @@ def test_iter_run_control_dirs_skips_symlinked_run_boundaries(tmp_path: Path) -> assert list(iter_run_control_dirs(root)) == [root / "real"] +def test_atomic_write_evicts_a_symlinked_destination(tmp_path: Path) -> None: + """A planted symlink is replaced by a private regular file, never followed. + + This is what POSIX ``rename`` already guarantees and what the first + admin-privileged Windows CI run showed the Windows branch refusing to do: + it raised on the reparse point, the crash-report writer swallowed the + OSError, and the attacker's link stayed in place for a later writer. + """ + from lh_harness.supervisor.control_bus import _atomic_bytes_write + + outside = tmp_path / "outside.json" + outside.write_text("private", encoding="utf-8") + destination = tmp_path / "report.json" + destination.symlink_to(outside) + + _atomic_bytes_write(destination, b"{}") + + assert not destination.is_symlink() + assert destination.read_bytes() == b"{}" + assert outside.read_text(encoding="utf-8") == "private" + + def test_atomic_json_write_closes_mkstemp_fd_when_fdopen_fails(monkeypatch, tmp_path: Path) -> None: """The temporary descriptor must not leak if wrapping it raises.""" diff --git a/tests/test_process_alive.py b/tests/test_process_alive.py new file mode 100644 index 00000000..6d80b461 --- /dev/null +++ b/tests/test_process_alive.py @@ -0,0 +1,50 @@ +"""The liveness probe must observe, never act. + +`os.kill(pid, 0)` is the POSIX idiom for "does this process exist" -- and a +kill switch on Windows, where os.kill can only deliver console control events +and terminates the target for every other signal value, zero included. The +first CI run on a Windows machine with admin rights demonstrated the failure +mode: a supervisor status poll probing a fake test pid terminated an unrelated +live process on the runner and took the whole pytest process down with it. +""" + +from __future__ import annotations + +import subprocess +import sys +import time + +from lh_harness.utils.process_group import process_alive + + +def test_probe_sees_a_live_process_and_does_not_kill_it(): + child = subprocess.Popen( + [sys.executable, "-c", "import time; time.sleep(30)"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + try: + assert process_alive(child.pid) is True + # The probe is the whole test: after it, the child must still be + # running. The old Windows implementation terminated it right here. + time.sleep(0.3) + assert child.poll() is None + assert process_alive(child.pid) is True + finally: + child.kill() + child.wait(timeout=10) + + +def test_probe_sees_a_dead_process(): + child = subprocess.Popen([sys.executable, "-c", "pass"]) + child.wait(timeout=30) + # PID reuse cannot strike in the instant between wait() returning and the + # probe; Windows keeps the pid reserved while the handle is open. + assert process_alive(child.pid) is False + + +def test_probe_rejects_nonsense_pids(): + # pid 0 is our own process group on POSIX and the idle process on Windows; + # negative pids are POSIX group addresses. None can be a tracked worker. + assert process_alive(0) is False + assert process_alive(-1) is False From 6a0bde73e900f38cea2c15158604ed4138f795ca Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 19:22:47 +0300 Subject: [PATCH 15/19] Keep the platform probes and the cross-platform dsh test honest under CI Two corrections to the previous commit's own additions, both caught by the first Windows CI round that got past the liveness-probe kill: * ``_run_quiet`` reached ``tasklist``/``taskkill`` through ``subprocess.run``, which resolves ``Popen`` through the module namespace at call time -- and the supervisor tests stub ``subprocess.Popen`` with a fake worker that is no context manager, so every resume-path status refresh on Windows died with ``AttributeError: __enter__``. The probes now hold the real ``Popen`` captured at import, the same pattern service.py already documents for its worker spawn. * The both-routes dsh test drove the *positional* delivery with a 9KB multi-line prompt on every platform. On Windows that argument cannot fit through the .CMD shim -- the exact limitation the patch route exists to bypass -- so the test was asserting the platform bug away. The big hostile prompt now exercises the patch route only; the positional case proves just the route itself with a plain prompt. --- src/lh_harness/utils/process_group.py | 27 ++++++++++++++++++++------ tests/test_deepseek_harness_adapter.py | 15 ++++++++++---- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/src/lh_harness/utils/process_group.py b/src/lh_harness/utils/process_group.py index 3c542326..0019ce90 100644 --- a/src/lh_harness/utils/process_group.py +++ b/src/lh_harness/utils/process_group.py @@ -201,22 +201,37 @@ def _windows_alive(pid: int) -> bool: )[1] +# Captured at import time, the same way service.py keeps ``_REAL_POPEN``: +# tests routinely stub ``subprocess.Popen`` to model a worker, and the platform +# probes below must keep reaching the real OS regardless -- ``subprocess.run`` +# resolves ``Popen`` through the (stubbed) module namespace at call time. +_REAL_POPEN = subprocess.Popen + + def _run_quiet(argv: list[str], *, capture: bool = False): """Run a Windows helper without flashing a console window.""" + proc = None try: - completed = subprocess.run( + proc = _REAL_POPEN( argv, - capture_output=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + stdin=subprocess.DEVNULL, text=True, - timeout=15, - check=False, creationflags=getattr(subprocess, "CREATE_NO_WINDOW", 0), ) + stdout, stderr = proc.communicate(timeout=15) except (OSError, subprocess.SubprocessError): + if proc is not None: + try: + proc.kill() + proc.communicate(timeout=15) + except (OSError, subprocess.SubprocessError): + pass return (1, "") if capture else 1 if not capture: - return completed.returncode - return completed.returncode, (completed.stdout or "") + (completed.stderr or "") + return proc.returncode + return proc.returncode, (stdout or "") + (stderr or "") def _install_handlers() -> None: diff --git a/tests/test_deepseek_harness_adapter.py b/tests/test_deepseek_harness_adapter.py index 2804b8b5..faad83de 100644 --- a/tests/test_deepseek_harness_adapter.py +++ b/tests/test_deepseek_harness_adapter.py @@ -104,9 +104,16 @@ def test_deepseek_runner_task_deliveries_work_on_every_platform( """ binary = _executable(tmp_path / "bin" / "dsh", "print(' '.join(sys.argv[1:]))\n") prompt_path = tmp_path / "prompt.md" - # Large enough that cmd.exe could never carry it, and awkward enough to - # prove the JSON-as-YAML escaping keeps the scalar on one line. - prompt = ("x" * 9000) + '\nline "two" ends with a backslash \\' + if via_patch: + # Large enough that cmd.exe could never carry it, and awkward enough + # to prove the JSON-as-YAML escaping keeps the scalar on one line. + prompt = ("x" * 9000) + '\nline "two" ends with a backslash \\' + else: + # The positional route genuinely cannot carry a large, multi-line or + # quote-riddled argument through the Windows .CMD shim -- those limits + # are the whole reason the patch route exists -- so this case proves + # only the route itself: prompt on argv, no override row in the patch. + prompt = "fix the project via the positional route" prompt_path.write_text(prompt, encoding="utf-8") assert run(binary, prompt_path, "deepseek-v4-flash", task_via_patch=via_patch) == 0 @@ -124,7 +131,7 @@ def test_deepseek_runner_task_deliveries_work_on_every_platform( assert f"task: {json.dumps(prompt, ensure_ascii=False)}" in patch_text assert "- id: headless-runner" in patch_text else: - assert record["text"].endswith('ends with a backslash \\') + assert record["text"].endswith("fix the project via the positional route") assert "headless-runner" not in patch_text From 3e1bcd913e4047b3949ad94f25563dc6958388aa Mon Sep 17 00:00:00 2001 From: TON14 Date: Thu, 20 Aug 2026 20:02:35 +0300 Subject: [PATCH 16/19] Revert "Wait out transient provider failures instead of aborting the run" This reverts commit cbf36a88d7d93df4c8a000434a22bbd51a701e88. --- src/lh_harness/manager.py | 143 ++++-------------------------- src/lh_harness/provider_errors.py | 34 +------ 2 files changed, 20 insertions(+), 157 deletions(-) diff --git a/src/lh_harness/manager.py b/src/lh_harness/manager.py index f68fb10e..76ffbb9b 100644 --- a/src/lh_harness/manager.py +++ b/src/lh_harness/manager.py @@ -23,7 +23,7 @@ from .environment.base import Environment from .environment.remote_files import ensure_remote_dir, write_remote_text from .runtime_signals import hard_signal_labels -from .provider_errors import classify_agent_runtime_failure, is_retryable_failure +from .provider_errors import classify_agent_runtime_failure from .trajectory_artifacts import persist_trajectory_artifacts from .utils import paths as long_paths from .role_prompts import ( @@ -409,9 +409,6 @@ def emit(event: str, **payload: Any) -> None: env, manager_budget, live_trajectory_path=str(round_dir / "manager_raw_trajectory.jsonl"), - on_retry=lambda a, d, f: _emit_retry_event( - events_path, emit, round_index, "manager", a, d, f - ), ) _save_role_result( round_dir, @@ -670,9 +667,6 @@ def emit(event: str, **payload: Any) -> None: env, executor_budget, live_trajectory_path=str(round_dir / "executor_raw_trajectory.jsonl"), - on_retry=lambda a, d, f: _emit_retry_event( - events_path, emit, round_index, f"{next_step}_executor", a, d, f - ), ) executor_episode_root = log_dir / ( "gui_executor_episodes" if next_step == MANAGER_NEXT_GUI else "cli_executor_episodes" @@ -813,9 +807,6 @@ def emit(event: str, **payload: Any) -> None: env, auditor_budget, live_trajectory_path=str(round_dir / "auditor_raw_trajectory.jsonl"), - on_retry=lambda a, d, f: _emit_retry_event( - events_path, emit, round_index, f"{next_step}_auditor", a, d, f - ), ) auditor_episode_root = log_dir / ( "gui_auditor_episodes" if next_step == MANAGER_NEXT_GUI else "cli_auditor_episodes" @@ -1233,74 +1224,6 @@ def _executor_binding( return cli_executor_agent, cli_executor_budget -@dataclass(frozen=True) -class _RetryPolicy: - max_attempts: int - base_seconds: float - cap_seconds: float - max_total_seconds: float - - -def _env_float(name: str, default: float) -> float: - raw = os.getenv(name) - if raw is None or not raw.strip(): - return default - try: - value = float(raw) - except ValueError: - return default - return value if value >= 0 else default - - -def _provider_retry_policy() -> _RetryPolicy: - """Backoff for transient provider failures (rate limit / network). - - Defaults wait out a rate-limit window (exp backoff 60s -> cap 900s, up to 8 - attempts or 2h total) so a run PAUSES and RESUMES instead of aborting the - moment the provider returns a 429/overloaded/connection error. Override via - LH_HARNESS_PROVIDER_RETRY_{MAX_ATTEMPTS,BASE_SECONDS,CAP_SECONDS,MAX_TOTAL_SECONDS}; - set MAX_ATTEMPTS=0 to restore the old fail-fast behaviour. - """ - return _RetryPolicy( - max_attempts=int(_env_float("LH_HARNESS_PROVIDER_RETRY_MAX_ATTEMPTS", 8)), - base_seconds=_env_float("LH_HARNESS_PROVIDER_RETRY_BASE_SECONDS", 60.0), - cap_seconds=_env_float("LH_HARNESS_PROVIDER_RETRY_CAP_SECONDS", 900.0), - max_total_seconds=_env_float("LH_HARNESS_PROVIDER_RETRY_MAX_TOTAL_SECONDS", 7200.0), - ) - - -def _emit_retry_event( - events_path: Path, - emit: Callable[..., None], - round_index: int, - phase: str, - attempt: int, - delay: float, - failure: Any, -) -> None: - """Surface a provider backoff wait in the event log + progress stream.""" - _append_event( - events_path, - "agent_runtime_retry", - { - "round": round_index, - "phase": phase, - "attempt": attempt, - "delay_seconds": round(delay, 1), - "kind": failure.kind, - "message": failure.message, - }, - ) - emit( - "role_retry", - round=round_index, - role=phase, - attempt=attempt, - delay_ms=int(delay * 1000), - error=failure.user_message, - ) - - async def _run_role_episode( agent: AgentAdapter, prompt: str, @@ -1308,55 +1231,23 @@ async def _run_role_episode( budget: EpisodeBudget, *, live_trajectory_path: str | None = None, - on_retry: Callable[[int, float, Any], None] | None = None, ) -> EpisodeResult: - """Run one role episode. - - Normalizes cooperative cancellation, and waits out transient provider - failures (rate limit / network) with exponential backoff so the run pauses - and resumes rather than aborting. Terminal failures return immediately for - the caller to classify and abort as before. - """ - policy = _provider_retry_policy() - started_total = time.monotonic() - attempt = 0 - while True: - started = time.monotonic() - try: - result = await agent.run_episode( - prompt, - env, - budget, - live_trajectory_path=live_trajectory_path, - ) - except asyncio.CancelledError: - return EpisodeResult( - status="cancelled", - error="Execution cancelled by operator", - duration_ms=int((time.monotonic() - started) * 1000), - metadata={"cancelled": True}, - ) - failure = classify_agent_runtime_failure(result) - if not is_retryable_failure(failure): - return result - attempt += 1 - elapsed = time.monotonic() - started_total - if policy.max_attempts <= 0 or attempt >= policy.max_attempts or elapsed >= policy.max_total_seconds: - return result - delay = min(policy.cap_seconds, policy.base_seconds * (2 ** (attempt - 1))) - delay = max(0.0, min(delay, policy.max_total_seconds - elapsed)) - if on_retry is not None: - with contextlib.suppress(Exception): - on_retry(attempt, delay, failure) - try: - await asyncio.sleep(delay) - except asyncio.CancelledError: - return EpisodeResult( - status="cancelled", - error="Execution cancelled by operator during provider backoff", - duration_ms=int((time.monotonic() - started) * 1000), - metadata={"cancelled": True}, - ) + """Normalize cooperative cancellation for every adapter implementation.""" + started = time.monotonic() + try: + return await agent.run_episode( + prompt, + env, + budget, + live_trajectory_path=live_trajectory_path, + ) + except asyncio.CancelledError: + return EpisodeResult( + status="cancelled", + error="Execution cancelled by operator", + duration_ms=int((time.monotonic() - started) * 1000), + metadata={"cancelled": True}, + ) async def _auditor_report_with_format_repair( diff --git a/src/lh_harness/provider_errors.py b/src/lh_harness/provider_errors.py index 40dee9ad..6e1efd4e 100644 --- a/src/lh_harness/provider_errors.py +++ b/src/lh_harness/provider_errors.py @@ -29,18 +29,6 @@ class AgentRuntimeFailure: user_message: str -# Provider failures that are transient: the provider is momentarily unhappy -# (429/overloaded, dropped connection) but the run can continue once it recovers. -# Terminal kinds (authentication, quota/billing, model_unavailable, timeout) are -# NOT here — retrying those wastes time or masks a real hang. -RETRYABLE_KINDS: frozenset[str] = frozenset({"rate_limit", "network"}) - - -def is_retryable_failure(failure: "AgentRuntimeFailure | None") -> bool: - """True when the failure is a transient provider hiccup worth waiting out.""" - return failure is not None and failure.kind in RETRYABLE_KINDS - - _CLASSIFIERS: tuple[tuple[str, re.Pattern[str], str], ...] = ( ( "model_unavailable", @@ -73,11 +61,7 @@ def is_retryable_failure(failure: "AgentRuntimeFailure | None") -> bool: ), ( "rate_limit", - re.compile( - r"(?:\b429\b|\b529\b|rate[ _-]?limit|ratelimittype|five[_ -]?hour|session limit|" - r"usage limit|too many requests|overloaded|请求过多|限流|过载|会话限制|用量限制)", - re.I, - ), + re.compile(r"(?:\b429\b|rate[ _-]?limit|too many requests|overloaded|请求过多|限流|过载)", re.I), "Provider 限流或过载", ), ( @@ -204,20 +188,8 @@ def _failure_messages(result: EpisodeResult, metadata: dict[str, Any]) -> list[s _append(values, error.get("message") if isinstance(error, dict) else error) elif record_type == "error": _append(values, record.get("message") or record.get("error")) - elif record_type == "result": - # A session/usage-limit rejection comes back as subtype "success" with - # is_error false but api_error_status 429 and a "session limit" result - # string, so key off the HTTP status too, not just is_error. - status_code = record.get("api_error_status") or record.get("status_code") - if record.get("is_error") or (isinstance(status_code, int) and status_code >= 400): - _append(values, record.get("result") or record.get("error") or record.get("subtype")) - elif record_type == "rate_limit_event": - info = record.get("rate_limit_info") - if isinstance(info, dict) and "rejected" in { - str(info.get("status", "")).lower(), - str(info.get("overageStatus", "")).lower(), - }: - _append(values, "provider rate limit rejected (session limit reached)") + elif record_type == "result" and record.get("is_error"): + _append(values, record.get("result") or record.get("error") or record.get("subtype")) _append(values, result.error) _append(values, metadata.get("stderr_tail")) signals = metadata.get("runtime_signals") From a547c14fbacdfb1e353b2284a0feee3936d28916 Mon Sep 17 00:00:00 2001 From: TON14 Date: Sun, 23 Aug 2026 10:14:44 +0300 Subject: [PATCH 17/19] Let runs opt Claude Code agents out of the operator's own toolbox A harness agent runs under the operator's account, so `claude` loads everything that account has accumulated: plugins, skills, hooks, user-level CLAUDE.md. None of it was chosen for the run -- it spends context tokens in every episode, a skill description can trigger on task text it was never meant for, and a hook can rewrite tool calls the harness believes it controls. The harness already isolates the DeepSeek CLI behind its own DSH_HOME and scopes MCP with --strict-mcp-config; Claude Code now gets the same property, opt-in and selective: claude_isolation = true claude_allowed_plugins = ["playwright"] claude_allowed_skills = ["graphify"] (config keys, with matching --claude-isolation / --claude-allowed-plugin / --claude-allowed-skill flags; naming anything in an allow-list implies isolation, because an allow-list only means something against a clean slate.) Isolation passes `--setting-sources project`, which drops exactly the account-level layer while keeping what the workspace repo itself declares -- deliberately not `--bare`, which also skips the account's OAuth credentials and fails every keyless run with "Not logged in" (verified against claude 2.1.239). Allowed plugins resolve through the CLI's own installed_plugins.json rather than by guessing cache paths, with bare names rejected as ambiguous when two marketplaces both carry them. A skill has no standalone CLI switch, but a plugin may carry skills, so the allowed skills are copied -- not symlinked, Windows accounts usually cannot make links -- into one synthesised plugin under the run's tmp tree, which also freezes the skill for the run's duration. With nothing re-admitted, --disable-slash-commands closes the explicit /skill-name escape hatch too. Verified live: an isolated run completes with a clean audit; the skill list visible to the agent shrinks to the CLI's built-ins plus exactly the allowed entries (lh-harness-allowed-skills:graphify), and the operator's other plugins and skills are gone from every role. --- src/lh_harness/adapters/claude_code.py | 32 ++++ src/lh_harness/adapters/claude_isolation.py | 147 ++++++++++++++++++ src/lh_harness/cli.py | 38 ++++- src/lh_harness/config.py | 24 +++ tests/test_claude_isolation.py | 159 ++++++++++++++++++++ 5 files changed, 399 insertions(+), 1 deletion(-) create mode 100644 src/lh_harness/adapters/claude_isolation.py create mode 100644 tests/test_claude_isolation.py diff --git a/src/lh_harness/adapters/claude_code.py b/src/lh_harness/adapters/claude_code.py index 116412e2..b0e65afe 100644 --- a/src/lh_harness/adapters/claude_code.py +++ b/src/lh_harness/adapters/claude_code.py @@ -5,6 +5,7 @@ from ..agent_logs import visible_output as extract_claude_visible_output from ..agent_registry import normalise_reasoning_effort +from .claude_isolation import build_skills_plugin, resolve_plugin_dirs from .claude_permissions import ( ClaudeRole, is_auditor_role, @@ -40,8 +41,15 @@ def __init__( hidden_paths: tuple[str, ...] = (), guard_exclude_paths: tuple[str, ...] = (), reasoning_effort: str | None = None, + isolation: bool = False, + allowed_plugins: tuple[str, ...] = (), + allowed_skills: tuple[str, ...] = (), ) -> None: policy = policy_for_role(role) + # Naming what is allowed only makes sense against a clean slate, so a + # non-empty allow-list implies isolation rather than silently doing + # nothing without the boolean. + isolation = bool(isolation or allowed_plugins or allowed_skills) effort = normalise_reasoning_effort(reasoning_effort) env_overrides: dict[str, str] = {} if api_key: @@ -96,6 +104,27 @@ def __init__( "--verbose", "--dangerously-skip-permissions", ] + if isolation: + # Project-only setting sources drop everything the *operator's + # account* accumulated -- user plugins, skills, hooks, user-level + # CLAUDE.md -- while keeping what the workspace repo itself + # declares, which the task may rely on. Not `--bare`: bare mode + # also skips the account's OAuth credentials, so every run without + # an explicit API key dies with "Not logged in". + argv.extend(["--setting-sources", "project"]) + plugin_dirs = resolve_plugin_dirs(tuple(allowed_plugins)) + skills_plugin = build_skills_plugin( + tuple(allowed_skills), + Path(prompt_dir).parent / "claude-allowed-skills", + ) + if skills_plugin: + plugin_dirs.append(skills_plugin) + for plugin_dir in plugin_dirs: + argv.extend(["--plugin-dir", plugin_dir]) + if not plugin_dirs: + # Nothing was re-admitted, so no skill should resolve even if + # explicitly named in task text. + argv.append("--disable-slash-commands") deny_tools = [*policy.disallowed_tools, *path_deny_rules(hidden_paths)] if deny_tools: argv.append("--disallowedTools") @@ -112,6 +141,9 @@ def __init__( self.role = role self.policy = policy self.reasoning_effort = effort + self.isolation = isolation + self.allowed_plugins = tuple(allowed_plugins) + self.allowed_skills = tuple(allowed_skills) # Snapshot-only exclusions: unlike hidden_paths these are not denied # to the agent — the guard just refrains from walking directories that # legitimately churn (build outputs) during an audit window. diff --git a/src/lh_harness/adapters/claude_isolation.py b/src/lh_harness/adapters/claude_isolation.py new file mode 100644 index 00000000..f5d47941 --- /dev/null +++ b/src/lh_harness/adapters/claude_isolation.py @@ -0,0 +1,147 @@ +"""Keep the operator's Claude Code customisations out of harness agents. + +A harness agent is a subprocess of the operator's account, so by default the +``claude`` CLI loads everything that account has accumulated: plugins, skills, +hooks, user-level CLAUDE.md. None of that was chosen for the run. It spends +context tokens in every episode, a skill description can trigger on task text +it was never meant for, and a hook can rewrite tool calls the harness believes +it controls. The harness already isolates the DeepSeek CLI behind its own +``DSH_HOME`` and scopes MCP with ``--strict-mcp-config``; this module gives +Claude Code the same property. + +Isolation is opt-in and selective, because sometimes part of the toolbox is +wanted: ``--bare`` starts the CLI without the account's context, and the +allow-lists re-admit exactly the named plugins and skills. Plugins resolve +through the CLI's own install manifest rather than by guessing cache paths. +A skill has no standalone CLI switch, but a plugin may carry skills, so the +allowed skills ride in a synthesised single-purpose plugin directory. +""" + +from __future__ import annotations + +import json +import re +import shutil +from pathlib import Path + +_NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$") + +SKILLS_PLUGIN_NAME = "lh-harness-allowed-skills" + + +def _checked_name(name: str, *, kind: str) -> str: + cleaned = str(name or "").strip() + if not _NAME_RE.match(cleaned): + raise ValueError( + f"invalid {kind} name {name!r}: letters, digits, '.', '_' and '-' only" + ) + return cleaned + + +def default_claude_home() -> Path: + return Path.home() / ".claude" + + +def resolve_plugin_dirs( + names: list[str] | tuple[str, ...], + *, + claude_home: Path | None = None, +) -> list[str]: + """Map installed-plugin names to their on-disk directories. + + Reads the CLI's own ``installed_plugins.json`` (the authority on what is + installed and where) instead of walking the cache layout, which encodes + marketplace and version segments that are not a stable interface. A name + matches either bare (``playwright``) or fully qualified + (``playwright@claude-plugins-official``); a bare name that matches more + than one marketplace is an error rather than a guess. + """ + + if not names: + return [] + home = claude_home or default_claude_home() + manifest_path = home / "plugins" / "installed_plugins.json" + try: + manifest = json.loads(manifest_path.read_text(encoding="utf-8")) + except FileNotFoundError: + raise ValueError( + f"no Claude Code plugins are installed (missing {manifest_path})" + ) from None + except (OSError, json.JSONDecodeError) as exc: + raise ValueError(f"could not read {manifest_path}: {exc}") from exc + + entries: dict[str, list[dict]] = manifest.get("plugins") or {} + resolved: list[str] = [] + for raw in names: + wanted = _checked_name(raw, kind="plugin") if "@" not in str(raw) else str(raw).strip() + matches = [ + installs + for qualified, installs in entries.items() + if qualified == wanted or qualified.split("@", 1)[0] == wanted + ] + if not matches: + installed = ", ".join(sorted(entries)) or "none" + raise ValueError( + f"Claude Code plugin {wanted!r} is not installed (installed: {installed})" + ) + if len(matches) > 1: + raise ValueError( + f"Claude Code plugin name {wanted!r} is ambiguous across marketplaces; " + "qualify it as name@marketplace" + ) + installs = matches[0] + install_path = str((installs or [{}])[0].get("installPath") or "") + if not install_path or not (Path(install_path) / ".claude-plugin" / "plugin.json").is_file(): + raise ValueError( + f"Claude Code plugin {wanted!r} has no usable install at {install_path!r}" + ) + resolved.append(install_path) + return resolved + + +def build_skills_plugin( + names: list[str] | tuple[str, ...], + target_dir: Path, + *, + claude_home: Path | None = None, +) -> str | None: + """Materialise the allowed user skills as one synthesised plugin. + + Copies (never symlinks: creating links needs a privilege most Windows + accounts lack) each ``~/.claude/skills/`` into a plugin-shaped + directory under the run's own tree. The copy also freezes the skill for + the run: an edit to the live skill mid-run cannot change agent behaviour + between episodes. + """ + + if not names: + return None + home = claude_home or default_claude_home() + skills_root = home / "skills" + target_dir = Path(target_dir) + if target_dir.exists(): + shutil.rmtree(target_dir) + (target_dir / ".claude-plugin").mkdir(parents=True) + (target_dir / ".claude-plugin" / "plugin.json").write_text( + json.dumps( + { + "name": SKILLS_PLUGIN_NAME, + "version": "0.0.0", + "description": "User skills explicitly allowed for this LongHorizon-Harness run.", + }, + indent=2, + ) + + "\n", + encoding="utf-8", + ) + for raw in names: + name = _checked_name(raw, kind="skill") + source = skills_root / name + if not (source / "SKILL.md").is_file(): + available = ", ".join(sorted(p.name for p in skills_root.glob("*/") )) or "none" + raise ValueError( + f"Claude Code skill {name!r} not found under {skills_root} " + f"(available: {available})" + ) + shutil.copytree(source, target_dir / "skills" / name) + return str(target_dir) diff --git a/src/lh_harness/cli.py b/src/lh_harness/cli.py index 59cd7bd2..570c0181 100644 --- a/src/lh_harness/cli.py +++ b/src/lh_harness/cli.py @@ -586,6 +586,28 @@ def add_command(name: str, help_text: str) -> argparse.ArgumentParser: help="MCP config for Codex, a TOML file holding `[mcp_servers.]` tables. Overrides " "the installed computer-use plugin, which is loaded automatically otherwise.", ) + run_parser.add_argument( + "--claude-isolation", + action=argparse.BooleanOptionalAction, + default=run_default("claude_isolation", False), + help="Start Claude Code agents bare: without this account's plugins, skills, " + "hooks and user-level CLAUDE.md. Naming --claude-allowed-plugin/-skill " + "implies this.", + ) + run_parser.add_argument( + "--claude-allowed-plugin", + action="append", + default=None, + help="Installed Claude Code plugin to re-admit into isolated agents " + "(name or name@marketplace). May be repeated; implies --claude-isolation.", + ) + run_parser.add_argument( + "--claude-allowed-skill", + action="append", + default=None, + help="User-level Claude Code skill (~/.claude/skills/) to re-admit into " + "isolated agents. May be repeated; implies --claude-isolation.", + ) run_parser.add_argument( "--mcp-add-dir", action="append", @@ -1829,6 +1851,9 @@ def build_role_agent(role: str, *, permission_role: str | None = None): hidden_paths=hidden_paths, guard_exclude_paths=guard_exclude_paths, reasoning_effort=effort, + claude_isolation=bool(getattr(args, "claude_isolation", False)), + claude_allowed_plugins=tuple(getattr(args, "claude_allowed_plugin", []) or ()), + claude_allowed_skills=tuple(getattr(args, "claude_allowed_skill", []) or ()), ) return agent_cache[key] @@ -1936,7 +1961,12 @@ def _outermost_paths(*paths: str | Path) -> tuple[str, ...]: return tuple(str(path) for path in kept) -_REPEATABLE_RUN_OPTIONS = ("mcp_add_dir", "guard_exclude_path") +_REPEATABLE_RUN_OPTIONS = ( + "mcp_add_dir", + "guard_exclude_path", + "claude_allowed_plugin", + "claude_allowed_skill", +) def _apply_repeatable_defaults(args: argparse.Namespace, run_defaults: dict[str, object]) -> None: @@ -2275,6 +2305,9 @@ def _build_agent( hidden_paths: tuple[str, ...] = (), guard_exclude_paths: tuple[str, ...] = (), reasoning_effort: str | None = None, + claude_isolation: bool = False, + claude_allowed_plugins: tuple[str, ...] = (), + claude_allowed_skills: tuple[str, ...] = (), ): if name == "codex": from .adapters.codex import CodexAdapter @@ -2308,6 +2341,9 @@ def _build_agent( # exclusions are a Claude-Code-only concern for now. guard_exclude_paths=guard_exclude_paths, reasoning_effort=reasoning_effort, + isolation=claude_isolation, + allowed_plugins=claude_allowed_plugins, + allowed_skills=claude_allowed_skills, ) if model is not None: kwargs["model"] = model diff --git a/src/lh_harness/config.py b/src/lh_harness/config.py index 659d1340..4c8c1a5c 100644 --- a/src/lh_harness/config.py +++ b/src/lh_harness/config.py @@ -38,6 +38,9 @@ "prompt_language", "claude_mcp_config", "codex_mcp_config", + "claude_isolation", + "claude_allowed_plugins", + "claude_allowed_skills", "mcp_add_dirs", "guard_exclude_paths", "max_rounds", @@ -85,6 +88,14 @@ # codex_mcp_config = "/path/to/mcp.toml" mcp_add_dirs = [] +# Start Claude Code agents without this account's own plugins, skills, hooks +# and user-level CLAUDE.md (`--bare`), so an operator's toolbox cannot leak +# into runs that never asked for it. The allow-lists re-admit exactly the +# named installed plugins / user skills; naming anything implies isolation. +# claude_isolation = true +# claude_allowed_plugins = ["playwright"] +# claude_allowed_skills = ["graphify"] + # Build/cache directories the auditor read-only guard should not snapshot, # e.g. ["target", "node_modules", "build", ".venv"]. Agents can still read # them. Exclusions must stay inside the workspace; ".git" and harness-owned @@ -207,6 +218,19 @@ def _flatten_run_table(run: dict[str, Any]) -> dict[str, Any]: defaults["dashboard"] = _boolean(run["dashboard"], "run.dashboard") if "dashboard_port" in run: defaults["dashboard_port"] = _port(run["dashboard_port"], "run.dashboard_port") + if "claude_isolation" in run: + defaults["claude_isolation"] = _boolean(run["claude_isolation"], "run.claude_isolation") + for key, dest in ( + ("claude_allowed_plugins", "claude_allowed_plugin"), + ("claude_allowed_skills", "claude_allowed_skill"), + ): + if key in run: + value = run[key] + if not isinstance(value, list) or not all( + isinstance(item, str) and item for item in value + ): + raise ProjectConfigError(f"run.{key} must be an array of non-empty strings") + defaults[dest] = list(value) if "mcp_add_dirs" in run: value = run["mcp_add_dirs"] if not isinstance(value, list) or not all(isinstance(item, str) and item for item in value): diff --git a/tests/test_claude_isolation.py b/tests/test_claude_isolation.py new file mode 100644 index 00000000..9e0f7744 --- /dev/null +++ b/tests/test_claude_isolation.py @@ -0,0 +1,159 @@ +"""Isolation keeps the operator's Claude toolbox out of harness agents. + +The default is untouched: without opting in, the CLI is launched exactly as +before and inherits whatever the account has installed. Opting in must start +from a clean slate and re-admit only what was named. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from lh_harness.adapters.claude_code import ClaudeCodeAdapter +from lh_harness.adapters.claude_isolation import ( + SKILLS_PLUGIN_NAME, + build_skills_plugin, + resolve_plugin_dirs, +) + + +def _claude_home(tmp_path: Path, *, plugins: dict[str, Path] = {}, skills: tuple[str, ...] = ()) -> Path: + home = tmp_path / "claude-home" + entries = {} + for qualified, install in plugins.items(): + (install / ".claude-plugin").mkdir(parents=True) + (install / ".claude-plugin" / "plugin.json").write_text( + json.dumps({"name": qualified.split("@", 1)[0]}), encoding="utf-8" + ) + entries[qualified] = [{"scope": "user", "installPath": str(install)}] + (home / "plugins").mkdir(parents=True) + (home / "plugins" / "installed_plugins.json").write_text( + json.dumps({"version": 2, "plugins": entries}), encoding="utf-8" + ) + for name in skills: + skill = home / "skills" / name + skill.mkdir(parents=True) + (skill / "SKILL.md").write_text(f"# {name}\n", encoding="utf-8") + (skill / "references").mkdir() + (skill / "references" / "notes.md").write_text("notes\n", encoding="utf-8") + return home + + +# --- default behaviour is unchanged ----------------------------------------- + + +def test_without_opt_in_no_isolation_flags_appear(tmp_path): + adapter = ClaudeCodeAdapter(workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p")) + assert "--setting-sources" not in adapter.argv + assert "--disable-slash-commands" not in adapter.argv + assert "--plugin-dir" not in adapter.argv + + +# --- bare isolation ---------------------------------------------------------- + + +def test_isolation_alone_is_bare_with_no_skills(tmp_path): + adapter = ClaudeCodeAdapter( + workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p"), isolation=True + ) + idx = adapter.argv.index("--setting-sources") + assert adapter.argv[idx + 1] == "project" + assert "--disable-slash-commands" in adapter.argv + assert "--plugin-dir" not in adapter.argv + + +def test_allow_lists_imply_isolation(tmp_path, monkeypatch): + home = _claude_home(tmp_path, skills=("graphify",)) + monkeypatch.setattr( + "lh_harness.adapters.claude_isolation.default_claude_home", lambda: home + ) + adapter = ClaudeCodeAdapter( + workspace_path=str(tmp_path), + prompt_dir=str(tmp_path / "run" / "prompts"), + allowed_skills=("graphify",), + ) + assert adapter.isolation is True + assert "--setting-sources" in adapter.argv + # Something was re-admitted, so skills must stay resolvable. + assert "--disable-slash-commands" not in adapter.argv + + +# --- plugin resolution ------------------------------------------------------- + + +def test_plugins_resolve_through_the_install_manifest(tmp_path): + install = tmp_path / "cache" / "playwright" / "1.0.0" + home = _claude_home(tmp_path, plugins={"playwright@official": install}) + dirs = resolve_plugin_dirs(["playwright"], claude_home=home) + assert dirs == [str(install)] + + +def test_unknown_plugin_is_a_startup_error(tmp_path): + home = _claude_home(tmp_path) + with pytest.raises(ValueError, match="not installed"): + resolve_plugin_dirs(["ghost"], claude_home=home) + + +def test_ambiguous_bare_name_requires_qualification(tmp_path): + home = _claude_home( + tmp_path, + plugins={ + "tool@market-a": tmp_path / "a" / "tool", + "tool@market-b": tmp_path / "b" / "tool", + }, + ) + with pytest.raises(ValueError, match="ambiguous"): + resolve_plugin_dirs(["tool"], claude_home=home) + assert resolve_plugin_dirs(["tool@market-a"], claude_home=home) == [ + str(tmp_path / "a" / "tool") + ] + + +# --- synthesised skills plugin ---------------------------------------------- + + +def test_allowed_skills_ride_in_a_synthesised_plugin(tmp_path): + home = _claude_home(tmp_path, skills=("graphify", "unrelated")) + target = tmp_path / "run" / "claude-allowed-skills" + plugin = build_skills_plugin(("graphify",), target, claude_home=home) + assert plugin == str(target) + manifest = json.loads((target / ".claude-plugin" / "plugin.json").read_text(encoding="utf-8")) + assert manifest["name"] == SKILLS_PLUGIN_NAME + # The named skill travels whole; the unnamed one does not travel at all. + assert (target / "skills" / "graphify" / "SKILL.md").is_file() + assert (target / "skills" / "graphify" / "references" / "notes.md").is_file() + assert not (target / "skills" / "unrelated").exists() + + +def test_missing_skill_is_a_startup_error(tmp_path): + home = _claude_home(tmp_path, skills=("graphify",)) + with pytest.raises(ValueError, match="not found"): + build_skills_plugin(("ghost",), tmp_path / "out", claude_home=home) + + +def test_skill_names_cannot_traverse_paths(tmp_path): + home = _claude_home(tmp_path, skills=("graphify",)) + with pytest.raises(ValueError, match="invalid skill name"): + build_skills_plugin(("../secrets",), tmp_path / "out", claude_home=home) + + +def test_adapter_passes_every_allowed_dir_to_the_cli(tmp_path, monkeypatch): + install = tmp_path / "cache" / "playwright" / "1.0.0" + home = _claude_home(tmp_path, plugins={"playwright@official": install}, skills=("graphify",)) + monkeypatch.setattr( + "lh_harness.adapters.claude_isolation.default_claude_home", lambda: home + ) + adapter = ClaudeCodeAdapter( + workspace_path=str(tmp_path), + prompt_dir=str(tmp_path / "run" / "prompts"), + allowed_plugins=("playwright",), + allowed_skills=("graphify",), + ) + plugin_dirs = [ + adapter.argv[i + 1] for i, part in enumerate(adapter.argv) if part == "--plugin-dir" + ] + assert str(install) in plugin_dirs + assert str(tmp_path / "run" / "claude-allowed-skills") in plugin_dirs From b545a6a849c46d08c50eba5ecc9d4c40fe487bd9 Mon Sep 17 00:00:00 2001 From: TON14 Date: Sun, 23 Aug 2026 10:41:29 +0300 Subject: [PATCH 18/19] Keep the CLI's built-in skills available under isolation Isolation without an allow-list also passed --disable-slash-commands, which disables *every* skill -- including the ones that ship inside the CLI itself (init, code-review, security-review, ...). That made the built-ins' availability depend on whether the allow-list happened to be empty: allow one user skill and they all came back. Incoherent, and not what the feature promises: it removes the operator account's layer, not the CLI's own capabilities. Project-only setting sources already keep user skills from resolving, so the flag added nothing to account isolation -- it only subtracted the built-ins. Gone; built-in skills now behave identically whether or not anything was re-admitted, exactly like the CLI's built-in tools. --- src/lh_harness/adapters/claude_code.py | 9 +++++---- tests/test_claude_isolation.py | 6 ++++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/lh_harness/adapters/claude_code.py b/src/lh_harness/adapters/claude_code.py index b0e65afe..55b7d927 100644 --- a/src/lh_harness/adapters/claude_code.py +++ b/src/lh_harness/adapters/claude_code.py @@ -121,10 +121,11 @@ def __init__( plugin_dirs.append(skills_plugin) for plugin_dir in plugin_dirs: argv.extend(["--plugin-dir", plugin_dir]) - if not plugin_dirs: - # Nothing was re-admitted, so no skill should resolve even if - # explicitly named in task text. - argv.append("--disable-slash-commands") + # Deliberately no --disable-slash-commands: isolation removes the + # *operator account's* layer, and project-only setting sources + # already keep user skills from resolving. The CLI's own built-in + # skills ship identically for everyone and stay available, exactly + # like its built-in tools. deny_tools = [*policy.disallowed_tools, *path_deny_rules(hidden_paths)] if deny_tools: argv.append("--disallowedTools") diff --git a/tests/test_claude_isolation.py b/tests/test_claude_isolation.py index 9e0f7744..498cf4cb 100644 --- a/tests/test_claude_isolation.py +++ b/tests/test_claude_isolation.py @@ -55,14 +55,16 @@ def test_without_opt_in_no_isolation_flags_appear(tmp_path): # --- bare isolation ---------------------------------------------------------- -def test_isolation_alone_is_bare_with_no_skills(tmp_path): +def test_isolation_alone_drops_the_account_layer_only(tmp_path): adapter = ClaudeCodeAdapter( workspace_path=str(tmp_path), prompt_dir=str(tmp_path / "p"), isolation=True ) idx = adapter.argv.index("--setting-sources") assert adapter.argv[idx + 1] == "project" - assert "--disable-slash-commands" in adapter.argv assert "--plugin-dir" not in adapter.argv + # The CLI's built-in skills are not the operator's toolbox; isolation must + # not disable them. + assert "--disable-slash-commands" not in adapter.argv def test_allow_lists_imply_isolation(tmp_path, monkeypatch): From f19ea5a5ffcd72b695cf1f7770f1fed4aaf66be9 Mon Sep 17 00:00:00 2001 From: TON14 Date: Sun, 23 Aug 2026 11:26:34 +0300 Subject: [PATCH 19/19] Point the empty-name refusal at the right spelling of "allow nothing" [""] is the natural but wrong way to write an empty allow-list -- it is an array holding one empty name, and the validator rejected the whole config with a message that never said what to write instead. The refusal now names the correct spellings (omit the key, or []), and the config template's examples use placeholder names instead of real ones from the author's machine, which read as if they were enabled. --- src/lh_harness/config.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/lh_harness/config.py b/src/lh_harness/config.py index 4c8c1a5c..52848efa 100644 --- a/src/lh_harness/config.py +++ b/src/lh_harness/config.py @@ -89,12 +89,14 @@ mcp_add_dirs = [] # Start Claude Code agents without this account's own plugins, skills, hooks -# and user-level CLAUDE.md (`--bare`), so an operator's toolbox cannot leak -# into runs that never asked for it. The allow-lists re-admit exactly the -# named installed plugins / user skills; naming anything implies isolation. +# and user-level CLAUDE.md, so an operator's toolbox cannot leak into runs +# that never asked for it. The CLI's built-in skills stay available. The +# allow-lists re-admit exactly the named installed plugins / user skills, and +# naming anything implies isolation. To allow nothing, leave the lists out +# entirely (or []): [""] is an empty *name* and is rejected at startup. # claude_isolation = true -# claude_allowed_plugins = ["playwright"] -# claude_allowed_skills = ["graphify"] +# claude_allowed_plugins = [""] +# claude_allowed_skills = [""] # Build/cache directories the auditor read-only guard should not snapshot, # e.g. ["target", "node_modules", "build", ".venv"]. Agents can still read @@ -229,7 +231,12 @@ def _flatten_run_table(run: dict[str, Any]) -> dict[str, Any]: if not isinstance(value, list) or not all( isinstance(item, str) and item for item in value ): - raise ProjectConfigError(f"run.{key} must be an array of non-empty strings") + # [""] is the natural but wrong spelling of "allow nothing", + # so the refusal must name the right one. + raise ProjectConfigError( + f"run.{key} must be an array of non-empty names; " + "to allow nothing, use [] or omit the key" + ) defaults[dest] = list(value) if "mcp_add_dirs" in run: value = run["mcp_add_dirs"]