Fix: report fixture cleanup failures in the agent_loader and script_js integration teardown - #662
Open
AmaadMartin wants to merge 11 commits into
Open
Fix: report fixture cleanup failures in the agent_loader and script_js integration teardown#662AmaadMartin wants to merge 11 commits into
AmaadMartin wants to merge 11 commits into
Conversation
#599) * fix(core): fall back to node:crypto so randomUUID cannot throw on Node randomUUID() consults only globalThis.crypto. That global was added in Node v17.4.0 and stayed behind --experimental-global-webcrypto until v19.0.0, so on a default Node 18 or earlier neither branch matches and the function throws, where it previously returned a weak UUID. That surfaces as a hard failure in createSession, AuthHandler.generateAuthUri and every A2A message id. node:crypto's randomUUID has existed since v14.17.0 and does not depend on the global, so using it as the last resort makes the throw unreachable on any Node this package could plausibly target. The two globalThis.crypto branches keep precedence, so browsers and Node 19+ are unaffected. randomUUID reaches the web bundle via index_web.ts -> common.ts -> events/event.js, so the node:crypto import is aliased to a browser shim, following the existing node:async_hooks precedent in build.js. The shim throws the message the function used to throw: it is reached only once both globalThis.crypto branches have been ruled out, which in a browser means the Web Crypto API is genuinely absent. Fixes #598. * docs(core): scope the alias note to the bundled web build The comment said the import is aliased in "the web build", but the alias in build.js applies only when platform is browser and bundle is set. The output package.json#browser points at, dist/web/index_web.js, comes from the non-bundle path and keeps the import verbatim, as it already does for node:async_hooks and node:path.
…file (#604) * fix(deploy): reject unsafe appName/project/region in generated Dockerfile createDockerFileContent interpolated options.appName, options.project, and options.region directly into the generated Dockerfile's ENV, COPY, and CMD instructions with no escaping. Since Dockerfile instructions are newline- delimited and the CMD line runs through /bin/sh at container start, a value containing a newline or shell metacharacters breaks out of its instruction: appName is derived by default from the basename of the agent path passed to `adk deploy cloud_run`/`adk deploy agent_engine` (only overridden by an explicit --app_name), so a maliciously-named agent directory or file — e.g. from a shared/cloned agent template a developer didn't author themselves — injects arbitrary Dockerfile instructions executed during `docker build` and/or arbitrary shell commands in the deployed container's CMD. Add assertSafeDockerfileToken, restricting these three values to a plain identifier (letters, digits, dot, dash, underscore) before they're ever embedded in the Dockerfile content, applied once in the shared createDockerFileContent so both deploy commands are covered. Confirmed by executing the function directly: a crafted appName previously produced a Dockerfile with a standalone injected RUN instruction; it's now rejected before any file is written. * fix(deploy): close remaining CMD-line injection via logLevel/allowOrigins/*ServiceUri logLevel, allowOrigins, sessionServiceUri and artifactServiceUri were still interpolated raw into the generated Dockerfile's CMD line, so a newline in any of them broke out of that instruction the same way appName did, and shell metacharacters reached /bin/sh at container start. These values are free-form (URIs, comma-separated lists) so they can't be restricted to the plain-identifier token used for appName/project/region; instead reject embedded newlines and single-quote-escape them for the shell. Also: error messages now JSON.stringify the rejected value instead of interpolating it raw, and the region rejection test now uses a newline payload since region only reaches the ENV line, not the shell-interpreted CMD line. * test(deploy): assert project survives the accept-case alongside appName Per review: the "should still accept dots/dashes/underscores" case only asserted appName made it into the Dockerfile, not project.
createAdkEventFromMetadata() restored `branch` straight from a remote
A2A peer's own response metadata (adk_branch), unlike `author` which is
always force-set by the caller. getContents() (content_processor_utils.ts)
uses an event's branch to keep sibling sub-agent conversation contexts
isolated from each other (a branch is visible in a given context only if
it is an ancestor of, or equal to, that context's current branch).
A malicious or compromised remote peer delegated a sub-task therefore
had two ways to break that isolation and inject its response into an
unrelated sibling sub-agent's LLM context:
- setting adk_branch to a shared ancestor branch (e.g. the parent
coordinator's branch instead of its own), or
- omitting adk_branch entirely, which the filter treats as "always
visible, in every branch".
This is the same class of bug fixed in #596 for actions.transferToAgent
(peer-controlled metadata able to corrupt local orchestrator state), on
a field that fix's allowlist didn't cover.
Fix: stop restoring `branch` in createAdkEventFromMetadata at all, and
thread it as an explicit parameter through toAdkEvent and its internal
per-event-type helpers instead, mirroring how `author` is already
force-set by the caller rather than trusted from peer metadata. The one
caller, A2ARemoteAgent.runAsyncImpl, now passes its own
InvocationContext.branch.
* fix(runner): persist onEventCallback output * fix(runner): preserve callback event identity
…Tools (adk-python parity) (#580) * Add canUseOutputSchemaWithTools predicate (adk-python parity) Ports can_use_output_schema_with_tools from adk-python. It reports whether a model can natively accept an output schema alongside tools, which is more reliable than the prompt-based set_model_response workaround. Composed from the existing getGoogleLlmVariant() and isGemini2OrAbove() helpers, so it recognises Gemini 2.0+ by numeric version only. Gemini Early Access Program names, which the Python predicate also matches, are therefore not recognised; that gap lives in the shared isGemini2OrAbove() predicate and is tracked separately. The helper is internal and intentionally not exported from the package barrels, matching how getGoogleLlmVariant() is treated. * Gate the set_model_response workaround on canUseOutputSchemaWithTools adk-js unconditionally fell back to the synthetic set_model_response tool whenever an agent had both an outputSchema and tools. adk-python applies that workaround only when the model cannot natively take a response schema alongside tools. On Vertex AI with Gemini 2.0+ the native path works and is more reliable. All three sites that key off "outputSchema and tools" move together: - LlmAgent.runOneStepAsync no longer appends the set_model_response tool. - InstructionsLlmRequestProcessor no longer appends the matching instruction. - BasicLlmRequestProcessor now DOES set the native response schema. The third site is load-bearing: gating only the first two would leave an affected request with neither mechanism, which is worse than the old behaviour. Tests assert both polarities so exactly one mechanism is always active. Only Vertex AI + Gemini 2.0+ + outputSchema + tools changes behaviour; every other combination is unchanged. * Simplify canUseOutputSchemaWithTools to a plain model-name predicate Addresses simplicity-audit findings: - Narrow the signature from `string | BaseLlm` to `string`. adk-python needs the union to isinstance-check LiteLlm; adk-js has no LiteLlm, so the BaseLlm branch only read `.model`, which the three callers now do themselves. This also matches isGemini2OrAbove(modelString: string). - Condense the JSDoc note on Early Access Program names to one sentence. - Drop the two duplicated call-site comments, keeping only the one on the inverse-polarity condition in the basic processor. - Collapse `!agent.tools || agent.tools.length === 0` to `!agent.tools?.length`. The two removed helper tests exercised the BaseLlm overload that no longer exists; the 11-row model-name table is untouched and the helper keeps 100% line and branch coverage. * Tighten output-schema JSDoc and align the two tool-presence checks Second simplicity-audit round: drop the caller-policy paragraph from the helper JSDoc (the surviving call-site comment already names the injection sites, and the "more reliable" rationale stays in the one-line summary), and use `agent.tools?.length` in the instructions processor so both processors spell the same test the same way. --------- Co-authored-by: Amaad Martin <amaadmartin@google.com>
… branch (#568) * fix(code-executors): detect pwsh as a PowerShell shell command The SHELL branch of UnsafeLocalCodeExecutor selected PowerShell spawn arguments with a substring test against `powershell`, so PowerShell 7+ (`pwsh`) was invoked without `-NoLogo -ExecutionPolicy Bypass -File` and its script was written with a `.sh` extension, which PowerShell refuses to run. The same substring test also misclassified unrelated commands whose path merely contains `powershell`. Detect PowerShell hosts on the executable name only (`powershell`/`pwsh`, case-insensitive, with or without `.exe`, either path separator) and use that for both the spawn arguments and the script extension. * refactor(code-executors): simplify PowerShell command detection Use path.win32.basename instead of a hand-rolled separator split (it splits on both separators on every platform), drop the two-element Set in favour of a direct comparison, and derive the spawn passthrough types in the test from the real spawn signature. * refactor(code-executors): tighten PowerShell detection and its tests Collapse the name check into a single anchored regex and drop assertions that restate behaviour already covered elsewhere in the file. * test(code-executors): use vitest autospy for the spawn recorder Replace the hand-written passthrough mock factory with vi.mock(..., {spy: true}), which wraps the real export without replacing its implementation, and pin -File to the argument before the script path. * test(code-executors): allow for PowerShell cold start in shell detection CI runners that ship PowerShell really launch it for these cases, and the first launch on a cold runner exceeded the default 5s test timeout. * fix(code-executors): keep shellCommandPath helper params optional Reverts an unrelated signature tightening so the diff stays scoped to the pwsh detection fix, per review feedback. The new PowerShell check guards against undefined the same way the cmd check on the next line does. * chore: park pwsh detection files at upstream state for merge Temporary: lets the upstream merge complete without conflicts so the merge auto-commit does not trip the pre-commit hook over unrelated files. The fix is restored in the next commit. * fix(code-executors): detect pwsh as a PowerShell shell command Restores the fix on top of the upstream merge. The SHELL branch selected PowerShell spawn arguments with a substring test against 'powershell', so PowerShell 7+ (pwsh) got neither the PowerShell flags nor a .ps1 script extension, and unrelated commands whose path merely contains the word were misclassified. Detection now matches the executable name only. Rebased onto the -NoProfile / /D change: the PowerShell branch reuses POWERSHELL_BASE_ARGS, and the tests reuse the existing spawn mock and EXPECTED_POWERSHELL_ARGS instead of the separate harness they used before. --------- Co-authored-by: Amaad Martin <amaadmartin@google.com>
…#527) * Fix: surface root-cause MCP session errors instead of swallowing them MCPSessionManager.createSession() awaited client.connect() with no error handling, so an IAP/gateway HTTP 403/401 (status on StreamableHTTPError.code, body baked into .message) reached callers as a generic/empty failure, and background transport errors were dropped entirely. Add an exported, cycle-safe formatError() helper that flattens AggregateError.errors and the Error.cause chain (joining leaves with " | ") and appends the HTTP status + a response body truncated to 1000 chars. Wire it into createSession() (rethrow "Failed to create MCP session: <detail>" with the original preserved via cause) and into transport.onerror for both the stdio and streamable-HTTP branches. Brings adk-js to parity with adk-python v2.4.0. * Refactor: drop redundant self-cause guard in formatError The `seen` visited-set already terminates cyclic cause/errors graphs, so the extra `cause !== err` check was dead weight. Coverage stays 100%. * Refactor: move MCP error formatting into mcp_error_utils.ts Review feedback: the error-formatting block did not belong in the session manager. Move the constants (MAX_RESPONSE_BODY_LENGTH, TRUNCATION_MARKER, UNKNOWN_ERROR, MIN/MAX_HTTP_STATUS) and helpers (asRecord, firstString, truncateBody, baseMessage, extractHttpDetails, formatErrorRecursive, formatError, logTransportError) verbatim into a co-located core/src/tools/mcp/mcp_error_utils.ts, matching the existing code_execution_utils.ts pattern. formatError stays the module's public surface (with logTransportError, which the session manager assigns to transport.onerror); every other helper remains module-private. mcp_session_manager.ts drops from 296 back to 145 lines and now holds session-manager logic only. The test file moves to mcp_error_utils_test.ts to mirror the source; it imports the module by relative path since these utils are deliberately not part of the @google/adk public API. Pure code move: no behavior change, no `any`, no eslint-disable. 52 MCP tests pass with 100% line+branch coverage of both files. * Refactor: move error formatting to shared utils as error_utils Follow-up review feedback: the helper is generic enough to be reused by other error handling, so it belongs in the shared utils directory under a name that does not read as MCP-only. - core/src/tools/mcp/mcp_error_utils.ts -> core/src/utils/error_utils.ts, alongside case_utils/file_utils/failover_utils; test moves to core/test/utils/error_utils_test.ts to match the sibling convention. - Drop the MCP framing: the module doc now describes generic error formatting, and StreamableHTTPError is mentioned only as one of several supported error shapes (`.status`, `.response`, numeric `.code`) rather than the purpose. - logTransportError stays in the MCP layer (now a private function in mcp_session_manager.ts) so the 'MCP transport error: ' label is not baked into a generic module; error_utils.ts exports only formatError. Still internal: nothing added to index.ts/common.ts. Pure move/rename with no behavior change, no any/eslint-disable. 52 targeted tests pass with 100% line+branch coverage of error_utils.ts and mcp_session_manager.ts. --------- Co-authored-by: Amaad Martin <amaadmartin@google.com>
…563) * fix(vertexai): fail fast when an Express Mode API key cannot be used VertexAiSessionService and VertexAiMemoryBankService resolved an Express Mode API key in their constructors and then built the Agent Engine client without it, producing a service that authenticated with ADC against projects/undefined/locations/undefined. The @google-cloud/vertexai Client constructor only accepts project, location and apiEndpoint, so the key can never be sent. Throw a shared, actionable error instead of silently dropping the credential. * test(vertexai): cover Express Mode fail-fast in both Agent Engine services Mock the Agent Engine Client in both suites so the default client path can be asserted without network or credentials, and make the pre-existing no-project/location test hermetic by stubbing the express-mode env vars. * refactor(vertexai): tighten Express Mode guard message and tests Address review feedback: shorten the error text and its doc comment, stop advertising expressModeApiKey in the fallback message now that it can never work, and table the three express-mode throw cases. * refactor(vertexai): drop dead env stubs and vendor signature from message Second review pass: GOOGLE_API_KEY is never read when GOOGLE_GENAI_USE_VERTEXAI is unset, the agentEngineId guard already runs first structurally, and enumerating the upstream constructor options in the error would go stale. * refactor(vertexai): fold duplicate guard test and trim redundant assertions * chore: park vertex_ai_session_service_test.ts at upstream content Temporarily takes upstream/main's copy of this file so the merge of upstream/main lands conflict-free; the express mode tests are restored in the following commit. * test(vertexai): restore express mode session tests after the upstream merge Re-applies the express mode suite on top of upstream's version of this file, which gained the ttl/expireTime and ApiError 404 tests. --------- Co-authored-by: Amaad Martin <amaadmartin@google.com>
…predictable parent) (#615) * fix(dev): create CLI temp directories atomically with mkdtemp getTempDir composed a temp path from a fixed prefix (adk_agent_loader, cloud_run_deploy_src, agent_engine_deploy_src) and a random leaf, then callers materialised it with a recursive mkdir. Recursive mkdir traverses an existing symlink at the intermediate component, so another local user who pre-creates that predictable parent owns every directory ADK writes into afterwards - the compiled agent bundle that is then import()ed, and the source tree handed to gcloud for deployment. createTempDir now creates the directory with a single mkdtemp call directly under the system temp root, so the whole path is unpredictable and, on POSIX, mode 0700. esbuild no longer sets allowOverwrite, and the deploy commands only pre-clean a temp folder the caller supplied. * test(dev): cover atomic temp-directory creation and the deploy default Adds file_utils_temp_dir_test.ts, which exercises createTempDir against the real filesystem (the sibling file_utils_test.ts mocks node:fs/promises module-wide, so real mkdtemp behaviour is unobservable there) and pins that a symlink planted at the predictable parent path cannot redirect the output. Also pins that the agent loader compiles without allowOverwrite, that both deploy commands create a private folder and skip the destructive pre-clean when the caller supplies none, and that the CLI leaves --temp_folder unset so no directory is created on an unrelated invocation. * fix(dev): build the gcloud args before creating the Cloud Run temp dir prepareGCloudArguments throws when the user passes an extra gcloud arg that conflicts with one ADK manages, and that error is deliberately propagated out of deployToCloudRun rather than caught. Creating the temp directory before that call left an empty 0700 directory behind on the user-error path, because the try/finally that removes it starts 30 lines later. Build the argument array first, while nothing exists on disk, then create the directory immediately before the try and push '--source <dir>' onto the array. This also removes the tempFolder parameter that had been threaded through prepareGCloudArguments, restoring its original signature. The redundant callerTempFolder alias is dropped in both deploy files in favour of reading options.tempFolder directly. --------- Co-authored-by: Amaad Martin <amaadmartin@google.com>
…tegration teardown
Both afterAll hooks discarded every removal error with .catch(() => {}), so a
failed node_modules teardown (EBUSY/EPERM on Windows is the realistic case) left
the fixture dirty with no diagnostic at all.
Wrap each hook's removals in a single try/catch that reports the failure with
console.error and still resolves, so a benign cleanup failure stays non-fatal
but stops being invisible. Convert the two fs.unlink(package-lock.json) calls to
fs.rm(..., {force: true}) so an absent lockfile remains a non-event rather than
becoming logged ENOENT noise now that errors are surfaced.
Per-test TEST_EXECUTION_TIMEOUT values and every it() body are untouched.
The previous revision collapsed every removal in each teardown hook behind a
single try/catch. That made each step conditional on all earlier ones, which
the per-call .catch(() => {}) it replaced did not: the realistic failure named
in the original change (EBUSY/EPERM on the Windows node_modules removal) is the
first removal in both hooks, so a cleanup failure would additionally strand
package-lock.json in agent_dirname_test.ts, and skip node_modules entirely in
agent_test.ts if an earlier generated-file removal failed.
Loop over the removal targets and attach a reporting handler to each call, so
every removal still runs and each failure names the target it belongs to.
recursive: true is a no-op on a file path, so one call form covers both files
and directories and the rationale comment stops being duplicated per removal.
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Link to an existing issue (if applicable):
N/A
Or, if no issue exists, describe the change:
Problem: The
afterAllhooks intests/integration/agent_loader/agent_dirname_test.tsandtests/integration/skills/script_js/agent_test.tsdiscard every cleanup error with.catch(() => {})— seven discarded removals across the two files. A failednode_modulesteardown (EBUSY/EPERMon thewindows-latestleg is the realistic case) therefore leaves the fixture directory dirty and produces no diagnostic at all. The suite reports success and the next run installs on top of a half-removed tree.Solution: Loop over each hook's removal targets and attach a reporting handler to each
fs.rmcall, so a failure is printed along with the specific target that failed. Teardown stays non-throwing — a benign WindowsEBUSYmust not turn a green suite red — but it stops being invisible.Design notes:
try/catch, so a failure in an early removal cannot skip the later ones. This matters precisely because the realistic failure —EBUSY/EPERMonnode_modules— is the first removal in both hooks, and the removals that follow it (a plainpackage-lock.jsonunlink; the generatedscript_jsoutputs) have no reason to fail just because anode_moduleshandle was locked. An earlier revision of this PR did collapse them behind a singletry; that was a regression against the.catch(() => {})behaviour it replaced, and it is fixed here. The three-way mutation matrix below measures it.recursive: trueis a no-op on a file path, sofs.rm(target, {recursive: true, force: true})covers both. That is what lets the targets collapse into a list and keeps the rationale comment stated once per hook instead of once per removal.force: truekeeps absence a non-event. This replaces the twofs.unlink(package-lock.json)calls. Once errors are surfaced,unlink'sENOENTon an absent lockfile would be pure logged noise, while a real I/O error still surfaces. It also matches howbuild_setup_test.tsremoves paths.tests/integration/test_case_utils.tsfor two call sites would add an exported symbol and enlarge the blast radius of a test-only fix for no reader benefit.console.error, notconsole.log.console.erroris the established idiom in this directory (tests/integration/build_setup/build_setup_test.ts,tests/integration/test_case_utils.ts).it()body, assertion, or per-test budget is touched.TEST_EXECUTION_TIMEOUTstays40000inagent_dirname_test.tsand60000inagent_test.ts, and bothit()calls still receive it. No test is added, removed, renamed, or skipped.The same silent
.catch(() => {})teardown survives intests/integration/app_loader/app_loader_test.tsandtests/integration/build_setup/build_setup_test.ts. Those are deliberately left alone to keep this diff to the two files the change is about.Scope deliberately reduced — please read. The task this came from also asked for an explicit
HOOK_TIMEOUT = 120000on these files'beforeAll/afterAll(their hook argument currently reuses the 40s/60s per-test constant, silently downgrading the 120000 ms projecthookTimeoutinvitest.config.ts). That half is already implemented in several live PRs on this fork and is deliberately omitted here rather than shipped as a competing fifth implementation:agent_dirname_test.tsscript_js/agent_test.tsbeforeAll)FIXTURE_HOOK_TIMEOUT_MS)INSTALL_TIMEOUT)No open PR on the fork fixes the swallowed-teardown half in these two files; #218 considered it and deliberately kept swallowing. That unclaimed half is what this PR ships. This PR branches from
mainrather than stacking on #405, because the change is independent of the hook argument (it rewrites the hook body, not its budget) and so survives whichever of the five timeout PRs is chosen. Expect a small rebase against whichever lands first — the hunks are adjacent.The hook-timeout defect is real and was reproduced while validating this change: on a cold fixture all three
agent_dirnamebeforeAllhooks failed withHook timed out in 40000ms, and the suite only went green once the budget was raised locally (cold install measured at ~70s under vitest, matching the mergedbuild_setup_test.tscomment). That evidence supports the PRs above; it is not re-fixed here.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
No new test file. The change adds no production code and no exported symbol;
vitest.config.ts'scoverage.includecovers onlycore/src,dev/srcandintegrations/src, so no coverage threshold is affected, and a unit test for a teardown hook's error handler would be test-of-test scaffolding. The two modified suites are the tests, and their existing assertions are the untouched regression signal. Correctness is demonstrated by the mutation matrices below instead.[x] All unit tests pass locally.
Both pass with the fixture directories clean beforehand, both emit zero cleanup diagnostics on the happy path, and both fixture directories are clean again afterwards — all three
agent_loaderfixtures and all fivescript_jsartifacts (ephemeral_entanglement.md,index.html,sketch.js,node_modules,package-lock.json) removed.Local environment caveat, stated plainly: on the corporate workstation used here
npm installinvoked throughexecAsyncinside a vitest hook takes ~70s (the same command takes 1.7s from a shell), so both suites hit the pre-existing 40s/60s hook budget described above and could only be run green after raising that budget locally. That local budget bump was scratch-only and is not in this diff —git diffshows the twoafterAllbodies and nothing else, and bothTEST_EXECUTION_TIMEOUTconstants still read40000and60000. This is exactly the flake the PRs listed above exist to fix.Also run on the pushed commit:
tsc --noEmitis already red on a cleanmainwith the identical count — 280 errors in 41 files with and without this change, and zero of them in either modified file. Pre-existing, unrelated, not run by CI, not addressed here.Proof the tests can fail — part 1: the diagnostic. Every row was run; the fix hunk was reverted or the exact line mutated, and the observed result is recorded.
agent_dirname_test.tsCleanup failed for .../__dirname/...: [Error: ENOENT ...]—Test Files 1 passedagent_dirname_test.tsmainbody; identical failing removal in the original.catch(() => {})styleCleanup failedoccurrences: 0.Test Files 1 passed. No diagnostic of any kindagent_dirname_test.tsfs.rm(..., {force: true})agent_dirname_test.tsfs.unlink(<absent path>)Cleanup failed ... Error: ENOENT: no such file or directory, unlink '.../ABSENT_LOCK.json'— pins theunlink→rm(force: true)conversion as load-bearingscript_js/agent_test.tsCleanup failed for .../skills/script_js/...: [Error: ENOENT ...]—Test Files 1 passedscript_js/agent_test.tsmainbody; identical failing removalTest Files 1 passedProof the tests can fail — part 2: independence. This pins the per-call handler specifically.
force: trueswallowsENOENT, so a genuine I/O failure was induced instead: alocked/directory at mode500containingvictim, which makesfs.rm('locked/victim', {recursive: true, force: true})fail withEACCESwhile siblings in the writable fixture root still remove cleanly.locked/victimwas then made the first removal target, and each run was inspected for whether the later targets survived.node_modulesafterpackage-lock.jsonaftermain— per-call.catch(() => {})try/catch(earlier revision of this PR)All three rows passed the suite (
Test Files 1 passed), confirming teardown stays non-fatal in every shape. Row 2 is the regression a reviewer caught in the earlier revision; row 3 is the fix. This PR is the only shape that both reports the failure and preservesmain's independence. All mutations were reverted; the working tree was diffed against a pristine copy afterwards and contains no residue (locked/victim,MUTANT_MISSING,ABSENT_LOCKand the scratch300000budget all grep to 0 in both files, and no stray fixture directories remain).Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
rm -rf tests/integration/skills/script_js/node_modules tests/integration/skills/script_js/package-lock.jsonforce: truemeans a missing path will not do it). On POSIX:mkdir -p tests/integration/skills/script_js/locked && echo x > tests/integration/skills/script_js/locked/victim && chmod 500 tests/integration/skills/script_js/locked, then add'locked/victim'as the first entry of the target list in that file'safterAll.npx vitest run --project integration tests/integration/skills/script_js/agent_test.tsCleanup failed for <path>/locked/victim: ... EACCES, still reports the suite as passed, and thatnode_modules,package-lock.jsonand the three generated files were all removed anyway.chmod 700thelockeddirectory, delete it, revert the target-list edit, and re-run: the suite passes with no diagnostic and the fixture directory is left clean.mainto see the same induced failure produce no output at all.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.