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..e94a896da 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,31 @@ 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.Errorf("Expected ForLLM and ForUser to match, got ForLLM=%q ForUser=%q", result.ForLLM, result.ForUser) + } + + 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 +430,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, result.ForLLM, result.ForUser) + 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..fda7e69c0 100644 --- a/pkg/tools/result_test.go +++ b/pkg/tools/result_test.go @@ -41,6 +41,47 @@ 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.ForLLM != result.ForUser { + t.Fatalf("Expected ForLLM and ForUser to match, got %q vs %q", result.ForLLM, result.ForUser) + } + 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") + } + + 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) + } +} + 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..99f7e9793 --- /dev/null +++ b/pkg/tools/shared/diff_result.go @@ -0,0 +1,52 @@ +package toolshared + +import ( + "fmt" + "path/filepath" + "strings" + + "github.com/pmezard/go-difflib/difflib" +) + +const noContentChangeDiffMessage = "(no content change)" + +// 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 exact change set. +func DiffResult(path string, before, after []byte) *ToolResult { + diff, err := buildUnifiedDiff(path, before, after) + if err != nil { + return UserResult(fmt.Sprintf("File edited: %s\n[diff unavailable: %v]", path, err)) + } + + content := fmt.Sprintf("File edited: %s\n```diff\n%s\n```", path, diff) + return UserResult(content) +} + +func buildUnifiedDiff(path string, before, after []byte) (string, error) { + diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ + A: difflib.SplitLines(string(before)), + B: difflib.SplitLines(string(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 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..7c06d205a --- /dev/null +++ b/pkg/tools/shared/diff_result_test.go @@ -0,0 +1,97 @@ +package toolshared + +import ( + "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.ForLLM != result.ForUser { + t.Fatalf("expected ForLLM and ForUser to match, got %q vs %q", result.ForLLM, result.ForUser) + } + if result.Silent { + t.Fatal("expected DiffResult to be user-visible") + } + if result.IsError { + t.Fatal("expected DiffResult to be successful") + } + + for _, want := range []string{ + "File edited: /tmp/example.txt", + "```diff", + "--- a/tmp/example.txt", + "+++ b/tmp/example.txt", + "@@ -1,4 +1,4 @@", + " 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_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 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) }