diff --git a/codex-rs/app-server/tests/common/rollout.rs b/codex-rs/app-server/tests/common/rollout.rs index c13f2355d4..a563e48d92 100644 --- a/codex-rs/app-server/tests/common/rollout.rs +++ b/codex-rs/app-server/tests/common/rollout.rs @@ -9,6 +9,7 @@ use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::TokenCountEvent; use codex_protocol::protocol::TokenUsage; use codex_protocol::protocol::TokenUsageInfo; +use core_test_support::test_path_buf; use serde_json::json; use std::fs; use std::fs::FileTimes; @@ -189,7 +190,7 @@ fn create_fake_rollout_with_source_and_parent_thread_id( forked_from_id: None, parent_thread_id, timestamp: meta_rfc3339.to_string(), - cwd: PathBuf::from("/"), + cwd: test_path_buf("/"), originator: "codex".to_string(), cli_version: "0.0.0".to_string(), source, @@ -279,7 +280,7 @@ pub fn create_fake_rollout_with_text_elements( forked_from_id: None, parent_thread_id: None, timestamp: meta_rfc3339.to_string(), - cwd: PathBuf::from("/"), + cwd: test_path_buf("/"), originator: "codex".to_string(), cli_version: "0.0.0".to_string(), source: SessionSource::Cli, diff --git a/codex-rs/app-server/tests/suite/conversation_summary.rs b/codex-rs/app-server/tests/suite/conversation_summary.rs index 9cb1409d79..4ea84b19a6 100644 --- a/codex-rs/app-server/tests/suite/conversation_summary.rs +++ b/codex-rs/app-server/tests/suite/conversation_summary.rs @@ -29,6 +29,7 @@ use codex_thread_store::InMemoryThreadStore; use codex_thread_store::ThreadPersistenceMetadata; use codex_thread_store::ThreadStore; use codex_utils_absolute_path::AbsolutePathBuf; +use core_test_support::test_path_buf; use pretty_assertions::assert_eq; use std::path::Path; use std::path::PathBuf; @@ -53,7 +54,7 @@ fn expected_summary(conversation_id: ThreadId, path: PathBuf) -> ConversationSum timestamp: Some(CREATED_AT_RFC3339.to_string()), updated_at: Some(UPDATED_AT_RFC3339.to_string()), model_provider: MODEL_PROVIDER.to_string(), - cwd: PathBuf::from("/"), + cwd: test_path_buf("/"), cli_version: "0.0.0".to_string(), source: SessionSource::Cli, git_info: None, diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 4cba4bddc5..d2911df407 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -656,11 +656,10 @@ async fn handle_patch_approval( let decision = if let Some(decision) = guardian_decision { decision } else { - let decision_rx = parent_session - .request_patch_approval(parent_ctx, call_id, changes, reason, grant_root) - .await; + let decision = + parent_session.request_patch_approval(parent_ctx, call_id, changes, reason, grant_root); await_approval_with_cancel( - async move { decision_rx.await.unwrap_or_default() }, + decision, parent_session, &approval_id, cancel_token, diff --git a/codex-rs/core/src/session/elicitation_holders_tests.rs b/codex-rs/core/src/session/elicitation_holders_tests.rs new file mode 100644 index 0000000000..df03aeb291 --- /dev/null +++ b/codex-rs/core/src/session/elicitation_holders_tests.rs @@ -0,0 +1,182 @@ +use std::collections::HashMap; + +use codex_protocol::protocol::ReviewDecision; +use codex_protocol::request_permissions::PermissionGrantScope; +use codex_protocol::request_permissions::RequestPermissionProfile; +use codex_protocol::request_permissions::RequestPermissionsArgs; +use codex_protocol::request_permissions::RequestPermissionsResponse; +use codex_protocol::request_user_input::RequestUserInputArgs; +use codex_protocol::request_user_input::RequestUserInputResponse; +use tokio::sync::watch; +use tokio_util::sync::CancellationToken; + +use super::tests::make_session_and_context_with_rx; +use crate::state::ActiveTurn; + +async fn wait_until_held(pause_state: &mut watch::Receiver) { + pause_state + .wait_for(|paused| *paused) + .await + .expect("elicitation service should remain available"); +} + +async fn wait_until_released(pause_state: &mut watch::Receiver) { + pause_state + .wait_for(|paused| !*paused) + .await + .expect("elicitation service should remain available"); +} + +#[tokio::test] +async fn command_approval_holds_an_elicitation_until_response() { + let (session, turn_context, events) = make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let mut pause_state = session.subscribe_elicitation_pause_state(); + #[allow(deprecated)] + let cwd = turn_context.cwd.clone(); + + let request = tokio::spawn({ + let session = session.clone(); + let turn_context = turn_context.clone(); + async move { + session + .request_command_approval( + turn_context.as_ref(), + "call-1".to_string(), + /*approval_id*/ None, + /*environment_id*/ None, + vec!["echo".to_string()], + cwd, + /*reason*/ None, + /*network_approval_context*/ None, + /*proposed_execpolicy_amendment*/ None, + /*additional_permissions*/ None, + /*available_decisions*/ None, + ) + .await + } + }); + + events.recv().await.expect("approval event"); + wait_until_held(&mut pause_state).await; + session + .notify_approval("call-1", ReviewDecision::Approved) + .await; + request.await.expect("approval task"); + wait_until_released(&mut pause_state).await; +} + +#[tokio::test] +async fn patch_approval_holds_an_elicitation_until_response() { + let (session, turn_context, events) = make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let mut pause_state = session.subscribe_elicitation_pause_state(); + + let request = tokio::spawn({ + let session = session.clone(); + let turn_context = turn_context.clone(); + async move { + session + .request_patch_approval( + turn_context.as_ref(), + "call-1".to_string(), + HashMap::new(), + /*reason*/ None, + /*grant_root*/ None, + ) + .await + } + }); + + events.recv().await.expect("approval event"); + wait_until_held(&mut pause_state).await; + session + .notify_approval("call-1", ReviewDecision::Approved) + .await; + request.await.expect("approval task"); + wait_until_released(&mut pause_state).await; +} + +#[tokio::test] +async fn permission_request_holds_an_elicitation_until_response() { + let (session, turn_context, events) = make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let mut pause_state = session.subscribe_elicitation_pause_state(); + + let request = tokio::spawn({ + let session = session.clone(); + let turn_context = turn_context.clone(); + async move { + let environment = turn_context + .environments + .primary() + .expect("primary environment") + .selection(); + session + .request_permissions_for_environment( + &turn_context, + "call-1".to_string(), + RequestPermissionsArgs { + environment_id: None, + reason: None, + permissions: RequestPermissionProfile::default(), + }, + environment, + CancellationToken::new(), + ) + .await + } + }); + + events.recv().await.expect("permission request event"); + wait_until_held(&mut pause_state).await; + session + .notify_request_permissions_response( + "call-1", + RequestPermissionsResponse { + permissions: RequestPermissionProfile::default(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + }, + ) + .await; + request.await.expect("permission request task"); + wait_until_released(&mut pause_state).await; +} + +#[tokio::test] +async fn request_user_input_holds_an_elicitation_until_response() { + let (session, turn_context, events) = make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let mut pause_state = session.subscribe_elicitation_pause_state(); + + let request = tokio::spawn({ + let session = session.clone(); + let turn_context = turn_context.clone(); + async move { + session + .request_user_input( + turn_context.as_ref(), + "call-1".to_string(), + RequestUserInputArgs { + questions: Vec::new(), + auto_resolution_ms: None, + }, + ) + .await + } + }); + + events.recv().await.expect("request user input event"); + wait_until_held(&mut pause_state).await; + + let response = RequestUserInputResponse { + answers: HashMap::new(), + }; + session + .notify_user_input_response(&turn_context.sub_id, response) + .await; + + request.await.expect("request user input task"); + wait_until_released(&mut pause_state).await; +} diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 822f24488b..ff109c12f3 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2164,6 +2164,7 @@ impl Session { additional_permissions: Option, available_decisions: Option>, ) -> ReviewDecision { + let _elicitation = self.services.elicitations.register(); // command-level approvals use `call_id`. // `approval_id` is only present for subcommand callbacks (execve intercept) let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); @@ -2235,7 +2236,8 @@ impl Session { changes: HashMap, reason: Option, grant_root: Option, - ) -> oneshot::Receiver { + ) -> ReviewDecision { + let _elicitation = self.services.elicitations.register(); // Add the tx_approve callback to the map before sending the request. let (tx_approve, rx_approve) = oneshot::channel(); let approval_id = call_id.clone(); @@ -2262,7 +2264,7 @@ impl Session { grant_root, }); self.send_event(turn_context, event).await; - rx_approve + rx_approve.await.unwrap_or(ReviewDecision::Abort) } #[expect( @@ -2390,6 +2392,7 @@ impl Session { return Some(response); } + let _elicitation = self.services.elicitations.register(); let (tx_response, rx_response) = oneshot::channel(); let prev_entry = { let mut active = self.active_turn.lock().await; @@ -2481,6 +2484,7 @@ impl Session { call_id: String, args: RequestUserInputArgs, ) -> Option { + let _elicitation = self.services.elicitations.register(); let sub_id = turn_context.sub_id.clone(); let (tx_response, rx_response) = oneshot::channel(); let event_id = sub_id.clone(); @@ -4051,5 +4055,9 @@ async fn build_hooks_for_config( }) } +#[cfg(test)] +#[path = "elicitation_holders_tests.rs"] +mod elicitation_holders_tests; + #[cfg(test)] pub(crate) mod tests; diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index b839b05c44..e5617e0a50 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -156,7 +156,7 @@ impl Approvable for ApplyPatchRuntime { return ReviewDecision::Approved; } if let Some(reason) = retry_reason { - let rx_approve = session + return session .request_patch_approval( turn, call_id, @@ -165,7 +165,6 @@ impl Approvable for ApplyPatchRuntime { /*grant_root*/ None, ) .await; - return rx_approve.await.unwrap_or_default(); } with_cached_approval( @@ -173,12 +172,11 @@ impl Approvable for ApplyPatchRuntime { "apply_patch", approval_keys, || async move { - let rx_approve = session + session .request_patch_approval( turn, call_id, changes, /*reason*/ None, /*grant_root*/ None, ) - .await; - rx_approve.await.unwrap_or_default() + .await }, ) .await diff --git a/codex-rs/core/tests/suite/code_mode_elicitation.rs b/codex-rs/core/tests/suite/code_mode_elicitation.rs new file mode 100644 index 0000000000..7df6cf2d4b --- /dev/null +++ b/codex-rs/core/tests/suite/code_mode_elicitation.rs @@ -0,0 +1,262 @@ +#![allow(clippy::unwrap_used)] + +use std::time::Duration; + +use anyhow::Result; +use codex_core::config::Config; +use codex_features::Feature; +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; +use codex_protocol::request_permissions::PermissionGrantScope; +use codex_protocol::request_permissions::RequestPermissionsResponse; +use codex_protocol::user_input::UserInput; +use core_test_support::responses; +use core_test_support::responses::ResponseMock; +use core_test_support::responses::ev_assistant_message; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_custom_tool_call; +use core_test_support::responses::ev_response_created; +use core_test_support::responses::sse; +use core_test_support::skip_if_no_network; +use core_test_support::skip_if_wine_exec; +use core_test_support::test_codex::TestCodex; +use core_test_support::test_codex::test_codex; +use core_test_support::test_codex::turn_permission_fields; +use core_test_support::wait_for_event; +use core_test_support::wait_for_event_match; +use core_test_support::wait_for_event_with_timeout; +use wiremock::MockServer; + +const YIELD_TIME_MS: u64 = 1_000; +const TURN_COMPLETE_TIMEOUT: Duration = Duration::from_secs(30); + +struct CodeModeElicitationHarness { + _server: MockServer, + test: TestCodex, + follow_up: ResponseMock, + turn_id: String, +} + +impl CodeModeElicitationHarness { + async fn start( + code: &str, + permission_profile: PermissionProfile, + configure: impl FnOnce(&mut Config) + Send + 'static, + ) -> Result { + let server = responses::start_mock_server().await; + let mut builder = + test_codex() + .with_model("test-gpt-5.1-codex") + .with_config(move |config| { + let _ = config.features.enable(Feature::CodeMode); + configure(config); + }); + let test = builder.build_with_auto_env(&server).await?; + let follow_up = mount_code_mode_responses(&server, code).await; + let turn_id = submit_turn(&test, permission_profile).await?; + Ok(Self { + _server: server, + test, + follow_up, + turn_id, + }) + } + + async fn assert_result_held(&self) { + tokio::time::sleep(Duration::from_millis(YIELD_TIME_MS + 250)).await; + assert!( + self.follow_up.requests().is_empty(), + "captured exec result should not return during a user elicitation" + ); + } + + async fn finish(self) { + wait_for_event_with_timeout( + &self.test.codex, + |event| match event { + EventMsg::TurnComplete(event) => event.turn_id == self.turn_id, + _ => false, + }, + TURN_COMPLETE_TIMEOUT, + ) + .await; + self.follow_up.single_request(); + } +} + +async fn mount_code_mode_responses(server: &MockServer, code: &str) -> ResponseMock { + responses::mount_sse_once( + server, + sse(vec![ + ev_response_created("resp-1"), + ev_custom_tool_call("call-1", "exec", code), + ev_completed("resp-1"), + ]), + ) + .await; + responses::mount_sse_once( + server, + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ) + .await +} + +async fn submit_turn(test: &TestCodex, permission_profile: PermissionProfile) -> Result { + let (sandbox_policy, permission_profile) = + turn_permission_fields(permission_profile, test.config.cwd.as_path()); + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "run a code-mode tool that needs user input".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + approval_policy: Some(AskForApproval::OnRequest), + sandbox_policy: Some(sandbox_policy), + permission_profile, + collaboration_mode: Some(codex_protocol::config_types::CollaborationMode { + mode: codex_protocol::config_types::ModeKind::Default, + settings: codex_protocol::config_types::Settings { + model: test.session_configured.model.clone(), + reasoning_effort: None, + developer_instructions: None, + }, + }), + ..Default::default() + }, + }) + .await?; + + Ok(wait_for_event_match(&test.codex, |event| match event { + EventMsg::TurnStarted(event) => Some(event.turn_id.clone()), + _ => None, + }) + .await) +} + +#[cfg_attr(windows, ignore = "no exec_command on Windows")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn code_mode_holds_yielded_result_during_command_approval() -> Result<()> { + skip_if_wine_exec!( + Ok(()), + "command approvals currently require a host-native cwd" + ); + skip_if_no_network!(Ok(())); + + let harness = CodeModeElicitationHarness::start( + r#"// @exec: {"yield_time_ms": 1000} +await tools.exec_command({ + cmd: "printf code_mode_approval_marker", + sandbox_permissions: "require_escalated", + justification: "test command approval", +});"#, + PermissionProfile::read_only(), + |_| {}, + ) + .await?; + let approval = wait_for_event_match(&harness.test.codex, |event| match event { + EventMsg::ExecApprovalRequest(approval) => Some(approval.clone()), + _ => None, + }) + .await; + + harness.assert_result_held().await; + harness + .test + .codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: Some(harness.turn_id.clone()), + decision: ReviewDecision::Approved, + }) + .await?; + harness.finish().await; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn code_mode_holds_yielded_result_during_patch_approval() -> Result<()> { + skip_if_no_network!(Ok(())); + + let harness = CodeModeElicitationHarness::start( + r#"// @exec: {"yield_time_ms": 1000} +await tools.apply_patch("*** Begin Patch\n*** Add File: code_mode_patch_approval.txt\n+held\n*** End Patch\n");"#, + PermissionProfile::read_only(), + |_| {}, + ) + .await?; + let approval = wait_for_event_match(&harness.test.codex, |event| match event { + EventMsg::ApplyPatchApprovalRequest(approval) => Some(approval.clone()), + _ => None, + }) + .await; + + harness.assert_result_held().await; + harness + .test + .codex + .submit(Op::PatchApproval { + id: approval.call_id, + decision: ReviewDecision::Approved, + }) + .await?; + harness.finish().await; + Ok(()) +} + +#[cfg_attr( + target_os = "linux", + ignore = "request_permissions tool integration is not supported on Linux" +)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn code_mode_holds_yielded_result_during_permission_request() -> Result<()> { + skip_if_no_network!(Ok(())); + + let harness = CodeModeElicitationHarness::start( + r#"// @exec: {"yield_time_ms": 1000} +await tools.request_permissions({ + reason: "test permission request", + permissions: { network: { enabled: true } }, +});"#, + PermissionProfile::read_only(), + |config| { + let _ = config.features.enable(Feature::RequestPermissionsTool); + }, + ) + .await?; + let request = wait_for_event(&harness.test.codex, |event| { + matches!( + event, + EventMsg::RequestPermissions(_) | EventMsg::TurnComplete(_) | EventMsg::Error(_) + ) + }) + .await; + let EventMsg::RequestPermissions(request) = request else { + panic!("expected request_permissions before turn completion, got {request:?}"); + }; + + harness.assert_result_held().await; + harness + .test + .codex + .submit(Op::RequestPermissionsResponse { + id: request.call_id, + response: RequestPermissionsResponse { + permissions: Default::default(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + }, + }) + .await?; + harness.finish().await; + Ok(()) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index e8bb48856c..ad0206f38f 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -42,6 +42,7 @@ mod cli_stream; mod client; mod client_websockets; mod code_mode; +mod code_mode_elicitation; mod codex_delegate; mod collaboration_instructions; mod compact; diff --git a/codex-rs/login/src/device_code_auth.rs b/codex-rs/login/src/device_code_auth.rs index 70e6dff225..e7a31916a5 100644 --- a/codex-rs/login/src/device_code_auth.rs +++ b/codex-rs/login/src/device_code_auth.rs @@ -145,15 +145,20 @@ async fn poll_for_token( } } -fn print_device_code_prompt(verification_url: &str, code: &str) { +fn device_code_prompt(verification_url: &str, code: &str) -> String { let version = env!("CARGO_PKG_VERSION"); - println!( + format!( "\nWelcome to Codex [v{ANSI_GRAY}{version}{ANSI_RESET}]\n{ANSI_GRAY}OpenAI's command-line coding agent{ANSI_RESET}\n\ \nFollow these steps to sign in with ChatGPT using device code authorization:\n\ \n1. Open this link in your browser and sign in to your account\n {ANSI_BLUE}{verification_url}{ANSI_RESET}\n\ \n2. Enter this one-time code {ANSI_GRAY}(expires in 15 minutes){ANSI_RESET}\n {ANSI_BLUE}{code}{ANSI_RESET}\n\ -\n{ANSI_GRAY}Device codes are a common phishing target. Never share this code.{ANSI_RESET}\n", - ); +\n{ANSI_GRAY}Continue only if you started this login in Codex. If a website or another person gave you this code, cancel.{ANSI_RESET}\n", + ) +} + +fn print_device_code_prompt(verification_url: &str, code: &str) { + let prompt = device_code_prompt(verification_url, code); + println!("{prompt}"); } pub async fn request_device_code(opts: &ServerOptions) -> std::io::Result { @@ -230,3 +235,7 @@ pub async fn run_device_code_login(opts: ServerOptions) -> std::io::Result<()> { print_device_code_prompt(&device_code.verification_url, &device_code.user_code); complete_device_code_login(opts, device_code).await } + +#[cfg(test)] +#[path = "device_code_auth_tests.rs"] +mod tests; diff --git a/codex-rs/login/src/device_code_auth_tests.rs b/codex-rs/login/src/device_code_auth_tests.rs new file mode 100644 index 0000000000..ba6071ba88 --- /dev/null +++ b/codex-rs/login/src/device_code_auth_tests.rs @@ -0,0 +1,10 @@ +use super::*; + +#[test] +fn device_code_prompt_renders_phishing_warning() { + let prompt = device_code_prompt("https://example.com/device", "ABCD-EFGH"); + + assert!(prompt.contains( + "\x1b[90mContinue only if you started this login in Codex. If a website or another person gave you this code, cancel.\x1b[0m" + )); +} diff --git a/codex-rs/tui/src/onboarding/auth/headless_chatgpt_login.rs b/codex-rs/tui/src/onboarding/auth/headless_chatgpt_login.rs index bdaac75ec3..2f7f2992d7 100644 --- a/codex-rs/tui/src/onboarding/auth/headless_chatgpt_login.rs +++ b/codex-rs/tui/src/onboarding/auth/headless_chatgpt_login.rs @@ -126,7 +126,7 @@ pub(super) fn render_device_code_login( ])); lines.push("".into()); lines.push( - " Device codes are a common phishing target. Never share this code." + " Continue only if you started this login in Codex. If a website or another person gave you this code, cancel." .dim() .into(), );