bedrock function toolname truncation

[akshaydeo]

Summary

Bedrock enforces a strict tool name format: names must match [A-Za-z0-9_-]{1,64}. MCP tool names (e.g. mcp__plugin_chrome-devtools-mcp_chrome-devtools__list_network_requests) violate this constraint, causing API errors. This PR introduces transparent aliasing so that long or unsafe tool names are automatically shortened to a safe alias before being sent to Bedrock, and then restored to their original names in the response.

Issue

closes #3788

Changes

• Added bedrockAliasToolName which, for any tool name that doesn't satisfy Bedrock's naming rules, generates a deterministic alias of the form <8-char sha1 hash>_<semantic suffix> (≤64 chars total). The alias-to-original mapping is stored in the BifrostContext using a typed context key.
• Added bedrockRestoreToolName which looks up the alias map in the context and returns the original name, used when converting Bedrock responses back to Bifrost format.
• Aliasing is applied consistently across all code paths: tool definitions, toolChoice.tool.name, tool calls in assistant messages, and tools extracted from conversation history.
• Name restoration is applied in both non-streaming (ToBifrostChatResponse) and streaming (ToBifrostChatCompletionStream) response conversion paths.
• BedrockStreamState now carries a context.Context, populated via NewBedrockStreamStateWithContext, so the streaming path has access to the alias map.
• Added two tests: one verifying that long MCP tool names are aliased correctly on the request side, and one verifying that aliased names are restored to their originals in the response.

Type of change

• Bug fix

Affected areas

• Core (Go)
• Providers/Integrations

How to test

go test ./core/providers/bedrock/... -run "TestToBedrockChatCompletionRequest_AliasesLongMCPToolNames"
go test ./core/providers/bedrock/... -run "TestBedrockToBifrostChatResponse_RestoresAliasedToolName"
go test ./core/providers/bedrock/...

Pass a tool with a name longer than 64 characters or containing characters outside [A-Za-z0-9_-] (such as an MCP-style name with __ and . segments) to a Bedrock-backed model. Verify that the request succeeds and that the tool name in the response matches the original name provided.

Breaking changes

• No

Security considerations

The alias map is scoped to a single BifrostContext per request and is never persisted or shared across requests. Tool names are hashed with SHA-1 solely for collision-resistant shortening; no sensitive data is derived from the hash.

Checklist

• I added/updated tests where appropriate
• I verified builds succeed (Go and UI)

Summary by CodeRabbit

• New Features

  • Enhanced Bedrock provider with improved tool name handling across chat and responses APIs
  • Updated code editor UI with better code folding controls

• Tests

  • Added comprehensive test coverage for Bedrock tool handling in chat completion and responses APIs

• Chores

  • Updated dependencies for internal tooling

[coderabbitai]

Warning

Review limit reached

@akshaydeo, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 9 minutes and 50 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 90d202b6-eddc-4972-a5f7-f4be792bcf77

📥 Commits

Reviewing files that changed from the base of the PR and between 6ff893e and 94956a5.

⛔ Files ignored due to path filters (1)

• core/go.sum is excluded by !**/*.sum

📒 Files selected for processing (8)

• core/go.mod
• core/providers/bedrock/bedrock.go
• core/providers/bedrock/bedrock_test.go
• core/providers/bedrock/chat.go
• core/providers/bedrock/responses.go
• core/providers/bedrock/utils.go
• ui/app/workspace/logs/sheets/logDetailView.tsx
• ui/components/ui/codeEditor.tsx

📝 Walkthrough

Walkthrough

This PR addresses the AWS Bedrock tool name length limit by implementing deterministic aliasing. Long MCP tool names (exceeding 64 characters) are converted to Bedrock-safe aliases during request conversion and restored to original names during response conversion. The solution propagates context through streaming and batch conversion paths for both ChatCompletion and Responses APIs, with comprehensive test coverage.

Changes

Tool Name Aliasing for Bedrock Requests

Layer / File(s)	Summary
Tool aliasing infrastructure and xxhash dependency core/go.mod, core/providers/bedrock/utils.go	Adds xxhash v2.3.0 dependency and implements core aliasing functions: bedrockAliasToolName() sanitizes long tool names into xxhash-prefixed shortened versions and stores reverse mappings in request context; bedrockRestoreToolName() retrieves originals.
Chat stream state context propagation core/providers/bedrock/chat.go, core/providers/bedrock/bedrock.go	BedrockStreamState gains a ctx field; new NewBedrockStreamStateWithContext(ctx) constructor initializes state with context. ChatCompletionStream uses the context-aware constructor.
Chat completion request and response aliasing core/providers/bedrock/chat.go, core/providers/bedrock/utils.go	ToBedrockChatCompletionRequest() passes context to ensureChatToolConfigForConversation() for tool-name aliasing. ToBifrostChatResponse() restores original names from tool-use blocks. Streaming tool-use "start" events restore aliased names. Helper functions updated to accept and propagate context.
Responses stream state context propagation core/providers/bedrock/responses.go, core/providers/bedrock/bedrock.go	BedrockResponsesStreamState gains a Ctx field; reset in pool. ResponsesStream assigns request context to stream state after creation.
Responses API request and response aliasing core/providers/bedrock/responses.go, core/providers/bedrock/utils.go	Tool specs and tool_choice names are aliased via bedrockAliasToolName(ctx, ...) in requests. Tool extraction and conversion functions updated to accept context. Responses restore original names from tool-use blocks and FunctionCall messages via bedrockRestoreToolName(ctx, ...).
Test coverage for tool name aliasing core/providers/bedrock/bedrock_test.go	Four new tests verify ChatCompletion and Responses APIs: long tool names are aliased with hash-prefix format in requests, and aliases are correctly restored to original names in responses including tool_choice handling.

CodeEditor Folding UI Adjustment

Layer / File(s)	Summary
CodeEditor folding UI configuration ui/components/ui/codeEditor.tsx	editorOptions now conditionally enables glyphMargin and adjusts lineDecorationsWidth based on collapsibleBlocks option for improved visual layout.

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

• danpiths
• roroghost17

🐰 Long tool names no longer exceed the sixty-four,
Bedrock requests now pass through the API door,
With xxhash aliases and context-aware care,
Original names restored with a restore-repair air! 🔧✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'bedrock function toolname truncation' is vague and overly generic, using imprecise language that doesn't clearly convey what was changed or how the problem was solved.	Revise the title to be more specific and descriptive, such as 'Add tool name aliasing for Bedrock to handle MCP names exceeding 64-character limit' to clearly explain the solution.
Out of Scope Changes check	❓ Inconclusive	Most changes are tightly scoped to the Bedrock tool name aliasing requirement. However, the UI change to codeEditor.tsx (folding UI adjustments) appears unrelated to the linked issue #3788 which focuses solely on Bedrock tool naming.	Clarify whether the codeEditor.tsx changes are intentional or should be moved to a separate PR, as they appear unrelated to the Bedrock tool name aliasing feature.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	The PR description is comprehensive and well-structured, covering the problem, solution, testing instructions, and security considerations clearly.
Linked Issues check	✅ Passed	The PR successfully addresses all objectives from issue #3788: tool name aliasing for Bedrock names exceeding 64 characters, deterministic alias generation with reverse mapping, context-aware restoration in both streaming and non-streaming paths, and comprehensive test coverage.
Docstring Coverage	✅ Passed	Docstring coverage is 82.76% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 05-29-bedrock_function_toolname_truncation

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[akshaydeo]

• changelogs for 1.5.6 #3892
• bedrock function toolname truncation #3890 👈 (View in Graphite)
• adds metadata to hybdrid store payload #3888
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Confidence Score: 4/5

The core aliasing logic is correct and the change is safe to merge, but an unresolved concurrent map write (flagged in a prior review pass) means a retry or parallel-tool path on the same BifrostContext could trigger a data race on the alias map.

The alias map is populated under a read lock on the context's value slot but is then mutated directly — a concurrent call to bedrockAliasToolName or bedrockRestoreToolName on the same context could race. Until the map access is protected, the Go race detector could fire on production workloads that retry or fan out tool calls.

core/providers/bedrock/utils.go — the alias map mutation and hash truncation both live here

Important Files Changed

Filename	Overview
core/providers/bedrock/utils.go	Introduces bedrockAliasToolName/bedrockRestoreToolName using xxhash. The alias map is written outside the lock guarding the context value slot (flagged separately), and the 32-bit hash truncation increases collision risk.
core/providers/bedrock/chat.go	Threads context through BedrockStreamState for alias restoration in both streaming and non-streaming paths. Nil-state fallback path (already flagged) silently skips restoration.
core/providers/bedrock/responses.go	Adds Ctx field to BedrockResponsesStreamState, properly resets it to nil in the pool acquire function, and applies aliasing/restoration consistently across the Responses API path.
core/providers/bedrock/bedrock.go	Updates both streaming paths to use the context-aware stream state constructors, ensuring the alias map is available during response conversion.
core/providers/bedrock/bedrock_test.go	Adds request-side aliasing and non-streaming restoration tests for both Chat and Responses APIs. Streaming restoration path for ToBifrostChatCompletionStream remains untested (flagged separately).
ui/components/ui/codeEditor.tsx	Enables glyph margin and widens line decoration area when folding is active so that fold controls render correctly. Purely visual improvement.
ui/app/workspace/logs/sheets/logDetailView.tsx	Whitespace-only reformatting (2-space to tab indentation). No functional changes.
core/go.mod	Adds github.com/cespare/xxhash/v2 v2.3.0 as a new dependency for the alias hash function.

_{Reviews (7): Last reviewed commit: "bedrock function toolname truncation" | Re-trigger Greptile}

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[akshaydeo]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

core/providers/bedrock/responses.go (1)

160-230: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Pool reset discipline violation: flush() does not reset Ctx field.

The flush() method resets all other fields of BedrockResponsesStreamState but omits resetting the Ctx field added at line 42. Since releaseBedrockResponsesStreamState() calls flush() before returning the object to the pool, this creates a data leakage risk where a request context could persist across pooled object reuses.

While acquireBedrockResponsesStreamState() does reset state.Ctx = nil (line 143), both reset paths should be consistent per repository patterns.

🔧 Proposed fix

 func (state *BedrockResponsesStreamState) flush() {
 	// Clear maps (reuse if already initialized, otherwise initialize)
 	// ... existing map clearing code ...
 	state.CreatedAt = int(time.Now().Unix())
 	state.HasEmittedCreated = false
 	state.HasEmittedInProgress = false
 	state.UsedStructuredOutputTool = false
+	state.Ctx = nil
 }

Add after line 229:

	state.Ctx = nil

As per coding guidelines: "Reset all fields of pooled objects before calling pool.Put() to prevent data leakage between requests" and "Pooled objects must have every field reset before returning to a pool."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/providers/bedrock/responses.go` around lines 160 - 230, The flush()
method of BedrockResponsesStreamState fails to clear the Ctx field, risking
context leakage when objects are returned to the pool; update flush() to set
state.Ctx = nil (mirroring acquireBedrockResponsesStreamState() and ensuring
releaseBedrockResponsesStreamState() returns fully-reset objects) so all fields
are reset before pool.Put().

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/providers/bedrock/bedrock_test.go`:
- Around line 5343-5382: The test
TestToBedrockChatCompletionRequest_AliasesLongMCPToolNames only checks
length/prefix/suffix; update it (and the similar block around lines 5430-5476)
to assert the full Bedrock tool-name regex /^[A-Za-z0-9_-]{1,64}$/ for alias and
add an extra subcase where the original tool name contains invalid characters
(e.g., spaces, ":", "/", ".") to confirm ToBedrockChatCompletionRequest
sanitizes them into a Bedrock-valid name; specifically, after computing alias :=
result.ToolConfig.Tools[0].ToolSpec.Name assert it matches the regex and that
the alias contains no disallowed characters, and add a new test input toolName
with invalid chars and repeat the same assertions (including equality with
result.ToolConfig.ToolChoice.Tool.Name).

In `@core/providers/bedrock/utils.go`:
- Around line 138-142: The function bedrockAliasToolName currently only checks
length and returns names with disallowed characters unchanged; update
bedrockAliasToolName to validate the name against Bedrock's pattern
`[A-Za-z0-9_-]{1,64}` (use a regexp or equivalent) and only return the original
name when it matches that pattern, otherwise generate a safe alias (e.g.,
truncated + deterministic hash) and record the reverse mapping as the existing
mapping logic expects; ensure you reference and update the same alias storage
used by bedrockAliasToolName so lookups still work.

---

Outside diff comments:
In `@core/providers/bedrock/responses.go`:
- Around line 160-230: The flush() method of BedrockResponsesStreamState fails
to clear the Ctx field, risking context leakage when objects are returned to the
pool; update flush() to set state.Ctx = nil (mirroring
acquireBedrockResponsesStreamState() and ensuring
releaseBedrockResponsesStreamState() returns fully-reset objects) so all fields
are reset before pool.Put().

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0cd64091-002b-4f86-a8cd-2b358c1b60cd

📥 Commits

Reviewing files that changed from the base of the PR and between b2c5f2a and 6ff893e.

⛔ Files ignored due to path filters (1)

• core/go.sum is excluded by !**/*.sum

📒 Files selected for processing (8)

• core/go.mod
• core/providers/bedrock/bedrock.go
• core/providers/bedrock/bedrock_test.go
• core/providers/bedrock/chat.go
• core/providers/bedrock/responses.go
• core/providers/bedrock/utils.go
• ui/app/workspace/logs/sheets/logDetailView.tsx
• ui/components/ui/codeEditor.tsx

[akshaydeo]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[akshaydeo]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[akshaydeo]

Merge activity

• May 29, 4:25 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 29, 4:27 PM UTC: Graphite rebased this pull request as part of a merge.
• May 29, 4:28 PM UTC: @akshaydeo merged this pull request with Graphite.