mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
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
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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::<Vec<_>>();
|
||||
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());
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user