Skip to content

Commit 08bcf76

Browse files
committed
fix(tools): prefix on_tools_listing_error fallback tools
Keep the BaseToolset naming contract on the isolate-and-continue path by routing listing-error replacements through the shared prefix helper.
1 parent 5a342a6 commit 08bcf76

6 files changed

Lines changed: 159 additions & 35 deletions

File tree

src/google/adk/agents/llm_agent.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,9 @@ async def _convert_tool_union_to_tools(
228228
e,
229229
exc_info=True,
230230
)
231-
return await tool_union.on_tools_listing_error(e, ctx)
231+
return tool_union._apply_tool_name_prefix(
232+
await tool_union.on_tools_listing_error(e, ctx)
233+
)
232234

233235

234236
# TODO: drop the explicit abc.ABC base once BaseNode surfaces ABCMeta to

src/google/adk/tools/base_toolset.py

Lines changed: 40 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,10 @@ async def on_tools_listing_error(
117117
original exception; this hook is only invoked by the framework's
118118
isolate-and-continue paths.
119119
120+
Framework callers apply ``tool_name_prefix`` to the returned tools the same
121+
way ``get_tools_with_prefix`` would, so overrides should return unprefixed
122+
tool names.
123+
120124
Args:
121125
error: The exception raised by ``get_tools`` / ``get_tools_with_prefix``.
122126
readonly_context: The same context passed to the failed listing call.
@@ -128,52 +132,27 @@ async def on_tools_listing_error(
128132
del error, readonly_context
129133
return []
130134

131-
@final
132-
async def get_tools_with_prefix(
133-
self,
134-
readonly_context: Optional[ReadonlyContext] = None,
135-
) -> list[BaseTool]:
136-
"""Return all tools with optional prefix applied to tool names.
137-
138-
This method calls get_tools() and applies prefixing if tool_name_prefix is provided.
135+
def _apply_tool_name_prefix(self, tools: list[BaseTool]) -> list[BaseTool]:
136+
"""Applies ``tool_name_prefix`` to tools, matching ``get_tools_with_prefix``.
139137
140138
Args:
141-
readonly_context (ReadonlyContext, optional): Context used to filter tools
142-
available to the agent. If None, all tools in the toolset are returned.
139+
tools: Tools with unprefixed names (as returned by ``get_tools`` or
140+
``on_tools_listing_error``).
143141
144142
Returns:
145-
list[BaseTool]: A list of tools with prefixed names if tool_name_prefix is provided.
143+
The same tools when no prefix is configured; otherwise shallow copies
144+
with prefixed ``name`` / declaration names.
146145
"""
147-
invocation_id = readonly_context.invocation_id if readonly_context else None
148-
149-
if (
150-
self._use_invocation_cache
151-
and self._cached_prefixed_tools is not None
152-
and self._cached_invocation_id == invocation_id
153-
):
154-
return self._cached_prefixed_tools
155-
156-
tools = await self.get_tools(readonly_context)
157-
158146
if not self.tool_name_prefix:
159-
self._cached_invocation_id = invocation_id
160-
self._cached_prefixed_tools = tools
161147
return tools
162148

163149
prefix = self.tool_name_prefix
164-
165-
# Create copies of tools to avoid modifying original instances
166150
prefixed_tools = []
167151
for tool in tools:
168-
# Create a shallow copy of the tool
169152
tool_copy = copy.copy(tool)
170-
171-
# Apply prefix to the copied tool
172-
prefixed_name = f"{prefix}_{tool.name}"
153+
prefixed_name = f'{prefix}_{tool.name}'
173154
tool_copy.name = prefixed_name
174155

175-
# Also update the function declaration name if the tool has one
176-
# Use default parameters to capture the current values in the closure
177156
def _create_prefixed_declaration(
178157
original_get_declaration=tool._get_declaration,
179158
prefixed_name=prefixed_name,
@@ -189,6 +168,35 @@ def _get_prefixed_declaration():
189168

190169
tool_copy._get_declaration = _create_prefixed_declaration()
191170
prefixed_tools.append(tool_copy)
171+
return prefixed_tools
172+
173+
@final
174+
async def get_tools_with_prefix(
175+
self,
176+
readonly_context: Optional[ReadonlyContext] = None,
177+
) -> list[BaseTool]:
178+
"""Return all tools with optional prefix applied to tool names.
179+
180+
This method calls get_tools() and applies prefixing if tool_name_prefix is provided.
181+
182+
Args:
183+
readonly_context (ReadonlyContext, optional): Context used to filter tools
184+
available to the agent. If None, all tools in the toolset are returned.
185+
186+
Returns:
187+
list[BaseTool]: A list of tools with prefixed names if tool_name_prefix is provided.
188+
"""
189+
invocation_id = readonly_context.invocation_id if readonly_context else None
190+
191+
if (
192+
self._use_invocation_cache
193+
and self._cached_prefixed_tools is not None
194+
and self._cached_invocation_id == invocation_id
195+
):
196+
return self._cached_prefixed_tools
197+
198+
tools = await self.get_tools(readonly_context)
199+
prefixed_tools = self._apply_tool_name_prefix(tools)
192200

193201
self._cached_invocation_id = invocation_id
194202
self._cached_prefixed_tools = prefixed_tools

src/google/adk/tools/skill_toolset.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1316,8 +1316,9 @@ async def _resolve_additional_tools_from_state(
13161316
)
13171317
# Let the toolset replace a failed listing (default: contribute
13181318
# nothing) so apps can surface expected auth failures as tools.
1319-
ts_tools = await toolset.on_tools_listing_error(
1320-
ts_tools, readonly_context
1319+
# Apply the same prefix contract as get_tools_with_prefix.
1320+
ts_tools = toolset._apply_tool_name_prefix(
1321+
await toolset.on_tools_listing_error(ts_tools, readonly_context)
13211322
)
13221323
if isinstance(ts_tools, BaseException):
13231324
raise ts_tools

tests/unittests/agents/test_llm_agent_fields.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -675,6 +675,40 @@ async def on_tools_listing_error(self, error, readonly_context=None):
675675
assert tools[0].name == 'connect_mcp_server'
676676
assert any('failed to load' in r.getMessage() for r in caplog.records)
677677

678+
async def test_canonical_tools_prefixes_on_tools_listing_error_fallback(
679+
self, caplog
680+
):
681+
"""Listing-error fallback tools honor the toolset tool_name_prefix."""
682+
from google.adk.tools.base_tool import BaseTool
683+
from google.adk.tools.base_toolset import BaseToolset
684+
685+
class AuthPromptToolset(BaseToolset):
686+
687+
async def get_tools(self, readonly_context=None):
688+
raise ConnectionError('HTTP 401 Unauthorized')
689+
690+
async def on_tools_listing_error(self, error, readonly_context=None):
691+
del error, readonly_context
692+
tool = mock.MagicMock(spec=BaseTool)
693+
tool.name = 'connect_mcp_server'
694+
tool.description = 'Authorize access'
695+
tool._get_declaration = mock.MagicMock(return_value=None)
696+
return [tool]
697+
698+
agent = LlmAgent(
699+
name='test_agent',
700+
model='gemini-pro',
701+
tools=[AuthPromptToolset(tool_name_prefix='books')],
702+
)
703+
ctx = await _create_readonly_context(agent)
704+
705+
with caplog.at_level(logging.ERROR, logger='google_adk'):
706+
tools = await agent.canonical_tools(ctx)
707+
708+
assert len(tools) == 1
709+
assert tools[0].name == 'books_connect_mcp_server'
710+
assert any('failed to load' in r.getMessage() for r in caplog.records)
711+
678712

679713
# Tests for multi-provider model support via string model names
680714
@pytest.mark.parametrize(

tests/unittests/tools/test_base_toolset.py

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -485,3 +485,36 @@ async def on_tools_listing_error(
485485
)
486486
assert len(tools) == 1
487487
assert tools[0].name == 'connect_mcp'
488+
489+
490+
@pytest.mark.asyncio
491+
async def test_on_tools_listing_error_fallback_respects_tool_name_prefix():
492+
"""Fallback tools from the listing-error hook receive tool_name_prefix."""
493+
494+
class _AuthPromptToolset(_TestingToolset):
495+
496+
async def get_tools(
497+
self, readonly_context: Optional[ReadonlyContext] = None
498+
) -> list[BaseTool]:
499+
del readonly_context
500+
raise ConnectionError('HTTP 401')
501+
502+
async def on_tools_listing_error(
503+
self,
504+
error: Exception,
505+
readonly_context: Optional[ReadonlyContext] = None,
506+
) -> list[BaseTool]:
507+
del error, readonly_context
508+
return [
509+
_TestingTool(
510+
name='connect_mcp',
511+
description='authorize MCP access',
512+
)
513+
]
514+
515+
toolset = _AuthPromptToolset(tool_name_prefix='mcp')
516+
tools = toolset._apply_tool_name_prefix(
517+
await toolset.on_tools_listing_error(ConnectionError('HTTP 401'))
518+
)
519+
assert len(tools) == 1
520+
assert tools[0].name == 'mcp_connect_mcp'

tests/unittests/tools/test_skill_toolset.py

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2400,11 +2400,13 @@ async def test_skill_toolset_resolution_isolates_failing_toolset(
24002400
failing_toolset = mock.create_autospec(
24012401
skill_toolset.BaseToolset, instance=True
24022402
)
2403+
failing_toolset.tool_name_prefix = None
24032404
failing_toolset.get_tools_with_prefix.side_effect = RuntimeError(
24042405
"MCP server unreachable"
24052406
)
24062407
# Default hook contributes nothing — same as historical skip behavior.
24072408
failing_toolset.on_tools_listing_error = mock.AsyncMock(return_value=[])
2409+
failing_toolset._apply_tool_name_prefix.side_effect = lambda tools: tools
24082410

24092411
toolset = skill_toolset.SkillToolset(
24102412
[mock_skill1],
@@ -2446,12 +2448,18 @@ async def test_skill_toolset_resolution_uses_on_tools_listing_error_hook(
24462448
failing_toolset = mock.create_autospec(
24472449
skill_toolset.BaseToolset, instance=True
24482450
)
2451+
failing_toolset.tool_name_prefix = None
24492452
failing_toolset.get_tools_with_prefix.side_effect = ConnectionError(
24502453
"HTTP 401 Unauthorized"
24512454
)
24522455
failing_toolset.on_tools_listing_error = mock.AsyncMock(
24532456
return_value=[placeholder]
24542457
)
2458+
failing_toolset._apply_tool_name_prefix.side_effect = (
2459+
lambda tools: skill_toolset.BaseToolset._apply_tool_name_prefix(
2460+
failing_toolset, tools
2461+
)
2462+
)
24552463

24562464
toolset = skill_toolset.SkillToolset(
24572465
[mock_skill1],
@@ -2470,6 +2478,44 @@ async def test_skill_toolset_resolution_uses_on_tools_listing_error_hook(
24702478
assert "Skipping toolset" in caplog.text
24712479

24722480

2481+
@pytest.mark.asyncio
2482+
async def test_skill_toolset_resolution_prefixes_listing_error_fallback(
2483+
mock_skill1, caplog
2484+
):
2485+
"""Skill additional-tool fallbacks from listing errors honor tool_name_prefix."""
2486+
mock_skill1.frontmatter.metadata = {
2487+
"adk_additional_tools": ["oauth_connect_mcp_server"]
2488+
}
2489+
mock_skill1.name = "skill1"
2490+
2491+
class _AuthPromptToolset(skill_toolset.BaseToolset):
2492+
2493+
async def get_tools(self, readonly_context=None):
2494+
raise ConnectionError("HTTP 401 Unauthorized")
2495+
2496+
async def on_tools_listing_error(self, error, readonly_context=None):
2497+
del error, readonly_context
2498+
placeholder = mock.create_autospec(skill_toolset.BaseTool, instance=True)
2499+
placeholder.name = "connect_mcp_server"
2500+
placeholder._get_declaration = mock.MagicMock(return_value=None)
2501+
return [placeholder]
2502+
2503+
toolset = skill_toolset.SkillToolset(
2504+
[mock_skill1],
2505+
additional_tools=[_AuthPromptToolset(tool_name_prefix="oauth")],
2506+
)
2507+
ctx = _make_tool_context_with_agent()
2508+
2509+
load_tool = skill_toolset.LoadSkillTool(toolset)
2510+
await load_tool.run_async(args={"skill_name": "skill1"}, tool_context=ctx)
2511+
2512+
with caplog.at_level(logging.WARNING):
2513+
tools = await toolset.get_tools(readonly_context=ctx)
2514+
2515+
assert "oauth_connect_mcp_server" in {t.name for t in tools}
2516+
assert "Skipping toolset" in caplog.text
2517+
2518+
24732519
@pytest.mark.asyncio
24742520
async def test_skill_toolset_resolution_propagates_system_exceptions(
24752521
mock_skill1,

0 commit comments

Comments
 (0)