From 554c75f5a5653c94b9dc8e3eb9f26ff5befcf683 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Mon, 31 Aug 2026 20:36:27 +0530 Subject: [PATCH 01/10] feat(tools): retain bounded structured file diffs --- internal/tools/diff_preview.go | 24 +++++++++++++++++++++- internal/tools/diff_preview_test.go | 28 ++++++++++++++++++++++++++ internal/tools/edit_file.go | 3 +++ internal/tools/registry.go | 11 ++++++++++ internal/tools/registry_test.go | 8 +++++++- internal/tools/structured_patch.go | 31 +++++++++++++++++++++++++++++ internal/tools/types.go | 4 ++++ internal/tools/write_file.go | 3 +++ internal/tools/write_tools_test.go | 14 +++++++++++++ 9 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 internal/tools/diff_preview_test.go diff --git a/internal/tools/diff_preview.go b/internal/tools/diff_preview.go index b2db6c7c9..d3367dd27 100644 --- a/internal/tools/diff_preview.go +++ b/internal/tools/diff_preview.go @@ -1,12 +1,34 @@ package tools -import udiff "github.com/aymanbagabas/go-udiff" +import ( + "unicode/utf8" + + 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 +// 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 string + 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 or unchanged content. +func boundedFileDiff(path, oldText, newText string) (FileDiff, bool) { + if path == "" || oldText == newText || !utf8.ValidString(oldText) || !utf8.ValidString(newText) || len(oldText)+len(newText) > maxToolPreviewBytes { + return FileDiff{}, false + } + return FileDiff{Path: path, OldText: oldText, NewText: newText}, true +} + // 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. diff --git a/internal/tools/diff_preview_test.go b/internal/tools/diff_preview_test.go new file mode 100644 index 000000000..967afff6a --- /dev/null +++ b/internal/tools/diff_preview_test.go @@ -0,0 +1,28 @@ +package tools + +import ( + "strings" + "testing" +) + +func TestBoundedFileDiffRefusesPartialOrBinaryContent(t *testing.T) { + if diff, ok := boundedFileDiff("a.txt", "old", "new"); !ok || diff.Path != "a.txt" || 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})}, + {"too large", strings.Repeat("a", maxToolPreviewBytes), "b"}, + } { + t.Run(tc.name, func(t *testing.T) { + if diff, ok := boundedFileDiff("a.txt", tc.old, tc.new); ok || diff != (FileDiff{}) { + t.Fatalf("unexpected diff = %#v, %t", diff, ok) + } + }) + } +} diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index dc70b01da..0e9b9fe33 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -196,6 +196,9 @@ 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 diff, ok := boundedFileDiff(relativePath, content, updated); ok { + result.FileDiffs = []FileDiff{diff} + } // 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)} diff --git a/internal/tools/registry.go b/internal/tools/registry.go index e270a67d6..ea9ea756b 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -343,6 +343,17 @@ func scrubResultSecrets(res Result) Result { res.Display.Preview = scrubbed res.Redacted = true } + for index := range res.FileDiffs { + diff := &res.FileDiffs[index] + 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 + } + } // 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 { diff --git a/internal/tools/registry_test.go b/internal/tools/registry_test.go index c71cabdd7..57e68ac2b 100644 --- a/internal/tools/registry_test.go +++ b/internal/tools/registry_test.go @@ -445,13 +445,19 @@ 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: "x", 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 TestRunWithOptionsScrubsSecretsOnDenialPaths(t *testing.T) { diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index 653f0cbb5..e8fcaea4b 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -183,10 +183,41 @@ func applyPatchOperations(applyRoot, relativeRoot string, operations []structure } result := okResult(summary) result.ChangedFiles = changedFilesFromStructuredPatch(relativeRoot, changes) + result.FileDiffs = fileDiffsFromStructuredPatch(relativeRoot, changes) result.Display = Display{Summary: summary, Kind: "diff", Preview: structuredPatchPreview(changes)} return result } +func fileDiffsFromStructuredPatch(relativeRoot string, changes []structuredPatchChange) []FileDiff { + diffs := make([]FileDiff, 0, len(changes)*2) + appendDiff := func(path, before, after string) { + if relativeRoot != "" && relativeRoot != "." { + path = filepath.ToSlash(filepath.Join(relativeRoot, path)) + } + if diff, ok := boundedFileDiff(path, before, after); ok { + diffs = append(diffs, diff) + } + } + for _, change := range changes { + switch { + case change.kind == structuredPatchDelete: + appendDiff(change.from.relative, change.before, "") + case change.kind == structuredPatchAdd: + appendDiff(change.to.relative, "", change.after) + case change.kind == structuredPatchCopy && change.from.absolute != change.to.absolute: + // A copy leaves its source unchanged; the destination is a create. + appendDiff(change.to.relative, "", change.after) + case change.kind == structuredPatchUpdate && change.from.absolute != change.to.absolute: + // A move is two filesystem changes, not a destination overwrite. + appendDiff(change.from.relative, change.before, "") + appendDiff(change.to.relative, "", change.after) + default: + appendDiff(change.to.relative, change.before, change.after) + } + } + return diffs +} + func parseStructuredPatch(patch string) ([]structuredPatchOperation, error) { normalized := strings.TrimSpace(strings.TrimPrefix(strings.ReplaceAll(patch, "\r\n", "\n"), "\ufeff")) if normalized == "" { 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..7ed05f0b7 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -140,6 +140,9 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) result := okResult(summary) result.ChangedFiles = []string{relativePath} + if diff, ok := boundedFileDiff(relativePath, priorContent, content); ok { + result.FileDiffs = []FileDiff{diff} + } // 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. diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 87849e859..3d99e4cf7 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" ) @@ -387,6 +388,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 != "code.go" || 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 +412,9 @@ 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) } + if got := res.FileDiffs; len(got) != 1 || got[0].Path != "new.txt" || got[0].OldText != "" || got[0].NewText != "line one\nline two\n" { + t.Fatalf("file diffs = %#v", got) + } } func TestWriteFileToolOverwriteEmitsRedGreenDiff(t *testing.T) { @@ -584,6 +591,13 @@ func TestApplyPatchToolAppliesStructuredAddAndMove(t *testing.T) { if got := result.ChangedFiles; strings.Join(got, ",") != "nested/new.txt,old.txt,moved.txt" { t.Fatalf("ChangedFiles = %v", got) } + if got, want := result.FileDiffs, []FileDiff{ + {Path: "nested/new.txt", OldText: "", NewText: "created\n"}, + {Path: "old.txt", OldText: "old\n", NewText: ""}, + {Path: "moved.txt", OldText: "", NewText: "moved\n"}, + }; !reflect.DeepEqual(got, want) { + t.Fatalf("FileDiffs = %#v, want %#v", got, want) + } } func TestApplyPatchToolStructuredPatchMatchesWhitespaceTolerantly(t *testing.T) { From 4988a07de60ef1b26d8259d67cd4d8e807508185 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Mon, 31 Aug 2026 20:36:27 +0530 Subject: [PATCH 02/10] feat(acp): emit structured file changes in tool updates --- internal/acp/translate.go | 16 ++++++++++++++-- internal/acp/translate_test.go | 6 +++++- internal/agent/loop.go | 3 +++ internal/agent/types.go | 1 + 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/internal/acp/translate.go b/internal/acp/translate.go index 565174904..f1643a74b 100644 --- a/internal/acp/translate.go +++ b/internal/acp/translate.go @@ -120,14 +120,26 @@ 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) + } + content = append(content, ToolContent(TextBlock(text))) + return appendToolResultDiffs(content, result.FileDiffs) +} + +func appendToolResultDiffs(content []ToolCallContent, diffs []tools.FileDiff) []ToolCallContent { + for _, diff := range diffs { + if strings.TrimSpace(diff.Path) == "" || diff.OldText == diff.NewText { + continue + } + content = append(content, ToolCallContent{Type: "diff", Path: diff.Path, OldText: diff.OldText, NewText: diff.NewText}) } - return []ToolCallContent{ToolContent(TextBlock(text))} + return content } func toolResultLocations(result agent.ToolResult) []ToolCallLocation { diff --git a/internal/acp/translate_test.go b/internal/acp/translate_test.go index 4a9adc16d..50433f008 100644 --- a/internal/acp/translate_test.go +++ b/internal/acp/translate_test.go @@ -80,13 +80,17 @@ func TestToolCallResult(t *testing.T) { Status: tools.StatusOK, Output: "applied\n", ChangedFiles: []string{"a.go", ""}, + FileDiffs: []tools.FileDiff{{Path: "a.go", 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 diff := ok.Content[1]; diff.Type != "diff" || diff.Path != "a.go" || diff.OldText != "before\n" || diff.NewText != "after\n" { + t.Fatalf("unexpected diff content: %+v", diff) + } if len(ok.Locations) != 1 || ok.Locations[0].Path != "a.go" { t.Fatalf("blank changed files should be dropped, got %+v", ok.Locations) } diff --git a/internal/agent/loop.go b/internal/agent/loop.go index fe691ac4c..a8e447c5b 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, @@ -1860,6 +1861,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 +2155,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/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 From 66660e330afacd3e1746b5ee11fe296cdec9a131 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Tue, 1 Sep 2026 09:24:12 +0530 Subject: [PATCH 03/10] fix(acp): harden structured file diff transport --- internal/acp/translate.go | 11 +++++- internal/acp/translate_test.go | 46 ++++++++++++++++++++++- internal/acp/types.go | 27 ++++++++++++-- internal/agent/loop.go | 4 ++ internal/agent/loop_test.go | 17 +++++++++ internal/tools/diff_preview.go | 39 +++++++++++++++---- internal/tools/diff_preview_test.go | 58 ++++++++++++++++++++++++++++- internal/tools/edit_file.go | 2 +- internal/tools/registry.go | 20 +++++++++- internal/tools/registry_test.go | 16 +++++++- internal/tools/structured_patch.go | 48 ++++++++++++++++++------ internal/tools/write_file.go | 16 ++++++-- internal/tools/write_tools_test.go | 51 ++++++++++++++++++++++--- 13 files changed, 313 insertions(+), 42 deletions(-) diff --git a/internal/acp/translate.go b/internal/acp/translate.go index f1643a74b..06980b62e 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" @@ -134,10 +135,16 @@ func toolResultContent(result agent.ToolResult) []ToolCallContent { func appendToolResultDiffs(content []ToolCallContent, diffs []tools.FileDiff) []ToolCallContent { for _, diff := range diffs { - if strings.TrimSpace(diff.Path) == "" || diff.OldText == diff.NewText { + if !filepath.IsAbs(diff.Path) || (!diff.OldExists && !diff.NewExists) { continue } - content = append(content, ToolCallContent{Type: "diff", Path: diff.Path, OldText: diff.OldText, NewText: diff.NewText}) + 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 } diff --git a/internal/acp/translate_test.go b/internal/acp/translate_test.go index 50433f008..d63d51b75 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,13 +76,14 @@ 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: "a.go", OldText: "before\n", NewText: "after\n"}}, + 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) @@ -88,7 +91,7 @@ func TestToolCallResult(t *testing.T) { if len(ok.Content) != 2 || ok.Content[0].Type != "content" || ok.Content[0].Content.Text != "applied" { t.Fatalf("unexpected content: %+v", ok.Content) } - if diff := ok.Content[1]; diff.Type != "diff" || diff.Path != "a.go" || diff.OldText != "before\n" || diff.NewText != "after\n" { + 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) != 1 || ok.Locations[0].Path != "a.go" { @@ -101,6 +104,45 @@ func TestToolCallResult(t *testing.T) { } } +func TestToolCallDiffJSONPreservesRequiredEmptyNewText(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: false, OldText: ""}, + }) + 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"]) + } + } +} + +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 a8e447c5b..0d06d82f4 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -1834,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) 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/tools/diff_preview.go b/internal/tools/diff_preview.go index d3367dd27..d635702f7 100644 --- a/internal/tools/diff_preview.go +++ b/internal/tools/diff_preview.go @@ -1,6 +1,7 @@ package tools import ( + "path/filepath" "unicode/utf8" udiff "github.com/aymanbagabas/go-udiff" @@ -14,19 +15,43 @@ const maxToolPreviewBytes = 48 * 1024 // 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 string - OldText string - NewText string + // 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 or unchanged content. -func boundedFileDiff(path, oldText, newText string) (FileDiff, bool) { - if path == "" || oldText == newText || !utf8.ValidString(oldText) || !utf8.ValidString(newText) || len(oldText)+len(newText) > maxToolPreviewBytes { +// fallback for large, unsafe, or unchanged content. Newlines, carriage returns, +// and tabs are normal text; the remaining C0/C1 controls are rejected rather +// than normalized, so they cannot split a secret before transcript redaction. +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)+len(newText) > maxToolPreviewBytes { return FileDiff{}, false } - return FileDiff{Path: path, OldText: oldText, NewText: newText}, true + return FileDiff{Path: path, OldExists: oldExists, NewExists: newExists, OldText: oldText, NewText: newText}, true +} + +func unsafeDiffText(text string) bool { + for _, r := range text { + if r == '\n' || r == '\r' || r == '\t' { + continue + } + if r < 0x20 || (r >= 0x7f && r <= 0x9f) { + return true + } + } + return false } // boundedUnifiedDiff returns a unified diff of oldContent -> newContent labelled diff --git a/internal/tools/diff_preview_test.go b/internal/tools/diff_preview_test.go index 967afff6a..e162631e6 100644 --- a/internal/tools/diff_preview_test.go +++ b/internal/tools/diff_preview_test.go @@ -1,12 +1,14 @@ package tools import ( + "path/filepath" "strings" "testing" ) func TestBoundedFileDiffRefusesPartialOrBinaryContent(t *testing.T) { - if diff, ok := boundedFileDiff("a.txt", "old", "new"); !ok || diff.Path != "a.txt" || diff.OldText != "old" || diff.NewText != "new" { + 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 { @@ -17,12 +19,64 @@ func TestBoundedFileDiffRefusesPartialOrBinaryContent(t *testing.T) { {"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"}, {"too large", strings.Repeat("a", maxToolPreviewBytes), "b"}, } { t.Run(tc.name, func(t *testing.T) { - if diff, ok := boundedFileDiff("a.txt", tc.old, tc.new); ok || diff != (FileDiff{}) { + 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 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", 20*1024) + budgeted := fileDiffsFromStructuredPatch(".", []structuredPatchChange{ + {kind: structuredPatchAdd, to: structuredPatchTarget{absolute: filepath.Join(root, "one")}, after: large}, + {kind: structuredPatchAdd, to: structuredPatchTarget{absolute: filepath.Join(root, "two")}, after: large}, + {kind: structuredPatchAdd, to: structuredPatchTarget{absolute: filepath.Join(root, "three")}, after: large}, + }) + if len(budgeted) != 2 { + t.Fatalf("aggregate file-diff budget = %d diffs, want 2", len(budgeted)) + } +} diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index 0e9b9fe33..7661269e7 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -196,7 +196,7 @@ 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 diff, ok := boundedFileDiff(relativePath, content, updated); ok { + if diff, ok := boundedFileDiff(absolutePath, content, updated, true, true); ok { result.FileDiffs = []FileDiff{diff} } // Card-only preview (Display.Preview): the model's Output stays the one-line diff --git a/internal/tools/registry.go b/internal/tools/registry.go index ea9ea756b..f2b0457e2 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -343,8 +343,15 @@ func scrubResultSecrets(res Result) Result { res.Display.Preview = scrubbed res.Redacted = true } - for index := range res.FileDiffs { - diff := &res.FileDiffs[index] + fileDiffs := res.FileDiffs[:0] + 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 @@ -353,7 +360,9 @@ func scrubResultSecrets(res Result) Result { 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 { @@ -365,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 57e68ac2b..f16de43e6 100644 --- a/internal/tools/registry_test.go +++ b/internal/tools/registry_test.go @@ -447,7 +447,7 @@ func TestScrubResultSecretsRedactsPreview(t *testing.T) { secret := "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" res := scrubResultSecrets(Result{ Display: Display{Preview: "+++ b/x\n+token := \"" + secret + "\""}, - FileDiffs: []FileDiff{{Path: "x", OldText: secret, NewText: 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) @@ -460,6 +460,20 @@ func TestScrubResultSecretsRedactsPreview(t *testing.T) { } } +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 TestRunWithOptionsScrubsSecretsOnDenialPaths(t *testing.T) { secret := "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" reg := NewRegistry() diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index e8fcaea4b..fe70f965b 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -188,32 +188,56 @@ func applyPatchOperations(applyRoot, relativeRoot string, operations []structure return result } -func fileDiffsFromStructuredPatch(relativeRoot string, changes []structuredPatchChange) []FileDiff { +func fileDiffsFromStructuredPatch(_ string, changes []structuredPatchChange) []FileDiff { + const maxToolResultFileDiffs = 64 diffs := make([]FileDiff, 0, len(changes)*2) - appendDiff := func(path, before, after string) { - if relativeRoot != "" && relativeRoot != "." { - path = filepath.ToSlash(filepath.Join(relativeRoot, path)) + usedBytes := 0 + appendGroup := func(group ...FileDiff) { + if len(group) == 0 || len(diffs)+len(group) > maxToolResultFileDiffs { + return } - if diff, ok := boundedFileDiff(path, before, after); ok { - diffs = append(diffs, diff) + groupBytes := 0 + for _, diff := range group { + groupBytes += len(diff.Path) + len(diff.OldText) + len(diff.NewText) } + if usedBytes+groupBytes > maxToolPreviewBytes { + return + } + diffs = append(diffs, group...) + usedBytes += groupBytes + } + 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: - appendDiff(change.from.relative, change.before, "") + if diff, ok := makeDiff(change.from.absolute, change.before, "", true, false); ok { + group = append(group, diff) + } case change.kind == structuredPatchAdd: - appendDiff(change.to.relative, "", change.after) + 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. - appendDiff(change.to.relative, "", change.after) + 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. - appendDiff(change.from.relative, change.before, "") - appendDiff(change.to.relative, "", change.after) + 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: - appendDiff(change.to.relative, change.before, change.after) + if diff, ok := makeDiff(change.to.absolute, change.before, change.after, true, true); ok { + group = append(group, diff) + } } + appendGroup(group...) } return diffs } diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 7ed05f0b7..3ff2e3016 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, } } @@ -82,7 +84,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 +97,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 } } @@ -140,8 +144,12 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) result := okResult(summary) result.ChangedFiles = []string{relativePath} - if diff, ok := boundedFileDiff(relativePath, priorContent, content); ok { - result.FileDiffs = []FileDiff{diff} + // 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 { + if diff, ok := boundedFileDiff(absolutePath, priorContent, content, existed, true); ok { + result.FileDiffs = []FileDiff{diff} + } } // 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 diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 3d99e4cf7..18c35ff32 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -324,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", @@ -368,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", }) @@ -388,7 +397,7 @@ 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 != "code.go" || got[0].OldText != "const a = 1\nconst b = 2\n" || got[0].NewText != "const a = 42\nconst b = 2\n" { + 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) } } @@ -412,7 +421,11 @@ 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) } - if got := res.FileDiffs; len(got) != 1 || got[0].Path != "new.txt" || got[0].OldText != "" || got[0].NewText != "line one\nline two\n" { + 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) } } @@ -436,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") @@ -591,10 +626,14 @@ 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: "nested/new.txt", OldText: "", NewText: "created\n"}, - {Path: "old.txt", OldText: "old\n", NewText: ""}, - {Path: "moved.txt", OldText: "", NewText: "moved\n"}, + {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) } From 9e40439831b9572f7e8adebb18b961036c216266 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Tue, 1 Sep 2026 12:11:40 +0530 Subject: [PATCH 04/10] fix(tools): reject unsafe rich diff previews --- internal/tools/diff_preview.go | 8 +++++++- internal/tools/diff_preview_test.go | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/internal/tools/diff_preview.go b/internal/tools/diff_preview.go index d635702f7..66f41a825 100644 --- a/internal/tools/diff_preview.go +++ b/internal/tools/diff_preview.go @@ -57,8 +57,14 @@ func unsafeDiffText(text string) bool { // 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, either side is unsafe text, or the diff +// exceeds maxToolPreviewBytes. This must use the same unsafe-text gate as +// FileDiff: Display.Preview is another durable human-facing rich-diff surface. func boundedUnifiedDiff(path, oldContent, newContent string) string { + if !utf8.ValidString(oldContent) || !utf8.ValidString(newContent) || + unsafeDiffText(oldContent) || unsafeDiffText(newContent) { + return "" + } diff := udiff.Unified(path, path, oldContent, newContent) if diff == "" || len(diff) > maxToolPreviewBytes { return "" diff --git a/internal/tools/diff_preview_test.go b/internal/tools/diff_preview_test.go index e162631e6..0fe2f6fa3 100644 --- a/internal/tools/diff_preview_test.go +++ b/internal/tools/diff_preview_test.go @@ -50,6 +50,21 @@ func TestBoundedFileDiffPreservesEmptyFileOperations(t *testing.T) { } } +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", + string([]byte{0xff}), + } { + if got := boundedUnifiedDiff("secret.txt", content, "safe\n"); got != "" { + t.Fatalf("unsafe rich diff = %q", got) + } + } +} + func TestStructuredPatchFileDiffsPreserveEmptyOperationsAndResultBudget(t *testing.T) { root, err := filepath.EvalSymlinks(t.TempDir()) if err != nil { From 9b6696f5900a92fe4491451f09f864ec48aeda76 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Tue, 1 Sep 2026 13:27:23 +0530 Subject: [PATCH 05/10] fix(acp): preserve safe rich diff semantics --- internal/acp/translate.go | 6 +++++- internal/acp/translate_test.go | 5 +++-- internal/tools/diff_preview.go | 27 +++++++++++++++------------ internal/tools/diff_preview_test.go | 12 ++++++++++++ 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/internal/acp/translate.go b/internal/acp/translate.go index 06980b62e..6d4fa3538 100644 --- a/internal/acp/translate.go +++ b/internal/acp/translate.go @@ -135,7 +135,11 @@ func toolResultContent(result agent.ToolResult) []ToolCallContent { func appendToolResultDiffs(content []ToolCallContent, diffs []tools.FileDiff) []ToolCallContent { for _, diff := range diffs { - if !filepath.IsAbs(diff.Path) || (!diff.OldExists && !diff.NewExists) { + // 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 diff --git a/internal/acp/translate_test.go b/internal/acp/translate_test.go index d63d51b75..4fba8a2c2 100644 --- a/internal/acp/translate_test.go +++ b/internal/acp/translate_test.go @@ -104,11 +104,12 @@ func TestToolCallResult(t *testing.T) { } } -func TestToolCallDiffJSONPreservesRequiredEmptyNewText(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: false, OldText: ""}, + {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) diff --git a/internal/tools/diff_preview.go b/internal/tools/diff_preview.go index 66f41a825..3c29c9e76 100644 --- a/internal/tools/diff_preview.go +++ b/internal/tools/diff_preview.go @@ -2,6 +2,7 @@ package tools import ( "path/filepath" + "unicode" "unicode/utf8" udiff "github.com/aymanbagabas/go-udiff" @@ -29,8 +30,9 @@ type FileDiff struct { // 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. Newlines, carriage returns, -// and tabs are normal text; the remaining C0/C1 controls are rejected rather -// than normalized, so they cannot split a secret before transcript redaction. +// tabs, and ASCII spaces are normal text. Other controls, Unicode format +// characters, and non-ASCII whitespace are rejected rather than normalized, so +// invisible separators cannot split a secret before transcript redaction. func boundedFileDiff(path, oldText, newText string, oldExists, newExists bool) (FileDiff, bool) { if !filepath.IsAbs(path) || (!oldExists && !newExists) || (oldExists == newExists && oldText == newText) || @@ -43,11 +45,15 @@ func boundedFileDiff(path, oldText, newText string, oldExists, newExists bool) ( } func unsafeDiffText(text string) bool { + if !utf8.ValidString(text) { + return true + } for _, r := range text { - if r == '\n' || r == '\r' || r == '\t' { + switch r { + case '\n', '\r', '\t', ' ': continue } - if r < 0x20 || (r >= 0x7f && r <= 0x9f) { + if unicode.IsControl(r) || unicode.Is(unicode.Cf, r) || unicode.IsSpace(r) { return true } } @@ -57,16 +63,13 @@ func unsafeDiffText(text string) bool { // 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, either side is unsafe text, or the diff -// exceeds maxToolPreviewBytes. This must use the same unsafe-text gate as -// FileDiff: Display.Preview is another durable human-facing rich-diff surface. +// 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 { - if !utf8.ValidString(oldContent) || !utf8.ValidString(newContent) || - unsafeDiffText(oldContent) || unsafeDiffText(newContent) { - return "" - } 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 index 0fe2f6fa3..33d829112 100644 --- a/internal/tools/diff_preview_test.go +++ b/internal/tools/diff_preview_test.go @@ -22,6 +22,11 @@ func TestBoundedFileDiffRefusesPartialOrBinaryContent(t *testing.T) { {"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), "b"}, } { t.Run(tc.name, func(t *testing.T) { @@ -57,12 +62,19 @@ func TestBoundedUnifiedDiffRejectsUnsafeRichText(t *testing.T) { 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) { From 4615ba2f12e3db248f309eda907834f2cf9aca76 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:35:41 +0530 Subject: [PATCH 06/10] fix(tools): bind structured diffs to committed changes --- internal/acp/translate.go | 32 ++++- internal/acp/translate_test.go | 21 +++- internal/tools/apply_patch_tolerance_test.go | 12 +- internal/tools/diff_preview.go | 47 +++++-- internal/tools/diff_preview_test.go | 61 ++++++++- internal/tools/edit_file.go | 20 ++- internal/tools/file_commit.go | 103 +++++++++++++++ internal/tools/file_commit_test.go | 81 ++++++++++++ internal/tools/format_on_write.go | 39 +++--- internal/tools/format_on_write_test.go | 126 ++++++++++++++++++- internal/tools/registry.go | 2 +- internal/tools/registry_test.go | 15 +++ internal/tools/structured_patch.go | 104 ++++++++++----- internal/tools/write_file.go | 24 +++- internal/tools/write_tools_test.go | 8 +- 15 files changed, 611 insertions(+), 84 deletions(-) create mode 100644 internal/tools/file_commit.go create mode 100644 internal/tools/file_commit_test.go diff --git a/internal/acp/translate.go b/internal/acp/translate.go index 6d4fa3538..75da41d15 100644 --- a/internal/acp/translate.go +++ b/internal/acp/translate.go @@ -154,16 +154,44 @@ func appendToolResultDiffs(content []ToolCallContent, diffs []tools.FileDiff) [] } 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 := strings.TrimSpace(diff.Path) + if path == "" || seen[path] { + continue + } + seen[path] = true + locs = append(locs, ToolCallLocation{Path: path}) + } for _, f := range result.ChangedFiles { - if strings.TrimSpace(f) == "" { + f = strings.TrimSpace(f) + if f == "" || locationCoveredByFileDiff(f, result.FileDiffs) || seen[f] { continue } + seen[f] = true locs = append(locs, ToolCallLocation{Path: f}) } return locs } +func locationCoveredByFileDiff(changed string, diffs []tools.FileDiff) bool { + changed = filepath.Clean(changed) + for _, diff := range diffs { + diffPath := filepath.Clean(diff.Path) + if filepath.IsAbs(changed) { + if diffPath == changed { + return true + } + continue + } + if diffPath == changed || strings.HasSuffix(diffPath, string(filepath.Separator)+changed) { + return true + } + } + return false +} + // planUpdate maps ZERO's plan items to an ACP "plan" update. func planUpdate(items []tools.PlanItem) PlanUpdate { entries := make([]PlanEntry, 0, len(items)) diff --git a/internal/acp/translate_test.go b/internal/acp/translate_test.go index 4fba8a2c2..82a6e4127 100644 --- a/internal/acp/translate_test.go +++ b/internal/acp/translate_test.go @@ -94,8 +94,8 @@ func TestToolCallResult(t *testing.T) { 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) != 1 || ok.Locations[0].Path != "a.go" { - t.Fatalf("blank changed files should be dropped, got %+v", ok.Locations) + if len(ok.Locations) != 1 || ok.Locations[0].Path != path { + t.Fatalf("rich diff location should use the same absolute path, got %+v", ok.Locations) } failed := toolCallResult(agent.ToolResult{ToolCallID: "tc2", Status: tools.StatusError, Output: "boom"}) @@ -129,6 +129,23 @@ func TestToolCallDiffJSONPreservesEmptyFilesWithoutClaimingDeletion(t *testing.T 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 TestToolResultLocationsCorrelateRichDiffsAndKeepFallbacks(t *testing.T) { + root := t.TempDir() + richPath := filepath.Join(root, "rich.go") + locations := toolResultLocations(agent.ToolResult{ + ChangedFiles: []string{"rich.go", "fallback.go"}, + FileDiffs: []tools.FileDiff{{ + Path: richPath, OldExists: true, NewExists: true, OldText: "before", NewText: "after", + }}, + }) + if len(locations) != 2 || locations[0].Path != richPath || locations[1].Path != "fallback.go" { + t.Fatalf("locations = %#v", locations) } } 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 3c29c9e76..7d7df606e 100644 --- a/internal/tools/diff_preview.go +++ b/internal/tools/diff_preview.go @@ -2,9 +2,11 @@ package tools import ( "path/filepath" + "strings" "unicode" "unicode/utf8" + "github.com/Gitlawb/zero/internal/redaction" udiff "github.com/aymanbagabas/go-udiff" ) @@ -13,6 +15,12 @@ import ( // 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 { @@ -29,16 +37,17 @@ type FileDiff struct { // 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. Newlines, carriage returns, -// tabs, and ASCII spaces are normal text. Other controls, Unicode format -// characters, and non-ASCII whitespace are rejected rather than normalized, so -// invisible separators cannot split a secret before transcript redaction. +// 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)+len(newText) > maxToolPreviewBytes { + len(oldText) > maxToolPreviewBytes || len(newText) > maxToolPreviewBytes { return FileDiff{}, false } return FileDiff{Path: path, OldExists: oldExists, NewExists: newExists, OldText: oldText, NewText: newText}, true @@ -48,16 +57,40 @@ 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) || unicode.Is(unicode.Cf, r) || unicode.IsSpace(r) { + if unicode.IsControl(r) { return true } + if unicode.Is(unicode.Cf, r) || unicode.IsSpace(r) { + hasCanonicalizableSeparator = true + } + } + if !hasCanonicalizableSeparator { + return false } - 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 diff --git a/internal/tools/diff_preview_test.go b/internal/tools/diff_preview_test.go index 33d829112..f433c9114 100644 --- a/internal/tools/diff_preview_test.go +++ b/internal/tools/diff_preview_test.go @@ -27,7 +27,7 @@ func TestBoundedFileDiffRefusesPartialOrBinaryContent(t *testing.T) { {"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), "b"}, + {"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{}) { @@ -97,13 +97,64 @@ func TestStructuredPatchFileDiffsPreserveEmptyOperationsAndResultBudget(t *testi } } - large := strings.Repeat("x", 20*1024) + large := strings.Repeat("x", 40*1024) budgeted := fileDiffsFromStructuredPatch(".", []structuredPatchChange{ {kind: structuredPatchAdd, to: structuredPatchTarget{absolute: filepath.Join(root, "one")}, after: large}, - {kind: structuredPatchAdd, to: structuredPatchTarget{absolute: filepath.Join(root, "two")}, 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) != 2 { - t.Fatalf("aggregate file-diff budget = %d diffs, want 2", len(budgeted)) + 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 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 7661269e7..c4931d50f 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,8 +202,12 @@ 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 diff, ok := boundedFileDiff(absolutePath, content, updated, true, true); ok { - result.FileDiffs = []FileDiff{diff} + 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. 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..92ee5e22f 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,126 @@ 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) + } +} diff --git a/internal/tools/registry.go b/internal/tools/registry.go index f2b0457e2..074092ebc 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -343,7 +343,7 @@ func scrubResultSecrets(res Result) Result { res.Display.Preview = scrubbed res.Redacted = true } - fileDiffs := res.FileDiffs[:0] + 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 diff --git a/internal/tools/registry_test.go b/internal/tools/registry_test.go index f16de43e6..2519fb3d8 100644 --- a/internal/tools/registry_test.go +++ b/internal/tools/registry_test.go @@ -474,6 +474,21 @@ func TestScrubResultSecretsDropsControlSplitFileDiff(t *testing.T) { } } +func TestScrubResultSecretsDoesNotMutateCallerFileDiffSlice(t *testing.T) { + path := filepath.Join(t.TempDir(), "x") + original := []FileDiff{ + {Path: path, OldExists: true, NewExists: true, OldText: "token=sk-proj-abc\x00def", NewText: "unsafe"}, + {Path: path, OldExists: true, NewExists: true, OldText: "before", NewText: "after"}, + } + result := scrubResultSecrets(Result{FileDiffs: original}) + if len(result.FileDiffs) != 1 || result.FileDiffs[0].OldText != "before" { + t.Fatalf("filtered FileDiffs = %#v", result.FileDiffs) + } + if original[0].NewText != "unsafe" || original[1].OldText != "before" { + t.Fatalf("caller slice was mutated: %#v", original) + } +} + func TestRunWithOptionsScrubsSecretsOnDenialPaths(t *testing.T) { secret := "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" reg := NewRegistry() diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index fe70f965b..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 { @@ -184,27 +191,38 @@ 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) { + appendGroup := func(group ...FileDiff) bool { if len(group) == 0 || len(diffs)+len(group) > maxToolResultFileDiffs { - return + return false } groupBytes := 0 for _, diff := range group { - groupBytes += len(diff.Path) + len(diff.OldText) + len(diff.NewText) + groupBytes += len(diff.OldText) + len(diff.NewText) } - if usedBytes+groupBytes > maxToolPreviewBytes { - return + 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) @@ -237,7 +255,9 @@ func fileDiffsFromStructuredPatch(_ string, changes []structuredPatchChange) []F group = append(group, diff) } } - appendGroup(group...) + if len(group) > 0 && !appendGroup(group...) { + break + } } return diffs } @@ -727,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) { @@ -969,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/write_file.go b/internal/tools/write_file.go index 3ff2e3016..ff297cbae 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -63,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.") } @@ -111,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 { @@ -146,9 +156,11 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an 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 { + 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 diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 18c35ff32..22a134a41 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -884,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) } @@ -910,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") } @@ -947,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") } @@ -1070,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) } From 2cec7ff32d48a8991f908925ecb3c064c03016aa Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Thu, 3 Sep 2026 18:59:35 +0530 Subject: [PATCH 07/10] fix(acp): preserve structured diff identity --- internal/acp/translate.go | 27 +++------ internal/acp/translate_test.go | 80 +++++++++++++++++++++++--- internal/tools/diff_preview_test.go | 24 +++++++- internal/tools/edit_file.go | 6 +- internal/tools/format_on_write_test.go | 31 ++++++++++ internal/tools/registry_test.go | 9 ++- internal/tools/write_file.go | 6 +- 7 files changed, 149 insertions(+), 34 deletions(-) diff --git a/internal/acp/translate.go b/internal/acp/translate.go index 75da41d15..84664e58d 100644 --- a/internal/acp/translate.go +++ b/internal/acp/translate.go @@ -157,16 +157,20 @@ func toolResultLocations(result agent.ToolResult) []ToolCallLocation { 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 := strings.TrimSpace(diff.Path) + 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 { - f = strings.TrimSpace(f) - if f == "" || locationCoveredByFileDiff(f, result.FileDiffs) || seen[f] { + if f == "" || seen[f] { continue } seen[f] = true @@ -175,23 +179,6 @@ func toolResultLocations(result agent.ToolResult) []ToolCallLocation { return locs } -func locationCoveredByFileDiff(changed string, diffs []tools.FileDiff) bool { - changed = filepath.Clean(changed) - for _, diff := range diffs { - diffPath := filepath.Clean(diff.Path) - if filepath.IsAbs(changed) { - if diffPath == changed { - return true - } - continue - } - if diffPath == changed || strings.HasSuffix(diffPath, string(filepath.Separator)+changed) { - return true - } - } - return false -} - // planUpdate maps ZERO's plan items to an ACP "plan" update. func planUpdate(items []tools.PlanItem) PlanUpdate { entries := make([]PlanEntry, 0, len(items)) diff --git a/internal/acp/translate_test.go b/internal/acp/translate_test.go index 82a6e4127..d54745cd1 100644 --- a/internal/acp/translate_test.go +++ b/internal/acp/translate_test.go @@ -94,8 +94,8 @@ func TestToolCallResult(t *testing.T) { 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) != 1 || ok.Locations[0].Path != path { - t.Fatalf("rich diff location should use the same absolute path, got %+v", ok.Locations) + 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"}) @@ -135,17 +135,81 @@ func TestToolCallDiffJSONPreservesEmptyFilesWithoutClaimingDeletion(t *testing.T } } -func TestToolResultLocationsCorrelateRichDiffsAndKeepFallbacks(t *testing.T) { +func TestToolResultLocationsPreserveDistinctPathIdentities(t *testing.T) { root := t.TempDir() - richPath := filepath.Join(root, "rich.go") + 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{"rich.go", "fallback.go"}, + 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: richPath, OldExists: true, NewExists: true, OldText: "before", NewText: "after", + Path: absolutePath, OldExists: true, NewExists: false, OldText: "before", }}, }) - if len(locations) != 2 || locations[0].Path != richPath || locations[1].Path != "fallback.go" { - t.Fatalf("locations = %#v", locations) + 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) } } diff --git a/internal/tools/diff_preview_test.go b/internal/tools/diff_preview_test.go index f433c9114..bda35b1d0 100644 --- a/internal/tools/diff_preview_test.go +++ b/internal/tools/diff_preview_test.go @@ -97,7 +97,7 @@ func TestStructuredPatchFileDiffsPreserveEmptyOperationsAndResultBudget(t *testi } } - large := strings.Repeat("x", 40*1024) + 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"}, @@ -109,6 +109,28 @@ func TestStructuredPatchFileDiffsPreserveEmptyOperationsAndResultBudget(t *testi } } +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{ diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index c4931d50f..35e0252f2 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -211,7 +211,11 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any } // 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/format_on_write_test.go b/internal/tools/format_on_write_test.go index 92ee5e22f..92dfa84d5 100644 --- a/internal/tools/format_on_write_test.go +++ b/internal/tools/format_on_write_test.go @@ -233,4 +233,35 @@ func TestWriteFileOmitsRichDiffWhenFormatterFinalReadFails(t *testing.T) { 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_test.go b/internal/tools/registry_test.go index 2519fb3d8..145f15388 100644 --- a/internal/tools/registry_test.go +++ b/internal/tools/registry_test.go @@ -476,15 +476,18 @@ func TestScrubResultSecretsDropsControlSplitFileDiff(t *testing.T) { 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: "before", NewText: "after"}, + {Path: path, OldExists: true, NewExists: true, OldText: retainedOld, NewText: retainedNew}, } result := scrubResultSecrets(Result{FileDiffs: original}) - if len(result.FileDiffs) != 1 || result.FileDiffs[0].OldText != "before" { + 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 != "before" { + if original[0].NewText != "unsafe" || original[1].OldText != retainedOld || original[1].NewText != retainedNew { t.Fatalf("caller slice was mutated: %#v", original) } } diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index ff297cbae..de6e60d29 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -166,7 +166,11 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an // 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 } From e7fca6bdf2fd7d5ccfba2cd8560af76ab7b28d6a Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sat, 5 Sep 2026 13:42:14 +0530 Subject: [PATCH 08/10] fix(tools): close rich diff race gaps --- internal/acp/translate_test.go | 31 ++++ internal/tools/diff_preview.go | 18 ++- internal/tools/diff_preview_test.go | 22 +-- internal/tools/edit_file.go | 11 +- internal/tools/file_commit.go | 36 +++++ internal/tools/format_on_write.go | 65 +++++++-- internal/tools/format_on_write_test.go | 191 ++++++++++++++++++++++++- internal/tools/registry_test.go | 27 ++++ internal/tools/structured_patch.go | 70 ++++++--- internal/tools/write_file.go | 11 +- internal/tools/write_tools_test.go | 53 +++++++ 11 files changed, 481 insertions(+), 54 deletions(-) diff --git a/internal/acp/translate_test.go b/internal/acp/translate_test.go index d54745cd1..a00e06f07 100644 --- a/internal/acp/translate_test.go +++ b/internal/acp/translate_test.go @@ -225,6 +225,37 @@ func TestToolCallResultEmitsOnlyRedactedFileDiffs(t *testing.T) { } } +func TestToolCallResultOmitsDefaultIgnorableSplitSecretsOnEitherSide(t *testing.T) { + secret := "sk-ant-api03-AAAABBBBCCCCDDDDEEEEFFFFGGGG" + for name, separator := range map[string]string{ + "combining grapheme joiner": "\u034f", + "variation selector": "\ufe0f", + } { + for _, side := range []string{"old", "new"} { + t.Run(name+" "+side, func(t *testing.T) { + obfuscated := secret[:20] + separator + secret[20:] + diff := tools.FileDiff{ + Path: filepath.Join(t.TempDir(), "secret.txt"), OldExists: true, NewExists: true, + OldText: "safe old", NewText: "safe new", + } + if side == "old" { + diff.OldText = obfuscated + } else { + diff.NewText = obfuscated + } + scrubbed := tools.ScrubResultSecrets(tools.Result{FileDiffs: []tools.FileDiff{diff}}) + if !scrubbed.Redacted || len(scrubbed.FileDiffs) != 0 { + t.Fatalf("registry boundary retained an obfuscated secret: %#v", scrubbed) + } + update := toolCallResult(agent.ToolResult{ToolCallID: "call", Status: tools.StatusOK, FileDiffs: scrubbed.FileDiffs}) + if len(update.Content) != 0 { + t.Fatalf("ACP content retained an obfuscated secret: %#v", update.Content) + } + }) + } + } +} + func TestPlanUpdateAndStatus(t *testing.T) { upd := planUpdate([]tools.PlanItem{ {Content: "step a", Status: "completed"}, diff --git a/internal/tools/diff_preview.go b/internal/tools/diff_preview.go index 7d7df606e..31081828c 100644 --- a/internal/tools/diff_preview.go +++ b/internal/tools/diff_preview.go @@ -66,7 +66,7 @@ func unsafeDiffText(text string) bool { if unicode.IsControl(r) { return true } - if unicode.Is(unicode.Cf, r) || unicode.IsSpace(r) { + if isDiffCredentialIgnorable(r) { hasCanonicalizableSeparator = true } } @@ -85,7 +85,7 @@ func diffTextRevealsObfuscatedSecret(text string) bool { case '\n', '\r', '\t', ' ': return r } - if unicode.Is(unicode.Cf, r) || unicode.IsSpace(r) { + if isDiffCredentialIgnorable(r) { return -1 } return r @@ -93,6 +93,20 @@ func diffTextRevealsObfuscatedSecret(text string) bool { return canonical != text && redaction.RedactString(canonical, redaction.Options{}) != canonical } +// isDiffCredentialIgnorable mirrors Unicode's Default_Ignorable_Code_Point +// derived property using the tables exposed by the Go standard library. Cf +// catches format controls, while the other two tables cover non-Cf default +// ignorables such as U+034F COMBINING GRAPHEME JOINER and variation selectors. +// Unicode whitespace is also removable because it can split a credential that +// the boundary redactor would otherwise recognize. The caller preserves normal +// ASCII layout whitespace before consulting this helper. +func isDiffCredentialIgnorable(r rune) bool { + return unicode.Is(unicode.Cf, r) || + unicode.Is(unicode.Other_Default_Ignorable_Code_Point, r) || + unicode.Is(unicode.Variation_Selector, r) || + unicode.IsSpace(r) +} + // 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. diff --git a/internal/tools/diff_preview_test.go b/internal/tools/diff_preview_test.go index bda35b1d0..757f32f0b 100644 --- a/internal/tools/diff_preview_test.go +++ b/internal/tools/diff_preview_test.go @@ -134,10 +134,12 @@ func TestStructuredPatchFileDiffsKeepEligibleSameBasenameSibling(t *testing.T) { 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", + "family emoji": "family: ๐Ÿ‘จโ€๐Ÿ‘ฉโ€๐Ÿ‘งโ€๐Ÿ‘ฆ\n", + "emoji variation selector": "heart: โค๏ธ\n", + "combining grapheme joiner": "ordinary\u034fprose\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 { @@ -151,11 +153,13 @@ func TestBoundedDiffPreservesOrdinaryUnicodeButRejectsObfuscatedSecrets(t *testi 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", + "zero width space": "\u200b", + "zero width joiner": "\u200d", + "byte order mark": "\ufeff", + "soft hyphen": "\u00ad", + "nonbreaking space": "\u00a0", + "combining grapheme joiner": "\u034f", + "variation selector": "\ufe0f", } { t.Run("split "+name, func(t *testing.T) { obfuscated := secret[:20] + separator + secret[20:] diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index 35e0252f2..5c50ffa99 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -164,10 +164,13 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any // 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, finalContentKnown := maybeFormatWrittenFile(ctx, absolutePath, updated) + updated, formattedInfo, finalContentKnown := maybeFormatWrittenFile(ctx, tool.workspaceRoot, tool.scope, 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) + newInfo := formattedInfo + if newInfo == nil { + newInfo, _ = os.Stat(absolutePath) + } if !finalContentKnown { options.FileTracker.Forget(absolutePath) } else if updated == modelKnownContent { @@ -199,7 +202,9 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any suffix = "s" } summary := fmt.Sprintf("Successfully edited %s (replaced %d occurrence%s).", relativePath, replacedCount, suffix) - summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) + if finalContentKnown { + summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) + } result := okResult(summary) result.ChangedFiles = []string{relativePath} if finalContentKnown { diff --git a/internal/tools/file_commit.go b/internal/tools/file_commit.go index e14c0493d..033973129 100644 --- a/internal/tools/file_commit.go +++ b/internal/tools/file_commit.go @@ -101,3 +101,39 @@ func writeAndVerifyFileIdentity(path string, file *os.File, content string, trun } return nil } + +// readRootedFile returns bytes and identity from the same opened object, and +// verifies that the rooted path still names that object after the read. Root.Open +// binds symlink containment to use rather than relying on a pathname pre-check. +func readRootedFile(root *os.Root, relativePath string) ([]byte, os.FileInfo, error) { + file, err := root.Open(relativePath) + if err != nil { + return nil, nil, err + } + closed := false + defer func() { + if !closed { + _ = file.Close() + } + }() + openedInfo, err := file.Stat() + if err != nil { + return nil, nil, err + } + if !openedInfo.Mode().IsRegular() { + return nil, nil, fmt.Errorf("%s is not a regular file", relativePath) + } + content, err := io.ReadAll(file) + if err != nil { + return nil, nil, err + } + pathInfo, err := root.Stat(relativePath) + if err != nil || !os.SameFile(openedInfo, pathInfo) { + return nil, nil, fmt.Errorf("%w: path identity changed", errFileChangedDuringWrite) + } + if err := file.Close(); err != nil { + return nil, nil, err + } + closed = true + return content, openedInfo, nil +} diff --git a/internal/tools/format_on_write.go b/internal/tools/format_on_write.go index 2e4267f4d..40c08ee17 100644 --- a/internal/tools/format_on_write.go +++ b/internal/tools/format_on_write.go @@ -2,11 +2,14 @@ package tools import ( "context" + "fmt" "os" "os/exec" "path/filepath" "strings" "time" + + "github.com/Gitlawb/zero/internal/sandbox" ) // Format-on-write for the mutating file tools. When enabled, a successful @@ -72,7 +75,7 @@ var runFormatOnWriteCommand = func(ctx context.Context, binaryPath string, argum return formatter.Run() } -var readFormattedFile = os.ReadFile +var readFormattedFile = readRootedFile // maybeFormatWrittenFile runs the configured formatter for absolutePath (when // enabled and on PATH) and returns the verified file content afterwards. A @@ -80,25 +83,71 @@ var readFormattedFile = os.ReadFile // 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) { +func maybeFormatWrittenFile(ctx context.Context, workspaceRoot string, scope PathScope, absolutePath string, writtenContent string) (string, os.FileInfo, bool) { if !formatOnWriteEnabled() { - return writtenContent, true + return writtenContent, nil, true } command, ok := formatterCommands[strings.ToLower(filepath.Ext(absolutePath))] if !ok { - return writtenContent, true + return writtenContent, nil, true } binaryPath, err := exec.LookPath(command[0]) if err != nil { - return writtenContent, true + return writtenContent, nil, true + } + root, relativePath, err := openFormattedFileRoot(workspaceRoot, scope, absolutePath) + if err != nil { + return writtenContent, nil, false } + defer root.Close() formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout) defer cancel() arguments := append(append([]string(nil), command[1:]...), absolutePath) _ = runFormatOnWriteCommand(formatCtx, binaryPath, arguments, filepath.Dir(absolutePath)) - formatted, err := readFormattedFile(absolutePath) + formatted, info, err := readFormattedFile(root, relativePath) if err != nil { - return writtenContent, false + return writtenContent, nil, false + } + return string(formatted), info, true +} + +// openFormattedFileRoot opens the write root before the formatter runs and +// computes the target relative to that descriptor-bound root. Atomic in-root +// replacement remains valid; a formatter that swaps the target to an escaping +// symlink is rejected when readFormattedFile opens it through the root. +func openFormattedFileRoot(workspaceRoot string, scope PathScope, absolutePath string) (*os.Root, string, error) { + roots, err := scopedRoots(workspaceRoot, scope) + if err != nil { + return nil, "", err + } + var firstErr error + for _, configuredRoot := range roots { + resolvedRoot, err := filepath.Abs(configuredRoot) + if err == nil { + resolvedRoot, err = filepath.EvalSymlinks(resolvedRoot) + } + if err != nil { + if firstErr == nil { + firstErr = err + } + continue + } + candidate := sandbox.NormalizePrefixForRoot(absolutePath, resolvedRoot) + relativePath, err := filepath.Rel(resolvedRoot, candidate) + if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { + continue + } + root, err := os.OpenRoot(resolvedRoot) + if err != nil { + if firstErr == nil { + firstErr = err + } + continue + } + return root, relativePath, nil + } + if firstErr != nil { + return nil, "", firstErr } - return string(formatted), true + return nil, "", fmt.Errorf("%s must stay inside the configured write roots", absolutePath) } diff --git a/internal/tools/format_on_write_test.go b/internal/tools/format_on_write_test.go index 92dfa84d5..30ce0bfeb 100644 --- a/internal/tools/format_on_write_test.go +++ b/internal/tools/format_on_write_test.go @@ -97,7 +97,8 @@ func TestFormatOnWriteFormatsAndKeepsTrackerConsistent(t *testing.T) { func TestFormatOnWriteSkipsUnknownExtensions(t *testing.T) { t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - content, known := maybeFormatWrittenFile(context.Background(), filepath.Join(t.TempDir(), "notes.xyz"), "raw text") + root := t.TempDir() + content, _, known := maybeFormatWrittenFile(context.Background(), root, nil, filepath.Join(root, "notes.xyz"), "raw text") if content != "raw text" || !known { t.Fatalf("unknown extension must pass through: %q", content) } @@ -111,7 +112,7 @@ func TestFormatOnWriteFormatterLookupFailure(t *testing.T) { if err := os.WriteFile(targetPath, []byte(uglyContent), 0o644); err != nil { t.Fatal(err) } - content, known := maybeFormatWrittenFile(context.Background(), targetPath, uglyContent) + content, _, known := maybeFormatWrittenFile(context.Background(), filepath.Dir(targetPath), nil, targetPath, uglyContent) if content != uglyContent || !known { t.Fatalf("missing formatter must return written content, got %q", content) } @@ -133,7 +134,7 @@ func TestFormatOnWriteReadsMutatedFileAfterFormatterFailure(t *testing.T) { } t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) - content, known := maybeFormatWrittenFile(context.Background(), targetPath, "requested") + content, _, known := maybeFormatWrittenFile(context.Background(), filepath.Dir(targetPath), nil, targetPath, "requested") if !known || content != "formatter-mutated" { t.Fatalf("formatter failure content = %q, known=%t", content, known) } @@ -149,13 +150,13 @@ func TestFormatOnWriteMarksUnreadableFinalStateUnknown(t *testing.T) { priorRunner := runFormatOnWriteCommand priorReader := readFormattedFile runFormatOnWriteCommand = func(context.Context, string, []string, string) error { return nil } - readFormattedFile = func(string) ([]byte, error) { return nil, os.ErrPermission } + readFormattedFile = func(*os.Root, string) ([]byte, os.FileInfo, error) { return nil, nil, os.ErrPermission } t.Cleanup(func() { runFormatOnWriteCommand = priorRunner readFormattedFile = priorReader }) - content, known := maybeFormatWrittenFile(context.Background(), targetPath, "requested") + content, _, known := maybeFormatWrittenFile(context.Background(), filepath.Dir(targetPath), nil, targetPath, "requested") if known || content != "requested" { t.Fatalf("unreadable formatter result = %q, known=%t", content, known) } @@ -221,7 +222,7 @@ func TestWriteFileOmitsRichDiffWhenFormatterFinalReadFails(t *testing.T) { priorRunner := runFormatOnWriteCommand priorReader := readFormattedFile runFormatOnWriteCommand = func(context.Context, string, []string, string) error { return exec.ErrNotFound } - readFormattedFile = func(string) ([]byte, error) { return nil, os.ErrPermission } + readFormattedFile = func(*os.Root, string) ([]byte, os.FileInfo, error) { return nil, nil, os.ErrPermission } t.Cleanup(func() { runFormatOnWriteCommand = priorRunner readFormattedFile = priorReader @@ -249,7 +250,7 @@ func TestEditFileOmitsPreviewWhenFormatterFinalReadFails(t *testing.T) { priorRunner := runFormatOnWriteCommand priorReader := readFormattedFile runFormatOnWriteCommand = func(context.Context, string, []string, string) error { return nil } - readFormattedFile = func(string) ([]byte, error) { return nil, os.ErrPermission } + readFormattedFile = func(*os.Root, string) ([]byte, os.FileInfo, error) { return nil, nil, os.ErrPermission } t.Cleanup(func() { runFormatOnWriteCommand = priorRunner readFormattedFile = priorReader @@ -265,3 +266,179 @@ func TestEditFileOmitsPreviewWhenFormatterFinalReadFails(t *testing.T) { t.Fatalf("unverified formatter result exposed stale preview: %q", result.Display.Preview) } } + +func TestWriteFileOmitsRichEvidenceWhenFormatterReplacesTargetWithOutOfRootSymlink(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + root := t.TempDir() + targetPath := filepath.Join(root, "a.go") + resolvedRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatal(err) + } + trackedPath := filepath.Join(resolvedRoot, "a.go") + outsidePath := filepath.Join(t.TempDir(), "outside.go") + outsideContent := "package external\n\nconst Secret = \"outside\"\n" + if err := os.WriteFile(outsidePath, []byte(outsideContent), 0o644); err != nil { + t.Fatal(err) + } + priorRunner := runFormatOnWriteCommand + runFormatOnWriteCommand = func(context.Context, string, []string, string) error { + if err := os.Remove(targetPath); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outsidePath, targetPath); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + return nil + } + t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) + tracker := NewFileTracker() + + diagnosticsCalled := false + result := NewScopedWriteFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", "content": "package requested\n", + }, RunOptions{FileTracker: tracker, Diagnostics: func(context.Context, string) string { + diagnosticsCalled = true + return "must not run" + }}) + if result.Status != StatusOK || len(result.ChangedFiles) != 1 || len(result.FileDiffs) != 0 || result.Display.Preview != "" { + t.Fatalf("out-of-root formatter result = status=%s changed=%#v diffs=%#v preview=%q output=%q", result.Status, result.ChangedFiles, result.FileDiffs, result.Display.Preview, result.Output) + } + if diagnosticsCalled { + t.Fatal("diagnostics must not inspect an unverified formatter target") + } + if _, tracked := tracker.Version(trackedPath); tracked { + t.Fatal("out-of-root formatter target must not be recorded in the tracker") + } +} + +func TestEditFileOmitsRichEvidenceWhenFormatterReplacesTargetWithOutOfRootSymlink(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + root := t.TempDir() + targetPath := filepath.Join(root, "a.go") + resolvedRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatal(err) + } + trackedPath := filepath.Join(resolvedRoot, "a.go") + if err := os.WriteFile(targetPath, []byte("package before\n"), 0o644); err != nil { + t.Fatal(err) + } + outsidePath := filepath.Join(t.TempDir(), "outside.go") + outsideContent := "package external\n\nconst Secret = \"outside\"\n" + if err := os.WriteFile(outsidePath, []byte(outsideContent), 0o644); err != nil { + t.Fatal(err) + } + priorRunner := runFormatOnWriteCommand + runFormatOnWriteCommand = func(context.Context, string, []string, string) error { + if err := os.Remove(targetPath); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outsidePath, targetPath); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + return nil + } + t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) + tracker := NewFileTracker() + read := NewScopedReadFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + }, RunOptions{FileTracker: tracker}) + if read.Status != StatusOK { + t.Fatalf("read before edit failed: %s", read.Output) + } + + diagnosticsCalled := false + result := NewScopedEditFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", "old_string": "before", "new_string": "requested", + }, RunOptions{FileTracker: tracker, Diagnostics: func(context.Context, string) string { + diagnosticsCalled = true + return "must not run" + }}) + if result.Status != StatusOK || len(result.ChangedFiles) != 1 || len(result.FileDiffs) != 0 || result.Display.Preview != "" { + t.Fatalf("out-of-root formatter result = status=%s changed=%#v diffs=%#v preview=%q output=%q", result.Status, result.ChangedFiles, result.FileDiffs, result.Display.Preview, result.Output) + } + if diagnosticsCalled { + t.Fatal("diagnostics must not inspect an unverified formatter target") + } + if _, tracked := tracker.Version(trackedPath); tracked { + t.Fatal("out-of-root formatter target must not be recorded in the tracker") + } +} + +func TestWriteFileAcceptsInRootAtomicFormatterReplacement(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + root := t.TempDir() + targetPath := filepath.Join(root, "a.go") + formatted := "package formatted\n" + priorRunner := runFormatOnWriteCommand + runFormatOnWriteCommand = func(context.Context, string, []string, string) error { + tempPath := filepath.Join(root, "formatter.tmp") + if err := os.WriteFile(tempPath, []byte(formatted), 0o644); err != nil { + t.Fatal(err) + } + return os.Rename(tempPath, targetPath) + } + t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) + tracker := NewFileTracker() + + result := NewScopedWriteFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", "content": "package requested\n", + }, RunOptions{FileTracker: tracker}) + if result.Status != StatusOK || len(result.FileDiffs) != 1 || result.FileDiffs[0].NewText != formatted { + t.Fatalf("atomic formatter write = status=%s diffs=%#v output=%q", result.Status, result.FileDiffs, result.Output) + } + resolvedRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatal(err) + } + version, tracked := tracker.Version(filepath.Join(resolvedRoot, "a.go")) + if !tracked || version.Hash != HashContent([]byte(formatted)) { + t.Fatalf("atomic formatter tracker = %#v, tracked=%t", version, tracked) + } +} + +func TestEditFileAcceptsInRootAtomicFormatterReplacement(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("package before\n"), 0o644); err != nil { + t.Fatal(err) + } + formatted := "package formatted\n" + priorRunner := runFormatOnWriteCommand + runFormatOnWriteCommand = func(context.Context, string, []string, string) error { + tempPath := filepath.Join(root, "formatter.tmp") + if err := os.WriteFile(tempPath, []byte(formatted), 0o644); err != nil { + t.Fatal(err) + } + return os.Rename(tempPath, targetPath) + } + t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) + tracker := NewFileTracker() + read := NewScopedReadFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + }, RunOptions{FileTracker: tracker}) + if read.Status != StatusOK { + t.Fatalf("read before edit failed: %s", read.Output) + } + + result := NewScopedEditFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", "old_string": "before", "new_string": "requested", + }, RunOptions{FileTracker: tracker}) + if result.Status != StatusOK || len(result.FileDiffs) != 1 || result.FileDiffs[0].NewText != formatted { + t.Fatalf("atomic formatter edit = status=%s diffs=%#v output=%q", result.Status, result.FileDiffs, result.Output) + } + resolvedRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatal(err) + } + version, tracked := tracker.Version(filepath.Join(resolvedRoot, "a.go")) + if !tracked || version.Hash != HashContent([]byte(formatted)) { + t.Fatalf("atomic formatter tracker = %#v, tracked=%t", version, tracked) + } +} diff --git a/internal/tools/registry_test.go b/internal/tools/registry_test.go index 145f15388..b78de7903 100644 --- a/internal/tools/registry_test.go +++ b/internal/tools/registry_test.go @@ -474,6 +474,33 @@ func TestScrubResultSecretsDropsControlSplitFileDiff(t *testing.T) { } } +func TestScrubResultSecretsDropsDefaultIgnorableSplitFileDiffOnEitherSide(t *testing.T) { + secret := "sk-ant-api03-AAAABBBBCCCCDDDDEEEEFFFFGGGG" + for name, separator := range map[string]string{ + "combining grapheme joiner": "\u034f", + "variation selector": "\ufe0f", + } { + for _, side := range []string{"old", "new"} { + t.Run(name+" "+side, func(t *testing.T) { + obfuscated := secret[:20] + separator + secret[20:] + diff := FileDiff{ + Path: filepath.Join(t.TempDir(), "x"), OldExists: true, NewExists: true, + OldText: "safe old", NewText: "safe new", + } + if side == "old" { + diff.OldText = obfuscated + } else { + diff.NewText = obfuscated + } + res := scrubResultSecrets(Result{FileDiffs: []FileDiff{diff}}) + 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" diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index 4bf938f4b..9ff62260a 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -86,12 +86,13 @@ type structuredPatchTarget struct { } type structuredPatchChange struct { - kind structuredPatchKind - from structuredPatchTarget - to structuredPatchTarget - before string - after string - mode os.FileMode + kind structuredPatchKind + from structuredPatchTarget + to structuredPatchTarget + before string + after string + mode os.FileMode + beforeInfo os.FileInfo } // unifiedHunkRangePattern recognises a unified-diff range header ("-12,4 +12,6", @@ -469,15 +470,12 @@ func planStructuredPatch(root *os.Root, operations []structuredPatchOperation, t } change.after = operation.contents case structuredPatchDelete, structuredPatchUpdate, structuredPatchCopy: - info, err := root.Stat(from.relative) - if err != nil { - return nil, fmt.Errorf("stating %s: %w", from.relative, err) - } - change.mode = info.Mode() - content, err := root.ReadFile(from.relative) + content, info, err := readRootedFile(root, from.relative) if err != nil { return nil, fmt.Errorf("reading %s: %w", from.relative, err) } + change.mode = info.Mode() + change.beforeInfo = info if err := tracker.CheckConflict(from.absolute, content); err != nil { return nil, fmt.Errorf("%s", fileConflictMessage(from.relative)) } @@ -812,6 +810,11 @@ func forgetStructuredPatchFiles(tracker *FileTracker, changes []structuredPatchC // deterministically; it is nil in production. var structuredPatchBeforeCommit func(change structuredPatchChange) +// structuredPatchBeforeRename runs after a same-path update has staged and +// closed its replacement but before the final preimage recheck and rename. +// Tests use it to reproduce a competing writer deterministically. +var structuredPatchBeforeRename func(change structuredPatchChange) + func applyStructuredPatchChange(root *os.Root, change structuredPatchChange) (bool, error) { if structuredPatchBeforeCommit != nil { structuredPatchBeforeCommit(change) @@ -821,12 +824,8 @@ func applyStructuredPatchChange(root *os.Root, change structuredPatchChange) (bo // by another process in between is refused rather than overwritten or // removed. if change.kind != structuredPatchAdd { - current, err := root.ReadFile(change.from.relative) - if err != nil { - return false, fmt.Errorf("re-reading %s before commit: %w", change.from.relative, err) - } - if string(current) != change.before { - return false, fmt.Errorf("%s changed on disk between planning and commit; re-read it and retry", change.from.relative) + if err := recheckStructuredPatchPreimage(root, change); err != nil { + return false, err } } switch change.kind { @@ -835,11 +834,13 @@ func applyStructuredPatchChange(root *os.Root, change structuredPatchChange) (bo return false, fmt.Errorf("deleting %s: %w", change.from.relative, err) } return true, nil - case structuredPatchAdd, structuredPatchCopy: - return writeStructuredPatchFile(root, change.to, change.after, change.mode, true) + case structuredPatchAdd: + return writeStructuredPatchFile(root, change.to, change.after, change.mode, true, nil) + case structuredPatchCopy: + return writeStructuredPatchFile(root, change.to, change.after, change.mode, true, structuredPatchPrePublishGuard(root, change)) case structuredPatchUpdate: moving := change.from.absolute != change.to.absolute - committed, err := writeStructuredPatchFile(root, change.to, change.after, change.mode, moving) + committed, err := writeStructuredPatchFile(root, change.to, change.after, change.mode, moving, structuredPatchPrePublishGuard(root, change)) if err != nil { return committed, err } @@ -853,7 +854,27 @@ func applyStructuredPatchChange(root *os.Root, change structuredPatchChange) (bo return false, fmt.Errorf("unsupported structured patch operation") } -func writeStructuredPatchFile(root *os.Root, target structuredPatchTarget, content string, mode os.FileMode, createOnly bool) (bool, error) { +func recheckStructuredPatchPreimage(root *os.Root, change structuredPatchChange) error { + current, info, err := readRootedFile(root, change.from.relative) + if err != nil { + return fmt.Errorf("re-reading %s before commit: %w", change.from.relative, err) + } + if (change.beforeInfo != nil && !os.SameFile(change.beforeInfo, info)) || string(current) != change.before { + return fmt.Errorf("%s changed on disk between planning and commit; re-read it and retry", change.from.relative) + } + return nil +} + +func structuredPatchPrePublishGuard(root *os.Root, change structuredPatchChange) func() error { + return func() error { + if change.kind == structuredPatchUpdate && change.from.absolute == change.to.absolute && structuredPatchBeforeRename != nil { + structuredPatchBeforeRename(change) + } + return recheckStructuredPatchPreimage(root, change) + } +} + +func writeStructuredPatchFile(root *os.Root, target structuredPatchTarget, content string, mode os.FileMode, createOnly bool, beforePublish func() error) (bool, error) { parent := filepath.Dir(target.relative) if err := root.MkdirAll(parent, 0o755); err != nil { return false, fmt.Errorf("creating parent directory for %s: %w", target.relative, err) @@ -874,6 +895,11 @@ func writeStructuredPatchFile(root *os.Root, target structuredPatchTarget, conte if err := temp.Close(); err != nil { return false, fmt.Errorf("writing %s: %w", target.relative, err) } + if beforePublish != nil { + if err := beforePublish(); err != nil { + return false, err + } + } if createOnly { return publishStructuredPatchNoReplace(root, tempName, target.relative, mode) } diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index de6e60d29..b03c8791d 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -124,10 +124,13 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an // 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, finalContentKnown := maybeFormatWrittenFile(ctx, absolutePath, content) + content, formattedInfo, finalContentKnown := maybeFormatWrittenFile(ctx, tool.workspaceRoot, tool.scope, 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) + newInfo := formattedInfo + if newInfo == nil { + newInfo, _ = os.Stat(absolutePath) + } if finalContentKnown { options.FileTracker.Record(absolutePath, []byte(content), newInfo) } else { @@ -151,7 +154,9 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an lines++ } summary := fmt.Sprintf("%s %s (%d lines).", verb, relativePath, lines) - summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) + if finalContentKnown { + 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 diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 22a134a41..bc68dfa7e 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -962,6 +962,59 @@ func TestStructuredPatchMoveWithMissingSourceIsRefusedBeforePublishing(t *testin } } +func TestStructuredPatchUpdatePreservesCompetingWriteBeforeRename(t *testing.T) { + for _, tc := range []struct { + name string + competing string + replaceIdentity bool + }{ + {name: "content changed", competing: "competing writer\n"}, + {name: "identity changed with same content", competing: "planned\n", replaceIdentity: true}, + } { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + targetPath := filepath.Join(root, "file.txt") + writeTestFile(t, targetPath, "planned\n") + patch := "*** Begin Patch\n*** Update File: file.txt\n@@\n-planned\n+patched\n*** End Patch\n" + hookRan := false + var competingInfo os.FileInfo + structuredPatchBeforeRename = func(change structuredPatchChange) { + if tc.replaceIdentity { + tempPath := filepath.Join(root, "competing.tmp") + writeTestFile(t, tempPath, tc.competing) + if err := os.Rename(tempPath, targetPath); err != nil { + t.Fatal(err) + } + } else if err := os.WriteFile(targetPath, []byte(tc.competing), 0o644); err != nil { + t.Fatal(err) + } + var err error + competingInfo, err = os.Stat(targetPath) + if err != nil { + t.Fatal(err) + } + hookRan = true + } + t.Cleanup(func() { structuredPatchBeforeRename = nil }) + + result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch}) + if !hookRan { + t.Fatal("pre-rename hook did not run") + } + if result.Status != StatusError || len(result.ChangedFiles) != 0 || len(result.FileDiffs) != 0 { + t.Fatalf("raced update = status=%s changed=%#v diffs=%#v output=%q", result.Status, result.ChangedFiles, result.FileDiffs, result.Output) + } + if got := mustReadTestFile(t, targetPath); got != tc.competing { + t.Fatalf("raced update overwrote competing content: %q", got) + } + finalInfo, err := os.Stat(targetPath) + if err != nil || !os.SameFile(competingInfo, finalInfo) { + t.Fatalf("raced update replaced the competing file identity: %v", err) + } + }) + } +} + func TestStructuredPatchCreateOnlyFallsBackWhenHardLinksUnsupported(t *testing.T) { root := t.TempDir() workspace, err := os.OpenRoot(root) From 6ed27e5ef67c738c0d518b47dddb05da2134e06f Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sat, 5 Sep 2026 13:56:26 +0530 Subject: [PATCH 09/10] fix(tools): report partial patch paths from workspace --- internal/tools/apply_patch_tolerance_test.go | 32 +++++++++++++++ internal/tools/file_commit.go | 36 ---------------- internal/tools/rooted_file.go | 43 ++++++++++++++++++++ internal/tools/structured_patch.go | 8 ++-- internal/tools/write_tools_test.go | 8 ++-- 5 files changed, 83 insertions(+), 44 deletions(-) create mode 100644 internal/tools/rooted_file.go diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go index 56046c19f..f01f89639 100644 --- a/internal/tools/apply_patch_tolerance_test.go +++ b/internal/tools/apply_patch_tolerance_test.go @@ -720,3 +720,35 @@ func TestApplyPatchOperationsReportsCommittedPrefixOnFailure(t *testing.T) { t.Fatalf("partial patch FileDiffs = %#v", got) } } + +func TestApplyPatchOperationsReportsWorkspaceRelativeCommittedPrefixUnderCwd(t *testing.T) { + root := t.TempDir() + applyRoot := filepath.Join(root, "sub", "dir") + if err := os.MkdirAll(applyRoot, 0o755); err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(applyRoot, "first.txt"), "one\n") + writeTestFile(t, filepath.Join(applyRoot, "second.txt"), "two\n") + structuredPatchBeforeCommit = func(change structuredPatchChange) { + if change.to.relative == "second.txt" { + writeTestFile(t, filepath.Join(applyRoot, "second.txt"), "changed\n") + } + } + t.Cleanup(func() { structuredPatchBeforeCommit = nil }) + patch := strings.Join([]string{ + "*** Begin Patch", + "*** Update File: first.txt", "@@", "-one", "+ONE", + "*** Update File: second.txt", "@@", "-two", "+TWO", + "*** End Patch", "", + }, "\n") + + result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{ + "cwd": "sub/dir", "patch": patch, + }) + if result.Status != StatusError || !strings.Contains(result.Output, "already committed: sub/dir/first.txt") { + t.Fatalf("nested partial failure = status=%s output=%q", result.Status, result.Output) + } + if got := result.ChangedFiles; len(got) != 1 || got[0] != "sub/dir/first.txt" { + t.Fatalf("nested partial ChangedFiles = %#v", got) + } +} diff --git a/internal/tools/file_commit.go b/internal/tools/file_commit.go index 033973129..e14c0493d 100644 --- a/internal/tools/file_commit.go +++ b/internal/tools/file_commit.go @@ -101,39 +101,3 @@ func writeAndVerifyFileIdentity(path string, file *os.File, content string, trun } return nil } - -// readRootedFile returns bytes and identity from the same opened object, and -// verifies that the rooted path still names that object after the read. Root.Open -// binds symlink containment to use rather than relying on a pathname pre-check. -func readRootedFile(root *os.Root, relativePath string) ([]byte, os.FileInfo, error) { - file, err := root.Open(relativePath) - if err != nil { - return nil, nil, err - } - closed := false - defer func() { - if !closed { - _ = file.Close() - } - }() - openedInfo, err := file.Stat() - if err != nil { - return nil, nil, err - } - if !openedInfo.Mode().IsRegular() { - return nil, nil, fmt.Errorf("%s is not a regular file", relativePath) - } - content, err := io.ReadAll(file) - if err != nil { - return nil, nil, err - } - pathInfo, err := root.Stat(relativePath) - if err != nil || !os.SameFile(openedInfo, pathInfo) { - return nil, nil, fmt.Errorf("%w: path identity changed", errFileChangedDuringWrite) - } - if err := file.Close(); err != nil { - return nil, nil, err - } - closed = true - return content, openedInfo, nil -} diff --git a/internal/tools/rooted_file.go b/internal/tools/rooted_file.go new file mode 100644 index 000000000..73aad9a4a --- /dev/null +++ b/internal/tools/rooted_file.go @@ -0,0 +1,43 @@ +package tools + +import ( + "fmt" + "io" + "os" +) + +// readRootedFile returns bytes and identity from the same opened object, and +// verifies that the rooted path still names that object after the read. Root.Open +// binds symlink containment to use rather than relying on a pathname pre-check. +func readRootedFile(root *os.Root, relativePath string) ([]byte, os.FileInfo, error) { + file, err := root.Open(relativePath) + if err != nil { + return nil, nil, err + } + closed := false + defer func() { + if !closed { + _ = file.Close() + } + }() + openedInfo, err := file.Stat() + if err != nil { + return nil, nil, err + } + if !openedInfo.Mode().IsRegular() { + return nil, nil, fmt.Errorf("%s is not a regular file", relativePath) + } + content, err := io.ReadAll(file) + if err != nil { + return nil, nil, err + } + pathInfo, err := root.Stat(relativePath) + if err != nil || !os.SameFile(openedInfo, pathInfo) { + return nil, nil, fmt.Errorf("%w: path identity changed", errFileChangedDuringWrite) + } + if err := file.Close(); err != nil { + return nil, nil, err + } + closed = true + return content, openedInfo, nil +} diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index 9ff62260a..28ed3e934 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -157,7 +157,7 @@ func applyPatchOperations(applyRoot, relativeRoot string, operations []structure } } } - applyOutcome, err := applyStructuredPatchChanges(workspace, changes, options.FileTracker) + applyOutcome, err := applyStructuredPatchChanges(workspace, relativeRoot, changes, options.FileTracker) if err != nil { result := errorResult("Error applying patch: " + err.Error()) result.ChangedFiles = changedFilesFromStructuredPatch(relativeRoot, applyOutcome.committed) @@ -750,7 +750,7 @@ type structuredPatchApplyOutcome struct { incompletePaths []string } -func applyStructuredPatchChanges(root *os.Root, changes []structuredPatchChange, tracker *FileTracker) (structuredPatchApplyOutcome, error) { +func applyStructuredPatchChanges(root *os.Root, relativeRoot string, 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. @@ -762,8 +762,8 @@ func applyStructuredPatchChanges(root *os.Root, changes []structuredPatchChange, outcome.incompletePaths = append(outcome.incompletePaths, change.to.relative) } forgetStructuredPatchFiles(tracker, changes) - committedPaths := changedFilesFromStructuredPatch(".", outcome.committed) - committedPaths = appendUniqueStructuredPatchPaths(committedPaths, ".", outcome.incompletePaths) + committedPaths := changedFilesFromStructuredPatch(relativeRoot, outcome.committed) + committedPaths = appendUniqueStructuredPatchPaths(committedPaths, relativeRoot, 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, ", ")) } diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index bc68dfa7e..5b8036be0 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -884,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) } @@ -910,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") } @@ -947,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") } @@ -1123,7 +1123,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) } From c2db83c1107f1d5de21cd922ae598966554aeb1d Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Mon, 7 Sep 2026 12:17:28 +0530 Subject: [PATCH 10/10] fix(tools): finalize structured diff evidence --- internal/acp/translate_test.go | 22 ++++ internal/tools/apply_patch_tolerance_test.go | 55 +++++++++- internal/tools/file_commit.go | 23 +++-- internal/tools/file_commit_test.go | 50 +++++++++ internal/tools/registry.go | 7 ++ internal/tools/registry_test.go | 46 ++++++++- internal/tools/structured_patch.go | 102 +++++++++++++++---- internal/tools/write_tools_test.go | 40 ++++++++ 8 files changed, 309 insertions(+), 36 deletions(-) diff --git a/internal/acp/translate_test.go b/internal/acp/translate_test.go index a00e06f07..7876ce5f5 100644 --- a/internal/acp/translate_test.go +++ b/internal/acp/translate_test.go @@ -225,6 +225,28 @@ func TestToolCallResultEmitsOnlyRedactedFileDiffs(t *testing.T) { } } +func TestToolCallResultOmitsSemanticallyUnchangedRedactedDiff(t *testing.T) { + oldSecret := "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + newSecret := "ghp_9876543210ZYXWVUTSRQPONMLKJIHGFEDCBA" + scrubbed := tools.ScrubResultSecrets(tools.Result{ + ChangedFiles: []string{"credentials.txt"}, + FileDiffs: []tools.FileDiff{{ + Path: filepath.Join(t.TempDir(), "credentials.txt"), OldExists: true, NewExists: true, + OldText: "token=" + oldSecret, NewText: "token=" + newSecret, + }}, + }) + update := toolCallResult(agent.ToolResult{ + ToolCallID: "call", Status: tools.StatusOK, + ChangedFiles: scrubbed.ChangedFiles, FileDiffs: scrubbed.FileDiffs, + }) + if len(update.Content) != 0 { + t.Fatalf("ACP emitted semantically unchanged redacted diff: %#v", update.Content) + } + if len(update.Locations) != 1 || update.Locations[0].Path != "credentials.txt" { + t.Fatalf("ACP path fallback = %#v", update.Locations) + } +} + func TestToolCallResultOmitsDefaultIgnorableSplitSecretsOnEitherSide(t *testing.T) { secret := "sk-ant-api03-AAAABBBBCCCCDDDDEEEEFFFFGGGG" for name, separator := range map[string]string{ diff --git a/internal/tools/apply_patch_tolerance_test.go b/internal/tools/apply_patch_tolerance_test.go index f01f89639..b652ce85e 100644 --- a/internal/tools/apply_patch_tolerance_test.go +++ b/internal/tools/apply_patch_tolerance_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "slices" "strconv" "strings" "testing" @@ -571,9 +572,9 @@ func TestApplyStructuredPatchChangeRefusesSourceChangedAfterPlanning(t *testing. "copy": {kind: structuredPatchCopy, from: target, to: destination, before: "planned\n", after: "planned\n", mode: 0o644}, } { writeTestFile(t, path, "changed after planning\n") - committed, err := applyStructuredPatchChange(workspace, change) - if err == nil || committed || !strings.Contains(err.Error(), "changed on disk between planning and commit") { - t.Fatalf("%s: expected a refusal, got committed=%v err=%v", name, committed, err) + outcome, err := applyStructuredPatchChange(workspace, change) + if err == nil || len(outcome.completed) != 0 || len(outcome.incompletePaths) != 0 || !strings.Contains(err.Error(), "changed on disk between planning and commit") { + t.Fatalf("%s: expected a refusal, got outcome=%#v err=%v", name, outcome, err) } if content, _ := os.ReadFile(path); string(content) != "changed after planning\n" { t.Fatalf("%s: file must be untouched, got %q", name, string(content)) @@ -752,3 +753,51 @@ func TestApplyPatchOperationsReportsWorkspaceRelativeCommittedPrefixUnderCwd(t * t.Fatalf("nested partial ChangedFiles = %#v", got) } } + +func TestApplyPatchMoveReportsPublishedDestinationWhenSourceRemovalFails(t *testing.T) { + root := t.TempDir() + writeTestFile(t, filepath.Join(root, "source.txt"), "before\n") + priorRemove := structuredPatchRemove + structuredPatchRemove = func(workspace *os.Root, name string) error { + if name == "source.txt" { + return os.ErrPermission + } + return priorRemove(workspace, name) + } + t.Cleanup(func() { structuredPatchRemove = priorRemove }) + + patch := strings.Join([]string{ + "*** Begin Patch", + "*** Update File: source.txt", + "*** Move to: destination.txt", + "@@", + "-before", + "+after", + "*** End Patch", + "", + }, "\n") + result := NewScopedApplyPatchTool(root, nil).Run(context.Background(), map[string]any{"patch": patch}) + if result.Status != StatusError { + t.Fatalf("move status = %s, want error", result.Status) + } + if got := mustReadTestFile(t, filepath.Join(root, "source.txt")); got != "before\n" { + t.Fatalf("source content = %q", got) + } + if got := mustReadTestFile(t, filepath.Join(root, "destination.txt")); got != "after\n" { + t.Fatalf("destination content = %q", got) + } + if got, want := result.ChangedFiles, []string{"destination.txt"}; !slices.Equal(got, want) { + t.Fatalf("ChangedFiles = %#v, want %#v", got, want) + } + resolvedDestination, err := filepath.EvalSymlinks(filepath.Join(root, "destination.txt")) + if err != nil { + t.Fatal(err) + } + wantDiff := FileDiff{Path: resolvedDestination, OldExists: false, NewExists: true, NewText: "after\n"} + if got := result.FileDiffs; len(got) != 1 || got[0] != wantDiff { + t.Fatalf("FileDiffs = %#v, want %#v", got, []FileDiff{wantDiff}) + } + if !strings.Contains(result.Display.Preview, "destination.txt") || strings.Contains(result.Display.Preview, "source.txt") { + t.Fatalf("preview must show only destination creation: %q", result.Display.Preview) + } +} diff --git a/internal/tools/file_commit.go b/internal/tools/file_commit.go index e14c0493d..924e056dd 100644 --- a/internal/tools/file_commit.go +++ b/internal/tools/file_commit.go @@ -14,6 +14,11 @@ var errFileChangedDuringWrite = errors.New("file changed on disk before the writ // object that will actually be mutated. var fileWriteBeforeCommit func(path string) +// fileWriteStat is a deterministic test seam for proving that the opened-file +// identity is captured before the final preimage comparison. Production uses +// the file descriptor directly. +var fileWriteStat = func(file *os.File) (os.FileInfo, error) { return file.Stat() } + // 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 @@ -33,7 +38,12 @@ func commitFileContents(path string, expectedInfo os.FileInfo, expectedContent * if err != nil { return err } - return writeAndVerifyFileIdentity(path, file, content, false) + openedInfo, err := fileWriteStat(file) + if err != nil { + _ = file.Close() + return err + } + return writeAndVerifyFileIdentity(path, file, openedInfo, content, false) } flags := os.O_WRONLY @@ -44,7 +54,7 @@ func commitFileContents(path string, expectedInfo os.FileInfo, expectedContent * if err != nil { return err } - openedInfo, err := file.Stat() + openedInfo, err := fileWriteStat(file) if err != nil { _ = file.Close() return err @@ -69,15 +79,10 @@ func commitFileContents(path string, expectedInfo os.FileInfo, expectedContent * return errFileChangedDuringWrite } } - return writeAndVerifyFileIdentity(path, file, content, true) + return writeAndVerifyFileIdentity(path, file, openedInfo, 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 - } +func writeAndVerifyFileIdentity(path string, file *os.File, openedInfo os.FileInfo, content string, truncate bool) error { if truncate { if err := file.Truncate(0); err != nil { _ = file.Close() diff --git a/internal/tools/file_commit_test.go b/internal/tools/file_commit_test.go index c85719537..8347412fc 100644 --- a/internal/tools/file_commit_test.go +++ b/internal/tools/file_commit_test.go @@ -15,6 +15,13 @@ func installFileWriteRace(t *testing.T, mutate func(string)) { t.Cleanup(func() { fileWriteBeforeCommit = prior }) } +func installFileWriteStat(t *testing.T, stat func(*os.File) (os.FileInfo, error)) { + t.Helper() + prior := fileWriteStat + fileWriteStat = stat + t.Cleanup(func() { fileWriteStat = prior }) +} + func TestWriteFileRefusesCreateAndOverwriteRaces(t *testing.T) { t.Run("create", func(t *testing.T) { root := t.TempDir() @@ -79,3 +86,46 @@ func TestEditFileRefusesPreimageRace(t *testing.T) { t.Fatalf("raced edit content = %q, err=%v", got, err) } } + +func TestOverwriteDoesNotStatOpenedFileAfterFinalPreimageComparison(t *testing.T) { + for name, run := range map[string]func(string) Result{ + "write overwrite": func(root string) Result { + return NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "content": "zero\n", "overwrite": true, + }) + }, + "edit": func(root string) Result { + return NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "old_string": "observed", "new_string": "zero", + }) + }, + } { + t.Run(name, 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) + } + + statCalls := 0 + installFileWriteStat(t, func(file *os.File) (os.FileInfo, error) { + statCalls++ + if statCalls == 2 { + // This preserves the inode, so identity-only checks cannot detect it. + if err := os.WriteFile(target, []byte("other writer\n"), 0o644); err != nil { + t.Fatal(err) + } + } + return file.Stat() + }) + + result := run(root) + if result.Status != StatusOK { + t.Fatalf("overwrite status = %s: %s", result.Status, result.Output) + } + if statCalls != 1 { + t.Fatalf("opened file was statted %d times; the final byte comparison must be followed directly by mutation", statCalls) + } + }) + } +} diff --git a/internal/tools/registry.go b/internal/tools/registry.go index 074092ebc..e553c2e55 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -360,6 +360,13 @@ func scrubResultSecrets(res Result) Result { diff.NewText = scrubbed res.Redacted = true } + // Redaction can collapse two distinct credentials to the same token. At + // this final outbound boundary, retain rich evidence only when the + // transformed sides still describe a real transition; ChangedFiles + // remains the safe path-only fallback. + if diff.OldExists == diff.NewExists && diff.OldText == diff.NewText { + continue + } fileDiffs = append(fileDiffs, diff) } res.FileDiffs = fileDiffs diff --git a/internal/tools/registry_test.go b/internal/tools/registry_test.go index b78de7903..7bc179818 100644 --- a/internal/tools/registry_test.go +++ b/internal/tools/registry_test.go @@ -447,7 +447,7 @@ func TestScrubResultSecretsRedactsPreview(t *testing.T) { secret := "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" 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}}, + FileDiffs: []FileDiff{{Path: filepath.Join(t.TempDir(), "x"), OldExists: true, NewExists: true, OldText: "before " + secret, NewText: "after " + 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) @@ -455,7 +455,7 @@ func TestScrubResultSecretsRedactsPreview(t *testing.T) { 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) { + if len(res.FileDiffs) != 1 || strings.Contains(res.FileDiffs[0].OldText, secret) || strings.Contains(res.FileDiffs[0].NewText, secret) { t.Errorf("FileDiff must be redacted: %#v", res.FileDiffs) } } @@ -519,6 +519,48 @@ func TestScrubResultSecretsDoesNotMutateCallerFileDiffSlice(t *testing.T) { } } +func TestScrubResultSecretsDropsSemanticallyUnchangedRedactedDiff(t *testing.T) { + path := filepath.Join(t.TempDir(), "credentials.txt") + oldSecret := "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + newSecret := "ghp_9876543210ZYXWVUTSRQPONMLKJIHGFEDCBA" + result := scrubResultSecrets(Result{ + ChangedFiles: []string{"credentials.txt"}, + FileDiffs: []FileDiff{{ + Path: path, OldExists: true, NewExists: true, + OldText: "token=" + oldSecret, NewText: "token=" + newSecret, + }}, + }) + if len(result.FileDiffs) != 0 { + t.Fatalf("semantically unchanged redacted diff = %#v", result.FileDiffs) + } + if got, want := result.ChangedFiles, []string{"credentials.txt"}; !slices.Equal(got, want) { + t.Fatalf("ChangedFiles = %#v, want %#v", got, want) + } + if !result.Redacted { + t.Fatal("credential rotation must set Redacted") + } +} + +func TestScrubResultSecretsKeepsRealTransitionAroundRedactedSecrets(t *testing.T) { + path := filepath.Join(t.TempDir(), "credentials.txt") + oldSecret := "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + newSecret := "ghp_9876543210ZYXWVUTSRQPONMLKJIHGFEDCBA" + result := scrubResultSecrets(Result{FileDiffs: []FileDiff{{ + Path: path, OldExists: true, NewExists: true, + OldText: "environment=staging token=" + oldSecret, + NewText: "environment=production token=" + newSecret, + }}}) + if len(result.FileDiffs) != 1 { + t.Fatalf("redacted real transition = %#v", result.FileDiffs) + } + if result.FileDiffs[0].OldText == result.FileDiffs[0].NewText { + t.Fatalf("redacted transition became unchanged: %#v", result.FileDiffs[0]) + } + if strings.Contains(result.FileDiffs[0].OldText, oldSecret) || strings.Contains(result.FileDiffs[0].NewText, newSecret) { + t.Fatalf("redacted transition leaked a secret: %#v", result.FileDiffs[0]) + } +} + func TestRunWithOptionsScrubsSecretsOnDenialPaths(t *testing.T) { secret := "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" reg := NewRegistry() diff --git a/internal/tools/structured_patch.go b/internal/tools/structured_patch.go index 28ed3e934..19c9b4360 100644 --- a/internal/tools/structured_patch.go +++ b/internal/tools/structured_patch.go @@ -750,17 +750,21 @@ type structuredPatchApplyOutcome struct { incompletePaths []string } +type structuredPatchChangeOutcome struct { + completed []structuredPatchChange + incompletePaths []string +} + func applyStructuredPatchChanges(root *os.Root, relativeRoot string, 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 outcome structuredPatchApplyOutcome for _, change := range changes { - done, err := applyStructuredPatchChange(root, change) + changeOutcome, err := applyStructuredPatchChange(root, change) + outcome.committed = append(outcome.committed, changeOutcome.completed...) + outcome.incompletePaths = append(outcome.incompletePaths, changeOutcome.incompletePaths...) if err != nil { - if done && change.to.relative != "" { - outcome.incompletePaths = append(outcome.incompletePaths, change.to.relative) - } forgetStructuredPatchFiles(tracker, changes) committedPaths := changedFilesFromStructuredPatch(relativeRoot, outcome.committed) committedPaths = appendUniqueStructuredPatchPaths(committedPaths, relativeRoot, outcome.incompletePaths) @@ -769,13 +773,56 @@ func applyStructuredPatchChanges(root *os.Root, relativeRoot string, changes []s } return outcome, err } - if done { - outcome.committed = append(outcome.committed, change) - } } return outcome, nil } +// completedStructuredPatchEffect turns a partially completed compound change +// into the exact filesystem sub-effect that is still verifiable after the +// error. In particular, a failed move may have published its destination while +// leaving the source in place; that is a destination creation, not a move. +func completedStructuredPatchEffect(root *os.Root, change structuredPatchChange) (structuredPatchChange, bool) { + if change.to.relative == "" { + return structuredPatchChange{}, false + } + content, _, err := readRootedFile(root, change.to.relative) + if err != nil || string(content) != change.after { + return structuredPatchChange{}, false + } + switch change.kind { + case structuredPatchAdd, structuredPatchCopy: + return structuredPatchChange{ + kind: structuredPatchAdd, + to: change.to, + after: change.after, + mode: change.mode, + }, true + case structuredPatchUpdate: + if change.from.absolute != change.to.absolute { + return structuredPatchChange{ + kind: structuredPatchAdd, + to: change.to, + after: change.after, + mode: change.mode, + }, true + } + } + return structuredPatchChange{}, false +} + +func incompleteStructuredPatchWrite(root *os.Root, change structuredPatchChange, published bool) structuredPatchChangeOutcome { + if !published { + return structuredPatchChangeOutcome{} + } + if completed, ok := completedStructuredPatchEffect(root, change); ok { + return structuredPatchChangeOutcome{completed: []structuredPatchChange{completed}} + } + if change.to.relative != "" { + return structuredPatchChangeOutcome{incompletePaths: []string{change.to.relative}} + } + return structuredPatchChangeOutcome{} +} + func appendUniqueStructuredPatchPaths(existing []string, relativeRoot string, paths []string) []string { seen := make(map[string]bool, len(existing)+len(paths)) for _, path := range existing { @@ -815,7 +862,12 @@ var structuredPatchBeforeCommit func(change structuredPatchChange) // Tests use it to reproduce a competing writer deterministically. var structuredPatchBeforeRename func(change structuredPatchChange) -func applyStructuredPatchChange(root *os.Root, change structuredPatchChange) (bool, error) { +// structuredPatchRemove is a deterministic test seam for failures after a +// move destination has been published. Production removes through the opened +// workspace root. +var structuredPatchRemove = func(root *os.Root, name string) error { return root.Remove(name) } + +func applyStructuredPatchChange(root *os.Root, change structuredPatchChange) (structuredPatchChangeOutcome, error) { if structuredPatchBeforeCommit != nil { structuredPatchBeforeCommit(change) } @@ -825,33 +877,39 @@ func applyStructuredPatchChange(root *os.Root, change structuredPatchChange) (bo // removed. if change.kind != structuredPatchAdd { if err := recheckStructuredPatchPreimage(root, change); err != nil { - return false, err + return structuredPatchChangeOutcome{}, err } } switch change.kind { case structuredPatchDelete: - if err := root.Remove(change.from.relative); err != nil { - return false, fmt.Errorf("deleting %s: %w", change.from.relative, err) + if err := structuredPatchRemove(root, change.from.relative); err != nil { + return structuredPatchChangeOutcome{}, fmt.Errorf("deleting %s: %w", change.from.relative, err) } - return true, nil - case structuredPatchAdd: - return writeStructuredPatchFile(root, change.to, change.after, change.mode, true, nil) - case structuredPatchCopy: - return writeStructuredPatchFile(root, change.to, change.after, change.mode, true, structuredPatchPrePublishGuard(root, change)) + return structuredPatchChangeOutcome{completed: []structuredPatchChange{change}}, nil + case structuredPatchAdd, structuredPatchCopy: + var beforePublish func() error + if change.kind == structuredPatchCopy { + beforePublish = structuredPatchPrePublishGuard(root, change) + } + published, err := writeStructuredPatchFile(root, change.to, change.after, change.mode, true, beforePublish) + if err != nil { + return incompleteStructuredPatchWrite(root, change, published), err + } + return structuredPatchChangeOutcome{completed: []structuredPatchChange{change}}, nil case structuredPatchUpdate: moving := change.from.absolute != change.to.absolute - committed, err := writeStructuredPatchFile(root, change.to, change.after, change.mode, moving, structuredPatchPrePublishGuard(root, change)) + published, err := writeStructuredPatchFile(root, change.to, change.after, change.mode, moving, structuredPatchPrePublishGuard(root, change)) if err != nil { - return committed, err + return incompleteStructuredPatchWrite(root, change, published), err } if moving { - if err := root.Remove(change.from.relative); err != nil { - return true, fmt.Errorf("removing moved source %s: %w", change.from.relative, err) + if err := structuredPatchRemove(root, change.from.relative); err != nil { + return incompleteStructuredPatchWrite(root, change, true), fmt.Errorf("removing moved source %s: %w", change.from.relative, err) } } - return true, nil + return structuredPatchChangeOutcome{completed: []structuredPatchChange{change}}, nil } - return false, fmt.Errorf("unsupported structured patch operation") + return structuredPatchChangeOutcome{}, fmt.Errorf("unsupported structured patch operation") } func recheckStructuredPatchPreimage(root *os.Root, change structuredPatchChange) error { diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 5b8036be0..29afd57b2 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "strings" "testing" ) @@ -1084,6 +1085,45 @@ func TestStructuredPatchCopyFailureReportsSurvivingPartialTarget(t *testing.T) { } } +func TestIncompleteStructuredPatchNoReplaceWriteReportsOnlyVerifiedTarget(t *testing.T) { + for name, content := range map[string]string{ + "complete publication": "complete content\n", + "partial publication": "partial", + } { + t.Run(name, func(t *testing.T) { + root := t.TempDir() + workspace, err := os.OpenRoot(root) + if err != nil { + t.Fatal(err) + } + defer workspace.Close() + targetPath := filepath.Join(root, "target.txt") + writeTestFile(t, targetPath, content) + change := structuredPatchChange{ + kind: structuredPatchAdd, + to: structuredPatchTarget{ + requested: "target.txt", + relative: "target.txt", + absolute: targetPath, + }, + after: "complete content\n", + mode: 0o644, + } + + outcome := incompleteStructuredPatchWrite(workspace, change, true) + if content == change.after { + if len(outcome.completed) != 1 || outcome.completed[0].kind != structuredPatchAdd || len(outcome.incompletePaths) != 0 { + t.Fatalf("verified publication outcome = %#v", outcome) + } + return + } + if len(outcome.completed) != 0 || !slices.Equal(outcome.incompletePaths, []string{"target.txt"}) { + t.Fatalf("partial publication outcome = %#v", outcome) + } + }) + } +} + func TestStructuredPatchPartialFailureLeavesCompletedChangeAndClearsTrackedState(t *testing.T) { root := t.TempDir() trackedPath := filepath.Join(root, "tracked.txt")