feat: propagate request context through mcp client connection - #3768
Conversation
|
Warning Review limit reached
More reviews will be available in 2 minutes and 55 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThe pull request updates MCP client connection operations to accept and thread request-scoped context through the call stack. Method signatures are updated at the interface and implementation layers; HTTP handlers convert fasthttp contexts to Bifrost-aware contexts; and all call sites, including tests and examples, provide context parameters to enable connection hooks to observe per-request values. ChangesMCP Request Context Threading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
Comment |
|
|
Confidence Score: 3/5The connect-phase context threading works as advertised, but the list-tools phase that immediately follows still uses the manager context, so plugin hooks reading request headers during list-tools will silently receive nil. The connect gate (runConnectWithPluginPipeline) correctly receives requestCtx, but toolRetrievalCtx at line 1444 of clientmanager.go is derived from m.ctx, not requestCtx. Any plugin that reads BifrostContextKeyRequestHeaders inside PreMCPHook or PostMCPHook when RequestType == MCPRequestTypeListTools will silently get nil — directly contradicting the PR description's explicit claim that list-tools hooks also use the caller's context. core/mcp/clientmanager.go around the toolRetrievalCtx construction (line 1444) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
477e3e0 to
c30f927
Compare
9cd567b to
098d015
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bifrost.go`:
- Around line 3644-3648: The example calls AddMCPClient with a value but the
function now expects a *schemas.MCPClientConfig; fix by constructing the config
as a literal and passing its address (e.g. cfg := &schemas.MCPClientConfig{
Name: "my-mcp-client", ConnectionType: schemas.MCPConnectionTypeHTTP,
ConnectionString: &url }; err := bifrost.AddMCPClient(ctx, cfg)), referencing
AddMCPClient and schemas.MCPClientConfig so the code compiles.
In `@examples/plugins/mcp-only/main.go`:
- Around line 232-233: The current fmt.Printf logs raw header values from
ctx.Value(schemas.BifrostContextKeyRequestHeaders) which can leak secrets;
instead, replace the direct print with a sanitized log: retrieve the headers
(allHeaders), iterate over them and either (A) log only header names and counts
or (B) redact values for sensitive keys (e.g., "Authorization", "Cookie",
"Set-Cookie", "Proxy-Authorization", "X-Api-Key") by replacing their values with
"[REDACTED]" before formatting. Update the println call that references
allHeaders to call this sanitizer and log the sanitized map or names only.
🪄 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: 5d4e3706-b1fb-49bc-8f67-ad1d0d714c79
⛔ Files ignored due to path filters (3)
examples/mcps/temperature/package-lock.jsonis excluded by!**/package-lock.jsonexamples/mcps/test-tools-server/package-lock.jsonis excluded by!**/package-lock.jsonexamples/plugins/mcp-only/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
core/bifrost.gocore/internal/mcptests/agent_filtering_test.gocore/internal/mcptests/client_management_test.gocore/internal/mcptests/concurrency_advanced_test.gocore/internal/mcptests/connect_ping_listtools_test.gocore/internal/mcptests/error_handling_protocol_test.gocore/internal/mcptests/health_monitoring_test.gocore/internal/mcptests/integration_test.gocore/mcp/clientmanager.gocore/mcp/interface.gocore/mcp/mcp.goexamples/plugins/mcp-only/main.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcp_per_user_headers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.go
098d015 to
fdf6555
Compare
Merge activity
|
The base branch was changed.
fdf6555 to
bab9c2e
Compare
## Summary `AddMCPClient` and `connectToMCPClient` previously used the manager's background context when running connection hooks, which meant request-scoped values (such as HTTP headers extracted by the transport layer) were invisible to MCP plugins during the connect phase. This PR threads the caller's `context.Context` through `AddMCPClient` so that connect-time hooks can read request-scoped values while keeping persistent transport lifetimes bound to the manager context. ## Changes - `MCPManager.AddClient`, `Bifrost.AddMCPClient`, and the internal `connectToMCPClient` now accept a `context.Context` parameter. The `BifrostContext` passed to connect/list-tools hooks is derived from the caller's context rather than the manager's background context. - `ReconnectClient`, `EnableClient`, and `UpdateClientConnection` continue to use the manager context (`m.ctx`) since they are infrastructure-initiated and have no caller request context. - `NewMCPManager` passes `manager.ctx` when calling `AddClient` during startup initialization, preserving existing behavior. - The HTTP transport's `addMCPClient`, `completeMCPClientOAuth`, and `flowSubmit` handlers now convert the incoming `fasthttp.RequestCtx` to a `BifrostContext` before calling `AddMCPClient` and related verification methods, so HTTP request headers are available to MCP plugins. - `MCPManagerInterface` updated to reflect the new `AddClient` signature. - The `mcp-only` plugin example logs request headers received in `PreMCPConnectionHook` to demonstrate the new capability. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/... go test ./transports/... ``` To verify that request headers are visible in a connect hook, configure the `mcp-only` plugin example with `EnableLogging: true` and call the `POST /mcp/clients` endpoint with custom headers. The plugin's `PreMCPConnectionHook` log line will print the headers extracted from the incoming request. ## Breaking changes - [x] Yes - [ ] No `MCPManagerInterface.AddClient` and `Bifrost.AddMCPClient` now require a `context.Context` as the first argument. Any callers implementing or calling these interfaces directly must add a context argument (e.g. `context.Background()` as a minimal migration). ## Related issues ## Security considerations Request headers passed through the context may contain credentials or tokens. MCP plugin authors should treat values read from `BifrostContextKeyRequestHeaders` as sensitive and avoid logging them in production. ## 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
2 participants
Summary
AddMCPClientandconnectToMCPClientpreviously used the manager's background context when running connection hooks, which meant request-scoped values (such as HTTP headers extracted by the transport layer) were invisible to MCP plugins during the connect phase. This PR threads the caller'scontext.ContextthroughAddMCPClientso that connect-time hooks can read request-scoped values while keeping persistent transport lifetimes bound to the manager context.Changes
MCPManager.AddClient,Bifrost.AddMCPClient, and the internalconnectToMCPClientnow accept acontext.Contextparameter. TheBifrostContextpassed to connect/list-tools hooks is derived from the caller's context rather than the manager's background context.ReconnectClient,EnableClient, andUpdateClientConnectioncontinue to use the manager context (m.ctx) since they are infrastructure-initiated and have no caller request context.NewMCPManagerpassesmanager.ctxwhen callingAddClientduring startup initialization, preserving existing behavior.addMCPClient,completeMCPClientOAuth, andflowSubmithandlers now convert the incomingfasthttp.RequestCtxto aBifrostContextbefore callingAddMCPClientand related verification methods, so HTTP request headers are available to MCP plugins.MCPManagerInterfaceupdated to reflect the newAddClientsignature.mcp-onlyplugin example logs request headers received inPreMCPConnectionHookto demonstrate the new capability.Type of change
Affected areas
How to test
To verify that request headers are visible in a connect hook, configure the
mcp-onlyplugin example withEnableLogging: trueand call thePOST /mcp/clientsendpoint with custom headers. The plugin'sPreMCPConnectionHooklog line will print the headers extracted from the incoming request.Breaking changes
MCPManagerInterface.AddClientandBifrost.AddMCPClientnow require acontext.Contextas the first argument. Any callers implementing or calling these interfaces directly must add a context argument (e.g.context.Background()as a minimal migration).Related issues
Security considerations
Request headers passed through the context may contain credentials or tokens. MCP plugin authors should treat values read from
BifrostContextKeyRequestHeadersas sensitive and avoid logging them in production.Checklist
docs/contributing/README.mdand followed the guidelines