Cli authed#1981
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
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 PR updates CLI Inspector render handling to make 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/src/lib/inspector-render.ts (1)
220-235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop retrying
unsupported_in_modeuntil the timeout expires.This branch treats
unsupported_in_modeas retryable, so a permanent mode mismatch now waits out the full render timeout before returning a skipped result. That clashes with the laterremediation: "none"classification and can turn a simple skip into a ~30s stall.Suggested fix
const retryable = response.status === "error" && (response.error.code === "no_active_client" || - response.error.code === "unsupported_in_mode" || response.error.code === "disconnected_server");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/lib/inspector-render.ts` around lines 220 - 235, The retry logic currently marks "unsupported_in_mode" as retryable in the retryable determination (the const retryable = ... expression), causing permanent mode mismatches to wait out the deadline; update that logic in inspector-render.ts (where retryable is computed) to exclude "unsupported_in_mode" so only "no_active_client" and "disconnected_server" are considered retryable (or equivalently, remove "unsupported_in_mode" from the OR list), which will cause the function to return immediately for unsupported_in_mode and avoid unnecessary waiting in the loop that uses emitWaitingProgress, remaining/deadline checks, delay, and the surrounding do/while retry.cli/src/commands/tools.ts (1)
192-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate stdin-backed option conflicts before parsing the server config.
--tool-args-stdinis only checked against--tool-args/--params, but shared flags like--client-capabilities -can also consume stdin. BecauseparseServerConfig()already runs before this validation, a command can drain stdin during config parsing and then fail when tool args try to read it a second time.Suggested direction
+ if (options.toolArgsStdin && options.clientCapabilities === "-") { + throw usageError( + "--tool-args-stdin cannot be used together with --client-capabilities -.", + ); + } const config = parseServerConfig({ ...options, timeout: globalOptions.timeout, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/tools.ts` around lines 192 - 220, The stdin conflict check must run before parseServerConfig to avoid draining stdin during config parsing: move the resolution/validation of tool args (calls to resolveAliasedStringOption for toolName and for toolArgs/params, and the options.toolArgsStdin check that throws the usageError) to execute before calling parseServerConfig, and ensure you also treat any aliased value of "-" (stdin) for other shared flags (e.g., clientCapabilities if present in options) as a conflict; then compute paramsInput and call parseJsonRecord as before after validating stdin usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/commands/tools.ts`:
- Around line 439-450: The outcome computation only checks render errors; update
the ternary to also treat overall invocation failures as errors by including
conditions on the payload (e.g., check debugOutputPayload.success === false or
debugOutputPayload.isError === true or any outputPayload flag that indicates
failure) alongside the existing
inspectorRenderError/inspectorRenderSkipped/requireRender logic so the artifact
status becomes "error" when the command validation/expect-success fails as well;
adjust the expression that builds outcome (the ternary using
inspectorRenderError, inspectorRenderSkipped, requireRender,
inspectorRenderIssue, debugOutputPayload) to include these payload failure
checks.
In `@docs/cli/reference.mdx`:
- Line 125: The doc incorrectly states that the App Builder URL and initial
"waiting to connect" progress appear only when stderr is a TTY; update the
sentence mentioning TTY stderr, `--ui`, `--open`, and `--quiet` to say that
milestone lines (App Builder URL and initial browser-client wait progress) are
emitted when `--open` is used even on non-TTY stderr, while only the
elapsed-seconds heartbeat is TTY-gated; keep references to `--ui`, `--open`,
`--quiet`, and the "elapsed-seconds heartbeat" so readers understand which
outputs are TTY-restricted versus emitted for `--open`.
In `@docs/cli/tools-resources-prompts.mdx`:
- Around line 137-142: Add a one-line note next to the `--tool-args-stdin`
equivalence example clarifying that `--tool-args-stdin` is mutually exclusive
with `--tool-args` (they cannot be combined) to avoid copy/paste misuse; update
the `--tool-args-stdin` example block (the `mcpjam tools call` command) to
include this conflict rule so readers know not to pass `--tool-args` when using
`--tool-args-stdin`.
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 854-889: The dedupeKey is based on the workspace seen at entry but
the loop can resolve a different active workspace later; freeze the intended
target at entry and abort if it changes before the mutation: capture a
frozenWorkspaceId (e.g. from initialWorkspaceKey) before the wait loop and
compute dedupeKey from that frozen id, then inside the loop after determining
workspaceId (the resolved target) verify workspaceId === frozenWorkspaceId
(treat "pending"/null as appropriate) and if it differs call clearDedupeKey()
and return "pending" or "noop" to avoid persisting into the wrong workspace;
update references to dedupeKey/persistRuntimeDedupeKeysRef and ensure
clearDedupeKey is invoked on mismatch.
- Around line 843-845: resolveRuntime currently always returns
runtimeServerOverride first, allowing a stale override to be used when checking
before persisting; change it to consult appStateServersRef.current[serverName]
first and only fall back to runtimeServerOverride if the current app state entry
is undefined/absent. Update the resolveRuntime implementation(s) (the function
named resolveRuntime and the duplicate logic around the second check) to return
appStateServersRef.current[serverName] ?? runtimeServerOverride, and apply the
same change to the other occurrence referenced in the comment so the pre-write
connection check always re-reads live runtime state before saving.
In `@mcpjam-inspector/client/src/hooks/use-workspace-state.ts`:
- Around line 441-479: The merge into mergedConvexWorkspaces currently scans all
scopedLocalWorkspaces and can project transient runtime-only servers from
non-active local workspaces; change the projection so it only uses the active
local workspace (do not iterate Object.values(scopedLocalWorkspaces)). In the
mapping logic for convexWorkspaces (inside mergedConvexWorkspaces), look up the
single active localProjection from scopedLocalWorkspaces using the app state’s
active/local workspace identifier (instead of finding any candidate by
sharedWorkspaceId), then apply the projectedServers filter from that single
projection; if no active projection exists, return the original workspace
unchanged.
---
Outside diff comments:
In `@cli/src/commands/tools.ts`:
- Around line 192-220: The stdin conflict check must run before
parseServerConfig to avoid draining stdin during config parsing: move the
resolution/validation of tool args (calls to resolveAliasedStringOption for
toolName and for toolArgs/params, and the options.toolArgsStdin check that
throws the usageError) to execute before calling parseServerConfig, and ensure
you also treat any aliased value of "-" (stdin) for other shared flags (e.g.,
clientCapabilities if present in options) as a conflict; then compute
paramsInput and call parseJsonRecord as before after validating stdin usage.
In `@cli/src/lib/inspector-render.ts`:
- Around line 220-235: The retry logic currently marks "unsupported_in_mode" as
retryable in the retryable determination (the const retryable = ... expression),
causing permanent mode mismatches to wait out the deadline; update that logic in
inspector-render.ts (where retryable is computed) to exclude
"unsupported_in_mode" so only "no_active_client" and "disconnected_server" are
considered retryable (or equivalently, remove "unsupported_in_mode" from the OR
list), which will cause the function to return immediately for
unsupported_in_mode and avoid unnecessary waiting in the loop that uses
emitWaitingProgress, remaining/deadline checks, delay, and the surrounding
do/while retry.
🪄 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: 0ef500ed-977d-4029-bb34-d907d03107d7
📒 Files selected for processing (23)
cli/src/commands/tools.tscli/src/lib/inspector-render.tscli/tests/agent-friendly-cli.test.tscli/tests/tools-call-ui.test.tsdocs/cli/apps-conformance.mdxdocs/cli/overview.mdxdocs/cli/reference.mdxdocs/cli/tools-resources-prompts.mdxdocs/inspector/skills.mdxmcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/ui-playground/AppBuilderTab.tsxmcpjam-inspector/client/src/hooks/__tests__/use-chat-session.hosted.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.hosted-oauth.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsxmcpjam-inspector/client/src/hooks/use-app-state.tsmcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/use-workspace-state.tsmcpjam-inspector/client/src/lib/__tests__/session-token.test.tsmcpjam-inspector/client/src/lib/session-token.tsskills/mcp-inspector/SKILL.mdskills/mcp-inspector/references/cli-surface-notes.md
![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: afbc9104fa
ℹ️ 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".
| ? new URL(input.url, baseOrigin) | ||
| : new URL(String(input), baseOrigin); | ||
|
|
||
| return isLoopbackHostname(parsed.hostname) && parsed.pathname.startsWith("/api/"); |
There was a problem hiding this comment.
Update shouldAttachSessionHeaders to allow same-origin /api/* requests, not just loopback hosts. As written, any non-hosted deployment served from a non-loopback origin (for example a production domain) will parse relative "/api/..." URLs with that hostname and return false, so authFetch drops X-MCP-Session-Auth and protected API routes start returning 401s. This is a regression from the previous behavior that always attached session auth in non-hosted mode.
Useful? React with 👍 / 👎.
# Conflicts: # cli/src/commands/tools.ts # mcpjam-inspector/client/src/hooks/use-chat-session.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
dosubot
Bot
removed
the
size:XXL
chelojimenez
temporarily deployed
to
preview-pr-1981
GitHub Actions
Inactive
dosubot
Bot
added
the
size:L
Internal previewPreview URL: https://mcp-inspector-pr-1981.up.railway.app |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d0ddfb08f
ℹ️ 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".
| "persistRuntimeServerToWorkspaceIfNeeded: unexpected error", | ||
| { | ||
| serverName, | ||
| workspaceId, |
There was a problem hiding this comment.
workspaceId to function scope before catch logging
workspaceId is declared inside the try block but referenced in the outer catch, so any exception reaching this catch will throw a new ReferenceError (workspaceId is not defined) while handling the original failure. In that scenario the helper no longer returns the intended "failed" result and can leave callers with an unhandled rejection instead of controlled error handling.
Useful? React with 👍 / 👎.
dosubot
Bot
added
size:XL
chelojimenez
temporarily deployed
to
preview-pr-1981
GitHub Actions
Inactive
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 `@docs/cli/reference.mdx`:
- Line 125: Update the paragraph describing Inspector skipped renders to
explicitly document that the `no_active_client` skipped-render code is an
exception when `--attach-only` is set: state that normally a missing browser
client yields `inspectorRender.status = "skipped"` with remediation
`open_browser`, but if the CLI is invoked with `--attach-only` then
`no_active_client` is treated as non-skippable (results in a nonzero exit like
other non-skippable failures) and should not be downgraded to a warning;
reference `inspectorRender.status`, `inspectorRender.remediation`, the
`no_active_client` code, and the `--attach-only` flag so readers and agents can
locate and update the behavior description accordingly.
🪄 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: 2db47220-882c-4bb3-957f-44e30c59f335
📒 Files selected for processing (7)
cli/src/commands/tools.tscli/src/lib/inspector-render.tsdocs/cli/reference.mdxdocs/cli/tools-resources-prompts.mdxmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/use-workspace-state.tsmcpjam-inspector/client/src/lib/session-token.ts
💤 Files with no reviewable changes (1)
- cli/src/lib/inspector-render.ts
✅ Files skipped from review due to trivial changes (2)
- docs/cli/tools-resources-prompts.mdx
- mcpjam-inspector/client/src/hooks/use-server-state.ts
chelojimenez
temporarily deployed
to
preview-pr-1981
GitHub Actions
Inactive
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/hooks/use-server-state.ts`:
- Around line 873-874: Declare workspaceId (as let workspaceId: string | null =
null) outside the try block so it is in the same scope as the catch handler;
move the existing workspaceId declaration from inside the try in
use-server-state.ts to immediately before the try (near readyStarted) so both
the try block and the catch at line ~1023 can reference it without a TypeScript
scoping error.
🪄 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: 6ef7a11b-f161-4222-88b6-9ed4b5b5c7b7
📒 Files selected for processing (2)
docs/cli/reference.mdxmcpjam-inspector/client/src/hooks/use-server-state.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/cli/reference.mdx
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9d6e45c. Configure here.
|
|
||
| async function sleep(ms: number): Promise<void> { | ||
| await new Promise((resolve) => setTimeout(resolve, ms)); | ||
| } |
There was a problem hiding this comment.
Utility sleep recreated every render inside component
Low Severity
The sleep helper is defined as a plain function inside the useServerState component body, causing it to be recreated on every render. Since it has no closures over React state and is a pure utility, it can be hoisted to module scope. Additionally, it's captured by the persistRuntimeServerToWorkspaceIfNeeded useCallback but is not listed in its dependency array, which would trigger the react-hooks/exhaustive-deps lint rule.
Reviewed by Cursor Bugbot for commit 9d6e45c. Configure here.
chelojimenez
temporarily deployed
to
preview-pr-1981
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7d8c4d9b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const resolveRuntime = (): ServerWithName | undefined => | ||
| runtimeServerOverride ?? appStateServersRef.current[serverName]; |
There was a problem hiding this comment.
persistRuntimeServerToWorkspaceIfNeeded accepts a runtimeServerOverride and resolveRuntime always prefers it, so the later connectivity guard (runtime.connectionStatus !== "connected") re-checks the same stale snapshot instead of current state. In the openAppBuilder flow (App.tsx passes a runtime override), this helper can wait for auth/workspace readiness for seconds and still persist even if the server disconnected in the meantime, which writes outdated runtime entries into the cloud workspace.
Useful? React with 👍 / 👎.
| initialWorkspaceKey && initialWorkspaceKey !== "none" | ||
| ? initialWorkspaceKey | ||
| : null; | ||
| const dedupeKey = `${frozenWorkspaceId ?? "pending"}:${serverName}`; |
There was a problem hiding this comment.
The in-flight dedupe key includes frozenWorkspaceId and falls back to "pending", so a call started before workspace resolution uses pending:<server> while a second call after resolution uses <workspaceId>:<server>. Because these keys differ, concurrent calls for the same server bypass dedupe and can both run persistence/mutations, creating duplicate write attempts during startup or workspace-loading races.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mcpjam-inspector/client/src/hooks/use-server-state.ts (1)
843-845:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer live runtime state over the override before persisting.
Line 843 still resolves
runtimeServerOverridefirst. If that override is an older"connected"snapshot, the re-check at Lines 950-951 can ignore a newer disconnected entry inappStateServersRef.currentand persist a dead runtime session.Suggested fix
- const resolveRuntime = (): ServerWithName | undefined => - runtimeServerOverride ?? appStateServersRef.current[serverName]; + const resolveRuntime = (): ServerWithName | undefined => + appStateServersRef.current[serverName] ?? runtimeServerOverride;Also applies to: 950-951
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 843 - 845, The resolveRuntime logic currently prefers runtimeServerOverride over the live app state, which can cause persisting an outdated "connected" snapshot; change resolveRuntime to prefer appStateServersRef.current[serverName] (the live runtime state) and fall back to runtimeServerOverride only if the live entry is undefined, and apply the same inversion to the runtime re-check logic referenced at the later check (the code around the 950-951 check) so both places use appStateServersRef.current[serverName] first and only use runtimeServerOverride as a fallback.
🤖 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/hooks/use-server-state.ts`:
- Around line 854-870: The current dedupe uses a temporary key `dedupeKey =
\`\${frozenWorkspaceId ?? "pending"}:\${serverName}\`` and registers it in
`persistRuntimeDedupeKeysRef`, but when the real `workspaceId` is resolved later
(the resolution that leads into `syncServerToConvex`) a second call computes a
different key and bypasses the guard, allowing races; fix by rekeying the
in-flight entry once the concrete `workspaceId` is known: after resolving the
workspace id, compute `realKey = \`\${workspaceId}:\${serverName}\``, atomically
move the entry in `persistRuntimeDedupeKeysRef` from the old `dedupeKey` to
`realKey` (e.g., if `persistRuntimeDedupeKeysRef.current.has(dedupeKey)` then
delete it and add `realKey`), and ensure `clearDedupeKey` deletes both old and
new keys (or tracks the current active key and deletes that) so cleanup on
success/failure still removes the correct entry; update the logic around
`initialWorkspaceKey`, `frozenWorkspaceId`, `dedupeKey`,
`persistRuntimeDedupeKeysRef`, `clearDedupeKey`, and the post-resolution path
that calls `syncServerToConvex`.
---
Duplicate comments:
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 843-845: The resolveRuntime logic currently prefers
runtimeServerOverride over the live app state, which can cause persisting an
outdated "connected" snapshot; change resolveRuntime to prefer
appStateServersRef.current[serverName] (the live runtime state) and fall back to
runtimeServerOverride only if the live entry is undefined, and apply the same
inversion to the runtime re-check logic referenced at the later check (the code
around the 950-951 check) so both places use
appStateServersRef.current[serverName] first and only use runtimeServerOverride
as a fallback.
🪄 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: b7a21508-05dd-46fc-b380-a97c546c5468
📒 Files selected for processing (1)
mcpjam-inspector/client/src/hooks/use-server-state.ts
chelojimenez
temporarily deployed
to
preview-pr-1981
GitHub Actions
Inactive
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mcpjam-inspector/client/src/hooks/use-server-state.ts (1)
843-845:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRead live runtime state before the override.
Line 843 still prefers
runtimeServerOverride, so a stale disconnected snapshot can short-circuit at Lines 846-852 even whenappStateServersRef.current[serverName]is already connected. Please flip the precedence and use the override only as a fallback when the live entry is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 843 - 845, The current resolveRuntime function prefers runtimeServerOverride over the live app state, which can let a stale override short-circuit a newly connected server; change resolveRuntime to read appStateServersRef.current[serverName] first and return that if present/connected, and only use runtimeServerOverride as a fallback (i.e., flip the precedence of runtimeServerOverride and appStateServersRef.current[serverName] within resolveRuntime).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 843-845: The current resolveRuntime function prefers
runtimeServerOverride over the live app state, which can let a stale override
short-circuit a newly connected server; change resolveRuntime to read
appStateServersRef.current[serverName] first and return that if
present/connected, and only use runtimeServerOverride as a fallback (i.e., flip
the precedence of runtimeServerOverride and
appStateServersRef.current[serverName] within resolveRuntime).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e72771f-2bb7-45dc-beec-7571e8ced29c
📒 Files selected for processing (1)
mcpjam-inspector/client/src/hooks/use-server-state.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83b0fcf23e
ℹ️ 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".
| (response.error.code === "no_active_client" || | ||
| response.error.code === "unsupported_in_mode" || | ||
| response.error.code === "disconnected_server"); |
There was a problem hiding this comment.
Keep unsupported_in_mode in the retryable set here. The browser client registers Inspector command handlers asynchronously in App.tsx (via registerInspectorCommandHandler(...) inside an effect), so the first command after tools call --ui --open can briefly return unsupported_in_mode before the app finishes bootstrapping. After this change, that transient response is treated as final, so rendering is skipped permanently even though retrying a few hundred milliseconds later would succeed.
Useful? React with 👍 / 👎.
chelojimenez
temporarily deployed
to
preview-pr-1981
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 911affc03e
ℹ️ 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".
| let convexResult: string | undefined; | ||
| try { | ||
| convexResult = await syncServerToConvex(serverName, runtime); | ||
| } catch (syncError) { |
There was a problem hiding this comment.
persistRuntimeServerToWorkspaceIfNeeded checks for a same-name saved server before writing, but then delegates to syncServerToConvex, which performs an upsert and will updateServer if a matching name appears in activeWorkspaceServersFlatRef just after that pre-check. In that race (e.g., another tab/client creates the server while this call is in flight), this path can overwrite an existing workspace server even though this flow is intended to skip on collisions ("skipped_existing_name"). Add a final collision guard in the mutation path (or a create-only persist API) so late-arriving matches cannot be updated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9519d06d32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const latestWorkspaceId = effectiveActiveWorkspaceIdRef.current; | ||
| if ( | ||
| latestUseLocalFallback || | ||
| !latestIsAuthenticated || | ||
| !latestWorkspaceId || |
There was a problem hiding this comment.
persistRuntimeServerToWorkspaceIfNeeded waits for and freezes a target workspaceId, but syncServerToConvex re-reads effectiveActiveWorkspaceIdRef.current at write time. If the active workspace changes after the pre-write guard in persistRuntimeServerToWorkspaceIfNeeded but before convexCreateServer/convexUpdateServer runs, this can write the runtime server into the new workspace instead of the one the flow validated, causing cross-workspace persistence drift during navigation races.
Useful? React with 👍 / 👎.
1 participant
Note
Medium Risk
Touches CLI exit-code/error semantics and Inspector client auth header attachment, plus introduces asynchronous persistence of runtime server config to Convex; regressions could change automation behavior or cause unintended writes if edge cases slip through.
Overview
Improves
tools call --uioutcome handling by centralizing debug outcome errors, refining skipped-vs-error classification, and making--attach-onlytreat missing Inspector browser clients (no_active_client) as a hard error rather than a skippable warning; adds coverage and updates docs for the new behavior and flag constraints.Hardens Inspector client behavior by (1) limiting local session-token headers to loopback
/api/*requests (avoiding cross-origin leakage) and (2) waiting briefly for Inspector command handlers to register before returningunsupported_in_mode.Adds a new Inspector client workflow to persist runtime-only, connected servers into the active workspace (with deduping, readiness/echo waits, and collision avoidance), and updates workspace state merging so eligible runtime-only server projections appear in the matched remote workspace; includes substantial new test coverage for these cases and hosted chat session resets when hosted scope changes.
Reviewed by Cursor Bugbot for commit 9519d06. Bugbot is set up for automated code reviews on this repo. Configure here.