fix(filesystem): refuse an edit_file edit whose oldText matches more than once

[dwin-gharibi]

handleEditFile guarded against zero matches but not multiple. An oldText occurring twice
had its first occurrence rewritten and the call reported
File edited successfully. Replaced 16 characters — no count, no location, indistinguishable
from an unambiguous edit.

Closes #3929.

Before

conf.py:
  def dev():
      debug = True
  def prod():
      debug = True

edit: oldText="    debug = True"  newText="    debug = False"

occurrences = 2
tool reported: "File edited successfully. Replaced 16 characters"

file after:
  def dev():
      debug = False      <- changed
  def prod():
      debug = True       <- silently left alone

The model cannot tell whether it hit the site it meant. If it meant prod, the wrong code is
modified and the agent was told it succeeded — so it never re-reads to check.

The fix

strings.Count replaces strings.Contains, which folds the existing zero-match check into the
same scan (two scans become one):

switch n := strings.Count(modifiedContent, edit.OldText); {
case n == 0:
	return tools.ResultError(fmt.Sprintf("Edit %d failed: old text not found", i+1)), nil
case n > 1:
	return tools.ResultError(fmt.Sprintf(
		"Edit %d failed: old text appears %d times; include more surrounding context so it matches exactly once",
		i+1, n)), nil
}
modifiedContent = strings.Replace(modifiedContent, edit.OldText, edit.NewText, 1)

The error names the count and says what to do about it, so the model can retry with more context
instead of guessing.

Counted against the running content, not the original. This is the subtle part. A multi-edit
call can legitimately have an earlier edit remove one of the duplicates, leaving a later edit
unique — counting against the original file would reject that valid call. There's a dedicated
test for it.

Edits are applied to an in-memory string and written once after the loop, so rejecting edit N
leaves the file untouched, including edits 1..N-1. Also tested.

Tests

TestFilesystemTool_EditFileRejectsAmbiguousMatch, four subtests:

Subtest	Role
two occurrences are refused	the regression — error names the count, file byte-identical
a uniquely matching edit still applies	control — unambiguous edits must keep working
an earlier edit may resolve a later edit's ambiguity	control — pins counting against running content
an ambiguous later edit discards the earlier one	no partial write

Written test-first. The two regression subtests failed on unpatched code with
"File edited successfully. Replaced 16 characters" does not contain "appears 2 times"; the two
controls passed before the change as well as after — which is what makes them controls. The
earlier edit may resolve... one matters most: it fails if the count is taken against the
original content, which is the plausible wrong way to write this fix.

Contract change — what I checked

This changes behaviour for any caller depending on first-match-wins. I looked for such callers
rather than assuming there were none:

• No Go test asserts multi-occurrence edit behaviour.
• e2e/ and e2e/tui pass unchanged.
• The recorded cassettes mention oldText only inside the edit_file JSON schema sent to the
  model ("oldText":{"description":"Exact text to replace"}) — not as multi-occurrence edit
  calls.

Worth a maintainer opinion on the wording of the error, since it is what the model reads and acts
on. I went with naming the count plus an explicit instruction, on the theory that a bare
"ambiguous" would invite a retry of the same payload.

Verification

Toolchain go1.26.5, darwin/arm64.

Check	Result
go test ./pkg/tools/builtin/filesystem/...	ok
go test ./pkg/tui/components/tool/editfile/...	ok
go test -race -count=1 ./pkg/tools/builtin/filesystem/	ok
go test ./e2e/... (via full suite)	ok — e2e and e2e/tui both pass
go build ./...	clean
go vet ./pkg/tools/builtin/filesystem/	clean
gofmt -l pkg/tools/builtin/filesystem/	no output
go test ./... (full suite, .env.test loaded)	only pkg/teamloader fails — pre-existing

[dwin-gharibi]

@Sayt-0

[aheritier]

The diagnosis is right and the implementation is the correct shape: counting against the running content rather than the original is the subtle call, and it is the one that makes multi-edit calls behave sanely. The test suite is unusually good — I verified it rather than taking the description's word for it. Reverting only filesystem.go to the base commit while keeping the new tests fails exactly the two regression subtests ("File edited successfully. Replaced 16 characters" does not contain "appears 2 times", plus the partial-write assertion) and leaves both controls passing. The an earlier edit may resolve a later edit's ambiguity control also has real teeth: changing strings.Count(modifiedContent, …) to strings.Count(originalContent, …) at filesystem.go:1024 fails that subtest and only that subtest. Local go test -race, go vet and gofmt are clean, and CI is green on c3742a90.

Four things I would like addressed before merge. None of them is a defect in the logic you wrote.

[should-fix] The same edit loop in the ACP toolset was not updated, so edit_file now means two different things depending on transport.
pkg/acp/filesystem.go:66-67 swaps in an ACP-backed handler for the same tool name, with the same schema and description registered at filesystem.go:513-528 — so the model sees one contract. But that handler still carries the pre-fix loop:

// pkg/acp/filesystem.go:285-288
if !strings.Contains(modifiedContent, edit.OldText) {
    return tools.ResultError(fmt.Sprintf("Edit %d failed: old text not found", i+1)), nil
}
modifiedContent = strings.Replace(modifiedContent, edit.OldText, edit.NewText, 1)

versus the fixed site at pkg/tools/builtin/filesystem/filesystem.go:1024-1032. Under ACP, the exact scenario from your PR description still silently rewrites the first debug = True and reports success. This is pre-existing duplication rather than something the PR introduced, but the whole argument for the fix ("the model cannot tell whether it hit the site it meant") applies verbatim there. Ideally the count-and-refuse step becomes one shared helper called from both loops, so the next change to these semantics cannot drift again.

[should-fix] The error text misdirects the model in the "replace every occurrence" case, which used to work.
Repeating an identical edit is the only way this schema can express "change both occurrences", and it worked before. Measured on main vs. this branch, same input (a = 1\nb = 2\na = 1\n, edits [{a = 1 → a = 9}, {a = 1 → a = 9}]):

main:        isError=false  "Changes:\nEdit 1: Replaced 5 characters\nEdit 2: Replaced 5 characters"  file="a = 9\nb = 2\na = 9\n"
this branch: isError=true   "Edit 1 failed: old text appears 2 times; include more surrounding context…"  file unchanged

Refusing is defensible — the model should disambiguate — but "include more surrounding context so it matches exactly once" is unhelpful advice for a model whose intent was both sites: it reads as "your text is wrong" rather than "send one edit per occurrence". A clause such as …matches exactly once (send one edit per occurrence to change several) keeps the strictness and removes the wasted retry. Worth a test pinning that multi-occurrence intent is still expressible via distinct context-extended edits.

[should-fix] An empty oldText now produces a nonsensical count, and this collides with #3926.
strings.Count(s, "") returns rune count + 1, so on this branch:

file "hello world\n", edits [{oldText:"", newText:"XX"}]
  -> isError=true  "Edit 1 failed: old text appears 13 times; include more surrounding context so it matches exactly once"
  (on main: isError=false, file becomes "XXhello world\n")

Refusing is an improvement over the silent prepend, but "appears 13 times" is not something a model can act on, and the suggested remedy is impossible to satisfy. #3926 fixes precisely this input, and the two branches conflict in both files (git merge of the two heads: CONFLICT (content) in filesystem.go and filesystem_test.go). Whichever lands first, please rebase the other and make sure an empty oldText ends up with its own explicit message rather than falling into the n > 1 arm.

[should-fix] The precondition is now enforced but never advertised, so the model only discovers it by failing.
pkg/tools/builtin/filesystem/filesystem.go:340 still reads OldText string `json:"oldText" jsonschema:"Exact text to replace"` and the tool description at line 520 is "Make line-based edits to a text file. Each edit replaces exact line sequences with new content." Neither mentions that a match must be unique, so every ambiguous edit costs a full round trip that a one-line schema description ("…must match exactly once; include surrounding context to disambiguate") would avoid. Flagging the cost honestly: pkg/fake/proxy.go:301-341 matches cassettes on the whole normalized request body, and the current text appears in 5 cassettes (TestExec_{OpenAI,Anthropic,Gemini,Mistral}_ToolCall, TestExec_OpenAI_HideToolCalls), so touching the description means re-recording those — your call whether that belongs here or in a follow-up, but the contract text and the enforcement should not stay out of sync for long.

[optional] During confirmation the TUI previews the edit with newContent = strings.Replace(oldContent, oldText, newText, 1) (pkg/tui/components/tool/editfile/render.go:213), so for an ambiguous edit the user is now shown, and asked to approve, a first-occurrence diff that the tool will then refuse. Harmless, but the preview could reuse the same count check to show the refusal instead.