diff --git a/AGENTS.md b/AGENTS.md index 0ed85621..0874bb3f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -250,6 +250,7 @@ Key invariants: - Safe inspection and output polling do not add risk-review model calls. A narrow shell fast path accepts validated `pwd`, `ls`, Git version, and hardened Git status/diff forms; shell expansion, chaining, redirects, unknown flags and unhardened Git commands still require review. Explicit restrictions are checked before fast approval. Galaxy Decides skips user-question pauses. External ACP/CLI agents retain their own protocol permission flows; unstructured ACP requests must not grant Galaxy terminal mutations. - Allow All bypasses independent model review and human approval for Galaxy-owned tools, while explicit command/MCP denylists, protected paths, workspace restrictions, command ownership, and child delegation boundaries remain enforced. +- Ask First and Allow All expose `ask_user_question` for clarifying missing information; Allow All still bypasses action-approval panes. Galaxy Decides and Read Only omit clarification pauses. CLI monitoring and tool-free assessments must not gain question tools through an explicit tool override. - Profiles persist `command_allowlist` regular expressions. A matching complete submitted command bypasses permission review in every mode, including Read Only, only after explicit restrictions are checked; denylist matching uses the shell command decomposition as well. Profile permission-mode changes preserve the explicit list but clear remembered command digests. - Profiles persist `orchestration_providers`: absent/null includes all providers (including future additions); an explicit empty list permits none. Entries use configured provider names. Orchestration tool descriptions and pickers filter models, and RunAgents validates per-child/default model selection again at launch and recovery. Restricted profiles cannot use custom routers or external harnesses to bypass provider selection. diff --git a/app/src/ai/agent/api/impl.rs b/app/src/ai/agent/api/impl.rs index df4c1cb2..4312bb43 100644 --- a/app/src/ai/agent/api/impl.rs +++ b/app/src/ai/agent/api/impl.rs @@ -28,6 +28,16 @@ pub(crate) fn prepare_direct_provider_params( .clone() .unwrap_or_else(|| get_supported_tools(params)); remove_orchestration_tools_if_disabled(&mut supported_tools, params.orchestration_enabled); + let ask_user_question_enabled = FeatureFlag::AskUserQuestion.is_enabled() + && params.ask_user_question_enabled + && matches!( + params.permission_mode, + crate::ai::execution_profiles::PermissionMode::AskFirst + | crate::ai::execution_profiles::PermissionMode::AllowAll + ); + supported_tools.retain(|tool| { + *tool != api::ToolType::AskUserQuestion || ask_user_question_enabled + }); // Computer use controls this machine, even when the terminal is connected elsewhere. // Only expose the initial request here; the dedicated profile owns subsequent actions. supported_tools.retain(|tool| *tool != api::ToolType::UseComputer @@ -39,6 +49,7 @@ pub(crate) fn prepare_direct_provider_params( params.orchestration_enabled, ); supported_cli_agent_tools.retain(|tool| !matches!(tool, api::ToolType::UseComputer | api::ToolType::RequestComputerUse)); + supported_cli_agent_tools.retain(|tool| *tool != api::ToolType::AskUserQuestion); if params.permission_mode == crate::ai::execution_profiles::PermissionMode::ReadOnly { let permitted = |tool: &api::ToolType| is_read_only_tool(tool) || (!params.command_allowlist.is_empty() && matches!(tool, api::ToolType::RunShellCommand | api::ToolType::ReadShellCommandOutput | api::ToolType::WriteToLongRunningShellCommand)); diff --git a/app/src/ai/agent/api/impl_tests.rs b/app/src/ai/agent/api/impl_tests.rs index 2bfef573..6882499d 100644 --- a/app/src/ai/agent/api/impl_tests.rs +++ b/app/src/ai/agent/api/impl_tests.rs @@ -191,9 +191,7 @@ fn supported_tools_omits_ask_user_question_when_disabled() { #[test] fn supported_tools_includes_ask_user_question_when_enabled_and_feature_flag_is_enabled() { - if !FeatureFlag::AskUserQuestion.is_enabled() { - return; - } + let _flag = FeatureFlag::AskUserQuestion.override_enabled(true); let params = request_params_with_ask_user_question_enabled(true); let supported_tools = get_supported_tools(¶ms); @@ -201,6 +199,47 @@ fn supported_tools_includes_ask_user_question_when_enabled_and_feature_flag_is_e assert!(supported_tools.contains(&api::ToolType::AskUserQuestion)); } +#[test] +fn question_overrides_respect_mode_capability_and_feature_without_enabling_cli_pauses() { + use crate::ai::execution_profiles::PermissionMode; + + for (mode, enabled, feature_enabled, expected) in [ + (PermissionMode::AskFirst, true, true, true), + (PermissionMode::AllowAll, true, true, true), + (PermissionMode::AllowAll, false, true, false), + (PermissionMode::AllowAll, true, false, false), + (PermissionMode::GalaxyDecides, true, true, false), + (PermissionMode::ReadOnly, true, true, false), + ] { + let _flag = FeatureFlag::AskUserQuestion.override_enabled(feature_enabled); + let mut params = request_params_with_ask_user_question_enabled(enabled); + params.permission_mode = mode; + params.supported_tools_override = Some(vec![api::ToolType::AskUserQuestion]); + + let (normal, cli) = super::prepare_direct_provider_params(&mut params); + + assert_eq!( + normal.contains(&api::ToolType::AskUserQuestion), + expected, + "{mode:?}" + ); + assert!(!cli.contains(&api::ToolType::AskUserQuestion), "{mode:?}"); + } +} + +#[test] +fn question_availability_does_not_expand_empty_tool_overrides() { + let _flag = FeatureFlag::AskUserQuestion.override_enabled(true); + let mut params = request_params_with_ask_user_question_enabled(true); + params.permission_mode = crate::ai::execution_profiles::PermissionMode::AllowAll; + params.supported_tools_override = Some(Vec::new()); + + let (normal, cli) = super::prepare_direct_provider_params(&mut params); + + assert!(normal.is_empty()); + assert!(cli.is_empty()); +} + #[test] fn remote_supported_tools_include_search_codebase_when_connected_and_feature_flag_is_enabled() { let _flag = FeatureFlag::RemoteCodebaseIndexing.override_enabled(true); diff --git a/app/src/ai/blocklist/block.rs b/app/src/ai/blocklist/block.rs index f60bfc74..8b4447c5 100644 --- a/app/src/ai/blocklist/block.rs +++ b/app/src/ai/blocklist/block.rs @@ -3998,12 +3998,7 @@ impl AIBlock { }); self.ask_user_question_view = Some(view.clone()); - if self - .action_model - .as_ref(ctx) - .get_action_status(self.client_ids.conversation_id, action_id) - .is_some_and(|status| status.is_blocked()) - { + if view.as_ref(ctx).is_waiting_on_user_answers(ctx) { ctx.focus(&view); } if self.sync_ask_user_question_speedbump_footer(ctx) @@ -4826,16 +4821,20 @@ impl AIBlock { self.model.status(app).is_complete() } - /// Returns `true` if the block contains any actions that are blocked on user confirmation. + /// Returns `true` when actions need confirmation or a question is waiting for an answer. pub fn is_blocked_on_user_confirmation(&self, app: &AppContext) -> bool { - self.requested_action_ids - .iter() - .filter_map(|id| { - self.action_model - .as_ref(app) - .get_action_status(self.client_ids.conversation_id, id) - }) - .any(|status| status.is_blocked()) + self.ask_user_question_view + .as_ref() + .is_some_and(|view| view.as_ref(app).is_waiting_on_user_answers(app)) + || self + .requested_action_ids + .iter() + .filter_map(|id| { + self.action_model + .as_ref(app) + .get_action_status(self.client_ids.conversation_id, id) + }) + .any(|status| status.is_blocked()) } /// Returns the server output ID for this AI block, if available. @@ -5369,10 +5368,7 @@ impl AIBlock { did_focus_subview = true; } else if let Some(ask_user_question_view) = self.ask_user_question_view.as_ref().filter(|view| { - pending_action_id.is_some_and(|id| { - let view = view.as_ref(ctx); - view.action_id() == id && view.is_editing() - }) + view.as_ref(ctx).is_waiting_on_user_answers(ctx) }) { ctx.focus(ask_user_question_view); diff --git a/app/src/ai/blocklist/inline_action/ask_user_question_view.rs b/app/src/ai/blocklist/inline_action/ask_user_question_view.rs index 4502bd7b..905f1f21 100644 --- a/app/src/ai/blocklist/inline_action/ask_user_question_view.rs +++ b/app/src/ai/blocklist/inline_action/ask_user_question_view.rs @@ -188,6 +188,17 @@ enum AskUserQuestionPhase<'a> { }, } +fn ask_user_question_is_waiting_on_user_answers( + phase: AskUserQuestionPhase<'_>, + status: Option<&AIActionStatus>, +) -> bool { + matches!(phase, AskUserQuestionPhase::Editing) + && matches!( + status, + Some(AIActionStatus::Blocked | AIActionStatus::RunningAsync) + ) +} + #[derive(Clone, Debug, Eq, PartialEq)] /// Editing-phase state for the questionnaire. /// Holds the active question index and one draft slot per question. @@ -806,10 +817,6 @@ impl AskUserQuestionView { &self.action_id } - pub fn is_editing(&self) -> bool { - self.session.is_editing() - } - /// Sets the settings-link mouse handle used in the speedbump footer. /// The footer is only actually drawn during the completed/finished render paths. pub fn set_speedbump_settings_link( @@ -902,10 +909,12 @@ impl AskUserQuestionView { }) } - /// True when the action model reports this inline action is blocked waiting on user answers. - fn is_waiting_on_user_answers(&self, app: &AppContext) -> bool { - self.action_status(app) - .is_some_and(|status| status.is_blocked()) + /// Questions can wait before execution or in an approved async executor, as in Allow All. + pub(crate) fn is_waiting_on_user_answers(&self, app: &AppContext) -> bool { + ask_user_question_is_waiting_on_user_answers( + self.session.phase(), + self.action_status(app).as_ref(), + ) } pub fn should_render_inline(&self, app: &AppContext) -> bool { @@ -1170,8 +1179,8 @@ impl AskUserQuestionView { self.abort_auto_advance(); let action_id = self.action_id().clone(); let conversation_id = self.conversation_id; - // Completing the executor only stages the result; execute_action drives the action model - // to publish final status/events for this inline block. + // A running executor receives the answer directly. A blocked question must first start + // execution; execute_action starts it and leaves an already-running receiver intact. self.action_model.update(ctx, |action_model, ctx| { action_model .ask_user_question_executor(ctx) diff --git a/app/src/ai/blocklist/inline_action/ask_user_question_view_tests.rs b/app/src/ai/blocklist/inline_action/ask_user_question_view_tests.rs index 212b57c5..cc990022 100644 --- a/app/src/ai/blocklist/inline_action/ask_user_question_view_tests.rs +++ b/app/src/ai/blocklist/inline_action/ask_user_question_view_tests.rs @@ -1,10 +1,16 @@ +use std::sync::Arc; + use ai::agent::action::{AskUserQuestionItem, AskUserQuestionOption, AskUserQuestionType}; -use ai::agent::action_result::AskUserQuestionAnswerItem; +use ai::agent::action_result::{AskUserQuestionAnswerItem, AskUserQuestionResult}; use super::{ - ask_user_question_view_state, AskUserQuestionAction, AskUserQuestionEffect, - AskUserQuestionPhase, AskUserQuestionSession, AskUserQuestionViewState, + ask_user_question_is_waiting_on_user_answers, ask_user_question_view_state, + AskUserQuestionAction, AskUserQuestionEffect, AskUserQuestionPhase, + AskUserQuestionSession, AskUserQuestionViewState, }; +use crate::ai::agent::task::TaskId; +use crate::ai::agent::{AIAgentActionId, AIAgentActionResult, AIAgentActionResultType}; +use crate::ai::blocklist::action_model::AIActionStatus; fn build_question( question_id: &str, @@ -42,6 +48,53 @@ fn current_draft(session: &AskUserQuestionSession) -> Option<&super::QuestionDra session.current().and_then(|current| current.draft) } +#[test] +fn blocked_and_running_questions_accept_answers_until_the_session_completes() { + let mut session = build_session(vec![build_question("q1", "Destination", false, true, &[])]); + let blocked = AIActionStatus::Blocked; + let running = AIActionStatus::RunningAsync; + + for status in [&blocked, &running] { + assert!(ask_user_question_is_waiting_on_user_answers( + session.phase(), + Some(status), + )); + } + + assert!(matches!( + session.apply(AskUserQuestionAction::SkipAll), + AskUserQuestionEffect::Submit(_) + )); + for status in [&blocked, &running] { + assert!(!ask_user_question_is_waiting_on_user_answers( + session.phase(), + Some(status), + )); + } +} + +#[test] +fn queued_preprocessing_missing_and_finished_questions_do_not_accept_answers() { + let session = build_session(vec![build_question("q1", "Destination", false, true, &[])]); + let finished = AIActionStatus::Finished(Arc::new(AIAgentActionResult { + result: AIAgentActionResultType::AskUserQuestion(AskUserQuestionResult::Cancelled), + task_id: TaskId::new("question-task".to_owned()), + id: AIAgentActionId::from("question-action".to_owned()), + })); + + for status in [ + None, + Some(AIActionStatus::Queued), + Some(AIActionStatus::Preprocessing), + Some(finished), + ] { + assert!(!ask_user_question_is_waiting_on_user_answers( + session.phase(), + status.as_ref(), + )); + } +} + #[test] fn enter_on_other_row_focuses_the_other_input() { let mut session = build_session(vec![build_question("q1", "Only", false, true, &["Stable"])]); diff --git a/app/src/ai/runtime/rig_request_tests.rs b/app/src/ai/runtime/rig_request_tests.rs index f14f8dad..830fb161 100644 --- a/app/src/ai/runtime/rig_request_tests.rs +++ b/app/src/ai/runtime/rig_request_tests.rs @@ -242,6 +242,58 @@ fn plan_turn_requires_a_plan_tool_result() { assert!(prompt.contains("Do not return the plan only as prose")); } +#[test] +fn normal_and_plan_turns_advertise_clarifications_only_for_interactive_permission_modes() { + use crate::ai::execution_profiles::{ + AIExecutionProfile, AskUserQuestionPermission, PermissionMode, + }; + + let _flag = galaxy_core::features::FeatureFlag::AskUserQuestion.override_enabled(true); + for (permission_mode, expected) in [ + (PermissionMode::AskFirst, true), + (PermissionMode::AllowAll, true), + (PermissionMode::GalaxyDecides, false), + (PermissionMode::ReadOnly, false), + ] { + for plan_mode in [false, true] { + let mut profile = AIExecutionProfile { + permission_mode, + ..Default::default() + }; + profile.project_permission_mode(); + let mut params = RequestParams::new_for_test(); + params.permission_mode = permission_mode; + params.ask_user_question_enabled = + profile.ask_user_question != AskUserQuestionPermission::Never; + let mut input = user_query("Clarify the requested destination before proceeding"); + if plan_mode { + let AIAgentInput::UserQuery { + user_query_mode, .. + } = &mut input + else { + unreachable!("user_query returns a user query input"); + }; + *user_query_mode = UserQueryMode::Plan; + } + params.input = vec![input]; + let (normal, cli) = + crate::ai::agent::api::prepare_direct_provider_params(&mut params); + assert!(!cli.contains(&ToolType::AskUserQuestion)); + + let prepared = prepare_rig_turn(&config(), params, normal, cli); + assert_eq!( + prepared + .request + .tools + .iter() + .any(|tool| tool.name == "ask_user_question"), + expected, + "{permission_mode:?}, plan_mode={plan_mode}" + ); + } + } +} + #[test] fn no_tools_turn_flattens_historical_tool_protocol_messages() { let mut params = RequestParams::new_for_test(); diff --git a/crates/cloud_object_models/src/ai_execution_profile.rs b/crates/cloud_object_models/src/ai_execution_profile.rs index 5ad265c3..ada17173 100644 --- a/crates/cloud_object_models/src/ai_execution_profile.rs +++ b/crates/cloud_object_models/src/ai_execution_profile.rs @@ -661,10 +661,14 @@ impl AIExecutionProfile { PermissionMode::GalaxyDecides => RunAgentsPermission::GalaxyDecides, PermissionMode::AllowAll => RunAgentsPermission::AlwaysAllow, }; - self.ask_user_question = if self.permission_mode == PermissionMode::AskFirst { - AskUserQuestionPermission::AlwaysAsk - } else { - AskUserQuestionPermission::Never + // Allow All bypasses tool approval, while clarification can still need user input. + self.ask_user_question = match self.permission_mode { + PermissionMode::AskFirst | PermissionMode::AllowAll => { + AskUserQuestionPermission::AlwaysAsk + } + PermissionMode::ReadOnly | PermissionMode::GalaxyDecides => { + AskUserQuestionPermission::Never + } }; self.directory_allowlist.clear(); self.mcp_allowlist.clear(); diff --git a/crates/cloud_object_models/src/ai_execution_profile_tests.rs b/crates/cloud_object_models/src/ai_execution_profile_tests.rs index fb53ee6d..470396c5 100644 --- a/crates/cloud_object_models/src/ai_execution_profile_tests.rs +++ b/crates/cloud_object_models/src/ai_execution_profile_tests.rs @@ -61,6 +61,7 @@ fn serialized_profiles_only_store_one_permission_mode_and_bounded_approvals() { "write_to_pty", "run_agents", "computer_use", + "ask_user_question", "directory_allowlist", "mcp_allowlist", ] { @@ -127,3 +128,25 @@ fn allow_all_projects_every_tool_to_always_allow() { assert_eq!(profile.computer_use, ComputerUsePermission::AlwaysAllow); assert_eq!(profile.run_agents, RunAgentsPermission::AlwaysAllow); } + +#[test] +fn clarification_availability_follows_permission_mode_after_projection_and_restore() { + for (mode, expected) in [ + (PermissionMode::AskFirst, AskUserQuestionPermission::AlwaysAsk), + (PermissionMode::AllowAll, AskUserQuestionPermission::AlwaysAsk), + (PermissionMode::GalaxyDecides, AskUserQuestionPermission::Never), + (PermissionMode::ReadOnly, AskUserQuestionPermission::Never), + ] { + let mut profile = AIExecutionProfile { + permission_mode: mode, + ..Default::default() + }; + profile.project_permission_mode(); + assert_eq!(profile.ask_user_question, expected, "{mode:?}"); + + let saved = serde_json::to_value(&profile).unwrap(); + assert!(saved.get("ask_user_question").is_none()); + let restored: AIExecutionProfile = serde_json::from_value(saved).unwrap(); + assert_eq!(restored.ask_user_question, expected, "{mode:?}"); + } +}