Extensions: first-class advertised-extensions map (Phase 1 of #1633)

[cliffhall]

Closes #1738

Phase 1 of #1633 (Extensions capability UI + MCP Apps alignment). Collapses the two ad-hoc capabilities.extensions spreads in the InspectorClient constructor into a single builder that is the source of truth for what the client advertises — the foundation Phase 2 (#1739, settings toggle) and Phase 3 (#1740, Connection Info + io.modelcontextprotocol/ui) build on.

What changed

• core/mcp/extensions.ts (new)
  • ADVERTISABLE_EXTENSIONS registry — { key, label, defaultAdvertised }. The shared list the Server Settings toggle UI (Phase 2) will drive, so the client builder and the UI never drift.
  • EMA_EXTENSION_KEY constant (SEP-2350 enterprise-managed authorization).
  • buildClientExtensions({ enterpriseManaged, advertised }) — assembles the map: registry entries resolve to advertised/not via the user override with the registry default as fallback; EMA is layered on as an auth-mode-driven built-in (not a free toggle).
• core/mcp/types.ts — new InspectorClientOptions.advertisedExtensions (per-extension overrides, keyed by extension id; a present key wins over the registry default). Plumbed through the client here; wired to the form in Phase 2.
• core/mcp/inspectorClient.ts — constructor now calls buildClientExtensions() instead of the two hardcoded spreads.

Behavior preserved

• The Tasks extension (io.modelcontextprotocol/tasks, SEP-2663) defaults to advertised, so the map is non-empty and rides every modern request envelope — the per-request declaration a server needs before it may return a CreateTaskResult.
• EMA is advertised iff enterpriseManaged, exactly as before.
• The raw modern-task envelope re-stamp (withModernTaskEnvelope) is intentionally left untouched: it fires only on the raw tasks/* channel, which requires the extension anyway.

Tests

• New clients/web/src/test/core/mcp/extensions.test.ts: registry shape + every builder branch (defaults, EMA on/off, override enable/disable, unknown key). 100% lines/statements/functions/branches on the new module.
• The existing tasks-era and EMA integration suites still assert the same on-the-wire advertisement — unchanged and green.
• npm run ci green (validate → coverage ≥90% gate → smoke → Storybook).

Notes

• No ext-apps SDK-v1 shim in this phase — the io.modelcontextprotocol/ui advertisement and its likely cast land in Phase 3 (Extensions: Connection Info both eras + io.modelcontextprotocol/ui advertisement (Phase 3 of #1633) #1740). The follow-up issue to remove all ext-apps v1 shims will be filed once Extensions: Connection Info both eras + io.modelcontextprotocol/ui advertisement (Phase 3 of #1633) #1740 is in review, so it can enumerate every shim site.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 23s —— View job

---

Review — Extensions advertised-map (Phase 1 of #1633)

• Read the diff against origin/v2/main
• Verify no unused imports / dead code
• Assess builder logic & type safety
• Assess test coverage
• Post review

Nicely scoped refactor. The two ad-hoc capabilities.extensions spreads collapse into one builder, behavior is preserved on every path, and the new module is fully covered. I read the surrounding constructor and the raw-envelope path — no functional regressions. Details below.

Correctness — behavior preserved ✅

• buildClientExtensions reproduces the old semantics exactly: Tasks default-on (map always non-empty) + EMA iff enterpriseManaged. The Object.keys(...).length > 0 guard is therefore always true today but is a correct forward-guard for when all registry entries can be disabled (Phase 2).
• TASKS_EXTENSION_KEY's import is not left dead — it's still used by isTasksExtensionNegotiated() (inspectorClient.ts:1797) and withModernTaskEnvelope() (:1820). Good.
• At the merge site (inspectorClient.ts:615-618), capabilities.extensions is guaranteed undefined (fresh {} at :561, nothing sets .extensions before this block), so ...capabilities.extensions is a harmless defensive spread. Fine to keep.
• Type derivation ExtensionAdvertisement = NonNullable<ClientCapabilities["extensions"]>[string] is the right way to stay pinned to the SDK wire shape without a hand-rolled type. 👍

Minor — a comment goes stale under Phase 2 (non-blocking)

withModernTaskEnvelope() at inspectorClient.ts:1816-1817:

// extensions always carries the tasks extension (advertised at
// construction), so spreading it is never a no-op.

This is accurate today (no way to disable Tasks — advertisedExtensions isn't wired to any UI yet), but once Phase 2 (#1739) lets a user set advertisedExtensions: { [TASKS_EXTENSION_KEY]: false }, this.clientCapabilities.extensions will not carry Tasks, and this comment's premise becomes false. The code stays correct either way because it force-stamps [TASKS_EXTENSION_KEY]: {} unconditionally (correct — the raw tasks/* channel requires the extension regardless of what the user advertised generally), so this is purely a comment-accuracy note to fix up in Phase 2, not a bug here. The PR body already flags this envelope as intentionally untouched, so this is consistent with the plan.

Test coverage — thorough, one optional guard

The new suite exercises every branch of the builder and the registry shape (100% as claimed). One invariant worth locking in explicitly, since it's central to the design ("EMA is not a free toggle"):

it("does not let an override advertise EMA (auth-mode only)", () => {
  const map = buildClientExtensions({
    enterpriseManaged: false,
    advertised: { [EMA_EXTENSION_KEY]: true },
  });
  expect(map).not.toHaveProperty(EMA_EXTENSION_KEY);
});

The existing "ignores override keys that are not in the registry" test covers this generically (EMA isn't in the registry), but an EMA-specific assertion documents the intent and would catch a future regression where someone mistakenly adds EMA to ADVERTISABLE_EXTENSIONS. Optional. Fix this →

Nits

• types.ts advertisedExtensions doc references ADVERTISABLE_EXTENSIONS by name but not as a {@link} (it lives in another module) — fine as prose, just noting the asymmetry with the {@link}s used elsewhere in the new code.

Overall: LGTM. Clean single-responsibility module, faithful behavior preservation, and a genuine improvement to the drift risk between the client builder and the upcoming settings UI. The one comment-staleness item is a Phase-2 cleanup, and the extra test is optional.
· branch v2/extensions-advertised-map-1738

[cliffhall]

Thanks for the review! Addressed in 6153e48:

• Stale comment under Phase 2 — rewrote the withModernTaskEnvelope tasks-stamp comment now rather than deferring it, so it can't go stale: it explains the force-stamp is load-bearing (the raw tasks/* channel requires the extension) and explicitly anticipates a user disabling general tasks advertisement via advertisedExtensions in Phase 2. Logic unchanged.
• Optional EMA-override test — added does not let an override advertise EMA (auth-mode only). Agree it documents the "EMA is not a free toggle" invariant more directly than the generic unknown-key test and would catch a regression where someone adds EMA to ADVERTISABLE_EXTENSIONS.
• Nit: {@link ADVERTISABLE_EXTENSIONS} in types.ts — left as prose intentionally. types.ts doesn't import from extensions.ts, and adding an import solely for a doc link would introduce a needless module dependency (arguably a cycle risk as the two modules grow). Kept the plain-name reference.

CI is green (web coverage gate 4396 tests, extensions.ts 100%; the one TUI step-up failure in a local ci run was the known flaky already_authorized/step-up frame assertion and passed clean on re-run — unrelated to this diff, which only touches core extension assembly + a comment + a web test).

@claude review

[claude]

Claude finished @cliffhall's task in 43s —— View job

---

Re-review — Extensions advertised-map (Phase 1 of #1633)

• Read the follow-up commit 6153e482
• Verify the envelope comment rewrite
• Verify the EMA-override test
• Post review

Both items from the prior review are cleanly addressed in 6153e482, and the third (the {@link} nit) has a reasonable declined rationale. LGTM — ship it.

1. Envelope comment rewrite ✅

inspectorClient.ts:1816-1819 now reads as a load-bearing force-stamp rather than a "never a no-op" observation:

// 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.

This is the right framing — it explains why the unconditional stamp is correct (the raw tasks/* channel requires the extension irrespective of the general advertisement) and pre-empts the Phase-2 staleness I flagged. The code ([TASKS_EXTENSION_KEY]: {} unconditionally) is unchanged and remains correct. Good call fixing the comment now instead of deferring.

2. EMA-override test ✅

extensions.test.ts:70-78 locks in the "EMA is not a free toggle" invariant directly. I traced it against buildClientExtensions (extensions.ts:79-93): the loop only iterates ADVERTISABLE_EXTENSIONS (which excludes EMA), and EMA is added only under input.enterpriseManaged. So with enterpriseManaged: false, an advertised: { [EMA_EXTENSION_KEY]: true } override has no path into the map — the assertion is exercising exactly that gap. If someone later mistakenly adds EMA to the registry, defaultAdvertised/override resolution would start honoring the override and this test flips red. Precisely the regression guard intended.

3. {@link ADVERTISABLE_EXTENSIONS} nit — declined, reasonable ✅

Agreed. types.ts has no import from extensions.ts, and adding one purely for a doc link would create a module dependency (and a plausible cycle as both modules grow in Phases 2–3) for zero runtime value. Prose reference is the right tradeoff.

CI note

Acknowledged — the diff touches only core extension assembly, one comment, and one web test; the flaky TUI already_authorized step-up frame assertion is unrelated to this change and passed on re-run.

Nothing further from me. Clean Phase-1 foundation.
· branch v2/extensions-advertised-map-1738

[cliffhall]

Also fixed the flaky TUI step-up test in this PR (3770f9c), since it can trip CI here.

Root cause was a test-infra timing budget, not product code: waitForFrame/waitUntil polled only 25× at a 25ms tick (~625ms), and the step-up flow has an extra async OAuth-runner hop plus React commits that can exceed that under CI load with v8 coverage. Hoisted the count into a named POLL_TRIES = 100 (~2.5s). A poll exits the instant its predicate is true, so the higher ceiling never slows a passing assertion — it only widens the margin for the slow path. The .not.toContain negative checks are unaffected (they assert on an already-settled frame, not a wait-bounded poll). Verified 5× green (3 plain + 2 under --coverage).

@claude review

[claude]

Claude finished @cliffhall's task in 1m 5s —— View job

---

Review — TUI step-up de-flake (3770f9c8)

• Read the App.test.tsx diff
• Verify the poll-count change and waitForFrame/waitUntil helpers
• Check the reasoning (predicate short-circuit, negative checks unaffected)
• Post review

LGTM — correct, minimal, and the reasoning holds up. This is a pure test-infra timing fix; no product code touched.

The change is sound

Hoisting 25 → POLL_TRIES = 100 as the default tries for both waitUntil (App.test.tsx:527-529) and waitForFrame (:541) is the right shape. I traced the loop (:530-534): it returns the instant predicate() is true, so on the passing path the ceiling is never reached — raising it from ~625ms to ~2.5s widens the slow-path margin without adding a single tick to any assertion that settles quickly. Your commit-message claim checks out against the code.

Verified the "negative checks unaffected" claim

This was the one thing worth confirming, and it holds:

• Every negative assertion in the suite is a direct expect(r.lastFrame() ?? "").not.toContain(...) on an already-settled frame (:712, :723, :744, :754, :774, :933, …), not a wait-bounded poll — so the higher ceiling can't touch them.
• Grepped for the one pattern that would be penalized — a waitUntil(() => !…) used to prove a negative by timing out — and there are none. So no test pays the 4× ceiling as a false-negative cost. Good; that's the failure mode that would have made this a bad trade.

The flaky path is the one being fixed

expectFrame (:546-548) — await waitForFrame(...) then expect(...).toContain(...) — is exactly what the step-up cases use for the "Step-up authorization succeeded" frame (:1123, :1297, :1309, :1380, :1394). Those sit behind the extra async OAuth-runner hop the comment calls out, so they're precisely the assertions that were straddling the old ~625ms budget under CI + v8 coverage. Targeting the shared default rather than sprinkling per-call tries overrides keeps it in one place.

Minor (non-blocking)

• The only real cost is wall-clock on a genuinely failing assertion: worst case grows from ~0.6s to ~2.5s per failed poll. That's acceptable — failures are rare and this is diagnostic-time only — but it's the honest tradeoff of a timing-ceiling bump. If step-up flakes ever recur even at 2.5s, the durable fix would be to await a deterministic signal (e.g. the OAuth-runner mock's settle) rather than climbing the tick ceiling again; not needed now.
• Scope note: this test-only commit rides on the Extensions Phase-1 PR. It's clearly explained and low-risk, so folding it in is fine — just flagging that a reviewer bisecting later will find a TUI-timing change under an "Extensions" PR title.

Nothing to change. Ship it.
· branch v2/extensions-advertised-map-1738

[cliffhall]

Thanks — noted both:

1. Wall-clock on genuine failures (0.6s→2.5s per failed poll) — agreed, acceptable at diagnostic-time only. If step-up ever flakes again even at 2.5s, I'll switch that path to await a deterministic settle signal from the OAuth-runner mock rather than climbing the tick ceiling further.
2. Scope: test-only TUI-timing commit under an "Extensions" PR — fair flag for a future bisector. Kept it here since the flake can trip CI on this very PR (it did in a local npm run ci), and it's isolated in its own commit (3770f9c) with a self-explanatory message, so a bisect lands on a clearly-labeled change.

No further changes. Both review rounds are green — ready to merge.