huskies: merge 1228 story Render agent questions as numbered options in chat protocols without question UI
This commit is contained in:
@@ -37,6 +37,46 @@ pub struct PermissionForward {
|
||||
pub response_tx: oneshot::Sender<PermissionDecision>,
|
||||
}
|
||||
|
||||
/// A single selectable choice within a [`QuestionSpec`].
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct QuestionOption {
|
||||
pub label: String,
|
||||
pub description: String,
|
||||
}
|
||||
|
||||
/// A multiple-choice question forwarded from the MCP `ask_question` tool to a
|
||||
/// chat transport for rendering as numbered text (story 1228).
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct QuestionSpec {
|
||||
pub header: String,
|
||||
pub question: String,
|
||||
pub options: Vec<QuestionOption>,
|
||||
pub multi_select: bool,
|
||||
}
|
||||
|
||||
/// The user's reply to a forwarded [`QuestionSpec`].
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub enum QuestionAnswer {
|
||||
/// 0-based indices into `QuestionSpec::options` the user selected.
|
||||
Selected(Vec<usize>),
|
||||
/// Freeform text the user typed instead of selecting a listed option
|
||||
/// (the always-available "Other" path, AC5).
|
||||
FreeText(String),
|
||||
}
|
||||
|
||||
/// A question request forwarded from the MCP `ask_question` tool to the
|
||||
/// active chat transport. The MCP handler blocks on `response_tx` until a
|
||||
/// chat reply resolves it (or it times out).
|
||||
///
|
||||
/// Kept structurally separate from `PermissionForward` / permission-router
|
||||
/// plumbing (see `service::question_router`) so a reply answering one is
|
||||
/// never misinterpreted as answering the other (story 1228, AC4).
|
||||
pub struct QuestionForward {
|
||||
pub request_id: String,
|
||||
pub question: QuestionSpec,
|
||||
pub response_tx: oneshot::Sender<Result<QuestionAnswer, String>>,
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
/// Shared application state threaded through all HTTP handlers via Poem's `Data` extractor.
|
||||
pub struct AppContext {
|
||||
@@ -56,6 +96,10 @@ pub struct AppContext {
|
||||
/// `prompt_permission` tool. The MCP handler sends a [`PermissionForward`]
|
||||
/// and awaits the oneshot response.
|
||||
pub perm_tx: mpsc::UnboundedSender<PermissionForward>,
|
||||
/// Sender for questions originating from the MCP `ask_question` tool.
|
||||
/// The MCP handler sends a [`QuestionForward`] and awaits the oneshot
|
||||
/// response (story 1228).
|
||||
pub question_tx: mpsc::UnboundedSender<QuestionForward>,
|
||||
/// Child process of the QA app launched for manual testing.
|
||||
/// Only one instance runs at a time.
|
||||
pub qa_app_process: Arc<std::sync::Mutex<Option<std::process::Child>>>,
|
||||
@@ -101,6 +145,8 @@ impl AppContext {
|
||||
let (reconciliation_tx, _) = broadcast::channel(64);
|
||||
let (perm_tx, perm_rx) = mpsc::unbounded_channel();
|
||||
let permission_registry = crate::service::permission_router::ResponderRegistry::new();
|
||||
let (question_tx, question_rx) = mpsc::unbounded_channel();
|
||||
let question_registry = crate::service::question_router::QuestionResponderRegistry::new();
|
||||
// Plain `#[test]` fns (no tokio runtime) construct `AppContext` too;
|
||||
// skip spawning when there's no reactor to spawn onto since those
|
||||
// tests never exercise the permission plumbing.
|
||||
@@ -109,6 +155,10 @@ impl AppContext {
|
||||
perm_rx,
|
||||
Arc::clone(&permission_registry),
|
||||
);
|
||||
crate::service::question_router::spawn_question_router(
|
||||
question_rx,
|
||||
Arc::clone(&question_registry),
|
||||
);
|
||||
}
|
||||
let timer_store = Arc::new(TimerStore::load(
|
||||
project_root.join(".huskies").join("timers.json"),
|
||||
@@ -130,6 +180,10 @@ impl AppContext {
|
||||
pending_perm_replies: crate::service::permission_router::PendingPermReplies::new(),
|
||||
permission_timeout_secs: 120,
|
||||
remembered_permissions: crate::service::permission_router::RememberedPermissions::new(),
|
||||
question_registry,
|
||||
pending_question_replies: crate::service::question_router::PendingQuestionReplies::new(
|
||||
),
|
||||
question_timeout_secs: 120,
|
||||
status: agents.status_broadcaster(),
|
||||
chat_dispatcher: Arc::new(crate::chat::dispatcher::ChatDispatcher::new(1_500)),
|
||||
});
|
||||
@@ -141,6 +195,7 @@ impl AppContext {
|
||||
watcher_tx,
|
||||
reconciliation_tx,
|
||||
perm_tx,
|
||||
question_tx,
|
||||
qa_app_process: Arc::new(std::sync::Mutex::new(None)),
|
||||
bot_shutdown: None,
|
||||
matrix_shutdown_tx: None,
|
||||
|
||||
@@ -7,10 +7,12 @@ use serde_json::{Value, json};
|
||||
|
||||
mod chat_telemetry;
|
||||
mod permission;
|
||||
mod question;
|
||||
mod usage;
|
||||
|
||||
pub(crate) use chat_telemetry::tool_chat_telemetry;
|
||||
pub(crate) use permission::tool_prompt_permission;
|
||||
pub(crate) use question::tool_ask_question;
|
||||
pub(crate) use usage::tool_get_token_usage;
|
||||
|
||||
pub(crate) fn tool_get_server_logs(args: &Value) -> Result<String, String> {
|
||||
|
||||
@@ -0,0 +1,229 @@
|
||||
//! MCP `ask_question` tool — presents a multiple-choice question to the user
|
||||
//! via chat transports that lack a native question UI (story 1228).
|
||||
|
||||
use serde_json::{Value, json};
|
||||
|
||||
use crate::http::context::{
|
||||
AppContext, QuestionAnswer, QuestionForward, QuestionOption, QuestionSpec,
|
||||
};
|
||||
|
||||
/// MCP tool called by an agent to ask the user a multiple-choice question.
|
||||
///
|
||||
/// Forwards the question through the shared channel to the active chat
|
||||
/// transport (currently Matrix), which renders it as numbered text. Blocks
|
||||
/// until a reply resolves it or the question times out.
|
||||
pub(crate) async fn tool_ask_question(args: &Value, ctx: &AppContext) -> Result<String, String> {
|
||||
let header = args
|
||||
.get("header")
|
||||
.and_then(|v| v.as_str())
|
||||
.unwrap_or("Question")
|
||||
.to_string();
|
||||
let question_text = args
|
||||
.get("question")
|
||||
.and_then(|v| v.as_str())
|
||||
.ok_or_else(|| "Missing required argument: question".to_string())?
|
||||
.to_string();
|
||||
let multi_select = args
|
||||
.get("multi_select")
|
||||
.and_then(|v| v.as_bool())
|
||||
.unwrap_or(false);
|
||||
let options: Vec<QuestionOption> = args
|
||||
.get("options")
|
||||
.and_then(|v| v.as_array())
|
||||
.ok_or_else(|| "Missing required argument: options".to_string())?
|
||||
.iter()
|
||||
.map(|o| QuestionOption {
|
||||
label: o
|
||||
.get("label")
|
||||
.and_then(|v| v.as_str())
|
||||
.unwrap_or_default()
|
||||
.to_string(),
|
||||
description: o
|
||||
.get("description")
|
||||
.and_then(|v| v.as_str())
|
||||
.unwrap_or_default()
|
||||
.to_string(),
|
||||
})
|
||||
.collect();
|
||||
|
||||
if options.len() < 2 {
|
||||
return Err("options must contain at least 2 entries".to_string());
|
||||
}
|
||||
|
||||
let question = QuestionSpec {
|
||||
header,
|
||||
question: question_text,
|
||||
options,
|
||||
multi_select,
|
||||
};
|
||||
|
||||
if ctx.services.question_registry.is_empty() {
|
||||
crate::slog!("[question] No interactive session active — cannot ask question");
|
||||
return serde_json::to_string_pretty(&json!({
|
||||
"answered": false,
|
||||
"message": "No interactive session active. Nobody is available to answer this question."
|
||||
}))
|
||||
.map_err(|e| format!("Serialization error: {e}"));
|
||||
}
|
||||
|
||||
let request_id = uuid::Uuid::new_v4().to_string();
|
||||
let (response_tx, response_rx) = tokio::sync::oneshot::channel();
|
||||
let option_labels: Vec<String> = question.options.iter().map(|o| o.label.clone()).collect();
|
||||
|
||||
if ctx
|
||||
.question_tx
|
||||
.send(QuestionForward {
|
||||
request_id: request_id.clone(),
|
||||
question,
|
||||
response_tx,
|
||||
})
|
||||
.is_err()
|
||||
{
|
||||
return serde_json::to_string_pretty(&json!({
|
||||
"answered": false,
|
||||
"message": "Failed to forward question — no active session."
|
||||
}))
|
||||
.map_err(|e| format!("Serialization error: {e}"));
|
||||
}
|
||||
|
||||
let timeout_secs = ctx.services.question_timeout_secs;
|
||||
let outcome = tokio::time::timeout(std::time::Duration::from_secs(timeout_secs), response_rx)
|
||||
.await
|
||||
.map_err(|_| format!("Question timed out after {timeout_secs} seconds with no reply"))?
|
||||
.map_err(|_| "Question response channel closed unexpectedly".to_string())?;
|
||||
|
||||
match outcome {
|
||||
Ok(QuestionAnswer::Selected(indices)) => {
|
||||
let labels: Vec<&String> = indices
|
||||
.iter()
|
||||
.filter_map(|&i| option_labels.get(i))
|
||||
.collect();
|
||||
serde_json::to_string_pretty(&json!({
|
||||
"answered": true,
|
||||
"selected_indices": indices,
|
||||
"selected_labels": labels,
|
||||
}))
|
||||
.map_err(|e| format!("Serialization error: {e}"))
|
||||
}
|
||||
Ok(QuestionAnswer::FreeText(text)) => serde_json::to_string_pretty(&json!({
|
||||
"answered": true,
|
||||
"free_text": text,
|
||||
}))
|
||||
.map_err(|e| format!("Serialization error: {e}")),
|
||||
Err(message) => serde_json::to_string_pretty(&json!({
|
||||
"answered": false,
|
||||
"message": message,
|
||||
}))
|
||||
.map_err(|e| format!("Serialization error: {e}")),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::http::test_helpers::test_ctx;
|
||||
|
||||
#[tokio::test]
|
||||
async fn tool_ask_question_no_interactive_session_returns_not_answered() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let ctx = test_ctx(tmp.path());
|
||||
|
||||
let result = tool_ask_question(
|
||||
&json!({
|
||||
"question": "Which approach?",
|
||||
"options": [
|
||||
{"label": "A", "description": "First"},
|
||||
{"label": "B", "description": "Second"}
|
||||
]
|
||||
}),
|
||||
&ctx,
|
||||
)
|
||||
.await
|
||||
.expect("must return Ok even when no session is active");
|
||||
|
||||
let parsed: Value = serde_json::from_str(&result).unwrap();
|
||||
assert_eq!(parsed["answered"], false);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn tool_ask_question_requires_at_least_two_options() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let ctx = test_ctx(tmp.path());
|
||||
|
||||
let result = tool_ask_question(
|
||||
&json!({
|
||||
"question": "Pick one?",
|
||||
"options": [{"label": "Only", "description": "one"}]
|
||||
}),
|
||||
&ctx,
|
||||
)
|
||||
.await;
|
||||
assert!(result.is_err());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn tool_ask_question_selected_answer_returns_labels() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let ctx = test_ctx(tmp.path());
|
||||
|
||||
let (guard, mut rx) = ctx.services.question_registry.register();
|
||||
tokio::spawn(async move {
|
||||
if let Some(forward) = rx.recv().await {
|
||||
let _ = forward
|
||||
.response_tx
|
||||
.send(Ok(QuestionAnswer::Selected(vec![1])));
|
||||
}
|
||||
drop(guard);
|
||||
});
|
||||
|
||||
let result = tool_ask_question(
|
||||
&json!({
|
||||
"question": "Which approach?",
|
||||
"options": [
|
||||
{"label": "A", "description": "First"},
|
||||
{"label": "B", "description": "Second"}
|
||||
]
|
||||
}),
|
||||
&ctx,
|
||||
)
|
||||
.await
|
||||
.expect("should succeed");
|
||||
|
||||
let parsed: Value = serde_json::from_str(&result).unwrap();
|
||||
assert_eq!(parsed["answered"], true);
|
||||
assert_eq!(parsed["selected_labels"][0], "B");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn tool_ask_question_free_text_answer() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let ctx = test_ctx(tmp.path());
|
||||
|
||||
let (guard, mut rx) = ctx.services.question_registry.register();
|
||||
tokio::spawn(async move {
|
||||
if let Some(forward) = rx.recv().await {
|
||||
let _ = forward
|
||||
.response_tx
|
||||
.send(Ok(QuestionAnswer::FreeText("Something else".to_string())));
|
||||
}
|
||||
drop(guard);
|
||||
});
|
||||
|
||||
let result = tool_ask_question(
|
||||
&json!({
|
||||
"question": "Which approach?",
|
||||
"options": [
|
||||
{"label": "A", "description": "First"},
|
||||
{"label": "B", "description": "Second"}
|
||||
]
|
||||
}),
|
||||
&ctx,
|
||||
)
|
||||
.await
|
||||
.expect("should succeed");
|
||||
|
||||
let parsed: Value = serde_json::from_str(&result).unwrap();
|
||||
assert_eq!(parsed["answered"], true);
|
||||
assert_eq!(parsed["free_text"], "Something else");
|
||||
}
|
||||
}
|
||||
@@ -86,6 +86,7 @@ pub async fn dispatch_tool_call(
|
||||
"get_version" => diagnostics::tool_get_version(ctx),
|
||||
// Permission bridge (Claude Code → frontend dialog)
|
||||
"prompt_permission" => diagnostics::tool_prompt_permission(&args, ctx).await,
|
||||
"ask_question" => diagnostics::tool_ask_question(&args, ctx).await,
|
||||
// Token usage
|
||||
"get_token_usage" => diagnostics::tool_get_token_usage(&args, ctx),
|
||||
// Chat turn telemetry (story 1209)
|
||||
|
||||
@@ -120,7 +120,8 @@ mod tests {
|
||||
assert!(names.contains(&"write"));
|
||||
assert!(names.contains(&"gc"));
|
||||
assert!(names.contains(&"chat_telemetry"));
|
||||
assert_eq!(tools.len(), 88);
|
||||
assert!(names.contains(&"ask_question"));
|
||||
assert_eq!(tools.len(), 89);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -60,6 +60,40 @@ pub(super) fn system_tools() -> Vec<Value> {
|
||||
"required": ["tool_name", "input"]
|
||||
}
|
||||
}),
|
||||
json!({
|
||||
"name": "ask_question",
|
||||
"description": "Ask the user a multiple-choice question via the active chat transport (e.g. Matrix). Renders as numbered options in chat protocols without a native question UI; the reply is parsed back (a number, a comma-separated list for multi_select, an option label, or free text) and returned here. Blocks until answered or timed out.",
|
||||
"inputSchema": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"question": {
|
||||
"type": "string",
|
||||
"description": "The question to ask, e.g. 'Which approach should we use?'"
|
||||
},
|
||||
"header": {
|
||||
"type": "string",
|
||||
"description": "Short label for the question (e.g. 'Approach'). Defaults to 'Question'."
|
||||
},
|
||||
"options": {
|
||||
"type": "array",
|
||||
"items": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"label": {"type": "string"},
|
||||
"description": {"type": "string"}
|
||||
},
|
||||
"required": ["label", "description"]
|
||||
},
|
||||
"description": "At least 2 selectable options, each with a label and description."
|
||||
},
|
||||
"multi_select": {
|
||||
"type": "boolean",
|
||||
"description": "If true, the user may select multiple options (e.g. reply '1,3'). Default false."
|
||||
}
|
||||
},
|
||||
"required": ["question", "options"]
|
||||
}
|
||||
}),
|
||||
json!({
|
||||
"name": "get_token_usage",
|
||||
"description": "Return per-agent token usage records from the persistent log. Shows input tokens, output tokens, cache tokens, and cost in USD for each agent session. Optionally filter by story_id.",
|
||||
|
||||
Reference in New Issue
Block a user