fix(filesystem): report completed removals/creations when a directory batch aborts

[dwin-gharibi]

remove_directory and create_directory accumulate a success line per path and then throw the
whole list away on the first error. The loops stop but do not roll back, so earlier paths are
already changed on disk while the result mentions only the failure.

Closes #3933.

Before

paths: ["empty-a", "empty-b", "not-empty"]     (not-empty contains a file)

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 irreversibly removed, named nowhere in the result. The agent reads a bare failure
and reasonably concludes nothing happened — so it retries the same call, or tells the user nothing
was removed. Both are wrong.

After

tool reported:
  Directory removed successfully: empty-a
  Directory removed successfully: empty-b
  Error removing directory not-empty: directory not empty

Scope: reporting only, semantics unchanged

I deliberately did not switch these loops to continue-past-errors, for two reasons:

1. TestFilesystemTool_RemoveDirectory_MultipleStopsOnError (filesystem_test.go:1357) already
   pins abort-on-first-error, including the comment
   // dir3 should still exist since processing stopped at nonexistent. It is intended behaviour,
   not an accident.
2. For a destructive batch, continuing to delete after an unexpected condition — including an
   allow-list rejection from resolveAndCheckPath — is the wrong instinct.

That existing test is also where the bug shows through: it asserts dir1 was deleted but never
that the output says so. So the deletion was documented; the reporting gap was not.

The fix

One helper, applied at all four early returns (both handlers × resolve-failure and
operation-failure):

// 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
}

The len(completed) == 0 branch matters: without it, a failure on the first path would gain a
blank leading line and change every existing error-message assertion. There's a test for that
specifically.

Note it deliberately does not use append(completed, errMsg) — that can write into the caller's
backing array. Harmless here since we return immediately, but building the string directly avoids
the aliasing question entirely.

Tests

TestFilesystemTool_DirectoryBatch_PartialFailureReportsCompletedWork, three subtests:

Subtest	Role
remove_directory names the directories it already removed	the regression
create_directory names the directories it already created	same defect, second handler
failure on the first path reports only the error	control — no blank leading line, and processing still stops at the first error

The two regression subtests assert the filesystem state with require before checking the
output, so a failure reads unambiguously as "the work happened but wasn't reported" rather than
"the work didn't happen".

Written test-first. Both regression subtests failed on unpatched code with
"Error removing directory nonexistent: no such file or directory" does not contain "Directory removed successfully: dir1". The control passed before the change as well as after — it exists to
catch the blank-line regression the naive version of this fix introduces.

create_directory's failure is induced with a regular file where a parent directory is expected,
making MkdirAll fail for any path below it — the same technique the cache suite uses.

Verification

Toolchain go1.26.5, darwin/arm64.

Check	Result
go test ./pkg/tools/builtin/filesystem/...	ok
go test -race -count=1 ./pkg/tools/builtin/filesystem/	ok
go build ./...	clean
go vet ./pkg/tools/builtin/filesystem/	clean
gofmt -l pkg/tools/builtin/filesystem/	no output
go test ./... (full suite, .env.test loaded)	only pkg/teamloader fails — pre-existing

[dwin-gharibi]

@Sayt-0

[aheritier]

Verified the fix and the tests locally; the change is correct and the tests are real. Not approving yet only because CI has not run on c2d07fd — the workflow runs sit at conclusion: action_required (check-runs total_count = 0, combined status pending), so a maintainer needs to release the workflows before this can be approved on verified-green CI.

What I verified

• go test ./pkg/tools/builtin/filesystem/..., go test -race on the new test, go build ./..., go vet, gofmt -l, and golangci-lint run pkg/tools/builtin/filesystem/... — all clean (go1.26.5, darwin/arm64).
• The tests genuinely exercise the new path. Reverting only filesystem.go to base a2746bb64 while keeping the new test file fails exactly as described:

  filesystem_test.go:1402: "Error removing directory nonexistent: no such file or directory"
      does not contain "Directory removed successfully: dir1"
  filesystem_test.go:1423: "Error creating directory blocker/sub: mkdir .../blocker: not a directory"
      does not contain "Directory created successfully: made1"
  

• No existing assertion breaks. Every prior test on these handlers asserts with Contains, not Equal (filesystem_test.go:1310, :1338, :1352, :1368; filesystem_paths_test.go:355-370), and the len(completed) == 0 guard keeps single-path error messages byte-identical.
• Contract impact is safe. tools.ResultError still sets IsError: true (pkg/tools/tools.go:121), no Meta is emitted for these two tools, and nothing in the repo parses their output — the only references to these strings are filesystem.go and its test. Docs describe purpose only, not output format (docs/tools/filesystem/index.md:50-51), so no doc update is owed.
• Cross-platform. CI runs task test on windows-latest too; the file-as-a-parent technique used in the create_directory subtest makes MkdirAll fail on Windows as well, and the existing precedent (pkg/tools/mcp/keyringstore/tokenstore_test.go:540-552) is not Windows-skipped.

Agreeing with the scope call in the description: keeping abort-on-first-error for a destructive batch is right, and withCompletedWork building the string instead of append(completed, errMsg) avoids the caller-aliasing question cleanly.

Findings

[should-fix] The output still says nothing about the paths that were skipped.
The same reasoning that motivates this PR — the agent cannot act on what the result does not say — applies to the tail of the batch. Five paths, failure at #3:

Directory removed successfully: a
Directory removed successfully: b
Error removing directory notempty: directory not empty

d and e are untouched on disk but appear nowhere, so a model still cannot distinguish "not processed" from "processed and unreported" — which is the retry ambiguity this PR sets out to remove. One extra line at the abort point would close it, e.g.:

if err := t.removeDir(resolvedPath); err != nil {
    return tools.ResultError(withCompletedWork(results,
        fmt.Sprintf("Error removing directory %s: %s\nStopped before: %s",
            path, err, strings.Join(args.Paths[i+1:], ", ")))), nil
}

Fine as a follow-up if you'd rather keep this PR minimal, but it's the other half of the same reporting gap.

[optional] Two of the four changed call sites are untested.
The resolveAndCheckPath early returns (filesystem.go:1643 and :1679) are only covered by TestFilesystemTool_HandlersUseAllowList, which rejects on the first path — so completed is always empty there and the new wiring is never exercised. I probed it manually and the behaviour is correct:

remove_directory IsError=true
  Directory removed successfully: gone
  path ".../002/keep" is outside the allowed directories (.)
create_directory IsError=true
  Directory created successfully: made
  path ".../002/nope" is outside the allowed directories (.)

A fourth subtest built on newTestToolSet(t, wd, WithAllowList([]string{"."})) with paths {"gone", <outside>} would pin it. This is the allow-list rejection case the description calls out as a reason not to continue past errors, so it's worth locking down.

[optional] Commit-message scope is a file path. fix(pkg/tools/builtin/filesystem/filesystem.go): — since this repo merges PRs with merge commits, these messages stay in main's history. fix(filesystem): / test(filesystem):, matching the PR title, reads better.

[optional, out of scope] The same defect class exists in the LSP tool. applyWorkspaceEdit writes files in a loop and discards modifiedFiles on the first failure (pkg/tools/builtin/lsp/lsp.go:1355-1362 and :1370-1379), so a partially applied rename reports only the error. Worth a separate issue rather than growing this PR.

[docker-agent]

Assessment: 🟢 APPROVE

The withCompletedWork helper is correct: the len(completed) == 0 guard properly handles the nil-slice case (since var results []string initialises to nil, and len(nil) == 0 is true in Go), the string construction is sound, and all four early-return sites in both handlers are patched. Tests assert filesystem state before output content, giving unambiguous failure messages. No bugs were found in the changed code.

Drafter summary: The PR introduces a small, correct helper. The len(completed) == 0 guard works for both nil and empty slices. No aliasing issues exist. The test subtests correctly assert both filesystem state and output content. One low-confidence observation (not posted inline): the create_directory failure test uses filepath.Join to construct a tool-argument path that could exercise a different code path on Windows — this is test fragility at low severity and does not affect correctness.

[dwin-gharibi]

Done. @aheritier