mcp flow refactoring#1458
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughMCP client/config types and APIs switched from value to pointer semantics; MCP manager became an interface (MCPManagerInterface); MCP persistence moved from table types to schema types; OAuth pending MCP configs persisted to DB; plugin load/reload and server callback flows refactored. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin/API
participant Server as BifrostHTTPServer
participant Loader as PluginLoader
participant Config as ConfigStore
participant Core as BifrostCore
participant Obs as Observability
Admin->>Server: ReloadPlugin(name, path, pluginConfig)
Server->>Loader: Instantiate plugin (path, pluginConfig)
Loader-->>Server: plugin instance / error
alt instantiation success
Server->>Server: SyncLoadedPlugin(name, plugin)
Server->>Config: Persist plugin registration (if required)
Server->>Core: Reload runtime config / refresh plugin lists
Core-->>Server: reload result
Server->>Obs: CollectObservabilityPlugins()
Obs-->>Server: updated observability wiring
Server-->>Admin: success
else instantiation error
Server-->>Admin: error (updatePluginErrorStatus recorded)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
core/mcp/mcp.go (1)
118-128: Guard against nil entries in ClientConfigs to avoid startup panics.With pointer-based configs, a nil entry will panic when accessing
clientConfig.Nameor insideAddClient. Add a nil check and onlywg.Add(1)for valid configs.🐛 Proposed fix
if len(config.ClientConfigs) > 0 { - // Add clients in parallel - wg := sync.WaitGroup{} - wg.Add(len(config.ClientConfigs)) - for _, clientConfig := range config.ClientConfigs { + // Add clients in parallel + wg := sync.WaitGroup{} + for _, clientConfig := range config.ClientConfigs { + if clientConfig == nil { + logger.Warn("%s Skipping nil MCP client config", MCPLogPrefix) + continue + } + wg.Add(1) go func(clientConfig *schemas.MCPClientConfig) { defer wg.Done() if err := manager.AddClient(clientConfig); err != nil { logger.Warn("%s Failed to add MCP client %s: %v", MCPLogPrefix, clientConfig.Name, err) } }(clientConfig) } wg.Wait() }core/mcp/toolsync.go (1)
224-233: Add a nil guard for pointer-based configs.With the new pointer signature, a nil config would panic. A small guard makes the function safe and preserves default behavior.
🐛 Suggested fix
func ResolveToolSyncInterval(clientConfig *schemas.MCPClientConfig, globalInterval time.Duration) time.Duration { + if clientConfig == nil { + if globalInterval > 0 { + return globalInterval + } + return DefaultToolSyncInterval + } // Per-client explicitly disabled (negative value) if clientConfig.ToolSyncInterval < 0 { return 0 // Disabled for this client }core/bifrost.go (1)
2803-2824: RenameEditMCPClienttoUpdateMCPClientto match the new API.The comment and PR summary indicate
UpdateMCPClient, but the exported method is stillEditMCPClient, which will break updated call sites and leave the public API inconsistent.🐛 Proposed fix
-func (bifrost *Bifrost) EditMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { +func (bifrost *Bifrost) UpdateMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { if bifrost.mcpManager == nil { return fmt.Errorf("MCP is not configured in this Bifrost instance") } return bifrost.mcpManager.EditClient(id, updatedConfig) }core/mcp/clientmanager.go (1)
75-112: Initialize ConnectionInfo in placeholders to avoid nil deref.
connectToMCPClientassumesConnectionInfois non-nil (Line 442). The placeholders created inAddClient/AddClientInMemoryomit it, which will panic on the first connect attempt.🐛 Proposed fix
m.clientMap[config.ID] = &schemas.MCPClientState{ Name: config.Name, ExecutionConfig: config, ToolMap: make(map[string]schemas.ChatTool), ToolNameMapping: make(map[string]string), + ConnectionInfo: &schemas.MCPClientConnectionInfo{ + Type: config.ConnectionType, + }, } @@ m.clientMap[config.ID] = &schemas.MCPClientState{ Name: config.Name, ExecutionConfig: config, ToolMap: make(map[string]schemas.ChatTool), ToolNameMapping: make(map[string]string), + ConnectionInfo: &schemas.MCPClientConnectionInfo{ + Type: config.ConnectionType, + }, }Also applies to: 122-159
transports/bifrost-http/lib/config.go (1)
2958-2977: Redaction currently mutates the in-memory config.
configCopy := configis a shallow copy of the pointer, so redaction overwrites the live config (secrets become permanently redacted). Return a value copy instead.🛠️ Proposed fix
func (c *Config) RedactMCPClientConfig(config *schemas.MCPClientConfig) *schemas.MCPClientConfig { - // Create a shallow copy - configCopy := config + if config == nil { + return nil + } + // Create a value copy + configCopy := *config // Redact connection string if present if config.ConnectionString != nil { configCopy.ConnectionString = config.ConnectionString.Redacted() } @@ } - return configCopy + return &configCopy }framework/configstore/rdb.go (2)
795-822: Return the global MCP tool sync interval in the response.
clientConfig.MCPToolSyncIntervalis persisted but not propagated intoschemas.MCPConfig.ToolSyncInterval, so a configured global interval is ignored at runtime.🐛 Proposed fix
return &schemas.MCPConfig{ ClientConfigs: clientConfigs, ToolManagerConfig: &toolManagerConfig, + ToolSyncInterval: time.Duration(clientConfig.MCPToolSyncInterval) * time.Minute, }, nil
889-903: Persist ToolPricing on create.
ToolPricingfrom the schema isn't mapped into the table model, so new clients drop pricing metadata.🐛 Proposed fix
dbClient := tables.TableMCPClient{ ClientID: clientConfigCopy.ID, Name: clientConfigCopy.Name, IsCodeModeClient: clientConfigCopy.IsCodeModeClient, ConnectionType: string(clientConfigCopy.ConnectionType), ConnectionString: clientConfigCopy.ConnectionString, StdioConfig: clientConfigCopy.StdioConfig, AuthType: string(clientConfigCopy.AuthType), OauthConfigID: clientConfigCopy.OauthConfigID, ToolsToExecute: clientConfigCopy.ToolsToExecute, ToolsToAutoExecute: clientConfigCopy.ToolsToAutoExecute, Headers: clientConfigCopy.Headers, + ToolPricing: clientConfigCopy.ToolPricing, IsPingAvailable: clientConfigCopy.IsPingAvailable, ToolSyncInterval: int(clientConfigCopy.ToolSyncInterval.Minutes()), }
🤖 Fix all issues with AI agents
In `@core/internal/mcptests/tool_filtering_test.go`:
- Around line 336-339: The test mutates the original clientConfig before calling
manager.EditClient which can alter the stored config; instead create a copy of
clientConfig (e.g., clone or shallow copy) and modify the copy's ToolsToExecute,
then pass a pointer to that copy into manager.EditClient; use bifrost.Ptr when
constructing the pointer to the edited config and ensure you still call
require.NoError on manager.EditClient to validate the operation (reference:
clientConfig, ToolsToExecute, manager.EditClient, bifrost.Ptr).
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 321-384: The handler can panic if the request body is JSON null
(leaving req nil) and when dereferencing h.store.ConfigStore; guard both: after
json.Unmarshal into req, check if req == nil and return a BadRequest via
SendError if so (referencing req and the json.Unmarshal call), and before
calling h.store.ConfigStore.UpdateMCPClientConfig and
h.store.ConfigStore.GetClientConfig verify h.store.ConfigStore != nil and return
an InternalServerError (or appropriate error) if it's nil; ensure these checks
occur before mergeMCPRedactedValues, UpdateMCPClientConfig, and GetClientConfig
are invoked.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 449-451: The CreateMCPClientConfig mock can panic when m.mcpConfig
is nil; add a nil-check in MockConfigStore.CreateMCPClientConfig to initialize
m.mcpConfig (and ensure its ClientConfigs slice is non-nil) before appending,
and also initialize m.mcpConfigsCreated if it is nil; update the function to
guard: if m.mcpConfig == nil { m.mcpConfig =
&schemas.MCPClientConfig{ClientConfigs: []*schemas.MCPClientConfig{}} } (or
similar) then append to m.mcpConfig.ClientConfigs and m.mcpConfigsCreated.
🧹 Nitpick comments (10)
transports/bifrost-http/lib/config_test.go (1)
473-500: Usebifrost.Ptr(...)for MCPClientConfig pointers.
This keeps pointer creation consistent across the codebase (including tests).♻️ Example update
- m.mcpConfig.ClientConfigs[i] = &schemas.MCPClientConfig{ + m.mcpConfig.ClientConfigs[i] = bifrost.Ptr(schemas.MCPClientConfig{ ID: clientConfig.ClientID, Name: clientConfig.Name, IsCodeModeClient: clientConfig.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(clientConfig.ConnectionType), ConnectionString: clientConfig.ConnectionString, StdioConfig: clientConfig.StdioConfig, Headers: clientConfig.Headers, ToolsToExecute: clientConfig.ToolsToExecute, ToolsToAutoExecute: clientConfig.ToolsToAutoExecute, - } + })- err = config1.ConfigStore.CreateMCPClientConfig(ctx, &mcpClientConfig) + err = config1.ConfigStore.CreateMCPClientConfig(ctx, bifrost.Ptr(mcpClientConfig))Also applies to: 8981-8988, 9070-9087, 9392-9409, 9561-9564, 9684-9686, 9777-9780
core/internal/mcptests/health_monitoring_test.go (1)
53-53: Prefer the repo pointer helper for MCPClientConfig values.Use
bifrost.Ptr(...)to keep pointer creation consistent and avoid direct address operators in tests. Based on learnings, preferbifrost.Ptrfor pointer creation in this repo.♻️ Suggested change
import ( "testing" "time" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ - err = manager.AddClient(&clientConfig) + err = manager.AddClient(bifrost.Ptr(clientConfig)) @@ - err = manager.AddClient(&clientConfig) + err = manager.AddClient(bifrost.Ptr(clientConfig)) @@ - err = manager.AddClient(&clientConfig) + err = manager.AddClient(bifrost.Ptr(clientConfig))Also applies to: 184-184, 395-395
core/internal/mcptests/agent_filtering_test.go (1)
534-534: Prefer the repo pointer helper for MCPClientConfig values.Use
bifrost.Ptr(...)to keep pointer creation consistent and avoid direct address operators in tests. Based on learnings, preferbifrost.Ptrfor pointer creation in this repo.♻️ Suggested change
import ( "testing" "time" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ - err = manager.AddClient(&tempConfig) + err = manager.AddClient(bifrost.Ptr(tempConfig)) @@ - err = manager.AddClient(&tempConfig) + err = manager.AddClient(bifrost.Ptr(tempConfig))Also applies to: 623-623
core/internal/mcptests/agent_request_id_test.go (1)
26-30: Use bifrost.Ptr when building MCPConfig pointer slices.This keeps pointer creation consistent and avoids aliasing the variadic slice backing array. Based on learnings, prefer
bifrost.Ptrfor pointer creation in this repo.♻️ Suggested change
import ( "context" "fmt" "sync" "testing" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/mcp" "github.com/maximhq/bifrost/core/mcp/codemode/starlark" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ clientConfigPtrs := make([]*schemas.MCPClientConfig, len(clientConfigs)) for i := range clientConfigs { - clientConfigPtrs[i] = &clientConfigs[i] + clientConfigPtrs[i] = bifrost.Ptr(clientConfigs[i]) }core/internal/mcptests/concurrency_test.go (1)
438-441: Prefer the repo pointer helper for updatedConfig.Use
core.Ptr(updatedConfig)to keep pointer creation consistent with repo conventions. Based on learnings, preferbifrost.Ptr(via the core package alias) for pointer creation in this repo.♻️ Suggested change
- err := manager.EditClient(clientConfig.ID, &updatedConfig) + err := manager.EditClient(clientConfig.ID, core.Ptr(updatedConfig))core/internal/mcptests/fixtures.go (1)
1469-1473: Use bifrost.Ptr when building MCPConfig pointer slices.Keeps pointer creation consistent across tests and avoids direct address operators. Based on learnings, prefer
bifrost.Ptrfor pointer creation in this repo.♻️ Suggested change
clientConfigPtrs := make([]*schemas.MCPClientConfig, len(clientConfigs)) for i := range clientConfigs { - clientConfigPtrs[i] = &clientConfigs[i] + clientConfigPtrs[i] = bifrost.Ptr(clientConfigs[i]) }core/internal/mcptests/integration_test.go (1)
34-38: Prefer pointer helper for config creation.To align with the repo convention, consider creating the config pointer via
bifrost.Ptr(...)and passing it through (also avoids mixing & usages).♻️ Suggested refactor
- httpConfig := GetSampleHTTPClientConfig(config.HTTPServerURL) - httpConfig.ID = "http-integration-test" - applyTestConfigHeaders(t, &httpConfig) - err := manager.AddClient(&httpConfig) + httpConfig := bifrost.Ptr(GetSampleHTTPClientConfig(config.HTTPServerURL)) + httpConfig.ID = "http-integration-test" + applyTestConfigHeaders(t, httpConfig) + err := manager.AddClient(httpConfig)- httpConfig := GetSampleHTTPClientConfig(config.HTTPServerURL) - httpConfig.ID = "reconnect-test-client" - applyTestConfigHeaders(t, &httpConfig) - err := manager.AddClient(&httpConfig) + httpConfig := bifrost.Ptr(GetSampleHTTPClientConfig(config.HTTPServerURL)) + httpConfig.ID = "reconnect-test-client" + applyTestConfigHeaders(t, httpConfig) + err := manager.AddClient(httpConfig)Based on learnings, this is the preferred pointer-creation style in this repo.
Also applies to: 341-345
core/internal/mcptests/concurrency_advanced_test.go (1)
158-166: Use the pointer helper for client configs.This keeps pointer creation consistent across tests.
♻️ Suggested refactor
- clientConfig := schemas.MCPClientConfig{ + clientConfig := bifrost.Ptr(schemas.MCPClientConfig{ ID: fmt.Sprintf("test-client-%d", id), Name: fmt.Sprintf("TestClient%d", id), ConnectionType: schemas.MCPConnectionTypeInProcess, ToolsToExecute: []string{"*"}, ToolsToAutoExecute: []string{}, - } - - err := manager.AddClient(&clientConfig) + }) + + err := manager.AddClient(clientConfig)Based on learnings, prefer
bifrost.Ptrover&for pointer creation.core/internal/mcptests/error_handling_protocol_test.go (1)
291-293: Preferbifrost.Ptrfor config pointers.This aligns these test helpers with the repo’s pointer-creation convention.
♻️ Suggested refactor
- errorServerConfig := GetErrorTestServerConfig(bifrostRoot) + errorServerConfig := bifrost.Ptr(GetErrorTestServerConfig(bifrostRoot)) - err := manager.AddClient(&errorServerConfig) + err := manager.AddClient(errorServerConfig)Based on learnings, use
bifrost.Ptrinstead of&for pointer creation.Also applies to: 368-371, 422-424, 471-473
core/internal/mcptests/client_management_test.go (1)
27-33: Use the pointer helper for configs passed to AddClient/EditClient.This keeps test code consistent with the repo’s pointer-creation convention.
♻️ Suggested refactor
- clientConfig := GetSampleHTTPClientConfig(config.HTTPServerURL) - err := manager.AddClient(&clientConfig) + clientConfig := bifrost.Ptr(GetSampleHTTPClientConfig(config.HTTPServerURL)) + err := manager.AddClient(clientConfig)- err := manager.EditClient(clientID, &updatedConfig) + err := manager.EditClient(clientID, bifrost.Ptr(updatedConfig))Based on learnings, prefer
bifrost.Ptrover&for pointer creation.Also applies to: 181-187, 200-203, 221-229, 256-261, 426-432
| // Edit client to only allow second tool | ||
| clientConfig.ToolsToExecute = []string{"hash"} | ||
| err := manager.EditClient(clientConfig.ID, clientConfig) | ||
| err := manager.EditClient(clientConfig.ID, &clientConfig) | ||
| require.NoError(t, err, "edit should succeed") |
There was a problem hiding this comment.
Avoid mutating the original config before EditClient.
Since configs are pointer-based now, mutating clientConfig here can modify the manager’s stored config before EditClient runs, which weakens the test. Use a copied config for the edit (and the pointer helper for consistency).
🐛 Suggested fix
- clientConfig.ToolsToExecute = []string{"hash"}
- err := manager.EditClient(clientConfig.ID, &clientConfig)
+ updatedConfig := clientConfig
+ updatedConfig.ToolsToExecute = []string{"hash"}
+ err := manager.EditClient(clientConfig.ID, bifrost.Ptr(updatedConfig))Based on learnings, prefer bifrost.Ptr for pointer creation.
🤖 Prompt for AI Agents
In `@core/internal/mcptests/tool_filtering_test.go` around lines 336 - 339, The
test mutates the original clientConfig before calling manager.EditClient which
can alter the stored config; instead create a copy of clientConfig (e.g., clone
or shallow copy) and modify the copy's ToolsToExecute, then pass a pointer to
that copy into manager.EditClient; use bifrost.Ptr when constructing the pointer
to the edited config and ensure you still call require.NoError on
manager.EditClient to validate the operation (reference: clientConfig,
ToolsToExecute, manager.EditClient, bifrost.Ptr).
| // Accept the full table client config to support tool_pricing | ||
| var req configstoreTables.TableMCPClient | ||
| var req *configstoreTables.TableMCPClient | ||
| if err := json.Unmarshal(ctx.PostBody(), &req); err != nil { | ||
| SendError(ctx, fasthttp.StatusBadRequest, fmt.Sprintf("Invalid request format: %v", err)) | ||
| return | ||
| } | ||
|
|
||
| req.ClientID = id | ||
|
|
||
| // Validate tools_to_execute | ||
| if err := validateToolsToExecute(req.ToolsToExecute); err != nil { | ||
| SendError(ctx, fasthttp.StatusBadRequest, fmt.Sprintf("Invalid tools_to_execute: %v", err)) | ||
| return | ||
| } | ||
|
|
||
| // Auto-clear tools_to_auto_execute if tools_to_execute is empty | ||
| // If no tools are allowed to execute, no tools can be auto-executed | ||
| if len(req.ToolsToExecute) == 0 { | ||
| req.ToolsToAutoExecute = []string{} | ||
| } | ||
|
|
||
| // Validate tools_to_auto_execute | ||
| if err := validateToolsToAutoExecute(req.ToolsToAutoExecute, req.ToolsToExecute); err != nil { | ||
| SendError(ctx, fasthttp.StatusBadRequest, fmt.Sprintf("Invalid tools_to_auto_execute: %v", err)) | ||
| return | ||
| } | ||
|
|
||
| // Validate client name | ||
| if err := validateMCPClientName(req.Name); err != nil { | ||
| SendError(ctx, fasthttp.StatusBadRequest, fmt.Sprintf("Invalid client name: %v", err)) | ||
| return | ||
| } | ||
|
|
||
| // Get existing config to handle redacted values | ||
| var existingConfig *configstoreTables.TableMCPClient | ||
| var existingConfig *schemas.MCPClientConfig | ||
| if h.store.MCPConfig != nil { | ||
| for i, client := range h.store.MCPConfig.ClientConfigs { | ||
| if client.ClientID == id { | ||
| existingConfig = &h.store.MCPConfig.ClientConfigs[i] | ||
| if client.ID == id { | ||
| existingConfig = h.store.MCPConfig.ClientConfigs[i] | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if existingConfig == nil { | ||
| SendError(ctx, fasthttp.StatusNotFound, "MCP client not found") | ||
| return | ||
| } | ||
|
|
||
| // Merge redacted values - preserve old values if incoming values are redacted and unchanged | ||
| req = mergeMCPRedactedValues(req, *existingConfig, h.store.RedactTableMCPClient(*existingConfig)) | ||
| req = mergeMCPRedactedValues(req, existingConfig, h.store.RedactMCPClientConfig(existingConfig)) | ||
| // Persist changes to config store | ||
| if h.store.ConfigStore != nil { | ||
| if err := h.store.ConfigStore.UpdateMCPClientConfig(ctx, id, req); err != nil { | ||
| SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("failed to update mcp client config in store: %v", err)) | ||
| return | ||
| } | ||
| } | ||
| toolSyncInterval := 10 * time.Minute | ||
| if req.ToolSyncInterval != 0 { | ||
| toolSyncInterval = time.Duration(req.ToolSyncInterval) * time.Minute | ||
| } else { | ||
| config, err := h.store.ConfigStore.GetClientConfig(ctx) | ||
| if err != nil { | ||
| SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("failed to get client config: %v", err)) | ||
| return | ||
| } | ||
| if config != nil { | ||
| toolSyncInterval = time.Duration(config.MCPToolSyncInterval) * time.Minute | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard against nil request bodies and nil ConfigStore.
Line 323 can leave req nil when the body is null, and Line 376 dereferences h.store.ConfigStore without checking for nil. Both cases can panic.
🛠️ Proposed fix
var req *configstoreTables.TableMCPClient
if err := json.Unmarshal(ctx.PostBody(), &req); err != nil {
SendError(ctx, fasthttp.StatusBadRequest, fmt.Sprintf("Invalid request format: %v", err))
return
}
+ if req == nil {
+ SendError(ctx, fasthttp.StatusBadRequest, "Request body is required")
+ return
+ }
req.ClientID = id
@@
toolSyncInterval := 10 * time.Minute
if req.ToolSyncInterval != 0 {
toolSyncInterval = time.Duration(req.ToolSyncInterval) * time.Minute
- } else {
+ } else if h.store.ConfigStore != nil {
config, err := h.store.ConfigStore.GetClientConfig(ctx)
if err != nil {
SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("failed to get client config: %v", err))
return
}
if config != nil {
toolSyncInterval = time.Duration(config.MCPToolSyncInterval) * time.Minute
}
+ } else if h.store != nil {
+ toolSyncInterval = time.Duration(h.store.ClientConfig.MCPToolSyncInterval) * time.Minute
}🤖 Prompt for AI Agents
In `@transports/bifrost-http/handlers/mcp.go` around lines 321 - 384, The handler
can panic if the request body is JSON null (leaving req nil) and when
dereferencing h.store.ConfigStore; guard both: after json.Unmarshal into req,
check if req == nil and return a BadRequest via SendError if so (referencing req
and the json.Unmarshal call), and before calling
h.store.ConfigStore.UpdateMCPClientConfig and
h.store.ConfigStore.GetClientConfig verify h.store.ConfigStore != nil and return
an InternalServerError (or appropriate error) if it's nil; ensure these checks
occur before mergeMCPRedactedValues, UpdateMCPClientConfig, and GetClientConfig
are invoked.
| func (m *MockConfigStore) CreateMCPClientConfig(ctx context.Context, clientConfig *schemas.MCPClientConfig) error { | ||
| m.mcpConfig.ClientConfigs = append(m.mcpConfig.ClientConfigs, clientConfig) | ||
| m.mcpConfigsCreated = append(m.mcpConfigsCreated, clientConfig) |
There was a problem hiding this comment.
Guard CreateMCPClientConfig against a nil mcpConfig.
m.mcpConfig can be nil in the mock (e.g., when Create is the first call), which will panic when appending to ClientConfigs.
🐛 Suggested fix
func (m *MockConfigStore) CreateMCPClientConfig(ctx context.Context, clientConfig *schemas.MCPClientConfig) error {
+ if m.mcpConfig == nil {
+ m.mcpConfig = &schemas.MCPConfig{
+ ClientConfigs: []*schemas.MCPClientConfig{},
+ }
+ }
m.mcpConfig.ClientConfigs = append(m.mcpConfig.ClientConfigs, clientConfig)
m.mcpConfigsCreated = append(m.mcpConfigsCreated, clientConfig)
return nil
}🤖 Prompt for AI Agents
In `@transports/bifrost-http/lib/config_test.go` around lines 449 - 451, The
CreateMCPClientConfig mock can panic when m.mcpConfig is nil; add a nil-check in
MockConfigStore.CreateMCPClientConfig to initialize m.mcpConfig (and ensure its
ClientConfigs slice is non-nil) before appending, and also initialize
m.mcpConfigsCreated if it is nil; update the function to guard: if m.mcpConfig
== nil { m.mcpConfig = &schemas.MCPClientConfig{ClientConfigs:
[]*schemas.MCPClientConfig{}} } (or similar) then append to
m.mcpConfig.ClientConfigs and m.mcpConfigsCreated.
akshaydeo
force-pushed
the
01-27-mcp_flow_refactoring
branch
from
098065e to
5a99bfb
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 (6)
core/bifrost.go (2)
2740-2769: Guard against nil MCP client config.With pointer-based configs, a nil input will likely panic downstream in
AddClient. Return a clear error early.🛠️ Suggested fix
func (bifrost *Bifrost) AddMCPClient(config *schemas.MCPClientConfig) error { + if config == nil { + return fmt.Errorf("config is required") + } if bifrost.mcpManager == nil { // Use sync.Once to ensure thread-safe initialization bifrost.mcpInitOnce.Do(func() {
2804-2826: Rename the exported method to UpdateMCPClient (currently still EditMCPClient).Comments/examples indicate
UpdateMCPClient, but the function name remainsEditMCPClient. If call sites were updated as the PR summary suggests, this will fail to compile and leaves the public API inconsistent.🛠️ Suggested fix
-// UpdateMCPClient updates the MCP client. +// UpdateMCPClient updates the MCP client. // This allows for dynamic MCP client tool management at runtime. @@ -// err := bifrost.UpdateMCPClient("my-mcp-client-id", schemas.MCPClientConfig{ +// err := bifrost.UpdateMCPClient("my-mcp-client-id", schemas.MCPClientConfig{ // Name: "my-mcp-client-name", // ToolsToExecute: []string{"tool1", "tool2"}, // }) -func (bifrost *Bifrost) EditMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { +func (bifrost *Bifrost) UpdateMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { + if updatedConfig == nil { + return fmt.Errorf("updatedConfig is required") + } if bifrost.mcpManager == nil { return fmt.Errorf("MCP is not configured in this Bifrost instance") } return bifrost.mcpManager.EditClient(id, updatedConfig) }framework/configstore/rdb.go (1)
851-873: Guard nil configs and persistToolPricingon create.
CreateMCPClientConfigandUpdateMCPClientConfigdereference pointer inputs without a nil check, which can panic. Also,ToolPricingis not persisted during create, so pricing data gets dropped.🛠️ Proposed fix
func (s *RDBConfigStore) CreateMCPClientConfig(ctx context.Context, clientConfig *schemas.MCPClientConfig) error { + if clientConfig == nil { + return fmt.Errorf("clientConfig cannot be nil") + } return s.db.Transaction(func(tx *gorm.DB) error { // Create a deep copy to avoid modifying the original clientConfigCopy, err := deepCopy(*clientConfig) if err != nil { return err } @@ dbClient := tables.TableMCPClient{ ClientID: clientConfigCopy.ID, Name: clientConfigCopy.Name, IsCodeModeClient: clientConfigCopy.IsCodeModeClient, ConnectionType: string(clientConfigCopy.ConnectionType), ConnectionString: clientConfigCopy.ConnectionString, StdioConfig: clientConfigCopy.StdioConfig, AuthType: string(clientConfigCopy.AuthType), OauthConfigID: clientConfigCopy.OauthConfigID, ToolsToExecute: clientConfigCopy.ToolsToExecute, ToolsToAutoExecute: clientConfigCopy.ToolsToAutoExecute, Headers: clientConfigCopy.Headers, IsPingAvailable: clientConfigCopy.IsPingAvailable, ToolSyncInterval: int(clientConfigCopy.ToolSyncInterval.Minutes()), + ToolPricing: clientConfigCopy.ToolPricing, } @@ func (s *RDBConfigStore) UpdateMCPClientConfig(ctx context.Context, id string, clientConfig *tables.TableMCPClient) error { + if clientConfig == nil { + return fmt.Errorf("clientConfig cannot be nil") + } return s.db.Transaction(func(tx *gorm.DB) error { // Find existing client var existingClient tables.TableMCPClientAlso applies to: 882-895
core/mcp/clientmanager.go (1)
431-453: InitializeConnectionInfobefore dereferencing.When a placeholder entry exists (created by
AddClient/AddClientInMemory),ConnectionInfocan be nil.existingClient.ConnectionInfo.Type = ...will panic. Initialize it (or skip this write) before use.🐛 Proposed fix
if existingClient, exists := m.clientMap[config.ID]; exists { // Client entry exists from config, check for existing connection, if it does then close if existingClient.CancelFunc != nil { existingClient.CancelFunc() existingClient.CancelFunc = nil } if existingClient.Conn != nil { existingClient.Conn.Close() } // Update connection type for this connection attempt - existingClient.ConnectionInfo.Type = config.ConnectionType + if existingClient.ConnectionInfo == nil { + existingClient.ConnectionInfo = &schemas.MCPClientConnectionInfo{} + } + existingClient.ConnectionInfo.Type = config.ConnectionType }transports/bifrost-http/lib/config.go (2)
2958-2977: Redaction mutates live config due to shallow copy.
configCopy := configkeeps the same pointer, so redaction overwrites the stored secrets in memory. Create a value copy before redacting to avoid corrupting runtime config.🛠️ Proposed fix
func (c *Config) RedactMCPClientConfig(config *schemas.MCPClientConfig) *schemas.MCPClientConfig { - // Create a shallow copy - configCopy := config + if config == nil { + return nil + } + // Create a value copy + configCopy := *config + redacted := &configCopy // Redact connection string if present if config.ConnectionString != nil { - configCopy.ConnectionString = config.ConnectionString.Redacted() + redacted.ConnectionString = config.ConnectionString.Redacted() } // Redact Header values if present if config.Headers != nil { - configCopy.Headers = make(map[string]schemas.EnvVar, len(config.Headers)) + redacted.Headers = make(map[string]schemas.EnvVar, len(config.Headers)) for header, value := range config.Headers { - configCopy.Headers[header] = *value.Redacted() + redacted.Headers[header] = *value.Redacted() } } - return configCopy + return redacted }
2946-2954: In-memory MCP config drops updates for new fields.After a successful update, only name/flags/headers/tools/tool_pricing are copied back to
c.MCPConfig. Fields likeIsPingAvailableandToolSyncInterval(sent by the handler) are silently discarded, so future reads/redaction use stale values. Either persist these fields here or explicitly reject edits to them.🛠️ Proposed fix
c.MCPConfig.ClientConfigs[configIndex].Name = updatedConfig.Name c.MCPConfig.ClientConfigs[configIndex].IsCodeModeClient = updatedConfig.IsCodeModeClient c.MCPConfig.ClientConfigs[configIndex].Headers = updatedConfig.Headers c.MCPConfig.ClientConfigs[configIndex].ToolsToExecute = updatedConfig.ToolsToExecute c.MCPConfig.ClientConfigs[configIndex].ToolsToAutoExecute = updatedConfig.ToolsToAutoExecute c.MCPConfig.ClientConfigs[configIndex].ToolPricing = updatedConfig.ToolPricing + c.MCPConfig.ClientConfigs[configIndex].IsPingAvailable = updatedConfig.IsPingAvailable + c.MCPConfig.ClientConfigs[configIndex].ToolSyncInterval = updatedConfig.ToolSyncInterval
🤖 Fix all issues with AI agents
In `@core/bifrost.go`:
- Around line 2104-2119: The RemovePlugin method currently ignores empty or
unsupported pluginTypes which can hide misuse; update RemovePlugin (and the
other similar method around the second occurrence) to validate pluginTypes: if
the slice is empty return an error like "no plugin types provided", and inside
the loop add a default case that returns an error for unknown schemas.PluginType
values instead of silently continuing; reference the existing helper methods
removeLLMPlugin and removeMCPPlugin when describing allowed types in the error
message so callers know valid options.
🧹 Nitpick comments (2)
transports/bifrost-http/lib/config_test.go (1)
8982-8988: Useschemas.Ptr(...)instead of&for MCPClientConfig pointers.Lines 8987 and similar call sites currently use
&for pointer creation. The repo convention in test files is to useschemas.Ptrconsistently, which is already imported in this file.♻️ Example refactor
- mcpClientConfig := schemas.MCPClientConfig{ + mcpClientConfig := schemas.MCPClientConfig{ ID: "mcp-client-1", Name: "test-mcp-client", ConnectionType: schemas.MCPConnectionTypeHTTP, } - err = config1.ConfigStore.CreateMCPClientConfig(ctx, &mcpClientConfig) + err = config1.ConfigStore.CreateMCPClientConfig(ctx, schemas.Ptr(mcpClientConfig))- config1.ConfigStore.CreateMCPClientConfig(ctx, &schemas.MCPClientConfig{ID: "mcp-1", Name: "mcp-1", ConnectionType: schemas.MCPConnectionTypeHTTP}) + config1.ConfigStore.CreateMCPClientConfig(ctx, schemas.Ptr(schemas.MCPClientConfig{ID: "mcp-1", Name: "mcp-1", ConnectionType: schemas.MCPConnectionTypeHTTP}))Also applies to: 9071-9087, 9393-9409, 9562-9564, 9685-9686, 9779-9780
core/internal/mcptests/client_management_test.go (1)
28-33: Preferbifrost.Ptr(...)for pointer construction in tests.This keeps pointer creation consistent with repo conventions and avoids mixing
&valuewith the helper used elsewhere.
Based on learnings, preferbifrost.Ptr(...)over&valuefor pointer creation.♻️ Example update (apply to all similar call sites)
import ( "testing" "time" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ - err := manager.AddClient(&clientConfig) + err := manager.AddClient(bifrost.Ptr(clientConfig))Also applies to: 186-187, 202-203, 228-229, 260-261, 431-432
| func (bifrost *Bifrost) RemovePlugin(name string, pluginTypes []schemas.PluginType) error { | ||
| for _, pluginType := range pluginTypes { | ||
| switch pluginType { | ||
| case schemas.PluginTypeLLM: | ||
| err := bifrost.removeLLMPlugin(name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| case schemas.PluginTypeMCP: | ||
| err := bifrost.removeMCPPlugin(name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Validate pluginTypes and reject unknown values (avoid silent no-ops).
Both methods now accept a slice but silently succeed on empty or unsupported types, which can mask misuse. Consider returning a clear error in those cases.
🛠️ Suggested fix
func (bifrost *Bifrost) RemovePlugin(name string, pluginTypes []schemas.PluginType) error {
+ if len(pluginTypes) == 0 {
+ return fmt.Errorf("pluginTypes is required")
+ }
for _, pluginType := range pluginTypes {
switch pluginType {
case schemas.PluginTypeLLM:
err := bifrost.removeLLMPlugin(name)
if err != nil {
return err
}
case schemas.PluginTypeMCP:
err := bifrost.removeMCPPlugin(name)
if err != nil {
return err
}
+ default:
+ return fmt.Errorf("unsupported plugin type: %s", pluginType)
}
}
return nil
}
func (bifrost *Bifrost) ReloadPlugin(plugin schemas.BasePlugin, pluginTypes []schemas.PluginType) error {
+ if len(pluginTypes) == 0 {
+ return fmt.Errorf("pluginTypes is required")
+ }
for _, pluginType := range pluginTypes {
switch pluginType {
case schemas.PluginTypeLLM:
llmPlugin, ok := plugin.(schemas.LLMPlugin)
if !ok {
return fmt.Errorf("plugin %s is not an LLMPlugin", plugin.GetName())
}
err := bifrost.reloadLLMPlugin(llmPlugin)
if err != nil {
return err
}
case schemas.PluginTypeMCP:
mcpPlugin, ok := plugin.(schemas.MCPPlugin)
if !ok {
return fmt.Errorf("plugin %s is not an MCPPlugin", plugin.GetName())
}
err := bifrost.reloadMCPPlugin(mcpPlugin)
if err != nil {
return err
}
+ default:
+ return fmt.Errorf("unsupported plugin type: %s", pluginType)
}
}
return nil
}Also applies to: 2196-2219
🤖 Prompt for AI Agents
In `@core/bifrost.go` around lines 2104 - 2119, The RemovePlugin method currently
ignores empty or unsupported pluginTypes which can hide misuse; update
RemovePlugin (and the other similar method around the second occurrence) to
validate pluginTypes: if the slice is empty return an error like "no plugin
types provided", and inside the loop add a default case that returns an error
for unknown schemas.PluginType values instead of silently continuing; reference
the existing helper methods removeLLMPlugin and removeMCPPlugin when describing
allowed types in the error message so callers know valid options.
5a99bfb to
bea260e
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 (9)
core/mcp/mcp.go (1)
122-128: Guard against nil client configs before dereferencing.With
ClientConfigsnow as a pointer slice, a nil entry would panic onclientConfig.Nameor insideAddClient. Consider skipping nil configs (or validating earlier) so initialization doesn’t crash.🛠️ Suggested defensive guard
for _, clientConfig := range config.ClientConfigs { go func(clientConfig *schemas.MCPClientConfig) { defer wg.Done() + if clientConfig == nil { + logger.Warn("%s Skipping nil MCP client config", MCPLogPrefix) + return + } if err := manager.AddClient(clientConfig); err != nil { logger.Warn("%s Failed to add MCP client %s: %v", MCPLogPrefix, clientConfig.Name, err) } }(clientConfig) }core/mcp/toolsync.go (1)
225-233: Handle nil clientConfig to avoid a panic.
ResolveToolSyncIntervalnow accepts a pointer; if nil slips through, dereferencingToolSyncIntervalwill panic. Add a guard to fall back to the global/default interval.🛠️ Suggested guard
func ResolveToolSyncInterval(clientConfig *schemas.MCPClientConfig, globalInterval time.Duration) time.Duration { + if clientConfig == nil { + if globalInterval > 0 { + return globalInterval + } + return DefaultToolSyncInterval + } // Per-client explicitly disabled (negative value) if clientConfig.ToolSyncInterval < 0 { return 0 // Disabled for this client }framework/configstore/rdb.go (2)
851-873: Persist ToolPricing on create
ToolPricingis mapped on read and update, but it’s not persisted when creating a new MCP client. This drops pricing from config.json/UI on initial insert.🐛 Proposed fix
dbClient := tables.TableMCPClient{ ClientID: clientConfigCopy.ID, Name: clientConfigCopy.Name, IsCodeModeClient: clientConfigCopy.IsCodeModeClient, ConnectionType: string(clientConfigCopy.ConnectionType), ConnectionString: clientConfigCopy.ConnectionString, StdioConfig: clientConfigCopy.StdioConfig, AuthType: string(clientConfigCopy.AuthType), OauthConfigID: clientConfigCopy.OauthConfigID, ToolsToExecute: clientConfigCopy.ToolsToExecute, ToolsToAutoExecute: clientConfigCopy.ToolsToAutoExecute, Headers: clientConfigCopy.Headers, IsPingAvailable: clientConfigCopy.IsPingAvailable, ToolSyncInterval: int(clientConfigCopy.ToolSyncInterval.Minutes()), + ToolPricing: clientConfigCopy.ToolPricing, }
919-946: FixIsPingAvailablepreservation in updatesThe guard
if clientConfigCopy.IsPingAvailable { clientConfigCopy.IsPingAvailable = true }is a no-op. The update map always writesis_ping_availableunconditionally, andmergeMCPRedactedValuesdoes not preserve it from the existing client (unlikeConnectionStringandHeaders). When an API request omitsis_ping_available, it unmarshals tofalse, inadvertently overwriting the existing value and disabling ping. Preserve the existing value when not explicitly set in the request, similar to how redacted fields are handled—either by extendingmergeMCPRedactedValuesor by excluding the field from the updates map when it's not explicitly provided.core/bifrost.go (2)
2741-2753: Update AddMCPClient example to use pointer.The signature now takes
*schemas.MCPClientConfig(Line 2746), but the example still shows a value.✏️ Doc fix
-// err := bifrost.AddMCPClient(schemas.MCPClientConfig{ +// err := bifrost.AddMCPClient(&schemas.MCPClientConfig{ // Name: "my-mcp-client", // ConnectionType: schemas.MCPConnectionTypeHTTP, // ConnectionString: &url, // })
2810-2832: Public API name mismatch: method isEditMCPClientbut documentation referencesUpdateMCPClient.The method at line 2826 is named
EditMCPClient, but the docstring example (line 2822) callsUpdateMCPClient. This creates an inconsistency with the public API: the higher-level wrappers inConfigandBifrostHTTPServerexposeUpdateMCPClient, while the coreBifrosttype usesEditMCPClient.There is one internal call site that directly uses
EditMCPClient(transports/bifrost-http/lib/config.go:2914). Align the core type with the public API naming by either renaming toUpdateMCPClientor adding a wrapper method.transports/bifrost-http/lib/config.go (3)
851-863: Add nil guards when iterating MCP client pointers.
ClientConfigsis now[]*MCPClientConfig, so a nil entry will panic during merge or lookup. Guard before dereferencing to keep startup/runtime resilient.🔧 Suggested fix
for _, newClientConfig := range tempMCPConfig.ClientConfigs { + if newClientConfig == nil { + continue + } found := false for _, existingClientConfig := range mcpConfig.ClientConfigs { + if existingClientConfig == nil { + continue + } if (newClientConfig.ID != "" && existingClientConfig.ID == newClientConfig.ID) || (newClientConfig.Name != "" && existingClientConfig.Name == newClientConfig.Name) { found = true break } } if !found { clientConfigsToAdd = append(clientConfigsToAdd, newClientConfig) } }for _, clientConfig := range c.MCPConfig.ClientConfigs { + if clientConfig == nil { + continue + } if clientConfig.ID == id { return clientConfig, nil } }Also applies to: 2768-2771
1483-1496: ToolPricing is dropped when converting schema → table.
convertSchemasMCPClientConfigToTabledoesn’t mapToolPricing, so persisted pricing can be lost and MCP catalog sync becomes inconsistent.🔧 Suggested fix
return &configstoreTables.TableMCPClient{ ClientID: clientConfig.ID, Name: clientConfig.Name, IsCodeModeClient: clientConfig.IsCodeModeClient, ConnectionType: string(clientConfig.ConnectionType), ConnectionString: clientConfig.ConnectionString, StdioConfig: clientConfig.StdioConfig, ToolsToExecute: clientConfig.ToolsToExecute, ToolsToAutoExecute: clientConfig.ToolsToAutoExecute, Headers: clientConfig.Headers, AuthType: string(clientConfig.AuthType), OauthConfigID: clientConfig.OauthConfigID, + ToolPricing: clientConfig.ToolPricing, }
2958-2977: Redaction mutates the original MCP client config.
configCopy := configkeeps the same pointer, so redaction overwrites the stored secrets. This can permanently lose connection strings/headers in memory.🔧 Suggested fix
func (c *Config) RedactMCPClientConfig(config *schemas.MCPClientConfig) *schemas.MCPClientConfig { - // Create a shallow copy - configCopy := config + if config == nil { + return nil + } + // Create a copy so we don't mutate the original + configCopy := *config // Redact connection string if present if config.ConnectionString != nil { configCopy.ConnectionString = config.ConnectionString.Redacted() } // Redact Header values if present if config.Headers != nil { configCopy.Headers = make(map[string]schemas.EnvVar, len(config.Headers)) for header, value := range config.Headers { configCopy.Headers[header] = *value.Redacted() } } - return configCopy + return &configCopy }
🧹 Nitpick comments (6)
transports/bifrost-http/lib/config_test.go (1)
8982-8988: Usebifrost.Ptr(...)for MCPClientConfig pointers (repo convention).Prefer
bifrost.Ptr(...)over&when passing pointer configs for consistency. Apply across these call sites.♻️ Example adjustment
-err = config1.ConfigStore.CreateMCPClientConfig(ctx, &mcpClientConfig) +err = config1.ConfigStore.CreateMCPClientConfig(ctx, bifrost.Ptr(mcpClientConfig))Based on learnings.
Also applies to: 9071-9077, 9081-9087, 9392-9409, 9562-9564, 9684-9686, 9778-9780
core/internal/mcptests/error_handling_protocol_test.go (1)
292-292: Usebifrost.Ptrfor MCPClientConfig pointers.Repository convention prefers
bifrost.Ptr(value)over&valuefor pointer creation. Apply this to the AddClient calls (and add the import). Based on learnings, preferbifrost.Ptrfor pointer creation.♻️ Example adjustment (apply to all AddClient call sites)
import ( "encoding/json" "fmt" "strings" "testing" "time" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -err := manager.AddClient(&errorServerConfig) +err := manager.AddClient(bifrost.Ptr(errorServerConfig))Also applies to: 370-370, 423-423, 472-472
core/internal/mcptests/integration_test.go (1)
34-38: Usebifrost.Ptrfor MCPClientConfig pointers.Follow the repo convention and pass configs via
bifrost.Ptrinstead of&valueat both call sites (with import update). Based on learnings, preferbifrost.Ptrfor pointer creation.♻️ Example adjustment (apply to both AddClient call sites)
import ( "fmt" "testing" "time" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -err := manager.AddClient(&httpConfig) +err := manager.AddClient(bifrost.Ptr(httpConfig))Also applies to: 341-345
core/internal/mcptests/fixtures.go (1)
1469-1473: Usebifrost.Ptrwhen building the client config pointer slice.This aligns with the repository’s pointer-creation convention and avoids direct address-of usage. Based on learnings, prefer
bifrost.Ptrfor pointer creation.♻️ Suggested change
clientConfigPtrs := make([]*schemas.MCPClientConfig, len(clientConfigs)) for i := range clientConfigs { - clientConfigPtrs[i] = &clientConfigs[i] + clientConfigPtrs[i] = bifrost.Ptr(clientConfigs[i]) }framework/configstore/rdb.go (1)
752-823: Preferbifrost.Ptrfor MCPClientConfig pointers (consistency)Repo convention favors
bifrost.Ptr(...)over&literal; consider applying it to theMCPClientConfigliterals in both branches for consistency.Based on learnings, apply `bifrost.Ptr` consistently where pointers are created.♻️ Example change
- clientConfigs[i] = &schemas.MCPClientConfig{ + clientConfigs[i] = bifrost.Ptr(schemas.MCPClientConfig{ ID: dbClient.ClientID, Name: dbClient.Name, // ... - } + })transports/bifrost-http/handlers/mcp.go (1)
289-302: Preferbifrost.Ptrfor MCPClientConfig pointers.Use the repo-standard helper for pointer creation instead of
&{...}to keep consistency.♻️ Proposed refactor
- schemasConfig := &schemas.MCPClientConfig{ + schemasConfig := bifrost.Ptr(schemas.MCPClientConfig{ ID: req.ClientID, Name: req.Name, IsCodeModeClient: req.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(req.ConnectionType), ConnectionString: req.ConnectionString, StdioConfig: req.StdioConfig, AuthType: schemas.MCPAuthType(req.AuthType), OauthConfigID: nil, ToolsToExecute: req.ToolsToExecute, ToolsToAutoExecute: req.ToolsToAutoExecute, Headers: req.Headers, - } + }) @@ - schemasConfig := &schemas.MCPClientConfig{ + schemasConfig := bifrost.Ptr(schemas.MCPClientConfig{ ID: req.ClientID, Name: req.Name, IsCodeModeClient: req.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(req.ConnectionType), ConnectionString: req.ConnectionString, StdioConfig: req.StdioConfig, ToolsToExecute: req.ToolsToExecute, ToolsToAutoExecute: req.ToolsToAutoExecute, Headers: req.Headers, AuthType: schemas.MCPAuthType(req.AuthType), OauthConfigID: req.OauthConfigID, IsPingAvailable: req.IsPingAvailable, ToolSyncInterval: toolSyncInterval, ToolPricing: req.ToolPricing, - } + })Based on learnings, prefer
bifrost.Ptr(...)for pointer creation in this repo.Also applies to: 387-402
akshaydeo
force-pushed
the
01-27-mcp_flow_refactoring
branch
from
bea260e to
31da0d6
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 (6)
core/bifrost.go (2)
2746-2776: AddMCPClient should reject nil configs.
The new pointer signature allows nil, which can panic or misbehave downstream; fail fast. Also update the example to pass a pointer.🛠️ Suggested fix
func (bifrost *Bifrost) AddMCPClient(config *schemas.MCPClientConfig) error { + if config == nil { + return fmt.Errorf("config is required") + } if bifrost.mcpManager == nil { // Use sync.Once to ensure thread-safe initialization
2810-2832: Rename wrapper to UpdateMCPClient and validate updatedConfig.
Docs/example already use UpdateMCPClient; keeping EditMCPClient will diverge from callers after the refactor. Also guard against nil pointers.🛠️ Suggested fix
-func (bifrost *Bifrost) EditMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { +func (bifrost *Bifrost) UpdateMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { + if updatedConfig == nil { + return fmt.Errorf("updatedConfig is required") + } if bifrost.mcpManager == nil { return fmt.Errorf("MCP is not configured in this Bifrost instance") } return bifrost.mcpManager.EditClient(id, updatedConfig) }framework/configstore/rdb.go (3)
752-822: Propagate global MCPToolSyncInterval into MCPConfig.
GetMCPConfig builds ToolManagerConfig but doesn’t map TableClientConfig.MCPToolSyncInterval, so custom global sync intervals are ignored.🛠️ Suggested fix
return &schemas.MCPConfig{ ClientConfigs: clientConfigs, + ToolSyncInterval: time.Duration(clientConfig.MCPToolSyncInterval) * time.Minute, ToolManagerConfig: &toolManagerConfig, }, nil }
851-876: CreateMCPClientConfig should reject nil input.
Pointer signature allows nil; deepCopy(*clientConfig) will panic.🛠️ Suggested fix
func (s *RDBConfigStore) CreateMCPClientConfig(ctx context.Context, clientConfig *schemas.MCPClientConfig) error { + if clientConfig == nil { + return fmt.Errorf("clientConfig is required") + } return s.db.Transaction(func(tx *gorm.DB) error {
882-947: Guard UpdateMCPClientConfig against nil and clarify IsPingAvailable defaulting.
Nil pointers will panic in deepCopy. Also,if clientConfigCopy.IsPingAvailable { clientConfigCopy.IsPingAvailable = true }is a no‑op; if you intended default‑true when omitted, you’ll need a tri‑state (e.g., *bool) or explicit presence info from callers.🛠️ Suggested fix
func (s *RDBConfigStore) UpdateMCPClientConfig(ctx context.Context, id string, clientConfig *tables.TableMCPClient) error { + if clientConfig == nil { + return fmt.Errorf("clientConfig is required") + } return s.db.Transaction(func(tx *gorm.DB) error {transports/bifrost-http/handlers/mcp.go (1)
145-154: Add nil check forclientConfigafter retrieval.If
GetMCPClientreturnsnilwithout an error (e.g., client not found), passingniltoAddMCPClientcould cause unexpected behavior or panics downstream.🛠️ Proposed fix
clientConfig, err := h.store.GetMCPClient(id) if err != nil { SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Failed to get MCP client config: %v", err)) return } + if clientConfig == nil { + SendError(ctx, fasthttp.StatusNotFound, "MCP client config not found") + return + } if err := h.client.AddMCPClient(clientConfig); err != nil {
🤖 Fix all issues with AI agents
In `@transports/bifrost-http/server/utils.go`:
- Around line 96-98: The call to s.Config.ReloadPlugin currently ignores its
error so the code always calls s.Config.UpdatePluginOverallStatus(name, name,
schemas.PluginStatusActive,...), which hides reload failures; change the
sequence in the function containing s.Config.ReloadPlugin to capture the
returned error, and if err != nil log the error and call
s.Config.UpdatePluginOverallStatus(name, name, schemas.PluginStatusInactive,
[]string{fmt.Sprintf("%s plugin reload failed: %v", name, err)}, nil) (or
appropriate failure messages), otherwise proceed to mark active and call
InferPluginTypes(plugin) as currently done; follow the same error-handling
pattern used around Config.ReloadPlugin in server.go (check error, log, set
inactive) so plugin status reflects reload outcome.
🧹 Nitpick comments (7)
core/internal/mcptests/agent_filtering_test.go (1)
534-534: Preferbifrost.Ptr(...)over&valuefor config pointers.For consistency with repo conventions, consider wrapping
tempConfigviabifrost.Ptr(...)(add the import if not already in scope).Based on learnings, prefer `bifrost.Ptr(...)` for pointer creation.♻️ Example
- err = manager.AddClient(&tempConfig) + err = manager.AddClient(bifrost.Ptr(tempConfig))Also applies to: 623-623
core/internal/mcptests/client_management_test.go (1)
28-28: Preferbifrost.Ptr(...)for MCPClientConfig pointers.To align with project conventions, consider switching these
&configusages tobifrost.Ptr(config)(add the import if needed).Based on learnings, prefer `bifrost.Ptr(...)` for pointer creation.♻️ Example
- err := manager.AddClient(&clientConfig) + err := manager.AddClient(bifrost.Ptr(clientConfig))Also applies to: 32-32, 186-186, 202-202, 228-228, 260-260, 431-431
core/internal/mcptests/health_monitoring_test.go (1)
53-53: Preferbifrost.Ptr(...)for MCPClientConfig pointers.If consistent with the rest of the test suite, replace
&clientConfigwithbifrost.Ptr(clientConfig)(and add the import if required).Based on learnings, prefer `bifrost.Ptr(...)` for pointer creation.♻️ Example
- err = manager.AddClient(&clientConfig) + err = manager.AddClient(bifrost.Ptr(clientConfig))Also applies to: 184-184, 395-395
core/internal/mcptests/concurrency_advanced_test.go (1)
166-166: Preferbifrost.Ptr(...)for MCPClientConfig pointers.For consistency with repo standards, consider
bifrost.Ptr(clientConfig)instead of&clientConfig(add import if needed).Based on learnings, prefer `bifrost.Ptr(...)` for pointer creation.♻️ Example
- err := manager.AddClient(&clientConfig) + err := manager.AddClient(bifrost.Ptr(clientConfig))core/internal/mcptests/concurrency_test.go (1)
440-440: Preferbifrost.Ptr(...)for MCPClientConfig pointers.To keep pointer creation consistent, consider
bifrost.Ptr(updatedConfig)instead of&updatedConfig(add import if required).Based on learnings, prefer `bifrost.Ptr(...)` for pointer creation.♻️ Example
- err := manager.EditClient(clientConfig.ID, &updatedConfig) + err := manager.EditClient(clientConfig.ID, bifrost.Ptr(updatedConfig))framework/configstore/rdb.go (1)
766-783: Prefer bifrost.Ptr for MCPClientConfig pointers.
Repo convention uses bifrost.Ptr over &value for pointer creation; apply in both loops for consistency.Based on learnings, prefer bifrost.Ptr(...) for pointer creation.♻️ Suggested refactor
- clientConfigs[i] = &schemas.MCPClientConfig{ + clientConfigs[i] = bifrost.Ptr(schemas.MCPClientConfig{ ID: dbClient.ClientID, Name: dbClient.Name, IsCodeModeClient: dbClient.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(dbClient.ConnectionType), ConnectionString: dbClient.ConnectionString, StdioConfig: dbClient.StdioConfig, AuthType: schemas.MCPAuthType(dbClient.AuthType), OauthConfigID: dbClient.OauthConfigID, ToolsToExecute: dbClient.ToolsToExecute, ToolsToAutoExecute: dbClient.ToolsToAutoExecute, Headers: dbClient.Headers, IsPingAvailable: dbClient.IsPingAvailable, ToolSyncInterval: time.Duration(dbClient.ToolSyncInterval) * time.Minute, ToolPricing: dbClient.ToolPricing, - } + })- clientConfigs[i] = &schemas.MCPClientConfig{ + clientConfigs[i] = bifrost.Ptr(schemas.MCPClientConfig{ ID: dbClient.ClientID, Name: dbClient.Name, IsCodeModeClient: dbClient.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(dbClient.ConnectionType), ConnectionString: dbClient.ConnectionString, StdioConfig: dbClient.StdioConfig, AuthType: schemas.MCPAuthType(dbClient.AuthType), OauthConfigID: dbClient.OauthConfigID, ToolsToExecute: dbClient.ToolsToExecute, ToolsToAutoExecute: dbClient.ToolsToAutoExecute, Headers: dbClient.Headers, IsPingAvailable: dbClient.IsPingAvailable, ToolSyncInterval: time.Duration(dbClient.ToolSyncInterval) * time.Minute, ToolPricing: dbClient.ToolPricing, - } + })Also applies to: 800-817
transports/bifrost-http/server/server.go (1)
644-653: Minor:updatePluginErrorStatusshadows outererrparameter.The function accepts an
errparameter but also declares a newerrvariable on line 646. While this works, it could be confusing. Consider using a different variable name for the inner error to improve clarity.♻️ Suggested improvement
func (s *BifrostHTTPServer) updatePluginErrorStatus(name, step string, err error) error { - if err := s.Config.UpdatePluginStatus(name, schemas.PluginStatusError); err != nil { + if statusErr := s.Config.UpdatePluginStatus(name, schemas.PluginStatusError); statusErr != nil { - return err + return statusErr } - if err := s.Config.AppendPluginStateLogs(name, []string{fmt.Sprintf("error %s plugin %s: %v", step, name, err)}); err != nil { + if logErr := s.Config.AppendPluginStateLogs(name, []string{fmt.Sprintf("error %s plugin %s: %v", step, name, err)}); logErr != nil { - return err + return logErr } return err }
| s.Config.ReloadPlugin(plugin) | ||
| s.Config.UpdatePluginOverallStatus(name, name, schemas.PluginStatusActive, | ||
| []string{fmt.Sprintf("%s plugin initialized successfully", name)}, getPluginTypes(plugin)) | ||
| []string{fmt.Sprintf("%s plugin initialized successfully", name)}, InferPluginTypes(plugin)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ReloadPlugin function definition
echo "=== Finding ReloadPlugin definition ==="
rg -n 'func.*ReloadPlugin' -A 5
echo -e "\n=== Checking ReloadPlugin calls and error handling ==="
rg -n 'ReloadPlugin' -B 2 -A 2
echo -e "\n=== Looking for InstantiatePlugin error handling pattern ==="
rg -n 'InstantiatePlugin' -B 2 -A 5Repository: maximhq/bifrost
Length of output: 13498
Handle ReloadPlugin error return value before marking plugin active.
The Config.ReloadPlugin method returns an error (lib/config.go:2412), but the call at line 96 ignores it and unconditionally marks the plugin as active. This hides failures. Use the same error-handling pattern applied elsewhere in the codebase (server.go:658):
Suggested fix
- s.Config.ReloadPlugin(plugin)
- s.Config.UpdatePluginOverallStatus(name, name, schemas.PluginStatusActive,
- []string{fmt.Sprintf("%s plugin initialized successfully", name)}, InferPluginTypes(plugin))
+ if err := s.Config.ReloadPlugin(plugin); err != nil {
+ logger.Error("failed to reload %s plugin: %v", name, err)
+ s.Config.UpdatePluginOverallStatus(name, name, schemas.PluginStatusError,
+ []string{fmt.Sprintf("error reloading %s plugin: %v", name, err)}, InferPluginTypes(plugin))
+ if failOnError {
+ return err
+ }
+ return nil
+ }
+ s.Config.UpdatePluginOverallStatus(name, name, schemas.PluginStatusActive,
+ []string{fmt.Sprintf("%s plugin initialized successfully", name)}, InferPluginTypes(plugin))Note: The same pattern is missing at plugins.go:217.
🤖 Prompt for AI Agents
In `@transports/bifrost-http/server/utils.go` around lines 96 - 98, The call to
s.Config.ReloadPlugin currently ignores its error so the code always calls
s.Config.UpdatePluginOverallStatus(name, name, schemas.PluginStatusActive,...),
which hides reload failures; change the sequence in the function containing
s.Config.ReloadPlugin to capture the returned error, and if err != nil log the
error and call s.Config.UpdatePluginOverallStatus(name, name,
schemas.PluginStatusInactive, []string{fmt.Sprintf("%s plugin reload failed:
%v", name, err)}, nil) (or appropriate failure messages), otherwise proceed to
mark active and call InferPluginTypes(plugin) as currently done; follow the same
error-handling pattern used around Config.ReloadPlugin in server.go (check
error, log, set inactive) so plugin status reflects reload outcome.
31da0d6 to
80d48cb
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 (6)
core/mcp/mcp.go (1)
118-129: Guard against nil client configs to avoid panic.
With[]*MCPClientConfig, a nil entry will panic onclientConfig.Nameor insideAddClient. Add a nil check and movewg.Addinside the loop.🐛 Suggested fix
if len(config.ClientConfigs) > 0 { // Add clients in parallel - wg := sync.WaitGroup{} - wg.Add(len(config.ClientConfigs)) + wg := sync.WaitGroup{} for _, clientConfig := range config.ClientConfigs { + if clientConfig == nil { + logger.Warn("%s Skipping nil MCP client config", MCPLogPrefix) + continue + } + wg.Add(1) go func(clientConfig *schemas.MCPClientConfig) { defer wg.Done() if err := manager.AddClient(clientConfig); err != nil { logger.Warn("%s Failed to add MCP client %s: %v", MCPLogPrefix, clientConfig.Name, err) } }(clientConfig) } wg.Wait() }core/mcp/toolsync.go (1)
225-233: Add a nil guard for the pointer-based config.
Now that this accepts*MCPClientConfig, a nil input will panic. Consider a safe fallback to global/default.🛡️ Suggested update
func ResolveToolSyncInterval(clientConfig *schemas.MCPClientConfig, globalInterval time.Duration) time.Duration { + if clientConfig == nil { + if globalInterval > 0 { + return globalInterval + } + return DefaultToolSyncInterval + } // Per-client explicitly disabled (negative value) if clientConfig.ToolSyncInterval < 0 { return 0 // Disabled for this client }core/bifrost.go (1)
2746-2775: Guard against nil MCP client config.
configcan be nil now that the API is pointer-based; add a fast-fail to prevent downstream panics.🐛 Suggested nil guard
func (bifrost *Bifrost) AddMCPClient(config *schemas.MCPClientConfig) error { + if config == nil { + return fmt.Errorf("mcp client config is required") + } if bifrost.mcpManager == nil { // Use sync.Once to ensure thread-safe initialization bifrost.mcpInitOnce.Do(func() {framework/configstore/rdb.go (2)
795-822: Global MCP tool sync interval is dropped.
GetMCPConfigbuilds the schema config but never mapsMCPToolSyncIntervalintoMCPConfig.ToolSyncInterval, so custom global sync settings are ignored after reloads.🐛 Suggested fix
return &schemas.MCPConfig{ ClientConfigs: clientConfigs, - ToolManagerConfig: &toolManagerConfig, + ToolManagerConfig: &toolManagerConfig, + ToolSyncInterval: time.Duration(clientConfig.MCPToolSyncInterval) * time.Minute, }, nil
859-873: ToolPricing isn’t persisted on MCP client creation.
CreateMCPClientConfigomitsToolPricing, so per-tool pricing from config/API will be lost after restart and MCPCatalog won’t reflect it.🐛 Suggested fix
dbClient := tables.TableMCPClient{ ClientID: clientConfigCopy.ID, Name: clientConfigCopy.Name, IsCodeModeClient: clientConfigCopy.IsCodeModeClient, ConnectionType: string(clientConfigCopy.ConnectionType), ConnectionString: clientConfigCopy.ConnectionString, StdioConfig: clientConfigCopy.StdioConfig, AuthType: string(clientConfigCopy.AuthType), OauthConfigID: clientConfigCopy.OauthConfigID, ToolsToExecute: clientConfigCopy.ToolsToExecute, ToolsToAutoExecute: clientConfigCopy.ToolsToAutoExecute, Headers: clientConfigCopy.Headers, IsPingAvailable: clientConfigCopy.IsPingAvailable, ToolSyncInterval: int(clientConfigCopy.ToolSyncInterval.Minutes()), + ToolPricing: clientConfigCopy.ToolPricing, }transports/bifrost-http/lib/config.go (1)
1483-1513: Conversion helpers drop ToolPricing/ToolSyncInterval/IsPingAvailable.Both conversions omit fields now present on
schemas.MCPClientConfig, so pricing, sync interval, and ping support can be silently lost when persisting or hydrating configs.🐛 Suggested fix
func convertSchemasMCPClientConfigToTable(clientConfig *schemas.MCPClientConfig) *configstoreTables.TableMCPClient { return &configstoreTables.TableMCPClient{ ClientID: clientConfig.ID, Name: clientConfig.Name, IsCodeModeClient: clientConfig.IsCodeModeClient, ConnectionType: string(clientConfig.ConnectionType), ConnectionString: clientConfig.ConnectionString, StdioConfig: clientConfig.StdioConfig, ToolsToExecute: clientConfig.ToolsToExecute, ToolsToAutoExecute: clientConfig.ToolsToAutoExecute, Headers: clientConfig.Headers, AuthType: string(clientConfig.AuthType), OauthConfigID: clientConfig.OauthConfigID, + IsPingAvailable: clientConfig.IsPingAvailable, + ToolSyncInterval: int(clientConfig.ToolSyncInterval.Minutes()), + ToolPricing: clientConfig.ToolPricing, } } func convertTablesMCPClientToSchemas(mcpClient configstoreTables.TableMCPClient) schemas.MCPClientConfig { return schemas.MCPClientConfig{ ID: mcpClient.ClientID, Name: mcpClient.Name, IsCodeModeClient: mcpClient.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(mcpClient.ConnectionType), ConnectionString: mcpClient.ConnectionString, StdioConfig: mcpClient.StdioConfig, ToolsToExecute: mcpClient.ToolsToExecute, ToolsToAutoExecute: mcpClient.ToolsToAutoExecute, Headers: mcpClient.Headers, AuthType: schemas.MCPAuthType(mcpClient.AuthType), OauthConfigID: mcpClient.OauthConfigID, + IsPingAvailable: mcpClient.IsPingAvailable, + ToolSyncInterval: time.Duration(mcpClient.ToolSyncInterval) * time.Minute, + ToolPricing: mcpClient.ToolPricing, } }
🧹 Nitpick comments (9)
core/internal/mcptests/agent_filtering_test.go (1)
534-534: Preferbifrost.Ptrfor MCPClientConfig pointers.
Use the repo’s pointer helper instead of&for consistency.♻️ Suggested update
import ( "testing" "time" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ - err = manager.AddClient(&tempConfig) + err = manager.AddClient(bifrost.Ptr(tempConfig)) @@ - err = manager.AddClient(&tempConfig) + err = manager.AddClient(bifrost.Ptr(tempConfig))Based on learnings, use
bifrost.Ptr()instead of&for pointers.Also applies to: 623-623
core/internal/mcptests/agent_request_id_test.go (1)
26-35: Usebifrost.Ptrwhen building pointer slices.
This keeps pointer creation consistent with repo conventions.♻️ Suggested update
import ( "context" "fmt" "sync" "testing" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/mcp" "github.com/maximhq/bifrost/core/mcp/codemode/starlark" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ clientConfigPtrs := make([]*schemas.MCPClientConfig, len(clientConfigs)) for i := range clientConfigs { - clientConfigPtrs[i] = &clientConfigs[i] + clientConfig := clientConfigs[i] + clientConfigPtrs[i] = bifrost.Ptr(clientConfig) }Based on learnings, use
bifrost.Ptr()instead of&for pointers.core/internal/mcptests/fixtures.go (1)
1469-1473: Preferbifrost.Ptrwhen creating config pointers.♻️ Suggested update
clientConfigPtrs := make([]*schemas.MCPClientConfig, len(clientConfigs)) for i := range clientConfigs { - clientConfigPtrs[i] = &clientConfigs[i] + clientConfig := clientConfigs[i] + clientConfigPtrs[i] = bifrost.Ptr(clientConfig) }Based on learnings, use
bifrost.Ptr()instead of&for pointers.core/internal/mcptests/concurrency_advanced_test.go (1)
158-166: Usebifrost.Ptrfor consistency with pointer conventions.♻️ Suggested update
import ( "context" "encoding/json" "fmt" "strings" "sync" "sync/atomic" "testing" "time" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/mcp" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ - err := manager.AddClient(&clientConfig) + err := manager.AddClient(bifrost.Ptr(clientConfig))Based on learnings, use
bifrost.Ptr()instead of&for pointers.transports/bifrost-http/lib/config_test.go (2)
475-485: Keep MockConfigStore MCP mapping in sync with new MCP fields.The mock updates only a subset of
schemas.MCPClientConfigfields. With the pointer refactor and newly added fields (e.g., AuthType, OAuthConfigID, IsPingAvailable, ToolSyncInterval, ToolPricing, State), the mock can fall out of parity and mask regressions. Consider copying all available fields fromtables.TableMCPClientwhen building the schema config.Also applies to: 490-499
8987-8987: Preferbifrost.Ptr(...)for MCPClientConfig pointers in tests.Repo convention favors
bifrost.Ptrover&for pointer creation; applying it here keeps tests consistent with the rest of the codebase. Based on learnings, ...♻️ Example adjustment
-err = config1.ConfigStore.CreateMCPClientConfig(ctx, &mcpClientConfig) +err = config1.ConfigStore.CreateMCPClientConfig(ctx, bifrost.Ptr(mcpClientConfig))Also applies to: 9076-9077, 9086-9087, 9398-9399, 9408-9409, 9563-9564, 9685-9685, 9779-9779
core/bifrost.go (1)
2810-2832: Align UpdateMCPClient naming and example with the actual method.The comment/example use
UpdateMCPClient, but the method is stillEditMCPClientand now expects a pointer. Consider renaming the method or updating the docs/example to match the real signature.transports/bifrost-http/handlers/mcp.go (1)
289-301: Usebifrost.Ptrfor MCP client config pointers.This keeps pointer creation consistent with repo conventions.
♻️ Suggested refactor
- schemasConfig := &schemas.MCPClientConfig{ + schemasConfig := bifrost.Ptr(schemas.MCPClientConfig{ ID: req.ClientID, Name: req.Name, IsCodeModeClient: req.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(req.ConnectionType), ConnectionString: req.ConnectionString, StdioConfig: req.StdioConfig, AuthType: schemas.MCPAuthType(req.AuthType), OauthConfigID: nil, ToolsToExecute: req.ToolsToExecute, ToolsToAutoExecute: req.ToolsToAutoExecute, Headers: req.Headers, - } + }) @@ - schemasConfig := &schemas.MCPClientConfig{ + schemasConfig := bifrost.Ptr(schemas.MCPClientConfig{ ID: req.ClientID, Name: req.Name, IsCodeModeClient: req.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(req.ConnectionType), ConnectionString: req.ConnectionString, StdioConfig: req.StdioConfig, ToolsToExecute: req.ToolsToExecute, ToolsToAutoExecute: req.ToolsToAutoExecute, Headers: req.Headers, AuthType: schemas.MCPAuthType(req.AuthType), OauthConfigID: req.OauthConfigID, IsPingAvailable: req.IsPingAvailable, ToolSyncInterval: toolSyncInterval, ToolPricing: req.ToolPricing, - } + })Based on learnings, keep pointer creation consistent with
bifrost.Ptr.Also applies to: 386-405
transports/bifrost-http/server/server.go (1)
644-653: Variable shadowing reduces readability.The
errparameter is shadowed by:=assignments in the if-statements. While technically correct (thefmt.Sprintfuses the parameter, and line 652 returns the original error), this is confusing to read.♻️ Suggested improvement for clarity
func (s *BifrostHTTPServer) updatePluginErrorStatus(name, step string, err error) error { - if err := s.Config.UpdatePluginStatus(name, schemas.PluginStatusError); err != nil { - return err + if statusErr := s.Config.UpdatePluginStatus(name, schemas.PluginStatusError); statusErr != nil { + return statusErr } - if err := s.Config.AppendPluginStateLogs(name, []string{fmt.Sprintf("error %s plugin %s: %v", step, name, err)}); err != nil { - return err + if logErr := s.Config.AppendPluginStateLogs(name, []string{fmt.Sprintf("error %s plugin %s: %v", step, name, err)}); logErr != nil { + return logErr } return err }
akshaydeo
force-pushed
the
01-27-mcp_flow_refactoring
branch
from
80d48cb to
e85d83e
Compare
akshaydeo
force-pushed
the
01-26-code_mode_compact_signatures
branch
from
1f01461 to
7318932
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 (5)
framework/configstore/rdb.go (1)
851-878: Add missing ToolPricing field and nil checkThe
dbClientstruct initialization omitsToolPricing, causing per-client pricing to be lost on create. Additionally, line 855 dereferencesclientConfigwithout checking for nil, risking a panic.✅ Proposed fix
func (s *RDBConfigStore) CreateMCPClientConfig(ctx context.Context, clientConfig *schemas.MCPClientConfig) error { + if clientConfig == nil { + return errors.New("clientConfig is nil") + } return s.db.Transaction(func(tx *gorm.DB) error { // Create a deep copy to avoid modifying the original clientConfigCopy, err := deepCopy(*clientConfig) @@ dbClient := tables.TableMCPClient{ ClientID: clientConfigCopy.ID, Name: clientConfigCopy.Name, @@ IsPingAvailable: clientConfigCopy.IsPingAvailable, ToolSyncInterval: int(clientConfigCopy.ToolSyncInterval.Minutes()), + ToolPricing: clientConfigCopy.ToolPricing, }core/schemas/mcp.go (1)
77-85: Implement backward compatibility for JSON field renameid→client_id.The struct field was renamed from
json:"id"tojson:"client_id", breaking existing config files and API payloads that send"id". Without a compatibility layer, the ID will unmarshal as empty. Add a custom UnmarshalJSON method to accept both field names, or ensure all callers are updated and document the breaking change.🔧 Custom UnmarshalJSON implementation
// UnmarshalJSON supports legacy "id" field for backward compatibility. func (c *MCPClientConfig) UnmarshalJSON(b []byte) error { type Alias MCPClientConfig var aux struct { *Alias LegacyID string `json:"id"` } aux.Alias = (*Alias)(c) if err := json.Unmarshal(b, &aux); err != nil { return err } if c.ID == "" && aux.LegacyID != "" { c.ID = aux.LegacyID } return nil }core/bifrost.go (2)
2739-2746: Update AddMCPClient example to pass a pointer.
Signature now expects *schemas.MCPClientConfig, but the doc example still shows a value.Based on learnings, prefer Ptr helper for pointer creation in examples.🛠️ Suggested doc update
-// err := bifrost.AddMCPClient(schemas.MCPClientConfig{ +// err := bifrost.AddMCPClient(bifrost.Ptr(schemas.MCPClientConfig{ // Name: "my-mcp-client", // ConnectionType: schemas.MCPConnectionTypeHTTP, -// ConnectionString: &url, -// }) +// ConnectionString: bifrost.Ptr(url), +// }))
2810-2832: Rename EditMCPClient or fix the UpdateMCPClient docs.
The comment/example now reference UpdateMCPClient, but the method is still EditMCPClient.🛠️ Suggested fix (rename method)
-func (bifrost *Bifrost) EditMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { +func (bifrost *Bifrost) UpdateMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error {core/mcp/clientmanager.go (1)
237-248: Guard against nil updatedConfig.
Pointer signature can panic at updatedConfig.Name; add an early check for a clearer error.🛠️ Suggested fix
func (m *MCPManager) EditClient(id string, updatedConfig *schemas.MCPClientConfig) error { + if updatedConfig == nil { + return fmt.Errorf("updatedConfig is required") + } m.mu.Lock() defer m.mu.Unlock()
🧹 Nitpick comments (6)
core/internal/mcptests/health_monitoring_test.go (1)
53-54: Usebifrost.Ptrfor MCPClientConfig pointers (consistency).The repo prefers
bifrost.Ptrover&for pointer creation; it also avoids accidental aliasing. Apply this to all threeAddClientcalls (add thebifrostimport if needed). Based on learnings, preferbifrost.Ptrfor pointer creation.♻️ Example update
- err = manager.AddClient(&clientConfig) + err = manager.AddClient(bifrost.Ptr(clientConfig))Also applies to: 184-185, 395-396
core/internal/mcptests/error_handling_protocol_test.go (1)
292-293: Usebifrost.Ptrfor MCPClientConfig pointers here as well.For consistency with the rest of the repo (and to avoid aliasing), use
bifrost.Ptrinstead of&in theseAddClientcalls (add thebifrostimport if needed). Based on learnings, preferbifrost.Ptrfor pointer creation.♻️ Example update
- err := manager.AddClient(&errorServerConfig) + err := manager.AddClient(bifrost.Ptr(errorServerConfig))Also applies to: 370-371, 423-424, 472-473
core/internal/mcptests/concurrency_test.go (1)
438-441: Preferbifrost.Ptr(updatedConfig)for pointer creation.Even with the copy in place, using the helper keeps pointer creation consistent across the repo (and avoids accidental aliasing). Add the
bifrostimport if needed. Based on learnings, preferbifrost.Ptrfor pointer creation.♻️ Suggested update
- err := manager.EditClient(clientConfig.ID, &updatedConfig) + err := manager.EditClient(clientConfig.ID, bifrost.Ptr(updatedConfig))core/internal/mcptests/client_management_test.go (1)
27-33: Preferbifrost.Ptr(...)for MCPClientConfig pointers in tests.Repo convention prefers
bifrost.Ptr(value)over&valuefor pointer creation. Apply this pattern here and across other pointer-based call sites in this file for consistency.
Based on learnings, please align pointer creation accordingly.♻️ Proposed update
@@ -import ( - "testing" - "time" - - "github.com/maximhq/bifrost/core/schemas" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) +import ( + "testing" + "time" + + bifrost "github.com/maximhq/bifrost/core" + "github.com/maximhq/bifrost/core/schemas" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) @@ -err := manager.AddClient(&clientConfig) +err := manager.AddClient(bifrost.Ptr(clientConfig)) @@ -err = manager.AddClient(&clientConfig) +err = manager.AddClient(bifrost.Ptr(clientConfig))core/mcp/clientmanager.go (2)
75-113:configCopyisn’t a copy; consider renaming or cloning.
The comment implies a copy, but it’s just another pointer; if callers mutate config after unlocking, state can change unexpectedly.🛠️ Suggested clarification (rename + comment)
- // Make a copy of the config to use after unlocking - configCopy := config + // Keep a pointer for use after unlocking (config is treated as immutable) + configPtr := config @@ - if err := m.connectToMCPClient(configCopy); err != nil { + if err := m.connectToMCPClient(configPtr); err != nil {- // Make a copy of the config to use after unlocking - configCopy := config + // Keep a pointer for use after unlocking (config is treated as immutable) + configPtr := config @@ - if err := m.connectToMCPClient(configCopy); err != nil { + if err := m.connectToMCPClient(configPtr); err != nil {Also applies to: 125-163
688-691: Prefer Ptr helper for StdioCommandString.
Use the repo’s pointer helper for consistency instead of&cmdString.Based on learnings, prefer Ptr helper for pointer creation.🛠️ Suggested change
- StdioCommandString: &cmdString, + StdioCommandString: schemas.Ptr(cmdString),
akshaydeo
force-pushed
the
01-27-mcp_flow_refactoring
branch
from
e85d83e to
31b7987
Compare
akshaydeo
force-pushed
the
01-27-mcp_flow_refactoring
branch
from
31b7987 to
12425fa
Compare
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 (8)
core/mcp/toolsync.go (1)
225-242: Add nil check forclientConfigparameter to prevent potential panic.The function now accepts a pointer but doesn't guard against nil. If a caller passes nil, accessing
clientConfig.ToolSyncIntervalat line 227 will cause a nil pointer dereference panic. The related functionshouldSkipToolForConfigin utils.go has a nil check (lines 206-208).Suggested fix
func ResolveToolSyncInterval(clientConfig *schemas.MCPClientConfig, globalInterval time.Duration) time.Duration { + // If no client config, use global or default interval + if clientConfig == nil { + if globalInterval > 0 { + return globalInterval + } + return DefaultToolSyncInterval + } + // Per-client explicitly disabled (negative value) if clientConfig.ToolSyncInterval < 0 { return 0 // Disabled for this client }core/bifrost.go (2)
2739-2753: Update the AddMCPClient example to pass a pointer.The signature now expects
*schemas.MCPClientConfig, but the example still passes a value.🛠️ Suggested doc fix
-// err := bifrost.AddMCPClient(schemas.MCPClientConfig{ +// err := bifrost.AddMCPClient(&schemas.MCPClientConfig{ // Name: "my-mcp-client", // ConnectionType: schemas.MCPConnectionTypeHTTP, // ConnectionString: &url, // })
2810-2831: Rename the method to UpdateMCPClient to match the new API name.The comment/example say
UpdateMCPClient, but the exported symbol is stillEditMCPClient. If the stack call sites were updated, this will break compilation and the public API contract.🛠️ Proposed fix
-func (bifrost *Bifrost) EditMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { +func (bifrost *Bifrost) UpdateMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { if bifrost.mcpManager == nil { return fmt.Errorf("MCP is not configured in this Bifrost instance") } return bifrost.mcpManager.EditClient(id, updatedConfig) }Given the stacked PRs, please ensure the rename is aligned across the stack before merge.
framework/configstore/rdb.go (1)
851-873: Persist ToolPricing on create.
CreateMCPClientConfigdoesn’t copyToolPricingintoTableMCPClient, so pricing gets dropped on first persist (the update path does persist it).🔧 Proposed fix
dbClient := tables.TableMCPClient{ ClientID: clientConfigCopy.ID, Name: clientConfigCopy.Name, IsCodeModeClient: clientConfigCopy.IsCodeModeClient, ConnectionType: string(clientConfigCopy.ConnectionType), ConnectionString: clientConfigCopy.ConnectionString, StdioConfig: clientConfigCopy.StdioConfig, AuthType: string(clientConfigCopy.AuthType), OauthConfigID: clientConfigCopy.OauthConfigID, ToolsToExecute: clientConfigCopy.ToolsToExecute, ToolsToAutoExecute: clientConfigCopy.ToolsToAutoExecute, Headers: clientConfigCopy.Headers, IsPingAvailable: clientConfigCopy.IsPingAvailable, ToolSyncInterval: int(clientConfigCopy.ToolSyncInterval.Minutes()), + ToolPricing: clientConfigCopy.ToolPricing, }core/mcp/clientmanager.go (1)
237-275: EditClient should propagate ToolPricing updates.
UpdateMCPClientupdates pricing, butEditClientnever copiesToolPricinginto the liveExecutionConfig, leaving MCP manager state stale (e.g.,GetClients()output).🔧 Proposed fix
config := client.ExecutionConfig config.Name = updatedConfig.Name config.Headers = updatedConfig.Headers config.ToolsToExecute = updatedConfig.ToolsToExecute config.ToolsToAutoExecute = updatedConfig.ToolsToAutoExecute config.IsCodeModeClient = updatedConfig.IsCodeModeClient +config.ToolPricing = updatedConfig.ToolPricingtransports/bifrost-http/lib/config.go (3)
857-869: Guard against nil client entries in pointer slices.
With[]*schemas.MCPClientConfig, JSONnullentries can appear and will panic on deref in merge or lookup.🔧 Proposed fix
for _, newClientConfig := range tempMCPConfig.ClientConfigs { + if newClientConfig == nil { + continue + } found := false for _, existingClientConfig := range mcpConfig.ClientConfigs { if (newClientConfig.ID != "" && existingClientConfig.ID == newClientConfig.ID) || (newClientConfig.Name != "" && existingClientConfig.Name == newClientConfig.Name) { @@ for _, clientConfig := range c.MCPConfig.ClientConfigs { - if clientConfig.ID == id { + if clientConfig == nil { + continue + } + if clientConfig.ID == id { return clientConfig, nil } }Also applies to: 2774-2776
1489-1503: Schema↔table conversion drops ToolPricing/ToolSyncInterval.
convertSchemasMCPClientConfigToTableomitsToolPricing,ToolSyncInterval, andIsPingAvailable, so persistence loses these fields. Consider mirroring them both ways.🔧 Proposed fix
func convertSchemasMCPClientConfigToTable(clientConfig *schemas.MCPClientConfig) *configstoreTables.TableMCPClient { return &configstoreTables.TableMCPClient{ ClientID: clientConfig.ID, Name: clientConfig.Name, IsCodeModeClient: clientConfig.IsCodeModeClient, ConnectionType: string(clientConfig.ConnectionType), ConnectionString: clientConfig.ConnectionString, StdioConfig: clientConfig.StdioConfig, ToolsToExecute: clientConfig.ToolsToExecute, ToolsToAutoExecute: clientConfig.ToolsToAutoExecute, Headers: clientConfig.Headers, AuthType: string(clientConfig.AuthType), OauthConfigID: clientConfig.OauthConfigID, + IsPingAvailable: clientConfig.IsPingAvailable, + ToolSyncInterval: int(clientConfig.ToolSyncInterval.Minutes()), + ToolPricing: clientConfig.ToolPricing, } } func convertTablesMCPClientToSchemas(mcpClient configstoreTables.TableMCPClient) schemas.MCPClientConfig { return schemas.MCPClientConfig{ ID: mcpClient.ClientID, Name: mcpClient.Name, IsCodeModeClient: mcpClient.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(mcpClient.ConnectionType), ConnectionString: mcpClient.ConnectionString, StdioConfig: mcpClient.StdioConfig, ToolsToExecute: mcpClient.ToolsToExecute, ToolsToAutoExecute: mcpClient.ToolsToAutoExecute, Headers: mcpClient.Headers, AuthType: schemas.MCPAuthType(mcpClient.AuthType), OauthConfigID: mcpClient.OauthConfigID, + IsPingAvailable: mcpClient.IsPingAvailable, + ToolSyncInterval: time.Duration(mcpClient.ToolSyncInterval) * time.Minute, + ToolPricing: mcpClient.ToolPricing, } }
2967-2988: Handle nil input in RedactMCPClientConfig.
Now that the function accepts a pointer, a nil input will panic.🔧 Proposed fix
func (c *Config) RedactMCPClientConfig(config *schemas.MCPClientConfig) *schemas.MCPClientConfig { + if config == nil { + return nil + } // Create an actual copy of the struct (not just a pointer copy) // This prevents modifying the original config when redacting configCopy := *config
🤖 Fix all issues with AI agents
In `@core/mcp/healthmonitor.go`:
- Around line 3-6: Read ExecutionConfig.Name while holding manager.mu and store
it in a local displayName variable before unlocking to avoid races and
nil-panics; update all locations that read ExecutionConfig.Name (e.g., in the
health monitor functions that currently read ExecutionConfig.Name after
releasing manager.mu) to capture displayName under the lock and then perform
logging/printing after unlocking. In other words, inside the critical sections
guarded by manager.mu (the same lock used by UpdateMCPClient), check for nil
ExecutionConfig, set displayName := manager.ExecutionConfig.Name (or a safe
default if nil), then release the lock and use displayName for subsequent logs;
do this for the other affected blocks (around lines noted: 68-86, 97-123,
191-209) to eliminate races and potential panics.
In `@transports/bifrost-http/server/plugins.go`:
- Around line 217-219: The call to s.Config.ReloadPlugin(plugin) currently
ignores its error and immediately calls s.Config.UpdatePluginOverallStatus to
mark the plugin active; change this to capture the error returned by
ReloadPlugin and follow the same pattern as registerPluginWithStatus in utils.go
(check for non-nil error, log/update status to PluginStatusFailed with the error
message and do not mark active on failure), only calling
UpdatePluginOverallStatus(..., PluginStatusActive, ...) when ReloadPlugin
returns nil; make sure to reference the plugin name via
plugin.GetName()/cfg.Name in the status update and include the error text in the
failure status messages.
🧹 Nitpick comments (7)
core/internal/mcptests/integration_test.go (1)
37-37: Preferbifrost.Ptrfor AddClient config pointersLine 37 and Line 344 use
&httpConfig; repo convention prefersbifrost.Ptr(...)for pointer creation to keep tests consistent with the MCP pointer refactor. Add thebifrostimport if it’s not already present.♻️ Suggested change
- err := manager.AddClient(&httpConfig) + err := manager.AddClient(bifrost.Ptr(httpConfig))- err := manager.AddClient(&httpConfig) + err := manager.AddClient(bifrost.Ptr(httpConfig))Based on learnings, prefer
bifrost.Ptrfor pointer creation.Also applies to: 344-344
core/internal/mcptests/agent_filtering_test.go (1)
534-534: Preferbifrost.PtrfortempConfigpointers inAddClientLine 534 and Line 623 use
&tempConfig; repo convention prefersbifrost.Ptr(...)for pointer creation. Add thebifrostimport if it’s not already present.♻️ Suggested change
- err = manager.AddClient(&tempConfig) + err = manager.AddClient(bifrost.Ptr(tempConfig))- err = manager.AddClient(&tempConfig) + err = manager.AddClient(bifrost.Ptr(tempConfig))Based on learnings, prefer
bifrost.Ptrfor pointer creation.Also applies to: 623-623
core/internal/mcptests/agent_request_id_test.go (1)
26-30: Usebifrost.Ptrwhen constructingClientConfigpointersLine 26–30 builds the pointer slice via
&clientConfigs[i]. Repo convention prefersbifrost.Ptr(...)for pointer creation. Add thebifrostimport if it’s not already present.♻️ Suggested change
- clientConfigPtrs[i] = &clientConfigs[i] + clientConfigPtrs[i] = bifrost.Ptr(clientConfigs[i])Based on learnings, prefer
bifrost.Ptrfor pointer creation.Also applies to: 34-34
transports/bifrost-http/handlers/oauth2.go (1)
226-230: Align pending MCP client storage with pointer-based config refactor.StorePendingMCPClient still takes a value type, which diverges from the pointer-based MCPClientConfig refactor and forces callers to deref or copy. Consider aligning this to *schemas.MCPClientConfig for consistency and to avoid avoidable struct copies.
♻️ Suggested change
-func (h *OAuthHandler) StorePendingMCPClient(oauthConfigID string, mcpClientConfig schemas.MCPClientConfig) error { - return h.oauthProvider.StorePendingMCPClient(oauthConfigID, mcpClientConfig) +func (h *OAuthHandler) StorePendingMCPClient(oauthConfigID string, mcpClientConfig *schemas.MCPClientConfig) error { + return h.oauthProvider.StorePendingMCPClient(oauthConfigID, mcpClientConfig) }framework/oauth2/main.go (1)
215-221: Prefer bifrost.Ptr(...) over &value for pointer creation.This repo consistently uses
bifrost.Ptr()for pointer creation; update the new pointer assignments/returns to match the convention.♻️ Suggested changes
- configStr := string(configJSON) - oauthConfig.MCPClientConfigJSON = &configStr + oauthConfig.MCPClientConfigJSON = bifrost.Ptr(string(configJSON))- return &config, nil + return bifrost.Ptr(config), nilBased on learnings, please keep pointer creation consistent across new code paths.
Also applies to: 252-257, 282-287
core/mcp/clientmanager.go (1)
688-691: Preferbifrost.Ptrfor pointer construction to match repo conventions.
This keeps pointer creation consistent across the codebase (e.g.,cmdString,ExecutionConfig,ConnectionInfo).♻️ Suggested refactor
import ( "context" "fmt" "maps" "os" "strings" "github.com/mark3labs/mcp-go/client" "github.com/mark3labs/mcp-go/client/transport" "github.com/mark3labs/mcp-go/mcp" "github.com/mark3labs/mcp-go/server" + bifrost "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" ) @@ connectionInfo := &schemas.MCPClientConnectionInfo{ Type: config.ConnectionType, - StdioCommandString: &cmdString, + StdioCommandString: bifrost.Ptr(cmdString), } @@ return &schemas.MCPClientState{ - ExecutionConfig: &schemas.MCPClientConfig{ + ExecutionConfig: bifrost.Ptr(schemas.MCPClientConfig{ ID: BifrostMCPClientKey, Name: BifrostMCPClientKey, ToolsToExecute: []string{"*"}, - }, + }), ToolMap: make(map[string]schemas.ChatTool), ToolNameMapping: make(map[string]string), - ConnectionInfo: &schemas.MCPClientConnectionInfo{ + ConnectionInfo: bifrost.Ptr(schemas.MCPClientConnectionInfo{ Type: schemas.MCPConnectionTypeInProcess, - }, + }), }, nilBased on learnings, prefer
bifrost.Ptrfor pointer construction.Also applies to: 825-833
transports/bifrost-http/server/server.go (1)
672-681: Errors from status updates are silently discarded.Lines 674 and 677 check for errors but don't propagate or log them. If
UpdatePluginStatusorAppendPluginStateLogsfails, the failure is hidden. Consider logging these errors.♻️ Suggested improvement
func (s *BifrostHTTPServer) updatePluginErrorStatus(name, step string, err error) error { - if err := s.Config.UpdatePluginStatus(name, schemas.PluginStatusError); err != nil { - return err + if statusErr := s.Config.UpdatePluginStatus(name, schemas.PluginStatusError); statusErr != nil { + logger.Warn("failed to update plugin status for %s: %v", name, statusErr) } - if err := s.Config.AppendPluginStateLogs(name, []string{fmt.Sprintf("error %s plugin %s: %v", step, name, err)}); err != nil { - return err + if logErr := s.Config.AppendPluginStateLogs(name, []string{fmt.Sprintf("error %s plugin %s: %v", step, name, err)}); logErr != nil { + logger.Warn("failed to append plugin state logs for %s: %v", name, logErr) } return err }
| import ( | ||
| "context" | ||
| "fmt" | ||
| "sync" |
There was a problem hiding this comment.
Protect display-name reads with the manager lock.
ExecutionConfig.Name is read after releasing manager.mu. If UpdateMCPClient (in this stack) mutates configs under the lock, these reads become racy; if ExecutionConfig is ever nil, it can also panic. Capture displayName while holding the lock and log after unlock.
🛠️ Proposed fix
import (
"context"
- "fmt"
"sync"
"time"
@@
// Check client exists FIRST before allocating resources
chm.manager.mu.RLock()
clientState, exists := chm.manager.clientMap[chm.clientID]
+ displayName := chm.clientID
+ if exists && clientState.ExecutionConfig != nil {
+ displayName = clientState.ExecutionConfig.Name
+ }
chm.manager.mu.RUnlock()
if !exists {
// Use clientID for logging when client is missing
- logger.Error("%s Health monitor failed to start for client %s, client not found in manager", MCPLogPrefix, chm.clientID)
+ logger.Error("%s Health monitor failed to start for client %s, client not found in manager", MCPLogPrefix, displayName)
return
}
@@
go chm.monitorLoop()
- logger.Debug("%s Health monitor started for client %s", MCPLogPrefix, clientState.ExecutionConfig.Name)
+ logger.Debug("%s Health monitor started for client %s", MCPLogPrefix, displayName)
}
@@
// Acquire read lock before reading clientMap to avoid race condition
chm.manager.mu.RLock()
clientState, exists := chm.manager.clientMap[chm.clientID]
- chm.manager.mu.RUnlock()
-
- // Determine display name for logging: use clientState.ExecutionConfig.Name if available, otherwise fall back to clientID
displayName := chm.clientID
- if exists {
+ if exists && clientState.ExecutionConfig != nil {
displayName = clientState.ExecutionConfig.Name
}
+ chm.manager.mu.RUnlock()
@@
// Only update if state changed
stateChanged := clientState.State != state
if stateChanged {
clientState.State = state
}
+ displayName := chm.clientID
+ if clientState.ExecutionConfig != nil {
+ displayName = clientState.ExecutionConfig.Name
+ }
chm.manager.mu.Unlock()
// Log after releasing the lock
if stateChanged {
- logger.Info(fmt.Sprintf("%s Client %s connection state changed to: %s", MCPLogPrefix, clientState.ExecutionConfig.Name, state))
+ logger.Info("%s Client %s connection state changed to: %s", MCPLogPrefix, displayName, state)
}
}Also applies to: 68-86, 97-123, 191-209
🤖 Prompt for AI Agents
In `@core/mcp/healthmonitor.go` around lines 3 - 6, Read ExecutionConfig.Name
while holding manager.mu and store it in a local displayName variable before
unlocking to avoid races and nil-panics; update all locations that read
ExecutionConfig.Name (e.g., in the health monitor functions that currently read
ExecutionConfig.Name after releasing manager.mu) to capture displayName under
the lock and then perform logging/printing after unlocking. In other words,
inside the critical sections guarded by manager.mu (the same lock used by
UpdateMCPClient), check for nil ExecutionConfig, set displayName :=
manager.ExecutionConfig.Name (or a safe default if nil), then release the lock
and use displayName for subsequent logs; do this for the other affected blocks
(around lines noted: 68-86, 97-123, 191-209) to eliminate races and potential
panics.
| s.Config.ReloadPlugin(plugin) | ||
| s.Config.UpdatePluginOverallStatus(plugin.GetName(), cfg.Name, schemas.PluginStatusActive, | ||
| []string{fmt.Sprintf("plugin %s initialized successfully", cfg.Name)}, getPluginTypes(plugin)) | ||
| []string{fmt.Sprintf("plugin %s initialized successfully", cfg.Name)}, InferPluginTypes(plugin)) |
There was a problem hiding this comment.
Handle ReloadPlugin error return value before marking plugin active.
The Config.ReloadPlugin method returns an error, but this call ignores it and unconditionally marks the plugin as active. This hides registration failures. Apply the same error-handling pattern used in registerPluginWithStatus (utils.go lines 72-99).
Suggested fix
- s.Config.ReloadPlugin(plugin)
- s.Config.UpdatePluginOverallStatus(plugin.GetName(), cfg.Name, schemas.PluginStatusActive,
- []string{fmt.Sprintf("plugin %s initialized successfully", cfg.Name)}, InferPluginTypes(plugin))
+ if err := s.Config.ReloadPlugin(plugin); err != nil {
+ logger.Error("failed to reload plugin %s: %v", cfg.Name, err)
+ s.Config.UpdatePluginOverallStatus(plugin.GetName(), cfg.Name, schemas.PluginStatusError,
+ []string{fmt.Sprintf("error reloading plugin %s: %v", cfg.Name, err)}, InferPluginTypes(plugin))
+ continue
+ }
+ s.Config.UpdatePluginOverallStatus(plugin.GetName(), cfg.Name, schemas.PluginStatusActive,
+ []string{fmt.Sprintf("plugin %s initialized successfully", cfg.Name)}, InferPluginTypes(plugin))🤖 Prompt for AI Agents
In `@transports/bifrost-http/server/plugins.go` around lines 217 - 219, The call
to s.Config.ReloadPlugin(plugin) currently ignores its error and immediately
calls s.Config.UpdatePluginOverallStatus to mark the plugin active; change this
to capture the error returned by ReloadPlugin and follow the same pattern as
registerPluginWithStatus in utils.go (check for non-nil error, log/update status
to PluginStatusFailed with the error message and do not mark active on failure),
only calling UpdatePluginOverallStatus(..., PluginStatusActive, ...) when
ReloadPlugin returns nil; make sure to reference the plugin name via
plugin.GetName()/cfg.Name in the status update and include the error text in the
failure status messages.
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 (9)
core/mcp/toolsync.go (1)
224-234: Guard against nil clientConfig to avoid panic.
The function now accepts a pointer, but it dereferences without a nil check. A nil input will crash the sync manager.🐛 Suggested fix
func ResolveToolSyncInterval(clientConfig *schemas.MCPClientConfig, globalInterval time.Duration) time.Duration { + if clientConfig == nil { + if globalInterval > 0 { + return globalInterval + } + return DefaultToolSyncInterval + } // Per-client explicitly disabled (negative value) if clientConfig.ToolSyncInterval < 0 { return 0 // Disabled for this client }core/bifrost.go (1)
2810-2832: Fix documentation and method name mismatch: UpdateMCPClient vs EditMCPClient.The documentation comment and example on lines 2810–2822 reference
UpdateMCPClient, but the actual exported method (line 2826) is namedEditMCPClient. The example code will not compile. Rename the function toUpdateMCPClientand provide a deprecatedEditMCPClientalias for the one internal call site attransports/bifrost-http/lib/config.go:2923.🛠️ Suggested fix
-func (bifrost *Bifrost) EditMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { +func (bifrost *Bifrost) UpdateMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { if bifrost.mcpManager == nil { return fmt.Errorf("MCP is not configured in this Bifrost instance") } return bifrost.mcpManager.EditClient(id, updatedConfig) } + +// Deprecated: use UpdateMCPClient. +func (bifrost *Bifrost) EditMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error { + return bifrost.UpdateMCPClient(id, updatedConfig) +}core/schemas/mcp.go (1)
79-106: Backwards‑compat risk fromid→client_idJSON rename.
Persisted MCP client configs that still serializeidwill now unmarshal with an empty ID, which can break updates/removals. Consider accepting both fields during a migration window or running a data migration before rollout.🛠️ Suggested compatibility shim
+// UnmarshalJSON supports legacy "id" alongside "client_id". +func (c *MCPClientConfig) UnmarshalJSON(b []byte) error { + type Alias MCPClientConfig + aux := struct { + Alias + LegacyID string `json:"id"` + }{} + if err := sonic.Unmarshal(b, &aux); err != nil { + return err + } + *c = MCPClientConfig(aux.Alias) + if c.ID == "" && aux.LegacyID != "" { + c.ID = aux.LegacyID + } + return nil +}#!/bin/bash # Check for legacy MCP client config JSON that still uses "id" rg -n '"id"\s*:\s*".+"' -g '*.json' -g '*.yml' -g '*.yaml' -g '*.toml' rg -n 'MCPClientConfigJSON|client_configs' -g '*.go' -g '*.sql' -g '*.json'framework/configstore/rdb.go (3)
795-822: Propagate the global MCPToolSyncInterval into MCPConfig.The returned MCPConfig no longer sets ToolSyncInterval, so a configured global interval from TableClientConfig is silently dropped and clients fall back to the default. Add the global interval to keep behavior stable across the stack-wide MCP refactor.
🛠️ Proposed fix
return &schemas.MCPConfig{ ClientConfigs: clientConfigs, - ToolManagerConfig: &toolManagerConfig, + ToolManagerConfig: &toolManagerConfig, + ToolSyncInterval: time.Duration(clientConfig.MCPToolSyncInterval) * time.Minute, }, nilAs per coding guidelines, reviewed in stack context.
851-873: Persist tool pricing on MCP client creation.ToolPricing from schemas.MCPClientConfig is not copied into the TableMCPClient, so pricing gets dropped on create even though updates persist it. This breaks MCP pricing for OAuth and non-OAuth creates.
🛠️ Proposed fix
dbClient := tables.TableMCPClient{ ClientID: clientConfigCopy.ID, Name: clientConfigCopy.Name, IsCodeModeClient: clientConfigCopy.IsCodeModeClient, @@ Headers: clientConfigCopy.Headers, IsPingAvailable: clientConfigCopy.IsPingAvailable, ToolSyncInterval: int(clientConfigCopy.ToolSyncInterval.Minutes()), + ToolPricing: clientConfigCopy.ToolPricing, }
899-947:is_ping_availablewill be reset tofalseif omitted from requests.When the update request deserializes into
TableMCPClient, any omitted boolean field defaults tofalse. ThemergeMCPRedactedValuesfunction (which preserves sensitive fields likeConnectionStringandHeaders) does not handleis_ping_available, and the update directly persists the deserialized value without fallback toexistingClient.IsPingAvailable. The code at lines 40–42 (if clientConfigCopy.IsPingAvailable { clientConfigCopy.IsPingAvailable = true }) is a no-op that only reinforces true values.To fix: either switch
IsPingAvailableto*boolfor optional handling, or add logic inmergeMCPRedactedValuesto preserve the existing value when the field is absent from the request.transports/bifrost-http/handlers/mcp.go (1)
191-258: OAuth pending config drops ping/tool-sync/pricing fields.In the OAuth path, the pending MCP config omits
IsPingAvailable,ToolSyncInterval, andToolPricing, so these values are lost after OAuth completes. Persist them the same way as non-OAuth create.🛠️ Proposed fix
if req.AuthType == "oauth" { @@ - pendingConfig := schemas.MCPClientConfig{ + toolSyncInterval := 10 * time.Minute + if req.ToolSyncInterval != 0 { + toolSyncInterval = time.Duration(req.ToolSyncInterval) * time.Minute + } else { + config, err := h.store.ConfigStore.GetClientConfig(ctx) + if err != nil { + SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("failed to get client config: %v", err)) + return + } + if config != nil { + toolSyncInterval = time.Duration(config.MCPToolSyncInterval) * time.Minute + } + } + + pendingConfig := schemas.MCPClientConfig{ ID: req.ClientID, Name: req.Name, IsCodeModeClient: req.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(req.ConnectionType), ConnectionString: req.ConnectionString, StdioConfig: req.StdioConfig, AuthType: schemas.MCPAuthType(req.AuthType), OauthConfigID: &flowInitiation.OauthConfigID, ToolsToExecute: req.ToolsToExecute, ToolsToAutoExecute: req.ToolsToAutoExecute, Headers: req.Headers, + IsPingAvailable: req.IsPingAvailable, + ToolSyncInterval: toolSyncInterval, + ToolPricing: req.ToolPricing, }transports/bifrost-http/lib/config.go (1)
857-869: Guard against nil MCP client entries before dereference.
ClientConfigsis now a pointer slice; anullentry in config JSON would panic during merge or lookup.🛠️ Suggested guard
for _, newClientConfig := range tempMCPConfig.ClientConfigs { + if newClientConfig == nil { + continue + } found := false for _, existingClientConfig := range mcpConfig.ClientConfigs { + if existingClientConfig == nil { + continue + } if (newClientConfig.ID != "" && existingClientConfig.ID == newClientConfig.ID) || (newClientConfig.Name != "" && existingClientConfig.Name == newClientConfig.Name) { found = true break } } if !found { clientConfigsToAdd = append(clientConfigsToAdd, newClientConfig) } } for _, clientConfig := range c.MCPConfig.ClientConfigs { + if clientConfig == nil { + continue + } if clientConfig.ID == id { return clientConfig, nil } }Also applies to: 2774-2776
core/mcp/clientmanager.go (1)
75-99: Avoid sharing mutable MCPClientConfig pointers across layers.These functions store the caller-owned pointer directly in
ExecutionConfig. The transport layer also mutates the same config under a different mutex, which can cause data races and inconsistent reads in the MCP manager. Consider cloning the config before storing/connecting so the manager owns its copy.🛠️ Suggested fix (clone before storing/connecting)
func (m *MCPManager) AddClient(config *schemas.MCPClientConfig) error { if err := validateMCPClientConfig(config); err != nil { return fmt.Errorf("invalid MCP client configuration: %w", err) } - // Make a copy of the config to use after unlocking - configCopy := config + // Make a copy of the config to use after unlocking + configCopy := cloneMCPClientConfig(config) m.mu.Lock() - if _, ok := m.clientMap[config.ID]; ok { + if _, ok := m.clientMap[configCopy.ID]; ok { m.mu.Unlock() return fmt.Errorf("client %s already exists", config.Name) } // Create placeholder entry - m.clientMap[config.ID] = &schemas.MCPClientState{ - Name: config.Name, - ExecutionConfig: config, + m.clientMap[configCopy.ID] = &schemas.MCPClientState{ + Name: configCopy.Name, + ExecutionConfig: configCopy, ToolMap: make(map[string]schemas.ChatTool), ToolNameMapping: make(map[string]string), ConnectionInfo: &schemas.MCPClientConnectionInfo{ - Type: config.ConnectionType, + Type: configCopy.ConnectionType, }, } // Temporarily unlock for the connection attempt m.mu.Unlock() // Connect using the copied config if err := m.connectToMCPClient(configCopy); err != nil { // Re-lock to clean up the failed entry m.mu.Lock() - delete(m.clientMap, config.ID) + delete(m.clientMap, configCopy.ID) m.mu.Unlock() return fmt.Errorf("failed to connect to MCP client %s: %w", config.Name, err) } return nil } func (m *MCPManager) AddClientInMemory(config *schemas.MCPClientConfig) error { if err := validateMCPClientConfig(config); err != nil { return fmt.Errorf("invalid MCP client configuration: %w", err) } - // Make a copy of the config to use after unlocking - configCopy := config + // Make a copy of the config to use after unlocking + configCopy := cloneMCPClientConfig(config) m.mu.Lock() - if _, ok := m.clientMap[config.ID]; ok { + if _, ok := m.clientMap[configCopy.ID]; ok { m.mu.Unlock() return fmt.Errorf("client %s already exists", config.Name) } // Create placeholder entry - m.clientMap[config.ID] = &schemas.MCPClientState{ - Name: config.Name, - ExecutionConfig: config, + m.clientMap[configCopy.ID] = &schemas.MCPClientState{ + Name: configCopy.Name, + ExecutionConfig: configCopy, ToolMap: make(map[string]schemas.ChatTool), ToolNameMapping: make(map[string]string), ConnectionInfo: &schemas.MCPClientConnectionInfo{ - Type: config.ConnectionType, + Type: configCopy.ConnectionType, }, } // Temporarily unlock for the connection attempt m.mu.Unlock() // Connect using the copied config if err := m.connectToMCPClient(configCopy); err != nil { // Re-lock to clean up the failed entry m.mu.Lock() - delete(m.clientMap, config.ID) + delete(m.clientMap, configCopy.ID) m.mu.Unlock() return fmt.Errorf("failed to connect to MCP client %s: %w", config.Name, err) } return nil }Add a small helper near these methods:
func cloneMCPClientConfig(in *schemas.MCPClientConfig) *schemas.MCPClientConfig { if in == nil { return nil } out := *in if in.ConnectionString != nil { cs := *in.ConnectionString out.ConnectionString = &cs } if in.StdioConfig != nil { sc := *in.StdioConfig sc.Args = append([]string(nil), in.StdioConfig.Args...) sc.Envs = append([]string(nil), in.StdioConfig.Envs...) out.StdioConfig = &sc } if in.Headers != nil { out.Headers = maps.Clone(in.Headers) } if in.ToolsToExecute != nil { out.ToolsToExecute = append([]string(nil), in.ToolsToExecute...) } if in.ToolsToAutoExecute != nil { out.ToolsToAutoExecute = append([]string(nil), in.ToolsToAutoExecute...) } if in.ToolPricing != nil { out.ToolPricing = maps.Clone(in.ToolPricing) } return &out }Also applies to: 125-149
🤖 Fix all issues with AI agents
In `@framework/oauth2/main.go`:
- Around line 201-311: The MCPClientConfigJSON field is being stored in
plaintext by StorePendingMCPClient and read back by
GetPendingMCPClient/GetPendingMCPClientByState which leaks secrets; update
StorePendingMCPClient to encrypt the marshalled MCPClientConfig before assigning
to oauthConfig.MCPClientConfigJSON using the same encryption routine you use for
the ClientSecret field, update GetPendingMCPClient and
GetPendingMCPClientByState to decrypt the stored MCPClientConfigJSON after
fetching and before unmarshalling, and ensure RemovePendingMCPClient still
clears the encrypted payload; reference the MCPClientConfigJSON field, the
ClientSecret encryption flow, and the methods StorePendingMCPClient,
GetPendingMCPClient, GetPendingMCPClientByState, and RemovePendingMCPClient when
making the changes.
In `@transports/bifrost-http/handlers/oauth2.go`:
- Around line 226-245: Change StorePendingMCPClient to accept a pointer
(*schemas.MCPClientConfig) instead of a value to match the Get* APIs: update the
method signature on OAuthHandler and the underlying
oauthProvider.StorePendingMCPClient call, add a nil check at the start of
StorePendingMCPClient to return an error if the pointer is nil, and adjust the
call site in transports/bifrost-http/handlers/mcp.go from passing pendingConfig
to passing &pendingConfig (or the appropriate pointer) so callers use pointer
semantics consistently with GetPendingMCPClient and GetPendingMCPClientByState.
🧹 Nitpick comments (3)
transports/bifrost-http/lib/config_test.go (1)
466-500: Preferbifrost.Ptr(...)for new MCP pointer literals.Repo convention favors
bifrost.Ptrover&for pointer creation. Apply it to the new MCPConfig/MCPClientConfig literals in the mock and tests (add the import if needed). Based on learnings, please align new pointers tobifrost.Ptr(...).♻️ Example updates
- m.mcpConfig = &schemas.MCPConfig{ - ClientConfigs: []*schemas.MCPClientConfig{}, - } + m.mcpConfig = bifrost.Ptr(schemas.MCPConfig{ + ClientConfigs: []*schemas.MCPClientConfig{}, + }) - m.mcpConfig.ClientConfigs[i] = &schemas.MCPClientConfig{ + m.mcpConfig.ClientConfigs[i] = bifrost.Ptr(schemas.MCPClientConfig{ ID: clientConfig.ClientID, Name: clientConfig.Name, IsCodeModeClient: clientConfig.IsCodeModeClient, ConnectionType: schemas.MCPConnectionType(clientConfig.ConnectionType), ConnectionString: clientConfig.ConnectionString, StdioConfig: clientConfig.StdioConfig, Headers: clientConfig.Headers, ToolsToExecute: clientConfig.ToolsToExecute, ToolsToAutoExecute: clientConfig.ToolsToAutoExecute, - } + }) - err = config1.ConfigStore.CreateMCPClientConfig(ctx, &mcpClientConfig) + err = config1.ConfigStore.CreateMCPClientConfig(ctx, bifrost.Ptr(mcpClientConfig))Also applies to: 8987-8987, 9076-9087, 9398-9408, 9563-9564, 9685-9686, 9779-9780
core/internal/mcptests/integration_test.go (1)
34-38: Prefer the repo pointer helper for MCP client configs.
Use the core pointer helper to align with the repo convention and avoid direct address-taking when passing configs intoAddClient. Consider aliasing the import to avoid the localbifrostvariable name.♻️ Suggested change
import ( "fmt" "testing" "time" + bifrostcore "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ - err := manager.AddClient(&httpConfig) + err := manager.AddClient(bifrostcore.Ptr(httpConfig)) @@ - err := manager.AddClient(&httpConfig) + err := manager.AddClient(bifrostcore.Ptr(httpConfig))Based on learnings, please prefer the core pointer helper for pointer creation.
Also applies to: 341-345
core/internal/mcptests/agent_filtering_test.go (1)
529-535: Prefer the repo pointer helper for MCP client configs.
Use the core pointer helper to keep pointer creation consistent with repo conventions when passing configs intoAddClient.♻️ Suggested change
import ( "testing" "time" + bifrostcore "github.com/maximhq/bifrost/core" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ - err = manager.AddClient(&tempConfig) + err = manager.AddClient(bifrostcore.Ptr(tempConfig)) @@ - err = manager.AddClient(&tempConfig) + err = manager.AddClient(bifrostcore.Ptr(tempConfig))Based on learnings, please prefer the core pointer helper for pointer creation.
Also applies to: 618-624
| // StorePendingMCPClient stores an MCP client config that's waiting for OAuth completion | ||
| func (p *OAuth2Provider) StorePendingMCPClient(mcpClientID string, mcpClientConfig schemas.MCPClientConfig) { | ||
| p.mu.Lock() | ||
| defer p.mu.Unlock() | ||
| oauthConfigID := "" | ||
| if mcpClientConfig.OauthConfigID != nil { | ||
| oauthConfigID = *mcpClientConfig.OauthConfigID | ||
| // The config is persisted in the database (oauth_configs.mcp_client_config_json) to support | ||
| // multi-instance deployments where OAuth callback may hit a different server instance. | ||
| func (p *OAuth2Provider) StorePendingMCPClient(oauthConfigID string, mcpClientConfig schemas.MCPClientConfig) error { | ||
| ctx := context.Background() | ||
|
|
||
| oauthConfig, err := p.configStore.GetOauthConfigByID(ctx, oauthConfigID) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get oauth config: %w", err) | ||
| } | ||
| p.pendingMCPClients[mcpClientID] = &PendingMCPClient{ | ||
| MCPClientConfig: mcpClientConfig, | ||
| OauthConfigID: oauthConfigID, | ||
| CreatedAt: time.Now(), | ||
| if oauthConfig == nil { | ||
| return fmt.Errorf("oauth config not found: %s", oauthConfigID) | ||
| } | ||
| } | ||
|
|
||
| // GetPendingMCPClient retrieves an MCP client config by mcp_client_id | ||
| func (p *OAuth2Provider) GetPendingMCPClient(mcpClientID string) *schemas.MCPClientConfig { | ||
| p.mu.RLock() | ||
| defer p.mu.RUnlock() | ||
| if pending, exists := p.pendingMCPClients[mcpClientID]; exists { | ||
| return &pending.MCPClientConfig | ||
| configJSON, err := json.Marshal(mcpClientConfig) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal MCP client config: %w", err) | ||
| } | ||
| configStr := string(configJSON) | ||
| oauthConfig.MCPClientConfigJSON = &configStr | ||
|
|
||
| if err := p.configStore.UpdateOauthConfig(ctx, oauthConfig); err != nil { | ||
| return fmt.Errorf("failed to update oauth config with MCP client config: %w", err) | ||
| } | ||
|
|
||
| logger.Debug("Stored pending MCP client config", "oauth_config_id", oauthConfigID) | ||
| return nil | ||
| } | ||
|
|
||
| // RemovePendingMCPClient removes a pending MCP client after OAuth completion | ||
| func (p *OAuth2Provider) RemovePendingMCPClient(mcpClientID string) { | ||
| p.mu.Lock() | ||
| defer p.mu.Unlock() | ||
| delete(p.pendingMCPClients, mcpClientID) | ||
| // GetPendingMCPClient retrieves an MCP client config by oauth_config_id | ||
| // Returns nil if no pending config is found or if the oauth config has expired | ||
| func (p *OAuth2Provider) GetPendingMCPClient(oauthConfigID string) (*schemas.MCPClientConfig, error) { | ||
| ctx := context.Background() | ||
|
|
||
| oauthConfig, err := p.configStore.GetOauthConfigByID(ctx, oauthConfigID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get oauth config: %w", err) | ||
| } | ||
| if oauthConfig == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| // Check if expired | ||
| if time.Now().After(oauthConfig.ExpiresAt) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| if oauthConfig.MCPClientConfigJSON == nil || *oauthConfig.MCPClientConfigJSON == "" { | ||
| return nil, nil | ||
| } | ||
|
|
||
| var config schemas.MCPClientConfig | ||
| if err := json.Unmarshal([]byte(*oauthConfig.MCPClientConfigJSON), &config); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal MCP client config: %w", err) | ||
| } | ||
|
|
||
| return &config, nil | ||
| } | ||
|
|
||
| // cleanupExpiredPendingClients removes pending MCP clients older than 5 minutes | ||
| func (p *OAuth2Provider) cleanupExpiredPendingClients() { | ||
| ticker := time.NewTicker(1 * time.Minute) | ||
| defer ticker.Stop() | ||
|
|
||
| for range ticker.C { | ||
| p.mu.Lock() | ||
| now := time.Now() | ||
| for mcpClientID, pending := range p.pendingMCPClients { | ||
| if now.Sub(pending.CreatedAt) > 5*time.Minute { | ||
| delete(p.pendingMCPClients, mcpClientID) | ||
| logger.Debug("Cleaned up expired pending MCP client", "mcp_client_id", mcpClientID) | ||
| } | ||
| } | ||
| p.mu.Unlock() | ||
| // GetPendingMCPClientByState retrieves an MCP client config by OAuth state token | ||
| // This is useful when the callback only has the state parameter | ||
| func (p *OAuth2Provider) GetPendingMCPClientByState(state string) (*schemas.MCPClientConfig, string, error) { | ||
| ctx := context.Background() | ||
|
|
||
| oauthConfig, err := p.configStore.GetOauthConfigByState(ctx, state) | ||
| if err != nil { | ||
| return nil, "", fmt.Errorf("failed to get oauth config by state: %w", err) | ||
| } | ||
| if oauthConfig == nil { | ||
| return nil, "", nil | ||
| } | ||
|
|
||
| // Check if expired | ||
| if time.Now().After(oauthConfig.ExpiresAt) { | ||
| return nil, "", nil | ||
| } | ||
|
|
||
| if oauthConfig.MCPClientConfigJSON == nil || *oauthConfig.MCPClientConfigJSON == "" { | ||
| return nil, oauthConfig.ID, nil | ||
| } | ||
|
|
||
| var config schemas.MCPClientConfig | ||
| if err := json.Unmarshal([]byte(*oauthConfig.MCPClientConfigJSON), &config); err != nil { | ||
| return nil, oauthConfig.ID, fmt.Errorf("failed to unmarshal MCP client config: %w", err) | ||
| } | ||
|
|
||
| return &config, oauthConfig.ID, nil | ||
| } | ||
|
|
||
| // RemovePendingMCPClient clears the pending MCP client config from the oauth config | ||
| // This is called after OAuth completion to clean up | ||
| func (p *OAuth2Provider) RemovePendingMCPClient(oauthConfigID string) error { | ||
| ctx := context.Background() | ||
|
|
||
| oauthConfig, err := p.configStore.GetOauthConfigByID(ctx, oauthConfigID) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get oauth config: %w", err) | ||
| } | ||
| if oauthConfig == nil { | ||
| return nil // Already removed or doesn't exist | ||
| } | ||
|
|
||
| oauthConfig.MCPClientConfigJSON = nil | ||
|
|
||
| if err := p.configStore.UpdateOauthConfig(ctx, oauthConfig); err != nil { | ||
| return fmt.Errorf("failed to clear pending MCP client config: %w", err) | ||
| } | ||
|
|
||
| logger.Debug("Removed pending MCP client config", "oauth_config_id", oauthConfigID) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for MCPClientConfigJSON usage and related storage/encryption handling
rg -n 'MCPClientConfigJSON' -g '*.go' -A 3 -B 3Repository: maximhq/bifrost
Length of output: 3563
🏁 Script executed:
# Check the schema definition for TableOauthConfig
rg -n 'type TableOauthConfig|type OauthConfig' -g '*.go' -A 20Repository: maximhq/bifrost
Length of output: 3382
🏁 Script executed:
# Look for encryption, KMS, Vault, or Secret handling
rg -n 'Encrypt|Decrypt|KMS|Vault|Secret|cipher|crypto' -g '*.go' | head -50Repository: maximhq/bifrost
Length of output: 5718
🏁 Script executed:
# Check database migrations or schema definitions
fd -e 'sql' -o -e 'migration*' | head -20Repository: maximhq/bifrost
Length of output: 175
🏁 Script executed:
# Find MCPClientConfig definition to see what sensitive data it contains
rg -n 'type MCPClientConfig' -g '*.go' -A 15Repository: maximhq/bifrost
Length of output: 2003
🏁 Script executed:
# Check if there's any encryption/decryption logic applied to MCPClientConfigJSON
rg -n 'MCPClientConfigJSON.*Encrypt|Decrypt.*MCPClientConfigJSON' -g '*.go'Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Look for any middleware or hooks that might encrypt this field
rg -n 'BeforeSave|AfterLoad|Hook' framework/configstore/ -g '*.go' -A 5 | head -100Repository: maximhq/bifrost
Length of output: 7260
🏁 Script executed:
# Check if UpdateOauthConfig or GetOauthConfigByID have any encryption logic
rg -n 'func.*UpdateOauthConfig|func.*GetOauthConfigByID' framework/configstore/ -g '*.go' -A 20Repository: maximhq/bifrost
Length of output: 3211
🏁 Script executed:
# Check where StorePendingMCPClient is called from
rg -n 'StorePendingMCPClient' -g '*.go' -B 5 -A 5Repository: maximhq/bifrost
Length of output: 3487
🏁 Script executed:
# Check if MCPClientConfig is encrypted/sanitized before being passed to StorePendingMCPClient
rg -n 'func.*OAuth.*MCP|func.*MCP.*OAuth' framework/oauth2/ -g '*.go' -A 10 | head -60Repository: maximhq/bifrost
Length of output: 2772
🏁 Script executed:
# Look for ClientSecret encryption to see how sensitive data is handled elsewhere in oauth
rg -n 'ClientSecret.*Encrypt|encrypt.*ClientSecret' -g '*.go' -B 3 -A 3Repository: maximhq/bifrost
Length of output: 1217
MCPClientConfigJSON stores secrets in plaintext without encryption.
MCPClientConfig can contain sensitive data (ConnectionString, Headers with API keys/auth tokens). The current implementation marshals this directly to JSON and stores it unencrypted in oauth_configs.mcp_client_config_json. This is inconsistent with the ClientSecret field which is encrypted, and creates a compliance/security risk.
Encrypt the MCPClientConfig before storing and decrypt after retrieval, similar to how ClientSecret is handled, or refactor to store only a reference and fetch the full config from a secure secrets store.
🤖 Prompt for AI Agents
In `@framework/oauth2/main.go` around lines 201 - 311, The MCPClientConfigJSON
field is being stored in plaintext by StorePendingMCPClient and read back by
GetPendingMCPClient/GetPendingMCPClientByState which leaks secrets; update
StorePendingMCPClient to encrypt the marshalled MCPClientConfig before assigning
to oauthConfig.MCPClientConfigJSON using the same encryption routine you use for
the ClientSecret field, update GetPendingMCPClient and
GetPendingMCPClientByState to decrypt the stored MCPClientConfigJSON after
fetching and before unmarshalling, and ensure RemovePendingMCPClient still
clears the encrypted payload; reference the MCPClientConfigJSON field, the
ClientSecret encryption flow, and the methods StorePendingMCPClient,
GetPendingMCPClient, GetPendingMCPClientByState, and RemovePendingMCPClient when
making the changes.
| // StorePendingMCPClient stores an MCP client config in the database while waiting for OAuth completion | ||
| // This supports multi-instance deployments where OAuth callback may hit a different server instance | ||
| func (h *OAuthHandler) StorePendingMCPClient(oauthConfigID string, mcpClientConfig schemas.MCPClientConfig) error { | ||
| return h.oauthProvider.StorePendingMCPClient(oauthConfigID, mcpClientConfig) | ||
| } | ||
|
|
||
| // GetPendingMCPClient retrieves a pending MCP client config by mcp_client_id | ||
| func (h *OAuthHandler) GetPendingMCPClient(mcpClientID string) *schemas.MCPClientConfig { | ||
| return h.oauthProvider.GetPendingMCPClient(mcpClientID) | ||
| // GetPendingMCPClient retrieves a pending MCP client config by oauth_config_id | ||
| func (h *OAuthHandler) GetPendingMCPClient(oauthConfigID string) (*schemas.MCPClientConfig, error) { | ||
| return h.oauthProvider.GetPendingMCPClient(oauthConfigID) | ||
| } | ||
|
|
||
| // GetPendingMCPClientByState retrieves a pending MCP client config by OAuth state token | ||
| func (h *OAuthHandler) GetPendingMCPClientByState(state string) (*schemas.MCPClientConfig, string, error) { | ||
| return h.oauthProvider.GetPendingMCPClientByState(state) | ||
| } | ||
|
|
||
| // RemovePendingMCPClient removes a pending MCP client after OAuth completion | ||
| func (h *OAuthHandler) RemovePendingMCPClient(mcpClientID string) { | ||
| h.oauthProvider.RemovePendingMCPClient(mcpClientID) | ||
| func (h *OAuthHandler) RemovePendingMCPClient(oauthConfigID string) error { | ||
| return h.oauthProvider.RemovePendingMCPClient(oauthConfigID) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all calls to StorePendingMCPClient
rg -n 'StorePendingMCPClient\(' -g '*.go'Repository: maximhq/bifrost
Length of output: 645
🏁 Script executed:
# Check the OAuthProvider interface definition
rg -n 'type OAuthProvider|StorePendingMCPClient' -g '*.go' -A 2 -B 2Repository: maximhq/bifrost
Length of output: 2133
🏁 Script executed:
# Verify the actual provider implementation to see what signature is there
fd -name '*.go' | xargs rg -l 'StorePendingMCPClient' | head -20Repository: maximhq/bifrost
Length of output: 414
🏁 Script executed:
# Check the context around the StorePendingMCPClient call in mcp.go
sed -n '240,270p' transports/bifrost-http/handlers/mcp.goRepository: maximhq/bifrost
Length of output: 1372
🏁 Script executed:
# Check the return types and signatures of the other pending MCP methods
rg -n 'GetPendingMCPClient|RemovePendingMCPClient' -g '*.go' -A 1Repository: maximhq/bifrost
Length of output: 2968
🏁 Script executed:
# Check the actual definition of pendingConfig variable
rg -n 'pendingConfig' transports/bifrost-http/handlers/mcp.go -B 5Repository: maximhq/bifrost
Length of output: 598
Align StorePendingMCPClient signature with other pending config APIs.
The Get methods return *schemas.MCPClientConfig (pointers), but StorePendingMCPClient takes a value. This inconsistency forces callers to manage value semantics for store and pointer semantics for retrieval. Change StorePendingMCPClient to accept a pointer and add a nil check for safety.
The call site at transports/bifrost-http/handlers/mcp.go:254 will need a minor update from pendingConfig to &pendingConfig.
♻️ Suggested signature alignment
-func (h *OAuthHandler) StorePendingMCPClient(oauthConfigID string, mcpClientConfig schemas.MCPClientConfig) error {
- return h.oauthProvider.StorePendingMCPClient(oauthConfigID, mcpClientConfig)
+func (h *OAuthHandler) StorePendingMCPClient(oauthConfigID string, mcpClientConfig *schemas.MCPClientConfig) error {
+ if mcpClientConfig == nil {
+ return fmt.Errorf("mcpClientConfig is required")
+ }
+ return h.oauthProvider.StorePendingMCPClient(oauthConfigID, *mcpClientConfig)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // StorePendingMCPClient stores an MCP client config in the database while waiting for OAuth completion | |
| // This supports multi-instance deployments where OAuth callback may hit a different server instance | |
| func (h *OAuthHandler) StorePendingMCPClient(oauthConfigID string, mcpClientConfig schemas.MCPClientConfig) error { | |
| return h.oauthProvider.StorePendingMCPClient(oauthConfigID, mcpClientConfig) | |
| } | |
| // GetPendingMCPClient retrieves a pending MCP client config by mcp_client_id | |
| func (h *OAuthHandler) GetPendingMCPClient(mcpClientID string) *schemas.MCPClientConfig { | |
| return h.oauthProvider.GetPendingMCPClient(mcpClientID) | |
| // GetPendingMCPClient retrieves a pending MCP client config by oauth_config_id | |
| func (h *OAuthHandler) GetPendingMCPClient(oauthConfigID string) (*schemas.MCPClientConfig, error) { | |
| return h.oauthProvider.GetPendingMCPClient(oauthConfigID) | |
| } | |
| // GetPendingMCPClientByState retrieves a pending MCP client config by OAuth state token | |
| func (h *OAuthHandler) GetPendingMCPClientByState(state string) (*schemas.MCPClientConfig, string, error) { | |
| return h.oauthProvider.GetPendingMCPClientByState(state) | |
| } | |
| // RemovePendingMCPClient removes a pending MCP client after OAuth completion | |
| func (h *OAuthHandler) RemovePendingMCPClient(mcpClientID string) { | |
| h.oauthProvider.RemovePendingMCPClient(mcpClientID) | |
| func (h *OAuthHandler) RemovePendingMCPClient(oauthConfigID string) error { | |
| return h.oauthProvider.RemovePendingMCPClient(oauthConfigID) | |
| } | |
| // StorePendingMCPClient stores an MCP client config in the database while waiting for OAuth completion | |
| // This supports multi-instance deployments where OAuth callback may hit a different server instance | |
| func (h *OAuthHandler) StorePendingMCPClient(oauthConfigID string, mcpClientConfig *schemas.MCPClientConfig) error { | |
| if mcpClientConfig == nil { | |
| return fmt.Errorf("mcpClientConfig is required") | |
| } | |
| return h.oauthProvider.StorePendingMCPClient(oauthConfigID, *mcpClientConfig) | |
| } | |
| // GetPendingMCPClient retrieves a pending MCP client config by oauth_config_id | |
| func (h *OAuthHandler) GetPendingMCPClient(oauthConfigID string) (*schemas.MCPClientConfig, error) { | |
| return h.oauthProvider.GetPendingMCPClient(oauthConfigID) | |
| } | |
| // GetPendingMCPClientByState retrieves a pending MCP client config by OAuth state token | |
| func (h *OAuthHandler) GetPendingMCPClientByState(state string) (*schemas.MCPClientConfig, string, error) { | |
| return h.oauthProvider.GetPendingMCPClientByState(state) | |
| } | |
| // RemovePendingMCPClient removes a pending MCP client after OAuth completion | |
| func (h *OAuthHandler) RemovePendingMCPClient(oauthConfigID string) error { | |
| return h.oauthProvider.RemovePendingMCPClient(oauthConfigID) | |
| } |
🤖 Prompt for AI Agents
In `@transports/bifrost-http/handlers/oauth2.go` around lines 226 - 245, Change
StorePendingMCPClient to accept a pointer (*schemas.MCPClientConfig) instead of
a value to match the Get* APIs: update the method signature on OAuthHandler and
the underlying oauthProvider.StorePendingMCPClient call, add a nil check at the
start of StorePendingMCPClient to return an error if the pointer is nil, and
adjust the call site in transports/bifrost-http/handlers/mcp.go from passing
pendingConfig to passing &pendingConfig (or the appropriate pointer) so callers
use pointer semantics consistently with GetPendingMCPClient and
GetPendingMCPClientByState.
1 participant
Summary
Refactored MCP client configuration to use pointer types for better memory management and consistency. This change improves the handling of MCP client configurations throughout the codebase.
Changes
MCPClientConfigparameter types from value to pointer types in function signaturesMCPConfig.ClientConfigsto use[]*MCPClientConfiginstead of[]MCPClientConfigEditMCPClienttoUpdateMCPClientin HTTP handlers for better API naming consistencyType of change
Affected areas
How to test
Breaking changes
This change modifies function signatures in the MCP manager and related components. Any code directly calling these functions will need to be updated to pass pointers to
MCPClientConfigobjects rather than values.Security considerations
No security implications as this is a refactoring of internal types.
Checklist