diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index b29c2e940a..92716cc41b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1348,7 +1348,6 @@ dependencies = [ "axum", "base64 0.22.1", "chrono", - "clap", "codex-app-server-protocol", "codex-arg0", "codex-backend-client", @@ -1363,12 +1362,9 @@ dependencies = [ "codex-protocol", "codex-rmcp-client", "codex-utils-absolute-path", - "codex-utils-cargo-bin", "codex-utils-json-to-toml", "core_test_support", - "futures", "os_info", - "owo-colors", "pretty_assertions", "rmcp", "serde", @@ -1378,7 +1374,6 @@ dependencies = [ "tempfile", "time", "tokio", - "tokio-tungstenite", "toml 0.9.11+spec-1.1.0", "tracing", "tracing-subscriber", @@ -2302,6 +2297,7 @@ dependencies = [ "url", "uuid", "vt100", + "webbrowser", "which", "windows-sys 0.52.0", "winsplit", diff --git a/codex-rs/README.md b/codex-rs/README.md index 9cdd772b2a..1c93852fe2 100644 --- a/codex-rs/README.md +++ b/codex-rs/README.md @@ -97,3 +97,5 @@ This folder is the root of a Cargo workspace. It contains quite a bit of experim - [`exec/`](./exec) "headless" CLI for use in automation. - [`tui/`](./tui) CLI that launches a fullscreen TUI built with [Ratatui](https://ratatui.rs/). - [`cli/`](./cli) CLI multitool that provides the aforementioned CLIs via subcommands. + +If you want to contribute or inspect behavior in detail, start by reading the module-level `README.md` files under each crate and run the project workspace from the top-level `codex-rs` directory so shared config, features, and build scripts stay aligned. diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index a36ebc6a09..de746e7a78 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -718,6 +718,16 @@ "default": false, "description": "Opt into receiving experimental API methods and fields.", "type": "boolean" + }, + "optOutNotificationMethods": { + "description": "Exact notification method names that should be suppressed for this connection (for example `codex/event/session_configured`).", + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] } }, "type": "object" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index b7e922dd17..88fb5ddbdb 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -5610,6 +5610,16 @@ "default": false, "description": "Opt into receiving experimental API methods and fields.", "type": "boolean" + }, + "optOutNotificationMethods": { + "description": "Exact notification method names that should be suppressed for this connection (for example `codex/event/session_configured`).", + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] } }, "type": "object" diff --git a/codex-rs/app-server-protocol/schema/json/v1/InitializeParams.json b/codex-rs/app-server-protocol/schema/json/v1/InitializeParams.json index dd71c717f1..050bcb9c50 100644 --- a/codex-rs/app-server-protocol/schema/json/v1/InitializeParams.json +++ b/codex-rs/app-server-protocol/schema/json/v1/InitializeParams.json @@ -29,6 +29,16 @@ "default": false, "description": "Opt into receiving experimental API methods and fields.", "type": "boolean" + }, + "optOutNotificationMethods": { + "description": "Exact notification method names that should be suppressed for this connection (for example `codex/event/session_configured`).", + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] } }, "type": "object" diff --git a/codex-rs/app-server-protocol/schema/typescript/InitializeCapabilities.ts b/codex-rs/app-server-protocol/schema/typescript/InitializeCapabilities.ts index 24f5302627..a6ac24efcd 100644 --- a/codex-rs/app-server-protocol/schema/typescript/InitializeCapabilities.ts +++ b/codex-rs/app-server-protocol/schema/typescript/InitializeCapabilities.ts @@ -9,4 +9,9 @@ export type InitializeCapabilities = { /** * Opt into receiving experimental API methods and fields. */ -experimentalApi: boolean, }; +experimentalApi: boolean, +/** + * Exact notification method names that should be suppressed for this + * connection (for example `codex/event/session_configured`). + */ +optOutNotificationMethods?: Array | null, }; diff --git a/codex-rs/app-server-protocol/src/export.rs b/codex-rs/app-server-protocol/src/export.rs index 5c4954b3cc..2cb3f9d1f1 100644 --- a/codex-rs/app-server-protocol/src/export.rs +++ b/codex-rs/app-server-protocol/src/export.rs @@ -1474,7 +1474,9 @@ mod tests { let allow_optional_nullable = path .file_stem() .and_then(|stem| stem.to_str()) - .is_some_and(|stem| stem.ends_with("Params")); + .is_some_and(|stem| { + stem.ends_with("Params") || stem == "InitializeCapabilities" + }); let contents = fs::read_to_string(&path)?; if contents.contains("| undefined") { diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 77d22f4abf..8452328693 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -806,6 +806,94 @@ mod tests { Ok(()) } + #[test] + fn serialize_initialize_with_opt_out_notification_methods() -> Result<()> { + let request = ClientRequest::Initialize { + request_id: RequestId::Integer(42), + params: v1::InitializeParams { + client_info: v1::ClientInfo { + name: "codex_vscode".to_string(), + title: Some("Codex VS Code Extension".to_string()), + version: "0.1.0".to_string(), + }, + capabilities: Some(v1::InitializeCapabilities { + experimental_api: true, + opt_out_notification_methods: Some(vec![ + "codex/event/session_configured".to_string(), + "item/agentMessage/delta".to_string(), + ]), + }), + }, + }; + + assert_eq!( + json!({ + "method": "initialize", + "id": 42, + "params": { + "clientInfo": { + "name": "codex_vscode", + "title": "Codex VS Code Extension", + "version": "0.1.0" + }, + "capabilities": { + "experimentalApi": true, + "optOutNotificationMethods": [ + "codex/event/session_configured", + "item/agentMessage/delta" + ] + } + } + }), + serde_json::to_value(&request)?, + ); + Ok(()) + } + + #[test] + fn deserialize_initialize_with_opt_out_notification_methods() -> Result<()> { + let request: ClientRequest = serde_json::from_value(json!({ + "method": "initialize", + "id": 42, + "params": { + "clientInfo": { + "name": "codex_vscode", + "title": "Codex VS Code Extension", + "version": "0.1.0" + }, + "capabilities": { + "experimentalApi": true, + "optOutNotificationMethods": [ + "codex/event/session_configured", + "item/agentMessage/delta" + ] + } + } + }))?; + + assert_eq!( + request, + ClientRequest::Initialize { + request_id: RequestId::Integer(42), + params: v1::InitializeParams { + client_info: v1::ClientInfo { + name: "codex_vscode".to_string(), + title: Some("Codex VS Code Extension".to_string()), + version: "0.1.0".to_string(), + }, + capabilities: Some(v1::InitializeCapabilities { + experimental_api: true, + opt_out_notification_methods: Some(vec![ + "codex/event/session_configured".to_string(), + "item/agentMessage/delta".to_string(), + ]), + }), + }, + } + ); + Ok(()) + } + #[test] fn conversation_id_serializes_as_plain_string() -> Result<()> { let id = ThreadId::from_string("67e55044-10b1-426f-9247-bb680e5fe0c8")?; diff --git a/codex-rs/app-server-protocol/src/protocol/v1.rs b/codex-rs/app-server-protocol/src/protocol/v1.rs index 09b4130b5d..25e60ca751 100644 --- a/codex-rs/app-server-protocol/src/protocol/v1.rs +++ b/codex-rs/app-server-protocol/src/protocol/v1.rs @@ -46,12 +46,16 @@ pub struct ClientInfo { } /// Client-declared capabilities negotiated during initialize. -#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, Default, JsonSchema, TS)] +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Default, JsonSchema, TS)] #[serde(rename_all = "camelCase")] pub struct InitializeCapabilities { /// Opt into receiving experimental API methods and fields. #[serde(default)] pub experimental_api: bool, + /// Exact notification method names that should be suppressed for this + /// connection (for example `codex/event/session_configured`). + #[ts(optional = nullable)] + pub opt_out_notification_methods: Option>, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server-test-client/src/lib.rs b/codex-rs/app-server-test-client/src/lib.rs index 90f6adf572..415f45e4a1 100644 --- a/codex-rs/app-server-test-client/src/lib.rs +++ b/codex-rs/app-server-test-client/src/lib.rs @@ -511,6 +511,7 @@ impl CodexClient { }, capabilities: Some(InitializeCapabilities { experimental_api: true, + opt_out_notification_methods: None, }), }, }; diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index f68e078775..a4e848213a 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -30,12 +30,8 @@ codex-protocol = { workspace = true } codex-app-server-protocol = { workspace = true } codex-feedback = { workspace = true } codex-rmcp-client = { workspace = true } -codex-utils-absolute-path = { workspace = true } codex-utils-json-to-toml = { workspace = true } chrono = { workspace = true } -clap = { workspace = true, features = ["derive"] } -futures = { workspace = true } -owo-colors = { workspace = true, features = ["supports-colors"] } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } tempfile = { workspace = true } @@ -48,7 +44,6 @@ tokio = { workspace = true, features = [ "rt-multi-thread", "signal", ] } -tokio-tungstenite = { workspace = true } tracing = { workspace = true, features = ["log"] } tracing-subscriber = { workspace = true, features = ["env-filter", "fmt"] } uuid = { workspace = true, features = ["serde", "v7"] } @@ -62,8 +57,8 @@ axum = { workspace = true, default-features = false, features = [ ] } base64 = { workspace = true } codex-execpolicy = { workspace = true } +codex-utils-absolute-path = { workspace = true } core_test_support = { workspace = true } -codex-utils-cargo-bin = { workspace = true } os_info = { workspace = true } pretty_assertions = { workspace = true } rmcp = { workspace = true, default-features = false, features = [ @@ -71,6 +66,5 @@ rmcp = { workspace = true, default-features = false, features = [ "transport-streamable-http-server", ] } serial_test = { workspace = true } -tokio-tungstenite = { workspace = true } wiremock = { workspace = true } shlex = { workspace = true } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 66d4a501ec..7608262077 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -19,14 +19,7 @@ ## Protocol -Similar to [MCP](https://modelcontextprotocol.io/), `codex app-server` supports bidirectional communication using JSON-RPC 2.0 messages (with the `"jsonrpc":"2.0"` header omitted on the wire). - -Supported transports: - -- stdio (`--listen stdio://`, default): newline-delimited JSON (JSONL) -- websocket (`--listen ws://IP:PORT`): one JSON-RPC message per websocket text frame (**experimental / unsupported**) - -Websocket transport is currently experimental and unsupported. Do not rely on it for production workloads. +Similar to [MCP](https://modelcontextprotocol.io/), `codex app-server` supports bidirectional communication, streaming JSONL over stdio. The protocol is JSON-RPC 2.0, though the `"jsonrpc":"2.0"` header is omitted. ## Message Schema @@ -49,7 +42,7 @@ Use the thread APIs to create, list, or archive conversations. Drive a conversat ## Lifecycle Overview -- Initialize once per connection: Immediately after opening a transport connection, send an `initialize` request with your client metadata, then emit an `initialized` notification. Any other request on that connection before this handshake gets rejected. +- Initialize once: Immediately after launching the codex app-server process, send an `initialize` request with your client metadata, then emit an `initialized` notification. Any other request before this handshake gets rejected. - Start (or resume) a thread: Call `thread/start` to open a fresh conversation. The response returns the thread object and you’ll also get a `thread/started` notification. If you’re continuing an existing conversation, call `thread/resume` with its ID instead. If you want to branch from an existing conversation, call `thread/fork` to create a new thread id with copied history. - Begin a turn: To send user input, call `turn/start` with the target `threadId` and the user's input. Optional fields let you override model, cwd, sandbox policy, etc. This immediately returns the new turn object and triggers a `turn/started` notification. - Stream events: After `turn/start`, keep reading JSON-RPC notifications on stdout. You’ll see `item/started`, `item/completed`, deltas like `item/agentMessage/delta`, tool progress, etc. These represent streaming model output plus any side effects (commands, tool calls, reasoning notes). @@ -57,7 +50,9 @@ Use the thread APIs to create, list, or archive conversations. Drive a conversat ## Initialization -Clients must send a single `initialize` request per transport connection before invoking any other method on that connection, then acknowledge with an `initialized` notification. The server returns the user agent string it will present to upstream services; subsequent requests issued before initialization receive a `"Not initialized"` error, and repeated `initialize` calls on the same connection receive an `"Already initialized"` error. +Clients must send a single `initialize` request before invoking any other method, then acknowledge with an `initialized` notification. The server returns the user agent string it will present to upstream services; subsequent requests issued before initialization receive a `"Not initialized"` error, and repeated `initialize` calls receive an `"Already initialized"` error. + +`initialize.params.capabilities` also supports per-connection notification opt-out via `optOutNotificationMethods`, which is a list of exact method names to suppress for that connection. Matching is exact (no wildcards/prefixes). Unknown method names are accepted and ignored. Applications building on top of `codex app-server` should identify themselves via the `clientInfo` parameter. @@ -81,6 +76,29 @@ Example (from OpenAI's official VSCode extension): } ``` +Example with notification opt-out: + +```json +{ + "method": "initialize", + "id": 1, + "params": { + "clientInfo": { + "name": "my_client", + "title": "My Client", + "version": "0.1.0" + }, + "capabilities": { + "experimentalApi": true, + "optOutNotificationMethods": [ + "codex/event/session_configured", + "item/agentMessage/delta" + ] + } + } +} +``` + ## API Overview - `thread/start` — create a new thread; emits `thread/started` and auto-subscribes you to turn/item events for that thread. @@ -487,6 +505,20 @@ Notes: Event notifications are the server-initiated event stream for thread lifecycles, turn lifecycles, and the items within them. After you start or resume a thread, keep reading stdout for `thread/started`, `turn/*`, and `item/*` notifications. +### Notification opt-out + +Clients can suppress specific notifications per connection by sending exact method names in `initialize.params.capabilities.optOutNotificationMethods`. + +- Exact-match only: `item/agentMessage/delta` suppresses only that method. +- Unknown method names are ignored. +- Applies to both legacy (`codex/event/*`) and v2 (`thread/*`, `turn/*`, `item/*`, etc.) notifications. +- Does not apply to requests/responses/errors. + +Examples: + +- Opt out of legacy session setup event: `codex/event/session_configured` +- Opt out of streamed agent text deltas: `item/agentMessage/delta` + ### Turn events The app-server streams JSON-RPC notifications while a turn is running. Each turn starts with `turn/started` (initial `turn`) and ends with `turn/completed` (final `turn` status). Token usage events stream separately via `thread/tokenUsage/updated`. Clients subscribe to the events they care about, rendering each item incrementally as updates arrive. The per-item lifecycle is always: `item/started` → zero or more item-specific deltas → `item/completed`. diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index bdf0a58baa..73647b1c08 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -140,7 +140,7 @@ pub(crate) async fn apply_bespoke_event_handling( ApiVersion::V1 => { let params = ApplyPatchApprovalParams { conversation_id, - call_id, + call_id: call_id.clone(), file_changes: changes.clone(), reason, grant_root, @@ -149,7 +149,7 @@ pub(crate) async fn apply_bespoke_event_handling( .send_request(ServerRequestPayload::ApplyPatchApproval(params)) .await; tokio::spawn(async move { - on_patch_approval_response(event_turn_id, rx, conversation).await; + on_patch_approval_response(call_id, rx, conversation).await; }); } ApiVersion::V2 => { @@ -216,7 +216,7 @@ pub(crate) async fn apply_bespoke_event_handling( ApiVersion::V1 => { let params = ExecCommandApprovalParams { conversation_id, - call_id, + call_id: call_id.clone(), command, cwd, reason, @@ -226,7 +226,7 @@ pub(crate) async fn apply_bespoke_event_handling( .send_request(ServerRequestPayload::ExecCommandApproval(params)) .await; tokio::spawn(async move { - on_exec_approval_response(event_turn_id, rx, conversation).await; + on_exec_approval_response(call_id, event_turn_id, rx, conversation).await; }); } ApiVersion::V2 => { @@ -1115,7 +1115,7 @@ pub(crate) async fn apply_bespoke_event_handling( ), data: None, }; - outgoing.send_error(request_id.clone(), error).await; + outgoing.send_error(request_id, error).await; return; } } @@ -1129,7 +1129,7 @@ pub(crate) async fn apply_bespoke_event_handling( ), data: None, }; - outgoing.send_error(request_id.clone(), error).await; + outgoing.send_error(request_id, error).await; return; } }; @@ -1428,7 +1428,7 @@ async fn handle_error( } async fn on_patch_approval_response( - event_turn_id: String, + call_id: String, receiver: oneshot::Receiver, codex: Arc, ) { @@ -1439,7 +1439,7 @@ async fn on_patch_approval_response( error!("request failed: {err:?}"); if let Err(submit_err) = codex .submit(Op::PatchApproval { - id: event_turn_id.clone(), + id: call_id.clone(), decision: ReviewDecision::Denied, }) .await @@ -1460,7 +1460,7 @@ async fn on_patch_approval_response( if let Err(err) = codex .submit(Op::PatchApproval { - id: event_turn_id, + id: call_id, decision: response.decision, }) .await @@ -1470,7 +1470,8 @@ async fn on_patch_approval_response( } async fn on_exec_approval_response( - event_turn_id: String, + call_id: String, + turn_id: String, receiver: oneshot::Receiver, conversation: Arc, ) { @@ -1496,7 +1497,8 @@ async fn on_exec_approval_response( if let Err(err) = conversation .submit(Op::ExecApproval { - id: event_turn_id, + id: call_id, + turn_id: Some(turn_id), decision: response.decision, }) .await @@ -1678,7 +1680,7 @@ async fn on_file_change_request_approval_response( if let Some(status) = completion_status { complete_file_change_item( conversation_id, - item_id, + item_id.clone(), changes, status, event_turn_id.clone(), @@ -1690,7 +1692,7 @@ async fn on_file_change_request_approval_response( if let Err(err) = codex .submit(Op::PatchApproval { - id: event_turn_id, + id: item_id, decision, }) .await @@ -1771,7 +1773,8 @@ async fn on_command_execution_request_approval_response( if let Err(err) = conversation .submit(Op::ExecApproval { - id: event_turn_id, + id: item_id, + turn_id: Some(event_turn_id), decision, }) .await @@ -1891,7 +1894,6 @@ async fn construct_mcp_tool_call_end_notification( mod tests { use super::*; use crate::CHANNEL_CAPACITY; - use crate::outgoing_message::OutgoingEnvelope; use crate::outgoing_message::OutgoingMessage; use crate::outgoing_message::OutgoingMessageSender; use anyhow::Result; @@ -1921,21 +1923,6 @@ mod tests { Arc::new(Mutex::new(HashMap::new())) } - async fn recv_broadcast_message( - rx: &mut mpsc::Receiver, - ) -> Result { - let envelope = rx - .recv() - .await - .ok_or_else(|| anyhow!("should send one message"))?; - match envelope { - OutgoingEnvelope::Broadcast { message } => Ok(message), - OutgoingEnvelope::ToConnection { connection_id, .. } => { - bail!("unexpected targeted message for connection {connection_id:?}") - } - } - } - #[test] fn file_change_accept_for_session_maps_to_approved_for_session() { let (decision, completion_status) = @@ -2037,7 +2024,10 @@ mod tests { ) .await; - let msg = recv_broadcast_message(&mut rx).await?; + let msg = rx + .recv() + .await + .ok_or_else(|| anyhow!("should send one notification"))?; match msg { OutgoingMessage::AppServerNotification(ServerNotification::TurnCompleted(n)) => { assert_eq!(n.turn.id, event_turn_id); @@ -2076,7 +2066,10 @@ mod tests { ) .await; - let msg = recv_broadcast_message(&mut rx).await?; + let msg = rx + .recv() + .await + .ok_or_else(|| anyhow!("should send one notification"))?; match msg { OutgoingMessage::AppServerNotification(ServerNotification::TurnCompleted(n)) => { assert_eq!(n.turn.id, event_turn_id); @@ -2115,7 +2108,10 @@ mod tests { ) .await; - let msg = recv_broadcast_message(&mut rx).await?; + let msg = rx + .recv() + .await + .ok_or_else(|| anyhow!("should send one notification"))?; match msg { OutgoingMessage::AppServerNotification(ServerNotification::TurnCompleted(n)) => { assert_eq!(n.turn.id, event_turn_id); @@ -2164,7 +2160,10 @@ mod tests { ) .await; - let msg = recv_broadcast_message(&mut rx).await?; + let msg = rx + .recv() + .await + .ok_or_else(|| anyhow!("should send one notification"))?; match msg { OutgoingMessage::AppServerNotification(ServerNotification::TurnPlanUpdated(n)) => { assert_eq!(n.thread_id, conversation_id.to_string()); @@ -2232,7 +2231,10 @@ mod tests { ) .await; - let first = recv_broadcast_message(&mut rx).await?; + let first = rx + .recv() + .await + .ok_or_else(|| anyhow!("expected usage notification"))?; match first { OutgoingMessage::AppServerNotification( ServerNotification::ThreadTokenUsageUpdated(payload), @@ -2248,7 +2250,10 @@ mod tests { other => bail!("unexpected notification: {other:?}"), } - let second = recv_broadcast_message(&mut rx).await?; + let second = rx + .recv() + .await + .ok_or_else(|| anyhow!("expected rate limit notification"))?; match second { OutgoingMessage::AppServerNotification( ServerNotification::AccountRateLimitsUpdated(payload), @@ -2385,7 +2390,10 @@ mod tests { .await; // Verify: A turn 1 - let msg = recv_broadcast_message(&mut rx).await?; + let msg = rx + .recv() + .await + .ok_or_else(|| anyhow!("should send first notification"))?; match msg { OutgoingMessage::AppServerNotification(ServerNotification::TurnCompleted(n)) => { assert_eq!(n.turn.id, a_turn1); @@ -2403,7 +2411,10 @@ mod tests { } // Verify: B turn 1 - let msg = recv_broadcast_message(&mut rx).await?; + let msg = rx + .recv() + .await + .ok_or_else(|| anyhow!("should send second notification"))?; match msg { OutgoingMessage::AppServerNotification(ServerNotification::TurnCompleted(n)) => { assert_eq!(n.turn.id, b_turn1); @@ -2421,7 +2432,10 @@ mod tests { } // Verify: A turn 2 - let msg = recv_broadcast_message(&mut rx).await?; + let msg = rx + .recv() + .await + .ok_or_else(|| anyhow!("should send third notification"))?; match msg { OutgoingMessage::AppServerNotification(ServerNotification::TurnCompleted(n)) => { assert_eq!(n.turn.id, a_turn2); @@ -2587,7 +2601,10 @@ mod tests { ) .await; - let msg = recv_broadcast_message(&mut rx).await?; + let msg = rx + .recv() + .await + .ok_or_else(|| anyhow!("should send one notification"))?; match msg { OutgoingMessage::AppServerNotification(ServerNotification::TurnDiffUpdated( notification, diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index c161636e07..376069f5f1 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -3,8 +3,6 @@ use crate::error_code::INTERNAL_ERROR_CODE; use crate::error_code::INVALID_REQUEST_ERROR_CODE; use crate::fuzzy_file_search::run_fuzzy_file_search; use crate::models::supported_models; -use crate::outgoing_message::ConnectionId; -use crate::outgoing_message::ConnectionRequestId; use crate::outgoing_message::OutgoingMessageSender; use crate::outgoing_message::OutgoingNotification; use chrono::DateTime; @@ -85,6 +83,7 @@ use codex_app_server_protocol::NewConversationParams; use codex_app_server_protocol::NewConversationResponse; use codex_app_server_protocol::RemoveConversationListenerParams; use codex_app_server_protocol::RemoveConversationSubscriptionResponse; +use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ResumeConversationParams; use codex_app_server_protocol::ResumeConversationResponse; use codex_app_server_protocol::ReviewDelivery as ApiReviewDelivery; @@ -253,10 +252,10 @@ use uuid::Uuid; use crate::filters::compute_source_filters; use crate::filters::source_kind_matches; -type PendingInterruptQueue = Vec<(ConnectionRequestId, ApiVersion)>; +type PendingInterruptQueue = Vec<(RequestId, ApiVersion)>; pub(crate) type PendingInterrupts = Arc>>; -pub(crate) type PendingRollbacks = Arc>>; +pub(crate) type PendingRollbacks = Arc>>; /// Per-conversation accumulation of the latest states e.g. error message while a turn runs. #[derive(Default, Clone)] @@ -487,137 +486,103 @@ impl CodexMessageProcessor { Ok((review_request, hint)) } - pub async fn process_request(&mut self, connection_id: ConnectionId, request: ClientRequest) { - let to_connection_request_id = |request_id| ConnectionRequestId { - connection_id, - request_id, - }; - + pub async fn process_request(&mut self, request: ClientRequest) { match request { ClientRequest::Initialize { .. } => { panic!("Initialize should be handled in MessageProcessor"); } // === v2 Thread/Turn APIs === ClientRequest::ThreadStart { request_id, params } => { - self.thread_start(to_connection_request_id(request_id), params) - .await; + self.thread_start(request_id, params).await; } ClientRequest::ThreadResume { request_id, params } => { - self.thread_resume(to_connection_request_id(request_id), params) - .await; + self.thread_resume(request_id, params).await; } ClientRequest::ThreadFork { request_id, params } => { - self.thread_fork(to_connection_request_id(request_id), params) - .await; + self.thread_fork(request_id, params).await; } ClientRequest::ThreadArchive { request_id, params } => { - self.thread_archive(to_connection_request_id(request_id), params) - .await; + self.thread_archive(request_id, params).await; } ClientRequest::ThreadSetName { request_id, params } => { - self.thread_set_name(to_connection_request_id(request_id), params) - .await; + self.thread_set_name(request_id, params).await; } ClientRequest::ThreadUnarchive { request_id, params } => { - self.thread_unarchive(to_connection_request_id(request_id), params) - .await; + self.thread_unarchive(request_id, params).await; } ClientRequest::ThreadCompactStart { request_id, params } => { - self.thread_compact_start(to_connection_request_id(request_id), params) - .await; + self.thread_compact_start(request_id, params).await; } ClientRequest::ThreadBackgroundTerminalsClean { request_id, params } => { - self.thread_background_terminals_clean( - to_connection_request_id(request_id), - params, - ) - .await; + self.thread_background_terminals_clean(request_id, params) + .await; } ClientRequest::ThreadRollback { request_id, params } => { - self.thread_rollback(to_connection_request_id(request_id), params) - .await; + self.thread_rollback(request_id, params).await; } ClientRequest::ThreadList { request_id, params } => { - self.thread_list(to_connection_request_id(request_id), params) - .await; + self.thread_list(request_id, params).await; } ClientRequest::ThreadLoadedList { request_id, params } => { - self.thread_loaded_list(to_connection_request_id(request_id), params) - .await; + self.thread_loaded_list(request_id, params).await; } ClientRequest::ThreadRead { request_id, params } => { - self.thread_read(to_connection_request_id(request_id), params) - .await; + self.thread_read(request_id, params).await; } ClientRequest::SkillsList { request_id, params } => { - self.skills_list(to_connection_request_id(request_id), params) - .await; + self.skills_list(request_id, params).await; } ClientRequest::SkillsRemoteRead { request_id, params } => { - self.skills_remote_read(to_connection_request_id(request_id), params) - .await; + self.skills_remote_read(request_id, params).await; } ClientRequest::SkillsRemoteWrite { request_id, params } => { - self.skills_remote_write(to_connection_request_id(request_id), params) - .await; + self.skills_remote_write(request_id, params).await; } ClientRequest::AppsList { request_id, params } => { - self.apps_list(to_connection_request_id(request_id), params) - .await; + self.apps_list(request_id, params).await; } ClientRequest::SkillsConfigWrite { request_id, params } => { - self.skills_config_write(to_connection_request_id(request_id), params) - .await; + self.skills_config_write(request_id, params).await; } ClientRequest::TurnStart { request_id, params } => { - self.turn_start(to_connection_request_id(request_id), params) - .await; + self.turn_start(request_id, params).await; } ClientRequest::TurnSteer { request_id, params } => { - self.turn_steer(to_connection_request_id(request_id), params) - .await; + self.turn_steer(request_id, params).await; } ClientRequest::TurnInterrupt { request_id, params } => { - self.turn_interrupt(to_connection_request_id(request_id), params) - .await; + self.turn_interrupt(request_id, params).await; } ClientRequest::ReviewStart { request_id, params } => { - self.review_start(to_connection_request_id(request_id), params) - .await; + self.review_start(request_id, params).await; } ClientRequest::NewConversation { request_id, params } => { // Do not tokio::spawn() to process new_conversation() // asynchronously because we need to ensure the conversation is // created before processing any subsequent messages. - self.process_new_conversation(to_connection_request_id(request_id), params) - .await; + self.process_new_conversation(request_id, params).await; } ClientRequest::GetConversationSummary { request_id, params } => { - self.get_thread_summary(to_connection_request_id(request_id), params) - .await; + self.get_thread_summary(request_id, params).await; } ClientRequest::ListConversations { request_id, params } => { - self.handle_list_conversations(to_connection_request_id(request_id), params) - .await; + self.handle_list_conversations(request_id, params).await; } ClientRequest::ModelList { request_id, params } => { let outgoing = self.outgoing.clone(); let thread_manager = self.thread_manager.clone(); let config = self.config.clone(); - let request_id = to_connection_request_id(request_id); tokio::spawn(async move { Self::list_models(outgoing, thread_manager, config, request_id, params).await; }); } ClientRequest::ExperimentalFeatureList { request_id, params } => { - self.experimental_feature_list(to_connection_request_id(request_id), params) - .await; + self.experimental_feature_list(request_id, params).await; } ClientRequest::CollaborationModeList { request_id, params } => { let outgoing = self.outgoing.clone(); let thread_manager = self.thread_manager.clone(); - let request_id = to_connection_request_id(request_id); tokio::spawn(async move { Self::list_collaboration_modes(outgoing, thread_manager, request_id, params) @@ -625,136 +590,109 @@ impl CodexMessageProcessor { }); } ClientRequest::MockExperimentalMethod { request_id, params } => { - self.mock_experimental_method(to_connection_request_id(request_id), params) - .await; + self.mock_experimental_method(request_id, params).await; } ClientRequest::McpServerOauthLogin { request_id, params } => { - self.mcp_server_oauth_login(to_connection_request_id(request_id), params) - .await; + self.mcp_server_oauth_login(request_id, params).await; } ClientRequest::McpServerRefresh { request_id, params } => { - self.mcp_server_refresh(to_connection_request_id(request_id), params) - .await; + self.mcp_server_refresh(request_id, params).await; } ClientRequest::McpServerStatusList { request_id, params } => { - self.list_mcp_server_status(to_connection_request_id(request_id), params) - .await; + self.list_mcp_server_status(request_id, params).await; } ClientRequest::LoginAccount { request_id, params } => { - self.login_v2(to_connection_request_id(request_id), params) - .await; + self.login_v2(request_id, params).await; } ClientRequest::LogoutAccount { request_id, params: _, } => { - self.logout_v2(to_connection_request_id(request_id)).await; + self.logout_v2(request_id).await; } ClientRequest::CancelLoginAccount { request_id, params } => { - self.cancel_login_v2(to_connection_request_id(request_id), params) - .await; + self.cancel_login_v2(request_id, params).await; } ClientRequest::GetAccount { request_id, params } => { - self.get_account(to_connection_request_id(request_id), params) - .await; + self.get_account(request_id, params).await; } ClientRequest::ResumeConversation { request_id, params } => { - self.handle_resume_conversation(to_connection_request_id(request_id), params) - .await; + self.handle_resume_conversation(request_id, params).await; } ClientRequest::ForkConversation { request_id, params } => { - self.handle_fork_conversation(to_connection_request_id(request_id), params) - .await; + self.handle_fork_conversation(request_id, params).await; } ClientRequest::ArchiveConversation { request_id, params } => { - self.archive_conversation(to_connection_request_id(request_id), params) - .await; + self.archive_conversation(request_id, params).await; } ClientRequest::SendUserMessage { request_id, params } => { - self.send_user_message(to_connection_request_id(request_id), params) - .await; + self.send_user_message(request_id, params).await; } ClientRequest::SendUserTurn { request_id, params } => { - self.send_user_turn(to_connection_request_id(request_id), params) - .await; + self.send_user_turn(request_id, params).await; } ClientRequest::InterruptConversation { request_id, params } => { - self.interrupt_conversation(to_connection_request_id(request_id), params) - .await; + self.interrupt_conversation(request_id, params).await; } ClientRequest::AddConversationListener { request_id, params } => { - self.add_conversation_listener(to_connection_request_id(request_id), params) - .await; + self.add_conversation_listener(request_id, params).await; } ClientRequest::RemoveConversationListener { request_id, params } => { - self.remove_thread_listener(to_connection_request_id(request_id), params) - .await; + self.remove_thread_listener(request_id, params).await; } ClientRequest::GitDiffToRemote { request_id, params } => { - self.git_diff_to_origin(to_connection_request_id(request_id), params.cwd) - .await; + self.git_diff_to_origin(request_id, params.cwd).await; } ClientRequest::LoginApiKey { request_id, params } => { - self.login_api_key_v1(to_connection_request_id(request_id), params) - .await; + self.login_api_key_v1(request_id, params).await; } ClientRequest::LoginChatGpt { request_id, params: _, } => { - self.login_chatgpt_v1(to_connection_request_id(request_id)) - .await; + self.login_chatgpt_v1(request_id).await; } ClientRequest::CancelLoginChatGpt { request_id, params } => { - self.cancel_login_chatgpt(to_connection_request_id(request_id), params.login_id) - .await; + self.cancel_login_chatgpt(request_id, params.login_id).await; } ClientRequest::LogoutChatGpt { request_id, params: _, } => { - self.logout_v1(to_connection_request_id(request_id)).await; + self.logout_v1(request_id).await; } ClientRequest::GetAuthStatus { request_id, params } => { - self.get_auth_status(to_connection_request_id(request_id), params) - .await; + self.get_auth_status(request_id, params).await; } ClientRequest::GetUserSavedConfig { request_id, params: _, } => { - self.get_user_saved_config(to_connection_request_id(request_id)) - .await; + self.get_user_saved_config(request_id).await; } ClientRequest::SetDefaultModel { request_id, params } => { - self.set_default_model(to_connection_request_id(request_id), params) - .await; + self.set_default_model(request_id, params).await; } ClientRequest::GetUserAgent { request_id, params: _, } => { - self.get_user_agent(to_connection_request_id(request_id)) - .await; + self.get_user_agent(request_id).await; } ClientRequest::UserInfo { request_id, params: _, } => { - self.get_user_info(to_connection_request_id(request_id)) - .await; + self.get_user_info(request_id).await; } ClientRequest::FuzzyFileSearch { request_id, params } => { - self.fuzzy_file_search(to_connection_request_id(request_id), params) - .await; + self.fuzzy_file_search(request_id, params).await; } ClientRequest::OneOffCommandExec { request_id, params } => { - self.exec_one_off_command(to_connection_request_id(request_id), params) - .await; + self.exec_one_off_command(request_id, params).await; } ClientRequest::ExecOneOffCommand { request_id, params } => { - self.exec_one_off_command(to_connection_request_id(request_id), params.into()) - .await; + self.exec_one_off_command(request_id, params.into()).await; } ClientRequest::ConfigRead { .. } | ClientRequest::ConfigValueWrite { .. } @@ -768,17 +706,15 @@ impl CodexMessageProcessor { request_id, params: _, } => { - self.get_account_rate_limits(to_connection_request_id(request_id)) - .await; + self.get_account_rate_limits(request_id).await; } ClientRequest::FeedbackUpload { request_id, params } => { - self.upload_feedback(to_connection_request_id(request_id), params) - .await; + self.upload_feedback(request_id, params).await; } } } - async fn login_v2(&mut self, request_id: ConnectionRequestId, params: LoginAccountParams) { + async fn login_v2(&mut self, request_id: RequestId, params: LoginAccountParams) { match params { LoginAccountParams::ApiKey { api_key } => { self.login_api_key_v2(request_id, LoginApiKeyParams { api_key }) @@ -856,11 +792,7 @@ impl CodexMessageProcessor { } } - async fn login_api_key_v1( - &mut self, - request_id: ConnectionRequestId, - params: LoginApiKeyParams, - ) { + async fn login_api_key_v1(&mut self, request_id: RequestId, params: LoginApiKeyParams) { match self.login_api_key_common(¶ms).await { Ok(()) => { self.outgoing @@ -884,11 +816,7 @@ impl CodexMessageProcessor { } } - async fn login_api_key_v2( - &mut self, - request_id: ConnectionRequestId, - params: LoginApiKeyParams, - ) { + async fn login_api_key_v2(&mut self, request_id: RequestId, params: LoginApiKeyParams) { match self.login_api_key_common(¶ms).await { Ok(()) => { let response = codex_app_server_protocol::LoginAccountResponse::ApiKey {}; @@ -952,7 +880,7 @@ impl CodexMessageProcessor { } // Deprecated in favor of login_chatgpt_v2. - async fn login_chatgpt_v1(&mut self, request_id: ConnectionRequestId) { + async fn login_chatgpt_v1(&mut self, request_id: RequestId) { match self.login_chatgpt_common().await { Ok(opts) => match run_login_server(opts) { Ok(server) => { @@ -1058,7 +986,7 @@ impl CodexMessageProcessor { } } - async fn login_chatgpt_v2(&mut self, request_id: ConnectionRequestId) { + async fn login_chatgpt_v2(&mut self, request_id: RequestId) { match self.login_chatgpt_common().await { Ok(opts) => match run_login_server(opts) { Ok(server) => { @@ -1182,7 +1110,7 @@ impl CodexMessageProcessor { } } - async fn cancel_login_chatgpt(&mut self, request_id: ConnectionRequestId, login_id: Uuid) { + async fn cancel_login_chatgpt(&mut self, request_id: RequestId, login_id: Uuid) { match self.cancel_login_chatgpt_common(login_id).await { Ok(()) => { self.outgoing @@ -1200,11 +1128,7 @@ impl CodexMessageProcessor { } } - async fn cancel_login_v2( - &mut self, - request_id: ConnectionRequestId, - params: CancelLoginAccountParams, - ) { + async fn cancel_login_v2(&mut self, request_id: RequestId, params: CancelLoginAccountParams) { let login_id = params.login_id; match Uuid::parse_str(&login_id) { Ok(uuid) => { @@ -1228,7 +1152,7 @@ impl CodexMessageProcessor { async fn login_chatgpt_auth_tokens( &mut self, - request_id: ConnectionRequestId, + request_id: RequestId, access_token: String, chatgpt_account_id: String, chatgpt_plan_type: Option, @@ -1343,7 +1267,7 @@ impl CodexMessageProcessor { .map(CodexAuth::api_auth_mode)) } - async fn logout_v1(&mut self, request_id: ConnectionRequestId) { + async fn logout_v1(&mut self, request_id: RequestId) { match self.logout_common().await { Ok(current_auth_method) => { self.outgoing @@ -1363,7 +1287,7 @@ impl CodexMessageProcessor { } } - async fn logout_v2(&mut self, request_id: ConnectionRequestId) { + async fn logout_v2(&mut self, request_id: RequestId) { match self.logout_common().await { Ok(current_auth_method) => { self.outgoing @@ -1392,7 +1316,7 @@ impl CodexMessageProcessor { } } - async fn get_auth_status(&self, request_id: ConnectionRequestId, params: GetAuthStatusParams) { + async fn get_auth_status(&self, request_id: RequestId, params: GetAuthStatusParams) { let include_token = params.include_token.unwrap_or(false); let do_refresh = params.refresh_token.unwrap_or(false); @@ -1441,7 +1365,7 @@ impl CodexMessageProcessor { self.outgoing.send_response(request_id, response).await; } - async fn get_account(&self, request_id: ConnectionRequestId, params: GetAccountParams) { + async fn get_account(&self, request_id: RequestId, params: GetAccountParams) { let do_refresh = params.refresh_token; self.refresh_token_if_requested(do_refresh).await; @@ -1493,13 +1417,13 @@ impl CodexMessageProcessor { self.outgoing.send_response(request_id, response).await; } - async fn get_user_agent(&self, request_id: ConnectionRequestId) { + async fn get_user_agent(&self, request_id: RequestId) { let user_agent = get_codex_user_agent(); let response = GetUserAgentResponse { user_agent }; self.outgoing.send_response(request_id, response).await; } - async fn get_account_rate_limits(&self, request_id: ConnectionRequestId) { + async fn get_account_rate_limits(&self, request_id: RequestId) { match self.fetch_account_rate_limits().await { Ok(rate_limits) => { let response = GetAccountRateLimitsResponse { @@ -1547,7 +1471,7 @@ impl CodexMessageProcessor { }) } - async fn get_user_saved_config(&self, request_id: ConnectionRequestId) { + async fn get_user_saved_config(&self, request_id: RequestId) { let service = ConfigService::new_with_defaults(self.config.codex_home.clone()); let user_saved_config: UserSavedConfig = match service.load_user_saved_config().await { Ok(config) => config, @@ -1568,7 +1492,7 @@ impl CodexMessageProcessor { self.outgoing.send_response(request_id, response).await; } - async fn get_user_info(&self, request_id: ConnectionRequestId) { + async fn get_user_info(&self, request_id: RequestId) { // Read alleged user email from cached auth (best-effort; not verified). let alleged_user_email = self .auth_manager @@ -1579,11 +1503,7 @@ impl CodexMessageProcessor { self.outgoing.send_response(request_id, response).await; } - async fn set_default_model( - &self, - request_id: ConnectionRequestId, - params: SetDefaultModelParams, - ) { + async fn set_default_model(&self, request_id: RequestId, params: SetDefaultModelParams) { let SetDefaultModelParams { model, reasoning_effort, @@ -1610,22 +1530,16 @@ impl CodexMessageProcessor { } } - async fn exec_one_off_command( - &self, - request_id: ConnectionRequestId, - params: CommandExecParams, - ) { + async fn exec_one_off_command(&self, request_id: RequestId, params: CommandExecParams) { tracing::debug!("ExecOneOffCommand params: {params:?}"); - let request = request_id.clone(); - if params.command.is_empty() { let error = JSONRPCErrorError { code: INVALID_REQUEST_ERROR_CODE, message: "command must not be empty".to_string(), data: None, }; - self.outgoing.send_error(request, error).await; + self.outgoing.send_error(request_id, error).await; return; } @@ -1643,7 +1557,7 @@ impl CodexMessageProcessor { message: format!("failed to start managed network proxy: {err}"), data: None, }; - self.outgoing.send_error(request, error).await; + self.outgoing.send_error(request_id, error).await; return; } }, @@ -1674,7 +1588,7 @@ impl CodexMessageProcessor { message: format!("invalid sandbox policy: {err}"), data: None, }; - self.outgoing.send_error(request, error).await; + self.outgoing.send_error(request_id, error).await; return; } }, @@ -1683,7 +1597,7 @@ impl CodexMessageProcessor { let codex_linux_sandbox_exe = self.config.codex_linux_sandbox_exe.clone(); let outgoing = self.outgoing.clone(); - let request_for_task = request; + let req_id = request_id; let sandbox_cwd = self.config.cwd.clone(); let started_network_proxy_for_task = started_network_proxy; let use_linux_sandbox_bwrap = self.config.features.enabled(Feature::UseLinuxSandboxBwrap); @@ -1706,7 +1620,7 @@ impl CodexMessageProcessor { stdout: output.stdout.text, stderr: output.stderr.text, }; - outgoing.send_response(request_for_task, response).await; + outgoing.send_response(req_id, response).await; } Err(err) => { let error = JSONRPCErrorError { @@ -1714,7 +1628,7 @@ impl CodexMessageProcessor { message: format!("exec failed: {err}"), data: None, }; - outgoing.send_error(request_for_task, error).await; + outgoing.send_error(req_id, error).await; } } }); @@ -1722,7 +1636,7 @@ impl CodexMessageProcessor { async fn process_new_conversation( &mut self, - request_id: ConnectionRequestId, + request_id: RequestId, params: NewConversationParams, ) { let NewConversationParams { @@ -1823,7 +1737,7 @@ impl CodexMessageProcessor { } } - async fn thread_start(&mut self, request_id: ConnectionRequestId, params: ThreadStartParams) { + async fn thread_start(&mut self, request_id: RequestId, params: ThreadStartParams) { let ThreadStartParams { model, model_provider, @@ -1988,11 +1902,7 @@ impl CodexMessageProcessor { } } - async fn thread_archive( - &mut self, - request_id: ConnectionRequestId, - params: ThreadArchiveParams, - ) { + async fn thread_archive(&mut self, request_id: RequestId, params: ThreadArchiveParams) { // TODO(jif) mostly rewrite this using sqlite after phase 1 let thread_id = match ThreadId::from_string(¶ms.thread_id) { Ok(id) => id, @@ -2042,7 +1952,7 @@ impl CodexMessageProcessor { } } - async fn thread_set_name(&self, request_id: ConnectionRequestId, params: ThreadSetNameParams) { + async fn thread_set_name(&self, request_id: RequestId, params: ThreadSetNameParams) { let ThreadSetNameParams { thread_id, name } = params; let Some(name) = codex_core::util::normalize_thread_name(&name) else { self.send_invalid_request_error( @@ -2072,11 +1982,7 @@ impl CodexMessageProcessor { .await; } - async fn thread_unarchive( - &mut self, - request_id: ConnectionRequestId, - params: ThreadUnarchiveParams, - ) { + async fn thread_unarchive(&mut self, request_id: RequestId, params: ThreadUnarchiveParams) { // TODO(jif) mostly rewrite this using sqlite after phase 1 let thread_id = match ThreadId::from_string(¶ms.thread_id) { Ok(id) => id, @@ -2253,11 +2159,7 @@ impl CodexMessageProcessor { } } - async fn thread_rollback( - &mut self, - request_id: ConnectionRequestId, - params: ThreadRollbackParams, - ) { + async fn thread_rollback(&mut self, request_id: RequestId, params: ThreadRollbackParams) { let ThreadRollbackParams { thread_id, num_turns, @@ -2277,20 +2179,18 @@ impl CodexMessageProcessor { } }; - let request = request_id.clone(); - { let mut map = self.pending_rollbacks.lock().await; if map.contains_key(&thread_id) { self.send_invalid_request_error( - request.clone(), + request_id, "rollback already in progress for this thread".to_string(), ) .await; return; } - map.insert(thread_id, request.clone()); + map.insert(thread_id, request_id.clone()); } if let Err(err) = thread.submit(Op::ThreadRollback { num_turns }).await { @@ -2299,16 +2199,12 @@ impl CodexMessageProcessor { let mut map = self.pending_rollbacks.lock().await; map.remove(&thread_id); - self.send_internal_error(request, format!("failed to start rollback: {err}")) + self.send_internal_error(request_id, format!("failed to start rollback: {err}")) .await; } } - async fn thread_compact_start( - &self, - request_id: ConnectionRequestId, - params: ThreadCompactStartParams, - ) { + async fn thread_compact_start(&self, request_id: RequestId, params: ThreadCompactStartParams) { let ThreadCompactStartParams { thread_id } = params; let (_, thread) = match self.load_thread(&thread_id).await { @@ -2334,7 +2230,7 @@ impl CodexMessageProcessor { async fn thread_background_terminals_clean( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: ThreadBackgroundTerminalsCleanParams, ) { let ThreadBackgroundTerminalsCleanParams { thread_id } = params; @@ -2363,7 +2259,7 @@ impl CodexMessageProcessor { } } - async fn thread_list(&self, request_id: ConnectionRequestId, params: ThreadListParams) { + async fn thread_list(&self, request_id: RequestId, params: ThreadListParams) { let ThreadListParams { cursor, limit, @@ -2404,11 +2300,7 @@ impl CodexMessageProcessor { self.outgoing.send_response(request_id, response).await; } - async fn thread_loaded_list( - &self, - request_id: ConnectionRequestId, - params: ThreadLoadedListParams, - ) { + async fn thread_loaded_list(&self, request_id: RequestId, params: ThreadLoadedListParams) { let ThreadLoadedListParams { cursor, limit } = params; let mut data = self .thread_manager @@ -2463,7 +2355,7 @@ impl CodexMessageProcessor { self.outgoing.send_response(request_id, response).await; } - async fn thread_read(&mut self, request_id: ConnectionRequestId, params: ThreadReadParams) { + async fn thread_read(&mut self, request_id: RequestId, params: ThreadReadParams) { let ThreadReadParams { thread_id, include_turns, @@ -2614,7 +2506,7 @@ impl CodexMessageProcessor { } } - async fn thread_resume(&mut self, request_id: ConnectionRequestId, params: ThreadResumeParams) { + async fn thread_resume(&mut self, request_id: RequestId, params: ThreadResumeParams) { let ThreadResumeParams { thread_id, history, @@ -2822,7 +2714,7 @@ impl CodexMessageProcessor { } } - async fn thread_fork(&mut self, request_id: ConnectionRequestId, params: ThreadForkParams) { + async fn thread_fork(&mut self, request_id: RequestId, params: ThreadForkParams) { let ThreadForkParams { thread_id, path, @@ -3027,7 +2919,7 @@ impl CodexMessageProcessor { async fn get_thread_summary( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: GetConversationSummaryParams, ) { if let GetConversationSummaryParams::ThreadId { conversation_id } = ¶ms @@ -3093,7 +2985,7 @@ impl CodexMessageProcessor { async fn handle_list_conversations( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: ListConversationsParams, ) { let ListConversationsParams { @@ -3261,7 +3153,7 @@ impl CodexMessageProcessor { outgoing: Arc, thread_manager: Arc, config: Arc, - request_id: ConnectionRequestId, + request_id: RequestId, params: ModelListParams, ) { let ModelListParams { limit, cursor } = params; @@ -3324,7 +3216,7 @@ impl CodexMessageProcessor { async fn list_collaboration_modes( outgoing: Arc, thread_manager: Arc, - request_id: ConnectionRequestId, + request_id: RequestId, params: CollaborationModeListParams, ) { let CollaborationModeListParams {} = params; @@ -3335,7 +3227,7 @@ impl CodexMessageProcessor { async fn experimental_feature_list( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: ExperimentalFeatureListParams, ) { let ExperimentalFeatureListParams { cursor, limit } = params; @@ -3445,7 +3337,7 @@ impl CodexMessageProcessor { async fn mock_experimental_method( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: MockExperimentalMethodParams, ) { let MockExperimentalMethodParams { value } = params; @@ -3453,7 +3345,7 @@ impl CodexMessageProcessor { self.outgoing.send_response(request_id, response).await; } - async fn mcp_server_refresh(&self, request_id: ConnectionRequestId, _params: Option<()>) { + async fn mcp_server_refresh(&self, request_id: RequestId, _params: Option<()>) { let config = match self.load_latest_config().await { Ok(config) => config, Err(error) => { @@ -3506,7 +3398,7 @@ impl CodexMessageProcessor { async fn mcp_server_oauth_login( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: McpServerOauthLoginParams, ) { let config = match self.load_latest_config().await { @@ -3603,28 +3495,26 @@ impl CodexMessageProcessor { async fn list_mcp_server_status( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: ListMcpServerStatusParams, ) { - let request = request_id.clone(); - let outgoing = Arc::clone(&self.outgoing); let config = match self.load_latest_config().await { Ok(config) => config, Err(error) => { - self.outgoing.send_error(request, error).await; + self.outgoing.send_error(request_id, error).await; return; } }; tokio::spawn(async move { - Self::list_mcp_server_status_task(outgoing, request, params, config).await; + Self::list_mcp_server_status_task(outgoing, request_id, params, config).await; }); } async fn list_mcp_server_status_task( outgoing: Arc, - request_id: ConnectionRequestId, + request_id: RequestId, params: ListMcpServerStatusParams, config: Config, ) { @@ -3707,7 +3597,7 @@ impl CodexMessageProcessor { async fn handle_resume_conversation( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: ResumeConversationParams, ) { let ResumeConversationParams { @@ -3915,7 +3805,7 @@ impl CodexMessageProcessor { async fn handle_fork_conversation( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: ForkConversationParams, ) { let ForkConversationParams { @@ -4111,7 +4001,7 @@ impl CodexMessageProcessor { self.outgoing.send_response(request_id, response).await; } - async fn send_invalid_request_error(&self, request_id: ConnectionRequestId, message: String) { + async fn send_invalid_request_error(&self, request_id: RequestId, message: String) { let error = JSONRPCErrorError { code: INVALID_REQUEST_ERROR_CODE, message, @@ -4120,7 +4010,7 @@ impl CodexMessageProcessor { self.outgoing.send_error(request_id, error).await; } - async fn send_internal_error(&self, request_id: ConnectionRequestId, message: String) { + async fn send_internal_error(&self, request_id: RequestId, message: String) { let error = JSONRPCErrorError { code: INTERNAL_ERROR_CODE, message, @@ -4131,7 +4021,7 @@ impl CodexMessageProcessor { async fn archive_conversation( &mut self, - request_id: ConnectionRequestId, + request_id: RequestId, params: ArchiveConversationParams, ) { let ArchiveConversationParams { @@ -4280,11 +4170,7 @@ impl CodexMessageProcessor { }) } - async fn send_user_message( - &self, - request_id: ConnectionRequestId, - params: SendUserMessageParams, - ) { + async fn send_user_message(&self, request_id: RequestId, params: SendUserMessageParams) { let SendUserMessageParams { conversation_id, items, @@ -4328,7 +4214,7 @@ impl CodexMessageProcessor { .await; } - async fn send_user_turn(&self, request_id: ConnectionRequestId, params: SendUserTurnParams) { + async fn send_user_turn(&self, request_id: RequestId, params: SendUserTurnParams) { let SendUserTurnParams { conversation_id, items, @@ -4386,7 +4272,7 @@ impl CodexMessageProcessor { .await; } - async fn apps_list(&self, request_id: ConnectionRequestId, params: AppsListParams) { + async fn apps_list(&self, request_id: RequestId, params: AppsListParams) { let mut config = match self.load_latest_config().await { Ok(config) => config, Err(error) => { @@ -4433,7 +4319,7 @@ impl CodexMessageProcessor { async fn apps_list_task( outgoing: Arc, - request_id: ConnectionRequestId, + request_id: RequestId, params: AppsListParams, config: Config, ) { @@ -4604,7 +4490,7 @@ impl CodexMessageProcessor { .await; } - async fn skills_list(&self, request_id: ConnectionRequestId, params: SkillsListParams) { + async fn skills_list(&self, request_id: RequestId, params: SkillsListParams) { let SkillsListParams { cwds, force_reload, @@ -4670,11 +4556,7 @@ impl CodexMessageProcessor { .await; } - async fn skills_remote_read( - &self, - request_id: ConnectionRequestId, - _params: SkillsRemoteReadParams, - ) { + async fn skills_remote_read(&self, request_id: RequestId, _params: SkillsRemoteReadParams) { match list_remote_skills(&self.config).await { Ok(skills) => { let data = skills @@ -4699,11 +4581,7 @@ impl CodexMessageProcessor { } } - async fn skills_remote_write( - &self, - request_id: ConnectionRequestId, - params: SkillsRemoteWriteParams, - ) { + async fn skills_remote_write(&self, request_id: RequestId, params: SkillsRemoteWriteParams) { let SkillsRemoteWriteParams { hazelnut_id, is_preload, @@ -4733,11 +4611,7 @@ impl CodexMessageProcessor { } } - async fn skills_config_write( - &self, - request_id: ConnectionRequestId, - params: SkillsConfigWriteParams, - ) { + async fn skills_config_write(&self, request_id: RequestId, params: SkillsConfigWriteParams) { let SkillsConfigWriteParams { path, enabled } = params; let edits = vec![ConfigEdit::SetSkillConfig { path, enabled }]; let result = ConfigEditsBuilder::new(&self.config.codex_home) @@ -4770,7 +4644,7 @@ impl CodexMessageProcessor { async fn interrupt_conversation( &mut self, - request_id: ConnectionRequestId, + request_id: RequestId, params: InterruptConversationParams, ) { let InterruptConversationParams { conversation_id } = params; @@ -4784,21 +4658,19 @@ impl CodexMessageProcessor { return; }; - let request = request_id.clone(); - // Record the pending interrupt so we can reply when TurnAborted arrives. { let mut map = self.pending_interrupts.lock().await; map.entry(conversation_id) .or_default() - .push((request, ApiVersion::V1)); + .push((request_id, ApiVersion::V1)); } // Submit the interrupt; we'll respond upon TurnAborted. let _ = conversation.submit(Op::Interrupt).await; } - async fn turn_start(&self, request_id: ConnectionRequestId, params: TurnStartParams) { + async fn turn_start(&self, request_id: RequestId, params: TurnStartParams) { let (_, thread) = match self.load_thread(¶ms.thread_id).await { Ok(v) => v, Err(error) => { @@ -4884,7 +4756,7 @@ impl CodexMessageProcessor { } } - async fn turn_steer(&self, request_id: ConnectionRequestId, params: TurnSteerParams) { + async fn turn_steer(&self, request_id: RequestId, params: TurnSteerParams) { let (_, thread) = match self.load_thread(¶ms.thread_id).await { Ok(v) => v, Err(error) => { @@ -4965,7 +4837,7 @@ impl CodexMessageProcessor { async fn emit_review_started( &self, - request_id: &ConnectionRequestId, + request_id: &RequestId, turn: Turn, parent_thread_id: String, review_thread_id: String, @@ -4989,7 +4861,7 @@ impl CodexMessageProcessor { async fn start_inline_review( &self, - request_id: &ConnectionRequestId, + request_id: &RequestId, parent_thread: Arc, review_request: ReviewRequest, display_text: &str, @@ -5019,7 +4891,7 @@ impl CodexMessageProcessor { async fn start_detached_review( &mut self, - request_id: &ConnectionRequestId, + request_id: &RequestId, parent_thread_id: ThreadId, review_request: ReviewRequest, display_text: &str, @@ -5111,7 +4983,7 @@ impl CodexMessageProcessor { Ok(()) } - async fn review_start(&mut self, request_id: ConnectionRequestId, params: ReviewStartParams) { + async fn review_start(&mut self, request_id: RequestId, params: ReviewStartParams) { let ReviewStartParams { thread_id, target, @@ -5165,11 +5037,7 @@ impl CodexMessageProcessor { } } - async fn turn_interrupt( - &mut self, - request_id: ConnectionRequestId, - params: TurnInterruptParams, - ) { + async fn turn_interrupt(&mut self, request_id: RequestId, params: TurnInterruptParams) { let TurnInterruptParams { thread_id, .. } = params; let (thread_uuid, thread) = match self.load_thread(&thread_id).await { @@ -5180,14 +5048,12 @@ impl CodexMessageProcessor { } }; - let request = request_id.clone(); - // Record the pending interrupt so we can reply when TurnAborted arrives. { let mut map = self.pending_interrupts.lock().await; map.entry(thread_uuid) .or_default() - .push((request, ApiVersion::V2)); + .push((request_id, ApiVersion::V2)); } // Submit the interrupt; we'll respond upon TurnAborted. @@ -5196,7 +5062,7 @@ impl CodexMessageProcessor { async fn add_conversation_listener( &mut self, - request_id: ConnectionRequestId, + request_id: RequestId, params: AddConversationListenerParams, ) { let AddConversationListenerParams { @@ -5219,7 +5085,7 @@ impl CodexMessageProcessor { async fn remove_thread_listener( &mut self, - request_id: ConnectionRequestId, + request_id: RequestId, params: RemoveConversationListenerParams, ) { let RemoveConversationListenerParams { subscription_id } = params; @@ -5349,7 +5215,7 @@ impl CodexMessageProcessor { Ok(subscription_id) } - async fn git_diff_to_origin(&self, request_id: ConnectionRequestId, cwd: PathBuf) { + async fn git_diff_to_origin(&self, request_id: RequestId, cwd: PathBuf) { let diff = git_diff_to_remote(&cwd).await; match diff { Some(value) => { @@ -5370,11 +5236,7 @@ impl CodexMessageProcessor { } } - async fn fuzzy_file_search( - &mut self, - request_id: ConnectionRequestId, - params: FuzzyFileSearchParams, - ) { + async fn fuzzy_file_search(&mut self, request_id: RequestId, params: FuzzyFileSearchParams) { let FuzzyFileSearchParams { query, roots, @@ -5414,7 +5276,7 @@ impl CodexMessageProcessor { self.outgoing.send_response(request_id, response).await; } - async fn upload_feedback(&self, request_id: ConnectionRequestId, params: FeedbackUploadParams) { + async fn upload_feedback(&self, request_id: RequestId, params: FeedbackUploadParams) { if !self.config.feedback_enabled { let error = JSONRPCErrorError { code: INVALID_REQUEST_ERROR_CODE, diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index ad049ad305..1b940c70d8 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -8,24 +8,14 @@ use codex_core::config::ConfigBuilder; use codex_core::config_loader::CloudRequirementsLoader; use codex_core::config_loader::ConfigLayerStackOrdering; use codex_core::config_loader::LoaderOverrides; -use std::collections::HashMap; use std::io::ErrorKind; use std::io::Result as IoResult; use std::path::PathBuf; -use std::sync::Arc; use crate::message_processor::MessageProcessor; use crate::message_processor::MessageProcessorArgs; -use crate::outgoing_message::ConnectionId; -use crate::outgoing_message::OutgoingEnvelope; +use crate::outgoing_message::OutgoingMessage; use crate::outgoing_message::OutgoingMessageSender; -use crate::transport::CHANNEL_CAPACITY; -use crate::transport::ConnectionState; -use crate::transport::TransportEvent; -use crate::transport::has_initialized_connections; -use crate::transport::route_outgoing_envelope; -use crate::transport::start_stdio_connection; -use crate::transport::start_websocket_acceptor; use codex_app_server_protocol::ConfigLayerSource; use codex_app_server_protocol::ConfigWarningNotification; use codex_app_server_protocol::JSONRPCMessage; @@ -36,9 +26,13 @@ use codex_core::check_execpolicy_for_warnings; use codex_core::config_loader::ConfigLoadError; use codex_core::config_loader::TextRange as CoreTextRange; use codex_feedback::CodexFeedback; +use tokio::io::AsyncBufReadExt; +use tokio::io::AsyncWriteExt; +use tokio::io::BufReader; +use tokio::io::{self}; use tokio::sync::mpsc; -use tokio::task::JoinHandle; use toml::Value as TomlValue; +use tracing::debug; use tracing::error; use tracing::info; use tracing::warn; @@ -57,9 +51,11 @@ mod fuzzy_file_search; mod message_processor; mod models; mod outgoing_message; -mod transport; -pub use crate::transport::AppServerTransport; +/// Size of the bounded channels used to communicate between tasks. The value +/// is a balance between throughput and memory usage – 128 messages should be +/// plenty for an interactive CLI. +const CHANNEL_CAPACITY: usize = 128; fn config_warning_from_error( summary: impl Into, @@ -177,39 +173,32 @@ pub async fn run_main( loader_overrides: LoaderOverrides, default_analytics_enabled: bool, ) -> IoResult<()> { - run_main_with_transport( - codex_linux_sandbox_exe, - cli_config_overrides, - loader_overrides, - default_analytics_enabled, - AppServerTransport::Stdio, - ) - .await -} + // Set up channels. + let (incoming_tx, mut incoming_rx) = mpsc::channel::(CHANNEL_CAPACITY); + let (outgoing_tx, mut outgoing_rx) = mpsc::channel::(CHANNEL_CAPACITY); -pub async fn run_main_with_transport( - codex_linux_sandbox_exe: Option, - cli_config_overrides: CliConfigOverrides, - loader_overrides: LoaderOverrides, - default_analytics_enabled: bool, - transport: AppServerTransport, -) -> IoResult<()> { - let (transport_event_tx, mut transport_event_rx) = - mpsc::channel::(CHANNEL_CAPACITY); - let (outgoing_tx, mut outgoing_rx) = mpsc::channel::(CHANNEL_CAPACITY); + // Task: read from stdin, push to `incoming_tx`. + let stdin_reader_handle = tokio::spawn({ + async move { + let stdin = io::stdin(); + let reader = BufReader::new(stdin); + let mut lines = reader.lines(); - let mut stdio_handles = Vec::>::new(); - let mut websocket_accept_handle = None; - match transport { - AppServerTransport::Stdio => { - start_stdio_connection(transport_event_tx.clone(), &mut stdio_handles).await?; + while let Some(line) = lines.next_line().await.unwrap_or_default() { + match serde_json::from_str::(&line) { + Ok(msg) => { + if incoming_tx.send(msg).await.is_err() { + // Receiver gone – nothing left to do. + break; + } + } + Err(e) => error!("Failed to deserialize JSONRPCMessage: {e}"), + } + } + + debug!("stdin reader finished (EOF)"); } - AppServerTransport::WebSocket { bind_address } => { - websocket_accept_handle = - Some(start_websocket_acceptor(bind_address, transport_event_tx.clone()).await?); - } - } - let shutdown_when_no_connections = matches!(transport, AppServerTransport::Stdio); + }); // Parse CLI overrides once and derive the base Config eagerly so later // components do not need to work with raw TOML values. @@ -336,14 +325,15 @@ pub async fn run_main_with_transport( } } + // Task: process incoming messages. let processor_handle = tokio::spawn({ - let outgoing_message_sender = Arc::new(OutgoingMessageSender::new(outgoing_tx)); + let outgoing_message_sender = OutgoingMessageSender::new(outgoing_tx); let cli_overrides: Vec<(String, TomlValue)> = cli_kv_overrides.clone(); let loader_overrides = loader_overrides_for_config_api; let mut processor = MessageProcessor::new(MessageProcessorArgs { outgoing: outgoing_message_sender, codex_linux_sandbox_exe, - config: Arc::new(config), + config: std::sync::Arc::new(config), cli_overrides, loader_overrides, cloud_requirements: cloud_requirements.clone(), @@ -351,65 +341,25 @@ pub async fn run_main_with_transport( config_warnings, }); let mut thread_created_rx = processor.thread_created_receiver(); - let mut connections = HashMap::::new(); async move { let mut listen_for_threads = true; loop { tokio::select! { - event = transport_event_rx.recv() => { - let Some(event) = event else { + msg = incoming_rx.recv() => { + let Some(msg) = msg else { break; }; - match event { - TransportEvent::ConnectionOpened { connection_id, writer } => { - connections.insert(connection_id, ConnectionState::new(writer)); - } - TransportEvent::ConnectionClosed { connection_id } => { - connections.remove(&connection_id); - if shutdown_when_no_connections && connections.is_empty() { - break; - } - } - TransportEvent::IncomingMessage { connection_id, message } => { - match message { - JSONRPCMessage::Request(request) => { - let Some(connection_state) = connections.get_mut(&connection_id) else { - warn!("dropping request from unknown connection: {:?}", connection_id); - continue; - }; - processor - .process_request( - connection_id, - request, - &mut connection_state.session, - ) - .await; - } - JSONRPCMessage::Response(response) => { - processor.process_response(response).await; - } - JSONRPCMessage::Notification(notification) => { - processor.process_notification(notification).await; - } - JSONRPCMessage::Error(err) => { - processor.process_error(err).await; - } - } - } + match msg { + JSONRPCMessage::Request(r) => processor.process_request(r).await, + JSONRPCMessage::Response(r) => processor.process_response(r).await, + JSONRPCMessage::Notification(n) => processor.process_notification(n).await, + JSONRPCMessage::Error(e) => processor.process_error(e).await, } } - envelope = outgoing_rx.recv() => { - let Some(envelope) = envelope else { - break; - }; - route_outgoing_envelope(&mut connections, envelope).await; - } created = thread_created_rx.recv(), if listen_for_threads => { match created { Ok(thread_id) => { - if has_initialized_connections(&connections) { - processor.try_attach_thread_listener(thread_id).await; - } + processor.try_attach_thread_listener(thread_id).await; } Err(tokio::sync::broadcast::error::RecvError::Lagged(_)) => { // TODO(jif) handle lag. @@ -430,17 +380,33 @@ pub async fn run_main_with_transport( } }); - drop(transport_event_tx); + // Task: write outgoing messages to stdout. + let stdout_writer_handle = tokio::spawn(async move { + let mut stdout = io::stdout(); + while let Some(outgoing_message) = outgoing_rx.recv().await { + let Ok(value) = serde_json::to_value(outgoing_message) else { + error!("Failed to convert OutgoingMessage to JSON value"); + continue; + }; + match serde_json::to_string(&value) { + Ok(mut json) => { + json.push('\n'); + if let Err(e) = stdout.write_all(json.as_bytes()).await { + error!("Failed to write to stdout: {e}"); + break; + } + } + Err(e) => error!("Failed to serialize JSONRPCMessage: {e}"), + } + } - let _ = processor_handle.await; + info!("stdout writer exited (channel closed)"); + }); - if let Some(handle) = websocket_accept_handle { - handle.abort(); - } - - for handle in stdio_handles { - let _ = handle.await; - } + // Wait for all tasks to finish. The typical exit path is the stdin reader + // hitting EOF which, once it drops `incoming_tx`, propagates shutdown to + // the processor and then to the stdout task. + let _ = tokio::join!(stdin_reader_handle, processor_handle, stdout_writer_handle); Ok(()) } diff --git a/codex-rs/app-server/src/main.rs b/codex-rs/app-server/src/main.rs index 40dec1dc80..71d6dc338c 100644 --- a/codex-rs/app-server/src/main.rs +++ b/codex-rs/app-server/src/main.rs @@ -1,6 +1,4 @@ -use clap::Parser; -use codex_app_server::AppServerTransport; -use codex_app_server::run_main_with_transport; +use codex_app_server::run_main; use codex_arg0::arg0_dispatch_or_else; use codex_common::CliConfigOverrides; use codex_core::config_loader::LoaderOverrides; @@ -10,34 +8,19 @@ use std::path::PathBuf; // managed config file without writing to /etc. const MANAGED_CONFIG_PATH_ENV_VAR: &str = "CODEX_APP_SERVER_MANAGED_CONFIG_PATH"; -#[derive(Debug, Parser)] -struct AppServerArgs { - /// Transport endpoint URL. Supported values: `stdio://` (default), - /// `ws://IP:PORT`. - #[arg( - long = "listen", - value_name = "URL", - default_value = AppServerTransport::DEFAULT_LISTEN_URL - )] - listen: AppServerTransport, -} - fn main() -> anyhow::Result<()> { arg0_dispatch_or_else(|codex_linux_sandbox_exe| async move { - let args = AppServerArgs::parse(); let managed_config_path = managed_config_path_from_debug_env(); let loader_overrides = LoaderOverrides { managed_config_path, ..Default::default() }; - let transport = args.listen; - run_main_with_transport( + run_main( codex_linux_sandbox_exe, CliConfigOverrides::default(), loader_overrides, false, - transport, ) .await?; Ok(()) diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 26da44df31..368d24e727 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -1,13 +1,13 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::RwLock; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; use crate::codex_message_processor::CodexMessageProcessor; use crate::codex_message_processor::CodexMessageProcessorArgs; use crate::config_api::ConfigApi; use crate::error_code::INVALID_REQUEST_ERROR_CODE; -use crate::outgoing_message::ConnectionId; -use crate::outgoing_message::ConnectionRequestId; use crate::outgoing_message::OutgoingMessageSender; use async_trait::async_trait; use codex_app_server_protocol::ChatgptAuthTokensRefreshParams; @@ -26,6 +26,7 @@ use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::JSONRPCNotification; use codex_app_server_protocol::JSONRPCRequest; use codex_app_server_protocol::JSONRPCResponse; +use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ServerNotification; use codex_app_server_protocol::ServerRequestPayload; use codex_app_server_protocol::experimental_required_message; @@ -111,17 +112,13 @@ pub(crate) struct MessageProcessor { codex_message_processor: CodexMessageProcessor, config_api: ConfigApi, config: Arc, - config_warnings: Arc>, -} - -#[derive(Debug, Default)] -pub(crate) struct ConnectionSessionState { - pub(crate) initialized: bool, - experimental_api_enabled: bool, + initialized: bool, + experimental_api_enabled: Arc, + config_warnings: Vec, } pub(crate) struct MessageProcessorArgs { - pub(crate) outgoing: Arc, + pub(crate) outgoing: OutgoingMessageSender, pub(crate) codex_linux_sandbox_exe: Option, pub(crate) config: Arc, pub(crate) cli_overrides: Vec<(String, TomlValue)>, @@ -145,6 +142,8 @@ impl MessageProcessor { feedback, config_warnings, } = args; + let outgoing = Arc::new(outgoing); + let experimental_api_enabled = Arc::new(AtomicBool::new(false)); let auth_manager = AuthManager::shared( config.codex_home.clone(), false, @@ -182,20 +181,14 @@ impl MessageProcessor { codex_message_processor, config_api, config, - config_warnings: Arc::new(config_warnings), + initialized: false, + experimental_api_enabled, + config_warnings, } } - pub(crate) async fn process_request( - &mut self, - connection_id: ConnectionId, - request: JSONRPCRequest, - session: &mut ConnectionSessionState, - ) { - let request_id = ConnectionRequestId { - connection_id, - request_id: request.id.clone(), - }; + pub(crate) async fn process_request(&mut self, request: JSONRPCRequest) { + let request_id = request.id.clone(); let request_json = match serde_json::to_value(&request) { Ok(request_json) => request_json, Err(err) => { @@ -226,11 +219,7 @@ impl MessageProcessor { // Handle Initialize internally so CodexMessageProcessor does not have to concern // itself with the `initialized` bool. ClientRequest::Initialize { request_id, params } => { - let request_id = ConnectionRequestId { - connection_id, - request_id, - }; - if session.initialized { + if self.initialized { let error = JSONRPCErrorError { code: INVALID_REQUEST_ERROR_CODE, message: "Already initialized".to_string(), @@ -239,16 +228,21 @@ impl MessageProcessor { self.outgoing.send_error(request_id, error).await; return; } else { - // TODO(maxj): Revisit capability scoping for `experimental_api_enabled`. - // Current behavior is per-connection. Reviewer feedback notes this can - // create odd cross-client behavior (for example dynamic tool calls on a - // shared thread when another connected client did not opt into - // experimental API). Proposed direction is instance-global first-write-wins - // with initialize-time mismatch rejection. - session.experimental_api_enabled = params - .capabilities - .as_ref() - .is_some_and(|cap| cap.experimental_api); + let (experimental_api_enabled, opt_out_notification_methods) = + match params.capabilities { + Some(capabilities) => ( + capabilities.experimental_api, + capabilities + .opt_out_notification_methods + .unwrap_or_default(), + ), + None => (false, Vec::new()), + }; + self.experimental_api_enabled + .store(experimental_api_enabled, Ordering::Relaxed); + self.outgoing + .set_opted_out_notification_methods(opt_out_notification_methods) + .await; let ClientInfo { name, title: _title, @@ -264,7 +258,7 @@ impl MessageProcessor { ), data: None, }; - self.outgoing.send_error(request_id.clone(), error).await; + self.outgoing.send_error(request_id, error).await; return; } SetOriginatorError::AlreadyInitialized => { @@ -285,20 +279,22 @@ impl MessageProcessor { let response = InitializeResponse { user_agent }; self.outgoing.send_response(request_id, response).await; - session.initialized = true; - for notification in self.config_warnings.iter().cloned() { - self.outgoing - .send_server_notification(ServerNotification::ConfigWarning( - notification, - )) - .await; + self.initialized = true; + if !self.config_warnings.is_empty() { + for notification in self.config_warnings.drain(..) { + self.outgoing + .send_server_notification(ServerNotification::ConfigWarning( + notification, + )) + .await; + } } return; } } _ => { - if !session.initialized { + if !self.initialized { let error = JSONRPCErrorError { code: INVALID_REQUEST_ERROR_CODE, message: "Not initialized".to_string(), @@ -311,7 +307,7 @@ impl MessageProcessor { } if let Some(reason) = codex_request.experimental_reason() - && !session.experimental_api_enabled + && !self.experimental_api_enabled.load(Ordering::Relaxed) { let error = JSONRPCErrorError { code: INVALID_REQUEST_ERROR_CODE, @@ -324,49 +320,22 @@ impl MessageProcessor { match codex_request { ClientRequest::ConfigRead { request_id, params } => { - self.handle_config_read( - ConnectionRequestId { - connection_id, - request_id, - }, - params, - ) - .await; + self.handle_config_read(request_id, params).await; } ClientRequest::ConfigValueWrite { request_id, params } => { - self.handle_config_value_write( - ConnectionRequestId { - connection_id, - request_id, - }, - params, - ) - .await; + self.handle_config_value_write(request_id, params).await; } ClientRequest::ConfigBatchWrite { request_id, params } => { - self.handle_config_batch_write( - ConnectionRequestId { - connection_id, - request_id, - }, - params, - ) - .await; + self.handle_config_batch_write(request_id, params).await; } ClientRequest::ConfigRequirementsRead { request_id, params: _, } => { - self.handle_config_requirements_read(ConnectionRequestId { - connection_id, - request_id, - }) - .await; + self.handle_config_requirements_read(request_id).await; } other => { - self.codex_message_processor - .process_request(connection_id, other) - .await; + self.codex_message_processor.process_request(other).await; } } } @@ -382,6 +351,9 @@ impl MessageProcessor { } pub(crate) async fn try_attach_thread_listener(&mut self, thread_id: ThreadId) { + if !self.initialized { + return; + } self.codex_message_processor .try_attach_thread_listener(thread_id) .await; @@ -400,7 +372,7 @@ impl MessageProcessor { self.outgoing.notify_client_error(err.id, err.error).await; } - async fn handle_config_read(&self, request_id: ConnectionRequestId, params: ConfigReadParams) { + async fn handle_config_read(&self, request_id: RequestId, params: ConfigReadParams) { match self.config_api.read(params).await { Ok(response) => self.outgoing.send_response(request_id, response).await, Err(error) => self.outgoing.send_error(request_id, error).await, @@ -409,7 +381,7 @@ impl MessageProcessor { async fn handle_config_value_write( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: ConfigValueWriteParams, ) { match self.config_api.write_value(params).await { @@ -420,7 +392,7 @@ impl MessageProcessor { async fn handle_config_batch_write( &self, - request_id: ConnectionRequestId, + request_id: RequestId, params: ConfigBatchWriteParams, ) { match self.config_api.batch_write(params).await { @@ -429,7 +401,7 @@ impl MessageProcessor { } } - async fn handle_config_requirements_read(&self, request_id: ConnectionRequestId) { + async fn handle_config_requirements_read(&self, request_id: RequestId) { match self.config_api.config_requirements_read().await { Ok(response) => self.outgoing.send_response(request_id, response).await, Err(error) => self.outgoing.send_error(request_id, error).await, diff --git a/codex-rs/app-server/src/outgoing_message.rs b/codex-rs/app-server/src/outgoing_message.rs index a5219dc2dc..b64bd5bce9 100644 --- a/codex-rs/app-server/src/outgoing_message.rs +++ b/codex-rs/app-server/src/outgoing_message.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::collections::HashSet; use std::sync::atomic::AtomicI64; use std::sync::atomic::Ordering; @@ -19,44 +20,35 @@ use crate::error_code::INTERNAL_ERROR_CODE; #[cfg(test)] use codex_protocol::account::PlanType; -/// Stable identifier for a transport connection. -#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] -pub(crate) struct ConnectionId(pub(crate) u64); - -/// Stable identifier for a client request scoped to a transport connection. -#[derive(Clone, Debug, Eq, Hash, PartialEq)] -pub(crate) struct ConnectionRequestId { - pub(crate) connection_id: ConnectionId, - pub(crate) request_id: RequestId, -} - -#[derive(Debug, Clone)] -pub(crate) enum OutgoingEnvelope { - ToConnection { - connection_id: ConnectionId, - message: OutgoingMessage, - }, - Broadcast { - message: OutgoingMessage, - }, -} - /// Sends messages to the client and manages request callbacks. pub(crate) struct OutgoingMessageSender { - next_server_request_id: AtomicI64, - sender: mpsc::Sender, + next_request_id: AtomicI64, + sender: mpsc::Sender, request_id_to_callback: Mutex>>, + opted_out_notification_methods: Mutex>, } impl OutgoingMessageSender { - pub(crate) fn new(sender: mpsc::Sender) -> Self { + pub(crate) fn new(sender: mpsc::Sender) -> Self { Self { - next_server_request_id: AtomicI64::new(0), + next_request_id: AtomicI64::new(0), sender, request_id_to_callback: Mutex::new(HashMap::new()), + opted_out_notification_methods: Mutex::new(HashSet::new()), } } + pub(crate) async fn set_opted_out_notification_methods(&self, methods: Vec) { + let mut opted_out = self.opted_out_notification_methods.lock().await; + opted_out.clear(); + opted_out.extend(methods); + } + + async fn should_skip_notification(&self, method: &str) -> bool { + let opted_out = self.opted_out_notification_methods.lock().await; + opted_out.contains(method) + } + pub(crate) async fn send_request( &self, request: ServerRequestPayload, @@ -69,7 +61,7 @@ impl OutgoingMessageSender { &self, request: ServerRequestPayload, ) -> (RequestId, oneshot::Receiver) { - let id = RequestId::Integer(self.next_server_request_id.fetch_add(1, Ordering::Relaxed)); + let id = RequestId::Integer(self.next_request_id.fetch_add(1, Ordering::Relaxed)); let outgoing_message_id = id.clone(); let (tx_approve, rx_approve) = oneshot::channel(); { @@ -79,13 +71,7 @@ impl OutgoingMessageSender { let outgoing_message = OutgoingMessage::Request(request.request_with_id(outgoing_message_id.clone())); - if let Err(err) = self - .sender - .send(OutgoingEnvelope::Broadcast { - message: outgoing_message, - }) - .await - { + if let Err(err) = self.sender.send(outgoing_message).await { warn!("failed to send request {outgoing_message_id:?} to client: {err:?}"); let mut request_id_to_callback = self.request_id_to_callback.lock().await; request_id_to_callback.remove(&outgoing_message_id); @@ -135,31 +121,17 @@ impl OutgoingMessageSender { entry.is_some() } - pub(crate) async fn send_response( - &self, - request_id: ConnectionRequestId, - response: T, - ) { + pub(crate) async fn send_response(&self, id: RequestId, response: T) { match serde_json::to_value(response) { Ok(result) => { - let outgoing_message = OutgoingMessage::Response(OutgoingResponse { - id: request_id.request_id, - result, - }); - if let Err(err) = self - .sender - .send(OutgoingEnvelope::ToConnection { - connection_id: request_id.connection_id, - message: outgoing_message, - }) - .await - { + let outgoing_message = OutgoingMessage::Response(OutgoingResponse { id, result }); + if let Err(err) = self.sender.send(outgoing_message).await { warn!("failed to send response to client: {err:?}"); } } Err(err) => { self.send_error( - request_id, + id, JSONRPCErrorError { code: INTERNAL_ERROR_CODE, message: format!("failed to serialize response: {err}"), @@ -172,11 +144,13 @@ impl OutgoingMessageSender { } pub(crate) async fn send_server_notification(&self, notification: ServerNotification) { + let method = notification.to_string(); + if self.should_skip_notification(&method).await { + return; + } if let Err(err) = self .sender - .send(OutgoingEnvelope::Broadcast { - message: OutgoingMessage::AppServerNotification(notification), - }) + .send(OutgoingMessage::AppServerNotification(notification)) .await { warn!("failed to send server notification to client: {err:?}"); @@ -186,35 +160,21 @@ impl OutgoingMessageSender { /// All notifications should be migrated to [`ServerNotification`] and /// [`OutgoingMessage::Notification`] should be removed. pub(crate) async fn send_notification(&self, notification: OutgoingNotification) { - let outgoing_message = OutgoingMessage::Notification(notification); - if let Err(err) = self - .sender - .send(OutgoingEnvelope::Broadcast { - message: outgoing_message, - }) + if self + .should_skip_notification(notification.method.as_str()) .await { + return; + } + let outgoing_message = OutgoingMessage::Notification(notification); + if let Err(err) = self.sender.send(outgoing_message).await { warn!("failed to send notification to client: {err:?}"); } } - pub(crate) async fn send_error( - &self, - request_id: ConnectionRequestId, - error: JSONRPCErrorError, - ) { - let outgoing_message = OutgoingMessage::Error(OutgoingError { - id: request_id.request_id, - error, - }); - if let Err(err) = self - .sender - .send(OutgoingEnvelope::ToConnection { - connection_id: request_id.connection_id, - message: outgoing_message, - }) - .await - { + pub(crate) async fn send_error(&self, id: RequestId, error: JSONRPCErrorError) { + let outgoing_message = OutgoingMessage::Error(OutgoingError { id, error }); + if let Err(err) = self.sender.send(outgoing_message).await { warn!("failed to send error to client: {err:?}"); } } @@ -254,8 +214,6 @@ pub(crate) struct OutgoingError { #[cfg(test)] mod tests { - use std::time::Duration; - use codex_app_server_protocol::AccountLoginCompletedNotification; use codex_app_server_protocol::AccountRateLimitsUpdatedNotification; use codex_app_server_protocol::AccountUpdatedNotification; @@ -266,7 +224,6 @@ mod tests { use codex_app_server_protocol::RateLimitWindow; use pretty_assertions::assert_eq; use serde_json::json; - use tokio::time::timeout; use uuid::Uuid; use super::*; @@ -403,75 +360,4 @@ mod tests { "ensure the notification serializes correctly" ); } - - #[tokio::test] - async fn send_response_routes_to_target_connection() { - let (tx, mut rx) = mpsc::channel::(4); - let outgoing = OutgoingMessageSender::new(tx); - let request_id = ConnectionRequestId { - connection_id: ConnectionId(42), - request_id: RequestId::Integer(7), - }; - - outgoing - .send_response(request_id.clone(), json!({ "ok": true })) - .await; - - let envelope = timeout(Duration::from_secs(1), rx.recv()) - .await - .expect("should receive envelope before timeout") - .expect("channel should contain one message"); - - match envelope { - OutgoingEnvelope::ToConnection { - connection_id, - message, - } => { - assert_eq!(connection_id, ConnectionId(42)); - let OutgoingMessage::Response(response) = message else { - panic!("expected response message"); - }; - assert_eq!(response.id, request_id.request_id); - assert_eq!(response.result, json!({ "ok": true })); - } - other => panic!("expected targeted response envelope, got: {other:?}"), - } - } - - #[tokio::test] - async fn send_error_routes_to_target_connection() { - let (tx, mut rx) = mpsc::channel::(4); - let outgoing = OutgoingMessageSender::new(tx); - let request_id = ConnectionRequestId { - connection_id: ConnectionId(9), - request_id: RequestId::Integer(3), - }; - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: "boom".to_string(), - data: None, - }; - - outgoing.send_error(request_id.clone(), error.clone()).await; - - let envelope = timeout(Duration::from_secs(1), rx.recv()) - .await - .expect("should receive envelope before timeout") - .expect("channel should contain one message"); - - match envelope { - OutgoingEnvelope::ToConnection { - connection_id, - message, - } => { - assert_eq!(connection_id, ConnectionId(9)); - let OutgoingMessage::Error(outgoing_error) = message else { - panic!("expected error message"); - }; - assert_eq!(outgoing_error.id, RequestId::Integer(3)); - assert_eq!(outgoing_error.error, error); - } - other => panic!("expected targeted error envelope, got: {other:?}"), - } - } } diff --git a/codex-rs/app-server/src/transport.rs b/codex-rs/app-server/src/transport.rs deleted file mode 100644 index 39fd13212c..0000000000 --- a/codex-rs/app-server/src/transport.rs +++ /dev/null @@ -1,459 +0,0 @@ -use crate::message_processor::ConnectionSessionState; -use crate::outgoing_message::ConnectionId; -use crate::outgoing_message::OutgoingEnvelope; -use crate::outgoing_message::OutgoingMessage; -use codex_app_server_protocol::JSONRPCMessage; -use futures::SinkExt; -use futures::StreamExt; -use owo_colors::OwoColorize; -use owo_colors::Stream; -use owo_colors::Style; -use std::collections::HashMap; -use std::io::ErrorKind; -use std::io::Result as IoResult; -use std::net::SocketAddr; -use std::str::FromStr; -use std::sync::Arc; -use std::sync::atomic::AtomicU64; -use std::sync::atomic::Ordering; -use tokio::io::AsyncBufReadExt; -use tokio::io::AsyncWriteExt; -use tokio::io::BufReader; -use tokio::io::{self}; -use tokio::net::TcpListener; -use tokio::net::TcpStream; -use tokio::sync::mpsc; -use tokio::task::JoinHandle; -use tokio_tungstenite::accept_async; -use tokio_tungstenite::tungstenite::Message as WebSocketMessage; -use tracing::debug; -use tracing::error; -use tracing::info; -use tracing::warn; - -/// Size of the bounded channels used to communicate between tasks. The value -/// is a balance between throughput and memory usage - 128 messages should be -/// plenty for an interactive CLI. -pub(crate) const CHANNEL_CAPACITY: usize = 128; - -fn colorize(text: &str, style: Style) -> String { - text.if_supports_color(Stream::Stderr, |value| value.style(style)) - .to_string() -} - -#[allow(clippy::print_stderr)] -fn print_websocket_startup_banner(addr: SocketAddr) { - let title = colorize("codex app-server (WebSockets)", Style::new().bold().cyan()); - let listening_label = colorize("listening on:", Style::new().dimmed()); - let listen_url = colorize(&format!("ws://{addr}"), Style::new().green()); - let note_label = colorize("note:", Style::new().dimmed()); - eprintln!("{title}"); - eprintln!(" {listening_label} {listen_url}"); - if addr.ip().is_loopback() { - eprintln!( - " {note_label} binds localhost only (use SSH port-forwarding for remote access)" - ); - } else { - eprintln!( - " {note_label} this is a raw WS server; consider running behind TLS/auth for real remote use" - ); - } -} - -#[allow(clippy::print_stderr)] -fn print_websocket_connection(peer_addr: SocketAddr) { - let connected_label = colorize("websocket client connected from", Style::new().dimmed()); - eprintln!("{connected_label} {peer_addr}"); -} - -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub enum AppServerTransport { - Stdio, - WebSocket { bind_address: SocketAddr }, -} - -#[derive(Debug, Clone, Eq, PartialEq)] -pub enum AppServerTransportParseError { - UnsupportedListenUrl(String), - InvalidWebSocketListenUrl(String), -} - -impl std::fmt::Display for AppServerTransportParseError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - AppServerTransportParseError::UnsupportedListenUrl(listen_url) => write!( - f, - "unsupported --listen URL `{listen_url}`; expected `stdio://` or `ws://IP:PORT`" - ), - AppServerTransportParseError::InvalidWebSocketListenUrl(listen_url) => write!( - f, - "invalid websocket --listen URL `{listen_url}`; expected `ws://IP:PORT`" - ), - } - } -} - -impl std::error::Error for AppServerTransportParseError {} - -impl AppServerTransport { - pub const DEFAULT_LISTEN_URL: &'static str = "stdio://"; - - pub fn from_listen_url(listen_url: &str) -> Result { - if listen_url == Self::DEFAULT_LISTEN_URL { - return Ok(Self::Stdio); - } - - if let Some(socket_addr) = listen_url.strip_prefix("ws://") { - let bind_address = socket_addr.parse::().map_err(|_| { - AppServerTransportParseError::InvalidWebSocketListenUrl(listen_url.to_string()) - })?; - return Ok(Self::WebSocket { bind_address }); - } - - Err(AppServerTransportParseError::UnsupportedListenUrl( - listen_url.to_string(), - )) - } -} - -impl FromStr for AppServerTransport { - type Err = AppServerTransportParseError; - - fn from_str(s: &str) -> Result { - Self::from_listen_url(s) - } -} - -#[derive(Debug)] -pub(crate) enum TransportEvent { - ConnectionOpened { - connection_id: ConnectionId, - writer: mpsc::Sender, - }, - ConnectionClosed { - connection_id: ConnectionId, - }, - IncomingMessage { - connection_id: ConnectionId, - message: JSONRPCMessage, - }, -} - -pub(crate) struct ConnectionState { - pub(crate) writer: mpsc::Sender, - pub(crate) session: ConnectionSessionState, -} - -impl ConnectionState { - pub(crate) fn new(writer: mpsc::Sender) -> Self { - Self { - writer, - session: ConnectionSessionState::default(), - } - } -} - -pub(crate) async fn start_stdio_connection( - transport_event_tx: mpsc::Sender, - stdio_handles: &mut Vec>, -) -> IoResult<()> { - let connection_id = ConnectionId(0); - let (writer_tx, mut writer_rx) = mpsc::channel::(CHANNEL_CAPACITY); - transport_event_tx - .send(TransportEvent::ConnectionOpened { - connection_id, - writer: writer_tx, - }) - .await - .map_err(|_| std::io::Error::new(ErrorKind::BrokenPipe, "processor unavailable"))?; - - let transport_event_tx_for_reader = transport_event_tx.clone(); - stdio_handles.push(tokio::spawn(async move { - let stdin = io::stdin(); - let reader = BufReader::new(stdin); - let mut lines = reader.lines(); - - loop { - match lines.next_line().await { - Ok(Some(line)) => { - if !forward_incoming_message( - &transport_event_tx_for_reader, - connection_id, - &line, - ) - .await - { - break; - } - } - Ok(None) => break, - Err(err) => { - error!("Failed reading stdin: {err}"); - break; - } - } - } - - let _ = transport_event_tx_for_reader - .send(TransportEvent::ConnectionClosed { connection_id }) - .await; - debug!("stdin reader finished (EOF)"); - })); - - stdio_handles.push(tokio::spawn(async move { - let mut stdout = io::stdout(); - while let Some(outgoing_message) = writer_rx.recv().await { - let Some(mut json) = serialize_outgoing_message(outgoing_message) else { - continue; - }; - json.push('\n'); - if let Err(err) = stdout.write_all(json.as_bytes()).await { - error!("Failed to write to stdout: {err}"); - break; - } - } - info!("stdout writer exited (channel closed)"); - })); - - Ok(()) -} - -pub(crate) async fn start_websocket_acceptor( - bind_address: SocketAddr, - transport_event_tx: mpsc::Sender, -) -> IoResult> { - let listener = TcpListener::bind(bind_address).await?; - let local_addr = listener.local_addr()?; - print_websocket_startup_banner(local_addr); - info!("app-server websocket listening on ws://{local_addr}"); - - let connection_counter = Arc::new(AtomicU64::new(1)); - Ok(tokio::spawn(async move { - loop { - match listener.accept().await { - Ok((stream, peer_addr)) => { - print_websocket_connection(peer_addr); - let connection_id = - ConnectionId(connection_counter.fetch_add(1, Ordering::Relaxed)); - let transport_event_tx_for_connection = transport_event_tx.clone(); - tokio::spawn(async move { - run_websocket_connection( - connection_id, - stream, - transport_event_tx_for_connection, - ) - .await; - }); - } - Err(err) => { - error!("failed to accept websocket connection: {err}"); - } - } - } - })) -} - -async fn run_websocket_connection( - connection_id: ConnectionId, - stream: TcpStream, - transport_event_tx: mpsc::Sender, -) { - let websocket_stream = match accept_async(stream).await { - Ok(stream) => stream, - Err(err) => { - warn!("failed to complete websocket handshake: {err}"); - return; - } - }; - - let (writer_tx, mut writer_rx) = mpsc::channel::(CHANNEL_CAPACITY); - if transport_event_tx - .send(TransportEvent::ConnectionOpened { - connection_id, - writer: writer_tx, - }) - .await - .is_err() - { - return; - } - - let (mut websocket_writer, mut websocket_reader) = websocket_stream.split(); - loop { - tokio::select! { - outgoing_message = writer_rx.recv() => { - let Some(outgoing_message) = outgoing_message else { - break; - }; - let Some(json) = serialize_outgoing_message(outgoing_message) else { - continue; - }; - if websocket_writer.send(WebSocketMessage::Text(json.into())).await.is_err() { - break; - } - } - incoming_message = websocket_reader.next() => { - match incoming_message { - Some(Ok(WebSocketMessage::Text(text))) => { - if !forward_incoming_message(&transport_event_tx, connection_id, &text).await { - break; - } - } - Some(Ok(WebSocketMessage::Ping(payload))) => { - if websocket_writer.send(WebSocketMessage::Pong(payload)).await.is_err() { - break; - } - } - Some(Ok(WebSocketMessage::Pong(_))) => {} - Some(Ok(WebSocketMessage::Close(_))) | None => break, - Some(Ok(WebSocketMessage::Binary(_))) => { - warn!("dropping unsupported binary websocket message"); - } - Some(Ok(WebSocketMessage::Frame(_))) => {} - Some(Err(err)) => { - warn!("websocket receive error: {err}"); - break; - } - } - } - } - } - - let _ = transport_event_tx - .send(TransportEvent::ConnectionClosed { connection_id }) - .await; -} - -async fn forward_incoming_message( - transport_event_tx: &mpsc::Sender, - connection_id: ConnectionId, - payload: &str, -) -> bool { - match serde_json::from_str::(payload) { - Ok(message) => transport_event_tx - .send(TransportEvent::IncomingMessage { - connection_id, - message, - }) - .await - .is_ok(), - Err(err) => { - error!("Failed to deserialize JSONRPCMessage: {err}"); - true - } - } -} - -fn serialize_outgoing_message(outgoing_message: OutgoingMessage) -> Option { - let value = match serde_json::to_value(outgoing_message) { - Ok(value) => value, - Err(err) => { - error!("Failed to convert OutgoingMessage to JSON value: {err}"); - return None; - } - }; - match serde_json::to_string(&value) { - Ok(json) => Some(json), - Err(err) => { - error!("Failed to serialize JSONRPCMessage: {err}"); - None - } - } -} - -pub(crate) async fn route_outgoing_envelope( - connections: &mut HashMap, - envelope: OutgoingEnvelope, -) { - match envelope { - OutgoingEnvelope::ToConnection { - connection_id, - message, - } => { - let Some(connection_state) = connections.get(&connection_id) else { - warn!( - "dropping message for disconnected connection: {:?}", - connection_id - ); - return; - }; - if connection_state.writer.send(message).await.is_err() { - connections.remove(&connection_id); - } - } - OutgoingEnvelope::Broadcast { message } => { - let target_connections: Vec = connections - .iter() - .filter_map(|(connection_id, connection_state)| { - if connection_state.session.initialized { - Some(*connection_id) - } else { - None - } - }) - .collect(); - - for connection_id in target_connections { - let Some(connection_state) = connections.get(&connection_id) else { - continue; - }; - if connection_state.writer.send(message.clone()).await.is_err() { - connections.remove(&connection_id); - } - } - } - } -} - -pub(crate) fn has_initialized_connections( - connections: &HashMap, -) -> bool { - connections - .values() - .any(|connection| connection.session.initialized) -} - -#[cfg(test)] -mod tests { - use super::*; - use pretty_assertions::assert_eq; - - #[test] - fn app_server_transport_parses_stdio_listen_url() { - let transport = AppServerTransport::from_listen_url(AppServerTransport::DEFAULT_LISTEN_URL) - .expect("stdio listen URL should parse"); - assert_eq!(transport, AppServerTransport::Stdio); - } - - #[test] - fn app_server_transport_parses_websocket_listen_url() { - let transport = AppServerTransport::from_listen_url("ws://127.0.0.1:1234") - .expect("websocket listen URL should parse"); - assert_eq!( - transport, - AppServerTransport::WebSocket { - bind_address: "127.0.0.1:1234".parse().expect("valid socket address"), - } - ); - } - - #[test] - fn app_server_transport_rejects_invalid_websocket_listen_url() { - let err = AppServerTransport::from_listen_url("ws://localhost:1234") - .expect_err("hostname bind address should be rejected"); - assert_eq!( - err.to_string(), - "invalid websocket --listen URL `ws://localhost:1234`; expected `ws://IP:PORT`" - ); - } - - #[test] - fn app_server_transport_rejects_unsupported_listen_url() { - let err = AppServerTransport::from_listen_url("http://127.0.0.1:1234") - .expect_err("unsupported scheme should fail"); - assert_eq!( - err.to_string(), - "unsupported --listen URL `http://127.0.0.1:1234`; expected `stdio://` or `ws://IP:PORT`" - ); - } -} diff --git a/codex-rs/app-server/tests/common/mcp_process.rs b/codex-rs/app-server/tests/common/mcp_process.rs index c1d004d407..7f77d8fc92 100644 --- a/codex-rs/app-server/tests/common/mcp_process.rs +++ b/codex-rs/app-server/tests/common/mcp_process.rs @@ -174,6 +174,7 @@ impl McpProcess { client_info, Some(InitializeCapabilities { experimental_api: true, + opt_out_notification_methods: None, }), ) .await diff --git a/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs b/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs deleted file mode 100644 index ddd4326fc9..0000000000 --- a/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs +++ /dev/null @@ -1,263 +0,0 @@ -use anyhow::Context; -use anyhow::Result; -use anyhow::bail; -use app_test_support::create_mock_responses_server_sequence_unchecked; -use codex_app_server_protocol::ClientInfo; -use codex_app_server_protocol::InitializeParams; -use codex_app_server_protocol::JSONRPCError; -use codex_app_server_protocol::JSONRPCMessage; -use codex_app_server_protocol::JSONRPCRequest; -use codex_app_server_protocol::JSONRPCResponse; -use codex_app_server_protocol::RequestId; -use futures::SinkExt; -use futures::StreamExt; -use serde_json::json; -use std::net::SocketAddr; -use std::path::Path; -use std::process::Stdio; -use tempfile::TempDir; -use tokio::io::AsyncBufReadExt; -use tokio::process::Child; -use tokio::process::Command; -use tokio::time::Duration; -use tokio::time::Instant; -use tokio::time::sleep; -use tokio::time::timeout; -use tokio_tungstenite::MaybeTlsStream; -use tokio_tungstenite::WebSocketStream; -use tokio_tungstenite::connect_async; -use tokio_tungstenite::tungstenite::Message as WebSocketMessage; - -const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(5); - -type WsClient = WebSocketStream>; - -#[tokio::test] -async fn websocket_transport_routes_per_connection_handshake_and_responses() -> Result<()> { - let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; - let codex_home = TempDir::new()?; - create_config_toml(codex_home.path(), &server.uri(), "never")?; - - let bind_addr = reserve_local_addr()?; - let mut process = spawn_websocket_server(codex_home.path(), bind_addr).await?; - - let mut ws1 = connect_websocket(bind_addr).await?; - let mut ws2 = connect_websocket(bind_addr).await?; - - send_initialize_request(&mut ws1, 1, "ws_client_one").await?; - let first_init = read_response_for_id(&mut ws1, 1).await?; - assert_eq!(first_init.id, RequestId::Integer(1)); - - // Initialize responses are request-scoped and must not leak to other - // connections. - assert_no_message(&mut ws2, Duration::from_millis(250)).await?; - - send_config_read_request(&mut ws2, 2).await?; - let not_initialized = read_error_for_id(&mut ws2, 2).await?; - assert_eq!(not_initialized.error.message, "Not initialized"); - - send_initialize_request(&mut ws2, 3, "ws_client_two").await?; - let second_init = read_response_for_id(&mut ws2, 3).await?; - assert_eq!(second_init.id, RequestId::Integer(3)); - - // Same request-id on different connections must route independently. - send_config_read_request(&mut ws1, 77).await?; - send_config_read_request(&mut ws2, 77).await?; - let ws1_config = read_response_for_id(&mut ws1, 77).await?; - let ws2_config = read_response_for_id(&mut ws2, 77).await?; - - assert_eq!(ws1_config.id, RequestId::Integer(77)); - assert_eq!(ws2_config.id, RequestId::Integer(77)); - assert!(ws1_config.result.get("config").is_some()); - assert!(ws2_config.result.get("config").is_some()); - - process - .kill() - .await - .context("failed to stop websocket app-server process")?; - Ok(()) -} - -async fn spawn_websocket_server(codex_home: &Path, bind_addr: SocketAddr) -> Result { - let program = codex_utils_cargo_bin::cargo_bin("codex-app-server") - .context("should find app-server binary")?; - let mut cmd = Command::new(program); - cmd.arg("--listen") - .arg(format!("ws://{bind_addr}")) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::piped()) - .env("CODEX_HOME", codex_home) - .env("RUST_LOG", "debug"); - let mut process = cmd - .kill_on_drop(true) - .spawn() - .context("failed to spawn websocket app-server process")?; - - if let Some(stderr) = process.stderr.take() { - let mut stderr_reader = tokio::io::BufReader::new(stderr).lines(); - tokio::spawn(async move { - while let Ok(Some(line)) = stderr_reader.next_line().await { - eprintln!("[websocket app-server stderr] {line}"); - } - }); - } - - Ok(process) -} - -fn reserve_local_addr() -> Result { - let listener = std::net::TcpListener::bind("127.0.0.1:0")?; - let addr = listener.local_addr()?; - drop(listener); - Ok(addr) -} - -async fn connect_websocket(bind_addr: SocketAddr) -> Result { - let url = format!("ws://{bind_addr}"); - let deadline = Instant::now() + Duration::from_secs(10); - loop { - match connect_async(&url).await { - Ok((stream, _response)) => return Ok(stream), - Err(err) => { - if Instant::now() >= deadline { - bail!("failed to connect websocket to {url}: {err}"); - } - sleep(Duration::from_millis(50)).await; - } - } - } -} - -async fn send_initialize_request(stream: &mut WsClient, id: i64, client_name: &str) -> Result<()> { - let params = InitializeParams { - client_info: ClientInfo { - name: client_name.to_string(), - title: Some("WebSocket Test Client".to_string()), - version: "0.1.0".to_string(), - }, - capabilities: None, - }; - send_request( - stream, - "initialize", - id, - Some(serde_json::to_value(params)?), - ) - .await -} - -async fn send_config_read_request(stream: &mut WsClient, id: i64) -> Result<()> { - send_request( - stream, - "config/read", - id, - Some(json!({ "includeLayers": false })), - ) - .await -} - -async fn send_request( - stream: &mut WsClient, - method: &str, - id: i64, - params: Option, -) -> Result<()> { - let message = JSONRPCMessage::Request(JSONRPCRequest { - id: RequestId::Integer(id), - method: method.to_string(), - params, - }); - send_jsonrpc(stream, message).await -} - -async fn send_jsonrpc(stream: &mut WsClient, message: JSONRPCMessage) -> Result<()> { - let payload = serde_json::to_string(&message)?; - stream - .send(WebSocketMessage::Text(payload.into())) - .await - .context("failed to send websocket frame") -} - -async fn read_response_for_id(stream: &mut WsClient, id: i64) -> Result { - let target_id = RequestId::Integer(id); - loop { - let message = read_jsonrpc_message(stream).await?; - if let JSONRPCMessage::Response(response) = message - && response.id == target_id - { - return Ok(response); - } - } -} - -async fn read_error_for_id(stream: &mut WsClient, id: i64) -> Result { - let target_id = RequestId::Integer(id); - loop { - let message = read_jsonrpc_message(stream).await?; - if let JSONRPCMessage::Error(err) = message - && err.id == target_id - { - return Ok(err); - } - } -} - -async fn read_jsonrpc_message(stream: &mut WsClient) -> Result { - loop { - let frame = timeout(DEFAULT_READ_TIMEOUT, stream.next()) - .await - .context("timed out waiting for websocket frame")? - .context("websocket stream ended unexpectedly")? - .context("failed to read websocket frame")?; - - match frame { - WebSocketMessage::Text(text) => return Ok(serde_json::from_str(text.as_ref())?), - WebSocketMessage::Ping(payload) => { - stream.send(WebSocketMessage::Pong(payload)).await?; - } - WebSocketMessage::Pong(_) => {} - WebSocketMessage::Close(frame) => { - bail!("websocket closed unexpectedly: {frame:?}") - } - WebSocketMessage::Binary(_) => bail!("unexpected binary websocket frame"), - WebSocketMessage::Frame(_) => {} - } - } -} - -async fn assert_no_message(stream: &mut WsClient, wait_for: Duration) -> Result<()> { - match timeout(wait_for, stream.next()).await { - Ok(Some(Ok(frame))) => bail!("unexpected frame while waiting for silence: {frame:?}"), - Ok(Some(Err(err))) => bail!("unexpected websocket read error: {err}"), - Ok(None) => bail!("websocket closed unexpectedly while waiting for silence"), - Err(_) => Ok(()), - } -} - -fn create_config_toml( - codex_home: &Path, - server_uri: &str, - approval_policy: &str, -) -> std::io::Result<()> { - let config_toml = codex_home.join("config.toml"); - std::fs::write( - config_toml, - format!( - r#" -model = "mock-model" -approval_policy = "{approval_policy}" -sandbox_mode = "read-only" - -model_provider = "mock_provider" - -[model_providers.mock_provider] -name = "Mock provider for test" -base_url = "{server_uri}/v1" -wire_api = "responses" -request_max_retries = 0 -stream_max_retries = 0 -"# - ), - ) -} diff --git a/codex-rs/app-server/tests/suite/v2/experimental_api.rs b/codex-rs/app-server/tests/suite/v2/experimental_api.rs index 5116633a48..798d52abfd 100644 --- a/codex-rs/app-server/tests/suite/v2/experimental_api.rs +++ b/codex-rs/app-server/tests/suite/v2/experimental_api.rs @@ -30,6 +30,7 @@ async fn mock_experimental_method_requires_experimental_api_capability() -> Resu default_client_info(), Some(InitializeCapabilities { experimental_api: false, + opt_out_notification_methods: None, }), ) .await?; @@ -61,6 +62,7 @@ async fn thread_start_mock_field_requires_experimental_api_capability() -> Resul default_client_info(), Some(InitializeCapabilities { experimental_api: false, + opt_out_notification_methods: None, }), ) .await?; @@ -97,6 +99,7 @@ async fn thread_start_without_dynamic_tools_allows_without_experimental_api_capa default_client_info(), Some(InitializeCapabilities { experimental_api: false, + opt_out_notification_methods: None, }), ) .await?; diff --git a/codex-rs/app-server/tests/suite/v2/initialize.rs b/codex-rs/app-server/tests/suite/v2/initialize.rs index b31a68833f..2edc83a7f4 100644 --- a/codex-rs/app-server/tests/suite/v2/initialize.rs +++ b/codex-rs/app-server/tests/suite/v2/initialize.rs @@ -3,8 +3,12 @@ use app_test_support::McpProcess; use app_test_support::create_mock_responses_server_sequence_unchecked; use app_test_support::to_response; use codex_app_server_protocol::ClientInfo; +use codex_app_server_protocol::InitializeCapabilities; use codex_app_server_protocol::InitializeResponse; use codex_app_server_protocol::JSONRPCMessage; +use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::ThreadStartParams; +use codex_app_server_protocol::ThreadStartResponse; use pretty_assertions::assert_eq; use std::path::Path; use tempfile::TempDir; @@ -108,6 +112,72 @@ async fn initialize_rejects_invalid_client_name() -> Result<()> { Ok(()) } +#[tokio::test] +async fn initialize_opt_out_notification_methods_filters_notifications() -> Result<()> { + let responses = Vec::new(); + let server = create_mock_responses_server_sequence_unchecked(responses).await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri(), "never")?; + let mut mcp = McpProcess::new(codex_home.path()).await?; + + let message = timeout( + DEFAULT_READ_TIMEOUT, + mcp.initialize_with_capabilities( + ClientInfo { + name: "codex_vscode".to_string(), + title: Some("Codex VS Code Extension".to_string()), + version: "0.1.0".to_string(), + }, + Some(InitializeCapabilities { + experimental_api: true, + opt_out_notification_methods: Some(vec![ + "thread/started".to_string(), + "codex/event/session_configured".to_string(), + ]), + }), + ), + ) + .await??; + let JSONRPCMessage::Response(_) = message else { + anyhow::bail!("expected initialize response, got {message:?}"); + }; + + let request_id = mcp + .send_thread_start_request(ThreadStartParams::default()) + .await?; + let response = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let message = mcp.read_next_message().await?; + match message { + JSONRPCMessage::Response(response) + if response.id == RequestId::Integer(request_id) => + { + return Ok(response); + } + JSONRPCMessage::Notification(notification) + if notification.method == "thread/started" => + { + anyhow::bail!("thread/started should be filtered by optOutNotificationMethods"); + } + _ => {} + } + } + }) + .await??; + let _: ThreadStartResponse = to_response(response)?; + + let thread_started = timeout( + std::time::Duration::from_millis(500), + mcp.read_stream_until_notification_message("thread/started"), + ) + .await; + assert!( + thread_started.is_err(), + "thread/started should be filtered by optOutNotificationMethods" + ); + Ok(()) +} + // Helper to create a config.toml pointing at the mock model server. fn create_config_toml( codex_home: &Path, diff --git a/codex-rs/app-server/tests/suite/v2/mod.rs b/codex-rs/app-server/tests/suite/v2/mod.rs index 48622acddb..e9e19395be 100644 --- a/codex-rs/app-server/tests/suite/v2/mod.rs +++ b/codex-rs/app-server/tests/suite/v2/mod.rs @@ -4,7 +4,6 @@ mod app_list; mod collaboration_mode_list; mod compaction; mod config_rpc; -mod connection_handling_websocket; mod dynamic_tools; mod experimental_api; mod experimental_feature_list; diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index 2f919eee38..1250c055dd 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -5,6 +5,8 @@ use app_test_support::create_mock_responses_server_repeating_assistant; use app_test_support::create_mock_responses_server_sequence; use app_test_support::create_shell_command_sse_response; use app_test_support::to_response; +use codex_app_server_protocol::CommandExecutionApprovalDecision; +use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::ItemCompletedNotification; use codex_app_server_protocol::ItemStartedNotification; use codex_app_server_protocol::JSONRPCError; @@ -208,7 +210,9 @@ async fn review_start_exec_approval_item_id_matches_command_execution_item() -> mcp.send_response( request_id, - serde_json::json!({ "decision": codex_core::protocol::ReviewDecision::Approved }), + serde_json::to_value(CommandExecutionRequestApprovalResponse { + decision: CommandExecutionApprovalDecision::Accept, + })?, ) .await?; timeout( diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 1cc4fcdaa7..defc063eb6 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -306,15 +306,6 @@ struct AppServerCommand { #[command(subcommand)] subcommand: Option, - /// Transport endpoint URL. Supported values: `stdio://` (default), - /// `ws://IP:PORT`. - #[arg( - long = "listen", - value_name = "URL", - default_value = codex_app_server::AppServerTransport::DEFAULT_LISTEN_URL - )] - listen: codex_app_server::AppServerTransport, - /// Controls whether analytics are enabled by default. /// /// Analytics are disabled by default for app-server. Users have to explicitly opt in @@ -596,13 +587,11 @@ async fn cli_main(codex_linux_sandbox_exe: Option) -> anyhow::Result<() } Some(Subcommand::AppServer(app_server_cli)) => match app_server_cli.subcommand { None => { - let transport = app_server_cli.listen; - codex_app_server::run_main_with_transport( + codex_app_server::run_main( codex_linux_sandbox_exe, root_config_overrides, codex_core::config_loader::LoaderOverrides::default(), app_server_cli.analytics_default_enabled, - transport, ) .await?; } @@ -1339,10 +1328,6 @@ mod tests { fn app_server_analytics_default_disabled_without_flag() { let app_server = app_server_from_args(["codex", "app-server"].as_ref()); assert!(!app_server.analytics_default_enabled); - assert_eq!( - app_server.listen, - codex_app_server::AppServerTransport::Stdio - ); } #[test] @@ -1352,36 +1337,6 @@ mod tests { assert!(app_server.analytics_default_enabled); } - #[test] - fn app_server_listen_websocket_url_parses() { - let app_server = app_server_from_args( - ["codex", "app-server", "--listen", "ws://127.0.0.1:4500"].as_ref(), - ); - assert_eq!( - app_server.listen, - codex_app_server::AppServerTransport::WebSocket { - bind_address: "127.0.0.1:4500".parse().expect("valid socket address"), - } - ); - } - - #[test] - fn app_server_listen_stdio_url_parses() { - let app_server = - app_server_from_args(["codex", "app-server", "--listen", "stdio://"].as_ref()); - assert_eq!( - app_server.listen, - codex_app_server::AppServerTransport::Stdio - ); - } - - #[test] - fn app_server_listen_invalid_url_fails_to_parse() { - let parse_result = - MultitoolCli::try_parse_from(["codex", "app-server", "--listen", "http://foo"]); - assert!(parse_result.is_err()); - } - #[test] fn features_enable_parses_feature_name() { let cli = MultitoolCli::try_parse_from(["codex", "features", "enable", "unified_exec"]) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 38fec2e121..d597d5ef04 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1946,7 +1946,7 @@ impl Session { /// Emit an exec approval request event and await the user's decision. /// - /// The request is keyed by `sub_id`/`call_id` so matching responses are delivered + /// The request is keyed by `call_id` so matching responses are delivered /// to the correct in-flight turn. If the task is aborted, this returns the /// default `ReviewDecision` (`Denied`). #[allow(clippy::too_many_arguments)] @@ -1959,22 +1959,21 @@ impl Session { reason: Option, proposed_execpolicy_amendment: Option, ) -> ReviewDecision { - let sub_id = turn_context.sub_id.clone(); // Add the tx_approve callback to the map before sending the request. let (tx_approve, rx_approve) = oneshot::channel(); - let event_id = sub_id.clone(); + let approval_id = call_id.clone(); let prev_entry = { let mut active = self.active_turn.lock().await; match active.as_mut() { Some(at) => { let mut ts = at.turn_state.lock().await; - ts.insert_pending_approval(sub_id, tx_approve) + ts.insert_pending_approval(approval_id.clone(), tx_approve) } None => None, } }; if prev_entry.is_some() { - warn!("Overwriting existing pending approval for sub_id: {event_id}"); + warn!("Overwriting existing pending approval for call_id: {approval_id}"); } let parsed_cmd = parse_command(&command); @@ -1999,22 +1998,21 @@ impl Session { reason: Option, grant_root: Option, ) -> oneshot::Receiver { - let sub_id = turn_context.sub_id.clone(); // Add the tx_approve callback to the map before sending the request. let (tx_approve, rx_approve) = oneshot::channel(); - let event_id = sub_id.clone(); + let approval_id = call_id.clone(); let prev_entry = { let mut active = self.active_turn.lock().await; match active.as_mut() { Some(at) => { let mut ts = at.turn_state.lock().await; - ts.insert_pending_approval(sub_id, tx_approve) + ts.insert_pending_approval(approval_id.clone(), tx_approve) } None => None, } }; if prev_entry.is_some() { - warn!("Overwriting existing pending approval for sub_id: {event_id}"); + warn!("Overwriting existing pending approval for call_id: {approval_id}"); } let event = EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { @@ -2106,13 +2104,13 @@ impl Session { } } - pub async fn notify_approval(&self, sub_id: &str, decision: ReviewDecision) { + pub async fn notify_approval(&self, approval_id: &str, decision: ReviewDecision) { let entry = { let mut active = self.active_turn.lock().await; match active.as_mut() { Some(at) => { let mut ts = at.turn_state.lock().await; - ts.remove_pending_approval(sub_id) + ts.remove_pending_approval(approval_id) } None => None, } @@ -2122,7 +2120,7 @@ impl Session { tx_approve.send(decision).ok(); } None => { - warn!("No pending approval found for sub_id: {sub_id}"); + warn!("No pending approval found for call_id: {approval_id}"); } } } @@ -2881,8 +2879,12 @@ async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiv handlers::user_input_or_turn(&sess, sub.id.clone(), sub.op, &mut previous_context) .await; } - Op::ExecApproval { id, decision } => { - handlers::exec_approval(&sess, id, decision).await; + Op::ExecApproval { + id: approval_id, + turn_id, + decision, + } => { + handlers::exec_approval(&sess, approval_id, turn_id, decision).await; } Op::PatchApproval { id, decision } => { handlers::patch_approval(&sess, id, decision).await; @@ -3205,7 +3207,13 @@ mod handlers { /// Propagate a user's exec approval decision to the session. /// Also optionally applies an execpolicy amendment. - pub async fn exec_approval(sess: &Arc, id: String, decision: ReviewDecision) { + pub async fn exec_approval( + sess: &Arc, + approval_id: String, + turn_id: Option, + decision: ReviewDecision, + ) { + let event_turn_id = turn_id.unwrap_or_else(|| approval_id.clone()); if let ReviewDecision::ApprovedExecpolicyAmendment { proposed_execpolicy_amendment, } = &decision @@ -3215,15 +3223,18 @@ mod handlers { .await { Ok(()) => { - sess.record_execpolicy_amendment_message(&id, proposed_execpolicy_amendment) - .await; + sess.record_execpolicy_amendment_message( + &event_turn_id, + proposed_execpolicy_amendment, + ) + .await; } Err(err) => { let message = format!("Failed to apply execpolicy amendment: {err}"); tracing::warn!("{message}"); let warning = EventMsg::Warning(WarningEvent { message }); sess.send_event_raw(Event { - id: id.clone(), + id: event_turn_id.clone(), msg: warning, }) .await; @@ -3234,7 +3245,7 @@ mod handlers { ReviewDecision::Abort => { sess.interrupt_task().await; } - other => sess.notify_approval(&id, other).await, + other => sess.notify_approval(&approval_id, other).await, } } diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index cb5a31a0cc..b9af1dd5f3 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -307,7 +307,7 @@ async fn forward_ops( /// Handle an ExecApprovalRequest by consulting the parent session and replying. async fn handle_exec_approval( codex: &Codex, - id: String, + turn_id: String, parent_session: &Session, parent_ctx: &TurnContext, event: ExecApprovalRequestEvent, @@ -321,6 +321,7 @@ async fn handle_exec_approval( proposed_execpolicy_amendment, .. } = event; + let approval_id = call_id.clone(); // Race approval with cancellation and timeout to avoid hangs. let approval_fut = parent_session.request_command_approval( parent_ctx, @@ -330,21 +331,22 @@ async fn handle_exec_approval( reason, proposed_execpolicy_amendment, ); - let decision = await_approval_with_cancel( - approval_fut, - parent_session, - &parent_ctx.sub_id, - cancel_token, - ) - .await; + let decision = + await_approval_with_cancel(approval_fut, parent_session, &approval_id, cancel_token).await; - let _ = codex.submit(Op::ExecApproval { id, decision }).await; + let _ = codex + .submit(Op::ExecApproval { + id: approval_id, + turn_id: Some(turn_id), + decision, + }) + .await; } /// Handle an ApplyPatchApprovalRequest by consulting the parent session and replying. async fn handle_patch_approval( codex: &Codex, - id: String, + _id: String, parent_session: &Session, parent_ctx: &TurnContext, event: ApplyPatchApprovalRequestEvent, @@ -357,17 +359,23 @@ async fn handle_patch_approval( grant_root, .. } = event; + let approval_id = call_id.clone(); let decision_rx = parent_session .request_patch_approval(parent_ctx, call_id, changes, reason, grant_root) .await; let decision = await_approval_with_cancel( async move { decision_rx.await.unwrap_or_default() }, parent_session, - &parent_ctx.sub_id, + &approval_id, cancel_token, ) .await; - let _ = codex.submit(Op::PatchApproval { id, decision }).await; + let _ = codex + .submit(Op::PatchApproval { + id: approval_id, + decision, + }) + .await; } async fn handle_request_user_input( @@ -423,7 +431,7 @@ where async fn await_approval_with_cancel( fut: F, parent_session: &Session, - sub_id: &str, + approval_id: &str, cancel_token: &CancellationToken, ) -> codex_protocol::protocol::ReviewDecision where @@ -433,7 +441,7 @@ where biased; _ = cancel_token.cancelled() => { parent_session - .notify_approval(sub_id, codex_protocol::protocol::ReviewDecision::Abort) + .notify_approval(approval_id, codex_protocol::protocol::ReviewDecision::Abort) .await; codex_protocol::protocol::ReviewDecision::Abort } diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 137b2c0056..b451d21b57 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -10,6 +10,7 @@ use async_channel::unbounded; pub use codex_app_server_protocol::AppInfo; use codex_protocol::protocol::SandboxPolicy; use tokio_util::sync::CancellationToken; +use tracing::warn; use crate::AuthManager; use crate::CodexAuth; @@ -96,6 +97,16 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_options( ) .await; + if force_refetch + && let Err(err) = mcp_connection_manager + .hard_refresh_codex_apps_tools_cache() + .await + { + warn!( + "failed to force-refresh tools for MCP server '{CODEX_APPS_MCP_SERVER_NAME}', using cached/startup tools: {err:#}" + ); + } + let codex_apps_ready = if let Some(cfg) = mcp_servers.get(CODEX_APPS_MCP_SERVER_NAME) { let timeout = cfg.startup_timeout_sec.unwrap_or(DEFAULT_STARTUP_TIMEOUT); mcp_connection_manager diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 4a6357162f..6f6bfad6e5 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -531,6 +531,33 @@ impl McpConnectionManager { tools } + /// Force-refresh codex apps tools by bypassing the in-process cache. + /// + /// On success, the refreshed tools replace the cache contents. On failure, + /// the existing cache remains unchanged. + pub async fn hard_refresh_codex_apps_tools_cache(&self) -> Result<()> { + let managed_client = self + .clients + .get(CODEX_APPS_MCP_SERVER_NAME) + .ok_or_else(|| anyhow!("unknown MCP server '{CODEX_APPS_MCP_SERVER_NAME}'"))? + .client() + .await + .context("failed to get client")?; + + let tools = list_tools_for_client_uncached( + CODEX_APPS_MCP_SERVER_NAME, + &managed_client.client, + managed_client.tool_timeout, + ) + .await + .with_context(|| { + format!("failed to refresh tools for MCP server '{CODEX_APPS_MCP_SERVER_NAME}'") + })?; + + write_cached_codex_apps_tools(&tools); + Ok(()) + } + /// Returns a single map that contains all resources. Each key is the /// server name and the value is a vector of resources. pub async fn list_all_resources(&self) -> HashMap> { diff --git a/codex-rs/core/src/model_provider_info.rs b/codex-rs/core/src/model_provider_info.rs index 305233ae7a..5d5ee36692 100644 --- a/codex-rs/core/src/model_provider_info.rs +++ b/codex-rs/core/src/model_provider_info.rs @@ -16,7 +16,6 @@ use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; use std::collections::HashMap; -use std::env::VarError; use std::time::Duration; const DEFAULT_STREAM_IDLE_TIMEOUT_MS: u64 = 300_000; @@ -116,7 +115,9 @@ pub struct ModelProviderInfo { impl ModelProviderInfo { fn build_header_map(&self) -> crate::error::Result { - let mut headers = HeaderMap::new(); + let capacity = self.http_headers.as_ref().map_or(0, HashMap::len) + + self.env_http_headers.as_ref().map_or(0, HashMap::len); + let mut headers = HeaderMap::with_capacity(capacity); if let Some(extra) = &self.http_headers { for (k, v) in extra { if let (Ok(name), Ok(value)) = (HeaderName::try_from(k), HeaderValue::try_from(v)) { @@ -179,21 +180,16 @@ impl ModelProviderInfo { pub fn api_key(&self) -> crate::error::Result> { match &self.env_key { Some(env_key) => { - let env_value = std::env::var(env_key); - env_value - .and_then(|v| { - if v.trim().is_empty() { - Err(VarError::NotPresent) - } else { - Ok(Some(v)) - } - }) - .map_err(|_| { + let api_key = std::env::var(env_key) + .ok() + .filter(|v| !v.trim().is_empty()) + .ok_or_else(|| { crate::error::CodexErr::EnvVar(EnvVarError { var: env_key.clone(), instructions: self.env_key_instructions.clone(), }) - }) + })?; + Ok(Some(api_key)) } None => Ok(None), } @@ -298,20 +294,19 @@ pub fn built_in_model_providers() -> HashMap { pub fn create_oss_provider(default_provider_port: u16, wire_api: WireApi) -> ModelProviderInfo { // These CODEX_OSS_ environment variables are experimental: we may // switch to reading values from config.toml instead. - let codex_oss_base_url = match std::env::var("CODEX_OSS_BASE_URL") + let default_codex_oss_base_url = format!( + "http://localhost:{codex_oss_port}/v1", + codex_oss_port = std::env::var("CODEX_OSS_PORT") + .ok() + .filter(|value| !value.trim().is_empty()) + .and_then(|value| value.parse::().ok()) + .unwrap_or(default_provider_port) + ); + + let codex_oss_base_url = std::env::var("CODEX_OSS_BASE_URL") .ok() .filter(|v| !v.trim().is_empty()) - { - Some(url) => url, - None => format!( - "http://localhost:{port}/v1", - port = std::env::var("CODEX_OSS_PORT") - .ok() - .filter(|v| !v.trim().is_empty()) - .and_then(|v| v.parse::().ok()) - .unwrap_or(default_provider_port) - ), - }; + .unwrap_or(default_codex_oss_base_url); create_oss_provider_with_base_url(&codex_oss_base_url, wire_api) } diff --git a/codex-rs/core/src/rollout/list.rs b/codex-rs/core/src/rollout/list.rs index 940402902e..af9223119c 100644 --- a/codex-rs/core/src/rollout/list.rs +++ b/codex-rs/core/src/rollout/list.rs @@ -1015,25 +1015,27 @@ async fn read_head_summary(path: &Path, head_limit: usize) -> io::Result { - summary.source = Some(session_meta_line.meta.source.clone()); - summary.model_provider = session_meta_line.meta.model_provider.clone(); - summary.thread_id = Some(session_meta_line.meta.id); - summary.cwd = Some(session_meta_line.meta.cwd.clone()); - summary.git_branch = session_meta_line - .git - .as_ref() - .and_then(|git| git.branch.clone()); - summary.git_sha = session_meta_line - .git - .as_ref() - .and_then(|git| git.commit_hash.clone()); - summary.git_origin_url = session_meta_line - .git - .as_ref() - .and_then(|git| git.repository_url.clone()); - summary.cli_version = Some(session_meta_line.meta.cli_version); - summary.created_at = Some(session_meta_line.meta.timestamp.clone()); - summary.saw_session_meta = true; + if !summary.saw_session_meta { + summary.source = Some(session_meta_line.meta.source.clone()); + summary.model_provider = session_meta_line.meta.model_provider.clone(); + summary.thread_id = Some(session_meta_line.meta.id); + summary.cwd = Some(session_meta_line.meta.cwd.clone()); + summary.git_branch = session_meta_line + .git + .as_ref() + .and_then(|git| git.branch.clone()); + summary.git_sha = session_meta_line + .git + .as_ref() + .and_then(|git| git.commit_hash.clone()); + summary.git_origin_url = session_meta_line + .git + .as_ref() + .and_then(|git| git.repository_url.clone()); + summary.cli_version = Some(session_meta_line.meta.cli_version); + summary.created_at = Some(session_meta_line.meta.timestamp.clone()); + summary.saw_session_meta = true; + } } RolloutItem::ResponseItem(_) => { summary.created_at = summary diff --git a/codex-rs/core/src/rollout/tests.rs b/codex-rs/core/src/rollout/tests.rs index a4bd72433e..a2e54d43f3 100644 --- a/codex-rs/core/src/rollout/tests.rs +++ b/codex-rs/core/src/rollout/tests.rs @@ -881,6 +881,7 @@ async fn test_list_threads_scans_past_head_for_user_event() { .unwrap(); assert_eq!(page.items.len(), 1); + assert_eq!(page.items[0].thread_id, Some(thread_id_from_uuid(uuid))); } #[tokio::test] diff --git a/codex-rs/core/src/seatbelt.rs b/codex-rs/core/src/seatbelt.rs index c08f70b1e5..e2061b52dc 100644 --- a/codex-rs/core/src/seatbelt.rs +++ b/codex-rs/core/src/seatbelt.rs @@ -243,7 +243,6 @@ pub(crate) fn create_seatbelt_command_args( "" }; - // TODO(mbolin): apply_patch calls must also honor the SandboxPolicy. let proxy = proxy_policy_inputs(network); let network_policy = dynamic_network_policy(sandbox_policy, enforce_managed_network, &proxy); diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 1b295964e8..ccf8760c8d 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1528,7 +1528,8 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { } test.codex .submit(Op::ExecApproval { - id: "0".into(), + id: approval.call_id, + turn_id: None, decision: decision.clone(), }) .await?; @@ -1549,7 +1550,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { } test.codex .submit(Op::PatchApproval { - id: "0".into(), + id: approval.call_id, decision: decision.clone(), }) .await?; @@ -1624,10 +1625,10 @@ async fn approving_apply_patch_for_session_skips_future_prompts_for_same_file() sandbox_policy.clone(), ) .await?; - let _ = expect_patch_approval(&test, call_id_1).await; + let approval = expect_patch_approval(&test, call_id_1).await; test.codex .submit(Op::PatchApproval { - id: "0".into(), + id: approval.call_id, decision: ReviewDecision::ApprovedForSession, }) .await?; @@ -1746,7 +1747,8 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts test.codex .submit(Op::ExecApproval { - id: "0".into(), + id: approval.call_id, + turn_id: None, decision: ReviewDecision::ApprovedExecpolicyAmendment { proposed_execpolicy_amendment: expected_execpolicy_amendment.clone(), }, diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs index c524234c80..ea759ba5f4 100644 --- a/codex-rs/core/tests/suite/codex_delegate.rs +++ b/codex-rs/core/tests/suite/codex_delegate.rs @@ -87,15 +87,19 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() { .await; // Expect parent-side approval request (forwarded by delegate). - wait_for_event(&test.codex, |ev| { + let approval_event = wait_for_event(&test.codex, |ev| { matches!(ev, EventMsg::ExecApprovalRequest(_)) }) .await; + let EventMsg::ExecApprovalRequest(approval) = approval_event else { + panic!("expected ExecApprovalRequest event"); + }; - // Approve via parent; id "0" is the active sub_id in tests. + // Approve via parent using the emitted approval call ID. test.codex .submit(Op::ExecApproval { - id: "0".into(), + id: approval.call_id, + turn_id: None, decision: ReviewDecision::Approved, }) .await @@ -161,15 +165,18 @@ async fn codex_delegate_forwards_patch_approval_and_proceeds_on_decision() { matches!(ev, EventMsg::EnteredReviewMode(_)) }) .await; - wait_for_event(&test.codex, |ev| { + let approval_event = wait_for_event(&test.codex, |ev| { matches!(ev, EventMsg::ApplyPatchApprovalRequest(_)) }) .await; + let EventMsg::ApplyPatchApprovalRequest(approval) = approval_event else { + panic!("expected ApplyPatchApprovalRequest event"); + }; - // Deny via parent so delegate can continue; id "0" is the active sub_id in tests. + // Deny via parent so delegate can continue, using the emitted approval call ID. test.codex .submit(Op::PatchApproval { - id: "0".into(), + id: approval.call_id, decision: ReviewDecision::Denied, }) .await diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index 5138a145ba..28bf7d71df 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -1032,11 +1032,16 @@ async fn handle_container_exec_user_approved_records_tool_decision() { .await .unwrap(); - wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let approval_event = + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let EventMsg::ExecApprovalRequest(approval) = approval_event else { + panic!("expected ExecApprovalRequest event"); + }; codex .submit(Op::ExecApproval { - id: "0".into(), + id: approval.call_id, + turn_id: None, decision: ReviewDecision::Approved, }) .await @@ -1092,11 +1097,16 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision() .await .unwrap(); - wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let approval_event = + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let EventMsg::ExecApprovalRequest(approval) = approval_event else { + panic!("expected ExecApprovalRequest event"); + }; codex .submit(Op::ExecApproval { - id: "0".into(), + id: approval.call_id, + turn_id: None, decision: ReviewDecision::ApprovedForSession, }) .await @@ -1152,11 +1162,16 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() { .await .unwrap(); - wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let approval_event = + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let EventMsg::ExecApprovalRequest(approval) = approval_event else { + panic!("expected ExecApprovalRequest event"); + }; codex .submit(Op::ExecApproval { - id: "0".into(), + id: approval.call_id, + turn_id: None, decision: ReviewDecision::Approved, }) .await @@ -1212,11 +1227,16 @@ async fn handle_container_exec_user_denies_records_tool_decision() { .await .unwrap(); - wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let approval_event = + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let EventMsg::ExecApprovalRequest(approval) = approval_event else { + panic!("expected ExecApprovalRequest event"); + }; codex .submit(Op::ExecApproval { - id: "0".into(), + id: approval.call_id, + turn_id: None, decision: ReviewDecision::Denied, }) .await @@ -1272,11 +1292,16 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision() .await .unwrap(); - wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let approval_event = + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let EventMsg::ExecApprovalRequest(approval) = approval_event else { + panic!("expected ExecApprovalRequest event"); + }; codex .submit(Op::ExecApproval { - id: "0".into(), + id: approval.call_id, + turn_id: None, decision: ReviewDecision::ApprovedForSession, }) .await @@ -1333,11 +1358,16 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() { .await .unwrap(); - wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let approval_event = + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecApprovalRequest(_))).await; + let EventMsg::ExecApprovalRequest(approval) = approval_event else { + panic!("expected ExecApprovalRequest event"); + }; codex .submit(Op::ExecApproval { - id: "0".into(), + id: approval.call_id, + turn_id: None, decision: ReviewDecision::Denied, }) .await diff --git a/codex-rs/core/tests/suite/prompt_caching.rs b/codex-rs/core/tests/suite/prompt_caching.rs index c59be715d1..e1ef097e20 100644 --- a/codex-rs/core/tests/suite/prompt_caching.rs +++ b/codex-rs/core/tests/suite/prompt_caching.rs @@ -551,9 +551,11 @@ async fn override_before_first_turn_emits_environment_context() -> anyhow::Resul }) .collect(); assert!( - permissions_texts - .iter() - .any(|text| text.contains("`approval_policy` is `never`")), + permissions_texts.iter().any(|text| { + let lower = text.to_ascii_lowercase(); + (lower.contains("approval policy") || lower.contains("approval_policy")) + && lower.contains("never") + }), "permissions message should reflect overridden approval policy: {permissions_texts:?}" ); diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 1c9c3adf7b..a7010ecaf1 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -371,6 +371,25 @@ async fn review_does_not_emit_agent_message_on_structured_output() { _ => false, }) .await; + // On slower CI hosts, the final AgentMessage can arrive immediately after + // TurnComplete. Drain a brief tail window to make ordering nondeterminism + // harmless while still enforcing "exactly one final AgentMessage". + while let Ok(Ok(event)) = + tokio::time::timeout(std::time::Duration::from_millis(200), codex.next_event()).await + { + match event.msg { + EventMsg::AgentMessage(_) => agent_messages += 1, + EventMsg::EnteredReviewMode(_) => saw_entered = true, + EventMsg::ExitedReviewMode(_) => saw_exited = true, + EventMsg::AgentMessageContentDelta(_) => { + panic!("unexpected AgentMessageContentDelta surfaced during review") + } + EventMsg::AgentMessageDelta(_) => { + panic!("unexpected AgentMessageDelta surfaced during review") + } + _ => {} + } + } assert_eq!(1, agent_messages, "expected exactly one AgentMessage event"); assert!(saw_entered && saw_exited, "missing review lifecycle events"); diff --git a/codex-rs/core/tests/suite/tool_parallelism.rs b/codex-rs/core/tests/suite/tool_parallelism.rs index b5a7a3a17f..48b9ced089 100644 --- a/codex-rs/core/tests/suite/tool_parallelism.rs +++ b/codex-rs/core/tests/suite/tool_parallelism.rs @@ -72,7 +72,7 @@ async fn build_codex_with_test_tool(server: &wiremock::MockServer) -> anyhow::Re fn assert_parallel_duration(actual: Duration) { // Allow headroom for slow CI scheduling; barrier synchronization already enforces overlap. assert!( - actual < Duration::from_millis(1_200), + actual < Duration::from_millis(1_600), "expected parallel execution to finish quickly, got {actual:?}" ); } diff --git a/codex-rs/debug-client/src/client.rs b/codex-rs/debug-client/src/client.rs index cf54ef9855..cacb5a7732 100644 --- a/codex-rs/debug-client/src/client.rs +++ b/codex-rs/debug-client/src/client.rs @@ -102,6 +102,7 @@ impl AppServerClient { }, capabilities: Some(InitializeCapabilities { experimental_api: true, + opt_out_notification_methods: None, }), }, }; diff --git a/codex-rs/mcp-server/src/exec_approval.rs b/codex-rs/mcp-server/src/exec_approval.rs index c7914d1e60..f4b0a3fe61 100644 --- a/codex-rs/mcp-server/src/exec_approval.rs +++ b/codex-rs/mcp-server/src/exec_approval.rs @@ -60,6 +60,7 @@ pub(crate) async fn handle_exec_approval_request( codex_parsed_cmd: Vec, thread_id: ThreadId, ) { + let approval_id = call_id.clone(); let escaped_command = shlex::try_join(command.iter().map(String::as_str)).unwrap_or_else(|_| command.join(" ")); let message = format!( @@ -100,14 +101,16 @@ pub(crate) async fn handle_exec_approval_request( // Listen for the response on a separate task so we don't block the main agent loop. { let codex = codex.clone(); + let approval_id = approval_id.clone(); let event_id = event_id.clone(); tokio::spawn(async move { - on_exec_approval_response(event_id, on_response, codex).await; + on_exec_approval_response(approval_id, event_id, on_response, codex).await; }); } } async fn on_exec_approval_response( + approval_id: String, event_id: String, receiver: tokio::sync::oneshot::Receiver, codex: Arc, @@ -133,7 +136,8 @@ async fn on_exec_approval_response( if let Err(err) = codex .submit(Op::ExecApproval { - id: event_id, + id: approval_id, + turn_id: Some(event_id), decision: response.decision, }) .await diff --git a/codex-rs/mcp-server/src/patch_approval.rs b/codex-rs/mcp-server/src/patch_approval.rs index 55938e257b..60eba66433 100644 --- a/codex-rs/mcp-server/src/patch_approval.rs +++ b/codex-rs/mcp-server/src/patch_approval.rs @@ -53,6 +53,7 @@ pub(crate) async fn handle_patch_approval_request( event_id: String, thread_id: ThreadId, ) { + let approval_id = call_id.clone(); let mut message_lines = Vec::new(); if let Some(r) = &reason { message_lines.push(r.clone()); @@ -92,15 +93,15 @@ pub(crate) async fn handle_patch_approval_request( // Listen for the response on a separate task so we don't block the main agent loop. { let codex = codex.clone(); - let event_id = event_id.clone(); + let approval_id = approval_id.clone(); tokio::spawn(async move { - on_patch_approval_response(event_id, on_response, codex).await; + on_patch_approval_response(approval_id, on_response, codex).await; }); } } pub(crate) async fn on_patch_approval_response( - event_id: String, + approval_id: String, receiver: tokio::sync::oneshot::Receiver, codex: Arc, ) { @@ -111,7 +112,7 @@ pub(crate) async fn on_patch_approval_response( error!("request failed: {err:?}"); if let Err(submit_err) = codex .submit(Op::PatchApproval { - id: event_id.clone(), + id: approval_id.clone(), decision: ReviewDecision::Denied, }) .await @@ -131,7 +132,7 @@ pub(crate) async fn on_patch_approval_response( if let Err(err) = codex .submit(Op::PatchApproval { - id: event_id, + id: approval_id, decision: response.decision, }) .await diff --git a/codex-rs/protocol/src/prompts/permissions/approval_policy/never.md b/codex-rs/protocol/src/prompts/permissions/approval_policy/never.md index be8fe34e64..851609759c 100644 --- a/codex-rs/protocol/src/prompts/permissions/approval_policy/never.md +++ b/codex-rs/protocol/src/prompts/permissions/approval_policy/never.md @@ -1 +1 @@ - Approvals are your mechanism to get user consent to run shell commands without the sandbox. `approval_policy` is `never`: This is a non-interactive mode where you may NEVER ask the user for approval to run commands. Instead, you must always persist and work around constraints to solve the task for the user. You MUST do your utmost best to finish the task and validate your work before yielding. If this mode is paired with `danger-full-access`, take advantage of it to deliver the best outcome for the user. Further, in this mode, your default testing philosophy is overridden: Even if you don't see local patterns for testing, you may add tests and scripts to validate your work. Just remove them before yielding. +Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected. diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 8f6e998792..2368a39ba3 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -198,6 +198,9 @@ pub enum Op { ExecApproval { /// The id of the submission we are approving id: String, + /// Turn id associated with the approval event, when available. + #[serde(default, skip_serializing_if = "Option::is_none")] + turn_id: Option, /// The user's decision in response to the request. decision: ReviewDecision, }, diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 3252912baf..1dced39b76 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -95,6 +95,7 @@ tree-sitter-highlight = { workspace = true } unicode-segmentation = { workspace = true } unicode-width = { workspace = true } url = { workspace = true } +webbrowser = { workspace = true } uuid = { workspace = true } codex-windows-sandbox = { workspace = true } diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index d18ec23544..b338a2462e 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -645,6 +645,17 @@ impl App { } } + fn open_url_in_browser(&mut self, url: String) { + if let Err(err) = webbrowser::open(&url) { + self.chat_widget + .add_error_message(format!("Failed to open browser for {url}: {err}")); + return; + } + + self.chat_widget + .add_info_message(format!("Opened {url} in your browser."), None); + } + async fn shutdown_current_thread(&mut self) { if let Some(thread_id) = self.chat_widget.thread_id() { // Clear any in-flight rollback guard when switching threads. @@ -1601,6 +1612,12 @@ impl App { is_installed, ); } + AppEvent::OpenUrlInBrowser { url } => { + self.open_url_in_browser(url); + } + AppEvent::RefreshConnectors { force_refetch } => { + self.chat_widget.refresh_connectors(force_refetch); + } AppEvent::StartFileSearch(query) => { self.file_search.on_user_query(query); } diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index b48960bbf1..0f456f4571 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -114,6 +114,16 @@ pub(crate) enum AppEvent { is_installed: bool, }, + /// Open the provided URL in the user's browser. + OpenUrlInBrowser { + url: String, + }, + + /// Refresh app connector state and mention bindings. + RefreshConnectors { + force_refetch: bool, + }, + InsertHistoryCell(Box), StartCommitAnimation, diff --git a/codex-rs/tui/src/bottom_pane/app_link_view.rs b/codex-rs/tui/src/bottom_pane/app_link_view.rs index 1f672607a2..05ef32bfa8 100644 --- a/codex-rs/tui/src/bottom_pane/app_link_view.rs +++ b/codex-rs/tui/src/bottom_pane/app_link_view.rs @@ -1,5 +1,6 @@ use crossterm::event::KeyCode; use crossterm::event::KeyEvent; +use crossterm::event::KeyModifiers; use ratatui::buffer::Buffer; use ratatui::layout::Constraint; use ratatui::layout::Layout; @@ -13,18 +14,33 @@ use textwrap::wrap; use super::CancellationEvent; use super::bottom_pane_view::BottomPaneView; +use super::scroll_state::ScrollState; +use super::selection_popup_common::GenericDisplayRow; +use super::selection_popup_common::measure_rows_height; +use super::selection_popup_common::render_rows; +use crate::app_event::AppEvent; +use crate::app_event_sender::AppEventSender; use crate::key_hint; use crate::render::Insets; use crate::render::RectExt as _; use crate::style::user_message_style; use crate::wrapping::word_wrap_lines; +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum AppLinkScreen { + Link, + InstallConfirmation, +} + pub(crate) struct AppLinkView { title: String, description: Option, instructions: String, url: String, is_installed: bool, + app_event_tx: AppEventSender, + screen: AppLinkScreen, + selected_action: usize, complete: bool, } @@ -35,6 +51,7 @@ impl AppLinkView { instructions: String, url: String, is_installed: bool, + app_event_tx: AppEventSender, ) -> Self { Self { title, @@ -42,11 +59,81 @@ impl AppLinkView { instructions, url, is_installed, + app_event_tx, + screen: AppLinkScreen::Link, + selected_action: 0, complete: false, } } + fn action_labels(&self) -> [&'static str; 2] { + match self.screen { + AppLinkScreen::Link => { + if self.is_installed { + ["Manage on ChatGPT", "Back"] + } else { + ["Install on ChatGPT", "Back"] + } + } + AppLinkScreen::InstallConfirmation => ["I already Installed it", "Back"], + } + } + + fn move_selection_prev(&mut self) { + self.selected_action = self.selected_action.saturating_sub(1); + } + + fn move_selection_next(&mut self) { + self.selected_action = (self.selected_action + 1).min(self.action_labels().len() - 1); + } + + fn handle_primary_action(&mut self) { + match self.screen { + AppLinkScreen::Link => { + self.app_event_tx.send(AppEvent::OpenUrlInBrowser { + url: self.url.clone(), + }); + if !self.is_installed { + self.screen = AppLinkScreen::InstallConfirmation; + self.selected_action = 0; + } + } + AppLinkScreen::InstallConfirmation => { + self.app_event_tx.send(AppEvent::RefreshConnectors { + force_refetch: true, + }); + self.complete = true; + } + } + } + + fn handle_secondary_action(&mut self) { + match self.screen { + AppLinkScreen::Link => { + self.complete = true; + } + AppLinkScreen::InstallConfirmation => { + self.screen = AppLinkScreen::Link; + self.selected_action = 0; + } + } + } + + fn activate_selected_action(&mut self) { + match self.selected_action { + 0 => self.handle_primary_action(), + _ => self.handle_secondary_action(), + } + } + fn content_lines(&self, width: u16) -> Vec> { + match self.screen { + AppLinkScreen::Link => self.link_content_lines(width), + AppLinkScreen::InstallConfirmation => self.install_confirmation_lines(width), + } + } + + fn link_content_lines(&self, width: u16) -> Vec> { let usable_width = width.max(1) as usize; let mut lines: Vec> = Vec::new(); @@ -61,6 +148,7 @@ impl AppLinkView { lines.push(Line::from(line.into_owned().dim())); } } + lines.push(Line::from("")); if self.is_installed { for line in wrap("Use $ to insert this app into the prompt.", usable_width) { @@ -91,21 +179,156 @@ impl AppLinkView { lines.push(Line::from("")); } - lines.push(Line::from(vec!["Open:".dim()])); + lines + } + + fn install_confirmation_lines(&self, width: u16) -> Vec> { + let usable_width = width.max(1) as usize; + let mut lines: Vec> = Vec::new(); + + lines.push(Line::from("Finish App Setup".bold())); + lines.push(Line::from("")); + + for line in wrap( + "Complete app setup on ChatGPT in the browser window that just opened.", + usable_width, + ) { + lines.push(Line::from(line.into_owned())); + } + for line in wrap( + "Sign in there if needed, then return here and select \"I already Installed it\".", + usable_width, + ) { + lines.push(Line::from(line.into_owned())); + } + + lines.push(Line::from("")); + lines.push(Line::from(vec!["Setup URL:".dim()])); let url_line = Line::from(vec![self.url.clone().cyan().underlined()]); lines.extend(word_wrap_lines(vec![url_line], usable_width)); lines } + + fn action_rows(&self) -> Vec { + self.action_labels() + .into_iter() + .enumerate() + .map(|(index, label)| { + let prefix = if self.selected_action == index { + '›' + } else { + ' ' + }; + GenericDisplayRow { + name: format!("{prefix} {}. {label}", index + 1), + ..Default::default() + } + }) + .collect() + } + + fn action_state(&self) -> ScrollState { + let mut state = ScrollState::new(); + state.selected_idx = Some(self.selected_action); + state + } + + fn action_rows_height(&self, width: u16) -> u16 { + let rows = self.action_rows(); + let state = self.action_state(); + measure_rows_height(&rows, &state, rows.len().max(1), width.max(1)) + } + + fn hint_line(&self) -> Line<'static> { + Line::from(vec![ + "Use ".into(), + key_hint::plain(KeyCode::Tab).into(), + " / ".into(), + key_hint::plain(KeyCode::Up).into(), + " ".into(), + key_hint::plain(KeyCode::Down).into(), + " to move, ".into(), + key_hint::plain(KeyCode::Enter).into(), + " to select, ".into(), + key_hint::plain(KeyCode::Esc).into(), + " to close".into(), + ]) + } } impl BottomPaneView for AppLinkView { fn handle_key_event(&mut self, key_event: KeyEvent) { - if let KeyEvent { - code: KeyCode::Esc, .. - } = key_event - { - self.on_ctrl_c(); + match key_event { + KeyEvent { + code: KeyCode::Esc, .. + } => { + self.on_ctrl_c(); + } + KeyEvent { + code: KeyCode::Up, .. + } + | KeyEvent { + code: KeyCode::Left, + .. + } + | KeyEvent { + code: KeyCode::BackTab, + .. + } + | KeyEvent { + code: KeyCode::Char('k'), + modifiers: KeyModifiers::NONE, + .. + } + | KeyEvent { + code: KeyCode::Char('h'), + modifiers: KeyModifiers::NONE, + .. + } => self.move_selection_prev(), + KeyEvent { + code: KeyCode::Down, + .. + } + | KeyEvent { + code: KeyCode::Right, + .. + } + | KeyEvent { + code: KeyCode::Tab, .. + } + | KeyEvent { + code: KeyCode::Char('j'), + modifiers: KeyModifiers::NONE, + .. + } + | KeyEvent { + code: KeyCode::Char('l'), + modifiers: KeyModifiers::NONE, + .. + } => self.move_selection_next(), + KeyEvent { + code: KeyCode::Char('1'), + modifiers: KeyModifiers::NONE, + .. + } => { + self.selected_action = 0; + self.activate_selected_action(); + } + KeyEvent { + code: KeyCode::Char('2'), + modifiers: KeyModifiers::NONE, + .. + } => { + self.selected_action = 1; + self.activate_selected_action(); + } + KeyEvent { + code: KeyCode::Enter, + modifiers: KeyModifiers::NONE, + .. + } => self.activate_selected_action(), + _ => {} } } @@ -123,7 +346,8 @@ impl crate::render::renderable::Renderable for AppLinkView { fn desired_height(&self, width: u16) -> u16 { let content_width = width.saturating_sub(4).max(1); let content_lines = self.content_lines(content_width); - content_lines.len() as u16 + 3 + let action_rows_height = self.action_rows_height(content_width); + content_lines.len() as u16 + action_rows_height + 3 } fn render(&self, area: Rect, buf: &mut Buffer) { @@ -135,13 +359,38 @@ impl crate::render::renderable::Renderable for AppLinkView { .style(user_message_style()) .render(area, buf); - let [content_area, hint_area] = - Layout::vertical([Constraint::Fill(1), Constraint::Length(1)]).areas(area); + let actions_height = self.action_rows_height(area.width.saturating_sub(4)); + let [content_area, actions_area, hint_area] = Layout::vertical([ + Constraint::Fill(1), + Constraint::Length(actions_height), + Constraint::Length(1), + ]) + .areas(area); + let inner = content_area.inset(Insets::vh(1, 2)); let content_width = inner.width.max(1); let lines = self.content_lines(content_width); Paragraph::new(lines).render(inner, buf); + if actions_area.height > 0 { + let actions_area = Rect { + x: actions_area.x.saturating_add(2), + y: actions_area.y, + width: actions_area.width.saturating_sub(2), + height: actions_area.height, + }; + let action_rows = self.action_rows(); + let action_state = self.action_state(); + render_rows( + actions_area, + buf, + &action_rows, + &action_state, + action_rows.len().max(1), + "No actions", + ); + } + if hint_area.height > 0 { let hint_area = Rect { x: hint_area.x.saturating_add(2), @@ -149,15 +398,7 @@ impl crate::render::renderable::Renderable for AppLinkView { width: hint_area.width.saturating_sub(2), height: hint_area.height, }; - hint_line().dim().render(hint_area, buf); + self.hint_line().dim().render(hint_area, buf); } } } - -fn hint_line() -> Line<'static> { - Line::from(vec![ - "Press ".into(), - key_hint::plain(KeyCode::Esc).into(), - " to close".into(), - ]) -} diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 7ec39cedbc..42abe174d4 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -195,6 +195,7 @@ impl ApprovalOverlay { self.app_event_tx.send(AppEvent::InsertHistoryCell(cell)); self.app_event_tx.send(AppEvent::CodexOp(Op::ExecApproval { id: id.to_string(), + turn_id: None, decision, })); } diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 8bf46bdc66..38376a5278 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -25,6 +25,8 @@ //! //! When recalling a local entry, the composer rehydrates text elements and image attachments. //! When recalling a persistent entry, only the text is restored. +//! Recalled entries move the cursor to end-of-line so repeated Up/Down presses keep shell-like +//! history traversal semantics instead of dropping to column 0. //! //! # Submission and Prompt Expansion //! @@ -532,9 +534,12 @@ impl ChatComposer { self.history.set_metadata(log_id, entry_count); } - /// Integrate an asynchronous response to an on-demand history lookup. If - /// the entry is present and the offset matches the current cursor we - /// immediately populate the textarea. + /// Integrate an asynchronous response to an on-demand history lookup. + /// + /// If the entry is present and the offset still matches the active history cursor, the + /// composer rehydrates the entry immediately. This path intentionally routes through + /// [`Self::apply_history_entry`] so cursor placement remains aligned with keyboard history + /// recall semantics. pub(crate) fn on_history_entry_response( &mut self, log_id: u64, @@ -783,6 +788,10 @@ impl ChatComposer { /// draft; if callers restore only text and images, mentions can appear /// intact to users while resolving to the wrong target or dropping on /// retry. + /// + /// This helper intentionally places the cursor at the start of the restored text. Callers + /// that need end-of-line restore behavior (for example shell-style history recall) should call + /// [`Self::move_cursor_to_end`] after this method. pub(crate) fn set_text_content_with_mention_bindings( &mut self, text: String, @@ -857,6 +866,13 @@ impl ChatComposer { self.textarea.text().to_string() } + /// Rehydrate a history entry into the composer with shell-like cursor placement. + /// + /// This path restores text, elements, images, mention bindings, and pending paste payloads, + /// then moves the cursor to end-of-line. If a caller reused + /// [`Self::set_text_content_with_mention_bindings`] directly for history recall and forgot the + /// final cursor move, repeated Up/Down would stop navigating history because cursor-gating + /// treats interior positions as normal editing mode. fn apply_history_entry(&mut self, entry: HistoryEntry) { let HistoryEntry { text, @@ -872,6 +888,7 @@ impl ChatComposer { mention_bindings, ); self.set_pending_pastes(pending_pastes); + self.move_cursor_to_end(); } pub(crate) fn text_elements(&self) -> Vec { @@ -5973,6 +5990,51 @@ mod tests { assert_eq!(text_elements.len(), 1); assert_eq!(text_elements[0].placeholder(&text), Some("[Image #1]")); assert_eq!(composer.local_image_paths(), vec![path]); + assert_eq!(composer.textarea.cursor(), composer.textarea.text().len()); + } + + #[test] + fn history_navigation_leaves_cursor_at_end_of_line() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + composer.set_steer_enabled(true); + + type_chars_humanlike(&mut composer, &['f', 'i', 'r', 's', 't']); + let (result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert!(matches!(result, InputResult::Submitted { .. })); + + type_chars_humanlike(&mut composer, &['s', 'e', 'c', 'o', 'n', 'd']); + let (result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert!(matches!(result, InputResult::Submitted { .. })); + + let (_result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + assert_eq!(composer.textarea.text(), "second"); + assert_eq!(composer.textarea.cursor(), composer.textarea.text().len()); + + let (_result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + assert_eq!(composer.textarea.text(), "first"); + assert_eq!(composer.textarea.cursor(), composer.textarea.text().len()); + + let (_result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); + assert_eq!(composer.textarea.text(), "second"); + assert_eq!(composer.textarea.cursor(), composer.textarea.text().len()); + + let (_result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); + assert!(composer.textarea.is_empty()); + assert_eq!(composer.textarea.cursor(), composer.textarea.text().len()); } #[test] 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 d080b79369..e1c1d32ca1 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs @@ -82,7 +82,8 @@ pub(crate) struct ChatComposerHistory { /// The text that was last inserted into the composer as a result of /// history navigation. Used to decide if further Up/Down presses should be - /// treated as navigation versus normal cursor movement. + /// treated as navigation versus normal cursor movement, together with the + /// "cursor at line boundary" check in [`Self::should_handle_navigation`]. last_history_text: Option, } @@ -136,8 +137,16 @@ impl ChatComposerHistory { self.last_history_text = None; } - /// Should Up/Down key presses be interpreted as history navigation given - /// the current content and cursor position of `textarea`? + /// Returns whether Up/Down should navigate history for the current textarea state. + /// + /// Empty text always enables history traversal. For non-empty text, this requires both: + /// + /// - the current text exactly matching the last recalled history entry, and + /// - the cursor being at a line boundary (start or end). + /// + /// This boundary gate keeps multiline cursor movement usable while preserving shell-like + /// history recall. If callers moved the cursor into the middle of a recalled entry and still + /// forced navigation, users would lose normal vertical movement within the draft. pub fn should_handle_navigation(&self, text: &str, cursor: usize) -> bool { if self.history_entry_count == 0 && self.local_history.is_empty() { return false; @@ -147,10 +156,11 @@ impl ChatComposerHistory { return true; } - // Textarea is not empty – only navigate when cursor is at start and - // text matches last recalled history entry so regular editing is not - // hijacked. - if cursor != 0 { + // Textarea is not empty – only navigate when text matches the last + // recalled history entry and the cursor is at a line boundary. This + // keeps shell-like Up/Down recall working while still allowing normal + // multiline cursor movement from interior positions. + if cursor != 0 && cursor != text.len() { return false; } @@ -378,4 +388,16 @@ mod tests { history.navigate_up(&tx) ); } + + #[test] + fn should_handle_navigation_when_cursor_is_at_line_boundaries() { + let mut history = ChatComposerHistory::new(); + history.record_local_submission(HistoryEntry::new("hello".to_string())); + history.last_history_text = Some("hello".to_string()); + + assert!(history.should_handle_navigation("hello", 0)); + assert!(history.should_handle_navigation("hello", "hello".len())); + assert!(!history.should_handle_navigation("hello", 1)); + assert!(!history.should_handle_navigation("other", 0)); + } } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index c381686c9f..e507a39a4d 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1161,6 +1161,7 @@ impl ChatWidget { instructions, url, is_installed, + self.app_event_tx.clone(), ); self.bottom_pane.show_view(Box::new(view)); self.request_redraw(); @@ -1786,21 +1787,19 @@ impl ChatWidget { self.add_to_history(history_cell::new_plan_update(update)); } - fn on_exec_approval_request(&mut self, id: String, ev: ExecApprovalRequestEvent) { - let id2 = id.clone(); + fn on_exec_approval_request(&mut self, _id: String, ev: ExecApprovalRequestEvent) { let ev2 = ev.clone(); self.defer_or_handle( - |q| q.push_exec_approval(id, ev), - |s| s.handle_exec_approval_now(id2, ev2), + |q| q.push_exec_approval(ev), + |s| s.handle_exec_approval_now(ev2), ); } - fn on_apply_patch_approval_request(&mut self, id: String, ev: ApplyPatchApprovalRequestEvent) { - let id2 = id.clone(); + fn on_apply_patch_approval_request(&mut self, _id: String, ev: ApplyPatchApprovalRequestEvent) { let ev2 = ev.clone(); self.defer_or_handle( - |q| q.push_apply_patch_approval(id, ev), - |s| s.handle_apply_patch_approval_now(id2, ev2), + |q| q.push_apply_patch_approval(ev), + |s| s.handle_apply_patch_approval_now(ev2), ); } @@ -2365,14 +2364,14 @@ impl ChatWidget { self.had_work_activity = true; } - pub(crate) fn handle_exec_approval_now(&mut self, id: String, ev: ExecApprovalRequestEvent) { + pub(crate) fn handle_exec_approval_now(&mut self, ev: ExecApprovalRequestEvent) { self.flush_answer_stream_with_separator(); let command = shlex::try_join(ev.command.iter().map(String::as_str)) .unwrap_or_else(|_| ev.command.join(" ")); self.notify(Notification::ExecApprovalRequested { command }); let request = ApprovalRequest::Exec { - id, + id: ev.call_id, command: ev.command, reason: ev.reason, proposed_execpolicy_amendment: ev.proposed_execpolicy_amendment, @@ -2382,15 +2381,11 @@ impl ChatWidget { self.request_redraw(); } - pub(crate) fn handle_apply_patch_approval_now( - &mut self, - id: String, - ev: ApplyPatchApprovalRequestEvent, - ) { + pub(crate) fn handle_apply_patch_approval_now(&mut self, ev: ApplyPatchApprovalRequestEvent) { self.flush_answer_stream_with_separator(); let request = ApprovalRequest::ApplyPatch { - id, + id: ev.call_id, reason: ev.reason, changes: ev.changes.clone(), cwd: self.config.cwd.clone(), @@ -4497,7 +4492,15 @@ impl ChatWidget { } } + pub(crate) fn refresh_connectors(&mut self, force_refetch: bool) { + self.prefetch_connectors_with_options(force_refetch); + } + fn prefetch_connectors(&mut self) { + self.prefetch_connectors_with_options(false); + } + + fn prefetch_connectors_with_options(&mut self, force_refetch: bool) { if !self.connectors_enabled() || self.connectors_prefetch_in_flight { return; } @@ -4511,7 +4514,12 @@ impl ChatWidget { let app_event_tx = self.app_event_tx.clone(); tokio::spawn(async move { let accessible_connectors = - match connectors::list_accessible_connectors_from_mcp_tools(&config).await { + match connectors::list_accessible_connectors_from_mcp_tools_with_options( + &config, + force_refetch, + ) + .await + { Ok(connectors) => connectors, Err(err) => { app_event_tx.send(AppEvent::ConnectorsLoaded { @@ -6366,7 +6374,10 @@ impl ChatWidget { return; } - match self.connectors_cache.clone() { + let connectors_cache = self.connectors_cache.clone(); + self.prefetch_connectors_with_options(true); + + match connectors_cache { ConnectorsCacheState::Ready(snapshot) => { if snapshot.connectors.is_empty() { self.add_info_message("No apps available.".to_string(), None); @@ -6376,8 +6387,6 @@ impl ChatWidget { } ConnectorsCacheState::Failed(err) => { self.add_to_history(history_cell::new_error_event(err)); - // Retry on demand so `/apps` can recover after transient failures. - self.prefetch_connectors(); } ConnectorsCacheState::Loading => { self.add_to_history(history_cell::new_info_event( @@ -6386,7 +6395,6 @@ impl ChatWidget { )); } ConnectorsCacheState::Uninitialized => { - self.prefetch_connectors(); self.add_to_history(history_cell::new_info_event( "Apps are still loading.".to_string(), Some("Try again in a moment.".to_string()), @@ -6736,7 +6744,9 @@ impl ChatWidget { match result { Ok(snapshot) => { self.refresh_connectors_popup_if_open(&snapshot.connectors); - self.connectors_cache = ConnectorsCacheState::Ready(snapshot.clone()); + if is_final || !matches!(self.connectors_cache, ConnectorsCacheState::Ready(_)) { + self.connectors_cache = ConnectorsCacheState::Ready(snapshot.clone()); + } self.bottom_pane.set_connectors_snapshot(Some(snapshot)); } Err(err) => { diff --git a/codex-rs/tui/src/chatwidget/interrupts.rs b/codex-rs/tui/src/chatwidget/interrupts.rs index 5241c79b89..fccd438434 100644 --- a/codex-rs/tui/src/chatwidget/interrupts.rs +++ b/codex-rs/tui/src/chatwidget/interrupts.rs @@ -14,8 +14,8 @@ use super::ChatWidget; #[derive(Debug)] pub(crate) enum QueuedInterrupt { - ExecApproval(String, ExecApprovalRequestEvent), - ApplyPatchApproval(String, ApplyPatchApprovalRequestEvent), + ExecApproval(ExecApprovalRequestEvent), + ApplyPatchApproval(ApplyPatchApprovalRequestEvent), Elicitation(ElicitationRequestEvent), RequestUserInput(RequestUserInputEvent), ExecBegin(ExecCommandBeginEvent), @@ -42,17 +42,13 @@ impl InterruptManager { self.queue.is_empty() } - pub(crate) fn push_exec_approval(&mut self, id: String, ev: ExecApprovalRequestEvent) { - self.queue.push_back(QueuedInterrupt::ExecApproval(id, ev)); + pub(crate) fn push_exec_approval(&mut self, ev: ExecApprovalRequestEvent) { + self.queue.push_back(QueuedInterrupt::ExecApproval(ev)); } - pub(crate) fn push_apply_patch_approval( - &mut self, - id: String, - ev: ApplyPatchApprovalRequestEvent, - ) { + pub(crate) fn push_apply_patch_approval(&mut self, ev: ApplyPatchApprovalRequestEvent) { self.queue - .push_back(QueuedInterrupt::ApplyPatchApproval(id, ev)); + .push_back(QueuedInterrupt::ApplyPatchApproval(ev)); } pub(crate) fn push_elicitation(&mut self, ev: ElicitationRequestEvent) { @@ -86,10 +82,8 @@ impl InterruptManager { pub(crate) fn flush_all(&mut self, chat: &mut ChatWidget) { while let Some(q) = self.queue.pop_front() { match q { - QueuedInterrupt::ExecApproval(id, ev) => chat.handle_exec_approval_now(id, ev), - QueuedInterrupt::ApplyPatchApproval(id, ev) => { - chat.handle_apply_patch_approval_now(id, ev) - } + QueuedInterrupt::ExecApproval(ev) => chat.handle_exec_approval_now(ev), + QueuedInterrupt::ApplyPatchApproval(ev) => chat.handle_apply_patch_approval_now(ev), QueuedInterrupt::Elicitation(ev) => chat.handle_elicitation_request_now(ev), QueuedInterrupt::RequestUserInput(ev) => chat.handle_request_user_input_now(ev), QueuedInterrupt::ExecBegin(ev) => chat.handle_exec_begin_now(ev), diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 78f9dd3081..1f1af74a11 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -3715,6 +3715,10 @@ async fn apps_popup_refreshes_when_connectors_snapshot_updates() { false, ); chat.add_connectors_output(); + assert!( + chat.connectors_prefetch_in_flight, + "expected /apps to trigger a forced connectors refresh" + ); let before = render_bottom_popup(&chat, 80); assert!( @@ -3761,6 +3765,71 @@ async fn apps_popup_refreshes_when_connectors_snapshot_updates() { ); } +#[tokio::test] +async fn apps_refresh_failure_keeps_existing_full_snapshot() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + chat.config.features.enable(Feature::Apps); + chat.bottom_pane.set_connectors_enabled(true); + + let full_connectors = vec![ + codex_chatgpt::connectors::AppInfo { + id: "connector_1".to_string(), + name: "Notion".to_string(), + description: Some("Workspace docs".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + install_url: Some("https://example.test/notion".to_string()), + is_accessible: true, + }, + codex_chatgpt::connectors::AppInfo { + id: "connector_2".to_string(), + name: "Linear".to_string(), + description: Some("Project tracking".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + install_url: Some("https://example.test/linear".to_string()), + is_accessible: false, + }, + ]; + chat.on_connectors_loaded( + Ok(ConnectorsSnapshot { + connectors: full_connectors.clone(), + }), + true, + ); + + chat.on_connectors_loaded( + Ok(ConnectorsSnapshot { + connectors: vec![codex_chatgpt::connectors::AppInfo { + id: "connector_1".to_string(), + name: "Notion".to_string(), + description: Some("Workspace docs".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + install_url: Some("https://example.test/notion".to_string()), + is_accessible: true, + }], + }), + false, + ); + chat.on_connectors_loaded(Err("failed to load apps".to_string()), true); + + assert_matches!( + &chat.connectors_cache, + ConnectorsCacheState::Ready(snapshot) if snapshot.connectors == full_connectors + ); + + chat.add_connectors_output(); + let popup = render_bottom_popup(&chat, 80); + assert!( + popup.contains("Installed 1 of 2 available apps."), + "expected previous full snapshot to be preserved, got:\n{popup}" + ); +} + #[tokio::test] async fn experimental_features_popup_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; @@ -5130,9 +5199,9 @@ async fn apply_patch_manual_flow_snapshot() { } #[tokio::test] -async fn apply_patch_approval_sends_op_with_submission_id() { +async fn apply_patch_approval_sends_op_with_call_id() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; - // Simulate receiving an approval request with a distinct submission id and call id + // Simulate receiving an approval request with a distinct event id and call id. let mut changes = HashMap::new(); changes.insert( PathBuf::from("file.rs"), @@ -5155,11 +5224,11 @@ async fn apply_patch_approval_sends_op_with_submission_id() { // Approve via key press 'y' chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); - // Expect a CodexOp with PatchApproval carrying the submission id, not call id + // Expect a CodexOp with PatchApproval carrying the call id. let mut found = false; while let Ok(app_ev) = rx.try_recv() { if let AppEvent::CodexOp(Op::PatchApproval { id, decision }) = app_ev { - assert_eq!(id, "sub-123"); + assert_eq!(id, "call-999"); assert_matches!(decision, codex_core::protocol::ReviewDecision::Approved); found = true; break; @@ -5207,7 +5276,7 @@ async fn apply_patch_full_flow_integration_like() { .expect("expected op forwarded to codex channel"); match forwarded { Op::PatchApproval { id, decision } => { - assert_eq!(id, "sub-xyz"); + assert_eq!(id, "call-1"); assert_matches!(decision, codex_core::protocol::ReviewDecision::Approved); } other => panic!("unexpected op forwarded: {other:?}"), diff --git a/docs/tui-chat-composer.md b/docs/tui-chat-composer.md index 1e84a1b13b..e4cf0c1b0c 100644 --- a/docs/tui-chat-composer.md +++ b/docs/tui-chat-composer.md @@ -144,6 +144,10 @@ Local history entries capture: Persistent history entries only restore text. They intentionally do **not** rehydrate attachments or pending paste payloads. +For non-empty drafts, Up/Down navigation is only treated as history recall when the current text +matches the last recalled history entry and the cursor is at a boundary (start or end of the +line). This keeps multiline cursor movement intact while preserving shell-like history traversal. + ### Draft recovery (Ctrl+C) Ctrl+C clears the composer but stashes the full draft state (text elements, image paths, and @@ -157,6 +161,7 @@ ranges and local image paths. Pending paste payloads are cleared during submissi placeholders are expanded into their full text before being recorded. This means: - Up/Down recall of a submitted message restores image placeholders and their local paths. +- Recalled entries place the cursor at end-of-line to match typical shell history editing. - Large-paste placeholders are not expected in recalled submitted history; the text is the expanded paste content.