Skip to content

fix(bedrock): sanitize tool names to the Converse pattern in specs and replayed history - #90016

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-90008
Open

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-90008

Conversation

@liuhao1024

Copy link
Copy Markdown

What does this PR do?

Fixes #90008: after a single tool call whose name violates Bedrock's Converse pattern, every subsequent ConverseStream request in the session fails with a ValidationException — including a plain "hi" — because the offending toolUse block sits in the replayed history and is re-validated on every request.

Bedrock validates toolSpec.name / toolUse.name against ^[a-zA-Z0-9_-]{1,64}$. Tool names sourced from MCP servers, plugins, or aliases can carry dots, slashes, or unicode (the report's flow hits it after a tool-search invocation). The adapter passed both sites through verbatim.

This adds _sanitize_bedrock_tool_name(): non-conforming characters fold to underscores, the result is truncated to 64, and an empty result falls back to tool. It is applied identically at both emission sites — the toolSpec definitions (convert_tools_to_converse) and the replayed toolUse history blocks (convert_messages_to_converse) — so the name the model's earlier calls are recorded under matches the spec it is offered. toolResult blocks pair by toolUseId, not by name, so tool results are unaffected by the rename.

Related Issue

Fixes #90008

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/bedrock_adapter.py: new _BEDROCK_TOOL_NAME_RE + _sanitize_bedrock_tool_name(); applied at the toolSpec name and the replayed toolUse name.

How to Test

  1. pytest tests/agent/test_bedrock_adapter.py -q — 71 passed (5 new tests in TestBedrockToolNameSanitization).
  2. Fail-on-main verified: with the source stashed, all 5 new tests fail on the unmodified tree.
  3. Full Bedrock suites (test_bedrock_adapter, test_bedrock_empty_text_blocks, test_bedrock_1m_context): 76 passed.
  4. Observed result: a tool named mcp.github.search_repos/v2·x becomes mcp_github_search_repos_v2_x in both the spec and the replayed history; conforming names (including mcp__server__tool) pass through unchanged.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (arm64)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

…d replayed history

Bedrock's Converse/ConverseStream APIs validate toolSpec.name and
toolUse.name against ^[a-zA-Z0-9_-]{1,64}$. Tool names sourced from MCP
servers, plugins, or aliases can carry dots, slashes, or unicode, and a
single offending toolUse block in the conversation history is replayed
(and re-validated) on EVERY subsequent request — after one bad call the
session fails permanently, even for a plain 'hi' (NousResearch#90008).

Fold non-conforming characters to underscores (truncate to 64, empty
fallback 'tool') identically at both emission sites — the toolSpec
definitions and the replayed toolUse history blocks — so the name the
model's earlier calls are recorded under matches the spec it is offered.
toolResult blocks pair by toolUseId, not by name, so tool results are
unaffected by the rename.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/mcp MCP client and OAuth provider/bedrock AWS Bedrock (boto3, IAM) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 19, 2026
@liuhao1024

Copy link
Copy Markdown
Author

The failing checks here are a pre-existing main regression, not caused by this diff: the failing test is the FAL catalog all-off upscale invariant (test_upscale_defaults_are_all_off), which was broken on upstream/main itself for a window on Aug 19 (a catalog entry shipped upscale: True against the opt-in-only policy the test pins). The same failure hit sibling PRs from that window and was root-caused on #89968/#89987.

Main has since resolved it on its own (the catalog entry was fixed and the invariant test was dropped in 00888e8), so this failure should disappear on the next rebase. This PR's changes don't touch the image-generation catalog or its tests. Not re-running CI since the failing slice is unrelated to the change under review.

This branch has not been deployed

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists provider/bedrock AWS Bedrock (boto3, IAM) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Bedrock ConverseStream fails when toolUse.name contains unsupported characters

2 participants