Send a steer into the running turn, and put an image under its call
**The queue was holding messages the CLI would have taken.** Two claims in this file contradicted each other: the module header said a mid-turn message is injected at the next tool boundary -- "the behavior this app exists for" -- and `Queue`'s own doc said a line written mid-turn simply becomes the next turn. The code followed the second, parking every message until `Status::Idle`, which is the end of the whole turn. Measured rather than argued, twice. Writing a line straight into a live session's stdin fifo mid-turn produced one `result` for the whole thing, so it was consumed inside that turn, not as a new one. The header was right and the queue was built on the wrong claim. The cost was exactly what Bryan reported: he steered after the second tool call and it sat unread until every remaining call had finished. Measured before and after on the same three-step turn -- steer sent at +13s, recorded at +24.7s before this change and at +14.1s after, which is the next tool boundary. So the line goes out immediately. What stays behind is the *announcement*: the CLI says nothing on stdout about having read a message, so `MessageTaken` now waits for the next assistant text or tool call, which is proof another model call happened and the steer was in it. That keeps a held message drawn below the working indicator until the session has actually taken it -- the thing that mattered when this was last changed -- without delaying the message to get it. Idle counts too, and is the case that must not be missed: a message written after a turn's last model call has no later output to prove anything. `closed` is untouched, and `Queue::close` still reports held messages by name rather than dropping them. **An image now names the call that produced it.** `Event::Image` gains `about`, the `tool_use_id` from the tool result it came out of, so a screenshot is drawn inside that call's card instead of floating beside it -- pairing them by position is what a page boundary breaks. `None` for a person's own attachment, which belongs to no call. The import path threads it through as well, so replayed history reads the same as live. Images show whether the card is open or closed: a call whose result *is* a picture says less closed than the one line it replaced. Verified on the emulator against a real haiku turn: the checkerboard sits inside `Read /tmp/tiny.png`, and the steer sits between that call and the next, where it was taken. 53 tests, clippy, rustfmt, lint and ktfmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
184b6fc6a6
commit
42131c75d6
9 files changed
+164
-56
No files matched your search
@@ -21,7 +21,11 @@ sealed class SessionEvent {
|
||||
|
||||
data class ToolEnd(val id: String, val output: String) : SessionEvent()
|
||||
|
||||
data class Image(val ref: String) : SessionEvent()
|
||||
data class Image(
|
||||
val ref: String,
|
||||
/** The tool call whose result carried it, or null for a person's own attachment. */
|
||||
val about: String?,
|
||||
) : SessionEvent()
|
||||
|
||||
data class Question(
|
||||
val id: String,
|
||||
@@ -62,7 +66,11 @@ fun parseSeqEvent(json: String): SeqEvent {
|
||||
)
|
||||
"toolUpdate" -> SessionEvent.ToolUpdate(body.getString("id"), body.getString("output"))
|
||||
"toolEnd" -> SessionEvent.ToolEnd(body.getString("id"), body.getString("output"))
|
||||
"image" -> SessionEvent.Image(body.getString("ref"))
|
||||
"image" ->
|
||||
SessionEvent.Image(
|
||||
ref = body.getString("ref"),
|
||||
about = body.optString("about").ifEmpty { null },
|
||||
)
|
||||
"question" ->
|
||||
SessionEvent.Question(
|
||||
id = body.getString("id"),
|
||||
|
||||
@@ -87,6 +87,13 @@ sealed class TranscriptItem {
|
||||
* a fact rather than a match on the input.
|
||||
*/
|
||||
val ask: QuestionCard? = null,
|
||||
/**
|
||||
* Images this call's result carried, drawn under it.
|
||||
*
|
||||
* Beside it they had to be paired by position, and position is the thing a page boundary
|
||||
* breaks -- a screenshot loaded on one page and its call on the next read as unrelated.
|
||||
*/
|
||||
val images: List<String> = emptyList(),
|
||||
) : TranscriptItem()
|
||||
|
||||
data class QuestionCard(
|
||||
@@ -161,7 +168,19 @@ fun foldEvent(items: List<TranscriptItem>, event: SessionEvent): List<Transcript
|
||||
}
|
||||
is SessionEvent.Status -> items
|
||||
is SessionEvent.Error -> items + TranscriptItem.ErrorMsg(event.message)
|
||||
is SessionEvent.Image -> items + TranscriptItem.ImageItem(event.ref)
|
||||
is SessionEvent.Image ->
|
||||
// Under the call that produced it when there is one, and a row of
|
||||
// its own when there is not -- a person's own attachment belongs
|
||||
// to no call, and neither does one whose call fell outside the
|
||||
// loaded window.
|
||||
if (
|
||||
event.about != null &&
|
||||
items.any { it is TranscriptItem.ToolRun && it.id == event.about }
|
||||
) {
|
||||
updateTool(items, event.about) { it.copy(images = it.images + event.ref) }
|
||||
} else {
|
||||
items + TranscriptItem.ImageItem(event.ref)
|
||||
}
|
||||
is SessionEvent.Unknown -> items + TranscriptItem.Note("[${event.type}]")
|
||||
// Screen-level state, not transcript rows -- see SessionScreen.
|
||||
is SessionEvent.UsageDelta -> items
|
||||
@@ -608,6 +627,7 @@ fun SessionScreen(
|
||||
act { answerQuestion(settings, summary.id, ask.id, answer) }
|
||||
}
|
||||
},
|
||||
image = { ref -> SessionImage(settings, summary.id, ref) },
|
||||
)
|
||||
is TranscriptRow.Single ->
|
||||
when (val item = row.item) {
|
||||
@@ -635,6 +655,7 @@ fun SessionScreen(
|
||||
}
|
||||
}
|
||||
},
|
||||
image = { ref -> SessionImage(settings, summary.id, ref) },
|
||||
)
|
||||
is TranscriptItem.QuestionCard ->
|
||||
QuestionRow(item) { answer ->
|
||||
|
||||
@@ -90,6 +90,7 @@ fun ToolGroup(
|
||||
isToolExpanded: (String) -> Boolean,
|
||||
onToolToggle: (String) -> Unit,
|
||||
onAnswer: (TranscriptItem.ToolRun, String) -> Unit,
|
||||
image: @Composable (String) -> Unit,
|
||||
) {
|
||||
if (!expanded) {
|
||||
Card(Modifier.fillMaxWidth().clickable(onClick = onToggle)) {
|
||||
@@ -113,6 +114,7 @@ fun ToolGroup(
|
||||
expanded = isToolExpanded(call.id),
|
||||
onToggle = { onToolToggle(call.id) },
|
||||
onAnswer = { answer -> onAnswer(call, answer) },
|
||||
image = image,
|
||||
)
|
||||
}
|
||||
CollapseBar(onToggle)
|
||||
@@ -154,6 +156,7 @@ fun ToolCard(
|
||||
expanded: Boolean,
|
||||
onToggle: () -> Unit,
|
||||
onAnswer: (String) -> Unit,
|
||||
image: @Composable (String) -> Unit = {},
|
||||
) {
|
||||
val parsed = remember(tool.tool, tool.input) { parseToolInput(tool.tool, tool.input) }
|
||||
val deciding = tool.ask != null && tool.ask.answer == null
|
||||
@@ -207,6 +210,11 @@ fun ToolCard(
|
||||
Text(tool.output, style = MaterialTheme.typography.bodySmall)
|
||||
}
|
||||
}
|
||||
// Shown open or closed. A call that produced a picture is one
|
||||
// whose result *is* the picture, and a row that hides it says
|
||||
// less than the one line it replaced -- unlike a command, which
|
||||
// is what the closed line already summarises.
|
||||
tool.images.forEach { ref -> image(ref) }
|
||||
tool.ask?.let { ask -> PermissionAsk(ask, onAnswer) }
|
||||
}
|
||||
}
|
||||
|
||||
@@ -106,28 +106,36 @@ const RESUME_FILE: &str = "claude-session.json";
|
||||
/// does not stop anything, so it has no grace period and needs none.
|
||||
const STOP_GRACE: std::time::Duration = std::time::Duration::from_secs(5);
|
||||
|
||||
/// Messages sent while a turn was already running, and whether one is.
|
||||
/// Messages handed to the CLI that it has not visibly acted on yet.
|
||||
///
|
||||
/// The CLI does not inject a message into a turn in flight: a line written
|
||||
/// to its stdin mid-turn simply becomes the next turn, and it says nothing
|
||||
/// on stdout about having read it. So the wait is held here instead, where
|
||||
/// the moment it ends is a line *this* driver writes -- which is what lets
|
||||
/// the phone be told, and what puts the message in the transcript where it
|
||||
/// was read rather than where it was typed.
|
||||
/// The CLI *does* take a message written mid-turn: it goes into the next
|
||||
/// model call, which is the next tool boundary, and the whole point of
|
||||
/// this app is steering a turn that is already running. An earlier version
|
||||
/// of this file claimed the opposite and held every mid-turn message until
|
||||
/// the turn ended -- so a steer sent after the second tool call sat unread
|
||||
/// until all the work it was meant to redirect had finished. Measured
|
||||
/// rather than argued: a line written between two Bash calls was answered
|
||||
/// inside the same turn, with one `result` for the whole thing.
|
||||
///
|
||||
/// What the CLI does not do is say on stdout that it has read one. So the
|
||||
/// line goes out immediately and the *announcement* waits here instead:
|
||||
/// the next assistant text or tool call is proof another model call has
|
||||
/// happened, and the message was in it. That keeps a phone's held bubble
|
||||
/// where it belongs -- below the working indicator until the session has
|
||||
/// actually taken it -- without delaying the message itself to get it.
|
||||
#[derive(Default)]
|
||||
struct Queue {
|
||||
/// A turn is in flight, so anything sent now waits for it.
|
||||
/// A turn is in flight, so a message sent now is a steer into it.
|
||||
running: bool,
|
||||
/// Each held message as the text to report and the line to write.
|
||||
waiting: VecDeque<(String, String)>,
|
||||
/// Written, not yet announced, oldest first.
|
||||
awaiting: VecDeque<String>,
|
||||
/// The process is gone, so nothing can be taken up any more.
|
||||
///
|
||||
/// Needed because every other way out of a turn is an `Idle` this
|
||||
/// driver sees, and an exit is the one that is not. Without it a
|
||||
/// process that died mid-turn left `running` true for good: the queue
|
||||
/// then held every later message forever, and since a message is only
|
||||
/// recorded when it is *taken*, each one vanished with nothing on
|
||||
/// screen to say it had not been delivered.
|
||||
/// process that died mid-turn left `running` true for good, and since
|
||||
/// a message is only recorded when it is *announced*, each later one
|
||||
/// vanished with nothing on screen to say it had not been delivered.
|
||||
closed: bool,
|
||||
}
|
||||
|
||||
@@ -140,7 +148,7 @@ impl Queue {
|
||||
fn close(&mut self, sink: &EventSink, why: &str) {
|
||||
self.closed = true;
|
||||
self.running = false;
|
||||
let lost: Vec<String> = self.waiting.drain(..).map(|(text, _)| text).collect();
|
||||
let lost: Vec<String> = self.awaiting.drain(..).collect();
|
||||
if lost.is_empty() {
|
||||
return;
|
||||
}
|
||||
@@ -287,7 +295,6 @@ impl ClaudeDriver {
|
||||
Arc::clone(&state),
|
||||
sink.clone(),
|
||||
Arc::clone(&queue),
|
||||
to_child.clone(),
|
||||
Arc::clone(&reading),
|
||||
format!("{} {}", provider.name, transport.describe()),
|
||||
));
|
||||
@@ -419,11 +426,17 @@ impl Driver for ClaudeDriver {
|
||||
return;
|
||||
}
|
||||
if queue.running {
|
||||
queue.waiting.push_back((text, line));
|
||||
// Into the running turn, now. Announced when the CLI shows it
|
||||
// has been round the model again -- see `Queue`.
|
||||
queue.awaiting.push_back(text);
|
||||
drop(queue);
|
||||
self.send_line(line);
|
||||
return;
|
||||
}
|
||||
queue.running = true;
|
||||
drop(queue);
|
||||
// Nothing is in flight, so there is nothing to wait for: this
|
||||
// message *is* the turn about to start.
|
||||
let _ = self.sink.send(Event::MessageTaken { text });
|
||||
let _ = self.sink.send(Event::Status {
|
||||
state: SessionStatus::Running,
|
||||
@@ -516,7 +529,6 @@ async fn follow(
|
||||
state: Arc<Mutex<Translator>>,
|
||||
sink: EventSink,
|
||||
queue: Arc<Mutex<Queue>>,
|
||||
to_child: mpsc::UnboundedSender<String>,
|
||||
reading: Arc<AtomicBool>,
|
||||
label: String,
|
||||
) {
|
||||
@@ -577,7 +589,7 @@ async fn follow(
|
||||
if line.trim().is_empty() {
|
||||
continue;
|
||||
}
|
||||
if !translate_line(line, &session_dir, &state, &sink, &queue, &to_child) {
|
||||
if !translate_line(line, &session_dir, &state, &sink, &queue) {
|
||||
return; // session torn down
|
||||
}
|
||||
}
|
||||
@@ -666,7 +678,6 @@ fn translate_line(
|
||||
state: &Arc<Mutex<Translator>>,
|
||||
sink: &EventSink,
|
||||
queue: &Arc<Mutex<Queue>>,
|
||||
to_child: &mpsc::UnboundedSender<String>,
|
||||
) -> bool {
|
||||
let Ok(message) = serde_json::from_str::<Value>(line) else {
|
||||
tracing::warn!(
|
||||
@@ -686,31 +697,35 @@ fn translate_line(
|
||||
write_resume_token(session_dir, &session_id);
|
||||
}
|
||||
for event in events {
|
||||
// A turn ending is when a held message is taken up, and the
|
||||
// session is then not idle at all -- it is about to start the
|
||||
// turn that message asked for. Reporting the idle would show a
|
||||
// phone a finished session for as long as it took the next
|
||||
// turn to produce anything, with the message it is holding
|
||||
// still drawn as waiting.
|
||||
// Anything the CLI says after a steer was written is proof it has
|
||||
// been round the model again, and the steer went with it -- so
|
||||
// that is the moment it is announced, and the moment a phone can
|
||||
// stop drawing it as still waiting. The announcement goes out
|
||||
// *before* the event that proves it, so the message is above the
|
||||
// output it produced rather than below it.
|
||||
//
|
||||
// A turn ending counts too, and is the case that must not be
|
||||
// missed: a message written after the last model call of a turn
|
||||
// has no later output to prove anything, and without this it would
|
||||
// never be announced at all.
|
||||
if announces_a_steer(&event) {
|
||||
let taken: Vec<String> = {
|
||||
let mut queue = queue.lock().unwrap();
|
||||
queue.awaiting.drain(..).collect()
|
||||
};
|
||||
for text in taken {
|
||||
if sink.send(Event::MessageTaken { text }).is_err() {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
if matches!(
|
||||
event,
|
||||
Event::Status {
|
||||
state: SessionStatus::Idle
|
||||
}
|
||||
) {
|
||||
let next = {
|
||||
let mut queue = queue.lock().unwrap();
|
||||
let next = queue.waiting.pop_front();
|
||||
queue.running = next.is_some();
|
||||
next
|
||||
};
|
||||
if let Some((text, line)) = next {
|
||||
if sink.send(Event::MessageTaken { text }).is_err() || to_child.send(line).is_err()
|
||||
{
|
||||
return false;
|
||||
}
|
||||
continue;
|
||||
}
|
||||
queue.lock().unwrap().running = false;
|
||||
}
|
||||
if sink.send(event).is_err() {
|
||||
return false; // session torn down
|
||||
@@ -719,6 +734,24 @@ fn translate_line(
|
||||
true
|
||||
}
|
||||
|
||||
/// Whether this event proves the CLI has consumed anything written to it
|
||||
/// since the last one did.
|
||||
///
|
||||
/// Assistant output and a tool call both mean another model call happened;
|
||||
/// an idle means the turn is over and nothing further is coming. Status
|
||||
/// changes that are not idle prove nothing -- a turn can go `running`
|
||||
/// without having read a line written a moment ago.
|
||||
fn announces_a_steer(event: &Event) -> bool {
|
||||
matches!(
|
||||
event,
|
||||
Event::AssistantText { .. }
|
||||
| Event::ToolStart { .. }
|
||||
| Event::Status {
|
||||
state: SessionStatus::Idle
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
/// The end of the stderr log, for an exit report a person reads.
|
||||
///
|
||||
/// Bounded because this is held in a message; trimmed of blank lines at
|
||||
@@ -917,8 +950,8 @@ mod tests {
|
||||
running: true,
|
||||
..Queue::default()
|
||||
};
|
||||
queue.waiting.push_back(("first".into(), "{}".into()));
|
||||
queue.waiting.push_back(("second".into(), "{}".into()));
|
||||
queue.awaiting.push_back("first".into());
|
||||
queue.awaiting.push_back("second".into());
|
||||
queue.close(&sink, "the session ended");
|
||||
|
||||
// Named rather than counted, because these never reached the
|
||||
|
||||
@@ -306,6 +306,10 @@ impl Translator {
|
||||
continue;
|
||||
}
|
||||
let mut texts = Vec::new();
|
||||
// Held until the call's id is in hand a few lines below: an
|
||||
// image is drawn under the call that produced it, so it has to
|
||||
// carry that id rather than merely arrive next to it.
|
||||
let mut images = Vec::new();
|
||||
match block.get("content") {
|
||||
Some(Value::String(text)) => texts.push(text.clone()),
|
||||
Some(Value::Array(parts)) => {
|
||||
@@ -318,7 +322,7 @@ impl Translator {
|
||||
}
|
||||
Some("image") => {
|
||||
if let Some(name) = save_image(&self.session_dir, part) {
|
||||
events.push(Event::Image { image: name });
|
||||
images.push(name);
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
@@ -327,12 +331,19 @@ impl Translator {
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
let about = block
|
||||
.get("tool_use_id")
|
||||
.and_then(Value::as_str)
|
||||
.unwrap_or_default()
|
||||
.to_string();
|
||||
for image in images {
|
||||
events.push(Event::Image {
|
||||
image,
|
||||
about: Some(about.clone()),
|
||||
});
|
||||
}
|
||||
events.push(Event::ToolEnd {
|
||||
id: block
|
||||
.get("tool_use_id")
|
||||
.and_then(Value::as_str)
|
||||
.unwrap_or_default()
|
||||
.to_string(),
|
||||
id: about.clone(),
|
||||
output: texts.join("\n"),
|
||||
});
|
||||
}
|
||||
@@ -578,10 +589,13 @@ mod tests {
|
||||
);
|
||||
let events = translator.translate(&serde_json::from_str(&line).expect("json"));
|
||||
|
||||
let Event::Image { image } = &events[0] else {
|
||||
let Event::Image { image, about } = &events[0] else {
|
||||
panic!("expected an image event, got {events:?}");
|
||||
};
|
||||
assert!(image.ends_with(".png"));
|
||||
// Named as belonging to the call that produced it, so a phone draws
|
||||
// it under that row rather than beside it.
|
||||
assert_eq!(about.as_deref(), Some("toolu_05"));
|
||||
let saved = dir.path().join("files").join(image);
|
||||
assert!(saved.is_file(), "image not saved at {}", saved.display());
|
||||
assert_eq!(
|
||||
|
||||
@@ -66,6 +66,15 @@ pub enum Event {
|
||||
Image {
|
||||
#[serde(rename = "ref")]
|
||||
image: ImageRef,
|
||||
/// The tool call whose result carried it, when one did.
|
||||
///
|
||||
/// A screenshot belongs under the call that took it, not floating
|
||||
/// beside it -- the reader has to pair them by position otherwise,
|
||||
/// and position is exactly what a page boundary breaks. `None` for
|
||||
/// an image a person attached to their own message, which belongs
|
||||
/// to no call.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
about: Option<String>,
|
||||
},
|
||||
/// Anything the session needs a human for: AskUserQuestion, and
|
||||
/// permission requests, are the same shape with different options.
|
||||
|
||||
@@ -441,7 +441,7 @@ fn push_user(events: &mut Vec<Event>, content: &Value, session_dir: &std::path::
|
||||
for block in blocks {
|
||||
// A picture the person attached to their own message, rather
|
||||
// than one a tool produced. Same block shape, one level up.
|
||||
push_images(events, std::slice::from_ref(block), session_dir);
|
||||
push_images(events, std::slice::from_ref(block), session_dir, None);
|
||||
if block.get("type").and_then(Value::as_str) == Some("tool_result")
|
||||
&& let Some(id) = block.get("tool_use_id").and_then(Value::as_str)
|
||||
{
|
||||
@@ -449,7 +449,7 @@ fn push_user(events: &mut Vec<Event>, content: &Value, session_dir: &std::path::
|
||||
// a screenshot belongs to the call that took it, and after
|
||||
// the result it reads as belonging to whatever came next.
|
||||
if let Some(Value::Array(parts)) = block.get("content") {
|
||||
push_images(events, parts, session_dir);
|
||||
push_images(events, parts, session_dir, Some(id));
|
||||
}
|
||||
events.push(Event::ToolEnd {
|
||||
id: id.to_string(),
|
||||
@@ -465,12 +465,24 @@ fn push_user(events: &mut Vec<Event>, content: &Value, session_dir: &std::path::
|
||||
}
|
||||
|
||||
/// Saves every image block in `parts` and references each one.
|
||||
fn push_images(events: &mut Vec<Event>, parts: &[Value], session_dir: &std::path::Path) {
|
||||
///
|
||||
/// `about` is the call the images came out of, or `None` for one a person attached
|
||||
/// to their own message -- the same distinction the live translator makes, so replayed
|
||||
/// history draws a screenshot under the call that took it exactly as a live one does.
|
||||
fn push_images(
|
||||
events: &mut Vec<Event>,
|
||||
parts: &[Value],
|
||||
session_dir: &std::path::Path,
|
||||
about: Option<&str>,
|
||||
) {
|
||||
for part in parts {
|
||||
if part.get("type").and_then(Value::as_str) == Some("image")
|
||||
&& let Some(name) = super::claude::translate::save_image(session_dir, part)
|
||||
{
|
||||
events.push(Event::Image { image: name });
|
||||
events.push(Event::Image {
|
||||
image: name,
|
||||
about: about.map(String::from),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -626,7 +638,7 @@ mod tests {
|
||||
);
|
||||
|
||||
let events = events_from(&line, dir.path());
|
||||
let Some(Event::Image { image }) = events.first() else {
|
||||
let Some(Event::Image { image, .. }) = events.first() else {
|
||||
panic!("a replayed screenshot must become an image event: {events:?}");
|
||||
};
|
||||
assert!(image.ends_with(".png"));
|
||||
|
||||
@@ -151,6 +151,8 @@ impl LiveSession {
|
||||
for image in &images {
|
||||
let _ = self.sink.send(Event::Image {
|
||||
image: image.clone(),
|
||||
// A person's own attachment belongs to no tool call.
|
||||
about: None,
|
||||
});
|
||||
}
|
||||
self.driver.send_user_message(text, images);
|
||||
|
||||
@@ -332,6 +332,7 @@ mod tests {
|
||||
},
|
||||
Event::Image {
|
||||
image: "img1".into(),
|
||||
about: None,
|
||||
},
|
||||
Event::Question {
|
||||
id: "q1".into(),
|
||||
|
||||
Reference in new issue
Block a user