📣 fix: Surface MCP Credential Failures During Tool Loading - #16323
Conversation
|
Review handoff for exact pushed head This head preserves typed MCP credential failures through all-tools and selected-tool loading and the outer agent loader. Outstanding MCP loads settle before the original fatal outcome is thrown. Ordinary optional-tool failures stay non-fatal; silent refresh and credential persistence are unchanged. Evidence: 19 new regression cases fail against unpatched dev and pass with this fix. The final focused runs passed 509 tests across eight suites. API-workspace TypeScript, touched-file ESLint/import order/formatting, static checks, and builds passed. Lighthouse was attempted but Chromium cannot launch on this worker without Self-review covered primary/subagent fallback, cancellation ownership, overlapping server loads, recovery/retry policy, permissions, persistence, and generation-error serialization. Generation tests confirm the actionable error is carried in the stream without an HTTP 401 triggering application-auth recovery. A maintainer needs to trigger the external review. No external review is claimed for this head yet. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 886eea35d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const loadedTools = (await Promise.all(toolPromises)).flatMap((plugin) => plugin || []); | ||
| const mcpToolPromises = []; | ||
| const mcpLoadGuard = createMCPToolLoadGuard(signal); |
There was a problem hiding this comment.
Move the MCP failure-guard lifecycle into a TypeScript helper under packages/api and leave this file to invoke it. Creating the guard here, deciding which failures to capture in three catch paths, and determining when to rethrow adds error-handling behavior to /api, while the repository explicitly requires CJS files in this tree to remain wiring-only even when editing existing files.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2e2ea88. The full loading lifecycle is now in packages/api/src/mcp/loading.ts: scheduling, catalog reuse, fatal-error capture, settlement, and cancellation precedence. handleTools.js supplies dependencies and request context in one loadMCPTools call. Direct TypeScript tests exercise the lifecycle, while adapter tests exercise the real built export. The catalog user ID is forwarded independently from the safe tool-user context to preserve the previous identity contract.
| if (!failure && isMCPInitializationError(error, signal)) { | ||
| failure = { error }; |
There was a problem hiding this comment.
When one parallel MCP load records a credential failure and the user presses Stop while another load is still settling, this first-failure check ignores the subsequent owned abort and throwIfFailed() returns the credential error. In the resumable agent path that sends execution through the initialization-error branch instead of the existing post-initialization abort handling, so a canceled turn can be published as an authentication failure; prefer the owning signal's cancellation at completion, or replace a stored credential failure when an owned abort is captured.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2e2ea88. After every started MCP load settles, the TypeScript loader checks the request signal before rethrowing any saved credential error or returning tools. It also stops starting new tools after cancellation, including cancellation during catalog loading. Regressions cover an earlier auth failure plus later cancellation, all-successful siblings plus cancellation, already-aborted requests, and cancellation during catalog discovery. Both new adapter cancellation cases failed on the prior head.
| const code = getErrorCode(error); | ||
| return ( | ||
| isOwnedAbortError(error, options.signal) || | ||
| isMCPInitializationError(error, options.signal) || |
There was a problem hiding this comment.
For an OBO-configured MCP server, this newly delegates fatal classification to isMCPInitializationError, which includes OboTokenResolutionError; however, that error class has neither status/statusCode nor code, only reason and retryable. The agent controller's getInitializationFailure() therefore cannot classify it, so both an expired sign-in and a transient exchange outage become the same untyped 500/plain job error and the retryability signal is discarded; normalize these outcomes to the corresponding authentication/refresh response before propagating them.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2e2ea88. Shared TypeScript getAgentErrorMetadata classifies OBO outcomes at all three outward API boundaries while preserving the original thrown error: retryable failures become 503 / MCP_AUTHENTICATION_REFRESH_FAILED; terminal failures become 403 / MCP_AUTHENTICATION_REJECTED. The UI stream also retains retryable. Terminal upstream failure is not conflated with a LibreChat session-expiry 401. Tests cover both OBO outcomes in the UI stream and both remote APIs, with negative controls on the previous head.
| const code = getErrorCode(error); | ||
| return ( | ||
| isOwnedAbortError(error, options.signal) || | ||
| isMCPInitializationError(error, options.signal) || |
There was a problem hiding this comment.
For /v1/chat/completions and the Responses API, this new fatal classification makes OpenIDReauthRequiredError, MCPAuthenticationRejectedError, and MCPAuthenticationRefreshError escape createToolLoader; their execution-error handlers inspect only error.status, while all three classes expose their intended 401/403/503 through statusCode. Consequently these newly surfaced credential failures are returned as HTTP 500 on both remote-agent ingresses, so use the same status ?? statusCode normalization as the resumable controller before returning them.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2e2ea88. Chat Completions and Responses now use the same TypeScript metadata adapter, honoring validated status and statusCode and preserving the credential code. Both endpoints have regression matrices for OpenID reauth (401), rejected MCP credentials (403), temporary refresh failure (503), and both OBO outcomes. Existing provider-error masking and code-only errors are retained. These remote response cases failed on the previous head and pass with the fix.
|
Review handoff for exact pushed head This head addresses all four findings on
Verified 679 tests across 13 focused suites, Lighthouse remains blocked: Chromium cannot launch without A maintainer needs to trigger the next external review. No review of this new head is claimed yet. |
|
@codex review the latest head |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Related to #16319 and #16318. The report does not identify a LibreChat version or which credential expired, so this fixes the independently reproduced propagation gap, not every possible expired-JWT configuration.
Existing MCP OAuth and direct-OpenID recovery should still refresh recoverable credentials silently. If recovery instead produces a typed credential failure, the MCP tool loader currently logs it and returns an incomplete toolset. Rethrowing there alone is insufficient: the outer agent loader also treats that error as an optional-tool failure.
No token-refresh policy, credential persistence, permissions, config, frontend code, or automatic logout behavior changes. The existing generation stream carries the actionable failure without an HTTP 401 that would trigger application-token recovery.
Verification
56301c1b5a32f37e5c59e09f0c8567f559ee0b76: 19 regression cases fail because errors are swallowed; the ordinary optional-tool control passes. Fixed source restored and rebuilt afterward.handleTools.test.js,Endpoints/agents/initialize.spec.js,request.resumeMetadata.spec.js,request.partialDisconnect.spec.js,handleTools.rolePermissions.test.js,openai.spec.js,responses.unit.spec.js, andMCP.spec.js.agents/errors.spec.ts,mcp/errors.spec.ts,mcp/openid.spec.ts,agents/discovery.spec.ts, andmcp/loading.spec.ts.npx tsc --noEmitinpackages/api: passed.npm run static-checks, andgit diff --check: passed.npm run lighthouse: attempted. The production build and local server startup succeeded, but Chromium could not launch because the worker lackslibatk-1.0.so.0. No Lighthouse score or budget result was produced. System libraries were not modified.Tests use a disposable MongoDB with its Unix socket disabled because this worker disallows the default
/tmpsocket. No database or machine service was modified.Invariant Review
Reviewed both tool-loading modes, catalog discovery, primary/subagent fallback readers, request-owned cancellation, overlapping server loads, and generation-error serialization. The guard is per invocation, retains the first typed outcome, and has no persistent state. Existing silent-recovery tests remain green. No stored-data migration or mixed-version schema change is introduced; the API package and JS adapter must be built/deployed together as usual.
Not run: manual expired-token testing against an external identity provider, browser end-to-end reauthentication, full repository test suites. This PR does not establish the original reporter's authentication configuration.
Review Follow-up
All four findings on
886eea35d45b95f464f2808a4400de3ee04d0d43were valid and are addressed:packages/api/src/mcp/loading.ts. LegacyhandleTools.jsonly supplies the loaders and request context. The temporary exported guard was removed after checking graph consumers and string references.MCP_AUTHENTICATION_REJECTED, and retryable refresh/exchange failure to 503 withMCP_AUTHENTICATION_REFRESH_FAILED. The UI stream retains retryability without treating an upstream failure as a LibreChat session expiry.statusCodevalues are no longer degraded to 500. Existing error masking and code-only provider error behavior are preserved.Negative controls on the previous pushed head: two cancellation regressions and twelve API response regressions failed before the fix. The final focused set passes 679 tests across 13 suites. Existing silent OpenID recovery, optional-tool fallback, permission filtering, primary/subagent behavior, and request/disconnect tests remain green.
The invariant review covered all three API entry points, request-owned cancellation, catalog reuse, outstanding sibling settlement, retryability, authorization, cleanup, and deployment compatibility. No new persisted state is introduced. The API package and JS adapters ship together.
Lighthouse browser preflight was rechecked for this round and still fails because
libatk-1.0.so.0is absent. The full Lighthouse command was not repeated on this follow-up; no new performance result is claimed. Manual identity-provider expiry, browser reauthentication E2E, and the full repository test suite remain unrun.