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: 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..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", @@ -2058,17 +2059,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" @@ -3050,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 5ef6a45fc3..808cdef1b9 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", @@ -48,6 +47,7 @@ members = [ "ext/extension-api", "ext/goal", "ext/guardian", + "ext/image-generation", "ext/memories", "ext/web-search", "external-agent-migration", @@ -166,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/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/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/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/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/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 = 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/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/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/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 { 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, 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/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(())); 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 391e33e77d..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", @@ -1068,11 +1076,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..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(); @@ -304,6 +311,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/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] 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/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"), + }) + }) + } + } } 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)" + ); + } } 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 &&"`