Merge remote-tracking branch 'origin/v2/main' into v2/fix/2297-401-response-body
Signed-off-by: cliffhall <cliff@futurescale.com> # Conflicts: # core/mcp/node/transport.ts
This commit is contained in:
cliffhall
commit
860b8d27ad
23 changed files with 4402 additions and 100 deletions
|
|
@ -40,6 +40,7 @@ import type {
|
|||
} from "../types.js";
|
||||
import { INACTIVE_SUBSCRIPTION_STREAM_STATE } from "../types.js";
|
||||
import type { MalformedListItem } from "../listSalvage.js";
|
||||
import type { ConnectionDiagnostics } from "../connectionDiagnostics.js";
|
||||
import type { SkillEntry, SkillResource } from "../skillsSchemas.js";
|
||||
import type { SkillsExtensionSupport } from "../skills.js";
|
||||
import type { JsonValue } from "../../json/jsonUtils.js";
|
||||
|
|
@ -321,6 +322,21 @@ export class FakeInspectorClient
|
|||
return this.discoverResult;
|
||||
}
|
||||
|
||||
private connectionDiagnostics: ConnectionDiagnostics = {
|
||||
capturedAt: 0,
|
||||
outstandingRequests: [],
|
||||
};
|
||||
|
||||
getConnectionDiagnostics(): ConnectionDiagnostics {
|
||||
return this.connectionDiagnostics;
|
||||
}
|
||||
|
||||
/** Test helper: set the diagnostics snapshot and emit the change (#2318). */
|
||||
setConnectionDiagnostics(diagnostics: ConnectionDiagnostics): void {
|
||||
this.connectionDiagnostics = diagnostics;
|
||||
this.dispatchTypedEvent("connectionDiagnosticsChange", diagnostics);
|
||||
}
|
||||
|
||||
resourceSubscriptionStreamState: ResourceSubscriptionStreamState =
|
||||
INACTIVE_SUBSCRIPTION_STREAM_STATE;
|
||||
|
||||
|
|
|
|||
252
core/mcp/connectionDiagnostics.ts
Normal file
252
core/mcp/connectionDiagnostics.ts
Normal file
|
|
@ -0,0 +1,252 @@
|
|||
/**
|
||||
* Connection diagnostics: what the client is still waiting on, when it last
|
||||
* heard back, and the state of the server-to-client notification stream — and
|
||||
* the one place that turns the SDK's bare `Request timed out` into a message
|
||||
* that reports those facts (#2318).
|
||||
*
|
||||
* The motivating case is #2187: a server that serves one request at a time.
|
||||
* The standalone `GET /mcp` stream the client opens after `initialize`
|
||||
* occupies that single slot, so `initialize` succeeds and every request after
|
||||
* it hangs — and the Inspector's only word on it was `Request timed out`,
|
||||
* which names the symptom the user can already see. Every fact needed to
|
||||
* suspect the cause was already in-process at that moment: the stream had been
|
||||
* open for the whole session and delivered nothing, `tools/list` had been
|
||||
* outstanding for 60s, and nothing had been answered since `initialize`.
|
||||
*
|
||||
* Everything here is pure. `InspectorClient` owns the bookkeeping (it sees
|
||||
* every outbound request, every inbound response and every tracked fetch) and
|
||||
* hands a snapshot to {@link annotateRequestTimeout}; the UI clients render the
|
||||
* same snapshot through `getConnectionDiagnostics()`.
|
||||
*
|
||||
* Deliberately factual: the summary reports what the connection's state *is*
|
||||
* and lets the reader draw the conclusion. It does not assert a diagnosis the
|
||||
* Inspector cannot verify from its own side.
|
||||
*/
|
||||
|
||||
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
|
||||
|
||||
/** A request this client sent that no response has arrived for. */
|
||||
export interface OutstandingRequest {
|
||||
id: string | number;
|
||||
method: string;
|
||||
/** Epoch ms at which the request went out. */
|
||||
sentAt: number;
|
||||
}
|
||||
|
||||
/** The most recent response received to one of this client's requests. */
|
||||
export interface LastResponse {
|
||||
/** The method of the request it answered. */
|
||||
method: string;
|
||||
/** Epoch ms at which it arrived. */
|
||||
receivedAt: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* The most recent long-lived server-to-client stream the transport opened —
|
||||
* the standalone `GET` on Streamable HTTP, or the primary event stream on
|
||||
* legacy SSE. Absent on stdio, and until the first such stream is opened.
|
||||
*/
|
||||
export interface NotificationStreamState {
|
||||
/** The request URL, as recorded (query already redacted). */
|
||||
url: string;
|
||||
/** Epoch ms at which the response headers arrived and the stream opened. */
|
||||
openedAt: number;
|
||||
/** SSE events delivered so far — messages, not keepalive comments. */
|
||||
eventCount: number;
|
||||
/** Epoch ms at which the stream ended, once it has. */
|
||||
closedAt?: number;
|
||||
}
|
||||
|
||||
export interface ConnectionDiagnostics {
|
||||
/**
|
||||
* Epoch ms at which this snapshot was taken. Every duration derived from
|
||||
* the snapshot ("sent 60s ago", "open for 4m12s") is measured against this
|
||||
* clock rather than against the moment of rendering, so the snapshot is
|
||||
* self-contained: a message written from it, a component rendering it and
|
||||
* a test asserting on it all read the same numbers.
|
||||
*/
|
||||
capturedAt: number;
|
||||
/**
|
||||
* Every request sent this session that has received no response, oldest
|
||||
* first. A request the SDK gave up on (a timeout, a cancel) stays here: it
|
||||
* is still unanswered, which is the fact being reported.
|
||||
*/
|
||||
outstandingRequests: OutstandingRequest[];
|
||||
lastResponse?: LastResponse;
|
||||
notificationStream?: NotificationStreamState;
|
||||
}
|
||||
|
||||
/**
|
||||
* The data the SDK attaches to a per-request timeout: `{ timeout }` in ms.
|
||||
* Narrowed here rather than imported because the SDK types it `unknown`.
|
||||
*/
|
||||
interface RequestTimeoutData {
|
||||
timeout: number;
|
||||
}
|
||||
|
||||
function isRequestTimeoutData(value: unknown): value is RequestTimeoutData {
|
||||
return (
|
||||
typeof value === "object" &&
|
||||
value !== null &&
|
||||
typeof (value as { timeout?: unknown }).timeout === "number"
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether `err` is the SDK's per-request timeout: an `SdkError` with
|
||||
* `RequestTimeout` and a numeric `timeout` in its data.
|
||||
*
|
||||
* The data check is not decoration. `Protocol.request` reuses the
|
||||
* `RequestTimeout` code for two other rejections that are not timeouts at
|
||||
* all — an abort through `options.signal` whose reason is not an `SdkError`
|
||||
* (the Cancel button), and "Maximum total timeout exceeded" — and neither
|
||||
* carries `{ timeout }`. Only the genuine per-request timeout does.
|
||||
*/
|
||||
export function isRequestTimeoutError(
|
||||
err: unknown,
|
||||
): err is SdkError & { data: RequestTimeoutData } {
|
||||
return (
|
||||
SdkError.isInstance(err) &&
|
||||
err.code === SdkErrorCode.RequestTimeout &&
|
||||
isRequestTimeoutData(err.data)
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* A compact human duration: `850ms`, `12s`, `4m12s`, `1h02m`. Sub-second
|
||||
* values are shown in ms so a very short timeout still reads as a number
|
||||
* rather than as `0s`.
|
||||
*/
|
||||
export function formatDuration(ms: number): string {
|
||||
const clamped = Math.max(0, ms);
|
||||
if (clamped < 1000) return `${Math.round(clamped)}ms`;
|
||||
const totalSeconds = Math.round(clamped / 1000);
|
||||
const hours = Math.floor(totalSeconds / 3600);
|
||||
const minutes = Math.floor((totalSeconds % 3600) / 60);
|
||||
const seconds = totalSeconds % 60;
|
||||
if (hours > 0) return `${hours}h${String(minutes).padStart(2, "0")}m`;
|
||||
if (minutes > 0) return `${minutes}m${String(seconds).padStart(2, "0")}s`;
|
||||
return `${seconds}s`;
|
||||
}
|
||||
|
||||
/**
|
||||
* How many outstanding requests the summary names individually before
|
||||
* collapsing the rest into "and N more". Enough to show the shape of a stall
|
||||
* (the one everything is queued behind, plus what queued behind it) without
|
||||
* turning a toast into a page.
|
||||
*/
|
||||
export const MAX_LISTED_OUTSTANDING_REQUESTS = 5;
|
||||
|
||||
/**
|
||||
* `GET /mcp` for a stream URL: method plus path only. The origin is the
|
||||
* server the user is already connected to, and the query string (already
|
||||
* redacted at tracking time) adds nothing to a one-line summary. Falls back to
|
||||
* the raw string for an unparseable URL rather than dropping the clause.
|
||||
*/
|
||||
export function describeStreamEndpoint(url: string): string {
|
||||
try {
|
||||
return `GET ${new URL(url).pathname}`;
|
||||
} catch {
|
||||
return `GET ${url}`;
|
||||
}
|
||||
}
|
||||
|
||||
function plural(count: number, noun: string): string {
|
||||
return `${count} ${noun}${count === 1 ? "" : "s"}`;
|
||||
}
|
||||
|
||||
function describeOutstanding(
|
||||
requests: OutstandingRequest[],
|
||||
now: number,
|
||||
): string {
|
||||
if (requests.length === 0) return "No requests are unanswered.";
|
||||
const listed = requests
|
||||
.slice(0, MAX_LISTED_OUTSTANDING_REQUESTS)
|
||||
.map((r) => `${r.method} (sent ${formatDuration(now - r.sentAt)} ago)`);
|
||||
const rest = requests.length - listed.length;
|
||||
const tail = rest > 0 ? `, and ${rest} more` : "";
|
||||
const verb = requests.length === 1 ? "is" : "are";
|
||||
return `${plural(requests.length, "request")} ${verb} unanswered: ${listed.join(", ")}${tail}.`;
|
||||
}
|
||||
|
||||
function describeLastResponse(
|
||||
lastResponse: LastResponse | undefined,
|
||||
now: number,
|
||||
): string {
|
||||
if (!lastResponse) return "No response has been received this session.";
|
||||
return `Last response received ${formatDuration(now - lastResponse.receivedAt)} ago (${lastResponse.method}).`;
|
||||
}
|
||||
|
||||
function describeStream(
|
||||
stream: NotificationStreamState | undefined,
|
||||
now: number,
|
||||
): string | undefined {
|
||||
if (!stream) return undefined;
|
||||
const endpoint = describeStreamEndpoint(stream.url);
|
||||
const events = plural(stream.eventCount, "event");
|
||||
if (stream.closedAt !== undefined) {
|
||||
return `Notification stream (${endpoint}) closed ${formatDuration(now - stream.closedAt)} ago after ${formatDuration(stream.closedAt - stream.openedAt)}, ${events} delivered.`;
|
||||
}
|
||||
return `Notification stream (${endpoint}) open for ${formatDuration(now - stream.openedAt)}, ${events} delivered.`;
|
||||
}
|
||||
|
||||
/**
|
||||
* One paragraph describing the connection's state as of the snapshot's own
|
||||
* clock: outstanding requests, the last response, and the notification
|
||||
* stream when there is one. Each clause is a sentence, so a consumer that
|
||||
* wants only one of them can split on the sentence boundary rather than
|
||||
* re-deriving it.
|
||||
*/
|
||||
export function describeConnectionDiagnostics(
|
||||
diagnostics: ConnectionDiagnostics,
|
||||
): string {
|
||||
const now = diagnostics.capturedAt;
|
||||
const parts = [
|
||||
describeOutstanding(diagnostics.outstandingRequests, now),
|
||||
describeLastResponse(diagnostics.lastResponse, now),
|
||||
describeStream(diagnostics.notificationStream, now),
|
||||
];
|
||||
return parts.filter((p): p is string => p !== undefined).join(" ");
|
||||
}
|
||||
|
||||
/**
|
||||
* The data carried by an annotated timeout: the SDK's own `{ timeout }`, plus
|
||||
* the method that timed out and the snapshot the message was written from.
|
||||
* Exposed as a type so a consumer that wants the structured form (a details
|
||||
* modal, a CLI's JSON output) can read it off `error.data` instead of parsing
|
||||
* the message.
|
||||
*/
|
||||
export interface AnnotatedRequestTimeoutData extends RequestTimeoutData {
|
||||
method: string;
|
||||
diagnostics: ConnectionDiagnostics;
|
||||
}
|
||||
|
||||
/**
|
||||
* Rebuild the SDK's per-request timeout with a message that says which
|
||||
* request timed out and what the connection looked like at that moment.
|
||||
*
|
||||
* Anything that is not that exact error is returned untouched — the SDK's
|
||||
* other `RequestTimeout` rejections (see {@link isRequestTimeoutError}), and
|
||||
* every other failure — so this is safe to apply blindly on the rejection
|
||||
* path of every request.
|
||||
*
|
||||
* The result is a fresh `SdkError` with the same code, so an `isInstance` or
|
||||
* code check downstream sees exactly what it saw before; only the message and
|
||||
* the data change. The original is kept as `cause`.
|
||||
*/
|
||||
export function annotateRequestTimeout(
|
||||
err: unknown,
|
||||
method: string,
|
||||
diagnostics: ConnectionDiagnostics,
|
||||
): unknown {
|
||||
if (!isRequestTimeoutError(err)) return err;
|
||||
const data: AnnotatedRequestTimeoutData = {
|
||||
...err.data,
|
||||
method,
|
||||
diagnostics,
|
||||
};
|
||||
const message = `Request timed out after ${formatDuration(err.data.timeout)} (${method}). ${describeConnectionDiagnostics(diagnostics)}`;
|
||||
const annotated = new SdkError(SdkErrorCode.RequestTimeout, message, data);
|
||||
annotated.cause = err;
|
||||
return annotated;
|
||||
}
|
||||
|
|
@ -1,4 +1,4 @@
|
|||
import type { FetchRequestEntryBase } from "./types.js";
|
||||
import type { FetchRequestEntryBase, FetchStreamState } from "./types.js";
|
||||
|
||||
/**
|
||||
* Header names whose values are replaced with `REDACTED_HEADER_VALUE` before a
|
||||
|
|
@ -198,11 +198,18 @@ export function redactBody(
|
|||
return body;
|
||||
}
|
||||
|
||||
/** Case-insensitive lookup of a header value from a plain header record. */
|
||||
function findHeader(
|
||||
headers: Record<string, string>,
|
||||
/**
|
||||
* Case-insensitive lookup of a header value from a plain header record.
|
||||
* Exported because a recorded entry's header casing is host-dependent (Node
|
||||
* lowercases, happy-dom preserves the wire casing), so any consumer reading
|
||||
* one back — the Network UI, the connection diagnostics — needs the same
|
||||
* tolerance the tracker itself has.
|
||||
*/
|
||||
export function findHeader(
|
||||
headers: Record<string, string> | undefined,
|
||||
name: string,
|
||||
): string | undefined {
|
||||
if (!headers) return undefined;
|
||||
const target = name.toLowerCase();
|
||||
for (const [key, value] of Object.entries(headers)) {
|
||||
if (key.toLowerCase() === target) return value;
|
||||
|
|
@ -223,11 +230,13 @@ export function isLongLivedStreamResponse(
|
|||
method: string,
|
||||
contentType: string | null | undefined,
|
||||
): boolean {
|
||||
if (method !== "GET") return false;
|
||||
if (method.toUpperCase() !== "GET") return false;
|
||||
if (!contentType) return false;
|
||||
// Media types are case-insensitive (RFC 9110 §8.3.1); a server may send
|
||||
// `Text/Event-Stream` and still be sending an event stream.
|
||||
const type = contentType.toLowerCase();
|
||||
return (
|
||||
contentType.includes("text/event-stream") ||
|
||||
contentType.includes("application/x-ndjson")
|
||||
type.includes("text/event-stream") || type.includes("application/x-ndjson")
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -333,6 +342,194 @@ export interface FetchTrackingCallbacks {
|
|||
* never invoked and the entry's responseBody stays undefined.
|
||||
*/
|
||||
updateResponseBody?: (id: string, responseBody: string) => void;
|
||||
/**
|
||||
* Called for a long-lived stream response — the one case `updateResponseBody`
|
||||
* never covers — each time an SSE event is delivered on it, and once more
|
||||
* when it ends (with `closedAt` set). Lets the consumer show that the stream
|
||||
* is open and what it has carried, which is the whole diagnostic when a
|
||||
* server serializes requests behind it (#2318).
|
||||
*/
|
||||
updateStream?: (id: string, stream: FetchStreamState) => void;
|
||||
/**
|
||||
* How often, at most, `updateStream` reports a moving event count, in ms.
|
||||
* The first report (open, zero events) and the close are immediate; counts
|
||||
* in between are coalesced so a server pushing hundreds of notifications a
|
||||
* second costs one update per window rather than one per event — each
|
||||
* update re-publishes the whole fetch log and, on the web client, crosses
|
||||
* the backend-to-browser event stream. Defaults to
|
||||
* `DEFAULT_STREAM_UPDATE_INTERVAL_MS`; `0` reports every event, for tests.
|
||||
*/
|
||||
streamUpdateIntervalMs?: number;
|
||||
}
|
||||
|
||||
/** See `FetchTrackingCallbacks.streamUpdateIntervalMs`. */
|
||||
export const DEFAULT_STREAM_UPDATE_INTERVAL_MS = 250;
|
||||
|
||||
/**
|
||||
* The framing a long-lived stream's events arrive in — the two content types
|
||||
* `isLongLivedStreamResponse` accepts. SSE dispatches on a blank line;
|
||||
* NDJSON dispatches on every line.
|
||||
*/
|
||||
export type LongLivedStreamFraming = "sse" | "ndjson";
|
||||
|
||||
/** The framing for a long-lived stream's content type. */
|
||||
export function longLivedStreamFraming(
|
||||
contentType: string | null | undefined,
|
||||
): LongLivedStreamFraming {
|
||||
return contentType?.toLowerCase().includes("application/x-ndjson")
|
||||
? "ndjson"
|
||||
: "sse";
|
||||
}
|
||||
|
||||
/**
|
||||
* Count the events on a long-lived stream without buffering it, and report
|
||||
* when it ends.
|
||||
*
|
||||
* Reads a `clone()` of the response through a reader and discards the bytes,
|
||||
* so the transport keeps consuming the original at its own pace (the same
|
||||
* tee the bounded-body path relies on). Counting follows the framing the
|
||||
* transport's own parser applies. For SSE a blank line dispatches the block
|
||||
* before it, and only a block carrying a `data` field is an event —
|
||||
* keepalive comments (`: ping`) and bare `event:`/`id:` lines are not; a
|
||||
* partial block at end-of-stream is dropped, exactly as the SSE spec and the
|
||||
* SDK's `EventSourceParserStream` (which has no flush) drop it. For NDJSON
|
||||
* every non-blank line is one event.
|
||||
*
|
||||
* Reports once as soon as watching starts (zero events, open), once per event,
|
||||
* and once more on every exit from the read loop — a clean end-of-stream, a
|
||||
* network error, or the transport aborting the fetch on disconnect (which
|
||||
* errors both tee branches) — with `closedAt` set. The reader is never cancelled from here:
|
||||
* cancelling one branch of a tee does not release the source, and
|
||||
* `ReadableStream.cancel()` adopts the source's promise, which on a teardown
|
||||
* path may never settle (the same hazard `core/mcp/remote/node/server.ts`
|
||||
* documents at its own `cancel()` call).
|
||||
*/
|
||||
function watchLongLivedStream(
|
||||
response: Response,
|
||||
id: string,
|
||||
framing: LongLivedStreamFraming,
|
||||
callbacks: FetchTrackingCallbacks,
|
||||
): void {
|
||||
const listener = callbacks.updateStream;
|
||||
if (!listener) return;
|
||||
// Every report goes through here: the listener is consumer code (a relay
|
||||
// sink that may already be closed, a state store), and it runs from three
|
||||
// places none of which may throw outward — synchronously inside the fetch
|
||||
// wrapper (the open), from a timer (a coalesced count), and off the
|
||||
// discarded watcher promise (the close). A throw is the listener's
|
||||
// problem; the stream's bookkeeping carries on.
|
||||
const report = (state: FetchStreamState): void => {
|
||||
try {
|
||||
listener(id, state);
|
||||
} catch {
|
||||
// See above.
|
||||
}
|
||||
};
|
||||
const intervalMs =
|
||||
callbacks.streamUpdateIntervalMs ?? DEFAULT_STREAM_UPDATE_INTERVAL_MS;
|
||||
let body: ReadableStream<Uint8Array> | null;
|
||||
try {
|
||||
body = response.clone().body;
|
||||
} catch {
|
||||
// Clone failed (consumed body, transport quirks) — there is nothing to
|
||||
// watch; the entry stays as tracked with no stream state.
|
||||
return;
|
||||
}
|
||||
if (!body) return;
|
||||
const reader = body.getReader();
|
||||
const decoder = new TextDecoder();
|
||||
let eventCount = 0;
|
||||
let pending = "";
|
||||
let blockHasData = false;
|
||||
// A moving count is reported at most once per interval (see
|
||||
// `streamUpdateIntervalMs`); the open and the close go out at once.
|
||||
let reportTimer: ReturnType<typeof setTimeout> | null = null;
|
||||
const reportCount = (): void => {
|
||||
if (intervalMs <= 0) {
|
||||
report({ eventCount });
|
||||
return;
|
||||
}
|
||||
if (reportTimer !== null) return;
|
||||
reportTimer = setTimeout(() => {
|
||||
reportTimer = null;
|
||||
report({ eventCount });
|
||||
}, intervalMs);
|
||||
};
|
||||
// Announce the stream as open before a byte arrives. A stream that never
|
||||
// delivers anything is the #2187 tell, and it would otherwise be the one
|
||||
// stream with no state to show — the count only moves on an event.
|
||||
report({ eventCount });
|
||||
|
||||
const consumeLine = (line: string): void => {
|
||||
if (framing === "ndjson") {
|
||||
if (line.trim() === "") return;
|
||||
eventCount += 1;
|
||||
reportCount();
|
||||
return;
|
||||
}
|
||||
if (line === "") {
|
||||
if (blockHasData) {
|
||||
eventCount += 1;
|
||||
blockHasData = false;
|
||||
reportCount();
|
||||
}
|
||||
return;
|
||||
}
|
||||
// A field line is `name[:value]`; `data` with no colon is still a data
|
||||
// field per the spec, so match the name rather than the `data:` prefix.
|
||||
if (line === "data" || line.startsWith("data:")) blockHasData = true;
|
||||
};
|
||||
|
||||
// Split off every complete line in `pending`. The SSE grammar (and the
|
||||
// SDK's parser) accept LF, CRLF and a bare CR as a line ending, so all three
|
||||
// end a line here; a CR that is the last byte so far is held back, since
|
||||
// the LF of a CRLF pair may still be in flight in the next chunk.
|
||||
const consumeCompleteLines = (): void => {
|
||||
for (;;) {
|
||||
const cr = pending.indexOf("\r");
|
||||
const lf = pending.indexOf("\n");
|
||||
const end = cr === -1 ? lf : lf === -1 ? cr : Math.min(cr, lf);
|
||||
if (end === -1) return;
|
||||
if (pending[end] === "\r") {
|
||||
if (end === pending.length - 1) return;
|
||||
const width = pending[end + 1] === "\n" ? 2 : 1;
|
||||
consumeLine(pending.slice(0, end));
|
||||
pending = pending.slice(end + width);
|
||||
} else {
|
||||
consumeLine(pending.slice(0, end));
|
||||
pending = pending.slice(end + 1);
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
const pump = async (): Promise<void> => {
|
||||
try {
|
||||
for (;;) {
|
||||
const { value, done } = await reader.read();
|
||||
if (done) break;
|
||||
pending += decoder.decode(value, { stream: true });
|
||||
consumeCompleteLines();
|
||||
}
|
||||
// A CR held back as a possible half of a CRLF is, at end-of-stream, a
|
||||
// whole line ending — `data: x\r\r` is a complete event. Anything after
|
||||
// it is a partial block and is dropped, as above.
|
||||
if (pending.endsWith("\r")) consumeLine(pending.slice(0, -1));
|
||||
} catch {
|
||||
// An errored or aborted stream has still ended — fall through to report
|
||||
// the close; the entry's `error` field is for a fetch that never got a
|
||||
// response, which this one did.
|
||||
}
|
||||
if (reportTimer !== null) {
|
||||
clearTimeout(reportTimer);
|
||||
reportTimer = null;
|
||||
}
|
||||
report({ eventCount, closedAt: new Date() });
|
||||
};
|
||||
// Deliberately not awaited: the fetch wrapper has to return the response
|
||||
// now, and this watcher lives as long as the stream does. It owns its own
|
||||
// failures — the read loop is wrapped in a `try`, and every report goes
|
||||
// through the non-throwing `report` above — so the promise cannot reject.
|
||||
void pump();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -357,7 +554,12 @@ export function createFetchTracker(
|
|||
: input instanceof URL
|
||||
? input.toString()
|
||||
: input.url;
|
||||
const method = init?.method || "GET";
|
||||
// The method decides the long-lived-stream classification below, so it
|
||||
// has to be the one the request actually carries: a `Request` input
|
||||
// carries its own, and HTTP methods compare case-insensitively.
|
||||
const method = (
|
||||
init?.method ?? (input instanceof Request ? input.method : "GET")
|
||||
).toUpperCase();
|
||||
|
||||
// Extract headers, redacting sensitive values BEFORE they reach any
|
||||
// downstream sink (logger, in-memory list, persisted session storage).
|
||||
|
|
@ -494,6 +696,18 @@ export function createFetchTracker(
|
|||
|
||||
callbacks.trackRequest?.(entry);
|
||||
|
||||
// A long-lived stream is watched instead of read: its body never ends,
|
||||
// but its event count and its eventual close are exactly the state a
|
||||
// stalled connection is diagnosed from (#2318).
|
||||
if (isLongLivedStream && response.body && !response.bodyUsed) {
|
||||
watchLongLivedStream(
|
||||
response,
|
||||
id,
|
||||
longLivedStreamFraming(response.headers.get("content-type")),
|
||||
callbacks,
|
||||
);
|
||||
}
|
||||
|
||||
// Kick off a fire-and-forget read of the cloned body. The clone is an
|
||||
// independent tee'd stream so the transport keeps consuming the
|
||||
// original at its own pace. When the read resolves we patch the entry
|
||||
|
|
|
|||
|
|
@ -13,6 +13,7 @@ import type {
|
|||
MessageOrigin,
|
||||
FetchRequestEntry,
|
||||
FetchRequestEntryBase,
|
||||
FetchStreamState,
|
||||
InspectorServerSettings,
|
||||
ResourceReadInvocation,
|
||||
ResourceTemplateReadInvocation,
|
||||
|
|
@ -265,10 +266,31 @@ import {
|
|||
import type { TokenRevocationOutcome } from "../auth/revocation.js";
|
||||
import type { OAuthTokens } from "@modelcontextprotocol/client";
|
||||
import { silentLogger, type InspectorLogger } from "../logging/logger.js";
|
||||
import { createFetchTracker } from "./fetchTracking.js";
|
||||
import {
|
||||
createFetchTracker,
|
||||
findHeader,
|
||||
isLongLivedStreamResponse,
|
||||
} from "./fetchTracking.js";
|
||||
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
|
||||
import {
|
||||
annotateRequestTimeout,
|
||||
isRequestTimeoutError,
|
||||
type ConnectionDiagnostics,
|
||||
type LastResponse,
|
||||
type NotificationStreamState,
|
||||
type OutstandingRequest,
|
||||
} from "./connectionDiagnostics.js";
|
||||
import { OAuthManager, type OAuthManagerConfig } from "./oauthManager.js";
|
||||
import { RemoteClientTransport } from "./remote/remoteClientTransport.js";
|
||||
|
||||
/**
|
||||
* The notification stream the diagnostics are following, plus the fetch entry
|
||||
* id that ties later `fetchRequestStreamUpdate`s back to it (#2318).
|
||||
*/
|
||||
interface TrackedNotificationStream extends NotificationStreamState {
|
||||
id: string;
|
||||
}
|
||||
|
||||
/** Internal record for a receiver task (server polls us for status/result). */
|
||||
interface ReceiverTaskRecord {
|
||||
task: Task;
|
||||
|
|
@ -490,13 +512,23 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
private outputValidator: AjvJsonSchemaValidator | null = null;
|
||||
private transport: Transport | MessageTrackingTransport | null = null;
|
||||
private baseTransport: Transport | null = null;
|
||||
// Correlation for `markResponseRejected` (#1953): the method of each
|
||||
// outbound request still awaiting a response, and — once one is answered —
|
||||
// the id of the most recently answered request per method. Entries are
|
||||
// dropped as responses arrive, so this holds at most one id per method
|
||||
// rather than growing with the session.
|
||||
private outboundRequestMethods = new Map<string | number, string>();
|
||||
// Every outbound request still awaiting a response, keyed by JSON-RPC id.
|
||||
// Serves two readers: the `markResponseRejected` correlation (#1953), which
|
||||
// needs the method, and the connection diagnostics (#2318), which need the
|
||||
// method and when it went out. Entries are dropped as responses arrive; a
|
||||
// request the SDK gave up on (a timeout, a cancel) stays until the session
|
||||
// resets, because it is still unanswered and that is what the diagnostics
|
||||
// report.
|
||||
private outstandingRequests = new Map<string | number, OutstandingRequest>();
|
||||
// The id of the most recently answered request per method, for
|
||||
// `markResponseRejected` (#1953). At most one id per method, so it does not
|
||||
// grow with the session.
|
||||
private lastAnsweredRequestByMethod = new Map<string, string | number>();
|
||||
// Connection diagnostics (#2318): the most recent response to one of our
|
||||
// requests, and the most recent long-lived transport stream (the standalone
|
||||
// `GET` on Streamable HTTP). Both per-session, reset with the maps above.
|
||||
private lastResponse: LastResponse | undefined;
|
||||
private notificationStream: TrackedNotificationStream | undefined;
|
||||
/** True when the cached transport was built with an OAuth authProvider attached. */
|
||||
private transportHasAuthProvider = false;
|
||||
/** Dedupes concurrent ambient auth challenges (reason + scopes). */
|
||||
|
|
@ -1021,6 +1053,47 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
this.clientInfo,
|
||||
Object.keys(clientOptions).length > 0 ? clientOptions : undefined,
|
||||
);
|
||||
this.annotateSdkRequestTimeouts(this.client);
|
||||
}
|
||||
|
||||
/**
|
||||
* Decorate the SDK Client's `request` so a per-request timeout rejects with
|
||||
* the connection's state at that moment instead of a bare `Request timed
|
||||
* out` (#2318).
|
||||
*
|
||||
* `Protocol.request` is the one funnel every SDK verb goes through —
|
||||
* `listTools`, `callTool`, `readResource`, the handshake's `initialize`,
|
||||
* and this class's own raw `client.request` calls all reach the wire
|
||||
* through it — so decorating it here covers the whole surface without a
|
||||
* `catch` at each of the several dozen call sites, and covers the next
|
||||
* verb added without anyone remembering to. The SDK offers no hook on the
|
||||
* rejection path: the timeout error is constructed and thrown inside
|
||||
* `request()` itself.
|
||||
*
|
||||
* Replaced on the instance rather than through a subclass because
|
||||
* `request` is declared with two overloads (spec method with a keyed
|
||||
* result, custom method with an explicit schema), and an `override` would
|
||||
* have to restate both against types that move with every SDK release. One
|
||||
* implementation signature serves both at runtime; the single cast on the
|
||||
* way back is what the overloads cost.
|
||||
*/
|
||||
private annotateSdkRequestTimeouts(client: Client): void {
|
||||
type SdkRequest = Client["request"];
|
||||
const sdkRequest: SdkRequest = client.request.bind(client);
|
||||
const decorated = async (
|
||||
...args: Parameters<SdkRequest>
|
||||
): Promise<unknown> => {
|
||||
try {
|
||||
return await sdkRequest(...args);
|
||||
} catch (err) {
|
||||
throw annotateRequestTimeout(
|
||||
err,
|
||||
args[0].method,
|
||||
this.getConnectionDiagnostics(),
|
||||
);
|
||||
}
|
||||
};
|
||||
client.request = decorated as SdkRequest;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -1058,7 +1131,12 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
return {
|
||||
trackRequest: (message: JSONRPCRequest, origin: MessageOrigin) => {
|
||||
if (origin === "client") {
|
||||
this.outboundRequestMethods.set(message.id, message.method);
|
||||
this.outstandingRequests.set(message.id, {
|
||||
id: message.id,
|
||||
method: message.method,
|
||||
sentAt: Date.now(),
|
||||
});
|
||||
this.dispatchConnectionDiagnosticsChange();
|
||||
}
|
||||
const entry: MessageEntry = {
|
||||
id: crypto.randomUUID(),
|
||||
|
|
@ -1069,6 +1147,20 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
};
|
||||
this.dispatchTypedEvent("message", entry);
|
||||
},
|
||||
trackSendFailure: (message: JSONRPCRequest, error: unknown) => {
|
||||
// A send that failed because the request timed out is not a send
|
||||
// that failed: the browser's remote transport awaits the response
|
||||
// inside `send`, so its relay timeout surfaces here — the request
|
||||
// reached the server and went unanswered, which is the state to
|
||||
// keep reporting. Every other failure means the frame never reached
|
||||
// the wire, and leaving it would report a request the server never
|
||||
// saw as unanswered, in every timeout message and Connection Info
|
||||
// row, until the session resets (#2318).
|
||||
if (isRequestTimeoutError(error)) return;
|
||||
if (this.outstandingRequests.delete(message.id)) {
|
||||
this.dispatchConnectionDiagnosticsChange();
|
||||
}
|
||||
},
|
||||
trackResponse: (
|
||||
message: JSONRPCResultResponse | JSONRPCErrorResponse,
|
||||
origin: MessageOrigin,
|
||||
|
|
@ -1080,11 +1172,13 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
// frame answers no specific request, so it is skipped.
|
||||
const responseId = message.id;
|
||||
if (origin === "server" && responseId !== undefined) {
|
||||
const method = this.outboundRequestMethods.get(responseId);
|
||||
this.outboundRequestMethods.delete(responseId);
|
||||
const method = this.outstandingRequests.get(responseId)?.method;
|
||||
this.outstandingRequests.delete(responseId);
|
||||
if (method !== undefined) {
|
||||
this.lastAnsweredRequestByMethod.set(method, responseId);
|
||||
this.lastResponse = { method, receivedAt: Date.now() };
|
||||
}
|
||||
this.dispatchConnectionDiagnosticsChange();
|
||||
}
|
||||
const entry: MessageEntry = {
|
||||
id: crypto.randomUUID(),
|
||||
|
|
@ -1943,8 +2037,11 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
// start-clean path rather than in `disconnect()` for the reason documented
|
||||
// above: one route out (`onerror` with no `onclose`) tears down nothing
|
||||
// (#1953).
|
||||
this.outboundRequestMethods.clear();
|
||||
this.outstandingRequests.clear();
|
||||
this.lastAnsweredRequestByMethod.clear();
|
||||
this.lastResponse = undefined;
|
||||
this.notificationStream = undefined;
|
||||
this.dispatchConnectionDiagnosticsChange();
|
||||
// Per-session for the same reason: both name entries of the PREVIOUS
|
||||
// server's list. Cleared here as well as in `disconnect()` because the
|
||||
// route out that tears down nothing (`onerror` with no `onclose`) would
|
||||
|
|
@ -2113,10 +2210,14 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
},
|
||||
onFetchRequest: (entry: FetchRequestEntryBase) => {
|
||||
this.dispatchFetchRequest({ ...entry, category: "transport" });
|
||||
this.noteTransportStream(entry);
|
||||
},
|
||||
onFetchResponseBody: (id: string, body: string) => {
|
||||
this.dispatchFetchRequestBodyUpdate(id, body);
|
||||
},
|
||||
onFetchStreamUpdate: (id: string, stream: FetchStreamState) => {
|
||||
this.dispatchFetchRequestStreamUpdate(id, stream);
|
||||
},
|
||||
...(this.serverSettings && { settings: this.serverSettings }),
|
||||
};
|
||||
if (this.isHttpOAuthConfig() && oauthManager) {
|
||||
|
|
@ -2857,8 +2958,19 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
const raw = await new Promise<unknown>((resolve, reject) => {
|
||||
const timer = setTimeout(() => {
|
||||
this.pendingRawWireRequests.delete(id);
|
||||
// The same error, with the same annotation, as an SDK request that
|
||||
// times out: this path bypasses `Protocol.request`, so it builds the
|
||||
// SDK's own timeout shape and runs it through the decorator's
|
||||
// annotation by hand — a raw-wire caller sees one kind of timeout,
|
||||
// not two (#2318).
|
||||
reject(
|
||||
new Error(`Raw request "${method}" timed out after ${timeoutMs} ms`),
|
||||
annotateRequestTimeout(
|
||||
new SdkError(SdkErrorCode.RequestTimeout, "Request timed out", {
|
||||
timeout: timeoutMs,
|
||||
}),
|
||||
method,
|
||||
this.getConnectionDiagnostics(),
|
||||
),
|
||||
);
|
||||
}, timeoutMs);
|
||||
this.pendingRawWireRequests.set(id, { resolve, reject, timer });
|
||||
|
|
@ -2868,7 +2980,17 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
clearTimeout(pending.timer);
|
||||
this.pendingRawWireRequests.delete(id);
|
||||
}
|
||||
reject(err instanceof Error ? err : new Error(String(err)));
|
||||
// The browser's remote transport awaits the response inside `send`,
|
||||
// so its relay wait can expire here first, as the SDK's timeout
|
||||
// shape; annotate it exactly as the local timer above does (a
|
||||
// non-timeout error passes through untouched).
|
||||
reject(
|
||||
annotateRequestTimeout(
|
||||
err instanceof Error ? err : new Error(String(err)),
|
||||
method,
|
||||
this.getConnectionDiagnostics(),
|
||||
),
|
||||
);
|
||||
});
|
||||
});
|
||||
return resultSchema.parse(raw);
|
||||
|
|
@ -5989,6 +6111,87 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
this.dispatchTypedEvent("fetchRequestBodyUpdate", { id, responseBody });
|
||||
}
|
||||
|
||||
private dispatchFetchRequestStreamUpdate(
|
||||
id: string,
|
||||
stream: FetchStreamState,
|
||||
): void {
|
||||
this.dispatchTypedEvent("fetchRequestStreamUpdate", { id, stream });
|
||||
// The diagnostics follow only the stream they are currently watching;
|
||||
// an update for an older one (a reconnected stream's predecessor
|
||||
// closing late) changes nothing they report.
|
||||
if (this.notificationStream?.id !== id) return;
|
||||
this.notificationStream = {
|
||||
...this.notificationStream,
|
||||
eventCount: stream.eventCount,
|
||||
...(stream.closedAt && { closedAt: stream.closedAt.getTime() }),
|
||||
};
|
||||
this.dispatchConnectionDiagnosticsChange();
|
||||
}
|
||||
|
||||
/**
|
||||
* Adopt a transport fetch as the notification stream when it is one — a
|
||||
* `GET` answered with an unbounded event stream — so the diagnostics can
|
||||
* say how long it has been open and what it has delivered (#2318). The
|
||||
* newest such stream wins: the SDK reopens the standalone `GET` after a
|
||||
* drop, and the current one is the one a stalled request is queued behind.
|
||||
*/
|
||||
private noteTransportStream(entry: FetchRequestEntryBase): void {
|
||||
if (
|
||||
!isLongLivedStreamResponse(
|
||||
entry.method,
|
||||
findHeader(entry.responseHeaders, "content-type"),
|
||||
)
|
||||
) {
|
||||
return;
|
||||
}
|
||||
this.notificationStream = {
|
||||
id: entry.id,
|
||||
url: entry.url,
|
||||
// The entry's `timestamp` is when the request went out; the stream is
|
||||
// open from when the response headers arrived, `duration` later.
|
||||
openedAt: entry.timestamp.getTime() + (entry.duration ?? 0),
|
||||
eventCount: 0,
|
||||
};
|
||||
this.dispatchConnectionDiagnosticsChange();
|
||||
}
|
||||
|
||||
/**
|
||||
* What this client is still waiting on, when it last heard back, and the
|
||||
* state of the notification stream (#2318). A fresh snapshot per call —
|
||||
* the same one `annotateRequestTimeout` writes a timeout message from, and
|
||||
* the one `connectionDiagnosticsChange` carries.
|
||||
*/
|
||||
getConnectionDiagnostics(): ConnectionDiagnostics {
|
||||
return {
|
||||
capturedAt: Date.now(),
|
||||
// Copies, not the tracked records: the snapshot is handed to callers
|
||||
// and carried on errors, and a mutation there must not reach the
|
||||
// correlation state.
|
||||
outstandingRequests: [...this.outstandingRequests.values()].map(
|
||||
(request) => ({ ...request }),
|
||||
),
|
||||
...(this.lastResponse && { lastResponse: { ...this.lastResponse } }),
|
||||
...(this.notificationStream && {
|
||||
notificationStream: this.publicStreamState(this.notificationStream),
|
||||
}),
|
||||
};
|
||||
}
|
||||
|
||||
/** The stream state minus the entry id, which is bookkeeping, not a fact. */
|
||||
private publicStreamState(
|
||||
stream: TrackedNotificationStream,
|
||||
): NotificationStreamState {
|
||||
const { id: _id, ...state } = stream;
|
||||
return state;
|
||||
}
|
||||
|
||||
private dispatchConnectionDiagnosticsChange(): void {
|
||||
this.dispatchTypedEvent(
|
||||
"connectionDiagnosticsChange",
|
||||
this.getConnectionDiagnostics(),
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Get current session ID (from OAuth state authId)
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ import type {
|
|||
MessageEntry,
|
||||
StderrLogEntry,
|
||||
FetchRequestEntry,
|
||||
FetchStreamState,
|
||||
PromptGetInvocation,
|
||||
ResourceReadInvocation,
|
||||
ResourceTemplateReadInvocation,
|
||||
|
|
@ -28,6 +29,7 @@ import type {
|
|||
RequestMetadata,
|
||||
} from "./types.js";
|
||||
import type { MalformedListItem } from "./listSalvage.js";
|
||||
import type { ConnectionDiagnostics } from "./connectionDiagnostics.js";
|
||||
import type {
|
||||
Tool,
|
||||
ServerCapabilities,
|
||||
|
|
@ -92,6 +94,20 @@ export interface InspectorClientEventMap {
|
|||
fetchRequest: FetchRequestEntry;
|
||||
/** Fired when an in-flight fetch's response body is read asynchronously. */
|
||||
fetchRequestBodyUpdate: { id: string; responseBody: string };
|
||||
/**
|
||||
* Fired as a tracked long-lived stream (the standalone `GET` on Streamable
|
||||
* HTTP, the legacy SSE event stream) delivers events and when it ends. The
|
||||
* body of such a stream is never captured, so this is the log's only view
|
||||
* of its lifetime (#2318).
|
||||
*/
|
||||
fetchRequestStreamUpdate: { id: string; stream: FetchStreamState };
|
||||
/**
|
||||
* Fired whenever the connection diagnostics snapshot changes — a request
|
||||
* goes out, a response comes back, the notification stream opens, delivers
|
||||
* an event or closes, or a new session resets it. Carries the fresh
|
||||
* snapshot; `getConnectionDiagnostics()` returns the same one (#2318).
|
||||
*/
|
||||
connectionDiagnosticsChange: ConnectionDiagnostics;
|
||||
/**
|
||||
* Fired whenever the client transitions `status` to `"error"` from a path
|
||||
* that is NOT an awaited promise — i.e. a mid-session transport failure
|
||||
|
|
|
|||
|
|
@ -40,6 +40,7 @@ import type {
|
|||
} from "@modelcontextprotocol/client";
|
||||
import type { JsonValue } from "../json/jsonUtils.js";
|
||||
import type { MalformedListItem } from "./listSalvage.js";
|
||||
import type { ConnectionDiagnostics } from "./connectionDiagnostics.js";
|
||||
import type { InspectorClientEventTarget } from "./inspectorClientEventTarget.js";
|
||||
import type { SkillEntry } from "./skillsSchemas.js";
|
||||
import type { SkillsExtensionSupport } from "./skills.js";
|
||||
|
|
@ -78,6 +79,14 @@ export interface InspectorClientProtocol extends InspectorClientEventTarget {
|
|||
getDiscoverResult(): DiscoverResult | undefined;
|
||||
getServerSettings(): InspectorServerSettings | undefined;
|
||||
setServerSettings(settings: InspectorServerSettings): void;
|
||||
/**
|
||||
* What the client is still waiting on, when it last heard back, and the
|
||||
* state of the notification stream — the snapshot a request timeout is
|
||||
* annotated with (#2318). Optional so existing test doubles satisfy the
|
||||
* interface without implementing it; `connectionDiagnosticsChange` fires
|
||||
* whenever it changes.
|
||||
*/
|
||||
getConnectionDiagnostics?(): ConnectionDiagnostics;
|
||||
getAppRendererClient(): AppRendererClient | null;
|
||||
|
||||
// Connection control
|
||||
|
|
|
|||
|
|
@ -24,6 +24,19 @@ export interface MessageTrackingCallbacks {
|
|||
message: JSONRPCNotification,
|
||||
origin: MessageOrigin,
|
||||
) => void;
|
||||
/**
|
||||
* An outgoing request the base transport failed to send (the connection
|
||||
* closed, the fetch failed before the frame reached the wire). Fires after
|
||||
* `trackRequest` already recorded it, so a consumer keeping "requests still
|
||||
* awaiting a response" can roll that entry back — a request that never went
|
||||
* out is not unanswered (#2318). Not fired when the caller aborted the
|
||||
* request through `requestSignal` *during* the send: the modern transport
|
||||
* aborts an in-flight request's own stream to cancel it (and the SDK does
|
||||
* the same on a per-request timeout), and a request that was sent and then
|
||||
* given up on is exactly the unanswered kind. A signal already aborted
|
||||
* before the send began is a failed send like any other.
|
||||
*/
|
||||
trackSendFailure?: (message: JSONRPCRequest, error: unknown) => void;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -95,7 +108,19 @@ export class MessageTrackingTransport implements Transport {
|
|||
"client",
|
||||
);
|
||||
} else if ("method" in message) {
|
||||
this.callbacks.trackRequest?.(message as JSONRPCRequest, "client");
|
||||
const request = message as JSONRPCRequest;
|
||||
this.callbacks.trackRequest?.(request, "client");
|
||||
// A signal already aborted here means the frame will not go out at
|
||||
// all — that is a failed send. One that becomes aborted while the
|
||||
// send is in flight is a cancellation of a request that may well
|
||||
// have reached the server, which stays unanswered.
|
||||
const abortedBeforeSend = options?.requestSignal?.aborted === true;
|
||||
return this.baseTransport.send(message, options).catch((err) => {
|
||||
if (abortedBeforeSend || !options?.requestSignal?.aborted) {
|
||||
this.callbacks.trackSendFailure?.(request, err);
|
||||
}
|
||||
throw err;
|
||||
});
|
||||
}
|
||||
} else if ("method" in message) {
|
||||
this.callbacks.trackNotification?.(
|
||||
|
|
|
|||
|
|
@ -8,7 +8,8 @@ import type {
|
|||
SseServerConfig,
|
||||
StreamableHttpServerConfig,
|
||||
} from "../types.js";
|
||||
import { normalizeServerType } from "../serverList.js";
|
||||
import { isProtocolEra, normalizeServerType } from "../serverList.js";
|
||||
import type { ServerProtocolEra } from "../types.js";
|
||||
import { toRecord } from "../../json/jsonUtils.js";
|
||||
|
||||
/**
|
||||
|
|
@ -86,6 +87,20 @@ export function parseHeaderPair(
|
|||
return { ...previous, [key]: val };
|
||||
}
|
||||
|
||||
/**
|
||||
* Validate a `--protocol-era` value. Used as the Commander option coerce for
|
||||
* all three clients, so an ad-hoc launch can pick the era without writing an
|
||||
* `mcp.json` entry for it (#2208). Pure function; no Commander dependency.
|
||||
*/
|
||||
export function parseProtocolEra(value: string): ServerProtocolEra {
|
||||
if (!isProtocolEra(value)) {
|
||||
throw new Error(
|
||||
`Invalid protocol era: ${value}. Valid eras are: legacy, auto, modern`,
|
||||
);
|
||||
}
|
||||
return value;
|
||||
}
|
||||
|
||||
/** On-disk contents of a freshly seeded empty catalog (pretty-printed). */
|
||||
const EMPTY_CATALOG_CONTENT = `${JSON.stringify({ mcpServers: {} }, null, 2)}\n`;
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
export {
|
||||
parseKeyValuePair,
|
||||
parseHeaderPair,
|
||||
parseProtocolEra,
|
||||
withDefaultCatalogPath,
|
||||
resolveServerConfigs,
|
||||
getNamedServerConfigs,
|
||||
|
|
|
|||
|
|
@ -1,6 +1,10 @@
|
|||
import type { SecretStore } from "../../auth/node/secret-store.js";
|
||||
import { defaultSecretStore } from "../../auth/node/secret-store-selection.js";
|
||||
import type { InspectorServerSettings, MCPServerConfig } from "../types.js";
|
||||
import type {
|
||||
InspectorServerSettings,
|
||||
MCPServerConfig,
|
||||
ServerProtocolEra,
|
||||
} from "../types.js";
|
||||
import {
|
||||
DEFAULT_CONNECTION_TIMEOUT_MS,
|
||||
DEFAULT_MAX_FETCH_REQUESTS,
|
||||
|
|
@ -30,10 +34,14 @@ export type ResolvedServer = {
|
|||
settings?: InspectorServerSettings;
|
||||
};
|
||||
|
||||
/** Loader options: the shared source flags plus a single `--header` set that is
|
||||
* broadcast into every resolved server's settings. */
|
||||
/** Loader options: the shared source flags plus a single `--header` set and an
|
||||
* optional `--protocol-era`, both broadcast into every resolved server's
|
||||
* settings. */
|
||||
export type ServerLoadOptions = ServerConfigOptions & {
|
||||
headers?: Record<string, string>;
|
||||
/** `--protocol-era`: overrides the file's `protocolEra` (or the legacy
|
||||
* default) the way `--header` overrides its headers (#2208). */
|
||||
protocolEra?: ServerProtocolEra;
|
||||
/** Test injection; defaults to the selected store (see
|
||||
* `secret-store-selection.ts`) for catalog/config loads. */
|
||||
secretStore?: SecretStore;
|
||||
|
|
@ -51,7 +59,15 @@ export function headersToServerSettings(
|
|||
return undefined;
|
||||
}
|
||||
return {
|
||||
...defaultServerSettings(),
|
||||
headers: Object.entries(headers).map(([key, value]) => ({ key, value })),
|
||||
};
|
||||
}
|
||||
|
||||
/** A settings node with every field at its product default. */
|
||||
function defaultServerSettings(): InspectorServerSettings {
|
||||
return {
|
||||
headers: [],
|
||||
env: [],
|
||||
metadata: {},
|
||||
connectionTimeout: DEFAULT_CONNECTION_TIMEOUT_MS,
|
||||
|
|
@ -65,18 +81,29 @@ export function headersToServerSettings(
|
|||
}
|
||||
|
||||
/**
|
||||
* Overlay CLI `--header` values onto the settings lifted from the file. Only the
|
||||
* `headers` field is overridden — timeouts, OAuth, and the rest of the file's
|
||||
* settings are preserved.
|
||||
* Overlay `--header` / `--protocol-era` onto the settings lifted from the file
|
||||
* (or onto nothing, for an ad-hoc target). Only those two fields are overridden
|
||||
* — timeouts, OAuth, and the rest of the file's settings are preserved. Returns
|
||||
* `base` untouched when neither flag was given.
|
||||
*/
|
||||
function mergeSettings(
|
||||
base: InspectorServerSettings | undefined,
|
||||
headers?: Record<string, string>,
|
||||
overrides: Pick<ServerLoadOptions, "headers" | "protocolEra">,
|
||||
): InspectorServerSettings | undefined {
|
||||
const fromHeaders = headersToServerSettings(headers);
|
||||
if (!fromHeaders) return base;
|
||||
if (!base) return fromHeaders;
|
||||
return { ...base, headers: fromHeaders.headers };
|
||||
let settings = base;
|
||||
const fromHeaders = headersToServerSettings(overrides.headers);
|
||||
if (fromHeaders) {
|
||||
settings = settings
|
||||
? { ...settings, headers: fromHeaders.headers }
|
||||
: fromHeaders;
|
||||
}
|
||||
if (overrides.protocolEra) {
|
||||
settings = {
|
||||
...(settings ?? defaultServerSettings()),
|
||||
protocolEra: overrides.protocolEra,
|
||||
};
|
||||
}
|
||||
return settings;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -117,10 +144,11 @@ export async function loadServerEntries(
|
|||
env: serverOptions.env,
|
||||
cwd: serverOptions.cwd,
|
||||
}),
|
||||
// Deliberate broadcast: a single `--header` set is merged into EVERY
|
||||
// server in the catalog/config (fine for the common single-server case;
|
||||
// for multi-server files, prefer per-server headers in the file itself).
|
||||
settings: mergeSettings(entry.settings, serverOptions.headers),
|
||||
// Deliberate broadcast: a single `--header` set (and `--protocol-era`)
|
||||
// is merged into EVERY server in the catalog/config (fine for the
|
||||
// common single-server case; for multi-server files, prefer per-server
|
||||
// settings in the file itself).
|
||||
settings: mergeSettings(entry.settings, serverOptions),
|
||||
};
|
||||
}
|
||||
return result;
|
||||
|
|
@ -135,7 +163,7 @@ export async function loadServerEntries(
|
|||
return {
|
||||
default: {
|
||||
config: configs[0]!,
|
||||
settings: headersToServerSettings(serverOptions.headers),
|
||||
settings: mergeSettings(undefined, serverOptions),
|
||||
},
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -49,11 +49,18 @@ export function createTransportNode(
|
|||
pipeStderr = false,
|
||||
onFetchRequest,
|
||||
onFetchResponseBody,
|
||||
onFetchStreamUpdate,
|
||||
authProvider,
|
||||
settings,
|
||||
interceptAuthChallenges = false,
|
||||
onAuthChallengeObserved,
|
||||
} = options;
|
||||
// Any of the three tracking callbacks earns a tracker: each is optional on
|
||||
// the tracker itself, and a caller that wants only stream updates (or only
|
||||
// bodies) must not be silently handed the bare fetch (#2318).
|
||||
const wantsFetchTracking = Boolean(
|
||||
onFetchRequest || onFetchResponseBody || onFetchStreamUpdate,
|
||||
);
|
||||
|
||||
// `optionsFetchFn` is the caller's whole fetch stack and already sits on top of
|
||||
// a proxy-aware base when one is needed — the Node clients install
|
||||
|
|
@ -68,10 +75,11 @@ export function createTransportNode(
|
|||
? createAuthChallengeObserverFetch(inner, onAuthChallengeObserved)
|
||||
: inner;
|
||||
const withTracking = (inner: typeof fetch): typeof fetch =>
|
||||
onFetchRequest
|
||||
wantsFetchTracking
|
||||
? createFetchTracker(inner, {
|
||||
trackRequest: onFetchRequest,
|
||||
updateResponseBody: onFetchResponseBody,
|
||||
updateStream: onFetchStreamUpdate,
|
||||
})
|
||||
: inner;
|
||||
// The observer sits *under* the interceptor so it still reports the
|
||||
|
|
|
|||
|
|
@ -58,6 +58,7 @@ export function createRemoteTransport(
|
|||
onStderr: transportOptions.onStderr,
|
||||
onFetchRequest: transportOptions.onFetchRequest,
|
||||
onFetchResponseBody: transportOptions.onFetchResponseBody,
|
||||
onFetchStreamUpdate: transportOptions.onFetchStreamUpdate,
|
||||
authProvider: transportOptions.authProvider,
|
||||
settings: transportOptions.settings,
|
||||
},
|
||||
|
|
|
|||
|
|
@ -4,7 +4,7 @@
|
|||
|
||||
import type { Transport } from "@modelcontextprotocol/client";
|
||||
import type { JSONRPCMessage } from "@modelcontextprotocol/client";
|
||||
import type { FetchRequestEntryBase } from "../../types.js";
|
||||
import type { FetchRequestEntryBase, FetchStreamState } from "../../types.js";
|
||||
import type { RemoteEvent } from "../types.js";
|
||||
import type { AuthChallenge } from "../../../auth/challenge.js";
|
||||
import { AuthChallengeError } from "../../../auth/challenge.js";
|
||||
|
|
@ -328,6 +328,17 @@ export class RemoteSession {
|
|||
});
|
||||
}
|
||||
|
||||
onFetchStreamUpdate(id: string, stream: FetchStreamState): void {
|
||||
this.pushEvent({
|
||||
type: "fetch_stream_update",
|
||||
data: {
|
||||
id,
|
||||
eventCount: stream.eventCount,
|
||||
...(stream.closedAt && { closedAt: stream.closedAt.toISOString() }),
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
onStderr(entry: { timestamp: Date; message: string }): void {
|
||||
this.pushEvent({
|
||||
type: "stdio_log",
|
||||
|
|
|
|||
|
|
@ -65,6 +65,7 @@ import {
|
|||
stripInspectorFields,
|
||||
} from "../../serverList.js";
|
||||
import { toRecord } from "../../../json/jsonUtils.js";
|
||||
import { isSkillCatalogLimit } from "../../skills.js";
|
||||
import { resolveImportSource } from "../../import/resolveSource.js";
|
||||
import { RemoteSession } from "./remote-session.js";
|
||||
import { createRemoteAuthProvider } from "./tokenAuthProvider.js";
|
||||
|
|
@ -868,6 +869,8 @@ export function createRemoteApp(
|
|||
onFetchRequest: (entry) => session.onFetchRequest(entry),
|
||||
onFetchResponseBody: (id, body) =>
|
||||
session.onFetchResponseBody(id, body),
|
||||
onFetchStreamUpdate: (id, stream) =>
|
||||
session.onFetchStreamUpdate(id, stream),
|
||||
authProvider: authHandle?.provider,
|
||||
settings: body.settings,
|
||||
// Always intercept 401/403 on the node MCP transport. Without this, the
|
||||
|
|
@ -1681,6 +1684,19 @@ export function createRemoteApp(
|
|||
);
|
||||
delete valObj.maxFetchRequests;
|
||||
}
|
||||
// #2294 — positive integers only; `0` is not "unlimited" for these.
|
||||
for (const key of [
|
||||
"skillCatalogMaxSkills",
|
||||
"skillCatalogMaxBytes",
|
||||
] as const) {
|
||||
if (key in valObj && !isSkillCatalogLimit(valObj[key])) {
|
||||
logWarn(
|
||||
{ route: "/api/servers", id, droppedKey: key },
|
||||
`Dropping malformed \`${key}\` field — expected a positive integer.`,
|
||||
);
|
||||
delete valObj[key];
|
||||
}
|
||||
}
|
||||
if ("oauth" in valObj && !isOauthObject(valObj.oauth)) {
|
||||
logWarn(
|
||||
{ route: "/api/servers", id, droppedKey: "oauth" },
|
||||
|
|
@ -1953,6 +1969,18 @@ export function createRemoteApp(
|
|||
error: "settings.maxFetchRequests must be a non-negative number",
|
||||
};
|
||||
}
|
||||
// #2294 — optional on the wire; absent means the default budget.
|
||||
for (const key of [
|
||||
"skillCatalogMaxSkills",
|
||||
"skillCatalogMaxBytes",
|
||||
] as const) {
|
||||
if (obj[key] !== undefined && !isSkillCatalogLimit(obj[key])) {
|
||||
return {
|
||||
ok: false,
|
||||
error: `settings.${key} must be a positive integer`,
|
||||
};
|
||||
}
|
||||
}
|
||||
for (const optional of [
|
||||
"oauthClientId",
|
||||
"oauthClientSecret",
|
||||
|
|
@ -2099,6 +2127,13 @@ export function createRemoteApp(
|
|||
// writes no spurious `roots` field.
|
||||
roots: isRootArray(obj.roots) ? obj.roots : [],
|
||||
};
|
||||
// Validated above; absent stays absent (the default budget).
|
||||
if (isSkillCatalogLimit(obj.skillCatalogMaxSkills)) {
|
||||
value.skillCatalogMaxSkills = obj.skillCatalogMaxSkills;
|
||||
}
|
||||
if (isSkillCatalogLimit(obj.skillCatalogMaxBytes)) {
|
||||
value.skillCatalogMaxBytes = obj.skillCatalogMaxBytes;
|
||||
}
|
||||
if (typeof obj.oauthClientId === "string" && obj.oauthClientId !== "") {
|
||||
value.oauthClientId = obj.oauthClientId;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -11,8 +11,10 @@ import type {
|
|||
JSONRPCMessage,
|
||||
MessageExtraInfo,
|
||||
} from "@modelcontextprotocol/client";
|
||||
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
|
||||
import { findHeader, isLongLivedStreamResponse } from "../fetchTracking.js";
|
||||
import type { InspectorServerSettings, StderrLogEntry } from "../types.js";
|
||||
import type { FetchRequestEntryBase } from "../types.js";
|
||||
import type { FetchRequestEntryBase, FetchStreamState } from "../types.js";
|
||||
import type {
|
||||
AuthChallenge,
|
||||
AuthChallengeOutcome,
|
||||
|
|
@ -61,6 +63,9 @@ export interface RemoteTransportOptions {
|
|||
/** Callback for async response-body updates to a previously tracked fetch. */
|
||||
onFetchResponseBody?: (id: string, responseBody: string) => void;
|
||||
|
||||
/** Callback for a previously tracked long-lived stream's events and close (#2318). */
|
||||
onFetchStreamUpdate?: (id: string, stream: FetchStreamState) => void;
|
||||
|
||||
/** Optional OAuth client provider for Bearer authentication */
|
||||
authProvider?: import("@modelcontextprotocol/client").OAuthClientProvider;
|
||||
|
||||
|
|
@ -261,6 +266,14 @@ export class RemoteClientTransport implements Transport {
|
|||
private eventStreamConsumeTask: Promise<void> | null = null;
|
||||
private restartingEventStream = false;
|
||||
private closed = false;
|
||||
/**
|
||||
* Long-lived streams the backend has reported open, by fetch entry id, with
|
||||
* the last event count seen for each. `close()` reports them closed
|
||||
* itself: it aborts the event channel before asking the backend to
|
||||
* disconnect, so the backend watcher's own close report lands on an
|
||||
* unconsumed queue and is lost with the session (#2318).
|
||||
*/
|
||||
private openStreams = new Map<string, number>();
|
||||
private readonly sseResponseWaits = new Map<
|
||||
string | number,
|
||||
SseResponseWait
|
||||
|
|
@ -539,12 +552,23 @@ export class RemoteClientTransport implements Transport {
|
|||
this.resetSseResponseWait(progressToken);
|
||||
}
|
||||
this.onmessage?.(msg, undefined);
|
||||
} else if (
|
||||
parsed.type === "fetch_request" &&
|
||||
this.options.onFetchRequest
|
||||
) {
|
||||
} else if (parsed.type === "fetch_request") {
|
||||
const entry = parsed.data;
|
||||
this.options.onFetchRequest({
|
||||
// Stream bookkeeping runs whether or not the consumer logs
|
||||
// requests: `onFetchStreamUpdate` is documented to work on its
|
||||
// own, and `close()` needs to know which streams are open. It
|
||||
// exists only to serve that callback, though — without one the
|
||||
// set would only ever grow, one entry per reconnect.
|
||||
if (
|
||||
this.options.onFetchStreamUpdate &&
|
||||
isLongLivedStreamResponse(
|
||||
entry.method,
|
||||
findHeader(entry.responseHeaders, "content-type"),
|
||||
)
|
||||
) {
|
||||
this.openStreams.set(entry.id, 0);
|
||||
}
|
||||
this.options.onFetchRequest?.({
|
||||
...entry,
|
||||
timestamp:
|
||||
typeof entry.timestamp === "string"
|
||||
|
|
@ -559,6 +583,19 @@ export class RemoteClientTransport implements Transport {
|
|||
parsed.data.id,
|
||||
parsed.data.responseBody,
|
||||
);
|
||||
} else if (
|
||||
parsed.type === "fetch_stream_update" &&
|
||||
this.options.onFetchStreamUpdate
|
||||
) {
|
||||
const { id, eventCount, closedAt } = parsed.data;
|
||||
if (closedAt !== undefined) this.openStreams.delete(id);
|
||||
else if (this.openStreams.has(id)) {
|
||||
this.openStreams.set(id, eventCount);
|
||||
}
|
||||
this.options.onFetchStreamUpdate(id, {
|
||||
eventCount,
|
||||
...(closedAt !== undefined && { closedAt: new Date(closedAt) }),
|
||||
});
|
||||
} else if (parsed.type === "stdio_log" && this.options.onStderr) {
|
||||
this.options.onStderr({
|
||||
timestamp: new Date(parsed.data.timestamp),
|
||||
|
|
@ -693,10 +730,17 @@ export class RemoteClientTransport implements Transport {
|
|||
const arm = () => {
|
||||
timer = setTimeout(() => {
|
||||
this.sseResponseWaits.delete(requestId);
|
||||
// The SDK's own per-request timeout shape, not a plain `Error`:
|
||||
// this wait can expire before the SDK's timer does (its budget is
|
||||
// the relay's, not the request's), and the rejection travels up
|
||||
// through `Client.request`, where `InspectorClient` annotates a
|
||||
// timeout of exactly this shape with the connection's state
|
||||
// (#2318). A plain error would pass that decorator untouched and
|
||||
// reach the user as a bare timeout on the web client alone.
|
||||
reject(
|
||||
new Error(
|
||||
`Timed out waiting for MCP response on SSE (${this.sseResponseTimeoutMs}ms)`,
|
||||
),
|
||||
new SdkError(SdkErrorCode.RequestTimeout, "Request timed out", {
|
||||
timeout: this.sseResponseTimeoutMs,
|
||||
}),
|
||||
);
|
||||
}, this.sseResponseTimeoutMs);
|
||||
};
|
||||
|
|
@ -836,6 +880,13 @@ export class RemoteClientTransport implements Transport {
|
|||
this.closed = true;
|
||||
this.cancelAllSseWaits(new Error("Transport closed"));
|
||||
await this.stopEventStream();
|
||||
// The disconnect below ends every stream the backend still holds open,
|
||||
// and its close reports cannot reach us any more (see `openStreams`).
|
||||
const closedAt = new Date();
|
||||
for (const [id, eventCount] of this.openStreams) {
|
||||
this.options.onFetchStreamUpdate?.(id, { eventCount, closedAt });
|
||||
}
|
||||
this.openStreams.clear();
|
||||
|
||||
if (this._sessionId) {
|
||||
try {
|
||||
|
|
|
|||
|
|
@ -119,7 +119,10 @@ export class RemoteInspectorClientStorage implements InspectorClientStorage {
|
|||
|
||||
const data = (await res.json()) as InspectorClientSessionState;
|
||||
|
||||
// Deserialize state (convert ISO strings back to Date objects)
|
||||
// Deserialize state (convert ISO strings back to Date objects). The
|
||||
// nested `stream.closedAt` is a Date too, and `JSON.stringify` turned it
|
||||
// into an ISO string on the way out; left as a string it would fail the
|
||||
// `getTime()` every consumer of the type performs (#2318).
|
||||
return {
|
||||
...data,
|
||||
fetchRequests: data.fetchRequests.map((req) => ({
|
||||
|
|
@ -130,6 +133,9 @@ export class RemoteInspectorClientStorage implements InspectorClientStorage {
|
|||
: req.timestamp instanceof Date
|
||||
? req.timestamp
|
||||
: new Date(req.timestamp),
|
||||
...(req.stream?.closedAt !== undefined && {
|
||||
stream: { ...req.stream, closedAt: new Date(req.stream.closedAt) },
|
||||
}),
|
||||
})),
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -137,6 +137,7 @@ export type RemoteEventType =
|
|||
| "message"
|
||||
| "fetch_request"
|
||||
| "fetch_request_body_update"
|
||||
| "fetch_stream_update"
|
||||
| "stdio_log"
|
||||
| "transport_error"
|
||||
| "auth_challenge";
|
||||
|
|
@ -156,6 +157,16 @@ export interface RemoteEventFetchRequestBodyUpdate {
|
|||
data: { id: string; responseBody: string };
|
||||
}
|
||||
|
||||
/**
|
||||
* A long-lived stream's lifecycle, forwarded from the backend's fetch tracker
|
||||
* (#2318). `closedAt` is an ISO string on the wire; the browser transport
|
||||
* rebuilds the `Date` the way it does for `fetch_request`'s timestamp.
|
||||
*/
|
||||
export interface RemoteEventFetchStreamUpdate {
|
||||
type: "fetch_stream_update";
|
||||
data: { id: string; eventCount: number; closedAt?: string };
|
||||
}
|
||||
|
||||
export interface RemoteEventStdioLog {
|
||||
type: "stdio_log";
|
||||
data: { timestamp: string; message: string };
|
||||
|
|
@ -178,6 +189,7 @@ export type RemoteEvent =
|
|||
| RemoteEventMessage
|
||||
| RemoteEventFetchRequest
|
||||
| RemoteEventFetchRequestBodyUpdate
|
||||
| RemoteEventFetchStreamUpdate
|
||||
| RemoteEventStdioLog
|
||||
| RemoteEventTransportError
|
||||
| RemoteEventAuthChallenge;
|
||||
|
|
|
|||
|
|
@ -13,6 +13,11 @@ import {
|
|||
DEFAULT_TASK_TTL_MS,
|
||||
isModernLogLevel,
|
||||
} from "./types.js";
|
||||
import {
|
||||
isSkillCatalogLimit,
|
||||
SKILL_MAX_CATALOG_BYTES,
|
||||
SKILL_MAX_CATALOG_SKILLS,
|
||||
} from "./skills.js";
|
||||
import type { Root } from "@modelcontextprotocol/client";
|
||||
import type {
|
||||
InspectorServerSettings,
|
||||
|
|
@ -160,6 +165,8 @@ type StoredInspectorFields = Pick<
|
|||
| "paginatedLists"
|
||||
| "advertisedExtensions"
|
||||
| "maxFetchRequests"
|
||||
| "skillCatalogMaxSkills"
|
||||
| "skillCatalogMaxBytes"
|
||||
| "oauth"
|
||||
| "roots"
|
||||
>;
|
||||
|
|
@ -521,6 +528,8 @@ export function storedFieldsToInspectorSettings(
|
|||
stored.paginatedLists !== undefined ||
|
||||
stored.advertisedExtensions !== undefined ||
|
||||
stored.maxFetchRequests !== undefined ||
|
||||
stored.skillCatalogMaxSkills !== undefined ||
|
||||
stored.skillCatalogMaxBytes !== undefined ||
|
||||
stored.oauth !== undefined ||
|
||||
stored.roots !== undefined ||
|
||||
stored.protocolEra !== undefined ||
|
||||
|
|
@ -557,6 +566,16 @@ export function storedFieldsToInspectorSettings(
|
|||
// `[]`, which `inspectorSettingsToStoredFields` then omits on write.
|
||||
roots: stored.roots ?? [],
|
||||
};
|
||||
// Lifted only when usable (#2294). Unlike maxFetchRequests these stay
|
||||
// optional in memory — absent means the default, which
|
||||
// `resolveSkillCatalogBudget` supplies — and a hand-edited `0` or `-1` is
|
||||
// dropped here rather than carried into a form that would re-persist it.
|
||||
if (isSkillCatalogLimit(stored.skillCatalogMaxSkills)) {
|
||||
settings.skillCatalogMaxSkills = stored.skillCatalogMaxSkills;
|
||||
}
|
||||
if (isSkillCatalogLimit(stored.skillCatalogMaxBytes)) {
|
||||
settings.skillCatalogMaxBytes = stored.skillCatalogMaxBytes;
|
||||
}
|
||||
// Absent on disk reads back as the default era; the write side then omits the
|
||||
// default so a byte-stable round-trip never injects `protocolEra` into files
|
||||
// that never set it. An unknown literal from a hand-edited file is dropped
|
||||
|
|
@ -737,6 +756,22 @@ export function inspectorSettingsToStoredFields(
|
|||
out.maxFetchRequests = settings.maxFetchRequests;
|
||||
}
|
||||
|
||||
// Same omit-the-default rule (#2294). An unusable value writes nothing, so
|
||||
// it reads back as the default rather than being persisted and then
|
||||
// rejected on the next load.
|
||||
if (
|
||||
isSkillCatalogLimit(settings.skillCatalogMaxSkills) &&
|
||||
settings.skillCatalogMaxSkills !== SKILL_MAX_CATALOG_SKILLS
|
||||
) {
|
||||
out.skillCatalogMaxSkills = settings.skillCatalogMaxSkills;
|
||||
}
|
||||
if (
|
||||
isSkillCatalogLimit(settings.skillCatalogMaxBytes) &&
|
||||
settings.skillCatalogMaxBytes !== SKILL_MAX_CATALOG_BYTES
|
||||
) {
|
||||
out.skillCatalogMaxBytes = settings.skillCatalogMaxBytes;
|
||||
}
|
||||
|
||||
const oauthFields: NonNullable<StoredMCPServer["oauth"]> = {};
|
||||
if (settings.oauthClientId) oauthFields.clientId = settings.oauthClientId;
|
||||
if (settings.oauthClientSecret)
|
||||
|
|
@ -815,6 +850,8 @@ const INSPECTOR_FIELD_KEY_MAP = {
|
|||
paginatedLists: true,
|
||||
advertisedExtensions: true,
|
||||
maxFetchRequests: true,
|
||||
skillCatalogMaxSkills: true,
|
||||
skillCatalogMaxBytes: true,
|
||||
oauth: true,
|
||||
roots: true,
|
||||
} as const satisfies Record<keyof StoredInspectorFields, true>;
|
||||
|
|
|
|||
|
|
@ -76,6 +76,47 @@ export const SKILL_MAX_CATALOG_SKILLS = 256;
|
|||
/** @see {@link SKILL_MAX_CATALOG_SKILLS} — 64 MiB across the whole run. */
|
||||
export const SKILL_MAX_CATALOG_BYTES = 64 * 1024 * 1024;
|
||||
|
||||
/**
|
||||
* Whether `value` is usable as a per-server catalog budget override (#2294):
|
||||
* a positive safe integer.
|
||||
*
|
||||
* ⚠️ **`0` is deliberately not "unlimited"**, unlike `maxFetchRequests`. The
|
||||
* budget exists so a `--verify` against a huge catalog terminates; an
|
||||
* unlimited setting would reintroduce exactly the unbounded run it closed. A
|
||||
* user who wants more raises the number.
|
||||
*/
|
||||
export function isSkillCatalogLimit(value: unknown): value is number {
|
||||
return typeof value === "number" && Number.isSafeInteger(value) && value > 0;
|
||||
}
|
||||
|
||||
/** A verification run's catalog budget, resolved from per-server settings. */
|
||||
export interface SkillCatalogBudget {
|
||||
maxSkills: number;
|
||||
maxBytes: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* The catalog budget for a run: the server's configured limits where they are
|
||||
* usable, else {@link SKILL_MAX_CATALOG_SKILLS} / {@link SKILL_MAX_CATALOG_BYTES}.
|
||||
*
|
||||
* A malformed value falls back to the default rather than throwing — settings
|
||||
* can arrive from a hand-edited `mcp.json`, and a verification that refused to
|
||||
* run over a typo would be a worse outcome than one that ran at the default.
|
||||
*/
|
||||
export function resolveSkillCatalogBudget(settings?: {
|
||||
skillCatalogMaxSkills?: number;
|
||||
skillCatalogMaxBytes?: number;
|
||||
}): SkillCatalogBudget {
|
||||
return {
|
||||
maxSkills: isSkillCatalogLimit(settings?.skillCatalogMaxSkills)
|
||||
? settings.skillCatalogMaxSkills
|
||||
: SKILL_MAX_CATALOG_SKILLS,
|
||||
maxBytes: isSkillCatalogLimit(settings?.skillCatalogMaxBytes)
|
||||
? settings.skillCatalogMaxBytes
|
||||
: SKILL_MAX_CATALOG_BYTES,
|
||||
};
|
||||
}
|
||||
|
||||
/** The suffix every skill URI ends with; the segment before it is the name. */
|
||||
export const SKILL_FILE_SUFFIX = "/SKILL.md";
|
||||
|
||||
|
|
@ -171,6 +212,17 @@ export function isSkillsExtensionSupported(
|
|||
* rejected too: containment could not be decided on it, and silently accepting
|
||||
* one would reintroduce exactly the hole this function closes.
|
||||
*
|
||||
* A **path-less authority** URI (`skill://data-analysis`) is accepted, and is
|
||||
* not the opaque case above: RFC 3986 gives a URI with an authority an empty
|
||||
* path, and the parser reports that as `pathname === ""`. The authority has to
|
||||
* be non-empty — `skill:` parses to an empty path too, with no host. It has no segments,
|
||||
* so nothing survives unnormalized. It is what a skill's root directory looks
|
||||
* like once `/SKILL.md` is removed, so rejecting it made a server echoing that
|
||||
* root back in a directory read read as "outside this skill" when it is the
|
||||
* skill itself (#2295). It names no file, so it still fails every check that
|
||||
* needs one — `skillNameFromUri` wants the `/SKILL.md` suffix, and a manifest
|
||||
* entry spelled this way does not start with `<root>/`.
|
||||
*
|
||||
* The scheme is deliberately **not** constrained. `skill://` is what SEP-2640
|
||||
* recommends and what this repo's fixture serves, but the SEP only says servers
|
||||
* SHOULD use it and explicitly allows a domain-native scheme (`github://…`), so
|
||||
|
|
@ -186,7 +238,10 @@ export function normalizeSkillUri(uri: string): string | undefined {
|
|||
} catch {
|
||||
return undefined;
|
||||
}
|
||||
if (!parsed.pathname.startsWith("/")) return undefined;
|
||||
// An empty path is only the root form when an authority carries the identity:
|
||||
// `skill:` and `mailto:` also parse to `pathname === ""`, with no host.
|
||||
const pathlessRoot = parsed.pathname === "" && parsed.host !== "";
|
||||
if (!pathlessRoot && !parsed.pathname.startsWith("/")) return undefined;
|
||||
return canonicalizePercentEncoding(parsed.href);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -32,9 +32,8 @@ import {
|
|||
checkSkillConformance,
|
||||
checkSkillFrontmatterMatch,
|
||||
checkSkillNameCollisions,
|
||||
resolveSkillCatalogBudget,
|
||||
skillDisplayName,
|
||||
SKILL_MAX_CATALOG_BYTES,
|
||||
SKILL_MAX_CATALOG_SKILLS,
|
||||
SKILL_MAX_RESOURCE_ENTRIES,
|
||||
SKILL_MAX_TOTAL_BYTES,
|
||||
skillFileBytes,
|
||||
|
|
@ -323,15 +322,27 @@ export async function verifySkills(
|
|||
// thousand skills, each individually conforming, made `--verify` run
|
||||
// indefinitely and transfer unboundedly (Copilot). See
|
||||
// {@link SKILL_MAX_CATALOG_SKILLS}.
|
||||
//
|
||||
// The limits are the server's configured ones when set (#2294). Read through
|
||||
// `getServerSettings()` rather than taken as a parameter, so every caller —
|
||||
// the CLI's `--verify` and the TUI pane — honors the setting without having
|
||||
// to remember to pass it. Optional-chained because a test double need not
|
||||
// implement the accessor.
|
||||
const budget = resolveSkillCatalogBudget(client.getServerSettings?.());
|
||||
let walkedSkills = 0;
|
||||
let catalogBytes = 0;
|
||||
for (const entry of entries) {
|
||||
// Static checks still run for every entry — they cost no I/O, so a skill
|
||||
// past the budget is still reported on, just not read. What stops is the
|
||||
// reading.
|
||||
//
|
||||
// ⚠️ STRICT on bytes, like the count. A run whose charged bytes exactly
|
||||
// equal the limit has REACHED it, and `<=` read one more skill past a
|
||||
// budget that was already spent (Copilot, #2294). The skill whose reads
|
||||
// cross the limit is still reported in full — that is decided by charging
|
||||
// after the entry, below, not by this comparison.
|
||||
const withinBudget =
|
||||
walkedSkills < SKILL_MAX_CATALOG_SKILLS &&
|
||||
catalogBytes <= SKILL_MAX_CATALOG_BYTES;
|
||||
walkedSkills < budget.maxSkills && catalogBytes < budget.maxBytes;
|
||||
// The entry's own SKILL.md, read once and used twice — for its digest and
|
||||
// for the frontmatter cross-check. Reading it twice would double the load
|
||||
// on the server and, worse, could compare a digest against one snapshot
|
||||
|
|
@ -368,7 +379,7 @@ export async function verifySkills(
|
|||
// total, by definition) is never truncated.
|
||||
const manifest = withinBudget ? boundedManifest(declared) : [];
|
||||
let incomplete = !withinBudget
|
||||
? `Not read: this run already reached its catalog budget of ${SKILL_MAX_CATALOG_SKILLS} skills / ${SKILL_MAX_CATALOG_BYTES} bytes. Nothing about this skill's files has been checked — verify it on its own with \`--method skills/get --uri\` to get a verdict.`
|
||||
? `Not read: this run already reached its catalog budget of ${budget.maxSkills} skills / ${budget.maxBytes} bytes (raise it in the server's Skills settings). Nothing about this skill's files has been checked — verify it on its own with \`--method skills/get --uri\` to get a verdict.`
|
||||
: manifest.length < declared.length
|
||||
? `Only ${manifest.length} of ${declared.length} manifest entries were read: the skill exceeds the ${SKILL_MAX_RESOURCE_ENTRIES}-entry / ${SKILL_MAX_TOTAL_BYTES}-byte interoperability limits, so the rest were not fetched and cannot be reported on.`
|
||||
: undefined;
|
||||
|
|
@ -499,6 +510,12 @@ export async function verifySkills(
|
|||
files.push({ uri: entry.uri, status: "read-error", reason });
|
||||
try {
|
||||
const invocation = await client.readResource(entry.uri, metadata);
|
||||
// ⚠️ Charged like a manifest read — raw response, or the decoded
|
||||
// length when larger. Uncharged, a catalog of `"dynamic"` skills (whose
|
||||
// ONLY read is this one) left `catalogBytes` at zero, so the run's byte
|
||||
// budget bounded nothing for it and even a configured limit of 1 read
|
||||
// every skill up to the count limit (Copilot, #2294).
|
||||
let charged = responseBytes(invocation.result);
|
||||
const contents = contentsFor(invocation.result, entry.uri);
|
||||
if (!contents) {
|
||||
fail(
|
||||
|
|
@ -507,10 +524,12 @@ export async function verifySkills(
|
|||
} else {
|
||||
try {
|
||||
entryBytes = skillFileBytes(contents);
|
||||
charged = Math.max(charged, entryBytes.byteLength);
|
||||
} catch (err) {
|
||||
fail(reasonOf(err));
|
||||
}
|
||||
}
|
||||
receivedBytes += charged;
|
||||
// If the DECLARED manifest lists this file but the read bounds
|
||||
// excluded it, verify it here too. The fallback exists for the
|
||||
// frontmatter check, but reading a file and then not checking the
|
||||
|
|
|
|||
Some files were not shown because too many files have changed in this diff Show more
Loading…
Reference in a new issue