From 181b4a853d396f009a3a02b9c36eb393e6acbe75 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Sun, 1 Nov 2026 14:08:25 +0000 Subject: [PATCH] Prioritize pending approvals in the subagent picker (#58720) ## What changed - Group threads needing approval first in `/subagents`, mark them with `(needs approval)`, and initially select the first waiting thread. - Refresh the open picker and approval notice as requests and agent metadata change, preserving the selected thread when rows move. - Use consistent agent labels across the picker and approval notice, preferring agent paths and using UUID suffixes to distinguish unnamed agents. - Track approval requests that arrive before thread metadata and refresh pending approvals after reconnecting. ## Testing Extend the picker test and snapshots to cover approvals arriving while the picker is open, agent paths arriving later, and selection staying on the same thread after reordering. GitOrigin-RevId: b7dd943d151ce98d1978f7c6b9f5e16903b9075b --- codex-rs/tui/src/app/agent_navigation.rs | 18 ++++- codex-rs/tui/src/app/agent_picker.rs | 37 ++++----- codex-rs/tui/src/app/reconnect.rs | 2 +- codex-rs/tui/src/app/session_lifecycle.rs | 81 +++++++++---------- codex-rs/tui/src/app/side.rs | 1 + codex-rs/tui/src/app/tests.rs | 59 ++++++++++++++ codex-rs/tui/src/app/thread_routing.rs | 68 ++++++++++------ .../bottom_pane/pending_thread_approvals.rs | 6 +- ..._app__tests__path_backed_agent_picker.snap | 4 +- 9 files changed, 183 insertions(+), 93 deletions(-) diff --git a/codex-rs/tui/src/app/agent_navigation.rs b/codex-rs/tui/src/app/agent_navigation.rs index f8862e908e..5ea219f8a8 100644 --- a/codex-rs/tui/src/app/agent_navigation.rs +++ b/codex-rs/tui/src/app/agent_navigation.rs @@ -43,18 +43,22 @@ use uuid::Uuid; pub(crate) struct AgentNavigationState { /// Latest picker metadata for each tracked thread id. threads: HashMap, - /// Stable first-seen traversal order for picker rows and keyboard cycling. + /// First-seen order for keyboard cycling. The picker groups waiting threads first. order: Vec, /// Threads with observed terminal liveness that must not be revived by delayed activity. stopped_threads: HashSet, /// Spawned child threads whose instructions are owned by their parent agent. parent_owned_threads: HashSet, picker_excluded_threads: HashSet, + /// Inactive threads with an approval outstanding in their event store. + pub(super) pending_approvals: HashSet, + /// Thread IDs in the currently rendered picker, so a refresh can restore its cursor by ID. + pub(super) picker_row_ids: Vec, /// Coalesces root refreshes while rejecting replies from a previous session. pub(super) picker_refresh: Option, } -/// Direction of keyboard traversal through the stable picker order. +/// Direction of keyboard traversal through the first-seen order. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(crate) enum AgentNavigationDirection { /// Move toward the entry that was seen earlier in spawn order, wrapping at the front. @@ -268,6 +272,8 @@ impl AgentNavigationState { self.stopped_threads.clear(); self.parent_owned_threads.clear(); self.picker_excluded_threads.clear(); + self.pending_approvals.clear(); + self.picker_row_ids.clear(); self.picker_refresh = None; } @@ -315,6 +321,12 @@ impl AgentNavigationState { .collect() } + pub(super) fn pending_approval_threads(&self) -> impl Iterator + '_ { + self.order.iter().copied().filter(|id| { + self.pending_approvals.contains(id) && !self.picker_excluded_threads.contains(id) + }) + } + pub(crate) fn set_picker_thread_visibility( &mut self, thread_id: ThreadId, @@ -353,7 +365,7 @@ impl AgentNavigationState { .collect() } - /// Returns tracked thread ids in the same stable order used by the picker. + /// Returns tracked thread ids in first-seen order. pub(crate) fn tracked_thread_ids(&self) -> Vec { self.ordered_threads() .into_iter() diff --git a/codex-rs/tui/src/app/agent_picker.rs b/codex-rs/tui/src/app/agent_picker.rs index 93c4b5391b..55584ae176 100644 --- a/codex-rs/tui/src/app/agent_picker.rs +++ b/codex-rs/tui/src/app/agent_picker.rs @@ -129,7 +129,6 @@ impl App { return; } }; - let selected_thread_id = self.selected_agent_picker_thread_id(); for thread in refresh.threads { let Ok(thread_id) = ThreadId::from_string(&thread.id) else { continue; @@ -177,7 +176,7 @@ impl App { } self.agent_navigation.prune_untracked_picker_exclusions(); - self.replace_agent_picker_view(selected_thread_id); + self.refresh_agent_picker(); if completion.follow_up_requested { self.refresh_agent_picker_threads(app_server, root); } @@ -188,9 +187,8 @@ impl App { thread_id: ThreadId, visibility: AgentPickerThreadVisibility, ) { - let selected_thread_id = self.selected_agent_picker_thread_id(); self.update_agent_picker_thread_visibility(thread_id, visibility); - self.replace_agent_picker_view(selected_thread_id); + self.refresh_agent_picker(); } fn update_agent_picker_thread_visibility( @@ -236,25 +234,24 @@ impl App { } } - fn selected_agent_picker_thread_id(&self) -> Option { + /// Keep the approval notice and any open picker in sync with the same navigation state. + pub(super) fn refresh_agent_picker(&mut self) -> bool { + self.sync_active_agent_label(); + let pending = self + .agent_navigation + .pending_approval_threads() + .map(|id| self.thread_label(id)) + .collect(); + self.chat_widget.set_pending_thread_approvals(pending); + if !self.chat_widget.has_active_modal() { + return false; + } let selected = self .chat_widget - .selected_index_for_present_view(AGENT_PICKER_VIEW_ID)?; - self.agent_navigation - .visible_threads() - .get(selected) - .map(|(thread_id, _)| *thread_id) - } - - fn replace_agent_picker_view(&mut self, selected_thread_id: Option) { - let selected = selected_thread_id.and_then(|selected_thread_id| { - self.agent_navigation - .visible_threads() - .iter() - .position(|(thread_id, _)| *thread_id == selected_thread_id) - }); + .selected_index_for_present_view(AGENT_PICKER_VIEW_ID) + .and_then(|index| self.agent_navigation.picker_row_ids.get(index).copied()); let params = self.agent_picker_selection_view_params(selected); self.chat_widget - .replace_selection_view_if_present(AGENT_PICKER_VIEW_ID, params); + .replace_selection_view_if_present(AGENT_PICKER_VIEW_ID, params) } } diff --git a/codex-rs/tui/src/app/reconnect.rs b/codex-rs/tui/src/app/reconnect.rs index e48819ac1f..a998f6c511 100644 --- a/codex-rs/tui/src/app/reconnect.rs +++ b/codex-rs/tui/src/app/reconnect.rs @@ -528,7 +528,6 @@ impl App { return Err(err); } self.config = self.chat_widget.config_ref().clone(); - self.refresh_pending_thread_approvals().await; if self.thread_unavailable(id) && !self.chat_widget.is_external_writer_view() { self.agent_navigation.mark_stopped(id); self.chat_widget.pause_unavailable_thread(); @@ -549,6 +548,7 @@ impl App { self.replace_chat_widget(ChatWidget::new_with_app_event(init)); self.chat_widget.restore_reconnected_input(input, &[]); } + self.refresh_pending_thread_approvals().await; // Discover tasks whose notifications were missed, without clearing retained rows. // A hidden overview performs this discovery when it is next opened. self.agents_overview.initialized = false; diff --git a/codex-rs/tui/src/app/session_lifecycle.rs b/codex-rs/tui/src/app/session_lifecycle.rs index 52ee5dc64a..9203adf62c 100644 --- a/codex-rs/tui/src/app/session_lifecycle.rs +++ b/codex-rs/tui/src/app/session_lifecycle.rs @@ -141,6 +141,7 @@ impl App { } } + self.sync_active_agent_label(); let has_non_primary_agent_thread = self .agent_navigation .has_non_primary_thread(self.primary_thread_id); @@ -158,15 +159,8 @@ impl App { return; } - let selected = self - .chat_widget - .selected_index_for_present_view(AGENT_PICKER_VIEW_ID); - let params = self.agent_picker_selection_view_params(selected); - if !self - .chat_widget - .replace_selection_view_if_present(AGENT_PICKER_VIEW_ID, params) - { - let params = self.agent_picker_selection_view_params(selected); + if !self.refresh_agent_picker() { + let params = self.agent_picker_selection_view_params(/*selected*/ None); self.chat_widget.show_selection_view(params); } if let Some(primary_thread_id) = self.primary_thread_id { @@ -175,42 +169,39 @@ impl App { } pub(super) fn agent_picker_selection_view_params( - &self, - selected: Option, + &mut self, + selected: Option, ) -> SelectionViewParams { - let mut initial_selected_idx = selected; - let items: Vec = self - .agent_navigation - .visible_threads() + let mut threads = self.agent_navigation.visible_threads(); + let pending = &self.agent_navigation.pending_approvals; + threads.sort_by_key(|(id, _)| !pending.contains(id)); + let mut row_ids = Vec::with_capacity(threads.len()); + let mut initial_selected_idx = None; + let items: Vec = threads .into_iter() .enumerate() .map(|(idx, (thread_id, entry))| { - if initial_selected_idx.is_none() && self.active_thread_id == Some(thread_id) { + row_ids.push(thread_id); + let is_pending = pending.contains(&thread_id); + let is_current = self.active_thread_id == Some(thread_id); + if selected == Some(thread_id) + || initial_selected_idx.is_none() && (is_pending || is_current) + { initial_selected_idx = Some(idx); } - let id = thread_id; - let is_primary = self.primary_thread_id == Some(thread_id); - let name = entry - .agent_path - .as_deref() - .map(str::trim) - .filter(|agent_path| !is_primary && !agent_path.is_empty()) - .map(ToOwned::to_owned) - .unwrap_or_else(|| { - format_agent_picker_item_name( - entry.agent_nickname.as_deref(), - entry.agent_role.as_deref(), - is_primary, - ) - }); + let name = self.thread_label(thread_id); let uuid = thread_id.to_string(); SelectionItem { - name: name.clone(), + name: if is_pending { + format!("{name} (needs approval)") + } else { + name.clone() + }, name_prefix_spans: agent_picker_status_dot_spans(entry.is_closed), description: Some(uuid.clone()), - is_current: self.active_thread_id == Some(thread_id), + is_current, actions: vec![Box::new(move |tx| { - tx.send(AppEvent::SelectAgentThread(id)); + tx.send(AppEvent::SelectAgentThread(thread_id)); })], dismiss_on_select: true, search_value: Some(format!("{name} {uuid}")), @@ -218,6 +209,7 @@ impl App { } }) .collect(); + self.agent_navigation.picker_row_ids = row_ids; SelectionViewParams { view_id: Some(AGENT_PICKER_VIEW_ID), @@ -251,10 +243,7 @@ impl App { }) } - /// Updates cached picker metadata and then mirrors any visible-label change into the footer. - /// - /// These two writes stay paired so the picker rows and contextual footer continue to describe - /// the same displayed thread after nickname or role updates. + /// Cache metadata in both the picker and transcript. Callers refresh the UI after a batch. pub(super) fn upsert_agent_picker_thread( &mut self, thread_id: ThreadId, @@ -269,7 +258,14 @@ impl App { ); self.agent_navigation .upsert(thread_id, agent_nickname, agent_role, is_closed); - self.sync_active_agent_label(); + } + + pub(super) fn sync_agent_picker_thread(&mut self, thread_id: ThreadId) { + if self.agent_navigation.pending_approvals.contains(&thread_id) { + self.refresh_agent_picker(); + } else { + self.sync_active_agent_label(); + } } /// Persists the app-server's authoritative ownership flag and updates the active composer. @@ -327,6 +323,7 @@ impl App { self.agent_navigation.mark_parent_owned(thread_id); } self.agent_navigation.set_agent_path(thread_id, agent_path); + self.sync_agent_picker_thread(thread_id); if is_running { self.agent_navigation.mark_running(thread_id); } else { @@ -359,6 +356,7 @@ impl App { } self.agent_navigation .set_running(thread_id, /*is_running*/ false); + self.sync_agent_picker_thread(thread_id); true } } @@ -400,6 +398,7 @@ impl App { entry.agent_role, /*is_closed*/ false, ); + self.sync_agent_picker_thread(thread_id); } if started.blocks_direct_input { self.agent_navigation.mark_parent_owned(thread_id); @@ -541,7 +540,7 @@ impl App { previous.park_voice(); self.background_voice = Some(Box::new(previous)); } - self.sync_active_agent_label(); + self.refresh_agent_picker(); } pub(super) async fn select_agent_thread( @@ -1321,7 +1320,7 @@ impl App { refreshed_thread_ids.insert(thread.thread_id); } } - self.sync_active_agent_label(); + self.refresh_agent_picker(); LoadedSubagentBackfill { completed: !had_read_error, diff --git a/codex-rs/tui/src/app/side.rs b/codex-rs/tui/src/app/side.rs index 34e0d63a00..78b0fcda53 100644 --- a/codex-rs/tui/src/app/side.rs +++ b/codex-rs/tui/src/app/side.rs @@ -785,6 +785,7 @@ impl App { /*agent_role*/ None, /*is_closed*/ false, ); + self.sync_agent_picker_thread(child_thread_id); if let Err(err) = app_server .thread_inject_items(child_thread_id, vec![Self::side_boundary_prompt_item()]) .await diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 5c7f76f943..f8d8f9a909 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -2594,6 +2594,18 @@ async fn open_agent_picker_selects_path_backed_agent() -> Result<()> { .expect("embedded app server"); let thread_id = ThreadId::from_string("00000000-0000-0000-0000-000000000123").expect("valid thread id"); + let main = ThreadId::from_u128(/*value*/ 1); + let waiting = ThreadId::from_u128(/*value*/ 2); + app.enqueue_primary_thread_session( + test_thread_session(main, test_path_buf("/tmp/project")), + Vec::new(), + ) + .await?; + app.enqueue_thread_request( + waiting, + exec_approval_request(waiting, "turn", "call", /*approval_id*/ None), + ) + .await?; app.thread_event_channels .insert(thread_id, ThreadEventChannel::new(/*capacity*/ 1)); app.agent_navigation @@ -2605,10 +2617,57 @@ async fn open_agent_picker_selects_path_backed_agent() -> Result<()> { Box::pin(app.open_agent_picker(&mut app_server)).await; + // The second request arrives while the picker is open. + let request = + exec_approval_request(thread_id, "turn", "worker-call", /*approval_id*/ None); + app.thread_event_channels[&thread_id] + .store + .lock() + .await + .push_request(request); + app.refresh_pending_thread_approvals().await; + assert_eq!( + app.chat_widget.pending_thread_approvals(), + &["Agent (000000000002)", "/root/worker"] + ); + + // The first request gains its path without another approval event. + app.cache_collab_receiver_threads_for_notification(&ServerNotification::ItemStarted( + ItemStartedNotification::new( + main.to_string(), + "turn-1".to_string(), + ThreadItemPayload::SubAgentActivity { + id: "activity".to_string(), + kind: codex_app_server_protocol::SubAgentActivityKind::Started, + agent_thread_id: waiting.to_string(), + agent_path: "/root/review".to_string(), + model: None, + reasoning_effort: None, + }, + /*started_at_ms*/ 0, + ), + )); + assert_eq!( + app.chat_widget.pending_thread_approvals(), + &["/root/review", "/root/worker"] + ); + assert_app_snapshot!( "path_backed_agent_picker", render_bottom_popup(&app.chat_widget, /*width*/ 80) ); + app.chat_widget + .handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); + app.enqueue_thread_notification( + thread_id, + turn_completed_notification(thread_id, "turn", TurnStatus::Completed), + ) + .await?; + assert_eq!( + app.chat_widget + .selected_index_for_present_view(agent_picker::AGENT_PICKER_VIEW_ID), + Some(2) + ); while app_event_rx.try_recv().is_ok() {} app.chat_widget .handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index dd3823939c..b824a9f528 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -199,28 +199,38 @@ impl App { pub(super) fn thread_label(&self, thread_id: ThreadId) -> String { let is_primary = self.primary_thread_id == Some(thread_id); - let fallback_label = if is_primary { - "Main [default]".to_string() - } else { - let thread_id = thread_id.to_string(); - let short_id: String = thread_id.chars().take(8).collect(); - format!("Agent ({short_id})") + let anonymous = |label: &str| { + // Agents spawned together share the timestamp prefix of their UUIDv7 IDs. + let id = thread_id.to_string(); + let suffix = id.rsplit('-').next().unwrap_or(&id); + format!("{label} ({suffix})") }; if let Some(entry) = self.agent_navigation.get(&thread_id) { + if let Some(path) = entry.agent_path.as_deref().map(str::trim) + && !is_primary + && !path.is_empty() + { + return path.to_string(); + } let label = format_agent_picker_item_name( entry.agent_nickname.as_deref(), entry.agent_role.as_deref(), is_primary, ); - if label == "Agent" { - let thread_id = thread_id.to_string(); - let short_id: String = thread_id.chars().take(8).collect(); - format!("{label} ({short_id})") + if !is_primary + && entry + .agent_nickname + .as_deref() + .is_none_or(|name| name.trim().is_empty()) + { + anonymous(&label) } else { label } + } else if is_primary { + "Main [default]".to_string() } else { - fallback_label + anonymous("Agent") } } @@ -1001,27 +1011,29 @@ impl App { .map(|(thread_id, channel)| (*thread_id, Arc::clone(&channel.store))) .collect(); - let mut pending_thread_ids = Vec::new(); + let mut pending_thread_ids = HashSet::new(); for (thread_id, store) in channels { if Some(thread_id) == self.active_thread_id || Some(thread_id) == side_parent_thread_id { continue; } - let store = store.lock().await; - if store.has_pending_thread_approvals() { - pending_thread_ids.push(thread_id); + if store.lock().await.has_pending_thread_approvals() { + pending_thread_ids.insert(thread_id); + // Approval requests can arrive before the thread's metadata. + if self.agent_navigation.get(&thread_id).is_none() { + self.upsert_agent_picker_thread( + thread_id, /*agent_nickname*/ None, /*agent_role*/ None, + /*is_closed*/ false, + ); + } } } - pending_thread_ids.sort_by_key(ThreadId::to_string); - - let threads = pending_thread_ids - .into_iter() - .map(|thread_id| self.thread_label(thread_id)) - .collect(); - - self.chat_widget.set_pending_thread_approvals(threads); + if self.agent_navigation.pending_approvals != pending_thread_ids { + self.agent_navigation.pending_approvals = pending_thread_ids; + self.refresh_agent_picker(); + } } pub(super) async fn refresh_side_parent_status_from_store(&mut self, thread_id: ThreadId) { @@ -1128,6 +1140,11 @@ impl App { started.thread.agent_role.clone(), /*is_closed*/ false, ); + self.agent_navigation.set_agent_path( + thread_id, + crate::app_server_session::source_agent_path(&started.thread.source), + ); + self.sync_agent_picker_thread(thread_id); // Lifecycle responses already contain authoritative session state. Their rollout may // not exist until the first turn, so inferring it again can wait through reader retries. @@ -1263,8 +1280,9 @@ impl App { if let Some(activity) = sub_agent_activity_item(notification).and_then(sub_agent_activity_display) { + let thread_id = activity.thread_id; self.agent_navigation.record_sub_agent_activity(activity); - self.sync_active_agent_label(); + self.sync_agent_picker_thread(thread_id); return; } @@ -1293,6 +1311,7 @@ impl App { thread_id, /*agent_nickname*/ None, /*agent_role*/ None, /*is_closed*/ false, ); + self.sync_agent_picker_thread(thread_id); } } @@ -1476,6 +1495,7 @@ impl App { thread_id, /*agent_nickname*/ None, /*agent_role*/ None, /*is_closed*/ false, ); + self.sync_agent_picker_thread(thread_id); let channel = self.ensure_thread_channel(thread_id); { let mut store = channel.store.lock().await; diff --git a/codex-rs/tui/src/bottom_pane/pending_thread_approvals.rs b/codex-rs/tui/src/bottom_pane/pending_thread_approvals.rs index 2ff9e6cdbb..2c1302eef6 100644 --- a/codex-rs/tui/src/bottom_pane/pending_thread_approvals.rs +++ b/codex-rs/tui/src/bottom_pane/pending_thread_approvals.rs @@ -61,7 +61,7 @@ impl PendingThreadApprovals { Line::from(vec![ " ".into(), "/subagents".cyan().bold(), - " to switch threads".dim(), + " to review requests".dim(), ]) .dim(), ); @@ -120,7 +120,7 @@ mod tests { snapshot_rows(&widget, /*width*/ 40).replace(' ', "."), @r" ..!.Approval.needed.in.Robie.[explorer]. - ..../subagents.to.switch.threads........ + ..../subagents.to.review.requests....... " ); } @@ -142,7 +142,7 @@ mod tests { ..!.Approval.needed.in.Robie.[explorer]..... ..!.Approval.needed.in.Inspector............ ............................................ - ..../subagents.to.switch.threads............ + ..../subagents.to.review.requests........... " ); } diff --git a/codex-rs/tui/src/snapshots/codex_tui__app__tests__path_backed_agent_picker.snap b/codex-rs/tui/src/snapshots/codex_tui__app__tests__path_backed_agent_picker.snap index 38f1898fc5..36384345e5 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__app__tests__path_backed_agent_picker.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__app__tests__path_backed_agent_picker.snap @@ -6,6 +6,8 @@ expression: "render_bottom_popup(&app.chat_widget, 80)" Select an agent to watch. ⌥← previous, ⌥→ next. -› 1. • /root/worker 00000000-0000-0000-0000-000000000123 +› 1. • /root/review (needs approval) 00000000-0000-0000-0000-000000000002 + 2. • /root/worker (needs approval) 00000000-0000-0000-0000-000000000123 + 3. • Main [default] (current) 00000000-0000-0000-0000-000000000001 enter select · esc back