Discover stdio MCP servers from selected executor plugins#27870
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf55b02be2
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 642d0f27e1
ℹ️ 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".
642d0f2 to
f6fbab9
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6fbab93e0
ℹ️ 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".
| @@ -39,3 +41,20 @@ impl McpServerContributor<Config> for HostedPluginRuntimeExtension { | |||
| pub fn install(builder: &mut ExtensionRegistryBuilder<Config>) { | |||
| builder.mcp_server_contributor(std::sync::Arc::new(HostedPluginRuntimeExtension)); | |||
| } | |||
|
|
|||
| /// Installs discovery for MCP servers declared by thread-selected executor plugins. | |||
| pub fn install_executor_plugins( | |||
There was a problem hiding this comment.
Wire executor plugin MCP discovery into startup
When executor plugins declare MCP servers, this installer is never wired into production startup: repo-wide rg shows the app-server production registry still only calls codex_mcp_extension::install, while install_executor_plugins and initialize_executor_plugin_thread_data are only referenced by tests. That means selected plugin MCP declarations won't be discovered for real threads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
In the next PR, calm down
f6fbab9 to
b718083
Compare
…7884) ## Why **In short:** this PR resolves already-discovered MCP registrations. It does not read selected plugins or discover their MCP servers. The resolved MCP catalog currently builds config and auto-discovered plugin registrations before runtime contributors are applied. A thread-selected plugin needs a distinct precedence tier in that same initial resolution pass: otherwise a disabled lower-precedence winner can leave stale name-level state behind, and the winning MCP tools cannot be attributed to the selected package reliably. This PR adds that catalog boundary before executor discovery is connected. ## What changed - Added an explicit selected-plugin registration tier between auto-discovered plugins and explicit config. - Collected selected-plugin contributions before the initial catalog build, while leaving compatibility and generic extension overlays in their existing runtime phase. - Retained the winning plugin ID and display name directly on plugin-owned catalog registrations. - Derived MCP tool provenance from the winning catalog entry instead of joining against local-only plugin summaries. - Retained the winning selected server's tool approval policy in the running connection manager, so a selected registration cannot inherit approval behavior from a losing local plugin. - Kept remembered approval session-scoped for selected plugins until there is an authority-aware persistence contract; Codex will not write approval back to an unrelated local plugin. - Preserved existing name-level disabled vetoes for discovered plugins and config, while keeping a selected package's own disabled registration scoped to that registration. - Preserved deterministic selection order and existing config, compatibility, and extension precedence. The resulting order is: ```text auto-discovered plugin < selected plugin < explicit config < compatibility registration < extension overlay ``` ## Behavior and scope This is a catalog and provenance change only. No production host contributes selected-plugin MCP registrations yet, so existing local MCP behavior remains unchanged. The stacked follow-up, #27870, installs the executor plugin provider that produces these registrations. App-server activation remains a separate final step. ## Verification Focused tests cover precedence, deterministic selected-plugin conflicts, disabled-veto behavior across catalog phases, managed requirements before selected-plugin resolution, winning-server approval policy, and attribution when local and selected packages share an ID or server name. CI owns execution of the test suite.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5453ca8f4
ℹ️ 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".
Why
In short: this PR discovers MCP registrations by reading a selected plugin's
.mcp.jsonon its executor. #27884 then resolves those registrations in the shared catalog.thread/start.selectedCapabilityRootscan select a plugin root owned by an executor, and Codex can resolve that package through the executor filesystem. MCP declarations inside the selected plugin are still ignored.This PR adds the source-specific discovery layer on top of the selected-plugin catalog boundary in #27884:
The existing MCP launcher and connection manager remain unchanged. MCP config parsing is shared with local plugins through #27863.
What changed
.mcp.json; a missing default file means the plugin has no MCP servers.Behavior and scope
There is intentionally no production behavior change yet. This PR provides the executor provider and contribution boundary, but app-server does not install it in this change. Existing local plugin MCP loading is unchanged, and no MCP process is launched by this PR alone.
Assumptions
Follow-up
The next PR installs this contributor in app-server and adds an end-to-end test proving that a selected plugin MCP tool launches on its owning executor, can be called by the model, survives an explicit MCP refresh, and is invisible when its root was not selected.
Resume, fork, environment removal or ID changes, dynamic catalog reload, and executor-owned HTTP MCP placement remain separate lifecycle decisions.
Verification
Focused tests cover executor-only filesystem reads, missing and malformed config, stdio filtering and normalization, managed requirements, package attribution, and selection order. CI owns execution of the test suite.