openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-01 19:33:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

Prioritize pending approvals in the subagent picker (#58720)

Browse Source 
## 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
This commit is contained in:
Tamir Duberstein
2026-11-01 15:51:11 +00:00
committed by copyberry
parent e02a32cd51
commit 181b4a853d
9 changed files with 183 additions and 93 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/app/agent_navigation.rs
+15 -3
View File
@@ -43,18 +43,22 @@ use uuid::Uuid;
pub(crate) struct AgentNavigationState {
/// Latest picker metadata for each tracked thread id.
threads: HashMap<ThreadId, AgentPickerThreadEntry>,
/// 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<ThreadId>,
/// Threads with observed terminal liveness that must not be revived by delayed activity.
stopped_threads: HashSet<ThreadId>,
/// Spawned child threads whose instructions are owned by their parent agent.
parent_owned_threads: HashSet<ThreadId>,
picker_excluded_threads: HashSet<ThreadId>,
/// Inactive threads with an approval outstanding in their event store.
pub(super) pending_approvals: HashSet<ThreadId>,
/// Thread IDs in the currently rendered picker, so a refresh can restore its cursor by ID.
pub(super) picker_row_ids: Vec<ThreadId>,
/// Coalesces root refreshes while rejecting replies from a previous session.
pub(super) picker_refresh: Option<AgentPickerRefreshState>,
}
/// 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<Item = ThreadId> + '_ {
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<ThreadId> {
self.ordered_threads()
.into_iter()
codex-rs/tui/src/app/agent_picker.rs
+17 -20
View File
@@ -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<ThreadId> {
/// 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<ThreadId>) {
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)
}
}
codex-rs/tui/src/app/reconnect.rs
+1 -1
View File
@@ -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;
codex-rs/tui/src/app/session_lifecycle.rs
+40 -41
View File
@@ -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<usize>,
&mut self,
selected: Option<ThreadId>,
) -> SelectionViewParams {
let mut initial_selected_idx = selected;
let items: Vec<SelectionItem> = 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<SelectionItem> = 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,
codex-rs/tui/src/app/side.rs
+1
View File
@@ -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
codex-rs/tui/src/app/tests.rs
+59
View File
@@ -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));
codex-rs/tui/src/app/thread_routing.rs
+44 -24
View File
@@ -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;
codex-rs/tui/src/bottom_pane/pending_thread_approvals.rs
+3 -3
View File
@@ -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...........
"
);
}
codex-rs/tui/src/snapshots/codex_tui__app__tests__path_backed_agent_picker.snap
+3 -1
View File
@@ -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