diff --git a/crates/protocol/src/subscription.rs b/crates/protocol/src/subscription.rs index 71f8cf2d..053f7810 100644 --- a/crates/protocol/src/subscription.rs +++ b/crates/protocol/src/subscription.rs @@ -554,6 +554,8 @@ pub struct SubscriptionWorker { /// Producer-owned monotonic revision for this Worker subject. pub subject_revision: u64, pub state: SubscriptionWorkerState, + #[serde(default)] + pub has_running_internal_workers: bool, #[serde(default, skip_serializing_if = "Option::is_none")] pub workspace_id: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -796,6 +798,7 @@ mod tests { runtime_id: None, subject_revision: 0, state: SubscriptionWorkerState::Idle, + has_running_internal_workers: false, workspace_id: Some("workspace-1".to_string()), display_name: Some(format!("Worker {value}")), profile: Some("builtin:coder".to_string()), diff --git a/crates/tui/src/app.rs b/crates/tui/src/app.rs index 1916605a..afef38d0 100644 --- a/crates/tui/src/app.rs +++ b/crates/tui/src/app.rs @@ -233,6 +233,12 @@ pub struct InternalWorkerView { pub app: Box, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct WorkerViewTab { + pub label: String, + pub selected: bool, +} + pub struct App { pub worker_name: String, pub connected: bool, @@ -281,8 +287,11 @@ pub struct App { /// replayable conversation rows during segment rotation. run_error_messages: Vec, /// Presentation-only Internal Worker projections keyed by session identity. - /// They are rendered in separate sub-panes and never mixed into `blocks`. + /// They are rendered in separate selectable views and never mixed into `blocks`. pub internal_workers: Vec, + /// Selected Internal Worker transcript/task view. `None` is the parent (`main`) + /// view; the stable session identity survives projection reordering. + selected_internal_worker_session_id: Option, /// Terminal child-session fences, reset only by an authoritative snapshot. removed_internal_workers: HashMap, pub scroll: Scroll, @@ -363,6 +372,7 @@ impl App { blocks: Vec::new(), run_error_messages: Vec::new(), internal_workers: Vec::new(), + selected_internal_worker_session_id: None, removed_internal_workers: HashMap::new(), scroll: Scroll::default(), mode: Mode::Normal, @@ -448,16 +458,98 @@ impl App { pub fn toggle_task_pane(&mut self) { self.task_pane_open = !self.task_pane_open; if !self.task_pane_open { - self.task_pane_scroll = 0; + self.selected_worker_view_mut().task_pane_scroll = 0; + } + } + + pub fn worker_view_tabs(&self) -> Vec { + let selected = self.selected_internal_worker_session_id.as_deref(); + let mut tabs = Vec::with_capacity(self.internal_workers.len().saturating_add(1)); + tabs.push(WorkerViewTab { + label: "main".to_owned(), + selected: selected.is_none(), + }); + tabs.extend(self.internal_workers.iter().map(|view| { + WorkerViewTab { + label: view + .worker + .name + .lines() + .next() + .filter(|name| !name.is_empty()) + .unwrap_or("subworker") + .to_owned(), + selected: selected == Some(view.worker.session_id.as_str()), + } + })); + tabs + } + + pub fn selected_internal_worker_index(&self) -> Option { + let selected = self.selected_internal_worker_session_id.as_deref()?; + self.internal_workers + .iter() + .position(|view| view.worker.session_id == selected) + } + + pub fn selected_worker_view(&self) -> &App { + self.selected_internal_worker_index() + .map(|index| self.internal_workers[index].app.as_ref()) + .unwrap_or(self) + } + + pub fn selected_worker_view_mut(&mut self) -> &mut App { + if let Some(index) = self.selected_internal_worker_index() { + self.internal_workers[index].app.as_mut() + } else { + self + } + } + + /// Cycle the presentation-only transcript/task view. Input and control + /// methods continue to target the parent Worker regardless of selection. + pub fn cycle_worker_view(&mut self) -> bool { + if self.internal_workers.is_empty() { + self.selected_internal_worker_session_id = None; + return false; + } + + self.selected_internal_worker_session_id = self + .selected_internal_worker_index() + .and_then(|index| self.internal_workers.get(index.saturating_add(1))) + .map(|view| view.worker.session_id.clone()) + .or_else(|| { + if self.selected_internal_worker_session_id.is_none() { + self.internal_workers + .first() + .map(|view| view.worker.session_id.clone()) + } else { + None + } + }); + true + } + + pub fn cycle_mode(&mut self) { + let mode = self.mode.cycle(); + self.set_mode_recursively(mode); + } + + fn set_mode_recursively(&mut self, mode: Mode) { + self.mode = mode; + for view in &mut self.internal_workers { + view.app.set_mode_recursively(mode); } } pub fn scroll_task_pane_up(&mut self, n: usize) { - self.task_pane_scroll = self.task_pane_scroll.saturating_sub(n); + let view = self.selected_worker_view_mut(); + view.task_pane_scroll = view.task_pane_scroll.saturating_sub(n); } pub fn scroll_task_pane_down(&mut self, n: usize) { - self.task_pane_scroll = self.task_pane_scroll.saturating_add(n); + let view = self.selected_worker_view_mut(); + view.task_pane_scroll = view.task_pane_scroll.saturating_add(n); } pub fn set_worker_status(&mut self, status: WorkerStatus) { @@ -1747,6 +1839,9 @@ impl App { } pub fn request_rewind_picker(&mut self) -> Option { + // Rewind is a parent Worker control surface. Bring the parent transcript + // back into view before presenting diagnostics or the picker. + self.selected_internal_worker_session_id = None; if self.rewind_submit_pending() { self.push_command_diagnostic( "rewind is already applying; wait for the Worker response", @@ -2007,15 +2102,66 @@ impl App { /// produced. Followed by `Event::Entry` updates for anything /// committed after the snapshot. fn replace_internal_worker_snapshots(&mut self, snapshots: Vec) { + let mode = self.mode; + let mut previous = std::mem::take(&mut self.internal_workers); self.internal_workers = snapshots .into_iter() - .map(Self::internal_worker_view_from_snapshot) + .map(|snapshot| { + if let Some(index) = previous + .iter() + .position(|view| view.worker.session_id == snapshot.worker.session_id) + { + let view = previous.remove(index); + Self::update_internal_worker_view_from_snapshot(view, snapshot, mode) + } else { + Self::internal_worker_view_from_snapshot(snapshot, mode) + } + }) .collect(); + if self.selected_internal_worker_index().is_none() { + self.selected_internal_worker_session_id = None; + } self.removed_internal_workers.clear(); } - fn internal_worker_view_from_snapshot(snapshot: InternalWorkerSnapshot) -> InternalWorkerView { + fn update_internal_worker_view_from_snapshot( + mut previous: InternalWorkerView, + snapshot: InternalWorkerSnapshot, + mode: Mode, + ) -> InternalWorkerView { + let mut refreshed = Self::internal_worker_view_from_snapshot(snapshot, mode); + Self::transfer_worker_view_state(&mut previous.app, &mut refreshed.app); + refreshed + } + + fn transfer_worker_view_state(previous: &mut App, refreshed: &mut App) { + refreshed.scroll = std::mem::take(&mut previous.scroll); + refreshed.text_selection = std::mem::take(&mut previous.text_selection); + refreshed.task_pane_scroll = previous.task_pane_scroll; + refreshed.selected_internal_worker_session_id = + previous.selected_internal_worker_session_id.take(); + + let mut previous_children = std::mem::take(&mut previous.internal_workers); + for child in &mut refreshed.internal_workers { + if let Some(index) = previous_children + .iter() + .position(|old| old.worker.session_id == child.worker.session_id) + { + let mut old = previous_children.remove(index); + Self::transfer_worker_view_state(&mut old.app, &mut child.app); + } + } + if refreshed.selected_internal_worker_index().is_none() { + refreshed.selected_internal_worker_session_id = None; + } + } + + fn internal_worker_view_from_snapshot( + snapshot: InternalWorkerSnapshot, + mode: Mode, + ) -> InternalWorkerView { let mut app = App::new(snapshot.worker.name.clone()); + app.mode = mode; app.restore_entries(&snapshot.entries, None); app.apply_in_flight_snapshot(snapshot.in_flight); app.set_worker_status(snapshot.status); @@ -2052,10 +2198,12 @@ impl App { let target = if let Some(index) = index { &mut self.internal_workers[index] } else { + let mut app = App::new(worker.name.clone()); + app.mode = self.mode; self.internal_workers.push(InternalWorkerView { worker: worker.clone(), revision: 0, - app: Box::new(App::new(worker.name.clone())), + app: Box::new(app), }); self.internal_workers.last_mut().unwrap() }; @@ -2068,13 +2216,17 @@ impl App { } fn remove_internal_worker(&mut self, worker: InternalWorkerRef, revision: u64) { + let session_id = worker.session_id; let Some(index) = self .internal_workers .iter() - .position(|candidate| candidate.worker.session_id == worker.session_id) + .position(|candidate| candidate.worker.session_id == session_id) else { + if self.selected_internal_worker_session_id.as_deref() == Some(session_id.as_str()) { + self.selected_internal_worker_session_id = None; + } self.removed_internal_workers - .entry(worker.session_id) + .entry(session_id) .and_modify(|current| *current = (*current).max(revision)) .or_insert(revision); return; @@ -2083,8 +2235,10 @@ impl App { return; } self.internal_workers.remove(index); - self.removed_internal_workers - .insert(worker.session_id, revision); + if self.selected_internal_worker_session_id.as_deref() == Some(session_id.as_str()) { + self.selected_internal_worker_session_id = None; + } + self.removed_internal_workers.insert(session_id, revision); } fn restore_snapshot( @@ -3583,6 +3737,188 @@ mod completion_flow_tests { ); } + fn test_internal_worker_snapshot( + session_id: &str, + name: &str, + revision: u64, + ) -> InternalWorkerSnapshot { + InternalWorkerSnapshot { + worker: InternalWorkerRef { + session_id: session_id.into(), + name: name.into(), + parent_session_id: Some("parent".into()), + kind: protocol::InternalWorkerKind::SubWorker, + }, + revision, + status: WorkerStatus::Idle, + entries: Vec::new(), + in_flight: protocol::InFlightSnapshot::default(), + error: None, + internal_workers: Vec::new(), + } + } + + #[test] + fn worker_view_cycle_uses_stable_session_identity_and_wraps_to_main() { + let mut app = App::new("parent".into()); + for (session_id, name) in [("child-a", "alpha"), ("child-b", "beta")] { + app.internal_workers.push(InternalWorkerView { + worker: InternalWorkerRef { + session_id: session_id.into(), + name: name.into(), + parent_session_id: Some("parent".into()), + kind: protocol::InternalWorkerKind::SubWorker, + }, + revision: 1, + app: Box::new(App::new(name.into())), + }); + } + + assert_eq!(app.selected_worker_view().worker_name, "parent"); + assert!(app.cycle_worker_view()); + assert_eq!(app.selected_worker_view().worker_name, "alpha"); + + app.internal_workers.swap(0, 1); + assert_eq!(app.selected_worker_view().worker_name, "alpha"); + assert!(app.cycle_worker_view()); + assert_eq!(app.selected_worker_view().worker_name, "parent"); + assert!(app.cycle_worker_view()); + assert_eq!(app.selected_worker_view().worker_name, "beta"); + } + + #[test] + fn worker_view_cycle_preserves_each_views_text_selection() { + use crate::text_selection::{HistoryViewport, SelectionRow}; + + fn select_first_row(app: &mut App, text: &str) { + app.text_selection.set_history_snapshot( + HistoryViewport { + x: 0, + y: 0, + width: 20, + height: 1, + top_offset: 0, + total_lines: 1, + }, + vec![SelectionRow::new(text.into(), true)], + ); + assert!(app.text_selection.begin_drag(0, 0)); + } + + let mut app = App::new("parent".into()); + app.replace_internal_worker_snapshots(vec![test_internal_worker_snapshot( + "child", "child", 1, + )]); + select_first_row(&mut app, "parent selection"); + select_first_row(app.internal_workers[0].app.as_mut(), "child selection"); + + app.cycle_worker_view(); + assert!(app.selected_worker_view().text_selection.has_selection()); + app.cycle_worker_view(); + + assert!(app.text_selection.has_selection()); + assert!(app.internal_workers[0].app.text_selection.has_selection()); + } + + #[test] + fn snapshot_removal_falls_selected_worker_view_back_to_main() { + let mut app = App::new("parent".into()); + app.internal_workers.push(InternalWorkerView { + worker: InternalWorkerRef { + session_id: "old".into(), + name: "old".into(), + parent_session_id: Some("parent".into()), + kind: protocol::InternalWorkerKind::SubWorker, + }, + revision: 1, + app: Box::new(App::new("old".into())), + }); + app.cycle_worker_view(); + assert_eq!(app.selected_worker_view().worker_name, "old"); + + app.replace_internal_worker_snapshots(Vec::new()); + + assert_eq!(app.selected_worker_view().worker_name, "parent"); + assert_eq!( + app.worker_view_tabs(), + vec![WorkerViewTab { + label: "main".into(), + selected: true, + }] + ); + } + + #[test] + fn same_session_snapshot_preserves_subworker_view_local_state() { + use crate::text_selection::{HistoryViewport, SelectionRow}; + + let mut app = App::new("parent".into()); + app.replace_internal_worker_snapshots(vec![test_internal_worker_snapshot( + "child", "child", 1, + )]); + let child = app.internal_workers[0].app.as_mut(); + child.scroll.follow_tail = false; + child.scroll.top_offset = 7; + child.task_pane_scroll = 4; + child.text_selection.set_history_snapshot( + HistoryViewport { + x: 0, + y: 0, + width: 20, + height: 1, + top_offset: 0, + total_lines: 1, + }, + vec![SelectionRow::new("selected".into(), true)], + ); + assert!(child.text_selection.begin_drag(0, 0)); + + app.replace_internal_worker_snapshots(vec![test_internal_worker_snapshot( + "child", + "renamed-child", + 2, + )]); + + let view = &app.internal_workers[0]; + assert_eq!(view.revision, 2); + assert_eq!(view.app.worker_name, "renamed-child"); + assert!(!view.app.scroll.follow_tail); + assert_eq!(view.app.scroll.top_offset, 7); + assert_eq!(view.app.task_pane_scroll, 4); + assert!(view.app.text_selection.has_selection()); + } + + #[test] + fn task_pane_scroll_is_local_to_selected_worker_view() { + let mut app = App::new("parent".into()); + app.replace_internal_worker_snapshots(vec![ + test_internal_worker_snapshot("child-a", "alpha", 1), + test_internal_worker_snapshot("child-b", "beta", 1), + ]); + app.task_pane_scroll = 3; + + app.cycle_worker_view(); + app.scroll_task_pane_down(5); + assert_eq!(app.selected_worker_view().task_pane_scroll, 5); + + app.cycle_worker_view(); + app.scroll_task_pane_down(7); + assert_eq!(app.selected_worker_view().task_pane_scroll, 7); + + app.cycle_worker_view(); + assert_eq!(app.selected_worker_view().worker_name, "parent"); + assert_eq!(app.task_pane_scroll, 3); + assert_eq!(app.internal_workers[0].app.task_pane_scroll, 5); + assert_eq!(app.internal_workers[1].app.task_pane_scroll, 7); + + app.cycle_worker_view(); + app.toggle_task_pane(); + app.toggle_task_pane(); + assert_eq!(app.internal_workers[0].app.task_pane_scroll, 0); + assert_eq!(app.internal_workers[1].app.task_pane_scroll, 7); + assert_eq!(app.task_pane_scroll, 3); + } + #[test] fn terminal_internal_worker_removal_drops_descendants_and_fences_late_events() { let mut app = App::new("parent".into()); @@ -3611,6 +3947,8 @@ mod completion_flow_tests { }); assert_eq!(app.internal_workers.len(), 1); assert_eq!(app.internal_workers[0].app.internal_workers.len(), 1); + app.cycle_worker_view(); + assert_eq!(app.selected_worker_view().worker_name, "child"); app.handle_worker_event(Event::InternalWorkerRemoved { worker: worker.clone(), @@ -3625,6 +3963,7 @@ mod completion_flow_tests { }); assert!(app.internal_workers.is_empty()); + assert_eq!(app.selected_worker_view().worker_name, "parent"); app.handle_worker_event(Event::Snapshot { greeting: test_greeting(), entries: Vec::new(), diff --git a/crates/tui/src/console/mod.rs b/crates/tui/src/console/mod.rs index 8c32fac8..e7991a9c 100644 --- a/crates/tui/src/console/mod.rs +++ b/crates/tui/src/console/mod.rs @@ -96,12 +96,12 @@ fn copy_to_terminal_clipboard(out: &mut W, text: &str) -> io::Resu } fn copy_selection_to_writer(app: &mut App, out: &mut W) -> bool { - let Some(text) = app.text_selection.copy_text() else { + let Some(text) = app.selected_worker_view_mut().text_selection.copy_text() else { return false; }; let result = copy_to_terminal_clipboard(out, &text); - app.text_selection.clear(); + app.selected_worker_view_mut().text_selection.clear(); match result { Ok(()) => { app.flash_actionbar_notice( @@ -890,25 +890,27 @@ const WHEEL_LINES: usize = 3; const PANE_SCROLL_LINES: usize = 5; fn handle_mouse(app: &mut App, mouse: MouseEvent) { + let rewind_picker_open = app.rewind_picker.is_some(); + let view = app.selected_worker_view_mut(); match mouse.kind { MouseEventKind::ScrollUp => { - app.text_selection.clear(); - app.scroll.scroll_up(WHEEL_LINES); + view.text_selection.clear(); + view.scroll.scroll_up(WHEEL_LINES); } MouseEventKind::ScrollDown => { - app.text_selection.clear(); - app.scroll.scroll_down(WHEEL_LINES); + view.text_selection.clear(); + view.scroll.scroll_down(WHEEL_LINES); } - MouseEventKind::Down(MouseButton::Left) if app.rewind_picker.is_none() => { - if !app.text_selection.begin_drag(mouse.column, mouse.row) { - app.text_selection.clear(); + MouseEventKind::Down(MouseButton::Left) if !rewind_picker_open => { + if !view.text_selection.begin_drag(mouse.column, mouse.row) { + view.text_selection.clear(); } } - MouseEventKind::Drag(MouseButton::Left) if app.rewind_picker.is_none() => { - app.text_selection.update_drag(mouse.column, mouse.row); + MouseEventKind::Drag(MouseButton::Left) if !rewind_picker_open => { + view.text_selection.update_drag(mouse.column, mouse.row); } - MouseEventKind::Up(MouseButton::Left) if app.rewind_picker.is_none() => { - app.text_selection.finish_drag(mouse.column, mouse.row); + MouseEventKind::Up(MouseButton::Left) if !rewind_picker_open => { + view.text_selection.finish_drag(mouse.column, mouse.row); } _ => {} } @@ -942,31 +944,31 @@ fn handle_key(app: &mut App, key: KeyEvent) -> Option { // Modifier-key bindings. if let Some(method) = match key.code { KeyCode::Up if shift => { - app.scroll.scroll_up(1); + app.selected_worker_view_mut().scroll.scroll_up(1); Some(None) } KeyCode::Down if shift => { - app.scroll.scroll_down(1); + app.selected_worker_view_mut().scroll.scroll_down(1); Some(None) } KeyCode::Home if ctrl => { - app.scroll.to_top(); + app.selected_worker_view_mut().scroll.to_top(); Some(None) } KeyCode::End if ctrl => { - app.scroll.to_bottom(); + app.selected_worker_view_mut().scroll.to_bottom(); Some(None) } KeyCode::Char('[') if ctrl => { - app.scroll.jump_prev_turn(); + app.selected_worker_view_mut().scroll.jump_prev_turn(); Some(None) } KeyCode::Char(']') if ctrl => { - app.scroll.jump_next_turn(); + app.selected_worker_view_mut().scroll.jump_next_turn(); Some(None) } KeyCode::Char('o') if ctrl => { - app.mode = app.mode.cycle(); + app.cycle_mode(); Some(None) } KeyCode::Char('t') if ctrl => { @@ -1047,7 +1049,7 @@ fn handle_key(app: &mut App, key: KeyEvent) -> Option { if app.task_pane_open { app.scroll_task_pane_up(PANE_SCROLL_LINES); } else { - app.scroll.page_up(); + app.selected_worker_view_mut().scroll.page_up(); } return None; } @@ -1055,7 +1057,7 @@ fn handle_key(app: &mut App, key: KeyEvent) -> Option { if app.task_pane_open { app.scroll_task_pane_down(PANE_SCROLL_LINES); } else { - app.scroll.page_down(); + app.selected_worker_view_mut().scroll.page_down(); } return None; } @@ -1130,12 +1132,17 @@ fn handle_key(app: &mut App, key: KeyEvent) -> Option { } } + if key.code == KeyCode::Tab && key.modifiers.is_empty() && app.completion.is_none() { + app.cycle_worker_view(); + return None; + } + if key.modifiers.is_empty() { match key.code { - KeyCode::Esc if app.text_selection.clear() => return None, - KeyCode::Char('y') if app.text_selection.has_selection() => { + KeyCode::Esc if app.selected_worker_view_mut().text_selection.clear() => return None, + KeyCode::Char('y') if app.selected_worker_view().text_selection.has_selection() => { if !copy_selection_to_terminal(app) { - app.text_selection.clear(); + app.selected_worker_view_mut().text_selection.clear(); app.flash_actionbar_notice( "Selection contains no copyable text.", ActionbarNoticeLevel::Warn, @@ -2170,6 +2177,18 @@ mod tests { #[test] fn command_completion_tab_applies_unambiguous_candidate() { let mut app = App::new("agent".to_string()); + app.handle_worker_event(Event::InternalWorker { + worker: protocol::InternalWorkerRef { + session_id: "child-session".into(), + name: "subworker-hoge".into(), + parent_session_id: Some("parent-session".into()), + kind: protocol::InternalWorkerKind::SubWorker, + }, + revision: 1, + event: Box::new(Event::Status { + status: WorkerStatus::Running, + }), + }); enter_command_mode(&mut app); type_keys(&mut app, "no"); @@ -2177,6 +2196,7 @@ mod tests { assert!(app.is_command_mode()); assert_eq!(app.command_text(), "noop "); + assert_eq!(app.selected_worker_view().worker_name, "agent"); assert_eq!(input_text(&app), ""); } @@ -2269,6 +2289,89 @@ mod tests { assert_eq!(input_text(&app), ""); } + #[test] + fn tab_cycles_main_and_subworker_view_without_changing_composer() { + let mut app = App::new("agent".to_string()); + type_keys(&mut app, "hello"); + app.handle_worker_event(Event::InternalWorker { + worker: protocol::InternalWorkerRef { + session_id: "child-session".into(), + name: "subworker-hoge".into(), + parent_session_id: Some("parent-session".into()), + kind: protocol::InternalWorkerKind::SubWorker, + }, + revision: 1, + event: Box::new(Event::Status { + status: WorkerStatus::Running, + }), + }); + + assert!(handle_key(&mut app, key(KeyCode::Tab)).is_none()); + assert_eq!(app.selected_worker_view().worker_name, "subworker-hoge"); + assert_eq!(input_text(&app), "hello"); + + assert!(handle_key(&mut app, key(KeyCode::Tab)).is_none()); + assert_eq!(app.selected_worker_view().worker_name, "agent"); + assert_eq!(input_text(&app), "hello"); + } + + #[test] + fn subworker_view_does_not_redirect_parent_worker_controls() { + let mut app = App::new("agent".to_string()); + app.set_worker_status(WorkerStatus::Idle); + app.handle_worker_event(Event::InternalWorker { + worker: protocol::InternalWorkerRef { + session_id: "child-session".into(), + name: "subworker-hoge".into(), + parent_session_id: Some("parent-session".into()), + kind: protocol::InternalWorkerKind::SubWorker, + }, + revision: 1, + event: Box::new(Event::Status { + status: WorkerStatus::Running, + }), + }); + handle_key(&mut app, key(KeyCode::Tab)); + assert_eq!(app.selected_worker_view().worker_name, "subworker-hoge"); + + let method = handle_key( + &mut app, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::CONTROL), + ); + + assert!(matches!(method, Some(Method::Shutdown))); + assert_eq!(app.worker_status, WorkerStatus::Idle); + } + + #[test] + fn active_composer_completion_takes_tab_priority_over_worker_view_cycle() { + let mut app = App::new("agent".to_string()); + app.insert_char('@'); + app.insert_char('s'); + let _ = app.refresh_completion(); + app.completion.as_mut().unwrap().entries = vec![protocol::CompletionEntry { + value: "src/main.rs".into(), + is_dir: false, + }]; + app.handle_worker_event(Event::InternalWorker { + worker: protocol::InternalWorkerRef { + session_id: "child-session".into(), + name: "subworker-hoge".into(), + parent_session_id: Some("parent-session".into()), + kind: protocol::InternalWorkerKind::SubWorker, + }, + revision: 1, + event: Box::new(Event::Status { + status: WorkerStatus::Running, + }), + }); + + let _ = handle_key(&mut app, key(KeyCode::Tab)); + + assert_eq!(app.selected_worker_view().worker_name, "agent"); + assert_eq!(input_text(&app), "@src/main.rs"); + } + #[test] fn command_completion_does_not_affect_normal_composer_without_popup() { let mut app = App::new("agent".to_string()); diff --git a/crates/tui/src/ui.rs b/crates/tui/src/ui.rs index 5f0ac086..63706fe2 100644 --- a/crates/tui/src/ui.rs +++ b/crates/tui/src/ui.rs @@ -27,7 +27,9 @@ use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; use protocol::{AlertLevel, CompletionEntry, Greeting, Segment, WorkerEvent}; -use crate::app::{ActionbarNoticeLevel, App, CompletionState, alert_source_label, fmt_tokens}; +use crate::app::{ + ActionbarNoticeLevel, App, CompletionState, WorkerViewTab, alert_source_label, fmt_tokens, +}; use crate::block::{Block, CompactEvent, ThinkingBlock, ThinkingState}; use crate::command::CommandCandidate; use crate::task::{TaskCounts, TaskEntry, TaskStatus, TaskStore}; @@ -52,7 +54,9 @@ pub fn draw(frame: &mut Frame, app: &mut App) { app.input .apply_cursor_viewport(&mut input_render, input_height); } - let mini_view_h = task_mini_view_height(&app.task_store); + let tabs = app.worker_view_tabs(); + let show_tabs = tabs.len() > 1; + let mini_view_h = task_mini_view_height(&app.selected_worker_view().task_store, show_tabs); // One blank row separates the history tail from the mini-view so // the latest message doesn't visually crash into the task summary. // Folds away with the mini-view when there are no tasks. @@ -69,11 +73,26 @@ pub fn draw(frame: &mut Frame, app: &mut App) { ]) .split(area); - draw_history(frame, app, chunks[0]); + let selected_index = app.selected_internal_worker_index(); + if let Some(index) = selected_index { + let task_pane_open = app.task_pane_open; + let view = app.internal_workers[index].app.as_mut(); + view.task_pane_open = task_pane_open; + draw_history(frame, view, chunks[0]); + } else { + draw_history(frame, app, chunks[0]); + } if mini_view_h > 0 { - draw_task_mini_view(frame, &app.task_store, chunks[2]); + draw_task_mini_view( + frame, + &app.selected_worker_view().task_store, + &tabs, + chunks[2], + ); } draw_separator(frame, chunks[3]); + // Status/composer/control surfaces remain parent-owned. View selection changes + // only transcript/task presentation and never implies SubWorker control. draw_status(frame, app, chunks[4]); draw_input(frame, app, &input_render, chunks[5]); draw_actionbar(frame, app, chunks[6]); @@ -89,19 +108,19 @@ pub fn draw(frame: &mut Frame, app: &mut App) { /// the summary. const MINI_VIEW_MAX_ACTIVE: usize = 3; -/// Height the mini-view section occupies. Returns 0 when there are no -/// tasks at all, so the section collapses cleanly into surrounding -/// layout — there's no point reserving rows for an empty store. -fn task_mini_view_height(store: &TaskStore) -> u16 { - if store.is_empty() { +/// Height the mini-view section occupies. Returns 0 only when there are +/// neither tasks nor Worker-view tabs, so SubWorker selection remains +/// available even when the selected task store is empty. +fn task_mini_view_height(store: &TaskStore, show_tabs: bool) -> u16 { + if store.is_empty() && !show_tabs { return 0; } let active_shown = store.counts().active().min(MINI_VIEW_MAX_ACTIVE); - // active rows + 1 summary line + // active rows + 1 summary/tab line (active_shown as u16).saturating_add(1) } -fn draw_task_mini_view(frame: &mut Frame, store: &TaskStore, area: Rect) { +fn draw_task_mini_view(frame: &mut Frame, store: &TaskStore, tabs: &[WorkerViewTab], area: Rect) { if area.height == 0 || area.width == 0 { return; } @@ -123,7 +142,7 @@ fn draw_task_mini_view(frame: &mut Frame, store: &TaskStore, area: Rect) { lines.push(mini_view_active_line(entry, inner.width)); shown += 1; } - lines.push(mini_view_summary_line(store.counts(), inner.width)); + lines.push(mini_view_summary_line(store.counts(), tabs, inner.width)); Paragraph::new(lines) .block(outer_block) @@ -146,8 +165,8 @@ fn mini_view_active_line(entry: &TaskEntry, width: u16) -> Line<'static> { ]) } -fn mini_view_summary_line(counts: TaskCounts, width: u16) -> Line<'static> { - let text = format!( +fn mini_view_summary_line(counts: TaskCounts, tabs: &[WorkerViewTab], width: u16) -> Line<'static> { + let summary = format!( "{} task(s) — pending: {}, inprogress: {}, completed: {}, deleted: {}", counts.total(), counts.pending, @@ -155,8 +174,79 @@ fn mini_view_summary_line(counts: TaskCounts, width: u16) -> Line<'static> { counts.completed, counts.deleted, ); - let shown = truncate_with_ellipsis(&text, width as usize); - Line::from(Span::styled(shown, Style::default().fg(Color::DarkGray))) + if tabs.len() <= 1 { + let shown = truncate_with_ellipsis(&summary, width as usize); + return Line::from(Span::styled(shown, Style::default().fg(Color::DarkGray))); + } + + let tabs_width = worker_view_tabs_width(tabs); + let width = width as usize; + if tabs_width >= width { + let selected = tabs.iter().find(|tab| tab.selected).unwrap_or(&tabs[0]); + if width <= 4 { + return Line::from(Span::styled( + truncate_with_ellipsis(&selected.label, width), + worker_view_selected_tab_style(), + )); + } + let shown = truncate_with_ellipsis(&selected.label, width.saturating_sub(4)); + let selected_width = UnicodeWidthStr::width(shown.as_str()); + return Line::from(vec![ + Span::raw(" ".repeat(width.saturating_sub(selected_width.saturating_add(4)))), + Span::styled("[ ", Style::default().fg(Color::DarkGray)), + Span::styled(shown, worker_view_selected_tab_style()), + Span::styled(" ]", Style::default().fg(Color::DarkGray)), + ]); + } + + let summary_budget = width.saturating_sub(tabs_width + 1); + let shown = truncate_with_ellipsis(&summary, summary_budget); + let shown_width = UnicodeWidthStr::width(shown.as_str()); + let padding = width.saturating_sub(shown_width + tabs_width); + let mut spans = vec![ + Span::styled(shown, Style::default().fg(Color::DarkGray)), + Span::raw(" ".repeat(padding)), + ]; + spans.extend(worker_view_tab_spans(tabs)); + Line::from(spans) +} + +fn worker_view_tabs_text(tabs: &[WorkerViewTab]) -> String { + format!( + "[ {} ]", + tabs.iter() + .map(|tab| tab.label.as_str()) + .collect::>() + .join(" | ") + ) +} + +fn worker_view_tabs_width(tabs: &[WorkerViewTab]) -> usize { + UnicodeWidthStr::width(worker_view_tabs_text(tabs).as_str()) +} + +fn worker_view_selected_tab_style() -> Style { + Style::default() + .fg(Color::Cyan) + .add_modifier(Modifier::BOLD) +} + +fn worker_view_tab_spans(tabs: &[WorkerViewTab]) -> Vec> { + let dim = Style::default().fg(Color::DarkGray); + let selected = worker_view_selected_tab_style(); + let mut spans = Vec::with_capacity(tabs.len().saturating_mul(2).saturating_add(1)); + spans.push(Span::styled("[ ", dim)); + for (index, tab) in tabs.iter().enumerate() { + if index > 0 { + spans.push(Span::styled(" | ", dim)); + } + spans.push(Span::styled( + tab.label.clone(), + if tab.selected { selected } else { dim }, + )); + } + spans.push(Span::styled(" ]", dim)); + spans } /// Two-character status marker + the style to render it with. Mirrors @@ -387,28 +477,6 @@ pub fn compute_history(app: &App, width: u16) -> HistoryLayout { i += 1; } - for internal in &app.internal_workers { - logical.push((Line::from(""), false)); - logical.push(( - Line::from(vec![ - Span::styled("SubWorker ", Style::default().bold()), - Span::raw(internal.worker.name.clone()), - Span::styled( - format!(" {:?}", internal.app.worker_status), - Style::default().fg(Color::DarkGray), - ), - ]), - false, - )); - let child_width = width.saturating_sub(2).max(1); - let child_history = compute_history(&internal.app, child_width); - logical.extend(child_history.rows.into_iter().map(|row| { - let mut spans = vec![Span::raw(" ")]; - spans.extend(row.line.spans); - (Line::from(spans), row.selectable) - })); - } - // Step 2: pre-wrap every logical line to char-based terminal rows so // scroll math is exact. Track the logical → wrapped mapping so // turn-start indices get translated into wrapped-row coordinates. @@ -1985,6 +2053,93 @@ mod tests { use protocol::WorkerStatus; use std::time::{Duration, Instant}; + #[test] + fn task_summary_right_aligns_worker_tabs_and_highlights_selection() { + let tabs = vec![ + WorkerViewTab { + label: "main".into(), + selected: false, + }, + WorkerViewTab { + label: "subworker-hoge".into(), + selected: true, + }, + WorkerViewTab { + label: "subworker-fuga".into(), + selected: false, + }, + ]; + + let line = mini_view_summary_line(TaskCounts::default(), &tabs, 96); + let text = line + .spans + .iter() + .map(|span| span.content.as_ref()) + .collect::(); + + assert_eq!(UnicodeWidthStr::width(text.as_str()), 96); + assert!(text.ends_with("[ main | subworker-hoge | subworker-fuga ]")); + let selected = line + .spans + .iter() + .find(|span| span.content == "subworker-hoge") + .expect("selected tab span"); + assert_eq!(selected.style.fg, Some(Color::Cyan)); + assert!(selected.style.add_modifier.contains(Modifier::BOLD)); + + let narrow = mini_view_summary_line(TaskCounts::default(), &tabs, 20); + let narrow_text = narrow + .spans + .iter() + .map(|span| span.content.as_ref()) + .collect::(); + assert_eq!(UnicodeWidthStr::width(narrow_text.as_str()), 20); + assert!(narrow_text.ends_with("[ subworker-hoge ]")); + } + + #[test] + fn selected_worker_view_history_is_not_appended_to_main_history() { + let mut app = App::new("main".into()); + app.handle_worker_event(protocol::Event::TextDelta { + text: "main transcript".into(), + }); + app.handle_worker_event(protocol::Event::InternalWorker { + worker: protocol::InternalWorkerRef { + session_id: "child-session".into(), + name: "subworker-hoge".into(), + parent_session_id: Some("parent-session".into()), + kind: protocol::InternalWorkerKind::SubWorker, + }, + revision: 1, + event: Box::new(protocol::Event::TextDelta { + text: "child transcript".into(), + }), + }); + + let main = compute_history(&app, 80) + .rows + .into_iter() + .map(|row| row.line.to_string()) + .collect::(); + assert!(main.contains("main transcript")); + assert!(!main.contains("child transcript")); + + app.cycle_worker_view(); + let child = compute_history(app.selected_worker_view(), 80) + .rows + .into_iter() + .map(|row| row.line.to_string()) + .collect::(); + assert!(!child.contains("main transcript")); + assert!(child.contains("child transcript")); + } + + #[test] + fn worker_tabs_keep_mini_view_visible_without_tasks() { + assert_eq!(task_mini_view_height(&TaskStore::new(), false), 0); + assert_eq!(task_mini_view_height(&TaskStore::new(), true), 1); + } + #[test] fn queue_status_text_includes_count_and_preview() { let mut app = App::new("test".into()); diff --git a/crates/worker-runtime/src/runtime.rs b/crates/worker-runtime/src/runtime.rs index 525ea88c..7447f329 100644 --- a/crates/worker-runtime/src/runtime.rs +++ b/crates/worker-runtime/src/runtime.rs @@ -335,6 +335,7 @@ impl Runtime { for (worker_id, worker) in &mut state.workers { if worker.status.is_active() { worker.status = WorkerStatus::Stopped; + worker.internal_workers.clear(); stopped.push(*worker_id); } } @@ -574,6 +575,7 @@ impl Runtime { run_generation: 1, working_directory: None, execution_handle: None, + internal_workers: BTreeMap::new(), }; state.workers.insert(worker_id, record); state.persist_runtime_snapshot()?; @@ -1474,7 +1476,8 @@ impl Runtime { let mut state = self.lock()?; state.ensure_worker_ref(worker_ref)?; let status_changed = state.project_protocol_event_to_status(worker_ref, &payload); - if status_changed { + let activity_changed = state.project_internal_worker_activity(worker_ref, &payload); + if status_changed || activity_changed { state.publish_worker_upsert(worker_ref.worker_id)?; } let event = state.push_worker_observation_event(worker_ref.clone(), payload); @@ -1531,6 +1534,7 @@ impl Runtime { let worker = state.worker_mut(worker_ref)?; worker.status = status; worker.execution_handle = None; + worker.internal_workers.clear(); let status = worker.status; state.publish_worker_upsert(worker_ref.worker_id)?; state.persist_runtime_snapshot()?; @@ -1943,6 +1947,7 @@ impl RuntimeState { run_generation: worker.run_generation, working_directory: worker.working_directory, execution_handle: None, + internal_workers: BTreeMap::new(), }, ); } @@ -2263,6 +2268,10 @@ impl RuntimeState { .copied() .unwrap_or(0), state: subscription_worker_state(worker.status), + has_running_internal_workers: worker + .internal_workers + .values() + .any(|worker| worker.status == protocol::WorkerStatus::Running), workspace_id: worker.workspace_id.clone(), display_name: worker.request.display_name.clone(), profile, @@ -2405,6 +2414,7 @@ impl RuntimeState { let worker = self.worker_mut(worker_ref)?; worker.execution_handle = None; worker.status = WorkerStatus::Stopped; + worker.internal_workers.clear(); self.publish_worker_upsert(worker_ref.worker_id)?; self.persist_runtime_snapshot()?; Ok(()) @@ -2458,7 +2468,134 @@ impl RuntimeState { event } - #[cfg(feature = "ws-server")] + fn internal_worker_snapshot_statuses( + statuses: &mut BTreeMap, + snapshot: &protocol::InternalWorkerSnapshot, + ) { + statuses.insert( + snapshot.worker.session_id.clone(), + InternalWorkerActivity { + status: snapshot.status, + parent_session_id: snapshot.worker.parent_session_id.clone(), + }, + ); + for child in &snapshot.internal_workers { + Self::internal_worker_snapshot_statuses(statuses, child); + } + } + + fn remove_internal_worker_subtree( + statuses: &mut BTreeMap, + root_session_id: &str, + ) { + let mut removed = vec![root_session_id.to_string()]; + while let Some(parent_session_id) = removed.pop() { + let children = statuses + .iter() + .filter_map(|(session_id, worker)| { + (worker.parent_session_id.as_deref() == Some(parent_session_id.as_str())) + .then(|| session_id.clone()) + }) + .collect::>(); + statuses.remove(&parent_session_id); + removed.extend(children); + } + } + + fn project_internal_worker_event( + statuses: &mut BTreeMap, + worker: &protocol::InternalWorkerRef, + event: &protocol::Event, + ) { + match event { + protocol::Event::Snapshot { + status, + internal_workers, + .. + } => { + Self::remove_internal_worker_subtree(statuses, &worker.session_id); + statuses.insert( + worker.session_id.clone(), + InternalWorkerActivity { + status: *status, + parent_session_id: worker.parent_session_id.clone(), + }, + ); + for child in internal_workers { + Self::internal_worker_snapshot_statuses(statuses, child); + } + } + protocol::Event::InternalWorker { + worker: nested_worker, + event, + .. + } => Self::project_internal_worker_event(statuses, nested_worker, event), + protocol::Event::Status { status } => { + statuses.insert( + worker.session_id.clone(), + InternalWorkerActivity { + status: *status, + parent_session_id: worker.parent_session_id.clone(), + }, + ); + } + protocol::Event::RunEnd { result } => { + let status = match result { + protocol::RunResult::Paused => protocol::WorkerStatus::Paused, + protocol::RunResult::Finished + | protocol::RunResult::LimitReached + | protocol::RunResult::RolledBack => protocol::WorkerStatus::Idle, + }; + statuses.insert( + worker.session_id.clone(), + InternalWorkerActivity { + status, + parent_session_id: worker.parent_session_id.clone(), + }, + ); + } + _ => {} + } + } + + fn update_internal_worker_activity( + statuses: &mut BTreeMap, + event: &protocol::Event, + ) -> bool { + let was_running = statuses + .values() + .any(|worker| worker.status == protocol::WorkerStatus::Running); + match event { + protocol::Event::Snapshot { + internal_workers, .. + } => { + statuses.clear(); + for child in internal_workers { + Self::internal_worker_snapshot_statuses(statuses, child); + } + } + protocol::Event::InternalWorker { worker, event, .. } => { + Self::project_internal_worker_event(statuses, worker, event); + } + _ => {} + } + let is_running = statuses + .values() + .any(|worker| worker.status == protocol::WorkerStatus::Running); + was_running != is_running + } + + fn project_internal_worker_activity( + &mut self, + worker_ref: &WorkerRef, + event: &protocol::Event, + ) -> bool { + let Some(worker) = self.workers.get_mut(&worker_ref.worker_id) else { + return false; + }; + Self::update_internal_worker_activity(&mut worker.internal_workers, event) + } + fn project_protocol_event_to_status( &mut self, worker_ref: &WorkerRef, @@ -2501,6 +2638,12 @@ impl RuntimeState { } } +#[derive(Debug, Clone)] +struct InternalWorkerActivity { + status: protocol::WorkerStatus, + parent_session_id: Option, +} + #[derive(Debug)] struct WorkerRecord { worker_ref: WorkerRef, @@ -2511,6 +2654,7 @@ struct WorkerRecord { run_generation: u64, working_directory: Option, execution_handle: Option, + internal_workers: BTreeMap, } impl WorkerRecord { @@ -2733,6 +2877,126 @@ mod tests { use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex}; + fn internal_worker_ref( + session_id: &str, + parent_session_id: Option<&str>, + ) -> protocol::InternalWorkerRef { + protocol::InternalWorkerRef { + session_id: session_id.to_string(), + parent_session_id: parent_session_id.map(str::to_string), + name: session_id.to_string(), + kind: protocol::InternalWorkerKind::SubWorker, + } + } + + fn internal_worker_status_event( + worker: protocol::InternalWorkerRef, + status: protocol::WorkerStatus, + ) -> protocol::Event { + protocol::Event::InternalWorker { + worker, + revision: 1, + event: Box::new(protocol::Event::Status { status }), + } + } + + #[test] + fn internal_worker_activity_tracks_running_children_independently() { + let mut activity = BTreeMap::new(); + assert!(RuntimeState::update_internal_worker_activity( + &mut activity, + &internal_worker_status_event( + internal_worker_ref("child-a", None), + protocol::WorkerStatus::Running, + ), + )); + assert!(!RuntimeState::update_internal_worker_activity( + &mut activity, + &internal_worker_status_event( + internal_worker_ref("child-b", None), + protocol::WorkerStatus::Running, + ), + )); + assert!(!RuntimeState::update_internal_worker_activity( + &mut activity, + &internal_worker_status_event( + internal_worker_ref("child-a", None), + protocol::WorkerStatus::Idle, + ), + )); + assert!(RuntimeState::update_internal_worker_activity( + &mut activity, + &internal_worker_status_event( + internal_worker_ref("child-b", None), + protocol::WorkerStatus::Idle, + ), + )); + } + + #[test] + fn nested_internal_worker_activity_reaches_the_parent_projection() { + let mut activity = BTreeMap::new(); + let direct_child = internal_worker_ref("child", None); + let nested_running = protocol::Event::InternalWorker { + worker: direct_child.clone(), + revision: 1, + event: Box::new(internal_worker_status_event( + internal_worker_ref("grandchild", Some("child")), + protocol::WorkerStatus::Running, + )), + }; + assert!(RuntimeState::update_internal_worker_activity( + &mut activity, + &nested_running, + )); + + let nested_idle = protocol::Event::InternalWorker { + worker: direct_child, + revision: 2, + event: Box::new(internal_worker_status_event( + internal_worker_ref("grandchild", Some("child")), + protocol::WorkerStatus::Idle, + )), + }; + assert!(RuntimeState::update_internal_worker_activity( + &mut activity, + &nested_idle, + )); + } + + #[test] + fn parent_snapshot_replaces_stale_internal_worker_activity() { + let mut activity = BTreeMap::new(); + RuntimeState::update_internal_worker_activity( + &mut activity, + &internal_worker_status_event( + internal_worker_ref("child-a", None), + protocol::WorkerStatus::Running, + ), + ); + let snapshot = protocol::Event::Snapshot { + entries: Vec::new(), + greeting: protocol::Greeting { + worker_name: "parent".to_string(), + cwd: "/tmp".to_string(), + provider: "test".to_string(), + model: "test".to_string(), + scope_summary: String::new(), + tools: Vec::new(), + context_window: 0, + context_tokens: 0, + }, + status: protocol::WorkerStatus::Idle, + in_flight: protocol::InFlightSnapshot::default(), + internal_workers: Vec::new(), + }; + assert!(RuntimeState::update_internal_worker_activity( + &mut activity, + &snapshot, + )); + assert!(activity.is_empty()); + } + #[test] fn runtime_identity_binding_is_immutable_and_host_owned() { let runtime = Runtime::new_memory(); @@ -3086,16 +3350,75 @@ mod tests { payload => panic!("unexpected subscription payload: {payload:?}"), } - runtime.stop_runtime().unwrap(); + runtime + .observe_worker_event( + &created.worker_ref, + internal_worker_status_event( + internal_worker_ref("child-live", None), + protocol::WorkerStatus::Running, + ), + ) + .unwrap(); let update = receive_subscription_update(&mut subscription).unwrap(); assert_eq!(update.subject_revision, 2); match update.payload { SubscriptionEventPayload::WorkerUpserted { worker } => { - assert_eq!(worker.worker_id.as_str(), created.worker_id.to_string()); - assert_eq!(worker.state, SubscriptionWorkerState::Stopped); + assert_eq!(worker.state, SubscriptionWorkerState::Idle); + assert!(worker.has_running_internal_workers); } payload => panic!("unexpected subscription payload: {payload:?}"), } + + runtime + .observe_worker_event( + &created.worker_ref, + internal_worker_status_event( + internal_worker_ref("child-live", None), + protocol::WorkerStatus::Idle, + ), + ) + .unwrap(); + let update = receive_subscription_update(&mut subscription).unwrap(); + assert_eq!(update.subject_revision, 3); + match update.payload { + SubscriptionEventPayload::WorkerUpserted { worker } => { + assert_eq!(worker.state, SubscriptionWorkerState::Idle); + assert!(!worker.has_running_internal_workers); + } + payload => panic!("unexpected subscription payload: {payload:?}"), + } + + runtime + .observe_worker_event( + &created.worker_ref, + internal_worker_status_event( + internal_worker_ref("child-live", None), + protocol::WorkerStatus::Running, + ), + ) + .unwrap(); + let update = receive_subscription_update(&mut subscription).unwrap(); + assert_eq!(update.subject_revision, 4); + match update.payload { + SubscriptionEventPayload::WorkerUpserted { worker } => { + assert_eq!(worker.state, SubscriptionWorkerState::Idle); + assert!(worker.has_running_internal_workers); + } + payload => panic!("unexpected subscription payload: {payload:?}"), + } + + runtime.stop_worker(&created.worker_ref, None).unwrap(); + let update = receive_subscription_update(&mut subscription).unwrap(); + assert_eq!(update.subject_revision, 5); + match update.payload { + SubscriptionEventPayload::WorkerUpserted { worker } => { + assert_eq!(worker.worker_id.as_str(), created.worker_id.to_string()); + assert_eq!(worker.state, SubscriptionWorkerState::Stopped); + assert!(!worker.has_running_internal_workers); + } + payload => panic!("unexpected subscription payload: {payload:?}"), + } + runtime.stop_runtime().unwrap(); } #[test] diff --git a/crates/worker-runtime/src/worker_source.rs b/crates/worker-runtime/src/worker_source.rs index 8acde9f1..fbf6f328 100644 --- a/crates/worker-runtime/src/worker_source.rs +++ b/crates/worker-runtime/src/worker_source.rs @@ -125,7 +125,6 @@ pub trait EmbeddedWorkerMutationDispatcher: Send + Sync { proof: InProcessWorkerMutationProof, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result; } @@ -187,7 +186,6 @@ impl RuntimeWorkerMutationForwarder { &self, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result { let proof = self.authority.issue_worker_remove( @@ -206,7 +204,6 @@ impl RuntimeWorkerMutationForwarder { token, target_runtime_id: target_runtime_id.to_string(), target_worker_id: target_worker_id.to_string(), - expected_worker_revision: expected_worker_revision.to_string(), reason: reason.to_string(), }), ( @@ -216,7 +213,6 @@ impl RuntimeWorkerMutationForwarder { claims, target_runtime_id, target_worker_id, - expected_worker_revision, reason, ), _ => Err(RuntimeWorkerMutationForwardError::AuthorityTransportMismatch), @@ -230,7 +226,6 @@ struct RemoteWorkerRemoveHttpRequest { token: String, target_runtime_id: String, target_worker_id: String, - expected_worker_revision: String, reason: String, } @@ -267,7 +262,6 @@ fn execute_remote_worker_remove_http_blocking( let body = serde_json::json!({ "target_runtime_id": request.target_runtime_id, "target_worker_id": request.target_worker_id, - "expected_worker_revision": request.expected_worker_revision, "reason": request.reason, }); let client = reqwest::blocking::Client::new(); @@ -474,7 +468,6 @@ impl WorkspaceClient for RuntimeOwnedWorkspaceClient { &self, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result { self.worker_remove @@ -484,12 +477,7 @@ impl WorkspaceClient for RuntimeOwnedWorkspaceClient { "Runtime-owned WorkerRemove forwarding is unavailable".to_string(), ) })? - .execute_worker_remove( - target_runtime_id, - target_worker_id, - expected_worker_revision, - reason, - ) + .execute_worker_remove(target_runtime_id, target_worker_id, reason) .map_err(|error| WorkspaceClientError::Request(error.to_string())) } } @@ -915,12 +903,7 @@ mod tests { format!("http://{address}"), ); let response = forwarder - .execute_worker_remove( - "runtime-target", - "worker-target", - "revision-7", - "retire obsolete Worker", - ) + .execute_worker_remove("runtime-target", "worker-target", "retire obsolete Worker") .unwrap(); assert_eq!(response.status, 204); server.join().unwrap(); @@ -929,7 +912,7 @@ mod tests { assert!(request.starts_with("POST /api/w/workspace-a/workers/remove HTTP/1.1")); assert!(request.contains("\"target_runtime_id\":\"runtime-target\"")); assert!(request.contains("\"target_worker_id\":\"worker-target\"")); - assert!(request.contains("\"expected_worker_revision\":\"revision-7\"")); + assert!(!request.contains("expected_worker_revision")); assert!(request.contains("\"reason\":\"retire obsolete Worker\"")); let token = request .lines() @@ -962,7 +945,7 @@ mod tests { #[derive(Default)] struct RecordingDispatcher { - seen: Mutex>, + seen: Mutex>, } impl EmbeddedWorkerMutationDispatcher for RecordingDispatcher { fn execute_worker_remove( @@ -970,14 +953,12 @@ mod tests { proof: InProcessWorkerMutationProof, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result { *self.seen.lock().unwrap() = Some(( proof.into_claims(), target_runtime_id.to_string(), target_worker_id.to_string(), - expected_worker_revision.to_string(), reason.to_string(), )); Ok(WorkspaceResponse { @@ -996,15 +977,10 @@ mod tests { dispatcher.clone(), ); let response = forwarder - .execute_worker_remove( - "runtime-target", - "worker-target", - "revision-7", - "retire obsolete Worker", - ) + .execute_worker_remove("runtime-target", "worker-target", "retire obsolete Worker") .unwrap(); assert_eq!(response.status, 202); - let (claims, target_runtime_id, target_worker_id, expected_revision, reason) = + let (claims, target_runtime_id, target_worker_id, reason) = dispatcher.seen.lock().unwrap().take().unwrap(); assert_eq!(claims.iss, "runtime-embedded"); assert_eq!(claims.worker_id, "worker-source"); @@ -1012,7 +988,6 @@ mod tests { assert_eq!(claims.target_worker_id, "worker-target"); assert_eq!(target_runtime_id, "runtime-target"); assert_eq!(target_worker_id, "worker-target"); - assert_eq!(expected_revision, "revision-7"); assert_eq!(reason, "retire obsolete Worker"); } diff --git a/crates/worker/src/feature/builtin/manage_worker.rs b/crates/worker/src/feature/builtin/manage_worker.rs index c48383a7..e6f069d9 100644 --- a/crates/worker/src/feature/builtin/manage_worker.rs +++ b/crates/worker/src/feature/builtin/manage_worker.rs @@ -48,7 +48,6 @@ pub trait WorkerControlService: Send + Sync { &self, runtime_id: &str, worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result; async fn execute_runtime( @@ -170,11 +169,10 @@ impl WorkerControlService for WorkspaceWorkerControlService { &self, runtime_id: &str, worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result { self.client - .execute_worker_remove(runtime_id, worker_id, expected_worker_revision, reason) + .execute_worker_remove(runtime_id, worker_id, reason) } async fn execute_runtime( @@ -535,7 +533,6 @@ struct WorkerStopInput { #[serde(deny_unknown_fields)] struct WorkerRemoveInput { subject: WorkerSubjectInput, - expected_worker_revision: String, reason: String, } @@ -597,7 +594,7 @@ impl WorkerOperation { "Restore a stopped Backend/Runtime Worker session in the current Workspace." } Self::Remove => { - "Remove an eligible stopped, unassigned, non-internal Worker. Supply the current Worker revision and a bounded reason; Backend validation and retention are authoritative." + "Remove an eligible stopped, unassigned, non-internal Worker. Supply a bounded reason; Backend validation and retention are authoritative." } } } @@ -747,8 +744,6 @@ impl Tool for WorkspaceWorkerTool { WorkerOperation::Remove => { let input = parse::(input_json, "WorkerRemove")?; let (runtime_id, worker_id) = runtime_subject_ids(&input.subject, self.operation)?; - let expected_worker_revision = - non_empty(input.expected_worker_revision, "expected_worker_revision")?; let reason = non_empty(input.reason, "reason")?; if reason.len() > 512 { return Err(ToolError::ExecutionFailed( @@ -756,12 +751,7 @@ impl Tool for WorkspaceWorkerTool { )); } self.control - .remove_runtime_worker( - &runtime_id, - &worker_id, - &expected_worker_revision, - &reason, - ) + .remove_runtime_worker(&runtime_id, &worker_id, &reason) .map_err(control_tool_error)? } }; @@ -957,7 +947,7 @@ mod tests { #[derive(Debug, Default)] struct RecordingWorkspaceClient { requests: Mutex>, - removals: Mutex>, + removals: Mutex>, } impl WorkspaceClient for RecordingWorkspaceClient { @@ -988,13 +978,11 @@ mod tests { &self, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result { self.removals.lock().unwrap().push(( target_runtime_id.to_string(), target_worker_id.to_string(), - expected_worker_revision.to_string(), reason.to_string(), )); Ok(WorkspaceResponse { @@ -1205,7 +1193,7 @@ mod tests { } #[tokio::test] - async fn worker_remove_forwards_only_target_revision_and_bounded_reason() { + async fn worker_remove_forwards_only_target_and_bounded_reason() { let client = Arc::new(RecordingWorkspaceClient::default()); let tool = WorkspaceWorkerTool { operation: WorkerOperation::Remove, @@ -1218,7 +1206,6 @@ mod tests { "runtime_id": "runtime-1", "worker_id": "worker-7", }, - "expected_worker_revision": "2026-08-11T20:00:00Z", "reason": " retire completed Worker " }) .to_string(), @@ -1231,7 +1218,6 @@ mod tests { [( "runtime-1".to_string(), "worker-7".to_string(), - "2026-08-11T20:00:00Z".to_string(), "retire completed Worker".to_string(), )] ); @@ -1239,15 +1225,18 @@ mod tests { let schema = serde_json::to_value(schemars::schema_for!(WorkerRemoveInput)) .unwrap() .to_string(); - for field in [ - "runtime_id", - "worker_id", - "expected_worker_revision", - "reason", - ] { + for field in ["runtime_id", "worker_id", "reason"] { assert!(schema.contains(field)); } - for forbidden in ["proof", "actor", "workspace_id", "policy", "plan", "stage"] { + for forbidden in [ + "expected_worker_revision", + "proof", + "actor", + "workspace_id", + "policy", + "plan", + "stage", + ] { assert!(!schema.contains(forbidden), "schema leaked {forbidden}"); } } @@ -1268,7 +1257,6 @@ mod tests { "runtime_id": "runtime-1", "worker_id": "worker-7", }, - "expected_worker_revision": "revision-1", "reason": reason, }) .to_string(), diff --git a/crates/worker/src/worker.rs b/crates/worker/src/worker.rs index dd4ec9f2..18c81401 100644 --- a/crates/worker/src/worker.rs +++ b/crates/worker/src/worker.rs @@ -272,7 +272,6 @@ pub trait WorkspaceClient: std::fmt::Debug + Send + Sync { &self, _target_runtime_id: &str, _target_worker_id: &str, - _expected_worker_revision: &str, _reason: &str, ) -> Result { Err(WorkspaceClientError::Unavailable( diff --git a/crates/workspace-server/src/retention.rs b/crates/workspace-server/src/retention.rs index 58d2baf8..f75a3aa4 100644 --- a/crates/workspace-server/src/retention.rs +++ b/crates/workspace-server/src/retention.rs @@ -59,7 +59,6 @@ pub struct WorkerRetentionPolicyUpdate { pub struct WorkerRemovalPlanRequest { pub workspace_id: String, pub worker: RuntimeWorkerRef, - pub expected_worker_revision: String, pub reason: String, } @@ -154,8 +153,6 @@ pub enum WorkerRetentionError { WorkerNotFound, #[error("Worker belongs to a different Workspace")] CrossWorkspace, - #[error("Worker revision changed: expected {expected}, current {actual}")] - WorkerRevisionConflict { expected: String, actual: String }, #[error("Worker removal is blocked: {0:?}")] Blocked(Vec), #[error("Worker removal plan {plan_id} is stale: {reason}")] @@ -308,17 +305,16 @@ impl SqliteWorkspaceStore { return Err(StoreError::InvalidInput(if other{"cross-workspace".into()}else{"worker-missing".into()})); } }; - if worker.updated_at!=req.expected_worker_revision { return Err(StoreError::InvalidInput(format!("worker-conflict:{}:{}",req.expected_worker_revision,worker.updated_at))); } let mut blockers=Vec::new(); if worker.retention_state=="pinned" { blockers.push(WorkerRemovalBlocker::Hold); } if let Some((assignment_id,ticket_id))=tx.query_row("SELECT a.assignment_id,a.ticket_id FROM ticket_current_worker_assignments c JOIN ticket_worker_assignments a ON a.workspace_id=c.workspace_id AND a.ticket_id=c.ticket_id AND a.assignment_id=c.assignment_id WHERE a.workspace_id=?1 AND a.runtime_id=?2 AND a.worker_id=?3",params![req.workspace_id,req.worker.runtime_id,req.worker.worker_id],|r|Ok((r.get(0)?,r.get(1)?))).optional()? { blockers.push(WorkerRemovalBlocker::CurrentAssignment{assignment_id,ticket_id}); } - let fp=fingerprint(req,inv,&policy,&blockers)?; + let fp=fingerprint(req,&worker.updated_at,inv,&policy,&blockers)?; let plan_id=stable("wrp",&fp); let operation_id=stable("wro",&fp); let archive_id=(policy.session_disposition==SessionDisposition::Archive).then(||stable("wra",&fp)); let state=if blockers.is_empty(){WorkerRemovalPlanState::Planned}else{WorkerRemovalPlanState::Blocked}; - tx.execute("INSERT OR IGNORE INTO worker_removal_operations(operation_id,plan_id,input_fingerprint,workspace_id,runtime_id,worker_id,worker_revision,run_generation,policy_id,policy_revision,session_disposition,metadata_disposition,archive_retention_kind,archive_retention_seconds,diagnostics_disposition,diagnostics_retention_seconds,archive_id,blockers_json,state,reason,created_at,updated_at) VALUES(?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,?14,?15,?16,?17,?18,?19,?20,?21,?21)",params![operation_id,plan_id,fp,req.workspace_id,req.worker.runtime_id,req.worker.worker_id,req.expected_worker_revision,inv.run_generation,policy.policy_id,policy.revision,sess(policy.session_disposition),meta(policy.metadata_disposition),archive_kind(policy.archive_retention),archive_seconds(policy.archive_retention),diag(policy.diagnostics_disposition),policy.diagnostics_retention_seconds,archive_id,serde_json::to_string(&blockers).map_err(|e|StoreError::InvalidInput(e.to_string()))?,state_s(state),req.reason,now])?; + tx.execute("INSERT OR IGNORE INTO worker_removal_operations(operation_id,plan_id,input_fingerprint,workspace_id,runtime_id,worker_id,worker_revision,run_generation,policy_id,policy_revision,session_disposition,metadata_disposition,archive_retention_kind,archive_retention_seconds,diagnostics_disposition,diagnostics_retention_seconds,archive_id,blockers_json,state,reason,created_at,updated_at) VALUES(?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13,?14,?15,?16,?17,?18,?19,?20,?21,?21)",params![operation_id,plan_id,fp,req.workspace_id,req.worker.runtime_id,req.worker.worker_id,worker.updated_at,inv.run_generation,policy.policy_id,policy.revision,sess(policy.session_disposition),meta(policy.metadata_disposition),archive_kind(policy.archive_retention),archive_seconds(policy.archive_retention),diag(policy.diagnostics_disposition),policy.diagnostics_retention_seconds,archive_id,serde_json::to_string(&blockers).map_err(|e|StoreError::InvalidInput(e.to_string()))?,state_s(state),req.reason,now])?; let plan=load_plan(&tx,&plan_id)?.ok_or_else(||StoreError::InvalidInput("plan missing".into()))?; if plan.input_fingerprint!=fp{return Err(StoreError::InvalidInput(format!("fingerprint:{}",plan.operation_id)));} tx.commit()?; Ok(plan) @@ -422,27 +418,22 @@ impl SqliteWorkspaceStore { &self, workspace_id: &str, worker: &RuntimeWorkerRef, - expected_worker_revision: &str, - reason: &str, ) -> Result, WorkerRetentionError> { bounded("workspace", workspace_id, 160)?; - bounded("revision", expected_worker_revision, 256)?; - bounded("reason", reason, 512)?; let plan = self.with_conn(|conn| { conn.query_row( "SELECT plan_id FROM worker_removal_operations WHERE workspace_id=?1 AND runtime_id=?2 AND worker_id=?3 - AND worker_revision=?4 AND reason=?5 - AND state IN ('executing','failed','succeeded') + AND state IN ('planned','executing','failed','succeeded') + AND ( + state='succeeded' OR worker_revision=( + SELECT updated_at FROM worker_registry + WHERE workspace_id=?1 AND runtime_id=?2 AND worker_id=?3 + ) + ) ORDER BY CASE state WHEN 'succeeded' THEN 0 ELSE 1 END, created_at DESC LIMIT 1", - params![ - workspace_id, - worker.runtime_id, - worker.worker_id, - expected_worker_revision, - reason, - ], + params![workspace_id, worker.runtime_id, worker.worker_id], |row| row.get::<_, String>(0), ) .optional() @@ -848,6 +839,7 @@ fn stale_error(plan: &WorkerRemovalPlan, reason: &str) -> StoreError { } fn fingerprint( r: &WorkerRemovalPlanRequest, + worker_revision: &str, i: &WorkerRetentionInventory, p: &WorkerRetentionPolicy, b: &[WorkerRemovalBlocker], @@ -856,7 +848,7 @@ fn fingerprint( r.workspace_id, r.worker.runtime_id, r.worker.worker_id, - r.expected_worker_revision, + worker_revision, i.run_generation, i.session_id, i.segment_ids, @@ -887,7 +879,6 @@ fn validate_plan( i: &WorkerRetentionInventory, ) -> Result<(), WorkerRetentionError> { bounded("workspace", &r.workspace_id, 160)?; - bounded("revision", &r.expected_worker_revision, 256)?; bounded("reason", &r.reason, 2000)?; if i.workspace_id != r.workspace_id || i.runtime_id != r.worker.runtime_id @@ -947,13 +938,6 @@ fn map_error(e: StoreError) -> WorkerRetentionError { if m == "worker-missing" { return WorkerRetentionError::WorkerNotFound; } - if let Some(x) = m.strip_prefix("worker-conflict:") { - let mut s = x.splitn(2, ':'); - return WorkerRetentionError::WorkerRevisionConflict { - expected: s.next().unwrap_or_default().into(), - actual: s.next().unwrap_or_default().into(), - }; - } if let Some(x) = m.strip_prefix("fingerprint:") { return WorkerRetentionError::OperationFingerprintConflict { operation_id: x.into(), @@ -1111,7 +1095,6 @@ mod tests { runtime_id: "r".into(), worker_id: worker_id().to_string(), }, - expected_worker_revision: "rev1".into(), reason: "cleanup".into(), } } @@ -1146,6 +1129,7 @@ mod tests { let a = s.plan_worker_removal(&req(), &inv()).unwrap(); let b = s.plan_worker_removal(&req(), &inv()).unwrap(); assert_eq!(a.plan_id, b.plan_id); + assert_eq!(a.worker_revision, "rev1"); s.with_conn(|c| { c.execute( "UPDATE worker_registry SET retention_state='pinned' WHERE workspace_id='w'", @@ -1154,9 +1138,7 @@ mod tests { Ok(()) }) .unwrap(); - let mut q = req(); - q.expected_worker_revision = "rev1".into(); - let p = s.plan_worker_removal(&q, &inv()).unwrap(); + let p = s.plan_worker_removal(&req(), &inv()).unwrap(); assert_eq!(p.blockers, vec![WorkerRemovalBlocker::Hold]); assert!(matches!( s.begin_worker_removal("w", &p.plan_id, &p.input_fingerprint), @@ -1580,6 +1562,77 @@ mod tests { ); } + #[test] + fn worker_removal_recovery_is_target_keyed_and_preserves_original_reason() { + let store = setup(); + let request = req(); + let plan = store.plan_worker_removal(&request, &inv()).unwrap(); + let recovered_planned = store + .recover_worker_removal_execution("w", &request.worker) + .unwrap() + .unwrap(); + assert_eq!( + recovered_planned.plan.state, + WorkerRemovalPlanState::Planned + ); + assert_eq!(recovered_planned.plan.plan_id, plan.plan_id); + store + .prepare_worker_removal_execution("w", &plan.plan_id, &plan.input_fingerprint) + .unwrap(); + store + .fail_worker_removal( + "w", + &plan.operation_id, + &plan.input_fingerprint, + "runtime_remove_failed", + ) + .unwrap(); + + let recovered = store + .recover_worker_removal_execution("w", &request.worker) + .unwrap() + .unwrap(); + assert_eq!(recovered.plan.plan_id, plan.plan_id); + assert_eq!(recovered.plan.reason, request.reason); + } + + #[test] + fn stale_failed_removal_is_not_recovered_after_worker_authority_changes() { + let store = setup(); + let request = req(); + let plan = store.plan_worker_removal(&request, &inv()).unwrap(); + store + .prepare_worker_removal_execution("w", &plan.plan_id, &plan.input_fingerprint) + .unwrap(); + store + .fail_worker_removal( + "w", + &plan.operation_id, + &plan.input_fingerprint, + "runtime_remove_failed", + ) + .unwrap(); + store + .with_conn(|conn| { + conn.execute( + "UPDATE worker_registry SET updated_at='rev2' WHERE workspace_id='w' AND runtime_id='r' AND worker_id=?1", + [worker_id().to_string()], + )?; + Ok(()) + }) + .unwrap(); + + assert!( + store + .recover_worker_removal_execution("w", &request.worker) + .unwrap() + .is_none() + ); + let replacement = store.plan_worker_removal(&request, &inv()).unwrap(); + assert_eq!(replacement.worker_revision, "rev2"); + assert_ne!(replacement.plan_id, plan.plan_id); + } + #[test] fn succeeded_worker_removal_recovers_after_registry_purge() { let s = setup(); @@ -1632,18 +1685,13 @@ mod tests { .is_none() ); let recovered = s - .recover_worker_removal_execution( - "w", - &request.worker, - &request.expected_worker_revision, - &request.reason, - ) + .recover_worker_removal_execution("w", &request.worker) .unwrap() .unwrap(); assert_eq!(recovered.plan.state, WorkerRemovalPlanState::Succeeded); assert_eq!( recovered.runtime_request.expected_worker_revision, - request.expected_worker_revision + plan.worker_revision ); } @@ -1662,12 +1710,7 @@ mod tests { ) .unwrap(); let recovered = s - .recover_worker_removal_execution( - "w", - &request.worker, - &request.expected_worker_revision, - &request.reason, - ) + .recover_worker_removal_execution("w", &request.worker) .unwrap() .unwrap(); assert_eq!( diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 3a20f09d..183c7f56 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -411,7 +411,6 @@ impl WorkspaceWorkerRemoveExecutor { source: crate::worker_source::VerifiedWorkerMutationSource, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> std::result::Result { let reason = reason.trim(); @@ -509,73 +508,63 @@ impl WorkspaceWorkerRemoveExecutor { let prepared = self .store - .recover_worker_removal_execution( - &self.workspace_id, - &target, - expected_worker_revision, - reason, - ) + .recover_worker_removal_execution(&self.workspace_id, &target) .map_err(|_| "Worker removal recovery authority is unavailable".to_string())?; if let Some(prepared) = prepared { if prepared.plan.state == crate::retention::WorkerRemovalPlanState::Succeeded { return Ok(worker_remove_success_response(&target)); } - let must_close_session = - prepared.prior_failure_category.as_deref() == Some("workdir_session_close_failed"); - let must_release_attachment = must_close_session - || prepared.prior_failure_category.as_deref() - == Some("workdir_attachment_release_failed"); - let prepared = - if prepared.plan.state == crate::retention::WorkerRemovalPlanState::Failed { - match self.store.prepare_worker_removal_execution( - &self.workspace_id, - &prepared.plan.plan_id, - &prepared.plan.input_fingerprint, - ) { - Ok(prepared) => prepared, - Err(error) => return Ok(worker_retention_error_response(error)), - } - } else { - prepared - }; - if must_close_session { - let session = { - self.workdir_sessions - .lock() - .map_err(|_| "Workdir session registry was poisoned".to_string())? - .get(&target) - .cloned() - }; - if let Some(session) = session { - if session.close().await.is_err() { - let _ = self.store.fail_worker_removal( - &self.workspace_id, - &prepared.plan.operation_id, - &prepared.plan.input_fingerprint, - "workdir_session_close_failed", - ); - return Ok(worker_remove_error_response( - StatusCode::SERVICE_UNAVAILABLE, - "attachment_close_failed", - "Worker Workdir session could not be closed; removal can be retried", - )); - } - self.workdir_sessions - .lock() - .map_err(|_| "Workdir session registry was poisoned".to_string())? - .remove(&target); + let prepared = if matches!( + prepared.plan.state, + crate::retention::WorkerRemovalPlanState::Planned + | crate::retention::WorkerRemovalPlanState::Failed + ) { + match self.store.prepare_worker_removal_execution( + &self.workspace_id, + &prepared.plan.plan_id, + &prepared.plan.input_fingerprint, + ) { + Ok(prepared) => prepared, + Err(error) => return Ok(worker_retention_error_response(error)), } - } - if must_release_attachment - && self - .store - .detach_worker_workdir( + } else { + prepared + }; + let session = { + self.workdir_sessions + .lock() + .map_err(|_| "Workdir session registry was poisoned".to_string())? + .get(&target) + .cloned() + }; + if let Some(session) = session { + if session.close().await.is_err() { + let _ = self.store.fail_worker_removal( &self.workspace_id, - &target, - None, - &Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), - ) - .is_err() + &prepared.plan.operation_id, + &prepared.plan.input_fingerprint, + "workdir_session_close_failed", + ); + return Ok(worker_remove_error_response( + StatusCode::SERVICE_UNAVAILABLE, + "attachment_close_failed", + "Worker Workdir session could not be closed; removal can be retried", + )); + } + self.workdir_sessions + .lock() + .map_err(|_| "Workdir session registry was poisoned".to_string())? + .remove(&target); + } + if self + .store + .detach_worker_workdir( + &self.workspace_id, + &target, + None, + &Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), + ) + .is_err() { let _ = self.store.fail_worker_removal( &self.workspace_id, @@ -632,7 +621,6 @@ impl WorkspaceWorkerRemoveExecutor { let request = crate::retention::WorkerRemovalPlanRequest { workspace_id: self.workspace_id.clone(), worker: target.clone(), - expected_worker_revision: expected_worker_revision.to_string(), reason: reason.to_string(), }; let plan = match self.store.plan_worker_removal(&request, &inventory) { @@ -737,13 +725,11 @@ impl crate::worker_source::VerifiedWorkerRemoveExecutor for WorkspaceWorkerRemov source: crate::worker_source::VerifiedWorkerMutationSource, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> std::result::Result { let executor = self.clone(); let target_runtime_id = target_runtime_id.to_string(); let target_worker_id = target_worker_id.to_string(); - let expected_worker_revision = expected_worker_revision.to_string(); let reason = reason.to_string(); std::thread::spawn(move || { tokio::runtime::Builder::new_current_thread() @@ -754,7 +740,6 @@ impl crate::worker_source::VerifiedWorkerRemoveExecutor for WorkspaceWorkerRemov source, &target_runtime_id, &target_worker_id, - &expected_worker_revision, &reason, )) }) @@ -6476,14 +6461,13 @@ fn worker_retention_error_response( "worker_not_found", "Worker was not found in this Workspace", ), - crate::retention::WorkerRetentionError::WorkerRevisionConflict { .. } - | crate::retention::WorkerRetentionError::PolicyRevisionConflict { .. } + crate::retention::WorkerRetentionError::PolicyRevisionConflict { .. } | crate::retention::WorkerRetentionError::StalePlan { .. } | crate::retention::WorkerRetentionError::OperationFingerprintConflict { .. } => { worker_remove_error_response( StatusCode::CONFLICT, - "worker_revision_conflict", - "Worker removal state changed; reread the Worker and retry", + "worker_removal_conflict", + "Worker removal state changed; retry the operation", ) } crate::retention::WorkerRetentionError::Blocked(_) => worker_remove_error_response( @@ -6510,7 +6494,6 @@ fn worker_retention_error_response( struct WorkerRemoveBoundaryRequest { target_runtime_id: String, target_worker_id: String, - expected_worker_revision: String, reason: String, } @@ -6548,7 +6531,6 @@ async fn scoped_worker_remove_source_boundary( source, &request.target_runtime_id, &request.target_worker_id, - &request.expected_worker_revision, &request.reason, ) .await @@ -16357,7 +16339,7 @@ mod tests { let temp = tempfile::tempdir().unwrap(); let app = build_router(test_api(temp.path()).await); - let body = r#"{"target_runtime_id":"runtime-target","target_worker_id":"target-worker","expected_worker_revision":"revision-1","reason":"retire target Worker"}"#; + let body = r#"{"target_runtime_id":"runtime-target","target_worker_id":"target-worker","reason":"retire target Worker"}"#; let browser = app .clone() .oneshot( @@ -16474,7 +16456,6 @@ mod tests { fresh_proof, "runtime-target", "target-worker", - "revision-1", "retire target Worker", ) .unwrap_err(); @@ -16482,7 +16463,7 @@ mod tests { } #[tokio::test] - async fn worker_remove_rejects_self_running_and_stale_revision_at_caller_boundary() { + async fn worker_remove_rejects_self_and_running_at_caller_boundary() { let temp = tempfile::tempdir().unwrap(); let api = test_api(temp.path()).await; let Json(orchestrator) = scoped_start_workspace_orchestrator( @@ -16507,7 +16488,6 @@ mod tests { verified_source(), &source.runtime_id, &source.worker_id, - "irrelevant", "must reject self", ) .await @@ -16549,37 +16529,12 @@ mod tests { verified_source(), &target.runtime_id, &target.worker_id, - "irrelevant", "must reject a live Worker", ) .await .unwrap(); assert_eq!(running_response.status, StatusCode::CONFLICT.as_u16()); assert!(running_response.body.contains("worker_not_stopped")); - - api.runtime - .stop_worker( - &target, - WorkerLifecycleRequest { - reason: Some("prepare stale revision guard".to_string()), - ticket_assignment: None, - }, - ) - .unwrap(); - let summary = api.runtime.worker(&target).unwrap(); - let record = sync_worker_observation(&api, &summary).unwrap(); - let stale_response = executor - .execute_async( - verified_source(), - &target.runtime_id, - &target.worker_id, - &format!("{}-stale", record.updated_at), - "must reject stale revision", - ) - .await - .unwrap(); - assert_eq!(stale_response.status, StatusCode::CONFLICT.as_u16()); - assert!(stale_response.body.contains("worker_revision_conflict")); } #[tokio::test] @@ -16653,7 +16608,7 @@ mod tests { ) .unwrap(); let summary = api.runtime.worker(&target).unwrap(); - let record = sync_worker_observation(&api, &summary).unwrap(); + sync_worker_observation(&api, &summary).unwrap(); seed_worker_control_grant(&api, &source, &target, "embedded-valid-proof"); let response = WorkspaceWorkerRemoveExecutor::new(&api) @@ -16667,7 +16622,6 @@ mod tests { }, &target.runtime_id, &target.worker_id, - &record.updated_at, "retire completed Worker", ) .await @@ -16878,7 +16832,7 @@ mod tests { route_token, ) .body(Body::from( - r#"{"target_runtime_id":"runtime-target","target_worker_id":"target-worker","expected_worker_revision":"revision-1","reason":"retire target Worker"}"#, + r#"{"target_runtime_id":"runtime-target","target_worker_id":"target-worker","reason":"retire target Worker"}"#, )) .unwrap(), ) diff --git a/crates/workspace-server/src/store.rs b/crates/workspace-server/src/store.rs index b9447295..a3cbc1fe 100644 --- a/crates/workspace-server/src/store.rs +++ b/crates/workspace-server/src/store.rs @@ -653,13 +653,11 @@ pub trait ControlPlaneStore: Send + Sync { &self, workspace_id: &str, worker: &RuntimeWorkerRef, - expected_worker_revision: &str, - reason: &str, ) -> std::result::Result< Option, crate::retention::WorkerRetentionError, > { - let _ = (workspace_id, worker, expected_worker_revision, reason); + let _ = (workspace_id, worker); Ok(None) } fn fail_worker_removal( @@ -1650,19 +1648,11 @@ impl ControlPlaneStore for SqliteWorkspaceStore { &self, workspace_id: &str, worker: &RuntimeWorkerRef, - expected_worker_revision: &str, - reason: &str, ) -> std::result::Result< Option, crate::retention::WorkerRetentionError, > { - SqliteWorkspaceStore::recover_worker_removal_execution( - self, - workspace_id, - worker, - expected_worker_revision, - reason, - ) + SqliteWorkspaceStore::recover_worker_removal_execution(self, workspace_id, worker) } fn fail_worker_removal( diff --git a/crates/workspace-server/src/worker_source.rs b/crates/workspace-server/src/worker_source.rs index 4d3a98da..08504cca 100644 --- a/crates/workspace-server/src/worker_source.rs +++ b/crates/workspace-server/src/worker_source.rs @@ -175,7 +175,6 @@ pub(crate) trait VerifiedWorkerRemoveExecutor: Send + Sync { source: VerifiedWorkerMutationSource, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result; } @@ -217,7 +216,6 @@ impl worker_runtime::worker_source::EmbeddedWorkerMutationDispatcher proof: InProcessWorkerMutationProof, target_runtime_id: &str, target_worker_id: &str, - expected_worker_revision: &str, reason: &str, ) -> Result< worker::WorkspaceResponse, @@ -241,13 +239,7 @@ impl worker_runtime::worker_source::EmbeddedWorkerMutationDispatcher ) })?; executor - .execute( - source, - target_runtime_id, - target_worker_id, - expected_worker_revision, - reason, - ) + .execute(source, target_runtime_id, target_worker_id, reason) .map_err(worker_runtime::worker_source::RuntimeWorkerMutationForwardError::Embedded) } } diff --git a/resources/prompts/role/orchestrator.md b/resources/prompts/role/orchestrator.md index 5a05800a..273298f6 100644 --- a/resources/prompts/role/orchestrator.md +++ b/resources/prompts/role/orchestrator.md @@ -22,4 +22,4 @@ Do not create or delegate an implementation worktree/branch until the Ticket rec Workspace roots, cwd, profile selector, and launch-prompt configuration are control-plane/environment facts rather than user instructions. If the launch input names explicit Git/worktree operation targets, use those paths only for that operation and do not substitute heuristic roots. -Use `WorkerRemove` only for a terminal or authoritatively reassigned non-internal Coder after implementation, review, fix, merge/commit, and report handoffs are complete. Do not remove a Coder merely because one turn completed or it is temporarily idle; retain it while review or request-changes work can still return. The Worker must already be stopped, must not be restoring, must have no current Ticket assignment, pending notification, Reviewer handoff, legal hold, or pin, and must not be this Orchestrator. Immediately before removal, reread authoritative Ticket state, assignment, thread/review evidence, and the target Worker with `WorkerShow`; pass the exact current `updated_at` value as `expected_worker_revision` with a concise reason. After removal, reread the Worker catalog and attachment state. Treat revision, assignment, running/restoring, retention-policy, attachment-close, and attachment-release conflicts as authoritative failures: do not guess policy or retry with stale input. `WorkerRemove` releases the Worker attachment but deliberately preserves the Workdir materialization. +Use `WorkerRemove` only for a terminal or authoritatively reassigned non-internal Coder after implementation, review, fix, merge/commit, and report handoffs are complete. Do not remove a Coder merely because one turn completed or it is temporarily idle; retain it while review or request-changes work can still return. The Worker must already be stopped, must not be restoring, must have no current Ticket assignment, pending notification, Reviewer handoff, legal hold, or pin, and must not be this Orchestrator. Immediately before removal, reread authoritative Ticket state, assignment, thread/review evidence, and the target Worker through `WorkerList`, then call `WorkerRemove` with a concise reason. Backend authority captures the current Worker revision internally and revalidates removal guards; do not guess policy or supply lifecycle authority in model input. After removal, reread the Worker catalog and attachment state. Treat assignment, running/restoring, retention-policy, attachment-close, and attachment-release conflicts as authoritative failures. `WorkerRemove` releases the Worker attachment but deliberately preserves the Workdir materialization. diff --git a/web/workspace/src/lib/generated/protocol.ts b/web/workspace/src/lib/generated/protocol.ts index f4b03115..1a1fbaf4 100644 --- a/web/workspace/src/lib/generated/protocol.ts +++ b/web/workspace/src/lib/generated/protocol.ts @@ -138,7 +138,7 @@ runtime_id?: string | null, /** * Producer-owned monotonic revision for this Worker subject. */ -subject_revision: number, state: SubscriptionWorkerState, workspace_id?: string | null, display_name?: string | null, profile?: string | null, repository_id?: string | null, working_directory_id?: SubscriptionWorkdirId | null, }; +subject_revision: number, state: SubscriptionWorkerState, has_running_internal_workers: boolean, workspace_id?: string | null, display_name?: string | null, profile?: string | null, repository_id?: string | null, working_directory_id?: SubscriptionWorkdirId | null, }; export type SubscriptionWorkdir = { working_directory_id: SubscriptionWorkdirId, repository_id: string, state: string, primary_worker_id?: SubscriptionWorkerId | null, }; diff --git a/web/workspace/src/lib/workspace/console/ConsoleTasks.svelte b/web/workspace/src/lib/workspace/console/ConsoleTasks.svelte index a78285d9..5c0a57bc 100644 --- a/web/workspace/src/lib/workspace/console/ConsoleTasks.svelte +++ b/web/workspace/src/lib/workspace/console/ConsoleTasks.svelte @@ -1,12 +1,26 @@ -{#if mode === "mini" && tasks.length > 0} +{#if mode === "mini" && (tasks.length > 0 || workerViews.length > 1)}
{#each activeTasks as task (task.taskid)}
@@ -38,8 +52,25 @@ {task.subject.split("\n", 1)[0]}
{/each} -
- {counts.total} task(s) — pending: {counts.pending}, inprogress: {counts.inprogress}, completed: {counts.completed}, deleted: {counts.deleted} +
+ + {counts.total} task(s) — pending: {counts.pending}, inprogress: {counts.inprogress}, completed: {counts.completed}, deleted: {counts.deleted} + + {#if workerViews.length > 1} + + + {#each workerViews as view, index (view.sessionId ?? "main")} + {#if index > 0}{/if} + + {/each} + + + {/if}
{:else if mode === "pane"} @@ -95,12 +126,69 @@ } .task-mini-row, - .task-heading { + .task-heading, + .task-summary-row { display: flex; min-width: 0; gap: 0.5rem; } + .task-summary-row { + align-items: baseline; + } + + .task-summary { + flex: 1 1 auto; + min-width: 0; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + } + + .worker-view-tabs { + display: flex; + flex: 0 1 auto; + min-width: 0; + max-width: 60%; + margin-left: auto; + overflow-x: auto; + color: var(--text-muted); + scrollbar-width: none; + white-space: nowrap; + } + + .worker-view-tabs::-webkit-scrollbar { + display: none; + } + + .worker-view-tabs button { + flex: 0 0 auto; + min-width: 0; + margin: 0; + padding: 0; + border: 0; + background: transparent; + color: inherit; + font: inherit; + line-height: inherit; + cursor: pointer; + } + + .worker-view-tabs button:hover, + .worker-view-tabs button:focus-visible { + color: var(--text); + } + + .worker-view-tabs button:focus-visible { + outline: 1px solid currentcolor; + outline-offset: 2px; + } + + .worker-view-tabs button.active { + color: var(--accent); + font-weight: 700; + } + .task-mark, .task-id { flex: 0 0 auto; diff --git a/web/workspace/src/lib/workspace/console/Spinner.svelte b/web/workspace/src/lib/workspace/console/Spinner.svelte index 620e9b71..21b90518 100644 --- a/web/workspace/src/lib/workspace/console/Spinner.svelte +++ b/web/workspace/src/lib/workspace/console/Spinner.svelte @@ -29,7 +29,7 @@ diff --git a/web/workspace/src/lib/workspace/console/model.test.ts b/web/workspace/src/lib/workspace/console/model.test.ts index 1005371a..5d75353c 100644 --- a/web/workspace/src/lib/workspace/console/model.test.ts +++ b/web/workspace/src/lib/workspace/console/model.test.ts @@ -2,11 +2,14 @@ import type { Event } from "$lib/generated/protocol"; import { type ConsoleEventInput, type ConsoleLine, + consoleWorkerViews, createConsoleProjector, isConsoleProjectionEvent, projectConsole, projectConsoleLines, projectOverviewLines, + resolveConsoleViewScrollTop, + resolveConsoleWorkerView, segmentsToText, selectConsoleTimelineLines, workerConsoleHref, @@ -1443,6 +1446,96 @@ Deno.test("Internal Worker output stays separate and revision-fenced", () => { }, }]); assertEquals(projection.internalWorkers[0].console.lines.length, 1); + + const views = consoleWorkerViews(projection); + assertEquals(views.map((view) => [view.sessionId, view.label]), [ + [null, "main"], + ["child-session", "research"], + ]); + assertEquals( + resolveConsoleWorkerView(projection, "child-session").console.lines[0].body, + "child output", + ); + assertEquals(resolveConsoleWorkerView(projection, "missing").sessionId, null); +}); + +Deno.test("console Worker views expose only direct Internal Workers", () => { + const projector = createConsoleProjector(); + projector.append([{ + eventId: "nested", + event: { + event: "internal_worker", + data: { + worker: { + session_id: "child-session", + name: "research", + parent_session_id: "parent-session", + kind: "sub_worker", + }, + revision: 1, + event: { + event: "internal_worker", + data: { + worker: { + session_id: "grandchild-session", + name: "nested", + parent_session_id: "child-session", + kind: "sub_worker", + }, + revision: 1, + event: { event: "status", data: { status: "running" } }, + }, + }, + }, + }, + }]); + + projector.append([{ + eventId: "peer", + event: { + event: "internal_worker", + data: { + worker: { + session_id: "peer-other", + name: "research", + parent_session_id: "parent-session", + kind: "sub_worker", + }, + revision: 1, + event: { event: "status", data: { status: "idle" } }, + }, + }, + }]); + + const projection = projector.snapshot(); + const views = consoleWorkerViews(projection); + assertEquals(views.map((view) => view.sessionId), [ + null, + "child-session", + "peer-other", + ]); + assertEquals(views[1].label, "research · ession"); + assertEquals(views[2].label, "research · -other"); + assertEquals( + resolveConsoleWorkerView(projection, "grandchild-session").sessionId, + null, + ); +}); + +Deno.test("console Worker view scroll restores manual offsets and auto-follow", () => { + assertEquals(resolveConsoleViewScrollTop(undefined, 1000, 200), 1000); + assertEquals( + resolveConsoleViewScrollTop({ top: 100, autoFollow: true }, 1000, 200), + 1000, + ); + assertEquals( + resolveConsoleViewScrollTop({ top: 300, autoFollow: false }, 1000, 200), + 300, + ); + assertEquals( + resolveConsoleViewScrollTop({ top: 900, autoFollow: false }, 1000, 200), + 800, + ); }); Deno.test("parent snapshot authoritatively replaces Internal Worker projections", () => { diff --git a/web/workspace/src/lib/workspace/console/model.ts b/web/workspace/src/lib/workspace/console/model.ts index 66958aa5..064e1189 100644 --- a/web/workspace/src/lib/workspace/console/model.ts +++ b/web/workspace/src/lib/workspace/console/model.ts @@ -91,18 +91,57 @@ export type InternalWorkerProjection = { console: ConsoleProjection; }; -export type FlattenedInternalWorkerProjection = InternalWorkerProjection & { - depth: number; +export type ConsoleViewScroll = { + top: number; + autoFollow: boolean; }; -export function flattenInternalWorkers( - workers: InternalWorkerProjection[], - depth = 0, -): FlattenedInternalWorkerProjection[] { - return workers.flatMap((worker) => [ - { ...worker, depth }, - ...flattenInternalWorkers(worker.console.internalWorkers, depth + 1), - ]); +export function resolveConsoleViewScrollTop( + state: ConsoleViewScroll | undefined, + scrollHeight: number, + clientHeight: number, +): number { + if (!state || state.autoFollow) return scrollHeight; + return Math.min(state.top, Math.max(0, scrollHeight - clientHeight)); +} + +export type ConsoleWorkerView = { + sessionId: string | null; + label: string; + console: ConsoleProjection; +}; + +export function consoleWorkerViews( + projection: ConsoleProjection, +): ConsoleWorkerView[] { + const labels = projection.internalWorkers.map((worker) => + worker.worker.name || "subworker" + ); + const labelCounts = new Map(); + for (const label of labels) { + labelCounts.set(label, (labelCounts.get(label) ?? 0) + 1); + } + return [ + { sessionId: null, label: "main", console: projection }, + ...projection.internalWorkers.map((worker, index) => { + const label = labels[index] ?? "subworker"; + return { + sessionId: worker.worker.session_id, + label: labelCounts.get(label) === 1 + ? label + : `${label} · ${worker.worker.session_id.slice(-6)}`, + console: worker.console, + }; + }), + ]; +} + +export function resolveConsoleWorkerView( + projection: ConsoleProjection, + selectedSessionId: string | null, +): ConsoleWorkerView { + const views = consoleWorkerViews(projection); + return views.find((view) => view.sessionId === selectedSessionId) ?? views[0]; } export type ConsoleProjection = { diff --git a/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts b/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts index 67c9ef34..fa30d021 100644 --- a/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts +++ b/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts @@ -797,7 +797,7 @@ Deno.test("Web Console renders the client-projected Worker task store", async () assert( consolePage.includes("ConsoleTasks") && - consolePage.includes("consoleProjection.tasks") && + consolePage.includes("selectedConsoleProjection.tasks") && consolePage.includes("taskPaneOpen"), "Console should expose the projected task store through its existing client model", ); @@ -818,3 +818,44 @@ Deno.test("Web Console renders the client-projected Worker task store", async () "Task projection should replay the protocol client-side without adding a task API", ); }); + +Deno.test("Web Console switches main and direct SubWorker views from the Tasks row", async () => { + const consolePage = await Deno.readTextFile( + new URL( + "./../../../routes/w/[workspaceId]/runtimes/[runtimeId]/workers/[workerId]/console/+page.svelte", + import.meta.url, + ), + ); + const tasksComponent = await Deno.readTextFile( + new URL("./ConsoleTasks.svelte", import.meta.url), + ); + const consoleModel = await Deno.readTextFile( + new URL("./model.ts", import.meta.url), + ); + + assert( + consolePage.includes("selectedWorkerViewSessionId") && + consolePage.includes("selectConsoleWorkerView") && + consolePage.includes("selectedConsoleProjection.lines") && + consolePage.includes("selectedConsoleProjection.tasks") && + consolePage.includes("onSelectWorkerView") && + consolePage.includes("selectConsoleWorkerView(resolvedSessionId, false)") && + consolePage.includes("consoleWorkerViewSelectionIsResolved") && + !consolePage.includes("internal-worker-pane") && + !consolePage.includes("flattenInternalWorkers"), + "Console should render one selected transcript/task projection without appending Internal Worker panes", + ); + assert( + tasksComponent.includes('role="group"') && + tasksComponent.includes("aria-pressed") && + tasksComponent.includes("onclick") && + tasksComponent.includes("tasks.length > 0 || workerViews.length > 1"), + "Tasks summary should expose a clickable and accessible Worker view selector even with zero tasks", + ); + assert( + consoleModel.includes("consoleWorkerViews") && + consoleModel.includes("projection.internalWorkers.map") && + consoleModel.includes("resolveConsoleWorkerView"), + "Worker view selection should use direct Internal Worker session identities with main fallback", + ); +}); diff --git a/web/workspace/src/lib/workspace/sidebar/WorkersNavSection.svelte b/web/workspace/src/lib/workspace/sidebar/WorkersNavSection.svelte index 584add88..b2fffcd6 100644 --- a/web/workspace/src/lib/workspace/sidebar/WorkersNavSection.svelte +++ b/web/workspace/src/lib/workspace/sidebar/WorkersNavSection.svelte @@ -1,4 +1,5 @@