feat: add NeedsReauth MCP connection state for dead OAuth2 credentials - #5712
Conversation
|
|
oauth_configs to new mcp_oauth_flows table
#5709
📝 WalkthroughSummary by CodeRabbit
WalkthroughShared MCP connections now expose ChangesMCP reauthentication lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthCredentialStore
participant connectToMCPClient
participant MCPClientManager
OAuthCredentialStore->>connectToMCPClient: return expired OAuth credential error
connectToMCPClient->>MCPClientManager: report connection failure
MCPClientManager->>MCPClientManager: set NeedsReauth
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ce4a9b1 to
fe261ef
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
b00a267 to
e214e46
Compare
fe261ef to
bd1ded9
Compare
Merge activity
|
The base branch was changed.
bd1ded9 to
3953567
Compare

Summary
When a shared-connection MCP client's OAuth2 credential permanently dies (refresh token rejected or expired), the client was being left in the generic
Disconnectedstate. This is misleading because no amount of automatic reconnection will fix it — only a human reauthorizing the client will. This PR introduces a newNeedsReauthconnection state that is set whenconnectToMCPClientdetects anErrOAuth2TokenExpired-wrapped failure, and ensures the health monitor does not silently overwrite it withConnectedorDisconnectedduring subsequent ping cycles.Changes
MCPConnectionStateNeedsReauthto theMCPConnectionStateenum inschemas/mcp.go, with documentation distinguishing it fromPendingVerificationand noting it only applies to shared-connection auth types.connectToMCPClient, after a gate error is returned, the error string is checked againstErrOAuth2TokenExpiredusing the newisOAuth2TokenExpiredErrorTexthelper. If matched, the client state is flipped toNeedsReauthunder the manager lock, mirroring how the success path setsConnected.EnableClient, the post-connect failure state update was changed from anif/elseto aswitchso thatNeedsReauth(already written byconnectToMCPClientunder its own lock) is preserved rather than overwritten withDisconnected.updateClientStateon the health monitor, the existingDisabledguard was extended to also bail out early forNeedsReauth, preventing health-check ticks from clobbering the more specific signal.isOAuth2TokenExpiredErrorTextinutils.go, using the same substring-matching technique asisTransientErrorbecauserunConnectWithPluginPipelineflattens the underlying Go error to a string on theBifrostErrorit returns, makingerrors.Is/errors.Asunusable at the call site.reauth_state_test.gocovering: theisOAuth2TokenExpiredErrorTexthelper, theconnectToMCPClientpath that setsNeedsReauthon OAuth expiry, the control case that generic failures stayDisconnected, the health monitor guard that preservesNeedsReauthagainst both failed and successful ping ticks, and a regression guard confirming the pre-existingDisabledpreservation behavior is unchanged.Type of change
Affected areas
How to test
go version go test ./core/mcp/...The new
reauth_state_test.gofile covers the primary scenarios:ErrOAuth2TokenExpired-wrapped error onConnectionHeadersshould land inMCPConnectionStateNeedsReauthafterconnectToMCPClientreturns.MCPConnectionStateDisconnected.NeedsReauthshould not be moved toConnectedorDisconnectedby health monitor ticks.Disabledshould still be unaffected by health monitor ticks (regression guard).Breaking changes
The new
needs_reauthstate string is additive. Existing clients inDisconnected,Disabled, or other states are unaffected.Security considerations
This change improves visibility into dead OAuth2 credentials by surfacing them as a distinct state rather than silently retrying forever. No secrets or PII are exposed; the state transition is based solely on error text matching against a known sentinel error string.
Checklist
docs/contributing/README.mdand followed the guidelines