feat: extend mcp plugin interface for list tools, ping and connections

[Pratham-Mishra04]

Summary

Briefly explain the purpose of this PR and the problem it solves.

Changes

• What was changed and why
• Any notable design decisions or trade-offs

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

Describe the steps to validate this change. Include commands and expected outcomes.

# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build

If adding new configs or environment variables, document them here.

Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

Breaking changes

• Yes
• No

If yes, describe impact and migration instructions.

Related issues

Link related issues and discussions. Example: Closes #123

Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Pratham-Mishra04]

• feat: extend mcp plugin interface for list tools, ping and connections #3389 👈 (View in Graphite)
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Typed MCP connection hooks (pre/post) for plugins and a unified MCP plugin pipeline for connect/list_tools/ping flows.
  • Manager-level single-tool execution endpoints so agent tool calls run through the plugin gate.

• Bug Fixes

  • Unified error-string formatting for clearer MCP error logs and improved health-monitor latency reporting.

• Tests

  • Large suite of new and updated tests exercising MCP connect/list_tools/ping hooks and agent-mode tool execution wiring.

• Style

  • Minor UI help-text clarification for External Base URLs.

Walkthrough

This PR centralizes MCP tool execution and connection lifecycle behind a plugin pipeline gate: it adds typed MCP envelopes/hooks (connect/ping/list_tools/execute_tool), moves plugin pipeline lifecycle ownership to ClientManager/MCPManager, implements gate wrappers for pre/post hooks and short-circuiting, refactors client connect/OAuth/tool discovery through the gate, and updates tests to the new agent-mode contract.

Changes

MCP Plugin Gate Architecture and Lifecycle Hooks

Layer / File(s)	Summary
Schema Types: Typed MCP Operations and Connection Lifecycle core/schemas/bifrost.go, core/schemas/plugin_native.go, core/schemas/trace.go, core/utils.go	Introduces typed Connect/Ping/ListTools/ExecuteTool envelopes and response types; adds BifrostMCPConnectResponse and server metadata; adds MCPRequestType and MCPRequestType.IsExecuteTool(); adds SpanKindMCPClient; centralizes error string logic via BifrostError.GetErrorString() and deprecates GetErrorMessage.
Plugin Interfaces: Connection Lifecycle Hooks and Pipeline Methods core/schemas/plugin.go, core/mcp/toolmanager.go	Adds MCPConnectionPlugin with typed PreMCPConnectionHook/PostMCPConnectionHook and MCPConnectionShortCircuit; provides MCPPluginNoOpHooks; extends ClientManager with GetPluginPipeline/ReleasePluginPipeline; extends PluginPipeline with RunMCPPreConnectionHooks/RunMCPPostConnectionHooks.
MCP Gate Orchestration core/mcp/pluginpipeline.go	Implements runWithPluginPipeline and runConnectWithPluginPipeline to coordinate pre/post hooks, short-circuiting, tracing, error wrapping, and plugin-log draining; adds runListToolsWithHooks and ClientHealthMonitor.runPingWithHooks.
Tool Execution: Request Pool and Gate Handlers core/mcp/exec.go	Adds sync.Pool-backed MCP request reuse and helper reset functions; implements executeToolWithHooks and executeToolForAgent; exposes ExecuteChatTool and ExecuteResponsesTool for single tool calls via the manager.
MCPManager: Pipeline Lifecycle and Agent Mode Callback Removal core/mcp/mcp.go	Stores plugin-pipeline provider/release callbacks on MCPManager, exposes GetPluginPipeline/ReleasePluginPipeline, routes agent-mode tool execution through internal executeToolForAgent, and removes the external executeTool callback parameter from agent handlers.
Interfaces and ToolsManager: Constructor and Dependency Updates core/mcp/interface.go, core/mcp/toolmanager.go	Updates MCPManagerInterface to add ExecuteChatTool/ExecuteResponsesTool and remove injected executeTool from agent handlers; removes pipeline provider/release from ToolsManager constructors and removes SetPluginPipeline; ExecuteAgentFor* now defaults nil executors to m.ExecuteTool.
Tool Discovery: Detailed Results and Hook Wrapping core/mcp/utils.go, core/mcp/toolsync.go, core/mcp/healthmonitor.go	Replaces retrieveExternalTools with retrieveExternalToolsDetailed returning listToolsResult (tools, mapping, raw count, skipped); uses runListToolsWithHooks for discovery and liveness checks; health monitor uses runPingWithHooks or runListToolsWithHooks.
Connection and OAuth: Plugin-Gated Establishment core/mcp/clientmanager.go	connectToMCPClient and VerifyPerUserOAuthConnection now build BifrostMCPConnectRequest, strip Authorization for pre-hooks, reinject token for transport start, run transport initialize inside the connect gate, and use runListToolsWithHooks for tool discovery; transport creators accept plugin-provided overrides.
CodeMode: Pipeline Sourcing via ClientManager core/mcp/codemode.go, core/mcp/codemode/starlark/starlark.go, core/mcp/codemode/starlark/executecode.go	Removes pipeline provider/release from CodeModeDependencies and StarlarkCodeMode; nested tool calls obtain/release pipelines via ClientManager.GetPluginPipeline/ReleasePluginPipeline.
Bifrost: Delegators and Connection Lifecycle Hook Support core/bifrost.go	Removes local MCP request pooling and pooling helpers; makes ExecuteChatMCPTool/ExecuteResponsesMCPTool thin delegators to MCPManager; updates agent-mode handoffs to the new MCPManager signatures; adds PluginPipeline.RunMCPPreConnectionHooks/RunMCPPostConnectionHooks methods.
Plugin Loading: MCP Connection Hook Symbol Resolution framework/plugins/soloader.go, framework/plugins/soplugin.go	SO plugin loader and DynamicPlugin now optionally load and dispatch typed PreMCPConnectionHook/PostMCPConnectionHook symbols and expose passthrough behavior for legacy plugins.
Built-in Plugins: Tool-Execution Request Type Filtering plugins/governance/main.go, plugins/logging/main.go	Governance and logging plugins now early-return for non-execute_tool MCP envelopes (ping/list/connect), applying logic only for execute-tool requests.
Test Infrastructure: Plugin Test Helpers core/internal/mcptests/test_plugins.go	Extends MCPLogEntry for typed connect captures; updates TestLoggingPlugin with connection hooks; adds TestConnectPlugin, TestPingPlugin, and TestListToolsPlugin to exercise pre/post hook behavior, mutation, and short-circuiting.
Comprehensive MCP Gate Tests core/internal/mcptests/connect_ping_listtools_test.go	Adds 17 tests verifying connect/list_tools/ping hook firing counts, short-circuit behaviors, header stripping, post-hook filtering, health monitor integration, and client-name propagation.
Agent and CodeMode Tests: Callback Removal core/internal/mcptests/agent_*.go, core/internal/mcptests/codemode_*.go, core/internal/mcptests/context_propagation_test.go	Updates ~47 agent and codemode tests to remove the inline execute-tool callback argument from CheckAndExecuteAgentForChatRequest/CheckAndExecuteAgentForResponsesRequest calls and adjusts test wiring/mocks accordingly.
Agent Executor and Test Mocks core/mcp/agent.go, core/mcp/agent_test.go, core/mcp/toolmanager_test.go, core/mcp/codemode/starlark/starlark_test.go	Simplifies auto-executable tool goroutine fallback handling; adds GetPluginPipeline/ReleasePluginPipeline no-op stubs to mock client managers used by tests.
UI Documentation ui/app/workspace/config/views/mcpView.tsx	Minor help-text bolding for External Base URLs instruction.

🎯 4 (Complex) | ⏱️ ~60 minutes

🐰 A gate opens wide, connecting threads,
Typed hooks dance through requests, spreading truth,
Tools now flow through pipelines smooth and keen,
OAuth tokens whisper secrets unseen,
Plugins orchestrate a symphony of care.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 05-11-feat_extend_mcp_plugin_interface_for_list_tools_ping_and_connections

[greptile-apps]

Confidence Score: 5/5

The change is safe to merge; previously reported runtime bugs are addressed and the new gate functions are well-tested.

The previously reported nil-context panic (SSE/STDIO), unconditional ping error return, and nil-dereference on gateErr.Error.Message are all corrected in this PR. The two remaining findings are a misleading trace span status (always reported Ok even on error) and a small dead-code branch in GetErrorString; neither affects correctness or data integrity.

core/mcp/pluginpipeline.go — span status reporting; core/schemas/bifrost.go — GetErrorString dead code

Important Files Changed

Filename	Overview
core/mcp/pluginpipeline.go	New file implementing the unified plugin gate for all MCP ops (ping, list_tools, execute_tool, connect); logic is correct but span status is always reported as Ok even on error paths.
core/mcp/exec.go	New file extracting execute-tool pool management and plugin-gate wiring from bifrost.go into the mcp package; logic is clean and pool reset covers all fields.
core/mcp/clientmanager.go	Connect path refactored to run through the typed MCPConnectionPlugin gate; nil context and unconditional gateErr.Error.Message dereferences from previous PRs are fixed; Authorization header is correctly stripped from PreHook and re-injected post-hook.
core/schemas/bifrost.go	New request/response types for ping, list_tools, connect, and the unified execute_tool stub; GetErrorString has unreachable dead code in the default StatusCode branch.
core/schemas/plugin.go	Introduces MCPConnectionPlugin interface and MCPPluginNoOpHooks helper for backwards-compat; design is clean and clearly documented.
core/mcp/healthmonitor.go	Health-check ping now routed through plugin gate via runPingWithHooks; previously reported unconditional error return is fixed.
core/mcp/mcp.go	Plugin pipeline provider/release lifted from ToolsManager to MCPManager; executeTool callback removed from agent interface and replaced with internal m.executeToolForAgent.
core/mcp/toolmanager.go	Plugin pipeline dependency moved up to MCPManager; PluginPipeline interface extended with RunMCPPreConnectionHooks/RunMCPPostConnectionHooks; clean refactor.
core/mcp/interface.go	executeTool parameter removed from CheckAndExecuteAgent* signatures; ExecuteChatTool/ExecuteResponsesTool added to the interface.
framework/plugins/soplugin.go	DynamicPlugin extended with optional PreMCPConnectionHook/PostMCPConnectionHook function fields; nil-safe dispatch correctly no-ops for legacy plugins.
plugins/governance/main.go	PreMCPHook/PostMCPHook now skip non-execute-tool envelope types (ping/list_tools) using IsExecuteTool(); prevents governance logic from running on liveness probes.
plugins/logging/main.go	Same as governance — PreMCPHook/PostMCPHook guarded by IsExecuteTool() to skip ping/list_tools envelopes; correct approach.
core/internal/mcptests/connect_ping_listtools_test.go	New integration test file covering connect, ping, and list_tools hook invocation, short-circuiting, mutation, and observability.
core/mcp/codemode/starlark/executecode.go	callMCPTool now retrieves the pipeline via clientManager.GetPluginPipeline() instead of the removed pluginPipelineProvider field; consistent with the MCPManager refactor.

_{Reviews (2): Last reviewed commit: "feat: extend mcp plugin interface for li..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

core/mcp/toolsync.go (1)

133-154: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don’t treat hook-suppressed list results as a successful sync.

This new path can now return empty maps when the hook pipeline removes the list_tools payload, but performSync still overwrites clientState.ToolMap and ToolNameMapping unconditionally. That lets one bad hook invocation temporarily erase every discovered tool for the client. Preserve the previous maps unless the hook path positively reports a real list-tools payload.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/mcp/toolsync.go` around lines 133 - 154, The current sync
unconditionally overwrites clientState.ToolMap and clientState.ToolNameMapping
with newTools/newMapping returned by cts.manager.runListToolsWithHooks, which
lets hook-suppressed (empty) payloads erase the existing maps; change the logic
after the runListToolsWithHooks call to only update the maps when the hook path
returned a real list (e.g., newTools or newMapping is non-empty/non-nil).
Concretely: after obtaining newTools, newMapping, err from
cts.manager.runListToolsWithHooks, if err != nil keep existing maps as you
already do; else if both len(newTools) == 0 and (newMapping == nil ||
len(newMapping) == 0) skip the write/update and log/debug that hooks suppressed
the payload; otherwise proceed to acquire cts.manager.mu and atomically assign
clientState.ToolMap = newTools and clientState.ToolNameMapping = newMapping.
Ensure you reference runListToolsWithHooks, clientState.ToolMap and
clientState.ToolNameMapping when making the change.

🧹 Nitpick comments (2)

core/utils.go (1)

412-414: ⚡ Quick win

Use Go’s standard Deprecated: doc format here.

The wrapper is fine, but the current comment won’t be picked up by gopls/godoc as a real deprecation marker. Prefer the standard form so callers actually see the migration hint.

♻️ Proposed fix

-// // [Deprecated] use err.GetErrorString() instead. Will be removed in a future release.
+// Deprecated: use (*schemas.BifrostError).GetErrorString instead.
 func GetErrorMessage(err *schemas.BifrostError) string {
 	return err.GetErrorString()
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/utils.go` around lines 412 - 414, Replace the nonstandard deprecation
comment above GetErrorMessage with Go's recognized form by prefixing the
sentence with "Deprecated:" (e.g., "Deprecated: use err.GetErrorString()
instead; will be removed in a future release."), so gopls/godoc will surface the
migration hint for callers of GetErrorMessage (which wraps
schemas.BifrostError.GetErrorString).

core/bifrost.go (1)

6547-6568: ⚡ Quick win

Skip non-connection plugins before opening connect spans.

These branches start a connect pre/post span and update BifrostContextKeySpanID even when the plugin does not implement schemas.MCPConnectionPlugin. That creates synthetic "skipped" spans and can incorrectly make later real connection hooks children of a plugin that never actually ran.

♻️ Suggested adjustment

 func (p *PluginPipeline) RunMCPPreConnectionHooks(ctx *schemas.BifrostContext, req *schemas.BifrostMCPConnectRequest) (*schemas.BifrostMCPConnectRequest, *schemas.MCPConnectionShortCircuit, int) {
 	...
 	for i, plugin := range p.mcpPlugins {
+		cp, ok := plugin.(schemas.MCPConnectionPlugin)
+		if !ok {
+			continue
+		}
+
 		pluginName := plugin.GetName()
 		p.logger.Debug("running MCP connect pre-hook for plugin %s", pluginName)
 		spanCtx, handle := p.tracer.StartSpan(ctx, fmt.Sprintf("plugin.%s.mcp_connect_prehook", sanitizeSpanName(pluginName)), schemas.SpanKindPlugin)
 		...
-
-		if cp, ok := plugin.(schemas.MCPConnectionPlugin); ok {
-			req, shortCircuit, err = cp.PreMCPConnectionHook(pluginCtx, req)
-		} else {
-			// Plugin only implements MCPPlugin — Connect is invisible to it.
-			pluginCtx.ReleasePluginScope()
-			p.tracer.EndSpan(handle, schemas.SpanStatusOk, "skipped (not MCPConnectionPlugin)")
-			p.executedPreHooks = i + 1
-			continue
-		}
+		req, shortCircuit, err = cp.PreMCPConnectionHook(pluginCtx, req)
 		...
 		p.executedPreHooks = i + 1
 	}
 }
 
 func (p *PluginPipeline) RunMCPPostConnectionHooks(ctx *schemas.BifrostContext, resp *schemas.BifrostMCPConnectResponse, bifrostErr *schemas.BifrostError, runFrom int) (*schemas.BifrostMCPConnectResponse, *schemas.BifrostError) {
 	...
 	for i := runFrom - 1; i >= 0; i-- {
 		plugin := p.mcpPlugins[i]
+		cp, ok := plugin.(schemas.MCPConnectionPlugin)
+		if !ok {
+			continue
+		}
+
 		pluginName := plugin.GetName()
 		p.logger.Debug("running MCP connect post-hook for plugin %s", pluginName)
 		spanCtx, handle := p.tracer.StartSpan(ctx, fmt.Sprintf("plugin.%s.mcp_connect_posthook", sanitizeSpanName(pluginName)), schemas.SpanKindPlugin)
 		...
-		cp, ok := plugin.(schemas.MCPConnectionPlugin)
-		if !ok {
-			pluginCtx.ReleasePluginScope()
-			p.tracer.EndSpan(handle, schemas.SpanStatusOk, "skipped (not MCPConnectionPlugin)")
-			continue
-		}
 		resp, bifrostErr, err = cp.PostMCPConnectionHook(pluginCtx, resp, bifrostErr)
 		...
 	}
 }

Also applies to: 6609-6627

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/bifrost.go` around lines 6547 - 6568, The code currently starts a
connect span and writes BifrostContextKeySpanID for every plugin before checking
whether it implements schemas.MCPConnectionPlugin, producing synthetic "skipped"
spans; to fix, only call p.tracer.StartSpan (and read/set
BifrostContextKeySpanID) after you assert the plugin implements
schemas.MCPConnectionPlugin (the type check on
plugin.(schemas.MCPConnectionPlugin)) and just before invoking
PreMCPConnectionHook/OnMCPConnectionHook; move the span creation, span-id
extraction (sanitizeSpanName, p.tracer.StartSpan,
ctx.SetValue(schemas.BifrostContextKeySpanID,...)) and corresponding
p.tracer.EndSpan into the branches that run PreMCPConnectionHook and
OnMCPConnectionHook so non-connection plugins keep being skipped without
creating spans (apply the same change to the corresponding post-hook block
around OnMCPConnectionHook).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/mcp/agent.go`:
- Around line 318-323: The current branch only handles mcpResponse.ChatMessage
and falls back to createToolResultMessage when no ChatMessage exists; update the
branch to explicitly check for mcpResponse.ResponsesMessage (in addition to
ChatMessage) and forward that to channelToolResults (or convert it into the same
tool-result shape as ChatMessage if types differ) before falling back to
createToolResultMessage(toolCall, "", nil); ensure you reference mcpResponse,
ResponsesMessage, ChatMessage, channelToolResults, createToolResultMessage, and
toolCall when making the change so ResponsesMessage results are not lost.

In `@core/mcp/clientmanager.go`:
- Around line 1011-1020: The per-attempt context assignment for STDIO/SSE was
removed causing externalClient.Start to be called with a nil/incorrect context;
restore the assignment perAttemptCtx = longLivedCtx inside the branch that
checks config.ConnectionType == schemas.MCPConnectionTypeSSE ||
config.ConnectionType == schemas.MCPConnectionTypeSTDIO so that
externalClient.Start(perAttemptCtx) uses the long-lived context for subprocess
lifetime, and keep the existing comment about not deferring cancel (leave
m.logger.Debug(...) in place).
- Around line 212-221: VerifyPerUserOAuthConnection() currently creates
connectReq.Headers as an empty map which drops static headers in config.Headers;
update the function to initialize connectReq.Headers by copying config.Headers
into it, but filter out any Authorization/authorization keys so the OAuth flow
supplies its own token (i.e., for each k,v in config.Headers set
connectReq.Headers[k]=v unless strings.EqualFold(k,"authorization")), ensuring
the headers map is non-nil before PreHooks run so verification sees the same
tenant/custom headers as normal connects.

In `@core/mcp/healthmonitor.go`:
- Around line 149-155: When preparing the clientName used for hook-aware probes
(e.g., ping/list_tools) in healthmonitor.go, fall back to stable identifiers
when ExecutionConfig.Name is nil or empty: if clientState.ExecutionConfig == nil
or clientState.ExecutionConfig.Name == "", set clientName = clientState.Name
(and if that is empty too, set clientName = chm.clientID) so plugin-side
filtering/logging receives a non-empty identifier; apply the same fallback logic
in the other occurrence mentioned (the block around the 181-187 region) to
ensure both probe paths always pass a stable clientName.

In `@core/mcp/pluginpipeline.go`:
- Around line 342-362: runPingWithHooks always returns an error string
unconditionally which causes successes to be reported as failures and may panic
when bErr is nil; update the function (around the use of bErr from
manager.runWithPluginPipeline in runPingWithHooks) to check if bErr == nil and
return nil on success, otherwise return a formatted error (e.g.,
fmt.Errorf("ping failed: %s", bErr.GetErrorString())) so you don't call
GetErrorString on a nil bErr or report success as failure.

In `@plugins/governance/main.go`:
- Around line 1509-1512: The PostMCPHook is currently only skipping early when
resp != nil && resp.ChatMessage == nil && resp.ResponsesMessage == nil, which
misses cases where resp == nil for non-tool errors (e.g., failed
list_tools/ping/connect) and allows UpdateUsage to charge usage; modify the
PostMCPHook to use the same gate as PreMCPHook by treating nil resp or non-tool
envelopes as non-tool executions and returning before calling UpdateUsage—i.e.,
check if resp == nil OR (resp.ChatMessage == nil && resp.ResponsesMessage ==
nil) and return (resp, bifrostErr, nil) to avoid charging virtual keys for those
paths.

In `@plugins/logging/main.go`:
- Around line 1262-1271: The PostMCPHook currently only gates logging by
inspecting resp (and calling bifrost.IsCodemodeTool(resp.ExtraFields.ToolName)),
which lets resp==nil error paths fall through and create synthetic MCP tool
logs; update PostMCPHook to use the same tool-execution gate used in PreMCPHook
by checking the original request classification or the presence of the pending
MCP log entry before early-returning—e.g., use the pending entry flag created in
PreMCPHook (or the original request's tool name/type) instead of relying solely
on resp and bifrost.IsCodemodeTool, and skip logging/error-path synthetic-entry
creation when no pending entry exists.

---

Outside diff comments:
In `@core/mcp/toolsync.go`:
- Around line 133-154: The current sync unconditionally overwrites
clientState.ToolMap and clientState.ToolNameMapping with newTools/newMapping
returned by cts.manager.runListToolsWithHooks, which lets hook-suppressed
(empty) payloads erase the existing maps; change the logic after the
runListToolsWithHooks call to only update the maps when the hook path returned a
real list (e.g., newTools or newMapping is non-empty/non-nil). Concretely: after
obtaining newTools, newMapping, err from cts.manager.runListToolsWithHooks, if
err != nil keep existing maps as you already do; else if both len(newTools) == 0
and (newMapping == nil || len(newMapping) == 0) skip the write/update and
log/debug that hooks suppressed the payload; otherwise proceed to acquire
cts.manager.mu and atomically assign clientState.ToolMap = newTools and
clientState.ToolNameMapping = newMapping. Ensure you reference
runListToolsWithHooks, clientState.ToolMap and clientState.ToolNameMapping when
making the change.

---

Nitpick comments:
In `@core/bifrost.go`:
- Around line 6547-6568: The code currently starts a connect span and writes
BifrostContextKeySpanID for every plugin before checking whether it implements
schemas.MCPConnectionPlugin, producing synthetic "skipped" spans; to fix, only
call p.tracer.StartSpan (and read/set BifrostContextKeySpanID) after you assert
the plugin implements schemas.MCPConnectionPlugin (the type check on
plugin.(schemas.MCPConnectionPlugin)) and just before invoking
PreMCPConnectionHook/OnMCPConnectionHook; move the span creation, span-id
extraction (sanitizeSpanName, p.tracer.StartSpan,
ctx.SetValue(schemas.BifrostContextKeySpanID,...)) and corresponding
p.tracer.EndSpan into the branches that run PreMCPConnectionHook and
OnMCPConnectionHook so non-connection plugins keep being skipped without
creating spans (apply the same change to the corresponding post-hook block
around OnMCPConnectionHook).

In `@core/utils.go`:
- Around line 412-414: Replace the nonstandard deprecation comment above
GetErrorMessage with Go's recognized form by prefixing the sentence with
"Deprecated:" (e.g., "Deprecated: use err.GetErrorString() instead; will be
removed in a future release."), so gopls/godoc will surface the migration hint
for callers of GetErrorMessage (which wraps
schemas.BifrostError.GetErrorString).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 59f04791-47af-4652-b4f3-43dd1b197311

📥 Commits

Reviewing files that changed from the base of the PR and between 287f47d and d2eb077.

📒 Files selected for processing (48)

• core/bifrost.go
• core/internal/mcptests/agent_adapter_test.go
• core/internal/mcptests/agent_basic_test.go
• core/internal/mcptests/agent_context_filtering_test.go
• core/internal/mcptests/agent_error_handling_test.go
• core/internal/mcptests/agent_filtering_test.go
• core/internal/mcptests/agent_limits_test.go
• core/internal/mcptests/agent_mixed_permissions_test.go
• core/internal/mcptests/agent_multiconnection_test.go
• core/internal/mcptests/agent_parallel_execution_test.go
• core/internal/mcptests/agent_request_id_test.go
• core/internal/mcptests/agent_state_transitions_test.go
• core/internal/mcptests/agent_test_helpers.go
• core/internal/mcptests/agent_test_helpers_example_test.go
• core/internal/mcptests/codemode_agent_multiturn_test.go
• core/internal/mcptests/codemode_agent_singleturn_test.go
• core/internal/mcptests/codemode_agent_test.go
• core/internal/mcptests/codemode_vs_noncodemode_test.go
• core/internal/mcptests/connect_ping_listtools_test.go
• core/internal/mcptests/context_propagation_test.go
• core/internal/mcptests/test_plugins.go
• core/internal/mcptests/tool_call_id_test.go
• core/mcp/agent.go
• core/mcp/agent_test.go
• core/mcp/clientmanager.go
• core/mcp/codemode.go
• core/mcp/codemode/starlark/executecode.go
• core/mcp/codemode/starlark/starlark.go
• core/mcp/codemode/starlark/starlark_test.go
• core/mcp/exec.go
• core/mcp/healthmonitor.go
• core/mcp/interface.go
• core/mcp/mcp.go
• core/mcp/pluginpipeline.go
• core/mcp/toolmanager.go
• core/mcp/toolmanager_test.go
• core/mcp/toolsync.go
• core/mcp/utils.go
• core/schemas/bifrost.go
• core/schemas/plugin.go
• core/schemas/plugin_native.go
• core/schemas/trace.go
• core/utils.go
• framework/plugins/soloader.go
• framework/plugins/soplugin.go
• plugins/governance/main.go
• plugins/logging/main.go
• ui/app/workspace/config/views/mcpView.tsx

💤 Files with no reviewable changes (19)

• core/mcp/codemode.go
• core/internal/mcptests/agent_test_helpers_example_test.go
• core/internal/mcptests/agent_multiconnection_test.go
• core/internal/mcptests/agent_test_helpers.go
• core/internal/mcptests/codemode_agent_multiturn_test.go
• core/internal/mcptests/tool_call_id_test.go
• core/internal/mcptests/codemode_vs_noncodemode_test.go
• core/internal/mcptests/agent_state_transitions_test.go
• core/internal/mcptests/agent_error_handling_test.go
• core/internal/mcptests/agent_context_filtering_test.go
• core/internal/mcptests/agent_adapter_test.go
• core/internal/mcptests/context_propagation_test.go
• core/internal/mcptests/agent_request_id_test.go
• core/internal/mcptests/codemode_agent_singleturn_test.go
• core/internal/mcptests/agent_mixed_permissions_test.go
• core/internal/mcptests/agent_parallel_execution_test.go
• core/internal/mcptests/agent_filtering_test.go
• core/internal/mcptests/codemode_agent_test.go
• core/internal/mcptests/agent_basic_test.go

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/utils.go`:
- Around line 413-414: GetErrorMessage currently calls err.GetErrorString()
unconditionally which can panic if a legacy caller passes nil; modify
GetErrorMessage to guard against nil by checking if err == nil and returning an
empty string (or another safe default) when nil, otherwise return
err.GetErrorString(); reference function GetErrorMessage, type
schemas.BifrostError, and method GetErrorString to locate the change.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9734f582-796d-40f5-86fb-6ca212c3029b

📥 Commits

Reviewing files that changed from the base of the PR and between d2eb077 and 576df36.

📒 Files selected for processing (48)

• core/bifrost.go
• core/internal/mcptests/agent_adapter_test.go
• core/internal/mcptests/agent_basic_test.go
• core/internal/mcptests/agent_context_filtering_test.go
• core/internal/mcptests/agent_error_handling_test.go
• core/internal/mcptests/agent_filtering_test.go
• core/internal/mcptests/agent_limits_test.go
• core/internal/mcptests/agent_mixed_permissions_test.go
• core/internal/mcptests/agent_multiconnection_test.go
• core/internal/mcptests/agent_parallel_execution_test.go
• core/internal/mcptests/agent_request_id_test.go
• core/internal/mcptests/agent_state_transitions_test.go
• core/internal/mcptests/agent_test_helpers.go
• core/internal/mcptests/agent_test_helpers_example_test.go
• core/internal/mcptests/codemode_agent_multiturn_test.go
• core/internal/mcptests/codemode_agent_singleturn_test.go
• core/internal/mcptests/codemode_agent_test.go
• core/internal/mcptests/codemode_vs_noncodemode_test.go
• core/internal/mcptests/connect_ping_listtools_test.go
• core/internal/mcptests/context_propagation_test.go
• core/internal/mcptests/test_plugins.go
• core/internal/mcptests/tool_call_id_test.go
• core/mcp/agent.go
• core/mcp/agent_test.go
• core/mcp/clientmanager.go
• core/mcp/codemode.go
• core/mcp/codemode/starlark/executecode.go
• core/mcp/codemode/starlark/starlark.go
• core/mcp/codemode/starlark/starlark_test.go
• core/mcp/exec.go
• core/mcp/healthmonitor.go
• core/mcp/interface.go
• core/mcp/mcp.go
• core/mcp/pluginpipeline.go
• core/mcp/toolmanager.go
• core/mcp/toolmanager_test.go
• core/mcp/toolsync.go
• core/mcp/utils.go
• core/schemas/bifrost.go
• core/schemas/plugin.go
• core/schemas/plugin_native.go
• core/schemas/trace.go
• core/utils.go
• framework/plugins/soloader.go
• framework/plugins/soplugin.go
• plugins/governance/main.go
• plugins/logging/main.go
• ui/app/workspace/config/views/mcpView.tsx

💤 Files with no reviewable changes (19)

• core/internal/mcptests/agent_test_helpers.go
• core/mcp/codemode.go
• core/internal/mcptests/tool_call_id_test.go
• core/internal/mcptests/codemode_vs_noncodemode_test.go
• core/internal/mcptests/codemode_agent_singleturn_test.go
• core/internal/mcptests/agent_request_id_test.go
• core/internal/mcptests/agent_filtering_test.go
• core/internal/mcptests/agent_adapter_test.go
• core/internal/mcptests/codemode_agent_multiturn_test.go
• core/internal/mcptests/agent_context_filtering_test.go
• core/internal/mcptests/agent_basic_test.go
• core/internal/mcptests/codemode_agent_test.go
• core/internal/mcptests/agent_parallel_execution_test.go
• core/internal/mcptests/agent_multiconnection_test.go
• core/internal/mcptests/agent_error_handling_test.go
• core/internal/mcptests/agent_test_helpers_example_test.go
• core/internal/mcptests/agent_state_transitions_test.go
• core/internal/mcptests/context_propagation_test.go
• core/internal/mcptests/agent_mixed_permissions_test.go

✅ Files skipped from review due to trivial changes (3)

• core/mcp/codemode/starlark/starlark_test.go
• core/schemas/trace.go
• ui/app/workspace/config/views/mcpView.tsx

🚧 Files skipped from review as they are similar to previous changes (21)

• core/mcp/toolsync.go
• core/mcp/codemode/starlark/starlark.go
• core/schemas/plugin_native.go
• core/mcp/codemode/starlark/executecode.go
• core/mcp/interface.go
• core/mcp/pluginpipeline.go
• framework/plugins/soloader.go
• core/mcp/utils.go
• core/schemas/plugin.go
• core/mcp/agent.go
• core/mcp/healthmonitor.go
• core/internal/mcptests/agent_limits_test.go
• core/mcp/toolmanager_test.go
• core/internal/mcptests/connect_ping_listtools_test.go
• core/schemas/bifrost.go
• core/internal/mcptests/test_plugins.go
• core/mcp/toolmanager.go
• core/mcp/exec.go
• core/mcp/mcp.go
• core/bifrost.go
• core/mcp/clientmanager.go

[akshaydeo]

Merge activity

• May 12, 8:14 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 12, 8:15 AM UTC: @akshaydeo merged this pull request with Graphite.