mirror of
https://github.com/openai/codex.git
synced 2026-10-28 18:58:46 +00:00
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
This commit is contained in:
committed by
copyberry
parent
95cafae96c
commit
b01148de59
@@ -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(),
|
||||
|
||||
@@ -1224,8 +1224,11 @@ fn agents_overview_group(
|
||||
thread: &Thread,
|
||||
children: &HashMap<String, Vec<&Thread>>,
|
||||
) -> 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))
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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()]);
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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()),
|
||||
|
||||
+2
-2
@@ -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
|
||||
────────────────────────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user