feat: added MCP support #94
Conversation
|
Warning Rate limit exceeded@Pratham-Mishra04 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces MCP (Model Context Protocol) integration into the Bifrost system. It adds new configuration structures, a manager for MCP tool discovery and execution, public APIs for tool registration and invocation, and comprehensive documentation. The README and codebase are updated to reflect these changes, enabling dynamic tool integration from local and external MCP servers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bifrost
participant MCPManager
participant MCPClient/Server
User->>Bifrost: Submit LLM request
Bifrost->>MCPManager: Augment request with MCP tools
MCPManager->>MCPClient/Server: Discover available tools
Bifrost->>PluginHooks: Run plugin hooks (if any)
User->>Bifrost: Approve tool call (if needed)
Bifrost->>MCPManager: Execute MCP tool call
MCPManager->>MCPClient/Server: Route tool execution
MCPClient/Server-->>MCPManager: Return tool result
MCPManager-->>Bifrost: Return tool response
Bifrost-->>User: Return LLM/tool response
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
core/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
README.md(2 hunks)core/bifrost.go(6 hunks)core/go.mod(1 hunks)core/mcp.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mcp.go(1 hunks)docs/mcp.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/mcp.md
[uncategorized] ~779-~779: Loose punctuation mark.
Context: ...ent Filtering - "mcp_include_clients": Whitelist specific clients for a reques...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~842-~842: Possible typo: you repeated a word.
Context: ...ry**: Only applies when include list is empty 3. Empty filters: All configured clients are a...
(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~893-~893: Possible subject-verb agreement error detected: Did you mean to use the plural form here?
Context: ...** (mcp_exclude_clients) - Applied if no include list 3. **Config-level tool whi...
(SUBJECT_NUMBER)
[duplication] ~1127-~1127: Possible typo: you repeated a word.
Context: ...ew ToolsToSkip configuration for each client - Client connection issues: Verify external MC...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
docs/mcp.md
17-17: Link fragments should be valid
null
(MD051, link-fragments)
533-533: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
638-638: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
680-680: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
990-990: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
1005-1005: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
1045-1045: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
1078-1078: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1091-1091: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1106-1106: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1120-1120: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1133-1133: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1147-1147: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1192-1192: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 golangci-lint (1.64.8)
core/mcp.go
481-481: Error return value of client.StdioCommand.Process.Kill is not checked
(errcheck)
482-482: Error return value of client.StdioCommand.Wait is not checked
(errcheck)
🔇 Additional comments (4)
README.md (1)
213-214: Documentation looks good.The README correctly surfaces the new feature and links to detailed docs. 👍
Also applies to: 284-285
core/go.mod (1)
27-58: The previousgo mod whycalls didn’t run in the module root, so they all errored. Let’s rerun them undercore/:#!/bin/bash set -e cd core # Check why these indirect modules remain for m in \ github.com/gin-gonic/gin \ github.com/go-playground/validator/v10 \ github.com/leodido/go-urn; do echo "== $m ==" go mod why -m "$m" || true donecore/bifrost.go (1)
240-249: Init fallback silently degrades when MCP manager fails.
newMCPManagerfailure only logs a warning. If the user supplied anMCPConfig, it probably should be a hard error (Initreturningerr) rather than continuing in a partially-configured state.core/mcp.go (1)
598-605: Verify stdin/stdout ordering for STDIO transport
stdio.NewStdioServerTransportWithIO(stdout, stdin)assumesstdoutis the reader andstdinthe writer. Double-check the library’s signature—if the order is(writer, reader)this will deadlock.
Pratham-Mishra04
force-pushed
the
06-18-feat_added_mcp_support
branch
from
1f6a97c to
6264848
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
core/schemas/mcp.go (2)
15-16: Add validation for connection-specific configuration.The struct allows both
HTTPConnectionStringandStdioConfigto be nil, but exactly one must be set based on theConnectionType. Consider adding aValidate()method to enforce this constraint at configuration load time.// Validate ensures the MCPClientConfig is properly configured func (c *MCPClientConfig) Validate() error { switch c.ConnectionType { case MCPConnectionTypeHTTP: if c.HTTPConnectionString == nil { return fmt.Errorf("HTTPConnectionString is required for HTTP connection type") } if c.StdioConfig != nil { return fmt.Errorf("StdioConfig should not be set for HTTP connection type") } case MCPConnectionTypeSTDIO: if c.StdioConfig == nil { return fmt.Errorf("StdioConfig is required for STDIO connection type") } if c.HTTPConnectionString != nil { return fmt.Errorf("HTTPConnectionString should not be set for STDIO connection type") } default: return fmt.Errorf("invalid connection type: %s", c.ConnectionType) } return nil }
17-18: Add validation for mutually exclusive tool filters.Both
ToolsToSkipandToolsToExecutecan be set simultaneously, creating ambiguous filtering behavior. Add validation to ensure only one filtering approach is used.// ValidateToolFilters ensures tool filtering configuration is unambiguous func (c *MCPClientConfig) ValidateToolFilters() error { if len(c.ToolsToSkip) > 0 && len(c.ToolsToExecute) > 0 { return fmt.Errorf("ToolsToSkip and ToolsToExecute are mutually exclusive - use only one") } return nil }docs/mcp.md (2)
529-534: Add language identifiers to fenced code blocksMultiple fenced code blocks lack language identifiers, causing markdown linters to flag them and reducing syntax highlighting quality.
Add appropriate language identifiers such as
text,console, ormermaidto improve readability and silence linter warnings:-``` +```textAlso applies to sequence diagrams around lines 634-688 and 676-688, and console output examples in the troubleshooting section.
Also applies to: 634-688, 676-688, 1074-1076, 1087-1089, 1102-1104, 1116-1118, 1129-1131, 1143-1145, 1188-1190
775-775: Fix grammar and punctuation issuesSeveral grammatical errors and loose punctuation marks need correction:
- Line 775: Remove the loose punctuation mark before "Whitelist"
- Line 838: Fix word duplication ("include list list" → "include list")
- Line 889: Subject-verb agreement issue with "include list"
- Line 1123: Remove duplicated word in "each client - **Client"
Apply these fixes to improve readability and pass language tool checks.
Also applies to: 838-838, 889-889, 1123-1123
core/mcp.go (2)
199-222: Prevent duplicate local tool registrationThe tool registration silently overwrites existing tools in the ToolMap without checking for duplicates, potentially causing confusion and masking registration conflicts.
Add an existence check before registration:
m.mu.Lock() defer m.mu.Unlock() +// Check for existing tool registration +if _, exists := m.clientMap[BifrostMCPClientKey].ToolMap[name]; exists { + return fmt.Errorf("tool '%s' is already registered", name) +} m.logger.Info(fmt.Sprintf("%s Registering typed tool: %s", MCPLogPrefix, name))
404-465: Reduce lock contention in connection setupThe
connectToMCPClientmethod holds a global lock while performing heavy I/O operations (process spawning, network connections, client initialization), which blocks all other MCP operations.Refactor to minimize lock duration:
func (m *MCPManager) connectToMCPClient(config schemas.MCPClientConfig) error { + // Short lock to check/create client entry m.mu.Lock() - defer m.mu.Unlock() // Initialize or validate client entry if existingClient, exists := m.clientMap[config.Name]; exists { if existingClient.Conn != nil { + m.mu.Unlock() return fmt.Errorf("client %s already has an active connection", config.Name) } existingClient.ConnectionInfo.Type = config.ConnectionType } else { m.clientMap[config.Name] = &MCPClient{ Name: config.Name, ExecutionConfig: config, ToolMap: make(map[string]schemas.Tool), ConnectionInfo: MCPClientConnectionInfo{ Type: config.ConnectionType, }, } } + m.mu.Unlock() var externalClient *mcp_golang.Client var err error // Heavy operations without lock switch config.ConnectionType { // ... connection creation ... } // ... initialization and tool registration ... + // Short lock to update final connection + m.mu.Lock() m.clientMap[config.Name].Conn = externalClient + m.mu.Unlock() return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
core/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
README.md(2 hunks)core/bifrost.go(6 hunks)core/go.mod(1 hunks)core/mcp.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mcp.go(1 hunks)docs/mcp.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
core/schemas/bifrost.go (1)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/schemas/bifrost.go:20-23
Timestamp: 2025-06-18T15:16:23.104Z
Learning: In the Bifrost project, BifrostConfig struct is never marshaled/unmarshaled, so serialization tags (json, yaml) are not needed for its fields.
core/bifrost.go (1)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/bifrost.go:823-845
Timestamp: 2025-06-18T15:15:51.303Z
Learning: In the Bifrost project, the team prioritizes maintaining consistent error handling patterns over exposing detailed error context. All errors should be wrapped in the standard `BifrostError` structure rather than creating specific error types or exposing richer error details like exit codes or stderr output.
🪛 LanguageTool
docs/mcp.md
[uncategorized] ~775-~775: Loose punctuation mark.
Context: ...ent Filtering - "mcp_include_clients": Whitelist specific clients for a reques...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~838-~838: Possible typo: you repeated a word.
Context: ...ry**: Only applies when include list is empty 3. Empty filters: All configured clients are a...
(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~889-~889: Possible subject-verb agreement error detected: Did you mean to use the plural form here?
Context: ...** (mcp_exclude_clients) - Applied if no include list 3. **Config-level tool whi...
(SUBJECT_NUMBER)
[duplication] ~1123-~1123: Possible typo: you repeated a word.
Context: ...ew ToolsToSkip configuration for each client - Client connection issues: Verify external MC...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
docs/mcp.md
17-17: Link fragments should be valid
null
(MD051, link-fragments)
529-529: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
634-634: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
676-676: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
986-986: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
1001-1001: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
1041-1041: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
1074-1074: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1087-1087: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1102-1102: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1116-1116: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1129-1129: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1143-1143: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1188-1188: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (14)
README.md (2)
213-213: Documentation update looks good.The addition of MCP integration to the features list accurately reflects the new functionality being introduced.
284-284: Proper documentation reference added.The link to the MCP documentation follows the existing pattern and provides users with detailed information about the new feature.
core/schemas/bifrost.go (1)
22-22: MCP configuration field properly integrated.The addition of the
MCPConfigfield toBifrostConfigfollows the existing pattern and enables optional MCP configuration as intended.core/go.mod (1)
27-58: Dependencies appropriately added for MCP integration.The new indirect dependencies support the MCP functionality, including the core MCP library (
github.com/metoro-io/mcp-golang) and supporting libraries for JSON processing, HTTP handling, validation, and other MCP-related operations.core/bifrost.go (6)
51-51: MCP manager field properly added.The
mcpManagerfield is correctly positioned and typed as a pointer to allow for optional MCP configuration.
240-249: MCP initialization handled appropriately.The MCP manager initialization correctly checks for configuration presence, handles errors gracefully with warning logs, and provides informative success logging.
615-618: MCP tool augmentation properly positioned.The MCP tools are correctly added to requests before plugin hooks execute, ensuring that plugins can operate on the augmented request. The identical implementation in both
tryTextCompletionandtryChatCompletionis acceptable based on past feedback.Also applies to: 746-749
813-845: Public MCP execution API well-designed.The
ExecuteMCPToolmethod provides a clean public interface for manual tool execution with proper error handling that maintains consistency with the existingBifrostErrorpattern.
847-876: Tool registration API is user-friendly.The
RegisterMCPToolmethod provides a clean interface for registering custom tools with comprehensive documentation and example usage in the comments.
893-899: MCP cleanup properly integrated.The cleanup logic correctly handles MCP manager disposal with appropriate error logging, ensuring resources are properly released during shutdown.
core/schemas/mcp.go (1)
7-7: ServerPort type correctly implemented.The
ServerPortfield is properly defined as*intrather than a string, avoiding runtime parsing issues and ensuring type safety.docs/mcp.md (1)
1-1190: Excellent comprehensive documentationThis documentation provides thorough coverage of the MCP integration feature including:
- Clear architecture explanations with helpful diagrams
- Comprehensive API reference
- Practical usage examples
- Security best practices
- Detailed troubleshooting guide
The content accurately reflects the implementation and provides valuable guidance for developers integrating MCP tools.
core/mcp.go (2)
1-849: Well-architected MCP integrationThis implementation provides a solid foundation for MCP integration with good separation of concerns:
- Proper abstraction between local and external MCP clients
- Comprehensive tool filtering and discovery mechanisms
- Robust error handling and logging throughout
- Clean resource management with proper cleanup
The concurrent design appropriately handles multiple MCP connections while maintaining thread safety where it has been addressed.
486-502: Improve process cleanup timeout handlingThe cleanup logic uses a 10-second timeout but doesn't handle the case where the process becomes a zombie. After the timeout, the process reference should be cleared to prevent resource leaks.
select { case err := <-done: // Process exited within timeout if err != nil && !isExpectedKillError(err) { m.logger.Warn(fmt.Sprintf("%s STDIO process %d exited with unexpected error: %v", MCPLogPrefix, client.StdioCommand.Process.Pid, err)) } m.logger.Info(fmt.Sprintf("%s STDIO process %d terminated successfully", MCPLogPrefix, client.StdioCommand.Process.Pid)) case <-time.After(10 * time.Second): // Process didn't exit within timeout - this is concerning but we can't wait forever m.logger.Warn(fmt.Sprintf("%s STDIO process %d did not terminate within 10 seconds after kill signal", MCPLogPrefix, client.StdioCommand.Process.Pid)) + // Clear the process reference to prevent further operations on a potentially zombie process + client.StdioCommand = nil }Likely an incorrect or invalid review comment.
Pratham-Mishra04
force-pushed
the
06-18-feat_added_mcp_support
branch
from
6264848 to
eb9fb9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
docs/mcp.md (3)
528-528: Add missing language identifiers to fenced code blocksMultiple fenced code blocks lack language identifiers, causing markdown linter warnings. This was supposedly addressed in a previous commit, but the issues persist.
Add appropriate language identifiers to improve syntax highlighting and fix linter warnings:
-``` +```text 1. User Message -> "Can you read config.json?" 2. Assistant Message -> [with tool_calls to read_file] 3. Tool Result(s) -> [file contents] 4. Assistant Message -> [final response with no tool_calls] -``` +``` -``` +```text ┌──────────────────────────────────────────────────────────────┐ │ Bifrost Core │ -``` +``` -``` +```text User Request │ ▼ -``` +``` Apply similar fixes to all other unmarked code blocks (error messages, logs, etc.) by adding `text` or `console` identifiers. Also applies to: 633-633, 675-675, 1073-1073, 1086-1086, 1101-1101, 1115-1115, 1128-1128, 1142-1142, 1187-1187 --- `985-985`: **Convert emphasis to proper markdown headings** Several sections use bold emphasis instead of proper markdown headings, which reduces document structure clarity and triggers linter warnings. Convert the emphasized text to proper heading levels: ```diff -**1. Default to Restrictive Filtering** +#### 1. Default to Restrictive Filtering -**2. Environment-based Tool Control** +#### 2. Environment-based Tool Control -**3. User Role-Based Filtering** +#### 3. User Role-Based FilteringNote: The author previously indicated this was "okay to leave" in past reviews, but proper heading structure is important for document navigation and accessibility.
Also applies to: 1000-1000, 1040-1040
774-774: Fix grammar and duplication issuesSeveral grammar and word duplication issues remain despite being flagged in previous reviews.
Apply these corrections:
-Filtering - `"mcp_include_clients"`: Whitelist specific clients for a reques... +Filtering - `"mcp_include_clients"`: Whitelist specific clients for a request... -Only applies when include list is empty 3. **Empty filters**: All configured clients are a... +Only applies when include list is empty. 3. **Empty filters**: All configured clients are a... -Applied if no include list 3. **Config-level tool whi... +Applied if no include lists exist. 3. **Config-level tool whi... -configuration for each client - **Client connection issues**: Verify external MC... +configuration for each client. **Client connection issues**: Verify external MC...Also applies to: 837-837, 888-888, 1122-1122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
core/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
README.md(2 hunks)core/bifrost.go(6 hunks)core/go.mod(1 hunks)core/mcp.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mcp.go(1 hunks)docs/mcp.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
core/schemas/bifrost.go (1)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/schemas/bifrost.go:20-23
Timestamp: 2025-06-18T15:16:23.104Z
Learning: In the Bifrost project, BifrostConfig struct is never marshaled/unmarshaled, so serialization tags (json, yaml) are not needed for its fields.
core/bifrost.go (1)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/bifrost.go:823-845
Timestamp: 2025-06-18T15:15:51.303Z
Learning: In the Bifrost project, the team prioritizes maintaining consistent error handling patterns over exposing detailed error context. All errors should be wrapped in the standard `BifrostError` structure rather than creating specific error types or exposing richer error details like exit codes or stderr output.
🪛 LanguageTool
docs/mcp.md
[uncategorized] ~774-~774: Loose punctuation mark.
Context: ...ent Filtering - "mcp_include_clients": Whitelist specific clients for a reques...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~837-~837: Possible typo: you repeated a word.
Context: ...ry**: Only applies when include list is empty 3. Empty filters: All configured clients are a...
(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~888-~888: Possible subject-verb agreement error detected: Did you mean to use the plural form here?
Context: ...** (mcp_exclude_clients) - Applied if no include list 3. **Config-level tool whi...
(SUBJECT_NUMBER)
[duplication] ~1122-~1122: Possible typo: you repeated a word.
Context: ...ew ToolsToSkip configuration for each client - Client connection issues: Verify external MC...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
docs/mcp.md
528-528: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
633-633: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
675-675: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
985-985: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
1000-1000: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
1040-1040: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
1073-1073: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1086-1086: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1101-1101: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1115-1115: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1128-1128: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1142-1142: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1187-1187: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (19)
core/schemas/mcp.go (1)
1-34: LGTM! MCP schema definitions are well-structured.The schema definitions provide a clear and comprehensive structure for MCP configuration. The use of JSON tags for serialization is appropriate, and the separation of concerns between different config types (server, client, connection types) follows good design principles.
README.md (2)
213-213: Good addition to the features list.The MCP Integration feature is appropriately documented and positioned in the features list.
284-284: Documentation link follows existing pattern.The MCP documentation link is properly added to the additional configurations section, maintaining consistency with other configuration links.
core/schemas/bifrost.go (1)
22-22: Clean integration of MCP configuration.The addition of the MCPConfig field as an optional pointer follows the existing pattern in the struct and enables proper MCP integration.
core/go.mod (1)
27-59: Appropriate dependencies for MCP integration.The added dependencies support the MCP functionality requirements including JSON processing, HTTP frameworks, validation, and schema generation. The dependency set is comprehensive and necessary for the MCP implementation.
core/bifrost.go (7)
51-51: Proper field addition for MCP integration.The mcpManager field is correctly typed as a pointer to allow for optional MCP functionality when not configured.
240-249: Robust MCP manager initialization.The initialization properly handles both success and failure cases with appropriate logging. The error handling allows Bifrost to continue functioning even if MCP initialization fails.
615-618: Appropriate placement of MCP tool injection.The MCP tools are added to the request before plugin hooks execute, which ensures tools are available for the entire request processing pipeline.
746-749: Consistent MCP tool injection pattern.The tool injection follows the same pattern as in tryTextCompletion, maintaining consistency across request types.
823-845: Well-designed public API for MCP tool execution.The ExecuteMCPTool method provides a clean public interface with proper error handling following the consistent BifrostError pattern. The documentation is clear and helpful.
870-876: Excellent public API for tool registration.The RegisterMCPTool method includes comprehensive documentation with a practical example. The interface is intuitive and follows Go conventions.
893-899: Proper resource cleanup implementation.The MCP manager cleanup is appropriately handled in the Cleanup method with error logging, ensuring graceful shutdown of MCP resources.
core/mcp.go (7)
202-205: LGTM: Duplicate tool registration prevention implementedThe duplicate tool check correctly prevents silent overwrites by returning a clear error when a tool name already exists.
This addresses the past review comment about preventing duplicate local tool registration.
410-434: LGTM: Lock contention reduced with proper synchronizationThe connection logic properly uses short locks around critical sections and performs heavy operations (network I/O, process creation) outside the lock, then re-acquires the lock only to update final state.
This addresses the past review comment about heavy operations under global lock degrading parallelism.
Also applies to: 474-492
510-530: LGTM: Process cleanup errors properly handledThe process cleanup code now properly checks and logs errors from
Kill()andWait(), with appropriate timeout handling to prevent blocking cleanup indefinitely.This addresses the past review comment about unchecked errors when killing STDIO processes.
566-568: LGTM: Thread-safe access with read lockThe
findMCPClientForToolmethod now properly uses read lock to protect access to the sharedclientMap.This addresses the past review comment about possible data race on
clientMapaccess.
821-834: LGTM: Efficient tool deduplication with map lookupThe code now uses a map for O(1) lookup to detect duplicates instead of nested loops, improving performance when dealing with many tools.
This addresses the past review comment about inefficient tool deduplication.
376-378: LGTM: Improved error context in tool executionThe JSON unmarshaling error now includes the tool name in the error message, providing better debugging context.
This addresses the past review comment about improving error context in tool execution.
863-880: Add validation for overlapping skip/execute tool listsThe validation correctly prevents conflicting tool configuration by detecting tools that appear in both
ToolsToSkipandToolsToExecutelists.This is good defensive programming that prevents configuration errors.
Pratham-Mishra04
force-pushed
the
06-18-feat_added_mcp_support
branch
3 times, most recently
from
fbf3e11 to
d42b6a7
Compare
Pratham-Mishra04
force-pushed
the
06-18-feat_added_mcp_support
branch
from
d42b6a7 to
6ff7a3e
Compare
Merge activity
|
Add Model Context Protocol (MCP) Integration
This PR adds built-in support for the Model Context Protocol (MCP), enabling seamless integration with external tools. The implementation allows Bifrost to discover, register, and execute tools from both local and external MCP servers.
Key features:
The implementation includes:
MCPManagercomponent for managing MCP connections and tool executionDocumentation has been added with detailed examples, architecture overview, and best practices for implementing chat conversations with MCP tools.