mirror of
https://github.com/openai/codex.git
synced 2026-11-04 00:24:43 +00:00
Retry safety-buffered turns on a forked thread (#33207)
## Why Retrying a safety-buffered turn by rolling back the active thread rewrites the source conversation and requires the TUI to repair its local transcript state. ## What changed - Interrupt the buffered turn, then fork after the preceding completed turn and submit the prompt on the new thread with the faster model and low reasoning effort. - Start a new thread when the buffered request is the conversation's first turn. - Preserve committed steering input in the retry, leave the source thread intact, and restore the prompt alongside any existing draft if branching fails. - Do not offer the retry action for side conversations or stale and in-progress turns. ## Testing Added coverage for retries with prior context, first-turn retries, committed steering input, branch failures, existing composer drafts and attachments, and side conversations. GitOrigin-RevId: adb49da8236e9212ef0082e033e6f715986b5cd9
This commit is contained in:
1 parent
77a3f4e80f
commit
9cddda7556
21 files changed
+897
-586
No files matched your search
@@ -253,9 +253,25 @@ impl ChatWidget {
|
||||
}
|
||||
|
||||
pub(crate) fn restore_user_message_to_composer(&mut self, user_message: UserMessage) {
|
||||
let draft = self.bottom_pane.composer_draft_snapshot();
|
||||
let pending_pastes = draft.pending_pastes;
|
||||
let draft_message = UserMessage {
|
||||
text: draft.text,
|
||||
text_elements: draft.text_elements,
|
||||
local_images: draft.local_images,
|
||||
remote_image_urls: draft.remote_image_urls,
|
||||
mention_bindings: draft.mention_bindings,
|
||||
};
|
||||
let mut messages = vec![user_message];
|
||||
if !draft_message.text.is_empty()
|
||||
|| !draft_message.local_images.is_empty()
|
||||
|| !draft_message.remote_image_urls.is_empty()
|
||||
{
|
||||
messages.push(draft_message);
|
||||
}
|
||||
self.restore_composer_state(Self::composer_state_from_user_message(
|
||||
user_message,
|
||||
Vec::new(),
|
||||
merge_user_messages(messages),
|
||||
pending_pastes,
|
||||
));
|
||||
}
|
||||
|
||||
|
||||
@@ -255,14 +255,6 @@ impl ChatWidget {
|
||||
self.copy_last_agent_markdown_with(crate::clipboard_copy::copy_to_clipboard);
|
||||
}
|
||||
|
||||
pub(crate) fn truncate_agent_copy_history_to_user_turn_count(
|
||||
&mut self,
|
||||
user_turn_count: usize,
|
||||
) {
|
||||
self.transcript
|
||||
.truncate_copy_history_to_user_turn_count(user_turn_count);
|
||||
}
|
||||
|
||||
/// Inner implementation with an injectable clipboard backend for testing.
|
||||
pub(super) fn copy_last_agent_markdown_with(
|
||||
&mut self,
|
||||
@@ -281,11 +273,6 @@ impl ChatWidget {
|
||||
"Copy failed: {error}"
|
||||
))),
|
||||
},
|
||||
_ if self.transcript.copy_history_evicted_by_rollback => {
|
||||
self.add_to_history(history_cell::new_error_event(format!(
|
||||
"Cannot copy that response after rewinding. Only the most recent {MAX_AGENT_COPY_HISTORY} responses are available to /copy."
|
||||
)));
|
||||
}
|
||||
_ => self.add_to_history(history_cell::new_error_event(
|
||||
"No agent response to copy".into(),
|
||||
)),
|
||||
|
||||
@@ -63,21 +63,22 @@ impl ChatWidget {
|
||||
.is_some_and(|active| active.turn_id == turn_id && !active.agent_message_started)
|
||||
}
|
||||
|
||||
pub(crate) fn prepare_safety_buffering_retry(&mut self) {
|
||||
let safety_buffering_prompt = self.safety_buffering_prompt.take();
|
||||
pub(crate) fn prepare_safety_buffered_retry_submission(&mut self, prompt: UserMessage) {
|
||||
self.last_rendered_user_message_display = None;
|
||||
self.finalize_turn();
|
||||
self.safety_buffering_prompt = safety_buffering_prompt;
|
||||
self.safety_buffering_prompt = Some(prompt);
|
||||
self.input_queue.user_turn_pending_start = true;
|
||||
}
|
||||
|
||||
pub(crate) fn fail_safety_buffering_retry(&mut self) {
|
||||
pub(crate) fn commit_safety_buffered_retry_submission(&mut self, prompt: UserMessage) {
|
||||
let display =
|
||||
user_message_display_for_history(prompt, &UserMessageHistoryRecord::UserMessageText);
|
||||
self.on_user_message_display(display);
|
||||
}
|
||||
|
||||
pub(crate) fn cancel_safety_buffered_retry_submission(&mut self) {
|
||||
self.input_queue.user_turn_pending_start = false;
|
||||
self.clear_safety_buffering();
|
||||
let prompt = self.safety_buffering_prompt.take();
|
||||
if let Some(prompt) = prompt {
|
||||
self.restore_user_message_to_composer(prompt);
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn on_model_safety_buffering_updated(
|
||||
@@ -112,14 +113,23 @@ impl ChatWidget {
|
||||
return;
|
||||
}
|
||||
|
||||
let retry_turn = self
|
||||
.safety_buffering
|
||||
.submitted_turn
|
||||
.as_ref()
|
||||
.filter(|(submitted_turn_id, _)| replay_kind.is_none() && submitted_turn_id == &turn_id)
|
||||
.map(|(_, turn)| turn.clone());
|
||||
let retry_turn = if self.side_conversation_active() {
|
||||
None
|
||||
} else {
|
||||
self.safety_buffering
|
||||
.submitted_turn
|
||||
.as_ref()
|
||||
.filter(|(submitted_turn_id, _)| {
|
||||
replay_kind.is_none() && submitted_turn_id == &turn_id
|
||||
})
|
||||
.map(|(_, turn)| turn.clone())
|
||||
};
|
||||
let thread_id = self.thread_id;
|
||||
let can_offer_retry = faster_model.is_some() && retry_turn.is_some() && thread_id.is_some();
|
||||
let retry_prompt = self.safety_buffering_prompt.clone();
|
||||
let can_offer_retry = faster_model.is_some()
|
||||
&& retry_turn.is_some()
|
||||
&& retry_prompt.is_some()
|
||||
&& thread_id.is_some();
|
||||
let previous_active = self
|
||||
.safety_buffering
|
||||
.active
|
||||
@@ -165,8 +175,8 @@ impl ChatWidget {
|
||||
}
|
||||
let header = ColumnRenderable::with(header);
|
||||
let mut items = Vec::new();
|
||||
if let (Some(faster_model), Some(turn), Some(thread_id)) =
|
||||
(faster_model, retry_turn, thread_id)
|
||||
if let (Some(faster_model), Some(turn), Some(prompt), Some(thread_id)) =
|
||||
(faster_model, retry_turn, retry_prompt, thread_id)
|
||||
{
|
||||
items.push(SelectionItem {
|
||||
name: "Retry with a faster model".to_string(),
|
||||
@@ -176,6 +186,7 @@ impl ChatWidget {
|
||||
turn_id: turn_id.clone(),
|
||||
model: faster_model.clone(),
|
||||
turn: turn.clone(),
|
||||
prompt: prompt.clone(),
|
||||
});
|
||||
})],
|
||||
dismiss_on_select: true,
|
||||
|
||||
+11
@@ -0,0 +1,11 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/app_server.rs
|
||||
expression: popup
|
||||
---
|
||||
Our systems are thinking a bit more about this request before responding.
|
||||
|
||||
› 1. Dismiss and keep waiting
|
||||
2. Learn more
|
||||
|
||||
No action is required. Codex will keep waiting, and this menu will close when
|
||||
the response is ready.
|
||||
@@ -144,14 +144,15 @@ async fn safety_buffering_offers_one_retry_with_app_wording() {
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE));
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE));
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
let (event_thread_id, event_turn_id, model, turn) = loop {
|
||||
let (event_thread_id, event_turn_id, model, turn, prompt) = loop {
|
||||
match rx.try_recv() {
|
||||
Ok(AppEvent::RetrySafetyBufferedTurn {
|
||||
thread_id,
|
||||
turn_id,
|
||||
model,
|
||||
turn,
|
||||
}) => break (thread_id, turn_id, model, turn),
|
||||
prompt,
|
||||
}) => break (thread_id, turn_id, model, turn, prompt),
|
||||
Ok(_) => continue,
|
||||
Err(err) => panic!("expected safety-buffering retry event: {err}"),
|
||||
}
|
||||
@@ -160,12 +161,32 @@ async fn safety_buffering_offers_one_retry_with_app_wording() {
|
||||
assert_eq!(event_turn_id, turn_id);
|
||||
assert_eq!(model, "faster-model");
|
||||
assert_matches!(turn, Op::UserTurn { .. });
|
||||
assert_eq!(prompt, UserMessage::from("Explain the request"));
|
||||
assert!(
|
||||
!render_bottom_popup(&chat, /*width*/ 80)
|
||||
.contains("Press enter to confirm or esc to go back")
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn safety_buffering_does_not_offer_retry_in_side_conversation() {
|
||||
let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
chat.set_side_conversation_active(/*active*/ true);
|
||||
let (thread_id, turn_id, _) = start_safety_buffering_test_turn(&mut chat, &mut op_rx);
|
||||
|
||||
chat.handle_server_notification(
|
||||
ServerNotification::ModelSafetyBufferingUpdated(safety_buffering_notification(
|
||||
thread_id,
|
||||
turn_id,
|
||||
Some("faster-model"),
|
||||
)),
|
||||
/*replay_kind*/ None,
|
||||
);
|
||||
|
||||
let popup = render_bottom_popup(&chat, /*width*/ 80);
|
||||
assert_chatwidget_snapshot!("safety_buffering_side_conversation_without_retry", popup);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn safety_buffering_remains_visible_until_turn_completes() {
|
||||
let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
@@ -190,20 +211,6 @@ async fn safety_buffering_remains_visible_until_turn_completes() {
|
||||
assert!(!render_bottom_popup(&chat, /*width*/ 80).contains(SAFETY_BUFFERING_HEADER_TEXT));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn failed_safety_buffering_retry_restores_submitted_prompt() {
|
||||
let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
start_safety_buffering_test_turn(&mut chat, &mut op_rx);
|
||||
|
||||
chat.prepare_safety_buffering_retry();
|
||||
assert!(chat.input_queue.user_turn_pending_start);
|
||||
|
||||
chat.fail_safety_buffering_retry();
|
||||
|
||||
assert_eq!(chat.composer_text_with_pending(), "Explain the request");
|
||||
assert!(!chat.input_queue.user_turn_pending_start);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn safety_buffering_without_retry_shows_short_app_message() {
|
||||
let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
@@ -735,6 +735,52 @@ async fn queued_restore_with_remote_images_keeps_local_placeholder_mapping() {
|
||||
assert_eq!(chat.remote_image_urls(), remote_image_urls);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn restored_message_preserves_existing_composer_draft_and_attachments() {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let retry_image = PathBuf::from("/tmp/retry.png");
|
||||
let draft_image = PathBuf::from("/tmp/draft.png");
|
||||
let paste_placeholder = "[Pasted Content 5 chars]";
|
||||
let draft_text = format!("[Image #1] {paste_placeholder} draft");
|
||||
chat.bottom_pane.set_composer_text(
|
||||
draft_text,
|
||||
vec![TextElement::new(
|
||||
(0.."[Image #1]".len()).into(),
|
||||
Some("[Image #1]".to_string()),
|
||||
)],
|
||||
vec![draft_image.clone()],
|
||||
);
|
||||
chat.bottom_pane
|
||||
.set_composer_pending_pastes(vec![(paste_placeholder.to_string(), "hello".to_string())]);
|
||||
|
||||
chat.restore_user_message_to_composer(UserMessage {
|
||||
text: "[Image #1] retry prompt".to_string(),
|
||||
local_images: vec![LocalImageAttachment {
|
||||
placeholder: "[Image #1]".to_string(),
|
||||
path: retry_image.clone(),
|
||||
}],
|
||||
remote_image_urls: Vec::new(),
|
||||
text_elements: vec![TextElement::new(
|
||||
(0.."[Image #1]".len()).into(),
|
||||
Some("[Image #1]".to_string()),
|
||||
)],
|
||||
mention_bindings: Vec::new(),
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
chat.bottom_pane.composer_text(),
|
||||
format!("[Image #1] retry prompt\n[Image #2] {paste_placeholder} draft")
|
||||
);
|
||||
assert_eq!(
|
||||
chat.bottom_pane.composer_local_image_paths(),
|
||||
vec![retry_image, draft_image]
|
||||
);
|
||||
assert_eq!(
|
||||
chat.bottom_pane.composer_pending_pastes(),
|
||||
vec![(paste_placeholder.to_string(), "hello".to_string())]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn interrupted_turn_restore_keeps_active_mode_for_resubmission() {
|
||||
let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(Some("gpt-5")).await;
|
||||
|
||||
@@ -2071,62 +2071,6 @@ async fn queued_menu_slash_keeps_agent_turn_complete_notification() {
|
||||
assert_matches!(op_rx.try_recv(), Err(TryRecvError::Empty));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn slash_copy_uses_latest_surviving_response_after_rollback() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
replay_user_message_text(&mut chat, "user-1", "foo", ReplayKind::ThreadSnapshot);
|
||||
replay_agent_message(
|
||||
&mut chat,
|
||||
"agent-1",
|
||||
"foo response",
|
||||
ReplayKind::ThreadSnapshot,
|
||||
);
|
||||
replay_user_message_text(&mut chat, "user-2", "bar", ReplayKind::ThreadSnapshot);
|
||||
replay_agent_message(
|
||||
&mut chat,
|
||||
"agent-2",
|
||||
"bar response",
|
||||
ReplayKind::ThreadSnapshot,
|
||||
);
|
||||
let _ = drain_insert_history(&mut rx);
|
||||
assert_eq!(chat.last_agent_markdown_text(), Some("bar response"));
|
||||
|
||||
chat.truncate_agent_copy_history_to_user_turn_count(/*user_turn_count*/ 1);
|
||||
|
||||
assert_eq!(chat.last_agent_markdown_text(), Some("foo response"));
|
||||
chat.copy_last_agent_markdown_with(|markdown| {
|
||||
assert_eq!(markdown, "foo response");
|
||||
Ok(None)
|
||||
});
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn slash_copy_reports_when_rewind_exceeds_retained_copy_history() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
replay_user_message_text(&mut chat, "user-1", "foo", ReplayKind::ThreadSnapshot);
|
||||
replay_agent_message(
|
||||
&mut chat,
|
||||
"agent-1",
|
||||
"foo response",
|
||||
ReplayKind::ThreadSnapshot,
|
||||
);
|
||||
let _ = drain_insert_history(&mut rx);
|
||||
|
||||
chat.truncate_agent_copy_history_to_user_turn_count(/*user_turn_count*/ 0);
|
||||
chat.dispatch_command(SlashCommand::Copy);
|
||||
|
||||
let cells = drain_insert_history(&mut rx);
|
||||
let rendered = lines_to_single_string(&cells[0]);
|
||||
assert!(
|
||||
rendered.contains(
|
||||
"Cannot copy that response after rewinding. Only the most recent 32 responses are available to /copy."
|
||||
),
|
||||
"expected evicted-history message, got {rendered:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn slash_exit_requests_exit() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
@@ -1,30 +1,14 @@
|
||||
//! Transcript and active-cell bookkeeping for `ChatWidget`.
|
||||
|
||||
use super::HistoryCell;
|
||||
use super::MAX_AGENT_COPY_HISTORY;
|
||||
|
||||
#[derive(Debug)]
|
||||
pub(super) struct AgentTurnMarkdown {
|
||||
pub(super) user_turn_count: usize,
|
||||
pub(super) markdown: String,
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
pub(super) struct TranscriptState {
|
||||
pub(super) active_cell: Option<Box<dyn HistoryCell>>,
|
||||
/// Monotonic-ish counter used to invalidate transcript overlay caching.
|
||||
pub(super) active_cell_revision: u64,
|
||||
/// Raw markdown of the most recently completed agent response that
|
||||
/// survived any local thread rollback.
|
||||
/// Raw markdown of the most recently completed agent response.
|
||||
pub(super) last_agent_markdown: Option<String>,
|
||||
/// Copyable agent responses keyed by the number of visible user turns at
|
||||
/// the time the response completed.
|
||||
pub(super) agent_turn_markdowns: Vec<AgentTurnMarkdown>,
|
||||
/// Number of user turns currently reflected in the visible transcript.
|
||||
pub(super) visible_user_turn_count: usize,
|
||||
/// True when rollback discarded the requested copy source because it was
|
||||
/// older than the retained copy history.
|
||||
pub(super) copy_history_evicted_by_rollback: bool,
|
||||
/// Raw markdown of the most recently completed proposed plan.
|
||||
pub(super) latest_proposed_plan_markdown: Option<String>,
|
||||
/// Whether this turn already produced a copyable response.
|
||||
@@ -61,48 +45,12 @@ impl TranscriptState {
|
||||
}
|
||||
|
||||
pub(super) fn record_agent_markdown(&mut self, markdown: String) {
|
||||
match self.agent_turn_markdowns.last_mut() {
|
||||
Some(entry) if entry.user_turn_count == self.visible_user_turn_count => {
|
||||
entry.markdown = markdown.clone();
|
||||
}
|
||||
_ => {
|
||||
self.agent_turn_markdowns.push(AgentTurnMarkdown {
|
||||
user_turn_count: self.visible_user_turn_count,
|
||||
markdown: markdown.clone(),
|
||||
});
|
||||
if self.agent_turn_markdowns.len() > MAX_AGENT_COPY_HISTORY {
|
||||
self.agent_turn_markdowns.remove(0);
|
||||
}
|
||||
}
|
||||
}
|
||||
self.last_agent_markdown = Some(markdown);
|
||||
self.copy_history_evicted_by_rollback = false;
|
||||
self.saw_copy_source_this_turn = true;
|
||||
}
|
||||
|
||||
pub(super) fn record_visible_user_turn(&mut self) {
|
||||
self.visible_user_turn_count = self.visible_user_turn_count.saturating_add(1);
|
||||
}
|
||||
|
||||
pub(super) fn reset_copy_history(&mut self) {
|
||||
self.last_agent_markdown = None;
|
||||
self.agent_turn_markdowns.clear();
|
||||
self.visible_user_turn_count = 0;
|
||||
self.copy_history_evicted_by_rollback = false;
|
||||
self.saw_copy_source_this_turn = false;
|
||||
}
|
||||
|
||||
pub(super) fn truncate_copy_history_to_user_turn_count(&mut self, user_turn_count: usize) {
|
||||
self.visible_user_turn_count = user_turn_count;
|
||||
let had_copy_history = !self.agent_turn_markdowns.is_empty();
|
||||
self.agent_turn_markdowns
|
||||
.retain(|entry| entry.user_turn_count <= user_turn_count);
|
||||
self.last_agent_markdown = self
|
||||
.agent_turn_markdowns
|
||||
.last()
|
||||
.map(|entry| entry.markdown.clone());
|
||||
self.copy_history_evicted_by_rollback =
|
||||
had_copy_history && self.last_agent_markdown.is_none();
|
||||
self.saw_copy_source_this_turn = false;
|
||||
}
|
||||
|
||||
@@ -134,18 +82,4 @@ mod tests {
|
||||
|
||||
assert_eq!(state.active_cell_revision, 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn copy_history_tracks_latest_visible_turn() {
|
||||
let mut state = TranscriptState::default();
|
||||
state.record_visible_user_turn();
|
||||
state.record_agent_markdown("first".to_string());
|
||||
state.record_visible_user_turn();
|
||||
state.record_agent_markdown("second".to_string());
|
||||
|
||||
state.truncate_copy_history_to_user_turn_count(/*user_turn_count*/ 1);
|
||||
|
||||
assert_eq!(state.last_agent_markdown.as_deref(), Some("first"));
|
||||
assert!(!state.copy_history_evicted_by_rollback);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user