diff --git a/agent/tool_dispatch_helpers.py b/agent/tool_dispatch_helpers.py index 911cf5fe639b..07b2c2e65e79 100644 --- a/agent/tool_dispatch_helpers.py +++ b/agent/tool_dispatch_helpers.py @@ -102,7 +102,7 @@ def _is_mcp_tool_parallel_safe(tool_name: str) -> bool: return False -def _plan_tool_batch_segments(tool_calls) -> List[tuple]: +def _plan_tool_batch_segments(tool_calls, *, execution_cwd: Optional[Path] = None) -> List[tuple]: """Split a tool-call batch into ordered ``(kind, calls)`` segments. ``kind`` is ``"parallel"`` (a maximal contiguous run of parallel-safe @@ -173,7 +173,7 @@ def _plan_tool_batch_segments(tool_calls) -> List[tuple]: continue if tool_name in _PATH_SCOPED_TOOLS: - scoped_path = _extract_parallel_scope_path(tool_name, function_args) + scoped_path = _extract_parallel_scope_path(tool_name, function_args, execution_cwd=execution_cwd) if scoped_path is None: _add_sequential(tool_call) continue @@ -217,8 +217,34 @@ def _should_parallelize_tool_batch(tool_calls) -> bool: return len(segments) == 1 and segments[0][0] == "parallel" -def _extract_parallel_scope_path(tool_name: str, function_args: dict) -> Optional[Path]: - """Return the normalized file target for path-scoped tools.""" +def _canonical_path(raw_path: str, execution_cwd: Optional[Path] = None) -> Path: + """Return a canonical, OS-aware path for overlap detection. + + Uses ``os.path.realpath`` to resolve symlinks on existing path components + and ``os.path.normcase`` for case-insensitive platforms (Windows). + Falls back to ``Path.cwd()`` when *execution_cwd* is not supplied. + """ + expanded = Path(raw_path).expanduser() + base = execution_cwd if execution_cwd is not None else Path.cwd() + candidate = expanded if expanded.is_absolute() else base / expanded + # realpath resolves symlinks on path components that exist; for + # not-yet-created files it canonicalises as far as possible. + resolved = os.path.normcase(os.path.realpath(os.path.abspath(str(candidate)))) + return Path(resolved) + + +def _extract_parallel_scope_path( + tool_name: str, + function_args: dict, + execution_cwd: Optional[Path] = None, +) -> Optional[Path]: + """Return the canonical file target for path-scoped tools. + + *execution_cwd* should be the working directory that the tool will + actually use at runtime. When omitted the process cwd is used, + which may differ from the tool execution environment on some + platforms (e.g. WSL, sandboxed sub-processes). + """ if tool_name not in _PATH_SCOPED_TOOLS: return None @@ -226,16 +252,16 @@ def _extract_parallel_scope_path(tool_name: str, function_args: dict) -> Optiona if not isinstance(raw_path, str) or not raw_path.strip(): return None - expanded = Path(raw_path).expanduser() - if expanded.is_absolute(): - return Path(os.path.abspath(str(expanded))) - - # Avoid resolve(); the file may not exist yet. - return Path(os.path.abspath(str(Path.cwd() / expanded))) + return _canonical_path(raw_path, execution_cwd) def _paths_overlap(left: Path, right: Path) -> bool: - """Return True when two paths may refer to the same subtree.""" + """Return True when two paths may refer to the same subtree. + + Both *left* and *right* must already be canonical (as returned by + ``_extract_parallel_scope_path`` / ``_canonical_path``) so that + symlink aliases and case differences are already normalised. + """ left_parts = left.parts right_parts = right.parts if not left_parts or not right_parts: @@ -613,6 +639,7 @@ __all__ = [ "_is_destructive_command", "_plan_tool_batch_segments", "_should_parallelize_tool_batch", + "_canonical_path", "_extract_parallel_scope_path", "_paths_overlap", "_is_multimodal_tool_result", diff --git a/agent/tool_executor.py b/agent/tool_executor.py index a5871442b449..f740fedb79c6 100644 --- a/agent/tool_executor.py +++ b/agent/tool_executor.py @@ -14,6 +14,7 @@ from __future__ import annotations import concurrent.futures import json +from pathlib import Path import logging import os import random @@ -1764,7 +1765,9 @@ def execute_tool_calls_segmented(agent, assistant_message, messages: list, effec from types import SimpleNamespace if segments is None: - segments = _plan_tool_batch_segments(assistant_message.tool_calls) + _active_env = get_active_env(effective_task_id) + _exec_cwd = Path(_active_env.cwd) if _active_env is not None and _active_env.cwd else None + segments = _plan_tool_batch_segments(assistant_message.tool_calls, execution_cwd=_exec_cwd) for kind, calls in segments: segment_message = SimpleNamespace(tool_calls=list(calls)) diff --git a/run_agent.py b/run_agent.py index 993ff8032a66..2d687a78c186 100644 --- a/run_agent.py +++ b/run_agent.py @@ -139,7 +139,7 @@ from model_tools import ( handle_function_call, # noqa: F401 # re-exported for tests that mock.patch("run_agent.handle_function_call") check_toolset_requirements, # noqa: F401 # re-exported for tests that mock.patch("run_agent.check_toolset_requirements") ) -from tools.terminal_tool import cleanup_vm +from tools.terminal_tool import cleanup_vm, get_active_env from tools.interrupt import set_interrupt as _set_interrupt from tools.browser_tool import cleanup_browser @@ -5768,7 +5768,9 @@ class AIAgent: ) from agent.tool_dispatch_helpers import _plan_tool_batch_segments - segments = _plan_tool_batch_segments(tool_calls) + _active_env = get_active_env(effective_task_id) + _exec_cwd = Path(_active_env.cwd) if _active_env is not None and _active_env.cwd else None + segments = _plan_tool_batch_segments(tool_calls, execution_cwd=_exec_cwd) if len(segments) == 1: kind = segments[0][0] diff --git a/tests/run_agent/test_tool_batch_segmentation.py b/tests/run_agent/test_tool_batch_segmentation.py index 9cde31479d00..acd75317c28c 100644 --- a/tests/run_agent/test_tool_batch_segmentation.py +++ b/tests/run_agent/test_tool_batch_segmentation.py @@ -12,6 +12,7 @@ concurrently, barrier calls sequentially — while preserving: """ import json +import sys import threading import time import uuid @@ -398,3 +399,122 @@ class TestSegmentedDispatchIntegration: contents = [m["content"] for m in messages] hits = [c for c in contents if "focus on the tests" in c] assert len(hits) == 1 + + +class TestPathCanonicalization: + """Regression tests for _canonical_path / _extract_parallel_scope_path fixes. + + Verifies that symlink aliases, relative/absolute cwd mismatches, and + (on Windows) case-insensitive aliases are never placed in the same + parallel segment. + """ + + def test_relative_and_absolute_same_target_use_separate_segments(self, tmp_path): + """A relative path resolved against execution_cwd and an absolute path + pointing to the same file must be detected as overlapping.""" + from agent.tool_dispatch_helpers import ( + _canonical_path, + _paths_overlap, + ) + + target = tmp_path / "config.json" + target.touch() + + abs_path = _canonical_path(str(target)) + rel_path = _canonical_path("config.json", execution_cwd=tmp_path) + + assert _paths_overlap(abs_path, rel_path), ( + "Absolute and relative paths pointing to the same file must overlap" + ) + + def test_symlink_aliases_are_not_parallelized(self, tmp_path): + """A symlink alias and the real path must be detected as overlapping + so they are never placed in the same parallel segment.""" + import os + from agent.tool_dispatch_helpers import ( + _canonical_path, + _paths_overlap, + ) + + real_dir = tmp_path / "real" + real_dir.mkdir() + target = real_dir / "config.json" + target.touch() + + alias_dir = tmp_path / "alias" + alias_dir.symlink_to(real_dir) + + real_path = _canonical_path(str(target)) + alias_path = _canonical_path(str(alias_dir / "config.json")) + + assert _paths_overlap(real_path, alias_path), ( + "Symlink alias and real path must overlap — " + "they must not be parallelized" + ) + + def test_execution_cwd_used_over_process_cwd(self, tmp_path, monkeypatch): + """_extract_parallel_scope_path must use execution_cwd, not + process cwd, when resolving relative paths.""" + from agent.tool_dispatch_helpers import ( + _extract_parallel_scope_path, + _paths_overlap, + ) + + exec_cwd = tmp_path / "sub" + exec_cwd.mkdir() + (exec_cwd / "x.txt").touch() + + # Point process cwd somewhere else entirely. + monkeypatch.chdir(tmp_path) + + # With execution_cwd supplied the relative path resolves under exec_cwd. + path_with_cwd = _extract_parallel_scope_path( + "write_file", {"path": "x.txt"}, execution_cwd=exec_cwd + ) + # The absolute path under exec_cwd must match. + path_absolute = _extract_parallel_scope_path( + "write_file", {"path": str(exec_cwd / "x.txt")} + ) + + assert path_with_cwd is not None + assert path_absolute is not None + assert _paths_overlap(path_with_cwd, path_absolute), ( + "execution_cwd-relative path and absolute path must overlap; " + "process cwd must not be used when execution_cwd is provided" + ) + + def test_symlink_alias_nonexistent_write_target_overlap(self, tmp_path): + """Symlink parent + not-yet-created leaf file must still be detected + as overlapping — write_file targets may not exist at planning time.""" + import os + from agent.tool_dispatch_helpers import _canonical_path, _paths_overlap + + real_dir = tmp_path / "real" + real_dir.mkdir() + alias_dir = tmp_path / "alias" + alias_dir.symlink_to(real_dir) + + # Leaf file does NOT exist yet (write_file scenario). + real_target = _canonical_path(str(real_dir / "new.txt")) + alias_target = _canonical_path(str(alias_dir / "new.txt")) + + assert _paths_overlap(real_target, alias_target), ( + "Symlink parent + nonexistent leaf must overlap — " + "write_file targets are planned before they exist" + ) + + @pytest.mark.skipif( + sys.platform != "win32", + reason="normcase() case-folding only matters on Windows", + ) + def test_case_insensitive_paths_overlap_windows(self, tmp_path): + """On Windows, FILE.txt and file.txt are the same file — they must + be detected as overlapping after normcase() canonicalisation.""" + from agent.tool_dispatch_helpers import _canonical_path, _paths_overlap + + upper = _canonical_path(str(tmp_path / "FILE.txt"), execution_cwd=tmp_path) + lower = _canonical_path(str(tmp_path / "file.txt"), execution_cwd=tmp_path) + + assert _paths_overlap(upper, lower), ( + "Case-insensitive aliases must overlap on Windows" + )