From 719e910acdc003d826247f15d296c30615ee4181 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Thu, 6 Aug 2026 16:42:08 +0330 Subject: [PATCH 1/5] fix(pkg/tools/builtin/filesystem/filesystem.go): fixing up the edit_file bug in docker agent filesystem --- pkg/tools/builtin/filesystem/filesystem.go | 50 ++++++++++++++-------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/pkg/tools/builtin/filesystem/filesystem.go b/pkg/tools/builtin/filesystem/filesystem.go index 5b8d1856d..24693f4bb 100644 --- a/pkg/tools/builtin/filesystem/filesystem.go +++ b/pkg/tools/builtin/filesystem/filesystem.go @@ -358,6 +358,7 @@ func ParseEditFileArgs(data []byte) (EditFileArgs, error) { Edits json.RawMessage `json:"edits"` } + didRepair := false if err := json.Unmarshal(data, &raw); err != nil { repaired, ok := tryRepairEditFileJSON(data) if !ok { @@ -367,6 +368,7 @@ func ParseEditFileArgs(data []byte) (EditFileArgs, error) { return EditFileArgs{}, fmt.Errorf("failed to parse edit_file arguments after repair: %w", err) } slog.Debug("Repaired malformed edit_file JSON arguments") + didRepair = true } args := EditFileArgs{Path: raw.Path} @@ -378,26 +380,33 @@ func ParseEditFileArgs(data []byte) (EditFileArgs, error) { } // Try parsing edits as an array first (normal case). - if err := json.Unmarshal(raw.Edits, &args.Edits); err == nil { - return args, nil - } - - // Try unwrapping a double-serialized JSON string. - var editsStr string - if err := json.Unmarshal(raw.Edits, &editsStr); err != nil { - return EditFileArgs{}, fmt.Errorf("edits field is neither an array nor a JSON string: %w", err) - } - if err := json.Unmarshal([]byte(editsStr), &args.Edits); err != nil { - // The inner payload can carry the same brace/bracket-counting - // mistakes as the outer JSON, so give it the same repair pass. - repaired, ok := tryRepairEditFileJSON([]byte(editsStr)) - if !ok { - return EditFileArgs{}, fmt.Errorf("failed to parse double-serialized edits string: %w", err) + if err := json.Unmarshal(raw.Edits, &args.Edits); err != nil { + // Try unwrapping a double-serialized JSON string. + var editsStr string + if err := json.Unmarshal(raw.Edits, &editsStr); err != nil { + return EditFileArgs{}, fmt.Errorf("edits field is neither an array nor a JSON string: %w", err) } - if err := json.Unmarshal(repaired, &args.Edits); err != nil { - return EditFileArgs{}, fmt.Errorf("failed to parse double-serialized edits string after repair: %w", err) + if err := json.Unmarshal([]byte(editsStr), &args.Edits); err != nil { + // The inner payload can carry the same brace/bracket-counting + // mistakes as the outer JSON, so give it the same repair pass. + repaired, ok := tryRepairEditFileJSON([]byte(editsStr)) + if !ok { + return EditFileArgs{}, fmt.Errorf("failed to parse double-serialized edits string: %w", err) + } + if err := json.Unmarshal(repaired, &args.Edits); err != nil { + return EditFileArgs{}, fmt.Errorf("failed to parse double-serialized edits string after repair: %w", err) + } + slog.Debug("Repaired malformed double-serialized edits payload") + didRepair = true } - slog.Debug("Repaired malformed double-serialized edits payload") + } + + // Validated once, for either repair path: a repair is only trustworthy if it + // removed a spurious character rather than a load-bearing one. Well-formed + // payloads are deliberately not second-guessed here — the TUI parses + // partially-streamed arguments with this function, where a not-yet-filled + // oldText is normal; handleEditFile is what refuses to apply it. + if didRepair { if err := validateRepairedEdits(args.Edits); err != nil { return EditFileArgs{}, err } @@ -1017,6 +1026,11 @@ func (t *ToolSet) handleEditFile(ctx context.Context, args EditFileArgs) (*tools var changes []string 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) { return tools.ResultError(fmt.Sprintf("Edit %d failed: old text not found", i+1)), nil } From 02506ecebafcd8153bd9d094568c68093a41a85c Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Thu, 6 Aug 2026 16:43:09 +0330 Subject: [PATCH 2/5] fix(pkg/tools/builtin/filesystem/filesystem_test.go): adding edge case tests for bug fix --- .../builtin/filesystem/filesystem_test.go | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/pkg/tools/builtin/filesystem/filesystem_test.go b/pkg/tools/builtin/filesystem/filesystem_test.go index c09e76ef1..9cb96e539 100644 --- a/pkg/tools/builtin/filesystem/filesystem_test.go +++ b/pkg/tools/builtin/filesystem/filesystem_test.go @@ -532,6 +532,58 @@ func TestFilesystemTool_EditFile(t *testing.T) { assert.Contains(t, result.Output, "old text not found") } +// An empty oldText is never a meaningful edit: strings.Contains(s, "") is always +// true and strings.Replace(s, "", new, 1) inserts at offset 0, so without a guard +// the file is silently prepended to and the tool still reports success. +func TestFilesystemTool_EditFileRejectsEmptyOldText(t *testing.T) { + t.Parallel() + + const original = "line one\nline two\n" + + t.Run("single empty edit", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "f.txt"), []byte(original), 0o644)) + + result, err := tool.handleEditFile(t.Context(), EditFileArgs{ + Path: "f.txt", + Edits: []Edit{{OldText: "", NewText: "INJECTED"}}, + }) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.Contains(t, result.Output, "oldText must not be empty") + + after, err := os.ReadFile(filepath.Join(tmpDir, "f.txt")) + require.NoError(t, err) + assert.Equal(t, original, string(after), "the file must be left untouched") + }) + + // The write happens once after every edit is applied in memory, so rejecting + // a later edit must not leave the earlier one persisted. + t.Run("empty edit after a valid one leaves the file untouched", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "f.txt"), []byte(original), 0o644)) + + result, err := tool.handleEditFile(t.Context(), EditFileArgs{ + Path: "f.txt", + Edits: []Edit{ + {OldText: "line one", NewText: "LINE ONE"}, + {OldText: "", NewText: "INJECTED"}, + }, + }) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.Contains(t, result.Output, "Edit 2") + + after, err := os.ReadFile(filepath.Join(tmpDir, "f.txt")) + require.NoError(t, err) + assert.Equal(t, original, string(after), "no edit may be persisted when a later one is rejected") + }) +} + func TestParseEditFileArgs(t *testing.T) { t.Parallel() @@ -690,6 +742,30 @@ func TestParseEditFileArgs(t *testing.T) { wantErr: true, wantErrMsg: "failed to parse edit_file arguments", }, + // Dropping the stray backslash closes the string early, leaving an empty + // oldText. That means the repair removed a load-bearing character, so the + // payload must be rejected — the double-serialized path already does this. + { + name: "repair that empties oldText is rejected (outer payload)", + input: `{"path": "target.txt", "edits": [{"oldText":\"", "newText": "INJECTED"}]}`, + wantErr: true, + wantErrMsg: "empty oldText", + }, + { + name: "repair that empties oldText is rejected (double-serialized payload)", + input: `{"path": "target.txt", "edits": "[{\"oldText\":\\\"\",\"newText\":\"INJECTED\"}]"}`, + wantErr: true, + wantErrMsg: "empty oldText", + }, + // Well-formed JSON is never second-guessed here: the TUI parses + // partially-streamed arguments with this function, where a not-yet-filled + // oldText is normal. handleEditFile is the layer that refuses to apply it. + { + name: "well-formed empty oldText still parses for streaming renderers", + input: `{"path": "target.txt", "edits": [{"oldText": "", "newText": "x"}]}`, + wantPath: "target.txt", + wantEdits: []Edit{{OldText: "", NewText: "x"}}, + }, } for _, tc := range tests { From 4f3cf383e87cc3a3a43ced9d4ce52be072496d20 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Fri, 7 Aug 2026 10:40:58 +0330 Subject: [PATCH 3/5] fix(acp): refuse edit_file edits with an empty oldText The ACP toolset serves the same edit_file tool name and schema as the built-in filesystem toolset, but its own edit loop had no empty-oldText guard: strings.Contains always matches "" and strings.Replace inserts at offset 0, so a well-formed empty oldText silently prepended to the file and the call reported success. Adds the same guard as the built-in handler, and drives a real AgentSideConnection to assert the refusal never reaches the client's fs/write_text_file. --- pkg/acp/filesystem.go | 7 ++ pkg/acp/filesystem_test.go | 133 +++++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+) diff --git a/pkg/acp/filesystem.go b/pkg/acp/filesystem.go index de9878427..618298d62 100644 --- a/pkg/acp/filesystem.go +++ b/pkg/acp/filesystem.go @@ -282,6 +282,13 @@ func (t *FilesystemToolset) handleEditFile(ctx context.Context, toolCall tools.T modifiedContent := resp.Content for i, edit := range args.Edits { + // strings.Contains always matches "" and strings.Replace would insert + // newText at offset 0, silently prepending to the file. Mirrors the + // guard in the built-in filesystem toolset, which serves the same + // edit_file tool name and schema over a different transport. + 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) { return tools.ResultError(fmt.Sprintf("Edit %d failed: old text not found", i+1)), nil } diff --git a/pkg/acp/filesystem_test.go b/pkg/acp/filesystem_test.go index 9a77f0253..adfaf73f3 100644 --- a/pkg/acp/filesystem_test.go +++ b/pkg/acp/filesystem_test.go @@ -1,6 +1,7 @@ package acp import ( + "context" "encoding/json" "fmt" "io" @@ -398,3 +399,135 @@ func TestFilesystemToolset_ReadFileRejectsInvalidRange(t *testing.T) { assert.Empty(t, responder.recordedRequests(), "invalid ranges must be rejected before any RPC") } + +// editFileResponder answers both fs/read_text_file and fs/write_text_file so an +// edit_file round trip can be driven over a real AgentSideConnection. Written +// content is recorded, which is what lets a test assert that a refused edit +// never reached the client. +type editFileResponder struct { + t *testing.T + peer io.Writer + content string + + mu sync.Mutex + written []string +} + +func (p *editFileResponder) Write(b []byte) (int, error) { + var msg struct { + ID json.RawMessage `json:"id"` + Method string `json:"method"` + Params json.RawMessage `json:"params"` + } + if err := json.Unmarshal(b, &msg); err != nil { + p.t.Errorf("peer received malformed JSON-RPC message %q: %v", b, err) + return 0, err + } + + var result any + switch msg.Method { + case acpsdk.ClientMethodFsReadTextFile: + result = acpsdk.ReadTextFileResponse{Content: p.content} + case acpsdk.ClientMethodFsWriteTextFile: + var req acpsdk.WriteTextFileRequest + if err := json.Unmarshal(msg.Params, &req); err != nil { + p.t.Errorf("peer failed to decode %s params: %v", msg.Method, err) + return 0, err + } + p.mu.Lock() + p.written = append(p.written, req.Content) + p.mu.Unlock() + result = acpsdk.WriteTextFileResponse{} + default: + err := fmt.Errorf("peer cannot answer JSON-RPC message %q (id %s)", msg.Method, msg.ID) + p.t.Error(err) + return 0, err + } + + response, err := json.Marshal(struct { + JSONRPC string `json:"jsonrpc"` + ID json.RawMessage `json:"id"` + Result any `json:"result"` + }{JSONRPC: "2.0", ID: msg.ID, Result: result}) + if err != nil { + return 0, fmt.Errorf("marshal response: %w", err) + } + if _, err := p.peer.Write(append(response, '\n')); err != nil { + return 0, err + } + return len(b), nil +} + +func (p *editFileResponder) writes() []string { + p.mu.Lock() + defer p.mu.Unlock() + return append([]string(nil), p.written...) +} + +// newEditFileFixture wires a FilesystemToolset to a real AgentSideConnection +// whose peer answers reads with content and records writes. +func newEditFileFixture(t *testing.T, content string) (*FilesystemToolset, context.Context, *editFileResponder) { + t.Helper() + + workingDir := t.TempDir() + const sessionID = "edit-session" + + acpAgent := &Agent{ + sessions: map[string]*Session{sessionID: {id: sessionID, workingDir: workingDir}}, + clientFS: acpsdk.FileSystemCapabilities{ReadTextFile: true, WriteTextFile: true}, + } + + peerReader, peerWriter := io.Pipe() + responder := &editFileResponder{t: t, peer: peerWriter, content: content} + conn := acpsdk.NewAgentSideConnection(acpAgent, responder, peerReader) + conn.SetLogger(slog.New(slog.DiscardHandler)) + acpAgent.SetAgentConnection(conn) + t.Cleanup(func() { + _ = peerWriter.Close() + select { + case <-conn.Done(): + case <-time.After(5 * time.Second): + t.Error("timed out waiting for ACP connection shutdown") + } + }) + + return NewFilesystemToolset(acpAgent, workingDir), withSessionID(t.Context(), sessionID), responder +} + +// The ACP toolset serves the same edit_file tool name and schema as the built-in +// filesystem toolset, so it must refuse an empty oldText for the same reason: +// strings.Contains always matches "" and strings.Replace would prepend. +func TestFilesystemToolset_EditFileRejectsEmptyOldText(t *testing.T) { + t.Parallel() + + const original = "line one\nline two\n" + ts, ctx, responder := newEditFileFixture(t, original) + + result, err := ts.handleEditFile(ctx, tools.ToolCall{ + Function: tools.FunctionCall{ + Name: filesystem.ToolNameEditFile, + Arguments: `{"path":"f.txt","edits":[{"oldText":"","newText":"INJECTED"}]}`, + }, + }, nil) + require.NoError(t, err) + assert.True(t, result.IsError, result.Output) + assert.Contains(t, result.Output, "oldText must not be empty") + assert.Empty(t, responder.writes(), "a refused edit must never reach the client") +} + +// A normal edit still works, so the guard is not over-broad. +func TestFilesystemToolset_EditFileAppliesNonEmptyEdit(t *testing.T) { + t.Parallel() + + ts, ctx, responder := newEditFileFixture(t, "line one\nline two\n") + + result, err := ts.handleEditFile(ctx, tools.ToolCall{ + Function: tools.FunctionCall{ + Name: filesystem.ToolNameEditFile, + Arguments: `{"path":"f.txt","edits":[{"oldText":"line one","newText":"LINE ONE"}]}`, + }, + }, nil) + require.NoError(t, err) + require.False(t, result.IsError, result.Output) + assert.Equal(t, []string{"LINE ONE\nline two\n"}, responder.writes()) +} From 6c566b61445f229a6721420d8bbb23e7b184de0b Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Fri, 7 Aug 2026 10:40:58 +0330 Subject: [PATCH 4/5] docs(filesystem): correct the validateRepairedEdits rationale The comment justified itself with "handleEditFile's strings.Replace would silently insert newText at the start of the file", which the handler guard made untrue. The real reason to reject is that an empty oldText after a repair means the repair removed a load-bearing character, so the payload is untrustworthy. --- pkg/tools/builtin/filesystem/filesystem.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/tools/builtin/filesystem/filesystem.go b/pkg/tools/builtin/filesystem/filesystem.go index 24693f4bb..1fab47cc9 100644 --- a/pkg/tools/builtin/filesystem/filesystem.go +++ b/pkg/tools/builtin/filesystem/filesystem.go @@ -416,10 +416,14 @@ func ParseEditFileArgs(data []byte) (EditFileArgs, error) { } // validateRepairedEdits guards against repair output that is structurally -// valid but semantically corrupted. An empty oldText is never a meaningful -// edit (handleEditFile's strings.Replace would silently insert newText at -// the start of the file), so its presence after a repair means the repair -// removed a load-bearing character rather than a spurious one. +// valid but semantically corrupted: an empty oldText after a repair means the +// repair removed a load-bearing character (a quote closing the string) rather +// than a spurious one, so the whole repaired payload is untrustworthy. +// +// This is about distrusting the repair, not about protecting the edit loop — +// handleEditFile refuses an empty oldText on its own. Rejecting here just +// reports the real problem at the parse boundary, where the message can say +// the payload was mis-repaired. func validateRepairedEdits(edits []Edit) error { for i, edit := range edits { if edit.OldText == "" { From aade3f449b9036f9ef0dbe28a6d3f964e034d88d Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Fri, 7 Aug 2026 10:40:58 +0330 Subject: [PATCH 5/5] test(filesystem): pin the empty-oldText parser/handler composition ParseEditFileArgs deliberately accepts a well-formed empty oldText because the TUI parses partially-streamed arguments with it, while handleEditFile refuses to apply one. Both halves were tested in isolation but never together, so a refactor could satisfy each test and still break how they combine. --- .../builtin/filesystem/filesystem_test.go | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/pkg/tools/builtin/filesystem/filesystem_test.go b/pkg/tools/builtin/filesystem/filesystem_test.go index 9cb96e539..1165e5bf5 100644 --- a/pkg/tools/builtin/filesystem/filesystem_test.go +++ b/pkg/tools/builtin/filesystem/filesystem_test.go @@ -1612,3 +1612,38 @@ func TestFilesystemTool_RootedListDirRefusesSymlinkSwap(t *testing.T) { require.Error(t, err, "rooted readDir must refuse a directory symlink that escapes the allow-list") } + +// The two layers are deliberately asymmetric: ParseEditFileArgs accepts a +// well-formed empty oldText (the TUI parses partially-streamed arguments with +// it, where a not-yet-filled oldText is normal) while handleEditFile refuses to +// apply one. Each half is tested in isolation elsewhere; this pins the +// composition end to end through the real tool handler, so a future refactor +// cannot satisfy both halves while breaking how they combine. +func TestFilesystemTool_EditFileHandlerRefusesWellFormedEmptyOldText(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + tool := New(tmpDir) + const original = "line one\nline two\n" + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "f.txt"), []byte(original), 0o644)) + + const payload = `{"path":"f.txt","edits":[{"oldText":"","newText":"INJECTED"}]}` + + // The parser accepts it: this is the streaming contract. + args, err := ParseEditFileArgs([]byte(payload)) + require.NoError(t, err, "the parser must not reject a well-formed payload") + require.Len(t, args.Edits, 1) + require.Empty(t, args.Edits[0].OldText) + + // The handler refuses to apply it, and the file is untouched. + result, err := tool.editFileHandler()(t.Context(), tools.ToolCall{ + Function: tools.FunctionCall{Name: ToolNameEditFile, Arguments: payload}, + }, nil) + require.NoError(t, err) + assert.True(t, result.IsError, result.Output) + assert.Contains(t, result.Output, "oldText must not be empty") + + after, err := os.ReadFile(filepath.Join(tmpDir, "f.txt")) + require.NoError(t, err) + assert.Equal(t, original, string(after)) +}