From 3d0e7e3376f15ff819645d3949f618f18f5e155e Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 9 Oct 2026 01:05:09 +0800 Subject: [PATCH 1/5] fix(agent): read the interactive PATH on Windows instead of a frozen login shell The WebUI reported an installed subagent "not on the login shell PATH" while its interpreter sat on disk. Two Windows-only defects stacked: shlex.split defaults to POSIX, so the agent's launch command "C:\...\python.exe -m raven acp" had every backslash read as an escape and reached shutil.which as the bare relative name "C:Users...python.exe", which can only miss a PATH lookup. On win32 the command is tokenized posix=False, the way CommandLineToArgvW parses a real spawn, so the absolute interpreter is handed to which whole. The git-bash .exe and backslash spellings of SHELL are also recognized as drivable. Windows has no login shell to run "bash -lic env -0" against, so login_shell_env fell back to the gateway's frozen os.environ and an agent installed after boot was invisible forever. _capture_windows now reads the HKLM and HKCU Environment stores a fresh terminal is assembled from, recursively expands %VAR% references, and unions the store PATH with the live process PATH (a service's own additions like MinGit's usr/bin survive). Everything else is kept from os.environ so a child loses no proxy, temp or conda variables. --- raven/agent/subagent/backends/env.py | 141 +++++++++++++++++++++++- raven/agent/subagent/probe.py | 27 ++++- tests/test_subagent_host_env.py | 158 +++++++++++++++++++++++++++ tests/test_subagent_probe.py | 46 ++++++++ tests/test_subagent_third_party.py | 4 + 5 files changed, 368 insertions(+), 8 deletions(-) diff --git a/raven/agent/subagent/backends/env.py b/raven/agent/subagent/backends/env.py index f38fb01ef..4cdfc8068 100644 --- a/raven/agent/subagent/backends/env.py +++ b/raven/agent/subagent/backends/env.py @@ -37,12 +37,22 @@ Known limitation: if the login shell's profile prints a banner to stdout, that banner corrupts the first parsed variable. This is not worked around here. + +Windows has no login shell to ask: explorer.exe holds the interactive +environment, and installers refresh a terminal opened afterwards by +broadcasting WM_SETTINGCHANGE. `_capture_windows` reads the persisted User +and Machine stores that broadcast refreshes, merging them over raven's +inherited environment so a child keeps the process environment a service +already had and only the installer-refreshed entries -- Path above all -- +are rebuilt from the store that is actually current. """ from __future__ import annotations import os +import re import subprocess +import sys from loguru import logger @@ -64,10 +74,22 @@ def _login_shell() -> str | None: - """The user's login shell if this module can drive it, else ``None``.""" - shell = os.environ.get("SHELL", "").strip() + r"""The user's login shell if this module can drive it, else ``None``. - return shell if shell and os.path.basename(shell) in _DRIVABLE_SHELLS else None + ``basename`` alone leaves a Windows-shaped bash unread: git-bash exports + ``SHELL=C:\Program Files\Git\usr\bin\bash.exe`` with backslashes, and + POSIX ``basename`` sees no separator. Both flavors of path are split so + that spelling drives the shell it names rather than falling back and + losing git-bash's profile-installed PATH entries on a POSIX host that is + not Windows. + """ + shell = os.environ.get("SHELL", "").strip() + if not shell: + return None + name = shell.replace("\\", "/").rsplit("/", 1)[-1] + if name.lower().endswith(".exe"): + name = name[:-4] + return shell if name in _DRIVABLE_SHELLS else None def login_shell_env() -> dict[str, str]: @@ -86,7 +108,11 @@ def login_shell_env() -> dict[str, str]: return dict(_LOGIN_ENV) if _LOGIN_ENV_FAILED: return dict(os.environ) - captured = _capture(consequence="subagents inherit raven's environment") + captured = ( + _capture_windows(consequence="subagents inherit raven's environment") + if sys.platform == "win32" + else _capture(consequence="subagents inherit raven's environment") + ) if captured is None: _LOGIN_ENV_FAILED = True return dict(os.environ) @@ -121,6 +147,113 @@ def refresh_login_shell_env() -> bool: return True +def _capture_windows(*, consequence: str) -> dict[str, str] | None: + """The environment a freshly opened Windows terminal would start with. + + Windows has no login shell to ask: explorer.exe holds the interactive + environment, and installers refresh a terminal opened afterwards by + broadcasting WM_SETTINGCHANGE (measured 2026-10-08: a `uv tool install` + made an agent resolvable from every new terminal minutes before the + running gateway, a service with a frozen `os.environ`, could see it -- + and the WebUI probe answered "not on the login shell PATH" while the + file sat on disk). + + Rather than round-trip a PowerShell (which needs quoting, base64, and + a WM broadcast), read the persisted stores the shell itself reassembles + a terminal from: HKCU\\Environment for the user's own variables and + HKLM\\...\\Environment for the machine's. Both are merged over raven's + inherited ``os.environ`` so a child keeps the process environment a + service already had (proxy, temp, SystemRoot, conda) and only the + installer-refreshed entries -- ``Path`` above all -- are rebuilt from + the store that is actually current. + + ``None`` when neither store can be read, which is the "capture failed" + signal the caller falls back on. A store that reads but has no ``Path`` + is still a valid capture: the probe of an absent agent then names the + registry it was looked up in, which is the same sentence the POSIX arm + gives for a profile with no PATH export. + """ + import winreg + + def _read_store(hive: int, subkey: str) -> dict[str, str]: + try: + with winreg.OpenKey(hive, subkey) as key: + entries = {} + for i in range(winreg.QueryInfoKey(key)[1]): + name, value, _vtype = winreg.EnumValue(key, i) + if name and isinstance(value, str): + entries[name] = value + return entries + except OSError as exc: + logger.warning( + "Windows login environment capture could not read {} ({}); {}", + subkey, + exc, + consequence, + ) + return {} + + machine = _read_store(winreg.HKEY_LOCAL_MACHINE, r"SYSTEM\CurrentControlSet\Control\Session Manager\Environment") + user = _read_store(winreg.HKEY_CURRENT_USER, r"Environment") + if not machine and not user: + return None + + machine_path = next((v for k, v in machine.items() if k.lower() == "path"), "") + user_path = next((v for k, v in user.items() if k.lower() == "path"), "") + # Expand %VAR% against the same machine-then-user precedence Windows + # applies when it assembles a terminal's environment -- registry Path + # entries hold unexpanded references, and passing them on raw hands the + # child a PATH whose entries never existed (the gateway's own measured + # log shows a launcher script dying on `%USERPROFILE%/go/bin`). + context: dict[str, str] = {} + for source in (os.environ, machine, user): + context.update({k.lower(): v for k, v in source.items()}) + context.pop("path", None) + machine_path = _expand_windows(machine_path, context) + user_path = _expand_windows(user_path, context) + + # Windows environment names are case-insensitive, so the merge keys on the + # fold: os.environ < machine < user, with one entry per name -- a child's + # env block carrying both `Path` and `PATH` resolves unpredictably. + merged: dict[str, str] = {} + known: dict[str, str] = {} + for source in (os.environ, machine, user): + for name, value in source.items(): + low = name.lower() + if low in known: + merged.pop(known[low], None) + known[low] = name + merged[name] = value + if machine_path or user_path: + merged.pop(known.get("path", ""), None) + # Union rather than replace: the machine+user stores are the PATH a + # *new* terminal gets, but raven's own launcher's bin (MinGit/usr/bin, + # conda) reached this process's PATH without touching the stores, and + # a child spawned from the gateway needs every one of them. + live = os.environ.get("PATH", "") + joined = [p for p in (machine_path, user_path) if p] + if live: + joined.append(live) + merged["Path"] = ";".join(joined) + return merged + + +def _expand_windows(value: str, context: dict[str, str], _depth: int = 0) -> str: + r"""``%VAR%`` references in a registry value, folded the way Windows folds them. + + The registry is what a terminal's Path is built from, so the references + resolve against the case-folded registry context (machine-then-user + shadowing the process's inherited env) rather than ``os.environ`` -- + a literal ``%USERPROFILE%\go\bin`` would otherwise reach a child as a + directory named exactly that. + """ + if not value or "%" not in value or _depth > 16: + return value + + expanded = re.sub(r"%([^%]+)%", lambda m: context.get(m.group(1).lower(), m.group(0)), value) + return value if expanded == value else _expand_windows(expanded, context, _depth + 1) + + def _capture(*, consequence: str) -> dict[str, str] | None: """One run of the login shell, parsed; ``None`` when it did not work, after saying why. diff --git a/raven/agent/subagent/probe.py b/raven/agent/subagent/probe.py index 0c03057f9..26f9071f3 100644 --- a/raven/agent/subagent/probe.py +++ b/raven/agent/subagent/probe.py @@ -157,6 +157,25 @@ def to_wire(self) -> dict[str, Any]: } +def _split_command(command: str) -> list[str]: + r"""``shlex.split`` resolved the way the OS that will run it resolves. + + POSIX is the default because every real spawn goes through it that way. + Windows PowerShell answers CommandLineToArgvW instead: no `\U`-strip of a + path, and the difference decides whether `C:\...\python.exe` reads as the + executable it names or as `C:Users...python.exe`. Only argv[0] is asked for + everywhere this helper runs, and Windows quotes drive alone anyway, so + ``posix=False`` answering with the quotes still on is fine. + """ + return shlex.split(command, posix=not _is_windows()) + + +def _is_windows() -> bool: + import sys + + return sys.platform == "win32" + + def _login_path() -> str: return login_shell_env().get("PATH", "") @@ -515,7 +534,7 @@ def _stale_node(cfg: Any) -> NodeTooOld | None: if not runs_on_node(cfg): return None try: - argv = shlex.split((getattr(cfg, "command", None) or "").strip()) + argv = _split_command((getattr(cfg, "command", None) or "").strip()) except ValueError: return None if not argv: @@ -537,7 +556,7 @@ def done(status: ProbeStatus, detail: str, target: str = "") -> ProbeResult: if not command: return done("unknown", "command is empty") try: - argv = shlex.split(command) + argv = _split_command(command) except ValueError as exc: return done("unknown", f"command cannot be parsed: {exc}") if not argv: @@ -583,7 +602,7 @@ def done(status: ProbeStatus, detail: str, target: str = "") -> ProbeResult: if not command: return done("unknown", "command is empty") try: - argv = shlex.split(command) + argv = _split_command(command) except ValueError as exc: return done("unknown", f"command cannot be parsed: {exc}") if not argv: @@ -1272,7 +1291,7 @@ def _unconfigured_acp_preset_rows(configured: set[str], *, path: str | None) -> here = [] for preset in wanted: try: - argv = shlex.split((preset.get("command") or "").strip()) + argv = _split_command((preset.get("command") or "").strip()) except ValueError: continue if argv and shutil.which(argv[0], path=path or None) is not None: diff --git a/tests/test_subagent_host_env.py b/tests/test_subagent_host_env.py index 1eccc219b..f2aefdde2 100644 --- a/tests/test_subagent_host_env.py +++ b/tests/test_subagent_host_env.py @@ -13,9 +13,11 @@ import shlex import sys from pathlib import Path +from typing import Any import pytest +import raven.agent.subagent.backends.env as backend_env_mod from raven.acp_client.client import AcpClient from raven.agent.subagent.backends import env as backend_env from raven.agent.subagent.role import SUBAGENT_ENV_VAR, is_subagent_process @@ -92,3 +94,159 @@ async def test_acp_child_is_told_it_serves_as_a_subagent(tmp_path: Path, monkeyp # the written form alone would pass while the child ignored it. monkeypatch.setenv(SUBAGENT_ENV_VAR, arrived) assert is_subagent_process(), f"the child was handed {arrived!r}, which it does not read as a sub-agent role" + + +# --- Windows login environment capture ----------------------------------- +# +# Windows has no login shell. The capture reads the two registry stores a +# terminal is assembled from (machine, then user), merged over raven's own +# inherited environment, with Path concatenated the way Windows builds it. + + +class _FakeKey: + def __init__(self, values: dict[str, str], *, fail: bool = False) -> None: + self.values = values + self.fail = fail + + def __enter__(self) -> "_FakeKey": + if self.fail: + raise OSError("access denied") + return self + + def __exit__(self, *_: Any) -> None: + return None + + +class _FakeWinreg: + """``import winreg`` is Windows-only, so the registry is faked here exactly. + + Only the three calls the capture makes are honest: ``OpenKey`` hands a key + over a store a test seeded, and the enumeration returns that store. The + dict keeps insertion order, so user-after-machine remains visible. + """ + + HKEY_LOCAL_MACHINE = 0x80000002 + HKEY_CURRENT_USER = 0x80000001 + REG_EXPAND_SZ = 2 + + def __init__(self, machine: dict[str, str] | bool, user: dict[str, str] | bool) -> None: + self._stores = {self.HKEY_LOCAL_MACHINE: machine, self.HKEY_CURRENT_USER: user} + + def OpenKey(self, hive: int, _subkey: str) -> _FakeKey: + store = self._stores[hive] + return _FakeKey({}, fail=True) if store is False else _FakeKey(dict(store)) + + def QueryInfoKey(self, key: _FakeKey) -> tuple[int, int, int]: + return (0, len(key.values), 0) + + def EnumValue(self, key: _FakeKey, index: int) -> tuple[str, str, int]: + name = list(key.values)[index] + return (name, key.values[name], 1) + + +def _fake_winreg( + monkeypatch: pytest.MonkeyPatch, + machine: dict[str, str] | bool, + user: dict[str, str] | bool, +) -> None: + monkeypatch.setattr(sys, "winreg", _FakeWinreg(machine, user), raising=False) + monkeypatch.setitem(sys.modules, "winreg", _FakeWinreg(machine, user)) + + +def test_windows_capture_refreshes_a_frozen_process_path(monkeypatch: pytest.MonkeyPatch) -> None: + """The reported failure: an installer ran after the gateway started. + + A service keeps the PATH it started with. A terminal opened after + `uv tool install` reads the store the installer wrote, and so does this + capture: the merged answer carries every inherited variable and the + store's Path -- not the process's stale one. + """ + monkeypatch.setenv("PATH", r"C:\stale\service-bin") + monkeypatch.setenv("HTTP_PROXY", "http://proxy:8888") + _fake_winreg( + monkeypatch, + machine={"Path": r"C:\Windows\system32", "ComSpec": r"C:\Windows\system32\cmd.exe"}, + user={"Path": r"C:\Users\me\AppData\Roaming\uv\tools\raven\Scripts"}, + ) + captured = backend_env_mod._capture_windows(consequence="subagents inherit raven's environment") + assert captured is not None + assert captured["Path"] == ( + r"C:\Windows\system32;C:\Users\me\AppData\Roaming\uv\tools\raven\Scripts;C:\stale\service-bin" + ) + assert captured["HTTP_PROXY"] == "http://proxy:8888", "a child keeps the process vars the store knows nothing of" + assert captured["ComSpec"] == r"C:\Windows\system32\cmd.exe" + assert sum(1 for k in captured if k.lower() == "path") == 1, "no duplicate Path spellings beside Path" + + +def test_windows_capture_environments_names_fold_case_insensitively(monkeypatch: pytest.MonkeyPatch) -> None: + """One name, one entry: `TEMP` from the process and `Temp` from the store are the same variable. + + A child env block carrying both spellings resolves to an arbitrary one on + Windows -- and to the later-written one, which the store's refresh would + then silently contradict. + """ + monkeypatch.setenv("TEMP", r"C:\stale-tmp") + _fake_winreg(monkeypatch, machine=False, user={"Temp": r"C:\Users\me\AppData\Local\Temp"}) + captured = backend_env_mod._capture_windows(consequence="...") + assert captured is not None + assert captured.get("Temp") == r"C:\Users\me\AppData\Local\Temp" + assert "TEMP" not in captured + + +def test_windows_capture_keeps_up_when_only_one_store_reads(monkeypatch: pytest.MonkeyPatch) -> None: + """HKLM can refuse a non-elevated read; the user's entries must not be lost with it.""" + monkeypatch.setenv("PATH", r"C:\legacy") + _fake_winreg(monkeypatch, machine=False, user={"Path": r"C:\user-only"}) + captured = backend_env_mod._capture_windows(consequence="...") + assert captured is not None + assert captured["Path"] == r"C:\user-only;C:\legacy" + + +def test_windows_capture_reports_failure_when_no_store_reads(monkeypatch: pytest.MonkeyPatch) -> None: + """Both stores refusing is the one case the driver must fall back on ``os.environ``.""" + _fake_winreg(monkeypatch, machine=False, user=False) + assert backend_env_mod._capture_windows(consequence="subagents inherit raven's environment") is None + + +def test_windows_capture_expands_percent_vars_in_the_stored_path(monkeypatch: pytest.MonkeyPatch) -> None: + r"""`%USERPROFILE%\go\bin` in the registry is a path; left raw it is not one. + + The gateway's measured log shows a launcher script dying on exactly that: + the store writes the reference, the read-before mine passed it on, and a + terminal got the expanded form. Machine-level references resolve against + machine values first (USERPROFILE is the service account's there), then + against the process's inherited env -- SHELL-adjusted var precedence. + """ + monkeypatch.setenv("USERPROFILE", r"C:\Users\me") + monkeypatch.setenv("PATH", r"C:\legacy") + _fake_winreg( + monkeypatch, + machine={"Path": r"C:\tools;%ProgramFiles%\App", "ProgramFiles": r"C:\Program Files"}, + user={"Path": r"%USERPROFILE%\AppData\Roaming\uv\tools\raven\Scripts"}, + ) + captured = backend_env_mod._capture_windows(consequence="...") + assert captured is not None + assert captured["Path"] == r"C:\tools;C:\Program Files\App;C:\Users\me\AppData\Roaming\uv\tools\raven\Scripts;C:\legacy" + + +def test_windows_capture_runs_in_place_of_the_posix_one_on_win32(monkeypatch: pytest.MonkeyPatch) -> None: + """The platform decides the capture; on win32 the registry-built path beats raven's frozen one.""" + monkeypatch.setenv("PATH", r"C:\service-legacy-bin") + _fake_winreg(monkeypatch, machine={"Path": r"C:\system32"}, user={"Path": r"C:\user"}) + monkeypatch.setattr(backend_env_mod.sys, "platform", "win32") + monkeypatch.setattr(backend_env_mod, "_LOGIN_ENV", None) + monkeypatch.setattr(backend_env_mod, "_LOGIN_ENV_FAILED", False) + env = backend_env_mod.login_shell_env() + assert env["Path"] == r"C:\system32;C:\user;C:\service-legacy-bin" + + +def test_login_shell_reads_a_windows_shaped_bash_path(monkeypatch: pytest.MonkeyPatch) -> None: + """basename alone dropped it: git-bash's `SHELL` has backslashes, and POSIX split sees no separator. + + The gateway's log had SHELL=`C:\\Program Files\\Git\\usr\\bin\\bash.exe` be "not one this + build can drive" and every probe ran on raven's own PATH -- invisible to the + README's own install-from-git-bash recipe, which then answered "agent missing". + """ + shell = r"C:\Program Files\Git\usr\bin\bash.exe" + monkeypatch.setenv("SHELL", shell) + assert backend_env_mod._login_shell() == shell diff --git a/tests/test_subagent_probe.py b/tests/test_subagent_probe.py index ab89d435c..0fff0ee35 100644 --- a/tests/test_subagent_probe.py +++ b/tests/test_subagent_probe.py @@ -55,6 +55,52 @@ def _openai(base_url: str, model: str = "m1", api_key: str = "k") -> ThirdPartyO # --- cli probe ----------------------------------------------------------- +def test_windows_argv_keeps_backslash_paths(monkeypatch: pytest.MonkeyPatch) -> None: + """The resolver that parses a row's command answers the argv Windows gives it. + + ``shlex.split`` defaults to POSIX: a backslash before a letter is an escape, + so every Windows absolute path resolved by ``{PYTHON}`` -- its only spelling + -- read as `C:Users...` to the probe and spawned that way too, whatever + PowerShell itself would have answered. ``posix=False`` parses a command + CommandLineToArgvW's way: backslashes are literal, and argv[0] is what + `PsiElement` starts. + + The check is argv[0] only. Arguments are never asked of this resolver -- + they reach the process raven spawns, whose own argv is the launch's, not + this file's -- and a probe of a Windows path from a POSIX host has to + answer the Windows answer. + """ + monkeypatch.setattr(probe_mod, "_is_windows", lambda: True) + assert probe_mod._split_command(r"C:\Users\me\AppData\Roaming\uv\tools\raven\Scripts\python.exe -m raven acp") == [ + r"C:\Users\me\AppData\Roaming\uv\tools\raven\Scripts\python.exe", + "-m", + "raven", + "acp", + ] + + +async def test_windows_command_reaches_which_intact(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + r"""A Windows absolute path reaches shutil.which with its backslashes, or it resolves to nothing. + + ``{PYTHON}`` resolves to ``C:\...\python.exe``. shlex.split in POSIX mode + reads every ``\X`` as an escape and drops it, so the probe asked which for + ``C:Users...python.exe`` -- a bare name, looked up on PATH, reported + missing while the file sat on disk. CommandLineToArgvW, which is what a + Windows spawn actually parses with, keeps backslashes literal. + """ + command = r"C:\venv\Scripts\python.exe -m raven acp" + asked: list[str] = [] + monkeypatch.setattr(probe_mod.shutil, "which", lambda exe, path=None: asked.append(exe) or None) + cfg = ThirdPartyAcpSubagentConfig(name="Agent", command=command) + + monkeypatch.setattr(probe_mod, "_is_windows", lambda: False) + await probe_one(cfg, source="config", path=str(tmp_path)) + monkeypatch.setattr(probe_mod, "_is_windows", lambda: True) + await probe_one(cfg, source="config", path=str(tmp_path)) + + assert asked == ["C:venvScriptspython.exe", r"C:\venv\Scripts\python.exe"] + + async def test_cli_probe_reports_the_resolved_absolute_path(tmp_path: Path) -> None: exe = tmp_path / "faux-agent" exe.write_text("#!/bin/sh\nexit 0\n") diff --git a/tests/test_subagent_third_party.py b/tests/test_subagent_third_party.py index c112cd902..6f8a29041 100644 --- a/tests/test_subagent_third_party.py +++ b/tests/test_subagent_third_party.py @@ -82,6 +82,10 @@ def _clear_login_env_cache(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(env_mod, "_LOGIN_ENV", None) monkeypatch.setattr(env_mod, "_LOGIN_ENV_FAILED", False) monkeypatch.setenv("SHELL", "/bin/bash") + # These tests capture from a POSIX login shell. On a Windows runner the + # platform would route them to `_capture_windows` (PowerShell) and their + # patched `bash -lic ...` would never be invoked, so pin the platform too. + monkeypatch.setattr(env_mod.sys, "platform", "linux") def test_login_shell_env_parses_nul_separated_output( From bbd4e36a0885687c709d7729ab9a665566fd3d80 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 9 Oct 2026 03:45:59 +0000 Subject: [PATCH 2/5] fix(agent): split the probe's command the way the launchers do The probe alone split a command with posix=False on Windows, while AcpClient.launch and the cli backend kept POSIX shlex.split. The two then disagreed in both directions: the probe found the interpreter of an unquoted {PYTHON} command that the launch still mangled into C:Users...python.exe, and it kept the quotes shlex.quote puts round host_raven_acp_command's path, so it reported missing a program the launch starts. One parser for the probe and both launchers is what #864 adds, so the probe goes back to the split the launchers use rather than gaining a second Windows rule here. Co-authored-by: Claude (claude-opus-5-5[1m]) --- raven/agent/subagent/probe.py | 27 +++----------------- tests/test_subagent_probe.py | 46 ----------------------------------- 2 files changed, 4 insertions(+), 69 deletions(-) diff --git a/raven/agent/subagent/probe.py b/raven/agent/subagent/probe.py index 26f9071f3..0c03057f9 100644 --- a/raven/agent/subagent/probe.py +++ b/raven/agent/subagent/probe.py @@ -157,25 +157,6 @@ def to_wire(self) -> dict[str, Any]: } -def _split_command(command: str) -> list[str]: - r"""``shlex.split`` resolved the way the OS that will run it resolves. - - POSIX is the default because every real spawn goes through it that way. - Windows PowerShell answers CommandLineToArgvW instead: no `\U`-strip of a - path, and the difference decides whether `C:\...\python.exe` reads as the - executable it names or as `C:Users...python.exe`. Only argv[0] is asked for - everywhere this helper runs, and Windows quotes drive alone anyway, so - ``posix=False`` answering with the quotes still on is fine. - """ - return shlex.split(command, posix=not _is_windows()) - - -def _is_windows() -> bool: - import sys - - return sys.platform == "win32" - - def _login_path() -> str: return login_shell_env().get("PATH", "") @@ -534,7 +515,7 @@ def _stale_node(cfg: Any) -> NodeTooOld | None: if not runs_on_node(cfg): return None try: - argv = _split_command((getattr(cfg, "command", None) or "").strip()) + argv = shlex.split((getattr(cfg, "command", None) or "").strip()) except ValueError: return None if not argv: @@ -556,7 +537,7 @@ def done(status: ProbeStatus, detail: str, target: str = "") -> ProbeResult: if not command: return done("unknown", "command is empty") try: - argv = _split_command(command) + argv = shlex.split(command) except ValueError as exc: return done("unknown", f"command cannot be parsed: {exc}") if not argv: @@ -602,7 +583,7 @@ def done(status: ProbeStatus, detail: str, target: str = "") -> ProbeResult: if not command: return done("unknown", "command is empty") try: - argv = _split_command(command) + argv = shlex.split(command) except ValueError as exc: return done("unknown", f"command cannot be parsed: {exc}") if not argv: @@ -1291,7 +1272,7 @@ def _unconfigured_acp_preset_rows(configured: set[str], *, path: str | None) -> here = [] for preset in wanted: try: - argv = _split_command((preset.get("command") or "").strip()) + argv = shlex.split((preset.get("command") or "").strip()) except ValueError: continue if argv and shutil.which(argv[0], path=path or None) is not None: diff --git a/tests/test_subagent_probe.py b/tests/test_subagent_probe.py index 0fff0ee35..ab89d435c 100644 --- a/tests/test_subagent_probe.py +++ b/tests/test_subagent_probe.py @@ -55,52 +55,6 @@ def _openai(base_url: str, model: str = "m1", api_key: str = "k") -> ThirdPartyO # --- cli probe ----------------------------------------------------------- -def test_windows_argv_keeps_backslash_paths(monkeypatch: pytest.MonkeyPatch) -> None: - """The resolver that parses a row's command answers the argv Windows gives it. - - ``shlex.split`` defaults to POSIX: a backslash before a letter is an escape, - so every Windows absolute path resolved by ``{PYTHON}`` -- its only spelling - -- read as `C:Users...` to the probe and spawned that way too, whatever - PowerShell itself would have answered. ``posix=False`` parses a command - CommandLineToArgvW's way: backslashes are literal, and argv[0] is what - `PsiElement` starts. - - The check is argv[0] only. Arguments are never asked of this resolver -- - they reach the process raven spawns, whose own argv is the launch's, not - this file's -- and a probe of a Windows path from a POSIX host has to - answer the Windows answer. - """ - monkeypatch.setattr(probe_mod, "_is_windows", lambda: True) - assert probe_mod._split_command(r"C:\Users\me\AppData\Roaming\uv\tools\raven\Scripts\python.exe -m raven acp") == [ - r"C:\Users\me\AppData\Roaming\uv\tools\raven\Scripts\python.exe", - "-m", - "raven", - "acp", - ] - - -async def test_windows_command_reaches_which_intact(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - r"""A Windows absolute path reaches shutil.which with its backslashes, or it resolves to nothing. - - ``{PYTHON}`` resolves to ``C:\...\python.exe``. shlex.split in POSIX mode - reads every ``\X`` as an escape and drops it, so the probe asked which for - ``C:Users...python.exe`` -- a bare name, looked up on PATH, reported - missing while the file sat on disk. CommandLineToArgvW, which is what a - Windows spawn actually parses with, keeps backslashes literal. - """ - command = r"C:\venv\Scripts\python.exe -m raven acp" - asked: list[str] = [] - monkeypatch.setattr(probe_mod.shutil, "which", lambda exe, path=None: asked.append(exe) or None) - cfg = ThirdPartyAcpSubagentConfig(name="Agent", command=command) - - monkeypatch.setattr(probe_mod, "_is_windows", lambda: False) - await probe_one(cfg, source="config", path=str(tmp_path)) - monkeypatch.setattr(probe_mod, "_is_windows", lambda: True) - await probe_one(cfg, source="config", path=str(tmp_path)) - - assert asked == ["C:venvScriptspython.exe", r"C:\venv\Scripts\python.exe"] - - async def test_cli_probe_reports_the_resolved_absolute_path(tmp_path: Path) -> None: exe = tmp_path / "faux-agent" exe.write_text("#!/bin/sh\nexit 0\n") From cdf52bee752b27064a8f0a8b479731f3a7416609 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 9 Oct 2026 03:46:43 +0000 Subject: [PATCH 3/5] fix(agent): keep raven's environment and rebuild only PATH on Windows The Windows capture stored its PATH under the registry's spelling, Path, while every reader asks for PATH: CPython keeps os.environ names upper-case on Windows, which is what the fallback this replaced had handed them. The probe read an empty PATH, shutil.which(exe, path="") answers None, and every bare-name agent read as missing, including the ones found on the gateway's PATH before the capture existed. It also copied every other stored value over raven's own. Those are what Windows builds an environment from, not what it hands a process: ComSpec and TEMP arrive unexpanded (%SystemRoot%\system32\cmd.exe, %USERPROFILE%\...), and the machine store's USERNAME is SYSTEM until a logon replaces it. A child given that ComSpec cannot run Popen(shell=True), which uses it only when it is an absolute path. The capture is now dict(os.environ) with PATH rebuilt from the stored machine and user Path followed by the live PATH. A stored reference expands with raven's own values first, since they are this logon's and the child carries them, then user over machine for a variable an installer added after raven started. The tests take Windows' shape: the stock store values unexpanded, an upper-case os.environ, and the ambient environment cleared, so a host that holds HTTP_PROXY beside http_proxy no longer fails them. The capture is checked through probe_all, its reader. Co-authored-by: Claude (claude-opus-5-5[1m]) --- raven/agent/subagent/backends/env.py | 100 +++++------ tests/test_subagent_host_env.py | 245 ++++++++++++++++----------- 2 files changed, 188 insertions(+), 157 deletions(-) diff --git a/raven/agent/subagent/backends/env.py b/raven/agent/subagent/backends/env.py index 4cdfc8068..27e225145 100644 --- a/raven/agent/subagent/backends/env.py +++ b/raven/agent/subagent/backends/env.py @@ -40,11 +40,9 @@ Windows has no login shell to ask: explorer.exe holds the interactive environment, and installers refresh a terminal opened afterwards by -broadcasting WM_SETTINGCHANGE. `_capture_windows` reads the persisted User -and Machine stores that broadcast refreshes, merging them over raven's -inherited environment so a child keeps the process environment a service -already had and only the installer-refreshed entries -- Path above all -- -are rebuilt from the store that is actually current. +broadcasting WM_SETTINGCHANGE. `_capture_windows` rebuilds PATH from the +persisted User and Machine stores that broadcast refreshes and keeps every +other value of raven's own environment. """ from __future__ import annotations @@ -73,6 +71,11 @@ _DRIVABLE_SHELLS = frozenset({"bash", "zsh"}) +def _on_windows() -> bool: + """Whether this is Windows, which decides how the environment below is captured.""" + return sys.platform == "win32" + + def _login_shell() -> str | None: r"""The user's login shell if this module can drive it, else ``None``. @@ -110,7 +113,7 @@ def login_shell_env() -> dict[str, str]: return dict(os.environ) captured = ( _capture_windows(consequence="subagents inherit raven's environment") - if sys.platform == "win32" + if _on_windows() else _capture(consequence="subagents inherit raven's environment") ) if captured is None: @@ -148,7 +151,7 @@ def refresh_login_shell_env() -> bool: def _capture_windows(*, consequence: str) -> dict[str, str] | None: - """The environment a freshly opened Windows terminal would start with. + r"""Raven's own environment, with the PATH a freshly opened Windows terminal would start with. Windows has no login shell to ask: explorer.exe holds the interactive environment, and installers refresh a terminal opened afterwards by @@ -160,18 +163,19 @@ def _capture_windows(*, consequence: str) -> dict[str, str] | None: Rather than round-trip a PowerShell (which needs quoting, base64, and a WM broadcast), read the persisted stores the shell itself reassembles - a terminal from: HKCU\\Environment for the user's own variables and - HKLM\\...\\Environment for the machine's. Both are merged over raven's - inherited ``os.environ`` so a child keeps the process environment a - service already had (proxy, temp, SystemRoot, conda) and only the - installer-refreshed entries -- ``Path`` above all -- are rebuilt from - the store that is actually current. + a terminal from: HKCU\Environment for the user's own variables and + HKLM\...\Environment for the machine's. + + Only PATH is taken from them. The rest of a store is what Windows builds an + environment *from*, not what it hands a process: ``ComSpec`` and ``TEMP`` + sit there unexpanded, and the machine's ``USERNAME`` is ``SYSTEM`` until a + logon replaces it. Raven's own values are those, already resolved, so the + child keeps them -- proxy, temp, SystemRoot and conda included -- under the + upper-case names ``os.environ`` gives them on Windows, which is the spelling + every reader of this capture asks for. ``None`` when neither store can be read, which is the "capture failed" - signal the caller falls back on. A store that reads but has no ``Path`` - is still a valid capture: the probe of an absent agent then names the - registry it was looked up in, which is the same sentence the POSIX arm - gives for a profile with no PATH export. + signal the caller falls back on. """ import winreg @@ -182,7 +186,7 @@ def _read_store(hive: int, subkey: str) -> dict[str, str]: for i in range(winreg.QueryInfoKey(key)[1]): name, value, _vtype = winreg.EnumValue(key, i) if name and isinstance(value, str): - entries[name] = value + entries[name.upper()] = value return entries except OSError as exc: logger.warning( @@ -198,59 +202,37 @@ def _read_store(hive: int, subkey: str) -> dict[str, str]: if not machine and not user: return None - machine_path = next((v for k, v in machine.items() if k.lower() == "path"), "") - user_path = next((v for k, v in user.items() if k.lower() == "path"), "") - # Expand %VAR% against the same machine-then-user precedence Windows - # applies when it assembles a terminal's environment -- registry Path - # entries hold unexpanded references, and passing them on raw hands the - # child a PATH whose entries never existed (the gateway's own measured - # log shows a launcher script dying on `%USERPROFILE%/go/bin`). - context: dict[str, str] = {} - for source in (os.environ, machine, user): - context.update({k.lower(): v for k, v in source.items()}) - context.pop("path", None) - machine_path = _expand_windows(machine_path, context) - user_path = _expand_windows(user_path, context) - - # Windows environment names are case-insensitive, so the merge keys on the - # fold: os.environ < machine < user, with one entry per name -- a child's - # env block carrying both `Path` and `PATH` resolves unpredictably. - merged: dict[str, str] = {} - known: dict[str, str] = {} - for source in (os.environ, machine, user): - for name, value in source.items(): - low = name.lower() - if low in known: - merged.pop(known[low], None) - known[low] = name - merged[name] = value - if machine_path or user_path: - merged.pop(known.get("path", ""), None) + captured = dict(os.environ) + # Raven's own values outrank the stores' in a stored Path's references: they + # are what this logon resolved, and the child carries them beside the result. + # A variable an installer added after raven started is only in the stores, + # where the user's shadows the machine's, the order Windows layers them in. + context = {**machine, **user, **{name.upper(): value for name, value in captured.items()}} + context.pop("PATH", None) + stored = [path for entries in (machine, user) if (path := _expand_windows(entries.get("PATH", ""), context))] + if stored: # Union rather than replace: the machine+user stores are the PATH a # *new* terminal gets, but raven's own launcher's bin (MinGit/usr/bin, # conda) reached this process's PATH without touching the stores, and # a child spawned from the gateway needs every one of them. - live = os.environ.get("PATH", "") - joined = [p for p in (machine_path, user_path) if p] - if live: - joined.append(live) - merged["Path"] = ";".join(joined) - return merged + live = captured.get("PATH", "") + captured["PATH"] = ";".join([*stored, live] if live else stored) + return captured def _expand_windows(value: str, context: dict[str, str], _depth: int = 0) -> str: - r"""``%VAR%`` references in a registry value, folded the way Windows folds them. + r"""``%VAR%`` references in a stored value, resolved against ``context`` whatever their case. - The registry is what a terminal's Path is built from, so the references - resolve against the case-folded registry context (machine-then-user - shadowing the process's inherited env) rather than ``os.environ`` -- - a literal ``%USERPROFILE%\go\bin`` would otherwise reach a child as a - directory named exactly that. + Stored Path entries hold unexpanded references, and passed on raw they hand + the child a PATH whose entries never existed (the gateway's own measured log + shows a launcher script dying on `%USERPROFILE%/go/bin`). A reference whose + value holds another is resolved again; one with no value stays as written, + which is what Windows does with it. """ if not value or "%" not in value or _depth > 16: return value - expanded = re.sub(r"%([^%]+)%", lambda m: context.get(m.group(1).lower(), m.group(0)), value) + expanded = re.sub(r"%([^%]+)%", lambda m: context.get(m.group(1).upper(), m.group(0)), value) return value if expanded == value else _expand_windows(expanded, context, _depth + 1) diff --git a/tests/test_subagent_host_env.py b/tests/test_subagent_host_env.py index f2aefdde2..4ac7b8025 100644 --- a/tests/test_subagent_host_env.py +++ b/tests/test_subagent_host_env.py @@ -4,6 +4,9 @@ a login shell's, which is the step that drops them if it is missed: the capture runs `$SHELL -lic` from a minimal base precisely so raven's own variables do not leak, so anything the host means the child to see has to be overlaid back. + +Windows has no login shell, so the same boundary is crossed there by a registry +read; the section at the end covers that capture. """ from __future__ import annotations @@ -17,10 +20,12 @@ import pytest -import raven.agent.subagent.backends.env as backend_env_mod +import raven.agent.subagent.probe as probe_mod from raven.acp_client.client import AcpClient from raven.agent.subagent.backends import env as backend_env +from raven.agent.subagent.probe import probe_all from raven.agent.subagent.role import SUBAGENT_ENV_VAR, is_subagent_process +from raven.config.schema import ThirdPartyAcpSubagentConfig def _reporting_child(script: Path, variable: str) -> str: @@ -96,21 +101,20 @@ async def test_acp_child_is_told_it_serves_as_a_subagent(tmp_path: Path, monkeyp assert is_subagent_process(), f"the child was handed {arrived!r}, which it does not read as a sub-agent role" -# --- Windows login environment capture ----------------------------------- +# --- Windows: the capture -------------------------------------------------- # -# Windows has no login shell. The capture reads the two registry stores a -# terminal is assembled from (machine, then user), merged over raven's own -# inherited environment, with Path concatenated the way Windows builds it. +# Windows has no login shell. The capture rebuilds PATH from the two registry +# stores a new terminal's Path is assembled from, and keeps every other value +# raven already has. The fixtures are shaped the way Windows hands them over: +# the stores' REG_EXPAND_SZ values unexpanded, and raven's own environment with +# every name upper-cased, which is how CPython's ``os.environ`` holds it there. class _FakeKey: - def __init__(self, values: dict[str, str], *, fail: bool = False) -> None: + def __init__(self, values: dict[str, str]) -> None: self.values = values - self.fail = fail def __enter__(self) -> "_FakeKey": - if self.fail: - raise OSError("access denied") return self def __exit__(self, *_: Any) -> None: @@ -118,126 +122,171 @@ def __exit__(self, *_: Any) -> None: class _FakeWinreg: - """``import winreg`` is Windows-only, so the registry is faked here exactly. - - Only the three calls the capture makes are honest: ``OpenKey`` hands a key - over a store a test seeded, and the enumeration returns that store. The - dict keeps insertion order, so user-after-machine remains visible. - """ + """``winreg`` over two stores a test edits; a store set to ``None`` refuses the read.""" HKEY_LOCAL_MACHINE = 0x80000002 HKEY_CURRENT_USER = 0x80000001 REG_EXPAND_SZ = 2 - def __init__(self, machine: dict[str, str] | bool, user: dict[str, str] | bool) -> None: - self._stores = {self.HKEY_LOCAL_MACHINE: machine, self.HKEY_CURRENT_USER: user} + def __init__(self, machine: dict[str, str] | None, user: dict[str, str] | None) -> None: + self.machine = machine + self.user = user def OpenKey(self, hive: int, _subkey: str) -> _FakeKey: - store = self._stores[hive] - return _FakeKey({}, fail=True) if store is False else _FakeKey(dict(store)) + store = self.machine if hive == self.HKEY_LOCAL_MACHINE else self.user + if store is None: + raise PermissionError(5, "Access is denied") + return _FakeKey(dict(store)) def QueryInfoKey(self, key: _FakeKey) -> tuple[int, int, int]: return (0, len(key.values), 0) def EnumValue(self, key: _FakeKey, index: int) -> tuple[str, str, int]: name = list(key.values)[index] - return (name, key.values[name], 1) - - -def _fake_winreg( - monkeypatch: pytest.MonkeyPatch, - machine: dict[str, str] | bool, - user: dict[str, str] | bool, + return (name, key.values[name], self.REG_EXPAND_SZ) + + +# What `reg query` shows in each store on a default install. +_STOCK_MACHINE = { + "ComSpec": r"%SystemRoot%\system32\cmd.exe", + "Path": r"%SystemRoot%\system32;%SystemRoot%", + "PSModulePath": r"%ProgramFiles%\WindowsPowerShell\Modules", + "TEMP": r"%SystemRoot%\TEMP", + "TMP": r"%SystemRoot%\TEMP", + "USERNAME": "SYSTEM", + "windir": r"%SystemRoot%", +} +_STOCK_USER = { + "Path": r"%USERPROFILE%\AppData\Local\Microsoft\WindowsApps", + "TEMP": r"%USERPROFILE%\AppData\Local\Temp", + "TMP": r"%USERPROFILE%\AppData\Local\Temp", +} +# Raven's own environment, as the logon built it from those stores. +_LOGON = { + "COMSPEC": r"C:\Windows\system32\cmd.exe", + "PATH": r"C:\Windows\system32;C:\Windows;C:\Users\me\AppData\Local\Microsoft\WindowsApps", + "SYSTEMROOT": r"C:\Windows", + "TEMP": r"C:\Users\me\AppData\Local\Temp", + "TMP": r"C:\Users\me\AppData\Local\Temp", + "USERNAME": "me", + "USERPROFILE": r"C:\Users\me", + "WINDIR": r"C:\Windows", +} + + +@pytest.fixture +def windows_host(monkeypatch: pytest.MonkeyPatch) -> _FakeWinreg: + """Raven started on Windows over the stock stores, with no capture taken yet. + + The ambient environment goes first: on Linux it can hold one variable under two + spellings (``HTTP_PROXY`` beside ``http_proxy``), which Windows never can, and a + test that read it would measure the machine it ran on. + """ + for name in list(os.environ): + if not name.startswith("PYTEST_"): + monkeypatch.delenv(name) + for name, value in _LOGON.items(): + monkeypatch.setenv(name, value) + registry = _FakeWinreg(dict(_STOCK_MACHINE), dict(_STOCK_USER)) + monkeypatch.setitem(sys.modules, "winreg", registry) + monkeypatch.setattr(backend_env, "_on_windows", lambda: True) + monkeypatch.setattr(backend_env, "_LOGIN_ENV", None) + monkeypatch.setattr(backend_env, "_LOGIN_ENV_FAILED", False) + return registry + + +def test_windows_capture_takes_path_from_the_stores_and_every_other_value_from_raven( + windows_host: _FakeWinreg, ) -> None: - monkeypatch.setattr(sys, "winreg", _FakeWinreg(machine, user), raising=False) - monkeypatch.setitem(sys.modules, "winreg", _FakeWinreg(machine, user)) - + r"""The reported failure: an installer ran after the gateway started. Only PATH moves. + + A terminal opened after ``uv tool install`` builds its Path from the store the + installer wrote, and so does this capture. The stores' other values are what + Windows builds an environment *from* -- ``ComSpec`` and ``TEMP`` unexpanded, the + machine's own ``USERNAME`` -- so copied over raven's they would hand a child a + ``ComSpec`` that ``Popen(shell=True)`` cannot start, a ``TEMP`` that is no + directory, and a second spelling of names raven already has. + """ + windows_host.user["Path"] += r";C:\Users\me\.local\bin" -def test_windows_capture_refreshes_a_frozen_process_path(monkeypatch: pytest.MonkeyPatch) -> None: - """The reported failure: an installer ran after the gateway started. + captured = backend_env._capture_windows(consequence="subagents inherit raven's environment") - A service keeps the PATH it started with. A terminal opened after - `uv tool install` reads the store the installer wrote, and so does this - capture: the merged answer carries every inherited variable and the - store's Path -- not the process's stale one. - """ - monkeypatch.setenv("PATH", r"C:\stale\service-bin") - monkeypatch.setenv("HTTP_PROXY", "http://proxy:8888") - _fake_winreg( - monkeypatch, - machine={"Path": r"C:\Windows\system32", "ComSpec": r"C:\Windows\system32\cmd.exe"}, - user={"Path": r"C:\Users\me\AppData\Roaming\uv\tools\raven\Scripts"}, - ) - captured = backend_env_mod._capture_windows(consequence="subagents inherit raven's environment") assert captured is not None - assert captured["Path"] == ( - r"C:\Windows\system32;C:\Users\me\AppData\Roaming\uv\tools\raven\Scripts;C:\stale\service-bin" + assert captured["PATH"] == ( + r"C:\Windows\system32;C:\Windows;C:\Users\me\AppData\Local\Microsoft\WindowsApps;C:\Users\me\.local\bin;" + r"C:\Windows\system32;C:\Windows;C:\Users\me\AppData\Local\Microsoft\WindowsApps" ) - assert captured["HTTP_PROXY"] == "http://proxy:8888", "a child keeps the process vars the store knows nothing of" - assert captured["ComSpec"] == r"C:\Windows\system32\cmd.exe" - assert sum(1 for k in captured if k.lower() == "path") == 1, "no duplicate Path spellings beside Path" + assert {name: value for name, value in captured.items() if name != "PATH"} == { + name: value for name, value in os.environ.items() if name != "PATH" + } -def test_windows_capture_environments_names_fold_case_insensitively(monkeypatch: pytest.MonkeyPatch) -> None: - """One name, one entry: `TEMP` from the process and `Temp` from the store are the same variable. +def test_windows_capture_expands_stored_references_with_ravens_own_values_first( + windows_host: _FakeWinreg, +) -> None: + r"""A reference in a stored Path resolves to what a terminal of this logon would get. - A child env block carrying both spellings resolves to an arbitrary one on - Windows -- and to the later-written one, which the store's refresh would - then silently contradict. + Raven's own values win because they are the logon's -- the machine store's + ``USERNAME`` is ``SYSTEM`` -- and the child carries them beside this PATH. A + variable an installer added after raven started is only in the stores, where the + user's shadows the machine's and may hold a reference of its own. """ - monkeypatch.setenv("TEMP", r"C:\stale-tmp") - _fake_winreg(monkeypatch, machine=False, user={"Temp": r"C:\Users\me\AppData\Local\Temp"}) - captured = backend_env_mod._capture_windows(consequence="...") - assert captured is not None - assert captured.get("Temp") == r"C:\Users\me\AppData\Local\Temp" - assert "TEMP" not in captured + windows_host.machine.update({"Path": r"C:\Users\%USERNAME%\bin;%NVM_HOME%", "NVM_HOME": r"C:\nvm\machine"}) + windows_host.user.update({"Path": "%NVM_SYMLINK%", "NVM_HOME": r"C:\nvm\user", "NVM_SYMLINK": r"%NVM_HOME%\nodejs"}) + captured = backend_env._capture_windows(consequence="subagents inherit raven's environment") -def test_windows_capture_keeps_up_when_only_one_store_reads(monkeypatch: pytest.MonkeyPatch) -> None: - """HKLM can refuse a non-elevated read; the user's entries must not be lost with it.""" - monkeypatch.setenv("PATH", r"C:\legacy") - _fake_winreg(monkeypatch, machine=False, user={"Path": r"C:\user-only"}) - captured = backend_env_mod._capture_windows(consequence="...") assert captured is not None - assert captured["Path"] == r"C:\user-only;C:\legacy" - + assert captured["PATH"].split(";")[:3] == [r"C:\Users\me\bin", r"C:\nvm\user", r"C:\nvm\user\nodejs"] -def test_windows_capture_reports_failure_when_no_store_reads(monkeypatch: pytest.MonkeyPatch) -> None: - """Both stores refusing is the one case the driver must fall back on ``os.environ``.""" - _fake_winreg(monkeypatch, machine=False, user=False) - assert backend_env_mod._capture_windows(consequence="subagents inherit raven's environment") is None +def test_windows_capture_keeps_the_users_store_when_the_machines_refuses(windows_host: _FakeWinreg) -> None: + """HKLM can refuse a read; the user's entries must not be lost with it.""" + windows_host.machine = None -def test_windows_capture_expands_percent_vars_in_the_stored_path(monkeypatch: pytest.MonkeyPatch) -> None: - r"""`%USERPROFILE%\go\bin` in the registry is a path; left raw it is not one. + captured = backend_env._capture_windows(consequence="subagents inherit raven's environment") - The gateway's measured log shows a launcher script dying on exactly that: - the store writes the reference, the read-before mine passed it on, and a - terminal got the expanded form. Machine-level references resolve against - machine values first (USERPROFILE is the service account's there), then - against the process's inherited env -- SHELL-adjusted var precedence. - """ - monkeypatch.setenv("USERPROFILE", r"C:\Users\me") - monkeypatch.setenv("PATH", r"C:\legacy") - _fake_winreg( - monkeypatch, - machine={"Path": r"C:\tools;%ProgramFiles%\App", "ProgramFiles": r"C:\Program Files"}, - user={"Path": r"%USERPROFILE%\AppData\Roaming\uv\tools\raven\Scripts"}, - ) - captured = backend_env_mod._capture_windows(consequence="...") assert captured is not None - assert captured["Path"] == r"C:\tools;C:\Program Files\App;C:\Users\me\AppData\Roaming\uv\tools\raven\Scripts;C:\legacy" + assert captured["PATH"] == r"C:\Users\me\AppData\Local\Microsoft\WindowsApps;" + _LOGON["PATH"] + +def test_windows_capture_reports_failure_when_no_store_reads(windows_host: _FakeWinreg) -> None: + """Both stores refusing is the one case the caller must fall back on ``os.environ``.""" + windows_host.machine = None + windows_host.user = None -def test_windows_capture_runs_in_place_of_the_posix_one_on_win32(monkeypatch: pytest.MonkeyPatch) -> None: - """The platform decides the capture; on win32 the registry-built path beats raven's frozen one.""" - monkeypatch.setenv("PATH", r"C:\service-legacy-bin") - _fake_winreg(monkeypatch, machine={"Path": r"C:\system32"}, user={"Path": r"C:\user"}) - monkeypatch.setattr(backend_env_mod.sys, "platform", "win32") - monkeypatch.setattr(backend_env_mod, "_LOGIN_ENV", None) - monkeypatch.setattr(backend_env_mod, "_LOGIN_ENV_FAILED", False) - env = backend_env_mod.login_shell_env() - assert env["Path"] == r"C:\system32;C:\user;C:\service-legacy-bin" + assert backend_env._capture_windows(consequence="subagents inherit raven's environment") is None + + +async def test_windows_probe_finds_agents_on_the_captured_path( + tmp_path: Path, windows_host: _FakeWinreg, monkeypatch: pytest.MonkeyPatch +) -> None: + """The capture is read by name -- ``probe_all`` asks it for ``PATH`` -- so it is checked through the probe. + + One agent sits on the PATH raven started with, one only in a directory the + user's store gained afterwards. Stored under the registry's ``Path`` spelling, + the capture left the probe an empty PATH and both read as missing, the first of + them found before the capture existed. + """ + boot_bin, fresh_bin = tmp_path / "boot-bin", tmp_path / "fresh-bin" + for directory, name in ((boot_bin, "on-boot-agent"), (fresh_bin, "fresh-agent")): + directory.mkdir() + (directory / name).write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + (directory / name).chmod(0o755) + monkeypatch.setenv("PATH", str(boot_bin)) + windows_host.user["Path"] = str(fresh_bin) + monkeypatch.setattr(os, "pathsep", ";") + monkeypatch.setattr(probe_mod, "acp_snapshot_for", lambda cfg: None) + rows = [ + ThirdPartyAcpSubagentConfig(name=name, command=f"{name} --acp") for name in ("on-boot-agent", "fresh-agent") + ] + + results = await probe_all([(cfg, "config") for cfg in rows]) + + assert [(r.name, r.status, r.target) for r in results] == [ + ("on-boot-agent", "attention", str(boot_bin / "on-boot-agent")), + ("fresh-agent", "attention", str(fresh_bin / "fresh-agent")), + ] def test_login_shell_reads_a_windows_shaped_bash_path(monkeypatch: pytest.MonkeyPatch) -> None: @@ -249,4 +298,4 @@ def test_login_shell_reads_a_windows_shaped_bash_path(monkeypatch: pytest.Monkey """ shell = r"C:\Program Files\Git\usr\bin\bash.exe" monkeypatch.setenv("SHELL", shell) - assert backend_env_mod._login_shell() == shell + assert backend_env._login_shell() == shell From cabc33deff3d5882452096348c3dfd105a238575 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 9 Oct 2026 03:46:52 +0000 Subject: [PATCH 4/5] fix(agent): refresh the Windows capture the way the first one is taken Check again, and every Connect or Test, refresh the capture through refresh_login_shell_env, which called the login-shell capture on every host. On Windows it found no shell, kept the first capture, and an agent installed after that stayed missing until a restart. With Git for Windows' SHELL it was worse: that bash is exported as C:\Program Files\Git\usr\bin\bash.exe, which _login_shell had been taught to drive, so a refresh ran bash -lic "env -0" from the POSIX bootstrap base and swapped an MSYS environment in, a ':'-joined PATH of /c/... entries with no SystemRoot. The first capture and every refresh now go through _capture_now, which reads the registry on Windows and the login shell elsewhere. The git-bash spelling is no longer recognized: _capture is not reachable on Windows any more, and a POSIX host never sees that spelling. The test fixture that pins the POSIX capture pins the same switch. Co-authored-by: Claude (claude-opus-5-5[1m]) --- raven/agent/subagent/backends/env.py | 37 +++++++++++-------------- tests/test_subagent_host_env.py | 40 +++++++++++++++++++--------- tests/test_subagent_third_party.py | 8 +++--- 3 files changed, 47 insertions(+), 38 deletions(-) diff --git a/raven/agent/subagent/backends/env.py b/raven/agent/subagent/backends/env.py index 27e225145..d73369f38 100644 --- a/raven/agent/subagent/backends/env.py +++ b/raven/agent/subagent/backends/env.py @@ -77,22 +77,10 @@ def _on_windows() -> bool: def _login_shell() -> str | None: - r"""The user's login shell if this module can drive it, else ``None``. - - ``basename`` alone leaves a Windows-shaped bash unread: git-bash exports - ``SHELL=C:\Program Files\Git\usr\bin\bash.exe`` with backslashes, and - POSIX ``basename`` sees no separator. Both flavors of path are split so - that spelling drives the shell it names rather than falling back and - losing git-bash's profile-installed PATH entries on a POSIX host that is - not Windows. - """ + """The user's login shell if this module can drive it, else ``None``.""" shell = os.environ.get("SHELL", "").strip() - if not shell: - return None - name = shell.replace("\\", "/").rsplit("/", 1)[-1] - if name.lower().endswith(".exe"): - name = name[:-4] - return shell if name in _DRIVABLE_SHELLS else None + + return shell if shell and os.path.basename(shell) in _DRIVABLE_SHELLS else None def login_shell_env() -> dict[str, str]: @@ -111,11 +99,7 @@ def login_shell_env() -> dict[str, str]: return dict(_LOGIN_ENV) if _LOGIN_ENV_FAILED: return dict(os.environ) - captured = ( - _capture_windows(consequence="subagents inherit raven's environment") - if _on_windows() - else _capture(consequence="subagents inherit raven's environment") - ) + captured = _capture_now(consequence="subagents inherit raven's environment") if captured is None: _LOGIN_ENV_FAILED = True return dict(os.environ) @@ -143,13 +127,24 @@ def refresh_login_shell_env() -> bool: returns. Returns whether a capture landed. """ global _LOGIN_ENV - captured = _capture(consequence="keeping the environment captured earlier") + captured = _capture_now(consequence="keeping the environment captured earlier") if captured is None: return False _LOGIN_ENV = captured return True +def _capture_now(*, consequence: str) -> dict[str, str] | None: + """This platform's capture: the registry's PATH on Windows, the login shell elsewhere. + + The first capture and every refresh both come here, so a refresh cannot go + looking for a login shell on a host whose first capture never had one. + """ + if _on_windows(): + return _capture_windows(consequence=consequence) + return _capture(consequence=consequence) + + def _capture_windows(*, consequence: str) -> dict[str, str] | None: r"""Raven's own environment, with the PATH a freshly opened Windows terminal would start with. diff --git a/tests/test_subagent_host_env.py b/tests/test_subagent_host_env.py index 4ac7b8025..c13c01b75 100644 --- a/tests/test_subagent_host_env.py +++ b/tests/test_subagent_host_env.py @@ -16,7 +16,7 @@ import shlex import sys from pathlib import Path -from typing import Any +from typing import Any, NoReturn import pytest @@ -195,6 +195,10 @@ def windows_host(monkeypatch: pytest.MonkeyPatch) -> _FakeWinreg: return registry +def _no_shell(argv: list[str], **_: Any) -> NoReturn: + raise AssertionError(f"no shell is started on Windows, got {argv}") + + def test_windows_capture_takes_path_from_the_stores_and_every_other_value_from_raven( windows_host: _FakeWinreg, ) -> None: @@ -258,6 +262,28 @@ def test_windows_capture_reports_failure_when_no_store_reads(windows_host: _Fake assert backend_env._capture_windows(consequence="subagents inherit raven's environment") is None +def test_a_refresh_on_windows_reads_the_stores_again( + windows_host: _FakeWinreg, monkeypatch: pytest.MonkeyPatch +) -> None: + """Check again, Connect and Test each refresh the capture; on Windows that is another registry read. + + Routed to the login-shell capture instead, a refresh found no shell, kept the + first capture, and an agent installed after it stayed missing until a restart -- + the case the Windows capture exists for. Git for Windows exports ``SHELL`` to + what it starts, and driving that bash would have been worse: an MSYS + environment, a ``:``-joined PATH of ``/c/...`` entries and no ``SystemRoot``, + swapped in for every probe and spawn after it. + """ + monkeypatch.setenv("SHELL", r"C:\Program Files\Git\usr\bin\bash.exe") + monkeypatch.setattr(backend_env.subprocess, "run", _no_shell) + assert r"C:\Users\me\.local\bin" not in backend_env.login_shell_env()["PATH"] + + windows_host.user["Path"] += r";C:\Users\me\.local\bin" + + assert backend_env.refresh_login_shell_env() is True + assert r"C:\Users\me\.local\bin" in backend_env.login_shell_env()["PATH"] + + async def test_windows_probe_finds_agents_on_the_captured_path( tmp_path: Path, windows_host: _FakeWinreg, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -287,15 +313,3 @@ async def test_windows_probe_finds_agents_on_the_captured_path( ("on-boot-agent", "attention", str(boot_bin / "on-boot-agent")), ("fresh-agent", "attention", str(fresh_bin / "fresh-agent")), ] - - -def test_login_shell_reads_a_windows_shaped_bash_path(monkeypatch: pytest.MonkeyPatch) -> None: - """basename alone dropped it: git-bash's `SHELL` has backslashes, and POSIX split sees no separator. - - The gateway's log had SHELL=`C:\\Program Files\\Git\\usr\\bin\\bash.exe` be "not one this - build can drive" and every probe ran on raven's own PATH -- invisible to the - README's own install-from-git-bash recipe, which then answered "agent missing". - """ - shell = r"C:\Program Files\Git\usr\bin\bash.exe" - monkeypatch.setenv("SHELL", shell) - assert backend_env._login_shell() == shell diff --git a/tests/test_subagent_third_party.py b/tests/test_subagent_third_party.py index 6f8a29041..d1eae073f 100644 --- a/tests/test_subagent_third_party.py +++ b/tests/test_subagent_third_party.py @@ -82,10 +82,10 @@ def _clear_login_env_cache(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(env_mod, "_LOGIN_ENV", None) monkeypatch.setattr(env_mod, "_LOGIN_ENV_FAILED", False) monkeypatch.setenv("SHELL", "/bin/bash") - # These tests capture from a POSIX login shell. On a Windows runner the - # platform would route them to `_capture_windows` (PowerShell) and their - # patched `bash -lic ...` would never be invoked, so pin the platform too. - monkeypatch.setattr(env_mod.sys, "platform", "linux") + # These tests capture from a POSIX login shell. On a Windows host the + # capture is a registry read instead and their patched `bash -lic ...` would + # never run, so the platform is pinned too. + monkeypatch.setattr(env_mod, "_on_windows", lambda: False) def test_login_shell_env_parses_nul_separated_output( From e79c65f7893ba576a5af3fce29443c5564e351e5 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 9 Oct 2026 03:47:02 +0000 Subject: [PATCH 5/5] fix(*): start a bare Windows program from the child's own PATH CreateProcess looks a bare program name up on the PATH of the process that calls it and never on the env block it is handed; POSIX exec reads the PATH of that env. So on Windows an agent installed after the gateway started, which the refreshed capture and the probe now find, still would not start: the launch searched the gateway's frozen PATH. resolve_program looks the name up on the child's PATH instead, by CreateProcess's own rule that a name with no extension means .exe, and puts in only what CreateProcess runs natively. A .cmd or .bat shim is left alone: CreateProcess never turns a bare name into one, and doing it here would put the shim's arguments, a cli agent's prompt among them, through cmd.exe's parser. The acp launch, the cli launch and the Kimi Code ask all start their program this way; POSIX is unchanged. Co-authored-by: Claude (claude-opus-5-5[1m]) --- raven/acp_client/client.py | 3 +- raven/agent/subagent/backends/cli_agent.py | 3 +- raven/agent/subagent/backends/env.py | 32 +++++++++- raven/agent/subagent/kimi_code.py | 5 +- tests/test_subagent_host_env.py | 73 +++++++++++++++++++++- tests/test_subagent_kimi_code.py | 24 +++++++ tests/test_subagent_third_party.py | 34 +++++++++- 7 files changed, 164 insertions(+), 10 deletions(-) diff --git a/raven/acp_client/client.py b/raven/acp_client/client.py index b4cbafcbe..97eddfc11 100644 --- a/raven/acp_client/client.py +++ b/raven/acp_client/client.py @@ -229,7 +229,7 @@ async def launch( # Imported here rather than at module level: backends/__init__ pulls in # the acp backend, which pulls in this module, so a module-level import # back into that package would close the cycle at init time. - from raven.agent.subagent.backends.env import host_identity_env, login_shell_env + from raven.agent.subagent.backends.env import host_identity_env, login_shell_env, resolve_program from raven.agent.subagent.role import subagent_role_env try: @@ -247,6 +247,7 @@ async def launch( # The role goes on before the caller's own map, so a config ``env`` entry # is the way to hand one agent back its full registry. child_env = {**base_env, **host_identity_env(), **subagent_role_env(), **(env or {})} + argv = resolve_program(argv, child_env) try: proc = await asyncio.create_subprocess_exec( *argv, diff --git a/raven/agent/subagent/backends/cli_agent.py b/raven/agent/subagent/backends/cli_agent.py index 59072bc24..21117e779 100644 --- a/raven/agent/subagent/backends/cli_agent.py +++ b/raven/agent/subagent/backends/cli_agent.py @@ -34,7 +34,7 @@ from raven.agent.subagent import activity from raven.agent.subagent.attachments import with_attachment_note from raven.agent.subagent.backends.base import bounded_delta, clamp_output -from raven.agent.subagent.backends.env import host_identity_env, login_shell_env +from raven.agent.subagent.backends.env import host_identity_env, login_shell_env, resolve_program from raven.agent.subagent.backends.observability import ( external_agent_span, record_outcome, @@ -438,6 +438,7 @@ async def _exec( # is the way to hand one agent back its full registry. A third-party # CLI reads none of these; a ``raven`` one does. env = {**env_base, **host_identity_env(), **subagent_role_env(), **(runtime_env or {}), **self.env} + argv = resolve_program(argv, env) logger.info("Subagent [{}] CLI agent {!r}: {}", task_id, self.name, argv[:1]) proc = await asyncio.create_subprocess_exec( *argv, diff --git a/raven/agent/subagent/backends/env.py b/raven/agent/subagent/backends/env.py index d73369f38..e89750c8c 100644 --- a/raven/agent/subagent/backends/env.py +++ b/raven/agent/subagent/backends/env.py @@ -42,15 +42,20 @@ environment, and installers refresh a terminal opened afterwards by broadcasting WM_SETTINGCHANGE. `_capture_windows` rebuilds PATH from the persisted User and Machine stores that broadcast refreshes and keeps every -other value of raven's own environment. +other value of raven's own environment. A launch finds a bare program name +differently there too, so `resolve_program` lives beside the capture whose +PATH it reads. """ from __future__ import annotations +import ntpath import os import re +import shutil import subprocess import sys +from collections.abc import Mapping from loguru import logger @@ -72,10 +77,31 @@ def _on_windows() -> bool: - """Whether this is Windows, which decides how the environment below is captured.""" + """Whether this is Windows, the one switch the capture and the launch lookup below read.""" return sys.platform == "win32" +def resolve_program(argv: list[str], env: Mapping[str, str]) -> list[str]: + r"""``argv`` with a bare program name looked up on the PATH of the env the child gets. + + POSIX needs nothing here: its exec resolves the name on that PATH itself. + CreateProcess searches the gateway's own PATH instead and never the block it is + handed, so on Windows an agent installed after raven started -- found by the + refreshed capture and by the probe -- would still not start. The lookup follows + CreateProcess's own rule that a name with no extension means ``.exe``, and only + a program CreateProcess runs natively is put in its place: CreateProcess never + turns a bare name into a ``.cmd`` or ``.bat`` shim, and doing it here would put + the shim's arguments, a cli agent's prompt among them, through cmd.exe's parser. + """ + if not _on_windows() or not argv or ntpath.dirname(argv[0]): + return argv + program = argv[0] if ntpath.splitext(argv[0])[1] else f"{argv[0]}.exe" + if ntpath.splitext(program)[1].lower() not in (".exe", ".com"): + return argv + found = shutil.which(program, path=env.get("PATH")) + return [found, *argv[1:]] if found else argv + + def _login_shell() -> str | None: """The user's login shell if this module can drive it, else ``None``.""" shell = os.environ.get("SHELL", "").strip() @@ -304,4 +330,4 @@ def host_identity_env() -> dict[str, str]: return {HOME_ENV_VAR: home} if home else {} -__all__ = ["host_identity_env", "login_shell_env", "refresh_login_shell_env"] +__all__ = ["host_identity_env", "login_shell_env", "refresh_login_shell_env", "resolve_program"] diff --git a/raven/agent/subagent/kimi_code.py b/raven/agent/subagent/kimi_code.py index 274058877..915fc94a3 100644 --- a/raven/agent/subagent/kimi_code.py +++ b/raven/agent/subagent/kimi_code.py @@ -340,7 +340,7 @@ async def _run(cfg: Any, *args: str) -> tuple[int | None, str]: being cancelled when the page that asked goes away -- so an ask never outlives the question. """ - from raven.agent.subagent.backends.env import host_identity_env, login_shell_env + from raven.agent.subagent.backends.env import host_identity_env, login_shell_env, resolve_program from raven.agent.subagent.role import subagent_role_env # The map `AcpClient.launch` builds, so the `kimi` asked is the one the @@ -350,8 +350,7 @@ async def _run(cfg: Any, *args: str) -> tuple[int | None, str]: with tempfile.TemporaryDirectory(prefix="raven_subagent_ask_") as cwd: try: proc = await asyncio.create_subprocess_exec( - "kimi", - *args, + *resolve_program(["kimi", *args], env), cwd=cwd, env=env, stdin=asyncio.subprocess.DEVNULL, diff --git a/tests/test_subagent_host_env.py b/tests/test_subagent_host_env.py index c13c01b75..bf082e3b0 100644 --- a/tests/test_subagent_host_env.py +++ b/tests/test_subagent_host_env.py @@ -6,7 +6,7 @@ leak, so anything the host means the child to see has to be overlaid back. Windows has no login shell, so the same boundary is crossed there by a registry -read; the section at the end covers that capture. +read; the sections at the end cover that capture and the launch that consumes it. """ from __future__ import annotations @@ -22,6 +22,7 @@ import raven.agent.subagent.probe as probe_mod from raven.acp_client.client import AcpClient +from raven.acp_client.protocol import AcpConnectionError from raven.agent.subagent.backends import env as backend_env from raven.agent.subagent.probe import probe_all from raven.agent.subagent.role import SUBAGENT_ENV_VAR, is_subagent_process @@ -313,3 +314,73 @@ async def test_windows_probe_finds_agents_on_the_captured_path( ("on-boot-agent", "attention", str(boot_bin / "on-boot-agent")), ("fresh-agent", "attention", str(fresh_bin / "fresh-agent")), ] + + +# --- Windows: the launch --------------------------------------------------- + + +def _recording_refusal(started: list[tuple[str, ...]]) -> Any: + async def refuse(*argv: str, **_: Any) -> NoReturn: + started.append(argv) + raise FileNotFoundError(2, "not started in a test") + + return refuse + + +async def test_on_windows_a_bare_program_starts_from_the_childs_own_path( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """CreateProcess finds a bare name on the gateway's own PATH, never on the env block it is handed. + + So an agent installed after raven started, which the refreshed capture and the + probe both find, would still not start. The launch looks the name up on the + child's PATH itself, by CreateProcess's own rule that a name with no extension + means ``.exe``. + """ + fresh_bin = tmp_path / "fresh-bin" + fresh_bin.mkdir() + (fresh_bin / "fresh-agent.exe").write_text("", encoding="utf-8") + (fresh_bin / "fresh-agent.exe").chmod(0o755) + monkeypatch.setattr(backend_env, "_on_windows", lambda: True) + monkeypatch.setattr(backend_env, "login_shell_env", lambda: {"PATH": str(fresh_bin)}) + started: list[tuple[str, ...]] = [] + monkeypatch.setattr(asyncio, "create_subprocess_exec", _recording_refusal(started)) + + with pytest.raises(AcpConnectionError): + await AcpClient.launch(name="fresh", command="fresh-agent acp") + + assert started == [(str(fresh_bin / "fresh-agent.exe"), "acp")] + + +async def test_off_windows_the_launch_leaves_the_lookup_to_exec( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """POSIX ``exec`` already resolves a bare name on the PATH of the env it is handed.""" + fresh_bin = tmp_path / "fresh-bin" + fresh_bin.mkdir() + (fresh_bin / "fresh-agent.exe").write_text("", encoding="utf-8") + (fresh_bin / "fresh-agent.exe").chmod(0o755) + monkeypatch.setattr(backend_env, "_on_windows", lambda: False) + monkeypatch.setattr(backend_env, "login_shell_env", lambda: {"PATH": str(fresh_bin)}) + started: list[tuple[str, ...]] = [] + monkeypatch.setattr(asyncio, "create_subprocess_exec", _recording_refusal(started)) + + with pytest.raises(AcpConnectionError): + await AcpClient.launch(name="fresh", command="fresh-agent acp") + + assert started == [("fresh-agent", "acp")] + + +def test_on_windows_a_shim_on_the_childs_path_is_not_made_the_program( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A ``.cmd`` shim runs through cmd.exe, whose parser would get the arguments -- a cli agent's prompt among them.""" + npm_bin = tmp_path / "npm" + npm_bin.mkdir() + (npm_bin / "codex.cmd").write_text("", encoding="utf-8") + (npm_bin / "codex.cmd").chmod(0o755) + monkeypatch.setattr(backend_env, "_on_windows", lambda: True) + child_env = {"PATH": str(npm_bin)} + + assert backend_env.resolve_program(["codex", "exec", "a & b"], child_env) == ["codex", "exec", "a & b"] + assert backend_env.resolve_program(["codex.cmd", "exec"], child_env) == ["codex.cmd", "exec"] diff --git a/tests/test_subagent_kimi_code.py b/tests/test_subagent_kimi_code.py index 9b1f6cd60..f3dc88173 100644 --- a/tests/test_subagent_kimi_code.py +++ b/tests/test_subagent_kimi_code.py @@ -21,6 +21,7 @@ import pytest +import raven.agent.subagent.backends.env as env_mod from raven.agent.subagent import kimi_code from raven.agent.subagent.probe_state import Remedy from raven.config.schema import ThirdPartyAcpSubagentConfig @@ -462,3 +463,26 @@ def test_the_plan_and_config_kinds_travel_to_the_page() -> None: assert Remedy.from_wire(Remedy("plan", PRICING).to_wire()) == Remedy("plan", PRICING) assert Remedy.from_wire(Remedy("config", "kimi doctor config").to_wire()) == Remedy("config", "kimi doctor config") assert Remedy.from_wire({"kind": "plan"}) == Remedy("plan") + + +async def test_on_windows_the_kimi_asked_is_the_one_on_the_childs_path( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The ask runs the ``kimi`` the connect started, so it is looked up the way the launch looks it up. + + On Windows that is the child's own PATH, under CreateProcess's rule that a name + with no extension means ``.exe``; the gateway's PATH is the one CreateProcess + would have searched, and an agent installed after raven started is not on it. + """ + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + kimi = bin_dir / "kimi.exe" + kimi.write_text("#!/bin/sh\nprintf '%s\\n' 'kimi, version 9.9.9'\n", encoding="utf-8") + kimi.chmod(0o755) + monkeypatch.setattr(env_mod, "_on_windows", lambda: True) + monkeypatch.setattr(env_mod, "_LOGIN_ENV", {"PATH": str(bin_dir)}) + + assert await kimi_code._run(ThirdPartyAcpSubagentConfig(name="Kimi Code", command="kimi acp"), "--version") == ( + 0, + "kimi, version 9.9.9\n", + ) diff --git a/tests/test_subagent_third_party.py b/tests/test_subagent_third_party.py index d1eae073f..8dae125ee 100644 --- a/tests/test_subagent_third_party.py +++ b/tests/test_subagent_third_party.py @@ -16,7 +16,7 @@ import uuid from contextlib import closing from pathlib import Path -from typing import Any +from typing import Any, NoReturn import pytest from aiohttp import web @@ -369,6 +369,38 @@ async def test_cli_backend_uses_login_env_and_per_agent_env_wins( assert out == "yes/agent" +async def test_cli_backend_on_windows_starts_a_bare_program_from_the_childs_path( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The cli launch looks a bare name up on the child's PATH, as the acp launch does. + + CreateProcess searches the gateway's own PATH and never the env block it is + handed, so an agent installed after raven started would be found by the probe + and still not start. + """ + fresh_bin = tmp_path / "fresh-bin" + fresh_bin.mkdir() + (fresh_bin / "fresh-agent.exe").write_text("", encoding="utf-8") + (fresh_bin / "fresh-agent.exe").chmod(0o755) + monkeypatch.setattr(env_mod, "_on_windows", lambda: True) + monkeypatch.setattr(env_mod, "_LOGIN_ENV", {"PATH": str(fresh_bin)}) + started: list[tuple[str, ...]] = [] + + async def refuse(*argv: str, **_: Any) -> NoReturn: + started.append(argv) + raise FileNotFoundError(2, "not started in a test") + + monkeypatch.setattr(asyncio, "create_subprocess_exec", refuse) + be = CliAgentBackend( + name="fresh", command="fresh-agent -p {prompt}", registry=InstanceRegistry(path=tmp_path / "inst.json") + ) + + with pytest.raises(FileNotFoundError): + await be.run("task", task_id="t1", workspace=tmp_path, executor=None) + + assert started == [(str(fresh_bin / "fresh-agent.exe"), "-p", "task")] + + async def test_cli_backend_exposes_parent_model_binding( tmp_path: Path, monkeypatch: pytest.MonkeyPatch,