feat: add MCP Plugin#93
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces a new MCP plugin system for the Bifrost framework, including a CLI chatbot, plugin infrastructure, and comprehensive integration tests. The schema is extended to support tracking approved and pending MCP tools. New files implement the MCP host, CLI chatbot, Go module definitions, and plugin tests, while schemas are updated for tool approval flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_Chatbot
participant Bifrost
participant MCP_Plugin
participant MCP_Server
User->>CLI_Chatbot: Enter message
CLI_Chatbot->>Bifrost: Send chat request
Bifrost->>MCP_Plugin: PreHook (inject tools)
MCP_Plugin->>MCP_Server: (if tool call) Execute tool(s)
MCP_Server-->>MCP_Plugin: Tool response(s)
MCP_Plugin->>Bifrost: PostHook (handle tool results)
Bifrost->>CLI_Chatbot: Return response (may include pending tools)
CLI_Chatbot->>User: Display response / prompt for tool approval
User->>CLI_Chatbot: Approve tool(s) (if needed)
CLI_Chatbot->>Bifrost: Send approved tool request
Bifrost->>MCP_Plugin: PreHook/PostHook (as above)
Bifrost->>CLI_Chatbot: Return final response
CLI_Chatbot->>User: Display final answer
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
plugins/mcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
core/schemas/bifrost.go(3 hunks)plugins/mcp/chat/chatbot.go(1 hunks)plugins/mcp/go.mod(1 hunks)plugins/mcp/main.go(1 hunks)plugins/mcp/plugin_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
plugins/mcp/plugin_test.go
591-591: Error return value of plugin.Cleanup is not checked
(errcheck)
688-688: Error return value of plugin.Cleanup is not checked
(errcheck)
174-174: func printChoicesAsJSON is unused
(unused)
plugins/mcp/chat/chatbot.go
429-429: Error return value of s.mcpPlugin.Cleanup is not checked
(errcheck)
416-416: SA1019: strings.Title has been deprecated since Go 1.18 and an alternative has been available since Go 1.0: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead.
(staticcheck)
plugins/mcp/main.go
125-125: Error return value of p.startLocalMCPServer is not checked
(errcheck)
132-132: Error return value of plugin.setupLocalHost is not checked
(errcheck)
95-95: ineffectual assignment to serverPort
(ineffassign)
🔇 Additional comments (2)
plugins/mcp/go.mod (1)
3-3: Verify the Go version specification.Go version 1.24.1 seems unusual. As of recent knowledge, Go versions were around 1.22.x. Please verify this is a valid Go version for June 2025.
What is the latest stable version of Go programming language?core/schemas/bifrost.go (1)
72-73: Schema extensions are well-designed.The new MCP-related fields are properly structured with:
- Optional pointers for backward compatibility
- Clear JSON tags and field documentation
- Appropriate data types for the safe mode approval flow
Also applies to: 407-413, 439-446
| serverPort := config.ServerPort | ||
| if serverPort == "" { | ||
| serverPort = DefaultServerPort | ||
| } | ||
|
|
||
| clientTransport := httpTransport.NewHTTPClientTransport("/mcp") | ||
| clientTransport.WithBaseURL(fmt.Sprintf("http://localhost%s", config.ServerPort)) | ||
| client := mcp_golang.NewClientWithInfo(clientTransport, mcp_golang.ClientInfo{ |
There was a problem hiding this comment.
Remove ineffectual assignment.
The initial assignment to serverPort on line 93 is immediately overwritten on line 95.
- serverPort := config.ServerPort
- if serverPort == "" {
+ serverPort := config.ServerPort
+ if serverPort == "" {
serverPort = DefaultServerPort
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.64.8)
95-95: ineffectual assignment to serverPort
(ineffassign)
🤖 Prompt for AI Agents
In plugins/mcp/main.go around lines 93 to 100, the variable serverPort is
assigned from config.ServerPort and then immediately overwritten if empty.
Remove the initial assignment on line 93 and directly assign serverPort with a
conditional expression that sets it to config.ServerPort if not empty, otherwise
to DefaultServerPort. Then use serverPort in the fmt.Sprintf call to ensure the
value is consistent and not overwritten unnecessarily.
| if s.mcpPlugin != nil { | ||
| s.mcpPlugin.Cleanup() | ||
| } |
There was a problem hiding this comment.
Check error return from cleanup.
The error from s.mcpPlugin.Cleanup() should be handled or at least logged.
if s.mcpPlugin != nil {
- s.mcpPlugin.Cleanup()
+ if err := s.mcpPlugin.Cleanup(); err != nil {
+ fmt.Printf("Warning: Failed to cleanup MCP plugin: %v\n", err)
+ }
}📝 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.
| if s.mcpPlugin != nil { | |
| s.mcpPlugin.Cleanup() | |
| } | |
| if s.mcpPlugin != nil { | |
| if err := s.mcpPlugin.Cleanup(); err != nil { | |
| fmt.Printf("Warning: Failed to cleanup MCP plugin: %v\n", err) | |
| } | |
| } |
🧰 Tools
🪛 golangci-lint (1.64.8)
429-429: Error return value of s.mcpPlugin.Cleanup is not checked
(errcheck)
🤖 Prompt for AI Agents
In plugins/mcp/chat/chatbot.go around lines 428 to 430, the call to
s.mcpPlugin.Cleanup() returns an error that is currently ignored. Modify the
code to capture this error and handle it appropriately, such as logging the
error using a logger or returning it if applicable, to ensure any cleanup issues
are not silently ignored.
| systemPrompt := session.history[0] // Assuming first message is system | ||
| session.history = []schemas.BifrostMessage{systemPrompt} | ||
| fmt.Println("🧹 Conversation history cleared!") |
There was a problem hiding this comment.
Potential index out of bounds error.
Assuming session.history[0] is the system prompt could cause a panic if history is empty.
case "/clear":
// Keep system prompt but clear conversation history
- systemPrompt := session.history[0] // Assuming first message is system
- session.history = []schemas.BifrostMessage{systemPrompt}
+ if len(session.history) > 0 && session.history[0].Role == schemas.ModelChatMessageRoleSystem {
+ systemPrompt := session.history[0]
+ session.history = []schemas.BifrostMessage{systemPrompt}
+ } else {
+ session.history = []schemas.BifrostMessage{}
+ }
fmt.Println("🧹 Conversation history cleared!")📝 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.
| systemPrompt := session.history[0] // Assuming first message is system | |
| session.history = []schemas.BifrostMessage{systemPrompt} | |
| fmt.Println("🧹 Conversation history cleared!") | |
| case "/clear": | |
| // Keep system prompt but clear conversation history | |
| if len(session.history) > 0 && session.history[0].Role == schemas.ModelChatMessageRoleSystem { | |
| systemPrompt := session.history[0] | |
| session.history = []schemas.BifrostMessage{systemPrompt} | |
| } else { | |
| session.history = []schemas.BifrostMessage{} | |
| } | |
| fmt.Println("🧹 Conversation history cleared!") |
🤖 Prompt for AI Agents
In plugins/mcp/chat/chatbot.go around lines 579 to 581, the code assumes
session.history has at least one element when accessing session.history[0],
which can cause a panic if the slice is empty. Add a check to ensure
session.history is not empty before accessing the first element. If it is empty,
handle that case appropriately, such as skipping the reset or initializing with
a default system prompt.
Pratham-Mishra04
force-pushed
the
06-17-feat_mcp_plugin_added
branch
from
03e6be1 to
d67cb2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
plugins/mcp/chat/chatbot.go (3)
421-421: Deprecatedstrings.Title– see earlier review; still unresolved.
434-435: Check error fromCleanupSwallowing the error masks failures in terminating external MCP processes.
- s.mcpPlugin.Cleanup() + if err := s.mcpPlugin.Cleanup(); err != nil { + fmt.Printf("Warning: MCP plugin cleanup failed: %v\n", err) + }
581-584: Potential slice-bounds panic on/clearWhen the user clears history at the very start of a session,
session.history[0]will panic.Guard the access:
- systemPrompt := session.history[0] - session.history = []schemas.BifrostMessage{systemPrompt} + if len(session.history) > 0 && session.history[0].Role == schemas.ModelChatMessageRoleSystem { + session.history = []schemas.BifrostMessage{session.history[0]} + } else { + session.history = nil + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
plugins/mcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
core/schemas/bifrost.go(3 hunks)plugins/mcp/chat/chatbot.go(1 hunks)plugins/mcp/go.mod(1 hunks)plugins/mcp/main.go(1 hunks)plugins/mcp/plugin_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
plugins/mcp/plugin_test.go
590-590: Error return value of plugin.Cleanup is not checked
(errcheck)
687-687: Error return value of plugin.Cleanup is not checked
(errcheck)
174-174: func printChoicesAsJSON is unused
(unused)
plugins/mcp/chat/chatbot.go
434-434: Error return value of s.mcpPlugin.Cleanup is not checked
(errcheck)
421-421: SA1019: strings.Title has been deprecated since Go 1.18 and an alternative has been available since Go 1.0: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead.
(staticcheck)
plugins/mcp/main.go
908-908: Error return value of client.StdioCommand.Process.Kill is not checked
(errcheck)
909-909: Error return value of client.StdioCommand.Wait is not checked
(errcheck)
🔇 Additional comments (1)
plugins/mcp/go.mod (1)
3-4: Verify required Go tool-chain version
go 1.24.1is not released yet. Building in CI with the latest stable (currently 1.22.x) will fail.Confirm the minimum version actually needed; otherwise downgrade the directive to the highest released version.
| func printChoicesAsJSON(t *testing.T, choices []schemas.BifrostResponseChoice) { | ||
| t.Helper() | ||
| jsonData, err := json.MarshalIndent(choices, "", " ") | ||
| if err != nil { | ||
| t.Errorf("Failed to marshal choices to JSON: %v", err) | ||
| return | ||
| } | ||
| fmt.Printf("--- Response Choices (JSON) ---\n%s\n-----------------------------\n", string(jsonData)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove dead code
printChoicesAsJSON is never invoked and is flagged by golangci-lint.
Delete it to keep the test file tidy.
🧰 Tools
🪛 golangci-lint (1.64.8)
174-174: func printChoicesAsJSON is unused
(unused)
🤖 Prompt for AI Agents
In plugins/mcp/plugin_test.go around lines 174 to 182, the function
printChoicesAsJSON is defined but never used, causing a lint warning. Remove the
entire printChoicesAsJSON function to eliminate dead code and keep the test file
clean.
| plugin.Cleanup() | ||
| }) |
There was a problem hiding this comment.
Handle Cleanup errors
plugin.Cleanup() returns an error that is currently ignored; this hides failures such as orphaned child processes.
- plugin.Cleanup()
+ if err := plugin.Cleanup(); err != nil {
+ t.Errorf("plugin cleanup failed: %v", err)
+ }Also applies to: 687-688
🧰 Tools
🪛 golangci-lint (1.64.8)
590-590: Error return value of plugin.Cleanup is not checked
(errcheck)
🤖 Prompt for AI Agents
In plugins/mcp/plugin_test.go at lines 590-591 and similarly at 687-688, the
call to plugin.Cleanup() ignores the returned error, which can hide failures
like orphaned child processes. Modify the code to capture the error returned by
plugin.Cleanup() and handle it appropriately, such as logging the error or
failing the test if cleanup fails, to ensure any issues during cleanup are not
silently ignored.
| // Clean up STDIO commands | ||
| for _, client := range p.clientMap { | ||
| if client.StdioCommand != nil && client.StdioCommand.Process != nil { | ||
| fmt.Printf("Terminating STDIO process: %d\n", client.StdioCommand.Process.Pid) | ||
| client.StdioCommand.Process.Kill() | ||
| client.StdioCommand.Wait() // Wait for cleanup | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate errors from process termination
Process.Kill() and cmd.Wait() return errors that should be surfaced to avoid zombie processes.
- client.StdioCommand.Process.Kill()
- client.StdioCommand.Wait() // Wait for cleanup
+ if err := client.StdioCommand.Process.Kill(); err != nil {
+ fmt.Printf("failed to kill process %d: %v\n", client.StdioCommand.Process.Pid, err)
+ }
+ if err := client.StdioCommand.Wait(); err != nil {
+ fmt.Printf("process %d did not exit cleanly: %v\n", client.StdioCommand.Process.Pid, err)
+ }📝 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.
| // Clean up STDIO commands | |
| for _, client := range p.clientMap { | |
| if client.StdioCommand != nil && client.StdioCommand.Process != nil { | |
| fmt.Printf("Terminating STDIO process: %d\n", client.StdioCommand.Process.Pid) | |
| client.StdioCommand.Process.Kill() | |
| client.StdioCommand.Wait() // Wait for cleanup | |
| } | |
| // Clean up STDIO commands | |
| for _, client := range p.clientMap { | |
| if client.StdioCommand != nil && client.StdioCommand.Process != nil { | |
| fmt.Printf("Terminating STDIO process: %d\n", client.StdioCommand.Process.Pid) | |
| - client.StdioCommand.Process.Kill() | |
| - client.StdioCommand.Wait() // Wait for cleanup | |
| + if err := client.StdioCommand.Process.Kill(); err != nil { | |
| + fmt.Printf("failed to kill process %d: %v\n", client.StdioCommand.Process.Pid, err) | |
| + } | |
| + if err := client.StdioCommand.Wait(); err != nil { | |
| + fmt.Printf("process %d did not exit cleanly: %v\n", client.StdioCommand.Process.Pid, err) | |
| + } | |
| } | |
| } |
🧰 Tools
🪛 golangci-lint (1.64.8)
908-908: Error return value of client.StdioCommand.Process.Kill is not checked
(errcheck)
909-909: Error return value of client.StdioCommand.Wait is not checked
(errcheck)
🤖 Prompt for AI Agents
In plugins/mcp/main.go around lines 904 to 910, the calls to Process.Kill() and
cmd.Wait() do not handle or propagate errors, which can lead to unnoticed
failures and potential zombie processes. Modify the code to capture and handle
the errors returned by Process.Kill() and Wait(), logging or returning these
errors appropriately to ensure any issues during process termination are
surfaced and managed.
d67cb2a to
bbf99c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
core/schemas/bifrost.go (2)
72-73: Avoid pointer-to-slice; plain slice already nil-able
*[]Tooladds an unnecessary level of indirection and complicates marshal/unmarshal logic.
Use[]Toolinstead.
407-414: Same pointer-to-slice issue forPendingMCPToolsMake the field a plain slice:
- PendingMCPTools *[]PendingMCPTool `json:"pending_mcp_tools,omitempty"` + PendingMCPTools []PendingMCPTool `json:"pending_mcp_tools,omitempty"`plugins/mcp/chat/chatbot.go (2)
433-435: HandleCleanuperror
mcpPlugin.Cleanup()returns an error that is silently ignored—failures (e.g., zombie processes) will go unnoticed.- s.mcpPlugin.Cleanup() + if err := s.mcpPlugin.Cleanup(); err != nil { + fmt.Printf("Warning: MCP cleanup failed: %v\n", err) + }
582-583: Potential panic when history is empty
session.history[0]assumes at least one element. Guard against empty slice:if len(session.history) > 0 && session.history[0].Role == schemas.ModelChatMessageRoleSystem { systemPrompt := session.history[0] session.history = []schemas.BifrostMessage{systemPrompt} } else { session.history = nil }plugins/mcp/main.go (1)
991-995: Check errors fromProcess.Kill/WaitIgnoring these errors can leave orphaned processes:
- client.StdioCommand.Process.Kill() - client.StdioCommand.Wait() +if err := client.StdioCommand.Process.Kill(); err != nil { + p.logger.Warn(fmt.Sprintf("%s failed to kill process %d: %v", LogPrefix, client.StdioCommand.Process.Pid, err)) +} +if err := client.StdioCommand.Wait(); err != nil { + p.logger.Warn(fmt.Sprintf("%s process %d did not exit cleanly: %v", LogPrefix, client.StdioCommand.Process.Pid, err)) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
plugins/mcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
core/schemas/bifrost.go(3 hunks)plugins/mcp/chat/chatbot.go(1 hunks)plugins/mcp/go.mod(1 hunks)plugins/mcp/main.go(1 hunks)plugins/mcp/plugin_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
plugins/mcp/plugin_test.go
174-174: func printChoicesAsJSON is unused
(unused)
plugins/mcp/main.go
993-993: Error return value of client.StdioCommand.Process.Kill is not checked
(errcheck)
994-994: Error return value of client.StdioCommand.Wait is not checked
(errcheck)
plugins/mcp/chat/chatbot.go
434-434: Error return value of s.mcpPlugin.Cleanup is not checked
(errcheck)
421-421: SA1019: strings.Title has been deprecated since Go 1.18 and an alternative has been available since Go 1.0: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead.
(staticcheck)
🔇 Additional comments (1)
plugins/mcp/go.mod (1)
3-3: Unreleased Go version declared
go 1.24.1is not a publicly-released toolchain; using it will breakgo mod tidyand CI on all standard runners.
Pin to the latest stable major (1.22) or omit the patch suffix.⛔ Skipped due to learnings
Learnt from: Pratham-Mishra04 PR: maximhq/bifrost#81 File: tests/core-providers/go.mod:3-4 Timestamp: 2025-06-16T03:55:16.949Z Learning: Go 1.24 was released in February 2025 and is stable and available for use in go.mod files.Learnt from: Pratham-Mishra04 PR: maximhq/bifrost#81 File: tests/core-providers/go.mod:3-4 Timestamp: 2025-06-16T04:27:53.538Z Learning: In Go module files, `go 1.24.1` (with patch version) can work fine in some setups, contrary to the general rule that go directives should only include major.minor versions.
| gopkg.in/yaml.v3 v3.0.1 // indirect | ||
| ) | ||
|
|
||
| replace github.com/maximhq/bifrost/core => ../../core |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Local replace blocks external consumers
replace github.com/maximhq/bifrost/core => ../../core prevents downstream users from go get-ing the module.
Keep the replace only in a private development overlay (e.g. go.work) and remove it from committed go.mod.
🤖 Prompt for AI Agents
In plugins/mcp/go.mod at line 62, the local replace directive pointing
github.com/maximhq/bifrost/core to a relative path ../../core should be removed
from the committed go.mod file because it blocks external consumers from
fetching the module. Instead, move this replace directive to a private
development overlay file such as go.work to keep local development convenience
without affecting downstream users.
| // Helper function to print choices as JSON | ||
| func printChoicesAsJSON(t *testing.T, choices []schemas.BifrostResponseChoice) { | ||
| t.Helper() | ||
| jsonData, err := json.MarshalIndent(choices, "", " ") | ||
| if err != nil { | ||
| t.Errorf("Failed to marshal choices to JSON: %v", err) | ||
| return | ||
| } | ||
| fmt.Printf("--- Response Choices (JSON) ---\n%s\n-----------------------------\n", string(jsonData)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove dead helper printChoicesAsJSON
The function is never called; it triggers unused lint warnings.
Delete it or invoke it from the tests.
🧰 Tools
🪛 golangci-lint (1.64.8)
174-174: func printChoicesAsJSON is unused
(unused)
🤖 Prompt for AI Agents
In plugins/mcp/plugin_test.go around lines 173 to 182, the helper function
printChoicesAsJSON is defined but never used, causing unused lint warnings. To
fix this, either remove the entire printChoicesAsJSON function if it is not
needed, or add calls to this function in relevant test cases where printing
choices as JSON would be helpful for debugging or output verification.
bbf99c3 to
876358a
Compare
Pratham-Mishra04
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
plugins/mcp/go.mod (1)
62-62: Remove the committed localreplaceThe
replace github.com/maximhq/bifrost/core => ../../coredirective blocks external users fromgo get-ing the module.
Move it to a privatego.work/go.mod.localinstead.core/schemas/bifrost.go (1)
72-74: Drop pointer-to-slice indirectionBoth
MCPTools *[]ToolandPendingMCPTools *[]PendingMCPTooladd an unnecessary level of indirection—nilvs empty slice is already distinguishable.
Use plain slices to simplify marshaling and call-site code:- MCPTools *[]Tool `json:"mcp_tools,omitempty"` + MCPTools []Tool `json:"mcp_tools,omitempty"` - PendingMCPTools *[]PendingMCPTool `json:"pending_mcp_tools,omitempty"` + PendingMCPTools []PendingMCPTool `json:"pending_mcp_tools,omitempty"`Also applies to: 407-414
plugins/mcp/plugin_test.go (2)
168-170: CheckCleanup()errors
client.Cleanup()returns an error that is silently ignored in multiple places.
Capture and fail the test when cleanup fails to avoid masking resource-leak issues.- client.Cleanup() + if err := client.Cleanup(); err != nil { + t.Errorf("cleanup failed: %v", err) + }Also applies to: 393-396, 497-499, 583-585, 679-681
173-182: Remove dead helperprintChoicesAsJSONThe function is still unused and keeps triggering
unusedlinter warnings.
Delete it or call it from the tests.plugins/mcp/chat/chatbot.go (2)
582-584: Prevent possible slice-bounds panic in/clear
session.history[0]assumes the slice is non-empty.
Guard the access to avoid panics on an empty history.
430-435: Handle cleanup errorsBoth
s.client.Cleanup()ands.mcpPlugin.Cleanup()return errors that are ignored.
Log or propagate them so resource-release failures aren’t missed.if s.client != nil { - s.client.Cleanup() + if err := s.client.Cleanup(); err != nil { + fmt.Printf("warning: client cleanup failed: %v\n", err) + } } if s.mcpPlugin != nil { - s.mcpPlugin.Cleanup() + if err := s.mcpPlugin.Cleanup(); err != nil { + fmt.Printf("warning: MCP plugin cleanup failed: %v\n", err) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
plugins/mcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
core/schemas/bifrost.go(3 hunks)plugins/mcp/chat/chatbot.go(1 hunks)plugins/mcp/go.mod(1 hunks)plugins/mcp/main.go(1 hunks)plugins/mcp/plugin_test.go(1 hunks)plugins/mcp/utils.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
plugins/mcp/plugin_test.go
174-174: func printChoicesAsJSON is unused
(unused)
plugins/mcp/chat/chatbot.go
434-434: Error return value of s.mcpPlugin.Cleanup is not checked
(errcheck)
421-421: SA1019: strings.Title has been deprecated since Go 1.18 and an alternative has been available since Go 1.0: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead.
(staticcheck)
plugins/mcp/main.go
690-690: Error return value of client.StdioCommand.Process.Kill is not checked
(errcheck)
691-691: Error return value of client.StdioCommand.Wait is not checked
(errcheck)
🔇 Additional comments (2)
plugins/mcp/go.mod (1)
3-3: Non-existent Go toolchain version
go 1.24.1targets a Go release that does not (yet) exist.
Stick to the latest stable (e.g.go 1.22) or the minimum version required by the codebase to avoid brokengocommands in CI and downstream consumers.⛔ Skipped due to learnings
Learnt from: Pratham-Mishra04 PR: maximhq/bifrost#81 File: tests/core-providers/go.mod:3-4 Timestamp: 2025-06-16T03:55:16.949Z Learning: Go 1.24 was released in February 2025 and is stable and available for use in go.mod files.plugins/mcp/main.go (1)
529-536: STDIO transport type appears inverted
NewStdioServerTransportWithIOis intended for servers, yet this code builds a client (mcp_golang.NewClientWithInfo).
Using the server-side transport on the client side can swap stdin/stdout directions and break the protocol.Verify the library API; it likely offers
NewStdioClientTransportWithIO(or similar). Replace accordingly:-stdioTransport := stdio.NewStdioServerTransportWithIO(stdout, stdin) +stdioTransport := stdio.NewStdioClientTransportWithIO(stdin, stdout)Failing to correct this will cause the handshake to stall.
| func (p *MCPPlugin) shouldSkipTool(toolName, clientName string) bool { | ||
| // ConnectToExternalMCP function already has the mutex lock | ||
| client, exists := p.clientMap[clientName] | ||
| if !exists { | ||
| return false // Don't skip if client config doesn't exist | ||
| } | ||
|
|
||
| return slices.Contains(client.ExecutionConfig.ToolsToSkip, toolName) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Possible data race on p.clientMap
shouldSkipTool accesses p.clientMap without acquiring the mutex, unlike other helpers that use p.mu.RLock().
If the map is mutated concurrently (e.g. during dynamic client registration) this can panic.
func (p *MCPPlugin) shouldSkipTool(toolName, clientName string) bool {
- client, exists := p.clientMap[clientName]
+ p.mu.RLock()
+ client, exists := p.clientMap[clientName]
+ p.mu.RUnlock()📝 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.
| func (p *MCPPlugin) shouldSkipTool(toolName, clientName string) bool { | |
| // ConnectToExternalMCP function already has the mutex lock | |
| client, exists := p.clientMap[clientName] | |
| if !exists { | |
| return false // Don't skip if client config doesn't exist | |
| } | |
| return slices.Contains(client.ExecutionConfig.ToolsToSkip, toolName) | |
| } | |
| func (p *MCPPlugin) shouldSkipTool(toolName, clientName string) bool { | |
| // ConnectToExternalMCP function already has the mutex lock | |
| p.mu.RLock() | |
| client, exists := p.clientMap[clientName] | |
| p.mu.RUnlock() | |
| if !exists { | |
| return false // Don't skip if client config doesn't exist | |
| } | |
| return slices.Contains(client.ExecutionConfig.ToolsToSkip, toolName) | |
| } |
🤖 Prompt for AI Agents
In plugins/mcp/utils.go around lines 196 to 204, the method shouldSkipTool
accesses the shared map p.clientMap without acquiring the mutex lock, which can
cause a data race and potential panic if the map is modified concurrently. To
fix this, add a read lock using p.mu.RLock() before accessing p.clientMap and
defer p.mu.RUnlock() immediately after acquiring the lock to ensure safe
concurrent access.
| content = strings.Join(textParts, "\n") | ||
| } | ||
|
|
||
| role := strings.Title(string(msg.Role)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated strings.Title
strings.Title is deprecated. Use cases.Title from golang.org/x/text/cases instead.
import "golang.org/x/text/cases"
import "golang.org/x/text/language"
caser := cases.Title(language.English)
role := caser.String(string(msg.Role))🧰 Tools
🪛 golangci-lint (1.64.8)
421-421: SA1019: strings.Title has been deprecated since Go 1.18 and an alternative has been available since Go 1.0: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead.
(staticcheck)
🤖 Prompt for AI Agents
In plugins/mcp/chat/chatbot.go at line 421, replace the deprecated strings.Title
function with cases.Title from golang.org/x/text/cases. Import the packages
"golang.org/x/text/cases" and "golang.org/x/text/language", create a caser using
cases.Title(language.English), and then use caser.String(string(msg.Role)) to
get the title-cased role string.
| // Execute the tool | ||
| toolMsg, err := p.executeSingleTool(context.Background(), toolCall) | ||
| if err != nil { | ||
| return nil, &schemas.BifrostResponse{}, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Execution loses caller context
executeSingleTool is invoked with context.Background(); any timeouts, cancellation, or tracing information carried in the incoming ctx are discarded.
-toolMsg, err := p.executeSingleTool(context.Background(), toolCall)
+toolMsg, err := p.executeSingleTool(*ctx, toolCall)Preserving the original context avoids hung tool calls when the request is cancelled upstream.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugins/mcp/main.go around lines 836 to 840, replace the use of
context.Background() in the call to executeSingleTool with the original context
variable (likely named ctx) passed into the function. This preserves
cancellation, timeout, and tracing information from the caller, preventing hung
tool calls when the request is cancelled upstream.
| if client.StdioCommand != nil && client.StdioCommand.Process != nil { | ||
| p.logger.Info(fmt.Sprintf(LogPrefix+" Terminating STDIO process: %d", client.StdioCommand.Process.Pid)) | ||
| client.StdioCommand.Process.Kill() | ||
| client.StdioCommand.Wait() // Wait for cleanup | ||
| } |
There was a problem hiding this comment.
Handle errors from process termination
The return values of Process.Kill() and Wait() are ignored. If either fails you may leave zombie processes.
-client.StdioCommand.Process.Kill()
-client.StdioCommand.Wait()
+if err := client.StdioCommand.Process.Kill(); err != nil {
+ p.logger.Warn(fmt.Sprintf(LogPrefix+" failed to kill process %d: %v", client.StdioCommand.Process.Pid, err))
+}
+if err := client.StdioCommand.Wait(); err != nil {
+ p.logger.Warn(fmt.Sprintf(LogPrefix+" process %d did not exit cleanly: %v", client.StdioCommand.Process.Pid, err))
+}This also satisfies errcheck linter warnings.
🧰 Tools
🪛 golangci-lint (1.64.8)
690-690: Error return value of client.StdioCommand.Process.Kill is not checked
(errcheck)
691-691: Error return value of client.StdioCommand.Wait is not checked
(errcheck)
🤖 Prompt for AI Agents
In plugins/mcp/main.go around lines 688 to 692, the calls to Process.Kill() and
Wait() ignore their returned errors, which can lead to zombie processes if
termination fails. Modify the code to capture and handle errors returned by both
Kill() and Wait(), logging any failures appropriately to ensure proper process
cleanup and satisfy the errcheck linter.
| go func() { | ||
| if err := p.server.Serve(); err != nil && err != http.ErrServerClosed { | ||
| p.logger.Error(fmt.Errorf(LogPrefix+" MCP server error: %w", err)) | ||
| p.mu.Lock() | ||
| p.serverRunning = false | ||
| p.mu.Unlock() | ||
| } | ||
| }() | ||
|
|
||
| // Mark server as running | ||
| p.serverRunning = true | ||
|
|
||
| // Initialize the client connection to the server | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| if _, ok := p.clientMap[BifrostClientKey]; !ok { | ||
| return fmt.Errorf("bifrost client not found") | ||
| } | ||
|
|
||
| _, err := p.clientMap[BifrostClientKey].Conn.Initialize(ctx) | ||
| if err != nil { | ||
| p.serverRunning = false | ||
| return fmt.Errorf("failed to initialize MCP client: %v", err) | ||
| } |
There was a problem hiding this comment.
Server keeps running when client init fails – shut it down on error
serverRunning is set to true before the client is initialised.
If the Initialize call (lines 303-306) fails, the flag is flipped back to false, but the goroutine started at 283 is still serving, leaving an orphaned HTTP server.
- go func() {
- if err := p.server.Serve(); err != nil && err != http.ErrServerClosed {
- ...
- }
- }()
- p.serverRunning = true
+ go func() {
+ if err := p.server.Serve(); err != nil && err != http.ErrServerClosed {
+ p.logger.Error(fmt.Errorf(LogPrefix+" MCP server error: %w", err))
+ }
+ }()
+
+ p.serverRunning = true
...
if _, ok := p.clientMap[BifrostClientKey]; !ok {
return fmt.Errorf("bifrost client not found")
}
_, err := p.clientMap[BifrostClientKey].Conn.Initialize(ctx)
if err != nil {
- p.serverRunning = false
+ p.serverRunning = false
+ _ = p.server.Shutdown(ctx) // ensure goroutine terminates
return fmt.Errorf("failed to initialize MCP client: %v", err)
}This prevents a leaked goroutine and a port left listening on failure.
📝 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.
| go func() { | |
| if err := p.server.Serve(); err != nil && err != http.ErrServerClosed { | |
| p.logger.Error(fmt.Errorf(LogPrefix+" MCP server error: %w", err)) | |
| p.mu.Lock() | |
| p.serverRunning = false | |
| p.mu.Unlock() | |
| } | |
| }() | |
| // Mark server as running | |
| p.serverRunning = true | |
| // Initialize the client connection to the server | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| if _, ok := p.clientMap[BifrostClientKey]; !ok { | |
| return fmt.Errorf("bifrost client not found") | |
| } | |
| _, err := p.clientMap[BifrostClientKey].Conn.Initialize(ctx) | |
| if err != nil { | |
| p.serverRunning = false | |
| return fmt.Errorf("failed to initialize MCP client: %v", err) | |
| } | |
| go func() { | |
| if err := p.server.Serve(); err != nil && err != http.ErrServerClosed { | |
| p.logger.Error(fmt.Errorf(LogPrefix+" MCP server error: %w", err)) | |
| } | |
| }() | |
| p.serverRunning = true | |
| // Initialize the client connection to the server | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| if _, ok := p.clientMap[BifrostClientKey]; !ok { | |
| return fmt.Errorf("bifrost client not found") | |
| } | |
| _, err := p.clientMap[BifrostClientKey].Conn.Initialize(ctx) | |
| if err != nil { | |
| p.serverRunning = false | |
| _ = p.server.Shutdown(ctx) // ensure the server goroutine exits | |
| return fmt.Errorf("failed to initialize MCP client: %v", err) | |
| } |
🤖 Prompt for AI Agents
In plugins/mcp/main.go around lines 283 to 307, the serverRunning flag is set to
true before the client initialization, but if initialization fails, the server
goroutine continues running, causing a resource leak. To fix this, move the
assignment of serverRunning = true to after the client initialization succeeds.
Additionally, if initialization fails, ensure the server is properly shut down
or stopped to prevent the goroutine from leaking and the port from remaining
open.
876358a to
6aacbb1
Compare
Pratham-Mishra04
force-pushed
the
06-17-feat_mcp_plugin_added
branch
from
6aacbb1 to
881a6be
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (8)
plugins/mcp/go.mod (1)
62-62: Local replace blocks external consumersplugins/mcp/utils.go (1)
196-204: Possible data race onp.clientMapplugins/mcp/plugin_test.go (1)
174-182: Remove dead codeplugins/mcp/chat/chatbot.go (3)
421-421: Replace deprecatedstrings.Title.
Usecases.Titlefromgolang.org/x/text/casesas previously suggested.
581-583: Guard against empty history before indexing.
Accessingsession.history[0]panics when history is empty; add a length check.
428-435: Handle errors fromCleanup()calls.
Check and log/propagate errors returned by boths.client.Cleanup()ands.mcpPlugin.Cleanup().plugins/mcp/main.go (2)
837-838: Preserve caller context when executing tools.
executeSingleToolis still invoked withcontext.Background(), discarding cancellation & deadlines. Pass the incoming context instead:- toolMsg, err := p.executeSingleTool(context.Background(), toolCall) + toolMsg, err := p.executeSingleTool(*ctx, toolCall)
686-692: Handle errors from process termination to avoid zombie processes.Return values of
Process.KillandWaitremain unchecked; propagate or log them as previously suggested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
plugins/mcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
core/schemas/bifrost.go(3 hunks)plugins/mcp/README.md(1 hunks)plugins/mcp/chat/chatbot.go(1 hunks)plugins/mcp/go.mod(1 hunks)plugins/mcp/main.go(1 hunks)plugins/mcp/plugin_test.go(1 hunks)plugins/mcp/utils.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/mcp/README.md
[uncategorized] ~170-~170: Loose punctuation mark.
Context: ...ToolExecutionPolicyRequireApproval: Tool requires user approval before exec...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~491-~491: Possible typo: you repeated a word.
Context: ...ry**: Only applies when include list is empty 3. Empty filters: All configured clients are a...
(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~533-~533: Possible subject-verb agreement error detected: Did you mean to use the plural form here?
Context: ...ContextKeyExcludeClients`) - Applied if no include list 3. **Config-level tool whi...
(SUBJECT_NUMBER)
[duplication] ~848-~848: Possible typo: you repeated a word.
Context: ...** - Tool results are sent back to the LLM - LLM synthesizes results into natural langua...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~1049-~1049: Possible typo: you repeated a word.
Context: ...ew ToolsToSkip configuration for each client - Client connection issues: Verify external MC...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
plugins/mcp/README.md
16-16: Link fragments should be valid
null
(MD051, link-fragments)
303-303: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
335-335: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
813-813: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
825-825: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
834-834: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
842-842: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
861-861: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
877-877: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
886-886: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
897-897: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
986-986: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
999-999: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1014-1014: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1028-1028: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1042-1042: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1064-1064: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1076-1076: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 golangci-lint (1.64.8)
plugins/mcp/chat/chatbot.go
16-16: could not import github.com/maximhq/bifrost/plugins/mcp (-: # github.com/maximhq/bifrost/plugins/mcp
./main.go:605:33: invalid operation: cannot indirect req.MCPTools (variable of type []schemas.Tool)
./main.go:813:24: invalid operation: cannot indirect req.MCPTools (variable of type []schemas.Tool)
./main.go:956:37: cannot use &pendingTools (value of type *[]schemas.PendingMCPTool) as []schemas.PendingMCPTool value in assignment)
(typecheck)
239-239: invalid operation: cannot indirect response.ExtraFields.PendingMCPTools (variable of type []schemas.PendingMCPTool)
(typecheck)
269-269: invalid operation: cannot indirect response.ExtraFields.PendingMCPTools (variable of type []schemas.PendingMCPTool)
(typecheck)
340-340: cannot use &approvedTools (value of type *[]schemas.Tool) as []schemas.Tool value in struct literal
(typecheck)
plugins/mcp/main.go
1-1: : # github.com/maximhq/bifrost/plugins/mcp [github.com/maximhq/bifrost/plugins/mcp.test]
./main.go:605:33: invalid operation: cannot indirect req.MCPTools (variable of type []schemas.Tool)
./main.go:813:24: invalid operation: cannot indirect req.MCPTools (variable of type []schemas.Tool)
./main.go:956:37: cannot use &pendingTools (value of type *[]schemas.PendingMCPTool) as []schemas.PendingMCPTool value in assignment
(typecheck)
🔇 Additional comments (2)
core/schemas/bifrost.go (1)
72-73: Schema additions look good!The MCP-related schema additions are well-structured:
MCPToolsinBifrostRequestfor approved toolsPendingMCPToolsin response for tools awaiting approvalPendingMCPToolstruct properly captures tool metadataGood choice using plain slices instead of pointer-to-slice.
Also applies to: 413-413, 439-444
plugins/mcp/plugin_test.go (1)
1-684: Comprehensive test coverage!The tests effectively cover:
- Local tool registration and execution
- External MCP server integration (Context7, Maxim)
- Both safe and agentic execution modes
- Proper cleanup and error handling
Well-structured test suite.
| @@ -0,0 +1,62 @@ | |||
| module github.com/maximhq/bifrost/plugins/mcp | |||
|
|
|||
| go 1.24.1 | |||
There was a problem hiding this comment.
Fix invalid Go version.
Go version 1.24.1 is invalid. Go uses two-part version numbers (e.g., 1.21, 1.22, 1.23).
Apply this diff to fix the Go version:
-go 1.24.1
+go 1.23📝 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.
| go 1.24.1 | |
| go 1.23 |
🤖 Prompt for AI Agents
In plugins/mcp/go.mod at line 3, the Go version is incorrectly specified as
1.24.1, which is invalid because Go versions use only two parts. Change the
version to a valid two-part version number such as 1.24 or the latest supported
version without the patch number.
|
|
||
| ### Plugin Architecture | ||
|
|
||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add language specifiers to fenced code blocks.
Code blocks should specify a language for proper syntax highlighting.
For the diagram blocks, you can use:
-```
+```textAlso applies to: 335-335
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
303-303: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In plugins/mcp/README.md at lines 303 and 335, the fenced code blocks lack
language specifiers, which are needed for proper syntax highlighting. Update
these code blocks by adding the appropriate language identifier, such as "text"
for diagram blocks, immediately after the opening triple backticks.
|
|
||
| #### Key Components of Safe Execution | ||
|
|
||
| **1. Pending Tools Detection** |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use proper headings instead of bold emphasis.
Several sections use bold text (text) instead of proper markdown headings.
Convert these to proper headings. For example:
-**1. Pending Tools Detection**
+##### 1. Pending Tools DetectionAlso applies to: 825-825, 834-834, 842-842, 861-861, 877-877, 886-886, 897-897
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
813-813: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In plugins/mcp/README.md at lines 813, 825, 834, 842, 861, 877, 886, and 897,
replace the bold text used for section titles with proper markdown headings by
prefixing the text with one or more '#' characters according to the heading
level desired. This will improve the document structure and readability.
| - [API Reference](#api-reference) | ||
| - [Advanced Features](#advanced-features) | ||
| - [Troubleshooting](#troubleshooting) | ||
| - [Contributing](#contributing) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix invalid link fragment.
The table of contents links to #contributing but there's no corresponding section in the document.
Either add a Contributing section or remove this link:
-- [Contributing](#contributing)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
16-16: Link fragments should be valid
null
(MD051, link-fragments)
🤖 Prompt for AI Agents
In plugins/mcp/README.md at line 16, the table of contents contains a link to a
non-existent Contributing section (#contributing). To fix this, either add a
Contributing section with relevant content below the table of contents or remove
the link from the table of contents to avoid broken navigation.
| // Check if response contains pending tools (requiring user approval) | ||
| if response.ExtraFields.PendingMCPTools != nil && len(*response.ExtraFields.PendingMCPTools) > 0 { | ||
| return s.handlePendingTools(response) | ||
| } |
There was a problem hiding this comment.
Fix invalid pointer dereference – PendingMCPTools is a slice, not *[].
Dereferencing response.ExtraFields.PendingMCPTools leads to a compile-time “invalid operation: cannot indirect” error.
Use the slice directly:
-if response.ExtraFields.PendingMCPTools != nil && len(*response.ExtraFields.PendingMCPTools) > 0 {
+if len(response.ExtraFields.PendingMCPTools) > 0 {
return s.handlePendingTools(response)
}
...
-pendingTools := *response.ExtraFields.PendingMCPTools
+pendingTools := response.ExtraFields.PendingMCPToolsAlso applies to: 269-270
🧰 Tools
🪛 golangci-lint (1.64.8)
239-239: invalid operation: cannot indirect response.ExtraFields.PendingMCPTools (variable of type []schemas.PendingMCPTool)
(typecheck)
🤖 Prompt for AI Agents
In plugins/mcp/chat/chatbot.go around lines 238 to 241 and also lines 269 to
270, the code incorrectly dereferences PendingMCPTools as a pointer to a slice,
but it is already a slice type. Remove the pointer dereference operator (*) and
use PendingMCPTools directly when checking its length and nil status to fix the
invalid pointer dereference compile error.
| // Create new request with approved tools | ||
| approvedRequest := &schemas.BifrostRequest{ | ||
| Provider: s.config.Provider, | ||
| Model: s.config.Model, | ||
| Input: schemas.RequestInput{ | ||
| ChatCompletionInput: &conversationForApproval, | ||
| }, | ||
| MCPTools: &approvedTools, | ||
| } |
There was a problem hiding this comment.
Remove superfluous & – MCPTools field expects a slice value.
Passing &approvedTools (pointer) breaks type-checking. Assign the slice directly:
- approvedRequest := &schemas.BifrostRequest{
...
- MCPTools: &approvedTools,
+ approvedRequest := &schemas.BifrostRequest{
...
+ MCPTools: approvedTools,
}📝 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.
| // Create new request with approved tools | |
| approvedRequest := &schemas.BifrostRequest{ | |
| Provider: s.config.Provider, | |
| Model: s.config.Model, | |
| Input: schemas.RequestInput{ | |
| ChatCompletionInput: &conversationForApproval, | |
| }, | |
| MCPTools: &approvedTools, | |
| } | |
| // Create new request with approved tools | |
| approvedRequest := &schemas.BifrostRequest{ | |
| Provider: s.config.Provider, | |
| Model: s.config.Model, | |
| Input: schemas.RequestInput{ | |
| ChatCompletionInput: &conversationForApproval, | |
| }, | |
| MCPTools: approvedTools, | |
| } |
🧰 Tools
🪛 golangci-lint (1.64.8)
340-340: cannot use &approvedTools (value of type *[]schemas.Tool) as []schemas.Tool value in struct literal
(typecheck)
🤖 Prompt for AI Agents
In plugins/mcp/chat/chatbot.go around lines 333 to 341, the MCPTools field is
incorrectly assigned a pointer to the approvedTools slice using &approvedTools,
but it expects a slice value directly. Remove the ampersand and assign
approvedTools directly to MCPTools to fix the type mismatch.
| func (p *MCPPlugin) PreHook(ctx *context.Context, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.BifrostResponse, error) { | ||
| // Check if this is an approved tool execution request | ||
| if req.MCPTools != nil && len(*req.MCPTools) > 0 && req.Input.ChatCompletionInput != nil { | ||
| return p.handleApprovedTools(ctx, req) | ||
| } |
There was a problem hiding this comment.
Correct slice vs pointer misuse for MCPTools and PendingMCPTools.
All three spots treat slice fields as *[], causing type-check failures.
- if req.MCPTools != nil && len(*req.MCPTools) > 0 {
+ if len(req.MCPTools) > 0 {
return p.handleApprovedTools(ctx, req)
}
...
- approvedTools := make(map[string]schemas.Tool)
- for _, tool := range *req.MCPTools {
+ approvedTools := make(map[string]schemas.Tool)
+ for _, tool := range req.MCPTools {
approvedTools[tool.Function.Name] = tool
}
...
- res.ExtraFields.PendingMCPTools = &pendingTools
+ res.ExtraFields.PendingMCPTools = pendingToolsThese changes resolve the invalid operation: cannot indirect compile errors reported by golangci-lint.
Also applies to: 811-816, 954-957
🤖 Prompt for AI Agents
In plugins/mcp/main.go around lines 603 to 607, the code incorrectly treats the
MCPTools slice as a pointer to a slice (*[]), causing compile errors. To fix
this, change all occurrences of MCPTools and PendingMCPTools from pointer types
to direct slice types ([]), and update the code to access them accordingly
without dereferencing pointers. Apply the same correction to the code sections
at lines 811-816 and 954-957 to ensure consistent and correct slice usage.
| // Mark server as running | ||
| p.serverRunning = true | ||
|
|
||
| // Initialize the client connection to the server | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| if _, ok := p.clientMap[BifrostClientKey]; !ok { | ||
| return fmt.Errorf("bifrost client not found") | ||
| } | ||
|
|
||
| _, err := p.clientMap[BifrostClientKey].Conn.Initialize(ctx) | ||
| if err != nil { | ||
| p.serverRunning = false | ||
| return fmt.Errorf("failed to initialize MCP client: %v", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid server goroutine leak on client-init failure.
serverRunning is set to true before client initialization; if initialization fails the HTTP server continues running. Move the flag assignment after successful init and call p.server.Shutdown(ctx) on error.
🤖 Prompt for AI Agents
In plugins/mcp/main.go around lines 292 to 306, the p.serverRunning flag is set
to true before client initialization, which can cause the server to keep running
if initialization fails. To fix this, move the assignment of p.serverRunning =
true to after the client initialization succeeds, and on initialization error,
call p.server.Shutdown(ctx) to properly stop the server and avoid goroutine
leaks.
1 participant
Add MCP Safe Mode and Agentic Mode Support
This PR adds support for Model-Controlled Programs (MCP) with two key modes:
Safe Mode: Requires user approval before executing MCP tools. When enabled, tool calls are presented to the user for approval before execution.
Agentic Mode: Enables a more sophisticated conversation flow where tool results are sent back to the LLM for synthesis into a comprehensive response.
Key Changes:
MCPToolsfield toBifrostRequestto support approved tools in safe modePendingMCPToolsfield toBifrostResponseExtraFieldsto store tools awaiting approvalPendingMCPToolstruct to represent tools requiring user approvalThe implementation allows for a more secure and interactive experience when using MCP tools, giving users control over tool execution while maintaining a natural conversation flow.