Skip to content

Fix: reject whitespace-padded artifact filenames in all three backends - #856

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/artifact-filename-whitespace-collapse
Open

Fix: reject whitespace-padded artifact filenames in all three backends#856
AmaadMartin wants to merge 2 commits into
mainfrom
fix/artifact-filename-whitespace-collapse

Conversation

@AmaadMartin

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):
    No existing issue.
  2. Or, if no issue exists, describe the change:
    Problem: FileArtifactService.getArtifactDir() trimmed the filename before it mapped the name onto a directory. ' a.txt' and 'a.txt' therefore addressed one artifact: a save under the padded name appended a version to the unpadded artifact, and a load under either name returned the other one's content. InMemoryArtifactService and GcsArtifactService keep the two names apart, so the three backends disagreed on the same key. An artifact filename is a storage key, so this is data corruption on the filesystem backend.

Solution: One shared guard, assertUnpaddedFilename(), rejects a filename whose scope-relative part has leading or trailing whitespace, and all three backends call it in saveArtifact. A filename becomes a directory name on the filesystem backend, and Windows removes trailing spaces from a path component (reference), so a padded name cannot be stored there distinctly from its unpadded twin; CI runs the unit suite on windows-latest, so "preserve the padding" is not implementable. The guard runs first in getArtifactDir(), whose existing try/catch callers turn a padded name into a miss, so every read and delete now misses instead of aliasing. The .trim() is deleted.

Breaking change: saveArtifact now rejects a padded filename on all three backends. FileArtifactService previously aliased such a name onto the unpadded artifact; the other two stored it as a distinct key. Reads on GCS stay unguarded, so a padded object written by an older version can still be listed, read and deleted. The one in-repo caller that passes a caller-controlled filename (Runner.saveArtifacts) already catches a save failure and keeps the original part.

Deviations from the spec (2):

  • The helper lives in a new core/src/artifacts/artifact_filename.ts, not in base_artifact_service.ts. core/src/common.ts re-exports * from './artifacts/base_artifact_service.js', so putting it there would publish it as @google/adk public API, which the spec forbids.
  • The GCS guard runs after the existing content check, not before it. All three backends then report the same error first for the same input.

Collision check: gh pr list --repo AmaadMartin/adk-js --state open --limit 100, then gh pr diff --name-only on every artifact PR. No open PR implements this fix. Three siblings touch the same files: #760 (host-independent rooting) and #765 (filename edge-case tests) both merge cleanly with this branch (git merge-tree); #855 (nested delete) rewrites getArtifactDir and conflicts textually, not semantically — whichever lands first, the other rebases.

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.

The new describe('whitespace-padded filenames') block is added at the end of the shared conformance suite runArtifactServiceTests(), so it runs against all three backends. No existing test is modified.

npx vitest run --project unit:core core/test/artifacts      # 143 passed
npx vitest run --project unit:core core/test/runner/runner_test.ts \
  core/test/runner/streaming_runner_test.ts \
  core/test/tools/forwarding_artifact_service_test.ts       # 56 passed
npx vitest run --project unit:dev dev/test/server/adk_api_server_test.ts \
  dev/test/cli/cli_run_test.ts                              # 59 passed
npx vitest run --project integration tests/integration/runner/runner_artifacts_test.ts  # 1 passed
npm run build && npm run lint && npm run format:check && npm run docs:check   # all clean

Coverage of the new module core/src/artifacts/artifact_filename.ts: 100% lines, 100% branches.

Proof the tests can fail. Each mutation was applied to the fixed code, and the suite was re-run:

Mutation Result
Restore cleanFilename = cleanFilename.trim(), keep the guard 143 passed. The trim is dead behind the guard, so its removal is cleanup, not the fix.
Remove the guard from getArtifactDir and restore the trim 10 failed on FileArtifactService. promise resolved "1" instead of rejecting (the original bug: a version appended to a.txt), expected { text: 'unpadded' } to be undefined.
Remove the guard from in_memory_artifact_service.ts 8 failed on InMemoryArtifactService: promise resolved "+0" instead of rejecting.
Remove the guard from gcs_artifact_service.ts 8 failed on GcsArtifactService: promise resolved "+0" instead of rejecting.
Make InMemoryArtifactService.saveArtifact synchronous again, so the guard throws instead of rejecting 8 failed: the error escapes at the call site (Artifact filename " padded.txt" must not have...) rather than rejecting the promise. This pins the async keyword.

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

Run the reproduction against the built package. GcsArtifactService needs a real bucket, so it is covered by the fake-bucket conformance suite instead.

// node repro.mjs, after `npm run build --workspace core`
import {
  FileArtifactService,
  InMemoryArtifactService,
} from './core/dist/esm/index.js';
const key = {appName: 'app', userId: 'u', sessionId: 's'};
await service.saveArtifact({
  ...key,
  filename: 'a.txt',
  artifact: {text: 'first'},
});
await service.saveArtifact({
  ...key,
  filename: ' a.txt',
  artifact: {text: 'second'},
});
await service.listVersions({...key, filename: 'a.txt'});

Result on both backends:

saveArtifact(' a.txt') -> Artifact filename " a.txt" must not have leading or trailing whitespace.
listVersions('a.txt')  -> [0]
loadArtifact('a.txt')  -> "first"
loadArtifact(' a.txt') -> undefined
listArtifactKeys()     -> ["a.txt"]

Before this change, FileArtifactService returned [0,1] and "second".

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 2 commits August 9, 2026 02:14
…ckends

FileArtifactService trimmed a filename before mapping it onto a directory,
so ' a.txt' and 'a.txt' addressed one artifact while the in-memory and GCS
backends kept them apart. A filename is a storage key, so the backends must
agree.

A filename becomes a directory name on the filesystem backend, and Windows
strips trailing spaces from a path component, so a padded name cannot be
stored distinctly there. Every backend now rejects a padded name on save,
and every read or delete path treats one as not found.
All three backends now report the same error first for the same input.
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