Extensions: advertised-extensions toggle in Server Settings (Phase 2 of #1633)

[cliffhall]

Closes #1739

Phase 2 of #1633. Adds the advertised-extensions toggle — the core debugging knob of the tracking issue: users choose which extensions the Inspector advertises to a server, and a server may register different tools in response. Builds directly on Phase 1's registry + advertisedExtensions client option (#1738, merged).

What changed

Settings + persistence (core)

• InspectorServerSettings.advertisedExtensions and the matching StoredMCPServer field, wired through serverList's read (storedFieldsToInspectorSettings) and write (inspectorSettingsToStoredFields) converters and the source-of-truth INSPECTOR_FIELD_KEY_MAP. Omit-when-empty on disk for a byte-stable round-trip.
• validateSettings (remote /api/servers PUT route): an isBooleanRecord guard rejects a malformed map; a valid non-empty map is carried, an empty one is dropped.
• App.tsx threads savedSettings.advertisedExtensions into the InspectorClient options (absent/empty → registry defaults).

UI

• ServerSettingsForm renders an "Advertised Extensions" checkbox group driven by ADVERTISABLE_EXTENSIONS; a per-server override wins over the registry default (mirrors buildClientExtensions). The change takes effect on the next connect (surfaced in the description).
• ServerSettingsModal folds a toggle into settings.advertisedExtensions.

Test server + end-to-end proof

• composable-test-server gains extensionGatedTools: a named tool is registered disabled and enabled on notifications/initialized only when the connected client declared the mapped extension (getClientCapabilities().extensions). Legacy stateful leg (the modern per-request leg has no persistent oninitialized). Threaded through load-config / resolve-config.
• test-servers/configs/advertised-extensions-http.json showcase + README note: echo always, get_weather gated on io.modelcontextprotocol/tasks.
• Integration test connects legacy with and without the tasks extension advertised and asserts the gated tool appears / disappears in tools/list. This is the acceptance criterion ("demonstrably changes server tool registration"), proven end to end rather than asserted.

Tests

• serverList round-trip (non-empty / absent / empty map), ServerSettingsForm toggle rows (default-checked, override-unchecks, click fires), ServerSettingsModal fold, validateSettings accept/reject/empty, and the integration gating test (3 cases).
• npm run ci green — validate → coverage (per-file ≥90 gate held) → smoke → Storybook.

Proof screenshots

End-to-end smoke test in the web client against the legacy advertised-extensions-http.json test server (port 3220), which gates get_weather on the io.modelcontextprotocol/tasks extension. More detail in pr-screenshots/README.md.

With Tasks advertised (default): the new toggle is on, and tools/list returns echo + get_weather.

Uncheck Tasks and reconnect: the client advertises no extensions, the server never enables the gated tool, and tools/list returns only echo.

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), after which the ext-apps-v1-shim-removal follow-up gets filed (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 27s —— View job

---

Code Review — Advertised-extensions toggle (Phase 2 of #1633)

• Read core changes (types, serverList, server.ts)
• Read UI changes (ServerSettingsForm, Modal, App)
• Read test-server + integration changes
• Review tests & coverage

Overall this is a clean, well-scoped, and thoroughly-tested PR. The registry-as-source-of-truth pattern (ADVERTISABLE_EXTENSIONS shared by buildClientExtensions and the form) keeps the UI and the wire from drifting, the omit-on-empty persistence keeps the round-trip byte-stable, and the integration test proves the acceptance criterion end-to-end rather than asserting it. I found no correctness bugs — just a couple of minor observations.

Observations (minor / optional)

1. Toggling an override back to its default value persists a redundant entry. core/mcp/serverList.ts:322 and ServerSettingsModal.tsx:179
The modal's handleAdvertisedExtensionChange always writes [key]: checked, and persistence only drops the map when it is entirely empty. So a user who unchecks Tasks and then re-checks it ends up with advertisedExtensions: { "io.modelcontextprotocol/tasks": true } on disk — functionally identical to the default (resolution is override ?? default), but no longer the byte-stable "no-override" file. Not a bug (the resolved behavior is the same), but it slightly undercuts the "keep the diff minimal for the common case" goal once any toggle is touched. If you wanted the round-trip to reconverge, the fold could delete the key when checked === ext.defaultAdvertised. Given this is a deliberate debugging knob, keeping the explicit state is a defensible choice too — flagging only so it's a conscious decision.

2. App.tsx:2274 conditional-spread evaluates to ...false on an empty map. The savedSettings?.advertisedExtensions && Object.keys(...).length > 0 && { … } expression yields false (not {}) when the map is empty, and ...false is a harmless no-op in an object literal — so this is correct, just a touch subtle. Matches the surrounding pattern (protocolEra), so consistent with the file's idiom.

Things I specifically checked and liked

• wireExtensionGatedTools correctly chains the prior oninitialized (composes with modern-tasks wiring) and reads getClientCapabilities()?.extensions — and enabling before the first tools/list is sound because initialized precedes it.
• The "fresh server per scenario" note in the integration test is exactly right — the gate only enables, so reusing a server would leak state across cases.
• isBooleanRecord treats {} as valid (vacuous .every) and the length guard downstream drops it — consistent accept-but-don't-persist behavior, and the three validateSettings tests (non-boolean value, non-object, empty) cover each branch.
• Legacy-only scoping of the gate (modern per-request leg has no persistent oninitialized) is documented in the config type, the wiring, and the README.

Nice work — LGTM modulo the optional persistence-reconvergence nit above.

[cliffhall]

Thanks for the thorough review! Addressed in bf3afe1:

• Observation 1 (redundant override on re-check) — implemented the reconvergence. handleAdvertisedExtensionChange now deletes the key when the toggle returns to the extension's registry default, and drops the whole map to undefined when it empties. So unchecking then re-checking Tasks returns the file to the byte-stable no-override state rather than persisting { tasks: true }. I went with reconvergence over keeping explicit state because it matches the omit-when-default policy every other setting (protocolEra, modernLogLevel, paginatedLists, …) already uses — consistency wins here. Added a modal test asserting it.
• Observation 2 (...false conditional spread in App.tsx) — left as-is; as you noted it's a correct no-op and matches the protocolEra/modernLogLevel idiom right above it in the same block, so I kept it consistent rather than special-casing.

Also added end-to-end proof screenshots to the PR description (four shots: settings toggle on/off × tools list with/without the gated get_weather), driven in the real web client against the advertised-extensions-http.json server.

npm run ci green.

@claude review

[claude]

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

---

Re-review — reconvergence fix (bf3afe1)

• Read the reconvergence change in ServerSettingsModal.tsx
• Verify the new modal test
• Confirm consistency with omit-when-default policy
• Post verdict

The reconvergence fix looks correct and well-implemented. LGTM. ✅

handleAdvertisedExtensionChange (ServerSettingsModal.tsx:180-194) now:

• Copies the current map ({ ...settings.advertisedExtensions }, safely {} when the source is undefined).
• Looks up the registry entry and, when checked === ext.defaultAdvertised, deletes the key rather than persisting a redundant { key: <default> }.
• Drops the whole map to undefined once it empties (Object.keys(next).length > 0 ? next : undefined).

This exactly matches the override ?? default resolution in buildClientExtensions (core/mcp/extensions.ts), so the on-disk state reconverges to the byte-stable no-override file while preserving identical resolved behavior — and it aligns with the omit-when-default policy every other setting (protocolEra, modernLogLevel, paginatedLists, …) already follows. Choosing reconvergence over explicit-state for consistency is the right call.

Details I checked

• Unknown-key safety: if ADVERTISABLE_EXTENSIONS.find(...) returns undefined (a key not in the registry), the else branch writes next[key] = checked — a sensible fallback, though in practice the form only emits registry keys.
• undefined spread guard: { ...undefined } → {}, so the first override on a server with no prior map works correctly.
• Test coverage: the new test (ServerSettingsModal.test.tsx:192) starts from the sole disabling override { tasks: false }, re-checks Tasks (→ its defaultAdvertised: true), and asserts onSettingsChange fires with advertisedExtensions: undefined — precisely the empty-map-drops-to-undefined path. Good, targeted assertion.
• Single write path: the modal owns the fold; ServerSettingsForm just emits (key, checked), so there's no second place that could persist a redundant entry.

Observation 2 (the ...false conditional spread in App.tsx) was correctly left as-is — it's a harmless no-op matching the surrounding protocolEra/modernLogLevel idiom.

Nothing further from me — the earlier review's sole actionable nit is resolved cleanly, with a test. 👍