Remove MCP sampling support - #10087
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 098043486c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| CallToolRequestParams::new("trigger-long-running-operation").with_arguments(object!({"duration": 1, "steps": 5 })), | ||
| CallToolRequestParams::new("get-structured-content").with_arguments(object!({"location": "New York"})), | ||
| CallToolRequestParams::new("trigger-sampling-request").with_arguments(object!({"prompt": "Please provide a quote from The Great Gatsby", "maxTokens": 100 })) | ||
| CallToolRequestParams::new("get-structured-content").with_arguments(object!({"location": "New York"})) |
There was a problem hiding this comment.
Checked .github/workflows/pr-smoke-test.yml: the PR smoke job still invokes bash scripts/test_mcp.sh, and that script still asks the everything server to call sampleLLM/trigger-sampling-request and exits 1 if the sampling tool is not observed (scripts/test_mcp.sh lines 75-115). With this commit no longer advertising sampling and no handler for sampling/createMessage, the smoke job's MCP phase will fail whenever it reaches that block, even though this Rust replay no longer calls the sampling tool. Please remove or rewrite the shell smoke-test sampling block along with this test case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 281b261efc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
|
|
||
| function isRetiredChatApp(app: GooseApp) { | ||
| return app.mcpServers?.includes('apps') && app.name.trim().toLowerCase() === 'chat'; |
There was a problem hiding this comment.
On a clean profile after the bundled Chat app is no longer installed, users can still create or import an unrelated Goose app named chat; this predicate treats any apps resource with that name as the retired sampling demo, so the Apps page shows the warning and disables Launch for a perfectly valid custom app. Please identify the old bundled app more narrowly, or migrate/delete only that cached entry, instead of blocking all custom apps named chat.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9efb8e647
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ca769adb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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>'}`, |
There was a problem hiding this comment.
In desktop sessions, MCP servers still see Goose advertise sampling because GooseClient::get_info still calls .enable_sampling() in crates/goose/src/agents/mcp_client.rs, but MCP app iframes now get an unhandled-method result for sampling/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.
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
* main: (42 commits) Fix MCP app sandbox bridge lifecycle (#10064) fix(bedrock): send inference config (max_tokens, temperature) on Converse (#9889) feat(providers): support OpenRouter request parameters (#9276) Migrate local inference model management to ACP (#10124) (attempt to) fix disk space errors in linux release builds (#10024) feat: add --edit session flag to edit conversation before forking (#9799) feat: add iFlytek Spark and Astron MaaS providers (#9837) fix(desktop): dedupe Nostr session deep link imports (#9918) [codex] Add SessionStart hook parity outside CLI (#9970) feat(providers): add Fireworks AI declarative provider (#9990) fix(providers): don't retry deterministically-permanent 400s (thinking-block immutability) (#10005) fix(deps): downgrade pkcs8 to v0.10 to match sec1/pkcs1 v0.7 (#10119) chore(deps): bump actions/cache from 5.0.2 to 6.0.0 (#10051) Make OpenAI Responses API store param configurable (#10040) remove unsupported model (#10121) chore(release): bump version to 1.40.0 (minor) (#10099) move ollama provider into goose-providers (#9986) UI acp migratoin: Decouple desktop UI types from generated OpenAPI types (#10109) fix(otel): use async reqwest client so OTLP export works in `goose serve` mode (#10100) feat (acp): exposed available tools in acp schema (#10097) ...
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7347b8fdb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "appsView.retiredChatApp": { | ||
| "defaultMessage": "Chat app retired" | ||
| }, | ||
| "appsView.retiredChatAppDetail": { | ||
| "defaultMessage": "We removed this feature because MCP sampling is no longer supported." | ||
| }, |
There was a problem hiding this comment.
Adding these source catalog keys means pnpm lint:check will run i18n:check, whose locale validator requires every non-English src/i18n/messages/*.json file to mirror en.json. The commit only adds the two appsView.retiredChatApp* keys to some locales; de, fr, id, it, ms, pt, vi, and zh-TW are still missing them, so the validation step fails as soon as this UI lint runs. Please add the keys to the remaining locale catalogs too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f9555c81c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| status: 'error' as const, | ||
| message: `Unhandled JSON-RPC method: ${request.method ?? '<unknown>'}`, | ||
| }; |
There was a problem hiding this comment.
Fresh evidence in this revision is that this fallback now always reports sampling/createMessage as unhandled, while GooseClient::get_info still calls .enable_sampling() in crates/goose/src/agents/mcp_client.rs:537 and the MCP client still implements create_message. For an MCP server that sees the sampling capability and serves an app whose iframe calls sampling/createMessage (the retired built-in chat used this path), the app now fails at runtime despite the advertised capability; either keep routing this bridge to the existing sampling handler or stop advertising/removing sampling globally in the same change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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.
* main: Remove MCP sampling support (#10087)
* main: (31 commits) test: generic validator for declarative providers (#10010) UI acp migratoin: Decouple desktop UI types from generated OpenAPI types (Part 2) (#10149) Remove MCP sampling support (#10087) Support TLS for ACP serve (#10088) feat (ui): Migrate dictation local model manager to ACP (#10131) Fix MCP app sandbox bridge lifecycle (#10064) fix(bedrock): send inference config (max_tokens, temperature) on Converse (#9889) feat(providers): support OpenRouter request parameters (#9276) Migrate local inference model management to ACP (#10124) (attempt to) fix disk space errors in linux release builds (#10024) feat: add --edit session flag to edit conversation before forking (#9799) feat: add iFlytek Spark and Astron MaaS providers (#9837) fix(desktop): dedupe Nostr session deep link imports (#9918) [codex] Add SessionStart hook parity outside CLI (#9970) feat(providers): add Fireworks AI declarative provider (#9990) fix(providers): don't retry deterministically-permanent 400s (thinking-block immutability) (#10005) fix(deps): downgrade pkcs8 to v0.10 to match sec1/pkcs1 v0.7 (#10119) chore(deps): bump actions/cache from 5.0.2 to 6.0.0 (#10051) Make OpenAI Responses API store param configurable (#10040) remove unsupported model (#10121) ...
2 participants
Summary
Testing