Carry a question in the event model, not in one provider's JSON
A question is now fully described by the event that reports it: the tag it was asked under, each option's label, what it means, and the sample of what picking it would produce, plus whether several may be picked at once. The app renders from that alone. It had been reading Claude Code's tool input to find the parts the event dropped -- that dialect's schema, written out a second time in Kotlin, where no other provider could reach it and where it would drift the first time the schema moved. Echo could not describe an option at all, and llama never will. Answers travel as a list for the same reason. A question that takes one answer sends a list of one rather than being a different shape, and the one place that flattens it is where the CLI is spoken to: its answers map holds a string, so several choices are joined there. That join was in the phone. Also here because it is the same rule: the permission ask reuses the question body rather than owning a second one, so Allow/Deny renders and resolves through exactly the code an AskUserQuestion does. Verified against both, since a refactor that only satisfies the case it was written for has been tried on the half that cannot fail: a two question `/ask` answered from the phone, one option and then two, and a real sonnet session's `rm -f` permission asked, allowed, and run.
This commit is contained in:
1 parent
fea8e7e92b
commit
bebaae7a94
13 files changed
+419
-237
No files matched your search
@@ -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::<usize>().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<String> {
|
||||
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<String> {
|
||||
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<Event> {
|
||||
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");
|
||||
|
||||
Reference in new issue
Block a user