From 7dceca51cfab1a7ff3f9699cf6fe8cfa43d33cf1 Mon Sep 17 00:00:00 2001 From: Ryan Ward Date: Thu, 22 Oct 2026 16:12:16 +0000 Subject: [PATCH] Allow clarifying questions in Allow All profiles Separate clarification questions from action approvals while preserving Galaxy Decides and Read Only behavior. Respect question capabilities in explicit tool overrides, keep CLI monitors focused, and render approved asynchronous questions while they wait for answers. Validation: cargo build passed on Linux. Added permission, request and question-state regressions; tests and GUI execution were not run under the build-only workflow. --- AGENTS.md | 1 + app/src/ai/agent/api/impl.rs | 11 ++++ app/src/ai/agent/api/impl_tests.rs | 45 +++++++++++++- app/src/ai/blocklist/block.rs | 34 +++++------ .../inline_action/ask_user_question_view.rs | 29 +++++---- .../ask_user_question_view_tests.rs | 59 ++++++++++++++++++- app/src/ai/runtime/rig_request_tests.rs | 52 ++++++++++++++++ .../src/ai_execution_profile.rs | 12 ++-- .../src/ai_execution_profile_tests.rs | 23 ++++++++ 9 files changed, 227 insertions(+), 39 deletions(-) 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:?}"); + } +}