Merge pull request #2857 from afjcjsbx/feat/edit-file-diff-preview

feat(tools): show unified diff for edit_file edits

pkg/tools/fs/edit.go

@@ -69,10 +69,11 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe
 		return ErrorResult("new_text is required")
 	}
 
-	if err := editFile(t.fs, path, oldText, newText); err != nil {
+	beforeContent, afterContent, err := editFile(t.fs, path, oldText, newText)
+	if err != nil {
 		return ErrorResult(err.Error())
 	}
-	return SilentResult(fmt.Sprintf("File edited: %s", path))
+	return DiffResult(path, beforeContent, afterContent)
 }
 
 type AppendFileTool struct {
@@ -131,18 +132,22 @@ func (t *AppendFileTool) Execute(ctx context.Context, args map[string]any) *Tool
 
 // editFile reads the file via sysFs, performs the replacement, and writes back.
 // It uses a fileSystem interface, allowing the same logic for both restricted and unrestricted modes.
-func editFile(sysFs fileSystem, path, oldText, newText string) error {
+func editFile(sysFs fileSystem, path, oldText, newText string) ([]byte, []byte, error) {
 	content, err := sysFs.ReadFile(path)
 	if err != nil {
-		return err
+		return nil, nil, err
 	}
 
 	newContent, err := replaceEditContent(content, oldText, newText)
 	if err != nil {
-		return err
+		return nil, nil, err
 	}
 
-	return sysFs.WriteFile(path, newContent)
+	if err := sysFs.WriteFile(path, newContent); err != nil {
+		return nil, nil, err
+	}
+
+	return content, newContent, nil
 }
 
 // appendFile reads the existing content (if any) via sysFs, appends new content, and writes back.

pkg/tools/fs/edit_test.go

@@ -2,6 +2,7 @@ package fstools
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -31,14 +32,34 @@ func TestEditTool_EditFile_Success(t *testing.T) {
 		t.Errorf("Expected success, got IsError=true: %s", result.ForLLM)
 	}
 
-	// Should return SilentResult
-	if !result.Silent {
-		t.Errorf("Expected Silent=true for EditFile, got false")
+	// Successful edits should surface a diff to the user.
+	if result.Silent {
+		t.Errorf("Expected Silent=false for EditFile, got true")
 	}
 
-	// ForUser should be empty (silent result)
-	if result.ForUser != "" {
-		t.Errorf("Expected ForUser to be empty for SilentResult, got: %s", result.ForUser)
+	if result.ForUser == "" {
+		t.Fatal("Expected ForUser to contain the diff preview")
+	}
+
+	if result.ForLLM == result.ForUser {
+		t.Fatalf("Expected ForLLM to be a compact summary, got identical outputs %q", result.ForLLM)
+	}
+	if result.ForLLM != fmt.Sprintf("File edited: %s", testFile) {
+		t.Fatalf("Expected compact ForLLM summary, got %q", result.ForLLM)
+	}
+
+	diffPath := strings.TrimLeft(filepath.ToSlash(testFile), "/")
+	for _, want := range []string{
+		fmt.Sprintf("File edited: %s", testFile),
+		"```diff",
+		"--- a/" + diffPath,
+		"+++ b/" + diffPath,
+		"-Hello World",
+		"+Hello Universe",
+	} {
+		if !strings.Contains(result.ForUser, want) {
+			t.Fatalf("Expected edit diff to contain %q, got:\n%s", want, result.ForUser)
+		}
 	}
 
 	// Verify file was actually edited
@@ -412,7 +433,13 @@ func TestEditFileTool_Restricted_InPlaceEdit(t *testing.T) {
 
 	result := tool.Execute(ctx, args)
 	assert.False(t, result.IsError, "Expected success, got: %s", result.ForLLM)
-	assert.True(t, result.Silent)
+	assert.False(t, result.Silent)
+	assert.Equal(t, "File edited: edit_target.txt", result.ForLLM)
+	assert.Contains(t, result.ForUser, "```diff")
+	assert.Contains(t, result.ForUser, "--- a/edit_target.txt")
+	assert.Contains(t, result.ForUser, "+++ b/edit_target.txt")
+	assert.Contains(t, result.ForUser, "-Hello World")
+	assert.Contains(t, result.ForUser, "+Hello Go")
 
 	data, err := os.ReadFile(filepath.Join(workspace, testFile))
 	assert.NoError(t, err)

pkg/tools/fs/shared.go

@@ -32,6 +32,10 @@ func SilentResult(forLLM string) *ToolResult {
 	return toolshared.SilentResult(forLLM)
 }
 
+func DiffResult(path string, before, after []byte) *ToolResult {
+	return toolshared.DiffResult(path, before, after)
+}
+
 func MediaResult(forLLM string, mediaRefs []string) *ToolResult {
 	return toolshared.MediaResult(forLLM, mediaRefs)
 }

pkg/tools/result_test.go

@@ -41,6 +41,53 @@ func TestSilentResult(t *testing.T) {
 	}
 }
 
+func TestDiffResult(t *testing.T) {
+	result := DiffResult("pkg/tools/fs/edit.go", []byte("hello world\n"), []byte("hello universe\n"))
+
+	if result.Silent {
+		t.Error("Expected Silent to be false")
+	}
+	if result.IsError {
+		t.Error("Expected IsError to be false")
+	}
+	if result.Async {
+		t.Error("Expected Async to be false")
+	}
+	if result.ForLLM == result.ForUser {
+		t.Fatalf("Expected ForLLM to omit the full diff, got %q", result.ForLLM)
+	}
+	if len(result.ForLLM) >= len(result.ForUser) {
+		t.Fatalf("Expected ForLLM to stay smaller than ForUser, got %d vs %d", len(result.ForLLM), len(result.ForUser))
+	}
+
+	for _, want := range []string{
+		"File edited: pkg/tools/fs/edit.go",
+		"```diff",
+		"--- a/pkg/tools/fs/edit.go",
+		"+++ b/pkg/tools/fs/edit.go",
+		"-hello world",
+		"+hello universe",
+	} {
+		if !strings.Contains(result.ForUser, want) {
+			t.Fatalf("DiffResult output missing %q:\n%s", want, result.ForUser)
+		}
+	}
+}
+
+func TestDiffResult_NormalizesAbsolutePathsAndHandlesNoOpChanges(t *testing.T) {
+	result := DiffResult("/tmp/test.txt", []byte("same\n"), []byte("same\n"))
+
+	if !strings.Contains(result.ForUser, "File edited: /tmp/test.txt") {
+		t.Fatalf("Expected original path in output, got %q", result.ForUser)
+	}
+	if !strings.Contains(result.ForUser, "(no content change)") {
+		t.Fatalf("Expected no-content-change marker, got %q", result.ForUser)
+	}
+	if !strings.Contains(result.ForLLM, "(no content change)") {
+		t.Fatalf("Expected compact no-op summary in ForLLM, got %q", result.ForLLM)
+	}
+}
+
 func TestAsyncResult(t *testing.T) {
 	result := AsyncResult("async task started")
 

pkg/tools/shared/diff_result.go

@@ -0,0 +1,162 @@
+package toolshared
+
+import (
+	"bytes"
+	"fmt"
+	"path/filepath"
+	"strings"
+	"unicode/utf8"
+
+	"github.com/pmezard/go-difflib/difflib"
+)
+
+const (
+	noContentChangeDiffMessage = "(no content change)"
+	noNewlineAtEOFMarker       = `\ No newline at end of file`
+	diffPreviewSkippedMessage  = "[diff preview skipped: file too large for inline preview]"
+	diffPreviewTruncatedNote   = "[diff preview truncated; call read_file for the full edited contents]"
+	maxDiffInputBytes          = 64 * 1024
+	maxDiffInputLines          = 2000
+	maxUserDiffPreviewBytes    = 16 * 1024
+)
+
+// DiffResult creates a user-visible tool result containing a unified diff for
+// a successful file edit. The diff is included for both the LLM and the user so
+// the follow-up assistant response can reason about the resulting change set,
+// including EOF newline transitions.
+func DiffResult(path string, before, after []byte) *ToolResult {
+	summary := fmt.Sprintf("File edited: %s", path)
+	if exceedsDiffPreviewLimits(before, after) {
+		return SilentResult(summary + "\n" + diffPreviewSkippedMessage)
+	}
+
+	diff, err := buildUnifiedDiff(path, before, after)
+	if err != nil {
+		return UserResult(fmt.Sprintf("%s\n[diff unavailable: %v]", summary, err))
+	}
+
+	userDiff, truncated := truncateDiffPreview(diff, maxUserDiffPreviewBytes)
+	userContent := fmt.Sprintf("%s\n```diff\n%s\n```", summary, userDiff)
+	if truncated {
+		userContent += "\n" + diffPreviewTruncatedNote
+	}
+
+	llmContent := summary
+	if diff == noContentChangeDiffMessage {
+		llmContent = summary + "\n" + noContentChangeDiffMessage
+	} else if truncated {
+		llmContent = summary + "\n" + diffPreviewTruncatedNote
+	}
+
+	return &ToolResult{
+		ForLLM:  llmContent,
+		ForUser: userContent,
+		Silent:  false,
+		IsError: false,
+		Async:   false,
+	}
+}
+
+func buildUnifiedDiff(path string, before, after []byte) (string, error) {
+	diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{
+		A:        splitDiffLinesPreservingEOF(before),
+		B:        splitDiffLinesPreservingEOF(after),
+		FromFile: "a/" + diffDisplayPath(path),
+		ToFile:   "b/" + diffDisplayPath(path),
+		Context:  3,
+	})
+	if err != nil {
+		return "", err
+	}
+
+	diff = strings.TrimRight(diff, "\n")
+	if diff == "" {
+		return noContentChangeDiffMessage, nil
+	}
+
+	return diff, nil
+}
+
+func splitDiffLinesPreservingEOF(content []byte) []string {
+	if len(content) == 0 {
+		return nil
+	}
+
+	lines := make([]string, 0, bytes.Count(content, []byte{'\n'})+1)
+	lineStart := 0
+	for i, b := range content {
+		if b != '\n' {
+			continue
+		}
+		lines = append(lines, string(content[lineStart:i+1]))
+		lineStart = i + 1
+	}
+	if lineStart < len(content) {
+		lines = append(lines, string(content[lineStart:]))
+	}
+
+	if lacksTrailingNewline(content) {
+		lines[len(lines)-1] += "\n"
+		lines = append(lines, noNewlineAtEOFMarker+"\n")
+	}
+
+	return lines
+}
+
+func lacksTrailingNewline(content []byte) bool {
+	return len(content) > 0 && !bytes.HasSuffix(content, []byte("\n"))
+}
+
+func exceedsDiffPreviewLimits(before, after []byte) bool {
+	return len(before) > maxDiffInputBytes ||
+		len(after) > maxDiffInputBytes ||
+		countDiffLines(before) > maxDiffInputLines ||
+		countDiffLines(after) > maxDiffInputLines
+}
+
+func countDiffLines(content []byte) int {
+	if len(content) == 0 {
+		return 0
+	}
+
+	lines := bytes.Count(content, []byte{'\n'})
+	if !bytes.HasSuffix(content, []byte("\n")) {
+		lines++
+	}
+	return lines
+}
+
+func truncateDiffPreview(diff string, maxBytes int) (string, bool) {
+	if maxBytes <= 0 || len(diff) <= maxBytes {
+		return diff, false
+	}
+
+	truncated := diff[:maxBytes]
+	for len(truncated) > 0 && !utf8.ValidString(truncated) {
+		truncated = truncated[:len(truncated)-1]
+	}
+
+	lastNewline := strings.LastIndexByte(truncated, '\n')
+	if lastNewline > 0 {
+		truncated = truncated[:lastNewline]
+	}
+
+	truncated = strings.TrimRight(truncated, "\n")
+	if truncated == "" {
+		truncated = diff[:maxBytes]
+		for len(truncated) > 0 && !utf8.ValidString(truncated) {
+			truncated = truncated[:len(truncated)-1]
+		}
+		truncated = strings.TrimRight(truncated, "\n")
+	}
+
+	return truncated, true
+}
+
+func diffDisplayPath(path string) string {
+	displayPath := strings.TrimLeft(filepath.ToSlash(path), "/")
+	if displayPath == "" {
+		return "file"
+	}
+	return displayPath
+}

pkg/tools/shared/diff_result_test.go

@@ -0,0 +1,177 @@
+package toolshared
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+)
+
+func TestDiffResult_UserVisibleUnifiedDiff(t *testing.T) {
+	result := DiffResult("/tmp/example.txt", []byte("alpha\nbeta\ngamma\n"), []byte("alpha\nbeta 2\ngamma\n"))
+
+	if result == nil {
+		t.Fatal("DiffResult() returned nil")
+	}
+	if result.Silent {
+		t.Fatal("expected DiffResult to be user-visible")
+	}
+	if result.IsError {
+		t.Fatal("expected DiffResult to be successful")
+	}
+	if result.ForLLM == result.ForUser {
+		t.Fatal("expected compact model context instead of duplicating the full diff")
+	}
+	if len(result.ForLLM) >= len(result.ForUser) {
+		t.Fatalf("expected ForLLM to stay smaller than ForUser, got %d vs %d", len(result.ForLLM), len(result.ForUser))
+	}
+	if result.ForLLM != "File edited: /tmp/example.txt" {
+		t.Fatalf("expected compact summary in ForLLM, got %q", result.ForLLM)
+	}
+
+	for _, want := range []string{
+		"File edited: /tmp/example.txt",
+		"```diff",
+		"--- a/tmp/example.txt",
+		"+++ b/tmp/example.txt",
+		"@@ -1,3 +1,3 @@",
+		" alpha",
+		"-beta",
+		"+beta 2",
+		" gamma",
+	} {
+		if !strings.Contains(result.ForUser, want) {
+			t.Fatalf("DiffResult output missing %q:\n%s", want, result.ForUser)
+		}
+	}
+}
+
+func TestBuildUnifiedDiff_NoContentChange(t *testing.T) {
+	diff, err := buildUnifiedDiff("test.txt", []byte("same\n"), []byte("same\n"))
+	if err != nil {
+		t.Fatalf("buildUnifiedDiff() error = %v", err)
+	}
+	if diff != noContentChangeDiffMessage {
+		t.Fatalf("buildUnifiedDiff() = %q, want %q", diff, noContentChangeDiffMessage)
+	}
+}
+
+func TestBuildUnifiedDiff_PreservesTrailingNewlineRemoval(t *testing.T) {
+	diff, err := buildUnifiedDiff("test.txt", []byte("same\n"), []byte("same"))
+	if err != nil {
+		t.Fatalf("buildUnifiedDiff() error = %v", err)
+	}
+
+	for _, want := range []string{
+		"--- a/test.txt",
+		"+++ b/test.txt",
+		" same",
+		"+" + noNewlineAtEOFMarker,
+	} {
+		if !strings.Contains(diff, want) {
+			t.Fatalf("buildUnifiedDiff() missing %q:\n%s", want, diff)
+		}
+	}
+}
+
+func TestBuildUnifiedDiff_PreservesTrailingNewlineAddition(t *testing.T) {
+	diff, err := buildUnifiedDiff("test.txt", []byte("same"), []byte("same\n"))
+	if err != nil {
+		t.Fatalf("buildUnifiedDiff() error = %v", err)
+	}
+
+	for _, want := range []string{
+		"--- a/test.txt",
+		"+++ b/test.txt",
+		" same",
+		"-" + noNewlineAtEOFMarker,
+	} {
+		if !strings.Contains(diff, want) {
+			t.Fatalf("buildUnifiedDiff() missing %q:\n%s", want, diff)
+		}
+	}
+}
+
+func TestBuildUnifiedDiff_UsesNormalizedDisplayPaths(t *testing.T) {
+	diff, err := buildUnifiedDiff("/tmp/nested/example.txt", []byte("before\n"), []byte("after\n"))
+	if err != nil {
+		t.Fatalf("buildUnifiedDiff() error = %v", err)
+	}
+
+	for _, want := range []string{
+		"--- a/tmp/nested/example.txt",
+		"+++ b/tmp/nested/example.txt",
+	} {
+		if !strings.Contains(diff, want) {
+			t.Fatalf("buildUnifiedDiff() missing %q:\n%s", want, diff)
+		}
+	}
+}
+
+func TestDiffResult_SkipsPreviewForLargeFiles(t *testing.T) {
+	before := bytes.Repeat([]byte("a"), maxDiffInputBytes+1)
+	after := bytes.Repeat([]byte("b"), maxDiffInputBytes+1)
+
+	result := DiffResult("big.txt", before, after)
+
+	if !result.Silent {
+		t.Fatal("expected large diff previews to be skipped silently")
+	}
+	if result.ForUser != "" {
+		t.Fatalf("expected no user-facing preview when skipped, got %q", result.ForUser)
+	}
+	if !strings.Contains(result.ForLLM, diffPreviewSkippedMessage) {
+		t.Fatalf("expected skipped-preview note, got %q", result.ForLLM)
+	}
+}
+
+func TestDiffResult_TruncatesLargeUserPreview(t *testing.T) {
+	after := []byte(strings.Repeat("abcd", maxUserDiffPreviewBytes/4) + "\n")
+
+	result := DiffResult("preview.txt", []byte("before\n"), after)
+
+	if result.Silent {
+		t.Fatal("expected preview to remain user-visible below the input caps")
+	}
+	if !strings.Contains(result.ForUser, diffPreviewTruncatedNote) {
+		t.Fatalf("expected truncated preview note, got %q", result.ForUser)
+	}
+	if !strings.Contains(result.ForLLM, diffPreviewTruncatedNote) {
+		t.Fatalf("expected model summary to mention truncation, got %q", result.ForLLM)
+	}
+	if len(result.ForLLM) >= len(result.ForUser) {
+		t.Fatalf("expected ForLLM to remain smaller than ForUser, "+
+			"got %d vs %d", len(result.ForLLM), len(result.ForUser))
+	}
+}
+
+func TestDiffDisplayPath(t *testing.T) {
+	tests := []struct {
+		name string
+		path string
+		want string
+	}{
+		{
+			name: "absolute path",
+			path: "/tmp/example.txt",
+			want: "tmp/example.txt",
+		},
+		{
+			name: "relative path",
+			path: "pkg/tools/fs/edit.go",
+			want: "pkg/tools/fs/edit.go",
+		},
+		{
+			name: "empty path",
+			path: "",
+			want: "file",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := diffDisplayPath(tt.path); got != tt.want {
+				t.Fatalf("diffDisplayPath(%q) = %q, want %q", tt.path, got, tt.want)
+			}
+		})
+	}
+}

pkg/tools/shared_facade.go

@@ -101,6 +101,10 @@ func SilentResult(forLLM string) *ToolResult {
 	return toolshared.SilentResult(forLLM)
 }
 
+func DiffResult(path string, before, after []byte) *ToolResult {
+	return toolshared.DiffResult(path, before, after)
+}
+
 func AsyncResult(forLLM string) *ToolResult {
 	return toolshared.AsyncResult(forLLM)
 }