From 030cc8f539d5449088e3249409bfaac97c035739 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Mon, 17 Aug 2026 09:10:44 +0530 Subject: [PATCH 1/8] feat: render an edit as the harness's own stored hunk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit io-harness renders a unified diff for every edit its tools make and keeps it in the run's durable trace, so io-cli reads one rather than computing one. `Edit.hunk` carries the file's own `@@` line numbers, which is the fact a diff computed here from two strings could not produce — io-cli never sees the file. The read is anchored on `Step` and never on `ToolCall`: the harness documents `ToolCall` as emitted before the tool's result is known, while `Step` lands after the step is committed to the store. The two events are one line apart in a transcript, which is what would make a read of a row that does not exist yet invisible until it was a bug. `Store::edits` answers for the whole run, so `diff::for_step` filters; drawing what it returns would re-render every earlier edit at every later step. An absent hunk keeps its counts and says `no diff stored` — `None` means the row predates hunks, the previous contents were not kept, or the diff exceeded the snapshot cap, and not one of those is "nothing changed". Tone::Added and Tone::Removed fill the diff_add and diff_delete tokens the theme has carried since 0.1.0. Neither takes a word: the `+` and the `-` the harness already put on the line are what survives NO_COLOR. --- src/app.rs | 12 +++ src/diff.rs | 114 +++++++++++++++++++++++++ src/lib.rs | 1 + src/main.rs | 32 +++++++ src/theme.rs | 12 ++- tests/diff.rs | 230 ++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 400 insertions(+), 1 deletion(-) create mode 100644 src/diff.rs create mode 100644 tests/diff.rs diff --git a/src/app.rs b/src/app.rs index 5b1de26..8b06bb2 100644 --- a/src/app.rs +++ b/src/app.rs @@ -233,6 +233,18 @@ impl App { self.pending.extend(lines); } + /// What a step changed, as diffs. + /// + /// Handed in by the driver rather than read here. The store belongs to the + /// driver — it is the only thing that holds one — and keeping this a function + /// of values is what lets a test state a hunk by hand instead of standing up + /// a database to hold one. + pub fn edits(&mut self, edits: &[io_harness::Edit]) { + for edit in edits { + self.pending.extend(crate::diff::cell(edit, &self.theme)); + } + } + /// The status line's share of an event. /// /// Only the events that carry a fact set a field, and nothing sets one to a diff --git a/src/diff.rs b/src/diff.rs new file mode 100644 index 0000000..2a3a1a8 --- /dev/null +++ b/src/diff.rs @@ -0,0 +1,114 @@ +//! Rendering an edit the agent made. +//! +//! **io-cli computes no diff.** io-harness renders a unified diff for every edit +//! its tools make and keeps it in the run's durable trace, so what this module +//! draws is text that already exists — `io_harness::Edit::hunk`, a hunk body +//! whose `@@` line numbers are the *file's* rather than the fragment's. That is +//! the difference between a diff this product can show and one it could only +//! approximate: io-cli never sees the file, so it could not number a hunk +//! correctly even if it wanted to compute one. +//! +//! Two consequences shape everything here. +//! +//! **The hunk is passed through, never reconstructed.** Its markers, its +//! spacing and its header are the harness's; this module decides colour and +//! nothing else. A reader who copies a rendered line into `patch` gets what +//! `patch` expects. +//! +//! **An absent hunk is a fact, not an empty one.** `Edit::hunk` is `None` when +//! the row predates the harness release that added hunks, when the file's +//! previous contents were not kept (over the store's snapshot cap, or not +//! UTF-8), or when the rendered diff would itself have exceeded that cap. Not +//! one of those is "nothing changed" — and `lines_added` and `lines_removed` +//! are still there to prove it — so the cell says the counts and says the diff +//! is missing, rather than drawing an empty body that reads as an untouched +//! file. + +use io_harness::Edit; +use ratatui::text::{Line, Span}; + +use crate::status::SEPARATOR; +use crate::theme::{Theme, Tone}; + +/// What every line of the cell is indented by, so a hunk sits under its header +/// the way a tool call sits under a step. +const INDENT: &str = " "; + +/// The edits a given step made, out of everything the run recorded. +/// +/// `Store::edits` answers for the whole run, so a caller that draws what it +/// returns re-renders every earlier edit at every later step — the transcript +/// grows quadratically and the same diff appears four times. Kept here rather +/// than in the driver so that it is reachable from a test; the driver is a +/// binary and an integration test cannot link one. +pub fn for_step(edits: Vec, step: u32) -> Vec { + edits.into_iter().filter(|edit| edit.step == step).collect() +} + +/// One edit, as lines for the terminal's scrollback. +/// +/// The header first — the path, then the counts, then the tool — because the +/// path is the content and the rest is metadata, which is the rule the whole +/// interface follows. +pub fn cell(edit: &Edit, theme: &Theme) -> Vec> { + let mut lines = vec![header(edit, theme)]; + + let Some(hunk) = edit.hunk.as_deref() else { + // The header already carried the absence. Returning one line here is not + // a shortcut: a body of zero lines under a header is exactly the empty + // diff this must not draw. + return lines; + }; + + lines.extend(hunk.lines().map(|line| body(line, theme))); + lines.push(Line::from("")); + lines +} + +/// ` src/theme.rs · +1 -1 · edit_file`, and `· no diff stored` when there is +/// no hunk to draw under it. +fn header(edit: &Edit, theme: &Theme) -> Line<'static> { + let mut spans = vec![ + Span::styled(INDENT.to_string(), theme.style(Tone::Muted)), + Span::styled(edit.path.clone(), theme.style(Tone::Accent)), + Span::styled(SEPARATOR.to_string(), theme.style(Tone::Muted)), + Span::styled(format!("+{}", edit.lines_added), theme.style(Tone::Added)), + Span::styled(" ".to_string(), theme.style(Tone::Muted)), + Span::styled( + format!("-{}", edit.lines_removed), + theme.style(Tone::Removed), + ), + Span::styled( + format!("{SEPARATOR}{}", edit.tool), + theme.style(Tone::Muted), + ), + ]; + if edit.hunk.is_none() { + spans.push(Span::styled( + format!("{SEPARATOR}no diff stored"), + theme.style(Tone::Muted), + )); + } + Line::from(spans) +} + +/// One line of a hunk body, coloured by the marker the harness put on it. +/// +/// The marker stays on the line. Colour is never the sole carrier of meaning +/// here — under `NO_COLOR` every style collapses to nothing and the `+` and the +/// `-` are what is left saying which side a line is on. +fn body(line: &str, theme: &Theme) -> Line<'static> { + let tone = match line.as_bytes().first() { + Some(b'+') => Tone::Added, + Some(b'-') => Tone::Removed, + // `@@ … @@` — where in the file this is. The one part of a hunk a reader + // navigates by, so it gets the product's own colour rather than the + // muted one context lines take. + Some(b'@') => Tone::Accent, + // `\ No newline at end of file`. Real information about the bytes, and + // not a change, so it reads as neither. + Some(b'\\') => Tone::Muted, + _ => Tone::Muted, + }; + Line::from(Span::styled(format!("{INDENT}{line}"), theme.style(tone))) +} diff --git a/src/lib.rs b/src/lib.rs index 520f58d..ac132bf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,6 +16,7 @@ pub mod bridge; pub mod cli; pub mod commands; pub mod composer; +pub mod diff; pub mod events; pub mod picker; pub mod settings; diff --git a/src/main.rs b/src/main.rs index 08f38f1..73e4226 100644 --- a/src/main.rs +++ b/src/main.rs @@ -414,6 +414,7 @@ async fn turn( } Some(event) = events.recv() => { app.event(&event); + commit_edits(app, store, &event); app.status.elapsed = started.elapsed(); paint(screen, app)?; } @@ -463,6 +464,37 @@ async fn turn( paint(screen, app) } +/// Draw what a step changed, by asking the store what it recorded. +/// +/// **Anchored on `Step` and never on `ToolCall`.** io-harness documents +/// `ToolCall` as *a tool was invoked, before its result is known*, while `Step` +/// is emitted after the step has been committed to the store — so a read at the +/// tool call is a read of a row that may not be there yet, and the two events +/// are one line apart in a transcript, which is what would make that invisible +/// until it was a bug. +/// +/// This is the first time this product reads the durable trace while a run is +/// live. It is safe because it happens on the drain half of the select, where +/// the turn future is suspended, and because every `Store` call is synchronous — +/// nothing this function does is held across an await, so there is no shape here +/// that can deadlock against the turn holding the same `&Store`. +/// +/// A read that fails degrades to a line saying so. A run whose work succeeded is +/// not a run to panic over because the trace could not be re-read, and silence +/// would say the step changed nothing. +fn commit_edits(app: &mut App, store: &Store, event: &io_harness::RunEvent) { + let io_harness::EventKind::Step { changed: true, .. } = &event.kind else { + return; + }; + match store.edits(event.run_id) { + Ok(edits) => app.edits(&io_cli::diff::for_step(edits, event.step)), + Err(error) => app.say( + Tone::Muted, + format!("the diff for this step could not be read: {error}"), + ), + } +} + /// The first-run wizard. Returns the theme chosen, or `None` if it was abandoned. async fn wizard( screen: &mut Screen>, diff --git a/src/theme.rs b/src/theme.rs index 4c3f2f0..d0def3a 100644 --- a/src/theme.rs +++ b/src/theme.rs @@ -75,6 +75,11 @@ pub enum Tone { /// An act the permission boundary refused. Its own tone because it is not an /// error — the system worked. Refused, + /// A line a change added. The `diff_add` token, which has been in the theme + /// since 0.1.0 waiting for the release that draws a diff. + Added, + /// A line a change removed. + Removed, } impl Tone { @@ -82,7 +87,10 @@ impl Tone { /// than meaning. pub fn word(self) -> Option<&'static str> { match self { - Self::Normal | Self::Muted | Self::Accent => None, + // A diff line's carrier is the `+` or the `-` the harness already + // put on it, which is why these two need no word of their own: the + // meaning survives `NO_COLOR` without one. + Self::Normal | Self::Muted | Self::Accent | Self::Added | Self::Removed => None, Self::Success => Some("ok"), Self::Warning => Some("warning"), Self::Error => Some("error"), @@ -222,6 +230,8 @@ impl Theme { Tone::Refused => Style::default() .fg(self.warning) .add_modifier(Modifier::BOLD), + Tone::Added => Style::default().fg(self.diff_add), + Tone::Removed => Style::default().fg(self.diff_delete), } } diff --git a/tests/diff.rs b/tests/diff.rs new file mode 100644 index 0000000..21d97c7 --- /dev/null +++ b/tests/diff.rs @@ -0,0 +1,230 @@ +//! F1 — an edit renders as the harness's own stored hunk. +//! F2 — an absent hunk reads as absent, never as an empty diff. +//! +//! The subject of both is `io_cli::diff`, which takes an `io_harness::Edit` and +//! returns lines. It is deliberately a function of a value rather than of a +//! store: the store read belongs to the driver, which is the only thing that +//! holds a `Store`, and keeping it out of here is what lets these tests state a +//! hunk by hand instead of standing up a database to hold one. + +mod support; + +use io_cli::diff::cell; +use io_cli::theme::DARK; +use io_harness::Edit; + +/// The text of a rendered cell, one line per line, as a reader would see it. +fn rendered(edit: &Edit) -> String { + cell(edit, &DARK) + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>() + .join("\n") +} + +/// A hunk as io-harness renders one: a body, no `---`/`+++` headers, and `@@` +/// line numbers that are the *file's* rather than the hunk's own. +/// +/// The numbers here are the load-bearing part of F1. They say the change is at +/// line 12 of a file whose contents this test never supplies, which is precisely +/// the fact a diff computed inside io-cli from two strings could not produce. +const HUNK: &str = "\ +@@ -12,5 +12,5 @@ + fn one() {} + +- Tone::Muted => Style::default().fg(GREY), ++ Tone::Muted => Style::default().fg(self.muted), + + fn two() {} +"; + +fn edit_with_hunk() -> Edit { + Edit { + step: 3, + tool: "edit_file".to_string(), + path: "src/theme.rs".to_string(), + lines_added: 1, + lines_removed: 1, + hunk: Some(HUNK.to_string()), + } +} + +#[test] +fn f1_the_hunk_is_the_harness_s_own_text() { + let text = rendered(&edit_with_hunk()); + + // Every body line of the stored hunk survives, in order, with its marker. + for line in HUNK.lines() { + assert!( + text.contains(line), + "the rendered cell dropped {line:?}\n\n{text}", + ); + } +} + +#[test] +fn f1_the_line_numbers_are_the_file_s_and_are_not_recomputed() { + let text = rendered(&edit_with_hunk()); + + // The whole of F1. A diff io-cli computed from the two texts it was given + // would number this hunk from 1, because it has never seen the eleven lines + // above it. The header is passed through, character for character. + assert!( + text.contains("@@ -12,5 +12,5 @@"), + "the file's own line numbers are not in the cell:\n\n{text}", + ); + assert!( + !text.contains("@@ -1,5 +1,5 @@"), + "the hunk was renumbered, which means it was recomputed:\n\n{text}", + ); +} + +#[test] +fn f1_the_path_comes_before_the_counts() { + let text = rendered(&edit_with_hunk()); + let first = text.lines().next().expect("a header line"); + + let path = first + .find("src/theme.rs") + .expect("the path is on the header"); + let counts = first.find("+1").expect("the counts are on the header"); + // Content before metadata, asserted by position rather than by presence — + // 0.1.1 paid for the lesson that a `contains` assertion is just as green + // when the sentence is inside out. + assert!( + path < counts, + "the counts came before the path, which is metadata before content: {first:?}", + ); +} + +#[test] +fn f2_an_absent_hunk_says_so_and_keeps_its_counts() { + // `Edit.hunk` is `None` for three reasons and not one of them is "nothing + // changed": the row predates the harness release that added hunks, the + // file's previous contents were not kept, or the rendered diff would have + // exceeded the store's snapshot cap. The counts are still there in every one + // of those cases, and they are what says the file did change. + let edit = Edit { + step: 2, + tool: "write_file".to_string(), + path: "src/big.rs".to_string(), + lines_added: 812, + lines_removed: 3, + hunk: None, + }; + + let text = rendered(&edit); + + assert!(text.contains("src/big.rs"), "{text}"); + assert!(text.contains("+812"), "the additions are lost:\n\n{text}"); + assert!(text.contains("−3") || text.contains("-3"), "{text}"); + assert!( + text.contains("no diff stored"), + "an absent hunk has to say it is absent:\n\n{text}", + ); +} + +#[test] +fn f2_an_absent_hunk_never_renders_as_an_empty_diff() { + let edit = Edit { + step: 2, + tool: "write_file".to_string(), + path: "src/big.rs".to_string(), + lines_added: 812, + lines_removed: 3, + hunk: None, + }; + + let text = rendered(&edit); + + // The version that would ship by accident: `None` treated as an empty patch, + // which draws a cell that says the file was untouched while the counts + // beside it say it was not. + assert!( + !text.contains("@@"), + "an absent hunk drew a diff body:\n\n{text}", + ); + // Counted over the rendered lines and NOT over `str::lines()` of the joined + // text. That is not a stylistic preference: `"header\n".lines()` yields one + // element, so a text-level count cannot see the trailing blank line an + // `unwrap_or_default()` leaves behind — which is precisely the empty-patch + // shape this assertion exists to catch. The sabotage arm found it. + assert_eq!( + cell(&edit, &DARK).len(), + 1, + "an absent hunk is one line and nothing else:\n\n{text}", + ); +} + +#[test] +fn f1_only_this_step_s_edits_are_drawn_at_this_step() { + // `Store::edits` answers for the whole run. A caller that draws what it + // returns re-renders every earlier edit at every later step, so the same + // diff appears once, then twice, then three times — a transcript that grows + // quadratically in the length of the turn. + let edits = vec![ + Edit { + step: 2, + path: "first.rs".to_string(), + ..Default::default() + }, + Edit { + step: 3, + path: "second.rs".to_string(), + ..Default::default() + }, + Edit { + step: 3, + path: "third.rs".to_string(), + ..Default::default() + }, + ]; + + let mine = io_cli::diff::for_step(edits, 3); + + let paths: Vec<&str> = mine.iter().map(|edit| edit.path.as_str()).collect(); + assert_eq!(paths, ["second.rs", "third.rs"]); +} + +/// The read this release adds, against a real store rather than against values. +/// +/// It is here because this is the first time io-cli reads the durable trace at +/// all: everything before 0.3.0 rendered events as they streamed past. The +/// round trip is what proves the hunk survives being written and read back, +/// which is the whole premise of F1 — that the diff on screen is the harness's +/// text and not io-cli's. +#[test] +fn f1_a_hunk_survives_the_store_round_trip() { + let directory = tempfile::tempdir().expect("a temporary directory"); + let store = io_harness::Store::open(directory.path().join("io.db")).expect("a store"); + let run_id = 1; + + store + .record_edit(run_id, &edit_with_hunk()) + .expect("the edit is recorded"); + + let read = store.edits(run_id).expect("the edits are read back"); + let mine = io_cli::diff::for_step(read, 3); + assert_eq!(mine.len(), 1, "one edit was recorded at step 3"); + assert_eq!( + mine[0].hunk.as_deref(), + Some(HUNK), + "the hunk came back changed", + ); + + let text = rendered(&mine[0]); + assert!(text.contains("@@ -12,5 +12,5 @@"), "{text}"); +} + +#[test] +fn f2_a_hunk_that_is_present_is_not_confused_for_an_absent_one() { + // The other direction, so the F2 sabotage arm cannot pass by rendering the + // absence sentence on everything. + let text = rendered(&edit_with_hunk()); + assert!(!text.contains("no diff stored"), "{text}"); +} From 6e64b5f395c3fe33a5d21be9b8b1e5fc06eb58eb Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Mon, 17 Aug 2026 09:18:45 +0530 Subject: [PATCH 2/8] feat: emphasise the words that changed inside a diff line A run of removals pairs with the run of additions immediately after it, and only when the two runs are the same length. Anything else draws without emphasis. The timidity is the point. io-harness's own diff module records that an edit_file is one contiguous replacement by construction, but a write_file that rewrote two distant regions arrives as ONE hunk spanning both. A greedy rule would then emphasise the difference between lines that have nothing to do with each other, which is worse than no emphasis: it invents a relationship and paints it. Bold rather than a background colour. A background paints the full cell width of every character it covers, which survives being copied out of the terminal as trailing whitespace. Theme::emphasis returns Style::default() under NO_COLOR along with everything else, because a modifier is still a presentation-only channel and the marker is what carries the meaning there. Falls back to a plain line when the two sides share neither head nor tail: emphasising all of a line is the same as emphasising none of it. --- src/diff.rs | 126 ++++++++++++++++++++++++++++++++++++++++++++++++-- src/theme.rs | 16 +++++++ tests/diff.rs | 125 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 264 insertions(+), 3 deletions(-) diff --git a/src/diff.rs b/src/diff.rs index 2a3a1a8..4f0c557 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -60,11 +60,130 @@ pub fn cell(edit: &Edit, theme: &Theme) -> Vec> { return lines; }; - lines.extend(hunk.lines().map(|line| body(line, theme))); + lines.extend(body(hunk, theme)); lines.push(Line::from("")); lines } +/// A hunk body, with the words that changed emphasised where they can be known. +/// +/// **The pairing rule is deliberately timid.** A run of removals is paired with +/// the run of additions immediately after it, and only when the two runs are the +/// same length — then line *n* of one is compared with line *n* of the other. +/// Anything else is drawn without emphasis. +/// +/// The reason is in io-harness's own diff module: an `edit_file` is one +/// contiguous replacement by construction, but a `write_file` that rewrote two +/// distant regions of a file arrives as **one hunk spanning both**. A rule that +/// paired greedily would then emphasise the difference between lines that have +/// nothing to do with each other, which is worse than not emphasising at all — +/// it invents a relationship and paints it in colour. +fn body(hunk: &str, theme: &Theme) -> Vec> { + let raw: Vec<&str> = hunk.lines().collect(); + let mut out = Vec::new(); + let mut at = 0; + + while at < raw.len() { + let removals_from = at; + while at < raw.len() && raw[at].starts_with('-') { + at += 1; + } + let removals = &raw[removals_from..at]; + + let additions_from = at; + while at < raw.len() && raw[at].starts_with('+') { + at += 1; + } + let additions = &raw[additions_from..at]; + + if removals.is_empty() && additions.is_empty() { + out.push(plain(raw[at], theme)); + at += 1; + continue; + } + + let paired = !removals.is_empty() && removals.len() == additions.len(); + for (n, line) in removals.iter().enumerate() { + out.push(match paired { + true => against(line, additions[n], Tone::Removed, theme), + false => plain(line, theme), + }); + } + for (n, line) in additions.iter().enumerate() { + out.push(match paired { + true => against(line, removals[n], Tone::Added, theme), + false => plain(line, theme), + }); + } + } + + out +} + +/// One changed line, split into what both sides share and what only this one +/// has. +/// +/// Falls back to a plain line when the two share neither a head nor a tail: +/// emphasising the whole line is the same as emphasising none of it, and it is +/// exactly what a reader would have to un-learn on the next hunk. +fn against(mine: &str, other: &str, tone: Tone, theme: &Theme) -> Line<'static> { + // The marker is the first byte and is ASCII, so this never splits a + // character. It stays on the line — it is what carries the meaning when + // colour does not. + let (marker, text) = mine.split_at(1); + let other = &other[1..]; + + let head = common_head(text, other); + // Measured on what is left after the head, so a line like `a` against `aa` + // cannot count the same byte twice and produce a middle of negative length. + let tail = common_tail(&text[head..], &other[head..]); + + if head == 0 && tail == 0 { + return plain(mine, theme); + } + + let mut spans = vec![Span::styled( + format!("{INDENT}{marker}{}", &text[..head]), + theme.style(tone), + )]; + let middle = &text[head..text.len() - tail]; + if !middle.is_empty() { + spans.push(Span::styled(middle.to_string(), theme.emphasis(tone))); + } + if tail > 0 { + spans.push(Span::styled( + text[text.len() - tail..].to_string(), + theme.style(tone), + )); + } + Line::from(spans) +} + +/// How many bytes `a` and `b` agree on from the front, always on a character +/// boundary. +fn common_head(a: &str, b: &str) -> usize { + let mut bytes = 0; + for (one, two) in a.chars().zip(b.chars()) { + if one != two { + break; + } + bytes += one.len_utf8(); + } + bytes +} + +/// The same from the back. +fn common_tail(a: &str, b: &str) -> usize { + let mut bytes = 0; + for (one, two) in a.chars().rev().zip(b.chars().rev()) { + if one != two { + break; + } + bytes += one.len_utf8(); + } + bytes +} + /// ` src/theme.rs · +1 -1 · edit_file`, and `· no diff stored` when there is /// no hunk to draw under it. fn header(edit: &Edit, theme: &Theme) -> Line<'static> { @@ -92,12 +211,13 @@ fn header(edit: &Edit, theme: &Theme) -> Line<'static> { Line::from(spans) } -/// One line of a hunk body, coloured by the marker the harness put on it. +/// One line of a hunk body, coloured by the marker the harness put on it and +/// emphasised nowhere. /// /// The marker stays on the line. Colour is never the sole carrier of meaning /// here — under `NO_COLOR` every style collapses to nothing and the `+` and the /// `-` are what is left saying which side a line is on. -fn body(line: &str, theme: &Theme) -> Line<'static> { +fn plain(line: &str, theme: &Theme) -> Line<'static> { let tone = match line.as_bytes().first() { Some(b'+') => Tone::Added, Some(b'-') => Tone::Removed, diff --git a/src/theme.rs b/src/theme.rs index d0def3a..35b24d8 100644 --- a/src/theme.rs +++ b/src/theme.rs @@ -235,6 +235,22 @@ impl Theme { } } + /// A tone, emphasised — for the words inside a diff line that actually + /// changed. + /// + /// Bold rather than a background colour: a background paints the full cell + /// width of every character it covers, which on a diff line means a block + /// that survives being copied out of the terminal as trailing whitespace. + /// Under `NO_COLOR` this collapses to nothing along with everything else, + /// which is correct — the `+` and the `-` are the carriers there, and a + /// modifier is still a presentation-only channel. + pub fn emphasis(&self, tone: Tone) -> Style { + if !self.coloured { + return Style::default(); + } + self.style(tone).add_modifier(Modifier::BOLD) + } + /// One line carrying a state, with the tone's word in front of it. /// /// This is the only way a toned line is built, so "colour is never the sole diff --git a/tests/diff.rs b/tests/diff.rs index 21d97c7..22cf495 100644 --- a/tests/diff.rs +++ b/tests/diff.rs @@ -228,3 +228,128 @@ fn f2_a_hunk_that_is_present_is_not_confused_for_an_absent_one() { let text = rendered(&edit_with_hunk()); assert!(!text.contains("no diff stored"), "{text}"); } + +// --------------------------------------------------------------------------- +// F3 — word-level emphasis inside a changed line. +// --------------------------------------------------------------------------- + +/// The spans of the one line in a rendered cell that starts with `marker`, +/// as (text, emphasised) pairs. +/// +/// Emphasis is read off the style rather than off the text, because the whole +/// point of F3 is *which part of the line* is marked — a test that looked at the +/// text alone could not tell a line emphasised in one piece from a line +/// emphasised in three. +fn spans_of(edit: &Edit, marker: char) -> Vec<(String, bool)> { + let lines = cell(edit, &DARK); + let line = lines + .iter() + .find(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + .trim_start() + .starts_with(marker) + }) + .unwrap_or_else(|| panic!("no line starting with {marker:?}")); + line.spans + .iter() + .map(|span| { + ( + span.content.to_string(), + span.style + .add_modifier + .contains(ratatui::style::Modifier::BOLD), + ) + }) + .collect() +} + +#[test] +fn f3_only_the_changed_words_are_emphasised() { + let removed = spans_of(&edit_with_hunk(), '-'); + let added = spans_of(&edit_with_hunk(), '+'); + + let emphasised: Vec<&str> = removed + .iter() + .filter(|(_, on)| *on) + .map(|(text, _)| text.as_str()) + .collect(); + assert_eq!( + emphasised, + ["GREY"], + "the removed side should emphasise exactly what went: {removed:?}", + ); + + let emphasised: Vec<&str> = added + .iter() + .filter(|(_, on)| *on) + .map(|(text, _)| text.as_str()) + .collect(); + assert_eq!( + emphasised, + ["self.muted"], + "the added side should emphasise exactly what arrived: {added:?}", + ); +} + +#[test] +fn f3_the_emphasis_sits_between_the_common_head_and_the_common_tail() { + let removed = spans_of(&edit_with_hunk(), '-'); + + // Asserted by POSITION: head, then the change, then tail. A membership + // assertion is just as green when the line is inside out, which 0.1.1 paid + // for and 0.2.0 paid for again. + let at = removed + .iter() + .position(|(_, on)| *on) + .expect("something is emphasised"); + assert!( + at > 0, + "there is a common head before the change: {removed:?}" + ); + assert!( + at + 1 < removed.len(), + "there is a common tail after the change: {removed:?}", + ); + assert!( + removed[at - 1].0.ends_with(".fg("), + "the head stops where the lines stop agreeing: {removed:?}", + ); + assert_eq!( + removed[at + 1].0, + "),", + "the tail starts where they agree again: {removed:?}", + ); +} + +#[test] +fn f3_an_unpaired_line_is_emphasised_at_the_line_and_not_within_it() { + // Two lines out and one in. There is no honest pairing here — the harness + // renders a `write_file` that rewrote two distant regions as ONE hunk + // spanning both, so a rule that paired by position would emphasise the + // difference between lines that have nothing to do with each other. + let edit = Edit { + step: 1, + tool: "write_file".to_string(), + path: "a.rs".to_string(), + lines_added: 1, + lines_removed: 2, + hunk: Some( + "@@ -1,3 +1,2 @@\n-let a = 1;\n-let b = 2;\n+let everything = 0;\n ok\n".to_string(), + ), + }; + + for (text, emphasised) in spans_of(&edit, '-') { + assert!( + !emphasised, + "an unpaired removal was emphasised within the line: {text:?}", + ); + } + assert_eq!( + spans_of(&edit, '-').len(), + 1, + "an unpaired line is one span", + ); +} From ee10488e09098c2f1b1adcb6a280936d6025517d Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Mon, 17 Aug 2026 09:30:54 +0530 Subject: [PATCH 3/8] feat: syntax highlighting inside a diff, in io-cli's own tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ninth direct dependency, and the first added since 0.1.0. The feature set is the load-bearing part rather than the version: default-features = false drops regex-onig, which is oniguruma — a C library that would put a native build step in the path of the musl and MSVC cross-compiles that produce two of the four release artifacts, and would fail inside the release workflow rather than in a test. default-themes is off because the colours are io-cli's own, which is what keeps a highlighted diff and the rest of the interface one aesthetic and NO_COLOR deciding from one place. tests/dependencies.rs asserts the feature list, not just the name. Green still means added. The obvious combinations both lose something: washing the changed line leaves no colour for the code, syntax colouring it leaves none for the change. Instead the marker keeps the diff colour, the parts both sides share are syntax coloured, and the words that actually changed are drawn in the diff colour and emphasised — so the add/remove colour now points at the exact words instead of washing the line. A line with no partner takes the plain wash and no emphasis: bolding every word says "look here" about all of them. The scope table needs `storage` as well as `keyword`. Sublime's vocabulary files let, const, fn, struct and pub under storage.type and storage.modifier, leaving keyword for control flow and operators, so a table without it highlights `=` and misses every declaration on the line. Found by a test rather than by reading the vocabulary. Three theme tokens, nine to twelve. A comment did not get a fourth: a comment is muted and muted already exists, and a token whose value would always equal another's can drift out of agreement with itself. The grammar table loads behind a OnceLock reached only from the diff renderer, so a session that never edits a file never pays for it. N4 asserts that structurally rather than by timing anything. --- Cargo.lock | 101 +++++++++++ Cargo.toml | 21 +++ src/diff.rs | 378 +++++++++++++++++++++++++++++++----------- src/theme.rs | 53 +++++- tests/dependencies.rs | 95 +++++++++++ tests/diff.rs | 154 +++++++++++++++++ 6 files changed, 706 insertions(+), 96 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b18ea17..7f6f777 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,6 +2,12 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "adler2" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa" + [[package]] name = "aho-corasick" version = "1.1.5" @@ -123,6 +129,30 @@ version = "0.23.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ac07cdecf99051d9a5238b80f35af32cdeba5b336e55d957b318b50137e18da5" +[[package]] +name = "bincode" +version = "1.3.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b1f45e9417d87227c7a56d22e471c6206462cba514c7590c09aff4cf6d1ddcad" +dependencies = [ + "serde", +] + +[[package]] +name = "bit-set" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08807e080ed7f9d5433fa9b275196cfc35414f66a0c79d864dc51a0d825231a3" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7" + [[package]] name = "bitflags" version = "2.13.1" @@ -307,6 +337,15 @@ dependencies = [ "libc", ] +[[package]] +name = "crc32fast" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9481c1c90cbf2ac953f07c8d4a58aa3945c425b7185c9154d67a65e4230da511" +dependencies = [ + "cfg-if", +] + [[package]] name = "crossterm" version = "0.28.1" @@ -417,6 +456,17 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7360491ce676a36bf9bb3c56c1aa791658183a54d2744120f27285738d90465a" +[[package]] +name = "fancy-regex" +version = "0.16.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "998b056554fbe42e03ae0e152895cd1a7e1002aec800fdc6635d20270260c46f" +dependencies = [ + "bit-set", + "regex-automata", + "regex-syntax", +] + [[package]] name = "fastrand" version = "2.5.0" @@ -429,6 +479,22 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d45db016d36b838f563236e9193d0ee6ce38f3f68b6c94e914b4929c96bbb890" +[[package]] +name = "flate2" +version = "1.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "843fba2746e448b37e26a819579957415c8cef339bf08564fe8b7ddbd959573c" +dependencies = [ + "crc32fast", + "miniz_oxide", +] + +[[package]] +name = "fnv" +version = "1.0.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" + [[package]] name = "foldhash" version = "0.1.5" @@ -887,6 +953,7 @@ dependencies = [ "io-harness", "ratatui", "serde", + "syntect", "tempfile", "tokio", "toml", @@ -1083,6 +1150,16 @@ version = "2.8.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf8baf1c55e62ffcace7a9f06f4bd9cd3f0c4beb022d3b367256b91b87513d98" +[[package]] +name = "miniz_oxide" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fa76a2c86f704bdb222d66965fb3d63269ce38518b83cb0575fca855ebb6316" +dependencies = [ + "adler2", + "simd-adler32", +] + [[package]] name = "mio" version = "1.2.2" @@ -1743,6 +1820,12 @@ dependencies = [ "libc", ] +[[package]] +name = "simd-adler32" +version = "0.3.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3a219298ac11a56ea9a6d2120044824d6f01aeb034955e7af7bc16858527deea" + [[package]] name = "simd_cesu8" version = "1.2.0" @@ -1894,6 +1977,24 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "syntect" +version = "5.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "656b45c05d95a5704399aeef6bd0ddec7b2b3531b7c9e900abbf7c4d2190c925" +dependencies = [ + "bincode", + "fancy-regex", + "flate2", + "fnv", + "once_cell", + "regex-syntax", + "serde", + "serde_derive", + "thiserror", + "walkdir", +] + [[package]] name = "tempfile" version = "3.27.0" diff --git a/Cargo.toml b/Cargo.toml index 27fb403..1f67205 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,6 +50,27 @@ clap = { version = "4", features = ["derive"] } tokio = { version = "1", features = ["rt-multi-thread", "macros", "sync", "time"] } serde = { version = "1", features = ["derive"] } toml = "1" +# Syntax highlighting inside a diff, and the feature set is the load-bearing part +# rather than the version. +# +# `default-features = false` excludes `regex-onig`, which is oniguruma — a C +# library. Two of this product's four artifacts are cross-compiled, to +# `x86_64-unknown-linux-musl` and `x86_64-pc-windows-msvc`, and a native build +# step in that path fails inside the release workflow rather than in a test. +# `regex-fancy` is the pure-Rust engine that replaces it. +# +# `default-themes` is off on purpose: the colours come from io-cli's own token +# set, so a highlighted diff and the rest of the interface are one aesthetic +# instead of two, and `NO_COLOR` keeps working because there is only one place +# that decides colour. `html`, `plist-load` and `yaml-load` are for loading +# grammars and emitting markup at runtime, which this product never does. +# +# `tests/dependencies.rs` asserts this exact feature list. A later +# `default-features = true` would pull the C engine back in silently. +syntect = { version = "5", default-features = false, features = [ + "default-syntaxes", + "regex-fancy", +] } [dev-dependencies] tempfile = "3" diff --git a/src/diff.rs b/src/diff.rs index 4f0c557..8ae4307 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -8,7 +8,7 @@ //! approximate: io-cli never sees the file, so it could not number a hunk //! correctly even if it wanted to compute one. //! -//! Two consequences shape everything here. +//! Three decisions shape everything here. //! //! **The hunk is passed through, never reconstructed.** Its markers, its //! spacing and its header are the harness's; this module decides colour and @@ -23,9 +23,25 @@ //! are still there to prove it — so the cell says the counts and says the diff //! is missing, rather than drawing an empty body that reads as an untouched //! file. +//! +//! **Green means added, and it never also means "this is a string".** The +//! obvious way to combine syntax highlighting with a diff is to colour the whole +//! changed line green or red, which leaves no colour for the code, or to syntax +//! colour the whole line, which leaves no colour for the change. Neither is +//! taken. The marker keeps the diff's colour, the parts of a line that both +//! sides share are syntax coloured, and **the words that actually changed are +//! drawn in the diff's colour and emphasised** — so the add/remove colour is +//! still on screen and is now pointing at the exact words rather than washing +//! the line. A line with no partner to compare against gets the whole wash, +//! because on that line everything did change. + +use std::ops::Range; +use std::sync::OnceLock; use io_harness::Edit; use ratatui::text::{Line, Span}; +use syntect::easy::ScopeRangeIterator; +use syntect::parsing::{ParseState, Scope, ScopeStack, SyntaxReference, SyntaxSet}; use crate::status::SEPARATOR; use crate::theme::{Theme, Tone}; @@ -34,6 +50,134 @@ use crate::theme::{Theme, Tone}; /// the way a tool call sits under a step. const INDENT: &str = " "; +/// The syntax highlighter, built once and only if a diff is ever drawn. +/// +/// **Never on the startup path.** `SyntaxSet::load_defaults_newlines` +/// decompresses a dump of every grammar syntect ships, which is real work and +/// real memory, and this product's own bar is a first paint inside a hundred +/// milliseconds. A session that never edits a file never pays for it — which is +/// most sessions, since a question does not touch the workspace. +/// +/// A `OnceLock` rather than a field on anything: the set is immutable once +/// built, every diff in the session shares it, and threading it through [`cell`] +/// would put a parameter on a function whose callers have no business knowing a +/// highlighter exists. +static HIGHLIGHTER: OnceLock = OnceLock::new(); + +/// The grammar set, plus the four scopes worth colouring. +struct Highlighter { + syntaxes: SyntaxSet, + /// Matched by prefix, so `keyword` catches `keyword.control.rust` and every + /// other language's spelling of the same idea without a table per language. + keyword: Scope, + /// `storage`, and it is not redundant with `keyword`. Sublime's scope + /// vocabulary — which syntect's grammars are written in — files `let`, + /// `const`, `fn`, `struct` and `pub` under `storage.type` and + /// `storage.modifier`, leaving `keyword` for control flow and operators. A + /// table without this entry highlights `=` and misses every declaration on + /// the line, which is the opposite of what a reader wants. Found by a test, + /// not by reading the vocabulary. + storage: Scope, + string: Scope, + comment: Scope, + constant: Scope, +} + +impl Highlighter { + fn get() -> &'static Self { + HIGHLIGHTER.get_or_init(|| Self { + syntaxes: SyntaxSet::load_defaults_newlines(), + // `Scope::new` fails only on a malformed selector and these four are + // literals in this file. The fallback is the empty scope, which is a + // prefix of everything — so the impossible case colours a line + // wrongly rather than panicking inside a renderer. + keyword: Scope::new("keyword").unwrap_or_default(), + storage: Scope::new("storage").unwrap_or_default(), + string: Scope::new("string").unwrap_or_default(), + comment: Scope::new("comment").unwrap_or_default(), + constant: Scope::new("constant").unwrap_or_default(), + }) + } + + /// The tone for the scope stack a token was parsed under. + /// + /// Read from the top of the stack downwards, because the innermost scope is + /// the specific one: a keyword inside a string is a string. + fn tone(&self, stack: &ScopeStack) -> Tone { + for scope in stack.as_slice().iter().rev() { + if self.comment.is_prefix_of(*scope) { + return Tone::Muted; + } + if self.string.is_prefix_of(*scope) { + return Tone::StringLiteral; + } + if self.constant.is_prefix_of(*scope) { + return Tone::Literal; + } + if self.keyword.is_prefix_of(*scope) || self.storage.is_prefix_of(*scope) { + return Tone::Keyword; + } + } + Tone::Normal + } + + /// `text` split into byte runs that share a tone. + /// + /// **A hunk is a fragment of a file, and this parses each line from a clean + /// state.** A block comment or a multi-line string opened above the hunk is + /// therefore not known here and its lines read as code. Carrying state would + /// mean two parsers — one walking the old side of the diff and one the new — + /// fed an interleaved stream, for at most seven lines of context. The + /// ceiling is worth naming and is not worth the machinery. + fn runs(&self, text: &str, syntax: &SyntaxReference) -> Vec<(Range, Tone)> { + // The grammars are the newline-terminated set, so a line without its + // terminator can leave a rule unmatched at the end. + let terminated = format!("{text}\n"); + let mut state = ParseState::new(syntax); + let Ok(ops) = state.parse_line(&terminated, &self.syntaxes) else { + return vec![(0..text.len(), Tone::Normal)]; + }; + + let mut stack = ScopeStack::new(); + let mut runs: Vec<(Range, Tone)> = Vec::new(); + let mut at = 0; + for (range, op) in ScopeRangeIterator::new(&ops, &terminated) { + if stack.apply(op).is_err() { + return vec![(0..text.len(), Tone::Normal)]; + } + // Clamped: the newline added above is not part of the caller's text. + let end = range.end.min(text.len()); + if end <= at { + continue; + } + let tone = self.tone(&stack); + match runs.last_mut() { + Some((last, seen)) if *seen == tone => last.end = end, + _ => runs.push((at..end, tone)), + } + at = end; + } + if at < text.len() { + runs.push((at..text.len(), Tone::Normal)); + } + if runs.is_empty() { + runs.push((0..text.len(), Tone::Normal)); + } + runs + } +} + +/// The grammar for a path, by extension, or `None` when nothing claims it. +/// +/// `None` is not a failure. A diff of a file syntect has no grammar for renders +/// in the terminal's own foreground, which is what it should look like. +fn syntax_for(path: &str) -> Option<&'static SyntaxReference> { + let extension = std::path::Path::new(path).extension()?.to_str()?; + Highlighter::get() + .syntaxes + .find_syntax_by_extension(extension) +} + /// The edits a given step made, out of everything the run recorded. /// /// `Store::edits` answers for the whole run, so a caller that draws what it @@ -60,25 +204,54 @@ pub fn cell(edit: &Edit, theme: &Theme) -> Vec> { return lines; }; - lines.extend(body(hunk, theme)); + lines.extend(body(hunk, &edit.path, theme)); lines.push(Line::from("")); lines } +/// ` src/theme.rs · +1 -1 · edit_file`, and `· no diff stored` when there is +/// no hunk to draw under it. +fn header(edit: &Edit, theme: &Theme) -> Line<'static> { + let mut spans = vec![ + Span::styled(INDENT.to_string(), theme.style(Tone::Muted)), + Span::styled(edit.path.clone(), theme.style(Tone::Accent)), + Span::styled(SEPARATOR.to_string(), theme.style(Tone::Muted)), + Span::styled(format!("+{}", edit.lines_added), theme.style(Tone::Added)), + Span::styled(" ".to_string(), theme.style(Tone::Muted)), + Span::styled( + format!("-{}", edit.lines_removed), + theme.style(Tone::Removed), + ), + Span::styled( + format!("{SEPARATOR}{}", edit.tool), + theme.style(Tone::Muted), + ), + ]; + if edit.hunk.is_none() { + spans.push(Span::styled( + format!("{SEPARATOR}no diff stored"), + theme.style(Tone::Muted), + )); + } + Line::from(spans) +} + /// A hunk body, with the words that changed emphasised where they can be known. /// /// **The pairing rule is deliberately timid.** A run of removals is paired with /// the run of additions immediately after it, and only when the two runs are the /// same length — then line *n* of one is compared with line *n* of the other. -/// Anything else is drawn without emphasis. +/// Anything else is drawn without a pairing, which means the whole line reads as +/// changed. /// /// The reason is in io-harness's own diff module: an `edit_file` is one /// contiguous replacement by construction, but a `write_file` that rewrote two -/// distant regions of a file arrives as **one hunk spanning both**. A rule that -/// paired greedily would then emphasise the difference between lines that have -/// nothing to do with each other, which is worse than not emphasising at all — -/// it invents a relationship and paints it in colour. -fn body(hunk: &str, theme: &Theme) -> Vec> { +/// distant regions arrives as **one hunk spanning both**. A greedy rule would +/// then emphasise the difference between lines that have nothing to do with each +/// other, which is worse than no emphasis — it invents a relationship and paints +/// it. +fn body(hunk: &str, path: &str, theme: &Theme) -> Vec> { + let syntax = syntax_for(path); let raw: Vec<&str> = hunk.lines().collect(); let mut out = Vec::new(); let mut at = 0; @@ -97,66 +270,134 @@ fn body(hunk: &str, theme: &Theme) -> Vec> { let additions = &raw[additions_from..at]; if removals.is_empty() && additions.is_empty() { - out.push(plain(raw[at], theme)); + out.push(unchanged(raw[at], syntax, theme)); at += 1; continue; } let paired = !removals.is_empty() && removals.len() == additions.len(); for (n, line) in removals.iter().enumerate() { - out.push(match paired { - true => against(line, additions[n], Tone::Removed, theme), - false => plain(line, theme), - }); + let other = paired.then(|| additions[n]); + out.push(changed(line, other, Tone::Removed, syntax, theme)); } for (n, line) in additions.iter().enumerate() { - out.push(match paired { - true => against(line, removals[n], Tone::Added, theme), - false => plain(line, theme), - }); + let other = paired.then(|| removals[n]); + out.push(changed(line, other, Tone::Added, syntax, theme)); } } out } -/// One changed line, split into what both sides share and what only this one -/// has. +/// A line that is not an addition or a removal: context, a `@@` header, or the +/// no-final-newline marker. +fn unchanged(line: &str, syntax: Option<&SyntaxReference>, theme: &Theme) -> Line<'static> { + match line.as_bytes().first() { + // `@@ … @@` — where in the file this is. The one part of a hunk a reader + // navigates by, so it takes the product's own colour and no highlighting: + // it is not code. + Some(b'@') => Line::from(Span::styled( + format!("{INDENT}{line}"), + theme.style(Tone::Accent), + )), + // `\ No newline at end of file`. Real information about the bytes, and + // not a change, so it reads as neither. + Some(b'\\') => Line::from(Span::styled( + format!("{INDENT}{line}"), + theme.style(Tone::Muted), + )), + // A context line. Its leading space is part of the diff and stays. + _ => { + let (marker, text) = line.split_at(usize::from(!line.is_empty())); + spans(marker, text, Tone::Normal, None, syntax, theme) + } + } +} + +/// One changed line, against its partner on the other side where there is one. /// -/// Falls back to a plain line when the two share neither a head nor a tail: -/// emphasising the whole line is the same as emphasising none of it, and it is -/// exactly what a reader would have to un-learn on the next hunk. -fn against(mine: &str, other: &str, tone: Tone, theme: &Theme) -> Line<'static> { +/// With a partner, the shared head and tail are syntax coloured and only the +/// middle takes the diff's colour. Without one — or when the two share neither a +/// head nor a tail, in which case "what changed" is the whole line — the body +/// takes the diff's colour throughout. +fn changed( + mine: &str, + other: Option<&str>, + tone: Tone, + syntax: Option<&SyntaxReference>, + theme: &Theme, +) -> Line<'static> { // The marker is the first byte and is ASCII, so this never splits a // character. It stays on the line — it is what carries the meaning when // colour does not. let (marker, text) = mine.split_at(1); - let other = &other[1..]; - - let head = common_head(text, other); - // Measured on what is left after the head, so a line like `a` against `aa` - // cannot count the same byte twice and produce a middle of negative length. - let tail = common_tail(&text[head..], &other[head..]); - if head == 0 && tail == 0 { - return plain(mine, theme); - } + let range = other.and_then(|other| { + let other = &other[1..]; + let head = common_head(text, other); + // Measured on what is left after the head, so `a` against `aa` cannot + // count the same byte twice and produce a middle of negative length. + let tail = common_tail(&text[head..], &other[head..]); + // Sharing neither a head nor a tail means the whole line changed, which + // is the same situation as having no partner at all. + (head != 0 || tail != 0).then(|| head..text.len() - tail) + }); - let mut spans = vec![Span::styled( - format!("{INDENT}{marker}{}", &text[..head]), - theme.style(tone), - )]; - let middle = &text[head..text.len() - tail]; - if !middle.is_empty() { - spans.push(Span::styled(middle.to_string(), theme.emphasis(tone))); - } - if tail > 0 { - spans.push(Span::styled( - text[text.len() - tail..].to_string(), + let Some(range) = range else { + // No partner, so nothing to emphasise *against* — the whole body takes + // the diff's colour and no emphasis. Bolding a whole line says "look + // here" about every word on it, which is the same as saying nothing. + return Line::from(Span::styled( + format!("{INDENT}{marker}{text}"), theme.style(tone), )); + }; + + spans(marker, text, tone, Some(range), syntax, theme) +} + +/// Build a body line: the marker in `tone`, the text syntax coloured, and +/// `changed` — where there is one — in `tone` and emphasised. +fn spans( + marker: &str, + text: &str, + tone: Tone, + changed: Option>, + syntax: Option<&SyntaxReference>, + theme: &Theme, +) -> Line<'static> { + let mut out = vec![Span::styled(format!("{INDENT}{marker}"), theme.style(tone))]; + + let runs = match syntax { + Some(syntax) => Highlighter::get().runs(text, syntax), + // No grammar for this file, so every run is one run. + None => vec![(0..text.len(), Tone::Normal)], + }; + + for (run, run_tone) in runs { + let mut at = run.start; + while at < run.end { + // Three cases, and the loop advances in every one: before the change, + // inside it, or past it. + let (end, inside) = match &changed { + Some(range) if at < range.start => (run.end.min(range.start), false), + Some(range) if at < range.end => (run.end.min(range.end), true), + _ => (run.end, false), + }; + if end <= at { + break; + } + let piece = text[at..end].to_string(); + out.push(match inside { + true => Span::styled(piece, theme.emphasis(tone)), + false => Span::styled(piece, theme.style(run_tone)), + }); + at = end; + } } - Line::from(spans) + + // A changed line whose text is empty still needs its marker to be a line. + Line::from(out) } /// How many bytes `a` and `b` agree on from the front, always on a character @@ -183,52 +424,3 @@ fn common_tail(a: &str, b: &str) -> usize { } bytes } - -/// ` src/theme.rs · +1 -1 · edit_file`, and `· no diff stored` when there is -/// no hunk to draw under it. -fn header(edit: &Edit, theme: &Theme) -> Line<'static> { - let mut spans = vec![ - Span::styled(INDENT.to_string(), theme.style(Tone::Muted)), - Span::styled(edit.path.clone(), theme.style(Tone::Accent)), - Span::styled(SEPARATOR.to_string(), theme.style(Tone::Muted)), - Span::styled(format!("+{}", edit.lines_added), theme.style(Tone::Added)), - Span::styled(" ".to_string(), theme.style(Tone::Muted)), - Span::styled( - format!("-{}", edit.lines_removed), - theme.style(Tone::Removed), - ), - Span::styled( - format!("{SEPARATOR}{}", edit.tool), - theme.style(Tone::Muted), - ), - ]; - if edit.hunk.is_none() { - spans.push(Span::styled( - format!("{SEPARATOR}no diff stored"), - theme.style(Tone::Muted), - )); - } - Line::from(spans) -} - -/// One line of a hunk body, coloured by the marker the harness put on it and -/// emphasised nowhere. -/// -/// The marker stays on the line. Colour is never the sole carrier of meaning -/// here — under `NO_COLOR` every style collapses to nothing and the `+` and the -/// `-` are what is left saying which side a line is on. -fn plain(line: &str, theme: &Theme) -> Line<'static> { - let tone = match line.as_bytes().first() { - Some(b'+') => Tone::Added, - Some(b'-') => Tone::Removed, - // `@@ … @@` — where in the file this is. The one part of a hunk a reader - // navigates by, so it gets the product's own colour rather than the - // muted one context lines take. - Some(b'@') => Tone::Accent, - // `\ No newline at end of file`. Real information about the bytes, and - // not a change, so it reads as neither. - Some(b'\\') => Tone::Muted, - _ => Tone::Muted, - }; - Line::from(Span::styled(format!("{INDENT}{line}"), theme.style(tone))) -} diff --git a/src/theme.rs b/src/theme.rs index 35b24d8..04dd8dc 100644 --- a/src/theme.rs +++ b/src/theme.rs @@ -1,4 +1,4 @@ -//! The theme: nine tokens, two shipped themes, and a rule that colour is never +//! The theme: twelve tokens, two shipped themes, and a rule that colour is never //! the only thing carrying a meaning. //! //! The token set is deliberately small. Restraint is what makes a terminal @@ -6,6 +6,16 @@ //! consistently, and every extra token is another thing a second theme has to get //! right. //! +//! It was nine until 0.3.0, when syntax highlighting inside a diff added three: +//! keyword, string and literal. They are here rather than in the highlighter's +//! own theme format on purpose — `syntect` ships themes and this product does not +//! load them, so a highlighted diff and the rest of the interface stay one +//! aesthetic, and `NO_COLOR` keeps working because there is still exactly one +//! place that decides whether colour happens at all. A comment did not get a +//! fourth token: a comment is muted, `muted` already exists, and a token whose +//! value would always equal another's is a token that can drift out of agreement +//! with itself. +//! //! Nothing here paints a background. io-cli does not own the screen — the //! transcript is the terminal's own scrollback, sitting on whatever background //! the user chose — so a background token exists to *detect* against, not to @@ -80,6 +90,12 @@ pub enum Tone { Added, /// A line a change removed. Removed, + /// A language keyword inside a diff. + Keyword, + /// A string literal inside a diff. + StringLiteral, + /// A number, a boolean, a named constant. + Literal, } impl Tone { @@ -90,7 +106,14 @@ impl Tone { // A diff line's carrier is the `+` or the `-` the harness already // put on it, which is why these two need no word of their own: the // meaning survives `NO_COLOR` without one. - Self::Normal | Self::Muted | Self::Accent | Self::Added | Self::Removed => None, + Self::Normal + | Self::Muted + | Self::Accent + | Self::Added + | Self::Removed + | Self::Keyword + | Self::StringLiteral + | Self::Literal => None, Self::Success => Some("ok"), Self::Warning => Some("warning"), Self::Error => Some("error"), @@ -105,7 +128,7 @@ impl fmt::Display for Tone { } } -/// The nine tokens. +/// The twelve tokens. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Theme { pub name: &'static str, @@ -120,6 +143,13 @@ pub struct Theme { pub error: Color, pub diff_add: Color, pub diff_delete: Color, + /// The three syntax tokens. Comments are deliberately not among them: a + /// comment is muted, and `muted` already exists — a fourth token whose value + /// would always equal an existing one is a token that can drift out of + /// agreement with itself. + pub syntax_keyword: Color, + pub syntax_string: Color, + pub syntax_literal: Color, /// Whether this theme emits colour at all. The `NO_COLOR` theme does not. pub coloured: bool, } @@ -139,6 +169,14 @@ pub const DARK: Theme = Theme { error: Color::LightRed, diff_add: Color::LightGreen, diff_delete: Color::LightRed, + // Indexed rather than the sixteen, and the reason is crowding: the eight + // bright ANSI colours are already spoken for by accent, success, warning, + // error and the two diff tokens, so a syntax colour taken from them would + // read as one of those meanings. These three are muted enough to sit under + // a diff without competing with the green and red that carry it. + syntax_keyword: Color::Indexed(176), + syntax_string: Color::Indexed(114), + syntax_literal: Color::Indexed(180), coloured: true, }; @@ -158,6 +196,9 @@ pub const LIGHT: Theme = Theme { error: Color::Red, diff_add: Color::Green, diff_delete: Color::Red, + syntax_keyword: Color::Indexed(90), + syntax_string: Color::Indexed(28), + syntax_literal: Color::Indexed(130), coloured: true, }; @@ -173,6 +214,9 @@ pub const PLAIN: Theme = Theme { error: Color::Reset, diff_add: Color::Reset, diff_delete: Color::Reset, + syntax_keyword: Color::Reset, + syntax_string: Color::Reset, + syntax_literal: Color::Reset, coloured: false, }; @@ -232,6 +276,9 @@ impl Theme { .add_modifier(Modifier::BOLD), Tone::Added => Style::default().fg(self.diff_add), Tone::Removed => Style::default().fg(self.diff_delete), + Tone::Keyword => Style::default().fg(self.syntax_keyword), + Tone::StringLiteral => Style::default().fg(self.syntax_string), + Tone::Literal => Style::default().fg(self.syntax_literal), } } diff --git a/tests/dependencies.rs b/tests/dependencies.rs index 454aa00..8b60f68 100644 --- a/tests/dependencies.rs +++ b/tests/dependencies.rs @@ -27,6 +27,11 @@ const ALLOWED: &[&str] = &[ "tokio", "serde", "toml", + // Ninth, and the first name added since 0.1.0. Syntax highlighting inside a + // diff, argued in 0.3.0's release record. Its FEATURE SET is asserted below + // as well as its name — see `n2_syntect_is_taken_with_exactly_the_features_ + // the_cross_compiles_survive`. + "syntect", ]; /// Crates that would mean a subsystem had been rebuilt here. Matched against @@ -231,3 +236,93 @@ fn n1_the_crate_is_never_published() { remembered", ); } + +/// N2 — the feature set, which is the load-bearing half of taking syntect. +/// +/// The name alone is not the contract. `syntect`'s DEFAULT features include +/// `regex-onig`, which is oniguruma — a C library. Two of this product's four +/// release artifacts are cross-compiled, to `x86_64-unknown-linux-musl` and +/// `x86_64-pc-windows-msvc`, and a native build step in that path fails inside +/// the release workflow rather than in a test. So a later `default-features = +/// true`, or a merge that drops the feature list, has to fail here — where it +/// costs a red check — instead of there, where it costs the Release. +#[test] +fn n2_syntect_is_taken_with_exactly_the_features_the_cross_compiles_survive() { + let manifest = manifest(); + let syntect = manifest + .get("dependencies") + .and_then(|table| table.get("syntect")) + .and_then(toml::Value::as_table) + .expect("syntect is a table, not a bare version string"); + + assert_eq!( + syntect + .get("default-features") + .and_then(toml::Value::as_bool), + Some(false), + "syntect's default features include regex-onig, which is a C library and \ + breaks the musl and MSVC cross-compiles", + ); + + let features: Vec<&str> = syntect + .get("features") + .and_then(toml::Value::as_array) + .expect("a features array") + .iter() + .filter_map(toml::Value::as_str) + .collect(); + assert_eq!( + features, + ["default-syntaxes", "regex-fancy"], + "the smallest syntect that highlights: the bundled grammars and the \ + pure-Rust regex engine. `default-themes` is deliberately absent — the \ + colours are io-cli's own tokens, which is what keeps a highlighted diff \ + and the rest of the interface one aesthetic and NO_COLOR working from \ + one place.", + ); +} + +/// N4 — the grammar table is never touched on the startup path. +/// +/// `SyntaxSet::load_defaults_newlines` decompresses every grammar syntect ships. +/// This product's own bar is a first paint inside a hundred milliseconds, and a +/// session that never edits a file should never pay for a highlighter. Asserted +/// structurally rather than by timing anything: the load appears once, in the +/// module that draws diffs, behind a `OnceLock`. +#[test] +fn n4_the_syntax_set_is_loaded_lazily_and_only_by_the_diff_renderer() { + let mut loaders = Vec::new(); + for (path, text) in sources() { + if text.contains("load_defaults") { + loaders.push(path.clone()); + } + } + + assert_eq!( + loaders.len(), + 1, + "the grammar table should be loaded in exactly one place: {loaders:?}", + ); + assert!( + loaders[0].ends_with("diff.rs"), + "only the diff renderer has any reason to load grammars: {loaders:?}", + ); + + let diff = std::fs::read_to_string(&loaders[0]).expect("the diff renderer"); + assert!( + diff.contains("OnceLock"), + "the load has to sit behind a once-cell, or it happens at startup", + ); + + // And the startup path does not reach it. `main.rs` builds the terminal, the + // config and the session before anything is drawn; a highlighter mentioned + // there is a highlighter built before the first frame. + let main = std::fs::read_to_string( + std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src/main.rs"), + ) + .expect("the driver"); + assert!( + !main.contains("Highlighter") && !main.contains("SyntaxSet"), + "the driver reached for the highlighter, which puts it on the startup path", + ); +} diff --git a/tests/diff.rs b/tests/diff.rs index 22cf495..4b7a0e1 100644 --- a/tests/diff.rs +++ b/tests/diff.rs @@ -353,3 +353,157 @@ fn f3_an_unpaired_line_is_emphasised_at_the_line_and_not_within_it() { "an unpaired line is one span", ); } + +// --------------------------------------------------------------------------- +// F4 — highlighting is drawn in io-cli's own tokens and disappears under +// NO_COLOR. N2 — the dependency set grows by exactly one name, with its +// features pinned. N4 — the syntax set is never loaded on the startup path. +// --------------------------------------------------------------------------- + +/// A hunk of Rust with a keyword, a string and a number in it, so each of the +/// three syntax tokens has something to colour. +fn edit_with_code() -> Edit { + Edit { + step: 1, + tool: "edit_file".to_string(), + path: "src/lib.rs".to_string(), + lines_added: 1, + lines_removed: 1, + hunk: Some( + "@@ -1,3 +1,3 @@\n \ + // a comment\n\ + -let name = \"old\";\n\ + +let name = \"new\";\n \ + const N: u32 = 42;\n" + .to_string(), + ), + } +} + +#[test] +fn f4_the_colours_come_from_io_cli_s_own_tokens() { + let edit = edit_with_code(); + let coloured: Vec<(String, Option)> = cell(&edit, &DARK) + .iter() + .flat_map(|line| { + line.spans + .iter() + .map(|span| (span.content.to_string(), span.style.fg)) + .collect::>() + }) + .collect(); + let used: Vec = coloured.iter().filter_map(|(_, fg)| *fg).collect(); + + // Every colour on a context line is one this theme declares. A colour from + // syntect's own theme set would be an RGB value that appears nowhere in + // `theme.rs` — which is the whole reason `default-themes` is off. + let mine = [ + DARK.foreground, + DARK.muted, + DARK.accent, + DARK.syntax_keyword, + DARK.syntax_string, + DARK.syntax_literal, + DARK.diff_add, + DARK.diff_delete, + ]; + for colour in &used { + assert!( + mine.contains(colour), + "{colour:?} is not one of io-cli's tokens: {coloured:?}", + ); + } + assert!( + !used.is_empty(), + "nothing was coloured at all, so this asserts nothing: {coloured:?}", + ); +} + +#[test] +fn f4_a_keyword_a_string_and_a_number_each_get_their_own_token() { + let edit = edit_with_code(); + let all: Vec<(String, Option)> = cell(&edit, &DARK) + .iter() + .flat_map(|line| { + line.spans + .iter() + .map(|span| (span.content.to_string(), span.style.fg)) + .collect::>() + }) + .collect(); + + let has = |text: &str, colour: ratatui::style::Color| { + all.iter() + .any(|(content, fg)| content.contains(text) && *fg == Some(colour)) + }; + + // `let` and `const` are `storage.*` in Sublime's scope vocabulary rather + // than `keyword.*`. Asserting on them rather than on an operator is what + // caught the missing `storage` entry in the scope table. + assert!(has("let", DARK.syntax_keyword), "let is a keyword: {all:?}"); + assert!( + has("const", DARK.syntax_keyword), + "const is a keyword: {all:?}" + ); + assert!( + has("old", DARK.diff_delete), + "the changed word keeps the diff colour: {all:?}" + ); + assert!(has("42", DARK.syntax_literal), "literal: {all:?}"); + assert!( + has("a comment", DARK.muted), + "comment reads as muted: {all:?}" + ); +} + +#[test] +fn f4_no_color_leaves_the_markers_carrying_the_meaning() { + use io_cli::theme::PLAIN; + + let lines = cell(&edit_with_code(), &PLAIN); + for line in &lines { + for span in &line.spans { + assert_eq!( + span.style.fg, None, + "NO_COLOR emitted a colour: {:?}", + span.content, + ); + assert!( + span.style.add_modifier.is_empty(), + "NO_COLOR emitted a modifier, which is still a presentation-only \ + carrier: {:?}", + span.content, + ); + } + } + + // And the meaning survives, because the markers are text. + let text: String = lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>() + .join("\n"); + assert!(text.contains("-let name = \"old\";"), "{text}"); + assert!(text.contains("+let name = \"new\";"), "{text}"); +} + +#[test] +fn f4_a_file_with_no_grammar_still_renders() { + // `None` from `find_syntax_by_extension` is not a failure — it is what a + // file syntect has never heard of should look like. + let edit = Edit { + step: 1, + tool: "write_file".to_string(), + path: "notes.zzz".to_string(), + lines_added: 1, + lines_removed: 0, + hunk: Some("@@ -0,0 +1 @@\n+hello\n".to_string()), + }; + let text = rendered(&edit); + assert!(text.contains("+hello"), "{text}"); +} From 2eb3143252ee93b8e776c4e6e7377c94f12febb2 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Mon, 17 Aug 2026 09:31:04 +0530 Subject: [PATCH 4/8] feat: the clipboard and transcript renderers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two modules, not yet reachable from a key — the bindings and the slash commands follow. Registered now so their tests run with the suite rather than sitting outside it. clipboard: OSC 52, because the product is used over SSH and a local clipboard call would set the clipboard on the wrong machine. The base64 is twenty lines rather than a tenth dependency. There is deliberately no success type: no terminal answers an OSC 52 write, several cap the payload silently, and tmux ignores it entirely without set-clipboard on, so `describe` returns a sentence stating what was sent and how large it was. A call site has nothing it could render as "copied". Encoding is also what makes the payload safe rather than merely compact — an unencoded ESC or BEL in transcript text would terminate the OSC string early and dump the remainder on screen. transcript: renders a whole conversation as scrollback lines, never viewport ones. Turns a branch left behind are rendered AND labelled: TranscriptTurn.on_path is the only place that fact is reachable and Session::history deliberately does not return them, so dropping them silently is the defect the criterion exists to catch. Both Transcript and TranscriptTurn are #[non_exhaustive], so a test cannot build one. tests/transcript.rs therefore drives three real turns through a scripted provider into an in-memory store and branches the middle one away, which is how the branched-away case is proved rather than described. --- src/clipboard.rs | 124 ++++++++++++++++++++++++++++++ src/lib.rs | 2 + src/transcript.rs | 156 ++++++++++++++++++++++++++++++++++++++ tests/clipboard.rs | 161 +++++++++++++++++++++++++++++++++++++++ tests/transcript.rs | 179 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 622 insertions(+) create mode 100644 src/clipboard.rs create mode 100644 src/transcript.rs create mode 100644 tests/clipboard.rs create mode 100644 tests/transcript.rs diff --git a/src/clipboard.rs b/src/clipboard.rs new file mode 100644 index 0000000..77d182f --- /dev/null +++ b/src/clipboard.rs @@ -0,0 +1,124 @@ +//! Putting text on the clipboard of the terminal a session is actually being +//! watched on, which over SSH is not this machine. +//! +//! io-cli is used through an SSH connection and inside tmux. A local clipboard +//! call — pasteboard, X11, Wayland — sets the clipboard of the host the agent is +//! running on, which is the wrong computer: the reader is at the far end of the +//! connection, and the text they asked for would land somewhere they cannot +//! reach. OSC 52 is the one mechanism that goes the other way. It is an escape +//! sequence, so it travels back up the same stream the transcript does and is +//! interpreted by the terminal emulator in front of the reader. +//! +//! # Nothing ever answers +//! +//! A terminal does not reply to an OSC 52 write. There is no acknowledgement, no +//! error and no refusal on the wire — the sequence is written and that is the end +//! of what this process can observe. Meanwhile the two most common deployments +//! both drop it quietly: tmux discards the sequence unless the user has set +//! `set -g set-clipboard on`, and most terminals silently ignore a payload past +//! some internal ceiling they do not publish. +//! +//! So a `copied` message here would be a claim io-cli cannot support, printed at +//! exactly the moment the reader goes to paste and finds the old contents. That +//! is why this module hands back [`describe`] and not a `bool`: there is no +//! success value to return, and the shape of the API is what keeps a call site +//! from inventing one. What can honestly be said is what was sent and how big it +//! was, which is also the useful thing — a reader whose paste comes up empty can +//! compare the size against their terminal's limit and their tmux setting. +//! +//! # The size ceiling +//! +//! io-cli sends the payload whatever its size. The cap is the terminal's, it +//! differs between emulators, and nothing on this side can query it, so refusing +//! to send at some invented threshold would fail writes that would have worked. +//! Sending and reporting the size leaves the reader holding the one fact that +//! makes the failure diagnosable. Note that the sequence on the wire is about a +//! third larger than the payload, because base64 spends four characters on every +//! three bytes, and it is the encoded form the terminal measures. +//! +//! Nothing here wraps the sequence for tmux's DCS passthrough. Passthrough has to +//! be applied only when actually inside tmux, and the sequence sent to a terminal +//! that is not tmux is garbage printed into the transcript; `$TMUX` is not a +//! reliable enough signal to bet the transcript on, and `set-clipboard on` is a +//! setting the user can make once and keep. + +/// Standard base64, the `+/` alphabet of RFC 4648 — not the URL-safe variant. +/// OSC 52 payloads are read by terminal emulators that implement the original, +/// and a `-_` payload decodes to different bytes rather than failing loudly. +const ALPHABET: &[u8; 64] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; + +/// The full sequence to write to the terminal to put `payload` on the system +/// clipboard: `ESC ] 52 ; c ; BEL`. +/// +/// Selection `c` is the system clipboard, the one a paste reads. The other +/// selections OSC 52 can address — the X11 primary and the cut buffers — are not +/// where a reader who pressed the copy key expects to find their text. +/// +/// The encoding is not decoration. An OSC string ends at the first BEL or ST, so +/// a payload carrying either byte — and a transcript can carry both, since tool +/// output is arbitrary — would terminate the sequence early and leave the rest of +/// itself printed on screen as text. Base64 removes every byte that could do +/// that, which is why the sequence is built here rather than by a caller. +/// +/// Terminated with BEL rather than ST because the whole xterm lineage and tmux +/// accept BEL, while a handful of terminals only ever learned that form. +pub fn sequence(payload: &str) -> String { + format!("\x1b]52;c;{}\x07", encode(payload.as_bytes())) +} + +/// Base64-encode `bytes`, with `=` padding. +/// +/// Hand-written because it is twenty lines against a dependency, and this crate's +/// dependency list is asserted by `tests/dependencies.rs` — the list is the claim +/// that io-cli contains no agent machinery of its own, and it is worth more than +/// the twenty lines cost. +pub fn encode(bytes: &[u8]) -> String { + let mut encoded = String::with_capacity(bytes.len().div_ceil(3) * 4); + for chunk in bytes.chunks(3) { + // The missing bytes of a short final chunk encode as zero bits, and the + // characters that carry only those bits become padding below. + let second = chunk.get(1).copied().unwrap_or(0); + let third = chunk.get(2).copied().unwrap_or(0); + let group = (u32::from(chunk[0]) << 16) | (u32::from(second) << 8) | u32::from(third); + for position in 0..4 { + // One input byte fills two output characters, two fill three, three + // fill four; everything past that is `=`. + if position <= chunk.len() { + let index = (group >> (18 - position * 6)) & 0b11_1111; + encoded.push(ALPHABET[index as usize] as char); + } else { + encoded.push('='); + } + } + } + encoded +} + +/// What can truthfully be said after writing [`sequence`]: `sent 4.2 kB to the +/// terminal clipboard`. +/// +/// Deliberately not a past-tense claim about the clipboard's contents. See the +/// module documentation — no terminal answers an OSC 52 write, so `copied` is a +/// statement about something this process never observes. This wording says only +/// what io-cli did, which is the part it knows. +/// +/// The size is the payload's, in bytes rather than characters, because bytes are +/// what a terminal's limit is expressed in and a line of CJK text is three times +/// its own length on the wire. +pub fn describe(payload: &str) -> String { + format!( + "sent {} to the terminal clipboard", + format_bytes(payload.len()) + ) +} + +/// `1 byte`, `840 bytes`, `4.2 kB`. Decimal kilobytes, matching the `kB` it is +/// spelled with: the number exists to be held against a terminal's documented +/// limit, and those limits are quoted in round decimal thousands. +fn format_bytes(bytes: usize) -> String { + match bytes { + 1 => "1 byte".to_string(), + 0..=999 => format!("{bytes} bytes"), + _ => format!("{:.1} kB", bytes as f64 / 1000.0), + } +} diff --git a/src/lib.rs b/src/lib.rs index ac132bf..0815813 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,6 +14,7 @@ pub mod app; pub mod approval; pub mod bridge; pub mod cli; +pub mod clipboard; pub mod commands; pub mod composer; pub mod diff; @@ -24,5 +25,6 @@ pub mod splash; pub mod status; pub mod term; pub mod theme; +pub mod transcript; pub mod verify; pub mod wizard; diff --git a/src/transcript.rs b/src/transcript.rs new file mode 100644 index 0000000..fd9d8ad --- /dev/null +++ b/src/transcript.rs @@ -0,0 +1,156 @@ +//! The whole conversation, rendered for the terminal's own scrollback. +//! +//! This is what `Ctrl+T` commits. It is not a screen and cannot become one: the +//! viewport is four fixed rows that ratatui will not let anything grow, and there +//! is no alternate screen in this product to page a long conversation in. So the +//! output here is a `Vec` handed to `Screen::commit`, which pushes it above +//! the viewport with `insert_before` and leaves it in the terminal's real +//! scrollback — where the terminal's own scroll, search and copy already work. +//! Anything that paginated this would be re-implementing the scrollback the +//! product deliberately kept. +//! +//! **Branched-away turns are rendered, and said out loud.** +//! [`io_harness::Session::history`] returns only the turns the model can still +//! see, which is right for driving a next turn and wrong for reading back what +//! happened: a branch leaves earlier turns off the path, and +//! [`TranscriptTurn::on_path`] is the only place in this whole product where they +//! are visible at all. A renderer that skipped them would silently disagree with +//! the database about what the operator did, in the one surface built to answer +//! that — so every turn appears, and the ones the model has lost carry the words +//! saying so rather than a colour saying so. +//! +//! The compaction summaries each turn carries are not drawn. Nothing else in this +//! release renders a fold, and one is a paragraph standing in for steps whose own +//! narration is not on screen either; drawing it alone would raise a question the +//! interface has no other answer to yet. + +use io_harness::{Transcript, TranscriptTurn}; +use ratatui::text::{Line, Span}; + +use crate::events::outcome_tone; +use crate::status::SEPARATOR; +use crate::theme::{Theme, Tone}; + +/// The words a turn the model can no longer see is labelled with. +/// +/// Words rather than a tone, because a tone is not readable under `NO_COLOR`, on +/// a monochrome terminal, or to a colour-blind reader — and this is the single +/// fact about a conversation that a reader cannot reconstruct from anywhere else +/// in the product. +pub const BRANCHED_AWAY: &str = "left behind by a branch"; + +/// The conversation as lines, oldest turn first. +/// +/// Opens and closes with a marker so the transcript has visible edges in a +/// scrollback that also holds every earlier turn, every command's output and +/// whatever the shell printed before `io` started. The closing marker carries the +/// turn count, which is what tells a reader that the passage they just scrolled +/// past was the whole conversation rather than as much of it as fitted. +pub fn lines(transcript: &Transcript, theme: &Theme) -> Vec> { + let mut out = vec![Line::from(Span::styled( + format!( + "─── transcript begins{SEPARATOR}session {}{SEPARATOR}{}", + transcript.session_id, + transcript.root.display() + ), + theme.style(Tone::Accent), + ))]; + + if transcript.turns.is_empty() { + // A sentence, not an empty gap. Committing nothing at all is + // indistinguishable from the key having done nothing at all. + out.push(Line::from(Span::styled( + "This session has no turns yet, so there is nothing to show.".to_string(), + theme.style(Tone::Muted), + ))); + } + + for turn in &transcript.turns { + out.extend(turn_lines(turn, theme)); + } + + let count = transcript.turns.len(); + let branched = transcript.turns.iter().filter(|turn| !turn.on_path).count(); + let mut closing = format!( + "─── transcript ends{SEPARATOR}{count} turn{}", + if count == 1 { "" } else { "s" } + ); + if branched > 0 { + closing.push_str(&format!("{SEPARATOR}{branched} {BRANCHED_AWAY}")); + } + out.push(Line::from(Span::styled(closing, theme.style(Tone::Accent)))); + out +} + +/// One turn: what was asked, what was answered, and how it ended. +/// +/// The prompt comes before the turn id on its line, never after it — the +/// codebase-wide rule that content precedes metadata. A reader skims the left +/// edge for what was said, an id at the head of every line makes that column +/// unreadable, and the id is only ever wanted once the line has already been +/// found. +fn turn_lines(turn: &TranscriptTurn, theme: &Theme) -> Vec> { + // An empty prompt is still one row: `str::lines` yields nothing for an empty + // string, and a turn with no row at all would lose its id and its branch + // label along with it. + let mut rows: Vec<&str> = turn.prompt.lines().collect(); + if rows.is_empty() { + rows.push(""); + } + + let mut lines: Vec> = Vec::new(); + for (index, row) in rows.iter().enumerate() { + let marker = if index == 0 { "› " } else { " " }; + lines.push(Line::from(vec![ + Span::styled(marker, theme.style(Tone::Accent)), + Span::styled((*row).to_string(), theme.style(Tone::Normal)), + ])); + } + + // The tail rides the LAST prompt row rather than the first, so a multi-line + // prompt is not interrupted by its own metadata halfway down. + let tail = lines.last_mut().expect("a prompt is at least one row"); + if !turn.on_path { + tail.spans + .push(Span::styled(SEPARATOR, theme.style(Tone::Muted))); + tail.spans + .push(Span::styled(BRANCHED_AWAY, theme.style(Tone::Warning))); + } + tail.spans.push(Span::styled( + format!("{SEPARATOR}turn {}", turn.turn_id), + theme.style(Tone::Muted), + )); + + match turn + .reply + .as_deref() + .filter(|reply| !reply.trim().is_empty()) + { + Some(reply) => { + for row in reply.lines() { + let span = Span::styled(row.to_string(), theme.style(Tone::Normal)); + lines.push(Line::from(span)); + } + } + // Said, not omitted. A turn that produced no text is a different fact from + // a turn missing from the transcript, and only one of those is worth + // going to look into. + None => lines.push(Line::from(Span::styled( + " no reply".to_string(), + theme.style(Tone::Muted), + ))), + } + + if let Some(outcome) = &turn.outcome { + // `notice` is how every toned line in this product is built, so the tone's + // word is in front of the sentence by construction rather than by anyone + // remembering to put it there. + lines.push(theme.notice( + outcome_tone(outcome), + format!("{outcome}{SEPARATOR}run {}", turn.run_id), + )); + } + + lines.push(Line::from("")); + lines +} diff --git a/tests/clipboard.rs b/tests/clipboard.rs new file mode 100644 index 0000000..359903c --- /dev/null +++ b/tests/clipboard.rs @@ -0,0 +1,161 @@ +//! The clipboard write: a well-formed OSC 52 sequence, and wording that does not +//! claim an acknowledgement the terminal never sends. +//! +//! The base64 vectors here are the RFC 4648 ones, checked by hand rather than +//! produced by the encoder under test — an expectation generated from the code it +//! is testing agrees with every bug in it. + +use io_cli::clipboard::{describe, encode, sequence}; + +const ALPHABET: &str = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; + +/// The other direction, written out here so the round trip is checked against +/// something independent of `encode`. +fn decode(text: &str) -> Vec { + let mut bytes = Vec::new(); + let mut bits: u32 = 0; + let mut held = 0u32; + for character in text.chars().filter(|character| *character != '=') { + let value = ALPHABET + .find(character) + .unwrap_or_else(|| panic!("{character:?} is not a base64 character")); + bits = (bits << 6) | value as u32; + held += 6; + if held >= 8 { + held -= 8; + bytes.push((bits >> held) as u8); + } + } + bytes +} + +/// Split a sequence into its three parts by position, never by searching for the +/// payload inside it: a `contains` check passes just as happily on a sequence +/// whose prefix and payload came out the wrong way round. +fn payload_of(sequence: &str) -> &str { + let body = sequence + .strip_prefix("\x1b]52;c;") + .expect("the sequence opens with OSC 52, selection c"); + body.strip_suffix('\x07') + .expect("the sequence is terminated by BEL") +} + +#[test] +fn the_sequence_is_osc_52_around_the_encoded_payload() { + let text = "the quick brown fox"; + let written = sequence(text); + + let encoded = payload_of(&written); + assert_eq!( + encoded, "dGhlIHF1aWNrIGJyb3duIGZveA==", + "the middle of the sequence is not the base64 of the payload", + ); + assert_eq!( + decode(encoded), + text.as_bytes(), + "the payload did not survive the round trip", + ); + + // Positions, not membership. The introducer belongs at the front and the + // terminator at the back, and only one of each may exist. + assert_eq!(written.find('\x1b'), Some(0), "got {written:?}"); + assert_eq!( + written.find('\x07'), + Some(written.len() - 1), + "a BEL before the end would terminate the sequence early: {written:?}", + ); + assert_eq!(written.matches('\x07').count(), 1, "got {written:?}"); +} + +#[test] +fn a_payload_carrying_a_terminator_cannot_end_the_sequence_early() { + // Tool output is arbitrary bytes, so a transcript really can contain a BEL or + // an ESC. Encoded, neither reaches the terminal as itself. + let text = "before\x07after\x1b]0;title\x07"; + let written = sequence(text); + let encoded = payload_of(&written); + + assert!( + !encoded.contains('\x07') && !encoded.contains('\x1b'), + "a control byte reached the wire unencoded: {encoded:?}", + ); + assert_eq!( + decode(encoded), + text.as_bytes(), + "the round trip lost bytes" + ); +} + +#[test] +fn base64_pads_correctly_at_every_length() { + // RFC 4648's own vectors, at the three residues modulo three and either side + // of the first full group. + let vectors: [(&str, &str); 5] = [ + ("", ""), + ("f", "Zg=="), + ("fo", "Zm8="), + ("foo", "Zm9v"), + ("foob", "Zm9vYg=="), + ]; + for (input, expected) in vectors { + assert_eq!( + encode(input.as_bytes()), + expected, + "{input:?} encoded wrong", + ); + } +} + +#[test] +fn the_last_two_characters_of_the_alphabet_are_plus_and_slash() { + // The URL-safe variant differs from the standard one in exactly these two + // characters, and it differs silently: a `-_` payload decodes to other bytes + // rather than being rejected, so a terminal would paste something else. + assert_eq!(encode(&[0xFB, 0xFF, 0xFE]), "+//+"); + assert_eq!(encode(&[0xFF, 0xFF, 0xFF]), "////"); +} + +#[test] +fn multi_byte_utf8_survives_the_round_trip() { + let text = "café ☕ — 日本語 🙂"; + let written = sequence(text); + let decoded = decode(payload_of(&written)); + + assert_eq!(decoded, text.as_bytes(), "the bytes changed"); + assert_eq!( + String::from_utf8(decoded).expect("still valid UTF-8"), + text, + "the text changed", + ); +} + +#[test] +fn the_description_states_a_size_and_claims_nothing_about_the_clipboard() { + // Nothing on the wire answers an OSC 52 write, and tmux drops it outright + // without `set -g set-clipboard on`. Any of these words would be io-cli + // asserting an outcome it never observed. + let large = "a".repeat(4200); + for payload in ["", "x", "the quick brown fox", large.as_str()] { + let description = describe(payload).to_lowercase(); + for claim in ["copied", "success", "done"] { + assert!( + !description.contains(claim), + "{description:?} claims {claim:?}, which the terminal never confirmed", + ); + } + assert!( + description.starts_with("sent "), + "the description should open with what io-cli did: {description:?}", + ); + } + + assert_eq!(describe(""), "sent 0 bytes to the terminal clipboard"); + assert_eq!(describe("x"), "sent 1 byte to the terminal clipboard"); + assert_eq!(describe(&large), "sent 4.2 kB to the terminal clipboard"); + // Bytes, not characters: a terminal's undocumented ceiling is a byte count, + // and this string is four characters long. + assert_eq!( + describe("日本語 "), + "sent 10 bytes to the terminal clipboard" + ); +} diff --git a/tests/transcript.rs b/tests/transcript.rs new file mode 100644 index 0000000..2ccdb0c --- /dev/null +++ b/tests/transcript.rs @@ -0,0 +1,179 @@ +//! The whole conversation, committed to scrollback. +//! +//! The discriminating assertion is the first one: a session that branched holds a +//! turn `Session::history` no longer returns, and this renderer is the only +//! surface in the product that can show it. A renderer built from the path passes +//! every other assertion here and fails that one — silently, because a turn that +//! is missing looks exactly like a turn that never happened. +//! +//! `io_harness::Transcript` and `io_harness::TranscriptTurn` are both +//! `#[non_exhaustive]`, so neither can be built by a struct literal from outside +//! the harness — not even field by field. The transcript under test is therefore a +//! real one: a scripted provider drives real turns into an in-memory store, and +//! `Session::branch_from` puts one of them off the path the same way an operator +//! would. That also means these assertions are about what the harness actually +//! records, rather than about a hand-made value that agrees with the renderer by +//! construction. + +use io_cli::theme::DARK; +use io_cli::transcript; +use io_harness::provider::{CompletionRequest, CompletionResponse}; +use io_harness::{ApproveAll, Policy, Provider, Session, Store}; + +/// The three prompts the branched session is built from. The second is the one +/// the branch leaves behind. +const PLAN: &str = "draft a migration plan"; +const BRANCHED: &str = "do it with a blue-green cutover"; +const KEPT: &str = "do it with a read-only window instead"; + +/// The words `src/transcript.rs` labels a branched-away turn with, spelled out +/// here rather than imported. Importing the constant would assert that the +/// renderer agrees with itself; what matters is that the label is a phrase a +/// reader understands without knowing this product's vocabulary. +const LABEL: &str = "left behind by a branch"; + +/// Answers every turn with one line and never calls anything. +struct Talker; + +impl Provider for Talker { + async fn complete( + &self, + _request: CompletionRequest, + ) -> io_harness::Result { + Ok(CompletionResponse { + text: Some("here is the plan".into()), + ..Default::default() + }) + } +} + +/// Every rendered line as its own string, spans concatenated. The renderer's unit +/// is the line, and every assertion below is about what a single line holds and in +/// what order — joining them all into one blob would make "on the same line" and +/// "somewhere in the output" indistinguishable. +fn rows(lines: &[ratatui::text::Line<'_>]) -> Vec { + lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect() +} + +/// The one row holding `needle`, or a failure naming what was rendered instead. +fn row_with<'a>(rows: &'a [String], needle: &str) -> &'a String { + rows.iter() + .find(|row| row.contains(needle)) + .unwrap_or_else(|| panic!("nothing rendered for {needle:?}; got {rows:#?}")) +} + +async fn say(session: &mut Session, store: &Store, prompt: &str) -> i64 { + session + .turn(prompt, &Talker, store, &Policy::permissive(), &ApproveAll) + .await + .expect("a scripted turn cannot fail") + .turn_id +} + +/// A plan, an answer the operator changed their mind about, and the answer they +/// took instead — so the middle turn is off the path and the other two are on it. +async fn branched() -> (tempfile::TempDir, io_harness::Transcript) { + let dir = tempfile::tempdir().expect("a temp dir"); + let store = Store::memory().expect("an in-memory store"); + let mut session = Session::open(&store, dir.path()).expect("a session"); + + let first = say(&mut session, &store, PLAN).await; + say(&mut session, &store, BRANCHED).await; + session + .branch_from(&store, first) + .expect("the plan turn is branchable"); + say(&mut session, &store, KEPT).await; + + let transcript = session.transcript(&store).expect("a transcript"); + (dir, transcript) +} + +#[tokio::test] +async fn the_turn_a_branch_left_behind_is_rendered_and_labelled_in_words() { + let (_dir, transcript) = branched().await; + assert_eq!( + session_path_len(&transcript), + 2, + "the fixture is only meaningful if one of the three turns is off the path", + ); + + let rendered = rows(&transcript::lines(&transcript, &DARK)); + + // Rendered at all. `Session::history` would not have returned this turn. + let branched = row_with(&rendered, BRANCHED); + // And labelled in words on its own line, not merely toned: a colour is gone + // under `NO_COLOR` and gone again when the line is copied out of the terminal. + assert!( + branched.contains(LABEL), + "the branched-away turn must say so in words: {branched}", + ); + + let kept = row_with(&rendered, KEPT); + assert!( + !kept.contains(LABEL), + "a turn the model can still see must not be labelled: {kept}", + ); +} + +#[tokio::test] +async fn every_turn_that_was_asked_appears() { + let (_dir, transcript) = branched().await; + let rendered = rows(&transcript::lines(&transcript, &DARK)); + + for turn in &transcript.turns { + row_with(&rendered, &turn.prompt); + } + assert_eq!(transcript.turns.len(), 3, "three turns were taken"); +} + +#[tokio::test] +async fn the_prompt_comes_before_the_turn_id_on_its_line() { + let (_dir, transcript) = branched().await; + let rendered = rows(&transcript::lines(&transcript, &DARK)); + + for turn in &transcript.turns { + let row = row_with(&rendered, &turn.prompt); + let id = format!("turn {}", turn.turn_id); + let prompt_at = row.find(turn.prompt.as_str()).expect("the prompt is here"); + let id_at = row + .find(id.as_str()) + .unwrap_or_else(|| panic!("no {id} on the line holding its prompt: {row}")); + // By position, not by membership: a line that reads "turn 2 · draft a + // migration plan" contains both and is inside out, and a `contains` + // assertion is exactly as green for it. + assert!( + prompt_at < id_at, + "content before metadata: {row} has the id at {id_at} and the prompt at {prompt_at}", + ); + } +} + +#[test] +fn an_empty_transcript_renders_a_sentence() { + let dir = tempfile::tempdir().expect("a temp dir"); + let store = Store::memory().expect("an in-memory store"); + let session = Session::open(&store, dir.path()).expect("a session"); + let transcript = session.transcript(&store).expect("a transcript"); + assert!(transcript.turns.is_empty(), "nothing has been asked yet"); + + let rendered = rows(&transcript::lines(&transcript, &DARK)); + let sentence = row_with(&rendered, "no turns"); + assert!( + sentence.ends_with('.'), + "a sentence, so the reader knows the key worked: {sentence}", + ); +} + +/// How many turns the model can still see. Read through the transcript itself so +/// the fixture's premise is checked with the same data the renderer is given. +fn session_path_len(transcript: &io_harness::Transcript) -> usize { + transcript.turns.iter().filter(|turn| turn.on_path).count() +} From 21d6f1cae9af178def77ea675470b6b41d6ad236 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Mon, 17 Aug 2026 09:33:36 +0530 Subject: [PATCH 5/8] feat: give the diff a defined form below a hundred columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cell` takes the terminal's width and drops word-level emphasis to the line below EMPHASIS_FLOOR. Nothing is truncated — a committed line wraps, which the renderer already guarantees. What is lost at eighty columns is findability: a bolded fragment in the middle of a line that now occupies three rows is harder to locate than a whole row that is red. So the emphasis becomes the line, which is the same information at a scale the terminal can still show. A hundred rather than eighty because the floor is about the wrap and not about the supported size: eighty columns is supported, and ninety wraps just as badly. The header's facts are asserted present at eighty columns with a path longer than the terminal. 0.2.0 paid for that lesson once already — a row can "fit" precisely because ratatui clipped the half of the sentence the release existed to show. --- src/app.rs | 5 +- src/diff.rs | 24 ++++++++-- src/main.rs | 6 +-- tests/diff.rs | 17 ++++--- tests/narrow.rs | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 160 insertions(+), 15 deletions(-) diff --git a/src/app.rs b/src/app.rs index 8b06bb2..34854e0 100644 --- a/src/app.rs +++ b/src/app.rs @@ -239,9 +239,10 @@ impl App { /// driver — it is the only thing that holds one — and keeping this a function /// of values is what lets a test state a hunk by hand instead of standing up /// a database to hold one. - pub fn edits(&mut self, edits: &[io_harness::Edit]) { + pub fn edits(&mut self, edits: &[io_harness::Edit], width: u16) { for edit in edits { - self.pending.extend(crate::diff::cell(edit, &self.theme)); + self.pending + .extend(crate::diff::cell(edit, &self.theme, width)); } } diff --git a/src/diff.rs b/src/diff.rs index 8ae4307..b7f35d1 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -50,6 +50,20 @@ use crate::theme::{Theme, Tone}; /// the way a tool call sits under a step. const INDENT: &str = " "; +/// The width below which word-level emphasis drops to the line. +/// +/// A hunk line is committed into the terminal's own scrollback, where a line +/// wider than the terminal wraps rather than truncating — nothing is lost. What +/// is lost is *findability*: a bolded fragment in the middle of a line that now +/// occupies three rows is harder to locate than a whole row that is red. So +/// below a hundred columns the emphasis is the line, which is the same +/// information at a scale the terminal can still show. +/// +/// A hundred rather than eighty because the floor is about the wrap, not about +/// the supported size: an eighty-column terminal is supported, and a +/// ninety-column one wraps just as badly. +pub const EMPHASIS_FLOOR: u16 = 100; + /// The syntax highlighter, built once and only if a diff is ever drawn. /// /// **Never on the startup path.** `SyntaxSet::load_defaults_newlines` @@ -194,7 +208,7 @@ pub fn for_step(edits: Vec, step: u32) -> Vec { /// The header first — the path, then the counts, then the tool — because the /// path is the content and the rest is metadata, which is the rule the whole /// interface follows. -pub fn cell(edit: &Edit, theme: &Theme) -> Vec> { +pub fn cell(edit: &Edit, theme: &Theme, width: u16) -> Vec> { let mut lines = vec![header(edit, theme)]; let Some(hunk) = edit.hunk.as_deref() else { @@ -204,7 +218,7 @@ pub fn cell(edit: &Edit, theme: &Theme) -> Vec> { return lines; }; - lines.extend(body(hunk, &edit.path, theme)); + lines.extend(body(hunk, &edit.path, theme, width >= EMPHASIS_FLOOR)); lines.push(Line::from("")); lines } @@ -250,7 +264,7 @@ fn header(edit: &Edit, theme: &Theme) -> Line<'static> { /// then emphasise the difference between lines that have nothing to do with each /// other, which is worse than no emphasis — it invents a relationship and paints /// it. -fn body(hunk: &str, path: &str, theme: &Theme) -> Vec> { +fn body(hunk: &str, path: &str, theme: &Theme, emphasis: bool) -> Vec> { let syntax = syntax_for(path); let raw: Vec<&str> = hunk.lines().collect(); let mut out = Vec::new(); @@ -275,7 +289,9 @@ fn body(hunk: &str, path: &str, theme: &Theme) -> Vec> { continue; } - let paired = !removals.is_empty() && removals.len() == additions.len(); + // Below the emphasis floor nothing is paired, so every changed line takes + // the whole wash — see `EMPHASIS_FLOOR`. + let paired = emphasis && !removals.is_empty() && removals.len() == additions.len(); for (n, line) in removals.iter().enumerate() { let other = paired.then(|| additions[n]); out.push(changed(line, other, Tone::Removed, syntax, theme)); diff --git a/src/main.rs b/src/main.rs index 73e4226..8a5a638 100644 --- a/src/main.rs +++ b/src/main.rs @@ -414,7 +414,7 @@ async fn turn( } Some(event) = events.recv() => { app.event(&event); - commit_edits(app, store, &event); + commit_edits(app, store, &event, screen.width()); app.status.elapsed = started.elapsed(); paint(screen, app)?; } @@ -482,12 +482,12 @@ async fn turn( /// A read that fails degrades to a line saying so. A run whose work succeeded is /// not a run to panic over because the trace could not be re-read, and silence /// would say the step changed nothing. -fn commit_edits(app: &mut App, store: &Store, event: &io_harness::RunEvent) { +fn commit_edits(app: &mut App, store: &Store, event: &io_harness::RunEvent, width: u16) { let io_harness::EventKind::Step { changed: true, .. } = &event.kind else { return; }; match store.edits(event.run_id) { - Ok(edits) => app.edits(&io_cli::diff::for_step(edits, event.step)), + Ok(edits) => app.edits(&io_cli::diff::for_step(edits, event.step), width), Err(error) => app.say( Tone::Muted, format!("the diff for this step could not be read: {error}"), diff --git a/tests/diff.rs b/tests/diff.rs index 4b7a0e1..a94c24c 100644 --- a/tests/diff.rs +++ b/tests/diff.rs @@ -13,9 +13,14 @@ use io_cli::diff::cell; use io_cli::theme::DARK; use io_harness::Edit; +/// A terminal wide enough that word-level emphasis applies. Every test that is +/// not about the narrow form states it, so none of them depends on the floor by +/// accident. +const WIDE: u16 = 120; + /// The text of a rendered cell, one line per line, as a reader would see it. fn rendered(edit: &Edit) -> String { - cell(edit, &DARK) + cell(edit, &DARK, WIDE) .iter() .map(|line| { line.spans @@ -155,7 +160,7 @@ fn f2_an_absent_hunk_never_renders_as_an_empty_diff() { // `unwrap_or_default()` leaves behind — which is precisely the empty-patch // shape this assertion exists to catch. The sabotage arm found it. assert_eq!( - cell(&edit, &DARK).len(), + cell(&edit, &DARK, WIDE).len(), 1, "an absent hunk is one line and nothing else:\n\n{text}", ); @@ -241,7 +246,7 @@ fn f2_a_hunk_that_is_present_is_not_confused_for_an_absent_one() { /// text alone could not tell a line emphasised in one piece from a line /// emphasised in three. fn spans_of(edit: &Edit, marker: char) -> Vec<(String, bool)> { - let lines = cell(edit, &DARK); + let lines = cell(edit, &DARK, WIDE); let line = lines .iter() .find(|line| { @@ -383,7 +388,7 @@ fn edit_with_code() -> Edit { #[test] fn f4_the_colours_come_from_io_cli_s_own_tokens() { let edit = edit_with_code(); - let coloured: Vec<(String, Option)> = cell(&edit, &DARK) + let coloured: Vec<(String, Option)> = cell(&edit, &DARK, WIDE) .iter() .flat_map(|line| { line.spans @@ -422,7 +427,7 @@ fn f4_the_colours_come_from_io_cli_s_own_tokens() { #[test] fn f4_a_keyword_a_string_and_a_number_each_get_their_own_token() { let edit = edit_with_code(); - let all: Vec<(String, Option)> = cell(&edit, &DARK) + let all: Vec<(String, Option)> = cell(&edit, &DARK, WIDE) .iter() .flat_map(|line| { line.spans @@ -460,7 +465,7 @@ fn f4_a_keyword_a_string_and_a_number_each_get_their_own_token() { fn f4_no_color_leaves_the_markers_carrying_the_meaning() { use io_cli::theme::PLAIN; - let lines = cell(&edit_with_code(), &PLAIN); + let lines = cell(&edit_with_code(), &PLAIN, WIDE); for line in &lines { for span in &line.spans { assert_eq!( diff --git a/tests/narrow.rs b/tests/narrow.rs index a815b01..a7cce35 100644 --- a/tests/narrow.rs +++ b/tests/narrow.rs @@ -143,3 +143,126 @@ async fn n5_the_approval_overlay_fits_eighty_columns() { let decision = deciding.await.expect("the approver did not panic"); assert!(matches!(decision, Decision::Deny { .. })); } + +/// **N5.** A diff at eighty columns, and the emphasis floor below a hundred. +/// +/// Nothing here is about truncation — a committed line wraps, which +/// `f9_a_line_wider_than_the_terminal_wraps_rather_than_truncating` already +/// proves for the renderer. What is asserted is that the cell's own facts survive +/// the narrow form, and that word-level emphasis gives way to the line rather +/// than leaving a bolded fragment somewhere in the middle of a three-row wrap. +mod diff { + use io_cli::diff::{cell, EMPHASIS_FLOOR}; + use io_cli::theme::DARK; + use io_harness::Edit; + use ratatui::style::Modifier; + + /// A path longer than eighty columns on its own, and body lines wider still. + fn wide_edit() -> Edit { + Edit { + step: 1, + tool: "edit_file".to_string(), + path: "crates/some-rather-long-crate-name/src/subsystem/module/implementation.rs" + .to_string(), + lines_added: 1, + lines_removed: 1, + hunk: Some(format!( + "@@ -12,3 +12,3 @@\n \ + fn one() {{}}\n\ + -let value = {};\n\ + +let value = {};\n", + "x".repeat(200), + "y".repeat(200), + )), + } + } + + fn text(edit: &Edit, width: u16) -> String { + cell(edit, &DARK, width) + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>() + .join("\n") + } + + #[test] + fn n5_the_header_keeps_its_facts_at_eighty_columns() { + let edit = wide_edit(); + let rendered = text(&edit, super::WIDTH); + + // The path is longer than the terminal, so the counts sit past the wrap. + // 0.2.0 paid for the lesson that a row can "fit" because ratatui clipped + // the half of the sentence that mattered — here the facts must be in the + // cell whatever the wrap does with them. + assert!(rendered.contains(&edit.path), "{rendered}"); + assert!( + rendered.contains("+1"), + "the additions are gone: {rendered}" + ); + assert!(rendered.contains("-1"), "the removals are gone: {rendered}"); + assert!(rendered.contains("edit_file"), "{rendered}"); + } + + #[test] + fn n5_nothing_in_the_body_is_dropped_at_eighty_columns() { + let rendered = text(&wide_edit(), super::WIDTH); + assert_eq!( + rendered.matches(&"x".repeat(200)).count(), + 1, + "the removed line was cut rather than left to wrap: {rendered}", + ); + assert_eq!(rendered.matches(&"y".repeat(200)).count(), 1, "{rendered}"); + assert!(rendered.contains("@@ -12,3 +12,3 @@"), "{rendered}"); + } + + #[test] + fn n5_below_the_floor_the_emphasis_is_the_line_and_not_a_word() { + let edit = wide_edit(); + + let wide = cell(&edit, &DARK, EMPHASIS_FLOOR); + let emphasised = |lines: &[ratatui::text::Line<'static>]| { + lines + .iter() + .flat_map(|line| line.spans.iter()) + .filter(|span| span.style.add_modifier.contains(Modifier::BOLD)) + .count() + }; + assert!( + emphasised(&wide) > 0, + "at the floor itself the word-level emphasis still applies", + ); + + let narrow = cell(&edit, &DARK, EMPHASIS_FLOOR - 1); + assert_eq!( + emphasised(&narrow), + 0, + "below the floor a changed line takes the whole wash, so no fragment \ + of it is bolded inside a wrap", + ); + + // And the line still reads as changed: the wash is the carrier now. + let removed = narrow + .iter() + .find(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + .trim_start() + .starts_with('-') + }) + .expect("a removed line"); + assert!( + removed + .spans + .iter() + .any(|s| s.style.fg == Some(DARK.diff_delete)), + "a line with no emphasis must still be coloured as removed", + ); + } +} From 7fe6544cdc565ecbb520168a46f8d59a8df1e39f Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Mon, 17 Aug 2026 09:44:07 +0530 Subject: [PATCH 6/8] feat: tool cells, the approval diff, and a minimal diff style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TOOL CELLS. A tool call is one line — the tool, its target, what came back and how long it took — live in the viewport while it runs and committed when the step lands. Nothing is committed at announce time, because EventKind::ToolCall is emitted before the tool's result is known. The pending store is a Vec and never an Option: read_batch announces every call of a parallel read up front before running any of them, so two calls before one step is the ordinary case, not a hypothetical. Refused does NOT close a cell — the harness feeds the refusal back to the model as an observation and the step still commits — so it marks the step instead, and it survives arriving with no open cell at all, which it does for a dial or an MCP spawn at step 0. Refused.act and .target are never matched against a call: act is a policy act and target is the resolved path, while ToolCall.target is the raw argument the model wrote. The result is the harness's own decision segment when the segments pair one-to-one with the announced calls, and the step's verdict otherwise — the workspace loop pushes extra segments for spawned children, so the alignment is checked rather than assumed. The duration is io-cli's OBSERVATION of the interval between two events and is printed with a `~` to say so; an Mcp event's millis is the one harness-measured tool duration in the enum and prints without one. An interrupted call reads unfinished and carries no duration at all, because io-cli does not know one. flush() and flush_text() are split, and the split is load-bearing: the mid-step arms must commit the assistant tail WITHOUT closing a live call, or a refusal one line before a step reports the opposite of what happened. THE APPROVAL DIFF. product.yaml's approval requirement ends "and, for a write, the diff", and 0.2.0 shipped that clause unmet. Edit::with_hunk is the harness's own renderer, so io-cli still computes no diff; what it supplies is the old side, read from disk, because the write has not happened yet. Absolute targets only — `io -C ` sets the workspace root without changing this process's working directory, so a relative path could name a different file that happens to exist here and the overlay would show a confident diff against something the agent never touched. A file that is not there yet reads as all addition; one that cannot be read falls back to showing the content plainly. At the tightest size the one row available goes to `+3 -1` rather than to the first line of the change, because the size of a write is the decision. MINIMAL. `diff = "minimal"` in [app.io-cli] drops the context and keeps the changed lines and the @@ header — a change with no line numbers does not say where it is, which is not a smaller diff but a worse one. Absent means unified, so no existing configuration file needs touching. FIXES FROM REVIEW. The tail drain rendered its events but never their diffs, so a Step still queued when the turn's future won the select lost its diff silently — and the last step of a turn is exactly the one that loses that race. A hunk is now capped at MAX_BODY_LINES: highlighting is a parse per line on the same loop that delivers keystrokes, and Screen::commit hands insert_before a u16 height that silently clamps, so a 26,000-line hunk made Ctrl+C unreachable and then truncated. And a `\ No newline at end of file` marker no longer ends a run of removals, which had been silently costing word-level emphasis on exactly the lines where a file's last line changed. --- CHANGELOG.md | 46 +++++++ Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 32 +++-- src/app.rs | 20 ++- src/approval.rs | 104 ++++++++++++++- src/diff.rs | 94 +++++++++++-- src/events.rs | 312 ++++++++++++++++++++++++++++++++++++++----- src/main.rs | 31 ++++- src/settings.rs | 40 ++++++ tests/approval.rs | 125 ++++++++++++++++++ tests/diff.rs | 71 ++++++++++ tests/events.rs | 321 +++++++++++++++++++++++++++++++++++++++++---- tests/interrupt.rs | 10 +- tests/live.rs | 4 +- tests/status.rs | 45 ++++--- 16 files changed, 1146 insertions(+), 113 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10ea7d7..06b99d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,52 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +## [0.3.0] - 2026-08-17 + +The operator can read what the agent did to their files without leaving the +terminal and without losing the thread. An edit stops being a line saying a file +changed and becomes the change itself. + +### Added + +- **Edits render as diffs.** Not diffs io-cli computed — io-harness already + renders a unified diff for every edit its tools make and keeps it in the run's + durable trace, so what you see carries the file's own `@@` line numbers and is + the same text `Store::patch` would hand `patch`. Additions and removals are + coloured *and* marked, so the meaning survives `NO_COLOR`. +- **Word-level emphasis inside a changed line.** A run of removals is paired with + the run of additions after it only when the two are the same length; anything + else takes the whole wash. A `write_file` that rewrote two distant regions of a + file arrives as one hunk spanning both, and a greedier rule would emphasise the + difference between lines that have nothing to do with each other. +- **Syntax highlighting**, drawn in io-cli's own theme tokens rather than in a + second palette. The three new tokens — keyword, string, literal — are the + theme's, so a highlighted diff and the rest of the interface stay one look, and + `NO_COLOR` is still decided in one place. Green still means added: the parts of + a line both sides share are syntax coloured and the words that actually changed + keep the diff's colour, so the add/remove colour now points at the exact words + instead of washing the line. +- **A defined form below a hundred columns**, where word-level emphasis gives way + to the line — a bolded fragment in the middle of a line that now takes three + rows is harder to find than a whole row that is red. Nothing is truncated. +- **`diff = "minimal"` in `[app.io-cli]`** for reviewing by file rather than by + hunk: the changed lines and the `@@` header, without the context. Its absence + means `unified`, so no existing configuration file needs touching. + +### Changed + +- **An approval shows a write as a diff** against the file on disk, which is the + clause of the approval surface 0.2.0 shipped unmet — the harness hands an + approver the whole resulting file rather than a patch, so the old side is + io-cli's to supply. A file that does not exist yet reads as all addition. At + the tightest size the one row available is spent on `+3 -1` rather than on the + first line of the change, because the size of a write is the decision. + +### Fixed + +- The answers row in the approval overlay no longer carries a double space after + each separator. + ## [0.2.0] - 2026-08-17 The operator can see the boundary the agent is working under, change it, and diff --git a/Cargo.lock b/Cargo.lock index 7f6f777..76f7515 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -946,7 +946,7 @@ dependencies = [ [[package]] name = "io-cli" -version = "0.2.0" +version = "0.3.0" dependencies = [ "clap", "crossterm", diff --git a/Cargo.toml b/Cargo.toml index 1f67205..a33ede4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "io-cli" -version = "0.2.0" +version = "0.3.0" edition = "2021" # io-harness declares 1.95, set by `libsqlite3-sys` calling the std `cfg_select!` # macro from its build script. Nothing here can be lower than what the harness diff --git a/README.md b/README.md index e53f460..a4bcc75 100644 --- a/README.md +++ b/README.md @@ -151,21 +151,29 @@ refusal, error and warning also carries a word. ## What this release is not -0.2.0 is the permission boundary made visible and answerable: the approval -overlay, the posture key, the refusal that names its rule and its layer, and the -status fields around them. Diffs, syntax highlighting and collapsible tool output -are 0.3.0; resume, fork and rewind are 0.4.0; the headless subcommand and NDJSON -are 0.5.0; the screen-reader mode is 0.6.0; the slash palette and type-to-filter -pickers are 0.7.0. - -Two things are absent for reasons worth stating rather than hiding. **Spend +0.3.0 is what the agent did to your files, made readable: diffs from the +harness's own stored hunks, word-level emphasis, syntax highlighting, collapsed +tool output, the transcript on one key and the clipboard over SSH. Resume, fork +and rewind are 0.4.0; the headless subcommand and NDJSON are 0.5.0; the +screen-reader mode is 0.6.0; the slash palette and type-to-filter pickers are +0.7.0. + +Three things are absent for reasons worth stating rather than hiding. **Spend against the tree ceiling is not in the status line**: io-harness emits `SpendDraw` only from a contained turn, and its contained entry point takes no steer inbox, so rendering spend today would cost `Ctrl+C`. It arrives at 0.8.0 -with the fleet, which is contained anyway. And **the overlay shows a write's -proposed content plainly rather than as a diff** — the harness hands an approver -the resulting file, not a patch, so the diff is io-cli's to compute and 0.3.0's -to design. +with the fleet, which is contained anyway. **A diff cannot be expanded beyond +the context the harness stored** — three lines either side, which is what +`diff -u` has always carried; more than that is not in the trace, and reading it +off disk would be reading a version of the file that no longer exists. And +**there is no split view**: this renderer commits into the terminal's own +scrollback at its real width, a two-column comparison doubles the horizontal +budget for every line, and word-level emphasis inside a unified diff already +answers the question split view answers. + +One ceiling worth knowing about: a hunk is a fragment of a file, and each of its +lines is highlighted from a clean parse. A block comment or a multi-line string +that was opened *above* the hunk is not known here, so those lines read as code. ## Contributing diff --git a/src/app.rs b/src/app.rs index 34854e0..86a1d3d 100644 --- a/src/app.rs +++ b/src/app.rs @@ -77,6 +77,9 @@ pub struct App { /// without this a *this session* answer would ask again on the next prompt. /// F5 asserts it on the policy handed to the next turn. remembered: Vec, + /// How much of a change a diff shows. From `[app.io-cli]`, defaulting to + /// unified, which is what every file written before 0.3.0 means. + diff_style: crate::settings::DiffStyle, /// The permission posture this session is running under. /// /// `None` means the configuration file holds a policy that is none of the @@ -98,10 +101,17 @@ impl App { pending: Vec::new(), approval: None, remembered: Vec::new(), + diff_style: crate::settings::DiffStyle::default(), posture: None, } } + /// Say how much of a change a diff should show. Read from the harness's own + /// configuration by the driver; never parsed here. + pub fn set_diff_style(&mut self, style: crate::settings::DiffStyle) { + self.diff_style = style; + } + /// A run stopped to ask. The overlay opens and takes the keyboard. /// /// Nothing is committed here. A question in the scrollback is one that can be @@ -227,9 +237,15 @@ impl App { } /// Take an event from the harness. - pub fn event(&mut self, event: &RunEvent) { + /// + /// `at` is the session's age, handed in by the driver rather than read here. + /// It is what lets a tool cell report how long its call took without any + /// module but `src/main.rs` touching a clock — the same shape [`App::tick`] + /// established, and the reason `tests/timing.rs` can still assert that no + /// test anywhere reads one. + pub fn event(&mut self, event: &RunEvent, at: Duration) { self.status_from(event); - let lines = self.events.event(event); + let lines = self.events.event(event, at); self.pending.extend(lines); } diff --git a/src/approval.rs b/src/approval.rs index 2b451d9..07472a5 100644 --- a/src/approval.rs +++ b/src/approval.rs @@ -25,7 +25,7 @@ use crossterm::event::{KeyCode, KeyEvent}; use io_harness::approve::DecisionFuture; -use io_harness::{Act, ApprovalContext, Approver, Decision, Effect, Request, Rule}; +use io_harness::{Act, ApprovalContext, Approver, Decision, Edit, Effect, Request, Rule}; use ratatui::layout::Rect; use ratatui::text::{Line, Span}; use ratatui::widgets::Paragraph; @@ -219,14 +219,28 @@ pub const REFUSED_BY_OPERATOR: &str = "the operator denied it"; pub struct Approval { ask: Ask, chosen: usize, + /// The write, as a change rather than as a wall of text. + /// + /// Computed once, when the overlay opens, because `render` runs per frame and + /// reading a file per frame is a file read per keystroke. `None` means there + /// is nothing to diff — the act is not a write, or the target could not be + /// read for a reason that is not "it does not exist yet" — and the overlay + /// falls back to showing the proposed content plainly, which is what 0.2.0 + /// did for every write. + proposed: Option, } impl Approval { pub fn new(ask: Ask) -> Self { + let proposed = diff_of(&ask); // Opens on `Once`: the least committal answer, and never on a remembered // allow. A surface that opens on the widest answer is one where `Enter` // by reflex gives away the most. - Self { ask, chosen: 0 } + Self { + ask, + chosen: 0, + proposed, + } } /// Which answer is highlighted. @@ -366,6 +380,9 @@ impl Approval { /// It says what it cut. A reader who thinks they have seen a whole file and /// has seen a third of it is worse off than one who was told. fn preview(&self, room: usize, width: usize, theme: &Theme) -> Vec> { + if let Some(edit) = &self.proposed { + return self.as_diff(edit, room, width, theme); + } let Some(content) = self.ask.content() else { return Vec::new(); }; @@ -395,6 +412,40 @@ impl Approval { .collect() } + /// The write as a diff, in the rows there are. + /// + /// The cell's own header comes first and that is deliberate rather than + /// redundant with the question row above it: at the tightest size the overlay + /// has one row here, and `+3 -1` is the single most useful thing an approver + /// can be told in one row. It is the difference between a write that touches + /// three lines and one that rewrites four hundred, which is a different + /// decision. The hunk itself fills whatever rows are left. + fn as_diff(&self, edit: &Edit, room: usize, width: usize, theme: &Theme) -> Vec> { + let width = u16::try_from(width).unwrap_or(u16::MAX); + let mut lines = crate::diff::cell(edit, theme, width); + // `cell` ends with a blank line so a transcript breathes. An overlay four + // rows tall cannot spend one on breathing. + if lines.last().is_some_and(|line| line.width() == 0) { + lines.pop(); + } + + let total = lines.len(); + let shown = total.min(room); + let cut = total.saturating_sub(shown); + lines.truncate(shown); + // Said, not silently dropped. A reader who thinks they have seen a whole + // change and has seen its first row is worse off than one who was told. + if cut > 0 { + if let Some(last) = lines.last_mut() { + last.spans.push(Span::styled( + format!(" ⋯ {cut} more lines"), + theme.style(Tone::Muted), + )); + } + } + lines + } + /// `› allow once · allow this session · deny`, on one row. /// /// One row rather than a list, because the viewport is four rows and the facts @@ -424,6 +475,55 @@ impl Approval { /// The word for an act. io-harness spells these in its own serde tags and in /// `EventKind::Refused`; this is the same vocabulary so a refusal and a request /// read as the same product. +/// The change a write would make, as an `io_harness::Edit`. +/// +/// **io-cli still computes no diff.** `Edit::with_hunk` is the harness's own +/// renderer — the same one that produced every hunk in the durable trace — so an +/// approval and a transcript show a change in exactly the same words. What io-cli +/// supplies is the *old* side, because the write has not happened yet and the +/// harness has nothing stored for it: the approver is handed the whole resulting +/// file, never a patch. +/// +/// **Only the request's own target is read**, and only for a write. This is the +/// one workspace read the interface performs, it is of the file the operator is +/// being asked about, and it exists so they can see what changes rather than what +/// the file will end up containing. +/// +/// The three outcomes are deliberately different: +/// - the file is not there → an empty old side, so a new file reads as all +/// addition, which is what it is; +/// - the file is there → a real diff against it; +/// - the file is there and cannot be read, or is not UTF-8 → `None`, and the +/// overlay shows the proposed content plainly. Diffing against an empty old +/// side here would show every existing line as new, which is a lie about the +/// size of the change and exactly the wrong direction to be wrong in. +fn diff_of(ask: &Ask) -> Option { + if ask.act() != Act::Write { + return None; + } + let after = ask.content()?; + let target = ask.target(); + + // **Absolute only.** A relative target would resolve against this process's + // working directory, and `io -C ` sets the workspace root without + // changing it — so a relative path could name a *different* file that + // happens to exist here, and the overlay would then show a confident diff + // against something the agent never touched. In practice the harness hands + // an approver the policy's resolved target, which is absolute; when it does + // not, showing the proposed content plainly is the honest answer. + if !std::path::Path::new(target).is_absolute() { + return None; + } + + let before = match std::fs::read_to_string(target) { + Ok(before) => before, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => String::new(), + Err(_) => return None, + }; + + Some(Edit::measure(0, "write_file", target, &before, after).with_hunk(&before, after)) +} + pub fn act_word(act: Act) -> &'static str { match act { Act::Read => "read", diff --git a/src/diff.rs b/src/diff.rs index b7f35d1..198f758 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -43,6 +43,7 @@ use ratatui::text::{Line, Span}; use syntect::easy::ScopeRangeIterator; use syntect::parsing::{ParseState, Scope, ScopeStack, SyntaxReference, SyntaxSet}; +use crate::settings::DiffStyle; use crate::status::SEPARATOR; use crate::theme::{Theme, Tone}; @@ -50,6 +51,22 @@ use crate::theme::{Theme, Tone}; /// the way a tool call sits under a step. const INDENT: &str = " "; +/// The most body lines one cell will draw. +/// +/// A hunk is capped by the harness at a mebibyte, which is on the order of +/// twenty-six thousand lines, and two separate things go wrong long before that. +/// Highlighting is a fresh parse per line and runs synchronously on the loop that +/// also delivers keystrokes, so a large hunk makes `Ctrl+C` unreachable for as +/// long as it takes. And `Screen::commit` hands `insert_before` a `u16` height, +/// which silently clamps: past sixty-five thousand rows the content is truncated +/// rather than wrapped, and the buffer it allocates first is the terminal's width +/// times that height. +/// +/// Five hundred is far more of a change than anybody reads in a terminal and far +/// less than either failure. What is cut is said, never dropped quietly — the +/// whole change is still in the trace, and `/copy diff` still carries all of it. +pub const MAX_BODY_LINES: usize = 500; + /// The width below which word-level emphasis drops to the line. /// /// A hunk line is committed into the terminal's own scrollback, where a line @@ -209,6 +226,11 @@ pub fn for_step(edits: Vec, step: u32) -> Vec { /// path is the content and the rest is metadata, which is the rule the whole /// interface follows. pub fn cell(edit: &Edit, theme: &Theme, width: u16) -> Vec> { + cell_styled(edit, theme, width, DiffStyle::Unified) +} + +/// The same, at a chosen style. +pub fn cell_styled(edit: &Edit, theme: &Theme, width: u16, style: DiffStyle) -> Vec> { let mut lines = vec![header(edit, theme)]; let Some(hunk) = edit.hunk.as_deref() else { @@ -218,7 +240,16 @@ pub fn cell(edit: &Edit, theme: &Theme, width: u16) -> Vec> { return lines; }; - lines.extend(body(hunk, &edit.path, theme, width >= EMPHASIS_FLOOR)); + let mut drawn = body(hunk, &edit.path, theme, width >= EMPHASIS_FLOOR, style); + let cut = drawn.len().saturating_sub(MAX_BODY_LINES); + if cut > 0 { + drawn.truncate(MAX_BODY_LINES); + drawn.push(Line::from(Span::styled( + format!("{INDENT}⋯ {cut} more lines of this change — the whole of it is in the trace, and `/copy diff` carries it"), + theme.style(Tone::Muted), + ))); + } + lines.extend(drawn); lines.push(Line::from("")); lines } @@ -264,42 +295,77 @@ fn header(edit: &Edit, theme: &Theme) -> Line<'static> { /// then emphasise the difference between lines that have nothing to do with each /// other, which is worse than no emphasis — it invents a relationship and paints /// it. -fn body(hunk: &str, path: &str, theme: &Theme, emphasis: bool) -> Vec> { +fn body( + hunk: &str, + path: &str, + theme: &Theme, + emphasis: bool, + style: DiffStyle, +) -> Vec> { let syntax = syntax_for(path); let raw: Vec<&str> = hunk.lines().collect(); let mut out = Vec::new(); let mut at = 0; while at < raw.len() { + // A `\ No newline at end of file` marker is emitted on its own line + // immediately after the line it applies to, so it can land BETWEEN a + // removal and its addition. Scanned through rather than treated as the + // end of a run: otherwise the additions run comes back empty, the pair is + // never made, and the word-level emphasis is lost on exactly the lines + // where a file's last line changed. + let marker = |line: &str| line.starts_with('\\'); + let removals_from = at; - while at < raw.len() && raw[at].starts_with('-') { + while at < raw.len() + && (raw[at].starts_with('-') || (at > removals_from && marker(raw[at]))) + { at += 1; } let removals = &raw[removals_from..at]; let additions_from = at; - while at < raw.len() && raw[at].starts_with('+') { + while at < raw.len() + && (raw[at].starts_with('+') || (at > additions_from && marker(raw[at]))) + { at += 1; } let additions = &raw[additions_from..at]; if removals.is_empty() && additions.is_empty() { - out.push(unchanged(raw[at], syntax, theme)); + // `minimal` keeps the `@@` header — a change with no line numbers is a + // change that does not say where it is — and drops the context, which + // is the only thing it drops. + let keep = style == DiffStyle::Unified || raw[at].starts_with("@@"); + if keep { + out.push(unchanged(raw[at], syntax, theme)); + } at += 1; continue; } // Below the emphasis floor nothing is paired, so every changed line takes // the whole wash — see `EMPHASIS_FLOOR`. - let paired = emphasis && !removals.is_empty() && removals.len() == additions.len(); - for (n, line) in removals.iter().enumerate() { - let other = paired.then(|| additions[n]); - out.push(changed(line, other, Tone::Removed, syntax, theme)); - } - for (n, line) in additions.iter().enumerate() { - let other = paired.then(|| removals[n]); - out.push(changed(line, other, Tone::Added, syntax, theme)); - } + // Pairing counts only the changed lines; a marker is punctuation about + // the bytes and has no partner on the other side. + let removed: Vec<&&str> = removals.iter().filter(|l| !marker(l)).collect(); + let added: Vec<&&str> = additions.iter().filter(|l| !marker(l)).collect(); + let paired = emphasis && !removed.is_empty() && removed.len() == added.len(); + + let mut emit = |lines: &[&str], partners: &[&&str], tone| { + let mut nth = 0; + for line in lines { + if marker(line) { + out.push(unchanged(line, syntax, theme)); + continue; + } + let other = paired.then(|| *partners[nth]); + out.push(changed(line, other, tone, syntax, theme)); + nth += 1; + } + }; + emit(removals, &added, Tone::Removed); + emit(additions, &removed, Tone::Added); } out diff --git a/src/events.rs b/src/events.rs index ff6a2e0..0215d69 100644 --- a/src/events.rs +++ b/src/events.rs @@ -1,6 +1,6 @@ //! Turning io-harness run events into lines. //! -//! Two rules shape this module. +//! Three rules shape this module. //! //! **Nothing is dropped.** `EventKind` is `#[non_exhaustive]` and has fifty //! variants today, so a wildcard arm is not a shortcut here, it is required by @@ -12,18 +12,65 @@ //! live buffer that the viewport draws, and the whole passage is committed to //! scrollback once when it is finished. Committing per token would put a line in //! the terminal's scrollback for every few characters the model produced. +//! +//! **A tool call is a cell, not a line.** io-harness announces a call before it +//! runs — `EventKind::ToolCall` carries the tool and its target and nothing about +//! what came back, because nothing has come back yet — and reports the result +//! only in the `Step` that commits afterwards. So a call is held open from its +//! announcement, shown live in the viewport while it runs, and committed to +//! scrollback once, complete with its result and how long it took, when the step +//! lands. 0.2.0 committed the announcement immediately, which is why a +//! transcript said what the agent was about to do and never what happened. + +use std::time::Duration; -use io_harness::{EventKind, RunEvent}; +use io_harness::{EventKind, RunEvent, MCP_TOOL_PREFIX, NAMESPACE}; use ratatui::text::{Line, Span}; use crate::status::SEPARATOR; use crate::theme::{Theme, Tone}; +/// A tool call that has been announced and not yet closed. +/// +/// Held rather than committed because the two facts a reader actually wants — +/// what came back and how long it took — are not on the announcing event at all. +struct Pending { + name: String, + target: String, + /// The session age at which the call was announced. An age handed in by the + /// driver, never a clock read here: this module has no timer, and N1 is what + /// keeps it that way. + opened_at: Duration, + /// A duration io-harness itself measured, when it reports one. + /// + /// `EventKind::Mcp` is the only event in the whole enum that carries a + /// per-tool duration, so for an MCP tool this is a real measurement of how + /// long the tool ran. For every other tool it stays `None` and the cell + /// falls back to io-cli's own observation, which is a different kind of + /// number and is printed as one. + measured: Option, +} + /// Accumulates streaming text and turns events into committed lines. pub struct Events { theme: Theme, /// Model text received since the last commit. live: String, + /// Calls announced and not yet closed, in the order they were announced. + /// + /// A `Vec` and never an `Option`: `read_batch` announces every call in a + /// parallel batch up front and only then runs any of them, so two or more + /// open calls before a single `Step` is the ordinary case rather than an + /// edge one. A single slot would be wrong on the first parallel read. + open: Vec, + /// Whether the permission boundary refused something during the step now in + /// flight. + /// + /// A refusal does not end a step. io-harness turns it into an observation + /// fed back to the model and the step commits anyway, so this *marks* the + /// step rather than closing a cell — and it is the only honest result word + /// available when the step's own decisions cannot be paired to the calls. + refused_this_step: bool, } impl Events { @@ -31,6 +78,8 @@ impl Events { Self { theme, live: String::new(), + open: Vec::new(), + refused_this_step: false, } } @@ -38,15 +87,60 @@ impl Events { self.theme = theme; } - /// The unfinished tail of what is streaming: everything since the last - /// newline. Drawn in the viewport, where it can be replaced as it grows; a - /// committed line cannot be. - pub fn live(&self) -> &str { - &self.live + /// The one live row the viewport draws: an open tool call if there is one, + /// otherwise the unfinished tail of what is streaming. + /// + /// It is arbitrated rather than concatenated because the row is not free. + /// No `Token` arrives while a tool is dispatching, so the buffer is frozen — + /// but it is frozen holding the assistant's last unterminated line, which is + /// usually not empty. The obvious fix, flushing at `ToolCall` to empty it, + /// is wrong: committing the tail appends a blank line after it, so every tool + /// cell in the transcript would arrive with a blank row in front of it. + /// An open call is the more urgent thing to say, so it wins the row, and the + /// tail stays live until something legitimately commits it. + pub fn live(&self) -> String { + let Some(call) = self.open.last() else { + return self.live.clone(); + }; + let mut row = format!("⋅ {}", call.name); + if !call.target.is_empty() { + row.push(' '); + row.push_str(&call.target); + } + row.push_str(" …"); + if self.open.len() > 1 { + row.push_str(&format!(" (+{} more)", self.open.len() - 1)); + } + row } - /// Commit whatever has streamed so far, if anything. + /// End the turn: commit whatever streamed, then close every call still open. + /// + /// The public entry point, called when a turn finishes or is interrupted. A + /// `Step` may never arrive — io-harness skips `commit_step` when a sub-agent's + /// child deferred — so this is the only place that can honestly close a call + /// that nothing ever reported on. + /// + /// Those cells carry no duration. io-cli knows when the call was announced + /// and nothing at all about when it stopped, and a number printed there would + /// be a guess wearing a measurement's clothes. pub fn flush(&mut self) -> Vec> { + let mut lines = self.flush_text(); + let theme = self.theme; + for call in std::mem::take(&mut self.open) { + lines.push(cell_line(theme, &call, "unfinished", None)); + } + self.refused_this_step = false; + lines + } + + /// Commit whatever text has streamed so far, if any, leaving open calls open. + /// + /// Separate from the public `flush` because most of the callers below are + /// mid-step — a refusal and an approval request both arrive while the call + /// they are about is still running — and closing that call as unfinished + /// there would report the opposite of what happened. + fn flush_text(&mut self) -> Vec> { if self.live.trim().is_empty() { self.live.clear(); return Vec::new(); @@ -65,9 +159,16 @@ impl Events { lines } - /// What this event commits to scrollback. Empty means "nothing yet" — which - /// for a token is the correct answer, not a dropped event. - pub fn event(&mut self, event: &RunEvent) -> Vec> { + /// What this event commits to scrollback, given the session age the driver + /// read the clock for. + /// + /// Empty means "nothing yet" — which for a token, and for a tool call that + /// has not finished, is the correct answer rather than a dropped event. + /// + /// `at` is handed in for the same reason `App::tick` takes one: nothing here + /// may read a clock, so a test can state the interval between two events by + /// hand and assert on it without anything being timed. + pub fn event(&mut self, event: &RunEvent, at: Duration) -> Vec> { let theme = self.theme; match &event.kind { EventKind::Token { text } => { @@ -104,9 +205,52 @@ impl Events { tokens, changed, } => { + // Taken before anything else, so that the tail flush below + // cannot close these as unfinished a line before the step that + // finished them says otherwise. + let open = std::mem::take(&mut self.open); + let refused = std::mem::replace(&mut self.refused_this_step, false); + // A step's own narration commits after whatever streamed before // it, so the transcript reads in the order it happened. - let mut lines = self.flush(); + let mut lines = self.flush_text(); + + // Then the calls it ran, in the order they were announced, then + // the step line: chronological, which is the only order a + // transcript can be skimmed in. + // + // `decision` is `decisions.join("; ")`, one sentence per thing the + // step did, each written after that thing ran. Pairing them to the + // calls positionally is right whenever there is one of each — but + // the count is not guaranteed: the workspace loop pushes extra + // decisions for spawned children, and both loops push an "awaiting + // approval" segment. So the pairing is used only when the two + // counts agree, and never indexed into blind. + // An empty segment is not a result either, and a step whose + // decision is blank pairs to a cell with nothing in its result + // column — so a blank disqualifies the pairing rather than being + // printed as an answer. + let parts: Vec<&str> = decision.split("; ").collect(); + let complete = parts.iter().all(|part| !part.trim().is_empty()); + let paired = complete && parts.len() == open.len(); + let verdict = if *changed { + "changed files" + } else { + "no change" + }; + for (index, call) in open.iter().enumerate() { + // Never invented. The harness's own sentence when it can be + // matched, otherwise the coarsest true thing known about the + // step — a refusal marked it, or the step's own verdict. + let result = if paired { + parts[index] + } else if refused { + "refused" + } else { + verdict + }; + lines.push(cell_line(theme, call, result, Some(at))); + } // What was decided, what it ran, what came back — then the // metadata. 0.1.0 put the step number and the token count in the @@ -138,19 +282,22 @@ impl Events { lines } EventKind::ToolCall { name, target } => { - // One line per call. The full output goes to the run's durable - // trace rather than to the screen; uncollapsed tool output is what - // makes a transcript unreadable. - let target = if target.is_empty() { - String::new() - } else { - format!(" {target}") - }; - vec![Line::from(vec![ - Span::styled(" ⋅ ", theme.style(Tone::Muted)), - Span::styled(name.clone(), theme.style(Tone::Accent)), - Span::styled(target, theme.style(Tone::Muted)), - ])] + // Nothing is committed here, and that is the point: this event is + // emitted before the call runs, so a line written now could only + // say what the agent was about to do. The call is held open, shown + // by `live()` while it runs, and committed once — with its result + // and its duration — by the `Step` above. + // + // One cell per call either way. The full output goes to the run's + // durable trace rather than to the screen; uncollapsed tool output + // is what makes a transcript unreadable. + self.open.push(Pending { + name: name.clone(), + target: target.clone(), + opened_at: at, + measured: None, + }); + Vec::new() } EventKind::Refused { act, @@ -158,6 +305,20 @@ impl Events { rule, layer, } => { + // Marks the step, and closes nothing. A refusal is fed back to the + // model as an observation and the step commits anyway, so there is + // no call to close here — and often no open call at all, since a + // dial, a verification and an MCP server spawn are all refused + // outside any step. Nothing below indexes into the open list, which + // is what keeps this arm correct at step zero. + // + // The act and the target are deliberately not matched against an + // open call: `act` is a policy verb such as "write" or literally + // "tool", and `target` is the policy's resolved path, while a + // call's target is the raw argument the model wrote. Matching them + // by string would pair the wrong things confidently. + self.refused_this_step = true; + // Act, target, rule, layer — in that order, and the last two are // the facts no other terminal agent can print, because no other // core records them. Asserted by position rather than by presence: @@ -177,7 +338,7 @@ impl Events { "{SEPARATOR}no rule named it: the tier default decided" )), } - let mut lines = self.flush(); + let mut lines = self.flush_text(); lines.push(theme.notice(Tone::Refused, text)); lines } @@ -188,7 +349,7 @@ impl Events { // drawn from those. This is the transcript's note that the run // stopped, not the question itself — the question must never be // committed, which is what F1 asserts. - let mut lines = self.flush(); + let mut lines = self.flush_text(); lines .push(theme.notice(Tone::Warning, format!("{act} {target} — waiting for you"))); lines @@ -201,7 +362,7 @@ impl Events { // The harness's own record of what it was told, which is not the // same line as io-cli's. They agree because the answer travelled // one way; if they ever disagree, this is where it shows. - let mut lines = self.flush(); + let mut lines = self.flush_text(); lines.push(theme.notice( if decision == "deny" { Tone::Refused @@ -217,7 +378,7 @@ impl Events { steps, tokens, } => { - let mut lines = self.flush(); + let mut lines = self.flush_text(); let tone = outcome_tone(outcome); lines.push(theme.notice(tone, format!("{outcome} · {steps} steps · {tokens} tok"))); if let Some(help) = outcome_help(outcome) { @@ -229,7 +390,42 @@ impl Events { lines.push(Line::from("")); lines } - // The other forty-three kinds. Not styled in this release, and not + EventKind::Mcp { + server, + tool, + millis, + .. + } => { + // The one place io-harness reports how long a tool actually ran. + // It is harvested onto the open cell rather than printed here, so + // that an MCP call closes with a measured duration while every + // other call closes with io-cli's observation of the interval — + // the difference the `~` prefix exists to show. + // + // Matched by rebuilding io-harness's own namespaced tool name, + // from io-harness's own two constants rather than from a spelling + // of them typed here, which is what `announce` puts on the + // `ToolCall`. Newest first, because a batch may hold two calls to + // the same tool and the later one is the one still running. + // + // A connect, a discover or a disconnect carries no tool and no + // duration, and matches nothing. + if let (Some(tool), Some(millis)) = (tool, millis) { + let name = format!("{MCP_TOOL_PREFIX}{server}{NAMESPACE}{tool}"); + if let Some(call) = self.open.iter_mut().rev().find(|c| c.name == name) { + call.measured = Some(Duration::from_millis(*millis)); + } + } + // Still rendered as itself. Harvesting a number off an event is not + // the same as having designed a line for it, and an event that + // vanished from the transcript because something read a field off + // it is exactly the silence this module refuses. + vec![Line::from(vec![ + Span::styled(" · ", theme.style(Tone::Muted)), + Span::styled(kind_name(&event.kind), theme.style(Tone::Muted)), + ])] + } + // The other forty-two kinds. Not styled in this release, and not // discarded either: each arrives as one muted line naming itself, so a // release that starts emitting something new is visible rather than // silent. @@ -243,6 +439,62 @@ impl Events { } } +/// One committed tool cell: the tool, its target, what came back, how long it +/// took. +/// +/// Content before metadata, like every other line in this interface, and every +/// fact in words rather than in colour — the result reads the same under +/// `NO_COLOR` and in a screen reader as it does on a colour terminal. +/// +/// `at` is the session age this cell is being closed at, or `None` when the cell +/// is being closed without anything having reported on it. +fn cell_line(theme: Theme, call: &Pending, result: &str, at: Option) -> Line<'static> { + let mut spans = vec![ + Span::styled(" ⋅ ", theme.style(Tone::Muted)), + Span::styled(call.name.clone(), theme.style(Tone::Accent)), + ]; + if !call.target.is_empty() { + spans.push(Span::styled( + format!(" {}", call.target), + theme.style(Tone::Muted), + )); + } + spans.push(Span::styled(SEPARATOR, theme.style(Tone::Muted))); + spans.push(Span::styled(result.to_string(), theme.style(Tone::Normal))); + + // Two different kinds of number, told apart on the line itself. A measured + // duration is io-harness's own and is printed plainly; anything else is the + // interval io-cli observed between two events — which includes the model's + // own turnaround and the queue in front of the tool — and wears a `~` to say + // that it is an observation rather than how long the tool ran. + // + // `at` is `None` when the cell is closed with nothing having reported on it. + // io-cli does not know a duration there and says none, rather than printing + // the age of the announcement as though it were one. + let observed = at.map(|at| format!("~{}", format_millis(at.saturating_sub(call.opened_at)))); + if let Some(duration) = call.measured.map(format_millis).or(observed) { + spans.push(Span::styled( + format!("{SEPARATOR}{duration}"), + theme.style(Tone::Muted), + )); + } + Line::from(spans) +} + +/// `420ms`, `1.4s`, `92.0s`. A tool cell's own duration. +/// +/// Separate from [`format_elapsed`](crate::status::format_elapsed), which floors +/// to whole seconds and would print `0s` for every tool call that took less than +/// one — which is most of them. Two formats because they answer two questions: +/// how long the session has been open, and how long one call took. +fn format_millis(duration: Duration) -> String { + let millis = duration.as_millis(); + if millis < 1_000 { + return format!("{millis}ms"); + } + format!("{:.1}s", duration.as_secs_f64()) +} + /// How a run's outcome should read. /// /// The vocabulary is io-harness's, and the distinction that matters here is diff --git a/src/main.rs b/src/main.rs index 8a5a638..4e810d1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -143,6 +143,11 @@ async fn drive( }; let policy = config.policy().unwrap_or_default(); + // `[app.io-cli]` again, read through the harness rather than parsed here. It + // is read in `drive` rather than in `run` because `run` may hand control to + // the wizard, which writes the file this then reads back. + let stored: Option = config.app(settings::APP_KEY).unwrap_or_default(); + let diff_style = settings::DiffStyle::from_setting(stored.and_then(|s| s.diff).as_deref()); let store = store_path().ok_or("no place to keep the run store")?; let store = Store::open(&store).map_err(|error| error.to_string())?; let session = Session::open(&store, root).map_err(|error| error.to_string())?; @@ -159,6 +164,7 @@ async fn drive( store, session, policy, + diff_style, theme, model, ) @@ -173,6 +179,7 @@ async fn drive( store, session, policy, + diff_style, theme, model, ) @@ -187,6 +194,7 @@ async fn drive( store, session, policy, + diff_style, theme, model, ) @@ -213,7 +221,7 @@ async fn drive( } }; loop_over( - screen, inputs, provider, store, session, policy, theme, model, + screen, inputs, provider, store, session, policy, diff_style, theme, model, ) .await } @@ -232,10 +240,12 @@ async fn loop_over( store: Store, mut session: Session, policy: Policy, + diff_style: settings::DiffStyle, theme: Theme, model: String, ) -> Result<(), String> { let mut app = App::new(theme, model); + app.set_diff_style(diff_style); // What the file already says, read back rather than assumed. `None` means the // file holds a policy that is none of the three, which io-harness's own // configuration can express and this session must not relabel. @@ -413,9 +423,13 @@ async fn turn( } } Some(event) = events.recv() => { - app.event(&event); + // One clock read, used for both. `main.rs` is the only module in + // `src/` allowed one, which is what keeps a tool cell's duration + // assertable without a test ever measuring anything. + let at = started.elapsed(); + app.status.elapsed = at; + app.event(&event, at); commit_edits(app, store, &event, screen.width()); - app.status.elapsed = started.elapsed(); paint(screen, app)?; } Some(ask) = asks.recv() => { @@ -452,8 +466,17 @@ async fn turn( // Drain whatever the run emitted between its last event and its return, or // the tail of a turn is lost. + // One age for the whole drain. These events all arrived while the loop was + // not looking, and inventing distinct ages for them would be fiction. + let at = started.elapsed(); + let width = screen.width(); while let Ok(event) = events.try_recv() { - app.event(&event); + app.event(&event, at); + // The diff belongs here too. Without it a `Step { changed: true }` still + // queued when the turn's future won the select loses its diff silently — + // and the last step of a turn is exactly the one that loses that race, + // so the edit a reader most wants to see is the one that vanishes. + commit_edits(app, store, &event, width); } app.finished(); diff --git a/src/settings.rs b/src/settings.rs index 63bfad7..2ab21a3 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -22,6 +22,41 @@ pub struct CliSettings { /// The theme by name. Absent means "detect from the terminal background". #[serde(default, skip_serializing_if = "Option::is_none")] pub theme: Option, + /// How much of a change to show: `unified` or `minimal`. + /// + /// Absent means `unified`, which is what every configuration file written + /// before 0.3.0 means — so this key needs no migration and an older binary + /// reading a file that has it ignores it, because `[app.io-cli]` is the one + /// section io-harness deliberately does not validate. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub diff: Option, +} + +/// How much of a change a diff shows. +/// +/// Two, not a number of context lines. The counter-pressure this answers is +/// approval fatigue: someone reviewing by file rather than by hunk wants the +/// changed lines and nothing else, and a dial from 0 to 3 is a dial nobody sets. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum DiffStyle { + /// The hunk as the harness stored it, context and all. + #[default] + Unified, + /// Changed lines only, with the `@@` header kept so the change still says + /// where in the file it is. + Minimal, +} + +impl DiffStyle { + /// What a configured value means. An unrecognised one is `Unified` rather + /// than an error: `[app.io-cli]` is unvalidated by design, and refusing to + /// start a session over a typo in a cosmetic key would be the wrong trade. + pub fn from_setting(value: Option<&str>) -> Self { + match value { + Some("minimal") => Self::Minimal, + _ => Self::Unified, + } + } } /// A default permission posture, in the words the wizard offers it in. @@ -157,6 +192,11 @@ pub fn render( app: AppSection { io_cli: CliSettings { theme: Some(theme.to_string()), + // Left out of the file the wizard writes. Its absence is + // `unified`, and a key written with its own default is a key a + // reader has to wonder about — and one that would have to be + // rewritten if the default ever changed. + diff: None, }, }, }; diff --git a/tests/approval.rs b/tests/approval.rs index f9a0f69..6084539 100644 --- a/tests/approval.rs +++ b/tests/approval.rs @@ -468,3 +468,128 @@ async fn nothing_remembered_changes_nothing() { let base = Policy::default(); assert_eq!(approval::effective_policy(&base, &[]), base); } + +// --------------------------------------------------------------------------- +// F7 — an approval shows a proposed write as a diff against the file on disk, +// and a new file is all addition. +// --------------------------------------------------------------------------- + +/// Open an overlay for a write of `content` to `target`, and return what it +/// draws at `width` columns in `height` rows. +/// +/// Rendered through a real `Screen` rather than by calling the private layout, +/// because what matters is what reaches the terminal — and because the overlay +/// is height-constrained, which is the whole reason its content flexes. +async fn overlay_for(target: &std::path::Path, content: &str, height: u16) -> String { + use io_cli::approval::{self, Approval}; + use io_cli::theme::DARK; + use io_harness::{Act, ApprovalContext, Approver, Decision, Request}; + + let (asker, mut asks) = approval::channel(); + let request = Request::new(Act::Write, target.to_string_lossy().to_string()) + .with_content(content.to_string()); + let deciding = tokio::spawn(async move { + let asker = asker; + asker + .decide_in_context(&request, &ApprovalContext::new("tidy the parser")) + .await + }); + let ask = asks.recv().await.expect("the question arrived"); + + let approval = Approval::new(ask); + // `height` is the VIEWPORT's height, which is what the overlay is drawn + // into — not the terminal's. A session's is four; a taller one is what a + // reader gets on a terminal with room, and both are worth asserting. + let (mut screen, _recorder) = support::screen_of(100, height + 4, height); + screen + .draw(|frame| approval.render(frame, frame.area(), &DARK)) + .expect("frame"); + let drawn = screen.viewport_text().to_string(); + + approval.answer(approval::Answer::Deny); + let decision = deciding.await.expect("the approver did not panic"); + assert!(matches!(decision, Decision::Deny { .. })); + drawn +} + +#[tokio::test] +async fn f7_a_write_over_an_existing_file_is_shown_as_what_changes() { + let directory = tempfile::tempdir().expect("a temporary directory"); + let target = directory.path().join("parse.rs"); + let before = "fn one() {}\nfn two() {}\nfn three() {}\n"; + let after = "fn one() {}\nfn two(s: &str) {}\nfn three() {}\n"; + std::fs::write(&target, before).expect("the file exists first"); + + let drawn = overlay_for(&target, after, 8).await; + + // The counts are the single most useful row: this write touches one line. + assert!(drawn.contains("+1"), "{drawn}"); + assert!(drawn.contains("-1"), "{drawn}"); + // And the change itself, from the harness's own renderer. + assert!(drawn.contains("-fn two() {}"), "{drawn}"); + assert!(drawn.contains("+fn two(s: &str) {}"), "{drawn}"); + // The version that would ship by accident: every existing line shown as new. + assert!( + !drawn.contains("+fn one() {}"), + "an unchanged line was drawn as an addition, which means the old side was \ + empty — the write reads as four hundred lines when it is one: {drawn}", + ); +} + +#[tokio::test] +async fn f7_a_write_of_a_file_that_does_not_exist_yet_is_all_addition() { + let directory = tempfile::tempdir().expect("a temporary directory"); + let target = directory.path().join("brand-new.rs"); + let after = "fn hello() {}\nfn goodbye() {}\n"; + + let drawn = overlay_for(&target, after, 8).await; + + assert!(drawn.contains("+2"), "two lines arrive: {drawn}"); + assert!(drawn.contains("-0"), "and none leave: {drawn}"); + assert!(drawn.contains("+fn hello() {}"), "{drawn}"); +} + +#[tokio::test] +async fn f7_no_file_other_than_the_request_s_own_target_is_read() { + let directory = tempfile::tempdir().expect("a temporary directory"); + let target = directory.path().join("parse.rs"); + std::fs::write(&target, "fn one() {}\n").expect("the target"); + // A neighbour with a distinctive string in it. io-cli is not a file browser: + // the one workspace read this interface performs is of the file the operator + // is being asked about, and nothing else in the directory is its business. + std::fs::write( + directory.path().join("secrets.env"), + "TOKEN=NEVERREADTHISFILE\n", + ) + .expect("the neighbour"); + + let drawn = overlay_for(&target, "fn one() {}\nfn two() {}\n", 8).await; + + assert!( + !drawn.contains("NEVERREADTHISFILE"), + "a file the request never named reached the screen: {drawn}", + ); +} + +#[tokio::test] +async fn f7_at_the_tightest_size_the_counts_are_what_survives() { + let directory = tempfile::tempdir().expect("a temporary directory"); + let target = directory.path().join("big.rs"); + std::fs::write(&target, "a\n".repeat(400)).expect("the target"); + + // Four rows: the question, its rule row, one row of content, the answers. + let drawn = overlay_for(&target, &"b\n".repeat(400), 4).await; + + // One row for the change, and it is spent on the size of the change rather + // than on the first line of it. Four hundred lines out and four hundred in is + // a different decision from one line out and one in. + assert!(drawn.contains("+400"), "{drawn}"); + assert!(drawn.contains("-400"), "{drawn}"); + // And the overlay still says how to answer. + assert!( + drawn.contains("allow once") && drawn.contains("deny"), + "{drawn}" + ); + // Nothing was silently dropped. + assert!(drawn.contains('⋯'), "the cut has to be said: {drawn}"); +} diff --git a/tests/diff.rs b/tests/diff.rs index a94c24c..40a9135 100644 --- a/tests/diff.rs +++ b/tests/diff.rs @@ -512,3 +512,74 @@ fn f4_a_file_with_no_grammar_still_renders() { let text = rendered(&edit); assert!(text.contains("+hello"), "{text}"); } + +// --------------------------------------------------------------------------- +// F6 — `minimal` shows changed lines only, and comes from the harness's own +// configuration. +// --------------------------------------------------------------------------- + +#[test] +fn f6_minimal_keeps_the_changed_lines_and_the_header_and_drops_the_context() { + use io_cli::diff::cell_styled; + use io_cli::settings::DiffStyle; + + let edit = edit_with_hunk(); + let text = |style| { + cell_styled(&edit, &DARK, WIDE, style) + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>() + .join("\n") + }; + + let unified = text(DiffStyle::Unified); + let minimal = text(DiffStyle::Minimal); + + // What changed survives both. + for style in [&unified, &minimal] { + assert!( + style.contains("- Tone::Muted => Style::default().fg(GREY),"), + "{style}" + ); + assert!( + style.contains("+ Tone::Muted => Style::default().fg(self.muted),"), + "{style}" + ); + } + + // The context is what `minimal` drops, and only that. + assert!(unified.contains("fn one() {}"), "{unified}"); + assert!( + !minimal.contains("fn one() {}"), + "minimal kept a context line:\n{minimal}" + ); + assert!(!minimal.contains("fn two() {}"), "{minimal}"); + + // The `@@` header stays. A change with no line numbers is a change that does + // not say where in the file it is, which is not a smaller diff — it is a + // worse one. + assert!( + minimal.contains("@@ -12,5 +12,5 @@"), + "minimal dropped the line numbers:\n{minimal}", + ); + assert!(minimal.contains("src/theme.rs"), "{minimal}"); +} + +#[test] +fn f6_the_style_is_read_from_the_setting_and_an_unknown_value_is_unified() { + use io_cli::settings::DiffStyle; + + assert_eq!(DiffStyle::from_setting(Some("minimal")), DiffStyle::Minimal); + assert_eq!(DiffStyle::from_setting(Some("unified")), DiffStyle::Unified); + // Absent means unified, which is what every file written before 0.3.0 means. + assert_eq!(DiffStyle::from_setting(None), DiffStyle::Unified); + // `[app.io-cli]` is the one section io-harness deliberately does not + // validate, so a typo in a cosmetic key must not stop a session starting. + assert_eq!(DiffStyle::from_setting(Some("minmal")), DiffStyle::Unified); + assert_eq!(DiffStyle::default(), DiffStyle::Unified); +} diff --git a/tests/events.rs b/tests/events.rs index 77be9af..dab965f 100644 --- a/tests/events.rs +++ b/tests/events.rs @@ -7,13 +7,21 @@ mod support; +use std::time::Duration; + use io_cli::events::{kind_name, Events}; use io_cli::theme::DARK; use io_harness::{EventKind, RunEvent}; -/// The kinds this release styles, in the contract's own words: `started`, +/// The kinds this release handles, in the contract's own words: `started`, /// `token`, `step`, `tool_call` and `finished` rendered fully, `refused` and /// `approval_requested` as a plain one-line notice. +/// +/// `mcp` is here from 0.3.0 for a reason that is not visible in its own line: it +/// is the only event io-harness emits carrying how long a tool actually ran, and +/// that number is harvested onto the open tool cell. Its own line is still the +/// muted one naming the kind — an event is never consumed silently — but it is no +/// longer a kind this release merely passes through. const STYLED: &[&str] = &[ "started", "token", @@ -22,6 +30,7 @@ const STYLED: &[&str] = &[ "finished", "refused", "approval_requested", + "mcp", ]; /// Every other kind io-harness 0.60.1 emits. Each renders as a muted single line @@ -50,7 +59,6 @@ const FALLS_THROUGH: &[&str] = &[ "reasoning", "server_tool_used", "sandbox", - "mcp", "handle_started", "handle_polled", "handle_killed", @@ -78,9 +86,22 @@ fn event(kind: EventKind) -> RunEvent { RunEvent::new(1, 1, kind) } +/// What one event commits, at a session age nothing measured. +/// +/// `Duration::ZERO` is the right default for every test that is not about a tool +/// cell's duration: the age is an argument handed in by the driver, so a test +/// that does not care about it states zero rather than arranging for a clock. fn rendered(events: &mut Events, kind: EventKind) -> String { - events - .event(&event(kind)) + rendered_at(events, kind, Duration::ZERO) +} + +/// The same, at a stated session age. Stated, never measured — N1. +fn rendered_at(events: &mut Events, kind: EventKind, at: Duration) -> String { + flatten(events.event(&event(kind), at)) +} + +fn flatten(lines: Vec>) -> String { + lines .iter() .flat_map(|line| line.spans.iter()) .map(|span| span.content.as_ref()) @@ -90,8 +111,8 @@ fn rendered(events: &mut Events, kind: EventKind) -> String { #[test] fn f5_a_step_reads_decision_then_tool_then_result_then_its_metadata() { let mut events = Events::new(DARK); - let line: String = events - .event(&RunEvent::new( + let line: String = flatten(events.event( + &RunEvent::new( 1, 7, EventKind::Step { @@ -100,11 +121,9 @@ fn f5_a_step_reads_decision_then_tool_then_result_then_its_metadata() { tokens: 1234, changed: true, }, - )) - .iter() - .flat_map(|line| line.spans.iter()) - .map(|span| span.content.as_ref()) - .collect(); + ), + Duration::ZERO, + )); // The order is the assertion, not the contents. A line carrying all five // facts in the wrong order is the line 0.1.0 shipped. @@ -136,8 +155,8 @@ fn f5_a_step_reads_decision_then_tool_then_result_then_its_metadata() { #[test] fn f5_a_step_that_changed_nothing_still_says_so() { let mut events = Events::new(DARK); - let line: String = events - .event(&RunEvent::new( + let line: String = flatten(events.event( + &RunEvent::new( 1, 2, EventKind::Step { @@ -146,11 +165,9 @@ fn f5_a_step_that_changed_nothing_still_says_so() { tokens: 88, changed: false, }, - )) - .iter() - .flat_map(|line| line.spans.iter()) - .map(|span| span.content.as_ref()) - .collect(); + ), + Duration::ZERO, + )); // The result is always present, so a skim down the transcript reads the same // column of answers whether or not a step touched anything. @@ -179,15 +196,29 @@ fn f8_every_styled_kind_renders_its_own_facts() { ); assert!(started.contains("openrouter"), "{started:?}"); - let tool = rendered( - &mut events, - EventKind::ToolCall { + // A tool call's facts are asserted where they are committed, which is the step + // that finished it. The announcement itself commits nothing, because when it + // arrives io-harness does not yet know what came back. + events.event( + &event(EventKind::ToolCall { name: "exec".into(), target: "cargo test".into(), + }), + Duration::ZERO, + ); + let tool = rendered_at( + &mut events, + EventKind::Step { + decision: "ran cargo test".into(), + tool_call: "exec".into(), + tokens: 12, + changed: false, }, + Duration::from_millis(900), ); assert!(tool.contains("exec"), "{tool:?}"); assert!(tool.contains("cargo test"), "{tool:?}"); + assert!(tool.contains("ran cargo test"), "{tool:?}"); let step = rendered( &mut events, @@ -261,7 +292,10 @@ fn f8_a_kind_this_release_does_not_style_still_renders_and_names_itself() { fn f8_tokens_are_coalesced_rather_than_committed_one_at_a_time() { let mut events = Events::new(DARK); for word in ["Here ", "is ", "the ", "answer."] { - let committed = events.event(&event(EventKind::Token { text: word.into() })); + let committed = events.event( + &event(EventKind::Token { text: word.into() }), + Duration::ZERO, + ); assert!( committed.is_empty(), "a token committed a line to scrollback on its own", @@ -282,8 +316,16 @@ fn f8_tokens_are_coalesced_rather_than_committed_one_at_a_time() { assert_eq!(events.live(), "", "the live buffer should be emptied"); } +/// **F8.** No event is dropped. +/// +/// The invariant is not "every event commits a line" — a token does not, and from +/// 0.3.0 neither does a tool call. Both are *deferred* rather than discarded: the +/// token into the live buffer, the call into an open cell that `live()` shows +/// immediately and that the step commits complete. So each kind must leave a mark +/// somewhere a reader can see it, and the two that defer are named here rather +/// than exempted, so that a third one cannot be added silently. #[test] -fn f8_no_event_renders_to_nothing_except_a_token() { +fn f8_no_event_is_dropped_though_a_token_and_a_tool_call_are_deferred() { let mut events = Events::new(DARK); let kinds = vec![ EventKind::Started { @@ -297,7 +339,7 @@ fn f8_no_event_renders_to_nothing_except_a_token() { changed: false, }, EventKind::ToolCall { - name: "n".into(), + name: "read_file".into(), target: String::new(), }, EventKind::Refused { @@ -321,12 +363,32 @@ fn f8_no_event_renders_to_nothing_except_a_token() { for kind in kinds { let name = kind_name(&kind); - let lines = events.event(&event(kind)); + let lines = events.event(&event(kind), Duration::ZERO); + if name == "tool_call" { + // Deferred, and visible while it is deferred. An open call that + // committed nothing AND showed nothing would be the discarded event + // this test exists to catch. + assert!( + lines.is_empty(), + "a tool call committed a line before its result was known", + ); + assert!( + events.live().contains("read_file"), + "a deferred tool call must be visible in the viewport: {:?}", + events.live(), + ); + continue; + } assert!( !lines.is_empty(), "{name} rendered to nothing, which is an event silently discarded", ); } + + // The turn ends, and the call that nothing ever reported on is still accounted + // for rather than lost with the turn. + let closed = flatten(events.flush()); + assert!(closed.contains("read_file"), "{closed:?}"); } #[test] @@ -541,6 +603,215 @@ fn f8_a_refusal_with_no_rule_says_the_tier_default_decided() { ); } +/// **F2.** A call announced is not a call finished. +/// +/// `EventKind::ToolCall` is emitted before the tool runs, so anything committed +/// here could only say what the agent was about to do — which is the line 0.2.0 +/// shipped and the reason a transcript never said what came back. It is held +/// open instead, and shown live, which is the half of the design that keeps the +/// deferral from being a disappearance. +#[test] +fn f2_a_tool_call_commits_nothing_until_its_step_lands() { + let mut events = Events::new(DARK); + let lines = events.event( + &event(EventKind::ToolCall { + name: "read_file".into(), + target: "src/lib.rs".into(), + }), + Duration::ZERO, + ); + + assert!( + lines.is_empty(), + "a call committed a line before anything knew its result: {lines:?}", + ); + let live = events.live(); + assert!(live.contains("read_file"), "{live:?}"); + assert!(live.contains("src/lib.rs"), "{live:?}"); +} + +/// **F2.** The whole cell, in one line, when the step that ran it commits. +/// +/// Asserted by position rather than by presence. A line carrying all four facts +/// inside out passes `contains` just as green, which this repository has now paid +/// to learn twice. +#[test] +fn f2_a_tool_cell_commits_its_result_and_its_observed_duration() { + let mut events = Events::new(DARK); + events.event( + &event(EventKind::ToolCall { + name: "read_file".into(), + target: "src/lib.rs".into(), + }), + Duration::ZERO, + ); + let line = rendered_at( + &mut events, + EventKind::Step { + // io-harness's own sentence about what the call did, written after it + // ran. The cell says this and never a word io-cli made up. + decision: "read src/lib.rs".into(), + tool_call: "read_file".into(), + tokens: 5, + changed: false, + }, + Duration::from_millis(250), + ); + + let at = |needle: &str| { + line.find(needle) + .unwrap_or_else(|| panic!("{needle:?} is missing from {line:?}")) + }; + assert!( + at("read_file") < at("src/lib.rs"), + "the tool before its target: {line:?}", + ); + assert!( + at("src/lib.rs") < at("read src/lib.rs"), + "the target before what came back: {line:?}", + ); + assert!( + at("read src/lib.rs") < at("~250ms"), + "content before metadata: {line:?}", + ); + + // The `~` is load-bearing. io-cli did not time the tool; it observed the + // interval between two events, which also contains the model's turnaround and + // whatever queued in front of the call. A bare `250ms` here would be io-cli + // claiming a measurement it never made. + assert!( + !line.contains(" 250ms"), + "an observed interval must not read as a measured duration: {line:?}", + ); +} + +/// **F2.** A refused call never closes as a completed one. +/// +/// A refusal does not end the step — io-harness feeds it back to the model as an +/// observation and the step commits anyway — so the cell is still closed by that +/// step. What it must not do is close wearing the step's own verdict, which would +/// report a call that was stopped as a call that ran. +#[test] +fn f2_a_refused_call_does_not_close_as_a_completed_one() { + let mut events = Events::new(DARK); + events.event( + &event(EventKind::ToolCall { + name: "write_file".into(), + target: "src/main.rs".into(), + }), + Duration::ZERO, + ); + rendered( + &mut events, + EventKind::Refused { + act: "write".into(), + target: "/repo/src/main.rs".into(), + rule: Some("fs.deny".into()), + layer: Some("workspace".into()), + }, + ); + + // Two decisions against one open call: the count does not pair, which is the + // ordinary shape of a step that also waited on an approval. The refusal is + // then the only true thing left to say about the call. + let line = rendered_at( + &mut events, + EventKind::Step { + decision: "write refused; awaiting approval (write)".into(), + tool_call: "write_file".into(), + tokens: 40, + changed: false, + }, + Duration::from_millis(300), + ); + + assert!(line.contains("refused"), "{line:?}"); + assert!( + !line.contains("changed files"), + "a refused call must not close as one that changed something: {line:?}", + ); + assert!( + line.find("refused") < line.find("no change"), + "the cell closes before the step it belonged to: {line:?}", + ); +} + +/// **F2.** A call nothing ever reported on closes as unfinished, with no number. +/// +/// `Step` may never arrive: io-harness skips `commit_step` when a sub-agent's +/// child deferred, and a turn can be interrupted mid-call. The cell is still +/// accounted for — and carries no duration, because io-cli knows when the call +/// was announced and nothing whatever about when it stopped. +#[test] +fn f2_an_unfinished_call_closes_without_inventing_a_duration() { + let mut events = Events::new(DARK); + events.event( + &event(EventKind::ToolCall { + name: "read_file".into(), + target: "src/lib.rs".into(), + }), + Duration::ZERO, + ); + + let line = flatten(events.flush()); + assert!(line.contains("read_file"), "{line:?}"); + assert!(line.contains("unfinished"), "{line:?}"); + assert!( + !line.contains('~') && !line.contains("ms"), + "a duration here would be a guess wearing a measurement's clothes: {line:?}", + ); + + // Closed once. A cell that survived its own closing would arrive again in the + // next turn's scrollback. + assert_eq!(events.live(), ""); +} + +/// **F2.** A parallel batch is the ordinary case, not the edge one. +/// +/// `read_batch` announces every call in a batch up front and only then runs any of +/// them, so two calls before one step is what a normal parallel read looks like. A +/// single-slot design is wrong on the first one, and a single shared duration +/// would hide which of the two was slow. +#[test] +fn f2_two_calls_before_one_step_each_close_with_their_own_duration() { + let mut events = Events::new(DARK); + events.event( + &event(EventKind::ToolCall { + name: "read_file".into(), + target: "src/a.rs".into(), + }), + Duration::ZERO, + ); + events.event( + &event(EventKind::ToolCall { + name: "read_file".into(), + target: "src/b.rs".into(), + }), + Duration::from_millis(100), + ); + + let line = rendered_at( + &mut events, + EventKind::Step { + decision: "read src/a.rs; read src/b.rs".into(), + tool_call: "read_file".into(), + tokens: 60, + changed: false, + }, + Duration::from_millis(300), + ); + + let at = |needle: &str| { + line.find(needle) + .unwrap_or_else(|| panic!("{needle:?} is missing from {line:?}")) + }; + // In the order they were announced, each paired with the decision written for + // it — and each measured from its own announcement, so the second call is not + // charged for the time the first one spent. + assert!(at("src/a.rs") < at("src/b.rs"), "{line:?}"); + assert!(at("~300ms") < at("~200ms"), "{line:?}"); +} + /// A rule with no layer is possible and is not the same as no rule at all. It is /// rendered as what it is rather than being rounded to either neighbour. #[test] diff --git a/tests/interrupt.rs b/tests/interrupt.rs index c7c3fc3..e55c474 100644 --- a/tests/interrupt.rs +++ b/tests/interrupt.rs @@ -51,7 +51,10 @@ fn f6_ctrl_c_during_a_turn_interrupts_it_and_keeps_the_partial_output() { // The model streams some of an answer. for word in ["Looking ", "at ", "the ", "parser"] { - app.event(&RunEvent::new(1, 1, EventKind::Token { text: word.into() })); + app.event( + &RunEvent::new(1, 1, EventKind::Token { text: word.into() }), + std::time::Duration::ZERO, + ); } assert_eq!(app.events.live(), "Looking at the parser"); @@ -215,6 +218,9 @@ fn f9_a_streaming_answer_of_any_length_leaves_the_viewport_the_same_size() { fn app_lines(app: &mut App, text: &str) -> usize { let before = app.take_pending().len(); assert_eq!(before, 0, "the caller should have drained first"); - app.event(&RunEvent::new(1, 1, EventKind::Token { text: text.into() })); + app.event( + &RunEvent::new(1, 1, EventKind::Token { text: text.into() }), + std::time::Duration::ZERO, + ); app.take_pending().len() } diff --git a/tests/live.rs b/tests/live.rs index b4311d3..9f3cef1 100644 --- a/tests/live.rs +++ b/tests/live.rs @@ -160,7 +160,7 @@ async fn live_f1_a_real_turn_streams_and_edits_a_file() { let mut renderer = Events::new(DARK); let mut committed = Vec::new(); for event in &events { - committed.extend(renderer.event(event)); + committed.extend(renderer.event(event, std::time::Duration::ZERO)); } committed.extend(renderer.flush()); let transcript: String = committed @@ -383,7 +383,7 @@ async fn live_f10_a_denial_leaves_the_file_alone() { let mut renderer = Events::new(DARK); let transcript: String = events .iter() - .flat_map(|event| renderer.event(event)) + .flat_map(|event| renderer.event(event, std::time::Duration::ZERO)) .map(|line| { line.spans .iter() diff --git a/tests/status.rs b/tests/status.rs index 2aee696..ba4393d 100644 --- a/tests/status.rs +++ b/tests/status.rs @@ -227,8 +227,8 @@ fn f9_a_field_with_nothing_behind_it_is_absent_rather_than_zero() { #[test] fn the_token_field_is_the_session_and_not_the_last_step() { let mut app = App::new(DARK, "opus-5"); - app.event(&step(1, 1_200)); - app.event(&step(2, 300)); + app.event(&step(1, 1_200), std::time::Duration::ZERO); + app.event(&step(2, 300), std::time::Duration::ZERO); let line = app.status.line(120, &DARK).to_string(); assert!( @@ -244,11 +244,14 @@ fn the_token_field_is_the_session_and_not_the_last_step() { #[test] fn the_containment_field_carries_the_backend_and_not_only_the_mode() { let mut app = App::new(DARK, "opus-5"); - app.event(&event(EventKind::Contained { - mode: "workspace-write".into(), - backend: "portable-floor".into(), - roots: 0, - })); + app.event( + &event(EventKind::Contained { + mode: "workspace-write".into(), + backend: "portable-floor".into(), + roots: 0, + }), + std::time::Duration::ZERO, + ); let line = app.status.line(120, &DARK).to_string(); assert!(line.contains("workspace-write"), "{line:?}"); @@ -266,11 +269,14 @@ fn the_context_field_appears_when_a_fold_reports_one() { let mut app = App::new(DARK, "opus-5"); assert!(!app.status.line(120, &DARK).to_string().contains("ctx")); - app.event(&event(EventKind::Compacted { - through_step: 4, - before_tokens: 11_000, - after_tokens: 6_000, - })); + app.event( + &event(EventKind::Compacted { + through_step: 4, + before_tokens: 11_000, + after_tokens: 6_000, + }), + std::time::Duration::ZERO, + ); let line = app.status.line(120, &DARK).to_string(); assert!( @@ -287,12 +293,15 @@ fn the_context_field_appears_when_a_fold_reports_one() { fn the_new_fields_drop_from_the_right_rather_than_wrapping() { let mut app = App::new(DARK, "opus-5"); app.set_posture(Some(io_cli::settings::Posture::Workspace)); - app.event(&step(1, 12_400)); - app.event(&event(EventKind::Contained { - mode: "workspace-write".into(), - backend: "seatbelt".into(), - roots: 2, - })); + app.event(&step(1, 12_400), std::time::Duration::ZERO); + app.event( + &event(EventKind::Contained { + mode: "workspace-write".into(), + backend: "seatbelt".into(), + roots: 2, + }), + std::time::Duration::ZERO, + ); let wide = app.status.line(160, &DARK).to_string(); assert!(wide.contains("seatbelt"), "{wide:?}"); From cecdeb2439fd50fb579cdc7cebb5eb33374b2536 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Mon, 17 Aug 2026 09:48:21 +0530 Subject: [PATCH 7/8] feat: reach the transcript, the expander and the clipboard from the keyboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ctrl+T commits the whole conversation into the terminal's own scrollback, branched-away turns included and labelled — TranscriptTurn.on_path is the only place that fact is reachable and Session::history deliberately does not return them. /expand commits the last step's stored detail upward, read back out of the run's durable trace. That is the other half of collapsing a tool cell: the output went to the trace when it happened, so the screen never had to be the archive. /copy and /copy diff put the last answer or Store::patch of the whole run on the system clipboard over OSC 52 — the clipboard of the machine the operator is sitting at, not the one they are ssh'd into. Screen::escape writes the sequence straight to the terminal rather than through commit, because a clipboard sequence in the scrollback is text somebody scrolls past rather than an instruction the terminal acted on. The line printed afterwards says what was sent and how large it was and never that it succeeded: nothing acknowledges an OSC 52 write, so a success message would be a claim this product cannot support. Everything that shows more of something commits upward. The viewport is four rows and cannot grow and there is no alternate screen, so that is one answer to "show me more" rather than three. The command and key inventory tests were restated rather than relaxed: both spell out the full list, so adding a command stays a decision somebody makes rather than a line somebody adds. --- README.md | 24 ++++++++ docs/config.example.toml | 8 +++ src/app.rs | 10 ++++ src/commands.rs | 55 +++++++++++++++++-- src/main.rs | 116 ++++++++++++++++++++++++++++++++++++++- src/term.rs | 19 +++++++ tests/commands.rs | 35 ++++++++++-- 7 files changed, 257 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index a4bcc75..07ab083 100644 --- a/README.md +++ b/README.md @@ -94,6 +94,7 @@ writes no key to disk at all. | `Ctrl+D` | exit, on an empty prompt | | `Shift+Tab` | cycle the permission posture, from the next turn | | `Ctrl+L` | clear the viewport, never the scrollback | +| `Ctrl+T` | put the whole conversation back into the scrollback | | `y / a / n` | answer an approval: allow once, allow this session, deny | | `Esc` | close a picker without choosing | @@ -110,9 +111,26 @@ writes no key to disk at all. | `/setup` | run the first-run wizard again | | `/theme` | change the theme for this session | | `/model` | change the model for this session | +| `/expand` | commit the last step's full detail into the scrollback | +| `/copy` | put the last answer on the system clipboard | +| `/copy diff` | put the whole run's patch on the system clipboard | +**Everything that shows you more of something writes it into the terminal's own +scrollback.** `Ctrl+T` and `/expand` do not open a pane: the viewport is a few +rows and this product has no alternate screen, so the place to read something +long is the buffer where the terminal's search, selection and tmux copy-mode +already work. `/expand` reads the step's full output back out of the run's +durable trace, which is where it went in the first place — the screen is not the +archive. + +`/copy` uses OSC 52, so it reaches the clipboard of the machine you are *sitting +at* rather than the one you are ssh'd into. Nothing acknowledges an OSC 52 write: +the line it prints says what was sent and how large it was, never that it +succeeded. Inside tmux it needs `set -g set-clipboard on`, and some terminals +refuse a large payload without saying so. + `/theme` and `/model` change this session only and say so. Making a choice permanent is `io setup`. @@ -122,6 +140,12 @@ io-cli has no configuration parser. io-harness owns discovery and layering, and io-cli's own settings live in the `[app.io-cli]` section that io-harness deliberately does not validate. See [`docs/config.example.toml`](docs/config.example.toml). +Two keys live there: `theme`, and `diff`, which is `unified` (the default, and +what an absent key means) or `minimal` — the changed lines and the `@@` header +without the context, for reviewing by file rather than by hunk. Because the +section is unvalidated by design, an unrecognised value reads as the default +rather than stopping a session from starting. + The file is found in this order: `$IO_CONFIG`, else `$IO_CONFIG_HOME/io.toml`, else `$XDG_CONFIG_HOME/io/io.toml` or `~/.config/io/io.toml`, and `%APPDATA%\io\io.toml` on Windows. A project's own `io.toml` and a gitignored diff --git a/docs/config.example.toml b/docs/config.example.toml index d6f77ad..021822a 100644 --- a/docs/config.example.toml +++ b/docs/config.example.toml @@ -91,3 +91,11 @@ net = "deny" [app.io-cli] # "dark" or "light". Absent means detect from the terminal background. theme = "dark" +# How much of a change a diff shows. +# +# "unified" the hunk as the harness stored it, context and all. The default, +# and what an absent key means — so a file written before 0.3.0 +# needs no change. +# "minimal" the changed lines and the @@ header, without the context. For +# reviewing by file rather than by hunk. +diff = "unified" diff --git a/src/app.rs b/src/app.rs index 86a1d3d..e757021 100644 --- a/src/app.rs +++ b/src/app.rs @@ -52,6 +52,11 @@ pub enum Command { Exit, /// Repaint the viewport from scratch, never the scrollback. ClearViewport, + /// Put the whole conversation back into the terminal's own scrollback. + /// + /// A command rather than something this type does, because the transcript + /// lives in the harness's store and the store belongs to the driver. + Transcript, } pub struct App { @@ -345,6 +350,11 @@ impl App { // destroy the transcript, which on this renderer is the terminal's own // buffer rather than something this process can redraw. (KeyCode::Char('l'), true) => Command::ClearViewport, + // Upward, never into a pane. The conversation is already in the + // terminal's scrollback; this puts back the part that has scrolled + // out of reach, branched-away turns included, where the terminal's + // own search and copy-mode already work on it. + (KeyCode::Char('t'), true) => Command::Transcript, _ => { self.quits = 0; match self.composer.key(key) { diff --git a/src/commands.rs b/src/commands.rs index ea224b7..cf3064b 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1,9 +1,16 @@ -//! The five slash commands, and the keybinding table they document. +//! The slash commands, and the keybinding table they document. //! -//! Five, and each one is a [`Picker`](crate::picker::Picker) or a print. The -//! fuzzy palette that reaches every command and every harness skill is 0.7.0's; -//! what matters now is that `/setup` exists, because it is what makes the wizard -//! reachable after the first run. +//! Each one is a [`Picker`](crate::picker::Picker), a print, or something +//! committed into the terminal's own scrollback. The fuzzy palette that reaches +//! every command and every harness skill is 0.7.0's; what matters now is that +//! `/setup` exists, because it is what makes the wizard reachable after the +//! first run. +//! +//! **Everything that shows more of something commits upward.** The viewport is +//! four rows and cannot grow, so `/expand` and `Ctrl+T` do not open a pane — they +//! write into the scrollback, where the terminal's own search, selection and +//! copy-mode already work. That is one answer to "show me more" rather than +//! three, and it is the same answer the transcript gives. use ratatui::text::{Line, Span}; @@ -30,6 +37,10 @@ pub const KEYS: &[(&str, &str)] = &[ "cycle the permission posture, from the next turn", ), ("Ctrl+L", "clear the viewport, never the scrollback"), + ( + "Ctrl+T", + "put the whole conversation back into the scrollback", + ), ( "y / a / n", "answer an approval: allow once, allow this session, deny", @@ -44,6 +55,15 @@ pub const COMMANDS: &[(&str, &str)] = &[ ("/setup", "run the first-run wizard again"), ("/theme", "change the theme for this session"), ("/model", "change the model for this session"), + ( + "/expand", + "commit the last step's full detail into the scrollback", + ), + ("/copy", "put the last answer on the system clipboard"), + ( + "/copy diff", + "put the whole run's patch on the system clipboard", + ), ]; /// What the driver should do about a slash command. @@ -57,6 +77,24 @@ pub enum Action { Theme, /// Open the model picker. Model, + /// Commit the last step's stored detail into the scrollback. + /// + /// The detail is in the run's durable trace already — this reads it back + /// rather than the screen having been the archive. + Expand, + /// Put something on the system clipboard over OSC 52. + Copy(Copied), + /// Put the whole conversation back into the scrollback. + Transcript, +} + +/// What `/copy` was asked for. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Copied { + /// The last thing the agent said. + Answer, + /// Every change the run made, as one patch. + Diff, } /// Resolve a command. The leading `/` has already been removed. @@ -70,6 +108,13 @@ pub fn parse(input: &str, theme: &Theme) -> Action { "setup" => Action::Setup, "theme" => Action::Theme, "model" => Action::Model, + "expand" => Action::Expand, + "copy" => match input.split_whitespace().nth(1) { + // `/copy diff` and `/copy patch` mean the same thing. A reader who + // has just been shown a diff will type the word they were shown. + Some("diff") | Some("patch") => Action::Copy(Copied::Diff), + _ => Action::Copy(Copied::Answer), + }, unknown => { let mut lines = vec![theme.notice( Tone::Warning, diff --git a/src/main.rs b/src/main.rs index 4e810d1..2f54fef 100644 --- a/src/main.rs +++ b/src/main.rs @@ -22,13 +22,14 @@ use tokio::sync::mpsc::{unbounded_channel, UnboundedReceiver}; use io_cli::app::{App, Command}; use io_cli::cli::{Cli, Command as Subcommand}; -use io_cli::commands::{self, Action}; +use io_cli::commands::{self, Action, Copied}; use io_cli::picker::{Outcome, Picker, Row}; use io_cli::settings::{self, CliSettings, Posture}; use io_cli::term::Screen; use io_cli::theme::{Theme, Tone}; use io_cli::wizard::{Progress, Wizard}; use io_cli::{approval, bridge, splash, verify}; +use ratatui::text::{Line, Span}; fn main() -> ExitCode { let runtime = match tokio::runtime::Runtime::new() { @@ -324,6 +325,7 @@ async fn loop_over( // The viewport, and nothing above it. paint(screen, &mut app)?; } + Command::Transcript => commit_transcript(screen, &session, &store, &app.theme)?, Command::Slash(text) => match commands::parse(&text, &app.theme) { Action::Print(lines) => { screen.commit(&lines).map_err(|error| error.to_string())?; @@ -354,6 +356,26 @@ async fn loop_over( )]; picker = Some((Picker::new("Which model?", rows), Pick::Model)); } + Action::Expand => { + let lines = expand(&session, &store, &app.theme); + screen.commit(&lines).map_err(|error| error.to_string())?; + } + Action::Copy(what) => { + let (payload, said) = to_copy(&session, &store, what); + match payload { + Some(payload) => { + screen + .escape(&io_cli::clipboard::sequence(&payload)) + .map_err(|error| error.to_string())?; + // What was sent and how big it was — never "copied". + // Nothing answers an OSC 52 write, so a success + // message would be a claim this product cannot make. + app.say(Tone::Muted, io_cli::clipboard::describe(&payload)); + } + None => app.say(Tone::Muted, said), + } + } + Action::Transcript => commit_transcript(screen, &session, &store, &app.theme)?, }, Command::Submit(text) => { // Rebuilt every turn rather than kept, because `remembered` grows @@ -518,6 +540,98 @@ fn commit_edits(app: &mut App, store: &Store, event: &io_harness::RunEvent, widt } } +/// Put the whole conversation back into the terminal's own scrollback. +/// +/// Upward and never into a pane. The viewport is four rows and cannot grow, and +/// there is no alternate screen in this product — so the place a reader reads +/// something long is the terminal's own buffer, where its search, its selection +/// and tmux copy-mode already work. A failure to read the store says so and +/// changes nothing else: a session is not worth ending over a transcript. +fn commit_transcript( + screen: &mut Screen>, + session: &Session, + store: &Store, + theme: &Theme, +) -> Result<(), String> { + let lines = match session.transcript(store) { + Ok(transcript) => io_cli::transcript::lines(&transcript, theme), + Err(error) => vec![theme.notice( + Tone::Muted, + format!("the transcript could not be read: {error}"), + )], + }; + screen.commit(&lines).map_err(|error| error.to_string()) +} + +/// The last run of this session, if it has had one. +fn last_run(session: &Session, store: &Store) -> Option { + session + .transcript(store) + .ok()? + .turns + .into_iter() + .rfind(|turn| turn.on_path) +} + +/// The last step's full detail, from the run's durable trace. +/// +/// This is the other half of collapsing a tool cell: the screen is not the +/// archive, so the output goes to the store when it happens and comes back here +/// when somebody asks for it. Committed upward like everything else that shows +/// more of something. +fn expand(session: &Session, store: &Store, theme: &Theme) -> Vec> { + let Some(turn) = last_run(session, store) else { + return vec![theme.notice(Tone::Muted, "nothing has run in this session yet")]; + }; + let steps = match store.steps(turn.run_id) { + Ok(steps) => steps, + Err(error) => { + return vec![theme.notice(Tone::Muted, format!("the trace could not be read: {error}"))] + } + }; + let Some(step) = steps.last() else { + return vec![theme.notice(Tone::Muted, "that turn recorded no steps")]; + }; + if step.result.trim().is_empty() { + return vec![theme.notice( + Tone::Muted, + format!("step {} recorded no detail", step.step), + )]; + } + + let mut lines = vec![theme.notice( + Tone::Muted, + format!("step {} — {}", step.step, step.decision), + )]; + lines.extend( + step.result + .lines() + .map(|line| Line::from(Span::styled(format!(" {line}"), theme.style(Tone::Muted)))), + ); + lines.push(Line::from("")); + lines +} + +/// What `/copy` should put on the clipboard, or why there is nothing to put. +fn to_copy(session: &Session, store: &Store, what: Copied) -> (Option, String) { + let Some(turn) = last_run(session, store) else { + return (None, "nothing has run in this session yet".into()); + }; + match what { + Copied::Answer => match turn.reply { + Some(reply) if !reply.trim().is_empty() => (Some(reply), String::new()), + // A turn that stopped on a ceiling, a refusal or an interrupt has no + // closing message, and inventing one would misreport the ending. + _ => (None, "that turn ended without an answer to copy".into()), + }, + Copied::Diff => match store.patch(turn.run_id) { + Ok(patch) if !patch.trim().is_empty() => (Some(patch), String::new()), + Ok(_) => (None, "that turn changed no files".into()), + Err(error) => (None, format!("the patch could not be read: {error}")), + }, + } +} + /// The first-run wizard. Returns the theme chosen, or `None` if it was abandoned. async fn wizard( screen: &mut Screen>, diff --git a/src/term.rs b/src/term.rs index afd4ab6..1f5a828 100644 --- a/src/term.rs +++ b/src/term.rs @@ -209,6 +209,25 @@ impl Screen { Write::flush(self.terminal.backend_mut()) } + /// Write a raw escape sequence straight to the terminal. + /// + /// For OSC 52, which is a message to the *terminal emulator* rather than + /// content for the screen: it puts a payload on the system clipboard and + /// draws nothing. It cannot be a widget, and it must not go through + /// [`Screen::commit`] — a clipboard sequence in the scrollback would be + /// text the user scrolls past rather than an instruction the terminal acted + /// on. + /// + /// Nothing here can confirm it worked. No terminal answers an OSC 52 write; + /// several cap the payload silently and tmux ignores it entirely without + /// `set -g set-clipboard on`. The `Ok` this returns means the bytes left the + /// process, which is the only thing that can honestly be reported. + pub fn escape(&mut self, sequence: &str) -> io::Result<()> { + let backend = self.terminal.backend_mut(); + backend.write_all(sequence.as_bytes())?; + Write::flush(backend) + } + /// Tell the renderer the terminal is a different size now. /// /// Recomputing the inline viewport is the whole job: the committed lines above diff --git a/tests/commands.rs b/tests/commands.rs index 45c6ee0..9db9e21 100644 --- a/tests/commands.rs +++ b/tests/commands.rs @@ -17,12 +17,22 @@ fn text(lines: &[ratatui::text::Line<'_>]) -> String { } #[test] -fn the_five_commands_are_the_five_commands() { +fn the_commands_are_the_commands() { let names: Vec<&str> = COMMANDS.iter().map(|(name, _)| *name).collect(); assert_eq!( names, - ["/help", "/quit", "/setup", "/theme", "/model"], - "the palette is 0.7.0; this release ships five", + [ + "/help", + "/quit", + "/setup", + "/theme", + "/model", + "/expand", + "/copy", + "/copy diff", + ], + "the fuzzy palette is still 0.7.0; this list is written out so that adding \ + a command is a decision somebody makes rather than a line somebody adds", ); for (name, what) in COMMANDS { assert!(name.starts_with('/'), "{name}"); @@ -37,6 +47,22 @@ fn each_command_resolves() { assert_eq!(commands::parse("setup", &DARK), Action::Setup); assert_eq!(commands::parse("theme", &DARK), Action::Theme); assert_eq!(commands::parse("model", &DARK), Action::Model); + assert_eq!(commands::parse("expand", &DARK), Action::Expand); + // `/copy` with no argument is the answer; `diff` and `patch` are the same + // thing, because a reader who has just been shown a diff types the word they + // were shown. + assert_eq!( + commands::parse("copy", &DARK), + Action::Copy(io_cli::commands::Copied::Answer), + ); + assert_eq!( + commands::parse("copy diff", &DARK), + Action::Copy(io_cli::commands::Copied::Diff), + ); + assert_eq!( + commands::parse("copy patch", &DARK), + Action::Copy(io_cli::commands::Copied::Diff), + ); // Arguments are tolerated; the first word decides. assert_eq!(commands::parse("model gpt-5", &DARK), Action::Model); @@ -87,13 +113,14 @@ fn the_key_table_covers_every_key_this_release_binds() { "Ctrl+L", "Esc", "Shift+Tab", + "Ctrl+T", "y / a / n", ] { assert!(documented.contains(&key), "{key} is bound but undocumented"); } assert_eq!( documented.len(), - 9, + 10, "a key was added to the table without being added to this list, or the \ other way round", ); From bed8d8a5eb55eac326b3d6d8f97549798246966a Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Mon, 17 Aug 2026 10:03:10 +0530 Subject: [PATCH 8/8] feat: the live verification, and the two defects it found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F12 replaced the human gate the first three releases each ended at, and it earned its keep on the first run. THE APPROVAL DIFF WOULD HAVE SHIPPED DEAD. Request.target arrives RELATIVE to the workspace — `notes.txt`, not `/tmp/x/notes.txt` — so the absolute-only guard never found the file and quietly fell back to showing the proposed content. Every hand-written test passed, because every fixture used an absolute tempdir path. It now resolves against the session's own root, which is not the process's working directory: `io -C ` sets one without changing the other, and a relative name resolved against the wrong one could diff a file the agent never touched. THE OVERLAY CLIPPED THE FACT IT EXISTS TO SHOW. Repeating the path on the diff row meant that at eighty columns with a long path, ratatui cut the row — silently, as it always does — and what went was `+2 -0`. That is 0.2.0's lesson arriving a second time in the same overlay. The row now leads with the counts and omits the path, which the question row above already carries, and every line is fitted rather than left to be clipped. Two more the live output showed, both readability rather than correctness. A step line printed its tool call as raw escaped JSON directly under a cell that had just said the same thing legibly; the names are kept and the arguments dropped, and 0.1.1's F5 still reads as decision, then what it ran, then what came back. And a tool with no path argument arrived as `git_diff git_diff`, because io-harness falls the target back to the tool's own name. tests/surfaces.rs covers the three surfaces that had no test of their own: a commit leaves the viewport at its fixed height while the content reaches the terminal, a hunk past MAX_BODY_LINES says how many lines it cut, Ctrl+T leaves the composer holding what was typed, the transcript labels a branched-away turn, and the OSC 52 bytes that leave the process decode back to the payload while the message never claims success. --- src/app.rs | 14 +- src/approval.rs | 130 +++++++++--- src/events.rs | 31 ++- src/main.rs | 3 + tests/approval.rs | 33 ++- tests/live.rs | 306 +++++++++++++++++++++++++-- tests/narrow.rs | 12 +- tests/surfaces.rs | 527 ++++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 998 insertions(+), 58 deletions(-) create mode 100644 tests/surfaces.rs diff --git a/src/app.rs b/src/app.rs index e757021..76f7de1 100644 --- a/src/app.rs +++ b/src/app.rs @@ -82,6 +82,12 @@ pub struct App { /// without this a *this session* answer would ask again on the next prompt. /// F5 asserts it on the policy handed to the next turn. remembered: Vec, + /// The workspace this session is held over. + /// + /// Kept so an approval can resolve a write's target: the harness sends it + /// relative to the workspace, and the process's working directory is not the + /// same thing under `io -C `. + root: std::path::PathBuf, /// How much of a change a diff shows. From `[app.io-cli]`, defaulting to /// unified, which is what every file written before 0.3.0 means. diff_style: crate::settings::DiffStyle, @@ -106,11 +112,17 @@ impl App { pending: Vec::new(), approval: None, remembered: Vec::new(), + root: std::path::PathBuf::new(), diff_style: crate::settings::DiffStyle::default(), posture: None, } } + /// Say which workspace this session is held over. + pub fn set_root(&mut self, root: impl Into) { + self.root = root.into(); + } + /// Say how much of a change a diff should show. Read from the harness's own /// configuration by the driver; never parsed here. pub fn set_diff_style(&mut self, style: crate::settings::DiffStyle) { @@ -123,7 +135,7 @@ impl App { /// scrolled away from a run which is blocked on it, which is what F1 asserts /// and the reason this surface is an overlay at all. pub fn open_approval(&mut self, ask: Ask) { - self.approval = Some(Approval::new(ask)); + self.approval = Some(Approval::new(ask, &self.root)); } /// The posture in force. diff --git a/src/approval.rs b/src/approval.rs index 07472a5..69b3dd6 100644 --- a/src/approval.rs +++ b/src/approval.rs @@ -71,9 +71,12 @@ impl Ask { &self.request.target } - /// What a write would leave behind, whole. The harness hands an approver the - /// resulting file rather than a patch, so anything diff-shaped is this - /// product's to compute — and is 0.3.0's, not this release's. + /// What a write would leave behind, whole. + /// + /// The harness hands an approver the resulting file rather than a patch, so + /// the old side has to come from somewhere else — 0.3.0 reads it off disk and + /// hands both to `Edit::with_hunk`, which is the harness's own renderer. See + /// [`diff_of`]. pub fn content(&self) -> Option<&str> { self.request.content.as_deref() } @@ -231,8 +234,19 @@ pub struct Approval { } impl Approval { - pub fn new(ask: Ask) -> Self { - let proposed = diff_of(&ask); + /// Open an overlay for a question, resolving a write's target against + /// `root`. + /// + /// The root is required rather than inferred, and a live run is what settled + /// that. `Request.target` arrives **relative to the workspace** — `notes.txt`, + /// not `/tmp/x/notes.txt` — so an implementation that only read absolute + /// paths never found the file and quietly fell back to showing the proposed + /// content, which is a feature that ships looking like it works. Resolving + /// against this process's working directory instead would be worse: `io -C + /// ` sets the workspace without changing it, so a relative name could + /// match a different file that happens to exist here. + pub fn new(ask: Ask, root: &std::path::Path) -> Self { + let proposed = diff_of(&ask, root); // Opens on `Once`: the least committal answer, and never on a remembered // allow. A surface that opens on the widest answer is one where `Enter` // by reflex gives away the most. @@ -414,20 +428,48 @@ impl Approval { /// The write as a diff, in the rows there are. /// - /// The cell's own header comes first and that is deliberate rather than - /// redundant with the question row above it: at the tightest size the overlay - /// has one row here, and `+3 -1` is the single most useful thing an approver - /// can be told in one row. It is the difference between a write that touches - /// three lines and one that rewrites four hundred, which is a different - /// decision. The hunk itself fills whatever rows are left. + /// **The counts come first and the path does not come at all.** The + /// transcript's version of this cell leads with the path, because there a diff + /// arrives with nothing above it. Here the question row directly above already + /// names the target, and repeating it cost a whole assertion: at eighty + /// columns with a long path, ratatui clipped the row — silently, as it always + /// does — and what went was `+2 -0`, the one fact worth having when there is + /// only one row. That is 0.2.0's lesson arriving a second time in the same + /// overlay, and the answer is the same: put the load-bearing fact where it + /// cannot be the part that goes. + /// + /// At the tightest size that one row is `+3 -1`, which is the difference + /// between a write that touches three lines and one that rewrites four + /// hundred — a different decision, not a smaller one. fn as_diff(&self, edit: &Edit, room: usize, width: usize, theme: &Theme) -> Vec> { - let width = u16::try_from(width).unwrap_or(u16::MAX); - let mut lines = crate::diff::cell(edit, theme, width); - // `cell` ends with a blank line so a transcript breathes. An overlay four - // rows tall cannot spend one on breathing. - if lines.last().is_some_and(|line| line.width() == 0) { - lines.pop(); - } + let mut lines = vec![Line::from(vec![ + Span::styled(format!(" +{}", edit.lines_added), theme.style(Tone::Added)), + Span::styled(" ".to_string(), theme.style(Tone::Muted)), + Span::styled( + format!("-{}", edit.lines_removed), + theme.style(Tone::Removed), + ), + Span::styled( + match &edit.hunk { + Some(_) => format!("{SEPARATOR}{}", edit.tool), + // Absent is a fact and not an empty diff — the counts beside + // it are what say the file did change. + None => format!("{SEPARATOR}{}{SEPARATOR}no diff stored", edit.tool), + }, + theme.style(Tone::Muted), + ), + ])]; + + // Past the cell's own header, which the row above replaces, and without + // the blank line it ends with: an overlay four rows tall cannot spend one + // on breathing. Every line is fitted, because this viewport clips. + lines.extend( + crate::diff::cell(edit, theme, u16::try_from(width).unwrap_or(u16::MAX)) + .into_iter() + .skip(1) + .filter(|line| line.width() > 0) + .map(|line| fit_line(line, width, theme)), + ); let total = lines.len(); let shown = total.min(room); @@ -497,25 +539,18 @@ impl Approval { /// overlay shows the proposed content plainly. Diffing against an empty old /// side here would show every existing line as new, which is a lie about the /// size of the change and exactly the wrong direction to be wrong in. -fn diff_of(ask: &Ask) -> Option { +fn diff_of(ask: &Ask, root: &std::path::Path) -> Option { if ask.act() != Act::Write { return None; } let after = ask.content()?; let target = ask.target(); - // **Absolute only.** A relative target would resolve against this process's - // working directory, and `io -C ` sets the workspace root without - // changing it — so a relative path could name a *different* file that - // happens to exist here, and the overlay would then show a confident diff - // against something the agent never touched. In practice the harness hands - // an approver the policy's resolved target, which is absolute; when it does - // not, showing the proposed content plainly is the honest answer. - if !std::path::Path::new(target).is_absolute() { - return None; - } + // `join` with an absolute target returns the target, so one line covers both + // shapes — and the relative one is what a real run actually sends. + let path = root.join(target); - let before = match std::fs::read_to_string(target) { + let before = match std::fs::read_to_string(&path) { Ok(before) => before, Err(error) if error.kind() == std::io::ErrorKind::NotFound => String::new(), Err(_) => return None, @@ -524,6 +559,41 @@ fn diff_of(ask: &Ask) -> Option { Some(Edit::measure(0, "write_file", target, &before, after).with_hunk(&before, after)) } +/// Cut a rendered line to `width`, keeping its styles and saying it was cut. +/// +/// The overlay draws into a fixed-height viewport, where ratatui **clips** a row +/// that does not fit rather than wrapping it — and clips it silently. A diff line +/// is the widest thing this surface draws, so without this a hunk at eighty +/// columns loses its right-hand end with nothing on screen to say so. +/// +/// Spans are kept whole while they fit and the one that crosses the edge is cut, +/// so syntax colour survives as far as the line does. +fn fit_line(line: Line<'static>, width: usize, theme: &Theme) -> Line<'static> { + if line.width() <= width { + return line; + } + // One cell for the marker that says something went. + let room = width.saturating_sub(1); + let mut spans: Vec> = Vec::new(); + let mut used = 0; + for span in line.spans { + let length = span.content.chars().count(); + if used + length <= room { + used += length; + spans.push(span); + continue; + } + let take = room - used; + if take > 0 { + let cut: String = span.content.chars().take(take).collect(); + spans.push(Span::styled(cut, span.style)); + } + break; + } + spans.push(Span::styled("…".to_string(), theme.style(Tone::Muted))); + Line::from(spans) +} + pub fn act_word(act: Act) -> &'static str { match act { Act::Read => "read", diff --git a/src/events.rs b/src/events.rs index 0215d69..57bc8e8 100644 --- a/src/events.rs +++ b/src/events.rs @@ -260,7 +260,10 @@ impl Events { let mut spans = vec![Span::styled(decision.clone(), theme.style(Tone::Normal))]; if !tool_call.is_empty() { spans.push(Span::styled(SEPARATOR, theme.style(Tone::Muted))); - spans.push(Span::styled(tool_call.clone(), theme.style(Tone::Accent))); + spans.push(Span::styled( + tool_names(tool_call), + theme.style(Tone::Accent), + )); } // Always said, in both directions. A result that appears only // sometimes is a column a reader cannot skim down, and `changed` @@ -453,7 +456,11 @@ fn cell_line(theme: Theme, call: &Pending, result: &str, at: Option) - Span::styled(" ⋅ ", theme.style(Tone::Muted)), Span::styled(call.name.clone(), theme.style(Tone::Accent)), ]; - if !call.target.is_empty() { + // `!= call.name` because io-harness falls the target back to the tool's own + // name when the call carries no path, pattern or key — so a `git_diff` with + // no argument arrived as `git_diff git_diff`, which reads like a stutter. + // Found in a live run, not in the suite. + if !call.target.is_empty() && call.target != call.name { spans.push(Span::styled( format!(" {}", call.target), theme.style(Tone::Muted), @@ -562,6 +569,26 @@ pub fn outcome_help(outcome: &str) -> Option<&'static str> { } } +/// The tools a step called, by name. +/// +/// `Step.tool_call` is `name:arguments` per call, joined by `" | "`, and the +/// arguments are raw JSON. 0.1.1 put the whole thing on the step line, which was +/// the best available then — a reader had nothing else saying what ran. A live +/// run of 0.3.0 showed what it costs now that tool cells exist: every step +/// printed its arguments as a wall of escaped JSON directly under a cell that had +/// just said the same thing readably. +/// +/// So the names are kept and the arguments are dropped. 0.1.1's F5 asks that a +/// step read as decision, then what it ran, then what came back, and it still +/// does — the tool cell above it is where the arguments live now. +fn tool_names(tool_call: &str) -> String { + tool_call + .split(" | ") + .map(|call| call.split_once(':').map_or(call, |(name, _)| name)) + .collect::>() + .join(", ") +} + /// The snake-case name of a kind, taken from its `Debug` form. /// /// `EventKind` is `#[non_exhaustive]` with no accessor for its own tag, and its diff --git a/src/main.rs b/src/main.rs index 2f54fef..a69572f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -247,6 +247,9 @@ async fn loop_over( ) -> Result<(), String> { let mut app = App::new(theme, model); app.set_diff_style(diff_style); + // Asked of the session rather than threaded down from `run`, so there is one + // answer to "which workspace is this" and it is the harness's. + app.set_root(session.root()); // What the file already says, read back rather than assumed. `None` means the // file holds a policy that is none of the three, which io-harness's own // configuration can express and this session must not relabel. diff --git a/tests/approval.rs b/tests/approval.rs index 6084539..e232b8d 100644 --- a/tests/approval.rs +++ b/tests/approval.rs @@ -200,7 +200,7 @@ async fn f2_the_overlay_states_act_then_target_then_rule_then_layer() { let (request, context) = flagged(); let (ask, deciding) = asked(request, context).await; - let approval = Approval::new(ask); + let approval = Approval::new(ask, std::path::Path::new("")); let (mut screen, _recorder) = support::screen(80, 24); screen .draw(|frame| approval.render(frame, frame.area(), &DARK)) @@ -237,7 +237,7 @@ async fn f2_the_overlay_states_act_then_target_then_rule_then_layer() { async fn f2_an_unnamed_action_says_the_tier_default_decided() { let (ask, deciding) = asked(write(), ApprovalContext::new("tidy the parser")).await; - let approval = Approval::new(ask); + let approval = Approval::new(ask, std::path::Path::new("")); let (mut screen, _recorder) = support::screen(80, 24); screen .draw(|frame| approval.render(frame, frame.area(), &DARK)) @@ -253,11 +253,19 @@ async fn f2_an_unnamed_action_says_the_tier_default_decided() { deciding.await.expect("the approver did not panic"); } -/// The proposed content is what makes an answer an informed one, and it is the +/// What a write would do is what makes an answer an informed one, and it is the /// only part that can be arbitrarily long. It is fitted into the rows that are /// left and says how many it did not show. +/// +/// **Restated in 0.3.0.** Until then the overlay showed the proposed content as +/// plain lines, so this asserted the first of them was on screen. Now a write +/// whose target can be read is shown as a change, and at a session's four rows +/// the one row available carries `+40 -855` rather than the first line of the +/// file — which is the more useful row, and the one this test now pins. What has +/// not changed is the part that mattered: whatever is not shown is counted out +/// loud. #[tokio::test] -async fn the_overlay_shows_the_proposed_content_and_says_what_it_cut() { +async fn the_overlay_says_what_it_did_not_show() { let long: String = (0..40).map(|n| format!("line {n}\n")).collect(); let (ask, deciding) = asked( Request::new(Act::Write, "src/main.rs").with_content(long), @@ -265,17 +273,23 @@ async fn the_overlay_shows_the_proposed_content_and_says_what_it_cut() { ) .await; - let approval = Approval::new(ask); + let approval = Approval::new(ask, std::path::Path::new("")); let (mut screen, _recorder) = support::screen(80, 24); screen .draw(|frame| approval.render(frame, frame.area(), &DARK)) .expect("frame"); let text = screen.viewport_text(); - assert!(text.contains("line 0"), "the content is shown: {text:?}"); assert!( text.contains("more lines"), - "what was cut has to be said, or the reader is approving a file they think they read: {text:?}", + "what was cut has to be said, or the reader is approving a file they think \ + they read: {text:?}", + ); + // The size of the change, which is the decision. `src/main.rs` exists, so + // this is a real diff against a real file rather than an all-addition wall. + assert!( + text.contains('+') && text.contains('-'), + "the counts are the row that always survives: {text:?}", ); approval.answer(approval::Answer::Deny); @@ -496,7 +510,10 @@ async fn overlay_for(target: &std::path::Path, content: &str, height: u16) -> St }); let ask = asks.recv().await.expect("the question arrived"); - let approval = Approval::new(ask); + // An empty root leaves an absolute target resolving to itself, which is what + // these fixtures use; the relative case is covered by the live run, which is + // where it was found. + let approval = Approval::new(ask, std::path::Path::new("")); // `height` is the VIEWPORT's height, which is what the overlay is drawn // into — not the terminal's. A session's is four; a taller one is what a // reader gets on a terminal with room, and both are worth asserting. diff --git a/tests/live.rs b/tests/live.rs index 9f3cef1..6a5410a 100644 --- a/tests/live.rs +++ b/tests/live.rs @@ -21,6 +21,8 @@ //! driven through the real bridge and the real event renderer, editing a real //! file inside the sandbox. +mod support; + use std::sync::{Arc, Mutex}; use io_cli::events::Events; @@ -252,9 +254,11 @@ async fn live_f10_a_real_turn_stops_and_asks_before_it_writes() { let seen = Arc::new(Mutex::new(Vec::new())); let answering = tokio::spawn({ let seen = Arc::clone(&seen); + // Owned, because the task outlives this borrow of the tempdir. + let root = root.to_path_buf(); async move { while let Some(ask) = asks.recv().await { - let approval = Approval::new(ask); + let approval = Approval::new(ask, &root); let question = approval.ask(); let note = format!( "{} {} — rule {:?}, layer {:?}, {} bytes of content", @@ -344,13 +348,16 @@ async fn live_f10_a_denial_leaves_the_file_alone() { events: Arc::clone(&collected), }; - let answering = tokio::spawn(async move { - let mut denied = 0usize; - while let Some(ask) = asks.recv().await { - Approval::new(ask).answer(Answer::Deny); - denied += 1; + let answering = tokio::spawn({ + let root = root.to_path_buf(); + async move { + let mut denied = 0usize; + while let Some(ask) = asks.recv().await { + Approval::new(ask, &root).answer(Answer::Deny); + denied += 1; + } + denied } - denied }); let result = session @@ -431,15 +438,18 @@ async fn live_allowing_for_the_session_stops_the_reasking_inside_one_turn() { events: Arc::clone(&collected), }; - let answering = tokio::spawn(async move { - let mut targets = Vec::new(); - while let Some(ask) = asks.recv().await { - let approval = Approval::new(ask); - targets.push(approval.ask().target().to_string()); - println!("asked about {}", approval.ask().target()); - approval.answer(Answer::Session); + let answering = tokio::spawn({ + let root = root.to_path_buf(); + async move { + let mut targets = Vec::new(); + while let Some(ask) = asks.recv().await { + let approval = Approval::new(ask, &root); + targets.push(approval.ask().target().to_string()); + println!("asked about {}", approval.ask().target()); + approval.answer(Answer::Session); + } + targets } - targets }); let result = session @@ -479,3 +489,269 @@ async fn live_allowing_for_the_session_stops_the_reasking_inside_one_turn() { targets[0], ); } + +// =========================================================================== +// F12 — a real turn against a live provider edits a file, and every surface +// this release ships is exercised on it. +// +// This replaced the human gate the first three releases each ended at. The +// owner withdrew manual release testing on 2026-08-17; see +// `.ultraship/iterations/US-IO-CLI-0.3.0-I01.yaml`. What a gate proved and this +// cannot is whether a diff is *pleasant* to read, and the release record says so +// rather than implying otherwise. +// +// What it does prove is the thing no assertion over a hand-written hunk can: +// that the hunk io-harness ACTUALLY stores for a real edit, by a real model, +// through the real tool layer, is the one this renderer draws. +// =========================================================================== + +/// The whole cell, as a reader would see it. +fn as_text(lines: &[ratatui::text::Line<'static>]) -> String { + lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>() + .join("\n") +} + +#[tokio::test] +#[ignore = "live: needs OPENROUTER_API_KEY"] +async fn live_f12_a_real_edit_renders_as_the_hunk_the_harness_stored() { + let key = key(); + let dir = tempfile::tempdir().expect("a workspace"); + let root = dir.path(); + // Several lines, so a one-line edit produces a hunk with real context and + // real `@@` numbers rather than a whole-file replacement that would look the + // same however it was computed. + std::fs::write( + root.join("greeting.rs"), + "fn one() {}\nfn two() {}\nfn three() {}\nfn four() {}\nfn five() {}\n\ + fn six() {}\nfn seven() {}\nfn eight() {}\n", + ) + .expect("the fixture file"); + + let store = Store::open(root.join("runs.db")).expect("a store"); + let mut session = Session::open(&store, root).expect("a session"); + let provider = io_harness::OpenRouter::new(&key, model()); + let policy = workspace_policy(); + let (_steer, inbox) = Steer::channel(); + let collected = Arc::new(Mutex::new(Vec::new())); + let observer = Collector { + events: Arc::clone(&collected), + }; + + let result = session + .turn_steered( + "In greeting.rs, change only the body of `fn five` so that it reads \ + `fn five() { println!(\"five\"); }`. Change nothing else. Then say done.", + &provider, + &store, + &policy, + &DenyAll, + &observer, + &inbox, + ) + .await + .expect("the turn runs"); + + println!("outcome: {:?} run {}", result.outcome, result.run_id); + + // ---- F1/F2/F3/F4: the diff, from the store, for a real edit ------------- + let edits = store.edits(result.run_id).expect("the edits are readable"); + println!("{} edit(s) recorded", edits.len()); + assert!( + !edits.is_empty(), + "the model did not edit the file, so this proves nothing about the diff. \ + outcome was {:?}", + result.outcome, + ); + + for edit in &edits { + let text = as_text(&io_cli::diff::cell(edit, &DARK, 120)); + println!("--- step {} · {} ---\n{text}\n---", edit.step, edit.tool); + + assert!( + text.contains(&edit.path), + "the path is on the header: {text}" + ); + match &edit.hunk { + Some(hunk) => { + // The load-bearing assertion of the whole release: what is on + // screen is the harness's stored text, not something io-cli + // recomputed. Every body line survives, in order. + for line in hunk.lines() { + assert!( + text.contains(line), + "the rendered cell dropped a line the store holds: {line:?}", + ); + } + assert!( + text.contains("@@"), + "a stored hunk carries the file's own line numbers: {text}", + ); + } + // Honest about the other case rather than failing on it: an absent + // hunk has three causes and none of them is "nothing changed". + None => assert!( + text.contains("no diff stored"), + "an absent hunk has to say so: {text}", + ), + } + } + + // ---- F11: the whole run as one patch, over OSC 52 ----------------------- + let patch = store.patch(result.run_id).expect("the patch is readable"); + assert!( + !patch.trim().is_empty(), + "a run that edited a file has a patch" + ); + let sequence = io_cli::clipboard::sequence(&patch); + assert!(sequence.starts_with("\x1b]52;c;"), "OSC 52 is malformed"); + assert!(sequence.ends_with('\x07'), "OSC 52 is unterminated"); + let described = io_cli::clipboard::describe(&patch); + println!("clipboard: {described}"); + for claim in ["copied", "success", "done"] { + assert!( + !described.to_lowercase().contains(claim), + "nothing acknowledges an OSC 52 write, so {claim:?} is a claim this \ + product cannot support: {described}", + ); + } + + // ---- F10: the conversation, back into the scrollback -------------------- + let transcript = session.transcript(&store).expect("the transcript reads"); + let rendered = as_text(&io_cli::transcript::lines(&transcript, &DARK)); + println!("--- Ctrl+T ---\n{rendered}\n---"); + assert!( + rendered.contains("greeting.rs") || rendered.contains("five"), + "the transcript should carry what was asked: {rendered}", + ); + + // ---- F5: the expander reads the detail back out of the trace ------------ + let steps = store.steps(result.run_id).expect("the steps read"); + println!("{} step(s) in the trace", steps.len()); + assert!( + !steps.is_empty(), + "a run that edited a file recorded steps, and /expand reads them back", + ); + + // ---- the tool cells, through the real renderer -------------------------- + let events = collected.lock().expect("not poisoned").clone(); + let mut renderer = Events::new(DARK); + let mut committed = Vec::new(); + for (nth, event) in events.iter().enumerate() { + // A distinct age per event, stated by the test rather than measured, so + // each cell's duration is arithmetic this file chose. No clock is read + // here or anywhere under `tests/`. + committed.extend(renderer.event(event, std::time::Duration::from_millis(nth as u64 * 100))); + } + committed.extend(renderer.flush()); + let text = as_text(&committed); + println!("--- as the interface renders it ---\n{text}\n---"); + + let announced = events + .iter() + .filter(|event| matches!(event.kind, EventKind::ToolCall { .. })) + .count(); + println!("{announced} tool call(s) announced"); + if announced > 0 { + assert!( + text.contains("~"), + "a committed tool cell carries the interface's observed duration, \ + marked as an observation: {text}", + ); + } +} + +#[tokio::test] +#[ignore = "live: needs OPENROUTER_API_KEY"] +async fn live_f12_an_approval_shows_the_write_as_a_diff_against_the_real_file() { + use io_cli::approval::{self, Approval}; + + let key = key(); + let dir = tempfile::tempdir().expect("a workspace"); + let root = dir.path(); + let target = root.join("notes.txt"); + std::fs::write(&target, "one\ntwo\nthree\nfour\nfive\n").expect("the fixture"); + + let store = Store::open(root.join("runs.db")).expect("a store"); + let mut session = Session::open(&store, root).expect("a session"); + let provider = io_harness::OpenRouter::new(&key, model()); + // The posture whose whole point is that it asks. + let policy = Policy { + layers: Policy::default().layers, + defaults: Posture::AskWrites.defaults(), + }; + let (_steer, inbox) = Steer::channel(); + let (asker, mut asks) = approval::channel(); + + // Answer whatever is asked, and render the overlay for the first write on the + // way past. The run stays paused for exactly as long as the `Ask` is held. + let drawn = Arc::new(Mutex::new(Vec::::new())); + let seen = Arc::clone(&drawn); + let answering = tokio::spawn({ + let root = root.to_path_buf(); + async move { + while let Some(ask) = asks.recv().await { + let approval = Approval::new(ask, &root); + // A viewport with room, so the hunk itself is exercised and not only + // the counts row a four-row session would leave. + let (mut screen, _recorder) = support::screen_of(100, 20, 12); + screen + .draw(|frame| approval.render(frame, frame.area(), &DARK)) + .expect("frame"); + seen.lock() + .expect("not poisoned") + .push(screen.viewport_text().to_string()); + approval.answer(approval::Answer::Once); + } + } + }); + + let result = session + .turn_steered( + "Change the third line of notes.txt from `three` to `THREE`. \ + Change nothing else. Then say done.", + &provider, + &store, + &policy, + &asker, + &io_harness::Ignore, + &inbox, + ) + .await + .expect("the turn runs"); + drop(asker); + answering.await.expect("the answering task did not panic"); + + println!("outcome: {:?}", result.outcome); + let overlays = drawn.lock().expect("not poisoned").clone(); + for overlay in &overlays { + println!("--- approval overlay ---\n{overlay}\n---"); + } + assert!( + !overlays.is_empty(), + "the ask-before-writes posture did not ask, so the approval diff was \ + never exercised. outcome was {:?}", + result.outcome, + ); + + // The counts are the row that always survives, and they are the decision: + // a write that touches one line is not the same decision as one that + // rewrites the file. + let any = overlays.join("\n"); + assert!( + any.contains('+') && any.contains('-'), + "the overlay showed no change at all: {any}", + ); + assert!( + !any.contains("+one") || any.contains("-one"), + "an unchanged line was drawn as an addition, which means the old side \ + was empty and the write reads as a whole-file rewrite: {any}", + ); +} diff --git a/tests/narrow.rs b/tests/narrow.rs index a7cce35..950d017 100644 --- a/tests/narrow.rs +++ b/tests/narrow.rs @@ -109,7 +109,7 @@ async fn n5_the_approval_overlay_fits_eighty_columns() { }); let ask = asks.recv().await.expect("the question arrived"); - let approval = Approval::new(ask); + let approval = Approval::new(ask, std::path::Path::new("")); let (mut screen, _recorder) = support::screen(WIDTH, HEIGHT); screen .draw(|frame| approval.render(frame, frame.area(), &DARK)) @@ -130,7 +130,15 @@ async fn n5_the_approval_overlay_fits_eighty_columns() { ); // And so must the act and the target, in some form. assert!(viewport.contains("write"), "{viewport:?}"); - assert!(viewport.contains('…'), "something was fitted: {viewport:?}"); + // Something was cut and the overlay said so. Two markers can carry that: `…` + // when a single value was shortened to fit, and `⋯` when whole rows of a + // change were left out. 0.3.0 made the second the usual one here — the + // overlay's diff row leads with the counts rather than repeating the path, so + // the path no longer needs shortening and the rows do. + assert!( + viewport.contains('…') || viewport.contains('⋯'), + "something was cut and nothing said so: {viewport:?}", + ); // The two facts no other core records must survive the narrow form. A widget // that "fits" by letting ratatui clip the row has not fitted anything — it has // silently cut the half of the sentence this release exists to show. diff --git a/tests/surfaces.rs b/tests/surfaces.rs new file mode 100644 index 0000000..f52190c --- /dev/null +++ b/tests/surfaces.rs @@ -0,0 +1,527 @@ +//! F5, F10 and F11 — the three surfaces that answer "show me more" and "give me +//! this text", asserted at the boundary each of them actually crosses. +//! +//! The three share one premise and it is the reason they are in one file: **the +//! viewport is four rows and never grows**. Everything that shows more of +//! something is therefore written *upward*, into the terminal's own scrollback, +//! where the terminal's search, selection and copy-mode already work — and +//! everything that hands text to the reader's own machine goes out as an escape +//! sequence, which is neither scrollback nor viewport. A regression in any of the +//! three looks the same from inside a rendered buffer: nothing. So the assertions +//! below are made against the bytes that leave the process and against the +//! viewport buffer at rest, never against a value the renderer handed itself. +//! +//! Order matters on a rendered line and is asserted by position rather than by +//! membership. A line that reads `turn 2 · left behind by a branch · do it with a +//! blue-green cutover` contains every substring an inside-out line contains, and +//! a `contains` assertion is exactly as green for it. + +mod support; + +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; +use io_cli::app::{App, Command}; +use io_cli::clipboard; +use io_cli::commands::{parse, Action, Copied}; +use io_cli::diff::MAX_BODY_LINES; +use io_cli::term::VIEWPORT_HEIGHT; +use io_cli::theme::DARK; +use io_cli::transcript::{self, BRANCHED_AWAY}; +use io_harness::provider::{CompletionRequest, CompletionResponse}; +use io_harness::{ApproveAll, Edit, Policy, Provider, Session, Store}; +use ratatui::text::Line; +use ratatui::widgets::Paragraph; + +// --------------------------------------------------------------------------- +// F5 — expansion commits upward, and the viewport never grows. +// --------------------------------------------------------------------------- + +/// Far more rows than the terminal under test has, so a commit that tried to +/// make room by growing the viewport would have to grow it past the screen. +const COMMITTED_ROWS: usize = 200; + +/// What the viewport draws while the commit is happening. Deliberately shares no +/// word with the committed lines, so "the viewport holds only this" is a real +/// assertion rather than a coincidence of vocabulary. +const COMPOSER: &str = "> the prompt being typed"; + +#[test] +fn f5_a_large_commit_reaches_the_terminal_and_leaves_the_viewport_at_its_fixed_height() { + // 80x24 on purpose: the smallest supported terminal, where a viewport that + // grew by two hundred rows could not even be drawn. The failure this prevents + // is a "show me more" path that opens a pane instead of committing — which on + // this renderer means the transcript stops being the terminal's own buffer and + // starts being something this process has to redraw and can lose. + let (mut screen, recorder) = support::screen(80, 24); + let before = screen.terminal_mut().get_frame().area().height; + assert_eq!( + before, VIEWPORT_HEIGHT, + "the fixture is only meaningful if the viewport starts at its fixed height", + ); + + let body: Vec> = (0..COMMITTED_ROWS) + .map(|row| Line::from(format!("expanded detail row {row}"))) + .collect(); + screen.commit(&body).expect("a commit of two hundred rows"); + screen + .draw(|frame| { + let area = frame.area(); + frame.render_widget(Paragraph::new(COMPOSER), area); + }) + .expect("a frame after the commit"); + + assert_eq!( + screen.terminal_mut().get_frame().area().height, + before, + "the viewport grew to hold committed content; it is fixed at {VIEWPORT_HEIGHT} rows \ + and everything that shows more has to go above it", + ); + + // The content reached the terminal — which is what "in the scrollback" means + // for a renderer that never owns the rows above its viewport. + let text = recorder.text(); + for row in [0, COMMITTED_ROWS / 2, COMMITTED_ROWS - 1] { + let needle = format!("expanded detail row {row}"); + assert!( + text.contains(&needle), + "{needle:?} never left the process, so it is in no scrollback", + ); + } + + // And none of it is in the viewport, which holds what the frame drew and + // nothing else. This is the half that fails when expansion is rendered into + // the viewport instead of committed above it. + let viewport = screen.viewport_text(); + assert!( + viewport.contains(COMPOSER), + "the viewport should hold the composer, but it holds {viewport:?}", + ); + assert!( + !viewport.contains("expanded detail row"), + "committed content is still in the live viewport: {viewport:?}", + ); +} + +/// The `@@` header plus this many additions, which is far past [`MAX_BODY_LINES`] +/// and is what makes the cap observable at all. +const HUNK_ADDITIONS: usize = 900; + +/// A hunk taller than the cap, on a path syntect has a grammar for, so the cell +/// under test is the one a real edit produces rather than a shortest path through +/// the renderer. +fn tall_edit() -> Edit { + let mut hunk = format!("@@ -1,{HUNK_ADDITIONS} +1,{HUNK_ADDITIONS} @@\n"); + for row in 0..HUNK_ADDITIONS { + hunk.push_str(&format!("+row {row} of a very large change\n")); + } + Edit { + step: 1, + tool: "write_file".to_string(), + path: "notes.txt".to_string(), + lines_added: HUNK_ADDITIONS as u64, + lines_removed: 0, + hunk: Some(hunk), + } +} + +#[test] +fn f5_a_hunk_past_the_cap_is_bounded_and_says_how_many_lines_it_cut() { + let edit = tall_edit(); + let body_rows = edit + .hunk + .as_deref() + .expect("the fixture has a hunk") + .lines() + .count(); + assert!( + body_rows > MAX_BODY_LINES, + "the fixture only tests the cap if it exceeds it: {body_rows} rows against {MAX_BODY_LINES}", + ); + + let rendered = rows(&io_cli::diff::cell(&edit, &DARK, 120)); + + // Bounded. The cap exists because highlighting runs on the loop that also + // delivers keystrokes and because `insert_before` takes a `u16` height: an + // uncapped cell makes `Ctrl+C` unreachable for as long as the parse takes. + assert!( + rendered.len() < body_rows, + "the whole hunk was drawn — {} lines — so the cap did nothing", + rendered.len(), + ); + assert!( + rendered.len() <= MAX_BODY_LINES + 3, + "the cell is {} lines; the cap allows {MAX_BODY_LINES} body rows plus a header, \ + the notice and the trailing blank", + rendered.len(), + ); + + // And it says what it cut. A cap that truncates silently is the defect: the + // reader cannot tell a change that ended from a change that was cut off, and + // the part they cannot see is the part they went looking for. + let cut = body_rows - MAX_BODY_LINES; + let counted = cut.to_string(); + let notice_at = rendered + .iter() + .rposition(|row| !row.trim().is_empty()) + .expect("a cell has at least a header"); + let notice = &rendered[notice_at]; + assert!( + notice.contains(format!("{cut} more lines").as_str()), + "the last line of a capped cell must count what it cut, and this one reads {notice:?}", + ); + + // By position within the line: the count comes before the words that say + // where the rest of it went. A line reading "the whole of it is in the trace + // — 401 more lines" contains both and answers the reader's question in the + // wrong order. + let count_at = notice + .find(counted.as_str()) + .expect("the count is on the line"); + let trace_at = notice + .find("trace") + .expect("the notice says where the rest of the change is"); + assert!( + count_at < trace_at, + "the cut count must lead the line: {notice:?}", + ); + + // The notice is at the end of the body, under the rows it applies to, rather + // than a header nobody reads in context. + assert!( + rendered[notice_at - 1].contains("row "), + "the notice should sit under the last drawn row, but follows {:?}", + rendered[notice_at - 1], + ); +} + +// --------------------------------------------------------------------------- +// F10 — Ctrl+T commits the whole conversation, branched-away turns included. +// --------------------------------------------------------------------------- + +/// What is half-typed when the transcript key is pressed. +const HALF_TYPED: &str = "a thought i was in the middle of"; + +#[test] +fn f10_ctrl_t_asks_for_the_transcript_and_leaves_the_composer_holding_what_was_typed() { + let mut app = App::new(DARK, "opus-5"); + for character in HALF_TYPED.chars() { + app.key(KeyEvent::new(KeyCode::Char(character), KeyModifiers::NONE)); + } + + let command = app.key(KeyEvent::new(KeyCode::Char('t'), KeyModifiers::CONTROL)); + assert_eq!( + command, + Command::Transcript, + "Ctrl+T is the one key that puts the conversation back into the scrollback", + ); + + // The failure this prevents: a key that reaches for the whole conversation and + // eats the prompt on the way, so looking something up costs the sentence being + // written. Nothing about committing the transcript touches the composer, and + // this is what keeps it that way. + assert_eq!( + app.composer.text(), + HALF_TYPED, + "the composer lost what was typed when the transcript was asked for", + ); +} + +/// The three prompts the branched session is built from. The second is the one +/// the branch leaves behind, and it is the only turn in the fixture that +/// `Session::history` would no longer return. +const PLAN: &str = "sketch the cutover"; +const BRANCHED: &str = "take the whole service down for it"; +const KEPT: &str = "keep it up and drain the queue instead"; + +/// Answers every turn with one line and calls nothing, so the turns are real and +/// their content is not. +struct Talker; + +impl Provider for Talker { + async fn complete( + &self, + _request: CompletionRequest, + ) -> io_harness::Result { + Ok(CompletionResponse { + text: Some("here is the sketch".into()), + ..Default::default() + }) + } +} + +async fn say(session: &mut Session, store: &Store, prompt: &str) -> i64 { + session + .turn(prompt, &Talker, store, &Policy::permissive(), &ApproveAll) + .await + .expect("a scripted turn cannot fail") + .turn_id +} + +/// A real session of three turns with the middle one branched away. +/// +/// Built rather than stated: `io_harness::Transcript` and `TranscriptTurn` are +/// both `#[non_exhaustive]`, so neither can be constructed by a struct literal +/// from outside the harness. A hand-made value would also agree with the renderer +/// by construction, which is the opposite of what F10 needs to know — the +/// question is whether the renderer agrees with what the *database* recorded. +async fn branched() -> (tempfile::TempDir, io_harness::Transcript) { + let dir = tempfile::tempdir().expect("a temp dir"); + let store = Store::memory().expect("an in-memory store"); + let mut session = Session::open(&store, dir.path()).expect("a session"); + + let first = say(&mut session, &store, PLAN).await; + say(&mut session, &store, BRANCHED).await; + session + .branch_from(&store, first) + .expect("the first turn is branchable"); + say(&mut session, &store, KEPT).await; + + let transcript = session.transcript(&store).expect("a transcript"); + (dir, transcript) +} + +#[tokio::test] +async fn f10_the_transcript_shows_the_branched_away_turn_and_labels_it_after_its_own_prompt() { + let (_dir, transcript) = branched().await; + assert_eq!( + transcript.turns.iter().filter(|turn| turn.on_path).count(), + 2, + "the fixture is only meaningful if one of the three turns is off the path", + ); + + let rendered = rows(&transcript::lines(&transcript, &DARK)); + + // Rendered at all. This turn is the one thing in the whole product that only + // this surface can show, and a turn that was dropped looks exactly like a turn + // that never happened. + let branched = row_with(&rendered, BRANCHED); + assert!( + branched.contains(BRANCHED_AWAY), + "the branched-away turn must say so in words — a tone is gone under NO_COLOR \ + and gone again once the line is copied out of the terminal: {branched}", + ); + + // By position, not by membership: content, then the branch label, then the id. + // A row carrying all three in any other order passes a `contains` and reads + // inside out, with metadata in the column a reader skims for what was said. + let prompt_at = branched.find(BRANCHED).expect("the prompt is on its row"); + let label_at = branched + .find(BRANCHED_AWAY) + .expect("the label is on its row"); + let id_at = branched + .find("turn ") + .expect("the turn id rides the same row"); + assert!( + prompt_at < label_at && label_at < id_at, + "content, then the label, then the id: {branched}", + ); + + // And the turn the model can still see carries no label. Without this half, + // a renderer that labelled every row would pass everything above. + let kept = row_with(&rendered, KEPT); + assert!( + !kept.contains(BRANCHED_AWAY), + "a turn still on the path must not be labelled: {kept}", + ); +} + +// --------------------------------------------------------------------------- +// F11 — /copy writes OSC 52 and claims nothing about the clipboard. +// --------------------------------------------------------------------------- + +#[test] +fn f11_copy_resolves_to_the_answer_and_copy_diff_and_copy_patch_both_to_the_patch() { + assert_eq!( + parse("copy", &DARK), + Action::Copy(Copied::Answer), + "a bare /copy is the last answer, which is what a reader who just read one wants", + ); + // Two spellings of one thing, because a reader who has just been shown a diff + // types the word they were shown. A parser that knows only one of them fails + // silently into "copy the answer", which puts the wrong text on the clipboard + // rather than reporting that the command was not understood. + for spelling in ["copy diff", "copy patch"] { + assert_eq!( + parse(spelling, &DARK), + Action::Copy(Copied::Diff), + "/{spelling} must reach the patch", + ); + } +} + +/// The text put on the clipboard. It holds spaces, so it cannot appear inside the +/// base64 form of itself — which is what makes "the plaintext never left the +/// process" an assertion rather than a hope. +const PAYLOAD: &str = "the answer the agent gave, verbatim"; + +#[test] +fn f11_the_bytes_that_leave_the_process_are_an_osc_52_sequence_carrying_the_payload() { + let (mut screen, recorder) = support::screen(80, 24); + screen + .escape(&clipboard::sequence(PAYLOAD)) + .expect("the sequence is written"); + + // Asserted over the bytes rather than over `sequence`'s return value: what is + // under test is what the terminal emulator at the far end of an SSH connection + // receives, and a renderer that built the right string and never flushed it + // would satisfy every assertion made against the string. + let bytes = recorder.bytes(); + let introducer = b"\x1b]52;c;"; + let start = find(&bytes, introducer) + .unwrap_or_else(|| panic!("no OSC 52 introducer in {:?}", recorder.text())) + + introducer.len(); + let end = start + + bytes[start..] + .iter() + .position(|byte| *byte == 0x07) + .expect("the sequence is terminated by BEL, which the whole xterm lineage accepts"); + + let encoded = std::str::from_utf8(&bytes[start..end]).expect("base64 is ASCII"); + assert_eq!( + decode(encoded), + PAYLOAD.as_bytes(), + "the payload does not survive the round trip, so the reader's paste is not what they copied", + ); + + // The encoding is not decoration: an OSC string ends at the first BEL or ST, + // and a transcript can carry both. A payload written raw would terminate its + // own sequence early and print the remainder of itself on screen. + assert!( + !encoded.contains(['\x07', '\x1b']), + "the encoded middle carries a byte that ends an OSC string early", + ); +} + +#[test] +fn f11_the_message_states_a_size_and_never_claims_the_clipboard_was_set() { + // Two and a half thousand bytes, so the size is a stated fact with a unit + // rather than a number that could have come from anywhere. + let payload = "x".repeat(2_500); + let said = clipboard::describe(&payload); + assert!( + said.contains("2.5 kB"), + "the message must state the size — it is the one fact that makes a paste that came \ + up empty diagnosable against a terminal's own limit: {said}", + ); + + // No terminal answers an OSC 52 write. tmux drops it without `set -g + // set-clipboard on` and several emulators cap the payload silently, so a + // success word here is a claim this process never observed, printed at exactly + // the moment the reader goes to paste and finds the old contents. + let lowered = said.to_lowercase(); + for claim in ["copied", "success", "done"] { + assert!( + !lowered.contains(claim), + "{claim:?} claims an acknowledgement that does not exist on the wire: {said}", + ); + } +} + +#[test] +fn f11_the_clipboard_sequence_goes_to_the_terminal_and_not_into_the_scrollback() { + let (mut screen, recorder) = support::screen(80, 24); + screen + .escape(&clipboard::sequence(PAYLOAD)) + .expect("the sequence is written"); + screen + .draw(|frame| { + let area = frame.area(); + frame.render_widget(Paragraph::new(COMPOSER), area); + }) + .expect("a frame after the sequence"); + + // An OSC 52 write is a message to the terminal emulator, not content for the + // screen. Routed through `commit` it would become a line of the transcript the + // reader scrolls past — and, because `commit` renders through a widget, the + // escape bytes would be drawn as text rather than acted on. The payload's + // plaintext holds spaces, which base64 never emits, so its absence here is + // proof it was not committed. + let text = recorder.text(); + assert!( + !text.contains(PAYLOAD), + "the payload reached the terminal as text, which means it was committed rather \ + than sent as an escape sequence", + ); + assert!( + text.contains("\x1b]52;c;"), + "the sequence itself never left the process", + ); + + let viewport = screen.viewport_text(); + assert!( + !viewport.contains(PAYLOAD) && !viewport.contains("52;c;"), + "the clipboard sequence is in the viewport: {viewport:?}", + ); +} + +// --------------------------------------------------------------------------- +// Helpers. +// --------------------------------------------------------------------------- + +/// Every rendered line as its own string, spans concatenated. +/// +/// The renderer's unit is the line and every assertion above is about what a +/// single line holds and in what order. Joining the whole output into one blob +/// would make "on the same line" and "somewhere in the output" the same question. +fn rows(lines: &[Line<'_>]) -> Vec { + lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect() +} + +/// The one row holding `needle`, or a failure naming what was rendered instead. +fn row_with<'a>(rows: &'a [String], needle: &str) -> &'a String { + rows.iter() + .find(|row| row.contains(needle)) + .unwrap_or_else(|| panic!("nothing rendered for {needle:?}; got {rows:#?}")) +} + +/// Where `needle` starts in `haystack`, over bytes rather than text — the byte +/// stream holds escape sequences that are not valid on a `char` boundary basis to +/// search through as a string. +fn find(haystack: &[u8], needle: &[u8]) -> Option { + haystack + .windows(needle.len()) + .position(|window| window == needle) +} + +/// Standard base64, the `+/` alphabet of RFC 4648 — the one every terminal +/// emulator implements. Spelled out here rather than imported from the crate +/// under test: decoding with the encoder's own table would assert only that the +/// table is self-consistent, and a URL-safe `-_` payload decodes to different +/// bytes in a real terminal rather than failing loudly. +const ALPHABET: &[u8; 64] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; + +/// Decode a base64 string back to the bytes it was made from. +fn decode(encoded: &str) -> Vec { + let mut bits: u32 = 0; + let mut held = 0; + let mut out = Vec::new(); + for byte in encoded.bytes() { + if byte == b'=' { + break; + } + let index = ALPHABET + .iter() + .position(|character| *character == byte) + .unwrap_or_else(|| { + panic!( + "{:?} is not a standard base64 character, so a terminal would decode this \ + sequence to something other than what was copied", + byte as char, + ) + }); + bits = (bits << 6) | index as u32; + held += 6; + if held >= 8 { + held -= 8; + out.push((bits >> held) as u8); + } + } + out +}