Cache stable plugin metadata separately from live MCP runtimes - #29946
Cache stable plugin metadata separately from live MCP runtimes#29946jif-oai wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ca277e6f7
ℹ️ 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".
e522746 to
f7c2e30
Compare
8ca277e to
1d223f1
Compare
1d223f1 to
cb2326b
Compare
dde87ee to
c33d6df
Compare
cb2326b to
ee61287
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee612873ca
ℹ️ 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".
| } = plugin.plugin().location(); | ||
| let mcp_servers = | ||
| load_from_file_system(plugin.plugin(), plugin_root, plugin.file_system()).await?; | ||
| let apps = load_apps(&plugin).await?; |
There was a problem hiding this comment.
When a selected executor plugin declares valid MCP servers plus an apps config path, any read or parse error in that app file makes ExecutorPluginRuntime::project fail here; project_executor_plugins then logs and skips the entire plugin, so the model and app-server lose the plugin’s MCP tools just because optional connector metadata is bad. The previous MCP provider did not depend on app config parsing, so keep app loading best-effort or add integration coverage for this failure mode.
AGENTS.md reference: AGENTS.md:L112-L118
Useful? React with 👍 / 👎.
| let connector_snapshot = codex_connectors::ConnectorSnapshot::from_plugin_capability_summaries( | ||
| loaded_plugins.capability_summaries(), | ||
| ); | ||
| let connector_snapshot = &step_context.mcp.config().connector_snapshot; |
There was a problem hiding this comment.
When a running session reloads user config, refresh_runtime_config clears plugin caches but does not rebuild the cached MCP runtime; this line now reads connector IDs from the runtime’s old connector_snapshot instead of the freshly loaded plugins above. If a user enables or disables plugin apps at runtime, model-visible app connectors remain stale until an MCP refresh or session restart, whereas this was rebuilt from current plugin config before the change.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| let mcp_runtime_context = McpRuntimeContext::new( | ||
| Arc::clone(&environment_manager), | ||
| session_configuration.cwd().to_path_buf(), | ||
| ); |
There was a problem hiding this comment.
When a thread starts with a primary environment whose cwd differs from session_configuration.cwd and has no selected capability roots, mcp_runtime_for_step reuses this base runtime directly. Building that runtime with the session cwd makes stdio MCP servers that omit an explicit cwd start in the wrong directory, whereas the previous initialization derived the fallback from the primary turn environment before constructing the manager.
Useful? React with 👍 / 👎.
| ResourceManager::Live(runtime) => runtime | ||
| .load_full() | ||
| .expect("MCP runtime must be installed before reading resources") |
There was a problem hiding this comment.
Session::new inserts McpResourceClient into ThreadStartInput.session_store before the MCP runtime is installed, so a thread lifecycle contributor that calls has_server, list_resources, read_resource, or cache_key during on_thread_start now hits this expect and panics. Before this change the client pointed at an uninitialized manager, so startup extensions could safely observe an empty/unavailable MCP resource set instead of crashing the thread.
Useful? React with 👍 / 👎.
| self.build_mcp_runtime( | ||
| McpRuntimeScope::Turn(turn_context), | ||
| mcp_config, | ||
| configured_servers, |
There was a problem hiding this comment.
When a selected capability root resolves to executor-plugin MCP servers and the thread also has any configured/base MCP server, this builds a new runtime from base.config() so those base servers are started again, while the original base runtime remains held in SelectedMcpRuntimeCache. That leaves the original manager and its stdio/HTTP MCP clients alive in parallel for the rest of the thread, duplicating server processes and background work; the base manager should be shut down/swapped out, or only the base config should be retained, after publishing the projected runtime.
Useful? React with 👍 / 👎.
ee61287 to
c7332f8
Compare
0015e2c to
cffa959
Compare
c7332f8 to
8ef08be
Compare
cffa959 to
addf6b5
Compare
8ef08be to
1cef19d
Compare
…step (#29856) ## Why `selectedCapabilityRoots` is durable thread intent: “use this capability root from environment `worker`.” The important product assumption is: > One environment ID always names the same logical executor and stable contents. `worker` does not silently change from executor A to an unrelated executor B. The process-local connection handle for `worker` can still be replaced while Codex is running, though, for example when `environment/add` registers a fresh handle for the same logical environment. The thread should persist only the stable selection. Each model step should pair that selection with the exact ready handle captured for that step. ## The boundary ```text persisted thread intent plugin@1 -> environment "worker" | | capture the current step v model-step view unavailable, or plugin@1 + worker's exact captured ready handle ``` The environment ID is the stable identity and cache key. The `Arc<Environment>` is only a process-local handle retained so consumers of one model step use the same captured environment. It is never persisted and it does not imply different environment contents. ## What changes ### Persist the stable selection Selected roots are written into `SessionMeta` and restored with the thread. Forked subagents inherit the same selections, including bounded-history forks. Only stable data is persisted: root ID, environment ID, and root path. ### Capture readiness together with the exact handle The environment snapshot records: ```rust environment_id -> Some(Arc<Environment>) // ready in this step environment_id -> None // still starting in this step ``` This prevents readiness and execution from coming from different registry snapshots. For example: ```text step snapshot: worker -> handle A, ready environment/add: worker -> fresh handle B for the same logical environment current step: plugin@1 still uses captured handle A ``` Without carrying handle A in the snapshot, the resolver could combine “A was ready” with handle B and treat B as ready before it had finished starting. This does not change cache invalidation. Stable capability metadata remains identified by environment ID and capability root. Replacing a process-local handle under the same stable environment ID does not invalidate or rediscover that metadata. ### Resolve availability per model step - A ready captured environment produces resolved roots using its captured handle. - A starting, missing, or failed environment is omitted from that step. - A selected lazy environment that is outside the turn's captured environment set is asked to start, and a later step can observe it as ready. - No capability files are scanned here. Transient transport disconnects remain the remote client's reconnect concern. This PR models initial attachment/readiness; it does not add live socket-connectivity state. ## Example ```text thread selection: plugin@1 -> environment "worker" step 1: worker is starting -> plugin@1 unavailable step 2: worker is ready -> plugin@1 resolves through worker's captured handle step 3: fresh local handle -> current step remains pinned; a later step captures its own view ``` Temporary unavailability does not discard the durable selection. Later PRs can retain stable metadata caches while projecting only currently available capabilities into model-visible World State. ## Compatibility The app-server request shape does not change. Older rollouts without `selected_capability_roots` deserialize to an empty list. ## Stack 1. **This PR:** persist stable selected roots and resolve them through an exact model-step handle. 2. #29960: cache stable skill metadata and project available skills into World State. 3. #29946: cache stable plugin declarations and manage the separate live MCP runtime.
…step (openai#29856) ## Why `selectedCapabilityRoots` is durable thread intent: “use this capability root from environment `worker`.” The important product assumption is: > One environment ID always names the same logical executor and stable contents. `worker` does not silently change from executor A to an unrelated executor B. The process-local connection handle for `worker` can still be replaced while Codex is running, though, for example when `environment/add` registers a fresh handle for the same logical environment. The thread should persist only the stable selection. Each model step should pair that selection with the exact ready handle captured for that step. ## The boundary ```text persisted thread intent plugin@1 -> environment "worker" | | capture the current step v model-step view unavailable, or plugin@1 + worker's exact captured ready handle ``` The environment ID is the stable identity and cache key. The `Arc<Environment>` is only a process-local handle retained so consumers of one model step use the same captured environment. It is never persisted and it does not imply different environment contents. ## What changes ### Persist the stable selection Selected roots are written into `SessionMeta` and restored with the thread. Forked subagents inherit the same selections, including bounded-history forks. Only stable data is persisted: root ID, environment ID, and root path. ### Capture readiness together with the exact handle The environment snapshot records: ```rust environment_id -> Some(Arc<Environment>) // ready in this step environment_id -> None // still starting in this step ``` This prevents readiness and execution from coming from different registry snapshots. For example: ```text step snapshot: worker -> handle A, ready environment/add: worker -> fresh handle B for the same logical environment current step: plugin@1 still uses captured handle A ``` Without carrying handle A in the snapshot, the resolver could combine “A was ready” with handle B and treat B as ready before it had finished starting. This does not change cache invalidation. Stable capability metadata remains identified by environment ID and capability root. Replacing a process-local handle under the same stable environment ID does not invalidate or rediscover that metadata. ### Resolve availability per model step - A ready captured environment produces resolved roots using its captured handle. - A starting, missing, or failed environment is omitted from that step. - A selected lazy environment that is outside the turn's captured environment set is asked to start, and a later step can observe it as ready. - No capability files are scanned here. Transient transport disconnects remain the remote client's reconnect concern. This PR models initial attachment/readiness; it does not add live socket-connectivity state. ## Example ```text thread selection: plugin@1 -> environment "worker" step 1: worker is starting -> plugin@1 unavailable step 2: worker is ready -> plugin@1 resolves through worker's captured handle step 3: fresh local handle -> current step remains pinned; a later step captures its own view ``` Temporary unavailability does not discard the durable selection. Later PRs can retain stable metadata caches while projecting only currently available capabilities into model-visible World State. ## Compatibility The app-server request shape does not change. Older rollouts without `selected_capability_roots` deserialize to an empty list. ## Stack 1. **This PR:** persist stable selected roots and resolve them through an exact model-step handle. 2. openai#29960: cache stable skill metadata and project available skills into World State. 3. openai#29946: cache stable plugin declarations and manage the separate live MCP runtime.
|
Closing this pull request because it has had no updates for more than 14 days. If you plan to continue working on it, feel free to reopen or open a new PR. |
Why
Selected plugin metadata and a running MCP server have different lifetimes:
.mcp.json, and app declarations belong to a stable selected environment root.This PR separates those two caches explicitly.
The flow
Cache 1: stable plugin projection
Successful projections are cached by the complete
SelectedCapabilityRootfor the session lifetime. A stable root that is not a plugin is also cached as such. Transient projection errors are not cached and may retry.Temporary environment unavailability does not discard this metadata. Returning availability does not reread the manifest, MCP declaration, or app declaration files.
There is intentionally no
.mcp.jsonor manifest file invalidation. Changing those files behind the same selected environment ID is outside the contract.Cache 2: live MCP runtime
The live runtime is reusable only when:
The
Arccomparison is only about live connections and processes. It is not plugin-content identity. If the connection handle changes, Codex may rebuild the MCP manager while reusing the stable plugin projection.The two live-runtime mutation points are explicit:
replace_base_and_invalidate_selected: install a refreshed base runtime and drop the selected live runtime.replace_selected_runtime: publish a runtime for the currently active selected roots and effective configuration.In-flight model steps keep their own
Arc<McpRuntimeSnapshot>, so publishing a new runtime does not mutate a manager already in use.Example
Consistent consumers
The same runtime supplies MCP tools, resources, apps, connector attribution, direct app-server requests, and elicitation routing. This prevents the model and app-server from observing different selected-plugin worlds.
The app-server request shape does not change.
Verification
Integration coverage exercises thread isolation, reuse of selected MCP state, connector/app projection, and thread resume. The previous test that created a plugin manifest after thread start was removed because mutable environment contents contradict the stable-environment contract.
Stack