Skip to content

feat(test-servers): mcp_app_demo preset + _meta on tool/resource defs#1587

Merged
cliffhall merged 1 commit into
1579-wave-1from
v2/1557-mcp-app-demo
Jul 2, 2026
Merged

feat(test-servers): mcp_app_demo preset + _meta on tool/resource defs#1587
cliffhall merged 1 commit into
1579-wave-1from
v2/1557-mcp-app-demo

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1557

Wave 1 of the PR #1510 decomposition (tracking #1579). Adds a bundled mcp_app_demo MCP App widget to test-servers/ so Apps-host conformance and CLI --app-info tests have a fixture to drive the full host surface without an external ext-apps server.

Changes (scoped to test-servers/)

  • composable-test-server.ts — optional _meta?: Record<string, unknown> on ToolDefinition and ResourceDefinition, passed through to the SDK's registerTool config (tool-level _meta) and onto the returned resource content item (resource-level _meta).
  • test-server-fixtures.tscreateMcpAppDemoTool() (carries _meta.ui.resourceUri + visibility), createMcpAppDemoResource() (carries _meta.ui.csp / permissions / prefersBorder), and the inline MCP_APP_DEMO_HTML widget (no external scripts; exercises ui/initialize, size-changed, ui/message, log notification, host-context render). Both are wired into getDefaultServerConfig().
  • preset-registry.tsmcp_app_demo tool preset and mcp_app_demo_widget resource preset.

Notes

  • The fixture HTML keeps a single rgba(0,0,0,0.06) in its inline <style>. This is a deliberate exception to the AGENTS.md color-token rule: it's a self-contained static fixture served into the sandbox iframe, not a Mantine component, so the --inspector-* tokens are not in scope. Documented with a comment at the source.
  • No preset-list docs / test-servers README exist to update.

Testing

  • New integration test clients/web/src/test/integration/mcp/mcp-app-demo.test.ts asserts the _meta plumbing end-to-end (tool-level _meta.ui.resourceUri on tools/list, resource-level _meta.ui on resources/read, and the tool handler echo).
  • cd clients/web && npm run validate — pass (2438 unit tests).
  • cd clients/web && npm run test:integration — pass (802 tests, 39 files).
  • cd clients/cli && npm run validate — pass (110 tests); confirms the two new default tools/resources don't regress the stdio CLI suite.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX

@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 1, 2026
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

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


Review: mcp_app_demo preset + _meta plumbing

  • Read changed files
  • Review composable-test-server.ts _meta plumbing
  • Review test-server-fixtures.ts fixture + widget HTML
  • Review preset-registry.ts presets
  • Review integration test
  • Post review feedback

Nice, tightly-scoped PR. The _meta passthrough is minimal and correct, the fixture is well-commented, the color-token exception is documented at the source as claimed, and the integration test asserts the plumbing end-to-end (tool-level _meta.ui.resourceUri on tools/list, resource-level _meta.ui on resources/read, and the handler echo). I confirmed the two new entries added to getDefaultServerConfig() don't regress the existing suites — the default-config assertions are all toBeGreaterThan(0), and the exact-count assertions (.toBe(3) etc.) run against separate createNumberedTools/createNumberedResources configs, not the default one.

A few observations, all non-blocking:

1. Resource _meta surfaces only on resources/read, not resources/list. composable-test-server.ts:573 attaches _meta to the returned content item (the resources/read response), which is exactly what the test checks. But the resource registered via registerResource gets no _meta in its listing metadata. Worth confirming that the downstream Apps-host / --app-info consumers (later waves of #1579) read the UI config from resources/read and not from the resources/list entry — if any path expects _meta.ui on the listing, this fixture won't drive it. If the intent is read-only, all good.

2. The widget's protocol method names aren't exercised by anything in this PR. The MCP_APP_DEMO_HTML uses ui/initialize, ui/notifications/initialized, ui/notifications/size-changed, ui/message, and ui/notifications/host-context-changed. None of these are covered by a test here, and the host-side AppRenderer.tsx drives the handshake through the AppBridge abstraction (ext-apps SDK) rather than raw postMessage, so a typo in one of these method names would slip through this PR and only surface when a later wave wires the fixture into the real host. Since this fixture exists precisely to drive that host surface, I'd recommend cross-checking each method name against the ext-apps spec / @modelcontextprotocol/ext-apps before those waves depend on it. (Given the fixture is deliberately no-SDK, this is a manual-diligence item, not something to add a test for now.)

3. Minor DRY. MCP_APP_DEMO_URI is defined in the fixture (test-server-fixtures.ts:757) and again in the test (mcp-app-demo.test.ts:13). Exporting the constant from the fixture and importing it would keep the two in lockstep. Trivial.

Nothing here blocks merge — items 1 and 2 are "confirm the assumption for the next wave" rather than defects in this diff. LGTM.
· branch v2/1557-mcp-app-demo

cliffhall added a commit that referenced this pull request Jul 1, 2026
Address code-review DRY note on PR #1587: MCP_APP_DEMO_URI was defined
in both the fixture and the integration test. Export it from the fixture
and import it in the test so the two stay in lockstep.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the review! Addressing each point:

1. Resource _meta on resources/read vs resources/list — Intentional and read-only. The downstream Apps-host / --app-info consumers (later waves of #1579) read the UI config from resources/read, which is what this fixture drives and what the test asserts. Leaving the listing metadata untouched on purpose.

2. Widget protocol method names — Noted as a manual-diligence item for the waves that wire this fixture into the real host; the method names (ui/initialize, ui/notifications/initialized, ui/notifications/size-changed, ui/message, ui/notifications/host-context-changed) are cross-checked against the ext-apps spec. No test added here since the fixture is deliberately no-SDK.

3. DRY on MCP_APP_DEMO_URI — Fixed in e5dbdca. Exported the constant from test-server-fixtures.ts and imported it in the integration test so the two stay in lockstep. Web validate + integration (802 passing) and CLI validate all green.

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

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


Re-review: mcp_app_demo preset + _meta plumbing

  • Read changed files at current HEAD
  • Verify prior review items (DRY fix in e5dbdca)
  • Review _meta plumbing + fixture + presets + test
  • Post review

Re-reviewed at HEAD (e5dbdca6). All three items from the prior review are resolved or confirmed:

  1. Resource _meta on resources/read only — confirmed intentional/read-only by the author; downstream --app-info consumers read from resources/read. ✅
  2. Widget protocol method names — confirmed cross-checked against the ext-apps spec as a manual-diligence item; no test added since the fixture is deliberately no-SDK. ✅
  3. DRY on MCP_APP_DEMO_URI — fixed: the constant is now exported from test-server-fixtures.ts:757 and imported into mcp-app-demo.test.ts:10, so the fixture and test stay in lockstep. ✅

The _meta passthrough in composable-test-server.ts is minimal and correct — conditional spreads (...(tool._meta != null && { _meta })) on both the registerTool config (:507) and the resources/read content item (:573), so servers without _meta are unaffected. The integration test asserts all three surfaces end-to-end (tool-level _meta.ui.resourceUri/visibility, resource-level _meta.ui.csp/permissions/prefersBorder, and the handler echo).

One new minor observation (non-blocking): the _meta passthrough is wired only into the regular tool-registration path. The task-tool path (registerToolTask config at composable-test-server.ts:490) does not spread _meta, so a future App tool that's also a task tool wouldn't surface tool-level _meta. Harmless for this PR — createMcpAppDemoTool() is a regular tool, not a task tool — but worth a comment there if a later wave of #1579 ever needs an App-flavored task tool. Fix this →

LGTM — the diff is tightly scoped, well-commented, and fully tested. Nothing blocks merge.
· branch v2/1557-mcp-app-demo

cliffhall added a commit that referenced this pull request Jul 1, 2026
…n path

Address round-2 code-review note on PR #1587: the _meta passthrough was
wired only into the regular registerTool path, so an App-flavored task
tool would not surface tool-level _meta. Add _meta to TaskToolDefinition
and a matching conditional spread on both registerToolTask overload
configs so the two registration paths behave symmetrically.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
@cliffhall

Copy link
Copy Markdown
Member Author

Addressed the round-2 observation in 39f9baa:

Task-tool path _meta passthrough — Made the two registration paths symmetric. Added _meta to TaskToolDefinition and a matching conditional _meta spread on both registerToolTask overload configs (with and without inputSchema), mirroring the regular registerTool path. So a future App-flavored task tool will surface tool-level _meta the same way. Web validate + integration (802 passing) and CLI validate all green.

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

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


Re-review: mcp_app_demo preset + _meta plumbing (round 3)

  • Read changed files at current HEAD
  • Verify round-2 fix (task-tool _meta passthrough in 39f9baa)
  • Review _meta plumbing, fixture, presets, and test
  • Post review

Re-reviewed at HEAD (39f9baab). The round-2 observation is resolved and everything else still holds.

Round-2 fix confirmed. TaskToolDefinition now carries _meta (composable-test-server.ts:120), and both registerToolTask overload configs spread it — the with-inputSchema branch (:487) and the without-inputSchema branch (:496). This is symmetric with the regular registerTool path (:511), so a future App-flavored task tool would surface tool-level _meta identically. Good, clean fix. ✅

The _meta plumbing is correct across all three surfaces:

  • Tool-level _metaregisterTool config (:511) and both task-tool configs (:487, :496), all via conditional spread ...(tool._meta != null && { _meta }) so _meta-less servers are untouched.
  • Resource-level _meta → the resources/read content item (:577).
  • Integration test asserts all three end-to-end: tool-level _meta.ui.resourceUri/visibility on tools/list, resource-level _meta.ui.csp/permissions/prefersBorder on resources/read, and the handler echo.

One remaining asymmetry (non-blocking, flagging only for the next wave). Resource _meta is spread only inside the default content handler at composable-test-server.ts:570-581. If a resource is registered with a customHandler via config.onRegisterResource (:558), that handler's contents fully replace the default, so resource._meta is dropped from the read response. Harmless here — createMcpAppDemoResource() uses the default handler — but if a later wave of #1579 needs an App UI resource with a custom read handler, the _meta.ui block won't reach the client unless the custom handler re-adds it. Worth a one-line doc note on ResourceDefinition._meta or onRegisterResource if that combination ever comes up. Note it →

The two prior items (resource _meta on resources/read only, and the widget protocol method names cross-checked against the ext-apps spec) were confirmed intentional/manual-diligence by the author.

LGTM — tightly scoped, well-commented, and fully tested. Nothing blocks merge.
· branch v2/1557-mcp-app-demo

cliffhall added a commit that referenced this pull request Jul 1, 2026
Address round-3 code-review note on PR #1587: resource `_meta` is applied
only by the default read handler; a customHandler from onRegisterResource
replaces the contents wholesale and would drop it. Document this on
ResourceDefinition._meta so a future App UI resource with a custom read
handler knows to re-add _meta itself.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
@cliffhall

Copy link
Copy Markdown
Member Author

Addressed the round-3 observation in 8059a3d:

Custom resource handler _meta asymmetry — Added a doc note on ResourceDefinition._meta explaining that only the default read handler applies _meta; a customHandler from config.onRegisterResource replaces the contents wholesale, so such a handler must re-add _meta itself if the resource needs it on the read response. (No behavior change — createMcpAppDemoResource() uses the default handler; this documents the constraint for a future App UI resource that needs a custom read handler.) Web validate + integration (802 passing) and CLI validate all green.

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

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


Re-review: mcp_app_demo preset + _meta plumbing (round 4)

  • Read changed files at current HEAD (8059a3d1)
  • Verify round-3 fix (custom-handler _meta doc note)
  • Review _meta plumbing, fixture, presets, and test
  • Post review

Re-reviewed at HEAD (8059a3d1). The round-3 observation is resolved and everything from the prior rounds still holds. This PR has fully converged.

Round-3 fix confirmed. ResourceDefinition._meta now carries the doc note (composable-test-server.ts:130-134) spelling out that only the default read handler applies _meta, and that a customHandler from config.onRegisterResource replaces contents wholesale — so such a handler must re-add _meta itself. Accurate against the code: the default handler at :572-584 spreads resource._meta into the content item, while a customHandler short-circuits it (customHandler || (async () => {...})). No behavior change; documents the constraint for a future custom-handler App resource. ✅

Full plumbing re-verified — correct across all four surfaces:

  • Tool-level _meta → regular registerTool config (:514) and both registerToolTask overload configs (:490 with-inputSchema, :499 without), all via the conditional spread ...(tool._meta != null && { _meta: tool._meta }), so _meta-less servers are untouched.
  • Resource-level _meta → the default resources/read content item (:580), with the documented custom-handler caveat.
  • TaskToolDefinition._meta mirrors ToolDefinition._meta with a matching doc comment (:119-120).

Fixture & presets: MCP_APP_DEMO_URI is exported once (test-server-fixtures.ts:758) and reused by both the tool's _meta.ui.resourceUri and the test (mcp-app-demo.test.ts:10), so they stay in lockstep. The widget HTML is well-commented — the ui/initialize request/response handshake, origin-pinning discipline (HOST_ORIGIN capture + inbound ev.origin check), and partial-context merge in renderCtx all read correctly against the spec references cited inline. The rgba(0,0,0,0.06) color-token exception remains documented at the source. Both presets are wired (preset-registry.ts:158, :182).

Test coverage asserts all three consumer-facing surfaces end-to-end: tool-level _meta.ui.resourceUri/visibility on tools/list, resource-level _meta.ui.csp/permissions/prefersBorder on resources/read, and the handler echo — over a real HTTP transport.

The two earliest items (resource _meta on resources/read only, and the widget protocol method names cross-checked against the ext-apps spec) were confirmed intentional/manual-diligence by the author.

LGTM — no outstanding items. The diff is tightly scoped (+288/-0, test-servers-only plus one integration test), well-documented, and fully tested. Nothing blocks merge.
· branch v2/1557-mcp-app-demo

@cliffhall

Copy link
Copy Markdown
Member Author

📋 Smoke test & AGENTS.md audit for this PR: #1557 (comment) (posted on the linked issue #1557).

@cliffhall cliffhall changed the base branch from v2/main to 1579-wave-1 July 2, 2026 16:44
@cliffhall cliffhall merged commit b96fd6e into 1579-wave-1 Jul 2, 2026
@cliffhall cliffhall deleted the v2/1557-mcp-app-demo branch July 2, 2026 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test-servers: mcp_app_demo preset + _meta support on tool/resource definitions

1 participant