adds mcp cleanup support for bedrock models - #4573
Merged
akshaydeo merged 2 commits intoJun 20, 2026
Merged
Conversation
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces ChangesResponses API tool filtering: drop unsupported tools instead of failing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/responses.go`:
- Around line 2144-2153: After filtering unsupported tools into the keepTools
slice using anthropic.ValidateResponsesToolsForProvider, the code later
constructs tool_choice from the original unfiltered request parameters. If a
tool referenced in tool_choice was dropped during filtering, Bedrock will
receive a tool_choice reference to a tool that no longer exists in the converted
tools array, causing request rejection. Reconcile the tool_choice against
keepTools before assigning to bedrockReq.ToolConfig.ToolChoice by validating
that any pinned tool exists in keepTools, and either null out the tool_choice or
remap it if the referenced tool was filtered out. Apply this validation in both
locations where ToolChoice is assigned: around the initial construction and also
at lines 2417-2421.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 530af14e-f792-4e59-a6cc-9b08fb2b3425
📒 Files selected for processing (6)
core/providers/anthropic/requestbuilder.gocore/providers/anthropic/requestbuilder_test.gocore/providers/anthropic/utils.gocore/providers/anthropic/validateresponsestools_test.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/responses.go
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
akshaydeo
force-pushed
the
06-20-adds_mcp_cleanup_support_for_bedrock_models
branch
from
June 20, 2026 11:20
4c37ef8 to
3a0cdd5
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/api/collections/provider-harness.json (1)
1925-2959: ⚡ Quick winStreaming coverage gap for lone-mcp-dropped scenario.
The test description states "Covers native + /openai drop-in + streaming", but the explicit streaming tests (lines 2830-2959) only cover the forced function call scenario. The lone-mcp-dropped tests (e.g., lines 1934-1993, 2125-2183) only assert when
ct.indexOf('event-stream') === -1, meaning they skip all assertions when the response is streamed.This leaves the streaming behavior for the all-tools-dropped edge case (where only MCP tools are present and all are stripped) unverified. While the behavior is expected to be simpler (text answer, no tool calls), explicit streaming coverage would strengthen the regression suite and match the coverage claim.
📋 Add streaming variants for lone-mcp-dropped tests
Add streaming test cases similar to lines 2830-2893, but for the lone-mcp-dropped scenario. Example structure:
{ "name": "bedrock/global.anthropic.claude-opus-4-7 · streaming · lone server-mcp dropped", "event": [ { "listen": "test", "script": { "type": "text/javascript", "exec": [ "var ct = (pm.response.headers.get('content-type') || '');", "var raw = pm.response.text() || '';", "pm.test('mcp server tool dropped, not rejected (`#3795`)', function () {", " pm.expect(raw).to.not.include(\"tool type 'mcp'\");", " pm.expect(raw.toLowerCase()).to.not.include('is not supported by provider');", "});", "pm.test('response body is non-empty', function () {", " pm.expect(raw.length).to.be.above(0);", "});", "pm.test('streaming text answer, no tool call events (mcp dropped, zero tools left)', function () {", " pm.expect(ct).to.include('event-stream');", " pm.expect(raw).to.not.match(/\"type\"\\s*:\\s*\"function_call\"/);", " pm.expect(raw).to.not.match(/\"type\"\\s*:\\s*\"tool_call\"/);", "});" ] } } ], "request": { "method": "POST", "header": [{"key": "Content-Type", "value": "application/json"}], "body": { "mode": "raw", "raw": "{...lone mcp payload with \"stream\": true...}" }, "url": {"raw": "{{baseUrl}}/v1/responses", ...} } }Repeat for Bedrock sonnet, Vertex opus, and Vertex sonnet to match the forced-function-call streaming coverage.
🤖 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 `@tests/e2e/api/collections/provider-harness.json` around lines 1925 - 2959, The test suite claims to cover "native + /openai drop-in + streaming" for MCP tool handling, but the lone-mcp-dropped scenario (where only MCP server tools are present and all are stripped, leaving zero tools) only has non-streaming test cases. The existing lone-mcp-dropped tests skip all assertions when the response is streamed (checking `ct.indexOf('event-stream') === -1`). Add new streaming test variants for the lone-mcp-dropped scenario by creating test items following the pattern of the existing streaming forced-function-call tests (around lines 2830-2893 and 2903-2959), but using the lone-mcp-dropped request payloads with "stream": true added. Create one variant for each of the four provider models (bedrock/global.anthropic.claude-opus-4-7, bedrock/global.anthropic.claude-sonnet-4-6, vertex/claude-opus-4-7, vertex/claude-sonnet-4-6) to match the coverage of the forced-function-call streaming variants and verify that streaming responses correctly return text without tool call events when all tools are dropped.Makefile (1)
1978-1978: 💤 Low valueOptional: Remove unused
failedfunction for clarity.The
sanitizefunction now unconditionally callstrimstream, which is the correct behavior for consistent report output. However, thefailedfunction defined earlier in the jq expression is no longer used.♻️ Simplify jq expression
-jq -s 'def failed: (((.assertions // []) | any(.error?)) or ((.response.code // 0) == 0) or ((.response.code // 0) >= 400) or (.response | not)); def trimstream: if (.response.stream.type? == "Buffer" and ((.response.stream.data // []) | length) > 20000) then (.response.stream.data = .response.stream.data[:20000] | .response.stream.truncated = true) else . end; def sanitize: trimstream; {collection: (.[0].collection // {}), environment: (.[0].environment // {}), run: {executions: [.[].run.executions[]? | sanitize], failures: [.[].run.failures[]?], stats: {iterations: {total: 1, pending: 0, failed: 0}, items: {total: ([.[].run.stats.items.total // 0] | add)}, requests: {total: ([.[].run.stats.requests.total // 0] | add), failed: ([.[].run.stats.requests.failed // 0] | add)}}, timings: (.[0].run.timings // {})}}' tmp/newman-report-*.json > tmp/newman-report.json || $(ECHO) "$(YELLOW)Report merge failed; per-provider reports remain at tmp/newman-report-*.json$(NC)"; \ +jq -s 'def trimstream: if (.response.stream.type? == "Buffer" and ((.response.stream.data // []) | length) > 20000) then (.response.stream.data = .response.stream.data[:20000] | .response.stream.truncated = true) else . end; def sanitize: trimstream; {collection: (.[0].collection // {}), environment: (.[0].environment // {}), run: {executions: [.[].run.executions[]? | sanitize], failures: [.[].run.failures[]?], stats: {iterations: {total: 1, pending: 0, failed: 0}, items: {total: ([.[].run.stats.items.total // 0] | add)}, requests: {total: ([.[].run.stats.requests.total // 0] | add), failed: ([.[].run.stats.requests.failed // 0] | add)}}, timings: (.[0].run.timings // {})}}' tmp/newman-report-*.json > tmp/newman-report.json || $(ECHO) "$(YELLOW)Report merge failed; per-provider reports remain at tmp/newman-report-*.json$(NC)"; \🤖 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 `@Makefile` at line 1978, The jq expression in the Makefile contains a defined `failed` function that is never referenced or called anywhere in the remaining jq pipeline. Remove the unused `failed` function definition from the jq expression to simplify and clarify the code, keeping only the `trimstream` and `sanitize` function definitions which are actively used in the data transformation.
🤖 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.
Nitpick comments:
In `@Makefile`:
- Line 1978: The jq expression in the Makefile contains a defined `failed`
function that is never referenced or called anywhere in the remaining jq
pipeline. Remove the unused `failed` function definition from the jq expression
to simplify and clarify the code, keeping only the `trimstream` and `sanitize`
function definitions which are actively used in the data transformation.
In `@tests/e2e/api/collections/provider-harness.json`:
- Around line 1925-2959: The test suite claims to cover "native + /openai
drop-in + streaming" for MCP tool handling, but the lone-mcp-dropped scenario
(where only MCP server tools are present and all are stripped, leaving zero
tools) only has non-streaming test cases. The existing lone-mcp-dropped tests
skip all assertions when the response is streamed (checking
`ct.indexOf('event-stream') === -1`). Add new streaming test variants for the
lone-mcp-dropped scenario by creating test items following the pattern of the
existing streaming forced-function-call tests (around lines 2830-2893 and
2903-2959), but using the lone-mcp-dropped request payloads with "stream": true
added. Create one variant for each of the four provider models
(bedrock/global.anthropic.claude-opus-4-7,
bedrock/global.anthropic.claude-sonnet-4-6, vertex/claude-opus-4-7,
vertex/claude-sonnet-4-6) to match the coverage of the forced-function-call
streaming variants and verify that streaming responses correctly return text
without tool call events when all tools are dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 12d94a87-a63c-49de-82a9-10a92c557cf7
📒 Files selected for processing (12)
Makefilecore/providers/anthropic/requestbuilder.gocore/providers/anthropic/requestbuilder_test.gocore/providers/anthropic/utils.gocore/providers/anthropic/validateresponsestools_test.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/responses.gotests/config.jsontests/e2e/api/HARNESS_COVERAGE_BACKLOG.mdtests/e2e/api/collections/provider-harness.jsontests/integrations/python/config.jsontests/integrations/typescript/config.json
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/api/HARNESS_COVERAGE_BACKLOG.md
🚧 Files skipped from review as they are similar to previous changes (6)
- core/providers/anthropic/requestbuilder_test.go
- core/providers/bedrock/bedrock_test.go
- core/providers/anthropic/requestbuilder.go
- core/providers/anthropic/validateresponsestools_test.go
- core/providers/bedrock/responses.go
- core/providers/anthropic/utils.go
akshaydeo
force-pushed
the
06-20-adds_mcp_cleanup_support_for_bedrock_models
branch
from
June 20, 2026 11:40
3a0cdd5 to
74c95ec
Compare
akshaydeo
force-pushed
the
06-20-adds_mcp_cleanup_support_for_bedrock_models
branch
from
June 20, 2026 11:56
74c95ec to
bf3aca1
Compare
akshaydeo
force-pushed
the
06-20-extra_header_forwarding_for_mcp_tools
branch
from
June 20, 2026 11:56
5f40c10 to
411f221
Compare
Contributor
Author
Merge activity
|
akshaydeo
changed the base branch from
06-20-extra_header_forwarding_for_mcp_tools
to
graphite-base/4573
June 20, 2026 12:06
akshaydeo
dismissed
coderabbitai[bot]’s stale review
June 20, 2026 12:06
The base branch was changed.
9 tasks
akshaydeo
added a commit
that referenced
this pull request
Jun 21, 2026
## Summary Fixes a regression introduced in v1.5.0 (issue closes #3795) where a `/v1/responses` request carrying a Bifrost-hosted `mcp` server tool alongside function tools would fail with `"tool type 'mcp' is not supported by provider 'bedrock'"`. The Responses path now silently strips provider-unsupported tools instead of rejecting the entire request, matching the existing behavior of the Chat path. ## Changes - Introduced `ValidateResponsesToolsForProvider` in `anthropic/utils.go` — a Responses-path mirror of `ValidateChatToolsForProvider`. It partitions `[]schemas.ResponsesTool` into a keep-set and a dropped-set using the same per-type feature flags as `ValidateToolsForProvider`, but returns both sets instead of erroring, leaving policy decisions to callers. - Updated `ToBedrockResponsesRequest` in `bedrock/responses.go` to call `ValidateResponsesToolsForProvider` and use the filtered keep-set for tool conversion, rather than calling `ValidateToolsForProvider` and returning an error on the first unsupported tool. - Updated `BuildAnthropicResponsesRequestBody` in `anthropic/requestbuilder.go` to strip unsupported tools via a shallow copy of the request (so the shared/pooled inbound request and its `Params` are never mutated) instead of failing the request. - Updated the `ValidateTools` field comment to reflect the new strip-silently policy. - Added `validateresponsestools_test.go` with a dedicated test table covering Bedrock, Vertex, Anthropic, Azure, unknown providers, and forward-compat cases. - Added regression tests in `bedrock_test.go` covering the mixed mcp+function case and the all-tools-dropped case. - Updated the existing `requestbuilder_test.go` test to assert that unsupported tools are stripped (not rejected) and that the inbound request is not mutated. ## Type of change - [x] Bug fix ## Affected areas - [x] Core (Go) - [x] Providers/Integrations ## How to test ```sh go test ./core/providers/anthropic/... ./core/providers/bedrock/... ``` Expected: all tests pass, including the new regression guards for issue #3795. Specifically, a `/v1/responses` request to Bedrock with a mixed `mcp` + function tool list should succeed, with only the function tool forwarded to Bedrock and the `mcp` tool silently dropped. The inbound request's tool slice must remain unmodified. ## Breaking changes - [x] No ## Related issues Closes #3795 ## Security considerations None. The change only affects which tools are forwarded to downstream providers. Unsupported tools are dropped rather than causing a hard failure; no secrets, auth, or PII handling is affected. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a regression introduced in v1.5.0 (issue closes #3795) where a
/v1/responsesrequest carrying a Bifrost-hostedmcpserver tool alongside function tools would fail with"tool type 'mcp' is not supported by provider 'bedrock'". The Responses path now silently strips provider-unsupported tools instead of rejecting the entire request, matching the existing behavior of the Chat path.Changes
ValidateResponsesToolsForProviderinanthropic/utils.go— a Responses-path mirror ofValidateChatToolsForProvider. It partitions[]schemas.ResponsesToolinto a keep-set and a dropped-set using the same per-type feature flags asValidateToolsForProvider, but returns both sets instead of erroring, leaving policy decisions to callers.ToBedrockResponsesRequestinbedrock/responses.goto callValidateResponsesToolsForProviderand use the filtered keep-set for tool conversion, rather than callingValidateToolsForProviderand returning an error on the first unsupported tool.BuildAnthropicResponsesRequestBodyinanthropic/requestbuilder.goto strip unsupported tools via a shallow copy of the request (so the shared/pooled inbound request and itsParamsare never mutated) instead of failing the request.ValidateToolsfield comment to reflect the new strip-silently policy.validateresponsestools_test.gowith a dedicated test table covering Bedrock, Vertex, Anthropic, Azure, unknown providers, and forward-compat cases.bedrock_test.gocovering the mixed mcp+function case and the all-tools-dropped case.requestbuilder_test.gotest to assert that unsupported tools are stripped (not rejected) and that the inbound request is not mutated.Type of change
Affected areas
How to test
go test ./core/providers/anthropic/... ./core/providers/bedrock/...Expected: all tests pass, including the new regression guards for issue #3795. Specifically, a
/v1/responsesrequest to Bedrock with a mixedmcp+ function tool list should succeed, with only the function tool forwarded to Bedrock and themcptool silently dropped. The inbound request's tool slice must remain unmodified.Breaking changes
Related issues
Closes #3795
Security considerations
None. The change only affects which tools are forwarded to downstream providers. Unsupported tools are dropped rather than causing a hard failure; no secrets, auth, or PII handling is affected.
Checklist
docs/contributing/README.mdand followed the guidelines