Unify MCP connect reporting and add OAuth strategy support#1793
Open
chelojimenez wants to merge 9 commits intomainfrom
Open
Unify MCP connect reporting and add OAuth strategy support#1793chelojimenez wants to merge 9 commits intomainfrom
chelojimenez wants to merge 9 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}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConnection flows were centralized around a new connectServerWithReport(...) API that returns structured ConnectReport objects. CLI, server routes, web validate/reconnect handlers, and client reconnection paths now use it. The SDK exposes connect/report types and helpers (connectServerWithReport, buildConnectedServerDoctorResult). A new "partial" status and isConnectedStatus(...) predicate were added and applied across the client. OAuth test-profile fields and an optional oauthContext were threaded through forms, persistence, orchestration, and HTTP APIs. 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
+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
|
|
||
| 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.
chelojimenez
temporarily deployed
to
preview-pr-1793
GitHub Actions
Inactive
| } | ||
|
|
||
| return key.toLowerCase() !== "authorization"; | ||
| }); |
There was a problem hiding this comment.
Custom headers falsely detected as auth credentials
Medium Severity
hasConnectionCredentials returns true if any non-Authorization header has a non-empty value, treating generic custom headers (e.g. Content-Type, routing headers) as authentication credentials. Since toMCPConfig always sets requestInit: { headers: formData.headers || {} }, any user-configured custom header triggers this. This causes classifyConnectIssue to produce AUTH_ERROR instead of OAUTH_REQUIRED for servers needing OAuth but having custom headers, so the connect report status becomes "failed" rather than "oauth_required", and the UI shows a generic failure instead of "Authorization required."
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3e5c82e. Configure here.
| registrationStrategy !== "cimd" | ||
| ) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Client handles protocol version server rejects
Medium Severity
buildOAuthContext explicitly accepts "2025-03-26" as a valid protocolVersion (with distinct registration strategy validation), and the UI in AuthenticationSection offers it as a selectable option. However, ConnectContext.oauth.protocolVersion only allows "2025-06-18" | "2025-11-25", and the server-side parseOAuthContext throws a 400 error for "2025-03-26". A user selecting this version would get a confusing validation error on connect.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 3e5c82e. Configure here.
Internal previewPreview URL: https://mcp-inspector-pr-1793.up.railway.app |
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 (4)
mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts (1)
96-131:⚠️ Potential issue | 🟠 MajorDo not derive edit-mode OAuth solely from browser storage.
This path still treats OAuth as present only when tokens or
mcp-oauth-config-*exist locally, and it only hydrates scopes/credentials from storage. For a shared or freshly loaded server that only carriesserver.useOAuth/server.oauthFlowProfile, the form opens asauthType === "none"with empty preregistered credentials, and the next save will quietly strip the OAuth setup.Possible fix
if (isHttpServer) { // Check if OAuth is configured by looking at multiple sources: // 1. Check if server has oauth tokens // 2. Check if there's stored OAuth data const hasOAuthTokens = server.oauthTokens != null; const hasStoredOAuthConfig = hasOAuthConfig(server.name); - hasOAuth = hasOAuthTokens || hasStoredOAuthConfig; + hasOAuth = + hasOAuthTokens || + hasStoredOAuthConfig || + server.useOAuth === true || + Boolean(server.oauthFlowProfile); @@ scopes = server.oauthTokens?.scope?.split(" ") || storedTokens?.scope?.split(" ") || oauthConfig.scopes || - []; + oauthProfile.scopes.trim().split(/\s+/).filter(Boolean); @@ - clientIdValue = storedTokens?.client_id || clientInfo?.client_id || ""; - - clientSecretValue = clientInfo?.client_secret || ""; + clientIdValue = + storedTokens?.client_id || + clientInfo?.client_id || + oauthProfile.clientId || + ""; + + clientSecretValue = + clientInfo?.client_secret || oauthProfile.clientSecret || "";🤖 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 96 - 131, The form currently sets hasOAuth and hydrates scopes/credentials only from local storage (hasOAuthTokens, hasStoredOAuthConfig, storedTokens), which causes servers that declare OAuth via server.useOAuth or server.oauthFlowProfile to be treated as authType "none"; update the logic in use-server-form (inside the isHttpServer branch) to also treat server.useOAuth === true and the presence of server.oauthFlowProfile as indicating OAuth is enabled (set hasOAuth accordingly) and to prefer hydrating scopes, clientIdValue, clientSecretValue, protocolVersionValue and registrationStrategyValue from server.oauthFlowProfile when stored data is absent; keep existing fallbacks to readStoredOAuthConfig, getStoredTokens, and clientInfo but ensure server.oauthFlowProfile and server.useOAuth are first-class sources so saving won’t drop a server-declared OAuth setup.mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts (1)
915-942:⚠️ Potential issue | 🟠 MajorHosted callbacks still reject CIMD flows without a stored
client_id.
initiateOAuth()now supports CIMD viaclientMetadataUrl, but it only persistsmcp-client-*when the user supplied explicit credentials. In the default CIMD path that meansclientInformation.client_idis empty here, so hosted OAuth dies with “OAuth client ID not found” before the completion request is even sent.Possible fix
const clientInformation = readStoredClientInformation(serverName); - if (!clientInformation.client_id) { + const oauthConfig = readStoredOAuthConfig(serverName); + const clientId = + clientInformation.client_id ?? + (oauthConfig.registrationStrategy === "cimd" + ? DEFAULT_MCPJAM_CLIENT_ID_METADATA_URL + : undefined); + + if (!clientId) { throw new Error("OAuth client ID not found"); } @@ redirectUri: getRedirectUri(), clientInformation: { - clientId: clientInformation.client_id, + clientId, ...(clientInformation.client_secret ? { clientSecret: clientInformation.client_secret } : {}), },🤖 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 915 - 942, The code currently throws if readStoredClientInformation() yields no client_id, which breaks CIMD flows; change the logic in the block before calling authFetch(`${convexSiteUrl}/web/oauth/complete`, ...) so that when clientInformation.client_id is missing you either (a) fetch the client metadata from the CIMD URL supplied during initiateOAuth (context.clientMetadataUrl) to populate clientId/clientSecret before sending the completion request, or (b) include the clientMetadataUrl in the POST body so the hosted callback can resolve the client dynamically; update the payload construction to conditionally send clientInformation.clientId/clientSecret OR clientMetadataUrl instead of throwing in readStoredClientInformation/clientInformation.client_id.mcpjam-inspector/client/src/hooks/use-server-state.ts (2)
1966-2005:⚠️ Potential issue | 🟠 Major
requiresFreshOAuthAuthorizationno longer sees the new report-shaped errors.Under the unified
ConnectReportmodel, a failed response can carryreport.issue.messagewitherrorunset (seeConnectionApiResponseinmcpjam-inspector/client/src/state/mcp-api.ts). This branch still probes onlyresult.error, so when the backend reports an OAuth-reauthorization need throughreport.issue, the hosted path will mark it a generic failure instead of escalating to a fresh OAuth flow — regressing the very UX this PR aims to unify. Route the detection throughgetConnectionErrorMessage(result)so both shapes are honored.🛠️ Proposed fix
- if (!requiresFreshOAuthAuthorization(result.error)) { + const failureMessage = getConnectionErrorMessage(result); + if (!requiresFreshOAuthAuthorization(failureMessage)) { dispatch({ type: "CONNECT_FAILURE", name: serverName, - error: result.error || "Reconnection failed", + error: failureMessage || "Reconnection failed", }); logger.error("Hosted reconnect failed", { serverName, result }); - toast.error(result.error || `Failed to reconnect: ${serverName}`); + toast.error(failureMessage || `Failed to reconnect: ${serverName}`); 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-server-state.ts` around lines 1966 - 2005, The hosted reconnect branch currently checks result.error directly, which misses report-shaped errors; update both places that call requiresFreshOAuthAuthorization(result.error) to call requiresFreshOAuthAuthorization(getConnectionErrorMessage(result)) and use getConnectionErrorMessage(result) when computing the logged/toasted error string (instead of result.error) so the unified ConnectReport shape (report.issue.message) is honored; keep the existing dispatch actions (type "CONNECT_FAILURE") but pass the message from getConnectionErrorMessage(result) and use that same message in logger.error and toast.error for consistency.
1938-1964:⚠️ Potential issue | 🟠 MajorHosted reconnect bypasses both
oauthContextandcompleteConnection.
oauthContextis computed at line 1859 but never threaded into thisguardedReconnectServer(serverName, hostedReconnectConfig)call, so the hosted path alone loses the protocol/strategy/custom-credential signals this PR just plumbed everywhere else. The branch also still hand-rollsCONNECT_SUCCESS+storeInitInfoinstead of going throughcompleteConnection, which means anyreporton the response is dropped andlastConnectionReportnever gets populated for hosted OAuth reconnects.♻️ Suggested alignment
- const result = await guardedReconnectServer( - serverName, - hostedReconnectConfig, - ); + const result = await guardedReconnectServer( + serverName, + hostedReconnectConfig, + oauthContext, + ); if (isStaleOp(serverName, token)) return; if (result.success) { - dispatch({ - type: "CONNECT_SUCCESS", - name: serverName, - config: server.config, - tokens: undefined, - useOAuth: true, - }); - logger.info("Hosted reconnect successful using stored OAuth", { - serverName, - result, - }); - storeInitInfo(serverName, result.initInfo).catch((err) => - logger.warn("Failed to fetch init info", { serverName, err }), - ); + await completeConnection(serverName, server.config, result, { + tokens: undefined, + useOAuth: true, + }); + logger.info("Hosted reconnect successful using stored OAuth", { + serverName, + }); 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-server-state.ts` around lines 1938 - 1964, The hosted reconnect branch currently calls guardedReconnectServer(serverName, hostedReconnectConfig) without passing the previously computed oauthContext and then manually dispatches CONNECT_SUCCESS and calls storeInitInfo, which bypasses completeConnection and drops the response.report (so lastConnectionReport is never set); fix by passing oauthContext into the guardedReconnectServer call (e.g., guardedReconnectServer(serverName, hostedReconnectConfig, oauthContext)) and, on success, invoke completeConnection with the serverName, token, server.config, result (or result.initInfo) so completeConnection handles the CONNECT_SUCCESS dispatch, storeInitInfo behavior, and populates lastConnectionReport/report instead of doing the manual dispatch/storeInitInfo steps.
♻️ Duplicate comments (1)
sdk/src/connect-report.ts (1)
252-281:⚠️ Potential issue | 🟠 Major
hasConnectionCredentialstreats any non-empty header as a credential.Lines 274–280 classify a config as "has credentials" whenever any non-authorization header carries a non-empty string value. In practice that includes wholly benign entries like
User-Agent,Accept,Content-Type, or amcp-session-id— none of which are auth material. The knock-on effect inclassifyConnectIssueis that a 401/403 from a server that legitimately needs OAuth gets bucketed asAUTH_ERRORinstead ofOAUTH_REQUIRED, steering the UI away from the OAuth recovery flow it just added support for.Consider restricting credential detection to OAuth/auth-bearing signals only (access/refresh tokens,
Authorization, and perhaps a known allow‑list such asX-Api-Key/Api-Key).🔧 Proposed narrowing
- return Object.entries(headers).some(([key, value]) => { - if (typeof value !== "string" || !value.trim()) { - return false; - } - - return key.toLowerCase() !== "authorization"; - }); + const CREDENTIAL_HEADER_NAMES = new Set([ + "x-api-key", + "api-key", + "x-auth-token", + ]); + return Object.entries(headers).some(([key, value]) => { + if (typeof value !== "string" || !value.trim()) { + return false; + } + return CREDENTIAL_HEADER_NAMES.has(key.toLowerCase()); + });🤖 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 252 - 281, hasConnectionCredentials currently treats any non-empty header as credentials which misclassifies errors in classifyConnectIssue; change hasConnectionCredentials (and its use of extractHeaders/headers) to only consider explicit auth signals: existing accessToken/refreshToken checks, Authorization bearer token, and a small allow-list such as X-Api-Key and Api-Key (case-insensitive). Remove the final Object.entries(headers).some(...) catch-all and replace it with checks for those specific header names (normalize key to lower case), returning true only when one of those auth headers has a non-empty string value.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/hooks/use-server-state.ts (1)
480-542: Silentundefinedreturn drops legitimate OAuth signals.When
protocolVersionorregistrationStrategyis missing/invalid,buildOAuthContextreturnsundefinedwholesale — which also discardsuseRegistryOAuthProxyandusedCustomClientCredentials, signals the backend may want even for servers whose protocol hints haven't been persisted yet (e.g., freshly-added servers whose stored config predates this PR). Consider either (a) logging alogger.warnon the invalid-combo path so silent fallbacks are diagnosable, or (b) still returning the proxy/custom-cred flags with the protocol fields omitted, so partial context is propagated instead of erased.🤖 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 480 - 542, The current buildOAuthContext function returns undefined when protocolVersion or registrationStrategy are invalid, which discards useRegistryOAuthProxy and usedCustomClientCredentials; change buildOAuthContext so that instead of returning undefined in those validation branches it still returns a ConnectContext["oauth"] object that includes useRegistryOAuthProxy and usedCustomClientCredentials (with protocolVersion and/or registrationStrategy set to undefined or omitted as appropriate), and also add a logger.warn (or similar) call when the protocol/registration combo is invalid to aid diagnostics; locate buildOAuthContext and the reads from readStoredOAuthConfig/readStoredClientInfo to compute useRegistryOAuthProxy and usedCustomClientCredentials, then adjust the conditional blocks that currently return undefined to return the partial context plus a warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/src/connect-report.ts`:
- Around line 45-59: The code currently defaults serverId to DEFAULT_SERVER_ID
even when an external manager is supplied, which risks id collisions; update
connect-report.ts so that when input.manager is provided and input.serverId is
undefined you either (a) throw an error asking the caller to provide an explicit
serverId, or (b) derive a unique id per call (e.g., append a UUID or
incrementing counter) and use that instead of DEFAULT_SERVER_ID; adjust the
variables serverId, ownsManager and the finally/cleanup logic that references
ownsManager/manager so shared MCPClientManager instances (MCPClientManager,
manager, ownsManager, DEFAULT_SERVER_ID) are not accidentally clobbered or left
connected.
---
Outside diff comments:
In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts`:
- Around line 96-131: The form currently sets hasOAuth and hydrates
scopes/credentials only from local storage (hasOAuthTokens,
hasStoredOAuthConfig, storedTokens), which causes servers that declare OAuth via
server.useOAuth or server.oauthFlowProfile to be treated as authType "none";
update the logic in use-server-form (inside the isHttpServer branch) to also
treat server.useOAuth === true and the presence of server.oauthFlowProfile as
indicating OAuth is enabled (set hasOAuth accordingly) and to prefer hydrating
scopes, clientIdValue, clientSecretValue, protocolVersionValue and
registrationStrategyValue from server.oauthFlowProfile when stored data is
absent; keep existing fallbacks to readStoredOAuthConfig, getStoredTokens, and
clientInfo but ensure server.oauthFlowProfile and server.useOAuth are
first-class sources so saving won’t drop a server-declared OAuth setup.
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 1966-2005: The hosted reconnect branch currently checks
result.error directly, which misses report-shaped errors; update both places
that call requiresFreshOAuthAuthorization(result.error) to call
requiresFreshOAuthAuthorization(getConnectionErrorMessage(result)) and use
getConnectionErrorMessage(result) when computing the logged/toasted error string
(instead of result.error) so the unified ConnectReport shape
(report.issue.message) is honored; keep the existing dispatch actions (type
"CONNECT_FAILURE") but pass the message from getConnectionErrorMessage(result)
and use that same message in logger.error and toast.error for consistency.
- Around line 1938-1964: The hosted reconnect branch currently calls
guardedReconnectServer(serverName, hostedReconnectConfig) without passing the
previously computed oauthContext and then manually dispatches CONNECT_SUCCESS
and calls storeInitInfo, which bypasses completeConnection and drops the
response.report (so lastConnectionReport is never set); fix by passing
oauthContext into the guardedReconnectServer call (e.g.,
guardedReconnectServer(serverName, hostedReconnectConfig, oauthContext)) and, on
success, invoke completeConnection with the serverName, token, server.config,
result (or result.initInfo) so completeConnection handles the CONNECT_SUCCESS
dispatch, storeInitInfo behavior, and populates lastConnectionReport/report
instead of doing the manual dispatch/storeInitInfo steps.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 915-942: The code currently throws if
readStoredClientInformation() yields no client_id, which breaks CIMD flows;
change the logic in the block before calling
authFetch(`${convexSiteUrl}/web/oauth/complete`, ...) so that when
clientInformation.client_id is missing you either (a) fetch the client metadata
from the CIMD URL supplied during initiateOAuth (context.clientMetadataUrl) to
populate clientId/clientSecret before sending the completion request, or (b)
include the clientMetadataUrl in the POST body so the hosted callback can
resolve the client dynamically; update the payload construction to conditionally
send clientInformation.clientId/clientSecret OR clientMetadataUrl instead of
throwing in readStoredClientInformation/clientInformation.client_id.
---
Duplicate comments:
In `@sdk/src/connect-report.ts`:
- Around line 252-281: hasConnectionCredentials currently treats any non-empty
header as credentials which misclassifies errors in classifyConnectIssue; change
hasConnectionCredentials (and its use of extractHeaders/headers) to only
consider explicit auth signals: existing accessToken/refreshToken checks,
Authorization bearer token, and a small allow-list such as X-Api-Key and Api-Key
(case-insensitive). Remove the final Object.entries(headers).some(...) catch-all
and replace it with checks for those specific header names (normalize key to
lower case), returning true only when one of those auth headers has a non-empty
string value.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 480-542: The current buildOAuthContext function returns undefined
when protocolVersion or registrationStrategy are invalid, which discards
useRegistryOAuthProxy and usedCustomClientCredentials; change buildOAuthContext
so that instead of returning undefined in those validation branches it still
returns a ConnectContext["oauth"] object that includes useRegistryOAuthProxy and
usedCustomClientCredentials (with protocolVersion and/or registrationStrategy
set to undefined or omitted as appropriate), and also add a logger.warn (or
similar) call when the protocol/registration combo is invalid to aid
diagnostics; locate buildOAuthContext and the reads from
readStoredOAuthConfig/readStoredClientInfo to compute useRegistryOAuthProxy and
usedCustomClientCredentials, then adjust the conditional blocks that currently
return undefined to return the partial context plus a warning.
🪄 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: a778352d-7118-4cad-b162-038fd9cf98ad
📒 Files selected for processing (18)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/EvalsTab.tsxmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/chat-v2/chat-input.tsxmcpjam-inspector/client/src/components/connection/ServerConnectionCard.tsxmcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.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-server-state.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/oauth-orchestrator.tsmcpjam-inspector/server/routes/web/servers.tssdk/src/connect-report.tssdk/src/server-doctor.ts
✅ Files skipped from review due to trivial changes (1)
- mcpjam-inspector/client/src/hooks/tests/use-server-state.test.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- mcpjam-inspector/client/src/components/mcp-sidebar.tsx
- mcpjam-inspector/client/src/components/EvalsTab.tsx
- mcpjam-inspector/client/src/App.tsx
- mcpjam-inspector/client/src/state/oauth-orchestrator.ts
- sdk/src/server-doctor.ts
- mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx
- mcpjam-inspector/server/routes/web/servers.ts
Comment on lines
+45
to
+59
| const serverId = input.serverId ?? DEFAULT_SERVER_ID; | ||
| const timeout = input.timeout ?? DEFAULT_TIMEOUT_MS; | ||
| const ownsManager = !input.manager; | ||
| const manager = | ||
| input.manager ?? | ||
| new MCPClientManager( | ||
| {}, | ||
| { | ||
| defaultTimeout: timeout, | ||
| defaultClientName: "mcpjam-sdk", | ||
| lazyConnect: true, | ||
| ...(input.retryPolicy ? { retryPolicy: input.retryPolicy } : {}), | ||
| ...(input.rpcLogger ? { rpcLogger: input.rpcLogger } : {}), | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
DEFAULT_SERVER_ID can collide with an externally supplied manager.
When a caller passes their own manager, defaulting serverId to the constant "__connect_report__" will clobber (or be clobbered by) any pre-existing connection registered under that id, and the finally branch leaves it connected since ownsManager is false. Requiring an explicit serverId whenever input.manager is provided — or deriving a unique id per call — would keep shared managers safe.
🤖 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 45 - 59, The code currently defaults
serverId to DEFAULT_SERVER_ID even when an external manager is supplied, which
risks id collisions; update connect-report.ts so that when input.manager is
provided and input.serverId is undefined you either (a) throw an error asking
the caller to provide an explicit serverId, or (b) derive a unique id per call
(e.g., append a UUID or incrementing counter) and use that instead of
DEFAULT_SERVER_ID; adjust the variables serverId, ownsManager and the
finally/cleanup logic that references ownsManager/manager so shared
MCPClientManager instances (MCPClientManager, manager, ownsManager,
DEFAULT_SERVER_ID) are not accidentally clobbered or left connected.
# Conflicts: # mcpjam-inspector/client/src/components/logger-view.tsx # mcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsx # mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts # mcpjam-inspector/client/src/state/mcp-api.ts
chelojimenez
temporarily deployed
to
preview-pr-1793
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts (2)
517-531: Asymmetric constructor:_protocolVersionaccepted, then dropped on the floor.
registrationStrategyis faithfully stored and later consulted byclientMetadataUrl, yet_protocolVersionis merely threaded through the signature and discarded. If protocol version is genuinely a per-discovery concern (as theloadCallbackDiscoveryState/seedDiscoveryStateplumbing suggests), consider removing the parameter to keep the provider's contract honest; if there's foreseeable future use, store it on the instance for symmetry.♻️ Option A — drop the unused parameter
constructor( serverName: string, serverUrl: string, customClientId?: string, customClientSecret?: string, - _protocolVersion?: OAuthProtocolVersion, registrationStrategy?: OAuthRegistrationStrategy, ) {Callers in
initiateOAuth,handleOAuthCallback, andrefreshOAuthTokenswould then stop passingprotocolVersioninto the constructor (it remains passed into discovery helpers, which is where it actually matters).🤖 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 517 - 531, The constructor accepts _protocolVersion but never stores or uses it; either remove the unused _protocolVersion parameter from the constructor signature and stop passing protocolVersion where this class is instantiated (e.g., from initiateOAuth, handleOAuthCallback, refreshOAuthTokens), or persist it on the instance (this.protocolVersion = _protocolVersion) for symmetry and future use; update any references or tests accordingly and keep registrationStrategy handling and clientMetadataUrl logic unchanged (see constructor, _protocolVersion, registrationStrategy, clientMetadataUrl, loadCallbackDiscoveryState, seedDiscoveryState, initiateOAuth, handleOAuthCallback, refreshOAuthTokens).
65-69: Hardcoded protocol version list will require manual updates if SDK adds new versions.The
isOAuthProtocolVersionguard hardcodes three date strings. If the SDK adds a newOAuthProtocolVersionin a future release, any previously-persisted config with that version will round-trip toundefinedinreadStoredOAuthConfig(line ~98)—gracefully degrading but silently dropping the stored version preference. Since the SDK exportsOAuthProtocolVersionas a type-only and does not provide a runtime constant or array, consider documenting this list as a sync point or adding a comment linking to the SDK's enum/union definition for maintainability.🤖 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 65 - 69, The type guard isOAuthProtocolVersion currently hardcodes three version strings which will silently drop unknown persisted versions; update the function by either adding a clear comment linking to the SDK's OAuthProtocolVersion union/enum as the authoritative sync point or refactor to a single-source-of-truth approach (e.g., derive allowed versions from a maintained exported array or documentation constant) and ensure readStoredOAuthConfig uses this updated guard; reference the isOAuthProtocolVersion function and readStoredOAuthConfig so reviewers can find and update the version list or add the comment/constant to document when to sync with the SDK.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 517-531: The constructor accepts _protocolVersion but never stores
or uses it; either remove the unused _protocolVersion parameter from the
constructor signature and stop passing protocolVersion where this class is
instantiated (e.g., from initiateOAuth, handleOAuthCallback,
refreshOAuthTokens), or persist it on the instance (this.protocolVersion =
_protocolVersion) for symmetry and future use; update any references or tests
accordingly and keep registrationStrategy handling and clientMetadataUrl logic
unchanged (see constructor, _protocolVersion, registrationStrategy,
clientMetadataUrl, loadCallbackDiscoveryState, seedDiscoveryState,
initiateOAuth, handleOAuthCallback, refreshOAuthTokens).
- Around line 65-69: The type guard isOAuthProtocolVersion currently hardcodes
three version strings which will silently drop unknown persisted versions;
update the function by either adding a clear comment linking to the SDK's
OAuthProtocolVersion union/enum as the authoritative sync point or refactor to a
single-source-of-truth approach (e.g., derive allowed versions from a maintained
exported array or documentation constant) and ensure readStoredOAuthConfig uses
this updated guard; reference the isOAuthProtocolVersion function and
readStoredOAuthConfig so reviewers can find and update the version list or add
the comment/constant to document when to sync with the SDK.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77316682-9f6e-4513-91de-c3a76910ab2b
📒 Files selected for processing (6)
cli/src/commands/server.tsmcpjam-inspector/client/src/components/logger-view.tsxmcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsxmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/state/mcp-api.tssdk/src/browser.ts
✅ Files skipped from review due to trivial changes (1)
- mcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- mcpjam-inspector/client/src/components/logger-view.tsx
- mcpjam-inspector/client/src/state/mcp-api.ts
- sdk/src/browser.ts
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8e1d5db. Configure here.
| .map(([name, server]) => { | ||
| const isSelected = | ||
| selectedServers?.includes(name) ?? false; | ||
| const isConnected = |
There was a problem hiding this comment.
Partial status missing from server sort order
Low Severity
The statusOrder map used to sort servers in the chat input dropdown does not include the new "partial" connection status. Servers with "partial" status fall through to the default value of 3, sorting them alongside "disconnected" servers at the bottom of the list. Since isConnectedStatus treats "partial" as connected everywhere else, these servers logically belong near "connected" (position 0).
Reviewed by Cursor Bugbot for commit 8e1d5db. 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
Medium Risk
Connection/validation flow is refactored across SDK, server routes, and client state, and now drives UI/exit codes from richer connect reports; regressions could affect connect reliability and OAuth reconnect behavior.
Overview
Unifies MCP connection/validation around a new SDK
connectServerWithReportthat returns a structuredConnectReport(includingconnectedvspartialvsoauth_required, issue codes/phases, init info, and optional diagnostics) and uses it in the CLI and inspector server routes instead of ad-hoc connect + tool-fetch checks.Inspector client state and UI are updated to store/display
lastConnectionReport, treatpartialas a connected state viaisConnectedStatus, and surface OAuth-required/diagnostic details in server cards/modals; connection APIs now pass anoauthContextthrough web/hosted/guest requests.Adds OAuth “strategy” support end-to-end: the server form captures protocol version + registration strategy, persists/reads these in stored OAuth config, enforces preregistered client ID requirements, and preserves strategy details during reconnects/forced OAuth flows (with updated tests).
Reviewed by Cursor Bugbot for commit 0426664. Bugbot is set up for automated code reviews on this repo. Configure here.