diff --git a/.claude/skills/local-dev/SKILL.md b/.claude/skills/local-dev/SKILL.md index c21df8b06d..0787c57d64 100644 --- a/.claude/skills/local-dev/SKILL.md +++ b/.claude/skills/local-dev/SKILL.md @@ -12,7 +12,7 @@ running and where a package goes. ## Install -Requires Node `>=22.19.0`. +Requires Node `^22.23.4 || ^24.22.1 || >=26.12.1`. ```sh npm install # at the REPO ROOT diff --git a/.github/ISSUE_TEMPLATE/1-bug_report.yml b/.github/ISSUE_TEMPLATE/1-bug_report.yml index e46c789064..fa6b804c2f 100644 --- a/.github/ISSUE_TEMPLATE/1-bug_report.yml +++ b/.github/ISSUE_TEMPLATE/1-bug_report.yml @@ -59,9 +59,9 @@ body: attributes: label: Node version description: > - Output of `node --version`. The current v2 line requires Node >= - 22.19.0; the deprecated v1 line has its own, older floor. - placeholder: "v22.19.0" + Output of `node --version`. The current v2 line requires Node ^22.23.4, + ^24.22.1 or >=26.12.1; the deprecated v1 line has its own, older floor. + placeholder: "v22.23.4" validations: required: true diff --git a/AGENTS.md b/AGENTS.md index 85ac58b11c..42ffc3fe5d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -88,7 +88,7 @@ silently. For the fuller map (what each `core/` area owns, what each v2 is **not** an npm workspace — each client under `clients/*` keeps its own `package.json` and `node_modules`. A single `npm install` at the **repo root** is still all you need: the root `postinstall` cascades into every client. Node -`>=22.19.0`. +`^22.23.4 || ^24.22.1 || >=26.12.1`. ```sh npm install # repo root diff --git a/Dockerfile b/Dockerfile index 2e3b405b35..5ceba71414 100644 --- a/Dockerfile +++ b/Dockerfile @@ -58,10 +58,12 @@ RUN mkdir -p /home/node/.mcp-inspector && chown -R node:node /home/node/.mcp-ins USER node WORKDIR /home/node -# Report readiness by probing the served SPA (`/` needs no auth). The probe -# connects to the address derived from the same `HOST` the server binds (a -# wildcard maps to loopback), so overriding `HOST` to one interface keeps the -# healthcheck valid (#2424) — see the script's header. Only `--web` has a +# Report readiness by probing the unauthenticated `/healthz` route (#2438). +# The probe connects to the address derived from the same `HOST` the server +# binds (a wildcard maps to loopback), so overriding `HOST` to one interface +# keeps the healthcheck valid (#2424), and uses https when both +# `INSPECTOR_TLS_*` paths are set, without verifying the certificate (#2825) +# — see the script's header. Only `--web` has a # server to probe, so the script reads the launch mode from PID 1's argv and # reports a `--cli`/`--tui` container healthy while it runs (#2415). COPY --from=builder /build/scripts/docker-healthcheck.mjs /usr/local/lib/mcp-inspector-healthcheck.mjs diff --git a/README.md b/README.md index 78c73eeffd..7f5c529411 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ npx -p @modelcontextprotocol/inspector mcpdo --help # mcpdo ## Quick start (development) -Requires Node `>=22.19.0`. +Requires Node `^22.23.4 || ^24.22.1 || >=26.12.1`: the patched release on each supported line. ```bash npm install # at the repo root; postinstall cascades into every client diff --git a/clients/cli/README.md b/clients/cli/README.md index 807b9881e9..46737fd048 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -94,9 +94,9 @@ Connections to remote HTTP/SSE servers honor the conventional proxy environment OAuth requests go through the destination guard rather than the plain proxied fetch: it reads the same variables, refuses link-local and cloud-metadata targets, and pins each direct connection to the addresses it checked ([#2749](https://github.com/modelcontextprotocol/inspector/issues/2749)). What it can and cannot promise behind a proxy is in [Outbound fetch destination guard](../web/README.md#outbound-fetch-destination-guard). -Proxy routing is powered by the [`undici`](https://www.npmjs.com/package/undici) package (declared in the **root** manifest only; `^8.x` requires Node `>= 22.19.0`, the Inspector's supported floor). It is imported lazily on the first request and, for MCP traffic, only when a proxy variable is set, so MCP connections without a proxy configured pay no cost. OAuth requests load it either way, for the destination guard. +Proxy routing is powered by the [`undici`](https://www.npmjs.com/package/undici) package (declared in the **root** manifest only; `^8.x` requires Node `>= 22.19.0`, below the Inspector's supported floor of `^22.23.4 || ^24.22.1 || >=26.12.1`). It is imported lazily on the first request and, for MCP traffic, only when a proxy variable is set, so MCP connections without a proxy configured pay no cost. OAuth requests load it either way, for the destination guard. -**Both halves of the pair come from userland undici — its `fetch` _and_ its `EnvHttpProxyAgent` ([#2067](https://github.com/modelcontextprotocol/inspector/issues/2067)).** The obvious-looking alternative is to hand the agent to Node's built-in `fetch` as a `dispatcher`, which is what v2.0.0–2.3.0 did. That couples two _different copies_ of undici at the dispatcher handler interface, and that interface is not stable across majors: Node 22 embeds undici 6, Node 24 embeds 7, Node 26 embeds 8. A userland undici 8 agent handed a built-in undici 7 handler is rejected outright — `fetch failed: invalid onRequestStart method` — and the request never leaves the process. Keeping both sides inside one copy is what makes proxying work unchanged from the Node 22.19 floor through Node 26. (Node's own `NODE_USE_ENV_PROXY` is not a substitute: it is unsupported at our 22.19 engine floor.) +**Both halves of the pair come from userland undici — its `fetch` _and_ its `EnvHttpProxyAgent` ([#2067](https://github.com/modelcontextprotocol/inspector/issues/2067)).** The obvious-looking alternative is to hand the agent to Node's built-in `fetch` as a `dispatcher`, which is what v2.0.0–2.3.0 did. That couples two _different copies_ of undici at the dispatcher handler interface, and that interface is not stable across majors: Node 22 embeds undici 6, Node 24 embeds 7, Node 26 embeds 8. A userland undici 8 agent handed a built-in undici 7 handler is rejected outright — `fetch failed: invalid onRequestStart method` — and the request never leaves the process. Keeping both sides inside one copy is what makes proxying work unchanged on every line the floor admits, Node 22 through Node 26. (Node's own `NODE_USE_ENV_PROXY` is not a substitute: it exists at our floor, added in 22.21.0, but it is opt-in at process start and still experimental, so a user who only exports `HTTPS_PROXY` would not get it.) Because undici's `Response` is a different class from `globalThis.Response`, the proxied fetch re-wraps each response as a genuine global `Response`, streaming preserved. Without that, `res instanceof Response` is `false` for callers that test it — including the MCP SDK, whose OAuth error formatter would degrade every message to `Raw body: [object Response]`. @@ -162,7 +162,7 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--load-requests ` | With `--load`: stop after starting this many calls. With `--load-duration` too, whichever is reached first ends the run. | | `--load-rate ` | With `--load`: start at most this many calls per second across all workers, spaced evenly (default: unlimited). | | `--load-max-error-rate ` | With `--load`: after printing the summary, exit `10` (`load_error_rate_exceeded`) when the share of failed calls is above this value. `0` to `1`; `0` fails on any failed call. See [Thresholds for CI](#thresholds-for-ci). | -| `--load-max-p99 ` | With `--load`: after printing the summary, exit `10` (`load_p99_exceeded`) when the p99 latency is above this many milliseconds, or when no call completed. Positive number. | +| `--load-max-p99 ` | With `--load`: after printing the summary, exit `10` (`load_p99_exceeded`) when the p99 latency of the successful calls is above this many milliseconds, or when no call succeeded. Positive number. | | `--relogin` | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. Requires an HTTP/SSE URL (rejected for stdio). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth` / catalog short-circuits. | | `--no-revoke` | With `--relogin`, skip the [RFC 7009](https://datatracker.ietf.org/doc/html/rfc7009) revocation request that would otherwise end the grant at the authorization server when the local state is deleted. The per-server `oauth.revokeOnClear` setting is the persistent form of the same opt-out; either one is enough to skip it. See [Revoking on `--relogin`](#revoking-on---relogin). | | `--stored-auth-only` | **CI / non-interactive safe:** never start interactive OAuth / step-up (and never auto-open a browser); use the shared store if present, otherwise fail immediately with `auth_required`. Prefer this over a bare pipe/CI run that would otherwise attempt interactive login. | @@ -331,8 +331,11 @@ is printed on stdout (wrapped in `{ "result": … }` with `--format json`): "errorRate": 0.0006, "elapsedMs": 30012.4, "throughput": 1373.12, - "latencyMs": { "min": 2.1, "mean": 14.5, "p50": 12.9, "p90": 22.4, "p99": 61.3, "max": 30004.8 }, - "errorMessages": [{ "message": "Request timed out", "count": 23 }] + "okThroughput": 1372.35, + "latencyMs": { "min": 2.1, "mean": 14.5, "p50": 12.9, "p90": 22.4, "p99": 61.3, "max": 212.8 }, + "errorLatencyMs": { "min": 30001.2, "mean": 30003.1, "p50": 30002.9, "p90": 30004.1, "p99": 30004.8, "max": 30004.8 }, + "errorMessages": [{ "message": "Request timed out", "count": 23 }], + "toolErrorMessages": [] } ``` @@ -342,12 +345,25 @@ is printed on stdout (wrapped in `{ "result": … }` with `--format json`): together over `requests`, rounded to four places. - **`errorMessages`** groups the thrown messages by frequency. The first ten distinct messages are kept; any further ones are counted under `(other)`. + **`toolErrorMessages`** does the same for the `isError` results, using the + first text block of each, trimmed and cut to 200 characters (`(no text + content)` when a result has none), so a run that is all `rate limited` + says so ([#2836](https://github.com/modelcontextprotocol/inspector/issues/2836)). - **Latency** is per call, client-side, in milliseconds, with nearest-rank - percentiles. It includes everything the Inspector does around the request, - which is negligible next to a network round trip but not next to a + percentiles. **`latencyMs` covers the successful (`ok`) calls only**, and + **`errorLatencyMs`** the failed ones (tool errors and thrown alike), so a + failure that comes back instantly never makes a struggling server look + faster; either is `null` when it has no calls. `throughput` counts every + completed call per second, **`okThroughput`** only the successful ones. + Latency includes everything the Inspector does around the request, which + is negligible next to a network round trip but not next to a sub-millisecond stdio echo. - **Bounds.** `--load-duration` stops *starting* calls; anything in flight is allowed to finish, so `elapsedMs` can exceed the window by up to one call. + A run that reaches its `--load-duration` is measured to the end of that + window at least, so a `--load-rate 1` run over five seconds reports a + throughput of 1/s rather than the 1.25/s its last start would suggest + ([#2837](https://github.com/modelcontextprotocol/inspector/issues/2837)). With `--load-rate`, starts follow an even `1/rate` grid; a timer that wakes a little late keeps the grid, so the average rate holds. A slot missed entirely — every worker busy, or a stalled event loop — is dropped rather @@ -355,7 +371,42 @@ is printed on stdout (wrapped in `{ "result": … }` with `--format json`): slot that falls past the window is never started. Node's timers resolve to about a millisecond, so a rate in the high hundreds per second or above is reached only approximately (1000/s measured ~700 against a local stdio - server); compare `requests` against what you asked for. + server). +- **`plannedStarts`, `droppedSlots`, `achievedRate`** appear only with + `--load-rate` ([#2837](https://github.com/modelcontextprotocol/inspector/issues/2837)). + `plannedStarts` is what the `1/rate` grid called for over the window the run + was starting calls in (the whole `--load-duration` for a run that reached + it, or up to the last start of a run that `--load-requests` ended); + `droppedSlots` is how many of those were + never started because every worker was busy; `achievedRate` is the calls + started per second over that window. Dropping slots keeps a stall from + turning into a burst, but it also makes the run closed-loop: a server too + slow for the target rate reports healthy latency while most of the planned + load never ran. So when fewer than 90% of `plannedStarts` were started, a + warning naming the shortfall is printed on stderr after the summary + (dropped by `--quiet`). Raise `--load-concurrency` or lower the rate. +- **The run stops early when the server is clearly gone** + ([#2835](https://github.com/modelcontextprotocol/inspector/issues/2835)). + A refused connection fails each call in microseconds, so a server that dies + mid-run would otherwise turn the rest of the window into thousands of + instant failures. Once 50 calls in a row have thrown — or calls have done + nothing but throw for a second, at least 5 of them — no further call + starts. Only thrown calls count; a tool error is the server answering, and + any other outcome resets the streak. The summary covers what completed and + carries **`stoppedEarly`**: `{ "reason": "consecutive_errors", "detail": + "…" }`, with a warning on stderr. The exit code is unchanged — `0` unless a + threshold is breached. +- **Ctrl+C or SIGTERM ends the run, not the summary** + ([#2834](https://github.com/modelcontextprotocol/inspector/issues/2834)). + The first signal stops new calls from starting and lets the in-flight ones + finish; the summary of what completed is then printed with + `"stoppedEarly": { "reason": "interrupted", "detail": "received SIGINT" }`, + and the CLI exits `130` (`interrupted`) without checking any threshold. A + second Ctrl+C exits `130` immediately, for in-flight calls that will not + settle. A repeat of the same signal within half a second of the first is + taken as a re-raise rather than a second press — a dependency of the CLI + (`when-exit`) re-sends the signal to the process after its own cleanup — so + the press that exits at once is one made after that. - **`connections`** appears only with `--load-connections` above `1` (see below), so a default run's summary is unchanged. @@ -411,7 +462,10 @@ the run with exit `5` (`tool_not_found`) instead of reporting a 100% error rate. Otherwise a completed run exits `0` whatever its error rate — the summary is the result — unless a threshold below is set. `--load` is rejected with any other method and with `--app-info`, `--output`, `--output-format`, `--list-stored-auth` and `--print-handoff`, and each `--load-*` flag is -rejected without `--load`. +rejected without `--load`. Each of those refusals — and `--load-connections` +above `--load-concurrency`, or above `1` over stdio — exits `1` with the +envelope `code` `load_flag_conflict`, the code `mcpdo` uses for the same +mistakes. ##### Thresholds for CI @@ -434,8 +488,10 @@ mcp-inspector --cli --method tools/call --tool-name search \ share of failed calls (`toolErrors` plus `errors`, over `requests`), not the four-place `errorRate`, so `--load-max-error-rate 0` fails even one failure that rounds away in the summary. -- With `--load-max-p99`, a run in which no call completed has no p99 to - compare, and fails rather than passing a latency check it never measured. +- `--load-max-p99` judges the **successful** calls (`latencyMs`), so a run + in which half the calls fail instantly cannot pass on the strength of its + failures. A run in which no call succeeded has no p99 to compare, and fails + rather than passing a latency check it never measured. - With no threshold set, nothing changes: a completed run exits `0`. #### Schema portability (`--strict`) @@ -524,7 +580,7 @@ The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:627 1. Starts the callback listener on `--callback-url` (or `MCP_OAUTH_CALLBACK_URL`) 2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** when allowed. Default: open on a **stderr** TTY (so `2>&1 | tee` prints a plain URL into the log but does not auto-open); plain URL only when stderr is non-TTY. `MCP_AUTO_OPEN_ENABLED=false` never opens; `=true` forces open even on a non-TTY (same as the web launcher). -3. Waits for the browser redirect, exchanges the code, and retries connect or the failed RPC +3. Waits for the browser redirect, exchanges the code, and retries connect or the failed RPC. Only a redirect echoing this flow's OAuth `state` is acted on — any other request to the listener, an `?error=` response included, gets a 400 and is ignored, so another local process or a web page cannot cancel the sign-in or choose the error it reports; the wait continues until the matching redirect or the 15-minute timeout Interactive OAuth (connect-time or mid-RPC) requires a TTY on **stdin or stderr** (so `2>&1 | tee` still works — stdin stays a TTY), or `MCP_AUTO_OPEN_ENABLED=true`. When neither stdin nor stderr is a TTY and that env is unset (typical CI), the CLI fails fast with `auth_required` instead of waiting up to 15 minutes on the loopback callback. Use **`--stored-auth-only`** for non-interactive runs that should only consume the shared store. @@ -720,6 +776,7 @@ prose from stderr: | `9` | `--verify --require-digests` found a skill that advertised no digests (`skills_unverifiable` — `resources: "dynamic"`). Only produced under `--require-digests`; without it such a skill is reported `unverifiable` and the run exits `0`. | | `10` | A `--load` run breached a threshold (`load_error_rate_exceeded` for `--load-max-error-rate`, `load_p99_exceeded` for `--load-max-p99`). The summary is still printed on stdout first. See [Thresholds for CI](#thresholds-for-ci). | | `11` | The server rate limited the request (`rate_limited` — HTTP 429). The envelope carries the server's retry timing; see below. | +| `130` | A `--load` run was interrupted by Ctrl+C or SIGTERM (`interrupted`). The partial summary is printed on stdout first. See [Load testing a tool](#load-testing-a-tool---load). | On any non-zero exit the CLI also writes a single JSON line to **stderr** — the `ErrorEnvelope`: diff --git a/clients/cli/__tests__/helpers/load-clock.ts b/clients/cli/__tests__/helpers/load-clock.ts new file mode 100644 index 0000000000..633bed6981 --- /dev/null +++ b/clients/cli/__tests__/helpers/load-clock.ts @@ -0,0 +1,50 @@ +import type { LoadClock } from "@inspector/core/cli/handlers/load-test.js"; + +/** + * A clock that only moves when told to, with a timer queue: each `sleep` + * wakes at an absolute time, and timers fire one at a time in wake order, + * each on its own macrotask so the woken worker runs (up to its next await) + * before the clock moves again — as real timers behave. A rate-limited run + * therefore completes instantly while keeping the schedule it would in real + * time. + */ +export function fakeClock( + options: { lateUntil?: number; jitter?: number } = {}, +): LoadClock & { advance: (ms: number) => void; sleeps: () => number } { + let t = 1000; + // A stalled event loop: no timer fires before this many ms into the run. + const earliestWake = 1000 + (options.lateUntil ?? 0); + const timers: { wakeAt: number; resolve: () => void }[] = []; + let pumping = false; + let sleepCount = 0; + const pump = (): void => { + timers.sort((a, b) => a.wakeAt - b.wakeAt); + const next = timers.shift(); + if (!next) { + pumping = false; + return; + } + t = Math.max(t, next.wakeAt); + next.resolve(); + setImmediate(pump); + }; + return { + now: () => t, + sleep: (ms) => + new Promise((resolve) => { + sleepCount++; + timers.push({ + wakeAt: Math.max(t + ms + (options.jitter ?? 0), earliestWake), + resolve, + }); + if (!pumping) { + pumping = true; + setImmediate(pump); + } + }), + advance: (ms) => { + t += ms; + }, + sleeps: () => sleepCount, + }; +} diff --git a/clients/cli/__tests__/load-interrupt-e2e.test.ts b/clients/cli/__tests__/load-interrupt-e2e.test.ts new file mode 100644 index 0000000000..afca26c23a --- /dev/null +++ b/clients/cli/__tests__/load-interrupt-e2e.test.ts @@ -0,0 +1,173 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { spawn, type ChildProcess } from "node:child_process"; +import { resolve, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; +import { + createTestServerHttp, + createTestServerInfo, +} from "@modelcontextprotocol/inspector-test-server"; +import { EXIT_CODES } from "@inspector/core/cli/error-handler.js"; +import { REPEAT_GRACE_MS } from "../src/handlers/load-interrupt.js"; + +const here = dirname(fileURLToPath(import.meta.url)); +const BIN = resolve(here, "../build/index.js"); + +/** + * Ctrl+C / SIGTERM delivered as a REAL signal to the BUILT CLI (#2834). + * + * The in-process suite (`load-summary.test.ts`) calls the CLI's own listener + * directly, which is exactly what let the first fix ship broken: `when-exit` + * (via `atomically`) also listens, and re-raises the signal with + * `process.kill(process.pid, …)` after its cleanups. That copy only exists + * when the kernel delivers a signal to a real process carrying the real + * dependency graph, so only a spawned binary can see it. + * + * Budgets: the child is killed after `CHILD_TIMEOUT_MS`, inside the test's own + * `E2E_SPAWN_MS`, so a hang names the CLI rather than timing out the test + * generically (the same arrangement as `e2e.test.ts`, #2323). + */ +const CHILD_TIMEOUT_MS = 15_000; +const E2E_SPAWN_MS = 25_000; + +type Run = { + child: ChildProcess; + done: Promise<{ code: number | null; stdout: string; stderr: string }>; + stderr: () => string; +}; + +function spawnLoad(url: string, tool: string): Run { + const child = spawn( + "node", + [ + BIN, + url, + "--transport", + "http", + "--method", + "tools/call", + "--tool-name", + tool, + "--load", + "--load-duration", + "60", + "--load-concurrency", + "2", + "--format", + "json", + ], + { stdio: ["ignore", "pipe", "pipe"] }, + ); + let stdout = ""; + let stderr = ""; + child.stdout!.on("data", (d) => (stdout += d.toString())); + child.stderr!.on("data", (d) => (stderr += d.toString())); + const done = new Promise<{ + code: number | null; + stdout: string; + stderr: string; + }>((resolveDone, reject) => { + const timer = setTimeout(() => { + child.kill("SIGKILL"); + reject(new Error(`built CLI did not exit within ${CHILD_TIMEOUT_MS}ms`)); + }, CHILD_TIMEOUT_MS); + child.on("error", (err) => { + clearTimeout(timer); + reject(err); + }); + child.on("close", (code) => { + clearTimeout(timer); + resolveDone({ code, stdout, stderr }); + }); + }); + return { child, done, stderr: () => stderr }; +} + +/** Resolve once `check` holds, polling every 50ms. */ +async function until(check: () => boolean): Promise { + while (!check()) await new Promise((r) => setTimeout(r, 50)); +} + +describe("--load under a real signal (built CLI)", () => { + let calls = 0; + let release: (() => void)[] = []; + const makeServer = () => + createTestServerHttp({ + serverInfo: createTestServerInfo("load-signal", "1.0.0"), + tools: [ + { + name: "slow", + description: "Answers after 50ms", + inputSchema: {}, + handler: async () => { + calls++; + await new Promise((r) => setTimeout(r, 50)); + return { content: [{ type: "text", text: "ok" }] }; + }, + }, + { + name: "hang", + description: "Answers only when the test lets it", + inputSchema: {}, + handler: async () => { + calls++; + await new Promise((r) => release.push(r)); + return { content: [{ type: "text", text: "late" }] }; + }, + }, + ], + }); + + let server: ReturnType; + + beforeEach(async () => { + calls = 0; + release = []; + server = makeServer(); + await server.start(); + }); + + afterEach(async () => { + for (const r of release) r(); + await server.stop(); + }); + + it.each(["SIGINT", "SIGTERM"] as const)( + "prints the partial summary and exits 130 on one %s", + async (signal) => { + const run = spawnLoad(server.url, "slow"); + await until(() => calls >= 10); + run.child.kill(signal); + const { code, stdout, stderr } = await run.done; + expect(code).toBe(EXIT_CODES.INTERRUPTED); + const { result } = JSON.parse(stdout.trim().split("\n")[0]!); + expect(result.stoppedEarly).toEqual({ + reason: "interrupted", + detail: `received ${signal}`, + }); + expect(result.ok).toBeGreaterThan(0); + expect(stderr).toContain(`Interrupted (${signal})`); + expect(JSON.parse(stderr.trim().split("\n").pop()!).error.code).toBe( + "interrupted", + ); + }, + E2E_SPAWN_MS, + ); + + it( + "exits 130 at once on a second Ctrl+C while calls will not settle", + async () => { + const run = spawnLoad(server.url, "hang"); + await until(() => calls >= 2); + run.child.kill("SIGINT"); + await until(() => run.stderr().includes("Interrupted (SIGINT)")); + // A person's second press, past the window that absorbs a re-raise. + await new Promise((r) => setTimeout(r, REPEAT_GRACE_MS + 200)); + run.child.kill("SIGINT"); + const { code, stdout } = await run.done; + expect(code).toBe(EXIT_CODES.INTERRUPTED); + // The in-flight calls never settled, so there was no summary to print. + expect(stdout).toBe(""); + }, + E2E_SPAWN_MS, + ); +}); diff --git a/clients/cli/__tests__/load-summary.test.ts b/clients/cli/__tests__/load-summary.test.ts new file mode 100644 index 0000000000..c19367d697 --- /dev/null +++ b/clients/cli/__tests__/load-summary.test.ts @@ -0,0 +1,900 @@ +import { describe, it, expect, vi, afterEach } from "vitest"; +import { runCli } from "./helpers/cli-runner.js"; +import { expectCliSuccess, expectValidJson } from "./helpers/assertions.js"; +import { fakeClock } from "./helpers/load-clock.js"; +import { + createTestServerHttp, + createTestServerInfo, + getTestMcpServerCommand, +} from "@modelcontextprotocol/inspector-test-server"; +import { + CIRCUIT_BREAKER_ERRORS, + CIRCUIT_BREAKER_MIN_ERRORS, + LOAD_FLAG_CONFLICT, + MAX_TOOL_ERROR_MESSAGE_LENGTH, + NO_TOOL_ERROR_TEXT, + loadRateShortfallWarning, + loadSummaryWarnings, + runLoad, + toolErrorText, + type LoadCallOutcome, + type LoadClock, + type LoadStop, + type LoadSummary, +} from "@inspector/core/cli/handlers/load-test.js"; +import { + CliExitCodeError, + EXIT_CODES, +} from "@inspector/core/cli/error-handler.js"; +import { + REPEAT_GRACE_MS, + assertNotInterrupted, + withLoadInterrupt, +} from "../src/handlers/load-interrupt.js"; + +/** + * The `--load` summary fixes of the v2.14.0 milestone: Ctrl+C keeps the + * partial summary (#2834), a circuit breaker for a dead server (#2835), + * success-only latency and tool-error messages (#2836), and the planned + * versus achieved figures of a `--load-rate` run (#2837). + */ + +/** + * Deliver a signal to the listener `withLoadInterrupt` added, and to nothing + * else: `process.emit` would also reach the test worker's own SIGINT/SIGTERM + * handlers, which end the worker. Returns false while no new listener exists. + */ +function signalLoadRun( + signal: "SIGINT" | "SIGTERM", + existing: NodeJS.SignalsListener[], +): boolean { + const added = process + .listeners(signal) + .find((listener) => !existing.includes(listener)); + if (!added) return false; + (added as (s: NodeJS.Signals) => void)(signal); + return true; +} + +describe("success and failure latency (#2836)", () => { + it("times successes apart from failures and counts ok throughput", async () => { + const clock = fakeClock(); + let n = 0; + // Every second call fails instantly; the successes take 200ms. + const call = async (): Promise => { + if (n++ % 2 === 1) return "tool_error"; + clock.advance(200); + return "ok"; + }; + const summary = await runLoad( + call, + { concurrency: 1, requests: 10 }, + clock, + ); + expect(summary).toMatchObject({ + ok: 5, + toolErrors: 5, + elapsedMs: 1000, + throughput: 10, + okThroughput: 5, + latencyMs: { min: 200, p50: 200, p99: 200 }, + errorLatencyMs: { min: 0, max: 0 }, + }); + }); + + it("reports no success latency when nothing succeeded", async () => { + const summary = await runLoad( + async () => "tool_error", + { concurrency: 1, requests: 3 }, + fakeClock(), + ); + expect(summary.latencyMs).toBeNull(); + expect(summary.errorLatencyMs).not.toBeNull(); + expect(summary.okThroughput).toBe(0); + }); + + it("tallies tool-error messages apart from thrown ones", async () => { + let n = 0; + const call = async (): Promise => { + const i = n++; + if (i < 3) return { outcome: "tool_error", message: "rate limited" }; + if (i === 3) return "tool_error"; + throw new Error("boom"); + }; + const summary = await runLoad( + call, + { concurrency: 1, requests: 5 }, + fakeClock(), + ); + expect(summary.toolErrorMessages).toEqual([ + { message: "rate limited", count: 3 }, + { message: NO_TOOL_ERROR_TEXT, count: 1 }, + ]); + expect(summary.errorMessages).toEqual([{ message: "boom", count: 1 }]); + }); + + describe("toolErrorText", () => { + it("takes the first non-empty text block, trimmed", () => { + expect( + toolErrorText([ + { type: "image", data: "", mimeType: "image/png" }, + null, + { type: "text", text: " " }, + { type: "text", text: " rate limited \n" }, + { type: "text", text: "second" }, + ]), + ).toBe("rate limited"); + }); + + it("caps a long text", () => { + const text = toolErrorText([{ type: "text", text: "x".repeat(500) }]); + expect(text).toBe(`${"x".repeat(MAX_TOOL_ERROR_MESSAGE_LENGTH)}…`); + }); + + it("falls back when there is no text to report", () => { + expect(toolErrorText([])).toBe(NO_TOOL_ERROR_TEXT); + expect(toolErrorText(undefined)).toBe(NO_TOOL_ERROR_TEXT); + expect(toolErrorText([{ type: "text", text: 7 }])).toBe( + NO_TOOL_ERROR_TEXT, + ); + }); + }); +}); + +describe("circuit breaker (#2835)", () => { + it(`stops after ${CIRCUIT_BREAKER_ERRORS} thrown calls in a row`, async () => { + const summary = await runLoad( + async () => { + throw new Error("fetch failed"); + }, + { concurrency: 1, requests: 1000 }, + fakeClock(), + ); + expect(summary.requests).toBe(CIRCUIT_BREAKER_ERRORS); + expect(summary.stoppedEarly).toEqual({ + reason: "consecutive_errors", + detail: `${CIRCUIT_BREAKER_ERRORS} calls in a row threw over 0ms; the server looks unavailable`, + }); + expect(summary.errorMessages).toEqual([ + { message: "fetch failed", count: CIRCUIT_BREAKER_ERRORS }, + ]); + }); + + it("stops a slow run after a second of nothing but thrown calls", async () => { + const clock = fakeClock(); + const summary = await runLoad( + async () => { + clock.advance(250); + throw new Error("timed out"); + }, + { concurrency: 1, durationMs: 60_000 }, + clock, + ); + // The first failure lands at 250ms; the fifth, at 1250ms, closes a 1s + // streak. + expect(summary.requests).toBe(CIRCUIT_BREAKER_MIN_ERRORS); + expect(summary.stoppedEarly?.reason).toBe("consecutive_errors"); + // Not measured to the end of a window it never ran to. + expect(summary.elapsedMs).toBe(1250); + }); + + it("does not call draining in-flight failures an early stop", async () => { + const clock = fakeClock(); + let release!: () => void; + const allStarted = new Promise((r) => (release = r)); + let starts = 0; + // All five calls start at once, then fail 300ms apart, well past the + // breaker's one-second window — but no further call could start anyway. + const summary = await runLoad( + async () => { + if (++starts === 5) release(); + await allStarted; + clock.advance(300); + throw new Error("timed out"); + }, + { concurrency: 5, requests: 5 }, + clock, + ); + expect(summary.errors).toBe(5); + expect(summary.stoppedEarly).toBeUndefined(); + }); + + it("lets an interrupt override a breaker stop while calls drain", async () => { + const abort = new AbortController(); + let releaseSlow!: () => void; + const slow = new Promise((r) => (releaseSlow = r)); + let thrown = 0; + const call = async (worker: number): Promise => { + if (worker === 0) { + await slow; + return "ok"; + } + if (++thrown === CIRCUIT_BREAKER_ERRORS) { + // The breaker has tripped; Ctrl+C arrives while worker 0 drains. + setImmediate(() => { + abort.abort(); + releaseSlow(); + }); + } + throw new Error("fetch failed"); + }; + const summary = await runLoad( + call, + { concurrency: 2, requests: 1000 }, + fakeClock(), + abort.signal, + ); + expect(summary.stoppedEarly?.reason).toBe("interrupted"); + }); + + it("resets on any answer from the server, tool errors included", async () => { + let n = 0; + const call = async (): Promise => { + if (n++ % CIRCUIT_BREAKER_ERRORS === 0) return "tool_error"; + throw new Error("flaky"); + }; + const summary = await runLoad( + call, + { concurrency: 1, requests: 500 }, + fakeClock(), + ); + expect(summary.requests).toBe(500); + expect(summary.stoppedEarly).toBeUndefined(); + }); +}); + +describe("early stops (#2834)", () => { + it("reports an abort as interrupted by default", async () => { + const abort = new AbortController(); + let calls = 0; + const summary = await runLoad( + async () => { + if (++calls === 2) abort.abort(); + return "ok"; + }, + { concurrency: 1, requests: 10 }, + fakeClock(), + abort.signal, + ); + expect(summary.requests).toBe(2); + expect(summary.stoppedEarly).toEqual({ + reason: "interrupted", + detail: "the run was interrupted", + }); + }); + + it("keeps the reason the caller aborted with", async () => { + const stop: LoadStop = { + reason: "connection_lost", + detail: "the connection went error", + }; + const summary = await runLoad( + async () => "ok", + { concurrency: 1, requests: 10 }, + fakeClock(), + AbortSignal.abort(stop), + ); + expect(summary.stoppedEarly).toEqual(stop); + }); + + it.each([ + [{ reason: "bogus", detail: "x" }], + [{ reason: "interrupted" }], + ["a string"], + [null], + ])("does not trust a malformed reason %j", async (reason) => { + const summary = await runLoad( + async () => "ok", + { concurrency: 1, requests: 1 }, + fakeClock(), + AbortSignal.abort(reason), + ); + expect(summary.stoppedEarly?.reason).toBe("interrupted"); + }); +}); + +describe("planned and achieved rate (#2837)", () => { + it("reports the planned starts a saturated run dropped", async () => { + const clock = fakeClock(); + // Capacity is 10/s (one worker, 100ms calls) against a 100/s target. + const summary = await runLoad( + async () => { + clock.advance(100); + return "ok"; + }, + { concurrency: 1, durationMs: 1000, rate: 100 }, + clock, + ); + expect(summary).toMatchObject({ + rate: 100, + requests: 10, + plannedStarts: 100, + droppedSlots: 90, + achievedRate: 10, + }); + expect(loadRateShortfallWarning(summary)).toBe( + "Warning: --load-rate 100/s was not reached: 10 of 100 planned calls started (10/s); 90 slots were dropped because every worker was busy. Raise --load-concurrency or lower --load-rate.", + ); + }); + + it("measures a duration-bound run to the end of its window", async () => { + const summary = await runLoad( + async () => "ok", + { concurrency: 1, durationMs: 5000, rate: 1 }, + fakeClock(), + ); + expect(summary).toMatchObject({ + requests: 5, + elapsedMs: 5000, + throughput: 1, + plannedStarts: 5, + droppedSlots: 0, + achievedRate: 1, + }); + expect(loadRateShortfallWarning(summary)).toBeUndefined(); + }); + + it("plans a request-bound run up to its last start", async () => { + const summary = await runLoad( + async () => "ok", + { concurrency: 1, requests: 3, durationMs: 10_000, rate: 4 }, + fakeClock(), + ); + // Starts at 0, 250 and 500ms cover the grid to 750ms: exactly 4/s. + expect(summary).toMatchObject({ + plannedStarts: 3, + droppedSlots: 0, + achievedRate: 4, + }); + }); + + it("counts the slots a saturated request-bound run missed", async () => { + const clock = fakeClock(); + const summary = await runLoad( + async () => { + clock.advance(500); + return "ok"; + }, + { concurrency: 1, requests: 3, rate: 10 }, + clock, + ); + // Starts at 0, 500 and 1000ms: the grid to 1100ms held 11 slots. + expect(summary).toMatchObject({ + requests: 3, + plannedStarts: 11, + droppedSlots: 8, + }); + expect(loadRateShortfallWarning(summary)).toContain("3 of 11 planned"); + }); + + it("does not count a slightly late final timer as a dropped slot", async () => { + const summary = await runLoad( + async () => "ok", + { concurrency: 1, requests: 3, rate: 10 }, + fakeClock({ jitter: 0.1 }), + ); + // Starts at 0, 100.1 and 200.1ms keep the grid; the window ends at 300. + expect(summary).toMatchObject({ plannedStarts: 3, droppedSlots: 0 }); + expect(loadRateShortfallWarning(summary)).toBeUndefined(); + }); + + it("ends the window at the request bound when an interrupt lands later", async () => { + const clock = fakeClock(); + const abort = new AbortController(); + const summary = await runLoad( + async () => { + clock.advance(5000); + abort.abort(); + return "ok"; + }, + { concurrency: 1, requests: 1, rate: 1 }, + clock, + abort.signal, + ); + expect(summary.stoppedEarly?.reason).toBe("interrupted"); + expect(summary).toMatchObject({ plannedStarts: 1, droppedSlots: 0 }); + }); + + it("ends the window at the deadline when an interrupt lands later", async () => { + const clock = fakeClock(); + const abort = new AbortController(); + let calls = 0; + const summary = await runLoad( + async () => { + // The tenth call, started at 900ms, outlives the 1s window. + if (++calls === 10) { + clock.advance(4000); + abort.abort(); + } + return "ok"; + }, + { concurrency: 1, durationMs: 1000, rate: 10 }, + clock, + abort.signal, + ); + expect(summary.stoppedEarly?.reason).toBe("interrupted"); + expect(summary).toMatchObject({ plannedStarts: 10, droppedSlots: 0 }); + }); + + it("does not count a slot claimed after the run stopped as started", async () => { + // A clock that moves 1ms per read, so at 1000/s every claim finds its + // slot already due and never waits on a timer. + let t = 0; + const clock: LoadClock = { now: () => (t += 1), sleep: async () => {} }; + const abort = new AbortController(); + let calls = 0; + const summary = await runLoad( + async () => { + calls++; + // Worker 1 has claimed its slot by now; this abort lands before it + // resumes, so its call never runs. + abort.abort(); + return "ok"; + }, + { concurrency: 2, requests: 10, rate: 1000 }, + clock, + abort.signal, + ); + expect(calls).toBe(1); + expect(summary.plannedStarts! - summary.droppedSlots!).toBe(1); + }); + + it("does not warn on a fractional rate it held", async () => { + const summary = await runLoad( + async () => "ok", + { concurrency: 1, durationMs: 70_000, rate: 0.014 }, + fakeClock(), + ); + expect(summary).toMatchObject({ plannedStarts: 1, droppedSlots: 0 }); + expect(loadRateShortfallWarning(summary)).toBeUndefined(); + }); + + it("plans an interrupted run up to the moment it stopped", async () => { + const clock = fakeClock(); + const abort = new AbortController(); + let calls = 0; + const summary = await runLoad( + async () => { + if (++calls === 3) { + clock.advance(250); + abort.abort(); + } + return "ok"; + }, + { concurrency: 1, durationMs: 10_000, rate: 10 }, + clock, + abort.signal, + ); + // Stopped at 450ms: five slots were due, three were started. + expect(summary).toMatchObject({ + requests: 3, + plannedStarts: 5, + droppedSlots: 2, + }); + }); + + it("reports no achieved rate for a window of zero", async () => { + const summary = await runLoad( + async () => "ok", + { concurrency: 1, durationMs: 0, rate: 10 }, + fakeClock(), + ); + expect(summary).toMatchObject({ + requests: 0, + plannedStarts: 0, + droppedSlots: 0, + achievedRate: 0, + }); + expect(loadRateShortfallWarning(summary)).toBeUndefined(); + }); + + it("leaves the rate figures out of a run with no rate", async () => { + const summary = await runLoad( + async () => "ok", + { concurrency: 1, requests: 2 }, + fakeClock(), + ); + expect(summary).not.toHaveProperty("plannedStarts"); + expect(summary).not.toHaveProperty("droppedSlots"); + expect(summary).not.toHaveProperty("achievedRate"); + expect(loadRateShortfallWarning(summary)).toBeUndefined(); + }); +}); + +describe("loadSummaryWarnings", () => { + const base: LoadSummary = { + concurrency: 1, + requests: 1, + ok: 1, + toolErrors: 0, + errors: 0, + errorRate: 0, + elapsedMs: 1, + throughput: 1, + okThroughput: 1, + latencyMs: null, + errorLatencyMs: null, + errorMessages: [], + toolErrorMessages: [], + }; + + it("is empty for a run that held its bounds", () => { + expect(loadSummaryWarnings(base)).toEqual([]); + }); + + it("names an early stop the caller did not ask for, but not an interrupt", () => { + expect( + loadSummaryWarnings({ + ...base, + stoppedEarly: { reason: "consecutive_errors", detail: "50 in a row" }, + }), + ).toEqual([ + "Warning: the load run stopped early (consecutive_errors): 50 in a row.", + ]); + expect( + loadSummaryWarnings({ + ...base, + stoppedEarly: { reason: "interrupted", detail: "received SIGINT" }, + }), + ).toEqual([]); + }); + + it("adds the rate shortfall", () => { + const warnings = loadSummaryWarnings({ + ...base, + rate: 10, + plannedStarts: 10, + droppedSlots: 5, + achievedRate: 5, + }); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("--load-rate 10/s was not reached"); + }); +}); + +describe("withLoadInterrupt (#2834)", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + /** Run until aborted, then settle with the abort's reason. */ + function untilAborted(signal: AbortSignal): Promise { + return new Promise((resolve) => + signal.addEventListener("abort", () => resolve(signal.reason)), + ); + } + + it("turns the first signal into an abort and says so on stderr", async () => { + const stderr = vi + .spyOn(process.stderr, "write") + .mockImplementation(() => true); + const existing = process.listeners("SIGTERM"); + const before = existing.length; + const run = withLoadInterrupt(untilAborted); + expect(process.listenerCount("SIGTERM")).toBe(before + 1); + expect(signalLoadRun("SIGTERM", existing)).toBe(true); + await expect(run).resolves.toEqual({ + reason: "interrupted", + detail: "received SIGTERM", + }); + expect(stderr).toHaveBeenCalledWith( + expect.stringContaining("Interrupted (SIGTERM)"), + ); + expect(process.listenerCount("SIGTERM")).toBe(before); + }); + + it("exits 130 on a second press, after the re-raise window", async () => { + const exit = vi.fn(); + const existing = process.listeners("SIGINT"); + let t = 0; + let finish!: () => void; + const run = withLoadInterrupt( + () => new Promise((resolve) => (finish = resolve)), + { quiet: true, exit, now: () => t }, + ); + signalLoadRun("SIGINT", existing); + // `when-exit` re-raises the first signal a moment later: not a press. + t = 5; + signalLoadRun("SIGINT", existing); + t = REPEAT_GRACE_MS - 1; + signalLoadRun("SIGINT", existing); + expect(exit).not.toHaveBeenCalled(); + t = REPEAT_GRACE_MS; + signalLoadRun("SIGINT", existing); + expect(exit).toHaveBeenCalledWith(EXIT_CODES.INTERRUPTED); + finish(); + await run; + }); + + it("exits at once on a different signal, inside the window too", async () => { + const exit = vi.fn(); + const ints = process.listeners("SIGINT"); + const terms = process.listeners("SIGTERM"); + let finish!: () => void; + const run = withLoadInterrupt( + () => new Promise((resolve) => (finish = resolve)), + { quiet: true, exit, now: () => 0 }, + ); + signalLoadRun("SIGINT", ints); + signalLoadRun("SIGTERM", terms); + expect(exit).toHaveBeenCalledWith(EXIT_CODES.INTERRUPTED); + finish(); + await run; + }); + + it("exits the real process by default on the second signal", async () => { + const exit = vi + .spyOn(process, "exit") + .mockImplementation((() => undefined) as never); + const ints = process.listeners("SIGINT"); + const terms = process.listeners("SIGTERM"); + let finish!: () => void; + const run = withLoadInterrupt( + () => new Promise((resolve) => (finish = resolve)), + { quiet: true }, + ); + // Different signals, so the real clock's re-raise window never applies. + signalLoadRun("SIGINT", ints); + signalLoadRun("SIGTERM", terms); + expect(exit).toHaveBeenCalledWith(130); + finish(); + await run; + }); + + it("removes its listeners when the run throws", async () => { + const before = process.listenerCount("SIGINT"); + await expect( + withLoadInterrupt(async () => { + throw new Error("nope"); + }), + ).rejects.toThrow("nope"); + expect(process.listenerCount("SIGINT")).toBe(before); + }); + + it("raises exit 130 only for an interrupted summary", () => { + const summary = { + requests: 7, + stoppedEarly: { reason: "interrupted", detail: "received SIGINT" }, + } as LoadSummary; + let err: unknown; + try { + assertNotInterrupted(summary); + } catch (e) { + err = e; + } + expect(err).toBeInstanceOf(CliExitCodeError); + expect(err).toMatchObject({ + exitCode: EXIT_CODES.INTERRUPTED, + envelope: { code: "interrupted" }, + }); + expect(() => + assertNotInterrupted({ + ...summary, + stoppedEarly: { reason: "consecutive_errors", detail: "x" }, + }), + ).not.toThrow(); + expect(() => + assertNotInterrupted({ ...summary, stoppedEarly: undefined }), + ).not.toThrow(); + }); +}); + +describe("--load summary fixes (CLI)", () => { + const { command, args } = getTestMcpServerCommand(); + + it("prints the partial summary on Ctrl+C and exits 130 (#2834)", async () => { + const existing = process.listeners("SIGINT"); + const baseline = existing.length; + const run = runCli( + [ + command, + ...args, + "--cli", + "--method", + "tools/call", + "--tool-name", + "echo", + "--tool-arg", + "message=hi", + "--load", + "--load-duration", + "30", + "--load-concurrency", + "2", + "--format", + "json", + ], + { timeout: 20_000 }, + ); + // Fired once the run is under way, so some calls have completed. + await vi.waitFor( + () => expect(signalLoadRun("SIGINT", existing)).toBe(true), + { timeout: 10_000, interval: 200 }, + ); + const result = await run; + expect(result.exitCode).toBe(EXIT_CODES.INTERRUPTED); + const lines = result.stdout.trim().split("\n"); + const { result: summary } = JSON.parse(lines[0]!); + expect(summary.stoppedEarly).toEqual({ + reason: "interrupted", + detail: "received SIGINT", + }); + expect(summary.requests).toBe(summary.ok); + expect(result.stderr).toContain("Interrupted (SIGINT)"); + const envelope = JSON.parse(result.stderr.trim().split("\n").pop()!); + expect(envelope.error.code).toBe("interrupted"); + expect(process.listenerCount("SIGINT")).toBe(baseline); + }); + + it.each([ + [["--load-duration", "1"]], + [["--load", "--app-info"]], + [["--load", "--load-connections", "3", "--load-concurrency", "2"]], + [["--load", "--load-connections", "2"]], + ])( + "uses the load_flag_conflict envelope code for %j (#2834)", + async (flags) => { + const result = await runCli([ + command, + ...args, + "--cli", + "--method", + "tools/call", + "--tool-name", + "echo", + ...flags, + ]); + expect(result.exitCode).toBe(EXIT_CODES.USAGE); + const envelope = JSON.parse(result.stderr.trim().split("\n").pop()!); + expect(envelope.error.code).toBe(LOAD_FLAG_CONFLICT); + }, + ); + + describe("over HTTP", () => { + let n = 0; + const makeServer = () => + createTestServerHttp({ + serverInfo: createTestServerInfo("load-summary", "1.0.0"), + tools: [ + { + name: "unique", + description: "Always fails", + inputSchema: {}, + // The SDK answers a throwing handler with `isError: true`. + handler: async () => { + throw new Error(`rate limited`); + }, + }, + { + name: "slow", + description: "Answers after 50ms", + inputSchema: {}, + handler: async () => { + n++; + await new Promise((r) => setTimeout(r, 50)); + return { content: [{ type: "text", text: "ok" }] }; + }, + }, + ], + }); + + let server: ReturnType; + const http = (tool: string, ...flags: string[]) => [ + server.url, + "--cli", + "--transport", + "http", + "--method", + "tools/call", + "--tool-name", + tool, + "--load", + ...flags, + ]; + + it("tallies the text of isError results (#2836)", async () => { + server = makeServer(); + await server.start(); + try { + const result = await runCli( + http("unique", "--load-requests", "6", "--load-concurrency", "2"), + ); + expectCliSuccess(result); + const json = expectValidJson(result); + expect(json).toMatchObject({ toolErrors: 6, errorMessages: [] }); + expect(json.latencyMs).toBeNull(); + expect(json.toolErrorMessages).toHaveLength(1); + expect(json.toolErrorMessages[0].count).toBe(6); + expect(json.toolErrorMessages[0].message).toContain("rate limited"); + } finally { + await server.stop(); + } + }); + + it("fails --load-max-p99 when no call succeeded (#2836)", async () => { + server = makeServer(); + await server.start(); + try { + const result = await runCli( + http("unique", "--load-requests", "3", "--load-max-p99", "60000"), + ); + expect(result.exitCode).toBe(EXIT_CODES.LOAD_THRESHOLD_EXCEEDED); + expect(result.stderr).toContain("no call succeeded"); + } finally { + await server.stop(); + } + }); + + it("warns when a --load-rate run falls short (#2837)", async () => { + server = makeServer(); + await server.start(); + try { + const result = await runCli( + http( + "slow", + "--load-rate", + "100", + "--load-concurrency", + "1", + "--load-duration", + "0.5", + ), + ); + expectCliSuccess(result); + const json = expectValidJson(result); + expect(json.plannedStarts).toBe(50); + expect(json.droppedSlots).toBeGreaterThan(25); + expect(json.achievedRate).toBeLessThan(90); + expect(result.stderr).toContain("Warning: --load-rate 100/s"); + // `--quiet` drops the advisory, not the summary. + const quiet = await runCli( + http( + "slow", + "--load-rate", + "100", + "--load-concurrency", + "1", + "--load-duration", + "0.2", + "--quiet", + ), + ); + expectCliSuccess(quiet); + expect(quiet.stderr).not.toContain("Warning"); + } finally { + await server.stop(); + } + }); + + it("stops early when the server dies mid-run (#2835)", async () => { + server = makeServer(); + await server.start(); + n = 0; + const run = runCli( + http( + "slow", + "--load-duration", + "15", + "--load-concurrency", + "4", + "--format", + "json", + ), + { timeout: 20_000 }, + ); + await vi.waitFor(() => expect(n).toBeGreaterThan(4), { + timeout: 10_000, + }); + await server.stop(); + const result = await run; + expectCliSuccess(result); + const { result: summary } = expectValidJson(result); + expect(summary.stoppedEarly?.reason).toBe("consecutive_errors"); + expect(summary.errors).toBeGreaterThan(0); + expect(summary.elapsedMs).toBeLessThan(15_000); + expect(result.stderr).toContain( + "Warning: the load run stopped early (consecutive_errors)", + ); + }); + }); +}); diff --git a/clients/cli/__tests__/load-test.test.ts b/clients/cli/__tests__/load-test.test.ts index c2e84df776..9a338c71c8 100644 --- a/clients/cli/__tests__/load-test.test.ts +++ b/clients/cli/__tests__/load-test.test.ts @@ -28,55 +28,7 @@ import { type LoadCallOutcome, type LoadClock, } from "@inspector/core/cli/handlers/load-test.js"; - -/** - * A clock that only moves when told to, with a timer queue: each `sleep` - * wakes at an absolute time, and timers fire one at a time in wake order, - * each on its own macrotask so the woken worker runs (up to its next await) - * before the clock moves again — as real timers behave. A rate-limited run - * therefore completes instantly while keeping the schedule it would in real - * time. - */ -function fakeClock( - options: { lateUntil?: number; jitter?: number } = {}, -): LoadClock & { advance: (ms: number) => void; sleeps: () => number } { - let t = 1000; - // A stalled event loop: no timer fires before this many ms into the run. - const earliestWake = 1000 + (options.lateUntil ?? 0); - const timers: { wakeAt: number; resolve: () => void }[] = []; - let pumping = false; - let sleepCount = 0; - const pump = (): void => { - timers.sort((a, b) => a.wakeAt - b.wakeAt); - const next = timers.shift(); - if (!next) { - pumping = false; - return; - } - t = Math.max(t, next.wakeAt); - next.resolve(); - setImmediate(pump); - }; - return { - now: () => t, - sleep: (ms) => - new Promise((resolve) => { - sleepCount++; - timers.push({ - wakeAt: Math.max(t + ms + (options.jitter ?? 0), earliestWake), - resolve, - }); - if (!pumping) { - pumping = true; - setImmediate(pump); - } - }), - advance: (ms) => { - t += ms; - }, - sleeps: () => sleepCount, - }; -} +import { fakeClock } from "./helpers/load-clock.js"; describe("load-test engine", () => { describe("percentile / summarizeLatency", () => { @@ -472,10 +424,10 @@ describe("load-test engine", () => { // before the run has spent its whole request count. setImmediate(() => abort.abort()); const limit = 200_000; + // Tool errors, not throws: a run of thrown calls would trip the + // circuit breaker (#2835) long before the first forced yield. const summary = await runLoad( - async () => { - throw new Error("rejected up front"); - }, + async () => "tool_error", { concurrency: 4, requests: limit }, undefined, abort.signal, diff --git a/clients/cli/__tests__/load-thresholds.test.ts b/clients/cli/__tests__/load-thresholds.test.ts index d91233ec6c..46cc5e8eb3 100644 --- a/clients/cli/__tests__/load-thresholds.test.ts +++ b/clients/cli/__tests__/load-thresholds.test.ts @@ -33,8 +33,11 @@ function summary(overrides: Partial = {}): LoadSummary { errorRate: 0, elapsedMs: 100, throughput: 1000, + okThroughput: 1000, latencyMs: { min: 1, mean: 2, p50: 2, p90: 3, p99: 4, max: 5 }, + errorLatencyMs: null, errorMessages: [], + toolErrorMessages: [], ...overrides, }; } @@ -149,13 +152,13 @@ describe("load thresholds", () => { ); }); - it("fails a p99 threshold when no call completed, and says why", () => { + it("fails a p99 threshold when no call succeeded, and says why", () => { const err = judge(summary({ requests: 0, ok: 0, latencyMs: null }), { maxP99Ms: 100, }); expect(err?.envelope?.code).toBe(LOAD_P99_EXCEEDED); expect(err?.message).toContain( - "no call completed, so there is no p99 latency to check", + "no call succeeded, so there is no p99 latency to check", ); }); diff --git a/clients/cli/__tests__/oauth-interactive.test.ts b/clients/cli/__tests__/oauth-interactive.test.ts index b73a725cef..18e9afec72 100644 --- a/clients/cli/__tests__/oauth-interactive.test.ts +++ b/clients/cli/__tests__/oauth-interactive.test.ts @@ -39,7 +39,7 @@ const oauthTestStatePath = join( async function completeOAuthAuthorization( authorizationUrl: URL, -): Promise<{ code: string; iss?: string }> { +): Promise<{ code: string; iss?: string; state?: string }> { let response = await fetch(authorizationUrl.toString(), { redirect: "manual", }); @@ -73,14 +73,17 @@ async function completeOAuthAuthorization( throw new Error("Missing authorization code"); } const iss = redirect.searchParams.get("iss") ?? undefined; - return iss ? { code, iss } : { code }; + // The loopback listener only acts on a callback echoing the flow's state + // (#2847), exactly as a real browser redirect does. + const state = redirect.searchParams.get("state") ?? undefined; + return { code, ...(iss ? { iss } : {}), ...(state ? { state } : {}) }; } function createAutoCompleteNavigation( redirectUrlProvider: MutableRedirectUrlProvider, ) { return new CallbackNavigation(async (url) => { - const { code, iss } = await completeOAuthAuthorization(url); + const { code, iss, state } = await completeOAuthAuthorization(url); const redirect = redirectUrlProvider.redirectUrl; if (!redirect) { throw new Error("redirectUrl not set"); @@ -88,6 +91,7 @@ function createAutoCompleteNavigation( const callback = new URL(redirect); callback.searchParams.set("code", code); if (iss) callback.searchParams.set("iss", iss); + if (state) callback.searchParams.set("state", state); await fetch(callback.href); }); } diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 6a2abd1196..cb9f3a4ed3 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -6,7 +6,10 @@ import { registerCompletionOption, } from "./completion.js"; type McpResponse = Record; -import { awaitableLog } from "@inspector/core/cli/utils/awaitable-log.js"; +import { + awaitableError, + awaitableLog, +} from "@inspector/core/cli/utils/awaitable-log.js"; import type { InspectorServerSettings, MCPServerConfig, @@ -61,10 +64,16 @@ import { consumeMethodOutcome } from "./handlers/consume-outcome.js"; import { positiveIntegerParser, positiveNumberParser, + LOAD_FLAG_CONFLICT, + loadSummaryWarnings, resolveLoadOptions, runToolLoad, type LoadOptions, } from "@inspector/core/cli/handlers/load-test.js"; +import { + assertNotInterrupted, + withLoadInterrupt, +} from "./handlers/load-interrupt.js"; import { checkLoadThresholds, fractionParser, @@ -166,6 +175,7 @@ async function callMethod( throw new CliExitCodeError( EXIT_CODES.USAGE, "--load-connections above 1 requires an HTTP or SSE server; over stdio each connection would spawn its own server process.", + { code: LOAD_FLAG_CONFLICT }, ); } @@ -332,23 +342,39 @@ async function callMethod( // `--load` (#2439): many calls of one tool, then a single summary, in // place of the one result a plain `tools/call` prints. if (load) { - const summary = await withCliAuthRecoveryRetry( - inspectorClient, - serverConfig, - redirectUrlProvider, - callbackUrlConfig, - serverSettings, - () => - runToolLoad(inspectorClient, args, load, { - openConnection: openLoadConnection, - }), - { storedAuthOnly, autoOpenControl, quiet: args.quiet }, + // Ctrl+C / SIGTERM end the run early rather than the process (#2834), + // so the partial summary below still gets out. + const summary = await withLoadInterrupt( + (signal) => + withCliAuthRecoveryRetry( + inspectorClient, + serverConfig, + redirectUrlProvider, + callbackUrlConfig, + serverSettings, + () => + runToolLoad(inspectorClient, args, load, { + openConnection: openLoadConnection, + signal, + }), + { storedAuthOnly, autoOpenControl, quiet: args.quiet }, + ), + { quiet: args.quiet }, ); await writeFormattedResult( summary, args.format === "json" ? "json" : "text", ); - // After the summary is out, so a breached run still shows its numbers. + // Advisory, so `--quiet` drops them (#2435): a missed `--load-rate` + // (#2837) and an early stop the summary also records (#2835). + if (!args.quiet) { + for (const warning of loadSummaryWarnings(summary)) { + await awaitableError(`${warning}\n`); + } + } + // After the summary is out, so a cut-short or breached run still shows + // its numbers. An interrupted run exits 130 and is not graded. + assertNotInterrupted(summary); checkLoadThresholds(summary, loadThresholds); return; } @@ -1138,7 +1164,7 @@ async function parseArgs(argv?: string[]): Promise { ) .option( "--load-max-p99 ", - "With --load: exit 10 (load_p99_exceeded) when the p99 latency is above this many milliseconds, or when no call completed, after printing the summary (positive number).", + "With --load: exit 10 (load_p99_exceeded) when the p99 latency of the successful calls is above this many milliseconds, or when no call succeeded, after printing the summary (positive number).", maxP99Parser, ) .option( @@ -1437,24 +1463,24 @@ async function parseArgs(argv?: string[]): Promise { ] as const; for (const [flag, value] of loadBounds) { if (value !== undefined && !options.load) { - throw new Error(`${flag} requires --load.`); + throw loadFlagConflict(`${flag} requires --load.`); } } if (options.load) { if (options.method !== "tools/call") { - throw new Error("--load requires --method tools/call."); + throw loadFlagConflict("--load requires --method tools/call."); } // Each of these shapes a single result; a load run prints a summary. if (options.appInfo) { - throw new Error("--load cannot be combined with --app-info."); + throw loadFlagConflict("--load cannot be combined with --app-info."); } if (options.listStoredAuth || options.printHandoff) { - throw new Error( + throw loadFlagConflict( "--load cannot be combined with --list-stored-auth or --print-handoff.", ); } if (options.output !== undefined || options.outputFormat !== undefined) { - throw new Error( + throw loadFlagConflict( "--load cannot be combined with --output or --output-format; redirect stdout to keep the summary.", ); } @@ -1774,17 +1800,47 @@ async function parseArgs(argv?: string[]): Promise { // `--no-revoke` makes it false. revoke: options.revoke !== false, ...(options.load && { - load: resolveLoadOptions({ - concurrency: options.loadConcurrency, - connections: options.loadConnections, - durationSec: options.loadDuration, - requests: options.loadRequests, - rate: options.loadRate, - }), + load: resolveCliLoadOptions(options), }), }; } +/** + * A `--load` flag combination the CLI refuses, under the envelope code mcpdo + * uses for the same mistakes (#2834), so a script can branch on one code + * whichever CLI it drove. Still exit 1, as before. + */ +function loadFlagConflict(message: string): CliExitCodeError { + return new CliExitCodeError(EXIT_CODES.USAGE, message, { + code: LOAD_FLAG_CONFLICT, + }); +} + +/** + * {@link resolveLoadOptions}, with its one refusal (`--load-connections` + * above `--load-concurrency`) raised as a {@link loadFlagConflict}. The engine + * only ever throws an `Error` there. + */ +function resolveCliLoadOptions(options: { + loadConcurrency?: number; + loadConnections?: number; + loadDuration?: number; + loadRequests?: number; + loadRate?: number; +}): LoadOptions { + try { + return resolveLoadOptions({ + concurrency: options.loadConcurrency, + connections: options.loadConnections, + durationSec: options.loadDuration, + requests: options.loadRequests, + rate: options.loadRate, + }); + } catch (err) { + throw loadFlagConflict((err as Error).message); + } +} + /** Stands in for `console.warn` during a `--quiet` run (see `parseArgs`). */ function discardWarning(): void {} diff --git a/clients/cli/src/handlers/load-interrupt.ts b/clients/cli/src/handlers/load-interrupt.ts new file mode 100644 index 0000000000..82431dca6b --- /dev/null +++ b/clients/cli/src/handlers/load-interrupt.ts @@ -0,0 +1,99 @@ +import { + CliExitCodeError, + EXIT_CODES, +} from "@inspector/core/cli/error-handler.js"; +import type { + LoadStop, + LoadSummary, +} from "@inspector/core/cli/handlers/load-test.js"; + +/** + * Ctrl+C / SIGTERM for a one-shot `--load` run (#2834). + * + * Without this the CLI had no SIGINT listener during a run, so Node's default + * handler killed the process and every number from the run was lost — the + * moment they are most wanted (the server was struggling, or a CI job hit its + * timeout and sent SIGTERM). Here the FIRST signal aborts the engine's + * `signal`: no further call starts, in-flight calls settle, and the caller + * prints the partial summary (marked `stoppedEarly.reason: "interrupted"`) + * before exiting 130 via {@link assertNotInterrupted}. A SECOND signal exits + * 130 at once, for a run whose in-flight calls will not settle. + * + * ⚠️ **A repeat of the first signal inside {@link REPEAT_GRACE_MS} is not a + * second press.** Dependencies of the CLI hook signals too, and `when-exit` + * (via `atomically`) runs its cleanups on SIGINT/SIGTERM and then + * unconditionally re-raises the signal with `process.kill(process.pid, …)`. + * That copy reaches this listener a moment after the first, and counting it + * as Ctrl+C #2 exited 130 before the partial summary was written — which only + * a real signal to the built binary showed. Telling the copy apart by origin + * is not possible from a listener, so the window is the discriminator: the + * re-raise lands within milliseconds, and a person who means "exit now" + * presses again after reading the line this prints. A press inside the window + * is dropped, and the next one exits. + * + * The listeners exist only while `run` is pending, so a signal before or after + * the run keeps Node's default behaviour, and an in-process caller (the + * launcher, the test runner) is left with no stray listener. + */ +/** How long after the first signal a repeat of it is treated as a re-raise. */ +export const REPEAT_GRACE_MS = 500; + +export async function withLoadInterrupt( + run: (signal: AbortSignal) => Promise, + options: { + quiet?: boolean; + /** Injected by tests; the real one ends the process. */ + exit?: (code: number) => void; + /** Injected by tests; milliseconds on a monotonic clock. */ + now?: () => number; + } = {}, +): Promise { + const controller = new AbortController(); + const exit = options.exit ?? ((code: number) => process.exit(code)); + const now = options.now ?? (() => performance.now()); + let first: { signal: NodeJS.Signals; at: number } | undefined; + const onSignal = (signal: NodeJS.Signals): void => { + if (first !== undefined) { + // A dependency's re-raise of the first signal, not a second press. + if (signal === first.signal && now() - first.at < REPEAT_GRACE_MS) { + return; + } + exit(EXIT_CODES.INTERRUPTED); + return; + } + first = { signal, at: now() }; + const stop: LoadStop = { + reason: "interrupted", + detail: `received ${signal}`, + }; + controller.abort(stop); + if (!options.quiet) { + process.stderr.write( + `Interrupted (${signal}): no further calls will start; waiting for in-flight calls to finish. Press Ctrl+C again to exit now.\n`, + ); + } + }; + process.on("SIGINT", onSignal); + process.on("SIGTERM", onSignal); + try { + return await run(controller.signal); + } finally { + process.off("SIGINT", onSignal); + process.off("SIGTERM", onSignal); + } +} + +/** + * After the summary is printed: exit 130 (`interrupted`) when the run was + * cut short by Ctrl+C / SIGTERM, the conventional status for a process ended + * by SIGINT. Checked before any `--load-max-*` threshold, since a partial run + * is not a verdict on the server. + */ +export function assertNotInterrupted(summary: LoadSummary): void { + if (summary.stoppedEarly?.reason !== "interrupted") return; + throw new CliExitCodeError( + EXIT_CODES.INTERRUPTED, + `Load run interrupted (${summary.stoppedEarly.detail}); the summary covers the ${summary.requests} calls that completed.`, + { code: "interrupted" }, + ); +} diff --git a/clients/launcher/README.md b/clients/launcher/README.md index 280f5e75f7..c4707dad72 100644 --- a/clients/launcher/README.md +++ b/clients/launcher/README.md @@ -35,8 +35,8 @@ transport and sets the era that connection negotiates, so a non-legacy era needs no `mcp.json` entry. **Seed contents are web-specific.** When the web backend creates a missing -writable catalog it seeds `DEFAULT_SEED_CONFIG` (`core/mcp/serverList.ts`) — a -`filesystem-server-default` scoped to `/tmp`, the canonical +writable catalog it seeds `defaultSeedConfig` (`core/mcp/serverList.ts`) — a +`filesystem-server-default` scoped to the platform temp dir (`os.tmpdir()`), the canonical `everything-server-default`, and `example-server-default`, the MCP org's remote feature-reference server (Streamable HTTP, no local process and no API key needed) — so a first launch has something to connect to. diff --git a/clients/mcpdo/README.md b/clients/mcpdo/README.md index b06d23d585..8d1b8462f4 100644 --- a/clients/mcpdo/README.md +++ b/clients/mcpdo/README.md @@ -228,10 +228,18 @@ mcpdo connect https://example.com/mcp --elicit url [load mode](../cli/README.md#load-testing-a-tool---load), run against an open connection ([#2744](https://github.com/modelcontextprotocol/inspector/issues/2744)). It calls one tool repeatedly and concurrently and prints a single summary in -place of a result: `ok`, `toolErrors`, `errors`, `errorRate`, `throughput`, -latency `min`/`mean`/`p50`/`p90`/`p99`/`max`, and the most frequent error -messages. Both CLIs run the same engine, so the flags, their defaults and the -summary fields mean the same thing in each. +place of a result: `ok`, `toolErrors`, `errors`, `errorRate`, `throughput` +and the success-only `okThroughput`, latency `min`/`mean`/`p50`/`p90`/`p99`/`max` +of the successful calls (`latencyMs`) and of the failed ones +(`errorLatencyMs`), the most frequent thrown messages (`errorMessages`) and +`isError` texts (`toolErrorMessages`), and — with `--load-rate` — +`plannedStarts`, `droppedSlots` and `achievedRate`. Both CLIs run the same +engine, so the flags, their defaults and the summary fields mean the same +thing in each; the [CLI's load section](../cli/README.md#load-testing-a-tool---load) +defines each field ([#2836](https://github.com/modelcontextprotocol/inspector/issues/2836), +[#2837](https://github.com/modelcontextprotocol/inspector/issues/2837)). A +`--load-rate` run that achieves under 90% of its target also prints a warning +on stderr after the summary. ```bash # 20 calls in flight for 30 seconds @@ -250,7 +258,7 @@ mcpdo @prod tools/call search query:=mcp --load --load-requests 500 --load-rate | `--load-rate ` | Start at most this many calls per second, spaced evenly (default: unlimited). | | `--load-connections ` | Spread the calls over this many independent sessions (HTTP/SSE only; at most `--load-concurrency`; default `1`). | | `--load-max-error-rate ` | Exit `10` (`load_error_rate_exceeded`) when the share of failed calls is above this value (`0` to `1`). | -| `--load-max-p99 ` | Exit `10` (`load_p99_exceeded`) when the p99 latency is above this many milliseconds, or no call completed. | +| `--load-max-p99 ` | Exit `10` (`load_p99_exceeded`) when the p99 latency of the successful calls is above this many milliseconds, or no call succeeded. | Each `--load-*` flag requires `--load`, and `--load` cannot be combined with `--app-info`. `--format json` prints the summary object itself, with no @@ -292,8 +300,16 @@ How a run behaves on a shared daemon connection: cancelled, so the connection is free for the next command at once. No summary is printed, since the caller has gone. - **A dropped connection ends the run early** with the summary of what - completed, rather than spending the rest of the window counting instant - failures. The next command revives the connection as usual. + completed, marked `"stoppedEarly": { "reason": "connection_lost", … }`, + rather than spending the rest of the window counting instant failures. The + next command revives the connection as usual. +- **A server that stops answering ends the run early too**, even when the + connection never changes state — over Streamable HTTP a refused request + need not ([#2835](https://github.com/modelcontextprotocol/inspector/issues/2835)). + The shared engine's circuit breaker stops starting calls once 50 in a row + have thrown, or a second has passed with nothing but thrown calls; the + summary carries `"stoppedEarly": { "reason": "consecutive_errors", … }` and + a warning follows on stderr. - **Elicitations are cancelled.** Many calls are in flight at once and none of them can be answered mid-run, so a server that elicits during a load run sees each request cancelled. Use `connect --elicit off` to keep the server from diff --git a/clients/mcpdo/__tests__/daemon-load.test.ts b/clients/mcpdo/__tests__/daemon-load.test.ts index 41b385d3d4..50e6a0fca0 100644 --- a/clients/mcpdo/__tests__/daemon-load.test.ts +++ b/clients/mcpdo/__tests__/daemon-load.test.ts @@ -106,8 +106,11 @@ const SUMMARY: LoadSummary = { errorRate: 0, elapsedMs: 10, throughput: 400, + okThroughput: 400, latencyMs: { min: 1, mean: 2, p50: 2, p90: 3, p99: 3, max: 3 }, + errorLatencyMs: null, errorMessages: [], + toolErrorMessages: [], }; const LOAD: LoadOptions = { concurrency: 2, requests: 4 }; @@ -276,6 +279,11 @@ describe("daemon rpc with --load", () => { expect(runSignal.aborted).toBe(false); fake.emitStatus("disconnected"); expect(runSignal.aborted).toBe(true); + // Named, so the summary reports the drop rather than an interrupt. + expect(runSignal.reason).toEqual({ + reason: "connection_lost", + detail: "the connection went disconnected", + }); expect((await call).ok).toBe(true); }); diff --git a/clients/mcpdo/__tests__/load.test.ts b/clients/mcpdo/__tests__/load.test.ts index 20eeaa5afe..96a9b2c0d0 100644 --- a/clients/mcpdo/__tests__/load.test.ts +++ b/clients/mcpdo/__tests__/load.test.ts @@ -45,8 +45,11 @@ const SUMMARY: LoadSummary = { errorRate: 0.1, elapsedMs: 1500, throughput: 20, + okThroughput: 18, latencyMs: { min: 1, mean: 2.5, p50: 2, p90: 4, p99: 5, max: 6 }, + errorLatencyMs: null, errorMessages: [{ message: "Request timed out", count: 2 }], + toolErrorMessages: [], }; describe("resolveLoadFlags", () => { @@ -137,7 +140,7 @@ describe("formatLoadSummaryHuman", () => { expect(text).toBe( [ "Load test `echo` (concurrency 4, duration 1.5s, limit 40, rate 20/s)", - " Requests: 30 in 1.50s (20/s)", + " Requests: 30 in 1.50s (20/s, 18/s ok)", " Outcomes: 27 ok, 1 tool errors, 2 errors", " Error rate: 10.00%", " Latency: min 1 / mean 2.5 / p50 2 / p90 4 / p99 5 / max 6 ms", @@ -157,16 +160,58 @@ describe("formatLoadSummaryHuman", () => { errorRate: 0, elapsedMs: 0, throughput: 0, + okThroughput: 0, latencyMs: null, + errorLatencyMs: null, errorMessages: [], + toolErrorMessages: [], }); expect(text).toContain("Load test (concurrency 1)"); expect(text).toContain("Error rate: 0.00%"); - expect(text).toContain("Latency: no calls completed"); + expect(text).toContain("Latency: no calls succeeded"); + expect(text).not.toContain("Failed in:"); + expect(text).not.toContain("Rate:"); + expect(text).not.toContain("Stopped early"); expect(text).not.toContain("Top errors"); expect(text).not.toContain("Sessions:"); }); + it("renders failure latency, tool errors, the rate shortfall and an early stop", () => { + const text = formatLoadSummaryHuman( + { + ...SUMMARY, + plannedStarts: 30, + droppedSlots: 12, + achievedRate: 12, + errorLatencyMs: { min: 0, mean: 1, p50: 1, p90: 2, p99: 2, max: 2 }, + toolErrorMessages: [{ message: "rate limited", count: 1 }], + stoppedEarly: { + reason: "consecutive_errors", + detail: + "50 calls in a row threw over 3ms; the server looks unavailable", + }, + }, + PLAIN, + ); + expect(text).toContain( + " Rate: 12/s achieved; 30 planned, 12 dropped\n", + ); + expect(text).toContain( + " Failed in: min 0 / mean 1 / p50 1 / p90 2 / p99 2 / max 2 ms\n", + ); + expect(text).toContain(" Top tool errors:\n 1× rate limited"); + expect(text).toContain( + " Stopped early (consecutive_errors): 50 calls in a row threw over 3ms; the server looks unavailable", + ); + // A rate held in full reports nothing dropped. + expect( + formatLoadSummaryHuman( + { ...SUMMARY, plannedStarts: 30, droppedSlots: 0, achievedRate: 20 }, + PLAIN, + ), + ).toContain(" Rate: 20/s achieved; 30 planned, 0 dropped"); + }); + it("renders the sessions of a multi-connection run, failures included", () => { const text = formatLoadSummaryHuman( { @@ -356,6 +401,34 @@ describe("mcpdo tools/call --load", { timeout: DAEMON_TEST_MS }, () => { ); } + it("warns on stderr when a --load-rate run falls short (#2837)", async () => { + const e = env(); + await connectStdio(e); + // No single worker starts 100k calls a second. + const result = await runMcp( + [ + "tools/call", + "echo", + "message:=hi", + "--load", + "--load-rate", + "100000", + "--load-concurrency", + "1", + "--load-duration", + "0.3", + "--format", + "json", + ], + { env: e, timeout: DAEMON_COMMAND_MS }, + ); + expectCliSuccess(result); + const summary = JSON.parse(result.stdout); + expect(summary).toMatchObject({ rate: 100000, plannedStarts: 30000 }); + expect(summary.droppedSlots).toBeGreaterThan(0); + expect(result.stderr).toContain("Warning: --load-rate 100000/s"); + }); + it("prints the summary, then fails a breached threshold with the CLI's code", async () => { const e = env(); await connectStdio(e); diff --git a/clients/mcpdo/__tests__/mcp-coverage.test.ts b/clients/mcpdo/__tests__/mcp-coverage.test.ts index 1422bbf4ee..61b8f71961 100644 --- a/clients/mcpdo/__tests__/mcp-coverage.test.ts +++ b/clients/mcpdo/__tests__/mcp-coverage.test.ts @@ -537,6 +537,48 @@ describe("mcp.ts coverage", () => { expect(unknown.stderr).toContain(`(config ${configPath}`); }); + // Windows named pipes have no sun_path limit, so the check is a no-op there. + it.skipIf(process.platform === "win32")( + "an over-limit daemon socket path fails connections/list and MRU inference like connect does (#2823)", + async () => { + configPath = createSampleTestConfig(); + const e = env(); + // Never created: the point is that nothing could ever bind here. + const longDir = path.join(storageDir!, "d".repeat(120)); + const longEnv = { ...e, MCP_INSPECTOR_DAEMON_DIR: longDir }; + + const listed = await runMcp(["connections/list", "--format", "json"], { + env: longEnv, + }); + expectCliFailure(listed); + expect(listed.stdout).not.toMatch(/connections/); + expect(listed.stderr).toMatch(/socket path is too long/); + expect(listed.stderr).toContain(path.join(longDir, "mcpdod.sock")); + + const noMru = await runMcp(["servers/show", "--config", configPath], { + env: longEnv, + timeout: 20000, + }); + expectCliFailure(noMru); + expect(noMru.stderr).toMatch(/socket path is too long/); + expect(noMru.stderr).not.toMatch(/no most-recently-used connection/); + + // Neither command starts the daemon, or even creates its directory. + expect(fs.existsSync(longDir)).toBe(false); + + // Same error, same exit code as the command that would start it. + const connected = await runMcp( + ["connect", "test-stdio", "--config", configPath], + { env: longEnv, timeout: 20000 }, + ); + expectCliFailure(connected); + expect(connected.stderr).toMatch(/socket path is too long/); + + expect(listed.exitCode).toBe(connected.exitCode); + expect(noMru.exitCode).toBe(connected.exitCode); + }, + ); + it("connections/list and daemon status do not auto-spawn the daemon", async () => { const e = env(); const listed = await runMcp(["connections/list", "--format", "json"], { diff --git a/clients/mcpdo/src/connection/dispatch.ts b/clients/mcpdo/src/connection/dispatch.ts index add91f6127..59a731dc41 100644 --- a/clients/mcpdo/src/connection/dispatch.ts +++ b/clients/mcpdo/src/connection/dispatch.ts @@ -6,6 +6,8 @@ import type { } from "@inspector/core/cli/handlers/method-types.js"; import type { OutputFormat } from "@inspector/core/cli/handlers/format-output.js"; import { checkLoadThresholds } from "@inspector/core/cli/handlers/load-thresholds.js"; +import { loadSummaryWarnings } from "@inspector/core/cli/handlers/load-test.js"; +import { awaitableError } from "@inspector/core/cli/utils/awaitable-log.js"; import type { LoadFlags } from "./load-flags.js"; import { writeConnectionOutput } from "./format-connection.js"; import { styleFromOpts, type Style } from "@inspector/core/cli/style.js"; @@ -147,6 +149,11 @@ export async function dispatchConnectionRpc( // the one-shot CLI's order (#2745). The breach is a CliExitCodeError, so it // leaves as mcpdo's error envelope with the CLI's code and exit code. if (outcome.kind === "load") { + // On stderr, after the summary, as the one-shot CLI prints them: an early + // stop (#2835) and a missed `--load-rate` (#2837). + for (const warning of loadSummaryWarnings(outcome.summary)) { + await awaitableError(`${warning}\n`); + } checkLoadThresholds(outcome.summary, opts.load?.thresholds); } } diff --git a/clients/mcpdo/src/connection/format-human.ts b/clients/mcpdo/src/connection/format-human.ts index e1e6df99ee..a71800d845 100644 --- a/clients/mcpdo/src/connection/format-human.ts +++ b/clients/mcpdo/src/connection/format-human.ts @@ -1000,22 +1000,47 @@ export function formatLoadSummaryHuman( const rate = `${(summary.errorRate * 100).toFixed(2)}%`; const lines = [ `${heading(style, "Load test")}${tool} ${style.dim(`(${bounds.join(", ")})`)}`, - ` Requests: ${summary.requests} in ${(summary.elapsedMs / 1000).toFixed(2)}s (${summary.throughput}/s)`, + ` Requests: ${summary.requests} in ${(summary.elapsedMs / 1000).toFixed(2)}s (${summary.throughput}/s, ${summary.okThroughput}/s ok)`, ` Outcomes: ${style.green(`${summary.ok} ok`)}, ${summary.toolErrors} tool errors, ${summary.errors} errors`, ` Error rate: ${failed > 0 ? style.red(rate) : rate}`, ]; + // `--load-rate` only (#2837): the planned starts the run missed. + if (summary.plannedStarts !== undefined) { + const dropped = `${summary.droppedSlots} dropped`; + lines.push( + ` Rate: ${summary.achievedRate}/s achieved; ${summary.plannedStarts} planned, ${summary.droppedSlots! > 0 ? style.red(dropped) : dropped}`, + ); + } const l = summary.latencyMs; lines.push( l === null - ? ` Latency: ${style.dim("no calls completed")}` + ? ` Latency: ${style.dim("no calls succeeded")}` : ` Latency: min ${l.min} / mean ${l.mean} / p50 ${l.p50} / p90 ${l.p90} / p99 ${l.p99} / max ${l.max} ms`, ); + // Failures are timed apart from successes (#2836). + const e = summary.errorLatencyMs; + if (e !== null) { + lines.push( + ` Failed in: min ${e.min} / mean ${e.mean} / p50 ${e.p50} / p90 ${e.p90} / p99 ${e.p99} / max ${e.max} ms`, + ); + } if (summary.errorMessages.length > 0) { lines.push(` ${style.bold("Top errors:")}`); for (const { message, count } of summary.errorMessages) { lines.push(` ${count}× ${message}`); } } + if (summary.toolErrorMessages.length > 0) { + lines.push(` ${style.bold("Top tool errors:")}`); + for (const { message, count } of summary.toolErrorMessages) { + lines.push(` ${count}× ${message}`); + } + } + if (summary.stoppedEarly !== undefined) { + lines.push( + ` ${style.red("Stopped early")} (${summary.stoppedEarly.reason}): ${summary.stoppedEarly.detail}`, + ); + } // `--load-connections` above 1 (#2800). Connect failures are not call // errors, so they are listed under this line rather than "Top errors". const c = summary.connections; diff --git a/clients/mcpdo/src/connection/load-flags.ts b/clients/mcpdo/src/connection/load-flags.ts index c9ad7cacdc..a632a32d65 100644 --- a/clients/mcpdo/src/connection/load-flags.ts +++ b/clients/mcpdo/src/connection/load-flags.ts @@ -3,6 +3,7 @@ import { EXIT_CODES, } from "@inspector/core/cli/error-handler.js"; import { + LOAD_FLAG_CONFLICT, resolveLoadOptions, type LoadOptions, } from "@inspector/core/cli/handlers/load-test.js"; @@ -93,6 +94,6 @@ export function resolveLoadFlags(flags: { return { options, ...(thresholds && { thresholds }) }; } -function usage(message: string, code = "load_flag_conflict"): CliExitCodeError { +function usage(message: string, code = LOAD_FLAG_CONFLICT): CliExitCodeError { return new CliExitCodeError(EXIT_CODES.USAGE, message, { code }); } diff --git a/clients/mcpdo/src/connection/mcp.ts b/clients/mcpdo/src/connection/mcp.ts index e9bee4df00..6f9aa21dcc 100644 --- a/clients/mcpdo/src/connection/mcp.ts +++ b/clients/mcpdo/src/connection/mcp.ts @@ -31,7 +31,12 @@ import { getSecretStorageInfo, warnAboutSecretStorage, } from "@inspector/core/auth/node/secret-store-selection.js"; -import { callDaemon, ensureDaemon } from "../daemon/index.js"; +import { + assertSocketPathWithinLimit, + callDaemon, + ensureDaemon, + getDaemonSocketPath, +} from "../daemon/index.js"; import type { ConnectionInfo, ConnectionShowResult, @@ -122,6 +127,19 @@ function isDaemonUnreachable(error: unknown): boolean { ); } +/** + * For commands that read "daemon unreachable" as "nothing is connected" + * without starting the daemon (`connections/list`, MRU name inference): + * rethrow anything that is not an unreachable daemon, and fail with + * `ensureDaemon()`'s own socket-path error when the daemon could never bind + * in this environment at all (#2823). Without the check, an over-limit + * socket path reads as a healthy empty machine while `connect` refuses. + */ +function assertNoDaemonMeansNothingConnected(error: unknown): void { + if (!isDaemonUnreachable(error)) throw error; + assertSocketPathWithinLimit(getDaemonSocketPath()); +} + /** * `servers/show` with no name falls back to the MRU connection's entry name, * under the same non-interactive gate as MRU connection targeting: an agent @@ -145,7 +163,7 @@ async function resolveMruEntryName(): Promise { ); connections = result.connections; } catch (error) { - if (!isDaemonUnreachable(error)) throw error; + assertNoDaemonMeansNothingConnected(error); } const mru = connections.find((c) => c.isMru); if (!mru) { @@ -1062,14 +1080,11 @@ function registerConnectionAdmin(program: CommandType): void { connections: result.connections, }); } catch (error) { - if (isDaemonUnreachable(error)) { - await writeConnectionOutput(outOpts(opts), { - kind: "connections/list", - connections: [], - }); - return; - } - throw error; + assertNoDaemonMeansNothingConnected(error); + await writeConnectionOutput(outOpts(opts), { + kind: "connections/list", + connections: [], + }); } }); @@ -1392,7 +1407,7 @@ function registerRpcCommands(program: CommandType): void { ) .option( "--load-max-p99 ", - "With --load: exit 10 (load_p99_exceeded) when the p99 latency is above this many milliseconds, or when no call completed, after printing the summary (positive number).", + "With --load: exit 10 (load_p99_exceeded) when the p99 latency of the successful calls is above this many milliseconds, or when no call succeeded, after printing the summary (positive number).", maxP99Parser, ); cmd.action( diff --git a/clients/mcpdo/src/daemon/server.ts b/clients/mcpdo/src/daemon/server.ts index 29fd73d356..bed3959e0c 100644 --- a/clients/mcpdo/src/daemon/server.ts +++ b/clients/mcpdo/src/daemon/server.ts @@ -15,6 +15,7 @@ import type { MethodArgs } from "@inspector/core/cli/handlers/method-types.js"; import { runToolLoad, type LoadOptions, + type LoadStop, } from "@inspector/core/cli/handlers/load-test.js"; import { acceptDaemonConnection, @@ -770,7 +771,14 @@ export class DaemonServer { const onStatus = ( event: TypedEventGeneric, ) => { - if (isTerminalStatus(event.detail)) stop.abort(); + if (!isTerminalStatus(event.detail)) return; + // Named, so the summary says why it stopped (#2835) rather than + // reading as an interrupt. + const lost: LoadStop = { + reason: "connection_lost", + detail: `the connection went ${event.detail}`, + }; + stop.abort(lost); }; client.addEventListener("statusChange", onStatus); // In-flight calls abort with the run, not only new ones (#1783). diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index 0d15556b35..712df66bf8 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -1164,6 +1164,49 @@ describe("App (status, layout, modals)", () => { expect(f).toContain("alpha"); }); + it("lists tools alphabetically by name, as the web sidebar does (#2827)", async () => { + h.ctrl.status = "connected"; + h.ctrl.tools = [ + { ...sampleTool, name: "zeta" }, + { ...sampleTool, name: "tool_10" }, + { ...sampleTool, name: "Beta" }, + { ...sampleTool, name: "tool_2" }, + ]; + const r = await mount(oneStdio()); + await press(r, ["t"]); + await expectFrame(r, "Tools (4)"); + const f = r.lastFrame() ?? ""; + const order = ["Beta", "tool_2", "tool_10", "zeta"].map((n) => + f.indexOf(n), + ); + expect(order.every((i) => i >= 0)).toBe(true); + expect(order).toEqual([...order].sort((a, b) => a - b)); + }); + + it("keeps the tool selection across an unrelated update such as a stderr line (#2827)", async () => { + h.ctrl.status = "connected"; + h.ctrl.tools = [ + { ...sampleTool, name: "zeta" }, + { ...sampleTool, name: "beta" }, + ]; + const r = await mount(oneStdio()); + await press(r, ["t", TAB, DOWN]); + await expectFrame(r, "▶ zeta"); + // A new stderr array re-derives the server state; the sorted tools must + // keep their identity, or ToolsTab resets the cursor to the top. + h.ctrl.stderrLogs = [stderrLog]; + r.rerender( + , + ); + await tick(); + await tick(); + expect(r.lastFrame() ?? "").toContain("▶ zeta"); + }); + it("opens the tool test modal with Enter from the list pane", async () => { h.ctrl.status = "connected"; h.ctrl.tools = [sampleTool]; diff --git a/clients/tui/__tests__/PromptsTab.test.tsx b/clients/tui/__tests__/PromptsTab.test.tsx index 8675653bcc..77fe052bb3 100644 --- a/clients/tui/__tests__/PromptsTab.test.tsx +++ b/clients/tui/__tests__/PromptsTab.test.tsx @@ -325,3 +325,22 @@ describe("PromptsTab list filter (#2430)", () => { expect(lastFrame()).toContain("Prompts (3)"); }); }); + +describe("PromptsTab in a narrow terminal (#2826)", () => { + it("truncates a long row to one line and shows the full name in the pane", () => { + const { lastFrame } = render( + , + ); + const lines = (lastFrame() ?? "").split("\n"); + const listColumn = lines.map((l) => l.split("│")[0] ?? ""); + expect(listColumn.some((l) => /▶ a_very_lo\S*…/.test(l))).toBe(true); + // The tail of the name never lands on a list line of its own. + expect(listColumn.some((l) => l.includes("name"))).toBe(false); + expect(lines.some((l) => l.includes("a_very_long_prompt_name"))).toBe(true); + }); +}); diff --git a/clients/tui/__tests__/ResourcesTab.test.tsx b/clients/tui/__tests__/ResourcesTab.test.tsx index 68f2ec1946..7eeae0e3a8 100644 --- a/clients/tui/__tests__/ResourcesTab.test.tsx +++ b/clients/tui/__tests__/ResourcesTab.test.tsx @@ -482,3 +482,21 @@ describe("ResourcesTab list filter — template titles (#2430)", () => { expect(frame).not.toContain("other"); }); }); + +describe("ResourcesTab in a narrow terminal (#2826)", () => { + it("truncates a long row to one line and shows the full name in the pane", () => { + const { lastFrame } = render( + , + ); + const lines = (lastFrame() ?? "").split("\n"); + const listColumn = lines.map((l) => l.split("│")[0] ?? ""); + expect(listColumn.some((l) => /▶ a_very_lo\S*…/.test(l))).toBe(true); + expect(listColumn.some((l) => l.includes("_nm"))).toBe(false); + expect(lines.some((l) => l.includes("a_very_long_resource_nm"))).toBe(true); + }); +}); diff --git a/clients/tui/__tests__/ToolsTab.test.tsx b/clients/tui/__tests__/ToolsTab.test.tsx index 15bbd36a05..ff0855c239 100644 --- a/clients/tui/__tests__/ToolsTab.test.tsx +++ b/clients/tui/__tests__/ToolsTab.test.tsx @@ -372,3 +372,135 @@ describe("ToolsTab list filter — duplicate names (#2430)", () => { expect(frame).toContain("second copy"); }); }); + +describe("ToolsTab in a narrow terminal (#2826)", () => { + // The issue's own fixture: the stdio test server's eight tools, the last of + // which is selected at 60x18. A 42-column tab gives a 16-column list (12 + // usable after border and padding) and a 26-column detail pane. + const names = [ + "echo", + "get_annotated_message", + "get_sum", + "long_running_operation", + "print_env", + "send_notification", + "write_to_stderr", + "mcp_app_demo", + ]; + const narrowTools = names.map((name) => + makeTool({ name, description: `${name} desc` }), + ); + + it("renders each row on one line, truncated with an ellipsis", () => { + const { lastFrame } = render( + , + ); + const frame = lastFrame() ?? ""; + // Truncated, not wrapped: the tail never lands on a line of its own. + expect(frame).toContain("…"); + expect(frame).not.toMatch(/^\s*sage\b/m); + expect(frame).not.toMatch(/│\s*sage\s/); + // Every tool still has a row, and the list holds one line per tool. + const rows = frame + .split("\n") + .filter((line) => /^\s*(▶ | {2})[a-z]/.test(line.split("│")[0] ?? "")); + expect(rows).toHaveLength(names.length); + }); + + it("keeps the selected row visible after moving to the last tool", async () => { + const { lastFrame, stdin } = render( + , + ); + for (let i = 0; i < names.length - 1; i++) { + stdin.write(DOWN); + await tick(); + } + const frame = lastFrame() ?? ""; + const listColumn = frame.split("\n").map((l) => l.split("│")[0] ?? ""); + expect(listColumn.some((l) => l.includes("▶ mcp_app_d"))).toBe(true); + // The full name is in the detail pane even though the row truncates it. + expect(frame).toContain("mcp_app_demo"); + }); + + it("keeps the selection in view when the terminal shrinks", async () => { + const props = { + tools: narrowTools, + isConnected: false, + width: 42, + focusedPane: "list" as const, + }; + const { lastFrame, stdin, rerender } = render( + , + ); + for (let i = 0; i < names.length - 1; i++) { + stdin.write(DOWN); + await tick(); + } + rerender(); + await tick(); + const listColumn = (lastFrame() ?? "") + .split("\n") + .map((l) => l.split("│")[0] ?? ""); + expect(listColumn.some((l) => l.includes("▶ mcp_app_d"))).toBe(true); + }); + + it("drops the Enter-to-Test hint below the title rather than overlapping it", () => { + const { lastFrame } = render( + , + ); + const lines = (lastFrame() ?? "").split("\n"); + const titleLine = lines.findIndex((l) => l.includes("mcp_app_demo")); + const hintLine = lines.findIndex((l) => l.includes("[Enter to Test]")); + expect(titleLine).toBeGreaterThanOrEqual(0); + expect(hintLine).toBe(titleLine + 1); + }); + + it("keeps the hint beside the title when there is room", () => { + const { lastFrame } = render( + , + ); + const lines = (lastFrame() ?? "").split("\n"); + expect( + lines.some((l) => l.includes("echo") && l.includes("[Enter to Test]")), + ).toBe(true); + }); + + it("keeps a row's schema marker visible when its name is truncated", () => { + const flagged = makeTool({ + name: "get_annotated_message", + outputSchema: { type: "object", properties: { data: true } }, + } as Partial); + const { lastFrame } = render( + , + ); + const frame = lastFrame() ?? ""; + const row = + frame + .split("\n") + .map((l) => l.split("│")[0] ?? "") + .find((l) => l.includes("▶ get_anno")) ?? ""; + expect(row).toMatch(/… !\s*$/); + }); +}); diff --git a/clients/tui/__tests__/formHeader.test.ts b/clients/tui/__tests__/formHeader.test.ts new file mode 100644 index 0000000000..f196390504 --- /dev/null +++ b/clients/tui/__tests__/formHeader.test.ts @@ -0,0 +1,22 @@ +import { describe, it, expect } from "vitest"; +import { untitledForm } from "../src/utils/formHeader.js"; +import { schemaToForm } from "../src/utils/schemaToForm.js"; + +describe("untitledForm (#2827)", () => { + it("drops the title the modal header already shows, keeping the sections", () => { + const form = schemaToForm( + { type: "object", properties: { q: { type: "string" } } }, + "search", + ); + expect(form.title).toBe("Test Tool: search"); + const body = untitledForm(form); + expect(body.title).toBeUndefined(); + expect(body.sections).toBe(form.sections); + }); + + it("does not mutate the form the header reads", () => { + const form = schemaToForm(null, "noParams"); + untitledForm(form); + expect(form.title).toBe("Test Tool: noParams"); + }); +}); diff --git a/clients/tui/__tests__/saveResult.queue.test.ts b/clients/tui/__tests__/saveResult.queue.test.ts index c004bf91d0..c69bf4f3a5 100644 --- a/clients/tui/__tests__/saveResult.queue.test.ts +++ b/clients/tui/__tests__/saveResult.queue.test.ts @@ -1,3 +1,4 @@ +import { resolve } from "node:path"; import { describe, it, expect, vi } from "vitest"; // A writeFile double whose calls settle only when the test says so, so the @@ -17,6 +18,11 @@ vi.mock("node:fs/promises", () => ({ import { saveResultToFile } from "../src/utils/saveResult.js"; +// saveResultToFile resolves its target with node:path `resolve`, so the +// expected path is built the same way: "/d/a.json" on POSIX, "D:\\d\\a.json" +// on Windows (#2810). +const inD = (name: string) => resolve("/d", name); + const TEXT = { content: [{ type: "text", text: "hi" }] }; const flush = async () => { @@ -34,11 +40,11 @@ describe("saveResultToFile queueing (#2571)", () => { order.push("b"); }); await flush(); - expect(writes.map((w) => w.path)).toEqual(["/d/a.json"]); + expect(writes.map((w) => w.path)).toEqual([inD("a.json")]); writes[0]!.resolve(); await first; await flush(); - expect(writes.map((w) => w.path)).toEqual(["/d/a.json", "/d/b.json"]); + expect(writes.map((w) => w.path)).toEqual([inD("a.json"), inD("b.json")]); writes[1]!.resolve(); await second; expect(order).toEqual(["a", "b"]); @@ -51,10 +57,12 @@ describe("saveResultToFile queueing (#2571)", () => { await flush(); expect(writes).toHaveLength(1); writes[0]!.reject(new Error("EACCES")); - await expect(failed).rejects.toThrow("Could not write /d/x.json: EACCES"); + await expect(failed).rejects.toThrow( + `Could not write ${inD("x.json")}: EACCES`, + ); await flush(); expect(writes).toHaveLength(2); writes[1]!.resolve(); - await expect(next).resolves.toMatchObject({ path: "/d/y.json" }); + await expect(next).resolves.toMatchObject({ path: inD("y.json") }); }); }); diff --git a/clients/tui/__tests__/tui-servers.test.ts b/clients/tui/__tests__/tui-servers.test.ts index 00ace5fccd..cb59652f9a 100644 --- a/clients/tui/__tests__/tui-servers.test.ts +++ b/clients/tui/__tests__/tui-servers.test.ts @@ -75,8 +75,8 @@ describe("loadTuiServers", () => { it("builds a single ad-hoc server from a positional target", async () => { const servers = await loadTuiServers({ target: ["my-server", "--flag"] }); - expect(Object.keys(servers)).toEqual(["default"]); - expect(servers.default?.config).toMatchObject({ + expect(Object.keys(servers)).toEqual(["my-server"]); + expect(servers["my-server"]?.config).toMatchObject({ type: "stdio", command: "my-server", args: ["--flag"], diff --git a/clients/tui/__tests__/useSelectableList.test.tsx b/clients/tui/__tests__/useSelectableList.test.tsx index 9ac86a9ae4..c8f603196b 100644 --- a/clients/tui/__tests__/useSelectableList.test.tsx +++ b/clients/tui/__tests__/useSelectableList.test.tsx @@ -131,4 +131,17 @@ describe("useSelectableList", () => { // itemCount === 0 => no clamp, selection retained expect(lastFrame()).toBe("sel=8 first=4"); }); + + it("keeps the selection in the window when visibleCount shrinks (#2826)", async () => { + const { lastFrame, stdin, rerender } = render( + , + ); + stdin.write("9"); + await tick(); + expect(lastFrame()).toBe("sel=9 first=0"); + // A terminal resize: the selection did not move, the window did. + rerender(); + await tick(); + expect(lastFrame()).toBe("sel=9 first=7"); + }); }); diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 1960a222ca..cd88fa4829 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -44,6 +44,7 @@ import { } from "@inspector/core/mcp/node/index.js"; import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js"; import { useManagedTools } from "@inspector/core/react/useManagedTools.js"; +import { compareToolsByName } from "@inspector/core/mcp/toolOrder.js"; import { useManagedResources } from "@inspector/core/react/useManagedResources.js"; import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js"; import { useManagedPrompts } from "@inspector/core/react/useManagedPrompts.js"; @@ -429,9 +430,10 @@ function App({ }), ...(savedSettings && { serverSettings: savedSettings }), // Advertise the roots configured for this server in mcp.json, as web - // and the CLI do. The TUI has no roots editor, but a user who set - // roots in the web UI expects them to apply to the same server here - // (#1797). + // and the CLI do (#1797). This is the starting list: the roots + // editor (#2432, `e` on the Info tab) changes it for this session + // through `InspectorClient.setRoots`, but never writes mcp.json, so + // these configured roots return on the next launch. roots: cleanRoots(savedSettings?.roots ?? []), // Per-server protocol era (SEP §7.8) from mcp.json → SDK // versionNegotiation; absent era defaults to legacy (#1626). @@ -697,6 +699,15 @@ function App({ selectedInspectorClient, selectedManagedToolsState, ); + // Alphabetical by the name each row shows, the order the web Tools sidebar + // uses (#2782, #2827) — not the server's `tools/list` order. A copy (the + // hook's array is the store's snapshot), memoized on that snapshot alone: + // ToolsTab resets its selection when its list's identity changes, so a + // fresh array on an unrelated update (a stderr line) would lose the cursor. + const sortedTools = useMemo( + () => [...managedTools].sort(compareToolsByName), + [managedTools], + ); // Resources, resource templates, prompts from managed state managers const selectedManagedResourcesState = useMemo( @@ -1272,7 +1283,7 @@ function App({ resources: managedResources, resourceTemplates: managedResourceTemplates, prompts: managedPrompts, - tools: managedTools, + tools: sortedTools, stderrLogs: inspectorStderrLogs, // InspectorClient manages this }; }, [ @@ -1286,7 +1297,7 @@ function App({ managedResources, managedResourceTemplates, managedPrompts, - managedTools, + sortedTools, inspectorStderrLogs, ]); diff --git a/clients/tui/src/components/PromptTestModal.tsx b/clients/tui/src/components/PromptTestModal.tsx index a78eec4927..317623bb31 100644 --- a/clients/tui/src/components/PromptTestModal.tsx +++ b/clients/tui/src/components/PromptTestModal.tsx @@ -7,6 +7,7 @@ import type { Prompt, GetPromptResult } from "@modelcontextprotocol/client"; import { promptArgsToForm } from "../utils/promptArgsToForm.js"; import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; import { redactErrorText, redactedJson } from "../utils/errorText.js"; +import { untitledForm } from "../utils/formHeader.js"; // Helper to extract error message from various error types function getErrorMessage(error: unknown): string { @@ -221,7 +222,7 @@ export function PromptTestModal({ )}
handleFormSubmit(values as Record) } diff --git a/clients/tui/src/components/PromptsTab.tsx b/clients/tui/src/components/PromptsTab.tsx index 6392ab06e6..fe56871e6a 100644 --- a/clients/tui/src/components/PromptsTab.tsx +++ b/clients/tui/src/components/PromptsTab.tsx @@ -220,7 +220,9 @@ export function PromptsTab({ paddingY={0} flexShrink={0} > - + {/* One rendered line per row (#2826); the detail pane + shows the full name. */} + {isSelected ? "▶ " : " "} {prompt.name || `Prompt ${ordinal + 1}`} diff --git a/clients/tui/src/components/ResourceTestModal.tsx b/clients/tui/src/components/ResourceTestModal.tsx index 1bbf305828..e7ebe43a26 100644 --- a/clients/tui/src/components/ResourceTestModal.tsx +++ b/clients/tui/src/components/ResourceTestModal.tsx @@ -12,6 +12,7 @@ import { } from "@inspector/core/mcp/uriTemplate.js"; import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; import { redactErrorText, redactedJson } from "../utils/errorText.js"; +import { untitledForm } from "../utils/formHeader.js"; // Helper to extract error message from various error types function getErrorMessage(error: unknown): string { @@ -274,7 +275,7 @@ export function ResourceTestModal({ )} handleFormSubmit(values as Record) } diff --git a/clients/tui/src/components/ResourcesTab.tsx b/clients/tui/src/components/ResourcesTab.tsx index 6c0ce35297..f0e58dea3b 100644 --- a/clients/tui/src/components/ResourcesTab.tsx +++ b/clients/tui/src/components/ResourcesTab.tsx @@ -315,7 +315,9 @@ export function ResourcesTab({ }`; return ( - + {/* One rendered line per row (#2826); the detail pane + shows the full name. */} + {isSelected ? "▶ " : " "} {label} diff --git a/clients/tui/src/components/ToolTestModal.tsx b/clients/tui/src/components/ToolTestModal.tsx index 30f7833e8e..aed74624b1 100644 --- a/clients/tui/src/components/ToolTestModal.tsx +++ b/clients/tui/src/components/ToolTestModal.tsx @@ -14,6 +14,7 @@ import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; import { inlineLocalRefs } from "@inspector/core/json/localRefs.js"; import { redactErrorText, redactedJson } from "../utils/errorText.js"; import type { ResultFileFormat } from "@inspector/core/mcp/resultFile.js"; +import { untitledForm } from "../utils/formHeader.js"; import { defaultResultFileName, saveResultToFile, @@ -384,7 +385,7 @@ export function ToolTestModal({ {state === "form" && ( void handleFormSubmit(value as Record) } diff --git a/clients/tui/src/components/ToolsTab.tsx b/clients/tui/src/components/ToolsTab.tsx index 55c1bb27a5..3a0baaf2e0 100644 --- a/clients/tui/src/components/ToolsTab.tsx +++ b/clients/tui/src/components/ToolsTab.tsx @@ -224,13 +224,19 @@ export function ToolsTab({ paddingY={0} flexShrink={0} > - + {/* One rendered line per row (#2826): the window counts + items, so a wrapped name would push the selected row + out of view. The marker sits outside the truncated + text so an ellipsis never swallows it. */} + {isSelected ? "▶ " : " "} {tool.name || `Tool ${ordinal + 1}`} - {marker && ( - {marker.glyph} - )} + {marker && ( + + {marker.glyph} + + )} ); })} @@ -249,10 +255,15 @@ export function ToolsTab({ {selectedTool ? ( <> {/* Fixed header */} + {/* The title shows the full name (rows truncate it), and the + hint wraps onto its own line when the two do not fit side by + side rather than overlapping the title (#2826). */} {isConnected && ( - + [Enter to Test] - + )} diff --git a/clients/tui/src/hooks/useSelectableList.ts b/clients/tui/src/hooks/useSelectableList.ts index 054624e9b0..86a62afff3 100644 --- a/clients/tui/src/hooks/useSelectableList.ts +++ b/clients/tui/src/hooks/useSelectableList.ts @@ -57,5 +57,13 @@ export function useSelectableList( } }, [itemCount, selectedIndex, visibleCount]); - return { selectedIndex, firstVisible, setSelection }; + // The stored window is re-clamped on every render, not only on a selection + // move: a terminal resize shrinks `visibleCount` without moving the + // selection, and the stale window would then slice the selected row off the + // bottom of the list (#2826). Pure, so it is safe to compute during render. + return { + selectedIndex, + firstVisible: clampFirstVisible(firstVisible, selectedIndex, visibleCount), + setSelection, + }; } diff --git a/clients/tui/src/utils/formHeader.ts b/clients/tui/src/utils/formHeader.ts new file mode 100644 index 0000000000..1da9688b31 --- /dev/null +++ b/clients/tui/src/utils/formHeader.ts @@ -0,0 +1,17 @@ +/** + * The form a test modal hands to `ink-form`, minus its title (#2827). + * + * Each test modal (tool, prompt, resource template) prints the form's title in + * its own header, because it has to stay visible in the loading and results + * states too — and `ink-form` prints the same `form.title` again in its own + * header box, so the open form showed "Test Tool: " twice. The modal's + * header is the one that survives every state, so the copy given to the form + * drops it. `title` is optional in `ink-form`'s `FormStructure`; without one + * its header keeps the navigation hint and the section tabs. + */ + +import type { FormStructure } from "ink-form"; + +export function untitledForm(form: FormStructure): FormStructure { + return { ...form, title: undefined }; +} diff --git a/clients/web/.storybook/main.ts b/clients/web/.storybook/main.ts index 69b7bb2204..a4a08b132d 100644 --- a/clients/web/.storybook/main.ts +++ b/clients/web/.storybook/main.ts @@ -10,11 +10,5 @@ const config: StorybookConfig = { "@storybook/addon-onboarding", ], framework: "@storybook/react-vite", - previewHead: (head) => ` - ${head} - - - - `, }; export default config; diff --git a/clients/web/.storybook/preview.tsx b/clients/web/.storybook/preview.tsx index cd2edd3ac3..6efcc5587b 100644 --- a/clients/web/.storybook/preview.tsx +++ b/clients/web/.storybook/preview.tsx @@ -3,6 +3,7 @@ import { MantineProvider, useMantineColorScheme } from "@mantine/core"; import { Notifications } from "@mantine/notifications"; import "@mantine/core/styles.css"; import "@mantine/notifications/styles.css"; +import "../src/theme/fonts/fonts.css"; import "../src/App.css"; import { theme } from "../src/theme/theme"; import { cssVariablesResolver } from "../src/theme/cssVariables"; diff --git a/clients/web/README.md b/clients/web/README.md index 18adee52b7..84aa2dcb65 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -419,6 +419,8 @@ The origin allow-list only sees requests that carry an `Origin` header, and a br - The **port is not compared**, so a remapped port (`-p 127.0.0.1:8080:6274`) or a proxy on `:443` still matches; the name is what a re-pointed page cannot change. A request with no `Host` header is refused. - `GET /healthz` is exempt — it discloses nothing — so a probe that addresses the container by a service name keeps working. +**The MCP Apps sandbox listener (`MCP_SANDBOX_PORT`, default 6275) applies the same rules** to `/sandbox`, in prod and `--dev` alike, and additionally admits the host of `MCP_SANDBOX_FULL_ADDRESS` when one is set; a refused `Host` gets the same 403 (#2827). + `MCP_INSPECTOR_ALLOWED_HOSTS` is read by the prod server only; under `--dev`, Vite's own `server.allowedHosts` check applies instead (see below) — except over TLS, where Vite skips it because the certificate check already refuses a page loaded under another name. **Running the Inspector behind a `*.localhost` proxy.** A common local-dev shape gives each service a friendly name — `my-api.localhost`, `my-app.localhost`, `inspector.localhost` — instead of a set of ports. The Inspector works there, but the origin allow-list does not include those names by default, so set it explicitly: diff --git a/clients/web/index.html b/clients/web/index.html index 6248265253..cf13d2e11a 100644 --- a/clients/web/index.html +++ b/clients/web/index.html @@ -4,9 +4,6 @@ - - - MCP Inspector diff --git a/clients/web/package-lock.json b/clients/web/package-lock.json index 263de1b9e9..a80bb685a0 100644 --- a/clients/web/package-lock.json +++ b/clients/web/package-lock.json @@ -43,11 +43,11 @@ "@types/react-dom": "^19.2.3", "@types/react-syntax-highlighter": "^15.5.13", "@vitejs/plugin-react": "^6.0.0", - "@vitest/browser-playwright": "4.1.11", + "@vitest/browser-playwright": "4.1.12", "eslint-plugin-react-hooks": "^7.1.1", "eslint-plugin-react-refresh": "^0.5.2", "eslint-plugin-storybook": "^10.5.5", - "happy-dom": "^20.11.2", + "happy-dom": "^21.0.4", "playwright": "^1.58.2", "storybook": "^10.5.5", "tsdown": "^0.23.1", @@ -3134,9 +3134,9 @@ "license": "MIT" }, "node_modules/@types/ws": { - "version": "8.18.1", - "resolved": "https://registry.npmjs.org/@types/ws/-/ws-8.18.1.tgz", - "integrity": "sha512-ThVF6DCVhA8kUGy+aazFQ4kXQ7E1Ty7A3ypFOe0IcJV8O/M511G99AW24irKrW56Wt44yG9+ij8FaqoBGkuBXg==", + "version": "8.18.2", + "resolved": "https://registry.npmjs.org/@types/ws/-/ws-8.18.2.tgz", + "integrity": "sha512-67MQl+fpWKVTT1NYdnmo3U4sc/xPo/zQBncVnI74qmQa0z/b+1g6iYqNmGCPbxO+zz2aklb08a0oHfegiVd0/w==", "dev": true, "license": "MIT", "dependencies": { @@ -3321,15 +3321,15 @@ } }, "node_modules/@vitest/browser": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/browser/-/browser-4.1.11.tgz", - "integrity": "sha512-bwMovvAeuTFOK5kIFevw4VEf+1gVEICv4SYK4k3knJOxl6b1zEWud8mYKD73e1B0odAn174h1MofURy2TPWf3w==", + "version": "4.1.12", + "resolved": "https://registry.npmjs.org/@vitest/browser/-/browser-4.1.12.tgz", + "integrity": "sha512-Yl/6JIxJrcMlWMqKPoRkMhZy9sDpaAXdSJ+90hMVEJVE94tVysjp34SXTwCXJSIXjAtW2ha33qEjMeGG0G/Z0Q==", "dev": true, "license": "MIT", "dependencies": { "@blazediff/core": "1.9.1", - "@vitest/mocker": "4.1.11", - "@vitest/utils": "4.1.11", + "@vitest/mocker": "4.1.12", + "@vitest/utils": "4.1.12", "magic-string": "^0.30.21", "pngjs": "^7.0.0", "sirv": "^3.0.2", @@ -3340,18 +3340,18 @@ "url": "https://opencollective.com/vitest" }, "peerDependencies": { - "vitest": "4.1.11" + "vitest": "4.1.12" } }, "node_modules/@vitest/browser-playwright": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/browser-playwright/-/browser-playwright-4.1.11.tgz", - "integrity": "sha512-riLBxPqwnJ0lWs2DN2WeUfYeKLoAjbP2Xx8cLQdSddzMi20sksIa6K2mPz79DyMZKKVKH2ksOC2yJvtNcZg8cg==", + "version": "4.1.12", + "resolved": "https://registry.npmjs.org/@vitest/browser-playwright/-/browser-playwright-4.1.12.tgz", + "integrity": "sha512-bU/TcyiBwFUijaIltxKXxgpQmLu9njO/t6G9wiD2LjSFX6SfmyUOf+pOJ8N26KsvhFOHWfVdzNS6P62ab/ONlA==", "dev": true, "license": "MIT", "dependencies": { - "@vitest/browser": "4.1.11", - "@vitest/mocker": "4.1.11", + "@vitest/browser": "4.1.12", + "@vitest/mocker": "4.1.12", "tinyrainbow": "^3.1.0" }, "funding": { @@ -3359,7 +3359,7 @@ }, "peerDependencies": { "playwright": "*", - "vitest": "4.1.11" + "vitest": "4.1.12" }, "peerDependenciesMeta": { "playwright": { @@ -3436,13 +3436,13 @@ } }, "node_modules/@vitest/mocker": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/mocker/-/mocker-4.1.11.tgz", - "integrity": "sha512-2XJVD55d1o5AZous5CCGKS74g/riOj9odEt2bQpCVZeblHyHdnMeFl4jl0XjU21stf4mbjUkew2eXQZt65g5CQ==", + "version": "4.1.12", + "resolved": "https://registry.npmjs.org/@vitest/mocker/-/mocker-4.1.12.tgz", + "integrity": "sha512-fKShNpKZzupWZuY0z7egydvL0sqW4INEZBEExMBMAa6izlENEVHwoXgdaTCaBZ5fwlr4xf2o1wXC83n9RpO+NA==", "dev": true, "license": "MIT", "dependencies": { - "@vitest/spy": "4.1.11", + "@vitest/spy": "4.1.12", "estree-walker": "^3.0.3", "magic-string": "^0.30.21" }, @@ -3473,9 +3473,9 @@ } }, "node_modules/@vitest/pretty-format": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/pretty-format/-/pretty-format-4.1.11.tgz", - "integrity": "sha512-yiZzPbGTS9Sr/JpFl8zHrcIkAofNbFV6k21vIgQN/cY/oxZeXhJv5sc/MBJ5jFKWmWs+oJHw0UXLZjmf931+Vw==", + "version": "4.1.12", + "resolved": "https://registry.npmjs.org/@vitest/pretty-format/-/pretty-format-4.1.12.tgz", + "integrity": "sha512-XW9O8+Mxx4VpCtkYdMJLZQQwsVryP9wk7cPKDxtXFfwSak6yRh5+2fep/Y0XF/ElBvJHq8bcYdi6aeuai0opZA==", "dev": true, "license": "MIT", "dependencies": { @@ -3486,14 +3486,14 @@ } }, "node_modules/@vitest/runner": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/runner/-/runner-4.1.11.tgz", - "integrity": "sha512-LztvUgdwMNJMIkj3hQnnxiC2Xy1zNxq928W/xhjCLaNCzqTZOudjwbQf6v9IntZGPw132i2Lq2rgTRZHD3JHNw==", + "version": "4.1.12", + "resolved": "https://registry.npmjs.org/@vitest/runner/-/runner-4.1.12.tgz", + "integrity": "sha512-NKlVKCr5QnCe8jijdMnmkpHHU+WTqiLFR6G0pHji9uRjXK9xJl1k1nTw6F3pOOj2PDcsDt2GKQMI7wIsaT491w==", "dev": true, "license": "MIT", "peer": true, "dependencies": { - "@vitest/utils": "4.1.11", + "@vitest/utils": "4.1.12", "pathe": "^2.0.3" }, "funding": { @@ -3501,15 +3501,15 @@ } }, "node_modules/@vitest/snapshot": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/snapshot/-/snapshot-4.1.11.tgz", - "integrity": "sha512-pN7ikn1ON7h8ee4gIAp4AzyK+zBtJPzVbqOgu5LCEh4VaJVbPQcgYQYJIMGQPXVeJJq1fnfazis7a5pFNPahog==", + "version": "4.1.12", + "resolved": "https://registry.npmjs.org/@vitest/snapshot/-/snapshot-4.1.12.tgz", + "integrity": "sha512-ko1y1Qgb/slzQ9mr+b89Cuqud8WG1TKtsPD8r7ZUBdvVOGt85Af2HwCqHujrhwJq95hs/9RM7fcAPg57o1vlzw==", "dev": true, "license": "MIT", "peer": true, "dependencies": { - "@vitest/pretty-format": "4.1.11", - "@vitest/utils": "4.1.11", + "@vitest/pretty-format": "4.1.12", + "@vitest/utils": "4.1.12", "magic-string": "^0.30.21", "pathe": "^2.0.3" }, @@ -3518,9 +3518,9 @@ } }, "node_modules/@vitest/spy": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/spy/-/spy-4.1.11.tgz", - "integrity": "sha512-apNa/prQy2qCeywhnixOHPRCgGNhvg7T4Dapfl1GahLp/R+uhBm5cPyFoNVyqsNd2h1nJxL6BqqdIjiABL60YA==", + "version": "4.1.12", + "resolved": "https://registry.npmjs.org/@vitest/spy/-/spy-4.1.12.tgz", + "integrity": "sha512-RGq9x4JNGNTZHEJxvN0oSiJjeSCExm0n2gZKKJZHY9cX5AVA6M7VJILBT3ZJ/tJ/0J9uWT7fETV9/btoi//vSg==", "dev": true, "license": "MIT", "funding": { @@ -3528,13 +3528,13 @@ } }, "node_modules/@vitest/utils": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/utils/-/utils-4.1.11.tgz", - "integrity": "sha512-zTCVGpyFsGWBhllOyKlTw/vnr6D9qxsfSDyfbyZmTyjHw5N/VuvzHpHoQjm2ZJzn4RJgx5w4r7V0er69CmLgPQ==", + "version": "4.1.12", + "resolved": "https://registry.npmjs.org/@vitest/utils/-/utils-4.1.12.tgz", + "integrity": "sha512-Q3awXrwOfEV+qc3WmO5ONyE0yO7Q1gQ9/oyHxGLZ/pl7N3NO/ZztqQwKLuwMkV+CA1JYSIeO3gkHnRDIo4AVaA==", "dev": true, "license": "MIT", "dependencies": { - "@vitest/pretty-format": "4.1.11", + "@vitest/pretty-format": "4.1.12", "convert-source-map": "^2.0.0", "tinyrainbow": "^3.1.0" }, @@ -5004,9 +5004,9 @@ "optional": true }, "node_modules/happy-dom": { - "version": "20.14.5", - "resolved": "https://registry.npmjs.org/happy-dom/-/happy-dom-20.14.5.tgz", - "integrity": "sha512-x/RzkpWO40bTjIoT30iQtt64FLLmH/iRcUCN2X//bLx7H3ifkdfPXyqsro/OYtqzIAhiLMMA7mmiOR9C3NOKjQ==", + "version": "21.0.4", + "resolved": "https://registry.npmjs.org/happy-dom/-/happy-dom-21.0.4.tgz", + "integrity": "sha512-tFm+GaLBgo6o0awHnvHaNoraVo27qJ+2tZEtZ+cIa08hXbeitrxAJajLI05hsUy/2/YTKcUjQYzHe5mCcraLBw==", "dev": true, "license": "MIT", "dependencies": { @@ -8013,9 +8013,9 @@ "peer": true }, "node_modules/std-env": { - "version": "4.2.0", - "resolved": "https://registry.npmjs.org/std-env/-/std-env-4.2.0.tgz", - "integrity": "sha512-oCUKSupKTHX53EyjDtuZQ64pjLJ6yYCtpmEw0goYxtjG9KpbRe8KAsl2tBUGU9DyMcJ0RwJ8GqJAFzMXcXW1Rw==", + "version": "4.3.0", + "resolved": "https://registry.npmjs.org/std-env/-/std-env-4.3.0.tgz", + "integrity": "sha512-OtU/EgQ1kIm5KwqQpBC6ZEMXrZRui11w8zgfTWp8cdO9B8OaPsbA8bTHO2P+HNo1VlUTGMVBwPhydu6poeXiag==", "dev": true, "license": "MIT", "peer": true @@ -8872,20 +8872,20 @@ } }, "node_modules/vitest": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/vitest/-/vitest-4.1.11.tgz", - "integrity": "sha512-fhACrNXUidIbGSBr5FlbuBkO7VWC1ZyLl0DO4CU2DrQoAPxX84Ysxs+HeGQpii5lZWV1Q4gBZTTu49mF+A6Edw==", + "version": "4.1.12", + "resolved": "https://registry.npmjs.org/vitest/-/vitest-4.1.12.tgz", + "integrity": "sha512-5eWdz2yb3ajxWF3C8C4TjkXl7FGm3n2E/VlazALrQHzK7nYMSGFnCZto0bbPNl77AwkOZVLk0YnBAejUu0NiWA==", "dev": true, "license": "MIT", "peer": true, "dependencies": { - "@vitest/expect": "4.1.11", - "@vitest/mocker": "4.1.11", - "@vitest/pretty-format": "4.1.11", - "@vitest/runner": "4.1.11", - "@vitest/snapshot": "4.1.11", - "@vitest/spy": "4.1.11", - "@vitest/utils": "4.1.11", + "@vitest/expect": "4.1.12", + "@vitest/mocker": "4.1.12", + "@vitest/pretty-format": "4.1.12", + "@vitest/runner": "4.1.12", + "@vitest/snapshot": "4.1.12", + "@vitest/spy": "4.1.12", + "@vitest/utils": "4.1.12", "es-module-lexer": "^2.0.0", "expect-type": "^1.3.0", "magic-string": "^0.30.21", @@ -8913,12 +8913,12 @@ "@edge-runtime/vm": "*", "@opentelemetry/api": "^1.9.0", "@types/node": "^20.0.0 || ^22.0.0 || >=24.0.0", - "@vitest/browser-playwright": "4.1.11", - "@vitest/browser-preview": "4.1.11", - "@vitest/browser-webdriverio": "4.1.11", - "@vitest/coverage-istanbul": "4.1.11", - "@vitest/coverage-v8": "4.1.11", - "@vitest/ui": "4.1.11", + "@vitest/browser-playwright": "4.1.12", + "@vitest/browser-preview": "4.1.12", + "@vitest/browser-webdriverio": "^4.1.11", + "@vitest/coverage-istanbul": "4.1.12", + "@vitest/coverage-v8": "4.1.12", + "@vitest/ui": "4.1.12", "happy-dom": "*", "jsdom": "*", "vite": "^6.0.0 || ^7.0.0 || ^8.0.0" @@ -8963,17 +8963,17 @@ } }, "node_modules/vitest/node_modules/@vitest/expect": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/expect/-/expect-4.1.11.tgz", - "integrity": "sha512-VX2x5vNJXET47KAFzwERI+KRMtTTCSWTfSMKsW7JsUsXV4psq++e3DvZpuTDOpHcxytiDs6p2nhVb2tVDiiUYw==", + "version": "4.1.12", + "resolved": "https://registry.npmjs.org/@vitest/expect/-/expect-4.1.12.tgz", + "integrity": "sha512-vFlTt+YEu9WBpNjdGayNmlMVzrWPi1+sv853/7t2FU7hpO5gAFBKHzQmkCq5Xjpr+evegF2994LQT3hF3mRlqQ==", "dev": true, "license": "MIT", "peer": true, "dependencies": { "@standard-schema/spec": "^1.1.0", "@types/chai": "^5.2.2", - "@vitest/spy": "4.1.11", - "@vitest/utils": "4.1.11", + "@vitest/spy": "4.1.12", + "@vitest/utils": "4.1.12", "chai": "^6.2.2", "tinyrainbow": "^3.1.0" }, @@ -8982,9 +8982,9 @@ } }, "node_modules/vitest/node_modules/chai": { - "version": "6.2.2", - "resolved": "https://registry.npmjs.org/chai/-/chai-6.2.2.tgz", - "integrity": "sha512-NUPRluOfOiTKBKvWPtSD4PhFvWCqOi0BGStNWs57X9js7XGTprSmFoz5F0tWhR4WPjNeR9jXqdC7/UpSJTnlRg==", + "version": "6.3.0", + "resolved": "https://registry.npmjs.org/chai/-/chai-6.3.0.tgz", + "integrity": "sha512-XWAtwJ6OHO+tj0EKCs0Y2UamnyOxseZWltU4x2U2wh8g4AigdjwvtUjvLP2tqkA/avxHEtzxNaqGq/YGNwckKg==", "dev": true, "license": "MIT", "peer": true, diff --git a/clients/web/package.json b/clients/web/package.json index 6249a34b55..ef9a491342 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -71,11 +71,11 @@ "@types/react-dom": "^19.2.3", "@types/react-syntax-highlighter": "^15.5.13", "@vitejs/plugin-react": "^6.0.0", - "@vitest/browser-playwright": "4.1.11", + "@vitest/browser-playwright": "4.1.12", "eslint-plugin-react-hooks": "^7.1.1", "eslint-plugin-react-refresh": "^0.5.2", "eslint-plugin-storybook": "^10.5.5", - "happy-dom": "^20.11.2", + "happy-dom": "^21.0.4", "playwright": "^1.58.2", "storybook": "^10.5.5", "tsdown": "^0.23.1", @@ -86,7 +86,7 @@ "@types/node": "^24.12.4", "esbuild": "^0.28.2", "open": "^11.0.4", - "vitest": "4.1.11", + "vitest": "4.1.12", "rolldown": "1.2.13" } } diff --git a/clients/web/public/fonts/Fredoka-OFL.txt b/clients/web/public/fonts/Fredoka-OFL.txt new file mode 100644 index 0000000000..791a98de78 --- /dev/null +++ b/clients/web/public/fonts/Fredoka-OFL.txt @@ -0,0 +1,93 @@ +Copyright 2016 The Fredoka Project Authors (https://github.com/hafontia/Fredoka-One) + +This Font Software is licensed under the SIL Open Font License, Version 1.1. +This license is copied below, and is also available with a FAQ at: +http://scripts.sil.org/OFL + + +----------------------------------------------------------- +SIL OPEN FONT LICENSE Version 1.1 - 26 February 2007 +----------------------------------------------------------- + +PREAMBLE +The goals of the Open Font License (OFL) are to stimulate worldwide +development of collaborative font projects, to support the font creation +efforts of academic and linguistic communities, and to provide a free and +open framework in which fonts may be shared and improved in partnership +with others. + +The OFL allows the licensed fonts to be used, studied, modified and +redistributed freely as long as they are not sold by themselves. The +fonts, including any derivative works, can be bundled, embedded, +redistributed and/or sold with any software provided that any reserved +names are not used by derivative works. The fonts and derivatives, +however, cannot be released under any other type of license. The +requirement for fonts to remain under this license does not apply +to any document created using the fonts or their derivatives. + +DEFINITIONS +"Font Software" refers to the set of files released by the Copyright +Holder(s) under this license and clearly marked as such. This may +include source files, build scripts and documentation. + +"Reserved Font Name" refers to any names specified as such after the +copyright statement(s). + +"Original Version" refers to the collection of Font Software components as +distributed by the Copyright Holder(s). + +"Modified Version" refers to any derivative made by adding to, deleting, +or substituting -- in part or in whole -- any of the components of the +Original Version, by changing formats or by porting the Font Software to a +new environment. + +"Author" refers to any designer, engineer, programmer, technical +writer or other person who contributed to the Font Software. + +PERMISSION & CONDITIONS +Permission is hereby granted, free of charge, to any person obtaining +a copy of the Font Software, to use, study, copy, merge, embed, modify, +redistribute, and sell modified and unmodified copies of the Font +Software, subject to the following conditions: + +1) Neither the Font Software nor any of its individual components, +in Original or Modified Versions, may be sold by itself. + +2) Original or Modified Versions of the Font Software may be bundled, +redistributed and/or sold with any software, provided that each copy +contains the above copyright notice and this license. These can be +included either as stand-alone text files, human-readable headers or +in the appropriate machine-readable metadata fields within text or +binary files as long as those fields can be easily viewed by the user. + +3) No Modified Version of the Font Software may use the Reserved Font +Name(s) unless explicit written permission is granted by the corresponding +Copyright Holder. This restriction only applies to the primary font name as +presented to the users. + +4) The name(s) of the Copyright Holder(s) or the Author(s) of the Font +Software shall not be used to promote, endorse or advertise any +Modified Version, except to acknowledge the contribution(s) of the +Copyright Holder(s) and the Author(s) or with their explicit written +permission. + +5) The Font Software, modified or unmodified, in part or in whole, +must be distributed entirely under this license, and must not be +distributed under any other license. The requirement for fonts to +remain under this license does not apply to any document created +using the Font Software. + +TERMINATION +This license becomes null and void if any of the above conditions are +not met. + +DISCLAIMER +THE FONT SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO ANY WARRANTIES OF +MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT +OF COPYRIGHT, PATENT, TRADEMARK, OR OTHER RIGHT. IN NO EVENT SHALL THE +COPYRIGHT HOLDER BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +INCLUDING ANY GENERAL, SPECIAL, INDIRECT, INCIDENTAL, OR CONSEQUENTIAL +DAMAGES, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +FROM, OUT OF THE USE OR INABILITY TO USE THE FONT SOFTWARE OR FROM +OTHER DEALINGS IN THE FONT SOFTWARE. diff --git a/clients/web/public/fonts/RobotoMono-OFL.txt b/clients/web/public/fonts/RobotoMono-OFL.txt new file mode 100644 index 0000000000..8e7df338d3 --- /dev/null +++ b/clients/web/public/fonts/RobotoMono-OFL.txt @@ -0,0 +1,93 @@ +Copyright 2015 The Roboto Mono Project Authors (https://github.com/googlefonts/robotomono) + +This Font Software is licensed under the SIL Open Font License, Version 1.1. +This license is copied below, and is also available with a FAQ at: +https://openfontlicense.org + + +----------------------------------------------------------- +SIL OPEN FONT LICENSE Version 1.1 - 26 February 2007 +----------------------------------------------------------- + +PREAMBLE +The goals of the Open Font License (OFL) are to stimulate worldwide +development of collaborative font projects, to support the font creation +efforts of academic and linguistic communities, and to provide a free and +open framework in which fonts may be shared and improved in partnership +with others. + +The OFL allows the licensed fonts to be used, studied, modified and +redistributed freely as long as they are not sold by themselves. The +fonts, including any derivative works, can be bundled, embedded, +redistributed and/or sold with any software provided that any reserved +names are not used by derivative works. The fonts and derivatives, +however, cannot be released under any other type of license. The +requirement for fonts to remain under this license does not apply +to any document created using the fonts or their derivatives. + +DEFINITIONS +"Font Software" refers to the set of files released by the Copyright +Holder(s) under this license and clearly marked as such. This may +include source files, build scripts and documentation. + +"Reserved Font Name" refers to any names specified as such after the +copyright statement(s). + +"Original Version" refers to the collection of Font Software components as +distributed by the Copyright Holder(s). + +"Modified Version" refers to any derivative made by adding to, deleting, +or substituting -- in part or in whole -- any of the components of the +Original Version, by changing formats or by porting the Font Software to a +new environment. + +"Author" refers to any designer, engineer, programmer, technical +writer or other person who contributed to the Font Software. + +PERMISSION & CONDITIONS +Permission is hereby granted, free of charge, to any person obtaining +a copy of the Font Software, to use, study, copy, merge, embed, modify, +redistribute, and sell modified and unmodified copies of the Font +Software, subject to the following conditions: + +1) Neither the Font Software nor any of its individual components, +in Original or Modified Versions, may be sold by itself. + +2) Original or Modified Versions of the Font Software may be bundled, +redistributed and/or sold with any software, provided that each copy +contains the above copyright notice and this license. These can be +included either as stand-alone text files, human-readable headers or +in the appropriate machine-readable metadata fields within text or +binary files as long as those fields can be easily viewed by the user. + +3) No Modified Version of the Font Software may use the Reserved Font +Name(s) unless explicit written permission is granted by the corresponding +Copyright Holder. This restriction only applies to the primary font name as +presented to the users. + +4) The name(s) of the Copyright Holder(s) or the Author(s) of the Font +Software shall not be used to promote, endorse or advertise any +Modified Version, except to acknowledge the contribution(s) of the +Copyright Holder(s) and the Author(s) or with their explicit written +permission. + +5) The Font Software, modified or unmodified, in part or in whole, +must be distributed entirely under this license, and must not be +distributed under any other license. The requirement for fonts to +remain under this license does not apply to any document created +using the Font Software. + +TERMINATION +This license becomes null and void if any of the above conditions are +not met. + +DISCLAIMER +THE FONT SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO ANY WARRANTIES OF +MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT +OF COPYRIGHT, PATENT, TRADEMARK, OR OTHER RIGHT. IN NO EVENT SHALL THE +COPYRIGHT HOLDER BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +INCLUDING ANY GENERAL, SPECIAL, INDIRECT, INCIDENTAL, OR CONSEQUENTIAL +DAMAGES, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +FROM, OUT OF THE USE OR INABILITY TO USE THE FONT SOFTWARE OR FROM +OTHER DEALINGS IN THE FONT SOFTWARE. diff --git a/clients/web/server/run-web.ts b/clients/web/server/run-web.ts index 7df47c9bde..0f1fd64e7b 100644 --- a/clients/web/server/run-web.ts +++ b/clients/web/server/run-web.ts @@ -20,6 +20,7 @@ import { parseProtocolEra, type ServerConfigOptions, } from "../../../core/mcp/node/config.ts"; +import { adHocServerName } from "../../../core/mcp/serverList.ts"; import { buildWebServerConfig, type WebServerConfig, @@ -42,22 +43,6 @@ function ensureStdioCwd(config: MCPServerConfig): MCPServerConfig { return config; } -/** - * Derive a readable catalog id for an ad-hoc server seeded from CLI flags: - * the URL host for HTTP/SSE, or the command basename for stdio. Falls back to - * "server" so the id is always a non-empty `mcpServers` map key. - */ -function deriveSeedServerId(config: MCPServerConfig): string { - if (config.type === "sse" || config.type === "streamable-http") { - try { - return new URL(config.url).host || "server"; - } catch { - return "server"; - } - } - return config.command?.split(/[\\/]/).pop() || "server"; -} - export async function runWeb(argv: string[]): Promise { const program = new Command(); @@ -256,7 +241,7 @@ export async function runWeb(argv: string[]): Promise { entry.protocolEra = opts.protocolEra; } initialServers = { - mcpServers: { [deriveSeedServerId(config)]: entry }, + mcpServers: { [adHocServerName(config)]: entry }, }; writable = false; } else if (hasHeaders) { diff --git a/clients/web/server/sandbox-controller.ts b/clients/web/server/sandbox-controller.ts index d55c063b07..fb6bf32572 100644 --- a/clients/web/server/sandbox-controller.ts +++ b/clients/web/server/sandbox-controller.ts @@ -12,6 +12,7 @@ import { isAllInterfacesHost, } from "../../../core/node/hostUrl.ts"; import { DEFAULT_BIND_HOST } from "./resolve-bind-host.js"; +import { ALLOWED_HOSTS_ENV, createHostAllowList } from "./host-allow-list.js"; const __dirname = dirname(fileURLToPath(import.meta.url)); @@ -37,6 +38,35 @@ export interface SandboxControllerOptions { * so a failed bind still reports no sandbox rather than a dead URL. */ publicUrl?: string; + /** + * Extra hostnames the listener accepts in `Host` — the main server's + * `MCP_INSPECTOR_ALLOWED_HOSTS` list. The sandbox applies the same `Host` + * allow-list as the main server (#2827): bind host, the hosts of + * `allowedOrigins`, these, plus the host of {@link publicUrl} (the name a + * reverse proxy forwards to this listener). + */ + allowedHosts?: string[]; +} + +/** + * The request paths that serve the proxy page: `/sandbox` or `/sandbox/`, + * ignoring any query string (#2827). A cache-buster or tracking parameter on + * the URL names the same resource, so refusing it 404'd a valid request. + */ +export function isSandboxPath(url: string | undefined): boolean { + const path = (url ?? "").split("?", 1)[0]; + return path === "/sandbox" || path === "/sandbox/"; +} + +/** The hostname of an advertised public URL, or none if it will not parse. */ +function publicUrlHosts(publicUrl: string | undefined): string[] { + if (!publicUrl) return []; + try { + return [new URL(publicUrl).hostname]; + } catch { + // public-address.ts validated it already; an unparsable value admits nothing. + return []; + } } // Loopback fallback. NB: no `http://[::1]:*` — a **bracketed IPv6 literal is not @@ -212,7 +242,22 @@ export function createSandboxController( // `localhost` — a name resolves to one address family and would reintroduce // the #1951 split (web on IPv4, sandbox on IPv6) for any future call site // that omits `host`. Both call sites pass `config.sandboxHost` today. - const { port, host = DEFAULT_BIND_HOST, allowedOrigins, publicUrl } = options; + const { + port, + host = DEFAULT_BIND_HOST, + allowedOrigins, + publicUrl, + allowedHosts, + } = options; + // The same DNS-rebinding defence the main server applies (see + // host-allow-list.ts). The proxy page carries no token, but it is the + // embedder of every MCP App the Inspector renders, so a page under a + // rebinding name has no business loading it either. + const isAllowedHost = createHostAllowList({ + bindHost: host, + allowedOrigins: allowedOrigins ?? [], + extraHosts: [...(allowedHosts ?? []), ...publicUrlHosts(publicUrl)], + }); let server: Server | null = null; let sandboxUrl: string | null = null; // Tracked separately from the URL: an advertised public URL carries the @@ -265,21 +310,29 @@ export function createSandboxController( }; server = createServer((req, res) => { - if ( - req.method !== "GET" || - (req.url !== "/sandbox" && req.url !== "/sandbox/") - ) { + if (!isAllowedHost(req.headers.host)) { + res.writeHead(403, { "Content-Type": "text/plain" }); + res.end( + `Forbidden: Host "${req.headers.host ?? ""}" is not allowed. To serve the sandbox under this name, add it to ${ALLOWED_HOSTS_ENV} (or to ALLOWED_ORIGINS).`, + ); + return; + } + // HEAD is answered like GET minus the body (RFC 9110 §9.3.2), so a + // probe (`curl -I`) sees the same status and headers a browser does. + const isHead = req.method === "HEAD"; + if ((req.method !== "GET" && !isHead) || !isSandboxPath(req.url)) { res.writeHead(404, { "Content-Type": "text/plain" }); res.end("Not Found"); return; } res.writeHead(200, { "Content-Type": "text/html; charset=utf-8", + "Content-Length": Buffer.byteLength(sandboxHtml), "Cache-Control": "no-store, no-cache, must-revalidate", Pragma: "no-cache", "Content-Security-Policy": SANDBOX_PROXY_CSP, }); - res.end(sandboxHtml); + res.end(isHead ? undefined : sandboxHtml); }); // A taken port used to mean "no sandbox at all", which was tolerable // only because the default was dynamic and therefore never collided. diff --git a/clients/web/server/server.ts b/clients/web/server/server.ts index 7c3cf4ea45..95fcbbd9f2 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -35,6 +35,15 @@ import type { WebServerHandle } from "./types.js"; export type { WebServerHandle }; +/** + * Whether a request path belongs to the API namespace the SPA fallback must + * not answer: `/api` exactly, or anything under `/api/`. A bare + * `startsWith("/api")` also swallowed `/apixyz` (#2827). + */ +export function isApiPath(path: string): boolean { + return path === "/api" || path.startsWith("/api/"); +} + const __filename = fileURLToPath(import.meta.url); const __dirname = dirname(__filename); @@ -50,6 +59,7 @@ export async function startHonoServer( host: config.sandboxHost, allowedOrigins: config.allowedOrigins, publicUrl: config.sandboxPublicUrl, + allowedHosts: config.allowedHosts, }); await sandboxController.start(); // The dedicated origin apps declaring `_meta.ui.domain` are served from @@ -152,7 +162,9 @@ export async function startHonoServer( // index.html — not the raw file — so bookmarks and hand-typed reloads at // those paths get the token global too, matching the `/` route. app.get("*", async (c) => { - if (c.req.path.startsWith("/api")) { + // `/api` itself or anything under `/api/` — never a sibling such as + // `/apixyz`, which is an ordinary deep link and gets the SPA (#2827). + if (isApiPath(c.req.path)) { return c.notFound(); } try { diff --git a/clients/web/server/vite-hono-plugin.ts b/clients/web/server/vite-hono-plugin.ts index 079f3fb598..f5677d35bc 100644 --- a/clients/web/server/vite-hono-plugin.ts +++ b/clients/web/server/vite-hono-plugin.ts @@ -77,6 +77,7 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { host: config.sandboxHost, allowedOrigins: config.allowedOrigins, publicUrl: config.sandboxPublicUrl, + allowedHosts: config.allowedHosts, }); await sandboxController.start(); // The dedicated origin apps declaring `_meta.ui.domain` are served from diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index 0b2bf229a3..16b048b2ed 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -448,6 +448,35 @@ export function defaultAllowedOrigins( return [webOrigin(scheme, h, port)]; } +/** + * The shortest custom API token accepted without a warning (#2827) — the + * length of a 128-bit secret in hex. The token generated when none is set is + * `randomBytes(32)` in hex (64 characters), so this only ever fires for one an + * operator chose. + */ +export const MIN_RECOMMENDED_AUTH_TOKEN_LENGTH = 32; + +/** + * The startup warning for a custom `MCP_INSPECTOR_API_TOKEN` (or its legacy + * name) shorter than {@link MIN_RECOMMENDED_AUTH_TOKEN_LENGTH}, or `null` when + * there is nothing to say: no custom token (one is generated) or one long + * enough. A warning rather than a refusal — a short token still gates + * `/api/*`, and a deployment that set one deliberately (a test rig, a fixed + * token in a script) keeps working; what it should not do is pass silently, + * since the token is the only thing between a page that can reach the port + * and every MCP server the Inspector can spawn. + */ +export function weakAuthTokenWarning(token: string): string | null { + if (token === "" || token.length >= MIN_RECOMMENDED_AUTH_TOKEN_LENGTH) { + return null; + } + return ( + `Warning: the custom API token (${API_SERVER_ENV_VARS.AUTH_TOKEN}) is ${token.length} character${token.length === 1 ? "" : "s"} long; ` + + `use at least ${MIN_RECOMMENDED_AUTH_TOKEN_LENGTH} random characters (e.g. \`openssl rand -hex 32\`), ` + + `or unset it to have one generated.` + ); +} + /** * Build WebServerConfig from process.env and optional initial MCP server config. * Used by the launcher runner, Vite dev (`buildWebServerConfigFromEnv`), and prod standalone. @@ -491,6 +520,9 @@ export function buildWebServerConfig( (process.env[LEGACY_AUTH_TOKEN_ENV] as string | undefined) ?? ""); + const weakToken = weakAuthTokenWarning(authToken); + if (weakToken) console.warn(weakToken); + const sandboxPort = resolveSandboxPort(); // The app-origin listener starts BEFORE the web server binds (both in prod // `startHonoServer` and in the Vite plugin), so a configured collision is not diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 15ea1de32f..2f10f8df74 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -44,6 +44,7 @@ import { useLastPersistedSettings } from "./hooks/useLastPersistedSettings"; import { usePaginatedListsOverride } from "./hooks/usePaginatedListsOverride"; import { useValueChange } from "./hooks/useValueChange"; import { useThemeToggle } from "./hooks/useThemeToggle"; +import { usePageHideRelease } from "./hooks/usePageHideRelease"; import { useTabUiState } from "./hooks/useTabUiState"; import { useSessionRef } from "./hooks/useSessionRef"; import { @@ -390,6 +391,9 @@ function App() { // screen renders from. Both hooks subscribe to the live client's events and // own their own toast-id bookkeeping. useProgressToasts(inspectorClient); + // Release the backend session (and any stdio child) when the tab is closed + // or reloaded, rather than leaving it to the backend's grace timer (#2824). + usePageHideRelease(inspectorClient); const { progressByTaskId, resetTaskProgress, activeToolCallTaskIdRef } = useTaskToasts(inspectorClient); diff --git a/clients/web/src/components/groups/AppControls/AppControls.tsx b/clients/web/src/components/groups/AppControls/AppControls.tsx index f157cc79d5..3d421ebf9f 100644 --- a/clients/web/src/components/groups/AppControls/AppControls.tsx +++ b/clients/web/src/components/groups/AppControls/AppControls.tsx @@ -12,6 +12,10 @@ import { ListChangedIndicator } from "../../elements/ListChangedIndicator/ListCh import { AppListItem } from "../AppListItem/AppListItem"; import { useScrollMemory } from "../../../hooks/useScrollMemory"; +// h2 under the page's visually hidden h1 (#2827), sized as the h4 it was so +// the look is unchanged; sections below it are h3 (axe `heading-order`). +const PanelHeading = Title.withProps({ order: 2, size: "h4" }); + export interface AppControlsProps { tools: Tool[]; selectedName?: string; @@ -55,7 +59,7 @@ export function AppControls({ return ( - MCP Apps ({tools.length}) + MCP Apps ({tools.length})