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 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` 一次性检查，见[检查运行环境](#检查运行环境)。 diff --git a/src/lh_harness/adapters/claude_code.py b/src/lh_harness/adapters/claude_code.py index 8586b09c..55b7d927 100644 --- a/src/lh_harness/adapters/claude_code.py +++ b/src/lh_harness/adapters/claude_code.py @@ -1,11 +1,11 @@ from __future__ import annotations import os -import shlex from pathlib import Path 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, @@ -41,25 +41,31 @@ 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_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 +88,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", @@ -99,28 +104,54 @@ 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]) + # 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: - 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 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. 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_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/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/deepseek_runner.py b/src/lh_harness/adapters/deepseek_runner.py index 8c2f2ef0..575b9669 100644 --- a/src/lh_harness/adapters/deepseek_runner.py +++ b/src/lh_harness/adapters/deepseek_runner.py @@ -32,17 +32,53 @@ 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 = [ + "- id: agent-default-model", + " config:", + " provider: deepseek-official", + f" model: {json.dumps(model, ensure_ascii=False)}", + ] + 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:", + 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 +91,9 @@ def run(binary: str, prompt_path: Path, model: str) -> int: "headless", "--patch", os.fspath(patch_path), - 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/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/cli.py b/src/lh_harness/cli.py index d78f70c1..570c0181 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, @@ -229,6 +230,46 @@ 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 + # 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( 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 @@ -385,7 +429,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 @@ -525,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", @@ -1768,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] @@ -1875,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: @@ -2214,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 @@ -2247,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..52848efa 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,16 @@ # 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, 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 = [""] +# 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 # them. Exclusions must stay inside the workspace; ".git" and harness-owned @@ -207,6 +220,24 @@ 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 + ): + # [""] 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"] if not isinstance(value, list) or not all(isinstance(item, str) and item for item in value): 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/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..f1be4f8d 100644 --- a/src/lh_harness/environment/local.py +++ b/src/lh_harness/environment/local.py @@ -3,24 +3,29 @@ 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.child_env import apply_working_directory 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 @@ -43,6 +48,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. @@ -51,8 +85,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: @@ -94,11 +135,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 +206,26 @@ 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, + # `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, + 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 +239,7 @@ async def exec( io_task = asyncio.create_task( self._communicate_streaming( proc, + stdin, tee_path, stdout_chunks, stderr_chunks, @@ -170,16 +276,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 +303,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 +316,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 +390,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 +399,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/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..c7962daf 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,47 @@ 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) + # 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: + 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 +442,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 +505,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 +695,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 +933,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..10c3d7ea 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,20 @@ DEFAULT_MAX_ROUNDS, MAX_ROUNDS, ) +from ..utils.child_env import apply_working_directory +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" + +# 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 +174,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 +305,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 +401,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 +775,18 @@ def _supervisor_locked(self): """Serialize launch/resume idempotency transactions across workers.""" lock_path = self.runs_root / ".supervisor.lock" + if _IS_WINDOWS: + 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: + _release_lock_windows(windows_handle) + return handle = None flock = None raw_fd: int | None = None @@ -818,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; @@ -1008,7 +1155,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 +1167,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: @@ -1675,6 +1822,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, @@ -1682,8 +1834,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 +1877,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 +2109,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 +2194,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 +2251,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 +2287,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/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/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/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..0019ce90 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,74 @@ 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.""" +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 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.killpg(pid, sig) - except (ProcessLookupError, PermissionError, OSError): + 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. + + 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 +139,101 @@ 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] + + +# 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: + proc = _REAL_POPEN( + argv, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + stdin=subprocess.DEVNULL, + text=True, + 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 proc.returncode + return proc.returncode, (stdout or "") + (stderr or "") + + def _install_handlers() -> None: global _installed with _lock: @@ -87,7 +248,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 +256,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/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/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/supervisor/test_hardening.py b/tests/supervisor/test_hardening.py index 371dddc5..8b3c6678 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") @@ -369,9 +369,33 @@ 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.""" + 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 +423,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 +449,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 +457,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 +470,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 +478,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 +624,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 +637,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 +653,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 +670,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"]) @@ -770,6 +794,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) @@ -829,7 +876,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") diff --git a/tests/test_adapter_argv.py b/tests/test_adapter_argv.py new file mode 100644 index 00000000..2001041e --- /dev/null +++ b/tests/test_adapter_argv.py @@ -0,0 +1,230 @@ +"""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 os + +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) + # 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 + assert any(rule.startswith("Read(//") for rule in rules) + assert all(rule.startswith(("Read(", "Edit(")) for rule in rules) 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_claude_isolation.py b/tests/test_claude_isolation.py new file mode 100644 index 00000000..498cf4cb --- /dev/null +++ b/tests/test_claude_isolation.py @@ -0,0 +1,161 @@ +"""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_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 "--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): + 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 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..faad83de 100644 --- a/tests/test_deepseek_harness_adapter.py +++ b/tests/test_deepseek_harness_adapter.py @@ -2,9 +2,11 @@ import asyncio import json +import os import shlex from pathlib import Path +import pytest from fastapi.testclient import TestClient from lh_harness import agent_logs @@ -13,18 +15,13 @@ 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 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 +53,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 +65,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") @@ -77,16 +75,70 @@ 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_PLACEHOLDER) + 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 + + +@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" + 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 + + 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("fix the project via the positional route") + 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", - "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 +179,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 +209,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..720d70ad --- /dev/null +++ b/tests/test_environment_local.py @@ -0,0 +1,253 @@ +"""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 os +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_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')))" + 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")) 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 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_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\'\'') 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()