feat(mcp): Dim 5 leftovers 1/2 — sampling completer prod wiring + /mcp command + embedded MCP plane#115
feat(mcp): Dim 5 leftovers 1/2 — sampling completer prod wiring + /mcp command + embedded MCP plane#115sweetcornna wants to merge 1 commit into
Conversation
…p command + embedded MCP plane Two of the four verified-open Dim 5 gaps (audit/GAP_CLOSE_2026-07-08.md A4 + A1): A4 — sampling completer production wiring. The v1.26.0 SamplingResponder had no completer writer (state.extras["mcp_sampling_completer"] had zero producers), so advertises_capability was always False and every server-initiated sampling/createMessage short-circuited to sampling_unavailable. New gateway/mcp/sampling_completer.py wraps the LIVE provider registry — read per call because the providers bootstrap runs AFTER the MCP block and hot-reload swaps the handle. Streamed token deltas fold into one SamplingResult; reasoning deltas are dropped (never leaked to the requesting server); finish_reason maps to MCP stopReason. A registry-less call degrades to a per-request INTERNAL_ERROR via the responder's existing exception guard. A1 — /mcp console command (claude-code analog): list / tools [server] / add <name> <cmd|url> [args…] / remove / restart / test / enable / disable, with /hooks-style "unavailable" degradation in attach mode. Substrate for A1: the embedded (Mode A) console now brings up the [mcp]-configured external servers — previously build_registry_invoker got mcp_manager=None, so the "full brain" had no MCP tool face at all. Advertisement + execution routing reuse the gateway's own register_mcp_tools seam (synthesized mcp-kind entries → McpToolBridge; tools_json → ChatStart), with the same allowedMcpServers/ deniedMcpServers policy semantics. /mcp hot-plug re-advertises via EmbeddedBrain.refresh_mcp_tools (new ChatService.with_advertised_tools setter) + prune_stale_mcp_entries; aclose() tears the manager down. Tests: 4 completer/responder end-to-end (incl. late-registry pickup), 7 /mcp command. Full console+gateway+mcp suites 1833 passed; ruff/mypy green.
github-actions
Bot
added
codex:needs-review
![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: 4d06e77651
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| manager = McpClientManager.from_config(cfg) | ||
| if manager.server_count == 0: | ||
| return None | ||
| await manager.connect_all() |
There was a problem hiding this comment.
In embedded Mode A, configured MCP managers are connected here but never get on_tools_changed assigned as the gateway does. When a server sends notifications/tools/list_changed, McpClientManager re-lists its internal cache, but without a callback into refresh_mcp_tools() the ChatService tools_json and synthesized registry entries stay at the boot snapshot, so newly added tools remain invisible and removed tools can stay advertised until a manual /mcp mutation or restart.
Useful? React with 👍 / 👎.
| provider, upstream_model, _params = registry.resolve( | ||
| request.model, | ||
| aliases=_alias_entries(models_cfg), | ||
| ) |
There was a problem hiding this comment.
ProviderRegistry.resolve() returns the merged provider/alias params, but the completer stores them in _params and then drops them before chat_stream(). For MCP sampling models configured as aliases with defaults or provider-specific options such as top_p, reasoning_effort, or other adapter extras, server-initiated sampling now runs with different or missing settings compared with normal gateway/direct chats, which fold these params into sampling knobs and extra.
Useful? React with 👍 / 👎.
| denied=denied, | ||
| ) | ||
| await prune_stale_mcp_entries(registry, advertised) | ||
| service.with_advertised_tools(tools_json) |
There was a problem hiding this comment.
When the console starts with no [mcp] servers, _build_plugin_tool_executor() has already built the RegistryToolExecutor with mcp_manager=None; later /mcp add creates a manager and this refresh only updates tools_json/registry entries. The next turn can therefore see the newly advertised MCP tool, but execution still goes through the stale invoker with no McpToolBridge and returns mcp_bridge_unavailable until restart.
Useful? React with 👍 / 👎.
| return "" | ||
|
|
||
| if sub == "add": | ||
| parts = rest.split() |
There was a problem hiding this comment.
/mcp add
Splitting the add command with plain str.split() breaks common stdio MCP commands whose arguments need quoting, such as filesystem roots containing spaces (/mcp add files npx ... "/Users/me/My Project"). Those quotes are not honored, so the spawned MCP server receives the path as multiple arguments and typically fails to start; using shell-style parsing would preserve the intended argv.
Useful? React with 👍 / 👎.
| try: | ||
| from corlinman_mcp_server import McpClientManager # noqa: PLC0415 | ||
|
|
||
| manager = McpClientManager.from_config(cfg) |
There was a problem hiding this comment.
This embedded MCP bring-up reads the same config as the gateway but creates McpClientManager without a SamplingResponder. In console Mode A sessions with [mcp.sampling] enabled, connected servers therefore never see the sampling capability and have no sampling/createMessage handler, even though the gateway path for the same config now wires provider-backed sampling.
Useful? React with 👍 / 👎.
github-actions
Bot
added
codex:reviewed
1 participant
Summary
Closes two of the four verified-open Dim 5 gaps (
audit/GAP_CLOSE_2026-07-08.mdA4 + A1):A4 — sampling completer production wiring
The v1.26.0
SamplingRespondershipped mode-gate/rate-limit/whitelist but no completer was ever injected (state.extras["mcp_sampling_completer"]had zero writers → capability never advertised, everysampling/createMessage→sampling_unavailable). New gateway/mcp/sampling_completer.py:finish_reason→ MCPstopReasonmapping; registry-less call degrades to a clean per-requestINTERNAL_ERRORthrough the responder's existing exception guard.Secure-by-default is unchanged: nothing advertises unless the operator opts in via
[mcp.sampling] mode + allowed_models.A1 —
/mcpconsole command + embedded MCP plane/mcp list | tools [server] | add <name> <cmd|url> [args…] | remove | restart | test | enable | disable, with/hooks-style unavailable degradation in attach mode.[mcp]-configured external servers — previouslybuild_registry_invokergotmcp_manager=None, so the "full brain" had no MCP tool face at all. Advertisement + execution reuse the gateway'sregister_mcp_toolsseam (synthesizedmcp-kind entries →McpToolBridge;tools_json→ChatStart), sameallowedMcpServers/deniedMcpServerspolicy./mcphot-plug re-advertises throughEmbeddedBrain.refresh_mcp_tools(newChatService.with_advertised_toolssetter) +prune_stale_mcp_entries;aclose()tears the manager down.Tests
/mcpcommand tests (list/tools/add url+stdio/dup/lifecycle/usage)Notes
.mcp.jsonscopes/precedence + clientresources/list+read) follows in a separate PR — both are L-sized with their own review surface.CHANGELOG.md/pyproject.tomlbump to 1.28.0 will trivially conflict with fix(security): W8 multi-tenant session isolation (critical, hunt-51) #114's 1.27.1 bump — whichever merges second rebases the version line.