From 97c5d06f20310622a9710ccaf159413500ec72eb Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Thu, 6 Aug 2026 18:19:22 +0330 Subject: [PATCH 1/4] fix(pkg/tools/builtin/filesystem/filesystem.go): fixing problems of remove_directory and create_directory of filesystem --- pkg/tools/builtin/filesystem/filesystem.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/pkg/tools/builtin/filesystem/filesystem.go b/pkg/tools/builtin/filesystem/filesystem.go index 5b8d1856d..3427e0cd7 100644 --- a/pkg/tools/builtin/filesystem/filesystem.go +++ b/pkg/tools/builtin/filesystem/filesystem.go @@ -1641,10 +1641,11 @@ func (t *ToolSet) handleCreateDirectory(ctx context.Context, args CreateDirector for _, path := range args.Paths { resolvedPath, err := t.resolveAndCheckPath(path) if err != nil { - return tools.ResultError(err.Error()), nil + return tools.ResultError(withCompletedWork(results, err.Error())), 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(withCompletedWork(results, + fmt.Sprintf("Error creating directory %s: %s", path, err))), nil } results = append(results, "Directory created successfully: "+path) } @@ -1652,6 +1653,17 @@ func (t *ToolSet) handleCreateDirectory(ctx context.Context, args CreateDirector return tools.ResultSuccess(strings.Join(results, "\n")), nil } +// withCompletedWork prefixes an error message with the operations that already +// succeeded. These loops stop at the first error but do not roll back, so +// reporting the error alone would read as a no-op and leave the caller unaware +// of changes already made on disk. +func withCompletedWork(completed []string, errMsg string) string { + if len(completed) == 0 { + return errMsg + } + return strings.Join(completed, "\n") + "\n" + errMsg +} + func (t *ToolSet) handleRemoveDirectory(ctx context.Context, args RemoveDirectoryArgs) (*tools.ToolCallResult, error) { annotateFilesystemSpan(ctx, "remove_directory", "") if span := trace.SpanFromContext(ctx); span.IsRecording() { @@ -1664,11 +1676,12 @@ func (t *ToolSet) handleRemoveDirectory(ctx context.Context, args RemoveDirector for _, path := range args.Paths { resolvedPath, err := t.resolveAndCheckPath(path) if err != nil { - return tools.ResultError(err.Error()), nil + return tools.ResultError(withCompletedWork(results, err.Error())), nil } if err := t.removeDir(resolvedPath); err != nil { - return tools.ResultError(fmt.Sprintf("Error removing directory %s: %s", path, err)), nil + return tools.ResultError(withCompletedWork(results, + fmt.Sprintf("Error removing directory %s: %s", path, err))), nil } results = append(results, "Directory removed successfully: "+path) } From c2d07fd1c2999d5b4cfb2f0cd90e06afa1f78606 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Thu, 6 Aug 2026 18:19:48 +0330 Subject: [PATCH 2/4] test(pkg/tools/builtin/filesystem/filesystem_test.go): adding edge case tests for filesystem tools bug --- .../builtin/filesystem/filesystem_test.go | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/pkg/tools/builtin/filesystem/filesystem_test.go b/pkg/tools/builtin/filesystem/filesystem_test.go index c09e76ef1..d707ea4ec 100644 --- a/pkg/tools/builtin/filesystem/filesystem_test.go +++ b/pkg/tools/builtin/filesystem/filesystem_test.go @@ -1374,6 +1374,76 @@ 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 only the error", 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") + }) +} + func createTestPNG(t *testing.T, w, h int) []byte { t.Helper() img := image.NewRGBA(image.Rect(0, 0, w, h)) From ec85b4e951107419ced92d01df1abb7ce7d4d952 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Fri, 7 Aug 2026 11:17:41 +0330 Subject: [PATCH 3/4] fix(filesystem): name the paths a directory batch never attempted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reporting the completed removals closed half the ambiguity. The other half remained: with nothing said about the tail, a caller still could not tell "not processed" from "processed but unreported" — which is the retry ambiguity this reporting exists to remove. The abort message now carries all three parts, each omitted when empty so a single-path failure keeps the bare error message it has always had: Directory removed successfully: a Directory removed successfully: b Error removing directory notempty: directory not empty Stopped before: d, e --- pkg/tools/builtin/filesystem/filesystem.go | 42 +++++++++++++--------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/pkg/tools/builtin/filesystem/filesystem.go b/pkg/tools/builtin/filesystem/filesystem.go index 3427e0cd7..3996e126a 100644 --- a/pkg/tools/builtin/filesystem/filesystem.go +++ b/pkg/tools/builtin/filesystem/filesystem.go @@ -1638,14 +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(withCompletedWork(results, 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(withCompletedWork(results, - 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) } @@ -1653,15 +1653,25 @@ func (t *ToolSet) handleCreateDirectory(ctx context.Context, args CreateDirector return tools.ResultSuccess(strings.Join(results, "\n")), nil } -// withCompletedWork prefixes an error message with the operations that already -// succeeded. These loops stop at the first error but do not roll back, so -// reporting the error alone would read as a no-op and leave the caller unaware -// of changes already made on disk. -func withCompletedWork(completed []string, errMsg string) string { - if len(completed) == 0 { - return errMsg +// 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(completed, "\n") + "\n" + errMsg + return strings.Join(parts, "\n") } func (t *ToolSet) handleRemoveDirectory(ctx context.Context, args RemoveDirectoryArgs) (*tools.ToolCallResult, error) { @@ -1673,15 +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(withCompletedWork(results, 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(withCompletedWork(results, - 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) } From 508ed160832acd11cb39236f0082c1eea8852321 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Fri, 7 Aug 2026 11:17:41 +0330 Subject: [PATCH 4/4] test(filesystem): cover the untouched tail and the allow-list abort Adds the untouched-tail assertions for both handlers, and pins that a single-path failure still produces a one-line message. Also covers the two resolveAndCheckPath early returns, which were previously unreachable in tests: the existing allow-list test rejects on the first path, so the completed-work list was always empty there and the reporting wiring never ran. --- .../builtin/filesystem/filesystem_test.go | 95 ++++++++++++++++++- 1 file changed, 94 insertions(+), 1 deletion(-) diff --git a/pkg/tools/builtin/filesystem/filesystem_test.go b/pkg/tools/builtin/filesystem/filesystem_test.go index d707ea4ec..5591445de 100644 --- a/pkg/tools/builtin/filesystem/filesystem_test.go +++ b/pkg/tools/builtin/filesystem/filesystem_test.go @@ -1426,7 +1426,7 @@ func TestFilesystemTool_DirectoryBatch_PartialFailureReportsCompletedWork(t *tes // 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 only the error", func(t *testing.T) { + t.Run("failure on the first path reports no completed work", func(t *testing.T) { t.Parallel() tmpDir := t.TempDir() tool := New(tmpDir) @@ -1442,6 +1442,99 @@ func TestFilesystemTool_DirectoryBatch_PartialFailureReportsCompletedWork(t *tes "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 {