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 29a6d91..f1e6608 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Events.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Events.kt @@ -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"), 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 a426be2..0575940 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt @@ -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 = emptyList(), ) : TranscriptItem() data class QuestionCard( @@ -161,7 +168,19 @@ fun foldEvent(items: List, event: SessionEvent): List 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 -> 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 cdccdf6..b8bdb51 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/ToolRows.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/ToolRows.kt @@ -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) } } } diff --git a/server/src/session/claude.rs b/server/src/session/claude.rs index 53b8199..6a18b4a 100644 --- a/server/src/session/claude.rs +++ b/server/src/session/claude.rs @@ -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, /// 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 = self.waiting.drain(..).map(|(text, _)| text).collect(); + let lost: Vec = 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>, sink: EventSink, queue: Arc>, - to_child: mpsc::UnboundedSender, reading: Arc, 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>, sink: &EventSink, queue: &Arc>, - to_child: &mpsc::UnboundedSender, ) -> bool { let Ok(message) = serde_json::from_str::(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 = { + 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 diff --git a/server/src/session/claude/translate.rs b/server/src/session/claude/translate.rs index 843430a..ddb2667 100644 --- a/server/src/session/claude/translate.rs +++ b/server/src/session/claude/translate.rs @@ -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!( diff --git a/server/src/session/driver.rs b/server/src/session/driver.rs index e594c93..a35c8fa 100644 --- a/server/src/session/driver.rs +++ b/server/src/session/driver.rs @@ -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, }, /// Anything the session needs a human for: AskUserQuestion, and /// permission requests, are the same shape with different options. diff --git a/server/src/session/import.rs b/server/src/session/import.rs index b22c13f..149db1e 100644 --- a/server/src/session/import.rs +++ b/server/src/session/import.rs @@ -441,7 +441,7 @@ fn push_user(events: &mut Vec, 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, 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, content: &Value, session_dir: &std::path:: } /// Saves every image block in `parts` and references each one. -fn push_images(events: &mut Vec, 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, + 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")); diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index 24af992..c9e1023 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -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); diff --git a/server/src/session/transcript.rs b/server/src/session/transcript.rs index 8b0c886..722873e 100644 --- a/server/src/session/transcript.rs +++ b/server/src/session/transcript.rs @@ -332,6 +332,7 @@ mod tests { }, Event::Image { image: "img1".into(), + about: None, }, Event::Question { id: "q1".into(),