From a09812981de703f18f38015cd5d500ab373f616c Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sun, 1 Nov 2026 23:12:19 +0000 Subject: [PATCH] Validate `/fork` before consuming the composer draft (#58767) ## Why MCP startup alone should not block `/fork`, and rejecting a fork while foreground work is pending should preserve the draft and its image attachments. ## What changed - Defer `/fork` draft consumption until dispatch validates the thread state, as for `/review`. - Allow `/fork` during MCP startup when no user turn or review is pending or running, and clear the composer after validation succeeds. - Preserve drafts and attachments when these commands are rejected in side conversations. ## Testing Extend regression coverage for `/fork` and `/review` draft preservation during MCP startup, successful fork dispatch and composer clearing, rejection during an active review, and attachment preservation in side conversations. GitOrigin-RevId: d152ba6659e4507d8750cb6bd3073d9f8b293538 --- codex-rs/tui/src/chatwidget/slash_dispatch.rs | 25 ++++++++++------- .../tui/src/chatwidget/tests/mcp_startup.rs | 28 ++++++++++++++++--- codex-rs/tui/src/chatwidget/tests/side.rs | 9 ++++++ codex-rs/tui/src/slash_command.rs | 2 +- 4 files changed, 49 insertions(+), 15 deletions(-) diff --git a/codex-rs/tui/src/chatwidget/slash_dispatch.rs b/codex-rs/tui/src/chatwidget/slash_dispatch.rs index 44f3c879e2..cd8131f782 100644 --- a/codex-rs/tui/src/chatwidget/slash_dispatch.rs +++ b/codex-rs/tui/src/chatwidget/slash_dispatch.rs @@ -139,6 +139,12 @@ impl ChatWidget { cmd: SlashCommand, source: SlashCommandDispatchSource, ) -> bool { + if source == SlashCommandDispatchSource::Live && cmd.requires_dispatch_validation() { + return self.is_user_turn_pending_or_running() + || (cmd == SlashCommand::Review + && self.input_queue.has_queued_follow_up_messages()); + } + (!cmd.available_during_task() && (self.turn_lifecycle.agent_turn_running || self.review.is_review_mode @@ -149,10 +155,6 @@ impl ChatWidget { && (self.input_queue.user_turn_pending_start || self.turn_lifecycle.agent_turn_running)) || (cmd == SlashCommand::Export && self.input_queue.suppress_queue_autosend) - || (cmd == SlashCommand::Review - && source == SlashCommandDispatchSource::Live - && (self.is_user_turn_pending_or_running() - || self.input_queue.has_queued_follow_up_messages())) } pub(super) fn dispatch_command(&mut self, cmd: SlashCommand) { @@ -187,6 +189,12 @@ impl ChatWidget { return; } + if source == SlashCommandDispatchSource::Live && cmd.requires_dispatch_validation() { + self.bottom_pane + .set_composer_text(String::new(), Vec::new(), Vec::new()); + self.bottom_pane.drain_pending_submission_state(); + } + match cmd { SlashCommand::Daybreak => { if !self.config.features.enabled(Feature::CliDaybreak) { @@ -354,11 +362,6 @@ impl ChatWidget { .send(AppEvent::GenerateRecap { thread_id }); } SlashCommand::Review => { - if source == SlashCommandDispatchSource::Live { - self.bottom_pane - .set_composer_text(String::new(), Vec::new(), Vec::new()); - self.bottom_pane.drain_pending_submission_state(); - } self.open_review_popup(); if self.mcp_startup_status.is_some() { self.defer_input_until_settings_applied(); @@ -1394,7 +1397,9 @@ impl ChatWidget { "'/{}' is unavailable in side conversations. {SIDE_SLASH_COMMAND_UNAVAILABLE_HINT}", cmd.command() )); - self.bottom_pane.drain_pending_submission_state(); + if !cmd.requires_dispatch_validation() { + self.bottom_pane.drain_pending_submission_state(); + } false } diff --git a/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs b/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs index 58bc2153d1..fea4cd691c 100644 --- a/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs +++ b/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs @@ -269,9 +269,13 @@ async fn inline_review_submits_during_mcp_startup() { } #[tokio::test] -async fn review_during_mcp_startup_preserves_draft_when_foreground_work_is_pending() { +async fn review_and_fork_during_mcp_startup_preserve_drafts_when_foreground_work_is_pending() { for activity in ["turn", "review", "pending", "queued", "images"] { - for draft in ["/review", "/review check regressions"] { + for draft in ["/review", "/review changes", "/fork", "/fork named"] { + let command = draft.split_whitespace().next().unwrap(); + if activity == "queued" && command == "/fork" { + continue; + } let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await; chat.set_mcp_startup_expected_servers(["slow".to_string()]); @@ -309,7 +313,9 @@ async fn review_during_mcp_startup_preserves_draft_when_foreground_work_is_pendi .collect::>(); assert_eq!( errors, - vec!["■ '/review' is disabled while a task is in progress.\n".to_string()] + vec![format!( + "■ '{command}' is disabled while a task is in progress.\n" + )] ); assert!( !std::iter::from_fn(|| op_rx.try_recv().ok()) @@ -429,6 +435,17 @@ async fn pending_mcp_startup_does_not_unblock_external_review() { notify_mcp_status(&mut chat, "slow", McpServerStartupState::Starting); chat.thread_id = Some(ThreadId::new()); + chat.set_local_worktree_operations(/*enabled*/ false); + chat.bottom_pane + .set_composer_text("/fork".into(), Vec::new(), Vec::new()); + chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + assert!( + std::iter::from_fn(|| rx.try_recv().ok()) + .any(|event| matches!(event, AppEvent::ForkCurrentSession { name: None })) + ); + assert_eq!(chat.bottom_pane.composer_text(), ""); + handle_entered_review_mode(&mut chat, "current changes"); assert!(chat.bottom_pane.status_indicator_visible()); chat.queue_user_message("queued follow-up".into()); @@ -440,7 +457,10 @@ async fn pending_mcp_startup_does_not_unblock_external_review() { while rx.try_recv().is_ok() {} chat.dispatch_command(crate::slash_command::SlashCommand::Fork); - assert_matches!(rx.try_recv(), Ok(AppEvent::InsertHistoryCell(_))); + insta::assert_snapshot!( + lines_to_single_string(&drain_insert_history(&mut rx).concat()), + @"■ '/fork' is disabled while a task is in progress." + ); chat.finish_mcp_startup(Vec::new(), Vec::new()); assert!(chat.bottom_pane.is_task_running()); diff --git a/codex-rs/tui/src/chatwidget/tests/side.rs b/codex-rs/tui/src/chatwidget/tests/side.rs index 32a6d78c53..6f82817537 100644 --- a/codex-rs/tui/src/chatwidget/tests/side.rs +++ b/codex-rs/tui/src/chatwidget/tests/side.rs @@ -91,6 +91,15 @@ async fn slash_commands_without_side_flag_are_rejected_for_side_threads() { } assert!(rx.try_recv().is_err(), "expected no follow-up events"); assert!(op_rx.try_recv().is_err(), "expected no review op"); + + chat.bottom_pane + .set_composer_text("/fork".into(), Vec::new(), Vec::new()); + let images = vec!["https://example.com/image.png".to_string()]; + chat.bottom_pane.set_remote_image_urls(images.clone()); + chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + assert_eq!(chat.bottom_pane.composer_text(), "/fork"); + assert_eq!(chat.bottom_pane.remote_image_urls(), images); } #[tokio::test] diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index dfd05384f7..5280846219 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -221,7 +221,7 @@ impl SlashCommand { /// Whether dispatch needs thread state to validate this command before consuming its draft. /// The composer must defer busy-state rejection and draft clearing for these commands. pub(crate) fn requires_dispatch_validation(self) -> bool { - matches!(self, SlashCommand::Review) + matches!(self, SlashCommand::Fork | SlashCommand::Review) } /// Commands that do not require a writable current thread. The server must still be connected.