fix(filesystem): refuse edit_file edits with an empty oldText

[dwin-gharibi]

strings.Contains(s, "") is always true and strings.Replace(s, "", new, 1) inserts at offset
0, so an edit with an empty oldText silently prepended to the file — and edit_file reported
File edited successfully. Replaced 0 characters.

Closes #3925.

Two entry points, one sink

The project already knew this was a hazard: validateRepairedEdits (filesystem.go:414) exists
to catch it and its doc comment describes this exact failure. It was just wired into only one of
the two repair paths in ParseEditFileArgs — the double-serialized-edits branch had it, the
outer-JSON branch did not.

{"path":"target.txt","edits":[{"oldText":\"","newText":"INJECTED"}]}

tryRepairEditFileJSON drops the stray \, which closes the string early and leaves
"oldText":"" — precisely the "repair removed a load-bearing character" case the guard was
written for. Before this PR:

tool result = "File edited successfully. Replaced 0 characters"
file before = "line one\nline two\n"
file after  = "INJECTEDline one\nline two\n"

The same corruption placed inside a double-serialized edits string was already rejected,
which is what pinned the cause to the asymmetry rather than to the repair pass itself.

But fixing only the parser would have been incomplete. Well-formed JSON reaches the same sink
with no repair at all:

{"path":"target.txt","edits":[{"oldText":"","newText":"INJECTED"}]}
→ "File edited successfully. Replaced 0 characters", file prepended

So the fix is at the sink, with the parser tightened as defence in depth.

Changes

1. handleEditFile refuses an empty oldText — the actual fix, covering every entry point
regardless of how the arguments were parsed:

for i, edit := range args.Edits {
+	// strings.Contains always matches "" and strings.Replace would insert
+	// newText at offset 0, silently prepending to the file.
+	if edit.OldText == "" {
+		return tools.ResultError(fmt.Sprintf("Edit %d failed: oldText must not be empty", i+1)), nil
+	}
 	if !strings.Contains(modifiedContent, edit.OldText) {

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. There's a test pinning that.

2. ParseEditFileArgs now validates repairs once, for both paths. Rather than adding a
second validateRepairedEdits call — duplicating the very asymmetry that caused the bug — the
array-success early return is folded into an else branch so there is exactly one
validation point, gated on whether a repair actually ran:

if didRepair {
    if err := validateRepairedEdits(args.Edits); err != nil {
        return EditFileArgs{}, err
    }
}

The diff looks larger than it is: it is mostly one if err == nil { return } becoming
if err != nil { ... } and the block below it re-indenting.

Why the parser still accepts a well-formed empty oldText

This is the one deliberate asymmetry, and it is load-bearing.

ParseEditFileArgs is also the TUI's parser for partially-streamed tool arguments
(pkg/tui/components/tool/editfile/editfile.go:24-29). Mid-stream, an oldText that has not
finished arriving is a normal transient state. Rejecting it in the parser would break live
rendering of an in-flight edit_file call.

So the split is: the parser only distrusts payloads it had to repair; handleEditFile — which
runs solely on complete tool calls — is what refuses to apply an empty oldText. There is a
test pinning the streaming case so a future tightening of the parser can't silently regress the
TUI.

Tests

pkg/tools/builtin/filesystem/filesystem_test.go:

• TestFilesystemTool_EditFileRejectsEmptyOldText
  • single empty edit — error returned, file byte-identical
  • empty edit after a valid one leaves the file untouched — proves no partial write when a
    later edit is rejected
• Three cases added to the TestParseEditFileArgs table:
  • repair that empties oldText is rejected — outer payload (the regression)
  • repair that empties oldText is rejected — double-serialized payload (was already
    passing; kept as a control so the symmetry is asserted, not assumed)
  • well-formed empty oldText still parses — pins the streaming contract above

Written test-first. Confirmed each fails on unpatched code for the right reason: the
outer-payload case with An error is expected but got nil, and both handler subtests with
"File edited successfully. Replaced 0 characters" does not contain "oldText must not be empty".
The double-serialized control and the streaming case passed before the change as well as after —
which is what makes them controls.

All 24 pre-existing TestParseEditFileArgs cases still pass, including the repo's own
repair: rejected when inner repair yields an edit with empty oldText, so the restructure
preserved existing behaviour.

Verification

Toolchain go1.26.5, darwin/arm64.

Check	Result
go test ./pkg/tools/builtin/filesystem/...	ok
go test ./pkg/tui/components/tool/editfile/...	ok (the streaming consumer)
go test -race -count=1 ./pkg/tools/builtin/filesystem/	ok
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]

Verified and approved. The fix is correctly placed at the sink, and the tests genuinely
exercise the new path — I confirmed that by mutation testing rather than by reading.

Verification

Reverting filesystem.go to the merge-base (a2746bb64) while keeping the new tests
fails all three new assertions, for the right reasons:

--- FAIL TestParseEditFileArgs/repair_that_empties_oldText_is_rejected_(outer_payload)
        Error: An error is expected but got nil.
--- FAIL TestFilesystemTool_EditFileRejectsEmptyOldText/single_empty_edit
        Error: "File edited successfully. Replaced 0 characters" does not contain "oldText must not be empty"
--- FAIL .../empty_edit_after_a_valid_one_leaves_the_file_untouched

Each half of the change is independently covered — removing only the handler guard fails
exactly the two handler subtests; removing only the outer-path didRepair = true fails
exactly the outer-payload parse case; and forcing the parser to always validate fails
exactly the streaming-contract case. That last one matters: pkg/tui/components/tool/editfile's
own tests still pass under that mutation, so the streaming pin has to live where you put it.

The parser restructure is the riskiest part of the diff, so I checked it directly: I ran every
prefix (316 of them) of four realistic payloads — including one whose text carries {}/[],
and a double-serialized one — through ParseEditFileArgs on main and on this branch. Results
are byte-identical, and no prefix ever yields a parsed empty oldText, so the new gate never
fires on a mid-stream payload. The asymmetry described in the PR body holds up.

CI green on 02506ec (lint, build-and-test, windows-tests, license-check, build-image amd64+arm64).
The duplicate cancelled save-context run is concurrency cancellation, superseded 3s later by
a successful one. Merges cleanly with current main.

[should-fix] The same sink exists in pkg/acp/filesystem.go and is still vulnerable

pkg/acp/filesystem.go:283-289 runs the identical loop with no guard:

for i, edit := range args.Edits {
    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)
}

It does call filesystem.ParseEditFileArgs (line 256), so the repaired-payload vector is covered
by change #2 — but a well-formed empty oldText reaches it untouched. Driving the ACP handler
over a real AgentSideConnection on this branch's HEAD:

args    = {"path":"f.txt","edits":[{"oldText":"","newText":"INJECTED"}]}
result  = isError=false  "File edited successfully"
client written content = "INJECTEDline one\nline two\n"   (original "line one\nline two\n")

Pre-existing, so not blocking — but the PR body describes the handler guard as "covering every
entry point regardless of how the arguments were parsed", and that claim doesn't hold for the ACP
toolset. Same four lines fix it. Happy either way: here, or as an immediate follow-up.

Coordination with #3930

#3930 edits the same loop and the two branches do conflict (content conflict in both
filesystem.go and filesystem_test.go). Worth noting for whoever rebases second: the empty
check must stay before the occurrence count, because strings.Count(s, "") returns
len+1, so #3930 alone rejects the empty case with a misleading message:

Edit 1 failed: old text appears 19 times; include more surrounding context so it matches exactly once

Keeping this PR's explicit guard first preserves the actionable message.

Optional

• validateRepairedEdits' doc comment still justifies itself with "handleEditFile's
  strings.Replace would silently insert newText at the start of the file" — this PR makes that
  no longer true. The rationale is now purely "the repair removed a load-bearing character",
  which the new inline comment above the if didRepair block already states well.
• The two deliberately asymmetric layers (parser permits a well-formed empty oldText, handler
  refuses it) are each tested in isolation but never together. One test through editFileHandler
  with the raw payload {"path":...,"edits":[{"oldText":"","newText":"x"}]} would pin the
  end-to-end contract, so a future refactor can't accidentally satisfy both tests while breaking
  the composition.
• Adjacent, out of scope: an edits: [] call still performs a no-op write and reports
  "File edited successfully. Changes:\n" (and triggers post-edit commands). Same
  "success without meaningful work" family as this bug, if you want a follow-up.

[docker-agent]

Assessment: 🟢 APPROVE

[dwin-gharibi]

Done. @aheritier @docker-agent

[docker-agent]

Sorry @dwin-gharibi, I can only respond to Docker org members.