From a04edc613d30abec7d54ece02e0ab4e2a8e17440 Mon Sep 17 00:00:00 2001 From: Ovtcharov Date: Sat, 5 Sep 2026 01:51:15 -0700 Subject: [PATCH 1/2] fix(tools): refuse an ambiguous edit instead of changing the first match When old_content appeared more than once, edit_file replaced the first occurrence and reported success, so the agent believed it had changed the region it asked for and the human reviewed a plausible-looking diff that touched somewhere else. Ambiguity is now an error naming the match count and the line of each. Two related gaps close with it. A rejected edit now carries the file's current content around the target region, so a retry no longer costs a separate read to find out why. And an edit against a file that changed since the agent read it is rejected rather than clobbering the newer contents, using a content-hash ledger ported from the C++ FileStateTracker so both trees keep the same semantics. Both Python implementations now route through one shared helper, so they cannot drift apart again. --- src/gaia/agents/tools/file_edit.py | 425 ++++++++++++++++++ src/gaia/agents/tools/file_io_tools.py | 62 ++- src/gaia/agents/tools/file_tools.py | 42 +- tests/unit/agents/test_file_edit_semantics.py | 335 ++++++++++++++ 4 files changed, 832 insertions(+), 32 deletions(-) create mode 100644 src/gaia/agents/tools/file_edit.py create mode 100644 tests/unit/agents/test_file_edit_semantics.py diff --git a/src/gaia/agents/tools/file_edit.py b/src/gaia/agents/tools/file_edit.py new file mode 100644 index 000000000..d4d3ca5f6 --- /dev/null +++ b/src/gaia/agents/tools/file_edit.py @@ -0,0 +1,425 @@ +#!/usr/bin/env python +# Copyright(C) 2026 Advanced Micro Devices, Inc. All rights reserved. +# SPDX-License-Identifier: MIT +"""Shared match-and-replace semantics for GAIA's file-editing tools. + +Every ``edit_*`` tool routes its match-and-replace through +:func:`apply_unique_replacement`, so the separate implementations in +``file_io_tools`` and ``file_tools`` cannot drift apart. + +The contract: + +- ``old_content`` must match **exactly once**. Two matches is an error naming + the count and the line of each, not a first-match replacement — the model + cannot tell a wrong-region edit from the one it asked for, and neither can + the human reading the diff. +- A rejected edit carries the file's current content around the region the + caller was aiming at, so the retry lands without a separate re-read. +- An edit against a file that changed since the agent read it is rejected by + :class:`FileStateTracker` rather than clobbering the newer contents. + +``FileStateTracker`` is a port of the C++ tracker in +``cpp/include/gaia/file_tools.h``; the two trees keep the same ledger +semantics deliberately. +""" + +import difflib +import hashlib +import os +import re +import threading +from dataclasses import dataclass +from typing import Any, Dict, List, Optional, Tuple + +# Lines of surrounding context returned with a rejected edit. +CONTEXT_RADIUS = 12 + +# Lines of context shown per location in an ambiguity report. +MATCH_CONTEXT_RADIUS = 2 + +# Locations described individually before an ambiguity report stops listing them. +MAX_REPORTED_MATCHES = 5 + +# Ceiling on a returned excerpt, so a rejection cannot flood the context window. +MAX_EXCERPT_CHARS = 4000 + +# Similarity a line needs against the probe line to anchor a not-found excerpt. +_ANCHOR_THRESHOLD = 0.6 + +_SHORT_HASH_CHARS = 12 + + +def hash_content(content: str) -> str: + """Lowercase hex SHA-256 of ``content``.""" + return hashlib.sha256(content.encode("utf-8")).hexdigest() + + +def short_hash(content_hash: str) -> str: + """First ``_SHORT_HASH_CHARS`` of a hash, for human-readable messages.""" + return content_hash[:_SHORT_HASH_CHARS] + + +def _key(file_path: str) -> str: + """Ledger key: one entry per file however the caller spelled the path.""" + return os.path.normcase(os.path.realpath(str(file_path))) + + +@dataclass +class Divergence: + """Result of comparing a file's current contents to what was read.""" + + diverged: bool = False + hash_at_read: str = "" + hash_now: str = "" + size_at_read: int = 0 + size_now: int = 0 + reason: str = "" + + +@dataclass +class _Record: + content_hash: str + size: int + + +class FileStateTracker: + """Content-hash ledger of every file an agent has read. + + A later write can then tell "the model is editing what it saw" from "the + file moved under it". No system prompt can prevent a stale write: by the + time the model emits an edit, the read that justified it may be many turns + old and a build step, a formatter, another agent, or the user may have + changed the file since. + + Semantics (matching the C++ tracker): + + - A read records the SHA-256 of the file's **full** contents, so a + line-range read still anchors the whole file. + - An edit is rejected when a record exists and the contents now hash + differently. + - A file with **no** record is not blocked. Requiring a prior read would + make the tools unusable for creating files, and an agent that never read + the file has nothing stale to be wrong about. + - A successful edit re-records the new contents, so consecutive edits work + without an intervening read. + """ + + _instance = None + _instance_lock = threading.Lock() + + def __init__(self) -> None: + self._records: Dict[str, _Record] = {} + self._lock = threading.RLock() + + @classmethod + def instance(cls) -> "FileStateTracker": + """Process-wide tracker shared by every file tool.""" + with cls._instance_lock: + if cls._instance is None: + cls._instance = cls() + return cls._instance + + def record_read(self, file_path: str, content: str) -> str: + """Record the contents an agent has just seen. Returns the hash.""" + digest = hash_content(content) + with self._lock: + self._records[_key(file_path)] = _Record( + digest, len(content.encode("utf-8")) + ) + return digest + + def record_write(self, file_path: str, content: str) -> str: + """Record contents an agent has just written. + + Identical to :meth:`record_read` but named for the call site so intent + stays readable. + """ + return self.record_read(file_path, content) + + def check(self, file_path: str, current_content: str) -> Divergence: + """Compare ``current_content`` against the recorded read. + + Returns ``diverged=False`` when there is no record for the path. + """ + with self._lock: + record = self._records.get(_key(file_path)) + if record is None: + return Divergence() + + digest = hash_content(current_content) + size_now = len(current_content.encode("utf-8")) + if digest == record.content_hash: + return Divergence( + hash_at_read=short_hash(record.content_hash), + hash_now=short_hash(digest), + size_at_read=record.size, + size_now=size_now, + ) + + return Divergence( + diverged=True, + hash_at_read=short_hash(record.content_hash), + hash_now=short_hash(digest), + size_at_read=record.size, + size_now=size_now, + reason=( + f"contents hashed {short_hash(record.content_hash)} when read " + f"and {short_hash(digest)} now " + f"({record.size} -> {size_now} bytes)" + ), + ) + + def has_record(self, file_path: str) -> bool: + with self._lock: + return _key(file_path) in self._records + + def forget(self, file_path: str) -> None: + """Drop the record for a path (e.g. the file was deleted or renamed).""" + with self._lock: + self._records.pop(_key(file_path), None) + + def clear(self) -> None: + """Drop every record. Intended for tests and session resets.""" + with self._lock: + self._records.clear() + + def size(self) -> int: + with self._lock: + return len(self._records) + + +def record_read(file_path: str, content: str) -> str: + """Record a read against the process-wide tracker.""" + return FileStateTracker.instance().record_read(file_path, content) + + +def record_write(file_path: str, content: str) -> str: + """Record a write against the process-wide tracker.""" + return FileStateTracker.instance().record_write(file_path, content) + + +def _line_of(content: str, offset: int) -> int: + """1-based line number of a character offset.""" + return content.count("\n", 0, offset) + 1 + + +def _match_offsets(content: str, needle: str) -> List[int]: + offsets = [] + start = 0 + while True: + found = content.find(needle, start) + if found == -1: + return offsets + offsets.append(found) + start = found + len(needle) + + +def _collapse_whitespace(text: str) -> str: + return re.sub(r"\s+", " ", text).strip() + + +def _clip(text: str) -> Tuple[str, bool]: + if len(text) <= MAX_EXCERPT_CHARS: + return text, False + return text[:MAX_EXCERPT_CHARS], True + + +def _slice_lines(lines: List[str], center: int, radius: int) -> Tuple[str, int, int]: + """Excerpt around a 0-based line index. Returns (text, start_1based, end_1based).""" + start = max(0, center - radius) + end = min(len(lines), center + radius + 1) + return "\n".join(lines[start:end]), start + 1, end + + +def _anchor_line(lines: List[str], old_content: str) -> Optional[int]: + """0-based index of the line most like the first real line of ``old_content``.""" + probe = next((ln.strip() for ln in old_content.splitlines() if ln.strip()), "") + if not probe: + return None + + best_index: Optional[int] = None + best_score = _ANCHOR_THRESHOLD + matcher = difflib.SequenceMatcher(b=probe, autojunk=False) + for index, line in enumerate(lines): + stripped = line.strip() + if not stripped: + continue + matcher.set_seq1(stripped) + # real_quick_ratio/quick_ratio are cheap upper bounds — skip the O(n*m) + # ratio() for lines that cannot clear the threshold. + if matcher.real_quick_ratio() <= best_score: + continue + if matcher.quick_ratio() <= best_score: + continue + score = matcher.ratio() + if score> best_score: + best_score = score + best_index = index + return best_index + + +def _excerpt(current_content: str, old_content: str) -> Dict[str, Any]: + """Current content around the region the caller was most likely aiming at. + + Centres on ``old_content`` when it occurs, on the closest fuzzy line match + otherwise, and falls back to the head of the file when nothing resembles it. + """ + lines = current_content.splitlines() + total = len(lines) + + offsets = _match_offsets(current_content, old_content) if old_content else [] + if offsets: + center = _line_of(current_content, offsets[0]) - 1 + anchored_on = "match" + else: + anchor = _anchor_line(lines, old_content) + if anchor is None: + center, anchored_on = min(CONTEXT_RADIUS, total), "file_start" + else: + center, anchored_on = anchor, "closest_line" + + text, start, end = _slice_lines(lines, center, CONTEXT_RADIUS) + text, truncated = _clip(text) + return { + "current_content": text, + "current_content_start_line": start, + "current_content_end_line": end, + "current_content_total_lines": total, + "current_content_truncated": truncated, + "current_content_anchored_on": anchored_on, + } + + +def _describe_matches(current_content: str, offsets: List[int]) -> List[Dict[str, Any]]: + lines = current_content.splitlines() + described = [] + for offset in offsets[:MAX_REPORTED_MATCHES]: + line_no = _line_of(current_content, offset) + context, start, end = _slice_lines(lines, line_no - 1, MATCH_CONTEXT_RADIUS) + clipped, _ = _clip(context) + described.append( + { + "line": line_no, + "context": clipped, + "context_start_line": start, + "context_end_line": end, + } + ) + return described + + +def _error( + message: str, file_path: str, match_count: int, extra: Dict[str, Any] +) -> Dict[str, Any]: + payload = { + "status": "error", + "error": message, + "file_path": str(file_path), + "match_count": match_count, + } + payload.update(extra) + return payload + + +def apply_unique_replacement( + file_path: str, + current_content: str, + old_content: str, + new_content: str, +) -> Tuple[Optional[str], Optional[Dict[str, Any]]]: + """Replace the single occurrence of ``old_content``, or explain why not. + + Returns ``(updated_content, None)`` on success and ``(None, error)`` on + every rejection. The error dict always carries ``status``, ``error``, + ``file_path`` and ``match_count``; rejections that a retry could fix also + carry ``current_content`` and its line range, so the caller does not have + to re-read the file to try again. + + Callers own their own security checks (path allowlist, size limits, + backups) — this function only decides *what* the new contents should be. + """ + file_path = str(file_path) + + if not old_content: + return None, _error( + f"old_content is empty, so there is nothing to find in {file_path} — " + "nothing was written. Pass the exact text to replace, or use " + "write_file to replace the whole file.", + file_path, + 0, + {}, + ) + + divergence = FileStateTracker.instance().check(file_path, current_content) + if divergence.diverged: + error = _error( + f"Edit rejected: {file_path} changed on disk after it was read — " + f"{divergence.reason}. Nothing was written. The file's current " + "content is included as `current_content`; reissue the edit against " + "that, not against what you read earlier.", + file_path, + len(_match_offsets(current_content, old_content)), + { + "stale": True, + "hash_at_read": divergence.hash_at_read, + "hash_now": divergence.hash_now, + **_excerpt(current_content, old_content), + }, + ) + # Returning the content *is* a read, so re-anchor. Without this the + # ledger still holds the superseded hash and the corrected retry is + # rejected as stale too — forever. + FileStateTracker.instance().record_read(file_path, current_content) + return None, error + + offsets = _match_offsets(current_content, old_content) + + if not offsets: + hint = "" + if _collapse_whitespace(old_content) in _collapse_whitespace(current_content): + hint = ( + " A whitespace-insensitive match does exist, so the indentation, " + "tabs-vs-spaces, or line endings in old_content differ from the file." + ) + excerpt = _excerpt(current_content, old_content) + return None, _error( + f"Content to replace not found in {file_path} — nothing was written." + f"{hint} The file's current content around the closest region is " + f"included as `current_content` (lines " + f"{excerpt['current_content_start_line']}-" + f"{excerpt['current_content_end_line']} of " + f"{excerpt['current_content_total_lines']}); copy old_content " + "verbatim from it.", + file_path, + 0, + excerpt, + ) + + if len(offsets)> 1: + line_numbers = [_line_of(current_content, offset) for offset in offsets] + shown = ", ".join(str(n) for n in line_numbers[:MAX_REPORTED_MATCHES]) + if len(line_numbers)> MAX_REPORTED_MATCHES: + shown += ", ..." + return None, _error( + f"Ambiguous edit: old_content matches {len(offsets)} locations in " + f"{file_path} (lines {shown}) — nothing was written, because there " + "is no way to tell which one you meant. Extend old_content with " + "enough surrounding lines to match exactly one location, then " + "reissue the edit. The candidate locations are listed in `matches`.", + file_path, + len(offsets), + { + "ambiguous": True, + "match_lines": line_numbers, + "matches": _describe_matches(current_content, offsets), + **_excerpt(current_content, old_content), + }, + ) + + offset = offsets[0] + updated = ( + current_content[:offset] + + new_content + + current_content[offset + len(old_content) :] + ) + return updated, None diff --git a/src/gaia/agents/tools/file_io_tools.py b/src/gaia/agents/tools/file_io_tools.py index f5f60dc0d..dcd76d1c6 100644 --- a/src/gaia/agents/tools/file_io_tools.py +++ b/src/gaia/agents/tools/file_io_tools.py @@ -14,6 +14,11 @@ from typing import Any, Dict, Optional from gaia.agents.base.tools import tool +from gaia.agents.tools.file_edit import ( + apply_unique_replacement, + record_read, + record_write, +) class FileIOToolsMixin: @@ -77,6 +82,9 @@ def read_file(file_path: str) -> Dict[str, Any]: "size_bytes": len(content_bytes), } + # Anchor later edits to what the agent actually saw. + record_read(file_path, content) + # Detect file type by extension ext = os.path.splitext(file_path)[1].lower() @@ -249,6 +257,7 @@ def write_python_file( # Write the file with open(file_path, "w", encoding="utf-8") as f: f.write(content) + record_write(str(file_path), content) # Audit successful write if path_validator is not None: @@ -285,9 +294,13 @@ def edit_python_file( Includes security guardrails: path validation, blocked directory enforcement, sensitive file protection, size limits, backup creation, and audit logging. + old_content must match exactly one location. Zero or several matches + are errors that carry the file's current content, so a retry does not + need a separate read. + Args: file_path: Path to the file to edit - old_content: Content to find and replace + old_content: Content to find and replace; must be unique in the file new_content: New content to insert backup: Whether to create a backup dry_run: Whether to only simulate the edit @@ -338,15 +351,15 @@ def edit_python_file( with open(file_path, "r", encoding="utf-8") as f: current_content = f.read() - # Check if old content exists - if old_content not in current_content: - return { - "status": "error", - "error": "Content to replace not found in file", - } - - # Create new content - modified_content = current_content.replace(old_content, new_content, 1) + modified_content, edit_error = apply_unique_replacement( + str(file_path), current_content, old_content, new_content + ) + if edit_error is not None: + if path_validator is not None: + path_validator.audit_write( + "edit", str(file_path), 0, "denied", edit_error["error"] + ) + return edit_error # Validate new content (graceful degradation: stdlib ast if no mixin) if hasattr(self, "_validate_python_syntax"): @@ -395,6 +408,7 @@ def edit_python_file( # Write the modified content with open(file_path, "w", encoding="utf-8") as f: f.write(modified_content) + record_write(str(file_path), modified_content) # Audit successful edit if path_validator is not None: @@ -614,6 +628,7 @@ def write_markdown_file( # Write the file with open(file_path, "w", encoding="utf-8") as f: f.write(content) + record_write(str(file_path), content) # Audit successful write if path_validator is not None: @@ -693,6 +708,7 @@ def write_file( # Write content to file path.write_text(content, encoding="utf-8") + record_write(str(path), content) console = getattr(self, "console", None) if console: @@ -743,9 +759,14 @@ def edit_file( Includes security guardrails: path validation, blocked directory enforcement, sensitive file protection, backup creation, and audit logging. + old_content must match exactly one location. Zero or several matches + are errors that carry the file's current content, so a retry does not + need a separate read. + Args: file_path: Path to the file to edit - old_content: Exact content to find and replace + old_content: Exact content to find and replace; must be unique + in the file new_content: New content to replace with project_dir: Project root directory for resolving relative paths @@ -806,21 +827,21 @@ def edit_file( # Read current content current_content = path.read_text(encoding="utf-8") - # Check if old_content exists in file - if old_content not in current_content: - return { - "status": "error", - "error": f"Content to replace not found in {file_path}", - } + updated_content, edit_error = apply_unique_replacement( + str(path), current_content, old_content, new_content + ) + if edit_error is not None: + if path_validator is not None: + path_validator.audit_write( + "edit", str(path), 0, "denied", edit_error["error"] + ) + return edit_error # Backup before editing backup_path = None if path_validator is not None: backup_path = path_validator.create_backup(str(path)) - # Replace content - updated_content = current_content.replace(old_content, new_content, 1) - # Generate diff before writing diff = "\n".join( difflib.unified_diff( @@ -834,6 +855,7 @@ def edit_file( # Write updated content path.write_text(updated_content, encoding="utf-8") + record_write(str(path), updated_content) console = getattr(self, "console", None) if console: diff --git a/src/gaia/agents/tools/file_tools.py b/src/gaia/agents/tools/file_tools.py index d0acd79b6..b403a1f44 100644 --- a/src/gaia/agents/tools/file_tools.py +++ b/src/gaia/agents/tools/file_tools.py @@ -19,6 +19,12 @@ from pathlib import Path, PureWindowsPath from typing import Any, Dict, List +from gaia.agents.tools.file_edit import ( + apply_unique_replacement, + record_read, + record_write, +) + logger = logging.getLogger(__name__) @@ -672,6 +678,9 @@ def read_file(file_path: str) -> Dict[str, Any]: "size_bytes": len(content_bytes), } + # Anchor later edits to what the agent actually saw. + record_read(file_path, content) + # Detect file type by extension ext = os.path.splitext(file_path)[1].lower() @@ -1056,6 +1065,7 @@ def write_file( # Write the file with open(resolved_path, "w", encoding="utf-8") as f: f.write(content) + record_write(str(resolved_path), content) # Audit the successful write if path_validator is not None: @@ -1306,7 +1316,7 @@ def _parse_numeric(val) -> float: @tool( atomic=True, name="edit_file", - description="Edit a file by replacing specific content. Finds old_content in the file and replaces it with new_content. Creates a backup before editing.", + description="Edit a file by replacing specific content. old_content must match exactly one place in the file — if it matches more than one, the edit is rejected rather than guessing. Creates a backup before editing.", parameters={ "file_path": { "type": "str", @@ -1315,7 +1325,11 @@ def _parse_numeric(val) -> float: }, "old_content": { "type": "str", - "description": "Exact content to find and replace in the file", + "description": ( + "Exact content to find and replace. Must appear exactly " + "once in the file — include enough surrounding lines to " + "make it unique, or the edit is rejected as ambiguous." + ), "required": True, }, "new_content": { @@ -1334,6 +1348,10 @@ def edit_file( Similar to Claude Code's Edit tool — performs a partial string replacement rather than overwriting the entire file. Includes all security guardrails. + old_content must match exactly one location. Zero or several matches + are errors that carry the file's current content, so a retry does not + need a separate read. + Security checks performed: 1. Path allowlist validation (PathValidator) 2. Blocked directory enforcement @@ -1395,22 +1413,21 @@ def edit_file( # Read current content current_content = resolved_path.read_text(encoding="utf-8") - # Check if old_content exists in file - if old_content not in current_content: - return { - "status": "error", - "error": f"Content to replace not found in {resolved_path}", - "operation": "edit_file", - } + updated_content, edit_error = apply_unique_replacement( + str(resolved_path), current_content, old_content, new_content + ) + if edit_error is not None: + if path_validator is not None: + path_validator.audit_write( + "edit", str(resolved_path), 0, "denied", edit_error["error"] + ) + return {**edit_error, "operation": "edit_file"} # Create backup before editing backup_path = None if path_validator is not None: backup_path = path_validator.create_backup(str(resolved_path)) - # Replace content (first occurrence only) - updated_content = current_content.replace(old_content, new_content, 1) - # Generate diff for logging/display diff = "\n".join( difflib.unified_diff( @@ -1423,6 +1440,7 @@ def edit_file( # Write updated content resolved_path.write_text(updated_content, encoding="utf-8") + record_write(str(resolved_path), updated_content) # Audit the edit edit_size = len(updated_content.encode("utf-8")) diff --git a/tests/unit/agents/test_file_edit_semantics.py b/tests/unit/agents/test_file_edit_semantics.py new file mode 100644 index 000000000..095181e03 --- /dev/null +++ b/tests/unit/agents/test_file_edit_semantics.py @@ -0,0 +1,335 @@ +# Copyright(C) 2026 Advanced Micro Devices, Inc. All rights reserved. +# SPDX-License-Identifier: MIT + +""" +Tests for the shared match-and-replace semantics of GAIA's file-editing tools. + +Purpose: an ``old_content`` that matches more than one place in a file used to +be replaced at the *first* match and reported as a success, so the agent +believed it had changed one region while the diff touched another (#3377). +These tests pin the replacement contract for every ``edit_*`` tool: + +- exactly one match replaces; zero or several is an error and writes nothing +- a rejected edit carries the file's current content, so the retry needs no + extra read +- an edit against a file that changed since it was read is rejected + +The behaviour table is parametrized over all three implementations, so the +suite fails if any one of them drifts from the others. + +No LLM or external service required. +""" + +import importlib +from pathlib import Path +from unittest.mock import patch + +import pytest + +from gaia.agents.base.tools import _TOOL_REGISTRY +from gaia.agents.tools.file_edit import FileStateTracker, apply_unique_replacement + +# Valid Python (edit_python_file rejects edits that break the parse) with a +# body line that deliberately appears twice. +SAMPLE = '''"""Module.""" + + +def alpha(): + value = 1 + return value + + +def beta(): + value = 1 + return value +''' + +DUPLICATED = " value = 1" +UNIQUE_OLD = "def alpha():\n value = 1" +UNIQUE_NEW = "def alpha():\n value = 99" + +# (id, module, mixin class, registrar, tool name) +EDIT_TOOLS = [ + ( + "file_io_tools.edit_file", + "gaia.agents.tools.file_io_tools", + "FileIOToolsMixin", + "register_file_io_tools", + "edit_file", + ), + ( + "file_io_tools.edit_python_file", + "gaia.agents.tools.file_io_tools", + "FileIOToolsMixin", + "register_file_io_tools", + "edit_python_file", + ), + ( + "file_tools.edit_file", + "gaia.agents.tools.file_tools", + "FileSearchToolsMixin", + "register_file_search_tools", + "edit_file", + ), +] + +EDIT_TOOL_IDS = [entry[0] for entry in EDIT_TOOLS] + + +@pytest.fixture(autouse=True) +def clean_tracker(): + """The tracker is process-wide; no test may inherit another's ledger.""" + FileStateTracker.instance().clear() + yield + FileStateTracker.instance().clear() + + +@pytest.fixture(params=EDIT_TOOLS, ids=EDIT_TOOL_IDS) +def edit_tool(request): + """Every edit tool, behind one ``(path, old, new) -> dict`` signature. + + The two ``edit_file`` implementations register under the same name and + overwrite each other in the registry, so each is registered and captured + on its own. + """ + _, module_name, class_name, registrar, tool_name = request.param + module = importlib.import_module(module_name) + mixin = getattr(module, class_name)() + + saved = dict(_TOOL_REGISTRY) + try: + getattr(mixin, registrar)() + entry = _TOOL_REGISTRY.get(tool_name) + assert entry is not None, f"{tool_name} was not registered by {registrar}" + function = entry["function"] + + def call(path, old, new): + return function(str(path), old, new) + + call.module_name = module_name + call.tool_name = tool_name + yield call + finally: + _TOOL_REGISTRY.clear() + _TOOL_REGISTRY.update(saved) + + +@pytest.fixture +def sample_file(tmp_path) -> Path: + path = tmp_path / "sample.py" + path.write_text(SAMPLE, encoding="utf-8") + return path + + +# ============================================================================ +# 1. AMBIGUITY IS AN ERROR, NOT A FIRST-MATCH REPLACEMENT +# ============================================================================ + + +class TestAmbiguousOldContent: + """A non-unique old_content must be refused, with the count named.""" + + def test_ambiguous_edit_is_rejected(self, edit_tool, sample_file): + result = edit_tool(sample_file, DUPLICATED, " value = 2") + assert result["status"] == "error" + + def test_ambiguous_edit_writes_nothing(self, edit_tool, sample_file): + edit_tool(sample_file, DUPLICATED, " value = 2") + assert sample_file.read_text(encoding="utf-8") == SAMPLE + + def test_ambiguous_error_states_the_match_count(self, edit_tool, sample_file): + result = edit_tool(sample_file, DUPLICATED, " value = 2") + assert result["match_count"] == 2 + assert "2" in result["error"] + + def test_ambiguous_error_locates_every_match(self, edit_tool, sample_file): + result = edit_tool(sample_file, DUPLICATED, " value = 2") + # SAMPLE puts the duplicated line on lines 5 and 10. + assert result["match_lines"] == [5, 10] + assert [m["line"] for m in result["matches"]] == [5, 10] + + def test_ambiguous_error_says_how_to_retry(self, edit_tool, sample_file): + result = edit_tool(sample_file, DUPLICATED, " value = 2") + assert "surrounding lines" in result["error"] + + +# ============================================================================ +# 2. A REJECTED EDIT CARRIES CURRENT CONTENT +# ============================================================================ + + +class TestNotFoundCarriesContent: + """The retry must not need a separate re-read to succeed.""" + + def test_not_found_is_an_error_with_zero_matches(self, edit_tool, sample_file): + result = edit_tool(sample_file, "def gamma():", "def delta():") + assert result["status"] == "error" + assert result["match_count"] == 0 + + def test_not_found_error_carries_file_content(self, edit_tool, sample_file): + result = edit_tool(sample_file, "def gamma():", "def delta():") + excerpt = result["current_content"] + assert excerpt, "not-found error returned no current content" + assert excerpt in SAMPLE, "excerpt is not verbatim from the file" + + def test_not_found_error_gives_the_excerpt_line_range(self, edit_tool, sample_file): + result = edit_tool(sample_file, "def gamma():", "def delta():") + assert result["current_content_start_line"]>= 1 + assert ( + result["current_content_end_line"] <= result["current_content_total_lines"] + ) + + def test_whitespace_only_mismatch_is_called_out(self, edit_tool, sample_file): + """The commonest not-found cause deserves naming, not guessing.""" + result = edit_tool(sample_file, "def alpha():", "def gamma():") + assert "whitespace-insensitive" in result["error"] + + def test_empty_old_content_is_rejected(self, edit_tool, sample_file): + result = edit_tool(sample_file, "", "anything") + assert result["status"] == "error" + assert sample_file.read_text(encoding="utf-8") == SAMPLE + + +# ============================================================================ +# 3. STALENESS +# ============================================================================ + + +class TestStalenessRejection: + """An edit against a file that moved under the agent must not clobber it.""" + + def test_stale_edit_is_rejected(self, edit_tool, sample_file): + FileStateTracker.instance().record_read(str(sample_file), "something older") + result = edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert result["status"] == "error" + assert result["stale"] is True + + def test_stale_edit_writes_nothing(self, edit_tool, sample_file): + FileStateTracker.instance().record_read(str(sample_file), "something older") + edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert sample_file.read_text(encoding="utf-8") == SAMPLE + + def test_stale_rejection_carries_current_content(self, edit_tool, sample_file): + FileStateTracker.instance().record_read(str(sample_file), "something older") + result = edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert result["current_content"] in SAMPLE + + def test_stale_rejection_names_both_hashes(self, edit_tool, sample_file): + FileStateTracker.instance().record_read(str(sample_file), "something older") + result = edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert result["hash_at_read"] != result["hash_now"] + assert result["hash_at_read"] in result["error"] + + def test_matching_read_does_not_block_the_edit(self, edit_tool, sample_file): + FileStateTracker.instance().record_read(str(sample_file), SAMPLE) + result = edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert result["status"] == "success" + + def test_unread_file_is_not_blocked(self, edit_tool, sample_file): + """No record means nothing stale to be wrong about.""" + assert not FileStateTracker.instance().has_record(str(sample_file)) + result = edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert result["status"] == "success" + + def test_consecutive_edits_need_no_intervening_read(self, edit_tool, sample_file): + """A successful edit re-anchors the ledger to what it just wrote.""" + first = edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert first["status"] == "success" + second = edit_tool(sample_file, "def beta():", "def gamma():") + assert second["status"] == "success" + + def test_stale_rejection_is_not_a_dead_end(self, edit_tool, sample_file): + """Rejecting forever is as broken as clobbering. + + The rejection hands the current content back, so it counts as a read: + the corrected retry must go through instead of hitting the superseded + hash again. + """ + FileStateTracker.instance().record_read(str(sample_file), "something older") + rejected = edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert rejected["status"] == "error" + + retry = edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert retry["status"] == "success", "stale rejection livelocked the agent" + + +# ============================================================================ +# 4. THE HAPPY PATH STILL WORKS +# ============================================================================ + + +class TestUniqueReplacement: + def test_unique_old_content_is_replaced(self, edit_tool, sample_file): + result = edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert result["status"] == "success" + assert sample_file.read_text(encoding="utf-8") == SAMPLE.replace( + UNIQUE_OLD, UNIQUE_NEW + ) + + def test_only_the_matched_region_changes(self, edit_tool, sample_file): + edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + updated = sample_file.read_text(encoding="utf-8") + assert "def alpha():\n value = 99" in updated + assert "def beta():\n value = 1" in updated + + +# ============================================================================ +# 5. THE IMPLEMENTATIONS CANNOT DIVERGE +# ============================================================================ + + +class TestImplementationsCannotDiverge: + """Every edit site must decide *what* to replace in exactly one place. + + The behaviour classes above already run against all three tools; these + tests stop a future edit from quietly reintroducing a private + match-and-replace that passes those by accident. + """ + + def test_every_edit_tool_delegates_to_the_shared_helper( + self, edit_tool, sample_file + ): + target = f"{edit_tool.module_name}.apply_unique_replacement" + with patch(target, wraps=apply_unique_replacement) as spy: + edit_tool(sample_file, UNIQUE_OLD, UNIQUE_NEW) + assert spy.call_count == 1, ( + f"{edit_tool.tool_name} did not route its replacement through " + "apply_unique_replacement" + ) + + @pytest.mark.parametrize( + "module_file", + [ + "src/gaia/agents/tools/file_io_tools.py", + "src/gaia/agents/tools/file_tools.py", + ], + ) + def test_no_edit_site_keeps_a_first_match_replace(self, module_file): + """``.replace(old_content, ..., 1)`` is the bug; it must not come back.""" + repo_root = Path(__file__).resolve().parents[3] + source = (repo_root / module_file).read_text(encoding="utf-8") + assert ( + "replace(old_content" not in source + ), f"{module_file} still does its own old_content replacement" + + def test_the_three_tools_agree_on_the_error_shape(self, tmp_path): + """Same input, same keys — a caller can handle one shape, not three.""" + shapes = {} + for tool_id, module_name, class_name, registrar, tool_name in EDIT_TOOLS: + module = importlib.import_module(module_name) + mixin = getattr(module, class_name)() + saved = dict(_TOOL_REGISTRY) + try: + getattr(mixin, registrar)() + function = _TOOL_REGISTRY[tool_name]["function"] + path = tmp_path / f"{tool_id.replace('.', '_')}.py" + path.write_text(SAMPLE, encoding="utf-8") + result = function(str(path), DUPLICATED, " value = 2") + finally: + _TOOL_REGISTRY.clear() + _TOOL_REGISTRY.update(saved) + # ``operation`` is file_tools' own pre-existing extra key. + shapes[tool_id] = frozenset(result) - {"operation"} + + distinct = set(shapes.values()) + assert len(distinct) == 1, f"edit tools disagree on error keys: {shapes}" From 200d43d0fcb8ef9d8e7030fa969672c0721ba0f2 Mon Sep 17 00:00:00 2001 From: Ovtcharov Date: Sat, 5 Sep 2026 01:54:49 -0700 Subject: [PATCH 2/2] fix(tools): converge the C++ file_edit on the same uniqueness rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The C++ tool replaced every occurrence where Python replaced the first — two wrong answers to the same question. Both now refuse an ambiguous match, so the toolbelts agree and a skill written against one behaves the same on the other. Spec docs updated to describe the rule they now share. --- cpp/include/gaia/file_tools.h | 12 ++- cpp/src/file_tools.cpp | 149 ++++++++++++++++++++++++++---- cpp/tests/test_file_tools.cpp | 102 +++++++++++++++++++- docs/spec/file-io-tools-mixin.mdx | 44 ++++++++- docs/spec/file-search-mixin.mdx | 33 +++++-- 5 files changed, 304 insertions(+), 36 deletions(-) diff --git a/cpp/include/gaia/file_tools.h b/cpp/include/gaia/file_tools.h index 4cec962df..30b0ed3c5 100644 --- a/cpp/include/gaia/file_tools.h +++ b/cpp/include/gaia/file_tools.h @@ -158,11 +158,15 @@ class GAIA_API FileIOTools { /// file_edit: Surgical string replacement in a file. /// Args: {"path": string, "old_string": string, "new_string": string} - /// Returns: {"success": true, "path": string, "replacements": int, + /// Returns: {"success": true, "path": string, "replacements": 1, /// "content_hash": string} - /// Rejected with {"error": ..., "stale": true} when the file changed - /// since it was read; a non-matching old_string returns an actionable - /// error rather than reporting success on a no-op. + /// old_string must match exactly one place. Matching several is rejected + /// with {"error": ..., "ambiguous": true, "match_lines": [int]} — picking + /// one of them would edit a region the caller never named, and it would + /// look like a success. Matching nothing is likewise an error rather than + /// a reported no-op. Rejected with {"error": ..., "stale": true} when the + /// file changed since it was read. Every rejection carries + /// "current_content" and its line range so the retry needs no extra read. /// On error: {"error": string} static ToolInfo fileEdit(); diff --git a/cpp/src/file_tools.cpp b/cpp/src/file_tools.cpp index 8612d7c92..0e6a32cd6 100644 --- a/cpp/src/file_tools.cpp +++ b/cpp/src/file_tools.cpp @@ -404,6 +404,79 @@ json staleRejection(const std::string& path, }; } +/// 1-based line number of a character offset. +int lineOfOffset(const std::string& content, std::string::size_type offset) { + return 1 + static_cast( + std::count(content.begin(), + content.begin() + static_cast(offset), + '\n')); +} + +/// Every offset at which `needle` occurs, non-overlapping. +std::vector matchOffsets(const std::string& content, + const std::string& needle) { + std::vector offsets; + if (needle.empty()) return offsets; + for (std::string::size_type pos = content.find(needle); pos != std::string::npos; + pos = content.find(needle, pos + needle.size())) { + offsets.push_back(pos); + } + return offsets; +} + +/// Line the caller was most likely aiming at: the first non-blank line of +/// `oldStr`, matched ignoring indentation — a wrong indent is the commonest +/// reason a match fails, and the excerpt is useless if it lands on line 1 of a +/// thousand-line file. Falls back to the top when nothing resembles it. +int anchorLineFor(const std::string& content, const std::string& oldStr) { + std::istringstream probeStream(oldStr); + std::string line; + std::string probe; + while (std::getline(probeStream, line)) { + const auto first = line.find_first_not_of(" \t\r"); + if (first == std::string::npos) continue; + const auto last = line.find_last_not_of(" \t\r"); + probe = line.substr(first, last - first + 1); + break; + } + if (probe.empty()) return 1; + + const auto pos = content.find(probe); + return pos == std::string::npos ? 1 : lineOfOffset(content, pos); +} + +/// Lines around `centerLine` (1-based), so a rejected edit hands back the +/// current text instead of costing the model a second read to find it. +json excerptAround(const std::string& content, int centerLine) { + constexpr int kRadius = 12; + constexpr std::size_t kMaxChars = 4000; + + std::vector lines; + std::string line; + std::istringstream stream(content); + while (std::getline(stream, line)) lines.push_back(line); + + const int total = static_cast(lines.size()); + const int start = std::max(1, centerLine - kRadius); + const int end = std::min(total, centerLine + kRadius); + + std::string excerpt; + for (int i = start; i <= end; ++i) { + if (!excerpt.empty()) excerpt += "\n"; + excerpt += lines[static_cast(i - 1)]; + } + const bool truncated = excerpt.size()> kMaxChars; + if (truncated) excerpt.resize(kMaxChars); + + return json{ + {"current_content", excerpt}, + {"current_content_start_line", start}, + {"current_content_end_line", end}, + {"current_content_total_lines", total}, + {"current_content_truncated", truncated}, + }; +} + } // namespace // --------------------------------------------------------------------------- @@ -635,10 +708,11 @@ ToolInfo FileIOTools::fileEdit() { ToolInfo info; info.name = "file_edit"; info.description = - "Perform surgical string replacement in a file. Finds all occurrences " - "of old_string and replaces them with new_string. Rejected if the file " - "changed on disk after the last file_read, or if old_string does not " - "appear verbatim — in both cases the file is left untouched."; + "Perform surgical string replacement in a file. old_string must match " + "exactly one place — include enough surrounding lines to make it " + "unique. Rejected if it matches nowhere, if it matches more than once, " + "or if the file changed on disk after the last file_read — in every " + "case the file is left untouched."; info.policy = ToolPolicy::CONFIRM; info.parameters = { {"path", ToolParamType::STRING, /*required=*/true, @@ -674,19 +748,22 @@ json FileIOTools::doFileEdit(const json& args) { FileStateTracker& tracker = FileStateTracker::instance(); const auto divergence = tracker.check(path, content); if (divergence.diverged) { - return staleRejection(path, "file_edit", divergence); - } - - // Replace all occurrences - int replacements = 0; - std::string::size_type pos = 0; - while ((pos = content.find(oldStr, pos)) != std::string::npos) { - content.replace(pos, oldStr.size(), newStr); - pos += newStr.size(); - ++replacements; - } - - if (replacements == 0) { + json rejection = staleRejection(path, "file_edit", divergence); + rejection.update(excerptAround(content, anchorLineFor(content, oldStr))); + // Handing the content back is a read, so re-anchor — otherwise the + // ledger keeps the superseded hash and the corrected retry is + // rejected as stale too. file_write stays strict: it names no + // old_string, so a blind retry would clobber the newer contents. + tracker.recordRead(path, content); + return rejection; + } + + // old_string must identify exactly one place. Replacing the first of + // several edits the wrong region; replacing all of them edits regions + // the model never named. Both report success, so ambiguity is an error. + const auto offsets = matchOffsets(content, oldStr); + + if (offsets.empty()) { // A silent no-op is the failure mode this tool exists to avoid: // say what did not match and what to do about it. std::string hint; @@ -696,17 +773,49 @@ json FileIOTools::doFileEdit(const json& args) { "indentation, tabs-vs-spaces, or line endings in " "old_string differ from the file."; } - return json{ + json result = json{ {"error", "old_string not found in file: " + path + " — no replacement was made and the file is " "unchanged." + hint + - " Re-read the file with file_read and copy " - "old_string verbatim from its contents."}, + " The current content is included as " + "current_content; copy old_string verbatim " + "from it."}, + {"path", path}, + {"replacements", 0}, + }; + result.update(excerptAround(content, anchorLineFor(content, oldStr))); + return result; + } + + if (offsets.size()> 1) { + std::string lineList; + json matchLines = json::array(); + for (const auto offset : offsets) { + const int lineNo = lineOfOffset(content, offset); + matchLines.push_back(lineNo); + if (!lineList.empty()) lineList += ", "; + lineList += std::to_string(lineNo); + } + json result = json{ + {"error", "Ambiguous edit: old_string matches " + + std::to_string(offsets.size()) + " locations in " + + path + " (lines " + lineList + + ") — nothing was written, because there is no way " + "to tell which one you meant. Extend old_string " + "with enough surrounding lines to match exactly " + "one location, then reissue the edit."}, + {"ambiguous", true}, {"path", path}, {"replacements", 0}, + {"match_lines", matchLines}, }; + result.update(excerptAround(content, lineOfOffset(content, offsets[0]))); + return result; } + content.replace(offsets[0], oldStr.size(), newStr); + const int replacements = 1; + // Write back std::ofstream outFile(path, std::ios::binary); if (!outFile.is_open()) { diff --git a/cpp/tests/test_file_tools.cpp b/cpp/tests/test_file_tools.cpp index b00ab32d5..b4eb2a5f6 100644 --- a/cpp/tests/test_file_tools.cpp +++ b/cpp/tests/test_file_tools.cpp @@ -159,7 +159,7 @@ TEST_F(FileToolsTest, FileWrite_MissingContent) { // --------------------------------------------------------------------------- TEST_F(FileToolsTest, FileEdit_BasicReplacement) { - std::string path = writeFile("edit_me.txt", "foo bar baz foo"); + std::string path = writeFile("edit_me.txt", "foo bar baz"); ToolInfo tool = FileIOTools::fileEdit(); ASSERT_TRUE(tool.callback); @@ -167,10 +167,101 @@ TEST_F(FileToolsTest, FileEdit_BasicReplacement) { json result = tool.callback({{"path", path}, {"old_string", "foo"}, {"new_string", "qux"}}); EXPECT_FALSE(result.contains("error")); EXPECT_EQ(result["success"], true); - EXPECT_EQ(result["replacements"], 2); + EXPECT_EQ(result["replacements"], 1); EXPECT_EQ(result["path"], path); - EXPECT_EQ(readFile(path), "qux bar baz qux"); + EXPECT_EQ(readFile(path), "qux bar baz"); +} + +TEST_F(FileToolsTest, FileEdit_AmbiguousOldStringIsRejected) { + // Two matches: neither "edit the first" nor "edit both" is what the caller + // asked for, and both report success. Refuse instead (#3377). + std::string path = writeFile("ambiguous.txt", "foo bar baz foo"); + + json result = FileIOTools::fileEdit().callback( + {{"path", path}, {"old_string", "foo"}, {"new_string", "qux"}}); + + ASSERT_TRUE(result.contains("error")) << result.dump(); + EXPECT_EQ(result["ambiguous"], true); + EXPECT_EQ(result["replacements"], 0); + EXPECT_NE(result["error"].get().find("2 locations"), std::string::npos); + + // Untouched — an ambiguous edit must not be a partial edit. + EXPECT_EQ(readFile(path), "foo bar baz foo"); +} + +TEST_F(FileToolsTest, FileEdit_AmbiguityNamesEveryMatchingLine) { + std::string path = writeFile("ambiguous_lines.txt", "alpha\nvalue\nbeta\nvalue\ngamma\n"); + + json result = FileIOTools::fileEdit().callback( + {{"path", path}, {"old_string", "value"}, {"new_string", "other"}}); + + ASSERT_TRUE(result.contains("match_lines")) << result.dump(); + EXPECT_EQ(result["match_lines"], (json::array({2, 4}))); +} + +TEST_F(FileToolsTest, FileEdit_UniqueMatchStillWorksWithContext) { + // The documented recovery from an ambiguity error: add surrounding lines. + std::string path = writeFile("recover.txt", "alpha\nvalue\nbeta\nvalue\ngamma\n"); + + json result = FileIOTools::fileEdit().callback( + {{"path", path}, {"old_string", "alpha\nvalue"}, {"new_string", "alpha\nother"}}); + + ASSERT_FALSE(result.contains("error")) << result.dump(); + EXPECT_EQ(result["replacements"], 1); + EXPECT_EQ(readFile(path), "alpha\nother\nbeta\nvalue\ngamma\n"); +} + +TEST_F(FileToolsTest, FileEdit_StaleRejectionIsNotADeadEnd) { + // Rejecting forever is as broken as clobbering: the rejection hands the + // current content back, so it counts as a read and the retry goes through. + std::string path = writeFile("relock.txt", "alpha\nbeta\n"); + FileStateTracker::instance().recordRead(path, "something older"); + + json rejected = FileIOTools::fileEdit().callback( + {{"path", path}, {"old_string", "alpha"}, {"new_string", "gamma"}}); + ASSERT_TRUE(rejected.contains("error")) << rejected.dump(); + EXPECT_EQ(rejected["stale"], true); + EXPECT_TRUE(rejected.contains("current_content")); + + json retry = FileIOTools::fileEdit().callback( + {{"path", path}, {"old_string", "alpha"}, {"new_string", "gamma"}}); + ASSERT_FALSE(retry.contains("error")) << retry.dump(); + EXPECT_EQ(readFile(path), "gamma\nbeta\n"); +} + +TEST_F(FileToolsTest, FileEdit_RejectionCarriesCurrentContent) { + // A rejected edit must hand back the text, or the retry costs a re-read. + std::string path = writeFile("carry.txt", "alpha\nbeta\ngamma\n"); + + json result = FileIOTools::fileEdit().callback( + {{"path", path}, {"old_string", "nowhere"}, {"new_string", "x"}}); + + ASSERT_TRUE(result.contains("current_content")) << result.dump(); + EXPECT_NE(result["current_content"].get().find("alpha"), + std::string::npos); + EXPECT_EQ(result["current_content_total_lines"], 3); +} + +TEST_F(FileToolsTest, FileEdit_ExcerptFollowsTheIntendedRegion) { + // Getting a later line's indentation wrong is the commonest miss. The + // excerpt must land where the caller was aiming, not on line 1 of a long + // file — the anchor is the first line of old_string, matched without its + // own indentation. + std::string body; + for (int i = 1; i <= 200; ++i) body += "filler line " + std::to_string(i) + "\n"; + body += "def target_function():\n inner = 1\n"; + std::string path = writeFile("long.py", body); + + json result = FileIOTools::fileEdit().callback( + {{"path", path}, + {"old_string", "def target_function():\n inner = 1"}, // wrong indent + {"new_string", "def target_function():\n inner = 2"}}); + + ASSERT_TRUE(result.contains("error")) << result.dump(); + EXPECT_NE(result["current_content"].get().find("target_function"), + std::string::npos); + EXPECT_GT(result["current_content_start_line"].get(), 150); } TEST_F(FileToolsTest, FileEdit_StringNotFound) { @@ -545,7 +636,10 @@ TEST_F(FileToolsHardeningTest, FileEdit_NonMatchingOldStringIsActionable) { const std::string message = result["error"].get(); EXPECT_NE(message.find("not found"), std::string::npos); EXPECT_NE(message.find("no replacement was made"), std::string::npos); - EXPECT_NE(message.find("file_read"), std::string::npos); + // The error hands the text back rather than sending the model to re-read. + EXPECT_NE(message.find("current_content"), std::string::npos); + EXPECT_NE(result["current_content"].get().find("quick brown"), + std::string::npos); EXPECT_EQ(result["replacements"], 0); EXPECT_EQ(readFile(path), before); } diff --git a/docs/spec/file-io-tools-mixin.mdx b/docs/spec/file-io-tools-mixin.mdx index ee2d8b130..6f73cd426 100644 --- a/docs/spec/file-io-tools-mixin.mdx +++ b/docs/spec/file-io-tools-mixin.mdx @@ -89,7 +89,8 @@ Edit a Python file by replacing content with validation. **Parameters:** - `file_path` (str, required): Path to the file -- `old_content` (str, required): Content to find and replace +- `old_content` (str, required): Content to find and replace. Must match + exactly one location — see [Edit semantics](#edit-semantics). - `new_content` (str, required): New content to insert - `backup` (bool, optional): Create backup (default: True) - `dry_run` (bool, optional): Only simulate (default: False) @@ -181,7 +182,8 @@ Edit any file by replacing content without validation. **Parameters:** - `file_path` (str, required): Path to file -- `old_content` (str, required): Exact content to find and replace +- `old_content` (str, required): Exact content to find and replace. Must match + exactly one location — see [Edit semantics](#edit-semantics). - `new_content` (str, required): New content to replace with - `project_dir` (str, optional): Project root for resolving paths @@ -197,6 +199,44 @@ Edit any file by replacing content without validation. } ``` +### Edit semantics + +`edit_file` and `edit_python_file` share one match-and-replace implementation, +[`gaia.agents.tools.file_edit`](https://github.com/amd/gaia/blob/main/src/gaia/agents/tools/file_edit.py), +so the two cannot drift apart. Three rules: + +**`old_content` must match exactly one location.** Matching several is an +error, not a first-match replacement — picking one edits a region the caller +never named while reporting success, and the resulting diff looks plausible. +Extend `old_content` with surrounding lines until it is unique. + +**A rejection carries the file's current content**, so the retry does not need +a separate read. + +**An edit against a file that changed since it was read is rejected.** Reads and +writes record a content hash; a mismatch means the file moved under the agent. +The rejection hands back the current content and re-anchors, so the corrected +retry proceeds. A file with no recorded read is not blocked. + +**Rejection shape:** +```python +{ + "status": "error", + "error": str, # names the cause and the fix + "match_count": int, # 0 = not found,>1 = ambiguous + "match_lines": [int], # ambiguous only + "matches": [dict], # ambiguous only: line + surrounding context + "stale": bool, # changed-on-disk only + "current_content": str, # excerpt around the target region + "current_content_start_line": int, + "current_content_end_line": int, + "current_content_total_lines": int +} +``` + +The C++ `file_edit` tool (`cpp/src/file_tools.cpp`) follows the same three +rules, so an agent behaves identically on either tree. + ### 8. replace_function Replace a specific function in a Python file. diff --git a/docs/spec/file-search-mixin.mdx b/docs/spec/file-search-mixin.mdx index 7ea830075..8d455d042 100644 --- a/docs/spec/file-search-mixin.mdx +++ b/docs/spec/file-search-mixin.mdx @@ -230,15 +230,23 @@ class FileSearchToolsMixin: """ Edit a file by replacing old content with new content. - Partial string replacement (first occurrence only) rather than a full - overwrite — like Claude Code's Edit tool. Runs the same security - guardrails as write_file (PathValidator allowlist, blocked-dir and - sensitive-file checks, backup creation, audit logging). The target file - must already exist and contain old_content verbatim. + Partial string replacement rather than a full overwrite — like Claude + Code's Edit tool. Runs the same security guardrails as write_file + (PathValidator allowlist, blocked-dir and sensitive-file checks, backup + creation, audit logging). The target file must already exist. + + old_content must match exactly one location. Matching several is an + error, not a first-match replacement: picking one would edit a region + the caller never named while reporting success. Matching nothing, and + editing a file that changed since it was read, are errors too. Every + rejection carries the file's current content so the retry needs no + separate read. The semantics live in `gaia.agents.tools.file_edit`, + shared by every edit tool. Args: file_path: File to edit - old_content: Exact text to find and replace + old_content: Exact text to find and replace; must be unique in the + file — include surrounding lines until it is new_content: Replacement text Returns: @@ -250,6 +258,19 @@ class FileSearchToolsMixin: "diff": str, # unified diff "backup_path": str # only when a backup was created } + + On rejection: + { + "status": "error", + "error": str, # names the cause and the fix + "match_count": int, # 0 = not found,>1 = ambiguous + "match_lines": [int], # ambiguous only + "stale": bool, # changed-on-disk only + "current_content": str, # excerpt around the target region + "current_content_start_line": int, + "current_content_end_line": int, + "current_content_total_lines": int + } """ pass

AltStyle によって変換されたページ (->オリジナル) /