Fix: require an explicit output directory and resolve the default in SkillToolset

[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):
   No existing issue.
2. Or, if no issue exists, describe the change:

Problem: materializeFiles(files, dir = process.cwd()) in core/src/utils/file_utils.ts made "write into whatever directory the host process happens to be running in" the silent behavior of a shared utility. Any caller that omitted the second argument inherited the host's working directory without ever naming it, and nothing in the type system objected. Agent-produced file names come from an executed script (model-influenced data); the destination came from an implicit default.

Solution: make the destination explicit, without changing where files actually land.

• materializeFiles(files, dir: string) — the = process.cwd() default is dropped, so dir is required. The function body is untouched. A call site that forgets the argument is now a compile error (TS2554), not a silent write into the launch directory.
• The cwd default is not removed, only relocated to something that declares it. SkillToolset resolves it in an outputDir getter (this.configuredOutputDir ?? process.cwd()), read per call so a host that chdirs is not pinned to the value captured at construction.
• Net observable behavior is unchanged: with no outputDir configured, both skill tools write exactly where they wrote before, with the same _2/_3 collision-suffix renaming reflected back into result.outputFiles.

This PR is stacked on fix/skill-script-output-dir (#298), not branched from main.

Collision check (run before writing any code, per contributor workflow): gh pr list --repo AmaadMartin/adk-js --state open --limit 100 --json number,title,headRefName, then gh pr diff <n> --name-only on every plausibly adjacent PR. Five overlaps found in this cluster:

• Fix: materialize skill script output into a declared directory, never process.cwd() #298 "Fix: make the skill script output directory configurable" — touches the same files and already adds the SkillToolset.outputDir option and the two-argument call sites, while deliberately keeping dir = process.cwd() in materializeFiles. That is an overlap, not a duplicate: it ships the knob and leaves the implicit default in place. This PR is therefore stacked on it and targets that branch, and contains only the residual delta (required dir + the getter). Please merge Fix: materialize skill script output into a declared directory, never process.cwd() #298 first. Nothing from Fix: materialize skill script output into a declared directory, never process.cwd() #298 is reimplemented here.
• Fix: require an explicit destination directory so skill script output never lands in process.cwd() #353 "Fix: require an explicit destination directory…" — also stacked on Fix: materialize skill script output into a declared directory, never process.cwd() #298, and also makes dir required. It then goes further and stops writing to process.cwd() altogether when outputDir is unset, plus namespaces output under <outputDir>/<invocationId>/. That is a deliberate behavior change; this PR is the behavior-preserving alternative. These two are alternatives for the same base — they should not both merge. The choice is a product decision: whether an unconfigured SkillToolset should keep writing to the launch directory (this PR) or write nothing (Fix: require an explicit destination directory so skill script output never lands in process.cwd() #353). This PR keeps the current behavior because three existing tests pin it as intended product behavior (see below).
• Fix: stop materializeFiles mutating its inputs, and bound and atomize collision resolution #409 (materializeFiles input mutation / collision bounding), Fix: test path containment, not string prefix, in materializeFiles #371 (the startsWith prefix containment defect), Chore: remove the accidentally committed output.txt from the repository root #429 (removes the stray tracked root output.txt) — all touch adjacent lines but fix different defects. Not duplicated and not stacked on; each is left strictly alone here.

Why the default destination is deliberately preserved. Writing skill-script output into the agent process's working directory is tested product behavior, not an accident — a local CLI agent generating files into the user's project directory is a real scenario. These three tests are the regression signal and are left completely untouched by this PR:

• tests/integration/tools/run_skill_script_tool_test.ts — creates files in process.cwd returned from execution
• tests/integration/tools/run_skill_inline_script_tool_test.ts — the same assertion for the inline tool
• tests/integration/skills/script_js/agent_test.ts — a full agent scenario spawned with cwd: PROJECT_PATH that asserts three generated files appear in PROJECT_PATH

Scope notes. materializeFiles is not exported from core/src/index.ts or core/src/common.ts, so requiring dir is an internal change with no downstream breakage, and it is not added to either barrel here. core/src/code_executors/unsafe_local_code_executor.ts already passed an explicit tempDir and is untouched. outputDir is host configuration, never a tool argument — it is not added to either tool's _getDeclaration(), so the model cannot influence where files are written. No new error codes: a materializeFiles rejection still propagates into each tool's existing try/catch and surfaces as EXECUTION_ERROR. outputDir is not validated at construction time, so an unwritable path fails at write time with the OS error rather than in a constructor that touches the filesystem. The materializeFiles doc comment describes its containment check honestly as a lexical guard rather than a sandbox.

Doc comments on materializeFiles and the outputDir getter were trimmed in the second round: the required-dir rationale now lives once, on @param dir, and the getter keeps only the per-read note instead of repeating the option doc's first sentence and default. The lexical-guard / TOCTOU caveat is retained verbatim.

No suppressions of any kind were added — no any, as any, as never, @ts-expect-error, @ts-ignore, eslint-disable, or coverage-tool ignore, in src or in tests.

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/tools/skills/ core/test/utils/file_utils_test.ts \
  core/test/code_executors/unsafe_local_code_executor_test.ts
  -> Test Files 9 passed (9) | Tests 107 passed (107)     # includes the third call site

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 20 passed | 4 skipped (24)

git status --porcelain   # empty: the integration run leaves nothing in the working tree

Added (core/test/tools/skills/skill_toolset_test.ts):

• defaults to the process working directory — pins that an unconfigured toolset resolves to process.cwd(), i.e. that the relocated default is really still the cwd.
• resolves the working directory on each read, not at construction — constructs the toolset, then moves process.cwd(), and asserts the getter follows. This is what makes the getter equivalent to the old call-time default rather than a construction-time snapshot.
• keeps a configured directory when the process working directory moves — the negative of the above: a configured outputDir must ignore cwd entirely.

Existing tests changed — declared explicitly. All four are tests #298 added on the branch this is stacked on; none of them exists on main, and none of the three cwd regression tests named earlier is touched.

File	Was	Now	Why
core/test/tools/skills/run_skill_script_tool_test.ts	toHaveBeenCalledWith([testFile], undefined)	toHaveBeenCalledWith([testFile], process.cwd())	Mechanical: the resolved value moved from inside materializeFiles to the getter, so the argument is now the path instead of undefined. Same behavior asserted.
core/test/tools/skills/run_skill_inline_script_tool_test.ts	same	same	same
core/test/tools/skills/skill_toolset_test.ts	is undefined when no directory is configured → toBeUndefined()	defaults to the process working directory → toBe(process.cwd())	Pins the removed field shape; outputDir is now a getter that never returns undefined.
core/test/utils/file_utils_test.ts	should default the base directory to the working directory of each call	removed	Asserts the code path this PR deletes, and calls materializeFiles(files) with one argument, which no longer compiles. Its intent — cwd resolved per call, not snapshotted — is not lost: it is carried by the new resolves the working directory on each read, not at construction case, at the location where that behavior now lives. Verified to still fail on mutation M4 below.

Test-isolation fixes (second review round). Two hazards were found in tests #298 added on the stack base; both are fixed here rather than deferred to the base PR, since this PR already edits those cases:

File	Hazard	Fix
run_skill_script_tool_test.ts, run_skill_inline_script_tool_test.ts	#298 hoisted one testFile object to describe scope, shared by three cases. materializeFiles mutates file.name in place when it resolves a collision (core/src/utils/file_utils.ts:59), so a shared fixture is a shared mutable object, safe only while the mock happens not to mutate it.	Each case builds its own local fixture again, restoring the pre-#298 shape. The one edit kept is 'utf8' as File to FileContentEncoding.UTF8, which removes a cast.
tests/integration/tools/run_skill_script_tool_test.ts	The configured-output-dir case asserted the absence of output_from_script.txt in the cwd and unconditionally fs.rm'd it in finally, but that name is owned by the two neighbouring cwd tests (one writes then unlinks it with no finally, the next pre-creates it). A failure in the preceding test made this one fail spuriously and destroyed the neighbour's file.	Added a dedicated fixture script create_file_for_output_dir.js writing output_to_configured_dir.txt, a name only this test uses. The sibling inline-script case already used a unique test_output_${Date.now()}.txt and is unchanged.

Verified both directions on the integration hazard, with a neighbour's file planted in the cwd:

• Before the fix: x creates files in the configured output directory -> expected true to be false, and the planted file was deleted by the test's finally.
• After the fix: the case passes and the planted file is left untouched.

Per the spec's guidance, no test asserting "the argument is mandatory" was added: that is a compile-time property and tsc --noEmit proves it directly (mutation M1). No test was skipped, disabled, .only'd, or weakened.

Proof each test can fail. Every assertion was run against mutated source and observed to fail.

#	Mutation	Result
M1	Call site reverted to await materializeFiles(result.outputFiles)	Compile error, which is the whole point of the change: core/src/tools/skill/run_skill_script_tool.ts(145,34): error TS2554: Expected 2 arguments, but got 1. A missed call site can no longer be a silent cwd write.
M2	Getter ignores config: return process.cwd();	4 unit + 2 integration fail. exposes the configured directory and keeps a configured directory when the process working directory moves; both tools' materializes output files into the configured output directory → expected "spy" to be called with arguments: [ …(2) ]; integration creates files in the configured output directory → ENOENT: no such file or directory, open '/tmp/adk-skill-output-vvwle3/output_from_script.txt' and …/test_output_1785557155050.txt.
M3	Wrong default: ?? '/tmp/not-the-cwd'	4 unit fail. defaults to the process working directory; resolves the working directory on each read…; both tools' calls materializeFiles with output files from executor.
M4	cwd captured at construction (resolvedOutputDir = options.outputDir ?? process.cwd() in the constructor)	1 unit fails, the case that replaced the deleted file_utils_test one: resolves the working directory on each read, not at construction → expected '/usr/local/google/home/…' to be '/tmp/skill-output-after-chdir'.

Coverage. Measured on the two changed source files:

File               | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
  skill_toolset.ts |   96.22 |    93.75 |    87.5 |   96.22 | 79,123-124,207-209
  file_utils.ts    |   94.59 |       80 |     100 |   94.59 | 69-72

Every uncovered line was cross-referenced against git diff and none is a line this PR adds: skill_toolset.ts:79 is the pre-existing skills-normalization ternary, 123-124 is getSkill, 207-209 is a toolCache path; file_utils.ts:69-72 is the pre-existing second traversal check (a separate defect, out of scope). Both branches of ?? process.cwd() are exercised. The per-file percentages are diluted by that pre-existing untested code, not by new code.

Type/lint checks on the exact pushed commit: npm run build and npm run lint both exit 0. npm run ts:check is red on this repo today for pre-existing reasons; the error profile was captured on the stack base and on this branch and diffed — 304 errors on both, byte-identical — so this change introduces zero new type errors.

CI: this PR targets fix/skill-script-output-dir, and every workflow in .github/workflows/ is gated on pull_request: branches: [main], so run-tests does not trigger for a stacked base. The commands above were therefore run locally against the exact pushed commit.

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

The integration suites above are the automated form of this, driving the real UnsafeLocalCodeExecutor against the real filesystem with no mocks. To reproduce by hand:

1. Mount a skill whose scripts/create_file.js is const fs = require('fs'); fs.writeFileSync('report.csv', 'a,b');, then from any directory (cd ~/my-project && node agent.js) build the toolset without an output directory and invoke run_skill_script:

   new SkillToolset([skill], {codeExecutor: new UnsafeLocalCodeExecutor()});

   report.csv appears in ~/my-project — unchanged from before this PR. That is the point: the default did not move.
2. Add an output directory and re-run:

   new SkillToolset([skill], {
     codeExecutor: new UnsafeLocalCodeExecutor(),
     outputDir: '/tmp/skill-output',
   });

   report.csv now appears at /tmp/skill-output/report.csv, ~/my-project stays clean, and result.outputFiles[].name remains relative to the destination.
3. Point outputDir at a directory and have the script write ../escape.txt. The existing Path traversal detected error still surfaces through each tool's existing catch block as EXECUTION_ERROR; no new error type or code was added.
4. tests/integration/skills/script_js/agent_test.ts is the end-to-end check for the unchanged default. Note: it cannot run in this sandbox — its beforeAll hook shells out to npm install in the fixture project and exceeds the hook timeout on a restricted network. This was confirmed to fail identically on the unmodified base commit (git stash → same beforeAll/npm install hook timeout, Tests 1 skipped), so the failure is environmental and pre-existing, not caused by this change. The file is not modified by this PR.

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.