Skip to content

feat(apps): add support for MCP apps to sample#7039

Open
alexhancock wants to merge 1 commit intomainfrom
alexhancock/mcp-app-sampling
Open

feat(apps): add support for MCP apps to sample#7039
alexhancock wants to merge 1 commit intomainfrom
alexhancock/mcp-app-sampling

Conversation

@alexhancock
Copy link
Collaborator

@alexhancock alexhancock commented Feb 6, 2026

Summary

  • Sparked by conversations with @DOsinga and @liady
  • Adds support from sampling from inside MCP Apps
  • Uses the existing postmessage bridge, but sends new sampling/createMessage reqs
  • Makes a new route in goosed for handling
  • Allows for text or images
  • Adds an example app with a simple chat interface as a demonstration

Demo

Screenshot 2026-02-06 at 2 06 20 PM

Request Flow

┌─────────────────────────────────────────────────────────────────────┐
│                         Desktop (Electron)                          │
│  ┌──────────────┐      ┌──────────────────┐      ┌──────────────┐  │
│  │   MCP App    │      │  McpAppRenderer  │      │  goose-server│  │
│  │   (iframe)   │      │   (React host)   │      │   (goosed)   │  │
│  └──────┬───────┘      └────────┬─────────┘      └──────┬───────┘  │
│         │                       │                       │          │
│         │ postMessage           │                       │          │
│         │ sampling/createMessage│                       │          │
│         │──────────────────────>│                       │          │
│         │                       │                       │          │
│         │                       │  POST /sessions/      │          │
│         │                       │  {id}/sampling/message│          │
│         │                       │──────────────────────>│          │
│         │                       │                       │          │
│         │                       │                       │ provider │
│         │                       │                       │.complete │
│         │                       │                       │──────┐   │
│         │                       │                       │      │   │
│         │                       │                       │<─────┘   │
│         │                       │                       │          │
│         │                       │    CreateMessageResp  │          │
│         │                       │<──────────────────────│          │
│         │                       │                       │          │
│         │  response (model,     │                       │          │
│         │  role, content)       │                       │          │
│         │<──────────────────────│                       │          │
│         │                       │                       │          │
└─────────┴───────────────────────┴───────────────────────┴──────────┘

@alexhancock alexhancock marked this pull request as ready for review February 6, 2026 13:25
Copilot AI review requested due to automatic review settings February 6, 2026 13:25
@alexhancock
Copy link
Collaborator Author

cc @liady

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds MCP “sampling” support so sandboxed MCP Apps can ask the running goosed session/provider to generate model responses, and includes a simple built-in chat MCP App as a demo.

Changes:

  • Adds a new MCP method (sampling/createMessage) to the desktop MCP Apps bridge and types.
  • Adds a new goose-server route (POST /sessions/{id}/sampling/message) to run provider completions for sampling.
  • Adds a built-in chat MCP App and wires it into the default apps cache.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
ui/desktop/src/components/McpApps/types.ts Introduces sampling request/response types for MCP Apps.
ui/desktop/src/components/McpApps/McpAppRenderer.tsx Forwards sampling/createMessage from the iframe to goosed via authenticated HTTP.
crates/goose/src/goose_apps/chat.html New built-in demo MCP App that uses sampling to implement a simple chat UI.
crates/goose/src/goose_apps/cache.rs Adds the new chat app to the default apps prepopulation logic.
crates/goose-server/src/routes/sampling.rs New backend endpoint that converts MCP sampling messages to goose messages and calls the provider.
crates/goose-server/src/routes/mod.rs Registers the new sampling routes with the server.

Comment on lines +124 to +129
window.addEventListener('message', (event) => {
const data = event.data;
if (!data || typeof data !== 'object') return;

if ('id' in data && pendingRequests.has(data.id)) {
const { resolve, reject } = pendingRequests.get(data.id);
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message event handler accepts responses from any source/origin; add a check (at least event.source === window.parent) to prevent other windows/iframes from spoofing JSON-RPC responses into the app.

Copilot uses AI. Check for mistakes.
Comment on lines +162 to +163
const responseText = response.content.text;
conversationHistory.push({ role: 'assistant', content: { type: 'text', text: responseText } });
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sampling/createMessage returns an MCP CreateMessageResult (with a nested message field), but this code reads response.content.text; update the sample to use response.message.content.text (and adjust how you append to conversationHistory) so it matches the server response.

Suggested change
const responseText = response.content.text;
conversationHistory.push({ role: 'assistant', content: { type: 'text', text: responseText } });
const responseText = response.message.content.text;
conversationHistory.push(response.message);

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +20
Router::new()
.route(
"/sessions/{session_id}/sampling/message",
post(create_message),
)
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This endpoint is intended to support image sampling, but there’s no explicit body-size limit layer here (unlike /reply and dictation), so base64 image requests may be rejected by the default request body limit; consider adding a DefaultBodyLimit::max(...) appropriate for image payloads.

Copilot uses AI. Check for mistakes.
Comment on lines +53 to +57
let (response, usage) = provider
.complete(&session_id, system, &messages, &[])
.await
.map_err(|e| {
tracing::error!("Sampling completion failed: {}", e);
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request accepts maxTokens but it’s currently ignored (the provider call uses the default model config), so clients won’t be able to constrain output length as requested; either plumb request.max_tokens into an overridden ModelConfig via complete_with_model(...) or remove the parameter from the client contract.

Copilot uses AI. Check for mistakes.
Comment on lines 46 to 48
stopReason: string;
role: 'assistant';
content: { type: 'text'; text: string };
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sampling/createMessage response type here doesn’t match the backend/MCP CreateMessageResult shape (it returns a message: { role, content } object and stopReason may be optional), so consumers like the sample app will read the wrong fields.

Suggested change
stopReason: string;
role: 'assistant';
content: { type: 'text'; text: string };
stopReason?: string;
message: {
role: 'assistant';
content: { type: 'text'; text: string };
};

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +30
for (uri, html) in [("apps://clock", CLOCK_HTML), ("apps://chat", CHAT_HTML)] {
if self.get_app(APPS_EXTENSION_NAME, uri).is_none() {
if let Ok(mut app) = GooseApp::from_html(html) {
app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()];
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default-app cache check uses apps://... URIs, but GooseApp::from_html() sets resource.uri to ui://apps/{name} and store_app() keys the cache by app.resource.uri, so this get_app() call will never hit and will rewrite the default apps on every startup.

Suggested change
for (uri, html) in [("apps://clock", CLOCK_HTML), ("apps://chat", CHAT_HTML)] {
if self.get_app(APPS_EXTENSION_NAME, uri).is_none() {
if let Ok(mut app) = GooseApp::from_html(html) {
app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()];
for html in [CLOCK_HTML, CHAT_HTML] {
if let Ok(mut app) = GooseApp::from_html(html) {
app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()];
if self
.get_app(APPS_EXTENSION_NAME, &app.resource.uri)
.is_none()
{

Copilot uses AI. Check for mistakes.
Comment on lines 242 to 267
case 'sampling/createMessage': {
if (!sessionId || !apiHost || !secretKey) {
throw new Error('Session not initialized for sampling request');
}
const { messages, systemPrompt, maxTokens } =
params as McpMethodParams['sampling/createMessage'];
const response = await fetch(`${apiHost}/sessions/${sessionId}/sampling/message`, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'X-Secret-Key': secretKey,
},
body: JSON.stringify({
messages: messages.map((m) => ({
role: m.role,
content: m.content,
})),
systemPrompt,
maxTokens,
}),
});
if (!response.ok) {
throw new Error(`Sampling request failed: ${response.statusText}`);
}
return (await response.json()) as McpMethodResponse['sampling/createMessage'];
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fantastic!

FYI, @alexhancock I'm working to integrate <AppRenderer/> from @mcp-ui/client over here. It has dedicated props for callbacks that map to a subset of JSON RPC messages supported by the MCP Apps spec. Don't think sampling/createMessage is supported on that side of things yet. Maybe I'm wrong?

@liady, how can we support sampling/createMessage while using <AppRenderer />?

++ @idosal, @ochafik, @infoxicator for input as well

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed with @idosal that we might be able to have a generic onRequest prop added <AppRender />, as a catch-all that can allow goose to listen for unsupported/non-standard JSON RPC methods. Not to say that sampling/createMessage is nonstandard, just not supported by the client SDK...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can wait for #7013 to go in and then adjust however needed. Feels like it would only be minor changes to use a better set of types etc.

Will stay tuned to this thread

@alexhancock alexhancock force-pushed the alexhancock/mcp-app-sampling branch from 6148d1b to 6fe5277 Compare February 6, 2026 17:39
@infoxicator
Copy link

Awesome stuff @alexhancock! looks like you did manage to spin something cool on the plane ride back!

Copilot AI review requested due to automatic review settings February 11, 2026 16:37
@alexhancock alexhancock force-pushed the alexhancock/mcp-app-sampling branch from 6fe5277 to 89732b1 Compare February 11, 2026 16:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

body: JSON.stringify({
messages: messages.map((m) => ({
role: m.role,
content: m.content,
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This request forwards content: m.content as-is, but MCP sampling messages typically use content as an array of blocks; if m.content isn’t serialized as an array, CreateMessageRequestParams deserialization will fail on the server.

Suggested change
content: m.content,
content: Array.isArray(m.content)
? m.content
: [
{
type: 'text',
text: String(m.content ?? ''),
},
],

Copilot uses AI. Check for mistakes.
if (!response.ok) {
throw new Error(`Sampling request failed: ${response.statusText}`);
}
return (await response.json()) as McpMethodResponse['sampling/createMessage'];
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting await response.json() directly to McpMethodResponse['sampling/createMessage'] is unsafe here because the backend returns an MCP CreateMessageResult (nested message), so callers will read the wrong fields unless you validate/transform the response.

Suggested change
return (await response.json()) as McpMethodResponse['sampling/createMessage'];
const raw = (await response.json()) as unknown;
if (
!raw ||
typeof raw !== 'object' ||
!('message' in raw) ||
raw.message == null
) {
throw new Error('Invalid sampling response format: missing message');
}
const { message } = raw as { message: McpMethodResponse['sampling/createMessage'] };
return message;

Copilot uses AI. Check for mistakes.
Comment on lines +118 to +121
const id = ++requestId;
pendingRequests.set(id, { resolve, reject });
window.parent.postMessage({ jsonrpc: '2.0', id, method, params }, '*');
});
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using '*' as the targetOrigin for postMessage allows the JSON-RPC request to be sent to any embedding origin; use the known proxy origin (or at least window.location.origin) as the targetOrigin.

Copilot uses AI. Check for mistakes.
Comment on lines +150 to +152
addMessage('user', text);
conversationHistory.push({ role: 'user', content: { type: 'text', text } });

Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conversationHistory pushes sampling messages with content as an object, but MCP sampling expects content to be an array of blocks, so the request shape won’t match what the host/server expects.

Copilot uses AI. Check for mistakes.
Role::User => Message::user(),
Role::Assistant => Message::assistant(),
};
content_to_message(base, &msg.content)
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this repo, rmcp sampling message content is handled as a list of blocks (msg.content.first() in crates/goose/src/agents/mcp_client.rs), but this route treats msg.content as a single Content; align the conversion with mcp_client.rs (iterate/take first block).

Suggested change
content_to_message(base, &msg.content)
if let Some(first_content) = msg.content.first() {
content_to_message(base, first_content)
} else {
base
}

Copilot uses AI. Check for mistakes.
Comment on lines 73 to 78
fn content_to_message(base: Message, content: &Content) -> Message {
match &content.raw {
RawContent::Text(text) => base.with_text(&text.text),
RawContent::Image(image) => base.with_image(&image.data, &image.mime_type),
_ => base,
}
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

content_to_message assumes a single Content (content.raw), but sampling content is block-based in rmcp; update this helper to accept the sampling content block list and convert supported blocks (text/image) consistently.

Suggested change
fn content_to_message(base: Message, content: &Content) -> Message {
match &content.raw {
RawContent::Text(text) => base.with_text(&text.text),
RawContent::Image(image) => base.with_image(&image.data, &image.mime_type),
_ => base,
}
fn content_to_message(mut base: Message, contents: &[Content]) -> Message {
for content in contents {
match &content.raw {
RawContent::Text(text) => {
base = base.with_text(&text.text);
}
RawContent::Image(image) => {
base = base.with_image(&image.data, &image.mime_type);
}
_ => {
// Ignore unsupported content types, preserving existing behavior.
}
}
}
base

Copilot uses AI. Check for mistakes.
Comment on lines 63 to 67
Ok(Json(CreateMessageResult {
model: usage.model,
stop_reason: Some(CreateMessageResult::STOP_REASON_END_TURN.to_string()),
message: SamplingMessage {
role: Role::Assistant,
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response construction uses SamplingMessage { content: Content::text(...) }, but goose’s existing MCP sampling implementation builds responses with SamplingMessage::new(..., SamplingMessageContent::text/ Image ...); using the wrong content type/shape here will break clients expecting MCP CreateMessageResult semantics.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +20
Router::new()
.route(
"/sessions/{session_id}/sampling/message",
post(create_message),
)
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an integration test for /sessions/{session_id}/sampling/message (similar to other route tests) to lock in the MCP request/response shape and prevent regressions.

Copilot uses AI. Check for mistakes.

export type SamplingMessage = {
role: 'user' | 'assistant';
content: { type: 'text'; text: string } | { type: 'image'; data: string; mimeType: string };
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SamplingMessage.content should be an array of content blocks (the rmcp sampling types are used as a list in crates/goose/src/agents/mcp_client.rs via msg.content.first()), so modeling it as a single block will break interop/deserialization.

Suggested change
content: { type: 'text'; text: string } | { type: 'image'; data: string; mimeType: string };
content: ContentBlock[];

Copilot uses AI. Check for mistakes.
@DOsinga
Copy link
Collaborator

DOsinga commented Feb 12, 2026

interesting - I like this of course, but i think we can do this simpler by just adding a tool to the apps platform extension, call_llm (or sample but I always found that confusing). we already have the plumbing in mcp apps for an app to call the mcp server that created it, so that should then all work. all we need to do is then add this to the prompt we use to generate or update the apps. let me know if you think this makes sense or we can chat about this?

@alexhancock
Copy link
Collaborator Author

interesting - I like this of course, but i think we can do this simpler by just adding a tool to the apps platform extension, call_llm (or sample but I always found that confusing). we already have the plumbing in mcp apps for an app to call the mcp server that created it, so that should then all work. all we need to do is then add this to the prompt we use to generate or update the apps. let me know if you think this makes sense or we can chat about this?

It does makes sense in the context of goose and the presence of that server, but I was trying to think of a way to do it that could work when the app is run in hosts other than goose as well. Would still need plumbing added in other hosts of course, but an app sending a sampling/createMessage across the postmessage bridge was my attempt at this. I am still familiarizing myself with apps. Does this make sense?

@DOsinga
Copy link
Collaborator

DOsinga commented Feb 13, 2026

yeah, but it will only work in other providers if this becomes part of the standard. we should push for that! but until that happens, what do we do? /cc @aharvard ?

@DOsinga
Copy link
Collaborator

DOsinga commented Feb 13, 2026

@idosal do you think adding sampling to the protocol like this would make sense?

aharvard added a commit that referenced this pull request Feb 13, 2026
Wire up the onFallbackRequest callback on AppRenderer to handle
unrecognized MCP JSON-RPC requests. This is a stub that logs the
request method and returns success, with a todo to implement
sampling/createMessage per #7039.

- Import RequestHandlerExtra from @mcp-ui/client
- Import JSONRPCRequest from @modelcontextprotocol/sdk/types.js
- Add handleFallbackRequest callback with correct types
- Pass handler to AppRenderer's onFallbackRequest prop
@aharvard aharvard mentioned this pull request Feb 13, 2026
aharvard added a commit that referenced this pull request Feb 13, 2026
Add a fallback request handler to gracefully handle unrecognized MCP
protocol requests from guest apps (e.g. sampling/createMessage).

Changes:
- Import RequestHandlerExtra and JSONRPCRequest types
- Add handleFallbackRequest callback that logs and returns success
- Wire onFallbackRequest prop to AppRenderer

Ref: #7039
aharvard added a commit that referenced this pull request Feb 13, 2026
Add a fallback request handler to McpAppRenderer that catches unhandled
MCP requests from the app iframe. This is a stub that logs the request
method and returns success, with a todo to implement sampling/createMessage
per #7039.

Changes:
- Import RequestHandlerExtra from @mcp-ui/client
- Import JSONRPCRequest from @modelcontextprotocol/sdk/types.js
- Add handleFallbackRequest useCallback
- Wire onFallbackRequest prop to AppRenderer
aharvard added a commit that referenced this pull request Feb 13, 2026
Upgrade @mcp-ui/client from ^6.0.0 to ^6.1.0 to get the new
onFallbackRequest prop on AppRenderer.

Add a fallback request handler that catches unhandled MCP requests from
the app iframe. This is a stub that logs the request method and returns
success, with a todo to implement sampling/createMessage per
#7039.

Changes:
- Upgrade @mcp-ui/client to ^6.1.0
- Import RequestHandlerExtra from @mcp-ui/client
- Import JSONRPCRequest from @modelcontextprotocol/sdk/types.js
- Add handleFallbackRequest async callback
- Wire onFallbackRequest prop to AppRenderer
@aharvard
Copy link
Collaborator

aharvard commented Feb 13, 2026

yeah, but it will only work in other providers if this becomes part of the standard. we should push for that! but until that happens, what do we do? /cc @aharvard ?

As to what we do, we can now use the new onFallbackRequest prop, that just landed (ty! @idosal) in @mcp-ui/clients v1.6.1. It's designed for hosts to experiment with implementing JSON RPC message-related content that might be promoted as a standard message for MCP Apps.

@alexhancock I upgraded to v1.6.1 and stubbed the handler here: #7208

Handling sampling/createMessage also feels like it could be easily promoted to become a standard for MCP Apps soon. For us that would show up as a prop on the <AppRenderer />, something like onCreateSamplingMessage, so long as the broader MCP standard keeps it in place. Has there been talk of deprecation?

Copilot AI review requested due to automatic review settings February 13, 2026 16:28
@alexhancock alexhancock force-pushed the alexhancock/mcp-app-sampling branch from 648ed51 to e9f1981 Compare February 13, 2026 16:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comment on lines +46 to +47
role: 'assistant';
content: { type: 'text'; text: string };
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response type definition doesn't match the actual API response structure. The backend returns CreateMessageResult which has a nested message field containing the role and content. The correct structure should be:

{
  model: string;
  stopReason: string;
  message: {
    role: 'assistant';
    content: { type: 'text'; text: string };
  };
}

Without this fix, accessing the response in consuming code will fail.

Suggested change
role: 'assistant';
content: { type: 'text'; text: string };
message: {
role: 'assistant';
content: { type: 'text'; text: string };
};

Copilot uses AI. Check for mistakes.
Comment on lines +25 to +60
async fn create_message(
State(state): State<Arc<AppState>>,
Path(session_id): Path<String>,
Json(request): Json<CreateMessageRequestParams>,
) -> Result<Json<CreateMessageResult>, StatusCode> {
let agent = state.get_agent_for_route(session_id.clone()).await?;

let provider = agent.provider().await.map_err(|e| {
tracing::error!("Failed to get provider: {}", e);
StatusCode::INTERNAL_SERVER_ERROR
})?;

let messages: Vec<Message> = request
.messages
.iter()
.map(|msg| {
let base = match msg.role {
Role::User => Message::user(),
Role::Assistant => Message::assistant(),
};
content_to_message(base, &msg.content)
})
.collect();

let system = request
.system_prompt
.as_deref()
.unwrap_or("You are a helpful AI assistant.");

let (response, usage) = provider
.complete(&session_id, system, &messages, &[])
.await
.map_err(|e| {
tracing::error!("Sampling completion failed: {}", e);
StatusCode::INTERNAL_SERVER_ERROR
})?;
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maxTokens parameter is accepted but never used in the sampling implementation. It's not passed to provider.complete(), which means token limits will fall back to provider defaults. Either remove this parameter from the API or pass it through to the provider's complete method.

Copilot uses AI. Check for mistakes.
@alexhancock alexhancock force-pushed the alexhancock/mcp-app-sampling branch from e9f1981 to 2cc164e Compare February 13, 2026 17:30
Copilot AI review requested due to automatic review settings February 13, 2026 20:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment on lines 379 to 380
});
}
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the API response doesn't match either expected format (neither response.content?.type === 'text' nor response.message?.content), no assistant message is added but the UI acts as if the request succeeded. Consider adding an else clause to show an error message when the response format is unexpected, so users understand why they didn't receive a response.

Suggested change
});
}
});
} else {
this.showError('Unexpected response content type');
}
} else {
this.showError('Unexpected response format from server');

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +60
let (response, usage) = provider
.complete(&session_id, system, &messages, &[])
.await
.map_err(|e| {
tracing::error!("Sampling completion failed: {}", e);
StatusCode::INTERNAL_SERVER_ERROR
})?;
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The max_tokens parameter from the sampling request is received but never used. The CreateMessageRequestParams includes this field, but it's not passed to the provider's complete method. This means MCP apps cannot control the maximum length of responses. Consider passing request.max_tokens to the provider if the API supports it, or document that this parameter is currently ignored.

Copilot uses AI. Check for mistakes.
Comment on lines 410 to 420
let meta = content.csp.map(|csp| ResourceMetadata {
ui: Some(UiMetadata {
csp: Some(CspMetadata {
connect_domains: Some(csp.connect_domains),
resource_domains: Some(csp.resource_domains),
frame_domains: None,
base_uri_domains: None,
}),
..Default::default()
}),
});
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When CSP is provided during app creation, the code unconditionally replaces all metadata. This would lose any existing UI metadata fields like permissions, domain, or prefers_border if they were set. Consider merging the CSP into existing metadata rather than replacing it entirely, or document that CSP replacement is intentional behavior.

Copilot uses AI. Check for mistakes.
Comment on lines 478 to 488
app.resource.meta = content.csp.map(|csp| ResourceMetadata {
ui: Some(UiMetadata {
csp: Some(CspMetadata {
connect_domains: Some(csp.connect_domains),
resource_domains: Some(csp.resource_domains),
frame_domains: None,
base_uri_domains: None,
}),
..Default::default()
}),
});
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same metadata replacement issue as in create_app_content. When CSP is provided during app update, all existing metadata is replaced, potentially losing permissions, domain, and prefers_border settings. This should merge CSP into existing metadata structure.

Suggested change
app.resource.meta = content.csp.map(|csp| ResourceMetadata {
ui: Some(UiMetadata {
csp: Some(CspMetadata {
connect_domains: Some(csp.connect_domains),
resource_domains: Some(csp.resource_domains),
frame_domains: None,
base_uri_domains: None,
}),
..Default::default()
}),
});
if let Some(new_csp) = content.csp {
// Merge new CSP into existing metadata instead of replacing it.
let mut meta = app.resource.meta.take().unwrap_or_default();
let mut ui = meta.ui.take().unwrap_or_default();
let mut csp_meta = ui.csp.take().unwrap_or_default();
csp_meta.connect_domains = Some(new_csp.connect_domains);
csp_meta.resource_domains = Some(new_csp.resource_domains);
// Preserve other CSP fields like frame_domains and base_uri_domains.
ui.csp = Some(csp_meta);
meta.ui = Some(ui);
app.resource.meta = Some(meta);
}

Copilot uses AI. Check for mistakes.
@alexhancock alexhancock force-pushed the alexhancock/mcp-app-sampling branch 2 times, most recently from 6c95970 to b857bf8 Compare February 13, 2026 20:44
Co-authored-by: Andrew Harvard <aharvard@block.xyz>
Copilot AI review requested due to automatic review settings February 13, 2026 21:05
@alexhancock alexhancock force-pushed the alexhancock/mcp-app-sampling branch from b857bf8 to fdb6520 Compare February 13, 2026 21:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comment on lines +501 to +502
systemPrompt,
maxTokens,
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request body sends fields in camelCase (systemPrompt, maxTokens), but CreateMessageRequestParams from rmcp likely expects snake_case (system_prompt, max_tokens) following Rust conventions. This mismatch may cause deserialization to fail or the fields to be silently ignored. Verify the expected format from the rmcp library and ensure the frontend matches it.

Suggested change
systemPrompt,
maxTokens,
system_prompt: systemPrompt,
max_tokens: maxTokens,

Copilot uses AI. Check for mistakes.
Comment on lines +162 to +165
const responseText = response.content.text;
conversationHistory.push({ role: 'assistant', content: { type: 'text', text: responseText } });

loadingEl.textContent = responseText;
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response structure doesn't match what the frontend expects. The CreateMessageResult from rmcp returns { model, stop_reason, message: { role, content } }, but the code tries to access response.content.text. This should be response.message.content.text or similar, depending on how SamplingMessage serializes its content field (it may be an array or object).

Suggested change
const responseText = response.content.text;
conversationHistory.push({ role: 'assistant', content: { type: 'text', text: responseText } });
loadingEl.textContent = responseText;
const message = response && response.message ? response.message : response;
let responseText = '';
if (message && Array.isArray(message.content)) {
const textPart = message.content.find(
(part) => part && typeof part === 'object' && typeof part.text === 'string'
);
if (textPart) {
responseText = textPart.text;
}
} else if (message && message.content && typeof message.content === 'object') {
if (typeof message.content.text === 'string') {
responseText = message.content.text;
}
}
conversationHistory.push({ role: 'assistant', content: { type: 'text', text: responseText } });
loadingEl.textContent = responseText || '[no response text]';

Copilot uses AI. Check for mistakes.
};
}
msg
}
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sampling route lacks test coverage. Other similar routes in the codebase (e.g., action_required.rs at lines 63-100, reply.rs at lines 468-506) have integration tests. Consider adding tests to verify the sampling endpoint works correctly, especially given the data transformation between MCP protocol types and the response format expected by the frontend.

Suggested change
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn content_to_message_with_empty_multiple_does_not_panic() {
let base = Message::user();
let content: SamplingContent<SamplingMessageContent> = SamplingContent::Multiple(vec![]);
let _result = content_to_message(base, &content);
}
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants