Skip to content

Commit 8a2a948

Browse files
cyrus-mzGWeale
authored andcommitted
fix: Windows file artifact URI path normalization
Merge #5523 Closes #5486 ## Summary This PR fixes Windows handling of local `file://` artifact URIs. Both `google/adk/cli/service_registry.py` and `google/adk/artifacts/file_artifact_service.py` were passing URI-style paths into `Path(...)` without Windows URI-to-filesystem normalization. On Windows, this can produce malformed drive-relative paths such as `C:foo` instead of absolute paths like `C:\foo`. This change applies `url2pathname()` under `os.name == "nt"` in both locations so canonical file URIs like `file:///C:/...` are converted to proper native Windows paths before constructing `Path(...)`. ## Validation Manually tested on Windows with: 1. current working directory on `C:` and artifact target on `C:` - before the patch, the bug reproduced and the artifact root was created in the wrong location - after the patch, the artifact root was created in the intended location 2. current working directory on `C:` and artifact target on `D:` - before the patch, the current code could appear to work - after the patch, it still worked, now through correct URI-to-path normalization 3. paths containing spaces - after the patch, the artifact root was created correctly in the intended directory I also verified the drive-relative behavior separately with: ```python import ntpath print(ntpath.abspath("C:foo")) print(ntpath.abspath("D:foo")) ``` With current working directory `C:\Users\user1\projects\agent1`, this produced: ```text C:\Users\user1\projects\agent1\foo D:\foo ``` This matches the observed issue: without Windows-specific normalization, the malformed URI-derived path can behave like a drive-relative path such as `C:foo` instead of a proper absolute path like `C:\foo`. ## Notes I have not added a regression test in this PR yet. I can add one in a follow-up if maintainers prefer a specific test location for this path-conversion logic. Co-authored-by: George Weale <gweale@google.com> COPYBARA_INTEGRATE_REVIEW=#5523 from cyrus-mz:fix/windows-file-artifact-uri e93847b PiperOrigin-RevId: 943367471
1 parent ade8577 commit 8a2a948

4 files changed

Lines changed: 67 additions & 2 deletions

File tree

src/google/adk/artifacts/file_artifact_service.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
from typing import Union
2626
from urllib.parse import unquote
2727
from urllib.parse import urlparse
28+
from urllib.request import url2pathname
2829

2930
from google.genai import types
3031
from pydantic import alias_generators
@@ -59,7 +60,10 @@ def _file_uri_to_path(uri: str) -> Optional[Path]:
5960
parsed = urlparse(uri)
6061
if parsed.scheme != "file":
6162
return None
62-
return Path(unquote(parsed.path))
63+
path_str = unquote(parsed.path)
64+
if os.name == "nt":
65+
path_str = url2pathname(path_str)
66+
return Path(path_str)
6367

6468

6569
_USER_NAMESPACE_PREFIX = "user:"

src/google/adk/cli/service_registry.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ def my_session_factory(uri: str, **kwargs):
7272
from typing import Protocol
7373
from urllib.parse import unquote
7474
from urllib.parse import urlparse
75+
from urllib.request import url2pathname
7576

7677
from ..artifacts.base_artifact_service import BaseArtifactService
7778
from ..memory.base_memory_service import BaseMemoryService
@@ -310,7 +311,12 @@ def file_artifact_factory(uri: str, **_):
310311
)
311312
if not parsed_uri.path:
312313
raise ValueError("file:// artifact URIs must include a path component.")
313-
artifact_path = Path(unquote(parsed_uri.path))
314+
315+
artifact_path_str = unquote(parsed_uri.path)
316+
if os.name == "nt":
317+
artifact_path_str = url2pathname(artifact_path_str)
318+
319+
artifact_path = Path(artifact_path_str)
314320
return FileArtifactService(root_dir=artifact_path)
315321

316322
registry.register_artifact_service("memory", memory_artifact_factory)

tests/unittests/artifacts/test_artifact_service.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import enum
2121
import json
2222
from pathlib import Path
23+
from types import SimpleNamespace
2324
from typing import Any
2425
from typing import Optional
2526
from typing import Union
@@ -28,6 +29,7 @@
2829
from urllib.parse import unquote
2930
from urllib.parse import urlparse
3031

32+
from google.adk.artifacts import file_artifact_service
3133
from google.adk.artifacts.base_artifact_service import ArtifactVersion
3234
from google.adk.artifacts.base_artifact_service import ensure_part
3335
from google.adk.artifacts.file_artifact_service import FileArtifactService
@@ -1591,3 +1593,25 @@ async def test_save_load_empty_text_artifact(
15911593
assert loaded is not None
15921594
assert loaded.text == ""
15931595
assert loaded.inline_data is None
1596+
1597+
1598+
def test_file_uri_to_path_normalizes_windows_file_uri(monkeypatch):
1599+
monkeypatch.setattr(file_artifact_service, "os", SimpleNamespace(name="nt"))
1600+
mocked_url2pathname = mock.Mock(return_value=r"C:\tmp\adk artifacts")
1601+
monkeypatch.setattr(
1602+
file_artifact_service, "url2pathname", mocked_url2pathname
1603+
)
1604+
1605+
result = file_artifact_service._file_uri_to_path(
1606+
"file:///C:/tmp/adk%20artifacts"
1607+
)
1608+
1609+
mocked_url2pathname.assert_called_once_with("/C:/tmp/adk artifacts")
1610+
assert result == Path(r"C:\tmp\adk artifacts")
1611+
1612+
1613+
def test_file_uri_to_path_returns_none_for_non_file_uri():
1614+
assert (
1615+
file_artifact_service._file_uri_to_path("gs://bucket/adk_artifacts")
1616+
is None
1617+
)

tests/unittests/cli/test_service_registry.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,12 @@
1212
# See the License for the specific language governing permissions and
1313
# limitations under the License.
1414

15+
from pathlib import Path
16+
from types import SimpleNamespace
17+
from unittest import mock
1518
from unittest.mock import patch
1619

20+
from google.adk.cli import service_registry
1721
import pytest
1822

1923

@@ -124,6 +128,33 @@ def test_create_artifact_service_gcs(registry, mock_services):
124128
)
125129

126130

131+
def test_file_artifact_factory_normalizes_windows_file_uri(monkeypatch):
132+
monkeypatch.setattr(service_registry, "os", SimpleNamespace(name="nt"))
133+
mocked_url2pathname = mock.Mock(return_value=r"C:\tmp\adk artifacts")
134+
monkeypatch.setattr(service_registry, "url2pathname", mocked_url2pathname)
135+
136+
registry = service_registry.ServiceRegistry()
137+
service_registry._register_builtin_services(registry)
138+
139+
with mock.patch(
140+
"google.adk.artifacts.file_artifact_service.FileArtifactService"
141+
) as mock_file_artifact_service:
142+
registry.create_artifact_service("file:///C:/tmp/adk%20artifacts")
143+
144+
mocked_url2pathname.assert_called_once_with("/C:/tmp/adk artifacts")
145+
mock_file_artifact_service.assert_called_once_with(
146+
root_dir=Path(r"C:\tmp\adk artifacts")
147+
)
148+
149+
150+
def test_file_artifact_factory_rejects_non_local_authority():
151+
registry = service_registry.ServiceRegistry()
152+
service_registry._register_builtin_services(registry)
153+
154+
with pytest.raises(ValueError, match="local filesystem"):
155+
registry.create_artifact_service("file://example.com/tmp/adk_artifacts")
156+
157+
127158
# Memory Service Tests
128159
@patch("google.adk.cli.utils.envs.load_dotenv_for_agent")
129160
def test_create_memory_service_rag(

0 commit comments

Comments
 (0)