Add MCP conformance testing UI and API endpoints#1781
Add MCP conformance testing UI and API endpoints#1781chelojimenez wants to merge 8 commits intomainfrom
Conversation
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. |
|
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:
WalkthroughThis pull request introduces comprehensive MCP conformance testing infrastructure across the frontend and backend. On the client side, a new Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/conformance/ConformancePanel.tsx`:
- Around line 328-333: Popup may be blocked causing window.open(...) to return
null and leaving the OAuth flow stuck; after calling window.open(authUrl,
"oauth_conformance_auth", ...), check if popup === null and immediately
transition the ConformancePanel out of the running/waiting state and surface a
deterministic error to the user. Specifically, inside ConformancePanel, after
the popup assignment, add an explicit null check for popup and then call the
component's error/cleanup path (for example set an error state, call any onError
callback, and stop any polling or timers started for the OAuth flow) with a
clear message like "Popup blocked — enable popups to continue" so the suite can
recover rather than hang.
- Around line 335-367: The postMessage/BroadcastChannel handlers (handleMessage
and channel.onmessage) accept any payload with a matching shape and don't
guarantee teardown on all paths; update them to validate the event origin/state
before acting (e.g., check event.origin or event.source and confirm
event.data.state matches the expected sessionId or a stored CSRF state), ensure
both listeners are removed and the BroadcastChannel closed in every code path
(success, failure, or exceptions), and centralize teardown into a single helper
so handleMessage, channel.onmessage, and any error/timeout branches call the
same cleanup before/after invoking handleOAuthCallback(sessionId, code, state);
also add a timeout or one-shot removal to prevent stale listeners.
In `@mcpjam-inspector/client/src/lib/apis/__tests__/mcp-conformance-api.test.ts`:
- Around line 26-28: The test mutates global.fetch by assigning fetchMock and
never restores it; save the original global.fetch into a variable (e.g.,
originalFetch) before assigning fetchMock and restore it in an afterEach or
afterAll hook to avoid cross-test pollution; update the tests that create
fetchMock (the fetchMock assignment and the similar mock at lines ~37-39) to
restore global.fetch = originalFetch after the suite completes and ensure
fetchMock is reset (vi.resetAllMocks() or fetchMock.mockReset()) in the teardown
hook.
In `@mcpjam-inspector/server/routes/mcp/conformance.ts`:
- Around line 216-223: The current assignment of customHeaders replaces
config.requestInit?.headers when oauthProfile.customHeaders exists, losing
per-server headers; change it to start from the existing headers
(config.requestInit?.headers) and merge/override them with
oauthProfile.customHeaders entries so server-level transport/auth headers are
preserved. Locate the customHeaders variable and modify the logic to copy/clone
config.requestInit?.headers into the base (e.g., into an object) and then
iterate oauthProfile.customHeaders (filtering out empty keys) to set or override
those keys on that base object, preserving types (Record<string,string>) and
handling undefined safely.
- Around line 277-297: The fixed 3-second sleep is fragile; replace it with a
real readiness signal that resolves as soon as an auth URL is captured (instead
of waiting a fixed interval). Introduce/await a Promise that is resolved by the
same code that sets capturedAuthUrl (the openUrl handler) and race that against
a short timeout or runnerPromise so the route returns immediately when
capturedAuthUrl becomes available; update the code paths around capturedAuthUrl,
createSession(...) and runnerPromise to await that resolver rather than using
setTimeout. Ensure the resolver is unique and tied to the openUrl/capture code
so the handler reliably returns the "authorization_needed" response as soon as
the URL is captured.
- Around line 232-285: The MCP handler starts an OAuthConformanceTest but
doesn't wire the session's code resolver into the SDK's interactive flow, so
authorization codes never reach the runner; update the oauthConfig passed to new
OAuthConformanceTest to include an interactive session callback (same pattern as
createInteractiveAuthorizationSession used in the web handler) that sets the
created session's codeResolver to the resolver provided by the SDK, and when you
call createSession(capturedAuthUrl) assign that session.id and
session.runnerPromise as before but ensure the SDK's callback uses
setSessionCodeResolver (or directly sets session.codeResolver) so
submitAuthorizationCode invoked by /oauth/authorize resolves the pending promise
in OAuthConformanceTest.
In `@mcpjam-inspector/server/routes/web/conformance.ts`:
- Around line 261-268: The current logic for computing customHeaders replaces
resolved.customHeaders when parsed.oauthProfile.customHeaders is present;
instead merge them so resolved.customHeaders remain and any keys from
parsed.oauthProfile.customHeaders override or add to them. Update the code that
builds customHeaders (the expression using
parsed.oauthProfile?.customHeaders?.reduce and the resolved.customHeaders
fallback) to start from resolved.customHeaders and then apply the
reduce/override of parsed.oauthProfile.customHeaders so both sets are preserved
and oauthProfile values take precedence.
- Around line 358-385: The handler uses a fixed 3s sleep to decide between
returning authorization_needed and awaiting runnerPromise, which can miss slower
servers; instead race the background runnerPromise against a promise that
resolves when capturedAuthUrl (and sessionId) is set (or times out very short)
so you deterministically return the authorization URL if it becomes available
first; modify the logic around capturedAuthUrl/sessionId/runnerPromise and
getSession so you await Promise.race([runnerPromise, authCapturedPromise]) (and
still attach session.runnerPromise when session exists) rather than using
setTimeout.
- Around line 320-332: The Promise created for the OAuth code currently only
captures { code } which drops the returned state; update the Promise type to {
code: string; state?: string } and change the session.codeResolver assignment in
the codePromise executor so it accepts ({ code, state }) and resolves with both
{ code, state } (keep session.codeRejecter and the timeout logic unchanged) so
the SDK's state validation can run; the relevant symbols are codePromise,
session.codeResolver, session.codeRejecter and waitForCode.
In `@mcpjam-inspector/server/services/conformance-oauth-sessions.ts`:
- Around line 68-84: The session ID generation in createSession is predictable
(id = `oauth-conf-${Date.now()}-${nextId++}`) and sessions are not bound to a
caller; replace this with a cryptographically random, unguessable ID (e.g., use
crypto.randomBytes/base64url or crypto.randomUUID) when constructing id in
createSession, persist the result to sessions, and add owner metadata on the
OAuthConformanceSession object (e.g., ownerUserId or ownerToken) so subsequent
endpoints can verify caller ownership against session.owner; update any code
that reads session.id or session shape to expect the new id format and to
validate ownership on follow-up endpoints.
🪄 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: 9213a1b2-26bb-42bb-a771-c850f0868411
📒 Files selected for processing (12)
mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsxmcpjam-inspector/client/src/components/conformance/__tests__/ConformancePanel.test.tsxmcpjam-inspector/client/src/components/connection/ServerInfoContent.tsxmcpjam-inspector/client/src/lib/apis/__tests__/mcp-conformance-api.test.tsmcpjam-inspector/client/src/lib/apis/mcp-conformance-api.tsmcpjam-inspector/server/routes/mcp/__tests__/conformance.test.tsmcpjam-inspector/server/routes/mcp/conformance.tsmcpjam-inspector/server/routes/mcp/index.tsmcpjam-inspector/server/routes/web/conformance.tsmcpjam-inspector/server/routes/web/index.tsmcpjam-inspector/server/services/__tests__/conformance-oauth-sessions.test.tsmcpjam-inspector/server/services/conformance-oauth-sessions.ts
| const popup = window.open( | ||
| authUrl, | ||
| "oauth_conformance_auth", | ||
| "width=600,height=700,scrollbars=yes", | ||
| ); | ||
|
|
There was a problem hiding this comment.
Handle popup-blocked authorization explicitly.
If window.open(...) returns null, the OAuth suite remains in a running/waiting state with no recovery path. Surface a deterministic error immediately.
🛠️ Proposed fix
const popup = window.open(
authUrl,
"oauth_conformance_auth",
"width=600,height=700,scrollbars=yes",
);
+ if (!popup) {
+ setOAuth({
+ status: "error",
+ error: "Authorization popup was blocked. Please allow popups and try again.",
+ });
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx`
around lines 328 - 333, Popup may be blocked causing window.open(...) to return
null and leaving the OAuth flow stuck; after calling window.open(authUrl,
"oauth_conformance_auth", ...), check if popup === null and immediately
transition the ConformancePanel out of the running/waiting state and surface a
deterministic error to the user. Specifically, inside ConformancePanel, after
the popup assignment, add an explicit null check for popup and then call the
component's error/cleanup path (for example set an error state, call any onError
callback, and stop any polling or timers started for the OAuth flow) with a
clear message like "Popup blocked — enable popups to continue" so the suite can
recover rather than hang.
| const fetchMock = vi.fn(); | ||
| global.fetch = fetchMock; | ||
|
|
There was a problem hiding this comment.
Restore global.fetch after this suite to prevent cross-test pollution.
Line 27 mutates global state permanently for this module run; later suites can inherit the mock unintentionally.
🧩 Proposed fix
-import { describe, expect, it, vi, beforeEach } from "vitest";
+import { describe, expect, it, vi, beforeEach, afterAll } from "vitest";
@@
const fetchMock = vi.fn();
-global.fetch = fetchMock;
+const originalFetch = global.fetch;
+global.fetch = fetchMock as typeof fetch;
@@
beforeEach(() => {
fetchMock.mockReset();
});
+
+afterAll(() => {
+ global.fetch = originalFetch;
+});Also applies to: 37-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/apis/__tests__/mcp-conformance-api.test.ts`
around lines 26 - 28, The test mutates global.fetch by assigning fetchMock and
never restores it; save the original global.fetch into a variable (e.g.,
originalFetch) before assigning fetchMock and restore it in an afterEach or
afterAll hook to avoid cross-test pollution; update the tests that create
fetchMock (the fetchMock assignment and the similar mock at lines ~37-39) to
restore global.fetch = originalFetch after the suite completes and ensure
fetchMock is reset (vi.resetAllMocks() or fetchMock.mockReset()) in the teardown
hook.
| const customHeaders = | ||
| oauthProfile?.customHeaders?.reduce( | ||
| (acc, { key, value }) => { | ||
| if (key) acc[key] = value; | ||
| return acc; | ||
| }, | ||
| {} as Record<string, string>, | ||
| ) ?? (config.requestInit?.headers as Record<string, string> | undefined); |
There was a problem hiding this comment.
Merge profile headers with the connected server headers.
When oauthProfile.customHeaders is present, this replaces config.requestInit?.headers outright. Any per-server auth or required transport headers are lost, so the conformance run may no longer target the same server configuration the user connected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/mcp/conformance.ts` around lines 216 - 223,
The current assignment of customHeaders replaces config.requestInit?.headers
when oauthProfile.customHeaders exists, losing per-server headers; change it to
start from the existing headers (config.requestInit?.headers) and merge/override
them with oauthProfile.customHeaders entries so server-level transport/auth
headers are preserved. Locate the customHeaders variable and modify the logic to
copy/clone config.requestInit?.headers into the base (e.g., into an object) and
then iterate oauthProfile.customHeaders (filtering out empty keys) to set or
override those keys on that base object, preserving types
(Record<string,string>) and handling undefined safely.
| const oauthConfig: OAuthConformanceConfig = { | ||
| serverUrl, | ||
| protocolVersion: (oauthProfile?.protocolVersion as any) || "2025-11-25", | ||
| registrationStrategy: | ||
| (oauthProfile?.registrationStrategy as any) || "cimd", | ||
| auth: { | ||
| mode: "interactive", | ||
| openUrl: async (url: string) => { | ||
| // Capture the authorization URL instead of opening the browser | ||
| capturedAuthUrl = url; | ||
| }, | ||
| }, | ||
| client: oauthProfile?.clientId | ||
| ? { | ||
| preregistered: { | ||
| clientId: oauthProfile.clientId, | ||
| clientSecret: oauthProfile.clientSecret, | ||
| }, | ||
| } | ||
| : undefined, | ||
| scopes: oauthProfile?.scopes, | ||
| customHeaders, | ||
| oauthConformanceChecks: runNegativeChecks ?? false, | ||
| }; | ||
|
|
||
| const test = new OAuthConformanceTest(oauthConfig); | ||
|
|
||
| // Start the runner in the background. It will pause at the authorization step | ||
| // where openUrl is called, then wait for the loopback callback. | ||
| const runnerPromise = test.run().then( | ||
| (result) => { | ||
| if (sessionId) setSessionResult(sessionId, result); | ||
| return result; | ||
| }, | ||
| (err) => { | ||
| if (sessionId) { | ||
| setSessionError( | ||
| sessionId, | ||
| err instanceof Error ? err.message : String(err), | ||
| ); | ||
| } | ||
| throw err; | ||
| }, | ||
| ); | ||
|
|
||
| // Wait briefly for the authorization URL to be captured via openUrl | ||
| await new Promise((resolve) => setTimeout(resolve, 3000)); | ||
|
|
||
| if (capturedAuthUrl) { | ||
| // Create a session so the client can track progress | ||
| const session = createSession(capturedAuthUrl); | ||
| sessionId = session.id; | ||
| session.runnerPromise = runnerPromise; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'createInteractiveAuthorizationSession|openUrl' \
sdk/src/oauth-conformance \
mcpjam-inspector/server/routes/mcp/conformance.ts \
mcpjam-inspector/server/routes/web/conformance.ts
rg -n -C3 'codeResolver|submitAuthorizationCode' \
mcpjam-inspector/server/routes/mcp/conformance.ts \
mcpjam-inspector/server/routes/web/conformance.ts \
mcpjam-inspector/server/services/conformance-oauth-sessions.tsRepository: MCPJam/inspector
Length of output: 15466
🏁 Script executed:
#!/bin/bash
# Check the complete POST /oauth/start handler in mcp/conformance.ts
# to see if codeResolver is set anywhere
sed -n '200,320p' mcpjam-inspector/server/routes/mcp/conformance.ts | cat -n
# Also check if there's any mention of codeResolver assignment in the file
echo "=== Searching for codeResolver assignments in mcp/conformance.ts ==="
rg -n 'codeResolver\s*=' mcpjam-inspector/server/routes/mcp/conformance.ts
# Compare with web version
echo "=== Searching for codeResolver in web/conformance.ts ==="
rg -n 'codeResolver\s*=' mcpjam-inspector/server/routes/web/conformance.tsRepository: MCPJam/inspector
Length of output: 4825
Wire the MCP OAuth handler to deliver authorization codes to the test runner.
The MCP /oauth/start handler creates a session but never sets session.codeResolver, so when /oauth/authorize calls submitAuthorizationCode, the code delivery fails silently. The test runner times out waiting for a code that never arrives.
The web handler wires this correctly by passing a createInteractiveAuthorizationSession callback to the OAuthConformanceTest constructor. The MCP handler must use the same pattern to link the session's code resolver to the SDK's interactive authorization flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/mcp/conformance.ts` around lines 232 - 285,
The MCP handler starts an OAuthConformanceTest but doesn't wire the session's
code resolver into the SDK's interactive flow, so authorization codes never
reach the runner; update the oauthConfig passed to new OAuthConformanceTest to
include an interactive session callback (same pattern as
createInteractiveAuthorizationSession used in the web handler) that sets the
created session's codeResolver to the resolver provided by the SDK, and when you
call createSession(capturedAuthUrl) assign that session.id and
session.runnerPromise as before but ensure the SDK's callback uses
setSessionCodeResolver (or directly sets session.codeResolver) so
submitAuthorizationCode invoked by /oauth/authorize resolves the pending promise
in OAuthConformanceTest.
| // Wait briefly for the authorization URL to be captured via openUrl | ||
| await new Promise((resolve) => setTimeout(resolve, 3000)); | ||
|
|
||
| if (capturedAuthUrl) { | ||
| // Create a session so the client can track progress | ||
| const session = createSession(capturedAuthUrl); | ||
| sessionId = session.id; | ||
| session.runnerPromise = runnerPromise; | ||
|
|
||
| return c.json({ | ||
| phase: "authorization_needed" as const, | ||
| sessionId: session.id, | ||
| authorizationUrl: capturedAuthUrl, | ||
| completedSteps: session.completedSteps, | ||
| }); | ||
| } | ||
|
|
||
| // No auth URL captured: test completed without needing authorization | ||
| try { | ||
| const result = await runnerPromise; | ||
| return c.json({ phase: "complete" as const, result }); |
There was a problem hiding this comment.
Replace the fixed 3-second wait with a real readiness signal.
If discovery or dynamic registration takes longer than 3 seconds, capturedAuthUrl is still unset and the route falls through to await runnerPromise. That can block until the OAuth timeout even though user interaction is required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/mcp/conformance.ts` around lines 277 - 297,
The fixed 3-second sleep is fragile; replace it with a real readiness signal
that resolves as soon as an auth URL is captured (instead of waiting a fixed
interval). Introduce/await a Promise that is resolved by the same code that sets
capturedAuthUrl (the openUrl handler) and race that against a short timeout or
runnerPromise so the route returns immediately when capturedAuthUrl becomes
available; update the code paths around capturedAuthUrl, createSession(...) and
runnerPromise to await that resolver rather than using setTimeout. Ensure the
resolver is unique and tied to the openUrl/capture code so the handler reliably
returns the "authorization_needed" response as soon as the URL is captured.
| const customHeaders = | ||
| parsed.oauthProfile?.customHeaders?.reduce( | ||
| (acc, { key, value }) => { | ||
| if (key) acc[key] = value; | ||
| return acc; | ||
| }, | ||
| {} as Record<string, string>, | ||
| ) ?? resolved.customHeaders; |
There was a problem hiding this comment.
Do not discard the resolved hosted headers.
If the caller supplies oauthProfile.customHeaders, this drops resolved.customHeaders entirely. In hosted mode those resolved headers carry the authorized server headers and any injected OAuth bearer token, so an override here can quietly de-authorize the run.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/web/conformance.ts` around lines 261 - 268,
The current logic for computing customHeaders replaces resolved.customHeaders
when parsed.oauthProfile.customHeaders is present; instead merge them so
resolved.customHeaders remain and any keys from
parsed.oauthProfile.customHeaders override or add to them. Update the code that
builds customHeaders (the expression using
parsed.oauthProfile?.customHeaders?.reduce and the resolved.customHeaders
fallback) to start from resolved.customHeaders and then apply the
reduce/override of parsed.oauthProfile.customHeaders so both sets are preserved
and oauthProfile values take precedence.
| const codePromise = new Promise<{ code: string }>( | ||
| (resolve, reject) => { | ||
| session.codeResolver = ({ code }) => resolve({ code }); | ||
| session.codeRejecter = reject; | ||
|
|
||
| setTimeout(() => { | ||
| reject(new Error("OAuth authorization timed out")); | ||
| }, input.timeoutMs || 120_000); | ||
| }, | ||
| ); | ||
| waitForCode = codePromise; | ||
|
|
||
| return codePromise; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'expectedState|state\?: string|authorize:\s*async' \
sdk/src/oauth-conformance \
mcpjam-inspector/server/routes/web/conformance.ts \
mcpjam-inspector/server/services/conformance-oauth-sessions.tsRepository: MCPJam/inspector
Length of output: 11620
🏁 Script executed:
rg -n -A8 'const authorizationResult' sdk/src/oauth-conformance/runner.tsRepository: MCPJam/inspector
Length of output: 513
🏁 Script executed:
rg -n -A15 'const authorizationResult =' sdk/src/oauth-conformance/runner.ts | head -30Repository: MCPJam/inspector
Length of output: 864
🏁 Script executed:
rg -n -A25 'const authorizationResult =' sdk/src/oauth-conformance/runner.ts | head -40Repository: MCPJam/inspector
Length of output: 1245
🏁 Script executed:
rg -n -A20 'activeExpectedState' sdk/src/oauth-conformance/auth-strategies/interactive.ts | head -50Repository: MCPJam/inspector
Length of output: 1681
Preserve state in the authorization code resolver.
The submitAuthorizationCode() function receives both code and state, but the custom session resolver discards state before passing the result to the SDK. This prevents the SDK's state validation logic (which checks for state mismatches at the callback handler) from ever executing.
Update the Promise type to include state?: string and pass it through:
Suggested fix
const codePromise = new Promise<{ code: string; state?: string }>(
(resolve, reject) => {
session.codeResolver = ({ code, state }) => resolve({ code, state });
session.codeRejecter = reject;
setTimeout(() => {
reject(new Error("OAuth authorization timed out"));
}, input.timeoutMs || 120_000);
},
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/web/conformance.ts` around lines 320 - 332,
The Promise created for the OAuth code currently only captures { code } which
drops the returned state; update the Promise type to { code: string; state?:
string } and change the session.codeResolver assignment in the codePromise
executor so it accepts ({ code, state }) and resolves with both { code, state }
(keep session.codeRejecter and the timeout logic unchanged) so the SDK's state
validation can run; the relevant symbols are codePromise, session.codeResolver,
session.codeRejecter and waitForCode.
| // Wait briefly for the authorization URL to be captured | ||
| await new Promise((resolve) => setTimeout(resolve, 3000)); | ||
|
|
||
| if (capturedAuthUrl && sessionId) { | ||
| const session = getSession(sessionId); | ||
| if (session) { | ||
| session.runnerPromise = runnerPromise; | ||
| } | ||
|
|
||
| return { | ||
| phase: "authorization_needed" as const, | ||
| sessionId, | ||
| authorizationUrl: capturedAuthUrl, | ||
| completedSteps: session?.completedSteps ?? [], | ||
| }; | ||
| } | ||
|
|
||
| // No auth URL captured: test may have completed or doesn't need OAuth | ||
| try { | ||
| const result = await runnerPromise; | ||
| return { phase: "complete" as const, result }; | ||
| } catch (error) { | ||
| throw new WebRouteError( | ||
| 500, | ||
| ErrorCode.INTERNAL_ERROR, | ||
| error instanceof Error ? error.message : "OAuth test failed", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid the 3-second authorization race here as well.
This route decides between authorization_needed and complete by sleeping for a constant interval. A slower server can miss that window, causing the handler to await the background runner instead of returning the authorization URL promptly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/web/conformance.ts` around lines 358 - 385,
The handler uses a fixed 3s sleep to decide between returning
authorization_needed and awaiting runnerPromise, which can miss slower servers;
instead race the background runnerPromise against a promise that resolves when
capturedAuthUrl (and sessionId) is set (or times out very short) so you
deterministically return the authorization URL if it becomes available first;
modify the logic around capturedAuthUrl/sessionId/runnerPromise and getSession
so you await Promise.race([runnerPromise, authCapturedPromise]) (and still
attach session.runnerPromise when session exists) rather than using setTimeout.
| let nextId = 1; | ||
|
|
||
| export function createSession( | ||
| authorizationUrl: string, | ||
| expectedState?: string, | ||
| ): OAuthConformanceSession { | ||
| const id = `oauth-conf-${Date.now()}-${nextId++}`; | ||
| const session: OAuthConformanceSession = { | ||
| id, | ||
| authorizationUrl, | ||
| expectedState, | ||
| completedSteps: [], | ||
| createdAt: Date.now(), | ||
| }; | ||
| sessions.set(id, session); | ||
| ensureCleanupTimer(); | ||
| return session; |
There was a problem hiding this comment.
Make OAuth session handles unguessable and owner-bound.
oauth-conf-${Date.now()}-${nextId++} is predictable, and the follow-up OAuth endpoints key off sessionId. On the hosted surface that makes active sessions enumerable enough to poll or tamper with another user’s run if an ID is learned or guessed. Generate a cryptographically random ID here and store caller ownership metadata on the session for later checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/services/conformance-oauth-sessions.ts` around lines
68 - 84, The session ID generation in createSession is predictable (id =
`oauth-conf-${Date.now()}-${nextId++}`) and sessions are not bound to a caller;
replace this with a cryptographically random, unguessable ID (e.g., use
crypto.randomBytes/base64url or crypto.randomUUID) when constructing id in
createSession, persist the result to sessions, and add owner metadata on the
OAuthConformanceSession object (e.g., ownerUserId or ownerToken) so subsequent
endpoints can verify caller ownership against session.owner; update any code
that reads session.id or session shape to expect the new id format and to
validate ownership on follow-up endpoints.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Create a session so the client can track progress | ||
| const session = createSession(capturedAuthUrl); | ||
| sessionId = session.id; | ||
| session.runnerPromise = runnerPromise; |
There was a problem hiding this comment.
Race condition: session created after runner error handler
Medium Severity
In the local mode OAuth start handler, sessionId is assigned only after the 3-second wait (line 283), but the runnerPromise error/success handlers check if (sessionId) before storing results. If test.run() errors after calling openUrl but before the 3s timer completes, sessionId is still undefined, so setSessionResult/setSessionError is silently skipped. The error is lost, and the client polls indefinitely until timeout. The web version avoids this by creating the session inside the authorize callback.
Reviewed by Cursor Bugbot for commit 122943e. Configure here.
| authorizationUrl: string, | ||
| expectedState?: string, | ||
| ): OAuthConformanceSession { | ||
| const id = `oauth-conf-${Date.now()}-${nextId++}`; |
There was a problem hiding this comment.
Predictable session IDs enable session enumeration
Low Severity
Session IDs are generated using Date.now() plus a sequential counter (oauth-conf-${Date.now()}-${nextId++}), making them predictable. In hosted mode, where multiple authenticated users share the same server process, a malicious user could enumerate session IDs and submit a crafted authorization code to another user's OAuth conformance session via /oauth/authorize. Using crypto.randomUUID() would eliminate this risk.
Reviewed by Cursor Bugbot for commit 122943e. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/apis/mcp-conformance-api.ts`:
- Around line 44-54: In localPost<T>, don't call response.json() before checking
response.ok; instead, if !response.ok read the raw body via
response.text().catch(...) to produce a safe error message and throw that
(include response.status in message), and only call await response.json() and
return data as T when response.ok is true; update the logic in localPost to
perform the status check first and use response.text() fallback for error text.
🪄 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: ca63f65e-92a9-43a0-a2da-267c7af406fb
📒 Files selected for processing (4)
mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsxmcpjam-inspector/client/src/components/conformance/__tests__/ConformancePanel.test.tsxmcpjam-inspector/client/src/lib/apis/mcp-conformance-api.tsmcpjam-inspector/server/routes/mcp/conformance.ts
✅ Files skipped from review due to trivial changes (1)
- mcpjam-inspector/server/routes/mcp/conformance.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- mcpjam-inspector/client/src/components/conformance/tests/ConformancePanel.test.tsx
- mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx
| async function localPost<T>(path: string, body: unknown): Promise<T> { | ||
| const response = await authFetch(path, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(body), | ||
| }); | ||
| const data = await response.json(); | ||
| if (!response.ok) { | ||
| throw new Error(data.error || `Request failed (${response.status})`); | ||
| } | ||
| return data as T; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect conformance route responses for non-JSON/empty-body branches"
fd -i 'conformance.ts' | while read -r f; do
echo "== $f =="
rg -n -C3 'router\.post|res\.(json|send|sendStatus|end)|return\s+res\.' "$f"
doneRepository: MCPJam/inspector
Length of output: 166
🏁 Script executed:
find . -type f -name "*.ts" -path "*/server/*" | xargs rg -l "conformance" | head -20Repository: MCPJam/inspector
Length of output: 467
🏁 Script executed:
fd -type f "\.ts$" --path "*/server/*" | xargs rg -l "conform" 2>/dev/null | head -20Repository: MCPJam/inspector
Length of output: 1127
🏁 Script executed:
find . -type f -name "*.ts" | xargs rg -l "router\.post.*conform|/api/.*conform" 2>/dev/null | head -20Repository: MCPJam/inspector
Length of output: 265
🏁 Script executed:
cat -n ./mcpjam-inspector/server/routes/mcp/conformance.tsRepository: MCPJam/inspector
Length of output: 15677
🏁 Script executed:
cat -n ./mcpjam-inspector/server/routes/web/conformance.tsRepository: MCPJam/inspector
Length of output: 17340
🏁 Script executed:
cat -n ./mcpjam-inspector/client/src/lib/apis/mcp-conformance-api.ts | head -80Repository: MCPJam/inspector
Length of output: 3079
Defensive: check HTTP status before parsing response body.
Line 50 calls response.json() before the status check at line 51, which can throw on malformed JSON and mask the actual HTTP error. While the server always returns JSON, reordering improves robustness:
const data = await response.json();
if (!response.ok) {
throw new Error(data.error || `Request failed (${response.status})`);
}
Should become:
if (!response.ok) {
const message = await response.text().catch(() => `Request failed (${response.status})`);
throw new Error(message);
}
const data = await response.json();
return data as T;
This ensures HTTP errors are reported before attempting to parse the body.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/apis/mcp-conformance-api.ts` around lines 44
- 54, In localPost<T>, don't call response.json() before checking response.ok;
instead, if !response.ok read the raw body via response.text().catch(...) to
produce a safe error message and throw that (include response.status in
message), and only call await response.json() and return data as T when
response.ok is true; update the logic in localPost to perform the status check
first and use response.text() fallback for error text.
Adds a conformance panel accessible from the server overview that runs Protocol, Apps, and OAuth conformance checks against connected servers. Both local and hosted Inspector modes are supported. Key changes: - Server routes: local (mcp/conformance.ts) and hosted (web/conformance.ts) with five endpoints: protocol, apps, oauth/start, oauth/authorize, oauth/complete - In-memory OAuth session store with 5-minute TTL and automatic cleanup - Client API layer (mcp-conformance-api.ts) abstracting local vs hosted - ConformancePanel slide-over component with progressive result rendering - Protocol/OAuth suites gated to HTTP transport; Apps runs on any transport - OAuth negative checks behind an advanced toggle (default off) https://claude.ai/code/session_01C3Vhri5s6GLqmzna777oL8
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0064c4e. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
mcpjam-inspector/client/src/components/connection/__tests__/ServerDetailModal.test.tsx (1)
201-207: Negative assertion hinges on an exact label that may never exist.
queryByRole("button", { name: "Run conformance" })is an exact, case-sensitive match — if the real launcher is labeled "Test", "Run Conformance", or anything else, this test passes vacuously and tells us nothing. Given the PR describes a "Test" button inServerInfoContent(later moved behind a feature flag to a sidebar tab), consider anchoring this check to the actual string the component would emit when the flag is on, or broaden the matcher so it can't silently rot.♻️ Suggested tightening
- expect( - screen.queryByRole("button", { name: "Run conformance" }), - ).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: /run conformance|conformance|^test$/i }), + ).not.toBeInTheDocument();Alternatively, assert on a stable
data-testidexposed (or intentionally omitted) by the conformance launcher so the guarantee survives copy changes.🤖 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__/ServerDetailModal.test.tsx` around lines 201 - 207, The test uses an exact case-sensitive label "Run conformance" which can silently break; update the negative assertion in ServerDetailModal.test.tsx to use a robust matcher such as screen.queryByRole("button", { name: /run conformance/i }) or better yet queryByTestId("conformance-launcher") and assert it's not in the document, and if necessary add a stable data-testid="conformance-launcher" to the conformance launcher element in ServerInfoContent so the test reliably detects presence/absence regardless of copy changes.mcpjam-inspector/client/src/components/conformance/__tests__/ConformancePanel.test.tsx (1)
167-463: Solid suite; one gap worth noting.Coverage of empty state, transport gating, expansion, rerun, and stale-run handling is thorough. The one path absent from the suite is the interactive OAuth branch —
authorization_needed→ popup →postMessage→submitOAuthConformanceCode→completeOAuthConformancepolling — which is precisely the flow where the component's statefulness (cleanup refs, run tokens, listener teardown) actually earns its keep. Worth adding a test that drivesmockStartOAuthtoauthorization_needed, dispatches a syntheticOAUTH_CALLBACKmessage, and asserts the completion/cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/conformance/__tests__/ConformancePanel.test.tsx` around lines 167 - 463, Add a test that simulates the interactive OAuth flow: have mockStartOAuth return { phase: "authorization_needed", url, state } (use createDeferred or a promise to control timing), render ConformanceTab, click "Run available checks", then dispatch a synthetic window.postMessage with type "OAUTH_CALLBACK" and the expected payload/state to trigger submitOAuthConformanceCode; finally resolve the polling/complete path (completeOAuthConformance or mockStartOAuth resolution) and assert that the OAuth run reaches the "complete" phase and that listeners/refs are cleaned up (no stray timeouts or event listeners left and UI shows final OAuth summary). Target symbols: ConformanceTab, mockStartOAuth, submitOAuthConformanceCode, completeOAuthConformance, and the "OAUTH_CALLBACK" message type.mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx (2)
394-419: Polling cap is a wall-clock surprise waiting to happen.
MAX_POLLS = 10only behaves sanely because the server long-polls ~25s per call — the budget is implicit and fragile. If the hosted endpoint ever shortens its long-poll window (or returns a non-complete phase immediately), this loop will spin 10 times and surface "OAuth conformance timed out" in milliseconds. A small refactor makes the intent explicit and future-proof:♻️ Proposed refactor
- const MAX_POLLS = 10; - for (let i = 0; i < MAX_POLLS; i++) { + const DEADLINE_MS = 5 * 60_000; // 5 minutes, matches session TTL + const started = Date.now(); + while (Date.now() - started < DEADLINE_MS) { try { const poll = await completeOAuthConformance(sessionId); if (!isRunActive(runToken, serverName)) return; if (poll.phase === "complete" && poll.result) { setOAuth({ status: "done", result: poll.result }); return; } + // Guard against tight loops if the server ever returns quickly. + await new Promise((r) => setTimeout(r, 500)); } catch (err) { ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx` around lines 394 - 419, The pollOAuthComplete loop uses a hard-coded MAX_POLLS which assumes a fixed server long-poll duration; change it to a time-based timeout (e.g., POLL_TIMEOUT_MS) inside pollOAuthComplete so the loop exits based on elapsed wall-clock time rather than iterations, using Date.now() (or performance.now()) to compute startTime and break when elapsed >= POLL_TIMEOUT_MS; keep calling completeOAuthConformance each iteration, preserve the isRunActive(runToken, serverName) checks and the setOAuth calls (status:"done"/"error"), and optionally add a short backoff/sleep between retries to avoid tight spins if the server responds quickly.
122-144: Render optionaldurationMsdefensively.
check.durationMsis rendered unconditionally as{durationMs}ms. If a backend result ever omits it (SDK types may mark it optional, or partial results during streaming), you'll render the literal stringundefinedms. Cheap to harden:- {check.durationMs}ms + {typeof check.durationMs === "number" ? `${check.durationMs}ms` : ""}Same applies to
OAuthStepRowat Line 211.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx` around lines 122 - 144, The code unconditionally renders check.durationMs which can be undefined and will show "undefinedms"; update CheckRow (and OAuthStepRow) to render the duration defensively by checking for a numeric value before showing the unit—for example, replace the literal {check.durationMs}ms with a conditional render like: if typeof check.durationMs === 'number' show `${check.durationMs}ms` (or a placeholder like '—' when absent), ensuring the component only prints the "ms" suffix when a real duration exists.mcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsx (1)
1424-1439: Redirect assertion is correct, but consider tightening the setup.The test asserts the hash lands on
#servers, which is the right user-visible outcome. Two small robustness notes:
beforeEachleavesmcp-oauth-pendingandmcp-serverUrl-asanainlocalStoragefrom the shared setup; this test clears the pending marker/sandbox session but not those keys. Unlikely to affect this assertion, but it's a latent source of flake if routing logic ever consults them.- Consider also asserting
ConformanceTabdid not render (e.g., by mocking it and checking the mock was not invoked), so a future refactor that renders the tab and updates the hash doesn't silently pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsx` around lines 1424 - 1439, The test "redirects conformance to servers when the feature flag is disabled" should tighten setup and assert non-rendering: explicitly clear any leftover localStorage keys used by hosted OAuth (remove 'mcp-oauth-pending' and 'mcp-serverUrl-asana') in addition to calling clearHostedOAuthPendingState() and clearSandboxSession(), and mock the ConformanceTab component (or reuse its existing mock) and add an assertion that ConformanceTab was not invoked (e.g., expect(mockedConformanceTab).not.toHaveBeenCalled()) after render and waitFor to ensure the test fails if the tab is rendered while the hash changes to "#servers".mcpjam-inspector/client/src/components/mcp-sidebar.tsx (1)
244-249: LGTM — flag gating wired up consistently.The new "Conformance" entry follows the established
featureFlagpattern, andconformanceEnabledis threaded through both thefeatureFlagsmemo (line 636) and its dependency array (line 644) without omission.One small aesthetic note worth weighing:
FlaskConicalis already carrying the torch for "Evaluate" on line 223. Two flask icons in the same sidebar can blur the mental map — consider a distinct glyph (e.g.,ShieldCheck,BadgeCheck, orClipboardCheck) for Conformance so the two testing-adjacent features remain visually distinguishable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/mcp-sidebar.tsx` around lines 244 - 249, The Conformance sidebar entry currently reuses the FlaskConical icon (icon: FlaskConical) which duplicates the Evaluate item's glyph; update the Conformance entry to use a distinct icon (for example ShieldCheck, BadgeCheck, or ClipboardCheck) instead of FlaskConical, keeping the title "Conformance" and featureFlag "conformance-enabled" unchanged so the feature-flag wiring remains intact; locate the Conformance object in mcp-sidebar.tsx and replace the icon reference to the chosen unique symbol to improve visual distinction from the Evaluate entry (which also uses FlaskConical).
🤖 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/App.tsx`:
- Around line 1113-1114: The redirect is firing before feature flags hydrate
because conformanceEnabled can be undefined; update the activeTab handling so
the conformance redirect only runs when flags have loaded (e.g., mirror the
ci-evals pattern and check posthog.featureFlags?.hasLoadedFlags or an
evaluateRunsFlagsLoaded-like boolean) in addition to conformanceEnabled !==
true; modify the conditional around applyNavigation("servers", { updateHash:
true }) to guard on both that flagsHaveLoaded and conformanceEnabled === false
so deep-linked "#conformance" waits for flag resolution.
In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx`:
- Around line 500-558: The hosted-mode flow can hang because pollOAuthComplete
is only called for non-hosted mode; add a hosted fallback by starting a hard
timeout and/or a periodic poll alongside the message listeners: when opening the
popup (where window.open and creation of cleanupFns/oauthListenerCleanupRef
occurs), start a timer (e.g., authTimeoutId via setTimeout) that on expiry calls
cleanup(), transitions the UI out of waitingForAuth to an error state, and/or
invokes pollOAuthComplete/session-complete checks (call pollOAuthComplete or
completeOAuthConformance) every few seconds via setInterval until success;
ensure the timer/interval IDs are pushed into cleanupFns and cleared in
cleanup(), and keep existing handleMessage and BroadcastChannel handlers to
short-circuit the timeout on success (they should call cleanup() and
handleOAuthCallback as now).
- Around line 167-179: The map render in ConformancePanel uses the warning
string as React keys which will collide for duplicate messages; update the list
item keys in the warnings.map and teachableMoments.map calls to use a stable
composite key (for example `${warning}-${i}`) or the index `i` from
`.map((warning, i) => ...)` instead of `key={warning}` so duplicate strings
won't break reconciliation; locate the JSX that currently has
`warnings.map((warning) => (<li key={warning}>...)` and the similar
`teachableMoments.map` usage and replace the key prop accordingly.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsx`:
- Around line 1424-1439: The test "redirects conformance to servers when the
feature flag is disabled" should tighten setup and assert non-rendering:
explicitly clear any leftover localStorage keys used by hosted OAuth (remove
'mcp-oauth-pending' and 'mcp-serverUrl-asana') in addition to calling
clearHostedOAuthPendingState() and clearSandboxSession(), and mock the
ConformanceTab component (or reuse its existing mock) and add an assertion that
ConformanceTab was not invoked (e.g.,
expect(mockedConformanceTab).not.toHaveBeenCalled()) after render and waitFor to
ensure the test fails if the tab is rendered while the hash changes to
"#servers".
In
`@mcpjam-inspector/client/src/components/conformance/__tests__/ConformancePanel.test.tsx`:
- Around line 167-463: Add a test that simulates the interactive OAuth flow:
have mockStartOAuth return { phase: "authorization_needed", url, state } (use
createDeferred or a promise to control timing), render ConformanceTab, click
"Run available checks", then dispatch a synthetic window.postMessage with type
"OAUTH_CALLBACK" and the expected payload/state to trigger
submitOAuthConformanceCode; finally resolve the polling/complete path
(completeOAuthConformance or mockStartOAuth resolution) and assert that the
OAuth run reaches the "complete" phase and that listeners/refs are cleaned up
(no stray timeouts or event listeners left and UI shows final OAuth summary).
Target symbols: ConformanceTab, mockStartOAuth, submitOAuthConformanceCode,
completeOAuthConformance, and the "OAUTH_CALLBACK" message type.
In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx`:
- Around line 394-419: The pollOAuthComplete loop uses a hard-coded MAX_POLLS
which assumes a fixed server long-poll duration; change it to a time-based
timeout (e.g., POLL_TIMEOUT_MS) inside pollOAuthComplete so the loop exits based
on elapsed wall-clock time rather than iterations, using Date.now() (or
performance.now()) to compute startTime and break when elapsed >=
POLL_TIMEOUT_MS; keep calling completeOAuthConformance each iteration, preserve
the isRunActive(runToken, serverName) checks and the setOAuth calls
(status:"done"/"error"), and optionally add a short backoff/sleep between
retries to avoid tight spins if the server responds quickly.
- Around line 122-144: The code unconditionally renders check.durationMs which
can be undefined and will show "undefinedms"; update CheckRow (and OAuthStepRow)
to render the duration defensively by checking for a numeric value before
showing the unit—for example, replace the literal {check.durationMs}ms with a
conditional render like: if typeof check.durationMs === 'number' show
`${check.durationMs}ms` (or a placeholder like '—' when absent), ensuring the
component only prints the "ms" suffix when a real duration exists.
In
`@mcpjam-inspector/client/src/components/connection/__tests__/ServerDetailModal.test.tsx`:
- Around line 201-207: The test uses an exact case-sensitive label "Run
conformance" which can silently break; update the negative assertion in
ServerDetailModal.test.tsx to use a robust matcher such as
screen.queryByRole("button", { name: /run conformance/i }) or better yet
queryByTestId("conformance-launcher") and assert it's not in the document, and
if necessary add a stable data-testid="conformance-launcher" to the conformance
launcher element in ServerInfoContent so the test reliably detects
presence/absence regardless of copy changes.
In `@mcpjam-inspector/client/src/components/mcp-sidebar.tsx`:
- Around line 244-249: The Conformance sidebar entry currently reuses the
FlaskConical icon (icon: FlaskConical) which duplicates the Evaluate item's
glyph; update the Conformance entry to use a distinct icon (for example
ShieldCheck, BadgeCheck, or ClipboardCheck) instead of FlaskConical, keeping the
title "Conformance" and featureFlag "conformance-enabled" unchanged so the
feature-flag wiring remains intact; locate the Conformance object in
mcp-sidebar.tsx and replace the icon reference to the chosen unique symbol to
improve visual distinction from the Evaluate entry (which also uses
FlaskConical).
🪄 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: 2191bb59-7548-4402-a040-53549741652e
📒 Files selected for processing (10)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsxmcpjam-inspector/client/src/components/__tests__/mcp-sidebar-feature-flags.test.tsmcpjam-inspector/client/src/components/conformance/ConformancePanel.tsxmcpjam-inspector/client/src/components/conformance/__tests__/ConformancePanel.test.tsxmcpjam-inspector/client/src/components/connection/__tests__/ServerDetailModal.test.tsxmcpjam-inspector/client/src/components/mcp-sidebar.tsxmcpjam-inspector/client/src/lib/__tests__/hosted-navigation.test.tsmcpjam-inspector/client/src/lib/__tests__/hosted-tab-policy.test.tsmcpjam-inspector/client/src/lib/hosted-tab-policy.ts
| } else if (activeTab === "conformance" && conformanceEnabled !== true) { | ||
| applyNavigation("servers", { updateHash: true }); |
There was a problem hiding this comment.
Flag-loading race redirects deep-linked #conformance visitors.
useFeatureFlagEnabled returns undefined until PostHog hydrates its flags, so conformanceEnabled !== true is true on first render and this effect bounces the user to #servers before the flag resolves. The pattern matches the neighbouring registry/learning/client-config checks, so this isn't a regression — but note that ci-evals above gates on evaluateRunsFlagsLoaded to avoid exactly this. If deep-linking to Conformance needs to survive a cold flag load, guard on posthog.featureFlags?.hasLoadedFlags here too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/App.tsx` around lines 1113 - 1114, The redirect
is firing before feature flags hydrate because conformanceEnabled can be
undefined; update the activeTab handling so the conformance redirect only runs
when flags have loaded (e.g., mirror the ci-evals pattern and check
posthog.featureFlags?.hasLoadedFlags or an evaluateRunsFlagsLoaded-like boolean)
in addition to conformanceEnabled !== true; modify the conditional around
applyNavigation("servers", { updateHash: true }) to guard on both that
flagsHaveLoaded and conformanceEnabled === false so deep-linked "#conformance"
waits for flag resolution.
| {warnings && warnings.length > 0 && ( | ||
| <div className="rounded-sm bg-muted/20 px-2 py-1.5 text-xs text-muted-foreground"> | ||
| <div className="mb-1 flex items-center gap-1 font-medium text-foreground/70"> | ||
| <AlertTriangle className="h-3 w-3" /> | ||
| Warnings | ||
| </div> | ||
| <ul className="space-y-1 pl-4 list-disc"> | ||
| {warnings.map((warning) => ( | ||
| <li key={warning}>{warning}</li> | ||
| ))} | ||
| </ul> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
key={warning} will collide on duplicate strings.
Using the warning text itself as a React key breaks if the backend ever emits two identical warnings (a plausible case for repeated misconfigurations). Prefer the index, or a composite key:
- {warnings.map((warning) => (
- <li key={warning}>{warning}</li>
+ {warnings.map((warning, i) => (
+ <li key={`${i}-${warning}`}>{warning}</li>
))}The same pattern repeats for teachableMoments at Line 226.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {warnings && warnings.length > 0 && ( | |
| <div className="rounded-sm bg-muted/20 px-2 py-1.5 text-xs text-muted-foreground"> | |
| <div className="mb-1 flex items-center gap-1 font-medium text-foreground/70"> | |
| <AlertTriangle className="h-3 w-3" /> | |
| Warnings | |
| </div> | |
| <ul className="space-y-1 pl-4 list-disc"> | |
| {warnings.map((warning) => ( | |
| <li key={warning}>{warning}</li> | |
| ))} | |
| </ul> | |
| </div> | |
| )} | |
| {warnings && warnings.length > 0 && ( | |
| <div className="rounded-sm bg-muted/20 px-2 py-1.5 text-xs text-muted-foreground"> | |
| <div className="mb-1 flex items-center gap-1 font-medium text-foreground/70"> | |
| <AlertTriangle className="h-3 w-3" /> | |
| Warnings | |
| </div> | |
| <ul className="space-y-1 pl-4 list-disc"> | |
| {warnings.map((warning, i) => ( | |
| <li key={`${i}-${warning}`}>{warning}</li> | |
| ))} | |
| </ul> | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx`
around lines 167 - 179, The map render in ConformancePanel uses the warning
string as React keys which will collide for duplicate messages; update the list
item keys in the warnings.map and teachableMoments.map calls to use a stable
composite key (for example `${warning}-${i}`) or the index `i` from
`.map((warning, i) => ...)` instead of `key={warning}` so duplicate strings
won't break reconciliation; locate the JSX that currently has
`warnings.map((warning) => (<li key={warning}>...)` and the similar
`teachableMoments.map` usage and replace the key prop accordingly.
chelojimenez
force-pushed
the
claude/add-conformance-panel-0KqFD
branch
from
0064c4e to
9ebdd3a
Compare
chelojimenez
temporarily deployed
to
preview-pr-1781
GitHub Actions
Inactive
Internal previewPreview URL: https://mcp-inspector-pr-1781.up.railway.app |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
mcpjam-inspector/server/routes/web/conformance.ts (3)
320-332:⚠️ Potential issue | 🟠 Major
stateis dropped on its way back to the SDK.
submitAuthorizationCodefaithfully carries{ code, state }intosession.codeResolver, but this resolver destructures onlycodeand resolves{ code }. The SDK's state-validation path in the interactive flow will never see the returnedstate, silently neutralizing a conformance check.🛠️ Suggested adjustment
- const codePromise = new Promise<{ code: string }>( + const codePromise = new Promise<{ code: string; state?: string }>( (resolve, reject) => { - session.codeResolver = ({ code }) => resolve({ code }); + session.codeResolver = ({ code, state }) => + resolve({ code, state }); session.codeRejecter = reject; setTimeout(() => { reject(new Error("OAuth authorization timed out")); }, input.timeoutMs || 120_000); }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/web/conformance.ts` around lines 320 - 332, The session.codeResolver currently destructures only { code } and resolves { code }, dropping the authorization state; update the resolver used by submitAuthorizationCode so it accepts and forwards both code and state (resolve({ code, state })) so waitForCode and the SDK receive the state for validation; locate the promise setup where session.codeResolver and session.codeRejecter are assigned (the codePromise/waitForCode block) and change the resolver signature/destructure to include state and resolve the full object, leaving the timeout/reject logic intact.
261-268:⚠️ Potential issue | 🟠 MajorHosted headers are discarded when
oauthProfile.customHeadersis supplied.With
??, the presence of any profile-levelcustomHeaderswipes outresolved.customHeaders. In hosted mode those resolved headers carry the authorized server headers plus any injected OAuth bearer — so an override here can quietly de-authorize the run.Base the result on
resolved.customHeadersand let profile entries override per key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/web/conformance.ts` around lines 261 - 268, customHeaders currently uses the nullish coalescing (??) which discards resolved.customHeaders whenever parsed.oauthProfile?.customHeaders is present; change the logic so resolved.customHeaders is the base and profile entries override per key: start with a copy of resolved.customHeaders, then iterate parsed.oauthProfile?.customHeaders (if present) and set/override entries by key (skip falsy keys) before assigning customHeaders; reference parsed.oauthProfile?.customHeaders, resolved.customHeaders and the customHeaders variable to locate and update this merge behavior.
358-385:⚠️ Potential issue | 🟠 MajorSame 3-second authorization race as the local handler.
Deciding between
authorization_neededand awaitingrunnerPromisevia a fixedsetTimeout(3000)is fragile: a slower server (discovery, DCR) can miss that window, leaving the route to block onrunnerPromiseinstead of promptly returning the authorization URL.Race
runnerPromiseagainst a deferred that resolves the momentcapturedAuthUrl/sessionIdare set so the handler always returns whichever outcome arrives first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/web/conformance.ts` around lines 358 - 385, The handler currently waits a fixed 3s then decides between returning authorization_needed or awaiting runnerPromise, which races poorly; replace the setTimeout with a promise that resolves immediately when capturedAuthUrl/sessionId are set and then Promise.race that "authCapturedPromise" against runnerPromise. Specifically, create an authCapturedPromise that resolves with {capturedAuthUrl, sessionId} as soon as those values are assigned (or rejects/times out if desired), use Promise.race([authCapturedPromise, runnerPromise]) to determine which finished first, and if the auth promise wins set session.runnerPromise via getSession(sessionId) and return the authorization_needed object (with completedSteps), otherwise handle the runnerPromise result or errors and throw WebRouteError on failure.mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx (3)
554-558:⚠️ Potential issue | 🟠 MajorHosted mode can wait on
postMessage/BroadcastChannelforever.
pollOAuthCompleteonly runs when!isHostedMode(), so in hosted mode the UI advances strictly viaOAUTH_CALLBACKmessages. If the popup is closed early, the callback origin differs, or an extension blocks the channel, the component remains inwaitingForAuthwith no exit. Consider either (a) a hard timeout that errors out after N minutes, or (b) a low-frequencycompleteOAuthConformancepoll running alongside the message listeners so hosted users have a recovery path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx` around lines 554 - 558, The component currently only calls pollOAuthComplete when !isHostedMode(), leaving hosted sessions stuck in waitingForAuth if postMessage/BroadcastChannel never arrives; update the logic around isHostedMode(), pollOAuthComplete, OAUTH_CALLBACK listeners, and cleanup so hosted mode gets a recovery path: implement a hard timeout (N minutes) that forces an error/cleanup from waitingForAuth, and/or start a low-frequency fallback poll using completeOAuthConformance that runs alongside the message listeners (and is cancelled by cleanup) so popup-closed/misorigin/blocked-channel cases unblock; ensure the timeout and fallback poll are clearly tied to the waitingForAuth state and cleaned up in cleanup().
500-504:⚠️ Potential issue | 🟠 MajorBlocked popup leaves the OAuth suite stuck in
waitingForAuth.
window.openreturnsnullwhen blocked by the browser; the suite would sit inrunning + waitingForAuthindefinitely, with no message, no fallback, and no recovery path. Surface a deterministic error immediately.🛠️ Suggested handling
- window.open( + const popup = window.open( startResult.authorizationUrl, "oauth_conformance_auth", "width=600,height=700,scrollbars=yes", ); + if (!popup) { + setOAuth({ + status: "error", + error: + "Authorization popup was blocked. Allow popups for this site and try again.", + }); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx` around lines 500 - 504, The call to window.open in ConformancePanel (using startResult.authorizationUrl) can return null if the browser blocks the popup, leaving the suite stuck in the waitingForAuth state; update the logic after the window.open(...) call to detect a null return (or an immediately closed window) and transition out of waitingForAuth by setting an explicit error state or throwing a deterministic error, include a clear message referencing the blocked popup and the URL (startResult.authorizationUrl) so the UI can show recovery options; ensure this check lives near the function handling OAuth start (the code that calls window.open) and triggers the existing error/display path rather than letting the suite remain in running+waitingForAuth.
174-175:⚠️ Potential issue | 🟡 Minor
key={warning}/key={moment}collides on duplicate strings.React keys must be unique among siblings. The backend can plausibly emit two identical warnings or teachable moments (repeated misconfigurations, reused templates), at which point reconciliation breaks. The same pattern is repeated at Line 227 for
teachableMoments.🩹 Suggested fix
- {warnings.map((warning) => ( - <li key={warning}>{warning}</li> + {warnings.map((warning, i) => ( + <li key={`${i}-${warning}`}>{warning}</li> ))} ... - {step.teachableMoments.map((moment) => ( - <li key={moment}>{moment}</li> + {step.teachableMoments.map((moment, i) => ( + <li key={`${i}-${moment}`}>{moment}</li> ))}Also applies to: 226-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx` around lines 174 - 175, The list item keys for warnings and teachableMoments are using the message string (key={warning} and key={moment}) which can collide when identical strings are emitted; update the map callbacks (warnings.map and teachableMoments.map) to use a stable unique key by including the index or another unique identifier (e.g., key={`${warning}-${i}`} and key={`tm-${i}`}) instead of the raw string so sibling keys are unique during reconciliation.mcpjam-inspector/server/routes/mcp/conformance.ts (3)
282-297:⚠️ Potential issue | 🟠 MajorFixed 3-second sleep is a fragile signal for "authorization needed".
If discovery or dynamic registration takes longer than 3 seconds,
capturedAuthUrlis still undefined when the sleep returns and the handler falls intoawait runnerPromise— which will then block until the OAuth timeout since no caller has asessionIdto submit the code against.Prefer a deferred that resolves when
openUrlcaptures the URL, raced againstrunnerPromise(and a conservative timeout) so the route deterministically returns as soon as either outcome is known.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/mcp/conformance.ts` around lines 282 - 297, The current 3-second sleep before checking capturedAuthUrl is fragile; replace it by creating a deferred Promise that openUrl resolves when it captures the authorization URL (e.g., capturedAuthUrlDeferred), then use Promise.race between that deferred, runnerPromise and a conservative timeout to deterministically decide the outcome; if the deferred resolves with a URL, call createSession(capturedUrl), set session.runnerPromise and sessionId, and return the authorization_needed JSON (same fields as before), otherwise continue waiting on runnerPromise or return a timeout/failure as appropriate. Ensure you update places where openUrl sets capturedAuthUrl to instead resolve the deferred so the route reacts immediately when the URL is captured.
237-280:⚠️ Potential issue | 🔴 CriticalSDK has no channel to deliver the authorization code back to the runner.
OAuthConformanceTestis constructed with onlyopenUrlinauth, and nocreateInteractiveAuthorizationSessionoverride is passed via dependencies. The session stored viacreateSessionnever has itscodeResolverassigned, sosubmitAuthorizationCodein/oauth/authorizereturnsfalseand the background runner hangs until the SDK's internal timeout. The hosted handler inserver/routes/web/conformance.tsalready wires this viacreateInteractiveAuthorizationSession; this local handler must do the same.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/mcp/conformance.ts` around lines 237 - 280, The test runner never receives the authorization code because OAuthConformanceTest is only given auth.openUrl and no createInteractiveAuthorizationSession, so the created session never gets a codeResolver and submitAuthorizationCode returns false; fix by supplying a createInteractiveAuthorizationSession implementation to the OAuthConformanceTest configuration (the same way the hosted handler in server/routes/web/conformance.ts does) that creates a session (createSession), attaches a codeResolver to the stored session, and returns the interactive session descriptor expected by the SDK; ensure this createInteractiveAuthorizationSession is provided on the oauthConfig/dependencies passed to new OAuthConformanceTest so the /oauth/authorize path can resolve the code and the runnerPromise can proceed (also keep sessionId, setSessionResult and setSessionError logic intact).
221-228:⚠️ Potential issue | 🟠 MajorPer-server transport headers are overwritten when
oauthProfile.customHeadersis present.The nullish-coalesce at Line 228 means any user-supplied
customHeadersreplacesconfig.requestInit?.headerswholesale. Headers the user connected with (auth, transport requirements) are silently dropped, so the conformance run can target the server with a different header set than the live connection.Merge instead of replace — start from
config.requestInit?.headersand letoauthProfile.customHeadersoverride per key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/mcp/conformance.ts` around lines 221 - 228, The current assignment to customHeaders uses nullish coalescing so oauthProfile.customHeaders replaces config.requestInit?.headers entirely; instead, build customHeaders starting from the base transport headers (config.requestInit?.headers) and then merge/override per-key from oauthProfile.customHeaders so existing headers are preserved unless a custom header with the same key overrides it; locate the customHeaders declaration and change the logic (use a base object from config.requestInit?.headers and apply the reduce over oauthProfile.customHeaders to set/override keys) to ensure auth and transport headers are not dropped.
🧹 Nitpick comments (2)
mcpjam-inspector/server/routes/web/conformance.ts (1)
124-128: Tautological ternary —oauthToken ? undefined : undefined.Both branches return
undefined, which makes the conditional decorative. The comment clarifies intent, but the expression reads like a bug. Since the token is injected into headers above, just returnundefineddirectly.♻️ Suggested cleanup
return { serverUrl: auth.serverConfig.url, - accessToken: oauthToken ? undefined : undefined, // OAuth token goes in headers + // OAuth token, when present, is injected into `headers` above; no separate accessToken. + accessToken: undefined, customHeaders: Object.keys(headers).length > 0 ? headers : undefined, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/web/conformance.ts` around lines 124 - 128, The returned config contains a tautological conditional for accessToken (oauthToken ? undefined : undefined); replace that expression with a direct undefined value (accessToken: undefined) and remove the redundant ternary/commentary so the return object (using auth.serverConfig.url, accessToken, customHeaders/headers) clearly reflects that OAuth tokens are injected via headers (oauthToken) instead of accessToken.mcpjam-inspector/client/src/components/mcp-sidebar.tsx (1)
248-253: Icon collision with "Evaluate".
FlaskConicalis already used for the Evaluate entry (Line 227). Two adjacent nav items sharing the same glyph can mildly confuse visual scanning — consider a different lucide icon for Conformance (e.g.,ShieldCheck,BadgeCheck,ClipboardCheck) to keep each sidebar entry distinguishable at a glance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/mcp-sidebar.tsx` around lines 248 - 253, The Conformance sidebar item is using the same icon component FlaskConical as the Evaluate entry (used for the Evaluate nav item), so change the Conformance icon to a distinct lucide icon (for example ShieldCheck, BadgeCheck, or ClipboardCheck) by replacing the icon property on the Conformance object (title: "Conformance") with the chosen component; ensure the new icon is imported/available where the sidebar components are declared so the Conformance entry uses the different glyph.
🤖 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/conformance/ConformancePanel.tsx`:
- Around line 428-436: The handler currently ignores
submitOAuthConformanceCode's returned { success: boolean } and proceeds to poll;
change it to await the result object from submitOAuthConformanceCode and branch
on result.success: if true continue as before (check isRunActive and setOAuth
waitingForAuth:false, status:"running"), but if false immediately set the OAuth
error state (e.g., setOAuth(prev => ({...prev, waitingForAuth:false,
status:"error", error: "<server reported failure>"})) and return so polling does
not start; reference submitOAuthConformanceCode, isRunActive, runToken,
serverName and setOAuth when implementing this branch.
In `@mcpjam-inspector/server/routes/mcp/conformance.ts`:
- Around line 40-47: The error returned from getHttpConfig is misleading when
reused by the OAuth flow; update getHttpConfig (and any callers such as the
/oauth/start handler) to return a neutral, correct error description and code
when the serverConfig isn't HTTP — e.g., set code "unsupportedTransport" with
message "Operation requires HTTP transport" or add an optional operationName
parameter to getHttpConfig to produce "OAuth conformance requires HTTP
transport" for oauth/start; adjust the getHttpConfig return path and its callers
(including the OAuth route) accordingly so the message accurately reflects the
operation requiring HTTP and avoid hard-coding "Protocol conformance requires
HTTP transport."
---
Duplicate comments:
In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx`:
- Around line 554-558: The component currently only calls pollOAuthComplete when
!isHostedMode(), leaving hosted sessions stuck in waitingForAuth if
postMessage/BroadcastChannel never arrives; update the logic around
isHostedMode(), pollOAuthComplete, OAUTH_CALLBACK listeners, and cleanup so
hosted mode gets a recovery path: implement a hard timeout (N minutes) that
forces an error/cleanup from waitingForAuth, and/or start a low-frequency
fallback poll using completeOAuthConformance that runs alongside the message
listeners (and is cancelled by cleanup) so
popup-closed/misorigin/blocked-channel cases unblock; ensure the timeout and
fallback poll are clearly tied to the waitingForAuth state and cleaned up in
cleanup().
- Around line 500-504: The call to window.open in ConformancePanel (using
startResult.authorizationUrl) can return null if the browser blocks the popup,
leaving the suite stuck in the waitingForAuth state; update the logic after the
window.open(...) call to detect a null return (or an immediately closed window)
and transition out of waitingForAuth by setting an explicit error state or
throwing a deterministic error, include a clear message referencing the blocked
popup and the URL (startResult.authorizationUrl) so the UI can show recovery
options; ensure this check lives near the function handling OAuth start (the
code that calls window.open) and triggers the existing error/display path rather
than letting the suite remain in running+waitingForAuth.
- Around line 174-175: The list item keys for warnings and teachableMoments are
using the message string (key={warning} and key={moment}) which can collide when
identical strings are emitted; update the map callbacks (warnings.map and
teachableMoments.map) to use a stable unique key by including the index or
another unique identifier (e.g., key={`${warning}-${i}`} and key={`tm-${i}`})
instead of the raw string so sibling keys are unique during reconciliation.
In `@mcpjam-inspector/server/routes/mcp/conformance.ts`:
- Around line 282-297: The current 3-second sleep before checking
capturedAuthUrl is fragile; replace it by creating a deferred Promise that
openUrl resolves when it captures the authorization URL (e.g.,
capturedAuthUrlDeferred), then use Promise.race between that deferred,
runnerPromise and a conservative timeout to deterministically decide the
outcome; if the deferred resolves with a URL, call createSession(capturedUrl),
set session.runnerPromise and sessionId, and return the authorization_needed
JSON (same fields as before), otherwise continue waiting on runnerPromise or
return a timeout/failure as appropriate. Ensure you update places where openUrl
sets capturedAuthUrl to instead resolve the deferred so the route reacts
immediately when the URL is captured.
- Around line 237-280: The test runner never receives the authorization code
because OAuthConformanceTest is only given auth.openUrl and no
createInteractiveAuthorizationSession, so the created session never gets a
codeResolver and submitAuthorizationCode returns false; fix by supplying a
createInteractiveAuthorizationSession implementation to the OAuthConformanceTest
configuration (the same way the hosted handler in
server/routes/web/conformance.ts does) that creates a session (createSession),
attaches a codeResolver to the stored session, and returns the interactive
session descriptor expected by the SDK; ensure this
createInteractiveAuthorizationSession is provided on the
oauthConfig/dependencies passed to new OAuthConformanceTest so the
/oauth/authorize path can resolve the code and the runnerPromise can proceed
(also keep sessionId, setSessionResult and setSessionError logic intact).
- Around line 221-228: The current assignment to customHeaders uses nullish
coalescing so oauthProfile.customHeaders replaces config.requestInit?.headers
entirely; instead, build customHeaders starting from the base transport headers
(config.requestInit?.headers) and then merge/override per-key from
oauthProfile.customHeaders so existing headers are preserved unless a custom
header with the same key overrides it; locate the customHeaders declaration and
change the logic (use a base object from config.requestInit?.headers and apply
the reduce over oauthProfile.customHeaders to set/override keys) to ensure auth
and transport headers are not dropped.
In `@mcpjam-inspector/server/routes/web/conformance.ts`:
- Around line 320-332: The session.codeResolver currently destructures only {
code } and resolves { code }, dropping the authorization state; update the
resolver used by submitAuthorizationCode so it accepts and forwards both code
and state (resolve({ code, state })) so waitForCode and the SDK receive the
state for validation; locate the promise setup where session.codeResolver and
session.codeRejecter are assigned (the codePromise/waitForCode block) and change
the resolver signature/destructure to include state and resolve the full object,
leaving the timeout/reject logic intact.
- Around line 261-268: customHeaders currently uses the nullish coalescing (??)
which discards resolved.customHeaders whenever
parsed.oauthProfile?.customHeaders is present; change the logic so
resolved.customHeaders is the base and profile entries override per key: start
with a copy of resolved.customHeaders, then iterate
parsed.oauthProfile?.customHeaders (if present) and set/override entries by key
(skip falsy keys) before assigning customHeaders; reference
parsed.oauthProfile?.customHeaders, resolved.customHeaders and the customHeaders
variable to locate and update this merge behavior.
- Around line 358-385: The handler currently waits a fixed 3s then decides
between returning authorization_needed or awaiting runnerPromise, which races
poorly; replace the setTimeout with a promise that resolves immediately when
capturedAuthUrl/sessionId are set and then Promise.race that
"authCapturedPromise" against runnerPromise. Specifically, create an
authCapturedPromise that resolves with {capturedAuthUrl, sessionId} as soon as
those values are assigned (or rejects/times out if desired), use
Promise.race([authCapturedPromise, runnerPromise]) to determine which finished
first, and if the auth promise wins set session.runnerPromise via
getSession(sessionId) and return the authorization_needed object (with
completedSteps), otherwise handle the runnerPromise result or errors and throw
WebRouteError on failure.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/mcp-sidebar.tsx`:
- Around line 248-253: The Conformance sidebar item is using the same icon
component FlaskConical as the Evaluate entry (used for the Evaluate nav item),
so change the Conformance icon to a distinct lucide icon (for example
ShieldCheck, BadgeCheck, or ClipboardCheck) by replacing the icon property on
the Conformance object (title: "Conformance") with the chosen component; ensure
the new icon is imported/available where the sidebar components are declared so
the Conformance entry uses the different glyph.
In `@mcpjam-inspector/server/routes/web/conformance.ts`:
- Around line 124-128: The returned config contains a tautological conditional
for accessToken (oauthToken ? undefined : undefined); replace that expression
with a direct undefined value (accessToken: undefined) and remove the redundant
ternary/commentary so the return object (using auth.serverConfig.url,
accessToken, customHeaders/headers) clearly reflects that OAuth tokens are
injected via headers (oauthToken) instead of accessToken.
🪄 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: 41b726f0-01bd-45df-b796-77b503086aa3
📒 Files selected for processing (19)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsxmcpjam-inspector/client/src/components/__tests__/mcp-sidebar-feature-flags.test.tsmcpjam-inspector/client/src/components/conformance/ConformancePanel.tsxmcpjam-inspector/client/src/components/conformance/__tests__/ConformancePanel.test.tsxmcpjam-inspector/client/src/components/connection/__tests__/ServerDetailModal.test.tsxmcpjam-inspector/client/src/components/mcp-sidebar.tsxmcpjam-inspector/client/src/lib/__tests__/hosted-navigation.test.tsmcpjam-inspector/client/src/lib/__tests__/hosted-tab-policy.test.tsmcpjam-inspector/client/src/lib/apis/__tests__/mcp-conformance-api.test.tsmcpjam-inspector/client/src/lib/apis/mcp-conformance-api.tsmcpjam-inspector/client/src/lib/hosted-tab-policy.tsmcpjam-inspector/server/routes/mcp/__tests__/conformance.test.tsmcpjam-inspector/server/routes/mcp/conformance.tsmcpjam-inspector/server/routes/mcp/index.tsmcpjam-inspector/server/routes/web/conformance.tsmcpjam-inspector/server/routes/web/index.tsmcpjam-inspector/server/services/__tests__/conformance-oauth-sessions.test.tsmcpjam-inspector/server/services/conformance-oauth-sessions.ts
✅ Files skipped from review due to trivial changes (8)
- mcpjam-inspector/client/src/lib/tests/hosted-tab-policy.test.ts
- mcpjam-inspector/client/src/components/connection/tests/ServerDetailModal.test.tsx
- mcpjam-inspector/client/src/components/tests/mcp-sidebar-feature-flags.test.ts
- mcpjam-inspector/client/src/lib/tests/hosted-navigation.test.ts
- mcpjam-inspector/server/routes/mcp/index.ts
- mcpjam-inspector/client/src/tests/App.hosted-oauth.test.tsx
- mcpjam-inspector/server/services/tests/conformance-oauth-sessions.test.ts
- mcpjam-inspector/server/routes/mcp/tests/conformance.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- mcpjam-inspector/server/routes/web/index.ts
- mcpjam-inspector/client/src/lib/hosted-tab-policy.ts
- mcpjam-inspector/client/src/App.tsx
- mcpjam-inspector/client/src/lib/apis/tests/mcp-conformance-api.test.ts
- mcpjam-inspector/client/src/lib/apis/mcp-conformance-api.ts
- mcpjam-inspector/server/services/conformance-oauth-sessions.ts
| ) => { | ||
| try { | ||
| await submitOAuthConformanceCode({ sessionId, code, state }); | ||
| if (!isRunActive(runToken, serverName)) return; | ||
| setOAuth((prev) => ({ | ||
| ...prev, | ||
| waitingForAuth: false, | ||
| status: "running", | ||
| })); |
There was a problem hiding this comment.
Ignoring submitOAuthConformanceCode's return value.
The API returns { success: boolean }, but the handler treats any resolved promise as success and immediately polls for completion. If the server reports success: false (e.g., session expired between popup open and code submission), the UI polls until the 10-iteration timeout before surfacing anything. Branch on the result and route the failure through the error state.
🛠️ Suggested handling
- await submitOAuthConformanceCode({ sessionId, code, state });
+ const submission = await submitOAuthConformanceCode({
+ sessionId,
+ code,
+ state,
+ });
if (!isRunActive(runToken, serverName)) return;
+ if (!submission?.success) {
+ setOAuth({
+ status: "error",
+ error:
+ "Failed to deliver authorization code — session may have expired.",
+ });
+ return;
+ }
setOAuth((prev) => ({
...prev,
waitingForAuth: false,
status: "running",
}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/conformance/ConformancePanel.tsx`
around lines 428 - 436, The handler currently ignores
submitOAuthConformanceCode's returned { success: boolean } and proceeds to poll;
change it to await the result object from submitOAuthConformanceCode and branch
on result.success: if true continue as before (check isRunActive and setOAuth
waitingForAuth:false, status:"running"), but if false immediately set the OAuth
error state (e.g., setOAuth(prev => ({...prev, waitingForAuth:false,
status:"error", error: "<server reported failure>"})) and return so polling does
not start; reference submitOAuthConformanceCode, isRunActive, runToken,
serverName and setOAuth when implementing this branch.
| if (!("url" in serverConfig) || !serverConfig.url) { | ||
| return { | ||
| error: "Protocol conformance requires HTTP transport", | ||
| code: "unsupportedTransport", | ||
| }; | ||
| } | ||
| return { config: serverConfig as HttpServerConfig }; | ||
| } |
There was a problem hiding this comment.
Misleading error message for OAuth transport check.
getHttpConfig hard-codes "Protocol conformance requires HTTP transport", but it's reused by /oauth/start at Line 209. A user triggering OAuth conformance against a stdio server would receive the wrong-suite error.
💬 Suggested tweak
function getHttpConfig(
mcpClientManager: any,
serverId: string,
+ transportErrorMessage = "HTTP transport required",
): { config: HttpServerConfig } | { error: string; code: string } {
const serverConfig = mcpClientManager.getServerConfig(serverId) as
| MCPServerConfig
| undefined;
if (!serverConfig) {
return { error: "Server not connected", code: "notConnected" };
}
if (!("url" in serverConfig) || !serverConfig.url) {
return {
- error: "Protocol conformance requires HTTP transport",
+ error: transportErrorMessage,
code: "unsupportedTransport",
};
}
return { config: serverConfig as HttpServerConfig };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/mcp/conformance.ts` around lines 40 - 47, The
error returned from getHttpConfig is misleading when reused by the OAuth flow;
update getHttpConfig (and any callers such as the /oauth/start handler) to
return a neutral, correct error description and code when the serverConfig isn't
HTTP — e.g., set code "unsupportedTransport" with message "Operation requires
HTTP transport" or add an optional operationName parameter to getHttpConfig to
produce "OAuth conformance requires HTTP transport" for oauth/start; adjust the
getHttpConfig return path and its callers (including the OAuth route)
accordingly so the message accurately reflects the operation requiring HTTP and
avoid hard-coding "Protocol conformance requires HTTP transport."
3 participants
Summary
This PR adds comprehensive conformance testing capabilities to the MCP Inspector, allowing users to validate MCP servers against protocol, apps, and OAuth specifications. The implementation includes a new UI panel, backend API endpoints for both local and hosted modes, and session management for OAuth flows.
Key Changes
Client-Side
ConformancePanel component (
client/src/components/conformance/ConformancePanel.tsx): New 570-line React component providing a tabbed interface for running three conformance test suites:Conformance API client (
client/src/lib/apis/mcp-conformance-api.ts): Abstraction layer supporting both local and hosted modes with functions for:ServerInfoContent integration: Added "Test" button with Flask icon to launch ConformancePanel from server details
Comprehensive test suite for both the panel component and API client
Server-Side (Local Mode)
Conformance routes (
server/routes/mcp/conformance.ts): Three POST endpoints:/protocol: Runs MCPConformanceTest against HTTP servers/apps: Runs MCPAppsConformanceTest against any server type/oauth/start: Initiates OAuth conformance with interactive mode, captures authorization URL, and manages session lifecycle/oauth/submit: Accepts authorization code from browser callback/oauth/complete: Polls for OAuth test completionOAuth session service (
server/services/conformance-oauth-sessions.ts): In-memory session store with:Test coverage for route validation and error handling
Server-Side (Hosted Mode)
server/routes/web/conformance.ts): Parallel endpoints supporting both guest and workspace-authenticated requests:Notable Implementation Details
runByModeutilityhttps://claude.ai/code/session_01C3Vhri5s6GLqmzna777oL8
Note
Medium Risk
Adds new backend endpoints and an interactive OAuth flow with in-memory session state and long-polling, which can impact auth-adjacent behavior and server resource usage if buggy, though changes are largely additive and feature-flag gated.
Overview
Adds a new feature-flagged
Conformancetab in the Inspector UI that can run Protocol, Apps, and OAuth conformance suites against the currently selected server, including expandable per-check/step details and an option to run negative OAuth checks.Introduces a client API layer (
mcp-conformance-api) plus new server endpoints for both local (/api/mcp/conformance/*) and hosted (/api/web/conformance/*) modes, including session-based handling for interactive OAuth runs (start/authorize/complete) backed by an in-memory TTL session store. Updates hosted navigation/tab policy and adds tests covering UI behavior, feature-flag gating/redirects, API calls, and route/session validation.Reviewed by Cursor Bugbot for commit fe0915d. Bugbot is set up for automated code reviews on this repo. Configure here.