diff --git a/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs b/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs index 9c4c3f18ad..a9747d58ab 100644 --- a/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs +++ b/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs @@ -1,5 +1,7 @@ +use codex_protocol::ThreadId; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::ActivePermissionProfile; +use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ThreadSettingsSnapshot; @@ -11,11 +13,61 @@ pub(super) struct PersistedResumeSettings { pub(super) approval_policy: AskForApproval, pub(super) approvals_reviewer: Option, pub(super) active_permission_profile: Option, + pub(super) unnamed_permission_profile: Option, } -pub(super) fn latest_persisted_resume_settings( +impl PersistedResumeSettings { + pub(super) fn from_history(history: &[RolloutItem], thread_id: ThreadId) -> Option { + // A running turn can record stale permissions. Prefer a settings update whose + // owner is known, including a legacy update following that owner's checkpoint. + let mut latest_legacy = None; + for item in history.iter().rev() { + let RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) = item else { + continue; + }; + match event.thread_id { + Some(owner) if owner == thread_id => { + return Some(Self::from(latest_legacy.unwrap_or(&event.thread_settings))); + } + Some(_) => latest_legacy = None, + None => { + latest_legacy.get_or_insert(&event.thread_settings); + } + } + } + latest_persisted_resume_settings(history) + } +} + +impl From<&ThreadSettingsSnapshot> for PersistedResumeSettings { + fn from(settings: &ThreadSettingsSnapshot) -> Self { + Self { + approval_policy: settings.approval_policy, + approvals_reviewer: Some(settings.approvals_reviewer), + active_permission_profile: settings.active_permission_profile.clone(), + unnamed_permission_profile: settings + .active_permission_profile + .is_none() + .then(|| settings.permission_profile.clone()), + } + } +} + +pub(super) fn latest_owned_thread_settings( history: &[RolloutItem], -) -> Option { + thread_id: ThreadId, +) -> Option<&ThreadSettingsSnapshot> { + history.iter().rev().find_map(|item| match item { + RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) + if event.thread_id == Some(thread_id) => + { + Some(&event.thread_settings) + } + _ => None, + }) +} + +fn latest_persisted_resume_settings(history: &[RolloutItem]) -> Option { history .iter() .enumerate() @@ -33,15 +85,17 @@ pub(super) fn latest_persisted_resume_settings( }) }), active_permission_profile: turn_context.active_permission_profile.clone(), + unnamed_permission_profile: None, }), RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => { + // History without an identifiable owner can supply a profile ID, but + // its raw filesystem grants may belong to another thread or executor. + let settings = &event.thread_settings; Some(PersistedResumeSettings { - approval_policy: event.thread_settings.approval_policy, - approvals_reviewer: Some(event.thread_settings.approvals_reviewer), - active_permission_profile: event - .thread_settings - .active_permission_profile - .clone(), + approval_policy: settings.approval_policy, + approvals_reviewer: Some(settings.approvals_reviewer), + active_permission_profile: settings.active_permission_profile.clone(), + unnamed_permission_profile: None, }) } _ => None, diff --git a/codex-rs/app-server/src/request_processors/persisted_resume_settings_tests.rs b/codex-rs/app-server/src/request_processors/persisted_resume_settings_tests.rs index 4531226b58..2a2ea5d3c6 100644 --- a/codex-rs/app-server/src/request_processors/persisted_resume_settings_tests.rs +++ b/codex-rs/app-server/src/request_processors/persisted_resume_settings_tests.rs @@ -1,5 +1,6 @@ use super::PersistedResumeSettings; use super::latest_persisted_resume_settings; +use codex_protocol::ThreadId; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::CollaborationMode; use codex_protocol::config_types::ModeKind; @@ -22,13 +23,14 @@ fn cwd() -> AbsolutePathBuf { } fn settings_item( + thread_id: Option, approval_policy: AskForApproval, approvals_reviewer: ApprovalsReviewer, active_permission_profile: Option, ) -> RolloutItem { RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied( ThreadSettingsAppliedEvent { - thread_id: None, + thread_id, thread_settings: ThreadSettingsSnapshot { disabled_plugin_ids: Vec::new(), model: "gpt-5".to_string(), @@ -90,14 +92,17 @@ fn latest_settings_snapshot_wins() { approval_policy: AskForApproval::OnRequest, approvals_reviewer: Some(ApprovalsReviewer::AutoReview), active_permission_profile: Some(ActivePermissionProfile::new("dev")), + unnamed_permission_profile: None, }; let history = vec![ settings_item( + /*thread_id*/ None, AskForApproval::Never, ApprovalsReviewer::User, /*active_permission_profile*/ None, ), settings_item( + /*thread_id*/ None, AskForApproval::OnRequest, ApprovalsReviewer::AutoReview, Some(ActivePermissionProfile::new("dev")), @@ -113,9 +118,11 @@ fn latest_turn_context_wins_over_earlier_settings_update() { approval_policy: AskForApproval::UnlessTrusted, approvals_reviewer: Some(ApprovalsReviewer::User), active_permission_profile: Some(ActivePermissionProfile::read_only()), + unnamed_permission_profile: None, }; let history = vec![ settings_item( + /*thread_id*/ None, AskForApproval::Never, ApprovalsReviewer::AutoReview, Some(ActivePermissionProfile::new("dev")), @@ -131,6 +138,69 @@ fn latest_turn_context_wins_over_earlier_settings_update() { assert_eq!(latest_persisted_resume_settings(&history), Some(expected)); } +#[test] +fn newer_legacy_permissions_only_supersede_their_own_threads_checkpoint() { + let thread_id = ThreadId::new(); + let other_thread_id = ThreadId::new(); + let workspace = ActivePermissionProfile::new(":workspace"); + let read_only = ActivePermissionProfile::read_only(); + let owned = settings_item( + Some(thread_id), + AskForApproval::Never, + ApprovalsReviewer::AutoReview, + Some(workspace.clone()), + ); + let legacy = settings_item( + /*thread_id*/ None, + AskForApproval::OnRequest, + ApprovalsReviewer::User, + Some(read_only.clone()), + ); + let stale_turn = turn_context_item( + "turn", + AskForApproval::Never, + Some(ApprovalsReviewer::AutoReview), + Some(workspace.clone()), + ); + let foreign = settings_item( + Some(other_thread_id), + AskForApproval::Never, + ApprovalsReviewer::AutoReview, + Some(workspace.clone()), + ); + let foreign_legacy = settings_item( + /*thread_id*/ None, + AskForApproval::Never, + ApprovalsReviewer::AutoReview, + Some(workspace.clone()), + ); + let mut history = vec![owned.clone(), legacy.clone(), stale_turn.clone()]; + let expected = Some(PersistedResumeSettings { + approval_policy: AskForApproval::OnRequest, + approvals_reviewer: Some(ApprovalsReviewer::User), + active_permission_profile: Some(read_only), + unnamed_permission_profile: None, + }); + assert_eq!( + PersistedResumeSettings::from_history(&history, thread_id), + expected + ); + history.extend([foreign.clone(), foreign_legacy]); + assert_eq!( + PersistedResumeSettings::from_history(&history, thread_id), + expected + ); + assert_eq!( + PersistedResumeSettings::from_history(&[owned, foreign, legacy, stale_turn], thread_id), + Some(PersistedResumeSettings { + approval_policy: AskForApproval::Never, + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + active_permission_profile: Some(workspace), + unnamed_permission_profile: None, + }) + ); +} + #[test] fn older_reviewer_is_used_when_latest_turn_context_omits_it() { let history = vec![ @@ -154,6 +224,20 @@ fn older_reviewer_is_used_when_latest_turn_context_omits_it() { approval_policy: AskForApproval::OnRequest, approvals_reviewer: Some(ApprovalsReviewer::AutoReview), active_permission_profile: None, + unnamed_permission_profile: None, }) ); } + +#[test] +fn an_unattributed_snapshot_does_not_restore_unnamed_permissions() { + let history = [settings_item( + /*thread_id*/ None, + AskForApproval::OnRequest, + ApprovalsReviewer::User, + /*active_permission_profile*/ None, + )]; + let settings = + PersistedResumeSettings::from_history(&history, ThreadId::new()).expect("legacy settings"); + assert_eq!(settings.unnamed_permission_profile, None); +} 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 b8907648ba..f5f0665e4e 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -5,8 +5,8 @@ mod daemon_continuation; mod daemon_snapshot; use super::persisted_resume_settings::PersistedResumeSettings; +use super::persisted_resume_settings::latest_owned_thread_settings; use super::persisted_resume_settings::latest_persisted_model_settings; -use super::persisted_resume_settings::latest_persisted_resume_settings; use super::thread_enrichment::enrich_loaded_threads; use super::thread_fork_goal::inherit_thread_goal_snapshot; use super::thread_input::can_accept_direct_input; @@ -38,7 +38,6 @@ use codex_protocol::SanitizedGitUrl; use codex_protocol::config_types::MultiAgentMode; use codex_protocol::error::CodexErrorDetails; use codex_protocol::mcp::ClientMcpExtensions; -use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::MultiAgentVersion; use codex_protocol::protocol::ThreadHistoryMode; use codex_thread_store::PersistContext; @@ -105,7 +104,6 @@ struct ResumeConfigState { workspace_roots: Option>, persisted_metadata: Option, persisted_settings: Option, - persisted_permission_profile: Option, } struct PreparedResumeConfig { @@ -309,6 +307,29 @@ fn has_permission_override( }) } +fn apply_persisted_permission_profile( + settings: &PersistedResumeSettings, + request_overrides: Option<&HashMap>, + config_overrides: &mut ConfigOverrides, +) { + if has_permission_override(request_overrides, config_overrides) { + return; + } + config_overrides.persisted_permission_profile_id = settings + .active_permission_profile + .as_ref() + .map(|profile| profile.id.clone()); + if !request_overrides.is_some_and(|overrides| { + overrides.keys().any(|key| { + key == "sandbox_workspace_write" || key.starts_with("sandbox_workspace_write.") + }) + }) { + config_overrides + .persisted_permission_profile + .clone_from(&settings.unnamed_permission_profile); + } +} + fn validate_dynamic_tools(tools: &[DynamicToolSpec]) -> Result<(), String> { const DYNAMIC_TOOL_NAME_MAX_LEN: usize = 128; const DYNAMIC_TOOL_NAMESPACE_MAX_LEN: usize = 64; @@ -4139,16 +4160,9 @@ impl ThreadRequestProcessor { } // Copied or referenced history can contain another thread's settings. Only snapshots - // explicitly owned by this thread can restore its permissions, cwd, and workspace folders. + // explicitly owned by this thread can restore its 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)) - if event.thread_id == Some(resumed.conversation_id) => - { - Some(&event.thread_settings) - } - _ => None, - }) + latest_owned_thread_settings(&resumed.history, resumed.conversation_id) } else { None }; @@ -4242,27 +4256,12 @@ impl ThreadRequestProcessor { let restore_summary = !has_override("model_reasoning_summary"); let restore_personality = typesafe_overrides.personality.is_none() && !has_override("personality"); - typesafe_overrides.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), + let persisted_settings = match &thread_history { + InitialHistory::Resumed(resumed) => { + PersistedResumeSettings::from_history(&resumed.history, resumed.conversation_id) + } 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, @@ -4281,7 +4280,6 @@ impl ThreadRequestProcessor { workspace_roots: typesafe_overrides.workspace_roots.clone(), persisted_metadata, persisted_settings, - persisted_permission_profile: typesafe_overrides.persisted_permission_profile.clone(), }; let mut config = match prepared_config.take() { Some(prepared) if prepared.state == config_state => prepared.config, @@ -4604,12 +4602,11 @@ impl ThreadRequestProcessor { { typesafe_overrides.approvals_reviewer = persisted_settings.approvals_reviewer; } - if !has_permission_override(request_overrides.as_ref(), typesafe_overrides) { - typesafe_overrides.persisted_permission_profile_id = persisted_settings - .active_permission_profile - .as_ref() - .map(|profile| profile.id.clone()); - } + apply_persisted_permission_profile( + persisted_settings, + request_overrides.as_ref(), + typesafe_overrides, + ); } let state_db_ctx = self.state_db.clone()?; let persisted_metadata = state_db_ctx @@ -5526,11 +5523,7 @@ impl ThreadRequestProcessor { let loaded_parent_settings = if paginated_source && needs_latest_settings { if let Some(parent) = loaded_parent.as_ref() { let snapshot = parent.thread_settings_snapshot().await; - Some(PersistedResumeSettings { - approval_policy: snapshot.approval_policy, - approvals_reviewer: Some(snapshot.approvals_reviewer), - active_permission_profile: snapshot.active_permission_profile, - }) + Some(PersistedResumeSettings::from(&snapshot)) } else { None } @@ -5570,11 +5563,10 @@ impl ThreadRequestProcessor { .and_then(|parent| parent.multi_agent_version()) }); let persisted_settings = loaded_parent_settings.or_else(|| { - latest_persisted_resume_settings( - latest_context - .as_deref() - .unwrap_or_else(|| source_history_items.as_ref()), - ) + let history = latest_context + .as_deref() + .unwrap_or_else(|| source_history_items.as_ref()); + PersistedResumeSettings::from_history(history, source_thread_id) }); if let Some(persisted_settings) = persisted_settings { if restore_approval_policy { @@ -5583,11 +5575,11 @@ impl ThreadRequestProcessor { if restore_approvals_reviewer { typesafe_overrides.approvals_reviewer = persisted_settings.approvals_reviewer; } - if restore_permission_profile { - typesafe_overrides.persisted_permission_profile_id = persisted_settings - .active_permission_profile - .map(|profile| profile.id); - } + apply_persisted_permission_profile( + &persisted_settings, + request_overrides.as_ref(), + &mut typesafe_overrides, + ); } let (inherited_options, inherited_settings) = inherited_fork.unzip(); let options = if let Some(options) = inherited_options { diff --git a/codex-rs/app-server/tests/suite/v2/thread_fork.rs b/codex-rs/app-server/tests/suite/v2/thread_fork.rs index 11678f0961..146cadfda3 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_fork.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_fork.rs @@ -84,6 +84,7 @@ use codex_protocol::protocol::TurnStartedEvent; use codex_protocol::protocol::UserMessageEvent; use codex_rollout::RolloutItem; use codex_rollout::RolloutLine; +use codex_rollout::RolloutRecorder; use codex_rollout::append_rollout_item_to_path; use codex_rollout::append_thread_name; use codex_rollout::read_session_meta_line; @@ -442,7 +443,7 @@ async fn assert_thread_fork_preserves_persisted_approvals_reviewer( let codex_home = TempDir::new()?; MockResponsesConfig::new(&server.uri()).write(codex_home.path())?; - let (source_thread_id, source_turn_id) = { + let (source_thread_id, source_turn_id, rollout_path) = { let mut mcp = TestAppServer::builder() .with_codex_home(codex_home.path()) .build_initialized() @@ -530,9 +531,20 @@ async fn assert_thread_fork_preserves_persisted_approvals_reviewer( ); } - (thread.id, turn.id) + ( + thread.id, + turn.id, + thread.path.expect("source rollout path"), + ) }; + // The first turn records its workspace permissions after the source switched to read-only. + let (items, _, _) = RolloutRecorder::load_rollout_items(&rollout_path).await?; + let frozen_context = items.into_iter().find(|item| { + matches!(item, RolloutItem::TurnContext(context) if context.turn_id.as_deref() == Some(&source_turn_id)) + }).expect("first turn context"); + append_rollout_item_to_path(&rollout_path, &frozen_context).await?; + let mut mcp = TestAppServer::builder() .with_codex_home(codex_home.path()) .build_initialized() 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 9179528428..060522cdb7 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_resume.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_resume.rs @@ -1018,6 +1018,82 @@ sandbox = "unelevated" Ok(()) } +#[tokio::test] +async fn resume_and_fork_restore_unnamed_permissions_from_a_legacy_update() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + MockResponsesConfig::new(&server.uri()) + .with_sandbox_mode("workspace-write") + .write(codex_home.path())?; + let stamp = "2025-01-05T12-00-00"; + let thread_id = create_fake_rollout( + codex_home.path(), + stamp, + "2025-01-05T12:00:00Z", + "Unnamed permissions", + Some("mock_provider"), + /*git_info*/ None, + )?; + let path = rollout_path(codex_home.path(), stamp, &thread_id); + let owned: ThreadSettingsAppliedEvent = serde_json::from_value(json!({ + "thread_id": thread_id, + "thread_settings": { + "model": "gpt-5.5", + "model_provider_id": "mock_provider", + "cwd": normalized_existing_path(codex_home.path())?, + "approval_policy": "never", + "approvals_reviewer": "user", + "permission_profile": PermissionProfile::workspace_write(), + "collaboration_mode": {"mode": "default", "settings": {"model": "gpt-5.5"}}, + }, + }))?; + let mut legacy = owned.clone(); + legacy.thread_id = None; + legacy.thread_settings.permission_profile = PermissionProfile::read_only(); + for event in [owned, legacy] { + append_rollout_item_to_path( + &path, + &RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)), + ) + .await?; + } + + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_managed_config() + .build_initialized() + .await?; + // Fork first so it reads the original history before resume writes a checkpoint. + let fork_id = mcp + .send_thread_fork_request(ThreadForkParams { + thread_id: thread_id.clone(), + ..Default::default() + }) + .await?; + let fork: ThreadForkResponse = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(fork_id)).await??; + let resume_id = mcp + .send_thread_resume_request(ThreadResumeParams { + thread_id, + ..Default::default() + }) + .await?; + let resumed: ThreadResumeResponse = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(resume_id)).await??; + let expected = ( + AppSandboxPolicy::ReadOnly { + network_access: false, + }, + None, + ); + assert_eq!( + (resumed.sandbox, resumed.active_permission_profile), + expected + ); + assert_eq!((fork.sandbox, fork.active_permission_profile), expected); + Ok(()) +} + #[tokio::test] async fn cold_resume_workspace_roots_validate_foreign_startup_paths() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; @@ -1474,6 +1550,9 @@ async fn thread_resume_preserves_acknowledged_model_effort_and_approvals_reviewe .send_thread_start_request_with_auto_env(ThreadStartParams { model: Some("gpt-5.4".to_string()), history_mode: Some(ThreadHistoryMode::Legacy), + approval_policy: Some(AskForApproval::Never), + approvals_reviewer: Some(ApprovalsReviewer::User), + permissions: Some(":workspace".to_string()), ..Default::default() }) .await?; @@ -1516,7 +1595,9 @@ async fn thread_resume_preserves_acknowledged_model_effort_and_approvals_reviewe thread_id: thread.id.clone(), model: Some("gpt-5.2-codex".to_string()), effort: Some(ReasoningEffort::Ultra), + approval_policy: Some(AskForApproval::OnRequest), approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + permissions: Some(":read-only".to_string()), cwd: Some(persisted_cwd.clone()), ..Default::default() }) @@ -1607,7 +1688,7 @@ async fn thread_resume_preserves_acknowledged_model_effort_and_approvals_reviewe .await??; timeout(DEFAULT_READ_TIMEOUT, mcp.shutdown_gracefully()).await??; - // Older rollouts can retain a frozen turn context after an accepted settings update. + // A running turn can record its original permissions after the thread accepts an update. let (items, _, _) = RolloutRecorder::load_rollout_items(&rollout_path).await?; let frozen_context = items .into_iter() @@ -1619,6 +1700,21 @@ async fn thread_resume_preserves_acknowledged_model_effort_and_approvals_reviewe .with_codex_home(codex_home.path()) .build_initialized() .await?; + let fork_id = mcp + .send_thread_fork_request(ThreadForkParams { + thread_id: thread_id.clone(), + ..Default::default() + }) + .await?; + let fork: ThreadForkResponse = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(fork_id)).await??; + assert_eq!(fork.approval_policy, AskForApproval::OnRequest); + assert_eq!(fork.approvals_reviewer, ApprovalsReviewer::AutoReview); + assert_eq!( + fork.active_permission_profile, + Some(ActivePermissionProfile::new(":read-only")) + ); + let resume_id = mcp .send_thread_resume_request(ThreadResumeParams { thread_id: thread_id.clone(), @@ -1630,10 +1726,25 @@ async fn thread_resume_preserves_acknowledged_model_effort_and_approvals_reviewe cwd, reasoning_effort, collaboration_mode, + approval_policy, + approvals_reviewer, + active_permission_profile, .. } = timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(resume_id)).await??; assert_eq!(reasoning_effort, Some(ReasoningEffort::High)); assert_eq!(cwd.as_path(), persisted_cwd); + assert_eq!( + ( + approval_policy, + approvals_reviewer, + active_permission_profile + ), + ( + AskForApproval::OnRequest, + ApprovalsReviewer::AutoReview, + Some(ActivePermissionProfile::new(":read-only")), + ) + ); let (items, _, _) = RolloutRecorder::load_rollout_items(&rollout_path).await?; let mode = items.iter().rev().find_map(|item| match item { RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event))