mirror of
https://github.com/openai/codex.git
synced 2026-10-21 05:22:21 +00:00
Restore approval policy when resuming threads (#37368)
## What changed - On cold `thread/resume`, restore the latest approval policy from persisted thread settings instead of falling back to the current configured default. - Preserve the precedence of an explicit request override over the persisted policy. - Account for settings applied during a turn when the subsequent compacted turn context contains a stale policy. ## Testing - Add unit coverage for selecting the latest persisted policy. - Add app-server tests for legacy and paginated history, including explicit resume overrides. GitOrigin-RevId: 9127a3c53a90f16d67957ded9e7430e79180fc60
This commit is contained in:
@@ -347,7 +347,7 @@ Start a fresh thread when you need a new Codex conversation.
|
||||
|
||||
Valid `personality` values are `"friendly"`, `"pragmatic"`, and `"none"`. When `"none"` is selected, the personality placeholder is replaced with an empty string.
|
||||
|
||||
To continue a stored session, call `thread/resume` with the `thread.id` you previously recorded. The response shape matches `thread/start`. When the stored session includes persisted token usage, the server emits `thread/tokenUsage/updated` immediately after the response so clients can render restored usage before the next turn starts. You can also pass the same configuration overrides supported by `thread/start`, including `approvalsReviewer`.
|
||||
To continue a stored session, call `thread/resume` with the `thread.id` you previously recorded. The response shape matches `thread/start`. When the stored session includes persisted token usage, the server emits `thread/tokenUsage/updated` immediately after the response so clients can render restored usage before the next turn starts. You can also pass the same configuration overrides supported by `thread/start`, including `approvalsReviewer`. On cold resume, approval policy uses the first allowed value in this order: request override, latest persisted thread setting, current configured default.
|
||||
|
||||
By default, `thread/resume` includes the reconstructed turn history in `thread.turns`. Experimental clients can pass `excludeTurns: true` to return only thread metadata and live resume state, then call `thread/turns/list` separately if they want to page the turn history over the network. A cold paginated resume can still replay persisted `thread/tokenUsage/updated` when it can identify the corresponding stored turn; resuming an already-loaded thread waits for the next live update.
|
||||
|
||||
|
||||
@@ -194,6 +194,42 @@ fn merge_persisted_approvals_reviewer(
|
||||
});
|
||||
}
|
||||
|
||||
fn latest_persisted_approval_policy(
|
||||
history: &[RolloutItem],
|
||||
) -> Option<codex_protocol::protocol::AskForApproval> {
|
||||
history
|
||||
.iter()
|
||||
.enumerate()
|
||||
.rev()
|
||||
.find_map(|(index, item)| match item {
|
||||
RolloutItem::TurnContext(turn_context) => {
|
||||
let updated_policy = turn_context.turn_id.as_ref().and_then(|turn_id| {
|
||||
let turn_start = history[..index].iter().rposition(|item| {
|
||||
matches!(
|
||||
item,
|
||||
RolloutItem::EventMsg(EventMsg::TurnStarted(event))
|
||||
if &event.turn_id == turn_id
|
||||
)
|
||||
})?;
|
||||
history[turn_start + 1..index]
|
||||
.iter()
|
||||
.rev()
|
||||
.find_map(|item| match item {
|
||||
RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => {
|
||||
Some(event.thread_settings.approval_policy)
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
});
|
||||
Some(updated_policy.unwrap_or(turn_context.approval_policy))
|
||||
}
|
||||
RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => {
|
||||
Some(event.thread_settings.approval_policy)
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
}
|
||||
|
||||
fn normalize_thread_list_cwd_filters(
|
||||
cwd: Option<ThreadListCwdFilter>,
|
||||
) -> Result<Option<Vec<PathBuf>>, JSONRPCErrorError> {
|
||||
@@ -3190,6 +3226,27 @@ impl ThreadRequestProcessor {
|
||||
developer_instructions,
|
||||
personality,
|
||||
);
|
||||
if typesafe_overrides.approval_policy.is_none()
|
||||
&& let Some(value) = request_overrides
|
||||
.as_mut()
|
||||
.and_then(|overrides| overrides.remove("approval_policy"))
|
||||
{
|
||||
let approval_policy = match serde_json::from_value(value) {
|
||||
Ok(approval_policy) => approval_policy,
|
||||
Err(err) => {
|
||||
self.outgoing
|
||||
.send_error(
|
||||
request_id,
|
||||
invalid_params(format!(
|
||||
"invalid `approval_policy` config override: {err}"
|
||||
)),
|
||||
)
|
||||
.await;
|
||||
return Ok(());
|
||||
}
|
||||
};
|
||||
typesafe_overrides.approval_policy = Some(approval_policy);
|
||||
}
|
||||
let has_explicit_model_resume_override =
|
||||
has_model_resume_override(request_overrides.as_ref(), &typesafe_overrides);
|
||||
let persisted_metadata = self
|
||||
@@ -3470,6 +3527,10 @@ impl ThreadRequestProcessor {
|
||||
request_overrides.as_ref(),
|
||||
typesafe_overrides,
|
||||
);
|
||||
if typesafe_overrides.approval_policy.is_none() {
|
||||
typesafe_overrides.approval_policy =
|
||||
latest_persisted_approval_policy(&resumed_history.history);
|
||||
}
|
||||
let state_db_ctx = self.state_db.clone()?;
|
||||
let persisted_metadata = state_db_ctx
|
||||
.get_thread(resumed_history.conversation_id)
|
||||
|
||||
@@ -36,6 +36,136 @@ mod thread_list_cwd_filter_tests {
|
||||
}
|
||||
}
|
||||
|
||||
mod persisted_resume_approval_policy_tests {
|
||||
use super::super::latest_persisted_approval_policy;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::config_types::CollaborationMode;
|
||||
use codex_protocol::config_types::ModeKind;
|
||||
use codex_protocol::config_types::Settings;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::RolloutItem;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::protocol::ThreadSettingsAppliedEvent;
|
||||
use codex_protocol::protocol::ThreadSettingsSnapshot;
|
||||
use codex_protocol::protocol::TurnContextItem;
|
||||
use codex_protocol::protocol::TurnStartedEvent;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
fn cwd() -> AbsolutePathBuf {
|
||||
AbsolutePathBuf::try_from(std::env::current_dir().expect("current directory"))
|
||||
.expect("absolute current directory")
|
||||
}
|
||||
|
||||
fn settings_item(approval_policy: AskForApproval) -> RolloutItem {
|
||||
RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(
|
||||
ThreadSettingsAppliedEvent {
|
||||
thread_settings: ThreadSettingsSnapshot {
|
||||
model: "gpt-5".to_string(),
|
||||
model_provider_id: "openai".to_string(),
|
||||
service_tier: None,
|
||||
approval_policy,
|
||||
approvals_reviewer: ApprovalsReviewer::User,
|
||||
permission_profile: PermissionProfile::read_only(),
|
||||
active_permission_profile: None,
|
||||
cwd: cwd(),
|
||||
reasoning_effort: None,
|
||||
reasoning_summary: None,
|
||||
personality: None,
|
||||
collaboration_mode: CollaborationMode {
|
||||
mode: ModeKind::Default,
|
||||
settings: Settings {
|
||||
model: "gpt-5".to_string(),
|
||||
reasoning_effort: None,
|
||||
developer_instructions: None,
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
))
|
||||
}
|
||||
|
||||
fn turn_started_item(turn_id: &str) -> RolloutItem {
|
||||
RolloutItem::EventMsg(EventMsg::TurnStarted(TurnStartedEvent {
|
||||
turn_id: turn_id.to_string(),
|
||||
trace_id: None,
|
||||
started_at: None,
|
||||
model_context_window: None,
|
||||
collaboration_mode_kind: ModeKind::Default,
|
||||
}))
|
||||
}
|
||||
|
||||
fn turn_context_item(turn_id: &str, approval_policy: AskForApproval) -> RolloutItem {
|
||||
RolloutItem::TurnContext(TurnContextItem {
|
||||
turn_id: Some(turn_id.to_string()),
|
||||
cwd: cwd(),
|
||||
workspace_roots: None,
|
||||
current_date: None,
|
||||
timezone: None,
|
||||
approval_policy,
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: "gpt-5".to_string(),
|
||||
comp_hash: None,
|
||||
personality: None,
|
||||
collaboration_mode: None,
|
||||
multi_agent_version: None,
|
||||
multi_agent_mode: None,
|
||||
realtime_active: None,
|
||||
effort: None,
|
||||
summary: codex_protocol::config_types::ReasoningSummary::Auto,
|
||||
})
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn latest_settings_snapshot_wins() {
|
||||
let history = vec![
|
||||
settings_item(AskForApproval::Never),
|
||||
settings_item(AskForApproval::OnRequest),
|
||||
];
|
||||
|
||||
assert_eq!(
|
||||
latest_persisted_approval_policy(&history),
|
||||
Some(AskForApproval::OnRequest)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn settings_applied_during_turn_wins_over_stale_compaction_context() {
|
||||
let history = vec![
|
||||
turn_started_item("turn-1"),
|
||||
settings_item(AskForApproval::Never),
|
||||
turn_context_item("turn-1", AskForApproval::OnRequest),
|
||||
];
|
||||
|
||||
assert_eq!(
|
||||
latest_persisted_approval_policy(&history),
|
||||
Some(AskForApproval::Never)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn later_turn_context_wins_over_earlier_settings_update() {
|
||||
let history = vec![
|
||||
turn_started_item("turn-1"),
|
||||
settings_item(AskForApproval::Never),
|
||||
turn_context_item("turn-1", AskForApproval::OnRequest),
|
||||
turn_started_item("turn-2"),
|
||||
turn_context_item("turn-2", AskForApproval::OnRequest),
|
||||
];
|
||||
|
||||
assert_eq!(
|
||||
latest_persisted_approval_policy(&history),
|
||||
Some(AskForApproval::OnRequest)
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
mod background_terminal_pagination_tests {
|
||||
use super::super::paginate_background_terminals;
|
||||
use codex_app_server_protocol::ThreadBackgroundTerminal;
|
||||
|
||||
@@ -716,6 +716,134 @@ async fn thread_resume_preserves_persisted_approvals_reviewer() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_restores_persisted_approval_policy() -> Result<()> {
|
||||
assert_thread_resume_approval_policy(
|
||||
ThreadHistoryMode::Legacy,
|
||||
/*approval_policy*/ None,
|
||||
AskForApproval::Never,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn paginated_thread_resume_restores_persisted_approval_policy() -> Result<()> {
|
||||
assert_thread_resume_approval_policy(
|
||||
ThreadHistoryMode::Paginated,
|
||||
/*approval_policy*/ None,
|
||||
AskForApproval::Never,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_approval_policy_override_wins_over_persisted_policy() -> Result<()> {
|
||||
assert_thread_resume_approval_policy(
|
||||
ThreadHistoryMode::Legacy,
|
||||
Some(AskForApproval::OnRequest),
|
||||
AskForApproval::OnRequest,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn assert_thread_resume_approval_policy(
|
||||
history_mode: ThreadHistoryMode,
|
||||
approval_policy: Option<AskForApproval>,
|
||||
expected_approval_policy: AskForApproval,
|
||||
) -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
let config_path = codex_home.path().join("config.toml");
|
||||
std::fs::write(
|
||||
&config_path,
|
||||
format!(
|
||||
r#"
|
||||
model = "gpt-5.4"
|
||||
approval_policy = "never"
|
||||
model_provider = "mock_provider"
|
||||
|
||||
[model_providers.mock_provider]
|
||||
name = "Mock provider for test"
|
||||
base_url = "{}/v1"
|
||||
wire_api = "responses"
|
||||
request_max_retries = 0
|
||||
stream_max_retries = 0
|
||||
"#,
|
||||
server.uri()
|
||||
),
|
||||
)?;
|
||||
|
||||
let thread_id = {
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.without_auto_env()
|
||||
.build_initialized()
|
||||
.await?;
|
||||
|
||||
let start_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
model: Some("gpt-5.4".to_string()),
|
||||
history_mode: Some(history_mode),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let ThreadStartResponse { thread, .. } =
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(start_id)).await??;
|
||||
|
||||
let turn_id = mcp
|
||||
.send_turn_start_request(TurnStartParams {
|
||||
thread_id: thread.id.clone(),
|
||||
client_user_message_id: None,
|
||||
input: vec![UserInput::Text {
|
||||
text: "materialize this thread".to_string(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(turn_id)),
|
||||
)
|
||||
.await??;
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("turn/completed"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
thread.id
|
||||
};
|
||||
|
||||
let config = std::fs::read_to_string(&config_path)?;
|
||||
std::fs::write(
|
||||
config_path,
|
||||
config.replace(
|
||||
"approval_policy = \"never\"",
|
||||
"approval_policy = \"on-request\"",
|
||||
),
|
||||
)?;
|
||||
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.without_auto_env()
|
||||
.build_initialized()
|
||||
.await?;
|
||||
let resume_id = mcp
|
||||
.send_thread_resume_request(ThreadResumeParams {
|
||||
thread_id,
|
||||
approval_policy,
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let ThreadResumeResponse {
|
||||
approval_policy, ..
|
||||
} = timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(resume_id)).await??;
|
||||
|
||||
assert_eq!(approval_policy, expected_approval_policy);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_preserves_goal_first_and_fork_approvals_reviewer() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
|
||||
Reference in New Issue
Block a user