mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Keep paused goals paused on thread resume (#20790)
## Summary Early adopters of the `/goal` feature have provided feedback that they expect a goal they explicitly paused to remain paused when they resume a thread. Previously, resuming a thread would reactivate a paused goal. This PR keeps persisted goal status unchanged during thread resume. This honors the user feedback while also simplifying the core goal logic. Rather than have the core logic automatically resume a paused goal, that responsibility is transferred to the client. The TUI now detects a resumed thread with a paused goal and asks the user whether to `Resume goal` or `Leave paused`. The prompt appears only for quiet resume flows, so users who resume with an immediate prompt are not interrupted. <img width="544" height="111" alt="image" src="https://github.com/user-attachments/assets/0ac9de1c-6ee6-47ba-b223-c03c8eb4c192" />
This commit is contained in:
committed by
GitHub
Unverified
parent
2f5c06a29c
commit
3c2dcbef85
@@ -385,7 +385,7 @@ async fn thread_resume_can_skip_turns_for_metadata_only_resume() -> Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_emits_active_goal_update_before_continuation() -> Result<()> {
|
||||
async fn thread_resume_keeps_paused_goal_paused() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml(codex_home.path(), &server.uri())?;
|
||||
@@ -477,12 +477,12 @@ async fn thread_resume_emits_active_goal_update_before_continuation() -> Result<
|
||||
let ServerNotification::ThreadGoalUpdated(notification) = notification else {
|
||||
anyhow::bail!("expected thread goal update notification");
|
||||
};
|
||||
assert_eq!(notification.goal.status, ThreadGoalStatus::Active);
|
||||
assert_eq!(notification.goal.status, ThreadGoalStatus::Paused);
|
||||
assert!(
|
||||
!mcp.pending_notification_methods()
|
||||
.iter()
|
||||
.any(|method| method == "turn/started"),
|
||||
"goal continuation should start only after the resume goal snapshot"
|
||||
"paused goal should not continue after thread resume"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
|
||||
+22
-72
@@ -270,10 +270,11 @@ impl Session {
|
||||
/// starts capture the active goal and token baseline, tool completions
|
||||
/// account usage and may inject budget steering, completion accounting
|
||||
/// suppresses that steering, external mutations account best-effort before
|
||||
/// changing state, interrupts pause active goals, resumes reactivate paused
|
||||
/// goals, explicit maybe-continue events start idle goal continuation turns,
|
||||
/// and continuation turns with no counted autonomous activity suppress the
|
||||
/// next automatic continuation until user/tool/external activity resets it.
|
||||
/// changing state, interrupts pause active goals, thread resumes restore
|
||||
/// runtime state for already-active goals, explicit maybe-continue events
|
||||
/// start idle goal continuation turns, and continuation turns with no counted
|
||||
/// autonomous activity suppress the next automatic continuation until
|
||||
/// user/tool/external activity resets it.
|
||||
pub(crate) fn goal_runtime_apply<'a>(
|
||||
self: &'a Arc<Self>,
|
||||
event: GoalRuntimeEvent<'a>,
|
||||
@@ -339,7 +340,7 @@ impl Session {
|
||||
Ok(())
|
||||
}),
|
||||
GoalRuntimeEvent::ThreadResumed => Box::pin(async move {
|
||||
self.activate_paused_thread_goal_after_resume().await?;
|
||||
self.restore_thread_goal_runtime_after_resume().await?;
|
||||
Ok(())
|
||||
}),
|
||||
}
|
||||
@@ -1016,15 +1017,15 @@ impl Session {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn activate_paused_thread_goal_after_resume(&self) -> anyhow::Result<bool> {
|
||||
async fn restore_thread_goal_runtime_after_resume(&self) -> anyhow::Result<()> {
|
||||
if !self.enabled(Feature::Goals) {
|
||||
return Ok(false);
|
||||
return Ok(());
|
||||
}
|
||||
if should_ignore_goal_for_mode(self.collaboration_mode().await.mode) {
|
||||
tracing::debug!(
|
||||
"skipping paused goal auto-resume while current collaboration mode ignores goals"
|
||||
"skipping goal runtime restore while current collaboration mode ignores goals"
|
||||
);
|
||||
return Ok(false);
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let _continuation_guard = self
|
||||
@@ -1034,79 +1035,28 @@ impl Session {
|
||||
.await
|
||||
.context("goal continuation semaphore closed")?;
|
||||
let Some(state_db) = self.state_db_for_thread_goals().await? else {
|
||||
return Ok(false);
|
||||
return Ok(());
|
||||
};
|
||||
let Some(goal) = state_db.get_thread_goal(self.conversation_id).await? else {
|
||||
*self.goal_runtime.budget_limit_reported_goal_id.lock().await = None;
|
||||
self.goal_runtime
|
||||
.accounting
|
||||
.lock()
|
||||
.await
|
||||
.wall_clock
|
||||
.clear_active_goal();
|
||||
return Ok(false);
|
||||
self.clear_stopped_thread_goal_runtime_state().await;
|
||||
return Ok(());
|
||||
};
|
||||
if goal.status != codex_state::ThreadGoalStatus::Paused {
|
||||
let goal_id = goal.goal_id.clone();
|
||||
let is_active = goal.status == codex_state::ThreadGoalStatus::Active;
|
||||
if is_active {
|
||||
match goal.status {
|
||||
codex_state::ThreadGoalStatus::Active => {
|
||||
self.goal_runtime
|
||||
.accounting
|
||||
.lock()
|
||||
.await
|
||||
.wall_clock
|
||||
.mark_active_goal(goal_id);
|
||||
} else {
|
||||
self.goal_runtime
|
||||
.accounting
|
||||
.lock()
|
||||
.await
|
||||
.wall_clock
|
||||
.clear_active_goal();
|
||||
.mark_active_goal(goal.goal_id);
|
||||
}
|
||||
codex_state::ThreadGoalStatus::Paused
|
||||
| codex_state::ThreadGoalStatus::BudgetLimited
|
||||
| codex_state::ThreadGoalStatus::Complete => {
|
||||
self.clear_stopped_thread_goal_runtime_state().await;
|
||||
}
|
||||
return Ok(false);
|
||||
}
|
||||
|
||||
let Some(goal) = state_db
|
||||
.update_thread_goal(
|
||||
self.conversation_id,
|
||||
codex_state::ThreadGoalUpdate {
|
||||
status: Some(codex_state::ThreadGoalStatus::Active),
|
||||
token_budget: None,
|
||||
expected_goal_id: Some(goal.goal_id.clone()),
|
||||
},
|
||||
)
|
||||
.await?
|
||||
else {
|
||||
*self.goal_runtime.budget_limit_reported_goal_id.lock().await = None;
|
||||
self.goal_runtime
|
||||
.accounting
|
||||
.lock()
|
||||
.await
|
||||
.wall_clock
|
||||
.clear_active_goal();
|
||||
return Ok(false);
|
||||
};
|
||||
let goal_id = goal.goal_id.clone();
|
||||
let goal = protocol_goal_from_state(goal);
|
||||
*self.goal_runtime.budget_limit_reported_goal_id.lock().await = None;
|
||||
let active_turn_id = self
|
||||
.active_turn_context()
|
||||
.await
|
||||
.map(|turn_context| turn_context.sub_id.clone());
|
||||
let current_token_usage = self.total_token_usage().await.unwrap_or_default();
|
||||
self.mark_active_goal_accounting(goal_id, active_turn_id, current_token_usage)
|
||||
.await;
|
||||
self.send_event_raw(Event {
|
||||
id: uuid::Uuid::new_v4().to_string(),
|
||||
msg: EventMsg::ThreadGoalUpdated(ThreadGoalUpdatedEvent {
|
||||
thread_id: self.conversation_id,
|
||||
turn_id: None,
|
||||
goal,
|
||||
}),
|
||||
})
|
||||
.await;
|
||||
Ok(true)
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn maybe_continue_goal_if_idle_runtime(self: &Arc<Self>) {
|
||||
|
||||
@@ -1123,7 +1123,7 @@ async fn interrupted_fork_snapshot_uses_persisted_mid_turn_history_without_live_
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn resumed_thread_activates_paused_goal_and_continues_on_request() -> anyhow::Result<()> {
|
||||
async fn resumed_thread_keeps_paused_goal_paused() -> anyhow::Result<()> {
|
||||
let temp_dir = tempdir().expect("tempdir");
|
||||
let mut config = test_config().await;
|
||||
config.codex_home = temp_dir.path().join("codex-home").abs();
|
||||
@@ -1188,7 +1188,7 @@ async fn resumed_thread_activates_paused_goal_and_continues_on_request() -> anyh
|
||||
.get_thread_goal(resumed.thread_id)
|
||||
.await?
|
||||
.expect("goal should still exist after resume");
|
||||
assert_eq!(codex_state::ThreadGoalStatus::Active, goal.status);
|
||||
assert_eq!(codex_state::ThreadGoalStatus::Paused, goal.status);
|
||||
assert!(
|
||||
resumed
|
||||
.thread
|
||||
@@ -1209,7 +1209,7 @@ async fn resumed_thread_activates_paused_goal_and_continues_on_request() -> anyh
|
||||
.active_turn
|
||||
.lock()
|
||||
.await
|
||||
.is_some()
|
||||
.is_none()
|
||||
);
|
||||
|
||||
resumed.thread.shutdown_and_wait().await?;
|
||||
|
||||
@@ -714,6 +714,12 @@ impl App {
|
||||
let enhanced_keys_supported = tui.enhanced_keys_supported();
|
||||
let wait_for_initial_session_configured =
|
||||
Self::should_wait_for_initial_session(&session_selection);
|
||||
let should_prompt_for_paused_goal_after_startup_resume =
|
||||
Self::should_prompt_for_paused_goal_after_startup_resume(
|
||||
&session_selection,
|
||||
&initial_prompt,
|
||||
&initial_images,
|
||||
);
|
||||
let (mut chat_widget, initial_started_thread) = match session_selection {
|
||||
SessionSelection::StartFresh | SessionSelection::Exit => {
|
||||
let started = app_server.start_thread(&config).await?;
|
||||
@@ -889,8 +895,13 @@ See the Codex keymap documentation for supported actions and examples."
|
||||
pending_hook_enabled_writes: HashMap::new(),
|
||||
};
|
||||
if let Some(started) = initial_started_thread {
|
||||
let thread_id = started.session.thread_id;
|
||||
app.enqueue_primary_thread_session(started.session, started.turns)
|
||||
.await?;
|
||||
if should_prompt_for_paused_goal_after_startup_resume {
|
||||
app.maybe_prompt_resume_paused_goal_after_resume(&mut app_server, thread_id)
|
||||
.await;
|
||||
}
|
||||
}
|
||||
|
||||
// On startup, if a managed filesystem sandbox is active, warn about
|
||||
|
||||
@@ -680,6 +680,7 @@ impl App {
|
||||
.await
|
||||
{
|
||||
Ok(resumed) => {
|
||||
let resumed_thread_id = resumed.session.thread_id;
|
||||
self.shutdown_current_thread(app_server).await;
|
||||
self.config = resume_config;
|
||||
tui.set_notification_settings(
|
||||
@@ -707,6 +708,11 @@ impl App {
|
||||
}
|
||||
self.chat_widget.add_plain_history_lines(lines);
|
||||
}
|
||||
self.maybe_prompt_resume_paused_goal_after_resume(
|
||||
app_server,
|
||||
resumed_thread_id,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
Err(err) => {
|
||||
self.chat_widget.add_error_message(format!(
|
||||
|
||||
@@ -152,6 +152,42 @@ fn startup_waiting_gate_is_only_for_fresh_or_exit_session_selection() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn startup_paused_goal_prompt_gate_is_only_for_quiet_resume() {
|
||||
let resume = SessionSelection::Resume(crate::resume_picker::SessionTarget {
|
||||
path: Some(PathBuf::from("/tmp/restore")),
|
||||
thread_id: ThreadId::new(),
|
||||
});
|
||||
let fork = SessionSelection::Fork(crate::resume_picker::SessionTarget {
|
||||
path: Some(PathBuf::from("/tmp/fork")),
|
||||
thread_id: ThreadId::new(),
|
||||
});
|
||||
let no_images: Vec<PathBuf> = Vec::new();
|
||||
let initial_images = vec![PathBuf::from("/tmp/image.png")];
|
||||
|
||||
assert!(App::should_prompt_for_paused_goal_after_startup_resume(
|
||||
&resume, &None, &no_images
|
||||
));
|
||||
assert!(!App::should_prompt_for_paused_goal_after_startup_resume(
|
||||
&resume,
|
||||
&Some("continue from here".to_string()),
|
||||
&no_images
|
||||
));
|
||||
assert!(!App::should_prompt_for_paused_goal_after_startup_resume(
|
||||
&resume,
|
||||
&None,
|
||||
&initial_images
|
||||
));
|
||||
assert!(!App::should_prompt_for_paused_goal_after_startup_resume(
|
||||
&SessionSelection::StartFresh,
|
||||
&None,
|
||||
&no_images
|
||||
));
|
||||
assert!(!App::should_prompt_for_paused_goal_after_startup_resume(
|
||||
&fork, &None, &no_images
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn startup_waiting_gate_holds_active_thread_events_until_primary_thread_configured() {
|
||||
let mut wait_for_initial_session =
|
||||
|
||||
@@ -42,6 +42,33 @@ impl App {
|
||||
self.chat_widget.show_goal_summary(goal);
|
||||
}
|
||||
|
||||
pub(super) async fn maybe_prompt_resume_paused_goal_after_resume(
|
||||
&mut self,
|
||||
app_server: &mut AppServerSession,
|
||||
thread_id: ThreadId,
|
||||
) {
|
||||
let result = app_server.thread_goal_get(thread_id).await;
|
||||
if self.current_displayed_thread_id() != Some(thread_id) {
|
||||
return;
|
||||
}
|
||||
|
||||
let response = match result {
|
||||
Ok(response) => response,
|
||||
Err(err) => {
|
||||
tracing::warn!("failed to read thread goal after resume: {err}");
|
||||
return;
|
||||
}
|
||||
};
|
||||
|
||||
let Some(goal) = response.goal else {
|
||||
return;
|
||||
};
|
||||
if goal.status == ThreadGoalStatus::Paused {
|
||||
self.chat_widget
|
||||
.show_resume_paused_goal_prompt(thread_id, goal.objective);
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) async fn set_thread_goal_objective(
|
||||
&mut self,
|
||||
app_server: &mut AppServerSession,
|
||||
|
||||
@@ -1281,6 +1281,16 @@ impl App {
|
||||
)
|
||||
}
|
||||
|
||||
pub(super) fn should_prompt_for_paused_goal_after_startup_resume(
|
||||
session_selection: &SessionSelection,
|
||||
initial_prompt: &Option<String>,
|
||||
initial_images: &[PathBuf],
|
||||
) -> bool {
|
||||
matches!(session_selection, SessionSelection::Resume(_))
|
||||
&& initial_prompt.is_none()
|
||||
&& initial_images.is_empty()
|
||||
}
|
||||
|
||||
pub(super) fn should_handle_active_thread_events(
|
||||
waiting_for_initial_session_configured: bool,
|
||||
has_active_thread_receiver: bool,
|
||||
|
||||
@@ -9,6 +9,41 @@ impl ChatWidget {
|
||||
self.add_plain_history_lines(goal_summary_lines(&goal));
|
||||
}
|
||||
|
||||
pub(crate) fn show_resume_paused_goal_prompt(
|
||||
&mut self,
|
||||
thread_id: ThreadId,
|
||||
objective: String,
|
||||
) {
|
||||
let resume_actions: Vec<SelectionAction> = vec![Box::new(move |tx| {
|
||||
tx.send(AppEvent::SetThreadGoalStatus {
|
||||
thread_id,
|
||||
status: AppThreadGoalStatus::Active,
|
||||
});
|
||||
})];
|
||||
self.show_selection_view(SelectionViewParams {
|
||||
title: Some("Resume paused goal?".to_string()),
|
||||
subtitle: Some(format!("Goal: {objective}")),
|
||||
footer_hint: Some(standard_popup_hint_line()),
|
||||
initial_selected_idx: Some(0),
|
||||
items: vec![
|
||||
SelectionItem {
|
||||
name: "Resume goal".to_string(),
|
||||
description: Some("Mark it active and continue when idle".to_string()),
|
||||
actions: resume_actions,
|
||||
dismiss_on_select: true,
|
||||
..Default::default()
|
||||
},
|
||||
SelectionItem {
|
||||
name: "Leave paused".to_string(),
|
||||
description: Some("Keep it paused; use /goal resume later".to_string()),
|
||||
dismiss_on_select: true,
|
||||
..Default::default()
|
||||
},
|
||||
],
|
||||
..Default::default()
|
||||
});
|
||||
}
|
||||
|
||||
pub(crate) fn on_thread_goal_cleared(&mut self, thread_id: &str) {
|
||||
if self
|
||||
.thread_id
|
||||
|
||||
+11
@@ -0,0 +1,11 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/goal_menu.rs
|
||||
expression: "render_bottom_popup(&chat, 100)"
|
||||
---
|
||||
Resume paused goal?
|
||||
Goal: Keep improving the bare goal command until it feels calm and useful.
|
||||
|
||||
› 1. Resume goal Mark it active and continue when idle
|
||||
2. Leave paused Keep it paused; use /goal resume later
|
||||
|
||||
Press enter to confirm or esc to go back
|
||||
@@ -42,6 +42,56 @@ async fn goal_menu_budget_limited_snapshot() {
|
||||
assert_chatwidget_snapshot!("goal_menu_budget_limited", rendered_goal_summary(&mut rx));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn resume_paused_goal_prompt_snapshot() {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let thread_id = ThreadId::new();
|
||||
|
||||
chat.show_resume_paused_goal_prompt(
|
||||
thread_id,
|
||||
"Keep improving the bare goal command until it feels calm and useful.".to_string(),
|
||||
);
|
||||
|
||||
assert_chatwidget_snapshot!(
|
||||
"resume_paused_goal_prompt",
|
||||
render_bottom_popup(&chat, /*width*/ 100)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn resume_paused_goal_prompt_default_resumes_goal() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let thread_id = ThreadId::new();
|
||||
|
||||
chat.show_resume_paused_goal_prompt(thread_id, "Finish the paused goal.".to_string());
|
||||
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
|
||||
|
||||
match rx.try_recv() {
|
||||
Ok(AppEvent::SetThreadGoalStatus {
|
||||
thread_id: event_thread_id,
|
||||
status,
|
||||
}) => {
|
||||
assert_eq!(event_thread_id, thread_id);
|
||||
assert_eq!(status, AppThreadGoalStatus::Active);
|
||||
}
|
||||
other => panic!("expected SetThreadGoalStatus event, got {other:?}"),
|
||||
}
|
||||
assert!(chat.no_modal_or_popup_active());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn resume_paused_goal_prompt_can_leave_goal_paused() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let thread_id = ThreadId::new();
|
||||
|
||||
chat.show_resume_paused_goal_prompt(thread_id, "Finish the paused goal.".to_string());
|
||||
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
|
||||
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
|
||||
|
||||
assert!(matches!(rx.try_recv(), Err(TryRecvError::Empty)));
|
||||
assert!(chat.no_modal_or_popup_active());
|
||||
}
|
||||
|
||||
fn test_goal(
|
||||
thread_id: ThreadId,
|
||||
status: AppThreadGoalStatus,
|
||||
|
||||
Reference in New Issue
Block a user