refactor: replace OAuth2 token-expired text matching with errors.Is and preserve last-known tool map on disconnect

[Pratham-Mishra04]

Summary

When an MCP client connection fails, the tool map was being cleared, making a dead connection indistinguishable from a tool that never existed. This caused callers to receive a generic "not available or not permitted" error rather than an actionable "client needs re-authorization" or "client is disconnected" message. Additionally, the OAuth2 token expiry detection relied on fragile substring matching against a flattened error string because runConnectWithPluginPipeline was not preserving the original Go error on ErrorField.Error. This PR fixes both issues.

Changes

• Preserve last-known tool map on connection failure: failConnectAttempt no longer clears ToolMap/ToolNameMapping when a connection fails. The maps are left as last-known-good so GetClientForTool can still resolve the tool and route it to the correct client. GetToolPerClient already filters on State directly, so disconnected clients' tools are not re-advertised.

• Add explicit state checks in prepareToolExecution: Before falling through to AcquireClientConn, the execution path now checks for NeedsReauth and Disconnected states and returns specific, actionable error messages rather than a generic "no active connection" error.

• Preserve the original error on BifrostError in runConnectWithPluginPipeline: Both early-return and late-return error paths now set ErrorField.Error alongside ErrorField.Message, enabling errors.Is/errors.As classification by callers.

• Replace text-matching OAuth2 expiry detection with errors.Is: connectToMCPClient now uses errors.Is(gateErr.Error.Error, schemas.ErrOAuth2TokenExpired) instead of the isOAuth2TokenExpiredErrorText substring matcher, which was only necessary because the original error was being discarded.

• Remove isOAuth2TokenExpiredErrorText and its tests: The function and its associated test coverage are deleted now that the typed error is preserved through the pipeline. The TestIsAuthFailureErrorText_DoesNotOverlapOAuth2TokenExpiredSentinel test is simplified to only assert the negative case, since there is no longer a separate text-matching function to validate the positive case for.

• Update make-before-break test assertions: The reconnect failure test now asserts that the tool map is retained (NotEmpty) rather than cleared (Empty), reflecting the new intended behavior.

Type of change

• Bug fix
• Refactor

Affected areas

• Core (Go)

How to test

go version
go test ./...

The makebeforebreak_test.go and reauth_state_test.go tests directly cover the changed behavior. After a simulated dial failure, verify that:

• The client entry retains its tool map.
• Attempting to execute a tool on a NeedsReauth client returns a message containing "needs re-authorization".
• Attempting to execute a tool on a Disconnected client returns a message containing "is disconnected".

Breaking changes

• Yes
• No

Security considerations

None. The change improves error message clarity for expired OAuth2 credentials but does not alter authentication logic or expose new information externally.

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.}

[coderabbitai]

Actionable comments posted: 3

🤖 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.go`:
- Around line 2015-2023: Preserve last-known tool mappings across every failed
reconnect: in core/mcp/clientmanager.go lines 2015-2023, avoid clearing ToolMap
and ToolNameMapping during close-first initialization and replace them only
after successful tool discovery; in core/mcp/makebeforebreak_test.go lines
360-367, add a second failed reconnect and assert both mappings still equal
their original last-known values.

In `@core/mcp/exec.go`:
- Around line 158-169: Move the NeedsReauth and Disconnected checks in the tool
execution flow after the global, client, tool, and request authorization
filters, including MCPContextKeyIncludeClients, ToolsToExecute, and
MCPContextKeyIncludeTools. Ensure the state-specific errors in the relevant
exec.go function are returned only after the tool invocation is authorized,
while preserving their existing messages and behavior.

In `@core/mcp/makebeforebreak_test.go`:
- Around line 360-367: Add a second reconnect attempt in the existing
make-before-break failure test, exercising the close-first path, and configure
it to fail. Capture the tool map and tool-name mapping before this attempt, then
assert afterward that both ToolMap and ToolNameMapping are unchanged from those
pre-failure values.

🪄 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: 4fe94bca-fc74-460c-bf85-cdd5ad3cc9bc

📥 Commits

Reviewing files that changed from the base of the PR and between 1832ada and acafca2.

📒 Files selected for processing (7)

• core/mcp/auth_retry_test.go
• core/mcp/clientmanager.go
• core/mcp/exec.go
• core/mcp/makebeforebreak_test.go
• core/mcp/pluginpipeline.go
• core/mcp/reauth_state_test.go
• core/mcp/utils.go

💤 Files with no reviewable changes (1)

• core/mcp/reauth_state_test.go

[coderabbitai]

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.

[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/mcp/clientmanager.go`:
- Around line 2021-2029: Update EnableClient to call beginExclusiveClientOp
before clearing ExecutionConfig.Disabled, and retain the exclusive-operation
guard through the entire enable flow. Ensure concurrent ReconnectClient calls
cannot observe or connect a client while it remains disabled, while preserving
the existing “already in progress” error behavior.

🪄 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: 610512b6-35db-4571-8e05-745ad62a8745

📥 Commits

Reviewing files that changed from the base of the PR and between 1179fbb and c65f7f1.

📒 Files selected for processing (7)

• core/mcp/auth_retry_test.go
• core/mcp/clientmanager.go
• core/mcp/exec.go
• core/mcp/makebeforebreak_test.go
• core/mcp/pluginpipeline.go
• core/mcp/reauth_state_test.go
• core/mcp/utils.go

💤 Files with no reviewable changes (1)

• core/mcp/reauth_state_test.go

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

• core/mcp/exec.go
• core/mcp/makebeforebreak_test.go
• core/mcp/utils.go
• core/mcp/auth_retry_test.go
• core/mcp/pluginpipeline.go

[Pratham-Mishra04]

Merge activity

• Aug 8, 10:33 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• Aug 8, 10:59 AM UTC: Graphite rebased this pull request as part of a merge.
• Aug 8, 11:00 AM UTC: @Pratham-Mishra04 merged this pull request with Graphite.

The base branch was changed.