Collapse hosted direct guest routes#2024
Conversation
dosubot
Bot
added
the
size:XL
chelojimenez
temporarily deployed
to
preview-pr-2024
GitHub Actions
Inactive
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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: fbfa42c493
ℹ️ 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 (HOSTED_MODE) { | ||
| return; |
There was a problem hiding this comment.
When an unauthenticated shared-server link requires OAuth, useHostedOAuthGate treats shared links as non-vault-backed and SharedServerChatPage reads the access token from getStoredTokens(serverName). With this early return, the OAuth callback's provider.saveTokens(tokens) succeeds without writing mcp-tokens-*, so the shared link remains in resuming/error with a missing token even after consent. This should only skip local token storage for hosted surfaces that are actually vault-backed, not shared-link guest OAuth.
Useful? React with 👍 / 👎.
| hosted: () => { | ||
| if (isGuestMode()) { | ||
| throw new Error(GUEST_UNSUPPORTED_MESSAGE); | ||
| } | ||
|
|
||
| return postEvalRequest(EVALS_API_ENDPOINTS.hosted.run, { | ||
| ...mergeHostedServerBatch(request), | ||
| storageServerIds: request.storageServerIds ?? request.serverIds, |
There was a problem hiding this comment.
For hosted guest actors this now posts full suite runs to /api/web/evals/run, but that route still calls withEphemeralConnection with guestUnsupportedMessage and rejects any request where c.get("guestId") is set. A Convex-backed guest can therefore enter the eval playground and start a run only to get the server's “Not available for guests yet” 403; either the server gate needs to be removed for these project-backed guests or this client path should keep blocking them before sending the request.
Useful? React with 👍 / 👎.
Internal previewPreview URL: https://mcp-inspector-pr-2024.up.railway.app |
|
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:
WalkthroughDirect-guest bypasses and per-server guest request paths were removed in favor of a single hosted/Convex authorization flow. The HostedApiContext shape was simplified (removed guest OAuth/config fields; now exposes oauthTokensByServerId, shareToken, chatboxToken, isAuthenticated, hasSession). HOSTED_MODE now disables localStorage-backed OAuth persistence and prevents direct-guest classification. Local default project IDs are generated dynamically. Server routes, client hooks, APIs, and tests were updated to require and validate a hosted projectId and to use unified batch/project-oriented payloads. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcpjam-inspector/client/src/hooks/use-server-state.ts (1)
1686-1702:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t continue hosted OAuth after the pre-sync failed to produce a server id.
In hosted mode,
prepareHostedProjectOAuthRedirect()is a no-op withouthostedServerId. That means the callback cannot create the server-side OAuth session, and the flow falls back to the local callback path even though hosted token persistence was removed inmcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts. The initial connect can appear to work, but reload/reconnect will not have a durable hosted credential to reuse.Possible fix
if (HOSTED_MODE) { try { const serverId = await syncServerToConvex( formData.name, serverEntryForSave, clientSecretSyncOptions ); if (serverId) { hostedServerId = serverId; injectHostedServerMapping(formData.name, serverId); } } catch (err) { logger.warn("Sync to Convex failed (pre-connection)", { serverName: formData.name, err, }); } + if (formData.useOAuth && !hostedServerId) { + const errorMessage = + "Could not save the hosted server before starting OAuth. Please try again."; + dispatch({ + type: "CONNECT_FAILURE", + name: formData.name, + error: errorMessage, + }); + toast.error(errorMessage); + return; + } } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 1686 - 1702, If HOSTED_MODE is true but syncServerToConvex fails or returns no serverId, stop the hosted OAuth flow instead of continuing: after calling syncServerToConvex in use-server-state (references: HOSTED_MODE, syncServerToConvex, hostedServerId, injectHostedServerMapping), if an error is thrown or serverId is falsy then do not set hostedServerId and immediately abort/return from the hosted-path logic so prepareHostedProjectOAuthRedirect (in mcp-oauth.ts) is never invoked; surface/log the failure and fall back to the non-hosted/local callback path or surface an explicit error to the caller. Ensure the catch branch treats missing serverId the same as an exception and prevents further hosted-specific steps.mcpjam-inspector/client/src/hooks/use-chat-session.ts (1)
1299-1305:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when hosted context is unresolved instead of building a partial hosted payload.
Line 1300 currently emits a hosted body without
projectId. Since direct guest mode is intentionally removed, this creates an invalid request path ifsendMessageis invoked before UI gating fully prevents submission (race/programmatic calls). Prefer throwing early here to hard-enforce the invariant.Proposed fix
const buildHostedBody = () => { if (!hostedProjectId) { - return { - chatSessionId, - directVisibility, - }; + throw new Error( + "Hosted chat context is not ready: missing projectId." + ); } const isHostedDirectChat = !hostedShareToken && !hostedChatboxToken; return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/hooks/use-chat-session.ts` around lines 1299 - 1305, The buildHostedBody function currently returns a partial hosted payload when hostedProjectId is missing; instead, enforce the invariant by throwing an error immediately if hostedProjectId is falsy so callers (e.g., sendMessage) cannot send invalid requests. Update buildHostedBody to check hostedProjectId at the top and throw a descriptive exception (mentioning hostedProjectId, chatSessionId) rather than returning { chatSessionId, directVisibility }, ensuring any callers relying on buildHostedBody fail fast when the hosted context is unresolved.
🧹 Nitpick comments (2)
mcpjam-inspector/server/routes/web/__tests__/servers-doctor.test.ts (1)
20-52: 💤 Low valueStale guest scaffolding lingers in the test file.
Now that
runGuestDoctoris gone, theoauth-proxymock (lines 20–32) and theguestId/mcpClientManagermiddleware increateGuestDoctorApp(lines 43–52) no longer feed any code path the route exercises — the new assertion only relies onprojectServerSchemarejecting the guest-shaped body. Trimming them keeps the test's intent crisp and avoids future readers wondering which guest branch the mocks support.♻️ Proposed cleanup
-vi.mock("../../../utils/oauth-proxy.js", () => ({ - OAuthProxyError: class OAuthProxyError extends Error { - constructor( - public readonly status: number, - message: string, - ) { - super(message); - } - }, - validateUrl: vi.fn().mockResolvedValue({ - url: new URL("https://guest.example.com/mcp"), - }), -})); - vi.mock("../../apps/SandboxProxyHtml.bundled.js", () => ({ CHATGPT_APPS_SANDBOX_PROXY_HTML: "<html></html>", MCP_APPS_SANDBOX_PROXY_HTML: "<html></html>", })); @@ -function createGuestDoctorApp(): Hono { - const app = new Hono(); - app.use("*", async (c, next) => { - (c as any).mcpClientManager = {}; - c.set("guestId", "guest-1"); - await next(); - }); - app.route("/api/web/servers", serversRoutes); - return app; -} +function createGuestDoctorApp(): Hono { + const app = new Hono(); + app.route("/api/web/servers", serversRoutes); + return app; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/routes/web/__tests__/servers-doctor.test.ts` around lines 20 - 52, Remove the stale guest-related scaffolding that no longer affects the test: delete the vi.mock for "../../../utils/oauth-proxy.js" (including OAuthProxyError and validateUrl) and the guest-focused middleware inside createGuestDoctorApp that sets c.set("guestId", "guest-1") and (c as any).mcpClientManager = {}; keep createGuestDoctorApp routing to serversRoutes and the test assertion that relies on projectServerSchema rejecting the guest-shaped body; references to runGuestDoctor can be removed as well.mcpjam-inspector/client/src/state/__tests__/storage.test.ts (1)
73-113: ⚡ Quick winPlease keep state tests in the canonical suite.
This new state-storage scenario is being added under
storage.test.ts, but the repo convention centralizes state tests inclient/src/state/__tests__/app-reducer.test.ts. Please move this coverage there instead of extending a parallel state test file. As per coding guidelines, "State tests should be located inclient/src/state/__tests__/app-reducer.test.ts".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/state/__tests__/storage.test.ts` around lines 73 - 113, The new state test belongs in the canonical suite; move the whole "drops persisted oauth traces from app state without clearing trace storage" test (including the localStorage.setItem setup, the call to loadAppState(), and the three expects) out of storage.test.ts and into client/src/state/__tests__/app-reducer.test.ts, placing it under the existing state-related describe block so it shares the same setup/teardown; update any imports/exports used by the test (e.g., ensure loadAppState is imported where needed) and remove the duplicate test from storage.test.ts to keep a single authoritative state-tests file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/components/evals/EvalTabGate.tsx`:
- Line 51: The current gate condition in EvalTabGate.tsx uses OR and therefore
blocks hosted guests with a resolved project; change the predicate to require
both unauthenticated and lacking a project/user instead. Replace the check if
(!isAuthenticated || (!user && !projectId)) with an AND-based check (e.g. if
(!isAuthenticated && !user && !projectId) or equivalently if (!isAuthenticated
&& (!user && !projectId))) so hosted guests with a valid projectId are allowed
consistent with the hosted-guest handling in the web context code.
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 143-146: In saveOAuthConfigToLocalStorage, clear any stale
localStorage entries with prefixes "mcp-tokens-", "mcp-client-",
"mcp-serverUrl-", and "mcp-oauth-config-" before performing the HOSTED_MODE
early return so hosted sessions cannot reuse old client-side tokens; ensure this
cleanup runs when HOSTED_MODE is true (and not just when persisting data), and
note that handleConnect still calls getStoredTokens(formData.name) so removing
those keys prevents silent reuse of stale tokens.
In `@mcpjam-inspector/client/src/state/app-types.ts`:
- Around line 140-182: The synthetic default project ID was changed to a random
value so isSyntheticDefaultProject (in use-project-state.ts) which checks
project.id === "default" no longer detects synthetic defaults; update the
creation helpers to either (A) preserve a stable sentinel id by making
createLocalDefaultProject use id = overrides.id ?? "default" (or have
createLocalProjectId return "default" when used for the local default), or (B)
explicitly mark the synthetic default (e.g., add a boolean flag like synthetic:
true in createLocalDefaultProject) and update isSyntheticDefaultProject to check
project.synthetic (or project.isDefault) instead of comparing the id; adjust
createInitialAppState/initialAppState accordingly so the synthetic default is
consistently recognized.
In `@mcpjam-inspector/client/src/state/storage.ts`:
- Around line 61-63: When HOSTED_MODE is detected inside the early-return branch
(the block that currently returns createInitialAppState()), clear any persisted
keys like "mcp-inspector-state" and "mcp-inspector-projects" from localStorage
(and any other hosted-only keys you know) before returning so old sensitive
blobs are purged on first hosted load; apply the same cleanup to the other
early-return branch around the other occurrence (the block at ~165-167) so both
code paths remove those keys prior to returning createInitialAppState().
---
Outside diff comments:
In `@mcpjam-inspector/client/src/hooks/use-chat-session.ts`:
- Around line 1299-1305: The buildHostedBody function currently returns a
partial hosted payload when hostedProjectId is missing; instead, enforce the
invariant by throwing an error immediately if hostedProjectId is falsy so
callers (e.g., sendMessage) cannot send invalid requests. Update buildHostedBody
to check hostedProjectId at the top and throw a descriptive exception
(mentioning hostedProjectId, chatSessionId) rather than returning {
chatSessionId, directVisibility }, ensuring any callers relying on
buildHostedBody fail fast when the hosted context is unresolved.
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 1686-1702: If HOSTED_MODE is true but syncServerToConvex fails or
returns no serverId, stop the hosted OAuth flow instead of continuing: after
calling syncServerToConvex in use-server-state (references: HOSTED_MODE,
syncServerToConvex, hostedServerId, injectHostedServerMapping), if an error is
thrown or serverId is falsy then do not set hostedServerId and immediately
abort/return from the hosted-path logic so prepareHostedProjectOAuthRedirect (in
mcp-oauth.ts) is never invoked; surface/log the failure and fall back to the
non-hosted/local callback path or surface an explicit error to the caller.
Ensure the catch branch treats missing serverId the same as an exception and
prevents further hosted-specific steps.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/state/__tests__/storage.test.ts`:
- Around line 73-113: The new state test belongs in the canonical suite; move
the whole "drops persisted oauth traces from app state without clearing trace
storage" test (including the localStorage.setItem setup, the call to
loadAppState(), and the three expects) out of storage.test.ts and into
client/src/state/__tests__/app-reducer.test.ts, placing it under the existing
state-related describe block so it shares the same setup/teardown; update any
imports/exports used by the test (e.g., ensure loadAppState is imported where
needed) and remove the duplicate test from storage.test.ts to keep a single
authoritative state-tests file.
In `@mcpjam-inspector/server/routes/web/__tests__/servers-doctor.test.ts`:
- Around line 20-52: Remove the stale guest-related scaffolding that no longer
affects the test: delete the vi.mock for "../../../utils/oauth-proxy.js"
(including OAuthProxyError and validateUrl) and the guest-focused middleware
inside createGuestDoctorApp that sets c.set("guestId", "guest-1") and (c as
any).mcpClientManager = {}; keep createGuestDoctorApp routing to serversRoutes
and the test assertion that relies on projectServerSchema rejecting the
guest-shaped body; references to runGuestDoctor can be removed as well.
🪄 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: 35766d26-5013-4a1a-8b37-0f7f52f490f2
📒 Files selected for processing (30)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/evals/EvalTabGate.tsxmcpjam-inspector/client/src/components/evals/single-test-case-runner.tsmcpjam-inspector/client/src/hooks/hosted/use-hosted-api-context.tsmcpjam-inspector/client/src/hooks/use-app-state.tsmcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/client/src/hooks/use-is-direct-guest.tsmcpjam-inspector/client/src/hooks/use-project-state.tsmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/lib/__tests__/hosted-web-context.test.tsmcpjam-inspector/client/src/lib/apis/__tests__/evals-api.hosted.test.tsmcpjam-inspector/client/src/lib/apis/__tests__/mcp-conformance-api.test.tsmcpjam-inspector/client/src/lib/apis/__tests__/mcp-prompts-api.hosted.test.tsmcpjam-inspector/client/src/lib/apis/evals-api.tsmcpjam-inspector/client/src/lib/apis/mcp-prompts-api.tsmcpjam-inspector/client/src/lib/apis/web/__tests__/context.guest-fallback.test.tsmcpjam-inspector/client/src/lib/apis/web/context.tsmcpjam-inspector/client/src/lib/evals/generate-and-persist-tests.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/state/__tests__/storage.test.tsmcpjam-inspector/client/src/state/app-types.tsmcpjam-inspector/client/src/state/storage.tsmcpjam-inspector/server/routes/web/__tests__/evals.test.tsmcpjam-inspector/server/routes/web/__tests__/servers-doctor.test.tsmcpjam-inspector/server/routes/web/auth.tsmcpjam-inspector/server/routes/web/chat-v2.tsmcpjam-inspector/server/routes/web/conformance.tsmcpjam-inspector/server/routes/web/evals.tsmcpjam-inspector/server/routes/web/hosted-rpc-logs.tsmcpjam-inspector/server/routes/web/servers.ts
💤 Files with no reviewable changes (6)
- mcpjam-inspector/server/routes/web/evals.ts
- mcpjam-inspector/server/routes/web/hosted-rpc-logs.ts
- mcpjam-inspector/client/src/App.tsx
- mcpjam-inspector/server/routes/web/conformance.ts
- mcpjam-inspector/client/src/hooks/hosted/use-hosted-api-context.ts
- mcpjam-inspector/client/src/lib/apis/tests/mcp-conformance-api.test.ts
| } | ||
|
|
||
| if (!isAuthenticated || !user) { | ||
| if (!isAuthenticated || (!user && !projectId)) { |
There was a problem hiding this comment.
Line 51 still locks out hosted guests with a resolved project.
Because the first disjunct is !isAuthenticated, any guest session is gated here even when projectId is already present. That conflicts with mcpjam-inspector/client/src/lib/apis/web/context.ts:54-75 and mcpjam-inspector/client/src/lib/apis/web/context.ts:286-320, which now treat hosted guests as valid project-bound callers, so the Playground path remains inaccessible for the flow this PR is trying to unify. Please align this predicate with the hosted guest rule instead of checking !isAuthenticated on its own.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/components/evals/EvalTabGate.tsx` at line 51, The
current gate condition in EvalTabGate.tsx uses OR and therefore blocks hosted
guests with a resolved project; change the predicate to require both
unauthenticated and lacking a project/user instead. Replace the check if
(!isAuthenticated || (!user && !projectId)) with an AND-based check (e.g. if
(!isAuthenticated && !user && !projectId) or equivalently if (!isAuthenticated
&& (!user && !projectId))) so hosted guests with a valid projectId are allowed
consistent with the hosted-guest handling in the web context code.
| function saveOAuthConfigToLocalStorage(formData: ServerFormData): void { | ||
| if (HOSTED_MODE) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Clear stale browser OAuth state before the hosted early-return.
Returning here leaves any old mcp-tokens-*, mcp-client-*, mcp-serverUrl-*, and mcp-oauth-config-* entries intact. Later in handleConnect, this same file still calls getStoredTokens(formData.name), so hosted sessions can silently reuse a stale local token instead of the new server-backed credential path.
Possible fix
function saveOAuthConfigToLocalStorage(formData: ServerFormData): void {
if (HOSTED_MODE) {
+ localStorage.removeItem(`mcp-tokens-${formData.name}`);
+ localStorage.removeItem(`mcp-client-${formData.name}`);
+ localStorage.removeItem(`mcp-serverUrl-${formData.name}`);
+ localStorage.removeItem(`mcp-oauth-config-${formData.name}`);
return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 143 -
146, In saveOAuthConfigToLocalStorage, clear any stale localStorage entries with
prefixes "mcp-tokens-", "mcp-client-", "mcp-serverUrl-", and "mcp-oauth-config-"
before performing the HOSTED_MODE early return so hosted sessions cannot reuse
old client-side tokens; ensure this cleanup runs when HOSTED_MODE is true (and
not just when persisting data), and note that handleConnect still calls
getStoredTokens(formData.name) so removing those keys prevents silent reuse of
stale tokens.
| export function createLocalProjectId(): string { | ||
| if ( | ||
| typeof crypto !== "undefined" && | ||
| typeof crypto.randomUUID === "function" | ||
| ) { | ||
| return crypto.randomUUID(); | ||
| } | ||
|
|
||
| return `local_${Date.now()}_${Math.random().toString(36).slice(2, 10)}`; | ||
| } | ||
|
|
||
| export function createLocalDefaultProject( | ||
| overrides: Partial<Project> = {}, | ||
| ): Project { | ||
| const now = new Date(); | ||
| const id = overrides.id ?? createLocalProjectId(); | ||
| return { | ||
| id, | ||
| name: "Default", | ||
| description: "Default project", | ||
| servers: {}, | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| isDefault: true, | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| export function createInitialAppState(): AppState { | ||
| const project = createLocalDefaultProject(); | ||
| return { | ||
| projects: { | ||
| [project.id]: project, | ||
| }, | ||
| }, | ||
| activeProjectId: "default", | ||
| servers: {}, | ||
| selectedServer: "none", | ||
| selectedMultipleServers: [], | ||
| isMultiSelectMode: false, | ||
| }; | ||
| activeProjectId: project.id, | ||
| servers: {}, | ||
| selectedServer: "none", | ||
| selectedMultipleServers: [], | ||
| isMultiSelectMode: false, | ||
| }; | ||
| } | ||
|
|
||
| export const initialAppState: AppState = createInitialAppState(); |
There was a problem hiding this comment.
Dynamic default IDs now bypass the synthetic-default guard.
use-project-state.ts still defines isSyntheticDefaultProject() as project.id === "default". After these helpers switched the placeholder default to a generated id, that guard stops matching, so the synthetic local default can fall into migratableLocalProjects and get uploaded as a real Convex project on first auth. Please either keep a stable sentinel or mark synthetic defaults explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/state/app-types.ts` around lines 140 - 182, The
synthetic default project ID was changed to a random value so
isSyntheticDefaultProject (in use-project-state.ts) which checks project.id ===
"default" no longer detects synthetic defaults; update the creation helpers to
either (A) preserve a stable sentinel id by making createLocalDefaultProject use
id = overrides.id ?? "default" (or have createLocalProjectId return "default"
when used for the local default), or (B) explicitly mark the synthetic default
(e.g., add a boolean flag like synthetic: true in createLocalDefaultProject) and
update isSyntheticDefaultProject to check project.synthetic (or
project.isDefault) instead of comparing the id; adjust
createInitialAppState/initialAppState accordingly so the synthetic default is
consistently recognized.
| if (HOSTED_MODE) { | ||
| return createInitialAppState(); | ||
| } |
There was a problem hiding this comment.
Hosted mode stops persisting, but it never purges the old blobs.
Returning early here prevents new writes, yet any pre-existing mcp-inspector-state / mcp-inspector-projects data remains in localStorage. Those records can still contain server configs and oauthTokens, so hosted users may retain sensitive state on disk indefinitely after this rollout. Please clear the hosted-only storage keys on first hosted load.
Suggested cleanup
export function loadAppState(): AppState {
try {
if (HOSTED_MODE) {
+ localStorage.removeItem(STORAGE_KEY);
+ localStorage.removeItem(PROJECTS_STORAGE_KEY);
+ localStorage.removeItem(LEGACY_WORKSPACES_STORAGE_KEY);
return createInitialAppState();
}Also applies to: 165-167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/state/storage.ts` around lines 61 - 63, When
HOSTED_MODE is detected inside the early-return branch (the block that currently
returns createInitialAppState()), clear any persisted keys like
"mcp-inspector-state" and "mcp-inspector-projects" from localStorage (and any
other hosted-only keys you know) before returning so old sensitive blobs are
purged on first hosted load; apply the same cleanup to the other early-return
branch around the other occurrence (the block at ~165-167) so both code paths
remove those keys prior to returning createInitialAppState().
- abort hosted OAuth when syncServerToConvex fails or returns no serverId; without it prepareHostedProjectOAuthRedirect is a no-op and mcp-oauth's saveTokens is gated on !HOSTED_MODE, so the OAuth dance would otherwise complete without a durable credential. - gate getStoredTokensState/readStoredOAuthConfig/hasOAuthConfig on !HOSTED_MODE at the source so hosted sessions can't reuse stale local tokens left over from prior visits (writes were already gated). - buildHostedBody now throws when hostedProjectId is missing instead of returning a partial body that the server would reject. - isSyntheticDefaultProject no longer compares project.id === "default"; the other six predicates uniquely identify the local placeholder, and the id check broke when local project ids became generated UUIDs. - delete the directGuestMode = false literal and its dead branches in use-chat-session. - inline the trivial mergeHostedEvalServerRequest passthrough. - drop stale guest scaffolding (oauth-proxy mock, guest middleware) from servers-doctor.test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chelojimenez
temporarily deployed
to
preview-pr-2024
GitHub Actions
Inactive
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 66af254. Configure here.
chelojimenez
temporarily deployed
to
preview-pr-2024
GitHub Actions
Inactive
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/server/routes/web/chat-v2.ts (1)
65-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnvalidated critical fields bypass schema protection.
hostedChatSchemavalidatesprojectId,selectedServerIds,selectedServerNames,surface,shareToken, andchatboxToken, yet the subsequent type-castrawBody as unknown as ChatV2Request & {...}sidesteps that validation. More critically, fields likemessages,model,systemPrompt,temperature, andrequireToolApproval—which are not in the schema at all—are extracted directly from the unvalidatedrawBodyand passed intoconvertToModelMessages(),createAuthorizedManager(), and handlers with no validation whatsoever. A type-cast provides no runtime safety. Extract validated fields from the parsedhostedBodyinstead, and either add schema validation for the remaining fields or document why they are intentionally unvalidated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/routes/web/chat-v2.ts` around lines 65 - 92, The code currently casts rawBody to ChatV2Request and reads critical fields (messages, model, systemPrompt, temperature, requireToolApproval, etc.) without runtime validation, bypassing hostedChatSchema; update the flow to pull all user-controlled fields from the already-parsed hostedBody (not rawBody) and extend hostedChatSchema to include messages, model, systemPrompt, temperature, requireToolApproval, selectedServerIds/names, shareToken, chatboxToken and surface (or add separate schemas) so those values are validated before use; ensure convertToModelMessages(), createAuthorizedManager(), and any downstream handlers receive validated values (or reject/bail on schema errors) and remove the unsafe rawBody type-cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@mcpjam-inspector/server/routes/web/chat-v2.ts`:
- Around line 65-92: The code currently casts rawBody to ChatV2Request and reads
critical fields (messages, model, systemPrompt, temperature,
requireToolApproval, etc.) without runtime validation, bypassing
hostedChatSchema; update the flow to pull all user-controlled fields from the
already-parsed hostedBody (not rawBody) and extend hostedChatSchema to include
messages, model, systemPrompt, temperature, requireToolApproval,
selectedServerIds/names, shareToken, chatboxToken and surface (or add separate
schemas) so those values are validated before use; ensure
convertToModelMessages(), createAuthorizedManager(), and any downstream handlers
receive validated values (or reject/bail on schema errors) and remove the unsafe
rawBody type-cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb151212-21dd-4a8c-afff-9a87ccafc1ea
📒 Files selected for processing (2)
mcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/server/routes/web/chat-v2.ts
dosubot
Bot
added
size:L
chelojimenez
temporarily deployed
to
preview-pr-2024
GitHub Actions
Inactive
There was a problem hiding this comment.
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/components/evals/use-eval-queries.ts (1)
33-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd scope guard to prevent empty-args query during bootstrap.
enableOverviewQuerycurrently depends only onhasActorAccess, allowing it to fire withsuiteOverviewArgs = {}during the window between authentication and scope resolution. Contrast this with other queries in the same hook:enableSuiteDetailsQuerycompoundshasActorAccesswith!!selectedSuiteId, and the pattern mirrorsconvert-chat-session-dialog.tsxwhich gates onopen && effectiveProjectId. Per PR notes, hosted flows should require a scope ("fail loudly" rather than accept unscoped requests).🛡️ Suggested guard
- const enableOverviewQuery = hasActorAccess; + const hasQueryScope = + !!projectId || (!!organizationId && !isDirectGuest); + const enableOverviewQuery = hasActorAccess && hasQueryScope;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/components/evals/use-eval-queries.ts` around lines 33 - 47, enableOverviewQuery currently only checks hasActorAccess which allows the query to run with an empty suiteOverviewArgs; change the gate to require a resolved scope by combining hasActorAccess with the same condition used to build suiteOverviewArgs (i.e., require projectId or (organizationId && !isDirectGuest)) so that the useQuery call for "testSuites:getTestSuitesOverview" only runs when a valid scope exists; update the enableOverviewQuery boolean (used by the useQuery invocation) accordingly so unscoped requests are skipped during bootstrap.
🧹 Nitpick comments (4)
mcpjam-inspector/client/src/lib/apis/evals-api.ts (2)
236-247: 💤 Low valueConfirm
storageServerIdsfallback is intentional after the merge.
mergeHostedServerBatch(request)already exposes the resolvedserverIdsfrom the hosted batch (canonicalized viaresolveHostedServerEntries), yet the fallback on line 243 reaches back to the pre-mergerequest.serverIds. With the currentEvalRequestWithServerscontract these are already IDs, so the two are equivalent — but if a caller ever passes server names (whichbuildHostedServerBatchRequesttolerates),storageServerIdswould diverge from the resolved IDs sent in the batch. Worth a quick sanity check; if equivalence is the contract, consider sourcing the fallback from the merged batch for clarity.♻️ Optional clarifying tweak
hosted: () => { - return postEvalRequest(EVALS_API_ENDPOINTS.hosted.run, { - ...mergeHostedServerBatch(request), - storageServerIds: request.storageServerIds ?? request.serverIds, - } as JsonRecord); + const merged = mergeHostedServerBatch(request); + return postEvalRequest(EVALS_API_ENDPOINTS.hosted.run, { + ...merged, + storageServerIds: request.storageServerIds ?? merged.serverIds, + } as JsonRecord); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/apis/evals-api.ts` around lines 236 - 247, In runEvals, the storageServerIds fallback currently references the original request.serverIds which can diverge from the resolved IDs returned by mergeHostedServerBatch; update the hosted branch to source the fallback from the merged batch (the object returned by mergeHostedServerBatch(request)) instead of request.serverIds so storageServerIds always matches the canonicalized server IDs produced by mergeHostedServerBatch (refer to runEvals and mergeHostedServerBatch to locate the change).
351-357: 💤 Low valueMinor readability nit on the ternary cast.
The two branches differ in parenthesization —
mergeHostedServerBatch(request) as JsonRecordvs.(request as JsonRecord). Precedence is correct (asbinds tighter than?:), but matching the styling makes the intent immediately legible.✨ Proposed cosmetic alignment
const payload = isHostedMode() - ? mergeHostedServerBatch(request) as JsonRecord - : (request as JsonRecord); + ? (mergeHostedServerBatch(request) as JsonRecord) + : (request as JsonRecord);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/apis/evals-api.ts` around lines 351 - 357, The payload ternary mixes parenthesis/casting styles which hurts readability; update the payload assignment (referencing payload, isHostedMode, and mergeHostedServerBatch) so the cast is applied consistently — either cast each branch the same way (e.g., (mergeHostedServerBatch(request) as JsonRecord) vs (request as JsonRecord)) or cast the entire ternary result ((isHostedMode() ? mergeHostedServerBatch(request) : request) as JsonRecord) to make the intent and precedence uniform and easier to read.mcpjam-inspector/client/src/components/evals/__tests__/use-eval-queries.test.ts (1)
51-64: 💤 Low valueTest title promises more than the assertion delivers.
Behaviorally this case is indistinguishable from the preceding "reports overview loading" test — both pass
isAuthenticated: truewith aprojectId, both expectenableOverviewQuery: true. The descriptive intent (hosted guest, no WorkOS user) is no longer observable through the hook's surface sinceuserwas removed from the signature. Consider asserting the actual hosted contract: thatuseQueryis invoked with{ projectId: "guest-project" }rather than"skip", which would lock in the regression you're guarding against.♻️ Strengthened assertion
expect(result.current.enableOverviewQuery).toBe(true); + expect(mocks.useQuery).toHaveBeenCalledWith( + "testSuites:getTestSuitesOverview", + { projectId: "guest-project" }, + ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/components/evals/__tests__/use-eval-queries.test.ts` around lines 51 - 64, The test title claims "hosted guests (Convex-authenticated, no WorkOS user)" but only asserts enableOverviewQuery, which duplicates the prior test; update the test for useEvalQueries to assert that the underlying useQuery (mock or spy) is called with the hosted contract (i.e., called with an argument object containing projectId: "guest-project") instead of returning "skip"; locate the hook under test by name useEvalQueries and the current test case in use-eval-queries.test.ts, add or adjust a mock/spy for useQuery (or the module that wraps it) and replace the enableOverviewQuery-only assertion with an assertion that useQuery was invoked with { projectId: "guest-project" } (and keep enableOverviewQuery assertion if desired).mcpjam-inspector/client/src/components/evals/__tests__/use-eval-handlers.test.ts (1)
18-31: ⚡ Quick winConsider using partial mocks to preserve module exports for maintainability.
The current full mocks of
@/lib/configand@/lib/apis/mode-clientwork correctly today—use-eval-handlers.tsonly importsisHostedModefrommode-client, which is fully specified in the mock. However, partial mocks that spread the actual module and override only the hosted-mode selectors are better practice. If future changes add new imports from these modules, the full mock pattern could silently returnundefinedfor those exports.Suggested adjustment
-vi.mock("@/lib/config", () => ({ - get HOSTED_MODE() { - return hostedModeRef.value; - }, -})); -vi.mock("@/lib/apis/mode-client", () => ({ - isHostedMode: () => hostedModeRef.value, - ensureLocalMode: vi.fn(), - runByMode: (handlers: { local: () => unknown; hosted: () => unknown }) => - hostedModeRef.value ? handlers.hosted() : handlers.local(), -})); +vi.mock("@/lib/config", async () => { + const actual = await vi.importActual<typeof import("@/lib/config")>( + "@/lib/config", + ); + return { + ...actual, + get HOSTED_MODE() { + return hostedModeRef.value; + }, + }; +}); +vi.mock("@/lib/apis/mode-client", async () => { + const actual = await vi.importActual<typeof import("@/lib/apis/mode-client")>( + "@/lib/apis/mode-client", + ); + return { + ...actual, + isHostedMode: () => hostedModeRef.value, + ensureLocalMode: vi.fn(), + runByMode: (handlers: { local: () => unknown; hosted: () => unknown }) => + hostedModeRef.value ? handlers.hosted() : handlers.local(), + }; +});Also applies to: 101-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/components/evals/__tests__/use-eval-handlers.test.ts` around lines 18 - 31, The test currently fully mocks "@/lib/config" and "@/lib/apis/mode-client", which can hide future exports; change these to partial mocks by importing the real modules and spreading them, then override only the hosted-mode selectors (use hostedModeRef.value for HOSTED_MODE and for isHostedMode/runByMode/ensureLocalMode overrides). Specifically adjust the mocks that reference hostedModeRef, isHostedMode, ensureLocalMode and runByMode so they call or override only those members while preserving other real exports from "@/lib/config" and "@/lib/apis/mode-client".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@mcpjam-inspector/client/src/components/evals/use-eval-queries.ts`:
- Around line 33-47: enableOverviewQuery currently only checks hasActorAccess
which allows the query to run with an empty suiteOverviewArgs; change the gate
to require a resolved scope by combining hasActorAccess with the same condition
used to build suiteOverviewArgs (i.e., require projectId or (organizationId &&
!isDirectGuest)) so that the useQuery call for
"testSuites:getTestSuitesOverview" only runs when a valid scope exists; update
the enableOverviewQuery boolean (used by the useQuery invocation) accordingly so
unscoped requests are skipped during bootstrap.
---
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/evals/__tests__/use-eval-handlers.test.ts`:
- Around line 18-31: The test currently fully mocks "@/lib/config" and
"@/lib/apis/mode-client", which can hide future exports; change these to partial
mocks by importing the real modules and spreading them, then override only the
hosted-mode selectors (use hostedModeRef.value for HOSTED_MODE and for
isHostedMode/runByMode/ensureLocalMode overrides). Specifically adjust the mocks
that reference hostedModeRef, isHostedMode, ensureLocalMode and runByMode so
they call or override only those members while preserving other real exports
from "@/lib/config" and "@/lib/apis/mode-client".
In
`@mcpjam-inspector/client/src/components/evals/__tests__/use-eval-queries.test.ts`:
- Around line 51-64: The test title claims "hosted guests (Convex-authenticated,
no WorkOS user)" but only asserts enableOverviewQuery, which duplicates the
prior test; update the test for useEvalQueries to assert that the underlying
useQuery (mock or spy) is called with the hosted contract (i.e., called with an
argument object containing projectId: "guest-project") instead of returning
"skip"; locate the hook under test by name useEvalQueries and the current test
case in use-eval-queries.test.ts, add or adjust a mock/spy for useQuery (or the
module that wraps it) and replace the enableOverviewQuery-only assertion with an
assertion that useQuery was invoked with { projectId: "guest-project" } (and
keep enableOverviewQuery assertion if desired).
In `@mcpjam-inspector/client/src/lib/apis/evals-api.ts`:
- Around line 236-247: In runEvals, the storageServerIds fallback currently
references the original request.serverIds which can diverge from the resolved
IDs returned by mergeHostedServerBatch; update the hosted branch to source the
fallback from the merged batch (the object returned by
mergeHostedServerBatch(request)) instead of request.serverIds so
storageServerIds always matches the canonicalized server IDs produced by
mergeHostedServerBatch (refer to runEvals and mergeHostedServerBatch to locate
the change).
- Around line 351-357: The payload ternary mixes parenthesis/casting styles
which hurts readability; update the payload assignment (referencing payload,
isHostedMode, and mergeHostedServerBatch) so the cast is applied consistently —
either cast each branch the same way (e.g., (mergeHostedServerBatch(request) as
JsonRecord) vs (request as JsonRecord)) or cast the entire ternary result
((isHostedMode() ? mergeHostedServerBatch(request) : request) as JsonRecord) to
make the intent and precedence uniform and easier to read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4a9918f-7648-4582-b9fe-5433ead194e9
📒 Files selected for processing (8)
mcpjam-inspector/client/src/components/CiEvalsTab.tsxmcpjam-inspector/client/src/components/EvalsTab.tsxmcpjam-inspector/client/src/components/evals/__tests__/use-eval-handlers.test.tsmcpjam-inspector/client/src/components/evals/__tests__/use-eval-queries.test.tsmcpjam-inspector/client/src/components/evals/use-eval-queries.tsmcpjam-inspector/client/src/hooks/__tests__/use-chat-session.hosted.test.tsxmcpjam-inspector/client/src/lib/apis/evals-api.tsmcpjam-inspector/server/routes/web/__tests__/chat-v2.guest.test.ts
💤 Files with no reviewable changes (4)
- mcpjam-inspector/client/src/components/EvalsTab.tsx
- mcpjam-inspector/client/src/components/CiEvalsTab.tsx
- mcpjam-inspector/server/routes/web/tests/chat-v2.guest.test.ts
- mcpjam-inspector/client/src/hooks/tests/use-chat-session.hosted.test.tsx
dosubot
Bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/client/src/hooks/use-chat-session.ts (1)
2087-2112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock hosted guest submit until the share/chatbox scope is resolved.
submitBlockednow waits forprojectIdand server IDs, but not for the hosted guest scope token. During bootstrap, that leaves a render whereprojectIdis present,hostedShareToken/hostedChatboxTokenare still unset, andbuildHostedBody()falls back toisHostedDirectChat—the very path this PR is removing.Suggested guard
+ const hostedGuestScopeNotReady = + HOSTED_MODE && + isHostedGuest && + !hostedShareToken && + !hostedChatboxToken; + const submitBlocked = disableForAuthentication || isAuthLoading || authHeadersNotReady || - hostedContextNotReady; + hostedContextNotReady || + hostedGuestScopeNotReady;You may also want
buildHostedBody()to throw for hosted guests when neither scope token is present, so programmatic sends cannot reintroduce the fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/hooks/use-chat-session.ts` around lines 2087 - 2112, The submit gating logic must also wait for hosted guest scope tokens: update the hostedContextNotReady (or include in submitBlocked) to consider hostedShareToken and hostedChatboxToken unresolved for HOSTED_MODE guests by checking that at least one required scope token is present (hostedShareToken || hostedChatboxToken) before allowing submit; reference variables: hostedContextNotReady, submitBlocked, hostedShareToken, hostedChatboxToken, hostedProjectId, hostedSelectedServerIds, selectedServers. Also update buildHostedBody() to explicitly throw when running as a hosted guest and neither hostedShareToken nor hostedChatboxToken is available so programmatic sends cannot fall back to isHostedDirectChat.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/state/__tests__/storage.test.ts`:
- Around line 108-112: The test currently only checks state.activeProjectId !==
"default", which can miss cases where the id points to a nonexistent project;
update the assertion after calling loadAppState() to assert the resolved id
actually exists in state.projects (e.g. verify state.projects contains an entry
whose id equals state.activeProjectId) so the migration failure is caught; refer
to loadAppState, state.activeProjectId, and state.projects when making this
change.
---
Outside diff comments:
In `@mcpjam-inspector/client/src/hooks/use-chat-session.ts`:
- Around line 2087-2112: The submit gating logic must also wait for hosted guest
scope tokens: update the hostedContextNotReady (or include in submitBlocked) to
consider hostedShareToken and hostedChatboxToken unresolved for HOSTED_MODE
guests by checking that at least one required scope token is present
(hostedShareToken || hostedChatboxToken) before allowing submit; reference
variables: hostedContextNotReady, submitBlocked, hostedShareToken,
hostedChatboxToken, hostedProjectId, hostedSelectedServerIds, selectedServers.
Also update buildHostedBody() to explicitly throw when running as a hosted guest
and neither hostedShareToken nor hostedChatboxToken is available so programmatic
sends cannot fall back to isHostedDirectChat.
🪄 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: 4e826c84-3a77-4d7f-89b1-f7308be7cf93
📒 Files selected for processing (6)
mcpjam-inspector/client/src/hooks/__tests__/useSharedChatWidgetCapture.test.tsxmcpjam-inspector/client/src/hooks/use-app-state.tsmcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/client/src/hooks/useSharedChatWidgetCapture.tsmcpjam-inspector/client/src/state/__tests__/storage.test.tsmcpjam-inspector/client/src/state/storage.ts
💤 Files with no reviewable changes (1)
- mcpjam-inspector/client/src/hooks/tests/useSharedChatWidgetCapture.test.tsx
| const state = loadAppState(); | ||
|
|
||
| expect(state.activeProjectId).not.toBe("default"); | ||
| expect(state.servers.asana.lastOAuthTrace).toBeUndefined(); | ||
| expect(localStorage.getItem("mcp-oauth-trace-asana")).toBeNull(); |
There was a problem hiding this comment.
Strengthen the active-project assertion.
expect(state.activeProjectId).not.toBe("default") still passes if the ID regresses to a missing project. Assert that the resolved id actually exists in state.projects so this test catches the failure mode the migration cares about.
Suggested assertion
- expect(state.activeProjectId).not.toBe("default");
+ const activeProject = state.projects[state.activeProjectId];
+ expect(activeProject).toBeDefined();
+ expect(activeProject?.isDefault).toBe(true);📝 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.
| const state = loadAppState(); | |
| expect(state.activeProjectId).not.toBe("default"); | |
| expect(state.servers.asana.lastOAuthTrace).toBeUndefined(); | |
| expect(localStorage.getItem("mcp-oauth-trace-asana")).toBeNull(); | |
| const state = loadAppState(); | |
| const activeProject = state.projects[state.activeProjectId]; | |
| expect(activeProject).toBeDefined(); | |
| expect(activeProject?.isDefault).toBe(true); | |
| expect(state.servers.asana.lastOAuthTrace).toBeUndefined(); | |
| expect(localStorage.getItem("mcp-oauth-trace-asana")).toBeNull(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/state/__tests__/storage.test.ts` around lines 108
- 112, The test currently only checks state.activeProjectId !== "default", which
can miss cases where the id points to a nonexistent project; update the
assertion after calling loadAppState() to assert the resolved id actually exists
in state.projects (e.g. verify state.projects contains an entry whose id equals
state.activeProjectId) so the migration failure is caught; refer to
loadAppState, state.activeProjectId, and state.projects when making this change.
1 participant
Note
Medium Risk
Moderate risk because it changes hosted guest routing/auth assumptions across chat, evals, prompts, servers/doctor, and OAuth token persistence; regressions could block guest usage or break hosted project bootstrap timing.
Overview
Hosted mode no longer supports direct guest request bodies (e.g.,
{serverUrl, serverHeaders}); hosted guests now always go through Convex-backed{projectId, serverId/serverIds}payloads, and multiple client/server routes are updated to throw validation/bootstrap errors whenprojectIdis missing.Client-side state and OAuth persistence are adjusted for this: hosted mode stops reading/writing server OAuth config/tokens/envs to
localStorage, chat session submission now requireshostedProjectId(no direct-guest fallback), eval queries/gates treat ConvexisAuthenticatedas sufficient for hosted guests, and local projects now use generated IDs viacreateLocalProjectId/createLocalDefaultProject(including updated storage migration/selection logic).Reviewed by Cursor Bugbot for commit 2bd5f76. Bugbot is set up for automated code reviews on this repo. Configure here.