From 2f80958d3ffa829982abf749b03ef4b838ef9cb1 Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Wed, 22 Jul 2026 15:25:45 -0400 Subject: [PATCH] Extensions: first-class advertised-extensions map (Phase 1 of #1633) (#1742) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Extensions: first-class advertised-extensions map (Phase 1 of #1633) Collapse the two ad-hoc `capabilities.extensions` spreads in the InspectorClient constructor (EMA, gated on enterpriseManaged; the modern Tasks extension, always-on) into a single builder that is the source of truth for what the client advertises. - core/mcp/extensions.ts: `ADVERTISABLE_EXTENSIONS` registry (id + label + default-on flag), the `EMA_EXTENSION_KEY` constant, and `buildClientExtensions()`. The registry is the shared list the Server Settings toggle UI (Phase 2, #1739) will drive; EMA stays a conditional built-in (auth-mode driven, not a free toggle). - InspectorClientOptions.advertisedExtensions: per-extension overrides (present key wins over the registry default) so a future UI can toggle advertised extensions as a debugging knob. - Behavior preserved: tasks defaults to advertised, so the map is non-empty and rides every modern request envelope; EMA advertised iff enterpriseManaged. The raw modern-task envelope re-stamp is left untouched (it fires only on the raw tasks channel, which requires the extension anyway). Tests: extensions.test.ts covers the registry and every builder branch (defaults, EMA on/off, override enable/disable, unknown key) at 100%. The tasks-era and EMA integration suites still assert the same wire advertisement. `advertisedExtensions` is plumbed but not yet wired to the form (that lands in Phase 2). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: address Phase 1 review (EMA-override test + envelope comment) - extensions.test.ts: add an EMA-specific assertion that an override for EMA_EXTENSION_KEY cannot advertise it (auth-mode only) — documents intent and guards against EMA being added to ADVERTISABLE_EXTENSIONS by mistake. - inspectorClient.ts: rewrite the withModernTaskEnvelope tasks-stamp comment so it stays accurate once Phase 2 lets a user disable general tasks advertisement — the force-stamp is load-bearing (the raw tasks/* channel requires the extension), not a redundant re-add. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * TUI: de-flake step-up OAuth frame assertions (raise poll ceiling) The step-up authorize tests intermittently timed out on the final "Step-up authorization succeeded" frame. Root cause is a test-infra timing budget, not product code: waitForFrame/waitUntil polled only 25× (25ms tick ≈ 625ms), and the step-up flow adds an extra async OAuth-runner hop plus React commits that can exceed that under CI load with v8 coverage. Hoist the poll count into a named POLL_TRIES = 100 (~2.5s ceiling). A poll exits the instant its predicate is true, so a higher ceiling never slows a passing assertion — it only widens the margin for the slow path. Verified 5× green (3 plain + 2 under --coverage), including the previously-flaky step-up cases. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * TUI: prettier-format the wrapped waitForFrame signature The POLL_TRIES change pushed waitForFrame's signature past the print width; Prettier wraps it. (Root has no aggregate format script — format is per-client, so this was missed locally.) Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) --- clients/tui/__tests__/App.test.tsx | 17 +++- .../web/src/test/core/mcp/extensions.test.ts | 97 +++++++++++++++++++ core/mcp/extensions.ts | 93 ++++++++++++++++++ core/mcp/inspectorClient.ts | 37 ++++--- core/mcp/types.ts | 10 ++ 5 files changed, 238 insertions(+), 16 deletions(-) create mode 100644 clients/web/src/test/core/mcp/extensions.test.ts create mode 100644 core/mcp/extensions.ts diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index dcf3be47..2c789315 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -516,8 +516,17 @@ async function press(r: RenderResult, keys: string[]) { * Poll until `predicate` is true (or the tries run out). React + ink schedule * renders across several macrotasks, so async state set by a flow can take more * than one fixed tick to land under coverage instrumentation. + * + * The default budget (POLL_TRIES × 25ms tick) is generous on purpose: a poll + * exits the instant the predicate is true, so a high ceiling never slows a + * passing assertion — it only widens the margin for the slow path. Flows with + * an extra async hop (e.g. the step-up OAuth runner before the success frame) + * plus React commits can exceed a tight budget under CI load with v8 coverage, + * which is what made the step-up frame assertions intermittently time out. */ -async function waitUntil(predicate: () => boolean, tries = 25) { +const POLL_TRIES = 100; + +async function waitUntil(predicate: () => boolean, tries = POLL_TRIES) { for (let i = 0; i < tries; i++) { if (predicate()) return; await tick(); @@ -529,7 +538,11 @@ async function waitUntil(predicate: () => boolean, tries = 25) { * settling races a single fixed tick under v8 coverage instrumentation, so * frame assertions that follow a mount/keypress use this instead of one tick. */ -async function waitForFrame(r: RenderResult, substr: string, tries = 25) { +async function waitForFrame( + r: RenderResult, + substr: string, + tries = POLL_TRIES, +) { await waitUntil(() => (r.lastFrame() ?? "").includes(substr), tries); } diff --git a/clients/web/src/test/core/mcp/extensions.test.ts b/clients/web/src/test/core/mcp/extensions.test.ts new file mode 100644 index 00000000..08716cf7 --- /dev/null +++ b/clients/web/src/test/core/mcp/extensions.test.ts @@ -0,0 +1,97 @@ +import { describe, it, expect } from "vitest"; +import { + ADVERTISABLE_EXTENSIONS, + EMA_EXTENSION_KEY, + buildClientExtensions, +} from "@inspector/core/mcp/extensions.js"; +import { TASKS_EXTENSION_KEY } from "@inspector/core/mcp/modernTaskSchemas.js"; + +describe("extensions (#1738)", () => { + describe("ADVERTISABLE_EXTENSIONS registry", () => { + it("lists the Tasks extension, advertised by default", () => { + const tasks = ADVERTISABLE_EXTENSIONS.find( + (e) => e.key === TASKS_EXTENSION_KEY, + ); + expect(tasks).toBeDefined(); + expect(tasks?.defaultAdvertised).toBe(true); + expect(tasks?.label).toContain("Tasks"); + }); + + it("does not list EMA (it follows the auth mode, not a toggle)", () => { + expect( + ADVERTISABLE_EXTENSIONS.some((e) => e.key === EMA_EXTENSION_KEY), + ).toBe(false); + }); + + it("has unique keys and non-empty labels", () => { + const keys = ADVERTISABLE_EXTENSIONS.map((e) => e.key); + expect(new Set(keys).size).toBe(keys.length); + for (const ext of ADVERTISABLE_EXTENSIONS) { + expect(ext.label.length).toBeGreaterThan(0); + } + }); + }); + + describe("buildClientExtensions", () => { + it("advertises registry defaults with no overrides (tasks on)", () => { + const map = buildClientExtensions({ enterpriseManaged: false }); + expect(map).toEqual({ [TASKS_EXTENSION_KEY]: {} }); + }); + + it("adds EMA when enterpriseManaged, alongside the registry defaults", () => { + const map = buildClientExtensions({ enterpriseManaged: true }); + expect(map).toEqual({ + [TASKS_EXTENSION_KEY]: {}, + [EMA_EXTENSION_KEY]: {}, + }); + }); + + it("omits EMA when not enterpriseManaged", () => { + const map = buildClientExtensions({ enterpriseManaged: false }); + expect(map).not.toHaveProperty(EMA_EXTENSION_KEY); + }); + + it("honors a user override that disables a default-on extension", () => { + const map = buildClientExtensions({ + enterpriseManaged: false, + advertised: { [TASKS_EXTENSION_KEY]: false }, + }); + expect(map).toEqual({}); + }); + + it("honors a user override that keeps a default-on extension enabled", () => { + const map = buildClientExtensions({ + enterpriseManaged: false, + advertised: { [TASKS_EXTENSION_KEY]: true }, + }); + expect(map).toEqual({ [TASKS_EXTENSION_KEY]: {} }); + }); + + it("does not let an override advertise EMA (auth-mode only)", () => { + // EMA is not a free toggle: it follows the auth mode, so an override for + // its key must not be able to advertise it. Locks in intent and guards + // against someone mistakenly adding EMA to ADVERTISABLE_EXTENSIONS. + const map = buildClientExtensions({ + enterpriseManaged: false, + advertised: { [EMA_EXTENSION_KEY]: true }, + }); + expect(map).not.toHaveProperty(EMA_EXTENSION_KEY); + }); + + it("ignores override keys that are not in the registry", () => { + const map = buildClientExtensions({ + enterpriseManaged: false, + advertised: { "io.example/unknown": true }, + }); + expect(map).toEqual({ [TASKS_EXTENSION_KEY]: {} }); + }); + + it("layers EMA on even when all registry entries are disabled", () => { + const map = buildClientExtensions({ + enterpriseManaged: true, + advertised: { [TASKS_EXTENSION_KEY]: false }, + }); + expect(map).toEqual({ [EMA_EXTENSION_KEY]: {} }); + }); + }); +}); diff --git a/core/mcp/extensions.ts b/core/mcp/extensions.ts new file mode 100644 index 00000000..6c67f257 --- /dev/null +++ b/core/mcp/extensions.ts @@ -0,0 +1,93 @@ +import type { ClientCapabilities } from "@modelcontextprotocol/client"; +import { TASKS_EXTENSION_KEY } from "./modernTaskSchemas.js"; + +/** + * Extension identifier for SEP-2350 enterprise-managed authorization. Advertised + * when the connection routes through the enterprise IdP (EMA), which is a + * property of the auth mode rather than a free debugging toggle — so it is a + * conditional built-in in {@link buildClientExtensions}, not a registry entry. + */ +export const EMA_EXTENSION_KEY = + "io.modelcontextprotocol/enterprise-managed-authorization"; + +/** + * The value the client stamps for each advertised extension. The wire shape is + * `{ [extensionId]: object }` (per `ClientCapabilities.extensions`); an empty + * object is the standard "declared, no sub-options" advertisement. + */ +export type ExtensionAdvertisement = NonNullable< + ClientCapabilities["extensions"] +>[string]; + +/** + * A single Inspector-advertisable extension. The registry of these is the shared + * source of truth for both the capability builder here and the Server Settings + * toggle UI (#1739), so the two never drift on which extensions exist or what + * they are called. + */ +export interface AdvertisableExtension { + /** Extension identifier stamped into `capabilities.extensions`. */ + key: string; + /** Human-readable label for the Server Settings toggle. */ + label: string; + /** + * Whether the Inspector advertises this extension when the user has expressed + * no explicit preference (the toggle's default position). + */ + defaultAdvertised: boolean; +} + +/** + * Catalog of extensions the Inspector can advertise and the user can toggle. + * EMA is deliberately absent — it is driven by the auth mode (see + * {@link EMA_EXTENSION_KEY}), not a standalone toggle. The `io.modelcontextprotocol/ui` + * Apps extension is added here in Phase 3 (#1740). + */ +export const ADVERTISABLE_EXTENSIONS: readonly AdvertisableExtension[] = [ + { + key: TASKS_EXTENSION_KEY, + label: "Tasks (io.modelcontextprotocol/tasks)", + // The modern Tasks extension (SEP-2663). Advertised by default so the SDK + // stamps it into every modern request envelope — the per-request + // declaration a server requires before it may return a `CreateTaskResult` + // (server-directed task creation). Harmless on legacy (extensions ignored). + defaultAdvertised: true, + }, +]; + +export interface BuildClientExtensionsInput { + /** True when the connection routes through the enterprise IdP (EMA). */ + enterpriseManaged: boolean; + /** + * Per-extension advertise overrides keyed by extension id, from + * {@link InspectorClientOptions.advertisedExtensions}. A key present here wins + * over the registry's `defaultAdvertised`; an absent key falls back to it. + */ + advertised?: Record; +} + +/** + * Assemble the `capabilities.extensions` map advertised at construction — the + * single source of truth that replaces the previously ad-hoc, per-extension + * spreads. Registry entries resolve to advertised/not via the user override with + * a registry-default fallback; EMA is layered on top as an auth-mode-driven + * built-in. + * + * With the Tasks entry defaulting to advertised, the map is non-empty for a + * default config, so `capabilities.extensions` is always attached. + */ +export function buildClientExtensions( + input: BuildClientExtensionsInput, +): Record { + const map: Record = {}; + for (const ext of ADVERTISABLE_EXTENSIONS) { + const advertised = input.advertised?.[ext.key] ?? ext.defaultAdvertised; + if (advertised) { + map[ext.key] = {}; + } + } + if (input.enterpriseManaged) { + map[EMA_EXTENSION_KEY] = {}; + } + return map; +} diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 89d0edea..ddb75141 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -127,6 +127,7 @@ import { isModernCreateTaskResult, type ModernDetailedTask, } from "./modernTaskSchemas.js"; +import { buildClientExtensions } from "./extensions.js"; import { EmptyResultSchema, CallToolResultSchema, @@ -430,6 +431,9 @@ export class InspectorClient extends InspectorClientEventTarget { private activeToolCallAbortController?: AbortController; // Receiver tasks (server-initiated: server sends createMessage/elicit with params.task, server polls us) private receiverTasks: boolean; + // Per-extension advertise overrides (#1738); undefined key falls back to the + // registry default in ADVERTISABLE_EXTENSIONS. + private advertisedExtensions?: Record; private receiverTaskTtlMs: number | (() => number); private receiverTaskRecords: Map = new Map(); // OAuth support (config owned by oauthManager; client delegates and uses !!oauthManager for "is OAuth configured") @@ -467,6 +471,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.sample = options.sample ?? true; this.elicit = options.elicit ?? true; this.receiverTasks = options.receiverTasks ?? false; + this.advertisedExtensions = options.advertisedExtensions; this.receiverTaskTtlMs = options.receiverTaskTtlMs ?? 60_000; this.progress = options.progress ?? true; this.resetTimeoutOnProgress = options.resetTimeoutOnProgress ?? true; @@ -594,22 +599,24 @@ export class InspectorClient extends InspectorClientEventTarget { }, }; } - if (options.oauth?.enterpriseManaged) { + // Assemble the advertised-extensions map from one builder (the single + // source of truth), instead of ad-hoc per-extension spreads. It layers the + // registry defaults (with any user overrides from `advertisedExtensions`) + // and the auth-mode-driven EMA extension. The Tasks entry defaults to + // advertised, so the map is non-empty and `capabilities.extensions` is + // always attached — the modern Tasks extension (SEP-2663) must ride every + // modern request envelope for a server to legally return a `CreateTaskResult` + // (harmless on legacy, where extensions are ignored). (#1738) + const advertisedExtensions = buildClientExtensions({ + enterpriseManaged: options.oauth?.enterpriseManaged ?? false, + advertised: this.advertisedExtensions, + }); + if (Object.keys(advertisedExtensions).length > 0) { capabilities.extensions = { ...capabilities.extensions, - "io.modelcontextprotocol/enterprise-managed-authorization": {}, + ...advertisedExtensions, }; } - // Advertise the modern Tasks extension (SEP-2663) so the SDK stamps it into - // every modern request's `clientCapabilities` envelope — the per-request - // declaration a server requires before it may return a `CreateTaskResult`. - // Harmless on legacy (extensions are ignored there). This is what makes - // server-directed ("unsolicited") task creation legal on modern, and it - // makes `capabilities` always non-empty, so it's always attached. - capabilities.extensions = { - ...capabilities.extensions, - [TASKS_EXTENSION_KEY]: {}, - }; clientOptions.capabilities = capabilities; this.clientCapabilities = capabilities; @@ -1806,8 +1813,10 @@ export class InspectorClient extends InspectorClientEventTarget { (params._meta as Record | undefined) ?? {}; const clientCapabilities = { ...this.clientCapabilities, - // extensions always carries the tasks extension (advertised at - // construction), so spreading it is never a no-op. + // Force-stamp the tasks extension regardless of what the client + // advertised at construction: the raw `tasks/*` channel requires it, and + // a user may disable general tasks advertisement via `advertisedExtensions` + // (#1738). So this stamp is load-bearing, not a redundant re-add. extensions: { ...this.clientCapabilities.extensions, [TASKS_EXTENSION_KEY]: {}, diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 87b83a51..3c6b4187 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -904,6 +904,16 @@ export interface InspectorClientOptions { */ roots?: Root[]; + /** + * Per-extension overrides for which extensions the Inspector advertises in + * `capabilities.extensions`, keyed by extension id. A present key wins over + * the registry default in `ADVERTISABLE_EXTENSIONS`; an absent key falls back + * to it. Lets a user toggle advertised extensions as a debugging knob — + * servers legitimately change tool registration on client-declared extensions + * (#1633). EMA is not configured here (it follows the auth mode). (#1738) + */ + advertisedExtensions?: Record; + /** * Whether to enable listChanged notification handlers (default: true) * If enabled, InspectorClient will subscribe to list_changed notifications and fire