Skip to content
59 changes: 55 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,27 +121,77 @@ 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 {
// 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 []ToolCallContent{ToolContent(TextBlock(text))}
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 := strings.TrimSpace(diff.Path)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
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))
Expand Down
70 changes: 67 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) != 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"})
Expand All @@ -97,6 +104,63 @@ 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 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)
}
}

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