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

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
 		}

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())
+}

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
 		}

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))
+}