refactor: expose RunWithPluginPipeline on ClientManager and route Starlark nested tool calls through canonical plugin gate#3794
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR exports the MCP plugin pipeline entrypoint, routes nested MCP tool/list/ping calls (including Starlark tool calls) through it, updates call sites and test mocks to implement the new method, and adjusts logging hooks/tests to skip client-prefixed codemode tools. ChangesMCP plugin pipeline standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
|
Confidence Score: 5/5Safe to merge — the refactor is a straightforward extraction of existing logic into the canonical gate, the logging bug fix is well-tested, and no data paths are altered in a way that changes observable behavior for correctly-named tools. The changes are well-scoped: the callMCPTool rewrite is a mechanical substitution of identical pre/post-hook semantics, PopulateExtraFields back-fills attribution without overwriting already-set fields, and the logging fix has a direct before/after test. No surprising edge cases were found after tracing the error paths, ExtraFields flow, and toolCallReq closure capture. No files require special attention. Important Files Changed
Reviews (4): Last reviewed commit: "fix: enclose codemode execution in mcp m..." | Re-trigger Greptile |
9866375 to
f2c3273
Compare
098d015 to
fdf6555
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 (1)
core/mcp/pluginpipeline.go (1)
118-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPopulate
ExtraFieldson the nil-pipeline fast path.When
GetPluginPipeline()returns nil, successful responses bypassPopulateExtraFields(...)entirely.runListToolsWithHooksandrunPingWithHooksonly setLatencyin their op closures, so this exported gate now dropsMCPRequestType/ClientNamemetadata in the exact pass-through mode it documents as canonical.Suggested fix
pipeline := m.GetPluginPipeline() if pipeline == nil { resp, opErr := op(req) + if resp != nil { + clientName := "" + toolName := "" + if req != nil { + clientName = req.ClientName + toolName = req.GetToolName() + } + resp.PopulateExtraFields(mcpReqType, clientName, toolName) + } if opErr != nil { return resp, &schemas.BifrostError{ IsBifrostError: false, Error: &schemas.ErrorField{Message: opErr.Error()}, ExtraFields: schemas.BifrostErrorExtraFields{MCPRequestType: mcpReqType},🤖 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/pluginpipeline.go` around lines 118 - 129, The nil-pipeline fast path in GetPluginPipeline() returns the op response without calling PopulateExtraFields, so metadata like MCPRequestType and ClientName is lost; update the nil-pipeline branch in pluginpipeline.go to call PopulateExtraFields (or otherwise set resp.ExtraFields) before returning: obtain/construct the same schemas.BifrostErrorExtraFields populated by PopulateExtraFields (at minimum MCPRequestType and ClientName, and ensure Latency from the op closure is preserved) so that successful responses on the direct-op path include the same ExtraFields as runListToolsWithHooks/runPingWithHooks.
🤖 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/codemode/starlark/executecode.go`:
- Around line 493-551: The closure passed to
s.clientManager.RunWithPluginPipeline must honor the full pre-hook mutated tool
call instead of silently falling back to originalToolName or old args: when
preReq != nil && preReq.ChatAssistantMessageToolCall != nil, copy the mutation
into toolCallReq and set callRequest.Params.Name to the mutated tool name (from
toolCallReq.Function.Name) and callRequest.Params.Arguments to the mutated
arguments (use the raw toolCallReq.Function.Arguments string or set args to
empty if cleared) rather than defaulting to originalToolName or previous args;
ensure parsing failures do not cause the code path to revert to originalToolName
or keep old args—still use the mutated name and either the raw argument string
or empty args so the post-prehook tool call is applied consistently.
In `@plugins/logging/operations_test.go`:
- Around line 225-253: The test TestPreMCPHookSkipsPrefixedCodemodeTool calls
Init which starts background workers but never calls Cleanup; modify the test to
call plugin.Cleanup() (or defer plugin.Cleanup()) after successful Init to stop
background goroutines/tickers and avoid leaks—locate the plugin variable
returned from Init in this test and add a defer plugin.Cleanup() immediately
after checking err.
---
Outside diff comments:
In `@core/mcp/pluginpipeline.go`:
- Around line 118-129: The nil-pipeline fast path in GetPluginPipeline() returns
the op response without calling PopulateExtraFields, so metadata like
MCPRequestType and ClientName is lost; update the nil-pipeline branch in
pluginpipeline.go to call PopulateExtraFields (or otherwise set
resp.ExtraFields) before returning: obtain/construct the same
schemas.BifrostErrorExtraFields populated by PopulateExtraFields (at minimum
MCPRequestType and ClientName, and ensure Latency from the op closure is
preserved) so that successful responses on the direct-op path include the same
ExtraFields as runListToolsWithHooks/runPingWithHooks.
🪄 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: a2adcedc-fc55-4982-9cc5-2ca28f298181
📒 Files selected for processing (9)
core/mcp/agent_test.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark_test.gocore/mcp/exec.gocore/mcp/pluginpipeline.gocore/mcp/toolmanager.gocore/mcp/toolmanager_test.goplugins/logging/main.goplugins/logging/operations_test.go
f2c3273 to
787532f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/mcp/pluginpipeline.go (1)
39-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard exported plugin-gate entrypoint against nil input panics.
Now that this method is public, nil inputs can panic (
ctx.Valueon Line 54,req.RequestTypeon Line 57). Add explicit input validation and return a typedBifrostErrorinstead of crashing.💡 Suggested fix
func (m *MCPManager) RunWithPluginPipeline( ctx *schemas.BifrostContext, req *schemas.BifrostMCPRequest, op MCPOpFunc, ) (finalResponse *schemas.BifrostMCPResponse, finalError *schemas.BifrostError) { + if ctx == nil { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{Message: "context cannot be nil"}, + } + } + if req == nil { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{Message: "MCP request cannot be nil"}, + } + } + // Ensure a request ID exists so plugin hooks have something to correlate on. // Connect/ping/list_tools fire from background contexts that typically lack one. - if ctx != nil { - if _, ok := ctx.Value(schemas.BifrostContextKeyRequestID).(string); !ok { - ctx.SetValue(schemas.BifrostContextKeyRequestID, uuid.New().String()) - } + if _, ok := ctx.Value(schemas.BifrostContextKeyRequestID).(string); !ok { + ctx.SetValue(schemas.BifrostContextKeyRequestID, uuid.New().String()) }🤖 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/pluginpipeline.go` around lines 39 - 57, RunWithPluginPipeline currently dereferences ctx and req without validation which can panic; add early input validation at the top of RunWithPluginPipeline to check for nil ctx and nil req and immediately return a typed *schemas.BifrostError (populated with an appropriate error code/message such as "invalid input: nil ctx" or "invalid input: nil req") instead of proceeding; ensure no access to ctx.Value or req.RequestType happens before these checks, and then proceed to the existing logic (tracer/span handling, plugin hooks) only after inputs are validated.
🤖 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.
Outside diff comments:
In `@core/mcp/pluginpipeline.go`:
- Around line 39-57: RunWithPluginPipeline currently dereferences ctx and req
without validation which can panic; add early input validation at the top of
RunWithPluginPipeline to check for nil ctx and nil req and immediately return a
typed *schemas.BifrostError (populated with an appropriate error code/message
such as "invalid input: nil ctx" or "invalid input: nil req") instead of
proceeding; ensure no access to ctx.Value or req.RequestType happens before
these checks, and then proceed to the existing logic (tracer/span handling,
plugin hooks) only after inputs are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10a85600-74a4-4974-9897-d6ef3f321ed5
📒 Files selected for processing (9)
core/mcp/agent_test.gocore/mcp/codemode/starlark/executecode.gocore/mcp/codemode/starlark/starlark_test.gocore/mcp/exec.gocore/mcp/pluginpipeline.gocore/mcp/toolmanager.gocore/mcp/toolmanager_test.goplugins/logging/main.goplugins/logging/operations_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/mcp/toolmanager_test.go
- core/mcp/codemode/starlark/starlark_test.go
- core/mcp/exec.go
- plugins/logging/operations_test.go
- plugins/logging/main.go
- core/mcp/agent_test.go
- core/mcp/codemode/starlark/executecode.go
Merge activity
|
The base branch was changed.
787532f to
b6208e5
Compare
… Starlark nested tool calls through canonical plugin gate (#3794) ## Summary Starlark codemode nested tool calls previously duplicated the plugin gate logic inline, diverging from the canonical `RunWithPluginPipeline` path used by the gateway. This PR promotes `runWithPluginPipeline` to an exported method on `MCPManager`, adds it to the `ClientManager` interface, and routes Starlark nested tool calls through it — making codemode tool execution observationally identical to gateway-routed calls (tracing, pre/post hooks, short-circuit semantics, plugin log draining). A secondary fix corrects the logging plugin's `PreMCPHook` to also skip codemode meta-tools when they arrive with a client prefix (e.g. `myclient-executeToolCode`), not just bare names. Previously, `PreMCPHook` would insert a pending log row for the prefixed name while `PostMCPHook` would skip on the stripped bare name, leaving an orphaned row to expire as a fake TTL error. ## Changes - `runWithPluginPipeline` renamed to `RunWithPluginPipeline` and added to the `ClientManager` interface so any call site outside the gateway (e.g. Starlark codemode) can use the canonical plugin gate. - `callMCPTool` in the Starlark codemode executor replaced its inline pre/post hook orchestration with a single `RunWithPluginPipeline` call. Connection acquisition still happens outside the gate, mirroring the gateway's `exec.go` ordering. - `BifrostMCPRequest.ClientName` is now set explicitly before the gate so plugins can attribute short-circuit responses without re-parsing the prefixed tool name. - Logging plugin's `PreMCPHook` now checks both the full tool name and the suffix after the client prefix for codemode meta-tools, preventing orphaned pending rows. - All mock `ClientManager` implementations in tests implement the new `RunWithPluginPipeline` method with a pass-through that wraps plain errors into `BifrostError`. - New test `TestPreMCPHookSkipsPrefixedCodemodeTool` verifies that `PreMCPHook` does not create a pending row for a client-prefixed codemode tool name. ## Type of change - [x] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/mcp/... go test ./core/mcp/codemode/starlark/... go test ./plugins/logging/... ``` Verify that: - Starlark nested tool calls flow through pre/post hooks identically to gateway-routed calls. - A codemode meta-tool invoked with a client prefix (e.g. `myclient-executeToolCode`) does not produce a pending log row in the logging plugin. ## Breaking changes - [x] Yes - [ ] No `ClientManager` interface gains a new `RunWithPluginPipeline` method. Any external implementation of `ClientManager` must add this method. All in-repo mocks have been updated. ## Related issues ## Security considerations No new auth, secrets, or PII surface area introduced. The gate's short-circuit and credential-error paths are preserved unchanged. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
## Summary This PR releases **core v1.5.14**, **framework v1.3.14**, **transports v1.5.6**, and bumps all dependent plugins to their respective `.14` patch versions. It delivers a broad set of new capabilities across MCP authentication, key rotation, OTel metrics, Bedrock/Anthropic compatibility, and UI improvements, alongside a number of targeted bug fixes and refactors. ## Changes - **Direct API Key Header** — Providers can now receive an API key passed directly via a request header (#3817) - **MCP Per-User Auth** — Introduced `MCPCredentialStore` abstraction, per-user MCP credential reconciliation, and a new per-user header auth type with lazy-auth submission flow (#3656, #3702, #3703, #3704, #3705) - **MCP TLS Configuration** — Added configurable TLS (`insecureSkipVerify`, `caCertPem`) for HTTP/SSE MCP client connections (#3779, #3783) - **MCP Sessions Management** — Filter, search, and pagination on the MCP sessions list API and table, plus a `can_reauth` identity gate (#3823, #3824, #3825) - **Key Rotation** — Keys now rotate on 401/402/403 responses; returns `502 upstream_credentials_exhausted` when all keys are permanently exhausted. Added `triggered_rotation` to `KeyAttemptRecord` and tightened `bifrost_key_rotation_events_total` semantics (#3430, #3491) - **OTel Metrics** — Added OTel spec-compatible metrics (backward compatible) with provider cache and semantic cache attributes in metrics export (#3865, #3816) - **Opus 4.8 Support** — System message handling and general compatibility for Opus 4.8 (#3868, #3878) - **Dimension Rankings** — New `GetDimensionRankings` API and dashboard tabs for team, customer, BU, and user rankings (#3766) - **Model Pricing Attributes** — `additional_attributes` field on model pricing rows with management API and UI editor (#3829) - **Prompt Cache Retention** — Added prompt cache retention parameter on responses requests (#3810) - **Tool Call Execution UI** — Inline tool-call execution, stop streaming, bulk execute/submit, and a redesigned tool-call UI (#3837, #3843) - **Sheet Navigation** — Prev/next keyboard navigation and URL state across virtual key, MCP client, and routing rule sheets (#3739, #3740, #3744, #3745) - **Bedrock Tool Name Truncation** — Truncate Bedrock function/tool names to the provider length limit - **Bedrock Guardrails** — Set guardrail config in Bedrock requests built from responses (#3862) - **Anthropic Tool Use** — Default `tool_use` input to `{}` when arguments are absent (#3880) - **Responses Streaming** — Fixed responses stream events (#3838) - **Compat Flow** — Fixed missing parameter parsing on the compat flow (#3881) - **Passthrough API Version** — Set a default API version in passthrough requests as a fallback (#3853) - **Virtual Key Updates** — Avoid overriding optional fields during virtual key update (#3855) - **User-Mode Flows** — Gate user-mode flows on caller `user_id`, skip temp token mint, and unify flow/credential kind filtering for pending flows (#3841, #3859) - **Partial Tool Calls** — Handle partial tool call execution failures and return successful results (#3849) - **URL Query Escaping** — Support escaped characters in URL query parameters (#3826) - **MCP Auth Errors** — Inline banner and retry support for MCP auth-required errors (#3856) - **Renamed Resolvers** — `staticHeadersResolver`/`serverOAuthResolver` renamed to `sharedHeadersResolver`/`sharedOAuthResolver` (#3840) - **Starlark Nested Tool Calls** — Exposed `RunWithPluginPipeline` on `ClientManager` and routed Starlark nested tool calls through the canonical plugin gate (#3794) - **Deferred-Fill OAuth Removed** — Removed deferred-fill user-mode OAuth flow support (#3839) - **Go 1.26.3** — Upgraded toolchain to Go 1.26.3 (#3782) ## Type of change - [x] Bug fix - [x] Feature - [x] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version # should report go1.26.3 go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` - Validate MCP per-user auth by configuring a per-user header auth type and confirming credentials are stored and reconciled on virtual key and MCP client changes. - Validate key rotation by triggering a 401/402/403 from an upstream provider and confirming rotation occurs; exhaust all keys and confirm a `502 upstream_credentials_exhausted` is returned. - Validate OTel metrics output includes `provider_cache` and `semantic_cache` attributes. - Validate Bedrock requests with tool names exceeding the provider limit are truncated correctly. - Validate Opus 4.8 system message handling by sending a request with a system message to an Opus 4.8 endpoint. ## Breaking changes - [x] Yes - [ ] No The deferred-fill user-mode OAuth flow has been removed (#3839). Any integrations relying on that flow must migrate to the new per-user credential store approach. The `staticHeadersResolver` and `serverOAuthResolver` identifiers have been renamed to `sharedHeadersResolver` and `sharedOAuthResolver` respectively (#3840); any direct references must be updated. ## Related issues #3817, #3656, #3702, #3703, #3704, #3705, #3779, #3783, #3823, #3824, #3825, #3430, #3491, #3865, #3816, #3868, #3878, #3766, #3829, #3810, #3837, #3843, #3739, #3740, #3744, #3745, #3862, #3880, #3838, #3881, #3853, #3855, #3841, #3859, #3849, #3826, #3856, #3840, #3794, #3839, #3782, #3724, #3814, #3836, #3869, #3886 ## Security considerations - MCP per-user credentials are stored via the new `MCPCredentialStore` abstraction; ensure the backing store is appropriately access-controlled and that credential values are encrypted at rest. - The direct API key header feature passes provider secrets via HTTP headers; ensure TLS is enforced on all ingress paths and that headers are not logged in plaintext. - User-mode flows are now gated on `caller user_id` and temp token minting is skipped where appropriate, reducing the surface for privilege escalation. - TLS configuration for MCP HTTP/SSE connections supports `insecureSkipVerify`; this should only be enabled in controlled environments. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable

Summary
Starlark codemode nested tool calls previously duplicated the plugin gate logic inline, diverging from the canonical
RunWithPluginPipelinepath used by the gateway. This PR promotesrunWithPluginPipelineto an exported method onMCPManager, adds it to theClientManagerinterface, and routes Starlark nested tool calls through it — making codemode tool execution observationally identical to gateway-routed calls (tracing, pre/post hooks, short-circuit semantics, plugin log draining).A secondary fix corrects the logging plugin's
PreMCPHookto also skip codemode meta-tools when they arrive with a client prefix (e.g.myclient-executeToolCode), not just bare names. Previously,PreMCPHookwould insert a pending log row for the prefixed name whilePostMCPHookwould skip on the stripped bare name, leaving an orphaned row to expire as a fake TTL error.Changes
runWithPluginPipelinerenamed toRunWithPluginPipelineand added to theClientManagerinterface so any call site outside the gateway (e.g. Starlark codemode) can use the canonical plugin gate.callMCPToolin the Starlark codemode executor replaced its inline pre/post hook orchestration with a singleRunWithPluginPipelinecall. Connection acquisition still happens outside the gate, mirroring the gateway'sexec.goordering.BifrostMCPRequest.ClientNameis now set explicitly before the gate so plugins can attribute short-circuit responses without re-parsing the prefixed tool name.PreMCPHooknow checks both the full tool name and the suffix after the client prefix for codemode meta-tools, preventing orphaned pending rows.ClientManagerimplementations in tests implement the newRunWithPluginPipelinemethod with a pass-through that wraps plain errors intoBifrostError.TestPreMCPHookSkipsPrefixedCodemodeToolverifies thatPreMCPHookdoes not create a pending row for a client-prefixed codemode tool name.Type of change
Affected areas
How to test
Verify that:
myclient-executeToolCode) does not produce a pending log row in the logging plugin.Breaking changes
ClientManagerinterface gains a newRunWithPluginPipelinemethod. Any external implementation ofClientManagermust add this method. All in-repo mocks have been updated.Related issues
Security considerations
No new auth, secrets, or PII surface area introduced. The gate's short-circuit and credential-error paths are preserved unchanged.
Checklist
docs/contributing/README.mdand followed the guidelines