Fix: require an explicit destination directory so skill script output never lands in 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(), 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 whatever directory the host Node process happened to be launched from — for a contributor running the dev server from a clone of this repo, the repository root. Four things make that worse than a cosmetic annoyance:

• The names are script-controlled. File.name comes back from the code executor, i.e. from output the model or the skill author produced. The only guard is a lexical path-containment check that keeps writes under the base directory — and the base directory here is the caller's cwd, which is precisely the sensitive location.
• It accumulates without bound. The de-duplication loop never overwrites; it appends _2, _3, … So repeated invocations pile up files with no cap, no rotation and no cleanup, inside a long-running agent process.
• Nothing reads the files back. The tool result already carries each file's content, contentEncoding and mimeType inline. The disk write serves no in-process consumer; its only observable effect is the pollution.
• It already happened here. A stray output.txt (content hello, 5 bytes) was tracked at the repository root. Its name and content match the fixture in core/test/tools/skills/run_skill_script_tool_test.ts, and nothing in the repository reads it. This PR deletes it.

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

• materializeFiles(files, dir) — the = process.cwd() default is deleted and dir becomes required. This is what stops a future caller reintroducing the footgun by omitting the argument; tsc rejects it.
• run_skill_script / run_skill_inline_script write only when the embedding application declared SkillToolset's outputDir. When it is unset they touch the host filesystem not at all — the bytes are still returned inline on the tool response, which is the whole contract.
• The write stays inside the tools' existing try, so a rejected traversal is converted by the existing catch into the normal EXECUTION_ERROR response rather than escaping the tool.
• core/src/code_executors/unsafe_local_code_executor.ts is untouched: it already passes an explicit tempDir.
• The materializeFiles doc comment is corrected while it is being rewritten anyway. It previously said names resolving outside dir are rejected; the check is fullPath.startsWith(resolvedBaseDir), a raw string-prefix comparison, so a sibling sharing the prefix (dir of /a/out, name ../out_leak.txt → /a/out_leak.txt) passes it. The comment now calls the check lexical and dir a tidiness boundary rather than a security one; the worked example above is the long form, deliberately kept out of the source comment. No logic changed — fixing the guard is a documented non-goal here and Fix: test path containment, not string prefix, in materializeFiles #371/Fix: enforce path containment by segment, not string prefix, in materializeFiles #523 already own it — but the new outputDir option is exactly what makes that boundary user-visible, so the docstring should not advertise a guarantee the code does not provide.

This matches adk-python, where skill_toolset.py's generated wrapper materializes only inside the code executor's own tempfile.TemporaryDirectory() and chdirs back in a finally — there is no host-side output-file materialization anywhere in that file.

This is an intended behavioural change and should land in release notes: skill script output files no longer appear in the agent process's working directory. Anyone who relied on that should pass outputDir to SkillToolset. It is not a public API break — materializeFiles is internal (exported from neither core/src/index.ts nor core/src/common.ts), and outputDir is an additive optional field.

Deliberate scope limits

Per the approved design, the following are out of scope and deliberately not done here:

• Saving output files to the ADK artifact service via toolContext.saveArtifact() (the pattern in code_execution_request_processor.ts). See the collision notes — two open PRs already pursue that.
• Making materializeFiles pure with respect to its input File objects. It still mutates file.name in the de-duplication loop, on purpose: unsafe_local_code_executor.ts decides which files in its temp dir are outputs by comparing against those same input File.name values, so untangling it is a separate change.
• Any change to the path-traversal guard itself, to the de-duplication scheme, or to .gitignore. An ignore entry for output.txt would hide a recurrence of exactly this bug, so none was added.

Collision check

gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 plus gh pr diff --name-only on every plausibly adjacent PR. Findings:

• Fix: materialize skill script output into a declared directory, never process.cwd() #298 fix/skill-script-output-dir — this PR is stacked on it, and it must merge first. Fix: materialize skill script output into a declared directory, never process.cwd() #298 adds the outputDir option but deliberately keeps the process.cwd() default, so it ships the knob without fixing the defect. This PR is based on that branch rather than main and supplies the behavioural half: required dir, and no host write when outputDir is unset.
• Fix: require an explicit output directory and resolve the default in SkillToolset #437 fix/skill-tools-declared-output-dir — a direct conflict, also stacked on Fix: materialize skill script output into a declared directory, never process.cwd() #298. It makes outputDir private behind a getter that resolves the default to process.cwd() ("Defaults to the agent process's current working directory"). That is the opposite design: it keeps writing to the launch directory. These two cannot both land; this PR is the one that stops the writes.
• 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 route skill script outputs to the artifact service instead. Complementary rather than competing — they replace the destination, this PR removes the implicit one — but whoever lands second should reconcile.
• 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 standalone. This PR deletes it too, because it is the direct artifact of the defect being fixed and the approved design lists it. Identical deletions merge cleanly; if one of those lands first, drop this one-line hunk.
• Fix: test path containment, not string prefix, in materializeFiles #371 and Fix: enforce path containment by segment, not string prefix, in materializeFiles #523 rework the containment check inside materializeFiles (segment-wise instead of string-prefix). Same function, disjoint lines — this PR keeps both guards verbatim and changes only the signature and the doc comment.
• Fix: stop skill-script integration tests leaking generated files into the repo working tree #305 mitigates the same leak at the test level only. Not duplicated 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.
[x] All unit tests pass locally.

New — core/test/tools/skills/skill_script_output_dir_test.ts (6 cases). This file deliberately does not vi.mock file_utils. The two existing skill tool test files mock that module at module scope, so neither of them can observe what actually reaches the disk; this one exercises the real filesystem out of a per-test fs.mkdtemp and tears it down in afterEach. It builds a real InvocationContext (new LlmAgent + createSession + new PluginManager) rather than casting a literal, so it adds no as unknown as. Cases:

1. run_skill_script writes output files into the configured outputDir.
2. run_skill_script writes nothing when outputDir is unset — asserted against a before/after Set snapshot of fs.readdir(process.cwd()), plus an empty temp dir, plus the executor's array returned unchanged. This is the regression test for the reported bug.
3. and 4. the same two for run_skill_inline_script.
4. Colliding names are suffixed inside outputDir (output.txt, output_2.txt) and the returned names match the on-disk ones.
5. Error path: an output name escaping the directory (../escape.txt, against a nested outputDir so the escape target stays inside the test's own temp tree) surfaces as {error: /Path traversal detected/, errorCode: 'EXECUTION_ERROR'}, and no file is created at the escape target.

Modified — existing tests that encoded the defective behaviour. Declaring these explicitly, since each destroys a prior assertion:

• core/test/utils/file_utils_test.ts: removed 'should default the base directory to the working directory of each call'. It asserted, via a process.cwd spy, that omitting dir follows the cwd at call time — i.e. it pinned the exact default this PR deletes, and it cannot compile against the required-dir signature. Its replacement signal is 'should require an explicit destination directory', which asserts materializeFiles.length === 2 (arity drops to 1 the moment a default is reintroduced), plus npm run ts:check.
• core/test/tools/skills/run_skill_script_tool_test.ts and run_skill_inline_script_tool_test.ts: the case asserting toHaveBeenCalledWith([testFile], undefined) became 'does not write output files when no output directory is configured', asserting expect(materializeFiles).not.toHaveBeenCalled() and that result.outputFiles is the executor's array unchanged. The old assertion pinned "call it anyway and let it pick cwd", which is the bug.
• tests/integration/tools/run_skill_script_tool_test.ts and run_skill_inline_script_tool_test.ts: 'creates files in process.cwd returned from execution' asserted the file was created in process.cwd() and then unlinked it. Split into 'returns output files inline without writing to disk when no output directory is configured' (asserts cwd stays clean and the bytes are on the result) and 'creates files in the configured output directory' (temp dir via mkdtemp, cleaned in finally). The collision case now pre-creates its fixture in the temp dir instead of the repo root.
• tests/integration/skills/script_js/ — the one a first review round caught me missing. This suite spawns a real agent (npm run start) and asserted that the skill script's three output files (ephemeral_entanglement.md, sketch.js, index.html) appeared in the agent's own project directory, cleaning them up in afterAll. That only ever worked because of the process.cwd() default — UnsafeLocalCodeExecutor runs the script in an os.tmpdir() directory and fs.rms it in a finally — so with the default gone the test failed with ENOENT. agent.ts now takes its outputDir from ADK_SKILL_OUTPUT_DIR; agent_test.ts creates a per-run fs.mkdtemp directory, passes it through the spawn env, reads the files back from there, and additionally asserts the three names are absent from the project directory. The case used to pin the leak; it now pins its absence, and it is the end-to-end demonstration of the migration path this PR gives users.
• core/test/tools/skills/run_skill_script_tool_test.ts keeps its bare vi.fn() module mock. An earlier revision of this branch gave it a mockImplementation(files => files) for symmetry with the inline-tool file; no test here needs materializeFiles to return its input, so that was shared fixture state buying nothing and it was reverted.
• No test was skipped, disabled, .only'd or weakened. The five pre-existing file_utils_test cases already passed tempDir explicitly and are untouched; that they still compile against the required-dir signature is itself a useful signal.

Added — core/test/utils/file_utils_test.ts: 'should create the target directory when it does not exist', passing path.join(tempDir, 'nested', 'out').

Proof the tests can fail (mutation testing)

Every new test was run against mutated source and observed to FAIL. Exact mutations and messages:

M1 — restore the defect wholesale: put dir = process.cwd() back in file_utils.ts and revert both tools to materializeFiles(result.outputFiles). 10 tests failed, and the run reproduced the bug end to end by leaking output.txt … output_6.txt into the repository root:

× run_skill_script writes nothing when no output directory is configured
  → expected Set{ '.audit_comments.md', …(36) } to deeply equal Set{ '.audit_comments.md', …(35) }
× run_skill_inline_script writes nothing when no output directory is configured
  → expected Set{ '.audit_comments.md', …(38) } to deeply equal Set{ '.audit_comments.md', …(37) }
× run_skill_script writes output files into the configured directory
  → ENOENT: no such file or directory, open '…/skill_script_output_54n7e9/output.txt'
× run_skill_inline_script writes output files into the configured directory
  → expected 'output_3.txt' to be 'output.txt'
× suffixes colliding names inside the configured directory
  → expected [ 'output_5.txt', 'output_6.txt' ] to deeply equal [ 'output.txt', 'output_2.txt' ]
× RunSkillScriptTool > does not write output files when no output directory is configured
  → expected "spy" to not be called at all, but actually been called 1 times
× RunSkillInlineScriptTool > does not write output files when no output directory is configured
  → expected "spy" to not be called at all, but actually been called 1 times
× RunSkillScriptTool > materializes output files into the configured output directory
  → expected "spy" to be called with arguments: [ …(2) ]
× RunSkillInlineScriptTool > materializes output files into the configured output directory
  → expected "spy" to be called with arguments: [ …(2) ]
× file_utils > should require an explicit destination directory
  → expected 1 to be 2

M2 — keep the guard but re-add a fallback: const outputDir = this.toolset.outputDir ?? process.cwd(); if (true) {. 4 tests failed — precisely the four "writes nothing" tests, which is the property this PR is about:

× RunSkillScriptTool > does not write output files when no output directory is configured
  → expected "spy" to not be called at all, but actually been called 1 times
× RunSkillInlineScriptTool > does not write output files when no output directory is configured
  → expected "spy" to not be called at all, but actually been called 1 times
× run_skill_script writes nothing when no output directory is configured
  → expected Set{ …(35) } to deeply equal Set{ …(34) }
× run_skill_inline_script writes nothing when no output directory is configured
  → expected Set{ …(36) } to deeply equal Set{ …(35) }

M3 — traversal guard. Case 6 is not killed by M1 or M2, so it was mutated directly. Worth flagging as a finding: materializeFiles has two containment checks (before de-duplication and after), and disabling either one alone leaves all tests green — the surviving one still catches the escape:

• if (!fullPath.startsWith(resolvedBaseDir)) → if (false): 6/6 pass.
• if (!finalPath.startsWith(resolvedBaseDir)) → if (false): 13/13 pass.
• Both → if (false): 3 tests fail, including the new case 6 —
  → expected { stdout: '', stderr: '', …(1) } to deeply equal { error: StringMatching{…}, …(1) }
  alongside the two pre-existing file_utils traversal tests.

So for a name that escapes before de-duplication the second check is redundant. Changing it is out of scope here (and #371/#523 already rework that guard), but the redundancy is real and a reviewer of those PRs should know a single-guard mutant is currently undetectable.

M5 — the script_js agent suite against the restored default: the same source mutation as M1, run against the spawn-based agent test. It fails, and the mutated run leaks the three files straight back into the repository working tree — the defect reproduced through a real agent process rather than a unit fixture:

× Agent with skills that generates JS script and runs it locally > should run agent with skills successfully
  → ENOENT: no such file or directory, open '…/adk-script-js-out-LXMCzw/ephemeral_entanglement.md'

$ git status --porcelain tests/integration/skills/script_js/
?? tests/integration/skills/script_js/ephemeral_entanglement.md
?? tests/integration/skills/script_js/index.html
?? tests/integration/skills/script_js/sketch.js

M4 — fs.mkdir(path.dirname(finalPath), {recursive: true}) deleted: the new file_utils case fails with
→ ENOENT: no such file or directory, open '…/file_utils_test_3uPX1q/nested/out/test.txt'.

Source files were byte-compared against pristine copies after every mutation, and every leaked file was removed; git status --porcelain is empty.

Coverage

100% line and branch coverage of all new/changed executable lines, verified by cross-referencing the v8 JSON report's uncovered-line list against the diff rather than by reading a percentage:

• skill_toolset.ts — this.outputDir = options.outputDir; covered; not in the uncovered set.
• run_skill_script_tool.ts lines 144–151 and run_skill_inline_script_tool.ts lines 143–150 — no uncovered statements, and neither if (outputDir) appears in the uncovered-branch set, so both arms execute.
• file_utils.ts — the only change is the parameter signature; there is no new executable line. Its remaining uncovered lines (67-70, 108-111, branches at 58, 66) are all pre-existing code this PR does not touch: the second containment guard discussed above, guessMimeType, and a ternary in the de-duplication loop.

Commands run

npx vitest run --project unit:core \
  core/test/tools/skills/skill_script_output_dir_test.ts \
  core/test/tools/skills/run_skill_script_tool_test.ts \
  core/test/tools/skills/run_skill_inline_script_tool_test.ts \
  core/test/tools/skills/skill_toolset_test.ts \
  core/test/utils/file_utils_test.ts \
  core/test/code_executors/unsafe_local_code_executor_test.ts
#  -> 6 files, 62 tests passed

npx vitest run --project integration \
  tests/integration/tools/run_skill_script_tool_test.ts \
  tests/integration/tools/run_skill_inline_script_tool_test.ts
#  -> 2 files, 20 passed, 4 skipped (platform-gated Windows cases)

# every agent suite under tests/integration/skills/, not just the two tool suites
npx vitest run --project integration \
  tests/integration/skills/script_sh tests/integration/skills/inline \
  tests/integration/skills/loader
#  -> 3 files, 3 passed  (their recorded events carry "outputFiles": [], so they
#     were never affected by this change)
npx vitest run --project integration tests/integration/skills/script_js/agent_test.ts
#  -> 1 passed

npm run build        # exit 0
npm run lint         # exit 0
npm run format:check # "All matched files use Prettier code style!"
npm run ts:check     # 307 errors in 47 files -- byte-identical to the stack base,
                     # i.e. zero new type errors (pre-existing repo state)

core/test/code_executors/unsafe_local_code_executor_test.ts was re-run unchanged. It is the existing integration-level exercise of materializeFiles through a real subprocess, and it proves the signature change disturbed neither the executor's input-file materialization nor its output-file scanning.

No suppressions of any kind were added — no any, as any, as unknown as, @ts-expect-error, eslint-disable, or coverage-ignore comment appears anywhere in this diff.

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 clone of this repository, build an LlmAgent with an UnsafeLocalCodeExecutor and a SkillToolset containing a skill whose scripts/write.js does require('node:fs').writeFileSync('output.txt', 'hello').
2. Invoke run_skill_script three times. Before this change git status at the repo root showed untracked output.txt, then output_2.txt, then output_3.txt. After it, git status stays clean and the tool result still carries the file inline as outputFiles[0].content.
3. Reconstruct the toolset as new SkillToolset(skills, {codeExecutor, outputDir: '/tmp/adk-skill-out'}) and invoke three more times. /tmp/adk-skill-out now holds output.txt, output_2.txt, output_3.txt, and the result's outputFiles[].name values match those on-disk names.
4. Run the targeted Vitest command above and confirm git status is still clean — the original symptom was a test-adjacent run leaving a file in the repository.

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.