mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
codex: auto-unarchive archived sessions safely on resume
Preserve the auto-unarchive-on-resume behavior while keeping archived-session lookup safe. This carries the rollout lookup hardening, the resume path updates, and the cross-platform guardian/TUI test fixes needed for current CI.
This commit is contained in:
@@ -213,6 +213,7 @@ use codex_core::exec::ExecExpiration;
|
||||
use codex_core::exec::ExecParams;
|
||||
use codex_core::exec_env::create_env;
|
||||
use codex_core::find_archived_thread_path_by_id_str;
|
||||
use codex_core::find_or_unarchive_thread_path_by_id_str;
|
||||
use codex_core::find_thread_name_by_id;
|
||||
use codex_core::find_thread_names_by_ids;
|
||||
use codex_core::find_thread_path_by_id_str;
|
||||
@@ -3852,7 +3853,7 @@ impl CodexMessageProcessor {
|
||||
if path.exists() {
|
||||
path
|
||||
} else {
|
||||
match find_thread_path_by_id_str(
|
||||
match find_or_unarchive_thread_path_by_id_str(
|
||||
&self.config.codex_home,
|
||||
&existing_thread_id.to_string(),
|
||||
)
|
||||
@@ -3878,7 +3879,7 @@ impl CodexMessageProcessor {
|
||||
}
|
||||
}
|
||||
} else {
|
||||
match find_thread_path_by_id_str(
|
||||
match find_or_unarchive_thread_path_by_id_str(
|
||||
&self.config.codex_home,
|
||||
&existing_thread_id.to_string(),
|
||||
)
|
||||
@@ -4036,7 +4037,7 @@ impl CodexMessageProcessor {
|
||||
}
|
||||
};
|
||||
|
||||
match find_thread_path_by_id_str(
|
||||
match find_or_unarchive_thread_path_by_id_str(
|
||||
&self.config.codex_home,
|
||||
&existing_thread_id.to_string(),
|
||||
)
|
||||
|
||||
@@ -224,6 +224,62 @@ async fn thread_resume_returns_rollout_history() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_unarchives_archived_rollout() -> 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 filename_ts = "2025-01-05T12-00-00";
|
||||
let conversation_id = create_fake_rollout_with_text_elements(
|
||||
codex_home.path(),
|
||||
filename_ts,
|
||||
"2025-01-05T12:00:00Z",
|
||||
"Saved user message",
|
||||
Vec::new(),
|
||||
Some("mock_provider"),
|
||||
None,
|
||||
)?;
|
||||
let active_rollout_path = rollout_path(codex_home.path(), filename_ts, &conversation_id);
|
||||
let archived_rollout_path = codex_home.path().join("archived_sessions/2025/01/05").join(
|
||||
active_rollout_path
|
||||
.file_name()
|
||||
.expect("active rollout file name"),
|
||||
);
|
||||
std::fs::create_dir_all(
|
||||
archived_rollout_path
|
||||
.parent()
|
||||
.expect("archived rollout parent directory"),
|
||||
)?;
|
||||
std::fs::rename(&active_rollout_path, &archived_rollout_path)?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let resume_id = mcp
|
||||
.send_thread_resume_request(ThreadResumeParams {
|
||||
thread_id: conversation_id.clone(),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let resume_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(resume_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadResumeResponse { thread, .. } = to_response::<ThreadResumeResponse>(resume_resp)?;
|
||||
|
||||
assert_eq!(thread.id, conversation_id);
|
||||
assert!(active_rollout_path.exists());
|
||||
assert!(!archived_rollout_path.exists());
|
||||
assert_eq!(
|
||||
std::fs::canonicalize(thread.path.as_ref().expect("thread path"))?,
|
||||
std::fs::canonicalize(&active_rollout_path)?
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_prefers_persisted_git_metadata_for_local_threads() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
|
||||
@@ -339,8 +339,8 @@ fn guardian_assessment_action_value_redacts_apply_patch_patch_text() {
|
||||
guardian_assessment_action_value(&action),
|
||||
serde_json::json!({
|
||||
"tool": "apply_patch",
|
||||
"cwd": cwd,
|
||||
"files": [file],
|
||||
"cwd": cwd.display().to_string(),
|
||||
"files": [file.display().to_string()],
|
||||
"change_count": 1,
|
||||
})
|
||||
);
|
||||
|
||||
@@ -168,6 +168,7 @@ pub use rollout::append_thread_name;
|
||||
pub use rollout::find_archived_thread_path_by_id_str;
|
||||
#[deprecated(note = "use find_thread_path_by_id_str")]
|
||||
pub use rollout::find_conversation_path_by_id_str;
|
||||
pub use rollout::find_or_unarchive_thread_path_by_id_str;
|
||||
pub use rollout::find_thread_name_by_id;
|
||||
pub use rollout::find_thread_path_by_id_str;
|
||||
pub use rollout::find_thread_path_by_name_str;
|
||||
|
||||
@@ -9,6 +9,7 @@ use codex_core::RolloutRecorder;
|
||||
use codex_core::RolloutRecorderParams;
|
||||
use codex_core::config::ConfigBuilder;
|
||||
use codex_core::find_archived_thread_path_by_id_str;
|
||||
use codex_core::find_or_unarchive_thread_path_by_id_str;
|
||||
use codex_core::find_thread_path_by_id_str;
|
||||
use codex_core::find_thread_path_by_name_str;
|
||||
use codex_protocol::ThreadId;
|
||||
@@ -219,3 +220,74 @@ async fn find_archived_locates_rollout_file_by_id() {
|
||||
|
||||
assert_eq!(found, Some(expected));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn find_thread_path_by_id_str_does_not_unarchive_archived_rollout() {
|
||||
let home = TempDir::new().unwrap();
|
||||
let id = Uuid::new_v4();
|
||||
let archived = write_minimal_rollout_with_id_in_subdir(home.path(), "archived_sessions", id);
|
||||
|
||||
let found = find_thread_path_by_id_str(home.path(), &id.to_string())
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(found, None);
|
||||
assert!(archived.exists());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn find_or_unarchive_restores_archived_rollout_file_by_id() {
|
||||
let home = TempDir::new().unwrap();
|
||||
let id = Uuid::new_v4();
|
||||
let archived = write_minimal_rollout_with_id_in_subdir(home.path(), "archived_sessions", id);
|
||||
let file_name = archived.file_name().unwrap().to_owned();
|
||||
let expected_restored = home.path().join("sessions/2024/01/01").join(file_name);
|
||||
|
||||
let found = find_or_unarchive_thread_path_by_id_str(home.path(), &id.to_string())
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(found, Some(expected_restored.clone()));
|
||||
assert!(expected_restored.exists());
|
||||
assert!(!archived.exists());
|
||||
|
||||
let archived_found = find_archived_thread_path_by_id_str(home.path(), &id.to_string())
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(archived_found, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn find_does_not_move_unrelated_file_for_stale_archived_db_path() {
|
||||
let home = TempDir::new().unwrap();
|
||||
let requested_id = Uuid::new_v4();
|
||||
let requested_thread_id = ThreadId::from_string(&requested_id.to_string()).unwrap();
|
||||
let unrelated_id = Uuid::new_v4();
|
||||
let unrelated_active_path = write_minimal_rollout_with_id(home.path(), unrelated_id);
|
||||
|
||||
upsert_thread_metadata(
|
||||
home.path(),
|
||||
requested_thread_id,
|
||||
unrelated_active_path.clone(),
|
||||
)
|
||||
.await;
|
||||
let runtime = StateRuntime::init(home.path().to_path_buf(), "test-provider".to_string())
|
||||
.await
|
||||
.unwrap();
|
||||
runtime.mark_backfill_complete(None).await.unwrap();
|
||||
runtime
|
||||
.mark_archived(
|
||||
requested_thread_id,
|
||||
unrelated_active_path.as_path(),
|
||||
Utc::now(),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let found = find_or_unarchive_thread_path_by_id_str(home.path(), &requested_id.to_string())
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(found, None);
|
||||
assert!(unrelated_active_path.exists());
|
||||
}
|
||||
|
||||
@@ -33,6 +33,7 @@ pub use codex_protocol::protocol::SessionMeta;
|
||||
pub use config::RolloutConfig;
|
||||
pub use config::RolloutConfigView;
|
||||
pub use list::find_archived_thread_path_by_id_str;
|
||||
pub use list::find_or_unarchive_thread_path_by_id_str;
|
||||
pub use list::find_thread_path_by_id_str;
|
||||
#[deprecated(note = "use find_thread_path_by_id_str")]
|
||||
pub use list::find_thread_path_by_id_str as find_conversation_path_by_id_str;
|
||||
|
||||
@@ -1246,9 +1246,97 @@ async fn find_thread_path_by_id_str_in_subdir(
|
||||
Ok(found)
|
||||
}
|
||||
|
||||
/// Locate a recorded thread rollout file by its UUID string using the existing
|
||||
/// paginated listing implementation. Returns `Ok(Some(path))` if found, `Ok(None)` if not present
|
||||
/// or the id is invalid.
|
||||
async fn try_unarchive_thread_path_by_id_str(
|
||||
codex_home: &Path,
|
||||
id_str: &str,
|
||||
) -> io::Result<Option<PathBuf>> {
|
||||
let Ok(requested_id) = Uuid::parse_str(id_str) else {
|
||||
return Ok(None);
|
||||
};
|
||||
let Some(archived_path) =
|
||||
find_thread_path_by_id_str_in_subdir(codex_home, ARCHIVED_SESSIONS_SUBDIR, id_str).await?
|
||||
else {
|
||||
return Ok(None);
|
||||
};
|
||||
let archived_root = codex_home.join(ARCHIVED_SESSIONS_SUBDIR);
|
||||
if archived_path.strip_prefix(&archived_root).is_err() {
|
||||
tracing::error!(
|
||||
"archived rollout candidate for thread {id_str} is not under {}: {}",
|
||||
archived_root.display(),
|
||||
archived_path.display()
|
||||
);
|
||||
return Ok(None);
|
||||
}
|
||||
|
||||
let Some(file_name) = archived_path.file_name().map(OsStr::to_owned) else {
|
||||
tracing::error!(
|
||||
"archived rollout path for thread {id_str} missing file name: {}",
|
||||
archived_path.display()
|
||||
);
|
||||
return Ok(None);
|
||||
};
|
||||
let file_name_str = file_name.to_string_lossy();
|
||||
let Some((_created_at, file_id)) = parse_timestamp_uuid_from_filename(&file_name_str) else {
|
||||
tracing::error!(
|
||||
"archived rollout path for thread {id_str} has invalid rollout filename: {}",
|
||||
archived_path.display()
|
||||
);
|
||||
return Ok(None);
|
||||
};
|
||||
if file_id != requested_id {
|
||||
tracing::error!(
|
||||
"archived rollout path for thread {id_str} has mismatched rollout filename: {}",
|
||||
archived_path.display()
|
||||
);
|
||||
return Ok(None);
|
||||
}
|
||||
let Some((year, month, day)) = rollout_date_parts(&file_name) else {
|
||||
tracing::error!(
|
||||
"archived rollout path for thread {id_str} missing filename timestamp: {}",
|
||||
archived_path.display()
|
||||
);
|
||||
return Ok(None);
|
||||
};
|
||||
|
||||
let restored_dir = codex_home
|
||||
.join(SESSIONS_SUBDIR)
|
||||
.join(year)
|
||||
.join(month)
|
||||
.join(day);
|
||||
tokio::fs::create_dir_all(&restored_dir).await?;
|
||||
let restored_path = restored_dir.join(&file_name);
|
||||
match tokio::fs::rename(&archived_path, &restored_path).await {
|
||||
Ok(()) => {}
|
||||
Err(err) => {
|
||||
if tokio::fs::try_exists(&restored_path).await.unwrap_or(false) {
|
||||
tracing::debug!(
|
||||
"archived rollout for thread {id_str} already restored concurrently to {}",
|
||||
restored_path.display()
|
||||
);
|
||||
} else {
|
||||
return Err(err);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(state_db_ctx) = state_db::open_if_present(codex_home, "").await
|
||||
&& let Ok(thread_id) = ThreadId::from_string(id_str)
|
||||
{
|
||||
let _ = state_db_ctx
|
||||
.mark_unarchived(thread_id, restored_path.as_path())
|
||||
.await;
|
||||
}
|
||||
|
||||
Ok(Some(restored_path))
|
||||
}
|
||||
|
||||
/// Locate a recorded active thread rollout file by its UUID string using the existing paginated
|
||||
/// listing implementation.
|
||||
///
|
||||
/// This helper intentionally has no side effects. Callers that want "resume and restore from
|
||||
/// archive if needed" semantics must opt in with [`find_or_unarchive_thread_path_by_id_str`] so
|
||||
/// that other lookup paths (for example fork-reference resolution) do not silently move archived
|
||||
/// rollouts back into `sessions/`.
|
||||
pub async fn find_thread_path_by_id_str(
|
||||
codex_home: &Path,
|
||||
id_str: &str,
|
||||
@@ -1256,6 +1344,18 @@ pub async fn find_thread_path_by_id_str(
|
||||
find_thread_path_by_id_str_in_subdir(codex_home, SESSIONS_SUBDIR, id_str).await
|
||||
}
|
||||
|
||||
/// Locate a thread rollout file by UUID string, restoring it from `archived_sessions/` when
|
||||
/// needed for resume flows.
|
||||
pub async fn find_or_unarchive_thread_path_by_id_str(
|
||||
codex_home: &Path,
|
||||
id_str: &str,
|
||||
) -> io::Result<Option<PathBuf>> {
|
||||
if let Some(active_path) = find_thread_path_by_id_str(codex_home, id_str).await? {
|
||||
return Ok(Some(active_path));
|
||||
}
|
||||
try_unarchive_thread_path_by_id_str(codex_home, id_str).await
|
||||
}
|
||||
|
||||
/// Locate an archived thread rollout file by its UUID string.
|
||||
pub async fn find_archived_thread_path_by_id_str(
|
||||
codex_home: &Path,
|
||||
|
||||
@@ -10714,13 +10714,15 @@ async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context
|
||||
"expected permissions popup to open with the current preset selected: {popup}"
|
||||
);
|
||||
|
||||
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
|
||||
#[cfg(target_os = "windows")]
|
||||
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
|
||||
let popup = render_bottom_popup(&chat, /*width*/ 120);
|
||||
assert!(
|
||||
popup
|
||||
.lines()
|
||||
.any(|line| line.contains("Guardian Approvals") && line.contains('›')),
|
||||
"expected one Down from Default to select Guardian Approvals: {popup}"
|
||||
"expected permissions popup to select Guardian Approvals: {popup}"
|
||||
);
|
||||
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
|
||||
|
||||
|
||||
Reference in New Issue
Block a user