docker / docker/docker-agent

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

Open Beginner friendly
#3,933 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

area/tools
Dominant language
Go
Stars
3.3k
Forks
462
Avg merge
1d 10h
Merged PRs (30d)
273

Description

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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in pkg/tools/builtin/filesystem/filesystem.go at handleCreateDirectory and remove_directory, then read TestFilesystemTool_RemoveDirectory_MultipleStopsOnError in filesystem_test.go. Preserve abort-on-first-error behavior while ensuring the result reports earlier paths that were already created or removed alongside the failure; add or update focused tests for both handlers.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
tooling
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
85/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.