Fix TUI remote config reads#23625
Conversation
…fig-reads # Conflicts: # codex-rs/tui/src/app/background_requests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 167b0ea080
ℹ️ 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".
| let mut capabilities = Vec::new(); | ||
| for read_params in mention_reads { | ||
| let plugin_name = read_params.plugin_name.clone(); | ||
| match fetch_plugin_detail(request_handle.clone(), read_params).await { |
There was a problem hiding this comment.
When the active workspace has project-scoped plugin settings, this plugin/read no longer resolves against the same cwd used for plugin/list; the server-side plugin_read_response derives config from the marketplace path parent, or from no cwd for remote plugins. In that context .codex layers for the current thread, such as skills.config disabling a plugin skill in this repo, are ignored and the mention popup can advertise/inject capabilities that are disabled for the active thread. Please carry the current cwd through hydration or use a read API that resolves against the same cwd.
Useful? React with 👍 / 👎.
| .collect() | ||
| fn plugin_detail_capability_summary(detail: PluginDetail) -> Option<PluginCapabilitySummary> { | ||
| let config_name = plugin_mention_config_name(&detail.marketplace_name, &detail.summary)?; | ||
| Some(PluginCapabilitySummary { |
There was a problem hiding this comment.
When plugin/read returns an enabled plugin whose skills are all disabled and that has no MCP servers or apps, this still creates a mention candidate. The previous PluginLoadOutcome::capability_summaries() path filtered out summaries unless they had at least one active skill, MCP server, or app, so this regression can surface $plugin suggestions that contribute nothing but still get submitted as UserInput::Mention. Please return None for zero-capability summaries.
Useful? React with 👍 / 👎.
| if let Err(err) = self.refresh_in_memory_config_from_disk().await { | ||
| tracing::warn!(error = %err, "failed to refresh config after plugin install"); | ||
| } | ||
| self.refresh_config_layer_stack_from_app_server_best_effort( |
There was a problem hiding this comment.
In the remote TUI, this server-side layer refresh is immediately followed by AppCommand::reload_user_config() in the same success branch, and thread_routing handles that command by calling self.refresh_in_memory_config_from_disk(), rebuilding from the client filesystem. When the server just mutated plugin or marketplace config and the client copy diverges, that subsequent local reload overwrites the app-server layer stack that was just fetched, so the stale-state bug returns after these mutation flows. Please keep the TUI refresh on the app-server config path, or skip the local disk rebuild for remote sessions.
Useful? React with 👍 / 👎.
| ) -> Result<HashMap<String, McpServerConfig>> { | ||
| let response = | ||
| fetch_app_server_config(request_handle, Some(cwd), /*include_layers*/ false).await?; | ||
| mcp_servers_from_app_server_config(response.config) |
There was a problem hiding this comment.
For workspaces where managed requirements disable MCP servers, config/read returns the raw effective config layers rather than the materialized Config after requirements are applied, and McpServerConfig.disabled_reason is also skipped during serialization. Decoding response.config here therefore makes requirement-disabled servers look enabled and drops the reason, while mcpServerStatus/list does not carry an enabled flag to correct it, so /mcp verbose can report a disabled server as enabled. Please fetch a materialized MCP config/status from the app server that includes the constrained enabled state and reason.
Useful? React with 👍 / 👎.
…fig-reads # Conflicts: # codex-rs/tui/src/app/config_persistence.rs
|
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
PRs #22914-#22916 are moving TUI config writes to the app server. During review, we found a separate class of bugs already present on main: some remote TUI flows still re-read client-local config after plugin, marketplace, or MCP state should be owned by the app server.
In remote mode, that can show stale client-local state after server-side mutations or render MCP inventory from the wrong config source.
What
config/readlayers instead of localconfig.toml.plugin/listandplugin/read, removing client-local plugin config/cache reads./mcpinventory from app-server status plus app-server config, so client/server config divergence does not misrender remote state.