new connect logic#1915
Conversation
chelojimenez
had a problem deploying
to
preview-pr-1915
GitHub Actions
Error
dosubot
Bot
added
the
size:XXL
✅ 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. |
chelojimenez
temporarily deployed
to
preview-pr-1915
GitHub Actions
Inactive
Internal previewPreview URL will appear in Railway after the deploy finishes. |
|
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:
WalkthroughAdds an OAuth authorization-planning layer and public plan-resolution APIs to the SDK; runOAuthLogin now resolves an authorization plan, probes when needed, and embeds protocol/registration modes and the full plan in results. The CLI oauth login flow uses a new login config builder with stricter validation and makes protocol/registration options optional/overrides. The inspector gains workspace connection defaults (headers, request timeout), advanced connection-settings UI, OAuth protocol/registration controls, client-capabilities overrides, and persists/propagates oauthResourceUrl/protocolMode/registrationMode. Traffic logging records automatic-resolution trace entries. 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| value === "2025-06-18" || | ||
| value === "2025-11-25" | ||
| ? value | ||
| : "2025-11-25"; |
There was a problem hiding this comment.
Duplicate normalizeOauthProtocolMode function across two files
Low Severity
normalizeOauthProtocolMode is defined with identical logic in both AddServerModal.tsx and use-server-form.ts. Having two copies of the same normalization function means a future change to one could be missed in the other. The function in use-server-form.ts already exists and could be exported for reuse.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 35093c8. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35093c8698
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| url: input.serverUrl, | ||
| protocolVersion: basePlan.protocolVersion, | ||
| headers: input.customHeaders, | ||
| timeoutMs: input.stepTimeout, |
There was a problem hiding this comment.
Automatic login planning calls probeMcpServer before normalizeOAuthConformanceConfig applies its default step timeout, and this code forwards input.stepTimeout directly. When callers omit stepTimeout (the common SDK case), timeoutMs is undefined, so the probe runs without an abort deadline and can hang indefinitely on a stalled network/server response. Use the same default timeout used by the normalized config (e.g. 30_000ms) for this probe path.
Useful? React with 👍 / 👎.
| const hasPreregisteredCredentials = Boolean( | ||
| input.useRegistryOAuthProxy || trimmedClientId || trimmedClientSecret, | ||
| ); |
There was a problem hiding this comment.
Auto planning currently treats any single preregistered field (clientId or clientSecret) as enough to pick preregistered, so status can be ready even when required preregistered inputs are missing. A concrete case is authMode=client_credentials with only clientId: this branch selects preregistered here, then runOAuthLogin later fails during config normalization because client_secret is required. Auto mode should only select preregistered when the credentials are complete for the selected auth mode, otherwise return a blocked plan.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
sdk/src/server-probe.ts (1)
588-606:⚠️ Potential issue | 🟡 MinorDiscovery-error fallback bypasses the new registration resolver.
When auth-server metadata discovery throws, the required-OAuth branch hardcodes
registrationStrategies: ["preregistered"]. But under the new resolver, protocol2025-11-25is eligible for CIMD independent of metadata (CIMD doesn't rely onregistration_endpoint). So a momentarily-unreachable metadata endpoint will silently strip CIMD from the client's option set, even though the protocol version alone would have justified offering it.Consider threading the protocol version through so the fallback still gets CIMD where warranted:
🧭 Suggested alignment with the resolver
return { required: true, optional: false, wwwAuthenticate: wwwAuthenticateHeader, resourceMetadataUrl, - registrationStrategies: ["preregistered"], + registrationStrategies: resolveRegistrationStrategies( + protocolVersion, + undefined, + ), discoveryError: error instanceof Error ? error.message : String(error), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/server-probe.ts` around lines 588 - 606, The fallback in the catch block for discovery currently hardcodes registrationStrategies: ["preregistered"], which drops CIMD even when the protocol version alone (e.g., "2025-11-25") would allow it; update the error-path inside the catch in server-probe.ts to derive registrationStrategies from the protocol version rather than hardcoding: thread the negotiated protocol version (the same value used by the registration resolver) into this branch and either call the resolver helper (e.g., resolveRegistrationStrategies or the equivalent registration resolver used elsewhere) with that protocolVersion, or apply the same rule (include CIMD when protocolVersion >= "2025-11-25") so discoveryError responses still advertise CIMD when appropriate while keeping wwwAuthenticateHeader, resourceMetadataUrl and discoveryError behavior intact.mcpjam-inspector/client/src/lib/workspace-serialization.ts (1)
235-254:⚠️ Potential issue | 🟡 MinorAsymmetric
resourceUrldefault can trigger spurious "changed" verdicts.
deserializeServersFromConvexnormalizes a missingresourceUrlto""(line 148), but here the remote profile keeps it asundefinedwhen neitheroauthResourceUrlnoroauthFlowProfile.resourceUrlis set. If the flat branch fires becauseoauthScopesorclientIdis present but no resource URL is configured, the local stringified profile contains"resourceUrl":""while the remote one drops the key — they compare as different and the workspace keeps believing remote is out of sync. The same hazard applies toscopes/clientIdon the remote side, though those already have matching|| ""fallbacks in deserialize. Worth aligning.🛠️ Proposed alignment
: { ...(remoteServer.oauthFlowProfile ?? {}), scopes: Array.isArray(remoteServer.oauthScopes) ? remoteServer.oauthScopes.join(",") - : remoteServer.oauthScopes, - clientId: remoteServer.clientId, + : (remoteServer.oauthScopes ?? ""), + clientId: remoteServer.clientId ?? "", resourceUrl: - remoteServer.oauthResourceUrl ?? - remoteServer.oauthFlowProfile?.resourceUrl, + remoteServer.oauthResourceUrl ?? + remoteServer.oauthFlowProfile?.resourceUrl ?? + "", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/workspace-serialization.ts` around lines 235 - 254, The remote OAuth profile construction in remoteOAuthProfile is creating undefined/omitted keys (notably resourceUrl, scopes, clientId) which causes JSON.stringify mismatch against localServer.oauthFlowProfile; change the remoteOAuthProfile builder so resourceUrl defaults to "" (e.g. use ?? "" on the computed resourceUrl) and ensure scopes and clientId are normalized to the same empty-string form the deserializeServersFromConvex uses (e.g. scopes: Array.isArray(...) ? joined : (remoteServer.oauthScopes ?? ""), clientId: remoteServer.clientId ?? ""), so the two profiles compare consistently before the JSON.stringify check.cli/src/commands/oauth.ts (1)
98-147:⚠️ Potential issue | 🟠 Major
oauth loginlost the--print-urlflag.
buildOAuthLoginConfig()still validatesoptions.printUrland wires the stderr printer, but the command definition no longer registers the option. Commander will rejectoauth login --print-url, so the new code path is unreachable.Suggested fix
.option( + "--print-url", + "In interactive mode, print the consent URL to stderr instead of launching a browser", + ) + .option( "--debug-out <path>", "Write a structured debug artifact to a file", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/oauth.ts` around lines 98 - 147, The oauth login command definition no longer registers the --print-url flag but buildOAuthLoginConfig still expects options.printUrl and wires the stderr URL printer, causing Commander to reject calls with --print-url; restore the CLI option by adding a boolean --print-url flag to the oauth.command("login") options (matching the name options.printUrl) with an appropriate description and default (false) so buildOAuthLoginConfig and the stderr printer receive the value as before.mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts (1)
484-528:⚠️ Potential issue | 🔴 CriticalBearer token silently dropped when no custom headers are set.
In
buildFormData(), theexplicitHeaderssnapshot at lines 492–493 is taken before the bearer branch addsAuthorizationat line 506. WhencustomHeadersis empty,headersstarts as an empty object, soexplicitHeadersevaluates toundefined. The later mutationheaders["Authorization"] = Bearer ...never updatesexplicitHeaders, leaving itundefinedin the returned payload. When at least one custom header exists, both variables reference the same object, so the mutation propagates and the bug is masked.The bearer token is silently dropped—the user picks bearer auth, enters a token, and the submitted form ships
headers: undefined.🐛 Proposed fix — compute the snapshot after auth handling
// Add custom headers customHeaders.forEach(({ key, value }) => { if (key.trim()) { headers[key.trim()] = value; } }); - const explicitHeaders = - Object.keys(headers).length > 0 ? headers : undefined; // Parse OAuth scopes from input const scopes = oauthScopesInput .trim() .split(/\s+/) .filter((s) => s.length > 0); const shouldUsePreregisteredCredentials = authType === "oauth" && oauthRegistrationMode === "preregistered"; // Handle authentication let useOAuth = false; if (authType === "bearer" && bearerToken) { headers["Authorization"] = `Bearer ${bearerToken.trim()}`; } else if (authType === "oauth") { useOAuth = true; } + const explicitHeaders = + Object.keys(headers).length > 0 ? headers : undefined; + return { name: name.trim(), type: "http", url: url.trim(), headers: explicitHeaders,🤖 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 484 - 528, The function buildFormData currently computes explicitHeaders from headers before the authentication branch, so when authType === "bearer" and Authorization is added later (headers["Authorization"] = ...), explicitHeaders remains undefined and the header is dropped; fix by moving the snapshot/assignment of explicitHeaders (the Object.keys(headers).length > 0 ? headers : undefined expression) to after the auth handling (after the Authorization assignment) or recompute it immediately before the return so explicitHeaders reflects any Authorization added, referencing the headers variable, the Authorization assignment, authType, bearerToken, and the returned headers field.
🧹 Nitpick comments (11)
mcpjam-inspector/client/src/components/connection/shared/AdvancedConnectionSettingsSection.tsx (1)
76-92: Nit: associate the label with its input.The "Timeout" label is a plain
<label>withouthtmlFor/id, so clicking it doesn't focus the input and screen readers can't announce the association. Same pattern for the Headers and Capabilities sections below.🤖 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/AdvancedConnectionSettingsSection.tsx` around lines 76 - 92, The Timeout label in AdvancedConnectionSettingsSection.tsx is not associated with its input, so add a unique id (e.g., timeoutInputId) on the Input used for requestTimeout and set the label's htmlFor to that id; update the Input usage (the element that receives value/requestTimeout and onChange/onRequestTimeoutChange) to include the id prop. Apply the same pattern to the Headers and Capabilities label/input pairs in this component (give each input a distinct id and reference it from the corresponding label) to restore click-to-focus and proper screen-reader association.mcpjam-inspector/client/src/components/ServersTab.tsx (1)
1119-1126: Telemetry label now lags the UI rename — intentional?The user-facing label became "Connection Settings" but this event still fires as
client_config_button_clicked(and the state/handler namesisClientConfigOpen/handleOpenClientConfigretain the old vocabulary). If you want historical continuity in PostHog dashboards, leaving the event name alone is perfectly defensible; if not, it's worth aligning the event so future analytics aren't archaeological.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/ServersTab.tsx` around lines 1119 - 1126, The telemetry event and some identifiers still use the old "client_config" naming while the UI label is "Connection Settings"; update posthog.capture in handleOpenClientConfig to use a new event key like "connection_settings_button_clicked" (and optionally send the legacy "client_config_button_clicked" as a secondary event if you need continuity), then rename related symbols isClientConfigOpen and setIsClientConfigOpen (and the handler handleOpenClientConfig) to isConnectionSettingsOpen, setIsConnectionSettingsOpen, and handleOpenConnectionSettings respectively (also update any other references, tests, and analytics docs to match the new event name or ensure both events are emitted if preserving historical dashboards).sdk/tests/oauth-login.test.ts (1)
123-211: Consider hoisting the shared CIMDfetchFnscaffolding.This new test duplicates nearly every branch of the fetch mock from the preceding case (lines 36–96), with only the
runOAuthLoginconfig and final assertions differing. Extracting a smallcreateCimdFetchMock({ serverUrl, resourceMetadataUrl, authServerUrl })helper would keep the auto-resolution test focused on what it actually proves — that planning fields get populated when no protocol/registration is supplied — and make the next CIMD scenario free to write.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tests/oauth-login.test.ts` around lines 123 - 211, The test duplicates the large CIMD fetch mock (fetchFn) used elsewhere; extract that repeated scaffolding into a reusable helper (e.g., createCimdFetchMock) and use it in this test instead of inlining fetchFn. Implement createCimdFetchMock to accept parameters like serverUrl, resourceMetadataUrl, authServerUrl and return a jest mock matching the current branches (responses for serverUrl unauthenticated 401 with WWW-Authenticate, resource metadata, authorization server well-known, DEFAULT_MCPJAM_CLIENT_ID_METADATA_URL, token endpoint, and authenticated serverUrl -> createMcpInitializeResponse), then replace the inline fetchFn passed to runOAuthLogin with createCimdFetchMock(...) to keep this test focused on runOAuthLogin behavior.mcpjam-inspector/client/src/components/connection/__tests__/EditServerFormContent.hosted.test.tsx (1)
28-30: Remove the orphanedCustomHeadersSectionmock.
EditServerFormContentdoesn't importCustomHeadersSection, and neither doesAdvancedConnectionSettingsSection(which handles headers inline). The mock at lines 28–30 is never used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/connection/__tests__/EditServerFormContent.hosted.test.tsx` around lines 28 - 30, Remove the unused mock for CustomHeadersSection in the test file—delete the vi.mock call that defines CustomHeadersSection (the mock returning <div data-testid="custom-headers-section" />) from EditServerFormContent.hosted.test.tsx because EditServerFormContent and AdvancedConnectionSettingsSection do not import or use CustomHeadersSection; leaving this orphaned vi.mock is unnecessary and should be removed to clean up the test.mcpjam-inspector/client/src/components/client-config/ClientConfigTab.tsx (2)
229-236: Inline reset escapes the design system.The header uses
<Button>from@mcpjam/design-system/button, while the per-section reset drops to a raw<button>with ad-hoc Tailwind. It renders fine, but it bypasses whatever focus ring/disabled/hover semantics the design system provides, and it won't follow along if the design system tightens accessibility. A<Button variant="ghost" size="sm">with theRotateCcwicon would keep everything speaking the same visual language.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/client-config/ClientConfigTab.tsx` around lines 229 - 236, The per-section reset is using a raw <button> with Tailwind which bypasses the design system; replace that element in ClientConfigTab with the design system Button component imported from `@mcpjam/design-system/button`, using the same icon (RotateCcw) and props to match header styling (e.g., Button variant="ghost" size="sm") and wire its onClick to resetSectionToDefault(section.key) so it preserves design-system focus, disabled and hover semantics.
155-177: Tiny render-churn thought on the inlinesectionsarray.The
sectionsarray is rebuilt on every render, which means the mapped<section>children get fresh object identities even when none of the underlying text/error/height changed. With three sections andJsonEditor's internal state this is almost certainly a non-issue, but if you see unnecessary editor re-renders later, wrapping this inuseMemokeyed on the six values it references is a cheap stabilizer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/client-config/ClientConfigTab.tsx` around lines 155 - 177, The inline sections array in ClientConfigTab is recreated on every render causing unstable object identities for sections; wrap the sections definition in React's useMemo (import/use useMemo) and return the array from useMemo with a dependency array containing connectionDefaultsText, connectionDefaultsError, connectionDefaultsHeight, clientCapabilitiesText, clientCapabilitiesError, clientCapabilitiesHeight, hostContextText, hostContextError, and hostContextHeight so the sections array is only rebuilt when those values change; keep the same shape/keys ("connectionDefaults", "clientCapabilities", "hostContext") so mapped <section> children and JsonEditor receive stable references.mcpjam-inspector/client/src/test/mocks/stores.ts (1)
183-203: Consider reusing the real default helper to keep the mock honest.Hardcoding
{ headers: {}, requestTimeout: 10000 }in two places duplicatesbuildDefaultWorkspaceConnectionDefaults()from@/lib/client-config(already imported by the real store). If the production default ever changes — say a longer timeout or a new field — this mock will silently drift, and the tests relying on it will report green against stale assumptions.♻️ Proposed refactor
import { vi } from "vitest"; import type { AppState, ServerWithName, Workspace } from "@/state/app-types"; import type { HostDisplayMode, WorkspaceClientConfig, } from "@/lib/client-config"; +import { buildDefaultWorkspaceConnectionDefaults } from "@/lib/client-config"; import { createServer, createWorkspace } from "../factories"; ... export function createMockWorkspaceClientConfig( overrides: Partial<WorkspaceClientConfig> = {}, ): WorkspaceClientConfig { return { version: 1, connectionDefaults: - overrides.connectionDefaults ?? { headers: {}, requestTimeout: 10000 }, + overrides.connectionDefaults ?? buildDefaultWorkspaceConnectionDefaults(), clientCapabilities: overrides.clientCapabilities ?? {}, hostContext: overrides.hostContext ?? {}, }; } ... connectionDefaultsText: stringifyJson( - draftConfig?.connectionDefaults ?? { headers: {}, requestTimeout: 10000 }, + draftConfig?.connectionDefaults ?? buildDefaultWorkspaceConnectionDefaults(), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/test/mocks/stores.ts` around lines 183 - 203, The mock duplicates the production defaults by hardcoding { headers: {}, requestTimeout: 10000 } in createMockClientConfigStoreState (for connectionDefaultsText); replace that literal with the real helper buildDefaultWorkspaceConnectionDefaults() so the mock stays in sync with production defaults, and ensure stringifyJson(draftConfig?.connectionDefaults ?? buildDefaultWorkspaceConnectionDefaults()) is used; add or reuse an import for buildDefaultWorkspaceConnectionDefaults if missing.mcpjam-inspector/client/src/lib/oauth/oauth-trace.ts (1)
323-370: Session-trace persistence is tidy and fault-tolerant.The new
sessionStorage-backed helpers cleanly mirror thelocalStoragecounterparts (shape validation,httpHistorydefault, swallowed quota errors), and the acknowledged "lost across redirect" tradeoff is a reasonable posture for a convenience resume trail.One optional thought:
saveOAuthTracealready has a "drophttpHistoryand retry" fallback when quota bites — you may want the same here eventually so a noisy trace doesn't take the whole session entry down with it on the redirect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/oauth-trace.ts` around lines 323 - 370, Add the same quota-fallback behavior to saveOAuthTraceToSession that saveOAuthTrace uses: when sessionStorage.setItem throws (quota/unavailable), catch the error, create a new persistable trace with httpHistory dropped (use buildPersistableTrace(trace) then remove httpHistory) and attempt sessionStorage.setItem again using sessionTraceKey(serverName); if the retry also fails, swallow the error as currently done. This change should be made inside saveOAuthTraceToSession and reference sessionTraceKey, buildPersistableTrace, and the same stringified payload path used now.mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts (1)
1099-1110: Persisted config assertion is precise but order-fragile — worth noting.Asserting the exact
JSON.stringifyoutput pins the implementation to a specific key-insertion order (registryServerId,useRegistryOAuthProxy,resourceUrl,protocolMode,protocolVersion,registrationMode,registrationStrategy). Functionally fine today, but a future refactor ofwriteStoredOAuthConfigthat reorders those writes will fail this test for a non-behavioral reason. ConsidertoEqual(JSON.parse(...))on the stored string if you'd like to decouple from key order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts` around lines 1099 - 1110, The test currently compares the exact JSON string from localStorage which is fragile to key order; update the assertion to parse the stored string and compare objects instead (e.g., JSON.parse(localStorage.getItem("mcp-oauth-config-asana"))). Use toEqual(...) against the expected object so the test checks content rather than property insertion order; update the assertion near the existing expect(localStorage.getItem("mcp-oauth-config-asana")) line that validates the output of writeStoredOAuthConfig.mcpjam-inspector/client/src/components/connection/hooks/__tests__/use-server-form.test.ts (1)
146-160:localStoragecleanup is conditional on the assertion passing.If
waitForthrows, theremoveItemon line 159 never runs and the seededmcp-oauth-config-Existing OAuth serverentry leaks into whatever test runs next in the same worker — turning a single failure into a cascading mystery. Atry { … } finally { localStorage.removeItem(...) }(or anafterEach(() => localStorage.clear())at the describe level) makes this resilient to red states.🤖 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/__tests__/use-server-form.test.ts` around lines 146 - 160, The test seeds localStorage then asserts inside waitFor but only calls localStorage.removeItem on the success path, so a failing waitFor leaks the "mcp-oauth-config-Existing OAuth server" entry; update the test around useServerForm (or the whole describe) to guarantee cleanup — either wrap the waitFor/assert block in try { … } finally { localStorage.removeItem("mcp-oauth-config-Existing OAuth server") } or add an afterEach(() => localStorage.clear() or localStorage.removeItem(...)) to ensure removal regardless of test outcome.mcpjam-inspector/client/src/stores/client-config-store.ts (1)
189-207: Authorization is blocked; consider siblings likeproxy-authorization.The case-insensitive check on
authorizationis the right instinct for not persisting secrets into workspace-level defaults. If you want belt-and-braces,proxy-authorizationandcookiecarry the same risk profile and would round out the denylist. Keeping as an optional thought sinceauthorizationis the overwhelmingly common case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/stores/client-config-store.ts` around lines 189 - 207, The current validation in client-config-store.ts checks connectionDefaults.headers for a case-insensitive "authorization" key only; extend the denylist to also reject "proxy-authorization" and "cookie" (case-insensitive) to avoid persisting similar secrets. In the headers validation loop inside the block that inspects connectionDefaults.headers, replace the single lowercase check (key.toLowerCase() === "authorization") with a check against a small set/array of forbidden header names (e.g., ["authorization","proxy-authorization","cookie"]) and throw the same style of Error when any of those are present; ensure the thrown message references the offending header name for clarity.
🤖 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/__tests__/AdvancedConnectionSettingsSection.test.tsx`:
- Around line 23-60: The tests assert labels and summary chips that no longer
match AdvancedConnectionSettingsSection's current markup; fix by either
restoring the summary-chip UX in AdvancedConnectionSettingsSection (render a
collapsed summary row showing "Connection Overrides" with a header count like "1
header configured" and a timeout pill "Timeout: 30000ms", and ensure the
add-header button text is "Add Header" or "Add" per test) or update the test
strings in AdvancedConnectionSettingsSection.test.tsx to match the component's
actual output (change expectations from "Connection Overrides" → "Connection
overrides", remove checks for summary chips if the component no longer shows
them, change "Custom Headers" → "Headers", button /add header/i → match "Add",
"Request Timeout" → "Timeout", and "Client Capabilities Override" →
"Capabilities override"); locate references by the component name
AdvancedConnectionSettingsSection and the test file
AdvancedConnectionSettingsSection.test.tsx and adjust either the component's
render logic for collapsed state or the test expectations accordingly.
In `@mcpjam-inspector/client/src/components/connection/AddServerModal.tsx`:
- Around line 124-135: The header hydration currently filters out only the exact
"Authorization" key so case-variants (e.g., "authorization") get treated as
custom headers; update the filter used when reading initialData.headers in
AddServerModal.tsx to compare keys case-insensitively (e.g., key.toLowerCase()
=== 'authorization') so Authorization is excluded from the custom headers array,
and ensure you still call formState.setCustomHeaders(...) and
formState.setShowConfiguration(true) only for non-auth headers.
In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts`:
- Around line 183-196: registrationModeValue currently trusts
server.oauthFlowProfile?.registrationStrategy directly (unlike protocolModeValue
which uses normalizeOauthProtocolMode), which can pull corrupted values from
localStorage; add a normalization step similar to normalizeOauthProtocolMode by
implementing normalizeOauthRegistrationMode(value?: string):
ServerFormOAuthRegistrationMode | undefined that returns only "auto" | "cimd" |
"dcr" | "preregistered" or undefined, then replace uses of
server.oauthFlowProfile?.registrationStrategy in the registrationModeValue
expression with
normalizeOauthRegistrationMode(server.oauthFlowProfile?.registrationStrategy) so
the form state is defensively sanitized before falling back to oauthConfig or
savedClientId/savedClientSecret logic.
In
`@mcpjam-inspector/client/src/components/connection/shared/AdvancedConnectionSettingsSection.tsx`:
- Around line 110-137: The list uses array index as React key which causes
inputs to remount on removal; update the header model to include a stable id
when adding headers (e.g., in onAddHeader generate an id like
crypto.randomUUID()), change customHeaders items to {id, key, value}, and use
that id as the map key in AdvancedConnectionSettingsSection instead of
key={index}; update onUpdateHeader and onRemoveHeader to operate by id (or find
index by id) so focus and input state are preserved when rows are removed or
reordered.
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 948-952: The OAuth callback builds oauthServerEntry using
result.serverConfig which only contains url and bearer token; do not replace
existingServer.config with it because that drops saved custom headers, timeout,
and capability overrides and causes guardedTestConnection() to run without
required headers. Fix by merging configs instead of overwriting: preserve
existingServer.config values and overlay the callback fields (url and token) so
saved headers/timeouts/capabilities remain, ensuring headers from
existingServer.config are merged with any headers in result.serverConfig; apply
the same merge at the other occurrence mentioned (lines ~989-992) where
result.serverConfig is used.
- Around line 240-243: The scopes string is being created space-delimited which
breaks later parsing; update the construction of oauthFlowProfile.scopes so it
remains comma-delimited by using storedOAuthConfig.scopes?.join(",") (or
otherwise producing a comma-separated string) instead of join(" "), referencing
the oauthFlowProfile.scopes assignment in use-server-state.ts and the
syncServerToConvex() flow that splits on commas.
In `@mcpjam-inspector/client/src/lib/client-config.ts`:
- Around line 432-441: The function normalizeWorkspaceRequestTimeout currently
accepts 0 and negative numbers because it only checks Number.isFinite; update
normalizeWorkspaceRequestTimeout to reject non-positive values by also verifying
value > 0 and returning fallback when value is not a finite positive number,
ensuring any persisted or user-provided timeout <= 0 falls back to the provided
default.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 1919-1920: The saved pre-redirect trace created by
saveOAuthTraceToSession(options.serverName, preRedirectTrace) lacks a
corresponding cleanup, so update clearOAuthData() to also remove the
session-stored trace for the same server key; specifically, ensure
clearOAuthData (or its caller) invokes the session-trace removal routine (the
counterpart used by saveOAuthTraceToSession) using the same options.serverName
so the preRedirectTrace from emitTraceFromState(getState()) is cleared and
cannot merge into the next OAuth run.
- Around line 515-565: The auto-discovery probe is not receiving caller-supplied
headers: forward options.customHeaders into the discovery step and into the
final plan so the probe can include workspace/server headers. Concretely, update
resolveOAuthExecutionPlan to call loadCallbackDiscoveryState(provider,
options.serverUrl, fetchFn, options.customHeaders) (or the appropriate
additional param) and pass options.customHeaders into the second
resolveAuthorizationPlan invocation (include a customHeaders property in that
call) so loadCallbackDiscoveryState and the returned discovery snapshot are
produced using the supplied headers; adjust loadCallbackDiscoveryState /
toAuthorizationDiscoverySnapshot signatures as needed to accept and propagate
customHeaders.
In `@mcpjam-inspector/client/src/stores/client-config-store.ts`:
- Around line 177-216: The validation in parseConnectionDefaultsJson currently
only checks for finite numbers, allowing zero or negative requestTimeouts;
update the guard for requestTimeout (the requestTimeout local and its validation
block in parseConnectionDefaultsJson) to require a positive finite number (e.g.,
> 0) and throw a clear error like "connectionDefaults.requestTimeout must be a
positive number" when it fails; keep the existing fallback to
buildDefaultWorkspaceConnectionDefaults().requestTimeout when requestTimeout is
undefined, but reject 0 or negatives as invalid.
---
Outside diff comments:
In `@cli/src/commands/oauth.ts`:
- Around line 98-147: The oauth login command definition no longer registers the
--print-url flag but buildOAuthLoginConfig still expects options.printUrl and
wires the stderr URL printer, causing Commander to reject calls with
--print-url; restore the CLI option by adding a boolean --print-url flag to the
oauth.command("login") options (matching the name options.printUrl) with an
appropriate description and default (false) so buildOAuthLoginConfig and the
stderr printer receive the value as before.
In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts`:
- Around line 484-528: The function buildFormData currently computes
explicitHeaders from headers before the authentication branch, so when authType
=== "bearer" and Authorization is added later (headers["Authorization"] = ...),
explicitHeaders remains undefined and the header is dropped; fix by moving the
snapshot/assignment of explicitHeaders (the Object.keys(headers).length > 0 ?
headers : undefined expression) to after the auth handling (after the
Authorization assignment) or recompute it immediately before the return so
explicitHeaders reflects any Authorization added, referencing the headers
variable, the Authorization assignment, authType, bearerToken, and the returned
headers field.
In `@mcpjam-inspector/client/src/lib/workspace-serialization.ts`:
- Around line 235-254: The remote OAuth profile construction in
remoteOAuthProfile is creating undefined/omitted keys (notably resourceUrl,
scopes, clientId) which causes JSON.stringify mismatch against
localServer.oauthFlowProfile; change the remoteOAuthProfile builder so
resourceUrl defaults to "" (e.g. use ?? "" on the computed resourceUrl) and
ensure scopes and clientId are normalized to the same empty-string form the
deserializeServersFromConvex uses (e.g. scopes: Array.isArray(...) ? joined :
(remoteServer.oauthScopes ?? ""), clientId: remoteServer.clientId ?? ""), so the
two profiles compare consistently before the JSON.stringify check.
In `@sdk/src/server-probe.ts`:
- Around line 588-606: The fallback in the catch block for discovery currently
hardcodes registrationStrategies: ["preregistered"], which drops CIMD even when
the protocol version alone (e.g., "2025-11-25") would allow it; update the
error-path inside the catch in server-probe.ts to derive registrationStrategies
from the protocol version rather than hardcoding: thread the negotiated protocol
version (the same value used by the registration resolver) into this branch and
either call the resolver helper (e.g., resolveRegistrationStrategies or the
equivalent registration resolver used elsewhere) with that protocolVersion, or
apply the same rule (include CIMD when protocolVersion >= "2025-11-25") so
discoveryError responses still advertise CIMD when appropriate while keeping
wwwAuthenticateHeader, resourceMetadataUrl and discoveryError behavior intact.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/client-config/ClientConfigTab.tsx`:
- Around line 229-236: The per-section reset is using a raw <button> with
Tailwind which bypasses the design system; replace that element in
ClientConfigTab with the design system Button component imported from
`@mcpjam/design-system/button`, using the same icon (RotateCcw) and props to match
header styling (e.g., Button variant="ghost" size="sm") and wire its onClick to
resetSectionToDefault(section.key) so it preserves design-system focus, disabled
and hover semantics.
- Around line 155-177: The inline sections array in ClientConfigTab is recreated
on every render causing unstable object identities for sections; wrap the
sections definition in React's useMemo (import/use useMemo) and return the array
from useMemo with a dependency array containing connectionDefaultsText,
connectionDefaultsError, connectionDefaultsHeight, clientCapabilitiesText,
clientCapabilitiesError, clientCapabilitiesHeight, hostContextText,
hostContextError, and hostContextHeight so the sections array is only rebuilt
when those values change; keep the same shape/keys ("connectionDefaults",
"clientCapabilities", "hostContext") so mapped <section> children and JsonEditor
receive stable references.
In
`@mcpjam-inspector/client/src/components/connection/__tests__/EditServerFormContent.hosted.test.tsx`:
- Around line 28-30: Remove the unused mock for CustomHeadersSection in the test
file—delete the vi.mock call that defines CustomHeadersSection (the mock
returning <div data-testid="custom-headers-section" />) from
EditServerFormContent.hosted.test.tsx because EditServerFormContent and
AdvancedConnectionSettingsSection do not import or use CustomHeadersSection;
leaving this orphaned vi.mock is unnecessary and should be removed to clean up
the test.
In
`@mcpjam-inspector/client/src/components/connection/hooks/__tests__/use-server-form.test.ts`:
- Around line 146-160: The test seeds localStorage then asserts inside waitFor
but only calls localStorage.removeItem on the success path, so a failing waitFor
leaks the "mcp-oauth-config-Existing OAuth server" entry; update the test around
useServerForm (or the whole describe) to guarantee cleanup — either wrap the
waitFor/assert block in try { … } finally {
localStorage.removeItem("mcp-oauth-config-Existing OAuth server") } or add an
afterEach(() => localStorage.clear() or localStorage.removeItem(...)) to ensure
removal regardless of test outcome.
In
`@mcpjam-inspector/client/src/components/connection/shared/AdvancedConnectionSettingsSection.tsx`:
- Around line 76-92: The Timeout label in AdvancedConnectionSettingsSection.tsx
is not associated with its input, so add a unique id (e.g., timeoutInputId) on
the Input used for requestTimeout and set the label's htmlFor to that id; update
the Input usage (the element that receives value/requestTimeout and
onChange/onRequestTimeoutChange) to include the id prop. Apply the same pattern
to the Headers and Capabilities label/input pairs in this component (give each
input a distinct id and reference it from the corresponding label) to restore
click-to-focus and proper screen-reader association.
In `@mcpjam-inspector/client/src/components/ServersTab.tsx`:
- Around line 1119-1126: The telemetry event and some identifiers still use the
old "client_config" naming while the UI label is "Connection Settings"; update
posthog.capture in handleOpenClientConfig to use a new event key like
"connection_settings_button_clicked" (and optionally send the legacy
"client_config_button_clicked" as a secondary event if you need continuity),
then rename related symbols isClientConfigOpen and setIsClientConfigOpen (and
the handler handleOpenClientConfig) to isConnectionSettingsOpen,
setIsConnectionSettingsOpen, and handleOpenConnectionSettings respectively (also
update any other references, tests, and analytics docs to match the new event
name or ensure both events are emitted if preserving historical dashboards).
In `@mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts`:
- Around line 1099-1110: The test currently compares the exact JSON string from
localStorage which is fragile to key order; update the assertion to parse the
stored string and compare objects instead (e.g.,
JSON.parse(localStorage.getItem("mcp-oauth-config-asana"))). Use toEqual(...)
against the expected object so the test checks content rather than property
insertion order; update the assertion near the existing
expect(localStorage.getItem("mcp-oauth-config-asana")) line that validates the
output of writeStoredOAuthConfig.
In `@mcpjam-inspector/client/src/lib/oauth/oauth-trace.ts`:
- Around line 323-370: Add the same quota-fallback behavior to
saveOAuthTraceToSession that saveOAuthTrace uses: when sessionStorage.setItem
throws (quota/unavailable), catch the error, create a new persistable trace with
httpHistory dropped (use buildPersistableTrace(trace) then remove httpHistory)
and attempt sessionStorage.setItem again using sessionTraceKey(serverName); if
the retry also fails, swallow the error as currently done. This change should be
made inside saveOAuthTraceToSession and reference sessionTraceKey,
buildPersistableTrace, and the same stringified payload path used now.
In `@mcpjam-inspector/client/src/stores/client-config-store.ts`:
- Around line 189-207: The current validation in client-config-store.ts checks
connectionDefaults.headers for a case-insensitive "authorization" key only;
extend the denylist to also reject "proxy-authorization" and "cookie"
(case-insensitive) to avoid persisting similar secrets. In the headers
validation loop inside the block that inspects connectionDefaults.headers,
replace the single lowercase check (key.toLowerCase() === "authorization") with
a check against a small set/array of forbidden header names (e.g.,
["authorization","proxy-authorization","cookie"]) and throw the same style of
Error when any of those are present; ensure the thrown message references the
offending header name for clarity.
In `@mcpjam-inspector/client/src/test/mocks/stores.ts`:
- Around line 183-203: The mock duplicates the production defaults by hardcoding
{ headers: {}, requestTimeout: 10000 } in createMockClientConfigStoreState (for
connectionDefaultsText); replace that literal with the real helper
buildDefaultWorkspaceConnectionDefaults() so the mock stays in sync with
production defaults, and ensure stringifyJson(draftConfig?.connectionDefaults ??
buildDefaultWorkspaceConnectionDefaults()) is used; add or reuse an import for
buildDefaultWorkspaceConnectionDefaults if missing.
In `@sdk/tests/oauth-login.test.ts`:
- Around line 123-211: The test duplicates the large CIMD fetch mock (fetchFn)
used elsewhere; extract that repeated scaffolding into a reusable helper (e.g.,
createCimdFetchMock) and use it in this test instead of inlining fetchFn.
Implement createCimdFetchMock to accept parameters like serverUrl,
resourceMetadataUrl, authServerUrl and return a jest mock matching the current
branches (responses for serverUrl unauthenticated 401 with WWW-Authenticate,
resource metadata, authorization server well-known,
DEFAULT_MCPJAM_CLIENT_ID_METADATA_URL, token endpoint, and authenticated
serverUrl -> createMcpInitializeResponse), then replace the inline fetchFn
passed to runOAuthLogin with createCimdFetchMock(...) to keep this test focused
on runOAuthLogin behavior.
🪄 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: 17e527df-3330-4107-b813-150ab63ea410
📒 Files selected for processing (42)
cli/src/commands/oauth.tscli/tests/oauth.test.tsmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/__tests__/ServersTab.test.tsxmcpjam-inspector/client/src/components/client-config/ClientConfigTab.tsxmcpjam-inspector/client/src/components/client-config/__tests__/ClientConfigTab.test.tsxmcpjam-inspector/client/src/components/client-config/__tests__/WorkspaceClientConfigSync.test.tsxmcpjam-inspector/client/src/components/connection/AddServerModal.tsxmcpjam-inspector/client/src/components/connection/EditServerFormContent.tsxmcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/connection/__tests__/AdvancedConnectionSettingsSection.test.tsxmcpjam-inspector/client/src/components/connection/__tests__/AuthenticationSection.test.tsxmcpjam-inspector/client/src/components/connection/__tests__/EditServerFormContent.hosted.test.tsxmcpjam-inspector/client/src/components/connection/hooks/__tests__/use-server-form.test.tsmcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/components/connection/shared/AdvancedConnectionSettingsSection.tsxmcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/useWorkspaces.tsmcpjam-inspector/client/src/lib/client-config.tsmcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.hosted-session.test.tsmcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.tsmcpjam-inspector/client/src/lib/oauth/__tests__/oauth-trace.test.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/lib/oauth/oauth-trace.tsmcpjam-inspector/client/src/lib/oauth/profile.tsmcpjam-inspector/client/src/lib/workspace-serialization.tsmcpjam-inspector/client/src/state/oauth-orchestrator.tsmcpjam-inspector/client/src/state/server-helpers.tsmcpjam-inspector/client/src/stores/client-config-store.tsmcpjam-inspector/client/src/test/mocks/stores.tsmcpjam-inspector/shared/types.tssdk/src/browser.tssdk/src/index.tssdk/src/oauth-login.tssdk/src/oauth/authorization-plan.tssdk/src/oauth/state-machines/trace.tssdk/src/server-probe.tssdk/tests/browser-entry.test.tssdk/tests/oauth-login.test.tssdk/tests/oauth/authorization-plan.test.ts
💤 Files with no reviewable changes (1)
- sdk/src/oauth/state-machines/trace.ts
| {customHeaders.length > 0 && ( | ||
| <div className="space-y-1"> | ||
| {customHeaders.map((header, index) => ( | ||
| <div key={index} className="flex items-center gap-1.5"> | ||
| <Input | ||
| value={header.key} | ||
| onChange={(e) => onUpdateHeader(index, "key", e.target.value)} | ||
| placeholder="Key" | ||
| className="h-7 flex-1 text-xs" | ||
| /> | ||
| <Input | ||
| value={header.value} | ||
| onChange={(e) => | ||
| onUpdateHeader(index, "value", e.target.value) | ||
| } | ||
| placeholder="Value" | ||
| className="h-7 flex-1 text-xs" | ||
| /> | ||
| <button | ||
| type="button" | ||
| onClick={() => onRemoveHeader(index)} | ||
| className="flex h-7 w-7 shrink-0 items-center justify-center rounded text-muted-foreground transition-colors hover:bg-muted hover:text-foreground" | ||
| > | ||
| <X className="h-3 w-3" /> | ||
| </button> | ||
| </div> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
Swap key={index} for a stable identifier to preserve focus on removal.
Using the array index as the React key means removing a row in the middle causes subsequent rows' inputs to be remounted/re-associated, which drops focus and can cause the wrong value to flicker into an adjacent input mid-edit. A stable per-header id (generated on onAddHeader) would make reordering/removal well-behaved.
♻️ Sketch
-interface HeaderEntry {
- key: string;
- value: string;
-}
+interface HeaderEntry {
+ id?: string;
+ key: string;
+ value: string;
+}
@@
- {customHeaders.map((header, index) => (
- <div key={index} className="flex items-center gap-1.5">
+ {customHeaders.map((header, index) => (
+ <div key={header.id ?? index} className="flex items-center gap-1.5">Pairs best with a small change in the header builder to emit stable ids (e.g. crypto.randomUUID()).
🤖 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/AdvancedConnectionSettingsSection.tsx`
around lines 110 - 137, The list uses array index as React key which causes
inputs to remount on removal; update the header model to include a stable id
when adding headers (e.g., in onAddHeader generate an id like
crypto.randomUUID()), change customHeaders items to {id, key, value}, and use
that id as the map key in AdvancedConnectionSettingsSection instead of
key={index}; update onUpdateHeader and onRemoveHeader to operate by id (or find
index by id) so focus and input state are preserved when rows are removed or
reordered.
…nspector into codex/new-connect-logic
chelojimenez
temporarily deployed
to
preview-pr-1915
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts (1)
1182-1192: Strict stringified-equality locks key order — deliberate but fragile.Asserting via
JSON.stringify({...})pins both the presence and the serialization order ofresourceUrl,protocolMode, andregistrationMode. If the producer ever reorders keys (e.g., movesprotocolVersionor adds a new field mid-object), this will break without a real regression. If you intend that strictness as a canary for persisted-config shape, great; otherwise switching toJSON.parse(localStorage.getItem(...))+toEqual({...})would be more resilient.♻️ Optional shape-based assertion
- expect(localStorage.getItem("mcp-oauth-config-asana")).toBe( - JSON.stringify({ - registryServerId: "registry-asana", - useRegistryOAuthProxy: true, - resourceUrl: "https://mcp.asana.com/v2/mcp", - protocolMode: "auto", - protocolVersion: "2025-11-25", - registrationMode: "preregistered", - registrationStrategy: "preregistered", - }) - ); + expect( + JSON.parse(localStorage.getItem("mcp-oauth-config-asana") ?? "null"), + ).toEqual({ + registryServerId: "registry-asana", + useRegistryOAuthProxy: true, + resourceUrl: "https://mcp.asana.com/v2/mcp", + protocolMode: "auto", + protocolVersion: "2025-11-25", + registrationMode: "preregistered", + registrationStrategy: "preregistered", + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts` around lines 1182 - 1192, The test currently asserts exact JSON string equality for localStorage.getItem("mcp-oauth-config-asana") which brittlely ties to key order; change the assertion to parse the stored string (JSON.parse(localStorage.getItem(...))) and compare the resulting object with toEqual(...) against the expected object shape so key order changes or new fields won't break the test; update the assertion in mcp-oauth.test.ts where the expect(...) call is made to use parsed object + toEqual with the same expected literal.mcpjam-inspector/client/src/stores/traffic-log-store.ts (1)
132-155: Synthetic decision row is idempotent and renders cleanly — nicely done.Keying the id on
step.startedAtmakes re-ingestion a stable upsert, and the payload carries enough viarelatedStepto cross-reference the originating step in expanded view. One optional tidy-up: the twoaddMcpServerLogcalls in this forEach share most of their skeleton (id prefix, serverId/serverName, direction, kind, oauth fields). A tiny local builder would dry this up without changing behavior.♻️ Illustrative extraction (optional)
+ const baseEntry = { + serverId, + serverName, + direction: "OAUTH" as const, + timestamp, + kind: "oauth" as const, + oauthSource: trace.source, + oauthStatus: step.status, + oauthRecovered: step.recovered === true, + }; + if (isAutomaticAuthorizationDecisionMessage(step.message)) { store.addMcpServerLog({ + ...baseEntry, id: `oauth:${serverId}:${trace.source}:automatic_resolution:${step.startedAt}`, - serverId, - serverName, - direction: "OAUTH", method: "Automatic Resolution", - timestamp, payload: { source: trace.source, step: "automatic_resolution", title: "Automatic Resolution", status: step.status, message: step.message, details: step.details, relatedStep: step.step, }, - kind: "oauth", - oauthStatus: step.status, - oauthSource: trace.source, - oauthRecovered: step.recovered === true, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/stores/traffic-log-store.ts` around lines 132 - 155, The two addMcpServerLog calls share most fields; create a small local builder (e.g., buildOauthLogBase or makeOauthLog) near the loop that returns the common log skeleton (id prefix, serverId, serverName, direction, kind, oauthStatus/oauthSource/oauthRecovered, timestamp and any common payload fields) and then spread/extend that base when calling addMcpServerLog in both branches (the branch using isAutomaticAuthorizationDecisionMessage and the other branch), preserving the existing id format (including step.startedAt), payload differences (relatedStep, step-specific title/method/status/message/details) and behavior of addMcpServerLog so functionality remains identical.
🤖 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/__tests__/mcp-oauth.test.ts`:
- Around line 1182-1192: The test currently asserts exact JSON string equality
for localStorage.getItem("mcp-oauth-config-asana") which brittlely ties to key
order; change the assertion to parse the stored string
(JSON.parse(localStorage.getItem(...))) and compare the resulting object with
toEqual(...) against the expected object shape so key order changes or new
fields won't break the test; update the assertion in mcp-oauth.test.ts where the
expect(...) call is made to use parsed object + toEqual with the same expected
literal.
In `@mcpjam-inspector/client/src/stores/traffic-log-store.ts`:
- Around line 132-155: The two addMcpServerLog calls share most fields; create a
small local builder (e.g., buildOauthLogBase or makeOauthLog) near the loop that
returns the common log skeleton (id prefix, serverId, serverName, direction,
kind, oauthStatus/oauthSource/oauthRecovered, timestamp and any common payload
fields) and then spread/extend that base when calling addMcpServerLog in both
branches (the branch using isAutomaticAuthorizationDecisionMessage and the other
branch), preserving the existing id format (including step.startedAt), payload
differences (relatedStep, step-specific title/method/status/message/details) and
behavior of addMcpServerLog so functionality remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04308086-b8ed-42d2-8481-a374b4366eeb
📒 Files selected for processing (4)
mcpjam-inspector/client/src/components/__tests__/logger-view.test.tsxmcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/stores/traffic-log-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4f31572e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const discoveryState = await loadCallbackDiscoveryState( | ||
| provider, | ||
| options.serverUrl, | ||
| fetchFn | ||
| ); |
There was a problem hiding this comment.
When registrationMode is auto and the plan requires discovery, this preflight path calls loadCallbackDiscoveryState without carrying options.customHeaders, so discovery runs unauthenticated even if the server requires per-request headers (API key, tenant header, etc.). In that case resolveAuthorizationPlan can incorrectly conclude no usable DCR/CIMD path and block OAuth before the normal state machine (which does send custom headers) ever runs. This is a functional regression for header-gated MCP endpoints.
Useful? React with 👍 / 👎.
chelojimenez
temporarily deployed
to
preview-pr-1915
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a44b1d1b8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (requestOrigin !== serverOrigin) { | ||
| return fetchFn(input, init); |
There was a problem hiding this comment.
Automatic OAuth planning now probes discovery through createScopedDiscoveryFetch, but this early-return drops customHeaders whenever the discovery URL is not the MCP server origin. In deployments where the authorization server metadata endpoint is on a different host and requires tenant/API-key headers, resolveOAuthExecutionPlan can fail and block OAuth before the normal state-machine flow starts, even though runtime OAuth requests do carry these headers. Preserve non-sensitive custom headers for cross-origin discovery (or align this behavior with the runtime header policy) so auto planning does not reject otherwise valid configurations.
Useful? React with 👍 / 👎.
chelojimenez
temporarily deployed
to
preview-pr-1915
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
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/lib/oauth/mcp-oauth.ts (1)
2707-2720:⚠️ Potential issue | 🟡 MinorLegacy callback stores a canonicalized
resourceUrlthat diverges from the one used for token exchange.On the non‑
storedSessionbranch,exchangeAuthorizationreceives the rawresourcereturned byselectResourceURL(line 2733), butresolveOAuthResourceUrlre-canonicalises it (lowercased host/scheme, stripped trailing slash) before it is persisted and returned asoauthResourceUrl. If an authorization server is strict about audience equality, a later refresh or downstream consumer replaying the stored value could end up with a subtly differentresourcethan the one bound to the token.Consider persisting the exact string actually sent to the token endpoint, and only canonicalising for display/equality comparisons:
🛠️ Suggested alignment
- const oauthResourceUrl = resolveOAuthResourceUrl({ - serverUrl, - configuredResourceUrl: - typeof resource === "string" - ? resource - : resource instanceof URL - ? resource.toString() - : oauthConfig.resourceUrl, - }); + const exchangeResource = + typeof resource === "string" + ? resource + : resource instanceof URL + ? resource.toString() + : undefined; + const oauthResourceUrl = + exchangeResource ?? + resolveOAuthResourceUrl({ + serverUrl, + configuredResourceUrl: oauthConfig.resourceUrl, + });Also applies to: 2751-2753
🤖 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 2707 - 2720, The persisted/canonicalized resourceUrl diverges from the raw resource used for token exchange; update the logic around selectResourceURL/resolveOAuthResourceUrl so you send and persist the exact string that is actually sent to the token endpoint (use the raw value returned by selectResourceURL for exchangeAuthorization and for storage) and only apply resolveOAuthResourceUrl canonicalization for display or equality checks (e.g., compute oauthResourceUrlCanon = resolveOAuthResourceUrl(...) for comparisons but store oauthResourceUrlRaw = resource (string or resource.toString()) as the authoritative persisted value and pass that raw string into exchangeAuthorization and any storedSession handling).
🧹 Nitpick comments (10)
mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx (1)
321-344: Minor: aliasedexistingServeracross bothserversandworkspaces[].servers.Both
appState.servers["demo-server"]andappState.workspaces.default.servers["demo-server"]are assigned the same object reference. Today this is harmless becauseuseServerStatedoesn't mutate the incoming server entries, but if that invariant ever slips, a stray mutation would silently corrupt both trees and mask the regression. A shallow clone on one side would make the fixture hermetic without changing semantics.🧪 Suggested hardening
appState.servers["demo-server"] = existingServer; - appState.workspaces.default.servers["demo-server"] = existingServer; + appState.workspaces.default.servers["demo-server"] = { ...existingServer };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx` around lines 321 - 344, The test creates an existingServer object and assigns the same object reference into both appState.servers["demo-server"] and appState.workspaces.default.servers["demo-server"], risking shared-mutation bugs; fix by shallow-cloning one side (e.g., set appState.workspaces.default.servers["demo-server"] = { ...existingServer } or clone only the config with { ...existingServer, config: { ...existingServer.config } }) so the two entries are distinct objects while preserving test semantics for createAppState()/useServerState.sdk/tests/oauth-login.probe-timeout.test.ts (1)
139-144: Tie the assertion to the source of truth.The literal
30_000here shadowsDEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MSinsdk/src/oauth-login.ts. If that default is ever tuned, this test will silently keep passing against a stale value (or start failing for a correct change) without pointing at the real contract. Exporting the constant and importing it would make the intent explicit.♻️ Suggested wiring
In
sdk/src/oauth-login.ts:-const DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS = 30_000; +export const DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS = 30_000;In this test:
- const { runOAuthLogin } = await import("../src/oauth-login.js"); + const { runOAuthLogin, DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS } = + await import("../src/oauth-login.js"); @@ expect(mockProbeMcpServer).toHaveBeenCalledWith( expect.objectContaining({ url: serverUrl, - timeoutMs: 30_000, + timeoutMs: DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS, }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tests/oauth-login.probe-timeout.test.ts` around lines 139 - 144, The test is asserting a hardcoded timeout (30_000) instead of using the source-of-truth constant; update the test to import and use DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS from sdk/src/oauth-login.ts and change the expectation on mockProbeMcpServer to expect.timeoutMs: DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS so the spec stays tied to the real default. Ensure the import is added at top of sdk/tests/oauth-login.probe-timeout.test.ts and replace the numeric literal in the expect.objectContaining call referencing mockProbeMcpServer.mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts (4)
2233-2235: Redundant secondloadOAuthTraceFromSessionincompleteHostedOAuthCallback.
previousTraceis already seeded from the session at lines 2233–2235; the re-read at 2278–2279 immediately beforeclearOAuthTraceSessioncannot yield a newer value (nothing in this function writes to the session between the two reads). The second call can be dropped so the intent — "load once, then clear" — reads cleanly.🧹 Tiny simplification
- previousTrace = - loadOAuthTraceFromSession(serverName) ?? previousTrace; - clearOAuthTraceSession(serverName); + clearOAuthTraceSession(serverName);Also applies to: 2278-2280
🤖 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 2233 - 2235, The code re-reads the session into previousTrace inside completeHostedOAuthCallback even though previousTrace was already initialized from loadOAuthTraceFromSession at the top; remove the redundant second call (the later loadOAuthTraceFromSession(...) usage around where clearOAuthTraceSession is called) so the function reads the trace once into previousTrace and then calls clearOAuthTraceSession(serverName) without reloading — update references to use the originally-initialized previousTrace and delete the duplicate loadOAuthTraceFromSession invocation.
1491-1506: Consolidate withnormalizeProxyTargetUrl— identical canonicalizer twins.
canonicalizeOAuthResourceUrl(1491–1506) is byte‑for‑byte the same shape asnormalizeProxyTargetUrl(617–632): lowercase scheme/host, drop hash, strip a single trailing slash. Two names, one behaviour — a DRY nudge.♻️ Suggested consolidation
-function normalizeProxyTargetUrl(url: string): string { - try { - const parsed = new URL(url); - parsed.protocol = parsed.protocol.toLowerCase(); - parsed.hostname = parsed.hostname.toLowerCase(); - parsed.hash = ""; - - if (parsed.pathname !== "/" && parsed.pathname.endsWith("/")) { - parsed.pathname = parsed.pathname.slice(0, -1); - } - - return parsed.toString(); - } catch { - return url; - } -} +function canonicalizeUrl(url: string): string { + try { + const parsed = new URL(url); + parsed.protocol = parsed.protocol.toLowerCase(); + parsed.hostname = parsed.hostname.toLowerCase(); + parsed.hash = ""; + if (parsed.pathname !== "/" && parsed.pathname.endsWith("/")) { + parsed.pathname = parsed.pathname.slice(0, -1); + } + return parsed.toString(); + } catch { + return url; + } +} +const normalizeProxyTargetUrl = canonicalizeUrl; +const canonicalizeOAuthResourceUrl = canonicalizeUrl;…and delete the duplicated body at 1491–1506.
🤖 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 1491 - 1506, The two identical functions canonicalizeOAuthResourceUrl and normalizeProxyTargetUrl should be consolidated: extract the shared normalization logic into a single helper (e.g., normalizeUrl or reuse normalizeProxyTargetUrl) and have the other call it, then remove the duplicate implementation; ensure the behavior (lowercase protocol/hostname, clear hash, strip single trailing slash, and safe-guard with try/catch returning original input) is preserved and that all call sites referencing canonicalizeOAuthResourceUrl are updated to call the consolidated function.
896-908: Dead branch in the trace step lookup.
Array.prototype.findyieldsundefinedwhen nothing matches; with a callback ofindex >= 0, it can never resolve to-1. ThetargetStepIndex === -1half of the guard is unreachable —== nullalone is sufficient.🧹 Minor tidy
- const targetStepIndex = [ + const targetStepIndex = [ "received_authorization_server_metadata", "authorization_request", "received_client_credentials", "request_client_registration", ] .map((stepName) => trace.steps.findIndex((step) => step.step === stepName) ) .find((index) => index >= 0); - if (targetStepIndex == null || targetStepIndex === -1) { + if (targetStepIndex == null) { return trace; }🤖 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 896 - 908, The guard after computing targetStepIndex is overly defensive because Array.prototype.find will return undefined (not -1) when no match is found; update the check that currently reads "if (targetStepIndex == null || targetStepIndex === -1) { return trace; }" to only test for nullish (e.g., "if (targetStepIndex == null) { return trace; }") so the unreachable "-1" branch is removed; locate the targetStepIndex declaration and the subsequent guard in the function handling trace step lookup and simplify the condition accordingly.
564-615: Trim the duplication inresolveOAuthExecutionPlan.The two
resolveAuthorizationPlaninvocations share eight identical arguments and re-runresolveOAuthProtocolMode/resolveOAuthRegistrationModefor each. Hoisting the common payload makes intent clearer and removes the risk of one call drifting from the other on a future edit.♻️ Suggested extraction
async function resolveOAuthExecutionPlan( provider: MCPOAuthProvider, fetchFn: typeof fetch, options: Pick< MCPOAuthOptions, | "serverUrl" | "protocolMode" | "protocolVersion" | "registrationMode" | "registrationStrategy" | "clientId" | "clientSecret" | "useRegistryOAuthProxy" | "customHeaders" >, ): Promise<ResolvedAuthorizationPlan> { - const basePlan = resolveAuthorizationPlan({ - serverUrl: options.serverUrl, - protocolMode: resolveOAuthProtocolMode(options), - protocolVersion: options.protocolVersion, - registrationMode: resolveOAuthRegistrationMode(options), - registrationStrategy: options.registrationStrategy, - clientId: options.clientId, - clientSecret: options.clientSecret, - useRegistryOAuthProxy: options.useRegistryOAuthProxy, - authMode: "interactive", - }); + const planInput = { + serverUrl: options.serverUrl, + protocolMode: resolveOAuthProtocolMode(options), + protocolVersion: options.protocolVersion, + registrationMode: resolveOAuthRegistrationMode(options), + registrationStrategy: options.registrationStrategy, + clientId: options.clientId, + clientSecret: options.clientSecret, + useRegistryOAuthProxy: options.useRegistryOAuthProxy, + authMode: "interactive" as const, + }; + const basePlan = resolveAuthorizationPlan(planInput); if (basePlan.status !== "discovery_required") { return basePlan; } const discoveryState = await loadCallbackDiscoveryState( provider, options.serverUrl, fetchFn, options.customHeaders ); - return resolveAuthorizationPlan({ - serverUrl: options.serverUrl, - protocolMode: resolveOAuthProtocolMode(options), - protocolVersion: options.protocolVersion, - registrationMode: resolveOAuthRegistrationMode(options), - registrationStrategy: options.registrationStrategy, - clientId: options.clientId, - clientSecret: options.clientSecret, - useRegistryOAuthProxy: options.useRegistryOAuthProxy, - authMode: "interactive", + return resolveAuthorizationPlan({ + ...planInput, discovery: toAuthorizationDiscoverySnapshot(discoveryState), }); }🤖 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 564 - 615, In resolveOAuthExecutionPlan, avoid duplicating the arguments to resolveAuthorizationPlan by hoisting the common payload: compute protocolMode once via resolveOAuthProtocolMode(options) and registrationMode once via resolveOAuthRegistrationMode(options), build a baseArgs object containing serverUrl, protocolMode, protocolVersion, registrationMode, registrationStrategy, clientId, clientSecret, useRegistryOAuthProxy and authMode: "interactive", then call resolveAuthorizationPlan(baseArgs) for the initial check and call resolveAuthorizationPlan({...baseArgs, discovery: toAuthorizationDiscoverySnapshot(discoveryState)}) after awaiting loadCallbackDiscoveryState(provider, options.serverUrl, fetchFn, options.customHeaders); ensure you reference resolveOAuthExecutionPlan, resolveAuthorizationPlan, resolveOAuthProtocolMode, resolveOAuthRegistrationMode, loadCallbackDiscoveryState and toAuthorizationDiscoverySnapshot in the change.mcpjam-inspector/client/src/components/connection/AddServerModal.tsx (1)
132-159: OAuth initialization order is load-bearing — worth a one-liner comment.The
initialData.clientIdbranch (Lines 152-156) deliberately runs after theoauthRegistrationModebranch so that a saved client ID always forces"preregistered", even if the stored registration mode disagrees. That precedence is correct, but it's the kind of ordering that future refactors will happily break. A short comment ("// A saved clientId implies preregistered; override any earlier mode assignment.") would keep the intent visible.🤖 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 132 - 159, Add a one-line comment explaining the load-bearing ordering: before the block that checks initialData.clientId (the branch that calls formState.setUseCustomClientId(true), formState.setOauthRegistrationMode("preregistered"), and formState.setClientId(initialData.clientId)), insert a short comment like "// A saved clientId implies preregistered; override any earlier mode assignment." so future refactors don't change the intended precedence over the earlier oauthRegistrationMode handling.mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts (1)
42-50:"auto"is absent from the accepted protocol values.
normalizeOauthRegistrationMode(below) accepts"auto"as a legitimate input, butnormalizeOauthProtocolModesilently coerces"auto"intoDEFAULT_OAUTH_PROTOCOL_MODE. This asymmetry is what forcesAuthenticationSectionto carry its owneffectiveOauthProtocolModefallback — and means any caller persisting"auto"as a protocol mode will lose that intent on the next edit-mode hydration. If protocol "auto" is simply not a supported mode here (because the UI materializes a concrete version before storage), consider either dropping"auto"from theServerFormOAuthProtocolModeunion, or accepting it here for consistency with registration.🤖 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 42 - 50, normalizeOauthProtocolMode currently rejects the "auto" token and coerces it to DEFAULT_OAUTH_PROTOCOL_MODE, causing mismatch with normalizeOauthRegistrationMode and losing persisted "auto" intent; update normalizeOauthProtocolMode to accept "auto" as a valid ServerFormOAuthProtocolMode (i.e., return value === "auto" || value === "2025-03-26" || value === "2025-06-18" || value === "2025-11-25" ? value : DEFAULT_OAUTH_PROTOCOL_MODE) or alternatively remove "auto" from the ServerFormOAuthProtocolMode union so both normalization and consumers like AuthenticationSection and normalizeOauthRegistrationMode remain consistent.mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx (1)
88-102: Defensive "auto" mapping may never fire — consider a type-level guard instead.
oauthProtocolMode === "auto" ? "2025-11-25" : oauthProtocolModeis a safety net, yet the upstream hook (use-server-form) already normalizes any non-explicit value to"2025-11-25"before it reaches this component, so this branch is effectively dead in the current wiring. More importantly, if"auto"ever does arrive (e.g. from initialData), the Select will display"2025-11-25"while the underlying state stays"auto"until the user explicitly chooses an option — a silent divergence between rendered value and stored state. Either drop the mapping once you're confident"auto"cannot reach here, or materialize the normalization back into state (callonOauthProtocolModeChange("2025-11-25")once on mount when the incoming mode is"auto") so the UI and store agree.🤖 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 88 - 102, The component currently maps oauthProtocolMode to effectiveOauthProtocolMode locally which can mask a mismatch between UI and state; instead normalize state on mount when oauthProtocolMode === "auto" by calling onOauthProtocolModeChange("2025-11-25") inside a useEffect in AuthenticationSection.tsx (or remove the local mapping entirely if you guarantee upstream never passes "auto"); locate the oauthProtocolMode handling and either delete effectiveOauthProtocolMode logic and use oauthProtocolMode directly, or add the useEffect that checks oauthProtocolMode === "auto" and invokes onOauthProtocolModeChange("2025-11-25") so the Select value and stored state remain consistent.mcpjam-inspector/client/src/components/connection/__tests__/AdvancedConnectionSettingsSection.test.tsx (1)
26-27: These negative assertions pass vacuously.Neither
"1 header configured"nor"Timeout: 30000ms"is ever rendered byAdvancedConnectionSettingsSectionin any state, so these.not.toBeInTheDocument()checks can never fail and add no coverage. If the intent is to guard against a future summary-chip UX regression, assert the stable collapsed-state contract instead (e.g., that the headers/timeout label rows are absent).✏️ Suggested tightening
- expect(screen.queryByText("1 header configured")).not.toBeInTheDocument(); - expect(screen.queryByText("Timeout: 30000ms")).not.toBeInTheDocument(); + expect(screen.queryByText("Headers")).not.toBeInTheDocument(); + expect(screen.queryByText(/Timeout/)).not.toBeInTheDocument(); + expect( + screen.queryByText("Capabilities override"), + ).not.toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/connection/__tests__/AdvancedConnectionSettingsSection.test.tsx` around lines 26 - 27, The two negative assertions are vacuous because AdvancedConnectionSettingsSection never renders the exact strings "1 header configured" or "Timeout: 30000ms"; update the test to assert the collapsed-state contract instead by checking for absence of the headers and timeout label rows themselves (e.g., query for the "Headers" label/row and the "Timeout" label/row or their test IDs) rather than the specific summary text; locate the expectations in AdvancedConnectionSettingsSection.test (the lines with expect(screen.queryByText("1 header configured")).not.toBeInTheDocument() and expect(screen.queryByText("Timeout: 30000ms")).not.toBeInTheDocument()) and replace them with queries that assert the headers row and timeout label are not in the document.
🤖 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/AddServerModal.tsx`:
- Around line 33-82: Extract the duplicated helper functions
normalizeOauthProtocolMode, normalizeOauthRegistrationMode,
isAuthorizationHeader, getAuthorizationHeaderValue, and createHeaderEntry into a
new shared module (e.g., components/connection/shared/oauth-form-utils.ts),
export them from that module with the same signatures and types, then remove the
copies from AddServerModal.tsx and use-server-form.ts and replace them with
imports from the new module; ensure the new module preserves the crypto-based
UUID fallback and the exact return types (ServerFormData["..."] where used) so
existing callers (createHeaderEntry, getAuthorizationHeaderValue,
normalizeOauthProtocolMode, normalizeOauthRegistrationMode,
isAuthorizationHeader) keep compiling and behaving identically.
In `@sdk/src/oauth-login.ts`:
- Around line 223-244: resolveOAuthLoginAuthorizationPlan currently lets
probeMcpServer rejections bubble up, breaking runOAuthLogin's expectation of
always returning a structured OAuthLoginResult; wrap the probeMcpServer(...)
call in a try/catch, and on any error call/build a blocked plan via
buildBlockedPlanResult (or the same error-path used by resolveAuthorizationPlan)
and return that structured result instead of rethrowing; also hoist the
toAuthorizationPlanInput(input) into a local variable so you reuse it when
calling resolveAuthorizationPlan and when injecting discovery on success.
---
Outside diff comments:
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 2707-2720: The persisted/canonicalized resourceUrl diverges from
the raw resource used for token exchange; update the logic around
selectResourceURL/resolveOAuthResourceUrl so you send and persist the exact
string that is actually sent to the token endpoint (use the raw value returned
by selectResourceURL for exchangeAuthorization and for storage) and only apply
resolveOAuthResourceUrl canonicalization for display or equality checks (e.g.,
compute oauthResourceUrlCanon = resolveOAuthResourceUrl(...) for comparisons but
store oauthResourceUrlRaw = resource (string or resource.toString()) as the
authoritative persisted value and pass that raw string into
exchangeAuthorization and any storedSession handling).
---
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/connection/__tests__/AdvancedConnectionSettingsSection.test.tsx`:
- Around line 26-27: The two negative assertions are vacuous because
AdvancedConnectionSettingsSection never renders the exact strings "1 header
configured" or "Timeout: 30000ms"; update the test to assert the collapsed-state
contract instead by checking for absence of the headers and timeout label rows
themselves (e.g., query for the "Headers" label/row and the "Timeout" label/row
or their test IDs) rather than the specific summary text; locate the
expectations in AdvancedConnectionSettingsSection.test (the lines with
expect(screen.queryByText("1 header configured")).not.toBeInTheDocument() and
expect(screen.queryByText("Timeout: 30000ms")).not.toBeInTheDocument()) and
replace them with queries that assert the headers row and timeout label are not
in the document.
In `@mcpjam-inspector/client/src/components/connection/AddServerModal.tsx`:
- Around line 132-159: Add a one-line comment explaining the load-bearing
ordering: before the block that checks initialData.clientId (the branch that
calls formState.setUseCustomClientId(true),
formState.setOauthRegistrationMode("preregistered"), and
formState.setClientId(initialData.clientId)), insert a short comment like "// A
saved clientId implies preregistered; override any earlier mode assignment." so
future refactors don't change the intended precedence over the earlier
oauthRegistrationMode handling.
In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts`:
- Around line 42-50: normalizeOauthProtocolMode currently rejects the "auto"
token and coerces it to DEFAULT_OAUTH_PROTOCOL_MODE, causing mismatch with
normalizeOauthRegistrationMode and losing persisted "auto" intent; update
normalizeOauthProtocolMode to accept "auto" as a valid
ServerFormOAuthProtocolMode (i.e., return value === "auto" || value ===
"2025-03-26" || value === "2025-06-18" || value === "2025-11-25" ? value :
DEFAULT_OAUTH_PROTOCOL_MODE) or alternatively remove "auto" from the
ServerFormOAuthProtocolMode union so both normalization and consumers like
AuthenticationSection and normalizeOauthRegistrationMode remain consistent.
In
`@mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx`:
- Around line 88-102: The component currently maps oauthProtocolMode to
effectiveOauthProtocolMode locally which can mask a mismatch between UI and
state; instead normalize state on mount when oauthProtocolMode === "auto" by
calling onOauthProtocolModeChange("2025-11-25") inside a useEffect in
AuthenticationSection.tsx (or remove the local mapping entirely if you guarantee
upstream never passes "auto"); locate the oauthProtocolMode handling and either
delete effectiveOauthProtocolMode logic and use oauthProtocolMode directly, or
add the useEffect that checks oauthProtocolMode === "auto" and invokes
onOauthProtocolModeChange("2025-11-25") so the Select value and stored state
remain consistent.
In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx`:
- Around line 321-344: The test creates an existingServer object and assigns the
same object reference into both appState.servers["demo-server"] and
appState.workspaces.default.servers["demo-server"], risking shared-mutation
bugs; fix by shallow-cloning one side (e.g., set
appState.workspaces.default.servers["demo-server"] = { ...existingServer } or
clone only the config with { ...existingServer, config: {
...existingServer.config } }) so the two entries are distinct objects while
preserving test semantics for createAppState()/useServerState.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 2233-2235: The code re-reads the session into previousTrace inside
completeHostedOAuthCallback even though previousTrace was already initialized
from loadOAuthTraceFromSession at the top; remove the redundant second call (the
later loadOAuthTraceFromSession(...) usage around where clearOAuthTraceSession
is called) so the function reads the trace once into previousTrace and then
calls clearOAuthTraceSession(serverName) without reloading — update references
to use the originally-initialized previousTrace and delete the duplicate
loadOAuthTraceFromSession invocation.
- Around line 1491-1506: The two identical functions
canonicalizeOAuthResourceUrl and normalizeProxyTargetUrl should be consolidated:
extract the shared normalization logic into a single helper (e.g., normalizeUrl
or reuse normalizeProxyTargetUrl) and have the other call it, then remove the
duplicate implementation; ensure the behavior (lowercase protocol/hostname,
clear hash, strip single trailing slash, and safe-guard with try/catch returning
original input) is preserved and that all call sites referencing
canonicalizeOAuthResourceUrl are updated to call the consolidated function.
- Around line 896-908: The guard after computing targetStepIndex is overly
defensive because Array.prototype.find will return undefined (not -1) when no
match is found; update the check that currently reads "if (targetStepIndex ==
null || targetStepIndex === -1) { return trace; }" to only test for nullish
(e.g., "if (targetStepIndex == null) { return trace; }") so the unreachable "-1"
branch is removed; locate the targetStepIndex declaration and the subsequent
guard in the function handling trace step lookup and simplify the condition
accordingly.
- Around line 564-615: In resolveOAuthExecutionPlan, avoid duplicating the
arguments to resolveAuthorizationPlan by hoisting the common payload: compute
protocolMode once via resolveOAuthProtocolMode(options) and registrationMode
once via resolveOAuthRegistrationMode(options), build a baseArgs object
containing serverUrl, protocolMode, protocolVersion, registrationMode,
registrationStrategy, clientId, clientSecret, useRegistryOAuthProxy and
authMode: "interactive", then call resolveAuthorizationPlan(baseArgs) for the
initial check and call resolveAuthorizationPlan({...baseArgs, discovery:
toAuthorizationDiscoverySnapshot(discoveryState)}) after awaiting
loadCallbackDiscoveryState(provider, options.serverUrl, fetchFn,
options.customHeaders); ensure you reference resolveOAuthExecutionPlan,
resolveAuthorizationPlan, resolveOAuthProtocolMode,
resolveOAuthRegistrationMode, loadCallbackDiscoveryState and
toAuthorizationDiscoverySnapshot in the change.
In `@sdk/tests/oauth-login.probe-timeout.test.ts`:
- Around line 139-144: The test is asserting a hardcoded timeout (30_000)
instead of using the source-of-truth constant; update the test to import and use
DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS from sdk/src/oauth-login.ts and change the
expectation on mockProbeMcpServer to expect.timeoutMs:
DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS so the spec stays tied to the real default.
Ensure the import is added at top of sdk/tests/oauth-login.probe-timeout.test.ts
and replace the numeric literal in the expect.objectContaining call referencing
mockProbeMcpServer.
🪄 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: 627d8b7b-0def-486a-b220-83241cd077ef
📒 Files selected for processing (16)
mcpjam-inspector/client/src/components/connection/AddServerModal.tsxmcpjam-inspector/client/src/components/connection/__tests__/AdvancedConnectionSettingsSection.test.tsxmcpjam-inspector/client/src/components/connection/hooks/__tests__/use-server-form.test.tsmcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/components/connection/shared/AdvancedConnectionSettingsSection.tsxmcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/lib/client-config.tsmcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/stores/client-config-store.tssdk/src/oauth-login.tssdk/src/oauth/authorization-plan.tssdk/tests/oauth-login.probe-timeout.test.tssdk/tests/oauth/authorization-plan.test.ts
✅ Files skipped from review due to trivial changes (3)
- mcpjam-inspector/client/src/components/connection/shared/AdvancedConnectionSettingsSection.tsx
- sdk/src/oauth/authorization-plan.ts
- mcpjam-inspector/client/src/lib/client-config.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- sdk/tests/oauth/authorization-plan.test.ts
- mcpjam-inspector/client/src/lib/oauth/tests/mcp-oauth.test.ts
- mcpjam-inspector/client/src/components/connection/hooks/tests/use-server-form.test.ts
- mcpjam-inspector/client/src/hooks/use-server-state.ts
| function normalizeOauthProtocolMode( | ||
| value?: ServerFormData["oauthProtocolMode"], | ||
| ): ServerFormData["oauthProtocolMode"] { | ||
| return value === "2025-03-26" || | ||
| value === "2025-06-18" || | ||
| value === "2025-11-25" | ||
| ? value | ||
| : "2025-11-25"; | ||
| } | ||
|
|
||
| function normalizeOauthRegistrationMode( | ||
| value?: ServerFormData["oauthRegistrationMode"], | ||
| ): ServerFormData["oauthRegistrationMode"] | undefined { | ||
| return value === "auto" || | ||
| value === "cimd" || | ||
| value === "dcr" || | ||
| value === "preregistered" | ||
| ? value | ||
| : undefined; | ||
| } | ||
|
|
||
| function isAuthorizationHeader(key: string): boolean { | ||
| return key.trim().toLowerCase() === "authorization"; | ||
| } | ||
|
|
||
| function getAuthorizationHeaderValue( | ||
| headers?: Record<string, string>, | ||
| ): string | undefined { | ||
| if (!headers) { | ||
| return undefined; | ||
| } | ||
|
|
||
| for (const [key, value] of Object.entries(headers)) { | ||
| if (isAuthorizationHeader(key)) { | ||
| return value; | ||
| } | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| function createHeaderEntry(key: string, value: string) { | ||
| return { | ||
| id: | ||
| typeof crypto !== "undefined" && typeof crypto.randomUUID === "function" | ||
| ? crypto.randomUUID() | ||
| : `${Date.now()}-${Math.random().toString(16).slice(2)}`, | ||
| key, | ||
| value, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Five helpers are copy-pasted from use-server-form.ts — extract a shared module.
normalizeOauthProtocolMode, normalizeOauthRegistrationMode, isAuthorizationHeader, getAuthorizationHeaderValue, and createHeaderEntry are duplicated verbatim between this file and mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts. Any future tweak (say, supporting a new protocol version or switching the Authorization-match to trim whitespace) will need to be mirrored in two places — and the previous review round on use-server-form.ts is evidence that this kind of logic drifts. Move them to a shared helpers module (e.g. components/connection/shared/oauth-form-utils.ts or similar) and import from both sites.
♻️ Sketch of the extraction
-function normalizeOauthProtocolMode(
- value?: ServerFormData["oauthProtocolMode"],
-): ServerFormData["oauthProtocolMode"] { /* … */ }
-
-function normalizeOauthRegistrationMode(
- value?: ServerFormData["oauthRegistrationMode"],
-): ServerFormData["oauthRegistrationMode"] | undefined { /* … */ }
-
-function isAuthorizationHeader(key: string): boolean { /* … */ }
-function getAuthorizationHeaderValue(/* … */) { /* … */ }
-function createHeaderEntry(key: string, value: string) { /* … */ }
+import {
+ normalizeOauthProtocolMode,
+ normalizeOauthRegistrationMode,
+ isAuthorizationHeader,
+ getAuthorizationHeaderValue,
+ createHeaderEntry,
+} from "./shared/connection-form-utils";🤖 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 33 - 82, Extract the duplicated helper functions
normalizeOauthProtocolMode, normalizeOauthRegistrationMode,
isAuthorizationHeader, getAuthorizationHeaderValue, and createHeaderEntry into a
new shared module (e.g., components/connection/shared/oauth-form-utils.ts),
export them from that module with the same signatures and types, then remove the
copies from AddServerModal.tsx and use-server-form.ts and replace them with
imports from the new module; ensure the new module preserves the crypto-based
UUID fallback and the exact return types (ServerFormData["..."] where used) so
existing callers (createHeaderEntry, getAuthorizationHeaderValue,
normalizeOauthProtocolMode, normalizeOauthRegistrationMode,
isAuthorizationHeader) keep compiling and behaving identically.
| async function resolveOAuthLoginAuthorizationPlan( | ||
| input: OAuthLoginConfig, | ||
| ): Promise<ResolvedAuthorizationPlan> { | ||
| const basePlan = resolveAuthorizationPlan(toAuthorizationPlanInput(input)); | ||
|
|
||
| if (basePlan.status !== "discovery_required") { | ||
| return basePlan; | ||
| } | ||
|
|
||
| const probe = await probeMcpServer({ | ||
| url: input.serverUrl, | ||
| protocolVersion: basePlan.protocolVersion, | ||
| headers: input.customHeaders, | ||
| timeoutMs: input.stepTimeout ?? DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS, | ||
| fetchFn: input.fetchFn, | ||
| }); | ||
|
|
||
| return resolveAuthorizationPlan({ | ||
| ...toAuthorizationPlanInput(input), | ||
| discovery: probe.oauth, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Probe failures escape runOAuthLogin as thrown errors.
State-machine failures downstream are carefully funneled into OAuthLoginResult.error, but a network hiccup or timeout inside probeMcpServer here will reject straight through the outer try { … } finally, breaking the implicit "always return a structured result" contract that callers of runOAuthLogin likely rely on. Since this probe is now on the critical path for every auto-planned login, wrapping the probe and surfacing the error through buildBlockedPlanResult (or an analogous path) would keep the failure shape consistent.
🛡️ Sketch of a uniform error surface
async function resolveOAuthLoginAuthorizationPlan(
input: OAuthLoginConfig,
-): Promise<ResolvedAuthorizationPlan> {
- const basePlan = resolveAuthorizationPlan(toAuthorizationPlanInput(input));
+): Promise<ResolvedAuthorizationPlan> {
+ const planInput = toAuthorizationPlanInput(input);
+ const basePlan = resolveAuthorizationPlan(planInput);
if (basePlan.status !== "discovery_required") {
return basePlan;
}
- const probe = await probeMcpServer({
- url: input.serverUrl,
- protocolVersion: basePlan.protocolVersion,
- headers: input.customHeaders,
- timeoutMs: input.stepTimeout ?? DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS,
- fetchFn: input.fetchFn,
- });
-
- return resolveAuthorizationPlan({
- ...toAuthorizationPlanInput(input),
- discovery: probe.oauth,
- });
+ try {
+ const probe = await probeMcpServer({
+ url: input.serverUrl,
+ protocolVersion: basePlan.protocolVersion,
+ headers: input.customHeaders,
+ timeoutMs: input.stepTimeout ?? DEFAULT_OAUTH_LOGIN_STEP_TIMEOUT_MS,
+ fetchFn: input.fetchFn,
+ });
+ return resolveAuthorizationPlan({ ...planInput, discovery: probe.oauth });
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error);
+ return {
+ ...basePlan,
+ status: "blocked",
+ blockers: [
+ `Automatic OAuth discovery probe failed: ${message}`,
+ ...basePlan.blockers,
+ ],
+ };
+ }
}The same diff also hoists toAuthorizationPlanInput(input) so the plan input isn't built twice on the discovery-required path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/oauth-login.ts` around lines 223 - 244,
resolveOAuthLoginAuthorizationPlan currently lets probeMcpServer rejections
bubble up, breaking runOAuthLogin's expectation of always returning a structured
OAuthLoginResult; wrap the probeMcpServer(...) call in a try/catch, and on any
error call/build a blocked plan via buildBlockedPlanResult (or the same
error-path used by resolveAuthorizationPlan) and return that structured result
instead of rethrowing; also hoist the toAuthorizationPlanInput(input) into a
local variable so you reuse it when calling resolveAuthorizationPlan and when
injecting discovery on success.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/hooks/__tests__/use-server-state.hosted-oauth.test.tsx (1)
199-210: Consider aligning the default mock shape with the realreadStoredOAuthConfigcontract.The empty
{}default works, but the real implementation always returns at least{ registryServerId, useRegistryOAuthProxy }, and the siblinguse-server-state.test.tsxmocks it that way. Matching that baseline here would keep the two suites honest about the contract and guard against future consumers that start reading those fields unconditionally.♻️ Proposed nudge toward the canonical shape
- readStoredOAuthConfigMock.mockReturnValue({}); + readStoredOAuthConfigMock.mockReturnValue({ + registryServerId: undefined, + useRegistryOAuthProxy: false, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.hosted-oauth.test.tsx` around lines 199 - 210, The test's default mock for readStoredOAuthConfigMock returns an empty object which doesn't match the real contract; update the mock in use-server-state.hosted-oauth.test.tsx so readStoredOAuthConfigMock.mockReturnValue(...) returns at least the canonical shape used in the sibling test (include registryServerId and useRegistryOAuthProxy) to mirror the real implementation and prevent consumers from breaking if they read those fields unconditionally.
🤖 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/hooks/__tests__/use-server-state.hosted-oauth.test.tsx`:
- Around line 199-210: The test's default mock for readStoredOAuthConfigMock
returns an empty object which doesn't match the real contract; update the mock
in use-server-state.hosted-oauth.test.tsx so
readStoredOAuthConfigMock.mockReturnValue(...) returns at least the canonical
shape used in the sibling test (include registryServerId and
useRegistryOAuthProxy) to mirror the real implementation and prevent consumers
from breaking if they read those fields unconditionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65ef75ac-5746-4fb0-ac4d-6ea523935d8c
📒 Files selected for processing (1)
mcpjam-inspector/client/src/hooks/__tests__/use-server-state.hosted-oauth.test.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a17b8797e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| localStorage.removeItem(`mcp-client-${formData.name}`); |
There was a problem hiding this comment.
Removing mcp-client-* whenever the form omits clientId/clientSecret breaks non-preregistered OAuth flows (DCR/CIMD): those flows rely on the stored dynamic client_id for later refresh and callback recovery, but this branch deletes it during ordinary saves/edits. After that, refresh/reconnect paths that read mcp-client-* can fail with missing/invalid client ID errors once access tokens expire, even though the user did not disable OAuth.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a17b879. Configure here.
| registrationStrategy ?? "dcr", | ||
| clientId, | ||
| clientSecret, | ||
| ); |
There was a problem hiding this comment.
Login config hardcodes "dcr" fallback for buildAuthConfig
Medium Severity
buildOAuthLoginConfig passes registrationStrategy ?? "dcr" to buildAuthConfig even though the config's registrationMode is set to "auto" when registrationStrategy is undefined. In client_credentials auth mode, this silently uses DCR placeholders for client ID/secret instead of respecting the "auto" registration mode, creating an inconsistency between the auth object and the declared registrationMode.
Reviewed by Cursor Bugbot for commit a17b879. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8340401199
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| localStorage.setItem(`mcp-serverUrl-${formData.name}`, formData.url); | ||
|
|
There was a problem hiding this comment.
saveOAuthConfigToLocalStorage now writes mcp-serverUrl-*/OAuth config unconditionally, so non-OAuth HTTP saves repopulate OAuth markers that were just cleared (for example in saveServerConfiguration when useOAuth is false). Because hasOAuthConfig treats those keys as OAuth setup, bearer/unauthenticated servers can be misdetected as OAuth on the next edit and accidentally re-saved with OAuth enabled, changing auth behavior unexpectedly.
Useful? React with 👍 / 👎.
| const protocolVersion = | ||
| existingProfile?.protocolVersion ?? | ||
| storedOAuthConfig.protocolVersion ?? | ||
| (storedOAuthConfig.protocolMode && storedOAuthConfig.protocolMode !== "auto" | ||
| ? storedOAuthConfig.protocolMode | ||
| : undefined); | ||
| const registrationStrategy = | ||
| existingProfile?.registrationStrategy ?? | ||
| storedOAuthConfig.registrationStrategy ?? |
There was a problem hiding this comment.
buildResolvedOAuthProfile gives precedence to existingProfile over the freshly stored OAuth config/credentials, so after users change protocol/registration/scopes/credentials and complete OAuth, the persisted oauthFlowProfile can keep stale values from the old profile. This causes reconnect/share/sync paths that read oauthFlowProfile to continue using outdated OAuth settings even though the latest run wrote new values to storage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx (4)
161-174: Banner surfaces only the first blocker/warning, silently dropping the rest.
oauthPlanVisibleBlockerscan carry several entries (for instance, explicit CIMD on a legacy protocol alongside aCIMD_UNSUPPORTED_AUTH_MODE), andplan.warningscan accumulate beyond one item too. Showing[0]leaves users fixing one issue, resubmitting, and then discovering the next. Consider mapping over both arrays so every diagnostic is rendered.✨ Proposed rendering
- {oauthPlanVisibleBlockers.length > 0 && ( - <p className="text-sm text-destructive"> - {oauthPlanVisibleBlockers[0]?.message} - </p> - )} - {oauthPlan.warnings.length > 0 && ( - <p className="text-xs text-amber-700"> - {oauthPlan.warnings[0]} - </p> - )} + {oauthPlanVisibleBlockers.map((blocker) => ( + <p key={blocker.code} className="text-sm text-destructive"> + {blocker.message} + </p> + ))} + {oauthPlan.warnings.map((warning, index) => ( + <p + key={`${index}-${warning}`} + className="text-xs text-amber-700" + > + {warning} + </p> + ))}🤖 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 161 - 174, The banner currently only renders the first item of oauthPlanVisibleBlockers and oauthPlan.warnings (using [0]); update the AuthenticationSection component to iterate (map) over oauthPlanVisibleBlockers and oauthPlan.warnings when rendering the banner so all diagnostics are shown (keep the existing markup/styling for each message and add unique keys for list items), ensuring this logic is used only when oauthPlan and showOauthPlanBanner are truthy.
86-87: Avoid re-declaring the "latest" protocol version here.Hardcoding
"2025-11-25"as the"auto"fallback quietly duplicates theDEFAULT_OAUTH_PROTOCOL_MODEconstant defined inmcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts. When the latest protocol version bumps, two unrelated files must move in lockstep or this component silently pins users to the previous version. Import and reuse the shared constant instead.🤖 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 86 - 87, The component re-declares the "latest" protocol date; replace the hardcoded fallback by importing and using the shared DEFAULT_OAUTH_PROTOCOL_MODE constant from use-server-form.ts: change the logic that computes effectiveOauthProtocolMode (currently using "2025-11-25") to use oauthProtocolMode === "auto" ? DEFAULT_OAUTH_PROTOCOL_MODE : oauthProtocolMode, and add the import for DEFAULT_OAUTH_PROTOCOL_MODE at the top of AuthenticationSection.tsx so the component always uses the single source of truth.
86-98: Memoize the plan resolution to spare the resolver on every keystroke.
resolveAuthorizationPlanis re-invoked on every render — including everyclientId/clientSecretkeypress — even though its inputs are straightforwardly memoizable. Wrapping the call inuseMemokeyed on the dependency tuple keeps behavior identical while avoiding repeat allocations inside the planner (capabilities, blocker arrays, summary string composition).♻️ Proposed memoization
+ const oauthPlan = useMemo( + () => + authType === "oauth" + ? resolveAuthorizationPlan({ + serverUrl, + protocolMode: effectiveOauthProtocolMode, + registrationMode: oauthRegistrationMode, + clientId: showClientCredentials ? clientId : undefined, + clientSecret: showClientCredentials ? clientSecret : undefined, + authMode: "interactive", + }) + : null, + [ + authType, + serverUrl, + effectiveOauthProtocolMode, + oauthRegistrationMode, + showClientCredentials, + clientId, + clientSecret, + ], + ); - const oauthPlan = - authType === "oauth" - ? resolveAuthorizationPlan({ - serverUrl, - protocolMode: effectiveOauthProtocolMode, - registrationMode: oauthRegistrationMode, - clientId: showClientCredentials ? clientId : undefined, - clientSecret: showClientCredentials ? clientSecret : undefined, - authMode: "interactive", - }) - : null;…and hoist
useMemointo the existingreactimport.🤖 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 86 - 98, resolveAuthorizationPlan is being called on every render (including each clientId/clientSecret keystroke), causing unnecessary work; wrap the oauthPlan calculation in React's useMemo so it only recomputes when its true dependencies change. Specifically, import/use useMemo and replace the direct assignment to oauthPlan with a useMemo that depends on authType, serverUrl, oauthProtocolMode (or effectiveOauthProtocolMode), oauthRegistrationMode, showClientCredentials, clientId, and clientSecret; keep the same call to resolveAuthorizationPlan and the same values passed (serverUrl, protocolMode, registrationMode, clientId/clientSecret conditional on showClientCredentials, authMode) so behavior is unchanged but memoized.
42-49: Align Protocol and Registration strategy dropdowns by exposing the"auto"option.
ServerFormOAuthProtocolModeincludes"auto", yetPROTOCOL_OPTIONSlists only the three explicit versions. Meanwhile, the registration strategy dropdown exposes"auto"with the label"Automatic". This creates an inconsistency: users can select automatic registration mode but not automatic protocol selection, despite both being valid type choices.Either add
{ value: "auto", label: "Automatic" }toPROTOCOL_OPTIONSfor parity with the registration dropdown, or document the intentional omission with a comment to clarify why"auto"is excluded.🤖 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 42 - 49, PROTOCOL_OPTIONS is missing the "auto" choice even though ServerFormOAuthProtocolMode includes it and the registration strategy dropdown exposes "auto"; update the PROTOCOL_OPTIONS array to include { value: "auto", label: "Automatic" } so the protocol and registration dropdowns are consistent (or alternatively add a clarifying comment above PROTOCOL_OPTIONS explaining an intentional omission if you prefer not to expose "auto"). Ensure you edit the PROTOCOL_OPTIONS constant in AuthenticationSection.tsx.cli/tests/oauth.test.ts (1)
140-151: Consider fleshing outbuildOAuthLoginConfigcoverage.Only the "all defaults" happy path is exercised. The twin validation rules already tested for
buildOAuthConformanceConfig(CIMD + pre-2025-11-25, CIMD + client_credentials, preregistered missing--client-secret,--print-url+ non-interactive, invalidredirectUrl) live in the same shared builder, so a couple of parity tests here would guard against login-specific regressions as the two builders diverge over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/tests/oauth.test.ts` around lines 140 - 151, Add additional unit tests exercising buildOAuthLoginConfig to mirror the validation rules covered for buildOAuthConformanceConfig: create tests that assert errors for (1) protocolVersion set to a pre-2025-11-25 CIMD value combined with protocol "cimd", (2) protocol "cimd" used with auth.mode "client_credentials", (3) registrationMode "preregistered" without a clientSecret, (4) presence of printUrl flag (or equivalent auth.printUrl) with auth.mode non-interactive, and (5) invalid redirectUrl values; use the same helper inputs and assertion style as the existing test so failures surface from buildOAuthLoginConfig (referencing buildOAuthLoginConfig and its returned config fields like protocolMode, registrationMode, protocolVersion, registrationStrategy, and auth.mode).mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts (2)
2247-2294: SecondloadOAuthTraceFromSessioncall is a no-op.
previousTraceis already populated at line 2247 from the sameserverName, and nothing between that point and line 2292 mutates session storage. The re-read on lines 2292–2293 can only ever reaffirm the same value (or fall back to itself via?? previousTrace), so it is pure redundancy — the one useful side effect in this block isclearOAuthTraceSessionon line 2294.♻️ Proposed fix
- previousTrace = - loadOAuthTraceFromSession(serverName) ?? previousTrace; clearOAuthTraceSession(serverName);🤖 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 2247 - 2294, The second call to loadOAuthTraceFromSession that reassigns previousTrace is redundant and should be removed: keep the initial previousTrace (set at variable declaration), do not re-read session storage via loadOAuthTraceFromSession in this block, and retain the clearOAuthTraceSession(serverName) call; update code to eliminate the repeated assignment to previousTrace (the loadOAuthTraceFromSession(...) ?? previousTrace expression) so previousTrace is not overwritten by an identical value.
564-615: Plan-resolution inputs are assembled twice when discovery is required.Both
resolveAuthorizationPlaninvocations receive an identical set of fields except fordiscovery, and each one independently re-invokesresolveOAuthProtocolMode(options)andresolveOAuthRegistrationMode(options). Hoisting the shared inputs into a single object keeps the two passes demonstrably in lockstep and makes it harder for a future edit to drift one call out of sync with the other.♻️ Proposed fix
async function resolveOAuthExecutionPlan( provider: MCPOAuthProvider, fetchFn: typeof fetch, options: Pick< MCPOAuthOptions, | "serverUrl" | "protocolMode" | "protocolVersion" | "registrationMode" | "registrationStrategy" | "clientId" | "clientSecret" | "useRegistryOAuthProxy" | "customHeaders" >, ): Promise<ResolvedAuthorizationPlan> { - const basePlan = resolveAuthorizationPlan({ - serverUrl: options.serverUrl, - protocolMode: resolveOAuthProtocolMode(options), - protocolVersion: options.protocolVersion, - registrationMode: resolveOAuthRegistrationMode(options), - registrationStrategy: options.registrationStrategy, - clientId: options.clientId, - clientSecret: options.clientSecret, - useRegistryOAuthProxy: options.useRegistryOAuthProxy, - authMode: "interactive", - }); - - if (basePlan.status !== "discovery_required") { - return basePlan; - } - - const discoveryState = await loadCallbackDiscoveryState( - provider, - options.serverUrl, - fetchFn, - options.customHeaders - ); - - return resolveAuthorizationPlan({ - serverUrl: options.serverUrl, - protocolMode: resolveOAuthProtocolMode(options), - protocolVersion: options.protocolVersion, - registrationMode: resolveOAuthRegistrationMode(options), - registrationStrategy: options.registrationStrategy, - clientId: options.clientId, - clientSecret: options.clientSecret, - useRegistryOAuthProxy: options.useRegistryOAuthProxy, - authMode: "interactive", - discovery: toAuthorizationDiscoverySnapshot(discoveryState), - }); + const planInputs = { + serverUrl: options.serverUrl, + protocolMode: resolveOAuthProtocolMode(options), + protocolVersion: options.protocolVersion, + registrationMode: resolveOAuthRegistrationMode(options), + registrationStrategy: options.registrationStrategy, + clientId: options.clientId, + clientSecret: options.clientSecret, + useRegistryOAuthProxy: options.useRegistryOAuthProxy, + authMode: "interactive" as const, + }; + + const basePlan = resolveAuthorizationPlan(planInputs); + if (basePlan.status !== "discovery_required") { + return basePlan; + } + + const discoveryState = await loadCallbackDiscoveryState( + provider, + options.serverUrl, + fetchFn, + options.customHeaders, + ); + + return resolveAuthorizationPlan({ + ...planInputs, + discovery: toAuthorizationDiscoverySnapshot(discoveryState), + }); }🤖 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 564 - 615, In resolveOAuthExecutionPlan, avoid assembling the same plan inputs twice: compute protocolMode and registrationMode once (call resolveOAuthProtocolMode(options) and resolveOAuthRegistrationMode(options) a single time), build a shared inputs object containing serverUrl, protocolMode, protocolVersion, registrationMode, registrationStrategy, clientId, clientSecret, useRegistryOAuthProxy, and authMode, then call resolveAuthorizationPlan(sharedInputs) first and if discovery is required call loadCallbackDiscoveryState(...) and then call resolveAuthorizationPlan({...sharedInputs, discovery: toAuthorizationDiscoverySnapshot(discoveryState)}); reference resolveOAuthExecutionPlan, resolveAuthorizationPlan, resolveOAuthProtocolMode, resolveOAuthRegistrationMode, loadCallbackDiscoveryState, and toAuthorizationDiscoverySnapshot when making the change.sdk/tests/oauth/authorization-plan.test.ts (1)
44-60: Consider one extra assertion to lock in the CIMD URL propagation.Since the CIMD branch of the planner conditionally attaches
clientIdMetadataUrlto the resolved plan (defaulting toDEFAULT_MCPJAM_CLIENT_ID_METADATA_URLwhen none is supplied), a singleexpect(plan.clientIdMetadataUrl).toBeDefined()here would guard against a regression that silently drops that field — a subtle but user-visible break, given downstream flows rely on it. Entirely optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tests/oauth/authorization-plan.test.ts` around lines 44 - 60, The test for resolveAuthorizationPlan misses asserting that the CIMD URL was propagated; add an assertion that plan.clientIdMetadataUrl is present (or equals DEFAULT_MCPJAM_CLIENT_ID_METADATA_URL when you expect the default) after building the plan so the CIMD branch (clientIdMetadataUrl attachment) doesn’t regress silently; update the test case using resolveAuthorizationPlan and the plan.clientIdMetadataUrl symbol to validate the URL is defined/equals the default.
🤖 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/shared/AuthenticationSection.tsx`:
- Around line 221-242: The Select's onValueChange currently force-sets
useCustomClientId on every change; change the handler so it still calls
onOauthRegistrationModeChange(value) but only calls onUseCustomClientIdChange
when the new value is transitioning into "preregistered" (e.g., value ===
"preregistered" && oauthRegistrationMode !== "preregistered") so existing custom
credential state isn't cleared when switching to other modes; reference
oauthRegistrationMode, onOauthRegistrationModeChange, and
onUseCustomClientIdChange to locate and update the callback.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 1538-1556: The function resolveOAuthResourceUrl currently returns
the resource extracted from the authorization URL
(readOAuthResourceFromAuthorizationUrl) before checking the caller-provided
configuredResourceUrl; change the precedence so that if
input.configuredResourceUrl (after trimming) is present it is returned first,
otherwise fall back to the resource from readOAuthResourceFromAuthorizationUrl
and finally canonicalizeOAuthResourceUrl(input.serverUrl). Update the logic in
resolveOAuthResourceUrl accordingly and, if you intentionally keep the
auth-URL-first behavior instead, add a concise comment in
resolveOAuthResourceUrl explaining why authorizationUrl should outrank
configuredResourceUrl to document the decision.
---
Nitpick comments:
In `@cli/tests/oauth.test.ts`:
- Around line 140-151: Add additional unit tests exercising
buildOAuthLoginConfig to mirror the validation rules covered for
buildOAuthConformanceConfig: create tests that assert errors for (1)
protocolVersion set to a pre-2025-11-25 CIMD value combined with protocol
"cimd", (2) protocol "cimd" used with auth.mode "client_credentials", (3)
registrationMode "preregistered" without a clientSecret, (4) presence of
printUrl flag (or equivalent auth.printUrl) with auth.mode non-interactive, and
(5) invalid redirectUrl values; use the same helper inputs and assertion style
as the existing test so failures surface from buildOAuthLoginConfig (referencing
buildOAuthLoginConfig and its returned config fields like protocolMode,
registrationMode, protocolVersion, registrationStrategy, and auth.mode).
In
`@mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx`:
- Around line 161-174: The banner currently only renders the first item of
oauthPlanVisibleBlockers and oauthPlan.warnings (using [0]); update the
AuthenticationSection component to iterate (map) over oauthPlanVisibleBlockers
and oauthPlan.warnings when rendering the banner so all diagnostics are shown
(keep the existing markup/styling for each message and add unique keys for list
items), ensuring this logic is used only when oauthPlan and showOauthPlanBanner
are truthy.
- Around line 86-87: The component re-declares the "latest" protocol date;
replace the hardcoded fallback by importing and using the shared
DEFAULT_OAUTH_PROTOCOL_MODE constant from use-server-form.ts: change the logic
that computes effectiveOauthProtocolMode (currently using "2025-11-25") to use
oauthProtocolMode === "auto" ? DEFAULT_OAUTH_PROTOCOL_MODE : oauthProtocolMode,
and add the import for DEFAULT_OAUTH_PROTOCOL_MODE at the top of
AuthenticationSection.tsx so the component always uses the single source of
truth.
- Around line 86-98: resolveAuthorizationPlan is being called on every render
(including each clientId/clientSecret keystroke), causing unnecessary work; wrap
the oauthPlan calculation in React's useMemo so it only recomputes when its true
dependencies change. Specifically, import/use useMemo and replace the direct
assignment to oauthPlan with a useMemo that depends on authType, serverUrl,
oauthProtocolMode (or effectiveOauthProtocolMode), oauthRegistrationMode,
showClientCredentials, clientId, and clientSecret; keep the same call to
resolveAuthorizationPlan and the same values passed (serverUrl, protocolMode,
registrationMode, clientId/clientSecret conditional on showClientCredentials,
authMode) so behavior is unchanged but memoized.
- Around line 42-49: PROTOCOL_OPTIONS is missing the "auto" choice even though
ServerFormOAuthProtocolMode includes it and the registration strategy dropdown
exposes "auto"; update the PROTOCOL_OPTIONS array to include { value: "auto",
label: "Automatic" } so the protocol and registration dropdowns are consistent
(or alternatively add a clarifying comment above PROTOCOL_OPTIONS explaining an
intentional omission if you prefer not to expose "auto"). Ensure you edit the
PROTOCOL_OPTIONS constant in AuthenticationSection.tsx.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 2247-2294: The second call to loadOAuthTraceFromSession that
reassigns previousTrace is redundant and should be removed: keep the initial
previousTrace (set at variable declaration), do not re-read session storage via
loadOAuthTraceFromSession in this block, and retain the
clearOAuthTraceSession(serverName) call; update code to eliminate the repeated
assignment to previousTrace (the loadOAuthTraceFromSession(...) ?? previousTrace
expression) so previousTrace is not overwritten by an identical value.
- Around line 564-615: In resolveOAuthExecutionPlan, avoid assembling the same
plan inputs twice: compute protocolMode and registrationMode once (call
resolveOAuthProtocolMode(options) and resolveOAuthRegistrationMode(options) a
single time), build a shared inputs object containing serverUrl, protocolMode,
protocolVersion, registrationMode, registrationStrategy, clientId, clientSecret,
useRegistryOAuthProxy, and authMode, then call
resolveAuthorizationPlan(sharedInputs) first and if discovery is required call
loadCallbackDiscoveryState(...) and then call
resolveAuthorizationPlan({...sharedInputs, discovery:
toAuthorizationDiscoverySnapshot(discoveryState)}); reference
resolveOAuthExecutionPlan, resolveAuthorizationPlan, resolveOAuthProtocolMode,
resolveOAuthRegistrationMode, loadCallbackDiscoveryState, and
toAuthorizationDiscoverySnapshot when making the change.
In `@sdk/tests/oauth/authorization-plan.test.ts`:
- Around line 44-60: The test for resolveAuthorizationPlan misses asserting that
the CIMD URL was propagated; add an assertion that plan.clientIdMetadataUrl is
present (or equals DEFAULT_MCPJAM_CLIENT_ID_METADATA_URL when you expect the
default) after building the plan so the CIMD branch (clientIdMetadataUrl
attachment) doesn’t regress silently; update the test case using
resolveAuthorizationPlan and the plan.clientIdMetadataUrl symbol to validate the
URL is defined/equals the default.
🪄 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: b5140c95-9374-487f-9ac0-f544bb682aac
📒 Files selected for processing (6)
cli/tests/oauth.test.tsmcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tssdk/src/oauth/authorization-plan.tssdk/tests/oauth/authorization-plan.test.ts
✅ Files skipped from review due to trivial changes (1)
- sdk/src/oauth/authorization-plan.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/hooks/tests/use-server-state.test.tsx
| <Select | ||
| value={oauthRegistrationMode} | ||
| onValueChange={( | ||
| value: ServerFormOAuthRegistrationMode, | ||
| ) => { | ||
| onOauthRegistrationModeChange(value); | ||
| onUseCustomClientIdChange( | ||
| value === "preregistered", | ||
| ); | ||
| }} | ||
| > | ||
| <SelectTrigger className="w-full h-10"> | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {REGISTRATION_OPTIONS.map((option) => ( | ||
| <SelectItem key={option.value} value={option.value}> | ||
| {option.label} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> |
There was a problem hiding this comment.
Switching registration strategies away from preregistered silently clears useCustomClientId.
onUseCustomClientIdChange(value === "preregistered") force-sets the flag on every change, so a user who turned custom credentials on and then toggles to auto/cimd/dcr loses that state. Since the old "Use custom OAuth credentials" checkbox was removed, this select is now the sole gate — which is a reasonable design — but consider only flipping the flag when transitioning into "preregistered" (or leave it alone otherwise) so advanced users aren't surprised by entries vanishing mid-edit.
🤖 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 221 - 242, The Select's onValueChange currently force-sets
useCustomClientId on every change; change the handler so it still calls
onOauthRegistrationModeChange(value) but only calls onUseCustomClientIdChange
when the new value is transitioning into "preregistered" (e.g., value ===
"preregistered" && oauthRegistrationMode !== "preregistered") so existing custom
credential state isn't cleared when switching to other modes; reference
oauthRegistrationMode, onOauthRegistrationModeChange, and
onUseCustomClientIdChange to locate and update the callback.
| function resolveOAuthResourceUrl(input: { | ||
| serverUrl: string; | ||
| authorizationUrl?: string; | ||
| configuredResourceUrl?: string; | ||
| }): string { | ||
| const requestedFromAuth = readOAuthResourceFromAuthorizationUrl( | ||
| input.authorizationUrl, | ||
| ); | ||
| if (requestedFromAuth) { | ||
| return requestedFromAuth; | ||
| } | ||
|
|
||
| const configured = input.configuredResourceUrl?.trim(); | ||
| if (configured) { | ||
| return configured; | ||
| } | ||
|
|
||
| return canonicalizeOAuthResourceUrl(input.serverUrl); | ||
| } |
There was a problem hiding this comment.
Authorization URL's resource parameter outranks the caller's configured resource.
resolveOAuthResourceUrl prefers a resource query parameter read from the authorization URL over configuredResourceUrl, which inverts the usual "explicit caller intent wins" precedence. In practice the authorization URL's resource is itself derived from configuredResourceUrl/discovery upstream, so the two should normally agree — but if a server-side planner ever rewrites resource (e.g. to a canonical form) while the caller has provided a different, intentional resourceUrl, the caller's value is quietly dropped. Audience-restricted bearer tokens may end up scoped to something the caller didn't request.
Worth sanity-checking the ordering against the audience-validation intent described in the PR summary; if the authorization URL really is authoritative, a one-line comment noting why would be valuable.
🤖 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 1538 - 1556,
The function resolveOAuthResourceUrl currently returns the resource extracted
from the authorization URL (readOAuthResourceFromAuthorizationUrl) before
checking the caller-provided configuredResourceUrl; change the precedence so
that if input.configuredResourceUrl (after trimming) is present it is returned
first, otherwise fall back to the resource from
readOAuthResourceFromAuthorizationUrl and finally
canonicalizeOAuthResourceUrl(input.serverUrl). Update the logic in
resolveOAuthResourceUrl accordingly and, if you intentionally keep the
auth-URL-first behavior instead, add a concise comment in
resolveOAuthResourceUrl explaining why authorizationUrl should outrank
configuredResourceUrl to document the decision.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1b0286cd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| protocolMode: oauthConfig.protocolMode, | ||
| protocolVersion: | ||
| server.oauthFlowProfile?.protocolVersion ?? oauthConfig.protocolVersion, | ||
| registrationMode: oauthConfig.registrationMode, |
There was a problem hiding this comment.
When reconnect falls back to ensureAuthorizedForReconnect, the MCPOAuthOptions are still sourced from persisted OAuth storage and do not include the current workspace connection-default headers, even though other reconnect/initiation paths now merge those defaults. In header-gated deployments (tenant/API-key headers configured at workspace level), refresh or re-auth discovery can fail after reconnect because OAuth requests are sent without the active workspace headers, blocking otherwise valid OAuth reconnects.
Useful? React with 👍 / 👎.
1 participant
Note
Medium Risk
Touches OAuth initiation/callback handling and connection config merging (headers, timeouts, capabilities), which can affect authentication and connectivity across all servers. Risk is mitigated by added validation/normalization and expanded test coverage, but regressions could break reconnect/login flows.
Overview
OAuth CLI login now supports automatic planning by default.
oauth loginno longer requires--protocol-version/--registration, adds an explicitbuildOAuthLoginConfig, and keeps conformance flows strict via required parsers.Inspector connection settings are expanded and standardized. Workspace
clientConfiggainsconnectionDefaults(headers + request timeout), the UI is renamed to Connection Settings, and server add/edit forms share a newAdvancedConnectionSettingsSectionwith header/timeout controls and optional per-server client-capabilities overrides.OAuth connect/reconnect behavior is updated. The inspector now persists/reads
protocolMode+registrationMode, shows planner feedback inAuthenticationSection, merges workspace headers/timeouts into effective configs, and merges OAuth callback configs with existing HTTP config (preserving non-Authorization headers, timeouts, and capability overrides). Tests are updated/added for these new behaviors, including trace rendering for automatic OAuth decisions and hosted callback resource URL replay.Reviewed by Cursor Bugbot for commit f1b0286. Bugbot is set up for automated code reviews on this repo. Configure here.