mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
Reject Guardian review threads during interactive CLI resume (#53816)
## What changed Check the selected thread's app-server metadata before resuming it in the TUI. Reject Guardian review threads with a clear error and suggest `codex resume <parent_thread_id>` when a parent session is available. Preserve existing resume behavior if the metadata read fails. ## Testing Add a regression test for Guardian thread rejection and the parent-session hint. Update daemon startup coverage to select a saved session when testing `resume`. GitOrigin-RevId: 64b43bb24b5e24eec14c8204c6e15232387dade8
This commit is contained in:
@@ -316,7 +316,33 @@ async fn daemon_startup(command: &str) -> Result<()> {
|
||||
"UseAmazonBedrock"
|
||||
} else {
|
||||
args.extend([command.into(), "--strict-config".into()]);
|
||||
steps.push_back(("Nosessionsyet", b"\x1b"));
|
||||
if command == "resume" {
|
||||
let thread_id = "00000000-0000-0000-0000-000000000001";
|
||||
let sessions = home.path().join("sessions/2025/01/05");
|
||||
fs::create_dir_all(&sessions)?;
|
||||
let metadata = serde_json::json!({
|
||||
"timestamp": "2025-01-05T12:00:00Z",
|
||||
"type": "session_meta",
|
||||
"payload": {
|
||||
"session_id": thread_id, "id": thread_id,
|
||||
"timestamp": "2025-01-05T12:00:00Z", "cwd": workspace_path,
|
||||
"originator": "codex", "cli_version": "0.0.0",
|
||||
"source": "cli", "model_provider": "openai"
|
||||
}
|
||||
});
|
||||
let message = serde_json::json!({
|
||||
"timestamp": "2025-01-05T12:00:00Z",
|
||||
"type": "event_msg",
|
||||
"payload": {"type": "user_message", "message": "Saved resume session", "kind": "plain"}
|
||||
});
|
||||
fs::write(
|
||||
sessions.join(format!("rollout-2025-01-05T12-00-00-{thread_id}.jsonl")),
|
||||
format!("{metadata}\n{message}\n"),
|
||||
)?;
|
||||
steps.push_back(("Savedresumesession", b"\r"));
|
||||
} else {
|
||||
steps.push_back(("Nosessionsyet", b"\x1b"));
|
||||
}
|
||||
steps.push_back(("GPT-5.6-Terra", b"\x14"));
|
||||
if elevated {
|
||||
"Runningasadministrator:sharedbackgroundserverdisabled."
|
||||
|
||||
@@ -1824,6 +1824,22 @@ async fn run_ratatui_app(
|
||||
},
|
||||
};
|
||||
|
||||
if let resume_picker::SessionSelection::Resume(target) = &session_selection {
|
||||
let validation = startup_draft
|
||||
.run_until(
|
||||
&mut tui,
|
||||
session_resume::validate_resume_target(&mut app_server, target.thread_id),
|
||||
)
|
||||
.await;
|
||||
if let Err(err) = validation
|
||||
.map_err(color_eyre::Report::from)
|
||||
.and_then(|result| result)
|
||||
{
|
||||
shutdown_startup_session(Some(app_server), &mut terminal_restore_guard).await;
|
||||
return Err(err);
|
||||
}
|
||||
}
|
||||
|
||||
// Remote startup keeps its existing explicit --cd trust check. Resolving other
|
||||
// remote folders requires authoritative project-root information from the server.
|
||||
if !uses_remote_workspace || remote_cwd_override.is_some() {
|
||||
@@ -2731,6 +2747,52 @@ requires_openai_auth = {requires_openai_auth}
|
||||
.await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn startup_resume_rejects_guardian_thread() -> color_eyre::Result<()> {
|
||||
let temp_dir = TempDir::new()?;
|
||||
let config = build_config(&temp_dir).await?;
|
||||
let thread_id = write_session_rollout(
|
||||
temp_dir.path(),
|
||||
"2025-01-05T12-00-00",
|
||||
"2025-01-05T12:00:00Z",
|
||||
"Guardian review",
|
||||
&config.model_provider_id,
|
||||
config.cwd.as_path(),
|
||||
)?;
|
||||
let rollout_path = temp_dir
|
||||
.path()
|
||||
.join("sessions/2025/01/05")
|
||||
.join(format!("rollout-2025-01-05T12-00-00-{thread_id}.jsonl"));
|
||||
let rollout = std::fs::read_to_string(&rollout_path)?;
|
||||
let (meta, history) = rollout.split_once('\n').expect("rollout has metadata");
|
||||
let mut meta: serde_json::Value = serde_json::from_str(meta)?;
|
||||
meta["payload"]["source"] = serde_json::json!({"subagent": {"other": "guardian"}});
|
||||
meta["payload"]["parent_thread_id"] =
|
||||
serde_json::json!("00000000-0000-0000-0000-000000000001");
|
||||
std::fs::write(&rollout_path, format!("{meta}\n{history}"))?;
|
||||
|
||||
let state_db =
|
||||
init_state_db_for_app_server_target(&config, &AppServerTarget::Embedded).await?;
|
||||
let mut app_server = start_app_server_for_picker(
|
||||
&config,
|
||||
&AppServerTarget::Embedded,
|
||||
Vec::new(),
|
||||
LoaderOverrides::without_managed_config_for_tests(),
|
||||
state_db,
|
||||
Arc::new(EnvironmentManager::default_for_tests()),
|
||||
)
|
||||
.await?;
|
||||
let error = session_resume::validate_resume_target(&mut app_server, thread_id)
|
||||
.await
|
||||
.expect_err("Guardian resume should be rejected");
|
||||
insta::assert_snapshot!(error.to_string(), @"
|
||||
Cannot resume a Guardian review thread.
|
||||
Resume its parent session instead: codex resume 00000000-0000-0000-0000-000000000001
|
||||
");
|
||||
app_server.shutdown().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn startup_resume_and_fork_use_configured_or_explicit_cwd() -> color_eyre::Result<()> {
|
||||
for (action, configured_mode, has_explicit_cwd, expected_directory) in [
|
||||
|
||||
@@ -1,7 +1,8 @@
|
||||
//! Resolve saved-session state needed before resuming or forking a thread.
|
||||
//!
|
||||
//! The app-server API owns thread metadata. This module coordinates the TUI-specific
|
||||
//! cwd prompt using the cwd reported by the selected thread.
|
||||
//! cwd prompt using the cwd reported by the selected thread and checks Guardian
|
||||
//! eligibility for interactive CLI resume.
|
||||
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
@@ -42,6 +43,28 @@ pub(crate) fn effective_resume_cwd_mode(
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn validate_resume_target(
|
||||
app_server: &mut AppServerSession,
|
||||
thread_id: ThreadId,
|
||||
) -> color_eyre::Result<()> {
|
||||
let Ok(thread) = app_server
|
||||
.thread_read(thread_id, /*include_turns*/ false)
|
||||
.await
|
||||
else {
|
||||
// Preserve existing resume behavior when this metadata check is unavailable.
|
||||
return Ok(());
|
||||
};
|
||||
if codex_state::is_guardian_review_source(&thread.source.into()) {
|
||||
let parent_hint = thread
|
||||
.parent_thread_id
|
||||
.map_or_else(String::new, |parent_id| {
|
||||
format!("\nResume its parent session instead: codex resume {parent_id}")
|
||||
});
|
||||
color_eyre::eyre::bail!("Cannot resume a Guardian review thread.{parent_hint}");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn read_session_cwd(
|
||||
app_server: &mut AppServerSession,
|
||||
thread_id: ThreadId,
|
||||
|
||||
Reference in New Issue
Block a user