feat: added Response API execution enpoint for MCP tools #1161
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughSplit MCP tool execution into explicit Chat and Responses flows: added ExecuteChatMCPTool and ExecuteResponsesMCPTool across core, MCP manager/tools manager, schema converters, HTTP handlers, and agent adapters; introduced bidirectional conversions and adapter boundary methods for API-neutral agent loops. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant HTTPHandler
participant Bifrost
participant Schemas
participant MCPManager
participant ToolsManager
Note over Client,HTTPHandler: HTTP POST /execute?format=chat|responses
Client->>HTTPHandler: POST tool payload
HTTPHandler->>Bifrost: ExecuteChatMCPTool(...) / ExecuteResponsesMCPTool(...)
alt Responses path
Bifrost->>Schemas: ResponsesToolMessage.ToChatAssistantMessageToolCall()
Schemas-->>Bifrost: ChatAssistantMessageToolCall
end
Bifrost->>MCPManager: ExecuteChatTool(ctx, ChatAssistantMessageToolCall)
MCPManager->>ToolsManager: ExecuteChatTool(...)
ToolsManager->>ToolsManager: invoke tool(s) / collect outputs
ToolsManager-->>MCPManager: ChatMessage (tool result)
alt Responses path
MCPManager->>Schemas: ChatMessage.ToResponsesToolMessage()
Schemas-->>Bifrost: ResponsesMessage
Bifrost-->>HTTPHandler: ResponsesMessage / BifrostError
else Chat path
MCPManager-->>Bifrost: ChatMessage
Bifrost-->>HTTPHandler: ChatMessage / BifrostError
end
HTTPHandler-->>Client: JSON response / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
Comment |
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}
There was a problem hiding this comment.
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)
transports/bifrost-http/handlers/mcp.go (1)
54-85: Update stale comment to reflect function rename.The comment on line 54 still references the old function name "executeTool", but the function has been renamed to "executeChatMCPTool". Update the comment for consistency.
🔎 Proposed fix
-// executeTool handles POST /v1/mcp/tool/execute - Execute MCP tool +// executeChatMCPTool handles POST /v1/mcp/tool/chat/execute - Execute MCP tool for Chat API func (h *MCPHandler) executeChatMCPTool(ctx *fasthttp.RequestCtx) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/bifrost.gocore/chatbot_test.gocore/mcp/agent_adaptors.gocore/mcp/agent_test.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/mux.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcp_server.gotransports/bifrost-http/server/server.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/handlers/mcp_server.gotransports/bifrost-http/server/server.gocore/schemas/mux.gocore/chatbot_test.gocore/mcp/toolmanager.gocore/bifrost.gocore/mcp/agent_adaptors.gocore/mcp/agent_test.gocore/mcp/mcp.gotransports/bifrost-http/handlers/mcp.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/handlers/mcp_server.gotransports/bifrost-http/server/server.gocore/schemas/mux.gocore/chatbot_test.gocore/mcp/toolmanager.gocore/bifrost.gocore/mcp/agent_adaptors.gocore/mcp/agent_test.gocore/mcp/mcp.gotransports/bifrost-http/handlers/mcp.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/mcp_server.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.go
🧬 Code graph analysis (7)
transports/bifrost-http/handlers/mcp_server.go (1)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)
transports/bifrost-http/server/server.go (4)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (4)
ChatMessage(108-118)BifrostError(234-240)ResponsesToolMessage(419-425)ResponsesMessage(439-455)core/schemas/bifrost.go (1)
BifrostError(466-475)core/schemas/responses.go (2)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)
core/schemas/mux.go (3)
core/schemas/responses.go (5)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)ResponsesMessageTypeFunctionCallOutput(303-303)ResponsesToolMessageOutputStruct(541-545)ResponsesMessageContentBlock(406-421)core/schemas/chatcompletions.go (4)
ChatAssistantMessageToolCall(710-716)ChatAssistantMessageToolCallFunction(719-722)ChatMessage(478-487)ChatToolMessage(644-646)core/schemas/utils.go (1)
Ptr(16-18)
core/mcp/toolmanager.go (2)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (3)
ChatMessage(108-118)ResponsesToolMessage(419-425)ResponsesMessage(439-455)
core/mcp/agent_adaptors.go (1)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)
core/mcp/mcp.go (3)
core/mcp.go (1)
MCPManager(49-56)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (3)
ChatMessage(108-118)ResponsesToolMessage(419-425)ResponsesMessage(439-455)
transports/bifrost-http/handlers/mcp.go (4)
transports/bifrost-http/lib/middleware.go (1)
ChainMiddlewares(12-24)transports/bifrost-http/handlers/utils.go (3)
SendBifrostError(47-62)SendJSON(16-22)SendError(35-44)ui/lib/types/logs.ts (1)
ResponsesToolMessage(419-425)transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(78-333)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (20)
core/schemas/mux.go (2)
116-150: LGTM! Conversion logic is sound.The conversion from ResponsesToolMessage to ChatAssistantMessageToolCall correctly handles nil checks, maps all relevant fields (CallID→ID, Name, Arguments), and defaults to empty string for Arguments when nil. This is appropriate for the Chat API format.
152-207: LGTM! Thorough conversion with proper nil handling.The conversion from ChatMessage to ResponsesMessage correctly:
- Guards against nil receivers and missing sub-structures
- Sets the appropriate message type (function_call_output)
- Maps CallID from the tool message
- Handles both string content and structured content blocks
- Populates the Output structure appropriately
core/chatbot_test.go (1)
566-566: LGTM! Call site correctly updated.The rename from
ExecuteMCPTooltoExecuteChatMCPToolcorrectly reflects the new public API surface and maintains the same behavior.core/mcp/agent_test.go (4)
486-527: LGTM! Comprehensive converter tests.The Phase 2 converter tests provide good coverage:
- Happy path conversion with all fields populated
- Nil handling validation
- Proper assertions on Type, Name, and Arguments fields
529-596: LGTM! Thorough edge case coverage.The tests cover important scenarios:
- Valid tool message conversion with output validation
- Nil receiver handling (returns nil as expected)
- Non-tool message handling (returns nil as expected)
These edge cases ensure robustness of the conversion logic.
604-670: LGTM! Excellent round-trip validation.This test comprehensively validates the conversion cycle:
- Responses format → Chat format conversion
- Tool execution simulation
- Result conversion back to Responses format
All data (CallID, Name, Arguments, Output) is verified to survive the round-trip, ensuring no information loss during format conversions.
672-708: LGTM! Structured output handling validated.The test ensures that structured content (ContentBlocks) is properly converted to Responses API output blocks, covering the non-string output path of the conversion logic.
transports/bifrost-http/handlers/mcp_server.go (2)
28-29: LGTM! Interface correctly updated for dual API support.The MCPToolManager interface now properly exposes both:
- ExecuteChatMCPTool for Chat API tool execution
- ExecuteResponsesMCPTool for Responses API tool execution
This enables the transport layer to route tool calls appropriately based on the API format.
242-242: LGTM! Call site correctly updated.The handler now calls
ExecuteChatMCPToolwhich aligns with the chat-oriented MCP server pathway, maintaining consistent behavior while adopting the new naming convention.core/mcp/toolmanager.go (3)
309-408: LGTM! Clear rename with updated documentation.The rename from
ExecuteTooltoExecuteChatToolproperly clarifies that this method works with Chat API formats. The updated documentation clearly explains:
- Primary usage for Chat API
- How Responses API users should use it (via conversion)
- Alternative wrapper available (ExecuteResponsesTool)
410-466: LGTM! Well-designed type-safe wrapper.The
ExecuteResponsesToolmethod provides an excellent type-safe wrapper that:
- Validates inputs (nil checks, required fields)
- Converts Responses format → Chat format
- Executes using the existing chat tool executor
- Converts results back to Responses format
- Handles errors at each step
The comprehensive documentation with usage examples makes this API easy to use correctly.
494-494: LGTM! Agent execution correctly wired.Both Chat and Responses agent execution paths correctly delegate to
ExecuteChatToolas the core tool executor. The Responses agent path will handle format conversions internally using the bidirectional conversion helpers.Also applies to: 525-525
core/mcp/mcp.go (3)
112-133: LGTM! Excellent documentation and clear API design.The renamed
ExecuteChatToolmethod provides:
- Clear primary usage documentation (Chat API format)
- Guidance for Responses API users (conversion steps)
- Mention of type-safe alternative (ExecuteResponsesTool)
- Proper delegation to the tool handler
The documentation makes the API surface easy to understand and use correctly.
135-145: LGTM! Clean delegation to handler.The
ExecuteResponsesToolmethod properly delegates to the tools handler, exposing the Responses API-specific tool execution to MCP manager users.
160-218: LGTM! Comprehensive agent mode documentation.The updated documentation for both agent execution methods clearly explains:
- Agent mode behavior (auto-execution, iterative loops, depth limits)
- Non-auto-executable tool handling
- For Responses API: format conversion details and feature parity
This makes it easy for users to understand how agent mode works across both API surfaces.
transports/bifrost-http/server/server.go (1)
495-503: LGTM! Server methods properly expose dual API surface.The HTTP server now exposes both:
ExecuteChatMCPToolfor Chat API tool executionExecuteResponsesMCPToolfor Responses API tool executionBoth methods properly delegate to the underlying Bifrost client, maintaining consistent error handling with
*schemas.BifrostErrorreturn types.core/bifrost.go (1)
1696-1733: LGTM! Function renaming aligns with API split.The rename from
ExecuteMCPTooltoExecuteChatMCPToolcorrectly clarifies the Chat API format, and the implementation properly delegates tomcpManager.ExecuteChatToolwith appropriate error context.core/mcp/agent_adaptors.go (2)
11-24: Excellent documentation for adapter pattern.The enhanced documentation clearly explains the adapter pattern, conversion boundaries, and how the design ensures API-neutral agent execution logic. This will help future maintainers understand the architecture.
73-86: Clear documentation of Responses API adapter conversions.The documentation effectively describes the key conversion flows for the Responses API adapter, making it easy to understand how tool calls and results are transformed between formats.
transports/bifrost-http/handlers/mcp.go (1)
45-46: LGTM! Routes correctly implement API split.The new routes properly separate Chat and Responses API tool execution endpoints, aligning with the PR objectives for feature parity.
Pratham-Mishra04
force-pushed
the
12-23-feat_added_responses_mcp_tool_execute_endpoint
branch
from
06d67a3 to
4a1a08e
Compare
Pratham-Mishra04
force-pushed
the
12-23-feat_added_binding_level_toggle_in_mcp_codemode
branch
from
8a83112 to
412e427
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/mcp/mcp.go (1)
156-199: Document context mutation behavior in agent execution.The context pointer usage is justified—context is intentionally mutated during agent execution (lines 159 and 339 in agent.go via
context.WithValue). The documentation should clarify that the context parameter may be modified during the iterative agent loop, explaining this as an intentional design choice to propagate context changes across iterations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/bifrost.gocore/chatbot_test.gocore/mcp/agent_adaptors.gocore/mcp/agent_test.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/mux.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcp_server.gotransports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/chatbot_test.go
- transports/bifrost-http/handlers/mcp_server.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/handlers/mcp.gocore/schemas/mux.gocore/bifrost.gocore/mcp/agent_adaptors.gocore/mcp/toolmanager.gotransports/bifrost-http/server/server.gocore/mcp/agent_test.gocore/mcp/mcp.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/handlers/mcp.gocore/schemas/mux.gocore/bifrost.gocore/mcp/agent_adaptors.gocore/mcp/toolmanager.gotransports/bifrost-http/server/server.gocore/mcp/agent_test.gocore/mcp/mcp.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/mcp.gotransports/bifrost-http/server/server.go
🧬 Code graph analysis (8)
transports/bifrost-http/handlers/mcp.go (3)
transports/bifrost-http/handlers/utils.go (2)
SendError(35-44)SendBifrostError(47-62)ui/lib/types/logs.ts (1)
ResponsesToolMessage(419-425)transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(78-333)
core/schemas/mux.go (3)
core/schemas/responses.go (5)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)ResponsesMessageTypeFunctionCallOutput(303-303)ResponsesToolMessageOutputStruct(541-545)ResponsesMessageContentBlock(406-421)core/schemas/chatcompletions.go (4)
ChatAssistantMessageToolCall(710-716)ChatAssistantMessageToolCallFunction(719-722)ChatMessage(478-487)ChatToolMessage(644-646)core/schemas/utils.go (1)
Ptr(16-18)
core/bifrost.go (3)
core/schemas/chatcompletions.go (2)
ChatAssistantMessageToolCall(710-716)ChatMessage(478-487)core/schemas/bifrost.go (5)
BifrostError(466-475)ErrorField(484-491)BifrostErrorExtraFields(533-537)RequestType(88-88)ResponsesRequest(96-96)core/schemas/responses.go (2)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)
core/mcp/agent_adaptors.go (1)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)
core/mcp/toolmanager.go (2)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (3)
ChatMessage(108-118)ResponsesToolMessage(419-425)ResponsesMessage(439-455)
transports/bifrost-http/server/server.go (4)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (4)
ChatMessage(108-118)BifrostError(234-240)ResponsesToolMessage(419-425)ResponsesMessage(439-455)core/schemas/bifrost.go (1)
BifrostError(466-475)core/schemas/responses.go (2)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)
core/mcp/agent_test.go (3)
core/schemas/utils.go (1)
Ptr(16-18)core/schemas/chatcompletions.go (5)
ChatMessageRoleTool(473-473)ChatToolMessage(644-646)ChatMessageRoleAssistant(470-470)ChatContentBlock(599-609)ChatContentBlockTypeText(591-591)core/schemas/responses.go (1)
ResponsesMessageTypeFunctionCallOutput(303-303)
core/mcp/mcp.go (3)
core/mcp.go (1)
MCPManager(49-56)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (3)
ChatMessage(108-118)ResponsesToolMessage(419-425)ResponsesMessage(439-455)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (18)
core/mcp/agent_test.go (2)
486-596: Excellent test coverage for basic converter paths.The Phase 2 converter tests provide solid coverage for the bidirectional conversions between Responses and Chat tool formats, including nil handling and edge cases. The test structure is clear and validates the key fields (Type, Name, Arguments, CallID, Output).
602-708: Strong round-trip conversion tests.These tests effectively validate that tool call metadata and outputs are preserved through format conversions (Responses → Chat → Responses). The structured output test specifically ensures ContentBlocks are correctly mapped to ResponsesFunctionToolCallOutputBlocks, which is critical for complex tool outputs.
transports/bifrost-http/server/server.go (1)
495-503: Clean API delegation.The new methods clearly separate Chat and Responses API tool execution paths with appropriate naming. Simple delegation to the client layer is the right pattern here.
core/mcp/toolmanager.go (3)
309-408: Appropriate method rename for API clarity.Renaming
ExecuteTooltoExecuteChatToolclarifies the Chat API format while maintaining the same implementation. The updated documentation correctly notes this is the primary executor used by both APIs.
410-466: Well-designed adapter for Responses API.The
ExecuteResponsesToolmethod provides a clean wrapper that:
- Validates input (nil checks, required fields)
- Converts between API formats using the new schema converters
- Delegates to the shared execution logic
- Handles errors at each conversion step
This design provides type safety for Responses API users while avoiding code duplication.
481-528: Consistent agent execution updates.Both agent execution methods now correctly use
ExecuteChatTool, maintaining consistency across Chat and Responses API agent modes.core/mcp/mcp.go (2)
112-145: Clear API separation with comprehensive documentation.The updated documentation for
ExecuteChatToolclearly explains its role as the primary executor for both APIs, with guidance on format conversions. The newExecuteResponsesToolprovides a convenient type-safe wrapper with proper delegation.
201-250: Comprehensive documentation for Responses API agent mode.The documentation clearly explains agent mode behavior for Responses API, including automatic format conversions and feature parity with Chat API. This helps users understand how the adapters work under the hood.
core/schemas/mux.go (2)
116-150: Correct conversion from Responses to Chat format.The conversion logic properly handles:
- Nil receiver check
- Type field (correctly set to "function")
- Field mapping (CallID → ID, Name → Name)
- Arguments handling (safe extraction with empty string default)
The implementation aligns with the test expectations in agent_test.go.
152-207: Correct conversion from Chat tool result to Responses format.The conversion properly handles:
- Nil checks (receiver, ChatToolMessage)
- Type set to function_call_output
- CallID mapping
- Output extraction from both string content and structured blocks
The dual-path output handling (ContentStr vs ContentBlocks) ensures compatibility with different tool result formats.
transports/bifrost-http/handlers/mcp.go (3)
72-103: Well-structured Chat tool execution handler.The handler properly:
- Unmarshals and validates the Chat format request
- Converts context with cleanup (defer cancel)
- Executes the tool with appropriate error handling
- Returns the tool result message
105-136: Well-structured Responses tool execution handler.The handler follows the same pattern as
executeChatMCPTool, with proper validation (Name field required), context conversion, and error handling.Note: There's an existing review comment about updating the function comment to reference the correct endpoint path.
42-51: Route implementation doesn't match PR summary: query parameter dispatch instead of separate endpoints.The code registers a single
/v1/mcp/tool/executeendpoint that routes internally based on aformatquery parameter (chatorresponses). However, the PR summary documents this as a breaking change to separate endpoints (/v1/mcp/tool/chat/executeand/v1/mcp/tool/responses/execute), which are not registered in the routing code.The implementation uses:
- Single route: POST
/v1/mcp/tool/execute- Internal dispatch in
executeTool()based on query parameter (lines 62-65)This discrepancy needs clarification: either update the PR summary to reflect the query parameter approach, or implement the documented separate endpoint routes.
core/bifrost.go (2)
1696-1733: LGTM! Clear API-specific naming for Chat Completions.The rename from
ExecuteMCPTooltoExecuteChatMCPToolclearly indicates this method is for Chat Completions API format, aligning well with the PR's goal of supporting both Chat and Responses APIs with feature parity.
1735-1771: Good implementation providing Responses API support.The new
ExecuteResponsesMCPToolfunction successfully extends MCP tool execution to the Responses API, achieving the PR's goal of feature parity between Chat Completions and Responses APIs. The error handling and structure appropriately mirrorExecuteChatMCPTool.Note: The parameter passing inconsistency between these two functions (value vs. pointer) has already been flagged in a previous review comment.
core/mcp/agent_adaptors.go (3)
11-64: Excellent adapter pattern design with comprehensive documentation.The enhanced
agentAPIAdapterinterface is well-architected to achieve API-agnostic agent execution. The documentation clearly explains:
- Format conversions are localized to adapter boundaries
- Tool execution logic remains format-agnostic
- Both APIs achieve feature parity
- The agent loop stays API-neutral
This design pattern successfully enables the PR's goal of supporting tool execution in both Chat Completions and Responses APIs with unified logic.
73-91: Well-documented adapter implementation strategy.The
responsesAPIAdapterdocumentation effectively explains how Responses API requests and responses are handled through format conversions. The specific method references (ToBifrostChatResponse(),ToResponsesMessages(),ToResponsesToolMessage()) clearly document the conversion strategy, making the implementation approach transparent and maintainable.
326-350: Smart reuse of existing conversion methods.The adapter implementations appropriately leverage existing conversion utilities (
ToBifrostChatResponse(),ToResponsesMessages()) rather than duplicating conversion logic. This approach:
- Reduces code duplication
- Maintains consistency with existing conversion behavior
- Keeps the adapter layer focused on orchestration rather than transformation details
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/bifrost.go (1)
1696-1733: Guard against nil contexts inExecuteChatMCPTool/ExecuteResponsesMCPToolto avoid panics.Other public methods on
Bifrost(e.g.,ChatCompletionRequest,ResponsesRequest) defensively normalize a nilctxtobifrost.ctx. These new MCP helpers don’t, and they passctxstraight through toMCPManager.Execute*Tool→ToolsManager.ExecuteChatTool, which usescontext.WithTimeout(ctx, …). If a caller invokes these APIs withctx == nil, you’ll hit a panic in the standard library.To keep behavior consistent and safe, add the same nil-ctx guard here:
Suggested fix
func (bifrost *Bifrost) ExecuteChatMCPTool(ctx context.Context, toolCall schemas.ChatAssistantMessageToolCall) (*schemas.ChatMessage, *schemas.BifrostError) { - if bifrost.mcpManager == nil { + if ctx == nil { + ctx = bifrost.ctx + } + if bifrost.mcpManager == nil { @@ func (bifrost *Bifrost) ExecuteResponsesMCPTool(ctx context.Context, toolCall *schemas.ResponsesToolMessage) (*schemas.ResponsesMessage, *schemas.BifrostError) { - if bifrost.mcpManager == nil { + if ctx == nil { + ctx = bifrost.ctx + } + if bifrost.mcpManager == nil {This aligns the MCP tool APIs with the rest of the surface and prevents accidental panics from nil contexts.
Also applies to: 1735-1771
🧹 Nitpick comments (4)
core/schemas/mux.go (1)
116-150: Tighten block-type mapping inToResponsesToolMessage(and consider reusing existing converter logic).The new helpers correctly preserve
CallID,Name, andArguments, and handle nil receivers safely. However, inToResponsesToolMessagethe content block types are mapped via a raw cast fromChatContentBlockTypetoResponsesMessageContentBlockType, which can produce values like"text"rather than the"output_text"/"input_text"style used elsewhere in this file (see theToResponsesMessagesconverter for non‑tool messages). That’s likely to diverge from the Responses schema expectations.Consider:
- Reusing the block‑mapping logic from
ToResponsesMessagesfor tool outputs (at least mappingChatContentBlockTypeTexttoResponsesOutputMessageContentTypeText), or- Delegating
ChatMessage→ResponsesMessageconversion for tool outputs throughToResponsesMessagesand selecting the function_call_output item, so there’s a single source of truth for block typing.This keeps function‑call output blocks schema‑consistent and avoids future drift between converters.
Also applies to: 171-207
core/mcp/agent_test.go (1)
486-528: Converter tests are solid; tiny naming nit.The new tests exercise the chat↔responses tool converters and round‑trips well, including nil and non‑tool cases, and structured outputs. Only nit:
TestResponsesToolMessageToChatAssistantMessageToolCall_Nildoesn’t actually test a nilResponsesToolMessage; consider either renaming it or adding a separate nil‑input test to match the name. Functionality itself looks correct.Also applies to: 529-571, 573-596, 602-670, 672-708
core/mcp/toolmanager.go (1)
309-408: Fix stale doc comment onExecuteResponsesTool; agent wiring looks correct.
ExecuteResponsesToolis implemented correctly (validation → Responses→Chat conversion →ExecuteChatTool→ Chat→Responses conversion) and the agent entrypoints now appropriately passm.ExecuteChatToolinto the shared loop.The exported comment block above
ExecuteResponsesToolis out of date though:
- Starts with
// ExecuteToolForResponses...while the function is now namedExecuteResponsesTool.- Mentions a
callIDparameter that no longer exists.- Still refers to wrapping “ExecuteTool” instead of
ExecuteChatTool.Updating the comment to reflect the actual function name, parameters, and underlying executor will keep godoc accurate and avoid confusion.
Also applies to: 410-466, 481-497, 513-527
core/mcp/agent_adaptors.go (1)
11-64: Responses agent adapter and aggregation helper correctly mirror chat behavior.The expanded adapter docs and the new
createResponsesResponseWithExecutedToolsAndNonAutoExecutableCallsfunction preserve Responses-specific metadata while summarizing executed tool results and surfacing non-auto-executable tool calls asfunction_callmessages. This matches the chat-side behavior and should give callers a consistent experience across APIs.There is some duplication with the chat helper (tool-results map + summary text); if these ever diverge, consider extracting a shared helper, but it’s not urgent.
Also applies to: 73-91, 373-388, 390-558
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/bifrost.gocore/chatbot_test.gocore/mcp/agent_adaptors.gocore/mcp/agent_test.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/mux.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcp_server.gotransports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (3)
- transports/bifrost-http/server/server.go
- core/chatbot_test.go
- transports/bifrost-http/handlers/mcp_server.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/mcp/mcp.gocore/schemas/mux.gocore/mcp/agent_test.gocore/mcp/toolmanager.gocore/bifrost.gocore/mcp/agent_adaptors.gotransports/bifrost-http/handlers/mcp.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/mcp/mcp.gocore/schemas/mux.gocore/mcp/agent_test.gocore/mcp/toolmanager.gocore/bifrost.gocore/mcp/agent_adaptors.gotransports/bifrost-http/handlers/mcp.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/mcp.go
🧬 Code graph analysis (6)
core/mcp/mcp.go (3)
core/mcp.go (1)
MCPManager(49-56)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (3)
ChatMessage(108-118)ResponsesToolMessage(419-425)ResponsesMessage(439-455)
core/schemas/mux.go (3)
core/schemas/responses.go (5)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)ResponsesToolMessageOutputStruct(541-545)ResponsesMessageContentBlock(406-421)ResponsesMessageContentBlockType(392-392)core/schemas/chatcompletions.go (3)
ChatAssistantMessageToolCall(710-716)ChatAssistantMessageToolCallFunction(719-722)ChatToolMessage(644-646)core/schemas/utils.go (1)
Ptr(16-18)
core/mcp/agent_test.go (3)
core/schemas/utils.go (1)
Ptr(16-18)core/schemas/chatcompletions.go (5)
ChatMessageRoleTool(473-473)ChatToolMessage(644-646)ChatMessageRoleAssistant(470-470)ChatContentBlock(599-609)ChatContentBlockTypeText(591-591)core/schemas/responses.go (1)
ResponsesMessageTypeFunctionCallOutput(303-303)
core/mcp/toolmanager.go (2)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (3)
ChatMessage(108-118)ResponsesToolMessage(419-425)ResponsesMessage(439-455)
core/bifrost.go (3)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)core/schemas/bifrost.go (5)
BifrostError(466-475)ErrorField(484-491)BifrostErrorExtraFields(533-537)RequestType(88-88)ResponsesRequest(96-96)core/schemas/responses.go (2)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)
core/mcp/agent_adaptors.go (1)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
core/mcp/mcp.go (1)
112-145: MCPManager tool-execution wrappers look good.The new
ExecuteChatTool/ExecuteResponsesToolmethods cleanly delegate to the underlyingToolsManagerand give callers explicit, format-aware entrypoints. The expanded agent-mode comments accurately describe the Chat vs Responses behavior without changing logic.No issues from this layer; any ctx‑nil handling can be centralized at higher-level callers (e.g.,
bifrost.Bifrostmethods).Also applies to: 156-168, 201-219
Pratham-Mishra04
force-pushed
the
12-23-feat_added_responses_mcp_tool_execute_endpoint
branch
from
4a1a08e to
d6c403b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
core/schemas/mux.go (1)
171-207: Structured tool outputs drop richer block metadata
ToResponsesToolMessagemapscm.Content.ContentBlockstoResponsesMessageContentBlockby copying onlyTypeandText. If tool outputs ever include images, files, or audio blocks, their specific subfields will be lost here (unlike the more complete mapping inToResponsesMessagesbelow).If tool outputs are expected to use non‑text blocks, consider mirroring the richer block‑type conversions from
ToResponsesMessagesfor consistency; otherwise this is fine as a text‑only helper.core/mcp/toolmanager.go (1)
410-466: Docblock forExecuteResponsesToolis stale vs. actual APIThe comment still refers to
ExecuteToolForResponses, describes it as a wrapper aroundExecuteTool, and the example shows an extracallIDparameter, but the current function isExecuteResponsesTool(ctx, toolMessage *schemas.ResponsesToolMessage)and delegates toExecuteChatTool.Update the docblock (name, description, and example) to match the real signature and underlying call to
ExecuteChatToolto avoid confusion.core/mcp/mcp.go (1)
112-145: MCPManager tool execution wrappers are wired correctly
ExecuteChatToolandExecuteResponsesToolcleanly delegate to the underlyingToolsManagerand provide a clear split between Chat and Responses formats. You might optionally rephrase the comments to recommend these MCPManager methods directly (rather than referring to ToolsManager) since they’re the public entry points, but behavior-wise this looks solid.core/bifrost.go (1)
1696-1733: Align MCP tool APIs’ctxhandling with other Bifrost public methodsMost Bifrost public APIs treat a nil
ctxdefensively (if ctx == nil { ctx = bifrost.ctx }), butExecuteChatMCPToolandExecuteResponsesMCPToolpassctxstraight through to the MCP manager. If a caller accidentally passes nil here,context.WithTimeoutinside the tool execution path will panic.Consider normalizing
ctxthe same way:Suggested adjustment
func (bifrost *Bifrost) ExecuteChatMCPTool(ctx context.Context, toolCall schemas.ChatAssistantMessageToolCall) (*schemas.ChatMessage, *schemas.BifrostError) { + if ctx == nil { + ctx = bifrost.ctx + } if bifrost.mcpManager == nil { // ... } result, err := bifrost.mcpManager.ExecuteChatTool(ctx, toolCall) // ... } func (bifrost *Bifrost) ExecuteResponsesMCPTool(ctx context.Context, toolCall *schemas.ResponsesToolMessage) (*schemas.ResponsesMessage, *schemas.BifrostError) { + if ctx == nil { + ctx = bifrost.ctx + } if bifrost.mcpManager == nil { // ... } result, err := bifrost.mcpManager.ExecuteResponsesTool(ctx, toolCall) // ... }Also applies to: 1735-1771
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/bifrost.gocore/chatbot_test.gocore/mcp/agent_adaptors.gocore/mcp/agent_test.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/mux.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcp_server.gotransports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (5)
- core/chatbot_test.go
- core/mcp/agent_test.go
- transports/bifrost-http/handlers/mcp_server.go
- transports/bifrost-http/server/server.go
- core/mcp/agent_adaptors.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/schemas/mux.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/bifrost.gotransports/bifrost-http/handlers/mcp.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/schemas/mux.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/bifrost.gotransports/bifrost-http/handlers/mcp.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/mcp.go
🧬 Code graph analysis (4)
core/schemas/mux.go (3)
core/schemas/responses.go (5)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)ResponsesMessageTypeFunctionCallOutput(303-303)ResponsesToolMessageOutputStruct(541-545)ResponsesMessageContentBlock(406-421)core/schemas/chatcompletions.go (4)
ChatAssistantMessageToolCall(710-716)ChatAssistantMessageToolCallFunction(719-722)ChatMessage(478-487)ChatToolMessage(644-646)core/schemas/utils.go (1)
Ptr(16-18)
core/mcp/toolmanager.go (2)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (3)
ChatMessage(108-118)ResponsesToolMessage(419-425)ResponsesMessage(439-455)
core/bifrost.go (3)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)core/schemas/bifrost.go (5)
BifrostError(466-475)ErrorField(484-491)BifrostErrorExtraFields(533-537)RequestType(88-88)ResponsesRequest(96-96)core/schemas/responses.go (2)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)
transports/bifrost-http/handlers/mcp.go (3)
transports/bifrost-http/handlers/utils.go (3)
SendError(35-44)SendBifrostError(47-62)SendJSON(16-22)ui/lib/types/logs.ts (1)
ResponsesToolMessage(419-425)transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(78-333)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (5)
core/mcp/toolmanager.go (2)
309-323: ExecuteChatTool public surface looks consistentRenamed
ExecuteTool→ExecuteChatToolwith a clear docstring and unchanged core behavior; signature matches the agent adapter’s expected executor type, so this change looks good.
481-497: Agent executors correctly switched toExecuteChatToolBoth
ExecuteAgentForChatRequestandExecuteAgentForResponsesRequestnow receivem.ExecuteChatToolas the executor, which aligns with the new primary tool execution path and keeps the agent logic API‑agnostic. No issues here.Also applies to: 513-527
transports/bifrost-http/handlers/mcp.go (3)
55-66: Format-based dispatcher now defaults to chat and validates cleanlyThe
executeToolswitch on a lowercasedformat(with""falling back to chat) matches the intended behavior and returns a clear 400 for invalid values. This resolves the prior inconsistency around treating a missing format as an error.
68-99: Chat MCP tool handler validates input and integrates correctly
executeChatMCPTooldoes strict JSON parsing, checksFunction.Name, usesConvertToBifrostContextwithallowDirectKeys=false, and sends eitherSendBifrostErroror the rawChatMessageas JSON. The flow intoExecuteChatMCPToolis sound; no issues spotted.
101-131: Responses MCP tool handler mirrors chat path appropriately
executeResponsesMCPToolmirrors the chat handler: JSON body →ResponsesToolMessage, validatesName, converts context (no direct keys), callsExecuteResponsesMCPTool, and returns the resultingResponsesMessageas JSON. Given the new mux converters, this completes the Responses API tool execution path cleanly.
Pratham-Mishra04
force-pushed
the
12-23-feat_added_responses_mcp_tool_execute_endpoint
branch
from
d6c403b to
f2f39bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/bifrost.go (1)
1696-1733: LGTM! Function rename and implementation are correct.The renamed
ExecuteChatMCPToolfunction correctly handles Chat API tool execution with appropriate validation and error handling. The breaking change is documented in the PR objectives.Note: The parameter passing inconsistency with
ExecuteResponsesMCPTool(value vs pointer) that was flagged in previous reviews is still present. While this doesn't break functionality, consider standardizing the approach across both functions for API consistency.
🧹 Nitpick comments (1)
core/mcp/mcp.go (1)
135-145: Fix incomplete documentation format.The documentation for
ExecuteResponsesToolis missing proper formatting. There should be a blank line between the description and the "Parameters:" section, and the description appears to be cut off or malformed (line 136 is missing context).🔎 Suggested documentation fix
-// ExecuteResponsesTool executes a single tool call and returns the result as a responses message. - +// ExecuteResponsesTool executes a single tool call and returns the result as a responses message. +// This is a type-safe wrapper around ExecuteChatTool that handles format conversions automatically. +// // - ctx: Context for the tool execution // - toolCall: The tool call to execute in Responses API format //
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/bifrost.gocore/chatbot_test.gocore/mcp/agent_adaptors.gocore/mcp/agent_test.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/mux.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcp_server.gotransports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/handlers/mcp_server.go
- core/chatbot_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/mcp/mcp.gotransports/bifrost-http/server/server.gocore/mcp/agent_test.gocore/schemas/mux.gocore/mcp/toolmanager.gotransports/bifrost-http/handlers/mcp.gocore/bifrost.gocore/mcp/agent_adaptors.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/mcp/mcp.gotransports/bifrost-http/server/server.gocore/mcp/agent_test.gocore/schemas/mux.gocore/mcp/toolmanager.gotransports/bifrost-http/handlers/mcp.gocore/bifrost.gocore/mcp/agent_adaptors.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.go
🧬 Code graph analysis (3)
core/mcp/mcp.go (3)
core/mcp.go (1)
MCPManager(49-56)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (3)
ChatMessage(108-118)ResponsesToolMessage(419-425)ResponsesMessage(439-455)
core/mcp/toolmanager.go (2)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (3)
ChatMessage(108-118)ResponsesToolMessage(419-425)ResponsesMessage(439-455)
core/mcp/agent_adaptors.go (1)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (6)
core/bifrost.go (1)
1735-1771: LGTM! New Responses API tool execution function is well-implemented.The
ExecuteResponsesMCPToolfunction correctly implements tool execution for the Responses API format, with proper validation, error handling, and context tagging. The implementation achieves the PR's goal of adding Responses API tool execution parity.transports/bifrost-http/handlers/mcp.go (3)
53-66: LGTM! Clean format-based routing with sensible defaults.The dispatcher correctly handles the format query parameter with case-insensitive matching, defaults to "chat" when empty, and provides clear error messages for invalid values. This addresses previous review concerns.
68-99: LGTM! Chat format handler is correctly implemented.The handler properly unmarshals, validates, and executes Chat API tool calls with appropriate error handling and context management. The validation logic correctly checks the nested
Function.Namefield.
101-132: LGTM! Responses format handler is correctly implemented.The handler properly unmarshals, validates, and executes Responses API tool calls. The validation logic correctly checks the
Namefield directly (not nested like Chat API), which matches theResponsesToolMessagestructure.core/mcp/agent_adaptors.go (2)
11-64: Excellent adapter pattern design with comprehensive documentation.The expanded
agentAPIAdapterinterface is well-documented and enables API-neutral tool execution. The boundary methods clearly delineate where format conversions occur, and the documentation explains the conversion strategy effectively. This design achieves feature parity between Chat and Responses APIs as stated in the PR objectives.
73-87: LGTM! Clear documentation for Responses API adapter.The documentation block effectively explains how the
responsesAPIAdapterhandles format conversions and enables feature parity. References to specific conversion methods (ToBifrostChatResponse,ToResponsesMessages) help developers understand the implementation.
Pratham-Mishra04
force-pushed
the
12-23-feat_added_binding_level_toggle_in_mcp_codemode
branch
from
412e427 to
f26d081
Compare
Pratham-Mishra04
force-pushed
the
12-23-feat_added_responses_mcp_tool_execute_endpoint
branch
from
f2f39bc to
fab6ecc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/mcp/toolmanager.go (1)
309-337: Fix stale docstring forExecuteResponsesToolThe comment block above
ExecuteResponsesToolis inconsistent with the function:
- It’s still titled
ExecuteToolForResponsesand refers to acallIDparameter (Line 422) that no longer exists.- The function name in the doc and the listed parameters should be updated to match the actual signature.
Updating the docstring to use
ExecuteResponsesTooland removing the straycallIDparameter description will avoid confusion for callers.Suggested doc-only fix
-// ExecuteToolForResponses executes a tool call from a Responses API tool message and returns -// the result in Responses API format. This is a type-safe wrapper around ExecuteTool that -// handles the conversion between Responses and Chat API formats. +// ExecuteResponsesTool executes a tool call from a Responses API tool message and returns +// the result in Responses API format. This is a type-safe wrapper around ExecuteChatTool that +// handles the conversion between Responses and Chat API formats. @@ -// - toolMessage: The Responses API tool message to execute -// - callID: The original call ID from the Responses API +// - toolMessage: The Responses API tool message to executeAlso applies to: 410-466
core/bifrost.go (1)
1696-1733: Guard MCP tool execution APIs against nil contextsThe new
ExecuteChatMCPToolandExecuteResponsesMCPToolmethods don’t handle a nilctx, butToolsManager.ExecuteChatTool/ExecuteResponsesToolultimately callcontext.WithTimeout(ctx, …), which will panic ifctxis nil. Other public APIs (e.g.,ChatCompletionRequest,ResponsesRequest) defensively replace a nil context withbifrost.ctx.To keep behavior consistent and avoid a potential panic if callers accidentally pass
nil, consider normalizing the context at the top of both methods.Suggested defensive fix
func (bifrost *Bifrost) ExecuteChatMCPTool(ctx context.Context, toolCall schemas.ChatAssistantMessageToolCall) (*schemas.ChatMessage, *schemas.BifrostError) { + if ctx == nil { + ctx = bifrost.ctx + } if bifrost.mcpManager == nil { @@ func (bifrost *Bifrost) ExecuteResponsesMCPTool(ctx context.Context, toolCall *schemas.ResponsesToolMessage) (*schemas.ResponsesMessage, *schemas.BifrostError) { + if ctx == nil { + ctx = bifrost.ctx + } if bifrost.mcpManager == nil {Also applies to: 1735-1771
🧹 Nitpick comments (3)
core/mcp/agent_test.go (1)
5-5: Strengthen converter tests for IDs and structured outputThe new converter tests look solid and exercise the mux helpers well. A couple of small improvements would make them more robust:
- Line 490+: In
TestResponsesToolMessageToChatAssistantMessageToolCall, you don’t assert thatCallIDonResponsesToolMessageis propagated toChatAssistantMessageToolCall.ID. Given you setCallIDto"call-123", adding an assertion onchatToolCall.IDwould guard against regressions in ID handling.- Line 685+: In
TestExecuteAgentForResponsesRequest_OutputStructured, you only assert the number of output blocks. Verifying the blockTypeandTextcontents match"Block 1"/"Block 2"would catch ordering or mapping bugs inToResponsesToolMessage().These are low-effort test tweaks that tighten guarantees around the new conversion paths.
Also applies to: 487-719
core/mcp/agent_adaptors.go (2)
11-64: Document conversation element types to avoid future panics
agentAPIAdapterassumes thatconversation []interface{}contains onlyschemas.ChatMessage(chat adapter) orschemas.ResponsesMessage(responses adapter), and both adapters type-assert accordingly (Lines 139-144, 355-357). That’s correct today but fragile if additional element types are ever added.Consider adding a brief comment near the interface or the
createNewRequestmethods clarifying that the conversation slice must only contain the API’s message type, so future changes don’t accidentally introduce mixed types and runtime panics.
373-388: Verify agent loop termination semantics for Responses final response
createResponsesResponseWithExecutedToolsAndNonAutoExecutableCallsmirrors the chat variant: it summarizesexecutedToolResultsinto a text assistant message and appends each non-auto-executable tool call as a separatefunction_callResponsesMessage(Lines 508-555).Because those messages have
Type = function_calland non-nilResponsesToolMessage,hasToolCallsForResponsesResponsewill see tool calls on the resulting response. That’s fine as long as the agent loop distinguishes auto-executable vs non-auto-executable calls and does not re-enter execution based solely onhasToolCallsForResponsesResponse.Please double-check that the responses agent loop:
- Treats
nonAutoExecutableToolCallsas terminal (i.e., returns this response without another iteration), and- Does not rely only on
hasToolCallsForResponsesResponseafter this helper runs.Adding a unit test that covers the “mixed” case (some auto-executable tools executed, some left in
nonAutoExecutableToolCalls) would lock in the intended behavior.Also applies to: 404-558
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/bifrost.gocore/chatbot_test.gocore/mcp/agent_adaptors.gocore/mcp/agent_test.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/mux.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcp_server.gotransports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/schemas/mux.go
- core/chatbot_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp_server.gocore/mcp/agent_adaptors.gocore/mcp/mcp.gotransports/bifrost-http/handlers/mcp.gocore/mcp/agent_test.gocore/bifrost.gocore/mcp/toolmanager.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp_server.gocore/mcp/agent_adaptors.gocore/mcp/mcp.gotransports/bifrost-http/handlers/mcp.gocore/mcp/agent_test.gocore/bifrost.gocore/mcp/toolmanager.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp_server.gotransports/bifrost-http/handlers/mcp.go
🧬 Code graph analysis (7)
transports/bifrost-http/server/server.go (4)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (4)
ChatMessage(108-118)BifrostError(234-240)ResponsesToolMessage(419-425)ResponsesMessage(439-455)core/schemas/bifrost.go (1)
BifrostError(466-475)core/schemas/responses.go (2)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)
transports/bifrost-http/handlers/mcp_server.go (1)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)
core/mcp/agent_adaptors.go (1)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)
transports/bifrost-http/handlers/mcp.go (3)
transports/bifrost-http/handlers/utils.go (3)
SendError(35-44)SendBifrostError(47-62)SendJSON(16-22)ui/lib/types/logs.ts (1)
ResponsesToolMessage(419-425)transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(78-333)
core/mcp/agent_test.go (3)
core/schemas/utils.go (1)
Ptr(16-18)core/schemas/chatcompletions.go (5)
ChatMessageRoleTool(473-473)ChatToolMessage(644-646)ChatMessageRoleAssistant(470-470)ChatContentBlock(599-609)ChatContentBlockTypeText(591-591)core/schemas/responses.go (1)
ResponsesMessageTypeFunctionCallOutput(303-303)
core/bifrost.go (3)
core/schemas/chatcompletions.go (2)
ChatAssistantMessageToolCall(710-716)ChatMessage(478-487)core/schemas/bifrost.go (5)
BifrostError(466-475)ErrorField(484-491)BifrostErrorExtraFields(533-537)RequestType(88-88)ResponsesRequest(96-96)core/schemas/responses.go (2)
ResponsesToolMessage(472-492)ResponsesMessage(321-334)
core/mcp/toolmanager.go (2)
core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(710-716)ui/lib/types/logs.ts (3)
ChatMessage(108-118)ResponsesToolMessage(419-425)ResponsesMessage(439-455)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (5)
core/mcp/mcp.go (1)
112-145: MCPManager tool execution split looks clean and consistentThe new
ExecuteChatTool/ExecuteResponsesToolmethods are thin, well-documented wrappers overToolsManager, and the expanded agent-mode comments clarify behavior and parity between Chat and Responses nicely. No functional issues spotted here.Also applies to: 156-219
transports/bifrost-http/handlers/mcp_server.go (1)
25-30: Interface and call-site updates for MCP tool execution are coherentThe
MCPToolManagerinterface extension and the switch toExecuteChatMCPToolin the server handler align with the new split between Chat and Responses execution paths. The error handling aroundExecuteChatMCPToolremains intact, and there’s no change to MCP server semantics for tool results. Looks good.Also applies to: 224-243
transports/bifrost-http/server/server.go (1)
490-498: Server-level MCP tool wrappers are correctly wired
ExecuteChatMCPToolandExecuteResponsesMCPToolare thin pass-throughs to the core client and match theMCPToolManagerinterface used by HTTP handlers. No issues with signatures or error propagation.transports/bifrost-http/handlers/mcp.go (2)
91-91: No action needed — the pointer usage is intentional and correct.The method signatures intentionally differ:
ExecuteChatMCPToolacceptsschemas.ChatAssistantMessageToolCallby value, whileExecuteResponsesMCPToolaccepts*schemas.ResponsesToolMessageby pointer. The calls at lines 91 and 124 correctly match their respective method signatures.Likely an incorrect or invalid review comment.
45-45: The pointer usage in the two execute calls correctly matches their respective method signatures and is not an inconsistency.ExecuteChatMCPToolexpects a value parameter, and line 91 correctly passesreqas a value.ExecuteResponsesMCPToolexpects a pointer parameter, and line 124 correctly passes&reqas a pointer. The different handling is by design based on the method signatures incore/bifrost.go.The single endpoint
/v1/mcp/tool/executewith format query parameter routing is a valid API design pattern. The implementation correctly routes requests based on the format parameter ("chat" or "responses") to the appropriate handler.Likely an incorrect or invalid review comment.
Merge activity
|
akshaydeo
changed the base branch from
12-23-feat_added_binding_level_toggle_in_mcp_codemode
to
graphite-base/1161
akshaydeo
deleted the
12-23-feat_added_responses_mcp_tool_execute_endpoint
branch
Summary
Add support for tool execution in the Responses API, enabling agent mode and tool execution for both Chat Completions and Responses APIs with full feature parity.
Changes
ExecuteMCPTooltoExecuteChatMCPToolto clarify it works with Chat API formatExecuteResponsesMCPToolto support tool execution with Responses API formatType of change
Affected areas
How to test
Test tool execution with both Chat and Responses APIs:
Breaking changes
The
ExecuteMCPToolmethod has been renamed toExecuteChatMCPTool. Any code directly calling this method will need to be updated. The HTTP endpoint has also changed from/v1/mcp/tool/executeto/v1/mcp/tool/chat/execute.Related issues
Enables Responses API to have full feature parity with Chat API for tool execution and agent mode.
Security considerations
No new security implications. Tool execution permissions and sandboxing remain unchanged.
Checklist