-
Notifications
You must be signed in to change notification settings - Fork 6k
Remove MCP sampling support #10087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove MCP sampling support #10087
Changes from 8 commits
0980434
d4eeca4
281b261
f9efb8e
6dac769
3ac29ed
3ca769a
7347b8f
2d7776f
2f9555c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,8 +51,6 @@ import { | |
| McpAppToolInputPartial, | ||
| DimensionLayout, | ||
| OnDisplayModeChange, | ||
| SamplingCreateMessageParams, | ||
| SamplingCreateMessageResponse, | ||
| } from './types'; | ||
| import { | ||
| useDisplayMode, | ||
|
|
@@ -676,13 +674,6 @@ export default function McpAppRenderer({ | |
|
|
||
| const [containerWidth, setContainerWidth] = useState<number>(0); | ||
| const [containerHeight, setContainerHeight] = useState<number>(0); | ||
| const [apiHost, setApiHost] = useState<string | null>(null); | ||
| const [secretKey, setSecretKey] = useState<string | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| window.electron.getGoosedHostPort().then(setApiHost); | ||
| window.electron.getSecretKey().then(setSecretKey); | ||
| }, []); | ||
|
|
||
| // Fetch the resource from the extension to get HTML and metadata (CSP, permissions, etc.). | ||
| // If cachedHtml is provided we show it immediately; the fetch updates metadata and | ||
|
|
@@ -933,38 +924,12 @@ export default function McpAppRenderer({ | |
|
|
||
| const handleFallbackRequest = useCallback( | ||
| async (request: JSONRPCRequest, _extra: RequestHandlerExtra) => { | ||
| if (request.method === 'sampling/createMessage') { | ||
| if (!sessionId || !apiHost || !secretKey) { | ||
| throw new Error('Session not initialized for sampling request'); | ||
| } | ||
| const { messages, systemPrompt, maxTokens } = | ||
| request.params as unknown as SamplingCreateMessageParams; | ||
| 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 SamplingCreateMessageResponse; | ||
| } | ||
| return { | ||
| status: 'error' as const, | ||
| message: `Unhandled JSON-RPC method: ${request.method ?? '<unknown>'}`, | ||
| }; | ||
|
Comment on lines
927
to
930
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence in this revision is that this fallback now always reports Useful? React with 👍 / 👎.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MCP sampling remains supported for MCP servers, but this PR intentionally does not support sampling from MCP app iframes. The app bridge does not advertise sampling in its host capabilities, and the retired built-in Chat app is now blocked because it depended on that unsupported iframe path. We are not removing global MCP sampling, so .enable_sampling() should remain. |
||
| }, | ||
| [sessionId, apiHost, secretKey] | ||
| [] | ||
| ); | ||
|
|
||
| const handleError = useCallback((err: Error) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In desktop sessions, MCP servers still see Goose advertise sampling because
GooseClient::get_infostill calls.enable_sampling()incrates/goose/src/agents/mcp_client.rs, but MCP app iframes now get an unhandled-method result forsampling/createMessage. For any MCP server that serves a UI app after seeing the sampling capability and has that app request sampling from the host, the app will fail at runtime even though the server was told the capability is available; remove the advertised capability/handler in the MCP client as well, or keep this bridge until both paths are removed.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since MCP sampling is still supported globally, removing the app iframe bridge creates an inconsistent capability contract. I’ll keep the bridge in this PR and limit the retirement behavior to the built-in Chat app