Preserve acknowledged permissions when resuming and forking threads (#57227)

## Why

A running turn can record stale permissions after the thread accepts a settings update. Resuming or forking from that history should preserve the accepted approval and permission settings.

## What changed

- Prefer settings snapshots owned by the source thread over stale turn contexts, including later legacy updates associated with that thread's checkpoint.
- Restore unnamed permission profiles from attributable settings updates while honoring explicit request overrides. Avoid restoring raw permission grants from history without an identifiable owner.
- Share permission restoration logic between resume and fork.

## Testing

Add regression coverage for stale turn contexts, legacy updates across thread ownership boundaries, and unnamed permission restoration on resume and fork.

GitOrigin-RevId: 450bc1688be78c7ec685f6dc56bcc5e5dd2d130a
This commit is contained in:
Charlie Marsh
2026-10-26 20:17:38 +00:00
committed by copyberry
parent ca8b7a0737
commit 3047741b63
5 changed files with 318 additions and 65 deletions
@@ -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<ApprovalsReviewer>,
pub(super) active_permission_profile: Option<ActivePermissionProfile>,
pub(super) unnamed_permission_profile: Option<PermissionProfile>,
}
pub(super) fn latest_persisted_resume_settings(
impl PersistedResumeSettings {
pub(super) fn from_history(history: &[RolloutItem], thread_id: ThreadId) -> Option<Self> {
// 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<PersistedResumeSettings> {
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<PersistedResumeSettings> {
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,
@@ -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<ThreadId>,
approval_policy: AskForApproval,
approvals_reviewer: ApprovalsReviewer,
active_permission_profile: Option<ActivePermissionProfile>,
) -> 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);
}
@@ -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<Vec<AbsolutePathBuf>>,
persisted_metadata: Option<ThreadMetadata>,
persisted_settings: Option<PersistedResumeSettings>,
persisted_permission_profile: Option<PermissionProfile>,
}
struct PreparedResumeConfig {
@@ -309,6 +307,29 @@ fn has_permission_override(
})
}
fn apply_persisted_permission_profile(
settings: &PersistedResumeSettings,
request_overrides: Option<&HashMap<String, serde_json::Value>>,
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 {
@@ -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()
@@ -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))