diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index 14aed4e57b..ffccb4cfb0 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -1028,21 +1028,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index 76d265c591..ce587a7f10 100644 --- a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json @@ -392,21 +392,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json index ef268908f9..adb50dee43 100644 --- a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json @@ -177,21 +177,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json index f49165296a..3e775a3da9 100644 --- a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json +++ b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json @@ -177,21 +177,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 629c0b97fa..aa5c944d9c 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1199,21 +1199,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/ServerRequest.json b/codex-rs/app-server-protocol/schema/json/ServerRequest.json index 50510adf98..51cab50810 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ServerRequest.json @@ -731,21 +731,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { 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 e639cc598a..1fdbfac009 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 @@ -8597,21 +8597,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { 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 5723e402cb..dd5fbae0ee 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 @@ -5116,21 +5116,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/CommandExecParams.json b/codex-rs/app-server-protocol/schema/json/v2/CommandExecParams.json index b85a0e7911..7db4d635ee 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/CommandExecParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/CommandExecParams.json @@ -146,21 +146,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json index e4a278330c..98f44e50a2 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -184,21 +184,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json index b4ad6af44b..16e47c2d72 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json @@ -177,21 +177,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { 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 d120fc8b5d..504dbdc67e 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkParams.json @@ -183,21 +183,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json index a2f2490a0b..3ab0077d0a 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json @@ -569,21 +569,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { 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 f3fe5648bf..aaa2a439cb 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.json @@ -257,21 +257,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json index 516627576e..12c38db5dc 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json @@ -569,21 +569,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartParams.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartParams.json index 5a59e280ea..bc8d83d2bc 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartParams.json @@ -209,21 +209,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json index f773c0be69..6e1637ec80 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json @@ -569,21 +569,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/TurnStartParams.json b/codex-rs/app-server-protocol/schema/json/v2/TurnStartParams.json index 559698100f..d866c03152 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/TurnStartParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/TurnStartParams.json @@ -218,21 +218,6 @@ "title": "MinimalFileSystemSpecialPath", "type": "object" }, - { - "properties": { - "kind": { - "enum": [ - "current_working_directory" - ], - "type": "string" - } - }, - "required": [ - "kind" - ], - "title": "CurrentWorkingDirectoryFileSystemSpecialPath", - "type": "object" - }, { "properties": { "kind": { diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/FileSystemSpecialPath.ts b/codex-rs/app-server-protocol/schema/typescript/v2/FileSystemSpecialPath.ts index bf27547ee7..f4dc2b01e6 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/FileSystemSpecialPath.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/FileSystemSpecialPath.ts @@ -2,4 +2,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. -export type FileSystemSpecialPath = { "kind": "root" } | { "kind": "minimal" } | { "kind": "current_working_directory" } | { "kind": "project_roots", subpath: string | null, } | { "kind": "tmpdir" } | { "kind": "slash_tmp" } | { "kind": "unknown", path: string, subpath: string | null, }; +export type FileSystemSpecialPath = { "kind": "root" } | { "kind": "minimal" } | { "kind": "project_roots", subpath: string | null, } | { "kind": "tmpdir" } | { "kind": "slash_tmp" } | { "kind": "unknown", path: string, subpath: string | null, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 26ca792c4d..5666eaebf5 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -1434,7 +1434,7 @@ v2_enum_from_core!( pub enum FileSystemSpecialPath { Root, Minimal, - CurrentWorkingDirectory, + #[serde(alias = "current_working_directory")] ProjectRoots { subpath: Option, }, @@ -1451,7 +1451,6 @@ impl From for FileSystemSpecialPath { match value { CoreFileSystemSpecialPath::Root => Self::Root, CoreFileSystemSpecialPath::Minimal => Self::Minimal, - CoreFileSystemSpecialPath::CurrentWorkingDirectory => Self::CurrentWorkingDirectory, CoreFileSystemSpecialPath::ProjectRoots { subpath } => Self::ProjectRoots { subpath }, CoreFileSystemSpecialPath::Tmpdir => Self::Tmpdir, CoreFileSystemSpecialPath::SlashTmp => Self::SlashTmp, @@ -1465,7 +1464,6 @@ impl From for CoreFileSystemSpecialPath { match value { FileSystemSpecialPath::Root => Self::Root, FileSystemSpecialPath::Minimal => Self::Minimal, - FileSystemSpecialPath::CurrentWorkingDirectory => Self::CurrentWorkingDirectory, FileSystemSpecialPath::ProjectRoots { subpath } => Self::ProjectRoots { subpath }, FileSystemSpecialPath::Tmpdir => Self::Tmpdir, FileSystemSpecialPath::SlashTmp => Self::SlashTmp, @@ -8151,6 +8149,26 @@ mod tests { .expect_err("zero glob scan depth should fail deserialization"); } + #[test] + fn legacy_current_working_directory_special_path_deserializes_as_project_roots() { + let special_path = serde_json::from_value::(json!({ + "kind": "current_working_directory", + })) + .expect("legacy cwd special path should deserialize"); + + assert_eq!( + special_path, + FileSystemSpecialPath::ProjectRoots { subpath: None } + ); + assert_eq!( + serde_json::to_value(&special_path).expect("serialize special path"), + json!({ + "kind": "project_roots", + "subpath": null, + }) + ); + } + #[test] fn permissions_request_approval_response_uses_granted_permission_profile_without_macos() { let read_only_path = if cfg!(windows) { diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 96323744da..31a399cb20 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -909,7 +909,7 @@ Run a standalone command (argv vector) in the server’s sandbox without creatin "type": "managed", "fileSystem": { "type": "restricted", "entries": [ { "path": { "type": "special", "value": { "kind": "root" } }, "access": "read" }, - { "path": { "type": "special", "value": { "kind": "current_working_directory" } }, "access": "write" } + { "path": { "type": "special", "value": { "kind": "project_roots", "subpath": null } }, "access": "write" } ] }, "network": { "enabled": false } }, @@ -1255,7 +1255,7 @@ the client can offer session-scoped and/or persistent approval choices. ### Permission requests -The built-in `request_permissions` tool sends an `item/permissions/requestApproval` JSON-RPC request to the client with the requested permission profile. This v2 payload mirrors the command-execution `additionalPermissions` shape: it can request network access and additional filesystem access. The `cwd` field identifies the directory used to resolve cwd-relative permissions such as `:cwd`, `:project_roots`, and relative deny globs. +The built-in `request_permissions` tool sends an `item/permissions/requestApproval` JSON-RPC request to the client with the requested permission profile. This v2 payload mirrors the command-execution `additionalPermissions` shape: it can request network access and additional filesystem access. The `cwd` field identifies the directory used to resolve project-root permissions and relative deny globs. ```json { diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index a1eba990c6..04de06f469 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -2,8 +2,8 @@ use crate::codex_message_processor::ApiVersion; use crate::codex_message_processor::read_rollout_items_from_rollout; use crate::codex_message_processor::read_summary_from_rollout; use crate::codex_message_processor::summary_to_thread; -use crate::error_code::INTERNAL_ERROR_CODE; -use crate::error_code::INVALID_REQUEST_ERROR_CODE; +use crate::error_code::internal_error; +use crate::error_code::invalid_request; use crate::outgoing_message::ClientRequestResult; use crate::outgoing_message::ThreadScopedOutgoingMessageSender; use crate::server_request_error::is_turn_transition_server_request_error; @@ -51,7 +51,6 @@ use codex_app_server_protocol::HookStartedNotification; use codex_app_server_protocol::InterruptConversationResponse; use codex_app_server_protocol::ItemCompletedNotification; use codex_app_server_protocol::ItemStartedNotification; -use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::McpServerElicitationAction; use codex_app_server_protocol::McpServerElicitationRequestParams; use codex_app_server_protocol::McpServerElicitationRequestResponse; @@ -1876,12 +1875,12 @@ pub(crate) async fn apply_bespoke_event_handling( if let Some(request_id) = pending { let Some(rollout_path) = conversation.rollout_path() else { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: "thread has no persisted rollout".to_string(), - data: None, - }; - outgoing.send_error(request_id, error).await; + outgoing + .send_error( + request_id, + invalid_request("thread has no persisted rollout"), + ) + .await; return; }; let response = match read_summary_from_rollout( @@ -1912,29 +1911,29 @@ pub(crate) async fn apply_bespoke_event_handling( ThreadRollbackResponse { thread } } Err(err) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!( - "failed to load rollout `{}`: {err}", - rollout_path.display() - ), - data: None, - }; - outgoing.send_error(request_id.clone(), error).await; + outgoing + .send_error( + request_id.clone(), + internal_error(format!( + "failed to load rollout `{}`: {err}", + rollout_path.display() + )), + ) + .await; return; } } } Err(err) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!( - "failed to load rollout `{}`: {err}", - rollout_path.display() - ), - data: None, - }; - outgoing.send_error(request_id.clone(), error).await; + outgoing + .send_error( + request_id.clone(), + internal_error(format!( + "failed to load rollout `{}`: {err}", + rollout_path.display() + )), + ) + .await; return; } }; @@ -2332,14 +2331,7 @@ async fn handle_thread_rollback_failed( if let Some(request_id) = pending_rollback { outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: message.clone(), - data: None, - }, - ) + .send_error(request_id, invalid_request(message)) .await; } } @@ -4008,7 +4000,7 @@ mod tests { file_system: Some(CoreFileSystemPermissions { entries: vec![FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }], @@ -4054,7 +4046,7 @@ mod tests { file_system: Some(CoreFileSystemPermissions { entries: vec![FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }], @@ -4104,7 +4096,8 @@ mod tests { "path": { "type": "special", "value": { - "kind": "current_working_directory" + "kind": "project_roots", + "subpath": null } }, "access": "write" diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index de68257ae6..df047e28de 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -497,6 +497,13 @@ enum ThreadReadViewError { mod thread_goal_handlers; use self::thread_goal_handlers::api_thread_goal_from_state; +fn thread_read_view_error(err: ThreadReadViewError) -> JSONRPCErrorError { + match err { + ThreadReadViewError::InvalidRequest(message) => invalid_request(message), + ThreadReadViewError::Internal(message) => internal_error(message), + } +} + impl Drop for ActiveLogin { fn drop(&mut self) { self.cancel(); @@ -2374,11 +2381,12 @@ impl CodexMessageProcessor { persist_extended_history, } = params; if sandbox.is_some() && permission_profile.is_some() { - self.send_invalid_request_error( - request_id, - "`permissionProfile` cannot be combined with `sandbox`".to_string(), - ) - .await; + self.outgoing + .send_error( + request_id, + invalid_request("`permissionProfile` cannot be combined with `sandbox`"), + ) + .await; return; } let environments = environments.map(|environments| { @@ -2395,7 +2403,11 @@ impl CodexMessageProcessor { .thread_manager .validate_environment_selections(environments) { - self.send_invalid_request_error(request_id, environment_selection_error_message(err)) + self.outgoing + .send_error( + request_id, + invalid_request(environment_selection_error_message(err)), + ) .await; return; } @@ -2517,246 +2529,223 @@ impl CodexMessageProcessor { experimental_raw_events: bool, request_trace: Option, ) { - let requested_cwd = typesafe_overrides.cwd.clone(); - let mut config = match config_manager - .load_with_overrides(config_overrides.clone(), typesafe_overrides.clone()) - .await - { - Ok(config) => config, - Err(err) => { - let error = config_load_error(&err); - listener_task_context - .outgoing - .send_error(request_id, error) - .await; - return; - } - }; - - // The user may have requested WorkspaceWrite or DangerFullAccess via - // the command line, though in the process of deriving the Config, it - // could be downgraded to ReadOnly (perhaps there is no sandbox - // available on Windows or the enterprise config disallows it). The cwd - // should still be considered "trusted" in this case. - let requested_permissions_trust_project = - requested_permissions_trust_project(&typesafe_overrides, config.cwd.as_path()); - let effective_permissions_trust_project = permission_profile_trusts_project( - &config.permissions.permission_profile(), - config.cwd.as_path(), - ); - - if requested_cwd.is_some() - && config.active_project.trust_level.is_none() - && (requested_permissions_trust_project || effective_permissions_trust_project) - { - let trust_target = resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &config.cwd) + let result = async { + let requested_cwd = typesafe_overrides.cwd.clone(); + let mut config = config_manager + .load_with_overrides(config_overrides.clone(), typesafe_overrides.clone()) .await - .unwrap_or_else(|| config.cwd.clone()); - let current_cli_overrides = config_manager.current_cli_overrides(); - let cli_overrides_with_trust; - let cli_overrides_for_reload = if let Err(err) = - codex_core::config::set_project_trust_level( - &listener_task_context.codex_home, - trust_target.as_path(), - TrustLevel::Trusted, - ) { - warn!( - "failed to persist trusted project state for {}; continuing with in-memory trust for this thread: {err}", - trust_target.display() - ); - let mut project = toml::map::Map::new(); - project.insert( - "trust_level".to_string(), - TomlValue::String("trusted".to_string()), - ); - let mut projects = toml::map::Map::new(); - projects.insert( - project_trust_key(trust_target.as_path()), - TomlValue::Table(project), - ); - cli_overrides_with_trust = current_cli_overrides - .iter() - .cloned() - .chain(std::iter::once(( - "projects".to_string(), - TomlValue::Table(projects), - ))) - .collect::>(); - cli_overrides_with_trust.as_slice() - } else { - current_cli_overrides.as_slice() - }; + .map_err(|err| config_load_error(&err))?; - config = match config_manager - .load_with_cli_overrides( - cli_overrides_for_reload, - config_overrides, - typesafe_overrides, - /*fallback_cwd*/ None, - ) - .await + // The user may have requested WorkspaceWrite or DangerFullAccess via + // the command line, though in the process of deriving the Config, it + // could be downgraded to ReadOnly (perhaps there is no sandbox + // available on Windows or the enterprise config disallows it). The cwd + // should still be considered "trusted" in this case. + let requested_permissions_trust_project = + requested_permissions_trust_project(&typesafe_overrides, config.cwd.as_path()); + let effective_permissions_trust_project = permission_profile_trusts_project( + &config.permissions.permission_profile(), + config.cwd.as_path(), + ); + + if requested_cwd.is_some() + && config.active_project.trust_level.is_none() + && (requested_permissions_trust_project || effective_permissions_trust_project) { - Ok(config) => config, - Err(err) => { - let error = config_load_error(&err); - listener_task_context - .outgoing - .send_error(request_id, error) - .await; - return; - } - }; - } + let trust_target = + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &config.cwd) + .await + .unwrap_or_else(|| config.cwd.clone()); + let current_cli_overrides = config_manager.current_cli_overrides(); + let cli_overrides_with_trust; + let cli_overrides_for_reload = + if let Err(err) = codex_core::config::set_project_trust_level( + &listener_task_context.codex_home, + trust_target.as_path(), + TrustLevel::Trusted, + ) { + warn!( + "failed to persist trusted project state for {}; continuing with in-memory trust for this thread: {err}", + trust_target.display() + ); + let mut project = toml::map::Map::new(); + project.insert( + "trust_level".to_string(), + TomlValue::String("trusted".to_string()), + ); + let mut projects = toml::map::Map::new(); + projects.insert( + project_trust_key(trust_target.as_path()), + TomlValue::Table(project), + ); + cli_overrides_with_trust = current_cli_overrides + .iter() + .cloned() + .chain(std::iter::once(( + "projects".to_string(), + TomlValue::Table(projects), + ))) + .collect::>(); + cli_overrides_with_trust.as_slice() + } else { + current_cli_overrides.as_slice() + }; - let instruction_sources = Self::instruction_sources_from_config(&config).await; - let environments = environments.unwrap_or_else(|| { - listener_task_context - .thread_manager - .default_environment_selections(&config.cwd) - }); - let dynamic_tools = dynamic_tools.unwrap_or_default(); - let core_dynamic_tools = if dynamic_tools.is_empty() { - Vec::new() - } else { - if let Err(message) = validate_dynamic_tools(&dynamic_tools) { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message, - data: None, - }; - listener_task_context - .outgoing - .send_error(request_id, error) - .await; - return; - } - dynamic_tools - .into_iter() - .map(|tool| CoreDynamicToolSpec { - namespace: tool.namespace, - name: tool.name, - description: tool.description, - input_schema: tool.input_schema, - defer_loading: tool.defer_loading, - }) - .collect() - }; - let core_dynamic_tool_count = core_dynamic_tools.len(); - - match listener_task_context - .thread_manager - .start_thread_with_tools_and_service_name(StartThreadWithToolsOptions { - config, - initial_history: match session_start_source - .unwrap_or(codex_app_server_protocol::ThreadStartSource::Startup) - { - codex_app_server_protocol::ThreadStartSource::Startup => InitialHistory::New, - codex_app_server_protocol::ThreadStartSource::Clear => InitialHistory::Cleared, - }, - dynamic_tools: core_dynamic_tools, - persist_extended_history, - metrics_service_name: service_name, - parent_trace: request_trace, - environments, - }) - .instrument(tracing::info_span!( - "app_server.thread_start.create_thread", - otel.name = "app_server.thread_start.create_thread", - thread_start.dynamic_tool_count = core_dynamic_tool_count, - thread_start.persist_extended_history = persist_extended_history, - )) - .await - { - Ok(new_conv) => { - let NewThread { - thread_id, - thread, - session_configured, - .. - } = new_conv; - if let Err(error) = Self::set_app_server_client_info( - thread.as_ref(), - app_server_client_name, - app_server_client_version, - ) - .await - { - listener_task_context - .outgoing - .send_error(request_id, error) - .await; - return; - } - let config_snapshot = thread - .config_snapshot() - .instrument(tracing::info_span!( - "app_server.thread_start.config_snapshot", - otel.name = "app_server.thread_start.config_snapshot", - )) - .await; - let mut thread = build_thread_from_snapshot( - thread_id, - &config_snapshot, - session_configured.rollout_path.clone(), - ); - - // Auto-attach a thread listener when starting a thread. - Self::log_listener_attach_result( - Self::ensure_conversation_listener_task( - listener_task_context.clone(), - thread_id, - request_id.connection_id, - experimental_raw_events, - ApiVersion::V2, + config = config_manager + .load_with_cli_overrides( + cli_overrides_for_reload, + config_overrides, + typesafe_overrides, + /*fallback_cwd*/ None, ) - .instrument(tracing::info_span!( - "app_server.thread_start.attach_listener", - otel.name = "app_server.thread_start.attach_listener", - thread_start.experimental_raw_events = experimental_raw_events, - )) - .await, + .await + .map_err(|err| config_load_error(&err))?; + } + + let instruction_sources = Self::instruction_sources_from_config(&config).await; + let environments = environments.unwrap_or_else(|| { + listener_task_context + .thread_manager + .default_environment_selections(&config.cwd) + }); + let dynamic_tools = dynamic_tools.unwrap_or_default(); + let core_dynamic_tools = if dynamic_tools.is_empty() { + Vec::new() + } else { + validate_dynamic_tools(&dynamic_tools).map_err(invalid_request)?; + dynamic_tools + .into_iter() + .map(|tool| CoreDynamicToolSpec { + namespace: tool.namespace, + name: tool.name, + description: tool.description, + input_schema: tool.input_schema, + defer_loading: tool.defer_loading, + }) + .collect() + }; + let core_dynamic_tool_count = core_dynamic_tools.len(); + + let NewThread { + thread_id, + thread, + session_configured, + .. + } = listener_task_context + .thread_manager + .start_thread_with_tools_and_service_name(StartThreadWithToolsOptions { + config, + initial_history: match session_start_source + .unwrap_or(codex_app_server_protocol::ThreadStartSource::Startup) + { + codex_app_server_protocol::ThreadStartSource::Startup => { + InitialHistory::New + } + codex_app_server_protocol::ThreadStartSource::Clear => { + InitialHistory::Cleared + } + }, + dynamic_tools: core_dynamic_tools, + persist_extended_history, + metrics_service_name: service_name, + parent_trace: request_trace, + environments, + }) + .instrument(tracing::info_span!( + "app_server.thread_start.create_thread", + otel.name = "app_server.thread_start.create_thread", + thread_start.dynamic_tool_count = core_dynamic_tool_count, + thread_start.persist_extended_history = persist_extended_history, + )) + .await + .map_err(|err| match err { + CodexErr::InvalidRequest(message) => invalid_request(message), + err => internal_error(format!("error creating thread: {err}")), + })?; + + Self::set_app_server_client_info( + thread.as_ref(), + app_server_client_name, + app_server_client_version, + ) + .await?; + + let config_snapshot = thread + .config_snapshot() + .instrument(tracing::info_span!( + "app_server.thread_start.config_snapshot", + otel.name = "app_server.thread_start.config_snapshot", + )) + .await; + let mut thread = build_thread_from_snapshot( + thread_id, + &config_snapshot, + session_configured.rollout_path.clone(), + ); + + // Auto-attach a thread listener when starting a thread. + Self::log_listener_attach_result( + Self::ensure_conversation_listener_task( + listener_task_context.clone(), thread_id, request_id.connection_id, - "thread", - ); + experimental_raw_events, + ApiVersion::V2, + ) + .instrument(tracing::info_span!( + "app_server.thread_start.attach_listener", + otel.name = "app_server.thread_start.attach_listener", + thread_start.experimental_raw_events = experimental_raw_events, + )) + .await, + thread_id, + request_id.connection_id, + "thread", + ); + listener_task_context + .thread_watch_manager + .upsert_thread_silently(thread.clone()) + .instrument(tracing::info_span!( + "app_server.thread_start.upsert_thread", + otel.name = "app_server.thread_start.upsert_thread", + )) + .await; + + thread.status = resolve_thread_status( listener_task_context .thread_watch_manager - .upsert_thread_silently(thread.clone()) + .loaded_status_for_thread(&thread.id) .instrument(tracing::info_span!( - "app_server.thread_start.upsert_thread", - otel.name = "app_server.thread_start.upsert_thread", + "app_server.thread_start.resolve_status", + otel.name = "app_server.thread_start.resolve_status", )) - .await; + .await, + /*has_in_progress_turn*/ false, + ); - thread.status = resolve_thread_status( - listener_task_context - .thread_watch_manager - .loaded_status_for_thread(&thread.id) - .instrument(tracing::info_span!( - "app_server.thread_start.resolve_status", - otel.name = "app_server.thread_start.resolve_status", - )) - .await, - /*has_in_progress_turn*/ false, - ); + let permission_profile = + thread_response_permission_profile(config_snapshot.permission_profile); - let permission_profile = - thread_response_permission_profile(config_snapshot.permission_profile); + let response = ThreadStartResponse { + thread: thread.clone(), + model: config_snapshot.model, + model_provider: config_snapshot.model_provider_id, + service_tier: config_snapshot.service_tier, + cwd: config_snapshot.cwd, + instruction_sources, + approval_policy: config_snapshot.approval_policy.into(), + approvals_reviewer: config_snapshot.approvals_reviewer.into(), + sandbox: config_snapshot.sandbox_policy.into(), + permission_profile, + reasoning_effort: config_snapshot.reasoning_effort, + }; + Ok::<_, JSONRPCErrorError>((response, thread_started_notification(thread))) + } + .await; - let response = ThreadStartResponse { - thread: thread.clone(), - model: config_snapshot.model, - model_provider: config_snapshot.model_provider_id, - service_tier: config_snapshot.service_tier, - cwd: config_snapshot.cwd, - instruction_sources, - approval_policy: config_snapshot.approval_policy.into(), - approvals_reviewer: config_snapshot.approvals_reviewer.into(), - sandbox: config_snapshot.sandbox_policy.into(), - permission_profile, - reasoning_effort: config_snapshot.reasoning_effort, - }; + match result { + Ok((response, notif)) => { listener_task_context .analytics_events_client .track_response( @@ -2776,7 +2765,6 @@ impl CodexMessageProcessor { )) .await; - let notif = thread_started_notification(thread); listener_task_context .outgoing .send_server_notification(ServerNotification::ThreadStarted(notif)) @@ -2786,23 +2774,7 @@ impl CodexMessageProcessor { )) .await; } - Err(CodexErr::InvalidRequest(message)) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message, - data: None, - }; - listener_task_context - .outgoing - .send_error(request_id, error) - .await; - } - Err(err) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("error creating thread: {err}"), - data: None, - }; + Err(error) => { listener_task_context .outgoing .send_error(request_id, error) @@ -2847,39 +2819,42 @@ impl CodexMessageProcessor { } async fn thread_archive(&self, request_id: ConnectionRequestId, params: ThreadArchiveParams) { - let thread_id = match ThreadId::from_string(¶ms.thread_id) { - Ok(id) => id, - Err(err) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid thread id: {err}"), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; + let result = self.thread_archive_response(params).await; + let archived_thread_ids = result + .as_ref() + .ok() + .map(|(_, thread_ids)| thread_ids.clone()); + self.outgoing + .send_result(request_id, result.map(|(response, _)| response)) + .await; + + if let Some(archived_thread_ids) = archived_thread_ids { + for thread_id in archived_thread_ids { + let notification = ThreadArchivedNotification { thread_id }; + self.outgoing + .send_server_notification(ServerNotification::ThreadArchived(notification)) + .await; } - }; + } + } + + async fn thread_archive_response( + &self, + params: ThreadArchiveParams, + ) -> Result<(ThreadArchiveResponse, Vec), JSONRPCErrorError> { + let thread_id = ThreadId::from_string(¶ms.thread_id) + .map_err(|err| invalid_request(format!("invalid thread id: {err}")))?; let mut thread_ids = vec![thread_id]; if let Some(state_db_ctx) = get_state_db(&self.config).await { - let descendants = match state_db_ctx.list_thread_spawn_descendants(thread_id).await { - Ok(descendants) => descendants, - Err(err) => { - self.outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!( - "failed to list spawned descendants for thread id {thread_id}: {err}" - ), - data: None, - }, - ) - .await; - return; - } - }; + let descendants = state_db_ctx + .list_thread_spawn_descendants(thread_id) + .await + .map_err(|err| { + internal_error(format!( + "failed to list spawned descendants for thread id {thread_id}: {err}" + )) + })?; let mut seen = HashSet::from([thread_id]); for descendant_id in descendants { if seen.insert(descendant_id) { @@ -2903,12 +2878,7 @@ impl CodexMessageProcessor { archive_thread_ids.push(thread_id); } } - Err(err) => { - self.outgoing - .send_error(request_id, thread_store_archive_error("archive", err)) - .await; - return; - } + Err(err) => return Err(thread_store_archive_error("archive", err)), } for descendant_thread_id in thread_ids.into_iter().skip(1) { match self @@ -2936,10 +2906,7 @@ impl CodexMessageProcessor { let mut archived_thread_ids = Vec::new(); let Some((parent_thread_id, descendant_thread_ids)) = archive_thread_ids.split_first() else { - self.outgoing - .send_response(request_id, ThreadArchiveResponse {}) - .await; - return; + return Ok((ThreadArchiveResponse {}, archived_thread_ids)); }; self.prepare_thread_for_archive(*parent_thread_id).await; @@ -2953,12 +2920,7 @@ impl CodexMessageProcessor { Ok(()) => { archived_thread_ids.push(parent_thread_id.to_string()); } - Err(err) => { - self.outgoing - .send_error(request_id, thread_store_archive_error("archive", err)) - .await; - return; - } + Err(err) => return Err(thread_store_archive_error("archive", err)), } for descendant_thread_id in descendant_thread_ids.iter().rev().copied() { @@ -2981,15 +2943,7 @@ impl CodexMessageProcessor { } } - self.outgoing - .send_response(request_id, ThreadArchiveResponse {}) - .await; - for thread_id in archived_thread_ids { - let notification = ThreadArchivedNotification { thread_id }; - self.outgoing - .send_server_notification(ServerNotification::ThreadArchived(notification)) - .await; - } + Ok((ThreadArchiveResponse {}, archived_thread_ids)) } async fn thread_increment_elicitation( @@ -2997,34 +2951,23 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ThreadIncrementElicitationParams, ) { - let (_, thread) = match self.load_thread(¶ms.thread_id).await { - Ok(value) => value, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; - - match thread.increment_out_of_band_elicitation_count().await { - Ok(count) => { - self.outgoing - .send_response( - request_id, - ThreadIncrementElicitationResponse { - count, - paused: count > 0, - }, - ) - .await; - } - Err(err) => { - self.send_internal_error( - request_id, - format!("failed to increment out-of-band elicitation counter: {err}"), - ) - .await; - } + let result = async { + let (_, thread) = self.load_thread(¶ms.thread_id).await?; + let count = thread + .increment_out_of_band_elicitation_count() + .await + .map_err(|err| { + internal_error(format!( + "failed to increment out-of-band elicitation counter: {err}" + )) + })?; + Ok::<_, JSONRPCErrorError>(ThreadIncrementElicitationResponse { + count, + paused: count > 0, + }) } + .await; + self.outgoing.send_result(request_id, result).await; } async fn thread_decrement_elicitation( @@ -3032,76 +2975,64 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ThreadDecrementElicitationParams, ) { - let (_, thread) = match self.load_thread(¶ms.thread_id).await { - Ok(value) => value, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; - - match thread.decrement_out_of_band_elicitation_count().await { - Ok(count) => { - self.outgoing - .send_response( - request_id, - ThreadDecrementElicitationResponse { - count, - paused: count > 0, - }, - ) - .await; - } - Err(CodexErr::InvalidRequest(message)) => { - self.send_invalid_request_error(request_id, message).await; - } - Err(err) => { - self.send_internal_error( - request_id, - format!("failed to decrement out-of-band elicitation counter: {err}"), - ) - .await; - } + let result = async { + let (_, thread) = self.load_thread(¶ms.thread_id).await?; + let count = thread + .decrement_out_of_band_elicitation_count() + .await + .map_err(|err| match err { + CodexErr::InvalidRequest(message) => invalid_request(message), + err => internal_error(format!( + "failed to decrement out-of-band elicitation counter: {err}" + )), + })?; + Ok::<_, JSONRPCErrorError>(ThreadDecrementElicitationResponse { + count, + paused: count > 0, + }) } + .await; + self.outgoing.send_result(request_id, result).await; } async fn thread_set_name(&self, request_id: ConnectionRequestId, params: ThreadSetNameParams) { - let ThreadSetNameParams { thread_id, name } = params; - let thread_id = match ThreadId::from_string(&thread_id) { - Ok(id) => id, - Err(err) => { - self.send_invalid_request_error(request_id, format!("invalid thread id: {err}")) - .await; - return; - } - }; - let Some(name) = codex_core::util::normalize_thread_name(&name) else { - self.send_invalid_request_error( - request_id, - "thread name must not be empty".to_string(), - ) + let result = self.thread_set_name_response(&request_id, params).await; + let notification = result + .as_ref() + .ok() + .and_then(|(_, notification)| notification.clone()); + self.outgoing + .send_result(request_id, result.map(|(response, _)| response)) .await; - return; + + if let Some(notification) = notification { + self.outgoing + .send_server_notification(ServerNotification::ThreadNameUpdated(notification)) + .await; + } + } + + async fn thread_set_name_response( + &self, + request_id: &ConnectionRequestId, + params: ThreadSetNameParams, + ) -> Result<(ThreadSetNameResponse, Option), JSONRPCErrorError> + { + let ThreadSetNameParams { thread_id, name } = params; + let thread_id = ThreadId::from_string(&thread_id) + .map_err(|err| invalid_request(format!("invalid thread id: {err}")))?; + let Some(name) = codex_core::util::normalize_thread_name(&name) else { + return Err(invalid_request("thread name must not be empty")); }; if let Ok(thread) = self.thread_manager.get_thread(thread_id).await { - if let Err(err) = self - .submit_core_op(&request_id, thread.as_ref(), Op::SetThreadName { name }) + self.submit_core_op(request_id, thread.as_ref(), Op::SetThreadName { name }) .await - { - self.send_internal_error(request_id, format!("failed to set thread name: {err}")) - .await; - return; - } - - self.outgoing - .send_response(request_id, ThreadSetNameResponse {}) - .await; - return; + .map_err(|err| internal_error(format!("failed to set thread name: {err}")))?; + return Ok((ThreadSetNameResponse {}, None)); } - if let Err(err) = self - .thread_store + self.thread_store .update_thread_metadata(StoreUpdateThreadMetadataParams { thread_id, patch: StoreThreadMetadataPatch { @@ -3111,23 +3042,15 @@ impl CodexMessageProcessor { include_archived: false, }) .await - { - self.outgoing - .send_error(request_id, thread_store_write_error("set thread name", err)) - .await; - return; - } + .map_err(|err| thread_store_write_error("set thread name", err))?; - self.outgoing - .send_response(request_id, ThreadSetNameResponse {}) - .await; - let notification = ThreadNameUpdatedNotification { - thread_id: thread_id.to_string(), - thread_name: Some(name), - }; - self.outgoing - .send_server_notification(ServerNotification::ThreadNameUpdated(notification)) - .await; + Ok(( + ThreadSetNameResponse {}, + Some(ThreadNameUpdatedNotification { + thread_id: thread_id.to_string(), + thread_name: Some(name), + }), + )) } async fn thread_memory_mode_set( @@ -3135,43 +3058,35 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ThreadMemoryModeSetParams, ) { + let result = self.thread_memory_mode_set_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn thread_memory_mode_set_response( + &self, + params: ThreadMemoryModeSetParams, + ) -> Result { let ThreadMemoryModeSetParams { thread_id, mode } = params; - let thread_id = match ThreadId::from_string(&thread_id) { - Ok(id) => id, - Err(err) => { - self.send_invalid_request_error(request_id, format!("invalid thread id: {err}")) - .await; - return; - } - }; + let thread_id = ThreadId::from_string(&thread_id) + .map_err(|err| invalid_request(format!("invalid thread id: {err}")))?; if let Ok(thread) = self.thread_manager.get_thread(thread_id).await { if thread.config_snapshot().await.ephemeral { - self.send_invalid_request_error( - request_id, - format!("ephemeral thread does not support memory mode updates: {thread_id}"), - ) - .await; - return; + return Err(invalid_request(format!( + "ephemeral thread does not support memory mode updates: {thread_id}" + ))); } - if let Err(err) = thread.set_thread_memory_mode(mode.to_core()).await { - self.send_internal_error( - request_id, - format!("failed to set thread memory mode: {err}"), - ) - .await; - return; - } - - self.outgoing - .send_response(request_id, ThreadMemoryModeSetResponse {}) - .await; - return; + thread + .set_thread_memory_mode(mode.to_core()) + .await + .map_err(|err| { + internal_error(format!("failed to set thread memory mode: {err}")) + })?; + return Ok(ThreadMemoryModeSetResponse {}); } - if let Err(err) = self - .thread_store + self.thread_store .update_thread_metadata(StoreUpdateThreadMetadataParams { thread_id, patch: StoreThreadMetadataPatch { @@ -3181,63 +3096,40 @@ impl CodexMessageProcessor { include_archived: false, }) .await - { - self.outgoing - .send_error( - request_id, - thread_store_write_error("set thread memory mode", err), - ) - .await; - return; - } + .map_err(|err| thread_store_write_error("set thread memory mode", err))?; - self.outgoing - .send_response(request_id, ThreadMemoryModeSetResponse {}) - .await; + Ok(ThreadMemoryModeSetResponse {}) } async fn memory_reset(&self, request_id: ConnectionRequestId, _params: Option<()>) { - let state_db = match StateRuntime::init( + let result = self.memory_reset_response().await; + self.outgoing.send_result(request_id, result).await; + } + + async fn memory_reset_response(&self) -> Result { + let state_db = StateRuntime::init( self.config.sqlite_home.clone(), self.config.model_provider_id.clone(), ) .await - { - Ok(state_db) => state_db, - Err(err) => { - self.send_internal_error( - request_id, - format!("failed to open state db for memory reset: {err}"), - ) - .await; - return; - } - }; + .map_err(|err| { + internal_error(format!("failed to open state db for memory reset: {err}")) + })?; - if let Err(err) = state_db.clear_memory_data().await { - self.send_internal_error( - request_id, - format!("failed to clear memory rows in state db: {err}"), - ) - .await; - return; - } + state_db.clear_memory_data().await.map_err(|err| { + internal_error(format!("failed to clear memory rows in state db: {err}")) + })?; - if let Err(err) = clear_memory_roots_contents(&self.config.codex_home).await { - self.send_internal_error( - request_id, - format!( + clear_memory_roots_contents(&self.config.codex_home) + .await + .map_err(|err| { + internal_error(format!( "failed to clear memory directories under {}: {err}", self.config.codex_home.display() - ), - ) - .await; - return; - } + )) + })?; - self.outgoing - .send_response(request_id, MemoryResetResponse {}) - .await; + Ok(MemoryResetResponse {}) } async fn thread_metadata_update( @@ -3245,19 +3137,21 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ThreadMetadataUpdateParams, ) { + let result = self.thread_metadata_update_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn thread_metadata_update_response( + &self, + params: ThreadMetadataUpdateParams, + ) -> Result { let ThreadMetadataUpdateParams { thread_id, git_info, } = params; - let thread_uuid = match ThreadId::from_string(&thread_id) { - Ok(id) => id, - Err(err) => { - self.send_invalid_request_error(request_id, format!("invalid thread id: {err}")) - .await; - return; - } - }; + let thread_uuid = ThreadId::from_string(&thread_id) + .map_err(|err| invalid_request(format!("invalid thread id: {err}")))?; let Some(ThreadMetadataGitInfoUpdateParams { sha, @@ -3265,21 +3159,11 @@ impl CodexMessageProcessor { origin_url, }) = git_info else { - self.send_invalid_request_error( - request_id, - "gitInfo must include at least one field".to_string(), - ) - .await; - return; + return Err(invalid_request("gitInfo must include at least one field")); }; if sha.is_none() && branch.is_none() && origin_url.is_none() { - self.send_invalid_request_error( - request_id, - "gitInfo must include at least one field".to_string(), - ) - .await; - return; + return Err(invalid_request("gitInfo must include at least one field")); } let loaded_thread = self.thread_manager.get_thread(thread_uuid).await.ok(); @@ -3288,72 +3172,20 @@ impl CodexMessageProcessor { state_db_ctx = get_state_db(&self.config).await; } let Some(state_db_ctx) = state_db_ctx else { - self.send_internal_error( - request_id, - format!("sqlite state db unavailable for thread {thread_uuid}"), - ) - .await; - return; + return Err(internal_error(format!( + "sqlite state db unavailable for thread {thread_uuid}" + ))); }; - if let Err(error) = self - .ensure_thread_metadata_row_exists(thread_uuid, &state_db_ctx, loaded_thread.as_ref()) - .await - { - self.outgoing.send_error(request_id, error).await; - return; - } + self.ensure_thread_metadata_row_exists(thread_uuid, &state_db_ctx, loaded_thread.as_ref()) + .await?; - let git_sha = match sha { - Some(Some(sha)) => { - let sha = sha.trim().to_string(); - if sha.is_empty() { - self.send_invalid_request_error( - request_id, - "gitInfo.sha must not be empty".to_string(), - ) - .await; - return; - } - Some(Some(sha)) - } - Some(None) => Some(None), - None => None, - }; - let git_branch = match branch { - Some(Some(branch)) => { - let branch = branch.trim().to_string(); - if branch.is_empty() { - self.send_invalid_request_error( - request_id, - "gitInfo.branch must not be empty".to_string(), - ) - .await; - return; - } - Some(Some(branch)) - } - Some(None) => Some(None), - None => None, - }; - let git_origin_url = match origin_url { - Some(Some(origin_url)) => { - let origin_url = origin_url.trim().to_string(); - if origin_url.is_empty() { - self.send_invalid_request_error( - request_id, - "gitInfo.originUrl must not be empty".to_string(), - ) - .await; - return; - } - Some(Some(origin_url)) - } - Some(None) => Some(None), - None => None, - }; + let git_sha = Self::normalize_thread_metadata_git_field(sha, "gitInfo.sha")?; + let git_branch = Self::normalize_thread_metadata_git_field(branch, "gitInfo.branch")?; + let git_origin_url = + Self::normalize_thread_metadata_git_field(origin_url, "gitInfo.originUrl")?; - let updated = match state_db_ctx + let updated = state_db_ctx .update_thread_git_info( thread_uuid, git_sha.as_ref().map(|value| value.as_deref()), @@ -3361,35 +3193,23 @@ impl CodexMessageProcessor { git_origin_url.as_ref().map(|value| value.as_deref()), ) .await - { - Ok(updated) => updated, - Err(err) => { - self.send_internal_error( - request_id, - format!("failed to update thread metadata for {thread_uuid}: {err}"), - ) - .await; - return; - } - }; + .map_err(|err| { + internal_error(format!( + "failed to update thread metadata for {thread_uuid}: {err}" + )) + })?; if !updated { - self.send_internal_error( - request_id, - format!("thread metadata disappeared before update completed: {thread_uuid}"), - ) - .await; - return; + return Err(internal_error(format!( + "thread metadata disappeared before update completed: {thread_uuid}" + ))); } let Some(summary) = read_summary_from_state_db_context_by_thread_id(Some(&state_db_ctx), thread_uuid).await else { - self.send_internal_error( - request_id, - format!("failed to reload updated thread metadata for {thread_uuid}"), - ) - .await; - return; + return Err(internal_error(format!( + "failed to reload updated thread metadata for {thread_uuid}" + ))); }; let mut thread = summary_to_thread(summary, &self.config.cwd); @@ -3401,9 +3221,24 @@ impl CodexMessageProcessor { /*has_in_progress_turn*/ false, ); - self.outgoing - .send_response(request_id, ThreadMetadataUpdateResponse { thread }) - .await; + Ok(ThreadMetadataUpdateResponse { thread }) + } + + fn normalize_thread_metadata_git_field( + value: Option>, + name: &str, + ) -> Result>, JSONRPCErrorError> { + match value { + Some(Some(value)) => { + let value = value.trim().to_string(); + if value.is_empty() { + return Err(invalid_request(format!("{name} must not be empty"))); + } + Ok(Some(Some(value))) + } + Some(None) => Ok(Some(None)), + None => Ok(None), + } } async fn ensure_thread_metadata_row_exists( @@ -3412,22 +3247,6 @@ impl CodexMessageProcessor { state_db_ctx: &Arc, loaded_thread: Option<&Arc>, ) -> Result<(), JSONRPCErrorError> { - fn invalid_request(message: String) -> JSONRPCErrorError { - JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message, - data: None, - } - } - - fn internal_error(message: String) -> JSONRPCErrorError { - JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message, - data: None, - } - } - match state_db_ctx.get_thread(thread_uuid).await { Ok(Some(_)) => return Ok(()), Ok(None) => {} @@ -3543,21 +3362,34 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ThreadUnarchiveParams, ) { - let thread_id = match ThreadId::from_string(¶ms.thread_id) { - Ok(id) => id, - Err(err) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid thread id: {err}"), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; - } - }; + let result = self.thread_unarchive_response(params).await; + let notification = + result + .as_ref() + .ok() + .map(|(_, thread_id)| ThreadUnarchivedNotification { + thread_id: thread_id.clone(), + }); + self.outgoing + .send_result(request_id, result.map(|(response, _)| response)) + .await; + + if let Some(notification) = notification { + self.outgoing + .send_server_notification(ServerNotification::ThreadUnarchived(notification)) + .await; + } + } + + async fn thread_unarchive_response( + &self, + params: ThreadUnarchiveParams, + ) -> Result<(ThreadUnarchiveResponse, String), JSONRPCErrorError> { + let thread_id = ThreadId::from_string(¶ms.thread_id) + .map_err(|err| invalid_request(format!("invalid thread id: {err}")))?; let fallback_provider = self.config.model_provider_id.clone(); - let result = self + let mut thread = self .thread_store .unarchive_thread(StoreArchiveThreadParams { thread_id }) .await @@ -3570,50 +3402,42 @@ impl CodexMessageProcessor { message: format!("failed to read unarchived thread {thread_id}"), data: None, }) - }); + })?; - match result { - Ok(mut thread) => { - thread.status = resolve_thread_status( - self.thread_watch_manager - .loaded_status_for_thread(&thread.id) - .await, - /*has_in_progress_turn*/ false, - ); - self.attach_thread_name(thread_id, &mut thread).await; - let thread_id = thread.id.clone(); - let response = ThreadUnarchiveResponse { thread }; - self.outgoing.send_response(request_id, response).await; - let notification = ThreadUnarchivedNotification { thread_id }; - self.outgoing - .send_server_notification(ServerNotification::ThreadUnarchived(notification)) - .await; - } - Err(err) => { - self.outgoing.send_error(request_id, err).await; - } - } + thread.status = resolve_thread_status( + self.thread_watch_manager + .loaded_status_for_thread(&thread.id) + .await, + /*has_in_progress_turn*/ false, + ); + self.attach_thread_name(thread_id, &mut thread).await; + let thread_id = thread.id.clone(); + Ok((ThreadUnarchiveResponse { thread }, thread_id)) } async fn thread_rollback(&self, request_id: ConnectionRequestId, params: ThreadRollbackParams) { + let result = self + .thread_rollback_start(&request_id, params) + .await + .map(|()| None::); + self.send_optional_result(request_id, result).await; + } + + async fn thread_rollback_start( + &self, + request_id: &ConnectionRequestId, + params: ThreadRollbackParams, + ) -> Result<(), JSONRPCErrorError> { let ThreadRollbackParams { thread_id, num_turns, } = params; if num_turns == 0 { - self.send_invalid_request_error(request_id, "numTurns must be >= 1".to_string()) - .await; - return; + return Err(invalid_request("numTurns must be >= 1")); } - let (thread_id, thread) = match self.load_thread(&thread_id).await { - Ok(v) => v, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; + let (thread_id, thread) = self.load_thread(&thread_id).await?; let request = request_id.clone(); @@ -3628,17 +3452,14 @@ impl CodexMessageProcessor { } }; if rollback_already_in_progress { - self.send_invalid_request_error( - request.clone(), - "rollback already in progress for this thread".to_string(), - ) - .await; - return; + return Err(invalid_request( + "rollback already in progress for this thread", + )); } if let Err(err) = self .submit_core_op( - &request_id, + request_id, thread.as_ref(), Op::ThreadRollback { num_turns }, ) @@ -3649,9 +3470,9 @@ impl CodexMessageProcessor { let thread_state = self.thread_state_manager.thread_state(thread_id).await; thread_state.lock().await.pending_rollbacks = None; - self.send_internal_error(request, format!("failed to start rollback: {err}")) - .await; + return Err(internal_error(format!("failed to start rollback: {err}"))); } + Ok(()) } async fn thread_compact_start( @@ -3661,28 +3482,15 @@ impl CodexMessageProcessor { ) { let ThreadCompactStartParams { thread_id } = params; - let (_, thread) = match self.load_thread(&thread_id).await { - Ok(v) => v, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; - - match self - .submit_core_op(&request_id, thread.as_ref(), Op::Compact) - .await - { - Ok(_) => { - self.outgoing - .send_response(request_id, ThreadCompactStartResponse {}) - .await; - } - Err(err) => { - self.send_internal_error(request_id, format!("failed to start compaction: {err}")) - .await; - } + let result = async { + let (_, thread) = self.load_thread(&thread_id).await?; + self.submit_core_op(&request_id, thread.as_ref(), Op::Compact) + .await + .map_err(|err| internal_error(format!("failed to start compaction: {err}")))?; + Ok::<_, JSONRPCErrorError>(ThreadCompactStartResponse {}) } + .await; + self.outgoing.send_result(request_id, result).await; } async fn thread_background_terminals_clean( @@ -3692,31 +3500,17 @@ impl CodexMessageProcessor { ) { let ThreadBackgroundTerminalsCleanParams { thread_id } = params; - let (_, thread) = match self.load_thread(&thread_id).await { - Ok(v) => v, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; - - match self - .submit_core_op(&request_id, thread.as_ref(), Op::CleanBackgroundTerminals) - .await - { - Ok(_) => { - self.outgoing - .send_response(request_id, ThreadBackgroundTerminalsCleanResponse {}) - .await; - } - Err(err) => { - self.send_internal_error( - request_id, - format!("failed to clean background terminals: {err}"), - ) - .await; - } + let result = async { + let (_, thread) = self.load_thread(&thread_id).await?; + self.submit_core_op(&request_id, thread.as_ref(), Op::CleanBackgroundTerminals) + .await + .map_err(|err| { + internal_error(format!("failed to clean background terminals: {err}")) + })?; + Ok::<_, JSONRPCErrorError>(ThreadBackgroundTerminalsCleanResponse {}) } + .await; + self.outgoing.send_result(request_id, result).await; } async fn thread_shell_command( @@ -3724,51 +3518,25 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ThreadShellCommandParams, ) { - let ThreadShellCommandParams { thread_id, command } = params; - let command = command.trim().to_string(); - if command.is_empty() { - self.outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: "command must not be empty".to_string(), - data: None, - }, - ) - .await; - return; - } - - let (_, thread) = match self.load_thread(&thread_id).await { - Ok(v) => v, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; + let result = async { + let ThreadShellCommandParams { thread_id, command } = params; + let command = command.trim().to_string(); + if command.is_empty() { + return Err(invalid_request("command must not be empty")); } - }; - match self - .submit_core_op( + let (_, thread) = self.load_thread(&thread_id).await?; + self.submit_core_op( &request_id, thread.as_ref(), Op::RunUserShellCommand { command }, ) .await - { - Ok(_) => { - self.outgoing - .send_response(request_id, ThreadShellCommandResponse {}) - .await; - } - Err(err) => { - self.send_internal_error( - request_id, - format!("failed to start shell command: {err}"), - ) - .await; - } + .map_err(|err| internal_error(format!("failed to start shell command: {err}")))?; + Ok::<_, JSONRPCErrorError>(ThreadShellCommandResponse {}) } + .await; + self.outgoing.send_result(request_id, result).await; } async fn thread_approve_guardian_denied_action( @@ -3776,55 +3544,34 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ThreadApproveGuardianDeniedActionParams, ) { - let ThreadApproveGuardianDeniedActionParams { thread_id, event } = params; - let event = match serde_json::from_value(event) { - Ok(event) => event, - Err(err) => { - self.outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid Guardian denial event: {err}"), - data: None, - }, - ) - .await; - return; - } - }; - let (_, thread) = match self.load_thread(&thread_id).await { - Ok(v) => v, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; + let result = async { + let ThreadApproveGuardianDeniedActionParams { thread_id, event } = params; + let event = serde_json::from_value(event) + .map_err(|err| invalid_request(format!("invalid Guardian denial event: {err}")))?; + let (_, thread) = self.load_thread(&thread_id).await?; - match self - .submit_core_op( + self.submit_core_op( &request_id, thread.as_ref(), Op::ApproveGuardianDeniedAction { event }, ) .await - { - Ok(_) => { - self.outgoing - .send_response(request_id, ThreadApproveGuardianDeniedActionResponse {}) - .await; - } - Err(err) => { - self.send_internal_error( - request_id, - format!("failed to approve Guardian denial: {err}"), - ) - .await; - } + .map_err(|err| internal_error(format!("failed to approve Guardian denial: {err}")))?; + Ok::<_, JSONRPCErrorError>(ThreadApproveGuardianDeniedActionResponse {}) } + .await; + self.outgoing.send_result(request_id, result).await; } async fn thread_list(&self, request_id: ConnectionRequestId, params: ThreadListParams) { + let result = self.thread_list_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn thread_list_response( + &self, + params: ThreadListParams, + ) -> Result { let ThreadListParams { cursor, limit, @@ -3837,13 +3584,7 @@ impl CodexMessageProcessor { use_state_db_only, search_term, } = params; - let cwd_filters = match normalize_thread_list_cwd_filters(cwd) { - Ok(cwd_filters) => cwd_filters, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; + let cwd_filters = normalize_thread_list_cwd_filters(cwd)?; let requested_page_size = limit .map(|value| value as usize) @@ -3854,7 +3595,7 @@ impl CodexMessageProcessor { ThreadSortKey::UpdatedAt => StoreThreadSortKey::UpdatedAt, }; let sort_direction = sort_direction.unwrap_or(SortDirection::Desc); - let list_result = self + let (summaries, next_cursor) = self .list_threads_common( requested_page_size, cursor, @@ -3869,14 +3610,7 @@ impl CodexMessageProcessor { use_state_db_only, }, ) - .await; - let (summaries, next_cursor) = match list_result { - Ok(r) => r, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; + .await?; let backwards_cursor = summaries.first().and_then(|summary| { thread_backwards_cursor_for_sort_key(summary, store_sort_key, sort_direction) }); @@ -3912,12 +3646,11 @@ impl CodexMessageProcessor { thread }) .collect(); - let response = ThreadListResponse { + Ok(ThreadListResponse { data, next_cursor, backwards_cursor, - }; - self.outgoing.send_response(request_id, response).await; + }) } async fn thread_loaded_list( @@ -3925,6 +3658,14 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ThreadLoadedListParams, ) { + let result = self.thread_loaded_list_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn thread_loaded_list_response( + &self, + params: ThreadLoadedListParams, + ) -> Result { let ThreadLoadedListParams { cursor, limit } = params; let mut data = self .thread_manager @@ -3935,12 +3676,10 @@ impl CodexMessageProcessor { .collect::>(); if data.is_empty() { - let response = ThreadLoadedListResponse { + return Ok(ThreadLoadedListResponse { data, next_cursor: None, - }; - self.outgoing.send_response(request_id, response).await; - return; + }); } data.sort(); @@ -3949,15 +3688,7 @@ impl CodexMessageProcessor { Some(cursor) => { let cursor = match ThreadId::from_string(&cursor) { Ok(id) => id.to_string(), - Err(_) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid cursor: {cursor}"), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; - } + Err(_) => return Err(invalid_request(format!("invalid cursor: {cursor}"))), }; match data.binary_search(&cursor) { Ok(idx) => idx + 1, @@ -3972,41 +3703,34 @@ impl CodexMessageProcessor { let page = data[start..end].to_vec(); let next_cursor = page.last().filter(|_| end < total).cloned(); - let response = ThreadLoadedListResponse { + Ok(ThreadLoadedListResponse { data: page, next_cursor, - }; - self.outgoing.send_response(request_id, response).await; + }) } async fn thread_read(&self, request_id: ConnectionRequestId, params: ThreadReadParams) { + let result = self.thread_read_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn thread_read_response( + &self, + params: ThreadReadParams, + ) -> Result { let ThreadReadParams { thread_id, include_turns, } = params; - let thread_uuid = match ThreadId::from_string(&thread_id) { - Ok(id) => id, - Err(err) => { - self.send_invalid_request_error(request_id, format!("invalid thread id: {err}")) - .await; - return; - } - }; + let thread_uuid = ThreadId::from_string(&thread_id) + .map_err(|err| invalid_request(format!("invalid thread id: {err}")))?; - let thread = match self.read_thread_view(thread_uuid, include_turns).await { - Ok(thread) => thread, - Err(ThreadReadViewError::InvalidRequest(message)) => { - self.send_invalid_request_error(request_id, message).await; - return; - } - Err(ThreadReadViewError::Internal(message)) => { - self.send_internal_error(request_id, message).await; - return; - } - }; - let response = ThreadReadResponse { thread }; - self.outgoing.send_response(request_id, response).await; + let thread = self + .read_thread_view(thread_uuid, include_turns) + .await + .map_err(thread_read_view_error)?; + Ok(ThreadReadResponse { thread }) } /// Builds the API view for `thread/read` from persisted metadata plus optional live state. @@ -4164,6 +3888,14 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ThreadTurnsListParams, ) { + let result = self.thread_turns_list_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn thread_turns_list_response( + &self, + params: ThreadTurnsListParams, + ) -> Result { let ThreadTurnsListParams { thread_id, cursor, @@ -4171,14 +3903,8 @@ impl CodexMessageProcessor { sort_direction, } = params; - let thread_uuid = match ThreadId::from_string(&thread_id) { - Ok(id) => id, - Err(err) => { - self.send_invalid_request_error(request_id, format!("invalid thread id: {err}")) - .await; - return; - } - }; + let thread_uuid = ThreadId::from_string(&thread_id) + .map_err(|err| invalid_request(format!("invalid thread id: {err}")))?; let state_db_ctx = get_state_db(&self.config).await; let mut rollout_path = self @@ -4198,21 +3924,15 @@ impl CodexMessageProcessor { { Ok(path) => path, Err(err) => { - self.send_invalid_request_error( - request_id, - format!("failed to locate archived thread id {thread_uuid}: {err}"), - ) - .await; - return; + return Err(invalid_request(format!( + "failed to locate archived thread id {thread_uuid}: {err}" + ))); } }, Err(err) => { - self.send_invalid_request_error( - request_id, - format!("failed to locate thread id {thread_uuid}: {err}"), - ) - .await; - return; + return Err(invalid_request(format!( + "failed to locate thread id {thread_uuid}: {err}" + ))); } }; } @@ -4222,92 +3942,63 @@ impl CodexMessageProcessor { Ok(thread) => { rollout_path = thread.rollout_path(); if rollout_path.is_none() { - self.send_invalid_request_error( - request_id, - "ephemeral threads do not support thread/turns/list".to_string(), - ) - .await; - return; + return Err(invalid_request( + "ephemeral threads do not support thread/turns/list", + )); } } - Err(_) => { - self.send_invalid_request_error( - request_id, - format!("thread not loaded: {thread_uuid}"), - ) - .await; - return; - } + Err(_) => return Err(invalid_request(format!("thread not loaded: {thread_uuid}"))), } } let Some(rollout_path) = rollout_path.as_ref() else { - self.send_internal_error( - request_id, - format!("failed to locate rollout for thread {thread_uuid}"), - ) - .await; - return; + return Err(internal_error(format!( + "failed to locate rollout for thread {thread_uuid}" + ))); }; - match read_rollout_items_from_rollout(rollout_path).await { - Ok(items) => { - // This API optimizes network transfer by letting clients page through a - // thread's turns incrementally, but it still replays the entire rollout on - // every request. Rollback and compaction events can change earlier turns, so - // the server has to rebuild the full turn list until turn metadata is indexed - // separately. - let has_live_in_progress_turn = - match self.thread_manager.get_thread(thread_uuid).await { - Ok(thread) => matches!(thread.agent_status().await, AgentStatus::Running), - Err(_) => false, - }; - let turns = reconstruct_thread_turns_from_rollout_items( - &items, - self.thread_watch_manager - .loaded_status_for_thread(&thread_uuid.to_string()) - .await, - has_live_in_progress_turn, - ); - let page = match paginate_thread_turns( - turns, - cursor.as_deref(), - limit, - sort_direction.unwrap_or(SortDirection::Desc), - ) { - Ok(page) => page, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; - let response = ThreadTurnsListResponse { - data: page.turns, - next_cursor: page.next_cursor, - backwards_cursor: page.backwards_cursor, - }; - self.outgoing.send_response(request_id, response).await; - } + let items = match read_rollout_items_from_rollout(rollout_path).await { + Ok(items) => items, Err(err) if err.kind() == std::io::ErrorKind::NotFound => { - self.send_invalid_request_error( - request_id, - format!( - "thread {thread_uuid} is not materialized yet; thread/turns/list is unavailable before first user message" - ), - ) - .await; + return Err(invalid_request(format!( + "thread {thread_uuid} is not materialized yet; thread/turns/list is unavailable before first user message" + ))); } Err(err) => { - self.send_internal_error( - request_id, - format!( - "failed to load rollout `{}` for thread {thread_uuid}: {err}", - rollout_path.display() - ), - ) - .await; + return Err(internal_error(format!( + "failed to load rollout `{}` for thread {thread_uuid}: {err}", + rollout_path.display() + ))); } - } + }; + + // This API optimizes network transfer by letting clients page through a + // thread's turns incrementally, but it still replays the entire rollout on + // every request. Rollback and compaction events can change earlier turns, so + // the server has to rebuild the full turn list until turn metadata is indexed + // separately. + let has_live_in_progress_turn = match self.thread_manager.get_thread(thread_uuid).await { + Ok(thread) => matches!(thread.agent_status().await, AgentStatus::Running), + Err(_) => false, + }; + let turns = reconstruct_thread_turns_from_rollout_items( + &items, + self.thread_watch_manager + .loaded_status_for_thread(&thread_uuid.to_string()) + .await, + has_live_in_progress_turn, + ); + let page = paginate_thread_turns( + turns, + cursor.as_deref(), + limit, + sort_direction.unwrap_or(SortDirection::Desc), + )?; + Ok(ThreadTurnsListResponse { + data: page.turns, + next_cursor: page.next_cursor, + backwards_cursor: page.backwards_cursor, + }) } pub(crate) fn thread_created_receiver(&self) -> broadcast::Receiver { @@ -4379,30 +4070,34 @@ impl CodexMessageProcessor { .await .contains(&thread_id) { - self.send_invalid_request_error( - request_id, - format!( - "thread {thread_id} is closing; retry thread/resume after the thread is closed" - ), - ) - .await; + self.outgoing + .send_error( + request_id, + invalid_request(format!( + "thread {thread_id} is closing; retry thread/resume after the thread is closed" + )), + ) + .await; return; } if params.sandbox.is_some() && params.permission_profile.is_some() { - self.send_invalid_request_error( - request_id, - "`permissionProfile` cannot be combined with `sandbox`".to_string(), - ) - .await; + self.outgoing + .send_error( + request_id, + invalid_request("`permissionProfile` cannot be combined with `sandbox`"), + ) + .await; return; } - if self - .resume_running_thread(request_id.clone(), ¶ms) - .await - { - return; + match self.resume_running_thread(&request_id, ¶ms).await { + Ok(true) => return, + Ok(false) => {} + Err(error) => { + self.outgoing.send_error(request_id, error).await; + return; + } } let ThreadResumeParams { @@ -4426,22 +4121,20 @@ impl CodexMessageProcessor { } = params; let include_turns = !exclude_turns; - let (thread_history, resume_source_thread) = if let Some(history) = history { - let Some(thread_history) = self - .resume_thread_from_history(request_id.clone(), history.as_slice()) + let (thread_history, resume_source_thread) = match if let Some(history) = history { + self.resume_thread_from_history(history.as_slice()) .await - else { - return; - }; - (thread_history, None) + .map(|thread_history| (thread_history, None)) } else { - let Some((thread_history, stored_thread)) = self - .resume_thread_from_rollout(request_id.clone(), &thread_id, path.as_ref()) + self.resume_thread_from_rollout(&thread_id, path.as_ref()) .await - else { + .map(|(thread_history, stored_thread)| (thread_history, Some(stored_thread))) + } { + Ok(value) => value, + Err(error) => { + self.outgoing.send_error(request_id, error).await; return; - }; - (thread_history, Some(stored_thread)) + } }; let history_cwd = thread_history.session_cwd(); @@ -4501,11 +4194,9 @@ impl CodexMessageProcessor { }) => { let SessionConfiguredEvent { rollout_path, .. } = session_configured; let Some(rollout_path) = rollout_path else { - self.send_internal_error( - request_id, - format!("rollout path missing for thread {thread_id}"), - ) - .await; + let error = + internal_error(format!("rollout path missing for thread {thread_id}")); + self.outgoing.send_error(request_id, error).await; return; }; // Auto-attach a thread listener when resuming a thread. @@ -4535,7 +4226,9 @@ impl CodexMessageProcessor { { Ok(thread) => thread, Err(message) => { - self.send_internal_error(request_id, message).await; + self.outgoing + .send_error(request_id, internal_error(message)) + .await; return; } }; @@ -4612,11 +4305,7 @@ impl CodexMessageProcessor { } } Err(err) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("error resuming thread: {err}"), - data: None, - }; + let error = internal_error(format!("error resuming thread: {err}")); self.outgoing.send_error(request_id, error).await; } } @@ -4643,21 +4332,16 @@ impl CodexMessageProcessor { async fn resume_running_thread( &self, - request_id: ConnectionRequestId, + request_id: &ConnectionRequestId, params: &ThreadResumeParams, - ) -> bool { + ) -> Result { 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 { if params.history.is_some() { - self.send_invalid_request_error( - request_id, - format!( - "cannot resume thread {existing_thread_id} with history while it is already running" - ), - ) - .await; - return true; + return Err(invalid_request(format!( + "cannot resume thread {existing_thread_id} with history while it is already running" + ))); } if let (Some(requested_path), Some(active_path)) = ( @@ -4665,69 +4349,47 @@ impl CodexMessageProcessor { existing_thread.rollout_path().as_ref(), ) && requested_path != active_path { - self.send_invalid_request_error( - request_id, - format!( - "cannot resume running thread {existing_thread_id} with mismatched path: requested `{}`, active `{}`", - requested_path.display(), - active_path.display() - ), - ) - .await; - return true; + return Err(invalid_request(format!( + "cannot resume running thread {existing_thread_id} with mismatched path: requested `{}`, active `{}`", + requested_path.display(), + active_path.display() + ))); } - let Some(source_thread) = self + let source_thread = self .read_stored_thread_for_resume( - request_id.clone(), ¶ms.thread_id, params.path.as_ref(), /*include_history*/ true, ) - .await - else { - return true; - }; + .await?; if source_thread.thread_id != existing_thread_id { - self.send_invalid_request_error( - request_id, - format!( - "cannot resume running thread {existing_thread_id} from source thread {}", - source_thread.thread_id - ), - ) - .await; - return true; + return Err(invalid_request(format!( + "cannot resume running thread {existing_thread_id} from source thread {}", + source_thread.thread_id + ))); } - let Some(history_items) = source_thread + let history_items = source_thread .history .as_ref() .map(|history| history.items.clone()) - else { - self.send_internal_error( - request_id, - format!("thread {existing_thread_id} did not include persisted history"), - ) - .await; - return true; - }; + .ok_or_else(|| { + internal_error(format!( + "thread {existing_thread_id} did not include persisted history" + )) + })?; let thread_state = self .thread_state_manager .thread_state(existing_thread_id) .await; - if let Err(error) = self - .ensure_listener_task_running( - existing_thread_id, - existing_thread.clone(), - thread_state.clone(), - ApiVersion::V2, - ) - .await - { - self.outgoing.send_error(request_id, error).await; - return true; - } + self.ensure_listener_task_running( + existing_thread_id, + existing_thread.clone(), + thread_state.clone(), + ApiVersion::V2, + ) + .await?; let config_snapshot = existing_thread.config_snapshot().await; let mismatch_details = collect_resume_override_mismatches(params, &config_snapshot); @@ -4749,10 +4411,7 @@ impl CodexMessageProcessor { .await { Ok(thread) => thread, - Err(message) => { - self.send_internal_error(request_id, message).await; - return true; - } + Err(message) => return Err(internal_error(message)), }; let mut config_for_instruction_sources = self.config.as_ref().clone(); config_for_instruction_sources.cwd = config_snapshot.cwd.clone(); @@ -4764,15 +4423,9 @@ impl CodexMessageProcessor { thread_state.listener_command_tx() }; let Some(listener_command_tx) = listener_command_tx else { - let err = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!( - "failed to enqueue running thread resume for thread {existing_thread_id}: thread listener is not running" - ), - data: None, - }; - self.outgoing.send_error(request_id, err).await; - return true; + return Err(internal_error(format!( + "failed to enqueue running thread resume for thread {existing_thread_id}: thread listener is not running" + ))); }; let emit_thread_goal_update = self.config.features.enabled(Feature::Goals); @@ -4799,32 +4452,23 @@ impl CodexMessageProcessor { }), ); if listener_command_tx.send(command).is_err() { - let err = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!( - "failed to enqueue running thread resume for thread {existing_thread_id}: thread listener command channel is closed" - ), - data: None, - }; - self.outgoing.send_error(request_id, err).await; - return true; + return Err(internal_error(format!( + "failed to enqueue running thread resume for thread {existing_thread_id}: thread listener command channel is closed" + ))); } - return true; + return Ok(true); } - false + Ok(false) } async fn resume_thread_from_history( &self, - request_id: ConnectionRequestId, history: &[ResponseItem], - ) -> Option { + ) -> Result { if history.is_empty() { - self.send_invalid_request_error(request_id, "history must not be empty".to_string()) - .await; - return None; + return Err(invalid_request("history must not be empty")); } - Some(InitialHistory::Forked( + Ok(InitialHistory::Forked( history .iter() .cloned() @@ -4835,34 +4479,24 @@ impl CodexMessageProcessor { async fn resume_thread_from_rollout( &self, - request_id: ConnectionRequestId, thread_id: &str, path: Option<&PathBuf>, - ) -> Option<(InitialHistory, StoredThread)> { - match self - .read_stored_thread_for_resume( - request_id.clone(), - thread_id, - path, - /*include_history*/ true, - ) - .await - { - Some(stored_thread) => self - .stored_thread_to_initial_history(request_id, &stored_thread) - .await - .map(|history| (history, stored_thread)), - None => None, - } + ) -> Result<(InitialHistory, StoredThread), JSONRPCErrorError> { + let stored_thread = self + .read_stored_thread_for_resume(thread_id, path, /*include_history*/ true) + .await?; + let history = self + .stored_thread_to_initial_history(&stored_thread) + .await?; + Ok((history, stored_thread)) } async fn read_stored_thread_for_resume( &self, - request_id: ConnectionRequestId, thread_id: &str, path: Option<&PathBuf>, include_history: bool, - ) -> Option { + ) -> Result { let result = if let Some(path) = path { self.thread_store .read_thread_by_rollout_path(StoreReadThreadByRolloutPathParams { @@ -4875,12 +4509,7 @@ impl CodexMessageProcessor { let existing_thread_id = match ThreadId::from_string(thread_id) { Ok(id) => id, Err(err) => { - self.send_invalid_request_error( - request_id, - format!("invalid thread id: {err}"), - ) - .await; - return None; + return Err(invalid_request(format!("invalid thread id: {err}"))); } }; let params = StoreReadThreadParams { @@ -4891,35 +4520,24 @@ impl CodexMessageProcessor { self.thread_store.read_thread(params).await }; - match result { - Ok(thread) => Some(thread), - Err(err) => { - self.outgoing - .send_error(request_id, thread_store_resume_read_error(err)) - .await; - None - } - } + result.map_err(thread_store_resume_read_error) } async fn stored_thread_to_initial_history( &self, - request_id: ConnectionRequestId, stored_thread: &StoredThread, - ) -> Option { + ) -> Result { let thread_id = stored_thread.thread_id; - let history = match stored_thread.history.as_ref() { - Some(history) => history.items.clone(), - None => { - self.send_internal_error( - request_id, - format!("thread {thread_id} did not include persisted history"), - ) - .await; - return None; - } - }; - Some(InitialHistory::Resumed(ResumedHistory { + let history = stored_thread + .history + .as_ref() + .map(|history| history.items.clone()) + .ok_or_else(|| { + internal_error(format!( + "thread {thread_id} did not include persisted history" + )) + })?; + Ok(InitialHistory::Resumed(ResumedHistory { conversation_id: thread_id, history, rollout_path: stored_thread.rollout_path.clone(), @@ -4947,27 +4565,18 @@ impl CodexMessageProcessor { async fn read_stored_thread_for_new_fork( &self, - request_id: ConnectionRequestId, thread_store: &dyn ThreadStore, thread_id: ThreadId, include_history: bool, - ) -> Option { - match thread_store + ) -> Result { + thread_store .read_thread(StoreReadThreadParams { thread_id, include_archived: true, include_history, }) .await - { - Ok(thread) => Some(thread), - Err(err) => { - self.outgoing - .send_error(request_id, thread_store_resume_read_error(err)) - .await; - None - } - } + .map_err(thread_store_resume_read_error) } async fn load_thread_from_resume_source_or_send_internal( @@ -5068,238 +4677,208 @@ impl CodexMessageProcessor { persist_extended_history, } = params; let include_turns = !exclude_turns; - if sandbox.is_some() && permission_profile.is_some() { - self.send_invalid_request_error( - request_id, - "`permissionProfile` cannot be combined with `sandbox`".to_string(), - ) - .await; - return; - } - - let Some(source_thread) = self - .read_stored_thread_for_resume( - request_id.clone(), - &thread_id, - path.as_ref(), - /*include_history*/ true, - ) - .await - else { - return; - }; - let source_thread_id = source_thread.thread_id; - let Some(history_items) = source_thread - .history - .as_ref() - .map(|history| history.items.clone()) - else { - self.send_internal_error( - request_id, - format!("thread {source_thread_id} did not include persisted history"), - ) - .await; - return; - }; - let history_cwd = Some(source_thread.cwd.clone()); - - // Persist Windows sandbox mode. - let mut cli_overrides = cli_overrides.unwrap_or_default(); - if cfg!(windows) { - match WindowsSandboxLevel::from_config(&self.config) { - WindowsSandboxLevel::Elevated => { - cli_overrides - .insert("windows.sandbox".to_string(), serde_json::json!("elevated")); - } - WindowsSandboxLevel::RestrictedToken => { - cli_overrides.insert( - "windows.sandbox".to_string(), - serde_json::json!("unelevated"), - ); - } - WindowsSandboxLevel::Disabled => {} + let result = async { + if sandbox.is_some() && permission_profile.is_some() { + return Err(invalid_request( + "`permissionProfile` cannot be combined with `sandbox`", + )); } - } - let request_overrides = if cli_overrides.is_empty() { - None - } else { - Some(cli_overrides) - }; - let mut typesafe_overrides = self.build_thread_config_overrides( - model, - model_provider, - service_tier, - cwd, - approval_policy, - approvals_reviewer, - sandbox, - permission_profile, - base_instructions, - developer_instructions, - /*personality*/ None, - ); - typesafe_overrides.ephemeral = ephemeral.then_some(true); - // Derive a Config using the same logic as new conversation, honoring overrides if provided. - let config = match self - .config_manager - .load_for_cwd(request_overrides, typesafe_overrides, history_cwd) - .await - { - Ok(config) => config, - Err(err) => { - self.outgoing - .send_error(request_id, config_load_error(&err)) - .await; - return; + + let source_thread = self + .read_stored_thread_for_resume( + &thread_id, + path.as_ref(), + /*include_history*/ true, + ) + .await?; + let source_thread_id = source_thread.thread_id; + let history_items = source_thread + .history + .as_ref() + .map(|history| history.items.clone()) + .ok_or_else(|| { + internal_error(format!( + "thread {source_thread_id} did not include persisted history" + )) + })?; + let history_cwd = Some(source_thread.cwd.clone()); + + // Persist Windows sandbox mode. + let mut cli_overrides = cli_overrides.unwrap_or_default(); + if cfg!(windows) { + match WindowsSandboxLevel::from_config(&self.config) { + WindowsSandboxLevel::Elevated => { + cli_overrides + .insert("windows.sandbox".to_string(), serde_json::json!("elevated")); + } + WindowsSandboxLevel::RestrictedToken => { + cli_overrides.insert( + "windows.sandbox".to_string(), + serde_json::json!("unelevated"), + ); + } + WindowsSandboxLevel::Disabled => {} + } } - }; + let request_overrides = if cli_overrides.is_empty() { + None + } else { + Some(cli_overrides) + }; + let mut typesafe_overrides = self.build_thread_config_overrides( + model, + model_provider, + service_tier, + cwd, + approval_policy, + approvals_reviewer, + sandbox, + permission_profile, + base_instructions, + developer_instructions, + /*personality*/ None, + ); + typesafe_overrides.ephemeral = ephemeral.then_some(true); + // Derive a Config using the same logic as new conversation, honoring overrides if provided. + let config = self + .config_manager + .load_for_cwd(request_overrides, typesafe_overrides, history_cwd) + .await + .map_err(|err| config_load_error(&err))?; - let fallback_model_provider = config.model_provider_id.clone(); - let instruction_sources = Self::instruction_sources_from_config(&config).await; - let fork_thread_store = configured_thread_store(&config); + let fallback_model_provider = config.model_provider_id.clone(); + let instruction_sources = Self::instruction_sources_from_config(&config).await; + let fork_thread_store = configured_thread_store(&config); - let NewThread { - thread_id, - thread: forked_thread, - session_configured, - .. - } = match self - .thread_manager - .fork_thread_from_history( - ForkSnapshot::Interrupted, - config, - InitialHistory::Resumed(ResumedHistory { - conversation_id: source_thread_id, - history: history_items.clone(), - rollout_path: source_thread.rollout_path.clone(), - }), - persist_extended_history, - self.request_trace_context(&request_id).await, - ) - .await - { - Ok(thread) => thread, - Err(err) => { - match err { + let NewThread { + thread_id, + thread: forked_thread, + session_configured, + .. + } = self + .thread_manager + .fork_thread_from_history( + ForkSnapshot::Interrupted, + config, + InitialHistory::Resumed(ResumedHistory { + conversation_id: source_thread_id, + history: history_items.clone(), + rollout_path: source_thread.rollout_path.clone(), + }), + persist_extended_history, + self.request_trace_context(&request_id).await, + ) + .await + .map_err(|err| match err { CodexErr::Io(_) | CodexErr::Json(_) => { - self.send_invalid_request_error( - request_id, - format!("failed to load thread {source_thread_id}: {err}"), - ) - .await; + invalid_request(format!("failed to load thread {source_thread_id}: {err}")) } - CodexErr::InvalidRequest(message) => { - self.send_invalid_request_error(request_id, message).await; - } - _ => { - self.send_internal_error( - request_id, - format!("error forking thread: {err}"), - ) - .await; - } - } - return; - } - }; + CodexErr::InvalidRequest(message) => invalid_request(message), + err => internal_error(format!("error forking thread: {err}")), + })?; - // Auto-attach a conversation listener when forking a thread. - Self::log_listener_attach_result( - self.ensure_conversation_listener( + // Auto-attach a conversation listener when forking a thread. + Self::log_listener_attach_result( + self.ensure_conversation_listener( + thread_id, + request_id.connection_id, + /*raw_events_enabled*/ false, + ApiVersion::V2, + ) + .await, thread_id, request_id.connection_id, - /*raw_events_enabled*/ false, - ApiVersion::V2, - ) - .await, - thread_id, - request_id.connection_id, - "thread", - ); + "thread", + ); - // Persistent forks materialize their own rollout immediately. Ephemeral forks stay - // pathless, so they rebuild their visible history from the copied source history instead. - let mut thread = if let Some(fork_rollout_path) = session_configured.rollout_path.as_ref() { - let Some(stored_thread) = self - .read_stored_thread_for_new_fork( - request_id.clone(), - fork_thread_store.as_ref(), - thread_id, - include_turns, - ) - .await - else { - return; - }; - match self - .stored_thread_to_api_thread( - stored_thread, - fallback_model_provider.as_str(), - include_turns, - ) - .await - { - Ok(thread) => thread, - Err(message) => { - self.send_internal_error( - request_id, - format!( + // Persistent forks materialize their own rollout immediately. Ephemeral forks stay + // pathless, so they rebuild their visible history from the copied source history instead. + let mut thread = + if let Some(fork_rollout_path) = session_configured.rollout_path.as_ref() { + let stored_thread = self + .read_stored_thread_for_new_fork( + fork_thread_store.as_ref(), + thread_id, + include_turns, + ) + .await?; + self.stored_thread_to_api_thread( + stored_thread, + fallback_model_provider.as_str(), + include_turns, + ) + .await + .map_err(|message| { + internal_error(format!( "failed to load rollout `{}` for thread {thread_id}: {message}", fork_rollout_path.display() - ), - ) - .await; - return; - } - } - } else { - let config_snapshot = forked_thread.config_snapshot().await; - // forked thread names do not inherit the source thread name - let mut thread = - build_thread_from_snapshot(thread_id, &config_snapshot, /*path*/ None); - thread.preview = preview_from_rollout_items(&history_items); - thread.forked_from_id = Some(source_thread_id.to_string()); - if include_turns - && let Err(message) = populate_thread_turns( - &mut thread, - ThreadTurnSource::HistoryItems(&history_items), - /*active_turn*/ None, - ) - .await - { - self.send_internal_error(request_id, message).await; + )) + })? + } else { + let config_snapshot = forked_thread.config_snapshot().await; + // forked thread names do not inherit the source thread name + let mut thread = + build_thread_from_snapshot(thread_id, &config_snapshot, /*path*/ None); + thread.preview = preview_from_rollout_items(&history_items); + thread.forked_from_id = Some(source_thread_id.to_string()); + if include_turns { + populate_thread_turns( + &mut thread, + ThreadTurnSource::HistoryItems(&history_items), + /*active_turn*/ None, + ) + .await + .map_err(internal_error)?; + } + thread + }; + + self.thread_watch_manager + .upsert_thread_silently(thread.clone()) + .await; + + thread.status = resolve_thread_status( + self.thread_watch_manager + .loaded_status_for_thread(&thread.id) + .await, + /*has_in_progress_turn*/ false, + ); + let permission_profile = thread_response_permission_profile( + forked_thread.config_snapshot().await.permission_profile, + ); + + let response = ThreadForkResponse { + thread: thread.clone(), + model: session_configured.model, + model_provider: session_configured.model_provider_id, + service_tier: session_configured.service_tier, + cwd: session_configured.cwd, + instruction_sources, + approval_policy: session_configured.approval_policy.into(), + approvals_reviewer: session_configured.approvals_reviewer.into(), + sandbox: session_configured.sandbox_policy.into(), + permission_profile, + reasoning_effort: session_configured.reasoning_effort, + }; + + Ok::<_, JSONRPCErrorError>(( + response, + thread_id, + forked_thread, + history_items, + thread_started_notification(thread), + )) + } + .await; + + let (response, thread_id, forked_thread, history_items, notif) = match result { + Ok(value) => value, + Err(error) => { + self.outgoing.send_error(request_id, error).await; return; } - thread }; - self.thread_watch_manager - .upsert_thread_silently(thread.clone()) - .await; - - thread.status = resolve_thread_status( - self.thread_watch_manager - .loaded_status_for_thread(&thread.id) - .await, - /*has_in_progress_turn*/ false, - ); - let permission_profile = thread_response_permission_profile( - forked_thread.config_snapshot().await.permission_profile, - ); - - let response = ThreadForkResponse { - thread: thread.clone(), - model: session_configured.model, - model_provider: session_configured.model_provider_id, - service_tier: session_configured.service_tier, - cwd: session_configured.cwd, - instruction_sources, - approval_policy: session_configured.approval_policy.into(), - approvals_reviewer: session_configured.approvals_reviewer.into(), - sandbox: session_configured.sandbox_policy.into(), - permission_profile, - reasoning_effort: session_configured.reasoning_effort, - }; self.analytics_events_client.track_response( request_id.connection_id.0, ClientResponse::ThreadFork { @@ -5337,7 +4916,6 @@ impl CodexMessageProcessor { .await; } - let notif = thread_started_notification(thread); self.outgoing .send_server_notification(ServerNotification::ThreadStarted(notif)) .await; @@ -5348,6 +4926,14 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: GetConversationSummaryParams, ) { + let result = self.get_thread_summary_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn get_thread_summary_response( + &self, + params: GetConversationSummaryParams, + ) -> Result { let fallback_provider = self.config.model_provider_id.as_str(); let read_result = match params { GetConversationSummaryParams::ThreadId { conversation_id } => self @@ -5365,13 +4951,9 @@ impl CodexMessageProcessor { .as_any() .downcast_ref::() else { - self.send_invalid_request_error( - request_id, - "rollout path queries are only supported with the local thread store" - .to_string(), - ) - .await; - return; + return Err(invalid_request( + "rollout path queries are only supported with the local thread store", + )); }; local_thread_store @@ -5385,27 +4967,14 @@ impl CodexMessageProcessor { } }; - match read_result { - Ok(stored_thread) => { - let Some(summary) = summary_from_stored_thread(stored_thread, fallback_provider) - else { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: - "failed to load conversation summary: thread is missing rollout path" - .to_string(), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; - }; - let response = GetConversationSummaryResponse { summary }; - self.outgoing.send_response(request_id, response).await; - } - Err(error) => { - self.outgoing.send_error(request_id, error).await; - } - } + let stored_thread = read_result?; + let summary = + summary_from_stored_thread(stored_thread, fallback_provider).ok_or_else(|| { + internal_error( + "failed to load conversation summary: thread is missing rollout path", + ) + })?; + Ok(GetConversationSummaryResponse { summary }) } async fn list_threads_common( @@ -5518,63 +5087,51 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ModelListParams, ) { - let ModelListParams { - limit, - cursor, - include_hidden, - } = params; - let models = supported_models(thread_manager, include_hidden.unwrap_or(false)).await; - let total = models.len(); + let result = async { + let ModelListParams { + limit, + cursor, + include_hidden, + } = params; + let models = supported_models(thread_manager, include_hidden.unwrap_or(false)).await; + let total = models.len(); - if total == 0 { - let response = ModelListResponse { - data: Vec::new(), - next_cursor: None, + if total == 0 { + return Ok(ModelListResponse { + data: Vec::new(), + next_cursor: None, + }); + } + + let effective_limit = limit.unwrap_or(total as u32).max(1) as usize; + let effective_limit = effective_limit.min(total); + let start = match cursor { + Some(cursor) => cursor + .parse::() + .map_err(|_| invalid_request(format!("invalid cursor: {cursor}")))?, + None => 0, }; - outgoing.send_response(request_id, response).await; - return; - } - let effective_limit = limit.unwrap_or(total as u32).max(1) as usize; - let effective_limit = effective_limit.min(total); - let start = match cursor { - Some(cursor) => match cursor.parse::() { - Ok(idx) => idx, - Err(_) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid cursor: {cursor}"), - data: None, - }; - outgoing.send_error(request_id, error).await; - return; - } - }, - None => 0, - }; + if start > total { + return Err(invalid_request(format!( + "cursor {start} exceeds total models {total}" + ))); + } - if start > total { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("cursor {start} exceeds total models {total}"), - data: None, + let end = start.saturating_add(effective_limit).min(total); + let items = models[start..end].to_vec(); + let next_cursor = if end < total { + Some(end.to_string()) + } else { + None }; - outgoing.send_error(request_id, error).await; - return; + Ok::<_, JSONRPCErrorError>(ModelListResponse { + data: items, + next_cursor, + }) } - - let end = start.saturating_add(effective_limit).min(total); - let items = models[start..end].to_vec(); - let next_cursor = if end < total { - Some(end.to_string()) - } else { - None - }; - let response = ModelListResponse { - data: items, - next_cursor, - }; - outgoing.send_response(request_id, response).await; + .await; + outgoing.send_result(request_id, result).await; } async fn list_collaboration_modes( @@ -5598,14 +5155,16 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ExperimentalFeatureListParams, ) { + let result = self.experimental_feature_list_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn experimental_feature_list_response( + &self, + params: ExperimentalFeatureListParams, + ) -> Result { let ExperimentalFeatureListParams { cursor, limit } = params; - let config = match self.load_latest_config(/*fallback_cwd*/ None).await { - Ok(config) => config, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; + let config = self.load_latest_config(/*fallback_cwd*/ None).await?; let auth = self.auth_manager.auth().await; let workspace_codex_plugins_enabled = self .workspace_codex_plugins_enabled(&config, auth.as_ref()) @@ -5654,16 +5213,10 @@ impl CodexMessageProcessor { let total = data.len(); if total == 0 { - self.outgoing - .send_response( - request_id, - ExperimentalFeatureListResponse { - data: Vec::new(), - next_cursor: None, - }, - ) - .await; - return; + return Ok(ExperimentalFeatureListResponse { + data: Vec::new(), + next_cursor: None, + }); } // Clamp to 1 so limit=0 cannot return a non-advancing page. @@ -5672,25 +5225,15 @@ impl CodexMessageProcessor { let start = match cursor { Some(cursor) => match cursor.parse::() { Ok(idx) => idx, - Err(_) => { - self.send_invalid_request_error( - request_id, - format!("invalid cursor: {cursor}"), - ) - .await; - return; - } + Err(_) => return Err(invalid_request(format!("invalid cursor: {cursor}"))), }, None => 0, }; if start > total { - self.send_invalid_request_error( - request_id, - format!("cursor {start} exceeds total feature flags {total}"), - ) - .await; - return; + return Err(invalid_request(format!( + "cursor {start} exceeds total feature flags {total}" + ))); } let end = start.saturating_add(effective_limit).min(total); @@ -5701,12 +5244,7 @@ impl CodexMessageProcessor { None }; - self.outgoing - .send_response( - request_id, - ExperimentalFeatureListResponse { data, next_cursor }, - ) - .await; + Ok(ExperimentalFeatureListResponse { data, next_cursor }) } async fn mock_experimental_method( @@ -5720,21 +5258,13 @@ impl CodexMessageProcessor { } async fn mcp_server_refresh(&self, request_id: ConnectionRequestId, _params: Option<()>) { - let config = match self.load_latest_config(/*fallback_cwd*/ None).await { - Ok(config) => config, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; - - if let Err(error) = self.queue_mcp_server_refresh_for_config(&config).await { - self.outgoing.send_error(request_id, error).await; - return; + let result = async { + let config = self.load_latest_config(/*fallback_cwd*/ None).await?; + self.queue_mcp_server_refresh_for_config(&config).await?; + Ok::<_, JSONRPCErrorError>(McpServerRefreshResponse {}) } - - let response = McpServerRefreshResponse {}; - self.outgoing.send_response(request_id, response).await; + .await; + self.outgoing.send_result(request_id, result).await; } async fn queue_mcp_server_refresh_for_config( @@ -5788,14 +5318,15 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: McpServerOauthLoginParams, ) { - let config = match self.load_latest_config(/*fallback_cwd*/ None).await { - Ok(config) => config, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; + let result = self.mcp_server_oauth_login_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + async fn mcp_server_oauth_login_response( + &self, + params: McpServerOauthLoginParams, + ) -> Result { + let config = self.load_latest_config(/*fallback_cwd*/ None).await?; let McpServerOauthLoginParams { name, scopes, @@ -5808,13 +5339,9 @@ impl CodexMessageProcessor { .configured_servers(&config) .await; let Some(server) = configured_servers.get(&name) else { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("No MCP server named '{name}' found."), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; + return Err(invalid_request(format!( + "No MCP server named '{name}' found." + ))); }; let (url, http_headers, env_http_headers) = match &server.transport { @@ -5825,14 +5352,9 @@ impl CodexMessageProcessor { .. } => (url.clone(), http_headers.clone(), env_http_headers.clone()), _ => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: "OAuth login is only supported for streamable HTTP servers." - .to_string(), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; + return Err(invalid_request( + "OAuth login is only supported for streamable HTTP servers.", + )); } }; @@ -5844,7 +5366,7 @@ impl CodexMessageProcessor { let resolved_scopes = resolve_oauth_scopes(scopes, server.scopes.clone(), discovered_scopes); - match perform_oauth_login_return_url( + let handle = perform_oauth_login_return_url( &name, &url, config.mcp_oauth_credentials_store_mode, @@ -5857,40 +5379,28 @@ impl CodexMessageProcessor { config.mcp_oauth_callback_url.as_deref(), ) .await - { - Ok(handle) => { - let authorization_url = handle.authorization_url().to_string(); - let notification_name = name.clone(); - let outgoing = Arc::clone(&self.outgoing); + .map_err(|err| internal_error(format!("failed to login to MCP server '{name}': {err}")))?; + let authorization_url = handle.authorization_url().to_string(); + let notification_name = name.clone(); + let outgoing = Arc::clone(&self.outgoing); - tokio::spawn(async move { - let (success, error) = match handle.wait().await { - Ok(()) => (true, None), - Err(err) => (false, Some(err.to_string())), - }; + tokio::spawn(async move { + let (success, error) = match handle.wait().await { + Ok(()) => (true, None), + Err(err) => (false, Some(err.to_string())), + }; - let notification = ServerNotification::McpServerOauthLoginCompleted( - McpServerOauthLoginCompletedNotification { - name: notification_name, - success, - error, - }, - ); - outgoing.send_server_notification(notification).await; - }); + let notification = ServerNotification::McpServerOauthLoginCompleted( + McpServerOauthLoginCompletedNotification { + name: notification_name, + success, + error, + }, + ); + outgoing.send_server_notification(notification).await; + }); - let response = McpServerOauthLoginResponse { authorization_url }; - self.outgoing.send_response(request_id, response).await; - } - Err(err) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("failed to login to MCP server '{name}': {err}"), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - } - } + Ok(McpServerOauthLoginResponse { authorization_url }) } async fn list_mcp_server_status( @@ -5948,6 +5458,26 @@ impl CodexMessageProcessor { auth: Option, runtime_environment: McpRuntimeEnvironment, ) { + let result = Self::list_mcp_server_status_response( + request_id.request_id.to_string(), + params, + config, + mcp_config, + auth, + runtime_environment, + ) + .await; + outgoing.send_result(request_id, result).await; + } + + async fn list_mcp_server_status_response( + request_id: String, + params: ListMcpServerStatusParams, + config: Config, + mcp_config: codex_mcp::McpConfig, + auth: Option, + runtime_environment: McpRuntimeEnvironment, + ) -> Result { let detail = match params.detail.unwrap_or(McpServerStatusDetail::Full) { McpServerStatusDetail::Full => McpSnapshotDetail::Full, McpServerStatusDetail::ToolsAndAuthOnly => McpSnapshotDetail::ToolsAndAuthOnly, @@ -5956,7 +5486,7 @@ impl CodexMessageProcessor { let snapshot = collect_mcp_server_status_snapshot_with_detail( &mcp_config, auth.as_ref(), - request_id.request_id.to_string(), + request_id, runtime_environment, detail, ) @@ -5991,27 +5521,15 @@ impl CodexMessageProcessor { let start = match params.cursor { Some(cursor) => match cursor.parse::() { Ok(idx) => idx, - Err(_) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid cursor: {cursor}"), - data: None, - }; - outgoing.send_error(request_id, error).await; - return; - } + Err(_) => return Err(invalid_request(format!("invalid cursor: {cursor}"))), }, None => 0, }; if start > total { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("cursor {start} exceeds total MCP servers {total}"), - data: None, - }; - outgoing.send_error(request_id, error).await; - return; + return Err(invalid_request(format!( + "cursor {start} exceeds total MCP servers {total}" + ))); } let end = start.saturating_add(effective_limit).min(total); @@ -6037,9 +5555,7 @@ impl CodexMessageProcessor { None }; - let response = ListMcpServerStatusResponse { data, next_cursor }; - - outgoing.send_response(request_id, response).await; + Ok(ListMcpServerStatusResponse { data, next_cursor }) } async fn read_mcp_resource( @@ -6113,39 +5629,16 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, result: anyhow::Result, ) { - match result { - Ok(result) => match serde_json::from_value::(result) { - Ok(response) => { - outgoing.send_response(request_id, response).await; - } - Err(error) => { - outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!( - "failed to deserialize MCP resource read response: {error}" - ), - data: None, - }, - ) - .await; - } - }, - Err(error) => { - outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("{error:#}"), - data: None, - }, - ) - .await; - } - } + let result = result + .map_err(|error| internal_error(format!("{error:#}"))) + .and_then(|result| { + serde_json::from_value::(result).map_err(|error| { + internal_error(format!( + "failed to deserialize MCP resource read response: {error}" + )) + }) + }); + outgoing.send_result(request_id, result).await; } async fn call_mcp_server_tool( @@ -6167,26 +5660,10 @@ impl CodexMessageProcessor { tokio::spawn(async move { let result = thread .call_mcp_tool(¶ms.server, ¶ms.tool, params.arguments, meta) - .await; - match result { - Ok(result) => { - outgoing - .send_response(request_id, McpServerToolCallResponse::from(result)) - .await; - } - Err(error) => { - outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("{error:#}"), - data: None, - }, - ) - .await; - } - } + .await + .map(McpServerToolCallResponse::from) + .map_err(|error| internal_error(format!("{error:#}"))); + outgoing.send_result(request_id, result).await; }); } @@ -6206,18 +5683,18 @@ impl CodexMessageProcessor { } } - async fn send_invalid_request_error(&self, request_id: ConnectionRequestId, message: String) { + async fn send_internal_error(&self, request_id: ConnectionRequestId, message: String) { let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, + code: INTERNAL_ERROR_CODE, message, data: None, }; self.outgoing.send_error(request_id, error).await; } - async fn send_internal_error(&self, request_id: ConnectionRequestId, message: String) { + async fn send_invalid_request_error(&self, request_id: ConnectionRequestId, message: String) { let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, + code: INVALID_REQUEST_ERROR_CODE, message, data: None, }; @@ -8709,14 +8186,7 @@ async fn handle_pending_thread_resume_request( .await { outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message, - data: None, - }, - ) + .send_error(request_id, internal_error(message)) .await; return; } @@ -8738,13 +8208,9 @@ async fn handle_pending_thread_resume_request( outgoing .send_error( request_id, - JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!( - "thread {conversation_id} is closing; retry thread/resume after the thread is closed" - ), - data: None, - }, + invalid_request(format!( + "thread {conversation_id} is closing; retry thread/resume after the thread is closed" + )), ) .await; return; 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 83718a8dc7..211cec9355 100644 --- a/codex-rs/app-server/tests/suite/v2/command_exec.rs +++ b/codex-rs/app-server/tests/suite/v2/command_exec.rs @@ -246,7 +246,7 @@ async fn command_exec_accepts_permission_profile() -> Result<()> { #[cfg(unix)] #[tokio::test] -async fn command_exec_permission_profile_cwd_uses_command_cwd() -> Result<()> { +async fn command_exec_permission_profile_project_roots_use_command_cwd() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; let codex_home = TempDir::new()?; let command_dir = codex_home.path().join("command-cwd"); @@ -264,7 +264,7 @@ async fn command_exec_permission_profile_cwd_uses_command_cwd() -> Result<()> { }; entries.push(FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::ProjectRoots { subpath: None }, }, access: FileSystemAccessMode::Write, }); @@ -298,7 +298,7 @@ async fn command_exec_permission_profile_cwd_uses_command_cwd() -> Result<()> { let response: CommandExecResponse = to_response(response)?; assert_eq!( response.exit_code, 0, - "parent cwd write should fail under command-cwd-scoped profile: {response:?}" + "parent cwd write should fail under command project-root profile: {response:?}" ); assert_eq!( std::fs::read_to_string(command_dir.join("child.txt"))?, @@ -306,7 +306,7 @@ async fn command_exec_permission_profile_cwd_uses_command_cwd() -> Result<()> { ); assert!( !codex_home.path().join("parent.txt").exists(), - "permissionProfile :cwd write should not grant the server cwd when command cwd differs" + "permissionProfile :project_roots write should not grant the server cwd when command cwd differs" ); Ok(()) diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index c49e28f20a..3997018eaa 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -98,6 +98,7 @@ use tokio::sync::oneshot; use tokio::sync::oneshot::error::TryRecvError; use tokio_tungstenite::tungstenite::Error; use tokio_tungstenite::tungstenite::Message; +use tokio_util::sync::CancellationToken; use tracing::instrument; use tracing::trace; use tracing::warn; @@ -1232,7 +1233,8 @@ impl ModelClientSession { Err(ApiError::Transport( unauthorized_transport @ TransportError::Http { status, .. }, )) if status == StatusCode::UNAUTHORIZED => { - inference_trace_attempt.record_failed(&unauthorized_transport); + inference_trace_attempt + .record_failed(&unauthorized_transport, /*output_items*/ &[]); pending_retry = PendingUnauthorizedRetry::from_recovery( handle_unauthorized( unauthorized_transport, @@ -1245,7 +1247,7 @@ impl ModelClientSession { } Err(err) => { let err = map_api_error(err); - inference_trace_attempt.record_failed(&err); + inference_trace_attempt.record_failed(&err, /*output_items*/ &[]); return Err(err); } } @@ -1372,7 +1374,7 @@ impl ModelClientSession { .await .map_err(|err| { let err = map_api_error(err); - inference_trace_attempt.record_failed(&err); + inference_trace_attempt.record_failed(&err, /*output_items*/ &[]); err })?; let (stream, last_request_rx) = map_response_stream( @@ -1621,6 +1623,9 @@ fn parent_thread_id_header_value(session_source: &SessionSource) -> Option( api_stream: S, session_telemetry: SessionTelemetry, @@ -1632,15 +1637,28 @@ where + Send + 'static, { - let (tx_event, rx_event) = mpsc::channel::>(1600); + let (tx_event, rx_event) = + mpsc::channel::>(RESPONSE_STREAM_CHANNEL_CAPACITY); let (tx_last_response, rx_last_response) = oneshot::channel::(); + let consumer_dropped = CancellationToken::new(); + let consumer_dropped_for_stream = consumer_dropped.clone(); tokio::spawn(async move { let mut logged_error = false; let mut tx_last_response = Some(tx_last_response); let mut items_added: Vec = Vec::new(); let mut api_stream = api_stream; - while let Some(event) = api_stream.next().await { + loop { + let event = tokio::select! { + _ = consumer_dropped.cancelled() => { + inference_trace_attempt.record_cancelled(STREAM_DROPPED_REASON, &items_added); + return; + } + event = api_stream.next() => event, + }; + let Some(event) = event else { + break; + }; match event { Ok(ResponseEvent::OutputItemDone(item)) => { items_added.push(item.clone()); @@ -1649,6 +1667,8 @@ where .await .is_err() { + inference_trace_attempt + .record_cancelled(STREAM_DROPPED_REASON, &items_added); return; } } @@ -1691,12 +1711,14 @@ where } Ok(event) => { if tx_event.send(Ok(event)).await.is_err() { + inference_trace_attempt + .record_cancelled(STREAM_DROPPED_REASON, &items_added); return; } } Err(err) => { let mapped = map_api_error(err); - inference_trace_attempt.record_failed(&mapped); + inference_trace_attempt.record_failed(&mapped, &items_added); if !logged_error { session_telemetry.see_event_completed_failed(&mapped); logged_error = true; @@ -1707,9 +1729,17 @@ where } } } + inference_trace_attempt + .record_failed("stream closed before response.completed", &items_added); }); - (ResponseStream { rx_event }, rx_last_response) + ( + ResponseStream { + rx_event, + consumer_dropped: consumer_dropped_for_stream, + }, + rx_last_response, + ) } /// Handles a 401 response by optionally refreshing ChatGPT tokens once. diff --git a/codex-rs/core/src/client_common.rs b/codex-rs/core/src/client_common.rs index e8e3754003..efe2670652 100644 --- a/codex-rs/core/src/client_common.rs +++ b/codex-rs/core/src/client_common.rs @@ -13,6 +13,7 @@ use std::pin::Pin; use std::task::Context; use std::task::Poll; use tokio::sync::mpsc; +use tokio_util::sync::CancellationToken; /// Review thread system prompt. Edit `core/src/review_prompt.md` to customize. pub const REVIEW_PROMPT: &str = include_str!("../review_prompt.md"); @@ -175,6 +176,9 @@ fn strip_total_output_header(output: &str) -> Option<(&str, u32)> { pub struct ResponseStream { pub(crate) rx_event: mpsc::Receiver>, + /// Signals the mapper task that the consumer stopped polling before the + /// provider stream reached its own terminal event. + pub(crate) consumer_dropped: CancellationToken, } impl Stream for ResponseStream { @@ -185,6 +189,12 @@ impl Stream for ResponseStream { } } +impl Drop for ResponseStream { + fn drop(&mut self) { + self.consumer_dropped.cancel(); + } +} + #[cfg(test)] #[path = "client_common_tests.rs"] mod tests; diff --git a/codex-rs/core/src/client_tests.rs b/codex-rs/core/src/client_tests.rs index f4575b26a0..e2934dc1cc 100644 --- a/codex-rs/core/src/client_tests.rs +++ b/codex-rs/core/src/client_tests.rs @@ -7,17 +7,37 @@ use super::X_CODEX_PARENT_THREAD_ID_HEADER; use super::X_CODEX_TURN_METADATA_HEADER; use super::X_CODEX_WINDOW_ID_HEADER; use super::X_OPENAI_SUBAGENT_HEADER; +use codex_api::ApiError; +use codex_api::ResponseEvent; use codex_app_server_protocol::AuthMode; use codex_model_provider::BearerAuthProvider; use codex_model_provider_info::WireApi; use codex_model_provider_info::create_oss_provider_with_base_url; use codex_otel::SessionTelemetry; use codex_protocol::ThreadId; +use codex_protocol::models::ContentItem; +use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::ModelInfo; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; +use codex_rollout_trace::ExecutionStatus; +use codex_rollout_trace::InferenceTraceAttempt; +use codex_rollout_trace::InferenceTraceContext; +use codex_rollout_trace::RawTraceEventPayload; +use codex_rollout_trace::RolloutTrace; +use codex_rollout_trace::TraceWriter; +use codex_rollout_trace::replay_bundle; +use futures::StreamExt; use pretty_assertions::assert_eq; use serde_json::json; +use std::collections::VecDeque; +use std::pin::Pin; +use std::sync::Arc; +use std::task::Context; +use std::task::Poll; +use std::time::Duration; +use tempfile::TempDir; +use tokio::sync::Notify; fn test_model_client(session_source: SessionSource) -> ModelClient { let provider = create_oss_provider_with_base_url("https://example.com/v1", WireApi::Responses); @@ -79,6 +99,92 @@ fn test_session_telemetry() -> SessionTelemetry { ) } +fn started_inference_attempt(temp: &TempDir) -> anyhow::Result { + let writer = Arc::new(TraceWriter::create( + temp.path(), + "trace-1".to_string(), + "rollout-1".to_string(), + "thread-root".to_string(), + )?); + writer.append(RawTraceEventPayload::ThreadStarted { + thread_id: "thread-root".to_string(), + agent_path: "/root".to_string(), + metadata_payload: None, + })?; + writer.append(RawTraceEventPayload::CodexTurnStarted { + codex_turn_id: "turn-1".to_string(), + thread_id: "thread-root".to_string(), + })?; + + let inference_trace = InferenceTraceContext::enabled( + writer, + "thread-root".to_string(), + "turn-1".to_string(), + "gpt-test".to_string(), + "test-provider".to_string(), + ); + let attempt = inference_trace.start_attempt(); + attempt.record_started(&json!({ + "model": "gpt-test", + "input": [{ + "type": "message", + "role": "user", + "content": [{"type": "input_text", "text": "hello"}] + }], + })); + Ok(attempt) +} + +fn output_message(id: &str, text: &str) -> ResponseItem { + ResponseItem::Message { + id: Some(id.to_string()), + role: "assistant".to_string(), + content: vec![ContentItem::OutputText { + text: text.to_string(), + }], + phase: None, + } +} + +async fn replay_until_cancelled(temp: &TempDir) -> anyhow::Result { + let mut rollout = replay_bundle(temp.path())?; + for _ in 0..50 { + let inference = rollout + .inference_calls + .values() + .next() + .expect("inference should be reduced"); + if inference.execution.status == ExecutionStatus::Cancelled { + return Ok(rollout); + } + tokio::time::sleep(Duration::from_millis(10)).await; + rollout = replay_bundle(temp.path())?; + } + Ok(rollout) +} + +struct NotifyAfterEventStream { + events: VecDeque, + yielded: usize, + notify_after: usize, + notify: Arc, +} + +impl futures::Stream for NotifyAfterEventStream { + type Item = std::result::Result; + + fn poll_next(mut self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + let Some(event) = self.events.pop_front() else { + return Poll::Pending; + }; + self.yielded += 1; + if self.yielded == self.notify_after { + self.notify.notify_one(); + } + Poll::Ready(Some(Ok(event))) + } +} + #[test] fn build_subagent_headers_sets_other_subagent_label() { let client = test_model_client(SessionSource::SubAgent(SubAgentSource::Other( @@ -151,6 +257,91 @@ async fn summarize_memories_returns_empty_for_empty_input() { assert_eq!(output.len(), 0); } +#[tokio::test] +async fn dropped_response_stream_traces_cancelled_partial_output() -> anyhow::Result<()> { + let temp = TempDir::new()?; + let attempt = started_inference_attempt(&temp)?; + + // The provider has produced one complete output item, but no terminal + // response.completed event. The harness has enough information to keep this + // item in history, so the trace should preserve it when the stream is + // abandoned. + let item = output_message("msg-1", "partial answer"); + let api_stream = futures::stream::iter([Ok(ResponseEvent::OutputItemDone(item))]) + .chain(futures::stream::pending()); + let (mut stream, _) = super::map_response_stream(api_stream, test_session_telemetry(), attempt); + + let observed = stream + .next() + .await + .expect("mapped stream should yield output item")?; + assert!(matches!(observed, ResponseEvent::OutputItemDone(_))); + + // Dropping the consumer is how turn interruption/preemption stops polling + // the provider stream. The mapper task observes that drop asynchronously + // and records cancellation using the output items it has already seen. + drop(stream); + + // Cancellation is recorded by the mapper task after Drop wakes it, so the + // replay may need a short wait before the terminal event appears on disk. + let rollout = replay_until_cancelled(&temp).await?; + let inference = rollout + .inference_calls + .values() + .next() + .expect("inference should be reduced"); + + assert_eq!(inference.execution.status, ExecutionStatus::Cancelled); + assert_eq!(inference.response_item_ids.len(), 1); + assert_eq!(rollout.raw_payloads.len(), 2); + + Ok(()) +} + +#[tokio::test] +async fn dropped_backpressured_response_stream_traces_cancelled_partial_output() +-> anyhow::Result<()> { + let temp = TempDir::new()?; + let attempt = started_inference_attempt(&temp)?; + let backpressured_item_yielded = Arc::new(Notify::new()); + let mut events = VecDeque::new(); + for _ in 0..super::RESPONSE_STREAM_CHANNEL_CAPACITY { + events.push_back(ResponseEvent::Created); + } + events.push_back(ResponseEvent::OutputItemDone(output_message( + "msg-1", + "partial answer", + ))); + let api_stream = NotifyAfterEventStream { + events, + yielded: 0, + notify_after: super::RESPONSE_STREAM_CHANNEL_CAPACITY + 1, + notify: Arc::clone(&backpressured_item_yielded), + }; + + let (stream, _) = super::map_response_stream(api_stream, test_session_telemetry(), attempt); + + // Fill the mapper channel with non-terminal events, then yield one output + // item. The mapper has observed that item and is blocked trying to send it + // downstream, so dropping the consumer covers the send-failure path rather + // than the `consumer_dropped` select branch. + backpressured_item_yielded.notified().await; + drop(stream); + + let rollout = replay_until_cancelled(&temp).await?; + let inference = rollout + .inference_calls + .values() + .next() + .expect("inference should be reduced"); + + assert_eq!(inference.execution.status, ExecutionStatus::Cancelled); + assert_eq!(inference.response_item_ids.len(), 1); + assert_eq!(rollout.raw_payloads.len(), 2); + + Ok(()) +} + #[test] fn auth_request_telemetry_context_tracks_attached_auth_and_retry_phase() { let auth_context = AuthRequestTelemetryContext::new( diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 995e4299c6..e25c83e58d 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -947,7 +947,7 @@ async fn permission_profile_override_applies_runtime_roots_to_legacy_projection( }, FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index fb90ef322f..3692e31ee7 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -1004,7 +1004,7 @@ fn managed_cwd_write_profile_is_not_read_only() { }, FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, diff --git a/codex-rs/core/src/exec_tests.rs b/codex-rs/core/src/exec_tests.rs index 4e8ba10c20..a9304e75ee 100644 --- a/codex-rs/core/src/exec_tests.rs +++ b/codex-rs/core/src/exec_tests.rs @@ -535,7 +535,9 @@ fn windows_restricted_token_rejects_split_only_filesystem_policies() { let file_system_policy = FileSystemSandboxPolicy::restricted(vec![ codex_protocol::permissions::FileSystemSandboxEntry { path: codex_protocol::permissions::FileSystemPath::Special { - value: codex_protocol::permissions::FileSystemSpecialPath::CurrentWorkingDirectory, + value: codex_protocol::permissions::FileSystemSpecialPath::project_roots( + /*subpath*/ None, + ), }, access: codex_protocol::permissions::FileSystemAccessMode::Write, }, @@ -630,7 +632,9 @@ fn windows_restricted_token_supports_full_read_split_write_read_carveouts() { }, codex_protocol::permissions::FileSystemSandboxEntry { path: codex_protocol::permissions::FileSystemPath::Special { - value: codex_protocol::permissions::FileSystemSpecialPath::CurrentWorkingDirectory, + value: codex_protocol::permissions::FileSystemSpecialPath::project_roots( + /*subpath*/ None, + ), }, access: codex_protocol::permissions::FileSystemAccessMode::Write, }, @@ -720,7 +724,9 @@ fn windows_elevated_supports_split_write_read_carveouts() { }, codex_protocol::permissions::FileSystemSandboxEntry { path: codex_protocol::permissions::FileSystemPath::Special { - value: codex_protocol::permissions::FileSystemSpecialPath::CurrentWorkingDirectory, + value: codex_protocol::permissions::FileSystemSpecialPath::project_roots( + /*subpath*/ None, + ), }, access: codex_protocol::permissions::FileSystemAccessMode::Write, }, @@ -774,7 +780,9 @@ fn windows_elevated_rejects_unreadable_split_carveouts() { }, codex_protocol::permissions::FileSystemSandboxEntry { path: codex_protocol::permissions::FileSystemPath::Special { - value: codex_protocol::permissions::FileSystemSpecialPath::CurrentWorkingDirectory, + value: codex_protocol::permissions::FileSystemSpecialPath::project_roots( + /*subpath*/ None, + ), }, access: codex_protocol::permissions::FileSystemAccessMode::Write, }, @@ -821,7 +829,9 @@ fn windows_elevated_rejects_unreadable_globs() { }, codex_protocol::permissions::FileSystemSandboxEntry { path: codex_protocol::permissions::FileSystemPath::Special { - value: codex_protocol::permissions::FileSystemSpecialPath::CurrentWorkingDirectory, + value: codex_protocol::permissions::FileSystemSpecialPath::project_roots( + /*subpath*/ None, + ), }, access: codex_protocol::permissions::FileSystemAccessMode::Write, }, @@ -870,7 +880,9 @@ fn windows_elevated_rejects_reopened_writable_descendants() { }, codex_protocol::permissions::FileSystemSandboxEntry { path: codex_protocol::permissions::FileSystemPath::Special { - value: codex_protocol::permissions::FileSystemSpecialPath::CurrentWorkingDirectory, + value: codex_protocol::permissions::FileSystemSpecialPath::project_roots( + /*subpath*/ None, + ), }, access: codex_protocol::permissions::FileSystemAccessMode::Write, }, diff --git a/codex-rs/core/src/safety_tests.rs b/codex-rs/core/src/safety_tests.rs index 0ca10e66e6..d699172498 100644 --- a/codex-rs/core/src/safety_tests.rs +++ b/codex-rs/core/src/safety_tests.rs @@ -258,7 +258,7 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index dcadac70a8..d4c0d337cf 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -149,19 +149,19 @@ impl SessionConfiguration { ); let file_system_policy_matches_legacy = current_file_system_sandbox_policy .is_semantically_equivalent_to(&legacy_file_system_projection, &self.cwd); - let file_system_policy_has_rebindable_cwd_write = current_file_system_sandbox_policy - .entries - .iter() - .any(|entry| { - entry.access.can_write() - && matches!( - &entry.path, - FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory - | FileSystemSpecialPath::ProjectRoots { subpath: None }, - } - ) - }); + let file_system_policy_has_rebindable_project_root_write = + current_file_system_sandbox_policy + .entries + .iter() + .any(|entry| { + entry.access.can_write() + && matches!( + &entry.path, + FileSystemPath::Special { + value: FileSystemSpecialPath::ProjectRoots { subpath: None }, + } + ) + }); if let Some(collaboration_mode) = updates.collaboration_mode.clone() { next_configuration.collaboration_mode = collaboration_mode; } @@ -228,7 +228,7 @@ impl SessionConfiguration { )?; } else if cwd_changed && file_system_policy_matches_legacy - && file_system_policy_has_rebindable_cwd_write + && file_system_policy_has_rebindable_project_root_write { // Preserve richer split policies across cwd-only updates; only // rederive when the session is already using a structurally diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 9515755323..283220e8fa 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -2874,7 +2874,7 @@ async fn session_configuration_apply_preserves_profile_file_system_policy_on_cwd let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -3187,7 +3187,7 @@ async fn session_configuration_apply_preserves_absolute_cwd_write_root_on_cwd_up !updated .file_system_sandbox_policy() .can_write_path_with_cwd(next_cwd.as_path(), updated.cwd.as_path()), - "cwd-only update must not reinterpret an absolute old-cwd grant as :cwd" + "cwd-only update must not reinterpret an absolute old-cwd grant as :project_roots" ); } @@ -3850,7 +3850,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec file_system: Some(FileSystemPermissions { entries: vec![FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }], diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index f96b49ad42..0ddd1e5062 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -354,7 +354,7 @@ mod tests { entries: vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, diff --git a/codex-rs/exec-server/src/file_system.rs b/codex-rs/exec-server/src/file_system.rs index cd31ae63c5..fde69d6421 100644 --- a/codex-rs/exec-server/src/file_system.rs +++ b/codex-rs/exec-server/src/file_system.rs @@ -117,9 +117,7 @@ pub(crate) fn file_system_policy_has_cwd_dependent_entries( .any(|entry| match &entry.path { FileSystemPath::GlobPattern { pattern } => !Path::new(pattern).is_absolute(), FileSystemPath::Special { - value: - FileSystemSpecialPath::CurrentWorkingDirectory - | FileSystemSpecialPath::ProjectRoots { .. }, + value: FileSystemSpecialPath::ProjectRoots { .. }, } => true, FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => false, }) diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index be3ea457bc..32cbba9572 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -118,7 +118,7 @@ fn sandbox_cwd(sandbox: &FileSystemSandboxContext) -> Result Self { Self::Managed { file_system: ManagedFileSystemPermissions::Restricted { @@ -415,7 +419,7 @@ impl PermissionProfile { }, FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 63c2e3c4f7..7d389a9519 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -80,7 +80,7 @@ impl FileSystemAccessMode { pub enum FileSystemSpecialPath { Root, Minimal, - CurrentWorkingDirectory, + #[serde(alias = "current_working_directory")] ProjectRoots { #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional)] @@ -414,9 +414,9 @@ impl FileSystemSandboxPolicy { /// Converts a legacy sandbox policy into a cwd-independent filesystem policy. /// - /// `WorkspaceWrite` uses symbolic entries for cwd-scoped access so callers - /// can preserve the active cwd binding until the policy is actually - /// resolved for a turn or command. + /// `WorkspaceWrite` uses symbolic project-root entries so callers can keep + /// the profile independent of the concrete root until it is resolved for a + /// turn or command. pub fn from_legacy_sandbox_policy(sandbox_policy: &SandboxPolicy) -> Self { let mut file_system_policy = Self::from(sandbox_policy); let SandboxPolicy::WorkspaceWrite { @@ -571,6 +571,28 @@ impl FileSystemSandboxPolicy { self.resolve_access_with_cwd(path, cwd).can_write() } + /// Replaces symbolic `:project_roots` entries with absolute paths resolved + /// against `cwd`. + /// + /// Use this when a durable permission profile must survive a cwd-only + /// update without rebinding its project-root authority to the new cwd. + pub fn materialize_project_roots_with_cwd(mut self, cwd: &Path) -> Self { + let cwd = AbsolutePathBuf::from_absolute_path(cwd).ok(); + for entry in &mut self.entries { + let FileSystemPath::Special { + value: FileSystemSpecialPath::ProjectRoots { .. }, + } = &entry.path + else { + continue; + }; + + if let Some(path) = resolve_file_system_path(&entry.path, cwd.as_ref()) { + entry.path = FileSystemPath::Path { path }; + } + } + self + } + pub fn with_additional_readable_roots( mut self, cwd: &Path, @@ -867,11 +889,6 @@ impl FileSystemSandboxPolicy { } }, FileSystemSpecialPath::Minimal => {} - FileSystemSpecialPath::CurrentWorkingDirectory => { - if entry.access.can_write() { - workspace_root_writable = true; - } - } FileSystemSpecialPath::ProjectRoots { subpath } => { if subpath.is_none() && entry.access.can_write() { workspace_root_writable = true; @@ -1001,7 +1018,7 @@ impl From<&SandboxPolicy> for FileSystemSandboxPolicy { entries.push(FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }); @@ -1099,20 +1116,8 @@ fn special_paths_share_target(left: &FileSystemSpecialPath, right: &FileSystemSp match (left, right) { (FileSystemSpecialPath::Root, FileSystemSpecialPath::Root) | (FileSystemSpecialPath::Minimal, FileSystemSpecialPath::Minimal) - | ( - FileSystemSpecialPath::CurrentWorkingDirectory, - FileSystemSpecialPath::CurrentWorkingDirectory, - ) | (FileSystemSpecialPath::Tmpdir, FileSystemSpecialPath::Tmpdir) | (FileSystemSpecialPath::SlashTmp, FileSystemSpecialPath::SlashTmp) => true, - ( - FileSystemSpecialPath::CurrentWorkingDirectory, - FileSystemSpecialPath::ProjectRoots { subpath: None }, - ) - | ( - FileSystemSpecialPath::ProjectRoots { subpath: None }, - FileSystemSpecialPath::CurrentWorkingDirectory, - ) => true, ( FileSystemSpecialPath::ProjectRoots { subpath: left }, FileSystemSpecialPath::ProjectRoots { subpath: right }, @@ -1210,10 +1215,6 @@ fn resolve_file_system_special_path( FileSystemSpecialPath::Root | FileSystemSpecialPath::Minimal | FileSystemSpecialPath::Unknown { .. } => None, - FileSystemSpecialPath::CurrentWorkingDirectory => { - let cwd = cwd?; - Some(cwd.clone()) - } FileSystemSpecialPath::ProjectRoots { subpath } => { let cwd = cwd?; match subpath.as_ref() { @@ -1550,7 +1551,7 @@ mod tests { let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }]); @@ -1566,7 +1567,7 @@ mod tests { } #[test] - fn legacy_workspace_write_projection_preserves_symbolic_cwd() { + fn legacy_workspace_write_projection_preserves_symbolic_project_root() { let policy = SandboxPolicy::WorkspaceWrite { writable_roots: Vec::new(), network_access: false, @@ -1585,7 +1586,7 @@ mod tests { }, FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -1611,6 +1612,27 @@ mod tests { ); } + #[test] + fn legacy_current_working_directory_special_path_deserializes_as_project_roots() + -> serde_json::Result<()> { + let value = serde_json::json!({ + "kind": "current_working_directory", + }); + + let special_path = serde_json::from_value::(value)?; + assert_eq!( + special_path, + FileSystemSpecialPath::project_roots(/*subpath*/ None) + ); + assert_eq!( + serde_json::to_value(&special_path)?, + serde_json::json!({ + "kind": "project_roots", + }) + ); + Ok(()) + } + #[cfg(unix)] #[test] fn writable_roots_skip_default_dot_codex_when_explicit_user_rule_exists() { @@ -1624,7 +1646,7 @@ mod tests { let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -1703,7 +1725,7 @@ mod tests { }, FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -1774,7 +1796,7 @@ mod tests { #[cfg(unix)] #[test] - fn current_working_directory_special_path_preserves_symlinked_cwd() { + fn project_roots_special_path_preserves_symlinked_root() { let cwd = TempDir::new().expect("tempdir"); let real_root = cwd.path().join("real"); let link_root = cwd.path().join("link"); @@ -1804,7 +1826,7 @@ mod tests { }, FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -2094,7 +2116,7 @@ mod tests { let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -2141,7 +2163,7 @@ mod tests { let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -2318,7 +2340,7 @@ mod tests { let cwd_root = AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute cwd"); let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Read, }]); @@ -2336,7 +2358,7 @@ mod tests { let cwd_root = AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute cwd"); let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }]); @@ -2356,7 +2378,7 @@ mod tests { .expect("resolve extra root"); let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }]); @@ -2368,7 +2390,7 @@ mod tests { FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 1e84e1806c..0e46b97b9f 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -4397,7 +4397,7 @@ mod tests { }, FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -4459,7 +4459,7 @@ mod tests { let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, diff --git a/codex-rs/rollout-trace/src/inference.rs b/codex-rs/rollout-trace/src/inference.rs index 935a2af6c9..3162308fb3 100644 --- a/codex-rs/rollout-trace/src/inference.rs +++ b/codex-rs/rollout-trace/src/inference.rs @@ -6,6 +6,7 @@ use std::fmt::Display; use std::sync::Arc; +use std::sync::atomic::AtomicBool; use std::sync::atomic::AtomicU64; use std::sync::atomic::Ordering; @@ -54,35 +55,36 @@ struct EnabledInferenceTraceContext { /// /// A Codex turn can create multiple attempts when auth recovery retries the /// HTTP request or WebSocket setup falls back to HTTP. Completion is often -/// observed after the client returns the response stream, so attempts are -/// cloneable and self-contained. -#[derive(Clone, Debug)] +/// observed after the client returns the response stream, so the attempt owns +/// the terminal guard that prevents duplicate lifecycle events. +#[derive(Debug)] pub struct InferenceTraceAttempt { state: InferenceTraceAttemptState, } -#[derive(Clone, Debug)] +#[derive(Debug)] enum InferenceTraceAttemptState { Disabled, Enabled(EnabledInferenceTraceAttempt), } -#[derive(Clone, Debug)] +#[derive(Debug)] struct EnabledInferenceTraceAttempt { context: EnabledInferenceTraceContext, inference_call_id: InferenceCallId, + terminal_recorded: AtomicBool, } -/// Non-delta response payload saved when a traced inference stream completes. +/// Non-delta response payload saved for completed or interrupted inference streams. /// /// We intentionally record completed output items instead of every stream delta /// here. The raw stream can be added later as a separate payload class; this -/// response summary gives the reducer stable response identity, usage, and -/// model-visible output without duplicating high-volume text deltas. +/// response summary gives the reducer stable response identity when available +/// plus model-visible output without duplicating high-volume text deltas. #[derive(Serialize)] -struct TracedResponseStreamCompleted<'a> { - response_id: &'a str, - token_usage: &'a Option, +struct TracedResponseStreamOutput<'a> { + response_id: Option<&'a str>, + token_usage: Option<&'a TokenUsage>, output_items: Vec, } @@ -123,6 +125,7 @@ impl InferenceTraceContext { state: InferenceTraceAttemptState::Enabled(EnabledInferenceTraceAttempt { context: context.clone(), inference_call_id: next_inference_call_id(), + terminal_recorded: AtomicBool::new(false), }), } } @@ -162,9 +165,9 @@ impl InferenceTraceAttempt { ); } - /// Records a bounded, non-streaming summary of the completed response stream. + /// Records successful provider completion and serializes the observed output items. /// - /// The caller passes protocol-native response items so this crate owns the + /// Callers pass protocol-native response items so this crate owns the /// trace-specific serialization rules. That keeps codex-core focused on /// transport behavior while preserving trace evidence that normal request /// serialization intentionally omits. @@ -174,18 +177,14 @@ impl InferenceTraceAttempt { token_usage: &Option, output_items: &[ResponseItem], ) { - let InferenceTraceAttemptState::Enabled(attempt) = &self.state else { + let Some(attempt) = self.take_terminal_attempt() else { return; }; - let response_payload = TracedResponseStreamCompleted { - response_id, - token_usage, - output_items: output_items.iter().map(trace_response_item_json).collect(), - }; - let Some(response_payload) = write_json_payload_best_effort( - &attempt.context.writer, - RawPayloadKind::InferenceResponse, - &response_payload, + let Some(response_payload) = write_response_payload_best_effort( + attempt, + Some(response_id), + token_usage.as_ref(), + output_items, ) else { return; }; @@ -201,19 +200,69 @@ impl InferenceTraceAttempt { } /// Records pre-response and mid-stream failures. - pub fn record_failed(&self, error: impl Display) { - let InferenceTraceAttemptState::Enabled(attempt) = &self.state else { + pub fn record_failed(&self, error: impl Display, output_items: &[ResponseItem]) { + let Some(attempt) = self.take_terminal_attempt() else { return; }; + let partial_response_payload = if output_items.is_empty() { + None + } else { + write_response_payload_best_effort( + attempt, + /*response_id*/ None, + /*token_usage*/ None, + output_items, + ) + }; append_with_context_best_effort( &attempt.context, RawTraceEventPayload::InferenceFailed { inference_call_id: attempt.inference_call_id.clone(), error: error.to_string(), - partial_response_payload: None, + partial_response_payload, }, ); } + + /// Records a provider stream that Codex intentionally stopped consuming. + /// + /// This happens when the turn is interrupted or when mailbox delivery + /// preempts the current sampling request. Complete output items observed + /// before that point are retained as partial response evidence. + pub fn record_cancelled(&self, reason: impl Display, output_items: &[ResponseItem]) { + let Some(attempt) = self.take_terminal_attempt() else { + return; + }; + let partial_response_payload = if output_items.is_empty() { + None + } else { + write_response_payload_best_effort( + attempt, + /*response_id*/ None, + /*token_usage*/ None, + output_items, + ) + }; + append_with_context_best_effort( + &attempt.context, + RawTraceEventPayload::InferenceCancelled { + inference_call_id: attempt.inference_call_id.clone(), + reason: reason.to_string(), + partial_response_payload, + }, + ); + } + + fn take_terminal_attempt(&self) -> Option<&EnabledInferenceTraceAttempt> { + let attempt = match &self.state { + InferenceTraceAttemptState::Disabled => return None, + InferenceTraceAttemptState::Enabled(attempt) => attempt, + }; + if attempt.terminal_recorded.swap(true, Ordering::AcqRel) { + return None; + } + Some(attempt) + } } /// Serializes a response item for trace evidence rather than future request construction. @@ -260,6 +309,24 @@ fn write_json_payload_best_effort( writer.write_json_payload(kind, payload).ok() } +fn write_response_payload_best_effort( + attempt: &EnabledInferenceTraceAttempt, + response_id: Option<&str>, + token_usage: Option<&TokenUsage>, + output_items: &[ResponseItem], +) -> Option { + let response_payload = TracedResponseStreamOutput { + response_id, + token_usage, + output_items: output_items.iter().map(trace_response_item_json).collect(), + }; + write_json_payload_best_effort( + &attempt.context.writer, + RawPayloadKind::InferenceResponse, + &response_payload, + ) +} + fn append_with_context_best_effort( context: &EnabledInferenceTraceContext, payload: RawTraceEventPayload, diff --git a/codex-rs/rollout-trace/src/raw_event.rs b/codex-rs/rollout-trace/src/raw_event.rs index b364601408..ad8f436da7 100644 --- a/codex-rs/rollout-trace/src/raw_event.rs +++ b/codex-rs/rollout-trace/src/raw_event.rs @@ -110,6 +110,13 @@ pub enum RawTraceEventPayload { /// Partial response payload, when stream events arrived before failure. partial_response_payload: Option, }, + InferenceCancelled { + inference_call_id: InferenceCallId, + /// Why Codex stopped consuming the provider stream before a terminal response event. + reason: String, + /// Completed output items observed before cancellation, if any. + partial_response_payload: Option, + }, ToolCallStarted { tool_call_id: ToolCallId, /// Protocol/model call ID when this runtime call came from model output. @@ -253,6 +260,10 @@ impl RawTraceEventPayload { partial_response_payload, .. } + | RawTraceEventPayload::InferenceCancelled { + partial_response_payload, + .. + } | RawTraceEventPayload::ToolCallStarted { invocation_payload: partial_response_payload, .. diff --git a/codex-rs/rollout-trace/src/reducer/inference.rs b/codex-rs/rollout-trace/src/reducer/inference.rs index ddd08142ff..c65b4236a2 100644 --- a/codex-rs/rollout-trace/src/reducer/inference.rs +++ b/codex-rs/rollout-trace/src/reducer/inference.rs @@ -103,6 +103,35 @@ impl TraceReducer { Ok(()) } + /// Closes any inference streams that are still live when the owning turn ends. + /// + /// Normal completion events close the active inference before the turn ends. + /// If a call is still `Running`, Codex stopped observing that provider stream + /// earlier and the reduced graph should not present it as live. + pub(super) fn close_running_inference_calls_for_turn_end( + &mut self, + seq: RawEventSeq, + wall_time_unix_ms: i64, + codex_turn_id: &str, + turn_status: &ExecutionStatus, + ) { + let inference_status = match turn_status { + ExecutionStatus::Running => return, + ExecutionStatus::Completed | ExecutionStatus::Cancelled => ExecutionStatus::Cancelled, + ExecutionStatus::Failed => ExecutionStatus::Failed, + ExecutionStatus::Aborted => ExecutionStatus::Aborted, + }; + for inference in self.rollout.inference_calls.values_mut() { + if inference.codex_turn_id == codex_turn_id + && inference.execution.status == ExecutionStatus::Running + { + inference.execution.ended_at_unix_ms = Some(wall_time_unix_ms); + inference.execution.ended_seq = Some(seq); + inference.execution.status = inference_status.clone(); + } + } + } + /// Completes an inference call and, when present, reduces response output items. pub(super) fn complete_inference_call( &mut self, @@ -130,14 +159,25 @@ impl TraceReducer { let Some(inference) = self.rollout.inference_calls.get_mut(&inference_call_id) else { bail!("inference call {inference_call_id} disappeared during response reduction"); }; - inference.execution.ended_at_unix_ms = Some(wall_time_unix_ms); - inference.execution.ended_seq = Some(seq); - inference.execution.status = status; - inference.upstream_request_id = response_id; - inference.raw_response_payload_id = response_payload.map(|payload| payload.raw_payload_id); + // Turn-end cleanup can close a stream before the async mapper observes + // cancellation. Preserve that terminal status while still retaining any + // late partial response evidence from the mapper. + if inference.execution.status == ExecutionStatus::Running { + inference.execution.ended_at_unix_ms = Some(wall_time_unix_ms); + inference.execution.ended_seq = Some(seq); + inference.execution.status = status; + inference.upstream_request_id = response_id; + } + if let Some(response_payload) = response_payload { + inference.raw_response_payload_id = Some(response_payload.raw_payload_id); + } if let Some(response_item_ids) = response_item_ids { inference.response_item_ids = response_item_ids; } Ok(()) } } + +#[cfg(test)] +#[path = "inference_tests.rs"] +mod tests; diff --git a/codex-rs/rollout-trace/src/reducer/inference_tests.rs b/codex-rs/rollout-trace/src/reducer/inference_tests.rs new file mode 100644 index 0000000000..d5ba0ba9d0 --- /dev/null +++ b/codex-rs/rollout-trace/src/reducer/inference_tests.rs @@ -0,0 +1,148 @@ +use pretty_assertions::assert_eq; +use serde_json::json; +use tempfile::TempDir; + +use crate::model::ConversationItemKind; +use crate::model::ExecutionStatus; +use crate::payload::RawPayloadKind; +use crate::raw_event::RawTraceEventPayload; +use crate::reducer::test_support::append_inference_start; +use crate::reducer::test_support::create_started_writer; +use crate::reducer::test_support::message; +use crate::reducer::test_support::start_turn; +use crate::replay_bundle; + +#[test] +fn cancelled_inference_reduces_partial_response_items() -> anyhow::Result<()> { + let temp = TempDir::new()?; + let writer = create_started_writer(&temp)?; + start_turn(&writer, "turn-1")?; + + let request = writer.write_json_payload( + RawPayloadKind::InferenceRequest, + &json!({ + "input": [message("user", "draft")] + }), + )?; + append_inference_start(&writer, "inference-1", "turn-1", request)?; + + let partial_response = writer.write_json_payload( + RawPayloadKind::InferenceResponse, + &json!({ + "response_id": null, + "token_usage": null, + "output_items": [{ + "type": "message", + "role": "assistant", + "content": [{"type": "output_text", "text": "partial"}] + }] + }), + )?; + writer.append(RawTraceEventPayload::InferenceCancelled { + inference_call_id: "inference-1".to_string(), + reason: "test interruption".to_string(), + partial_response_payload: Some(partial_response), + })?; + + let rollout = replay_bundle(temp.path())?; + let inference = &rollout.inference_calls["inference-1"]; + let response_item_id = &inference.response_item_ids[0]; + + assert_eq!(inference.execution.status, ExecutionStatus::Cancelled); + assert_eq!(inference.response_item_ids.len(), 1); + assert_eq!( + rollout.conversation_items[response_item_id].kind, + ConversationItemKind::Message, + ); + assert_eq!( + rollout.conversation_items[response_item_id].produced_by, + vec![crate::model::ProducerRef::Inference { + inference_call_id: "inference-1".to_string(), + }], + ); + + Ok(()) +} + +#[test] +fn cancelled_turn_closes_running_inference_call() -> anyhow::Result<()> { + let temp = TempDir::new()?; + let writer = create_started_writer(&temp)?; + start_turn(&writer, "turn-1")?; + + let request = writer.write_json_payload( + RawPayloadKind::InferenceRequest, + &json!({ + "input": [message("user", "wait")] + }), + )?; + append_inference_start(&writer, "inference-1", "turn-1", request)?; + let turn_end = writer.append(RawTraceEventPayload::CodexTurnEnded { + codex_turn_id: "turn-1".to_string(), + status: ExecutionStatus::Cancelled, + })?; + + let rollout = replay_bundle(temp.path())?; + let inference = &rollout.inference_calls["inference-1"]; + + assert_eq!(inference.execution.status, ExecutionStatus::Cancelled); + assert_eq!(inference.execution.ended_seq, Some(turn_end.seq)); + + Ok(()) +} + +#[test] +fn late_cancelled_inference_preserves_turn_end_status() -> anyhow::Result<()> { + let temp = TempDir::new()?; + let writer = create_started_writer(&temp)?; + start_turn(&writer, "turn-1")?; + + let request = writer.write_json_payload( + RawPayloadKind::InferenceRequest, + &json!({ + "input": [message("user", "interrupt")] + }), + )?; + append_inference_start(&writer, "inference-1", "turn-1", request)?; + let turn_end = writer.append(RawTraceEventPayload::CodexTurnEnded { + codex_turn_id: "turn-1".to_string(), + status: ExecutionStatus::Failed, + })?; + + let partial_response = writer.write_json_payload( + RawPayloadKind::InferenceResponse, + &json!({ + "response_id": null, + "token_usage": null, + "output_items": [{ + "type": "message", + "role": "assistant", + "content": [{"type": "output_text", "text": "late partial"}] + }] + }), + )?; + writer.append(RawTraceEventPayload::InferenceCancelled { + inference_call_id: "inference-1".to_string(), + reason: "stream mapper noticed cancellation after turn end".to_string(), + partial_response_payload: Some(partial_response.clone()), + })?; + + let rollout = replay_bundle(temp.path())?; + let inference = &rollout.inference_calls["inference-1"]; + assert_eq!(inference.execution.status, ExecutionStatus::Failed); + assert_eq!(inference.execution.ended_seq, Some(turn_end.seq)); + assert_eq!( + inference.raw_response_payload_id, + Some(partial_response.raw_payload_id), + ); + assert_eq!(inference.response_item_ids.len(), 1); + let response_item_id = &inference.response_item_ids[0]; + assert_eq!( + rollout.conversation_items[response_item_id].body.parts, + vec![crate::model::ConversationPart::Text { + text: "late partial".to_string(), + }], + ); + + Ok(()) +} diff --git a/codex-rs/rollout-trace/src/reducer/mod.rs b/codex-rs/rollout-trace/src/reducer/mod.rs index e4f6b837f4..34a4e5dcfd 100644 --- a/codex-rs/rollout-trace/src/reducer/mod.rs +++ b/codex-rs/rollout-trace/src/reducer/mod.rs @@ -254,6 +254,20 @@ impl TraceReducer { partial_response_payload, )?; } + RawTraceEventPayload::InferenceCancelled { + inference_call_id, + partial_response_payload, + .. + } => { + self.complete_inference_call( + event.seq, + event.wall_time_unix_ms, + inference_call_id, + ExecutionStatus::Cancelled, + /*response_id*/ None, + partial_response_payload, + )?; + } RawTraceEventPayload::ProtocolEventObserved { .. } => { // Protocol wrappers are raw debug breadcrumbs. Typed hooks own // the reduced graph, so these payload refs are retained without diff --git a/codex-rs/rollout-trace/src/reducer/thread.rs b/codex-rs/rollout-trace/src/reducer/thread.rs index 6f24f69470..4ef39ee563 100644 --- a/codex-rs/rollout-trace/src/reducer/thread.rs +++ b/codex-rs/rollout-trace/src/reducer/thread.rs @@ -187,6 +187,12 @@ impl TraceReducer { &codex_turn_id, &status, )?; + self.close_running_inference_calls_for_turn_end( + seq, + wall_time_unix_ms, + &codex_turn_id, + &status, + ); Ok(()) } diff --git a/codex-rs/rollout-trace/src/thread_tests.rs b/codex-rs/rollout-trace/src/thread_tests.rs index 4d582bbe0f..a27026f28d 100644 --- a/codex-rs/rollout-trace/src/thread_tests.rs +++ b/codex-rs/rollout-trace/src/thread_tests.rs @@ -134,7 +134,7 @@ fn disabled_thread_context_accepts_trace_calls_without_writing() -> anyhow::Resu inference_attempt.record_started(&serde_json::json!({ "kind": "inference" })); let token_usage: Option = None; inference_attempt.record_completed("response-1", &token_usage, &[]); - inference_attempt.record_failed("inference failed"); + inference_attempt.record_failed("inference failed", &[]); let compaction_trace = thread_trace.compaction_trace_context( "turn-1", diff --git a/codex-rs/rollout/src/recorder.rs b/codex-rs/rollout/src/recorder.rs index 7b3037c4a9..f24cd9db94 100644 --- a/codex-rs/rollout/src/recorder.rs +++ b/codex-rs/rollout/src/recorder.rs @@ -444,9 +444,19 @@ impl RolloutRecorder { )); } - // Warm the DB by repairing every filesystem hit before querying SQLite. + // For metadata-filtered listings the filesystem page is the page we return. Track those + // IDs so the later DB page only triggers full reconciliation for DB-only hits. + let fs_page_thread_ids = fs_page + .items + .iter() + .filter_map(|item| item.thread_id) + .collect::>(); + + // Warm the DB by repairing every filesystem hit before querying SQLite. Source/provider/cwd + // filters are already validated from rollout head metadata, so lightweight read-repair is + // enough there. Search can depend on full title metadata, so keep full reconciliation. for item in &fs_page.items { - if listing_has_metadata_filters { + if search_term.is_some() { state_db::reconcile_rollout( state_db_ctx.as_deref(), item.path.as_path(), @@ -517,6 +527,12 @@ impl RolloutRecorder { } if listing_has_metadata_filters { for item in &db_page.items { + // Rows that also appeared in the filesystem page were just validated from the + // rollout head. Rows only found by SQLite may be stale filter matches, so fully + // reconcile those before returning the filesystem-backed page. + if fs_page_thread_ids.contains(&item.id) { + continue; + } state_db::reconcile_rollout( state_db_ctx.as_deref(), item.rollout_path.as_path(), @@ -1030,13 +1046,13 @@ fn fill_missing_thread_item_metadata(item: &mut ThreadItem, state_item: ThreadIt if item.cwd.is_none() { item.cwd = cwd; } - if item.git_branch.is_none() { + if git_branch.is_some() { item.git_branch = git_branch; } - if item.git_sha.is_none() { + if git_sha.is_some() { item.git_sha = git_sha; } - if item.git_origin_url.is_none() { + if git_origin_url.is_some() { item.git_origin_url = git_origin_url; } if item.source.is_none() { diff --git a/codex-rs/rollout/src/recorder_tests.rs b/codex-rs/rollout/src/recorder_tests.rs index 1516987669..a53731bece 100644 --- a/codex-rs/rollout/src/recorder_tests.rs +++ b/codex-rs/rollout/src/recorder_tests.rs @@ -779,7 +779,7 @@ async fn list_threads_metadata_filter_overlays_state_db_list_metadata() -> std:: } #[test] -fn fill_missing_thread_item_metadata_preserves_filesystem_identity() { +fn fill_missing_thread_item_metadata_preserves_identity_and_prefers_state_git_fields() { let filesystem_thread_id = ThreadId::new(); let state_thread_id = ThreadId::new(); let filesystem_path = PathBuf::from("/tmp/filesystem-rollout.jsonl"); @@ -789,9 +789,9 @@ fn fill_missing_thread_item_metadata_preserves_filesystem_identity() { thread_id: Some(filesystem_thread_id), first_user_message: Some("filesystem message".to_string()), cwd: None, - git_branch: None, - git_sha: None, - git_origin_url: None, + git_branch: Some("filesystem-branch".to_string()), + git_sha: Some("filesystem-sha".to_string()), + git_origin_url: Some("https://example.com/filesystem.git".to_string()), source: None, agent_nickname: None, agent_role: None, diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs index fc352865a8..3dad862bea 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -350,9 +350,7 @@ fn materialize_cwd_dependent_entry( ) -> FileSystemSandboxEntry { match &entry.path { FileSystemPath::Special { - value: - FileSystemSpecialPath::CurrentWorkingDirectory - | FileSystemSpecialPath::ProjectRoots { .. }, + value: FileSystemSpecialPath::ProjectRoots { .. }, } => resolve_permission_path(&entry.path, cwd) .map(|path| FileSystemSandboxEntry { path: FileSystemPath::Path { path }, @@ -380,9 +378,6 @@ fn resolve_permission_path(path: &FileSystemPath, cwd: &Path) -> Option { - AbsolutePathBuf::from_absolute_path(cwd).ok() - } FileSystemSpecialPath::ProjectRoots { subpath } => { let cwd = AbsolutePathBuf::from_absolute_path(cwd).ok()?; Some(match subpath { diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index 9b41205735..3404eee3da 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -299,7 +299,7 @@ fn intersect_permission_profiles_accepts_child_path_granted_for_requested_cwd() file_system: Some(FileSystemPermissions { entries: vec![FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }], @@ -332,7 +332,7 @@ fn intersect_permission_profiles_materializes_cwd_grant_for_reuse() { file_system: Some(FileSystemPermissions { entries: vec![FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }], @@ -383,7 +383,7 @@ fn intersect_permission_profiles_deduplicates_materialized_grants() { entries: vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -425,7 +425,7 @@ fn intersect_permission_profiles_materializes_cwd_deny_entries() { }, FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::None, }, @@ -474,7 +474,7 @@ fn intersect_permission_profiles_drops_deny_entries_without_filesystem_grants() entries: vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -512,7 +512,7 @@ fn intersect_permission_profiles_rejects_concrete_grants_matched_by_requested_de entries: vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }, @@ -550,7 +550,7 @@ fn intersect_permission_profiles_materializes_relative_deny_globs_for_reuse() { .expect("absolute later cwd"); let cwd_write = FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }; @@ -629,7 +629,7 @@ fn intersect_permission_profiles_drops_broader_cwd_grant_for_requested_child_pat file_system: Some(FileSystemPermissions { entries: vec![FileSystemSandboxEntry { path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), }, access: FileSystemAccessMode::Write, }], diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index ce3b04b33b..666d6556df 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -102,7 +102,7 @@ impl ApprovalRequest { } } - fn matches_resolved_request(&self, request: &ResolvedAppServerRequest) -> bool { + pub(super) fn matches_resolved_request(&self, request: &ResolvedAppServerRequest) -> bool { match (self, request) { ( ApprovalRequest::Exec { id, .. }, @@ -875,7 +875,6 @@ fn special_path_label(value: &FileSystemSpecialPath) -> String { match value { FileSystemSpecialPath::Root => ":root".to_string(), FileSystemSpecialPath::Minimal => ":minimal".to_string(), - FileSystemSpecialPath::CurrentWorkingDirectory => ":cwd".to_string(), FileSystemSpecialPath::ProjectRoots { subpath } => path_label(":project_roots", subpath), FileSystemSpecialPath::Tmpdir => ":tmpdir".to_string(), FileSystemSpecialPath::SlashTmp => "/tmp".to_string(), diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index a2067fb7e7..7defaaf6ff 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -13,6 +13,7 @@ //! //! Some UI is time-based rather than input-based, such as the transient "press again to quit" //! hint. The pane schedules redraws so those hints can expire even when the UI is otherwise idle. +use std::collections::VecDeque; use std::path::PathBuf; use crate::app::app_server_requests::ResolvedAppServerRequest; @@ -42,6 +43,7 @@ use ratatui::buffer::Buffer; use ratatui::layout::Rect; use ratatui::text::Line; use std::time::Duration; +use std::time::Instant; mod app_link_view; mod approval_overlay; @@ -139,6 +141,8 @@ pub(crate) use selection_tabs::SelectionTab; /// Keeping a single value ensures Ctrl+C and Ctrl+D behave identically. pub(crate) const QUIT_SHORTCUT_TIMEOUT: Duration = Duration::from_secs(1); +const APPROVAL_PROMPT_TYPING_IDLE_DELAY: Duration = Duration::from_secs(1); + /// Whether Ctrl+C/Ctrl+D require a second press to quit. /// /// This UX experiment was enabled by default, but requiring a double press to quit feels janky in @@ -170,6 +174,11 @@ pub(crate) use experimental_features_view::ExperimentalFeaturesView; pub(crate) use list_selection_view::SelectionAction; pub(crate) use list_selection_view::SelectionItem; +struct DelayedApprovalRequest { + request: ApprovalRequest, + features: Features, +} + /// Pane displayed in the lower half of the chat UI. /// /// This is the owning container for the prompt input (`ChatComposer`) and the view stack @@ -182,6 +191,8 @@ pub(crate) struct BottomPane { /// Stack of views displayed instead of the composer (e.g. popups/modals). view_stack: Vec>, + delayed_approval_requests: VecDeque, + last_composer_activity_at: Option, app_event_tx: AppEventSender, frame_requester: FrameRequester, @@ -243,6 +254,8 @@ impl BottomPane { Self { composer, view_stack: Vec::new(), + delayed_approval_requests: VecDeque::new(), + last_composer_activity_at: None, app_event_tx, frame_requester, has_input_focus, @@ -442,6 +455,48 @@ impl BottomPane { } } + fn approval_prompt_delay_remaining(&self, now: Instant) -> Option { + self.last_composer_activity_at.and_then(|last_activity_at| { + last_activity_at + .checked_add(APPROVAL_PROMPT_TYPING_IDLE_DELAY) + .and_then(|show_at| show_at.checked_duration_since(now)) + .filter(|delay| !delay.is_zero()) + }) + } + + fn record_composer_activity_at(&mut self, now: Instant) { + self.last_composer_activity_at = Some(now); + if !self.delayed_approval_requests.is_empty() + && let Some(delay) = self.approval_prompt_delay_remaining(now) + { + self.request_redraw_in(delay); + } + } + + fn maybe_show_delayed_approval_requests_at(&mut self, now: Instant) { + if self.delayed_approval_requests.is_empty() || !self.view_stack.is_empty() { + return; + } + if let Some(delay) = self.approval_prompt_delay_remaining(now) { + self.request_redraw_in(delay); + return; + } + + // Promote the oldest delayed approval once typing has been idle long enough. + // `ApprovalOverlay` advances its internal queue with `pop()`, so drain the + // remaining delayed approvals from the back to preserve FIFO display order. + let Some(first) = self.delayed_approval_requests.pop_front() else { + return; + }; + let mut modal = + ApprovalOverlay::new(first.request, self.app_event_tx.clone(), first.features); + while let Some(delayed) = self.delayed_approval_requests.pop_back() { + modal.enqueue_request(delayed.request); + } + self.pause_status_timer_for_modal(); + self.push_view(Box::new(modal)); + } + /// Forward a key event to the active view or the composer. pub fn handle_key_event(&mut self, key_event: KeyEvent) -> InputResult { // If a modal/view is active, handle it here; otherwise forward to composer. @@ -512,7 +567,21 @@ impl BottomPane { self.request_redraw(); return InputResult::None; } + let records_composer_activity = + matches!(key_event.kind, KeyEventKind::Press | KeyEventKind::Repeat) + && !key_hint::has_ctrl_or_alt(key_event.modifiers) + && matches!( + key_event.code, + KeyCode::Char(_) + | KeyCode::Backspace + | KeyCode::Delete + | KeyCode::Enter + | KeyCode::Tab + ); let (input_result, needs_redraw) = self.composer.handle_key_event(key_event); + if records_composer_activity { + self.record_composer_activity_at(Instant::now()); + } if needs_redraw { self.request_redraw(); } @@ -560,6 +629,7 @@ impl BottomPane { } pub fn handle_paste(&mut self, pasted: String) { + let has_pasted_text = !pasted.is_empty(); if let Some(view) = self.view_stack.last_mut() { let needs_redraw = view.handle_paste(pasted); let view_complete = view.is_complete(); @@ -572,6 +642,9 @@ impl BottomPane { } } else { let needs_redraw = self.composer.handle_paste(pasted); + if has_pasted_text { + self.record_composer_activity_at(Instant::now()); + } self.composer.sync_popups(); if needs_redraw { self.request_redraw(); @@ -586,7 +659,12 @@ impl BottomPane { } pub(crate) fn pre_draw_tick(&mut self) { + self.pre_draw_tick_at(Instant::now()); + } + + fn pre_draw_tick_at(&mut self, now: Instant) { self.composer.sync_popups(); + self.maybe_show_delayed_approval_requests_at(now); } /// Replace the composer text with `text`. @@ -989,10 +1067,22 @@ impl BottomPane { request }; - // Otherwise create a new approval modal overlay. - let modal = ApprovalOverlay::new(request, self.app_event_tx.clone(), features.clone()); - self.pause_status_timer_for_modal(); - self.push_view(Box::new(modal)); + let now = Instant::now(); + if !self.delayed_approval_requests.is_empty() + || self.approval_prompt_delay_remaining(now).is_some() + { + self.delayed_approval_requests + .push_back(DelayedApprovalRequest { + request, + features: features.clone(), + }); + self.maybe_show_delayed_approval_requests_at(now); + } else { + // No recent composer activity, so show the approval modal immediately. + let modal = ApprovalOverlay::new(request, self.app_event_tx.clone(), features.clone()); + self.pause_status_timer_for_modal(); + self.push_view(Box::new(modal)); + } } /// Called when the agent requests user input. @@ -1107,11 +1197,19 @@ impl BottomPane { &mut self, request: &ResolvedAppServerRequest, ) -> bool { + let delayed_len = self.delayed_approval_requests.len(); + self.delayed_approval_requests + .retain(|delayed| !delayed.request.matches_resolved_request(request)); + let delayed_changed = self.delayed_approval_requests.len() != delayed_len; + if self.view_stack.is_empty() { - return false; + if delayed_changed { + self.request_redraw(); + } + return delayed_changed; } - let mut changed = false; + let mut changed = delayed_changed; let mut completed_indices = Vec::new(); for index in (0..self.view_stack.len()).rev() { let view = &mut self.view_stack[index]; @@ -1356,6 +1454,7 @@ mod tests { use crate::test_support::PathBufExt; use crate::test_support::test_path_buf; use codex_protocol::protocol::Op; + use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SkillScope; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; @@ -1367,6 +1466,7 @@ mod tests { use ratatui::layout::Rect; use std::cell::Cell; use std::rc::Rc; + use std::time::Instant; use tokio::sync::mpsc::unbounded_channel; fn snapshot_buffer(buf: &Buffer) -> String { @@ -1388,13 +1488,20 @@ mod tests { } fn test_pane(app_event_tx: AppEventSender) -> BottomPane { + test_pane_with_disable_paste_burst(app_event_tx, /*disable_paste_burst*/ false) + } + + fn test_pane_with_disable_paste_burst( + app_event_tx: AppEventSender, + disable_paste_burst: bool, + ) -> BottomPane { BottomPane::new(BottomPaneParams { app_event_tx, frame_requester: FrameRequester::test_dummy(), has_input_focus: true, enhanced_keys_supported: false, placeholder_text: "Ask Codex to do anything".to_string(), - disable_paste_burst: false, + disable_paste_burst, animations_enabled: true, skills: Some(Vec::new()), }) @@ -1565,6 +1672,136 @@ mod tests { ); } + #[test] + fn approval_request_shows_immediately_without_recent_typing() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let features = Features::with_defaults(); + let mut pane = test_pane(tx); + + pane.push_approval_request(exec_request(), &features); + + assert_eq!(pane.view_stack.len(), 1); + assert!(pane.delayed_approval_requests.is_empty()); + } + + #[test] + fn approval_request_is_delayed_after_recent_typing() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let features = Features::with_defaults(); + let mut pane = test_pane(tx); + let now = Instant::now(); + pane.last_composer_activity_at = Some(now); + + pane.push_approval_request(exec_request(), &features); + + assert!(pane.view_stack.is_empty()); + assert_eq!(pane.delayed_approval_requests.len(), 1); + + pane.pre_draw_tick_at( + now + APPROVAL_PROMPT_TYPING_IDLE_DELAY - Duration::from_millis(/*millis*/ 1), + ); + assert!(pane.view_stack.is_empty()); + assert_eq!(pane.delayed_approval_requests.len(), 1); + + pane.pre_draw_tick_at(now + APPROVAL_PROMPT_TYPING_IDLE_DELAY); + assert_eq!(pane.view_stack.len(), 1); + assert!(pane.delayed_approval_requests.is_empty()); + } + + #[test] + fn continued_typing_resets_delayed_approval_idle_deadline() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let features = Features::with_defaults(); + let mut pane = test_pane(tx); + let first_activity = Instant::now(); + pane.last_composer_activity_at = Some(first_activity); + pane.push_approval_request(exec_request(), &features); + + let continued_activity = first_activity + Duration::from_millis(/*millis*/ 750); + pane.record_composer_activity_at(continued_activity); + + pane.pre_draw_tick_at(first_activity + APPROVAL_PROMPT_TYPING_IDLE_DELAY); + assert!(pane.view_stack.is_empty()); + assert_eq!(pane.delayed_approval_requests.len(), 1); + + pane.pre_draw_tick_at(continued_activity + APPROVAL_PROMPT_TYPING_IDLE_DELAY); + assert_eq!(pane.view_stack.len(), 1); + assert!(pane.delayed_approval_requests.is_empty()); + } + + #[test] + fn typed_approval_shortcuts_during_delay_stay_in_composer() { + let (tx_raw, mut rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let features = Features::with_defaults(); + let mut pane = test_pane_with_disable_paste_burst(tx, /*disable_paste_burst*/ true); + pane.last_composer_activity_at = Some(Instant::now()); + pane.push_approval_request(exec_request(), &features); + + pane.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + pane.handle_key_event(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::NONE)); + + assert_eq!(pane.composer_text(), "ya"); + assert!(pane.view_stack.is_empty()); + assert_eq!(pane.delayed_approval_requests.len(), 1); + while let Ok(event) = rx.try_recv() { + assert!( + !matches!(event, AppEvent::SubmitThreadOp { .. }), + "delayed approval shortcut should not submit an approval: {event:?}" + ); + } + } + + #[test] + fn delayed_approval_shortcut_works_after_idle_deadline() { + let (tx_raw, mut rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let features = Features::with_defaults(); + let mut pane = test_pane(tx); + let now = Instant::now(); + pane.last_composer_activity_at = Some(now); + pane.push_approval_request(exec_request(), &features); + + pane.pre_draw_tick_at(now + APPROVAL_PROMPT_TYPING_IDLE_DELAY); + pane.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + + let mut approval_decision = None; + while let Ok(event) = rx.try_recv() { + if let AppEvent::SubmitThreadOp { + op: Op::ExecApproval { decision, .. }, + .. + } = event + { + approval_decision = Some(decision); + } + } + assert_eq!(approval_decision, Some(ReviewDecision::Approved)); + } + + #[test] + fn dismiss_app_server_request_prunes_delayed_approval() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let features = Features::with_defaults(); + let mut pane = test_pane(tx); + let now = Instant::now(); + pane.last_composer_activity_at = Some(now); + pane.push_approval_request(exec_request(), &features); + + assert!( + pane.dismiss_app_server_request(&ResolvedAppServerRequest::ExecApproval { + id: "1".to_string(), + }) + ); + assert!(pane.delayed_approval_requests.is_empty()); + + pane.pre_draw_tick_at(now + APPROVAL_PROMPT_TYPING_IDLE_DELAY); + assert!(pane.view_stack.is_empty()); + } + #[test] fn dismiss_app_server_request_removes_matching_buried_view() { let (tx_raw, _rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/markdown_render.rs b/codex-rs/tui/src/markdown_render.rs index ae680f5db3..3dfef1d9d8 100644 --- a/codex-rs/tui/src/markdown_render.rs +++ b/codex-rs/tui/src/markdown_render.rs @@ -154,6 +154,8 @@ where inline_styles: Vec