fix: enable per-call tool discovery for shared oauth/headers/none MCP clients and rename UpdateClientConnection to UpdateClientCredentials - #5970
Merged
Conversation
This was referenced Aug 8, 2026
|
|
This was referenced Aug 8, 2026
Merged
Collaborator
Author
This was referenced Aug 8, 2026
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR renames MCP credential-update APIs and extends per-call client support. Shared OAuth, headers, and none-auth clients can resolve credentials, discover tools, transition from pending verification, and start monitoring. HTTP handlers and the UI now use reconnect eligibility. ChangesMCP credential and discovery flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthFlow
participant MCPManager
participant CredentialStore
participant MCPServer
participant ConnectionChecker
OAuthFlow->>MCPManager: update MCP client credentials
MCPManager->>CredentialStore: resolve per-call headers
CredentialStore->>MCPServer: send resolved credentials
MCPServer-->>MCPManager: return discovered tools
MCPManager->>ConnectionChecker: start monitoring
MCPManager-->>OAuthFlow: complete refresh or return not-applicable
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 8, 2026
Collaborator
Author
Merge activity
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/lib/config.go (1)
6719-6740: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMirror
PendingOAuthConfigafter the runtime update.
UpdateClientCredentialstreatsnewConfig.PendingOAuthConfig == nilas a meaningful authorization-complete state. This method updates onlyHeadersandOauthConfigID. The old pending OAuth block can remain inMCPConfigafter the runtime client has cleared it.Set
cc.PendingOAuthConfig = newConfig.PendingOAuthConfigwhile holdingmuMCP. This keeps later reads and reloads consistent with the live client.Proposed fix
if newConfig.OauthConfigID != nil { cc.OauthConfigID = newConfig.OauthConfigID } + cc.PendingOAuthConfig = newConfig.PendingOAuthConfig breakAs per path instructions, keep runtime config updates and persistent store updates rollback-aware.
🤖 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 `@transports/bifrost-http/lib/config.go` around lines 6719 - 6740, Update the MCPConfig synchronization in the client-update method to assign cc.PendingOAuthConfig from newConfig.PendingOAuthConfig while muMCP is held, including nil to represent authorization completion. Keep the existing Headers and OauthConfigID updates unchanged, and preserve the current rollback-aware handling for runtime and persistent-store updates.Source: Path instructions
🤖 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/clientmanager_test.go`:
- Around line 326-409: Register the test manager for cleanup immediately after
each NewMCPManager call in the two tests, using t.Cleanup to invoke m.Cleanup
before the test exits. Ensure the connection checkers started by
UpdateClientCredentials are stopped, including for the HTTP-backed discovery
test.
---
Outside diff comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 6719-6740: Update the MCPConfig synchronization in the
client-update method to assign cc.PendingOAuthConfig from
newConfig.PendingOAuthConfig while muMCP is held, including nil to represent
authorization completion. Keep the existing Headers and OauthConfigID updates
unchanged, and preserve the current rollback-aware handling for runtime and
persistent-store updates.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bd58f323-7f2b-424b-9dee-f6f005cec42e
📒 Files selected for processing (23)
core/bifrost.gocore/mcp/addclient_discoveredtools_test.gocore/mcp/admin_tool_discovery_test.gocore/mcp/clientmanager.gocore/mcp/clientmanager_test.gocore/mcp/connectionchecker.gocore/mcp/connectionchecker_test.gocore/mcp/credstore/none.gocore/mcp/credstore/none_test.gocore/mcp/credstore/shared_headers.gocore/mcp/credstore/shared_headers_test.gocore/mcp/credstore/shared_oauth.gocore/mcp/credstore/shared_oauth_test.gocore/mcp/interface.gocore/mcp/reauth_state_test.gocore/schemas/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcp_updateclientcredentials_retry_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.utils.test.tsui/app/workspace/mcp-registry/views/mcpClientsTable.utils.ts
Comment on lines
+326
to
+409
| func TestUpdateClientCredentials_PerCallSharedOAuth_PendingVerification_TransitionsToHealthy(t *testing.T) { | ||
| m := NewMCPManager(context.Background(), schemas.MCPConfig{}, nil, nil, nil) | ||
| config := &schemas.MCPClientConfig{ | ||
| ID: "client-pending-percall", | ||
| Name: "pending-percall-client", | ||
| AuthType: schemas.MCPAuthTypeOauth, | ||
| ConnectionType: schemas.MCPConnectionTypeHTTP, | ||
| } | ||
|
|
||
| m.mu.Lock() | ||
| m.clientMap[config.ID] = &schemas.MCPClientState{ | ||
| Name: config.Name, | ||
| ExecutionConfig: config, | ||
| State: schemas.MCPConnectionStatePendingVerification, | ||
| ToolMap: make(map[string]schemas.ChatTool), | ||
| ToolNameMapping: make(map[string]string), | ||
| } | ||
| m.mu.Unlock() | ||
|
|
||
| err := m.UpdateClientCredentials(config.ID, config) | ||
| require.NoError(t, err, "the first connection out of pending_verification must succeed, not report not-applicable") | ||
|
|
||
| m.mu.RLock() | ||
| state := *m.clientMap[config.ID] | ||
| m.mu.RUnlock() | ||
| assert.Equal(t, schemas.MCPConnectionStateHealthy, state.State, "must transition out of pending_verification, mirroring AddClient's own per-call setup") | ||
|
|
||
| // Without a connection checker, nothing would ever discover this | ||
| // client's tools afterward (no persistent Conn, no DiscoveredTools to | ||
| // restore for a client completing its first connection here) — see | ||
| // TestAddClient_PerCallConnection_StartsConnectionChecker for the | ||
| // AddClient-side counterpart of this same fix. | ||
| m.checkerManager.mu.RLock() | ||
| _, hasChecker := m.checkerManager.checkers[config.ID] | ||
| m.checkerManager.mu.RUnlock() | ||
| assert.True(t, hasChecker, "must start a connection checker so tools actually get discovered") | ||
|
|
||
| // A second call, now that the client is already Healthy, is the plain | ||
| // reauthorize case: genuinely nothing left to do. | ||
| err = m.UpdateClientCredentials(config.ID, config) | ||
| require.Error(t, err) | ||
| assert.True(t, errors.Is(err, schemas.ErrMCPReconnectNotApplicable)) | ||
| } | ||
|
|
||
| // TestUpdateClientCredentials_PerCallSharedType_PendingVerification_DiscoversToolsSynchronously | ||
| // pins the second half of the same bug report: a connection checker alone | ||
| // is not enough, because ClientConnectionChecker.Start uses the SLOW | ||
| // healthyInterval (not the fast Unstable one) for a client that already | ||
| // starts Healthy — so without a synchronous first discovery pass, a shared | ||
| // oauth/headers/none per-call client completing OAuth/verification here | ||
| // would sit at Healthy/0-tools for up to healthyInterval before its first | ||
| // real check ever ran. | ||
| func TestUpdateClientCredentials_PerCallSharedType_PendingVerification_DiscoversToolsSynchronously(t *testing.T) { | ||
| ts, _ := buildAdminDiscoveryHTTPServer(t) | ||
|
|
||
| m := NewMCPManager(context.Background(), schemas.MCPConfig{}, nil, nil, nil) | ||
| config := &schemas.MCPClientConfig{ | ||
| ID: "client-pending-percall-sync", | ||
| Name: "pending-percall-client-sync", | ||
| AuthType: schemas.MCPAuthTypeHeaders, | ||
| ConnectionType: schemas.MCPConnectionTypeHTTP, | ||
| ConnectionString: schemas.NewSecretVar(ts.URL), | ||
| Headers: map[string]schemas.SecretVar{"Authorization": *schemas.NewSecretVar("Bearer shared-update-token")}, | ||
| } | ||
|
|
||
| m.mu.Lock() | ||
| m.clientMap[config.ID] = &schemas.MCPClientState{ | ||
| Name: config.Name, | ||
| ExecutionConfig: config, | ||
| State: schemas.MCPConnectionStatePendingVerification, | ||
| ToolMap: make(map[string]schemas.ChatTool), | ||
| ToolNameMapping: make(map[string]string), | ||
| } | ||
| m.mu.Unlock() | ||
|
|
||
| err := m.UpdateClientCredentials(config.ID, config) | ||
| require.NoError(t, err) | ||
|
|
||
| m.mu.RLock() | ||
| state := *m.clientMap[config.ID] | ||
| m.mu.RUnlock() | ||
| assert.Equal(t, schemas.MCPConnectionStateHealthy, state.State) | ||
| assert.Contains(t, state.ToolMap, "pending-percall-client-sync-echo", "tools must be discovered synchronously, not deferred entirely to the periodic checker") | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Stop the connection checkers after each test.
These tests call UpdateClientCredentials, which starts a connection checker. The test-local MCPManager remains active after the test returns. The HTTP-backed test can continue to run after its test server closes.
Register m.Cleanup() with t.Cleanup immediately after NewMCPManager.
Proposed fix
m := NewMCPManager(context.Background(), schemas.MCPConfig{}, nil, nil, nil)
+ t.Cleanup(func() {
+ require.NoError(t, m.Cleanup())
+ })🤖 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/clientmanager_test.go` around lines 326 - 409, Register the test
manager for cleanup immediately after each NewMCPManager call in the two tests,
using t.Cleanup to invoke m.Cleanup before the test exits. Ensure the connection
checkers started by UpdateClientCredentials are stopped, including for the
HTTP-backed discovery test.
Pratham-Mishra04
changed the base branch from
08-06-feat_add_vk_and_users_filters_to_oauth_grants_sidebar
to
graphite-base/5970
August 8, 2026 11:33
…d-OAuth reconnect/verify errors
Pratham-Mishra04
force-pushed
the
08-07-fix_discover_tools_synchronously_for_per-call_mcp_clients_fix_shared-oauth_reconnect_verify_errors
branch
from
August 8, 2026 11:36
bdd683f to
22f7baa
Compare
Pratham-Mishra04
deleted the
08-07-fix_discover_tools_synchronously_for_per-call_mcp_clients_fix_shared-oauth_reconnect_verify_errors
branch
August 8, 2026 11:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a family of related bugs affecting shared MCP clients (
auth_type: oauth,headers, ornone) running in per-call mode (needs_session_stickinessnil/false): these clients never had their tools discovered after registration, got stuck inpending_verificationafter completing OAuth, and were incorrectly described as "per-user auth clients" in error messages. Additionally renamesUpdateClientConnection→UpdateClientCredentialsacross the stack for clarity.Changes
performAdminToolDiscoverypreviously rejectedoauth,headers, andnoneauth types outright. It now routes them through the same bearer/headers dispatch paths as their per-user counterparts, sinceAdminConnectionHeadersfor these resolvers now delegates toConnectionHeaderswhen the client is per-call.AdminConnectionHeaderson shared resolvers (sharedOAuthResolver,sharedHeadersResolver,noneResolver): changed from unconditional errors to delegating toConnectionHeadersfor per-call clients, and erroring only for sticky clients (which should never reach this method).VerifyHeadersConnectionguard relaxed: the "user headers are required" guard now only applies whenauth_type == per_user_headers; shared headers/none clients may legitimately resolve empty headers.AddClientandUpdateClientCredentials: shared per-call clients with noDiscoveredToolsto restore now get an immediate tool discovery pass rather than waiting for the periodic checker's first slow tick (which useshealthyInterval, not the fast unstable interval).AddClient: the per-call branch ofAddClientnow starts aClientConnectionChecker, matching whatconnectToMCPClientalready does for sticky clients.UpdateClientCredentialshandlesPendingVerificationper-call clients: a per-call client completing its first OAuth flow (still inpending_verification) is now correctly transitioned toHealthywith a checker started, instead of being stuck permanently.ErrMCPReconnectNotApplicablereturned for shared per-call clients:ReconnectClient,CloseAndMarkNeedsReauth, andUpdateClientCredentialsnow return the same sentinel for shared per-call clients as for genuine per-user clients, with corrected error messages that no longer say "per-user auth clients."ErrMCPReconnectNotApplicableas a non-error:completeMCPClientOAuthno longer rolls back the DB update or returns a 500 when the client is per-call — the fresh credential is already live for the next dial.ErrMCPReconnectNotApplicable:updateMCPClientCredentialsWithRetryreturns immediately on this sentinel instead of burning retry attempts (the sentinel's message contains "reconnect", which previously matched the transient-retry substring check).UpdateClientConnection→UpdateClientCredentialsacrosscore/bifrost.go,core/mcp/clientmanager.go,core/mcp/interface.go,transports/bifrost-http/handlers/mcp.go,transports/bifrost-http/lib/config.go, andtransports/bifrost-http/server/server.go.canReconnectMCPClientutility: replaces the inlineisPerUserAuthcheck with a function that mirrorscredstore.RequiresPerCallConnectionexactly — shared HTTP clients withneeds_session_stickinessnot explicitlytrueare also excluded from the Reconnect action.sticky top-0 z-20with correctbg-muted(notbg-muted/50), and the table container usesoverflow-hidden/overflow-autocorrectly to keep the header pinned.maps.CopyinSetClientTools: replaces the manual loop.Type of change
Affected areas
How to test
Manual verification:
oauthorheadersMCP client withneeds_session_stickinessunset (default). Confirm tools are populated immediately after registration without waiting for the periodic checker.pending_verificationtohealthywith tools discovered, and does not return a 500.ErrMCPReconnectNotApplicable(not an opaque error).Breaking changes
UpdateClientConnection/UpdateMCPClientConnection/UpdateClientConnectionare renamed toUpdateClientCredentials/UpdateMCPClientCredentials/UpdateClientCredentialseverywhere. Any external code calling these methods by name must be updated.Security considerations
AdminConnectionHeadersfor shared resolvers now resolves the same credential a real tool call would (no separate admin credential exists for these types). This is intentional and consistent with how per-call tool discovery already works for per-user auth types. Sticky clients are explicitly guarded and will still error if they somehow reach this path.Checklist
docs/contributing/README.mdand followed the guidelines