Skip to content

feat(core): add AppInfo + extractAppInfo() to core/mcp/apps.ts#1584

Merged
cliffhall merged 1 commit into
1579-wave-1from
v2/1556-core-appinfo
Jul 2, 2026
Merged

feat(core): add AppInfo + extractAppInfo() to core/mcp/apps.ts#1584
cliffhall merged 1 commit into
1579-wave-1from
v2/1556-core-appinfo

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1556

Summary

Wave 1 of the PR #1510 decomposition (tracking #1579). Adds a machine-readable app summary to core/mcp/apps.ts:

  • AppInfo interface — hasApp, toolName, resourceUri, visibility, plus resource-side csp / permissions / domain / prefersBorder / resourceMimeType.
  • extractAppInfo(tool, resource?) — merges the tool's _meta.ui (resourceUri, visibility) with the linked UI resource's _meta.ui (csp, permissions, domain, prefersBorder). Returns { hasApp: false, toolName } for non-App tools and propagates the underlying throw on a malformed resourceUri.
  • Private helpers readUiMeta(), normalizeResourceUri(), findResourceContent(). Content-block matching is exact-URI → lowercase + trailing-slash normalized → single-block fallback (since resources/read returns the requested resource by definition), then result-level _meta.ui as a last resort.

Per spec, csp / permissions / domain live on the resource (not the tool), so a caller wanting the security posture must read the resource and pass it. _meta.ui carriers are read structurally (the ext-apps named types aren't importable under NodeNext), with no any.

Why

Shared plumbing for the programmatic-review path — the CLI --app-info mode and integration tests need one canonical answer to "does this tool have an app, and what does it ask for?" without rendering it.

Tests

clients/web/src/test/core/apps.test.ts — new extractAppInfo describe block (non-App tool, tool-only info, full merge, result-level fallback, prefersBorder:false, malformed-URI throw, and a resource-content URI matching group covering exact/normalized/single-block/no-arbitrary-pick/empty-URI cases). 25 tests pass.

Coverage for core/mcp/apps.ts: 100% stmts / 96.66% branch / 100% funcs / 100% lines (all ≥90 gate). cd clients/web && npm run validate passes (2449 unit tests).

🤖 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 cliffhall linked an issue Jul 1, 2026 that may be closed by this pull request
@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 2m 18s —— View job


Review: AppInfo + extractAppInfo()

  • Read changed files
  • Analyze diff against origin/v2/main
  • Review for correctness, spec compliance, best practices
  • Post review feedback

Overall this is a clean, well-documented, well-tested addition. The pure/isomorphic design, the "tool advertises URI+visibility / resource declares csp+permissions+domain+prefersBorder" split, and the structural _meta.ui reads (no any) are all sound and match how the web AppRenderer already consumes McpUiResourceMeta (createAppBridgeFactory.ts:88,144-145). No blocking issues found. A few observations below, mostly minor.

Correctness / design notes

1. _meta.ui resolution is either/or, not mergedcore/mcp/apps.ts:140-142

const resourceUi =
  readUiMeta(content as WithUiMeta | undefined) ??
  readUiMeta(resource as WithUiMeta | undefined);

If the matched content block has any _meta.ui (even just csp), the result-level _meta.ui is never consulted — so a domain declared only at the result level would be dropped when the content block also carries a partial ui. Per the current spec that's likely fine (posture lives on the content block), but if a server ever splits fields across both, this silently loses data. Worth a one-line comment noting the precedence is intentional, or a shallow merge ({ ...resultUi, ...contentUi }) if the spec permits both carriers. Not blocking.

2. domain: "" is dropped by the truthiness spreadcore/mcp/apps.ts:150

...(resourceUi?.domain ? { domain: resourceUi.domain } : {}),

prefersBorder correctly uses !== undefined to preserve false, but domain (and csp/permissions if empty {}) use truthiness. An empty-string domain is almost certainly meaningless, so this is fine in practice — just flagging the asymmetry with prefersBorder in case strictness is desired.

3. Single-block fallback returns a block regardless of URIcore/mcp/apps.ts:118. Documented and defensible (resources/read returns the requested resource by definition), and its mimeType flows into resourceMimeType. Good that the "multiple non-matching blocks" case is explicitly not guessed and is covered by a test.

4. readUiMeta treats arrays as objectsapps.ts:82 (typeof ui === "object" is true for arrays). Harmless (field reads return undefined), just noting it isn't a strict object guard.

Style / nits

  • Structural casts vs named types (apps.ts:62-85): the doc comment explaining why McpUiToolMeta/McpUiResourceMeta can't be imported under NodeNext is helpful. Note the web client does import McpUiResourceMeta (createAppBridgeFactory.ts:8) — because it resolves via Vite/bundler, not NodeNext — so the structural approach is genuinely core-specific. Consider a // TODO to switch to the named types if/when upstream fixes the extensionless re-export, so this doesn't linger as tech debt.
  • readUiMeta could take unknown and narrow internally, avoiding the three as WithUiMeta casts at the call sites — a small readability win, optional.

Tests

Thorough — exact/normalized/single-block/no-arbitrary-pick/empty-URI matching, prefersBorder:false, result-level fallback, and the malformed-URI throw are all covered. The 96.66% branch / 100% line coverage clears the ≥90 gate. 👍
v2/1556-core-appinfo

@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the round-1 review. Addressed the two cheap, zero-behavioral-risk nits in ee4b232:

  • readUiMeta casts — refactored to accept unknown and narrow internally, removing the three as WithUiMeta casts at the call sites.
  • Either/or vs merge precedence (item 1) — added a comment documenting that content-block-then-result precedence is intentional (posture lives on the content block per spec; deliberately no shallow merge).
  • Named-types TODO (style nit) — added a TODO to switch to McpUiToolMeta/McpUiResourceMeta once the upstream extensionless re-export is fixed for NodeNext.

Declined (all explicitly "fine in practice" in the review):

  • domain: "" truthiness asymmetry (item 2) — an empty-string domain is meaningless; keeping the truthiness spread.
  • Shallow merge of split _meta.ui fields (item 1, heavier option) — speculative per the current spec; documented the intentional precedence instead.
  • readUiMeta arrays-as-objects (item 4) — harmless (field reads return undefined); no strict-object guard added.

apps.ts coverage remains 100% lines / 97.05% branch / 100% funcs — clears the ≥90 gate.

@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 50s —— View job


Review (round 2): AppInfo + extractAppInfo()

  • Read updated files & round-1 changes
  • Verify round-1 nits were addressed
  • Review for new issues
  • Post feedback

Re-reviewed after ee4b232a. The round-1 follow-ups landed cleanly and I found no new issues. This looks ready to merge from my side.

Round-1 items — verified

  • readUiMeta now takes unknown and narrows internally (apps.ts:88-96). The three as WithUiMeta casts are gone from the call sites; readUiMeta(tool) / readUiMeta(content) / readUiMeta(resource) all pass structural values straight through. The double null-guard (carrier object → _meta?.ui object) is correct.
  • Either/or precedence documented (apps.ts:151-155). The comment makes the "content-block is authoritative, no shallow-merge" decision explicit — future readers won't mistake it for an oversight.
  • Named-types TODO added (apps.ts:72-73) so the NodeNext structural workaround doesn't linger silently as tech debt.
  • Declines are all reasonabledomain: "" truthiness, speculative split-carrier merge, and arrays-as-objects were each flagged "fine in practice" in round 1; keeping them is the right call.

New pass — nothing blocking

A couple of ultra-minor observations, purely optional, no change requested:

  • UiMetaShape.visibility is dead within this module (apps.ts:76). visibility is only ever read off toolUi and its type already flows from that read; the field on UiMetaShape is harmless documentation of the shared carrier shape but isn't load-bearing. Fine to leave for symmetry with the ext-apps types the TODO points at.
  • Mixed-carrier result is possible but sensible — when the single-block fallback matches a content block that carries no _meta.ui while the result level does, resourceMimeType comes from the content block and csp/permissions/domain from the result-level ui (apps.ts:156 ?? + apps.ts:168 independent content?.mimeType). That's a reasonable outcome, and the "does not pick an arbitrary block" test already exercises the result-level path. Not worth a dedicated test.

Tests & coverage

The resource-content URI matching group is thorough — exact / normalized (with the two-block guard so the single-block fallback can't mask a failed normalize) / single-block fallback / no-arbitrary-pick / empty-URI. 97% branch clears the ≥90 gate. 👍

Nice, tidy wave-1 slice. LGTM.
v2/1556-core-appinfo

@cliffhall

Copy link
Copy Markdown
Member Author

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

@cliffhall cliffhall changed the base branch from v2/main to 1579-wave-1 July 2, 2026 16:43
@cliffhall cliffhall merged commit 72ae119 into 1579-wave-1 Jul 2, 2026
@cliffhall cliffhall deleted the v2/1556-core-appinfo branch July 2, 2026 16:43
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.

core: add AppInfo + extractAppInfo() to core/mcp/apps.ts

1 participant