diff --git a/codex-rs/core/src/session/mcp_runtime.rs b/codex-rs/core/src/session/mcp_runtime.rs index 9a68c553b4..f51bdbbde3 100644 --- a/codex-rs/core/src/session/mcp_runtime.rs +++ b/codex-rs/core/src/session/mcp_runtime.rs @@ -67,8 +67,7 @@ impl Session { .primary() .and_then(|environment| environment.cwd().to_abs_path().ok()) .unwrap_or_else(|| session_configuration.cwd().clone()); - let mut config = Self::build_per_turn_config(&session_configuration, cwd); - config.permissions.approval_policy = session_configuration.approval_policy.clone(); + let config = Self::build_per_turn_config(&session_configuration, cwd); McpDesiredState { config: Arc::new(config), @@ -90,8 +89,7 @@ impl Session { ) -> anyhow::Result<()> { let cwd = AbsolutePathBuf::from_absolute_path(local_stdio_fallback_cwd) .unwrap_or_else(|_| session_configuration.cwd().clone()); - let mut config = Self::build_per_turn_config(session_configuration, cwd); - config.permissions.approval_policy = session_configuration.approval_policy.clone(); + let config = Self::build_per_turn_config(session_configuration, cwd); let desired = McpDesiredState { config: Arc::new(config), auth, diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index 1009386814..872a73a7da 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -48,11 +48,8 @@ pub(super) async fn spawn_review_thread( let mut per_turn_config = (*config).clone(); per_turn_config.model = Some(model.clone()); per_turn_config.features = review_features.clone(); - per_turn_config.permissions.shell_environment_policy = parent_turn_context - .config - .permissions - .shell_environment_policy - .clone(); + per_turn_config.permissions = parent_turn_context.config.permissions.clone(); + per_turn_config.approvals_reviewer = parent_turn_context.config.approvals_reviewer; per_turn_config.codex_linux_sandbox_exe = parent_turn_context.config.codex_linux_sandbox_exe.clone(); per_turn_config.compact_prompt = parent_turn_context.config.compact_prompt.clone(); diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 040138acc0..9a21b6a1a2 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -8148,6 +8148,11 @@ async fn refreshed_mcp_binding_captures_current_approval_authority() { let (session, old_turn) = make_session_and_context().await; let session = Arc::new(session); let previous_policy = old_turn.approval_policy.value(); + assert_ne!(previous_policy, AskForApproval::Never); + assert_eq!( + old_turn.config.permissions.approval_policy.value(), + previous_policy + ); session .update_settings(SessionSettingsUpdate { @@ -8180,6 +8185,17 @@ async fn refreshed_mcp_binding_captures_current_approval_authority() { ) ); assert_eq!(old_turn.approval_policy.value(), previous_policy); + assert_eq!( + old_turn.config.permissions.approval_policy.value(), + previous_policy + ); + + let new_turn = session.new_default_turn().await; + assert_eq!(new_turn.approval_policy.value(), AskForApproval::Never); + assert_eq!( + new_turn.config.permissions.approval_policy.value(), + AskForApproval::Never + ); } #[tokio::test] diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 30ae7e2d81..29518e98ff 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -440,6 +440,7 @@ impl Session { let config = session_configuration.original_config_do_not_use.clone(); let mut per_turn_config = (*config).clone(); per_turn_config.cwd = cwd; + per_turn_config.permissions.approval_policy = session_configuration.approval_policy.clone(); let workspace_roots = session_configuration.primary_workspace_roots(); per_turn_config.workspace_roots = workspace_roots.clone(); per_turn_config @@ -478,7 +479,6 @@ impl Session { let mut config = Self::build_per_turn_config(session_configuration, session_configuration.cwd().clone()); config.model = Some(session_configuration.collaboration_mode.model().to_string()); - config.permissions.approval_policy = session_configuration.approval_policy.clone(); config } diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index f91114f740..3c52102659 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -1,9 +1,14 @@ use codex_core::CodexThread; use codex_core::REVIEW_PROMPT; use codex_core::config::Config; +use codex_core::config::Constrained; +use codex_core::find_thread_path_by_id_str; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::items::TurnItem; use codex_protocol::models::ContentItem; +use codex_protocol::models::PermissionProfile; use codex_protocol::models::ResponseItem; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ENVIRONMENT_CONTEXT_OPEN_TAG; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExitedReviewModeEvent; @@ -16,6 +21,7 @@ use codex_protocol::protocol::ReviewRequest; use codex_protocol::protocol::ReviewTarget; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::RolloutLine; +use codex_protocol::protocol::ThreadSettingsOverrides; use codex_protocol::review_format::render_review_output_text; use codex_protocol::user_input::UserInput; use core_test_support::PathBufExt; @@ -549,6 +555,95 @@ async fn review_does_not_emit_agent_message_on_structured_output() { server.verify().await; } +/// Review requests must inherit permissions and reviewer settings updated after session startup. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_uses_updated_turn_permissions_and_approval_policy() { + skip_if_no_network!(); + + let (server, request_log) = + start_responses_server_with_sse(completed_sse(), /*expected_requests*/ 1).await; + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |config| { + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config + .permissions + .set_permission_profile(PermissionProfile::read_only()) + .expect("initial permission profile should be valid"); + }) + .await; + + core_test_support::submit_thread_settings( + &codex, + ThreadSettingsOverrides { + approval_policy: Some(AskForApproval::Never), + approvals_reviewer: Some(ApprovalsReviewer::User), + permission_profile: Some(PermissionProfile::Disabled), + ..Default::default() + }, + ) + .await + .expect("updated thread permissions should be accepted"); + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "review current permissions".to_string(), + }, + user_facing_hint: None, + }, + }) + .await + .expect("review should start"); + wait_for_event(&codex, |event| matches!(event, EventMsg::TurnComplete(_))).await; + + let request = request_log.single_request(); + assert!( + request + .message_input_texts("developer") + .iter() + .any(|text| text.contains("Approval policy is currently never")), + "review should use the updated approval policy" + ); + assert!( + request + .message_input_texts("user") + .iter() + .any(|text| text.contains("")), + "review should use the updated permission profile" + ); + let review_thread_id = request.body_json()["client_metadata"]["thread_id"] + .as_str() + .expect("review request should include its thread ID") + .to_string(); + let review_rollout_path = find_thread_path_by_id_str( + codex_home.path(), + &review_thread_id, + /*state_db_ctx*/ None, + ) + .await + .expect("review rollout lookup should succeed") + .expect("review thread should have a rollout"); + let review_rollout = + std::fs::read_to_string(review_rollout_path).expect("review rollout should be readable"); + let review_approvals_reviewer = review_rollout + .lines() + .filter_map(|line| { + let rollout_line: RolloutLine = + serde_json::from_str(line).expect("review rollout line should be valid"); + match rollout_line.item { + RolloutItem::TurnContext(turn_context) => turn_context.approvals_reviewer, + _ => None, + } + }) + .next_back(); + assert_eq!(review_approvals_reviewer, Some(ApprovalsReviewer::User)); + + let _codex_home_guard = codex_home; + server.verify().await; +} + /// Ensure that when a custom `review_model` is set in the config, the review /// request uses that model (and not the main chat model). #[tokio::test(flavor = "multi_thread", worker_threads = 2)]