mcp tool call with error handling

[akshaydeo]

Summary

Failed MCP tool executions were being replayed to the model as successful results. The is_error / IsError marker was either never set, silently dropped during JSON unmarshalling, or lost when converting between the chat completions and Responses surfaces. This PR closes all three gaps so that Anthropic tool_result.is_error, Bedrock toolResult.status, and the Responses surface status: "incomplete" all reflect the actual outcome of a tool call.

Changes

• Agent loop (mcp/agent.go): createToolResultMessage now sets IsError: true on the returned ChatToolMessage when the execution error is non-nil, so a tool call that bifrost itself watched fail is no longer reported to the model as a success.
• MCP tool manager (mcp/toolmanager.go, mcp/utils.go): createToolResponseMessage gains an isError bool parameter. executeToolInternal passes toolResponse.IsError through, carrying the MCP protocol's own failure signal (mcp.CallToolResult.IsError) which was previously discarded.
• CodeMode (mcp/codemode/starlark/): All call sites of createToolResponseMessage are updated to pass an explicit isError flag. Lookup failures (unknown server, unknown tool, ambiguous filename), sandbox execution failures, and MCP protocol-level errors are all marked true; successful responses and informational listings are marked false.
• Unmarshal gate (schemas/chatcompletions.go): The ChatMessage.UnmarshalJSON reattach gate previously keyed solely on ToolCallID, which silently dropped ChatToolMessage — and with it IsError — for any tool message that carried the error marker without a tool_call_id. The gate now also triggers on IsError != nil.
• Chat ↔ Responses mux (schemas/mux.go): ToResponsesMessages maps IsError: true to Status: "incomplete" (the Responses surface equivalent already read back as is_error by the Anthropic Responses converter). ToChatMessages maps both ResponsesToolMessage.Error (non-empty string) and Status: "incomplete" back to IsError: true, so the marker survives a full chat → Responses → chat round trip.
• Tests: New test files cover the agent-loop marker (mcp/toolerrormarker_test.go), the CodeMode marker (mcp/codemode/starlark/toolerrormarker_test.go), the unmarshal gate edge case (schemas/toolmessageiserror_test.go), and both directions of the mux conversion including a round-trip regression (schemas/toolerrormux_test.go).

Type of change

• Bug fix

Affected areas

• Core (Go)
• Providers/Integrations

How to test

go test ./core/...

Key test targets:

• core/mcp — TestCreateToolResultMessageMarksError, TestCreateToolResponseMessageMarksError
• core/mcp/codemode/starlark — TestCreateToolResponseMessageMarksError
• core/schemas — TestChatMessageIsErrorWithoutToolCallID, TestChatToResponsesCarriesToolError, TestResponsesToChatCarriesToolError, TestToolErrorSurvivesSurfaceRoundTrip

Breaking changes

• Yes
• No

Related issues

Closes #5890

Security considerations

None. Changes are confined to how tool result metadata is propagated between internal surfaces and providers.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes

  • Tool execution failures are now preserved across Chat and Responses formats.
  • Failed MCP and provider tool results are no longer treated as successful responses.
  • Error-only tool messages are retained even without a tool call ID.
  • Tool error status remains accurate through conversions and round trips.

• Tests

  • Added coverage for MCP, provider, agent, and cross-format tool error handling.

Walkthrough

MCP execution and protocol failures now set ChatToolMessage.IsError. CodeMode responses pass explicit error status. Chat and Responses conversions preserve tool errors, including messages without a tool_call_id. Bedrock conversion tests cover failed and successful tool results.

Changes

Tool error propagation

Layer / File(s)	Summary
MCP and CodeMode error markers core/mcp/agent.go, core/mcp/utils.go, core/mcp/toolmanager.go, core/mcp/codemode/starlark/..., core/mcp/*toolerrormarker_test.go	Tool execution failures and MCP IsError results now set ChatToolMessage.IsError. CodeMode lookup, sandbox, and nested MCP failures pass explicit error status.
Chat, Responses, and Bedrock conversion core/schemas/chatcompletions.go, core/schemas/mux.go, core/schemas/toolerrormux_test.go, core/schemas/toolmessageiserror_test.go, core/providers/bedrock/toolerroringress_test.go	Chat tool errors convert to Responses incomplete status. Responses error strings and incomplete status convert back to IsError=true. Error-only messages without tool_call_id are preserved. Bedrock tests cover failed and successful conversions.
Changelog core/changelog.md	The changelog records the tool-result error propagation changes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MCPExecution
  participant ChatToolMessage
  participant ChatToResponses
  participant ResponsesToChat
  MCPExecution->>ChatToolMessage: Set IsError for failed execution
  ChatToolMessage->>ChatToResponses: Convert IsError=true
  ChatToResponses->>ResponsesToChat: Preserve incomplete status
  ResponsesToChat->>ChatToolMessage: Restore IsError=true

Loading

Possibly related PRs

• maximhq/bifrost#5450: Extends the ChatToolMessage.IsError propagation introduced by this change with provider wire mappings.
• maximhq/bifrost#5573: Modifies related nested Starlark MCP error propagation.

Suggested reviewers: pratham-mishra04

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes satisfy #5890 by preserving tool-error markers through execution, unmarshalling, and Chat-to-Responses conversions, with regression tests.
Out of Scope Changes check	✅ Passed	The production changes, tests, and changelog entries remain focused on MCP tool-error propagation and the requirements of #5890.
Docstring Coverage	✅ Passed	Docstring coverage is 94.12% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title clearly identifies MCP tool-call error handling, which matches the primary change.
Description check	✅ Passed	The description explains the problem, changes, tests, affected areas, breaking status, issue, security impact, and checklist.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 08-06-mcp_tool_call_with_error_handling

---

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

[akshaydeo]

• pr resolve skill #5922
• fixed message start frame #5907
• mcp tool call with error handling #5894 👈 (View in Graphite)
• fail soft on invalid_encrypted_content by stripping replayed reasoning and retrying once #5892
• normalize tool_search_tool_* on the Responses path #5891
• deepseek thinking fixes #5888
• keep pricing objects in anthropic path for ai-sdk compatibility #5886
• file type mapping bug fix #5884
• harness tests #5880
• dev

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

[coderabbitai]

Actionable comments posted: 3

🤖 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/mcp/codemode/starlark/executecode.go`:
- Around line 199-201: Update the result.Errors != nil branch in the sandbox
execution flow to set executionSuccess = false before createToolResponseMessage
is called, ensuring failed executions are marked as tool failures. Add a
regression test covering an execution result with non-nil Errors and verify the
response is flagged as unsuccessful.
- Around line 556-560: The extractResultFromChatMessage path must propagate
protocol-level tool failures by checking ChatToolMessage.IsError and returning
an error when it is set, including for responses without an “Error: ” text
prefix. Preserve the existing successful result handling when the flag is false.

In `@core/schemas/mux.go`:
- Around line 851-858: Update the Responses-to-Chat allocation guard in
core/schemas/mux.go#L851-L858 to create ChatToolMessage when CallID is present
or when ResponsesToolMessage.Error is non-empty or Status is "incomplete",
preserving IsError for ID-less failures. Add corresponding error-string and
incomplete-status fixtures without CallID in
core/schemas/toolerrormux_test.go#L51-L114, asserting IsError is true for both.

🪄 Autofix

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: 996f9d94-1a15-459e-bcfb-59e193eafdf7

📥 Commits

Reviewing files that changed from the base of the PR and between cee6003 and e68d6ff.

📒 Files selected for processing (15)

• core/changelog.md
• core/mcp/agent.go
• core/mcp/codemode/starlark/executecode.go
• core/mcp/codemode/starlark/getdocs.go
• core/mcp/codemode/starlark/listfiles.go
• core/mcp/codemode/starlark/readfile.go
• core/mcp/codemode/starlark/toolerrormarker_test.go
• core/mcp/codemode/starlark/utils.go
• core/mcp/toolerrormarker_test.go
• core/mcp/toolmanager.go
• core/mcp/utils.go
• core/schemas/chatcompletions.go
• core/schemas/mux.go
• core/schemas/toolerrormux_test.go
• core/schemas/toolmessageiserror_test.go

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

core/mcp/toolerrormarker_test.go (1)

10-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use schemas.Ptr for fixture pointers.

Replace &id and &name with schemas.Ptr(id) and schemas.Ptr(name). This follows the repository pointer-helper convention for simple values.

Based on learnings, “prefer using bifrost.Ptr() to create pointers instead of the address operator (&) … including test utilities.”

Proposed change

-		ID: &id,
+		ID: schemas.Ptr(id),
 		Function: schemas.ChatAssistantMessageToolCallFunction{
-			Name:      &name,
+			Name:      schemas.Ptr(name),

🤖 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/mcp/toolerrormarker_test.go` around lines 10 - 18, Update the
testToolCall fixture to use schemas.Ptr(id) for ID and schemas.Ptr(name) for
Function.Name instead of address operators, preserving the existing
ChatAssistantMessageToolCall values and repository pointer-helper convention.

Source: Learnings

🤖 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/toolerroringress_test.go`:
- Around line 110-115: Update the success-conversion assertions in the test to
first locate a non-nil ChatToolMessage within chatReq.Input and fail if none
exists, then assert that the located message’s IsError field is nil. Ensure the
test distinguishes a missing successful tool message from one that is present
but incorrectly marked as an error.

---

Nitpick comments:
In `@core/mcp/toolerrormarker_test.go`:
- Around line 10-18: Update the testToolCall fixture to use schemas.Ptr(id) for
ID and schemas.Ptr(name) for Function.Name instead of address operators,
preserving the existing ChatAssistantMessageToolCall values and repository
pointer-helper convention.

🪄 Autofix

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: c480247c-da45-454b-8388-b79b8f553fee

📥 Commits

Reviewing files that changed from the base of the PR and between cee6003 and 413be75.

📒 Files selected for processing (16)

• core/changelog.md
• core/mcp/agent.go
• core/mcp/codemode/starlark/executecode.go
• core/mcp/codemode/starlark/getdocs.go
• core/mcp/codemode/starlark/listfiles.go
• core/mcp/codemode/starlark/readfile.go
• core/mcp/codemode/starlark/toolerrormarker_test.go
• core/mcp/codemode/starlark/utils.go
• core/mcp/toolerrormarker_test.go
• core/mcp/toolmanager.go
• core/mcp/utils.go
• core/providers/bedrock/toolerroringress_test.go
• core/schemas/chatcompletions.go
• core/schemas/mux.go
• core/schemas/toolerrormux_test.go
• core/schemas/toolmessageiserror_test.go

🚧 Files skipped from review as they are similar to previous changes (14)

• core/mcp/toolmanager.go
• core/mcp/codemode/starlark/listfiles.go
• core/mcp/codemode/starlark/toolerrormarker_test.go
• core/mcp/codemode/starlark/getdocs.go
• core/schemas/mux.go
• core/changelog.md
• core/mcp/agent.go
• core/mcp/codemode/starlark/executecode.go
• core/schemas/toolmessageiserror_test.go
• core/schemas/chatcompletions.go
• core/mcp/codemode/starlark/readfile.go
• core/schemas/toolerrormux_test.go
• core/mcp/codemode/starlark/utils.go
• core/mcp/utils.go

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[akshaydeo]

Merge activity

• Aug 6, 7:58 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Aug 6, 8:05 PM UTC: @akshaydeo merged this pull request with Graphite.

The base branch was changed.