diff --git a/pkg/tools/fs/edit.go b/pkg/tools/fs/edit.go index 827ea50c8..7a54a1b01 100644 --- a/pkg/tools/fs/edit.go +++ b/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. diff --git a/pkg/tools/fs/edit_test.go b/pkg/tools/fs/edit_test.go index 4c25322ef..b04c41fff 100644 --- a/pkg/tools/fs/edit_test.go +++ b/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) diff --git a/pkg/tools/fs/shared.go b/pkg/tools/fs/shared.go index 6d46e692b..acf14169e 100644 --- a/pkg/tools/fs/shared.go +++ b/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) } diff --git a/pkg/tools/result_test.go b/pkg/tools/result_test.go index 5f08cb4fa..3e7848687 100644 --- a/pkg/tools/result_test.go +++ b/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") diff --git a/pkg/tools/shared/diff_result.go b/pkg/tools/shared/diff_result.go new file mode 100644 index 000000000..3ed7bdda1 --- /dev/null +++ b/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 +} diff --git a/pkg/tools/shared/diff_result_test.go b/pkg/tools/shared/diff_result_test.go new file mode 100644 index 000000000..9d4f38ea5 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/pkg/tools/shared_facade.go b/pkg/tools/shared_facade.go index 8409ea060..85bac140a 100644 --- a/pkg/tools/shared_facade.go +++ b/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) }