mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Fix invalid TUI resume hints (#18059)
Addresses #18011 Problem: #16987 allowed zero-token TUI exits to print resume hints, which exposed precomputed thread ids before their rollout files were persisted; #17222 made the same invalid hint visible when switching sessions via `/resume`. Solution: Only include resume commands for TUI sessions backed by a materialized non-empty rollout, and cover both missing-rollout and persisted-rollout summary behavior. Testing: Manually verified by pressing Ctrl+D before the first prompt and confirming that no "to continue this session" message was generated.
This commit is contained in:
committed by
GitHub
Unverified
parent
9c56e89e4f
commit
ab82568536
+78
-8
@@ -325,8 +325,12 @@ fn session_summary(
|
||||
token_usage: TokenUsage,
|
||||
thread_id: Option<ThreadId>,
|
||||
thread_name: Option<String>,
|
||||
rollout_path: Option<&Path>,
|
||||
) -> Option<SessionSummary> {
|
||||
let usage_line = (!token_usage.is_zero()).then(|| FinalOutput::from(token_usage).to_string());
|
||||
let (thread_id, thread_name) = resumable_thread(thread_id, thread_name, rollout_path)
|
||||
.map(|thread| (Some(thread.thread_id), thread.thread_name))
|
||||
.unwrap_or((None, None));
|
||||
let resume_command =
|
||||
crate::legacy_core::util::resume_command(thread_name.as_deref(), thread_id);
|
||||
|
||||
@@ -340,6 +344,29 @@ fn session_summary(
|
||||
})
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
struct ResumableThread {
|
||||
thread_id: ThreadId,
|
||||
thread_name: Option<String>,
|
||||
}
|
||||
|
||||
fn resumable_thread(
|
||||
thread_id: Option<ThreadId>,
|
||||
thread_name: Option<String>,
|
||||
rollout_path: Option<&Path>,
|
||||
) -> Option<ResumableThread> {
|
||||
let thread_id = thread_id?;
|
||||
let rollout_path = rollout_path?;
|
||||
rollout_path_is_resumable(rollout_path).then_some(ResumableThread {
|
||||
thread_id,
|
||||
thread_name,
|
||||
})
|
||||
}
|
||||
|
||||
fn rollout_path_is_resumable(rollout_path: &Path) -> bool {
|
||||
std::fs::metadata(rollout_path).is_ok_and(|metadata| metadata.is_file() && metadata.len() > 0)
|
||||
}
|
||||
|
||||
fn errors_for_cwd(cwd: &Path, response: &ListSkillsResponseEvent) -> Vec<SkillErrorInfo> {
|
||||
response
|
||||
.skills
|
||||
@@ -3448,6 +3475,7 @@ impl App {
|
||||
self.chat_widget.token_usage(),
|
||||
self.chat_widget.thread_id(),
|
||||
self.chat_widget.thread_name(),
|
||||
self.chat_widget.rollout_path().as_deref(),
|
||||
);
|
||||
self.shutdown_current_thread(app_server).await;
|
||||
let tracked_thread_ids: Vec<ThreadId> =
|
||||
@@ -4126,10 +4154,15 @@ impl App {
|
||||
return Err(err);
|
||||
}
|
||||
};
|
||||
let resumable_thread = resumable_thread(
|
||||
app.chat_widget.thread_id(),
|
||||
app.chat_widget.thread_name(),
|
||||
app.chat_widget.rollout_path().as_deref(),
|
||||
);
|
||||
Ok(AppExitInfo {
|
||||
token_usage: app.token_usage(),
|
||||
thread_id: app.chat_widget.thread_id(),
|
||||
thread_name: app.chat_widget.thread_name(),
|
||||
thread_id: resumable_thread.as_ref().map(|thread| thread.thread_id),
|
||||
thread_name: resumable_thread.and_then(|thread| thread.thread_name),
|
||||
update_action: app.pending_update_action,
|
||||
exit_reason,
|
||||
})
|
||||
@@ -4249,6 +4282,7 @@ impl App {
|
||||
self.chat_widget.token_usage(),
|
||||
self.chat_widget.thread_id(),
|
||||
self.chat_widget.thread_name(),
|
||||
self.chat_widget.rollout_path().as_deref(),
|
||||
);
|
||||
match app_server
|
||||
.resume_thread(resume_config.clone(), target_session.thread_id)
|
||||
@@ -4403,6 +4437,7 @@ impl App {
|
||||
self.chat_widget.token_usage(),
|
||||
self.chat_widget.thread_id(),
|
||||
self.chat_widget.thread_name(),
|
||||
self.chat_widget.rollout_path().as_deref(),
|
||||
);
|
||||
self.chat_widget
|
||||
.add_plain_history_lines(vec!["/fork".magenta().into()]);
|
||||
@@ -11496,14 +11531,33 @@ guardian_approval = true
|
||||
session_summary(
|
||||
TokenUsage::default(),
|
||||
/*thread_id*/ None,
|
||||
/*thread_name*/ None
|
||||
/*thread_name*/ None,
|
||||
/*rollout_path*/ None,
|
||||
)
|
||||
.is_none()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn session_summary_includes_resume_hint() {
|
||||
async fn session_summary_skips_resume_hint_until_rollout_exists() {
|
||||
let usage = TokenUsage::default();
|
||||
let conversation = ThreadId::from_string("123e4567-e89b-12d3-a456-426614174000").unwrap();
|
||||
let temp_dir = tempdir().expect("temp dir");
|
||||
let rollout_path = temp_dir.path().join("rollout.jsonl");
|
||||
|
||||
assert!(
|
||||
session_summary(
|
||||
usage,
|
||||
Some(conversation),
|
||||
/*thread_name*/ None,
|
||||
Some(&rollout_path),
|
||||
)
|
||||
.is_none()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn session_summary_includes_resume_hint_for_persisted_rollout() {
|
||||
let usage = TokenUsage {
|
||||
input_tokens: 10,
|
||||
output_tokens: 2,
|
||||
@@ -11511,9 +11565,17 @@ guardian_approval = true
|
||||
..Default::default()
|
||||
};
|
||||
let conversation = ThreadId::from_string("123e4567-e89b-12d3-a456-426614174000").unwrap();
|
||||
let temp_dir = tempdir().expect("temp dir");
|
||||
let rollout_path = temp_dir.path().join("rollout.jsonl");
|
||||
std::fs::write(&rollout_path, "{}\n").expect("write rollout");
|
||||
|
||||
let summary =
|
||||
session_summary(usage, Some(conversation), /*thread_name*/ None).expect("summary");
|
||||
let summary = session_summary(
|
||||
usage,
|
||||
Some(conversation),
|
||||
/*thread_name*/ None,
|
||||
Some(&rollout_path),
|
||||
)
|
||||
.expect("summary");
|
||||
assert_eq!(
|
||||
summary.usage_line,
|
||||
Some("Token usage: total=12 input=10 output=2".to_string())
|
||||
@@ -11533,9 +11595,17 @@ guardian_approval = true
|
||||
..Default::default()
|
||||
};
|
||||
let conversation = ThreadId::from_string("123e4567-e89b-12d3-a456-426614174000").unwrap();
|
||||
let temp_dir = tempdir().expect("temp dir");
|
||||
let rollout_path = temp_dir.path().join("rollout.jsonl");
|
||||
std::fs::write(&rollout_path, "{}\n").expect("write rollout");
|
||||
|
||||
let summary = session_summary(usage, Some(conversation), Some("my-session".to_string()))
|
||||
.expect("summary");
|
||||
let summary = session_summary(
|
||||
usage,
|
||||
Some(conversation),
|
||||
Some("my-session".to_string()),
|
||||
Some(&rollout_path),
|
||||
)
|
||||
.expect("summary");
|
||||
assert_eq!(
|
||||
summary.resume_command,
|
||||
Some("codex resume my-session".to_string())
|
||||
|
||||
Reference in New Issue
Block a user