From 8a827d6426e354aa2da5f6f32154131fdcc58c67 Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Thu, 28 May 2026 09:38:34 -0700 Subject: [PATCH 1/4] Expose MCP server info as part of server status (#24698) # Summary Expose MCP server info via App Server (when available) so apps can render a richer MCP experience --- .../codex_app_server_protocol.schemas.json | 51 +++++ .../codex_app_server_protocol.v2.schemas.json | 51 +++++ .../json/v2/ListMcpServerStatusResponse.json | 51 +++++ .../schema/typescript/McpServerInfo.ts | 9 + .../schema/typescript/index.ts | 1 + .../schema/typescript/v2/McpServerStatus.ts | 3 +- .../src/protocol/v2/mcp.rs | 2 + .../src/protocol/v2/tests.rs | 75 +++++++ codex-rs/app-server/README.md | 2 +- .../src/request_processors/mcp_processor.rs | 2 + .../tests/suite/v2/mcp_server_status.rs | 12 +- codex-rs/codex-mcp/src/codex_apps.rs | 81 +++++++- codex-rs/codex-mcp/src/connection_manager.rs | 34 ++- .../codex-mcp/src/connection_manager_tests.rs | 193 ++++++++++++++++-- codex-rs/codex-mcp/src/mcp/mod.rs | 5 + codex-rs/codex-mcp/src/rmcp_client.rs | 50 ++++- codex-rs/protocol/src/mcp.rs | 12 ++ codex-rs/tui/src/app/background_requests.rs | 2 + codex-rs/tui/src/app/tests.rs | 1 + codex-rs/tui/src/history_cell/tests.rs | 2 + 20 files changed, 599 insertions(+), 40 deletions(-) create mode 100644 codex-rs/app-server-protocol/schema/typescript/McpServerInfo.ts diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 1d1a64a2fc..c5ce0f8622 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -10964,6 +10964,47 @@ "title": "McpResourceReadResponse", "type": "object" }, + "McpServerInfo": { + "description": "Presentation metadata advertised by an initialized MCP server.", + "properties": { + "description": { + "type": [ + "string", + "null" + ] + }, + "icons": { + "items": true, + "type": [ + "array", + "null" + ] + }, + "name": { + "type": "string" + }, + "title": { + "type": [ + "string", + "null" + ] + }, + "version": { + "type": "string" + }, + "websiteUrl": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "name", + "version" + ], + "type": "object" + }, "McpServerMigration": { "properties": { "name": { @@ -11074,6 +11115,16 @@ }, "type": "array" }, + "serverInfo": { + "anyOf": [ + { + "$ref": "#/definitions/v2/McpServerInfo" + }, + { + "type": "null" + } + ] + }, "tools": { "additionalProperties": { "$ref": "#/definitions/v2/Tool" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 1b49b38641..92134a2df3 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -7493,6 +7493,47 @@ "title": "McpResourceReadResponse", "type": "object" }, + "McpServerInfo": { + "description": "Presentation metadata advertised by an initialized MCP server.", + "properties": { + "description": { + "type": [ + "string", + "null" + ] + }, + "icons": { + "items": true, + "type": [ + "array", + "null" + ] + }, + "name": { + "type": "string" + }, + "title": { + "type": [ + "string", + "null" + ] + }, + "version": { + "type": "string" + }, + "websiteUrl": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "name", + "version" + ], + "type": "object" + }, "McpServerMigration": { "properties": { "name": { @@ -7603,6 +7644,16 @@ }, "type": "array" }, + "serverInfo": { + "anyOf": [ + { + "$ref": "#/definitions/McpServerInfo" + }, + { + "type": "null" + } + ] + }, "tools": { "additionalProperties": { "$ref": "#/definitions/Tool" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json index fc181c2702..0dc2f5e2e4 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json @@ -10,6 +10,47 @@ ], "type": "string" }, + "McpServerInfo": { + "description": "Presentation metadata advertised by an initialized MCP server.", + "properties": { + "description": { + "type": [ + "string", + "null" + ] + }, + "icons": { + "items": true, + "type": [ + "array", + "null" + ] + }, + "name": { + "type": "string" + }, + "title": { + "type": [ + "string", + "null" + ] + }, + "version": { + "type": "string" + }, + "websiteUrl": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "name", + "version" + ], + "type": "object" + }, "McpServerStatus": { "properties": { "authStatus": { @@ -30,6 +71,16 @@ }, "type": "array" }, + "serverInfo": { + "anyOf": [ + { + "$ref": "#/definitions/McpServerInfo" + }, + { + "type": "null" + } + ] + }, "tools": { "additionalProperties": { "$ref": "#/definitions/Tool" diff --git a/codex-rs/app-server-protocol/schema/typescript/McpServerInfo.ts b/codex-rs/app-server-protocol/schema/typescript/McpServerInfo.ts new file mode 100644 index 0000000000..a3f6b0e142 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/McpServerInfo.ts @@ -0,0 +1,9 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { JsonValue } from "./serde_json/JsonValue"; + +/** + * Presentation metadata advertised by an initialized MCP server. + */ +export type McpServerInfo = { name: string, title: string | null, version: string, description: string | null, icons: Array | null, websiteUrl: string | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/index.ts b/codex-rs/app-server-protocol/schema/typescript/index.ts index 8be75af546..458d2e43b9 100644 --- a/codex-rs/app-server-protocol/schema/typescript/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/index.ts @@ -42,6 +42,7 @@ export type { InternalSessionSource } from "./InternalSessionSource"; export type { LocalShellAction } from "./LocalShellAction"; export type { LocalShellExecAction } from "./LocalShellExecAction"; export type { LocalShellStatus } from "./LocalShellStatus"; +export type { McpServerInfo } from "./McpServerInfo"; export type { MessagePhase } from "./MessagePhase"; export type { ModeKind } from "./ModeKind"; export type { NetworkPolicyAmendment } from "./NetworkPolicyAmendment"; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts b/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts index 430494e268..d2e99ce96f 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts @@ -1,9 +1,10 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { McpServerInfo } from "../McpServerInfo"; import type { Resource } from "../Resource"; import type { ResourceTemplate } from "../ResourceTemplate"; import type { Tool } from "../Tool"; import type { McpAuthStatus } from "./McpAuthStatus"; -export type McpServerStatus = { name: string, tools: { [key in string]?: Tool }, resources: Array, resourceTemplates: Array, authStatus: McpAuthStatus, }; +export type McpServerStatus = { name: string, serverInfo: McpServerInfo | null, tools: { [key in string]?: Tool }, resources: Array, resourceTemplates: Array, authStatus: McpAuthStatus, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs b/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs index 31ec85a478..ae61f12b2d 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs @@ -2,6 +2,7 @@ use super::shared::v2_enum_from_core; use codex_protocol::approvals::ElicitationRequest as CoreElicitationRequest; use codex_protocol::items::McpToolCallError as CoreMcpToolCallError; use codex_protocol::mcp::CallToolResult as CoreMcpCallToolResult; +use codex_protocol::mcp::McpServerInfo; use codex_protocol::mcp::Resource as McpResource; pub use codex_protocol::mcp::ResourceContent as McpResourceContent; use codex_protocol::mcp::ResourceTemplate as McpResourceTemplate; @@ -53,6 +54,7 @@ pub enum McpServerStatusDetail { #[ts(export_to = "v2/")] pub struct McpServerStatus { pub name: String, + pub server_info: Option, pub tools: std::collections::HashMap, pub resources: Vec, pub resource_templates: Vec, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs index 0465a4e012..90273cd868 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs @@ -11,6 +11,7 @@ use codex_protocol::items::TurnItem; use codex_protocol::items::UserMessageItem; use codex_protocol::items::WebSearchItem; use codex_protocol::mcp::CallToolResult; +use codex_protocol::mcp::McpServerInfo; use codex_protocol::memory_citation::MemoryCitation as CoreMemoryCitation; use codex_protocol::memory_citation::MemoryCitationEntry as CoreMemoryCitationEntry; use codex_protocol::models::AdditionalPermissionProfile as CoreAdditionalPermissionProfile; @@ -2003,6 +2004,80 @@ fn mcp_server_elicitation_response_serializes_nullable_content() { ); } +#[test] +fn mcp_server_status_serializes_absent_server_info_as_null() { + let response = ListMcpServerStatusResponse { + data: vec![McpServerStatus { + name: "not-ready".to_string(), + server_info: None, + tools: HashMap::new(), + resources: Vec::new(), + resource_templates: Vec::new(), + auth_status: McpAuthStatus::Unsupported, + }], + next_cursor: None, + }; + + assert_eq!( + serde_json::to_value(response).expect("response should serialize"), + json!({ + "data": [{ + "name": "not-ready", + "serverInfo": null, + "tools": {}, + "resources": [], + "resourceTemplates": [], + "authStatus": "unsupported", + }], + "nextCursor": null, + }) + ); +} + +#[test] +fn mcp_server_status_serializes_absent_server_info_metadata_as_null() { + let response = ListMcpServerStatusResponse { + data: vec![McpServerStatus { + name: "initialized".to_string(), + server_info: Some(McpServerInfo { + name: "lookup-server".to_string(), + title: None, + version: "1.0.0".to_string(), + description: None, + icons: None, + website_url: None, + }), + tools: HashMap::new(), + resources: Vec::new(), + resource_templates: Vec::new(), + auth_status: McpAuthStatus::Unsupported, + }], + next_cursor: None, + }; + + assert_eq!( + serde_json::to_value(response).expect("response should serialize"), + json!({ + "data": [{ + "name": "initialized", + "serverInfo": { + "name": "lookup-server", + "title": null, + "version": "1.0.0", + "description": null, + "icons": null, + "websiteUrl": null, + }, + "tools": {}, + "resources": [], + "resourceTemplates": [], + "authStatus": "unsupported", + }], + "nextCursor": null, + }) + ); +} + #[test] fn sandbox_policy_round_trips_workspace_write_access() { let v2_policy = SandboxPolicy::WorkspaceWrite { diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index d7dfb00fc1..b91b206d6b 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -216,7 +216,7 @@ Example with notification opt-out: - `mcpServer/oauth/login` — start an OAuth login for a configured MCP server; returns an `authorization_url` and later emits `mcpServer/oauthLogin/completed` once the browser flow finishes. - `tool/requestUserInput` — prompt the user with 1–3 short questions for a tool call and return their answers (experimental). - `config/mcpServer/reload` — reload MCP server config from disk and queue a refresh for loaded threads (applied on each thread's next active turn); returns `{}`. Use this after editing `config.toml` without restarting the server. -- `mcpServerStatus/list` — enumerate configured MCP servers with their tools and auth status, plus resources/resource templates for `full` detail; supports optional `threadId` and cursor+limit pagination. If `threadId` is omitted, the server reads from the latest global config directly. If `detail` is omitted, the server defaults to `full`. +- `mcpServerStatus/list` — enumerate configured MCP servers with their tools, auth status, server info, plus resources/resource templates for `full` detail; supports optional `threadId` and cursor+limit pagination. If `threadId` is omitted, the server reads from the latest global config directly. If `detail` is omitted, the server defaults to `full`. - `mcpServer/resource/read` — read a resource from a configured MCP server by optional `threadId`, `server`, and `uri`, returning text/blob resource `contents`. If `threadId` is omitted, the server reads from the latest MCP config directly. - `mcpServer/tool/call` — call a tool on a thread's configured MCP server by `threadId`, `server`, `tool`, optional `arguments`, and optional `_meta`, returning the MCP tool result. - `windowsSandbox/setupStart` — start Windows sandbox setup for the selected mode (`elevated` or `unelevated`); accepts an optional absolute `cwd` to target setup for a specific workspace, returns `{ started: true }` immediately, and later emits `windowsSandbox/setupCompleted`. diff --git a/codex-rs/app-server/src/request_processors/mcp_processor.rs b/codex-rs/app-server/src/request_processors/mcp_processor.rs index 22d5a4326a..ae62e2e785 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -276,6 +276,7 @@ impl McpRequestProcessor { .await; let McpServerStatusSnapshot { + server_infos, tools_by_server, resources, resource_templates, @@ -315,6 +316,7 @@ impl McpRequestProcessor { .iter() .map(|name| McpServerStatus { name: name.clone(), + server_info: server_infos.get(name).cloned(), tools: tools_by_server.get(name).cloned().unwrap_or_default(), resources: resources.get(name).cloned().unwrap_or_default(), resource_templates: resource_templates.get(name).cloned().unwrap_or_default(), diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index 7af89d887b..32f98a1738 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -20,6 +20,7 @@ use codex_core::config::set_project_trust_level; use codex_protocol::config_types::TrustLevel; use pretty_assertions::assert_eq; use rmcp::handler::server::ServerHandler; +use rmcp::model::Implementation; use rmcp::model::JsonObject; use rmcp::model::ListResourceTemplatesResult; use rmcp::model::ListResourcesResult; @@ -99,6 +100,13 @@ url = "{mcp_server_url}/mcp" .map(|tool| tool.name.as_str()), Some("look-up.raw") ); + assert_eq!( + status + .server_info + .as_ref() + .and_then(|info| info.title.as_deref()), + Some("Lookup Server") + ); mcp_server_handle.abort(); let _ = mcp_server_handle.await; @@ -205,7 +213,9 @@ struct McpStatusServer { impl ServerHandler for McpStatusServer { fn get_info(&self) -> ServerInfo { - ServerInfo::new(ServerCapabilities::builder().enable_tools().build()) + ServerInfo::new(ServerCapabilities::builder().enable_tools().build()).with_server_info( + Implementation::new("lookup-server", "1.0.0").with_title("Lookup Server"), + ) } async fn list_tools( diff --git a/codex-rs/codex-mcp/src/codex_apps.rs b/codex-rs/codex-mcp/src/codex_apps.rs index eb88ffa6b1..3196a143e8 100644 --- a/codex-rs/codex-mcp/src/codex_apps.rs +++ b/codex-rs/codex-mcp/src/codex_apps.rs @@ -12,7 +12,9 @@ use crate::mcp::CODEX_APPS_MCP_SERVER_NAME; use crate::runtime::emit_duration; use crate::tools::MCP_TOOLS_CACHE_WRITE_DURATION_METRIC; use crate::tools::ToolInfo; +use anyhow::Context; use codex_login::CodexAuth; +use codex_protocol::mcp::McpServerInfo; use codex_utils_plugins::mcp_connector::is_connector_id_allowed; use codex_utils_plugins::mcp_connector::sanitize_name; use serde::Deserialize; @@ -20,8 +22,6 @@ use serde::Serialize; use sha1::Digest; use sha1::Sha1; -pub(crate) const CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION: u8 = 3; - #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CodexAppsToolsCacheKey { pub(crate) account_id: Option, @@ -44,11 +44,19 @@ pub(crate) struct CodexAppsToolsCacheContext { } impl CodexAppsToolsCacheContext { - pub(crate) fn cache_path(&self) -> PathBuf { + pub(crate) fn tools_cache_path(&self) -> PathBuf { + self.cache_path_in(CODEX_APPS_TOOLS_CACHE_DIR) + } + + pub(crate) fn server_info_cache_path(&self) -> PathBuf { + self.cache_path_in(CODEX_APPS_SERVER_INFO_CACHE_DIR) + } + + fn cache_path_in(&self, cache_dir: &str) -> PathBuf { let user_key_json = serde_json::to_string(&self.user_key).unwrap_or_default(); let user_key_hash = sha1_hex(&user_key_json); self.codex_home - .join(CODEX_APPS_TOOLS_CACHE_DIR) + .join(cache_dir) .join(format!("{user_key_hash}.json")) } } @@ -136,6 +144,7 @@ pub(crate) fn normalize_codex_apps_callable_namespace( pub(crate) fn write_cached_codex_apps_tools_if_needed( server_name: &str, cache_context: Option<&CodexAppsToolsCacheContext>, + server_info: &McpServerInfo, tools: &[ToolInfo], ) { if server_name != CODEX_APPS_MCP_SERVER_NAME { @@ -145,6 +154,9 @@ pub(crate) fn write_cached_codex_apps_tools_if_needed( if let Some(cache_context) = cache_context { let cache_write_start = Instant::now(); write_cached_codex_apps_tools(cache_context, tools); + if let Err(err) = write_cached_codex_apps_server_info(cache_context, server_info) { + tracing::warn!("failed to write Codex Apps server info cache: {err:#}"); + } emit_duration( MCP_TOOLS_CACHE_WRITE_DURATION_METRIC, cache_write_start.elapsed(), @@ -169,6 +181,17 @@ pub(crate) fn load_startup_cached_codex_apps_tools_snapshot( } } +pub(crate) fn load_startup_cached_codex_apps_server_info( + server_name: &str, + cache_context: Option<&CodexAppsToolsCacheContext>, +) -> Option { + if server_name != CODEX_APPS_MCP_SERVER_NAME { + return None; + } + + load_cached_codex_apps_server_info(cache_context?) +} + #[cfg(test)] pub(crate) fn read_cached_codex_apps_tools( cache_context: &CodexAppsToolsCacheContext, @@ -182,7 +205,7 @@ pub(crate) fn read_cached_codex_apps_tools( pub(crate) fn load_cached_codex_apps_tools( cache_context: &CodexAppsToolsCacheContext, ) -> CachedCodexAppsToolsLoad { - let cache_path = cache_context.cache_path(); + let cache_path = cache_context.tools_cache_path(); let bytes = match std::fs::read(cache_path) { Ok(bytes) => bytes, Err(err) if err.kind() == std::io::ErrorKind::NotFound => { @@ -204,7 +227,7 @@ pub(crate) fn write_cached_codex_apps_tools( cache_context: &CodexAppsToolsCacheContext, tools: &[ToolInfo], ) { - let cache_path = cache_context.cache_path(); + let cache_path = cache_context.tools_cache_path(); if let Some(parent) = cache_path.parent() && std::fs::create_dir_all(parent).is_err() { @@ -220,6 +243,42 @@ pub(crate) fn write_cached_codex_apps_tools( let _ = std::fs::write(cache_path, bytes); } +pub(crate) fn load_cached_codex_apps_server_info( + cache_context: &CodexAppsToolsCacheContext, +) -> Option { + let bytes = std::fs::read(cache_context.server_info_cache_path()).ok()?; + let cache: CodexAppsServerInfoDiskCache = serde_json::from_slice(&bytes).ok()?; + (cache.schema_version == CODEX_APPS_SERVER_INFO_CACHE_SCHEMA_VERSION) + .then_some(cache.server_info) +} + +fn write_cached_codex_apps_server_info( + cache_context: &CodexAppsToolsCacheContext, + server_info: &McpServerInfo, +) -> anyhow::Result<()> { + let cache_path = cache_context.server_info_cache_path(); + if let Some(parent) = cache_path.parent() { + std::fs::create_dir_all(parent).with_context(|| { + format!( + "failed to create Codex Apps server info cache directory `{}`", + parent.display() + ) + })?; + } + let bytes = serde_json::to_vec_pretty(&CodexAppsServerInfoDiskCache { + schema_version: CODEX_APPS_SERVER_INFO_CACHE_SCHEMA_VERSION, + server_info: server_info.clone(), + }) + .context("failed to serialize Codex Apps server info cache")?; + std::fs::write(&cache_path, bytes).with_context(|| { + format!( + "failed to write Codex Apps server info cache `{}`", + cache_path.display() + ) + })?; + Ok(()) +} + pub(crate) fn filter_disallowed_codex_apps_tools(tools: Vec) -> Vec { tools .into_iter() @@ -237,7 +296,17 @@ struct CodexAppsToolsDiskCache { tools: Vec, } +#[derive(Debug, Clone, Serialize, Deserialize)] +struct CodexAppsServerInfoDiskCache { + schema_version: u8, + server_info: McpServerInfo, +} + const CODEX_APPS_TOOLS_CACHE_DIR: &str = "cache/codex_apps_tools"; +pub(crate) const CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION: u8 = 3; + +const CODEX_APPS_SERVER_INFO_CACHE_DIR: &str = "cache/codex_apps_server_info"; +const CODEX_APPS_SERVER_INFO_CACHE_SCHEMA_VERSION: u8 = 1; fn sha1_hex(s: &str) -> String { let mut hasher = Sha1::new(); diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index da7c9963ac..d4a35eab1e 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -9,6 +9,7 @@ use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; +use std::sync::atomic::Ordering; use std::time::Duration; use std::time::Instant; @@ -44,6 +45,7 @@ use codex_config::McpServerTransportConfig; use codex_config::types::OAuthCredentialsStoreMode; use codex_login::CodexAuth; use codex_protocol::mcp::CallToolResult; +use codex_protocol::mcp::McpServerInfo; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; @@ -385,13 +387,13 @@ impl McpConnectionManager { pub async fn list_all_tools(&self) -> Vec { let mut tools = Vec::new(); for (server_name, managed_client) in &self.clients { - let has_startup_snapshot = managed_client.startup_snapshot.is_some(); + let has_cached_tool_info_snapshot = managed_client.cached_tool_info_snapshot.is_some(); let startup_complete = managed_client .startup_complete .load(std::sync::atomic::Ordering::Acquire); trace!( server_name = %server_name, - has_startup_snapshot, + has_cached_tool_info_snapshot, startup_complete, "waiting for MCP server tools while building tool list" ); @@ -400,7 +402,7 @@ impl McpConnectionManager { .instrument(trace_span!( "list_tools_for_server", server_name = %server_name, - has_startup_snapshot, + has_cached_tool_info_snapshot, startup_complete )) .await @@ -421,6 +423,31 @@ impl McpConnectionManager { normalize_tools_for_model_with_prefix(tools, self.prefix_mcp_tool_names) } + /// Returns presentation metadata without waiting for uncached clients still initializing. + /// Cached values will be used if available and the server is still starting up. + pub async fn list_available_server_infos(&self) -> HashMap { + let mut server_infos = HashMap::new(); + for (server_name, client) in &self.clients { + if !client.startup_complete.load(Ordering::Acquire) { + if let Some(server_info) = client.cached_server_info.clone() { + server_infos.insert(server_name.clone(), server_info); + } + continue; + } + match client.client().await { + Ok(managed_client) => { + server_infos.insert(server_name.clone(), managed_client.server_info); + } + Err(_) => { + if let Some(server_info) = client.cached_server_info.clone() { + server_infos.insert(server_name.clone(), server_info); + } + } + } + } + server_infos + } + /// Force-refresh codex apps tools by bypassing the in-process cache. /// /// On success, the refreshed tools replace the cache contents and the @@ -456,6 +483,7 @@ impl McpConnectionManager { write_cached_codex_apps_tools_if_needed( CODEX_APPS_MCP_SERVER_NAME, managed_client.codex_apps_tools_cache_context.as_ref(), + &managed_client.server_info, &tools, ); emit_duration( diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index a1e312d543..60a0026eeb 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1,9 +1,11 @@ use super::*; use crate::codex_apps::CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION; use crate::codex_apps::CodexAppsToolsCacheContext; +use crate::codex_apps::load_startup_cached_codex_apps_server_info; use crate::codex_apps::load_startup_cached_codex_apps_tools_snapshot; use crate::codex_apps::read_cached_codex_apps_tools; use crate::codex_apps::write_cached_codex_apps_tools; +use crate::codex_apps::write_cached_codex_apps_tools_if_needed; use crate::declared_openai_file_input_param_names; use crate::elicitation::ElicitationRequestManager; use crate::elicitation::elicitation_is_rejected_by_policy; @@ -20,6 +22,7 @@ use codex_config::Constrained; use codex_config::McpServerConfig; use codex_exec_server::EnvironmentManager; use codex_protocol::ToolName; +use codex_protocol::mcp::McpServerInfo; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::GranularApprovalConfig; use codex_protocol::protocol::McpAuthStatus; @@ -82,6 +85,17 @@ fn create_codex_apps_tools_cache_context( } } +fn create_test_server_info(title: &str) -> McpServerInfo { + McpServerInfo { + name: "codex-apps".to_string(), + title: Some(title.to_string()), + version: "1.0.0".to_string(), + description: None, + icons: None, + website_url: None, + } +} + fn model_tool_names(tools: &[ToolInfo]) -> HashSet { tools .iter() @@ -582,8 +596,8 @@ fn codex_apps_tools_cache_is_scoped_per_user() { assert_eq!(read_user_1[0].callable_name, "one"); assert_eq!(read_user_2[0].callable_name, "two"); assert_ne!( - cache_context_user_1.cache_path(), - cache_context_user_2.cache_path(), + cache_context_user_1.tools_cache_path(), + cache_context_user_2.tools_cache_path(), "each user should get an isolated cache file" ); } @@ -627,7 +641,7 @@ fn codex_apps_tools_cache_is_ignored_when_schema_version_mismatches() { Some("account-one"), Some("user-one"), ); - let cache_path = cache_context.cache_path(); + let cache_path = cache_context.tools_cache_path(); if let Some(parent) = cache_path.parent() { std::fs::create_dir_all(parent).expect("create parent"); } @@ -649,7 +663,7 @@ fn codex_apps_tools_cache_is_ignored_when_json_is_invalid() { Some("account-one"), Some("user-one"), ); - let cache_path = cache_context.cache_path(); + let cache_path = cache_context.tools_cache_path(); if let Some(parent) = cache_path.parent() { std::fs::create_dir_all(parent).expect("create parent"); } @@ -670,21 +684,112 @@ fn startup_cached_codex_apps_tools_loads_from_disk_cache() { CODEX_APPS_MCP_SERVER_NAME, "calendar_search", )]; - write_cached_codex_apps_tools(&cache_context, &cached_tools); + let server_info = create_test_server_info("Codex Apps"); + write_cached_codex_apps_tools_if_needed( + CODEX_APPS_MCP_SERVER_NAME, + Some(&cache_context), + &server_info, + &cached_tools, + ); - let startup_snapshot = load_startup_cached_codex_apps_tools_snapshot( + let startup_tools = load_startup_cached_codex_apps_tools_snapshot( + CODEX_APPS_MCP_SERVER_NAME, + Some(&cache_context), + ) + .expect("expected startup snapshot to load from cache"); + let cached_server_info = load_startup_cached_codex_apps_server_info( CODEX_APPS_MCP_SERVER_NAME, Some(&cache_context), ); - let startup_tools = startup_snapshot.expect("expected startup snapshot to load from cache"); assert_eq!(startup_tools.len(), 1); assert_eq!(startup_tools[0].server_name, CODEX_APPS_MCP_SERVER_NAME); assert_eq!(startup_tools[0].callable_name, "calendar_search"); + assert_eq!(cached_server_info, Some(server_info)); +} + +#[test] +fn startup_cached_codex_apps_tools_loads_without_server_info_cache() { + let codex_home = tempdir().expect("tempdir"); + let cache_context = create_codex_apps_tools_cache_context( + codex_home.path().to_path_buf(), + Some("account-one"), + Some("user-one"), + ); + let cache_path = cache_context.tools_cache_path(); + if let Some(parent) = cache_path.parent() { + std::fs::create_dir_all(parent).expect("create parent"); + } + let bytes = serde_json::to_vec_pretty(&serde_json::json!({ + "schema_version": CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION, + "tools": [create_test_tool(CODEX_APPS_MCP_SERVER_NAME, "calendar_search")], + })) + .expect("serialize"); + std::fs::write(cache_path, bytes).expect("write"); + + let startup_tools = load_startup_cached_codex_apps_tools_snapshot( + CODEX_APPS_MCP_SERVER_NAME, + Some(&cache_context), + ) + .expect("legacy startup snapshot should remain available"); + let cached_server_info = load_startup_cached_codex_apps_server_info( + CODEX_APPS_MCP_SERVER_NAME, + Some(&cache_context), + ); + + assert_eq!(startup_tools.len(), 1); + assert_eq!(startup_tools[0].callable_name, "calendar_search"); + assert_eq!(cached_server_info, None); +} + +#[test] +fn codex_apps_server_info_cache_survives_legacy_tools_cache_write() { + let codex_home = tempdir().expect("tempdir"); + let cache_context = create_codex_apps_tools_cache_context( + codex_home.path().to_path_buf(), + Some("account-one"), + Some("user-one"), + ); + let server_info = create_test_server_info("Codex Apps"); + write_cached_codex_apps_tools_if_needed( + CODEX_APPS_MCP_SERVER_NAME, + Some(&cache_context), + &server_info, + &[create_test_tool( + CODEX_APPS_MCP_SERVER_NAME, + "calendar_search", + )], + ); + + let cache_path = cache_context.tools_cache_path(); + if let Some(parent) = cache_path.parent() { + std::fs::create_dir_all(parent).expect("create parent"); + } + let bytes = serde_json::to_vec_pretty(&serde_json::json!({ + "schema_version": CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION - 1, + "tools": [create_test_tool(CODEX_APPS_MCP_SERVER_NAME, "calendar_search")], + })) + .expect("serialize"); + std::fs::write(cache_path, bytes).expect("write legacy tools cache"); + + assert_eq!( + load_startup_cached_codex_apps_server_info( + CODEX_APPS_MCP_SERVER_NAME, + Some(&cache_context), + ), + Some(server_info) + ); + assert!( + load_startup_cached_codex_apps_tools_snapshot( + CODEX_APPS_MCP_SERVER_NAME, + Some(&cache_context), + ) + .is_none() + ); } #[tokio::test] -async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { +async fn list_all_tools_uses_cached_tool_info_snapshot_while_client_is_pending() { let startup_tools = vec![create_test_tool( CODEX_APPS_MCP_SERVER_NAME, "calendar_create_event", @@ -703,7 +808,8 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { CODEX_APPS_MCP_SERVER_NAME.to_string(), AsyncManagedClient { client: pending_client, - startup_snapshot: Some(startup_tools), + cached_tool_info_snapshot: Some(startup_tools), + cached_server_info: None, startup_complete: Arc::new(std::sync::atomic::AtomicBool::new(false)), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), cancel_token: CancellationToken::new(), @@ -722,6 +828,43 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { assert_eq!(tool.callable_name, "calendar_create_event"); } +#[tokio::test] +async fn list_available_server_infos_uses_cache_while_client_is_pending() { + let pending_client = futures::future::pending::>() + .boxed() + .shared(); + let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); + let permission_profile = Constrained::allow_any(PermissionProfile::default()); + let mut manager = McpConnectionManager::new_uninitialized( + &approval_policy, + &permission_profile, + /*prefix_mcp_tool_names*/ true, + ); + let server_info = create_test_server_info("Codex Apps"); + manager.clients.insert( + CODEX_APPS_MCP_SERVER_NAME.to_string(), + AsyncManagedClient { + client: pending_client, + cached_tool_info_snapshot: Some(Vec::new()), + cached_server_info: Some(server_info.clone()), + startup_complete: Arc::new(std::sync::atomic::AtomicBool::new(false)), + tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), + cancel_token: CancellationToken::new(), + }, + ); + + let timeout_result = tokio::time::timeout( + Duration::from_millis(10), + manager.list_available_server_infos(), + ) + .await; + let server_infos = timeout_result.expect("server info lookup should not block on startup"); + assert_eq!( + server_infos.get(CODEX_APPS_MCP_SERVER_NAME), + Some(&server_info) + ); +} + #[tokio::test] async fn list_all_tools_accepts_canonical_namespaced_tool_names() { let startup_tools = vec![create_test_tool("rmcp", "echo")]; @@ -739,7 +882,8 @@ async fn list_all_tools_accepts_canonical_namespaced_tool_names() { "rmcp".to_string(), AsyncManagedClient { client: pending_client, - startup_snapshot: Some(startup_tools), + cached_tool_info_snapshot: Some(startup_tools), + cached_server_info: None, startup_complete: Arc::new(std::sync::atomic::AtomicBool::new(false)), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), cancel_token: CancellationToken::new(), @@ -781,7 +925,8 @@ async fn list_all_tools_applies_legacy_mcp_prefix_by_default() { "rmcp".to_string(), AsyncManagedClient { client: pending_client, - startup_snapshot: Some(startup_tools), + cached_tool_info_snapshot: Some(startup_tools), + cached_server_info: None, startup_complete: Arc::new(std::sync::atomic::AtomicBool::new(false)), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), cancel_token: CancellationToken::new(), @@ -807,7 +952,7 @@ async fn list_all_tools_applies_legacy_mcp_prefix_by_default() { } #[tokio::test] -async fn list_all_tools_blocks_while_client_is_pending_without_startup_snapshot() { +async fn list_all_tools_blocks_while_client_is_pending_without_cached_tool_info_snapshot() { let pending_client = futures::future::pending::>() .boxed() .shared(); @@ -822,7 +967,8 @@ async fn list_all_tools_blocks_while_client_is_pending_without_startup_snapshot( CODEX_APPS_MCP_SERVER_NAME.to_string(), AsyncManagedClient { client: pending_client, - startup_snapshot: None, + cached_tool_info_snapshot: None, + cached_server_info: None, startup_complete: Arc::new(std::sync::atomic::AtomicBool::new(false)), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), cancel_token: CancellationToken::new(), @@ -835,7 +981,7 @@ async fn list_all_tools_blocks_while_client_is_pending_without_startup_snapshot( } #[tokio::test] -async fn list_all_tools_does_not_block_when_startup_snapshot_cache_hit_is_empty() { +async fn list_all_tools_does_not_block_when_cached_tool_info_snapshot_is_empty() { let pending_client = futures::future::pending::>() .boxed() .shared(); @@ -850,7 +996,8 @@ async fn list_all_tools_does_not_block_when_startup_snapshot_cache_hit_is_empty( CODEX_APPS_MCP_SERVER_NAME.to_string(), AsyncManagedClient { client: pending_client, - startup_snapshot: Some(Vec::new()), + cached_tool_info_snapshot: Some(Vec::new()), + cached_server_info: None, startup_complete: Arc::new(std::sync::atomic::AtomicBool::new(false)), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), cancel_token: CancellationToken::new(), @@ -864,11 +1011,12 @@ async fn list_all_tools_does_not_block_when_startup_snapshot_cache_hit_is_empty( } #[tokio::test] -async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { +async fn list_all_tools_uses_cached_tool_info_snapshot_when_client_startup_fails() { let startup_tools = vec![create_test_tool( CODEX_APPS_MCP_SERVER_NAME, "calendar_create_event", )]; + let server_info = create_test_server_info("Codex Apps"); let failed_client = futures::future::ready::>(Err( StartupOutcomeError::Failed { error: "startup failed".to_string(), @@ -888,7 +1036,8 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { CODEX_APPS_MCP_SERVER_NAME.to_string(), AsyncManagedClient { client: failed_client, - startup_snapshot: Some(startup_tools), + cached_tool_info_snapshot: Some(startup_tools), + cached_server_info: Some(server_info.clone()), startup_complete, tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), cancel_token: CancellationToken::new(), @@ -905,6 +1054,13 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); assert_eq!(tool.callable_name, "calendar_create_event"); + assert_eq!( + manager + .list_available_server_infos() + .await + .get(CODEX_APPS_MCP_SERVER_NAME), + Some(&server_info) + ); } #[tokio::test] @@ -935,7 +1091,8 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() { server_name.to_string(), AsyncManagedClient { client: pending_client, - startup_snapshot: Some(startup_tools), + cached_tool_info_snapshot: Some(startup_tools), + cached_server_info: None, startup_complete: Arc::new(std::sync::atomic::AtomicBool::new(false)), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), cancel_token: CancellationToken::new(), diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index d6b496ab08..51a6f18686 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -25,6 +25,7 @@ use codex_config::types::ApprovalsReviewer; use codex_config::types::OAuthCredentialsStoreMode; use codex_login::CodexAuth; use codex_plugin::PluginCapabilitySummary; +use codex_protocol::mcp::McpServerInfo; use codex_protocol::mcp::Resource; use codex_protocol::mcp::ResourceTemplate; use codex_protocol::mcp::Tool; @@ -308,6 +309,7 @@ pub async fn read_mcp_resource( #[derive(Debug, Clone)] pub struct McpServerStatusSnapshot { + pub server_infos: HashMap, pub tools_by_server: HashMap>, pub resources: HashMap>, pub resource_templates: HashMap>, @@ -327,6 +329,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( let tool_plugin_provenance = tool_plugin_provenance(config); if mcp_servers.is_empty() { return McpServerStatusSnapshot { + server_infos: HashMap::new(), tools_by_server: HashMap::new(), resources: HashMap::new(), resource_templates: HashMap::new(), @@ -599,6 +602,7 @@ async fn collect_mcp_server_status_snapshot_from_manager( } }, ); + let server_infos = mcp_connection_manager.list_available_server_infos().await; let mut tools_by_server = HashMap::>::new(); for tool_info in tools { @@ -614,6 +618,7 @@ async fn collect_mcp_server_status_snapshot_from_manager( } McpServerStatusSnapshot { + server_infos, tools_by_server, resources: convert_mcp_resources(resources), resource_templates: convert_mcp_resource_templates(resource_templates), diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index e268720aff..78078720e7 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -20,6 +20,7 @@ use crate::codex_apps::CachedCodexAppsToolsLoad; use crate::codex_apps::CodexAppsToolsCacheContext; use crate::codex_apps::filter_disallowed_codex_apps_tools; use crate::codex_apps::load_cached_codex_apps_tools; +use crate::codex_apps::load_startup_cached_codex_apps_server_info; use crate::codex_apps::load_startup_cached_codex_apps_tools_snapshot; use crate::codex_apps::normalize_codex_apps_callable_name; use crate::codex_apps::normalize_codex_apps_callable_namespace; @@ -47,6 +48,7 @@ use codex_config::McpServerTransportConfig; use codex_config::types::OAuthCredentialsStoreMode; use codex_exec_server::HttpClient; use codex_exec_server::ReqwestHttpClient; +use codex_protocol::mcp::McpServerInfo; use codex_protocol::protocol::Event; use codex_rmcp_client::ExecutorStdioServerLauncher; use codex_rmcp_client::LocalStdioServerLauncher; @@ -85,6 +87,7 @@ const UNTRUSTED_CONNECTOR_META_KEYS: &[&str] = &[ #[derive(Clone)] pub(crate) struct ManagedClient { pub(crate) client: Arc, + pub(crate) server_info: McpServerInfo, pub(crate) tools: Vec, pub(crate) tool_filter: ToolFilter, pub(crate) tool_timeout: Option, @@ -123,7 +126,8 @@ impl ManagedClient { #[derive(Clone)] pub(crate) struct AsyncManagedClient { pub(crate) client: Shared>>, - pub(crate) startup_snapshot: Option>, + pub(crate) cached_tool_info_snapshot: Option>, + pub(crate) cached_server_info: Option, pub(crate) startup_complete: Arc, pub(crate) tool_plugin_provenance: Arc, pub(crate) cancel_token: CancellationToken, @@ -150,11 +154,16 @@ impl AsyncManagedClient { .configured_config() .map(ToolFilter::from_config) .unwrap_or_default(); - let startup_snapshot = load_startup_cached_codex_apps_tools_snapshot( + let cached_tool_info_snapshot = load_startup_cached_codex_apps_tools_snapshot( &server_name, codex_apps_tools_cache_context.as_ref(), - ) - .map(|tools| filter_tools(tools, &tool_filter)); + ); + let cached_tool_info_snapshot = + cached_tool_info_snapshot.map(|tools| filter_tools(tools, &tool_filter)); + let cached_server_info = load_startup_cached_codex_apps_server_info( + &server_name, + codex_apps_tools_cache_context.as_ref(), + ); let startup_tool_filter = tool_filter; let startup_complete = Arc::new(AtomicBool::new(false)); let startup_complete_for_fut = Arc::clone(&startup_complete); @@ -207,7 +216,7 @@ impl AsyncManagedClient { outcome }; let client = fut.boxed().shared(); - if startup_snapshot.is_some() { + if cached_tool_info_snapshot.is_some() { let startup_task = client.clone(); tokio::spawn(async move { let _ = startup_task.await; @@ -216,7 +225,8 @@ impl AsyncManagedClient { Self { client, - startup_snapshot, + cached_tool_info_snapshot, + cached_server_info, startup_complete, tool_plugin_provenance, cancel_token, @@ -238,9 +248,9 @@ impl AsyncManagedClient { } } - fn startup_snapshot_while_initializing(&self) -> Option> { + fn cached_tool_info_snapshot_while_initializing(&self) -> Option> { if !self.startup_complete.load(Ordering::Acquire) { - return self.startup_snapshot.clone(); + return self.cached_tool_info_snapshot.clone(); } None } @@ -298,12 +308,13 @@ impl AsyncManagedClient { }; // Keep cache payloads raw; plugin provenance is resolved per-session at read time. - let tools = if let Some(startup_tools) = self.startup_snapshot_while_initializing() { + let tools = if let Some(startup_tools) = self.cached_tool_info_snapshot_while_initializing() + { Some(startup_tools) } else { match self.client().await { Ok(client) => Some(client.listed_tools()), - Err(_) => self.startup_snapshot.clone(), + Err(_) => self.cached_tool_info_snapshot.clone(), } }; tools.map(annotate_tools) @@ -506,9 +517,11 @@ async fn start_server_task( fetch_start.elapsed(), &[], ); + let server_info = mcp_server_info_from_implementation(initialize_result.server_info); write_cached_codex_apps_tools_if_needed( &server_name, codex_apps_tools_cache_context.as_ref(), + &server_info, &tools, ); if server_name == CODEX_APPS_MCP_SERVER_NAME { @@ -522,6 +535,7 @@ async fn start_server_task( let managed = ManagedClient { client: Arc::clone(&client), + server_info, tools, tool_timeout: Some(tool_timeout), tool_filter, @@ -533,6 +547,22 @@ async fn start_server_task( Ok(managed) } +fn mcp_server_info_from_implementation(server_info: Implementation) -> McpServerInfo { + McpServerInfo { + name: server_info.name, + title: server_info.title, + version: server_info.version, + description: server_info.description, + icons: server_info.icons.map(|icons| { + icons + .into_iter() + .filter_map(|icon| serde_json::to_value(icon).ok()) + .collect() + }), + website_url: server_info.website_url, + } +} + struct StartServerTaskParams { startup_timeout: Option, // TODO: cancel_token should handle this. tool_timeout: Duration, diff --git a/codex-rs/protocol/src/mcp.rs b/codex-rs/protocol/src/mcp.rs index f6e69743b9..a1916d424c 100644 --- a/codex-rs/protocol/src/mcp.rs +++ b/codex-rs/protocol/src/mcp.rs @@ -26,6 +26,18 @@ impl std::fmt::Display for RequestId { } } +/// Presentation metadata advertised by an initialized MCP server. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +pub struct McpServerInfo { + pub name: String, + pub title: Option, + pub version: String, + pub description: Option, + pub icons: Option>, + pub website_url: Option, +} + /// Definition for a tool the client can call. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema, TS)] #[serde(rename_all = "camelCase")] diff --git a/codex-rs/tui/src/app/background_requests.rs b/codex-rs/tui/src/app/background_requests.rs index b3e1edb901..d90f511d6f 100644 --- a/codex-rs/tui/src/app/background_requests.rs +++ b/codex-rs/tui/src/app/background_requests.rs @@ -1061,6 +1061,7 @@ mod tests { let statuses = vec![ McpServerStatus { name: "docs".to_string(), + server_info: None, tools: HashMap::from([( "list".to_string(), Tool { @@ -1080,6 +1081,7 @@ mod tests { }, McpServerStatus { name: "disabled".to_string(), + server_info: None, tools: HashMap::new(), resources: Vec::new(), resource_templates: Vec::new(), diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index f4c8a4df79..d0c39b91ca 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -147,6 +147,7 @@ async fn handle_mcp_inventory_result_respects_origin_thread() { app.handle_mcp_inventory_result( Ok(vec![McpServerStatus { name: "docs".to_string(), + server_info: None, tools: HashMap::new(), resources: Vec::new(), resource_templates: Vec::new(), diff --git a/codex-rs/tui/src/history_cell/tests.rs b/codex-rs/tui/src/history_cell/tests.rs index b4674cec8a..76fe17f027 100644 --- a/codex-rs/tui/src/history_cell/tests.rs +++ b/codex-rs/tui/src/history_cell/tests.rs @@ -843,6 +843,7 @@ async fn mcp_tools_output_lists_tools_for_hyphenated_server_names() { fn mcp_tools_output_from_statuses_renders_status_only_servers() { let statuses = vec![McpServerStatus { name: "plugin_docs".to_string(), + server_info: None, tools: HashMap::from([( "lookup".to_string(), Tool { @@ -872,6 +873,7 @@ fn mcp_tools_output_from_statuses_renders_status_only_servers() { fn mcp_tools_output_from_statuses_renders_verbose_inventory() { let statuses = vec![McpServerStatus { name: "plugin_docs".to_string(), + server_info: None, tools: HashMap::from([( "lookup".to_string(), Tool { From e2551a5e362eb1538e21a47c8f7babf124f6d029 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 28 May 2026 18:48:43 +0200 Subject: [PATCH 2/4] Reap stale multi-agent slots (#24903) ## Summary - Let `close_agent` clean up an agent that is still registered in `AgentRegistry` even when its underlying thread is already missing. - Preserve the explicit-close boundary: for known stale thread-spawn agents, mark the persisted spawn edge `Closed`, then treat `ThreadNotFound` / `InternalAgentDied` as a successful close so the registry slot can be released. - Add a regression for MultiAgentV2 task-name targets where `close_agent("worker")` succeeds after the worker thread has already disappeared. ## Motivation A worker can disappear from `ThreadManager` while its metadata still exists in the root `AgentRegistry`. Before this change, the close tool failed while trying to subscribe to the missing thread status, so it never reached the cleanup path that releases the registered agent slot. With `agents.max_threads = 1`, an explicit close of that stale task-name agent could fail and leave the session unable to spawn a replacement. ## Scope This PR intentionally does not add automatic stale-agent reaping to `spawn_agent`, `resume_agent`, or `list_agents`. A thread being missing from `ThreadManager` is not the same as an explicit close: persisted open spawn edges are still the durable source of truth for resume and task-name ownership until `close_agent` is called. ## Validation - `just test -p codex-core -E 'test(multi_agent_v2_close_agent_reaps_stale_task_name_target) | test(resume_agent_from_rollout_reopens_open_descendants_after_manager_shutdown)'` - `just fix -p codex-core` --- codex-rs/core/src/agent/control.rs | 46 +++++-- .../handlers/multi_agents/close_agent.rs | 12 +- .../src/tools/handlers/multi_agents_tests.rs | 121 ++++++++++++++++++ .../handlers/multi_agents_v2/close_agent.rs | 12 +- 4 files changed, 173 insertions(+), 18 deletions(-) diff --git a/codex-rs/core/src/agent/control.rs b/codex-rs/core/src/agent/control.rs index 61ef994acd..1091761f79 100644 --- a/codex-rs/core/src/agent/control.rs +++ b/codex-rs/core/src/agent/control.rs @@ -772,15 +772,45 @@ impl AgentControl { /// agent and any live descendants reached from the in-memory tree. pub(crate) async fn close_agent(&self, agent_id: ThreadId) -> CodexResult { let state = self.upgrade()?; - if let Ok(thread) = state.get_thread(agent_id).await - && let Some(state_db_ctx) = thread.state_db() - && let Err(err) = state_db_ctx - .set_thread_spawn_edge_status(agent_id, DirectionalThreadSpawnEdgeStatus::Closed) - .await - { - warn!("failed to persist thread-spawn edge status for {agent_id}: {err}"); + let known_agent = self.state.agent_metadata_for_thread(agent_id).is_some(); + match state.get_thread(agent_id).await { + Ok(thread) => { + if let Some(state_db_ctx) = thread.state_db() + && let Err(err) = state_db_ctx + .set_thread_spawn_edge_status( + agent_id, + DirectionalThreadSpawnEdgeStatus::Closed, + ) + .await + { + warn!("failed to persist thread-spawn edge status for {agent_id}: {err}"); + } + } + Err(CodexErr::ThreadNotFound(_)) if known_agent => { + if let Some(state_db_ctx) = state.state_db() + && let Err(err) = state_db_ctx + .set_thread_spawn_edge_status( + agent_id, + DirectionalThreadSpawnEdgeStatus::Closed, + ) + .await + { + return Err(CodexErr::Fatal(format!( + "failed to persist stale thread-spawn edge status for {agent_id}: {err}" + ))); + } + } + Err(CodexErr::ThreadNotFound(_)) => {} + Err(err) => { + warn!("failed to inspect agent before close {agent_id}: {err}"); + } + } + match Box::pin(self.shutdown_agent_tree(agent_id)).await { + Err(CodexErr::ThreadNotFound(_)) | Err(CodexErr::InternalAgentDied) if known_agent => { + Ok(String::new()) + } + result => result, } - Box::pin(self.shutdown_agent_tree(agent_id)).await } /// Shut down `agent_id` and any live descendants reachable from the in-memory spawn tree. diff --git a/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs b/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs index de069854f1..bcfa5231b9 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs @@ -1,6 +1,7 @@ use super::*; use crate::tools::handlers::multi_agents_spec::create_close_agent_tool_v1; use crate::turn_timing::now_unix_timestamp_ms; +use codex_protocol::error::CodexErr; use codex_tools::ToolSpec; pub(crate) struct Handler; @@ -36,11 +37,9 @@ async fn handle_close_agent( let arguments = function_arguments(payload)?; let args: CloseAgentArgs = parse_arguments(&arguments)?; let agent_id = parse_agent_id_target(&args.target)?; - let receiver_agent = session - .services - .agent_control - .get_agent_metadata(agent_id) - .unwrap_or_default(); + let receiver_agent = session.services.agent_control.get_agent_metadata(agent_id); + let known_agent = receiver_agent.is_some(); + let receiver_agent = receiver_agent.unwrap_or_default(); session .send_event( &turn, @@ -60,6 +59,9 @@ async fn handle_close_agent( .await { Ok(mut status_rx) => status_rx.borrow_and_update().clone(), + Err(CodexErr::ThreadNotFound(_)) if known_agent => { + session.services.agent_control.get_status(agent_id).await + } Err(err) => { let status = session.services.agent_control.get_status(agent_id).await; session diff --git a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs index 68b19adea9..164d5410e0 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs @@ -52,6 +52,7 @@ use codex_protocol::protocol::TurnAbortReason; use codex_protocol::protocol::TurnAbortedEvent; use codex_protocol::protocol::TurnCompleteEvent; use codex_protocol::user_input::UserInput; +use codex_state::DirectionalThreadSpawnEdgeStatus; use core_test_support::TempDirExt; use pretty_assertions::assert_eq; use serde::Deserialize; @@ -3751,6 +3752,126 @@ async fn multi_agent_v2_close_agent_accepts_task_name_target() { ); } +#[tokio::test] +async fn multi_agent_v2_close_agent_reaps_stale_task_name_target() { + let (mut session, mut turn) = make_session_and_context().await; + let mut config = (*turn.config).clone(); + config.agent_max_threads = Some(1); + config + .features + .enable(Feature::MultiAgentV2) + .expect("test config should allow feature update"); + config + .features + .enable(Feature::Sqlite) + .expect("test config should allow sqlite"); + let state_db = init_state_db(&config) + .await + .expect("sqlite state db should initialize"); + let manager = ThreadManager::with_models_provider_home_and_state_for_tests( + CodexAuth::from_api_key("dummy"), + config.model_provider.clone(), + config.codex_home.to_path_buf(), + Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()), + Some(state_db.clone()), + ); + let root = manager + .start_thread(config.clone()) + .await + .expect("root thread should start"); + session.services.agent_control = manager.agent_control(); + session.conversation_id = root.thread_id; + turn.config = Arc::new(config.clone()); + + let session = Arc::new(session); + let turn = Arc::new(turn); + SpawnAgentHandlerV2::default() + .handle(invocation( + session.clone(), + turn.clone(), + "spawn_agent", + function_payload(json!({ + "message": "inspect this repo", + "task_name": "worker" + })), + )) + .await + .expect("spawn_agent should succeed"); + + let agent_id = session + .services + .agent_control + .resolve_agent_reference(session.conversation_id, &turn.session_source, "worker") + .await + .expect("worker path should resolve"); + let stale_thread = manager + .remove_thread(&agent_id) + .await + .expect("worker thread should be loaded before removal"); + stale_thread + .submit(Op::Shutdown {}) + .await + .expect("removed worker thread should still accept shutdown"); + stale_thread.wait_until_terminated().await; + + let output = CloseAgentHandlerV2 + .handle(invocation( + session.clone(), + turn.clone(), + "close_agent", + function_payload(json!({"target": "worker"})), + )) + .await + .expect("close_agent should reap stale v2 task names"); + let (content, success) = expect_text_output(output); + let result: close_agent::CloseAgentResult = + serde_json::from_str(&content).expect("close_agent result should be json"); + assert_eq!(result.previous_status, AgentStatus::NotFound); + assert_eq!(success, Some(true)); + + let open_children = state_db + .list_thread_spawn_children_with_status( + root.thread_id, + DirectionalThreadSpawnEdgeStatus::Open, + ) + .await + .expect("open children should load"); + assert_eq!(open_children, Vec::::new()); + let closed_children = state_db + .list_thread_spawn_children_with_status( + root.thread_id, + DirectionalThreadSpawnEdgeStatus::Closed, + ) + .await + .expect("closed children should load"); + assert_eq!(closed_children, vec![agent_id]); + + SpawnAgentHandlerV2::default() + .handle(invocation( + session.clone(), + turn.clone(), + "spawn_agent", + function_payload(json!({ + "message": "inspect this repo again", + "task_name": "replacement" + })), + )) + .await + .expect("spawn_agent should succeed after stale close releases the slot"); + let replacement_id = session + .services + .agent_control + .resolve_agent_reference(session.conversation_id, &turn.session_source, "replacement") + .await + .expect("replacement path should resolve"); + let _ = session + .services + .agent_control + .shutdown_live_agent(replacement_id) + .await + .expect("replacement should shut down"); +} + #[tokio::test] async fn multi_agent_v2_close_agent_rejects_root_target_and_id() { let (mut session, mut turn) = make_session_and_context().await; diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/close_agent.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/close_agent.rs index 088e531062..15502572a5 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/close_agent.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/close_agent.rs @@ -1,6 +1,7 @@ use super::*; use crate::tools::handlers::multi_agents_spec::create_close_agent_tool_v2; use crate::turn_timing::now_unix_timestamp_ms; +use codex_protocol::error::CodexErr; use codex_tools::ToolSpec; pub(crate) struct Handler; @@ -36,11 +37,9 @@ async fn handle_close_agent( let arguments = function_arguments(payload)?; let args: CloseAgentArgs = parse_arguments(&arguments)?; let agent_id = resolve_agent_target(&session, &turn, &args.target).await?; - let receiver_agent = session - .services - .agent_control - .get_agent_metadata(agent_id) - .unwrap_or_default(); + let receiver_agent = session.services.agent_control.get_agent_metadata(agent_id); + let known_agent = receiver_agent.is_some(); + let receiver_agent = receiver_agent.unwrap_or_default(); if receiver_agent .agent_path .as_ref() @@ -69,6 +68,9 @@ async fn handle_close_agent( .await { Ok(mut status_rx) => status_rx.borrow_and_update().clone(), + Err(CodexErr::ThreadNotFound(_)) if known_agent => { + session.services.agent_control.get_status(agent_id).await + } Err(err) => { let status = session.services.agent_control.get_status(agent_id).await; session From 2264fdd4a2bb517c1173ee2a5936337661d54536 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 28 May 2026 09:59:34 -0700 Subject: [PATCH 3/4] Fix extension turn item emitter test event ordering (#24936) ## Why PR #24813 added extension `TurnItemEmitter` coverage and introduced a test that records a conversation history item before asserting extension-emitted turn item events. `record_conversation_items()` also emits a `RawResponseItem` event to observers. The test was reading from the same event receiver and expected the next event to be `ItemStarted`, so the test failed reliably once the setup history item was present. ## What Changed Update `passes_turn_fields_and_scoped_turn_item_emitter_to_extension_call` to consume and assert the expected setup `RawResponseItem` before checking the extension `ItemStarted`, `WebSearchBegin`, `ItemCompleted`, and `WebSearchEnd` events. This is test-only and does not change extension runtime behavior. ## Verification - `cargo nextest run --no-fail-fast -p codex-core tools::handlers::extension_tools::tests::passes_turn_fields_and_scoped_turn_item_emitter_to_extension_call` --- codex-rs/core/src/tools/handlers/extension_tools.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/codex-rs/core/src/tools/handlers/extension_tools.rs b/codex-rs/core/src/tools/handlers/extension_tools.rs index e9b16e97d5..d04a6c520e 100644 --- a/codex-rs/core/src/tools/handlers/extension_tools.rs +++ b/codex-rs/core/src/tools/handlers/extension_tools.rs @@ -271,6 +271,11 @@ mod tests { session .record_conversation_items(&turn, std::slice::from_ref(&history_item)) .await; + let raw_history_event = rx.recv().await.expect("history raw response item event"); + let EventMsg::RawResponseItem(raw_history_item) = raw_history_event.msg else { + panic!("expected raw response item event"); + }; + assert_eq!(raw_history_item.item, history_item); let invocation = ToolInvocation { session, turn, From c341c6ac426f4eedad8d141318dd0e6fd2c85ea4 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 28 May 2026 09:59:40 -0700 Subject: [PATCH 4/4] fix: surface denied filesystem reads in prompts Expose denied-read entries from the active permission profile to model-visible context and automatic approval review prompts so escalation decisions can account for policy-restricted paths. --- .../core/src/context/environment_context.rs | 91 ++++++++++++++++++- .../src/context/environment_context_tests.rs | 30 ++++++ .../src/context/permissions_instructions.rs | 78 ++++++++++++---- .../context/permissions_instructions_tests.rs | 45 +++++++++ codex-rs/core/src/guardian/mod.rs | 2 + codex-rs/core/src/guardian/prompt.rs | 49 ++++++++++ codex-rs/core/src/guardian/review_session.rs | 5 +- codex-rs/core/src/guardian/tests.rs | 62 +++++++++++++ 8 files changed, 336 insertions(+), 26 deletions(-) diff --git a/codex-rs/core/src/context/environment_context.rs b/codex-rs/core/src/context/environment_context.rs index a3052b6642..4077944c98 100644 --- a/codex-rs/core/src/context/environment_context.rs +++ b/codex-rs/core/src/context/environment_context.rs @@ -1,9 +1,11 @@ use crate::session::turn_context::TurnContext; use crate::session::turn_context::TurnEnvironment; use crate::shell::Shell; +use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::TurnContextItem; use codex_protocol::protocol::TurnContextNetworkItem; use codex_utils_absolute_path::AbsolutePathBuf; +use std::path::Path; use super::ContextualUserFragment; @@ -13,6 +15,7 @@ pub(crate) struct EnvironmentContext { pub(crate) current_date: Option, pub(crate) timezone: Option, pub(crate) network: Option, + pub(crate) filesystem: Option, pub(crate) subagents: Option, } @@ -83,6 +86,55 @@ impl EnvironmentContextEnvironments { } } +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub(crate) struct FileSystemContext { + unreadable_roots: Vec, + unreadable_globs: Vec, +} + +impl FileSystemContext { + fn new(unreadable_roots: Vec, unreadable_globs: Vec) -> Option { + if unreadable_roots.is_empty() && unreadable_globs.is_empty() { + None + } else { + Some(Self { + unreadable_roots, + unreadable_globs, + }) + } + } + + fn from_permission_profile(permission_profile: &PermissionProfile, cwd: &Path) -> Option { + let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy(); + Self::new( + file_system_sandbox_policy + .get_unreadable_roots_with_cwd(cwd) + .into_iter() + .map(|root| root.to_string_lossy().into_owned()) + .collect(), + file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd), + ) + } + + fn render(&self) -> String { + let mut rendered = "".to_string(); + Self::push_rendered_element(&mut rendered, "paths", &self.unreadable_roots); + Self::push_rendered_element(&mut rendered, "globs", &self.unreadable_globs); + rendered.push_str(""); + rendered + } + + fn push_rendered_element(rendered_filesystem: &mut String, name: &str, values: &[String]) { + if values.is_empty() { + return; + } + + rendered_filesystem.push_str(&format!("<{name}>")); + rendered_filesystem.push_str(&values.join(",")); + rendered_filesystem.push_str(&format!("")); + } +} + #[derive(Debug, Clone, PartialEq, Eq, Default)] pub(crate) struct NetworkContext { allowed_domains: Vec, @@ -129,6 +181,7 @@ impl EnvironmentContext { current_date, timezone, network, + filesystem: None, subagents, } } @@ -138,6 +191,7 @@ impl EnvironmentContext { current_date: Option, timezone: Option, network: Option, + filesystem: Option, subagents: Option, ) -> Self { Self { @@ -145,6 +199,7 @@ impl EnvironmentContext { current_date, timezone, network, + filesystem, subagents, } } @@ -157,6 +212,7 @@ impl EnvironmentContext { && self.current_date == other.current_date && self.timezone == other.timezone && self.network == other.network + && self.filesystem == other.filesystem && self.subagents == other.subagents } @@ -165,6 +221,7 @@ impl EnvironmentContext { after: &EnvironmentContext, ) -> Self { let before_network = Self::network_from_turn_context_item(before); + let before_filesystem = Self::filesystem_from_turn_context_item(before); let environments = match &after.environments { EnvironmentContextEnvironments::Single(environment) => { if before.cwd.as_path() != environment.cwd.as_path() { @@ -186,17 +243,25 @@ impl EnvironmentContext { } else { before_network }; + let filesystem = if before_filesystem != after.filesystem { + after.filesystem.clone() + } else { + before_filesystem + }; EnvironmentContext::new_with_environments( environments, after.current_date.clone(), after.timezone.clone(), network, + filesystem, /*subagents*/ None, ) } pub(crate) fn from_turn_context(turn_context: &TurnContext, shell: &Shell) -> Self { - Self::new( + #[allow(deprecated)] + let cwd = &turn_context.cwd; + let mut context = Self::new( EnvironmentContextEnvironment::from_turn_environments( &turn_context.environments.turn_environments, shell, @@ -205,7 +270,10 @@ impl EnvironmentContext { turn_context.timezone.clone(), Self::network_from_turn_context(turn_context), /*subagents*/ None, - ) + ); + context.filesystem = + FileSystemContext::from_permission_profile(&turn_context.permission_profile, cwd); + context } pub(crate) fn from_turn_context_item( @@ -216,11 +284,14 @@ impl EnvironmentContext { Ok(cwd) => cwd, Err(_) => AbsolutePathBuf::resolve_path_against_base(&turn_context_item.cwd, "/"), }; - Self::new( - vec![EnvironmentContextEnvironment::legacy(cwd, shell)], + Self::new_with_environments( + EnvironmentContextEnvironments::from_vec(vec![EnvironmentContextEnvironment::legacy( + cwd, shell, + )]), turn_context_item.current_date.clone(), turn_context_item.timezone.clone(), Self::network_from_turn_context_item(turn_context_item), + Self::filesystem_from_turn_context_item(turn_context_item), /*subagents*/ None, ) } @@ -266,6 +337,15 @@ impl EnvironmentContext { denied_domains.clone(), )) } + + fn filesystem_from_turn_context_item( + turn_context_item: &TurnContextItem, + ) -> Option { + FileSystemContext::from_permission_profile( + &turn_context_item.permission_profile(), + &turn_context_item.cwd, + ) + } } impl ContextualUserFragment for EnvironmentContext { @@ -324,6 +404,9 @@ impl ContextualUserFragment for EnvironmentContext { // lines.push(" ".to_string()); } } + if let Some(filesystem) = &self.filesystem { + lines.push(format!(" {}", filesystem.render())); + } if let Some(subagents) = &self.subagents { lines.push(" ".to_string()); lines.extend(subagents.lines().map(|line| format!(" {line}"))); diff --git a/codex-rs/core/src/context/environment_context_tests.rs b/codex-rs/core/src/context/environment_context_tests.rs index 68ff7c9d44..2d0855b326 100644 --- a/codex-rs/core/src/context/environment_context_tests.rs +++ b/codex-rs/core/src/context/environment_context_tests.rs @@ -79,6 +79,36 @@ fn serialize_environment_context_with_network() { assert_eq!(context.render(), expected); } +#[test] +fn serialize_environment_context_with_filesystem_denied_reads() { + let mut context = EnvironmentContext::new( + vec![EnvironmentContextEnvironment { + id: "local".to_string(), + cwd: test_path_buf("/repo").abs(), + shell: fake_shell_name(), + }], + /*current_date*/ None, + /*timezone*/ None, + /*network*/ None, + /*subagents*/ None, + ); + context.filesystem = FileSystemContext::new( + vec!["/repo/private".to_string()], + vec!["/repo/private/**".to_string()], + ); + + let expected = format!( + r#" + {} + bash + /repo/private/repo/private/** +"#, + test_path_buf("/repo").display() + ); + + assert_eq!(context.render(), expected); +} + #[test] fn serialize_read_only_environment_context() { let context = EnvironmentContext::new( diff --git a/codex-rs/core/src/context/permissions_instructions.rs b/codex-rs/core/src/context/permissions_instructions.rs index db8f983c16..e87dceec01 100644 --- a/codex-rs/core/src/context/permissions_instructions.rs +++ b/codex-rs/core/src/context/permissions_instructions.rs @@ -4,6 +4,7 @@ use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::SandboxMode; use codex_protocol::models::PermissionProfile; use codex_protocol::models::format_allow_prefixes; +use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::GranularApprovalConfig; @@ -68,9 +69,11 @@ impl PermissionsInstructions { exec_permission_approvals_enabled: bool, request_permissions_tool_enabled: bool, ) -> Self { - let (sandbox_mode, writable_roots) = sandbox_prompt_from_profile(permission_profile, cwd); + let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy(); + let (sandbox_mode, writable_roots) = + sandbox_prompt_from_policy(&file_system_sandbox_policy, cwd); - Self::from_permissions_with_network( + Self::from_permissions_with_network_and_denied_reads( sandbox_mode, network_access_from_policy(permission_profile.network_sandbox_policy()), PermissionsPromptConfig { @@ -81,14 +84,32 @@ impl PermissionsInstructions { request_permissions_tool_enabled, }, writable_roots, + denied_reads_text(&file_system_sandbox_policy, cwd), ) } + #[cfg(test)] fn from_permissions_with_network( sandbox_mode: SandboxMode, network_access: NetworkAccess, config: PermissionsPromptConfig<'_>, writable_roots: Option>, + ) -> Self { + Self::from_permissions_with_network_and_denied_reads( + sandbox_mode, + network_access, + config, + writable_roots, + /*denied_reads*/ None, + ) + } + + fn from_permissions_with_network_and_denied_reads( + sandbox_mode: SandboxMode, + network_access: NetworkAccess, + config: PermissionsPromptConfig<'_>, + writable_roots: Option>, + denied_reads: Option, ) -> Self { let mut text = String::new(); append_section(&mut text, &sandbox_text(sandbox_mode, network_access)); @@ -105,6 +126,9 @@ impl PermissionsInstructions { if let Some(writable_roots) = writable_roots_text(writable_roots) { append_section(&mut text, &writable_roots); } + if let Some(denied_reads) = denied_reads { + append_section(&mut text, &denied_reads); + } if !text.ends_with('\n') { text.push('\n'); } @@ -112,27 +136,19 @@ impl PermissionsInstructions { } } -fn sandbox_prompt_from_profile( - permission_profile: &PermissionProfile, +fn sandbox_prompt_from_policy( + file_system_policy: &FileSystemSandboxPolicy, cwd: &Path, ) -> (SandboxMode, Option>) { - match permission_profile { - PermissionProfile::Disabled | PermissionProfile::External { .. } => { - (SandboxMode::DangerFullAccess, None) - } - PermissionProfile::Managed { .. } => { - let file_system_policy = permission_profile.file_system_sandbox_policy(); - if file_system_policy.has_full_disk_write_access() { - return (SandboxMode::DangerFullAccess, None); - } + if file_system_policy.has_full_disk_write_access() { + return (SandboxMode::DangerFullAccess, None); + } - let writable_roots = file_system_policy.get_writable_roots_with_cwd(cwd); - if writable_roots.is_empty() { - (SandboxMode::ReadOnly, None) - } else { - (SandboxMode::WorkspaceWrite, Some(writable_roots)) - } - } + let writable_roots = file_system_policy.get_writable_roots_with_cwd(cwd); + if writable_roots.is_empty() { + (SandboxMode::ReadOnly, None) + } else { + (SandboxMode::WorkspaceWrite, Some(writable_roots)) } } @@ -254,6 +270,28 @@ fn writable_roots_text(writable_roots: Option>) -> Option Option { + let mut entries = file_system_policy + .get_unreadable_roots_with_cwd(cwd) + .into_iter() + .map(|root| format!("- path `{}`", root.to_string_lossy())) + .collect::>(); + entries.extend( + file_system_policy + .get_unreadable_globs_with_cwd(cwd) + .into_iter() + .map(|glob| format!("- glob `{glob}`")), + ); + if entries.is_empty() { + return None; + } + + Some(format!( + "## Denied filesystem reads\nThe active permission profile denies reading these paths/globs. Do not request escalation or additional permissions to read them; these denials are policy restrictions.\n{}", + entries.join("\n") + )) +} + fn approved_command_prefixes_text(exec_policy: &Policy) -> Option { format_allow_prefixes(exec_policy.get_allowed_prefixes()) .filter(|prefixes| !prefixes.is_empty()) diff --git a/codex-rs/core/src/context/permissions_instructions_tests.rs b/codex-rs/core/src/context/permissions_instructions_tests.rs index 6d1aa5d886..466b8a35d7 100644 --- a/codex-rs/core/src/context/permissions_instructions_tests.rs +++ b/codex-rs/core/src/context/permissions_instructions_tests.rs @@ -83,6 +83,51 @@ fn builds_permissions_from_profile() { assert!(text.contains(writable_root.to_string_lossy().as_ref())); } +#[test] +fn builds_permissions_from_profile_with_denied_reads() { + let cwd = PathBuf::from("/tmp"); + let denied_root = + AbsolutePathBuf::from_absolute_path(cwd.join("blocked")).expect("absolute path"); + let permission_profile = PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: denied_root.clone(), + }, + access: FileSystemAccessMode::Deny, + }, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "/tmp/blocked/**".to_string(), + }, + access: FileSystemAccessMode::Deny, + }, + ]), + NetworkSandboxPolicy::Restricted, + ); + + let instructions = PermissionsInstructions::from_permission_profile( + &permission_profile, + AskForApproval::OnRequest, + ApprovalsReviewer::AutoReview, + &Policy::empty(), + &cwd, + /*exec_permission_approvals_enabled*/ false, + /*request_permissions_tool_enabled*/ false, + ); + let text = instructions.body(); + assert!(text.contains("## Denied filesystem reads")); + assert!(text.contains("Do not request escalation or additional permissions")); + assert!(text.contains(denied_root.to_string_lossy().as_ref())); + assert!(text.contains("glob `/tmp/blocked/**`")); +} + #[test] fn includes_request_rule_instructions_for_on_request() { let mut exec_policy = Policy::empty(); diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index f5c6fe5231..8cc04e5c42 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -149,6 +149,8 @@ use prompt::GuardianTranscriptEntryKind; #[cfg(test)] use prompt::build_guardian_prompt_items; #[cfg(test)] +use prompt::build_guardian_prompt_items_with_parent_turn; +#[cfg(test)] use prompt::collect_guardian_transcript_entries; #[cfg(test)] use prompt::guardian_output_schema; diff --git a/codex-rs/core/src/guardian/prompt.rs b/codex-rs/core/src/guardian/prompt.rs index b1b132a984..062a21deef 100644 --- a/codex-rs/core/src/guardian/prompt.rs +++ b/codex-rs/core/src/guardian/prompt.rs @@ -10,6 +10,7 @@ use serde_json::Value; use crate::compact::content_items_to_text; use crate::event_mapping::is_contextual_user_message_content; use crate::session::session::Session; +use crate::session::turn_context::TurnContext; use codex_utils_output_truncation::approx_bytes_for_tokens; use codex_utils_output_truncation::approx_token_count; use codex_utils_output_truncation::approx_tokens_from_byte_count; @@ -86,11 +87,29 @@ pub(crate) enum GuardianPromptMode { /// Split the variable request into separate user content items so the /// Responses request snapshot shows clear boundaries while preserving exact /// prompt text through trailing newlines. +#[cfg(test)] pub(crate) async fn build_guardian_prompt_items( session: &Session, retry_reason: Option, request: GuardianApprovalRequest, mode: GuardianPromptMode, +) -> serde_json::Result { + build_guardian_prompt_items_with_parent_turn( + session, + /*parent_turn*/ None, + retry_reason, + request, + mode, + ) + .await +} + +pub(crate) async fn build_guardian_prompt_items_with_parent_turn( + session: &Session, + parent_turn: Option<&TurnContext>, + retry_reason: Option, + request: GuardianApprovalRequest, + mode: GuardianPromptMode, ) -> serde_json::Result { let history = session.clone_history().await; let transcript_entries = collect_guardian_transcript_entries(history.raw_items()); @@ -172,6 +191,11 @@ pub(crate) async fn build_guardian_prompt_items( if let Some(note) = omission_note { push_text(format!("\n{note}\n")); } + if let Some(denied_reads_context) = parent_turn.and_then(parent_turn_denied_reads_context) { + push_text("\n>>> PARENT TURN PERMISSION CONTEXT START\n".to_string()); + push_text(denied_reads_context); + push_text(">>> PARENT TURN PERMISSION CONTEXT END\n".to_string()); + } match &request { GuardianApprovalRequest::NetworkAccess { trigger, .. } => { push_text(">>> APPROVAL REQUEST START\n".to_string()); @@ -216,6 +240,31 @@ pub(crate) async fn build_guardian_prompt_items( }) } +fn parent_turn_denied_reads_context(turn: &TurnContext) -> Option { + #[allow(deprecated)] + let cwd = &turn.cwd; + let file_system_policy = turn.permission_profile.file_system_sandbox_policy(); + let mut entries = file_system_policy + .get_unreadable_roots_with_cwd(cwd) + .into_iter() + .map(|root| format!("- path `{}`", root.to_string_lossy())) + .collect::>(); + entries.extend( + file_system_policy + .get_unreadable_globs_with_cwd(cwd) + .into_iter() + .map(|glob| format!("- glob `{glob}`")), + ); + if entries.is_empty() { + return None; + } + + Some(format!( + "The parent turn's active permission profile denies reading these paths/globs. These are policy restrictions; do not approve escalation whose purpose is to read them.\n{}\n", + entries.join("\n") + )) +} + enum GuardianPromptShape { Full, Delta { already_seen_entry_count: usize }, diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index f52792cb60..fed87cd75f 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -51,7 +51,7 @@ use super::GUARDIAN_REVIEWER_NAME; use super::GuardianApprovalRequest; use super::prompt::GuardianPromptMode; use super::prompt::GuardianTranscriptCursor; -use super::prompt::build_guardian_prompt_items; +use super::prompt::build_guardian_prompt_items_with_parent_turn; use super::prompt::guardian_policy_prompt; use super::prompt::guardian_policy_prompt_with_config; @@ -686,8 +686,9 @@ async fn run_review_on_session( ) .await; - build_guardian_prompt_items( + build_guardian_prompt_items_with_parent_turn( params.parent_session.as_ref(), + Some(params.parent_turn.as_ref()), params.retry_reason.clone(), params.request.clone(), prompt_mode, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 6df3b69f61..0bba01e869 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -33,6 +33,11 @@ use codex_protocol::models::ContentItem; use codex_protocol::models::PermissionProfile; use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::ReasoningEffort; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; @@ -357,6 +362,63 @@ async fn build_guardian_prompt_full_mode_preserves_initial_review_format() -> an Ok(()) } +#[tokio::test(flavor = "current_thread")] +async fn build_guardian_prompt_includes_parent_turn_denied_reads() -> anyhow::Result<()> { + let (mut session, mut turn) = crate::session::tests::make_session_and_context().await; + session.conversation_id = fixed_guardian_parent_session_id(); + let denied_root = test_path_buf("/repo/private").abs(); + turn.permission_profile = PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: denied_root.clone(), + }, + access: FileSystemAccessMode::Deny, + }, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "/repo/private/**".to_string(), + }, + access: FileSystemAccessMode::Deny, + }, + ]), + NetworkSandboxPolicy::Restricted, + ); + let session = Arc::new(session); + let turn = Arc::new(turn); + seed_guardian_parent_history(&session, &turn).await; + + let prompt = build_guardian_prompt_items_with_parent_turn( + session.as_ref(), + Some(turn.as_ref()), + Some("Sandbox denied reading /repo/private/secret.txt.".to_string()), + GuardianApprovalRequest::Shell { + id: "shell-1".to_string(), + command: vec!["cat".to_string(), "/repo/private/secret.txt".to_string()], + cwd: test_path_buf("/repo").abs(), + sandbox_permissions: crate::sandboxing::SandboxPermissions::RequireEscalated, + additional_permissions: None, + justification: Some("Need to inspect the secret file.".to_string()), + }, + GuardianPromptMode::Full, + ) + .await?; + + let text = guardian_prompt_text(&prompt.items); + assert!(text.contains("PARENT TURN PERMISSION CONTEXT START")); + assert!(text.contains("do not approve escalation whose purpose is to read them")); + assert!(text.contains(denied_root.to_string_lossy().as_ref())); + assert!(text.contains("glob `/repo/private/**`")); + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] async fn build_guardian_prompt_delta_mode_preserves_original_numbering() -> anyhow::Result<()> { let (session, turn) = guardian_test_session_and_turn_with_base_url("http://localhost").await;