Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
FileArtifactServiceacceptsproject/versions/readme.txtas a separate artifact, but stores it insideproject's privateversions/directory. The save succeeds and the child can be loaded directly, yet it is absent from artifact listings. Deleting the artifact namedprojectthen deletes the separately saved child as well. Ordinary nested names such asproject/releases/readme.txtare 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:The shared save path now rejects an exact
versionsfilename 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
Full
tox --recreateran the repository's unchangedpytest tests/unittestscommand in each environment, with overall exit 0: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
322e3bf0ae4896f44ce4589926c1daa930c781b5with 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 webCLI 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:
Use
/apps/example/users/owner/sessions/session/artifactsas 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:Formatting and types
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.