Skip to content

remove_directory and create_directory hide work they already did when a later path fails #3933

Description

@dwin-gharibi

Description

Both handlers loop over args.Paths, accumulate a success line per path, and return
ResultError on the first failure — discarding everything accumulated so far:

// pkg/tools/builtin/filesystem/filesystem.go:1663-1676 (remove_directory)
var results []string
for _, path := range args.Paths {
    resolvedPath, err := t.resolveAndCheckPath(path)
    if err != nil {
        return tools.ResultError(err.Error()), nil                 // results dropped
    }

    if err := t.removeDir(resolvedPath); err != nil {
        return tools.ResultError(fmt.Sprintf("Error removing directory %s: %s", path, err)), nil  // results dropped
    }
    results = append(results, "Directory removed successfully: "+path)
}

return tools.ResultSuccess(strings.Join(results, "\n")), nil

handleCreateDirectory (:1640-1652) has the identical shape.

Stopping at the first error is intentional and already pinned by
TestFilesystemTool_RemoveDirectory_MultipleStopsOnError (filesystem_test.go:1357). The defect
is not the abort — it is that the loop does not roll back, so earlier paths have already been
changed on disk while the reported result mentions only the failure.

Expected Behavior

The result names the paths that were actually removed (or created) alongside the error, so the
caller knows the filesystem state.

Actual Behavior

Only the error is returned. The completed removals are invisible.

Steps to Reproduce

<tmp>/empty-a/      (empty)
<tmp>/empty-b/      (empty)
<tmp>/not-empty/    (contains a file, so rmdir fails)
{"paths":["empty-a","empty-b","not-empty"]}
tool reported: "Error removing directory not-empty: directory not empty"

empty-a still exists? false      <- deleted
empty-b still exists? false      <- deleted
output mentions empty-a=false empty-b=false

Two directories were irreversibly removed and the result names neither.

The same happens for create_directory; a regular file in the path makes MkdirAll fail for any
path below it:

{"paths":["made1","made2","blocker/sub"]}

made1 and made2 exist afterwards, and the result mentions only the blocker/sub error.

Docker Agent version

No response

OS & terminal

No response

Model used

No response

Error output

Screenshots

No response

Additional context

Impact

The agent reads a bare failure and reasonably concludes the call was a no-op. The two natural
next moves — retry the same call, or tell the user nothing was removed — are both wrong.

For remove_directory the hidden work is destructive and not undoable, which also makes the
session transcript an inaccurate record of what happened to the filesystem. create_directory is
recoverable, but the same misreporting applies.

Additional context

  • This is purely a reporting fix; the abort-on-first-error semantics should stay, both because
    they are pinned by an existing test and because continuing to delete after an unexpected
    condition is the wrong instinct for a destructive batch.
  • Worth checking the other multi-path handlers (read_multiple_files, write_file) for the same
    shape.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/toolsFor features/issues/fixes related to the usage of built-in and MCP tools

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions