diff --git a/internal/acp/translate.go b/internal/acp/translate.go index 565174904..84664e58d 100644 --- a/internal/acp/translate.go +++ b/internal/acp/translate.go @@ -2,6 +2,7 @@ package acp import ( "encoding/json" + "path/filepath" "strings" "unicode/utf8" @@ -120,22 +121,59 @@ func toolCallResult(result agent.ToolResult) ToolCallUpdate { } func toolResultContent(result agent.ToolResult) []ToolCallContent { + content := make([]ToolCallContent, 0, 1+len(result.FileDiffs)) text := strings.TrimRight(result.Output, "\n") if text == "" { text = result.Display.Summary } if text == "" { - return nil + return appendToolResultDiffs(content, result.FileDiffs) } - return []ToolCallContent{ToolContent(TextBlock(text))} + content = append(content, ToolContent(TextBlock(text))) + return appendToolResultDiffs(content, result.FileDiffs) +} + +func appendToolResultDiffs(content []ToolCallContent, diffs []tools.FileDiff) []ToolCallContent { + for _, diff := range diffs { + // ACP's diff block has no file-existence bit. A deleted file and an + // existing file replaced with empty content would otherwise serialize + // identically, so omit deletions rather than present a false truncation. + // ChangedFiles remains the conservative fallback for the operation. + if !filepath.IsAbs(diff.Path) || !diff.NewExists { + continue + } + newText := diff.NewText + var oldText *string + if diff.OldExists { + old := diff.OldText + oldText = &old + } + content = append(content, ToolCallContent{Type: "diff", Path: diff.Path, OldText: oldText, NewText: &newText}) + } + return content } func toolResultLocations(result agent.ToolResult) []ToolCallLocation { - locs := make([]ToolCallLocation, 0, len(result.ChangedFiles)) + locs := make([]ToolCallLocation, 0, len(result.FileDiffs)+len(result.ChangedFiles)) + seen := make(map[string]bool, len(result.FileDiffs)+len(result.ChangedFiles)) + for _, diff := range result.FileDiffs { + path := diff.Path + if path == "" || seen[path] { + continue + } + seen[path] = true + locs = append(locs, ToolCallLocation{Path: path}) + } + // FileDiff.Path is canonical absolute path data while ChangedFiles is + // normally workspace-relative. Without the trusted workspace root these + // coordinate systems cannot be correlated safely: a suffix match would let + // /workspace/sub/a.go consume the fallback for a distinct root a.go. The + // shared seen set deduplicates only identities already exactly comparable. for _, f := range result.ChangedFiles { - if strings.TrimSpace(f) == "" { + if f == "" || seen[f] { continue } + seen[f] = true locs = append(locs, ToolCallLocation{Path: f}) } return locs diff --git a/internal/acp/translate_test.go b/internal/acp/translate_test.go index 4a9adc16d..d54745cd1 100644 --- a/internal/acp/translate_test.go +++ b/internal/acp/translate_test.go @@ -1,6 +1,8 @@ package acp import ( + "encoding/json" + "path/filepath" "strings" "testing" "unicode/utf8" @@ -74,21 +76,26 @@ func TestToolCallStart(t *testing.T) { } func TestToolCallResult(t *testing.T) { + path := filepath.Join(t.TempDir(), "a.go") ok := toolCallResult(agent.ToolResult{ ToolCallID: "tc1", Name: "edit_file", Status: tools.StatusOK, Output: "applied\n", ChangedFiles: []string{"a.go", ""}, + FileDiffs: []tools.FileDiff{{Path: path, OldExists: true, NewExists: true, OldText: "before\n", NewText: "after\n"}}, }) if ok.SessionUpdate != UpdateToolCallUpdate || ok.Status != ToolStatusCompleted { t.Fatalf("unexpected ok result: %+v", ok) } - if len(ok.Content) != 1 || ok.Content[0].Type != "content" || ok.Content[0].Content.Text != "applied" { + if len(ok.Content) != 2 || ok.Content[0].Type != "content" || ok.Content[0].Content.Text != "applied" { t.Fatalf("unexpected content: %+v", ok.Content) } - if len(ok.Locations) != 1 || ok.Locations[0].Path != "a.go" { - t.Fatalf("blank changed files should be dropped, got %+v", ok.Locations) + if diff := ok.Content[1]; diff.Type != "diff" || diff.Path != path || diff.OldText == nil || *diff.OldText != "before\n" || diff.NewText == nil || *diff.NewText != "after\n" { + t.Fatalf("unexpected diff content: %+v", diff) + } + if len(ok.Locations) != 2 || ok.Locations[0].Path != path || ok.Locations[1].Path != "a.go" { + t.Fatalf("unproven absolute/relative aliases must both remain visible, got %+v", ok.Locations) } failed := toolCallResult(agent.ToolResult{ToolCallID: "tc2", Status: tools.StatusError, Output: "boom"}) @@ -97,6 +104,127 @@ func TestToolCallResult(t *testing.T) { } } +func TestToolCallDiffJSONPreservesEmptyFilesWithoutClaimingDeletion(t *testing.T) { + path := filepath.Join(t.TempDir(), "empty.txt") + content := appendToolResultDiffs(nil, []tools.FileDiff{ + {Path: path, OldExists: false, NewExists: true, NewText: ""}, + {Path: path, OldExists: true, NewExists: true, OldText: "before", NewText: ""}, + {Path: path, OldExists: true, NewExists: false, OldText: "before"}, + }) + if len(content) != 2 { + t.Fatalf("diff content = %#v", content) + } + for index, diff := range content { + encoded, err := json.Marshal(diff) + if err != nil { + t.Fatal(err) + } + var wire map[string]any + if err := json.Unmarshal(encoded, &wire); err != nil { + t.Fatal(err) + } + if wire["path"] != path || wire["newText"] != "" { + t.Fatalf("wire diff %d = %s", index, encoded) + } + if index == 0 && wire["oldText"] != nil { + t.Fatalf("create oldText = %#v, want null", wire["oldText"]) + } + if index == 1 && wire["oldText"] != "before" { + t.Fatalf("update oldText = %#v, want before", wire["oldText"]) + } + } +} + +func TestToolResultLocationsPreserveDistinctPathIdentities(t *testing.T) { + root := t.TempDir() + rootPath := filepath.Join(root, "a.go") + nestedPath := filepath.Join(root, "sub", "a.go") + diff := func(path string) tools.FileDiff { + return tools.FileDiff{Path: path, OldExists: true, NewExists: true, OldText: "before", NewText: "after"} + } + for _, tc := range []struct { + name string + diffs []tools.FileDiff + want []string + }{ + {name: "both rich", diffs: []tools.FileDiff{diff(rootPath), diff(nestedPath)}, want: []string{rootPath, nestedPath, "a.go", filepath.Join("sub", "a.go")}}, + {name: "root rich", diffs: []tools.FileDiff{diff(rootPath)}, want: []string{rootPath, "a.go", filepath.Join("sub", "a.go")}}, + {name: "nested rich", diffs: []tools.FileDiff{diff(nestedPath)}, want: []string{nestedPath, "a.go", filepath.Join("sub", "a.go")}}, + } { + t.Run(tc.name, func(t *testing.T) { + locations := toolResultLocations(agent.ToolResult{ + ChangedFiles: []string{"a.go", filepath.Join("sub", "a.go")}, + FileDiffs: tc.diffs, + }) + if len(locations) != len(tc.want) { + t.Fatalf("locations = %#v, want %#v", locations, tc.want) + } + for index := range tc.want { + if locations[index].Path != tc.want[index] { + t.Fatalf("locations = %#v, want %#v", locations, tc.want) + } + } + }) + } +} + +func TestToolCallResultPreservesWhitespaceInFilePaths(t *testing.T) { + relativePath := " report.txt " + absolutePath := filepath.Join(t.TempDir(), relativePath) + update := toolCallResult(agent.ToolResult{ + ChangedFiles: []string{relativePath}, + FileDiffs: []tools.FileDiff{{ + Path: absolutePath, OldExists: true, NewExists: true, OldText: "before", NewText: "after", + }}, + }) + if len(update.Content) != 1 || update.Content[0].Path != absolutePath { + t.Fatalf("diff content path = %#v, want %q", update.Content, absolutePath) + } + if len(update.Locations) != 2 || update.Locations[0].Path != absolutePath || update.Locations[1].Path != relativePath { + t.Fatalf("locations = %#v, want exact paths %q and %q", update.Locations, absolutePath, relativePath) + } +} + +func TestToolResultLocationsDeduplicateOnlyExactPaths(t *testing.T) { + path := filepath.Join(t.TempDir(), "a.go") + locations := toolResultLocations(agent.ToolResult{ + ChangedFiles: []string{path, path}, + FileDiffs: []tools.FileDiff{{Path: path, OldExists: true, NewExists: true, OldText: "before", NewText: "after"}}, + }) + if len(locations) != 1 || locations[0].Path != path { + t.Fatalf("exact duplicate locations = %#v", locations) + } +} + +func TestDeletedFileKeepsPathOnlyLocation(t *testing.T) { + relativePath := "deleted.go" + absolutePath := filepath.Join(t.TempDir(), relativePath) + update := toolCallResult(agent.ToolResult{ + ChangedFiles: []string{relativePath}, + FileDiffs: []tools.FileDiff{{ + Path: absolutePath, OldExists: true, NewExists: false, OldText: "before", + }}, + }) + if len(update.Content) != 0 { + t.Fatalf("deleted file must not emit an ambiguous ACP diff: %#v", update.Content) + } + if len(update.Locations) != 2 || update.Locations[0].Path != absolutePath || update.Locations[1].Path != relativePath { + t.Fatalf("deleted file locations = %#v", update.Locations) + } +} + +func TestToolCallResultEmitsOnlyRedactedFileDiffs(t *testing.T) { + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + path := filepath.Join(t.TempDir(), "secret.txt") + scrubbed := tools.ScrubResultSecrets(tools.Result{FileDiffs: []tools.FileDiff{{ + Path: path, OldExists: true, NewExists: true, OldText: "token=" + secret, NewText: "safe", + }}}) + update := toolCallResult(agent.ToolResult{ToolCallID: "call", Status: tools.StatusError, FileDiffs: scrubbed.FileDiffs}) + if len(update.Content) != 1 || update.Content[0].OldText == nil || strings.Contains(*update.Content[0].OldText, secret) { + t.Fatalf("ACP content leaked unredacted diff: %#v", update.Content) + } +} + func TestPlanUpdateAndStatus(t *testing.T) { upd := planUpdate([]tools.PlanItem{ {Content: "step a", Status: "completed"}, diff --git a/internal/acp/types.go b/internal/acp/types.go index b00bf672a..f4f601fb3 100644 --- a/internal/acp/types.go +++ b/internal/acp/types.go @@ -220,9 +220,30 @@ type ToolCallContent struct { // type == "content" Content *ContentBlock `json:"content,omitempty"` // type == "diff" - Path string `json:"path,omitempty"` - OldText string `json:"oldText,omitempty"` - NewText string `json:"newText,omitempty"` + Path string `json:"path,omitempty"` + OldText *string `json:"oldText,omitempty"` + NewText *string `json:"newText,omitempty"` +} + +// MarshalJSON preserves ACP's discriminated content union. A diff always has +// path and newText (including an intentionally empty deletion value); oldText +// is JSON null for a newly created file. Other content variants omit all diff +// fields rather than serializing irrelevant nulls. +func (content ToolCallContent) MarshalJSON() ([]byte, error) { + if content.Type == "diff" { + return json.Marshal(struct { + Type string `json:"type"` + Path string `json:"path"` + OldText *string `json:"oldText"` + NewText *string `json:"newText"` + }{ + Type: content.Type, Path: content.Path, OldText: content.OldText, NewText: content.NewText, + }) + } + return json.Marshal(struct { + Type string `json:"type"` + Content *ContentBlock `json:"content,omitempty"` + }{Type: content.Type, Content: content.Content}) } func ToolContent(block ContentBlock) ToolCallContent { diff --git a/internal/agent/loop.go b/internal/agent/loop.go index fe691ac4c..0d06d82f4 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -1527,6 +1527,7 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal Images: result.Images, Redacted: result.Redacted, ChangedFiles: result.ChangedFiles, + FileDiffs: result.FileDiffs, ChangeSummaries: result.ChangeSummaries, Display: result.HumanDisplay(), Outcome: result.Outcome, @@ -1833,6 +1834,10 @@ func runToolForUnsandboxedRetry(ctx context.Context, registry *tools.Registry, n } func toolResultFromPrePermissionReject(call ToolCall, result tools.Result) ToolResult { + // PrePermissionRejecter runs before Registry.RunWithOptions, so it must + // explicitly cross the same transcript/redaction boundary before its result + // can be forwarded through ACP. + result = tools.ScrubResultSecrets(result) output, outputRedacted := scrubInterceptedOutput(result.Output) display := result.Display summary, summaryRedacted := scrubInterceptedOutput(display.Summary) @@ -1860,6 +1865,7 @@ func toolResultFromPrePermissionReject(call ToolCall, result tools.Result) ToolR Meta: meta, Redacted: result.Redacted || outputRedacted || summaryRedacted || metaRedacted, ChangedFiles: result.ChangedFiles, + FileDiffs: result.FileDiffs, ChangeSummaries: result.ChangeSummaries, Display: display, LoadedTools: loadedToolsFromResult(meta), @@ -2153,6 +2159,7 @@ func askUserFallbackResult(ctx context.Context, registry *tools.Registry, call T Meta: result.Meta, Redacted: result.Redacted, ChangedFiles: result.ChangedFiles, + FileDiffs: result.FileDiffs, ChangeSummaries: result.ChangeSummaries, Display: result.HumanDisplay(), Outcome: result.Outcome, diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index f17e9be46..7233f2f08 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -27,6 +27,23 @@ type mockProvider struct { requests []zeroruntime.CompletionRequest } +func TestPrePermissionRejectScrubsFileDiffs(t *testing.T) { + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + result := toolResultFromPrePermissionReject(ToolCall{ID: "call", Name: "test"}, tools.Result{ + Status: tools.StatusError, + FileDiffs: []tools.FileDiff{{ + Path: filepath.Join(t.TempDir(), "secret.txt"), + OldExists: true, + NewExists: true, + OldText: "token=" + secret, + NewText: "safe", + }}, + }) + if len(result.FileDiffs) != 1 || strings.Contains(result.FileDiffs[0].OldText, secret) || !result.Redacted { + t.Fatalf("pre-permission FileDiff = %#v, redacted = %t", result.FileDiffs, result.Redacted) + } +} + func TestTypedExecutionOutcomeOverridesLegacySandboxHeuristics(t *testing.T) { engine := sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: t.TempDir(), Policy: sandbox.DefaultPolicy()}) call := ToolCall{Name: tools.ExecCommandToolName} diff --git a/internal/agent/types.go b/internal/agent/types.go index 511ea7140..19ff955bc 100644 --- a/internal/agent/types.go +++ b/internal/agent/types.go @@ -84,6 +84,7 @@ type ToolResult struct { Images []zeroruntime.ImageBlock Redacted bool ChangedFiles []string + FileDiffs []tools.FileDiff // ChangeSummaries are non-selectable generated-tree summaries emitted by // command execution; callers must not schedule per-file work from them. ChangeSummaries []execution.Change diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go index 2561d4eb7..56046c19f 100644 --- a/internal/tools/apply_patch_tolerance_test.go +++ b/internal/tools/apply_patch_tolerance_test.go @@ -363,7 +363,7 @@ func TestUnifiedPatchCopyOperation(t *testing.T) { if content, _ := os.ReadFile(filepath.Join(root, "dst.txt")); string(content) != "hello\nnew\n" { t.Fatalf("copy destination = %q", string(content)) } - if len(result.ChangedFiles) != 2 || result.ChangedFiles[0] != "src.txt" || result.ChangedFiles[1] != "dst.txt" { + if len(result.ChangedFiles) != 1 || result.ChangedFiles[0] != "dst.txt" { t.Fatalf("changed files = %v", result.ChangedFiles) } destination, err := filepath.EvalSymlinks(filepath.Join(root, "dst.txt")) @@ -709,4 +709,14 @@ func TestApplyPatchOperationsReportsCommittedPrefixOnFailure(t *testing.T) { if content, _ := os.ReadFile(filepath.Join(root, "third.txt")); string(content) != "three\n" { t.Fatalf("third.txt must be untouched, got %q", string(content)) } + if got := result.ChangedFiles; len(got) != 1 || got[0] != "first.txt" { + t.Fatalf("partial patch ChangedFiles = %#v, want committed prefix only", got) + } + resolvedFirst, err := filepath.EvalSymlinks(filepath.Join(root, "first.txt")) + if err != nil { + t.Fatal(err) + } + if got := result.FileDiffs; len(got) != 1 || got[0] != (FileDiff{Path: resolvedFirst, OldExists: true, NewExists: true, OldText: "one\n", NewText: "ONE\n"}) { + t.Fatalf("partial patch FileDiffs = %#v", got) + } } diff --git a/internal/tools/diff_preview.go b/internal/tools/diff_preview.go index b2db6c7c9..7d7df606e 100644 --- a/internal/tools/diff_preview.go +++ b/internal/tools/diff_preview.go @@ -1,19 +1,108 @@ package tools -import udiff "github.com/aymanbagabas/go-udiff" +import ( + "path/filepath" + "strings" + "unicode" + "unicode/utf8" + + "github.com/Gitlawb/zero/internal/redaction" + udiff "github.com/aymanbagabas/go-udiff" +) // maxToolPreviewBytes caps the inline diff a write tool appends to its result, so // a large generated file can't flood the transcript or balloon the persisted // session events. Past this the tool falls back to its summary line alone. const maxToolPreviewBytes = 48 * 1024 +// A structured replacement contains two complete file sides, so its transport +// budget is intentionally separate from the single rendered-preview budget. +// Each side may be as large as a normal preview; aggregate producers apply the +// two-sided result cap in file order. +const maxToolResultFileDiffBytes = 2 * maxToolPreviewBytes + +// FileDiff is a human-facing before/after file change. Registry-boundary +// redaction applies to both sides before any caller receives it. +type FileDiff struct { + // Path is the canonical absolute path required by ACP diff content. The + // separate ChangedFiles result remains workspace-relative for local UI use. + Path string + // OldExists and NewExists distinguish an empty file from a missing side of a + // create/delete/move. Empty strings alone cannot encode that difference. + OldExists bool + NewExists bool + OldText string + NewText string +} + +// boundedFileDiff declines rather than truncating: a truncated side would look +// like an exact file replacement. Callers keep ChangedFiles as the safe +// fallback for large, unsafe, or unchanged content. Each complete side gets the +// same bound as a rendered preview; aggregate result producers apply their own +// cap. Control bytes are rejected. Ordinary Unicode format/space characters are +// retained unless removing them reveals a credential shape that the normal +// redactor could not see in the original text. +func boundedFileDiff(path, oldText, newText string, oldExists, newExists bool) (FileDiff, bool) { + if !filepath.IsAbs(path) || (!oldExists && !newExists) || + (oldExists == newExists && oldText == newText) || + !utf8.ValidString(oldText) || !utf8.ValidString(newText) || + unsafeDiffText(oldText) || unsafeDiffText(newText) || + len(oldText) > maxToolPreviewBytes || len(newText) > maxToolPreviewBytes { + return FileDiff{}, false + } + return FileDiff{Path: path, OldExists: oldExists, NewExists: newExists, OldText: oldText, NewText: newText}, true +} + +func unsafeDiffText(text string) bool { + if !utf8.ValidString(text) { + return true + } + hasCanonicalizableSeparator := false + for _, r := range text { + switch r { + case '\n', '\r', '\t', ' ': + continue + } + if unicode.IsControl(r) { + return true + } + if unicode.Is(unicode.Cf, r) || unicode.IsSpace(r) { + hasCanonicalizableSeparator = true + } + } + if !hasCanonicalizableSeparator { + return false + } + return diffTextRevealsObfuscatedSecret(text) +} + +func diffTextRevealsObfuscatedSecret(text string) bool { + if !utf8.ValidString(text) { + return false + } + canonical := strings.Map(func(r rune) rune { + switch r { + case '\n', '\r', '\t', ' ': + return r + } + if unicode.Is(unicode.Cf, r) || unicode.IsSpace(r) { + return -1 + } + return r + }, text) + return canonical != text && redaction.RedactString(canonical, redaction.Options{}) != canonical +} + // boundedUnifiedDiff returns a unified diff of oldContent -> newContent labelled // with path, suitable for the TUI's diff card renderer. A create (oldContent "") // yields an all-additions (green) preview; an overwrite/edit yields red/green. -// Returns "" when there is no change or the diff exceeds maxToolPreviewBytes. +// Returns "" when there is no change, the rendered diff is unsafe text, or the +// diff exceeds maxToolPreviewBytes. This applies the same unsafe-text gate as +// FileDiff to what reaches Display.Preview, without discarding a safe hunk only +// because an unrelated part of the source file contains an unsafe byte. func boundedUnifiedDiff(path, oldContent, newContent string) string { diff := udiff.Unified(path, path, oldContent, newContent) - if diff == "" || len(diff) > maxToolPreviewBytes { + if diff == "" || !utf8.ValidString(diff) || unsafeDiffText(diff) || len(diff) > maxToolPreviewBytes { return "" } return diff diff --git a/internal/tools/diff_preview_test.go b/internal/tools/diff_preview_test.go new file mode 100644 index 000000000..bda35b1d0 --- /dev/null +++ b/internal/tools/diff_preview_test.go @@ -0,0 +1,182 @@ +package tools + +import ( + "path/filepath" + "strings" + "testing" +) + +func TestBoundedFileDiffRefusesPartialOrBinaryContent(t *testing.T) { + path := filepath.Join(t.TempDir(), "a.txt") + if diff, ok := boundedFileDiff(path, "old", "new", true, true); !ok || diff.Path != path || !diff.OldExists || !diff.NewExists || diff.OldText != "old" || diff.NewText != "new" { + t.Fatalf("small text diff = %#v, %t", diff, ok) + } + for _, tc := range []struct { + name string + old string + new string + }{ + {"unchanged", "same", "same"}, + {"binary old", string([]byte{0xff}), "text"}, + {"binary new", "text", string([]byte{0xff})}, + {"nul old", "token=sk-proj-abc\x00def", "text"}, + {"escape new", "text", "token=sk-proj-abc\x1bdef"}, + {"c1 old", "token=sk-proj-abc\u0085def", "text"}, + {"zero width space", "token=sk-proj-abc\u200bdef", "text"}, + {"zero width joiner", "token=sk-proj-abc\u200ddef", "text"}, + {"byte order mark", "token=sk-proj-abc\ufeffdef", "text"}, + {"soft hyphen", "token=sk-proj-abc\u00addef", "text"}, + {"non breaking space", "token=sk-proj-abc\u00a0def", "text"}, + {"too large", strings.Repeat("a", maxToolPreviewBytes+1), "b"}, + } { + t.Run(tc.name, func(t *testing.T) { + if diff, ok := boundedFileDiff(path, tc.old, tc.new, true, true); ok || diff != (FileDiff{}) { + t.Fatalf("unexpected diff = %#v, %t", diff, ok) + } + }) + } +} + +func TestBoundedFileDiffPreservesEmptyFileOperations(t *testing.T) { + path := filepath.Join(t.TempDir(), "empty.txt") + for _, tc := range []struct { + name string + oldExists, newExists bool + }{ + {name: "create", newExists: true}, + {name: "delete", oldExists: true}, + } { + t.Run(tc.name, func(t *testing.T) { + diff, ok := boundedFileDiff(path, "", "", tc.oldExists, tc.newExists) + if !ok || diff.OldExists != tc.oldExists || diff.NewExists != tc.newExists { + t.Fatalf("empty %s = %#v, %t", tc.name, diff, ok) + } + }) + } +} + +func TestBoundedUnifiedDiffRejectsUnsafeRichText(t *testing.T) { + if got := boundedUnifiedDiff("safe.txt", "before\n", "after\n"); got == "" { + t.Fatal("safe unified diff was unexpectedly omitted") + } + for _, content := range []string{ + "token=sk-proj-abc\x00def", + "token=sk-proj-abc\x1bdef", + "token=sk-proj-abc\u200bdef", + string([]byte{0xff}), + } { + if got := boundedUnifiedDiff("secret.txt", content, "safe\n"); got != "" { + t.Fatalf("unsafe rich diff = %q", got) + } + } + + old := strings.Repeat("unchanged\n", 12) + "form\ffeed\n" + strings.Repeat("unchanged\n", 12) + updated := strings.Replace(old, "unchanged\n", "changed\n", 1) + if got := boundedUnifiedDiff("safe-hunk.txt", old, updated); got == "" || strings.Contains(got, "\f") { + t.Fatalf("safe hunk near unrelated unsafe text = %q", got) + } +} + +func TestStructuredPatchFileDiffsPreserveEmptyOperationsAndResultBudget(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + changes := []structuredPatchChange{ + {kind: structuredPatchAdd, to: structuredPatchTarget{absolute: filepath.Join(root, "created"), relative: "created"}}, + {kind: structuredPatchDelete, from: structuredPatchTarget{absolute: filepath.Join(root, "deleted"), relative: "deleted"}}, + {kind: structuredPatchUpdate, from: structuredPatchTarget{absolute: filepath.Join(root, "from"), relative: "from"}, to: structuredPatchTarget{absolute: filepath.Join(root, "to"), relative: "to"}}, + } + diffs := fileDiffsFromStructuredPatch(".", changes) + if len(diffs) != 4 { + t.Fatalf("empty create/delete/move diffs = %#v", diffs) + } + for _, diff := range diffs { + if diff.Path == "" || (!diff.OldExists && !diff.NewExists) { + t.Fatalf("invalid diff = %#v", diff) + } + } + + large := strings.Repeat("x", maxToolResultFileDiffBytes/3+1) + budgeted := fileDiffsFromStructuredPatch(".", []structuredPatchChange{ + {kind: structuredPatchAdd, to: structuredPatchTarget{absolute: filepath.Join(root, "one")}, after: large}, + {kind: structuredPatchAdd, to: structuredPatchTarget{absolute: filepath.Join(root, "two")}, after: "tiny"}, + {kind: structuredPatchAdd, to: structuredPatchTarget{absolute: filepath.Join(root, "three")}, after: large}, + {kind: structuredPatchAdd, to: structuredPatchTarget{absolute: filepath.Join(root, "four")}, after: large}, + }) + if len(budgeted) != 3 || filepath.Base(budgeted[0].Path) != "one" || filepath.Base(budgeted[1].Path) != "two" || filepath.Base(budgeted[2].Path) != "three" { + t.Fatalf("ordered aggregate file-diff budget = %#v", budgeted) + } +} + +func TestStructuredPatchFileDiffsKeepEligibleSameBasenameSibling(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + diffs := fileDiffsFromStructuredPatch(".", []structuredPatchChange{ + { + kind: structuredPatchAdd, + to: structuredPatchTarget{absolute: filepath.Join(root, "a.go"), relative: "a.go"}, + after: strings.Repeat("x", maxToolPreviewBytes+1), + }, + { + kind: structuredPatchAdd, + to: structuredPatchTarget{absolute: filepath.Join(root, "sub", "a.go"), relative: filepath.Join("sub", "a.go")}, + after: "package sub\n", + }, + }) + if len(diffs) != 1 || diffs[0].Path != filepath.Join(root, "sub", "a.go") { + t.Fatalf("same-basename rich diffs = %#v", diffs) + } +} + +func TestBoundedDiffPreservesOrdinaryUnicodeButRejectsObfuscatedSecrets(t *testing.T) { + path := filepath.Join(t.TempDir(), "unicode.txt") + for name, content := range map[string]string{ + "family emoji": "family: ๐Ÿ‘จโ€๐Ÿ‘ฉโ€๐Ÿ‘งโ€๐Ÿ‘ฆ\n", + "nonbreaking space": "ordinary\u00a0prose\n", + "byte order mark": "\ufeffdocument\n", + "soft hyphen": "co\u00adoperate\n", + } { + t.Run(name, func(t *testing.T) { + if _, ok := boundedFileDiff(path, "before\n", content, true, true); !ok { + t.Fatalf("ordinary Unicode content was rejected: %q", content) + } + if preview := boundedUnifiedDiff("unicode.txt", "before\n", content); preview == "" { + t.Fatal("ordinary Unicode preview was omitted") + } + }) + } + + secret := "sk-ant-api03-AAAABBBBCCCCDDDDEEEEFFFFGGGG" + for name, separator := range map[string]string{ + "zero width space": "\u200b", + "zero width joiner": "\u200d", + "byte order mark": "\ufeff", + "soft hyphen": "\u00ad", + "nonbreaking space": "\u00a0", + } { + t.Run("split "+name, func(t *testing.T) { + obfuscated := secret[:20] + separator + secret[20:] + if _, ok := boundedFileDiff(path, "before\n", obfuscated, true, true); ok { + t.Fatalf("obfuscated credential produced a rich diff: %q", obfuscated) + } + if preview := boundedUnifiedDiff("secret.txt", "before\n", obfuscated); preview != "" { + t.Fatalf("obfuscated credential produced preview: %q", preview) + } + }) + } +} + +func TestWriteFileMarksSuppressedObfuscatedSecretAsRedacted(t *testing.T) { + root := t.TempDir() + secret := "sk-ant-api03-AAAABBBBCCCCDDDDEEEEFFFFGGGG" + obfuscated := secret[:20] + "\u200b" + secret[20:] + result := NewScopedWriteFileTool(root, nil).Run(t.Context(), map[string]any{ + "path": "secret.txt", "content": obfuscated, + }) + if result.Status != StatusOK || len(result.ChangedFiles) != 1 || len(result.FileDiffs) != 0 || !result.Redacted { + t.Fatalf("obfuscated-secret result = status=%s changed=%#v diffs=%#v redacted=%t", result.Status, result.ChangedFiles, result.FileDiffs, result.Redacted) + } +} diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index dc70b01da..35e0252f2 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -80,6 +80,10 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any } } content := string(contentBytes) + priorInfo, err := os.Stat(absolutePath) + if err != nil { + return errorResult("Error reading " + relativePath + ": " + err.Error()) + } occurrences := strings.Count(content, oldString) // CRLF fallback: read_file normalizes \r\n โ†’ \n before presenting content to @@ -153,18 +157,20 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any if err := recheckScopedWriteTarget(tool.workspaceRoot, tool.scope, requestedPath); err != nil { return errorResult("Error writing " + relativePath + ": " + err.Error()) } - if err := os.WriteFile(absolutePath, []byte(updated), 0o644); err != nil { + if err := commitFileContents(absolutePath, priorInfo, &content, updated); err != nil { return errorResult("Error writing " + relativePath + ": " + err.Error()) } modelKnownContent := updated // Optional format-on-write (ZERO_FORMAT_ON_WRITE). Must run BEFORE the // FileTracker re-baseline: recording pre-format content would make the very // next edit look like an external modification and trip the conflict guard. - updated = maybeFormatWrittenFile(ctx, absolutePath, updated) + updated, finalContentKnown := maybeFormatWrittenFile(ctx, absolutePath, updated) // Re-baseline to the content we just wrote so subsequent edits in this session // compare against the current on-disk state, not the pre-edit version. newInfo, _ := os.Stat(absolutePath) - if updated == modelKnownContent { + if !finalContentKnown { + options.FileTracker.Forget(absolutePath) + } else if updated == modelKnownContent { // OUR edit, so we know precisely which lines moved: RecordEdit carries // across the reads this edit did not disturb instead of dropping them. // @@ -196,9 +202,20 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) result := okResult(summary) result.ChangedFiles = []string{relativePath} + if finalContentKnown { + if diff, ok := boundedFileDiff(absolutePath, content, updated, true, true); ok { + result.FileDiffs = []FileDiff{diff} + } else if diffTextRevealsObfuscatedSecret(content) || diffTextRevealsObfuscatedSecret(updated) { + result.Redacted = true + } + } // Card-only preview (Display.Preview): the model's Output stays the one-line // summary, so the red/green diff costs zero model tokens. - result.Display = Display{Summary: fmt.Sprintf("Edited %s", relativePath), Kind: "diff", Preview: boundedUnifiedDiff(relativePath, content, updated)} + preview := "" + if finalContentKnown { + preview = boundedUnifiedDiff(relativePath, content, updated) + } + result.Display = Display{Summary: fmt.Sprintf("Edited %s", relativePath), Kind: "diff", Preview: preview} return result } diff --git a/internal/tools/file_commit.go b/internal/tools/file_commit.go new file mode 100644 index 000000000..e14c0493d --- /dev/null +++ b/internal/tools/file_commit.go @@ -0,0 +1,103 @@ +package tools + +import ( + "errors" + "fmt" + "io" + "os" +) + +var errFileChangedDuringWrite = errors.New("file changed on disk before the write committed") + +// fileWriteBeforeCommit is a deterministic test hook. Production leaves it +// nil; tests use it to replace a path after observation but before opening the +// object that will actually be mutated. +var fileWriteBeforeCommit func(path string) + +// commitFileContents binds an overwrite to the file identity and bytes that +// the caller observed. A create uses exclusive creation. An overwrite opens the +// observed object without truncation, verifies identity/content through that +// handle, then truncates and writes the same handle. A path replacement before +// or during commit therefore fails instead of publishing stale rich evidence. +// +// expectedInfo nil means the caller observed a missing path. expectedContent +// may be nil for an existing but unreadable file; that path may still be +// overwritten, but callers must omit rich before/after evidence. +func commitFileContents(path string, expectedInfo os.FileInfo, expectedContent *string, content string) error { + if fileWriteBeforeCommit != nil { + fileWriteBeforeCommit(path) + } + + if expectedInfo == nil { + file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) + if err != nil { + return err + } + return writeAndVerifyFileIdentity(path, file, content, false) + } + + flags := os.O_WRONLY + if expectedContent != nil { + flags = os.O_RDWR + } + file, err := os.OpenFile(path, flags, 0) + if err != nil { + return err + } + openedInfo, err := file.Stat() + if err != nil { + _ = file.Close() + return err + } + if !os.SameFile(expectedInfo, openedInfo) { + _ = file.Close() + return errFileChangedDuringWrite + } + pathInfo, err := os.Stat(path) + if err != nil || !os.SameFile(openedInfo, pathInfo) { + _ = file.Close() + return errFileChangedDuringWrite + } + if expectedContent != nil { + current, readErr := io.ReadAll(file) + if readErr != nil { + _ = file.Close() + return readErr + } + if string(current) != *expectedContent { + _ = file.Close() + return errFileChangedDuringWrite + } + } + return writeAndVerifyFileIdentity(path, file, content, true) +} + +func writeAndVerifyFileIdentity(path string, file *os.File, content string, truncate bool) error { + openedInfo, err := file.Stat() + if err != nil { + _ = file.Close() + return err + } + if truncate { + if err := file.Truncate(0); err != nil { + _ = file.Close() + return err + } + if _, err := file.Seek(0, io.SeekStart); err != nil { + _ = file.Close() + return err + } + } + if _, err := io.WriteString(file, content); err != nil { + _ = file.Close() + return err + } + if err := file.Close(); err != nil { + return err + } + pathInfo, err := os.Stat(path) + if err != nil || !os.SameFile(openedInfo, pathInfo) { + return fmt.Errorf("%w: path identity changed", errFileChangedDuringWrite) + } + return nil +} diff --git a/internal/tools/file_commit_test.go b/internal/tools/file_commit_test.go new file mode 100644 index 000000000..c85719537 --- /dev/null +++ b/internal/tools/file_commit_test.go @@ -0,0 +1,81 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +func installFileWriteRace(t *testing.T, mutate func(string)) { + t.Helper() + prior := fileWriteBeforeCommit + fileWriteBeforeCommit = mutate + t.Cleanup(func() { fileWriteBeforeCommit = prior }) +} + +func TestWriteFileRefusesCreateAndOverwriteRaces(t *testing.T) { + t.Run("create", func(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "created.txt") + installFileWriteRace(t, func(path string) { + if err := os.WriteFile(path, []byte("other writer\n"), 0o644); err != nil { + t.Fatal(err) + } + }) + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "created.txt", "content": "zero\n", + }) + if result.Status != StatusError { + t.Fatalf("raced create status = %s, want error", result.Status) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "other writer\n" { + t.Fatalf("raced create content = %q, err=%v", got, err) + } + }) + + t.Run("overwrite", func(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + installFileWriteRace(t, func(path string) { + if err := os.WriteFile(path, []byte("other writer\n"), 0o644); err != nil { + t.Fatal(err) + } + }) + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "content": "zero\n", "overwrite": true, + }) + if result.Status != StatusError || !strings.Contains(result.Output, errFileChangedDuringWrite.Error()) { + t.Fatalf("raced overwrite = %s: %s", result.Status, result.Output) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "other writer\n" { + t.Fatalf("raced overwrite content = %q, err=%v", got, err) + } + }) +} + +func TestEditFileRefusesPreimageRace(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + installFileWriteRace(t, func(path string) { + if err := os.WriteFile(path, []byte("other writer\n"), 0o644); err != nil { + t.Fatal(err) + } + }) + result := NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "old_string": "observed", "new_string": "zero", + }) + if result.Status != StatusError || !strings.Contains(result.Output, errFileChangedDuringWrite.Error()) { + t.Fatalf("raced edit = %s: %s", result.Status, result.Output) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "other writer\n" { + t.Fatalf("raced edit content = %q, err=%v", got, err) + } +} diff --git a/internal/tools/format_on_write.go b/internal/tools/format_on_write.go index cb5bc6159..2e4267f4d 100644 --- a/internal/tools/format_on_write.go +++ b/internal/tools/format_on_write.go @@ -65,35 +65,40 @@ func formatOnWriteEnabled() bool { return value != "" && value != "0" && !strings.EqualFold(value, "false") } +var runFormatOnWriteCommand = func(ctx context.Context, binaryPath string, arguments []string, directory string) error { + formatter := exec.CommandContext(ctx, binaryPath, arguments...) + formatter.Dir = directory + formatter.Stdin = strings.NewReader("") + return formatter.Run() +} + +var readFormattedFile = os.ReadFile + // maybeFormatWrittenFile runs the configured formatter for absolutePath (when -// enabled and on PATH) and returns the file's content afterwards. Best-effort -// throughout: any failure โ€” no formatter, formatter error, timeout, unreadable -// result โ€” returns writtenContent so the caller's state matches the last write -// it performed itself. -func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenContent string) string { +// enabled and on PATH) and returns the verified file content afterwards. A +// formatter may mutate the file and then fail or time out, so its process error +// never substitutes the originally requested bytes for a final read. The bool +// is false only when a formatter ran and the resulting file could not be read; +// callers keep ChangedFiles but omit exact rich evidence in that case. +func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenContent string) (string, bool) { if !formatOnWriteEnabled() { - return writtenContent + return writtenContent, true } command, ok := formatterCommands[strings.ToLower(filepath.Ext(absolutePath))] if !ok { - return writtenContent + return writtenContent, true } binaryPath, err := exec.LookPath(command[0]) if err != nil { - return writtenContent + return writtenContent, true } formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout) defer cancel() arguments := append(append([]string(nil), command[1:]...), absolutePath) - formatter := exec.CommandContext(formatCtx, binaryPath, arguments...) - formatter.Dir = filepath.Dir(absolutePath) - formatter.Stdin = strings.NewReader("") - if err := formatter.Run(); err != nil { - return writtenContent - } - formatted, err := os.ReadFile(absolutePath) + _ = runFormatOnWriteCommand(formatCtx, binaryPath, arguments, filepath.Dir(absolutePath)) + formatted, err := readFormattedFile(absolutePath) if err != nil { - return writtenContent + return writtenContent, false } - return string(formatted) + return string(formatted), true } diff --git a/internal/tools/format_on_write_test.go b/internal/tools/format_on_write_test.go index acca3e868..92dfa84d5 100644 --- a/internal/tools/format_on_write_test.go +++ b/internal/tools/format_on_write_test.go @@ -97,8 +97,8 @@ func TestFormatOnWriteFormatsAndKeepsTrackerConsistent(t *testing.T) { func TestFormatOnWriteSkipsUnknownExtensions(t *testing.T) { t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - content := maybeFormatWrittenFile(context.Background(), filepath.Join(t.TempDir(), "notes.xyz"), "raw text") - if content != "raw text" { + content, known := maybeFormatWrittenFile(context.Background(), filepath.Join(t.TempDir(), "notes.xyz"), "raw text") + if content != "raw text" || !known { t.Fatalf("unknown extension must pass through: %q", content) } } @@ -111,8 +111,157 @@ func TestFormatOnWriteFormatterLookupFailure(t *testing.T) { if err := os.WriteFile(targetPath, []byte(uglyContent), 0o644); err != nil { t.Fatal(err) } - content := maybeFormatWrittenFile(context.Background(), targetPath, uglyContent) - if content != uglyContent { + content, known := maybeFormatWrittenFile(context.Background(), targetPath, uglyContent) + if content != uglyContent || !known { t.Fatalf("missing formatter must return written content, got %q", content) } } + +func TestFormatOnWriteReadsMutatedFileAfterFormatterFailure(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + targetPath := filepath.Join(t.TempDir(), "a.go") + if err := os.WriteFile(targetPath, []byte("requested"), 0o644); err != nil { + t.Fatal(err) + } + priorRunner := runFormatOnWriteCommand + runFormatOnWriteCommand = func(_ context.Context, _ string, _ []string, _ string) error { + if err := os.WriteFile(targetPath, []byte("formatter-mutated"), 0o644); err != nil { + t.Fatal(err) + } + return exec.ErrNotFound + } + t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) + + content, known := maybeFormatWrittenFile(context.Background(), targetPath, "requested") + if !known || content != "formatter-mutated" { + t.Fatalf("formatter failure content = %q, known=%t", content, known) + } +} + +func TestFormatOnWriteMarksUnreadableFinalStateUnknown(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + targetPath := filepath.Join(t.TempDir(), "a.go") + if err := os.WriteFile(targetPath, []byte("requested"), 0o644); err != nil { + t.Fatal(err) + } + priorRunner := runFormatOnWriteCommand + priorReader := readFormattedFile + runFormatOnWriteCommand = func(context.Context, string, []string, string) error { return nil } + readFormattedFile = func(string) ([]byte, error) { return nil, os.ErrPermission } + t.Cleanup(func() { + runFormatOnWriteCommand = priorRunner + readFormattedFile = priorReader + }) + + content, known := maybeFormatWrittenFile(context.Background(), targetPath, "requested") + if known || content != "requested" { + t.Fatalf("unreadable formatter result = %q, known=%t", content, known) + } +} + +func TestWriteFileUsesVerifiedBytesAfterFormatterFailure(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + root := t.TempDir() + targetPath := filepath.Join(root, "a.go") + priorRunner := runFormatOnWriteCommand + runFormatOnWriteCommand = func(_ context.Context, _ string, _ []string, _ string) error { + if err := os.WriteFile(targetPath, []byte("formatter-mutated\n"), 0o644); err != nil { + t.Fatal(err) + } + return exec.ErrNotFound + } + t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "a.go", "content": "requested\n", + }) + if result.Status != StatusOK { + t.Fatalf("write status = %s: %s", result.Status, result.Output) + } + if got := result.FileDiffs; len(got) != 1 || got[0].NewText != "formatter-mutated\n" { + t.Fatalf("formatter-failure FileDiff = %#v", got) + } +} + +func TestEditFileUsesVerifiedBytesAfterFormatterFailure(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + root := t.TempDir() + targetPath := filepath.Join(root, "a.go") + if err := os.WriteFile(targetPath, []byte("before\n"), 0o644); err != nil { + t.Fatal(err) + } + priorRunner := runFormatOnWriteCommand + runFormatOnWriteCommand = func(_ context.Context, _ string, _ []string, _ string) error { + if err := os.WriteFile(targetPath, []byte("formatter-mutated\n"), 0o644); err != nil { + t.Fatal(err) + } + return exec.ErrNotFound + } + t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) + + result := NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "a.go", "old_string": "before", "new_string": "requested", + }) + if result.Status != StatusOK { + t.Fatalf("edit status = %s: %s", result.Status, result.Output) + } + if got := result.FileDiffs; len(got) != 1 || got[0].OldText != "before\n" || got[0].NewText != "formatter-mutated\n" { + t.Fatalf("formatter-failure edit FileDiff = %#v", got) + } +} + +func TestWriteFileOmitsRichDiffWhenFormatterFinalReadFails(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + root := t.TempDir() + priorRunner := runFormatOnWriteCommand + priorReader := readFormattedFile + runFormatOnWriteCommand = func(context.Context, string, []string, string) error { return exec.ErrNotFound } + readFormattedFile = func(string) ([]byte, error) { return nil, os.ErrPermission } + t.Cleanup(func() { + runFormatOnWriteCommand = priorRunner + readFormattedFile = priorReader + }) + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "a.go", "content": "requested\n", + }) + if result.Status != StatusOK || len(result.ChangedFiles) != 1 || len(result.FileDiffs) != 0 { + t.Fatalf("unverified formatter result = status=%s changed=%#v diffs=%#v", result.Status, result.ChangedFiles, result.FileDiffs) + } + if result.Display.Preview != "" { + t.Fatalf("unverified formatter result exposed stale preview: %q", result.Display.Preview) + } +} + +func TestEditFileOmitsPreviewWhenFormatterFinalReadFails(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + root := t.TempDir() + targetPath := filepath.Join(root, "a.go") + if err := os.WriteFile(targetPath, []byte("before\n"), 0o644); err != nil { + t.Fatal(err) + } + priorRunner := runFormatOnWriteCommand + priorReader := readFormattedFile + runFormatOnWriteCommand = func(context.Context, string, []string, string) error { return nil } + readFormattedFile = func(string) ([]byte, error) { return nil, os.ErrPermission } + t.Cleanup(func() { + runFormatOnWriteCommand = priorRunner + readFormattedFile = priorReader + }) + + result := NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "a.go", "old_string": "before", "new_string": "requested", + }) + if result.Status != StatusOK || len(result.ChangedFiles) != 1 || len(result.FileDiffs) != 0 { + t.Fatalf("unverified formatter result = status=%s changed=%#v diffs=%#v", result.Status, result.ChangedFiles, result.FileDiffs) + } + if result.Display.Preview != "" { + t.Fatalf("unverified formatter result exposed stale preview: %q", result.Display.Preview) + } +} diff --git a/internal/tools/registry.go b/internal/tools/registry.go index e270a67d6..074092ebc 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -343,6 +343,26 @@ func scrubResultSecrets(res Result) Result { res.Display.Preview = scrubbed res.Redacted = true } + fileDiffs := make([]FileDiff, 0, len(res.FileDiffs)) + for _, diff := range res.FileDiffs { + // Never normalize control bytes in a diff: normalizing after redaction can + // reassemble a split credential. Decline unsafe rich content entirely and + // leave ChangedFiles as the safe fallback. + if unsafeDiffText(diff.OldText) || unsafeDiffText(diff.NewText) { + res.Redacted = true + continue + } + if scrubbed := redaction.RedactString(diff.OldText, redaction.Options{}); scrubbed != diff.OldText { + diff.OldText = scrubbed + res.Redacted = true + } + if scrubbed := redaction.RedactString(diff.NewText, redaction.Options{}); scrubbed != diff.NewText { + diff.NewText = scrubbed + res.Redacted = true + } + fileDiffs = append(fileDiffs, diff) + } + res.FileDiffs = fileDiffs // Meta values carry model-controlled strings (e.g. glob pattern, bash cwd) and // are forwarded into the transcript, so they are part of the boundary too. for key, value := range res.Meta { @@ -354,6 +374,13 @@ func scrubResultSecrets(res Result) Result { return res } +// ScrubResultSecrets applies the registry's transcript boundary to a result +// that was produced before Registry.RunWithOptions could own it, such as a +// local pre-permission rejection in the agent loop. +func ScrubResultSecrets(res Result) Result { + return scrubResultSecrets(res) +} + func CoreReadOnlyToolsScoped(workspaceRoot string, scope PathScope) []Tool { return []Tool{ NewScopedReadMinifiedFileTool(workspaceRoot, scope), diff --git a/internal/tools/registry_test.go b/internal/tools/registry_test.go index c71cabdd7..145f15388 100644 --- a/internal/tools/registry_test.go +++ b/internal/tools/registry_test.go @@ -445,13 +445,51 @@ func (t denyTool) Run(context.Context, map[string]any) Result { return Result{St // must be scrubbed too, not just the tool-execution paths. func TestScrubResultSecretsRedactsPreview(t *testing.T) { secret := "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" - res := scrubResultSecrets(Result{Display: Display{Preview: "+++ b/x\n+token := \"" + secret + "\""}}) + res := scrubResultSecrets(Result{ + Display: Display{Preview: "+++ b/x\n+token := \"" + secret + "\""}, + FileDiffs: []FileDiff{{Path: filepath.Join(t.TempDir(), "x"), OldExists: true, NewExists: true, OldText: secret, NewText: secret}}, + }) if strings.Contains(res.Display.Preview, secret) { t.Errorf("Display.Preview (the card-only code preview) must be redacted, leaked: %q", res.Display.Preview) } if !res.Redacted { t.Error("scrubbing a secret from the preview should set Redacted") } + if strings.Contains(res.FileDiffs[0].OldText, secret) || strings.Contains(res.FileDiffs[0].NewText, secret) { + t.Errorf("FileDiff must be redacted: %#v", res.FileDiffs) + } +} + +func TestScrubResultSecretsDropsControlSplitFileDiff(t *testing.T) { + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + res := scrubResultSecrets(Result{FileDiffs: []FileDiff{{ + Path: filepath.Join(t.TempDir(), "x"), + OldExists: true, + NewExists: true, + OldText: "token=" + secret[:12] + "\x00" + secret[12:], + NewText: "safe", + }}}) + if len(res.FileDiffs) != 0 || !res.Redacted { + t.Fatalf("unsafe FileDiff = %#v, redacted = %t", res.FileDiffs, res.Redacted) + } +} + +func TestScrubResultSecretsDoesNotMutateCallerFileDiffSlice(t *testing.T) { + path := filepath.Join(t.TempDir(), "x") + secret := "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + retainedOld := "before token=" + secret + retainedNew := "after token=" + secret + original := []FileDiff{ + {Path: path, OldExists: true, NewExists: true, OldText: "token=sk-proj-abc\x00def", NewText: "unsafe"}, + {Path: path, OldExists: true, NewExists: true, OldText: retainedOld, NewText: retainedNew}, + } + result := scrubResultSecrets(Result{FileDiffs: original}) + if len(result.FileDiffs) != 1 || strings.Contains(result.FileDiffs[0].OldText, secret) || strings.Contains(result.FileDiffs[0].NewText, secret) { + t.Fatalf("filtered FileDiffs = %#v", result.FileDiffs) + } + if original[0].NewText != "unsafe" || original[1].OldText != retainedOld || original[1].NewText != retainedNew { + t.Fatalf("caller slice was mutated: %#v", original) + } } func TestRunWithOptionsScrubsSecretsOnDenialPaths(t *testing.T) { diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index 653f0cbb5..4bf938f4b 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -156,8 +156,15 @@ func applyPatchOperations(applyRoot, relativeRoot string, operations []structure } } } - if err := applyStructuredPatchChanges(workspace, changes, options.FileTracker); err != nil { - return errorResult("Error applying patch: " + err.Error()) + applyOutcome, err := applyStructuredPatchChanges(workspace, changes, options.FileTracker) + if err != nil { + result := errorResult("Error applying patch: " + err.Error()) + result.ChangedFiles = changedFilesFromStructuredPatch(relativeRoot, applyOutcome.committed) + result.ChangedFiles = appendUniqueStructuredPatchPaths(result.ChangedFiles, relativeRoot, applyOutcome.incompletePaths) + result.FileDiffs = fileDiffsFromStructuredPatch(relativeRoot, applyOutcome.committed) + result.Redacted = structuredPatchContainsObfuscatedSecret(applyOutcome.committed) + result.Display = Display{Summary: result.Output, Kind: "diff", Preview: structuredPatchPreview(applyOutcome.committed)} + return result } for _, change := range changes { @@ -183,10 +190,78 @@ func applyPatchOperations(applyRoot, relativeRoot string, operations []structure } result := okResult(summary) result.ChangedFiles = changedFilesFromStructuredPatch(relativeRoot, changes) + result.FileDiffs = fileDiffsFromStructuredPatch(relativeRoot, changes) + result.Redacted = structuredPatchContainsObfuscatedSecret(changes) result.Display = Display{Summary: summary, Kind: "diff", Preview: structuredPatchPreview(changes)} return result } +func structuredPatchContainsObfuscatedSecret(changes []structuredPatchChange) bool { + for _, change := range changes { + if diffTextRevealsObfuscatedSecret(change.before) || diffTextRevealsObfuscatedSecret(change.after) { + return true + } + } + return false +} + +func fileDiffsFromStructuredPatch(_ string, changes []structuredPatchChange) []FileDiff { + const maxToolResultFileDiffs = 64 + diffs := make([]FileDiff, 0, len(changes)*2) + usedBytes := 0 + appendGroup := func(group ...FileDiff) bool { + if len(group) == 0 || len(diffs)+len(group) > maxToolResultFileDiffs { + return false + } + groupBytes := 0 + for _, diff := range group { + groupBytes += len(diff.OldText) + len(diff.NewText) + } + if usedBytes+groupBytes > maxToolResultFileDiffBytes { + return false + } + diffs = append(diffs, group...) + usedBytes += groupBytes + return true + } + makeDiff := func(path, before, after string, oldExists, newExists bool) (FileDiff, bool) { + return boundedFileDiff(path, before, after, oldExists, newExists) + } + for _, change := range changes { + var group []FileDiff + switch { + case change.kind == structuredPatchDelete: + if diff, ok := makeDiff(change.from.absolute, change.before, "", true, false); ok { + group = append(group, diff) + } + case change.kind == structuredPatchAdd: + if diff, ok := makeDiff(change.to.absolute, "", change.after, false, true); ok { + group = append(group, diff) + } + case change.kind == structuredPatchCopy && change.from.absolute != change.to.absolute: + // A copy leaves its source unchanged; the destination is a create. + if diff, ok := makeDiff(change.to.absolute, "", change.after, false, true); ok { + group = append(group, diff) + } + case change.kind == structuredPatchUpdate && change.from.absolute != change.to.absolute: + // A move is two filesystem changes, not a destination overwrite. + from, fromOK := makeDiff(change.from.absolute, change.before, "", true, false) + to, toOK := makeDiff(change.to.absolute, "", change.after, false, true) + if fromOK && toOK { + group = append(group, from, to) + } + default: + if diff, ok := makeDiff(change.to.absolute, change.before, change.after, true, true); ok { + group = append(group, diff) + } + } + if len(group) > 0 && !appendGroup(group...) { + break + } + } + return diffs +} + func parseStructuredPatch(patch string) ([]structuredPatchOperation, error) { normalized := strings.TrimSpace(strings.TrimPrefix(strings.ReplaceAll(patch, "\r\n", "\n"), "\ufeff")) if normalized == "" { @@ -672,38 +747,52 @@ func findStructuredPatchSequence(lines, wanted []string, start int, endOfFile bo return -1, false } -func applyStructuredPatchChanges(root *os.Root, changes []structuredPatchChange, tracker *FileTracker) error { +type structuredPatchApplyOutcome struct { + committed []structuredPatchChange + incompletePaths []string +} + +func applyStructuredPatchChanges(root *os.Root, changes []structuredPatchChange, tracker *FileTracker) (structuredPatchApplyOutcome, error) { // committed lists, in order, the paths whose change reached disk before a // later change failed, so the caller (and the model) knows exactly which // files now hold the patched content and which were never touched. - var committed []string + var outcome structuredPatchApplyOutcome for _, change := range changes { done, err := applyStructuredPatchChange(root, change) - if done { - committed = append(committed, structuredPatchChangePaths(change)...) - } if err != nil { + if done && change.to.relative != "" { + outcome.incompletePaths = append(outcome.incompletePaths, change.to.relative) + } forgetStructuredPatchFiles(tracker, changes) - if len(committed) > 0 { - return fmt.Errorf("%w; patch was partially applied โ€” already committed: %s; the remaining files are unchanged; re-read the committed files before retrying", err, strings.Join(committed, ", ")) + committedPaths := changedFilesFromStructuredPatch(".", outcome.committed) + committedPaths = appendUniqueStructuredPatchPaths(committedPaths, ".", outcome.incompletePaths) + if len(committedPaths) > 0 { + return outcome, fmt.Errorf("%w; patch was partially applied โ€” already committed: %s; the remaining files are unchanged; re-read the committed files before retrying", err, strings.Join(committedPaths, ", ")) } - return err + return outcome, err + } + if done { + outcome.committed = append(outcome.committed, change) } } - return nil + return outcome, nil } -// structuredPatchChangePaths names the workspace-relative paths a committed -// change touched: the destination, plus the source of a move or copy. -func structuredPatchChangePaths(change structuredPatchChange) []string { - if change.kind == structuredPatchDelete { - return []string{change.from.relative} - } - paths := []string{change.to.relative} - if change.from.absolute != change.to.absolute && change.from.relative != "" { - paths = append([]string{change.from.relative}, paths...) +func appendUniqueStructuredPatchPaths(existing []string, relativeRoot string, paths []string) []string { + seen := make(map[string]bool, len(existing)+len(paths)) + for _, path := range existing { + seen[path] = true + } + for _, path := range paths { + if relativeRoot != "" && relativeRoot != "." { + path = filepath.ToSlash(filepath.Join(relativeRoot, path)) + } + if path != "" && !seen[path] { + seen[path] = true + existing = append(existing, path) + } } - return paths + return existing } func forgetStructuredPatchFiles(tracker *FileTracker, changes []structuredPatchChange) { @@ -914,9 +1003,19 @@ func changedFilesFromStructuredPatch(relativeRoot string, changes []structuredPa seen := make(map[string]bool) var paths []string for _, change := range changes { - targets := []structuredPatchTarget{change.to} - if change.from.absolute != change.to.absolute { - targets = append([]structuredPatchTarget{change.from}, targets...) + var targets []structuredPatchTarget + switch change.kind { + case structuredPatchDelete: + targets = []structuredPatchTarget{change.from} + case structuredPatchUpdate: + targets = []structuredPatchTarget{change.to} + if change.from.absolute != change.to.absolute { + targets = append([]structuredPatchTarget{change.from}, targets...) + } + default: + // Adds and copies mutate only their destination; the copy source is + // evidence for the operation, not a changed file. + targets = []structuredPatchTarget{change.to} } for _, target := range targets { path := target.relative diff --git a/internal/tools/types.go b/internal/tools/types.go index 27755d8d4..c5338e203 100644 --- a/internal/tools/types.go +++ b/internal/tools/types.go @@ -120,6 +120,10 @@ type Result struct { // entries under a granted extra write root are absolute, since // workspace-relative would be ambiguous there. ChangedFiles []string + // FileDiffs carries exact, bounded before/after text for built-in file + // mutations. A missing entry means the client must fall back to ChangedFiles + // rather than inventing a partial diff. + FileDiffs []FileDiff // ChangeSummaries contains bounded generated-tree changes. These are shown // in session evidence and the Files panel but are never treated as files to // open or diagnose individually. diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 76f5f1baa..de6e60d29 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -12,6 +12,7 @@ type writeFileTool struct { baseTool workspaceRoot string scope PathScope + readFile func(string) ([]byte, error) } func NewScopedWriteFileTool(workspaceRoot string, scope PathScope) Tool { @@ -34,6 +35,7 @@ func NewScopedWriteFileTool(workspaceRoot string, scope PathScope) Tool { }, workspaceRoot: normalizeWorkspaceRoot(workspaceRoot), scope: scope, + readFile: os.ReadFile, } } @@ -61,8 +63,10 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an } existed := false - if _, err := os.Stat(absolutePath); err == nil { + var priorInfo os.FileInfo + if info, err := os.Stat(absolutePath); err == nil { existed = true + priorInfo = info if !overwrite { return errorResult("Error: " + relativePath + " already exists. Pass overwrite: true to replace it.") } @@ -82,7 +86,7 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an // Fail CLOSED: if the tracked file can't be re-read to verify it, refuse // the overwrite rather than clobbering a file whose current state is // unknown (it may have been replaced or removed out from under us). - current, rerr := os.ReadFile(absolutePath) + current, rerr := tool.readFile(absolutePath) if rerr != nil { return errorResult(fileConflictMessage(relativePath)) } @@ -95,9 +99,11 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an // Capture the prior content (before we replace it) so an overwrite can show a // real diff; a fresh create stays "" and previews as all-additions. priorContent := "" + priorContentKnown := !existed if existed { - if prev, rerr := os.ReadFile(absolutePath); rerr == nil { + if prev, rerr := tool.readFile(absolutePath); rerr == nil { priorContent = string(prev) + priorContentKnown = true } } @@ -107,19 +113,27 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an if err := recheckScopedWriteTarget(tool.workspaceRoot, tool.scope, requestedPath); err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) } - if err := os.WriteFile(absolutePath, []byte(content), 0o644); err != nil { + var expectedContent *string + if priorContentKnown { + expectedContent = &priorContent + } + if err := commitFileContents(absolutePath, priorInfo, expectedContent, content); err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) } modelKnownContent := content // Optional format-on-write (ZERO_FORMAT_ON_WRITE). Must run BEFORE the // FileTracker baseline: recording pre-format content would make the very // next edit look like an external modification and trip the conflict guard. - content = maybeFormatWrittenFile(ctx, absolutePath, content) + content, finalContentKnown := maybeFormatWrittenFile(ctx, absolutePath, content) // Baseline the freshly written content so a later edit/overwrite in this // session compares against what is now on disk. newInfo, _ := os.Stat(absolutePath) - options.FileTracker.Record(absolutePath, []byte(content), newInfo) - if content == modelKnownContent { + if finalContentKnown { + options.FileTracker.Record(absolutePath, []byte(content), newInfo) + } else { + options.FileTracker.Forget(absolutePath) + } + if finalContentKnown && content == modelKnownContent { options.FileTracker.RecordSeenRange(absolutePath, 1, trackedLineTotal(content), trackedLineTotal(content)) } if !existed { @@ -140,10 +154,23 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) result := okResult(summary) result.ChangedFiles = []string{relativePath} + // Do not pretend an unreadable overwrite was a creation. The write may be + // valid, but ACP only receives an exact before/after pair we actually saw. + if priorContentKnown && finalContentKnown { + if diff, ok := boundedFileDiff(absolutePath, priorContent, content, existed, true); ok { + result.FileDiffs = []FileDiff{diff} + } else if diffTextRevealsObfuscatedSecret(priorContent) || diffTextRevealsObfuscatedSecret(content) { + result.Redacted = true + } + } // Card-only preview: a real unified diff (all-green for a create, red/green for // an overwrite) on Display.Preview. Output stays the summary, so the model never // re-reads the file โ€” the rich preview costs zero model tokens. - result.Display = Display{Summary: summary, Kind: "file", Preview: boundedUnifiedDiff(relativePath, priorContent, content)} + preview := "" + if priorContentKnown && finalContentKnown { + preview = boundedUnifiedDiff(relativePath, priorContent, content) + } + result.Display = Display{Summary: summary, Kind: "file", Preview: preview} return result } diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 87849e859..22a134a41 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -6,6 +6,7 @@ import ( "io" "os" "path/filepath" + "reflect" "strings" "testing" ) @@ -323,6 +324,10 @@ func TestEditFileToolReplacesExactStrings(t *testing.T) { root := t.TempDir() path := filepath.Join(root, "code.go") writeTestFile(t, path, "const a = 1\nconst b = 2\n") + path, err := filepath.EvalSymlinks(path) + if err != nil { + t.Fatal(err) + } result := NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ "path": "code.go", @@ -367,7 +372,12 @@ func TestEditFileToolReplacesCRLF(t *testing.T) { func TestEditFileToolEmitsUnifiedDiff(t *testing.T) { root := t.TempDir() - writeTestFile(t, filepath.Join(root, "code.go"), "const a = 1\nconst b = 2\n") + path := filepath.Join(root, "code.go") + writeTestFile(t, path, "const a = 1\nconst b = 2\n") + path, err := filepath.EvalSymlinks(path) + if err != nil { + t.Fatal(err) + } res := NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ "path": "code.go", "old_string": "const a = 1", "new_string": "const a = 42", }) @@ -387,6 +397,9 @@ func TestEditFileToolEmitsUnifiedDiff(t *testing.T) { t.Fatalf("edit preview missing diff marker %q: %q", want, res.Display.Preview) } } + if got := res.FileDiffs; len(got) != 1 || got[0].Path != path || !got[0].OldExists || !got[0].NewExists || got[0].OldText != "const a = 1\nconst b = 2\n" || got[0].NewText != "const a = 42\nconst b = 2\n" { + t.Fatalf("file diffs = %#v", got) + } } func TestWriteFileToolEmitsAdditionsDiff(t *testing.T) { @@ -408,6 +421,13 @@ func TestWriteFileToolEmitsAdditionsDiff(t *testing.T) { if strings.Contains(res.Display.Preview, "\n-line") { t.Fatalf("a fresh-create diff must have no removed lines: %q", res.Display.Preview) } + path, err := filepath.EvalSymlinks(filepath.Join(root, "new.txt")) + if err != nil { + t.Fatal(err) + } + if got := res.FileDiffs; len(got) != 1 || got[0].Path != path || got[0].OldExists || !got[0].NewExists || got[0].OldText != "" || got[0].NewText != "line one\nline two\n" { + t.Fatalf("file diffs = %#v", got) + } } func TestWriteFileToolOverwriteEmitsRedGreenDiff(t *testing.T) { @@ -429,6 +449,28 @@ func TestWriteFileToolOverwriteEmitsRedGreenDiff(t *testing.T) { } } +func TestWriteFileToolOmitsDiffWhenOverwritePreimageCannotBeRead(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, "private.txt") + writeTestFile(t, path, "before\n") + tool := NewScopedWriteFileTool(root, nil).(writeFileTool) + tool.readFile = func(string) ([]byte, error) { return nil, os.ErrPermission } + registry := NewRegistry() + registry.Register(tool) + result := registry.RunWithOptions(context.Background(), tool.Name(), map[string]any{ + "path": "private.txt", "content": "after\n", "overwrite": true, + }, RunOptions{PermissionGranted: true}) + if result.Status != StatusOK { + t.Fatalf("write = %s", result.Output) + } + if len(result.FileDiffs) != 0 { + t.Fatalf("unreadable preimage must not produce a create-like diff: %#v", result.FileDiffs) + } + if got, err := os.ReadFile(path); err != nil || string(got) != "after\n" { + t.Fatalf("written content = %q, err = %v", got, err) + } +} + func TestEditFileToolAllowsDeletingRegions(t *testing.T) { root := t.TempDir() path := filepath.Join(root, "notes.txt") @@ -584,6 +626,17 @@ func TestApplyPatchToolAppliesStructuredAddAndMove(t *testing.T) { if got := result.ChangedFiles; strings.Join(got, ",") != "nested/new.txt,old.txt,moved.txt" { t.Fatalf("ChangedFiles = %v", got) } + resolvedRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatal(err) + } + if got, want := result.FileDiffs, []FileDiff{ + {Path: filepath.Join(resolvedRoot, "nested", "new.txt"), OldExists: false, NewExists: true, OldText: "", NewText: "created\n"}, + {Path: filepath.Join(resolvedRoot, "old.txt"), OldExists: true, NewExists: false, OldText: "old\n", NewText: ""}, + {Path: filepath.Join(resolvedRoot, "moved.txt"), OldExists: false, NewExists: true, OldText: "", NewText: "moved\n"}, + }; !reflect.DeepEqual(got, want) { + t.Fatalf("FileDiffs = %#v, want %#v", got, want) + } } func TestApplyPatchToolStructuredPatchMatchesWhitespaceTolerantly(t *testing.T) { @@ -831,7 +884,7 @@ func TestStructuredPatchAddDoesNotOverwriteRacedDestination(t *testing.T) { after: "patch content\n", mode: 0o644, } - err = applyStructuredPatchChanges(workspace, []structuredPatchChange{change}, nil) + _, err = applyStructuredPatchChanges(workspace, []structuredPatchChange{change}, nil) if err == nil || !errors.Is(err, os.ErrExist) { t.Fatalf("raced add destination = %v, want os.ErrExist", err) } @@ -857,7 +910,7 @@ func TestStructuredPatchFailedDeleteDoesNotRecreateMissingFile(t *testing.T) { before: "removed by another writer\n", mode: 0o644, } - err = applyStructuredPatchChanges(workspace, []structuredPatchChange{change}, nil) + _, err = applyStructuredPatchChanges(workspace, []structuredPatchChange{change}, nil) if err == nil { t.Fatal("delete of an already removed file should fail") } @@ -894,7 +947,7 @@ func TestStructuredPatchMoveWithMissingSourceIsRefusedBeforePublishing(t *testin } defer func() { structuredPatchBeforeCommit = nil }() - err = applyStructuredPatchChanges(workspace, []structuredPatchChange{change}, nil) + _, err = applyStructuredPatchChanges(workspace, []structuredPatchChange{change}, nil) if !removed { t.Fatal("pre-commit hook did not run") } @@ -1017,7 +1070,7 @@ func TestStructuredPatchPartialFailureLeavesCompletedChangeAndClearsTrackedState }, } - err = applyStructuredPatchChanges(workspace, changes, tracker) + _, err = applyStructuredPatchChanges(workspace, changes, tracker) if err == nil || !strings.Contains(err.Error(), "partially applied") { t.Fatalf("second change = %v, want partial-application error", err) }