diff --git a/codex-rs/app-server/src/request_processors/thread_processor.rs b/codex-rs/app-server/src/request_processors/thread_processor.rs index 5b4801a0df..321e3ef892 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -4039,7 +4039,7 @@ impl ThreadRequestProcessor { } // Copied or referenced history can contain another thread's settings. Only snapshots - // explicitly owned by this thread can override its startup cwd and workspace folders. + // explicitly owned by this thread can restore its permissions, cwd, and workspace folders. let history_settings = if let InitialHistory::Resumed(resumed) = &thread_history { resumed.history.iter().rev().find_map(|item| match item { RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) @@ -4131,9 +4131,31 @@ impl ThreadRequestProcessor { } let has_explicit_model_resume_override = has_model_resume_override(request_overrides.as_ref(), &typesafe_overrides); + let persisted_permission_profile = history_settings + .filter(|settings| { + settings.active_permission_profile.is_none() + && !has_permission_override(request_overrides.as_ref(), &typesafe_overrides) + && !request_overrides.as_ref().is_some_and(|overrides| { + overrides.keys().any(|key| { + key == "sandbox_workspace_write" + || key.starts_with("sandbox_workspace_write.") + }) + }) + }) + .map(|settings| settings.permission_profile.clone()); + let mut persisted_settings = match &thread_history { + InitialHistory::Resumed(resumed) => latest_persisted_resume_settings(&resumed.history), + InitialHistory::New | InitialHistory::Cleared | InitialHistory::Forked(_) => None, + }; + // A TurnContext can describe an executor's profile, so an owned snapshot + // is authoritative for named permissions too. + if let (Some(settings), Some(snapshot)) = (&mut persisted_settings, history_settings) { + settings.active_permission_profile = snapshot.active_permission_profile.clone(); + } let persisted_metadata = self .load_and_apply_persisted_resume_metadata( &thread_history, + persisted_settings.as_ref(), &mut request_overrides, &mut typesafe_overrides, ) @@ -4147,12 +4169,7 @@ impl ThreadRequestProcessor { history_cwd: history_cwd.clone(), workspace_roots: typesafe_overrides.workspace_roots.clone(), persisted_metadata, - persisted_settings: match &thread_history { - InitialHistory::Resumed(resumed) => { - latest_persisted_resume_settings(&resumed.history) - } - InitialHistory::New | InitialHistory::Cleared | InitialHistory::Forked(_) => None, - }, + persisted_settings, }; let mut config = match prepared_config.take() { Some(prepared) if prepared.state == config_state => prepared.config, @@ -4181,18 +4198,18 @@ impl ThreadRequestProcessor { match self .thread_manager - .resume_thread_with_history( - config, - thread_history, - self.auth_manager.clone(), - match target { + .start_thread(StartThreadOptions { + initial_history: thread_history, + persisted_permission_profile, + parent_trace: match target { ThreadResumeTarget::Client(request_id) => { self.request_trace_context(request_id).await } ThreadResumeTarget::DaemonRecovery(_) => None, }, client_mcp_extensions, - ) + ..StartThreadOptions::new(config) + }) .await { Ok(NewThread { @@ -4441,14 +4458,14 @@ impl ThreadRequestProcessor { async fn load_and_apply_persisted_resume_metadata( &self, thread_history: &InitialHistory, + persisted_settings: Option<&PersistedResumeSettings>, request_overrides: &mut Option>, typesafe_overrides: &mut ConfigOverrides, ) -> Option { let InitialHistory::Resumed(resumed_history) = thread_history else { return None; }; - if let Some(persisted_settings) = latest_persisted_resume_settings(&resumed_history.history) - { + if let Some(persisted_settings) = persisted_settings { if typesafe_overrides.approval_policy.is_none() { typesafe_overrides.approval_policy = Some(persisted_settings.approval_policy); } @@ -4462,7 +4479,8 @@ impl ThreadRequestProcessor { if !has_permission_override(request_overrides.as_ref(), typesafe_overrides) { typesafe_overrides.persisted_permission_profile_id = persisted_settings .active_permission_profile - .map(|profile| profile.id); + .as_ref() + .map(|profile| profile.id.clone()); } } let state_db_ctx = self.state_db.clone()?; diff --git a/codex-rs/app-server/tests/suite/v2/daemon_update_recovery.rs b/codex-rs/app-server/tests/suite/v2/daemon_update_recovery.rs index 47ea50e7b6..b386ace52f 100644 --- a/codex-rs/app-server/tests/suite/v2/daemon_update_recovery.rs +++ b/codex-rs/app-server/tests/suite/v2/daemon_update_recovery.rs @@ -641,7 +641,7 @@ async fn managed_shutdown_records_interrupted_turn(outcome: &str) -> Result<()> daemon_recovery::write_snapshot(&path, &snapshot)?; } let mut successor = spawn_server(home.path(), &socket)?; - if !matches!(outcome, "running" | "plan") { + if !matches!(outcome, "running" | "plan" | "read_only") { let mut client = connect_default_daemon_client(&socket).await?; timeout(DEFAULT_READ_TIMEOUT, async { loop { diff --git a/codex-rs/app-server/tests/suite/v2/thread_resume.rs b/codex-rs/app-server/tests/suite/v2/thread_resume.rs index 7a2f671ac9..b5f3d24c79 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_resume.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_resume.rs @@ -139,6 +139,7 @@ use std::path::PathBuf; use std::process::Command; use std::time::Duration; use tempfile::TempDir; +use test_case::test_case; use tokio::sync::oneshot; use tokio::time::timeout; use uuid::Uuid; @@ -805,7 +806,18 @@ async fn thread_resume_restores_startup_and_updated_runtime_workspace_roots() -> async fn cold_resume_workspace_roots_honor_owned_snapshot_and_caller_overrides() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - mock_responses_config(&server.uri()).write(codex_home.path())?; + MockResponsesConfig::new(&server.uri()) + .with_sandbox_mode("workspace-write") + .with_extra_config( + r#"[sandbox_workspace_write] +exclude_tmpdir_env_var = true +exclude_slash_tmp = true + +[windows] +sandbox = "unelevated" +"#, + ) + .write(codex_home.path())?; let saved_cwd = normalized_existing_path(codex_home.path())?.abs(); let new_workspace = TempDir::new()?; let new_cwd = normalized_existing_path(new_workspace.path())?.abs(); @@ -829,13 +841,17 @@ async fn cold_resume_workspace_roots_honor_owned_snapshot_and_caller_overrides() "runtime_workspace_roots": [saved_cwd, new_cwd, extra_root], "approval_policy": "never", "approvals_reviewer": "user", - "permission_profile": PermissionProfile::read_only(), + "permission_profile": PermissionProfile::workspace_write() + .materialize_project_roots_with_workspace_roots( + &[saved_cwd.clone(), new_cwd.clone(), extra_root.clone()] + ), "collaboration_mode": { "mode": "default", "settings": { "model": "gpt-5.4" } }, }, }))?; let mut foreign = owned.clone(); foreign.thread_id = Some(ThreadId::new()); foreign.thread_settings.runtime_workspace_roots = Some(vec![extra_root.clone()]); + foreign.thread_settings.permission_profile = PermissionProfile::Disabled; for settings in [owned, foreign] { append_rollout_item_to_path( &path, @@ -843,7 +859,6 @@ async fn cold_resume_workspace_roots_honor_owned_snapshot_and_caller_overrides() ) .await?; } - // Every iteration is a cold resume; the final one must retain the preceding explicit clear. for (cwd_override, roots_override, expected_cwd, expected_roots) in [ ( @@ -864,6 +879,7 @@ async fn cold_resume_workspace_roots_honor_owned_snapshot_and_caller_overrides() let mut mcp = TestAppServer::builder() .with_codex_home(codex_home.path()) .without_auto_env() + .without_managed_config() .build_initialized() .await?; let resume_id = mcp @@ -891,14 +907,25 @@ async fn cold_resume_workspace_roots_honor_owned_snapshot_and_caller_overrides() Some(( event.thread_settings.cwd, event.thread_settings.runtime_workspace_roots, + event.thread_settings.permission_profile, )) } _ => None, }); + let (persisted_cwd, persisted_roots, permission_profile) = + persisted.expect("resumed thread settings"); assert_eq!( - persisted, - Some((expected_cwd.clone(), Some(expected_roots))) + (persisted_cwd, persisted_roots), + (expected_cwd.clone(), Some(expected_roots.clone())) ); + let file_system = permission_profile.file_system_sandbox_policy(); + for root in [&saved_cwd, &new_cwd, &extra_root] { + assert_eq!( + file_system.can_write_local_path_with_cwd(root.as_path(), expected_cwd.as_path()), + expected_roots.contains(root), + "resumed write permission at {root:?}" + ); + } timeout(DEFAULT_READ_TIMEOUT, mcp.shutdown_gracefully()).await??; } Ok(()) @@ -1788,23 +1815,59 @@ async fn cold_resume_permission_overrides_win_over_persisted_profile() -> Result Ok(()) } +// Unelevated Windows sandboxes cannot enforce managed deny-read. +#[cfg_attr(unix, test_case( + "[permissions.filesystem]\ndeny_read = [\"./private\"]", + AppSandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + network_access: false, + exclude_tmpdir_env_var: false, + exclude_slash_tmp: false, + }; + "managed deny-read" +))] +#[test_case("", AppSandboxPolicy::DangerFullAccess; "restore saved full access")] +#[test_case( + "allowed_sandbox_modes = [\"read-only\"]", + AppSandboxPolicy::ReadOnly { network_access: false }; + "managed modes" +)] +#[test_case( + "default_permissions = \":read-only\"\n[allowed_permission_profiles]\n\":read-only\" = true", + AppSandboxPolicy::ReadOnly { network_access: false }; + "managed catalog" +)] #[tokio::test] -async fn cold_resume_without_active_permission_profile_uses_current_config() -> Result<()> { +async fn cold_resume_without_active_permission_profile_restores_base_permissions( + requirements: &str, + expected: AppSandboxPolicy, +) -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; mock_responses_config(&server.uri()).write(codex_home.path())?; + // Only deny-read names a path on the app-server host. + let local = requirements.contains("deny_read"); let thread_id = { - let mut mcp = TestAppServer::builder() + let mut builder = TestAppServer::builder() .with_codex_home(codex_home.path()) - .without_managed_config() - .build_initialized() - .await?; - let ThreadStartResponse { thread, .. } = mcp - .start_thread(ThreadStartParams { - model: Some("mock-model".to_string()), - ..Default::default() - }) - .await?; + .without_managed_config(); + if local { + builder = builder.without_auto_env(); + } + let mut mcp = builder.build_initialized().await?; + let params = ThreadStartParams { + model: Some("mock-model".to_string()), + sandbox: Some(SandboxMode::DangerFullAccess), + approval_policy: Some(AskForApproval::Never), + ..Default::default() + }; + let request_id = if local { + mcp.send_thread_start_request(params).await? + } else { + mcp.send_thread_start_request_with_auto_env(params).await? + }; + let ThreadStartResponse { thread, .. } = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(request_id)).await??; timeout( DEFAULT_READ_TIMEOUT, mcp.start_turn_and_wait_for_completion(TurnStartParams { @@ -1813,7 +1876,6 @@ async fn cold_resume_without_active_permission_profile_uses_current_config() -> text: "persist full access".to_string(), text_elements: Vec::new(), }], - sandbox_policy: Some(AppSandboxPolicy::DangerFullAccess), ..Default::default() }), ) @@ -1826,29 +1888,24 @@ async fn cold_resume_without_active_permission_profile_uses_current_config() -> "default_permissions = \"{BUILT_IN_PERMISSION_PROFILE_WORKSPACE}\"" )) .write(codex_home.path())?; - let mut mcp = TestAppServer::builder() - .with_codex_home(codex_home.path()) - .without_managed_config() - .build_initialized() - .await?; + std::fs::write(codex_home.path().join("requirements.toml"), requirements)?; + let mut builder = TestAppServer::builder().with_codex_home(codex_home.path()); + if local { + builder = builder.without_auto_env(); + } + let mut mcp = builder.build_initialized().await?; let resume_id = mcp .send_thread_resume_request(ThreadResumeParams { thread_id, ..Default::default() }) .await?; - let ThreadResumeResponse { - sandbox, - active_permission_profile, - .. - } = timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(resume_id)).await??; - - assert!(matches!(sandbox, AppSandboxPolicy::WorkspaceWrite { .. })); + let response: ThreadResumeResponse = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(resume_id)).await??; assert_eq!( - active_permission_profile, - Some(ActivePermissionProfile::new( - BUILT_IN_PERMISSION_PROFILE_WORKSPACE - )) + (response.sandbox, response.approval_policy), + (expected, AskForApproval::Never), + "requirements: {requirements}" ); Ok(()) } diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index e2c30ad988..8029c1e011 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -124,6 +124,7 @@ pub(crate) async fn run_codex_thread_interactive( history_initialization, disabled_plugin_ids: (isolation == codex_extension_api::SessionIsolation::Inherit) .then(|| parent_ctx.disabled_plugin_ids.clone()), + persisted_permission_profile: None, requested_history_mode: None, fork_persistence: ForkPersistence::Copied, session_source, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index e9148a522f..1bc14e7981 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -137,6 +137,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::models::SandboxEnforcement; use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelPreset; +use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::permissions::NetworkSandboxPolicy; @@ -459,6 +460,7 @@ pub(crate) struct SessionSpawnArgs { pub(crate) conversation_history: InitialHistory, pub(crate) history_initialization: HistoryInitialization, pub(crate) disabled_plugin_ids: Option>, + pub(crate) persisted_permission_profile: Option, pub(crate) requested_history_mode: Option, pub(crate) fork_persistence: ForkPersistence, pub(crate) session_source: SessionSource, @@ -574,6 +576,7 @@ impl Session { conversation_history, history_initialization, disabled_plugin_ids, + persisted_permission_profile, requested_history_mode, fork_persistence, session_source, @@ -869,7 +872,7 @@ impl Session { let service_tier = get_service_tier(config.service_tier.clone(), &config.features, &model_info); let storage_originator = AuthStorageOriginator::from_client_name(&originator); - let session_configuration = SessionConfiguration { + let mut session_configuration = SessionConfiguration { turn_extension_init, provider: create_model_provider( config.model_provider.clone(), @@ -912,6 +915,35 @@ impl Session { dynamic_tools, user_shell_override, }; + // Restore the thread's base policy, not an executor's enforcement or + // materialized grants that might outlive the workspace they came from. + if let Some(profile) = persisted_permission_profile + && !matches!(profile, PermissionProfile::External { .. }) + && !profile.file_system_sandbox_policy().entries.iter().any(|entry| { + entry.access.can_read() + && matches!( + entry.path, + FileSystemPath::Path { .. } | FileSystemPath::GlobPattern { .. } + ) + }) + && config + .config_layer_stack + .requirements_toml() + .allowed_permission_profiles + .is_none() + // Reject a disallowed saved mode before the live-update path can + // narrow it into a different policy (for example, Full + denies). + && config.permissions.can_set_permission_profile(&profile).is_ok() + && let Ok(restored) = session_configuration.apply( + &SessionSettingsUpdate { + permission_profile: Some(profile), + ..Default::default() + }, + &environment_selections, + ) + { + session_configuration = restored; + } session_configuration .validate(&environment_selections) .map_err(|err| CodexErr::InvalidRequest(err.to_string()))?; @@ -4804,8 +4836,21 @@ impl Session { } // Persist one `TurnContextItem` per real user turn so resume/lazy replay can recover the // latest durable baseline even when this turn emitted no model-visible context diffs. - self.persist_rollout_items(&[RolloutItem::TurnContext(turn_context_item.clone())]) - .await; + // Checkpoint current thread-owned settings after the first turn's context: + // the step may predate an acknowledged update or use executor-owned permissions. + // Serialize with settings updates so an older checkpoint cannot supersede a newer one. + let _settings_guard = if should_inject_full_context { + Some(thread_settings::acquire_persistence_lock(self).await) + } else { + None + }; + let mut rollout_items = vec![RolloutItem::TurnContext(turn_context_item.clone())]; + if should_inject_full_context { + rollout_items.push(RolloutItem::EventMsg( + thread_settings::applied_event(self).await, + )); + } + self.persist_rollout_items(&rollout_items).await; // Advance the persisted-settings baseline even when this turn emitted no model-visible // context items. diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index ab9a2be02e..1d19731835 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -1305,6 +1305,7 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() { conversation_history: InitialHistory::New, history_initialization: HistoryInitialization::New, disabled_plugin_ids: None, + persisted_permission_profile: None, requested_history_mode: None, fork_persistence: ForkPersistence::Copied, session_source: SessionSource::SubAgent(SubAgentSource::Other( diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index b6931ad555..a0b6f169d5 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -304,6 +304,9 @@ pub struct StartThreadOptions { pub reserved_thread_id: Option, /// Initial thread-owned plugin selection; omission restores persisted settings. pub disabled_plugin_ids: Option>, + /// Unnamed permissions recovered from an owned rollout snapshot on resume. + /// The session applies these only if current managed requirements allow them. + pub persisted_permission_profile: Option, } impl StartThreadOptions { @@ -328,6 +331,7 @@ impl StartThreadOptions { client_mcp_extensions: ClientMcpExtensions::default(), reserved_thread_id: None, disabled_plugin_ids: None, + persisted_permission_profile: None, } } } @@ -2100,6 +2104,7 @@ impl ThreadManagerState { client_mcp_extensions, mut reserved_thread_id, disabled_plugin_ids, + persisted_permission_profile, } = options; let inherited_environments = captured_environments.or(inherited_environments); let session_source = session_source.unwrap_or_else(|| self.session_source.clone()); @@ -2320,6 +2325,7 @@ impl ThreadManagerState { conversation_history: initial_history, history_initialization, disabled_plugin_ids, + persisted_permission_profile, requested_history_mode: history_mode, fork_persistence, // Keep only the manager registration internal. The session and its saved diff --git a/codex-rs/core/tests/suite/settings_commits.rs b/codex-rs/core/tests/suite/settings_commits.rs index 0c341aed11..823a1b50de 100644 --- a/codex-rs/core/tests/suite/settings_commits.rs +++ b/codex-rs/core/tests/suite/settings_commits.rs @@ -8,6 +8,7 @@ use codex_extension_api::ExtensionData; use codex_extension_api::ExtensionRegistryBuilder; use codex_history::RolloutItem; use codex_history::RolloutLine; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; use codex_protocol::protocol::ThreadHistoryMode; @@ -62,7 +63,7 @@ fn assert_checkpoints(test: &TestCodex, expected: &[ThreadSettingsSnapshot]) -> #[test_case(ThreadHistoryMode::Legacy; "legacy")] #[test_case(ThreadHistoryMode::Paginated; "paginated")] #[tokio::test] -async fn initial_plugin_ids_use_turn_context_without_extra_settings_checkpoints( +async fn initial_settings_include_plugin_ids_without_duplicate_checkpoints( history_mode: ThreadHistoryMode, ) -> Result<()> { skip_if_no_network!(Ok(())); @@ -95,7 +96,8 @@ async fn initial_plugin_ids_use_turn_context_without_extra_settings_checkpoints( ) .await; test.codex.flush_rollout().await?; - assert_checkpoints(&test, &[])?; + let mut expected = vec![test.codex.thread_settings_snapshot().await]; + assert_checkpoints(&test, &expected)?; let rollout_path = test.codex.rollout_path().expect("rollout path"); let (items, _, parse_errors) = codex_rollout::RolloutRecorder::load_rollout_items(&rollout_path).await?; @@ -124,8 +126,9 @@ async fn initial_plugin_ids_use_turn_context_without_extra_settings_checkpoints( }, ) .await?; - let expected = vec![test.codex.thread_settings_snapshot().await]; - assert!(expected[0].disabled_plugin_ids.is_empty()); + let cleared = test.codex.thread_settings_snapshot().await; + assert!(cleared.disabled_plugin_ids.is_empty()); + expected.push(cleared); test.codex.flush_rollout().await?; assert_checkpoints(&test, &expected)?; let response = @@ -203,6 +206,7 @@ async fn settings_notifications_keep_their_commit_across_postcommit_work( initial.disabled_plugin_ids = None; let thread_settings = ThreadSettingsOverrides { model: Some(COMMITTED_MODEL.to_string()), + approval_policy: Some(AskForApproval::Never), disabled_plugin_ids: Some(vec!["slack@openai".to_string()]), ..Default::default() }; @@ -289,6 +293,25 @@ async fn settings_notifications_keep_their_commit_across_postcommit_work( expected_request_model ); assert_eq!(test.codex.thread_settings_snapshot().await, restored); + test.codex.flush_rollout().await?; + let rollout_path = test.codex.rollout_path().expect("rollout path"); + let (items, _, parse_errors) = + codex_rollout::RolloutRecorder::load_rollout_items(&rollout_path).await?; + assert_eq!(parse_errors, 0); + let latest_settings = items.iter().rev().find_map(|item| match item { + RolloutItem::TurnContext(context) => { + Some((context.model.as_str(), context.approval_policy)) + } + RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(applied)) => Some(( + applied.thread_settings.model.as_str(), + applied.thread_settings.approval_policy, + )), + _ => None, + }); + assert_eq!( + latest_settings, + Some((restored.model.as_str(), restored.approval_policy)) + ); Ok(()) }