Fix: contain skill script output files in a declared output directory instead of process.cwd()

[AmaadMartin]

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):
   Not applicable — no public issue tracks this.
2. Or, if no issue exists, describe the change:
   Problem: materializeFiles() declared its destination as dir = process.cwd() (core/src/utils/file_utils.ts), and both skill script tools called it with no directory at all:

result.outputFiles = await materializeFiles(result.outputFiles);

So every file an executed skill script emitted was written into whichever directory the host Node process happened to be launched from — the repository root when running the dev server from a checkout. The filenames are not ours: they come back from the code executor, i.e. from the script, and for run_skill_inline_script from script content the model authored. That is how a stray root-level output.txt (contents hello) ended up tracked in this repository; this PR deletes it.

The rest of the codebase already does this correctly. UnsafeLocalCodeExecutor owns a scratch directory under os.tmpdir(), materializes into it with an explicit tempDir, runs the child with cwd set to it, and removes it in a finally. Only the two skill tools wrote to the ambient working directory.

A second, smaller defect lives in the same function. The containment guard was a raw string-prefix test:

if (!fullPath.startsWith(resolvedBaseDir)) { throw ... }

startsWith does not respect path boundaries, so a sibling directory whose name merely extends the base name passes it. With dir = /var/app/out, a file named ../outX/leak.txt resolves to /var/app/outX/leak.txt, which startsWith('/var/app/out') — the write escapes. This was latent while dir was the whole working directory; it becomes load-bearing the moment outputDir names a small, declared directory, so it is fixed here.

Solution: writing to the host filesystem is a side effect a library must be told where to perform.

• materializeFiles(files, dir) — dir is now required. Deleting the default is what stops a future caller reintroducing the footgun by omitting the argument: tsc rejects it. Internal change only — the symbol is exported from neither core/src/index.ts nor core/src/common.ts. Both remaining in-repo call sites are updated (unsafe_local_code_executor.ts already passed an explicit tempDir).
• New core/src/tools/skill/script_output_utils.ts with materializeScriptOutputs(result, outputDir?) and the SkillScriptResult response type. Co-located with the skill tools rather than in core/src/utils/, on the same justification as code_executors/code_execution_utils.ts: it is meaningless outside them. It returns a new object rather than mutating the executor's result in place.
• SkillToolset gains an outputDir?: string option, exposed as public readonly outputDir. Resolution to an absolute path happens at use, not at construction, so a host that changes its working directory is not pinned to a stale value. There is deliberately no getter that falls back to process.cwd() — that would reinstate the defect behind a different name.
• When outputDir is unset, each execution that produces output files gets a fresh directory from fs.mkdtemp(path.join(os.tmpdir(), 'adk-skill-outputs-')) and the tool response reports its absolute path, so the files stay findable. mkdtemp rather than a hand-built name: a fixed name in the world-writable temp directory can be pre-created as a symlink by another local user.
• Zero filesystem effects when the script produced no output files — no directory is created at all.
• The tracked root output.txt is deleted, and it is the smoking gun rather than incidental cleanup. It was committed by feat: skills: support script execution (#276) (commit 8d5cc0a) — the same PR that added these tools — its contents are the 5 bytes hello, and nothing in the repository reads it. Its name and content match the skill-script test fixture: it is precisely the artifact this defect produces, sitting at the repository root ever since.
• Containment is now path-segment aware. A module-level isContained(baseDir, fullPath) replaces startsWith at both guard sites (before, and again after the collision-rename loop). The thrown message is byte-identical, so the existing /Path traversal detected/ assertions keep passing unmodified.
• The materializeFiles JSDoc is rewritten. It previously said "Creates files with the given paths in the current working directory". It now describes the _2/_3 collision suffixing and the return value, and states plainly that the containment check is a lexical comparison of resolved paths and not a sandbox — it does not survive symlinks, hardlinks, bind mounts, or a TOCTOU race.

Behavioural change, called out deliberately. The default output location moves from process.cwd() to a per-execution temp directory. This is the point of the fix and it is observable. Nothing is lost: CodeExecutionResult.outputFiles already carries the file bytes inline on the tool response, and the new outputDir field reports the new location. Both skill script tools are @experimental. Anyone who relied on the old behaviour sets outputDir to process.cwd().

Intentional asymmetry: unlike the code executor's scratch directory, the output directory is not deleted — it holds the artifacts the script was asked to produce. Unconfigured runs therefore rely on OS temp cleanup; an application that wants a managed lifetime sets outputDir. This is stated in the outputDir JSDoc.

Cross-language parity: not applicable, deliberately. adk-python never materializes script outputs onto the host at all — skill_toolset.py's generated wrapper runs inside a tempfile.TemporaryDirectory() and chdirs back in a finally, and its UnsafeLocalCodeExecutor returns output_files=[]. Writing to process.cwd() is a JS-only defect, not a parity gap, so nothing was ported.

Deliberate scope limits

• Output files are not routed to the ArtifactService. Persisting script outputs as session artifacts changes what the model sees in the tool response; it is a different capability and belongs in its own change on top of this one. (Fix: persist skill script output files to the artifact service instead of the agent process cwd #410 and Feat: Save skill script output files as artifacts (opt-in saveOutputsAsArtifacts) #517 pursue that route.)
• UnsafeLocalCodeExecutor is untouched; its scratch-directory naming is a separate task (Test: pin that UnsafeLocalCodeExecutor removes its scratch directory on both the success and failure paths #355).
• materializeFiles still mutates file.name in the de-duplication loop. Untangling that is Fix: stop materializeFiles mutating its inputs, and bound and atomize collision resolution #409's scope, not this PR's.
• No .gitignore entry for output.txt — an ignore rule would hide a recurrence of exactly this bug.
• No new dependencies: node:fs/promises, node:os and node:path are already used in core/src.

Collision check

gh pr list --repo AmaadMartin/adk-js --state open --limit 1000, then gh pr diff --name-only on every plausibly adjacent PR. This area is heavily contested — the reviewer should treat these as mutually exclusive and pick one:

• Fix: materialize skill script output into a declared directory, never process.cwd() #298 fix/skill-script-output-dir adds the outputDir option but keeps the process.cwd() default, so it ships the knob without fixing the defect. Fix: require an explicit destination directory so skill script output never lands in process.cwd() #353 and Fix: require an explicit output directory and resolve the default in SkillToolset #437 are both stacked on it and are opposite designs: Fix: require an explicit destination directory so skill script output never lands in process.cwd() #353 writes nothing at all when outputDir is unset, Fix: require an explicit output directory and resolve the default in SkillToolset #437 resolves the default back to process.cwd(). This PR is a third answer — write to a declared directory, or to a reported per-execution temp directory — and is branched from main rather than stacked, because it reverses Fix: require an explicit destination directory so skill script output never lands in process.cwd() #353's central decision and could not stack on it honestly.
• Fix: test path containment, not string prefix, in materializeFiles #371 and Fix: enforce path containment by segment, not string prefix, in materializeFiles #523 are two duplicate implementations of the same segment-aware containment fix. This PR contains a third, because the spec makes the guard load-bearing for the new small outputDir and the change is four lines. If either of those lands first, drop that hunk here.
• Fix: persist skill script output files to the artifact service instead of the agent process cwd #410 / Feat: Save skill script output files as artifacts (opt-in saveOutputsAsArtifacts) #517 route skill script outputs to the artifact service instead. Different capability, explicitly out of scope above.
• Fix: stop skill-script integration tests leaking generated files into the repo working tree #305 / Fix: remove the stray tracked output.txt artifact and isolate the cwd-writing skill-script integration tests #438 stop the skill-script integration tests leaking files into the tree; this PR does the same as a consequence of the fix.
• Chore: remove stray output.txt from the repo root #271 / Chore: remove the accidentally committed output.txt from the repository root #429 / Fix: remove the stray tracked output.txt artifact and isolate the cwd-writing skill-script integration tests #438 / Chore: remove stray output.txt from the repository root #473 / Chore: remove stray output.txt from the repository root #539 each delete the stray root output.txt. This PR deletes it too, as the artifact the defect produced. Whichever lands first makes the rest a no-op.

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.
[x] All unit tests pass locally.

npx vitest run --project unit:core core/test/utils/file_utils_test.ts core/test/tools/skills/
  -> Test Files 9 passed (9) | Tests 99 passed (99)

npx vitest run --project integration \
  tests/integration/tools/run_skill_script_tool_test.ts \
  tests/integration/tools/run_skill_inline_script_tool_test.ts
  -> Test Files 2 passed (2) | Tests 19 passed | 4 skipped (23)

npm run build      -> OK
npm run lint       -> clean
npm run format:check -> "All matched files use Prettier code style!"
npx tsc --noEmit -p tsconfig.json -> no errors in any file this PR touches

npm run ts:check still reports pre-existing errors elsewhere in the repo (that gate is red on main; #370 / #487 own it). It reported one error in a file here — TS2352 on an as File cast that already existed on main and that this PR moves — so it is fixed properly (File + FileContentEncoding.UTF8) rather than carried.

New unit tests

• core/test/tools/skills/script_output_utils_test.ts (new): explicit directory, relative-directory resolution, the unconfigured temp-directory default, a distinct directory per call, the no-output-files early return, traversal rejection, and collision suffixing. The suite redirects TMPDIR/TEMP/TMP to a root it owns for the duration, so "nothing was created" is an exact observation rather than a race against a shared os.tmpdir().
• core/test/utils/file_utils_test.ts: two cases added, none of the five existing cases modified — a sibling directory whose name extends the base name is rejected, and a nested path whose segment merely starts with .. (..data/report.txt) is allowed.
• core/test/tools/skills/skill_toolset_test.ts: outputDir is exposed when configured and undefined when not. No process.cwd() fallback is asserted, because there must not be one.

Existing tests rewritten — 7 cases, enumerated, with reasons

No test case is deleted. Every one of these pinned the behaviour that is the bug, so there is no version of this fix that leaves them passing untouched; each replacement is strictly stronger, asserting the file is absent from process.cwd().

#	Test case	Why the old assertion was wrong
1	core/test/tools/skills/run_skill_script_tool_test.ts — calls materializeFiles with output files from executor	Asserted materializeFiles was called. The tool no longer calls it, so the assertion pinned an internal call rather than a behaviour. Now materializes output files with no directory when none is configured, asserting on materializeScriptOutputs.
2	core/test/tools/skills/run_skill_inline_script_tool_test.ts — same case, same rename	As above.
3	tests/integration/tools/run_skill_script_tool_test.ts — creates files in process.cwd returned from execution	The name says it: it encoded process.cwd() as the output location, i.e. asserted the defect. Now writes output files into the configured outputDir.
4	tests/integration/tools/run_skill_inline_script_tool_test.ts — same case, same rename	As above.
5	tests/integration/tools/run_skill_script_tool_test.ts — handles file collisions by appending a numeric suffix	Pre-created the colliding file in the repository root and deleted it afterwards, so an early failure left it behind. Same _2 assertions, now inside a per-test mkdtemp directory.
6	tests/integration/tools/run_skill_inline_script_tool_test.ts — same case	As above.
7	tests/integration/skills/script_js/agent_test.ts — should run agent with skills successfully	Asserted the generated art files appeared in the agent's project directory. Now passes a temp directory via ADK_SKILL_OUTPUT_DIR and additionally asserts all three are absent from the project directory.

Two further disclosures on that set:

• run_skill_script_tool_test.ts also restructures the surviving assertions behind new executorReturning() and runTool() helpers. That is more churn than the behaviour change strictly forces — it is a readability refactor sharing setup across the three cases in that block, kept deliberately, and flagged here so the refactor is distinguishable from the behaviour change in review.
• Case 7 does not pass in my sandbox, and fails identically on unmodified main: the spawned agent child process never gets past npm run start here. I could not run it green either before or after, so that one rewrite is verified by reading only.

Proof the tests can fail. Each new test was run against mutated source; recorded failures:

Mutation	Test that fails	Failure
isContained() → return fullPath.startsWith(baseDir)	rejects a sibling directory whose name extends the base directory name	AssertionError: promise resolved "[ { name: '../outX/leak.txt', …(3) } ]" instead of rejecting
segment check → !rel.startsWith('..')	allows a nested path whose segment merely starts with '..'	Error: Path traversal detected: ..data/report.txt resolves outside of …/file_utils_test_wUCAGZ
unconfigured default → process.cwd()	writes to a fresh temp directory when no output directory is configured, creates a distinct directory per call, and integration does not write output files into the working directory by default	AssertionError: expected '<repo root>' to be '<tmpdir>'
delete the outputFiles.length === 0 early return	returns the result unchanged and creates nothing when there are no output files	AssertionError: expected { stdout: 'out', stderr: 'err', …(2) } to be { stdout: 'out', stderr: 'err', …(1) }
tools hardcode undefined instead of this.toolset.outputDir	passes the toolset outputDir through in both tool suites	AssertionError: expected "spy" to be called with arguments: [ …(2) ]

Coverage. script_output_utils.ts (the new file) is at 100% statements / branches / functions / lines. file_utils.ts measures 94.93% lines and 80% branches; the shortfall is lines 83–86, the throw body of the second, post-collision containment guard. That guard is defense in depth and is unreachable through the public surface, because the rename loop only appends a suffix to the basename inside the same directory — a path contained before the loop is still contained after. It is kept deliberately (the invariant is "checked both before and after the rename loop") rather than deleted to make the number go up.

resolves a relative output directory against the working directory moves the process working directory to the test's own temp root for the duration (restored in a finally) rather than deriving a relative path from the real one — on Windows the two sit on different drives, where no relative path between them exists, and the first revision of this PR failed the Windows leg for exactly that reason. Vitest's default forks pool gives each test file its own process, so the chdir cannot leak into another suite.

One test-hygiene note worth flagging: the integration suite registers a tool-chosen outputDir for cleanup only after asserting it is directly under os.tmpdir() (trackToolOutputDir). Without that ordering, running the process.cwd() mutation above makes afterEach recursively delete the repository — which is exactly what happened to me once while producing the table above.

CI note — a flaky Windows leg, disclosed with numbers. run-tests is green on ubuntu-latest, macos-latest and windows-latest. Getting Windows green took several attempts: across this branch it failed 5 of 9 runs, always on the same test — core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdout, Test timed out in 5000ms. Evidence that it is not this change:

• That file is not in this diff, and the executor it exercises is untouched.
• Two of the failing runs were triggered by commits that change no test behaviour at all — one edits only JSDoc text, the other only git rm --cacheds a stray markdown file — on top of a commit whose Windows leg had already passed. The tree those runs executed is behaviourally identical to one that passed.
• The fork's own main branch currently has a red Windows leg too, on a different flake (tests/integration/adk_web/webui_test.ts, "CLI exited prematurely with code 1"). Windows is unstable here at baseline.

The underlying cause is a real-subprocess test running against vitest's 5s default on a cold PowerShell; #498 is the open fix for exactly that. Not addressed here, to keep this diff to the change it is for — but a reviewer re-running this PR should expect to need a retry on Windows.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

1. From a clean checkout, build: npm run build.
2. Run an agent whose SkillToolset has an UnsafeLocalCodeExecutor and a skill script that writes a file — tests/integration/skills/script_js/agent.ts is a ready-made fixture — with the dev server started from the repository root.
3. Prompt the agent to run the script. Before this change, the script's output file appears in the repository root and shows up in git status. After it, git status is clean and the tool response carries outputDir pointing at a adk-skill-outputs-* directory under the OS temp directory that contains the file.
4. Repeat with outputDir set on the toolset (the fixture reads ADK_SKILL_OUTPUT_DIR) and confirm the file lands there instead.

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.