From 28360107c1bfab57fb0de99d74bfa48e74423ca8 Mon Sep 17 00:00:00 2001 From: Can Celik Date: Mon, 21 Sep 2026 20:47:14 +0300 Subject: fix: distinguish agent completion from startup and session changes (#4457) refs #3338 refs #3990 refs #3916 --- src/app/actions.rs | 364 ++++++++++++++++++++++++++++++++++++----------------- src/app/agents.rs | 1 + 2 files changed, 248 insertions(+), 117 deletions(-) (limited to 'src/app') diff --git a/src/app/actions.rs b/src/app/actions.rs index 85314331..953bcd94 100644 --- a/src/app/actions.rs +++ b/src/app/actions.rs @@ -25,12 +25,7 @@ fn is_background_completion_transition(prev_state: AgentState, new_state: AgentS } fn is_completion_transition(change: &EffectiveStateChange) -> bool { - is_completion_transition_parts( - change.previous_state, - change.state, - change.previous_agent_label.as_deref(), - change.agent_label.as_deref(), - ) + is_background_completion_transition(change.previous_state, change.state) } fn public_tab_id_for_index(ws: &crate::workspace::Workspace, tab_idx: usize) -> Option { @@ -40,19 +35,6 @@ fn public_tab_id_for_index(ws: &crate::workspace::Workspace, tab_idx: usize) -> )) } -pub fn is_completion_transition_parts( - previous_state: AgentState, - state: AgentState, - previous_agent_label: Option<&str>, - agent_label: Option<&str>, -) -> bool { - is_background_completion_transition(previous_state, state) - || (previous_state == AgentState::Unknown - && state == AgentState::Idle - && previous_agent_label.is_some() - && previous_agent_label == agent_label) -} - pub fn active_tab_suppresses_notifications( is_active_tab: bool, outer_terminal_focus: Option, @@ -60,7 +42,6 @@ pub fn active_tab_suppresses_notifications( is_active_tab && outer_terminal_focus != Some(false) } -#[cfg(test)] pub fn notification_sound_for_state_change( suppress_active_tab_notifications: bool, prev_state: AgentState, @@ -82,58 +63,21 @@ pub fn notification_sound_for_state_change( } } -pub fn notification_sound_for_state_change_with_agent_labels( - suppress_active_tab_notifications: bool, - prev_state: AgentState, - new_state: AgentState, - previous_agent_label: Option<&str>, - agent_label: Option<&str>, -) -> Option { - if new_state == prev_state { - return None; - } - - match new_state { - AgentState::Blocked => Some(crate::sound::Sound::Request), - AgentState::Idle - if is_completion_transition_parts( - prev_state, - new_state, - previous_agent_label, - agent_label, - ) && !suppress_active_tab_notifications => - { - Some(crate::sound::Sound::Done) - } - _ => None, - } -} - fn notification_sound_for_effective_state_change( suppress_active_tab_notifications: bool, change: &EffectiveStateChange, ) -> Option { - if change.state == change.previous_state { - return None; - } - - match change.state { - AgentState::Blocked => Some(crate::sound::Sound::Request), - AgentState::Idle - if is_completion_transition(change) && !suppress_active_tab_notifications => - { - Some(crate::sound::Sound::Done) - } - _ => None, - } + notification_sound_for_state_change( + suppress_active_tab_notifications, + change.previous_state, + change.state, + ) } -pub fn notification_toast_for_state_change_with_agent_labels( +pub fn notification_toast_for_state_change( suppress_active_tab_notifications: bool, prev_state: AgentState, new_state: AgentState, - previous_agent_label: Option<&str>, - agent_label: Option<&str>, ) -> Option { if suppress_active_tab_notifications || new_state == prev_state { return None; @@ -141,14 +85,7 @@ pub fn notification_toast_for_state_change_with_agent_labels( match new_state { AgentState::Blocked => Some(ToastKind::NeedsAttention), - AgentState::Idle - if is_completion_transition_parts( - prev_state, - new_state, - previous_agent_label, - agent_label, - ) => - { + AgentState::Idle if is_background_completion_transition(prev_state, new_state) => { Some(ToastKind::Finished) } _ => None, @@ -159,15 +96,11 @@ fn notification_toast_for_effective_state_change( suppress_active_tab_notifications: bool, change: &EffectiveStateChange, ) -> Option { - if suppress_active_tab_notifications || change.state == change.previous_state { - return None; - } - - match change.state { - AgentState::Blocked => Some(ToastKind::NeedsAttention), - AgentState::Idle if is_completion_transition(change) => Some(ToastKind::Finished), - _ => None, - } + notification_toast_for_state_change( + suppress_active_tab_notifications, + change.previous_state, + change.state, + ) } pub fn notification_toast_for_pane_state_update( @@ -181,12 +114,10 @@ pub fn notification_toast_for_pane_state_update( return None; } - notification_toast_for_state_change_with_agent_labels( + notification_toast_for_state_change( suppress_active_tab_notifications, update.previous_state, update.state, - update.previous_agent_label.as_deref(), - update.agent_label.as_deref(), ) } @@ -1709,39 +1640,55 @@ impl AppState { managed_changed, agent_name_changed, unchanged_change, - managed_launch_pending, suppress_acquisition_completion, + completion_reset, ) = { let terminal = self.terminals.get_mut(&terminal_id)?; let previous_agent_name = terminal.agent_name.clone(); - let managed_launch_pending = terminal.managed_agent_launch_pending(); + let had_completion = terminal.last_agent_completion_seq.is_some() || !previous_seen; let mutation = update(terminal)?; + let completion_reset = mutation.session_ref_changed + || mutation + .effective_state_change + .as_ref() + .is_some_and(|change| change.previous_agent_label != change.agent_label); + if completion_reset { + terminal.last_agent_completion_seq = None; + } let managed_changed = terminal.reconcile_managed_agent_at(now, false); let suppress_acquisition_completion = terminal.finish_agent_process_acquisition(); let agent_name_changed = terminal.agent_name != previous_agent_name; - let unchanged_change = (mutation.agent_released || agent_name_changed) + let unchanged_change = (mutation.agent_released + || agent_name_changed + || (completion_reset && had_completion)) .then(|| terminal.unchanged_effective_state_change_at(now)); ( mutation, managed_changed, agent_name_changed, unchanged_change, - managed_launch_pending, suppress_acquisition_completion, + completion_reset, ) }; + if completion_reset { + self.pending_agent_notifications.remove(&pane_id); + self.workspaces[ws_idx].pane_state_mut(pane_id)?.seen = true; + } if mutation.session_ref_changed || managed_changed || agent_name_changed { self.mark_session_dirty(); } let agent_released = mutation.agent_released; let change = mutation.effective_state_change.or(unchanged_change)?; let suppress_completion = force_suppress_completion - || (change.state == AgentState::Idle - && (managed_launch_pending || suppress_acquisition_completion)); + || (change.state == AgentState::Idle && suppress_acquisition_completion); if change.previous_state != change.state { self.next_agent_state_change_seq += 1; if let Some(terminal) = self.terminals.get_mut(&terminal_id) { terminal.last_agent_state_change_seq = Some(self.next_agent_state_change_seq); + terminal.last_agent_completion_seq = (!suppress_completion + && is_completion_transition(&change)) + .then_some(self.next_agent_state_change_seq); } } let seen = self.apply_pane_state_change(ws_idx, pane_id, &change, suppress_completion)?; @@ -3054,34 +3001,217 @@ mod tests { assert!(pane.seen); } + fn assert_completion_guard_sequence( + acquired: bool, + states: &[AgentState], + expect_completion: bool, + ) { + let mut app = app_with_workspaces(&["active", "background"]); + app.active = Some(0); + app.toast_config.delivery = crate::config::ToastDelivery::Herdr; + app.toast_config.delay_seconds = 0; + let pane_id = app.workspaces[1].tabs[0].root_pane; + if acquired { + app.handle_app_event(AppEvent::AgentProcessDetected { + pane_id, + agent: Agent::Pi, + observed_at: Instant::now(), + }); + } + for &state in states { + app.handle_app_event(AppEvent::StateChanged { + pane_id, + agent: Some(Agent::Pi), + state, + visible_blocker: state == AgentState::Blocked, + visible_working: state == AgentState::Working, + process_exited: false, + observed_at: Instant::now(), + }); + } + let finished = app + .toast + .as_ref() + .is_some_and(|toast| toast.kind == ToastKind::Finished); + assert_eq!( + finished, expect_completion, + "completion notification: {states:?}" + ); + assert_eq!( + !app.workspaces[1].panes[&pane_id].seen, expect_completion, + "unseen completion: {states:?}" + ); + let terminal = &app.terminals[&app.workspaces[1].panes[&pane_id].attached_terminal_id]; + assert_eq!( + terminal.last_agent_completion_seq.is_some(), + expect_completion + ); + if expect_completion { + assert_eq!( + terminal.last_agent_completion_seq, + terminal.last_agent_state_change_seq + ); + } + app.assert_invariants_for_test(); + } + #[test] - fn idle_after_known_unknown_agent_in_background_marks_done() { - let mut state = app_with_workspaces(&["active", "background"]); - state.toast_config.delivery = crate::config::ToastDelivery::Herdr; - state.active = Some(0); - let bg_pane_id = *state.workspaces[1].panes.keys().next().unwrap(); + fn completion_guard_unknown_to_idle_is_not_completed_work() { + assert_completion_guard_sequence(false, &[AgentState::Unknown, AgentState::Idle], false); + } - state.handle_app_event(AppEvent::StateChanged { - pane_id: bg_pane_id, - agent: Some(Agent::Pi), - state: AgentState::Unknown, - visible_blocker: false, - visible_working: false, - process_exited: false, - observed_at: std::time::Instant::now(), - }); - state.handle_app_event(AppEvent::StateChanged { - pane_id: bg_pane_id, - agent: Some(Agent::Pi), - state: AgentState::Idle, - visible_blocker: false, - visible_working: false, - process_exited: false, - observed_at: std::time::Instant::now(), - }); + #[test] + fn completion_guard_first_work_finishes_without_prior_idle() { + assert_completion_guard_sequence(true, &[AgentState::Working, AgentState::Idle], true); + } - let pane = state.workspaces[1].panes.get(&bg_pane_id).unwrap(); - assert!(!pane.seen); + #[test] + fn completion_guard_first_work_can_pause_for_permission() { + assert_completion_guard_sequence( + true, + &[AgentState::Working, AgentState::Blocked, AgentState::Idle], + true, + ); + } + + #[test] + fn completion_guard_startup_trust_is_not_completed_work() { + assert_completion_guard_sequence(true, &[AgentState::Blocked, AgentState::Idle], false); + } + + #[test] + fn completion_guard_managed_launch_readiness_does_not_swallow_work() { + for first_state in [AgentState::Blocked, AgentState::Working] { + let mut app = app_with_workspaces(&["active", "background"]); + app.active = Some(0); + let pane_id = app.workspaces[1].tabs[0].root_pane; + let terminal_id = app.workspaces[1].panes[&pane_id] + .attached_terminal_id + .clone(); + app.terminals + .get_mut(&terminal_id) + .unwrap() + .begin_managed_agent( + "worker".into(), + Agent::Pi, + Instant::now(), + std::time::Duration::ZERO, + std::time::Duration::from_secs(60), + ); + for state in [first_state, AgentState::Idle] { + app.handle_app_event(AppEvent::StateChanged { + pane_id, + agent: Some(Agent::Pi), + state, + visible_blocker: state == AgentState::Blocked, + visible_working: state == AgentState::Working, + process_exited: false, + observed_at: Instant::now(), + }); + } + let terminal = &app.terminals[&terminal_id]; + assert!(terminal.managed_agent_interactive_ready()); + assert_eq!( + terminal.last_agent_completion_seq.is_some(), + first_state == AgentState::Working + ); + assert_eq!( + !app.workspaces[1].panes[&pane_id].seen, + first_state == AgentState::Working + ); + } + } + + #[test] + fn completion_guard_same_state_agent_replacement_clears_old_work() { + let mut app = app_with_workspaces(&["active", "background"]); + app.active = Some(0); + app.toast_config.delay_seconds = 5; + let pane_id = app.workspaces[1].tabs[0].root_pane; + for (seq, label, state) in [ + (1, "old", AgentState::Working), + (2, "old", AgentState::Idle), + (3, "new", AgentState::Idle), + ] { + app.handle_app_event(AppEvent::HookStateReported { + pane_id, + source: "custom:worker".into(), + agent_label: label.into(), + state, + message: None, + seq: Some(seq), + session_ref: None, + }); + if seq == 2 { + assert!(!app.workspaces[1].panes[&pane_id].seen); + assert!(app.pending_agent_notifications.contains_key(&pane_id)); + } + } + let terminal = &app.terminals[&app.workspaces[1].panes[&pane_id].attached_terminal_id]; + assert_eq!(terminal.effective_agent_label(), Some("new")); + assert!(terminal.last_agent_completion_seq.is_none()); + assert!(app.workspaces[1].panes[&pane_id].seen); + assert!(!app.pending_agent_notifications.contains_key(&pane_id)); + } + + #[test] + fn completion_guard_idle_session_replacement_clears_seen_and_pending_delivery() { + let mut app = app_with_workspaces(&["active", "background"]); + app.active = Some(0); + app.toast_config.delay_seconds = 5; + let pane_id = app.workspaces[1].tabs[0].root_pane; + for (seq, session, reason) in [(1, "old-session", "startup"), (2, "new-session", "clear")] { + let updates = app.handle_app_event(AppEvent::AgentSessionReported { + pane_id, + source: "herdr:claude".into(), + agent_label: "claude".into(), + seq: Some(seq), + session_ref: crate::agent_resume::AgentSessionRef::id(session), + session_start_source: Some(reason.into()), + }); + if seq == 1 { + for state in [AgentState::Working, AgentState::Idle] { + app.handle_app_event(AppEvent::StateChanged { + pane_id, + agent: Some(Agent::Claude), + state, + visible_blocker: false, + visible_working: state == AgentState::Working, + process_exited: false, + observed_at: Instant::now(), + }); + } + assert!(!app.workspaces[1].panes[&pane_id].seen); + assert!(app.pending_agent_notifications.contains_key(&pane_id)); + } else { + assert!( + !updates.is_empty(), + "session replacement must publish attention reset" + ); + } + } + let terminal = &app.terminals[&app.workspaces[1].panes[&pane_id].attached_terminal_id]; + assert_eq!( + terminal + .persisted_agent_session + .as_ref() + .unwrap() + .session_ref + .value, + "new-session" + ); + assert!(terminal.last_agent_completion_seq.is_none()); + assert!(app.workspaces[1].panes[&pane_id].seen); + assert!(!app.pending_agent_notifications.contains_key(&pane_id)); + } + + #[test] + fn completion_guard_normal_turn_still_finishes() { + assert_completion_guard_sequence( + true, + &[AgentState::Idle, AgentState::Working, AgentState::Idle], + true, + ); } #[test] @@ -3139,9 +3269,9 @@ mod tests { .pop() .expect("idle state update"); - assert!(update.suppress_completion); - assert!(state.workspaces[1].panes[&pane_id].seen); - assert!(!matches!( + assert!(!update.suppress_completion); + assert!(!state.workspaces[1].panes[&pane_id].seen); + assert!(matches!( state.toast.as_ref().map(|toast| toast.kind), Some(ToastKind::Finished) )); diff --git a/src/app/agents.rs b/src/app/agents.rs index 5033505d..fd2a58ed 100644 --- a/src/app/agents.rs +++ b/src/app/agents.rs @@ -395,6 +395,7 @@ impl App { launch_pending: terminal.managed_agent_launch_pending(), interactive_ready: terminal.managed_agent_interactive_ready(), state_change_seq: terminal.last_agent_state_change_seq.unwrap_or(0), + completion_seq: terminal.last_agent_completion_seq, cwd: pane.cwd, foreground_cwd: pane.foreground_cwd, revision: pane.revision, -- cgit v1.2.3