feat(tools): show unified diff for edit_file edits
This commit is contained in:
parent
223ebdf0c7
commit
4a81f0e740
7 changed files with 240 additions and 13 deletions
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
||||
|
|
|
|||
52
pkg/tools/shared/diff_result.go
Normal file
52
pkg/tools/shared/diff_result.go
Normal file
|
|
@ -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
|
||||
}
|
||||
97
pkg/tools/shared/diff_result_test.go
Normal file
97
pkg/tools/shared/diff_result_test.go
Normal file
|
|
@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue