mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-21 16:04:41 +00:00
fix(agent): canonicalise paths in parallel-batch planner to prevent same-file concurrent mutation
_extract_parallel_scope_path used Path.cwd() (process cwd) instead of the tool's actual execution cwd, and os.path.abspath() instead of os.path.realpath(), so symlink aliases and relative/absolute path pairs that resolve to the same physical file were treated as distinct targets and placed in the same parallel segment. On case-insensitive platforms (Windows) os.path.normcase() was also absent, allowing Foo.txt and foo.txt to race. Changes: - agent/tool_dispatch_helpers.py: introduce _canonical_path(raw_path, execution_cwd) applying expanduser->abspath->realpath->normcase; thread execution_cwd through _extract_parallel_scope_path and _plan_tool_batch_segments - agent/tool_executor.py: pass get_active_env(effective_task_id).cwd as execution_cwd to _plan_tool_batch_segments; add pathlib.Path import - run_agent.py: pass active env cwd to _plan_tool_batch_segments at the second call site inside _execute_tool_calls - tests/run_agent/test_tool_batch_segmentation.py: add 5 regression tests covering relative/absolute same target, symlink alias, execution_cwd vs process cwd, symlink parent + nonexistent write target, and Windows case-insensitive alias (skipped on non-Windows) Fixes a file-corruption / lost-update race introduced by the mixed tool-batch segmentation feature (perf commit #64460).
This commit is contained in:
parent
b4e4b5a43e
commit
9a21d0e3f2
4 changed files with 166 additions and 14 deletions
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue