From b01148de595719a05092dcbd5e0a252bb4349378 Mon Sep 17 00:00:00 2001 From: Benjamin Carlsson Date: Tue, 27 Oct 2026 00:20:38 +0000 Subject: [PATCH] Keep failed subagents from marking tasks as needing attention (#57315) ## Why A subagent failure ends its work without requiring user action, but the agent command center treated it as needing attention and promoted its parent task into the `Needs you` group. ## What changed Classify failed subagents as inactive when computing task groups and selecting details. Recognize subagents by either a parent thread or a subagent session source, including background workers without a parent. Keep approval and user-input requests from descendants actionable, even beneath a failed agent. Show these requests as `Needs input` rather than `Error` when the displayed task is a failed subagent. Errors in non-subagent tasks continue to require attention. ## Testing Add and update regression tests and a snapshot covering stable ordering after subagent failures, idle parents staying ready, failed background workers, and pending requests beneath failed agents. GitOrigin-RevId: 1a06d5d00c08bb77c9ed36aadffd5e0514f0b2c6 --- codex-rs/tui/src/app/agent_center_tests.rs | 75 +++++++++++++++---- codex-rs/tui/src/app/agents_overview.rs | 11 ++- .../tui/src/app/agents_overview_details.rs | 4 +- codex-rs/tui/src/app/agents_overview_tests.rs | 15 ++-- codex-rs/tui/src/app/agents_overview_view.rs | 18 +++-- ...tatus_order_across_progress_refreshes.snap | 4 +- 6 files changed, 95 insertions(+), 32 deletions(-) diff --git a/codex-rs/tui/src/app/agent_center_tests.rs b/codex-rs/tui/src/app/agent_center_tests.rs index 952a4d687d..40ec900d39 100644 --- a/codex-rs/tui/src/app/agent_center_tests.rs +++ b/codex-rs/tui/src/app/agent_center_tests.rs @@ -91,13 +91,57 @@ async fn live_center_keeps_status_order_across_progress_refreshes() { .unwrap(); assert_eq!(app.agents_overview.visible_thread_ids[selected], ids[0]); } + for parent in [ids[0], ids[3]] { + threads.push(overview_thread( + ThreadId::new(), + Some(parent), + "Failed subagent", + ThreadStatus::SystemError, + )); + } let view = app.agents_overview_view(threads, Some(ids[0])); + assert!( + view.rows[view.selected_index().unwrap()] + .details + .lines + .is_empty() + ); let after = screen(&view, /*width*/ 100, /*height*/ 18); insta::assert_snapshot!(format!( - "Initial display:\n{before}\n\nAfter progress updates:\n{after}" + "Initial display:\n{before}\n\nAfter subagents fail under Alpha and Ready task:\n{after}" )); } +#[tokio::test] +async fn failed_background_worker_only_needs_attention_for_a_pending_request() { + let app = make_test_app().await; + let worker_id = ThreadId::new(); + let mut worker = overview_thread( + worker_id, + /*parent_thread_id*/ None, + "Background review", + ThreadStatus::SystemError, + ); + worker.source = SessionSource::SubAgent(SubAgentSource::Review); + let view = app.agents_overview_view(vec![worker.clone()], Some(worker_id)); + assert_eq!(view.rows[0].group, AgentsOverviewGroup::Finished); + assert!(view.rows[0].details.lines.is_empty()); + + let child = overview_thread( + ThreadId::new(), + Some(worker_id), + "Check dependencies", + ThreadStatus::Active { + active_flags: vec![ThreadActiveFlag::WaitingOnApproval], + }, + ); + let view = app.agents_overview_view(vec![worker, child], Some(worker_id)); + assert_eq!(view.rows[0].group, AgentsOverviewGroup::NeedsYou); + let rendered = screen(&view, /*width*/ 100, /*height*/ 32); + assert!(rendered.contains("● Needs input")); + assert!(rendered.contains("Waiting for approval.")); +} + #[tokio::test] async fn live_center_appends_arrivals_and_status_transitions() { let app = make_test_app().await; @@ -133,24 +177,29 @@ async fn live_center_appends_arrivals_and_status_transitions() { let mut arrival = overview_thread(delta, /*parent_thread_id*/ None, "Delta", working); arrival.updated_at += 100; threads.push(arrival); + let mut child = overview_thread( + child, + /*parent_thread_id*/ None, + "Child", + ThreadStatus::SystemError, + ); + child.parent_thread_id = Some(bravo.to_string()); + threads.push(child); assert_eq!( app.agents_overview_view(threads.clone(), Some(bravo)) .thread_ids(), vec![alpha, bravo, delta, charlie] ); threads[1].status = ThreadStatus::Idle; - assert_eq!( - app.agents_overview_view(threads.clone(), Some(bravo)) - .thread_ids(), - vec![alpha, delta, charlie, bravo] - ); - // A descendant can change the root's effective group without changing its own status. - threads.push(overview_thread( - child, - Some(bravo), - "Child", - ThreadStatus::SystemError, - )); + let view = app.agents_overview_view(threads.clone(), Some(bravo)); + assert_eq!(view.thread_ids(), vec![alpha, delta, charlie, bravo]); + let selected = &view.rows[view.selected_index().unwrap()]; + assert_eq!(selected.group, AgentsOverviewGroup::Ready); + assert!(selected.details.lines.is_empty()); + // A request from a descendant can still bring its idle parent to the top. + threads.last_mut().unwrap().status = ThreadStatus::Active { + active_flags: vec![ThreadActiveFlag::WaitingOnApproval], + }; assert_eq!( app.agents_overview_view(threads.clone(), Some(bravo)) .thread_ids(), diff --git a/codex-rs/tui/src/app/agents_overview.rs b/codex-rs/tui/src/app/agents_overview.rs index 0176c9cdd5..82160d24d9 100644 --- a/codex-rs/tui/src/app/agents_overview.rs +++ b/codex-rs/tui/src/app/agents_overview.rs @@ -1224,8 +1224,11 @@ fn agents_overview_group( thread: &Thread, children: &HashMap>, ) -> AgentsOverviewGroup { - children.get(&thread.id).into_iter().flatten().fold( - AgentsOverviewGroup::for_status(&thread.status), - |group, child| group.min(agents_overview_group(child, children)), - ) + children + .get(&thread.id) + .into_iter() + .flatten() + .fold(AgentsOverviewGroup::for_thread(thread), |group, child| { + group.min(agents_overview_group(child, children)) + }) } diff --git a/codex-rs/tui/src/app/agents_overview_details.rs b/codex-rs/tui/src/app/agents_overview_details.rs index 5a8fa768c3..9629ea829f 100644 --- a/codex-rs/tui/src/app/agents_overview_details.rs +++ b/codex-rs/tui/src/app/agents_overview_details.rs @@ -139,7 +139,7 @@ impl App { let mut lines = Vec::new(); // Urgency comes first; on ties, prefer available current detail over history. let priority = |thread: &Thread| { - let group = AgentsOverviewGroup::for_status(&thread.status); + let group = AgentsOverviewGroup::for_thread(thread); let has_detail = ThreadId::from_string(&thread.id).is_ok_and(|id| match group { AgentsOverviewGroup::NeedsYou => self.agents_overview_request_preview(id).is_some(), AgentsOverviewGroup::Working => { @@ -180,7 +180,7 @@ impl App { lines.push(Line::default()); lines.push(vec!["Agent: ".dim(), preview_text(name).into()].into()); } - if AgentsOverviewGroup::for_status(&source.status) == AgentsOverviewGroup::NeedsYou { + if AgentsOverviewGroup::for_thread(source) == AgentsOverviewGroup::NeedsYou { if !is_child { lines.extend([Line::default(), "Needs attention".red().into()]); } diff --git a/codex-rs/tui/src/app/agents_overview_tests.rs b/codex-rs/tui/src/app/agents_overview_tests.rs index 57e9b69159..9ad879d931 100644 --- a/codex-rs/tui/src/app/agents_overview_tests.rs +++ b/codex-rs/tui/src/app/agents_overview_tests.rs @@ -5,6 +5,7 @@ use crate::app::app_server_requests::BufferedServerRequest; #[path = "agent_center_tests.rs"] mod command_center; +use super::super::agents_overview_view::AgentsOverviewGroup; use super::super::agents_overview_view::AgentsOverviewGrouping; use super::*; use crate::app::tests::make_test_app_with_channels; @@ -782,7 +783,7 @@ async fn check_hidden_thread_does_not_refresh_shared_overview(source: &str) { async fn agents_overview_details_show_available_attention_without_expanding_rows() -> Result<()> { let mut app = make_test_app().await; let app_server = crate::start_embedded_app_server_for_picker(&app.config).await?; - let [root, child, unloaded] = std::array::from_fn(|_| ThreadId::new()); + let [root, failed_agent, child, unloaded] = std::array::from_fn(|_| ThreadId::new()); let waiting = ThreadStatus::Active { active_flags: vec![ ThreadActiveFlag::WaitingOnApproval, @@ -792,13 +793,19 @@ async fn agents_overview_details_show_available_attention_without_expanding_rows // The child question takes priority over the generic waiting parent. let mut threads = [ (root, None, "Repair authentication", waiting.clone()), - (child, Some(root), "Check dependencies", waiting), + (child, Some(failed_agent), "Check dependencies", waiting), ( unloaded, None, "Investigate parser", ThreadStatus::NotLoaded, ), + ( + failed_agent, + Some(root), + "Failed agent", + ThreadStatus::SystemError, + ), ] .map(|(id, parent, title, status)| overview_thread(id, parent, title, status)) .to_vec(); @@ -1346,10 +1353,6 @@ async fn overview_model_grouping_shows_details_and_preserves_selection() { #[tokio::test] async fn shared_overview_shows_only_root_sessions() { - assert_eq!( - AgentsOverviewGroup::for_status(&ThreadStatus::SystemError), - AgentsOverviewGroup::NeedsYou - ); let mut app = make_test_app().await; let first_root = ThreadId::from_string("00000000-0000-0000-0000-000000000101").unwrap(); let unloaded_root = ThreadId::from_string("00000000-0000-0000-0000-000000000102").unwrap(); diff --git a/codex-rs/tui/src/app/agents_overview_view.rs b/codex-rs/tui/src/app/agents_overview_view.rs index 4e9da2b0d0..c3843db7f1 100644 --- a/codex-rs/tui/src/app/agents_overview_view.rs +++ b/codex-rs/tui/src/app/agents_overview_view.rs @@ -36,6 +36,7 @@ use crate::terminal_hyperlinks::mark_buffer_hyperlinks; use crate::terminal_hyperlinks::plain_hyperlink_lines; use crate::terminal_hyperlinks::remap_wrapped_line; use crate::terminal_hyperlinks::visible_lines; +use codex_app_server_protocol::SessionSource; use codex_app_server_protocol::Thread; use codex_app_server_protocol::ThreadActiveFlag; use codex_app_server_protocol::ThreadStatus; @@ -74,8 +75,8 @@ pub(super) enum AgentsOverviewGroup { } impl AgentsOverviewGroup { - pub(super) fn for_status(status: &ThreadStatus) -> Self { - match status { + pub(super) fn for_thread(thread: &Thread) -> Self { + match &thread.status { ThreadStatus::Active { active_flags } if active_flags.contains(&ThreadActiveFlag::WaitingOnApproval) || active_flags.contains(&ThreadActiveFlag::WaitingOnUserInput) => @@ -84,8 +85,9 @@ impl AgentsOverviewGroup { } ThreadStatus::Active { .. } => Self::Working, ThreadStatus::Idle => Self::Ready, - ThreadStatus::SystemError => Self::NeedsYou, - ThreadStatus::NotLoaded => Self::Finished, + // A subagent failure ends its work without asking the user to act. + ThreadStatus::SystemError if !is_subagent(thread) => Self::NeedsYou, + ThreadStatus::SystemError | ThreadStatus::NotLoaded => Self::Finished, } } @@ -99,6 +101,10 @@ impl AgentsOverviewGroup { } } +fn is_subagent(thread: &Thread) -> bool { + thread.parent_thread_id.is_some() || matches!(thread.source, SessionSource::SubAgent(_)) +} + #[derive(Clone)] pub(super) struct AgentsOverviewRow { pub(super) details: AgentsOverviewDetails, @@ -427,7 +433,9 @@ impl AgentsOverviewView { fn status(row: &AgentsOverviewRow) -> (&'static str, Span<'static>) { match row.group { - AgentsOverviewGroup::NeedsYou if row.thread.status == ThreadStatus::SystemError => { + AgentsOverviewGroup::NeedsYou + if row.thread.status == ThreadStatus::SystemError && !is_subagent(&row.thread) => + { ("Error", "!".red()) } AgentsOverviewGroup::NeedsYou => ("Needs input", "●".red()), diff --git a/codex-rs/tui/src/app/snapshots/codex_tui__app__agents_overview__tests__command_center__live_center_keeps_status_order_across_progress_refreshes.snap b/codex-rs/tui/src/app/snapshots/codex_tui__app__agents_overview__tests__command_center__live_center_keeps_status_order_across_progress_refreshes.snap index eb6f4c954e..a0a543c046 100644 --- a/codex-rs/tui/src/app/snapshots/codex_tui__app__agents_overview__tests__command_center__live_center_keeps_status_order_across_progress_refreshes.snap +++ b/codex-rs/tui/src/app/snapshots/codex_tui__app__agents_overview__tests__command_center__live_center_keeps_status_order_across_progress_refreshes.snap @@ -1,6 +1,6 @@ --- source: tui/src/app/agent_center_tests.rs -expression: "format!(\"Initial display:\\n{before}\\n\\nAfter progress updates:\\n{after}\")" +expression: "format!(\"Initial display:\\n{before}\\n\\nAfter subagents fail under Alpha and Ready task:\\n{after}\")" --- Initial display: Agent command center Group: Status g @@ -22,7 +22,7 @@ Initial display: │ ? help esc back ↑/↓ move enter open n new -After progress updates: +After subagents fail under Alpha and Ready task: Agent command center Group: Status g All 5 Needs you 1 Working 3 Ready 1 Inactive 0 tab/⇧tab filter ────────────────────────────────────────────────────────────────────────────────────────────────