-
Notifications
You must be signed in to change notification settings - Fork 162
fix(tools): refuse an ambiguous edit instead of changing the first match - #3396
fix(tools): refuse an ambiguous edit instead of changing the first match #3396kovtcharov-amd wants to merge 2 commits into
Conversation
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.
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.
kovtcharov-amd
commented
Sep 5, 2026
Now also carries the C++ convergence and the spec-doc updates, which were developed on a parallel branch (#3395, closed as a duplicate of this one).
The C++ file_edit replaced every occurrence where Python replaced the first — two different wrong answers to the same question. Both now refuse an ambiguous match, so a skill written against one toolbelt behaves the same on the other.
Verdict: Approve with suggestions
This closes a genuine footgun — silent first-match replacement on ambiguous old_content was the kind of bug that looks right in review and silently edits the wrong region. The fix is thorough: shared helper, unified contract, staleness liveness (stale rejection re-anchors so the retry goes through), and 66 tests covering every branch. C++ and Python now agree.
Two small things to clean up before or after merge:
🟢 The docstring for old_content in file-search-mixin.mdx ends mid-sentence — "include surrounding lines until it is" trails off without finishing the thought.
🟢 current_content_truncated and current_content_anchored_on appear in every rejection payload but are absent from the rejection shapes documented in both spec files. They're useful fields (a caller needs truncated to know whether the excerpt is complete); worth a one-line addition to each spec block.
🔍 Technical details
Truncated sentence — docs/spec/file-search-mixin.mdx
The diff at the old_content arg description reads:
old_content: Exact text to find and replace; must be unique in the
file — include surrounding lines until it is
new_content: Replacement text
The sentence stops at "until it is". The matching description in file-io-tools-mixin.mdx says "until it is unique" — that's the missing tail.
Undocumented rejection fields
apply_unique_replacement always includes current_content_truncated and current_content_anchored_on in the excerpt dict (_excerpt in file_edit.py:754-761). Neither appears in the rejection shape in docs/spec/file-io-tools-mixin.mdx (lines ~393-404) or docs/spec/file-search-mixin.mdx (lines ~451-462). A caller relying on the spec would not know truncated signals an incomplete excerpt.
Suggested addition to each spec block:
"current_content_truncated": bool, # True when excerpt was clipped at 4000 chars "current_content_anchored_on": str, # "match" | "closest_line" | "file_start"
No other issues. The singleton lock strategy, thread safety, TOCTOU handling, and the stale-rejection re-anchor are all correct.
Approve with suggestions
This fixes a genuinely nasty failure mode: an edit whose target text appeared twice used to change the first one and report success, so both the agent and the human reviewing the diff saw a plausible-looking change in the wrong place. Ambiguity is now a refusal that names every match, rejections hand back the surrounding text so a retry costs no extra read, and an edit against a file that moved is refused. All three edit tools route through one shared helper, so they can't drift apart again — that's the right shape for this fix.
Three things worth a pass before merge:
- A rejection can now flood the model's context. When the text to replace is short and common, the refusal lists every match. I measured a file with 5,000 matches producing a ~30 KB error payload — and the C++ version is worse, since it puts the whole line list inside the error message itself. The code already caps the per-match context at five locations; the line list should be capped the same way.
- Two tools that rewrite files don't tell the tracker they did. Replacing a function, or regenerating GAIA.md, leaves the ledger pointing at the old contents, so the next edit on that file is rejected as "changed on disk after it was read" — blaming an outside change that was actually the agent's own. It recovers on the retry, but it costs a turn and the message is misleading.
- The PR description contradicts the diff. The reviewer notes say converging the C++ tool is deliberately left out, but the second commit does exactly that — it goes from replacing all occurrences to refusing anything but a single match. That's a behaviour break for existing C++ callers, and a reviewer trusting the note would skip reviewing it. Worth correcting the description and calling the break out.
One process note: the diff bundle handed to this review contained only the four Python files; I pulled the full nine-file PR and reviewed the C++ and docs changes too.
Real-world evidence
The automated evidence stage failed before producing anything (evidence-bundle.md records the harness error, not "nothing to test"), and this runner has no pytest and no installed gaia, so the PR's test plan could not be replayed here. The shared helper is pure stdlib, so I exercised it directly against the PR's own sample file:
== ambiguous (2 matches) ==
updated: None (nothing written)
error: Ambiguous edit: old_content matches 2 locations in /tmp/sample.py (lines 5, 10) — nothing was
written, because there is no way to tell which one you meant. Extend old_content with enough ...
match_count: 2 match_lines: [5, 10]
== unique match ==
error: None | beta untouched: True | alpha changed: True
== not found ==
error: Content to replace not found in /tmp/sample.py — nothing was written. A whitespace-insensitive
match does exist, so the indentation, tabs-vs-spaces, or line endings in old_content differ ...
== staleness ==
stale: True | Edit rejected: /tmp/sample.py changed on disk after it was read — contents hashed
427e0efb621f when read and 374827d2bf41 now (15 -> 105 bytes). Nothing was written ...
retry after rejection -> error is None: True
== ambiguity report size, 5000 matches ==
matches: 5000 | len(error msg): 320 | len(json payload): 30157
len(match_lines): 5000 | len(matches): 5
The core contract behaves as advertised: ambiguity refuses and writes nothing, a unique match applies and leaves the neighbouring region alone, the whitespace hint fires, and a stale rejection does not livelock. The last block is the measurement behind the first finding above. The tool wrappers, the security guardrails around them, and the whole C++ side were not exercised — the verdict on those rests on static review alone.
🔍 Technical details
🟡 Ambiguity report is uncapped (src/gaia/agents/tools/file_edit.py:413)
shown correctly truncates the message at MAX_REPORTED_MATCHES (file_edit.py:400-402) and _describe_matches truncates the per-location context (:296), but match_lines returns the full list. Measured: 5,000 matches → 30,157-char payload. The module defines MAX_EXCERPT_CHARS precisely to stop a rejection flooding the context window; this path bypasses that intent.
"match_lines": line_numbers[:MAX_REPORTED_MATCHES],
"match_lines_truncated": len(line_numbers) > MAX_REPORTED_MATCHES,
The C++ side is the worse case — cpp/src/file_tools.cpp builds lineList from every offset and embeds it in the error string, and pushes every line into match_lines, so the same input yields roughly double. Apply the same cap there.
🟡 Two write paths skip record_write (src/gaia/agents/tools/file_io_tools.py:1125, :978)
replace_function and create_gaia_md write the file without recording it. Sequence that breaks: read_file(x) → record; replace_function(x, ...) → file changes, ledger stale; edit_file(x, ...) → rejected with "changed on disk after it was read". The rejection re-anchors so the retry succeeds, but the agent burns a turn on an error that blames an external mutation it caused itself.
with open(file_path, "w", encoding="utf-8") as f:
f.write(modified_content)
record_write(str(file_path), modified_content)
(same pattern at :978 for gaia_path / content)
🟡 C++ semantic break not in the description
cpp/src/file_tools.cpp drops the replace-all loop for single-match-or-reject, and cpp/tests/test_file_tools.cpp::FileEdit_BasicReplacement changes its expectation from replacements == 2 to 1. The PR body's reviewer note still reads "The C++ file_edit still replaces all occurrences ... converging it is left out of this PR". The convergence is the right call; the note is stale and hides a behaviour break from anyone consuming the C++ tool. Also worth noting: the C++ anchorLineFor uses a plain substring search for the anchor while Python uses fuzzy difflib matching, so excerpt placement differs between trees even though the three rules match — the doc claim "an agent behaves identically on either tree" is slightly stronger than the code.
🟢 Staleness guarantee is weaker than the docs state (docs/spec/file-io-tools-mixin.mdx)
Two caveats the spec doesn't mention: the rejection re-anchors, so an identical blind retry goes through (safe here only because old_content is re-validated against the new content — worth saying so); and FileStateTracker is a process-wide singleton, so in the UI/API server another session's read or write re-anchors the entry and the "changed since it was read" check silently no-ops for the first session. Neither is a regression, but "an edit against a file that changed since it was read is rejected" reads as an absolute.
🟢 Tracker reset lives only in the new test file (tests/unit/agents/test_file_edit_semantics.py:80)
The clean_tracker autouse fixture is correct, but the singleton spans the whole pytest process. Any other test that reads through a file tool and then mutates the file out-of-band now depends on execution order. Moving FileStateTracker.instance().clear() into an autouse fixture in tests/conftest.py makes that ordering-independent for the whole suite.
🟢 Every rejected edit writes a "denied" security audit entry
file_io_tools.py:360, :836 and file_tools.py:1422 all call path_validator.audit_write("edit", path, 0, "denied", ...) on any edit_error. audit_write logs denied at WARNING (src/gaia/security.py:707). A not-found or ambiguous old_content is a normal retryable tool error, not a security denial — routing it through the same channel as blocklist and allowlist rejections makes the audit log noisy and dilutes the entries that matter. Consider a distinct status, or skip the audit call for match-shape errors.
Strengths
- The single shared helper plus
test_no_edit_site_keeps_a_first_match_replace— a source-level assertion thatreplace(old_contentnever comes back — is a durable guard, not just a passing test. Parametrizing the whole behaviour table over all three tools is the right way to stop them re-diverging. test_stale_rejection_is_not_a_dead_endcatches the failure the naive version of this fix would ship: rejecting forever is as broken as clobbering. Both trees carry the same reasoning inline, and the C++ comment correctly explains whyfile_writestays strict wherefile_editre-anchors.- Docs were updated across both spec pages and the C++ header comment in the same change, with the rejection payload shape written down — that's the "update every doc that describes it" rule followed properly.
_anchor_lineguards the O(n·m)difflib.ratio()behindreal_quick_ratio/quick_ratioupper bounds, so the not-found path stays cheap on large files.
If the text an edit was replacing appeared more than once in a file, GAIA changed the first occurrence and reported success. The agent believed it had edited the region it asked for, and the human reviewed a diff that looked entirely plausible while touching somewhere else in the file. That is the failure mode least likely to be caught in review, because nothing about it looks wrong. Ambiguity is now an error that names 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 region the caller was aiming at, so a retry no longer costs a separate read just to find out why it failed. And an edit issued against a file that changed since the agent read it is refused rather than silently clobbering the newer contents.
Closes #3377.
Test plan
python -m pytest tests/unit/agents/test_file_edit_semantics.py -q— 66 tests covering ambiguity, uniqueness, not-found excerpts and stalenesspython -m pytest tests/unit/test_file_tools.py tests/unit/test_file_write_guardrails.py -q— no regression in the existing file-tool suitesold_contentmatching twice now returns an error naming both line numbers, and that a unique match still appliesNotes for the reviewer
file_io_tools,file_tools) now route through one shared helper, so they cannot drift apart again.edit_python_file— the third site with the same defect — goes through it too.FileStateTracker, deliberately keeping the same semantics across the two trees. The C++file_editstill replaces all occurrences, which is a third semantic; converging it is left out of this PR rather than bundled in.test_filesystem_index.py::TestScanDirectory::test_scan_incremental_skips_unchanged, fails on this branch and on a cleanmaincheckout alike; it is not from this change.