Feat: Save skill script output files as artifacts (opt-in saveOutputsAsArtifacts)

[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: RunSkillScriptTool and RunSkillInlineScriptTool deliver the files a script produced by writing them onto the host filesystem and returning their paths (run_skill_script_tool.ts:145, run_skill_inline_script_tool.ts:144). ADK already has a first-class, storage-backed channel for handing produced files back to a session — Context.saveArtifact() (core/src/agents/context.ts:103), backed by the configured SessionArtifactService — but the skill script tools never used it. Outputs were therefore invisible to load_artifacts, to the artifact REST surface, and to later turns of the session.

Solution: a new opt-in SkillToolset option, saveOutputsAsArtifacts, that additionally saves each output file to the artifact service and reports the result.

• New core/src/utils/artifact_utils.ts exports saveFilesAsArtifacts(context, files). It saves each file with context.saveArtifact(name, {inlineData: {data, mimeType}}) — the same API and the same Part shape the built-in code-execution flow uses, so eventActions.artifactDelta[name] = version is recorded and the artifact is announced on the tool event like every other ADK artifact write. It is placed in core/src/utils/ and named generically because it operates on code-execution output files; nothing in it mentions skills.
• Both tools gained the same four lines after materializeFiles(), so the two cannot drift. Saving happens after materialization, so a collision-renamed output_2.txt is stored under the name the tool actually reports.
• The result gains savedArtifacts: [{filename, version}] and artifactSaveErrors: [{filename, error}]. The two arrays together account for every entry of outputFiles, so no file can go silently missing from both.

Design decisions worth calling out for review:

• Default off. saveOutputsAsArtifacts defaults to false, so every existing caller's result object is unchanged key-for-key and no artifact API is touched.
• Degrade, don't throw, when no artifact service is configured. The helper pre-checks invocationContext.artifactService and returns undefined, and the tools then return the plain result. The equivalent adk-python code path (_code_execution.py:477-488) raises here; that is correct for a mandatory flow but wrong for an opt-in one, where failing a script that already ran would discard more information than it protects. This mirrors the graceful-degradation idiom in adk-python's save_files_as_artifacts_plugin.py. The service is pre-checked rather than string-matching the 'Artifact service is not initialized.' error Context.saveArtifact throws.
• A save failure is reported, never swallowed. Each failure is logged at error level and pushed to artifactSaveErrors; remaining files are still attempted and stdout/stderr/outputFiles are still returned.
• user: filenames are refused. A leading user: escalates an artifact from session scope to cross-session user scope (in_memory_artifact_service.ts:229). Output filenames originate from executed script content, i.e. a model-influenced trust boundary, so such a name is refused and recorded as an ArtifactSaveError instead of being saved.
• Byte fidelity. Base64 content is passed through untouched; other content is encoded exactly once, using the same Buffer.from(content, contentEncoding) idiom as file_utils.ts:62-63, so the artifact carries the same bytes that were written to disk.

Explicitly out of scope: this saves artifacts in addition to materializing files. It does not change where files are written, and does not delete them afterwards. Running skill scripts without accumulating files on the host is separate work and is not claimed here.

Collision check (required before implementation): I scanned all 415 open PRs on the fork (gh pr list --repo AmaadMartin/adk-js --state open --limit 1000) and diffed every plausibly adjacent one. The relevant finding is #410 "persist skill script output files to the artifact service instead of the agent process cwd" (open, unmerged), which replaces the same two call sites and saves outputs with the same saveArtifact mechanism. It is not a duplicate of this PR and the two are not stackable: #410 makes saving unconditional and a replacement for host materialization (it removes materializeFiles from both tools and drops its process.cwd() default), whereas this PR is an opt-in flag defaulting to false that saves in addition to materialization. Gating #410's behaviour behind a default-false flag would leave outputs neither materialized nor saved. Whichever design is preferred, two pieces here are absent from #410 and worth keeping: the user: namespace guard, and structured per-file artifactSaveErrors in place of a single prose warning string. #437/#353/#298 only make the materialization output directory explicit and do not overlap; #130/#91/#95/#89/#88/#83 port the runner-level SaveFilesAsArtifactsPlugin (input blobs / code-execution flow), not the skill script tools.

Note on one flagged pattern in the diff: as unknown as InvocationContext appears in the new test's context fixture. That is the established idiom for faking an InvocationContext in this repo — 22 existing files in core/test/ use it, including all four skill test files touched here. No any, no @ts-expect-error, no @ts-ignore, and no eslint-disable were added anywhere in this change.

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.

Added core/test/utils/artifact_utils_test.ts (7 cases: per-file save with the exact Part asserted; utf-8 encoded exactly once; base64 passed through unchanged; artifactDelta recorded; no artifact service → undefined + warning; one save rejects → reported per file while the rest still save; user: filename refused without a saveArtifact call). Added four cases each to run_skill_script_tool_test.ts and run_skill_inline_script_tool_test.ts (option off; option on; option on with no artifact service; option on with a rejecting save), and two to skill_toolset_test.ts for the option default and override. Existing tests were not modified beyond adding an optional artifactService parameter to each file's createMockContext helper; no test was deleted, skipped, or weakened.

Commands run on the pushed commit:

npx vitest run --project unit:core core/test/utils/artifact_utils_test.ts core/test/tools/skills/
  -> Test Files 8 passed (8) | Tests 96 passed (96)
npx vitest run --project integration tests/integration/tools/run_skill_script_tool_test.ts
  -> Test Files 1 passed (1) | Tests 9 passed | 4 skipped (13)
npm run build   -> success
npm run lint    -> clean
npx tsc --noEmit --pretty false | grep -c "error TS"
  -> 281 both before and after this change (pre-existing baseline; this branch adds none)

Coverage of the new code (--coverage.include='core/src/utils/artifact_utils.ts'): 100% statements, 100% branches, 100% functions, 100% lines. Every statement added to the two tools is hit, and both sides of both new branches (the option gate and the artifacts ? ... : result ternary) are exercised.

Proof the tests can fail. Each new test was run against mutated source and confirmed to FAIL:

1. Per-file catch body replaced with a no-op → 3 tests fail, e.g. surfaces a failed save without failing the script: AssertionError: expected [] to deeply equal [ { filename: 'chart.png', …(1) } ].
2. if (file.name.startsWith(USER_NAMESPACE_PREFIX)) → if (false) → refuses a filename in the user namespace without calling saveArtifact fails: AssertionError: expected [ { …(2) }, …(1) ] to deeply equal [ { filename: 'allowed.txt', …(1) } ].
3. Option gate if (!this.toolset.saveOutputsAsArtifacts) return result; deleted → 2 tests fail, incl. AssertionError: expected "saveArtifact" to not be called at all, but actually been called 1 times.
4. No-artifact-service guard changed from return undefined to throw → 3 tests fail across all three test files.
5. Encoding branch collapsed to Buffer.from(file.content, 'utf-8') (a genuine double-encode) → 2 tests fail: AssertionError: expected 'WW1sdVlYSjVJR0o1ZEdWeg==' to be 'YmluYXJ5IGJ5dGVz'. Noted for honesty: a first attempt at this mutation (Buffer.from(content, file.contentEncoding) for both branches) was not caught, because Buffer.from(x, 'base64').toString('base64') is the identity on canonical base64 — that mutant is semantically equivalent, not a coverage gap, which is why the mutation above was used instead.

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

An automated round trip against real components (no mocks) is included in tests/integration/tools/run_skill_script_tool_test.ts: it runs the existing create_file.js skill script through a real UnsafeLocalCodeExecutor with a real InMemoryArtifactService, asserts savedArtifacts reports output_from_script.txt at version 0, and loads that artifact back out of the service to confirm it decodes to hello from script file. It unlinks the materialized file so no stray file is left in the repo.

To verify by hand:

1. Build a runner with an InMemoryArtifactService and an agent whose SkillToolset is constructed with {codeExecutor, saveOutputsAsArtifacts: true}.
2. Ask the model to run a skill script that writes a file. Confirm the tool result carries savedArtifacts, and that the file is listed by the load_artifacts flow / the artifact tab of the dev UI.
3. Remove the artifact service from the runner and repeat. The script still succeeds, the result carries neither new key, and a single warning is logged.

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.

CI note (Windows)

The first CI run failed only on run-tests (windows-latest), in the new integration test. The cause is a pre-existing defect in UnsafeLocalCodeExecutor, not in this change: it skips a skill's own input resources when collecting outputs by comparing inputFiles[].name (scripts/create_file.js) with a path from fs.readdir (scripts\create_file.js on Windows). The separator mismatch means every input resource is reported as an output — and then re-materialized into process.cwd(), which is where the _2…_11 suffixes in the failure output came from. The executor is untouched by this branch and the behaviour is present on main today; the surrounding tests never noticed because they match with .find() rather than asserting the whole list.

The integration test now matches the expected artifact with toContainEqual instead of pinning the entire list, since the executor's output-detection behaviour is not what it is testing. The exact byte round-trip assertion is unchanged, and the test was re-verified to still fail when the feature is broken (making the tool return result without spreading the artifact fields fails it). All four run-tests jobs (ubuntu, macos, windows, and the aggregate) pass on the pushed commit. The underlying executor bug is filed separately rather than fixed here, as it is independent of this change.

Revision after complexity review

• user: is now owned in one place. Rather than adding a fifth copy of the prefix, core/src/artifacts/base_artifact_service.ts now exports USER_NAMESPACE_PREFIX and fileHasUserNamespace(), and the four places that previously hardcoded the string import it: file_artifact_service.ts (a private const of the same name), in_memory_artifact_service.ts (a private copy of the helper, now deleted), and gcs_artifact_service.ts (two bare 'user:' literals, one of which paired with a magic filename.substring(5) that is now substring(USER_NAMESPACE_PREFIX.length)). All 99 pre-existing artifact-service tests (core/test/artifacts/) pass unchanged, which is what pins that this refactor is behaviour-preserving.
• The three tests that re-declared the result shape now reuse SaveFilesAsArtifactsResult, which artifact_utils.ts already exports (type ArtifactAugmentedResult = CodeExecutionResult & SaveFilesAsArtifactsResult); the integration test no longer re-spells the field types inline. Type declarations only — no test case changed.
• Dropped a redundant beforeEach in run_skill_inline_script_tool_test.ts that re-installed the behaviour the module mock factory already declares. (The equivalent hook in run_skill_script_tool_test.ts is kept: that file's factory is a bare vi.fn(), so there it is load-bearing.)
• Collapsed artifacts ? {...result, ...artifacts} : result to {...result, ...(await saveFilesAsArtifacts(...))} in both tools — spreading undefined is already a no-op, so the ternary was a branch with no behaviour behind it. The "no artifact service" test at each call site still passes unchanged.
• Left as-is: the as unknown as InvocationContext fixture cast, which the review flagged as non-blocking (it is the established idiom, with 40 pre-existing occurrences under core/test, and no InvocationContext test factory exists to use instead).

Re-verified after the revision: 206 unit tests pass (core/test/utils/artifact_utils_test.ts, core/test/tools/skills/, core/test/artifacts/), the integration test passes, npm run build and npm run lint are clean, tsc is unchanged at the 281 pre-existing errors, and the user:-guard and swallowed-failure mutations were re-run against the refactored code and still fail the tests they are supposed to fail.

Related inconsistency, not fixed here. The sibling path at core/src/agents/processors/code_execution_request_processor.ts:504-509 saves code-executor output files with saveArtifact({filename: outputFile.name, ...}) and no user: guard, so the same trust boundary is enforced in this change but not there. It is pre-existing and independent of this PR, so it is not bundled into this diff.