fix(bedrock): sanitize tool names for Bedrock API constraints#1474

Open

frank-bee wants to merge 4 commits intokagent-dev:mainfrom

frank-bee:fix/bedrock-tool-name-sanitization

frank-bee commented Mar 10, 2026

Fixes #1473

Problem

When using kagent with Amazon Bedrock models, MCP servers (e.g. GitHub Copilot) may produce tool names containing dots, spaces, or other characters that violate Bedrock's API constraints.

Bedrock requires tool names to match [a-zA-Z0-9_-]+ with a minimum length of 1. Any name outside this pattern causes API errors like:

ValidationException: A tool name is invalid. Tool names can only contain letters, numbers, underscores, and hyphens

Root Cause

KAgentLiteLlm does not sanitize tool names before passing them to the Bedrock API, and does not sanitize tool names in the conversation history (function call parts) sent in subsequent requests.

Fix

Override generate_content_async in KAgentLiteLlm to:

Sanitize tool names in the outgoing LlmRequest history (function call parts in contents)
Sanitize tool names in incoming LlmResponse parts

Invalid characters are replaced with _. Empty/completely invalid names fall back to unknown_tool_<idx>. A warning is logged whenever a name is sanitized, to aid debugging.

frank-bee requested review from EItanya and peterj as code owners

March 10, 2026 20:20

Copilot AI review requested due to automatic review settings

March 10, 2026 20:20

frank-bee requested a review from yuval-k as a code owner

March 10, 2026 20:20

Copilot started reviewing on behalf of frank-bee

March 10, 2026 20:20


          fix(bedrock): sanitize tool names for Bedrock API constraints

7555d12

Bedrock requires tool names to match [a-zA-Z0-9_-]+ with length >= 1.
MCP servers (e.g. GitHub Copilot) may produce tool names with dots,
spaces, or other invalid characters. Override generate_content_async
in KAgentLiteLlm to sanitize tool names in both the outgoing request
history and incoming response parts when using a Bedrock model.

Signed-off-by: Frank Bernhardt <Frank.Bernhardt@quantum-machines.co>

frank-bee force-pushed the fix/bedrock-tool-name-sanitization branch from b83598f to 7555d12 Compare

March 10, 2026 20:20

Copilot AI reviewed

View reviewed changes

Contributor

Copilot AI left a comment

Pull request overview

This PR updates the KAgentLiteLlm Bedrock integration to prevent Amazon Bedrock API validation errors caused by tool names containing disallowed characters.

Changes:

Add Bedrock tool-name validation/sanitization helpers (_sanitize_tool_name, request/response sanitizers).
Override generate_content_async to sanitize function-call tool names in outgoing request history and incoming responses when targeting Bedrock.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

python/packages/kagent-adk/src/kagent/adk/models/_litellm.py

Comment on lines +66 to +75

+                  async def generate_content_async(
+                      self, llm_request: LlmRequest, stream: bool = False
+                  ) -> AsyncGenerator[LlmResponse, None]:
+                      effective_model = llm_request.model or self.model
+                      if _is_bedrock_model(effective_model):
+                          _sanitize_llm_request(llm_request)
+                          idx = 0
+                          async for response in super().generate_content_async(llm_request, stream=stream):
+                              yield _sanitize_llm_response(response, idx)
+                              idx += 1

Copilot AI Mar 10, 2026

Tool-name sanitization logs at WARNING every time an invalid name is seen. In streaming mode, generate_content_async yields many chunks, so the same invalid tool name could generate a large number of warning log lines (one per chunk/part), which can be noisy and increase log volume. Consider de-duplicating warnings per request/response (e.g., track already-sanitized originals) or lowering to INFO/DEBUG if this is expected in normal operation.

Copilot uses AI. Check for mistakes.

Contributor

EItanya Mar 10, 2026

I agree with this, I think DEBUG if at all.

Author

frank-bee Mar 11, 2026

Fixed — changed to logger.debug in bc1b190.

python/packages/kagent-adk/src/kagent/adk/models/_litellm.py

Comment on lines +66 to +78

+                  async def generate_content_async(
+                      self, llm_request: LlmRequest, stream: bool = False
+                  ) -> AsyncGenerator[LlmResponse, None]:
+                      effective_model = llm_request.model or self.model
+                      if _is_bedrock_model(effective_model):
+                          _sanitize_llm_request(llm_request)
+                          idx = 0
+                          async for response in super().generate_content_async(llm_request, stream=stream):
+                              yield _sanitize_llm_response(response, idx)
+                              idx += 1
+                      else:
+                          async for response in super().generate_content_async(llm_request, stream=stream):
+                              yield response

Copilot AI Mar 10, 2026

This change introduces Bedrock-specific mutation of LlmRequest/LlmResponse tool-call names, but there are no unit tests exercising the new sanitization logic. Adding focused tests (e.g., construct an LlmRequest with function_call parts containing invalid names and assert they are sanitized when model is bedrock/..., and unchanged otherwise) would help prevent regressions.

Copilot uses AI. Check for mistakes.

Contributor

EItanya Mar 10, 2026

Agreed, we need unit tests

Author

frank-bee Mar 11, 2026

Added unit tests in tests/unittests/models/test_litellm_bedrock.py (15 tests, all passing locally). Covers sanitize helpers, Bedrock vs non-Bedrock routing, no-collision across parts, and log-level assertion.

python/packages/kagent-adk/src/kagent/adk/models/_litellm.py Outdated Show resolved Hide resolved


          Apply suggestions from code review

5dacaa3

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

EItanya requested changes

View reviewed changes

python/packages/kagent-adk/src/kagent/adk/models/_litellm.py

Comment on lines +66 to +75

+                  async def generate_content_async(
+                      self, llm_request: LlmRequest, stream: bool = False
+                  ) -> AsyncGenerator[LlmResponse, None]:
+                      effective_model = llm_request.model or self.model
+                      if _is_bedrock_model(effective_model):
+                          _sanitize_llm_request(llm_request)
+                          idx = 0
+                          async for response in super().generate_content_async(llm_request, stream=stream):
+                              yield _sanitize_llm_response(response, idx)
+                              idx += 1

Contributor

EItanya Mar 10, 2026

I agree with this, I think DEBUG if at all.

python/packages/kagent-adk/src/kagent/adk/models/_litellm.py

Comment on lines +66 to +78

+                  async def generate_content_async(
+                      self, llm_request: LlmRequest, stream: bool = False
+                  ) -> AsyncGenerator[LlmResponse, None]:
+                      effective_model = llm_request.model or self.model
+                      if _is_bedrock_model(effective_model):
+                          _sanitize_llm_request(llm_request)
+                          idx = 0
+                          async for response in super().generate_content_async(llm_request, stream=stream):
+                              yield _sanitize_llm_response(response, idx)
+                              idx += 1
+                      else:
+                          async for response in super().generate_content_async(llm_request, stream=stream):
+                              yield response

Contributor

EItanya Mar 10, 2026

Agreed, we need unit tests


          Merge branch 'main' into fix/bedrock-tool-name-sanitization

d68aeb2

frank-bee requested a review from EItanya

March 11, 2026 07:31


          fix(bedrock): downgrade sanitization log to DEBUG, add unit tests

bc1b190

- Change logger.warning -> logger.debug for tool-name sanitization to
  avoid log noise in streaming mode
- Fix _sanitize_tool_name type hint to accept str|int (composite suffix)
- Add unit tests covering sanitize helpers and Bedrock vs non-Bedrock routing

Signed-off-by: Frank Bernhardt <Frank.Bernhardt@quantum-machines.co>

frank-bee force-pushed the fix/bedrock-tool-name-sanitization branch from f1a2e39 to bc1b190 Compare

March 11, 2026 07:36

Author

frank-bee commented Mar 11, 2026

Addressed all review comments in bc1b190:

Log level: logger.warning → logger.debug for tool-name sanitization
Collision-safe suffix: already using f"{idx}_{i}" composite suffix (no idx * 100 + i)
Unit tests: added tests/unittests/models/test_litellm_bedrock.py with 15 tests covering:
- _sanitize_tool_name (valid names unchanged, replacement of invalid chars, empty fallback, composite suffix, log level)
- _sanitize_llm_request (fixes invalid names, leaves valid names, handles empty parts)
- _sanitize_llm_response (fixes names, no-collision across parts, handles null content)
- KAgentLiteLlm.generate_content_async (Bedrock path sanitizes, non-Bedrock path passes through unchanged)

All 15 tests pass locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

peterj Awaiting requested review from peterj peterj is a code owner

yuval-k Awaiting requested review from yuval-k yuval-k is a code owner

EItanya Awaiting requested review from EItanya EItanya is a code owner

Requested changes must be addressed to merge this pull request.

Labels

None yet