Skip to content

[codex] Preserve reviewer when resuming threads #30278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
viyatb-oai wants to merge 1 commit into
base: main
Choose a base branch
from
+129 −0
Open

[codex] Preserve reviewer when resuming threads #30278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions codex-rs/app-server/src/request_processors/thread_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,28 @@ 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"))
Comment on lines +172 to +173

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Don't treat default exec reviewer as an override

This guard assumes Some means the resume request explicitly overrode the reviewer, but the codex exec resume path always fills ThreadResumeParams.approvals_reviewer from the resolved config (codex-rs/exec/src/lib.rs:1094, via a helper that always returns Some at 1144-1147). As a result, resuming an auto-review thread through exec with the current config/default set to user still exits here before reading the persisted rollout reviewer, preserving the exact silent switch this change is meant to prevent for that resume surface.

AGENTS.md reference: AGENTS.md:L102-L110

Useful? React with 👍 / 👎.

{
return;
}

let InitialHistory::Resumed(resumed_history) = thread_history else {
return;
};
typesafe_overrides.approvals_reviewer = resumed_history.history.iter().rev().find_map(|item| {
let RolloutItem::TurnContext(turn_context) = item else {
return None;
};
turn_context.approvals_reviewer
Comment on lines +181 to +185

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Read reviewer from existing rollout events

When resuming an existing rollout created before this new field was added, or one where the latest change was thread/settings/update before the next user turn, the latest reviewer is persisted on SessionConfigured/ThreadSettingsApplied events rather than on a TurnContext item. Because this scan only accepts TurnContext, it returns None and the later config load falls back to the current config, so auto-review threads can still silently resume as user-review in those cases; please fall back to those existing events when no TurnContext reviewer is present.

AGENTS.md reference: AGENTS.md:L102-L110

Useful? React with 👍 / 👎.

});
}

fn normalize_thread_list_cwd_filters(
cwd: Option<ThreadListCwdFilter>,
) -> Result<Option<Vec<PathBuf>>, JSONRPCErrorError> {
Expand Down Expand Up @@ -2936,6 +2958,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;
};
Expand Down
82 changes: 82 additions & 0 deletions codex-rs/app-server/tests/suite/v2/thread_resume.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -415,6 +416,87 @@ 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::new(codex_home.path()).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::new(codex_home.path()).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;
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/src/context_manager/history_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,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,
Expand Down
8 changes: 8 additions & 0 deletions codex-rs/core/src/session/rollout_reconstruction_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/src/session/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3022,6 +3022,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,
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/src/session/turn_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/tests/suite/resume_warning.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions codex-rs/protocol/src/protocol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3242,6 +3242,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>,
Expand Down Expand Up @@ -5750,6 +5752,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 {
Expand Down
1 change: 1 addition & 0 deletions codex-rs/rollout/src/recorder_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions codex-rs/state/src/extract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ mod tests {
current_date: None,
timezone: None,
approval_policy: AskForApproval::Never,
approvals_reviewer: None,
sandbox_policy: SandboxPolicy::DangerFullAccess,
permission_profile: None,
network: None,
Expand Down Expand Up @@ -413,6 +414,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,
Expand Down Expand Up @@ -454,6 +456,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,
Expand Down Expand Up @@ -492,6 +495,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,
Expand Down
Loading