From 40e334b8d33864dd61696a75db88e3cd03145333 Mon Sep 17 00:00:00 2001 From: Kristen Pereira <26pkristen@gmail.com> Date: Wed, 19 Nov 2025 00:06:27 -0800 Subject: [PATCH 1/4] fix Windows path compatibility issues - Replace string slicing with os.path.dirname() in config_agent_utils.py for cross-platform path handling - Use pathlib.Path for path normalization in AgentLoader to handle mixed separators - Replace hardcoded '/' with os.sep in error messages for correct path display on Windows - Use os.path.join() for consistent path construction in error messages --- src/google/adk/agents/config_agent_utils.py | 2 +- src/google/adk/cli/utils/agent_loader.py | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/google/adk/agents/config_agent_utils.py b/src/google/adk/agents/config_agent_utils.py index 7982a9cf59..38ba2e2578 100644 --- a/src/google/adk/agents/config_agent_utils.py +++ b/src/google/adk/agents/config_agent_utils.py @@ -132,7 +132,7 @@ def resolve_agent_reference( else: return from_config( os.path.join( - referencing_agent_config_abs_path.rsplit("/", 1)[0], + os.path.dirname(referencing_agent_config_abs_path), ref_config.config_path, ) ) diff --git a/src/google/adk/cli/utils/agent_loader.py b/src/google/adk/cli/utils/agent_loader.py index 0755c9147c..cb63f763c0 100644 --- a/src/google/adk/cli/utils/agent_loader.py +++ b/src/google/adk/cli/utils/agent_loader.py @@ -56,7 +56,7 @@ class AgentLoader(BaseAgentLoader): """ def __init__(self, agents_dir: str): - self.agents_dir = agents_dir.rstrip("/") + self.agents_dir = str(Path(agents_dir)) self._original_sys_path = None self._agent_cache: dict[str, Union[BaseAgent, App]] = {} @@ -250,12 +250,13 @@ def _perform_load(self, agent_name: str) -> Union[BaseAgent, App]: f"No root_agent found for '{agent_name}'. Searched in" f" '{actual_agent_name}.agent.root_agent'," f" '{actual_agent_name}.root_agent' and" - f" '{actual_agent_name}/root_agent.yaml'.\n\nExpected directory" - f" structure:\n /\n {actual_agent_name}/\n " - " agent.py (with root_agent) OR\n root_agent.yaml\n\nThen run:" - f" adk web \n\nEnsure '{agents_dir}/{actual_agent_name}' is" - " structured correctly, an .env file can be loaded if present, and a" - f" root_agent is exposed.{hint}" + f" '{actual_agent_name}{os.sep}root_agent.yaml'.\n\nExpected directory" + f" structure:\n {os.sep}\n " + f" {actual_agent_name}{os.sep}\n agent.py (with root_agent) OR\n " + " root_agent.yaml\n\nThen run: adk web \n\nEnsure" + f" '{os.path.join(agents_dir, actual_agent_name)}' is structured" + " correctly, an .env file can be loaded if present, and a root_agent" + f" is exposed.{hint}" ) def _record_origin_metadata( From a83b1e3926e7d21f4d4e491a4f7f6e99d80e0bbe Mon Sep 17 00:00:00 2001 From: Kristen Pereira <26pkristen@gmail.com> Date: Wed, 19 Nov 2025 00:06:51 -0800 Subject: [PATCH 2/4] Add tests for Windows-style paths, mixed separators, and path normalization --- .../unittests/cli/utils/test_agent_loader.py | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/unittests/cli/utils/test_agent_loader.py b/tests/unittests/cli/utils/test_agent_loader.py index 5c66160aed..edf5f40b4d 100644 --- a/tests/unittests/cli/utils/test_agent_loader.py +++ b/tests/unittests/cli/utils/test_agent_loader.py @@ -887,3 +887,47 @@ def test_yaml_config_agents_dir_parameter(self): # Verify they are different agents assert default_agent.name != custom_agent.name assert explicit_agent.name == default_agent.name + + def test_windows_style_path_normalization_on_unix(self): + """ + Even on Unix, Windows-style path strings (with backslashes) should not break. + Path() on Unix treats backslashes as literal characters, not separators. + This test verifies that AgentLoader uses Path() consistently and safely. + """ + windows_path = "C:\\Dev\\adk\\" + + loader = AgentLoader(windows_path) + + # Path(...) must return a valid string and must not crash + assert isinstance(loader.agents_dir, str) + + # On Unix, Path("C:\\Dev\\adk\\") → "C:\\Dev\\adk" + # (Backslashes preserved, trailing slash removed) + assert loader.agents_dir == str(Path(windows_path)) + + def test_mixed_separators_are_handled(self): + """Windows users often accidentally mix / and \\ separators.""" + mixed_path = "C:\\Dev/adk\\subdir/" + + loader = AgentLoader(mixed_path) + + # Path resolves extraneous slashes + # The normalized path should not have trailing separators + assert not loader.agents_dir.endswith("/") + assert not loader.agents_dir.endswith("\\") + + def test_error_message_contains_correct_joined_paths(self): + """Test that error messages use os.path.join() for consistent path formatting.""" + with tempfile.TemporaryDirectory() as temp_dir: + loader = AgentLoader(temp_dir) + + # Try to load a nonexistent agent + agent_name = "missing_agent" + try: + loader.load_agent(agent_name) + except ValueError as e: + message = str(e) + + # Should contain the correct joined path using os.path.join() + expected_path = os.path.join(temp_dir, agent_name) + assert expected_path in message From 42f2191518dbb89895a4f153f117f34f000c65c5 Mon Sep 17 00:00:00 2001 From: Kristen Pereira <26pkristen@gmail.com> Date: Thu, 20 Nov 2025 00:41:50 -0800 Subject: [PATCH 3/4] test: add Windows path regression tests Add mock-based tests that simulate Windows behavior to catch path handling regressions --- tests/unittests/agents/test_agent_config.py | 91 ++++++++++++++++ .../unittests/cli/utils/test_agent_loader.py | 103 ++++++++++++------ 2 files changed, 162 insertions(+), 32 deletions(-) diff --git a/tests/unittests/agents/test_agent_config.py b/tests/unittests/agents/test_agent_config.py index c2300f5f5d..bdca5ac6ed 100644 --- a/tests/unittests/agents/test_agent_config.py +++ b/tests/unittests/agents/test_agent_config.py @@ -12,6 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import ntpath +import os from pathlib import Path from typing import Literal from typing import Type @@ -20,6 +22,7 @@ from google.adk.agents.agent_config import AgentConfig from google.adk.agents.base_agent import BaseAgent from google.adk.agents.base_agent_config import BaseAgentConfig +from google.adk.agents.common_configs import AgentRefConfig from google.adk.agents.llm_agent import LlmAgent from google.adk.agents.loop_agent import LoopAgent from google.adk.agents.parallel_agent import ParallelAgent @@ -280,3 +283,91 @@ class MyCustomAgentConfig(BaseAgentConfig): config.root.model_dump() ) assert my_custom_config.other_field == "other value" + + +def test_resolve_agent_reference_resolves_relative_paths(tmp_path: Path): + """Verify resolve_agent_reference correctly resolves relative sub-agent paths + based on the directory of the referencing config file, including nested dirs. + """ + + sub_agent_dir = tmp_path / "sub_agents" + sub_agent_dir.mkdir() + + (sub_agent_dir / "child.yaml").write_text(""" +agent_class: LlmAgent +name: child_agent +model: gemini-2.0-flash +instruction: I am a child agent +""") + + main_config = tmp_path / "main.yaml" + main_config.write_text(""" +agent_class: LlmAgent +name: main_agent +model: gemini-2.0-flash +instruction: I am the main agent +sub_agents: + - config_path: sub_agents/child.yaml +""") + + ref_config = AgentRefConfig(config_path="sub_agents/child.yaml") + agent = config_agent_utils.resolve_agent_reference( + ref_config, str(main_config) + ) + assert agent.name == "child_agent" + + main_config_abs = str(main_config.resolve()) + dirname = os.path.dirname(main_config_abs) + assert dirname == str(tmp_path.resolve()) + assert os.path.exists(os.path.join(dirname, "sub_agents", "child.yaml")) + + nested_dir = tmp_path / "level1" / "level2" + nested_dir.mkdir(parents=True) + nested_sub_dir = nested_dir / "sub" + nested_sub_dir.mkdir() + + (nested_sub_dir / "nested_child.yaml").write_text(""" +agent_class: LlmAgent +name: nested_child +model: gemini-2.0-flash +instruction: I am nested +""") + + (nested_dir / "nested_main.yaml").write_text(""" +agent_class: LlmAgent +name: nested_main +model: gemini-2.0-flash +instruction: I reference a nested child +sub_agents: + - config_path: sub/nested_child.yaml +""") + + ref_nested = AgentRefConfig(config_path="sub/nested_child.yaml") + agent_nested = config_agent_utils.resolve_agent_reference( + ref_nested, str(nested_dir / "nested_main.yaml") + ) + assert agent_nested.name == "nested_child" + + +def test_resolve_agent_reference_uses_windows_dirname(monkeypatch): + """Ensure Windows-style config references resolve via os.path.dirname.""" + ref_config = AgentRefConfig(config_path="sub\\child.yaml") + recorded: dict[str, str] = {} + + def fake_from_config(path: str): + recorded["path"] = path + return "sentinel" + + monkeypatch.setattr( + config_agent_utils, "from_config", fake_from_config, raising=False + ) + monkeypatch.setattr(config_agent_utils.os, "path", ntpath, raising=False) + + referencing = r"C:\workspace\agents\main.yaml" + result = config_agent_utils.resolve_agent_reference(ref_config, referencing) + + expected_path = ntpath.join( + ntpath.dirname(referencing), ref_config.config_path + ) + assert result == "sentinel" + assert recorded["path"] == expected_path diff --git a/tests/unittests/cli/utils/test_agent_loader.py b/tests/unittests/cli/utils/test_agent_loader.py index edf5f40b4d..883d048894 100644 --- a/tests/unittests/cli/utils/test_agent_loader.py +++ b/tests/unittests/cli/utils/test_agent_loader.py @@ -12,12 +12,15 @@ # See the License for the specific language governing permissions and # limitations under the License. +import ntpath import os from pathlib import Path +from pathlib import PureWindowsPath import sys import tempfile from textwrap import dedent +from google.adk.cli.utils import agent_loader as agent_loader_module from google.adk.cli.utils.agent_loader import AgentLoader from pydantic import ValidationError import pytest @@ -280,6 +283,46 @@ def test_load_multiple_different_agents(self): assert agent2 is not agent3 assert agent1.agent_id != agent2.agent_id != agent3.agent_id + def test_error_messages_use_os_sep_consistently(self): + """Verify error messages use os.sep instead of hardcoded '/'.""" + with tempfile.TemporaryDirectory() as temp_dir: + loader = AgentLoader(temp_dir) + agent_name = "missing_agent" + + try: + loader.load_agent(agent_name) + except ValueError as e: + message = str(e) + expected_path = os.path.join(temp_dir, agent_name) + + assert expected_path in message + assert f"{agent_name}{os.sep}root_agent.yaml" in message + assert f"{os.sep}" in message + + def test_agent_loader_with_mocked_windows_path(self, monkeypatch): + """Mock Path() to simulate Windows behavior and catch regressions. + + REGRESSION TEST: Fails with rstrip('/'), passes with str(Path()). + """ + windows_path = "C:\\Users\\dev\\agents\\" + + def mock_path_constructor(path_str): + class MockPath: + + def __str__(self): + return str(PureWindowsPath(path_str)) + + return MockPath() + + with monkeypatch.context() as m: + m.setattr("google.adk.cli.utils.agent_loader.Path", mock_path_constructor) + loader = AgentLoader(windows_path) + + expected = str(PureWindowsPath(windows_path)) + assert loader.agents_dir == expected + assert not loader.agents_dir.endswith("\\") + assert not loader.agents_dir.endswith("/") + def test_agent_not_found_error(self): """Test that appropriate error is raised when agent is not found.""" with tempfile.TemporaryDirectory() as temp_dir: @@ -888,46 +931,42 @@ def test_yaml_config_agents_dir_parameter(self): assert default_agent.name != custom_agent.name assert explicit_agent.name == default_agent.name - def test_windows_style_path_normalization_on_unix(self): - """ - Even on Unix, Windows-style path strings (with backslashes) should not break. - Path() on Unix treats backslashes as literal characters, not separators. - This test verifies that AgentLoader uses Path() consistently and safely. - """ + def test_windows_style_path_normalization_uses_path_equivalent( + self, monkeypatch + ): + """AgentLoader should normalize Windows paths exactly like pathlib on Windows.""" + monkeypatch.setattr(agent_loader_module, "Path", PureWindowsPath) windows_path = "C:\\Dev\\adk\\" loader = AgentLoader(windows_path) - # Path(...) must return a valid string and must not crash - assert isinstance(loader.agents_dir, str) + assert loader.agents_dir == str(PureWindowsPath(windows_path)) - # On Unix, Path("C:\\Dev\\adk\\") → "C:\\Dev\\adk" - # (Backslashes preserved, trailing slash removed) - assert loader.agents_dir == str(Path(windows_path)) - - def test_mixed_separators_are_handled(self): - """Windows users often accidentally mix / and \\ separators.""" - mixed_path = "C:\\Dev/adk\\subdir/" + def test_windows_mixed_separators_collapse_to_consistent_form( + self, monkeypatch + ): + """Mixed / and \\ from Windows paths should normalize to PureWindowsPath output.""" + monkeypatch.setattr(agent_loader_module, "Path", PureWindowsPath) + mixed_path = r"C:\Dev/adk\subdir/" loader = AgentLoader(mixed_path) - # Path resolves extraneous slashes - # The normalized path should not have trailing separators - assert not loader.agents_dir.endswith("/") - assert not loader.agents_dir.endswith("\\") + assert loader.agents_dir == str(PureWindowsPath(mixed_path)) - def test_error_message_contains_correct_joined_paths(self): - """Test that error messages use os.path.join() for consistent path formatting.""" - with tempfile.TemporaryDirectory() as temp_dir: - loader = AgentLoader(temp_dir) + def test_missing_root_agent_error_uses_windows_separators( + self, tmp_path, monkeypatch + ): + """Error guidance should honor os.sep/os.path.join so Windows users see backslashes.""" + monkeypatch.setattr(agent_loader_module.os, "sep", "\\", raising=False) + monkeypatch.setattr( + agent_loader_module.os.path, "join", ntpath.join, raising=False + ) + loader = AgentLoader(str(tmp_path)) - # Try to load a nonexistent agent - agent_name = "missing_agent" - try: - loader.load_agent(agent_name) - except ValueError as e: - message = str(e) + with pytest.raises(ValueError) as exc_info: + loader.load_agent("missing_agent") - # Should contain the correct joined path using os.path.join() - expected_path = os.path.join(temp_dir, agent_name) - assert expected_path in message + message = str(exc_info.value) + assert "missing_agent\\root_agent.yaml" in message + expected_path = ntpath.join(str(tmp_path), "missing_agent") + assert expected_path in message From 8cb0310bd4450097a0a7714eaac6521b6a447442 Mon Sep 17 00:00:00 2001 From: Kristen Pereira <26pkristen@gmail.com> Date: Thu, 20 Nov 2025 00:53:58 -0800 Subject: [PATCH 4/4] remove redundant tests --- .../unittests/cli/utils/test_agent_loader.py | 40 ------------------- 1 file changed, 40 deletions(-) diff --git a/tests/unittests/cli/utils/test_agent_loader.py b/tests/unittests/cli/utils/test_agent_loader.py index 883d048894..d88980984d 100644 --- a/tests/unittests/cli/utils/test_agent_loader.py +++ b/tests/unittests/cli/utils/test_agent_loader.py @@ -930,43 +930,3 @@ def test_yaml_config_agents_dir_parameter(self): # Verify they are different agents assert default_agent.name != custom_agent.name assert explicit_agent.name == default_agent.name - - def test_windows_style_path_normalization_uses_path_equivalent( - self, monkeypatch - ): - """AgentLoader should normalize Windows paths exactly like pathlib on Windows.""" - monkeypatch.setattr(agent_loader_module, "Path", PureWindowsPath) - windows_path = "C:\\Dev\\adk\\" - - loader = AgentLoader(windows_path) - - assert loader.agents_dir == str(PureWindowsPath(windows_path)) - - def test_windows_mixed_separators_collapse_to_consistent_form( - self, monkeypatch - ): - """Mixed / and \\ from Windows paths should normalize to PureWindowsPath output.""" - monkeypatch.setattr(agent_loader_module, "Path", PureWindowsPath) - mixed_path = r"C:\Dev/adk\subdir/" - - loader = AgentLoader(mixed_path) - - assert loader.agents_dir == str(PureWindowsPath(mixed_path)) - - def test_missing_root_agent_error_uses_windows_separators( - self, tmp_path, monkeypatch - ): - """Error guidance should honor os.sep/os.path.join so Windows users see backslashes.""" - monkeypatch.setattr(agent_loader_module.os, "sep", "\\", raising=False) - monkeypatch.setattr( - agent_loader_module.os.path, "join", ntpath.join, raising=False - ) - loader = AgentLoader(str(tmp_path)) - - with pytest.raises(ValueError) as exc_info: - loader.load_agent("missing_agent") - - message = str(exc_info.value) - assert "missing_agent\\root_agent.yaml" in message - expected_path = ntpath.join(str(tmp_path), "missing_agent") - assert expected_path in message