From ca81ace2dc7d79e8c9f28edbd02d60386a343274 Mon Sep 17 00:00:00 2001 From: Jason Cat Date: Sun, 11 Oct 2026 18:40:31 +0800 Subject: [PATCH] fix(acp): resolve an absolute Windows agent command before spawning (#6656) * fix(acp): resolve an absolute Windows agent command before spawning A command configured as an absolute path without an extension -- npm's global shim layout, e.g. C:\npm\mcode beside mcode.cmd -- was handed to the ACP SDK unchanged and always failed: create_subprocess_exec applies no PATHEXT, so it ran npm's extensionless shell script and died with WinError 193 (or raised FileNotFoundError when only the .cmd exists). The separator guard from #6598 was too broad: only relative paths need to stay with the spawn, because their meaning is owned by the ACP-workspace cwd, while an absolute path does not depend on the cwd. Absolute paths now go through the same shutil.which lookup as bare names: the lookup searches the command's own directory and probes PATHEXT, and the configured value is kept when nothing matches so the not-found remediation still fires. Relative paths (bin/agent, ./bin/agent) still go to the spawn unchanged. Docs: README.md and backend/packages/harness/deerflow/tools/AGENTS.md now describe the bare/absolute vs relative split. AI disclosure: this change was drafted with AI assistance at my request, under my direction and review. * fix(acp): keep already-absolute lookup results unchanged Review feedback on #6656 (willem-bd): os.path.abspath collapsed a `..` that follows a symlink, so an absolute POSIX command like `/srv/current/../shared/agent` (with `current -> releases/v1`) was rewritten to `/srv/shared/agent` -- a path that either does not exist or names a different launcher. The anchoring to the Gateway cwd exists only for relative PATH lookup results, where shutil.which cannot know the cwd the spawn will use; an already-absolute result needs no rewrite. Regressions: a platform-independent stub-lookup test pinning the literal parent-segment path, and a POSIX-only test that builds the symlink layout and asserts the configured command is returned unchanged. --- README.md | 2 +- .../packages/harness/deerflow/tools/AGENTS.md | 2 +- .../tools/builtins/invoke_acp_agent_tool.py | 28 ++-- backend/tests/test_invoke_acp_agent_tool.py | 135 +++++++++++++++++- 4 files changed, 154 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 772c943c69..ce7e738519 100644 --- a/README.md +++ b/README.md @@ -372,7 +372,7 @@ streams simulate chunks from a non-streaming response, while no-tool streams sta - `CLAUDE_CODE_OAUTH_TOKEN_FILE_DESCRIPTOR` accepts a UTF-8 token handoff and reuses it for later model instances in the same process. Undecodable handoffs are skipped so Claude Code can still try its override or default credentials file. - CLI credential JSON files accept UTF-8 with or without a BOM, independently of the host locale. `make doctor` accepts the same files when checking CLI authentication. Invalid text encoding is treated as an unreadable source; Claude Code can still try its default file after an invalid override. - ACP agent entries are separate from model providers — if you configure `acp_agents.codex`, point it at a Codex ACP adapter such as `npx -y @zed-industries/codex-acp` - - A bare ACP agent `command` is resolved before the agent starts, so npm-installed launchers work on Windows too: `npx` or `mcode` is spawned as its `npx.cmd`/`mcode.cmd` shim instead of failing with a command-not-found error. The lookup uses the `PATH` the agent subprocess will actually see (`acp_agents..env.PATH` when set, otherwise the Gateway's) and hands the spawn an absolute path, so a relative `PATH` entry cannot be re-interpreted inside the agent's own workspace. A `command` containing a path separator is used as configured, and a bare name that cannot be resolved is still reported with the install guidance below. + - A bare or absolute ACP agent `command` is resolved before the agent starts, so npm-installed launchers work on Windows too: `npx` or `mcode` is spawned as its `npx.cmd`/`mcode.cmd` shim instead of failing with a command-not-found error, and an absolute path such as `C:\Users\me\AppData\Roaming\npm\mcode` resolves to the `mcode.cmd` shim beside npm's extensionless shell script instead of failing with `WinError 193`. The lookup uses the `PATH` the agent subprocess will actually see (`acp_agents..env.PATH` when set, otherwise the Gateway's) and hands the spawn an absolute path, so a relative `PATH` entry cannot be re-interpreted inside the agent's own workspace. A relative `command` (for example `bin/mcode`) is used as configured, and a command that cannot be resolved is still reported with the install guidance below. - Each ACP agent's `timeout_seconds` (default: 1800) is one shared budget for initialization, session creation, and the prompt, starting after the subprocess launches. On timeout, DeerFlow aborts the invocation and closes the subprocess before returning an error. Workspace/MCP preparation and subprocess cleanup are outside this budget. A `TimeoutError` raised by the SDK before this deadline expires retains its own error message. - MiniMax Code speaks ACP directly. Install and authenticate it, then add it as an ACP agent: diff --git a/backend/packages/harness/deerflow/tools/AGENTS.md b/backend/packages/harness/deerflow/tools/AGENTS.md index 7e48bcb881..a71eb39505 100644 --- a/backend/packages/harness/deerflow/tools/AGENTS.md +++ b/backend/packages/harness/deerflow/tools/AGENTS.md @@ -108,7 +108,7 @@ E2B output sync records remote file versions and actual host file metadata in a - `timeout_seconds` is one deadline shared by initialization, session creation, and prompt after process launch. Keep the SDK process context outside the timeout so teardown finishes before the tool returns; workspace/MCP preparation and teardown are outside the protocol budget. Real-process regressions in `tests/test_invoke_acp_agent_tool.py` cover every hung phase, the shared budget, success, and caller cancellation. Only report budget expiry when the timeout context's `expired()` is true; otherwise propagate SDK `TimeoutError` to `_format_invocation_error`. - ACP results collect only `agent_message_chunk` text. Thought chunks remain internal and must not be concatenated into the tool result - Missing ACP executables now return an actionable error message instead of a raw `[Errno 2]` -- A bare configured `command` is resolved before the spawn (`_resolve_agent_command`), off the event loop because the lookup stats the filesystem. The ACP SDK's stdio transport calls `asyncio.create_subprocess_exec`, which performs no `PATHEXT` lookup on Windows, so a bare npm launcher (`npx`, `mcode`) would raise `FileNotFoundError` although its `.cmd` shim is on `PATH`. Resolution consults the `PATH` the agent subprocess will actually see (`acp_agents..env.PATH` when the agent config sets one, the Gateway's otherwise) and returns an absolute path, so a relative `PATH` entry cannot be re-interpreted inside the ACP workspace. A `command` containing a path separator goes to the spawn unchanged, so the spawn — not `shutil.which` — decides what it refers to, and `_format_invocation_error` always receives the configured value so its remediation still matches `config.yaml` +- A configured `command` that is a bare name or an absolute path is resolved before the spawn (`_resolve_agent_command`), off the event loop because the lookup stats the filesystem. The ACP SDK's stdio transport calls `asyncio.create_subprocess_exec`, which performs no `PATHEXT` lookup on Windows, so a bare npm launcher (`npx`, `mcode`) would raise `FileNotFoundError`, and an absolute path without an extension (`C:\npm\mcode`, npm's shell script beside its `.cmd` shim) would die with `WinError 193`, although the shim exists. Resolution consults the `PATH` the agent subprocess will actually see (`acp_agents..env.PATH` when the agent config sets one, the Gateway's otherwise) and returns an absolute path, so a relative `PATH` entry cannot be re-interpreted inside the ACP workspace; an absolute command is resolved inside its own directory. A relative `command` goes to the spawn unchanged, so the spawn — not `shutil.which` — decides what it refers to, and `_format_invocation_error` always receives the configured value so its remediation still matches `config.yaml` - Each ACP agent uses a per-thread workspace at `{base_dir}/users/{user_id}/threads/{thread_id}/acp-workspace/`. The workspace is accessible to the lead agent via the virtual path `/mnt/acp-workspace/` (read-only). In docker sandbox mode, the directory is volume-mounted into the container at `/mnt/acp-workspace` (read-only); in local sandbox mode, path translation is handled by `tools.py` Ordinary `task` results forward bounded `artifact.knowledge_sources` records diff --git a/backend/packages/harness/deerflow/tools/builtins/invoke_acp_agent_tool.py b/backend/packages/harness/deerflow/tools/builtins/invoke_acp_agent_tool.py index 067d2ebd28..17415e6e5b 100644 --- a/backend/packages/harness/deerflow/tools/builtins/invoke_acp_agent_tool.py +++ b/backend/packages/harness/deerflow/tools/builtins/invoke_acp_agent_tool.py @@ -143,18 +143,22 @@ def _agent_path(env: dict[str, str] | None) -> str | None: def _resolve_agent_command(command: str, path: str | None = None) -> str: - """Return the host path to spawn for a configured ACP agent command. + r"""Return the host path to spawn for a configured ACP agent command. ``asyncio.create_subprocess_exec``, which the ACP SDK's stdio transport uses, does not apply ``PATHEXT`` on Windows: a bare ``npx`` or ``mcode`` raises ``FileNotFoundError`` even though the npm shim (``npx.cmd``) is on - ``PATH``. Resolve the name the way the MCP Python SDK normalizes stdio - commands, and keep the configured value when nothing matches so the - not-found remediation still fires. + ``PATH``, and an absolute ``C:\npm\mcode`` spawns npm's extensionless shell + script instead of ``mcode.CMD`` and dies with ``WinError 193``. Resolve + the command the way the MCP Python SDK normalizes stdio commands, and keep + the configured value when nothing matches so the not-found remediation + still fires. - Only bare names are resolved. A configured path is handed to the spawn - unchanged, so the spawn -- not ``shutil.which`` -- decides what it refers - to. + Bare names and absolute paths are resolved. A relative configured path is + handed to the spawn unchanged: it refers to the ACP workspace the spawn + runs in, not to the Gateway's cwd. An absolute path keeps its own + directory -- ``shutil.which`` searches that directory and probes + ``PATHEXT`` there. ``path`` is the ``PATH`` the agent subprocess will actually see (see ``_agent_path``); the Gateway's own is only the fallback. A successful @@ -163,16 +167,22 @@ def _resolve_agent_command(command: str, path: str | None = None) -> str: ``cwd`` set to the ACP workspace, where that path resolves elsewhere -- or not at all. + An already-absolute result is returned unchanged: a lexical ``abspath`` + would collapse a ``..`` that follows a symlink, changing which file the + spawn actually runs. + The lookup stats the filesystem (``shutil.which`` -> ``os.access``), so callers must run it off the event loop. """ - if os.path.dirname(command): + if os.path.dirname(command) and not os.path.isabs(command): return command try: resolved = shutil.which(command, path=path) except OSError: return command - return os.path.abspath(resolved) if resolved else command + if not resolved: + return command + return resolved if os.path.isabs(resolved) else os.path.abspath(resolved) def _format_invocation_error(agent: str, cmd: str, exc: Exception) -> str: diff --git a/backend/tests/test_invoke_acp_agent_tool.py b/backend/tests/test_invoke_acp_agent_tool.py index 96dfa2aebd..54a32cb7d3 100644 --- a/backend/tests/test_invoke_acp_agent_tool.py +++ b/backend/tests/test_invoke_acp_agent_tool.py @@ -1061,6 +1061,53 @@ async def test_invoke_acp_agent_spawns_the_path_resolved_command(monkeypatch, tm assert result == "(no response)" +@pytest.mark.anyio +async def test_invoke_acp_agent_spawns_an_absolute_command_without_an_extension(monkeypatch, tmp_path): + """An absolute command has to reach its ``PATHEXT`` shim before the spawn. + + npm installs ``mcode``, ``mcode.cmd``, and ``mcode.ps1`` side by side; a + configured absolute path without the extension would spawn the shell + script and fail with ``WinError 193``. The resolver has to pick the + ``.cmd`` shim the same way it does for a bare name. + """ + import acp as acp_module + + from deerflow.config import paths as paths_module + + monkeypatch.setattr(paths_module, "get_paths", lambda: paths_module.Paths(base_dir=tmp_path)) + monkeypatch.setattr( + "deerflow.config.extensions_config.ExtensionsConfig.from_file", + classmethod(lambda cls: ExtensionsConfig(mcp_servers={}, skills={})), + ) + script_path = tmp_path / "test_acp_agent.py" + script_path.write_text(_ACP_AGENT_SCRIPT, encoding="utf-8") + phase_path = tmp_path / "phase.txt" + + shim_name = "deerflow-acp-probe" + shim = _write_launcher_shim(tmp_path / "npm", shim_name, f'"{sys.executable}" "{script_path}" "" 0 "{phase_path}"') + if sys.platform == "win32": + # npm also drops an extensionless shell script next to the .cmd shim. + (shim.parent / shim_name).write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + + captured: dict[str, object] = {} + real_spawn_agent_process = acp_module.spawn_agent_process + + @contextlib.asynccontextmanager + async def _spying_spawn_agent_process(client, cmd, *args, env=None, cwd=None): + captured["cmd"] = cmd + async with real_spawn_agent_process(client, cmd, *args, env=env, cwd=cwd) as (conn, proc): + yield conn, proc + + monkeypatch.setattr(acp_module, "spawn_agent_process", _spying_spawn_agent_process) + + configured = str(shim.parent / shim_name) + tool = build_invoke_acp_agent_tool({"probe": ACPAgentConfig(command=configured, description="Probe", timeout_seconds=30)}) + result = await asyncio.wait_for(tool.coroutine(agent="probe", prompt="do work"), timeout=20) + + assert Path(captured["cmd"]) == shim + assert result == "(no response)" + + @pytest.mark.anyio async def test_invoke_acp_agent_reports_guidance_for_an_unresolvable_command(monkeypatch, tmp_path): """A command PATH cannot resolve still reports the configured name. @@ -1092,8 +1139,8 @@ def test_agent_path_reads_the_configured_override_case_insensitively(): assert _agent_path({"Path": "/agent-bin"}) == "/agent-bin" -def test_resolve_agent_command_only_resolves_bare_names(monkeypatch, tmp_path): - """A configured path is left to the spawn, which runs in the ACP workspace.""" +def test_resolve_agent_command_leaves_relative_paths_to_the_spawn(monkeypatch, tmp_path): + """A relative configured path is left to the spawn, which runs in the ACP workspace.""" resolved_dir = tmp_path / "resolved" monkeypatch.setattr( "deerflow.tools.builtins.invoke_acp_agent_tool.shutil.which", @@ -1105,6 +1152,90 @@ def test_resolve_agent_command_only_resolves_bare_names(monkeypatch, tmp_path): assert _resolve_agent_command("./bin/agent") == "./bin/agent" +def test_resolve_agent_command_resolves_absolute_paths_through_the_lookup(monkeypatch, tmp_path): + """An absolute command keeps its directory but still goes through the lookup.""" + seen: list[tuple[str, str | None]] = [] + + def _which(command, path=None): + seen.append((command, path)) + return command + ".CMD" + + monkeypatch.setattr("deerflow.tools.builtins.invoke_acp_agent_tool.shutil.which", _which) + + absolute = str(tmp_path / "npm" / "mcode") + assert _resolve_agent_command(absolute, str(tmp_path / "agent-bin")) == absolute + ".CMD" + assert seen == [(absolute, str(tmp_path / "agent-bin"))] + + +def test_resolve_agent_command_keeps_an_absolute_lookup_result_as_returned(monkeypatch, tmp_path): + """An already-absolute lookup result must not be lexically rewritten. + + ``os.path.abspath`` collapses a ``..`` that the kernel resolves after a + symlink, which changes the file the spawn runs; the anchoring exists for + relative ``PATH`` results only. + """ + literal = str(tmp_path / "current" / ".." / "shared" / "agent") + monkeypatch.setattr( + "deerflow.tools.builtins.invoke_acp_agent_tool.shutil.which", + lambda command, path=None: command, + ) + + assert _resolve_agent_command(literal) == literal + + +@pytest.mark.skipif(sys.platform != "win32", reason="PATHEXT sibling resolution is Windows-only") +def test_resolve_agent_command_resolves_a_windows_path_without_an_extension(tmp_path): + """npm's extensionless sh script and its ``.cmd`` shim sit side by side. + + ``asyncio.create_subprocess_exec`` does not probe ``PATHEXT``, so spawning + the configured path runs the shell script and fails with ``WinError 193``; + the lookup has to pick the ``.cmd`` shim instead. + """ + npm_bin = tmp_path / "npm" + npm_bin.mkdir() + extensionless = npm_bin / "mcode" + extensionless.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + (npm_bin / "mcode.cmd").write_text("@echo off\r\necho ran\r\n", encoding="ascii") + + resolved = _resolve_agent_command(str(extensionless)) + + assert Path(resolved) == npm_bin / "mcode.CMD" + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX has no PATHEXT indirection to resolve") +def test_resolve_agent_command_keeps_an_absolute_posix_command(tmp_path): + """On POSIX the absolute command itself is the executable; resolution keeps it.""" + tool = tmp_path / "bin" / "mcode" + tool.parent.mkdir() + tool.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + tool.chmod(0o755) + + assert _resolve_agent_command(str(tool)) == str(tool) + + +@pytest.mark.skipif(sys.platform == "win32", reason="symlink-plus-parent-segment resolution is POSIX-specific") +def test_resolve_agent_command_keeps_a_posix_symlink_that_precedes_a_parent_segment(tmp_path): + """An absolute command is returned as configured, not lexically rewritten. + + With ``current -> releases/v1``, ``current/../shared/agent`` resolves to + ``releases/shared/agent`` for the kernel, while a lexical ``abspath`` + would collapse it to the nonexistent ``shared/agent`` beside the symlink + or to a different file that happens to live there. + """ + release = tmp_path / "releases" / "v1" + release.mkdir(parents=True) + tool = tmp_path / "releases" / "shared" / "agent" + tool.parent.mkdir(parents=True) + tool.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + tool.chmod(0o755) + current = tmp_path / "current" + current.symlink_to(release, target_is_directory=True) + + configured = str(current / ".." / "shared" / "agent") + + assert _resolve_agent_command(configured) == configured + + def test_resolve_agent_command_hands_the_effective_path_to_the_lookup(monkeypatch, tmp_path): """The agent's own PATH, not the Gateway's, is what the lookup consults.""" seen: list[str | None] = []