diff --git a/pkg/tools/builtin/filesystem/filesystem.go b/pkg/tools/builtin/filesystem/filesystem.go index 5b8d1856d..3996e126a 100644 --- a/pkg/tools/builtin/filesystem/filesystem.go +++ b/pkg/tools/builtin/filesystem/filesystem.go @@ -1638,13 +1638,14 @@ func (t *ToolSet) handleCreateDirectory(ctx context.Context, args CreateDirector ) } var results []string - for _, path := range args.Paths { + for i, path := range args.Paths { resolvedPath, err := t.resolveAndCheckPath(path) if err != nil { - return tools.ResultError(err.Error()), nil + return tools.ResultError(batchAbort(results, err.Error(), args.Paths[i+1:])), nil } if err := t.mkdirAll(resolvedPath, 0o755); err != nil { - return tools.ResultError(fmt.Sprintf("Error creating directory %s: %s", path, err)), nil + return tools.ResultError(batchAbort(results, + fmt.Sprintf("Error creating directory %s: %s", path, err), args.Paths[i+1:])), nil } results = append(results, "Directory created successfully: "+path) } @@ -1652,6 +1653,27 @@ func (t *ToolSet) handleCreateDirectory(ctx context.Context, args CreateDirector return tools.ResultSuccess(strings.Join(results, "\n")), nil } +// batchAbort renders the outcome of a path batch that stopped partway: what +// already succeeded, the error that stopped it, and what was never attempted. +// +// All three parts are needed for the caller to know the filesystem state. These +// loops stop at the first error but do not roll back, so the error alone reads +// as a no-op; and without the untouched tail the caller still cannot tell "not +// processed" from "processed but unreported", which is the retry ambiguity this +// reporting exists to remove. +// +// Each part is omitted when empty, so a single-path failure keeps the bare error +// message it has always had. +func batchAbort(completed []string, errMsg string, remaining []string) string { + parts := make([]string, 0, len(completed)+2) + parts = append(parts, completed...) + parts = append(parts, errMsg) + if len(remaining) > 0 { + parts = append(parts, "Stopped before: "+strings.Join(remaining, ", ")) + } + return strings.Join(parts, "\n") +} + func (t *ToolSet) handleRemoveDirectory(ctx context.Context, args RemoveDirectoryArgs) (*tools.ToolCallResult, error) { annotateFilesystemSpan(ctx, "remove_directory", "") if span := trace.SpanFromContext(ctx); span.IsRecording() { @@ -1661,14 +1683,15 @@ func (t *ToolSet) handleRemoveDirectory(ctx context.Context, args RemoveDirector ) } var results []string - for _, path := range args.Paths { + for i, path := range args.Paths { resolvedPath, err := t.resolveAndCheckPath(path) if err != nil { - return tools.ResultError(err.Error()), nil + return tools.ResultError(batchAbort(results, err.Error(), args.Paths[i+1:])), nil } if err := t.removeDir(resolvedPath); err != nil { - return tools.ResultError(fmt.Sprintf("Error removing directory %s: %s", path, err)), nil + return tools.ResultError(batchAbort(results, + fmt.Sprintf("Error removing directory %s: %s", path, err), args.Paths[i+1:])), nil } results = append(results, "Directory removed successfully: "+path) } diff --git a/pkg/tools/builtin/filesystem/filesystem_test.go b/pkg/tools/builtin/filesystem/filesystem_test.go index c09e76ef1..5591445de 100644 --- a/pkg/tools/builtin/filesystem/filesystem_test.go +++ b/pkg/tools/builtin/filesystem/filesystem_test.go @@ -1374,6 +1374,169 @@ func TestFilesystemTool_RemoveDirectory_MultipleStopsOnError(t *testing.T) { assert.DirExists(t, dir3) } +// A batch that aborts partway has already changed the filesystem. Reporting only +// the error reads as a no-op, so the agent cannot know what was done — and for +// remove_directory the completed work is not undoable. +func TestFilesystemTool_DirectoryBatch_PartialFailureReportsCompletedWork(t *testing.T) { + t.Parallel() + + t.Run("remove_directory names the directories it already removed", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + + dir1 := filepath.Join(tmpDir, "dir1") + dir2 := filepath.Join(tmpDir, "dir2") + require.NoError(t, os.Mkdir(dir1, 0o755)) + require.NoError(t, os.Mkdir(dir2, 0o755)) + + result, err := tool.handleRemoveDirectory(t.Context(), RemoveDirectoryArgs{ + Paths: []string{"dir1", "dir2", "nonexistent"}, + }) + require.NoError(t, err) + assert.True(t, result.IsError) + + // Both were really removed, so both must appear in the result. + require.NoDirExists(t, dir1) + require.NoDirExists(t, dir2) + assert.Contains(t, result.Output, "Directory removed successfully: dir1") + assert.Contains(t, result.Output, "Directory removed successfully: dir2") + assert.Contains(t, result.Output, "nonexistent") + }) + + t.Run("create_directory names the directories it already created", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + + // A regular file makes MkdirAll fail for any path below it. + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "blocker"), []byte("x"), 0o644)) + + result, err := tool.handleCreateDirectory(t.Context(), CreateDirectoryArgs{ + Paths: []string{"made1", "made2", filepath.Join("blocker", "sub")}, + }) + require.NoError(t, err) + assert.True(t, result.IsError) + + require.DirExists(t, filepath.Join(tmpDir, "made1")) + require.DirExists(t, filepath.Join(tmpDir, "made2")) + assert.Contains(t, result.Output, "Directory created successfully: made1") + assert.Contains(t, result.Output, "Directory created successfully: made2") + }) + + // Nothing completed before the failure: the message must stay exactly as it + // was, with no empty leading line from an empty completed-work list. + t.Run("failure on the first path reports no completed work", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + require.NoError(t, os.Mkdir(filepath.Join(tmpDir, "dir2"), 0o755)) + + result, err := tool.handleRemoveDirectory(t.Context(), RemoveDirectoryArgs{ + Paths: []string{"nonexistent", "dir2"}, + }) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.NotContains(t, result.Output, "successfully") + assert.False(t, strings.HasPrefix(result.Output, "\n"), + "no blank leading line when nothing completed: %q", result.Output) + assert.DirExists(t, filepath.Join(tmpDir, "dir2"), "processing still stops at the first error") + }) + + // A single-path call has no completed work and no untouched tail, so it must + // keep the bare error message it has always had. + t.Run("single path failure keeps the bare error message", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + + result, err := tool.handleRemoveDirectory(t.Context(), RemoveDirectoryArgs{ + Paths: []string{"nonexistent"}, + }) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.NotContains(t, result.Output, "\n", "no extra lines for a single-path batch") + assert.NotContains(t, result.Output, "Stopped before") + }) + + // Reporting what already happened is only half the ambiguity: without the + // untouched tail the caller still cannot tell "not processed" from + // "processed but unreported". + t.Run("paths never attempted are named", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + + for _, name := range []string{"a", "b", "d", "e"} { + require.NoError(t, os.Mkdir(filepath.Join(tmpDir, name), 0o755)) + } + notEmpty := filepath.Join(tmpDir, "notempty") + require.NoError(t, os.Mkdir(notEmpty, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(notEmpty, "f.txt"), []byte("x"), 0o644)) + + result, err := tool.handleRemoveDirectory(t.Context(), RemoveDirectoryArgs{ + Paths: []string{"a", "b", "notempty", "d", "e"}, + }) + require.NoError(t, err) + require.True(t, result.IsError) + + // a and b are gone; d and e were never touched. + require.NoDirExists(t, filepath.Join(tmpDir, "a")) + require.NoDirExists(t, filepath.Join(tmpDir, "b")) + require.DirExists(t, filepath.Join(tmpDir, "d")) + require.DirExists(t, filepath.Join(tmpDir, "e")) + + assert.Contains(t, result.Output, "Directory removed successfully: a") + assert.Contains(t, result.Output, "Directory removed successfully: b") + assert.Contains(t, result.Output, "Stopped before: d, e") + }) + + // The two resolveAndCheckPath early returns are otherwise uncovered: the + // existing allow-list test rejects on the first path, so completed is always + // empty there and the reporting wiring never runs. + t.Run("allow-list rejection reports completed work and the untouched tail", func(t *testing.T) { + t.Parallel() + wd := t.TempDir() + outside := filepath.Join(t.TempDir(), "outside") + require.NoError(t, os.Mkdir(outside, 0o755)) + require.NoError(t, os.Mkdir(filepath.Join(wd, "gone"), 0o755)) + require.NoError(t, os.Mkdir(filepath.Join(wd, "later"), 0o755)) + + tool := newTestToolSet(t, wd, WithAllowList([]string{"."})) + + result, err := tool.handleRemoveDirectory(t.Context(), RemoveDirectoryArgs{ + Paths: []string{"gone", outside, "later"}, + }) + require.NoError(t, err) + require.True(t, result.IsError) + + require.NoDirExists(t, filepath.Join(wd, "gone")) + require.DirExists(t, filepath.Join(wd, "later")) + + assert.Contains(t, result.Output, "Directory removed successfully: gone") + assert.Contains(t, result.Output, "outside the allowed directories") + assert.Contains(t, result.Output, "Stopped before: later") + }) + + t.Run("create_directory names the untouched tail too", func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + tool := New(tmpDir) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "blocker"), []byte("x"), 0o644)) + + result, err := tool.handleCreateDirectory(t.Context(), CreateDirectoryArgs{ + Paths: []string{"made1", filepath.Join("blocker", "sub"), "never"}, + }) + require.NoError(t, err) + require.True(t, result.IsError) + + require.DirExists(t, filepath.Join(tmpDir, "made1")) + require.NoDirExists(t, filepath.Join(tmpDir, "never")) + assert.Contains(t, result.Output, "Directory created successfully: made1") + assert.Contains(t, result.Output, "Stopped before: never") + }) +} + func createTestPNG(t *testing.T, w, h int) []byte { t.Helper() img := image.NewRGBA(image.Rect(0, 0, w, h))