Unify MCP connect reporting and add OAuth strategy support#1793
Open
chelojimenez wants to merge 4 commits intomainfrom
Open
Unify MCP connect reporting and add OAuth strategy support#1793chelojimenez wants to merge 4 commits intomainfrom
chelojimenez wants to merge 4 commits intomainfrom
Conversation
dosubot
bot
added
the
size:XXL
Contributor
Author
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
mcpjam-inspector/client/src/hooks/use-explore-cases-prefetch-on-connect.ts
Outdated
Show resolved
Hide resolved
mcpjam-inspector/client/src/components/ActiveServerSelector.tsx
Outdated
Show resolved
Hide resolved
Contributor
WalkthroughThe PR replaces ad-hoc connection flows with a centralized Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/client/src/hooks/use-explore-cases-prefetch-on-connect.ts (1)
70-76:⚠️ Potential issue | 🟠 MajorConnected-status logic is inconsistent and drops
partialprefetch.You mark pending when entering a connected-like state, but immediately clear/return unless status is exactly
"connected". That prevents prefetch on"partial"transitions.✅ Suggested correction
- if (isConnectedStatus(status) && !isConnectedStatus(prev as any)) { + if (isConnectedStatus(status) && !isConnectedStatus(prev)) { pendingExplorePrefetchRef.current = true; } - if (status !== "connected") { + if (!isConnectedStatus(status)) { pendingExplorePrefetchRef.current = false; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-explore-cases-prefetch-on-connect.ts` around lines 70 - 76, The code sets pendingExplorePrefetchRef.current = true when entering any connected-like state via isConnectedStatus(status) but then immediately clears it unless status === "connected", which drops transitions to "partial"; change the clearing check to use the same predicate — clear and return only when !isConnectedStatus(status) — so pending is preserved for "partial" and other connected-like states; update the conditional using isConnectedStatus(status) (and keep the existing isConnectedStatus(prev) check) around pendingExplorePrefetchRef.current handling.
🧹 Nitpick comments (3)
mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts (1)
511-512: Unused constructor parameter_protocolVersion.The
_protocolVersionparameter is accepted but never stored or used withinMCPOAuthProvider. If it's intentionally reserved for future use, consider adding a brief comment. Otherwise, it may be omitted to reduce confusion.✨ Proposed simplification if not needed
constructor( serverName: string, serverUrl: string, customClientId?: string, customClientSecret?: string, - _protocolVersion?: OAuthProtocolVersion, registrationStrategy?: OAuthRegistrationStrategy, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts` around lines 511 - 512, The constructor of MCPOAuthProvider currently accepts an unused parameter _protocolVersion of type OAuthProtocolVersion; either remove this parameter from the constructor signature (and any callers) to eliminate confusion, or explicitly store or document it: e.g., assign it to a private field on MCPOAuthProvider or add a one-line comment explaining it’s reserved for future use; update the constructor signature and any references to OAuthProtocolVersion/OAuthRegistrationStrategy accordingly so the code and intent are consistent.mcpjam-inspector/client/src/components/connection/AddServerModal.tsx (1)
289-294: Clarify the preregistered strategy guard.The early return prevents disabling custom client ID when
oauthRegistrationStrategy === "preregistered", which is semantically correct—preregistered clients require explicit credentials. A brief comment would illuminate this constraint:Suggested refinement
onUseCustomClientIdChange={(checked) => { + // Preregistered strategy mandates custom client credentials if ( formState.oauthRegistrationStrategy === "preregistered" && !checked ) { return; } formState.setUseCustomClientId(checked);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/connection/AddServerModal.tsx` around lines 289 - 294, The early-return guard inside the AddServerModal that checks formState.oauthRegistrationStrategy === "preregistered" && !checked should be annotated with a short comment explaining that preregistered strategy requires explicit client credentials and therefore prevents toggling off the custom client ID control; update the block near the toggle handler (referencing formState.oauthRegistrationStrategy, "preregistered", and checked) to include this clarifying comment so future readers understand why the return is necessary and not a bug.mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsx (1)
46-47: Consider a top-level import forOAuthTestProfile.The inline
import("@/lib/oauth/profile").OAuthTestProfileworks, but a standard import at the file's header would align with the rest of the codebase:import type { OAuthTestProfile } from "@/lib/oauth/profile";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsx` around lines 46 - 47, Replace the inline type import in the function signature that uses options?: { oauthProfile?: import("@/lib/oauth/profile").OAuthTestProfile } with a top-level type import; add import type { OAuthTestProfile } from "@/lib/oauth/profile" at the file header and update the signature to reference OAuthTestProfile (e.g., options?: { oauthProfile?: OAuthTestProfile }) so the file uses the named type like the rest of the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/commands/server.ts`:
- Around line 335-336: The current exit logic treats result.success as
sufficient even when validation is "partial"; update the check after
connectServerWithReport() so that a "partial" validation is considered failing:
inspect the returned result (the object from connectServerWithReport()) and call
setProcessExitCode(1) when either result.success is false OR result.validation
=== 'partial' (or equivalent enum/string used by the function), ensuring server
validate returns non-zero for degraded/partial validations.
In `@mcpjam-inspector/client/src/components/connection/server-card-utils.ts`:
- Around line 20-25: The partial object in server-card-utils.ts references the
AlertCircle component but it isn’t imported; add AlertCircle to the existing
import list from 'lucide-react' (e.g., include AlertCircle alongside other
icons) so the Icon: AlertCircle usage resolves and the module compiles.
In
`@mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx`:
- Around line 145-148: AuthenticationSection.tsx's SelectContent currently lists
only "2025-06-18" and "2025-11-25", but the SDK and the mcp-oauth.ts type guard
also support "2025-03-26" (used as legacy in OAuthProfileModal), so add a
SelectItem with value "2025-03-26" to this SelectContent to keep versions
consistent; update the SelectContent/SelectItem block in AuthenticationSection
(and ensure any nearby validation/labels in the component reflect legacy wording
if needed) so the dropdown includes "2025-03-26" alongside the other two
versions.
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 2035-2047: The fast-path reconnect is calling
guardedTestConnection with originalServer.config, so header/timeout/capability
edits in mcpConfig aren’t actually validated; change the guardedTestConnection
invocation (the call that currently wraps
withWorkspaceClientCapabilities(originalServer.config)) to use the edited
mcpConfig (wrapped by withWorkspaceClientCapabilities(mcpConfig)) so the live
connection test exercises the updated config; ensure this change is applied
wherever the fast-path reconnect uses originalServer.config and that the
CONNECT_SUCCESS path persists only after the guardedTestConnection on mcpConfig
succeeds.
- Around line 521-560: completeConnection currently treats any result.success as
full success; change the success check to exclude partial validation results by
checking result.status !== "partial" (i.e., use if (result.success &&
result.status !== "partial") ) so that partial results are handled by the
failure branch. Update the logic around dispatching CONNECT_SUCCESS, calling
storeInitInfo, and showing successToast to only run for non-partial successes,
leaving partial responses to dispatch CONNECT_FAILURE (still including
result.report) and to trigger failureToast handling as it does now; reference
completeConnection, ConnectionApiResponse, and result.status/"partial".
In `@mcpjam-inspector/client/src/state/app-reducer.ts`:
- Around line 73-75: The reducer currently only spreads initializationInfo when
action.report.initInfo is truthy, leaving stale data when a new report has
initInfo: null; update the reducer to always set initializationInfo from
action.report (use initializationInfo: action.report?.initInfo) so the field is
overwritten to null when the report provides null; locate the update in the app
reducer where the report merge occurs (the block using
...(action.report?.initInfo ? { initializationInfo: action.report.initInfo } :
{})) and replace the conditional spread with an unconditional initializationInfo
assignment.
In `@mcpjam-inspector/client/src/state/oauth-orchestrator.ts`:
- Around line 77-80: The current expression treats an empty scopes string as a
present value because split(...).filter(...) yields [] which is truthy, so
change the logic to detect an absent or empty trimmed scopes string before
splitting: check oauthProfile?.scopes?.trim() (or test the resulting array
length) and if non-empty perform the split/filter, otherwise fall back to
oauthConfig.scopes; update the code around oauthProfile.scopes and
oauthConfig.scopes accordingly (e.g., use a conditional that picks
oauthConfig.scopes when oauthProfile?.scopes is null/undefined or when
oauthProfile.scopes.trim() === "").
In `@mcpjam-inspector/server/routes/mcp/connect.ts`:
- Around line 11-15: Separate parsing of the incoming JSON from the call to
connectServerWithReport: wrap c.req.json() in its own try/catch and return a 400
with a "Failed to parse request body" message only when that parse fails; then
call connectServerWithReport(serverConfig, serverId, oauthContext, ...) in a
separate try/catch and return a different error response (e.g., 500 or a
descriptive connect error) if that call throws. Update both the first block
around c.req.json() and the similar logic later around connectServerWithReport
(lines covering the other occurrence) so parsing errors and connect errors are
handled and logged distinctly using the same function names (c.req.json and
connectServerWithReport) to find the code.
In `@mcpjam-inspector/server/routes/web/servers.ts`:
- Around line 147-160: The current validation silently returns undefined for
unsupported protocolVersion and registrationStrategy; instead, detect when
protocolVersion is not one of "2025-06-18" or "2025-11-25" or when
registrationStrategy is not one of "dcr", "preregistered", "cimd" and explicitly
reject the request by throwing or returning a Bad Request error (HTTP 400) with
a clear message referencing the invalid field and its value; update the helper
that performs these checks (the block validating protocolVersion and
registrationStrategy) to raise/propagate an error (or call next(err) in Express)
rather than returning undefined so callers handle invalid OAuth context as a
failure.
In `@sdk/src/connect-report.ts`:
- Around line 179-194: The code infers OAUTH_REQUIRED too broadly by using
hasConnectionCredentials() (stored in hasCredentials) which treats any static
custom header as credentials; update the logic to only treat OAuth-style
credentials as OAuth-specific (introduce or use a helper like isOAuthCredentials
or refine hasConnectionCredentials to detect only access_token/refresh_token or
Authorization: Bearer) and change the branch in the auth.isAuth handling (and
the similar block around the other auth handling at the second occurrence) to
return "OAUTH_REQUIRED" only when OAuth-style credentials are present, otherwise
classify as "AUTH_ERROR" (preserving statusCode/retryable behavior). Ensure you
update both places (the block using hasCredentials near auth.isAuth and the
repeated block at the later lines) to reference the new OAuth-specific check
(e.g., replace hasCredentials with
isOAuthCredentials(hasConnectionCredentialsDetail) or adjust
hasConnectionCredentials implementation).
- Around line 109-116: collectPartialDiagnostics is awaited directly and can
throw (e.g., when getToolsForAiSdk() is downgraded to "partial"), so wrap the
call to collectPartialDiagnostics in a try/catch similar to how failure
diagnostics are guarded: call collectPartialDiagnostics(manager, serverId,
input.target, error, dependencies.collectConnectedState ??
collectConnectedServerDoctorState, dependencies.buildConnectedDoctorResult ??
buildConnectedServerDoctorResult) inside a try block and on any exception return
a structured partial diagnostics result (mark it partial/partialReason and
include the caught error info) instead of letting the exception bubble;
reference the collectPartialDiagnostics call and the fallback helpers
collectConnectedServerDoctorState / buildConnectedServerDoctorResult when
implementing the guard.
---
Outside diff comments:
In `@mcpjam-inspector/client/src/hooks/use-explore-cases-prefetch-on-connect.ts`:
- Around line 70-76: The code sets pendingExplorePrefetchRef.current = true when
entering any connected-like state via isConnectedStatus(status) but then
immediately clears it unless status === "connected", which drops transitions to
"partial"; change the clearing check to use the same predicate — clear and
return only when !isConnectedStatus(status) — so pending is preserved for
"partial" and other connected-like states; update the conditional using
isConnectedStatus(status) (and keep the existing isConnectedStatus(prev) check)
around pendingExplorePrefetchRef.current handling.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/connection/AddServerModal.tsx`:
- Around line 289-294: The early-return guard inside the AddServerModal that
checks formState.oauthRegistrationStrategy === "preregistered" && !checked
should be annotated with a short comment explaining that preregistered strategy
requires explicit client credentials and therefore prevents toggling off the
custom client ID control; update the block near the toggle handler (referencing
formState.oauthRegistrationStrategy, "preregistered", and checked) to include
this clarifying comment so future readers understand why the return is necessary
and not a bug.
In `@mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsx`:
- Around line 46-47: Replace the inline type import in the function signature
that uses options?: { oauthProfile?:
import("@/lib/oauth/profile").OAuthTestProfile } with a top-level type import;
add import type { OAuthTestProfile } from "@/lib/oauth/profile" at the file
header and update the signature to reference OAuthTestProfile (e.g., options?: {
oauthProfile?: OAuthTestProfile }) so the file uses the named type like the rest
of the codebase.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 511-512: The constructor of MCPOAuthProvider currently accepts an
unused parameter _protocolVersion of type OAuthProtocolVersion; either remove
this parameter from the constructor signature (and any callers) to eliminate
confusion, or explicitly store or document it: e.g., assign it to a private
field on MCPOAuthProvider or add a one-line comment explaining it’s reserved for
future use; update the constructor signature and any references to
OAuthProtocolVersion/OAuthRegistrationStrategy accordingly so the code and
intent are consistent.
🪄 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: 4a0e2792-8e39-4c82-b112-1b021367cc75
📒 Files selected for processing (40)
cli/src/commands/server.tsmcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/ActiveServerSelector.tsxmcpjam-inspector/client/src/components/EvalsTab.tsxmcpjam-inspector/client/src/components/OAuthFlowTab.tsxmcpjam-inspector/client/src/components/RegistryTab.tsxmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/chat-v2/chat-input.tsxmcpjam-inspector/client/src/components/connection/AddServerModal.tsxmcpjam-inspector/client/src/components/connection/EditServerFormContent.tsxmcpjam-inspector/client/src/components/connection/ServerConnectionCard.tsxmcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/components/connection/server-card-utils.tsmcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsxmcpjam-inspector/client/src/components/logger-view.tsxmcpjam-inspector/client/src/components/mcp-sidebar.tsxmcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/use-app-state.tsmcpjam-inspector/client/src/hooks/use-eval-tab-context.tsmcpjam-inspector/client/src/hooks/use-explore-cases-prefetch-on-connect.tsmcpjam-inspector/client/src/hooks/use-onboarding.tsmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/useRegistryServers.tsmcpjam-inspector/client/src/lib/apis/web/context.tsmcpjam-inspector/client/src/lib/apis/web/servers-api.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/state/app-reducer.tsmcpjam-inspector/client/src/state/app-types.tsmcpjam-inspector/client/src/state/mcp-api.tsmcpjam-inspector/client/src/state/oauth-orchestrator.tsmcpjam-inspector/server/routes/mcp/connect.tsmcpjam-inspector/server/routes/mcp/servers.tsmcpjam-inspector/server/routes/web/servers.tssdk/src/browser.tssdk/src/connect-report-types.tssdk/src/connect-report.tssdk/src/index.tssdk/src/server-doctor.ts
Comment on lines
+145
to
+148
| <SelectContent> | ||
| <SelectItem value="2025-06-18">2025-06-18</SelectItem> | ||
| <SelectItem value="2025-11-25">2025-11-25</SelectItem> | ||
| </SelectContent> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if 2025-03-26 protocol version is used elsewhere in the codebase
rg -n "2025-03-26" --type=ts --type=tsx -g '!node_modules'Repository: MCPJam/inspector
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Search for 2025-03-26 in TypeScript files (corrected flags)
rg "2025-03-26" -t ts -g '!node_modules'Repository: MCPJam/inspector
Length of output: 6423
🏁 Script executed:
#!/bin/bash
# Find mcp-oauth.ts and check the protocol version type definition
fd "mcp-oauth" -t f && echo "---" && rg -A 5 -B 5 "protocol.*version|version.*guard" -t ts | head -100Repository: MCPJam/inspector
Length of output: 7296
🏁 Script executed:
#!/bin/bash
# Find and read AuthenticationSection.tsx around lines 145-148
fd "AuthenticationSection.tsx" -x cat {} | head -160 | tail -30Repository: MCPJam/inspector
Length of output: 1400
🏁 Script executed:
#!/bin/bash
# Read mcp-oauth.ts to see the protocol version validation
cat mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts | head -150Repository: MCPJam/inspector
Length of output: 3991
Protocol version "2025-03-26" is supported by the SDK but missing from this component's dropdown.
The type guard in mcp-oauth.ts validates all three versions: "2025-03-26", "2025-06-18", and "2025-11-25". The OAuthProfileModal component includes "2025-03-26" (labeled as legacy) in its SelectItem options, but it's absent here. Consider adding it for consistency unless there's a deliberate reason to exclude it from this specific context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx`
around lines 145 - 148, AuthenticationSection.tsx's SelectContent currently
lists only "2025-06-18" and "2025-11-25", but the SDK and the mcp-oauth.ts type
guard also support "2025-03-26" (used as legacy in OAuthProfileModal), so add a
SelectItem with value "2025-03-26" to this SelectContent to keep versions
consistent; update the SelectContent/SelectItem block in AuthenticationSection
(and ensure any nearby validation/labels in the component reflect legacy wording
if needed) so the dropdown includes "2025-03-26" alongside the other two
versions.
Comment on lines
+521
to
+560
| const completeConnection = useCallback( | ||
| async ( | ||
| serverName: string, | ||
| serverConfig: MCPServerConfig, | ||
| result: ConnectionApiResponse, | ||
| options?: { | ||
| tokens?: ReturnType<typeof getStoredTokens>; | ||
| successToast?: string; | ||
| failureToast?: string; | ||
| }, | ||
| ): Promise<boolean> => { | ||
| if (result.success) { | ||
| dispatch({ | ||
| type: "CONNECT_SUCCESS", | ||
| name: serverName, | ||
| config: serverConfig, | ||
| ...(options?.tokens ? { tokens: options.tokens } : {}), | ||
| ...(result.report ? { report: result.report } : {}), | ||
| }); | ||
| await storeInitInfo( | ||
| serverName, | ||
| result.report?.initInfo ?? result.initInfo, | ||
| ); | ||
| if (options?.successToast) { | ||
| toast.success(options.successToast); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| const errorMessage = getConnectionErrorMessage(result); | ||
| dispatch({ | ||
| type: "CONNECT_FAILURE", | ||
| name: serverName, | ||
| error: errorMessage, | ||
| ...(result.report ? { report: result.report } : {}), | ||
| }); | ||
| if (options?.failureToast) { | ||
| toast.error(options.failureToast.replace("{error}", errorMessage)); | ||
| } | ||
| return false; |
Contributor
There was a problem hiding this comment.
"partial" is being treated as full success.
The new report model uses success: true, status: "partial" for post-connect validation failures, but completeConnection() keys only off result.success. That makes callers show success toasts and skip fallback logic for a degraded connection, even though the reducer preserves "partial" as a separate state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 521 -
560, completeConnection currently treats any result.success as full success;
change the success check to exclude partial validation results by checking
result.status !== "partial" (i.e., use if (result.success && result.status !==
"partial") ) so that partial results are handled by the failure branch. Update
the logic around dispatching CONNECT_SUCCESS, calling storeInitInfo, and showing
successToast to only run for non-partial successes, leaving partial responses to
dispatch CONNECT_FAILURE (still including result.report) and to trigger
failureToast handling as it does now; reference completeConnection,
ConnectionApiResponse, and result.status/"partial".
Comment on lines
+179
to
+194
| const hasCredentials = hasConnectionCredentials(config); | ||
|
|
||
| if (auth.isAuth) { | ||
| const code: ConnectIssueCode = hasCredentials | ||
| ? "AUTH_ERROR" | ||
| : "OAUTH_REQUIRED"; | ||
| return { | ||
| code, | ||
| phase: "authorize", | ||
| message: | ||
| code === "OAUTH_REQUIRED" | ||
| ? "Server requires OAuth before it can be connected." | ||
| : normalized.message, | ||
| ...(auth.statusCode ? { statusCode: auth.statusCode } : {}), | ||
| retryable: true, | ||
| }; |
Contributor
There was a problem hiding this comment.
oauth_required is inferred too eagerly from auth failures.
hasConnectionCredentials() only detects access tokens, refresh tokens, or Bearer auth headers. A server using static custom headers can still legitimately return 401/403, but this code will classify that as OAUTH_REQUIRED and push the client into the wrong recovery flow.
Also applies to: 245-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/connect-report.ts` around lines 179 - 194, The code infers
OAUTH_REQUIRED too broadly by using hasConnectionCredentials() (stored in
hasCredentials) which treats any static custom header as credentials; update the
logic to only treat OAuth-style credentials as OAuth-specific (introduce or use
a helper like isOAuthCredentials or refine hasConnectionCredentials to detect
only access_token/refresh_token or Authorization: Bearer) and change the branch
in the auth.isAuth handling (and the similar block around the other auth
handling at the second occurrence) to return "OAUTH_REQUIRED" only when
OAuth-style credentials are present, otherwise classify as "AUTH_ERROR"
(preserving statusCode/retryable behavior). Ensure you update both places (the
block using hasCredentials near auth.isAuth and the repeated block at the later
lines) to reference the new OAuth-specific check (e.g., replace hasCredentials
with isOAuthCredentials(hasConnectionCredentialsDetail) or adjust
hasConnectionCredentials implementation).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcpjam-inspector/client/src/hooks/use-server-state.ts (2)
1917-1988:⚠️ Potential issue | 🟠 MajorPreserve the old OAuth storage before clearing the old name.
In the rename +
skipAutoConnectpath,removeServerFromStateAndCloud(originalServerName)clearsmcp-oauth-config-*andmcp-client-*first. The latersaveOAuthConfigToLocalStorage(... preserveExistingConfigFrom: originalServerName)then copies nothing, so renamed OAuth servers lose stored strategy, proxy, and client metadata.💡 Suggested fix
const updatedServer: ServerWithName = { ...(originalServer ?? {}), name: nextServerName, config: mcpConfig, @@ useOAuth: formData.useOAuth ?? false, } as ServerWithName; + saveOAuthConfigToLocalStorage(formData, { + oauthProfile: updatedServer.oauthFlowProfile, + preserveExistingConfigFrom: isRename + ? originalServerName + : nextServerName, + }); + if (isRename) { await handleDisconnect(originalServerName); await removeServerFromStateAndCloud(originalServerName); } @@ - saveOAuthConfigToLocalStorage(formData, { - oauthProfile: updatedServer.oauthFlowProfile, - preserveExistingConfigFrom: isRename - ? originalServerName - : nextServerName, - }); if (appState.selectedServer === originalServerName && isRename) { setSelectedServer(nextServerName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 1917 - 1988, Renamed servers lose OAuth metadata because removeServerFromStateAndCloud(originalServerName) clears local OAuth storage before saveOAuthConfigToLocalStorage(...) runs; to fix, capture/preserve the old OAuth config before removal and call saveOAuthConfigToLocalStorage (or otherwise stash the original OAuth storage) prior to calling removeServerFromStateAndCloud in handleUpdate so preserveExistingConfigFrom can copy data from originalServerName (refer to handleUpdate, removeServerFromStateAndCloud, saveOAuthConfigToLocalStorage, and clearOAuthData).
1709-1839:⚠️ Potential issue | 🟠 MajorPromote
oauth_requiredinto a forced OAuth reconnect here.Without
forceOAuthFlow, this falls through toensureAuthorizedForReconnect(server), which explicitly skips OAuth whenserver.useOAuthis false and no tokens exist. Any server whose last report isoauth_requiredwill therefore just retry the same unauthenticated connect from the switch or any other plain reconnect caller.💡 Suggested fix
- if (options?.forceOAuthFlow) { + const shouldForceOAuthFlow = + options?.forceOAuthFlow || + server.lastConnectionReport?.status === "oauth_required"; + + if (shouldForceOAuthFlow) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 1709 - 1839, The reconnect handler (handleReconnect) needs to promote servers that reported "oauth_required" into the forced OAuth flow: after computing oauthContext (and before the existing if (options?.forceOAuthFlow) block), detect the server's last report (e.g. server.lastReport === "oauth_required" or the equivalent flag) and set options = { forceOAuthFlow: true } (or otherwise branch into the same forced-OAuth logic) so the code will take the forceOAuthFlow path instead of falling through to ensureAuthorizedForReconnect; update references in this area (handleReconnect, options.forceOAuthFlow, server.lastReport, oauthContext) accordingly.
♻️ Duplicate comments (1)
sdk/src/connect-report.ts (1)
192-195:⚠️ Potential issue | 🟠 Major
OAUTH_REQUIREDvsAUTH_ERRORclassification is currently inverted.When OAuth-style credentials are present and auth fails, this should drive the OAuth recovery path (
"OAUTH_REQUIRED"), not"AUTH_ERROR". The current ternary does the opposite.Suggested fix
- const code: ConnectIssueCode = hasCredentials - ? "AUTH_ERROR" - : "OAUTH_REQUIRED"; + const code: ConnectIssueCode = hasCredentials + ? "OAUTH_REQUIRED" + : "AUTH_ERROR";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/connect-report.ts` around lines 192 - 195, The ternary that sets the ConnectIssueCode is inverted: in the block where auth.isAuth is true you should set code to "OAUTH_REQUIRED" when OAuth-style credentials are present and to "AUTH_ERROR" otherwise. Update the assignment that computes code (the variable named code, type ConnectIssueCode) to use hasCredentials ? "OAUTH_REQUIRED" : "AUTH_ERROR" so the OAuth recovery path is chosen when OAuth credentials exist.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/App.tsx (1)
690-696: Remove unnecessary(server as any)cast—workspaceServersis already properly typed.The code iterates
workspaceServers(which isRecord<string, ServerWithName>from the workspace), andServerWithNamehas aconnectionStatusfield. The cast is redundant and hides type information. Simply accessserver.connectionStatusdirectly without casting.♻️ Suggested patch
- const firstConnected = Object.entries(workspaceServers).find(([, server]) => - isConnectedStatus((server as any).connectionStatus), - ); + const firstConnected = Object.entries(workspaceServers).find(([, server]) => + isConnectedStatus(server.connectionStatus), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/App.tsx` around lines 690 - 696, The code uses an unnecessary cast "(server as any)" when iterating workspaceServers; remove that cast and access server.connectionStatus directly (the workspaceServers variable is typed as Record<string, ServerWithName> where ServerWithName includes connectionStatus). Update the find callback to use isConnectedStatus(server.connectionStatus) and keep the rest of the logic (firstConnected and setSelectedServer) unchanged so TypeScript retains the proper typing and you don't hide type errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts`:
- Around line 388-397: buildOAuthProfile currently serializes raw state and
leaves stale clientId/clientSecret in server.oauthFlowProfile; mirror the same
gating used in buildFormData so credentials are only included when useOAuth &&
(useCustomClientId || oauthRegistrationStrategy === "preregistered"). Update
buildOAuthProfile to set clientId and clientSecret using that condition and
trim() or undefined (e.g., clientId: condition ? clientId.trim() || undefined :
undefined), ensuring it uses the same logic as buildFormData to avoid persisting
stale credentials.
- Line 80: deriveOAuthProfileFromServer(server) currently only reads
server.oauthFlowProfile and returns EMPTY_OAUTH_TEST_PROFILE for servers that
only have persisted mcp-oauth-config-* metadata, causing saved
protocolVersion/registrationStrategy to be overwritten on submit; update the
three call sites that call deriveOAuthProfileFromServer (the one around the
oauthProfile constant and the calls at the other two locations referenced) to
first attempt to load the stored OAuth metadata (the mcp-oauth-config-* entry
for that server) and use that as the edit-form fallback when
server.oauthFlowProfile is absent, e.g., derive the profile by merging
server.oauthFlowProfile || storedOAuthMetadata before falling back to
EMPTY_OAUTH_TEST_PROFILE so protocolVersion and registrationStrategy are
preserved.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 141-156: buildStoredOAuthConfig currently omits persisted
customHeaders so calling code (e.g., initiateOAuth) overwrites and loses them;
change buildStoredOAuthConfig to accept an optional existing StoredOAuthConfig
(or merge parameter) and preserve/merge existing.customHeaders into the returned
StoredOAuthConfig (while keeping registryServerId, useRegistryOAuthProxy,
protocolVersion, registrationStrategy as before), then update invoke sites such
as initiateOAuth to pass readStoredOAuthConfig(...) into buildStoredOAuthConfig
so customHeaders are retained when saving via
saveOAuthConfigToLocalStorage/readStoredOAuthConfig.
---
Outside diff comments:
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 1917-1988: Renamed servers lose OAuth metadata because
removeServerFromStateAndCloud(originalServerName) clears local OAuth storage
before saveOAuthConfigToLocalStorage(...) runs; to fix, capture/preserve the old
OAuth config before removal and call saveOAuthConfigToLocalStorage (or otherwise
stash the original OAuth storage) prior to calling removeServerFromStateAndCloud
in handleUpdate so preserveExistingConfigFrom can copy data from
originalServerName (refer to handleUpdate, removeServerFromStateAndCloud,
saveOAuthConfigToLocalStorage, and clearOAuthData).
- Around line 1709-1839: The reconnect handler (handleReconnect) needs to
promote servers that reported "oauth_required" into the forced OAuth flow: after
computing oauthContext (and before the existing if (options?.forceOAuthFlow)
block), detect the server's last report (e.g. server.lastReport ===
"oauth_required" or the equivalent flag) and set options = { forceOAuthFlow:
true } (or otherwise branch into the same forced-OAuth logic) so the code will
take the forceOAuthFlow path instead of falling through to
ensureAuthorizedForReconnect; update references in this area (handleReconnect,
options.forceOAuthFlow, server.lastReport, oauthContext) accordingly.
---
Duplicate comments:
In `@sdk/src/connect-report.ts`:
- Around line 192-195: The ternary that sets the ConnectIssueCode is inverted:
in the block where auth.isAuth is true you should set code to "OAUTH_REQUIRED"
when OAuth-style credentials are present and to "AUTH_ERROR" otherwise. Update
the assignment that computes code (the variable named code, type
ConnectIssueCode) to use hasCredentials ? "OAUTH_REQUIRED" : "AUTH_ERROR" so the
OAuth recovery path is chosen when OAuth credentials exist.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/App.tsx`:
- Around line 690-696: The code uses an unnecessary cast "(server as any)" when
iterating workspaceServers; remove that cast and access server.connectionStatus
directly (the workspaceServers variable is typed as Record<string,
ServerWithName> where ServerWithName includes connectionStatus). Update the find
callback to use isConnectedStatus(server.connectionStatus) and keep the rest of
the logic (firstConnected and setSelectedServer) unchanged so TypeScript retains
the proper typing and you don't hide type errors.
🪄 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: f71e7c14-7910-4e74-a2ad-e3c386ec6040
📒 Files selected for processing (24)
cli/src/commands/server.tsmcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/ActiveServerSelector.tsxmcpjam-inspector/client/src/components/RegistryTab.tsxmcpjam-inspector/client/src/components/chat-v2/chat-input.tsxmcpjam-inspector/client/src/components/connection/ServerConnectionCard.tsxmcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/components/connection/server-card-utils.tsmcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsxmcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsxmcpjam-inspector/client/src/hooks/use-app-state.tsmcpjam-inspector/client/src/hooks/use-explore-cases-prefetch-on-connect.tsmcpjam-inspector/client/src/hooks/use-onboarding.tsmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/state/app-reducer.tsmcpjam-inspector/client/src/state/mcp-api.tsmcpjam-inspector/client/src/state/oauth-orchestrator.tsmcpjam-inspector/server/routes/mcp/connect.tsmcpjam-inspector/server/routes/web/servers.tssdk/src/connect-report.tssdk/src/index.tssdk/src/server-doctor.ts
✅ Files skipped from review due to trivial changes (2)
- mcpjam-inspector/client/src/state/app-reducer.ts
- mcpjam-inspector/client/src/components/connection/server-card-utils.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- mcpjam-inspector/client/src/hooks/use-explore-cases-prefetch-on-connect.ts
- mcpjam-inspector/client/src/components/chat-v2/chat-input.tsx
- mcpjam-inspector/client/src/components/RegistryTab.tsx
- mcpjam-inspector/client/src/components/ActiveServerSelector.tsx
- sdk/src/server-doctor.ts
- mcpjam-inspector/client/src/hooks/use-onboarding.ts
- mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx
- mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsx
- mcpjam-inspector/client/src/hooks/use-app-state.ts
- cli/src/commands/server.ts
- mcpjam-inspector/server/routes/web/servers.ts
| // Initialize form with server data (for edit mode) | ||
| useEffect(() => { | ||
| if (server) { | ||
| const oauthProfile = deriveOAuthProfileFromServer(server); |
Contributor
There was a problem hiding this comment.
Use stored OAuth metadata as the edit-form fallback.
deriveOAuthProfileFromServer(server) only sees server.oauthFlowProfile. Fresh connect flows still persist protocolVersion and registrationStrategy in mcp-oauth-config-*, so opening this form for those servers falls back to EMPTY_OAUTH_TEST_PROFILE and silently overwrites the saved strategy on the next submit.
💡 Suggested direction
-import { hasOAuthConfig, getStoredTokens } from "@/lib/oauth/mcp-oauth";
+import {
+ hasOAuthConfig,
+ getStoredTokens,
+ readStoredOAuthConfig,
+} from "@/lib/oauth/mcp-oauth";
@@
- const oauthProfile = deriveOAuthProfileFromServer(server);
+ const storedOAuthConfig = readStoredOAuthConfig(server.name);
+ const derivedOAuthProfile = deriveOAuthProfileFromServer(server);
+ const oauthProfile: OAuthTestProfile = {
+ ...derivedOAuthProfile,
+ protocolVersion:
+ server.oauthFlowProfile?.protocolVersion ??
+ storedOAuthConfig.protocolVersion ??
+ derivedOAuthProfile.protocolVersion,
+ registrationStrategy:
+ server.oauthFlowProfile?.registrationStrategy ??
+ storedOAuthConfig.registrationStrategy ??
+ derivedOAuthProfile.registrationStrategy,
+ };Also applies to: 161-162, 220-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts`
at line 80, deriveOAuthProfileFromServer(server) currently only reads
server.oauthFlowProfile and returns EMPTY_OAUTH_TEST_PROFILE for servers that
only have persisted mcp-oauth-config-* metadata, causing saved
protocolVersion/registrationStrategy to be overwritten on submit; update the
three call sites that call deriveOAuthProfileFromServer (the one around the
oauthProfile constant and the calls at the other two locations referenced) to
first attempt to load the stored OAuth metadata (the mcp-oauth-config-* entry
for that server) and use that as the edit-form fallback when
server.oauthFlowProfile is absent, e.g., derive the profile by merging
server.oauthFlowProfile || storedOAuthMetadata before falling back to
EMPTY_OAUTH_TEST_PROFILE so protocolVersion and registrationStrategy are
preserved.
Comment on lines
+388
to
+397
| clientId: | ||
| useOAuth && | ||
| (useCustomClientId || oauthRegistrationStrategy === "preregistered") | ||
| ? clientId.trim() || undefined | ||
| : undefined, | ||
| clientSecret: | ||
| useOAuth && | ||
| (useCustomClientId || oauthRegistrationStrategy === "preregistered") | ||
| ? clientSecret.trim() || undefined | ||
| : undefined, |
Contributor
There was a problem hiding this comment.
Mirror the credential-gating logic in buildOAuthProfile().
buildFormData() intentionally drops clientId and clientSecret when custom credentials are off, but buildOAuthProfile() still serializes the raw state. That leaves stale credentials in server.oauthFlowProfile, and the reconnect path will prefer them on the next OAuth attempt.
💡 Suggested fix
- const buildOAuthProfile = (): OAuthTestProfile => ({
- serverUrl: url.trim(),
- clientId: clientId.trim(),
- clientSecret: clientSecret.trim(),
- scopes: oauthScopesInput.trim(),
- customHeaders: customHeaders
- .filter(({ key }) => key.trim().length > 0)
- .map(({ key, value }) => ({ key: key.trim(), value })),
- protocolVersion: oauthProtocolVersion,
- registrationStrategy: oauthRegistrationStrategy,
- });
+ const buildOAuthProfile = (): OAuthTestProfile => {
+ const persistCustomCredentials =
+ authType === "oauth" &&
+ (useCustomClientId || oauthRegistrationStrategy === "preregistered");
+
+ return {
+ serverUrl: url.trim(),
+ clientId: persistCustomCredentials ? clientId.trim() : "",
+ clientSecret: persistCustomCredentials ? clientSecret.trim() : "",
+ scopes: oauthScopesInput.trim(),
+ customHeaders: customHeaders
+ .filter(({ key }) => key.trim().length > 0)
+ .map(({ key, value }) => ({ key: key.trim(), value })),
+ protocolVersion: oauthProtocolVersion,
+ registrationStrategy: oauthRegistrationStrategy,
+ };
+ };Also applies to: 402-412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts`
around lines 388 - 397, buildOAuthProfile currently serializes raw state and
leaves stale clientId/clientSecret in server.oauthFlowProfile; mirror the same
gating used in buildFormData so credentials are only included when useOAuth &&
(useCustomClientId || oauthRegistrationStrategy === "preregistered"). Update
buildOAuthProfile to set clientId and clientSecret using that condition and
trim() or undefined (e.g., clientId: condition ? clientId.trim() || undefined :
undefined), ensuring it uses the same logic as buildFormData to avoid persisting
stale credentials.
Comment on lines
141
to
156
| export function buildStoredOAuthConfig( | ||
| options: Pick< | ||
| MCPOAuthOptions, | ||
| "scopes" | "registryServerId" | "useRegistryOAuthProxy" | ||
| | "scopes" | ||
| | "registryServerId" | ||
| | "useRegistryOAuthProxy" | ||
| | "protocolVersion" | ||
| | "registrationStrategy" | ||
| >, | ||
| ): StoredOAuthConfig { | ||
| const config: StoredOAuthConfig = { | ||
| registryServerId: options.registryServerId, | ||
| useRegistryOAuthProxy: options.useRegistryOAuthProxy === true, | ||
| protocolVersion: options.protocolVersion, | ||
| registrationStrategy: options.registrationStrategy, | ||
| }; |
Contributor
There was a problem hiding this comment.
Keep customHeaders when rebuilding stored OAuth config.
saveOAuthConfigToLocalStorage() and readStoredOAuthConfig() both treat customHeaders as part of the persisted OAuth state, but buildStoredOAuthConfig() rewrites the same record without them. Starting an OAuth flow will erase any extra headers before the callback or refresh path can reuse them.
💡 Suggested direction
export function buildStoredOAuthConfig(
options: Pick<
MCPOAuthOptions,
| "scopes"
| "registryServerId"
| "useRegistryOAuthProxy"
| "protocolVersion"
| "registrationStrategy"
- >,
+ >,
+ existing?: Pick<StoredOAuthConfig, "customHeaders">,
): StoredOAuthConfig {
const config: StoredOAuthConfig = {
registryServerId: options.registryServerId,
useRegistryOAuthProxy: options.useRegistryOAuthProxy === true,
protocolVersion: options.protocolVersion,
registrationStrategy: options.registrationStrategy,
};
if (options.scopes && options.scopes.length > 0) {
config.scopes = options.scopes;
}
+
+ if (existing?.customHeaders && Object.keys(existing.customHeaders).length > 0) {
+ config.customHeaders = existing.customHeaders;
+ }
return config;
}Then merge the existing stored value at the call site in initiateOAuth(...), e.g.:
const existingConfig = readStoredOAuthConfig(options.serverName);
const oauthConfig = buildStoredOAuthConfig(options, existingConfig);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function buildStoredOAuthConfig( | |
| options: Pick< | |
| MCPOAuthOptions, | |
| "scopes" | "registryServerId" | "useRegistryOAuthProxy" | |
| | "scopes" | |
| | "registryServerId" | |
| | "useRegistryOAuthProxy" | |
| | "protocolVersion" | |
| | "registrationStrategy" | |
| >, | |
| ): StoredOAuthConfig { | |
| const config: StoredOAuthConfig = { | |
| registryServerId: options.registryServerId, | |
| useRegistryOAuthProxy: options.useRegistryOAuthProxy === true, | |
| protocolVersion: options.protocolVersion, | |
| registrationStrategy: options.registrationStrategy, | |
| }; | |
| export function buildStoredOAuthConfig( | |
| options: Pick< | |
| MCPOAuthOptions, | |
| | "scopes" | |
| | "registryServerId" | |
| | "useRegistryOAuthProxy" | |
| | "protocolVersion" | |
| | "registrationStrategy" | |
| >, | |
| existing?: Pick<StoredOAuthConfig, "customHeaders">, | |
| ): StoredOAuthConfig { | |
| const config: StoredOAuthConfig = { | |
| registryServerId: options.registryServerId, | |
| useRegistryOAuthProxy: options.useRegistryOAuthProxy === true, | |
| protocolVersion: options.protocolVersion, | |
| registrationStrategy: options.registrationStrategy, | |
| }; | |
| if (options.scopes && options.scopes.length > 0) { | |
| config.scopes = options.scopes; | |
| } | |
| if (existing?.customHeaders && Object.keys(existing.customHeaders).length > 0) { | |
| config.customHeaders = existing.customHeaders; | |
| } | |
| return config; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts` around lines 141 - 156,
buildStoredOAuthConfig currently omits persisted customHeaders so calling code
(e.g., initiateOAuth) overwrites and loses them; change buildStoredOAuthConfig
to accept an optional existing StoredOAuthConfig (or merge parameter) and
preserve/merge existing.customHeaders into the returned StoredOAuthConfig (while
keeping registryServerId, useRegistryOAuthProxy, protocolVersion,
registrationStrategy as before), then update invoke sites such as initiateOAuth
to pass readStoredOAuthConfig(...) into buildStoredOAuthConfig so customHeaders
are retained when saving via
saveOAuthConfigToLocalStorage/readStoredOAuthConfig.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 15754f2. Configure here.
|
|
||
| if (result.status !== "connected") { | ||
| setProcessExitCode(1); | ||
| } |
There was a problem hiding this comment.
CLI exit code 1 for partial (successful) connections
Medium Severity
The server validate command sets exit code 1 for any status other than "connected", including "partial". However, a "partial" report has success: true — the server connected and initialized, but post-connect tool listing had a warning. Treating this the same as a failure ("failed" or "oauth_required") may break CI pipelines or scripts that rely on exit code 0 to mean "server is reachable and usable". Elsewhere in the inspector UI, isConnectedStatus treats "partial" as connected.
Reviewed by Cursor Bugbot for commit 15754f2. Configure here.
| report, | ||
| initInfo: report.initInfo, | ||
| ...(report.issue ? { error: report.issue.message } : {}), | ||
| }); |
There was a problem hiding this comment.
Connect route returns HTTP 200 for failed connections
Low Severity
The connect endpoint now always returns HTTP 200 regardless of report.success. Previously, connection failures returned HTTP 500. While the client checks result.success rather than the HTTP status, this semantic change means any infrastructure-level monitoring, reverse proxy error logging, or middleware that keys on 5xx status codes will no longer detect connection failures.
Reviewed by Cursor Bugbot for commit 15754f2. Configure here.
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
1 participant
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.
Note
High Risk
Refactors core server connection and OAuth initiation/reconnect flows across SDK, web API, and UI; mistakes could break connectivity or incorrectly handle OAuth requirements/credentials in production.
Overview
Introduces a unified SDK-level
connectServerWithReportthat returns a structuredConnectReport(includingpartialandoauth_requiredoutcomes, issue codes/phases, and optional diagnostics), and wires this into CLIserver validateplus server/api/mcp/connect,/api/mcp/servers/reconnect, and hosted/api/web/servers/validateresponses.Updates the inspector client to treat
partialas connected viaisConnectedStatus, surfacelastConnectionReport/diagnostics in server cards/modals, and propagate OAuth context (protocolVersion,registrationStrategy, registry proxy usage, custom-credential usage) through connect/validate/reconnect requests.Extends OAuth configuration UX and persistence: adds protocol version + registration strategy controls, enforces preregistered client ID requirements, preserves saved OAuth strategy details on forced reconnect, and seeds/loads OAuth discovery state with protocol hints (including CIMD client metadata URL support).
Reviewed by Cursor Bugbot for commit 15754f2. Bugbot is set up for automated code reviews on this repo. Configure here.