Skip to content

Fix: let removeFolder reject with the original fs.rm error - #815

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/remove-folder-propagate-rm-error
Open

Fix: let removeFolder reject with the original fs.rm error#815
AmaadMartin wants to merge 3 commits into
mainfrom
fix/remove-folder-propagate-rm-error

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

No public issue.

  1. Or, if no issue exists, describe the change:

Problem: removeFolder in dev/src/utils/file_utils.ts logged an fs.rm
failure and then resolved. adk create uses it to clear an existing agent
folder before it recreates one, so a failed removal let the command continue and
write the new scaffold over the stale directory. The user saw a Failed to remove folder ... line scroll past above the success banner, and the command
exited 0. listFiles returns [] on a read failure, but its one-line doc
comment did not say so.

Solution: removeFolder now returns fs.rm(...) and rejects with the
original error object, matching loadFileData and saveToFile in the same
module. AgentFile.dispose() is the one caller that wants cleanup to be
best-effort, so it catches the failure at the call site and logs it through the
AdkLogger it already owns at debug level. listFiles keeps its fallback and
its JSDoc now says an empty result is indistinguishable from an unreadable
directory.

Two decisions a reviewer will want stated:

  • dispose() also drops its fsPromises.unlink(cleanupFilePath) call.
    load() assigns cleanupDirPath and cleanupFilePath together at
    agent_loader.ts:212-213, and it creates the compiled file inside that
    directory, so the recursive removeFolder already deletes the file. The
    inner if (this.cleanupDirPath) guard could never be false either. Removing
    both keeps the new catch scoped to removeFolder, the only call whose
    contract this PR changes, so no other error path is downgraded.
  • I rejected two alternatives. Adding a second export, removeFolderOrThrow,
    gives identical behaviour but costs an extra symbol whose only difference is
    its error handling. Calling fs.rm directly from cli_create.ts works, but
    that file routes every filesystem call through file_utils today. With
    removeFolder propagating, cli_create.ts needs no change at all.

Collision check: gh pr list --repo AmaadMartin/adk-js --state open --limit 400 plus gh pr diff --name-only on every adjacent PR. No open PR makes
removeFolder propagate. Two PRs touch lines next to mine and neither is a
duplicate: #708 routes the file_utils diagnostics through AdkLogger (it
rewrites the console.error inside the removeFolder catch that this PR
deletes), and #725 moves this.disposed = true above the cleanupFilePath
guard in dispose(). I branched from main rather than stacking, because
stacking on #708 would put a deletion of that PR's new removeFolder logs the failure and resolves test into this diff. Whichever lands second resolves a
one-hunk conflict. #704 makes the sibling createFolder propagate; it is a
different function and merges cleanly.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.
npx vitest run --project unit:dev dev/test/utils/file_utils_test.ts \
  dev/test/utils/agent_loader_test.ts dev/test/cli/cli_create_test.ts

62 passed, 1 failed. The failure is pre-existing and unrelated: should handle Vertex AI selection with gcloud defaults reads the host's real gcloud config,
so it fails on any machine with gcloud configured. It fails the same way on a
clean checkout of main.

Coverage of the changed source lines is 100% line and branch, measured with
--coverage.include on the two changed files. The residual gaps in
file_utils.ts are createFolder (untouched here) and createTempDir.

Proof each new test can fail. I ran every test against mutated source and
recorded the failure:

Test Mutation Failure
removeFolder removes the folder recursively drop {recursive: true} expected "spy" to be called with arguments: [ '/some/dir', { recursive: true } ]
removeFolder rejects with the original fs error instead of swallowing it restore the try/catch around fs.rm promise resolved "undefined" instead of rejecting
listFiles returns an empty array when the directory cannot be read change the catch to throw e promise rejected "Error: EACCES: permission denied, scandir" instead of resolving
dispose resolves when the temp folder cannot be removed delete the try/catch from dispose() promise rejected "Error: EACCES: permission denied, rm" instead of resolving
dispose resolves when the compiled artifact is already gone reintroduce the unlink call ahead of removeFolder expected "spy" to be called with arguments: [ Array(1) ] / Number of calls: 0
should surface a failed overwrite instead of writing into the stale folder wrap the removeFolder call in cli_create.ts in a try/catch promise resolved "undefined" instead of rejecting
should still finish when the post-create file listing comes back empty throw when files.length === 0 promise rejected "Error: No files were created" instead of resolving

Be aware of what the sixth test does and does not prove. cli_create_test.ts
mocks the whole file_utils module, so that test passes both before and after
the source change. It locks the contract that cli_create must not swallow a
removal failure. The regression test for the fix itself is removeFolder rejects with the original fs error instead of swallowing it.

Manual End-to-End (E2E) Tests:

I ran the real CLI against a real filesystem, as a non-root user.

npm run build -w dev
mkdir -p /tmp/adk-e2e/demo
echo 'stale marker' > /tmp/adk-e2e/demo/STALE.txt
echo 'export const stale = true;' > /tmp/adk-e2e/demo/agent.ts
chmod 500 /tmp/adk-e2e          # deny writes to the parent, so rm of demo/ fails
cd /tmp/adk-e2e && node <repo>/dev/dist/esm/cli_entrypoint.js create demo -y

Before this change: Failed to remove folder ... and a second Failed to create folder ... EEXIST on stderr, then the scaffold merged into the stale folder
(package.json and tsconfig.json appeared next to STALE.txt, and agent.ts
was overwritten).

After this change: one line, [ADK CLI] Error creating agent: EACCES: permission denied, rmdir '/tmp/adk-e2e/demo'. STALE.txt and the original agent.ts
survive untouched and no Created the following files in ... banner prints.

I also ran adk run agent.ts on a scaffolded agent and typed exit, which
exercises AgentFile.dispose() through the await using binding in
cli_run.ts. The count of /tmp/adk_agent_loader-* directories was the same
before and after, so the temp directory is still cleaned up.

npm run lint and npx prettier --check are clean on the touched files.
npm run ts:check reports the same 288 pre-existing errors before and after
this change, none of them in the files I touched.

CI note: the macOS leg failed once on two tests/integration/app_loader/app_loader_test.ts
cases timing out at 40s. Neither test calls dispose(). A re-run of the same
commit passed on all three operating systems, and #506, #545, #652 and #664
already track that flake, so I left it alone.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Amaad Martin added 3 commits August 8, 2026 09:51
removeFolder logged the fs.rm failure and resolved, so `adk create` on an
existing folder continued into createFolder and generateFiles and merged the
new scaffold over the stale directory. removeFolder now propagates, and
AgentFile.dispose() -- the one caller that wants cleanup to be best-effort --
swallows the failure at the call site and logs it at debug level.

listFiles keeps its empty-array fallback. Its JSDoc now states that an empty
result is indistinguishable from an unreadable directory.
Covers the propagated fs.rm error, the surviving {recursive: true} option, the
listFiles empty-array fallback, both dispose() cleanup failures (rm and
unlink), and that `adk create` stops on a failed overwrite.
load() assigns cleanupDirPath and cleanupFilePath together, and the compiled
file is created inside that directory, so the recursive removeFolder call
already deletes it. The separate unlink was redundant and the inner
cleanupDirPath guard could never be false. Removing the unlink also keeps the
catch scoped to removeFolder, the only call whose contract this branch changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant