feat: add dynamic MCP client management API#143
Merged
Conversation
Contributor
|
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 9 minutes and 55 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 selected for processing (6)
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change introduces a complete runtime management API for MCP (Model Client Proxy) clients, including new methods in the Bifrost core, MCP manager, HTTP handlers, and configuration store. It enables listing, adding, removing, reconnecting, and editing MCP clients and their tools via HTTP endpoints and in-memory operations, with thread-safe updates and schema extensions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HTTPHandler
participant ConfigStore
participant Bifrost
participant MCPManager
User->>HTTPHandler: (e.g., POST /mcp/clients) Add MCP Client
HTTPHandler->>ConfigStore: AddMCPClient(config)
ConfigStore->>Bifrost: AddMCPClient(config)
Bifrost->>MCPManager: AddClient(config)
MCPManager-->>Bifrost: result
Bifrost-->>ConfigStore: result
ConfigStore-->>HTTPHandler: result
HTTPHandler-->>User: JSON response
User->>HTTPHandler: (e.g., GET /mcp/clients) List MCP Clients
HTTPHandler->>ConfigStore: GetMCPConfig()
HTTPHandler->>Bifrost: GetMCPClients()
Bifrost->>MCPManager: GetClients()
MCPManager-->>Bifrost: []MCPClient
Bifrost-->>HTTPHandler: []MCPClient
HTTPHandler-->>User: JSON response
Possibly related PRs
Suggested reviewers
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 (
|
Collaborator
Author
This was referenced Jul 3, 2025
Merged
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
core/bifrost.go(1 hunks)core/mcp.go(3 hunks)core/schemas/mcp.go(1 hunks)transports/bifrost-http/handlers/mcp.go(2 hunks)transports/bifrost-http/lib/store.go(3 hunks)transports/bifrost-http/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/mcp.go:696-711
Timestamp: 2025-06-18T20:16:37.560Z
Learning: For the Bifrost MCP implementation, performance optimizations like caching tool discovery results should be deferred until after core functionality is complete. The author prefers to implement core features first and handle optimizations as follow-up work.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: transports/bifrost-http/main.go:2-2
Timestamp: 2025-06-15T16:05:13.489Z
Learning: For the Bifrost project, HTTP transport integration routers for new providers (like Mistral and Ollama) are implemented in separate PRs from the core provider support, following a focused PR strategy.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#54
File: core/schemas/provider.go:148-148
Timestamp: 2025-06-04T03:57:50.981Z
Learning: Breaking changes in the Bifrost codebase are managed by first merging and tagging core schema changes, then updating dependent code references in subsequent steps after the core version is released.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#55
File: core/providers/anthropic.go:358-388
Timestamp: 2025-06-04T05:37:59.699Z
Learning: User Pratham-Mishra04 prefers not to extract small code duplications (around 2 lines) into helper functions, considering the overhead not worth it for such minor repetition.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#102
File: README.md:62-66
Timestamp: 2025-06-19T17:03:03.639Z
Learning: Pratham-Mishra04 prefers using the implicit 'latest' tag for the maximhq/bifrost Docker image rather than pinning to specific versions.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#138
File: docs/usage/go-package/mcp.md:408-412
Timestamp: 2025-07-01T12:40:08.576Z
Learning: Pratham-Mishra04 is okay with keeping bullet list formatting that uses colons after dashes in markdown documentation, even if it triggers linter warnings, preferring functionality over strict formatting rules.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#138
File: transports/README.md:26-28
Timestamp: 2025-07-01T12:45:06.906Z
Learning: Pratham-Mishra04 prefers keeping documentation examples simple and concise, trusting users to handle production-specific considerations like version pinning themselves rather than cluttering examples with additional notes.
transports/bifrost-http/main.go (9)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#103
File: .github/workflows/transport-dependency-update.yml:53-75
Timestamp: 2025-06-20T16:21:18.912Z
Learning: In the bifrost repository's transport dependency update workflow, when updating the core dependency to a new version using `go get`, the go.mod and go.sum files will always change in normal operation, making the safety check for changes more of a defensive programming practice rather than handling a common scenario.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/providers/mistral.go:168-170
Timestamp: 2025-06-15T14:24:49.882Z
Learning: In the Bifrost codebase, performance is prioritized over defensive copying for HTTP service operations. Specifically, shallow slice assignments in provider response handling are acceptable due to object pool reset patterns and JSON unmarshaling behavior that minimize practical data corruption risks.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#85
File: core/providers/anthropic.go:150-156
Timestamp: 2025-06-15T16:07:53.140Z
Learning: In the Bifrost codebase, constructor functions are allowed to mutate input ProviderConfig objects in-place (e.g., setting default BaseURL values and trimming trailing slashes). This pattern is acceptable and doesn't need to be flagged as a code review issue.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: transports/bifrost-http/main.go:2-2
Timestamp: 2025-06-15T16:05:13.489Z
Learning: For the Bifrost project, HTTP transport integration routers for new providers (like Mistral and Ollama) are implemented in separate PRs from the core provider support, following a focused PR strategy.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and handles the conversion internally, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#131
File: transports/bifrost-http/lib/config.go:35-68
Timestamp: 2025-06-26T07:37:24.385Z
Learning: In the Bifrost project's MCP configuration handling, empty environment variables should be treated as missing/invalid rather than as valid empty values. The current implementation using `os.Getenv(envKey); envValue != ""` to check for non-empty values is the desired behavior.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/end_to_end_tool_calling.go:43-45
Timestamp: 2025-06-16T04:13:55.437Z
Learning: In the Bifrost codebase, errors returned from client methods like ChatCompletionRequest are of type BifrostError, not the standard error interface. For testing these errors, use require.Nilf instead of require.NoErrorf since BifrostError doesn't work with the standard error assertion methods.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/complete_end_to_end.go:39-41
Timestamp: 2025-06-16T04:12:05.427Z
Learning: In the Bifrost system, error returns are of type `BifrostError` rather than the standard Go `error` interface. Therefore, use `require.Nilf(t, err, ...)` instead of `require.NoError(t, err)` when checking for errors in Bifrost function calls.
core/schemas/mcp.go (1)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
core/mcp.go (5)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#88
File: core/providers/openai.go:202-210
Timestamp: 2025-06-16T06:56:51.547Z
Learning: In Go, when JSON unmarshaling into a pooled struct, slices within the struct are freshly allocated by the JSON unmarshaler, not part of the pooled struct's original memory. Defensive copying of these slices is unnecessary since they don't reference pooled memory that could be reused.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#88
File: core/providers/azure.go:0-0
Timestamp: 2025-06-15T19:04:55.993Z
Learning: In Go, when JSON unmarshaling populates pooled structs, the slice and string fields point to newly allocated memory created by json.Unmarshal(). Shallow copying these fields (copying slice headers) is safe because the underlying data is not part of the pooled memory - it's owned by the JSON unmarshaling process. Deep copying is unnecessary and wasteful in this pattern.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#88
File: core/providers/ollama.go:171-179
Timestamp: 2025-06-16T06:56:52.953Z
Learning: In Go, when json.Unmarshal populates slice fields in structs, it allocates fresh backing arrays independently of the struct's memory. The struct only stores slice headers (pointer, length, capacity). When pooled structs are reset with *resp = ResponseType{}, only the slice headers are cleared, not the underlying backing arrays, so no deep copying is needed to avoid memory corruption.
Learnt from: connyay
PR: maximhq/bifrost#92
File: core/providers/utils.go:60-64
Timestamp: 2025-06-17T14:04:21.104Z
Learning: The bifrost codebase uses string alias types (like `type ContextKey string`) for context keys consistently across multiple packages including plugins/maxim/main.go and transports/bifrost-http/tracking/plugin.go. This pattern should be maintained for consistency rather than switching to the canonical struct{} approach.
core/bifrost.go (4)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#54
File: core/schemas/provider.go:148-148
Timestamp: 2025-06-04T03:57:50.981Z
Learning: Breaking changes in the Bifrost codebase are managed by first merging and tagging core schema changes, then updating dependent code references in subsequent steps after the core version is released.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#103
File: .github/workflows/transport-dependency-update.yml:53-75
Timestamp: 2025-06-20T16:21:18.912Z
Learning: In the bifrost repository's transport dependency update workflow, when updating the core dependency to a new version using `go get`, the go.mod and go.sum files will always change in normal operation, making the safety check for changes more of a defensive programming practice rather than handling a common scenario.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/mcp.go:696-711
Timestamp: 2025-06-18T20:16:37.560Z
Learning: For the Bifrost MCP implementation, performance optimizations like caching tool discovery results should be deferred until after core functionality is complete. The author prefers to implement core features first and handle optimizations as follow-up work.
transports/bifrost-http/lib/store.go (13)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#131
File: transports/bifrost-http/lib/config.go:35-68
Timestamp: 2025-06-26T07:37:24.385Z
Learning: In the Bifrost project's MCP configuration handling, empty environment variables should be treated as missing/invalid rather than as valid empty values. The current implementation using `os.Getenv(envKey); envValue != ""` to check for non-empty values is the desired behavior.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#103
File: .github/workflows/transport-dependency-update.yml:53-75
Timestamp: 2025-06-20T16:21:18.912Z
Learning: In the bifrost repository's transport dependency update workflow, when updating the core dependency to a new version using `go get`, the go.mod and go.sum files will always change in normal operation, making the safety check for changes more of a defensive programming practice rather than handling a common scenario.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#135
File: docs/core-package.md:105-116
Timestamp: 2025-06-27T17:07:39.462Z
Learning: In Go, when importing a package, the identifier used to access it is determined by the `package` declaration in the Go source files, not the directory name. For the Bifrost project, the core directory files declare `package bifrost`, so importing `"github.com/maximhq/bifrost/core"` automatically makes it accessible as `bifrost.Init`, `bifrost.Ptr`, etc., without needing an explicit alias.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/openai_test.go:1-2
Timestamp: 2025-06-16T04:29:53.409Z
Learning: In the Bifrost project, the user prefers to use `package main` for test files in the tests/core-providers directory rather than more descriptive package names like `coreproviders_test`.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/end_to_end_tool_calling.go:43-45
Timestamp: 2025-06-16T04:13:55.437Z
Learning: In the Bifrost codebase, errors returned from client methods like ChatCompletionRequest are of type BifrostError, not the standard error interface. For testing these errors, use require.Nilf instead of require.NoErrorf since BifrostError doesn't work with the standard error assertion methods.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/schemas/bifrost.go:20-23
Timestamp: 2025-06-18T15:16:23.127Z
Learning: In the Bifrost project, BifrostConfig struct is never marshaled/unmarshaled, so serialization tags (json, yaml) are not needed for its fields.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/complete_end_to_end.go:39-41
Timestamp: 2025-06-16T04:12:05.427Z
Learning: In the Bifrost system, error returns are of type `BifrostError` rather than the standard Go `error` interface. Therefore, use `require.Nilf(t, err, ...)` instead of `require.NoError(t, err)` when checking for errors in Bifrost function calls.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/providers/mistral.go:168-170
Timestamp: 2025-06-15T14:24:49.882Z
Learning: In the Bifrost codebase, performance is prioritized over defensive copying for HTTP service operations. Specifically, shallow slice assignments in provider response handling are acceptable due to object pool reset patterns and JSON unmarshaling behavior that minimize practical data corruption risks.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#80
File: plugins/maxim/main.go:249-264
Timestamp: 2025-06-14T06:17:54.426Z
Learning: In the BifrostError struct, the Error field is a struct (not a pointer), so accessing bifrostErr.Error.Message, bifrostErr.Error.Code, and bifrostErr.Error.Type is safe without nil checks on the Error field itself. The Code and Type fields are of type *string.
Learnt from: connyay
PR: maximhq/bifrost#92
File: core/providers/utils.go:60-64
Timestamp: 2025-06-17T14:04:21.104Z
Learning: The bifrost codebase uses string alias types (like `type ContextKey string`) for context keys consistently across multiple packages including plugins/maxim/main.go and transports/bifrost-http/tracking/plugin.go. This pattern should be maintained for consistency rather than switching to the canonical struct{} approach.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/simple_chat.go:39-41
Timestamp: 2025-06-16T04:13:42.755Z
Learning: In the Bifrost codebase, errors returned from methods like ChatCompletionRequest are of type BifrostError (a custom error type) rather than the standard Go error interface. Therefore, require.Nilf should be used for error assertions instead of require.NoErrorf.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/schemas/bifrost.go:186-190
Timestamp: 2025-06-15T14:18:32.703Z
Learning: In core/schemas/bifrost.go, the ToolChoice UnmarshalJSON validation intentionally only checks for empty Type fields and lets providers handle validation of specific tool choice values. This architectural decision keeps schema validation focused on structure while allowing provider-specific semantic validation.
transports/bifrost-http/handlers/mcp.go (2)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#131
File: transports/bifrost-http/lib/config.go:35-68
Timestamp: 2025-06-26T07:37:24.385Z
Learning: In the Bifrost project's MCP configuration handling, empty environment variables should be treated as missing/invalid rather than as valid empty values. The current implementation using `os.Getenv(envKey); envValue != ""` to check for non-empty values is the desired behavior.
🧬 Code Graph Analysis (4)
transports/bifrost-http/main.go (3)
transports/bifrost-http/handlers/providers.go (1)
NewProviderHandler(25-31)transports/bifrost-http/handlers/completions.go (1)
NewCompletionHandler(23-28)transports/bifrost-http/handlers/mcp.go (1)
NewMCPHandler(24-30)
core/mcp.go (2)
core/schemas/mcp.go (2)
MCPClient(47-52)MCPClientConfig(11-18)core/schemas/bifrost.go (1)
Tool(125-129)
core/bifrost.go (2)
core/schemas/mcp.go (4)
MCPClient(47-52)MCPConnectionStateConnected(39-39)MCPConnectionStateDisconnected(40-40)MCPClientConfig(11-18)core/mcp.go (1)
MCPClient(57-64)
transports/bifrost-http/lib/store.go (3)
core/schemas/logger.go (1)
Logger(18-35)core/bifrost.go (1)
Bifrost(51-64)core/schemas/mcp.go (2)
MCPConfig(6-8)MCPClientConfig(11-18)
🔇 Additional comments (21)
transports/bifrost-http/main.go (2)
213-215: LGTM! Proper integration of Bifrost client with the store.The placement of
SetBifrostClientafter successful client initialization ensures the store has a valid client reference for MCP operations.
219-219: LGTM! MCPHandler constructor updated correctly.The addition of the store parameter enables the MCP handler to perform configuration management operations.
core/schemas/mcp.go (2)
36-42: LGTM! Well-defined connection state constants.The
MCPConnectionStatetype provides clear, type-safe state tracking for MCP clients.
44-52: LGTM! Comprehensive MCPClient struct.The struct properly encapsulates all necessary client information with clear documentation and appropriate JSON tags for API responses.
core/mcp.go (6)
7-7: LGTM! Good refactoring of the constructor.The constructor now properly delegates to
AddClientfor consistency and handles individual client failures gracefully with warnings, allowing partial initialization success.Also applies to: 103-111
113-128: LGTM! Thread-safe client retrieval.The method properly uses read locks and returns client copies (not pointers) to prevent external modifications.
198-238: LGTM! Well-structured client removal with comprehensive cleanup.Good separation between public API and internal implementation. The cleanup properly handles all connection types including SSE context cancellation.
240-281: LGTM! Clean implementation of tool configuration updates.The method properly updates tool filters and refreshes the tool map from the external server. Good use of
maps.Copyfor safe map operations.
1093-1096: LGTM! Good reuse of removeClientUnsafe.The cleanup method now properly reuses the comprehensive cleanup logic, avoiding code duplication.
130-153: Fix the mutex handling in ReconnectClient.The method unlocks the mutex at line 144 but doesn't have proper error handling if
connectToMCPClientfails or panics. The mutex won't be re-acquired in error cases.Consider this safer approach:
- m.mu.Unlock() - - // connectToMCPClient handles locking internally - err := m.connectToMCPClient(client.ExecutionConfig) - if err != nil { - return fmt.Errorf("failed to connect to MCP client %s: %w", name, err) - } - - return nil + // Store config before unlocking + config := client.ExecutionConfig + m.mu.Unlock() + + // connectToMCPClient handles locking internally + err := m.connectToMCPClient(config) + if err != nil { + return fmt.Errorf("failed to connect to MCP client %s: %w", name, err) + } + + return nil⛔ Skipped due to learnings
Learnt from: Pratham-Mishra04 PR: maximhq/bifrost#100 File: core/mcp.go:489-490 Timestamp: 2025-06-19T12:38:07.544Z Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.core/bifrost.go (3)
1034-1037: Good documentation of performance implications.Important warning for users about potential latency impacts during MCP client management operations.
1039-1075: LGTM! Proper transformation to public schema.The method correctly transforms internal client representation to the public schema format, including tool name extraction and connection state determination.
1077-1152: LGTM! Clean API wrappers for MCP client management.All methods follow a consistent pattern with proper validation and delegation to the internal manager. The API is well-documented with clear examples.
transports/bifrost-http/lib/store.go (3)
30-33: LGTM! Good separation of concerns with dedicated MCP mutex.Using a separate mutex for MCP operations prevents lock contention with provider operations.
605-636: LGTM! Correct order of operations.The method properly removes from the Bifrost client first, then updates the configuration only on success, ensuring consistency.
638-670: LGTM! Consistent implementation pattern.The method follows the same pattern as other operations - updating the Bifrost client first, then the configuration on success.
transports/bifrost-http/handlers/mcp.go (5)
17-30: LGTM! Clean integration of ConfigStore dependency.The struct extension and constructor changes properly integrate the ConfigStore dependency into the MCPHandler, maintaining consistency with the existing pattern.
36-40: LGTM! RESTful API design follows best practices.The route registration properly implements RESTful conventions with appropriate HTTP methods for each MCP client management operation.
80-121: LGTM! Complex merging logic correctly implemented.The GetMCPClients handler properly merges configuration store data with runtime connection states, providing a comprehensive view of both connected and disconnected MCP clients. The error handling and response structure are well-designed.
138-155: LGTM! Clean implementation with proper error handling.The AddMCPClient handler correctly unmarshals the request body and handles errors appropriately, following the consistent pattern used throughout the codebase.
157-179: LGTM! Both handlers are well-implemented with consistent patterns.The EditMCPClientTools and RemoveMCPClient handlers follow the established patterns with proper error handling and response formatting. The use of an anonymous struct for the tools editing request is appropriate for the API contract.
Also applies to: 181-194
This was referenced Jul 5, 2025
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Mutex Double Unlock in Client Methods
Double mutex unlock bug in AddClient and EditClientTools methods. Both functions use defer m.mu.Unlock() but then manually unlock and re-lock the mutex within their execution. This leads to a panic when the deferred unlock attempts to unlock an already unlocked mutex upon function return.
core/mcp.go#L161-L196
Lines 161 to 196 in 7cbe74f
core/mcp.go#L239-L281
Lines 239 to 281 in 7cbe74f
Bug: Unsynchronized Access to Shared Config
Race condition: The GetMCPConfig() method's mutex protection (s.mu.RLock()) was removed, allowing it to access s.mcpConfig without synchronization. Concurrently, new MCP modification methods (AddMCPClient, RemoveMCPClient, EditMCPClientTools) modify s.mcpConfig using a separate mutex (s.muMCP). This creates a data race where GetMCPConfig() can read s.mcpConfig while it's being modified, leading to inconsistent data.
transports/bifrost-http/lib/store.go#L569-L572
bifrost/transports/bifrost-http/lib/store.go
Lines 569 to 572 in 7cbe74f
Bug: Mutex Unlock Error in ReconnectClient
The ReconnectClient method contains a double mutex unlock bug. It uses defer m.mu.Unlock() but also manually calls m.mu.Unlock() before a network operation. This causes a panic when the deferred unlock attempts to unlock an already unlocked mutex upon function return.
core/mcp.go#L130-L153
Lines 130 to 153 in 7cbe74f
BugBot free trial expires on July 22, 2025
You have used $0.00 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
Pratham-Mishra04
force-pushed
the
07-03-feat_drop_access_request_and_prom_labels_shifted_to_config
branch
from
e91cd9d to
78b7b31
Compare
Pratham-Mishra04
force-pushed
the
07-03-feat_hotreload_for_mcp_clients_added
branch
from
7cbe74f to
a3d74d8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
core/mcp.go (1)
155-196: Good implementation with proper cleanup logic.The method correctly handles the placeholder pattern and cleans up on failure. The unlock/lock pattern for long operations is appropriate.
transports/bifrost-http/lib/store.go (2)
567-572: Add locking to GetMCPConfig to prevent race conditions.The method returns a pointer to the MCP config without any locking, while other methods modify it under
muMCP. This could lead to race conditions.
574-603: Good implementation with proper rollback logic.The method correctly handles configuration updates and rolls back on failure.
transports/bifrost-http/handlers/mcp.go (3)
123-136: Add type assertion safety checks to prevent potential panics.The type assertion
ctx.UserValue("name").(string)at line 125 could panic if the name parameter is not a string type.
157-179: Add type assertion safety checks to prevent potential panics.The type assertion
ctx.UserValue("name").(string)at line 159 could panic if the name parameter is not a string type.
181-194: Add type assertion safety checks to prevent potential panics.The type assertion
ctx.UserValue("name").(string)at line 183 could panic if the name parameter is not a string type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
core/bifrost.go(1 hunks)core/mcp.go(3 hunks)core/schemas/mcp.go(1 hunks)transports/bifrost-http/handlers/mcp.go(2 hunks)transports/bifrost-http/lib/store.go(3 hunks)transports/bifrost-http/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: transports/bifrost-http/main.go:2-2
Timestamp: 2025-06-15T16:05:13.489Z
Learning: For the Bifrost project, HTTP transport integration routers for new providers (like Mistral and Ollama) are implemented in separate PRs from the core provider support, following a focused PR strategy.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/mcp.go:696-711
Timestamp: 2025-06-18T20:16:37.560Z
Learning: For the Bifrost MCP implementation, performance optimizations like caching tool discovery results should be deferred until after core functionality is complete. The author prefers to implement core features first and handle optimizations as follow-up work.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#54
File: core/schemas/provider.go:148-148
Timestamp: 2025-06-04T03:57:50.981Z
Learning: Breaking changes in the Bifrost codebase are managed by first merging and tagging core schema changes, then updating dependent code references in subsequent steps after the core version is released.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#55
File: core/providers/anthropic.go:358-388
Timestamp: 2025-06-04T05:37:59.699Z
Learning: User Pratham-Mishra04 prefers not to extract small code duplications (around 2 lines) into helper functions, considering the overhead not worth it for such minor repetition.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#102
File: README.md:62-66
Timestamp: 2025-06-19T17:03:03.639Z
Learning: Pratham-Mishra04 prefers using the implicit 'latest' tag for the maximhq/bifrost Docker image rather than pinning to specific versions.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#138
File: docs/usage/go-package/mcp.md:408-412
Timestamp: 2025-07-01T12:40:08.576Z
Learning: Pratham-Mishra04 is okay with keeping bullet list formatting that uses colons after dashes in markdown documentation, even if it triggers linter warnings, preferring functionality over strict formatting rules.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#138
File: transports/README.md:26-28
Timestamp: 2025-07-01T12:45:06.906Z
Learning: Pratham-Mishra04 prefers keeping documentation examples simple and concise, trusting users to handle production-specific considerations like version pinning themselves rather than cluttering examples with additional notes.
transports/bifrost-http/main.go (10)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#103
File: .github/workflows/transport-dependency-update.yml:53-75
Timestamp: 2025-06-20T16:21:18.912Z
Learning: In the bifrost repository's transport dependency update workflow, when updating the core dependency to a new version using `go get`, the go.mod and go.sum files will always change in normal operation, making the safety check for changes more of a defensive programming practice rather than handling a common scenario.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/providers/mistral.go:168-170
Timestamp: 2025-06-15T14:24:49.882Z
Learning: In the Bifrost codebase, performance is prioritized over defensive copying for HTTP service operations. Specifically, shallow slice assignments in provider response handling are acceptable due to object pool reset patterns and JSON unmarshaling behavior that minimize practical data corruption risks.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and handles the conversion internally, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#131
File: transports/bifrost-http/lib/config.go:35-68
Timestamp: 2025-06-26T07:37:24.385Z
Learning: In the Bifrost project's MCP configuration handling, empty environment variables should be treated as missing/invalid rather than as valid empty values. The current implementation using `os.Getenv(envKey); envValue != ""` to check for non-empty values is the desired behavior.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: transports/bifrost-http/main.go:2-2
Timestamp: 2025-06-15T16:05:13.489Z
Learning: For the Bifrost project, HTTP transport integration routers for new providers (like Mistral and Ollama) are implemented in separate PRs from the core provider support, following a focused PR strategy.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and always returns a pointer to a valid context. It starts with context.Background() and only adds values to it, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#64
File: transports/bifrost-http/integrations/genai/types.go:273-313
Timestamp: 2025-06-09T16:35:26.914Z
Learning: In convertGenerationConfigToParams method in transports/bifrost-http/integrations/genai/types.go, pre-allocating the ExtraParams map is preferred over lazy allocation because the method has multiple potential ExtraParams assignments, making the computational overhead of conditional checks exceed the memory savings of an empty map.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/end_to_end_tool_calling.go:43-45
Timestamp: 2025-06-16T04:13:55.437Z
Learning: In the Bifrost codebase, errors returned from client methods like ChatCompletionRequest are of type BifrostError, not the standard error interface. For testing these errors, use require.Nilf instead of require.NoErrorf since BifrostError doesn't work with the standard error assertion methods.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/complete_end_to_end.go:39-41
Timestamp: 2025-06-16T04:12:05.427Z
Learning: In the Bifrost system, error returns are of type `BifrostError` rather than the standard Go `error` interface. Therefore, use `require.Nilf(t, err, ...)` instead of `require.NoError(t, err)` when checking for errors in Bifrost function calls.
core/schemas/mcp.go (1)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
core/bifrost.go (5)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#54
File: core/schemas/provider.go:148-148
Timestamp: 2025-06-04T03:57:50.981Z
Learning: Breaking changes in the Bifrost codebase are managed by first merging and tagging core schema changes, then updating dependent code references in subsequent steps after the core version is released.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/providers/mistral.go:168-170
Timestamp: 2025-06-15T14:24:49.882Z
Learning: In the Bifrost codebase, performance is prioritized over defensive copying for HTTP service operations. Specifically, shallow slice assignments in provider response handling are acceptable due to object pool reset patterns and JSON unmarshaling behavior that minimize practical data corruption risks.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/mcp.go:696-711
Timestamp: 2025-06-18T20:16:37.560Z
Learning: For the Bifrost MCP implementation, performance optimizations like caching tool discovery results should be deferred until after core functionality is complete. The author prefers to implement core features first and handle optimizations as follow-up work.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#103
File: .github/workflows/transport-dependency-update.yml:53-75
Timestamp: 2025-06-20T16:21:18.912Z
Learning: In the bifrost repository's transport dependency update workflow, when updating the core dependency to a new version using `go get`, the go.mod and go.sum files will always change in normal operation, making the safety check for changes more of a defensive programming practice rather than handling a common scenario.
core/mcp.go (5)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#88
File: core/providers/openai.go:202-210
Timestamp: 2025-06-16T06:56:51.547Z
Learning: In Go, when JSON unmarshaling into a pooled struct, slices within the struct are freshly allocated by the JSON unmarshaler, not part of the pooled struct's original memory. Defensive copying of these slices is unnecessary since they don't reference pooled memory that could be reused.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#88
File: core/providers/azure.go:0-0
Timestamp: 2025-06-15T19:04:55.993Z
Learning: In Go, when JSON unmarshaling populates pooled structs, the slice and string fields point to newly allocated memory created by json.Unmarshal(). Shallow copying these fields (copying slice headers) is safe because the underlying data is not part of the pooled memory - it's owned by the JSON unmarshaling process. Deep copying is unnecessary and wasteful in this pattern.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#88
File: core/providers/ollama.go:171-179
Timestamp: 2025-06-16T06:56:52.953Z
Learning: In Go, when json.Unmarshal populates slice fields in structs, it allocates fresh backing arrays independently of the struct's memory. The struct only stores slice headers (pointer, length, capacity). When pooled structs are reset with *resp = ResponseType{}, only the slice headers are cleared, not the underlying backing arrays, so no deep copying is needed to avoid memory corruption.
Learnt from: connyay
PR: maximhq/bifrost#92
File: core/providers/utils.go:60-64
Timestamp: 2025-06-17T14:04:21.104Z
Learning: The bifrost codebase uses string alias types (like `type ContextKey string`) for context keys consistently across multiple packages including plugins/maxim/main.go and transports/bifrost-http/tracking/plugin.go. This pattern should be maintained for consistency rather than switching to the canonical struct{} approach.
transports/bifrost-http/handlers/mcp.go (12)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#131
File: transports/bifrost-http/lib/config.go:35-68
Timestamp: 2025-06-26T07:37:24.385Z
Learning: In the Bifrost project's MCP configuration handling, empty environment variables should be treated as missing/invalid rather than as valid empty values. The current implementation using `os.Getenv(envKey); envValue != ""` to check for non-empty values is the desired behavior.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#103
File: .github/workflows/transport-dependency-update.yml:53-75
Timestamp: 2025-06-20T16:21:18.912Z
Learning: In the bifrost repository's transport dependency update workflow, when updating the core dependency to a new version using `go get`, the go.mod and go.sum files will always change in normal operation, making the safety check for changes more of a defensive programming practice rather than handling a common scenario.
Learnt from: connyay
PR: maximhq/bifrost#92
File: core/providers/utils.go:60-64
Timestamp: 2025-06-17T14:04:21.104Z
Learning: The bifrost codebase uses string alias types (like `type ContextKey string`) for context keys consistently across multiple packages including plugins/maxim/main.go and transports/bifrost-http/tracking/plugin.go. This pattern should be maintained for consistency rather than switching to the canonical struct{} approach.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/simple_chat.go:39-41
Timestamp: 2025-06-16T04:13:42.755Z
Learning: In the Bifrost codebase, errors returned from methods like ChatCompletionRequest are of type BifrostError (a custom error type) rather than the standard Go error interface. Therefore, require.Nilf should be used for error assertions instead of require.NoErrorf.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#67
File: transports/bifrost-http/integrations/anthropic/router.go:26-34
Timestamp: 2025-06-10T11:19:29.604Z
Learning: The Generic router in transports/bifrost-http/integrations/utils.go already handles nil pointers from RequestConverter functions. When a RequestConverter returns a nil *schemas.BifrostRequest, the Generic router automatically returns an HTTP 400 error, making additional nil checks in individual router implementations redundant.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#65
File: transports/bifrost-http/integrations/anthropic/types.go:140-146
Timestamp: 2025-06-10T13:51:52.859Z
Learning: In Bifrost core v1.0.9, ImageContent.Type was a pointer type (*string accessed via bifrost.Ptr), but in v1.0.10 it was changed to a value type (ImageContentType). When reviewing code, check the core version being used to determine the correct assignment pattern.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#67
File: transports/bifrost-http/integrations/anthropic/router.go:26-34
Timestamp: 2025-06-10T11:19:29.604Z
Learning: The Generic router in transports/bifrost-http/integrations/utils.go already handles nil pointers from RequestConverter functions. When a RequestConverter returns a nil *schemas.BifrostRequest, the Generic router automatically returns an HTTP 400 error with "Invalid request" message, making additional nil checks in individual router implementations redundant.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/end_to_end_tool_calling.go:43-45
Timestamp: 2025-06-16T04:13:55.437Z
Learning: In the Bifrost codebase, errors returned from client methods like ChatCompletionRequest are of type BifrostError, not the standard error interface. For testing these errors, use require.Nilf instead of require.NoErrorf since BifrostError doesn't work with the standard error assertion methods.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#89
File: transports/bifrost-http/integrations/genai/types.go:151-155
Timestamp: 2025-06-16T14:44:42.893Z
Learning: In transports/bifrost-http/integrations/genai/types.go, when SystemInstruction has an empty role, the user prefers to return a validation error rather than setting a default role value. This represents a design preference for strict input validation over silent error correction.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#55
File: core/providers/anthropic.go:526-550
Timestamp: 2025-06-04T09:29:46.287Z
Learning: In core/providers/anthropic.go, the content field in formattedMessages is always of type []interface{} because it's explicitly constructed that way upstream in the prepareAnthropicChatRequest function. Defensive type casting for multiple types is not needed since the type is guaranteed by the construction logic.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/providers/mistral.go:168-170
Timestamp: 2025-06-15T14:24:49.882Z
Learning: In the Bifrost codebase, performance is prioritized over defensive copying for HTTP service operations. Specifically, shallow slice assignments in provider response handling are acceptable due to object pool reset patterns and JSON unmarshaling behavior that minimize practical data corruption risks.
transports/bifrost-http/lib/store.go (13)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#100
File: core/mcp.go:489-490
Timestamp: 2025-06-19T12:38:07.544Z
Learning: In the Bifrost MCP manager (core/mcp.go), the connectToMCPClient method is called during initialization/connection setup, not frequently during runtime. Logging operations like m.logger.Info inside critical sections in this context have negligible performance impact and don't require optimization for lock contention.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#103
File: .github/workflows/transport-dependency-update.yml:53-75
Timestamp: 2025-06-20T16:21:18.912Z
Learning: In the bifrost repository's transport dependency update workflow, when updating the core dependency to a new version using `go get`, the go.mod and go.sum files will always change in normal operation, making the safety check for changes more of a defensive programming practice rather than handling a common scenario.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#131
File: transports/bifrost-http/lib/config.go:35-68
Timestamp: 2025-06-26T07:37:24.385Z
Learning: In the Bifrost project's MCP configuration handling, empty environment variables should be treated as missing/invalid rather than as valid empty values. The current implementation using `os.Getenv(envKey); envValue != ""` to check for non-empty values is the desired behavior.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#135
File: docs/core-package.md:105-116
Timestamp: 2025-06-27T17:07:39.462Z
Learning: In Go, when importing a package, the identifier used to access it is determined by the `package` declaration in the Go source files, not the directory name. For the Bifrost project, the core directory files declare `package bifrost`, so importing `"github.com/maximhq/bifrost/core"` automatically makes it accessible as `bifrost.Init`, `bifrost.Ptr`, etc., without needing an explicit alias.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/openai_test.go:1-2
Timestamp: 2025-06-16T04:29:53.409Z
Learning: In the Bifrost project, the user prefers to use `package main` for test files in the tests/core-providers directory rather than more descriptive package names like `coreproviders_test`.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/end_to_end_tool_calling.go:43-45
Timestamp: 2025-06-16T04:13:55.437Z
Learning: In the Bifrost codebase, errors returned from client methods like ChatCompletionRequest are of type BifrostError, not the standard error interface. For testing these errors, use require.Nilf instead of require.NoErrorf since BifrostError doesn't work with the standard error assertion methods.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/schemas/bifrost.go:20-23
Timestamp: 2025-06-18T15:16:23.127Z
Learning: In the Bifrost project, BifrostConfig struct is never marshaled/unmarshaled, so serialization tags (json, yaml) are not needed for its fields.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/complete_end_to_end.go:39-41
Timestamp: 2025-06-16T04:12:05.427Z
Learning: In the Bifrost system, error returns are of type `BifrostError` rather than the standard Go `error` interface. Therefore, use `require.Nilf(t, err, ...)` instead of `require.NoError(t, err)` when checking for errors in Bifrost function calls.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/providers/mistral.go:168-170
Timestamp: 2025-06-15T14:24:49.882Z
Learning: In the Bifrost codebase, performance is prioritized over defensive copying for HTTP service operations. Specifically, shallow slice assignments in provider response handling are acceptable due to object pool reset patterns and JSON unmarshaling behavior that minimize practical data corruption risks.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#80
File: plugins/maxim/main.go:249-264
Timestamp: 2025-06-14T06:17:54.426Z
Learning: In the BifrostError struct, the Error field is a struct (not a pointer), so accessing bifrostErr.Error.Message, bifrostErr.Error.Code, and bifrostErr.Error.Type is safe without nil checks on the Error field itself. The Code and Type fields are of type *string.
Learnt from: connyay
PR: maximhq/bifrost#92
File: core/providers/utils.go:60-64
Timestamp: 2025-06-17T14:04:21.104Z
Learning: The bifrost codebase uses string alias types (like `type ContextKey string`) for context keys consistently across multiple packages including plugins/maxim/main.go and transports/bifrost-http/tracking/plugin.go. This pattern should be maintained for consistency rather than switching to the canonical struct{} approach.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/simple_chat.go:39-41
Timestamp: 2025-06-16T04:13:42.755Z
Learning: In the Bifrost codebase, errors returned from methods like ChatCompletionRequest are of type BifrostError (a custom error type) rather than the standard Go error interface. Therefore, require.Nilf should be used for error assertions instead of require.NoErrorf.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/schemas/bifrost.go:186-190
Timestamp: 2025-06-15T14:18:32.703Z
Learning: In core/schemas/bifrost.go, the ToolChoice UnmarshalJSON validation intentionally only checks for empty Type fields and lets providers handle validation of specific tool choice values. This architectural decision keeps schema validation focused on structure while allowing provider-specific semantic validation.
🧬 Code Graph Analysis (4)
transports/bifrost-http/main.go (3)
transports/bifrost-http/handlers/providers.go (1)
NewProviderHandler(25-31)transports/bifrost-http/handlers/completions.go (1)
NewCompletionHandler(23-28)transports/bifrost-http/handlers/mcp.go (1)
NewMCPHandler(24-30)
core/bifrost.go (2)
core/schemas/mcp.go (4)
MCPClient(47-52)MCPConnectionStateConnected(39-39)MCPConnectionStateDisconnected(40-40)MCPClientConfig(11-18)core/mcp.go (1)
MCPClient(57-64)
core/mcp.go (2)
core/schemas/mcp.go (2)
MCPClient(47-52)MCPClientConfig(11-18)core/schemas/bifrost.go (1)
Tool(125-129)
transports/bifrost-http/lib/store.go (3)
core/schemas/logger.go (1)
Logger(18-35)core/bifrost.go (1)
Bifrost(51-64)core/schemas/mcp.go (2)
MCPConfig(6-8)MCPClientConfig(11-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (17)
transports/bifrost-http/main.go (2)
214-214: LGTM!Correctly links the Bifrost client with the ConfigStore immediately after initialization, enabling dynamic MCP client management through the store.
219-219: LGTM!The MCP handler constructor correctly receives the store parameter to support the new MCP client management endpoints.
core/schemas/mcp.go (1)
36-52: LGTM!Well-structured schema additions for MCP client state representation. The
MCPConnectionStateconstants properly cover all client lifecycle states, and theMCPClientstruct provides a clean API response model.core/mcp.go (5)
7-7: LGTM!Correct use of the
mapspackage for efficient map copying inEditClientTools.
103-111: Good refactoring for resilience.Using
AddClient()in the constructor with warning logs instead of failing initialization allows the MCP manager to start even if some clients fail to connect. This improves system resilience.
113-128: LGTM!Thread-safe implementation that correctly returns a snapshot of clients by value, preventing external modification of internal state.
198-238: LGTM! Thorough cleanup implementation.Excellent separation of concerns with the public wrapper and internal implementation. The cleanup properly handles all resources including SSE context cancellation, connection closing, and tool map clearing.
1093-1096: LGTM! Good refactoring.Reusing
removeClientUnsafeensures consistent cleanup logic and better maintainability.core/bifrost.go (5)
1034-1038: Good documentation of performance implications.The warning clearly communicates the potential latency impact of MCP client management operations, helping users make informed decisions about when to perform these operations.
1039-1075: LGTM!Clean implementation that properly converts internal MCP client representation to the public API schema, including connection state determination and tool name extraction.
1077-1090: LGTM!Simple and clean wrapper implementation.
1092-1114: LGTM!Well-documented method with a clear example showing proper usage.
1116-1137: LGTM!Clear documentation with example usage.
transports/bifrost-http/lib/store.go (2)
605-636: Well-implemented client removal with proper error handling.The method correctly removes the client from Bifrost first before updating the config, ensuring consistency. The thread safety and nil checks are properly implemented.
638-671: Clean implementation of tool configuration updates.The method properly coordinates updates between the Bifrost client and the config store, with appropriate locking and error handling. The order of operations ensures consistency.
transports/bifrost-http/handlers/mcp.go (2)
80-121: Excellent implementation of client status merging.The handler elegantly combines configured clients with their actual connection status, providing a comprehensive view of all MCP clients. The error state handling for disconnected clients is particularly well done.
138-155: Clean implementation of client addition endpoint.The handler properly validates the request, delegates to the store for the actual operation, and returns appropriate responses.
Pratham-Mishra04
force-pushed
the
07-03-feat_hotreload_for_mcp_clients_added
branch
from
a3d74d8 to
dd0bcff
Compare
Pratham-Mishra04
force-pushed
the
07-03-feat_hotreload_for_mcp_clients_added
branch
from
dd0bcff to
dcb2e19
Compare
This was referenced Jul 8, 2025
Merged
Contributor
Merge activity
|
akshaydeo
changed the base branch from
07-03-feat_drop_access_request_and_prom_labels_shifted_to_config
to
graphite-base/143
akshaydeo
added a commit
that referenced
this pull request
Nov 17, 2025
# Dynamic MCP Client Management API
This PR adds a comprehensive API for managing Model Control Protocol (MCP) clients at runtime. The implementation allows users to:
- List all connected MCP clients with their status and available tools
- Add new MCP clients dynamically
- Remove existing MCP clients
- Edit tool configurations for MCP clients
- Reconnect disconnected clients
The implementation includes:
1. New Bifrost core methods for MCP client management:
- `GetMCPClients()`
- `AddMCPClient()`
- `RemoveMCPClient()`
- `EditMCPClientTools()`
- `ReconnectMCPClient()`
2. New HTTP endpoints in the REST API:
- `GET /mcp/clients` - List all clients
- `POST /mcp/client` - Add a new client
- `DELETE /mcp/client/{name}` - Remove a client
- `PUT /mcp/client/{name}` - Edit client tools
- `POST /mcp/client/{name}/reconnect` - Reconnect a client
3. Enhanced `ConfigStore` with methods to persist MCP client changes
4. New schema types for representing MCP client connection states
The implementation includes proper mutex handling to ensure thread safety during client management operations, with a warning note about potential brief latency increases during these operations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dynamic MCP Client Management API
This PR adds a comprehensive API for managing Model Control Protocol (MCP) clients at runtime. The implementation allows users to:
The implementation includes:
New Bifrost core methods for MCP client management:
GetMCPClients()AddMCPClient()RemoveMCPClient()EditMCPClientTools()ReconnectMCPClient()New HTTP endpoints in the REST API:
GET /mcp/clients- List all clientsPOST /mcp/client- Add a new clientDELETE /mcp/client/{name}- Remove a clientPUT /mcp/client/{name}- Edit client toolsPOST /mcp/client/{name}/reconnect- Reconnect a clientEnhanced
ConfigStorewith methods to persist MCP client changesNew schema types for representing MCP client connection states
The implementation includes proper mutex handling to ensure thread safety during client management operations, with a warning note about potential brief latency increases during these operations.