diff --git a/pkg/acp/filesystem.go b/pkg/acp/filesystem.go index de9878427c..618298d62c 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 9a77f0253d..adfaf73f35 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()) +} diff --git a/pkg/tools/builtin/filesystem/filesystem.go b/pkg/tools/builtin/filesystem/filesystem.go index 5b8d1856dc..1fab47cc94 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 } @@ -407,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 == "" { @@ -1017,6 +1030,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 } diff --git a/pkg/tools/builtin/filesystem/filesystem_test.go b/pkg/tools/builtin/filesystem/filesystem_test.go index c09e76ef1f..1165e5bf5f 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 { @@ -1536,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)) +}