From 1845b9e20ebb8532842af2dbb8094b329efbcb52 Mon Sep 17 00:00:00 2001 From: Ryan Ward Date: Wed, 9 Sep 2026 13:55:34 -0500 Subject: [PATCH] Fix child startup readiness and ordered provider tools Wait for shell bootstrap before dispatching child prompts, serialize provider preprocessing, and make permission callbacks idempotent. Restrict task lists to concrete multistep plans and stop inferring completion from tool activity. Update regression fixtures and resolve existing lint and test-layout failures. Verified formatting, both presubmit Clippy commands, and 354 targeted nextest tests. --- app/src/ai/acp/runtime_model.rs | 120 +------------ app/src/ai/acp/runtime_model_tests.rs | 116 ++++++++++++ app/src/ai/acp/transport.rs | 1 - app/src/ai/blocklist/action_model.rs | 86 ++++++++- .../action_model/execute/start_agent_tests.rs | 40 ++--- .../execute/wait_for_events_tests.rs | 30 ++-- app/src/ai/blocklist/action_model_tests.rs | 70 ++++++++ app/src/ai/blocklist/block/cli_controller.rs | 85 +-------- .../blocklist/block/cli_controller_tests.rs | 81 +++++++++ app/src/ai/blocklist/controller_tests.rs | 1 - app/src/ai/crosscheck/prompt.rs | 30 +--- app/src/ai/crosscheck/prompt_tests.rs | 26 +++ app/src/ai/provider/discovery.rs | 39 +--- app/src/ai/provider/discovery_tests.rs | 35 ++++ app/src/ai/provider/mod.rs | 1 - app/src/ai/remote_logging.rs | 44 +---- app/src/ai/remote_logging_tests.rs | 40 +++++ app/src/ai/runtime/event_translator.rs | 167 ++++-------------- app/src/ai/runtime/event_translator_tests.rs | 147 +++++++-------- .../runtime/provider_run_coordinator_tests.rs | 11 +- app/src/ai/runtime/rig.rs | 2 - app/src/ai/runtime/rig_request.rs | 35 +--- app/src/ai/runtime/rig_request_tests.rs | 4 +- app/src/ai/tool_diagnostics.rs | 16 +- app/src/ai/tool_diagnostics_tests.rs | 12 ++ app/src/code_review/merge_conflicts.rs | 2 +- app/src/pane_group/pane/terminal_pane.rs | 20 +-- app/src/terminal/view.rs | 66 +++++++ app/src/terminal/view_tests.rs | 79 +++++++++ .../codebase_index.rs | 19 +- .../full_source_code_embedding/manager.rs | 8 +- .../full_source_code_embedding/snapshot.rs | 2 - .../ai/src/index/local_project_index/mod.rs | 1 + crates/galaxy_agent_core/src/provider_run.rs | 9 + .../src/provider_run_tests.rs | 55 ++++++ crates/galaxy_agent_rig/src/chatgpt.rs | 121 +------------ crates/galaxy_agent_rig/src/chatgpt_tests.rs | 117 ++++++++++++ crates/galaxy_agent_rig/src/native.rs | 13 +- crates/galaxy_agent_rig/src/native_tests.rs | 9 + crates/galaxy_agent_rig/src/stream.rs | 118 +------------ .../galaxy_agent_rig/src/stream_unit_tests.rs | 114 ++++++++++++ 41 files changed, 1097 insertions(+), 895 deletions(-) create mode 100644 app/src/ai/acp/runtime_model_tests.rs create mode 100644 app/src/ai/blocklist/block/cli_controller_tests.rs create mode 100644 app/src/ai/crosscheck/prompt_tests.rs create mode 100644 app/src/ai/provider/discovery_tests.rs create mode 100644 app/src/ai/remote_logging_tests.rs create mode 100644 app/src/ai/tool_diagnostics_tests.rs create mode 100644 crates/galaxy_agent_rig/src/chatgpt_tests.rs create mode 100644 crates/galaxy_agent_rig/src/native_tests.rs create mode 100644 crates/galaxy_agent_rig/src/stream_unit_tests.rs diff --git a/app/src/ai/acp/runtime_model.rs b/app/src/ai/acp/runtime_model.rs index 65fc308f..2f6f0b77 100644 --- a/app/src/ai/acp/runtime_model.rs +++ b/app/src/ai/acp/runtime_model.rs @@ -270,121 +270,5 @@ pub(crate) enum AcpRuntimeModelEvent { impl SingletonEntity for AcpRuntimeModel {} #[cfg(test)] -mod tests { - use galaxy_acp::{SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption}; - - use super::{AcpDiscoveryState, AcpRuntimeModel}; - use crate::settings::{AcpAgentSettings, AcpConfigOptionSettings}; - - #[test] - fn normalize_config_options_preserves_values_and_order() { - let options = vec![ - SessionConfigOption::select( - "model", - "Model", - "fast", - vec![ - SessionConfigSelectOption::new("fast", "Fast"), - SessionConfigSelectOption::new("accurate", "Accurate"), - ], - ) - .category(SessionConfigOptionCategory::Model), - SessionConfigOption::boolean("thinking", "Thinking", true), - ]; - - let normalized = AcpRuntimeModel::normalize_config_options(options); - - assert_eq!(normalized.len(), 2); - assert_eq!(normalized[0].kind, "select"); - assert_eq!(normalized[0].current_value, serde_json::json!("fast")); - assert_eq!(normalized[0].options[0].value, serde_json::json!("fast")); - assert_eq!( - normalized[0].options[1].value, - serde_json::json!("accurate") - ); - assert_eq!(normalized[1].kind, "boolean"); - assert_eq!(normalized[1].current_value, serde_json::json!(true)); - } - - #[test] - fn normalize_config_options_preserves_unknown_categories() { - let option = SessionConfigOption::select( - "custom", - "Custom", - "one", - Vec::::new(), - ) - .category(SessionConfigOptionCategory::Other("_custom".to_owned())); - - let normalized = AcpRuntimeModel::normalize_config_options(vec![option]); - - assert_eq!(normalized[0].category.as_deref(), Some("_custom")); - } - - #[test] - fn selection_replaces_model_and_preserves_other_current_values() { - let options = vec![ - AcpConfigOptionSettings { - id: "model".to_owned(), - name: "Model".to_owned(), - description: None, - category: Some("model".to_owned()), - kind: "select".to_owned(), - current_value: serde_json::json!("fast"), - options: Vec::new(), - }, - AcpConfigOptionSettings { - id: "thinking".to_owned(), - name: "Thinking".to_owned(), - description: None, - category: None, - kind: "boolean".to_owned(), - current_value: serde_json::json!(true), - options: Vec::new(), - }, - ]; - - assert_eq!( - AcpRuntimeModel::selection_values_for_model( - &options, - "model", - &serde_json::json!("accurate") - ), - std::collections::BTreeMap::from([ - ("model".to_owned(), serde_json::json!("accurate")), - ("thinking".to_owned(), serde_json::json!(true)), - ]) - ); - } - - #[test] - fn discovery_state_has_explicit_running_success_and_failure_values() { - assert_eq!(AcpDiscoveryState::default(), AcpDiscoveryState::Idle); - assert_eq!( - AcpDiscoveryState::Succeeded { option_count: 2 }, - AcpDiscoveryState::Succeeded { option_count: 2 } - ); - assert_eq!( - AcpDiscoveryState::Failed { - message: "timeout".to_owned() - }, - AcpDiscoveryState::Failed { - message: "timeout".to_owned() - } - ); - } - - #[test] - fn discovery_metadata_fields_are_optional_for_legacy_settings() { - let json = serde_json::json!({ - "id": "codex", - "name": "Codex", - "config_options": [] - }); - let agent: AcpAgentSettings = serde_json::from_value(json).unwrap(); - - assert!(agent.discovery_timestamp.is_none()); - assert!(agent.discovery_source.is_none()); - assert!(agent.discovery_error.is_none()); - } -} +#[path = "runtime_model_tests.rs"] +mod tests; diff --git a/app/src/ai/acp/runtime_model_tests.rs b/app/src/ai/acp/runtime_model_tests.rs new file mode 100644 index 00000000..07c0f9a9 --- /dev/null +++ b/app/src/ai/acp/runtime_model_tests.rs @@ -0,0 +1,116 @@ +use galaxy_acp::{SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption}; + +use super::{AcpDiscoveryState, AcpRuntimeModel}; +use crate::settings::{AcpAgentSettings, AcpConfigOptionSettings}; + +#[test] +fn normalize_config_options_preserves_values_and_order() { + let options = vec![ + SessionConfigOption::select( + "model", + "Model", + "fast", + vec![ + SessionConfigSelectOption::new("fast", "Fast"), + SessionConfigSelectOption::new("accurate", "Accurate"), + ], + ) + .category(SessionConfigOptionCategory::Model), + SessionConfigOption::boolean("thinking", "Thinking", true), + ]; + + let normalized = AcpRuntimeModel::normalize_config_options(options); + + assert_eq!(normalized.len(), 2); + assert_eq!(normalized[0].kind, "select"); + assert_eq!(normalized[0].current_value, serde_json::json!("fast")); + assert_eq!(normalized[0].options[0].value, serde_json::json!("fast")); + assert_eq!( + normalized[0].options[1].value, + serde_json::json!("accurate") + ); + assert_eq!(normalized[1].kind, "boolean"); + assert_eq!(normalized[1].current_value, serde_json::json!(true)); +} + +#[test] +fn normalize_config_options_preserves_unknown_categories() { + let option = SessionConfigOption::select( + "custom", + "Custom", + "one", + Vec::::new(), + ) + .category(SessionConfigOptionCategory::Other("_custom".to_owned())); + + let normalized = AcpRuntimeModel::normalize_config_options(vec![option]); + + assert_eq!(normalized[0].category.as_deref(), Some("_custom")); +} + +#[test] +fn selection_replaces_model_and_preserves_other_current_values() { + let options = vec![ + AcpConfigOptionSettings { + id: "model".to_owned(), + name: "Model".to_owned(), + description: None, + category: Some("model".to_owned()), + kind: "select".to_owned(), + current_value: serde_json::json!("fast"), + options: Vec::new(), + }, + AcpConfigOptionSettings { + id: "thinking".to_owned(), + name: "Thinking".to_owned(), + description: None, + category: None, + kind: "boolean".to_owned(), + current_value: serde_json::json!(true), + options: Vec::new(), + }, + ]; + + assert_eq!( + AcpRuntimeModel::selection_values_for_model( + &options, + "model", + &serde_json::json!("accurate") + ), + std::collections::BTreeMap::from([ + ("model".to_owned(), serde_json::json!("accurate")), + ("thinking".to_owned(), serde_json::json!(true)), + ]) + ); +} + +#[test] +fn discovery_state_has_explicit_running_success_and_failure_values() { + assert_eq!(AcpDiscoveryState::default(), AcpDiscoveryState::Idle); + assert_eq!( + AcpDiscoveryState::Succeeded { option_count: 2 }, + AcpDiscoveryState::Succeeded { option_count: 2 } + ); + assert_eq!( + AcpDiscoveryState::Failed { + message: "timeout".to_owned() + }, + AcpDiscoveryState::Failed { + message: "timeout".to_owned() + } + ); +} + +#[test] +fn discovery_metadata_fields_are_optional_for_legacy_settings() { + let json = serde_json::json!({ + "id": "codex", + "name": "Codex", + "config_options": [] + }); + let agent: AcpAgentSettings = serde_json::from_value(json).unwrap(); + + assert!(agent.discovery_timestamp.is_none()); + assert!(agent.discovery_source.is_none()); + assert!(agent.discovery_error.is_none()); +} diff --git a/app/src/ai/acp/transport.rs b/app/src/ai/acp/transport.rs index f9d24b36..a37a4b22 100644 --- a/app/src/ai/acp/transport.rs +++ b/app/src/ai/acp/transport.rs @@ -228,7 +228,6 @@ fn response_translator( max_context_tokens: None, capabilities: RuntimeCapabilities::session_runtime(), empty_output_message: Some("> ACP agent completed without a text response.".to_owned()), - todo_items: None, }) } diff --git a/app/src/ai/blocklist/action_model.rs b/app/src/ai/blocklist/action_model.rs index 753a6c2d..8706b44f 100644 --- a/app/src/ai/blocklist/action_model.rs +++ b/app/src/ai/blocklist/action_model.rs @@ -780,10 +780,15 @@ pub struct BlocklistAIActionModel { /// Permission-card rejections whose cancelled action result must not emit a second provider event. denied_permissions: HashSet<(AIConversationId, AIAgentActionId)>, + pending_permissions: HashSet<(AIConversationId, AIAgentActionId)>, /// Actions parked because their executor-specific UI or state was not ready yet. not_ready_actions: NotReadyActionTracker, + /// Provider preprocessing waits for execution order, since it can read file contents. + /// The value is true while preprocessing is in flight. + deferred_provider_preprocessing: HashMap<(AIConversationId, AIAgentActionId), bool>, + /// Durable provider work identity for actions owned by an active provider run. provider_tool_executions: HashMap<(AIConversationId, AIAgentActionId), ProviderToolExecutionRef>, @@ -899,7 +904,9 @@ impl BlocklistAIActionModel { running_actions: Default::default(), action_order: Default::default(), denied_permissions: Default::default(), + pending_permissions: Default::default(), not_ready_actions: Default::default(), + deferred_provider_preprocessing: Default::default(), provider_tool_executions: Default::default(), terminal_view_id, pending_preprocessed_actions: Default::default(), @@ -1280,6 +1287,13 @@ impl BlocklistAIActionModel { conversation_id: AIConversationId, id: &AIAgentActionId, ) -> Option { + if self + .deferred_provider_preprocessing + .get(&(conversation_id, id.clone())) + == Some(&true) + { + return Some(AIActionStatus::Preprocessing); + } if let Some(status) = pending_action_status( &self.pending_actions, &self.running_actions, @@ -1507,7 +1521,7 @@ impl BlocklistAIActionModel { } fn handle_not_executed_action( - &self, + &mut self, action: &AIAgentAction, reason: NotExecutedReason, conversation_id: AIConversationId, @@ -1515,6 +1529,12 @@ impl BlocklistAIActionModel { ) { match reason { NotExecutedReason::NeedsConfirmation => { + if !self + .pending_permissions + .insert((conversation_id, action.id.clone())) + { + return; + } #[cfg(not(target_family = "wasm"))] log_tool_event( ctx, @@ -1624,6 +1644,42 @@ impl BlocklistAIActionModel { .get(&conversation_id) .and_then(|queue| queue.iter().position(|action| &action.id == action_id))?; + let key = (conversation_id, action_id.clone()); + if self.provider_tool_executions.contains_key(&key) && idx != 0 { + return None; + } + // Readiness callbacks also enter here directly, bypassing the normal queue loop. + // They must obey the same serial barriers before preprocessing or executing. + if let Some(current_phase) = self.action_execution_phase(conversation_id) { + let action = self.pending_actions[&conversation_id][idx].clone(); + if !self.can_start_action_in_current_phase(&action, conversation_id, current_phase, ctx) + { + return None; + } + } + if let Some(in_flight) = self.deferred_provider_preprocessing.get_mut(&key) { + if *in_flight { + return None; + } + *in_flight = true; + let action = self.pending_actions[&conversation_id][idx].clone(); + let execution_ref = self.provider_tool_execution_ref(conversation_id, action_id); + let future = self.preprocess_action(&action, conversation_id, ctx); + ctx.spawn(future, move |me, (), ctx| { + // A cancelled or replaced generation must never restart this action. + if me.provider_tool_execution_ref(conversation_id, &action.id) != execution_ref { + return; + } + me.deferred_provider_preprocessing.remove(&key); + if is_user_initiated { + me.execute_action(&action.id, conversation_id, ctx); + } else { + me.try_to_execute_available_actions(conversation_id, ctx); + } + }); + return None; + } + let action = self .pending_actions .get_mut(&conversation_id)? @@ -1640,7 +1696,13 @@ impl BlocklistAIActionModel { "permission_kind": format!("{:?}", permission_kind_for_action(&action.action)), "phase": format!("{phase:?}"), }); - if is_user_initiated { + let resolves_permission = self.pending_permissions.contains(&key) + && (is_user_initiated + || self.executor.update(ctx, |executor, ctx| { + executor.can_autoexecute_action(&action, conversation_id, ctx) + })); + if resolves_permission { + self.pending_permissions.remove(&key); #[cfg(not(target_family = "wasm"))] log_tool_event( ctx, @@ -1884,7 +1946,15 @@ impl BlocklistAIActionModel { for action in actions.iter() { action_ids.insert(action.id.clone()); - preprocess_future.push(self.preprocess_action(action, conversation_id, ctx)); + if self + .provider_tool_executions + .contains_key(&(conversation_id, action.id.clone())) + { + self.deferred_provider_preprocessing + .insert((conversation_id, action.id.clone()), false); + } else { + preprocess_future.push(self.preprocess_action(action, conversation_id, ctx)); + } } let preprocess_id = self @@ -2053,6 +2123,8 @@ impl BlocklistAIActionModel { reason: Option, ctx: &mut ModelContext, ) { + self.deferred_provider_preprocessing + .retain(|(conv_id, _), _| *conv_id != conversation_id); self.executor.update(ctx, |executor, ctx| { executor.cancel_all_running_async_actions_for_conversation(conversation_id, reason, ctx) }); @@ -2253,12 +2325,16 @@ impl BlocklistAIActionModel { conversation_id: AIConversationId, ctx: &mut ModelContext, ) { + self.deferred_provider_preprocessing + .retain(|(conv_id, _), _| *conv_id != conversation_id); self.past_action_results .retain(|(conv_id, _), _| *conv_id != conversation_id); self.provider_tool_executions .retain(|(conv_id, _), _| *conv_id != conversation_id); self.denied_permissions .retain(|(conv_id, _)| *conv_id != conversation_id); + self.pending_permissions + .retain(|(conv_id, _)| *conv_id != conversation_id); self.pending_actions.remove(&conversation_id); self.running_actions.remove(&conversation_id); self.finished_action_results.remove(&conversation_id); @@ -2372,6 +2448,10 @@ impl BlocklistAIActionModel { let execution_ref = self .provider_tool_executions .remove(&(conversation_id, action_result.id.clone())); + self.deferred_provider_preprocessing + .remove(&(conversation_id, action_result.id.clone())); + self.pending_permissions + .remove(&(conversation_id, action_result.id.clone())); let permission_denied = self .denied_permissions .remove(&(conversation_id, action_result.id.clone())); diff --git a/app/src/ai/blocklist/action_model/execute/start_agent_tests.rs b/app/src/ai/blocklist/action_model/execute/start_agent_tests.rs index 0d8c2543..38676a86 100644 --- a/app/src/ai/blocklist/action_model/execute/start_agent_tests.rs +++ b/app/src/ai/blocklist/action_model/execute/start_agent_tests.rs @@ -16,11 +16,23 @@ use crate::test_util::settings::initialize_history_persistence_for_tests; const FIRST_REQUEST_ID: StartAgentRequestId = StartAgentRequestId::from_raw_for_test(0); -/// Stable placeholder run_id assigned to the parent conversation in tests -/// that exercise the server-backed Oz child path. Tests without this id -/// exercise the direct-provider local child path instead. +/// Stable server run ID for parent conversations. Child execution mode, rather +/// than the presence of this ID, selects startup versus completion waits. const PARENT_RUN_ID: &str = "00000000-0000-0000-0000-000000000001"; +fn remote_execution_mode() -> StartAgentExecutionMode { + StartAgentExecutionMode::Remote { + environment_id: "env-123".to_string(), + skill_references: vec![], + model_id: String::new(), + computer_use_enabled: false, + worker_host: String::new(), + harness_type: "oz".to_string(), + title: String::new(), + auth_secret_name: None, + } +} + #[derive(Default)] struct CapturedDirectProviderChildLinks(Vec<(AIAgentActionId, AIConversationId, AIConversationId)>); @@ -394,10 +406,7 @@ fn execute_returns_error_when_child_startup_is_blocked_before_initialization() { ctx, ); }); - let action = build_start_agent_action( - StartAgentVersion::V1, - StartAgentExecutionMode::local_with_defaults(), - ); + let action = build_start_agent_action(StartAgentVersion::V1, remote_execution_mode()); let execution = executor.update(&mut app, |executor, ctx| { let input = ExecuteActionInput { @@ -581,10 +590,7 @@ fn execute_resolves_success_when_request_linkage_happens_after_child_already_sta ctx, ); }); - let action = build_start_agent_action( - StartAgentVersion::V1, - StartAgentExecutionMode::local_with_defaults(), - ); + let action = build_start_agent_action(StartAgentVersion::V1, remote_execution_mode()); let execution = executor.update(&mut app, |executor, ctx| { let input = ExecuteActionInput { @@ -739,10 +745,7 @@ fn hosted_child_link_does_not_publish_direct_provider_panel_event() { ctx, ); }); - let action = build_start_agent_action( - StartAgentVersion::V1, - StartAgentExecutionMode::local_with_defaults(), - ); + let action = build_start_agent_action(StartAgentVersion::V1, remote_execution_mode()); let execution = executor.update(&mut app, |executor, ctx| { let input = ExecuteActionInput { @@ -1769,7 +1772,7 @@ struct PendingChildLaunch { child_conversation_id: AIConversationId, } -/// Dispatches a local child launch and creates (but does not yet link) its +/// Dispatches a remote child launch and creates (but does not yet link) its /// child conversation, leaving one in-flight pending in the executor with a /// model subscribed to capture `CleanupFailedChildLaunch` events. Tests link /// the child and drive it to a terminal state, then assert on cleanup. The @@ -1803,10 +1806,7 @@ fn dispatch_pending_child_launch( ctx, ); }); - let action = build_start_agent_action( - StartAgentVersion::V1, - StartAgentExecutionMode::local_with_defaults(), - ); + let action = build_start_agent_action(StartAgentVersion::V1, remote_execution_mode()); // The pending lives in the executor regardless of the returned execution, // and cleanup is emitted synchronously from the child status update, so the // action-result plumbing is discarded. diff --git a/app/src/ai/blocklist/action_model/execute/wait_for_events_tests.rs b/app/src/ai/blocklist/action_model/execute/wait_for_events_tests.rs index 16e5d9cd..2b0f7c35 100644 --- a/app/src/ai/blocklist/action_model/execute/wait_for_events_tests.rs +++ b/app/src/ai/blocklist/action_model/execute/wait_for_events_tests.rs @@ -19,6 +19,7 @@ use crate::ai::blocklist::orchestration_event_streamer::OrchestrationEventStream use crate::ai::blocklist::BlocklistAIHistoryModel; use crate::server::server_api::ai::{AIClient, MockAIClient}; use crate::server::server_api::ServerApiProvider; +use crate::test_util::settings::initialize_history_persistence_for_tests; #[test] fn watchdog_timeout_constants_match_documented_values() { @@ -88,24 +89,12 @@ fn execute_invokes_parent_registration_and_honors_child_short_circuit() { // without a server fetch (asserted via the mock's times(0) expectation), // and the wait still flips the conversation into WaitingForEvents. App::test((), |mut app| async move { + initialize_history_persistence_for_tests(&mut app); let _flag_guard = FeatureFlag::WaitForEventsParentRegistration.override_enabled(true); let terminal_view_id = EntityId::new(); let history_model = app.add_singleton_model(|_| BlocklistAIHistoryModel::new(vec![], &[])); - // A streamer whose server fetch must never be called: the child - // short-circuit precedes any `get_ambient_agent_task` call. - let mut mock = MockAIClient::new(); - mock.expect_get_ambient_agent_task().times(0); - let ai_client: Arc = Arc::new(mock); - let server_api = ServerApiProvider::new_for_test().get(); - // Held for the lifetime of the test so the mock's times(0) expectation - // is verified on drop; resolved internally by `execute()` via - // `OrchestrationEventStreamer::handle`. - let _streamer = app.add_singleton_model(|ctx| { - OrchestrationEventStreamer::new_with_clients_for_test(ai_client, server_api, ctx) - }); - let executor = app.add_model(|ctx| WaitForEventsExecutor::new(terminal_view_id, ctx)); // Child conversation: own run_id plus a parent_agent_id. @@ -123,6 +112,21 @@ fn execute_invokes_parent_registration_and_honors_child_short_circuit() { ); }); + // Subscribe after restoring the fixture so unrelated restore-time harness + // discovery does not count as a parent-registration fetch. + // A streamer whose server fetch must never be called: the child + // short-circuit precedes any `get_ambient_agent_task` call. + let mut mock = MockAIClient::new(); + mock.expect_get_ambient_agent_task().times(0); + let ai_client: Arc = Arc::new(mock); + let server_api = ServerApiProvider::new_for_test().get(); + // Held for the lifetime of the test so the mock's times(0) expectation + // is verified on drop; resolved internally by `execute()` via + // `OrchestrationEventStreamer::handle`. + let _streamer = app.add_singleton_model(|ctx| { + OrchestrationEventStreamer::new_with_clients_for_test(ai_client, server_api, ctx) + }); + let action = AIAgentAction { id: AIAgentActionId::from("wait-action".to_string()), action: AIAgentActionType::WaitForEvents { diff --git a/app/src/ai/blocklist/action_model_tests.rs b/app/src/ai/blocklist/action_model_tests.rs index 82b68d79..d8ce5253 100644 --- a/app/src/ai/blocklist/action_model_tests.rs +++ b/app/src/ai/blocklist/action_model_tests.rs @@ -7,6 +7,7 @@ use crate::ai::agent::{ AIAgentAction, AIAgentActionResultType, AIAgentActionType, AnyFileContent, FileContext, GrepResult, ReadFilesResult, }; +use crate::test_util::terminal::{add_window_with_terminal, initialize_app_for_terminal_view}; fn make_action_result(id: &str) -> Arc { Arc::new(AIAgentActionResult { @@ -54,6 +55,75 @@ fn pending_tool_batch(call_ids: &[&str]) -> PendingToolBatch { } } +#[test] +fn readiness_retry_cannot_preprocess_across_a_serial_barrier_or_skip_a_pending_tool() { + warpui::App::test((), |mut app| async move { + initialize_app_for_terminal_view(&mut app); + let terminal = add_window_with_terminal(&mut app, None); + terminal.update(&mut app, |view, ctx| { + let terminal_view_id = ctx.view_id(); + let relevant_files = ctx.add_model(GetRelevantFilesController::new); + let action_model = ctx.add_model(|ctx| { + BlocklistAIActionModel::new( + view.model.clone(), + view.active_session().clone(), + view.model_event_dispatcher(), + relevant_files, + terminal_view_id, + ctx, + ) + }); + action_model.update(ctx, |model, ctx| { + let conversation_id = AIConversationId::new(); + let batch = pending_tool_batch(&["first", "second"]); + let actions = vec![action("first"), action("second")]; + model.provider_tool_executions.extend( + provider_action_correlations(&actions, conversation_id, &batch).unwrap(), + ); + for action in &actions { + model + .deferred_provider_preprocessing + .insert((conversation_id, action.id.clone()), false); + } + model + .pending_actions + .insert(conversation_id, actions.clone().into()); + model.add_running_action( + conversation_id, + AIAgentActionId::from("running".to_string()), + RunningActionPhase::Serial, + ); + assert!(model + .start_pending_action_by_id( + &actions[0].id, + conversation_id, + ActionExecutionInitiator::Automatic, + ctx + ) + .is_none()); + assert!( + !model.deferred_provider_preprocessing + [&(conversation_id, actions[0].id.clone())] + ); + model.running_actions.remove(&conversation_id); + assert!(model + .start_pending_action_by_id( + &actions[1].id, + conversation_id, + ActionExecutionInitiator::User, + ctx + ) + .is_none()); + assert!( + !model.deferred_provider_preprocessing + [&(conversation_id, actions[1].id.clone())] + ); + assert_eq!(model.pending_actions[&conversation_id].len(), 2); + }); + }); + }); +} + fn count_startable_actions_for_pass(phases: &[(RunningActionPhase, bool)]) -> usize { let mut current_phase = None; let mut count = 0; diff --git a/app/src/ai/blocklist/block/cli_controller.rs b/app/src/ai/blocklist/block/cli_controller.rs index db6dd330..4c5d551d 100644 --- a/app/src/ai/blocklist/block/cli_controller.rs +++ b/app/src/ai/blocklist/block/cli_controller.rs @@ -1158,86 +1158,5 @@ fn matches_requested_command_identity( } #[cfg(test)] -mod tests { - use super::*; - - #[test] - fn stop_takeover_does_not_request_a_completion_assessment() { - let state = LongRunningCommandControlState::User { - reason: UserTakeOverReason::Stop, - }; - - assert!(!should_request_completion_assessment(Some(&state))); - } - - #[test] - fn non_stop_control_states_can_request_a_completion_assessment() { - let agent_state = LongRunningCommandControlState::Agent { - is_blocked: false, - should_hide_responses: false, - }; - let transfer_state = LongRunningCommandControlState::User { - reason: UserTakeOverReason::TransferFromAgent { - reason: "needs user input".to_owned(), - }, - }; - - assert!(should_request_completion_assessment(None)); - assert!(should_request_completion_assessment(Some(&agent_state))); - assert!(should_request_completion_assessment(Some(&transfer_state))); - } - - #[test] - fn prose_monitor_turn_is_nudged_once_until_a_tool_action_runs() { - assert!(should_nudge_monitor_turn(false, false)); - assert!(!should_nudge_monitor_turn(false, true)); - assert!(!should_nudge_monitor_turn(true, false)); - } - - #[test] - fn shell_control_event_must_match_conversation_and_requested_command() { - let active_conversation_id = AIConversationId::new(); - let other_conversation_id = AIConversationId::new(); - let active_action_id = AIAgentActionId::from("same-action".to_owned()); - let other_action_id = AIAgentActionId::from("other-action".to_owned()); - - assert!(matches_active_requested_command( - active_conversation_id, - &active_action_id, - Some(active_conversation_id), - Some(&active_action_id), - )); - assert!(!matches_active_requested_command( - other_conversation_id, - &active_action_id, - Some(active_conversation_id), - Some(&active_action_id), - )); - assert!(!matches_active_requested_command( - active_conversation_id, - &other_action_id, - Some(active_conversation_id), - Some(&active_action_id), - )); - } - - #[test] - fn requested_command_identity_rejects_duplicate_id_from_another_conversation() { - let active_conversation_id = AIConversationId::new(); - let other_conversation_id = AIConversationId::new(); - let duplicate_action_id = AIAgentActionId::from("duplicate-action".to_owned()); - - assert!(matches_requested_command_identity( - active_conversation_id, - &duplicate_action_id, - Some(active_conversation_id), - Some(&duplicate_action_id), - )); - assert!(!matches_requested_command_identity( - other_conversation_id, - &duplicate_action_id, - Some(active_conversation_id), - Some(&duplicate_action_id), - )); - } -} +#[path = "cli_controller_tests.rs"] +mod tests; diff --git a/app/src/ai/blocklist/block/cli_controller_tests.rs b/app/src/ai/blocklist/block/cli_controller_tests.rs new file mode 100644 index 00000000..82a5f256 --- /dev/null +++ b/app/src/ai/blocklist/block/cli_controller_tests.rs @@ -0,0 +1,81 @@ +use super::*; + +#[test] +fn stop_takeover_does_not_request_a_completion_assessment() { + let state = LongRunningCommandControlState::User { + reason: UserTakeOverReason::Stop, + }; + + assert!(!should_request_completion_assessment(Some(&state))); +} + +#[test] +fn non_stop_control_states_can_request_a_completion_assessment() { + let agent_state = LongRunningCommandControlState::Agent { + is_blocked: false, + should_hide_responses: false, + }; + let transfer_state = LongRunningCommandControlState::User { + reason: UserTakeOverReason::TransferFromAgent { + reason: "needs user input".to_owned(), + }, + }; + + assert!(should_request_completion_assessment(None)); + assert!(should_request_completion_assessment(Some(&agent_state))); + assert!(should_request_completion_assessment(Some(&transfer_state))); +} + +#[test] +fn prose_monitor_turn_is_nudged_once_until_a_tool_action_runs() { + assert!(should_nudge_monitor_turn(false, false)); + assert!(!should_nudge_monitor_turn(false, true)); + assert!(!should_nudge_monitor_turn(true, false)); +} + +#[test] +fn shell_control_event_must_match_conversation_and_requested_command() { + let active_conversation_id = AIConversationId::new(); + let other_conversation_id = AIConversationId::new(); + let active_action_id = AIAgentActionId::from("same-action".to_owned()); + let other_action_id = AIAgentActionId::from("other-action".to_owned()); + + assert!(matches_active_requested_command( + active_conversation_id, + &active_action_id, + Some(active_conversation_id), + Some(&active_action_id), + )); + assert!(!matches_active_requested_command( + other_conversation_id, + &active_action_id, + Some(active_conversation_id), + Some(&active_action_id), + )); + assert!(!matches_active_requested_command( + active_conversation_id, + &other_action_id, + Some(active_conversation_id), + Some(&active_action_id), + )); +} + +#[test] +fn requested_command_identity_rejects_duplicate_id_from_another_conversation() { + let active_conversation_id = AIConversationId::new(); + let other_conversation_id = AIConversationId::new(); + let duplicate_action_id = AIAgentActionId::from("duplicate-action".to_owned()); + + assert!(matches_requested_command_identity( + active_conversation_id, + &duplicate_action_id, + Some(active_conversation_id), + Some(&duplicate_action_id), + )); + assert!(!matches_requested_command_identity( + other_conversation_id, + &duplicate_action_id, + Some(active_conversation_id), + Some(&duplicate_action_id), + )); +} diff --git a/app/src/ai/blocklist/controller_tests.rs b/app/src/ai/blocklist/controller_tests.rs index 0e456ac9..9637cc7b 100644 --- a/app/src/ai/blocklist/controller_tests.rs +++ b/app/src/ai/blocklist/controller_tests.rs @@ -267,7 +267,6 @@ fn provider_snapshot(conversation_id: AIConversationId) -> super::ActiveProvider max_context_tokens: Some(128_000), capabilities: RuntimeCapabilities::provider(), empty_output_message: None, - todo_items: None, }, action_context: crate::ai::runtime::ProviderActionContext::new_for_test( task_id.to_string(), diff --git a/app/src/ai/crosscheck/prompt.rs b/app/src/ai/crosscheck/prompt.rs index 48194873..00169592 100644 --- a/app/src/ai/crosscheck/prompt.rs +++ b/app/src/ai/crosscheck/prompt.rs @@ -54,31 +54,5 @@ pub fn is_approved(response: &str) -> bool { } #[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_is_approved_exact() { - assert!(is_approved("LGTM!")); - } - - #[test] - fn test_is_approved_with_whitespace() { - assert!(is_approved(" LGTM! ")); - assert!(is_approved("\nLGTM!\n")); - } - - #[test] - fn test_is_approved_case_insensitive() { - assert!(is_approved("lgtm!")); - assert!(is_approved("Lgtm!")); - assert!(is_approved("lgtm")); - } - - #[test] - fn test_is_not_approved_with_feedback() { - assert!(!is_approved("LGTM! But also fix the typo.")); - assert!(!is_approved("1. Fix the loop\n2. Rename variable")); - assert!(!is_approved("")); - } -} +#[path = "prompt_tests.rs"] +mod tests; diff --git a/app/src/ai/crosscheck/prompt_tests.rs b/app/src/ai/crosscheck/prompt_tests.rs new file mode 100644 index 00000000..095bc9a6 --- /dev/null +++ b/app/src/ai/crosscheck/prompt_tests.rs @@ -0,0 +1,26 @@ +use super::*; + +#[test] +fn test_is_approved_exact() { + assert!(is_approved("LGTM!")); +} + +#[test] +fn test_is_approved_with_whitespace() { + assert!(is_approved(" LGTM! ")); + assert!(is_approved("\nLGTM!\n")); +} + +#[test] +fn test_is_approved_case_insensitive() { + assert!(is_approved("lgtm!")); + assert!(is_approved("Lgtm!")); + assert!(is_approved("lgtm")); +} + +#[test] +fn test_is_not_approved_with_feedback() { + assert!(!is_approved("LGTM! But also fix the typo.")); + assert!(!is_approved("1. Fix the loop\n2. Rename variable")); + assert!(!is_approved("")); +} diff --git a/app/src/ai/provider/discovery.rs b/app/src/ai/provider/discovery.rs index 1e40f839..401c04b2 100644 --- a/app/src/ai/provider/discovery.rs +++ b/app/src/ai/provider/discovery.rs @@ -171,40 +171,5 @@ fn prettify_model_id(model_id: &str) -> String { } #[cfg(test)] -mod tests { - use super::model_availability_is_usable; - - #[test] - fn requires_every_availability_status() { - assert!(model_availability_is_usable( - Some("AVAILABLE"), - "AUTHORIZED", - "AVAILABLE", - "AVAILABLE", - )); - assert!(!model_availability_is_usable( - None, - "AUTHORIZED", - "AVAILABLE", - "AVAILABLE", - )); - assert!(!model_availability_is_usable( - Some("AVAILABLE"), - "NOT_AUTHORIZED", - "AVAILABLE", - "AVAILABLE", - )); - assert!(!model_availability_is_usable( - Some("AVAILABLE"), - "AUTHORIZED", - "NOT_AVAILABLE", - "AVAILABLE", - )); - assert!(!model_availability_is_usable( - Some("AVAILABLE"), - "AUTHORIZED", - "AVAILABLE", - "NOT_AVAILABLE", - )); - } -} +#[path = "discovery_tests.rs"] +mod tests; diff --git a/app/src/ai/provider/discovery_tests.rs b/app/src/ai/provider/discovery_tests.rs new file mode 100644 index 00000000..4d73764f --- /dev/null +++ b/app/src/ai/provider/discovery_tests.rs @@ -0,0 +1,35 @@ +use super::model_availability_is_usable; + +#[test] +fn requires_every_availability_status() { + assert!(model_availability_is_usable( + Some("AVAILABLE"), + "AUTHORIZED", + "AVAILABLE", + "AVAILABLE", + )); + assert!(!model_availability_is_usable( + None, + "AUTHORIZED", + "AVAILABLE", + "AVAILABLE", + )); + assert!(!model_availability_is_usable( + Some("AVAILABLE"), + "NOT_AUTHORIZED", + "AVAILABLE", + "AVAILABLE", + )); + assert!(!model_availability_is_usable( + Some("AVAILABLE"), + "AUTHORIZED", + "NOT_AVAILABLE", + "AVAILABLE", + )); + assert!(!model_availability_is_usable( + Some("AVAILABLE"), + "AUTHORIZED", + "AVAILABLE", + "NOT_AVAILABLE", + )); +} diff --git a/app/src/ai/provider/mod.rs b/app/src/ai/provider/mod.rs index 498d84a5..253c09db 100644 --- a/app/src/ai/provider/mod.rs +++ b/app/src/ai/provider/mod.rs @@ -1,7 +1,6 @@ pub mod types; use crate::ai::openai::client::OpenAIClientConfig; - #[cfg(not(target_family = "wasm"))] use crate::ai::provider::client::BedrockClientConfig; diff --git a/app/src/ai/remote_logging.rs b/app/src/ai/remote_logging.rs index 65eb9b10..4fb30521 100644 --- a/app/src/ai/remote_logging.rs +++ b/app/src/ai/remote_logging.rs @@ -308,45 +308,5 @@ fn tail_limited_payload_context(payload: &str, max_chars: usize) -> Value { } #[cfg(test)] -mod tests { - use serde_json::json; - - use super::{normalize_endpoint_url, sanitize_error, tail_limited_payload_context}; - - #[test] - fn endpoint_accepts_base_or_logs_path() { - assert_eq!( - normalize_endpoint_url("https://logging.ryserve.net").as_deref(), - Some("https://logging.ryserve.net/api/logs") - ); - assert_eq!( - normalize_endpoint_url("https://logging.ryserve.net/api/logs/").as_deref(), - Some("https://logging.ryserve.net/api/logs") - ); - } - - #[test] - fn errors_are_compacted_truncated_and_lightly_redacted() { - let error = format!("failed\nAuthorization: Bearer sk-test {}", "x".repeat(700)); - let sanitized = sanitize_error(error); - - assert!(!sanitized.contains("sk-test")); - assert!(!sanitized.contains('\n')); - assert!(sanitized.chars().count() <= 501); - } - - #[test] - fn raw_payload_cap_keeps_tail() { - assert_eq!( - tail_limited_payload_context("0123456789", 4), - json!({ - "payload": "6789", - "payload_total_chars": 10, - "payload_included_chars": 4, - "payload_max_chars": 4, - "truncated": true, - "truncation_strategy": "tail", - }) - ); - } -} +#[path = "remote_logging_tests.rs"] +mod tests; diff --git a/app/src/ai/remote_logging_tests.rs b/app/src/ai/remote_logging_tests.rs new file mode 100644 index 00000000..e05c66b2 --- /dev/null +++ b/app/src/ai/remote_logging_tests.rs @@ -0,0 +1,40 @@ +use serde_json::json; + +use super::{normalize_endpoint_url, sanitize_error, tail_limited_payload_context}; + +#[test] +fn endpoint_accepts_base_or_logs_path() { + assert_eq!( + normalize_endpoint_url("https://logging.ryserve.net").as_deref(), + Some("https://logging.ryserve.net/api/logs") + ); + assert_eq!( + normalize_endpoint_url("https://logging.ryserve.net/api/logs/").as_deref(), + Some("https://logging.ryserve.net/api/logs") + ); +} + +#[test] +fn errors_are_compacted_truncated_and_lightly_redacted() { + let error = format!("failed\nAuthorization: Bearer sk-test {}", "x".repeat(700)); + let sanitized = sanitize_error(error); + + assert!(!sanitized.contains("sk-test")); + assert!(!sanitized.contains('\n')); + assert!(sanitized.chars().count() <= 501); +} + +#[test] +fn raw_payload_cap_keeps_tail() { + assert_eq!( + tail_limited_payload_context("0123456789", 4), + json!({ + "payload": "6789", + "payload_total_chars": 10, + "payload_included_chars": 4, + "payload_max_chars": 4, + "truncated": true, + "truncation_strategy": "tail", + }) + ); +} diff --git a/app/src/ai/runtime/event_translator.rs b/app/src/ai/runtime/event_translator.rs index 7b209dc0..c8d6bd23 100644 --- a/app/src/ai/runtime/event_translator.rs +++ b/app/src/ai/runtime/event_translator.rs @@ -26,9 +26,6 @@ pub(crate) struct RuntimeResponseConfig { pub(crate) max_context_tokens: Option, pub(crate) capabilities: RuntimeCapabilities, pub(crate) empty_output_message: Option, - /// Todo items supplied by the existing task transcript, when one is available. - #[serde(skip)] - pub(crate) todo_items: Option>, } /// Converts the provider-neutral runtime lifecycle into Galaxy's existing @@ -54,19 +51,16 @@ pub(crate) struct RuntimeResponseTranslator { pub(crate) struct ProviderRunResponseProjector { translator: RuntimeResponseTranslator, has_started_model_turn: bool, - todo_phase: usize, - todo_started: bool, + plan_tasks_projected: bool, finished: bool, } impl ProviderRunResponseProjector { pub(crate) fn new(config: RuntimeResponseConfig) -> Self { - let todo_started = config.todo_items.is_some(); Self { translator: RuntimeResponseTranslator::new(config), has_started_model_turn: false, - todo_phase: 0, - todo_started, + plan_tasks_projected: false, finished: false, } } @@ -78,9 +72,7 @@ impl ProviderRunResponseProjector { Self { translator: RuntimeResponseTranslator::restored(config, projection_was_initialized), has_started_model_turn: false, - // Task-list events are part of the already persisted projection. - todo_phase: usize::MAX, - todo_started: true, + plan_tasks_projected: true, finished: false, } } @@ -98,11 +90,9 @@ impl ProviderRunResponseProjector { self.translator.begin_followup_turn(); } self.has_started_model_turn = true; - let mut events = self.translator.translate(AgentEvent::TurnStarted { + self.translator.translate(AgentEvent::TurnStarted { runtime_request_id: String::new(), - })?; - events.extend(self.todo_phase_events()); - Ok(events) + }) } ProviderRunProjection::ModelEvent { event, .. } => self.translator.translate(event), ProviderRunProjection::ModelRetry { .. } => { @@ -111,10 +101,9 @@ impl ProviderRunResponseProjector { ProviderRunProjection::ModelTurnRequested { .. } | ProviderRunProjection::ModelTurnFinished { .. } => Ok(Vec::new()), ProviderRunProjection::ToolBatchReady { batch } => { - if !self.todo_started { + if !self.plan_tasks_projected { if let Some(todos) = todos_from_plan_batch(&batch) { - self.todo_started = true; - self.todo_phase = 1; + self.plan_tasks_projected = true; return Ok(vec![build_todo_update( &self.translator.config.task_id, api::message::update_todos::Operation::CreateTodoList( @@ -124,34 +113,9 @@ impl ProviderRunResponseProjector { ), )]); } - return Ok(Vec::new()); } - let todo_index = self.todo_phase.saturating_sub(1); - let Some(todo) = self.todo_items().get(todo_index).cloned() else { - return Ok(Vec::new()); - }; - self.todo_phase += 1; - let mut events = vec![build_todo_update( - &self.translator.config.task_id, - api::message::update_todos::Operation::MarkTodosCompleted( - api::MarkTodosCompleted { - todo_ids: vec![todo.id], - }, - ), - )]; - events.push(build_todo_update( - &self.translator.config.task_id, - api::message::update_todos::Operation::UpdatePendingTodos( - api::UpdatePendingTodos { - updated_pending_todos: self - .todo_items() - .into_iter() - .skip(self.todo_phase) - .collect(), - }, - ), - )); - Ok(events) + // Tool batches indicate runtime activity, not completion of planned work. + Ok(Vec::new()) } } } @@ -170,14 +134,9 @@ impl ProviderRunResponseProjector { } self.finished = true; match outcome { - ProviderRunOutcome::Completed(completion) => { - let mut events = self.todo_completion_events(); - events.extend( - self.translator - .finish_provider_run(completion.stop_reason.clone(), aggregate_usage), - ); - Ok(events) - } + ProviderRunOutcome::Completed(completion) => Ok(self + .translator + .finish_provider_run(completion.stop_reason.clone(), aggregate_usage)), ProviderRunOutcome::Failed(failure) => Ok(self.translator.provider_failure( &failure.message, failure.source.as_ref(), @@ -188,49 +147,10 @@ impl ProviderRunResponseProjector { .finish_provider_run(StopReason::Cancelled, aggregate_usage)), } } - - // Keep the direct-provider workflow visible in the existing task list protocol. These are - // response events, so the normal history model remains the sole owner of task-list state. - fn todo_phase_events(&mut self) -> Vec { - Vec::new() - } - - fn todo_completion_events(&self) -> Vec { - if self.todo_phase == 0 || self.todo_phase == usize::MAX { - return Vec::new(); - } - let todos = self.todo_items(); - if todos.is_empty() { - return Vec::new(); - } - vec![ - build_todo_update( - &self.translator.config.task_id, - api::message::update_todos::Operation::MarkTodosCompleted( - api::MarkTodosCompleted { - todo_ids: todos.iter().map(|todo| todo.id.clone()).collect(), - }, - ), - ), - build_todo_update( - &self.translator.config.task_id, - api::message::update_todos::Operation::UpdatePendingTodos( - api::UpdatePendingTodos { - updated_pending_todos: Vec::new(), - }, - ), - ), - ] - } - fn todo_items(&self) -> Vec { - self.translator - .config - .todo_items - .clone() - .unwrap_or_default() - } } +// Only explicit multi-step checklists in a plan become tasks. Other document bullets +// (risks, examples, requirements) are not executable tasks. fn todos_from_plan_batch( batch: &galaxy_agent_core::PendingToolBatch, ) -> Option> { @@ -240,46 +160,33 @@ fn todos_from_plan_batch( "create_plan" | "create_documents" ) })?; - let documents = plan_call.call.arguments.get("documents")?.as_array()?; - let content = documents.first()?.get("content")?.as_str()?; - let section = content - .split_once("## Tasks") - .or_else(|| content.split_once("## Implementation Tasks")) - .map(|(_, section)| section) - .unwrap_or(content); - let todos = section - .lines() - .filter_map(|line| { - let item = line - .trim() - .strip_prefix("- [ ]") - .or_else(|| line.trim().strip_prefix("-"))? - .trim(); - if item.is_empty() { - return None; - } - let title = item + let content = plan_call + .call + .arguments + .get("documents")? + .as_array()? + .first()? + .get("content")? + .as_str()?; + let mut lines = content.lines(); + lines.find(|line| matches!(line.trim(), "## Tasks" | "## Implementation Tasks"))?; + let todos = lines + .take_while(|line| !line.trim().starts_with("## ")) + .filter_map(|line| line.trim().strip_prefix("- [ ]").map(str::trim)) + .filter(|item| !item.is_empty()) + .take(50) + .enumerate() + .map(|(index, item)| api::TodoItem { + id: format!("plan-{}-{index}", plan_call.call.id), + title: item .split_once(" - ") .map_or(item, |(title, _)| title) - .trim(); - let id = format!( - "plan-{}", - title - .chars() - .filter_map(|character| character - .is_ascii_alphanumeric() - .then_some(character.to_ascii_lowercase())) - .collect::() - ); - Some(api::TodoItem { - id, - title: title.to_owned(), - description: item.to_owned(), - }) + .trim() + .to_owned(), + description: item.to_owned(), }) - .take(50) .collect::>(); - (!todos.is_empty()).then_some(todos) + (todos.len() > 1).then_some(todos) } fn build_todo_update( diff --git a/app/src/ai/runtime/event_translator_tests.rs b/app/src/ai/runtime/event_translator_tests.rs index 0640ac61..d25bd524 100644 --- a/app/src/ai/runtime/event_translator_tests.rs +++ b/app/src/ai/runtime/event_translator_tests.rs @@ -17,7 +17,6 @@ fn provider_translator() -> RuntimeResponseTranslator { max_context_tokens: Some(1_000), capabilities: RuntimeCapabilities::provider(), empty_output_message: None, - todo_items: None, }) } @@ -31,7 +30,6 @@ fn session_translator() -> RuntimeResponseTranslator { max_context_tokens: None, capabilities: RuntimeCapabilities::session_runtime(), empty_output_message: Some("> runtime completed without text".to_owned()), - todo_items: None, }) } @@ -46,7 +44,6 @@ fn restored_provider_projection_skips_stream_initialization() { max_context_tokens: Some(1_000), capabilities: RuntimeCapabilities::provider(), empty_output_message: None, - todo_items: None, }; let mut projector = ProviderRunResponseProjector::restored(config, true); let work_id = galaxy_agent_core::ExternalWorkId { @@ -86,24 +83,7 @@ fn restored_provider_projection_skips_stream_initialization() { } #[test] -fn provider_projection_uses_supplied_todos_and_advances_each_id() { - let todos = vec![ - warp_multi_agent_api::TodoItem { - id: "research".to_owned(), - title: "Research".to_owned(), - description: "Inspect".to_owned(), - }, - warp_multi_agent_api::TodoItem { - id: "implement".to_owned(), - title: "Implement".to_owned(), - description: "Edit".to_owned(), - }, - warp_multi_agent_api::TodoItem { - id: "verify".to_owned(), - title: "Verify".to_owned(), - description: "Check".to_owned(), - }, - ]; +fn provider_activity_does_not_create_or_complete_task_lists() { let mut projector = ProviderRunResponseProjector::new(RuntimeResponseConfig { task_id: "task".to_owned(), conversation_id: "conversation".to_owned(), @@ -113,13 +93,12 @@ fn provider_projection_uses_supplied_todos_and_advances_each_id() { max_context_tokens: Some(1_000), capabilities: RuntimeCapabilities::provider(), empty_output_message: None, - todo_items: Some(todos), }); let work_id = galaxy_agent_core::ExternalWorkId { run_id: galaxy_agent_core::ProviderRunId::new("run"), epoch: galaxy_agent_core::RunEpoch::new(1), }; - let initial = projector + let mut events = projector .project(ProviderRunProjection::ModelTurnStarted { work_id: work_id.clone(), profile: galaxy_agent_core::ProviderRequestProfile::new("base"), @@ -130,62 +109,40 @@ fn provider_projection_uses_supplied_todos_and_advances_each_id() { elapsed_ms: 1, }) .unwrap(); - assert_eq!(initial.len(), 2); - let first = projector + assert_eq!(events.len(), 1); + let tool_events = projector .project(ProviderRunProjection::ToolBatchReady { batch: galaxy_agent_core::PendingToolBatch { - work_id: galaxy_agent_core::ExternalWorkId { - run_id: galaxy_agent_core::ProviderRunId::new("run"), - epoch: galaxy_agent_core::RunEpoch::new(1), - }, + work_id, calls: Vec::new(), }, }) .unwrap(); - let second = projector - .project(ProviderRunProjection::ToolBatchReady { - batch: galaxy_agent_core::PendingToolBatch { - work_id: galaxy_agent_core::ExternalWorkId { - run_id: galaxy_agent_core::ProviderRunId::new("run"), - epoch: galaxy_agent_core::RunEpoch::new(1), - }, - calls: Vec::new(), - }, - }) - .unwrap(); - let ids = |events: &[warp_multi_agent_api::ResponseEvent]| { - events - .iter() - .flat_map(|event| match &event.r#type { - Some(response_event::Type::ClientActions(actions)) => actions - .actions - .iter() - .filter_map(|action| match &action.action { - Some(client_action::Action::AddMessagesToTask(add)) => add - .messages - .iter() - .filter_map(|message| match &message.message { - Some(message::Message::UpdateTodos(update)) => match update - .operation - .as_ref() - { - Some(message::update_todos::Operation::MarkTodosCompleted( - mark, - )) => Some(mark.todo_ids[0].clone()), - _ => None, - }, - _ => None, - }) - .next(), - _ => None, - }) - .collect::>(), - _ => Vec::new(), - }) - .collect::>() - }; - assert_eq!(ids(&first), vec!["research"]); - assert_eq!(ids(&second), vec!["implement"]); + assert!(tool_events.is_empty()); + events.extend( + projector + .finish( + &galaxy_agent_core::ProviderRunOutcome::Completed( + galaxy_agent_core::ProviderRunCompletion { + stop_reason: StopReason::Completed, + }, + ), + &Usage::default(), + ) + .unwrap(), + ); + for event in events { + if let Some(response_event::Type::ClientActions(actions)) = event.r#type { + for action in actions.actions { + if let Some(client_action::Action::AddMessagesToTask(add)) = action.action { + assert!(add.messages.iter().all(|message| !matches!( + message.message, + Some(message::Message::UpdateTodos(_)) + ))); + } + } + } + } } #[test] @@ -199,7 +156,6 @@ fn restored_uninitialized_projection_replays_init_before_live_delta() { max_context_tokens: Some(1_000), capabilities: RuntimeCapabilities::provider(), empty_output_message: None, - todo_items: None, }; let mut projector = ProviderRunResponseProjector::restored(config, false); let work_id = galaxy_agent_core::ExternalWorkId { @@ -227,18 +183,11 @@ fn restored_uninitialized_projection_replays_init_before_live_delta() { }) .unwrap(); - assert_eq!(started.len(), 2); + assert_eq!(started.len(), 1); assert!(matches!( started[0].r#type, Some(response_event::Type::Init(_)) )); - let Some(response_event::Type::ClientActions(actions)) = &started[1].r#type else { - panic!("initial provider turn should publish its task list"); - }; - assert!(matches!( - actions.actions[0].action, - Some(client_action::Action::AddMessagesToTask(_)) - )); assert_eq!(delta.len(), 1); assert!(matches!( delta[0].r#type, @@ -257,7 +206,6 @@ fn provider_followup_turn_starts_a_distinct_text_message() { max_context_tokens: Some(1_000), capabilities: RuntimeCapabilities::provider(), empty_output_message: None, - todo_items: None, }); let first_work_id = galaxy_agent_core::ExternalWorkId { run_id: galaxy_agent_core::ProviderRunId::new("run"), @@ -481,7 +429,6 @@ fn provider_retry_clears_failed_attempt_output_before_new_messages() { max_context_tokens: Some(1_000), capabilities: RuntimeCapabilities::provider(), empty_output_message: None, - todo_items: None, }); let work_id = galaxy_agent_core::ExternalWorkId { run_id: galaxy_agent_core::ProviderRunId::new("run"), @@ -661,3 +608,35 @@ fn capabilities_reject_events_owned_by_the_other_runtime_shape() { }) .is_err()); } + +#[test] +fn plan_tasks_require_multiple_explicit_checklist_items_in_a_task_section() { + let batch = |content: &str| galaxy_agent_core::PendingToolBatch { + work_id: galaxy_agent_core::ExternalWorkId { + run_id: galaxy_agent_core::ProviderRunId::new("run"), + epoch: galaxy_agent_core::RunEpoch::new(1), + }, + calls: vec![galaxy_agent_core::PendingToolCall { + call: galaxy_agent_core::ToolCall { + id: "plan-call".to_string(), + name: "create_plan".to_string(), + arguments: serde_json::json!({"documents": [{"content": content}]}), + }, + state: galaxy_agent_core::PendingToolCallState::Proposed, + }], + }; + for content in [ + "## Risks\n- [ ] Risk one\n- [ ] Risk two", + "## Tasks\n- [ ] Update the parser", + "## Tasks\n- Requirement one\n- Requirement two", + ] { + assert!(super::todos_from_plan_batch(&batch(content)).is_none()); + } + let todos = super::todos_from_plan_batch(&batch( + "## Tasks\n- [ ] Add retry correlation\n- [ ] Test stale callbacks\n## Risks\n- [ ] Slow shell startup", + )).unwrap(); + assert_eq!(todos.len(), 2); + assert_eq!(todos[0].title, "Add retry correlation"); + assert_eq!(todos[1].title, "Test stale callbacks"); + assert_ne!(todos[0].id, todos[1].id); +} diff --git a/app/src/ai/runtime/provider_run_coordinator_tests.rs b/app/src/ai/runtime/provider_run_coordinator_tests.rs index 934d0c6a..ced2b6f8 100644 --- a/app/src/ai/runtime/provider_run_coordinator_tests.rs +++ b/app/src/ai/runtime/provider_run_coordinator_tests.rs @@ -734,7 +734,7 @@ async fn execution_failure_commits_one_correlated_error_result() { } #[tokio::test] -async fn inline_tool_batches_continue_without_leaving_the_coordinator() { +async fn inline_tool_batches_commit_before_the_next_model_boundary() { let recall_turn = Ok(vec![ started("request-recall"), Ok(AgentEvent::Tool { @@ -759,6 +759,13 @@ async fn inline_tool_batches_continue_without_leaving_the_coordinator() { .await .unwrap(); + assert!(matches!(block, ProviderRunBlock::ReadyToCallModel)); + assert_eq!(runtime.requests().len(), 1); + let (_sender, control) = turn_control(); + let block = coordinator + .drive_until_blocked(control, collect_projection(&mut projections)) + .await + .unwrap(); assert!(matches!(block, ProviderRunBlock::AwaitingDriver { .. })); assert_eq!(runtime.requests().len(), 2); assert!(projections.iter().any(|projection| matches!( @@ -1208,7 +1215,6 @@ async fn transcript_projector_emits_one_ui_stream_for_the_whole_run() { max_context_tokens: Some(100_000), capabilities: RuntimeCapabilities::provider(), empty_output_message: None, - todo_items: None, }); let mut ui_events = Vec::new(); let (_sender, control) = turn_control(); @@ -1267,7 +1273,6 @@ fn transcript_projector_preserves_provider_failure_message() { max_context_tokens: Some(100_000), capabilities: RuntimeCapabilities::provider(), empty_output_message: None, - todo_items: None, }); let events = projector .finish( diff --git a/app/src/ai/runtime/rig.rs b/app/src/ai/runtime/rig.rs index 818c2e93..438ddbcf 100644 --- a/app/src/ai/runtime/rig.rs +++ b/app/src/ai/runtime/rig.rs @@ -137,7 +137,6 @@ pub(crate) async fn prepare_provider_run( task_id, needs_create_task, user_query, - todo_items, request, persistent_messages, tool_result_archive, @@ -160,7 +159,6 @@ pub(crate) async fn prepare_provider_run( max_context_tokens, capabilities: base_runtime.descriptor().capabilities.clone(), empty_output_message: None, - todo_items, }; Ok(PreparedProviderRun { base_profile: ProviderRunProfile::new(base_runtime, request), diff --git a/app/src/ai/runtime/rig_request.rs b/app/src/ai/runtime/rig_request.rs index dac3202d..0dadef9f 100644 --- a/app/src/ai/runtime/rig_request.rs +++ b/app/src/ai/runtime/rig_request.rs @@ -27,7 +27,6 @@ pub(crate) struct PreparedRigTurn { pub task_id: String, pub needs_create_task: bool, pub user_query: Option, - pub todo_items: Option>, pub request: TurnRequest, pub persistent_messages: Vec, pub tool_result_archive: Vec, @@ -234,9 +233,6 @@ fn prepare_rig_turn_for_provider( let needs_create_task = tasks.is_empty(); let user_query = input.iter().find_map(input_user_query); let mode = mode_override.unwrap_or_else(|| request_mode(&input)); - // Only an LLM-authored UpdateTodos message creates the checklist. Ordinary turns - // and orchestration prompts must not receive a fabricated plan. - let todo_items = todo_items_from_tasks(&tasks); let available_tools = match mode { RigRequestMode::Cli => supported_cli_agent_tools, RigRequestMode::CompletedCommandAssessment => Vec::new(), @@ -304,7 +300,6 @@ fn prepare_rig_turn_for_provider( task_id, needs_create_task, user_query, - todo_items, request, persistent_messages, tool_result_archive, @@ -313,29 +308,6 @@ fn prepare_rig_turn_for_provider( } } -// Reuse a plan emitted by the model when the existing transcript contains one. This keeps -// direct-provider projection aligned with UpdateTodos instead of inventing a second plan. -fn todo_items_from_tasks(tasks: &[api::Task]) -> Option> { - let mut items = None; - for task in tasks { - for message in &task.messages { - let Some(api::message::Message::UpdateTodos(update)) = &message.message else { - continue; - }; - match update.operation.as_ref()? { - api::message::update_todos::Operation::CreateTodoList(create) => { - items = Some(create.initial_todos.clone()); - } - api::message::update_todos::Operation::UpdatePendingTodos(update) => { - items = Some(update.updated_pending_todos.clone()); - } - api::message::update_todos::Operation::MarkTodosCompleted(_) => {} - } - } - } - items.filter(|items| !items.is_empty()) -} - fn input_messages( inputs: Vec, tool_results: Vec, @@ -759,6 +731,9 @@ fn build_system_prompt( prompt.push_str( "## Execution Contract\nContinue until the user's requested outcome is complete and validated. Do not stop at an intermediate analysis, plan, status update, or promise of future work, and do not ask the user to say \"continue\". After each tool result, choose and perform the next necessary step. Stop only when the request is fulfilled or a concrete blocker requires user input; identify that blocker explicitly.\n\n", ); + prompt.push_str( + "## Task Tracking\nUse task tracking only for substantial work with multiple concrete steps, after planning has established those steps or when the user provides an actionable plan. Skip it for simple requests, questions, and exploratory planning. Each task must name a specific change or deliverable with a clear completion condition. Do not create generic workflow items such as 'research the request', 'critique the plan', 'execute the plan', or 'verify the result'. Update tasks only when their actual work is complete; a tool call or model turn alone is not evidence of completion.\n\n", + ); let contexts = inputs.iter().filter_map(AIAgentInput::context).flatten(); let mut environment = Vec::new(); let mut request_time = None; @@ -921,7 +896,7 @@ fn build_system_prompt( match mode { RigRequestMode::Normal => {} RigRequestMode::Plan => prompt.push_str( - "## Plan Mode\nInspect and produce an implementation-ready plan. Do not edit files or perform state-changing actions. Research as needed, then finish by calling `create_plan` to write the plan with the built-in planning tools. Include a concise `## Tasks` section in the document with short, specific `- [ ]` items. If a plan document already exists for this task, call `edit_plan` instead. Do not return the plan only as prose, and do not claim completion until the plan tool succeeds. Once the plan is created, avoid asking follow-up questions unless you are genuinely blocked or materially uncertain about the next step.\n\n", + "## Plan Mode\nInspect and produce an implementation-ready plan. Do not edit files or perform state-changing actions. Research as needed, then finish by calling `create_plan` to write the plan with the built-in planning tools. For multi-step implementation work, include a concise `## Tasks` section in the document with short, specific `- [ ]` items. If a plan document already exists for this task, call `edit_plan` instead. Do not return the plan only as prose, and do not claim completion until the plan tool succeeds. Once the plan is created, avoid asking follow-up questions unless you are genuinely blocked or materially uncertain about the next step.\n\n", ), RigRequestMode::Orchestrate => prompt.push_str( "## Orchestration Mode\nDelegate only independent, bounded work where parallelism materially helps, then synthesize the results.\n\n", @@ -964,7 +939,7 @@ fn build_system_prompt( } if tools.iter().any(|tool| tool.name == "create_plan") { prompt.push_str( - "Plan document creation is available through `create_plan`. When the user asks you to create a plan for review, research first as needed, then call `create_plan`; do not merely return the plan as prose or claim that no plan-creation tool is available. Include a concise `## Tasks` section with short, specific `- [ ]` items so the user can track progress. If the plan structure changes materially, invalidate it and create a replacement; status changes should update the existing task list instead. If the user asks to review the plan before implementation, creating the document and presenting it for review is the requested outcome; do not implement it until they approve. Once a plan and task list exist, continue working against them and avoid follow-up questions unless genuinely blocked or materially uncertain.\n", + "Plan document creation is available through `create_plan`. When the user asks you to create a plan for review, research first as needed, then call `create_plan`; do not merely return the plan as prose or claim that no plan-creation tool is available. For multi-step implementation work, include a concise `## Tasks` section with short, specific `- [ ]` items so the user can track progress. If the plan structure changes materially, invalidate it and create a replacement; status changes should update the existing task list instead. If the user asks to review the plan before implementation, creating the document and presenting it for review is the requested outcome; do not implement it until they approve. Once a plan and task list exist, continue working against them and avoid follow-up questions unless genuinely blocked or materially uncertain.\n", ); } } diff --git a/app/src/ai/runtime/rig_request_tests.rs b/app/src/ai/runtime/rig_request_tests.rs index d8e434c4..6012795b 100644 --- a/app/src/ai/runtime/rig_request_tests.rs +++ b/app/src/ai/runtime/rig_request_tests.rs @@ -517,7 +517,9 @@ fn lrc_snapshot_follow_up_uses_the_cli_monitor_prompt_and_tools() { let prompt = prepared.request.system_prompt.expect("system prompt"); assert!(prompt.contains("## Running Command Monitor")); - assert!(prompt.contains("`read_shell_command_output` with a short delay")); + assert!( + prompt.contains("Use the command ID from the tool result for every read/write operation") + ); assert!(prompt.contains("one concise, user-facing sentence")); assert!(prompt.contains("Do not send a text-only progress response")); assert!(prompt.contains("alternate screen containing `(END)` is `less`")); diff --git a/app/src/ai/tool_diagnostics.rs b/app/src/ai/tool_diagnostics.rs index 6e29706e..53a5f45a 100644 --- a/app/src/ai/tool_diagnostics.rs +++ b/app/src/ai/tool_diagnostics.rs @@ -30,17 +30,5 @@ macro_rules! tool_debug { pub(crate) use tool_debug; #[cfg(test)] -mod tests { - use std::ffi::OsStr; - - use super::env_value_is_enabled; - - #[test] - fn diagnostic_env_accepts_only_explicit_true_values() { - assert!(env_value_is_enabled(Some(OsStr::new("1")))); - assert!(env_value_is_enabled(Some(OsStr::new("TRUE")))); - assert!(!env_value_is_enabled(Some(OsStr::new("0")))); - assert!(!env_value_is_enabled(Some(OsStr::new("yes")))); - assert!(!env_value_is_enabled(None)); - } -} +#[path = "tool_diagnostics_tests.rs"] +mod tests; diff --git a/app/src/ai/tool_diagnostics_tests.rs b/app/src/ai/tool_diagnostics_tests.rs new file mode 100644 index 00000000..83516772 --- /dev/null +++ b/app/src/ai/tool_diagnostics_tests.rs @@ -0,0 +1,12 @@ +use std::ffi::OsStr; + +use super::env_value_is_enabled; + +#[test] +fn diagnostic_env_accepts_only_explicit_true_values() { + assert!(env_value_is_enabled(Some(OsStr::new("1")))); + assert!(env_value_is_enabled(Some(OsStr::new("TRUE")))); + assert!(!env_value_is_enabled(Some(OsStr::new("0")))); + assert!(!env_value_is_enabled(Some(OsStr::new("yes")))); + assert!(!env_value_is_enabled(None)); +} diff --git a/app/src/code_review/merge_conflicts.rs b/app/src/code_review/merge_conflicts.rs index de3bb3bd..abaacb8a 100644 --- a/app/src/code_review/merge_conflicts.rs +++ b/app/src/code_review/merge_conflicts.rs @@ -58,7 +58,7 @@ impl ConflictBlock { } #[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub(crate) enum ConflictResolution { +pub enum ConflictResolution { Ours, Theirs, Both, diff --git a/app/src/pane_group/pane/terminal_pane.rs b/app/src/pane_group/pane/terminal_pane.rs index 9bea3c50..6d096a6d 100644 --- a/app/src/pane_group/pane/terminal_pane.rs +++ b/app/src/pane_group/pane/terminal_pane.rs @@ -1742,15 +1742,11 @@ fn launch_local_no_harness_child( }); new_terminal_view.update(ctx, |terminal_view, ctx| { - terminal_view - .ai_controller() - .update(ctx, |controller, ctx| { - controller.send_agent_query_in_conversation( - prompt.clone(), - conversation_id, - ctx, - ); - }); + terminal_view.send_child_agent_query_when_ready( + prompt.clone(), + conversation_id, + ctx, + ); terminal_view.enter_agent_view( None, @@ -1852,11 +1848,7 @@ fn launch_direct_provider_child( }); terminal_view.update(ctx, |terminal_view, ctx| { - terminal_view - .ai_controller() - .update(ctx, |controller, ctx| { - controller.send_agent_query_in_conversation(prompt, conversation_id, ctx); - }); + terminal_view.send_child_agent_query_when_ready(prompt, conversation_id, ctx); terminal_view.enter_agent_view( None, Some(conversation_id), diff --git a/app/src/terminal/view.rs b/app/src/terminal/view.rs index 91846d73..d6799f95 100644 --- a/app/src/terminal/view.rs +++ b/app/src/terminal/view.rs @@ -2610,6 +2610,8 @@ pub struct TerminalView { bootstrap_start: Option, is_login_shell_bootstrapped: bool, + /// A child prompt must wait for the shell's session context to be available. + pending_child_agent_query: Option<(AIConversationId, String)>, /// Set when a pending command is submitted to the shell. Cleared on the /// next `AfterBlockCompleted`, at which point `Event::PendingCommandCompleted` /// is emitted so subscribers know the command has finished. @@ -4362,6 +4364,7 @@ impl TerminalView { last_hover_fragment_boundary: None, bootstrap_start: None, is_login_shell_bootstrapped: false, + pending_child_agent_query: None, awaiting_pending_command_completion: false, pending_command_queue: Default::default(), enter_agent_view_after_pending_commands: false, @@ -11992,6 +11995,10 @@ impl TerminalView { // bootstrap block so the user might be able to see what went wrong. if !self.is_login_shell_bootstrapped { self.show_initialization_block(); + self.fail_pending_child_agent_query( + "Child agent shell exited before initialization completed.".to_string(), + ctx, + ); } if !self.pty_spawn_failed { @@ -13937,6 +13944,9 @@ impl TerminalView { } self.refresh_warp_prompt(ctx); + if let Some((conversation_id, prompt)) = self.pending_child_agent_query.take() { + self.send_child_agent_query_when_ready(prompt, conversation_id, ctx); + } ctx.emit(Event::SessionBootstrapped); } @@ -16195,6 +16205,58 @@ impl TerminalView { pub fn is_login_shell_bootstrapped(&self) -> bool { self.is_login_shell_bootstrapped } + + fn fail_pending_child_agent_query(&mut self, message: String, ctx: &mut ViewContext) { + let Some((conversation_id, _)) = self.pending_child_agent_query.take() else { + return; + }; + let terminal_view_id = ctx.view_id(); + BlocklistAIHistoryModel::handle(ctx).update(ctx, |history, ctx| { + if history + .conversation(&conversation_id) + .is_some_and(|conversation| { + matches!(conversation.status(), ConversationStatus::InProgress) + }) + { + history.update_conversation_status_with_error( + terminal_view_id, + conversation_id, + ConversationStatus::Error, + Some(RenderableAIError::other(message, false)), + ctx, + ); + } + }); + } + + pub(crate) fn send_child_agent_query_when_ready( + &mut self, + prompt: String, + conversation_id: AIConversationId, + ctx: &mut ViewContext, + ) { + let can_start = BlocklistAIHistoryModel::as_ref(ctx) + .conversation(&conversation_id) + .is_some_and(|conversation| { + matches!(conversation.status(), ConversationStatus::InProgress) + }); + if !can_start { + return; + } + if !self.is_login_shell_bootstrapped { + self.pending_child_agent_query = Some((conversation_id, prompt)); + if self.pty_spawn_failed { + self.fail_pending_child_agent_query( + "Child agent shell failed to start.".to_string(), + ctx, + ); + } + return; + } + self.ai_controller.update(ctx, |controller, ctx| { + controller.send_agent_query_in_conversation(prompt, conversation_id, ctx); + }); + } pub fn has_pending_command_or_awaiting_completion(&self, ctx: &AppContext) -> bool { self.awaiting_pending_command_completion || !self.pending_command_queue.is_empty() @@ -26203,6 +26265,10 @@ impl TerminalSurface for TerminalView { #[cfg(feature = "local_tty")] fn on_pty_spawn_failed(&mut self, error: anyhow::Error, ctx: &mut ViewContext) { self.pty_spawn_failed = true; + self.fail_pending_child_agent_query( + format!("Child agent shell failed to start: {error:#}"), + ctx, + ); // Emit before the banner so the terminal driver can cancel its // bootstrap wait immediately, without waiting for the 60 s timeout. let reason = format!("{error:#}"); diff --git a/app/src/terminal/view_tests.rs b/app/src/terminal/view_tests.rs index 2e65b13a..56b69a37 100644 --- a/app/src/terminal/view_tests.rs +++ b/app/src/terminal/view_tests.rs @@ -8230,3 +8230,82 @@ fn cmd_k_in_agent_view_cancels_in_progress_conversation_and_starts_new_one() { }); }) } + +#[test] +fn child_prompt_waits_for_bootstrap_and_does_not_restart_cancelled_child() { + App::test((), |mut app| async move { + initialize_app_for_terminal_view(&mut app); + let terminal = add_window_with_terminal(&mut app, None); + terminal.update(&mut app, |view, ctx| { + let terminal_view_id = ctx.view_id(); + let conversation_id = + BlocklistAIHistoryModel::handle(ctx).update(ctx, |history, ctx| { + history.start_new_conversation(terminal_view_id, true, false, false, ctx) + }); + view.is_login_shell_bootstrapped = false; + view.send_child_agent_query_when_ready( + "child prompt".to_string(), + conversation_id, + ctx, + ); + assert_eq!( + view.pending_child_agent_query, + Some((conversation_id, "child prompt".to_string())) + ); + assert!(BlocklistAIHistoryModel::as_ref(ctx) + .conversation(&conversation_id) + .unwrap() + .latest_exchange() + .is_none()); + BlocklistAIHistoryModel::handle(ctx).update(ctx, |history, ctx| { + history.update_conversation_status( + terminal_view_id, + conversation_id, + ConversationStatus::Cancelled, + ctx, + ); + }); + view.is_login_shell_bootstrapped = true; + let (conversation_id, prompt) = view.pending_child_agent_query.take().unwrap(); + view.send_child_agent_query_when_ready(prompt, conversation_id, ctx); + assert!(view.pending_child_agent_query.is_none()); + assert_eq!( + BlocklistAIHistoryModel::as_ref(ctx) + .conversation(&conversation_id) + .unwrap() + .status(), + &ConversationStatus::Cancelled + ); + }); + }); +} + +#[test] +fn child_shell_startup_failure_finishes_the_waiting_conversation() { + App::test((), |mut app| async move { + initialize_app_for_terminal_view(&mut app); + let terminal = add_window_with_terminal(&mut app, None); + terminal.update(&mut app, |view, ctx| { + let terminal_view_id = ctx.view_id(); + let conversation_id = + BlocklistAIHistoryModel::handle(ctx).update(ctx, |history, ctx| { + history.start_new_conversation(terminal_view_id, true, false, false, ctx) + }); + view.is_login_shell_bootstrapped = false; + view.send_child_agent_query_when_ready( + "child prompt".to_string(), + conversation_id, + ctx, + ); + view.fail_pending_child_agent_query("shell failed".to_string(), ctx); + assert!(view.pending_child_agent_query.is_none()); + assert_eq!( + BlocklistAIHistoryModel::as_ref(ctx) + .conversation(&conversation_id) + .unwrap() + .status(), + &ConversationStatus::Error + ); + }); + }); +} diff --git a/crates/ai/src/index/full_source_code_embedding/codebase_index.rs b/crates/ai/src/index/full_source_code_embedding/codebase_index.rs index c3e43485..e55d117e 100644 --- a/crates/ai/src/index/full_source_code_embedding/codebase_index.rs +++ b/crates/ai/src/index/full_source_code_embedding/codebase_index.rs @@ -386,7 +386,6 @@ impl CodebaseIndex { repository: ModelHandle, store_client: Arc, embedding_config: EmbeddingConfig, - max_files_repo_limit: usize, embedding_generation_batch_size: usize, ctx: &mut ModelContext, ) -> Self { @@ -398,7 +397,7 @@ impl CodebaseIndex { ctx, ); - if let Err(err) = index.build_and_sync_from_repository_root(max_files_repo_limit, ctx) { + if let Err(err) = index.build_and_sync_from_repository_root(ctx) { safe_error!( safe: ("Failed to build index: {err:?}"), full: ("Failed to build index at root {}: {err:?}", index.repo_path.display()) @@ -803,7 +802,6 @@ impl CodebaseIndex { #[cfg(feature = "local_fs")] fn build_and_sync_from_repository_root( &mut self, - max_files_repo_limit: usize, ctx: &mut ModelContext<'_, Self>, ) -> Result<(), Error> { let repo_path = self.repo_path.clone(); @@ -1127,13 +1125,9 @@ impl CodebaseIndex { /// Performs a full reparse of the merkle tree, followed by a full server sync. This force evicts the /// existing merkle tree state. #[cfg(feature = "local_fs")] - pub(super) fn full_sync_index( - &mut self, - max_files_repo_limit: usize, - ctx: &mut ModelContext, - ) -> Result<(), Error> { + pub(super) fn full_sync_index(&mut self, ctx: &mut ModelContext) -> Result<(), Error> { self.update_tree_sync_state(TreeSourceSyncState::unsynced(), ctx); - self.build_and_sync_from_repository_root(max_files_repo_limit, ctx) + self.build_and_sync_from_repository_root(ctx) } /// Attempt to perform a full SERVER sync on the current index. We only proceed with the sync if there is @@ -1328,7 +1322,6 @@ impl CodebaseIndex { #[cfg(not(feature = "local_fs"))] pub fn build_and_sync_from_repository_root( &mut self, - _max_num_files_limit: usize, _ctx: &mut ModelContext<'_, Self>, ) -> Result<(), Error> { Err(Error::UnsupportedPlatform) @@ -1705,7 +1698,6 @@ impl CodebaseIndex { store_client: Arc, embedding_config: EmbeddingConfig, snapshot_bytes: Vec, - max_files_repo_limit: usize, embedding_generation_batch_size: usize, ctx: &mut ModelContext, ) -> Result { @@ -1716,7 +1708,7 @@ impl CodebaseIndex { embedding_generation_batch_size, ctx, ); - index.rebuild_and_sync_from_snapshot(snapshot_bytes, max_files_repo_limit, ctx); + index.rebuild_and_sync_from_snapshot(snapshot_bytes, ctx); Ok(index) } @@ -1724,7 +1716,6 @@ impl CodebaseIndex { fn rebuild_and_sync_from_snapshot( &mut self, snapshot_bytes: Vec, - max_files_repo_limit: usize, ctx: &mut ModelContext<'_, Self>, ) { let repo_metadata = RepoMetadata { @@ -1756,7 +1747,6 @@ impl CodebaseIndex { let (changed_files, gitignores) = Self::diff_filesystem_with_tree( repo_path.clone(), &tree, - max_files_repo_limit, ) .map_err(SnapshotLoadError::DiffFailed)?; @@ -1922,7 +1912,6 @@ impl CodebaseIndex { fn diff_filesystem_with_tree( repo_path: PathBuf, tree: &MerkleTree, - max_files_repo_limit: usize, ) -> Result<(ChangedFiles, Vec), Error> { let mut gitignores = Self::construct_initial_ignores(&repo_path); diff --git a/crates/ai/src/index/full_source_code_embedding/manager.rs b/crates/ai/src/index/full_source_code_embedding/manager.rs index 9cc05b8a..6bb8e953 100644 --- a/crates/ai/src/index/full_source_code_embedding/manager.rs +++ b/crates/ai/src/index/full_source_code_embedding/manager.rs @@ -723,7 +723,7 @@ impl CodebaseIndexManager { )) ) { index.update(_ctx, |code_index, ctx| { - let _ = code_index.full_sync_index(self.max_files_repo_limit, ctx); + let _ = code_index.full_sync_index(ctx); }); } } @@ -940,7 +940,6 @@ impl CodebaseIndexManager { let index = Self::build_and_sync_codebase_index_internal( self.store_client.clone(), handle, - self.max_files_repo_limit, self.embedding_generation_batch_size, #[cfg(feature = "local_fs")] snapshot_storage, @@ -967,7 +966,6 @@ impl CodebaseIndexManager { fn build_and_sync_codebase_index_internal( store_client: Arc, repository: ModelHandle, - max_files_repo_limit: usize, embedding_generation_batch_size: usize, #[cfg(feature = "local_fs")] snapshot_storage: Option, ctx: &mut ModelContext, @@ -991,7 +989,6 @@ impl CodebaseIndexManager { store_client.clone(), snapshot_storage.path(), repository.clone(), - max_files_repo_limit, embedding_generation_batch_size, ctx, ) { @@ -1020,7 +1017,6 @@ impl CodebaseIndexManager { repository, store_client, EmbeddingConfig::default(), - max_files_repo_limit, embedding_generation_batch_size, ctx, ) @@ -1185,7 +1181,7 @@ impl CodebaseIndexManager { }; codebase_index.update(ctx, |index, ctx| { - let _ = index.full_sync_index(self.max_files_repo_limit, ctx); + let _ = index.full_sync_index(ctx); }) } diff --git a/crates/ai/src/index/full_source_code_embedding/snapshot.rs b/crates/ai/src/index/full_source_code_embedding/snapshot.rs index 5821474c..d29e1acb 100644 --- a/crates/ai/src/index/full_source_code_embedding/snapshot.rs +++ b/crates/ai/src/index/full_source_code_embedding/snapshot.rs @@ -157,7 +157,6 @@ pub(super) fn read_snapshot( store_client: Arc, snapshot_dir: &Path, repository: ModelHandle, - max_files_repo_limit: usize, embedding_generation_batch_size: usize, ctx: &mut ModelContext, ) -> anyhow::Result { @@ -171,7 +170,6 @@ pub(super) fn read_snapshot( store_client.clone(), EmbeddingConfig::default(), snapshot_bytes, - max_files_repo_limit, embedding_generation_batch_size, ctx, ); diff --git a/crates/ai/src/index/local_project_index/mod.rs b/crates/ai/src/index/local_project_index/mod.rs index 5c267db5..4f0f34b9 100644 --- a/crates/ai/src/index/local_project_index/mod.rs +++ b/crates/ai/src/index/local_project_index/mod.rs @@ -674,6 +674,7 @@ async fn acquire_repository_lock(storage_root: &Path, root_path: &Path) -> Resul .read(true) .write(true) .create(true) + .truncate(false) .open(&lock_path) .with_context(|| format!("Failed to open local index lock {}", lock_path.display()))?; loop { diff --git a/crates/galaxy_agent_core/src/provider_run.rs b/crates/galaxy_agent_core/src/provider_run.rs index b76fc4b9..e773c0d1 100644 --- a/crates/galaxy_agent_core/src/provider_run.rs +++ b/crates/galaxy_agent_core/src/provider_run.rs @@ -1044,6 +1044,9 @@ impl ProviderRun { call.state = PendingToolCallState::PermissionPending { request }; Ok(()) } + PendingToolCallState::PermissionPending { request: pending } if pending == &request => { + Ok(()) + } PendingToolCallState::Resolved { .. } => { Err(ProviderRunProtocolError::DuplicateToolUpdate { call_id: call.call.id.clone(), @@ -1068,6 +1071,12 @@ impl ProviderRun { let call = self.pending_tool_call_mut(work_id, call_id)?; let pending_request_id = match &call.state { PendingToolCallState::PermissionPending { request } => request.id.clone(), + PendingToolCallState::Approved { + request_id: approved_request_id, + decision: approved_decision, + } if approved_request_id == request_id && approved_decision == &decision => { + return Ok(()); + } PendingToolCallState::Resolved { .. } => { return Err(ProviderRunProtocolError::DuplicateToolUpdate { call_id: call.call.id.clone(), diff --git a/crates/galaxy_agent_core/src/provider_run_tests.rs b/crates/galaxy_agent_core/src/provider_run_tests.rs index 854894da..c2df7b1b 100644 --- a/crates/galaxy_agent_core/src/provider_run_tests.rs +++ b/crates/galaxy_agent_core/src/provider_run_tests.rs @@ -528,6 +528,61 @@ fn exact_batch_submission_rejects_missing_duplicate_and_unknown_results_without_ assert_eq!(run, before); } +#[test] +fn repeated_permission_notifications_preserve_pending_sibling_tools() { + let mut run = run(); + let batch = accept_tool_turn( + &mut run, + tool_turn( + vec![ + tool_call("first", "run_shell_command"), + tool_call("second", "run_shell_command"), + ], + &["run_shell_command"], + ), + ); + let request = PermissionRequest { + id: "permission-first".to_string(), + call_id: "first".to_string(), + kind: PermissionKind::Execute, + reason: Some("run a command".to_string()), + }; + run.request_tool_permission(&batch.work_id, request.clone()) + .unwrap(); + run.request_tool_permission(&batch.work_id, request.clone()) + .unwrap(); + let mut conflicting = request; + conflicting.id = "different-permission".to_string(); + assert!( + run.request_tool_permission(&batch.work_id, conflicting) + .is_err() + ); + for _ in 0..2 { + run.resolve_tool_permission( + &batch.work_id, + "first", + "permission-first", + PermissionDecision::AllowOnce, + ) + .unwrap(); + } + assert!( + run.resolve_tool_permission( + &batch.work_id, + "first", + "permission-first", + PermissionDecision::AlwaysAllow, + ) + .is_err() + ); + run.start_tool(&batch.work_id, "first").unwrap(); + let ProviderRunState::AwaitingTools { batch } = run.state() else { + panic!("permission notifications must leave the batch pending"); + }; + assert_eq!(batch.calls[0].state, PendingToolCallState::Executing); + assert_eq!(batch.calls[1].state, PendingToolCallState::Proposed); +} + #[test] fn permission_denial_becomes_one_correlated_result() { let mut run = run(); diff --git a/crates/galaxy_agent_rig/src/chatgpt.rs b/crates/galaxy_agent_rig/src/chatgpt.rs index 7c644b64..f4af1f7d 100644 --- a/crates/galaxy_agent_rig/src/chatgpt.rs +++ b/crates/galaxy_agent_rig/src/chatgpt.rs @@ -144,122 +144,5 @@ impl AgentRuntime for ChatGPTSubscriptionRuntime { } #[cfg(test)] -mod tests { - use rig_core::client::CompletionClient; - use rig_core::completion::{AssistantContent, CompletionModel, Message}; - use rig_core::message::{ToolResultContent, UserContent}; - use rig_core::providers::chatgpt::ChatGPTAuth; - use rig_core::test_utils::RecordingHttpClient; - - use super::*; - - const COMPLETED_RESPONSE: &str = r#"data: {"type":"response.output_text.delta","delta":"ok"} -data: {"type":"response.completed","response":{"id":"resp_test","object":"response","created_at":1,"status":"completed","error":null,"incomplete_details":null,"instructions":null,"max_output_tokens":null,"model":"gpt-5.3-codex","usage":{"input_tokens":1,"input_tokens_details":{"cached_tokens":0},"output_tokens":1,"output_tokens_details":{"reasoning_tokens":0},"total_tokens":2},"output":[{"type":"message","id":"msg_test","status":"completed","role":"assistant","content":[{"type":"output_text","annotations":[],"text":"ok"}]}],"tools":[]}} -data: [DONE]"#; - - #[test] - fn runtime_descriptor_identifies_chatgpt_subscription() { - let runtime = ChatGPTSubscriptionRuntime::new(ChatGPTSubscriptionRuntimeConfig { - model: "gpt-5.3-codex".to_string(), - reasoning_effort: Some("high".to_string()), - max_output_tokens: None, - auth_file: None, - }); - - assert_eq!( - runtime.descriptor().id, - "rig-chatgpt-subscription:gpt-5.3-codex" - ); - } - - #[test] - fn reasoning_effort_is_encoded_for_the_responses_request() { - let request = TurnRequest::new( - "gpt-5.4".to_string(), - vec![ConversationMessage { - role: MessageRole::User, - content: MessageContent::Text("hello".to_string()), - }], - ); - let request = build_completion_request( - request, - None, - true, - false, - reasoning_additional_params(Some("xhigh")), - ) - .expect("request should convert"); - - assert_eq!( - request.additional_params, - reasoning_additional_params(Some("xhigh")) - ); - - assert_eq!( - reasoning_additional_params(Some("ultra")), - Some(serde_json::json!({ - "reasoning": { "effort": "max" } - })) - ); - } - - #[tokio::test] - async fn chatgpt_follow_up_request_preserves_responses_call_ids() { - let http_client = RecordingHttpClient::new(COMPLETED_RESPONSE); - let client = chatgpt::Client::builder() - .api_key(ChatGPTAuth::AccessToken { - access_token: "test-token".to_string(), - account_id: None, - }) - .http_client(http_client.clone()) - .build() - .expect("client should build"); - let model = client.completion_model("gpt-5.3-codex"); - let assistant_tool_call = AssistantContent::tool_call_with_call_id( - "fc_native_1", - "call_native_1".to_string(), - "read_files", - serde_json::json!({"files": ["Cargo.toml"]}), - ); - let tool_result = UserContent::tool_result_with_call_id( - "fc_native_1", - "call_native_1".to_string(), - rig_core::OneOrMany::one(ToolResultContent::text("contents")), - ); - let chat_history = rig_core::OneOrMany::many(vec![ - Message::Assistant { - id: None, - content: rig_core::OneOrMany::one(assistant_tool_call), - }, - Message::User { - content: rig_core::OneOrMany::one(tool_result), - }, - Message::user("Continue."), - ]) - .expect("history should contain messages"); - - model - .completion(rig_core::completion::CompletionRequest { - model: Some("gpt-5.3-codex".to_string()), - preamble: None, - chat_history, - documents: Vec::new(), - tools: Vec::new(), - temperature: None, - max_tokens: None, - tool_choice: None, - additional_params: None, - output_schema: None, - record_telemetry_content: false, - }) - .await - .expect("request should reach the mocked provider"); - - let requests = http_client.requests(); - assert_eq!(requests.len(), 1); - let body: serde_json::Value = serde_json::from_slice(&requests[0].body).unwrap(); - let input = body["input"].as_array().expect("input should be an array"); - assert_eq!(input[0]["call_id"], "call_native_1"); - assert_eq!(input[1]["call_id"], "call_native_1"); - } -} +#[path = "chatgpt_tests.rs"] +mod tests; diff --git a/crates/galaxy_agent_rig/src/chatgpt_tests.rs b/crates/galaxy_agent_rig/src/chatgpt_tests.rs new file mode 100644 index 00000000..c176c402 --- /dev/null +++ b/crates/galaxy_agent_rig/src/chatgpt_tests.rs @@ -0,0 +1,117 @@ +use rig_core::client::CompletionClient; +use rig_core::completion::{AssistantContent, CompletionModel, Message}; +use rig_core::message::{ToolResultContent, UserContent}; +use rig_core::providers::chatgpt::ChatGPTAuth; +use rig_core::test_utils::RecordingHttpClient; + +use super::*; + +const COMPLETED_RESPONSE: &str = r#"data: {"type":"response.output_text.delta","delta":"ok"} +data: {"type":"response.completed","response":{"id":"resp_test","object":"response","created_at":1,"status":"completed","error":null,"incomplete_details":null,"instructions":null,"max_output_tokens":null,"model":"gpt-5.3-codex","usage":{"input_tokens":1,"input_tokens_details":{"cached_tokens":0},"output_tokens":1,"output_tokens_details":{"reasoning_tokens":0},"total_tokens":2},"output":[{"type":"message","id":"msg_test","status":"completed","role":"assistant","content":[{"type":"output_text","annotations":[],"text":"ok"}]}],"tools":[]}} +data: [DONE]"#; + +#[test] +fn runtime_descriptor_identifies_chatgpt_subscription() { + let runtime = ChatGPTSubscriptionRuntime::new(ChatGPTSubscriptionRuntimeConfig { + model: "gpt-5.3-codex".to_string(), + reasoning_effort: Some("high".to_string()), + max_output_tokens: None, + auth_file: None, + }); + + assert_eq!( + runtime.descriptor().id, + "rig-chatgpt-subscription:gpt-5.3-codex" + ); +} + +#[test] +fn reasoning_effort_is_encoded_for_the_responses_request() { + let request = TurnRequest::new( + "gpt-5.4".to_string(), + vec![ConversationMessage { + role: MessageRole::User, + content: MessageContent::Text("hello".to_string()), + }], + ); + let request = build_completion_request( + request, + None, + true, + false, + reasoning_additional_params(Some("xhigh")), + ) + .expect("request should convert"); + + assert_eq!( + request.additional_params, + reasoning_additional_params(Some("xhigh")) + ); + + assert_eq!( + reasoning_additional_params(Some("ultra")), + Some(serde_json::json!({ + "reasoning": { "effort": "max" } + })) + ); +} + +#[tokio::test] +async fn chatgpt_follow_up_request_preserves_responses_call_ids() { + let http_client = RecordingHttpClient::new(COMPLETED_RESPONSE); + let client = chatgpt::Client::builder() + .api_key(ChatGPTAuth::AccessToken { + access_token: "test-token".to_string(), + account_id: None, + }) + .http_client(http_client.clone()) + .build() + .expect("client should build"); + let model = client.completion_model("gpt-5.3-codex"); + let assistant_tool_call = AssistantContent::tool_call_with_call_id( + "fc_native_1", + "call_native_1".to_string(), + "read_files", + serde_json::json!({"files": ["Cargo.toml"]}), + ); + let tool_result = UserContent::tool_result_with_call_id( + "fc_native_1", + "call_native_1".to_string(), + rig_core::OneOrMany::one(ToolResultContent::text("contents")), + ); + let chat_history = rig_core::OneOrMany::many(vec![ + Message::Assistant { + id: None, + content: rig_core::OneOrMany::one(assistant_tool_call), + }, + Message::User { + content: rig_core::OneOrMany::one(tool_result), + }, + Message::user("Continue."), + ]) + .expect("history should contain messages"); + + model + .completion(rig_core::completion::CompletionRequest { + model: Some("gpt-5.3-codex".to_string()), + preamble: None, + chat_history, + documents: Vec::new(), + tools: Vec::new(), + temperature: None, + max_tokens: None, + tool_choice: None, + additional_params: None, + output_schema: None, + record_telemetry_content: false, + }) + .await + .expect("request should reach the mocked provider"); + + let requests = http_client.requests(); + assert_eq!(requests.len(), 1); + let body: serde_json::Value = serde_json::from_slice(&requests[0].body).unwrap(); + let input = body["input"].as_array().expect("input should be an array"); + assert_eq!(input[0]["call_id"], "call_native_1"); + assert_eq!(input[1]["call_id"], "call_native_1"); +} diff --git a/crates/galaxy_agent_rig/src/native.rs b/crates/galaxy_agent_rig/src/native.rs index f20ab763..ab62c1d8 100644 --- a/crates/galaxy_agent_rig/src/native.rs +++ b/crates/galaxy_agent_rig/src/native.rs @@ -243,14 +243,5 @@ fn native_descriptor(provider: &str, model: &str) -> RuntimeDescriptor { } #[cfg(test)] -mod tests { - use super::native_descriptor; - - #[test] - fn native_descriptors_are_provider_specific() { - let descriptor = native_descriptor("anthropic", "claude-sonnet"); - - assert_eq!(descriptor.id, "rig-anthropic:claude-sonnet"); - assert_eq!(descriptor.display_name, "Rig / anthropic / claude-sonnet"); - } -} +#[path = "native_tests.rs"] +mod tests; diff --git a/crates/galaxy_agent_rig/src/native_tests.rs b/crates/galaxy_agent_rig/src/native_tests.rs new file mode 100644 index 00000000..9e497680 --- /dev/null +++ b/crates/galaxy_agent_rig/src/native_tests.rs @@ -0,0 +1,9 @@ +use super::native_descriptor; + +#[test] +fn native_descriptors_are_provider_specific() { + let descriptor = native_descriptor("anthropic", "claude-sonnet"); + + assert_eq!(descriptor.id, "rig-anthropic:claude-sonnet"); + assert_eq!(descriptor.display_name, "Rig / anthropic / claude-sonnet"); +} diff --git a/crates/galaxy_agent_rig/src/stream.rs b/crates/galaxy_agent_rig/src/stream.rs index d28236cd..3379040c 100644 --- a/crates/galaxy_agent_rig/src/stream.rs +++ b/crates/galaxy_agent_rig/src/stream.rs @@ -519,122 +519,8 @@ fn text_indicates_authentication_failure(text: &str) -> bool { } #[cfg(test)] -mod tests { - use galaxy_agent_core::{AgentErrorKind, StopReason}; - use rig_core::completion::CompletionError; - - use super::{ - completion_error_stop_reason, domain_tool_call, is_keepalive_event, map_completion_error, - }; - - #[test] - fn domain_tool_call_prefers_responses_call_id() { - let tool_call = rig_core::message::ToolCall::new( - "fc_item_123".to_string(), - rig_core::message::ToolFunction { - name: "read_files".to_string(), - arguments: serde_json::json!({"files": ["Cargo.toml"]}), - }, - ) - .with_call_id("call_123".to_string()); - - let call = domain_tool_call(tool_call); - - assert_eq!(call.id, "call_123"); - assert_eq!(call.name, "read_files"); - } - - #[test] - fn domain_tool_call_falls_back_to_wire_id_without_responses_call_id() { - let tool_call = rig_core::message::ToolCall::new( - "fc_item_123".to_string(), - rig_core::message::ToolFunction { - name: "read_files".to_string(), - arguments: serde_json::json!({"files": ["Cargo.toml"]}), - }, - ); - - let call = domain_tool_call(tool_call); - - assert_eq!(call.id, "fc_item_123"); - assert_eq!(call.name, "read_files"); - } - - #[test] - fn provider_context_window_error_maps_to_semantic_stop_reason_and_kind() { - let status = rig_core::http_client::Response::builder() - .status(400) - .body(()) - .unwrap() - .status(); - let error = CompletionError::from_http_response( - status, - r#"{"error":{"message":"Your input exceeds the context window of this model. Please adjust your input and try again.","code":"400"}}"#, - ); - - assert_eq!( - completion_error_stop_reason(&error), - Some(StopReason::ContextWindowExceeded) - ); - let mapped = map_completion_error(error); - assert_eq!(mapped.kind, AgentErrorKind::ContextWindowExceeded); - assert!(!mapped.recoverable); - } - - #[test] - fn provider_context_length_string_maps_to_semantic_stop_reason() { - let error = CompletionError::ProviderError( - "context_length_exceeded: maximum context length is 128000 tokens".to_string(), - ); - - assert_eq!( - completion_error_stop_reason(&error), - Some(StopReason::ContextWindowExceeded) - ); - } - - #[test] - fn provider_keepalive_event_is_a_transport_heartbeat() { - assert!(is_keepalive_event(&serde_json::json!({ - "type": "keepalive", - "sequence_number": 3, - }))); - assert!(!is_keepalive_event(&serde_json::json!({ - "type": "unsupported", - }))); - } - - #[test] - fn provider_server_error_is_recoverable() { - let status = rig_core::http_client::Response::builder() - .status(503) - .body(()) - .unwrap() - .status(); - let error = CompletionError::from_http_response( - status, - r#"{"error":{"message":"Service temporarily unavailable"}}"#, - ); - - let mapped = map_completion_error(error); - assert_eq!(mapped.kind, AgentErrorKind::Provider); - assert!(mapped.recoverable); - } - - #[test] - fn flattened_bedrock_credential_errors_are_authentication_failures() { - for message in [ - r#"{"__type":"ExpiredTokenException","message":"The security token included in the request is expired"}"#, - "UnrecognizedClientException: The security token included in the request is invalid", - "AccessDeniedException: not authorized to perform bedrock:ConverseStream", - ] { - let mapped = map_completion_error(CompletionError::ProviderError(message.to_string())); - assert_eq!(mapped.kind, AgentErrorKind::Authentication); - assert!(!mapped.recoverable); - assert!(mapped.user_message.is_some()); - } - } -} +#[path = "stream_unit_tests.rs"] +mod tests; #[cfg(test)] #[path = "stream_tests.rs"] diff --git a/crates/galaxy_agent_rig/src/stream_unit_tests.rs b/crates/galaxy_agent_rig/src/stream_unit_tests.rs new file mode 100644 index 00000000..ac737561 --- /dev/null +++ b/crates/galaxy_agent_rig/src/stream_unit_tests.rs @@ -0,0 +1,114 @@ +use galaxy_agent_core::{AgentErrorKind, StopReason}; +use rig_core::completion::CompletionError; + +use super::{ + completion_error_stop_reason, domain_tool_call, is_keepalive_event, map_completion_error, +}; + +#[test] +fn domain_tool_call_prefers_responses_call_id() { + let tool_call = rig_core::message::ToolCall::new( + "fc_item_123".to_string(), + rig_core::message::ToolFunction { + name: "read_files".to_string(), + arguments: serde_json::json!({"files": ["Cargo.toml"]}), + }, + ) + .with_call_id("call_123".to_string()); + + let call = domain_tool_call(tool_call); + + assert_eq!(call.id, "call_123"); + assert_eq!(call.name, "read_files"); +} + +#[test] +fn domain_tool_call_falls_back_to_wire_id_without_responses_call_id() { + let tool_call = rig_core::message::ToolCall::new( + "fc_item_123".to_string(), + rig_core::message::ToolFunction { + name: "read_files".to_string(), + arguments: serde_json::json!({"files": ["Cargo.toml"]}), + }, + ); + + let call = domain_tool_call(tool_call); + + assert_eq!(call.id, "fc_item_123"); + assert_eq!(call.name, "read_files"); +} + +#[test] +fn provider_context_window_error_maps_to_semantic_stop_reason_and_kind() { + let status = rig_core::http_client::Response::builder() + .status(400) + .body(()) + .unwrap() + .status(); + let error = CompletionError::from_http_response( + status, + r#"{"error":{"message":"Your input exceeds the context window of this model. Please adjust your input and try again.","code":"400"}}"#, + ); + + assert_eq!( + completion_error_stop_reason(&error), + Some(StopReason::ContextWindowExceeded) + ); + let mapped = map_completion_error(error); + assert_eq!(mapped.kind, AgentErrorKind::ContextWindowExceeded); + assert!(!mapped.recoverable); +} + +#[test] +fn provider_context_length_string_maps_to_semantic_stop_reason() { + let error = CompletionError::ProviderError( + "context_length_exceeded: maximum context length is 128000 tokens".to_string(), + ); + + assert_eq!( + completion_error_stop_reason(&error), + Some(StopReason::ContextWindowExceeded) + ); +} + +#[test] +fn provider_keepalive_event_is_a_transport_heartbeat() { + assert!(is_keepalive_event(&serde_json::json!({ + "type": "keepalive", + "sequence_number": 3, + }))); + assert!(!is_keepalive_event(&serde_json::json!({ + "type": "unsupported", + }))); +} + +#[test] +fn provider_server_error_is_recoverable() { + let status = rig_core::http_client::Response::builder() + .status(503) + .body(()) + .unwrap() + .status(); + let error = CompletionError::from_http_response( + status, + r#"{"error":{"message":"Service temporarily unavailable"}}"#, + ); + + let mapped = map_completion_error(error); + assert_eq!(mapped.kind, AgentErrorKind::Provider); + assert!(mapped.recoverable); +} + +#[test] +fn flattened_bedrock_credential_errors_are_authentication_failures() { + for message in [ + r#"{"__type":"ExpiredTokenException","message":"The security token included in the request is expired"}"#, + "UnrecognizedClientException: The security token included in the request is invalid", + "AccessDeniedException: not authorized to perform bedrock:ConverseStream", + ] { + let mapped = map_completion_error(CompletionError::ProviderError(message.to_string())); + assert_eq!(mapped.kind, AgentErrorKind::Authentication); + assert!(!mapped.recoverable); + assert!(mapped.user_message.is_some()); + } +}