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.
This commit is contained in:
Ryan Ward
2026-10-22 16:12:16 +00:00
parent f6c6434e5e
commit 7dceca51cf
9 changed files with 227 additions and 39 deletions
+1
View File
@@ -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. - 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. - 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 `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. - 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.
+11
View File
@@ -28,6 +28,16 @@ pub(crate) fn prepare_direct_provider_params(
.clone() .clone()
.unwrap_or_else(|| get_supported_tools(params)); .unwrap_or_else(|| get_supported_tools(params));
remove_orchestration_tools_if_disabled(&mut supported_tools, params.orchestration_enabled); 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. // Computer use controls this machine, even when the terminal is connected elsewhere.
// Only expose the initial request here; the dedicated profile owns subsequent actions. // Only expose the initial request here; the dedicated profile owns subsequent actions.
supported_tools.retain(|tool| *tool != api::ToolType::UseComputer supported_tools.retain(|tool| *tool != api::ToolType::UseComputer
@@ -39,6 +49,7 @@ pub(crate) fn prepare_direct_provider_params(
params.orchestration_enabled, params.orchestration_enabled,
); );
supported_cli_agent_tools.retain(|tool| !matches!(tool, api::ToolType::UseComputer | api::ToolType::RequestComputerUse)); 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 { if params.permission_mode == crate::ai::execution_profiles::PermissionMode::ReadOnly {
let permitted = |tool: &api::ToolType| is_read_only_tool(tool) 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)); || (!params.command_allowlist.is_empty() && matches!(tool, api::ToolType::RunShellCommand | api::ToolType::ReadShellCommandOutput | api::ToolType::WriteToLongRunningShellCommand));
+42 -3
View File
@@ -191,9 +191,7 @@ fn supported_tools_omits_ask_user_question_when_disabled() {
#[test] #[test]
fn supported_tools_includes_ask_user_question_when_enabled_and_feature_flag_is_enabled() { fn supported_tools_includes_ask_user_question_when_enabled_and_feature_flag_is_enabled() {
if !FeatureFlag::AskUserQuestion.is_enabled() { let _flag = FeatureFlag::AskUserQuestion.override_enabled(true);
return;
}
let params = request_params_with_ask_user_question_enabled(true); let params = request_params_with_ask_user_question_enabled(true);
let supported_tools = get_supported_tools(&params); let supported_tools = get_supported_tools(&params);
@@ -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)); 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] #[test]
fn remote_supported_tools_include_search_codebase_when_connected_and_feature_flag_is_enabled() { fn remote_supported_tools_include_search_codebase_when_connected_and_feature_flag_is_enabled() {
let _flag = FeatureFlag::RemoteCodebaseIndexing.override_enabled(true); let _flag = FeatureFlag::RemoteCodebaseIndexing.override_enabled(true);
+8 -12
View File
@@ -3998,12 +3998,7 @@ impl AIBlock {
}); });
self.ask_user_question_view = Some(view.clone()); self.ask_user_question_view = Some(view.clone());
if self if view.as_ref(ctx).is_waiting_on_user_answers(ctx) {
.action_model
.as_ref(ctx)
.get_action_status(self.client_ids.conversation_id, action_id)
.is_some_and(|status| status.is_blocked())
{
ctx.focus(&view); ctx.focus(&view);
} }
if self.sync_ask_user_question_speedbump_footer(ctx) if self.sync_ask_user_question_speedbump_footer(ctx)
@@ -4826,9 +4821,13 @@ impl AIBlock {
self.model.status(app).is_complete() 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 { pub fn is_blocked_on_user_confirmation(&self, app: &AppContext) -> bool {
self.requested_action_ids 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() .iter()
.filter_map(|id| { .filter_map(|id| {
self.action_model self.action_model
@@ -5369,10 +5368,7 @@ impl AIBlock {
did_focus_subview = true; did_focus_subview = true;
} else if let Some(ask_user_question_view) = } else if let Some(ask_user_question_view) =
self.ask_user_question_view.as_ref().filter(|view| { self.ask_user_question_view.as_ref().filter(|view| {
pending_action_id.is_some_and(|id| { view.as_ref(ctx).is_waiting_on_user_answers(ctx)
let view = view.as_ref(ctx);
view.action_id() == id && view.is_editing()
})
}) })
{ {
ctx.focus(ask_user_question_view); ctx.focus(ask_user_question_view);
@@ -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)] #[derive(Clone, Debug, Eq, PartialEq)]
/// Editing-phase state for the questionnaire. /// Editing-phase state for the questionnaire.
/// Holds the active question index and one draft slot per question. /// Holds the active question index and one draft slot per question.
@@ -806,10 +817,6 @@ impl AskUserQuestionView {
&self.action_id &self.action_id
} }
pub fn is_editing(&self) -> bool {
self.session.is_editing()
}
/// Sets the settings-link mouse handle used in the speedbump footer. /// Sets the settings-link mouse handle used in the speedbump footer.
/// The footer is only actually drawn during the completed/finished render paths. /// The footer is only actually drawn during the completed/finished render paths.
pub fn set_speedbump_settings_link( 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. /// Questions can wait before execution or in an approved async executor, as in Allow All.
fn is_waiting_on_user_answers(&self, app: &AppContext) -> bool { pub(crate) fn is_waiting_on_user_answers(&self, app: &AppContext) -> bool {
self.action_status(app) ask_user_question_is_waiting_on_user_answers(
.is_some_and(|status| status.is_blocked()) self.session.phase(),
self.action_status(app).as_ref(),
)
} }
pub fn should_render_inline(&self, app: &AppContext) -> bool { pub fn should_render_inline(&self, app: &AppContext) -> bool {
@@ -1170,8 +1179,8 @@ impl AskUserQuestionView {
self.abort_auto_advance(); self.abort_auto_advance();
let action_id = self.action_id().clone(); let action_id = self.action_id().clone();
let conversation_id = self.conversation_id; let conversation_id = self.conversation_id;
// Completing the executor only stages the result; execute_action drives the action model // A running executor receives the answer directly. A blocked question must first start
// to publish final status/events for this inline block. // execution; execute_action starts it and leaves an already-running receiver intact.
self.action_model.update(ctx, |action_model, ctx| { self.action_model.update(ctx, |action_model, ctx| {
action_model action_model
.ask_user_question_executor(ctx) .ask_user_question_executor(ctx)
@@ -1,10 +1,16 @@
use std::sync::Arc;
use ai::agent::action::{AskUserQuestionItem, AskUserQuestionOption, AskUserQuestionType}; use ai::agent::action::{AskUserQuestionItem, AskUserQuestionOption, AskUserQuestionType};
use ai::agent::action_result::AskUserQuestionAnswerItem; use ai::agent::action_result::{AskUserQuestionAnswerItem, AskUserQuestionResult};
use super::{ use super::{
ask_user_question_view_state, AskUserQuestionAction, AskUserQuestionEffect, ask_user_question_is_waiting_on_user_answers, ask_user_question_view_state,
AskUserQuestionPhase, AskUserQuestionSession, AskUserQuestionViewState, 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( fn build_question(
question_id: &str, question_id: &str,
@@ -42,6 +48,53 @@ fn current_draft(session: &AskUserQuestionSession) -> Option<&super::QuestionDra
session.current().and_then(|current| current.draft) 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] #[test]
fn enter_on_other_row_focuses_the_other_input() { fn enter_on_other_row_focuses_the_other_input() {
let mut session = build_session(vec![build_question("q1", "Only", false, true, &["Stable"])]); let mut session = build_session(vec![build_question("q1", "Only", false, true, &["Stable"])]);
+52
View File
@@ -242,6 +242,58 @@ fn plan_turn_requires_a_plan_tool_result() {
assert!(prompt.contains("Do not return the plan only as prose")); 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] #[test]
fn no_tools_turn_flattens_historical_tool_protocol_messages() { fn no_tools_turn_flattens_historical_tool_protocol_messages() {
let mut params = RequestParams::new_for_test(); let mut params = RequestParams::new_for_test();
@@ -661,10 +661,14 @@ impl AIExecutionProfile {
PermissionMode::GalaxyDecides => RunAgentsPermission::GalaxyDecides, PermissionMode::GalaxyDecides => RunAgentsPermission::GalaxyDecides,
PermissionMode::AllowAll => RunAgentsPermission::AlwaysAllow, PermissionMode::AllowAll => RunAgentsPermission::AlwaysAllow,
}; };
self.ask_user_question = if self.permission_mode == PermissionMode::AskFirst { // 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 AskUserQuestionPermission::AlwaysAsk
} else { }
PermissionMode::ReadOnly | PermissionMode::GalaxyDecides => {
AskUserQuestionPermission::Never AskUserQuestionPermission::Never
}
}; };
self.directory_allowlist.clear(); self.directory_allowlist.clear();
self.mcp_allowlist.clear(); self.mcp_allowlist.clear();
@@ -61,6 +61,7 @@ fn serialized_profiles_only_store_one_permission_mode_and_bounded_approvals() {
"write_to_pty", "write_to_pty",
"run_agents", "run_agents",
"computer_use", "computer_use",
"ask_user_question",
"directory_allowlist", "directory_allowlist",
"mcp_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.computer_use, ComputerUsePermission::AlwaysAllow);
assert_eq!(profile.run_agents, RunAgentsPermission::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:?}");
}
}