fix(bedrock): sanitize tool names to satisfy Bedrock Converse API constraint#1730
fix(bedrock): sanitize tool names to satisfy Bedrock Converse API constraint#1730mesutoezdil wants to merge 3 commits intokagent-dev:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Bedrock Converse integration to sanitize MCP tool names to meet Bedrock’s [a-zA-Z0-9_-]+ constraint, while preserving original tool names in the ADK-facing responses.
Changes:
- Add
_sanitize_tool_name()and apply it when converting tool specs for Bedrock. - Thread a tool-name mapping through message-history conversion so prior
toolUseblocks can use sanitized names. - Remap Bedrock-returned sanitized tool names back to original names in
generate_content_async(), with accompanying unit/integration tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
python/packages/kagent-adk/src/kagent/adk/models/_bedrock.py |
Sanitizes tool spec names and remaps tool-use names in request/response paths. |
python/packages/kagent-adk/tests/unittests/models/test_bedrock.py |
Adds unit tests for sanitization and conversion, plus an end-to-end test for request/response remapping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Reverse map lets us restore original tool names from sanitized names in Bedrock responses. | ||
| reverse_name_map: dict[str, str] = {v: k for k, v in tool_name_map.items()} |
| raw_name = part.function_call.name or "" | ||
| sanitized_name = tool_name_map.get(raw_name, raw_name) if tool_name_map else raw_name | ||
| blocks.append( | ||
| { | ||
| "toolUse": { | ||
| "toolUseId": _sanitize_tool_id(part.function_call.id or "", id_map, counter), | ||
| "name": part.function_call.name or "", | ||
| "name": sanitized_name, | ||
| "input": part.function_call.args or {}, |
| def test_fully_invalid_name_gets_synthetic(self): | ||
| name_map: dict = {} | ||
| counter = [0] | ||
| # After substitution "!!!" becomes "___" which still passes the regex, | ||
| # but a name that starts with digits after substitution is fine too. | ||
| # Test a name that after substitution yields an empty string (all chars removed | ||
| # if we had a stricter regex — here it will be underscores, which is valid). | ||
| result = _sanitize_tool_name("valid_after_sub", name_map, counter) | ||
| assert _sanitize_tool_name.__module__ # just ensure it ran without error | ||
| assert result == "valid_after_sub" |
| from google.genai import types | ||
|
|
github-actions
Bot
added
bug
MCP server tool names often contain dots, colons, or spaces (e.g. "fetch.get_url", "filesystem:read_file") which are rejected by Bedrock's tool name constraint ([a-zA-Z0-9_-]+). Add _sanitize_tool_name() mirroring the existing _sanitize_tool_id() logic, wire it into _convert_tools_to_converse() so tool specs sent to Bedrock always have valid names, and pass the resulting name_map to _convert_content_to_converse_messages() so conversation history uses the same sanitized names. Build a reverse map from the sanitized names back to originals so LlmResponse function_call.name reflects the original tool name the ADK framework registered, not the Bedrock-sanitized variant. Fixes kagent-dev#1473 Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
mesutoezdil
force-pushed
the
fix/bedrock-tool-name-sanitization
branch
from
945d241 to
1663ca8
Compare
github-actions
Bot
added
bug
|
Hi @mesutoezdil thanks for the PR, can you also add it to Go runtime: https://github.com/kagent-dev/kagent/blob/main/go/adk/pkg/models/bedrock.go Have you been able to test the fix with a MCP server that actually has invalid tool name for Bedrock? I'm pretty sure this issue is valid and this is the right fix, but just double checking since I haven't got the chance to reproduce it |
MCP tool names often contain dots, colons, or spaces (e.g. fetch.get_url, filesystem:read_file) that Bedrock rejects because its Converse API requires tool names to match [a-zA-Z0-9_-]+. Added sanitizeBedrockToolName that replaces invalid characters with underscores, caching the mapping so repeated calls for the same name are consistent. convertGenaiToolsToBedrock now returns the original->sanitized name map alongside the tool list. The name map is threaded through GenerateContent so: - conversation history FunctionCall blocks use the sanitized name Bedrock already knows about - a reverse map restores the original name in both streaming and non-streaming responses so the ADK framework dispatches to the correctly registered tool Tests cover: sanitizer basics (dots, colons, spaces, caching, empty input), end-to-end tool spec name sanitization, and the name map contents. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
|
Go runtime is now updated. sanitizeBedrockToolName follows the same pattern as sanitizeBedrockToolID already in the file. The name map is built in convertGenaiToolsToBedrock, threaded through GenerateContent, applied to conversation history in convertGenaiContentsToBedrockMessages, and reversed in both generateNonStreaming and generateStreaming so the original tool name comes back to the ADK dispatcher. On testing: I have not run this against a live MCP server with an invalid tool name. The fix mirrors the Python implementation exactly and is covered by unit tests (sanitizer, end-to-end tool spec conversion, name map contents). If you have a setup to test with fetch or filesystem MCP tools that expose names like fetch.get_url, that would give us the live confirmation. |
Closes #1473.
MCP server tool names often contain dots, colons, or spaces (e.g.
fetch.get_urlorfilesystem:read_file). Bedrock's Converse API rejects any tool name outside[a-zA-Z0-9_-]+, so agents wired up to MCP tools hit a hard validation error before the first call goes through.Added
_sanitize_tool_name()that replaces invalid characters with underscores and caches the mapping for the duration of a request, same pattern as the existing_sanitize_tool_id(). Wired it into_convert_tools_to_converse()so everytoolSpec.namesent to Bedrock is clean. The same map is passed to_convert_content_to_converse_messages()so conversation history uses the sanitized names Bedrock expects. A reverse map is built from it so that when Bedrock echoes back a sanitized name in atoolUseresponse, we restore the original name before returning it in theLlmResponse, that way the ADK framework can still dispatch to the correctly registered tool.Tests cover the sanitizer itself (dots, colons, spaces, hyphens, caching, empty input) and the end-to-end flow verifying that the tool spec reaches Bedrock with the sanitized name and the response comes back with the original.