WIP: LCORE-1595: Design doc/s for MCP Apps integration#1443
WIP: LCORE-1595: Design doc/s for MCP Apps integration#1443anik120 wants to merge 1 commit intolightspeed-core:mainfrom
Conversation
anik120
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new design doc specifying Lightspeed as an MCP Host: discover tools with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Lightspeed
participant ToolRegistry
participant MCPToolServer
Client->>Lightspeed: POST /v1/query
Lightspeed->>ToolRegistry: Lookup tool definitions (check _meta.ui.resourceUri)
alt ui metadata present
Lightspeed->>Lightspeed: Enrich ToolResultSummary with ui_resource (proxy URL /v1/mcp-ui-resources/{server_name})
Lightspeed-->>Client: Respond with enriched results
Client->>Lightspeed: GET /v1/mcp-ui-resources/{server_name}?path=...
Lightspeed->>MCPToolServer: Proxy GET (validate server registration, propagate auth)
MCPToolServer-->>Lightspeed: Return UI resource (text/html / assets)
Lightspeed-->>Client: Return proxied UI resource
else no ui metadata
Lightspeed-->>Client: Respond without ui_resource
end
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md`:
- Around line 416-417: The fetch_url construction embeds raw ui:// URIs into
path segments (see fetch_url and endpoint /v1/mcp-ui-resources using server_name
and resource_uri); instead, percent-encode or use URL-safe base64 to encode
resource_uri when building fetch_url and update the corresponding server-side
handler to decode and validate the encoded value before use; ensure encoding is
applied everywhere the ui:// resource URI is placed in a path (examples around
lines referencing fetch_url, server_name, resource_uri and the
/v1/mcp-ui-resources endpoints) so routing and canonicalization are robust.
- Around line 329-349: The cache currently keyed only by tool_name in
ToolDefinitionCache (_tools and methods update_from_toolgroups and
get_tool_metadata) will collide across servers; change the key to a composite
identifier (e.g., (server_name, tool_name) or f"{server_name}:{tool_name}") when
storing and retrieving entries so metadata like _meta.ui.resourceUri is
server-scoped, update update_from_toolgroups to extract the server identifier
from each toolgroup/tool payload and use that when inserting into self._tools,
and update get_tool_metadata to accept (or construct) the same composite key (or
a helper method that maps (server_name, tool_name) to the stored key) so lookups
return the correct server-scoped tool definition.
- Around line 445-474: The get_mcp_ui_resource handler currently only checks
that server_name is registered but does not validate the ui:// resource URI;
update the request handling (in get_mcp_ui_resource and the analogous logic
around the other ui resource handler) to parse the incoming resource URI and
enforce that its scheme equals "ui" and that the parsed URI host
(resource_uri.host) exactly matches the provided server_name before performing
any downstream fetch; if either check fails, raise HTTPException(400) or 404 as
appropriate to stop cross-server fetches.
- Line 135: Fix the typo "invesitage" to "investigate" in the sentence currently
reading "Need to invesitage Llama stack's plan for MCP apps support" and remove
all trailing spaces flagged by markdownlint on the noted lines (the sentence
around the typo and the other occurrences). After editing, run the repository's
markdown linting/CI check to confirm no remaining trailing-whitespace or
spelling issues in the document and commit the cleaned-up markdown.
- Around line 611-614: The current guideline that specifies "sandbox attributes:
sandbox=\"allow-scripts allow-same-origin\"" weakens isolation; change the
recommendation to avoid combining allow-scripts with allow-same-origin: specify
sandbox=\"allow-scripts\" (no allow-same-origin) and update the "UI resources
MUST be rendered in sandboxed iframes" guidance to require origin isolation for
untrusted content (serve from a dedicated origin or subdomain) and/or enforce
COOP/COEP headers for cross-origin isolation; also call out using
postMessage-based APIs and strict CSP for needed interactions instead of
relaxing sandbox flags.
- Around line 595-599: The example uses top-level import and await (import { App
} from '@modelcontextprotocol/ext-apps'; const app = new App(); await
app.connect();) inside a non-module <script>, which is invalid; update the
snippet to use a module script by changing the tag to <script type="module"> so
the import and top-level await for App and app.connect() are valid and match the
later example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9a060fc-29a1-4170-8270-c6704409f392
📒 Files selected for processing (1)
docs/design/mcp-apps-for-ui-components/mcp-apps.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🧰 Additional context used
🪛 LanguageTool
docs/design/mcp-apps-for-ui-components/mcp-apps.md
[style] ~105-~105: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ....5.2` - MCP Apps announced: January 26, 2026 - Issue: llama-stack 0.5.2 likely p...
(MISSING_COMMA_AFTER_YEAR)
[style] ~120-~120: The double modal “Requires sandboxed” is nonstandard (only accepted in certain dialects). Consider “to be sandboxed”.
Context: ...handle UI resource rendering - Requires sandboxed iframe implementation - Scope: This...
(NEEDS_FIXED)
[grammar] ~135-~135: Ensure spelling is correct
Context: ... abstraction layer Cons: - Need to invesitage Llama stack's plan for MCP apps support...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.0)
docs/design/mcp-apps-for-ui-components/mcp-apps.md
[warning] 35-35: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 49-49: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 73-73: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 125-125: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 139-139: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 158-158: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 163-163: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 163-163: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 198-198: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 204-204: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 274-274: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 275-275: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 391-391: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 516-516: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 539-539: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 556-556: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 580-580: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 580-580: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 586-586: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 643-643: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 685-685: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 704-704: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (1)
docs/design/mcp-apps-for-ui-components/mcp-apps.md (1)
630-658: Testing strategy is well-structured and release-ready.Good split across unit/integration/E2E with concrete Behave scenarios and assertions for
ui_resourceand proxy behavior.
| - UI resources MUST be rendered in sandboxed iframes | ||
| - Sandbox attributes: `sandbox="allow-scripts allow-same-origin"` | ||
| - CSP headers to restrict external resource loading | ||
|
|
There was a problem hiding this comment.
Sandbox policy weakens isolation when combining allow-scripts with allow-same-origin.
Serving untrusted HTML under the same origin with both flags risks token/session exposure and privileged API access. Prefer origin isolation (dedicated domain) and avoid allow-same-origin unless absolutely required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 611 - 614,
The current guideline that specifies "sandbox attributes:
sandbox=\"allow-scripts allow-same-origin\"" weakens isolation; change the
recommendation to avoid combining allow-scripts with allow-same-origin: specify
sandbox=\"allow-scripts\" (no allow-same-origin) and update the "UI resources
MUST be rendered in sandboxed iframes" guidance to require origin isolation for
untrusted content (serve from a dedicated origin or subdomain) and/or enforce
COOP/COEP headers for cross-origin isolation; also call out using
postMessage-based APIs and strict CSP for needed interactions instead of
relaxing sandbox flags.
55f97a7 to
f394896
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md`:
- Around line 613-617: Add a concrete "Rate limiting and resilience" subsection
under "Security Considerations" for the /v1/mcp-ui-resources endpoint: specify
per-client rate limits (e.g., max requests per minute per auth token and
guidance to consider shared quotas with /v1/query), describe resource-level
caching of fetched UI assets with TTL or cache-control header behavior to reduce
upstream load, and define a circuit-breaker/failure-tracking policy for MCP
server fetches that temporarily disables fetches for failing servers and returns
cached content or 503 with Retry-After; reference the endpoint name
(/v1/mcp-ui-resources), the existing "Rate limit UI resource fetches" note, and
recommend documenting these before Phase 2.1 implementation.
- Around line 181-193: Add a cache invalidation section to the "Tool Definition
Cache" describing explicit triggers and refresh behavior for ui_resource
metadata: list invalidation triggers (MCP server add/remove, toolgroup listing
refresh, tool definition updates), recommend a TTL for dynamic servers, specify
refresh actions (re-populate cache after each /v1/tools call and clear
server-specific entries when a server is removed), and state the cache
consistency guarantees to document in the API docs; reference the "Tool
Definition Cache" section, "ui_resource" metadata, and the /v1/tools endpoint so
reviewers can locate and integrate these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dac9447d-c9bf-4c8d-bcf7-cb3cc58f3b16
📒 Files selected for processing (1)
docs/design/mcp-apps-for-ui-components/mcp-apps.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🧰 Additional context used
🪛 LanguageTool
docs/design/mcp-apps-for-ui-components/mcp-apps.md
[style] ~103-~103: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ....5.2` - MCP Apps announced: January 26, 2026 - Issue: llama-stack 0.5.2 likely p...
(MISSING_COMMA_AFTER_YEAR)
[style] ~118-~118: The double modal “Requires sandboxed” is nonstandard (only accepted in certain dialects). Consider “to be sandboxed”.
Context: ...handle UI resource rendering - Requires sandboxed iframe implementation - Scope: This...
(NEEDS_FIXED)
[grammar] ~133-~133: Ensure spelling is correct
Context: ... abstraction layer Cons: - Need to invesitage Llama stack's plan for MCP apps support...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.0)
docs/design/mcp-apps-for-ui-components/mcp-apps.md
[warning] 35-35: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 49-49: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 123-123: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 137-137: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 156-156: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 161-161: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 196-196: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 202-202: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 272-272: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 273-273: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 389-389: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 514-514: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 537-537: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 554-554: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 578-578: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 578-578: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 584-584: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 641-641: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 683-683: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 702-702: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (6)
docs/design/mcp-apps-for-ui-components/mcp-apps.md (6)
324-346: Past issue remains: Cache key collision across servers.The cache design still keys entries by
tool_namealone (Line 327, Line 337, Line 346), which will cause collisions when multiple MCP servers expose tools with identical names. This is critical because the wrongui_resourcemetadata could be attached to results.
411-415: Past issue remains: Raw UI URI embedded in URL path.Line 414 constructs
fetch_urlby embedding the rawresource_uri(e.g.,ui://kube-mcp/namespaces-table) directly into the path segment. This is brittle and can break routing. The issue is already flagged in past comments.
442-472: Past issue remains: Missing strict resource URI validation.The endpoint design shows validation stubs (Line 468-472: "# Validate server is registered") but doesn't specify the critical checks needed to prevent cross-server fetch abuse. The past comment correctly identifies this security gap.
593-601: Past issue remains: Invalid JavaScript syntax in example.The example uses top-level
importandawait(Lines 594, 596) inside a non-module<script>tag (Line 593). This is invalid syntax. The past comment correctly identifies this issue.
608-612: Past issue remains: Weak sandbox policy.Line 610 recommends
sandbox="allow-scripts allow-same-origin", which weakens isolation by allowing untrusted UI resources to access same-origin storage and APIs. This is a critical security issue already flagged in past comments.
308-311: The design follows the established backward-compatible pattern already used throughout the codebase. Addingui_resource: Optional[UIResourceMetadata] = Field(None, ...)to ToolResultSummary is safe because:
Existing serialization practice: The codebase extensively uses
model_dump(exclude_none=True)for JSON responses, which omits fields withNonevalues entirely. Existing clients will never see the new field unless a tool actually provides UI metadata.Tested pattern: The codebase has tests confirming this behavior (e.g.,
test_build_turn_serialization_excludes_none_referenced_documents), verifying that Optional fields withNonevalues are properly excluded from serialized output.No breaking changes: Unknown fields in JSON responses are safely ignored by API clients, so even older clients receiving the field will not break.
The addition aligns with dozens of existing Optional fields in
src/models/responses.py,src/utils/types.py, and across the codebase, confirming this is the established pattern for optional response data.
| The UI resource URI is **static metadata** tied to the tool definition, not dynamic per-execution. We can look it up from the tool definition we already cache. | ||
|
|
||
| **Pros:** | ||
| - Minimal changes to existing architecture | ||
| - No Llama Stack version dependency | ||
| - No need to implement full MCP protocol | ||
| - Clear separation: Llama Stack handles logic, Lightspeed adds presentation layer | ||
|
|
||
| **Cons:** | ||
| - Clients must fetch UI resources themselves (not embedded in response) | ||
| - Requires caching tool definitions with metadata | ||
|
|
||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider cache invalidation strategy for tool definitions.
The design states UI resource URIs are "static metadata tied to the tool definition" and proposes caching them. However, the design doesn't address when/how the cache is refreshed when:
- MCP servers are reconfigured
- Tool definitions are updated
- Dynamic MCP servers are added/removed via the API
Without a clear cache invalidation strategy, clients could receive stale ui_resource metadata pointing to non-existent or incorrect UI resources.
Suggested additions to the design
Consider adding to the Tool Definition Cache design:
-
Cache invalidation triggers:
- On MCP server add/remove events
- On toolgroup listing refresh (periodic or on-demand)
- TTL-based expiration for dynamic servers
-
Refresh strategy:
- Re-populate cache after each
/v1/toolsendpoint call - Clear server-specific entries when a server is removed
- Document cache consistency guarantees in API docs
- Re-populate cache after each
Add this to the "Tool Definition Cache" section around Line 314-351.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 181 - 193,
Add a cache invalidation section to the "Tool Definition Cache" describing
explicit triggers and refresh behavior for ui_resource metadata: list
invalidation triggers (MCP server add/remove, toolgroup listing refresh, tool
definition updates), recommend a TTL for dynamic servers, specify refresh
actions (re-populate cache after each /v1/tools call and clear server-specific
entries when a server is removed), and state the cache consistency guarantees to
document in the API docs; reference the "Tool Definition Cache" section,
"ui_resource" metadata, and the /v1/tools endpoint so reviewers can locate and
integrate these changes.
| **2. Server Validation** | ||
| - Validate server_name is registered MCP server | ||
| - Apply same authentication as tool calls | ||
| - Rate limit UI resource fetches | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Specify rate limiting strategy for UI resource endpoint.
Line 616 mentions "Rate limit UI resource fetches" but provides no specifics. Given that the new /v1/mcp-ui-resources endpoint can trigger external MCP server requests, unbounded access could:
- Enable DoS against MCP servers
- Cause cascading load if multiple clients request the same resource
- Exhaust backend connection pools
Recommended additions to the design
Add a subsection under "Security Considerations" specifying:
-
Per-client rate limits:
- Max requests per minute per authentication token
- Consider shared limits with
/v1/queryendpoint
-
Resource-level caching:
- Cache fetched UI resources (HTML is typically static per tool version)
- TTL or cache-control header-based expiration
- Reduces load on MCP servers
-
Circuit breaker pattern:
- Track MCP server fetch failures
- Temporarily disable fetching from failing servers
- Return cached version or 503 with retry-after
This should be documented before implementation (Phase 2.1).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 613 - 617,
Add a concrete "Rate limiting and resilience" subsection under "Security
Considerations" for the /v1/mcp-ui-resources endpoint: specify per-client rate
limits (e.g., max requests per minute per auth token and guidance to consider
shared quotas with /v1/query), describe resource-level caching of fetched UI
assets with TTL or cache-control header behavior to reduce upstream load, and
define a circuit-breaker/failure-tracking policy for MCP server fetches that
temporarily disables fetches for failing servers and returns cached content or
503 with Retry-After; reference the endpoint name (/v1/mcp-ui-resources), the
existing "Rate limit UI resource fetches" note, and recommend documenting these
before Phase 2.1 implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md`:
- Around line 392-414: The code path that sets tool_result.ui_resource currently
falls back to server_name or "unknown", which can produce invalid fetch_url;
update the enrichment so server identity is deterministically sourced and
required: when handling mcp_call (mcp_call_item) and resolving tool_def via
tool_cache.get_tool_metadata, extract server identity from the MCP payload
(e.g., a server_id/server_name field on mcp_call_item) or from an explicit
call-context map passed into this function, and if no server identity is
present, do not emit UIResourceMetadata (or raise/log an error) instead of
defaulting to "unknown"; ensure tool_result.ui_resource and the
UIResourceMetadata constructor only use the validated server_name value.
- Around line 592-596: The example's <script> block imports App from a bare
specifier and lacks type="module", which breaks in browsers; update both
occurrences where App is imported (the import of App from
'@modelcontextprotocol/ext-apps' and the instantiation/await app.connect()) to
use a module script (add type="module" on the <script> tag) and switch the
import to a browser-resolvable ESM URL (e.g., the same pattern used later like
'https://esm.sh/@modelcontextprotocol/ext-apps@1.1.2') or document that a
bundler/import-map is required—ensure the code shows "import { App } from
'https://...'; const app = new App(); await app.connect();" inside <script
type="module"> for both places.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6653970-1088-4670-9ae3-dd4048cadcad
📒 Files selected for processing (1)
docs/design/mcp-apps-for-ui-components/mcp-apps.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
🪛 LanguageTool
docs/design/mcp-apps-for-ui-components/mcp-apps.md
[style] ~103-~103: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ....5.2` - MCP Apps announced: January 26, 2026 - Issue: llama-stack 0.5.2 likely p...
(MISSING_COMMA_AFTER_YEAR)
[style] ~118-~118: The double modal “Requires sandboxed” is nonstandard (only accepted in certain dialects). Consider “to be sandboxed”.
Context: ...handle UI resource rendering - Requires sandboxed iframe implementation - Scope: This...
(NEEDS_FIXED)
[grammar] ~133-~133: Ensure spelling is correct
Context: ... abstraction layer Cons: - Need to invesitage Llama stack's plan for MCP apps support...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.0)
docs/design/mcp-apps-for-ui-components/mcp-apps.md
[warning] 35-35: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 49-49: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 123-123: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 137-137: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 156-156: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 161-161: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 195-195: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 271-271: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 272-272: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 388-388: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 513-513: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 536-536: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 553-553: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 577-577: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 577-577: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 583-583: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 640-640: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 682-682: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 701-701: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (4)
docs/design/mcp-apps-for-ui-components/mcp-apps.md (4)
326-346: Cache lookup is still ambiguous across MCP servers.
ToolDefinitionCacheandget_tool_metadata(tool_name)are still keyed by tool name only, so same-named tools across servers can attach the wrong UI metadata (Line 326, Line 336, Line 405).Also applies to: 405-406
410-414: Do not place rawui://URIs directly in URL paths.
fetch_urland request examples still embedui://...in path segments (Line 413, Line 567, Line 578, Line 648, Line 652), which is fragile for routing/canonicalization. Encode resource URI (URL-safe base64 or percent-encoding) and decode/validate server-side.Also applies to: 567-579, 648-653
467-471: Endpoint validation remains incomplete for cross-server safety.The flow validates registered
server_namebut still does not requirescheme == "ui"andresource_uri.host == server_namebefore fetch (Line 467, Line 613). This leaves cross-server fetch abuse risk.Also applies to: 612-615
609-610: Sandbox guidance still weakens iframe isolation.Recommending
sandbox="allow-scripts allow-same-origin"for untrusted UI content significantly weakens isolation (Line 609).
There was a problem hiding this comment.
♻️ Duplicate comments (7)
docs/design/mcp-apps-for-ui-components/mcp-apps.md (7)
619-623:⚠️ Potential issue | 🟠 MajorBrowser example is invalid: module import requires
type="module"and resolvable specifier.Line 620–623 should match the valid pattern already shown later (module script + ESM URL/import map note). Current snippet will fail in a plain browser context.
For `@modelcontextprotocol/ext-apps` browser usage, is `import { App } from '@modelcontextprotocol/ext-apps'` valid without an import map or bundler, and must inline scripts use `<script type="module">` for top-level `import`/`await`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 619 - 623, The browser snippet uses top-level ESM import and top-level await but is missing type="module" and a resolvable specifier/import map; update the snippet that imports App (import { App } from '@modelcontextprotocol/ext-apps') to use a <script type="module"> block and either a full ESM URL or document the required import map/bundler resolution, and show creating and awaiting the App instance (new App(); await app.connect()) within that module script so the example runs in a plain browser environment.
418-419:⚠️ Potential issue | 🟠 MajorAvoid raw URI/path embedding in fetch URLs; use encoded resource identifiers.
Use URL-safe encoding for the resource identifier in
fetch_url, then decode + validate server-side. This prevents routing/canonicalization bugs and path parsing edge cases.Also applies to: 599-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 418 - 419, The fetch URL currently embeds raw resource_path into the query string (see the fetch_url assignment using server_name and resource_path); change it to URL-safe encode the resource identifier (e.g., base64url or encodeURIComponent equivalent) when constructing fetch_url and ensure the server handler for /v1/mcp-ui-resources/{server_name} decodes and validates the incoming encoded path before use; update any client-side call sites that build fetch_url and the server-side validation logic that consumes the decoded identifier so canonicalization/authorization checks operate on the decoded, validated path.
457-478:⚠️ Potential issue | 🔴 CriticalRequire strict URI validation:
scheme == "ui"andresource_uri.host == server_name.Current flow reconstructs URI from
pathwithout validating host binding. This allows cross-server fetch abuse ifpathencodes a different host. Validate parsed URI strictly before any fetch and reject mismatches with 400/404.Also applies to: 592-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 457 - 478, Reconstructing resource_uri from path allows an attacker to encode a different host; parse the resulting resource_uri and enforce strict validation before any network fetch: ensure the parsed URI's scheme is exactly "ui" and its host (or netloc) equals server_name (use the same checks in the other similar block around lines 592-607), and if validation fails raise an HTTPException (400 or 404 as appropriate) referencing resource_uri, path and server_name; perform these checks immediately after computing resource_uri and before any downstream fetch or logging.
395-419:⚠️ Potential issue | 🟠 MajorDo not default
server_nameto"unknown"when emittingui_resource.Line 417 introduces broken metadata and invalid
fetch_urlgeneration when server identity is missing. Require a validated server identity (from call payload/context), otherwise do not emitui_resource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 395 - 419, The code currently defaults server_name to "unknown" when building tool_result.ui_resource, producing invalid metadata and fetch_url; instead require a validated server identity and only emit UIResourceMetadata when server_name is present and trusted. In the block that looks up tool_def via tool_cache.get_tool_metadata(mcp_call_item.name) and builds UIResourceMetadata (resource_uri, server_name, fetch_url), add a check that server_name is non-empty and validated from the call payload/context (do not use the literal "unknown"), and skip setting tool_result.ui_resource if no valid server identity exists; preserve the existing resource_uri parsing (resource_uri.replace("ui://", "")) and only construct fetch_url when server_name passes validation.
635-637:⚠️ Potential issue | 🔴 CriticalSandbox policy currently weakens isolation.
Line 636 combines
allow-scriptswithallow-same-origin, which undermines iframe sandbox protections for untrusted UI content. Prefersandbox="allow-scripts"and require dedicated origin isolation if same-origin behavior is truly needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 635 - 637, The sandbox policy weakens isolation by combining allow-scripts with allow-same-origin (the sandbox attribute string `sandbox="allow-scripts allow-same-origin"`); change the recommendation to require `sandbox="allow-scripts"` for untrusted UI resources and only permit `allow-same-origin` when a documented, reviewed justification exists along with mandatory dedicated origin isolation (e.g., deploying the iframe content on a separate origin or using COOP/COEP headers) and explicit inter-frame communication patterns (e.g., postMessage). Update the sandbox attribute recommendation and the CSP/headers guidance to require the origin-isolation controls whenever same-origin behavior is allowed.
133-133:⚠️ Potential issue | 🟡 MinorFix typo in design text.
Line 133 still has “invesitage” → “investigate”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` at line 133, Fix the typo in the design text by replacing the misspelled word "invesigate" with "investigate" in the sentence that currently reads "Need to invesigate Llama stack's plan for MCP apps support" (update the text in mcp-apps.md so it reads "Need to investigate Llama stack's plan for MCP apps support").
329-349:⚠️ Potential issue | 🟠 MajorKey tool cache entries by
(server_name, tool_name)to prevent cross-server collisions.Line 329 and Line 348 still imply single-key lookup by
tool_name. In multi-server setups, identical tool names will return incorrect UI metadata. Make cache insert/lookup server-scoped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 329 - 349, The cache currently keyed by _tools: dict[str, dict[str, Any]] and accessed via get_tool_metadata(tool_name) will collide across servers; change the cache key to include server scope (use a tuple key like (server_name, tool_name) and update the typing to dict[tuple[str,str], dict[str, Any]]), update update_from_toolgroups to extract server_name from each toolgroup when inserting entries into self._tools using (server_name, tool_name), and modify get_tool_metadata to accept a server_name parameter and perform lookup using self._tools.get((server_name, tool_name)) so identical tool names on different servers do not conflict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md`:
- Around line 619-623: The browser snippet uses top-level ESM import and
top-level await but is missing type="module" and a resolvable specifier/import
map; update the snippet that imports App (import { App } from
'@modelcontextprotocol/ext-apps') to use a <script type="module"> block and
either a full ESM URL or document the required import map/bundler resolution,
and show creating and awaiting the App instance (new App(); await app.connect())
within that module script so the example runs in a plain browser environment.
- Around line 418-419: The fetch URL currently embeds raw resource_path into the
query string (see the fetch_url assignment using server_name and resource_path);
change it to URL-safe encode the resource identifier (e.g., base64url or
encodeURIComponent equivalent) when constructing fetch_url and ensure the server
handler for /v1/mcp-ui-resources/{server_name} decodes and validates the
incoming encoded path before use; update any client-side call sites that build
fetch_url and the server-side validation logic that consumes the decoded
identifier so canonicalization/authorization checks operate on the decoded,
validated path.
- Around line 457-478: Reconstructing resource_uri from path allows an attacker
to encode a different host; parse the resulting resource_uri and enforce strict
validation before any network fetch: ensure the parsed URI's scheme is exactly
"ui" and its host (or netloc) equals server_name (use the same checks in the
other similar block around lines 592-607), and if validation fails raise an
HTTPException (400 or 404 as appropriate) referencing resource_uri, path and
server_name; perform these checks immediately after computing resource_uri and
before any downstream fetch or logging.
- Around line 395-419: The code currently defaults server_name to "unknown" when
building tool_result.ui_resource, producing invalid metadata and fetch_url;
instead require a validated server identity and only emit UIResourceMetadata
when server_name is present and trusted. In the block that looks up tool_def via
tool_cache.get_tool_metadata(mcp_call_item.name) and builds UIResourceMetadata
(resource_uri, server_name, fetch_url), add a check that server_name is
non-empty and validated from the call payload/context (do not use the literal
"unknown"), and skip setting tool_result.ui_resource if no valid server identity
exists; preserve the existing resource_uri parsing
(resource_uri.replace("ui://", "")) and only construct fetch_url when
server_name passes validation.
- Around line 635-637: The sandbox policy weakens isolation by combining
allow-scripts with allow-same-origin (the sandbox attribute string
`sandbox="allow-scripts allow-same-origin"`); change the recommendation to
require `sandbox="allow-scripts"` for untrusted UI resources and only permit
`allow-same-origin` when a documented, reviewed justification exists along with
mandatory dedicated origin isolation (e.g., deploying the iframe content on a
separate origin or using COOP/COEP headers) and explicit inter-frame
communication patterns (e.g., postMessage). Update the sandbox attribute
recommendation and the CSP/headers guidance to require the origin-isolation
controls whenever same-origin behavior is allowed.
- Line 133: Fix the typo in the design text by replacing the misspelled word
"invesigate" with "investigate" in the sentence that currently reads "Need to
invesigate Llama stack's plan for MCP apps support" (update the text in
mcp-apps.md so it reads "Need to investigate Llama stack's plan for MCP apps
support").
- Around line 329-349: The cache currently keyed by _tools: dict[str, dict[str,
Any]] and accessed via get_tool_metadata(tool_name) will collide across servers;
change the cache key to include server scope (use a tuple key like (server_name,
tool_name) and update the typing to dict[tuple[str,str], dict[str, Any]]),
update update_from_toolgroups to extract server_name from each toolgroup when
inserting entries into self._tools using (server_name, tool_name), and modify
get_tool_metadata to accept a server_name parameter and perform lookup using
self._tools.get((server_name, tool_name)) so identical tool names on different
servers do not conflict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54af3d4d-aadc-4e0f-a698-2e08d6cd1a56
📒 Files selected for processing (1)
docs/design/mcp-apps-for-ui-components/mcp-apps.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
- GitHub Check: build-pr
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
🪛 LanguageTool
docs/design/mcp-apps-for-ui-components/mcp-apps.md
[style] ~103-~103: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ....5.2` - MCP Apps announced: January 26, 2026 - Issue: llama-stack 0.5.2 likely p...
(MISSING_COMMA_AFTER_YEAR)
[style] ~118-~118: The double modal “Requires sandboxed” is nonstandard (only accepted in certain dialects). Consider “to be sandboxed”.
Context: ...handle UI resource rendering - Requires sandboxed iframe implementation - Scope: This...
(NEEDS_FIXED)
[grammar] ~133-~133: Ensure spelling is correct
Context: ... abstraction layer Cons: - Need to invesitage Llama stack's plan for MCP apps support...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.0)
docs/design/mcp-apps-for-ui-components/mcp-apps.md
[warning] 35-35: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 49-49: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 123-123: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 137-137: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 156-156: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 161-161: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 203-203: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 274-274: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 275-275: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 391-391: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 538-538: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 561-561: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 578-578: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 602-602: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 602-602: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 610-610: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 667-667: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 709-709: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 728-728: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
This PR introduces design docs for introducing support for the MCP Apps extension into Lightspeed core, such that responses from tools can include interactive UI components, instead of a "text wall that summarizes complex data" Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
docs/design/mcp-apps-for-ui-components/mcp-apps.md (3)
147-149:⚠️ Potential issue | 🔴 CriticalUse encoded
resource_uriend-to-end and require strict URI/server binding checks.Current examples/prose still center on
path=...and server-name validation only. The design should explicitly require parsing the requested URI and enforcingscheme == "ui"andhost == server_namebefore fetch, with encoded transport for the URI value.Also applies to: 249-265, 277-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 147 - 149, Update the API design and examples to use an encoded resource_uri end-to-end instead of `path=...`, and add strict URI/server binding checks: when handling GET /v1/mcp-ui-resources/{server_name} (and anywhere documented in the ranges noted), parse the provided encoded resource_uri, require scheme == "ui" and host == server_name before calling llama-stack's /v1/resources/read, and reject requests that fail these checks; also update the ToolResultSummary.ui_resource docs to state it carries only metadata and must contain the encoded resource_uri with the same binding semantics.
273-275:⚠️ Potential issue | 🔴 CriticalSandbox policy still weakens isolation.
sandbox="allow-scripts allow-same-origin"for untrusted UI content materially reduces iframe isolation. Keepallow-same-originout by default and require origin isolation guidance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 273 - 275, The doc currently recommends using sandbox="allow-scripts allow-same-origin", which weakens iframe isolation; change the default recommendation to sandbox="allow-scripts" (omit allow-same-origin) and add a short note on when allow-same-origin is permitted: only for trusted apps and only with explicit origin isolation (COOP/COEP) or equivalent guidance, plus safe messaging patterns (postMessage with origin checks); also update the CSP remark to mention COOP/COEP and origin-isolation requirements when allow-same-origin is used.
131-133:⚠️ Potential issue | 🟡 MinorTrailing whitespace remains and will keep markdownlint noisy.
There are still trailing spaces in headings/section lines; please clean these to keep docs CI green.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 131 - 133, Remove trailing whitespace from the markdown headings so markdownlint stops flagging them; specifically edit the lines containing "## Design" and "### Extend Llama Stack with Resources API" to ensure there are no trailing spaces at the end of those lines (and scan nearby headings/section lines in docs/design/mcp-apps-for-ui-components/mcp-apps.md for any other trailing spaces and trim them).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md`:
- Around line 283-285: The phrase "Signed/verified by MCP server" is
underspecified; either (A) expand the design to define a concrete signature
verification flow—specify signature format (e.g. JSON Web Signature or CMS), key
types/rotation policy, trust roots/CA or pinned public keys, how the MCP server
signs UI resources, verification steps on the client, error/failure behavior and
logging, and threat assumptions for the "UI resources are signed/verified by MCP
server" bullet; or (B) explicitly downgrade that bullet to a "future
consideration" and remove any present-tense implications of an implemented
control. Update the same list entry "No user-provided HTML content" if its
security relies on the signing claim so readers aren’t misled.
- Around line 59-66: The fenced code blocks containing plain text lists (e.g.,
the block starting with "1. LLM selects tool with _meta.ui.resourceUri" and the
other unlabeled fenced blocks in the document) must include an explicit language
tag (for example `text`) to satisfy MD040 and improve readability; update each
unlabeled triple-backtick fence to use ```text (including the other unlabeled
blocks mentioned in the review) so the renderer/linter recognizes them as plain
text.
- Around line 146-147: The upgrade list is missing the llama-stack-api
dependency; update the bullet that currently mentions upgrading `llama-stack`
and `llama-stack-client` to also include `llama-stack-api` so it matches the
dependency referenced earlier (`llama-stack-api==0.5.2`), ensuring the line that
lists "Upgrade `llama-stack` and `llama-stack-client`" becomes "Upgrade
`llama-stack`, `llama-stack-client`, and `llama-stack-api`".
---
Duplicate comments:
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md`:
- Around line 147-149: Update the API design and examples to use an encoded
resource_uri end-to-end instead of `path=...`, and add strict URI/server binding
checks: when handling GET /v1/mcp-ui-resources/{server_name} (and anywhere
documented in the ranges noted), parse the provided encoded resource_uri,
require scheme == "ui" and host == server_name before calling llama-stack's
/v1/resources/read, and reject requests that fail these checks; also update the
ToolResultSummary.ui_resource docs to state it carries only metadata and must
contain the encoded resource_uri with the same binding semantics.
- Around line 273-275: The doc currently recommends using sandbox="allow-scripts
allow-same-origin", which weakens iframe isolation; change the default
recommendation to sandbox="allow-scripts" (omit allow-same-origin) and add a
short note on when allow-same-origin is permitted: only for trusted apps and
only with explicit origin isolation (COOP/COEP) or equivalent guidance, plus
safe messaging patterns (postMessage with origin checks); also update the CSP
remark to mention COOP/COEP and origin-isolation requirements when
allow-same-origin is used.
- Around line 131-133: Remove trailing whitespace from the markdown headings so
markdownlint stops flagging them; specifically edit the lines containing "##
Design" and "### Extend Llama Stack with Resources API" to ensure there are no
trailing spaces at the end of those lines (and scan nearby headings/section
lines in docs/design/mcp-apps-for-ui-components/mcp-apps.md for any other
trailing spaces and trim them).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5921ebce-2967-493d-82ea-456aadb3a8e9
📒 Files selected for processing (1)
docs/design/mcp-apps-for-ui-components/mcp-apps.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🧰 Additional context used
🪛 LanguageTool
docs/design/mcp-apps-for-ui-components/mcp-apps.md
[style] ~113-~113: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ....5.2` - MCP Apps announced: January 26, 2026 - Issue: llama-stack 0.5.2 likely p...
(MISSING_COMMA_AFTER_YEAR)
[style] ~128-~128: The double modal “Requires sandboxed” is nonstandard (only accepted in certain dialects). Consider “to be sandboxed”.
Context: ...handle UI resource rendering - Requires sandboxed iframe implementation - Scope: This...
(NEEDS_FIXED)
🪛 markdownlint-cli2 (0.22.0)
docs/design/mcp-apps-for-ui-components/mcp-apps.md
[warning] 45-45: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 59-59: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 131-131: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 133-133: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 153-153: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 153-153: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 212-212: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
[warning] 231-231: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/1/1
(MD029, ol-prefix)
[warning] 260-260: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 305-305: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 347-347: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 366-366: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
| ``` | ||
| 1. LLM selects tool with _meta.ui.resourceUri | ||
| 2. Host fetches UI resource from MCP server (ui:// → HTML) | ||
| 3. Host renders HTML in sandboxed iframe | ||
| 4. UI initializes with ui/initialize handshake | ||
| 5. Tool result pushed to UI via postMessage | ||
| 6. UI can call tools back via tools/call requests | ||
| ``` |
There was a problem hiding this comment.
Several fenced code blocks lack explicit language tags.
Add language identifiers (for example text) to unlabeled fenced blocks to satisfy MD040 and improve readability.
Also applies to: 81-107, 153-178, 260-268
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 59-59: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 59 - 66, The
fenced code blocks containing plain text lists (e.g., the block starting with
"1. LLM selects tool with _meta.ui.resourceUri" and the other unlabeled fenced
blocks in the document) must include an explicit language tag (for example
`text`) to satisfy MD040 and improve readability; update each unlabeled
triple-backtick fence to use ```text (including the other unlabeled blocks
mentioned in the review) so the renderer/linter recognizes them as plain text.
| - Upgrade `llama-stack` and `llama-stack-client` dependencies | ||
| - Add UI resource proxy endpoint: `GET /v1/mcp-ui-resources/{server_name}?path=...` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which llama stack package names are actually used in the repo
rg -n -C2 --iglob '*requirements*.txt' --iglob '*pyproject*.toml' --iglob '*poetry.lock' --iglob '*Pipfile*' 'llama-stack(-api|-client)?'Repository: lightspeed-core/lightspeed-stack
Length of output: 3314
🏁 Script executed:
cat -n docs/design/mcp-apps-for-ui-components/mcp-apps.md | sed -n '110,120p; 144,150p'Repository: lightspeed-core/lightspeed-stack
Length of output: 1148
Line 146 should also list llama-stack-api for consistency with the current versions shown in line 112.
Line 112 identifies llama-stack-api==0.5.2 as a current dependency, but the upgrade action in line 146 only mentions upgrading llama-stack and llama-stack-client. Include llama-stack-api in the dependency upgrade list to match what's already documented as in use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 146 - 147,
The upgrade list is missing the llama-stack-api dependency; update the bullet
that currently mentions upgrading `llama-stack` and `llama-stack-client` to also
include `llama-stack-api` so it matches the dependency referenced earlier
(`llama-stack-api==0.5.2`), ensuring the line that lists "Upgrade `llama-stack`
and `llama-stack-client`" becomes "Upgrade `llama-stack`, `llama-stack-client`,
and `llama-stack-api`".
| - MCP servers are trusted (pre-registered) | ||
| - UI resources are signed/verified by MCP server | ||
| - No user-provided HTML content |
There was a problem hiding this comment.
“Signed/verified by MCP server” is underspecified and can create false security guarantees.
This statement reads as an implemented control, but no verification mechanism/trust model is defined in the design. Either define concrete signature verification flow (format, keys, trust roots, failure behavior) or downgrade this to a future consideration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/mcp-apps-for-ui-components/mcp-apps.md` around lines 283 - 285,
The phrase "Signed/verified by MCP server" is underspecified; either (A) expand
the design to define a concrete signature verification flow—specify signature
format (e.g. JSON Web Signature or CMS), key types/rotation policy, trust
roots/CA or pinned public keys, how the MCP server signs UI resources,
verification steps on the client, error/failure behavior and logging, and threat
assumptions for the "UI resources are signed/verified by MCP server" bullet; or
(B) explicitly downgrade that bullet to a "future consideration" and remove any
present-tense implications of an implemented control. Update the same list entry
"No user-provided HTML content" if its security relies on the signing claim so
readers aren’t misled.
2 participants
Description
This PR introduces design docs for introducing support for the MCP Apps extension into Lightspeed core, such that responses from tools can include interactive UI components, instead of a "text wall that summarizes complex data"
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit