mirror of
https://github.com/openai/codex.git
synced 2026-09-29 16:57:06 +08:00
[codex] Preserve reviewer when resuming threads (#30278)
## Why A thread resumed without an explicit reviewer could pick up the reviewer from the current config instead of preserving the reviewer already in use by the thread. After an app restart, this meant a thread running with auto review could silently switch back to user review, and the next turn could continue under the wrong reviewer. ## What changed Persist the effective reviewer with each turn and restore the latest persisted value when the thread resumes. If the resume request explicitly provides a reviewer, that value still takes precedence. ## Test plan - Added a regression test that starts a thread with auto review, records a turn, restarts with user review in config, resumes without an override, and verifies that auto review is preserved. - `just test -p codex-protocol` - `just test -p codex-state` - `just test -p codex-rollout` - `just test -p codex-app-server thread_resume_preserves_persisted_approvals_reviewer` - Clippy for the affected crates
This commit is contained in:
@@ -164,6 +164,34 @@ fn merge_persisted_resume_metadata(
|
||||
}
|
||||
}
|
||||
|
||||
fn merge_persisted_approvals_reviewer(
|
||||
thread_history: &InitialHistory,
|
||||
request_overrides: Option<&HashMap<String, serde_json::Value>>,
|
||||
typesafe_overrides: &mut ConfigOverrides,
|
||||
) {
|
||||
if typesafe_overrides.approvals_reviewer.is_some()
|
||||
|| request_overrides.is_some_and(|overrides| overrides.contains_key("approvals_reviewer"))
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
let InitialHistory::Resumed(resumed_history) = thread_history else {
|
||||
return;
|
||||
};
|
||||
typesafe_overrides.approvals_reviewer =
|
||||
resumed_history
|
||||
.history
|
||||
.iter()
|
||||
.rev()
|
||||
.find_map(|item| match item {
|
||||
RolloutItem::TurnContext(turn_context) => turn_context.approvals_reviewer,
|
||||
RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => {
|
||||
Some(event.thread_settings.approvals_reviewer)
|
||||
}
|
||||
_ => None,
|
||||
});
|
||||
}
|
||||
|
||||
fn normalize_thread_list_cwd_filters(
|
||||
cwd: Option<ThreadListCwdFilter>,
|
||||
) -> Result<Option<Vec<PathBuf>>, JSONRPCErrorError> {
|
||||
@@ -2957,6 +2985,11 @@ impl ThreadRequestProcessor {
|
||||
request_overrides: &mut Option<HashMap<String, serde_json::Value>>,
|
||||
typesafe_overrides: &mut ConfigOverrides,
|
||||
) -> Option<ThreadMetadata> {
|
||||
merge_persisted_approvals_reviewer(
|
||||
thread_history,
|
||||
request_overrides.as_ref(),
|
||||
typesafe_overrides,
|
||||
);
|
||||
let InitialHistory::Resumed(resumed_history) = thread_history else {
|
||||
return None;
|
||||
};
|
||||
|
||||
@@ -14,6 +14,7 @@ use app_test_support::test_absolute_path;
|
||||
use app_test_support::to_response;
|
||||
use app_test_support::write_chatgpt_auth;
|
||||
use chrono::Utc;
|
||||
use codex_app_server_protocol::ApprovalsReviewer;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
use codex_app_server_protocol::ClientInfo;
|
||||
use codex_app_server_protocol::CommandExecutionApprovalDecision;
|
||||
@@ -42,6 +43,8 @@ use codex_app_server_protocol::ThreadReadResponse;
|
||||
use codex_app_server_protocol::ThreadResumeInitialTurnsPageParams;
|
||||
use codex_app_server_protocol::ThreadResumeParams;
|
||||
use codex_app_server_protocol::ThreadResumeResponse;
|
||||
use codex_app_server_protocol::ThreadSettingsUpdateParams;
|
||||
use codex_app_server_protocol::ThreadSettingsUpdateResponse;
|
||||
use codex_app_server_protocol::ThreadSource;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_app_server_protocol::ThreadStartResponse;
|
||||
@@ -436,6 +439,192 @@ async fn turn_start_updates_runtime_workspace_roots_for_loaded_thread() -> Resul
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_preserves_persisted_approvals_reviewer() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml(codex_home.path(), &server.uri())?;
|
||||
|
||||
let thread_id = {
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.without_auto_env()
|
||||
.build()
|
||||
.await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let start_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
model: Some("gpt-5.4".to_string()),
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let start_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(start_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadStartResponse { thread, .. } = to_response::<ThreadStartResponse>(start_resp)?;
|
||||
|
||||
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_path = codex_home.path().join("config.toml");
|
||||
let config = std::fs::read_to_string(&config_path)?;
|
||||
std::fs::write(
|
||||
config_path,
|
||||
config.replace(
|
||||
"approval_policy = \"never\"\n",
|
||||
"approval_policy = \"never\"\napprovals_reviewer = \"user\"\n",
|
||||
),
|
||||
)?;
|
||||
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.without_auto_env()
|
||||
.build()
|
||||
.await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
let resume_id = mcp
|
||||
.send_thread_resume_request(ThreadResumeParams {
|
||||
thread_id,
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let resume_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(resume_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadResumeResponse {
|
||||
approvals_reviewer, ..
|
||||
} = to_response::<ThreadResumeResponse>(resume_resp)?;
|
||||
|
||||
assert_eq!(approvals_reviewer, ApprovalsReviewer::AutoReview);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_preserves_acknowledged_approvals_reviewer_update() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml(codex_home.path(), &server.uri())?;
|
||||
|
||||
let thread_id = {
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.without_auto_env()
|
||||
.build()
|
||||
.await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let start_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
model: Some("gpt-5.4".to_string()),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let start_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(start_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadStartResponse { thread, .. } = to_response::<ThreadStartResponse>(start_resp)?;
|
||||
|
||||
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??;
|
||||
|
||||
let update_id = mcp
|
||||
.send_thread_settings_update_request(ThreadSettingsUpdateParams {
|
||||
thread_id: thread.id.clone(),
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let update_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(update_id)),
|
||||
)
|
||||
.await??;
|
||||
let _: ThreadSettingsUpdateResponse = to_response(update_resp)?;
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("thread/settings/updated"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
thread.id
|
||||
};
|
||||
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.without_auto_env()
|
||||
.build()
|
||||
.await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
let resume_id = mcp
|
||||
.send_thread_resume_request(ThreadResumeParams {
|
||||
thread_id,
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let resume_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(resume_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadResumeResponse {
|
||||
approvals_reviewer, ..
|
||||
} = to_response::<ThreadResumeResponse>(resume_resp)?;
|
||||
|
||||
assert_eq!(approvals_reviewer, ApprovalsReviewer::AutoReview);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_goal_get_rejects_unmaterialized_thread() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
|
||||
@@ -223,6 +223,7 @@ fn reference_context_item() -> TurnContextItem {
|
||||
current_date: Some("2026-03-23".to_string()),
|
||||
timezone: Some("America/Los_Angeles".to_string()),
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
|
||||
@@ -95,14 +95,25 @@ pub async fn update_thread_settings(
|
||||
thread_settings: ThreadSettingsOverrides,
|
||||
) {
|
||||
let updates = thread_settings_update(sess, thread_settings).await;
|
||||
let msg = match sess.update_settings(updates).await {
|
||||
Ok(()) => thread_settings_applied_event(sess).await,
|
||||
Err(err) => EventMsg::Error(ErrorEvent {
|
||||
message: format!("invalid thread settings override: {err}"),
|
||||
codex_error_info: Some(CodexErrorInfo::BadRequest),
|
||||
}),
|
||||
};
|
||||
sess.send_event_raw(Event { id: sub_id, msg }).await;
|
||||
match sess.update_settings(updates).await {
|
||||
Ok(()) => {
|
||||
sess.send_event_raw_without_materializing_rollout(Event {
|
||||
id: sub_id,
|
||||
msg: thread_settings_applied_event(sess).await,
|
||||
})
|
||||
.await;
|
||||
}
|
||||
Err(err) => {
|
||||
sess.send_event_raw(Event {
|
||||
id: sub_id,
|
||||
msg: EventMsg::Error(ErrorEvent {
|
||||
message: format!("invalid thread settings override: {err}"),
|
||||
codex_error_info: Some(CodexErrorInfo::BadRequest),
|
||||
}),
|
||||
})
|
||||
.await;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async fn thread_settings_update(
|
||||
@@ -209,7 +220,7 @@ pub(super) async fn user_input_or_turn_inner(
|
||||
return;
|
||||
};
|
||||
if emit_thread_settings_applied {
|
||||
sess.send_event_raw(Event {
|
||||
sess.send_event_raw_without_materializing_rollout(Event {
|
||||
id: sub_id.clone(),
|
||||
msg: thread_settings_applied_event(sess).await,
|
||||
})
|
||||
|
||||
@@ -1941,9 +1941,29 @@ impl Session {
|
||||
}
|
||||
|
||||
pub(crate) async fn send_event_raw(&self, event: Event) {
|
||||
self.send_event_raw_with_persistence(event, /*persist*/ true)
|
||||
.await;
|
||||
}
|
||||
|
||||
/// Delivers an event without creating a local rollout for a thread that has not materialized.
|
||||
pub(crate) async fn send_event_raw_without_materializing_rollout(&self, event: Event) {
|
||||
let persist = match self.current_rollout_path().await {
|
||||
Ok(Some(path)) => codex_rollout::existing_rollout_path(&path).await.is_some(),
|
||||
Ok(None) => true,
|
||||
Err(err) => {
|
||||
warn!("failed to check whether thread persistence is materialized: {err}");
|
||||
true
|
||||
}
|
||||
};
|
||||
self.send_event_raw_with_persistence(event, persist).await;
|
||||
}
|
||||
|
||||
async fn send_event_raw_with_persistence(&self, event: Event, persist: bool) {
|
||||
// Persist the event into rollout storage (the store filters as needed).
|
||||
let rollout_items = vec![RolloutItem::EventMsg(event.msg.clone())];
|
||||
self.persist_rollout_items(&rollout_items).await;
|
||||
if persist {
|
||||
let rollout_items = vec![RolloutItem::EventMsg(event.msg.clone())];
|
||||
self.persist_rollout_items(&rollout_items).await;
|
||||
}
|
||||
self.services
|
||||
.rollout_thread_trace
|
||||
.record_protocol_event(&event.msg);
|
||||
|
||||
@@ -172,6 +172,7 @@ async fn record_initial_history_resumed_bare_turn_context_does_not_hydrate_previ
|
||||
current_date: turn_context.current_date.clone(),
|
||||
timezone: turn_context.timezone.clone(),
|
||||
approval_policy: turn_context.approval_policy.value(),
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: turn_context.sandbox_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
@@ -218,6 +219,7 @@ async fn record_initial_history_resumed_hydrates_previous_turn_settings_from_lif
|
||||
current_date: turn_context.current_date.clone(),
|
||||
timezone: turn_context.timezone.clone(),
|
||||
approval_policy: turn_context.approval_policy.value(),
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: turn_context.sandbox_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
@@ -1252,6 +1254,7 @@ async fn record_initial_history_resumed_turn_context_after_compaction_reestablis
|
||||
current_date: turn_context.current_date.clone(),
|
||||
timezone: turn_context.timezone.clone(),
|
||||
approval_policy: turn_context.approval_policy.value(),
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: turn_context.sandbox_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
@@ -1338,6 +1341,7 @@ async fn record_initial_history_resumed_turn_context_after_compaction_reestablis
|
||||
current_date: turn_context.current_date.clone(),
|
||||
timezone: turn_context.timezone.clone(),
|
||||
approval_policy: turn_context.approval_policy.value(),
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: turn_context.sandbox_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
@@ -1369,6 +1373,7 @@ async fn record_initial_history_resumed_aborted_turn_without_id_clears_active_tu
|
||||
current_date: turn_context.current_date.clone(),
|
||||
timezone: turn_context.timezone.clone(),
|
||||
approval_policy: turn_context.approval_policy.value(),
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: turn_context.sandbox_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
@@ -1495,6 +1500,7 @@ async fn record_initial_history_resumed_unmatched_abort_preserves_active_turn_fo
|
||||
current_date: turn_context.current_date.clone(),
|
||||
timezone: turn_context.timezone.clone(),
|
||||
approval_policy: turn_context.approval_policy.value(),
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: turn_context.sandbox_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
@@ -1616,6 +1622,7 @@ async fn record_initial_history_resumed_trailing_incomplete_turn_compaction_clea
|
||||
current_date: turn_context.current_date.clone(),
|
||||
timezone: turn_context.timezone.clone(),
|
||||
approval_policy: turn_context.approval_policy.value(),
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: turn_context.sandbox_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
@@ -1783,6 +1790,7 @@ async fn record_initial_history_resumed_replaced_incomplete_compacted_turn_clear
|
||||
current_date: turn_context.current_date.clone(),
|
||||
timezone: turn_context.timezone.clone(),
|
||||
approval_policy: turn_context.approval_policy.value(),
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: turn_context.sandbox_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
|
||||
@@ -3082,6 +3082,7 @@ async fn record_initial_history_forked_hydrates_previous_turn_settings() {
|
||||
current_date: turn_context.current_date.clone(),
|
||||
timezone: turn_context.timezone.clone(),
|
||||
approval_policy: turn_context.approval_policy.value(),
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: turn_context.sandbox_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
|
||||
@@ -361,6 +361,7 @@ impl TurnContext {
|
||||
current_date: self.current_date.clone(),
|
||||
timezone: self.timezone.clone(),
|
||||
approval_policy: self.approval_policy.value(),
|
||||
approvals_reviewer: Some(self.config.approvals_reviewer),
|
||||
sandbox_policy: self.sandbox_policy(),
|
||||
permission_profile: Some(self.permission_profile()),
|
||||
network: self.turn_context_network_item(),
|
||||
|
||||
@@ -34,6 +34,7 @@ fn resume_history(
|
||||
current_date: None,
|
||||
timezone: None,
|
||||
approval_policy: config.permissions.approval_policy.value(),
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: config.legacy_sandbox_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
|
||||
+19
-10
@@ -206,6 +206,7 @@ struct ExecRunArgs {
|
||||
state_db: Option<StateDbHandle>,
|
||||
command: Option<ExecCommand>,
|
||||
config: Config,
|
||||
resume_approvals_reviewer_override: Option<codex_app_server_protocol::ApprovalsReviewer>,
|
||||
dangerously_bypass_approvals_and_sandbox: bool,
|
||||
exec_span: tracing::Span,
|
||||
images: Vec<PathBuf>,
|
||||
@@ -463,6 +464,10 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result
|
||||
build_config,
|
||||
)
|
||||
.await?;
|
||||
let resume_approvals_reviewer_override = cli_kv_overrides
|
||||
.iter()
|
||||
.any(|(key, _)| key == "approvals_reviewer")
|
||||
.then(|| config.approvals_reviewer.into());
|
||||
|
||||
#[allow(clippy::print_stderr)]
|
||||
match check_execpolicy_for_warnings(&config.config_layer_stack).await {
|
||||
@@ -576,6 +581,7 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result
|
||||
state_db,
|
||||
command,
|
||||
config,
|
||||
resume_approvals_reviewer_override,
|
||||
dangerously_bypass_approvals_and_sandbox,
|
||||
exec_span: exec_span.clone(),
|
||||
images,
|
||||
@@ -673,6 +679,7 @@ async fn run_exec_session(args: ExecRunArgs) -> anyhow::Result<()> {
|
||||
state_db,
|
||||
command,
|
||||
config,
|
||||
resume_approvals_reviewer_override,
|
||||
dangerously_bypass_approvals_and_sandbox,
|
||||
exec_span,
|
||||
images,
|
||||
@@ -804,7 +811,11 @@ async fn run_exec_session(args: ExecRunArgs) -> anyhow::Result<()> {
|
||||
&client,
|
||||
ClientRequest::ThreadResume {
|
||||
request_id: request_ids.next(),
|
||||
params: thread_resume_params_from_config(&config, thread_id),
|
||||
params: thread_resume_params_from_config(
|
||||
&config,
|
||||
thread_id,
|
||||
resume_approvals_reviewer_override,
|
||||
),
|
||||
},
|
||||
"thread/resume",
|
||||
)
|
||||
@@ -1066,7 +1077,7 @@ fn thread_start_params_from_config(config: &Config) -> ThreadStartParams {
|
||||
cwd: Some(config.cwd.to_string_lossy().to_string()),
|
||||
runtime_workspace_roots: Some(config.workspace_roots.clone()),
|
||||
approval_policy: Some(config.permissions.approval_policy.value().into()),
|
||||
approvals_reviewer: approvals_reviewer_override_from_config(config),
|
||||
approvals_reviewer: Some(config.approvals_reviewer.into()),
|
||||
sandbox: sandbox.flatten(),
|
||||
permissions,
|
||||
config: thread_config_overrides_from_config(config),
|
||||
@@ -1076,7 +1087,11 @@ fn thread_start_params_from_config(config: &Config) -> ThreadStartParams {
|
||||
}
|
||||
}
|
||||
|
||||
fn thread_resume_params_from_config(config: &Config, thread_id: String) -> ThreadResumeParams {
|
||||
fn thread_resume_params_from_config(
|
||||
config: &Config,
|
||||
thread_id: String,
|
||||
approvals_reviewer_override: Option<codex_app_server_protocol::ApprovalsReviewer>,
|
||||
) -> ThreadResumeParams {
|
||||
let permissions = permissions_selection_from_config(config);
|
||||
let sandbox = permissions.is_none().then(|| {
|
||||
sandbox_mode_from_permission_profile(
|
||||
@@ -1091,7 +1106,7 @@ fn thread_resume_params_from_config(config: &Config, thread_id: String) -> Threa
|
||||
cwd: Some(config.cwd.to_string_lossy().to_string()),
|
||||
runtime_workspace_roots: Some(config.workspace_roots.clone()),
|
||||
approval_policy: Some(config.permissions.approval_policy.value().into()),
|
||||
approvals_reviewer: approvals_reviewer_override_from_config(config),
|
||||
approvals_reviewer: approvals_reviewer_override,
|
||||
sandbox: sandbox.flatten(),
|
||||
permissions,
|
||||
config: thread_config_overrides_from_config(config),
|
||||
@@ -1141,12 +1156,6 @@ fn sandbox_mode_from_permission_profile(
|
||||
}
|
||||
}
|
||||
|
||||
fn approvals_reviewer_override_from_config(
|
||||
config: &Config,
|
||||
) -> Option<codex_app_server_protocol::ApprovalsReviewer> {
|
||||
Some(config.approvals_reviewer.into())
|
||||
}
|
||||
|
||||
async fn send_request_with_response<T>(
|
||||
client: &InProcessAppServerClient,
|
||||
request: ClientRequest,
|
||||
|
||||
@@ -491,6 +491,39 @@ async fn thread_start_params_include_review_policy_when_auto_review_is_enabled()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_params_only_include_explicit_review_policy_override() {
|
||||
let codex_home = tempdir().expect("create temp codex home");
|
||||
let cwd = tempdir().expect("create temp cwd");
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.harness_overrides(ConfigOverrides {
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
..Default::default()
|
||||
})
|
||||
.fallback_cwd(Some(cwd.path().to_path_buf()))
|
||||
.build()
|
||||
.await
|
||||
.expect("build config with guardian review policy");
|
||||
|
||||
let params_without_override = thread_resume_params_from_config(
|
||||
&config,
|
||||
"thread-id".to_string(),
|
||||
/*approvals_reviewer_override*/ None,
|
||||
);
|
||||
let params_with_override = thread_resume_params_from_config(
|
||||
&config,
|
||||
"thread-id".to_string(),
|
||||
Some(codex_app_server_protocol::ApprovalsReviewer::AutoReview),
|
||||
);
|
||||
|
||||
assert_eq!(params_without_override.approvals_reviewer, None);
|
||||
assert_eq!(
|
||||
params_with_override.approvals_reviewer,
|
||||
Some(codex_app_server_protocol::ApprovalsReviewer::AutoReview)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn build_exec_config_retries_without_invalid_headless_policy_for_auto_review() {
|
||||
let codex_home = tempdir().expect("create temp codex home");
|
||||
@@ -616,7 +649,11 @@ async fn thread_lifecycle_params_preserve_hook_trust_bypass() {
|
||||
)]));
|
||||
|
||||
let start_params = thread_start_params_from_config(&config);
|
||||
let resume_params = thread_resume_params_from_config(&config, "thread-id".to_string());
|
||||
let resume_params = thread_resume_params_from_config(
|
||||
&config,
|
||||
"thread-id".to_string(),
|
||||
/*approvals_reviewer_override*/ None,
|
||||
);
|
||||
|
||||
assert_eq!(start_params.config, expected_config);
|
||||
assert_eq!(resume_params.config, expected_config);
|
||||
@@ -648,7 +685,11 @@ async fn thread_lifecycle_params_include_legacy_sandbox_when_no_active_profile()
|
||||
.expect("build config with legacy sandbox override");
|
||||
|
||||
let start_params = thread_start_params_from_config(&config);
|
||||
let resume_params = thread_resume_params_from_config(&config, "thread-id".to_string());
|
||||
let resume_params = thread_resume_params_from_config(
|
||||
&config,
|
||||
"thread-id".to_string(),
|
||||
/*approvals_reviewer_override*/ None,
|
||||
);
|
||||
|
||||
assert_eq!(config.permissions.active_permission_profile(), None);
|
||||
assert_eq!(
|
||||
|
||||
@@ -3194,6 +3194,8 @@ pub struct TurnContextItem {
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub timezone: Option<String>,
|
||||
pub approval_policy: AskForApproval,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub approvals_reviewer: Option<ApprovalsReviewer>,
|
||||
pub sandbox_policy: SandboxPolicy,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub permission_profile: Option<PermissionProfile>,
|
||||
@@ -5939,6 +5941,7 @@ mod tests {
|
||||
current_date: None,
|
||||
timezone: None,
|
||||
approval_policy: AskForApproval::Never,
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: SandboxPolicy::DangerFullAccess,
|
||||
permission_profile: None,
|
||||
network: Some(TurnContextNetworkItem {
|
||||
|
||||
@@ -95,6 +95,7 @@ pub fn should_persist_event_msg(ev: &EventMsg) -> bool {
|
||||
| EventMsg::TurnAborted(_)
|
||||
| EventMsg::TurnStarted(_)
|
||||
| EventMsg::TurnComplete(_)
|
||||
| EventMsg::ThreadSettingsApplied(_)
|
||||
| EventMsg::WebSearchEnd(_)
|
||||
| EventMsg::ImageGenerationEnd(_)
|
||||
| EventMsg::SubAgentActivity(_) => true,
|
||||
@@ -132,7 +133,6 @@ pub fn should_persist_event_msg(ev: &EventMsg) -> bool {
|
||||
| EventMsg::AgentReasoningSectionBreak(_)
|
||||
| EventMsg::RawResponseItem(_)
|
||||
| EventMsg::SessionConfigured(_)
|
||||
| EventMsg::ThreadSettingsApplied(_)
|
||||
| EventMsg::McpToolCallBegin(_)
|
||||
| EventMsg::ExecCommandBegin(_)
|
||||
| EventMsg::TerminalInteraction(_)
|
||||
|
||||
@@ -1212,6 +1212,7 @@ async fn resume_candidate_matches_cwd_reads_latest_turn_context() -> std::io::Re
|
||||
current_date: None,
|
||||
timezone: None,
|
||||
approval_policy: AskForApproval::Never,
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
|
||||
@@ -369,6 +369,7 @@ mod tests {
|
||||
current_date: None,
|
||||
timezone: None,
|
||||
approval_policy: AskForApproval::Never,
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: SandboxPolicy::DangerFullAccess,
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
@@ -414,6 +415,7 @@ mod tests {
|
||||
current_date: None,
|
||||
timezone: None,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: SandboxPolicy::DangerFullAccess,
|
||||
permission_profile: Some(permission_profile.clone()),
|
||||
network: None,
|
||||
@@ -455,6 +457,7 @@ mod tests {
|
||||
current_date: None,
|
||||
timezone: None,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
@@ -493,6 +496,7 @@ mod tests {
|
||||
current_date: None,
|
||||
timezone: None,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
|
||||
Reference in New Issue
Block a user