Skip to content

fix(artifacts): prevent filenames from colliding with version storage - #7122

Open
baba9811 wants to merge 1 commit into
google:mainfrom
baba9811:fix/file-artifact-versions-path
Open

baba9811 wants to merge 1 commit into
google:mainfrom
baba9811:fix/file-artifact-versions-path

Conversation

@baba9811

Copy link
Copy Markdown
Contributor

Description of Change

FileArtifactService accepts project/versions/readme.txt as a separate artifact, but stores it inside project's private versions/ directory. The save succeeds and the child can be loaded directly, yet it is absent from artifact listings. Deleting the artifact named project then deletes the separately saved child as well. Ordinary nested names such as project/releases/readme.txt are already preserved by the service.

Reproduction on main b8de4266d90fb9a6500079d062561cfdd4cf21d6 (package version 2.9.0), Python 3.12.10, macOS. No model or LiteLLM is involved:

import asyncio
from tempfile import TemporaryDirectory

from google.adk.artifacts.file_artifact_service import FileArtifactService
from google.genai.types import Part


async def reproduce():
    with TemporaryDirectory() as root:
        service = FileArtifactService(root)
        scope = dict(app_name="app", user_id="owner", session_id="session")
        child = "project/versions/readme.txt"
        for name in ["project", child]:
            await service.save_artifact(
                **scope, filename=name, artifact=Part(text=name)
            )
        print(await service.list_artifact_keys(**scope))  # ['project']
        print(await service.load_artifact(**scope, filename=child) is not None)  # True
        await service.delete_artifact(**scope, filename="project")
        print(await service.load_artifact(**scope, filename=child))  # None


asyncio.run(reproduce())

The shared save path now rejects an exact versions filename component in any casing before writing. It reuses the service's namespace and separator normalization, so the same validation covers user-scoped and Windows-style names. The existing API handler returns HTTP 400 for this validation error.

Read and delete access to existing records remains available. This prevents new collisions; it does not migrate or recover existing ambiguous data. Ordinary nested filenames and other storage backends retain their behavior. The save docstring and the companion guide change in google/adk-docs#2236 describe the restriction.

Testing Plan

Unit and regression tests

  • New regression tests failed on unchanged source: six direct-service cases accepted unsafe names, and the HTTP case returned 200. After the fix, all twelve focused cases passed.
  • Artifact and FastAPI suites: 1,112 passed, five skipped, one xfailed. Existing dependency/deprecation/experimental/OpenAPI warnings remain.
  • The original reproduction matrix now rejects all six unsafe file saves; fourteen supported file/memory controls pass across scopes and save order.

Full tox --recreate ran the repository's unchanged pytest tests/unittests command in each environment, with overall exit 0:

Python Passed Skipped Xfailed Xpassed
3.10.19 14,843 87 27 2
3.11.14 14,850 86 27 2
3.12.10 14,843 87 27 2
3.13.3 14,843 87 27 2
3.14.7 14,843 87 27 2

These runs were on macOS arm64, with managed interpreters, gcloud excluded from the test process PATH, and stdin closed to avoid an existing interactive-login path. Dependency/deprecation/experimental warnings remain. Linux CI and alternate A2A/MCP dependency matrices have not been run locally.

Before submission, main advanced to 322e3bf0ae4896f44ce4589926c1daa930c781b5 with an unrelated RemoteA2aAgent change. The bug still reproduces there. Applying this exact patch in a separate verification checkout passed the artifact, FastAPI, and RemoteA2aAgent suites: 1,219 passed, five skipped, one xfailed. The full tox results above are for this PR commit on its original base.

Manual E2E

Built and installed the wheel in a separate environment, then started the actual adk web CLI with a file artifact service and memory session/memory services. No model call or cloud credentials were needed. Before the fix the unsafe save returned 200.

To repeat the server check after installing the locally built wheel:

scratch=$(mktemp -d)
mkdir "$scratch/agents"
adk web "$scratch/agents" --host 127.0.0.1 --port 8000 --no-reload \
  --session_service_uri memory:// \
  --artifact_service_uri "file://$scratch/artifacts" \
  --memory_service_uri memory://

Use /apps/example/users/owner/sessions/session/artifacts as the REST base. POST each filename below using a body such as {"filename": "project", "artifact": {"text": "payload"}}, list the keys, DELETE /project, then GET /project/releases/readme.txt/versions/0:

POST project                              200
POST project/versions/readme.txt          400
POST project/releases/readme.txt          200
GET artifact keys                        ['project', 'project/releases/readme.txt']
DELETE project                           200
GET project/releases/readme.txt/versions/0 200
  • Real Context and forwarding/per-agent storage checks confirmed rejected saves do not record artifact deltas and ordinary nested artifacts survive parent-artifact deletion.

Formatting and types

  • Changed-file pre-commit passed. Mypy 2.3.1 on Python 3.12.10 reported the same 845 baseline errors on both revisions, with no new normalized diagnostics.
  • Full pre-commit exits 1 on both unchanged main and this branch because pyink removes the same pre-existing blank line in tests/unittests/plugins/test_bigquery_agent_analytics_plugin.py. Every other hook passes. That unrelated formatter edit is excluded from this PR. Mypy is likewise a baseline comparison, not a clean checker pass; other CI interpreter/type-check combinations were not rerun locally.

Compatibility and release handling

This rejects previously accepted FileArtifactService filenames, including attempts to save another version under an existing unsafe name. Future saves require a safe filename, while existing reads and deletes remain available. Given the API stability guidance, should this ship as corrective validation, or be staged through deprecation or a major release? The before/after data-loss evidence and documentation change are included to support that decision.

File artifact names could overlap the backend's private versions directory, hiding saved data and exposing it to deletion with another artifact. Reject the reserved path component before creating artifact directories while preserving legacy read and delete access.
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.

2 participants