diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt index 55f6beb..3448629 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt @@ -460,17 +460,28 @@ fun fetchUsage(settings: ServerSettings): List = } } +/** + * Answers one question with everything that was chosen. + * + * A list even when one thing was picked, because that is the shape of the answer rather than a + * special case of it. What a provider makes of several answers is its own business and is decided + * on the server; nothing here joins, splits or reformats them for one. + */ fun answerQuestion( settings: ServerSettings, sessionId: String, questionId: String, - answer: String, + answers: List, ) { requestFromServer( settings, "/sessions/$sessionId/answer", method = "POST", - jsonBody = JSONObject().put("questionId", questionId).put("answer", answer).toString(), + jsonBody = + JSONObject() + .put("questionId", questionId) + .put("answers", JSONArray(answers)) + .toString(), ) {} } diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/AskQuestion.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/AskQuestion.kt index bd3a9a4..42ae30f 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/AskQuestion.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/AskQuestion.kt @@ -27,112 +27,41 @@ import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier import androidx.compose.ui.text.font.FontFamily import androidx.compose.ui.unit.dp -import org.json.JSONException -import org.json.JSONObject /** - * A question the model asked, as AskUserQuestion states it. + * Every question one tool call is waiting on. * - * Parsed from the tool call's own input rather than carried in the question event, because the - * event's job is to be answerable -- an id and the labels that can be sent back -- and this is - * everything the reader needs to *decide*, which is a longer list: what each option means, and - * sometimes a worked example of what picking it would produce. - */ -data class AskedQuestion( - val question: String, - /** A short tag for the question, at most a dozen characters. Absent in older calls. */ - val header: String?, - /** Whether more than one option may be chosen at once. */ - val multiSelect: Boolean, - val options: List, -) - -data class AskedOption( - val label: String, - val description: String?, - /** A block to show as-is -- a mockup, a diff, a config file -- when the option carries one. */ - val preview: String?, -) - -/** - * Reads the questions out of an AskUserQuestion call's input. - * - * Empty when the input is not that shape, which is what a caller falls back on: a question that - * cannot be read in full is still answerable from the labels the event carried, and showing those - * beats showing nothing. - */ -fun parseAskedQuestions(input: String): List { - val body = - try { - JSONObject(input) - } catch (_: JSONException) { - return emptyList() - } - val questions = body.optJSONArray("questions") ?: return emptyList() - return (0 until questions.length()).mapNotNull { index -> - val entry = questions.optJSONObject(index) ?: return@mapNotNull null - val options = entry.optJSONArray("options") - AskedQuestion( - question = - entry.optString("question").ifEmpty { - return@mapNotNull null - }, - header = entry.optString("header").ifEmpty { null }, - multiSelect = entry.optBoolean("multiSelect", false), - options = - (0 until (options?.length() ?: 0)).mapNotNull { at -> - val option = options?.optJSONObject(at) ?: return@mapNotNull null - AskedOption( - label = - option.optString("label").ifEmpty { - return@mapNotNull null - }, - description = option.optString("description").ifEmpty { null }, - preview = option.optString("preview").ifEmpty { null }, - ) - }, - ) - } -} - -/** - * Everything an AskUserQuestion call is waiting for, on the call itself. - * - * The questions come from the call's input and the ids to answer with come from the events; they - * are matched by the question's own text, which the backend uses verbatim as the prompt. An event - * with no matching question still renders -- from its labels alone -- because a question that draws - * as nothing is indistinguishable from one that was never asked, and the turn is blocked on it - * either way. + * All of it comes from the question events themselves -- what each option means, what picking it + * would produce, whether several may be picked at once. None of it is read out of the call's own + * input, which is one provider's JSON: parsing that here would put that provider's schema in the + * app, where no other provider can reach it and where it drifts the first time the schema moves. */ @Composable fun AskUserQuestionBody( - input: String, asks: List, - onAnswer: (questionId: String, answer: String) -> Unit, + onAnswer: (questionId: String, answers: List) -> Unit, ) { - val asked = remember(input) { parseAskedQuestions(input) } Column(Modifier.fillMaxWidth()) { asks.forEach { ask -> Spacer(Modifier.height(12.dp)) - AskedQuestionCard( - ask = ask, - asked = asked.firstOrNull { it.question == ask.prompt }, - onAnswer = { answer -> onAnswer(ask.id, answer) }, - ) + AskedQuestion(ask) { answers -> onAnswer(ask.id, answers) } } } } +/** + * One question: what is being asked, what can be answered, and what was. + * + * The same body wherever a question appears -- on the call that asked it, or as a card of its own + * when nothing did. A question is the same thing either way, and two renderings of it would be two + * places for an answer to go missing. + */ @Composable -private fun AskedQuestionCard( - ask: TranscriptItem.QuestionCard, - asked: AskedQuestion?, - onAnswer: (String) -> Unit, -) { +fun AskedQuestion(ask: TranscriptItem.QuestionCard, onAnswer: (List) -> Unit) { Column(Modifier.fillMaxWidth()) { - asked?.header?.let { header -> - // The tool's own chip text. Its own line rather than beside the question, because it - // is a label *for* the question and the question is the thing to read. + ask.header?.let { header -> + // Its own line rather than beside the question, because it is a label *for* the + // question and the question is the thing to read. Text( header.uppercase(), style = MaterialTheme.typography.labelSmall, @@ -141,20 +70,26 @@ private fun AskedQuestionCard( } Text(ask.prompt, style = MaterialTheme.typography.bodyLarge) Spacer(Modifier.height(8.dp)) - if (ask.answer != null) { + if (ask.answers.isNotEmpty()) { + // Joined for reading only: they arrived as a list and stay one everywhere else. Text( - "Answered: ${ask.answer}", + "Answered: ${ask.answers.joinToString(", ")}", style = MaterialTheme.typography.labelLarge, color = MaterialTheme.colorScheme.onSurfaceVariant, ) return@Column } - val options = asked?.options ?: ask.options.map { AskedOption(it, null, null) } - if (asked?.multiSelect == true) MultipleChoice(options, onAnswer) - else - options.forEach { option -> - OptionCard(option, selected = false) { onAnswer(option.label) } + if (ask.multiSelect) { + MultipleChoice(ask.options, onAnswer) + } else if (ask.options.all { it.description == null && it.preview == null }) { + // Nothing to read, so nothing to lay out: Allow and Deny are two words, and two words + // do not need a card each. + AnswerOptions(ask.options, onAnswer) + } else { + ask.options.forEach { option -> + OptionCard(option, selected = false) { onAnswer(listOf(option.label)) } } + } OtherAnswer(onAnswer) } } @@ -162,12 +97,12 @@ private fun AskedQuestionCard( /** * Options that can be chosen together, with one button to send them. * - * The answer goes back as a string whatever the question, so several choices are one line: the - * labels in the order they were offered. That is the tool's own schema -- its `answers` map is - * string-valued -- rather than a guess about how a list would be encoded. + * The answer goes back as the list it is. What a provider makes of several answers is decided where + * that provider is spoken to -- Claude Code's answers map holds a string, so they are joined there + * -- and nothing on this side has to know that. */ @Composable -private fun MultipleChoice(options: List, onAnswer: (String) -> Unit) { +private fun MultipleChoice(options: List, onAnswer: (List) -> Unit) { var chosen by remember { mutableStateOf(setOf()) } options.forEach { option -> OptionCard(option, selected = option.label in chosen) { @@ -176,9 +111,9 @@ private fun MultipleChoice(options: List, onAnswer: (String) -> Uni } Spacer(Modifier.height(4.dp)) OutlinedButton( - onClick = { - onAnswer(options.filter { it.label in chosen }.joinToString(", ") { it.label }) - }, + // In the order they were offered rather than the order they were tapped: the reader is + // answering a list, and it should read back as that list. + onClick = { onAnswer(options.map { it.label }.filter { it in chosen }) }, enabled = chosen.isNotEmpty(), ) { Text(if (chosen.size <= 1) "Send answer" else "Send ${chosen.size} answers") @@ -194,7 +129,7 @@ private fun MultipleChoice(options: List, onAnswer: (String) -> Uni * is one cue and it is unambiguous. */ @Composable -private fun OptionCard(option: AskedOption, selected: Boolean, onPick: () -> Unit) { +private fun OptionCard(option: QuestionOption, selected: Boolean, onPick: () -> Unit) { OutlinedCard( onClick = onPick, modifier = Modifier.fillMaxWidth().padding(top = 6.dp), @@ -228,7 +163,13 @@ private fun OptionCard(option: AskedOption, selected: Boolean, onPick: () -> Uni } } -/** An option's worked example, shown as written. */ +/** + * An option's worked example, shown as written. + * + * On its own surface, because it is a different kind of thing from the sentence above it: that + * describes the option, this is a sample of what the option produces, and monospace alone reads as + * a description that happens to be in code font. + */ @Composable private fun Preview(preview: String) { Surface( @@ -248,14 +189,14 @@ private fun Preview(preview: String) { } /** - * The choice the harness always adds, and the app has to as well. + * The choice the asker always leaves open, and the app has to as well. * * Every AskUserQuestion carries an implicit "Other" -- the reader may answer in their own words * rather than pick. Leaving it out narrows a question that was never that narrow, and the reader * cannot tell that it was ever open. */ @Composable -private fun OtherAnswer(onAnswer: (String) -> Unit) { +private fun OtherAnswer(onAnswer: (List) -> Unit) { var text by remember { mutableStateOf("") } Row(Modifier.fillMaxWidth().padding(top = 8.dp)) { OutlinedTextField( @@ -265,14 +206,14 @@ private fun OtherAnswer(onAnswer: (String) -> Unit) { singleLine = true, modifier = Modifier.weight(1f), ) - TextButton(onClick = { onAnswer(text.trim()) }, enabled = text.isNotBlank()) { + TextButton(onClick = { onAnswer(listOf(text.trim())) }, enabled = text.isNotBlank()) { Text("Send") } } } /** - * The answers to a question, wrapped rather than in a row. + * Bare options, wrapped rather than in a row. * * A Row hands out intrinsic widths in order and clips whatever runs past the edge, so a question * with four options showed the first one or two and dropped the rest off the side of the screen. @@ -280,14 +221,14 @@ private fun OtherAnswer(onAnswer: (String) -> Unit) { * way for a list of choices to be wrong. */ @Composable -fun AnswerOptions(options: List, onAnswer: (String) -> Unit) { +fun AnswerOptions(options: List, onAnswer: (List) -> Unit) { FlowRow( horizontalArrangement = Arrangement.spacedBy(8.dp), verticalArrangement = Arrangement.spacedBy(4.dp), modifier = Modifier.fillMaxWidth(), ) { options.forEach { option -> - OutlinedButton(onClick = { onAnswer(option) }) { Text(option) } + OutlinedButton(onClick = { onAnswer(listOf(option.label)) }) { Text(option.label) } } } } diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Events.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Events.kt index 129712f..9f12332 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Events.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Events.kt @@ -10,6 +10,15 @@ import org.json.JSONObject /** One transcript line: the event plus its resume cursor and time. */ data class SeqEvent(val seq: Long, val ts: Double, val event: SessionEvent) +/** + * One choice offered in answer to a question. + * + * More than a label because the reader is deciding rather than confirming: what an option means, + * and what picking it would produce, are the things that decide it. Both are absent on a + * permission, whose Allow and Deny mean exactly what they say. + */ +data class QuestionOption(val label: String, val description: String?, val preview: String?) + sealed class SessionEvent { data class UserMessage(val text: String) : SessionEvent() @@ -30,12 +39,17 @@ sealed class SessionEvent { data class Question( val id: String, val prompt: String, - val options: List, + /** A few words naming what the question is about, when the asker offered one. */ + val header: String?, + val options: List, + /** Whether several options may be chosen at once. */ + val multiSelect: Boolean, /** The tool call this is permission for, or null when it is not about one. */ val about: String?, ) : SessionEvent() - data class Answered(val id: String, val answer: String) : SessionEvent() + /** Everything chosen for one question, in the order it was offered. */ + data class Answered(val id: String, val answers: List) : SessionEvent() /** * A message another agent sent this session. @@ -106,13 +120,28 @@ fun parseSeqEvent(json: String): SeqEvent { SessionEvent.Question( id = body.getString("id"), prompt = body.getString("prompt"), + header = body.optString("header").ifEmpty { null }, options = body.getJSONArray("options").let { options -> - (0 until options.length()).map { options.getString(it) } + (0 until options.length()).map { at -> + val option = options.getJSONObject(at) + QuestionOption( + label = option.getString("label"), + description = option.optString("description").ifEmpty { null }, + preview = option.optString("preview").ifEmpty { null }, + ) + } }, + multiSelect = body.optBoolean("multiSelect", false), about = body.optString("about").ifEmpty { null }, ) - "answered" -> SessionEvent.Answered(body.getString("id"), body.getString("answer")) + "answered" -> + SessionEvent.Answered( + body.getString("id"), + body.getJSONArray("answers").let { answers -> + (0 until answers.length()).map { answers.getString(it) } + }, + ) "peerMessage" -> SessionEvent.PeerMessage(body.getString("from"), body.getString("text")) "status" -> SessionEvent.Status(body.getString("state")) diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt index cf52c9f..071d769 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt @@ -121,8 +121,13 @@ sealed class TranscriptItem { override val seq: Long, val id: String, val prompt: String, - val options: List, - val answer: String?, + /** A few words naming what this is about, when the asker offered one. */ + val header: String?, + val options: List, + /** Whether several options may be chosen at once. */ + val multiSelect: Boolean, + /** What was chosen, once something was; empty until then. */ + val answers: List, ) : TranscriptItem() data class ErrorMsg(override val seq: Long, val message: String) : TranscriptItem() @@ -207,8 +212,10 @@ fun foldEvent(items: List, entry: SeqEvent): List, entry: SeqEvent): List - it.copy(answer = event.answer) + it.copy(answers = event.answers) it is TranscriptItem.ToolRun && it.asks.any { ask -> ask.id == event.id } -> it.copy( asks = it.asks.map { ask -> - if (ask.id == event.id) ask.copy(answer = event.answer) else ask + if (ask.id == event.id) ask.copy(answers = event.answers) + else ask } ) else -> it @@ -806,8 +814,10 @@ fun SessionScreen( if (id in expandedTools) expandedTools - id else expandedTools + id }, - onAnswer = { questionId, answer -> - act { answerQuestion(settings, summary.id, questionId, answer) } + onAnswer = { questionId, answers -> + act { + answerQuestion(settings, summary.id, questionId, answers) + } }, image = { ref -> SessionImage(settings, summary.id, ref) }, ) @@ -825,22 +835,22 @@ fun SessionScreen( expandedTools - item.id else expandedTools + item.id }, - onAnswer = { questionId, answer -> + onAnswer = { questionId, answers -> act { answerQuestion( settings, summary.id, questionId, - answer, + answers, ) } }, image = { ref -> SessionImage(settings, summary.id, ref) }, ) is TranscriptItem.QuestionCard -> - QuestionRow(item) { answer -> + QuestionRow(item) { answers -> act { - answerQuestion(settings, summary.id, item.id, answer) + answerQuestion(settings, summary.id, item.id, answers) } } is TranscriptItem.ErrorMsg -> @@ -1025,24 +1035,15 @@ private fun UserBubble(text: String, pending: Boolean = false) { * connected device. */ @Composable -private fun QuestionRow(question: TranscriptItem.QuestionCard, onAnswer: (String) -> Unit) { +private fun QuestionRow( + question: TranscriptItem.QuestionCard, + onAnswer: (List) -> Unit, +) { Card(Modifier.fillMaxWidth()) { Column(Modifier.padding(12.dp)) { - Text(question.prompt, style = MaterialTheme.typography.bodyLarge) - Spacer(Modifier.height(8.dp)) - if (question.answer != null) { - Text( - "Answered: ${question.answer}", - style = MaterialTheme.typography.labelLarge, - color = MaterialTheme.colorScheme.onSurfaceVariant, - ) - } else { - // Wrapped, not in a Row. A Row hands out intrinsic widths in order and clips - // whatever runs past the edge, so a question with four options showed the first - // one or two and silently dropped the rest off the side of the screen -- which - // does not look like a bug, it looks like those were the only choices. - AnswerOptions(question.options, onAnswer) - } + // The same body the questions on a tool call get: one question is the same + // thing whether or not something else asked it. + AskedQuestion(question, onAnswer) } } } diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/ToolRows.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/ToolRows.kt index 81beef0..66d3af6 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/ToolRows.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/ToolRows.kt @@ -102,7 +102,7 @@ fun ToolGroup( onToggle: () -> Unit, isToolExpanded: (String) -> Boolean, onToolToggle: (String) -> Unit, - onAnswer: (questionId: String, answer: String) -> Unit, + onAnswer: (questionId: String, answers: List) -> Unit, image: @Composable (String) -> Unit, ) { if (!expanded) { @@ -168,11 +168,11 @@ fun ToolCard( tool: TranscriptItem.ToolRun, expanded: Boolean, onToggle: () -> Unit, - onAnswer: (questionId: String, answer: String) -> Unit, + onAnswer: (questionId: String, answers: List) -> Unit, image: @Composable (String) -> Unit = {}, ) { val parsed = remember(tool.tool, tool.input) { parseToolInput(tool.tool, tool.input) } - val deciding = tool.asks.any { it.answer == null } + val deciding = tool.asks.any { it.answers.isEmpty() } val open = expanded || deciding Card(Modifier.fillMaxWidth().clickable(onClick = onToggle)) { Column(Modifier.padding(12.dp)) { @@ -246,10 +246,10 @@ fun ToolCard( tool.images.forEach { ref -> image(ref) } if (tool.asks.isNotEmpty()) { if (tool.tool == ASK_USER_QUESTION) { - AskUserQuestionBody(tool.input, tool.asks, onAnswer) + AskUserQuestionBody(tool.asks, onAnswer) } else { tool.asks.forEach { ask -> - PermissionAsk(ask) { answer -> onAnswer(ask.id, answer) } + PermissionAsk(ask) { answers -> onAnswer(ask.id, answers) } } } } @@ -264,16 +264,16 @@ fun ToolCard( * the ask can stand alone, and here it does not have to -- the card above is showing exactly that. */ @Composable -private fun PermissionAsk(ask: TranscriptItem.QuestionCard, onAnswer: (String) -> Unit) { +private fun PermissionAsk(ask: TranscriptItem.QuestionCard, onAnswer: (List) -> Unit) { Spacer(Modifier.height(8.dp)) Text( ask.prompt.substringBefore('\n'), style = MaterialTheme.typography.bodyMedium, color = awaitingColor, ) - if (ask.answer != null) { + if (ask.answers.isNotEmpty()) { Text( - "Answered: ${ask.answer}", + "Answered: ${ask.answers.joinToString(", ")}", style = MaterialTheme.typography.labelLarge, color = MaterialTheme.colorScheme.onSurfaceVariant, ) diff --git a/server/src/routes.rs b/server/src/routes.rs index 7fbaab0..276e943 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -15,7 +15,7 @@ //! (a backlog past CATCH_UP_LIMIT arrives as a //! `reset` frame plus the newest window) //! POST /sessions/{id}/message {text, attachmentIds?} -//! POST /sessions/{id}/answer {questionId, answer} (questions and permissions) +//! POST /sessions/{id}/answer {questionId, answers} (questions and permissions) //! POST /sessions/{id}/interrupt //! POST /sessions/{id}/title {title} //! POST /sessions/{id}/model {model} @@ -620,7 +620,11 @@ async fn message( #[serde(deny_unknown_fields)] struct AnswerRequest { question_id: String, - answer: String, + /// Everything chosen, in the order it was offered. A question that + /// takes one answer sends a list of one, so there is one shape here + /// rather than a single-answer route and a multi-answer route beside + /// it. + answers: Vec, } async fn answer( @@ -628,7 +632,12 @@ async fn answer( UrlPath(id): UrlPath, axum::Json(body): axum::Json, ) -> Result { - lookup(&manager, &id)?.answer_question(&body.question_id, &body.answer); + if body.answers.is_empty() { + return Err(bad_request(anyhow::anyhow!( + "an answer needs at least one choice" + ))); + } + lookup(&manager, &id)?.answer_question(&body.question_id, &body.answers); Ok(StatusCode::NO_CONTENT) } diff --git a/server/src/session/claude.rs b/server/src/session/claude.rs index 4ba9909..680a814 100644 --- a/server/src/session/claude.rs +++ b/server/src/session/claude.rs @@ -473,10 +473,10 @@ impl Driver for ClaudeDriver { self.send_line(line); } - fn answer_question(&self, id: &str, answer: &str) { + fn answer_question(&self, id: &str, answers: &[String]) { let response = { let mut state = self.state.lock().unwrap(); - state.answer(id, answer) + state.answer(id, answers) }; match response { AnswerOutcome::Respond(control_response) => { diff --git a/server/src/session/claude/translate.rs b/server/src/session/claude/translate.rs index cbb6595..89bb6c4 100644 --- a/server/src/session/claude/translate.rs +++ b/server/src/session/claude/translate.rs @@ -17,7 +17,7 @@ use std::path::{Path, PathBuf}; use serde_json::{Value, json}; -use super::super::driver::{Event, SessionStatus}; +use super::super::driver::{Event, QuestionOption, SessionStatus}; /// What answering a question produced. pub(super) enum AnswerOutcome { @@ -365,18 +365,33 @@ impl Translator { .and_then(Value::as_str) .unwrap_or("(question)") .to_string(); + // Everything the reader decides on, carried in the event. + // The alternative -- and what this was -- is the phone + // reaching into the tool call's input for the parts the + // event dropped, which puts this dialect's schema in the + // app where no other dialect can reach it. let options = question .get("options") .and_then(Value::as_array) .into_iter() .flatten() - .filter_map(|option| option.get("label").and_then(Value::as_str)) - .map(String::from) + .filter_map(|option| { + Some(QuestionOption { + label: option.get("label").and_then(Value::as_str)?.to_string(), + description: text_field(option, "description"), + preview: text_field(option, "preview"), + }) + }) .collect(); events.push(Event::Question { id: format!("{request_id}#{i}"), prompt: text.clone(), + header: text_field(question, "header"), options, + multi_select: question + .get("multiSelect") + .and_then(Value::as_bool) + .unwrap_or(false), // The call that is asking, so all of this draws as one // thing. It used to be `None` on the grounds that a // question the model asked is not permission for a @@ -394,7 +409,14 @@ impl Translator { events.push(Event::Question { id: request_id.clone(), prompt: format!("Allow {tool_name}?\n{summary}"), - options: vec!["Allow".to_string(), "Deny".to_string()], + // No header: the question is about the call it names, and + // the phone draws it on that call's own row. + header: None, + options: vec![ + QuestionOption::plain("Allow"), + QuestionOption::plain("Deny"), + ], + multi_select: false, about: about.clone(), }); } @@ -415,7 +437,13 @@ impl Translator { /// Applies one answer from the phone. Question ids are the control /// request id, suffixed `#i` for AskUserQuestion sub-questions. - pub(super) fn answer(&mut self, question_id: &str, answer: &str) -> AnswerOutcome { + pub(super) fn answer(&mut self, question_id: &str, answers: &[String]) -> AnswerOutcome { + // Where this dialect's shape is put on: the CLI's `answers` map is + // string-valued whatever the question, so several choices become + // one line here rather than everything upstream pretending a + // question can only ever have one answer. + let answer = answers.join(", "); + let answer = answer.as_str(); let (request_id, sub) = match question_id.split_once('#') { Some((request_id, index)) => (request_id, index.parse::().ok()), None => (question_id, None), @@ -514,6 +542,19 @@ impl Translator { } } +/// A string field that is there and not empty, or `None`. +/// +/// The CLI omits these rather than sending them empty, but a caller that +/// sends `""` means the same thing and should not produce a description +/// that draws as a blank line. +fn text_field(value: &Value, name: &str) -> Option { + value + .get(name) + .and_then(Value::as_str) + .filter(|text| !text.trim().is_empty()) + .map(str::to_string) +} + /// Decodes one base64 image block into `files/` and returns its ref. /// /// A free function rather than a method because the import replay needs @@ -551,6 +592,15 @@ pub(in crate::session) fn save_image(session_dir: &Path, part: &Value) -> Option mod tests { use super::*; + /// What a phone sends back: everything chosen, even when that is one. + fn chose(answer: &str) -> Vec { + vec![answer.to_string()] + } + + fn labels(options: &[QuestionOption]) -> Vec<&str> { + options.iter().map(|option| option.label.as_str()).collect() + } + fn translate_lines(translator: &mut Translator, lines: &[&str]) -> Vec { lines .iter() @@ -754,6 +804,7 @@ mod tests { prompt, options, about, + .. } = &events[0] else { panic!("expected a question, got {events:?}"); @@ -763,7 +814,7 @@ mod tests { // tool's row instead of as a second card repeating its input. assert_eq!(about.as_deref(), Some("toolu_03")); assert!(prompt.contains("Bash") && prompt.contains("rm -rf /tmp/x")); - assert_eq!(options, &["Allow", "Deny"]); + assert_eq!(labels(options), ["Allow", "Deny"]); assert_eq!( events[1], Event::Status { @@ -772,7 +823,7 @@ mod tests { ); // Allowing echoes the input back; the request is then gone. - let AnswerOutcome::Respond(response) = translator.answer("req-1", "Allow") else { + let AnswerOutcome::Respond(response) = translator.answer("req-1", &chose("Allow")) else { panic!("expected a control response"); }; assert_eq!(response["response"]["request_id"], "req-1"); @@ -782,7 +833,7 @@ mod tests { "rm -rf /tmp/x" ); assert!(matches!( - translator.answer("req-1", "Allow"), + translator.answer("req-1", &chose("Allow")), AnswerOutcome::Unknown )); } @@ -797,7 +848,7 @@ mod tests { r#"{"type":"control_request","request_id":"req-2","request":{"subtype":"can_use_tool","tool_name":"Write","input":{"file_path":"/etc/passwd"}}}"#, ], ); - let AnswerOutcome::Respond(response) = translator.answer("req-2", "Deny") else { + let AnswerOutcome::Respond(response) = translator.answer("req-2", &chose("Deny")) else { panic!("expected a control response"); }; assert_eq!(response["response"]["response"]["behavior"], "deny"); @@ -822,8 +873,9 @@ mod tests { id, prompt, options, + multi_select, .. - } => Some((id.clone(), prompt.clone(), options.clone())), + } => Some((id.clone(), prompt.clone(), options.clone(), *multi_select)), _ => None, }) .collect(); @@ -835,15 +887,15 @@ mod tests { _ => true, })); assert_eq!(questions[0].1, "Which color?"); - assert_eq!(questions[0].2, vec!["Red", "Blue"]); + assert_eq!(labels(&questions[0].2), ["Red", "Blue"]); // First answer alone isn't enough; the response goes out when the // last sub-question is answered, with all answers aboard. assert!(matches!( - translator.answer("req-3#0", "Blue"), + translator.answer("req-3#0", &chose("Blue")), AnswerOutcome::Pending )); - let AnswerOutcome::Respond(response) = translator.answer("req-3#1", "L") else { + let AnswerOutcome::Respond(response) = translator.answer("req-3#1", &chose("L")) else { panic!("expected a control response"); }; let updated = &response["response"]["response"]["updatedInput"]; @@ -852,6 +904,59 @@ mod tests { assert_eq!(updated["questions"][0]["question"], "Which color?"); } + #[test] + fn a_question_carries_what_it_takes_to_answer_it() { + // Descriptions and previews are what the reader decides on, and a + // multi-select is how many answers the question takes. All of it + // travels in the event: a phone that had to read this dialect's + // tool input to find them would be the only place that knew how, + // and no other provider could reach it. + let dir = tempfile::tempdir().expect("tempdir"); + let mut translator = Translator::new(dir.path().to_path_buf()); + let events = translate_lines( + &mut translator, + &[ + r#"{"type":"control_request","request_id":"req-9","request":{"subtype":"can_use_tool","tool_name":"AskUserQuestion","input":{"questions":[{"question":"Which to collapse?","header":"Collapsed","multiSelect":true,"options":[{"label":"Tool calls","description":"A run becomes one card."},{"label":"Peer messages","description":"From other agents.","preview":"from: dev-updater\\npull before you touch it"}]}]},"tool_use_id":"toolu_09"}}"#, + ], + ); + let Event::Question { + header, + options, + multi_select, + .. + } = &events[0] + else { + panic!("expected a question, got {events:?}"); + }; + assert_eq!(header.as_deref(), Some("Collapsed")); + assert!(multi_select); + assert_eq!( + options[0].description.as_deref(), + Some("A run becomes one card.") + ); + assert!(options[0].preview.is_none()); + assert!( + options[1] + .preview + .as_deref() + .unwrap() + .contains("dev-updater") + ); + + // Two choices, one answer: the joining is this dialect's shape, + // done where it is spoken. The CLI's answers map holds strings. + let AnswerOutcome::Respond(response) = translator.answer( + "req-9#0", + &["Tool calls".to_string(), "Peer messages".to_string()], + ) else { + panic!("expected a control response"); + }; + assert_eq!( + response["response"]["response"]["updatedInput"]["answers"]["Which to collapse?"], + "Tool calls, Peer messages" + ); + } + #[test] fn images_in_tool_results_are_saved_and_referenced() { let dir = tempfile::tempdir().expect("tempdir"); diff --git a/server/src/session/driver.rs b/server/src/session/driver.rs index 140cf3f..66b63aa 100644 --- a/server/src/session/driver.rs +++ b/server/src/session/driver.rs @@ -15,6 +15,35 @@ use tokio::sync::mpsc; /// directions use the one id so the transcript renders them identically. pub type ImageRef = String; +/// One choice offered in answer to a [`Event::Question`]. +/// +/// More than a label because the reader is deciding, not confirming: what +/// an option means, and what picking it would produce, are the things that +/// decide it. Both are optional -- a permission's Allow and Deny mean +/// exactly what they say. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct QuestionOption { + pub label: String, + /// A sentence about what this option means. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub description: Option, + /// A block to show as written -- a mockup, a diff, a config file. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub preview: Option, +} + +impl QuestionOption { + /// An option that is only its label, which is most of them. + pub fn plain(label: impl Into) -> Self { + Self { + label: label.into(), + description: None, + preview: None, + } + } +} + /// Everything a session can tell the outside world. Every event is /// appended to the session's transcript with a sequence number, then fanned /// out to SSE subscribers; the phone renders purely from this stream, so @@ -93,7 +122,22 @@ pub enum Event { Question { id: String, prompt: String, - options: Vec, + /// A few words naming what the question is about, when the asker + /// offered one -- a tag beside the question rather than part of + /// it. `None` for a permission, which is about the call above it. + #[serde(default, skip_serializing_if = "Option::is_none")] + header: Option, + options: Vec, + /// Whether several options may be chosen at once. + /// + /// Here rather than left for a phone to work out from the dialect + /// underneath: how many answers a question takes is a fact about + /// the question, and the alternative was the app parsing Claude + /// Code's tool input to find out -- one dialect's schema, written + /// out a second time in Kotlin, where no other dialect could + /// reach it. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + multi_select: bool, /// The tool call this is permission for, when it is one. /// /// The CLI's `can_use_tool` request carries the `tool_use_id` of @@ -120,9 +164,15 @@ pub enum Event { /// The manager's record of a question being answered, so a rendered /// question card resolves on every device, not just the one that /// answered it. + /// + /// A list because a question can take several answers, and one that + /// took one is the list of length one rather than a different shape. + /// What a dialect makes of that -- Claude Code's answers map holds a + /// string, so several become one line -- is that dialect's business + /// and is done where it talks to it. Answered { id: String, - answer: String, + answers: Vec, }, Status { state: SessionStatus, @@ -217,7 +267,10 @@ pub trait Driver: Send + Sync { /// message in the transcript, so a driver that never sends it drops /// the message from the conversation entirely. fn send_user_message(&self, text: String, images: Vec); - fn answer_question(&self, id: &str, answer: &str); + /// Answers one question with everything that was chosen, in the order + /// it was offered. One answer is a list of one; a driver whose dialect + /// takes a single value joins them where it writes it. + fn answer_question(&self, id: &str, answers: &[String]); /// Stop mid-run; the session survives. fn interrupt(&self); fn set_model(&self, model: &str); diff --git a/server/src/session/echo.rs b/server/src/session/echo.rs index 8b02905..669c515 100644 --- a/server/src/session/echo.rs +++ b/server/src/session/echo.rs @@ -37,7 +37,7 @@ use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; use std::time::Duration; -use super::driver::{Driver, Event, EventSink, ImageRef, SessionStatus}; +use super::driver::{Driver, Event, EventSink, ImageRef, QuestionOption, SessionStatus}; /// Delay between streamed deltas -- long enough that streaming is visibly /// streaming in the UI, short enough that tests waiting on a full turn @@ -91,74 +91,102 @@ impl EchoDriver { /// easy to leave out of a fixture -- a header, an option with a /// description, an option with a preview block, and a multi-select. fn ask_user_question(&self) { + // Written once, in the shape the events carry, and turned into + // the tool call's own input below -- the CLI sends both, and two + // hand-written copies of one question would drift. + let asked = [ + ( + "Theme", + "Which colour scheme should the transcript use?", + false, + vec![ + QuestionOption { + label: "Catppuccin Mocha (Recommended)".to_string(), + description: Some( + "What the app uses now: a dark base with muted accents.".to_string(), + ), + preview: None, + }, + QuestionOption { + label: "Solarized Dark".to_string(), + description: Some( + "Lower contrast, warmer. Easier at night, harder in sun.".to_string(), + ), + preview: None, + }, + QuestionOption { + label: "High contrast".to_string(), + description: Some( + "Pure black behind white text, for reading outdoors.".to_string(), + ), + preview: Some( + "background: #000000\nforeground: #ffffff\naccent: #ffd700" + .to_string(), + ), + }, + ], + ), + ( + "Collapsed", + "Which of these should be shown collapsed by default?", + true, + vec![ + QuestionOption { + label: "Tool calls".to_string(), + description: Some("A run of them becomes one card.".to_string()), + preview: None, + }, + QuestionOption { + label: "Peer messages".to_string(), + description: Some("Messages from other agents.".to_string()), + preview: None, + }, + QuestionOption { + label: "Compaction notes".to_string(), + description: Some("What a compaction recovered.".to_string()), + preview: None, + }, + ], + ), + ]; + let call = format!("echo-ask-{}", super::random_hex()); - let questions = serde_json::json!({"questions": [ - { - "question": "Which colour scheme should the transcript use?", - "header": "Theme", - "multiSelect": false, - "options": [ - {"label": "Catppuccin Mocha (Recommended)", - "description": "What the app uses now: a dark base with muted accents."}, - {"label": "Solarized Dark", - "description": "Lower contrast, warmer. Easier at night, harder in sun."}, - {"label": "High contrast", - "description": "Pure black behind white text, for reading outdoors.", - "preview": "background: #000000\nforeground: #ffffff\naccent: #ffd700"}, - ], - }, - { - "question": "Which of these should be shown collapsed by default?", - "header": "Collapsed", - "multiSelect": true, - "options": [ - {"label": "Tool calls", "description": "A run of them becomes one card."}, - {"label": "Peer messages", "description": "Messages from other agents."}, - {"label": "Compaction notes", "description": "What a compaction recovered."}, - ], - }, - ]}); self.emit(Event::Status { state: SessionStatus::Running, }); self.emit(Event::ToolStart { id: call.clone(), tool: "AskUserQuestion".to_string(), - input: questions, + input: serde_json::json!({"questions": asked + .iter() + .map(|(header, question, multi, options)| serde_json::json!({ + "question": question, + "header": header, + "multiSelect": multi, + "options": options, + })) + .collect::>()}), }); - let mut pending = self.pending_questions.lock().unwrap(); - for (index, question) in [ - ( - "Which colour scheme should the transcript use?", - vec![ - "Catppuccin Mocha (Recommended)", - "Solarized Dark", - "High contrast", - ], - ), - ( - "Which of these should be shown collapsed by default?", - vec!["Tool calls", "Peer messages", "Compaction notes"], - ), - ] - .into_iter() - .enumerate() - { + for (index, (header, question, multi, options)) in asked.into_iter().enumerate() { let id = format!("{call}#{index}"); - pending.push(PendingQuestion { - id: id.clone(), - call: Some(call.clone()), - }); + self.pending_questions + .lock() + .unwrap() + .push(PendingQuestion { + id: id.clone(), + call: Some(call.clone()), + }); self.emit(Event::Question { id, - prompt: question.0.to_string(), - options: question.1.into_iter().map(str::to_string).collect(), + prompt: question.to_string(), + header: Some(header.to_string()), + options, + multi_select: multi, // The call that asked, so all of it draws as one thing -- // which is the whole point of the fixture. about: Some(call.clone()), }); } - drop(pending); self.emit(Event::Status { state: SessionStatus::AwaitingInput, }); @@ -277,7 +305,9 @@ impl Driver for EchoDriver { self.emit(Event::Question { id, prompt, - options: vec!["Yes".to_string(), "No".to_string()], + header: None, + options: vec![QuestionOption::plain("Yes"), QuestionOption::plain("No")], + multi_select: false, about: None, }); self.emit(Event::Status { @@ -412,7 +442,8 @@ impl Driver for EchoDriver { }); } - fn answer_question(&self, id: &str, answer: &str) { + fn answer_question(&self, id: &str, answers: &[String]) { + let answer = answers.join(", "); let (answered, waiting) = { let mut pending = self.pending_questions.lock().unwrap(); let Some(at) = pending.iter().position(|question| question.id == id) else { diff --git a/server/src/session/llama.rs b/server/src/session/llama.rs index 1acb859..a969256 100644 --- a/server/src/session/llama.rs +++ b/server/src/session/llama.rs @@ -371,7 +371,7 @@ impl Driver for LlamaDriver { }); } - fn answer_question(&self, _id: &str, _answer: &str) { + fn answer_question(&self, _id: &str, _answers: &[String]) { // Nothing here asks questions: this driver has no tools, so no // permission prompts and no AskUserQuestion. } diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index 76c92e0..a5aad1a 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -163,12 +163,12 @@ impl LiveSession { self.driver.send_user_message(text, images); } - pub fn answer_question(&self, question_id: &str, answer: &str) { + pub fn answer_question(&self, question_id: &str, answers: &[String]) { let _ = self.sink.send(Event::Answered { id: question_id.to_string(), - answer: answer.to_string(), + answers: answers.to_vec(), }); - self.driver.answer_question(question_id, answer); + self.driver.answer_question(question_id, answers); } pub fn interrupt(&self) { @@ -1360,11 +1360,11 @@ mod tests { }) .expect("question event"); - session.answer_question(&question_id, "Yes"); + session.answer_question(&question_id, &["Yes".to_string()]); let seen = collect_until(&mut rx, is_idle).await; assert!(seen.iter().any(|entry| matches!( &entry.event, - Event::Answered { id, answer } if *id == question_id && answer == "Yes" + Event::Answered { id, answers } if *id == question_id && answers == &["Yes".to_string()] ))); } diff --git a/server/src/session/transcript.rs b/server/src/session/transcript.rs index 722873e..a1c0091 100644 --- a/server/src/session/transcript.rs +++ b/server/src/session/transcript.rs @@ -186,7 +186,7 @@ pub fn read_after(path: &Path, after: u64) -> Result> { #[cfg(test)] mod tests { use super::*; - use crate::session::driver::SessionStatus; + use crate::session::driver::{QuestionOption, SessionStatus}; fn text(delta: &str) -> Event { Event::AssistantText { @@ -337,12 +337,14 @@ mod tests { Event::Question { id: "q1".into(), prompt: "Allow?".into(), - options: vec!["Yes".into(), "No".into()], + header: None, + options: vec![QuestionOption::plain("Yes"), QuestionOption::plain("No")], + multi_select: false, about: None, }, Event::Answered { id: "q1".into(), - answer: "Yes".into(), + answers: vec!["Yes".into()], }, Event::Status { state: SessionStatus::Idle,