Fix: persist skill script output files to the artifact service instead of the agent 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):

No existing issue.

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

Problem: both skill script tools ended their happy path with the same line —
result.outputFiles = await materializeFiles(result.outputFiles)
(run_skill_script_tool.ts:145, run_skill_inline_script_tool.ts:144) — and
materializeFiles defaulted its destination to process.cwd()
(core/src/utils/file_utils.ts:17). Three defects follow from that one default:

1. Host filesystem pollution, not session storage. Every file a skill script
   produces is written into the working directory of the agent server process —
   a directory no caller named and the session cannot address. It is not
   per-session either: two sessions running the same skill write into the same
   directory and collide, silently renamed _2, _3, … by the loop at
   file_utils.ts:37-52. The repository carried the proof: a tracked output.txt
   (contents: hello) at the repo root, left behind by the change that
   introduced skill script execution. Reproduced below — reverting the fix and
   running the suites re-creates that exact file plus six siblings.
2. Nothing was ever persisted as an artifact. No saveArtifact call existed,
   so outputs were invisible to load_artifacts, to the artifact REST surface
   and to the UI — inconsistent with the built-in code-execution flow, which does
   persist executor output files
   (code_execution_request_processor.ts:503-513).
3. Raw file bytes went into the model context. The whole
   CodeExecutionResult was returned as the tool response, including
   File.content — the encoded bytes of every output file. A script emitting a
   2 MB PNG puts ~2.7 MB of base64 into the prompt.

Solution: send the bytes where the session can reach them, and send the model
only a manifest.

• New core/src/tools/skill/script_output_utils.ts exports
  saveScriptOutputs(toolContext, result), which saves each output file through
  Context.saveArtifact(name, {inlineData}) — the same API the code-execution
  flow uses, which also records eventActions.artifactDelta[name] = version and
  is what surfaces a file to clients. It returns
  {stdout, stderr, outputFiles: [{name, mimeType}], warning?}; the
  SkillScriptResponse type makes it impossible to put bytes on that response.
  Saves run concurrently via Promise.allSettled, so one failing file does not
  serialise or sink the rest.
• New toBase64Content(file) in code_execution_utils.ts, co-located with the
  File type it operates on. Artifact payloads must be base64
  (FileArtifactService does Buffer.from(data, 'base64')), but the two
  executors disagree: UnsafeLocalCodeExecutor declares contentEncoding: 'utf-8' for text types while AgentEngineSandboxCodeExecutor leaves it
  undefined on content that is already base64. The rule is therefore
  "utf-8 → encode, anything else → pass through". It deliberately does not
  reuse getEncodedFileContent, which sniffs with isBase64Encoded and
  misclassifies plain text that happens to be valid base64 — hello is exactly
  such a string, and is exactly what a skill script writes.
• materializeFiles(files, dir: string): Promise<void> — the process.cwd()
  default is removed, so the compiler now prevents the next caller from
  re-arming the footgun. Its Promise<File[]> return and the createdFiles
  accumulator behind it are removed too: the skill tools were the only consumer
  of that list, the one remaining caller
  (unsafe_local_code_executor.ts:182, which already passes an fs.mkdtemp
  directory) discards it, and no test asserted it. The collision loop and the
  in-place file.name mutation stay — unsafe_local_code_executor.ts:274
  matches output candidates against the mutated inputFiles[].name to skip
  input files, and file_utils_test.ts pins the suffix behaviour for duplicate
  names within one batch. That mutation is now documented on the function
  instead of being implied by the return value.
• ScopedArtifactService is now exported from core/src/common.ts. It is the
  framework's own bridge between the two already-public artifact interfaces
  (BaseArtifactService → SessionArtifactService), and exporting it deletes a
  seven-method copy of it that the integration test util would otherwise carry
  and that would silently drift from the real class. This is the one public API
  addition beyond the two response types.
• Deleted the tracked output.txt at the repo root. It is a leaked script output
  from the mechanism this PR removes, and the only grep -rn "output.txt" hits
  are unrelated in-test filename literals.

Design notes:

• The artifact service is the single destination; no outputDir knob is
  added. One destination means one contract. The artifact service is
  session-scoped, versioned, already sanitizes filenames and rejects traversal
  (file_artifact_service.ts:476-516), is reachable by clients and by
  load_artifacts, and matches the built-in code-execution flow. A configurable
  host directory would re-introduce a second, unversioned, cross-session-colliding
  path for no demonstrated requirement.

• Collisions are handled by artifact versioning, not by _2/_3 renaming.
  Running a script twice now yields versions 0 and 1 of one artifact.

• A persistence failure is never a tool error. stdout/stderr always come
  back. No artifact service configured → the produced filenames are still
  reported with an explicit warning, and one logger.warn; a rejected
  saveArtifact → the saved subset plus a warning naming the failures, with
  each rejection reason logged (never returned, so it cannot enter the model
  context). The existing EXECUTION_ERROR catch was deliberately not widened
  — a storage outage must not be reported as "Failed to execute script". No new
  error codes.

  A review round proposed collapsing this to a plain Promise.all and letting a
  rejection propagate, matching the ten-line loop in
  code_execution_request_processor.ts:503-513. Declined, with the reasoning
  here rather than in a comment. The same argument the reviewer accepted for
  keeping the no-artifact-service warning ("turning a completed script run into
  a hard tool error would be worse than reporting the loss") applies unchanged
  to a GCS outage or a filename the artifact backend rejects: in both cases the
  script ran and its stdout is worth returning. Concretely, Promise.all here
  would be worse than the reviewer's own description of the trade-off — the
  tools do return saveScriptOutputs(...) inside their try, and a returned
  promise's rejection is not caught by the enclosing catch in an async
  function, so the failure would surface as an unhandled runAsync rejection
  rather than as a mildly inaccurate EXECUTION_ERROR. The processor is not a
  precedent for this: it builds an event inside the framework's own error
  handling, whereas this runs at a tool boundary that must return a response.

  What the review did fix here: the redundant outputFiles.length === 0 early
  return is folded into the no-service guard, and describeFile is replaced by
  one up-front names projection. A new case,
  does not warn about a missing artifact service when the script produced no files, pins the behaviour that early return used to provide.

• Both tools are fixed in one change, on purpose. It is the same one-line
  defect in sibling tools sharing the new helper. Splitting it would leave the
  two tools with contradictory output semantics for a release, and would produce
  a second PR conflicting on the same file. The src diff is ~150 lines across
  6 files, one logical checkpoint, so it is not stacked.

• Not modified: DEFAULT_SKILL_SYSTEM_INSTRUCTION (the response already
  names the saved artifacts; editing it would churn prompt-text assertions for no
  behavioural gain). Out of scope: stdout/stderr truncation, and
  normalising contentEncoding in the executors themselves — toBase64Content
  handles that divergence at the point of use.

Breaking changes (both tool classes are @experimental, and runAsync is
typed Promise<unknown>):

1. outputFiles entries lose content and contentEncoding, and a warning
   field may appear. grep -rn "outputFiles" core/src dev/src integrations/src
   finds only the two tools and the code-execution processor, so the impact is
   limited to the model-facing contract — which is the point of the fix.
2. Files no longer appear in process.cwd(). Anyone relying on that now gets a
   session-scoped, versioned artifact instead.
3. materializeFiles(files) now requires dir, and returns void instead of
   File[]. It is not exported from core/src/index.ts or core/src/common.ts,
   and all in-repo callers already pass a directory and discard the return, so
   this is internal only.
4. ScopedArtifactService is newly exported (additive).

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, and the suppression grep over this diff now returns
nothing. The new unit test originally carried one
{…} as unknown as InvocationContext (the repo's existing fixture pattern for
that type, in 22 core/test files); a review round asked whether a real context
was cheap enough, and it is — the new file now builds
new InvocationContext({invocationId, agent, session, pluginManager, artifactService}) from a real LlmAgent and createSession. The four
pre-existing casts in the tool suites are left alone; they belong to fixtures
this PR only extends. npx tsc --noEmit is red on main
today with 280 pre-existing errors; on this branch it is 279, and a per-file
diff of the two reports shows the single difference is one removed error in
run_skill_script_tool_test.ts (the as File cast in the deleted test). This
change introduces zero new type errors and removes one.

Collision check (run before implementing): gh pr list --repo AmaadMartin/adk-js --state open --limit 100, plus gh pr diff --name-only on
every plausibly adjacent PR. Five overlaps found; none lands this change:

• Fix: require an explicit destination directory so skill script output never lands in process.cwd() #353 "require an explicit destination directory so skill script output never
  lands in process.cwd()" (stacked on Fix: materialize skill script output into a declared directory, never process.cwd() #298 "make the skill script output
  directory configurable") touches nine of the same files and fixes defect 1 by
  a competing design: it keeps writing to the host filesystem and adds an
  outputDir option to SkillToolset. It does not persist anything as an
  artifact and still returns raw File.content to the model, so defects 2 and 3
  survive it. This PR is deliberately not stacked on that branch: the guidance
  to stack exists to keep a review digestible, and a child whose diff is dominated
  by deleting its parent's central feature (outputDir /
  materializeOutputFiles and their tests) is harder to review than the same
  change against main, not easier. The two are mutually exclusive designs for
  one defect and only one should land — this one, per the "single destination"
  note above. Both remove the materializeFiles default identically, so whichever
  lands second needs a trivial resolution in file_utils.ts.
• Fix: stop skill-script integration tests leaking generated files into the repo working tree #305 "stop skill-script integration tests leaking generated files into the
  repo working tree" touches the same two integration files. It mitigates the
  leak in tests; this PR removes its cause. Not stacked on, same reason.
• Fix: stop materializeFiles mutating its inputs, and bound and atomize collision resolution #409 (materializeFiles mutation/collision bounds) and Fix: test path containment, not string prefix, in materializeFiles #371 (path
  containment) change materializeFiles' body, which this PR does not touch —
  they compose cleanly with the signature change here.
• Chore: remove stray output.txt from the repo root #271 "remove stray output.txt from the repo root" deletes the same file; if
  it lands first, that hunk drops out cleanly.

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:

• I have added or updated unit tests for my change.
• All unit tests pass locally.

npx vitest run --project unit:core core/test/tools/skills \
  core/test/code_executors/code_execution_utils_test.ts \
  core/test/utils/file_utils_test.ts
  -> Test Files 10 passed (10) | Tests 130 passed (130)

npx vitest run --project integration tests/integration/tools
  -> Test Files 10 passed (10) | Tests 28 passed | 4 skipped (32)

npx vitest run --project integration tests/integration/skills/script_js/agent_test.ts
  -> Test Files 1 passed (1) | Tests 1 passed (1)

npm run build && npm run lint && npm run format:check && npm run docs:check
  -> all exit 0

git status --porcelain    # empty after both runs: no stray *.txt at the repo root

That last line is the human-visible form of the bug and is worth keeping: before
this change, running the two skill-script integration suites left
output_from_script*.txt, test_output_*.txt and test_inline_output_*.txt
in the repository root.

CI: run-tests is green on all three matrix legs — ubuntu-latest,
macos-latest and windows-latest — running the full npm run test:coverage
suite (2691 tests), plus npm run lint, npm run format:check and
npm run docs:check. Two intermediate red runs are worth naming so they are not
mistaken for flakes that were papered over: the first was the real
script_js/agent_test.ts regression described above, the second the real Windows
toEqual over-assertion described above, and both were fixed rather than
retried. One further macOS failure was
tests/integration/app_loader/app_loader_test.ts > should discover apps vs agents across directories and standalone files timing out at 40000 ms — a
pre-existing, install-bound timing flake in a file this PR does not touch, which
passed on the same commit on ubuntu and windows and on re-run. After the review
revisions, one windows leg similarly timed out at 5000 ms in
core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdout — a shell cold-start flake in a test this PR does
not touch (the very effect the existing TEST_EXECUTION_TIMEOUT comment in the
skill integration suite documents); it passes locally and passed on re-run.

Added:

• core/test/tools/skills/script_output_utils_test.ts (new, 10 cases) — no
  script output, and no script output with no artifact service (which must not
  warn about discarding zero files); save-and-summarise with an explicit
  assertion that
  Object.keys(outputFiles[0]) is exactly ['name', 'mimeType']; the three
  encoding cases (utf-8 encoded, base64 passed through, undeclared treated as
  base64); artifact-delta recording; a repeated filename becoming version 1; the
  no-artifact-service warning path; and a partial failure where the second of
  three saves rejects, asserting the other two are returned, the warning names
  the failed file, the reason is logged rather than returned, and nothing throws.
  It drives a real InMemoryArtifactService bound to a session, so the assertions
  are about bytes that actually round-tripped, not about a recording spy.
• core/test/code_executors/code_execution_utils_test.ts — a
  describe('toBase64Content') appended next to getEncodedFileContent; the file
  is otherwise unrestructured.
• Both skill tool unit suites — saves script output files to the artifact service and omits file bytes from the response, does not write script output files to the process working directory (snapshots fs.readdir(process.cwd())
  either side of runAsync; this is the regression guard for the reported
  defect), and reports produced files with a warning when no artifact service is configured.
• Both integration suites — real UnsafeLocalCodeExecutor, no mocks:
  saves script output files to the artifact service (loads the artifact back
  and decodes it to hello from script file, the end-to-end proof that the
  encoding normalization is right, and asserts the file is absent from
  process.cwd()), creates a new artifact version instead of a renamed file on repeat runs (two versions of one artifact, no _2.txt anywhere), and the
  no-artifact-service warning path.
• tests/integration/tools/artifact_service_test_utils.ts — two small helpers
  shared by both integration suites: createSessionArtifactService() (an
  InMemoryArtifactService wrapped in the now-exported ScopedArtifactService)
  and loadArtifactText(). An earlier revision hand-rolled a seven-method
  session-scoped adapter here because ScopedArtifactService was not public;
  a review round correctly called that a copy that would drift, so the real
  class is exported instead and the duplicate is gone.

Existing tests rewritten in place — declared explicitly, invoking the
documented exception. The repo's guidance is to add a test rather than edit
one, precisely so a reviewer can tell a wrong assertion from an inconvenient
one. Eight cases are edited in place here, and all eight are the documented
"the existing test encodes the wrong behaviour" case: every one of them
asserts either that a file appears in process.cwd() (or in the CLI's launch
directory) or that a collision produces a _2.txt rename — i.e. each pins
exactly the defect being removed, so no untouched original can survive the fix.
The replacements are strictly stronger: they assert the working directory stays
clean, that the artifact bytes round-trip, and that a repeat run increments the
artifact version. The two vi.mock('../../../src/utils/file_utils.js') fixtures
deleted alongside them are part of the same exception — with the mock in place
the new cwd-snapshot tests could never observe a real write, so keeping it would
have made the regression guard vacuous.

The eight, enumerated:

#	File	Case
1	core/test/tools/skills/run_skill_script_tool_test.ts	calls materializeFiles with output files from executor
2	core/test/tools/skills/run_skill_inline_script_tool_test.ts	calls materializeFiles with output files from executor
3	tests/integration/tools/run_skill_script_tool_test.ts	creates files in process.cwd returned from execution
4	tests/integration/tools/run_skill_script_tool_test.ts	handles file collisions by appending a numeric suffix
5	tests/integration/tools/run_skill_inline_script_tool_test.ts	creates files in process.cwd returned from execution
6	tests/integration/tools/run_skill_inline_script_tool_test.ts	handles file collisions by appending a numeric suffix
7	tests/integration/skills/script_js/agent_test.ts	should run agent with skills successfully (read its three files out of the CLI's launch directory)
8	both core tool suites	the vi.mock('file_utils') fixtures, per the paragraph above

Detail on what each pinned and what replaced it:

File	Removed test	Why it cannot be kept
core/test/tools/skills/run_skill_script_tool_test.ts	calls materializeFiles with output files from executor	Asserted materializeFiles is called with the cwd default. The tool no longer calls it at all. The vi.mock('../../../src/utils/file_utils.js') block went with it — and removing that mock is what lets the new cwd-snapshot test see a real write.
core/test/tools/skills/run_skill_inline_script_tool_test.ts	calls materializeFiles with output files from executor	Same.
tests/integration/tools/run_skill_script_tool_test.ts	creates files in process.cwd returned from execution	Asserted fs.access(path.join(process.cwd(), 'output_from_script.txt')) succeeds — literally the defect. Its inverse is now asserted by saves script output files to the artifact service.
tests/integration/tools/run_skill_script_tool_test.ts	handles file collisions by appending a numeric suffix	Pinned the _2 rename that artifact versioning replaces. Its subject is now covered by creates a new artifact version instead of a renamed file on repeat runs, which asserts versions [0, 1] and that no _2.txt exists.
tests/integration/tools/run_skill_inline_script_tool_test.ts	both of the above	Same, for the inline tool.

Every other test in all five files is untouched. No test was skipped, disabled,
.only'd, or weakened, and no case was deleted without a named replacement.

One further test rewritten, caught by CI, not by me. The first push of this
branch went red on run-tests (ubuntu-latest):
tests/integration/skills/script_js/agent_test.ts — a CLI end-to-end test that
spawns adk run in a fixture directory and then reads
ephemeral_entanglement.md, index.html and sketch.js out of that
directory, because process.cwd() of the spawned agent process was the
fixture directory. It is the same defect one layer up, and it is the best
possible demonstration of it: this suite is why the CLI's launch directory
accumulates model output. Rather than delete the content assertions, the test now
runs the CLI with --artifact_service_uri file://<mkdtemp>, asserts the three
files are absent from the fixture directory, and compares the saved artifacts
against the same expected/ fixtures — so every original assertion survives,
pointed at the new destination. Locally: passes; with the fix reverted it fails
with AssertionError: promise resolved "undefined" instead of rejecting (the
file is back in the launch directory).

Two pre-existing Windows defects surfaced while validating this, neither fixed
here. Both are called out at their call sites and queued separately:

1. UnsafeLocalCodeExecutor skips input files when scanning for outputs by
   comparing File.name (forward slashes, e.g. scripts/hello.js) against an
   fs.readdir({recursive: true}) entry (backslashes on Windows). The
   comparison never matches there, so every input file is reported as an
   output — the windows-latest leg returned 12 output files for a script that
   writes one. The integration tests therefore assert the script's output by
   containment rather than list equality, with a comment naming the cause; the
   exact-shape assertion (toEqual([...]), and the Object.keys(...) check that
   no bytes are present) lives in the unit tests, where the executor is a mock.
   Note this defect is worsened in kind by nothing in this PR — it previously
   copied every skill script into the launch directory, and now saves them as
   artifacts instead — but it should be fixed on its own.
2. getArtifactServiceFromUri mangles file:// URIs on Windows (below).

The CLI test builds its file:// URI as `file://${root.split(path.sep).join('/')}`
rather than with pathToFileURL, because getArtifactServiceFromUri strips the
scheme with uri.split('://')[1]: a canonical file:///C:/… would leave
/C:/…, which path.resolve mangles on Windows. That is a pre-existing
limitation of the CLI's URI parsing, is commented at the call site, and is out of
scope here — it has been queued separately rather than fixed in this PR.

Proof each test can fail. Each new test was run against mutated source and
observed to fail:

Mutation	Result
Restore dir = process.cwd() in materializeFiles and result.outputFiles = await materializeFiles(result.outputFiles); return result; in run_skill_script_tool.ts	3 unit + 3 integration tests fail. does not write script output files to the process working directory → AssertionError: expected [ '…', …(30) ] to deeply equal [ '…', …(29) ]. saves script output files to the artifact service and omits file bytes… → expected { stdout: 'script stdout', …(2) } to deeply equal { … }. Integration: expected [ { …(4) } ] to deeply equal [ { …(2) } ] — four keys instead of two, i.e. the file bytes back in the response. The run also left seven files in the repository root — output.txt, output_from_script.txt, _2, _3, _4, cwd_regression_output.txt, unsaved_output.txt — reproducing the defect, including the committed output.txt this PR deletes.
Same, applied to run_skill_inline_script_tool.ts instead	3 unit + 3 integration tests fail, same shapes, on the inline suites.
toBase64Content always return file.content;	2 unit + 1 integration test fail. toBase64Content > base64-encodes content declared as utf-8 and saveScriptOutputs > base64-encodes utf-8 file content before saving → expected 'hello' to be 'aGVsbG8='. Integration saves script output files to the artifact service → expected '\ufffd\ufffde\ufffd…' to be 'hello from script file' (mojibake from double-decoding).
saveScriptOutputs returns {...result} (the raw File[])	13 tests fail across the three suites, including the Object.keys(…) === ['name','mimeType'] assertion and expected {} to deeply equal { 'a.txt': +0, 'b.txt': +0 } for the artifact delta.
No-artifact-service branch returns {stdout, stderr, outputFiles: []} with no warning	3 tests fail — the reports produced files with a warning… case in all three suites.
Partial-failure path pushes every settled outcome as saved	1 test fails — returns the saved subset with a warning when an artifact save fails: the failed file is reported as saved.
Same as the first row, against the CLI end-to-end suite	tests/integration/skills/script_js/agent_test.ts fails with AssertionError: promise resolved "undefined" instead of rejecting, and the three generated files reappear in the fixture directory the agent was launched from.

One further mutation, added with the review revisions: dropping the
outputFiles.length > 0 guard on the no-service branch fails
does not warn about a missing artifact service when the script produced no files, which starts reporting "0 output file(s) … discarded". Every mutation in
this table was re-run after the review revisions and still fails as recorded.

Coverage. Measured with --coverage restricted to the touched files:

File	Stmts	Branch	Funcs	Lines
core/src/tools/skill/script_output_utils.ts	100%	100%	100%	100%
core/src/code_executors/code_execution_utils.ts	100%	100%	100%	100%
core/src/utils/file_utils.ts	94.11%	80%	100%	94.11%

The file_utils.ts shortfall is lines 61-64, the pre-existing second
path-traversal check; this PR changes only that function's signature, return
type and doc comment, adding no executable line to it. No /* v8 ignore */,
istanbul ignore or any other coverage suppression was added.

Manual End-to-End (E2E) Tests:

Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

The integration tests above are the automated form of this — they drive the real
UnsafeLocalCodeExecutor 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');, wire
   new SkillToolset([skill], {codeExecutor: new UnsafeLocalCodeExecutor()})
   into an agent, and run it with an artifact service configured (e.g.
   InMemoryRunner). Have the model call run_skill_script.
   • Before: report.csv appears in the directory you launched the server from,
     and its bytes are in the tool response.
   • After: the launch directory stays clean, the response is
     {stdout, stderr, outputFiles: [{name: 'report.csv', mimeType: 'text/csv'}]},
     and report.csv is loadable from the session's artifact service — including
     via the load_artifacts tool.
2. Run it a second time. You get version 1 of report.csv, not a
   report_2.csv.
3. Run the same agent with no artifact service. The script still runs,
   stdout/stderr still come back, outputFiles still names report.csv, and
   warning says the file was discarded. One logger.warn is emitted. Nothing
   is written to disk.
4. git status in the repository root after any of the above: clean.

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.