-
Notifications
You must be signed in to change notification settings - Fork 162
fix(tools): reject ambiguous edits instead of silently editing the first match - #3395
fix(tools): reject ambiguous edits instead of silently editing the first match #3395kovtcharov-amd wants to merge 2 commits into
Conversation
... the repo Nine design and reference docs were sitting untracked in the working copy, so the decisions they record were invisible to everyone else — including the skill-bound task execution design that the async-task and multi-slot-broker work is meant to build against. Alongside them sat a 45MB mailbox corpus, agent-run captures, and internal analysis, none of which were gitignored. `git add -A` would have committed a mailbox to a public repo. The ignore block that already quarantines private working reports now covers those classes too, and its pointer to where that material lives is corrected — the path it named has not existed for some time.
...rst match When the text an edit replaced appeared more than once in a file, the edit tools replaced the *first* occurrence and reported success. The agent believed it had changed the region it named; the user reviewed a diff that touched a different one. It is the failure mode least likely to be caught in review, because the diff is a real edit of the right shape in the wrong place. Three sites had the defect independently — a membership test proving *at least* one match, followed by `.replace(old, new, 1)` picking the first, with nothing in between establishing uniqueness: - file_io_tools.edit_file - file_io_tools.edit_python_file - file_tools.edit_file All three now route through one module, gaia.agents.tools.file_edit, so they cannot drift apart. old_content must match exactly one location; several is an error naming the count and the line of each. Two smaller problems in the same path: A not-found error said only "not found", so the agent spent a round trip re-reading the file and often guessed again. Every rejection now carries the current content around the region the caller was aiming at, anchored on the closest line when the text does not match at all. There was no staleness check, so an edit could land on a version the agent had never seen. FileStateTracker — ported from the C++ tracker in cpp/include/gaia/file_tools.h — records a content hash on read and write and rejects an edit against a file that changed since. The rejection hands the content back and re-anchors, so the corrected retry proceeds rather than livelocking against a superseded hash. The C++ tree converges on the same contract. Its file_edit replaced *all* occurrences — a third semantic, and a worse one: it corrupts every region the model did not name rather than just one. It now requires a unique match and carries current content on rejection too, so an agent behaves identically on either tree. No caller relies on first-match semantics: these tools are model-invoked, and a grep for edit_file(/edit_python_file( across src/, hub/ and tests/ returns no call sites, only registration and grant lists.
kovtcharov-amd
commented
Sep 5, 2026
Closing as a duplicate of #3396 — my fault for racing this branch rather than waiting for it. The implementation here was the better one and #3396 now carries all of it: the shared file_edit helper, both Python call sites, the 66-case test file, the C++ convergence, and the two spec-doc updates.
The reason to keep #3396 instead of this one is scope, not content. This branch was cut from a local commit that has not landed on main, so its diff against main also carries about 2,700 lines of unrelated plan and reference docs. #3396 is the same change with only the files it needs.
Nothing from this branch is lost.
Request changes
This fixes a genuinely nasty failure: an edit whose target text appeared twice silently changed the first one and reported success. Now a non-unique match is an error that says how many places matched and where, nothing is written, and a rejected edit hands back the file's current text so the retry doesn't cost a re-read. The implementation is shared across all three Python edit tools and the C++ tool, with a test table that runs against every one of them — it's careful work.
The one thing holding it up is proof rather than code. This changes the description and parameter text of a tool the flagship agent is handed, and it changes what the model gets back when an edit fails. That is exactly the class of change the project requires an agent eval run for, because the new behaviour only pays off if the model actually recovers from an ambiguity error instead of looping on it — no unit test can tell you that. The test plan lists unit and C++ suites only, and the automated evidence stage on this run errored out, so nothing exercised the real surface.
Two smaller things worth fixing while you're here, both on the C++ side: the stale-edit message still tells the model to go re-read the file, which is the round trip this PR set out to remove and now contradicts the content it ships alongside; and the ambiguity error lists every matching line with no cap, so on a file with many matches it crowds out the excerpt the model actually needs.
Real-world evidence
The evidence harness failed to run on this PR — evidence-bundle.md reports that the stage errored before producing anything (auth / install / rate-limit; see the run logs), so the changed surface was not exercised here. This verdict rests on static review plus the author's reported unit and gtest results only.
What's missing and would settle it: a gaia eval agent run against the file-editing category compared to the committed baseline, and one real agent transcript showing an ambiguous edit being rejected and the model recovering on the retry.
🔍 Technical details
🟡 Important
No agent eval for an LLM-affecting change. edit_file is in the flagship/chat tool bundles (hub/agents/chat/python/gaia_agent_chat/tool_bundles.py:115), and this PR rewrites its description and its old_content parameter description in the schema sent to Lemonade (src/gaia/agents/tools/file_tools.py:1319-1332), plus the docstrings on all three tools. CLAUDE.md lists "tool registration, tool docstrings, or the JSON tool schema sent to Lemonade" as requiring gaia eval agent against the relevant category with a baseline comparison before merge. The behavioural half matters more than the wording: a model that previously always got a success now has to read an ambiguity error, widen old_content, and reissue — and the failure mode if it can't is a retry loop, which is invisible to the unit suite.
🟢 Minor
C++ stale message contradicts the payload it now ships (cpp/src/file_tools.cpp:398). staleRejection still ends with "Re-read the file with file_read and reissue the change against the current contents", but doFileEdit now attaches current_content and re-anchors the ledger. The header comment says every rejection "carries current_content ... so the retry needs no extra read"; the message says the opposite. staleRejection is shared with file_write, which genuinely does need the re-read, so override the tail in the file_edit path rather than editing the shared string:
json rejection = staleRejection(path, "file_edit", divergence);
rejection["error"] =
std::string("file_edit rejected: ") + path +
" changed on disk after it was read — " + 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.";
rejection.update(excerptAround(content, anchorLineFor(content, oldStr)));
C++ ambiguity error has no match cap, and its excerpt budget outruns the tool-result budget (cpp/src/file_tools.cpp:790-812). Python caps the listed locations at MAX_REPORTED_MATCHES = 5 and appends ", ..."; the C++ branch enumerates every match. Meanwhile truncateToolResult (cpp/src/agent.cpp:1017) clips the serialised result at 4000 chars — head 2000 + tail 1500 — while excerptAround alone is allowed 4000. On a file with many matches, or with long lines, the error string and the middle of the excerpt are what get discarded, which is the part the retry depends on. Cap the line list the way Python does and drop kMaxChars to something that survives the 4000-char clip alongside the message.
excerpt.resize(kMaxChars) can split a multibyte UTF-8 character (cpp/src/file_tools.cpp in excerptAround). readWholeFile reads bytes, and nlohmann's default dump() at agent.cpp:1017 throws type_error.316 on invalid UTF-8. The same byte-cap pattern already exists in file_read, so this isn't new — but the excerpt makes it reachable from every rejection path, on any file with non-ASCII content. Trimming back to the last byte that isn't a continuation byte ((b & 0xC0) == 0x80) closes it.
The tracker is process-wide, not per-agent/session. FileStateTracker.instance() is a module singleton, so in the Agent UI server one session's successful write re-anchors the ledger for every other session. Agent B can then edit against contents it never saw, because A's write made the hash current. The module docstring's framing ("the model is editing what it saw") holds per process, not per agent — worth a line in the docstring so the next reader doesn't over-trust it.
Four new .mdx plans aren't in the docs nav — plans/email-eval-node-frontend, plans/email-inbox-cleanup, plans/email-triage-skill, plans/github-agent are absent from docs/docs.json (there's existing precedent, but CLAUDE.md asks for the nav entry). Related process note: ~2,500 lines of plan and reference docs unrelated to the fix ride along in this PR, which makes the diff harder to review than the change deserves.
Strengths
- One implementation, enforced. The three Python tools route through
apply_unique_replacement, and the test table doesn't just check behaviour —test_every_edit_tool_delegates_to_the_shared_helperandtest_no_edit_site_keeps_a_first_match_replacestop a future edit from quietly reintroducing a privatereplace(old_content, ..., 1). That's the difference between a fix and a fix that stays fixed. - The stale path is recoverable by construction. Re-anchoring on rejection (
file_edit.py:372,file_tools.cppindoFileEdit) turns what would be a permanent block into one wasted call, andtest_stale_rejection_is_not_a_dead_endpins it. Keepingfile_writestrict, since it names noold_string, is the right asymmetry. - The C++ tree converged on the stricter semantic rather than keeping replace-all, which was the worse of the two defects — and
FileEdit_ExcerptFollowsTheIntendedRegiontests the excerpt anchoring on a 200-line file rather than a toy one. - Rejection ordering is correct throughout: validation, then read, then match, then backup. No rejection path creates a backup, writes, or records a hash.
Closes #3377.
When the text an edit was replacing appeared more than once in a file, GAIA edited the first occurrence and reported success. The agent believed it had changed the thing it named, and the user reviewed a diff that touched a different region of the file. It is the failure mode least likely to be caught in review, because the diff is a real edit, of the right shape, in the wrong place. Now a non-unique match is an error that names how many places matched and where, and nothing is written. Two related costs go away with it: a failed edit hands back the file's current content instead of a bare "not found", so the retry no longer burns a round trip re-reading; and an edit issued against a file that changed since the agent read it is rejected rather than clobbering the newer version.
Threads
file_io_tools.edit_file,file_io_tools.edit_python_fileandfile_tools.edit_fileeach had the defect independently. They now sharegaia.agents.tools.file_edit, and the test table runs against all three so they cannot drift apart again.file_editreplaced all occurrences — worse than first-match, since it corrupts every region the model did not name. It now requires a unique match and carries current content on rejection, so an agent behaves the same on either tree.file_writestays strict, because it names noold_stringand a blind retry would clobber.No caller relies on first-match semantics. These tools are model-invoked; grepping
edit_file(/edit_python_file(acrosssrc/,hub/andtests/returns no call sites, only registration and grant lists.Test plan
python -m pytest tests/unit/agents/test_file_edit_semantics.py -q— 66 pass. Parametrized over all three tools: ambiguity errors and writes nothing, not-found carries verbatim content, stale rejection carries content and the retry then succeeds, unique match still replaces.apply_unique_replacementback toif old not in c: error; else c.replace(old, new, 1)and re-run — 42 of the 63 then-existing tests fail.python -m pytest tests/unit/ -q— no regressions. Against cleanmainas a baseline: 616 failed / 488 errors / 9621 passed → 614 / 488 / 9686. Pre-existing failures are environmental (no network in the sandbox; an editable install resolvinggaia_agent_emailto another worktree causes 24 collection errors, excluded from both runs).python -m pyteston every test touching the changed modules (test_file_tools,test_file_write_guardrails,test_builder_agent,test_tool_grants,test_confirmation_required_tools,test_chat_agent_integration,test_skill_binary_grants,test_starter_skills,test_mcp_tool_risk_classification) — 900 pass, 0 fail.cmake --build <dir> --target tests_mock && tests_mock.exe— 1068 pass, 0 fail.--gtest_filter='FileTools*'covers the new ambiguity, recovery, and excerpt-anchoring cases.black,isort,flake8,pylintclean on the changed files (util/lint.py --allcannot fetch its tools viauvxin this sandbox, so each was run from the local env with the same project config).