diff --git a/codex-rs/app-server-client/src/lib.rs b/codex-rs/app-server-client/src/lib.rs index 49a7a3b800..c7f5b9889a 100644 --- a/codex-rs/app-server-client/src/lib.rs +++ b/codex-rs/app-server-client/src/lib.rs @@ -2178,11 +2178,13 @@ mod tests { let environment_manager = Arc::new( EnvironmentManager::create_for_tests( Some("ws://127.0.0.1:8765".to_string()), - ExecServerRuntimePaths::new( - std::env::current_exe().expect("current exe"), - /*codex_linux_sandbox_exe*/ None, - ) - .expect("runtime paths"), + Some( + ExecServerRuntimePaths::new( + std::env::current_exe().expect("current exe"), + /*codex_linux_sandbox_exe*/ None, + ) + .expect("runtime paths"), + ), ) .await, ); diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index 2621a0bd1e..58aa1c4449 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -3007,7 +3007,7 @@ "type": "object" }, "ThreadForkParams": { - "description": "There are two ways to fork a thread: 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread. 2. By path: load the thread from disk by path and fork it into a new thread.\n\nIf using path, the thread_id param will be ignored.\n\nPrefer using thread_id whenever possible.", + "description": "There are two ways to fork a thread: 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread. 2. By path: load the thread from disk by path and fork it into a new thread.\n\nIf using a non-empty path, the thread_id param will be ignored. Empty string path values are treated as absent.\n\nPrefer using thread_id whenever possible.", "properties": { "approvalPolicy": { "anyOf": [ @@ -3413,7 +3413,7 @@ ] }, "ThreadResumeParams": { - "description": "There are three ways to resume a thread: 1. By thread_id: load the thread from disk by thread_id and resume it. 2. By history: instantiate the thread from memory and resume it. 3. By path: load the thread from disk by path and resume it.\n\nThe precedence is: history > path > thread_id. If using history or path, the thread_id param will be ignored.\n\nPrefer using thread_id whenever possible.", + "description": "There are three ways to resume a thread: 1. By thread_id: load the thread from disk by thread_id and resume it. 2. By history: instantiate the thread from memory and resume it. 3. By path: load the thread from disk by path and resume it.\n\nFor non-running threads, the precedence is: history > non-empty path > thread_id. If using history or a non-empty path for a non-running thread, the thread_id param will be ignored.\n\nIf thread_id identifies a running thread, app-server rejoins that thread and treats a non-empty path as a consistency check against the active rollout path. Empty string path values are treated as absent.\n\nPrefer using thread_id whenever possible.", "properties": { "approvalPolicy": { "anyOf": [ 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 e219f42551..b50cacefa4 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 @@ -7089,6 +7089,17 @@ ], "type": "object" }, + "ComputerUseRequirements": { + "properties": { + "allowLockedComputerUse": { + "type": [ + "boolean", + "null" + ] + } + }, + "type": "object" + }, "Config": { "additionalProperties": true, "properties": { @@ -7635,6 +7646,16 @@ "null" ] }, + "computerUse": { + "anyOf": [ + { + "$ref": "#/definitions/v2/ComputerUseRequirements" + }, + { + "type": "null" + } + ] + }, "enforceResidency": { "anyOf": [ { @@ -15370,7 +15391,7 @@ }, "ThreadForkParams": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "There are two ways to fork a thread: 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread. 2. By path: load the thread from disk by path and fork it into a new thread.\n\nIf using path, the thread_id param will be ignored.\n\nPrefer using thread_id whenever possible.", + "description": "There are two ways to fork a thread: 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread. 2. By path: load the thread from disk by path and fork it into a new thread.\n\nIf using a non-empty path, the thread_id param will be ignored. Empty string path values are treated as absent.\n\nPrefer using thread_id whenever possible.", "properties": { "approvalPolicy": { "anyOf": [ @@ -16868,7 +16889,7 @@ }, "ThreadResumeParams": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "There are three ways to resume a thread: 1. By thread_id: load the thread from disk by thread_id and resume it. 2. By history: instantiate the thread from memory and resume it. 3. By path: load the thread from disk by path and resume it.\n\nThe precedence is: history > path > thread_id. If using history or path, the thread_id param will be ignored.\n\nPrefer using thread_id whenever possible.", + "description": "There are three ways to resume a thread: 1. By thread_id: load the thread from disk by thread_id and resume it. 2. By history: instantiate the thread from memory and resume it. 3. By path: load the thread from disk by path and resume it.\n\nFor non-running threads, the precedence is: history > non-empty path > thread_id. If using history or a non-empty path for a non-running thread, the thread_id param will be ignored.\n\nIf thread_id identifies a running thread, app-server rejoins that thread and treats a non-empty path as a consistency check against the active rollout path. Empty string path values are treated as absent.\n\nPrefer using thread_id whenever possible.", "properties": { "approvalPolicy": { "anyOf": [ diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index fb1a8d2505..9da530463a 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -3478,6 +3478,17 @@ ], "type": "object" }, + "ComputerUseRequirements": { + "properties": { + "allowLockedComputerUse": { + "type": [ + "boolean", + "null" + ] + } + }, + "type": "object" + }, "Config": { "additionalProperties": true, "properties": { @@ -4024,6 +4035,16 @@ "null" ] }, + "computerUse": { + "anyOf": [ + { + "$ref": "#/definitions/ComputerUseRequirements" + }, + { + "type": "null" + } + ] + }, "enforceResidency": { "anyOf": [ { @@ -13194,7 +13215,7 @@ }, "ThreadForkParams": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "There are two ways to fork a thread: 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread. 2. By path: load the thread from disk by path and fork it into a new thread.\n\nIf using path, the thread_id param will be ignored.\n\nPrefer using thread_id whenever possible.", + "description": "There are two ways to fork a thread: 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread. 2. By path: load the thread from disk by path and fork it into a new thread.\n\nIf using a non-empty path, the thread_id param will be ignored. Empty string path values are treated as absent.\n\nPrefer using thread_id whenever possible.", "properties": { "approvalPolicy": { "anyOf": [ @@ -14692,7 +14713,7 @@ }, "ThreadResumeParams": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "There are three ways to resume a thread: 1. By thread_id: load the thread from disk by thread_id and resume it. 2. By history: instantiate the thread from memory and resume it. 3. By path: load the thread from disk by path and resume it.\n\nThe precedence is: history > path > thread_id. If using history or path, the thread_id param will be ignored.\n\nPrefer using thread_id whenever possible.", + "description": "There are three ways to resume a thread: 1. By thread_id: load the thread from disk by thread_id and resume it. 2. By history: instantiate the thread from memory and resume it. 3. By path: load the thread from disk by path and resume it.\n\nFor non-running threads, the precedence is: history > non-empty path > thread_id. If using history or a non-empty path for a non-running thread, the thread_id param will be ignored.\n\nIf thread_id identifies a running thread, app-server rejoins that thread and treats a non-empty path as a consistency check against the active rollout path. Empty string path values are treated as absent.\n\nPrefer using thread_id whenever possible.", "properties": { "approvalPolicy": { "anyOf": [ diff --git a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json index 577cd7bbc9..3515b5d1d1 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json @@ -60,6 +60,17 @@ } ] }, + "ComputerUseRequirements": { + "properties": { + "allowLockedComputerUse": { + "type": [ + "boolean", + "null" + ] + } + }, + "type": "object" + }, "ConfigRequirements": { "properties": { "allowManagedHooksOnly": { @@ -95,6 +106,16 @@ "null" ] }, + "computerUse": { + "anyOf": [ + { + "$ref": "#/definitions/ComputerUseRequirements" + }, + { + "type": "null" + } + ] + }, "enforceResidency": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkParams.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkParams.json index 102cfa0299..9d2f834dd8 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkParams.json @@ -77,7 +77,7 @@ "type": "string" } }, - "description": "There are two ways to fork a thread: 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread. 2. By path: load the thread from disk by path and fork it into a new thread.\n\nIf using path, the thread_id param will be ignored.\n\nPrefer using thread_id whenever possible.", + "description": "There are two ways to fork a thread: 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread. 2. By path: load the thread from disk by path and fork it into a new thread.\n\nIf using a non-empty path, the thread_id param will be ignored. Empty string path values are treated as absent.\n\nPrefer using thread_id whenever possible.", "properties": { "approvalPolicy": { "anyOf": [ diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.json index 253ebf4572..027989479a 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.json @@ -963,7 +963,7 @@ "type": "string" } }, - "description": "There are three ways to resume a thread: 1. By thread_id: load the thread from disk by thread_id and resume it. 2. By history: instantiate the thread from memory and resume it. 3. By path: load the thread from disk by path and resume it.\n\nThe precedence is: history > path > thread_id. If using history or path, the thread_id param will be ignored.\n\nPrefer using thread_id whenever possible.", + "description": "There are three ways to resume a thread: 1. By thread_id: load the thread from disk by thread_id and resume it. 2. By history: instantiate the thread from memory and resume it. 3. By path: load the thread from disk by path and resume it.\n\nFor non-running threads, the precedence is: history > non-empty path > thread_id. If using history or a non-empty path for a non-running thread, the thread_id param will be ignored.\n\nIf thread_id identifies a running thread, app-server rejoins that thread and treats a non-empty path as a consistency check against the active rollout path. Empty string path values are treated as absent.\n\nPrefer using thread_id whenever possible.", "properties": { "approvalPolicy": { "anyOf": [ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ComputerUseRequirements.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ComputerUseRequirements.ts new file mode 100644 index 0000000000..7a82e7aec8 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ComputerUseRequirements.ts @@ -0,0 +1,5 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +export type ComputerUseRequirements = { allowLockedComputerUse: boolean | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts index 1b0625e5e1..5d2755d5ac 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ConfigRequirements.ts @@ -3,7 +3,8 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { WebSearchMode } from "../WebSearchMode"; import type { AskForApproval } from "./AskForApproval"; +import type { ComputerUseRequirements } from "./ComputerUseRequirements"; import type { ResidencyRequirement } from "./ResidencyRequirement"; import type { SandboxMode } from "./SandboxMode"; -export type ConfigRequirements = {allowedApprovalPolicies: Array | null, allowedSandboxModes: Array | null, allowedWebSearchModes: Array | null, allowManagedHooksOnly: boolean | null, featureRequirements: { [key in string]?: boolean } | null, enforceResidency: ResidencyRequirement | null}; +export type ConfigRequirements = {allowedApprovalPolicies: Array | null, allowedSandboxModes: Array | null, allowedWebSearchModes: Array | null, allowManagedHooksOnly: boolean | null, computerUse: ComputerUseRequirements | null, featureRequirements: { [key in string]?: boolean } | null, enforceResidency: ResidencyRequirement | null}; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkParams.ts index 6076a4bb14..c5109b2c7d 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkParams.ts @@ -12,7 +12,8 @@ import type { ThreadSource } from "./ThreadSource"; * 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread. * 2. By path: load the thread from disk by path and fork it into a new thread. * - * If using path, the thread_id param will be ignored. + * If using a non-empty path, the thread_id param will be ignored. + * Empty string path values are treated as absent. * * Prefer using thread_id whenever possible. */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadResumeParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadResumeParams.ts index 6d1dbdca4f..0ec8953435 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadResumeParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadResumeParams.ts @@ -13,8 +13,13 @@ import type { SandboxMode } from "./SandboxMode"; * 2. By history: instantiate the thread from memory and resume it. * 3. By path: load the thread from disk by path and resume it. * - * The precedence is: history > path > thread_id. - * If using history or path, the thread_id param will be ignored. + * For non-running threads, the precedence is: history > non-empty path > thread_id. + * If using history or a non-empty path for a non-running thread, the thread_id + * param will be ignored. + * + * If thread_id identifies a running thread, app-server rejoins that thread and + * treats a non-empty path as a consistency check against the active rollout path. + * Empty string path values are treated as absent. * * Prefer using thread_id whenever possible. */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index dc400a6f8e..7c09be0def 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -62,6 +62,7 @@ export type { CommandExecutionRequestApprovalResponse } from "./CommandExecution export type { CommandExecutionSource } from "./CommandExecutionSource"; export type { CommandExecutionStatus } from "./CommandExecutionStatus"; export type { CommandMigration } from "./CommandMigration"; +export type { ComputerUseRequirements } from "./ComputerUseRequirements"; export type { Config } from "./Config"; export type { ConfigBatchWriteParams } from "./ConfigBatchWriteParams"; export type { ConfigEdit } from "./ConfigEdit"; diff --git a/codex-rs/app-server-protocol/src/protocol/serde_helpers.rs b/codex-rs/app-server-protocol/src/protocol/serde_helpers.rs index 0e35ebdba7..6d6747a5cc 100644 --- a/codex-rs/app-server-protocol/src/protocol/serde_helpers.rs +++ b/codex-rs/app-server-protocol/src/protocol/serde_helpers.rs @@ -1,8 +1,18 @@ +use std::path::PathBuf; + use serde::Deserialize; use serde::Deserializer; use serde::Serialize; use serde::Serializer; +pub fn deserialize_empty_path_as_none<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let path = Option::::deserialize(deserializer)?; + Ok(path.filter(|path| !path.as_os_str().is_empty())) +} + pub fn deserialize_double_option<'de, T, D>(deserializer: D) -> Result>, D::Error> where T: Deserialize<'de>, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/config.rs b/codex-rs/app-server-protocol/src/protocol/v2/config.rs index 0b7741c656..25d75b56b4 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/config.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/config.rs @@ -388,6 +388,7 @@ pub struct ConfigRequirements { pub allowed_sandbox_modes: Option>, pub allowed_web_search_modes: Option>, pub allow_managed_hooks_only: Option, + pub computer_use: Option, pub feature_requirements: Option>, #[experimental("configRequirements/read.hooks")] pub hooks: Option, @@ -396,6 +397,13 @@ pub struct ConfigRequirements { pub network: Option, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct ComputerUseRequirements { + pub allow_locked_computer_use: Option, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] diff --git a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs index 0e7161d5e8..2ee583f991 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs @@ -661,6 +661,33 @@ fn permission_profile_selection_uses_id_string() { ); } +#[test] +fn thread_path_params_deserialize_empty_path_as_none() { + let resume: ThreadResumeParams = serde_json::from_value(json!({ + "threadId": "thread-1", + "path": "", + })) + .expect("thread/resume params deserialize"); + assert_eq!(resume.path, None); + + let fork: ThreadForkParams = serde_json::from_value(json!({ + "threadId": "thread-1", + "path": "", + })) + .expect("thread/fork params deserialize"); + assert_eq!(fork.path, None); + + let resume_with_path: ThreadResumeParams = serde_json::from_value(json!({ + "threadId": "thread-1", + "path": "/tmp/resume-thread.jsonl", + })) + .expect("thread/resume params deserialize"); + assert_eq!( + resume_with_path.path, + Some(PathBuf::from("/tmp/resume-thread.jsonl")) + ); +} + #[test] fn fs_get_metadata_response_round_trips_minimal_fields() { let response = FsGetMetadataResponse { @@ -1703,6 +1730,7 @@ fn config_requirements_granular_allowed_approval_policy_is_marked_experimental() allowed_sandbox_modes: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, feature_requirements: None, hooks: None, enforce_residency: None, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/thread.rs b/codex-rs/app-server-protocol/src/protocol/v2/thread.rs index 41994e59b6..059bef05f2 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/thread.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/thread.rs @@ -229,8 +229,13 @@ pub struct ThreadStartResponse { /// 2. By history: instantiate the thread from memory and resume it. /// 3. By path: load the thread from disk by path and resume it. /// -/// The precedence is: history > path > thread_id. -/// If using history or path, the thread_id param will be ignored. +/// For non-running threads, the precedence is: history > non-empty path > thread_id. +/// If using history or a non-empty path for a non-running thread, the thread_id +/// param will be ignored. +/// +/// If thread_id identifies a running thread, app-server rejoins that thread and +/// treats a non-empty path as a consistency check against the active rollout path. +/// Empty string path values are treated as absent. /// /// Prefer using thread_id whenever possible. pub struct ThreadResumeParams { @@ -244,8 +249,14 @@ pub struct ThreadResumeParams { pub history: Option>, /// [UNSTABLE] Specify the rollout path to resume from. - /// If specified, the thread_id param will be ignored. + /// If specified for a non-running thread, the thread_id param will be ignored. + /// If thread_id identifies a running thread, the path must match the active + /// rollout path. #[experimental("thread/resume.path")] + #[serde( + default, + deserialize_with = "crate::protocol::serde_helpers::deserialize_empty_path_as_none" + )] #[ts(optional = nullable)] pub path: Option, @@ -346,7 +357,8 @@ pub struct ThreadResumeResponse { /// 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread. /// 2. By path: load the thread from disk by path and fork it into a new thread. /// -/// If using path, the thread_id param will be ignored. +/// If using a non-empty path, the thread_id param will be ignored. +/// Empty string path values are treated as absent. /// /// Prefer using thread_id whenever possible. pub struct ThreadForkParams { @@ -355,6 +367,10 @@ pub struct ThreadForkParams { /// [UNSTABLE] Specify the rollout path to fork from. /// If specified, the thread_id param will be ignored. #[experimental("thread/fork.path")] + #[serde( + default, + deserialize_with = "crate::protocol::serde_helpers::deserialize_empty_path_as_none" + )] #[ts(optional = nullable)] pub path: Option, diff --git a/codex-rs/app-server-transport/src/lib.rs b/codex-rs/app-server-transport/src/lib.rs index f4eb1fc6a5..8608102f36 100644 --- a/codex-rs/app-server-transport/src/lib.rs +++ b/codex-rs/app-server-transport/src/lib.rs @@ -6,6 +6,7 @@ pub use outgoing_message::OutgoingError; pub use outgoing_message::OutgoingMessage; pub use outgoing_message::OutgoingResponse; pub use outgoing_message::QueuedOutgoingMessage; +pub use transport::AppServerStartupLock; pub use transport::AppServerTransport; pub use transport::AppServerTransportParseError; pub use transport::CHANNEL_CAPACITY; @@ -14,8 +15,11 @@ pub use transport::RemoteControlHandle; pub use transport::RemoteControlStartConfig; pub use transport::RemoteControlUnavailable; pub use transport::TransportEvent; +pub use transport::acquire_app_server_startup_lock; pub use transport::app_server_control_socket_path; +pub use transport::app_server_startup_lock_path; pub use transport::auth; +pub use transport::prepare_control_socket_path; pub use transport::start_control_socket_acceptor; pub use transport::start_remote_control; pub use transport::start_stdio_connection; diff --git a/codex-rs/app-server-transport/src/transport/mod.rs b/codex-rs/app-server-transport/src/transport/mod.rs index a9e806c128..7452f06938 100644 --- a/codex-rs/app-server-transport/src/transport/mod.rs +++ b/codex-rs/app-server-transport/src/transport/mod.rs @@ -35,6 +35,9 @@ pub use remote_control::RemoteControlStartConfig; pub use remote_control::RemoteControlUnavailable; pub use remote_control::start_remote_control; pub use stdio::start_stdio_connection; +pub use unix_socket::AppServerStartupLock; +pub use unix_socket::acquire_app_server_startup_lock; +pub use unix_socket::prepare_control_socket_path; pub use unix_socket::start_control_socket_acceptor; pub use websocket::start_websocket_acceptor; @@ -42,6 +45,7 @@ const OVERLOADED_ERROR_CODE: i64 = -32001; const APP_SERVER_CONTROL_SOCKET_DIR_NAME: &str = "app-server-control"; const APP_SERVER_CONTROL_SOCKET_FILE_NAME: &str = "app-server-control.sock"; +const APP_SERVER_STARTUP_LOCK_FILE_NAME: &str = "app-server-startup.lock"; pub fn app_server_control_socket_path(codex_home: &Path) -> std::io::Result { AbsolutePathBuf::from_absolute_path( @@ -51,6 +55,14 @@ pub fn app_server_control_socket_path(codex_home: &Path) -> std::io::Result std::io::Result { + AbsolutePathBuf::from_absolute_path( + codex_home + .join(APP_SERVER_CONTROL_SOCKET_DIR_NAME) + .join(APP_SERVER_STARTUP_LOCK_FILE_NAME), + ) +} + #[derive(Clone, Debug, Eq, PartialEq)] pub enum AppServerTransport { Stdio, diff --git a/codex-rs/app-server-transport/src/transport/unix_socket.rs b/codex-rs/app-server-transport/src/transport/unix_socket.rs index f75d3fe99a..dd46d88280 100644 --- a/codex-rs/app-server-transport/src/transport/unix_socket.rs +++ b/codex-rs/app-server-transport/src/transport/unix_socket.rs @@ -1,3 +1,4 @@ +use std::fs::OpenOptions; use std::io::ErrorKind; use std::io::Result as IoResult; use std::path::Path; @@ -89,7 +90,7 @@ async fn run_control_socket_acceptor( info!("control socket acceptor shutting down"); } -async fn prepare_control_socket_path(socket_path: &Path) -> IoResult<()> { +pub async fn prepare_control_socket_path(socket_path: &Path) -> IoResult<()> { if let Some(parent) = socket_path.parent() { codex_uds::prepare_private_socket_directory(parent).await?; } @@ -130,6 +131,30 @@ async fn prepare_control_socket_path(socket_path: &Path) -> IoResult<()> { tokio::fs::remove_file(socket_path).await } +pub struct AppServerStartupLock { + _file: std::fs::File, +} + +pub async fn acquire_app_server_startup_lock( + startup_lock_path: AbsolutePathBuf, +) -> IoResult { + if let Some(parent) = startup_lock_path.as_path().parent() { + codex_uds::prepare_private_socket_directory(parent).await?; + } + tokio::task::spawn_blocking(move || { + let file = OpenOptions::new() + .create(true) + .truncate(false) + .read(true) + .write(true) + .open(startup_lock_path.as_path())?; + file.lock()?; + Ok(AppServerStartupLock { _file: file }) + }) + .await + .map_err(|err| std::io::Error::other(format!("startup lock task failed: {err}")))? +} + #[cfg(unix)] async fn set_control_socket_permissions(socket_path: &Path) -> IoResult<()> { use std::os::unix::fs::PermissionsExt; diff --git a/codex-rs/app-server-transport/src/transport/unix_socket_tests.rs b/codex-rs/app-server-transport/src/transport/unix_socket_tests.rs index 0b7dec0a23..ac0b2b00c4 100644 --- a/codex-rs/app-server-transport/src/transport/unix_socket_tests.rs +++ b/codex-rs/app-server-transport/src/transport/unix_socket_tests.rs @@ -1,6 +1,7 @@ use super::AppServerTransport; use super::CHANNEL_CAPACITY; use super::TransportEvent; +use super::acquire_app_server_startup_lock; use super::app_server_control_socket_path; use super::start_control_socket_acceptor; use codex_app_server_protocol::JSONRPCMessage; @@ -140,6 +141,28 @@ async fn control_socket_acceptor_upgrades_and_forwards_websocket_text_messages_a assert_socket_path_removed(socket_path.as_path()); } +#[tokio::test] +async fn app_server_startup_lock_serializes_waiters() { + let temp_dir = tempfile::TempDir::new().expect("temp dir"); + let lock_path = test_startup_lock_path(temp_dir.path()); + let first_lock = acquire_app_server_startup_lock(lock_path.clone()) + .await + .expect("first startup lock should succeed"); + let mut second_lock = tokio::spawn(acquire_app_server_startup_lock(lock_path)); + + assert!( + timeout(Duration::from_millis(100), &mut second_lock) + .await + .is_err() + ); + + drop(first_lock); + second_lock + .await + .expect("second startup lock task should join") + .expect("second startup lock should succeed"); +} + #[cfg(unix)] #[tokio::test] async fn control_socket_file_is_private_after_bind() { @@ -185,6 +208,15 @@ fn test_socket_path(temp_dir: &Path) -> AbsolutePathBuf { .expect("socket path should resolve") } +fn test_startup_lock_path(temp_dir: &Path) -> AbsolutePathBuf { + AbsolutePathBuf::from_absolute_path( + temp_dir + .join("app-server-control") + .join("app-server-startup.lock"), + ) + .expect("startup lock path should resolve") +} + async fn connect_to_socket(socket_path: &Path) -> IoResult { UnixStream::connect(socket_path).await } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 9073282b4e..1b7575141f 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -223,7 +223,7 @@ Example with notification opt-out: - `externalAgentConfig/import` — apply selected external-agent migration items by passing explicit `migrationItems` with `cwd` (`null` for home) and any plugin/session `details` returned by detect. When a request includes migration items, the server emits `externalAgentConfig/import/completed` once after the full import finishes (immediately after the response when everything completed synchronously, or after background imports finish). - `config/value/write` — write a single config key/value to the user's config.toml on disk; dotted paths such as `desktop.someKey` use the same generic write surface. - `config/batchWrite` — apply multiple config edits atomically to the user's config.toml on disk, with optional `reloadUserConfig: true` to hot-reload loaded threads, including multiple `desktop.*` edits. -- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), lifecycle hook lockdown (`allowManagedHooksOnly`), pinned feature values (`featureRequirements`), managed lifecycle hooks (`hooks`), `enforceResidency`, and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`. +- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), lifecycle hook lockdown (`allowManagedHooksOnly`), computer use policy (`computerUse`), pinned feature values (`featureRequirements`), managed lifecycle hooks (`hooks`), `enforceResidency`, and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`. ### Example: Start or resume a thread diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index b62b2bdc40..ef4106aa14 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -31,7 +31,10 @@ use crate::transport::ConnectionState; use crate::transport::OutboundConnectionState; use crate::transport::RemoteControlStartConfig; use crate::transport::TransportEvent; +use crate::transport::acquire_app_server_startup_lock; +use crate::transport::app_server_startup_lock_path; use crate::transport::auth::policy_from_settings; +use crate::transport::prepare_control_socket_path; use crate::transport::route_outgoing_envelope; use crate::transport::start_control_socket_acceptor; use crate::transport::start_remote_control; @@ -445,9 +448,9 @@ pub async fn run_main_with_transport_options( arg0_paths.codex_linux_sandbox_exe.clone(), )?; let environment_manager = if loader_overrides.ignore_user_config { - EnvironmentManager::from_env(local_runtime_paths).await + EnvironmentManager::from_env(Some(local_runtime_paths)).await } else { - EnvironmentManager::from_codex_home(codex_home.clone(), local_runtime_paths).await + EnvironmentManager::from_codex_home(codex_home.clone(), Some(local_runtime_paths)).await } .map(Arc::new) .map_err(std::io::Error::other)?; @@ -516,6 +519,15 @@ pub async fn run_main_with_transport_options( })?; codex_core::otel_init::record_process_start(otel.as_ref(), OTEL_SERVICE_NAME); codex_core::otel_init::install_sqlite_telemetry(otel.as_ref(), OTEL_SERVICE_NAME); + let unix_socket_startup_lock = match &transport { + AppServerTransport::UnixSocket { socket_path } => { + let startup_lock_path = app_server_startup_lock_path(&codex_home)?; + let startup_lock = acquire_app_server_startup_lock(startup_lock_path).await?; + prepare_control_socket_path(socket_path.as_path()).await?; + Some(startup_lock) + } + _ => None, + }; let state_db = match rollout_state_db::try_init(&config).await { Ok(state_db) => Some(state_db), Err(err) => { @@ -682,6 +694,7 @@ pub async fn run_main_with_transport_options( } AppServerTransport::Off => {} } + drop(unix_socket_startup_lock); let auth_manager = AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false).await; diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 4e2c6f38cc..e37c9afd22 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -301,6 +301,7 @@ impl MessageProcessor { // affect per-thread behavior, but they must not move newly started, // resumed, or forked threads to a different persistence backend/root. let thread_store = codex_core::thread_store_from_config(config.as_ref(), state_db.clone()); + let environment_manager_for_requests = Arc::clone(&environment_manager); let thread_manager = Arc::new_cyclic(|thread_manager| { ThreadManager::new( config.as_ref(), @@ -355,8 +356,12 @@ impl MessageProcessor { Arc::clone(&config), outgoing.clone(), config_manager.clone(), + Arc::clone(&environment_manager_for_requests), + ); + let process_exec_processor = ProcessExecRequestProcessor::new( + outgoing.clone(), + Arc::clone(&environment_manager_for_requests), ); - let process_exec_processor = ProcessExecRequestProcessor::new(outgoing.clone()); let feedback_processor = FeedbackRequestProcessor::new( auth_manager.clone(), Arc::clone(&thread_manager), @@ -443,7 +448,6 @@ impl MessageProcessor { Some(on_effective_plugins_changed), ); } - let fs_watch_manager = FsWatchManager::new(outgoing.clone()); let config_processor = ConfigRequestProcessor::new( outgoing.clone(), config_manager.clone(), @@ -462,11 +466,8 @@ impl MessageProcessor { let environment_processor = EnvironmentRequestProcessor::new(thread_manager.environment_manager()); let fs_processor = FsRequestProcessor::new( - thread_manager - .environment_manager() - .local_environment() - .get_filesystem(), - fs_watch_manager, + Arc::clone(&environment_manager_for_requests), + FsWatchManager::new(outgoing.clone()), ); let windows_sandbox_processor = WindowsSandboxRequestProcessor::new( outgoing.clone(), diff --git a/codex-rs/app-server/src/request_processors/command_exec_processor.rs b/codex-rs/app-server/src/request_processors/command_exec_processor.rs index 1219eb17f9..930cc6d18f 100644 --- a/codex-rs/app-server/src/request_processors/command_exec_processor.rs +++ b/codex-rs/app-server/src/request_processors/command_exec_processor.rs @@ -6,6 +6,7 @@ pub(crate) struct CommandExecRequestProcessor { config: Arc, outgoing: Arc, config_manager: ConfigManager, + environment_manager: Arc, command_exec_manager: CommandExecManager, } @@ -15,12 +16,14 @@ impl CommandExecRequestProcessor { config: Arc, outgoing: Arc, config_manager: ConfigManager, + environment_manager: Arc, ) -> Self { Self { arg0_paths, config, outgoing, config_manager, + environment_manager, command_exec_manager: CommandExecManager::default(), } } @@ -30,6 +33,7 @@ impl CommandExecRequestProcessor { request_id: &ConnectionRequestId, params: CommandExecParams, ) -> Result, JSONRPCErrorError> { + self.require_local_environment()?; self.exec_one_off_command(request_id, params) .await .map(|()| None) @@ -74,6 +78,14 @@ impl CommandExecRequestProcessor { .await; } + fn require_local_environment(&self) -> Result<(), JSONRPCErrorError> { + self.environment_manager + .try_local_environment() + .is_some() + .then_some(()) + .ok_or_else(|| internal_error("local environment is not configured")) + } + async fn exec_one_off_command( &self, request_id: &ConnectionRequestId, diff --git a/codex-rs/app-server/src/request_processors/config_processor.rs b/codex-rs/app-server/src/request_processors/config_processor.rs index 486970f929..860385a1ff 100644 --- a/codex-rs/app-server/src/request_processors/config_processor.rs +++ b/codex-rs/app-server/src/request_processors/config_processor.rs @@ -9,6 +9,7 @@ use crate::outgoing_message::OutgoingMessageSender; use codex_analytics::AnalyticsEventsClient; use codex_app_server_protocol::AppListUpdatedNotification; use codex_app_server_protocol::ClientResponsePayload; +use codex_app_server_protocol::ComputerUseRequirements; use codex_app_server_protocol::ConfigBatchWriteParams; use codex_app_server_protocol::ConfigReadParams; use codex_app_server_protocol::ConfigReadResponse; @@ -429,6 +430,9 @@ fn map_requirements_toml_to_api(requirements: ConfigRequirementsToml) -> ConfigR normalized }), allow_managed_hooks_only: requirements.allow_managed_hooks_only, + computer_use: requirements + .computer_use + .map(map_computer_use_requirements_to_api), feature_requirements: requirements .feature_requirements .map(|requirements| requirements.entries), @@ -440,6 +444,14 @@ fn map_requirements_toml_to_api(requirements: ConfigRequirementsToml) -> ConfigR } } +fn map_computer_use_requirements_to_api( + computer_use: codex_config::ComputerUseRequirementsToml, +) -> ComputerUseRequirements { + ComputerUseRequirements { + allow_locked_computer_use: computer_use.allow_locked_computer_use, + } +} + fn map_hooks_requirements_to_api(hooks: ManagedHooksRequirementsToml) -> ManagedHooksRequirements { let ManagedHooksRequirementsToml { managed_dir, @@ -617,6 +629,7 @@ fn config_write_error(code: ConfigWriteErrorCode, message: impl Into) -> #[cfg(test)] mod tests { use super::map_requirements_toml_to_api; + use codex_config::ComputerUseRequirementsToml; use codex_config::ConfigRequirementsToml; use pretty_assertions::assert_eq; @@ -630,4 +643,21 @@ mod tests { assert_eq!(mapped.allow_managed_hooks_only, Some(true)); assert_eq!(mapped.hooks, None); } + + #[test] + fn requirements_api_includes_computer_use_requirements() { + let mapped = map_requirements_toml_to_api(ConfigRequirementsToml { + computer_use: Some(ComputerUseRequirementsToml { + allow_locked_computer_use: Some(false), + }), + ..ConfigRequirementsToml::default() + }); + + assert_eq!( + mapped + .computer_use + .and_then(|requirements| requirements.allow_locked_computer_use), + Some(false) + ); + } } diff --git a/codex-rs/app-server/src/request_processors/fs_processor.rs b/codex-rs/app-server/src/request_processors/fs_processor.rs index 01b9b20bfd..99a8620b4f 100644 --- a/codex-rs/app-server/src/request_processors/fs_processor.rs +++ b/codex-rs/app-server/src/request_processors/fs_processor.rs @@ -26,6 +26,7 @@ use codex_app_server_protocol::FsWriteFileResponse; use codex_app_server_protocol::JSONRPCErrorError; use codex_exec_server::CopyOptions; use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::EnvironmentManager; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::RemoveOptions; use std::io; @@ -33,21 +34,28 @@ use std::sync::Arc; #[derive(Clone)] pub(crate) struct FsRequestProcessor { - file_system: Arc, + environment_manager: Arc, fs_watch_manager: FsWatchManager, } impl FsRequestProcessor { pub(crate) fn new( - file_system: Arc, + environment_manager: Arc, fs_watch_manager: FsWatchManager, ) -> Self { Self { - file_system, + environment_manager, fs_watch_manager, } } + fn file_system(&self) -> Result, JSONRPCErrorError> { + self.environment_manager + .try_local_environment() + .map(|environment| environment.get_filesystem()) + .ok_or_else(|| internal_error("local filesystem is not configured")) + } + pub(crate) async fn connection_closed(&self, connection_id: ConnectionId) { self.fs_watch_manager.connection_closed(connection_id).await; } @@ -57,7 +65,7 @@ impl FsRequestProcessor { params: FsReadFileParams, ) -> Result { let bytes = self - .file_system + .file_system()? .read_file(¶ms.path, /*sandbox*/ None) .await .map_err(map_fs_error)?; @@ -75,7 +83,7 @@ impl FsRequestProcessor { "fs/writeFile requires valid base64 dataBase64: {err}" )) })?; - self.file_system + self.file_system()? .write_file(¶ms.path, bytes, /*sandbox*/ None) .await .map_err(map_fs_error)?; @@ -86,7 +94,7 @@ impl FsRequestProcessor { &self, params: FsCreateDirectoryParams, ) -> Result { - self.file_system + self.file_system()? .create_directory( ¶ms.path, CreateDirectoryOptions { @@ -104,7 +112,7 @@ impl FsRequestProcessor { params: FsGetMetadataParams, ) -> Result { let metadata = self - .file_system + .file_system()? .get_metadata(¶ms.path, /*sandbox*/ None) .await .map_err(map_fs_error)?; @@ -122,7 +130,7 @@ impl FsRequestProcessor { params: FsReadDirectoryParams, ) -> Result { let entries = self - .file_system + .file_system()? .read_directory(¶ms.path, /*sandbox*/ None) .await .map_err(map_fs_error)?; @@ -142,7 +150,7 @@ impl FsRequestProcessor { &self, params: FsRemoveParams, ) -> Result { - self.file_system + self.file_system()? .remove( ¶ms.path, RemoveOptions { @@ -160,7 +168,7 @@ impl FsRequestProcessor { &self, params: FsCopyParams, ) -> Result { - self.file_system + self.file_system()? .copy( ¶ms.source_path, ¶ms.destination_path, @@ -179,6 +187,7 @@ impl FsRequestProcessor { connection_id: ConnectionId, params: FsWatchParams, ) -> Result { + self.file_system()?; self.fs_watch_manager.watch(connection_id, params).await } @@ -187,6 +196,7 @@ impl FsRequestProcessor { connection_id: ConnectionId, params: FsUnwatchParams, ) -> Result { + self.file_system()?; self.fs_watch_manager.unwatch(connection_id, params).await } } diff --git a/codex-rs/app-server/src/request_processors/mcp_processor.rs b/codex-rs/app-server/src/request_processors/mcp_processor.rs index 548bb5be87..4bb9431f42 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -205,17 +205,14 @@ impl McpRequestProcessor { .await; let auth = self.auth_manager.auth().await; let environment_manager = self.thread_manager.environment_manager(); - let runtime_environment = match environment_manager.default_environment() { - Some(environment) => { - // Status listing has no turn cwd. This fallback is used only - // by executor-backed stdio MCPs whose config omits `cwd`. - McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf()) - } - None => McpRuntimeEnvironment::new( - environment_manager.local_environment(), - config.cwd.to_path_buf(), - ), - }; + // Status listing has no turn cwd. Prefer the configured default env, + // then configured local if present; do not manufacture a hidden local + // env in no-local modes. + let runtime_environment = McpRuntimeEnvironment::new( + environment_manager.default_or_local_environment(), + environment_manager.try_local_environment(), + config.cwd.to_path_buf(), + ); tokio::spawn(async move { Self::list_mcp_server_status_task( @@ -369,15 +366,15 @@ impl McpRequestProcessor { .to_mcp_config(self.thread_manager.plugins_manager().as_ref()) .await; let auth = self.auth_manager.auth().await; - let runtime_environment = { - let environment_manager = self.thread_manager.environment_manager(); - let environment = environment_manager - .default_environment() - .unwrap_or_else(|| environment_manager.local_environment()); - // Resource reads without a thread have no turn cwd. This fallback - // is used only by executor-backed stdio MCPs whose config omits `cwd`. - McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf()) - }; + let environment_manager = self.thread_manager.environment_manager(); + // Resource reads without a thread have no turn cwd. Prefer the + // configured default env, then configured local if present; do not + // manufacture a hidden local env in no-local modes. + let runtime_environment = McpRuntimeEnvironment::new( + environment_manager.default_or_local_environment(), + environment_manager.try_local_environment(), + config.cwd.to_path_buf(), + ); let request_id = request_id.clone(); tokio::spawn(async move { diff --git a/codex-rs/app-server/src/request_processors/process_exec_processor.rs b/codex-rs/app-server/src/request_processors/process_exec_processor.rs index 5742d0e4d5..0b84c7f7b9 100644 --- a/codex-rs/app-server/src/request_processors/process_exec_processor.rs +++ b/codex-rs/app-server/src/request_processors/process_exec_processor.rs @@ -23,6 +23,7 @@ use codex_app_server_protocol::ServerNotification; use codex_core::exec::ExecExpiration; use codex_core::exec::ExecExpirationOutcome; use codex_core::exec::IO_DRAIN_TIMEOUT_MS; +use codex_exec_server::EnvironmentManager; use codex_protocol::exec_output::bytes_to_string_smart; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_pty::DEFAULT_OUTPUT_BYTES_CAP; @@ -48,13 +49,18 @@ const OUTPUT_CHUNK_SIZE_HINT: usize = 64 * 1024; #[derive(Clone)] pub(crate) struct ProcessExecRequestProcessor { outgoing: Arc, + environment_manager: Arc, process_exec_manager: ProcessExecManager, } impl ProcessExecRequestProcessor { - pub(crate) fn new(outgoing: Arc) -> Self { + pub(crate) fn new( + outgoing: Arc, + environment_manager: Arc, + ) -> Self { Self { outgoing, + environment_manager, process_exec_manager: ProcessExecManager::default(), } } @@ -64,6 +70,7 @@ impl ProcessExecRequestProcessor { request_id: ConnectionRequestId, params: ProcessSpawnParams, ) -> Result<(), JSONRPCErrorError> { + self.require_local_environment()?; let ProcessSpawnParams { command, process_handle, @@ -173,6 +180,14 @@ impl ProcessExecRequestProcessor { .connection_closed(connection_id) .await; } + + fn require_local_environment(&self) -> Result<(), JSONRPCErrorError> { + self.environment_manager + .try_local_environment() + .is_some() + .then_some(()) + .ok_or_else(|| internal_error("local environment is not configured")) + } } #[derive(Clone, Default)] diff --git a/codex-rs/app-server/src/request_processors/thread_processor.rs b/codex-rs/app-server/src/request_processors/thread_processor.rs index dd2dd6a57e..6b6e1729a5 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -1738,6 +1738,16 @@ impl ThreadRequestProcessor { if command.is_empty() { return Err(invalid_request("command must not be empty")); } + // `thread/shellCommand` is app-server's local-host shell escape hatch, + // not the normal turn-selected shell tool path. + if self + .thread_manager + .environment_manager() + .try_local_environment() + .is_none() + { + return Err(internal_error("local environment is not configured")); + } let (_, thread) = self.load_thread(&thread_id).await?; self.submit_core_op( @@ -2629,31 +2639,6 @@ impl ThreadRequestProcessor { ))); } None - } else if params.path.is_some() { - let source_thread = self - .read_stored_thread_for_resume( - ¶ms.thread_id, - params.path.as_ref(), - /*include_history*/ true, - ) - .await?; - let existing_thread_id = source_thread.thread_id; - if let Ok(existing_thread) = self.thread_manager.get_thread(existing_thread_id).await { - if let (Some(requested_path), Some(active_path)) = ( - params.path.as_ref(), - existing_thread.rollout_path().as_ref(), - ) && requested_path != active_path - { - return Err(invalid_request(format!( - "cannot resume running thread {existing_thread_id} with stale path: requested `{}`, active `{}`", - requested_path.display(), - active_path.display() - ))); - } - Some((existing_thread_id, existing_thread, source_thread)) - } else { - None - } } else if let Ok(existing_thread_id) = ThreadId::from_string(¶ms.thread_id) && let Ok(existing_thread) = self.thread_manager.get_thread(existing_thread_id).await { @@ -2676,6 +2661,19 @@ impl ThreadRequestProcessor { }; if let Some((existing_thread_id, existing_thread, source_thread)) = running_thread { + let existing_thread_rollout_path = existing_thread.rollout_path(); + let active_path = existing_thread_rollout_path + .as_ref() + .or(source_thread.rollout_path.as_ref()); + if let (Some(requested_path), Some(active_path)) = (params.path.as_ref(), active_path) + && requested_path != active_path + { + return Err(invalid_request(format!( + "cannot resume running thread {existing_thread_id} with stale path: requested `{}`, active `{}`", + requested_path.display(), + active_path.display() + ))); + } let redact_resume_payloads = should_redact_thread_resume_payloads(app_server_client_name.as_deref()); let history_items = source_thread diff --git a/codex-rs/app-server/src/transport.rs b/codex-rs/app-server/src/transport.rs index f7222bd28f..2be5b55414 100644 --- a/codex-rs/app-server/src/transport.rs +++ b/codex-rs/app-server/src/transport.rs @@ -22,8 +22,11 @@ pub(crate) use codex_app_server_transport::RemoteControlHandle; pub(crate) use codex_app_server_transport::RemoteControlStartConfig; pub(crate) use codex_app_server_transport::RemoteControlUnavailable; pub(crate) use codex_app_server_transport::TransportEvent; +pub(crate) use codex_app_server_transport::acquire_app_server_startup_lock; pub use codex_app_server_transport::app_server_control_socket_path; +pub(crate) use codex_app_server_transport::app_server_startup_lock_path; pub use codex_app_server_transport::auth; +pub(crate) use codex_app_server_transport::prepare_control_socket_path; pub(crate) use codex_app_server_transport::start_control_socket_acceptor; pub(crate) use codex_app_server_transport::start_remote_control; pub(crate) use codex_app_server_transport::start_stdio_connection; diff --git a/codex-rs/app-server/tests/suite/v2/command_exec.rs b/codex-rs/app-server/tests/suite/v2/command_exec.rs index b4038299cf..9757767c9f 100644 --- a/codex-rs/app-server/tests/suite/v2/command_exec.rs +++ b/codex-rs/app-server/tests/suite/v2/command_exec.rs @@ -17,6 +17,7 @@ use codex_app_server_protocol::JSONRPCMessage; use codex_app_server_protocol::JSONRPCNotification; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::SandboxPolicy; +use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR; use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_READ_ONLY; use pretty_assertions::assert_eq; use std::collections::HashMap; @@ -400,6 +401,45 @@ async fn command_exec_permission_profile_project_roots_use_command_cwd() -> Resu Ok(()) } +#[tokio::test] +async fn command_exec_returns_error_when_local_environment_is_disabled() -> 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 mut mcp = McpProcess::new_with_env( + codex_home.path(), + &[(CODEX_EXEC_SERVER_URL_ENV_VAR, Some("none"))], + ) + .await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let command_request_id = mcp + .send_command_exec_request(CommandExecParams { + command: vec!["sh".to_string(), "-lc".to_string(), "true".to_string()], + process_id: None, + tty: false, + stream_stdin: false, + stream_stdout_stderr: false, + output_bytes_cap: None, + disable_output_cap: false, + disable_timeout: false, + timeout_ms: None, + cwd: None, + env: None, + size: None, + sandbox_policy: None, + permission_profile: None, + }) + .await?; + + let error = mcp + .read_stream_until_error_message(RequestId::Integer(command_request_id)) + .await?; + assert_eq!(error.error.message, "local environment is not configured"); + + Ok(()) +} + #[tokio::test] async fn command_exec_rejects_sandbox_policy_with_permission_profile() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; diff --git a/codex-rs/app-server/tests/suite/v2/fs.rs b/codex-rs/app-server/tests/suite/v2/fs.rs index a780a51e0b..3ca511a482 100644 --- a/codex-rs/app-server/tests/suite/v2/fs.rs +++ b/codex-rs/app-server/tests/suite/v2/fs.rs @@ -14,6 +14,7 @@ use codex_app_server_protocol::FsWatchResponse; use codex_app_server_protocol::FsWriteFileParams; use codex_app_server_protocol::JSONRPCNotification; use codex_app_server_protocol::RequestId; +use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use serde_json::json; @@ -119,6 +120,28 @@ async fn fs_get_metadata_returns_only_used_fields() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn fs_methods_return_error_when_local_environment_is_disabled() -> Result<()> { + let codex_home = TempDir::new()?; + let absolute_file = codex_home.path().join("absolute.txt"); + + let mut mcp = McpProcess::new_with_env( + codex_home.path(), + &[(CODEX_EXEC_SERVER_URL_ENV_VAR, Some("none"))], + ) + .await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let read_id = mcp + .send_fs_read_file_request(codex_app_server_protocol::FsReadFileParams { + path: absolute_path(absolute_file), + }) + .await?; + expect_error_message(&mut mcp, read_id, "local filesystem is not configured").await?; + + Ok(()) +} + #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn fs_get_metadata_reports_symlink() -> Result<()> { diff --git a/codex-rs/app-server/tests/suite/v2/process_exec.rs b/codex-rs/app-server/tests/suite/v2/process_exec.rs index 5dd3e84b4c..d273257e6e 100644 --- a/codex-rs/app-server/tests/suite/v2/process_exec.rs +++ b/codex-rs/app-server/tests/suite/v2/process_exec.rs @@ -6,6 +6,7 @@ use codex_app_server_protocol::ProcessExitedNotification; use codex_app_server_protocol::ProcessKillParams; use codex_app_server_protocol::ProcessSpawnParams; use codex_app_server_protocol::RequestId; +use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::collections::HashMap; @@ -102,6 +103,33 @@ async fn process_spawn_returns_before_exit_and_emits_exit_notification() -> Resu Ok(()) } +#[tokio::test] +async fn process_spawn_returns_error_when_local_environment_is_disabled() -> Result<()> { + let codex_home = TempDir::new()?; + let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; + create_config_toml(codex_home.path(), &server.uri(), "never")?; + let mut mcp = McpProcess::new_with_env( + codex_home.path(), + &[(CODEX_EXEC_SERVER_URL_ENV_VAR, Some("none"))], + ) + .await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let process_request_id = mcp + .send_process_spawn_request(process_spawn_params( + "disabled-process".to_string(), + codex_home.path(), + vec!["sh".to_string(), "-lc".to_string(), "true".to_string()], + )?) + .await?; + let error = mcp + .read_stream_until_error_message(RequestId::Integer(process_request_id)) + .await?; + assert_eq!(error.error.message, "local environment is not configured"); + + Ok(()) +} + #[tokio::test] async fn process_spawn_reports_buffered_output_cap_reached() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/app-server/tests/suite/v2/thread_fork.rs b/codex-rs/app-server/tests/suite/v2/thread_fork.rs index 3eb262bd2b..cc89dd71cb 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_fork.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_fork.rs @@ -454,6 +454,46 @@ async fn thread_fork_rejects_unmaterialized_thread() -> Result<()> { Ok(()) } +#[tokio::test] +async fn thread_fork_with_empty_path_uses_thread_id() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let conversation_id = create_fake_rollout( + codex_home.path(), + "2025-01-05T12-00-00", + "2025-01-05T12:00:00Z", + "Saved user message", + Some("mock_provider"), + /*git_info*/ None, + )?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let fork_id = mcp + .send_thread_fork_request(ThreadForkParams { + thread_id: conversation_id.clone(), + path: Some(std::path::PathBuf::new()), + thread_source: Some(ThreadSource::User), + ..Default::default() + }) + .await?; + let fork_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(fork_id)), + ) + .await??; + let ThreadForkResponse { thread, .. } = to_response::(fork_resp)?; + + assert_eq!( + thread.forked_from_id.as_deref(), + Some(conversation_id.as_str()) + ); + Ok(()) +} + #[tokio::test] async fn thread_fork_surfaces_cloud_requirements_load_errors() -> Result<()> { let server = MockServer::start().await; diff --git a/codex-rs/app-server/tests/suite/v2/thread_resume.rs b/codex-rs/app-server/tests/suite/v2/thread_resume.rs index 8bcad54efe..036f2368a0 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_resume.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_resume.rs @@ -184,6 +184,70 @@ async fn thread_resume_rejects_unmaterialized_thread() -> Result<()> { Ok(()) } +#[tokio::test] +async fn thread_resume_with_empty_path_uses_running_thread_id() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let start_id = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("gpt-5.4".to_string()), + ..Default::default() + }) + .await?; + let start_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(start_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + + let turn_id = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![UserInput::Text { + text: "materialize rollout".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_id)), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + + let resume_id = mcp + .send_thread_resume_request(ThreadResumeParams { + thread_id: thread.id.clone(), + path: Some(PathBuf::new()), + exclude_turns: true, + ..Default::default() + }) + .await?; + let resume_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(resume_id)), + ) + .await??; + let ThreadResumeResponse { + thread: resumed, .. + } = to_response::(resume_resp)?; + + assert_eq!(resumed.id, thread.id); + Ok(()) +} + #[tokio::test] async fn turn_start_updates_runtime_workspace_roots_for_loaded_thread() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; @@ -2143,7 +2207,7 @@ async fn thread_resume_rejects_history_when_thread_is_running() -> Result<()> { } #[tokio::test] -async fn thread_resume_uses_path_over_thread_id_when_thread_is_running() -> Result<()> { +async fn thread_resume_rejects_mismatched_path_for_running_thread_id() -> Result<()> { let server = responses::start_mock_server().await; let first_body = responses::sse(vec![ responses::ev_response_created("resp-1"), @@ -2223,7 +2287,6 @@ async fn thread_resume_uses_path_over_thread_id_when_thread_is_running() -> Resu ) .await??; - let other_thread_id = ThreadId::new().to_string(); let stale_path = rollout_path(codex_home.path(), "2025-01-01T00-00-00", &thread_id); std::fs::create_dir_all(stale_path.parent().expect("stale path parent"))?; let thread_uuid = Uuid::parse_str(&thread_id)?; @@ -2255,7 +2318,7 @@ async fn thread_resume_uses_path_over_thread_id_when_thread_is_running() -> Resu let stale_resume_id = primary .send_thread_resume_request(ThreadResumeParams { - thread_id: other_thread_id.clone(), + thread_id: thread_id.clone(), path: Some(stale_path), ..Default::default() }) @@ -2271,23 +2334,6 @@ async fn thread_resume_uses_path_over_thread_id_when_thread_is_running() -> Resu stale_resume_err.error.message ); - let resume_by_path_id = primary - .send_thread_resume_request(ThreadResumeParams { - thread_id: other_thread_id.clone(), - path: thread.path, - ..Default::default() - }) - .await?; - let resume_by_path_resp: JSONRPCResponse = timeout( - DEFAULT_READ_TIMEOUT, - primary.read_stream_until_response_message(RequestId::Integer(resume_by_path_id)), - ) - .await??; - let ThreadResumeResponse { - thread: resumed, .. - } = to_response::(resume_by_path_resp)?; - assert_eq!(resumed.id, thread_id); - primary .interrupt_turn_and_wait_for_aborted(thread_id, running_turn.id, DEFAULT_READ_TIMEOUT) .await?; @@ -2984,53 +3030,22 @@ async fn thread_resume_surfaces_cloud_requirements_load_errors() -> Result<()> { } #[tokio::test] -async fn thread_resume_uses_path_over_invalid_thread_id() -> Result<()> { +async fn thread_resume_uses_path_over_non_running_thread_id() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; create_config_toml(codex_home.path(), &server.uri())?; - let mut mcp = McpProcess::new(codex_home.path()).await?; - timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + let RestartedThreadFixture { + mut mcp, + thread_id, + rollout_file_path, + .. + } = start_materialized_thread_and_restart(codex_home.path(), "materialize").await?; - let start_id = mcp - .send_thread_start_request(ThreadStartParams { - model: Some("gpt-5.4".to_string()), - ..Default::default() - }) - .await?; - let start_resp: JSONRPCResponse = timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(start_id)), - ) - .await??; - let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; - - let turn_id = mcp - .send_turn_start_request(TurnStartParams { - thread_id: thread.id.clone(), - input: vec![UserInput::Text { - text: "materialize".to_string(), - text_elements: Vec::new(), - }], - ..Default::default() - }) - .await?; - timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(turn_id)), - ) - .await??; - timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_notification_message("turn/completed"), - ) - .await??; - - let thread_path = thread.path.clone().expect("thread path"); let resume_id = mcp .send_thread_resume_request(ThreadResumeParams { - thread_id: "not-a-valid-thread-id".to_string(), - path: Some(thread_path.to_path_buf()), + thread_id: ThreadId::new().to_string(), + path: Some(rollout_file_path), ..Default::default() }) .await?; @@ -3043,7 +3058,7 @@ async fn thread_resume_uses_path_over_invalid_thread_id() -> Result<()> { let ThreadResumeResponse { thread: resumed, .. } = to_response::(resume_resp)?; - assert_eq!(resumed.id, thread.id); + assert_eq!(resumed.id, thread_id); Ok(()) } diff --git a/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs b/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs index b7cfba2f95..4ab677aca1 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs @@ -32,6 +32,7 @@ use codex_app_server_protocol::TurnStartParams; use codex_app_server_protocol::TurnStartResponse; use codex_app_server_protocol::UserInput as V2UserInput; use codex_core::shell::default_user_shell; +use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR; use codex_features::FEATURES; use codex_features::Feature; use pretty_assertions::assert_eq; @@ -182,6 +183,49 @@ async fn thread_shell_command_history_responses_exclude_persisted_command_execut Ok(()) } +#[tokio::test] +async fn thread_shell_command_returns_error_when_local_environment_is_disabled() -> Result<()> { + let tmp = TempDir::new()?; + let codex_home = tmp.path().join("codex_home"); + std::fs::create_dir(&codex_home)?; + let server = create_mock_responses_server_sequence(vec![]).await; + create_config_toml( + codex_home.as_path(), + &server.uri(), + "never", + &BTreeMap::default(), + )?; + + let mut mcp = McpProcess::new_with_env( + codex_home.as_path(), + &[(CODEX_EXEC_SERVER_URL_ENV_VAR, Some("none"))], + ) + .await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let start_id = mcp + .send_thread_start_request(ThreadStartParams::default()) + .await?; + let start_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(start_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + let shell_id = mcp + .send_thread_shell_command_request(ThreadShellCommandParams { + thread_id: thread.id, + command: "pwd".to_string(), + }) + .await?; + let error = mcp + .read_stream_until_error_message(RequestId::Integer(shell_id)) + .await?; + assert_eq!(error.error.message, "local environment is not configured"); + + Ok(()) +} + #[tokio::test] async fn thread_shell_command_uses_existing_active_turn() -> Result<()> { let tmp = TempDir::new()?; diff --git a/codex-rs/cloud-requirements/src/lib.rs b/codex-rs/cloud-requirements/src/lib.rs index 8812762cbe..885e907ef8 100644 --- a/codex-rs/cloud-requirements/src/lib.rs +++ b/codex-rs/cloud-requirements/src/lib.rs @@ -1205,6 +1205,7 @@ mod tests { remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -1288,6 +1289,7 @@ mod tests { remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -1322,6 +1324,7 @@ mod tests { remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -1373,6 +1376,7 @@ mod tests { remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -1524,6 +1528,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -1605,6 +1610,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -1684,6 +1690,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -1891,6 +1898,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -1932,6 +1940,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -1993,6 +2002,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -2050,6 +2060,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -2109,6 +2120,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -2169,6 +2181,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -2229,6 +2242,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -2319,6 +2333,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, @@ -2351,6 +2366,7 @@ command = "sample-mcp" remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 50b1bf6c12..952bc8207b 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -847,6 +847,112 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() { assert_eq!(tool.server_origin.as_deref(), Some("https://docs.example")); } +#[tokio::test] +async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { + let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); + let (tx_event, rx_event) = async_channel::unbounded(); + drop(rx_event); + let codex_home = tempdir().expect("tempdir"); + let mcp_servers = HashMap::from([ + ( + "stdio".to_string(), + EffectiveMcpServer::configured(McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: "echo".to_string(), + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + experimental_environment: None, + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + }), + ), + ( + "http".to_string(), + EffectiveMcpServer::configured(McpServerConfig { + transport: McpServerTransportConfig::StreamableHttp { + url: "http://127.0.0.1:1".to_string(), + bearer_token_env_var: None, + http_headers: None, + env_http_headers: None, + }, + experimental_environment: None, + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + }), + ), + ]); + + let (manager, cancel_token) = McpConnectionManager::new( + &mcp_servers, + OAuthCredentialsStoreMode::default(), + HashMap::new(), + &approval_policy, + String::new(), + tx_event, + PermissionProfile::default(), + McpRuntimeEnvironment::new( + /*environment*/ None, + /*local_environment*/ None, + PathBuf::from("/tmp"), + ), + codex_home.path().to_path_buf(), + CodexAppsToolsCacheKey { + account_id: None, + chatgpt_user_id: None, + is_workspace_account: false, + }, + /*host_owned_codex_apps_enabled*/ false, + ElicitationCapability::default(), + ToolPluginProvenance::default(), + /*auth*/ None, + /*elicitation_reviewer*/ None, + ) + .await; + + assert!(manager.clients.contains_key("stdio")); + assert!(manager.clients.contains_key("http")); + assert!( + !manager + .wait_for_server_ready("stdio", Duration::from_millis(10)) + .await + ); + let failures = manager + .required_startup_failures(&["stdio".to_string()]) + .await; + assert_eq!(failures.len(), 1); + assert_eq!(failures[0].server, "stdio"); + assert_eq!( + failures[0].error, + "local stdio MCP server `stdio` requires a local environment" + ); + cancel_token.cancel(); +} + #[test] fn elicitation_capability_uses_2025_06_18_shape_for_form_only_support() { let capability = Some(ElicitationCapability::default()); diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index fcc9d1c86e..687576dfac 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -566,6 +566,9 @@ async fn make_rmcp_client( let config = match server.launch() { McpServerLaunch::Configured(config) => config.as_ref().clone(), }; + if let Some(reason) = runtime_environment.startup_unavailable_reason(server_name, &config) { + return Err(StartupOutcomeError::from(anyhow!(reason))); + } let McpServerConfig { transport, experimental_environment, @@ -573,14 +576,7 @@ async fn make_rmcp_client( } = config; let remote_environment = match experimental_environment.as_deref() { None | Some("local") => false, - Some("remote") => { - if !runtime_environment.environment().is_remote() { - return Err(StartupOutcomeError::from(anyhow!( - "remote MCP server `{server_name}` requires a remote environment" - ))); - } - true - } + Some("remote") => true, Some(environment) => { return Err(StartupOutcomeError::from(anyhow!( "unsupported experimental_environment `{environment}` for MCP server `{server_name}`" @@ -604,8 +600,15 @@ async fn make_rmcp_client( .collect::>() }); let launcher = if remote_environment { + // Preflight should reject this first, but keep client startup + // defensive if optional runtime placement is mis-threaded. + let Some(environment) = runtime_environment.environment() else { + return Err(StartupOutcomeError::from(anyhow!( + "remote MCP server requires a runtime environment" + ))); + }; Arc::new(ExecutorStdioServerLauncher::new( - runtime_environment.environment().get_exec_backend(), + environment.get_exec_backend(), runtime_environment.fallback_cwd(), )) } else { @@ -628,7 +631,12 @@ async fn make_rmcp_client( bearer_token_env_var, } => { let http_client: Arc = if remote_environment { - runtime_environment.environment().get_http_client() + let Some(environment) = runtime_environment.environment() else { + return Err(StartupOutcomeError::from(anyhow!( + "remote MCP server requires a runtime environment" + ))); + }; + environment.get_http_client() } else { Arc::new(ReqwestHttpClient) }; diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index 4284c96ff6..dc1fcec219 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -30,33 +30,79 @@ pub struct SandboxState { /// Runtime placement information used when starting MCP server transports. /// -/// `McpConfig` describes what servers exist. This value describes where those -/// servers should run for the current caller. Keep it explicit at manager -/// construction time so status/snapshot paths and real sessions make the same -/// local-vs-remote decision. `fallback_cwd` is not a per-server override; it is -/// used when a stdio server omits `cwd` and the launcher needs a concrete -/// process working directory. +/// `McpConfig` describes what servers exist. This value describes which +/// selected/default environment MCP servers should use for the current caller. +/// Keep it explicit at manager construction time so status/snapshot paths and +/// real sessions make the same placement decision. `fallback_cwd` is not a +/// per-server override; it is used when a stdio server omits `cwd` and the +/// launcher needs a concrete process working directory. `local_environment` +/// is separate because a remote selected/default environment can coexist with +/// an explicitly configured local environment that may launch local stdio MCPs. #[derive(Clone)] pub struct McpRuntimeEnvironment { - environment: Arc, + environment: Option>, + local_environment: Option>, fallback_cwd: PathBuf, } impl McpRuntimeEnvironment { - pub fn new(environment: Arc, fallback_cwd: PathBuf) -> Self { + pub fn new( + environment: Option>, + local_environment: Option>, + fallback_cwd: PathBuf, + ) -> Self { Self { environment, + local_environment, fallback_cwd, } } - pub(crate) fn environment(&self) -> Arc { - Arc::clone(&self.environment) + pub(crate) fn environment(&self) -> Option> { + self.environment.as_ref().map(Arc::clone) } pub(crate) fn fallback_cwd(&self) -> PathBuf { self.fallback_cwd.clone() } + + pub(crate) fn startup_unavailable_reason( + &self, + server_name: &str, + config: &codex_config::McpServerConfig, + ) -> Option { + // This is intentionally narrower than "no env means no MCP": local + // stdio needs a local process launcher, while local HTTP can still use + // the ambient HTTP client with no local environment configured. + match config.experimental_environment.as_deref() { + None | Some("local") => { + // Local stdio only needs an explicitly configured local + // launcher. The selected/default MCP environment can be remote + // when both local and remote environments are configured. + if self.local_environment.is_none() + && matches!( + config.transport, + codex_config::McpServerTransportConfig::Stdio { .. } + ) + { + Some(format!( + "local stdio MCP server `{server_name}` requires a local environment" + )) + } else { + None + } + } + Some("remote") => match self.environment.as_ref() { + Some(environment) if environment.is_remote() => None, + _ => Some(format!( + "remote MCP server `{server_name}` requires a remote environment" + )), + }, + Some(environment) => Some(format!( + "unsupported experimental_environment `{environment}` for MCP server `{server_name}`" + )), + } + } } pub(crate) fn emit_duration(metric: &str, duration: Duration, tags: &[(&str, &str)]) { @@ -64,3 +110,156 @@ pub(crate) fn emit_duration(metric: &str, duration: Duration, tags: &[(&str, &st let _ = metrics.record_duration(metric, duration, tags); } } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use codex_config::McpServerConfig; + use codex_config::McpServerTransportConfig; + use pretty_assertions::assert_eq; + + use super::*; + + fn stdio_server(experimental_environment: Option<&str>) -> McpServerConfig { + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: "echo".to_string(), + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + experimental_environment: experimental_environment.map(str::to_string), + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + } + } + + fn http_server(experimental_environment: Option<&str>) -> McpServerConfig { + McpServerConfig { + transport: McpServerTransportConfig::StreamableHttp { + url: "http://127.0.0.1:1".to_string(), + bearer_token_env_var: None, + http_headers: None, + env_http_headers: None, + }, + experimental_environment: experimental_environment.map(str::to_string), + ..stdio_server(/*experimental_environment*/ None) + } + } + + #[test] + fn local_stdio_requires_local_stdio_availability() { + let runtime_environment = McpRuntimeEnvironment::new( + /*environment*/ None, + /*local_environment*/ None, + PathBuf::from("/tmp"), + ); + + assert_eq!( + runtime_environment.startup_unavailable_reason( + "stdio", + &stdio_server(/*experimental_environment*/ None) + ), + Some("local stdio MCP server `stdio` requires a local environment".to_string()) + ); + } + + #[test] + fn local_http_does_not_require_local_stdio_availability() { + let runtime_environment = McpRuntimeEnvironment::new( + /*environment*/ None, + /*local_environment*/ None, + PathBuf::from("/tmp"), + ); + + assert_eq!( + runtime_environment.startup_unavailable_reason( + "http", + &http_server(/*experimental_environment*/ None) + ), + None + ); + } + + #[test] + fn remote_stdio_requires_remote_environment() { + let runtime_environment = McpRuntimeEnvironment::new( + /*environment*/ None, + /*local_environment*/ None, + PathBuf::from("/tmp"), + ); + + assert_eq!( + runtime_environment.startup_unavailable_reason( + "stdio", + &stdio_server(/*experimental_environment*/ Some("remote")), + ), + Some("remote MCP server `stdio` requires a remote environment".to_string()) + ); + } + + #[test] + fn remote_stdio_and_http_accept_remote_environment() { + let environment = Arc::new( + Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) + .expect("remote environment"), + ); + let runtime_environment = McpRuntimeEnvironment::new( + Some(environment), + /*local_environment*/ None, + PathBuf::from("/tmp"), + ); + + assert_eq!( + runtime_environment.startup_unavailable_reason( + "stdio", + &stdio_server(/*experimental_environment*/ Some("remote")), + ), + None + ); + assert_eq!( + runtime_environment.startup_unavailable_reason( + "http", + &http_server(/*experimental_environment*/ Some("remote")), + ), + None + ); + } + + #[tokio::test] + async fn local_stdio_accepts_remote_runtime_when_local_environment_exists() { + let remote_environment = Arc::new( + Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) + .expect("remote environment"), + ); + let local_environment = Arc::new( + Environment::create_for_tests(/*exec_server_url*/ None).expect("local environment"), + ); + let runtime_environment = McpRuntimeEnvironment::new( + Some(remote_environment), + Some(local_environment), + PathBuf::from("/tmp"), + ); + + assert_eq!( + runtime_environment.startup_unavailable_reason( + "stdio", + &stdio_server(/*experimental_environment*/ None), + ), + None + ); + } +} diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index 4e4336a2b5..8e2e21f0c4 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -88,6 +88,7 @@ pub struct ConfigRequirements { pub permission_profile: ConstrainedWithSource, pub web_search_mode: ConstrainedWithSource, pub allow_managed_hooks_only: Option>, + pub computer_use: Option>, pub feature_requirements: Option>, pub managed_hooks: Option>, pub mcp_servers: Option>>, @@ -122,6 +123,7 @@ impl Default for ConfigRequirements { /*source*/ None, ), allow_managed_hooks_only: None, + computer_use: None, feature_requirements: None, managed_hooks: None, mcp_servers: None, @@ -588,6 +590,17 @@ impl fmt::Display for WebSearchModeRequirement { } } +#[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)] +pub struct ComputerUseRequirementsToml { + pub allow_locked_computer_use: Option, +} + +impl ComputerUseRequirementsToml { + pub fn is_empty(&self) -> bool { + self.allow_locked_computer_use.is_none() + } +} + #[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)] pub struct FeatureRequirementsToml { #[serde(flatten)] @@ -691,6 +704,7 @@ pub struct ConfigRequirementsToml { pub remote_sandbox_config: Option>, pub allowed_web_search_modes: Option>, pub allow_managed_hooks_only: Option, + pub computer_use: Option, #[serde(rename = "features", alias = "feature_requirements")] pub feature_requirements: Option, pub hooks: Option, @@ -740,6 +754,7 @@ pub struct ConfigRequirementsWithSources { pub allowed_sandbox_modes: Option>>, pub allowed_web_search_modes: Option>>, pub allow_managed_hooks_only: Option>, + pub computer_use: Option>, pub feature_requirements: Option>, pub hooks: Option>, pub mcp_servers: Option>>, @@ -777,6 +792,7 @@ impl ConfigRequirementsWithSources { remote_sandbox_config: _, allowed_web_search_modes: _, allow_managed_hooks_only: _, + computer_use: _, feature_requirements: _, hooks: _, mcp_servers: _, @@ -807,6 +823,7 @@ impl ConfigRequirementsWithSources { allowed_sandbox_modes, allowed_web_search_modes, allow_managed_hooks_only, + computer_use, feature_requirements, hooks, mcp_servers, @@ -835,6 +852,7 @@ impl ConfigRequirementsWithSources { allowed_sandbox_modes, allowed_web_search_modes, allow_managed_hooks_only, + computer_use, feature_requirements, hooks, mcp_servers, @@ -853,6 +871,7 @@ impl ConfigRequirementsWithSources { remote_sandbox_config: None, allowed_web_search_modes: allowed_web_search_modes.map(|sourced| sourced.value), allow_managed_hooks_only: allow_managed_hooks_only.map(|sourced| sourced.value), + computer_use: computer_use.map(|sourced| sourced.value), feature_requirements: feature_requirements.map(|sourced| sourced.value), hooks: hooks.map(|sourced| sourced.value), mcp_servers: mcp_servers.map(|sourced| sourced.value), @@ -937,6 +956,10 @@ impl ConfigRequirementsToml { && self.remote_sandbox_config.is_none() && self.allowed_web_search_modes.is_none() && self.allow_managed_hooks_only.is_none() + && self + .computer_use + .as_ref() + .is_none_or(ComputerUseRequirementsToml::is_empty) && self .feature_requirements .as_ref() @@ -975,6 +998,7 @@ impl TryFrom for ConfigRequirements { allowed_sandbox_modes, allowed_web_search_modes, allow_managed_hooks_only, + computer_use, feature_requirements, hooks, mcp_servers, @@ -1211,6 +1235,7 @@ impl TryFrom for ConfigRequirements { permission_profile, web_search_mode, allow_managed_hooks_only, + computer_use, feature_requirements, managed_hooks, mcp_servers, @@ -1279,6 +1304,7 @@ mod tests { remote_sandbox_config: _, allowed_web_search_modes, allow_managed_hooks_only, + computer_use, feature_requirements, hooks, mcp_servers, @@ -1301,6 +1327,7 @@ mod tests { .map(|value| Sourced::new(value, RequirementSource::Unknown)), allow_managed_hooks_only: allow_managed_hooks_only .map(|value| Sourced::new(value, RequirementSource::Unknown)), + computer_use: computer_use.map(|value| Sourced::new(value, RequirementSource::Unknown)), feature_requirements: feature_requirements .map(|value| Sourced::new(value, RequirementSource::Unknown)), hooks: hooks.map(|value| Sourced::new(value, RequirementSource::Unknown)), @@ -1343,6 +1370,25 @@ mod tests { Ok(()) } + #[test] + fn deserialize_computer_use_requirements() -> Result<()> { + let requirements: ConfigRequirementsToml = from_str( + r#" + [computer_use] + allow_locked_computer_use = false + "#, + )?; + + assert_eq!( + requirements.computer_use, + Some(ComputerUseRequirementsToml { + allow_locked_computer_use: Some(false), + }) + ); + assert!(!requirements.is_empty()); + Ok(()) + } + #[test] fn merge_unset_fields_copies_every_field_and_sets_sources() { let mut target = ConfigRequirementsWithSources::default(); @@ -1362,6 +1408,9 @@ mod tests { let feature_requirements = FeatureRequirementsToml { entries: BTreeMap::from([("personality".to_string(), true)]), }; + let computer_use = ComputerUseRequirementsToml { + allow_locked_computer_use: Some(false), + }; let enforce_residency = ResidencyRequirement::Us; let enforce_source = source.clone(); let guardian_policy_config = "Use the company-managed guardian policy.".to_string(); @@ -1375,6 +1424,7 @@ mod tests { remote_sandbox_config: None, allowed_web_search_modes: Some(allowed_web_search_modes.clone()), allow_managed_hooks_only: Some(true), + computer_use: Some(computer_use.clone()), feature_requirements: Some(feature_requirements.clone()), hooks: None, mcp_servers: None, @@ -1409,6 +1459,7 @@ mod tests { /*value*/ true, enforce_source.clone(), )), + computer_use: Some(Sourced::new(computer_use, enforce_source.clone())), feature_requirements: Some(Sourced::new( feature_requirements, enforce_source.clone(), @@ -1452,6 +1503,7 @@ mod tests { allowed_sandbox_modes: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, feature_requirements: None, hooks: None, mcp_servers: None, @@ -1500,6 +1552,7 @@ mod tests { allowed_sandbox_modes: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, feature_requirements: None, hooks: None, mcp_servers: None, diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 3b49db638f..32065e3682 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -39,6 +39,7 @@ pub use config_requirements::AppRequirementToml; pub use config_requirements::AppToolRequirementToml; pub use config_requirements::AppToolsRequirementsToml; pub use config_requirements::AppsRequirementsToml; +pub use config_requirements::ComputerUseRequirementsToml; pub use config_requirements::ConfigRequirements; pub use config_requirements::ConfigRequirementsToml; pub use config_requirements::ConfigRequirementsWithSources; diff --git a/codex-rs/core/src/agent/role.rs b/codex-rs/core/src/agent/role.rs index 2ab16cd22a..a38930c00e 100644 --- a/codex-rs/core/src/agent/role.rs +++ b/codex-rs/core/src/agent/role.rs @@ -73,12 +73,14 @@ async fn apply_role_to_config_inner( } let (preserve_current_profile, preserve_current_provider) = preservation_policy(config, &role_layer_toml); + let preserve_current_service_tier = role_layer_toml.get("service_tier").is_none(); *config = reload::build_next_config( config, role_layer_toml, preserve_current_profile, preserve_current_provider, + preserve_current_service_tier, ) .await?; Ok(()) @@ -157,6 +159,7 @@ mod reload { role_layer_toml: TomlValue, preserve_current_profile: bool, preserve_current_provider: bool, + preserve_current_service_tier: bool, ) -> anyhow::Result { let active_profile_name = preserve_current_profile .then_some(config.active_profile.as_deref()) @@ -171,7 +174,11 @@ mod reload { let mut next_config = Config::load_config_with_layer_stack( LOCAL_FS.as_ref(), merged_config, - reload_overrides(config, preserve_current_provider), + reload_overrides( + config, + preserve_current_provider, + preserve_current_service_tier, + ), config.codex_home.clone(), config_layer_stack, ) @@ -261,10 +268,15 @@ mod reload { ConfigLayerEntry::new(ConfigLayerSource::SessionFlags, role_layer_toml) } - fn reload_overrides(config: &Config, preserve_current_provider: bool) -> ConfigOverrides { + fn reload_overrides( + config: &Config, + preserve_current_provider: bool, + preserve_current_service_tier: bool, + ) -> ConfigOverrides { ConfigOverrides { cwd: Some(config.cwd.to_path_buf()), model_provider: preserve_current_provider.then(|| config.model_provider_id.clone()), + service_tier: preserve_current_service_tier.then(|| config.service_tier.clone()), codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(), main_execve_wrapper_exe: config.main_execve_wrapper_exe.clone(), ..Default::default() @@ -323,8 +335,11 @@ pub(crate) mod spawn_tool_spec { let reasoning_effort = role_toml .get("model_reasoning_effort") .and_then(TomlValue::as_str); + let service_tier = role_toml + .get("service_tier") + .and_then(TomlValue::as_str); - match (model, reasoning_effort) { + let model_and_reasoning_note = match (model, reasoning_effort) { (Some(model), Some(reasoning_effort)) => format!( "\n- This role's model is set to `{model}` and its reasoning effort is set to `{reasoning_effort}`. These settings cannot be changed." ), @@ -339,7 +354,15 @@ pub(crate) mod spawn_tool_spec { ) } (None, None) => String::new(), - } + }; + let service_tier_note = service_tier + .map(|service_tier| { + format!( + "\n- This role's service tier is set to `{service_tier}`. If it is supported by the resolved model, it takes precedence over a valid spawn request service tier." + ) + }) + .unwrap_or_default(); + format!("{model_and_reasoning_note}{service_tier_note}") }) .unwrap_or_default(); format!("{name}: {{\n{description}{locked_settings_note}\n}}") diff --git a/codex-rs/core/src/agent/role_tests.rs b/codex-rs/core/src/agent/role_tests.rs index 1c99fb5950..828fa5e591 100644 --- a/codex-rs/core/src/agent/role_tests.rs +++ b/codex-rs/core/src/agent/role_tests.rs @@ -6,6 +6,7 @@ use crate::skills_load_input_from_config; use codex_config::ConfigLayerStackOrdering; use codex_core_plugins::PluginsManager; use codex_protocol::config_types::ReasoningSummary; +use codex_protocol::config_types::ServiceTier; use codex_protocol::config_types::Verbosity; use codex_protocol::openai_models::ReasoningEffort; use codex_utils_absolute_path::test_support::PathExt; @@ -214,6 +215,66 @@ async fn apply_role_preserves_unspecified_keys() { ); } +#[tokio::test] +async fn apply_role_reports_explicit_service_tier() { + let (home, mut config) = test_config_with_cli_overrides(Vec::new()).await; + let role_path = write_role_config( + &home, + "tiered-role.toml", + r#"developer_instructions = "Stay focused" +service_tier = "priority" +"#, + ) + .await; + config.agent_roles.insert( + "custom".to_string(), + AgentRoleConfig { + description: None, + config_file: Some(role_path), + nickname_candidates: None, + }, + ); + + apply_role_to_config(&mut config, Some("custom")) + .await + .expect("custom role should apply"); + + assert_eq!( + config.service_tier, + Some(ServiceTier::Fast.request_value().to_string()) + ); +} + +#[tokio::test] +async fn apply_role_preserves_existing_service_tier_without_override() { + let (home, mut config) = test_config_with_cli_overrides(Vec::new()).await; + config.service_tier = Some(ServiceTier::Fast.request_value().to_string()); + let role_path = write_role_config( + &home, + "default-tier-role.toml", + r#"developer_instructions = "Stay focused" +"#, + ) + .await; + config.agent_roles.insert( + "custom".to_string(), + AgentRoleConfig { + description: None, + config_file: Some(role_path), + nickname_candidates: None, + }, + ); + + apply_role_to_config(&mut config, Some("custom")) + .await + .expect("custom role should apply"); + + assert_eq!( + config.service_tier, + Some(ServiceTier::Fast.request_value().to_string()) + ); +} + #[tokio::test] async fn apply_role_preserves_active_profile_and_model_provider() { let home = TempDir::new().expect("create temp dir"); @@ -766,6 +827,31 @@ fn spawn_tool_spec_marks_role_locked_reasoning_effort_only() { )); } +#[test] +fn spawn_tool_spec_marks_role_locked_service_tier() { + let tempdir = TempDir::new().expect("create temp dir"); + let role_path = tempdir.path().join("tiered.toml"); + fs::write( + &role_path, + "developer_instructions = \"Stay fast\"\nservice_tier = \"priority\"\n", + ) + .expect("write role config"); + let user_defined_roles = BTreeMap::from([( + "tiered".to_string(), + AgentRoleConfig { + description: Some("Stay fast.".to_string()), + config_file: Some(role_path), + nickname_candidates: None, + }, + )]); + + let spec = spawn_tool_spec::build(&user_defined_roles); + + assert!(spec.contains( + "Stay fast.\n- This role's service tier is set to `priority`. If it is supported by the resolved model, it takes precedence over a valid spawn request service tier." + )); +} + #[test] fn built_in_config_file_contents_resolves_explorer_only() { assert_eq!( diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index cd84f06cda..760b1f3fc5 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -1098,6 +1098,7 @@ allowed_approval_policies = ["on-request"] remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, feature_requirements: None, hooks: None, mcp_servers: None, @@ -1156,6 +1157,7 @@ allowed_approval_policies = ["on-request"] remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, feature_requirements: None, hooks: None, mcp_servers: None, @@ -1363,6 +1365,7 @@ async fn load_config_layers_includes_cloud_requirements() -> anyhow::Result<()> remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, feature_requirements: None, hooks: None, mcp_servers: None, diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 2297314e06..dac1bf317e 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -8636,6 +8636,7 @@ async fn test_requirements_web_search_mode_allowlist_does_not_warn_when_unset() remote_sandbox_config: None, allowed_web_search_modes: Some(vec![codex_config::WebSearchModeRequirement::Cached]), allow_managed_hooks_only: None, + computer_use: None, feature_requirements: None, hooks: None, mcp_servers: None, @@ -9419,6 +9420,7 @@ async fn explicit_sandbox_mode_falls_back_when_disallowed_by_requirements() -> s remote_sandbox_config: None, allowed_web_search_modes: None, allow_managed_hooks_only: None, + computer_use: None, feature_requirements: None, hooks: None, mcp_servers: None, diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 336d54b382..93b67deb1a 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2426,6 +2426,7 @@ impl Config { permission_profile: mut constrained_permission_profile, web_search_mode: mut constrained_web_search_mode, allow_managed_hooks_only: _, + computer_use: _, feature_requirements, managed_hooks: _, mcp_servers, diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index f4ee42c821..de41919a89 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -198,7 +198,8 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_options_and_status( config.codex_linux_sandbox_exe.clone(), )?; let environment_manager = - EnvironmentManager::from_codex_home(config.codex_home.clone(), local_runtime_paths).await?; + EnvironmentManager::from_codex_home(config.codex_home.clone(), Some(local_runtime_paths)) + .await?; list_accessible_connectors_from_mcp_tools_with_environment_manager( config, force_refetch, @@ -261,10 +262,6 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_environment_manager( let (tx_event, rx_event) = unbounded(); drop(rx_event); - let environment = environment_manager - .default_environment() - .unwrap_or_else(|| environment_manager.local_environment()); - let (mut mcp_connection_manager, cancel_token) = McpConnectionManager::new( &mcp_servers, config.mcp_oauth_credentials_store_mode, @@ -273,7 +270,13 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_environment_manager( INITIAL_SUBMIT_ID.to_owned(), tx_event, PermissionProfile::default(), - McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf()), + // Connector discovery is threadless. Use an actually configured env if + // one exists, but do not reintroduce the old hidden-local fallback. + McpRuntimeEnvironment::new( + environment_manager.default_or_local_environment(), + environment_manager.try_local_environment(), + config.cwd.to_path_buf(), + ), config.codex_home.to_path_buf(), codex_apps_tools_cache_key(auth.as_ref()), host_owned_codex_apps_enabled, diff --git a/codex-rs/core/src/environment_selection.rs b/codex-rs/core/src/environment_selection.rs index 89808c27ee..749212f70a 100644 --- a/codex-rs/core/src/environment_selection.rs +++ b/codex-rs/core/src/environment_selection.rs @@ -106,7 +106,7 @@ mod tests { let cwd = AbsolutePathBuf::current_dir().expect("cwd"); let manager = EnvironmentManager::create_for_tests( Some("ws://127.0.0.1:8765".to_string()), - test_runtime_paths(), + Some(test_runtime_paths()), ) .await; @@ -132,9 +132,10 @@ url = "ws://127.0.0.1:8765" ) .expect("write environments.toml"); let cwd = AbsolutePathBuf::current_dir().expect("cwd"); - let manager = EnvironmentManager::from_codex_home(temp_dir.path(), test_runtime_paths()) - .await - .expect("environment manager"); + let manager = + EnvironmentManager::from_codex_home(temp_dir.path(), Some(test_runtime_paths())) + .await + .expect("environment manager"); assert_eq!( default_thread_environment_selections(&manager, &cwd), @@ -154,7 +155,7 @@ url = "ws://127.0.0.1:8765" #[tokio::test] async fn default_thread_environment_selections_empty_when_default_disabled() { let cwd = AbsolutePathBuf::current_dir().expect("cwd"); - let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); + let manager = EnvironmentManager::without_environments(); assert_eq!( default_thread_environment_selections(&manager, &cwd), diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 902ede574e..6dc9f82465 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -661,7 +661,8 @@ async fn maybe_request_codex_apps_auth_elicitation( }; let response = sess .request_mcp_server_elicitation(turn_context, request_id, params) - .await; + .await + .response; if !response .as_ref() .is_some_and(|response| response.action == ElicitationAction::Accept) @@ -1325,7 +1326,8 @@ async fn maybe_request_mcp_tool_approval( ); let decision = parse_mcp_tool_approval_elicitation_response( sess.request_mcp_server_elicitation(turn_context.as_ref(), request_id, params) - .await, + .await + .response, &question_id, ); let decision = normalize_approval_decision_for_mode(decision, approval_mode); diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 25f19c87dd..50a6bb4d45 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1235,8 +1235,8 @@ async fn install_host_owned_codex_apps_manager(session: &Session, turn_context: let environment = session .services .environment_manager - .default_environment() - .unwrap_or_else(|| session.services.environment_manager.local_environment()); + .default_or_local_environment() + .expect("test session should have an MCP runtime environment"); let (manager, _cancel_token) = codex_mcp::McpConnectionManager::new( &HashMap::new(), turn_context.config.mcp_oauth_credentials_store_mode, @@ -1245,10 +1245,14 @@ async fn install_host_owned_codex_apps_manager(session: &Session, turn_context: turn_context.sub_id.clone(), session.get_tx_event(), turn_context.permission_profile(), - codex_mcp::McpRuntimeEnvironment::new(environment, { - #[allow(deprecated)] - turn_context.cwd.to_path_buf() - }), + codex_mcp::McpRuntimeEnvironment::new( + Some(environment), + session.services.environment_manager.try_local_environment(), + { + #[allow(deprecated)] + turn_context.cwd.to_path_buf() + }, + ), turn_context.config.codex_home.to_path_buf(), codex_mcp::codex_apps_tools_cache_key(auth.as_ref()), /*host_owned_codex_apps_enabled*/ true, diff --git a/codex-rs/core/src/prompt_debug.rs b/codex-rs/core/src/prompt_debug.rs index c6386d7c77..f6bf7d0022 100644 --- a/codex-rs/core/src/prompt_debug.rs +++ b/codex-rs/core/src/prompt_debug.rs @@ -45,9 +45,12 @@ pub async fn build_prompt_input( Arc::clone(&auth_manager), SessionSource::Exec, Arc::new( - EnvironmentManager::from_codex_home(config.codex_home.clone(), local_runtime_paths) - .await - .map_err(|err| CodexErr::Fatal(err.to_string()))?, + EnvironmentManager::from_codex_home( + config.codex_home.clone(), + Some(local_runtime_paths), + ) + .await + .map_err(|err| CodexErr::Fatal(err.to_string()))?, ), empty_extension_registry(), /*analytics_events_client*/ None, diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 9f27751f7e..738a328671 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -5,6 +5,7 @@ use codex_mcp::ElicitationReviewerHandle; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::mcp_approval_meta::APPROVAL_KIND_KEY as MCP_ELICITATION_APPROVAL_KIND_KEY; use codex_protocol::mcp_approval_meta::APPROVAL_KIND_MCP_TOOL_CALL as MCP_ELICITATION_APPROVAL_KIND_MCP_TOOL_CALL; +use codex_protocol::mcp_approval_meta::APPROVAL_KIND_TOOL_SUGGESTION as MCP_ELICITATION_APPROVAL_KIND_TOOL_SUGGESTION; use codex_protocol::mcp_approval_meta::APPROVALS_REVIEWER_KEY as MCP_ELICITATION_APPROVALS_REVIEWER_KEY; use codex_protocol::mcp_approval_meta::CONNECTOR_DESCRIPTION_KEY as MCP_ELICITATION_CONNECTOR_DESCRIPTION_KEY; use codex_protocol::mcp_approval_meta::CONNECTOR_ID_KEY as MCP_ELICITATION_CONNECTOR_ID_KEY; @@ -21,6 +22,10 @@ use rmcp::model::Meta; use serde_json::Map; const MCP_ELICITATION_DECLINE_MESSAGE_KEY: &str = "message"; +const TOOL_SUGGESTION_ACTION_INSTALL: &str = "install"; +const TOOL_SUGGESTION_ACTION_KEY: &str = "suggest_type"; +const TOOL_SUGGESTION_TOOL_ID_KEY: &str = "tool_id"; +const TOOL_SUGGESTION_TOOL_TYPE_KEY: &str = "tool_type"; #[derive(Debug, PartialEq)] enum GuardianElicitationReview { @@ -33,6 +38,18 @@ struct GuardianMcpElicitationReviewer { session: std::sync::Weak, } +pub(crate) struct McpServerElicitationOutcome { + pub(crate) response: Option, + pub(crate) sent: bool, +} + +#[derive(Debug, PartialEq, Eq)] +struct PluginInstallElicitationTelemetryMetadata { + tool_type: String, + tool_id: String, + tool_name: String, +} + impl GuardianMcpElicitationReviewer { fn new(session: &Arc) -> Self { Self { @@ -70,7 +87,7 @@ impl Session { turn_context: &TurnContext, request_id: RequestId, params: McpServerElicitationRequestParams, - ) -> Option { + ) -> McpServerElicitationOutcome { if self .services .mcp_connection_manager @@ -78,11 +95,14 @@ impl Session { .await .elicitations_auto_deny() { - return Some(ElicitationResponse { - action: codex_rmcp_client::ElicitationAction::Accept, - content: Some(serde_json::json!({})), - meta: None, - }); + return McpServerElicitationOutcome { + response: Some(ElicitationResponse { + action: codex_rmcp_client::ElicitationAction::Accept, + content: Some(serde_json::json!({})), + meta: None, + }), + sent: false, + }; } let server_name = params.server_name.clone(); @@ -98,7 +118,10 @@ impl Session { warn!( "failed to serialize MCP elicitation schema for server_name: {server_name}, request_id: {request_id}: {err:#}" ); - return None; + return McpServerElicitationOutcome { + response: None, + sent: false, + }; } }; codex_protocol::approvals::ElicitationRequest::Form { @@ -154,11 +177,24 @@ impl Session { id, request, }); + let plugin_install_telemetry = plugin_install_elicitation_telemetry_metadata(&event); turn_context .turn_metadata_state .mark_user_input_requested_during_turn(); self.send_event(turn_context, event).await; - rx_response.await.ok() + if let Some(plugin_install_telemetry) = plugin_install_telemetry { + turn_context + .session_telemetry + .record_plugin_install_elicitation_sent( + plugin_install_telemetry.tool_type.as_str(), + plugin_install_telemetry.tool_id.as_str(), + plugin_install_telemetry.tool_name.as_str(), + ); + } + McpServerElicitationOutcome { + response: rx_response.await.ok(), + sent: true, + } } #[expect( @@ -291,14 +327,15 @@ impl Session { compute_auth_statuses(mcp_servers.iter(), store_mode, auth.as_ref()).await; let mcp_runtime_environment = match turn_context.environments.primary() { Some(turn_environment) => McpRuntimeEnvironment::new( - Arc::clone(&turn_environment.environment), + Some(Arc::clone(&turn_environment.environment)), + self.services.environment_manager.try_local_environment(), turn_environment.cwd.to_path_buf(), ), None => McpRuntimeEnvironment::new( self.services .environment_manager - .default_environment() - .unwrap_or_else(|| self.services.environment_manager.local_environment()), + .default_or_local_environment(), + self.services.environment_manager.try_local_environment(), #[allow(deprecated)] turn_context.cwd.to_path_buf(), ), @@ -550,6 +587,33 @@ fn metadata_owned_string(meta: &Map, key: &str) -> Option .map(ToOwned::to_owned) } +fn plugin_install_elicitation_telemetry_metadata( + event: &EventMsg, +) -> Option { + let EventMsg::ElicitationRequest(ElicitationRequestEvent { request, .. }) = event else { + return None; + }; + let codex_protocol::approvals::ElicitationRequest::Form { + meta: Some(Value::Object(meta)), + .. + } = request + else { + return None; + }; + if metadata_str(meta, MCP_ELICITATION_APPROVAL_KIND_KEY) + != Some(MCP_ELICITATION_APPROVAL_KIND_TOOL_SUGGESTION) + || metadata_str(meta, TOOL_SUGGESTION_ACTION_KEY) != Some(TOOL_SUGGESTION_ACTION_INSTALL) + { + return None; + } + + Some(PluginInstallElicitationTelemetryMetadata { + tool_type: metadata_owned_string(meta, TOOL_SUGGESTION_TOOL_TYPE_KEY)?, + tool_id: metadata_owned_string(meta, TOOL_SUGGESTION_TOOL_ID_KEY)?, + tool_name: metadata_owned_string(meta, MCP_ELICITATION_TOOL_NAME_KEY)?, + }) +} + fn mcp_elicitation_request_id(id: &RequestId) -> String { match id { rmcp::model::NumberOrString::String(value) => value.to_string(), diff --git a/codex-rs/core/src/session/mcp_tests.rs b/codex-rs/core/src/session/mcp_tests.rs index 31b304faa5..0e3664019b 100644 --- a/codex-rs/core/src/session/mcp_tests.rs +++ b/codex-rs/core/src/session/mcp_tests.rs @@ -96,6 +96,63 @@ fn guardian_elicitation_review_request_defaults_missing_tool_params() { assert_eq!(arguments, Some(json!({}))); } +#[test] +fn plugin_install_elicitation_telemetry_metadata_requires_install_tool_suggestion() { + let event = EventMsg::ElicitationRequest(ElicitationRequestEvent { + turn_id: Some("turn-1".to_string()), + server_name: "codex_apps".to_string(), + id: codex_protocol::mcp::RequestId::String("request-1".to_string()), + request: codex_protocol::approvals::ElicitationRequest::Form { + meta: Some(json!({ + "codex_approval_kind": "tool_suggestion", + "suggest_type": "install", + "tool_type": "plugin", + "tool_id": "slack@openai-curated", + "tool_name": "Slack", + })), + message: "Install Slack?".to_string(), + requested_schema: json!({ + "type": "object", + "properties": {}, + }), + }, + }); + + assert_eq!( + plugin_install_elicitation_telemetry_metadata(&event), + Some(PluginInstallElicitationTelemetryMetadata { + tool_type: "plugin".to_string(), + tool_id: "slack@openai-curated".to_string(), + tool_name: "Slack".to_string(), + }) + ); + + let enable_event = EventMsg::ElicitationRequest(ElicitationRequestEvent { + turn_id: Some("turn-1".to_string()), + server_name: "codex_apps".to_string(), + id: codex_protocol::mcp::RequestId::String("request-2".to_string()), + request: codex_protocol::approvals::ElicitationRequest::Form { + meta: Some(json!({ + "codex_approval_kind": "tool_suggestion", + "suggest_type": "enable", + "tool_type": "plugin", + "tool_id": "slack@openai-curated", + "tool_name": "Slack", + })), + message: "Enable Slack?".to_string(), + requested_schema: json!({ + "type": "object", + "properties": {}, + }), + }, + }); + + assert_eq!( + plugin_install_elicitation_telemetry_metadata(&enable_event), + None + ); +} + #[test] fn guardian_elicitation_review_request_requires_opt_in() { let request = form_request(meta(json!({ diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 2d96cf9165..884c440f28 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1050,14 +1050,13 @@ impl Session { .cloned(); let mcp_runtime_environment = match turn_environment { Some(turn_environment) => McpRuntimeEnvironment::new( - Arc::clone(&turn_environment.environment), + Some(Arc::clone(&turn_environment.environment)), + sess.services.environment_manager.try_local_environment(), turn_environment.cwd.to_path_buf(), ), None => McpRuntimeEnvironment::new( - sess.services - .environment_manager - .default_environment() - .unwrap_or_else(|| sess.services.environment_manager.local_environment()), + sess.services.environment_manager.default_or_local_environment(), + sess.services.environment_manager.try_local_environment(), session_configuration.cwd.to_path_buf(), ), }; diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 88e9092da3..bfe3cfb3fe 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -328,13 +328,14 @@ async fn request_mcp_server_elicitation_auto_accepts_when_auto_deny_is_enabled() .await; assert_eq!( - response, + response.response, Some(ElicitationResponse { action: ElicitationAction::Accept, content: Some(json!({})), meta: None, }) ); + assert!(!response.sent); assert!(rx.try_recv().is_err()); } diff --git a/codex-rs/core/src/thread_manager_tests.rs b/codex-rs/core/src/thread_manager_tests.rs index 43ec706f1a..255a8336b9 100644 --- a/codex-rs/core/src/thread_manager_tests.rs +++ b/codex-rs/core/src/thread_manager_tests.rs @@ -309,7 +309,7 @@ async fn start_thread_rejects_explicit_local_environment_when_default_provider_i let environment_manager = Arc::new( codex_exec_server::EnvironmentManager::create_for_tests( Some("none".to_string()), - runtime_paths, + Some(runtime_paths), ) .await, ); @@ -373,7 +373,7 @@ args = ["dev", "cd /tmp && true"] let environment_manager = Arc::new( codex_exec_server::EnvironmentManager::from_codex_home( config.codex_home.clone(), - runtime_paths, + Some(runtime_paths), ) .await .expect("environment manager"), diff --git a/codex-rs/core/src/tools/handlers/list_available_plugins_to_install.rs b/codex-rs/core/src/tools/handlers/list_available_plugins_to_install.rs new file mode 100644 index 0000000000..143eb5a800 --- /dev/null +++ b/codex-rs/core/src/tools/handlers/list_available_plugins_to_install.rs @@ -0,0 +1,173 @@ +use codex_tools::LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME; +use codex_tools::ListAvailablePluginsToInstallResult; +use codex_tools::RequestPluginInstallEntry; +use codex_tools::ToolName; +use codex_tools::ToolSpec; + +use crate::function_tool::FunctionCallError; +use crate::tools::context::FunctionToolOutput; +use crate::tools::context::ToolInvocation; +use crate::tools::context::ToolPayload; +use crate::tools::context::boxed_tool_output; +use crate::tools::handlers::list_available_plugins_to_install_spec::create_list_available_plugins_to_install_tool; +use crate::tools::registry::CoreToolRuntime; +use crate::tools::registry::ToolExecutor; + +const MAX_LIST_AVAILABLE_PLUGINS_TO_INSTALL_DESCRIPTION_CHARS: usize = 240; + +pub struct ListAvailablePluginsToInstallHandler { + tools: Vec, +} + +impl ListAvailablePluginsToInstallHandler { + pub(crate) fn new(mut tools: Vec) -> Self { + tools.sort_by(|left, right| { + left.name + .cmp(&right.name) + .then_with(|| left.id.cmp(&right.id)) + }); + Self { tools } + } + + fn result(&self) -> ListAvailablePluginsToInstallResult { + ListAvailablePluginsToInstallResult { + tools: self + .tools + .iter() + .map(|tool| RequestPluginInstallEntry { + id: tool.id.clone(), + name: tool.name.clone(), + description: tool.description.as_ref().map(|description| { + truncate_to_char_boundary( + description, + MAX_LIST_AVAILABLE_PLUGINS_TO_INSTALL_DESCRIPTION_CHARS, + ) + .to_string() + }), + tool_type: tool.tool_type, + has_skills: tool.has_skills, + mcp_server_names: tool.mcp_server_names.clone(), + app_connector_ids: tool.app_connector_ids.clone(), + }) + .collect(), + } + } +} + +#[async_trait::async_trait] +impl ToolExecutor for ListAvailablePluginsToInstallHandler { + fn tool_name(&self) -> ToolName { + ToolName::plain(LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME) + } + + fn spec(&self) -> Option { + Some(create_list_available_plugins_to_install_tool()) + } + + fn supports_parallel_tool_calls(&self) -> bool { + false + } + + async fn handle( + &self, + invocation: ToolInvocation, + ) -> Result, FunctionCallError> { + let ToolInvocation { payload, .. } = invocation; + match payload { + ToolPayload::Function { .. } => {} + _ => { + return Err(FunctionCallError::Fatal(format!( + "{LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME} handler received unsupported payload" + ))); + } + } + + let content = serde_json::to_string(&self.result()).map_err(|err| { + FunctionCallError::Fatal(format!( + "failed to serialize {LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME} response: {err}" + )) + })?; + + Ok(boxed_tool_output(FunctionToolOutput::from_text( + content, + Some(true), + ))) + } +} + +impl CoreToolRuntime for ListAvailablePluginsToInstallHandler {} + +fn truncate_to_char_boundary(value: &str, max_chars: usize) -> &str { + match value.char_indices().nth(max_chars) { + Some((index, _)) => &value[..index], + None => value, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use codex_tools::DiscoverableToolType; + use pretty_assertions::assert_eq; + + #[test] + fn list_tool_does_not_support_parallel_calls() { + assert!( + !ListAvailablePluginsToInstallHandler::new(Vec::new()).supports_parallel_tool_calls() + ); + } + + #[test] + fn result_truncates_candidate_descriptions() { + let handler = ListAvailablePluginsToInstallHandler::new(vec![ + RequestPluginInstallEntry { + id: "sample@openai-curated".to_string(), + name: "Sample Plugin".to_string(), + description: Some( + "x".repeat(MAX_LIST_AVAILABLE_PLUGINS_TO_INSTALL_DESCRIPTION_CHARS + 1), + ), + tool_type: DiscoverableToolType::Plugin, + has_skills: true, + mcp_server_names: vec!["sample-mcp".to_string()], + app_connector_ids: vec!["connector-sample".to_string()], + }, + RequestPluginInstallEntry { + id: "calendar@openai-curated".to_string(), + name: "Calendar".to_string(), + description: Some("calendar".to_string()), + tool_type: DiscoverableToolType::Plugin, + has_skills: false, + mcp_server_names: Vec::new(), + app_connector_ids: Vec::new(), + }, + ]); + + assert_eq!( + handler.result(), + ListAvailablePluginsToInstallResult { + tools: vec![ + RequestPluginInstallEntry { + id: "calendar@openai-curated".to_string(), + name: "Calendar".to_string(), + description: Some("calendar".to_string()), + tool_type: DiscoverableToolType::Plugin, + has_skills: false, + mcp_server_names: Vec::new(), + app_connector_ids: Vec::new(), + }, + RequestPluginInstallEntry { + id: "sample@openai-curated".to_string(), + name: "Sample Plugin".to_string(), + description: Some( + "x".repeat(MAX_LIST_AVAILABLE_PLUGINS_TO_INSTALL_DESCRIPTION_CHARS,) + ), + tool_type: DiscoverableToolType::Plugin, + has_skills: true, + mcp_server_names: vec!["sample-mcp".to_string()], + app_connector_ids: vec!["connector-sample".to_string()], + }, + ], + } + ); + } +} diff --git a/codex-rs/core/src/tools/handlers/list_available_plugins_to_install_spec.rs b/codex-rs/core/src/tools/handlers/list_available_plugins_to_install_spec.rs new file mode 100644 index 0000000000..ac3120753d --- /dev/null +++ b/codex-rs/core/src/tools/handlers/list_available_plugins_to_install_spec.rs @@ -0,0 +1,45 @@ +use codex_tools::JsonSchema; +use codex_tools::LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME; +use codex_tools::REQUEST_PLUGIN_INSTALL_TOOL_NAME; +use codex_tools::ResponsesApiTool; +use codex_tools::TOOL_SEARCH_TOOL_NAME; +use codex_tools::ToolSpec; +pub(crate) fn create_list_available_plugins_to_install_tool() -> ToolSpec { + let description = format!( + "# List plugin/connector install candidates\n\nUse this tool only when both are true:\n- The user explicitly asks to use a specific plugin or connector that is not already available in the current context or active `tools` list.\n- `{TOOL_SEARCH_TOOL_NAME}` is not available, or it has already been called and did not find or make the requested tool callable.\n\nReturns known plugins and connectors that can be passed to `{REQUEST_PLUGIN_INSTALL_TOOL_NAME}`. When both a plugin and a connector match, prefer the plugin; use the connector only when its corresponding plugin is already installed.\n" + ); + + ToolSpec::Function(ResponsesApiTool { + name: LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME.to_string(), + description, + strict: false, + defer_loading: None, + parameters: JsonSchema::object(Default::default(), Some(Vec::new()), Some(false.into())), + output_schema: None, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn create_list_available_plugins_to_install_tool_uses_expected_wire_shape() { + assert_eq!( + create_list_available_plugins_to_install_tool(), + ToolSpec::Function(ResponsesApiTool { + name: "list_available_plugins_to_install".to_string(), + description: "# List plugin/connector install candidates\n\nUse this tool only when both are true:\n- The user explicitly asks to use a specific plugin or connector that is not already available in the current context or active `tools` list.\n- `tool_search` is not available, or it has already been called and did not find or make the requested tool callable.\n\nReturns known plugins and connectors that can be passed to `request_plugin_install`. When both a plugin and a connector match, prefer the plugin; use the connector only when its corresponding plugin is already installed.\n".to_string(), + strict: false, + defer_loading: None, + parameters: JsonSchema::object( + Default::default(), + Some(Vec::new()), + Some(false.into()), + ), + output_schema: None, + }) + ); + } +} diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index fe956faa37..b2cbe62907 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -6,6 +6,8 @@ mod dynamic; pub(crate) mod extension_tools; mod goal; pub(crate) mod goal_spec; +mod list_available_plugins_to_install; +pub(crate) mod list_available_plugins_to_install_spec; mod mcp; mod mcp_resource; pub(crate) mod mcp_resource_spec; @@ -54,6 +56,7 @@ pub use dynamic::DynamicToolHandler; pub use goal::CreateGoalHandler; pub use goal::GetGoalHandler; pub use goal::UpdateGoalHandler; +pub use list_available_plugins_to_install::ListAvailablePluginsToInstallHandler; pub use mcp::McpHandler; pub use mcp_resource::ListMcpResourceTemplatesHandler; pub use mcp_resource::ListMcpResourcesHandler; diff --git a/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs b/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs index ceb46083c5..dc0518de6b 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs @@ -83,6 +83,9 @@ async fn handle_spawn_agent( .await; let mut config = build_agent_spawn_config(&session.get_base_instructions().await, turn.as_ref())?; + if let Some(service_tier) = args.service_tier.as_ref() { + config.service_tier = Some(service_tier.clone()); + } if args.fork_context { reject_full_fork_spawn_overrides(role_name, args.model.as_deref(), args.reasoning_effort)?; } else { diff --git a/codex-rs/core/src/tools/handlers/multi_agents_common.rs b/codex-rs/core/src/tools/handlers/multi_agents_common.rs index 968810494d..4f459a0d0e 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_common.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_common.rs @@ -344,9 +344,16 @@ pub(crate) async fn apply_spawn_agent_service_tier( parent_service_tier: Option<&str>, requested_service_tier: Option<&str>, ) -> Result<(), FunctionCallError> { - let Some(candidate_service_tier) = requested_service_tier.or(parent_service_tier) else { + let candidate_service_tiers = [ + config.service_tier.clone(), + requested_service_tier.map(str::to_string), + parent_service_tier.map(str::to_string), + ]; + if candidate_service_tiers.iter().all(Option::is_none) { + config.service_tier = None; return Ok(()); - }; + } + let model = config.model.clone().ok_or_else(|| { FunctionCallError::RespondToModel( "spawn_agent could not resolve the child model for service tier validation".to_string(), @@ -358,29 +365,32 @@ pub(crate) async fn apply_spawn_agent_service_tier( .get_model_info(model.as_str(), &config.to_models_manager_config()) .await; - if model_info.supports_service_tier(candidate_service_tier) { - config.service_tier = Some(candidate_service_tier.to_string()); - return Ok(()); + if let Some(requested_service_tier) = requested_service_tier + && !model_info.supports_service_tier(requested_service_tier) + { + let supported_service_tiers = if model_info.service_tiers.is_empty() { + "none".to_string() + } else { + model_info + .service_tiers + .iter() + .map(|tier| tier.id.as_str()) + .collect::>() + .join(", ") + }; + return Err(FunctionCallError::RespondToModel(format!( + "Service tier `{requested_service_tier}` is not supported for model `{model}`. Supported service tiers: {supported_service_tiers}" + ))); } - if requested_service_tier.is_none() { - config.service_tier = None; - return Ok(()); - } - - let supported_service_tiers = if model_info.service_tiers.is_empty() { - "none".to_string() - } else { - model_info - .service_tiers - .iter() - .map(|tier| tier.id.as_str()) - .collect::>() - .join(", ") - }; - Err(FunctionCallError::RespondToModel(format!( - "Service tier `{candidate_service_tier}` is not supported for model `{model}`. Supported service tiers: {supported_service_tiers}" - ))) + config.service_tier = + candidate_service_tiers + .into_iter() + .flatten() + .find(|candidate_service_tier| { + model_info.supports_service_tier(candidate_service_tier.as_str()) + }); + Ok(()) } fn find_spawn_agent_model_name( diff --git a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs index ad1702b207..d1e2aff3fb 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs @@ -704,6 +704,129 @@ service_tier = "priority" } } +#[tokio::test] +async fn spawn_agent_role_service_tier_falls_back_to_supported_parent_tier() { + #[derive(Debug, Deserialize)] + struct SpawnAgentResult { + agent_id: String, + } + + let (mut session, turn) = make_session_and_context().await; + let mut turn = turn + .with_model("gpt-5.4".to_string(), &session.services.models_manager) + .await; + tokio::fs::create_dir_all(&turn.config.codex_home) + .await + .expect("codex home should be created"); + let role_config_path = turn.config.codex_home.as_path().join("tiered-role.toml"); + tokio::fs::write( + &role_config_path, + r#"model = "gpt-5.4" +service_tier = "turbo" +"#, + ) + .await + .expect("role config should be written"); + + let role_name = "tiered-role".to_string(); + let mut config = (*turn.config).clone(); + config.service_tier = Some(ServiceTier::Fast.request_value().to_string()); + config.agent_roles.insert( + role_name.clone(), + AgentRoleConfig { + description: Some("Role with an unsupported child tier".to_string()), + config_file: Some(role_config_path), + nickname_candidates: None, + }, + ); + turn.config = Arc::new(config); + let manager = thread_manager(); + let root = manager + .start_thread((*turn.config).clone()) + .await + .expect("root thread should start"); + session.services.agent_control = manager.agent_control(); + session.conversation_id = root.thread_id; + + let output = SpawnAgentHandler::default() + .handle(invocation( + Arc::new(session), + Arc::new(turn), + "spawn_agent", + function_payload(json!({ + "message": "inspect this repo", + "agent_type": role_name + })), + )) + .await + .expect("spawn_agent should fall back to the supported parent tier"); + let (content, _) = expect_text_output(output); + let result: SpawnAgentResult = + serde_json::from_str(&content).expect("spawn_agent result should be json"); + let snapshot = manager + .get_thread(parse_agent_id(&result.agent_id)) + .await + .expect("spawned agent thread should exist") + .config_snapshot() + .await; + + assert_eq!( + snapshot.service_tier, + Some(ServiceTier::Fast.request_value().to_string()) + ); +} + +#[tokio::test] +async fn spawn_agent_role_service_tier_does_not_hide_invalid_spawn_request() { + let (session, mut turn) = make_session_and_context().await; + tokio::fs::create_dir_all(&turn.config.codex_home) + .await + .expect("codex home should be created"); + let role_config_path = turn.config.codex_home.as_path().join("tiered-role.toml"); + tokio::fs::write( + &role_config_path, + r#"model = "gpt-5.4" +service_tier = "priority" +"#, + ) + .await + .expect("role config should be written"); + + let role_name = "tiered-role".to_string(); + let mut config = (*turn.config).clone(); + config.agent_roles.insert( + role_name.clone(), + AgentRoleConfig { + description: Some("Role with a supported child tier".to_string()), + config_file: Some(role_config_path), + nickname_candidates: None, + }, + ); + turn.config = Arc::new(config); + + let err = SpawnAgentHandler::default() + .handle(invocation( + Arc::new(session), + Arc::new(turn), + "spawn_agent", + function_payload(json!({ + "message": "inspect this repo", + "agent_type": role_name, + "service_tier": "turbo" + })), + )) + .await + .expect_err("invalid spawn service tier should still be rejected"); + + assert_eq!( + err, + FunctionCallError::RespondToModel( + "Service tier `turbo` is not supported for model `gpt-5.4`. Supported service tiers: priority" + .to_string() + ) + ); +} + #[tokio::test] async fn spawn_agent_full_history_fork_accepts_explicit_service_tier() { #[derive(Debug, Deserialize)] diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs index 2daa1f07db..74a87d8a54 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs @@ -82,6 +82,9 @@ async fn handle_spawn_agent( .await; let mut config = build_agent_spawn_config(&session.get_base_instructions().await, turn.as_ref())?; + if let Some(service_tier) = args.service_tier.as_ref() { + config.service_tier = Some(service_tier.clone()); + } if matches!(fork_mode, Some(SpawnAgentForkMode::FullHistory)) { reject_full_fork_spawn_overrides(role_name, args.model.as_deref(), args.reasoning_effort)?; } else { diff --git a/codex-rs/core/src/tools/handlers/request_plugin_install.rs b/codex-rs/core/src/tools/handlers/request_plugin_install.rs index 3b53fd007a..ffb5bb568b 100644 --- a/codex-rs/core/src/tools/handlers/request_plugin_install.rs +++ b/codex-rs/core/src/tools/handlers/request_plugin_install.rs @@ -8,17 +8,16 @@ use codex_rmcp_client::ElicitationResponse; use codex_tools::DiscoverableTool; use codex_tools::DiscoverableToolAction; use codex_tools::DiscoverableToolType; +use codex_tools::LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME; use codex_tools::REQUEST_PLUGIN_INSTALL_PERSIST_ALWAYS_VALUE; use codex_tools::REQUEST_PLUGIN_INSTALL_PERSIST_KEY; use codex_tools::REQUEST_PLUGIN_INSTALL_TOOL_NAME; use codex_tools::RequestPluginInstallArgs; -use codex_tools::RequestPluginInstallEntry; use codex_tools::RequestPluginInstallResult; use codex_tools::ToolName; use codex_tools::ToolSpec; use codex_tools::all_requested_connectors_picked_up; use codex_tools::build_request_plugin_install_elicitation_request; -use codex_tools::collect_request_plugin_install_entries; use codex_tools::filter_request_plugin_install_discoverable_tools_for_client; use codex_tools::verified_connector_install_completed; use rmcp::model::RequestId; @@ -38,18 +37,7 @@ use crate::tools::handlers::request_plugin_install_spec::create_request_plugin_i use crate::tools::registry::CoreToolRuntime; use crate::tools::registry::ToolExecutor; -#[derive(Default)] -pub struct RequestPluginInstallHandler { - discoverable_tools: Vec, -} - -impl RequestPluginInstallHandler { - pub(crate) fn new(discoverable_tools: &[DiscoverableTool]) -> Self { - Self { - discoverable_tools: collect_request_plugin_install_entries(discoverable_tools), - } - } -} +pub struct RequestPluginInstallHandler; #[async_trait::async_trait] impl ToolExecutor for RequestPluginInstallHandler { @@ -58,7 +46,7 @@ impl ToolExecutor for RequestPluginInstallHandler { } fn spec(&self) -> Option { - Some(create_request_plugin_install_tool(&self.discoverable_tools)) + Some(create_request_plugin_install_tool()) } fn supports_parallel_tool_calls(&self) -> bool { @@ -142,7 +130,7 @@ impl ToolExecutor for RequestPluginInstallHandler { .find(|tool| tool.tool_type() == args.tool_type && tool.id() == args.tool_id) .ok_or_else(|| { FunctionCallError::RespondToModel(format!( - "tool_id must match one of the discoverable tools exposed by {REQUEST_PLUGIN_INSTALL_TOOL_NAME}" + "tool_id must match one of the discoverable tools returned by {LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME}" )) })?; @@ -155,9 +143,10 @@ impl ToolExecutor for RequestPluginInstallHandler { suggest_reason, &tool, ); - let response = session + let elicitation = session .request_mcp_server_elicitation(turn.as_ref(), request_id, params) .await; + let response = elicitation.response; if let Some(response) = response.as_ref() { maybe_persist_disabled_install_request(&session, &turn, &tool, response).await; } @@ -177,6 +166,27 @@ impl ToolExecutor for RequestPluginInstallHandler { .await; } + if elicitation.sent { + let tool_type = match args.tool_type { + DiscoverableToolType::Connector => "connector", + DiscoverableToolType::Plugin => "plugin", + }; + let response_action = match response.as_ref().map(|response| &response.action) { + Some(ElicitationAction::Accept) => "accept", + Some(ElicitationAction::Decline) => "decline", + Some(ElicitationAction::Cancel) => "cancel", + None => "unavailable", + }; + turn.session_telemetry.record_plugin_install_suggestion( + tool_type, + tool.id(), + tool.name(), + response_action, + user_confirmed, + completed, + ); + } + let content = serde_json::to_string(&RequestPluginInstallResult { completed, user_confirmed, diff --git a/codex-rs/core/src/tools/handlers/request_plugin_install_spec.rs b/codex-rs/core/src/tools/handlers/request_plugin_install_spec.rs index d8b0a042c4..ed143283e7 100644 --- a/codex-rs/core/src/tools/handlers/request_plugin_install_spec.rs +++ b/codex-rs/core/src/tools/handlers/request_plugin_install_spec.rs @@ -1,15 +1,11 @@ -use codex_tools::DiscoverableToolType; use codex_tools::JsonSchema; +use codex_tools::LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME; use codex_tools::REQUEST_PLUGIN_INSTALL_TOOL_NAME; -use codex_tools::RequestPluginInstallEntry; use codex_tools::ResponsesApiTool; -use codex_tools::TOOL_SEARCH_TOOL_NAME; use codex_tools::ToolSpec; use std::collections::BTreeMap; -pub(crate) fn create_request_plugin_install_tool( - discoverable_tools: &[RequestPluginInstallEntry], -) -> ToolSpec { +pub(crate) fn create_request_plugin_install_tool() -> ToolSpec { let properties = BTreeMap::from([ ( "tool_type".to_string(), @@ -35,9 +31,8 @@ pub(crate) fn create_request_plugin_install_tool( ), ]); - let discoverable_tools = format_discoverable_tools(discoverable_tools); let description = format!( - "# Request plugin/connector install\n\nUse this tool only to ask the user to install one known plugin or connector from the list below. The list contains known candidates that are not currently installed.\n\nUse this ONLY when all of the following are true:\n- The user explicitly asks to use a specific plugin or connector that is not already available in the current context or active `tools` list.\n- `{TOOL_SEARCH_TOOL_NAME}` is not available, or it has already been called and did not find or make the requested tool callable.\n- The plugin or connector is one of the known installable plugins or connectors listed below. Only ask to install plugins or connectors from this list.\n\nDo not use this tool for adjacent capabilities, broad recommendations, or tools that merely seem useful. Only use when the user explicitly asks to use that exact listed plugin or connector.\n\nKnown plugins/connectors available to install:\n{discoverable_tools}\n\nWorkflow:\n\n1. Check the current context and active `tools` list first. If current active tools aren't relevant and `{TOOL_SEARCH_TOOL_NAME}` is available, only call this tool after `{TOOL_SEARCH_TOOL_NAME}` has already been tried and found no relevant tool.\n2. Match the user's explicit request against the known plugin/connector list above. Only proceed when one listed plugin or connector exactly fits.\n3. If we found both connectors and plugins to install, use plugins first, only use connectors if the corresponding plugin is installed but the connector is not.\n4. If one plugin or connector clearly fits, call `{REQUEST_PLUGIN_INSTALL_TOOL_NAME}` with:\n - `tool_type`: `connector` or `plugin`\n - `action_type`: `install`\n - `tool_id`: exact id from the known plugin/connector list above\n - `suggest_reason`: concise one-line user-facing reason this plugin or connector can help with the current request\n5. After the request flow completes:\n - if the user finished the install flow, continue by searching again or using the newly available plugin or connector\n - if the user did not finish, continue without that plugin or connector, and don't request it again unless the user explicitly asks for it.\n\nIMPORTANT: DO NOT call this tool in parallel with other tools." + "# Request plugin/connector install\n\nUse this tool only after `{LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME}` returns a plugin or connector that exactly matches the user's explicit request.\n\nDo not use it for adjacent capabilities, broad recommendations, or tools that merely seem useful. Pass the returned `tool_type` through directly, and pass the returned `id` as `tool_id`.\n\nIMPORTANT: DO NOT call this tool in parallel with other tools." ); ToolSpec::Function(ResponsesApiTool { @@ -59,74 +54,6 @@ pub(crate) fn create_request_plugin_install_tool( }) } -fn format_discoverable_tools(discoverable_tools: &[RequestPluginInstallEntry]) -> String { - let mut discoverable_tools = discoverable_tools.to_vec(); - discoverable_tools.sort_by(|left, right| { - left.name - .cmp(&right.name) - .then_with(|| left.id.cmp(&right.id)) - }); - - discoverable_tools - .into_iter() - .map(|tool| { - let description = tool_description_or_fallback(&tool); - format!( - "- {} (id: `{}`, type: {}, action: install): {}", - tool.name, - tool.id, - discoverable_tool_type_str(tool.tool_type), - description - ) - }) - .collect::>() - .join("\n") -} - -fn tool_description_or_fallback(tool: &RequestPluginInstallEntry) -> String { - if let Some(description) = tool - .description - .as_deref() - .map(str::trim) - .filter(|description| !description.is_empty()) - { - return description.to_string(); - } - - match tool.tool_type { - DiscoverableToolType::Connector => "No description provided.".to_string(), - DiscoverableToolType::Plugin => plugin_summary(tool), - } -} - -fn plugin_summary(tool: &RequestPluginInstallEntry) -> String { - let mut capabilities = Vec::new(); - if tool.has_skills { - capabilities.push("skills".to_string()); - } - if !tool.mcp_server_names.is_empty() { - capabilities.push(format!("MCP servers: {}", tool.mcp_server_names.join(", "))); - } - if !tool.app_connector_ids.is_empty() { - capabilities.push(format!( - "app connectors: {}", - tool.app_connector_ids.join(", ") - )); - } - if capabilities.is_empty() { - "No description provided.".to_string() - } else { - capabilities.join("; ") - } -} - -fn discoverable_tool_type_str(tool_type: DiscoverableToolType) -> &'static str { - match tool_type { - DiscoverableToolType::Connector => "connector", - DiscoverableToolType::Plugin => "plugin", - } -} - #[cfg(test)] mod tests { use super::*; @@ -135,54 +62,16 @@ mod tests { use std::collections::BTreeMap; #[test] - fn create_request_plugin_install_tool_uses_plugin_summary_fallback() { + fn create_request_plugin_install_tool_uses_expected_wire_shape() { let expected_description = concat!( "# Request plugin/connector install\n\n", - "Use this tool only to ask the user to install one known plugin or connector from the list below. The list contains known candidates that are not currently installed.\n\n", - "Use this ONLY when all of the following are true:\n", - "- The user explicitly asks to use a specific plugin or connector that is not already available in the current context or active `tools` list.\n", - "- `tool_search` is not available, or it has already been called and did not find or make the requested tool callable.\n", - "- The plugin or connector is one of the known installable plugins or connectors listed below. Only ask to install plugins or connectors from this list.\n\n", - "Do not use this tool for adjacent capabilities, broad recommendations, or tools that merely seem useful. Only use when the user explicitly asks to use that exact listed plugin or connector.\n\n", - "Known plugins/connectors available to install:\n", - "- GitHub (id: `github`, type: plugin, action: install): skills; MCP servers: github-mcp; app connectors: github-app\n", - "- Slack (id: `slack@openai-curated`, type: connector, action: install): No description provided.\n\n", - "Workflow:\n\n", - "1. Check the current context and active `tools` list first. If current active tools aren't relevant and `tool_search` is available, only call this tool after `tool_search` has already been tried and found no relevant tool.\n", - "2. Match the user's explicit request against the known plugin/connector list above. Only proceed when one listed plugin or connector exactly fits.\n", - "3. If we found both connectors and plugins to install, use plugins first, only use connectors if the corresponding plugin is installed but the connector is not.\n", - "4. If one plugin or connector clearly fits, call `request_plugin_install` with:\n", - " - `tool_type`: `connector` or `plugin`\n", - " - `action_type`: `install`\n", - " - `tool_id`: exact id from the known plugin/connector list above\n", - " - `suggest_reason`: concise one-line user-facing reason this plugin or connector can help with the current request\n", - "5. After the request flow completes:\n", - " - if the user finished the install flow, continue by searching again or using the newly available plugin or connector\n", - " - if the user did not finish, continue without that plugin or connector, and don't request it again unless the user explicitly asks for it.\n\n", + "Use this tool only after `list_available_plugins_to_install` returns a plugin or connector that exactly matches the user's explicit request.\n\n", + "Do not use it for adjacent capabilities, broad recommendations, or tools that merely seem useful. Pass the returned `tool_type` through directly, and pass the returned `id` as `tool_id`.\n\n", "IMPORTANT: DO NOT call this tool in parallel with other tools.", ); assert_eq!( - create_request_plugin_install_tool(&[ - RequestPluginInstallEntry { - id: "slack@openai-curated".to_string(), - name: "Slack".to_string(), - description: None, - tool_type: DiscoverableToolType::Connector, - has_skills: false, - mcp_server_names: Vec::new(), - app_connector_ids: Vec::new(), - }, - RequestPluginInstallEntry { - id: "github".to_string(), - name: "GitHub".to_string(), - description: None, - tool_type: DiscoverableToolType::Plugin, - has_skills: true, - mcp_server_names: vec!["github-mcp".to_string()], - app_connector_ids: vec!["github-app".to_string()], - }, - ]), + create_request_plugin_install_tool(), ToolSpec::Function(ResponsesApiTool { name: "request_plugin_install".to_string(), description: expected_description.to_string(), diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index d75d56404d..ab9d02f97c 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -7,6 +7,7 @@ use crate::tools::handlers::CodeModeWaitHandler; use crate::tools::handlers::CreateGoalHandler; use crate::tools::handlers::DynamicToolHandler; use crate::tools::handlers::GetGoalHandler; +use crate::tools::handlers::ListAvailablePluginsToInstallHandler; use crate::tools::handlers::ListMcpResourceTemplatesHandler; use crate::tools::handlers::ListMcpResourcesHandler; use crate::tools::handlers::McpHandler; @@ -69,6 +70,7 @@ use codex_tools::ToolOutput; use codex_tools::ToolSpec; use codex_tools::can_request_original_image_detail; use codex_tools::collect_code_mode_exec_prompt_tool_definitions; +use codex_tools::collect_request_plugin_install_entries; use codex_tools::default_namespace_description; use codex_tools::request_user_input_available_modes; use codex_tools::shell_command_backend_for_features; @@ -522,7 +524,10 @@ fn add_core_utility_tools(context: &CoreToolPlanContext<'_>, planned_tools: &mut && let Some(discoverable_tools) = context.discoverable_tools.filter(|tools| !tools.is_empty()) { - planned_tools.add_runtime(RequestPluginInstallHandler::new(discoverable_tools)); + planned_tools.add_runtime(ListAvailablePluginsToInstallHandler::new( + collect_request_plugin_install_entries(discoverable_tools), + )); + planned_tools.add_runtime(RequestPluginInstallHandler); } if environment_mode.has_environment() && turn_context.model_info.apply_patch_tool_type.is_some() diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index 22276af550..a936ac0ea1 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -19,6 +19,7 @@ use codex_protocol::protocol::SubAgentSource; use codex_tools::DiscoverablePluginInfo; use codex_tools::DiscoverableTool; use codex_tools::ResponsesApiNamespaceTool; +use codex_tools::ResponsesApiTool; use codex_tools::ToolExposure; use codex_tools::ToolName; use codex_tools::ToolSpec; @@ -524,7 +525,10 @@ async fn request_plugin_install_requires_all_discovery_features_and_discoverable }, ) .await; - plan.assert_visible_lacks(&["request_plugin_install"]); + plan.assert_visible_lacks(&[ + "list_available_plugins_to_install", + "request_plugin_install", + ]); } let no_candidates = probe(|turn| { @@ -534,7 +538,10 @@ async fn request_plugin_install_requires_all_discovery_features_and_discoverable ); }) .await; - no_candidates.assert_visible_lacks(&["request_plugin_install"]); + no_candidates.assert_visible_lacks(&[ + "list_available_plugins_to_install", + "request_plugin_install", + ]); let enabled = probe_with( |turn| { @@ -549,7 +556,74 @@ async fn request_plugin_install_requires_all_discovery_features_and_discoverable }, ) .await; - enabled.assert_visible_contains(&["request_plugin_install"]); + enabled.assert_visible_contains(&[ + "list_available_plugins_to_install", + "request_plugin_install", + ]); +} + +#[tokio::test] +async fn install_suggestion_tools_stay_visible_without_tool_search() { + let plan = probe_with( + |turn| { + turn.model_info.supports_search_tool = false; + set_features( + turn, + &[Feature::ToolSuggest, Feature::Apps, Feature::Plugins], + ); + }, + ToolPlanInputs { + discoverable_tools: Some(vec![discoverable_plugin("github", "GitHub")]), + ..ToolPlanInputs::default() + }, + ) + .await; + + plan.assert_visible_contains(&[ + "list_available_plugins_to_install", + "request_plugin_install", + ]); + plan.assert_visible_lacks(&["tool_search"]); +} + +#[tokio::test] +async fn request_plugin_install_description_defers_inventory_to_list_tool() { + let plan = probe_with( + |turn| { + set_features( + turn, + &[Feature::ToolSuggest, Feature::Apps, Feature::Plugins], + ); + }, + ToolPlanInputs { + discoverable_tools: Some(vec![discoverable_plugin("github", "GitHub")]), + ..ToolPlanInputs::default() + }, + ) + .await; + + let ToolSpec::Function(ResponsesApiTool { + description: list_description, + .. + }) = plan.visible_spec("list_available_plugins_to_install") + else { + panic!("expected list_available_plugins_to_install function spec"); + }; + assert!(list_description.contains( + "Returns known plugins and connectors that can be passed to `request_plugin_install`." + )); + + let ToolSpec::Function(ResponsesApiTool { + description: request_description, + .. + }) = plan.visible_spec("request_plugin_install") + else { + panic!("expected request_plugin_install function spec"); + }; + assert!(request_description.contains( + "Use this tool only after `list_available_plugins_to_install` returns a plugin or connector that exactly matches the user's explicit request." + )); + assert!(!request_description.contains("github")); } #[tokio::test] diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index a1ba7b8fb2..5f5c7ee85a 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -431,7 +431,7 @@ impl TestCodexBuilder { } else { codex_exec_server::EnvironmentManager::create_for_tests( exec_server_url, - local_runtime_paths, + Some(local_runtime_paths), ) .await }); diff --git a/codex-rs/core/tests/suite/request_plugin_install.rs b/codex-rs/core/tests/suite/request_plugin_install.rs index 443ec7495f..60db47b2ff 100644 --- a/codex-rs/core/tests/suite/request_plugin_install.rs +++ b/codex-rs/core/tests/suite/request_plugin_install.rs @@ -22,6 +22,7 @@ use core_test_support::test_codex::test_codex; use serde_json::Value; const TOOL_SEARCH_TOOL_NAME: &str = "tool_search"; +const LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME: &str = "list_available_plugins_to_install"; const REQUEST_PLUGIN_INSTALL_TOOL_NAME: &str = "request_plugin_install"; const DISCOVERABLE_GMAIL_ID: &str = "connector_68df038e0ba48191908c8434991bbac2"; @@ -125,6 +126,12 @@ async fn request_plugin_install_is_available_without_search_tool_after_discovery !tools.iter().any(|name| name == TOOL_SEARCH_TOOL_NAME), "tools list should not include {TOOL_SEARCH_TOOL_NAME}: {tools:?}" ); + assert!( + tools + .iter() + .any(|name| name == LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME), + "tools list should include {LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME}: {tools:?}" + ); assert!( tools .iter() @@ -132,18 +139,26 @@ async fn request_plugin_install_is_available_without_search_tool_after_discovery "tools list should include {REQUEST_PLUGIN_INSTALL_TOOL_NAME}: {tools:?}" ); + let list_description = + function_tool_description(&body, LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME) + .expect("description"); + assert!(list_description.contains( + "The user explicitly asks to use a specific plugin or connector that is not already available in the current context or active `tools` list." + )); + assert!(list_description.contains( + "`tool_search` is not available, or it has already been called and did not find or make the requested tool callable." + )); + assert!(list_description.contains( + "When both a plugin and a connector match, prefer the plugin; use the connector only when its corresponding plugin is already installed." + )); + let description = function_tool_description(&body, REQUEST_PLUGIN_INSTALL_TOOL_NAME).expect("description"); assert!(description.contains( - "Use this tool only to ask the user to install one known plugin or connector from the list below" - )); - assert!(description.contains( - "`tool_search` is not available, or it has already been called and did not find or make the requested tool callable." - )); - assert!(description.contains( - "Only use when the user explicitly asks to use that exact listed plugin or connector." + "Use this tool only after `list_available_plugins_to_install` returns a plugin or connector that exactly matches the user's explicit request." )); assert!(description.contains("IMPORTANT: DO NOT call this tool in parallel with other tools.")); + assert!(!description.contains(DISCOVERABLE_GMAIL_ID)); assert!(!description.contains("tool_search fails to find a good match")); Ok(()) diff --git a/codex-rs/exec-server/src/connection.rs b/codex-rs/exec-server/src/connection.rs index cf504bbea7..b211c504ac 100644 --- a/codex-rs/exec-server/src/connection.rs +++ b/codex-rs/exec-server/src/connection.rs @@ -323,37 +323,20 @@ impl JsonRpcConnection { where S: AsyncRead + AsyncWrite + Unpin + Send + 'static, { - let (websocket_writer, websocket_reader) = stream.split(); - Self::from_websocket_parts( - websocket_writer, - websocket_reader, - connection_label, - Some(WEBSOCKET_KEEPALIVE_INTERVAL), - ) + Self::from_websocket_stream(stream, connection_label, /*ping_interval*/ None) } pub(crate) fn from_axum_websocket(stream: AxumWebSocket, connection_label: String) -> Self { - let (websocket_writer, websocket_reader) = stream.split(); - Self::from_websocket_parts( - websocket_writer, - websocket_reader, - connection_label, - // Axum only wraps inbound exec-server websocket accepts. Outbound websocket clients - // own keepalive pings so one side does not accidentally create redundant traffic. - /*keepalive_interval*/ - None, - ) + Self::from_websocket_stream(stream, connection_label, Some(WEBSOCKET_KEEPALIVE_INTERVAL)) } - fn from_websocket_parts( - mut websocket_writer: W, - mut websocket_reader: R, + fn from_websocket_stream( + mut websocket: T, connection_label: String, - keepalive_interval: Option, + ping_interval: Option, ) -> Self where - W: Sink + Unpin + Send + 'static, - R: Stream> + Unpin + Send + 'static, + T: Sink + Stream> + Unpin + Send + 'static, M: JsonRpcWebSocketMessage, E: std::fmt::Display + Send + 'static, { @@ -361,118 +344,106 @@ impl JsonRpcConnection { let (incoming_tx, incoming_rx) = mpsc::channel(CHANNEL_CAPACITY); let (disconnected_tx, disconnected_rx) = watch::channel(false); - let reader_label = connection_label.clone(); - let incoming_tx_for_reader = incoming_tx.clone(); - let disconnected_tx_for_reader = disconnected_tx.clone(); - let reader_task = tokio::spawn(async move { + let websocket_task = tokio::spawn(async move { + let mut ping_interval = ping_interval.map(|ping_interval| { + let mut interval = tokio::time::interval_at( + tokio::time::Instant::now() + ping_interval, + ping_interval, + ); + interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); + interval + }); + loop { - match websocket_reader.next().await { - Some(Ok(message)) => match message.parse_jsonrpc_frame() { - Ok(JsonRpcWebSocketFrame::Message(message)) => { - if incoming_tx_for_reader - .send(JsonRpcConnectionEvent::Message(message)) - .await - .is_err() - { - break; - } + tokio::select! { + maybe_message = outgoing_rx.recv() => { + let Some(message) = maybe_message else { + break; + }; + if let Err(reason) = send_websocket_jsonrpc_message( + &mut websocket, + &connection_label, + &message, + ) + .await + { + send_disconnected(&incoming_tx, &disconnected_tx, Some(reason)).await; + break; } - Err(err) => { - send_malformed_message( - &incoming_tx_for_reader, - Some(format!( - "failed to parse websocket JSON-RPC message from {reader_label}: {err}" - )), - ) - .await; + } + _ = async { + match ping_interval.as_mut() { + Some(interval) => interval.tick().await, + None => std::future::pending().await, } - Ok(JsonRpcWebSocketFrame::Close) => { + } => { + if let Err(err) = websocket.send(M::ping()).await { send_disconnected( - &incoming_tx_for_reader, - &disconnected_tx_for_reader, - /*reason*/ None, + &incoming_tx, + &disconnected_tx, + Some(format!( + "failed to write websocket ping to {connection_label}: {err}" + )), ) .await; break; } - Ok(JsonRpcWebSocketFrame::Ignore) => {} - }, - Some(Err(err)) => { - send_disconnected( - &incoming_tx_for_reader, - &disconnected_tx_for_reader, - Some(format!( - "failed to read websocket JSON-RPC message from {reader_label}: {err}" - )), - ) - .await; - break; } - None => { - send_disconnected( - &incoming_tx_for_reader, - &disconnected_tx_for_reader, - /*reason*/ None, - ) - .await; - break; - } - } - } - }); - - let writer_task = tokio::spawn(async move { - if let Some(keepalive_interval) = keepalive_interval { - let mut keepalive = tokio::time::interval_at( - tokio::time::Instant::now() + keepalive_interval, - keepalive_interval, - ); - keepalive.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); - loop { - tokio::select! { - maybe_message = outgoing_rx.recv() => { - let Some(message) = maybe_message else { - break; - }; - if let Err(reason) = send_websocket_jsonrpc_message( - &mut websocket_writer, - &connection_label, - &message, - ) - .await - { - send_disconnected(&incoming_tx, &disconnected_tx, Some(reason)).await; - break; - } - } - _ = keepalive.tick() => { - if let Err(err) = websocket_writer.send(M::ping()).await { + incoming_message = websocket.next() => { + match incoming_message { + Some(Ok(message)) => match message.parse_jsonrpc_frame() { + Ok(JsonRpcWebSocketFrame::Message(message)) => { + if incoming_tx + .send(JsonRpcConnectionEvent::Message(message)) + .await + .is_err() + { + break; + } + } + Ok(JsonRpcWebSocketFrame::Close) => { + send_disconnected( + &incoming_tx, + &disconnected_tx, + /*reason*/ None, + ) + .await; + break; + } + Ok(JsonRpcWebSocketFrame::Ignore) => {} + Err(err) => { + send_malformed_message( + &incoming_tx, + Some(format!( + "failed to parse websocket JSON-RPC message from {connection_label}: {err}" + )), + ) + .await; + } + }, + Some(Err(err)) => { send_disconnected( &incoming_tx, &disconnected_tx, Some(format!( - "failed to write websocket ping to {connection_label}: {err}" + "failed to read websocket JSON-RPC message from {connection_label}: {err}" )), ) .await; break; } + None => { + send_disconnected( + &incoming_tx, + &disconnected_tx, + /*reason*/ None, + ) + .await; + break; + } } } } - } else { - while let Some(message) = outgoing_rx.recv().await { - if let Err(reason) = send_websocket_jsonrpc_message( - &mut websocket_writer, - &connection_label, - &message, - ) - .await - { - send_disconnected(&incoming_tx, &disconnected_tx, Some(reason)).await; - break; - } - } } }); @@ -480,7 +451,7 @@ impl JsonRpcConnection { outgoing_tx, incoming_rx, disconnected_rx, - task_handles: vec![reader_task, writer_task], + task_handles: vec![websocket_task], transport: JsonRpcTransport::Plain, } } @@ -619,34 +590,250 @@ fn serialize_jsonrpc_message(message: &JSONRPCMessage) -> Result, + #[tokio::test] + async fn websocket_connection_sends_configured_ping() -> anyhow::Result<()> { + let (client_websocket, mut server_websocket) = websocket_pair().await?; + let connection = JsonRpcConnection::from_websocket_stream( + client_websocket, + "test".into(), + Some(WEBSOCKET_KEEPALIVE_INTERVAL), + ); + + let message = timeout(Duration::from_secs(1), server_websocket.next()) + .await? + .expect("websocket should stay open")?; + assert!(matches!(message, Message::Ping(_))); + + drop(connection); + Ok(()) } - impl Sink for TestWebSocketSink { + #[tokio::test] + async fn websocket_connection_ignores_server_pong() -> anyhow::Result<()> { + let (client_websocket, mut server_websocket) = websocket_pair().await?; + let mut connection = JsonRpcConnection::from_websocket(client_websocket, "test".into()); + + server_websocket + .send(Message::Pong(b"check".to_vec().into())) + .await?; + assert!( + timeout(Duration::from_millis(50), connection.incoming_rx.recv()) + .await + .is_err() + ); + + drop(connection); + Ok(()) + } + + #[tokio::test] + async fn websocket_connection_reports_server_close() -> anyhow::Result<()> { + let (client_websocket, mut server_websocket) = websocket_pair().await?; + let mut connection = JsonRpcConnection::from_websocket(client_websocket, "test".into()); + + server_websocket.close(None).await?; + assert!(matches!( + timeout(Duration::from_secs(1), connection.incoming_rx.recv()).await?, + Some(JsonRpcConnectionEvent::Disconnected { reason: None }) + )); + + drop(connection); + Ok(()) + } + + #[tokio::test] + async fn websocket_connection_accepts_binary_jsonrpc_message() -> anyhow::Result<()> { + let (client_websocket, mut server_websocket) = websocket_pair().await?; + let mut connection = JsonRpcConnection::from_websocket(client_websocket, "test".into()); + let message = JSONRPCMessage::Request(JSONRPCRequest { + id: RequestId::Integer(1), + method: "test".to_string(), + params: None, + trace: None, + }); + + server_websocket + .send(Message::Binary(serde_json::to_vec(&message)?.into())) + .await?; + assert!(matches!( + timeout(Duration::from_secs(1), connection.incoming_rx.recv()).await?, + Some(JsonRpcConnectionEvent::Message(actual)) if actual == message + )); + + drop(connection); + Ok(()) + } + + #[tokio::test] + async fn websocket_connection_keeps_outbound_message_while_send_is_backpressured() + -> anyhow::Result<()> { + let (websocket, control, mut outbound_rx) = + ControlledWebSocket::new(/*write_ready*/ false); + let mut connection = JsonRpcConnection::from_websocket_stream( + websocket, + "test".into(), + /*ping_interval*/ None, + ); + let message = test_jsonrpc_message(); + + connection.outgoing_tx.send(message.clone()).await?; + control.wait_for_blocked_write().await?; + control.send_inbound(Message::Pong(b"check".to_vec().into()))?; + assert!( + timeout(Duration::from_millis(50), connection.incoming_rx.recv()) + .await + .is_err() + ); + + control.set_write_ready(); + assert!(matches!( + timeout(Duration::from_secs(1), outbound_rx.next()).await?, + Some(Message::Text(text)) if serde_json::from_str::(&text)? == message + )); + drop(connection); + Ok(()) + } + + async fn websocket_pair() -> anyhow::Result<( + WebSocketStream>, + WebSocketStream, + )> { + let listener = TcpListener::bind("127.0.0.1:0").await?; + let websocket_url = format!("ws://{}", listener.local_addr()?); + let server_task = tokio::spawn(async move { + let (stream, _) = listener.accept().await?; + accept_async(stream).await.map_err(anyhow::Error::from) + }); + let (client_websocket, _) = connect_async(websocket_url).await?; + let server_websocket = server_task.await??; + Ok((client_websocket, server_websocket)) + } + + fn test_jsonrpc_message() -> JSONRPCMessage { + JSONRPCMessage::Request(JSONRPCRequest { + id: RequestId::Integer(1), + method: "test".to_string(), + params: None, + trace: None, + }) + } + + struct ControlledWebSocket { + inbound_rx: futures_mpsc::UnboundedReceiver>, + outbound_tx: futures_mpsc::UnboundedSender, + write_ready: Arc, + write_blocked: Arc, + write_blocked_waker: Arc, + write_waker: Arc, + } + + struct ControlledWebSocketHandle { + inbound_tx: futures_mpsc::UnboundedSender>, + write_ready: Arc, + write_blocked: Arc, + write_blocked_waker: Arc, + write_waker: Arc, + } + + impl ControlledWebSocket { + fn new( + write_ready: bool, + ) -> ( + Self, + ControlledWebSocketHandle, + futures_mpsc::UnboundedReceiver, + ) { + let (inbound_tx, inbound_rx) = futures_mpsc::unbounded(); + let (outbound_tx, outbound_rx) = futures_mpsc::unbounded(); + let write_ready = Arc::new(AtomicBool::new(write_ready)); + let write_blocked = Arc::new(AtomicBool::new(false)); + let write_blocked_waker = Arc::new(AtomicWaker::new()); + let write_waker = Arc::new(AtomicWaker::new()); + ( + Self { + inbound_rx, + outbound_tx, + write_ready: Arc::clone(&write_ready), + write_blocked: Arc::clone(&write_blocked), + write_blocked_waker: Arc::clone(&write_blocked_waker), + write_waker: Arc::clone(&write_waker), + }, + ControlledWebSocketHandle { + inbound_tx, + write_ready, + write_blocked, + write_blocked_waker, + write_waker, + }, + outbound_rx, + ) + } + } + + impl ControlledWebSocketHandle { + fn send_inbound(&self, message: Message) -> anyhow::Result<()> { + self.inbound_tx + .unbounded_send(Ok(message)) + .map_err(anyhow::Error::from) + } + + fn set_write_ready(&self) { + self.write_ready.store(true, Ordering::Release); + self.write_waker.wake(); + } + + async fn wait_for_blocked_write(&self) -> anyhow::Result<()> { + timeout( + Duration::from_secs(1), + futures::future::poll_fn(|cx| { + if self.write_blocked.load(Ordering::Acquire) { + Poll::Ready(()) + } else { + self.write_blocked_waker.register(cx.waker()); + Poll::Pending + } + }), + ) + .await?; + Ok(()) + } + } + + impl Sink for ControlledWebSocket { type Error = std::convert::Infallible; - fn poll_ready( - self: Pin<&mut Self>, - _cx: &mut Context<'_>, - ) -> Poll> { - Poll::Ready(Ok(())) + fn poll_ready(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + if self.write_ready.load(Ordering::Acquire) { + Poll::Ready(Ok(())) + } else { + self.write_blocked.store(true, Ordering::Release); + self.write_blocked_waker.wake(); + self.write_waker.register(cx.waker()); + Poll::Pending + } } fn start_send(self: Pin<&mut Self>, item: Message) -> Result<(), Self::Error> { - self.get_mut() - .message_tx + self.outbound_tx .unbounded_send(item) - .expect("test websocket receiver should stay open"); + .expect("test outbound receiver should stay open"); Ok(()) } @@ -665,24 +852,11 @@ mod tests { } } - #[tokio::test] - async fn websocket_connection_sends_keepalive_ping() { - let (message_tx, mut message_rx) = futures_mpsc::unbounded::(); - let websocket_writer = TestWebSocketSink { message_tx }; - let websocket_reader = stream::pending::>(); - let connection = JsonRpcConnection::from_websocket_parts( - websocket_writer, - websocket_reader, - "test".into(), - Some(WEBSOCKET_KEEPALIVE_INTERVAL), - ); + impl Stream for ControlledWebSocket { + type Item = Result; - let message = timeout(Duration::from_secs(1), message_rx.next()) - .await - .expect("keepalive ping should arrive before timeout") - .expect("keepalive ping should be sent"); - assert!(matches!(message, Message::Ping(_))); - - drop(connection); + fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + Pin::new(&mut self.inbound_rx).poll_next(cx) + } } } diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 1d7d6e75f5..55dc031273 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -27,12 +27,11 @@ pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL"; /// /// `EnvironmentManager` is a shared registry for concrete environments. Its /// default constructor preserves the legacy `CODEX_EXEC_SERVER_URL` behavior -/// while provider-based construction accepts a provider-supplied snapshot. +/// while configured construction accepts a provider-supplied snapshot. /// /// Setting `CODEX_EXEC_SERVER_URL=none` disables environment access by leaving -/// the default environment unset while still keeping an explicit local -/// environment available through `local_environment()`. Callers use -/// `default_environment().is_some()` as the signal for model-facing +/// the default environment unset and omitting the local environment. Callers +/// use `default_environment().is_some()` as the signal for model-facing /// shell/filesystem tool availability. /// /// Remote environments create remote filesystem and execution backends that @@ -42,25 +41,13 @@ pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL"; pub struct EnvironmentManager { default_environment: Option, environments: RwLock>>, - local_environment: Arc, + local_environment: Option>, + local_runtime_paths: Option, } pub const LOCAL_ENVIRONMENT_ID: &str = "local"; pub const REMOTE_ENVIRONMENT_ID: &str = "remote"; -#[derive(Clone, Debug)] -pub struct EnvironmentManagerArgs { - pub local_runtime_paths: ExecServerRuntimePaths, -} - -impl EnvironmentManagerArgs { - pub fn new(local_runtime_paths: ExecServerRuntimePaths) -> Self { - Self { - local_runtime_paths, - } - } -} - impl EnvironmentManager { /// Builds a test-only manager without configured sandbox helper paths. pub fn default_for_tests() -> Self { @@ -70,37 +57,29 @@ impl EnvironmentManager { LOCAL_ENVIRONMENT_ID.to_string(), Arc::new(Environment::default_for_tests()), )])), - local_environment: Arc::new(Environment::default_for_tests()), + local_environment: Some(Arc::new(Environment::default_for_tests())), + local_runtime_paths: None, } } - /// Builds a test-only manager with environment access disabled. - pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self { + /// Builds a manager with no configured execution environments. + pub fn without_environments() -> Self { Self { default_environment: None, environments: RwLock::new(HashMap::new()), - local_environment: Arc::new(Environment::local(local_runtime_paths)), + local_environment: None, + local_runtime_paths: None, } } /// Builds a test-only manager from a raw exec-server URL value. pub async fn create_for_tests( exec_server_url: Option, - local_runtime_paths: ExecServerRuntimePaths, + local_runtime_paths: Option, ) -> Self { Self::from_default_provider_url(exec_server_url, local_runtime_paths).await } - /// Builds a manager from `CODEX_EXEC_SERVER_URL` and local runtime paths - /// used when creating local filesystem helpers. - pub async fn new(args: EnvironmentManagerArgs) -> Self { - let EnvironmentManagerArgs { - local_runtime_paths, - } = args; - let exec_server_url = std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok(); - Self::from_default_provider_url(exec_server_url, local_runtime_paths).await - } - /// Builds a manager from `CODEX_HOME` and local runtime paths used when /// creating local filesystem helpers. /// @@ -109,27 +88,27 @@ impl EnvironmentManager { /// `CODEX_EXEC_SERVER_URL` behavior. pub async fn from_codex_home( codex_home: impl AsRef, - local_runtime_paths: ExecServerRuntimePaths, + local_runtime_paths: Option, ) -> Result { let provider = environment_provider_from_codex_home(codex_home.as_ref())?; - Self::from_provider(provider.as_ref(), local_runtime_paths).await + Self::from_snapshot(provider.snapshot().await?, local_runtime_paths) } /// Builds a manager from the legacy environment-variable provider without /// reading user config files from `CODEX_HOME`. pub async fn from_env( - local_runtime_paths: ExecServerRuntimePaths, + local_runtime_paths: Option, ) -> Result { let provider = DefaultEnvironmentProvider::from_env(); - Self::from_provider(&provider, local_runtime_paths).await + Self::from_snapshot(provider.snapshot().await?, local_runtime_paths) } async fn from_default_provider_url( exec_server_url: Option, - local_runtime_paths: ExecServerRuntimePaths, + local_runtime_paths: Option, ) -> Self { let provider = DefaultEnvironmentProvider::new(exec_server_url); - match Self::from_provider(&provider, local_runtime_paths).await { + match Self::from_snapshot(provider.snapshot_inner(), local_runtime_paths) { Ok(manager) => manager, Err(err) => panic!("default provider should create valid environments: {err}"), } @@ -143,26 +122,15 @@ impl EnvironmentManager { ) -> Self { let mut snapshot = DefaultEnvironmentProvider::new(exec_server_url).snapshot_inner(); snapshot.include_local = true; - match Self::from_provider_snapshot(snapshot, local_runtime_paths) { + match Self::from_snapshot(snapshot, Some(local_runtime_paths)) { Ok(manager) => manager, Err(err) => panic!("test provider with local should create valid environments: {err}"), } } - /// Builds a manager from a provider-supplied startup snapshot. - pub async fn from_provider

( - provider: &P, - local_runtime_paths: ExecServerRuntimePaths, - ) -> Result - where - P: EnvironmentProvider + ?Sized, - { - Self::from_provider_snapshot(provider.snapshot().await?, local_runtime_paths) - } - - fn from_provider_snapshot( + fn from_snapshot( snapshot: EnvironmentProviderSnapshot, - local_runtime_paths: ExecServerRuntimePaths, + local_runtime_paths: Option, ) -> Result { let EnvironmentProviderSnapshot { environments, @@ -171,13 +139,21 @@ impl EnvironmentManager { } = snapshot; let mut environment_map = HashMap::with_capacity(environments.len() + usize::from(include_local)); - let local_environment = Arc::new(Environment::local(local_runtime_paths)); - if include_local { + let local_environment = if include_local { + let local_runtime_paths = local_runtime_paths.clone().ok_or_else(|| { + ExecServerError::Protocol( + "local environment requires configured runtime paths".to_string(), + ) + })?; + let local_environment = Arc::new(Environment::local(local_runtime_paths)); environment_map.insert( LOCAL_ENVIRONMENT_ID.to_string(), Arc::clone(&local_environment), ); - } + Some(local_environment) + } else { + None + }; for (id, environment) in environments { if id.is_empty() { return Err(ExecServerError::Protocol( @@ -213,6 +189,7 @@ impl EnvironmentManager { default_environment, environments: RwLock::new(environment_map), local_environment, + local_runtime_paths, }) } @@ -248,9 +225,15 @@ impl EnvironmentManager { environment_ids } - /// Returns the local environment instance used for internal runtime work. - pub fn local_environment(&self) -> Arc { - Arc::clone(&self.local_environment) + /// Returns the local environment instance when one is configured. + pub fn try_local_environment(&self) -> Option> { + self.local_environment.as_ref().map(Arc::clone) + } + + /// Returns the default environment or local environment when either exists. + pub fn default_or_local_environment(&self) -> Option> { + self.default_environment() + .or_else(|| self.try_local_environment()) } /// Returns a named environment instance. @@ -285,10 +268,8 @@ impl EnvironmentManager { "remote environment requires an exec-server url".to_string(), )); }; - let environment = Environment::remote_inner( - exec_server_url, - self.local_environment.local_runtime_paths.clone(), - ); + let environment = + Environment::remote_inner(exec_server_url, self.local_runtime_paths.clone()); self.environments .write() .unwrap_or_else(std::sync::PoisonError::into_inner) @@ -452,25 +433,12 @@ mod tests { use super::EnvironmentManager; use super::LOCAL_ENVIRONMENT_ID; use super::REMOTE_ENVIRONMENT_ID; - use crate::EnvironmentProvider; - use crate::ExecServerError; use crate::ExecServerRuntimePaths; use crate::ProcessId; use crate::environment_provider::EnvironmentDefault; use crate::environment_provider::EnvironmentProviderSnapshot; use pretty_assertions::assert_eq; - struct TestEnvironmentProvider { - snapshot: EnvironmentProviderSnapshot, - } - - #[async_trait::async_trait] - impl EnvironmentProvider for TestEnvironmentProvider { - async fn snapshot(&self) -> Result { - Ok(self.snapshot.clone()) - } - } - fn test_runtime_paths() -> ExecServerRuntimePaths { ExecServerRuntimePaths::new( std::env::current_exe().expect("current exe"), @@ -479,6 +447,10 @@ mod tests { .expect("runtime paths") } + fn assert_local_environment_unavailable(manager: &EnvironmentManager) { + assert!(manager.try_local_environment().is_none()); + } + #[tokio::test] async fn create_local_environment_does_not_connect() { let environment = Environment::create(/*exec_server_url*/ None, test_runtime_paths()) @@ -491,7 +463,8 @@ mod tests { #[tokio::test] async fn environment_manager_normalizes_empty_url() { let manager = - EnvironmentManager::create_for_tests(Some(String::new()), test_runtime_paths()).await; + EnvironmentManager::create_for_tests(Some(String::new()), Some(test_runtime_paths())) + .await; let environment = manager.default_environment().expect("default environment"); assert_eq!(manager.default_environment_id(), Some(LOCAL_ENVIRONMENT_ID)); @@ -501,18 +474,22 @@ mod tests { .get_environment(LOCAL_ENVIRONMENT_ID) .expect("local environment") )); - assert!(Arc::ptr_eq(&environment, &manager.local_environment())); + assert!(Arc::ptr_eq( + &environment, + &manager.try_local_environment().expect("local environment") + )); + assert!(manager.try_local_environment().is_some()); assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none()); assert!(!environment.is_remote()); } #[tokio::test] - async fn disabled_environment_manager_has_no_default_but_keeps_explicit_local_environment() { - let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); + async fn disabled_environment_manager_has_no_default_or_local_environment() { + let manager = EnvironmentManager::without_environments(); assert!(manager.default_environment().is_none()); assert_eq!(manager.default_environment_id(), None); - assert!(!manager.local_environment().is_remote()); + assert_local_environment_unavailable(&manager); assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none()); assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none()); } @@ -521,7 +498,7 @@ mod tests { async fn environment_manager_reports_remote_url() { let manager = EnvironmentManager::create_for_tests( Some("ws://127.0.0.1:8765".to_string()), - test_runtime_paths(), + Some(test_runtime_paths()), ) .await; @@ -539,7 +516,7 @@ mod tests { .expect("remote environment") )); assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none()); - assert!(!manager.local_environment().is_remote()); + assert_local_environment_unavailable(&manager); } #[tokio::test] @@ -557,20 +534,17 @@ mod tests { } #[tokio::test] - async fn environment_manager_builds_from_provider() { - let provider = TestEnvironmentProvider { - snapshot: EnvironmentProviderSnapshot { - environments: vec![( - REMOTE_ENVIRONMENT_ID.to_string(), - Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) - .expect("remote environment"), - )], - default: EnvironmentDefault::EnvironmentId(REMOTE_ENVIRONMENT_ID.to_string()), - include_local: false, - }, + async fn environment_manager_builds_from_snapshot() { + let snapshot = EnvironmentProviderSnapshot { + environments: vec![( + REMOTE_ENVIRONMENT_ID.to_string(), + Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) + .expect("remote environment"), + )], + default: EnvironmentDefault::EnvironmentId(REMOTE_ENVIRONMENT_ID.to_string()), + include_local: false, }; - let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths()) - .await + let manager = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths())) .expect("environment manager"); assert_eq!( @@ -584,20 +558,17 @@ mod tests { .is_remote() ); assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none()); - assert!(!manager.local_environment().is_remote()); + assert_local_environment_unavailable(&manager); } #[tokio::test] async fn environment_manager_rejects_empty_environment_id() { - let provider = TestEnvironmentProvider { - snapshot: EnvironmentProviderSnapshot { - environments: vec![("".to_string(), Environment::default_for_tests())], - default: EnvironmentDefault::Disabled, - include_local: false, - }, + let snapshot = EnvironmentProviderSnapshot { + environments: vec![("".to_string(), Environment::default_for_tests())], + default: EnvironmentDefault::Disabled, + include_local: false, }; - let err = EnvironmentManager::from_provider(&provider, test_runtime_paths()) - .await + let err = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths())) .expect_err("empty id should fail"); assert_eq!( @@ -608,18 +579,15 @@ mod tests { #[tokio::test] async fn environment_manager_rejects_provider_supplied_local_environment() { - let provider = TestEnvironmentProvider { - snapshot: EnvironmentProviderSnapshot { - environments: vec![( - LOCAL_ENVIRONMENT_ID.to_string(), - Environment::default_for_tests(), - )], - default: EnvironmentDefault::Disabled, - include_local: false, - }, + let snapshot = EnvironmentProviderSnapshot { + environments: vec![( + LOCAL_ENVIRONMENT_ID.to_string(), + Environment::default_for_tests(), + )], + default: EnvironmentDefault::Disabled, + include_local: false, }; - let err = EnvironmentManager::from_provider(&provider, test_runtime_paths()) - .await + let err = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths())) .expect_err("local id should fail"); assert_eq!( @@ -630,19 +598,16 @@ mod tests { #[tokio::test] async fn environment_manager_uses_explicit_provider_default() { - let provider = TestEnvironmentProvider { - snapshot: EnvironmentProviderSnapshot { - environments: vec![( - "devbox".to_string(), - Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) - .expect("remote environment"), - )], - default: EnvironmentDefault::EnvironmentId("devbox".to_string()), - include_local: true, - }, + let snapshot = EnvironmentProviderSnapshot { + environments: vec![( + "devbox".to_string(), + Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) + .expect("remote environment"), + )], + default: EnvironmentDefault::EnvironmentId("devbox".to_string()), + include_local: true, }; - let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths()) - .await + let manager = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths())) .expect("manager"); assert_eq!(manager.default_environment_id(), Some("devbox")); @@ -655,19 +620,16 @@ mod tests { #[tokio::test] async fn environment_manager_disables_provider_default() { - let provider = TestEnvironmentProvider { - snapshot: EnvironmentProviderSnapshot { - environments: vec![( - "devbox".to_string(), - Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) - .expect("remote environment"), - )], - default: EnvironmentDefault::Disabled, - include_local: true, - }, + let snapshot = EnvironmentProviderSnapshot { + environments: vec![( + "devbox".to_string(), + Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) + .expect("remote environment"), + )], + default: EnvironmentDefault::Disabled, + include_local: true, }; - let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths()) - .await + let manager = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths())) .expect("manager"); assert_eq!(manager.default_environment_id(), None); @@ -676,25 +638,22 @@ mod tests { &manager .get_environment(LOCAL_ENVIRONMENT_ID) .expect("local environment"), - &manager.local_environment() + &manager.try_local_environment().expect("local environment") )); } #[tokio::test] async fn environment_manager_rejects_unknown_provider_default() { - let provider = TestEnvironmentProvider { - snapshot: EnvironmentProviderSnapshot { - environments: vec![( - "devbox".to_string(), - Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) - .expect("remote environment"), - )], - default: EnvironmentDefault::EnvironmentId("missing".to_string()), - include_local: true, - }, + let snapshot = EnvironmentProviderSnapshot { + environments: vec![( + "devbox".to_string(), + Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) + .expect("remote environment"), + )], + default: EnvironmentDefault::EnvironmentId("missing".to_string()), + include_local: true, }; - let err = EnvironmentManager::from_provider(&provider, test_runtime_paths()) - .await + let err = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths())) .expect_err("unknown default should fail"); assert_eq!( @@ -707,7 +666,7 @@ mod tests { async fn environment_manager_includes_local_for_default_provider_without_url() { let manager = EnvironmentManager::create_for_tests( /*exec_server_url*/ None, - test_runtime_paths(), + Some(test_runtime_paths()), ) .await; @@ -719,7 +678,10 @@ mod tests { .get_environment(LOCAL_ENVIRONMENT_ID) .expect("local environment") )); - assert!(Arc::ptr_eq(&environment, &manager.local_environment())); + assert!(Arc::ptr_eq( + &environment, + &manager.try_local_environment().expect("local environment") + )); assert!(!environment.is_remote()); } @@ -728,44 +690,59 @@ mod tests { let runtime_paths = test_runtime_paths(); let manager = EnvironmentManager::create_for_tests( /*exec_server_url*/ None, - runtime_paths.clone(), + Some(runtime_paths.clone()), ) .await; - let environment = manager.local_environment(); + let environment = manager.try_local_environment().expect("local environment"); assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths)); let manager = EnvironmentManager::create_for_tests( environment.exec_server_url().map(str::to_owned), - environment - .local_runtime_paths() - .expect("local runtime paths") - .clone(), + Some( + environment + .local_runtime_paths() + .expect("local runtime paths") + .clone(), + ), ) .await; - let environment = manager.local_environment(); + let environment = manager.try_local_environment().expect("local environment"); assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths)); } - #[tokio::test] - async fn disabled_environment_manager_has_no_default_environment() { - let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); - - assert!(manager.default_environment().is_none()); - assert_eq!(manager.default_environment_id(), None); - } - #[tokio::test] async fn environment_manager_omits_default_provider_local_lookup_when_default_disabled() { - let manager = - EnvironmentManager::create_for_tests(Some("none".to_string()), test_runtime_paths()) - .await; + let manager = EnvironmentManager::create_for_tests( + Some("none".to_string()), + Some(test_runtime_paths()), + ) + .await; assert!(manager.default_environment().is_none()); assert_eq!(manager.default_environment_id(), None); assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none()); assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none()); - assert!(!manager.local_environment().is_remote()); + assert_local_environment_unavailable(&manager); + } + + #[tokio::test] + async fn environment_manager_snapshot_without_local_environment_disables_local_default() { + let mut snapshot = EnvironmentProviderSnapshot { + environments: Vec::new(), + default: EnvironmentDefault::EnvironmentId(LOCAL_ENVIRONMENT_ID.to_string()), + include_local: true, + }; + snapshot.include_local = false; + snapshot.default = EnvironmentDefault::Disabled; + let manager = + EnvironmentManager::from_snapshot(snapshot, /*local_runtime_paths*/ None) + .expect("environment manager"); + + assert!(manager.default_environment().is_none()); + assert_eq!(manager.default_environment_id(), None); + assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none()); + assert_local_environment_unavailable(&manager); } #[tokio::test] @@ -777,7 +754,7 @@ mod tests { #[tokio::test] async fn environment_manager_upserts_named_remote_environment() { - let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); + let manager = EnvironmentManager::without_environments(); manager .upsert_environment("executor-a".to_string(), "ws://127.0.0.1:8765".to_string()) @@ -802,7 +779,7 @@ mod tests { #[tokio::test] async fn environment_manager_rejects_empty_remote_environment_url() { - let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); + let manager = EnvironmentManager::without_environments(); let err = manager .upsert_environment("executor-a".to_string(), String::new()) diff --git a/codex-rs/exec-server/src/environment_toml.rs b/codex-rs/exec-server/src/environment_toml.rs index 90f4c78262..26c178b5b8 100644 --- a/codex-rs/exec-server/src/environment_toml.rs +++ b/codex-rs/exec-server/src/environment_toml.rs @@ -27,6 +27,7 @@ const MAX_ENVIRONMENT_ID_LEN: usize = 64; #[serde(deny_unknown_fields)] struct EnvironmentsToml { default: Option, + include_local: Option, #[serde(default)] environments: Vec, @@ -50,6 +51,7 @@ struct EnvironmentToml { #[derive(Clone, Debug, PartialEq, Eq)] struct TomlEnvironmentProvider { default: EnvironmentDefault, + include_local: bool, environments: Vec<(String, ExecServerTransportParams)>, } @@ -63,21 +65,31 @@ impl TomlEnvironmentProvider { config: EnvironmentsToml, config_dir: Option<&Path>, ) -> Result { - let mut ids = HashSet::from([LOCAL_ENVIRONMENT_ID.to_string()]); - let mut environments = Vec::with_capacity(config.environments.len()); - for item in config.environments { + let EnvironmentsToml { + default, + include_local, + environments, + } = config; + let include_local = include_local.unwrap_or(true); + let mut ids = HashSet::new(); + if include_local { + ids.insert(LOCAL_ENVIRONMENT_ID.to_string()); + } + let mut parsed_environments = Vec::with_capacity(environments.len()); + for item in environments { let (id, transport) = parse_environment_toml(item, config_dir)?; if !ids.insert(id.clone()) { return Err(ExecServerError::Protocol(format!( "environment id `{id}` is duplicated" ))); } - environments.push((id, transport)); + parsed_environments.push((id, transport)); } - let default = normalize_default_environment_id(config.default.as_deref(), &ids)?; + let default = normalize_default_environment_id(default.as_deref(), include_local, &ids)?; Ok(Self { default, - environments, + include_local, + environments: parsed_environments, }) } } @@ -99,7 +111,7 @@ impl EnvironmentProvider for TomlEnvironmentProvider { Ok(EnvironmentProviderSnapshot { environments, default: self.default.clone(), - include_local: true, + include_local: self.include_local, }) } } @@ -212,12 +224,17 @@ pub(crate) fn environment_provider_from_codex_home( fn normalize_default_environment_id( default: Option<&str>, + include_local: bool, ids: &HashSet, ) -> Result { let Some(default) = default.map(str::trim) else { - return Ok(EnvironmentDefault::EnvironmentId( - LOCAL_ENVIRONMENT_ID.to_string(), - )); + return if include_local { + Ok(EnvironmentDefault::EnvironmentId( + LOCAL_ENVIRONMENT_ID.to_string(), + )) + } else { + Ok(EnvironmentDefault::Disabled) + }; }; if default.is_empty() { return Err(ExecServerError::Protocol( @@ -330,6 +347,7 @@ mod tests { async fn toml_provider_includes_local_and_adds_configured_environments() { let provider = TomlEnvironmentProvider::new(EnvironmentsToml { default: Some("ssh-dev".to_string()), + include_local: None, environments: vec![ EnvironmentToml { id: "devbox".to_string(), @@ -396,6 +414,7 @@ mod tests { async fn toml_provider_default_none_disables_default() { let provider = TomlEnvironmentProvider::new(EnvironmentsToml { default: Some("none".to_string()), + include_local: None, environments: Vec::new(), }) .expect("provider"); @@ -405,6 +424,55 @@ mod tests { assert_eq!(snapshot.default, EnvironmentDefault::Disabled); } + #[tokio::test] + async fn toml_provider_can_disable_local_environment() { + let provider = TomlEnvironmentProvider::new(EnvironmentsToml { + default: Some("ssh-dev".to_string()), + include_local: Some(false), + environments: vec![EnvironmentToml { + id: "ssh-dev".to_string(), + program: Some("ssh".to_string()), + ..Default::default() + }], + }) + .expect("provider"); + let snapshot = provider.snapshot().await.expect("environments"); + + assert!(!snapshot.include_local); + assert_eq!( + snapshot.default, + EnvironmentDefault::EnvironmentId("ssh-dev".to_string()) + ); + } + + #[tokio::test] + async fn toml_provider_without_local_and_default_omitted_disables_default() { + let provider = TomlEnvironmentProvider::new(EnvironmentsToml { + include_local: Some(false), + ..Default::default() + }) + .expect("provider"); + let snapshot = provider.snapshot().await.expect("environments"); + + assert!(!snapshot.include_local); + assert_eq!(snapshot.default, EnvironmentDefault::Disabled); + } + + #[test] + fn toml_provider_rejects_local_default_when_local_is_disabled() { + let err = TomlEnvironmentProvider::new(EnvironmentsToml { + default: Some(LOCAL_ENVIRONMENT_ID.to_string()), + include_local: Some(false), + environments: Vec::new(), + }) + .expect_err("local default without local environment should fail"); + + assert_eq!( + err.to_string(), + "exec-server protocol error: default environment `local` is not configured" + ); + } + #[test] fn toml_provider_rejects_invalid_environments() { let cases = [ @@ -479,6 +547,7 @@ mod tests { for (item, expected) in cases { let err = TomlEnvironmentProvider::new(EnvironmentsToml { default: None, + include_local: None, environments: vec![item], }) .expect_err("invalid item should fail"); @@ -496,6 +565,7 @@ mod tests { let provider = TomlEnvironmentProvider::new_with_config_dir( EnvironmentsToml { default: None, + include_local: None, environments: vec![EnvironmentToml { id: "ssh-dev".to_string(), program: Some("ssh".to_string()), @@ -525,6 +595,7 @@ mod tests { fn toml_provider_parses_configured_transport_timeouts() { let provider = TomlEnvironmentProvider::new(EnvironmentsToml { default: None, + include_local: None, environments: vec![ EnvironmentToml { id: "devbox".to_string(), @@ -569,6 +640,7 @@ mod tests { fn toml_provider_rejects_relative_stdio_cwd_without_config_dir() { let err = TomlEnvironmentProvider::new(EnvironmentsToml { default: None, + include_local: None, environments: vec![EnvironmentToml { id: "ssh-dev".to_string(), program: Some("ssh".to_string()), @@ -588,6 +660,7 @@ mod tests { fn toml_provider_rejects_duplicate_ids() { let err = TomlEnvironmentProvider::new(EnvironmentsToml { default: None, + include_local: None, environments: vec![ EnvironmentToml { id: "devbox".to_string(), @@ -614,6 +687,7 @@ mod tests { let id = "a".repeat(MAX_ENVIRONMENT_ID_LEN + 1); let err = TomlEnvironmentProvider::new(EnvironmentsToml { default: None, + include_local: None, environments: vec![EnvironmentToml { id: id.clone(), url: Some("ws://127.0.0.1:8765".to_string()), @@ -634,6 +708,7 @@ mod tests { fn toml_provider_rejects_unknown_default() { let err = TomlEnvironmentProvider::new(EnvironmentsToml { default: Some("missing".to_string()), + include_local: None, environments: Vec::new(), }) .expect_err("unknown default should fail"); @@ -652,6 +727,7 @@ mod tests { &path, r#" default = "ssh-dev" +include_local = false [[environments]] id = "devbox" @@ -673,6 +749,7 @@ CODEX_LOG = "debug" let environments = load_environments_toml(&path).expect("environments.toml"); assert_eq!(environments.default.as_deref(), Some("ssh-dev")); + assert_eq!(environments.include_local, Some(false)); assert_eq!(environments.environments.len(), 2); assert_eq!( environments.environments[0], @@ -736,6 +813,7 @@ unknown = true fn toml_provider_rejects_malformed_websocket_url() { let err = TomlEnvironmentProvider::new(EnvironmentsToml { default: None, + include_local: None, environments: vec![EnvironmentToml { id: "devbox".to_string(), url: Some("ws://".to_string()), @@ -758,6 +836,7 @@ unknown = true codex_home.path().join(ENVIRONMENTS_TOML_FILE), r#" default = "none" +include_local = false "#, ) .expect("write environments.toml"); @@ -772,7 +851,7 @@ default = "none" .map(|(id, _environment)| id) .collect(); - assert!(snapshot.include_local); + assert!(!snapshot.include_local); assert!(!environment_ids.contains(&LOCAL_ENVIRONMENT_ID.to_string())); assert_eq!(snapshot.default, EnvironmentDefault::Disabled); } diff --git a/codex-rs/exec-server/src/relay.rs b/codex-rs/exec-server/src/relay.rs index 7470a6290c..b3708f1d83 100644 --- a/codex-rs/exec-server/src/relay.rs +++ b/codex-rs/exec-server/src/relay.rs @@ -1,7 +1,9 @@ use std::collections::HashMap; use codex_app_server_protocol::JSONRPCMessage; +use futures::Sink; use futures::SinkExt; +use futures::Stream; use futures::StreamExt; use prost::Message as ProstMessage; use tokio::io::AsyncRead; @@ -19,7 +21,6 @@ use crate::connection::CHANNEL_CAPACITY; use crate::connection::JsonRpcConnection; use crate::connection::JsonRpcConnectionEvent; use crate::connection::JsonRpcTransport; -use crate::connection::WEBSOCKET_KEEPALIVE_INTERVAL; use crate::relay_proto::RelayData; use crate::relay_proto::RelayMessageFrame; use crate::relay_proto::RelayResume; @@ -140,121 +141,25 @@ fn jsonrpc_payload(message: &JSONRPCMessage) -> Result, ExecServerError> serde_json::to_vec(message).map_err(ExecServerError::Json) } -pub(crate) fn harness_connection_from_websocket( - stream: WebSocketStream, +pub(crate) fn harness_connection_from_websocket( + stream: T, connection_label: String, ) -> JsonRpcConnection where - S: AsyncRead + AsyncWrite + Unpin + Send + 'static, + T: Sink + Stream> + Unpin + Send + 'static, + E: std::fmt::Display + Send + 'static, { let stream_id = Uuid::new_v4().to_string(); - let (mut websocket_writer, mut websocket_reader) = stream.split(); let (outgoing_tx, mut outgoing_rx) = mpsc::channel(CHANNEL_CAPACITY); let (incoming_tx, incoming_rx) = mpsc::channel(CHANNEL_CAPACITY); let (disconnected_tx, disconnected_rx) = watch::channel(false); - let reader_label = connection_label; - let reader_stream_id = stream_id.clone(); - let incoming_tx_for_reader = incoming_tx; - let disconnected_tx_for_reader = disconnected_tx.clone(); - let reader_task = tokio::spawn(async move { - loop { - match websocket_reader.next().await { - Some(Ok(Message::Binary(payload))) => { - let frame = match decode_relay_message_frame(payload.as_ref()) { - Ok(frame) => frame, - Err(err) => { - let _ = incoming_tx_for_reader - .send(JsonRpcConnectionEvent::MalformedMessage { - reason: format!( - "failed to parse relay message frame from {reader_label}: {err}" - ), - }) - .await; - continue; - } - }; - if frame.stream_id != reader_stream_id { - continue; - } - let kind = match frame.validate() { - Ok(kind) => kind, - Err(err) => { - let _ = incoming_tx_for_reader - .send(JsonRpcConnectionEvent::MalformedMessage { - reason: err.to_string(), - }) - .await; - continue; - } - }; - match kind { - RelayFrameBodyKind::Data => match frame.into_jsonrpc_message() { - Ok(message) => { - if incoming_tx_for_reader - .send(JsonRpcConnectionEvent::Message(message)) - .await - .is_err() - { - break; - } - } - Err(err) => { - let _ = incoming_tx_for_reader - .send(JsonRpcConnectionEvent::MalformedMessage { - reason: err.to_string(), - }) - .await; - } - }, - RelayFrameBodyKind::Reset => { - let _ = disconnected_tx_for_reader.send(true); - let _ = incoming_tx_for_reader - .send(JsonRpcConnectionEvent::Disconnected { - reason: frame.into_reset_reason(), - }) - .await; - break; - } - RelayFrameBodyKind::Ack - | RelayFrameBodyKind::Resume - | RelayFrameBodyKind::Heartbeat => {} - } - } - Some(Ok(Message::Close(_))) | None => { - let _ = disconnected_tx_for_reader.send(true); - let _ = incoming_tx_for_reader - .send(JsonRpcConnectionEvent::Disconnected { reason: None }) - .await; - break; - } - Some(Ok(Message::Ping(_) | Message::Pong(_) | Message::Frame(_))) => {} - Some(Ok(Message::Text(_))) => { - let _ = incoming_tx_for_reader - .send(JsonRpcConnectionEvent::MalformedMessage { - reason: "relay exec-server transport expects binary protobuf frames" - .to_string(), - }) - .await; - } - Some(Err(err)) => { - let _ = disconnected_tx_for_reader.send(true); - let _ = incoming_tx_for_reader - .send(JsonRpcConnectionEvent::Disconnected { - reason: Some(format!( - "failed to read relay websocket frame from {reader_label}: {err}" - )), - }) - .await; - break; - } - } - } - }); - - let writer_task = tokio::spawn(async move { + let websocket_task = tokio::spawn(async move { + let mut websocket = stream; + let reader_label = connection_label; + let reader_stream_id = stream_id.clone(); let resume = RelayMessageFrame::resume(stream_id.clone()); - if websocket_writer + if websocket .send(Message::Binary(encode_relay_message_frame(&resume).into())) .await .is_err() @@ -263,11 +168,6 @@ where return; } - let mut keepalive = tokio::time::interval_at( - tokio::time::Instant::now() + WEBSOCKET_KEEPALIVE_INTERVAL, - WEBSOCKET_KEEPALIVE_INTERVAL, - ); - keepalive.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); let mut next_seq = 0u32; loop { tokio::select! { @@ -284,7 +184,7 @@ where }; let frame = RelayMessageFrame::data(stream_id.clone(), next_seq, payload); next_seq = next_seq.wrapping_add(1); - if websocket_writer + if websocket .send(Message::Binary(encode_relay_message_frame(&frame).into())) .await .is_err() @@ -293,10 +193,96 @@ where break; } } - _ = keepalive.tick() => { - if websocket_writer.send(Message::Ping(Vec::new().into())).await.is_err() { - let _ = disconnected_tx.send(true); - break; + incoming_message = websocket.next() => { + match incoming_message { + Some(Ok(Message::Binary(payload))) => { + let frame = match decode_relay_message_frame(payload.as_ref()) { + Ok(frame) => frame, + Err(err) => { + let _ = incoming_tx + .send(JsonRpcConnectionEvent::MalformedMessage { + reason: format!( + "failed to parse relay message frame from {reader_label}: {err}" + ), + }) + .await; + continue; + } + }; + if frame.stream_id != reader_stream_id { + continue; + } + let kind = match frame.validate() { + Ok(kind) => kind, + Err(err) => { + let _ = incoming_tx + .send(JsonRpcConnectionEvent::MalformedMessage { + reason: err.to_string(), + }) + .await; + continue; + } + }; + match kind { + RelayFrameBodyKind::Data => match frame.into_jsonrpc_message() { + Ok(message) => { + if incoming_tx + .send(JsonRpcConnectionEvent::Message(message)) + .await + .is_err() + { + break; + } + } + Err(err) => { + let _ = incoming_tx + .send(JsonRpcConnectionEvent::MalformedMessage { + reason: err.to_string(), + }) + .await; + } + }, + RelayFrameBodyKind::Reset => { + let _ = disconnected_tx.send(true); + let _ = incoming_tx + .send(JsonRpcConnectionEvent::Disconnected { + reason: frame.into_reset_reason(), + }) + .await; + break; + } + RelayFrameBodyKind::Ack + | RelayFrameBodyKind::Resume + | RelayFrameBodyKind::Heartbeat => {} + } + } + Some(Ok(Message::Close(_))) | None => { + let _ = disconnected_tx.send(true); + let _ = incoming_tx + .send(JsonRpcConnectionEvent::Disconnected { reason: None }) + .await; + break; + } + Some(Ok(Message::Ping(_) | Message::Pong(_) | Message::Frame(_))) => {} + Some(Ok(Message::Text(_))) => { + let _ = incoming_tx + .send(JsonRpcConnectionEvent::MalformedMessage { + reason: "relay exec-server transport expects binary protobuf frames" + .to_string(), + }) + .await; + } + Some(Err(err)) => { + let _ = disconnected_tx.send(true); + let _ = incoming_tx + .send(JsonRpcConnectionEvent::Disconnected { + reason: Some(format!( + "failed to read relay websocket frame from {reader_label}: {err}" + )), + }) + .await; + break; + } } } } @@ -307,7 +293,7 @@ where outgoing_tx, incoming_rx, disconnected_rx, - task_handles: vec![reader_task, writer_task], + task_handles: vec![websocket_task], transport: JsonRpcTransport::Plain, } } @@ -318,59 +304,42 @@ pub(crate) async fn run_multiplexed_executor( ) where S: AsyncRead + AsyncWrite + Unpin + Send + 'static, { - let (mut websocket_writer, mut websocket_reader) = stream.split(); + let mut websocket = stream; let (physical_outgoing_tx, mut physical_outgoing_rx) = mpsc::channel::>(CHANNEL_CAPACITY); - let writer_task = tokio::spawn(async move { - let mut keepalive = tokio::time::interval_at( - tokio::time::Instant::now() + WEBSOCKET_KEEPALIVE_INTERVAL, - WEBSOCKET_KEEPALIVE_INTERVAL, - ); - keepalive.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); - loop { - tokio::select! { - maybe_encoded = physical_outgoing_rx.recv() => { - let Some(encoded) = maybe_encoded else { - break; - }; - if websocket_writer - .send(Message::Binary(encoded.into())) - .await - .is_err() - { - break; - } - } - _ = keepalive.tick() => { - if websocket_writer.send(Message::Ping(Vec::new().into())).await.is_err() { - break; - } - } - } - } - }); let mut streams: HashMap = HashMap::new(); loop { - let frame = match websocket_reader.next().await { - Some(Ok(Message::Binary(payload))) => { - match decode_relay_message_frame(payload.as_ref()) { - Ok(frame) => frame, - Err(err) => { - warn!("dropping malformed relay message frame from harness: {err}"); - continue; - } + let frame = tokio::select! { + maybe_encoded = physical_outgoing_rx.recv() => { + let Some(encoded) = maybe_encoded else { + break; + }; + if websocket.send(Message::Binary(encoded.into())).await.is_err() { + break; } - } - Some(Ok(Message::Close(_))) | None => break, - Some(Ok(Message::Ping(_) | Message::Pong(_) | Message::Frame(_))) => continue, - Some(Ok(Message::Text(_))) => { - warn!("dropping non-binary relay message frame from harness"); continue; } - Some(Err(err)) => { - debug!("multiplexed executor websocket read failed: {err}"); - break; + incoming_message = websocket.next() => match incoming_message { + Some(Ok(Message::Binary(payload))) => { + match decode_relay_message_frame(payload.as_ref()) { + Ok(frame) => frame, + Err(err) => { + warn!("dropping malformed relay message frame from harness: {err}"); + continue; + } + } + } + Some(Ok(Message::Close(_))) | None => break, + Some(Ok(Message::Ping(_) | Message::Pong(_) | Message::Frame(_))) => continue, + Some(Ok(Message::Text(_))) => { + warn!("dropping non-binary relay message frame from harness"); + continue; + } + Some(Err(err)) => { + debug!("multiplexed executor websocket read failed: {err}"); + break; + } } }; @@ -423,7 +392,6 @@ pub(crate) async fn run_multiplexed_executor( stream.disconnect(/*reason*/ None).await; } drop(physical_outgoing_tx); - let _ = writer_task.await; } struct VirtualStream { @@ -492,8 +460,20 @@ fn spawn_virtual_stream( #[cfg(test)] mod tests { + use std::pin::Pin; + use std::sync::Arc; + use std::sync::atomic::AtomicBool; + use std::sync::atomic::Ordering; + use std::task::Context; + use std::task::Poll; use std::time::Duration; + use codex_app_server_protocol::JSONRPCRequest; + use codex_app_server_protocol::RequestId; + use futures::Sink; + use futures::Stream; + use futures::channel::mpsc as futures_mpsc; + use futures::task::AtomicWaker; use tokio::net::TcpListener; use tokio::time::timeout; use tokio_tungstenite::accept_async; @@ -502,40 +482,107 @@ mod tests { use super::*; - fn test_runtime_paths() -> anyhow::Result { - crate::ExecServerRuntimePaths::new( - std::env::current_exe()?, - /*codex_linux_sandbox_exe*/ None, - ) - .map_err(anyhow::Error::from) - } - #[tokio::test] - async fn multiplexed_executor_sends_keepalive_ping() -> anyhow::Result<()> { + async fn harness_connection_receives_relay_data() -> anyhow::Result<()> { let (client_websocket, mut server_websocket) = websocket_pair().await?; - let executor_task = tokio::spawn(run_multiplexed_executor( - client_websocket, - ConnectionProcessor::new(test_runtime_paths()?), + let mut connection = + harness_connection_from_websocket(client_websocket, "test".to_string()); + let stream_id = read_resume_stream_id(&mut server_websocket).await?; + let message = test_jsonrpc_message(); + + server_websocket + .send(Message::Binary( + encode_relay_message_frame(&RelayMessageFrame::data( + stream_id, + /*seq*/ 0, + jsonrpc_payload(&message)?, + )) + .into(), + )) + .await?; + assert!(matches!( + timeout(Duration::from_secs(1), connection.incoming_rx.recv()).await?, + Some(JsonRpcConnectionEvent::Message(actual)) if actual == message )); - read_keepalive_ping(&mut server_websocket).await?; - - executor_task.abort(); - let _ = executor_task.await; + drop(connection); Ok(()) } #[tokio::test] - async fn harness_connection_sends_keepalive_ping() -> anyhow::Result<()> { + async fn harness_connection_reports_text_frames_as_malformed() -> anyhow::Result<()> { let (client_websocket, mut server_websocket) = websocket_pair().await?; - let connection = harness_connection_from_websocket(client_websocket, "test".to_string()); + let mut connection = + harness_connection_from_websocket(client_websocket, "test".to_string()); - read_keepalive_ping(&mut server_websocket).await?; + read_resume_stream_id(&mut server_websocket).await?; + server_websocket.send(Message::Text("nope".into())).await?; + assert!(matches!( + timeout(Duration::from_secs(1), connection.incoming_rx.recv()).await?, + Some(JsonRpcConnectionEvent::MalformedMessage { reason }) + if reason == "relay exec-server transport expects binary protobuf frames" + )); drop(connection); Ok(()) } + #[tokio::test] + async fn harness_connection_reports_server_close() -> anyhow::Result<()> { + let (client_websocket, mut server_websocket) = websocket_pair().await?; + let mut connection = + harness_connection_from_websocket(client_websocket, "test".to_string()); + + read_resume_stream_id(&mut server_websocket).await?; + server_websocket.close(None).await?; + assert!(matches!( + timeout(Duration::from_secs(1), connection.incoming_rx.recv()).await?, + Some(JsonRpcConnectionEvent::Disconnected { reason: None }) + )); + + drop(connection); + Ok(()) + } + + #[tokio::test] + async fn harness_connection_keeps_outbound_frame_while_send_is_backpressured() + -> anyhow::Result<()> { + let (websocket, control, mut outbound_rx) = + ControlledWebSocket::new(/*write_ready*/ true); + let mut connection = harness_connection_from_websocket(websocket, "test".to_string()); + let Message::Binary(resume_payload) = timeout(Duration::from_secs(1), outbound_rx.next()) + .await? + .expect("resume frame") + else { + anyhow::bail!("expected relay resume frame"); + }; + let stream_id = decode_relay_message_frame(resume_payload.as_ref())?.stream_id; + let message = test_jsonrpc_message(); + + control.set_write_blocked(); + connection.outgoing_tx.send(message.clone()).await?; + control.wait_for_blocked_write().await?; + control.send_inbound(Message::Pong(b"check".to_vec().into()))?; + assert!( + timeout(Duration::from_millis(50), connection.incoming_rx.recv()) + .await + .is_err() + ); + + control.set_write_ready(); + let Message::Binary(data_payload) = timeout(Duration::from_secs(1), outbound_rx.next()) + .await? + .expect("data frame") + else { + anyhow::bail!("expected relay data frame"); + }; + let frame = decode_relay_message_frame(data_payload.as_ref())?; + assert_eq!(frame.stream_id, stream_id); + assert_eq!(frame.into_jsonrpc_message()?, message); + drop(connection); + Ok(()) + } + async fn websocket_pair() -> anyhow::Result<( WebSocketStream>, WebSocketStream, @@ -551,18 +598,155 @@ mod tests { Ok((client_websocket, server_websocket)) } - async fn read_keepalive_ping( + async fn read_resume_stream_id( websocket: &mut WebSocketStream, - ) -> anyhow::Result<()> { - loop { - let Some(message) = timeout(Duration::from_secs(1), websocket.next()).await? else { - anyhow::bail!("websocket closed before keepalive ping"); - }; - match message? { - Message::Ping(_) => return Ok(()), - Message::Binary(_) | Message::Text(_) | Message::Pong(_) | Message::Frame(_) => {} - Message::Close(_) => anyhow::bail!("websocket closed before keepalive ping"), + ) -> anyhow::Result { + let message = timeout(Duration::from_secs(1), websocket.next()) + .await? + .expect("websocket should stay open")?; + let Message::Binary(payload) = message else { + anyhow::bail!("expected relay resume frame, got {message:?}"); + }; + let frame = decode_relay_message_frame(payload.as_ref())?; + assert_eq!(frame.validate()?, RelayFrameBodyKind::Resume); + Ok(frame.stream_id) + } + + fn test_jsonrpc_message() -> JSONRPCMessage { + JSONRPCMessage::Request(JSONRPCRequest { + id: RequestId::Integer(1), + method: "test".to_string(), + params: None, + trace: None, + }) + } + + struct ControlledWebSocket { + inbound_rx: futures_mpsc::UnboundedReceiver>, + outbound_tx: futures_mpsc::UnboundedSender, + write_ready: Arc, + write_blocked: Arc, + write_blocked_waker: Arc, + write_waker: Arc, + } + + struct ControlledWebSocketHandle { + inbound_tx: futures_mpsc::UnboundedSender>, + write_ready: Arc, + write_blocked: Arc, + write_blocked_waker: Arc, + write_waker: Arc, + } + + impl ControlledWebSocket { + fn new( + write_ready: bool, + ) -> ( + Self, + ControlledWebSocketHandle, + futures_mpsc::UnboundedReceiver, + ) { + let (inbound_tx, inbound_rx) = futures_mpsc::unbounded(); + let (outbound_tx, outbound_rx) = futures_mpsc::unbounded(); + let write_ready = Arc::new(AtomicBool::new(write_ready)); + let write_blocked = Arc::new(AtomicBool::new(false)); + let write_blocked_waker = Arc::new(AtomicWaker::new()); + let write_waker = Arc::new(AtomicWaker::new()); + ( + Self { + inbound_rx, + outbound_tx, + write_ready: Arc::clone(&write_ready), + write_blocked: Arc::clone(&write_blocked), + write_blocked_waker: Arc::clone(&write_blocked_waker), + write_waker: Arc::clone(&write_waker), + }, + ControlledWebSocketHandle { + inbound_tx, + write_ready, + write_blocked, + write_blocked_waker, + write_waker, + }, + outbound_rx, + ) + } + } + + impl ControlledWebSocketHandle { + fn send_inbound(&self, message: Message) -> anyhow::Result<()> { + self.inbound_tx + .unbounded_send(Ok(message)) + .map_err(anyhow::Error::from) + } + + fn set_write_blocked(&self) { + self.write_ready.store(false, Ordering::Release); + } + + fn set_write_ready(&self) { + self.write_ready.store(true, Ordering::Release); + self.write_waker.wake(); + } + + async fn wait_for_blocked_write(&self) -> anyhow::Result<()> { + timeout( + Duration::from_secs(1), + futures::future::poll_fn(|cx| { + if self.write_blocked.load(Ordering::Acquire) { + Poll::Ready(()) + } else { + self.write_blocked_waker.register(cx.waker()); + Poll::Pending + } + }), + ) + .await?; + Ok(()) + } + } + + impl Sink for ControlledWebSocket { + type Error = std::convert::Infallible; + + fn poll_ready(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + if self.write_ready.load(Ordering::Acquire) { + Poll::Ready(Ok(())) + } else { + self.write_blocked.store(true, Ordering::Release); + self.write_blocked_waker.wake(); + self.write_waker.register(cx.waker()); + Poll::Pending } } + + fn start_send(self: Pin<&mut Self>, item: Message) -> Result<(), Self::Error> { + self.outbound_tx + .unbounded_send(item) + .expect("test outbound receiver should stay open"); + Ok(()) + } + + fn poll_flush( + self: Pin<&mut Self>, + _cx: &mut Context<'_>, + ) -> Poll> { + Poll::Ready(Ok(())) + } + + fn poll_close( + self: Pin<&mut Self>, + _cx: &mut Context<'_>, + ) -> Poll> { + Poll::Ready(Ok(())) + } + } + + impl Stream for ControlledWebSocket { + type Item = Result; + + fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + Pin::new(&mut self.inbound_rx).poll_next(cx) + } } } diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 9e54a72abd..3db7a51576 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -522,9 +522,10 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result )?; let state_db = codex_core::init_state_db(&config).await; let environment_manager = if run_loader_overrides.ignore_user_config { - EnvironmentManager::from_env(local_runtime_paths).await? + EnvironmentManager::from_env(Some(local_runtime_paths)).await? } else { - EnvironmentManager::from_codex_home(config.codex_home.clone(), local_runtime_paths).await? + EnvironmentManager::from_codex_home(config.codex_home.clone(), Some(local_runtime_paths)) + .await? }; let in_process_start_args = InProcessClientStartArgs { arg0_paths, diff --git a/codex-rs/mcp-server/src/lib.rs b/codex-rs/mcp-server/src/lib.rs index d82de48f9e..4a4ff054be 100644 --- a/codex-rs/mcp-server/src/lib.rs +++ b/codex-rs/mcp-server/src/lib.rs @@ -96,10 +96,10 @@ pub async fn run_main( let environment_manager = Arc::new( EnvironmentManager::from_codex_home( config.codex_home.clone(), - ExecServerRuntimePaths::from_optional_paths( + Some(ExecServerRuntimePaths::from_optional_paths( arg0_paths.codex_self_exe.clone(), arg0_paths.codex_linux_sandbox_exe.clone(), - )?, + )?), ) .await .map_err(std::io::Error::other)?, diff --git a/codex-rs/otel/src/events/session_telemetry.rs b/codex-rs/otel/src/events/session_telemetry.rs index 6394c73ea2..6a77125e70 100644 --- a/codex-rs/otel/src/events/session_telemetry.rs +++ b/codex-rs/otel/src/events/session_telemetry.rs @@ -8,6 +8,8 @@ use crate::metrics::API_CALL_DURATION_METRIC; use crate::metrics::MetricsClient; use crate::metrics::MetricsConfig; use crate::metrics::MetricsError; +use crate::metrics::PLUGIN_INSTALL_ELICITATION_SENT_METRIC; +use crate::metrics::PLUGIN_INSTALL_SUGGESTION_METRIC; use crate::metrics::PROFILE_USAGE_METRIC; use crate::metrics::RESPONSES_API_ENGINE_IAPI_TBT_DURATION_METRIC; use crate::metrics::RESPONSES_API_ENGINE_IAPI_TTFT_DURATION_METRIC; @@ -230,6 +232,67 @@ impl SessionTelemetry { ); } + /// Records the moment a plugin or connector install elicitation is dispatched. + pub fn record_plugin_install_elicitation_sent( + &self, + tool_type: &str, + tool_id: &str, + tool_name: &str, + ) { + self.counter( + PLUGIN_INSTALL_ELICITATION_SENT_METRIC, + /*inc*/ 1, + &[("tool_type", tool_type)], + ); + log_and_trace_event!( + self, + common: { + event.name = "codex.plugin_install_elicitation_sent", + plugin_install.tool_type = tool_type, + plugin_install.tool_id = tool_id, + plugin_install.tool_name = tool_name, + }, + log: {}, + trace: {}, + ); + } + + /// Records the outcome of a surfaced plugin or connector install suggestion. + pub fn record_plugin_install_suggestion( + &self, + tool_type: &str, + tool_id: &str, + tool_name: &str, + response_action: &str, + user_confirmed: bool, + completed: bool, + ) { + let completed_tag = if completed { "true" } else { "false" }; + self.counter( + PLUGIN_INSTALL_SUGGESTION_METRIC, + /*inc*/ 1, + &[ + ("tool_type", tool_type), + ("response_action", response_action), + ("completed", completed_tag), + ], + ); + log_and_trace_event!( + self, + common: { + event.name = "codex.plugin_install_suggestion", + plugin_install.tool_type = tool_type, + plugin_install.tool_id = tool_id, + plugin_install.tool_name = tool_name, + plugin_install.response_action = response_action, + plugin_install.user_confirmed = user_confirmed, + plugin_install.completed = completed, + }, + log: {}, + trace: {}, + ); + } + pub fn start_timer(&self, name: &str, tags: &[(&str, &str)]) -> Result { let Some(metrics) = &self.metrics else { return Err(MetricsError::ExporterDisabled); diff --git a/codex-rs/otel/src/metrics/names.rs b/codex-rs/otel/src/metrics/names.rs index 17a39816a7..429db8116e 100644 --- a/codex-rs/otel/src/metrics/names.rs +++ b/codex-rs/otel/src/metrics/names.rs @@ -37,6 +37,8 @@ pub const GOAL_BLOCKED_METRIC: &str = "codex.goal.blocked"; pub const GOAL_TOKEN_COUNT_METRIC: &str = "codex.goal.token_count"; pub const GOAL_DURATION_SECONDS_METRIC: &str = "codex.goal.duration_s"; pub const PROFILE_USAGE_METRIC: &str = "codex.profile.usage"; +pub const PLUGIN_INSTALL_ELICITATION_SENT_METRIC: &str = "codex.plugins.install_elicitation.sent"; +pub const PLUGIN_INSTALL_SUGGESTION_METRIC: &str = "codex.plugins.install_suggestion"; pub const CURATED_PLUGINS_STARTUP_SYNC_METRIC: &str = "codex.plugins.startup_sync"; pub const CURATED_PLUGINS_STARTUP_SYNC_FINAL_METRIC: &str = "codex.plugins.startup_sync.final"; pub const HOOK_RUN_METRIC: &str = "codex.hooks.run"; diff --git a/codex-rs/otel/tests/suite/manager_metrics.rs b/codex-rs/otel/tests/suite/manager_metrics.rs index d95f42dfc5..e13de3f432 100644 --- a/codex-rs/otel/tests/suite/manager_metrics.rs +++ b/codex-rs/otel/tests/suite/manager_metrics.rs @@ -2,6 +2,8 @@ use crate::harness::attributes_to_map; use crate::harness::build_metrics_with_defaults; use crate::harness::find_metric; use crate::harness::latest_metrics; +use codex_otel::PLUGIN_INSTALL_ELICITATION_SENT_METRIC; +use codex_otel::PLUGIN_INSTALL_SUGGESTION_METRIC; use codex_otel::Result; use codex_otel::SessionTelemetry; use codex_otel::TelemetryAuthMode; @@ -161,3 +163,100 @@ fn manager_attaches_optional_service_name_tag() -> Result<()> { Ok(()) } + +#[test] +fn manager_records_plugin_install_suggestion_metric() -> Result<()> { + let (metrics, exporter) = build_metrics_with_defaults(&[])?; + let manager = SessionTelemetry::new( + ThreadId::new(), + "gpt-5.1", + "gpt-5.1", + Some("account-id".to_string()), + /*account_email*/ None, + Some(TelemetryAuthMode::ApiKey), + "test_originator".to_string(), + /*log_user_prompts*/ false, + "tty".to_string(), + SessionSource::Cli, + ) + .with_metrics_without_metadata_tags(metrics); + + manager.record_plugin_install_suggestion( + "connector", + "connector_calendar", + "Google Calendar", + "accept", + /*user_confirmed*/ true, + /*completed*/ false, + ); + manager.shutdown_metrics()?; + + let resource_metrics = latest_metrics(&exporter); + let metric = find_metric(&resource_metrics, PLUGIN_INSTALL_SUGGESTION_METRIC) + .expect("plugin install suggestion metric missing"); + let attrs = match metric.data() { + AggregatedMetrics::U64(data) => match data { + MetricData::Sum(sum) => { + let points: Vec<_> = sum.data_points().collect(); + assert_eq!(points.len(), 1); + attributes_to_map(points[0].attributes()) + } + _ => panic!("unexpected counter aggregation"), + }, + _ => panic!("unexpected counter data type"), + }; + + assert_eq!( + attrs, + BTreeMap::from([ + ("completed".to_string(), "false".to_string()), + ("response_action".to_string(), "accept".to_string()), + ("tool_type".to_string(), "connector".to_string()), + ]) + ); + + Ok(()) +} + +#[test] +fn manager_records_plugin_install_elicitation_sent_metric() -> Result<()> { + let (metrics, exporter) = build_metrics_with_defaults(&[])?; + let manager = SessionTelemetry::new( + ThreadId::new(), + "gpt-5.1", + "gpt-5.1", + Some("account-id".to_string()), + /*account_email*/ None, + Some(TelemetryAuthMode::ApiKey), + "test_originator".to_string(), + /*log_user_prompts*/ false, + "tty".to_string(), + SessionSource::Cli, + ) + .with_metrics_without_metadata_tags(metrics); + + manager.record_plugin_install_elicitation_sent("plugin", "slack@openai-curated", "Slack"); + manager.shutdown_metrics()?; + + let resource_metrics = latest_metrics(&exporter); + let metric = find_metric(&resource_metrics, PLUGIN_INSTALL_ELICITATION_SENT_METRIC) + .expect("plugin install elicitation sent metric missing"); + let attrs = match metric.data() { + AggregatedMetrics::U64(data) => match data { + MetricData::Sum(sum) => { + let points: Vec<_> = sum.data_points().collect(); + assert_eq!(points.len(), 1); + attributes_to_map(points[0].attributes()) + } + _ => panic!("unexpected counter aggregation"), + }, + _ => panic!("unexpected counter data type"), + }; + + assert_eq!( + attrs, + BTreeMap::from([("tool_type".to_string(), "plugin".to_string())]) + ); + + Ok(()) +} diff --git a/codex-rs/thread-manager-sample/src/main.rs b/codex-rs/thread-manager-sample/src/main.rs index cf499c7703..9fdd6db90f 100644 --- a/codex-rs/thread-manager-sample/src/main.rs +++ b/codex-rs/thread-manager-sample/src/main.rs @@ -116,7 +116,8 @@ async fn run_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> { )?; let thread_store = thread_store_from_config(&config, state_db.clone()); let environment_manager = Arc::new( - EnvironmentManager::from_codex_home(config.codex_home.clone(), local_runtime_paths).await?, + EnvironmentManager::from_codex_home(config.codex_home.clone(), Some(local_runtime_paths)) + .await?, ); let installation_id = resolve_installation_id(&config.codex_home).await?; let thread_manager = ThreadManager::new( diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index 4d972af2cf..7b64776dca 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -71,6 +71,8 @@ pub use tool_discovery::DiscoverablePluginInfo; pub use tool_discovery::DiscoverableTool; pub use tool_discovery::DiscoverableToolAction; pub use tool_discovery::DiscoverableToolType; +pub use tool_discovery::LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME; +pub use tool_discovery::ListAvailablePluginsToInstallResult; pub use tool_discovery::REQUEST_PLUGIN_INSTALL_TOOL_NAME; pub use tool_discovery::RequestPluginInstallEntry; pub use tool_discovery::TOOL_SEARCH_DEFAULT_LIMIT; diff --git a/codex-rs/tools/src/tool_discovery.rs b/codex-rs/tools/src/tool_discovery.rs index a5beb768a2..ac5aac904b 100644 --- a/codex-rs/tools/src/tool_discovery.rs +++ b/codex-rs/tools/src/tool_discovery.rs @@ -5,6 +5,7 @@ use serde::Serialize; const TUI_CLIENT_NAME: &str = "codex-tui"; pub const TOOL_SEARCH_TOOL_NAME: &str = "tool_search"; pub const TOOL_SEARCH_DEFAULT_LIMIT: usize = 8; +pub const LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME: &str = "list_available_plugins_to_install"; pub const REQUEST_PLUGIN_INSTALL_TOOL_NAME: &str = "request_plugin_install"; #[derive(Clone, Debug, PartialEq, Eq)] @@ -99,7 +100,7 @@ pub struct DiscoverablePluginInfo { pub app_connector_ids: Vec, } -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, Serialize, PartialEq, Eq)] pub struct RequestPluginInstallEntry { pub id: String, pub name: String, @@ -110,6 +111,11 @@ pub struct RequestPluginInstallEntry { pub app_connector_ids: Vec, } +#[derive(Clone, Debug, Serialize, PartialEq, Eq)] +pub struct ListAvailablePluginsToInstallResult { + pub tools: Vec, +} + pub fn collect_request_plugin_install_entries( discoverable_tools: &[DiscoverableTool], ) -> Vec { diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 5cfb520dbe..3164f16acb 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -3,6 +3,7 @@ //! This module owns the `App` struct, shared imports, and the high-level run loop that coordinates //! the focused app submodules. +use crate::AppServerTarget; use crate::app_backtrack::BacktrackState; use crate::app_command::AppCommand; use crate::app_event::AppEvent; @@ -82,7 +83,6 @@ use crate::workspace_command::AppServerWorkspaceCommandRunner; use crate::workspace_command::WorkspaceCommandRunner; use codex_ansi_escape::ansi_escape_line; use codex_app_server_client::AppServerRequestHandle; -use codex_app_server_client::RemoteAppServerEndpoint; use codex_app_server_client::TypedRequestError; use codex_app_server_protocol::AddCreditsNudgeCreditType; use codex_app_server_protocol::AskForApproval; @@ -516,7 +516,7 @@ pub(crate) struct App { pub(crate) feedback: codex_feedback::CodexFeedback, feedback_audience: FeedbackAudience, environment_manager: Arc, - remote_app_server_endpoint: Option, + app_server_target: AppServerTarget, /// Set when the user confirms an update; propagated on exit. pub(crate) pending_update_action: Option, @@ -654,7 +654,7 @@ impl App { is_first_run: bool, entered_trust_nux: bool, should_prompt_windows_sandbox_nux_at_startup: bool, - remote_app_server_endpoint: Option, + app_server_target: AppServerTarget, state_db: Option, environment_manager: Arc, startup_hooks_browser: Option, @@ -941,7 +941,7 @@ See the Codex keymap documentation for supported actions and examples." feedback: feedback.clone(), feedback_audience, environment_manager, - remote_app_server_endpoint, + app_server_target, pending_update_action: None, pending_shutdown_exit_thread_id: None, windows_sandbox: WindowsSandboxState::default(), diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 987c9490d7..68bd759e2f 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -57,10 +57,7 @@ impl App { AppEvent::OpenResumePicker => { let picker_app_server = match crate::start_app_server_for_picker( &self.config, - &match self.remote_app_server_endpoint.clone() { - Some(endpoint) => crate::AppServerTarget::Remote { endpoint }, - None => crate::AppServerTarget::Embedded, - }, + &self.app_server_target, self.state_db.clone(), self.environment_manager.clone(), ) @@ -1680,7 +1677,7 @@ impl App { { Ok(()) => { self.chat_widget.update_skill_enabled(path, enabled); - if !app_server.is_remote() + if !app_server.uses_remote_workspace() && let Err(err) = self.refresh_in_memory_config_from_disk().await { tracing::warn!( @@ -1724,7 +1721,7 @@ impl App { { Ok(_) => { self.chat_widget.update_connector_enabled(&id, enabled); - if !app_server.is_remote() + if !app_server.uses_remote_workspace() && let Err(err) = self.refresh_in_memory_config_from_disk().await { tracing::warn!(error = %err, "failed to refresh config after app toggle"); diff --git a/codex-rs/tui/src/app/session_lifecycle.rs b/codex-rs/tui/src/app/session_lifecycle.rs index 1a57b596b0..6ea53d8b3c 100644 --- a/codex-rs/tui/src/app/session_lifecycle.rs +++ b/codex-rs/tui/src/app/session_lifecycle.rs @@ -633,7 +633,7 @@ impl App { } let current_cwd = self.config.cwd.to_path_buf(); - let resume_cwd = if self.remote_app_server_endpoint.is_some() { + let resume_cwd = if self.app_server_target.uses_remote_workspace() { current_cwd.clone() } else { match crate::session_resume::resolve_cwd_for_resume_or_fork( diff --git a/codex-rs/tui/src/app/test_support.rs b/codex-rs/tui/src/app/test_support.rs index a22107c226..6b6c424210 100644 --- a/codex-rs/tui/src/app/test_support.rs +++ b/codex-rs/tui/src/app/test_support.rs @@ -45,7 +45,7 @@ pub(super) async fn make_test_app() -> App { feedback: codex_feedback::CodexFeedback::new(), feedback_audience: FeedbackAudience::External, environment_manager: Arc::new(EnvironmentManager::default_for_tests()), - remote_app_server_endpoint: None, + app_server_target: crate::AppServerTarget::Embedded, pending_update_action: None, pending_shutdown_exit_thread_id: None, windows_sandbox: WindowsSandboxState::default(), diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 547f70890c..bddf5e544c 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -3867,7 +3867,7 @@ async fn make_test_app() -> App { feedback: codex_feedback::CodexFeedback::new(), feedback_audience: FeedbackAudience::External, environment_manager: Arc::new(EnvironmentManager::default_for_tests()), - remote_app_server_endpoint: None, + app_server_target: crate::AppServerTarget::Embedded, pending_update_action: None, pending_shutdown_exit_thread_id: None, windows_sandbox: WindowsSandboxState::default(), @@ -3930,7 +3930,7 @@ async fn make_test_app_with_channels() -> ( feedback: codex_feedback::CodexFeedback::new(), feedback_audience: FeedbackAudience::External, environment_manager: Arc::new(EnvironmentManager::default_for_tests()), - remote_app_server_endpoint: None, + app_server_target: crate::AppServerTarget::Embedded, pending_update_action: None, pending_shutdown_exit_thread_id: None, windows_sandbox: WindowsSandboxState::default(), diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index 5ea7bccf97..1bcb4a5597 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -149,10 +149,11 @@ pub(crate) struct AppServerSession { client: AppServerClient, next_request_id: i64, remote_cwd_override: Option, + thread_params_mode: ThreadParamsMode, } -#[derive(Clone, Copy)] -enum ThreadParamsMode { +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum ThreadParamsMode { Embedded, Remote, } @@ -182,11 +183,12 @@ pub(crate) enum TurnPermissionsOverride { } impl AppServerSession { - pub(crate) fn new(client: AppServerClient) -> Self { + pub(crate) fn new(client: AppServerClient, thread_params_mode: ThreadParamsMode) -> Self { Self { client, next_request_id: 1, remote_cwd_override: None, + thread_params_mode, } } @@ -199,8 +201,8 @@ impl AppServerSession { self.remote_cwd_override.as_deref() } - pub(crate) fn is_remote(&self) -> bool { - matches!(self.client, AppServerClient::Remote(_)) + pub(crate) fn uses_remote_workspace(&self) -> bool { + matches!(self.thread_params_mode, ThreadParamsMode::Remote) } pub(crate) async fn bootstrap(&mut self, config: &Config) -> Result { @@ -426,10 +428,7 @@ impl AppServerSession { } fn thread_params_mode(&self) -> ThreadParamsMode { - match &self.client { - AppServerClient::InProcess(_) => ThreadParamsMode::Embedded, - AppServerClient::Remote(_) => ThreadParamsMode::Remote, - } + self.thread_params_mode } async fn fork_parent_title_from_app_server( diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 3f93c13898..5aeb2a4cfd 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -1315,7 +1315,7 @@ impl BottomPane { self.has_input_focus, self.enhanced_keys_supported, self.disable_paste_burst, - self.keymap.list.clone(), + self.keymap.clone(), ); self.pause_status_timer_for_modal(); self.set_composer_input_enabled( diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs b/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs index 68c6755e45..13cbc34daa 100644 --- a/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs +++ b/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs @@ -4,7 +4,7 @@ //! - Each question can be answered by selecting one option and/or providing notes. //! - Notes are stored per question and appended as extra answers. //! - Typing while focused on options jumps into notes to keep freeform input fast. -//! - Enter advances to the next question; the last question submits all answers. +//! - The composer submit binding advances to the next question; the last question submits all answers. //! - Freeform-only questions submit an empty answer list when empty. use std::collections::HashMap; use std::collections::VecDeque; @@ -29,8 +29,10 @@ use crate::bottom_pane::scroll_state::ScrollState; use crate::bottom_pane::selection_popup_common::GenericDisplayRow; use crate::bottom_pane::selection_popup_common::measure_rows_height; use crate::history_cell; +use crate::key_hint::KeyBinding; use crate::key_hint::KeyBindingListExt; use crate::keymap::ListKeymap; +use crate::keymap::RuntimeKeymap; use crate::render::renderable::Renderable; #[cfg(test)] @@ -140,6 +142,7 @@ pub(crate) struct RequestUserInputOverlay { done: bool, pending_submission_draft: Option, confirm_unanswered: Option, + composer_submit_keys: Vec, list_keymap: ListKeymap, } @@ -158,7 +161,7 @@ impl RequestUserInputOverlay { has_input_focus, enhanced_keys_supported, disable_paste_burst, - crate::keymap::RuntimeKeymap::defaults().list, + RuntimeKeymap::defaults(), ) } @@ -168,7 +171,7 @@ impl RequestUserInputOverlay { has_input_focus: bool, enhanced_keys_supported: bool, disable_paste_burst: bool, - list_keymap: ListKeymap, + keymap: RuntimeKeymap, ) -> Self { // Use the same composer widget, but disable popups/slash-commands and // image-path attachment so it behaves like a focused notes field. @@ -180,6 +183,7 @@ impl RequestUserInputOverlay { disable_paste_burst, ChatComposerConfig::plain_text(), ); + composer.set_keymap_bindings(&keymap); // The overlay renders its own footer hints, so keep the composer footer empty. composer.set_footer_hint_override(Some(Vec::new())); let mut overlay = Self { @@ -193,7 +197,8 @@ impl RequestUserInputOverlay { done: false, pending_submission_draft: None, confirm_unanswered: None, - list_keymap, + composer_submit_keys: keymap.composer.submit.clone(), + list_keymap: keymap.list, }; overlay.reset_for_request(); overlay.ensure_focus_available(); @@ -477,14 +482,23 @@ impl RequestUserInputOverlay { let question_count = self.question_count(); let is_last_question = self.current_index().saturating_add(1) >= question_count; - let enter_tip = if question_count == 1 { - FooterTip::highlighted("enter to submit answer") - } else if is_last_question { - FooterTip::highlighted("enter to submit all") + let submit_key = if self.focus_is_notes() || !self.has_options() { + self.composer_submit_keys + .first() + .map(KeyBinding::display_label) } else { - FooterTip::new("enter to submit answer") + Some("enter".to_string()) }; - tips.push(enter_tip); + if let Some(submit_key) = submit_key { + let submit_tip = if question_count == 1 { + FooterTip::highlighted(format!("{submit_key} to submit answer")) + } else if is_last_question { + FooterTip::highlighted(format!("{submit_key} to submit all")) + } else { + FooterTip::new(format!("{submit_key} to submit answer")) + }; + tips.push(submit_tip); + } if question_count > 1 { if self.has_options() && !self.focus_is_notes() { tips.push(FooterTip::new("←/→ to navigate questions")); @@ -1054,6 +1068,20 @@ impl BottomPaneView for RequestUserInputOverlay { return; } + if self.focus_is_notes() && self.composer_submit_keys.is_pressed(key_event) { + self.ensure_selected_for_notes(); + self.pending_submission_draft = Some(self.capture_composer_draft()); + let (result, _) = self.composer.handle_key_event(key_event); + if !self.handle_composer_input_result(result) { + self.pending_submission_draft = None; + if self.has_options() { + self.select_current_option(/*committed*/ true); + } + self.go_next_or_submit(); + } + return; + } + // Question navigation is always available. match key_event { KeyEvent { @@ -1201,19 +1229,6 @@ impl BottomPaneView for RequestUserInputOverlay { self.sync_composer_placeholder(); return; } - if matches!(key_event.code, KeyCode::Enter) { - self.ensure_selected_for_notes(); - self.pending_submission_draft = Some(self.capture_composer_draft()); - let (result, _) = self.composer.handle_key_event(key_event); - if !self.handle_composer_input_result(result) { - self.pending_submission_draft = None; - if self.has_options() { - self.select_current_option(/*committed*/ true); - } - self.go_next_or_submit(); - } - return; - } if self.has_options() && matches!(key_event.code, KeyCode::Up | KeyCode::Down) { let options_len = self.options_len(); match key_event.code { @@ -2008,6 +2023,28 @@ mod tests { ); } + #[test] + fn freeform_footer_shows_configured_submit_binding() { + let (tx, _rx) = test_sender(); + let mut keymap = RuntimeKeymap::defaults(); + keymap.composer.submit = vec![crate::key_hint::ctrl(KeyCode::Char('j'))]; + let overlay = RequestUserInputOverlay::new_with_keymap( + request_event("turn-1", vec![question_without_options("q1", "Notes")]), + tx, + /*has_input_focus*/ true, + /*enhanced_keys_supported*/ false, + /*disable_paste_burst*/ false, + keymap, + ); + + let tips = overlay.footer_tips(); + let tip_texts = tips.iter().map(|tip| tip.text.as_str()).collect::>(); + assert_eq!( + tip_texts, + vec!["ctrl + j to submit answer", "esc to interrupt"] + ); + } + #[test] fn tab_opens_notes_when_option_selected() { let (tx, _rx) = test_sender(); @@ -2374,6 +2411,97 @@ mod tests { assert_eq!(overlay.unanswered_count(), 2); } + #[test] + fn freeform_shift_enter_inserts_newline_without_advancing() { + let (tx, _rx) = test_sender(); + let mut overlay = RequestUserInputOverlay::new( + request_event( + "turn-1", + vec![ + question_without_options("q1", "Notes"), + question_without_options("q2", "More"), + ], + ), + tx, + /*has_input_focus*/ true, + /*enhanced_keys_supported*/ true, + /*disable_paste_burst*/ false, + ); + + overlay + .composer + .set_text_content("Draft".to_string(), Vec::new(), Vec::new()); + overlay.composer.move_cursor_to_end(); + + overlay.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::SHIFT)); + + assert_eq!(overlay.current_index(), 0); + assert_eq!(overlay.composer.current_text_with_pending(), "Draft\n"); + assert_eq!(overlay.answers[0].answer_committed, false); + } + + #[test] + fn freeform_uses_configured_composer_submit_binding() { + let (tx, _rx) = test_sender(); + let mut keymap = RuntimeKeymap::defaults(); + keymap.composer.submit = vec![crate::key_hint::ctrl(KeyCode::Char('j'))]; + let mut overlay = RequestUserInputOverlay::new_with_keymap( + request_event( + "turn-1", + vec![ + question_without_options("q1", "Notes"), + question_without_options("q2", "More"), + ], + ), + tx, + /*has_input_focus*/ true, + /*enhanced_keys_supported*/ false, + /*disable_paste_burst*/ false, + keymap, + ); + + overlay + .composer + .set_text_content("Draft".to_string(), Vec::new(), Vec::new()); + overlay.composer.move_cursor_to_end(); + + overlay.handle_key_event(KeyEvent::new(KeyCode::Char('j'), KeyModifiers::CONTROL)); + + assert_eq!(overlay.current_index(), 1); + assert_eq!(overlay.answers[0].answer_committed, true); + } + + #[test] + fn freeform_submit_binding_wins_over_question_navigation() { + let (tx, _rx) = test_sender(); + let mut keymap = RuntimeKeymap::defaults(); + keymap.composer.submit = vec![crate::key_hint::ctrl(KeyCode::Char('n'))]; + let mut overlay = RequestUserInputOverlay::new_with_keymap( + request_event( + "turn-1", + vec![ + question_without_options("q1", "Notes"), + question_without_options("q2", "More"), + ], + ), + tx, + /*has_input_focus*/ true, + /*enhanced_keys_supported*/ false, + /*disable_paste_burst*/ false, + keymap, + ); + + overlay + .composer + .set_text_content("Draft".to_string(), Vec::new(), Vec::new()); + overlay.composer.move_cursor_to_end(); + + overlay.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::CONTROL)); + + assert_eq!(overlay.current_index(), 1); + assert_eq!(overlay.answers[0].answer_committed, true); + } + #[test] fn freeform_questions_submit_empty_when_empty() { let (tx, mut rx) = test_sender(); @@ -3025,6 +3153,26 @@ mod tests { ); } + #[test] + fn request_user_input_freeform_remapped_submit_snapshot() { + let (tx, _rx) = test_sender(); + let mut keymap = RuntimeKeymap::defaults(); + keymap.composer.submit = vec![crate::key_hint::ctrl(KeyCode::Char('j'))]; + let overlay = RequestUserInputOverlay::new_with_keymap( + request_event("turn-1", vec![question_without_options("q1", "Goal")]), + tx, + /*has_input_focus*/ true, + /*enhanced_keys_supported*/ false, + /*disable_paste_burst*/ false, + keymap, + ); + let area = Rect::new(0, 0, 120, 10); + insta::assert_snapshot!( + "request_user_input_freeform_remapped_submit", + render_snapshot(&overlay, area) + ); + } + #[test] fn request_user_input_multi_question_first_snapshot() { let (tx, _rx) = test_sender(); diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_freeform_remapped_submit.snap b/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_freeform_remapped_submit.snap new file mode 100644 index 0000000000..a2aeaa4d3e --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_freeform_remapped_submit.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/request_user_input/mod.rs +expression: "render_snapshot(&overlay, area)" +--- + + Question 1/1 (1 unanswered) + Share details. + + › Type your answer (optional) + + + + ctrl + j to submit answer | esc to interrupt diff --git a/codex-rs/tui/src/debug_config.rs b/codex-rs/tui/src/debug_config.rs index 7eb4539f1d..2075636e3d 100644 --- a/codex-rs/tui/src/debug_config.rs +++ b/codex-rs/tui/src/debug_config.rs @@ -700,6 +700,7 @@ mod tests { remote_sandbox_config: None, allowed_web_search_modes: Some(vec![WebSearchModeRequirement::Cached]), allow_managed_hooks_only: Some(true), + computer_use: None, guardian_policy_config: Some("Use the managed guardian policy.".to_string()), feature_requirements: Some(FeatureRequirementsToml { entries: BTreeMap::from([("guardian_approval".to_string(), true)]), @@ -914,6 +915,7 @@ approval_policy = "never" remote_sandbox_config: None, allowed_web_search_modes: Some(Vec::new()), allow_managed_hooks_only: None, + computer_use: None, guardian_policy_config: None, feature_requirements: None, hooks: None, diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 9ece5e3776..40cd121c71 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -20,6 +20,7 @@ use app::App; pub use app::AppExitInfo; pub use app::ExitReason; use app_server_session::AppServerSession; +use app_server_session::ThreadParamsMode; use codex_app_server_client::AppServerClient; use codex_app_server_client::DEFAULT_IN_PROCESS_CHANNEL_CAPACITY; use codex_app_server_client::InProcessAppServerClient; @@ -312,9 +313,24 @@ async fn start_embedded_app_server( #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum AppServerTarget { Embedded, + LocalDaemon { endpoint: RemoteAppServerEndpoint }, Remote { endpoint: RemoteAppServerEndpoint }, } +impl AppServerTarget { + pub(crate) fn uses_remote_workspace(&self) -> bool { + matches!(self, Self::Remote { .. }) + } + + fn thread_params_mode(&self) -> ThreadParamsMode { + if self.uses_remote_workspace() { + ThreadParamsMode::Remote + } else { + ThreadParamsMode::Embedded + } + } +} + async fn init_state_db_for_app_server_target( config: &Config, app_server_target: &AppServerTarget, @@ -326,7 +342,9 @@ async fn init_state_db_for_app_server_target( err.to_string(), )) }), - AppServerTarget::Remote { .. } => Ok(state_db::get_state_db(config).await), + AppServerTarget::LocalDaemon { .. } | AppServerTarget::Remote { .. } => { + Ok(state_db::get_state_db(config).await) + } } } @@ -496,7 +514,9 @@ async fn start_app_server( ) .await .map(AppServerClient::InProcess), - AppServerTarget::Remote { endpoint } => connect_remote_app_server(endpoint.clone()).await, + AppServerTarget::LocalDaemon { endpoint } | AppServerTarget::Remote { endpoint } => { + connect_remote_app_server(endpoint.clone()).await + } } } @@ -520,7 +540,10 @@ pub(crate) async fn start_app_server_for_picker( environment_manager, ) .await?; - Ok(AppServerSession::new(app_server)) + Ok(AppServerSession::new( + app_server, + target.thread_params_mode(), + )) } #[cfg(test)] @@ -695,7 +718,7 @@ async fn lookup_latest_session_target_with_app_server( ) -> color_eyre::Result> { let response = app_server .thread_list(latest_session_lookup_params( - app_server.is_remote(), + app_server.uses_remote_workspace(), config, cwd_filter, include_non_interactive, @@ -708,7 +731,7 @@ async fn lookup_latest_session_target_with_app_server( } fn latest_session_lookup_params( - is_remote: bool, + uses_remote_workspace: bool, config: &Config, cwd_filter: Option<&Path>, include_non_interactive: bool, @@ -718,7 +741,7 @@ fn latest_session_lookup_params( limit: Some(1), sort_key: Some(AppServerThreadSortKey::UpdatedAt), sort_direction: None, - model_providers: if is_remote { + model_providers: if uses_remote_workspace { None } else { Some(vec![config.model_provider_id.clone()]) @@ -737,7 +760,7 @@ fn config_cwd_for_app_server_target( app_server_target: &AppServerTarget, environment_manager: &EnvironmentManager, ) -> std::io::Result> { - if matches!(app_server_target, AppServerTarget::Remote { .. }) + if app_server_target.uses_remote_workspace() || environment_manager .default_environment() .is_some_and(|environment| environment.is_remote()) @@ -758,12 +781,11 @@ fn should_load_configured_environments( loader_overrides: &LoaderOverrides, app_server_target: &AppServerTarget, ) -> bool { - !loader_overrides.ignore_user_config - && !matches!(app_server_target, AppServerTarget::Remote { .. }) + !loader_overrides.ignore_user_config && !app_server_target.uses_remote_workspace() } fn latest_session_cwd_filter<'a>( - remote_mode: bool, + uses_remote_workspace: bool, remote_cwd_override: Option<&'a Path>, config: &'a Config, show_all: bool, @@ -772,13 +794,62 @@ fn latest_session_cwd_filter<'a>( return None; } - if remote_mode { + if uses_remote_workspace { remote_cwd_override } else { Some(config.cwd.as_path()) } } +fn app_server_target_for_launch( + explicit_remote_endpoint: Option, + default_daemon_socket: Option, + can_reuse_implicit_local_daemon: bool, +) -> AppServerTarget { + match explicit_remote_endpoint { + Some(endpoint) => AppServerTarget::Remote { endpoint }, + None if can_reuse_implicit_local_daemon => { + default_daemon_socket.map_or(AppServerTarget::Embedded, |socket_path| { + AppServerTarget::LocalDaemon { + endpoint: RemoteAppServerEndpoint::UnixSocket { socket_path }, + } + }) + } + None => AppServerTarget::Embedded, + } +} + +fn loader_overrides_are_default(loader_overrides: &LoaderOverrides) -> bool { + let loader_overrides_are_default = loader_overrides.user_config_path.is_none() + && loader_overrides.user_config_profile.is_none() + && loader_overrides.managed_config_path.is_none() + && loader_overrides.system_config_path.is_none() + && loader_overrides.system_requirements_path.is_none() + && !loader_overrides.ignore_managed_requirements + && !loader_overrides.ignore_user_config + && !loader_overrides.ignore_user_and_project_exec_policy_rules + && loader_overrides + .macos_managed_config_requirements_base64 + .is_none(); + #[cfg(target_os = "macos")] + let loader_overrides_are_default = + loader_overrides_are_default && loader_overrides.managed_preferences_base64.is_none(); + loader_overrides_are_default +} + +fn can_reuse_implicit_local_daemon( + cli_kv_overrides: &[(String, toml::Value)], + loader_overrides: &LoaderOverrides, + strict_config: bool, + has_non_replayable_launch_overrides: bool, +) -> bool { + // A reused daemon cannot adopt this invocation's full launch config state. + cli_kv_overrides.is_empty() + && loader_overrides_are_default(loader_overrides) + && !strict_config + && !has_non_replayable_launch_overrides +} + pub async fn run_main( mut cli: Cli, arg0_paths: Arg0DispatchPaths, @@ -830,21 +901,32 @@ pub async fn run_main( } }; - let remote_endpoint = match explicit_remote_endpoint { - Some(endpoint) => Some(endpoint), - None => maybe_probe_default_daemon_socket(&codex_home) - .await - .map(|socket_path| RemoteAppServerEndpoint::UnixSocket { socket_path }), + let mut launch_loader_overrides = loader_overrides.clone(); + if let Some(profile_v2) = cli.config_profile_v2.as_ref() { + let user_config_path = resolve_profile_v2_config_path(&codex_home, profile_v2); + launch_loader_overrides.user_config_path = Some(user_config_path); + launch_loader_overrides.user_config_profile = Some(profile_v2.clone()); + } + let reuse_implicit_local_daemon = can_reuse_implicit_local_daemon( + &cli_kv_overrides, + &launch_loader_overrides, + strict_config, + cli.bypass_hook_trust, + ); + let default_daemon = if explicit_remote_endpoint.is_none() && reuse_implicit_local_daemon { + maybe_probe_default_daemon_socket(&codex_home).await + } else { + None }; - let app_server_target = remote_endpoint - .clone() - .map_or(AppServerTarget::Embedded, |endpoint| { - AppServerTarget::Remote { endpoint } - }); + let app_server_target = app_server_target_for_launch( + explicit_remote_endpoint, + default_daemon, + reuse_implicit_local_daemon, + ); let remote_cwd_override = cli .cwd .clone() - .filter(|_| matches!(app_server_target, AppServerTarget::Remote { .. })); + .filter(|_| app_server_target.uses_remote_workspace()); let local_runtime_paths = ExecServerRuntimePaths::from_optional_paths( arg0_paths.codex_self_exe.clone(), @@ -852,9 +934,9 @@ pub async fn run_main( )?; let environment_manager = if should_load_configured_environments(&loader_overrides, &app_server_target) { - EnvironmentManager::from_codex_home(codex_home.clone(), local_runtime_paths).await + EnvironmentManager::from_codex_home(codex_home.clone(), Some(local_runtime_paths)).await } else { - EnvironmentManager::from_env(local_runtime_paths).await + EnvironmentManager::from_env(Some(local_runtime_paths)).await } .map(Arc::new) .map_err(std::io::Error::other)?; @@ -952,7 +1034,7 @@ pub async fn run_main( model, approval_policy, sandbox_mode, - cwd: if matches!(app_server_target, AppServerTarget::Remote { .. }) { + cwd: if app_server_target.uses_remote_workspace() { None } else { cwd @@ -1072,7 +1154,7 @@ pub async fn run_main( } } - if matches!(app_server_target, AppServerTarget::Embedded) { + if !app_server_target.uses_remote_workspace() { #[allow(clippy::print_stderr)] if let Err(err) = enforce_login_restrictions(&AuthConfig { codex_home: config.codex_home.to_path_buf(), @@ -1180,7 +1262,6 @@ pub async fn run_main( feedback, log_db, state_db, - remote_endpoint, environment_manager, ) .await @@ -1202,10 +1283,9 @@ async fn run_ratatui_app( feedback: codex_feedback::CodexFeedback, log_db: Option, state_db: Option, - remote_endpoint: Option, environment_manager: Arc, ) -> color_eyre::Result { - let remote_mode = matches!(&app_server_target, AppServerTarget::Remote { .. }); + let uses_remote_workspace = app_server_target.uses_remote_workspace(); color_eyre::install()?; tooltips::announcement::prewarm(); @@ -1268,7 +1348,7 @@ async fn run_ratatui_app( ) .await { - Ok(app_server) => AppServerSession::new(app_server), + Ok(app_server) => AppServerSession::new(app_server, app_server_target.thread_params_mode()), Err(err) => { terminal_restore_guard.restore_silently(); session_log::log_session_end(); @@ -1278,7 +1358,8 @@ async fn run_ratatui_app( .with_remote_cwd_override(remote_cwd_override.clone()); let mut app_server = Some(app_server_session); - let should_show_trust_screen_flag = !remote_mode && should_show_trust_screen(&initial_config); + let should_show_trust_screen_flag = + !uses_remote_workspace && should_show_trust_screen(&initial_config); let mut trust_decision_was_made = false; let login_status = if initial_config.model_provider.requires_openai_auth { let Some(app_server) = app_server.as_mut() else { @@ -1328,7 +1409,7 @@ async fn run_ratatui_app( // If this onboarding run included the login step, always refresh cloud requirements and // rebuild config. This avoids missing newly available cloud requirements due to login // status detection edge cases. - if show_login_screen && !remote_mode { + if show_login_screen && !uses_remote_workspace { cloud_requirements = cloud_requirements_loader_for_storage( initial_config.codex_home.to_path_buf(), /*enable_codex_api_key_env*/ false, @@ -1341,7 +1422,7 @@ async fn run_ratatui_app( // If the user made an explicit trust decision, or we showed the login flow, reload config // so current process state reflects persisted trust/auth changes. if onboarding_result.directory_trust_decision.is_some() - || (show_login_screen && !remote_mode) + || (show_login_screen && !uses_remote_workspace) { load_config_or_exit( cli_kv_overrides.clone(), @@ -1389,7 +1470,7 @@ async fn run_ratatui_app( } } else if cli.fork_last { let filter_cwd = latest_session_cwd_filter( - remote_mode, + uses_remote_workspace, remote_cwd_override.as_deref(), &config, cli.fork_show_all, @@ -1446,7 +1527,7 @@ async fn run_ratatui_app( } } else if cli.resume_last { let filter_cwd = latest_session_cwd_filter( - remote_mode, + uses_remote_workspace, remote_cwd_override.as_deref(), &config, cli.resume_show_all, @@ -1496,7 +1577,7 @@ async fn run_ratatui_app( }; let current_cwd = config.cwd.clone(); - let allow_prompt = !remote_mode && cli.cwd.is_none(); + let allow_prompt = !uses_remote_workspace && cli.cwd.is_none(); let action_and_target_session_if_resume_or_fork = match &session_selection { resume_picker::SessionSelection::Resume(target_session) => { Some((CwdPromptAction::Resume, target_session)) @@ -1508,7 +1589,7 @@ async fn run_ratatui_app( }; let fallback_cwd = match action_and_target_session_if_resume_or_fork { Some((action, target_session)) => { - if remote_mode { + if uses_remote_workspace { Some(current_cwd.to_path_buf()) } else { match resolve_cwd_for_resume_or_fork( @@ -1614,8 +1695,10 @@ async fn run_ratatui_app( ) .await { - Ok(app_server) => AppServerSession::new(app_server) - .with_remote_cwd_override(remote_cwd_override.clone()), + Ok(app_server) => { + AppServerSession::new(app_server, app_server_target.thread_params_mode()) + .with_remote_cwd_override(remote_cwd_override.clone()) + } Err(err) => { terminal_restore_guard.restore_silently(); session_log::log_session_end(); @@ -1645,7 +1728,7 @@ async fn run_ratatui_app( should_show_trust_screen, // Proxy to: is it a first run in this directory? should_show_trust_screen_flag, // Preserve the startup-time trust NUX signal before onboarding should_prompt_windows_sandbox_nux_at_startup, - remote_endpoint, + app_server_target, state_db, environment_manager, startup_hooks_browser, @@ -1989,6 +2072,117 @@ mod tests { Ok(()) } + #[test] + fn app_server_target_for_launch_uses_local_daemon_for_default_socket() -> color_eyre::Result<()> + { + let socket_path = AbsolutePathBuf::relative_to_current_dir("codex.sock")?; + let target = app_server_target_for_launch( + /*explicit_remote_endpoint*/ None, + Some(socket_path.clone()), + /*can_reuse_implicit_local_daemon*/ true, + ); + + assert_eq!( + target, + AppServerTarget::LocalDaemon { + endpoint: RemoteAppServerEndpoint::UnixSocket { socket_path }, + } + ); + assert!(!target.uses_remote_workspace()); + assert_eq!(target.thread_params_mode(), ThreadParamsMode::Embedded); + Ok(()) + } + + #[test] + fn app_server_target_for_launch_prefers_explicit_remote_endpoint() -> color_eyre::Result<()> { + let explicit_endpoint = RemoteAppServerEndpoint::UnixSocket { + socket_path: AbsolutePathBuf::relative_to_current_dir("explicit.sock")?, + }; + let target = app_server_target_for_launch( + Some(explicit_endpoint.clone()), + Some(AbsolutePathBuf::relative_to_current_dir("default.sock")?), + /*can_reuse_implicit_local_daemon*/ false, + ); + + assert_eq!( + target, + AppServerTarget::Remote { + endpoint: explicit_endpoint, + } + ); + assert!(target.uses_remote_workspace()); + assert_eq!(target.thread_params_mode(), ThreadParamsMode::Remote); + Ok(()) + } + + #[test] + fn app_server_target_for_launch_skips_local_daemon_when_launch_config_is_not_replayable() + -> color_eyre::Result<()> { + let socket_path = AbsolutePathBuf::relative_to_current_dir("codex.sock")?; + let target = app_server_target_for_launch( + /*explicit_remote_endpoint*/ None, + Some(socket_path), + /*can_reuse_implicit_local_daemon*/ false, + ); + + assert_eq!(target, AppServerTarget::Embedded); + Ok(()) + } + + #[test] + fn can_reuse_implicit_local_daemon_requires_default_launch_config() -> color_eyre::Result<()> { + let mut loader_overrides = LoaderOverrides::default(); + let cli_kv_overrides = vec![("web_search".to_string(), toml::Value::String("live".into()))]; + + assert!(can_reuse_implicit_local_daemon( + &[], + &LoaderOverrides::default(), + /*strict_config*/ false, + /*has_non_replayable_launch_overrides*/ false, + )); + assert!(!can_reuse_implicit_local_daemon( + &cli_kv_overrides, + &LoaderOverrides::default(), + /*strict_config*/ false, + /*has_non_replayable_launch_overrides*/ false, + )); + loader_overrides.ignore_user_config = true; + assert!(!can_reuse_implicit_local_daemon( + &[], + &loader_overrides, + /*strict_config*/ false, + /*has_non_replayable_launch_overrides*/ false, + )); + assert!(!can_reuse_implicit_local_daemon( + &[], + &LoaderOverrides::default(), + /*strict_config*/ true, + /*has_non_replayable_launch_overrides*/ false, + )); + assert!(!can_reuse_implicit_local_daemon( + &[], + &LoaderOverrides::default(), + /*strict_config*/ false, + /*has_non_replayable_launch_overrides*/ true, + )); + Ok(()) + } + + #[test] + fn should_load_configured_environments_for_local_daemon() -> color_eyre::Result<()> { + let target = AppServerTarget::LocalDaemon { + endpoint: RemoteAppServerEndpoint::UnixSocket { + socket_path: AbsolutePathBuf::relative_to_current_dir("codex.sock")?, + }, + }; + + assert!(should_load_configured_environments( + &LoaderOverrides::default(), + &target, + )); + Ok(()) + } + #[tokio::test] async fn latest_session_lookup_params_keep_local_filters_for_embedded_sessions() -> std::io::Result<()> { @@ -1997,7 +2191,34 @@ mod tests { let cwd = temp_dir.path().join("project"); let params = latest_session_lookup_params( - /*is_remote*/ false, + /*uses_remote_workspace*/ false, + &config, + Some(cwd.as_path()), + /*include_non_interactive*/ false, + ); + + assert_eq!(params.model_providers, Some(vec![config.model_provider_id])); + assert_eq!( + params.cwd, + Some(ThreadListCwdFilter::One(cwd.to_string_lossy().to_string())) + ); + Ok(()) + } + + #[tokio::test] + async fn latest_session_lookup_params_keep_local_filters_for_local_daemon_sessions() + -> color_eyre::Result<()> { + let temp_dir = TempDir::new()?; + let config = build_config(&temp_dir).await?; + let cwd = temp_dir.path().join("project"); + let target = AppServerTarget::LocalDaemon { + endpoint: RemoteAppServerEndpoint::UnixSocket { + socket_path: AbsolutePathBuf::relative_to_current_dir("codex.sock")?, + }, + }; + + let params = latest_session_lookup_params( + target.uses_remote_workspace(), &config, Some(cwd.as_path()), /*include_non_interactive*/ false, @@ -2018,7 +2239,7 @@ mod tests { let config = build_config(&temp_dir).await?; let params = latest_session_lookup_params( - /*is_remote*/ true, &config, /*cwd_filter*/ None, + /*uses_remote_workspace*/ true, &config, /*cwd_filter*/ None, /*include_non_interactive*/ false, ); @@ -2035,7 +2256,7 @@ mod tests { let cwd = Path::new("repo/on/server"); let params = latest_session_lookup_params( - /*is_remote*/ true, + /*uses_remote_workspace*/ true, &config, Some(cwd), /*include_non_interactive*/ false, @@ -2056,15 +2277,15 @@ mod tests { let remote_cwd = Path::new("repo/on/server"); let local_filter = latest_session_cwd_filter( - /*remote_mode*/ false, /*remote_cwd_override*/ None, &config, + /*uses_remote_workspace*/ false, /*remote_cwd_override*/ None, &config, /*show_all*/ false, ); let show_all_filter = latest_session_cwd_filter( - /*remote_mode*/ false, /*remote_cwd_override*/ None, &config, + /*uses_remote_workspace*/ false, /*remote_cwd_override*/ None, &config, /*show_all*/ true, ); let remote_filter = latest_session_cwd_filter( - /*remote_mode*/ true, + /*uses_remote_workspace*/ true, Some(remote_cwd), &config, /*show_all*/ false, @@ -2189,12 +2410,14 @@ mod tests { &other_cwd, )?; - let mut app_server = - AppServerSession::new(codex_app_server_client::AppServerClient::InProcess( + let mut app_server = AppServerSession::new( + codex_app_server_client::AppServerClient::InProcess( start_test_embedded_app_server(config.clone()).await?, - )); + ), + ThreadParamsMode::Embedded, + ); let filter_cwd = latest_session_cwd_filter( - /*remote_mode*/ false, /*remote_cwd_override*/ None, &config, + /*uses_remote_workspace*/ false, /*remote_cwd_override*/ None, &config, /*show_all*/ false, ); let scoped_target = lookup_latest_session_target_with_app_server( @@ -2206,7 +2429,7 @@ mod tests { .await? .expect("expected project-scoped fork --last target"); let show_all_filter_cwd = latest_session_cwd_filter( - /*remote_mode*/ false, /*remote_cwd_override*/ None, &config, + /*uses_remote_workspace*/ false, /*remote_cwd_override*/ None, &config, /*show_all*/ true, ); let show_all_target = lookup_latest_session_target_with_app_server( @@ -2265,6 +2488,29 @@ mod tests { Ok(()) } + #[tokio::test] + async fn config_cwd_for_app_server_target_canonicalizes_local_daemon_cli_cwd() + -> std::io::Result<()> { + let temp_dir = TempDir::new()?; + let target = AppServerTarget::LocalDaemon { + endpoint: RemoteAppServerEndpoint::UnixSocket { + socket_path: AbsolutePathBuf::relative_to_current_dir("codex.sock")?, + }, + }; + let environment_manager = EnvironmentManager::default_for_tests(); + + let config_cwd = + config_cwd_for_app_server_target(Some(temp_dir.path()), &target, &environment_manager)?; + + assert_eq!( + config_cwd, + Some(AbsolutePathBuf::from_absolute_path(dunce::canonicalize( + temp_dir.path() + )?)?) + ); + Ok(()) + } + #[tokio::test] async fn config_cwd_for_app_server_target_errors_for_missing_embedded_cli_cwd() -> std::io::Result<()> { @@ -2291,10 +2537,10 @@ mod tests { let target = AppServerTarget::Embedded; let environment_manager = EnvironmentManager::create_for_tests( Some("ws://127.0.0.1:8765".to_string()), - ExecServerRuntimePaths::new( + Some(ExecServerRuntimePaths::new( std::env::current_exe().expect("current exe"), /*codex_linux_sandbox_exe*/ None, - )?, + )?), ) .await; @@ -2388,10 +2634,12 @@ mod tests { .await .map_err(std::io::Error::other)?; - let mut app_server = - AppServerSession::new(codex_app_server_client::AppServerClient::InProcess( + let mut app_server = AppServerSession::new( + codex_app_server_client::AppServerClient::InProcess( start_test_embedded_app_server(config).await?, - )); + ), + ThreadParamsMode::Embedded, + ); let target = lookup_session_target_by_name_with_app_server(&mut app_server, "saved-session") .await?; diff --git a/codex-rs/tui/src/resume_picker.rs b/codex-rs/tui/src/resume_picker.rs index c7be652f85..3dbc2c3da6 100644 --- a/codex-rs/tui/src/resume_picker.rs +++ b/codex-rs/tui/src/resume_picker.rs @@ -349,15 +349,15 @@ async fn run_resume_picker_with_launch_context( launch_context: SessionPickerLaunchContext, ) -> Result { let (bg_tx, bg_rx) = mpsc::unbounded_channel(); - let is_remote = app_server.is_remote(); + let uses_remote_workspace = app_server.uses_remote_workspace(); let cwd_filter = picker_cwd_filter( config.cwd.as_path(), /*show_all*/ false, - is_remote, + uses_remote_workspace, app_server.remote_cwd_override(), ); - let local_filter_cwd = local_picker_cwd_filter(&cwd_filter, is_remote); - let provider_filter = picker_provider_filter(config, is_remote); + let local_filter_cwd = local_picker_cwd_filter(&cwd_filter, uses_remote_workspace); + let provider_filter = picker_provider_filter(config, uses_remote_workspace); let runtime_keymap = picker_runtime_keymap(config)?; let options = SessionPickerRunOptions { show_all, @@ -395,15 +395,15 @@ pub async fn run_fork_picker_with_app_server( app_server: AppServerSession, ) -> Result { let (bg_tx, bg_rx) = mpsc::unbounded_channel(); - let is_remote = app_server.is_remote(); + let uses_remote_workspace = app_server.uses_remote_workspace(); let cwd_filter = picker_cwd_filter( config.cwd.as_path(), /*show_all*/ false, - is_remote, + uses_remote_workspace, app_server.remote_cwd_override(), ); - let local_filter_cwd = local_picker_cwd_filter(&cwd_filter, is_remote); - let provider_filter = picker_provider_filter(config, is_remote); + let local_filter_cwd = local_picker_cwd_filter(&cwd_filter, uses_remote_workspace); + let provider_filter = picker_provider_filter(config, uses_remote_workspace); let runtime_keymap = picker_runtime_keymap(config)?; let options = SessionPickerRunOptions { show_all, @@ -513,12 +513,19 @@ fn raw_reasoning_visibility(config: &Config) -> RawReasoningVisibility { } } -fn local_picker_cwd_filter(cwd_filter: &Option, is_remote: bool) -> Option { - if is_remote { None } else { cwd_filter.clone() } +fn local_picker_cwd_filter( + cwd_filter: &Option, + uses_remote_workspace: bool, +) -> Option { + if uses_remote_workspace { + None + } else { + cwd_filter.clone() + } } -fn picker_provider_filter(config: &Config, is_remote: bool) -> ProviderFilter { - if is_remote { +fn picker_provider_filter(config: &Config, uses_remote_workspace: bool) -> ProviderFilter { + if uses_remote_workspace { ProviderFilter::Any } else { ProviderFilter::MatchDefault(config.model_provider_id.to_string()) @@ -533,12 +540,12 @@ fn picker_runtime_keymap(config: &Config) -> Result { fn picker_cwd_filter( config_cwd: &Path, show_all: bool, - is_remote: bool, + uses_remote_workspace: bool, remote_cwd_override: Option<&Path>, ) -> Option { if show_all { None - } else if is_remote { + } else if uses_remote_workspace { remote_cwd_override.map(Path::to_path_buf) } else { Some(config_cwd.to_path_buf()) @@ -3309,7 +3316,7 @@ mod tests { let cwd_filter = picker_cwd_filter( Path::new("/tmp/project"), /*show_all*/ false, - /*is_remote*/ false, + /*uses_remote_workspace*/ false, /*remote_cwd_override*/ None, ); let params = thread_list_params( @@ -3588,7 +3595,8 @@ mod tests { remote_cwd.clone(), SessionPickerAction::Resume, ); - state.local_filter_cwd = local_picker_cwd_filter(&remote_cwd, /*is_remote*/ true); + state.local_filter_cwd = + local_picker_cwd_filter(&remote_cwd, /*uses_remote_workspace*/ true); state.start_initial_load();