Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 42 additions & 4 deletions internal/acp/translate.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package acp

import (
"encoding/json"
"path/filepath"
"strings"
"unicode/utf8"

Expand Down Expand Up @@ -120,22 +121,59 @@ func toolCallResult(result agent.ToolResult) ToolCallUpdate {
}

func toolResultContent(result agent.ToolResult) []ToolCallContent {
content := make([]ToolCallContent, 0, 1+len(result.FileDiffs))
text := strings.TrimRight(result.Output, "\n")
if text == "" {
text = result.Display.Summary
}
if text == "" {
return nil
return appendToolResultDiffs(content, result.FileDiffs)
}
return []ToolCallContent{ToolContent(TextBlock(text))}
content = append(content, ToolContent(TextBlock(text)))
return appendToolResultDiffs(content, result.FileDiffs)
}

func appendToolResultDiffs(content []ToolCallContent, diffs []tools.FileDiff) []ToolCallContent {
for _, diff := range diffs {
// ACP's diff block has no file-existence bit. A deleted file and an
// existing file replaced with empty content would otherwise serialize
// identically, so omit deletions rather than present a false truncation.
// ChangedFiles remains the conservative fallback for the operation.
if !filepath.IsAbs(diff.Path) || !diff.NewExists {
continue
}
newText := diff.NewText
var oldText *string
if diff.OldExists {
old := diff.OldText
oldText = &old
}
content = append(content, ToolCallContent{Type: "diff", Path: diff.Path, OldText: oldText, NewText: &newText})
}
return content
}

func toolResultLocations(result agent.ToolResult) []ToolCallLocation {
locs := make([]ToolCallLocation, 0, len(result.ChangedFiles))
locs := make([]ToolCallLocation, 0, len(result.FileDiffs)+len(result.ChangedFiles))
seen := make(map[string]bool, len(result.FileDiffs)+len(result.ChangedFiles))
for _, diff := range result.FileDiffs {
path := diff.Path
if path == "" || seen[path] {
continue
}
seen[path] = true
locs = append(locs, ToolCallLocation{Path: path})
}
// FileDiff.Path is canonical absolute path data while ChangedFiles is
// normally workspace-relative. Without the trusted workspace root these
// coordinate systems cannot be correlated safely: a suffix match would let
// /workspace/sub/a.go consume the fallback for a distinct root a.go. The
// shared seen set deduplicates only identities already exactly comparable.
for _, f := range result.ChangedFiles {
if strings.TrimSpace(f) == "" {
if f == "" || seen[f] {
continue
}
seen[f] = true
locs = append(locs, ToolCallLocation{Path: f})
}
return locs
Expand Down
134 changes: 131 additions & 3 deletions internal/acp/translate_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package acp

import (
"encoding/json"
"path/filepath"
"strings"
"testing"
"unicode/utf8"
Expand Down Expand Up @@ -74,21 +76,26 @@ func TestToolCallStart(t *testing.T) {
}

func TestToolCallResult(t *testing.T) {
path := filepath.Join(t.TempDir(), "a.go")
ok := toolCallResult(agent.ToolResult{
ToolCallID: "tc1",
Name: "edit_file",
Status: tools.StatusOK,
Output: "applied\n",
ChangedFiles: []string{"a.go", ""},
FileDiffs: []tools.FileDiff{{Path: path, OldExists: true, NewExists: true, OldText: "before\n", NewText: "after\n"}},
})
if ok.SessionUpdate != UpdateToolCallUpdate || ok.Status != ToolStatusCompleted {
t.Fatalf("unexpected ok result: %+v", ok)
}
if len(ok.Content) != 1 || ok.Content[0].Type != "content" || ok.Content[0].Content.Text != "applied" {
if len(ok.Content) != 2 || ok.Content[0].Type != "content" || ok.Content[0].Content.Text != "applied" {
t.Fatalf("unexpected content: %+v", ok.Content)
}
if len(ok.Locations) != 1 || ok.Locations[0].Path != "a.go" {
t.Fatalf("blank changed files should be dropped, got %+v", ok.Locations)
if diff := ok.Content[1]; diff.Type != "diff" || diff.Path != path || diff.OldText == nil || *diff.OldText != "before\n" || diff.NewText == nil || *diff.NewText != "after\n" {
t.Fatalf("unexpected diff content: %+v", diff)
}
if len(ok.Locations) != 2 || ok.Locations[0].Path != path || ok.Locations[1].Path != "a.go" {
t.Fatalf("unproven absolute/relative aliases must both remain visible, got %+v", ok.Locations)
}

failed := toolCallResult(agent.ToolResult{ToolCallID: "tc2", Status: tools.StatusError, Output: "boom"})
Expand All @@ -97,6 +104,127 @@ func TestToolCallResult(t *testing.T) {
}
}

func TestToolCallDiffJSONPreservesEmptyFilesWithoutClaimingDeletion(t *testing.T) {
path := filepath.Join(t.TempDir(), "empty.txt")
content := appendToolResultDiffs(nil, []tools.FileDiff{
{Path: path, OldExists: false, NewExists: true, NewText: ""},
{Path: path, OldExists: true, NewExists: true, OldText: "before", NewText: ""},
{Path: path, OldExists: true, NewExists: false, OldText: "before"},
})
if len(content) != 2 {
t.Fatalf("diff content = %#v", content)
}
for index, diff := range content {
encoded, err := json.Marshal(diff)
if err != nil {
t.Fatal(err)
}
var wire map[string]any
if err := json.Unmarshal(encoded, &wire); err != nil {
t.Fatal(err)
}
if wire["path"] != path || wire["newText"] != "" {
t.Fatalf("wire diff %d = %s", index, encoded)
}
if index == 0 && wire["oldText"] != nil {
t.Fatalf("create oldText = %#v, want null", wire["oldText"])
}
if index == 1 && wire["oldText"] != "before" {
t.Fatalf("update oldText = %#v, want before", wire["oldText"])
}
}
}

func TestToolResultLocationsPreserveDistinctPathIdentities(t *testing.T) {
root := t.TempDir()
rootPath := filepath.Join(root, "a.go")
nestedPath := filepath.Join(root, "sub", "a.go")
diff := func(path string) tools.FileDiff {
return tools.FileDiff{Path: path, OldExists: true, NewExists: true, OldText: "before", NewText: "after"}
}
for _, tc := range []struct {
name string
diffs []tools.FileDiff
want []string
}{
{name: "both rich", diffs: []tools.FileDiff{diff(rootPath), diff(nestedPath)}, want: []string{rootPath, nestedPath, "a.go", filepath.Join("sub", "a.go")}},
{name: "root rich", diffs: []tools.FileDiff{diff(rootPath)}, want: []string{rootPath, "a.go", filepath.Join("sub", "a.go")}},
{name: "nested rich", diffs: []tools.FileDiff{diff(nestedPath)}, want: []string{nestedPath, "a.go", filepath.Join("sub", "a.go")}},
} {
t.Run(tc.name, func(t *testing.T) {
locations := toolResultLocations(agent.ToolResult{
ChangedFiles: []string{"a.go", filepath.Join("sub", "a.go")},
FileDiffs: tc.diffs,
})
if len(locations) != len(tc.want) {
t.Fatalf("locations = %#v, want %#v", locations, tc.want)
}
for index := range tc.want {
if locations[index].Path != tc.want[index] {
t.Fatalf("locations = %#v, want %#v", locations, tc.want)
}
}
})
}
}

func TestToolCallResultPreservesWhitespaceInFilePaths(t *testing.T) {
relativePath := " report.txt "
absolutePath := filepath.Join(t.TempDir(), relativePath)
update := toolCallResult(agent.ToolResult{
ChangedFiles: []string{relativePath},
FileDiffs: []tools.FileDiff{{
Path: absolutePath, OldExists: true, NewExists: true, OldText: "before", NewText: "after",
}},
})
if len(update.Content) != 1 || update.Content[0].Path != absolutePath {
t.Fatalf("diff content path = %#v, want %q", update.Content, absolutePath)
}
if len(update.Locations) != 2 || update.Locations[0].Path != absolutePath || update.Locations[1].Path != relativePath {
t.Fatalf("locations = %#v, want exact paths %q and %q", update.Locations, absolutePath, relativePath)
}
}

func TestToolResultLocationsDeduplicateOnlyExactPaths(t *testing.T) {
path := filepath.Join(t.TempDir(), "a.go")
locations := toolResultLocations(agent.ToolResult{
ChangedFiles: []string{path, path},
FileDiffs: []tools.FileDiff{{Path: path, OldExists: true, NewExists: true, OldText: "before", NewText: "after"}},
})
if len(locations) != 1 || locations[0].Path != path {
t.Fatalf("exact duplicate locations = %#v", locations)
}
}

func TestDeletedFileKeepsPathOnlyLocation(t *testing.T) {
relativePath := "deleted.go"
absolutePath := filepath.Join(t.TempDir(), relativePath)
update := toolCallResult(agent.ToolResult{
ChangedFiles: []string{relativePath},
FileDiffs: []tools.FileDiff{{
Path: absolutePath, OldExists: true, NewExists: false, OldText: "before",
}},
})
if len(update.Content) != 0 {
t.Fatalf("deleted file must not emit an ambiguous ACP diff: %#v", update.Content)
}
if len(update.Locations) != 2 || update.Locations[0].Path != absolutePath || update.Locations[1].Path != relativePath {
t.Fatalf("deleted file locations = %#v", update.Locations)
}
}

func TestToolCallResultEmitsOnlyRedactedFileDiffs(t *testing.T) {
secret := "sk-proj-abcdefghijklmnopqrstuvwxyz"
path := filepath.Join(t.TempDir(), "secret.txt")
scrubbed := tools.ScrubResultSecrets(tools.Result{FileDiffs: []tools.FileDiff{{
Path: path, OldExists: true, NewExists: true, OldText: "token=" + secret, NewText: "safe",
}}})
update := toolCallResult(agent.ToolResult{ToolCallID: "call", Status: tools.StatusError, FileDiffs: scrubbed.FileDiffs})
if len(update.Content) != 1 || update.Content[0].OldText == nil || strings.Contains(*update.Content[0].OldText, secret) {
t.Fatalf("ACP content leaked unredacted diff: %#v", update.Content)
}
}

func TestPlanUpdateAndStatus(t *testing.T) {
upd := planUpdate([]tools.PlanItem{
{Content: "step a", Status: "completed"},
Expand Down
27 changes: 24 additions & 3 deletions internal/acp/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions internal/agent/loop.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1833,6 +1834,10 @@ func runToolForUnsandboxedRetry(ctx context.Context, registry *tools.Registry, n
}

func toolResultFromPrePermissionReject(call ToolCall, result tools.Result) ToolResult {
// PrePermissionRejecter runs before Registry.RunWithOptions, so it must
// explicitly cross the same transcript/redaction boundary before its result
// can be forwarded through ACP.
result = tools.ScrubResultSecrets(result)
output, outputRedacted := scrubInterceptedOutput(result.Output)
display := result.Display
summary, summaryRedacted := scrubInterceptedOutput(display.Summary)
Expand Down Expand Up @@ -1860,6 +1865,7 @@ func toolResultFromPrePermissionReject(call ToolCall, result tools.Result) ToolR
Meta: meta,
Redacted: result.Redacted || outputRedacted || summaryRedacted || metaRedacted,
ChangedFiles: result.ChangedFiles,
FileDiffs: result.FileDiffs,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
ChangeSummaries: result.ChangeSummaries,
Display: display,
LoadedTools: loadedToolsFromResult(meta),
Expand Down Expand Up @@ -2153,6 +2159,7 @@ func askUserFallbackResult(ctx context.Context, registry *tools.Registry, call T
Meta: result.Meta,
Redacted: result.Redacted,
ChangedFiles: result.ChangedFiles,
FileDiffs: result.FileDiffs,
ChangeSummaries: result.ChangeSummaries,
Display: result.HumanDisplay(),
Outcome: result.Outcome,
Expand Down
17 changes: 17 additions & 0 deletions internal/agent/loop_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
1 change: 1 addition & 0 deletions internal/agent/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 11 additions & 1 deletion internal/tools/apply_patch_tolerance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -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)
}
}
Loading
Loading