Skip to content

Feat: Persist skill script outputs whenever an artifact service is configured - #564

Open
AmaadMartin wants to merge 3 commits into
feat/skill-script-outputs-as-artifactsfrom
feat/skill-script-output-artifacts
Open

Feat: Persist skill script outputs whenever an artifact service is configured#564
AmaadMartin wants to merge 3 commits into
feat/skill-script-outputs-as-artifactsfrom
feat/skill-script-output-artifacts

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 3, 2026

Copy link
Copy Markdown
Owner

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):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:

Stacked on #517. Collision check was run before any code was written:
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 surfaced
#517 (feat/skill-script-outputs-as-artifacts), which already adds
core/src/utils/artifact_utils.ts::saveFilesAsArtifacts and wires it into both
skill script tools, and #410
(fix/skill-script-outputs-to-artifact-service), which persists outputs via
core/src/tools/skill/script_output_utils.ts but removes the local-disk
write. Four more open PRs (#556, #437, #353, #298) mutate the same two
call-site lines for output-directory containment. Rather than declare a second
conflicting artifact_utils.ts, this PR branches from #517 and ships only
the delta
. Base is feat/skill-script-outputs-as-artifacts, so it should be
reviewed and merged after #517.

Which diff to read. Against the stacked base this PR is 11 files,
+290/-248 — that is the reviewable unit. Against fork/main it necessarily
also contains all of #517, because #517 is its base; that is not a second copy
of #517's work. (git diff main is misleading in fork clones: local main
tracks origin/main, not this branch's base.)

Problem: Skill script output files were the only executor-produced files in
ADK that never reached the artifact service. #517 added the machinery but left
three behaviours that keep it from solving the problem it was written for:

  1. It is off by default. Persistence is gated on a new
    SkillToolset.saveOutputsAsArtifacts option defaulting to false, so the
    hosted-deployment case — an Agent Engine / Cloud Run container whose local
    disk is recycled between requests — still silently loses every generated
    file unless the developer discovers and flips a flag.
  2. Artifact keys inherit the on-disk rename. The save runs after
    materializeFiles(), which renames on local collision (output.txt ->
    output_2.txt) and mutates file.name in place. Artifact services version
    by filename, so this turns a second output.txt into a second artifact key
    instead of version 1 — and makes the key a function of whatever unrelated
    files happen to be sitting in process.cwd(), which in a server deployment
    is other sessions' leftovers.
  3. An absent contentEncoding is decoded as utf-8.
    Buffer.from(file.content, undefined) defaults to utf-8, but the one in-repo
    producer that omits the field (AgentEngineSandboxCodeExecutor) emits
    base64, so its bytes were being re-encoded and corrupted.

Solution:

  • The artifact service's presence is the only switch. Removed
    SkillToolset.saveOutputsAsArtifacts entirely. This matches the two existing
    precedents in the repo — code_execution_request_processor.ts saves output
    files unconditionally, and Runner.saveArtifacts() returns early when
    !this.artifactService rather than requiring a toggle. Artifact persistence
    in adk-js is opportunistic, never mandatory and never opt-in.
  • Save before materializeFiles(). Artifacts are keyed by the
    executor-reported name; the disk path keeps the collision-renamed name.
    Neither is derived from the other, and the response carries both:
    outputFiles[].name is where the file landed on disk, savedArtifacts keys
    are the artifact filenames.
  • savedArtifacts: Record<string, number>, added to the response only when
    at least one artifact was actually saved. With no artifact service (or no
    output files) the response is byte-identical to before this PR and to before
    Feat: Save skill script output files as artifacts (opt-in saveOutputsAsArtifacts) #517, so existing users and the recorded integration goldens are untouched.
  • Encoding is decided by FileContentEncoding, not guessed.
    UTF8 -> base64Encode(content), everything else (including undefined)
    passes through as base64, via the browser-safe base64Encode from
    env_aware_utils.ts rather than a raw Buffer.
  • Persistence stays best-effort: no artifact service is a logger.debug skip,
    and a single failing saveArtifact is a logger.warn that leaves the other
    files saved and never turns a script that ran, printed and wrote its files
    into an EXECUTION_ERROR.

Kept from #517: the local-disk write (removing it would break
tests/integration/skills/script_js/agent_test.ts, which is the product
contract for the CLI scenario), and the guard refusing an executor-produced
filename in the user: namespace, so generated code cannot widen an artifact
beyond the session that produced it.

Behaviour change to call out: when an artifact service is configured,
skill-script output files now consume artifact storage and appear in
artifactDelta. For a chatty script this is new storage volume. This is
deliberate — it is the same contract the code-execution path has always had.

Tests changed rather than added, and why. Three tests on the base branch
assert the behaviour this PR overturns and cannot compile once the option is
gone:

  • skill_toolset_test.tsdefaults saveOutputsAsArtifacts to false and
    enables saveOutputsAsArtifacts when the option is passed. Removed; the
    option no longer exists. Nothing else covered it.
  • Both tool tests' does not touch the artifact service when the option is off.
    Removed. The surviving guard for existing users is
    returns the plain result when no artifact service is configured, in both
    files, which now also asserts expect(warnSpy).not.toHaveBeenCalled().
  • The remaining base cases were reshaped for Record<string, number> (e.g.
    surfaces a failed save without failing the script became
    omits savedArtifacts and still succeeds when every save fails); no scenario
    was dropped.

Deliberate public-API additions. Two, both intentional:

  • USER_NAMESPACE_PREFIX and fileHasUserNamespace() (added by Feat: Save skill script output files as artifacts (opt-in saveOutputsAsArtifacts) #517 in
    base_artifact_service.ts) reach @google/adk through the existing
    export * in common.ts. Keeping them public is the right call: the user:
    prefix is part of the artifact service contract that every implementation of
    BaseArtifactService has to honour, so a third-party implementation needs
    them. This PR also finishes the sweep Feat: Save skill script output files as artifacts (opt-in saveOutputsAsArtifacts) #517 started — file_artifact_service.ts
    had two sites still open-coding startsWith(USER_NAMESPACE_PREFIX), which now
    go through the helper.
  • ScopedArtifactService is now exported from common.ts. Before this,
    tests/integration/tools/run_skill_script_tool_test.ts was the only file in
    the entire tests/integration suite importing from core/src by relative
    path. It is the framework's own bridge from a BaseArtifactService to the
    session-scoped service an invocation carries, and SessionArtifactService
    the interface it implements — is already public, so exporting the one concrete
    implementation of it is consistent. The alternative (a hand-rolled adapter in
    the test) was ~10 lines and worse.

No any, no @ts-expect-error, no eslint-disable, no coverage suppression
was added anywhere in this diff. The helper test builds its context with the
real InvocationContext constructor and createSession() rather than an
as unknown as InvocationContext cast.

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/artifact_utils_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/artifacts
  -> Test Files 9 passed (9) | Tests 162 passed (162)
     (core/test/artifacts is included because this PR touches
      file_artifact_service.ts; its 42 tests, and the gcs/in_memory/scoped
      suites, all pass)

npx vitest run --project integration tests/integration/tools/run_skill_script_tool_test.ts
  -> Tests 9 passed | 4 skipped (13)   (4 skips are the Windows-only cases)

npx vitest run --project integration tests/integration/skills
  -> Test Files 4 passed (4) | Tests 4 passed (4)   (unchanged fixtures/goldens)

npm run build   -> ok
npm run lint    -> clean
npm run format:check -> "All matched files use Prettier code style!"
npm run docs:check -> clean (this PR adds a public export)
npx tsc --noEmit -> 281 errors, exactly the same count as the base branch;
  all are the repo's pre-existing core/dist-vs-core/src type duality. The only
  one in a file this PR touches, `contentEncoding: 'utf8' as File` at
  run_skill_script_tool_test.ts:221, is pre-existing and untouched. (tsc is not
  a CI gate here: validation.yaml runs build, test:coverage, lint,
  format:check and docs:check.)

Coverage of the new/changed module, measured with
npx vitest run --project unit:core --coverage.enabled --coverage.include='core/src/utils/artifact_utils.ts' --coverage.reporter=text core/test/utils/artifact_utils_test.ts:

File               | % Stmts | % Branch | % Funcs | % Lines
 artifact_utils.ts |     100 |      100 |     100 |     100

Proof the tests can fail. Each mutation was applied to the source, the
targeted suites re-run, and the source restored:

# Mutation Result
1 Move the saveFilesAsArtifacts call back to after materializeFiles 3 failed — saves under the executor-reported name, not the materialized one: expected "saveArtifact" to be called with arguments: [ ObjectContaining{…} ]; saves artifacts before materializing the files: expected [ 'materializeFiles', 'saveArtifact' ] to deeply equal [ 'saveArtifact', 'materializeFiles' ]
2 Pass utf-8 file.content through unencoded instead of base64Encode(...) 3 failed — base64-encodes utf-8 content exactly once: expected '\ufffd\ufffde\ufffd…' to be 'hello from script file'; the inline tool's round-trip failed the same way
3 Delete the artifactService guard so the helper always calls context.saveArtifact 3 failed — skips silently when no artifact service is configured: expected "warn" to not be called at all, but actually been called 2 times, plus returns the plain result when no artifact service is configured in both tool tests
4 Always attach savedArtifacts instead of only when non-empty 5 failed — e.g. returns the plain result when no artifact service is configured: expected { stdout: 'script ran', …(3) } to not have property "savedArtifacts"

Mutation 3 is worth a note: on the first attempt it killed only one test.
Removing the guard leaves the tool response identical, because
Context.saveArtifact then throws Artifact service is not initialized. per
file straight into the helper's per-file catch. Coverage was already 100% and
did not notice. The no-service tests were strengthened to assert that the
supported no-artifact-service setup emits no warning — a real behavioural
difference between "skipped" and "attempted and failed N times" — which is what
makes mutation 3 fail at both the helper and the tool level.

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

Scratch script (not committed): a real UnsafeLocalCodeExecutor running a skill
script that writes notes.md (text) and chart.png (binary), through a real
RunSkillScriptTool + ScopedArtifactService/InMemoryArtifactService, in a
temp cwd, invoked twice in the same session. No mocks. Observed:

{
  "turn1_savedArtifacts": {"chart.png": 0, "notes.md": 0},
  "turn1_disk": ["notes.md", "chart.png"],
  "turn2_savedArtifacts": {"chart.png": 1, "notes.md": 1},
  "turn2_disk": ["notes_2.md", "chart_2.png"],
  "onDisk": ["chart.png", "chart_2.png", "notes.md", "notes_2.md"],
  "artifactKeys": ["chart.png", "notes.md"],
  "notesVersions": [0, 1],
  "notesRoundTrips": true,
  "chartByteIdentical": true,
  "turn2_artifactDelta": {"chart.png": 1, "notes.md": 1}
}

Four files on disk, two artifact keys with two versions each: the disk
rename did not leak into the artifact namespace. notes.md v0 decodes back to
the exact original text (this is the check that catches the base64 mistake) and
chart.png is byte-identical. artifactDelta is populated on the
function-response event, so artifact-aware clients see the files.

CI note: this PR targets feat/skill-script-outputs-as-artifacts, not
main. .github/workflows/validation.yaml triggers on
pull_request: branches: [main], so no test job will run on this PR. The local
results above were produced on the exact commit pushed.

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.

Amaad Martin added 3 commits August 2, 2026 22:22
Three changes to how the skill script tools persist output files:

Persistence is no longer gated on an opt-in toolset option. The presence
of an artifact service on the invocation is the switch, matching the
code-execution processor and Runner.saveArtifacts; a default-off option
meant the hosted-deployment case it exists for never happened.

The save now runs before materializeFiles rather than after. Artifact
services version by filename, and materializeFiles renames on local disk
collision (output.txt -> output_2.txt) and mutates file.name in place, so
saving afterwards made artifact keys a function of whatever unrelated
files happened to be sitting in process.cwd(). The helper returns a
filename -> version map and the tools add it to the response only when
something was saved, leaving the response byte-identical otherwise.

Absent contentEncoding is treated as base64 rather than decoded as utf-8,
which is what the one in-repo producer that omits the field actually
emits, and encoding goes through the browser-safe base64Encode helper.
Dropping the artifactService guard leaves the tool response identical,
because Context.saveArtifact then throws per file into the helper's
per-file catch. Assert instead that the supported no-service setup logs
no warning, so removing the guard is caught at both levels.
- Build the helper test's context with the real InvocationContext
  constructor and createSession instead of an `as unknown as` cast, so the
  compiler sees the fields the context is missing.
- Export ScopedArtifactService from common.ts and import it from
  @google/adk in the integration test, which was the only core/src import
  in the whole tests/integration suite.
- Drop the per-file "Saved ... as an artifact" debug log; the return value
  and the tools' savedArtifacts field already carry it, and nothing
  pinned it. The no-service skip log stays: it is the only signal on that
  path and a test asserts it.
- Finish routing file_artifact_service through fileHasUserNamespace()
  rather than open-coding startsWith(USER_NAMESPACE_PREFIX).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant