From 2264fdd4a2bb517c1173ee2a5936337661d54536 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 28 May 2026 09:59:34 -0700 Subject: [PATCH 01/10] 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 577ec03bf82fb52e7041fb6b684e694b1e53451a Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Thu, 28 May 2026 10:24:03 -0700 Subject: [PATCH 02/10] [codex] Support ui visibility meta for tools (#24700) ## Summary Adds support for the same ui.visibility metadata as resources [spec](https://github.com/modelcontextprotocol/ext-apps/blob/main/specification/draft/apps.mdx#resource-discovery) --- .../app-server/tests/suite/v2/app_list.rs | 67 ++++++++++++++ codex-rs/codex-mcp/src/connection_manager.rs | 29 ++++++ codex-rs/codex-mcp/src/lib.rs | 1 + codex-rs/core/src/mcp_tool_exposure.rs | 8 +- codex-rs/core/src/mcp_tool_exposure_test.rs | 89 ++++++++++++++++++ .../core/tests/common/apps_test_server.rs | 58 +++++++++++- codex-rs/core/tests/suite/code_mode.rs | 90 +++++++++++++++++++ codex-rs/core/tests/suite/search_tool.rs | 78 ++++++++++++++++ 8 files changed, 418 insertions(+), 2 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/app_list.rs b/codex-rs/app-server/tests/suite/v2/app_list.rs index aa7b67a574..039fffe5cd 100644 --- a/codex-rs/app-server/tests/suite/v2/app_list.rs +++ b/codex-rs/app-server/tests/suite/v2/app_list.rs @@ -316,6 +316,73 @@ connectors = false Ok(()) } +#[tokio::test] +async fn list_apps_keeps_apps_with_app_only_tools_accessible() -> Result<()> { + let connectors = vec![AppInfo { + id: "beta".to_string(), + name: "Beta".to_string(), + description: Some("Beta connector".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: None, + is_accessible: false, + is_enabled: true, + plugin_display_names: Vec::new(), + }]; + let mut app_only_tool = connector_tool("beta", "Beta App")?; + app_only_tool + .meta + .as_mut() + .expect("connector tool should include metadata") + .0 + .insert("ui".to_string(), json!({ "visibility": ["app"] })); + let tools = vec![app_only_tool]; + let (server_url, server_handle) = + start_apps_server_with_delays(connectors, tools, Duration::ZERO, Duration::ZERO).await?; + + let codex_home = TempDir::new()?; + write_connectors_config(codex_home.path(), &server_url)?; + write_chatgpt_auth( + codex_home.path(), + ChatGptAuthFixture::new("chatgpt-token") + .account_id("account-123") + .chatgpt_user_id("user-app-only") + .chatgpt_account_id("account-123"), + AuthCredentialsStoreMode::File, + )?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_apps_list_request(AppsListParams { + limit: None, + cursor: None, + thread_id: None, + force_refetch: true, + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let AppsListResponse { data, next_cursor } = to_response(response)?; + + assert_eq!(data.len(), 1); + assert_eq!(data[0].id, "beta"); + assert!(data[0].is_accessible); + assert!(next_cursor.is_none()); + + server_handle.abort(); + let _ = server_handle.await; + Ok(()) +} + #[tokio::test] async fn list_apps_reports_is_enabled_from_config() -> Result<()> { let connectors = vec![AppInfo { diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index d4a35eab1e..d15a2e1500 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -64,6 +64,7 @@ use rmcp::model::ReadResourceResult; use rmcp::model::RequestId; use rmcp::model::Resource; use rmcp::model::ResourceTemplate; +use serde_json::Value as JsonValue; use tokio::task::JoinSet; use tokio_util::sync::CancellationToken; use tracing::Instrument; @@ -72,6 +73,34 @@ use tracing::trace; use tracing::trace_span; use tracing::warn; +const MCP_UI_META_KEY: &str = "ui"; +const MCP_UI_VISIBILITY_META_KEY: &str = "visibility"; +const MCP_UI_MODEL_VISIBILITY: &str = "model"; + +/// Returns whether a tool may be included in model-facing tool declarations. +/// +/// Tools without visibility metadata remain visible. +/// Tools with visibility metadata are hidden unless they explicitly include `model`. +/// +/// +pub fn tool_is_model_visible(tool: &ToolInfo) -> bool { + let Some(visibility) = tool + .tool + .meta + .as_deref() + .and_then(|meta| meta.get(MCP_UI_META_KEY)) + .and_then(JsonValue::as_object) + .and_then(|ui| ui.get(MCP_UI_VISIBILITY_META_KEY)) + .and_then(JsonValue::as_array) + else { + return true; + }; + + visibility + .iter() + .any(|target| target.as_str() == Some(MCP_UI_MODEL_VISIBILITY)) +} + /// A thin wrapper around a set of running [`RmcpClient`] instances. pub struct McpConnectionManager { clients: HashMap, diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 7b45bcc7a7..83419175d4 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -1,4 +1,5 @@ pub use connection_manager::McpConnectionManager; +pub use connection_manager::tool_is_model_visible; pub use elicitation::ElicitationReviewRequest; pub use elicitation::ElicitationReviewer; pub use elicitation::ElicitationReviewerHandle; diff --git a/codex-rs/core/src/mcp_tool_exposure.rs b/codex-rs/core/src/mcp_tool_exposure.rs index e58be65f9c..5dc66af734 100644 --- a/codex-rs/core/src/mcp_tool_exposure.rs +++ b/codex-rs/core/src/mcp_tool_exposure.rs @@ -3,6 +3,7 @@ use std::collections::HashSet; use codex_features::Feature; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::ToolInfo as McpToolInfo; +use codex_mcp::tool_is_model_visible; use crate::config::Config; use crate::connectors; @@ -51,7 +52,9 @@ pub(crate) fn build_mcp_tool_exposure( fn filter_non_codex_apps_mcp_tools_only(mcp_tools: &[McpToolInfo]) -> Vec { mcp_tools .iter() - .filter(|tool| tool.server_name != CODEX_APPS_MCP_SERVER_NAME) + .filter(|tool| { + tool.server_name != CODEX_APPS_MCP_SERVER_NAME && tool_is_model_visible(tool) + }) .cloned() .collect() } @@ -72,6 +75,9 @@ fn filter_codex_apps_mcp_tools( if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { return false; } + if !tool_is_model_visible(tool) { + return false; + } let Some(connector_id) = tool.connector_id.as_deref() else { return false; }; diff --git a/codex-rs/core/src/mcp_tool_exposure_test.rs b/codex-rs/core/src/mcp_tool_exposure_test.rs index c23e1e3765..4918f768aa 100644 --- a/codex-rs/core/src/mcp_tool_exposure_test.rs +++ b/codex-rs/core/src/mcp_tool_exposure_test.rs @@ -7,6 +7,7 @@ use codex_mcp::ToolInfo; use codex_tools::ToolName; use pretty_assertions::assert_eq; use rmcp::model::JsonObject; +use rmcp::model::Meta; use rmcp::model::Tool; use super::*; @@ -80,6 +81,16 @@ fn tool_names(tools: &[ToolInfo]) -> HashSet { .collect() } +fn with_visibility(mut tool: ToolInfo, visibility: &[&str]) -> ToolInfo { + tool.tool.meta = Some(Meta( + serde_json::json!({ "ui": { "visibility": visibility } }) + .as_object() + .expect("metadata object") + .clone(), + )); + tool +} + #[tokio::test] async fn directly_exposes_small_effective_tool_sets() { let config = test_config().await; @@ -93,6 +104,84 @@ async fn directly_exposes_small_effective_tool_sets() { assert!(exposure.deferred_tools.is_none()); } +#[tokio::test] +async fn excludes_tools_hidden_from_model_exposure() { + let config = test_config().await; + let visible_tool = make_mcp_tool( + "rmcp", + "visible_tool", + "mcp__rmcp", + "visible_tool", + /*connector_id*/ None, + /*connector_name*/ None, + ); + let hidden_tool = with_visibility( + make_mcp_tool( + "rmcp", + "hidden_tool", + "mcp__rmcp", + "hidden_tool", + /*connector_id*/ None, + /*connector_name*/ None, + ), + &["app"], + ); + let empty_visibility_tool = with_visibility( + make_mcp_tool( + "rmcp", + "empty_visibility_tool", + "mcp__rmcp", + "empty_visibility_tool", + /*connector_id*/ None, + /*connector_name*/ None, + ), + &[], + ); + let visible_app_tool = with_visibility( + make_mcp_tool( + CODEX_APPS_MCP_SERVER_NAME, + "calendar_read", + "mcp__codex_apps__calendar", + "read", + Some("calendar"), + Some("Calendar"), + ), + &["app", "model"], + ); + let hidden_app_tool = with_visibility( + make_mcp_tool( + CODEX_APPS_MCP_SERVER_NAME, + "calendar_open", + "mcp__codex_apps__calendar", + "open", + Some("calendar"), + Some("Calendar"), + ), + &["app"], + ); + let mcp_tools = vec![ + visible_tool.clone(), + hidden_tool, + empty_visibility_tool, + visible_app_tool.clone(), + hidden_app_tool, + ]; + let connectors = vec![make_connector("calendar", "Calendar")]; + + let exposure = build_mcp_tool_exposure( + &mcp_tools, + Some(connectors.as_slice()), + &config, + /*search_tool_enabled*/ false, + ); + + assert_eq!( + tool_names(&exposure.direct_tools), + tool_names(&[visible_tool, visible_app_tool]) + ); + assert!(exposure.deferred_tools.is_none()); +} + #[tokio::test] async fn searches_large_effective_tool_sets() { let config = test_config().await; diff --git a/codex-rs/core/tests/common/apps_test_server.rs b/codex-rs/core/tests/common/apps_test_server.rs index 2f2875867b..7f7873e30b 100644 --- a/codex-rs/core/tests/common/apps_test_server.rs +++ b/codex-rs/core/tests/common/apps_test_server.rs @@ -27,12 +27,15 @@ const SERVER_NAME: &str = "codex-apps-test"; const SERVER_VERSION: &str = "1.0.0"; const SEARCHABLE_TOOL_COUNT: usize = 100; const CALENDAR_CREATE_EVENT_TOOL_NAME: &str = "calendar_create_event"; +const CALENDAR_APP_ONLY_TOOL_NAME: &str = "calendar_app_only_action"; pub const CALENDAR_EXTRACT_TEXT_TOOL_NAME: &str = "calendar_extract_text"; const CALENDAR_LIST_EVENTS_TOOL_NAME: &str = "calendar_list_events"; pub const DIRECT_CALENDAR_CREATE_EVENT_TOOL: &str = "mcp__codex_apps__calendar__create_event"; +pub const DIRECT_CALENDAR_APP_ONLY_TOOL: &str = "mcp__codex_apps__calendar__app_only_action"; pub const DIRECT_CALENDAR_LIST_EVENTS_TOOL: &str = "mcp__codex_apps__calendar__list_events"; pub const DIRECT_CALENDAR_EXTRACT_TEXT_TOOL: &str = "mcp__codex_apps__calendar__extract_text"; pub const SEARCH_CALENDAR_NAMESPACE: &str = "mcp__codex_apps__calendar"; +pub const SEARCH_CALENDAR_APP_ONLY_TOOL: &str = "_app_only_action"; pub const SEARCH_CALENDAR_CREATE_TOOL: &str = "_create_event"; pub const SEARCH_CALENDAR_EXTRACT_TEXT_TOOL: &str = "_extract_text"; pub const SEARCH_CALENDAR_LIST_TOOL: &str = "_list_events"; @@ -49,6 +52,12 @@ pub struct AppsTestServer { pub chatgpt_base_url: String, } +#[derive(Clone, Copy)] +pub enum AppsTestToolLoading { + Direct, + Searchable, +} + impl AppsTestServer { pub async fn mount(server: &MockServer) -> Result { Self::mount_with_connector_name(server, CONNECTOR_NAME).await @@ -62,6 +71,7 @@ impl AppsTestServer { CONNECTOR_NAME.to_string(), CONNECTOR_DESCRIPTION.to_string(), /*searchable*/ true, + /*include_app_only_tool*/ false, ) .await; Ok(Self { @@ -80,6 +90,26 @@ impl AppsTestServer { connector_name.to_string(), CONNECTOR_DESCRIPTION.to_string(), /*searchable*/ false, + /*include_app_only_tool*/ false, + ) + .await; + Ok(Self { + chatgpt_base_url: server.uri(), + }) + } + + pub async fn mount_with_app_only_tool( + server: &MockServer, + tool_loading: AppsTestToolLoading, + ) -> Result { + mount_oauth_metadata(server).await; + mount_connectors_directory(server).await; + mount_streamable_http_json_rpc( + server, + CONNECTOR_NAME.to_string(), + CONNECTOR_DESCRIPTION.to_string(), + matches!(tool_loading, AppsTestToolLoading::Searchable), + /*include_app_only_tool*/ true, ) .await; Ok(Self { @@ -136,7 +166,7 @@ fn apps_tool_call_id(body: &Value) -> Option<&str> { .as_str() } -async fn recorded_apps_tool_calls(server: &MockServer) -> Vec { +pub async fn recorded_apps_tool_calls(server: &MockServer) -> Vec { server .received_requests() .await @@ -233,6 +263,7 @@ async fn mount_streamable_http_json_rpc( connector_name: String, connector_description: String, searchable: bool, + include_app_only_tool: bool, ) { Mock::given(method("POST")) .and(path_regex("^/api/codex/apps/?$")) @@ -240,6 +271,7 @@ async fn mount_streamable_http_json_rpc( connector_name, connector_description, searchable, + include_app_only_tool, }) .mount(server) .await; @@ -249,6 +281,7 @@ struct CodexAppsJsonRpcResponder { connector_name: String, connector_description: String, searchable: bool, + include_app_only_tool: bool, } impl Respond for CodexAppsJsonRpcResponder { @@ -419,6 +452,29 @@ impl Respond for CodexAppsJsonRpcResponder { })); } } + if self.include_app_only_tool + && let Some(tools) = response + .pointer_mut("/result/tools") + .and_then(Value::as_array_mut) + { + tools.push(json!({ + "name": CALENDAR_APP_ONLY_TOOL_NAME, + "description": "Open a calendar app-only action.", + "inputSchema": { + "type": "object", + "properties": {}, + "additionalProperties": false + }, + "_meta": { + "connector_id": CONNECTOR_ID, + "connector_name": self.connector_name.clone(), + "connector_description": self.connector_description.clone(), + "ui": { + "visibility": ["app"] + } + } + })); + } ResponseTemplate::new(200).set_body_json(response) } "tools/call" => { diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index 90daf21963..cbf6993bc9 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -18,6 +18,10 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; use codex_protocol::user_input::UserInput; use core_test_support::apps_test_server::AppsTestServer; +use core_test_support::apps_test_server::AppsTestToolLoading; +use core_test_support::apps_test_server::DIRECT_CALENDAR_APP_ONLY_TOOL; +use core_test_support::apps_test_server::recorded_apps_tool_calls; +use core_test_support::apps_test_server::search_capable_apps_builder; use core_test_support::assert_regex_match; use core_test_support::responses; use core_test_support::responses::ResponseMock; @@ -495,6 +499,92 @@ if (!tool) { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn app_only_tools_are_not_visible_or_runnable_by_code_mode_model() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let apps_server = + AppsTestServer::mount_with_app_only_tool(&server, AppsTestToolLoading::Searchable).await?; + let code = format!( + r#" +const visibleTool = ALL_TOOLS.find(({{ name }}) => name === {visible_tool_name:?}); +const tool = ALL_TOOLS.find(({{ name }}) => name === {tool_name:?}); +let error = null; +try {{ + await tools[{tool_name:?}]({{}}); +}} catch (caught) {{ + error = String(caught); +}} +text(JSON.stringify({{ + visibleListed: visibleTool !== undefined, + listed: tool !== undefined, + callable: typeof tools[{tool_name:?}] === "function", + error, +}})); +"#, + visible_tool_name = "mcp__codex_apps__calendar_timezone_option_99", + tool_name = DIRECT_CALENDAR_APP_ONLY_TOOL, + ); + + responses::mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_custom_tool_call("call-1", "exec", &code), + ev_completed("resp-1"), + ]), + ) + .await; + let second_mock = responses::mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ) + .await; + + let mut builder = search_capable_apps_builder(apps_server.chatgpt_base_url.clone()) + .with_config(|config| { + config + .features + .enable(Feature::CodeMode) + .expect("test config should allow feature update"); + config + .features + .enable(Feature::CodeModeOnly) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + test.submit_turn("try to call the app-only calendar tool through exec") + .await?; + + let req = second_mock.single_request(); + let (output, success) = custom_tool_output_body_and_success(&req, "call-1"); + assert_ne!( + success, + Some(false), + "code mode visibility check should complete successfully: {output}" + ); + let parsed: Value = serde_json::from_str(&output)?; + assert_eq!(parsed["visibleListed"], true); + assert_eq!(parsed["listed"], false); + assert_eq!(parsed["callable"], false); + assert!( + parsed["error"] + .as_str() + .is_some_and(|error| error.contains("is not a function")), + "app-only code mode call should fail before MCP dispatch: {parsed:?}" + ); + assert!( + recorded_apps_tool_calls(&server).await.is_empty(), + "app-only code mode call should not reach the MCP server" + ); + + Ok(()) +} + #[cfg_attr(windows, ignore = "no exec_command on Windows")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn code_mode_only_can_call_nested_tools() -> Result<()> { diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index e9ffeb62ef..c548ded95c 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -17,16 +17,20 @@ use codex_protocol::protocol::McpInvocation; use codex_protocol::protocol::Op; use codex_protocol::user_input::UserInput; use core_test_support::apps_test_server::AppsTestServer; +use core_test_support::apps_test_server::AppsTestToolLoading; use core_test_support::apps_test_server::CALENDAR_CREATE_EVENT_MCP_APP_RESOURCE_URI; use core_test_support::apps_test_server::CALENDAR_CREATE_EVENT_RESOURCE_URI; use core_test_support::apps_test_server::DIRECT_CALENDAR_CREATE_EVENT_TOOL as CALENDAR_CREATE_TOOL; use core_test_support::apps_test_server::DIRECT_CALENDAR_LIST_EVENTS_TOOL as CALENDAR_LIST_TOOL; +use core_test_support::apps_test_server::SEARCH_CALENDAR_APP_ONLY_TOOL; use core_test_support::apps_test_server::SEARCH_CALENDAR_CREATE_TOOL; use core_test_support::apps_test_server::SEARCH_CALENDAR_LIST_TOOL; use core_test_support::apps_test_server::SEARCH_CALENDAR_NAMESPACE; +use core_test_support::apps_test_server::apps_enabled_builder; use core_test_support::apps_test_server::configure_search_capable_apps; use core_test_support::apps_test_server::configure_search_capable_model; use core_test_support::apps_test_server::recorded_apps_tool_call_by_call_id; +use core_test_support::apps_test_server::recorded_apps_tool_calls; use core_test_support::apps_test_server::search_capable_apps_builder as configured_builder; use core_test_support::responses::ResponsesRequest; use core_test_support::responses::ev_assistant_message; @@ -216,6 +220,80 @@ async fn always_defer_feature_hides_small_app_tool_sets() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn app_only_tools_are_not_visible_or_runnable_by_direct_model_calls() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let apps_server = + AppsTestServer::mount_with_app_only_tool(&server, AppsTestToolLoading::Direct).await?; + let call_id = "app-only-direct-call"; + let mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call_with_namespace( + call_id, + SEARCH_CALENDAR_NAMESPACE, + SEARCH_CALENDAR_APP_ONLY_TOOL, + "{}", + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let mut builder = apps_enabled_builder(apps_server.chatgpt_base_url.clone()); + let test = builder.build(&server).await?; + test.submit_turn_with_approval_and_permission_profile( + "Try to call the app-only calendar tool.", + AskForApproval::Never, + PermissionProfile::Disabled, + ) + .await?; + + let requests = mock.requests(); + assert!( + namespace_child_tool( + &requests[0].body_json(), + SEARCH_CALENDAR_NAMESPACE, + SEARCH_CALENDAR_CREATE_TOOL + ) + .is_some(), + "visible tool from the app-only tool's connector should be declared" + ); + assert!( + namespace_child_tool( + &requests[0].body_json(), + SEARCH_CALENDAR_NAMESPACE, + SEARCH_CALENDAR_APP_ONLY_TOOL + ) + .is_none(), + "app-only tool should not be declared to a direct model" + ); + assert!( + requests[1] + .function_call_output(call_id) + .get("output") + .and_then(Value::as_str) + .is_some_and(|output| output.contains("unsupported call")), + "forced app-only direct call should not dispatch" + ); + assert!( + recorded_apps_tool_calls(&server).await.is_empty(), + "forced app-only direct call should not reach the MCP server" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn app_search_sources_are_hidden_for_api_key_auth() -> Result<()> { skip_if_no_network!(Ok(())); From 489bf386580409891b1792331eff4dd92afd4404 Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Thu, 28 May 2026 10:29:06 -0700 Subject: [PATCH 03/10] chore: add GPT-5.5 to the Amazon Bedrock catalog (#24701) ## Summary Amazon Bedrock should expose GPT-5.5 alongside GPT-5.4, and the Bedrock GPT entries should stay aligned with the canonical bundled OpenAI model metadata instead of carrying a separate hand-written copy that can drift over time. This change will be merged when the model is online. This change: - Adds the Bedrock Mantle model id for `openai.gpt-5.5`. - Builds the Bedrock GPT-5.5 and GPT-5.4 catalog entries from the bundled OpenAI model catalog, then overrides the Bedrock-facing slug, explicit priority, and Bedrock-specific context windows. - Hardcodes both `context_window` and `max_context_window` to `272000` for Bedrock GPT-5.5 and GPT-5.4. - Keeps `openai.gpt-5.5` as the default Bedrock model ahead of `openai.gpt-5.4` and the Bedrock OSS models. --- codex-rs/model-provider-info/src/lib.rs | 1 + .../src/amazon_bedrock/catalog.rs | 126 ++++++++---------- codex-rs/model-provider/src/provider.rs | 3 +- 3 files changed, 61 insertions(+), 69 deletions(-) diff --git a/codex-rs/model-provider-info/src/lib.rs b/codex-rs/model-provider-info/src/lib.rs index 65f71851d6..c20a51a3aa 100644 --- a/codex-rs/model-provider-info/src/lib.rs +++ b/codex-rs/model-provider-info/src/lib.rs @@ -37,6 +37,7 @@ pub const OPENAI_PROVIDER_ID: &str = "openai"; pub const CHATGPT_CODEX_BASE_URL: &str = "https://chatgpt.com/backend-api/codex"; const AMAZON_BEDROCK_PROVIDER_NAME: &str = "Amazon Bedrock"; pub const AMAZON_BEDROCK_PROVIDER_ID: &str = "amazon-bedrock"; +pub const AMAZON_BEDROCK_GPT_5_5_MODEL_ID: &str = "openai.gpt-5.5"; pub const AMAZON_BEDROCK_GPT_5_4_MODEL_ID: &str = "openai.gpt-5.4"; pub const AMAZON_BEDROCK_DEFAULT_BASE_URL: &str = "https://bedrock-mantle.us-east-1.api.aws/openai/v1"; diff --git a/codex-rs/model-provider/src/amazon_bedrock/catalog.rs b/codex-rs/model-provider/src/amazon_bedrock/catalog.rs index 8fec7f835e..6ebb736fca 100644 --- a/codex-rs/model-provider/src/amazon_bedrock/catalog.rs +++ b/codex-rs/model-provider/src/amazon_bedrock/catalog.rs @@ -1,83 +1,66 @@ use codex_model_provider_info::AMAZON_BEDROCK_GPT_5_4_MODEL_ID; +use codex_model_provider_info::AMAZON_BEDROCK_GPT_5_5_MODEL_ID; +use codex_models_manager::bundled_models_response; use codex_models_manager::model_info::BASE_INSTRUCTIONS; use codex_protocol::config_types::ReasoningSummary; -use codex_protocol::config_types::ServiceTier; -use codex_protocol::config_types::Verbosity; -use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::InputModality; use codex_protocol::openai_models::ModelInfo; -use codex_protocol::openai_models::ModelServiceTier; use codex_protocol::openai_models::ModelVisibility; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::openai_models::ReasoningEffortPreset; -use codex_protocol::openai_models::SPEED_TIER_FAST; use codex_protocol::openai_models::TruncationPolicyConfig; use codex_protocol::openai_models::WebSearchToolType; const GPT_OSS_CONTEXT_WINDOW: i64 = 128_000; -const GPT_5_4_CONTEXT_WINDOW: i64 = 272_000; -const GPT_5_4_MAX_CONTEXT_WINDOW: i64 = 1_000_000; +const GPT_5_BEDROCK_CONTEXT_WINDOW: i64 = 272_000; +const GPT_5_5_OPENAI_MODEL_ID: &str = "gpt-5.5"; +const GPT_5_4_OPENAI_MODEL_ID: &str = "gpt-5.4"; pub(crate) fn static_model_catalog() -> ModelsResponse { ModelsResponse { models: vec![ - gpt_5_4_cmb_bedrock_model(/*priority*/ 0), + gpt_5_bedrock_model( + GPT_5_5_OPENAI_MODEL_ID, + AMAZON_BEDROCK_GPT_5_5_MODEL_ID, + /*priority*/ 0, + ), + gpt_5_bedrock_model( + GPT_5_4_OPENAI_MODEL_ID, + AMAZON_BEDROCK_GPT_5_4_MODEL_ID, + /*priority*/ 1, + ), bedrock_oss_model( "openai.gpt-oss-120b", "GPT OSS 120B on Bedrock", - /*priority*/ 1, + /*priority*/ 2, ), bedrock_oss_model( "openai.gpt-oss-20b", "GPT OSS 20B on Bedrock", - /*priority*/ 2, + /*priority*/ 3, ), ], } } -fn gpt_5_4_cmb_bedrock_model(priority: i32) -> ModelInfo { - ModelInfo { - slug: AMAZON_BEDROCK_GPT_5_4_MODEL_ID.to_string(), - display_name: "gpt-5.4".to_string(), - description: Some("Strong model for everyday coding.".to_string()), - default_reasoning_level: Some(ReasoningEffort::Medium), - supported_reasoning_levels: gpt_5_4_cmb_reasoning_levels(), - shell_type: ConfigShellToolType::ShellCommand, - visibility: ModelVisibility::List, - supported_in_api: true, - priority, - additional_speed_tiers: Vec::new(), - service_tiers: vec![ModelServiceTier { - id: ServiceTier::Fast.request_value().to_string(), - name: SPEED_TIER_FAST.to_string(), - description: "Fastest inference with increased plan usage".to_string(), - }], - default_service_tier: None, - availability_nux: None, - upgrade: None, - base_instructions: BASE_INSTRUCTIONS.to_string(), - model_messages: None, - supports_reasoning_summaries: true, - default_reasoning_summary: ReasoningSummary::None, - support_verbosity: true, - default_verbosity: Some(Verbosity::Medium), - apply_patch_tool_type: Some(ApplyPatchToolType::Freeform), - web_search_tool_type: WebSearchToolType::TextAndImage, - truncation_policy: TruncationPolicyConfig::tokens(/*limit*/ 10_000), - supports_parallel_tool_calls: true, - supports_image_detail_original: true, - context_window: Some(GPT_5_4_CONTEXT_WINDOW), - max_context_window: Some(GPT_5_4_MAX_CONTEXT_WINDOW), - auto_compact_token_limit: None, - effective_context_window_percent: 95, - experimental_supported_tools: Vec::new(), - input_modalities: vec![InputModality::Text, InputModality::Image], - used_fallback_model_metadata: false, - supports_search_tool: true, - } +fn gpt_5_bedrock_model(openai_slug: &str, bedrock_slug: &str, priority: i32) -> ModelInfo { + let mut model = bundled_openai_model(openai_slug); + model.slug = bedrock_slug.to_string(); + model.priority = priority; + model.context_window = Some(GPT_5_BEDROCK_CONTEXT_WINDOW); + model.max_context_window = Some(GPT_5_BEDROCK_CONTEXT_WINDOW); + model +} + +fn bundled_openai_model(slug: &str) -> ModelInfo { + bundled_models_response() + .unwrap_or_else(|err| panic!("bundled models.json should parse: {err}")) + .models + .into_iter() + .find(|model| model.slug == slug) + .unwrap_or_else(|| panic!("bundled models.json should include {slug}")) } fn bedrock_oss_model(slug: &str, display_name: &str, priority: i32) -> ModelInfo { @@ -122,15 +105,6 @@ fn bedrock_oss_model(slug: &str, display_name: &str, priority: i32) -> ModelInfo } } -fn gpt_5_4_cmb_reasoning_levels() -> Vec { - vec![ - reasoning_effort_preset(ReasoningEffort::Minimal), - reasoning_effort_preset(ReasoningEffort::Low), - reasoning_effort_preset(ReasoningEffort::Medium), - reasoning_effort_preset(ReasoningEffort::High), - ] -} - fn reasoning_effort_preset(effort: ReasoningEffort) -> ReasoningEffortPreset { ReasoningEffortPreset { effort, @@ -156,24 +130,40 @@ mod tests { fn catalog_uses_mantle_model_ids_as_slugs() { let catalog = static_model_catalog(); - assert_eq!(catalog.models.len(), 3); - assert_eq!(catalog.models[0].slug, AMAZON_BEDROCK_GPT_5_4_MODEL_ID); - assert_eq!(catalog.models[1].slug, "openai.gpt-oss-120b"); - assert_eq!(catalog.models[2].slug, "openai.gpt-oss-20b"); + assert_eq!(catalog.models.len(), 4); + assert_eq!(catalog.models[0].slug, AMAZON_BEDROCK_GPT_5_5_MODEL_ID); + assert_eq!(catalog.models[1].slug, AMAZON_BEDROCK_GPT_5_4_MODEL_ID); + assert_eq!(catalog.models[2].slug, "openai.gpt-oss-120b"); + assert_eq!(catalog.models[3].slug, "openai.gpt-oss-20b"); } #[test] - fn gpt_5_4_cmb_advertises_only_bedrock_supported_reasoning_levels() { + fn gpt_5_bedrock_models_use_bedrock_context_window() { let catalog = static_model_catalog(); - let cmb_model = catalog + let gpt_5_5 = catalog + .models + .iter() + .find(|model| model.slug == AMAZON_BEDROCK_GPT_5_5_MODEL_ID) + .expect("Bedrock catalog should include GPT-5.5"); + let gpt_5_4 = catalog .models .iter() .find(|model| model.slug == AMAZON_BEDROCK_GPT_5_4_MODEL_ID) - .expect("Bedrock catalog should include GPT-5.4 CMB"); + .expect("Bedrock catalog should include GPT-5.4"); assert_eq!( - cmb_model.supported_reasoning_levels, - gpt_5_4_cmb_reasoning_levels() + (gpt_5_5.context_window, gpt_5_5.max_context_window), + ( + Some(GPT_5_BEDROCK_CONTEXT_WINDOW), + Some(GPT_5_BEDROCK_CONTEXT_WINDOW) + ) + ); + assert_eq!( + (gpt_5_4.context_window, gpt_5_4.max_context_window), + ( + Some(GPT_5_BEDROCK_CONTEXT_WINDOW), + Some(GPT_5_BEDROCK_CONTEXT_WINDOW) + ) ); } } diff --git a/codex-rs/model-provider/src/provider.rs b/codex-rs/model-provider/src/provider.rs index 1ef7c22962..8230c0163b 100644 --- a/codex-rs/model-provider/src/provider.rs +++ b/codex-rs/model-provider/src/provider.rs @@ -510,6 +510,7 @@ mod tests { assert_eq!( model_ids, vec![ + "openai.gpt-5.5", "openai.gpt-5.4", "openai.gpt-oss-120b", "openai.gpt-oss-20b" @@ -523,7 +524,7 @@ mod tests { .find(|preset| preset.is_default) .expect("Bedrock catalog should have a default model"); - assert_eq!(default_model.model, "openai.gpt-5.4"); + assert_eq!(default_model.model, "openai.gpt-5.5"); } #[tokio::test] From 6c1215dac66494fea326b2f1d472b5ff676a28a3 Mon Sep 17 00:00:00 2001 From: canvrno-oai Date: Thu, 28 May 2026 10:30:15 -0700 Subject: [PATCH 04/10] TUI: Unified mentions tweaks + polish mentions rendering (#23363) This change keeps unified @mentions behind the mentions_v2 gate, moves the flag to under-development, and polishes mention rendering/history behavior. It also adds a few small improvements to the mentions feature around mention rendering and history round-tripping for plugin/tool mentions in message edit scenarios. Plugin selections now insert `@` mentions with better casing, and saved history preserves the visible sigil so recalled messages look the same as what the user typed. - Preserves `@` sigils when encoding/decoding mention history for tool/plugin paths. - Improves plugin mention insertion so display names/casing are reflected more cleanly in the composer. - Update composer to render user-entered plugin mentions in the same color as the mentions menu. ALso applies to recalled/edited messages. - Left/right arrows no longer switch unified-mention search modes after an @mention has already been accepted (Ex: arrowing left through a composed message that contains @mentions). - Keeps bound mentions stable around punctuation, so accepted `@` mentions do not reopen the popup and punctuated `$` mentions still persist to cross-session history. **Steps to test** - Ensure mentions_v2 is enabled through configuration or `--enable mentions_v2` - Type `@` in the TUI composer and verify filesystem/plugin/skill results are displayed in the unified mentions menu. - Select a plugin mention from the `@` popup and confirm the inserted text is an `@...` mention with casing, then recall/edit the message and confirm it still renders as `@...`. - Mention a skill and verify that skills still insert as `$skill` mentions rather than `@` mentions. - Verify punctuated mentions such as `@plugin.` and `($skill)` keep their bound mention behavior across editing and history recall. --- codex-rs/features/src/lib.rs | 6 +- codex-rs/features/src/tests.rs | 7 + codex-rs/tui/src/bottom_pane/chat_composer.rs | 692 ++++++++++++++++-- .../bottom_pane/chat_composer/draft_state.rs | 1 + .../src/bottom_pane/chat_composer_history.rs | 96 ++- .../bottom_pane/mentions_v2/search_catalog.rs | 106 ++- codex-rs/tui/src/bottom_pane/mod.rs | 4 +- ...at_mentions_render_with_plugin_accent.snap | 6 + .../tui/src/chatwidget/input_submission.rs | 1 + .../chatwidget/tests/composer_submission.rs | 3 + .../tui/src/chatwidget/tests/plan_mode.rs | 1 + .../tui/src/chatwidget/tests/review_mode.rs | 1 + .../src/chatwidget/tests/slash_commands.rs | 3 + codex-rs/tui/src/mention_codec.rs | 347 ++++++++- 14 files changed, 1170 insertions(+), 104 deletions(-) create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__plugin_at_mentions_render_with_plugin_accent.snap diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 391e33e77d..1b0bd15486 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -1068,11 +1068,7 @@ pub const FEATURES: &[FeatureSpec] = &[ FeatureSpec { id: Feature::MentionsV2, key: "mentions_v2", - stage: Stage::Experimental { - name: "Mentions v2", - menu_description: "Use a unified @ mention popup for files, folders, apps, plugins, and skills.", - announcement: "", - }, + stage: Stage::UnderDevelopment, default_enabled: false, }, FeatureSpec { diff --git a/codex-rs/features/src/tests.rs b/codex-rs/features/src/tests.rs index e1f48745f9..b8cd2fd5c6 100644 --- a/codex-rs/features/src/tests.rs +++ b/codex-rs/features/src/tests.rs @@ -304,6 +304,13 @@ fn auth_elicitation_is_under_development() { ); } +#[test] +fn mentions_v2_is_under_development_and_disabled_by_default() { + assert_eq!(Feature::MentionsV2.stage(), Stage::UnderDevelopment); + assert_eq!(Feature::MentionsV2.default_enabled(), false); + assert_eq!(feature_for_key("mentions_v2"), Some(Feature::MentionsV2)); +} + #[test] fn remote_control_is_removed_and_disabled_by_default() { assert_eq!(Feature::RemoteControl.stage(), Stage::Removed); diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index e62cab9e86..21208de946 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -256,7 +256,6 @@ use std::path::PathBuf; use std::time::Duration; use std::time::Instant; -#[cfg(test)] use ratatui::style::Color; /// If the pasted content exceeds this number of characters, replace it with a @@ -590,6 +589,7 @@ impl ChatComposer { pub fn set_mentions_v2_enabled(&mut self, enabled: bool) { self.mentions_v2_enabled = enabled; + self.history.set_at_mention_restore_enabled(enabled); self.sync_popups(); } @@ -609,11 +609,13 @@ impl ChatComposer { pub(crate) fn take_mention_bindings(&mut self) -> Vec { let elements = self.current_mention_elements(); let mut ordered = Vec::new(); - for (id, mention) in elements { + for (id, sigil, mention) in elements { if let Some(binding) = self.draft.mention_bindings.remove(&id) + && binding.sigil == sigil && binding.mention == mention { ordered.push(MentionBinding { + sigil: binding.sigil, mention: binding.mention, path: binding.path, }); @@ -1195,7 +1197,7 @@ impl ChatComposer { /// /// This is the "fresh draft" path: it clears pending paste payloads and /// mention link targets. Callers restoring a previously submitted draft - /// that must keep `$name -> path` resolution should use + /// that must keep sigiled mention target resolution should use /// [`Self::set_text_content_with_mention_bindings`] instead. pub(crate) fn set_text_content( &mut self, @@ -1649,7 +1651,7 @@ impl ChatComposer { result } - /// Return true if either the slash-command popup or the file-search popup is active. + /// Return true if any popup or history search is active. pub(crate) fn popup_active(&self) -> bool { self.history_search.is_some() || self.popups.active() } @@ -1802,7 +1804,6 @@ impl ChatComposer { KeyEvent { code: KeyCode::Esc, .. } => { - // Hide popup without modifying text, remember token to avoid immediate reopen. if let Some(tok) = Self::current_at_token(&self.draft.textarea) { self.popups.dismissed_file_token = Some(tok); } @@ -1827,24 +1828,17 @@ impl ChatComposer { }; let sel_path = sel.to_string_lossy().to_string(); - // If selected path looks like an image (png/jpeg), attach as image instead of inserting text. - let is_image = Self::is_image_path(&sel_path); - if is_image { - // Determine dimensions; if that fails fall back to normal path insertion. + if Self::is_image_path(&sel_path) { let path_buf = PathBuf::from(&sel_path); match image::image_dimensions(&path_buf) { Ok((width, height)) => { tracing::debug!("selected image dimensions={}x{}", width, height); - // Remove the current @token (mirror logic from insert_selected_path without inserting text) - // using the flat text and byte-offset cursor API. let cursor_offset = self.draft.textarea.cursor(); let text = self.draft.textarea.text(); - // Clamp to a valid char boundary to avoid panics when slicing. let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); let before_cursor = &text[..safe_cursor]; let after_cursor = &text[safe_cursor..]; - // Determine token boundaries in the full text. let start_idx = before_cursor .char_indices() .rfind(|(_, c)| c.is_whitespace()) @@ -1861,17 +1855,14 @@ impl ChatComposer { self.draft.textarea.set_cursor(start_idx); self.attach_image(path_buf); - // Add a trailing space to keep typing fluid. self.draft.textarea.insert_str(" "); } Err(err) => { tracing::trace!("image dimensions lookup failed: {err}"); - // Fallback to plain path insertion if metadata read fails. self.insert_selected_path(&sel_path); } } } else { - // Non-image: inserting file path. self.insert_selected_path(&sel_path); } self.popups.active = ActivePopup::None; @@ -1881,6 +1872,7 @@ impl ChatComposer { } } + /// Handle key events when the legacy skill mention popup is visible. fn handle_key_event_with_skill_popup(&mut self, key_event: KeyEvent) -> (InputResult, bool) { if self.handle_shortcut_overlay_key(&key_event) { return (InputResult::None, true); @@ -1962,6 +1954,7 @@ impl ChatComposer { return (InputResult::None, true); } self.footer.mode = reset_mode_after_activity(self.footer.mode); + let can_switch_search_mode = self.current_editable_at_token().is_some(); let ActivePopup::MentionV2(popup) = &mut self.popups.active else { unreachable!(); @@ -1969,6 +1962,7 @@ impl ChatComposer { let mut selected: Option = None; let mut close_popup = false; + let mut submit_without_popup = false; let result = match key_event { KeyEvent { @@ -1999,16 +1993,24 @@ impl ChatComposer { modifiers: KeyModifiers::NONE, .. } => { - popup.previous_search_mode(); - (InputResult::None, true) + if can_switch_search_mode { + popup.previous_search_mode(); + (InputResult::None, true) + } else { + self.handle_input_basic(key_event) + } } KeyEvent { code: KeyCode::Right, modifiers: KeyModifiers::NONE, .. } => { - popup.next_search_mode(); - (InputResult::None, true) + if can_switch_search_mode { + popup.next_search_mode(); + (InputResult::None, true) + } else { + self.handle_input_basic(key_event) + } } KeyEvent { code: KeyCode::Esc, .. @@ -2021,14 +2023,19 @@ impl ChatComposer { } KeyEvent { code: KeyCode::Tab, .. + } => { + selected = popup.selected(); + close_popup = true; + (InputResult::None, true) } - | KeyEvent { + KeyEvent { code: KeyCode::Enter, modifiers: KeyModifiers::NONE, .. } => { selected = popup.selected(); close_popup = true; + submit_without_popup = selected.is_none(); (InputResult::None, true) } input => self.handle_input_basic(input), @@ -2046,6 +2053,9 @@ impl ChatComposer { } } self.popups.active = ActivePopup::None; + if submit_without_popup { + return self.handle_key_event_without_popup(key_event); + } } result @@ -2241,14 +2251,14 @@ impl ChatComposer { /// second `@` in `@scope/pkg@latest`), keep treating the surrounding /// whitespace-delimited token as the active token rather than starting a /// new token at that nested prefix. - /// - If the token under the cursor starts with `prefix`, that token is - /// returned without the leading prefix. When `allow_empty` is true, a - /// lone prefix character yields `Some(String::new())` to surface hints. - fn current_prefixed_token( + /// - If the token under the cursor starts with `prefix`, its byte range and + /// text without the leading prefix are returned. When `allow_empty` is + /// true, a lone prefix character yields `Some(String::new())` to surface hints. + fn current_prefixed_token_range( textarea: &TextArea, prefix: char, allow_empty: bool, - ) -> Option { + ) -> Option<(Range, String)> { let cursor_offset = textarea.cursor(); let text = textarea.text(); @@ -2321,15 +2331,17 @@ impl ChatComposer { let left_match = token_left.filter(|t| t.starts_with(prefix)); let right_match = token_right.filter(|t| t.starts_with(prefix)); - let left_prefixed = left_match.map(|t| t[prefix.len_utf8()..].to_string()); - let right_prefixed = right_match.map(|t| t[prefix.len_utf8()..].to_string()); + let left_prefixed = + left_match.map(|t| (start_left..end_left, t[prefix.len_utf8()..].to_string())); + let right_prefixed = + right_match.map(|t| (start_right..end_right, t[prefix.len_utf8()..].to_string())); if at_whitespace { if right_prefixed.is_some() { return right_prefixed; } if token_left.is_some_and(|t| t == prefix_str) { - return allow_empty.then(String::new); + return allow_empty.then(|| (start_left..end_left, String::new())); } return left_prefixed; } @@ -2347,6 +2359,14 @@ impl ChatComposer { left_prefixed.or(right_prefixed) } + fn current_prefixed_token( + textarea: &TextArea, + prefix: char, + allow_empty: bool, + ) -> Option { + Self::current_prefixed_token_range(textarea, prefix, allow_empty).map(|(_, token)| token) + } + /// Extract the `@token` that the cursor is currently positioned on, if any. /// /// The returned string **does not** include the leading `@`. @@ -2354,11 +2374,48 @@ impl ChatComposer { Self::current_prefixed_token(textarea, '@', /*allow_empty*/ false) } + fn current_editable_at_token_with_options(&self, allow_empty: bool) -> Option { + let (range, token) = + Self::current_prefixed_token_range(&self.draft.textarea, '@', allow_empty)?; + if self + .draft + .textarea + .element_id_for_exact_range(range.clone()) + .is_some() + { + return None; + } + + let name_len = token + .as_bytes() + .iter() + .take_while(|byte| is_mention_name_char(**byte)) + .count(); + let mention_end = range.start + '@'.len_utf8() + name_len; + if name_len > 0 + && mention_end < range.end + && ends_plaintext_at_mention(self.draft.textarea.text().as_bytes(), mention_end) + && self + .draft + .textarea + .element_id_for_exact_range(range.start..mention_end) + .is_some() + { + return None; + } + + Some(token) + } + + fn current_editable_at_token(&self) -> Option { + self.current_editable_at_token_with_options(/*allow_empty*/ false) + } + fn current_mentions_v2_token(&self) -> Option { if !self.mentions_v2_enabled { return None; } - Self::current_prefixed_token(&self.draft.textarea, '@', /*allow_empty*/ true) + self.current_editable_at_token_with_options(/*allow_empty*/ true) } fn current_mention_token(&self) -> Option { @@ -2441,12 +2498,13 @@ impl ChatComposer { self.draft.textarea.set_cursor(start_idx); let id = self.draft.textarea.insert_element(insert_text); - if let (Some(path), Some(mention)) = - (path, Self::mention_name_from_insert_text(insert_text)) + if let (Some(path), Some((sigil, mention))) = + (path, Self::mention_token_from_insert_text(insert_text)) { self.draft.mention_bindings.insert( id, ComposerMentionBinding { + sigil, mention, path: path.to_string(), }, @@ -2460,8 +2518,12 @@ impl ChatComposer { self.draft.textarea.set_cursor(new_cursor); } - fn mention_name_from_insert_text(insert_text: &str) -> Option { - let name = insert_text.strip_prefix('$')?; + fn mention_token_from_insert_text(insert_text: &str) -> Option<(char, String)> { + let sigil = insert_text.chars().next()?; + if !matches!(sigil, '$' | '@') { + return None; + } + let name = &insert_text[sigil.len_utf8()..]; if name.is_empty() { return None; } @@ -2470,31 +2532,33 @@ impl ChatComposer { .iter() .all(|byte| is_mention_name_char(*byte)) { - Some(name.to_string()) + Some((sigil, name.to_string())) } else { None } } - fn current_mention_elements(&self) -> Vec<(u64, String)> { + fn current_mention_elements(&self) -> Vec<(u64, char, String)> { self.draft .textarea .text_element_snapshots() .into_iter() .filter_map(|snapshot| { - Self::mention_name_from_insert_text(snapshot.text.as_str()) - .map(|mention| (snapshot.id, mention)) + Self::mention_token_from_insert_text(snapshot.text.as_str()) + .map(|(sigil, mention)| (snapshot.id, sigil, mention)) }) .collect() } fn snapshot_mention_bindings(&self) -> Vec { let mut ordered = Vec::new(); - for (id, mention) in self.current_mention_elements() { + for (id, sigil, mention) in self.current_mention_elements() { if let Some(binding) = self.draft.mention_bindings.get(&id) + && binding.sigil == sigil && binding.mention == mention { ordered.push(MentionBinding { + sigil: binding.sigil, mention: binding.mention.clone(), path: binding.path.clone(), }); @@ -2512,7 +2576,7 @@ impl ChatComposer { let text = self.draft.textarea.text().to_string(); let mut scan_from = 0usize; for binding in mention_bindings { - let token = format!("${}", binding.mention); + let token = format!("{}{}", binding.sigil, binding.mention); let Some(range) = find_next_mention_token_range(text.as_str(), token.as_str(), scan_from) else { @@ -2531,6 +2595,7 @@ impl ChatComposer { self.draft.mention_bindings.insert( id, ComposerMentionBinding { + sigil: binding.sigil, mention: binding.mention, path: binding.path, }, @@ -2540,6 +2605,21 @@ impl ChatComposer { } } + fn plugin_at_mention_highlights(&self) -> Vec<(Range, Style)> { + self.draft + .textarea + .text_element_snapshots() + .into_iter() + .filter_map(|snapshot| { + let binding = self.draft.mention_bindings.get(&snapshot.id)?; + if !binding.path.starts_with("plugin://") || !snapshot.text.starts_with('@') { + return None; + } + Some((snapshot.range, Style::default().fg(Color::Magenta))) + }) + .collect() + } + /// Prepare text for submission/queuing. Returns None if submission should be suppressed. /// On success, clears pending paste payloads because placeholders have been expanded. /// @@ -3411,7 +3491,7 @@ impl ChatComposer { let file_token = if self.mentions_v2_enabled { None } else { - Self::current_at_token(&self.draft.textarea) + self.current_editable_at_token() }; let browsing_history = self .history @@ -3537,10 +3617,8 @@ impl ChatComposer { } } - /// Synchronize `self.file_search_popup` with the current text in the textarea. - /// Note this is only called when the active popup is NOT Command. + /// Synchronize the legacy file-search popup with the current `@` token. fn sync_file_search_popup(&mut self, query: String) { - // If user dismissed popup for this exact query, don't reopen until text changes. if self.popups.dismissed_file_token.as_ref() == Some(&query) { return; } @@ -3901,16 +3979,46 @@ fn is_mention_name_char(byte: u8) -> bool { matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'_' | b'-') } +fn ends_plaintext_at_mention(bytes: &[u8], index: usize) -> bool { + bytes.get(index).is_none_or(|byte| { + byte.is_ascii_whitespace() + || *byte == b'.' + && bytes.get(index + 1).is_none_or(|next| { + next.is_ascii_whitespace() + || !next.is_ascii_alphanumeric() && *next != b'_' && *next != b'-' + }) + || !matches!(*byte, b'.' | b'/' | b'\\') + && !byte.is_ascii_alphanumeric() + && *byte != b'_' + && *byte != b'-' + }) +} + +fn starts_plaintext_at_mention(text: &str, index: usize) -> bool { + if index == 0 { + return true; + } + + text.get(..index) + .and_then(|prefix| prefix.chars().next_back()) + .is_some_and(|ch| ch.is_whitespace() || !is_mention_name_char_char(ch)) +} + +fn is_mention_name_char_char(ch: char) -> bool { + ch.is_ascii_alphanumeric() || matches!(ch, '_' | '-') +} + fn find_next_mention_token_range(text: &str, token: &str, from: usize) -> Option> { if token.is_empty() || from >= text.len() { return None; } let bytes = text.as_bytes(); let token_bytes = token.as_bytes(); + let sigil = *token_bytes.first()?; let mut index = from; while index < bytes.len() { - if bytes[index] != b'$' { + if bytes[index] != sigil { index += 1; continue; } @@ -3924,10 +4032,24 @@ fn find_next_mention_token_range(text: &str, token: &str, from: usize) -> Option continue; } - if bytes - .get(end) - .is_none_or(|byte| !is_mention_name_char(*byte)) - { + // Fix for restored `@` mentions: rebinding must not attach to embedded substrings such + // as email addresses, while preserving the existing `$` mention matching behavior. + let starts_plaintext_mention = if sigil == b'@' { + starts_plaintext_at_mention(text, index) + } else { + true + }; + // Fix for restored `@` mentions: mirror history encoding's trailing boundary so path-like + // text such as `@sample/pkg` is not rebound as the plain `@sample` mention. + let ends_plaintext_mention = if sigil == b'@' { + ends_plaintext_at_mention(bytes, end) + } else { + bytes + .get(end) + .is_none_or(|byte| !is_mention_name_char(*byte)) + }; + + if starts_plaintext_mention && ends_plaintext_mention { return Some(index..end); } @@ -4286,8 +4408,15 @@ impl ChatComposer { .textarea .render_ref_masked(textarea_rect, buf, &mut state, mask_char); } else { - let highlight_ranges = self.history_search_highlight_ranges(); - if highlight_ranges.is_empty() { + let mut highlights = self.plugin_at_mention_highlights(); + let search_highlight_style = + Style::default().add_modifier(Modifier::REVERSED | Modifier::BOLD); + highlights.extend( + self.history_search_highlight_ranges() + .into_iter() + .map(|range| (range, search_highlight_style)), + ); + if highlights.is_empty() { StatefulWidgetRef::render_ref( &(&self.draft.textarea), textarea_rect, @@ -4295,12 +4424,6 @@ impl ChatComposer { &mut state, ); } else { - let highlight_style = - Style::default().add_modifier(Modifier::REVERSED | Modifier::BOLD); - let highlights = highlight_ranges - .into_iter() - .map(|range| (range, highlight_style)) - .collect::>(); self.draft.textarea.render_ref_styled_with_highlights( textarea_rect, buf, @@ -4736,6 +4859,142 @@ mod tests { ); } + fn plugin_mention_foreground_color(composer: &ChatComposer) -> Option { + let area = Rect::new(0, 0, 40, 5); + let mut buf = Buffer::empty(area); + composer.render(area, &mut buf); + + let textarea_row = 1; + let row_text = (0..area.width) + .map(|x| { + buf[(x, textarea_row)] + .symbol() + .chars() + .next() + .unwrap_or(' ') + }) + .collect::(); + let mention_x = row_text + .find("@sample") + .expect("expected plugin mention in composer row"); + buf[(mention_x as u16, textarea_row)].style().fg + } + + #[test] + fn plugin_at_mentions_use_plugin_accent_style() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ true, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer.set_text_content_with_mention_bindings( + "@sample plugin".to_string(), + Vec::new(), + Vec::new(), + vec![MentionBinding { + sigil: '@', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }], + ); + + assert_eq!( + plugin_mention_foreground_color(&composer), + Some(Color::Magenta) + ); + } + + #[test] + fn plugin_at_mentions_render_with_plugin_accent_snapshot() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ true, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer.set_text_content_with_mention_bindings( + "@sample plugin".to_string(), + Vec::new(), + Vec::new(), + vec![MentionBinding { + sigil: '@', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }], + ); + + let area = Rect::new(0, 0, 40, 5); + let mut buf = Buffer::empty(area); + composer.render(area, &mut buf); + + let textarea_row = 1; + let mut text = String::new(); + let mut magenta = String::new(); + for x in 0..area.width { + let cell = &buf[(x, textarea_row)]; + text.push(cell.symbol().chars().next().unwrap_or(' ')); + magenta.push(if cell.style().fg == Some(Color::Magenta) { + '^' + } else { + ' ' + }); + } + while text.ends_with(' ') { + text.pop(); + } + while magenta.ends_with(' ') { + magenta.pop(); + } + + insta::assert_snapshot!( + "plugin_at_mentions_render_with_plugin_accent", + format!("text: {text}\nmagenta: {magenta}") + ); + } + + #[test] + fn recalled_plugin_at_mentions_keep_plugin_accent_style() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ true, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer.set_text_content_with_mention_bindings( + "@sample plugin".to_string(), + Vec::new(), + Vec::new(), + vec![MentionBinding { + sigil: '@', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }], + ); + let (result, _) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert!(matches!(result, InputResult::Submitted { .. })); + + composer.set_text_content(String::new(), Vec::new(), Vec::new()); + let (_, needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + assert!(needs_redraw); + + assert_eq!( + plugin_mention_foreground_color(&composer), + Some(Color::Magenta) + ); + } + #[test] fn status_line_hyperlink_marks_pr_number_cells() { let (tx, _rx) = unbounded_channel::(); @@ -6446,6 +6705,291 @@ mod tests { } } + #[test] + fn set_text_content_rebinds_at_sigiled_mentions() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + + let mention_bindings = vec![MentionBinding { + sigil: '@', + mention: "figma".to_string(), + path: "/tmp/user/figma/SKILL.md".to_string(), + }]; + composer.set_text_content_with_mention_bindings( + "@figma please".to_string(), + Vec::new(), + Vec::new(), + mention_bindings.clone(), + ); + + assert_eq!(composer.mention_bindings(), mention_bindings); + } + + #[test] + fn set_text_content_rebinds_matching_sigil_only() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + + let mention_bindings = vec![MentionBinding { + sigil: '$', + mention: "figma".to_string(), + path: "app://figma".to_string(), + }]; + composer.set_text_content_with_mention_bindings( + "@figma then $figma".to_string(), + Vec::new(), + Vec::new(), + mention_bindings.clone(), + ); + + let bound_tokens = composer + .current_mention_elements() + .into_iter() + .map(|(_, sigil, mention)| (sigil, mention)) + .collect::>(); + assert_eq!(bound_tokens, vec![('$', "figma".to_string())]); + assert_eq!(composer.mention_bindings(), mention_bindings); + } + + #[test] + fn set_text_content_rebinds_both_sigil_forms() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + + let mention_bindings = vec![ + MentionBinding { + sigil: '@', + mention: "figma".to_string(), + path: "plugin://figma@test".to_string(), + }, + MentionBinding { + sigil: '$', + mention: "figma".to_string(), + path: "app://figma".to_string(), + }, + ]; + composer.set_text_content_with_mention_bindings( + "@figma then $figma".to_string(), + Vec::new(), + Vec::new(), + mention_bindings.clone(), + ); + + let bound_tokens = composer + .current_mention_elements() + .into_iter() + .map(|(_, sigil, mention)| (sigil, mention)) + .collect::>(); + assert_eq!( + bound_tokens, + vec![('@', "figma".to_string()), ('$', "figma".to_string())] + ); + assert_eq!(composer.mention_bindings(), mention_bindings); + } + + #[test] + fn set_text_content_rebinds_at_mentions_after_email_substrings() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + + let text = "foo@sample.com then @sample".to_string(); + let mention_start = text.rfind("@sample").expect("expected bound mention token"); + let mention_range = mention_start..mention_start + "@sample".len(); + let mention_bindings = vec![MentionBinding { + sigil: '@', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }]; + composer.set_text_content_with_mention_bindings( + text, + Vec::new(), + Vec::new(), + mention_bindings.clone(), + ); + + // Fix coverage: only the plaintext `@sample` should be atomic; the email substring stays editable. + assert_eq!( + composer + .draft + .textarea + .text_element_snapshots() + .into_iter() + .map(|snapshot| snapshot.range) + .collect::>(), + vec![mention_range] + ); + assert_eq!(composer.mention_bindings(), mention_bindings); + } + + #[test] + fn set_text_content_rebinds_at_mentions_after_punctuation() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + + let text = "Please ask (@sample)".to_string(); + let mention_start = text.find("@sample").expect("expected bound mention token"); + let mention_range = mention_start..mention_start + "@sample".len(); + let mention_bindings = vec![MentionBinding { + sigil: '@', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }]; + composer.set_text_content_with_mention_bindings( + text, + Vec::new(), + Vec::new(), + mention_bindings.clone(), + ); + + assert_eq!( + composer + .draft + .textarea + .text_element_snapshots() + .into_iter() + .map(|snapshot| snapshot.range) + .collect::>(), + vec![mention_range] + ); + assert_eq!(composer.mention_bindings(), mention_bindings); + } + + #[test] + fn bound_at_mentions_do_not_block_arrow_navigation() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + + composer.set_text_content_with_mention_bindings( + "go @figma now".to_string(), + Vec::new(), + Vec::new(), + vec![MentionBinding { + sigil: '@', + mention: "figma".to_string(), + path: "plugin://figma@debug".to_string(), + }], + ); + composer.draft.textarea.set_cursor("go".len()); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Right, KeyModifiers::NONE)); + assert_eq!(composer.draft.textarea.cursor(), "go".len() + 1); + assert!(matches!(composer.popups.active, ActivePopup::None)); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Right, KeyModifiers::NONE)); + assert_eq!(composer.draft.textarea.cursor(), "go @figma".len()); + assert!(matches!(composer.popups.active, ActivePopup::None)); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Left, KeyModifiers::NONE)); + assert_eq!(composer.draft.textarea.cursor(), "go ".len()); + assert!(matches!(composer.popups.active, ActivePopup::None)); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Left, KeyModifiers::NONE)); + assert_eq!(composer.draft.textarea.cursor(), "go".len()); + assert!(matches!(composer.popups.active, ActivePopup::None)); + } + + #[test] + fn restored_bound_at_mentions_do_not_open_mention_popup() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + for (text, move_cursor_to_end) in [ + ("@sample".to_string(), false), + ("Please ask @sample.".to_string(), true), + ] { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer.set_plugin_mentions(Some(vec![PluginCapabilitySummary { + config_name: "sample@test".to_string(), + display_name: "sample".to_string(), + description: None, + has_skills: true, + mcp_server_names: vec!["sample".to_string()], + app_connector_ids: Vec::new(), + }])); + + composer.set_text_content_with_mention_bindings( + text.clone(), + Vec::new(), + Vec::new(), + vec![MentionBinding { + sigil: '@', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }], + ); + if move_cursor_to_end { + composer.move_cursor_to_end(); + } + + assert!(matches!(composer.popups.active, ActivePopup::None)); + + let (result, consumed) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert!(consumed); + match result { + InputResult::Submitted { + text: submitted, .. + } => assert_eq!(submitted, text), + _ => panic!("expected restored bound mention to submit"), + } + } + } + #[test] fn enter_submits_when_file_popup_has_no_selection() { use crossterm::event::KeyCode; @@ -6478,6 +7022,39 @@ mod tests { } } + #[test] + fn enter_submits_when_unified_mention_popup_has_no_selection() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer.set_mentions_v2_enabled(/*enabled*/ true); + + let input = "npx -y @kaeawc/auto-mobile@latest"; + composer.draft.textarea.insert_str(input); + composer.draft.textarea.set_cursor(input.len()); + composer.sync_popups(); + + assert!(matches!(composer.popups.active, ActivePopup::MentionV2(_))); + + let (result, consumed) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert!(consumed); + match result { + InputResult::Submitted { text, .. } => assert_eq!(text, input), + _ => panic!("expected Submitted"), + } + } + /// Behavior: if the ASCII path has a pending first char (flicker suppression) and a non-ASCII /// char arrives next, the pending ASCII char should still be preserved and the overall input /// should submit normally (i.e. we should not misclassify this as a paste burst). @@ -8783,6 +9360,7 @@ mod tests { ); let mention_bindings = vec![MentionBinding { + sigil: '$', mention: "figma".to_string(), path: "/tmp/user/figma/SKILL.md".to_string(), }]; diff --git a/codex-rs/tui/src/bottom_pane/chat_composer/draft_state.rs b/codex-rs/tui/src/bottom_pane/chat_composer/draft_state.rs index f739f10ec9..b219097ee1 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer/draft_state.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer/draft_state.rs @@ -40,6 +40,7 @@ impl DraftState { #[derive(Clone, Debug)] pub(super) struct ComposerMentionBinding { + pub(super) sigil: char, pub(super) mention: String, pub(super) path: String, } diff --git a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs index 6a490e81ec..23b553fa08 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs @@ -18,7 +18,7 @@ use std::path::PathBuf; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::MentionBinding; -use crate::mention_codec::decode_history_mentions; +use crate::mention_codec::decode_history_mentions_with_at_mentions; use codex_protocol::ThreadId; use codex_protocol::user_input::TextElement; @@ -47,7 +47,11 @@ impl HistoryEntry { /// recorded with the full `HistoryEntry` value built by the composer; using `new` for a local /// image or paste submission would make recall lose placeholder ownership. pub(crate) fn new(text: String) -> Self { - let decoded = decode_history_mentions(&text); + Self::new_with_at_mentions(text, /*at_mentions_enabled*/ true) + } + + pub(crate) fn new_with_at_mentions(text: String, at_mentions_enabled: bool) -> Self { + let decoded = decode_history_mentions_with_at_mentions(&text, at_mentions_enabled); Self { text: decoded.text, text_elements: Vec::new(), @@ -57,6 +61,7 @@ impl HistoryEntry { .mentions .into_iter() .map(|mention| MentionBinding { + sigil: mention.sigil, mention: mention.mention, path: mention.path, }) @@ -132,6 +137,8 @@ pub(crate) struct ChatComposerHistory { /// Active incremental history search, if Ctrl+R search mode is open. search: Option, + /// Whether persistent history restore should rehydrate `@` tool mentions. + at_mention_restore_enabled: bool, } #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -226,9 +233,21 @@ impl ChatComposerHistory { history_cursor: None, last_history_text: None, search: None, + at_mention_restore_enabled: false, } } + pub fn set_at_mention_restore_enabled(&mut self, enabled: bool) { + if self.at_mention_restore_enabled == enabled { + return; + } + self.at_mention_restore_enabled = enabled; + self.fetched_history.clear(); + self.history_cursor = None; + self.last_history_text = None; + self.search = None; + } + /// Updates persistent history metadata when a new session is configured. /// /// This clears fetched entries, local entries, navigation cursors, and active search state @@ -393,7 +412,9 @@ impl ChatComposerHistory { return HistoryEntryResponse::Ignored; } - let entry = entry.map(HistoryEntry::new); + let entry = entry.map(|entry| { + HistoryEntry::new_with_at_mentions(entry, self.at_mention_restore_enabled) + }); if let Some(entry) = entry.clone() { self.fetched_history.insert(offset, entry); } @@ -840,6 +861,75 @@ mod tests { ); } + #[test] + fn persistent_restore_gates_at_mentions() { + let (tx, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + let mut history = ChatComposerHistory::new(); + history.set_metadata(test_thread_id(), /*log_id*/ 42, /*entry_count*/ 1); + + assert!(history.navigate_up(&tx).is_none()); + let disabled = history.on_entry_response( + /*log_id*/ 42, + /*offset*/ 0, + Some("[@sample](plugin://sample@test) and [$figma](app://figma)".to_string()), + &tx, + ); + assert_eq!( + disabled, + HistoryEntryResponse::Found(HistoryEntry { + text: "$sample and $figma".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), + mention_bindings: vec![ + MentionBinding { + sigil: '$', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }, + MentionBinding { + sigil: '$', + mention: "figma".to_string(), + path: "app://figma".to_string(), + }, + ], + pending_pastes: Vec::new(), + }) + ); + + history.set_at_mention_restore_enabled(/*enabled*/ true); + assert!(history.navigate_up(&tx).is_none()); + let enabled = history.on_entry_response( + /*log_id*/ 42, + /*offset*/ 0, + Some("[@sample](plugin://sample@test) and [$figma](app://figma)".to_string()), + &tx, + ); + assert_eq!( + enabled, + HistoryEntryResponse::Found(HistoryEntry { + text: "@sample and $figma".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), + mention_bindings: vec![ + MentionBinding { + sigil: '@', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }, + MentionBinding { + sigil: '$', + mention: "figma".to_string(), + path: "app://figma".to_string(), + }, + ], + pending_pastes: Vec::new(), + }) + ); + } + #[test] fn navigation_with_async_fetch() { let (tx, mut rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/bottom_pane/mentions_v2/search_catalog.rs b/codex-rs/tui/src/bottom_pane/mentions_v2/search_catalog.rs index b1dd7f914b..a555070a86 100644 --- a/codex-rs/tui/src/bottom_pane/mentions_v2/search_catalog.rs +++ b/codex-rs/tui/src/bottom_pane/mentions_v2/search_catalog.rs @@ -50,6 +50,7 @@ fn plugin_candidate(plugin: &PluginCapabilitySummary) -> Candidate { .config_name .split_once('@') .unwrap_or((plugin.config_name.as_str(), "")); + let mention_name = plugin_mention_name(plugin_name, plugin.display_name.as_str()); let mut search_terms = vec![plugin_name.to_string(), plugin.config_name.clone()]; if plugin.display_name != plugin_name { search_terms.push(plugin.display_name.clone()); @@ -64,12 +65,88 @@ fn plugin_candidate(plugin: &PluginCapabilitySummary) -> Candidate { search_terms, mention_type: MentionType::Plugin, selection: Selection::Tool { - insert_text: format!("${plugin_name}"), + insert_text: format!("@{mention_name}"), path: Some(format!("plugin://{}", plugin.config_name)), }, } } +fn plugin_mention_name(plugin_name: &str, display_name: &str) -> String { + let plugin_segments = split_plugin_name_segments(plugin_name); + let display_segments = split_display_name_segments(display_name); + + if plugin_segments.len() == display_segments.len() + && plugin_segments.iter().zip(&display_segments).all( + |((plugin_segment, _), display_segment)| { + plugin_segment.eq_ignore_ascii_case(display_segment.as_str()) + }, + ) + { + let mut result = String::new(); + for ((_, separator), display_segment) in plugin_segments.into_iter().zip(display_segments) { + result.push_str(display_segment.as_str()); + if let Some(separator) = separator { + result.push(separator); + } + } + return result; + } + + title_case_plugin_name(plugin_name) +} + +fn split_plugin_name_segments(plugin_name: &str) -> Vec<(String, Option)> { + let mut segments = Vec::new(); + let mut current = String::new(); + + for ch in plugin_name.chars() { + if matches!(ch, '-' | '_') { + if !current.is_empty() { + segments.push((std::mem::take(&mut current), Some(ch))); + } + } else { + current.push(ch); + } + } + + if !current.is_empty() { + segments.push((current, None)); + } + + segments +} + +fn split_display_name_segments(display_name: &str) -> Vec { + display_name + .split(|ch: char| !ch.is_ascii_alphanumeric()) + .filter(|segment| !segment.is_empty()) + .map(ToString::to_string) + .collect() +} + +fn title_case_plugin_name(plugin_name: &str) -> String { + let mut result = String::with_capacity(plugin_name.len()); + let mut capitalize_next = true; + + for ch in plugin_name.chars() { + if matches!(ch, '-' | '_') { + capitalize_next = true; + result.push(ch); + continue; + } + + if capitalize_next && ch.is_ascii_alphabetic() { + result.push(ch.to_ascii_uppercase()); + capitalize_next = false; + } else { + result.push(ch); + capitalize_next = false; + } + } + + result +} + fn plugin_description(plugin: &PluginCapabilitySummary) -> Option { let capability_labels = plugin_capability_labels(plugin); plugin.description.clone().or_else(|| { @@ -109,3 +186,30 @@ fn optional_skill_description(skill: &SkillMetadata) -> Option { let description = skill_description(skill).trim(); (!description.is_empty()).then(|| description.to_string()) } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn plugin_mention_name_uses_display_segments_when_they_match_plugin_name() { + assert_eq!( + plugin_mention_name("mcp-search", "MCP Search"), + "MCP-Search" + ); + assert_eq!( + plugin_mention_name("google_calendar", "Google Calendar"), + "Google_Calendar" + ); + } + + #[test] + fn plugin_mention_name_falls_back_to_title_cased_plugin_name() { + assert_eq!(plugin_mention_name("sample", "Sample Plugin"), "Sample"); + assert_eq!( + plugin_mention_name("browser-use", "Browser Use"), + "Browser-Use" + ); + } +} diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 1fcc7635c0..7ef385570e 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -83,7 +83,9 @@ pub(crate) struct LocalImageAttachment { #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct MentionBinding { - /// Mention token text without the leading `$`. + /// Visible mention sigil (`$` or `@`). + pub(crate) sigil: char, + /// Mention token text without the leading sigil (`$` or `@`). pub(crate) mention: String, /// Canonical mention target (for example `app://...` or absolute SKILL.md path). pub(crate) path: String, diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__plugin_at_mentions_render_with_plugin_accent.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__plugin_at_mentions_render_with_plugin_accent.snap new file mode 100644 index 0000000000..0f6c47dca0 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__plugin_at_mentions_render_with_plugin_accent.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: "format!(\"text: {text}\\nmagenta: {magenta}\")" +--- +text: › @sample plugin +magenta: ^^^^^^^ diff --git a/codex-rs/tui/src/chatwidget/input_submission.rs b/codex-rs/tui/src/chatwidget/input_submission.rs index 4700ffc0cc..578a35728f 100644 --- a/codex-rs/tui/src/chatwidget/input_submission.rs +++ b/codex-rs/tui/src/chatwidget/input_submission.rs @@ -363,6 +363,7 @@ impl ChatWidget { let encoded_mentions = mention_bindings .iter() .map(|binding| LinkedMention { + sigil: binding.sigil, mention: binding.mention.clone(), path: binding.path.clone(), }) diff --git a/codex-rs/tui/src/chatwidget/tests/composer_submission.rs b/codex-rs/tui/src/chatwidget/tests/composer_submission.rs index 23487af437..27ae4daff2 100644 --- a/codex-rs/tui/src/chatwidget/tests/composer_submission.rs +++ b/codex-rs/tui/src/chatwidget/tests/composer_submission.rs @@ -570,6 +570,7 @@ async fn submission_prefers_selected_duplicate_skill_path() { Vec::new(), Vec::new(), vec![MentionBinding { + sigil: '$', mention: "figma".to_string(), path: user_skill_path.to_string_lossy().into_owned(), }], @@ -605,6 +606,7 @@ async fn blocked_image_restore_preserves_mention_bindings() { path: PathBuf::from("/tmp/blocked.png"), }]; let mention_bindings = vec![MentionBinding { + sigil: '$', mention: "file".to_string(), path: "/tmp/skills/file/SKILL.md".to_string(), }]; @@ -1236,6 +1238,7 @@ async fn submit_user_message_ignores_inaccessible_app_mentions_from_bindings() { remote_image_urls: Vec::new(), text_elements: Vec::new(), mention_bindings: vec![MentionBinding { + sigil: '$', mention: "arabica-uae".to_string(), path: "app://arabica_uae".to_string(), }], diff --git a/codex-rs/tui/src/chatwidget/tests/plan_mode.rs b/codex-rs/tui/src/chatwidget/tests/plan_mode.rs index 5fc2fa2ce9..74cd527038 100644 --- a/codex-rs/tui/src/chatwidget/tests/plan_mode.rs +++ b/codex-rs/tui/src/chatwidget/tests/plan_mode.rs @@ -1244,6 +1244,7 @@ async fn submit_user_message_emits_structured_plugin_mentions_from_bindings() { remote_image_urls: Vec::new(), text_elements: Vec::new(), mention_bindings: vec![MentionBinding { + sigil: '$', mention: "sample".to_string(), path: "plugin://sample@test".to_string(), }], diff --git a/codex-rs/tui/src/chatwidget/tests/review_mode.rs b/codex-rs/tui/src/chatwidget/tests/review_mode.rs index 48bc341f6b..6281740724 100644 --- a/codex-rs/tui/src/chatwidget/tests/review_mode.rs +++ b/codex-rs/tui/src/chatwidget/tests/review_mode.rs @@ -875,6 +875,7 @@ async fn manual_interrupt_restores_pending_steer_mention_bindings_to_composer() chat.on_agent_message_delta("Final answer line\n".to_string()); let mention_bindings = vec![MentionBinding { + sigil: '$', mention: "figma".to_string(), path: "/tmp/skills/figma/SKILL.md".to_string(), }]; diff --git a/codex-rs/tui/src/chatwidget/tests/slash_commands.rs b/codex-rs/tui/src/chatwidget/tests/slash_commands.rs index 0346b97069..ff1feb084c 100644 --- a/codex-rs/tui/src/chatwidget/tests/slash_commands.rs +++ b/codex-rs/tui/src/chatwidget/tests/slash_commands.rs @@ -656,6 +656,7 @@ async fn goal_slash_command_uses_plain_text_for_mentions() { Vec::new(), Vec::new(), vec![MentionBinding { + sigil: '$', mention: "figma".to_string(), path: "app://figma".to_string(), }], @@ -917,6 +918,7 @@ fn merged_history_record_preserves_raw_text_and_rebased_elements() { remote_image_urls: Vec::new(), text_elements: vec![TextElement::new((4..10).into(), Some("$figma".to_string()))], mention_bindings: vec![MentionBinding { + sigil: '$', mention: "figma".to_string(), path: "app://figma".to_string(), }], @@ -1034,6 +1036,7 @@ async fn interrupted_merged_message_history_encodes_mentions_once() { Vec::new(), Vec::new(), vec![MentionBinding { + sigil: '$', mention: "figma".to_string(), path: "app://figma".to_string(), }], diff --git a/codex-rs/tui/src/mention_codec.rs b/codex-rs/tui/src/mention_codec.rs index f6cd38bd4f..520a6712ea 100644 --- a/codex-rs/tui/src/mention_codec.rs +++ b/codex-rs/tui/src/mention_codec.rs @@ -6,6 +6,7 @@ use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL; #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct LinkedMention { + pub(crate) sigil: char, pub(crate) mention: String, pub(crate) path: String, } @@ -22,10 +23,16 @@ pub(crate) fn encode_history_mentions(text: &str, mentions: &[LinkedMention]) -> return text.to_string(); } - let mut mentions_by_name: HashMap<&str, VecDeque<&str>> = HashMap::new(); + let mut mentions_by_token: HashMap<(char, &str), VecDeque<&str>> = HashMap::new(); for mention in mentions { - mentions_by_name - .entry(mention.mention.as_str()) + if !matches!( + mention.sigil, + TOOL_MENTION_SIGIL | PLUGIN_TEXT_MENTION_SIGIL + ) { + continue; + } + mentions_by_token + .entry((mention.sigil, mention.mention.as_str())) .or_default() .push_back(mention.path.as_str()); } @@ -35,28 +42,38 @@ pub(crate) fn encode_history_mentions(text: &str, mentions: &[LinkedMention]) -> let mut index = 0usize; while index < bytes.len() { - if bytes[index] == TOOL_MENTION_SIGIL as u8 { - let name_start = index + 1; - if let Some(first) = bytes.get(name_start) - && is_mention_name_char(*first) - { - let mut name_end = name_start + 1; - while let Some(next) = bytes.get(name_end) - && is_mention_name_char(*next) + if matches!( + bytes[index], + byte if byte == TOOL_MENTION_SIGIL as u8 || byte == PLUGIN_TEXT_MENTION_SIGIL as u8 + ) { + let sigil = bytes[index] as char; + if sigil == TOOL_MENTION_SIGIL || starts_plaintext_mention(text, index) { + let name_start = index + 1; + if let Some(first) = bytes.get(name_start) + && is_mention_name_char(*first) { - name_end += 1; - } + let mut name_end = name_start + 1; + while let Some(next) = bytes.get(name_end) + && is_mention_name_char(*next) + { + name_end += 1; + } - let name = &text[name_start..name_end]; - if let Some(path) = mentions_by_name.get_mut(name).and_then(VecDeque::pop_front) { - out.push('['); - out.push(TOOL_MENTION_SIGIL); - out.push_str(name); - out.push_str("]("); - out.push_str(path); - out.push(')'); - index = name_end; - continue; + let name = &text[name_start..name_end]; + if (sigil == TOOL_MENTION_SIGIL || ends_plaintext_mention(bytes, name_end)) + && let Some(path) = mentions_by_token + .get_mut(&(sigil, name)) + .and_then(VecDeque::pop_front) + { + out.push('['); + out.push(sigil); + out.push_str(name); + out.push_str("]("); + out.push_str(path); + out.push(')'); + index = name_end; + continue; + } } } } @@ -71,7 +88,10 @@ pub(crate) fn encode_history_mentions(text: &str, mentions: &[LinkedMention]) -> out } -pub(crate) fn decode_history_mentions(text: &str) -> DecodedHistoryText { +pub(crate) fn decode_history_mentions_with_at_mentions( + text: &str, + at_mentions_enabled: bool, +) -> DecodedHistoryText { let bytes = text.as_bytes(); let mut out = String::with_capacity(text.len()); let mut mentions = Vec::new(); @@ -79,11 +99,13 @@ pub(crate) fn decode_history_mentions(text: &str) -> DecodedHistoryText { while index < bytes.len() { if bytes[index] == b'[' - && let Some((name, path, end_index)) = parse_history_linked_mention(text, bytes, index) + && let Some((sigil, name, path, end_index)) = + parse_history_linked_mention(text, bytes, index, at_mentions_enabled) { - out.push(TOOL_MENTION_SIGIL); + out.push(sigil); out.push_str(name); mentions.push(LinkedMention { + sigil, mention: name.to_string(), path: path.to_string(), }); @@ -108,22 +130,32 @@ fn parse_history_linked_mention<'a>( text: &'a str, text_bytes: &[u8], start: usize, -) -> Option<(&'a str, &'a str, usize)> { - // TUI writes `$name`, but may read plugin `[@name](plugin://...)` links from other clients. - if let Some(mention @ (name, path, _)) = + at_mentions_enabled: bool, +) -> Option<(char, &'a str, &'a str, usize)> { + // TUI historically wrote `$name`, but selected unified `@` mentions should preserve `@` on + // history round-trip for any canonical tool path. + if let Some((name, path, end_index)) = parse_linked_tool_mention(text, text_bytes, start, TOOL_MENTION_SIGIL) && !is_common_env_var(name) && is_tool_path(path) { - return Some(mention); + return Some((TOOL_MENTION_SIGIL, name, path, end_index)); } - if let Some(mention @ (name, path, _)) = + if at_mentions_enabled { + if let Some((name, path, end_index)) = + parse_linked_tool_mention(text, text_bytes, start, PLUGIN_TEXT_MENTION_SIGIL) + && !is_common_env_var(name) + && is_tool_path(path) + { + return Some((PLUGIN_TEXT_MENTION_SIGIL, name, path, end_index)); + } + } else if let Some((name, path, end_index)) = parse_linked_tool_mention(text, text_bytes, start, PLUGIN_TEXT_MENTION_SIGIL) && !is_common_env_var(name) && path.starts_with("plugin://") { - return Some(mention); + return Some((TOOL_MENTION_SIGIL, name, path, end_index)); } None @@ -190,6 +222,35 @@ fn is_mention_name_char(byte: u8) -> bool { matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'_' | b'-') } +fn starts_plaintext_mention(text: &str, index: usize) -> bool { + if index == 0 { + return true; + } + + text.get(..index) + .and_then(|prefix| prefix.chars().next_back()) + .is_some_and(|ch| ch.is_whitespace() || !is_mention_name_char_char(ch)) +} + +fn ends_plaintext_mention(text_bytes: &[u8], index: usize) -> bool { + text_bytes.get(index).is_none_or(|byte| { + byte.is_ascii_whitespace() + || *byte == b'.' + && text_bytes.get(index + 1).is_none_or(|next| { + next.is_ascii_whitespace() + || !next.is_ascii_alphanumeric() && *next != b'_' && *next != b'-' + }) + || !matches!(*byte, b'.' | b'/' | b'\\') + && !byte.is_ascii_alphanumeric() + && *byte != b'_' + && *byte != b'-' + }) +} + +fn is_mention_name_char_char(ch: char) -> bool { + ch.is_ascii_alphanumeric() || matches!(ch, '_' | '-') +} + fn is_common_env_var(name: &str) -> bool { let upper = name.to_ascii_uppercase(); matches!( @@ -226,22 +287,26 @@ mod tests { #[test] fn decode_history_mentions_restores_visible_tokens() { - let decoded = decode_history_mentions( + let decoded = decode_history_mentions_with_at_mentions( "Use [$figma](app://figma-1), [$sample](plugin://sample@test), and [$figma](/tmp/figma/SKILL.md).", + /*at_mentions_enabled*/ true, ); assert_eq!(decoded.text, "Use $figma, $sample, and $figma."); assert_eq!( decoded.mentions, vec![ LinkedMention { + sigil: '$', mention: "figma".to_string(), path: "app://figma-1".to_string(), }, LinkedMention { + sigil: '$', mention: "sample".to_string(), path: "plugin://sample@test".to_string(), }, LinkedMention { + sigil: '$', mention: "figma".to_string(), path: "/tmp/figma/SKILL.md".to_string(), }, @@ -251,18 +316,21 @@ mod tests { #[test] fn decode_history_mentions_restores_plugin_links_with_at_sigil() { - let decoded = decode_history_mentions( + let decoded = decode_history_mentions_with_at_mentions( "Use [@sample](plugin://sample@test) and [$figma](app://figma-1).", + /*at_mentions_enabled*/ true, ); - assert_eq!(decoded.text, "Use $sample and $figma."); + assert_eq!(decoded.text, "Use @sample and $figma."); assert_eq!( decoded.mentions, vec![ LinkedMention { + sigil: '@', mention: "sample".to_string(), path: "plugin://sample@test".to_string(), }, LinkedMention { + sigil: '$', mention: "figma".to_string(), path: "app://figma-1".to_string(), }, @@ -271,13 +339,58 @@ mod tests { } #[test] - fn decode_history_mentions_ignores_at_sigil_for_non_plugin_paths() { - let decoded = decode_history_mentions("Use [@figma](app://figma-1)."); + fn decode_history_mentions_without_at_mentions_uses_legacy_plugin_fallback() { + let decoded = decode_history_mentions_with_at_mentions( + "Use [@sample](plugin://sample@test) and [$figma](app://figma-1).", + /*at_mentions_enabled*/ false, + ); + assert_eq!(decoded.text, "Use $sample and $figma."); + assert_eq!( + decoded.mentions, + vec![ + LinkedMention { + sigil: '$', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }, + LinkedMention { + sigil: '$', + mention: "figma".to_string(), + path: "app://figma-1".to_string(), + }, + ] + ); + } + + #[test] + fn decode_history_mentions_without_at_mentions_ignores_at_non_plugin_paths() { + let decoded = decode_history_mentions_with_at_mentions( + "Use [@figma](app://figma-1).", + /*at_mentions_enabled*/ false, + ); assert_eq!(decoded.text, "Use [@figma](app://figma-1)."); assert_eq!(decoded.mentions, Vec::::new()); } + #[test] + fn decode_history_mentions_restores_at_sigil_for_tool_paths() { + let decoded = decode_history_mentions_with_at_mentions( + "Use [@figma](app://figma-1).", + /*at_mentions_enabled*/ true, + ); + + assert_eq!(decoded.text, "Use @figma."); + assert_eq!( + decoded.mentions, + vec![LinkedMention { + sigil: '@', + mention: "figma".to_string(), + path: "app://figma-1".to_string(), + }] + ); + } + #[test] fn encode_history_mentions_links_bound_mentions_in_order() { let text = "$figma then $sample then $figma then $other"; @@ -285,14 +398,17 @@ mod tests { text, &[ LinkedMention { + sigil: '$', mention: "figma".to_string(), path: "app://figma-app".to_string(), }, LinkedMention { + sigil: '$', mention: "sample".to_string(), path: "plugin://sample@test".to_string(), }, LinkedMention { + sigil: '$', mention: "figma".to_string(), path: "/tmp/figma/SKILL.md".to_string(), }, @@ -303,4 +419,161 @@ mod tests { "[$figma](app://figma-app) then [$sample](plugin://sample@test) then [$figma](/tmp/figma/SKILL.md) then $other" ); } + + #[test] + fn encode_history_mentions_links_dollar_mentions_after_punctuation() { + let encoded = encode_history_mentions( + "($figma)", + &[LinkedMention { + sigil: '$', + mention: "figma".to_string(), + path: "app://figma".to_string(), + }], + ); + assert_eq!(encoded, "([$figma](app://figma))"); + } + + #[test] + fn encode_history_mentions_links_dollar_mentions_with_path_like_suffixes() { + let mention = LinkedMention { + sigil: '$', + mention: "figma".to_string(), + path: "app://figma".to_string(), + }; + + assert_eq!( + encode_history_mentions("$figma/docs", std::slice::from_ref(&mention)), + "[$figma](app://figma)/docs" + ); + assert_eq!( + encode_history_mentions("$figma.suffix", std::slice::from_ref(&mention)), + "[$figma](app://figma).suffix" + ); + assert_eq!( + encode_history_mentions("$figma\\docs", &[mention]), + "[$figma](app://figma)\\docs" + ); + } + + #[test] + fn encode_history_mentions_preserves_at_sigils() { + let text = "@figma then @sample then $other"; + let encoded = encode_history_mentions( + text, + &[ + LinkedMention { + sigil: '@', + mention: "figma".to_string(), + path: "/tmp/figma/SKILL.md".to_string(), + }, + LinkedMention { + sigil: '@', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }, + ], + ); + assert_eq!( + encoded, + "[@figma](/tmp/figma/SKILL.md) then [@sample](plugin://sample@test) then $other" + ); + } + + #[test] + fn encode_history_mentions_links_both_sigils_for_same_name() { + let text = "@figma then $figma"; + let encoded = encode_history_mentions( + text, + &[ + LinkedMention { + sigil: '@', + mention: "figma".to_string(), + path: "plugin://figma@test".to_string(), + }, + LinkedMention { + sigil: '$', + mention: "figma".to_string(), + path: "app://figma".to_string(), + }, + ], + ); + assert_eq!( + encoded, + "[@figma](plugin://figma@test) then [$figma](app://figma)" + ); + } + + #[test] + fn encode_history_mentions_does_not_let_at_token_steal_later_tool_binding() { + let text = "@figma then $figma"; + let encoded = encode_history_mentions( + text, + &[LinkedMention { + sigil: '$', + mention: "figma".to_string(), + path: "app://figma-app".to_string(), + }], + ); + assert_eq!(encoded, "@figma then [$figma](app://figma-app)"); + } + + #[test] + fn encode_history_mentions_links_at_mentions_after_unicode_whitespace() { + // Fix coverage: full-width space should remain a valid plaintext boundary for `@` links. + let text = "foo　@sample"; + let encoded = encode_history_mentions( + text, + &[LinkedMention { + sigil: '@', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }], + ); + assert_eq!(encoded, "foo　[@sample](plugin://sample@test)"); + } + + #[test] + fn encode_history_mentions_links_sentence_ending_at_mentions() { + let text = "Please ask @figma."; + let encoded = encode_history_mentions( + text, + &[LinkedMention { + sigil: '@', + mention: "figma".to_string(), + path: "/tmp/figma/SKILL.md".to_string(), + }], + ); + assert_eq!(encoded, "Please ask [@figma](/tmp/figma/SKILL.md)."); + } + + #[test] + fn encode_history_mentions_links_parenthesized_at_mentions() { + let text = "Please ask (@figma)"; + let encoded = encode_history_mentions( + text, + &[LinkedMention { + sigil: '@', + mention: "figma".to_string(), + path: "plugin://figma@test".to_string(), + }], + ); + assert_eq!(encoded, "Please ask ([@figma](plugin://figma@test))"); + } + + #[test] + fn encode_history_mentions_skips_embedded_at_substrings() { + let text = "foo@sample.com npx @sample/pkg then @sample"; + let encoded = encode_history_mentions( + text, + &[LinkedMention { + sigil: '@', + mention: "sample".to_string(), + path: "plugin://sample@test".to_string(), + }], + ); + assert_eq!( + encoded, + "foo@sample.com npx @sample/pkg then [@sample](plugin://sample@test)" + ); + } } From c2508db60db0eae343d12a586dcfbc37e491f122 Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Thu, 28 May 2026 10:49:41 -0700 Subject: [PATCH 05/10] Revert "Add app-server startup benchmark crate" (#24937) Reverts openai/codex#24651, broke musl job https://github.com/openai/codex/actions/runs/26585495205/job/78330166927 --- .github/workflows/rust-ci-full.yml | 7 - codex-rs/Cargo.lock | 11 -- codex-rs/Cargo.toml | 1 - .../benchmarks/app-server-start/BUILD.bazel | 20 -- .../benchmarks/app-server-start/Cargo.toml | 16 -- .../benchmarks/app-server-start/src/main.rs | 174 ------------------ justfile | 22 +-- 7 files changed, 4 insertions(+), 247 deletions(-) delete mode 100644 codex-rs/benchmarks/app-server-start/BUILD.bazel delete mode 100644 codex-rs/benchmarks/app-server-start/Cargo.toml delete mode 100644 codex-rs/benchmarks/app-server-start/src/main.rs diff --git a/.github/workflows/rust-ci-full.yml b/.github/workflows/rust-ci-full.yml index 3031cb6853..60d508b21d 100644 --- a/.github/workflows/rust-ci-full.yml +++ b/.github/workflows/rust-ci-full.yml @@ -402,13 +402,6 @@ jobs: - name: cargo clippy run: cargo clippy --target ${{ matrix.target }} --tests --profile ${{ matrix.profile }} --timings -- -D warnings - - uses: taiki-e/install-action@44c6d64aa62cd779e873306675c7a58e86d6d532 # v2.62.49 - with: - tool: just - - - name: End-to-end benchmark smoke test - run: just bench-e2e-smoke - - name: Upload Cargo timings (clippy) if: always() uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 21f8af0d6e..aac0d6f90c 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2058,17 +2058,6 @@ dependencies = [ "uuid", ] -[[package]] -name = "codex-app-server-start-bench" -version = "0.0.0" -dependencies = [ - "anyhow", - "codex-app-server-protocol", - "divan", - "serde_json", - "tempfile", -] - [[package]] name = "codex-app-server-test-client" version = "0.0.0" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 5ef6a45fc3..b1f9e4c31a 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -14,7 +14,6 @@ members = [ "app-server-client", "app-server-protocol", "app-server-test-client", - "benchmarks/app-server-start", "debug-client", "apply-patch", "arg0", diff --git a/codex-rs/benchmarks/app-server-start/BUILD.bazel b/codex-rs/benchmarks/app-server-start/BUILD.bazel deleted file mode 100644 index 4b8f09413e..0000000000 --- a/codex-rs/benchmarks/app-server-start/BUILD.bazel +++ /dev/null @@ -1,20 +0,0 @@ -load("//:defs.bzl", "codex_rust_crate", "workspace_root_test") - -codex_rust_crate( - name = "app-server-start-bench", - crate_name = "codex_app_server_start_bench", -) - -workspace_root_test( - name = "startup-bench", - # Include the Bazel-built CLI in this target's runfiles and expose its - # resolved executable path through the environment consumed by the runner. - runfile_env = { - "//codex-rs/cli:codex": "CODEX_BIN", - }, - size = "large", - tags = ["manual"], - test_bin = ":codex-app-server-start-bench", - timeout = "long", - workspace_root_marker = "//codex-rs/utils/cargo-bin:repo_root.marker", -) diff --git a/codex-rs/benchmarks/app-server-start/Cargo.toml b/codex-rs/benchmarks/app-server-start/Cargo.toml deleted file mode 100644 index edf680eeea..0000000000 --- a/codex-rs/benchmarks/app-server-start/Cargo.toml +++ /dev/null @@ -1,16 +0,0 @@ -[package] -name = "codex-app-server-start-bench" -version.workspace = true -edition.workspace = true -license.workspace = true -publish = false - -[lints] -workspace = true - -[dependencies] -anyhow = { workspace = true } -codex-app-server-protocol = { workspace = true } -serde_json = { workspace = true } -divan = { workspace = true } -tempfile = { workspace = true } diff --git a/codex-rs/benchmarks/app-server-start/src/main.rs b/codex-rs/benchmarks/app-server-start/src/main.rs deleted file mode 100644 index 5363b49ffc..0000000000 --- a/codex-rs/benchmarks/app-server-start/src/main.rs +++ /dev/null @@ -1,174 +0,0 @@ -use std::io::BufRead; -use std::io::BufReader; -use std::io::Write; -use std::path::Path; -use std::path::PathBuf; -use std::process::Child; -use std::process::ChildStdin; -use std::process::ChildStdout; -use std::process::Command; -use std::process::Stdio; - -use anyhow::Context; -use anyhow::Result; -use anyhow::bail; -use codex_app_server_protocol::ClientInfo; -use codex_app_server_protocol::ClientRequest; -use codex_app_server_protocol::InitializeCapabilities; -use codex_app_server_protocol::InitializeParams; -use codex_app_server_protocol::InitializeResponse; -use codex_app_server_protocol::JSONRPCMessage; -use codex_app_server_protocol::JSONRPCNotification; -use codex_app_server_protocol::JSONRPCResponse; -use codex_app_server_protocol::RequestId; -use divan::Bencher; - -fn main() { - divan::main(); -} - -// Process startup is slow enough that 30 samples keep runs practical, and -// sample size 1 lets each measured server be reaped before the next starts. -// The benchmark warms one CODEX_HOME before timing to measure normal restarts. -// This e2e runner receives its separately built Codex binary from just or Bazel. -#[divan::bench(sample_count = 30, sample_size = 1, skip_ext_time)] -#[allow(clippy::expect_used)] -fn initialize_response(bencher: Bencher) { - let codex_bin = std::env::var_os("CODEX_BIN") - .map(PathBuf::from) - .expect("CODEX_BIN must point to the codex binary; run via `just bench-e2e` or Bazel"); - let codex_home = tempfile::tempdir().expect("benchmark CODEX_HOME should be created"); - drop( - start_until_initialize_response(&codex_bin, codex_home.path()) - .expect("benchmark CODEX_HOME should be initialized"), - ); - - bencher.bench_local(|| { - start_until_initialize_response(&codex_bin, codex_home.path()) - .expect("codex app-server should return an initialize response") - }); -} - -/// A running app-server that has returned a valid `initialize` response. -/// -/// Divan drops benchmark outputs after it stops each timer interval. Returning -/// this value from the measured closure keeps the `initialized` notification -/// and forced process reaping outside startup latency. -struct InitializedAppServer { - child: Child, - stdin: Option, - stdout: Option>, - acknowledge_on_drop: bool, -} - -/// Spawn a stdio app-server and return once it responds successfully to `initialize`. -fn start_until_initialize_response( - codex_bin: &Path, - codex_home: &Path, -) -> Result { - let request_id = RequestId::Integer(0); - let child = Command::new(codex_bin) - .arg("app-server") - .arg("--listen") - .arg("stdio://") - .env("CODEX_HOME", codex_home) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .spawn() - .with_context(|| format!("failed to spawn `{}` app-server", codex_bin.display()))?; - let mut server = InitializedAppServer { - child, - stdin: None, - stdout: None, - acknowledge_on_drop: false, - }; - server.stdin = Some( - server - .child - .stdin - .take() - .context("app-server stdin unavailable")?, - ); - server.stdout = Some(BufReader::new( - server - .child - .stdout - .take() - .context("app-server stdout unavailable")?, - )); - - let request = ClientRequest::Initialize { - request_id: request_id.clone(), - params: InitializeParams { - client_info: ClientInfo { - name: "codex-app-server-start-bench".to_string(), - title: Some("Codex App Server Start Benchmark".to_string()), - version: env!("CARGO_PKG_VERSION").to_string(), - }, - capabilities: Some(InitializeCapabilities { - experimental_api: false, - request_attestation: false, - opt_out_notification_methods: None, - }), - }, - }; - let stdin = server - .stdin - .as_mut() - .context("app-server stdin unavailable")?; - writeln!(stdin, "{}", serde_json::to_string(&request)?)?; - stdin - .flush() - .context("failed to flush initialize request")?; - - let mut line = String::new(); - loop { - line.clear(); - if server - .stdout - .as_mut() - .context("app-server stdout unavailable")? - .read_line(&mut line)? - == 0 - { - bail!("app-server closed stdout before returning initialize response"); - } - - match serde_json::from_str::(line.trim())? { - JSONRPCMessage::Response(JSONRPCResponse { id, result }) if id == request_id => { - let _: InitializeResponse = serde_json::from_value(result) - .context("initialize response missing expected payload")?; - server.acknowledge_on_drop = true; - return Ok(server); - } - JSONRPCMessage::Error(error) if error.id == request_id => { - bail!("initialize failed: {error:?}"); - } - JSONRPCMessage::Request(_) - | JSONRPCMessage::Response(_) - | JSONRPCMessage::Notification(_) - | JSONRPCMessage::Error(_) => {} - } - } -} - -impl Drop for InitializedAppServer { - fn drop(&mut self) { - if self.acknowledge_on_drop - && let Some(stdin) = self.stdin.as_mut() - { - let initialized = JSONRPCMessage::Notification(JSONRPCNotification { - method: "initialized".to_string(), - params: None, - }); - if let Ok(payload) = serde_json::to_string(&initialized) { - let _ = writeln!(stdin, "{payload}"); - let _ = stdin.flush(); - } - } - let _ = self.stdin.take(); - let _ = self.child.kill(); - let _ = self.child.wait(); - } -} diff --git a/justfile b/justfile index d061b90fda..d5e9fc3a36 100644 --- a/justfile +++ b/justfile @@ -2,9 +2,6 @@ set working-directory := "codex-rs" set positional-arguments rust_min_stack := "8388608" # 8 MiB -e2e_benchmark_packages := "codex-app-server-start-bench" -e2e_codex_bin := if os() == "windows" { "./target/release/codex.exe" } else { "./target/release/codex" } -e2e_smoke_codex_bin := if os() == "windows" { "./target/debug/codex.exe" } else { "./target/debug/codex" } # Display help help: @@ -33,11 +30,6 @@ app-server-test-client *args: cargo build -p codex-cli cargo run -p codex-app-server-test-client -- --codex-bin ./target/debug/codex "$@" -# Run end-to-end performance benchmarks that require a built Codex binary. -bench-e2e *args: - cargo build --release -p codex-cli --bin codex - for package in {{ e2e_benchmark_packages }}; do CODEX_BIN="{{ e2e_codex_bin }}" cargo run --release -p "$package" -- --bench "$@"; done - # Format Rust and Python SDK code. fmt: cargo fmt -- --config imports_granularity=Item 2>/dev/null @@ -63,19 +55,13 @@ test *args: RUST_MIN_STACK={{ rust_min_stack }} cargo nextest run --no-fail-fast "$@" just bench-smoke -# Run unit-test-style benchmark targets managed entirely by Cargo. -bench-unit *args: +# Run explicit workspace benchmark targets. +bench *args: cargo bench --workspace --bench '*' "$@" -# Smoke Cargo-managed benchmarks and compile e2e runners without measured binaries. +# Run benchmark targets once to ensure they start successfully. bench-smoke: - just bench-unit -- --test - for package in {{ e2e_benchmark_packages }}; do cargo build -p "$package" --bin "$package"; done - -# Run end-to-end performance benchmark targets once. -bench-e2e-smoke: - cargo build -p codex-cli --bin codex - for package in {{ e2e_benchmark_packages }}; do CODEX_BIN="{{ e2e_smoke_codex_bin }}" cargo run -p "$package" -- --test; done + just bench -- --test # Build and run Codex from source using Bazel. # Note we have to use the combination of `[no-cd]` and `--run_under="cd $PWD &&"` From 462deb0426bfea72f80ae7d56605958d7206309e Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 28 May 2026 20:05:41 +0200 Subject: [PATCH 06/10] Wire task completion into thread-idle lifecycle (#24928) ## Why #24744 introduced the thread idle lifecycle hook so idle continuation can be owned by lifecycle contributors instead of hard-coded goal runtime plumbing. Task completion still called `goal_runtime_apply(GoalRuntimeEvent::MaybeContinueIfIdle)` directly, so the post-turn idle transition remained goal-specific and did not notify generic thread lifecycle contributors. ## What Changed - Add `Session::emit_thread_idle_lifecycle_if_idle()` to gate idle emission on both no active turn and no queued trigger-turn mailbox work. - Call that helper when a task clears the active turn, replacing the direct `GoalRuntimeEvent::MaybeContinueIfIdle` path. - Cover the behavior with `codex-core` session tests for emitting after task completion and suppressing idle emission while trigger-turn mailbox work is pending. ## Verification - New tests in `core/src/session/tests.rs` exercise the idle lifecycle emission and trigger-turn mailbox guard. --- codex-rs/core/src/session/tests.rs | 103 +++++++++++++++++++++++++++ codex-rs/core/src/tasks/lifecycle.rs | 17 +++++ codex-rs/core/src/tasks/mod.rs | 1 + 3 files changed, 121 insertions(+) diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 7cca56dfb2..a596ff3e79 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -7894,6 +7894,29 @@ async fn realtime_conversation_list_voices_emits_builtin_list() { ); } +#[derive(Clone, Copy)] +struct CompletingTask; + +impl SessionTask for CompletingTask { + fn kind(&self) -> TaskKind { + TaskKind::Regular + } + + fn span_name(&self) -> &'static str { + "session_task.completing" + } + + async fn run( + self: Arc, + _session: Arc, + _ctx: Arc, + _input: Vec, + _cancellation_token: CancellationToken, + ) -> Option { + None + } +} + #[derive(Clone, Copy)] struct NeverEndingTask { kind: TaskKind, @@ -8238,6 +8261,86 @@ async fn task_finish_emits_turn_item_lifecycle_for_leftover_pending_user_input() )); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn task_finish_emits_thread_idle_lifecycle_after_active_turn_clears() { + struct ThreadIdleRecorder { + calls: Arc, + idle_tx: async_channel::Sender<()>, + expected_thread_id: ThreadId, + } + + #[async_trait::async_trait] + impl codex_extension_api::ThreadLifecycleContributor for ThreadIdleRecorder { + async fn on_thread_idle(&self, input: codex_extension_api::ThreadIdleInput<'_>) { + assert_eq!( + self.expected_thread_id.to_string(), + input.thread_store.level_id() + ); + self.calls.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + self.idle_tx.send(()).await.expect("idle receiver open"); + } + } + + let (mut session, turn_context) = make_session_and_context().await; + let calls = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let (idle_tx, idle_rx) = async_channel::bounded(1); + let mut builder = codex_extension_api::ExtensionRegistryBuilder::::new(); + builder.thread_lifecycle_contributor(Arc::new(ThreadIdleRecorder { + calls: Arc::clone(&calls), + idle_tx, + expected_thread_id: session.conversation_id, + })); + session.services.extensions = Arc::new(builder.build()); + + let session = Arc::new(session); + session + .spawn_task(Arc::new(turn_context), Vec::new(), CompletingTask) + .await; + + timeout(StdDuration::from_secs(2), idle_rx.recv()) + .await + .expect("thread idle lifecycle") + .expect("idle receiver open"); + assert_eq!(1, calls.load(std::sync::atomic::Ordering::SeqCst)); + assert!(session.active_turn.lock().await.is_none()); +} + +#[tokio::test] +async fn thread_idle_lifecycle_waits_for_trigger_turn_mailbox_work() { + struct ThreadIdleRecorder { + calls: Arc, + } + + #[async_trait::async_trait] + impl codex_extension_api::ThreadLifecycleContributor for ThreadIdleRecorder { + async fn on_thread_idle(&self, _input: codex_extension_api::ThreadIdleInput<'_>) { + self.calls.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + } + } + + let (mut session, _turn_context) = make_session_and_context().await; + let calls = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let mut builder = codex_extension_api::ExtensionRegistryBuilder::::new(); + builder.thread_lifecycle_contributor(Arc::new(ThreadIdleRecorder { + calls: Arc::clone(&calls), + })); + session.services.extensions = Arc::new(builder.build()); + session + .input_queue + .enqueue_mailbox_communication(InterAgentCommunication::new( + AgentPath::root(), + AgentPath::root(), + Vec::new(), + "pending trigger".to_string(), + /*trigger_turn*/ true, + )) + .await; + + session.emit_thread_idle_lifecycle_if_idle().await; + + assert_eq!(0, calls.load(std::sync::atomic::Ordering::SeqCst)); +} + #[tokio::test] async fn steer_input_requires_active_turn() { let (sess, _tc, _rx) = make_session_and_context_with_rx().await; diff --git a/codex-rs/core/src/tasks/lifecycle.rs b/codex-rs/core/src/tasks/lifecycle.rs index 882e787608..782cc8c7de 100644 --- a/codex-rs/core/src/tasks/lifecycle.rs +++ b/codex-rs/core/src/tasks/lifecycle.rs @@ -38,6 +38,23 @@ impl Session { } } + pub(crate) async fn emit_thread_idle_lifecycle_if_idle(&self) { + if self.active_turn.lock().await.is_some() + || self.input_queue.has_trigger_turn_mailbox_items().await + { + return; + } + + for contributor in self.services.extensions.thread_lifecycle_contributors() { + contributor + .on_thread_idle(codex_extension_api::ThreadIdleInput { + session_store: &self.services.session_extension_data, + thread_store: &self.services.thread_extension_data, + }) + .await; + } + } + pub(super) async fn emit_turn_abort_lifecycle( &self, reason: TurnAbortReason, diff --git a/codex-rs/core/src/tasks/mod.rs b/codex-rs/core/src/tasks/mod.rs index d8b5689061..38a8a96276 100644 --- a/codex-rs/core/src/tasks/mod.rs +++ b/codex-rs/core/src/tasks/mod.rs @@ -793,6 +793,7 @@ impl Session { { warn!("failed to apply goal runtime maybe-continue event: {err}"); } + self.emit_thread_idle_lifecycle_if_idle().await; } async fn take_active_turn(&self) -> Option { From ecb41fcb64f22445b051d3943fc970d2ce51d0cd Mon Sep 17 00:00:00 2001 From: Won Park Date: Thu, 28 May 2026 11:44:55 -0700 Subject: [PATCH 07/10] Add feature-gated standalone image generation extension (#24723) ## Why Add a standalone image generation path that can be exercised independently of hosted Responses image generation, while retaining the hosted tool as fallback unless the extension is actually available to the model. ## What changed - Added the `codex-image-generation-extension` crate with standalone generate/edit execution, prior-image selection for edits, model-visible image output, and local generated-image persistence. - Installed the extension in app-server behind the disabled-by-default `imagegenext` feature and backend eligibility checks. - Updated core tool planning so eligible `image_gen.imagegen` exposure replaces hosted `image_generation`, while unavailable configurations retain hosted fallback. - Added coverage for extension behavior, edit history reuse, feature gating, auth eligibility, and hosted-tool replacement. - The extension is installed through app-server only in this PR; other execution paths retain hosted image generation because hosted replacement occurs only when the standalone executor is actually registered and model-visible. - The initial extension contract intentionally fixes the image model to `gpt-image-2` and uses automatic image parameters. - Native generated-image history/card parity and rollout persistence cleanup are intentionally deferred follow-up work. ## Validation - `just test -p codex-image-generation-extension` - `just test -p codex-features` - `just test -p codex-core hosted_tools_follow_provider_auth_model_and_config_gates` - `just test -p codex-app-server` - `just fix -p codex-image-generation-extension -p codex-features -p codex-core -p codex-app-server` - `just fmt` - `just bazel-lock-update` - `just bazel-lock-check` --------- Co-authored-by: jif-oai --- codex-rs/Cargo.lock | 26 ++ codex-rs/Cargo.toml | 2 + codex-rs/app-server/Cargo.toml | 1 + codex-rs/app-server/src/extensions.rs | 3 +- codex-rs/core/config.schema.json | 6 + codex-rs/core/src/tools/spec_plan.rs | 39 +- codex-rs/core/src/tools/spec_plan_tests.rs | 10 + codex-rs/ext/image-generation/BUILD.bazel | 9 + codex-rs/ext/image-generation/Cargo.toml | 37 ++ .../image-generation/imagegen_description.md | 11 + codex-rs/ext/image-generation/src/backend.rs | 60 +++ .../ext/image-generation/src/extension.rs | 99 +++++ codex-rs/ext/image-generation/src/lib.rs | 8 + codex-rs/ext/image-generation/src/tests.rs | 341 +++++++++++++++ codex-rs/ext/image-generation/src/tool.rs | 395 ++++++++++++++++++ codex-rs/features/src/lib.rs | 8 + codex-rs/features/src/tests.rs | 7 + 17 files changed, 1056 insertions(+), 6 deletions(-) create mode 100644 codex-rs/ext/image-generation/BUILD.bazel create mode 100644 codex-rs/ext/image-generation/Cargo.toml create mode 100644 codex-rs/ext/image-generation/imagegen_description.md create mode 100644 codex-rs/ext/image-generation/src/backend.rs create mode 100644 codex-rs/ext/image-generation/src/extension.rs create mode 100644 codex-rs/ext/image-generation/src/lib.rs create mode 100644 codex-rs/ext/image-generation/src/tests.rs create mode 100644 codex-rs/ext/image-generation/src/tool.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index aac0d6f90c..1951db642f 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1928,6 +1928,7 @@ dependencies = [ "codex-git-utils", "codex-guardian", "codex-hooks", + "codex-image-generation-extension", "codex-login", "codex-mcp", "codex-memories-extension", @@ -3039,6 +3040,31 @@ dependencies = [ "uuid", ] +[[package]] +name = "codex-image-generation-extension" +version = "0.0.0" +dependencies = [ + "async-trait", + "base64 0.22.1", + "codex-api", + "codex-core", + "codex-extension-api", + "codex-features", + "codex-login", + "codex-model-provider", + "codex-model-provider-info", + "codex-protocol", + "codex-tools", + "http 1.4.0", + "pretty_assertions", + "schemars 0.8.22", + "serde", + "serde_json", + "tempfile", + "tokio", + "tracing", +] + [[package]] name = "codex-install-context" version = "0.0.0" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index b1f9e4c31a..808cdef1b9 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -47,6 +47,7 @@ members = [ "ext/extension-api", "ext/goal", "ext/guardian", + "ext/image-generation", "ext/memories", "ext/web-search", "external-agent-migration", @@ -165,6 +166,7 @@ codex-execpolicy = { path = "execpolicy" } codex-extension-api = { path = "ext/extension-api" } codex-goal-extension = { path = "ext/goal" } codex-guardian = { path = "ext/guardian" } +codex-image-generation-extension = { path = "ext/image-generation" } codex-external-agent-migration = { path = "external-agent-migration" } codex-external-agent-sessions = { path = "external-agent-sessions" } codex-experimental-api-macros = { path = "codex-experimental-api-macros" } diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index 09e3197b77..d1a69783ad 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -54,6 +54,7 @@ codex-backend-client = { workspace = true } codex-file-search = { workspace = true } codex-chatgpt = { workspace = true } codex-login = { workspace = true } +codex-image-generation-extension = { workspace = true } codex-memories-extension = { workspace = true } codex-web-search-extension = { workspace = true } codex-memories-write = { workspace = true } diff --git a/codex-rs/app-server/src/extensions.rs b/codex-rs/app-server/src/extensions.rs index 1246da7b37..7b2673ca06 100644 --- a/codex-rs/app-server/src/extensions.rs +++ b/codex-rs/app-server/src/extensions.rs @@ -31,7 +31,8 @@ where let mut builder = ExtensionRegistryBuilder::::with_event_sink(event_sink); codex_guardian::install(&mut builder, guardian_agent_spawner); codex_memories_extension::install(&mut builder, codex_otel::global()); - codex_web_search_extension::install(&mut builder, auth_manager); + codex_web_search_extension::install(&mut builder, auth_manager.clone()); + codex_image_generation_extension::install(&mut builder, auth_manager); Arc::new(builder.build()) } diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index ab855d733b..1dd6bda8a4 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -473,6 +473,9 @@ "image_generation": { "type": "boolean" }, + "imagegenext": { + "type": "boolean" + }, "in_app_browser": { "type": "boolean" }, @@ -4564,6 +4567,9 @@ "image_generation": { "type": "boolean" }, + "imagegenext": { + "type": "boolean" + }, "in_app_browser": { "type": "boolean" }, diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index 2610bee95c..bb7baf4bd8 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -85,6 +85,8 @@ use std::sync::Arc; use tracing::warn; const MULTI_AGENT_V2_NAMESPACE_DESCRIPTION: &str = "Tools for spawning and managing sub-agents."; +const IMAGE_GEN_NAMESPACE: &str = "image_gen"; +const IMAGEGEN_TOOL_NAME: &str = "imagegen"; type PlannedRuntime = Arc; @@ -257,7 +259,9 @@ fn hosted_model_tool_specs(context: &CoreToolPlanContext<'_>) -> Vec { }) { specs.push(web_search_tool); } - if image_generation_tool_enabled(turn_context) { + if image_generation_tool_enabled(turn_context) + && !standalone_image_generation_available(turn_context, context.extension_tool_executors) + { specs.push(create_image_generation_tool("png")); } specs @@ -316,21 +320,41 @@ fn agent_jobs_worker_tools_enabled(turn_context: &TurnContext) -> bool { } fn image_generation_tool_enabled(turn_context: &TurnContext) -> bool { + image_generation_runtime_enabled(turn_context) + && turn_context + .features + .get() + .enabled(Feature::ImageGeneration) +} + +fn image_generation_runtime_enabled(turn_context: &TurnContext) -> bool { turn_context .auth_manager .as_deref() .is_some_and(AuthManager::current_auth_uses_codex_backend) && turn_context.provider.capabilities().image_generation - && turn_context - .features - .get() - .enabled(Feature::ImageGeneration) && turn_context .model_info .input_modalities .contains(&InputModality::Image) } +fn standalone_image_generation_model_visible(turn_context: &TurnContext) -> bool { + image_generation_runtime_enabled(turn_context) + && turn_context.features.get().enabled(Feature::ImageGenExt) + && namespace_tools_enabled(turn_context) +} + +fn standalone_image_generation_available( + turn_context: &TurnContext, + extension_tools: &[Arc>], +) -> bool { + standalone_image_generation_model_visible(turn_context) + && extension_tools.iter().any(|executor| { + executor.tool_name() == ToolName::namespaced(IMAGE_GEN_NAMESPACE, IMAGEGEN_TOOL_NAME) + }) +} + fn wait_agent_timeout_options(turn_context: &TurnContext) -> WaitAgentTimeoutOptions { if multi_agent_v2_enabled(turn_context) { return WaitAgentTimeoutOptions { @@ -839,6 +863,11 @@ fn append_extension_tool_executors( for executor in executors.iter().cloned() { let tool_name = executor.tool_name(); + if tool_name == ToolName::namespaced(IMAGE_GEN_NAMESPACE, IMAGEGEN_TOOL_NAME) + && !standalone_image_generation_model_visible(turn_context) + { + continue; + } if !reserved_tool_names.insert(tool_name.clone()) { warn!("Skipping extension tool `{tool_name}`: tool already registered"); continue; diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index 94587803b8..a274f934c4 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -960,6 +960,16 @@ async fn hosted_tools_follow_provider_auth_model_and_config_gates() { .await; image_generation.assert_visible_contains(&["image_generation"]); + let extension_flag_without_imagegen_tool = probe(|turn| { + use_chatgpt_auth(turn); + set_feature(turn, Feature::ImageGeneration, /*enabled*/ true); + set_feature(turn, Feature::ImageGenExt, /*enabled*/ true); + turn.model_info.input_modalities = vec![InputModality::Image]; + }) + .await; + extension_flag_without_imagegen_tool.assert_visible_contains(&["image_generation"]); + extension_flag_without_imagegen_tool.assert_visible_lacks(&["image_gen"]); + let live_web_search = probe(|turn| { set_web_search_mode(turn, WebSearchMode::Live); turn.model_info.web_search_tool_type = WebSearchToolType::TextAndImage; diff --git a/codex-rs/ext/image-generation/BUILD.bazel b/codex-rs/ext/image-generation/BUILD.bazel new file mode 100644 index 0000000000..5ed05a5dc8 --- /dev/null +++ b/codex-rs/ext/image-generation/BUILD.bazel @@ -0,0 +1,9 @@ +load("//:defs.bzl", "codex_rust_crate") + +codex_rust_crate( + name = "image-generation", + crate_name = "codex_image_generation_extension", + compile_data = [ + "imagegen_description.md", + ], +) diff --git a/codex-rs/ext/image-generation/Cargo.toml b/codex-rs/ext/image-generation/Cargo.toml new file mode 100644 index 0000000000..d4f2cb50de --- /dev/null +++ b/codex-rs/ext/image-generation/Cargo.toml @@ -0,0 +1,37 @@ +[package] +edition.workspace = true +license.workspace = true +name = "codex-image-generation-extension" +version.workspace = true + +[lib] +name = "codex_image_generation_extension" +path = "src/lib.rs" +doctest = false + +[lints] +workspace = true + +[dependencies] +async-trait = { workspace = true } +base64 = { workspace = true } +codex-api = { workspace = true } +codex-core = { workspace = true } +codex-extension-api = { workspace = true } +codex-features = { workspace = true } +codex-login = { workspace = true } +codex-model-provider = { workspace = true } +codex-model-provider-info = { workspace = true } +codex-protocol = { workspace = true } +codex-tools = { workspace = true } +http = { workspace = true } +schemars = { workspace = true } +serde = { workspace = true, features = ["derive"] } +serde_json = { workspace = true } +tokio = { workspace = true, features = ["fs"] } +tracing = { workspace = true } + +[dev-dependencies] +pretty_assertions = { workspace = true } +tempfile = { workspace = true } +tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } diff --git a/codex-rs/ext/image-generation/imagegen_description.md b/codex-rs/ext/image-generation/imagegen_description.md new file mode 100644 index 0000000000..7ae6ecb5fa --- /dev/null +++ b/codex-rs/ext/image-generation/imagegen_description.md @@ -0,0 +1,11 @@ +The `image_gen.imagegen` tool enables image generation from descriptions and editing of existing images based on specific instructions. Use it when: + +- The user requests an image based on a scene description, such as a diagram, portrait, comic, meme, or any other visual. +- The user wants to modify an attached or previously generated image with specific changes, including adding or removing elements, altering colors, improving quality/resolution, or transforming the style (e.g., cartoon, oil painting). + +Guidelines: +- Set `action` to `generate` when the user asks for a brand new image. +- Set `action` to `edit` when the user asks to modify an existing image from the conversation history. +- Directly generate the image without reconfirmation or clarification. +- After each image generation, do not mention anything related to download. Do not summarize the image. Do not ask followup question. Do not say ANYTHING after you generate an image. +- Always use this tool for image editing unless the user explicitly requests otherwise. Do not use the `python` tool for image editing unless specifically instructed. diff --git a/codex-rs/ext/image-generation/src/backend.rs b/codex-rs/ext/image-generation/src/backend.rs new file mode 100644 index 0000000000..9b837772c7 --- /dev/null +++ b/codex-rs/ext/image-generation/src/backend.rs @@ -0,0 +1,60 @@ +use codex_api::ImageEditRequest; +use codex_api::ImageGenerationRequest; +use codex_api::ImageResponse; +use codex_api::ImagesClient; +use codex_api::ReqwestTransport; +use codex_login::default_client::build_reqwest_client; +use codex_model_provider::SharedModelProvider; +use http::HeaderMap; + +#[derive(Clone)] +pub(crate) struct CodexImagesBackend { + provider: SharedModelProvider, +} + +impl CodexImagesBackend { + /// Creates a backend that sends image requests through the active model provider. + pub(crate) fn new(provider: SharedModelProvider) -> Self { + Self { provider } + } + + /// Resolves the provider and auth required for the current image API request. + async fn client(&self) -> Result, String> { + let provider = self + .provider + .api_provider() + .await + .map_err(|err| err.to_string())?; + let auth = self + .provider + .api_auth() + .await + .map_err(|err| err.to_string())?; + Ok(ImagesClient::new( + ReqwestTransport::new(build_reqwest_client()), + provider, + auth, + )) + } + + /// Sends a standalone image generation request through the configured Images client. + pub(crate) async fn generate( + &self, + request: ImageGenerationRequest, + ) -> Result { + self.client() + .await? + .generate(&request, HeaderMap::new()) + .await + .map_err(|err| err.to_string()) + } + + /// Sends a standalone image edit request through the configured Images client. + pub(crate) async fn edit(&self, request: ImageEditRequest) -> Result { + self.client() + .await? + .edit(&request, HeaderMap::new()) + .await + .map_err(|err| err.to_string()) + } +} diff --git a/codex-rs/ext/image-generation/src/extension.rs b/codex-rs/ext/image-generation/src/extension.rs new file mode 100644 index 0000000000..8f0b09f8cb --- /dev/null +++ b/codex-rs/ext/image-generation/src/extension.rs @@ -0,0 +1,99 @@ +use std::path::PathBuf; +use std::sync::Arc; + +use codex_core::config::Config; +use codex_extension_api::ConfigContributor; +use codex_extension_api::ExtensionData; +use codex_extension_api::ExtensionRegistryBuilder; +use codex_extension_api::ThreadLifecycleContributor; +use codex_extension_api::ThreadStartInput; +use codex_extension_api::ToolCall; +use codex_extension_api::ToolContributor; +use codex_extension_api::ToolExecutor; +use codex_features::Feature; +use codex_login::AuthManager; +use codex_model_provider::create_model_provider; +use codex_model_provider_info::ModelProviderInfo; + +use crate::backend::CodexImagesBackend; +use crate::tool::ImageGenerationTool; +use crate::tool::generated_image_output_dir; + +#[derive(Clone)] +struct ImageGenerationExtension { + auth_manager: Arc, +} + +#[derive(Clone)] +struct ImageGenerationExtensionConfig { + enabled: bool, + provider: ModelProviderInfo, + codex_home: PathBuf, +} + +impl From<&Config> for ImageGenerationExtensionConfig { + /// Resolves whether standalone image generation should be available for a thread. + fn from(config: &Config) -> Self { + Self { + enabled: config.features.enabled(Feature::ImageGenExt) + && config.model_provider.is_openai(), + provider: config.model_provider.clone(), + codex_home: config.codex_home.to_path_buf(), + } + } +} + +#[async_trait::async_trait] +impl ThreadLifecycleContributor for ImageGenerationExtension { + /// Seeds image-generation availability when a thread begins. + async fn on_thread_start(&self, input: ThreadStartInput<'_, Config>) { + input + .thread_store + .insert(ImageGenerationExtensionConfig::from(input.config)); + } +} + +impl ConfigContributor for ImageGenerationExtension { + /// Refreshes image-generation availability after thread configuration changes. + fn on_config_changed( + &self, + _session_store: &ExtensionData, + thread_store: &ExtensionData, + _previous_config: &Config, + new_config: &Config, + ) { + thread_store.insert(ImageGenerationExtensionConfig::from(new_config)); + } +} + +impl ToolContributor for ImageGenerationExtension { + /// Creates the image-generation tool exposed by this installed extension. + fn tools( + &self, + _session_store: &ExtensionData, + thread_store: &ExtensionData, + ) -> Vec>> { + let Some(config) = thread_store.get::() else { + return Vec::new(); + }; + if !config.enabled || !self.auth_manager.current_auth_uses_codex_backend() { + return Vec::new(); + } + + vec![Arc::new(ImageGenerationTool::new( + CodexImagesBackend::new(create_model_provider( + config.provider.clone(), + Some(self.auth_manager.clone()), + )), + generated_image_output_dir(&config.codex_home, thread_store.level_id()), + ))] + } +} + +/// Installs the feature-gated standalone image-generation extension contributors. +pub fn install(registry: &mut ExtensionRegistryBuilder, auth_manager: Arc) { + let extension = Arc::new(ImageGenerationExtension { auth_manager }); + registry.thread_lifecycle_contributor(extension.clone()); + registry.config_contributor(extension.clone()); + registry.tool_contributor(extension); +} diff --git a/codex-rs/ext/image-generation/src/lib.rs b/codex-rs/ext/image-generation/src/lib.rs new file mode 100644 index 0000000000..63f1ab2482 --- /dev/null +++ b/codex-rs/ext/image-generation/src/lib.rs @@ -0,0 +1,8 @@ +mod backend; +mod extension; +mod tool; + +pub use extension::install; + +pub(crate) const IMAGE_GEN_NAMESPACE: &str = "image_gen"; +pub(crate) const IMAGEGEN_TOOL_NAME: &str = "imagegen"; diff --git a/codex-rs/ext/image-generation/src/tests.rs b/codex-rs/ext/image-generation/src/tests.rs new file mode 100644 index 0000000000..1b56270ed3 --- /dev/null +++ b/codex-rs/ext/image-generation/src/tests.rs @@ -0,0 +1,341 @@ +use codex_api::ImageBackground; +use codex_api::ImageEditRequest; +use codex_api::ImageGenerationRequest; +use codex_api::ImageQuality; +use codex_api::ImageUrl; +use codex_extension_api::ToolOutput; +use codex_extension_api::ToolPayload; +use codex_extension_api::ToolSpec; +use codex_protocol::models::ContentItem; +use codex_protocol::models::DEFAULT_IMAGE_DETAIL; +use codex_protocol::models::FunctionCallOutputBody; +use codex_protocol::models::FunctionCallOutputContentItem; +use codex_protocol::models::FunctionCallOutputPayload; +use codex_protocol::models::ResponseInputItem; +use codex_protocol::models::ResponseItem; +use codex_tools::ResponsesApiNamespaceTool; +use pretty_assertions::assert_eq; + +use super::GeneratedImageOutput; +use super::ImageRequest; +use super::ImagegenAction; +use super::ImagegenArgs; +use super::generated_image_output_dir; +use super::imagegen_tool_spec; +use super::persist_generated_image; +use super::request_for_action; +use crate::IMAGE_GEN_NAMESPACE; +use crate::IMAGEGEN_TOOL_NAME; + +const RESULT: &str = "cG5n"; + +#[test] +fn uses_reserved_image_gen_namespace() { + let ToolSpec::Namespace(spec) = imagegen_tool_spec() else { + panic!("imagegen should advertise a namespace tool"); + }; + assert_eq!(spec.name, IMAGE_GEN_NAMESPACE); + let ResponsesApiNamespaceTool::Function(function) = &spec.tools[0]; + assert_eq!(function.name, IMAGEGEN_TOOL_NAME); +} + +#[test] +fn generate_uses_fixed_request_defaults() { + assert_eq!( + request_for_action(&args(ImagegenAction::Generate, "paint a moonlit lake"), &[]) + .expect("generation request should build"), + ImageRequest::Generate(ImageGenerationRequest { + prompt: "paint a moonlit lake".to_string(), + background: Some(ImageBackground::Auto), + model: "gpt-image-2".to_string(), + n: None, + quality: Some(ImageQuality::Auto), + size: Some("auto".to_string()), + }) + ); +} + +#[tokio::test] +async fn generated_output_returns_image_input_and_persists_artifact() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let output_hint = persist_generated_image(tempdir.path(), "call-1", RESULT) + .await + .expect("generated image should persist"); + let output = GeneratedImageOutput { + result: RESULT.to_string(), + output_hint: Some(output_hint), + }; + + let ResponseInputItem::FunctionCallOutput { + output: response_output, + .. + } = output.to_response_item("call-1", &function_payload()) + else { + panic!("imagegen should return function tool output"); + }; + let FunctionCallOutputBody::ContentItems(content_items) = response_output.body else { + panic!("imagegen output should contain generated image bytes"); + }; + assert_eq!( + content_items, + vec![ + FunctionCallOutputContentItem::InputImage { + image_url: format!("data:image/png;base64,{RESULT}"), + detail: Some(DEFAULT_IMAGE_DETAIL), + }, + FunctionCallOutputContentItem::InputText { + text: format!( + "Generated images are saved to {} as {} by default.\n\ + If you need to use a generated image at another path, copy it and leave the original in place unless the user explicitly asks you to delete it.", + tempdir.path().display(), + tempdir.path().join("call-1.png").display(), + ), + }, + ] + ); + assert_eq!( + std::fs::read(tempdir.path().join("call-1.png")).expect("saved generated image"), + b"png" + ); +} + +#[test] +fn edit_matches_context_selector_for_generated_images_after_latest_user_anchor() { + let history = vec![ + generated_item("g1"), + generated_item("g2"), + generated_item("g3"), + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ + ContentItem::InputImage { + image_url: "data:image/png;base64,u1".to_string(), + detail: None, + }, + ContentItem::InputImage { + image_url: "data:image/png;base64,u2".to_string(), + detail: None, + }, + ], + phase: None, + }, + generated_item("g4"), + generated_item("g5"), + generated_item("g6"), + generated_item("g7"), + ]; + + assert_eq!( + edit_request("change the lighting", &history), + expected_edit_request( + "change the lighting", + &[ + "data:image/png;base64,u1", + "data:image/png;base64,u2", + "data:image/png;base64,g5", + "data:image/png;base64,g6", + "data:image/png;base64,g7", + ] + ) + ); +} + +#[test] +fn edit_preserves_a_generated_image_when_user_anchor_fills_the_limit() { + let history = vec![ + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: ["a", "b", "c", "d", "e"] + .into_iter() + .map(|image| ContentItem::InputImage { + image_url: format!("data:image/png;base64,{image}"), + detail: None, + }) + .collect(), + phase: None, + }, + generated_item("generated"), + ]; + + assert_eq!( + edit_request("edit the last generated image", &history), + expected_edit_request( + "edit the last generated image", + &[ + "data:image/png;base64,b", + "data:image/png;base64,c", + "data:image/png;base64,d", + "data:image/png;base64,e", + "data:image/png;base64,generated", + ] + ) + ); +} + +#[test] +fn edit_uses_latest_user_upload_before_a_text_only_follow_up() { + let history = vec![ + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::InputImage { + image_url: "data:image/png;base64,user".to_string(), + detail: None, + }], + phase: None, + }, + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::InputText { + text: "edit this image".to_string(), + }], + phase: None, + }, + ]; + + assert_eq!( + edit_request("change the lighting", &history), + expected_edit_request("change the lighting", &["data:image/png;base64,user"]) + ); +} + +#[test] +fn edit_reuses_images_from_prior_standalone_imagegen_calls() { + let history = vec![ + ResponseItem::FunctionCall { + id: None, + name: IMAGEGEN_TOOL_NAME.to_string(), + namespace: Some(IMAGE_GEN_NAMESPACE.to_string()), + arguments: "{}".to_string(), + call_id: "imagegen-1".to_string(), + }, + generated_function_output("imagegen-1", "standalone"), + ]; + + assert_eq!( + edit_request("change the lighting", &history), + expected_edit_request("change the lighting", &["data:image/png;base64,standalone"]) + ); +} + +#[test] +fn edit_keeps_newest_standalone_generated_images_when_over_limit() { + let history = (1..=6) + .flat_map(|index| { + let call_id = format!("imagegen-{index}"); + vec![ + ResponseItem::FunctionCall { + id: None, + name: IMAGEGEN_TOOL_NAME.to_string(), + namespace: Some(IMAGE_GEN_NAMESPACE.to_string()), + arguments: "{}".to_string(), + call_id: call_id.clone(), + }, + generated_function_output(&call_id, &index.to_string()), + ] + }) + .collect::>(); + + assert_eq!( + edit_request("change the lighting", &history), + expected_edit_request( + "change the lighting", + &[ + "data:image/png;base64,2", + "data:image/png;base64,3", + "data:image/png;base64,4", + "data:image/png;base64,5", + "data:image/png;base64,6", + ] + ) + ); +} + +#[test] +fn edit_without_image_history_returns_tool_error() { + let error = request_for_action(&args(ImagegenAction::Edit, "change the lighting"), &[]) + .expect_err("edit should require image context"); + + assert_eq!( + error.to_string(), + "image edit requested without any usable image in conversation history" + ); +} + +#[test] +fn generated_image_output_dir_is_scoped_to_sanitized_thread_id() { + assert_eq!( + generated_image_output_dir(std::path::Path::new("/tmp/codex-home"), "thread/1"), + std::path::PathBuf::from("/tmp/codex-home/generated_images/thread_1") + ); +} + +fn args(action: ImagegenAction, prompt: &str) -> ImagegenArgs { + ImagegenArgs { + prompt: prompt.to_string(), + action, + } +} + +fn edit_request(prompt: &str, history: &[ResponseItem]) -> ImageEditRequest { + let ImageRequest::Edit(request) = + request_for_action(&args(ImagegenAction::Edit, prompt), history) + .expect("edit request should build") + else { + panic!("expected edit request"); + }; + request +} + +fn expected_edit_request(prompt: &str, images: &[&str]) -> ImageEditRequest { + ImageEditRequest { + images: images + .iter() + .map(|image_url| ImageUrl { + image_url: (*image_url).to_string(), + }) + .collect(), + prompt: prompt.to_string(), + background: Some(ImageBackground::Auto), + model: "gpt-image-2".to_string(), + n: None, + quality: Some(ImageQuality::Auto), + size: Some("auto".to_string()), + } +} + +fn generated_item(result: &str) -> ResponseItem { + ResponseItem::ImageGenerationCall { + id: format!("id-{result}"), + status: "completed".to_string(), + revised_prompt: None, + result: result.to_string(), + } +} + +fn generated_function_output(call_id: &str, result: &str) -> ResponseItem { + ResponseItem::FunctionCallOutput { + call_id: call_id.to_string(), + output: FunctionCallOutputPayload { + body: FunctionCallOutputBody::ContentItems(vec![ + FunctionCallOutputContentItem::InputImage { + image_url: format!("data:image/png;base64,{result}"), + detail: Some(DEFAULT_IMAGE_DETAIL), + }, + FunctionCallOutputContentItem::InputText { + text: "generated image save hint".to_string(), + }, + ]), + success: Some(true), + }, + } +} + +fn function_payload() -> ToolPayload { + ToolPayload::Function { + arguments: "{}".to_string(), + } +} diff --git a/codex-rs/ext/image-generation/src/tool.rs b/codex-rs/ext/image-generation/src/tool.rs new file mode 100644 index 0000000000..fa1614cd47 --- /dev/null +++ b/codex-rs/ext/image-generation/src/tool.rs @@ -0,0 +1,395 @@ +use std::path::Path; +use std::path::PathBuf; + +use base64::Engine; +use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; +use codex_api::ImageBackground; +use codex_api::ImageEditRequest; +use codex_api::ImageGenerationRequest; +use codex_api::ImageQuality; +use codex_api::ImageUrl; +use codex_extension_api::FunctionCallError; +use codex_extension_api::ToolCall; +use codex_extension_api::ToolExecutor; +use codex_extension_api::ToolName; +use codex_extension_api::ToolOutput; +use codex_extension_api::ToolPayload; +use codex_extension_api::ToolSpec; +use codex_extension_api::parse_tool_input_schema; +use codex_protocol::models::ContentItem; +use codex_protocol::models::DEFAULT_IMAGE_DETAIL; +use codex_protocol::models::FunctionCallOutputBody; +use codex_protocol::models::FunctionCallOutputContentItem; +use codex_protocol::models::FunctionCallOutputPayload; +use codex_protocol::models::ResponseInputItem; +use codex_protocol::models::ResponseItem; +use codex_tools::ResponsesApiNamespace; +use codex_tools::ResponsesApiNamespaceTool; +use codex_tools::ResponsesApiTool; +use codex_tools::ToolExposure; +use codex_tools::default_namespace_description; +use schemars::JsonSchema; +use schemars::r#gen::SchemaSettings; +use serde::Deserialize; +use serde_json::Map; +use serde_json::Value; + +use crate::IMAGE_GEN_NAMESPACE; +use crate::IMAGEGEN_TOOL_NAME; +use crate::backend::CodexImagesBackend; + +const IMAGE_MODEL: &str = "gpt-image-2"; +const MAX_EDIT_IMAGES: usize = 5; +const IMAGEGEN_DESCRIPTION: &str = include_str!("../imagegen_description.md"); +const GENERATED_IMAGE_ARTIFACTS_DIR: &str = "generated_images"; + +#[derive(Clone)] +pub(crate) struct ImageGenerationTool { + backend: CodexImagesBackend, + output_dir: PathBuf, +} + +impl ImageGenerationTool { + /// Creates an image-generation tool backed by an image API executor. + pub(crate) fn new(backend: CodexImagesBackend, output_dir: PathBuf) -> Self { + Self { + backend, + output_dir, + } + } +} + +#[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] +struct ImagegenArgs { + prompt: String, + action: ImagegenAction, +} + +#[derive(Debug, Deserialize, JsonSchema)] +#[serde(rename_all = "lowercase")] +enum ImagegenAction { + Generate, + Edit, +} + +#[async_trait::async_trait] +impl ToolExecutor for ImageGenerationTool { + /// Keeps the tool in the existing image-generation Responses namespace. + fn tool_name(&self) -> ToolName { + ToolName::namespaced(IMAGE_GEN_NAMESPACE, IMAGEGEN_TOOL_NAME) + } + + /// Advertises the model contract: a rewritten prompt and semantic action. + fn spec(&self) -> ToolSpec { + imagegen_tool_spec() + } + + /// Keeps this model-facing tool out of the nested code-mode tool surface. + fn exposure(&self) -> ToolExposure { + ToolExposure::DirectModelOnly + } + + /// Executes the selected image operation and returns the completed image result. + async fn handle(&self, call: ToolCall) -> Result, FunctionCallError> { + let args = parse_args(&call)?; + let request = request_for_action(&args, call.conversation_history.items())?; + + let response = match request { + ImageRequest::Generate(request) => self.backend.generate(request).await, + ImageRequest::Edit(request) => self.backend.edit(request).await, + } + .map_err(|err| { + FunctionCallError::RespondToModel(format!("image generation failed: {err}")) + })?; + let Some(result) = response.data.into_iter().next().map(|data| data.b64_json) else { + return Err(FunctionCallError::RespondToModel( + "image generation returned no image data".to_string(), + )); + }; + let output_hint = + match persist_generated_image(&self.output_dir, &call.call_id, &result).await { + Ok(output_hint) => Some(output_hint), + Err(err) => { + tracing::warn!( + call_id = %call.call_id, + output_dir = %self.output_dir.display(), + "failed to save generated image: {err}" + ); + None + } + }; + Ok(Box::new(GeneratedImageOutput { + result, + output_hint, + })) + } +} + +#[derive(Debug, PartialEq)] +enum ImageRequest { + Generate(ImageGenerationRequest), + Edit(ImageEditRequest), +} + +/// Maps the model-selected action to the fixed image API request parameters. +fn request_for_action( + args: &ImagegenArgs, + history: &[ResponseItem], +) -> Result { + match args.action { + ImagegenAction::Generate => Ok(ImageRequest::Generate(ImageGenerationRequest { + prompt: args.prompt.clone(), + background: Some(ImageBackground::Auto), + model: IMAGE_MODEL.to_string(), + n: None, + quality: Some(ImageQuality::Auto), + size: Some("auto".to_string()), + })), + ImagegenAction::Edit => { + let images = edit_images(history); + if images.is_empty() { + return Err(FunctionCallError::RespondToModel( + "image edit requested without any usable image in conversation history" + .to_string(), + )); + } + Ok(ImageRequest::Edit(ImageEditRequest { + images, + prompt: args.prompt.clone(), + background: Some(ImageBackground::Auto), + model: IMAGE_MODEL.to_string(), + n: None, + quality: Some(ImageQuality::Auto), + size: Some("auto".to_string()), + })) + } + } +} + +/// Selects edit context using the hosted imagegen anchor and truncation behavior. +fn edit_images(history: &[ResponseItem]) -> Vec { + let latest_uploaded_images = history.iter().enumerate().rev().find_map(|(index, item)| { + let ResponseItem::Message { role, content, .. } = item else { + return None; + }; + if role != "user" { + return None; + } + let images = content + .iter() + .filter_map(|item| match item { + ContentItem::InputImage { image_url, .. } => Some(ImageUrl { + image_url: image_url.clone(), + }), + ContentItem::InputText { .. } | ContentItem::OutputText { .. } => None, + }) + .collect::>(); + (!images.is_empty()).then_some((index, images)) + }); + let (user_images, follow_up_start) = latest_uploaded_images + .map_or_else(|| (Vec::new(), 0), |(index, images)| (images, index + 1)); + let mut generated_images = Vec::new(); + for item in &history[follow_up_start..] { + match item { + ResponseItem::ImageGenerationCall { result, .. } if !result.is_empty() => { + generated_images.push(ImageUrl { + image_url: format!("data:image/png;base64,{result}"), + }); + } + ResponseItem::FunctionCallOutput { call_id, output } + if history.iter().any(|item| { + matches!( + item, + ResponseItem::FunctionCall { + name, + namespace: Some(namespace), + call_id: function_call_id, + .. + } if function_call_id == call_id + && name == IMAGEGEN_TOOL_NAME + && namespace == IMAGE_GEN_NAMESPACE + ) + }) => + { + generated_images.extend(output.content_items().into_iter().flatten().filter_map( + |item| match item { + FunctionCallOutputContentItem::InputImage { image_url, .. } => { + Some(ImageUrl { + image_url: image_url.clone(), + }) + } + FunctionCallOutputContentItem::InputText { .. } + | FunctionCallOutputContentItem::EncryptedContent { .. } => None, + }, + )); + } + ResponseItem::Message { .. } + | ResponseItem::Reasoning { .. } + | ResponseItem::LocalShellCall { .. } + | ResponseItem::FunctionCall { .. } + | ResponseItem::ToolSearchCall { .. } + | ResponseItem::FunctionCallOutput { .. } + | ResponseItem::CustomToolCall { .. } + | ResponseItem::CustomToolCallOutput { .. } + | ResponseItem::ToolSearchOutput { .. } + | ResponseItem::WebSearchCall { .. } + | ResponseItem::ImageGenerationCall { .. } + | ResponseItem::Compaction { .. } + | ResponseItem::CompactionTrigger + | ResponseItem::ContextCompaction { .. } + | ResponseItem::Other => {} + } + } + truncate_images(user_images, generated_images) +} + +/// Truncates edit inputs while preserving the newest generated image when possible. +fn truncate_images( + mut user_images: Vec, + mut generated_images: Vec, +) -> Vec { + let mut excess = (user_images.len() + generated_images.len()).saturating_sub(MAX_EDIT_IMAGES); + let drop_generated = excess.min(generated_images.len().saturating_sub(1)); + generated_images.drain(..drop_generated); + excess -= drop_generated; + let drop_user = excess.min(user_images.len()); + user_images.drain(..drop_user); + excess -= drop_user; + generated_images.drain(..excess); + + user_images.extend(generated_images); + user_images +} + +/// Parses the strict model-facing arguments for an image-generation call. +fn parse_args(call: &ToolCall) -> Result { + serde_json::from_str(call.function_arguments()?) + .map_err(|err| FunctionCallError::RespondToModel(err.to_string())) +} + +/// Resolves where generated images for one thread are persisted by the extension. +pub(crate) fn generated_image_output_dir(codex_home: &Path, thread_id: &str) -> PathBuf { + codex_home + .join(GENERATED_IMAGE_ARTIFACTS_DIR) + .join(sanitize_path_component(thread_id)) +} + +fn generated_image_output_path(output_dir: &Path, call_id: &str) -> PathBuf { + output_dir.join(format!("{}.png", sanitize_path_component(call_id))) +} + +fn sanitize_path_component(value: &str) -> String { + let sanitized: String = value + .chars() + .map(|ch| { + if ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' { + ch + } else { + '_' + } + }) + .collect(); + if sanitized.is_empty() { + "generated_image".to_string() + } else { + sanitized + } +} + +async fn persist_generated_image( + output_dir: &Path, + call_id: &str, + result: &str, +) -> Result { + let bytes = BASE64_STANDARD + .decode(result.trim().as_bytes()) + .map_err(|err| format!("invalid image generation payload: {err}"))?; + tokio::fs::create_dir_all(output_dir) + .await + .map_err(|err| err.to_string())?; + tokio::fs::write(generated_image_output_path(output_dir, call_id), bytes) + .await + .map_err(|err| err.to_string())?; + + Ok(format!( + "Generated images are saved to {} as {} by default.\n\ + If you need to use a generated image at another path, copy it and leave the original in place unless the user explicitly asks you to delete it.", + output_dir.display(), + generated_image_output_path(output_dir, call_id).display(), + )) +} + +/// Builds the namespace function schema exposed to the model. +fn imagegen_tool_spec() -> ToolSpec { + let mut schema_value = serde_json::to_value( + SchemaSettings::draft2019_09() + .with(|settings| settings.inline_subschemas = true) + .into_generator() + .into_root_schema_for::(), + ) + .unwrap_or_else(|err| panic!("imagegen schema should serialize: {err}")); + let Value::Object(ref mut schema) = schema_value else { + unreachable!("imagegen root schema must be an object"); + }; + let mut input_schema = Map::new(); + for key in ["properties", "required", "type", "additionalProperties"] { + if let Some(value) = schema.remove(key) { + input_schema.insert(key.to_string(), value); + } + } + ToolSpec::Namespace(ResponsesApiNamespace { + name: IMAGE_GEN_NAMESPACE.to_string(), + description: default_namespace_description(IMAGE_GEN_NAMESPACE), + tools: vec![ResponsesApiNamespaceTool::Function(ResponsesApiTool { + name: IMAGEGEN_TOOL_NAME.to_string(), + description: IMAGEGEN_DESCRIPTION.to_string(), + strict: false, + parameters: parse_tool_input_schema(&Value::Object(input_schema)) + .unwrap_or_else(|err| panic!("imagegen input schema should parse: {err}")), + output_schema: None, + defer_loading: None, + })], + }) +} + +struct GeneratedImageOutput { + result: String, + output_hint: Option, +} + +impl ToolOutput for GeneratedImageOutput { + /// Avoids copying image bytes into tool-call telemetry. + fn log_preview(&self) -> String { + "[generated image]".to_string() + } + + /// Reports a completed images request as successful tool execution. + fn success_for_logging(&self) -> bool { + true + } + + /// Returns generated bytes and persisted-artifact context for the model's follow-up response. + fn to_response_item(&self, call_id: &str, _payload: &ToolPayload) -> ResponseInputItem { + let mut content = vec![FunctionCallOutputContentItem::InputImage { + image_url: format!("data:image/png;base64,{}", self.result), + detail: Some(DEFAULT_IMAGE_DETAIL), + }]; + if let Some(output_hint) = &self.output_hint { + content.push(FunctionCallOutputContentItem::InputText { + text: output_hint.clone(), + }); + } + ResponseInputItem::FunctionCallOutput { + call_id: call_id.to_string(), + output: FunctionCallOutputPayload { + body: FunctionCallOutputBody::ContentItems(content), + success: Some(true), + }, + } + } +} + +#[cfg(test)] +#[path = "tests.rs"] +mod tests; diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 1b0bd15486..116db0a5b5 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -170,6 +170,8 @@ pub enum Feature { ExternalMigration, /// Allow the model to invoke the built-in image generation tool. ImageGeneration, + /// Replace hosted image generation with the standalone image-generation extension. + ImageGenExt, /// Allow prompting and installing missing MCP dependencies. SkillMcpDependencyInstall, /// Removed compatibility flag for deleted skill env var dependency prompting. @@ -1053,6 +1055,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Stable, default_enabled: true, }, + FeatureSpec { + id: Feature::ImageGenExt, + key: "imagegenext", + stage: Stage::UnderDevelopment, + default_enabled: false, + }, FeatureSpec { id: Feature::SkillMcpDependencyInstall, key: "skill_mcp_dependency_install", diff --git a/codex-rs/features/src/tests.rs b/codex-rs/features/src/tests.rs index b8cd2fd5c6..5d7087e90b 100644 --- a/codex-rs/features/src/tests.rs +++ b/codex-rs/features/src/tests.rs @@ -244,6 +244,13 @@ fn image_generation_is_stable_and_enabled_by_default() { assert_eq!(Feature::ImageGeneration.default_enabled(), true); } +#[test] +fn image_generation_extension_is_under_development_and_disabled_by_default() { + assert_eq!(Feature::ImageGenExt.stage(), Stage::UnderDevelopment); + assert_eq!(Feature::ImageGenExt.default_enabled(), false); + assert_eq!(feature_for_key("imagegenext"), Some(Feature::ImageGenExt)); +} + #[test] fn use_legacy_landlock_config_records_deprecation_notice() { let mut entries = BTreeMap::new(); From 9dd39f334e8f9f2953f2314f6cb02e6307d26949 Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Thu, 28 May 2026 12:43:04 -0700 Subject: [PATCH 08/10] Move Bazel Windows jobs onto codex-runners (#24952) The codex-windows runner group should be much faster than the default GHA runners. Since bazel jobs on windows are frequently the long pole for PRs checks, this will hopefully get people landing a bit faster. --- .github/workflows/bazel.yml | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/.github/workflows/bazel.yml b/.github/workflows/bazel.yml index 11c0988ceb..153ace0fc9 100644 --- a/.github/workflows/bazel.yml +++ b/.github/workflows/bazel.yml @@ -141,7 +141,9 @@ jobs: - 2 - 3 - 4 - runs-on: windows-latest + runs-on: + group: codex-runners + labels: codex-windows-x64 name: Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard ${{ matrix.shard }}/4 steps: @@ -246,7 +248,9 @@ jobs: # it a larger timeout. if: github.event_name == 'push' && github.ref == 'refs/heads/main' timeout-minutes: 40 - runs-on: windows-latest + runs-on: + group: codex-runners + labels: codex-windows-x64 name: Bazel test on windows-latest for x86_64-pc-windows-gnullvm (native main) steps: @@ -332,7 +336,10 @@ jobs: target: aarch64-apple-darwin - os: windows-latest target: x86_64-pc-windows-gnullvm - runs-on: ${{ matrix.os }} + runs_on: + group: codex-runners + labels: codex-windows-x64 + runs-on: ${{ matrix.runs_on || matrix.os }} name: Bazel clippy on ${{ matrix.os }} for ${{ matrix.target }} steps: @@ -422,7 +429,10 @@ jobs: target: aarch64-apple-darwin - os: windows-latest target: x86_64-pc-windows-gnullvm - runs-on: ${{ matrix.os }} + runs_on: + group: codex-runners + labels: codex-windows-x64 + runs-on: ${{ matrix.runs_on || matrix.os }} name: Verify release build on ${{ matrix.os }} for ${{ matrix.target }} steps: From b90ec46387927582371ad737cd22981bf26e1614 Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Thu, 28 May 2026 12:43:30 -0700 Subject: [PATCH 09/10] Add `codex app-server --stdio` alias (#24940) ## Summary - Add `--stdio` as a direct alias for `codex app-server --listen stdio://`. - Keep `--stdio` and `--listen` mutually exclusive. - Update the app-server README to document both forms. --- codex-rs/app-server/README.md | 2 +- codex-rs/cli/src/main.rs | 30 +++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index b91b206d6b..25f300e5d2 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -23,7 +23,7 @@ Similar to [MCP](https://modelcontextprotocol.io/), `codex app-server` supports Supported transports: -- stdio (`--listen stdio://`, default): newline-delimited JSON (JSONL) +- stdio (`--stdio` or `--listen stdio://`, default): newline-delimited JSON (JSONL) - websocket (`--listen ws://IP:PORT`): one JSON-RPC message per websocket text frame (**experimental / unsupported**) - unix socket (`--listen unix://` or `--listen unix://PATH`): websocket connections over `$CODEX_HOME/app-server-control/app-server-control.sock` or a custom socket path, using the standard HTTP Upgrade handshake - off (`--listen off`): do not expose a local transport diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index e0812e3fdc..8dfe6dc33f 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -452,6 +452,10 @@ struct AppServerCommand { )] listen: codex_app_server::AppServerTransport, + /// Use stdio as the transport (equivalent to `--listen stdio://`). + #[arg(long = "stdio", conflicts_with = "listen")] + stdio: bool, + /// Enable remote control for this app-server process. #[arg(long = "remote-control", hide = true)] remote_control: bool, @@ -965,6 +969,7 @@ async fn cli_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> { subcommand, strict_config: app_server_strict_config, listen, + stdio, remote_control, analytics_default_enabled, auth, @@ -978,7 +983,11 @@ async fn cli_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> { )?; match subcommand { None => { - let transport = listen; + let transport = if stdio { + codex_app_server::AppServerTransport::Stdio + } else { + listen + }; let auth = auth.try_into_settings()?; let runtime_options = codex_app_server::AppServerRuntimeOptions { remote_control_enabled: remote_control, @@ -3197,6 +3206,25 @@ mod tests { ); } + #[test] + fn app_server_stdio_flag_parses() { + let app_server = app_server_from_args(["codex", "app-server", "--stdio"].as_ref()); + assert!(app_server.stdio); + } + + #[test] + fn app_server_stdio_flag_conflicts_with_listen() { + let err = MultitoolCli::try_parse_from([ + "codex", + "app-server", + "--stdio", + "--listen", + "stdio://", + ]) + .expect_err("--stdio and --listen should be rejected together"); + assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); + } + #[test] fn app_server_listen_unix_socket_url_parses() { let app_server = From 2e0c4f4977a042c90505ac5465a924192b198872 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Thu, 28 May 2026 16:53:59 -0300 Subject: [PATCH 10/10] fix(tui): prevent repository-configured code execution in /diff (#24954) ## Why `/diff` is intended to display working-tree changes, but its Git invocations honored repository-selected executable helpers. A repository could configure diff/text conversion helpers, clean/process filters, `core.fsmonitor`, or `post-index-change` hooks that execute when a user runs `/diff`. Fixes [PSEC-4395](https://linear.app/openai/issue/PSEC-4395/codex-cli-diff-executes-repository-selected-diff-helpers). ## What Changed - Pass `--no-textconv` and `--no-ext-diff` for tracked and untracked diff generation. - Discover configured `filter..clean` and `.process` entries, then neutralize the selected drivers through structured `GIT_CONFIG_KEY_*` / `GIT_CONFIG_VALUE_*` overrides, including driver names containing `=`. - Run all `/diff` Git probes with `core.fsmonitor=false` and a null `core.hooksPath`. - Use short submodule reporting while ignoring dirty submodule worktrees, since inspecting a checked-out submodule for dirtiness can execute filters from that child repository. This intentionally omits dirty-only submodule markers in order to preserve the non-executing security boundary. - Add real-Git marker tests covering filters, fsmonitor, hooks, and configured helpers inside checked-out submodules. ## How to Test 1. In a repository with ordinary tracked and untracked edits, run `/diff`. 2. Confirm the normal working-tree diff is shown for top-level files. 3. Run the targeted tests below; they configure executable marker helpers for repository filters, fsmonitor, hooks, and a checked-out submodule, then verify `/diff` does not invoke them. 4. Confirm a dirty-only submodule does not cause Codex to enter the submodule and execute its configured helper. Targeted tests: - `just test -p codex-tui get_git_diff_` Validation note: `just test -p codex-tui` runs the new coverage, but this worktree currently also has two unrelated failing guardian tests: `app::tests::update_feature_flags_disabling_guardian_clears_review_policy_and_restores_default` and `app::tests::update_feature_flags_disabling_guardian_clears_manual_review_policy_without_history`. --- codex-rs/tui/src/get_git_diff.rs | 493 ++++++++++++++++++++++++++++--- 1 file changed, 446 insertions(+), 47 deletions(-) diff --git a/codex-rs/tui/src/get_git_diff.rs b/codex-rs/tui/src/get_git_diff.rs index a7b4b668fb..7f55450707 100644 --- a/codex-rs/tui/src/get_git_diff.rs +++ b/codex-rs/tui/src/get_git_diff.rs @@ -13,6 +13,13 @@ use crate::workspace_command::WorkspaceCommandExecutor; use crate::workspace_command::WorkspaceCommandOutput; const DIFF_COMMAND_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 30); +const DISABLE_FSMONITOR_CONFIG: &str = "core.fsmonitor=false"; +const DISABLE_HOOKS_CONFIG: &str = if cfg!(windows) { + "core.hooksPath=NUL" +} else { + "core.hooksPath=/dev/null" +}; +const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|process)$"; /// Return value of [`get_git_diff`]. /// @@ -27,9 +34,22 @@ pub(crate) async fn get_git_diff( return Ok((false, String::new())); } - // Run tracked diff and untracked file listing in parallel. + // Keep `/diff` informational: repository configuration must not select executable diff helpers. + let diff_config_overrides = diff_filter_config_overrides(runner, cwd).await?; let (tracked_diff_res, untracked_output_res) = tokio::join!( - run_git_capture_diff(runner, cwd, &["diff", "--color"]), + run_git_capture_diff( + runner, + cwd, + &diff_config_overrides, + &[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ] + ), run_git_capture_stdout(runner, cwd, &["ls-files", "--others", "--exclude-standard"]), ); let tracked_diff = tracked_diff_res?; @@ -48,8 +68,19 @@ pub(crate) async fn get_git_diff( .map(str::trim) .filter(|s| !s.is_empty()) { - let args = ["diff", "--color", "--no-index", "--", null_path, file]; - let diff = run_git_capture_diff(runner, cwd, &args).await?; + let args = [ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + "--no-index", + "--", + null_path, + file, + ]; + let diff = run_git_capture_diff(runner, cwd, &diff_config_overrides, &args).await?; untracked_diff.push_str(&diff); } @@ -63,7 +94,7 @@ async fn run_git_capture_stdout( cwd: &Path, args: &[&str], ) -> Result { - let output = run_git_command(runner, cwd, args).await?; + let output = run_git_command(runner, cwd, &[], args).await?; if output.success() { Ok(output.stdout) } else { @@ -79,9 +110,10 @@ async fn run_git_capture_stdout( async fn run_git_capture_diff( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, + config_overrides: &[(String, String)], args: &[&str], ) -> Result { - let output = run_git_command(runner, cwd, args).await?; + let output = run_git_command(runner, cwd, config_overrides, args).await?; if output.success() || output.exit_code == 1 { Ok(output.stdout) } else { @@ -92,32 +124,88 @@ async fn run_git_capture_diff( } } +/// Return Git configuration overrides that prevent configured filter drivers +/// from executing while generating diffs. +async fn diff_filter_config_overrides( + runner: &dyn WorkspaceCommandExecutor, + cwd: &Path, +) -> Result, String> { + let args = [ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]; + let output = run_git_command(runner, cwd, &[], &args).await?; + if output.exit_code != 0 && output.exit_code != 1 { + return Err(format!( + "git {:?} failed with status {}", + args, output.exit_code + )); + } + + let mut drivers = output + .stdout + .split('\0') + .filter_map(|key| { + key.strip_suffix(".clean") + .or_else(|| key.strip_suffix(".process")) + }) + .map(str::to_string) + .collect::>(); + drivers.sort(); + drivers.dedup(); + + Ok(drivers + .into_iter() + .flat_map(|driver| { + [ + (format!("{driver}.clean"), String::new()), + (format!("{driver}.process"), String::new()), + (format!("{driver}.required"), "false".to_string()), + ] + }) + .collect()) +} + /// Determine if the current directory is inside a Git repository. async fn inside_git_repo( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, ) -> Result { - let output = run_git_command(runner, cwd, &["rev-parse", "--is-inside-work-tree"]).await?; + let output = run_git_command(runner, cwd, &[], &["rev-parse", "--is-inside-work-tree"]).await?; Ok(output.success()) } async fn run_git_command( runner: &dyn WorkspaceCommandExecutor, cwd: &Path, + config_overrides: &[(String, String)], args: &[&str], ) -> Result { - let mut argv = Vec::with_capacity(args.len() + 1); + let mut argv = Vec::with_capacity(args.len() + 5); argv.push("git".to_string()); + argv.extend([ + "-c".to_string(), + DISABLE_FSMONITOR_CONFIG.to_string(), + "-c".to_string(), + DISABLE_HOOKS_CONFIG.to_string(), + ]); argv.extend(args.iter().map(|arg| (*arg).to_string())); - runner - .run( - WorkspaceCommand::new(argv) - .cwd(cwd.to_path_buf()) - .timeout(DIFF_COMMAND_TIMEOUT) - .disable_output_cap(), - ) - .await - .map_err(|err| err.to_string()) + let mut command = WorkspaceCommand::new(argv) + .cwd(cwd.to_path_buf()) + .timeout(DIFF_COMMAND_TIMEOUT) + .disable_output_cap(); + if !config_overrides.is_empty() { + command = command.env("GIT_CONFIG_COUNT", config_overrides.len().to_string()); + for (index, (key, value)) in config_overrides.iter().enumerate() { + command = command + .env(format!("GIT_CONFIG_KEY_{index}"), key) + .env(format!("GIT_CONFIG_VALUE_{index}"), value); + } + } + runner.run(command).await.map_err(|err| err.to_string()) } #[cfg(test)] @@ -125,17 +213,24 @@ mod tests { use super::*; use crate::workspace_command::WorkspaceCommandError; use pretty_assertions::assert_eq; + use std::collections::HashMap; use std::collections::VecDeque; + #[cfg(unix)] + use std::fs; use std::future::Future; + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt; use std::path::PathBuf; use std::pin::Pin; + #[cfg(unix)] + use std::process::Command as ProcessCommand; use std::sync::Mutex; #[tokio::test] async fn get_git_diff_returns_not_git_for_non_git_cwd() { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![response( - &["git", "rev-parse", "--is-inside-work-tree"], + git_command(&["rev-parse", "--is-inside-work-tree"]), /*exit_code*/ 128, "", )]); @@ -145,40 +240,61 @@ mod tests { assert_eq!(result, Ok((false, String::new()))); assert_commands( &runner.commands(), - &[&["git", "rev-parse", "--is-inside-work-tree"]], + &[git_command(&["rev-parse", "--is-inside-work-tree"])], &cwd, ); } #[tokio::test] - async fn get_git_diff_concatenates_tracked_and_untracked_diffs() { + async fn get_git_diff_disables_helpers_for_tracked_and_untracked_diffs() { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![ response( - &["git", "rev-parse", "--is-inside-work-tree"], + git_command(&["rev-parse", "--is-inside-work-tree"]), /*exit_code*/ 0, "true\n", ), response( - &["git", "diff", "--color"], + git_command(&[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]), + /*exit_code*/ 0, + "filter.evil.clean\0filter.evil.process\0", + ), + response( + git_command(&[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ]), /*exit_code*/ 1, "tracked\n", ), response( - &["git", "ls-files", "--others", "--exclude-standard"], + git_command(&["ls-files", "--others", "--exclude-standard"]), /*exit_code*/ 0, "new.txt\n", ), response( - &[ - "git", + git_command(&[ "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", "--color", "--no-index", "--", null_device(), "new.txt", - ], + ]), /*exit_code*/ 1, "untracked\n", ), @@ -187,24 +303,44 @@ mod tests { let result = get_git_diff(&runner, &cwd).await; assert_eq!(result, Ok((true, "tracked\nuntracked\n".to_string()))); + let commands = runner.commands(); assert_commands( - &runner.commands(), + &commands, &[ - &["git", "rev-parse", "--is-inside-work-tree"], - &["git", "diff", "--color"], - &["git", "ls-files", "--others", "--exclude-standard"], - &[ - "git", + git_command(&["rev-parse", "--is-inside-work-tree"]), + git_command(&[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]), + git_command(&[ "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ]), + git_command(&["ls-files", "--others", "--exclude-standard"]), + git_command(&[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", "--color", "--no-index", "--", null_device(), "new.txt", - ], + ]), ], &cwd, ); + assert_eq!(commands[2].env, filter_override_env("filter.evil")); + assert_eq!(commands[4].env, filter_override_env("filter.evil")); } #[tokio::test] @@ -212,17 +348,35 @@ mod tests { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![ response( - &["git", "rev-parse", "--is-inside-work-tree"], + git_command(&["rev-parse", "--is-inside-work-tree"]), /*exit_code*/ 0, "true\n", ), response( - &["git", "diff", "--color"], + git_command(&[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]), + /*exit_code*/ 1, + "", + ), + response( + git_command(&[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ]), /*exit_code*/ 1, "tracked\n", ), response( - &["git", "ls-files", "--others", "--exclude-standard"], + git_command(&["ls-files", "--others", "--exclude-standard"]), /*exit_code*/ 0, "", ), @@ -238,13 +392,35 @@ mod tests { let cwd = PathBuf::from("/workspace"); let runner = FakeRunner::new(vec![ response( - &["git", "rev-parse", "--is-inside-work-tree"], + git_command(&["rev-parse", "--is-inside-work-tree"]), /*exit_code*/ 0, "true\n", ), - response(&["git", "diff", "--color"], /*exit_code*/ 2, ""), response( - &["git", "ls-files", "--others", "--exclude-standard"], + git_command(&[ + "config", + "--null", + "--name-only", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]), + /*exit_code*/ 1, + "", + ), + response( + git_command(&[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + "--color", + ]), + /*exit_code*/ 2, + "", + ), + response( + git_command(&["ls-files", "--others", "--exclude-standard"]), /*exit_code*/ 0, "", ), @@ -255,14 +431,180 @@ mod tests { .expect_err("unexpected git diff status should fail"); assert!( - error.contains("git [\"diff\", \"--color\"] failed with status 2"), + error.contains( + "git [\"diff\", \"--no-textconv\", \"--no-ext-diff\", \"--submodule=short\", \"--ignore-submodules=dirty\", \"--color\"] failed with status 2" + ), "unexpected error: {error}", ); } - fn response(argv: &[&str], exit_code: i32, stdout: &str) -> FakeResponse { + #[cfg(unix)] + #[tokio::test] + async fn get_git_diff_does_not_execute_configured_filters_fsmonitor_or_hooks() { + let tempdir = tempfile::tempdir().expect("create temp directory"); + let repo = tempdir.path().join("repo"); + fs::create_dir(&repo).expect("create test repository directory"); + run_git_setup(&repo, &["init", "-q"]); + run_git_setup(&repo, &["config", "user.name", "test"]); + run_git_setup(&repo, &["config", "user.email", "test@example.com"]); + fs::write(repo.join(".gitattributes"), "*.txt filter=x=y\n").expect("write attributes"); + fs::write(repo.join("tracked.txt"), "before\n").expect("write tracked file"); + fs::write(repo.join("unchanged.txt"), "unchanged\n").expect("write unchanged file"); + run_git_setup( + &repo, + &["add", ".gitattributes", "tracked.txt", "unchanged.txt"], + ); + run_git_setup(&repo, &["commit", "-qm", "initial"]); + + let filter_helper = tempdir.path().join("filter-helper.sh"); + let fsmonitor_helper = tempdir.path().join("fsmonitor-helper.sh"); + let hooks_dir = tempdir.path().join("hooks"); + let hook_helper = hooks_dir.join("post-index-change"); + fs::create_dir(&hooks_dir).expect("create hooks directory"); + write_marker_helper(&filter_helper); + write_marker_helper(&fsmonitor_helper); + write_marker_helper(&hook_helper); + run_git_setup( + &repo, + &[ + "config", + "filter.x=y.clean", + filter_helper.to_str().expect("filter helper path"), + ], + ); + run_git_setup( + &repo, + &[ + "config", + "filter.x=y.process", + filter_helper.to_str().expect("filter helper path"), + ], + ); + run_git_setup(&repo, &["config", "filter.x=y.required", "true"]); + run_git_setup( + &repo, + &[ + "config", + "core.fsmonitor", + fsmonitor_helper.to_str().expect("fsmonitor helper path"), + ], + ); + run_git_setup( + &repo, + &[ + "config", + "core.hooksPath", + hooks_dir.to_str().expect("hooks directory path"), + ], + ); + std::thread::sleep(Duration::from_secs(/*secs*/ 1)); + fs::write(repo.join("unchanged.txt"), "unchanged\n").expect("refresh unchanged file"); + fs::write(repo.join("tracked.txt"), "after\n").expect("modify tracked file"); + + let result = get_git_diff(&LocalRunner, &repo) + .await + .expect("generate diff without invoking helpers"); + + assert!(result.1.contains("before")); + assert!(result.1.contains("after")); + assert!(!filter_helper.with_extension("sh.ran").exists()); + assert!(!fsmonitor_helper.with_extension("sh.ran").exists()); + assert!(!hook_helper.with_extension("sh.ran").exists()); + } + + #[cfg(unix)] + #[tokio::test] + async fn get_git_diff_does_not_execute_helpers_while_checking_dirty_submodules() { + let tempdir = tempfile::tempdir().expect("create temp directory"); + let child = tempdir.path().join("child"); + let repo = tempdir.path().join("repo"); + fs::create_dir(&child).expect("create child repository directory"); + fs::create_dir(&repo).expect("create parent repository directory"); + run_git_setup(&child, &["init", "-q"]); + run_git_setup(&child, &["config", "user.name", "test"]); + run_git_setup(&child, &["config", "user.email", "test@example.com"]); + fs::write(child.join(".gitattributes"), "*.txt filter=evil\n") + .expect("write child attributes"); + fs::write(child.join("tracked.txt"), "before\n").expect("write child tracked file"); + run_git_setup(&child, &["add", ".gitattributes", "tracked.txt"]); + run_git_setup(&child, &["commit", "-qm", "initial"]); + + run_git_setup(&repo, &["init", "-q"]); + run_git_setup(&repo, &["config", "user.name", "test"]); + run_git_setup(&repo, &["config", "user.email", "test@example.com"]); + run_git_setup( + &repo, + &[ + "-c", + "protocol.file.allow=always", + "submodule", + "add", + "-q", + child.to_str().expect("child repository path"), + "child", + ], + ); + run_git_setup(&repo, &["commit", "-qm", "add submodule"]); + + let helper = tempdir.path().join("submodule-helper.sh"); + write_marker_helper(&helper); + let checkout = repo.join("child"); + run_git_setup( + &checkout, + &[ + "config", + "filter.evil.clean", + helper.to_str().expect("submodule helper path"), + ], + ); + run_git_setup(&checkout, &["config", "filter.evil.required", "true"]); + std::thread::sleep(Duration::from_secs(/*secs*/ 1)); + fs::write(checkout.join("tracked.txt"), "before\n").expect("refresh child tracked file"); + + let result = get_git_diff(&LocalRunner, &repo) + .await + .expect("generate diff without inspecting submodule worktrees"); + + assert!(result.1.is_empty()); + assert!(!helper.with_extension("sh.ran").exists()); + } + + fn git_command(args: &[&str]) -> Vec { + let mut argv = vec![ + "git".to_string(), + "-c".to_string(), + DISABLE_FSMONITOR_CONFIG.to_string(), + "-c".to_string(), + DISABLE_HOOKS_CONFIG.to_string(), + ]; + argv.extend(args.iter().map(|arg| (*arg).to_string())); + argv + } + + fn filter_override_env(driver: &str) -> HashMap> { + HashMap::from([ + ("GIT_CONFIG_COUNT".to_string(), Some("3".to_string())), + ( + "GIT_CONFIG_KEY_0".to_string(), + Some(format!("{driver}.clean")), + ), + ("GIT_CONFIG_VALUE_0".to_string(), Some(String::new())), + ( + "GIT_CONFIG_KEY_1".to_string(), + Some(format!("{driver}.process")), + ), + ("GIT_CONFIG_VALUE_1".to_string(), Some(String::new())), + ( + "GIT_CONFIG_KEY_2".to_string(), + Some(format!("{driver}.required")), + ), + ("GIT_CONFIG_VALUE_2".to_string(), Some("false".to_string())), + ]) + } + + fn response(argv: Vec, exit_code: i32, stdout: &str) -> FakeResponse { FakeResponse { - argv: argv.iter().map(|arg| (*arg).to_string()).collect(), + argv, output: WorkspaceCommandOutput { exit_code, stdout: stdout.to_string(), @@ -275,15 +617,32 @@ mod tests { if cfg!(windows) { "NUL" } else { "/dev/null" } } - fn assert_commands(commands: &[WorkspaceCommand], expected: &[&[&str]], cwd: &Path) { + #[cfg(unix)] + fn run_git_setup(cwd: &Path, args: &[&str]) { + let status = ProcessCommand::new("git") + .args(args) + .current_dir(cwd) + .status() + .expect("run git setup command"); + assert!(status.success(), "git setup command failed: {args:?}"); + } + + #[cfg(unix)] + fn write_marker_helper(path: &Path) { + fs::write(path, "#!/bin/sh\nprintf ran >> \"$0.ran\"\nexit 1\n") + .expect("write helper script"); + let mut permissions = fs::metadata(path) + .expect("read helper metadata") + .permissions(); + permissions.set_mode(/*mode*/ 0o755); + fs::set_permissions(path, permissions).expect("make helper executable"); + } + + fn assert_commands(commands: &[WorkspaceCommand], expected: &[Vec], cwd: &Path) { let actual: Vec> = commands .iter() .map(|command| command.argv.clone()) .collect(); - let expected: Vec> = expected - .iter() - .map(|argv| argv.iter().map(|arg| (*arg).to_string()).collect()) - .collect(); assert_eq!(actual, expected); for command in commands { @@ -336,4 +695,44 @@ mod tests { }) } } + + #[cfg(unix)] + struct LocalRunner; + + #[cfg(unix)] + impl WorkspaceCommandExecutor for LocalRunner { + fn run( + &self, + command: WorkspaceCommand, + ) -> Pin< + Box< + dyn Future> + + Send + + '_, + >, + > { + Box::pin(async move { + let mut process = ProcessCommand::new(&command.argv[0]); + process + .args(&command.argv[1..]) + .current_dir(command.cwd.expect("test command cwd")); + for (key, value) in command.env { + match value { + Some(value) => { + process.env(key, value); + } + None => { + process.env_remove(key); + } + } + } + let output = process.output().expect("run test command"); + Ok(WorkspaceCommandOutput { + exit_code: output.status.code().expect("test command exit code"), + stdout: String::from_utf8(output.stdout).expect("utf8 stdout"), + stderr: String::from_utf8(output.stderr).expect("utf8 stderr"), + }) + }) + } + } }