From dbf478850fb84b7d32b4b9d4c4df43aa8539be83 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Fri, 18 Sep 2026 01:31:32 +0000 Subject: [PATCH] Make TUI async question replies compatible with desktop (#46486) ## Why Async questions answered on another client should disappear from the TUI without losing drafts for other questions, even when questions have identical titles. ## What changed - Send answers using the desktop reply envelope with stable per-question IDs, and resolve matching questions from committed messages and replayed history. - Render replies as readable question-and-answer text in transcripts, queue previews, and input history. - Preserve separate reply envelopes and message order when retrying rejected or interrupted input. - Account for JSON escaping in input limits and fall back to plain text for oversized question IDs. ## Testing Add regression coverage for cross-client dismissal, draft preservation, replay ordering, reply parsing, IDE context, distinct replies with identical text, and retry ordering. Update the async question scenario to use the reply envelope. GitOrigin-RevId: 9b9e4b1e140508590401622d96cfc16fc192eb7c --- codex-rs/Cargo.lock | 1 + codex-rs/context-fragments/Cargo.toml | 1 + .../src/answered_question.rs | 38 +++- .../src/answered_question_tests.rs | 18 +- codex-rs/core/tests/suite/scenarios.rs | 3 +- ...rios__astra_async_question_and_answer.snap | 7 +- codex-rs/tui/src/async_question_reply.rs | 64 ++++++ .../tui/src/async_question_reply_tests.rs | 35 +++ .../src/bottom_pane/async_questions/mod.rs | 4 +- .../src/bottom_pane/async_questions/state.rs | 73 +++++- .../async_questions/state_tests.rs | 41 ++++ codex-rs/tui/src/bottom_pane/questions.rs | 2 +- codex-rs/tui/src/chatwidget/input_restore.rs | 51 +++-- .../tui/src/chatwidget/input_submission.rs | 9 +- codex-rs/tui/src/chatwidget/replay.rs | 9 + .../src/chatwidget/tests/questions_tests.rs | 215 +++++++++++++++++- ...c_question_answered_on_another_client.snap | 12 + ...s_tests__desktop_async_question_reply.snap | 9 + ...ns_tests__queued_async_question_reply.snap | 15 ++ codex-rs/tui/src/chatwidget/user_messages.rs | 34 ++- codex-rs/tui/src/ide_context/prompt.rs | 46 ++++ codex-rs/tui/src/lib.rs | 1 + codex-rs/tui/src/thread_transcript.rs | 11 +- 23 files changed, 639 insertions(+), 60 deletions(-) create mode 100644 codex-rs/tui/src/async_question_reply.rs create mode 100644 codex-rs/tui/src/async_question_reply_tests.rs create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__async_question_answered_on_another_client.snap create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__desktop_async_question_reply.snap create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__queued_async_question_reply.snap diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index a2c788e06d..3cfd47c489 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2966,6 +2966,7 @@ dependencies = [ "codex-protocol", "codex-utils-string", "pretty_assertions", + "serde_json", ] [[package]] diff --git a/codex-rs/context-fragments/Cargo.toml b/codex-rs/context-fragments/Cargo.toml index 936bd17e78..74ecc5f209 100644 --- a/codex-rs/context-fragments/Cargo.toml +++ b/codex-rs/context-fragments/Cargo.toml @@ -15,6 +15,7 @@ workspace = true [dependencies] codex-protocol = { workspace = true } codex-utils-string = { workspace = true } +serde_json = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } diff --git a/codex-rs/context-fragments/src/answered_question.rs b/codex-rs/context-fragments/src/answered_question.rs index 5f9ef8a6db..d6fe0b086a 100644 --- a/codex-rs/context-fragments/src/answered_question.rs +++ b/codex-rs/context-fragments/src/answered_question.rs @@ -1,23 +1,28 @@ -//! Bounded question framing accompanying an explicitly submitted user answer. +//! Async answers use the desktop's existing reply envelope and stable question identity. +//! Model-authored framing is bounded; oversized identities use the previous plain-text format. use super::ContextualUserFragment; use codex_protocol::models::ContentItemKind; /// Identifies an answered question without repeating an unbounded model-authored prompt. -pub struct AnsweredQuestion { +pub struct AnsweredQuestion<'a> { + question_id: Option<&'a str>, question: String, + answer: &'a str, } -impl AnsweredQuestion { - pub fn new(question: &str) -> Self { +impl<'a> AnsweredQuestion<'a> { + pub fn new(question_id: &'a str, question: &str, answer: &'a str) -> Self { let end = question.floor_char_boundary(question.len().min(512)); Self { - question: question[..end].to_string(), + question_id: (question_id.len() <= 512).then_some(question_id), + question: question[..end].replace(['\n', '\r'], " "), + answer, } } } -impl ContextualUserFragment for AnsweredQuestion { +impl ContextualUserFragment for AnsweredQuestion<'_> { fn content_kind(&self) -> ContentItemKind { ContentItemKind("user.answered_question".into()) } @@ -25,13 +30,28 @@ impl ContextualUserFragment for AnsweredQuestion { "user" } fn markers(&self) -> (&'static str, &'static str) { - Self::type_markers() + if self.question_id.is_some() { + Self::type_markers() + } else { + ("", "") + } } fn type_markers() -> (&'static str, &'static str) { - ("", "") + ( + "", + "", + ) } fn body(&self) -> String { - format!("> {}\n\n", self.question.replace(['\n', '\r'], " ")) + let Some(question_id) = self.question_id else { + return format!("> {}\n\n{}", self.question, self.answer); + }; + let replies = serde_json::json!([{ + "answer": self.answer, + "question": self.question, + "questionItemId": question_id, + }]); + format!("\n{replies}\n") } } diff --git a/codex-rs/context-fragments/src/answered_question_tests.rs b/codex-rs/context-fragments/src/answered_question_tests.rs index 34bc159de0..9151e39532 100644 --- a/codex-rs/context-fragments/src/answered_question_tests.rs +++ b/codex-rs/context-fragments/src/answered_question_tests.rs @@ -1,4 +1,4 @@ -//! Bounds and Unicode handling for model-authored question framing. +//! Bounds, Unicode handling, and escaping for async question replies. use super::*; use pretty_assertions::assert_eq; @@ -6,13 +6,15 @@ use pretty_assertions::assert_eq; #[test] fn question_context_is_bounded_and_keeps_unicode_boundaries() { let text = "é\n".repeat(1_000); - let rendered = AnsweredQuestion::new(&text).render(); - assert!(rendered.len() <= 516); + let id = r#"["request_user_input_async","message",1]"#; + let answer = "A \"quoted\" answer\nwith a second line"; + let fragment = AnsweredQuestion::new(id, &text, answer); assert_eq!( - rendered, - format!( - "> {}\n\n", - text[..text.floor_char_boundary(512)].replace('\n', " ") - ) + serde_json::from_str::(&fragment.body()).unwrap(), + serde_json::json!([{ + "questionItemId": id, + "question": text[..text.floor_char_boundary(512)].replace('\n', " "), + "answer": answer, + }]), ); } diff --git a/codex-rs/core/tests/suite/scenarios.rs b/codex-rs/core/tests/suite/scenarios.rs index 8e87dbdb53..e964ec1915 100644 --- a/codex-rs/core/tests/suite/scenarios.rs +++ b/codex-rs/core/tests/suite/scenarios.rs @@ -277,7 +277,8 @@ async fn astra_asks_an_async_question_and_receives_the_answer_while_working() -> }) .await; - let answer = format!("{}Customers", AnsweredQuestion::new(question).render()); + let question_id = json!(["request_user_input_async", "audience-question", 0]).to_string(); + let answer = AnsweredQuestion::new(&question_id, question, "Customers").render(); test.codex .steer_turn(TurnInputRequest::user_input(vec![text(&answer)]), turn_id) .await?; diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__astra_async_question_and_answer.snap b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__astra_async_question_and_answer.snap index 2643928dda..60938b0c66 100644 --- a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__astra_async_question_and_answer.snap +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__astra_async_question_and_answer.snap @@ -1,5 +1,6 @@ --- source: core/tests/suite/scenarios.rs +assertion_line: 304 expression: "context_snapshot::format_context_snapshot(\"Astra asks who a launch update is for, keeps working, and receives the user's answer in the active turn.\",\n&entries, &ContextSnapshotOptions::default().rewrite_known_segments(),)" --- Scenario: Astra asks who a launch update is for, keeps working, and receives the user's answer in the active turn. @@ -41,9 +42,9 @@ Scenario: Astra asks who a launch update is for, keeps working, and receives the 09:message/assistant: I drafted a short launch update. 10:message/user: - > Who should receive the launch update? - - Customers + + [{"answer":"Customers","question":"Who should receive the launch update?","questionItemId":"[\\"request_user_input_async\\",\\"audience-question\\",0]"}] + -- request 4 (request) -- 11:message/assistant: Here is the launch update for customers. diff --git a/codex-rs/tui/src/async_question_reply.rs b/codex-rs/tui/src/async_question_reply.rs new file mode 100644 index 0000000000..0930137d01 --- /dev/null +++ b/codex-rs/tui/src/async_question_reply.rs @@ -0,0 +1,64 @@ +//! Recognize the desktop's existing async-question reply envelope for display and dismissal. +//! Only complete envelopes, optionally following the standard IDE context prefix, are interpreted. + +use codex_app_server_protocol::UserInput; +use serde::Deserialize; + +#[derive(Debug, Deserialize, PartialEq)] +#[serde(rename_all = "camelCase")] +pub(crate) struct AsyncQuestionReply { + pub(crate) question_item_id: String, + question: String, + answer: String, +} + +pub(crate) fn parse(text: &str) -> Option> { + #[derive(Deserialize)] + #[serde(untagged)] + enum Replies { + Many(Vec), + One(AsyncQuestionReply), + } + let text = text.trim(); + // JSON strings escape newlines, so this cannot match a delimiter inside an answer. + let text = if text.starts_with("# Context from my IDE setup:\n") { + text.rsplit_once("\n## My request for Codex:\n")?.1.trim() + } else { + text + }; + let json = text + .strip_prefix("")? + .strip_suffix("")?; + let replies = match serde_json::from_str::(json).ok()? { + Replies::Many(replies) => replies, + Replies::One(reply) => vec![reply], + }; + (!replies.is_empty()).then_some(replies) +} + +pub(crate) fn display_text(text: &str) -> Option { + Some( + parse(text)? + .iter() + .map(|reply| format!("> {}\n\n{}", reply.question, reply.answer)) + .collect::>() + .join("\n\n"), + ) +} + +pub(crate) fn parse_input(input: &[UserInput]) -> Option> { + let mut content = input + .iter() + .filter(|item| !matches!(item, UserInput::Skill { .. } | UserInput::Mention { .. })); + let UserInput::Text { text, .. } = content.next()? else { + return None; + }; + if content.next().is_some() { + return None; + } + parse(text) +} + +#[cfg(test)] +#[path = "async_question_reply_tests.rs"] +mod tests; diff --git a/codex-rs/tui/src/async_question_reply_tests.rs b/codex-rs/tui/src/async_question_reply_tests.rs new file mode 100644 index 0000000000..95b720d869 --- /dev/null +++ b/codex-rs/tui/src/async_question_reply_tests.rs @@ -0,0 +1,35 @@ +use super::*; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[test] +fn desktop_question_reply_accepts_single_and_batched_envelopes() { + let reply = json!({"questionItemId": "[\"request_user_input_async\",\"item\",0]", "question": "Which environment?", "answer": "Staging", "extra": true}); + for payload in [reply.clone(), json!([reply])] { + let text = format!( + " \n\n{payload}\n\n " + ); + assert_eq!( + display_text(&text).as_deref(), + Some("> Which environment?\n\nStaging") + ); + } + let text = "[{\"questionItemId\":\"one\",\"question\":\"First?\",\"answer\":\"Yes\"},{\"questionItemId\":\"two\",\"question\":\"Second?\",\"answer\":\"No\"}]"; + assert_eq!( + display_text(text).as_deref(), + Some("> First?\n\nYes\n\n> Second?\n\nNo") + ); +} + +#[test] +fn malformed_or_embedded_question_envelopes_remain_ordinary_text() { + for text in [ + "> Which environment?\n\nStaging", + "[]", + "[{\"questionItemId\":\"one\",\"question\":\"First?\",\"answer\":\"Yes\"},null]", + "Quoted: {\"questionItemId\":\"one\",\"question\":\"First?\",\"answer\":\"Yes\"}", + "{\"questionItemId\":\"one\",\"question\":\"First?\",\"answer\":\"Yes\"} trailing text", + ] { + assert_eq!(parse(text), None, "{text}"); + } +} diff --git a/codex-rs/tui/src/bottom_pane/async_questions/mod.rs b/codex-rs/tui/src/bottom_pane/async_questions/mod.rs index 0de055ff45..47f6d92693 100644 --- a/codex-rs/tui/src/bottom_pane/async_questions/mod.rs +++ b/codex-rs/tui/src/bottom_pane/async_questions/mod.rs @@ -1,5 +1,5 @@ //! Inline editing for asynchronous questions. Legacy request_user_input keeps its own overlay. -//! Only locally accepted submissions remove questions; arrival and expiry never steal focus. +//! Local submissions and committed desktop replies remove questions; arrival never steals focus. use crate::app_event_sender::AppEventSender; use crate::bottom_pane::CancellationEvent; @@ -36,6 +36,7 @@ pub(super) const DESIRED_SPACERS_BETWEEN_SECTIONS: u16 = 2; #[derive(Debug, Clone, PartialEq)] struct PendingQuestion { message_id: String, + question_id: String, question: AsyncUserInputQuestion, options_state: ScrollState, draft: ComposerDraft, @@ -49,6 +50,7 @@ pub(crate) struct QuestionState { current_idx: usize, expanded: bool, seen_ids: HashSet, + answered_ids: HashSet, } pub(crate) enum QuestionSubmission { diff --git a/codex-rs/tui/src/bottom_pane/async_questions/state.rs b/codex-rs/tui/src/bottom_pane/async_questions/state.rs index 0a4a82effa..4e33b14640 100644 --- a/codex-rs/tui/src/bottom_pane/async_questions/state.rs +++ b/codex-rs/tui/src/bottom_pane/async_questions/state.rs @@ -1,4 +1,4 @@ -//! Pending async questions are local drafts; handling one removes it immediately. +//! Pending async questions retain drafts until handled locally or answered by another client. //! Message IDs survive removal so replay cannot reopen an answered or skipped question. use super::*; @@ -12,7 +12,15 @@ impl AsyncQuestions { } let was_empty = self.state.pending.is_empty(); let expires_at = (!self.expanded).then(|| Instant::now() + Duration::from_secs(30)); - self.state.pending.extend(questions.iter().map(|question| { + for (index, question) in questions.iter().enumerate() { + // Match the desktop's JSON.stringify([tool name, item id, question index]). + let question_id = + serde_json::json!(["request_user_input_async", message_id, index]).to_string(); + if self.state.answered_ids.contains(&question_id) + || self.state.answered_ids.contains(message_id) + { + continue; + } // Bound work before cloning or wrapping model-authored suggestions. let question = AsyncUserInputQuestion { title: question.title.clone(), @@ -31,14 +39,15 @@ impl AsyncQuestions { .is_some_and(|options| !options.is_empty()); let mut options_state = ScrollState::new(); options_state.selected_idx = has_options.then_some(0); - PendingQuestion { + self.state.pending.push(PendingQuestion { message_id: message_id.into(), + question_id, question, options_state, draft: ComposerDraft::default(), expires_at, - } - })); + }); + } if was_empty { self.state.current_idx = 0; self.restore_current_draft(); @@ -120,15 +129,54 @@ impl AsyncQuestions { selected.to_string() }; let text = text.trim(); - let framing = AnsweredQuestion::new(&answer.question.title).render(); - let limit = codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS - framing.chars().count(); - if text.chars().count() > limit { + if text.is_empty() { + return; + } + let reply = + AnsweredQuestion::new(&answer.question_id, &answer.question.title, text).render(); + if reply.chars().count() > codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS { self.composer.show_footer_flash( - format!("Answer too long; limit {limit} characters").into(), + "Answer too long; shorten it before sending".into(), Duration::from_secs(5), ); - } else if !text.is_empty() { - self.submission = Some(QuestionSubmission::Submit(format!("{framing}{text}"))); + } else { + self.submission = Some(QuestionSubmission::Submit(reply)); + } + } + + pub(crate) fn resolve_answers(&mut self, question_ids: &[String]) { + // History can arrive before live questions or before restoring local drafts. + self.state.answered_ids.extend(question_ids.iter().cloned()); + // Older desktop replies identify the whole source message instead of one question. + let answered = |question: &PendingQuestion| { + question_ids.contains(&question.question_id) + || question_ids.contains(&question.message_id) + }; + if !self.state.pending.iter().any(answered) { + return; + } + let current_answered = self.current_answer().is_some_and(answered); + if current_answered { + self.composer.flush_pending_input(); + } + let current_idx = self + .state + .pending + .iter() + .take(self.state.current_idx) + .filter(|question| !answered(question)) + .count(); + self.state.pending.retain(|question| !answered(question)); + self.state.current_idx = if current_idx < self.state.pending.len() { + current_idx + } else { + 0 + }; + self.expanded &= !self.state.pending.is_empty(); + self.visible_options.set((0, 0)); + if current_answered { + self.restore_current_draft(); + self.composer.reset_vim_mode(); } } @@ -168,6 +216,8 @@ impl AsyncQuestions { pub(crate) fn restore(&mut self, saved: QuestionState) { self.visible_options.set((0, 0)); let incoming = std::mem::replace(&mut self.state, saved); + let mut answered_ids = incoming.answered_ids; + answered_ids.extend(self.state.answered_ids.iter().cloned()); self.state.pending.extend( incoming .pending @@ -180,5 +230,6 @@ impl AsyncQuestions { self.snooze_auto_resolution(); } self.restore_current_draft(); + self.resolve_answers(&answered_ids.into_iter().collect::>()); } } diff --git a/codex-rs/tui/src/bottom_pane/async_questions/state_tests.rs b/codex-rs/tui/src/bottom_pane/async_questions/state_tests.rs index d9929d89e3..354a260127 100644 --- a/codex-rs/tui/src/bottom_pane/async_questions/state_tests.rs +++ b/codex-rs/tui/src/bottom_pane/async_questions/state_tests.rs @@ -392,3 +392,44 @@ fn question_navigation_resets_history_recall() { editor.handle_key_event(KeyCode::Up.into()); assert_eq!(editor.composer.current_text(), "newer"); } + +#[test] +fn answered_questions_do_not_reopen_when_history_precedes_local_drafts() { + let mut original = editor(); + let saved = original.capture(); + original.resolve_answers(&["unknown".into()]); + assert_eq!(original.state.pending, saved.pending); + let mut restored = editor(); + restored.state = QuestionState::default(); + restored.resolve_answers(&[r#"["request_user_input_async","message",0]"#.into()]); + restored.append( + "message", + &[ + question("First", /*options*/ None), + question("Second", /*options*/ None), + ], + ); + assert_eq!(restored.unanswered_count(), 1); + restored.restore(saved); + assert_eq!(restored.state.pending, original.state.pending[1..]); + // Legacy desktop replies refer to the entire source message. + restored.resolve_answers(&["message".into()]); + assert_eq!(restored.unanswered_count(), 0); + assert!(restored.composer.is_empty()); +} + +#[test] +fn oversized_question_ids_keep_questions_answerable_without_echoing_the_id() { + let mut editor = editor(); + editor.clear_pending(); + editor.append(&"x".repeat(1024), &[question("Question", /*options*/ None)]); + assert_eq!(editor.unanswered_count(), 1); + editor.set_expanded(/*expanded*/ true); + editor.handle_paste("Answer".into()); + render_editor(&editor, /*width*/ 80, /*height*/ 20); + editor.go_next_or_submit(); + let Some(QuestionSubmission::Submit(reply)) = editor.submission else { + panic!("submitted answer"); + }; + assert_eq!(reply, "> Question\n\nAnswer"); +} diff --git a/codex-rs/tui/src/bottom_pane/questions.rs b/codex-rs/tui/src/bottom_pane/questions.rs index 7645406e6a..c1053995f8 100644 --- a/codex-rs/tui/src/bottom_pane/questions.rs +++ b/codex-rs/tui/src/bottom_pane/questions.rs @@ -20,7 +20,7 @@ impl BottomPane { } } - fn question_editor(&mut self) -> &mut AsyncQuestions { + pub(crate) fn question_editor(&mut self) -> &mut AsyncQuestions { self.questions.get_or_insert_with(|| { let mut questions = AsyncQuestions::new( self.app_event_tx.clone(), diff --git a/codex-rs/tui/src/chatwidget/input_restore.rs b/codex-rs/tui/src/chatwidget/input_restore.rs index f6b702c49f..6377809eba 100644 --- a/codex-rs/tui/src/chatwidget/input_restore.rs +++ b/codex-rs/tui/src/chatwidget/input_restore.rs @@ -196,15 +196,23 @@ impl ChatWidget { (user_message, history_record) }) } else { + // Reply envelopes must remain separate messages so other clients can read them. + let count = self + .input_queue + .rejected_steers_queue + .iter() + .position(|message| crate::async_question_reply::parse(&message.text).is_some()) + .map(|index| index.max(/*other*/ 1)) + .unwrap_or(self.input_queue.rejected_steers_queue.len()); let rejected_messages = self .input_queue .rejected_steers_queue - .drain(..) + .drain(..count) .collect::>(); let sources = self .input_queue .rejected_steer_sources - .drain(..) + .drain(..count.min(self.input_queue.rejected_steer_sources.len())) .collect::>(); let source = if !rejected_messages.is_empty() && sources.len() == rejected_messages.len() @@ -219,7 +227,7 @@ impl ChatWidget { let mut history_records = self .input_queue .rejected_steer_history_records - .drain(..) + .drain(..count.min(self.input_queue.rejected_steer_history_records.len())) .collect::>(); history_records.resize( rejected_messages.len(), @@ -329,15 +337,32 @@ impl ChatWidget { .pending_steers .drain(..) .collect::>(); - if !pending_steers.is_empty() { - let source = if pending_steers - .iter() - .all(|pending| pending.source == UserMessageSource::QuestionAnswer) - { - UserMessageSource::QuestionAnswer - } else { - UserMessageSource::Prompt - }; + if pending_steers + .iter() + .any(|pending| pending.source == UserMessageSource::QuestionAnswer) + { + // Keep answers intact when an interrupt retries uncommitted input. + for pending in pending_steers { + self.input_queue + .rejected_steers_queue + .push_back(pending.user_message); + self.input_queue + .rejected_steer_sources + .push_back(pending.source); + self.input_queue + .rejected_steer_history_records + .push_back(pending.history_record); + } + if let Some((message, history_record)) = self.pop_next_queued_user_message() { + let source = message.source; + self.submit_user_message_with_history_and_shell_escape_policy( + message.into_user_message(), + history_record, + ShellEscapePolicy::Allow, + source, + ); + } + } else if !pending_steers.is_empty() { let (user_message, history_record) = merge_user_messages_with_history_record( pending_steers .into_iter() @@ -348,7 +373,7 @@ impl ChatWidget { user_message, history_record, ShellEscapePolicy::Allow, - source, + UserMessageSource::Prompt, ); } else if let Some(combined) = self.drain_pending_messages_for_restore() { self.restore_composer_state(combined); diff --git a/codex-rs/tui/src/chatwidget/input_submission.rs b/codex-rs/tui/src/chatwidget/input_submission.rs index 211b66ed54..39d33426cc 100644 --- a/codex-rs/tui/src/chatwidget/input_submission.rs +++ b/codex-rs/tui/src/chatwidget/input_submission.rs @@ -282,7 +282,9 @@ impl ChatWidget { .retain(|binding| crate::task_mentions::valid_thread_path(&binding.path).is_none()); } - let mentions = collect_tool_mentions(&text, &HashMap::new()); + let reply_text = crate::async_question_reply::display_text(&text); + let mentions = + collect_tool_mentions(reply_text.as_deref().unwrap_or(&text), &HashMap::new()); let bound_names: HashSet = mention_bindings .iter() .map(|binding| binding.mention.clone()) @@ -513,6 +515,11 @@ impl ChatWidget { } }; if let Some((text, elements)) = history { + let reply_text = crate::async_question_reply::display_text(text); + let (text, elements) = match &reply_text { + Some(text) => (text.as_str(), &[][..]), + None => (text.as_str(), elements), + }; self.append_message_history_entry(encode_history_mentions_at_elements( text, &encoded_mentions, diff --git a/codex-rs/tui/src/chatwidget/replay.rs b/codex-rs/tui/src/chatwidget/replay.rs index 49e9f0bf0c..ba23b504b1 100644 --- a/codex-rs/tui/src/chatwidget/replay.rs +++ b/codex-rs/tui/src/chatwidget/replay.rs @@ -260,6 +260,15 @@ impl ChatWidget { ThreadItem::UserMessage { content, client_id, .. } => { + if let Some(replies) = crate::async_question_reply::parse_input(&content) { + let ids = replies + .into_iter() + .map(|reply| reply.question_item_id) + .collect::>(); + self.bottom_pane.question_editor().resolve_answers(&ids); + self.refresh_pending_input_preview(); + self.request_redraw(); + } self.on_committed_user_message( &content, client_id.as_deref(), diff --git a/codex-rs/tui/src/chatwidget/tests/questions_tests.rs b/codex-rs/tui/src/chatwidget/tests/questions_tests.rs index 81e84ba8da..2db8a938da 100644 --- a/codex-rs/tui/src/chatwidget/tests/questions_tests.rs +++ b/codex-rs/tui/src/chatwidget/tests/questions_tests.rs @@ -47,10 +47,12 @@ async fn accepted_question_answer_uses_existing_delivery_and_keeps_main_draft() assert_eq!(chat.bottom_pane.composer_text(), "main draft"); let expected = "> Which way?\n\n!literal answer"; if queued { - assert_eq!( - chat.input_queue.queued_user_messages.front().unwrap().text, - expected + insta::assert_snapshot!( + "queued_async_question_reply", + render_bottom_popup(&chat, /*width*/ 80) ); + let restored = chat.pop_latest_queued_composer_state().unwrap(); + assert_eq!(restored.text, expected); assert!(op_rx.try_recv().is_err()); } else { assert_answer(op_rx.try_recv().unwrap(), expected); @@ -58,6 +60,54 @@ async fn accepted_question_answer_uses_existing_delivery_and_keeps_main_draft() } } +#[tokio::test] +async fn async_question_answers_preserve_ambiguous_skill_selection_and_dismiss_remotely() { + use codex_context_fragments::AnsweredQuestion; + use codex_context_fragments::ContextualUserFragment; + + let (mut chat, _rx, mut ops) = make_chatwidget_manual(/*model_override*/ None).await; + chat.thread_id = Some(ThreadId::new()); + let skill = SkillMetadata { + name: "route".into(), + description: "Choose a route".into(), + short_description: None, + interface: None, + dependencies: None, + path: test_path_buf("/tmp/route/SKILL.md").abs(), + scope: crate::test_support::skill_scope_repo(), + enabled: true, + plugin_id: None, + }; + let mut duplicate = skill.clone(); + duplicate.path = test_path_buf("/tmp/other-route/SKILL.md").abs(); + chat.set_skills(Some(vec![skill.clone(), duplicate])); + chat.add_async_questions("message", &questions()); + chat.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::ALT)); + chat.bottom_pane.handle_paste("Use $route".into()); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + let Op::UserTurn { items, .. } = ops.try_recv().unwrap() else { + panic!("user turn") + }; + let id = serde_json::json!(["request_user_input_async", "message", 0]).to_string(); + assert_eq!( + items, + vec![ + UserInput::Text { + text: AnsweredQuestion::new(&id, "Which way?", "Use $route").render(), + text_elements: Vec::new(), + }, + UserInput::Skill { + name: skill.name, + path: skill.path.to_path_buf() + }, + ] + ); + let (mut other, _rx, _ops) = make_chatwidget_manual(/*model_override*/ None).await; + other.add_async_questions("message", &questions()); + complete_user_message_for_inputs(&mut other, "answer", items); + assert_eq!(question_count(&other), 1); +} + #[tokio::test] async fn ordinary_follow_up_clears_unanswered_questions_after_accepted_input() { for queued in [false, true] { @@ -388,12 +438,14 @@ async fn selected_answers_preserve_long_labels_and_reject_oversized_submissions( if length > 512 { assert_eq!(question_count(&chat), saved); assert!(ops.try_recv().is_err()); - chat.bottom_pane - .handle_paste("x".repeat(codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS)); + // JSON escaping must count toward the limit even when the answer itself fits. + chat.bottom_pane.handle_paste( + "\"".repeat(codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS / 2), + ); chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); assert_eq!(question_count(&chat), saved); let rendered = render_bottom_popup(&chat, /*width*/ 80); - insta::assert_snapshot!(rendered.lines().find(|line| line.contains("Answer too long")).unwrap(), @" Answer too long; limit 1048562 characters"); + insta::assert_snapshot!(rendered.lines().find(|line| line.contains("Answer too long")).unwrap(), @" Answer too long; shorten it before sending"); } else { assert_answer(ops.try_recv().unwrap(), &format!("> What next?\n\n{label}")); } @@ -614,7 +666,10 @@ async fn question_queue_key_does_not_steer_the_running_turn() { chat.bottom_pane.handle_paste(" later ".into()); chat.handle_key_event(KeyEvent::from(KeyCode::Tab)); assert_eq!( - chat.input_queue.queued_user_messages.front().unwrap().text, + crate::async_question_reply::display_text( + &chat.input_queue.queued_user_messages.front().unwrap().text + ) + .unwrap(), "> First?\n\nlater" ); let mut repeat = KeyEvent::from(KeyCode::Tab); @@ -773,6 +828,10 @@ fn assert_answer(op: Op, expected: &str) { let Op::UserTurn { items, .. } = op else { panic!("user turn") }; + let mut items = items; + if let [UserInput::Text { text, .. }] = items.as_mut_slice() { + *text = crate::async_question_reply::display_text(text).unwrap_or_else(|| text.clone()); + } assert_eq!( items, vec![UserInput::Text { @@ -812,3 +871,145 @@ async fn questions_and_queued_messages_share_the_resolved_shortcut() { chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); } } + +#[tokio::test] +async fn desktop_async_answer_dismisses_only_its_question_and_preserves_the_other_draft() { + for replay_kind in [None, Some(ReplayKind::ThreadSnapshot)] { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + let questions = vec![ + question("Same title?", /*options*/ None), + question("Same title?", /*options*/ None), + ]; + chat.add_async_questions("questions", &questions); + chat.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::ALT)); + chat.bottom_pane + .questions + .as_mut() + .unwrap() + .navigate(/*forward*/ true); + chat.bottom_pane + .handle_paste("My draft for the second question".into()); + let reply = r#" +[{"questionItemId":"[\"request_user_input_async\",\"questions\",0]","question":"Same title?","answer":"Answer from desktop"}] +"#; + let reply = format!( + "# Context from my IDE setup:\n\n## Open tabs:\n- lib.rs: src/lib.rs\n\n## My request for Codex:\n{reply}" + ); + let item = AppServerThreadItem::UserMessage { + id: "answer".into(), + client_id: None, + content: vec![UserInput::Text { + text: reply, + text_elements: Vec::new(), + }], + }; + chat.handle_server_notification( + ServerNotification::ItemCompleted(ItemCompletedNotification { + thread_id: "thread".into(), + turn_id: "turn".into(), + completed_at_ms: 0, + item: item.clone(), + }), + replay_kind, + ); + assert_eq!(question_count(&chat), 1); + let cells = crate::thread_transcript::thread_items_to_transcript_cells( + /*thread_id*/ None, + &chat.config.cwd, + [item], + crate::thread_transcript::RawReasoningVisibility::Hidden, + /*config*/ None, + ); + insta::assert_snapshot!( + "desktop_async_question_reply", + lines_to_single_string(&cells[0].transcript_lines(/*width*/ 80)) + ); + insta::assert_snapshot!( + "async_question_answered_on_another_client", + render_bottom_popup(&chat, /*width*/ 80) + ); + chat.thread_id = Some(ThreadId::new()); + chat.input_queue.suppress_queue_autosend = true; + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + assert_eq!(question_count(&chat), 0); + // Removing the first question must not renumber the surviving question's identity. + let queued = &chat.input_queue.queued_user_messages.front().unwrap().text; + let replies = crate::async_question_reply::parse(queued).unwrap(); + assert_eq!( + replies[0].question_item_id, + r#"["request_user_input_async","questions",1]"# + ); + assert_eq!( + crate::async_question_reply::display_text(queued).unwrap(), + "> Same title?\n\nMy draft for the second question" + ); + } +} + +#[tokio::test] +async fn distinct_async_question_replies_with_identical_text_both_render() { + use codex_context_fragments::AnsweredQuestion; + use codex_context_fragments::ContextualUserFragment; + + let (mut chat, mut rx, _ops) = make_chatwidget_manual(/*model_override*/ None).await; + drain_insert_history(&mut rx); + for id in ["first", "first", "second"] { + let items = [UserInput::Text { + text: AnsweredQuestion::new(id, "Continue?", "Yes").render(), + text_elements: Vec::new(), + }]; + chat.on_committed_user_message( + &items, /*client_id*/ None, /*from_replay*/ false, "turn", + ); + } + let history = drain_insert_history(&mut rx); + assert_eq!(history.len(), 2); +} + +#[tokio::test] +async fn retried_question_answers_keep_separate_envelopes_and_order() { + for interrupt in [false, true] { + let (mut chat, _rx, mut ops) = make_chatwidget_manual(/*model_override*/ None).await; + chat.thread_id = Some(ThreadId::new()); + chat.on_task_started(); + chat.submit_user_message(UserMessage::from("before")); + chat.add_async_questions("message", &questions()); + chat.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::ALT)); + for answer in ["North", "Bring a map"] { + chat.bottom_pane.handle_paste(answer.into()); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + } + chat.submit_user_message(UserMessage::from("after")); + let mut original = Vec::new(); + while let Ok(op) = ops.try_recv() { + let Op::UserTurn { items, .. } = op else { + panic!("user turn") + }; + let [UserInput::Text { text, .. }] = items.as_slice() else { + panic!("one text input") + }; + original.push(text.clone()); + } + assert_eq!(original.len(), 4); + let mut retried = Vec::new(); + if interrupt { + chat.input_queue.submit_pending_steers_after_interrupt = true; + chat.on_interrupted_turn(TurnAbortReason::Interrupted); + let Op::UserTurn { items, .. } = ops.try_recv().unwrap() else { + panic!("user turn") + }; + let [UserInput::Text { text, .. }] = items.as_slice() else { + panic!("one text input") + }; + retried.push(text.clone()); + } else { + while !chat.input_queue.pending_steers.is_empty() { + assert!(chat.enqueue_rejected_steer()); + } + } + while let Some((message, _)) = chat.pop_next_queued_user_message() { + retried.push(message.text.clone()); + } + assert_eq!(retried, original); + } +} diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__async_question_answered_on_another_client.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__async_question_answered_on_another_client.snap new file mode 100644 index 0000000000..f86a62f1aa --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__async_question_answered_on_another_client.snap @@ -0,0 +1,12 @@ +--- +source: tui/src/chatwidget/tests/questions_tests.rs +assertion_line: 859 +expression: "render_bottom_popup(&chat, 80)" +--- +• Queued follow-up inputs + + Same title? + + My draft for the second question + + enter submit ctrl + ] skip ⌥ + ↓ main prompt diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__desktop_async_question_reply.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__desktop_async_question_reply.snap new file mode 100644 index 0000000000..0e6dcf8d13 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__desktop_async_question_reply.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/chatwidget/tests/questions_tests.rs +assertion_line: 856 +expression: "lines_to_single_string(&cells[0].transcript_lines(80))" +--- + +› > Same title? + + Answer from desktop diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__queued_async_question_reply.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__queued_async_question_reply.snap new file mode 100644 index 0000000000..e8ca16fffa --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__questions_tests__queued_async_question_reply.snap @@ -0,0 +1,15 @@ +--- +source: tui/src/chatwidget/tests/questions_tests.rs +assertion_line: 55 +expression: "render_bottom_popup(&chat, 80)" +--- +• Queued follow-up inputs + ↳ > Which way? + + !literal answer + + Any details? + + Type your answer + + enter submit ctrl + ] skip ⌥ + ↓ main prompt ⌥ + ↑ queued messages diff --git a/codex-rs/tui/src/chatwidget/user_messages.rs b/codex-rs/tui/src/chatwidget/user_messages.rs index 21f5175b4a..533e0eaf02 100644 --- a/codex-rs/tui/src/chatwidget/user_messages.rs +++ b/codex-rs/tui/src/chatwidget/user_messages.rs @@ -453,7 +453,7 @@ fn merge_remapped_user_messages(messages: impl IntoIterator) } pub(super) fn user_message_for_restore( - message: UserMessage, + mut message: UserMessage, history_record: &UserMessageHistoryRecord, ) -> UserMessage { match history_record { @@ -463,6 +463,10 @@ pub(super) fn user_message_for_restore( ..message }, UserMessageHistoryRecord::Override(_) | UserMessageHistoryRecord::UserMessageText => { + if let Some(text) = crate::async_question_reply::display_text(&message.text) { + message.text = text; + message.text_elements.clear(); + } message } } @@ -478,7 +482,8 @@ pub(super) fn user_message_preview_text( } Some(UserMessageHistoryRecord::Override(_)) | Some(UserMessageHistoryRecord::UserMessageText) - | None => message.text.clone(), + | None => crate::async_question_reply::display_text(&message.text) + .unwrap_or_else(|| message.text.clone()), } } @@ -486,7 +491,10 @@ pub(super) fn user_message_display_for_history( message: UserMessage, history_record: &UserMessageHistoryRecord, ) -> UserMessageDisplay { - let message = user_message_for_restore(message, history_record); + let message = match history_record { + UserMessageHistoryRecord::UserMessageText => message, + UserMessageHistoryRecord::Override(_) => user_message_for_restore(message, history_record), + }; ChatWidget::user_message_display_from_parts( message.text, message.text_elements, @@ -551,6 +559,8 @@ pub(super) fn merge_user_messages_with_history_record( #[derive(Clone, Debug, PartialEq)] pub(crate) struct UserMessageDisplay { pub(crate) message: String, + // Keep distinct replies distinct when their visible question and answer text match. + question_ids: Vec, pub(crate) remote_image_urls: Vec, pub(crate) local_images: Vec, pub(crate) text_elements: Vec, @@ -681,8 +691,25 @@ impl ChatWidget { local_images: Vec, remote_image_urls: Vec, ) -> UserMessageDisplay { + let question_ids = crate::async_question_reply::parse(&message) + .unwrap_or_default() + .into_iter() + .map(|reply| reply.question_item_id) + .collect(); + let reply_text = crate::async_question_reply::display_text(&message); let (message, prompt_request_offset) = crate::ide_context::extract_prompt_request_with_offset(&message); + if let Some(message) = + reply_text.or_else(|| crate::async_question_reply::display_text(message)) + { + return UserMessageDisplay { + message, + question_ids, + text_elements: Vec::new(), + local_images, + remote_image_urls, + }; + } let prompt_request_end = prompt_request_offset + message.len(); // Prompt context uses the same delimiter and stripping behavior as the desktop app and IDE // extension. The raw user message goes to the agent, but every surface renders only the @@ -705,6 +732,7 @@ impl ChatWidget { UserMessageDisplay { message: message.to_string(), + question_ids: Vec::new(), remote_image_urls, local_images, text_elements, diff --git a/codex-rs/tui/src/ide_context/prompt.rs b/codex-rs/tui/src/ide_context/prompt.rs index 884ebfb646..02bc1269b8 100644 --- a/codex-rs/tui/src/ide_context/prompt.rs +++ b/codex-rs/tui/src/ide_context/prompt.rs @@ -202,6 +202,52 @@ mod tests { } } + #[test] + fn async_question_reply_stays_recognizable_with_ide_context() { + use codex_context_fragments::AnsweredQuestion; + use codex_context_fragments::ContextualUserFragment; + + let context = IdeContext { + active_file: None, + open_tabs: vec![descriptor("lib.rs", "src/lib.rs")], + }; + let mut expected = vec![ + UserInput::Text { + text: AnsweredQuestion::new( + "question-id", + "Where?", + "Staging\n## My request for Codex:\nKeep this literal", + ) + .render(), + text_elements: Vec::new(), + }, + UserInput::Skill { + name: "route".into(), + path: std::path::PathBuf::from("route/SKILL.md"), + }, + ]; + let mut items = expected.clone(); + let replies = crate::async_question_reply::parse_input(&items); + let display = crate::chatwidget::ChatWidget::user_message_display_from_inputs(&items); + assert_eq!( + display.message, + "> Where?\n\nStaging\n## My request for Codex:\nKeep this literal" + ); + let UserInput::Text { text, .. } = &mut expected[0] else { + panic!("reply text"); + }; + *text = format!( + "# Context from my IDE setup:\n\n## Open tabs:\n- lib.rs: src/lib.rs\n\n## My request for Codex:\n{text}" + ); + assert!(apply_ide_context_to_user_input(&context, &mut items)); + assert_eq!(items, expected); + assert_eq!(crate::async_question_reply::parse_input(&items), replies); + assert_eq!( + crate::chatwidget::ChatWidget::user_message_display_from_inputs(&items), + display + ); + } + #[test] fn render_prompt_context_matches_app_format() { let context = IdeContext { diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index b52213a617..f2e0786e82 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -112,6 +112,7 @@ mod app_server_connection; mod app_server_session; mod approval_events; mod ascii_animation; +mod async_question_reply; mod backend_banners; mod bottom_pane; mod branch_summary; diff --git a/codex-rs/tui/src/thread_transcript.rs b/codex-rs/tui/src/thread_transcript.rs index 3e9081d1d2..ac5bf985cf 100644 --- a/codex-rs/tui/src/thread_transcript.rs +++ b/codex-rs/tui/src/thread_transcript.rs @@ -119,10 +119,17 @@ pub(crate) fn thread_items_to_transcript_cells( .map(codex_app_server_protocol::UserInput::into_core) .collect(), }; + let message = item.message(); + let reply_text = crate::async_question_reply::display_text(&message); + let text_elements = if reply_text.is_some() { + Vec::new() + } else { + item.text_elements() + }; cells.push(Arc::new(UserHistoryCell { spoken: false, - message: item.message(), - text_elements: item.text_elements(), + message: reply_text.unwrap_or(message), + text_elements, local_image_paths: item.local_image_paths(), remote_image_urls: item.image_urls(), }));