Skip to content

Navigation Menu

Sign in
Sign up

fix(tools): refuse an ambiguous edit instead of changing the first match #3396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kovtcharov-amd wants to merge 2 commits into main
base: main
Choose a base branch
Loading
from fix/ambiguous-edit-first-match
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions cpp/include/gaia/file_tools.h
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
149 changes: 129 additions & 20 deletions cpp/src/file_tools.cpp
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<int>(
std::count(content.begin(),
content.begin() + static_cast<std::ptrdiff_t>(offset),
'\n'));
}

/// Every offset at which `needle` occurs, non-overlapping.
std::vector<std::string::size_type> matchOffsets(const std::string& content,
const std::string& needle) {
std::vector<std::string::size_type> 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<std::string> lines;
std::string line;
std::istringstream stream(content);
while (std::getline(stream, line)) lines.push_back(line);

const int total = static_cast<int>(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<std::size_t>(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

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -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()) {
Expand Down
102 changes: 98 additions & 4 deletions cpp/tests/test_file_tools.cpp
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,18 +159,109 @@ 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);

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<std::string>().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<std::string>().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<std::string>().find("target_function"),
std::string::npos);
EXPECT_GT(result["current_content_start_line"].get<int>(), 150);
}

TEST_F(FileToolsTest, FileEdit_StringNotFound) {
Expand Down Expand Up @@ -545,7 +636,10 @@ TEST_F(FileToolsHardeningTest, FileEdit_NonMatchingOldStringIsActionable) {
const std::string message = result["error"].get<std::string>();
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<std::string>().find("quick brown"),
std::string::npos);
EXPECT_EQ(result["replacements"], 0);
EXPECT_EQ(readFile(path), before);
}
Expand Down
44 changes: 42 additions & 2 deletions docs/spec/file-io-tools-mixin.mdx
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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

Expand All @@ -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.
Expand Down
Loading
Loading

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