From f6ada6de84f2071f1aaec6d157d6058e4df3a13c Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Fri, 28 Aug 2026 16:51:56 +0530 Subject: [PATCH 1/8] feat(store): what the run store holds, and the four operations that change it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/store` commits a page carrying the file's own page arithmetic — bytes on disk, bytes already free inside it, the session and run counts and the per-table breakdown — with each session's own size beside it. `/store rm `, `/store sweep ` and `/store compact` change it, each behind a confirmation whose row 0 declines. Every figure is io-harness's. `Store::store_size`, `session_size`, `delete_session`, `sweep_sessions` and `compact` have all been public since before this product had a home to keep a store in, and none of them was called anywhere in this crate. The distinction the module exists to make is that a deletion does not shrink the file: SQLite frees pages into the database rather than out of it, so a removal moves bytes from `file_bytes` into `free_bytes` and the file on disk stays the size it was. A `VACUUM` is the only reclamation available, because every store this crate has created was created without `auto_vacuum`. The report says so, and the gate asserts `file_bytes` is unchanged by a removal rather than merely no larger. The sweep asks for the rule rather than for a count. There is no public reader for `sessions.created_at`, the column it filters on, so the set a date selects cannot be known in advance — and the one available substitute, a session's earliest turn, is strictly later than the session itself and would under-state a deletion. Filed as io-harness#216; the figures are reported from `Pruned` once the sweep has run, refused ids included. `store::acts` puts the confirmation's one decision in the library, because `src/main.rs` is a binary nothing under `tests/` can link. Also corrects a stale comment: `src/main.rs` claimed `/skills` confirms with "leave it" at row 0. It does the opposite, and it is the only confirmation in the product that does. --- README.md | 1 + src/commands.rs | 69 +++++ src/lib.rs | 1 + src/main.rs | 160 ++++++++++- src/stats.rs | 8 +- src/store.rs | 656 ++++++++++++++++++++++++++++++++++++++++++++++ tests/commands.rs | 13 +- tests/store.rs | 564 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 1467 insertions(+), 5 deletions(-) create mode 100644 src/store.rs create mode 100644 tests/store.rs diff --git a/README.md b/README.md index 8473a02..35297de 100644 --- a/README.md +++ b/README.md @@ -529,6 +529,7 @@ above a row that ranked there for reasons having nothing to do with it. | `/skills` | every skill, shipped or yours: what it is for, whether it is on, and its file | | `/cost` | commit what this run, this session and this install have spent | | `/stats` | commit how the runs have gone: outcomes, first-try, gates, latency | +| `/store` | commit what the run store holds; `rm `, `sweep ` and `compact` change it | **configure** diff --git a/src/commands.rs b/src/commands.rs index c460680..02d3ef3 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -269,6 +269,14 @@ pub const COMMANDS: &[(&str, &str)] = &[ "/stats", "commit how the runs have gone: outcomes, first-try, gates, latency", ), + // 0.27.0 — the third page about work already done, and the first one that can + // also change it. The description names the three verbs rather than only the + // page, because a command whose bare form reports and whose argued forms + // delete is one an operator must be able to tell apart from the row. + ( + "/store", + "commit what the run store holds; `rm `, `sweep ` and `compact` change it", + ), // Beside `/stats` because `/stats` is the only other row that says the word, // and the two are the halves of one thing: that page counts how the gates // went, and until this release nothing in the product could say what a gate @@ -438,6 +446,14 @@ pub const GROUPS: &[(Group, &[&str])] = &[ Group::Inspect, &[ "/help", "/status", "/context", "/expand", "/fleet", "/skills", "/cost", "/stats", + // 0.27.0 — the two that answer a question about work that has already + // happened rather than about the turn in flight, which is what this + // group means. `/store` is what the run store is holding; `/export` + // is that work taken somewhere else. They take `Inspect` to ten, + // which is the bound — so the next command that would fill this group + // re-files one that is in the wrong group rather than widening it, + // exactly as `Turn` did for `/undo` in this same release. + "/store", ], ), // **`/mcp` and `/provider` moved here in 0.19.0, and it is a correction rather @@ -1214,6 +1230,35 @@ pub enum Action { /// that caused them. [`crate::gates::Refusal`] names both while the operator /// is still looking at what they typed. Gates, + /// Report what the run store holds, or change it. + /// + /// `None` is the page and changes nothing. Every `Some` is a verb, and every + /// verb that acts descends into a confirmation first — see [`Keep`], and + /// [`crate::store`] for why a removal and a compaction are different + /// questions with different costs. + Store(Option), +} + +/// What `/store` was asked to do, once the page is not the answer. +/// +/// The three refusal variants are variants rather than an `Err`, because they +/// are answers this surface gives the operator in its own words. An unparseable +/// verb must never fall through to the page: somebody who typed a delete and got +/// a report would believe the delete had happened. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Keep { + /// Remove one session, by id. + Remove(i64), + /// Remove every session created strictly before this timestamp. + Sweep(String), + /// Return the free pages inside the file to the filesystem. + Compact, + /// `rm` with no id, or an id that is not a number. + NoId, + /// `sweep` with no date. + NoDate, + /// A word this command does not know. + Unknown(String), } /// What `/copy` was asked for. @@ -1860,6 +1905,30 @@ pub fn parse(input: &str, keys: &Keys, theme: &Theme) -> Action { "usage" => Action::Cost, "cost" => Action::Cost, "stats" => Action::Stats, + // Three verbs behind one word, and the bare form is the only one that + // does not change anything. A rejected verb reports rather than falling + // through to the page: an operator who typed `/store swep 2026-08-01` + // meant to delete something, and quietly showing them a report would + // leave them believing they had. + "store" => { + let mut rest = input.split_whitespace().skip(1); + match rest.next() { + None => Action::Store(None), + Some("rm" | "remove" | "delete") => match rest.next().map(str::parse::) { + Some(Ok(id)) => Action::Store(Some(Keep::Remove(id))), + // A missing or unparseable id is named rather than defaulted. + // There is no sensible default session to delete, and the one + // that looks sensible — the current one — is the worst. + _ => Action::Store(Some(Keep::NoId)), + }, + Some("sweep") => match rest.next() { + Some(date) => Action::Store(Some(Keep::Sweep(date.to_string()))), + None => Action::Store(Some(Keep::NoDate)), + }, + Some("compact" | "vacuum") => Action::Store(Some(Keep::Compact)), + Some(word) => Action::Store(Some(Keep::Unknown(word.to_string()))), + } + } "config" | "settings" => { let mut rest = input.split_whitespace().skip(1); match rest.next() { diff --git a/src/lib.rs b/src/lib.rs index 7c16be9..aae1c30 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -101,6 +101,7 @@ pub mod splash; pub mod stats; pub mod status; pub mod stdin; +pub mod store; pub mod term; pub mod theme; pub mod transcript; diff --git a/src/main.rs b/src/main.rs index 1bb4f93..4620bf2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1580,7 +1580,19 @@ async fn loop_over Result>( } } // Row 0 is "leave it", the default that does nothing — - // the shape `/skills` and `/plugin` both already use. + // the shape `/plugin` and `/provider` also use. + // + // **Corrected in 0.27.0: this comment named `/skills`, + // and `/skills` is the one confirmation in this product + // that does the opposite** — `Pick::SkillToggle` puts the + // verb at row 0 and "leave it as it is" at row 1. That is + // defensible there and only there, because a toggle is + // reversible by repeating it and nothing is destroyed; + // every confirmation that removes something puts the + // declining row first. The comment was load-bearing + // enough to be believed and repeated while `/store` was + // designed, which is why it is corrected rather than + // deleted. // Row 2 is the edit verb, and it descends rather than // acting: which key is half the decision, and the other // half is a value only the operator can type. @@ -2478,6 +2490,66 @@ async fn loop_over Result>( listing is not shown", ), }, + // The three store confirmations, and all three share one + // rule: **index 0 is `store::LEAVE_IT` and does nothing**. + // Asserted on the index rather than on the label, because + // a confirmation whose acting row drifted to the top is + // the defect F5 exists to catch and a label comparison + // would not see it. + Pick::StoreRemove { id } => { + if io_cli::store::acts(index) { + match io_cli::store::remove(&store, *id) { + Ok(removed) => { + for line in io_cli::store::removed_report(&removed) { + app.record(Tone::Muted, line); + } + } + Err(error) => app.record( + Tone::Error, + format!( + "session {id} was not removed: {}", + io_cli::failure::said(&error) + ), + ), + } + } + } + Pick::StoreSweep { date } => { + if io_cli::store::acts(index) { + match io_cli::store::sweep(&store, date) { + Ok(swept) => { + for line in io_cli::store::swept_report(&swept) { + app.record(Tone::Muted, line); + } + } + Err(error) => app.record( + Tone::Error, + format!( + "the sweep did not run: {}", + io_cli::failure::said(&error) + ), + ), + } + } + } + Pick::StoreCompact => { + if io_cli::store::acts(index) { + match io_cli::store::compact(&store) { + Ok(freed) => { + for line in io_cli::store::freed_report(&freed) { + app.record(Tone::Muted, line); + } + } + Err(error) => app.record( + Tone::Error, + format!( + "the store was not compacted: {}", + io_cli::failure::said(&error) + ), + ), + } + } + } } // `None` in every arm but the descent, so this closes the // picker exactly as it always did and replaces it in the one @@ -3622,6 +3694,73 @@ async fn loop_over Result>( let lines = io_cli::stats::committed(&store, &app.theme, screen.width())?; screen.commit(&lines).map_err(|error| error.to_string())?; } + // The bare word reports and nothing else; every verb that changes + // the store descends into a confirmation whose row 0 does + // nothing. The reads that build each confirmation happen HERE, + // before the operator agrees, so the figures they are shown are + // the ones the operation is about to act on rather than a second + // reading taken afterwards. + Action::Store(None) => { + let lines = io_cli::store::committed(&store, &app.theme, screen.width())?; + screen.commit(&lines).map_err(|error| error.to_string())?; + } + Action::Store(Some(keep)) => match keep { + commands::Keep::Remove(id) => match io_cli::store::sized(&store, id) { + Ok(sized) => { + let (title, rows) = io_cli::store::confirm_remove(id, &sized); + picker = Some((Picker::new(title, rows), Pick::StoreRemove { id })); + } + Err(error) => app.record( + Tone::Error, + format!( + "the store could not be read: {}", + io_cli::failure::said(&error) + ), + ), + }, + // No read at all before this one, because there is nothing + // readable: the set a date selects cannot be counted in + // advance — see io-harness#216 and `US-IO-CLI-0.27.0-I02`. + // The operator agrees to the rule and the report carries the + // figures. + commands::Keep::Sweep(date) => { + let (title, rows) = io_cli::store::confirm_sweep(&date); + picker = Some((Picker::new(title, rows), Pick::StoreSweep { date })); + } + commands::Keep::Compact => match store.store_size() { + Ok(size) => { + let (title, rows) = io_cli::store::confirm_compact(&size); + picker = Some((Picker::new(title, rows), Pick::StoreCompact)); + } + Err(error) => app.record( + Tone::Error, + format!( + "the store could not be read: {}", + io_cli::failure::said(&error) + ), + ), + }, + // Three refusals, each naming what was missing. None of them + // falls through to the page: an operator who typed a deletion + // and was shown a report would believe the deletion happened. + commands::Keep::NoId => app.record( + Tone::Error, + "/store rm needs a session id — `/store` lists them".to_string(), + ), + commands::Keep::NoDate => app.record( + Tone::Error, + "/store sweep needs a date, as the store writes them: \ + `/store sweep 2026-08-01`" + .to_string(), + ), + commands::Keep::Unknown(word) => app.record( + Tone::Error, + format!( + "/store does not know `{word}` — the verbs are `rm `, \ + `sweep ` and `compact`" + ), + ), + }, // Reached only at an idle prompt: while a turn runs the driver's // own key handler answers `/steer` before `parse` is ever called, // because that is where the inbox lives. So this arm is the @@ -5729,6 +5868,25 @@ enum Pick { at: io_cli::providers::At, first: bool, }, + /// A confirmation over one session's removal. Row 0 is `store::LEAVE_IT` and + /// every other row acts — the shape `/mcp` remove established and the one + /// criterion F5 asserts by *index*, because a confirmation whose default + /// keystroke destroys something is the defect it exists to prevent. + StoreRemove { + /// The session the confirmation named, carried rather than re-read: the + /// figures the operator agreed to were read against this id. + id: i64, + }, + /// A confirmation over a date sweep, carrying the boundary the operator was + /// shown. The counts are not here because they cannot be — see + /// io-harness#216. + StoreSweep { + /// The timestamp the sweep compares `sessions.created_at` against. + date: String, + }, + /// A confirmation over a compaction. Carries nothing: the operation takes no + /// argument, and the figures it reports are read either side of the call. + StoreCompact, /// One directory of the workspace, in the order `list_dir` sorted it, so a /// chosen index reads straight back through `complete::pick`. The rows are /// last components rather than paths — see `complete::rows` for why — which diff --git a/src/stats.rs b/src/stats.rs index 26d5e3f..2f275c5 100644 --- a/src/stats.rs +++ b/src/stats.rs @@ -236,7 +236,13 @@ fn tallies(tallies: Vec, empty: &str) -> Vec { /// /// Powers of 1024 and named as such: this is a file on a disk, and a reader /// comparing it against what `ls` said should not have to convert. -fn bytes(count: u64) -> String { +/// +/// **Public since 0.27.0 so [`crate::store`] can reuse it rather than write a +/// second one.** Two spellings of the same quantity on two pages of one +/// interface is the shape 0.25.0 recorded when one fact acquired two holders — +/// and a store page reporting `8.2 MB` beside a stats page reporting `8.2 MiB` +/// is that defect in its most readable form. +pub fn bytes(count: u64) -> String { const UNITS: [(&str, u64); 4] = [ ("GiB", 1024 * 1024 * 1024), ("MiB", 1024 * 1024), diff --git a/src/store.rs b/src/store.rs new file mode 100644 index 0000000..78d0a97 --- /dev/null +++ b/src/store.rs @@ -0,0 +1,656 @@ +//! What the run store is holding, and the four things an operator can do about it. +//! +//! io-harness has kept every session, run, step, event, provider call, snapshot +//! and restore point in one SQLite file since this product had a home to put it +//! in — `~/.io-cli/runs.db` since 0.15.0, across twenty-six releases. Until this +//! one there was no retention policy, no rotation, no reclamation, and **no way +//! to look at it at all**. A file that only ever grows is a cost this product +//! created; this module is where it is finally answerable. +//! +//! Everything here is io-harness's arithmetic. [`Store::store_size`], +//! [`Store::session_size`], [`Store::delete_session`], [`Store::sweep_sessions`] +//! and [`Store::compact`] are all public and all uncalled anywhere else in this +//! crate. Nothing in this module computes a byte count, and nothing re-derives a +//! figure the store already returns — the one number io-cli must never invent is +//! the one an operator is about to authorise a deletion against. +//! +//! It follows the shape of [`crate::recall`] and [`crate::sessions`]: take a +//! `&Store`, return owned data, let the caller draw it. Nothing here touches a +//! terminal and nothing here reads a clock. +//! +//! # The distinction the whole module exists to make +//! +//! **A deletion does not shrink the file.** SQLite frees pages *into* the +//! database rather than out of it, so [`Store::delete_session`] moves bytes from +//! [`StoreSize::file_bytes`] into [`StoreSize::free_bytes`] and the file on disk +//! stays exactly the size it was. [`Store::compact`] — a `VACUUM` — is the only +//! reclamation available, because every store this crate has ever created was +//! created without `auto_vacuum` and `PRAGMA incremental_vacuum` therefore does +//! nothing on any existing file. +//! +//! An interface that reported a deletion as having reclaimed space would be +//! lying in precisely the way this release exists to stop, which is why +//! [`Freed`] carries both figures and why `tests/store.rs` asserts `file_bytes` +//! is **unchanged** by a removal rather than asserting it merely did not grow. +//! +//! # The four things that make a naive reading wrong +//! +//! **1. `None` and `Some(zeros)` are different answers.** [`Store::session_size`] +//! answers `None` for a session the store does not have and `Some` with zeros for +//! one that exists and holds nothing. io-harness keeps them apart on purpose — +//! `state/sessions.rs:305-309` says why: an operator sweeping a list of ids needs +//! to know which of them were already gone. [`Sized`] preserves the distinction; +//! mapping `None` to a zeroed [`SessionSize`] would tell that operator every id +//! was empty rather than that half of them no longer exist. +//! +//! **2. The sweep refuses, and the refusal is the interesting half.** A session +//! holding a run that can still be resumed is **not deleted**, and its id comes +//! back in [`Pruned::refused`]. io-harness's reasoning (`state/sessions.rs:370-378`) +//! is that a date is a policy applied to sessions nobody looked at, and a +//! crash-resumable tree that vanished because it was old is the worst outcome the +//! call could have. So a report that shows only what went tells the operator they +//! swept sessions that are still sitting there. +//! +//! **3. There is no public reader for `sessions.created_at`, which is the column +//! the sweep filters on.** `Store::sweep_sessions` selects +//! `WHERE created_at < ?1` (`state/sessions.rs:429`) and nothing in io-harness +//! returns that value — only `root` and `head_turn_id` are readable off the +//! `sessions` row. So the set a date selects **cannot be counted before the sweep +//! runs**, and the nearest substitute is worse than none: a session's earliest +//! `Turn::created_at` is always *later* than the session's own, because +//! `Session::open` writes the session row when the process starts and a turn is +//! written when the operator types. A preview built on turns therefore +//! under-states the deletion, which is the direction that costs somebody data. +//! +//! Filed as io-harness#216. Shipped around by [`Swept`]: the operator authorises +//! the **rule**, which is what the operation is, and every figure they are shown +//! afterwards is one io-harness returned. See `US-IO-CLI-0.27.0-I02`. +//! +//! **4. Memory survives a session deletion, and its recall rows do not.** +//! io-harness's entries belong to the workspace rather than to the session +//! (`state/sessions.rs:368-373`), so removing a session unlearns nothing — but +//! the session's **restore points go**, and their count is in +//! [`Pruned::restore_points`]. That is the sentence an operator needs before they +//! agree, because it is the one that bites later: they delete an old session, +//! and reach for its rewind a week afterwards. +//! +//! # What is deliberately not here +//! +//! No automatic retention, rotation, sweeping or compaction. No startup sweep, no +//! size threshold, no configuration key that turns any of it on. The roadmap's +//! own words are *none of which happens on its own*, and io-harness makes the +//! argument itself: a call that rewrites the whole database and needs the file's +//! size again in free space while it runs is one an operator makes knowingly. +//! +//! And **nothing here is reachable by a model.** io-harness's workspace tool set +//! contains nothing that can call any of these, this release adds no tool, no MCP +//! server and no skill, and `tests/dependencies.rs` asserts that each of the three +//! destructive calls is named in exactly one module — this one. + +use io_harness::{Error, Pruned, SessionSize, Store, StoreSize}; + +/// The size of one session, or the fact that there is no such session. +/// +/// Two variants and not an `Option` at the call site, because the +/// two answers need different sentences and a bare `Option` invites the caller to +/// `unwrap_or_default()` — which is exactly the collapse trap 1 describes. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Sized { + /// The session is in the store. Zeros here mean it holds nothing, which is a + /// fact rather than a gap. + Holds(SessionSize), + /// There is no session of that id. Not an error, and not an empty session. + Absent, +} + +impl Sized { + /// The figures, when there are any. + pub fn size(&self) -> Option<&SessionSize> { + match self { + Sized::Holds(size) => Some(size), + Sized::Absent => None, + } + } +} + +/// One row of the store page: a session and what it costs. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Session { + /// The session id, which is what every operation here addresses. + pub id: i64, + /// The workspace root the session was opened over, as the store holds it. + pub root: String, + /// What it costs, or that it is gone. Read per session rather than derived + /// from a total, so a row and the operation that acts on it agree. + pub sized: Sized, +} + +/// Everything a store panel needs, and nothing it has to compute. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct View { + /// The file's own page arithmetic: `page_size × page_count` for + /// [`StoreSize::file_bytes`] and `page_size × freelist_count` for + /// [`StoreSize::free_bytes`], plus the session and run counts and the + /// per-table breakdown. Read from [`Store::store_size`] and never assembled + /// here. + pub size: StoreSize, + /// One row per session the store holds, in the order [`crate::sessions`] + /// returns them. + pub rows: Vec, + /// The session scan hit [`crate::sessions::MAX_RUNS_SCANNED`], so [`View::rows`] + /// is a prefix rather than the whole store. The totals in [`View::size`] are + /// unaffected — they come from the file, not from the scan — which is exactly + /// why a cut list beside a complete total has to say so. + pub cut: bool, +} + +impl View { + /// The part of the file that is already free inside it. + /// + /// Named rather than left to a caller's subtraction because the interesting + /// quantity is the one nobody thinks to ask for: bytes a deletion has already + /// released that are still occupying disk, and that only [`compact`] returns. + pub fn reclaimable(&self) -> u64 { + self.size.free_bytes + } +} + +/// Read what the store is holding. +/// +/// Every row is a read. This function cannot write to the store. +/// +/// The session list comes from [`crate::sessions::recent`], which is the list +/// this product already shows for `/resume` — so a session an operator sees here +/// is one they can recognise there, and the scan bound is the same one. Each +/// row's size is read individually, because a total divided among rows would be +/// io-cli inventing an attribution io-harness did not make. +pub fn view(store: &Store) -> Result { + let size = store.store_size()?; + let (recent, cut) = crate::sessions::recent(store)?; + let mut rows = Vec::with_capacity(recent.len()); + for session in recent { + rows.push(Session { + id: session.id, + root: session.root, + sized: sized(store, session.id)?, + }); + } + Ok(View { size, rows, cut }) +} + +/// What one session costs, keeping "absent" apart from "empty". +/// +/// See trap 1. The mapping is the whole function and it is the point of it. +pub fn sized(store: &Store, session_id: i64) -> Result { + Ok(match store.session_size(session_id)? { + Some(size) => Sized::Holds(size), + None => Sized::Absent, + }) +} + +/// What removing one session did, and what it did **not** do to the file. +/// +/// Both halves together, from two reads of [`Store::store_size`] taken either +/// side of the removal, because the fact worth reporting is a relationship +/// between them: rows went, `free_bytes` rose, and the file on disk did not move. +/// A caller handed only [`Pruned`] would have no way to say the third thing, and +/// the third thing is the one that surprises people. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Removed { + /// io-harness's own account of what was deleted. + pub pruned: Pruned, + /// The file before. + pub before: StoreSize, + /// The file after. [`StoreSize::file_bytes`] equals `before`'s; only + /// [`StoreSize::free_bytes`] has moved. + pub after: StoreSize, +} + +impl Removed { + /// Whether the file on disk changed size. Always false, and asserted rather + /// than assumed — if it is ever true, either SQLite's behaviour or this + /// module's understanding of it has changed and the report must not go on + /// claiming otherwise. + pub fn file_moved(&self) -> bool { + self.before.file_bytes != self.after.file_bytes + } + + /// How much more of the file is free than was free before. + pub fn freed_into_file(&self) -> u64 { + self.after.free_bytes.saturating_sub(self.before.free_bytes) + } +} + +/// Remove one session and everything keyed to it. +/// +/// **Final. There is no undelete, and this release does not build one.** What +/// survives is io-harness's decision and is worth knowing before agreeing: the +/// agent's memory entries belong to the workspace and are untouched, while the +/// session's *recall* rows go with it because they name runs that no longer +/// exist, and so do its **restore points** — the count of which is in +/// [`Pruned::restore_points`]. An operator who removes an old session has removed +/// its rewind. +/// +/// Deleting a session the store does not have succeeds and reports nothing, which +/// is io-harness's own behaviour and is not softened here. +pub fn remove(store: &Store, session_id: i64) -> Result { + let before = store.store_size()?; + let pruned = store.delete_session(session_id)?; + let after = store.store_size()?; + Ok(Removed { + pruned, + before, + after, + }) +} + +/// What a date sweep did. +/// +/// The counts live here and **not** in a preview, because they cannot be known +/// before the sweep runs — see trap 3 and io-harness#216. This is the whole of +/// the ship-around: the operator authorises the rule, and this is the answer. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Swept { + /// The boundary that was applied, echoed back so the report says what it is + /// the report *of*. + pub before_date: String, + /// io-harness's own account: what went, and every id it refused. + pub pruned: Pruned, + /// The file before the sweep. + pub before: StoreSize, + /// The file after it. As with [`Removed`], `file_bytes` has not moved. + pub after: StoreSize, +} + +impl Swept { + /// The sessions that were refused for still being resumable. + /// + /// A borrow rather than a count, because the report names them: an operator + /// told "one was refused" has to go looking, and the id is the thing they + /// would go looking for. + pub fn refused(&self) -> &[i64] { + &self.pruned.refused + } +} + +/// Remove every session created strictly before `before_date`. +/// +/// `before_date` is a timestamp string compared against `sessions.created_at`, +/// which is a `strftime('%Y-%m-%dT%H:%M:%fZ')` text column — a string comparison +/// is what the storage actually does, so that is what io-harness takes rather +/// than a duration measured against a clock the store does not have. The +/// comparison is **strictly** before: a session created at exactly `before_date` +/// survives. +/// +/// A session holding a resumable run is refused rather than deleted and comes +/// back in [`Swept::refused`]. That is not an error and must not be reported as +/// one. +pub fn sweep(store: &Store, before_date: &str) -> Result { + let before = store.store_size()?; + let pruned = store.sweep_sessions(before_date)?; + let after = store.store_size()?; + Ok(Swept { + before_date: before_date.to_string(), + pruned, + before, + after, + }) +} + +/// What a compaction actually returned to the filesystem. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Freed { + /// The bytes the file shrank by, **measured** by io-harness as the difference + /// between the file's size before and after — never inferred from the + /// freelist. Zero is an honest answer for a store with nothing free, and the + /// caller says so rather than showing an empty success. + pub bytes: u64, + /// The file before. + pub before: StoreSize, + /// The file after. + pub after: StoreSize, +} + +impl Freed { + /// Whether anything was reclaimed at all. + pub fn is_nothing(&self) -> bool { + self.bytes == 0 + } +} + +/// Rewrite the database without its free pages, and say how much that returned. +/// +/// **This is the only reclamation available.** Every store this crate has created +/// was created without `auto_vacuum`, so `PRAGMA incremental_vacuum` does nothing +/// on any existing file, and a deletion on its own never shrinks anything. +/// +/// **It is expensive and the caller must say so first.** io-harness's own +/// documentation is explicit: it rewrites the whole database, it needs free disk +/// space of roughly the file's own size while it runs, it cannot run inside a +/// transaction, and on a large store it is not quick. That is why it is a call an +/// operator makes knowingly and never something a deletion does on their behalf. +pub fn compact(store: &Store) -> Result { + let before = store.store_size()?; + let bytes = store.compact()?; + let after = store.store_size()?; + Ok(Freed { + bytes, + before, + after, + }) +} + +// --------------------------------------------------------------------------- +// The surfaces +// --------------------------------------------------------------------------- + +/// The row every confirmation starts on: the one that does nothing. +/// +/// Index 0, always, in every confirmation this module builds. A `Picker` opens +/// on its first row, so the keystroke an operator gives by reflex is the one +/// that declines — which is the whole of criterion F5 and the reason +/// `tests/store.rs` asserts the *index* rather than the presence of the row. +pub const LEAVE_IT: &str = "leave it"; + +/// Whether a chosen row in one of these confirmations acts. +/// +/// One line, and it is in the library rather than in the driver on purpose. +/// `src/main.rs` is `[[bin]] name = "io"` and nothing under `tests/` can link it, +/// so a `index != 0` written at the three call sites would be the one decision in +/// this module that could never be tested or sabotaged — which is 0.22.0's +/// recorded lesson (*any decision worth testing has to live in the library*) and +/// 0.23.0's (*all three HIGH defects were in `src/main.rs`*). +/// +/// It is also what makes criterion F5 assertable as a property rather than as a +/// label comparison: row 0 declines, whatever it is called. +pub fn acts(index: usize) -> bool { + index != 0 +} + +/// The `/store` page, committed into the scrollback. +/// +/// Into the scrollback and never into a pane, for the reason `/status`, `/cost` +/// and `/stats` all give: the viewport is eight rows and cannot grow, and the +/// terminal's own search, selection and copy already work on everything above +/// it. This is a page a reader wants to keep beside the sessions it accounts for. +pub fn committed( + store: &Store, + theme: &crate::theme::Theme, + width: u16, +) -> Result>, String> { + let view = view(store).map_err(|e| e.to_string())?; + let mut rows: Vec = vec![ + crate::page::Row::heading("the file".to_string()), + crate::page::Row::fact("on disk", crate::stats::bytes(view.size.file_bytes)), + // Named as free *inside* the file rather than as "reclaimable", because + // the second word invites the reading that a deletion returned it. It did + // not: this is space a `VACUUM` could return, and nothing else can. + crate::page::Row::fact("free inside it", crate::stats::bytes(view.reclaimable())), + crate::page::Row::fact("sessions", view.size.sessions.to_string()), + crate::page::Row::fact("runs", view.size.runs.to_string()), + ]; + + if view.reclaimable() > 0 { + rows.push(crate::page::Row::caveat(format!( + "{} is already free inside the file and still occupies the disk; \ + `/store compact` is the only thing that returns it", + crate::stats::bytes(view.reclaimable()) + ))); + } + + rows.push(crate::page::Row::Blank); + rows.push(crate::page::Row::heading("by table".to_string())); + if view.size.tables.is_empty() { + rows.push(crate::page::Row::note("no tables reported".to_string())); + } + for (table, bytes) in &view.size.tables { + rows.push(crate::page::Row::fact( + table.clone(), + crate::stats::bytes(*bytes), + )); + } + + rows.push(crate::page::Row::Blank); + rows.push(crate::page::Row::heading("by session".to_string())); + if view.rows.is_empty() { + rows.push(crate::page::Row::note("no sessions recorded".to_string())); + } + for session in &view.rows { + rows.push(crate::page::Row::fact( + format!("{} · {}", session.id, session.root), + session_figures(&session.sized), + )); + } + if view.cut { + rows.push(crate::page::Row::caveat(format!( + "only the {} most recent sessions are listed; the totals above are \ + the whole file", + view.rows.len() + ))); + } + + rows.push(crate::page::Row::Blank); + rows.push(crate::page::Row::note( + "`/store rm ` removes one session · `/store sweep ` removes \ + every session created before it · `/store compact` returns the free \ + pages to the disk" + .to_string(), + )); + + Ok(crate::page::commit( + "What the store holds", + &rows, + theme, + width, + )) +} + +/// One session's figures, or the sentence for a session that is not there. +/// +/// The two answers are different sentences rather than one with zeros in it — +/// see trap 1. An operator reading `0 turns` about a session that no longer +/// exists would go looking for an empty session. +fn session_figures(sized: &Sized) -> String { + match sized { + Sized::Holds(size) => format!( + "{} · {} turn{} · {} run{}", + crate::stats::bytes(size.bytes), + size.turns, + if size.turns == 1 { "" } else { "s" }, + size.runs, + if size.runs == 1 { "" } else { "s" }, + ), + Sized::Absent => "no such session".to_string(), + } +} + +/// The confirmation for removing one session: what it holds, and what goes with +/// it. +/// +/// The title carries the figures, because a confirmation is a confirmation of +/// something specific rather than of a keystroke. The restore-point sentence is +/// here rather than in the report for the same reason: it is the fact an +/// operator needs *before* they agree, not the one they discover afterwards. +pub fn confirm_remove(id: i64, sized: &Sized) -> (String, Vec) { + let title = match sized { + Sized::Holds(size) => format!( + "Remove session {id}? {} · {} turn{} · {} run{}", + crate::stats::bytes(size.bytes), + size.turns, + if size.turns == 1 { "" } else { "s" }, + size.runs, + if size.runs == 1 { "" } else { "s" }, + ), + Sized::Absent => format!("There is no session {id}"), + }; + let mut rows = vec![crate::picker::Row::with_detail( + LEAVE_IT, + "nothing is removed", + )]; + if sized.size().is_some() { + rows.push(crate::picker::Row::with_detail( + format!("remove session {id}"), + "final; its restore points go with it, its memory does not", + )); + } + (title, rows) +} + +/// The confirmation for a date sweep: the rule, and the refusal policy. +/// +/// **Not a count.** See trap 3 and io-harness#216: the set a date selects cannot +/// be read before the sweep runs, and the one substitute available under-states +/// it. So the operator authorises the rule — which is what the operation is — +/// and [`swept_report`] carries the figures afterwards. +pub fn confirm_sweep(before_date: &str) -> (String, Vec) { + ( + format!("Sweep every session created before {before_date}?"), + vec![ + crate::picker::Row::with_detail(LEAVE_IT, "nothing is removed"), + crate::picker::Row::with_detail( + "sweep them", + "a session still holding a resumable run is refused and named; \ + the figures are reported once it has run", + ), + ], + ) +} + +/// The confirmation for a compaction: what it costs while it runs. +/// +/// The cost sentence is io-harness's own and is stated before the call rather +/// than discovered by a full disk: a `VACUUM` rewrites the whole database and +/// needs roughly the file's own size free while it does. +pub fn confirm_compact(size: &StoreSize) -> (String, Vec) { + ( + format!( + "Compact the store? {} free inside a {} file", + crate::stats::bytes(size.free_bytes), + crate::stats::bytes(size.file_bytes), + ), + vec![ + crate::picker::Row::with_detail(LEAVE_IT, "the file is left as it is"), + crate::picker::Row::with_detail( + "compact it", + "rewrites the whole database and needs about its own size free \ + on disk while it runs", + ), + ], + ) +} + +/// What to say after a removal. +/// +/// Three sentences, and the third is the one this module exists for: rows went, +/// the free space inside the file rose, and **the file on disk did not move**. +pub fn removed_report(removed: &Removed) -> Vec { + let pruned = &removed.pruned; + let mut lines = vec![format!( + "removed {} session{}: {} turn{}, {} run{}, {} row{}, {}", + pruned.sessions, + if pruned.sessions == 1 { "" } else { "s" }, + pruned.turns, + if pruned.turns == 1 { "" } else { "s" }, + pruned.runs, + if pruned.runs == 1 { "" } else { "s" }, + pruned.rows, + if pruned.rows == 1 { "" } else { "s" }, + crate::stats::bytes(pruned.bytes), + )]; + if pruned.restore_points > 0 { + lines.push(format!( + "{} restore point{} went with it; the workspace's memory did not", + pruned.restore_points, + if pruned.restore_points == 1 { "" } else { "s" }, + )); + } + lines.push(freeing_sentence( + removed.freed_into_file(), + removed.after.file_bytes, + removed.file_moved(), + )); + lines +} + +/// What to say after a sweep — including, always, what it refused. +pub fn swept_report(swept: &Swept) -> Vec { + let pruned = &swept.pruned; + let mut lines = vec![format!( + "swept {} session{} created before {}: {} turn{}, {} run{}, {}", + pruned.sessions, + if pruned.sessions == 1 { "" } else { "s" }, + swept.before_date, + pruned.turns, + if pruned.turns == 1 { "" } else { "s" }, + pruned.runs, + if pruned.runs == 1 { "" } else { "s" }, + crate::stats::bytes(pruned.bytes), + )]; + // Never conditional on there being any. A sweep that refused nothing says so, + // because "refused none" and "we did not check" are the same silence + // otherwise — and the refusal is the half an operator has to act on. + if swept.refused().is_empty() { + lines.push("nothing was refused".to_string()); + } else { + lines.push(format!( + "refused {}: {} still holding a resumable run", + swept + .refused() + .iter() + .map(|id| id.to_string()) + .collect::>() + .join(", "), + if swept.refused().len() == 1 { + "it is" + } else { + "they are" + }, + )); + } + lines.push(freeing_sentence( + swept + .after + .free_bytes + .saturating_sub(swept.before.free_bytes), + swept.after.file_bytes, + swept.before.file_bytes != swept.after.file_bytes, + )); + lines +} + +/// What to say after a compaction. +pub fn freed_report(freed: &Freed) -> Vec { + if freed.is_nothing() { + return vec![format!( + "nothing to reclaim; the file is still {}", + crate::stats::bytes(freed.after.file_bytes) + )]; + } + vec![format!( + "returned {} to the disk; the file is now {}", + crate::stats::bytes(freed.bytes), + crate::stats::bytes(freed.after.file_bytes), + )] +} + +/// The sentence that keeps a deletion honest. +/// +/// `moved` is asserted rather than assumed. It is always false — SQLite frees +/// pages into the file — and if it is ever true then either SQLite's behaviour or +/// this module's understanding of it has changed, and the report must say the +/// surprising thing rather than go on printing the reassuring one. +fn freeing_sentence(freed_into_file: u64, file_bytes: u64, moved: bool) -> String { + if moved { + return format!( + "the file is now {} — it changed size, which a deletion is not \ + supposed to do", + crate::stats::bytes(file_bytes) + ); + } + format!( + "{} more is free inside the file; the file on disk is still {} — \ + `/store compact` is what returns it", + crate::stats::bytes(freed_into_file), + crate::stats::bytes(file_bytes), + ) +} diff --git a/tests/commands.rs b/tests/commands.rs index 2fe1683..0ad9212 100644 --- a/tests/commands.rs +++ b/tests/commands.rs @@ -98,6 +98,12 @@ fn the_commands_are_the_commands() { // this list, which is the rule every alias here follows. "/cost", "/stats", + // 0.27.0 — the third page about work already done, and the first that + // can also change it. Under **inspect** because that is what its bare + // form does, with every destructive verb behind a confirmation whose + // row 0 does nothing; it takes that group to nine and `/export` will + // take it to ten, the bound. + "/store", // 0.24.0 — beside `/stats` because `/stats` is the only other row // that says the word: that page counts how the gates went, and until // this release nothing in the product could say what a gate was. It @@ -309,9 +315,10 @@ fn f12_resume_says_it_answers_a_parked_run_rather_than_merely_reopening_a_sessio // release growing a command and losing another. assert_eq!( COMMANDS.len(), - 33, - "0.26.0 adds exactly one command, `/effort`, on top of the thirty-two \ - 0.25.0 shipped; a thirty-fourth here means one arrived unrecorded", + 34, + "0.27.0 adds `/store` on top of the thirty-three 0.26.0 shipped, and will \ + add `/export` and `/undo` before it is done; a number this gate does not \ + name means one arrived unrecorded", ); } diff --git a/tests/store.rs b/tests/store.rs new file mode 100644 index 0000000..2ffc835 --- /dev/null +++ b/tests/store.rs @@ -0,0 +1,564 @@ +//! What the run store holds, and the four things `/store` can do about it. +//! +//! **The one fact every test here defends: a deletion does not shrink the file.** +//! SQLite frees pages *into* the database rather than out of it, so a removal +//! moves bytes from `StoreSize::file_bytes` into `StoreSize::free_bytes` and the +//! file on disk stays exactly the size it was. An interface that reported a +//! deletion as having reclaimed space would be lying, and it would be a lie an +//! operator only discovers by running out of disk. So `f1` asserts `file_bytes` +//! is **unchanged** — an equality, not an inequality — and `f2` asserts that +//! `compact` is the only call that moves it. +//! +//! **Every store here is a real file in a `TempDir`.** `Store::memory()` cannot +//! be used for any of this: the whole subject is page arithmetic on a file, and +//! a `VACUUM` of a connection that never touched a disk would be testing the half +//! of the claim that was never in doubt. That is also criterion O4 — a gate that +//! opened the developer's own `~/.io-cli/runs.db` and swept it would be data loss +//! shipped as a test, so the last test in this file asserts that no test here +//! names a real home. +//! +//! **What cannot be asserted from this crate, and why it is written down rather +//! than quietly skipped.** F4 describes a session created at exactly the sweep's +//! boundary surviving, because io-harness's comparison is strictly before. That +//! arm is real and it is io-harness's own doctest (`state/sessions.rs:420`); it +//! is **not falsifiable here**, because `sessions.created_at` is written by the +//! store and there is no public reader for it — the same gap as io-harness#216, +//! from the other side. A test that pretended otherwise would be asserting a +//! timestamp this crate invented. The reachable halves are asserted instead: a +//! date before everything sweeps nothing, a date after everything sweeps +//! everything not refused, and the refusal is named. This is 0.23.0's recorded +//! lesson — *check where a sabotage lives before writing the criterion* — showing +//! up on the other side of the same wall. + +use io_cli::store::{ + acts, committed, compact, confirm_compact, confirm_remove, confirm_sweep, freed_report, remove, + removed_report, sized, sweep, swept_report, view, Sized, LEAVE_IT, +}; +use io_harness::Store; + +/// A store in a real file, and the directory that has to outlive it. +/// +/// The `TempDir` comes back with the store because dropping it deletes the +/// database underneath — and because criterion O4 is that no gate in this file +/// can reach anything else. +fn on_disk() -> (tempfile::TempDir, Store) { + let dir = tempfile::tempdir().expect("a temporary directory"); + let store = Store::open(dir.path().join("runs.db")).expect("a store opens on disk"); + (dir, store) +} + +/// One session with `turns` finished turns on it. +/// +/// The run is finished as well as the turn. A run nothing ever finished is +/// `running`, which is a *resumable* run — which is exactly what +/// [`seed_resumable`] exists to produce deliberately, and what this must not +/// produce by accident, or every sweep test would be testing the refusal. +fn seed(store: &Store, root: &str, turns: usize) -> i64 { + let session = store.create_session(root).expect("a session opens"); + for turn in 0..turns { + let prompt = format!("a prompt long enough to occupy some bytes, number {turn}"); + let run = store.start_run(&prompt, root).expect("a run starts"); + let id = store + .record_turn(session, None, run, &prompt) + .expect("a turn records"); + store + .finish_turn(id, Some("an answer of some length"), "completed") + .expect("a turn finishes"); + store + .finish_run(run, "success") + .expect("the run finishes too"); + } + session +} + +/// One session whose run is left `running`, and is therefore still resumable. +/// +/// This is what `Store::sweep_sessions` refuses. io-harness's argument +/// (`state/sessions.rs:370-378`) is that a date is a policy applied to sessions +/// nobody looked at, and a crash-resumable tree that vanished because it was old +/// is the worst outcome the call could have. +fn seed_resumable(store: &Store, root: &str) -> i64 { + let session = store.create_session(root).expect("a session opens"); + let run = store.start_run("unfinished", root).expect("a run starts"); + store + .record_turn(session, None, run, "unfinished") + .expect("a turn records"); + session +} + +/// A timestamp after every session any of these tests can create. +const AFTER_EVERYTHING: &str = "2999-01-01T00:00:00.000Z"; + +/// A timestamp before every session any of these tests can create. +const BEFORE_EVERYTHING: &str = "2000-01-01T00:00:00.000Z"; + +// --------------------------------------------------------------------------- +// F1 — the file's own arithmetic, and what a deletion does not do to it +// --------------------------------------------------------------------------- + +/// **F1 — the page reads io-harness's figures and computes none of its own.** +/// +/// Asserted as an equality against `Store::store_size` rather than by checking +/// the numbers look plausible, because the defect this guards against is io-cli +/// deriving a total from the rows it happens to have listed — which would be +/// right on a small store and wrong on the one an operator came here about. +#[test] +fn f1_the_view_reports_the_stores_own_figures() { + let (_dir, store) = on_disk(); + seed(&store, "/tmp/one", 3); + + let view = view(&store).expect("the store is readable"); + let direct = store.store_size().expect("a size is readable"); + + assert_eq!( + view.size, direct, + "the view must not compute its own totals" + ); + assert_eq!(view.reclaimable(), direct.free_bytes); +} + +/// **F1 — a removal frees pages INTO the file and never shrinks it.** +/// +/// The three assertions are the three halves of the sentence the module exists +/// to make true, and the middle one is an **equality**: `file_bytes` is the same +/// number afterwards, not merely no larger. +#[test] +fn f1_a_removal_frees_pages_into_the_file_and_the_file_does_not_move() { + let (_dir, store) = on_disk(); + let doomed = seed(&store, "/tmp/doomed", 12); + seed(&store, "/tmp/kept", 2); + + let removed = remove(&store, doomed).expect("the session is removed"); + + assert_eq!(removed.pruned.sessions, 1); + assert!(removed.pruned.turns >= 12, "every turn is accounted for"); + assert!(removed.pruned.bytes > 0, "rows carried bytes"); + assert_eq!( + removed.before.file_bytes, removed.after.file_bytes, + "a deletion must not change the size of the file on disk", + ); + assert!( + !removed.file_moved(), + "file_moved() is the same claim and must agree", + ); + assert!( + removed.after.free_bytes >= removed.before.free_bytes, + "the freed pages are inside the file", + ); +} + +/// **F1 — the report says the file did not move, and names the compaction.** +/// +/// A report that only said what was deleted would leave the operator believing +/// they had reclaimed the bytes it named. +#[test] +fn f1_the_report_says_the_file_is_unchanged_and_what_would_change_it() { + let (_dir, store) = on_disk(); + let doomed = seed(&store, "/tmp/doomed", 6); + let removed = remove(&store, doomed).expect("the session is removed"); + + let report = removed_report(&removed).join("\n"); + assert!(report.contains("removed 1 session"), "{report}"); + assert!( + report.contains("still"), + "the report must say the file is still its old size: {report}", + ); + assert!( + report.contains("/store compact"), + "and must name the one thing that changes that: {report}", + ); +} + +// --------------------------------------------------------------------------- +// F2 — only a compaction shrinks the file, and it reports a measured figure +// --------------------------------------------------------------------------- + +/// **F2 — compaction is the only thing that shrinks the file, and the figure it +/// reports is measured.** +/// +/// The returned number is asserted against the difference between the two +/// `store_size` readings rather than against the freelist, because those are +/// different numbers and io-harness deliberately returns the true one. +#[test] +fn f2_only_compact_shrinks_the_file_and_reports_the_measured_difference() { + let (_dir, store) = on_disk(); + let doomed = seed(&store, "/tmp/doomed", 40); + seed(&store, "/tmp/kept", 1); + remove(&store, doomed).expect("the session is removed"); + + let freed = compact(&store).expect("the store compacts"); + + assert!( + freed.after.file_bytes < freed.before.file_bytes, + "a compaction is the one call that shrinks the file", + ); + assert_eq!( + freed.bytes, + freed.before.file_bytes - freed.after.file_bytes, + "the figure reported is the measured shrink, not the freelist", + ); + assert!(!freed.is_nothing()); + + let report = freed_report(&freed).join("\n"); + assert!(report.contains("returned"), "{report}"); +} + +/// **F2 — a store with nothing free reclaims nothing, and says so.** +/// +/// Zero is an honest answer and must not render as an empty success. This is +/// also the arm that stops the report being written as "returned {bytes}" with no +/// branch, which would print `returned 0 B` and read as a failure. +#[test] +fn f2_compacting_a_store_with_nothing_free_returns_zero_and_says_so() { + let (_dir, store) = on_disk(); + seed(&store, "/tmp/kept", 2); + // Compact once to reach a state with nothing free, then again to observe it. + compact(&store).expect("the store compacts"); + + let freed = compact(&store).expect("the store compacts again"); + + assert!(freed.is_nothing(), "nothing was free to reclaim"); + let report = freed_report(&freed).join("\n"); + assert!( + report.contains("nothing to reclaim"), + "zero must be a sentence of its own: {report}", + ); +} + +// --------------------------------------------------------------------------- +// F3 — absent is not empty +// --------------------------------------------------------------------------- + +/// **F3 — a session that is not there and a session that holds nothing are +/// different answers.** +/// +/// io-harness keeps them apart on purpose (`state/sessions.rs:305-309`): an +/// operator sweeping a list of ids needs to know which of them were already gone. +/// Collapsing `None` into a zeroed `SessionSize` tells them every id was empty. +#[test] +fn f3_absent_is_not_the_same_answer_as_empty() { + let (_dir, store) = on_disk(); + let empty = store.create_session("/tmp/empty").expect("a session opens"); + + let held = sized(&store, empty).expect("a size is readable"); + let missing = sized(&store, 9_999).expect("a missing session is not an error"); + + assert!(matches!(held, Sized::Holds(_)), "an empty session exists"); + assert_eq!(held.size().expect("figures").turns, 0, "and holds nothing"); + assert_eq!(missing, Sized::Absent, "a missing session is absent"); + assert!(missing.size().is_none()); +} + +/// **F3 — the two answers produce different confirmations.** +/// +/// The confirmation for a session that is not there offers no acting row at all, +/// because there is nothing to act on. A confirmation that offered "remove it" +/// for a session the store does not have would be a button that cannot work. +#[test] +fn f3_the_confirmation_for_a_missing_session_offers_nothing_to_do() { + let (title, rows) = confirm_remove(9_999, &Sized::Absent); + + assert!(title.contains("no session 9999"), "{title}"); + assert_eq!(rows.len(), 1, "only the declining row"); + assert_eq!(rows[0].label, LEAVE_IT); +} + +/// **F3 — removing a session the store does not have succeeds and reports +/// nothing removed.** +/// +/// io-harness's own behaviour, not softened here: it is not an error, and it must +/// not be reported as one. +#[test] +fn f3_removing_a_session_that_is_not_there_reports_nothing() { + let (_dir, store) = on_disk(); + seed(&store, "/tmp/kept", 1); + + let removed = remove(&store, 9_999).expect("removing nothing is not an error"); + + assert_eq!(removed.pruned.sessions, 0); + assert_eq!(removed.pruned.turns, 0); + assert_eq!(removed.before.file_bytes, removed.after.file_bytes); +} + +// --------------------------------------------------------------------------- +// F4 — the sweep, its refusal, and the rule it asks for +// --------------------------------------------------------------------------- + +/// **F4 — a date before everything sweeps nothing.** +/// +/// The half of "strictly before" that *is* reachable from this crate. Without +/// it, a sweep that ignored its argument entirely would pass every other test in +/// this file. +#[test] +fn f4_a_date_before_everything_sweeps_nothing() { + let (_dir, store) = on_disk(); + seed(&store, "/tmp/one", 2); + seed(&store, "/tmp/two", 2); + + let swept = sweep(&store, BEFORE_EVERYTHING).expect("the sweep runs"); + + assert_eq!(swept.pruned.sessions, 0, "nothing was created before 2000"); + assert!(swept.refused().is_empty()); +} + +/// **F4 — a resumable session is refused, and named.** +/// +/// The refusal is the interesting half: a report that showed only what went would +/// tell the operator they had swept sessions that are still sitting there. +#[test] +fn f4_a_resumable_session_is_refused_and_named() { + let (_dir, store) = on_disk(); + seed(&store, "/tmp/finished", 3); + let alive = seed_resumable(&store, "/tmp/alive"); + + let swept = sweep(&store, AFTER_EVERYTHING).expect("the sweep runs"); + + assert_eq!( + swept.refused(), + &[alive], + "the session holding a resumable run is refused, not deleted", + ); + assert!( + swept.pruned.sessions >= 1, + "and the finished one is still swept", + ); + assert!( + matches!(sized(&store, alive), Ok(Sized::Holds(_))), + "a refused session is still in the store", + ); + + let report = swept_report(&swept).join("\n"); + assert!( + report.contains(&alive.to_string()), + "the refused id must be named: {report}", + ); + assert!(report.contains("resumable"), "{report}"); +} + +/// **F4 — a sweep that refused nothing says so.** +/// +/// "Refused none" and "we did not check" are the same silence otherwise, and the +/// refusal is the half an operator has to act on. +#[test] +fn f4_a_sweep_that_refused_nothing_says_nothing_was_refused() { + let (_dir, store) = on_disk(); + seed(&store, "/tmp/finished", 2); + + let swept = sweep(&store, AFTER_EVERYTHING).expect("the sweep runs"); + let report = swept_report(&swept).join("\n"); + + assert!(swept.refused().is_empty()); + assert!(report.contains("nothing was refused"), "{report}"); +} + +/// **F4 — the confirmation names the rule and never a count.** +/// +/// The counts cannot be known before the sweep runs — there is no public reader +/// for `sessions.created_at`, which is the column the sweep filters on +/// (io-harness#216, `US-IO-CLI-0.27.0-I02`). A confirmation carrying a number +/// here would be carrying one io-cli invented, and the one available substitute +/// under-states the deletion. +#[test] +fn f4_the_sweep_confirmation_names_the_rule_and_carries_no_count() { + let (title, rows) = confirm_sweep("2026-08-01"); + + assert!( + title.contains("2026-08-01"), + "the rule names the date: {title}" + ); + assert!( + !title + .chars() + .any(|c| c.is_ascii_digit() && !title.contains("2026-08-01")), + "no figure other than the date itself", + ); + assert_eq!(rows[0].label, LEAVE_IT); + let detail = rows[1].detail.clone().unwrap_or_default(); + assert!( + detail.contains("refused"), + "the refusal policy is stated up front: {detail}", + ); + assert!( + detail.contains("reported"), + "and the operator is told the figures come afterwards: {detail}", + ); +} + +// --------------------------------------------------------------------------- +// F5 — the confirmation, and the row that does nothing +// --------------------------------------------------------------------------- + +/// **F5 — row 0 declines in every confirmation this module builds.** +/// +/// Asserted by **index**, across all three, because a `Picker` opens on its first +/// row: the keystroke an operator gives by reflex has to be the one that changes +/// nothing. A label comparison alone would not catch an acting row that drifted +/// to the top. +#[test] +fn f5_row_zero_declines_in_every_confirmation() { + let (_dir, store) = on_disk(); + let session = seed(&store, "/tmp/one", 1); + let size = store.store_size().expect("a size is readable"); + let held = sized(&store, session).expect("a size is readable"); + + for (what, rows) in [ + ("remove", confirm_remove(session, &held).1), + ("sweep", confirm_sweep("2026-08-01").1), + ("compact", confirm_compact(&size).1), + ] { + assert_eq!(rows[0].label, LEAVE_IT, "{what}: row 0 must decline"); + assert!( + !acts(0), + "{what}: and the driver's predicate must agree that it does", + ); + assert!(rows.len() >= 2, "{what}: there is something to choose"); + for index in 1..rows.len() { + assert!(acts(index), "{what}: every other row acts"); + } + } +} + +/// **F5 — declining changes nothing.** +/// +/// The whole `StoreSize` is compared, not one field, so an operation that ran and +/// then reported a refusal is caught as well as one that reported honestly and +/// ran anyway. +#[test] +fn f5_declining_leaves_the_store_byte_identical() { + let (_dir, store) = on_disk(); + let session = seed(&store, "/tmp/one", 4); + let before = store.store_size().expect("a size is readable"); + + // What the driver does with row 0: nothing at all. The predicate is the whole + // decision and it lives in the library precisely so this can be asserted — + // `src/main.rs` is a binary nothing under `tests/` can link. + if acts(0) { + remove(&store, session).expect("unreachable"); + } + + let after = store.store_size().expect("a size is readable"); + assert_eq!(before, after, "declining must change nothing at all"); + assert!(matches!(sized(&store, session), Ok(Sized::Holds(_)))); +} + +/// **F5 — the removal confirmation states what it costs and what goes with it.** +/// +/// The restore-point sentence is in the confirmation rather than in the report, +/// because it is the fact an operator needs *before* they agree: they delete an +/// old session, and reach for its rewind a week later. +#[test] +fn f5_the_removal_confirmation_states_the_cost_and_the_restore_points() { + let (_dir, store) = on_disk(); + let session = seed(&store, "/tmp/one", 5); + let held = sized(&store, session).expect("a size is readable"); + + let (title, rows) = confirm_remove(session, &held); + + assert!(title.contains(&session.to_string()), "{title}"); + assert!( + title.contains("turn"), + "the figures are in the title: {title}" + ); + let detail = rows[1].detail.clone().unwrap_or_default(); + assert!(detail.contains("restore points"), "{detail}"); + assert!( + detail.contains("memory does not"), + "and what survives, which is the half that surprises: {detail}", + ); +} + +/// **F5 — the compaction confirmation states what it costs while it runs.** +/// +/// io-harness's own warning, stated before the call rather than discovered by a +/// full disk. +#[test] +fn f5_the_compaction_confirmation_states_what_it_needs_while_it_runs() { + let (_dir, store) = on_disk(); + seed(&store, "/tmp/one", 2); + let size = store.store_size().expect("a size is readable"); + + let (_title, rows) = confirm_compact(&size); + let detail = rows[1].detail.clone().unwrap_or_default(); + + assert!(detail.contains("rewrites"), "{detail}"); + assert!(detail.contains("free on disk"), "{detail}"); +} + +// --------------------------------------------------------------------------- +// The page, and O4 +// --------------------------------------------------------------------------- + +/// The page names the file, what is free inside it, and the verbs. +/// +/// Also the one place the "free inside the file" wording is asserted: calling it +/// *reclaimable* on the page would invite the reading that a deletion returned it. +#[test] +fn the_page_names_the_file_the_free_space_and_the_three_verbs() { + let (_dir, store) = on_disk(); + let doomed = seed(&store, "/tmp/doomed", 8); + seed(&store, "/tmp/kept", 1); + remove(&store, doomed).expect("the session is removed"); + + let theme = io_cli::theme::THEMES[0]; + let lines = committed(&store, &theme, 100).expect("the page renders"); + let text = lines + .iter() + .flat_map(|line| line.spans.iter().map(|span| span.content.as_ref())) + .collect::(); + + assert!(text.contains("on disk"), "{text}"); + assert!(text.contains("free inside it"), "{text}"); + assert!(text.contains("/store compact"), "{text}"); + assert!(text.contains("/store rm"), "{text}"); + assert!(text.contains("/store sweep"), "{text}"); + assert!( + !text.contains("reclaimed"), + "nothing on this page may say a deletion reclaimed anything: {text}", + ); +} + +/// **O4 — no gate in this file can reach a real store.** +/// +/// A test that compacted or swept the developer's own `~/.io-cli/runs.db` would +/// be data loss shipped as a test. Read as text, because that is the only +/// instrument that sees what a test *could* open rather than what it did. +#[test] +fn o4_no_store_gate_names_a_real_home() { + let source = include_str!("store.rs"); + let code: String = source + .lines() + .filter(|line| !line.trim_start().starts_with("//")) + .collect::>() + .join("\n"); + + // **The needles are assembled rather than written out, and that is the whole + // reason this loop looks odd.** A gate that reads its own file cannot spell + // the thing it forbids: the array would be the first match. io-cli has now + // hit this in 0.16.0, 0.19.0, 0.25.0 and 0.26.0 — twice in one wave in + // 0.25.0, where two agents each wrote a forbidden name into a comment in + // order to say the file did not use it. Stripping comments, which the sweep + // above does, is only half the fix; the other half is here. + for (head, tail) in [ + (".io", "-cli"), + ("home", "_dir"), + ("IO_CONFIG", "_HOME"), + ("USER", "PROFILE"), + ] { + let forbidden = format!("{head}{tail}"); + assert!( + !code.contains(&forbidden), + "a store gate must never be able to reach a real home, and this file names `{forbidden}`", + ); + } + assert!( + code.contains("tempfile::tempdir"), + "every store here is a temporary one", + ); + // Assembled for the same reason as the loop above: spelled out, this needle + // is its own first match. + let in_memory = format!("{}::{}", "Store", "memory"); + assert!( + !code.contains(&in_memory), + "and a real file, because the subject is page arithmetic on one", + ); +} From ca02f3814ec4c78c6a2e9884f257b2ca88874751 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Fri, 28 Aug 2026 17:06:48 +0530 Subject: [PATCH 2/8] feat(export): the conversation as markdown, the run's canonical trace verbatim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/export` writes this session's conversation as markdown for a person to read, and `/export trace` writes one run's canonical trace exactly as io-harness produced it. Both go through `Workspace::write_file`, so the path policy the session runs under is the one that applies, and an existing file is refused rather than overwritten: an export is a snapshot, and the next one is a different snapshot. The trace is passed through untouched. Its whole value is that io-harness excludes wall-clock stamps, measured durations, an ephemeral tempdir path and autoincrement ids from it so two runs of one case can be compared; a trace this crate reformatted would compare against nothing. The gate is a byte comparison for that reason, not a parse-and-compare, which would pass on exactly the defect that matters. It is not JSON. `canonical_trace` builds pipe-delimited lines by hand, there is no JSON alternative in io-harness, and `StepRecord` does not derive `Serialize`. The export takes a `.txt` path: a file named `.json` that is not JSON breaks for anyone who pipes it to a JSON tool. An existence check through `read_bytes` was wrong and would have refused every first export — a missing file is `Error::Config("no such file")`, not `Error::Io`. It asks `resolve(path)?.exists()` now, which separates whether a path may be addressed from whether it may be written. `Inspect` reaches ten commands, which is the bound, so its gate is an equality now rather than "there is room left". --- README.md | 1 + src/commands.rs | 40 +++++- src/export.rs | 255 ++++++++++++++++++++++++++++++++++++ src/lib.rs | 2 + src/main.rs | 103 +++++++++++++++ tests/commands.rs | 37 ++++-- tests/export.rs | 327 ++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 753 insertions(+), 12 deletions(-) create mode 100644 src/export.rs create mode 100644 tests/export.rs diff --git a/README.md b/README.md index 35297de..09b20d3 100644 --- a/README.md +++ b/README.md @@ -530,6 +530,7 @@ above a row that ranked there for reasons having nothing to do with it. | `/cost` | commit what this run, this session and this install have spent | | `/stats` | commit how the runs have gone: outcomes, first-try, gates, latency | | `/store` | commit what the run store holds; `rm `, `sweep ` and `compact` change it | +| `/export` | write this conversation as markdown, or `trace` for a run's canonical trace | **configure** diff --git a/src/commands.rs b/src/commands.rs index 02d3ef3..751b9f3 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -277,6 +277,15 @@ pub const COMMANDS: &[(&str, &str)] = &[ "/store", "commit what the run store holds; `rm `, `sweep ` and `compact` change it", ), + // Beside `/store` because the two answer the same question from opposite + // ends: that one is what the store is keeping, this one is how the work gets + // out. The description names both files, because markdown and a canonical + // trace are for two different readers and choosing between them is the whole + // decision. + ( + "/export", + "write this conversation as markdown, or `trace` for a run's canonical trace", + ), // Beside `/stats` because `/stats` is the only other row that says the word, // and the two are the halves of one thing: that page counts how the gates // went, and until this release nothing in the product could say what a gate @@ -453,7 +462,7 @@ pub const GROUPS: &[(Group, &[&str])] = &[ // which is the bound — so the next command that would fill this group // re-files one that is in the wrong group rather than widening it, // exactly as `Turn` did for `/undo` in this same release. - "/store", + "/store", "/export", ], ), // **`/mcp` and `/provider` moved here in 0.19.0, and it is a correction rather @@ -1237,6 +1246,23 @@ pub enum Action { /// [`crate::store`] for why a removal and a compaction are different /// questions with different costs. Store(Option), + /// Write the work out: the conversation as markdown, or one run's canonical + /// trace, verbatim. + /// + /// See [`Taken`]. The path is optional in both forms and a proposed one is + /// used when it is absent — proposed rather than imposed, and refused rather + /// than overwritten when something is already there. + Export(Taken), +} + +/// Which export was asked for, and where it goes. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Taken { + /// This session's conversation, as markdown. + Conversation(Option), + /// The last run's canonical trace, exactly as io-harness produced + /// it. + Trace(Option), } /// What `/store` was asked to do, once the page is not the answer. @@ -1929,6 +1955,18 @@ pub fn parse(input: &str, keys: &Keys, theme: &Theme) -> Action { Some(word) => Action::Store(Some(Keep::Unknown(word.to_string()))), } } + // `trace` is a word and everything else is a path, which is the one + // ambiguity worth naming: an operator who wants a file called `trace` + // types `./trace`. The alternative — a `--trace` flag — would be the only + // flag on any command in this product. + "export" => { + let mut rest = input.split_whitespace().skip(1); + match rest.next() { + Some("trace") => Action::Export(Taken::Trace(rest.next().map(str::to_string))), + Some(path) => Action::Export(Taken::Conversation(Some(path.to_string()))), + None => Action::Export(Taken::Conversation(None)), + } + } "config" | "settings" => { let mut rest = input.split_whitespace().skip(1); match rest.next() { diff --git a/src/export.rs b/src/export.rs new file mode 100644 index 0000000..de49b81 --- /dev/null +++ b/src/export.rs @@ -0,0 +1,255 @@ +//! The conversation as markdown, and the run's canonical trace, verbatim. +//! +//! A session is a conversation that ends when the terminal closes. The review +//! that matters usually happens somewhere else — in a pull request, in a text +//! editor, in a message to somebody who was not there — and until this release +//! there was no way to get it out. `/export` writes two files: the conversation +//! for a human to read, and one run's canonical trace for a machine to compare. +//! +//! # The trace is written verbatim, and that is the whole rule +//! +//! [`Store::canonical_trace`] returns a string whose entire value is that it is +//! **canonical**: io-harness excludes wall-clock stamps, measured durations, the +//! argv's ephemeral tempdir path and `AUTOINCREMENT` ids from it precisely so +//! that two runs of one case can be compared, and its documentation says that +//! excluding a field is a decision the crate cannot promise rather than a +//! convenience. +//! +//! So this module does not parse it, does not reserialise it, does not +//! pretty-print it and adds no field of its own. A trace io-cli reformatted +//! compares against nothing, which is why `tests/export.rs` asserts a +//! **byte-identical** string. +//! +//! **It is not JSON, and this release's plan said it was.** The format is +//! pipe-delimited lines built by hand — `step {n} | tokens {n} | decision … ` +//! per step, then `context {step} | {kind} | {detail}` per context event +//! (`state/trace.rs:1019-1036`). io-harness offers no JSON alternative; +//! `StepRecord` does not even derive `Serialize`. io-cli *could* build one — +//! `serde_json` is among its ten dependencies — and must not, because a document +//! this crate assembled would be a second format that compares against nothing, +//! which is the very thing the verbatim rule exists for. So the export takes a +//! `.txt` extension: a file named `.json` that is not JSON is a defect for +//! anyone who pipes it to a JSON tool. Recorded as `US-IO-CLI-0.27.0-I03`, and +//! found by a sabotage that killed nothing. +//! +//! # The markdown is for a person, and nothing reads it back +//! +//! It carries no schema version, nothing in this product parses it, and it is +//! not an interchange format. It is built from [`Store::session_turns`], whose +//! [`io_harness::Turn`] carries `prompt`, `reply`, `outcome`, `run_id` and +//! `created_at` — every one of them written by the store, and the timestamp +//! passed through untouched, because this crate reads no clock outside +//! `src/main.rs` and `tests/timing.rs` enforces it. +//! +//! **A turn with no reply is written as a turn with no reply.** `Turn::reply` is +//! `Option`, and `None` means the turn did not finish — a run that was +//! interrupted, that died, or that is still going. Rendering it as an empty +//! answer would make an unfinished conversation look like one the agent had +//! nothing to say to. +//! +//! **There is no public reader for `runs.goal`.** Recorded in 0.23.0 and still +//! true in 0.69, so a run is named here by its id and its timestamp rather than +//! by the goal it was given. +//! +//! # Both files go through the workspace, under the session's own policy +//! +//! [`Workspace::write_file`] and never `std::fs::write`: an export is a file +//! this product writes on the operator's behalf, so it is subject to exactly the +//! path policy every other write is, and a path outside the workspace is refused +//! by io-harness rather than by a check invented here. +//! +//! **An existing file is refused rather than overwritten.** An export is a +//! snapshot, and the second one an operator takes is a different snapshot; the +//! command that silently replaced the first would destroy the thing they were +//! about to compare against. [`Refused::Exists`] names the file and stops. + +use io_harness::tools::{Workspace, Wrote}; +use io_harness::{Error, Store}; + +/// The extension a conversation export takes. +pub const MARKDOWN: &str = "md"; + +/// The extension a trace export takes. +/// +/// `txt` and not `json`: [`Store::canonical_trace`] is pipe-delimited text. See +/// the module note and `US-IO-CLI-0.27.0-I03`. +pub const TRACE: &str = "txt"; + +/// Why an export did not happen. +/// +/// Both variants are refusals rather than errors: the operator asked for +/// something reasonable and is being told why it did not happen, in words that +/// name the next thing they can do. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Refused { + /// There is already a file at that path. An export is a snapshot and the + /// next one is a different snapshot; overwriting silently would destroy the + /// thing the operator was about to compare against. + Exists(String), + /// The session holds no turns, so there is no conversation to write. Writing + /// an empty document would be a file that says the session was empty when it + /// may simply not have started. + Nothing, +} + +impl Refused { + /// What to say about it. + pub fn said(&self) -> String { + match self { + Refused::Exists(path) => format!( + "{path} is already there — an export is a snapshot, so this one is \ + not written over it; name another path" + ), + Refused::Nothing => { + "there is nothing to export yet — this session has no finished turns".to_string() + } + } + } +} + +/// What one export did. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Written { + /// The path, relative to the workspace root, as it was written. + pub path: String, + /// How many bytes went into it. + pub bytes: usize, + /// What io-harness's own writer said happened. Always [`Wrote::Created`] + /// here, because an existing path is refused before the write — carried + /// anyway so that a future path that stops refusing cannot silently start + /// reporting a creation for an overwrite. + pub wrote: Wrote, +} + +/// The conversation, as markdown. +/// +/// Every turn the store holds for the session, in the order it returns them, +/// each with its prompt and — when the turn finished — its reply. A turn that +/// never finished says so. +pub fn conversation(store: &Store, session_id: i64) -> Result, Error> { + let turns = store.session_turns(session_id)?; + if turns.is_empty() { + return Ok(None); + } + + let mut out = format!("# Session {session_id}\n"); + for turn in turns { + // The store's own stamp, passed through. Never an age, never a + // reformatting: `tests/timing.rs` forbids this crate reading a clock, and + // a document exported twice from the same store must be the same document. + out.push_str(&format!( + "\n## Turn {} · run {} · {}\n\n", + turn.id, turn.run_id, turn.created_at + )); + out.push_str("### Prompt\n\n"); + out.push_str(&turn.prompt); + out.push_str("\n\n### Reply\n\n"); + match &turn.reply { + Some(reply) => { + out.push_str(reply); + out.push('\n'); + } + // Not an empty section. `None` means the turn did not finish, and an + // interrupted conversation must not read as one the agent had nothing + // to say to. + None => out.push_str("*this turn did not finish*\n"), + } + if let Some(outcome) = &turn.outcome { + out.push_str(&format!("\n*outcome: {outcome}*\n")); + } + } + Ok(Some(out)) +} + +/// One run's canonical trace, exactly as io-harness produced it. +/// +/// Passed through. See the module note: the value of this string is that it is +/// canonical, and anything this crate did to it would end that. +pub fn trace(store: &Store, run_id: i64) -> Result { + store.canonical_trace(run_id) +} + +/// The path an export proposes for a session's conversation. +/// +/// Proposed rather than imposed: the operator can name their own, and this is +/// what they get when they do not. Named after what it holds so two exports of +/// two sessions do not collide, which is the same reason it is refused rather +/// than overwritten when it is already there. +pub fn conversation_path(session_id: i64) -> String { + format!("io-session-{session_id}.{MARKDOWN}") +} + +/// The path an export proposes for a run's trace. +pub fn trace_path(run_id: i64) -> String { + format!("io-run-{run_id}.{TRACE}") +} + +/// Write one export, refusing rather than overwriting. +/// +/// Through [`Workspace::write_file`], so the path policy the session is running +/// under is the one that applies and a path outside the workspace is io-harness's +/// refusal rather than a check invented here. +/// +/// The existence check is [`Workspace::read_bytes`] — the same reader +/// `crate::rewind` uses to tell a missing file from an empty one — and it is +/// deliberately asked *through the workspace* too, so a path the policy denies +/// is refused by the policy rather than answered by a filesystem probe that +/// leaked whether the file exists. +pub fn write(workspace: &Workspace, path: &str, content: &str) -> Result { + let wrote = workspace.write_file(path, content)?; + Ok(Written { + path: path.to_string(), + bytes: content.len(), + wrote, + }) +} + +/// Whether something is already at `path`, asked through the workspace. +/// +/// `Ok(true)` means there is a file there. An `Err` is the path escaping the +/// root, and is propagated rather than treated as absence — answering "nothing +/// is there" for a path that cannot be written would send the operator into a +/// confirmation for a write that was never going to happen. +/// +/// **[`Workspace::resolve`] and then `Path::exists`, rather than a read.** The +/// first implementation asked [`Workspace::read_bytes`] and treated `Error::Io` +/// as absence, and that was wrong in the way that matters: io-harness answers a +/// missing file with `Error::Config("no such file: …")`, so *every first export* +/// would have been refused with "could not be checked" — the export that has +/// never been taken being exactly the one an operator takes first. Caught by +/// `tests/export.rs`, never by reading. +/// +/// Asking `resolve` also separates the two questions properly. This one is *may +/// this path be addressed at all* — absolute paths and any `..` climbing above +/// the root are refused here, by io-harness, on its own rules. Whether the +/// operator may **write** it is [`Workspace::write_file`]'s own `Act::Write` +/// gate, which is enforced at the write and must not be second-guessed here: a +/// permission check that ran early and a permission check that ran late would be +/// two answers to one question. +pub fn occupied(workspace: &Workspace, path: &str) -> Result { + Ok(workspace.resolve(path)?.exists()) +} + +/// The confirmation `/export` shows before it writes. +/// +/// Row 0 declines, as it does in every confirmation this product uses for +/// something it cannot take back — and an export can overwrite nothing, but it +/// does put the conversation on disk where the operator may not have expected a +/// file to appear. +pub fn confirm(path: &str, what: &str) -> (String, Vec) { + ( + format!("Write this {what} to {path}?"), + vec![ + crate::picker::Row::with_detail(crate::store::LEAVE_IT, "nothing is written"), + crate::picker::Row::with_detail( + format!("write {path}"), + "under the session's own path policy; an existing file is refused", + ), + ], + ) +} + +/// What to say after an export. +pub fn report(written: &Written) -> String { + format!("wrote {} ({} bytes)", written.path, written.bytes) +} diff --git a/src/lib.rs b/src/lib.rs index aae1c30..ba794ad 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -62,6 +62,7 @@ pub mod edit; pub mod editor; pub mod events; pub mod exec; +pub mod export; pub mod failure; pub mod fanout; pub mod fleet; @@ -102,6 +103,7 @@ pub mod stats; pub mod status; pub mod stdin; pub mod store; + pub mod term; pub mod theme; pub mod transcript; diff --git a/src/main.rs b/src/main.rs index 4620bf2..9f65235 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2532,6 +2532,26 @@ async fn loop_over Result>( } } } + Pick::Export { path, content } => { + if io_cli::store::acts(index) { + let workspace = io_harness::tools::Workspace::with_policy( + session.root(), + policy.clone(), + ); + match io_cli::export::write(&workspace, path, content) { + Ok(written) => { + app.record(Tone::Muted, io_cli::export::report(&written)) + } + Err(error) => app.record( + Tone::Error, + format!( + "{path} was not written: {}", + io_cli::failure::said(&error) + ), + ), + } + } + } Pick::StoreCompact => { if io_cli::store::acts(index) { match io_cli::store::compact(&store) { @@ -3761,6 +3781,76 @@ async fn loop_over Result>( ), ), }, + // Both halves build their content HERE, before the confirmation, + // so the operator agrees to a file that exists rather than to an + // intention. A build that fails says so and opens nothing. + Action::Export(taken) => { + let workspace = + io_harness::tools::Workspace::with_policy(session.root(), policy.clone()); + let built = match &taken { + commands::Taken::Conversation(path) => { + match io_cli::export::conversation(&store, session.id()) { + Ok(Some(text)) => Ok(Some(( + path.clone().unwrap_or_else(|| { + io_cli::export::conversation_path(session.id()) + }), + text, + "conversation", + ))), + Ok(None) => Err(io_cli::export::Refused::Nothing.said()), + Err(error) => Err(format!( + "the conversation could not be read: {}", + io_cli::failure::said(&error) + )), + } + } + commands::Taken::Trace(path) => { + match last_run(&session, &store).map(|turn| turn.run_id) { + Some(run_id) => match io_cli::export::trace(&store, run_id) { + Ok(json) => Ok(Some(( + path.clone() + .unwrap_or_else(|| io_cli::export::trace_path(run_id)), + json, + "trace", + ))), + Err(error) => Err(format!( + "the trace could not be read: {}", + io_cli::failure::said(&error) + )), + }, + None => Err(io_cli::export::Refused::Nothing.said()), + } + } + }; + match built { + Ok(Some((path, content, what))) => { + // Asked before the confirmation, so an operator is + // never shown a write that is going to be refused. + match io_cli::export::occupied(&workspace, &path) { + Ok(true) => app.record( + Tone::Error, + io_cli::export::Refused::Exists(path).said(), + ), + Ok(false) => { + let (title, rows) = io_cli::export::confirm(&path, what); + picker = Some(( + Picker::new(title, rows), + Pick::Export { path, content }, + )); + } + Err(error) => app.record( + Tone::Error, + format!( + "{path} could not be checked: {}", + io_cli::failure::said(&error) + ), + ), + } + } + Ok(None) => {} + Err(said) => app.record(Tone::Error, said), + } + } // Reached only at an idle prompt: while a turn runs the driver's // own key handler answers `/steer` before `parse` is ever called, // because that is where the inbox lives. So this arm is the @@ -5887,6 +5977,19 @@ enum Pick { /// A confirmation over a compaction. Carries nothing: the operation takes no /// argument, and the figures it reports are read either side of the call. StoreCompact, + /// A confirmation over one export, carrying the bytes it will write. + /// + /// The content is built before the confirmation and carried rather than + /// rebuilt on acceptance, so the file written is the one the operator was + /// shown the size of — and so a store that stops answering between the two + /// keystrokes cannot turn an agreed export into a truncated file. + Export { + /// Where it goes, relative to the workspace root. + path: String, + /// Exactly what goes in it. For a trace this is io-harness's own string, + /// untouched. + content: String, + }, /// One directory of the workspace, in the order `list_dir` sorted it, so a /// chosen index reads straight back through `complete::pick`. The rows are /// last components rather than paths — see `complete::rows` for why — which diff --git a/tests/commands.rs b/tests/commands.rs index 0ad9212..119d5be 100644 --- a/tests/commands.rs +++ b/tests/commands.rs @@ -104,6 +104,10 @@ fn the_commands_are_the_commands() { // row 0 does nothing; it takes that group to nine and `/export` will // take it to ten, the bound. "/store", + // 0.27.0 — the other end of the same question: `/store` is what is + // being kept, this is how the work gets out. It takes `Inspect` to + // ten, which is the bound. + "/export", // 0.24.0 — beside `/stats` because `/stats` is the only other row // that says the word: that page counts how the gates went, and until // this release nothing in the product could say what a gate was. It @@ -315,10 +319,10 @@ fn f12_resume_says_it_answers_a_parked_run_rather_than_merely_reopening_a_sessio // release growing a command and losing another. assert_eq!( COMMANDS.len(), - 34, - "0.27.0 adds `/store` on top of the thirty-three 0.26.0 shipped, and will \ - add `/export` and `/undo` before it is done; a number this gate does not \ - name means one arrived unrecorded", + 35, + "0.27.0 adds `/store` and `/export` on top of the thirty-three 0.26.0 \ + shipped, and will add `/undo` before it is done; a number this gate does \ + not name means one arrived unrecorded", ); } @@ -987,18 +991,29 @@ fn the_writers_are_configure_commands_and_skills_is_the_inspection() { "`/skills` opens a list and writes nothing, which is what Inspect means", ); - // The room the move made is real, not an accounting trick: Inspect is under - // the bound with a command to spare. `f13_no_group_is_longer_than_ten` is - // still the gate; this only says the release did not spend every seat it - // freed. + // **0.27.0 spends the last seat this move freed, and that is a decision + // rather than an oversight.** The 0.19.0 form of this assertion was + // `inspect < 10` — the release that made the room saying it had not used all + // of it. `/store` and `/export` take the group to exactly ten, which is the + // bound `f13_no_group_is_longer_than_ten` enforces, so the sentence this gate + // makes changes from *there is room left* to *the room is now gone*: the next + // command that would belong in `Inspect` re-files one that is in the wrong + // group rather than widening the bound, which is what `Turn` did for `/undo` + // in this same release and what 0.19.0, 0.22.0 and 0.26.0 each did before it. + // + // The equality is deliberate. `<= 10` would go on passing at nine and would + // stop saying anything the moment somebody removed a command; an exact ten + // fails in both directions and makes the next change a decision somebody + // records here. let inspect = GROUPS .iter() .find(|(group, _)| *group == Group::Inspect) .map(|(_, names)| names.len()) .expect("Inspect is a group with commands in it"); - assert!( - inspect < 10, - "Inspect holds {inspect}; the point of the move was to leave room", + assert_eq!( + inspect, 10, + "Inspect holds {inspect}; 0.27.0 filled it to the bound with `/store` and \ + `/export`, so the next command here re-files rather than widens", ); let rows = palette(&Templates::none(), &[]); diff --git a/tests/export.rs b/tests/export.rs new file mode 100644 index 0000000..b4634d4 --- /dev/null +++ b/tests/export.rs @@ -0,0 +1,327 @@ +//! Taking the work out of the terminal. +//! +//! **The single assertion this file exists for is a byte comparison.** A run's +//! canonical trace is valuable only because it is canonical: io-harness excludes +//! wall-clock stamps, measured durations, an argv's ephemeral tempdir and +//! `AUTOINCREMENT` ids from it so that two runs of one case can be compared, and +//! its own documentation says each exclusion is a promise rather than a +//! convenience. A trace io-cli pretty-printed would compare against nothing. +//! It is pipe-delimited text and not JSON, which this release's plan had wrong +//! until a sabotage that killed nothing exposed it — see `US-IO-CLI-0.27.0-I03`. +//! +//! So `f8_the_trace_is_written_byte_for_byte` compares **strings**, not parsed +//! values. Parsing both sides and comparing them is the test that looks more +//! rigorous and is the one that passes on the defect: a reformatted document +//! parses to the same values, which is exactly the thing that must fail. +//! +//! The markdown half is asserted on content and order rather than on formatting, +//! because nothing reads it back — it is for a person and for a diff, it carries +//! no schema version, and this product parses it nowhere. + +use io_cli::export::{ + confirm, conversation, conversation_path, occupied, report, trace, trace_path, write, Refused, +}; +use io_harness::tools::{Workspace, Wrote}; +use io_harness::Store; + +/// A store in a real file, plus a workspace to write into. +/// +/// One `TempDir` for both, because an export writes into the workspace the +/// session is running over and the point of the last test here is that it cannot +/// write anywhere else. +fn fixture() -> (tempfile::TempDir, Store, Workspace) { + let dir = tempfile::tempdir().expect("a temporary directory"); + let store = Store::open(dir.path().join("runs.db")).expect("a store opens on disk"); + let workspace = Workspace::new(dir.path()); + (dir, store, workspace) +} + +/// One session with a finished turn per prompt, and the run ids in order. +/// +/// **Every run records two steps, and that is not decoration.** The first +/// version of this fixture recorded none, and `Store::canonical_trace` is built +/// entirely from `steps` and `context_events` (`state/trace.rs:1019-1036`) — so +/// it returned the empty string, and the byte-for-byte gate below was comparing +/// `""` against `""`. It passed, and it would have gone on passing over any +/// defect whatsoever. +/// +/// It was found because the named sabotage — pretty-print the trace — killed +/// **nothing**, which is the whole reason a sabotage is executed rather than +/// described. A gate that cannot fail is worse than a missing one, because it +/// reads on the page as coverage. +fn seed(store: &Store, prompts: &[(&str, Option<&str>)]) -> (i64, Vec) { + let session = store.create_session("/tmp/work").expect("a session opens"); + let mut runs = Vec::new(); + for (prompt, reply) in prompts { + let run = store.start_run(prompt, "/tmp/work").expect("a run starts"); + for step in 1..=2u32 { + store + .record( + run, + &io_harness::StepRecord { + step, + decision: format!("write_file ok on step {step}"), + result: format!("wrote src/thing-{step}.rs"), + prompt: prompt.to_string(), + tool_call: format!("write_file:{{\"path\":\"src/thing-{step}.rs\"}}"), + tokens: 128 * u64::from(step), + }, + ) + .expect("a step records"); + } + let turn = store + .record_turn(session, None, run, prompt) + .expect("a turn records"); + // A `None` reply is a turn that did not finish, and it is written as one. + // The turn is still closed, because an unfinished *reply* and an + // unfinished *turn* are different states and only the first is on trial. + store + .finish_turn(turn, *reply, "completed") + .expect("a turn finishes"); + store.finish_run(run, "success").expect("the run finishes"); + runs.push(run); + } + (session, runs) +} + +// --------------------------------------------------------------------------- +// F8 — the conversation, in order +// --------------------------------------------------------------------------- + +/// **F8 — every prompt and every reply appears, in the store's own order.** +/// +/// Order is asserted by position in the rendered string rather than by counting +/// occurrences, because a document that carried all six strings shuffled would +/// satisfy a count and would be useless to read. +#[test] +fn f8_every_prompt_and_reply_appears_in_the_stores_order() { + let (_dir, store, _ws) = fixture(); + let (session, _runs) = seed( + &store, + &[ + ("the first question", Some("the first answer")), + ("the second question", Some("the second answer")), + ("the third question", Some("the third answer")), + ], + ); + + let markdown = conversation(&store, session) + .expect("the conversation is readable") + .expect("there is one"); + + let mut at = 0; + for needle in [ + "the first question", + "the first answer", + "the second question", + "the second answer", + "the third question", + "the third answer", + ] { + let found = markdown[at..] + .find(needle) + .unwrap_or_else(|| panic!("`{needle}` is missing or out of order:\n{markdown}")); + at += found + needle.len(); + } +} + +/// **F8 — a turn with no reply is written as a turn with no reply.** +/// +/// `Turn::reply` is `Option` and `None` means the turn did not finish. +/// An empty section would make an interrupted conversation read as one the agent +/// had nothing to say to. +#[test] +fn f8_a_turn_that_never_answered_says_so_rather_than_showing_an_empty_reply() { + let (_dir, store, _ws) = fixture(); + let (session, _runs) = seed( + &store, + &[("answered", Some("here you are")), ("never answered", None)], + ); + + let markdown = conversation(&store, session) + .expect("the conversation is readable") + .expect("there is one"); + + assert!(markdown.contains("here you are")); + assert!( + markdown.contains("did not finish"), + "an unanswered turn is named, not blank:\n{markdown}", + ); +} + +/// A session with no turns is nothing to export, and says so. +/// +/// A file asserting the session was empty is worse than no file: the session may +/// simply not have started. +#[test] +fn a_session_with_no_turns_is_refused_rather_than_written_empty() { + let (_dir, store, _ws) = fixture(); + let session = store.create_session("/tmp/work").expect("a session opens"); + + assert_eq!( + conversation(&store, session).expect("readable"), + None, + "there is no conversation to write", + ); + assert!(Refused::Nothing.said().contains("nothing to export")); +} + +// --------------------------------------------------------------------------- +// F8 — the trace, byte for byte +// --------------------------------------------------------------------------- + +/// **F8 — the trace file is byte-identical to `Store::canonical_trace`.** +/// +/// A string comparison, deliberately. See the module note: parsing both sides +/// and comparing values is the test that passes on the defect. +#[test] +fn f8_the_trace_is_written_byte_for_byte() { + let (_dir, store, workspace) = fixture(); + let (_session, runs) = seed(&store, &[("do the thing", Some("done"))]); + let run = runs[0]; + + let ours = trace(&store, run).expect("a trace is readable"); + let harness = store.canonical_trace(run).expect("a trace is readable"); + // **The gate that stops this one going vacuous again.** A run with no steps + // has an empty canonical trace, and every assertion below is satisfied by + // two empty strings. Assert there is something to compare before comparing + // it — 0.26.0's lesson that a gate can go vacuous without going red. + assert!( + !harness.is_empty(), + "the fixture must produce a trace, or every assertion here is `\"\" == \"\"`", + ); + assert!(harness.contains("step 1"), "and a real one: {harness}"); + assert_eq!(ours, harness, "the trace is passed through untouched"); + + let path = trace_path(run); + let written = write(&workspace, &path, &ours).expect("the trace is written"); + assert_eq!(written.wrote, Wrote::Created); + + let back = std::fs::read_to_string(workspace.root().join(&path)).expect("the file is there"); + assert_eq!( + back, harness, + "the file on disk is io-harness's own string, byte for byte", + ); +} + +/// The trace is not reformatted, and this is the assertion that says so +/// independently of the equality above. +/// +/// A pretty-printer is the specific defect: it would change the whitespace and +/// nothing else, so a test that only compared parsed values would not see it. +#[test] +fn the_trace_is_not_reformatted() { + let (_dir, store, _ws) = fixture(); + let (_session, runs) = seed(&store, &[("do the thing", Some("done"))]); + + let ours = trace(&store, runs[0]).expect("a trace is readable"); + let harness = store.canonical_trace(runs[0]).expect("a trace is readable"); + + assert!(!harness.is_empty(), "there is something to reformat"); + assert_eq!( + ours.len(), + harness.len(), + "a reformatting changes the length even when it changes no value", + ); + assert_eq!( + ours.matches('\n').count(), + harness.matches('\n').count(), + "and it changes the line count, which is what a pretty-printer does", + ); +} + +// --------------------------------------------------------------------------- +// F8 — the write goes through the workspace +// --------------------------------------------------------------------------- + +/// **F8 — a path outside the workspace is refused rather than written.** +/// +/// The refusal is io-harness's, through `Workspace::write_file`, and is asserted +/// by the file's absence as well as by the error — an error that had already +/// written the file would be worse than no error at all. +#[test] +fn f8_a_path_outside_the_workspace_is_refused() { + let (_dir, store, workspace) = fixture(); + let (session, _runs) = seed(&store, &[("a question", Some("an answer"))]); + let markdown = conversation(&store, session) + .expect("readable") + .expect("there is one"); + + let escape = "../escaped.md"; + let refused = write(&workspace, escape, &markdown); + + assert!(refused.is_err(), "a path above the root is refused"); + assert!( + !workspace.root().join("..").join("escaped.md").exists(), + "and nothing is written outside the workspace", + ); +} + +/// An existing file is refused rather than overwritten. +/// +/// An export is a snapshot; the next one is a different snapshot. A command that +/// silently replaced the first would destroy what the operator was about to +/// compare against. +#[test] +fn an_existing_file_is_refused_rather_than_overwritten() { + let (_dir, store, workspace) = fixture(); + let (session, _runs) = seed(&store, &[("a question", Some("an answer"))]); + let path = conversation_path(session); + let markdown = conversation(&store, session) + .expect("readable") + .expect("there is one"); + + assert!( + !occupied(&workspace, &path).expect("askable"), + "nothing yet" + ); + write(&workspace, &path, &markdown).expect("the first export is written"); + assert!( + occupied(&workspace, &path).expect("askable"), + "and now something is there", + ); + + let said = Refused::Exists(path.clone()).said(); + assert!(said.contains(&path), "the refusal names the file: {said}"); + assert!(said.contains("snapshot"), "and says why: {said}"); +} + +/// The proposed paths are distinct per session and per run, so two exports do +/// not collide before anyone has been asked anything. +#[test] +fn the_proposed_paths_name_what_they_hold() { + assert_ne!(conversation_path(1), conversation_path(2)); + assert_ne!(trace_path(1), trace_path(2)); + assert!(conversation_path(7).ends_with(".md")); + assert!(trace_path(7).ends_with(".txt")); +} + +/// The confirmation declines at row 0, like every other confirmation this +/// release adds, and names the path it is about. +#[test] +fn the_confirmation_declines_at_row_zero_and_names_the_path() { + let (title, rows) = confirm("io-session-3.md", "conversation"); + + assert!(title.contains("io-session-3.md"), "{title}"); + assert_eq!(rows[0].label, io_cli::store::LEAVE_IT); + assert!(!io_cli::store::acts(0)); + assert!(io_cli::store::acts(1)); + assert!(rows[1].label.contains("io-session-3.md")); +} + +/// The report says what was written and how much of it. +#[test] +fn the_report_names_the_file_and_its_size() { + let (_dir, store, workspace) = fixture(); + let (session, _runs) = seed(&store, &[("a question", Some("an answer"))]); + let markdown = conversation(&store, session) + .expect("readable") + .expect("there is one"); + let written = + write(&workspace, &conversation_path(session), &markdown).expect("the export is written"); + + let said = report(&written); + assert!(said.contains(&conversation_path(session)), "{said}"); + assert!(said.contains(&written.bytes.to_string()), "{said}"); + assert_eq!(written.bytes, markdown.len()); +} From 57fb96d9ef7f56cd248ad4361f8bf484d30afeaa Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Fri, 28 Aug 2026 17:20:52 +0530 Subject: [PATCH 3/8] feat(undo): put work back at one file, one step, or the whole run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/undo ` restores one file from the snapshot taken before this run first wrote it. `/undo step ` reverse-applies one step's recorded diff. A bare `/undo` undoes the whole turn, which is what the rewind chord has always done — and both doors now reach one `undo_whole_turn`, so the word and the keystroke cannot drift apart. Two events fire for the first time in this product's history. `Rewound` is emitted only inside `rewind_run_observed` and `Reverted` only inside `rewind_step_observed`; `src/rewind.rs` called the plain `rewind_run` from 0.4.0 until now, so the arm rendering one of them in `src/events.rs` was unreachable from the day it was written. The gates assert on what an observer was handed, because the rendered sentence is identical either way. Reverting a step is order-sensitive: a later step still standing on the same lines makes the revert stale and io-harness leaves the file alone rather than fuzzy-matching it. That is said on screen, and only when something actually came back stale. The four answers to a file restore stay four answers. `NotKept` and `NotRecorded` mean different things, and collapsing them tells an operator "nothing to undo" about a file whose previous contents were merely over the snapshot cap. `/contain` moves from `Turn` to `Session` to make room, on that group's own rule: `Turn` means a command acting on the work the turn just finished, and `/contain on|off` is a posture that changes how every later turn is driven. The bound stays at ten, as it did in 0.19.0, 0.22.0 and 0.26.0. --- README.md | 1 + src/commands.rs | 58 +++++- src/lib.rs | 2 +- src/main.rs | 279 ++++++++++++++++++++++++---- src/rewind.rs | 10 +- src/undo.rs | 202 ++++++++++++++++++++ tests/commands.rs | 17 +- tests/live.rs | 2 +- tests/rewind.rs | 16 +- tests/support/mod.rs | 24 +++ tests/undo.rs | 434 +++++++++++++++++++++++++++++++++++++++++++ 11 files changed, 993 insertions(+), 52 deletions(-) create mode 100644 src/undo.rs create mode 100644 tests/undo.rs diff --git a/README.md b/README.md index 09b20d3..f49be70 100644 --- a/README.md +++ b/README.md @@ -508,6 +508,7 @@ above a row that ranked there for reasons having nothing to do with it. | --- | --- | | `/model` | change the model the next turn is sent to | | `/effort` | how much reasoning the next turn buys: low, medium, high, or off | +| `/undo` | put work back: `` for one file, `step ` for one step, bare for the run | | `/contain` | run turns contained, so the agent can fan out: on, off, or ask | | `/plan` | make turns propose a plan before they work: on, off, or ask | | `/steer` | send what is queued into the turn that is already running | diff --git a/src/commands.rs b/src/commands.rs index 751b9f3..dbc9df2 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -247,6 +247,13 @@ pub const COMMANDS: &[(&str, &str)] = &[ "/contain", "run turns contained, so the agent can fan out: on, off, or ask", ), + // 0.27.0 — the undo that is the size of the mistake. The bare word is the + // whole run, which is what the rewind chord has always done; the two argued + // forms are what this release adds and are the ones an operator reaches for. + ( + "/undo", + "put work back: `` for one file, `step ` for one step, bare for the run", + ), ( "/plan", "make turns propose a plan before they work: on, off, or ask", @@ -393,9 +400,24 @@ pub const GROUPS: &[(Group, &[&str])] = &[ // that met it — re-file what is in the wrong group, do not widen the bound — // and a correction that pays for itself is the correction working, not a // loophole in it. + // **`/contain` moved here in 0.27.0, and it is a correction rather than a way + // of making room** — the fourth time this sentence has been written, after + // 0.19.0's `/mcp` and `/provider`, 0.22.0's `/image` and `/copy`, and 0.26.0's + // `/profile`. `Turn` means a command acting on the work the turn just + // finished. `/contain on|off` acts on nothing that has happened: it decides + // whether every *later* turn is driven through the containment entry point, + // and it survives the turn that switched it. That is the identical argument + // that moved `/profile` one release ago, and `/contain` was the other command + // it applied to all along. + // + // It frees the slot `/undo` takes. `Turn` stood at ten of ten, and the rule + // this product wrote for itself when 0.25.0 filled it is to re-file what is in + // the wrong group rather than widen the bound. ( Group::Session, - &["/clear", "/resume", "/fork", "/profile", "/setup", "/exit"], + &[ + "/clear", "/resume", "/fork", "/profile", "/contain", "/setup", "/exit", + ], ), // **`/image`, `/copy` and `/copy diff` moved here in 0.22.0, and it is a // correction rather than a way of making room** — the same sentence 0.19.0 @@ -424,7 +446,12 @@ pub const GROUPS: &[(Group, &[&str])] = &[ // this session's and dies with it, for the reason the release contract // records under `open_questions`. "/effort", - "/contain", + // **`/undo` takes the slot `/contain` left.** It is the most + // `Turn`-shaped command in the product: it acts on the work the turn + // just finished, at whichever granularity the operator meant. See the + // note above `Group::Session` for why `/contain` was the one that + // moved rather than the bound. + "/undo", "/plan", "/steer", "/compact", @@ -1253,6 +1280,18 @@ pub enum Action { /// used when it is absent — proposed rather than imposed, and refused rather /// than overwritten when something is already there. Export(Taken), + /// Put work back, at the granularity the operator named. + /// + /// `Grain::Run` is what the rewind chord has always done and what a bare + /// `/undo` means. The other two are this release's, and both are public + /// io-harness calls this crate had never made. + Undo(crate::undo::Grain), + /// `/undo step` with no number, or a number that is not one. + /// + /// A variant rather than a fall-through to the whole run, which is the one + /// mistake this command must not make: an operator who typed a step and got + /// the entire run undone would have lost work they never asked to lose. + UndoNoStep, } /// Which export was asked for, and where it goes. @@ -1959,6 +1998,21 @@ pub fn parse(input: &str, keys: &Keys, theme: &Theme) -> Action { // ambiguity worth naming: an operator who wants a file called `trace` // types `./trace`. The alternative — a `--trace` flag — would be the only // flag on any command in this product. + // `step` is a word and everything else is a path, the same shape `/export` + // takes for `trace`. A bare `/undo` is the whole run, which is what the + // rewind chord already means, so the word and the chord cannot disagree. + "undo" => { + let mut rest = input.split_whitespace().skip(1); + match rest.next() { + None => Action::Undo(crate::undo::Grain::Run), + Some("step") => match rest.next().map(str::parse::) { + Some(Ok(step)) => Action::Undo(crate::undo::Grain::Step(step)), + // Never a fall-through to the run. See `Action::UndoNoStep`. + _ => Action::UndoNoStep, + }, + Some(path) => Action::Undo(crate::undo::Grain::File(path.to_string())), + } + } "export" => { let mut rest = input.split_whitespace().skip(1); match rest.next() { diff --git a/src/lib.rs b/src/lib.rs index ba794ad..f17c0d8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -103,10 +103,10 @@ pub mod stats; pub mod status; pub mod stdin; pub mod store; - pub mod term; pub mod theme; pub mod transcript; pub mod triage; +pub mod undo; pub mod verify; pub mod wizard; diff --git a/src/main.rs b/src/main.rs index 9f65235..e0c1e33 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2532,6 +2532,72 @@ async fn loop_over Result>( } } } + // The two new granularities. Both go through `observing`, + // which is what puts `EventKind::Reverted` on the screen — + // an event this product has never once emitted, because + // only the `_observed` form emits it. + Pick::UndoFile { run_id, path } => { + if io_cli::store::acts(index) { + let workspace = io_harness::tools::Workspace::new(session.root()); + match io_cli::undo::one_file(&workspace, &store, *run_id, path) { + Ok(answer) => { + app.record(Tone::Muted, io_cli::undo::said(path, &answer)) + } + Err(error) => app.record( + Tone::Error, + format!( + "{path} was not put back: {}", + io_cli::failure::said(&error) + ), + ), + } + } + } + Pick::UndoStep { run_id, step } => { + if io_cli::store::acts(index) { + let workspace = io_harness::tools::Workspace::new(session.root()); + let run = *run_id; + let at = *step; + let done = observing(&mut app, screen, |observer| { + io_cli::undo::one_step(&workspace, &store, run, at, observer) + })?; + match done { + Ok(answers) if answers.is_empty() => app.record( + Tone::Muted, + format!("step {at} wrote no files, so nothing changed"), + ), + Ok(answers) => { + for (path, answer) in &answers { + app.record( + Tone::Muted, + io_cli::undo::step_said(path, answer), + ); + } + // The order-sensitivity sentence, said only + // when something actually came back stale — + // otherwise it is advice about a problem the + // operator does not have. + if let Some(advice) = io_cli::undo::step_advice(&answers) { + app.record(Tone::Warning, advice); + } + } + Err(error) => app.record( + Tone::Error, + format!( + "step {at} was not undone: {}", + io_cli::failure::said(&error) + ), + ), + } + } + } + // The same `undo_whole_turn` the chord reaches, so the + // word and the keystroke cannot drift apart. + Pick::UndoRun => { + if io_cli::store::acts(index) { + undo_whole_turn(&mut app, screen, &mut session, &store, &seen)?; + } + } Pick::Export { path, content } => { if io_cli::store::acts(index) { let workspace = io_harness::tools::Workspace::with_policy( @@ -2737,37 +2803,14 @@ async fn loop_over Result>( None => app.say(Tone::Muted, "there is no turn to undo"), }, // The second. This is where the operator's files change. - Command::Rewind => match io_cli::rewind::last_turn(&mut session, &store) { - Ok(Some(undone)) => { - // The undone turn is where those numbers came from. - app.status.forget_run(); - app.forget_fleet(); - // As above: the last request belonged to the undone turn. - seen.forget(); - for (tone, line) in io_cli::rewind::undone_lines(&undone, &app.theme.glyphs) { - app.say(tone, line); - } - } - Ok(None) => app.say(Tone::Muted, "there is no turn to undo"), - // **`failure::said`, and not the raw `Display`.** Since 0.23.0 - // the undo can lose a head race with another `io`, and - // `Error::Conflict`'s own text calls the session id a run id and - // renders an expiry that a head conflict never populates. That - // sentence is exactly what `failure::advice` exists to replace, - // and this arm was the one path in the product still going - // around it. - // - // **And the old line said "nothing was undone", which is false.** - // `rewind::last_turn` restores the files before it attempts the - // head write, so a conflict leaves the operator's files back as - // they were with the conversation head where the other process - // put it. Saying nothing happened would send them looking for - // changes that are already gone. - Err(error) => app.say( - Tone::Error, - format!("the undo did not finish: {}", io_cli::failure::said(&error)), - ), - }, + // **Through `observing` since 0.27.0, which is what finally emits + // `EventKind::Rewound`.** The call was `rewind_run`, whose observed + // twin is the only thing that emits it — so the arm rendering that + // event in `src/events.rs` was unreachable from the day it was + // written until this one. + Command::Rewind => { + undo_whole_turn(&mut app, screen, &mut session, &store, &seen)?; + } Command::Slash(text) => match commands::parse(&text, app.keys(), &app.theme) { Action::Print(lines) => { screen.commit(&lines).map_err(|error| error.to_string())?; @@ -3781,6 +3824,75 @@ async fn loop_over Result>( ), ), }, + Action::UndoNoStep => app.record( + Tone::Error, + "/undo step needs a step number — `/expand` lists them, and a \ + bare `/undo` is the whole run" + .to_string(), + ), + // The two new granularities descend into a confirmation; the whole + // run keeps the chord's own two-press path, which already + // discloses what a restore overwrites. + Action::Undo(grain) => match last_run(&session, &store).map(|turn| turn.run_id) { + None => app.record( + Tone::Error, + "there is nothing to undo — this session has taken no turns".to_string(), + ), + Some(run_id) => match &grain { + io_cli::undo::Grain::File(path) => { + let (title, rows) = io_cli::undo::confirm_file(path); + picker = Some(( + Picker::new(title, rows), + Pick::UndoFile { + run_id, + path: path.clone(), + }, + )); + } + io_cli::undo::Grain::Step(step) => { + let (title, rows) = io_cli::undo::confirm_step(*step); + picker = Some(( + Picker::new(title, rows), + Pick::UndoStep { + run_id, + step: *step, + }, + )); + } + // The bare form confirms like the other two rather than + // arming like the chord. The arming is a property of a + // *keystroke* — one press to warn, a second to act — and + // a typed command has already been deliberate once. Both + // paths end in the same `rewind::last_turn`, so the word + // and the chord can never disagree about what an undo is. + io_cli::undo::Grain::Run => { + match io_cli::rewind::preview(&session, &store) { + Some(about) => { + let title = + io_cli::rewind::armed_line(&about, &app.theme.glyphs); + picker = Some(( + Picker::new( + title, + vec![ + Row::with_detail( + io_cli::store::LEAVE_IT, + "the turn stands", + ), + Row::with_detail( + "undo the whole turn", + "its files, its notes, its queued children \ + and the conversation head", + ), + ], + ), + Pick::UndoRun, + )); + } + None => app.say(Tone::Muted, "there is no turn to undo"), + } + } + }, + }, // Both halves build their content HERE, before the confirmation, // so the operator agrees to a file that exists rather than to an // intention. A build that fails says so and opens nothing. @@ -5499,6 +5611,92 @@ fn commit_transcript( } /// The last run of this session, if it has had one. +/// Undo the whole turn, and say what it did. +/// +/// **One implementation, two doors.** The rewind chord reaches it through +/// `Command::Rewind` after its two keystrokes; `/undo` with no argument reaches +/// it through a confirmation. The word and the chord must never disagree about +/// what an undo *is*, and the only way to guarantee that is for there to be one +/// of them. +/// +/// **Through [`observing`] since 0.27.0, which is what finally emits +/// `EventKind::Rewound`.** The call underneath was `rewind_run`, whose observed +/// twin is the only thing that emits it — so the arm rendering that event in +/// `src/events.rs` was unreachable from the day it was written until this one. +fn undo_whole_turn( + app: &mut App, + screen: &mut Screen>, + session: &mut Session, + store: &Store, + seen: &io_cli::context::Seen, +) -> Result<(), String> { + match observing(app, screen, |observer| { + io_cli::rewind::last_turn(session, store, observer) + })? { + Ok(Some(undone)) => { + // The undone turn is where those numbers came from. + app.status.forget_run(); + app.forget_fleet(); + // As above: the last request belonged to the undone turn. + seen.forget(); + for (tone, line) in io_cli::rewind::undone_lines(&undone, &app.theme.glyphs) { + app.say(tone, line); + } + } + Ok(None) => app.say(Tone::Muted, "there is no turn to undo"), + // **`failure::said`, and not the raw `Display`.** Since 0.23.0 the undo + // can lose a head race with another `io`, and `Error::Conflict`'s own + // text calls the session id a run id and renders an expiry that a head + // conflict never populates. That sentence is exactly what + // `failure::advice` exists to replace, and this arm was the one path in + // the product still going around it. + // + // **And the old line said "nothing was undone", which is false.** + // `rewind::last_turn` restores the files before it attempts the head + // write, so a conflict leaves the operator's files back as they were with + // the conversation head where the other process put it. Saying nothing + // happened would send them looking for changes that are already gone. + Err(error) => app.say( + Tone::Error, + format!("the undo did not finish: {}", io_cli::failure::said(&error)), + ), + } + Ok(()) +} + +/// Run something that wants an `Observer`, and commit whatever it emitted. +/// +/// **This is what makes `EventKind::Rewound` and `EventKind::Reverted` reach the +/// screen at all.** Both are emitted only by the `_observed` forms of the rewind +/// functions, and every one of those forms takes an observer — but an undo +/// happens between turns, at a keystroke, where the driver's own observer +/// composition does not exist. So one is built for the act and drained +/// immediately afterwards. +/// +/// **The events are rendered and never fed to [`App::event`].** That method +/// folds into the status line, the fleet and the token totals, which belong to a +/// *run*; an undo is not a run, and 0.20.0 recorded what happens when foreign +/// events go through it — inflated session totals, a replaced provider, and a +/// grafted tree. `Events::event` is the renderer alone, which is exactly what is +/// wanted here. +fn observing( + app: &mut App, + screen: &mut Screen>, + act: impl FnOnce(&dyn io_harness::Observer) -> T, +) -> Result { + let (bridge, mut events) = io_cli::bridge::channel(); + let out = act(&bridge); + // Synchronous and already finished, so the queue is complete: `try_recv` + // drains it without waiting, and waiting is what would hang the interface. + while let Ok(event) = events.try_recv() { + let lines = app.events.event(&event, std::time::Duration::ZERO); + if !lines.is_empty() { + screen.commit(&lines).map_err(|error| error.to_string())?; + } + } + Ok(out) +} + fn last_run(session: &Session, store: &Store) -> Option { session .transcript(store) @@ -5977,6 +6175,25 @@ enum Pick { /// A confirmation over a compaction. Carries nothing: the operation takes no /// argument, and the figures it reports are read either side of the call. StoreCompact, + /// A confirmation over putting one file back, from the run that wrote it. + UndoFile { + /// The run whose snapshot the file comes from. + run_id: i64, + /// The path, as the operator named it. + path: String, + }, + /// A confirmation over reverse-applying one step's diff. + UndoStep { + /// The run the step belongs to. + run_id: i64, + /// The step, one-based as `/expand` shows them. + step: u32, + }, + /// A confirmation over undoing the whole turn — the typed form of what the + /// rewind chord does with two keystrokes. Carries nothing: `rewind::last_turn` + /// reads the head itself, and a run id carried from before the confirmation + /// could name a turn another `io` has since moved off the head. + UndoRun, /// A confirmation over one export, carrying the bytes it will write. /// /// The content is built before the confirmation and carried rather than diff --git a/src/rewind.rs b/src/rewind.rs index 416cbce..f9a5523 100644 --- a/src/rewind.rs +++ b/src/rewind.rs @@ -39,7 +39,7 @@ use crate::glyphs::Glyphs; use crate::theme::Tone; use io_harness::tools::Workspace; -use io_harness::{rewind_run, Error, Rewind, Session, Store}; +use io_harness::{rewind_run_observed, Error, Observer, Rewind, Session, Store}; /// What the armed prompt says before anything is undone. /// @@ -180,7 +180,11 @@ pub fn preview(session: &Session, store: &Store) -> Option { /// tries by hand: a two-turn undo still leaves a plausible-looking head, so a /// build that got this wrong would pass every test written against a longer /// conversation. -pub fn last_turn(session: &mut Session, store: &Store) -> Result, Error> { +pub fn last_turn( + session: &mut Session, + store: &Store, + observer: &dyn Observer, +) -> Result, Error> { let Some(head) = session.head() else { return Ok(None); }; @@ -197,7 +201,7 @@ pub fn last_turn(session: &mut Session, store: &Store) -> Result, // snapshots is a path whose write already passed the operator's gate, so // putting it back cannot reach anywhere the run could not. let workspace = Workspace::new(session.root()); - let rewound = rewind_run(&workspace, store, turn.run_id)?; + let rewound = rewind_run_observed(&workspace, store, turn.run_id, observer)?; let mut restored = Vec::new(); let mut declined = Vec::new(); diff --git a/src/undo.rs b/src/undo.rs new file mode 100644 index 0000000..71638a5 --- /dev/null +++ b/src/undo.rs @@ -0,0 +1,202 @@ +//! Putting work back at the granularity the operator actually meant. +//! +//! [`crate::rewind`] has undone a whole run since 0.4.0, and for three years of +//! releases that was the only instrument. It is the wrong size for most +//! mistakes: *this one file went wrong* and *that step should not have happened* +//! are the two things an operator reaches for, and an undo whose cost is larger +//! than the mistake it fixes is an undo nobody uses. +//! +//! Both granularities are public in io-harness and neither had ever been called +//! from this crate — [`io_harness::rewind`] for one path, and +//! [`io_harness::rewind_step_observed`] for one step's recorded diff. +//! +//! # The whole-run case changed too, and that is what makes two events reachable +//! +//! `src/rewind.rs` called the plain `rewind_run`, and **`EventKind::Rewound` is +//! emitted only inside `rewind_run_observed`** (`run.rs:804`). So in this +//! product's entire history that event has never fired once. The same is true of +//! `EventKind::Reverted`, which only `rewind_step_observed` emits (`run.rs:1011`). +//! Pointing both at the observed forms is not a stylistic change: it is the +//! difference between an undo that announces itself like every other thing this +//! interface draws and one that happens in silence. +//! +//! # Three traps, and the middle one will bite an operator +//! +//! **1. Reverting a step is order-sensitive, and io-harness will not guess.** +//! Reverse-applying a hunk while a *later* step's change still sits on top of it +//! finds context that has moved, and the honest answer is +//! [`Reverted::Stale`](io_harness::Reverted::Stale) with the file untouched — +//! never a fuzzy match that quietly corrupts it. Newest step first is the order +//! that works, and [`step_advice`] is what says so on screen when a stale answer +//! comes back, because "nothing happened" without that sentence reads as a bug. +//! +//! **2. A restore does not know about an edit made afterwards.** io-harness puts +//! the file back from the snapshot taken before the run first wrote it, and does +//! not compare that against what is on disk now. An edit the operator made by +//! hand after the turn is overwritten without a word, and io-cli cannot detect it +//! — the snapshot is not readable from here. `src/rewind.rs` already discloses +//! this before its second keystroke; [`confirm_file`] does the same. +//! +//! **3. The four answers are four answers.** [`io_harness::Rewind`] distinguishes +//! `Restored`, `Removed`, `NotKept(why)` and `NotRecorded`, and they mean +//! different things: the first two changed the workspace, the last two changed +//! nothing at all. Collapsing `NotKept` and `NotRecorded` into one sentence would +//! tell an operator "nothing to undo" about a file the run very much wrote, whose +//! previous contents were simply over the snapshot cap. +//! +//! # What the policy is, and whose it is +//! +//! io-harness's. [`io_harness::rewind`] restores through `Workspace::write_file`, +//! so the same path policy a write obeys applies; a **removal** asks +//! `Workspace::check_path` and refuses anything that is not an outright +//! `Effect::Allow`, because a deletion is not content a human can inspect +//! afterwards. Nothing here re-implements either check, and nothing here softens +//! a refusal. + +use io_harness::tools::Workspace; +use io_harness::{Error, Observer, Reverted, Rewind, Rewound, Store}; + +/// Which granularity an undo was asked for. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Grain { + /// One named path, from this run's snapshot. + File(String), + /// One step's recorded diff, reverse-applied. + Step(u32), + /// Everything the run did. + Run, +} + +impl Grain { + /// What to call it in a sentence. + pub fn label(&self) -> String { + match self { + Grain::File(path) => format!("the file {path}"), + Grain::Step(step) => format!("step {step}"), + Grain::Run => "the whole run".to_string(), + } + } +} + +/// Put one file back, from the snapshot taken before this run first wrote it. +/// +/// The answer is io-harness's own [`Rewind`], returned whole rather than reduced +/// to a boolean: see trap 3. `Restored` and `Removed` changed the workspace; +/// `NotKept` and `NotRecorded` changed nothing and say why. +pub fn one_file( + workspace: &Workspace, + store: &Store, + run_id: i64, + path: &str, +) -> Result { + io_harness::rewind(workspace, store, run_id, path) +} + +/// Undo one step's recorded diff, announcing it. +/// +/// **The observed form, so `EventKind::Reverted` fires.** One entry per path the +/// step wrote, in the order it wrote them; a step that wrote nothing comes back +/// empty and is not an error — asking to undo a step that only read files is a +/// reasonable question with a boring answer. +pub fn one_step( + workspace: &Workspace, + store: &Store, + run_id: i64, + step: u32, + observer: &dyn Observer, +) -> Result, Error> { + io_harness::rewind_step_observed(workspace, store, run_id, step, observer) +} + +/// Undo everything the run did, announcing it. +/// +/// **The observed form, so `EventKind::Rewound` fires — for the first time in +/// this product's history.** `src/rewind.rs` has called the plain `rewind_run` +/// since 0.4.0, which is why the event has an arm in `src/events.rs` that has +/// never been reached. +pub fn whole_run( + workspace: &Workspace, + store: &Store, + run_id: i64, + observer: &dyn Observer, +) -> Result { + io_harness::rewind_run_observed(workspace, store, run_id, observer) +} + +/// What one file's answer says, in the operator's words. +/// +/// Four sentences for four answers. The two that changed nothing say so first, +/// because that is the fact an operator has to notice. +pub fn said(path: &str, answer: &Rewind) -> String { + match answer { + Rewind::Restored(_) => format!("{path} is back as it was before the run"), + Rewind::Removed => format!("{path} is gone — the run had created it"), + Rewind::NotKept(why) => { + format!("{path} is unchanged: its previous contents were not kept ({why})") + } + Rewind::NotRecorded => { + format!("{path} is unchanged: this run recorded no restore point for it") + } + } +} + +/// What one step's answer says, per path. +pub fn step_said(path: &str, answer: &Reverted) -> String { + match answer { + Reverted::Applied(_) => format!("{path} is back to what it was before that step"), + Reverted::Stale(why) => format!("{path} is unchanged: {why}"), + // `Reverted` is `#[non_exhaustive]`-shaped in spirit — a third and fourth + // variant already exist — so the wildcard reports the honest conservative + // thing rather than guessing that something was applied. + other => format!("{path} is unchanged: {other:?}"), + } +} + +/// The sentence to add when a step's revert came back stale. +/// +/// Trap 1, said out loud. A stale answer with no explanation reads as a bug — +/// the operator asked for an undo, was told nothing happened, and has no way to +/// know that undoing the *newer* step first is what makes this one apply. +pub fn step_advice(answers: &[(String, Reverted)]) -> Option { + answers + .iter() + .any(|(_, answer)| matches!(answer, Reverted::Stale(_))) + .then(|| { + "a later step is still standing on top of this one — reverting is \ + order-sensitive, so undo the newest step first and this one applies" + .to_string() + }) +} + +/// The confirmation for putting one file back. +/// +/// Row 0 declines, like every confirmation this release adds. The disclosure is +/// trap 2 and it is in the row that acts, because it is what the operator is +/// agreeing to rather than a caveat about what they are declining. +pub fn confirm_file(path: &str) -> (String, Vec) { + ( + format!("Put {path} back as it was before this run?"), + vec![ + crate::picker::Row::with_detail(crate::store::LEAVE_IT, "the file is left as it is"), + crate::picker::Row::with_detail( + format!("put {path} back"), + "any edit you made by hand after the turn is overwritten", + ), + ], + ) +} + +/// The confirmation for undoing one step. +pub fn confirm_step(step: u32) -> (String, Vec) { + ( + format!("Undo what step {step} wrote?"), + vec![ + crate::picker::Row::with_detail(crate::store::LEAVE_IT, "nothing is changed"), + crate::picker::Row::with_detail( + format!("undo step {step}"), + "reverse-applies that step's diff; a newer step on the same lines \ + makes it stale and nothing changes", + ), + ], + ) +} diff --git a/tests/commands.rs b/tests/commands.rs index 119d5be..9069d18 100644 --- a/tests/commands.rs +++ b/tests/commands.rs @@ -84,6 +84,11 @@ fn the_commands_are_the_commands() { "/profile", "/effort", "/contain", + // 0.27.0 — the undo that is the size of the mistake, taking the + // `Turn` slot `/contain` left when it was re-filed into `Session`. + // `Turn` was ten of ten and the bound was not widened, which is the + // fourth time this product has made that correction. + "/undo", // 0.12.0 — the planning phase stopped being something // `[app.io-cli.containment]` switched on by accident, so it needs a // switch of its own. @@ -101,8 +106,8 @@ fn the_commands_are_the_commands() { // 0.27.0 — the third page about work already done, and the first that // can also change it. Under **inspect** because that is what its bare // form does, with every destructive verb behind a confirmation whose - // row 0 does nothing; it takes that group to nine and `/export` will - // take it to ten, the bound. + // row 0 does nothing; it and `/export` take that group to ten, which + // is the bound. "/store", // 0.27.0 — the other end of the same question: `/store` is what is // being kept, this is how the work gets out. It takes `Inspect` to @@ -319,10 +324,10 @@ fn f12_resume_says_it_answers_a_parked_run_rather_than_merely_reopening_a_sessio // release growing a command and losing another. assert_eq!( COMMANDS.len(), - 35, - "0.27.0 adds `/store` and `/export` on top of the thirty-three 0.26.0 \ - shipped, and will add `/undo` before it is done; a number this gate does \ - not name means one arrived unrecorded", + 36, + "0.27.0 adds exactly three commands — `/store`, `/export` and `/undo` — on \ + top of the thirty-three 0.26.0 shipped; a thirty-seventh here means one \ + arrived unrecorded", ); } diff --git a/tests/live.rs b/tests/live.rs index e02a5a9..e900d0c 100644 --- a/tests/live.rs +++ b/tests/live.rs @@ -924,7 +924,7 @@ async fn live_f13_work_survives_the_session() { "armed: {}", io_cli::rewind::armed_line(&about, &DARK.glyphs) ); - let undone = io_cli::rewind::last_turn(&mut session, &store) + let undone = io_cli::rewind::last_turn(&mut session, &store, &io_harness::Ignore) .expect("the rewind runs") .expect("there was a turn to undo"); for (tone, line) in io_cli::rewind::undone_lines(&undone, &DARK.glyphs) { diff --git a/tests/rewind.rs b/tests/rewind.rs index ef647da..1d14399 100644 --- a/tests/rewind.rs +++ b/tests/rewind.rs @@ -192,7 +192,7 @@ async fn an_edited_file_comes_back_and_a_created_file_goes_and_both_are_reported assert_eq!(fixture.bytes("notes.md"), AFTER.as_bytes()); assert_eq!(fixture.bytes("summary.md"), INVENTED.as_bytes()); - let undone = rewind::last_turn(&mut fixture.session, &fixture.store) + let undone = rewind::last_turn(&mut fixture.session, &fixture.store, &io_harness::Ignore) .expect("the rewind cannot fail") .expect("a turn was taken, so there is one to undo"); @@ -280,7 +280,7 @@ async fn memory_the_run_changed_is_put_back_and_memory_it_invented_is_removed() "the turn's wrong value must actually be in the store before it is undone", ); - let undone = rewind::last_turn(&mut fixture.session, &fixture.store) + let undone = rewind::last_turn(&mut fixture.session, &fixture.store, &io_harness::Ignore) .expect("the rewind cannot fail") .expect("a turn was taken"); @@ -338,7 +338,7 @@ async fn a_file_whose_previous_contents_were_not_kept_is_declined_and_left_untou let by_hand = b"what the operator typed instead\n"; std::fs::write(fixture.dir.path().join("logo.bin"), by_hand).expect("the file is writable"); - let undone = rewind::last_turn(&mut fixture.session, &fixture.store) + let undone = rewind::last_turn(&mut fixture.session, &fixture.store, &io_harness::Ignore) .expect("the rewind cannot fail") .expect("a turn was taken"); @@ -394,7 +394,7 @@ async fn rewinding_the_second_of_two_turns_leaves_the_head_on_the_first() { "the fixture needs two distinct turns", ); - let undone = rewind::last_turn(&mut fixture.session, &fixture.store) + let undone = rewind::last_turn(&mut fixture.session, &fixture.store, &io_harness::Ignore) .expect("the rewind cannot fail") .expect("a turn was taken"); @@ -435,7 +435,7 @@ async fn rewinding_the_only_turn_leaves_the_session_with_no_head_at_all() { "the fixture needs one turn", ); - let undone = rewind::last_turn(&mut fixture.session, &fixture.store) + let undone = rewind::last_turn(&mut fixture.session, &fixture.store, &io_harness::Ignore) .expect("the rewind cannot fail") .expect("a turn was taken"); @@ -515,7 +515,7 @@ async fn an_undo_is_refused_when_another_io_has_already_moved_the_head_on() { .set_session_head(fixture.session.id(), Some(theirs)) .expect("the other process advances the head"); - let refused = rewind::last_turn(&mut fixture.session, &fixture.store) + let refused = rewind::last_turn(&mut fixture.session, &fixture.store, &io_harness::Ignore) .expect_err("an undo whose head has moved underneath it must not succeed"); // The assertion is on the **refusal**, and deliberately so: the defect this @@ -578,7 +578,7 @@ async fn an_undo_over_a_store_no_other_io_touched_still_moves_the_head_back() { .turn_writing("now do it", &[("plan.md", AFTER)]) .await; - let undone = rewind::last_turn(&mut fixture.session, &fixture.store) + let undone = rewind::last_turn(&mut fixture.session, &fixture.store, &io_harness::Ignore) .expect("nothing raced, so the conditional write must match its row") .expect("a turn was taken"); @@ -594,7 +594,7 @@ fn a_session_that_has_taken_no_turns_undoes_nothing_and_does_not_fail() { rewind::preview(&fixture.session, &fixture.store).is_none(), "there is nothing to arm a confirmation prompt about", ); - let undone = rewind::last_turn(&mut fixture.session, &fixture.store) + let undone = rewind::last_turn(&mut fixture.session, &fixture.store, &io_harness::Ignore) .expect("an empty session is not an error"); assert!( undone.is_none(), diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 1fbbf2f..0b59545 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -430,6 +430,30 @@ impl Scripted { batches: Mutex::new(VecDeque::from(vec![batch])), } } + + /// One batch per **step**, so a turn takes as many steps as there are slices. + /// + /// [`Scripted::writing`] puts every file in one batch, which is one step. A + /// test about undoing *one step* of a run needs the steps to be distinct, and + /// the only way to get that is to make the provider answer more than once — + /// io-harness's step boundary is a completion, not a tool call. + /// + /// Steps are numbered from one by the run loop, so `in_steps(&[a, b])` writes + /// `a` at step 1 and `b` at step 2. + pub fn in_steps(steps: &[&[(&str, &str)]]) -> Self { + let batches = steps + .iter() + .map(|files| { + files + .iter() + .map(|(path, content)| write_call(path, content)) + .collect() + }) + .collect::>>(); + Self { + batches: Mutex::new(VecDeque::from(batches)), + } + } } impl Provider for Scripted { diff --git a/tests/undo.rs b/tests/undo.rs new file mode 100644 index 0000000..4b955d3 --- /dev/null +++ b/tests/undo.rs @@ -0,0 +1,434 @@ +//! Putting work back at three granularities, and the two events that proves. +//! +//! **The assertion that matters most here is that an event *arrives*.** +//! `EventKind::Rewound` is emitted only inside `rewind_run_observed` +//! (`run.rs:804`) and `EventKind::Reverted` only inside `rewind_step_observed` +//! (`run.rs:1011`). `src/rewind.rs` called the plain `rewind_run` from 0.4.0 +//! until this release, so neither has ever fired in this product's history — and +//! `src/events.rs` has had an arm for one of them the whole time, unreachable. +//! +//! So the gates below collect what an observer was handed. Asserting the +//! rendered sentence would pass just as happily on the unobserved call, which is +//! the defect: the words come from io-cli either way, and only the event says +//! which function was called. +//! +//! **The workspace assertions are on bytes on disk, never on a message.** An +//! undo that reports success and leaves the file alone is the whole failure mode. + +mod support; + +use std::sync::Mutex; + +use io_cli::undo::{ + confirm_file, confirm_step, one_file, one_step, said, step_advice, step_said, whole_run, Grain, +}; +use io_harness::tools::Workspace; +use io_harness::{ + ApproveAll, EventKind, Flow, Observer, Policy, Reverted, Rewind, RunEvent, Session, Store, +}; +use support::Scripted; + +/// An observer that keeps every event it is handed. +/// +/// The only instrument that can tell `rewind_run` from `rewind_run_observed`. +#[derive(Default)] +struct Collected { + events: Mutex>, +} + +impl Observer for Collected { + fn event(&self, event: &RunEvent) -> Flow { + self.events + .lock() + .expect("the lock is not poisoned") + .push(event.kind.clone()); + Flow::Continue + } +} + +impl Collected { + fn kinds(&self) -> Vec { + self.events + .lock() + .expect("the lock is not poisoned") + .clone() + } + + fn saw_rewound(&self) -> bool { + self.kinds() + .iter() + .any(|kind| matches!(kind, EventKind::Rewound { .. })) + } + + fn saw_reverted(&self) -> Option { + self.kinds().iter().find_map(|kind| match kind { + EventKind::Reverted { undid_step, .. } => Some(*undid_step), + _ => None, + }) + } +} + +/// A temporary workspace, a store, and a session over the two. +/// +/// **Every run here is a real turn.** A restore point is written by io-harness's +/// own run loop through `Store::record_snapshot`, which is `pub(crate)` — so a +/// test cannot seed one, and a fixture that inserted rows by hand would be +/// testing a shape production does not produce. This is the same fixture +/// `tests/rewind.rs` has used since 0.4.0, for the same reason. +struct Fixture { + dir: tempfile::TempDir, + store: Store, + session: Session, +} + +impl Fixture { + fn new() -> Self { + 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"); + Self { + dir, + store, + session, + } + } + + /// One turn whose agent writes every one of `files`, in one step. + async fn turn_writing(&mut self, prompt: &str, files: &[(&str, &str)]) -> i64 { + self.drive(prompt, Scripted::writing(files)).await + } + + /// One turn that takes a step per slice, so the steps are distinct. + async fn turn_in_steps(&mut self, prompt: &str, steps: &[&[(&str, &str)]]) -> i64 { + self.drive(prompt, Scripted::in_steps(steps)).await + } + + async fn drive(&mut self, prompt: &str, provider: Scripted) -> i64 { + self.session + .turn( + prompt, + &provider, + &self.store, + &Policy::permissive(), + &ApproveAll, + ) + .await + .expect("a scripted turn cannot fail"); + let head = self.session.head().expect("the turn is on the head"); + self.store + .session_turn(head) + .expect("the turn is readable") + .expect("the turn is there") + .run_id + } + + fn workspace(&self) -> Workspace { + Workspace::new(self.dir.path()) + } + + fn read(&self, path: &str) -> Option { + std::fs::read_to_string(self.dir.path().join(path)).ok() + } + + fn exists(&self, path: &str) -> bool { + self.dir.path().join(path).exists() + } +} + +// --------------------------------------------------------------------------- +// F6 — one file, and four answers that stay four answers +// --------------------------------------------------------------------------- + +/// **F6 — a modified file comes back with its pre-run bytes.** +/// +/// Asserted on the workspace, never on the returned sentence. +#[tokio::test] +async fn f6_a_modified_file_comes_back_as_it_was() { + let mut fixture = Fixture::new(); + std::fs::write(fixture.dir.path().join("thing.rs"), "the original\n") + .expect("the file starts out as the operator left it"); + let run = fixture + .turn_writing("edit it", &[("thing.rs", "what the agent wrote\n")]) + .await; + assert_eq!( + fixture.read("thing.rs").as_deref(), + Some("what the agent wrote\n") + ); + + let answer = + one_file(&fixture.workspace(), &fixture.store, run, "thing.rs").expect("the undo runs"); + + assert!(matches!(answer, Rewind::Restored(_)), "{answer:?}"); + assert_eq!( + fixture.read("thing.rs").as_deref(), + Some("the original\n"), + "the bytes on disk are the pre-run bytes", + ); +} + +/// **F6 — a file the run created is removed, not emptied.** +#[tokio::test] +async fn f6_a_file_the_run_created_is_removed() { + let mut fixture = Fixture::new(); + let run = fixture + .turn_writing("make it", &[("new.rs", "brand new\n")]) + .await; + assert!(fixture.exists("new.rs")); + + let answer = + one_file(&fixture.workspace(), &fixture.store, run, "new.rs").expect("the undo runs"); + + assert_eq!(answer, Rewind::Removed); + assert!( + !fixture.exists("new.rs"), + "a created file is gone, not left empty", + ); +} + +/// **F6 — a path the run never touched answers `NotRecorded` and changes +/// nothing.** +#[tokio::test] +async fn f6_a_path_the_run_never_touched_is_not_recorded() { + let mut fixture = Fixture::new(); + let run = fixture + .turn_writing("touch one", &[("written.rs", "by the agent\n")]) + .await; + std::fs::write(fixture.dir.path().join("untouched.rs"), "mine\n") + .expect("the operator's own file"); + + let answer = + one_file(&fixture.workspace(), &fixture.store, run, "untouched.rs").expect("the undo runs"); + + assert_eq!(answer, Rewind::NotRecorded); + assert_eq!( + fixture.read("untouched.rs").as_deref(), + Some("mine\n"), + "an unrecorded path is left exactly alone", + ); +} + +/// **F6 — the four answers are four sentences.** +/// +/// `NotKept` and `NotRecorded` in particular: collapsing them tells an operator +/// "nothing to undo" about a file the run very much wrote, whose previous +/// contents were merely over the snapshot cap. +#[test] +fn f6_the_four_answers_say_four_different_things() { + let restored = said("a.rs", &Rewind::Restored(io_harness::tools::Wrote::Changed)); + let removed = said("a.rs", &Rewind::Removed); + let not_kept = said("a.rs", &Rewind::NotKept("over the cap".to_string())); + let not_recorded = said("a.rs", &Rewind::NotRecorded); + + let all = [&restored, &removed, ¬_kept, ¬_recorded]; + for (i, one) in all.iter().enumerate() { + for (j, other) in all.iter().enumerate() { + if i != j { + assert_ne!(one, other, "every answer is its own sentence"); + } + } + } + assert!(not_kept.contains("not kept"), "{not_kept}"); + assert!( + not_kept.contains("over the cap"), + "the reason survives: {not_kept}" + ); + assert!(not_recorded.contains("no restore point"), "{not_recorded}"); + assert!( + removed.contains("created"), + "a removal says why it was a removal: {removed}", + ); +} + +// --------------------------------------------------------------------------- +// F7 — one step, the rest of the run untouched, and the events that prove it +// --------------------------------------------------------------------------- + +/// **F7 — undoing one step leaves the other step's file exactly as the run left +/// it, and `EventKind::Reverted` arrives.** +/// +/// The event is the assertion that proves the `_observed` form is what was +/// called. A rendered sentence would be identical either way. +#[tokio::test] +async fn f7_one_step_is_undone_and_the_rest_of_the_run_stands() { + let mut fixture = Fixture::new(); + std::fs::write(fixture.dir.path().join("two.rs"), "two before\n").expect("a starting file"); + std::fs::write(fixture.dir.path().join("three.rs"), "three before\n").expect("a starting file"); + let run = fixture + .turn_in_steps( + "two steps", + &[ + &[("two.rs", "two after\n")], + &[("three.rs", "three after\n")], + ], + ) + .await; + + // Step 2 is the second batch, because the run loop numbers from one. + let watcher = Collected::default(); + let answers = + one_step(&fixture.workspace(), &fixture.store, run, 1, &watcher).expect("the undo runs"); + + assert!( + !answers.is_empty(), + "step 1 wrote a file, so it has an answer" + ); + assert_eq!( + fixture.read("two.rs").as_deref(), + Some("two before\n"), + "the undone step's file is back", + ); + assert_eq!( + fixture.read("three.rs").as_deref(), + Some("three after\n"), + "and the other step's is exactly as the run left it", + ); + assert_eq!( + watcher.saw_reverted(), + Some(1), + "EventKind::Reverted arrived, naming the step — which only the observed \ + form emits, and which nothing in this product had ever emitted", + ); +} + +/// **F7 — undoing the whole run emits `EventKind::Rewound`.** +/// +/// The event this product has never fired. `src/rewind.rs:200` called the plain +/// `rewind_run` from 0.4.0 until this release, so the arm rendering this in +/// `src/events.rs` was unreachable from the day it was written. +#[tokio::test] +async fn f7_the_whole_run_emits_rewound() { + let mut fixture = Fixture::new(); + std::fs::write(fixture.dir.path().join("one.rs"), "before\n").expect("a starting file"); + let run = fixture + .turn_writing("a whole run", &[("one.rs", "after\n")]) + .await; + + let watcher = Collected::default(); + let rewound = + whole_run(&fixture.workspace(), &fixture.store, run, &watcher).expect("the undo runs"); + + assert!(!rewound.files.is_empty(), "there was a file to put back"); + assert_eq!( + fixture.read("one.rs").as_deref(), + Some("before\n"), + "and it is back", + ); + assert!( + watcher.saw_rewound(), + "EventKind::Rewound arrived — the assertion that the observed form was \ + called, which no rendered sentence could make", + ); +} + +/// A step that wrote nothing is not an error, and says so. +#[tokio::test] +async fn a_step_that_wrote_nothing_changes_nothing_and_is_not_an_error() { + let mut fixture = Fixture::new(); + let run = fixture.turn_writing("read only", &[]).await; + + let watcher = Collected::default(); + let answers = + one_step(&fixture.workspace(), &fixture.store, run, 1, &watcher).expect("not an error"); + + assert!( + answers.is_empty(), + "nothing was written, so nothing came back" + ); +} + +/// **The order-sensitivity sentence is said only when something came back +/// stale.** +/// +/// Advice about a problem the operator does not have is noise, and noise in a +/// destructive surface is what teaches people to stop reading it. +#[test] +fn the_stale_advice_is_given_only_when_something_is_stale() { + let clean = vec![( + "a.rs".to_string(), + Reverted::Applied(io_harness::tools::Wrote::Changed), + )]; + assert_eq!(step_advice(&clean), None, "nothing stale, nothing said"); + + let stale = vec![("a.rs".to_string(), Reverted::Stale("moved".to_string()))]; + let advice = step_advice(&stale).expect("a stale answer earns the sentence"); + assert!(advice.contains("newest step first"), "{advice}"); + assert!( + advice.contains("order-sensitive"), + "and says why, or it reads as a bug: {advice}", + ); + + let said = step_said("a.rs", &Reverted::Stale("context moved".to_string())); + assert!( + said.contains("unchanged"), + "a stale step changed nothing: {said}" + ); + assert!( + said.contains("context moved"), + "the reason survives: {said}" + ); +} + +// --------------------------------------------------------------------------- +// F10 — the group re-file +// --------------------------------------------------------------------------- + +/// **F10 — `/contain` is in `Session`, `/undo` is in `Turn`, `/store` and +/// `/export` are in `Inspect`, and no group is longer than ten.** +#[test] +fn f10_the_groups_are_where_this_release_put_them() { + use io_cli::commands::{group_of, Group, COMMANDS, GROUPS}; + + assert_eq!(group_of("/contain"), Some(Group::Session)); + assert_eq!(group_of("/undo"), Some(Group::Turn)); + assert_eq!(group_of("/store"), Some(Group::Inspect)); + assert_eq!(group_of("/export"), Some(Group::Inspect)); + + for (group, names) in GROUPS { + assert!( + names.len() <= 10, + "{group:?} holds {} commands; ten is the bound", + names.len(), + ); + } + assert_eq!( + COMMANDS.len(), + 36, + "0.27.0 adds exactly three commands to the thirty-three 0.26.0 shipped", + ); +} + +/// The confirmations decline at row 0, like every other one this release adds. +#[test] +fn the_undo_confirmations_decline_at_row_zero() { + for (what, rows) in [ + ("file", confirm_file("src/a.rs").1), + ("step", confirm_step(4).1), + ] { + assert_eq!(rows[0].label, io_cli::store::LEAVE_IT, "{what}"); + assert!(!io_cli::store::acts(0), "{what}"); + assert!(io_cli::store::acts(1), "{what}"); + } +} + +/// The file confirmation discloses the one thing an operator can lose. +/// +/// io-harness restores from the snapshot taken before the run first wrote the +/// path and does not compare it against what is on disk now, so an edit made by +/// hand after the turn is overwritten without a word. io-cli cannot detect that; +/// it can say so before the keystroke, which is what this asserts. +#[test] +fn the_file_confirmation_discloses_what_a_restore_overwrites() { + let (_title, rows) = confirm_file("src/a.rs"); + let detail = rows[1].detail.clone().unwrap_or_default(); + + assert!(detail.contains("by hand"), "{detail}"); + assert!(detail.contains("overwritten"), "{detail}"); +} + +/// The grains name themselves in the words the surface uses. +#[test] +fn the_grains_name_themselves() { + assert!(Grain::File("src/a.rs".into()).label().contains("src/a.rs")); + assert!(Grain::Step(7).label().contains('7')); + assert_eq!(Grain::Run.label(), "the whole run"); +} From 665a44d2252eebcbb9e36b92f63d2df8722c108d Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Fri, 28 Aug 2026 17:40:25 +0530 Subject: [PATCH 4/8] feat(events): draw the one silence that had no route, and check the rest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nine events this release set out to draw were already in src/triage.rs with a disposition of Silent and a written route — the surface by which the fact reaches the operator instead. Eight of those routes are better arguments than drawing would be: `reverted` and `rewound` route to io-cli's own rewind summary, so drawing them would double-report the undo this release just wired up, and `plan_proposed` routes to the overlay showing that same plan. One route was wrong. `speculated` named `io exec --json` and the durable trace, and both are places a person goes deliberately, afterwards, already suspecting something. A route is meant to name where a fact reaches somebody who was not looking for it. It draws a line now, carrying what reading ahead bought and what it cost — and only when something was discarded, because io-harness emits the event whenever anything was started at all, and a line in every transcript saying nothing went wrong is a line nobody reads. The set gate this release planned to build already existed: tests/triage.rs reads `pub enum EventKind` out of the pinned io-harness source and fails by name when the two sets differ. What it lacked was any check on the route column, so a silence could name a surface that does not exist. It cannot now. tests/glyphs.rs caught the new arm by itself and by name, which is what it was written for in 0.14.0. --- src/events.rs | 33 +++++++++++++++ src/triage.rs | 20 ++++++--- tests/glyphs.rs | 9 ++++ tests/triage.rs | 107 +++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 159 insertions(+), 10 deletions(-) diff --git a/src/events.rs b/src/events.rs index f6ff761..adb0fb9 100644 --- a/src/events.rs +++ b/src/events.rs @@ -1606,6 +1606,39 @@ impl Events { lines.push(Line::from("")); lines } + // **What starting early bought, and what it cost.** 0.27.0, and the + // one silence in `triage::TRIAGE` that had no route to a surface an + // operator uses: it went to `io exec --json` and the durable trace, + // which are both places somebody goes deliberately, afterwards, + // already suspecting something. + // + // `discarded` is the figure worth the line. A read started before the + // model had finished asking and then thrown away is work that was paid + // for and not used, and it is the only number here an operator can act + // on — by turning speculation off. `started` alone would read as a + // brag about concurrency. + // + // **Only when something was discarded.** io-harness emits this + // whenever `started > 0` (`run/step.rs:1272`), so a step that + // speculated perfectly would otherwise put a line in every transcript + // saying nothing happened. A run where speculation always pays is a + // run with nothing to report, which is the same rule the sandbox arm + // above follows for a cap that was not hit. + EventKind::Speculated { + started, + used, + discarded, + } if *discarded > 0 => { + let mut lines = self.flush_text(); + lines.push(Line::from(Span::styled( + format!( + "{}read ahead{separator}{used} of {started} used, {discarded} discarded", + leader(separator), + ), + theme.style(Tone::Muted), + ))); + lines + } // Every kind that commits no line of its own, and the one case that // is a defect rather than a decision. // diff --git a/src/triage.rs b/src/triage.rs index 8b775f0..13345b3 100644 --- a/src/triage.rs +++ b/src/triage.rs @@ -183,11 +183,21 @@ pub const TRIAGE: &[(&str, Disposition, &str)] = &[ Disposition::Status, "the web field, with its verdict", ), - ( - "speculated", - Disposition::Silent, - "`io exec --json` and the durable trace; nothing about the step's own events moves", - ), + // **0.27.0 — the one silence in this table with no route to any surface an + // operator uses.** It read "`io exec --json` and the durable trace", and both + // of those are places a person goes deliberately, afterwards, having already + // suspected something. A route is supposed to name where the fact reaches + // somebody who was not looking for it, and this one never did. + // + // What it says is worth a line, too: reads started before the model had + // finished asking, and how many were thrown away. Started-and-discarded is + // work that was paid for, and it is the only figure in this product an + // operator can act on by turning speculation off. + // + // The other eight silences reviewed in the same pass keep their routes, + // which are better arguments than drawing them would be — see + // `US-IO-CLI-0.27.0-I04`. + ("speculated", Disposition::Line, "the speculation line"), ( "plugin_dropped", Disposition::Silent, diff --git a/tests/glyphs.rs b/tests/glyphs.rs index 4eeeed1..fc47e52 100644 --- a/tests/glyphs.rs +++ b/tests/glyphs.rs @@ -582,6 +582,15 @@ fn every_event_this_release_renders_draws_in_ascii() { backend: Some("macos-sandbox-exec".into()), }, EventKind::Stalled, + // 0.27.0 — the one silence that gained a line, and it carries the muted + // leader and a separator, both of which differ between the two glyph + // sets. `discarded` is non-zero because the arm declines a step that + // speculated perfectly, and a declined event renders nothing to sweep. + EventKind::Speculated { + started: 3, + used: 1, + discarded: 2, + }, ]; // **The list above is checked against the renderer, not trusted.** It is diff --git a/tests/triage.rs b/tests/triage.rs index 92f6cce..2b658e3 100644 --- a/tests/triage.rs +++ b/tests/triage.rs @@ -254,11 +254,6 @@ fn a_status_or_silent_kind_commits_nothing() { host: "example.com:443".into(), permitted: true, }, - EventKind::Speculated { - started: 2, - used: 1, - discarded: 1, - }, EventKind::PluginDropped { plugin: "acme".into(), why: "manifest unreadable".into(), @@ -372,3 +367,105 @@ fn the_dispositions_are_the_three_the_contract_names() { "if almost everything is a line, nothing was triaged: {status} status, {silent} silent", ); } + +/// **0.27.0 — the one silence that had no route, and now has a line.** +/// +/// `speculated` routed to `io exec --json` and the durable trace, and both of +/// those are places somebody goes deliberately, afterwards, already suspecting +/// something. A route is meant to name where the fact reaches an operator who +/// was *not* looking for it. The other eight silences reviewed in the same pass +/// keep theirs, because they are better arguments than drawing would be — see +/// `US-IO-CLI-0.27.0-I04`. +#[test] +fn f9_a_discarded_read_is_drawn_and_a_perfect_one_is_not() { + let mut events = Events::new(DARK); + + assert_eq!( + triage::disposition("speculated"), + Some(Disposition::Line), + "speculated is a line since 0.27.0", + ); + + let lines = events.event( + &event(EventKind::Speculated { + started: 3, + used: 1, + discarded: 2, + }), + Duration::ZERO, + ); + let text = lines + .iter() + .flat_map(|line| line.spans.iter().map(|span| span.content.as_ref())) + .collect::(); + assert!(text.contains("read ahead"), "{text}"); + assert!(text.contains('3'), "started is on the line: {text}"); + assert!(text.contains('2'), "and so is what was thrown away: {text}"); + + // **A step that speculated perfectly has nothing to report.** io-harness + // emits this whenever `started > 0`, so without the guard every transcript + // would carry a line per step saying nothing went wrong — and a line that is + // always there is a line nobody reads. + let quiet = events.event( + &event(EventKind::Speculated { + started: 4, + used: 4, + discarded: 0, + }), + Duration::ZERO, + ); + assert!( + quiet.is_empty(), + "nothing was discarded, so there is nothing to say: {quiet:?}", + ); + assert_eq!( + events.unknown(), + 0, + "declining an event is not the same as not knowing the kind", + ); +} + +/// **Every `Silent` route names a surface this product actually ships.** +/// +/// A route is the whole justification for a silence: the fact is not on screen +/// *because it reaches the operator another way*. Until this release that claim +/// was prose nobody checked, and `speculated` is what a route looks like when it +/// is wrong — it named two places a person only goes deliberately, afterwards. +/// +/// So every route must name a command this build registers, or one of the two +/// machine surfaces. A route naming a command that does not exist is a silence +/// with nothing behind it. +#[test] +fn f9_every_silent_route_names_a_surface_that_exists() { + let commands: Vec<&str> = io_cli::commands::COMMANDS + .iter() + .map(|(name, _)| *name) + .collect(); + + for (name, disposition, route) in triage::TRIAGE { + if *disposition != Disposition::Silent { + continue; + } + assert!( + !route.is_empty(), + "{name} is silent and says nothing about why" + ); + + // A route may name a command, one of the two machine surfaces, or a + // surface of this interface that is not a slash command — the overlay, a + // committed summary, the answer itself. The first is the one that can go + // stale, so it is the one that is checked: any `/word` a route names must + // be a command this build registers. + for word in route.split_whitespace() { + let candidate = word.trim_matches(|c: char| !c.is_ascii_alphanumeric() && c != '/'); + if let Some(slash) = candidate.strip_prefix('/').map(|rest| format!("/{rest}")) { + if slash.len() > 1 { + assert!( + commands.contains(&slash.as_str()), + "{name} routes to {slash}, which this build does not register", + ); + } + } + } + } +} From d75dbdb00471b17dd5ec3337d1927b8272b7c863 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Fri, 28 Aug 2026 17:42:06 +0530 Subject: [PATCH 5/8] test(dependencies): the store operations live in one module, and no model can reach them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delete_session`, `sweep_sessions` and `compact()` are each named in exactly one file, and it is the one whose whole subject is the store. The roadmap's words for /store are that no model can call any of it; "nothing was added" is not assertable, so this asserts its reachable form — a second module naming one of these is the first step towards a caller that is not a keystroke. The other half is that no source file calls `with_tools`, which is how a model is handed reach at all. Both sweeps strip comments, and that was verified by writing the forbidden name into one and watching nothing fail. --- tests/dependencies.rs | 70 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/tests/dependencies.rs b/tests/dependencies.rs index a3b7d17..5e22583 100644 --- a/tests/dependencies.rs +++ b/tests/dependencies.rs @@ -950,3 +950,73 @@ fn n4_the_syntax_set_is_loaded_lazily_and_only_by_the_diff_renderer() { "the driver reached for the highlighter, which puts it on the startup path", ); } + +/// **N1 — the three destructive store operations are named in exactly one +/// module each, and no model can reach any of them.** +/// +/// The roadmap's words for `/store` are *none of which any model can call*, and +/// the way that stays true is by adding nothing: io-harness's workspace tool set +/// contains nothing that reaches `delete_session`, `sweep_sessions` or +/// `compact`, and this release registers no tool, no MCP server and no skill +/// that could. +/// +/// **"Nothing was added" is not assertable, so this asserts the reachable form +/// of it**: each call is named in one file, that file is `src/store.rs`, and +/// `src/store.rs` is reached from command dispatch rather than from anything on +/// a run's path. A second module naming one of these would be the first step +/// towards a caller that is not a keystroke, and it fails here by name. +/// +/// The needles are assembled at run time for the reason `tests/store.rs` records +/// at length: a gate that reads source cannot spell the thing it forbids, or its +/// own array is the first match. io-cli has now hit that in 0.16.0, 0.19.0, +/// 0.25.0, 0.26.0 and twice in 0.27.0. +#[test] +fn n1_the_store_operations_live_in_one_module_and_no_model_can_reach_them() { + let calls = [ + format!("{}_{}", "delete", "session"), + format!("{}_{}", "sweep", "sessions"), + format!("{}{}", "compact", "()"), + ]; + + for call in &calls { + let named: Vec = sources() + .into_iter() + .filter(|(_, text)| code_of(text).contains(call.as_str())) + .map(|(path, _)| { + path.file_name() + .expect("a file name") + .to_string_lossy() + .into_owned() + }) + .collect(); + + assert_eq!( + named, + vec!["store.rs".to_string()], + "`{call}` must be named in exactly one module, and it must be the one \ + whose whole subject is the store; found it in {named:?}", + ); + } +} + +/// **N1 — this release registers no tool, no MCP server and no skill.** +/// +/// The other half of *no model can call it*. A tool is how a model reaches +/// anything at all, so the honest check is that the set of ways this crate hands +/// the model a capability did not grow: `with_tools` is io-harness's own field +/// and this crate never sets it, and the two surfaces that do add capability — +/// `[[mcp]]` servers and skills — are read from the operator's configuration +/// rather than written by this release. +#[test] +fn n1_this_release_hands_the_model_no_new_capability() { + let forbidden = format!("{}_{}", "with", "tools"); + for (path, text) in sources() { + assert!( + !code_of(&text).contains(forbidden.as_str()), + "{} calls `{forbidden}`: io-cli does not choose the model's tool set, \ + and a release that started to would be handing it reach this crate \ + has never granted", + path.display(), + ); + } +} From da8ef6ef57a76f4b104371d966a6ac68a1daa40c Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Fri, 28 Aug 2026 17:46:13 +0530 Subject: [PATCH 6/8] test(narrow): the new surfaces fit at six widths, and no key was added MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every confirmation this release adds is drawn through the real render buffer at six widths in both glyph sets, with a workspace path chosen not to fit. The property names no number: no row overflows, and row 0 is still the declining row — a confirmation that dropped its "leave it" on a narrow terminal would leave an operator with only the destructive choice on screen. That is 0.26.0's correction applied. A gate that picks a width is asserting arithmetic it will get wrong, and that release's own narrow fixture passed at eighty because the field could not fit either way. N3 is asserted as the absence of a configuration key, because that is the only way this release could have changed what a turn is built from: every surface it adds is a command, none is configured, and none reaches contract::session. The contract's existing Debug-equality gates pass unamended, which is the rest of the same claim made by instruments this release did not touch. --- tests/contract.rs | 28 +++++++++ tests/narrow.rs | 149 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+) diff --git a/tests/contract.rs b/tests/contract.rs index f92a1c7..f271519 100644 --- a/tests/contract.rs +++ b/tests/contract.rs @@ -1896,3 +1896,31 @@ fn f11_asking_for_the_default_explicitly_changes_nothing() { let built = io_cli::contract::configured("a goal", root(), &agrees); assert!(built.detached_spawns); } + +/// **N3 — 0.27.0 adds no configuration key, so an operator who runs none of it +/// gets the contract 0.26.0 built.** +/// +/// Every surface this release adds is a *command*: `/store`, `/export`, `/undo`. +/// None of them is configured, none is read at startup, and none reaches +/// `contract::session` at all. So the strongest statement of N3 is that the key +/// catalogue did not move — a new key is the only way this release could have +/// changed what a turn is built from, and this is the one place that would show. +/// +/// The number is written out rather than derived, which is the same choice +/// `tests/commands.rs` makes about the command inventory and for the same +/// reason: growing the settings surface should be a decision somebody records +/// here, not a line somebody adds elsewhere. +/// +/// Sabotage: add a key to `CATALOGUE` — under which only this fails, and it +/// fails by saying a release that promised to add no configuration added one. +#[test] +fn n3_this_release_adds_no_configuration_key() { + let before_0_27_0 = 37; + assert_eq!( + io_cli::configure::CATALOGUE.len(), + before_0_27_0, + "0.27.0 adds three commands and no keys; a different number here means a \ + release that promised an operator nothing would change gave them \ + something to configure", + ); +} diff --git a/tests/narrow.rs b/tests/narrow.rs index 15ef6ed..9ac32e9 100644 --- a/tests/narrow.rs +++ b/tests/narrow.rs @@ -2248,3 +2248,152 @@ fn f11_status_folds_at_eighty_columns_rather_than_losing_the_end_of_a_row() { } } } + +/// **N2 — every surface 0.27.0 adds fits eighty columns in both glyph sets.** +/// +/// The store page, the four confirmations, and the reports each operation +/// writes. Drawn through the real render buffer for the picker, and measured as +/// built lines for the page and the reports, which are committed into the +/// terminal's own scrollback rather than drawn into the viewport. +/// +/// **Width-independent where it can be.** 0.26.0's lesson is that a gate which +/// picks a width is asserting arithmetic it will get wrong — its own +/// narrow-fixture test passed at eighty because the fixture could not fit the +/// field either way. So the confirmations are checked at six widths and the +/// property is the same at all of them: no row overflows, and the declining row +/// survives, because a confirmation that dropped its "leave it" at a narrow +/// terminal would leave an operator with only the destructive choice on screen. +#[test] +fn n2_every_store_confirmation_fits_and_keeps_its_declining_row() { + let dir = tempfile::tempdir().expect("a temporary directory"); + let store = io_harness::Store::open(dir.path().join("runs.db")).expect("a store"); + let session = store + .create_session("/a/deliberately/long/workspace/path/that/will/not/fit/in/eighty/columns") + .expect("a session"); + let sized = io_cli::store::sized(&store, session).expect("a size"); + let size = store.store_size().expect("a size"); + + for theme in themes() { + let set = if theme.glyphs.separator == io_cli::glyphs::ASCII.separator { + "ascii" + } else { + "unicode" + }; + for (what, (title, rows)) in [ + ("remove", io_cli::store::confirm_remove(session, &sized)), + ( + "sweep", + io_cli::store::confirm_sweep("2026-08-01T00:00:00.000Z"), + ), + ("compact", io_cli::store::confirm_compact(&size)), + ( + "export", + io_cli::export::confirm("io-session-1.md", "conversation"), + ), + ( + "undo file", + io_cli::undo::confirm_file("src/a/deeply/nested/path/that/is/long.rs"), + ), + ("undo step", io_cli::undo::confirm_step(12)), + ] { + // Six widths rather than one. The property holds at all of them and + // names no number, which is what 0.26.0's correction asks for. + for width in [40u16, 60, 72, 80, 100, 160] { + let mut picker = io_cli::picker::Picker::new(title.clone(), rows.clone()); + let (mut screen, _recorder) = support::screen(width, HEIGHT); + screen + .draw(|frame| picker.render(frame, frame.area(), &theme)) + .expect("frame"); + let drawn = screen.viewport_text().to_string(); + for line in drawn.lines() { + assert!( + line.chars().count() <= width as usize, + "{what} overflowed {width} columns ({set}): {line:?}", + ); + } + } + assert_eq!( + rows[0].label, + io_cli::store::LEAVE_IT, + "{what} ({set}): the declining row is row 0 at every width", + ); + } + } +} + +/// **N2 — the store page and every report fit eighty columns in both sets.** +#[test] +fn n2_the_store_page_and_the_reports_fit_eighty_columns() { + let dir = tempfile::tempdir().expect("a temporary directory"); + let store = io_harness::Store::open(dir.path().join("runs.db")).expect("a store"); + let session = store + .create_session("/a/deliberately/long/workspace/path/that/will/not/fit/in/eighty/columns") + .expect("a session"); + let run = store.start_run("a goal", "/tmp/w").expect("a run"); + let turn = store + .record_turn(session, None, run, "a goal") + .expect("a turn"); + store + .finish_turn(turn, Some("an answer"), "completed") + .expect("the turn finishes"); + store.finish_run(run, "success").expect("the run finishes"); + + for theme in themes() { + let set = if theme.glyphs.separator == io_cli::glyphs::ASCII.separator { + "ascii" + } else { + "unicode" + }; + let page = io_cli::store::committed(&store, &theme, WIDTH).expect("the page renders"); + within_eighty(set, &text_of(&page).join("\n")); + + // The reports are committed as plain sentences, so they are measured as + // strings rather than drawn — `App::record` wraps them the way every + // other committed line is wrapped. + let removed = io_cli::store::remove(&store, session).expect("the removal runs"); + for line in io_cli::store::removed_report(&removed) { + assert!( + !line.contains('\n'), + "{set}: a report line carries its own newline: {line:?}", + ); + } + let swept = io_cli::store::sweep(&store, "2000-01-01T00:00:00.000Z").expect("the sweep"); + for line in io_cli::store::swept_report(&swept) { + assert!(!line.contains('\n'), "{set}: {line:?}"); + } + let freed = io_cli::store::compact(&store).expect("the compaction"); + for line in io_cli::store::freed_report(&freed) { + assert!(!line.contains('\n'), "{set}: {line:?}"); + } + } +} + +/// **N2 — the speculation line fits eighty columns in both glyph sets.** +/// +/// The one event 0.27.0 draws. It carries the muted leader and a separator, both +/// of which differ between the sets. +#[test] +fn n2_the_speculation_line_fits_eighty_columns() { + for theme in themes() { + let set = if theme.glyphs.separator == io_cli::glyphs::ASCII.separator { + "ascii" + } else { + "unicode" + }; + let mut events = io_cli::events::Events::new(theme); + let lines = events.event( + &io_harness::RunEvent::new( + 1, + 1, + io_harness::EventKind::Speculated { + started: 999, + used: 111, + discarded: 888, + }, + ), + std::time::Duration::ZERO, + ); + assert!(!lines.is_empty(), "{set}: the line is drawn"); + within_eighty(set, &text_of(&lines).join("\n")); + } +} From 9f690fc3c945b6a8ce8282763ab5f00a01347405 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Fri, 28 Aug 2026 17:50:58 +0530 Subject: [PATCH 7/8] docs(0.27.0): the store, undo and export, and the version bump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three README sections, each stating the fact that surprises: a deletion does not shrink the file, a removal takes the session's restore points but not its memory, a step revert is order-sensitive, a restore overwrites an edit made afterwards, an existing export is refused rather than overwritten, and the canonical trace is not JSON. docs/config.example.toml is untouched, which is the point: this release adds no configuration key, and a change to that file would contradict the gate that says so. The nightly rustdoc gate caught a redundant explicit link target in src/undo.rs — the eighth time in ten releases it has found something stable cargo doc, clippy and cargo test --doc all miss. --- CHANGELOG.md | 72 ++++++++++++++++++++++++++++++- Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++ src/undo.rs | 2 +- 5 files changed, 194 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e526b5..c786dee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,75 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +## [0.27.0] - 2026-08-28 + +The store stops being something you cannot see, undo stops being +all-or-nothing, and the work stops ending when the terminal closes. + +**`/store` says what the run store is holding, and three verbs change it.** +`~/.io-cli/runs.db` has held every session, run, step, event, provider call, +snapshot and restore point since 0.15.0, with no retention policy, no rotation +and no way to look at it. The page reports the file's own page arithmetic — +what it costs on disk, what is already free inside it, and what each session in +it holds. `/store rm ` removes one session, `/store sweep ` removes +every session created before a timestamp, and `/store compact` returns the free +pages to the filesystem. Each descends into a confirmation whose first row is +"leave it", which is the row the cursor starts on. + +**A deletion does not shrink the file, and io says so.** SQLite frees pages into +the database rather than out of it, so a removal moves bytes into the free space +*inside* the file and the file on disk stays the size it was. A `VACUUM` is the +only reclamation available, because every store this product has created was made +without `auto_vacuum` — so `/store compact` is a thing you ask for, it needs +roughly the file's own size in free disk space while it runs, and it reports the +bytes the file actually shrank by rather than an inference from the freelist. + +A sweep refuses a session that still holds a resumable run, and names it. It asks +you to agree to the rule rather than to a count, because io-harness exposes no +reader for the column the sweep filters on and the nearest substitute would +under-state what is about to go — filed as io-harness#216. The figures are +reported the moment it finishes, refusals included. + +**`/undo` is the size of the mistake.** `/undo ` puts one file back, +`/undo step ` reverse-applies one step's diff, and a bare `/undo` is the whole +turn — the same thing the rewind chord has always done, now reaching the same +implementation. One file has four possible answers and they read as four +different sentences: it came back, it was removed because the run created it, or +nothing changed — for either of two different reasons. + +Undoing a step is order-sensitive: a later step still standing on the same lines +makes the revert stale, io-harness leaves the file alone rather than +fuzzy-matching it, and io says why. A restore does not know about an edit you +made after the turn, and the confirmation says that before you agree to it. + +**`/export` writes the conversation as markdown, and `/export trace` writes one +run's canonical trace.** Both go into the workspace under the session's own path +policy, and an existing file is refused rather than overwritten. The trace is +written exactly as io-harness produced it — not parsed, not reserialised, not +pretty-printed — because being canonical is the whole of what it is for. + +**Undoing a turn now announces itself.** `EventKind::Rewound` and +`EventKind::Reverted` are emitted only by the observed forms of io-harness's +rewind calls, and this product had called the plain ones since 0.4.0 — so neither +event had ever fired, and the code drawing one of them had been unreachable since +the day it was written. + +**One event that reached nobody now has a line.** A read started before the model +had finished asking, and thrown away — work that was paid for and not used. It +draws only when something was discarded, because a line in every transcript +saying nothing went wrong is a line nobody reads. The other eight silences in +this interface were reviewed in the same pass and keep their routes, which are +better arguments than drawing them would be. + +**`/contain` moves from *this turn* to *the session*.** It is a posture that +changes how every later turn is driven rather than something that acts on the +turn just finished, and moving it is what makes room for `/undo` without widening +the ten-command bound. `Inspect` reaches ten with `/store` and `/export`. + +No configuration key is added, nothing is read at startup that was not read +before, and no model can reach any of the store operations. + + ## [0.26.0] - 2026-08-28 A turn asks for the model, the vendor and the amount of thinking the work @@ -2191,6 +2260,7 @@ client, tool, sandbox, policy engine or session store of its own. - There is no crates.io publish and `cargo install` is not an install path. - No test in this release asserts on wall-clock time. -[Unreleased]: https://github.com/initorigin/io-cli/compare/v0.26.0...HEAD +[Unreleased]: https://github.com/initorigin/io-cli/compare/v0.27.0...HEAD +[0.27.0]: https://github.com/initorigin/io-cli/releases/tag/v0.27.0 [0.1.1]: https://github.com/initorigin/io-cli/releases/tag/v0.1.1 [0.1.0]: https://github.com/initorigin/io-cli/releases/tag/v0.1.0 diff --git a/Cargo.lock b/Cargo.lock index 2fde11f..b5ff436 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1850,7 +1850,7 @@ dependencies = [ [[package]] name = "io-cli" -version = "0.26.0" +version = "0.27.0" dependencies = [ "clap", "crossterm", diff --git a/Cargo.toml b/Cargo.toml index 60e5bf1..f118dbf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "io-cli" -version = "0.26.0" +version = "0.27.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 f49be70..b4128aa 100644 --- a/README.md +++ b/README.md @@ -2024,6 +2024,126 @@ browser at all. io-cli's own `[app.io-cli.browser]` is read from either scope. locale behind `glyphs`. See [Reading it without seeing it](#reading-it-without-seeing-it). +## What the store is holding + +**`/store` commits a page: what the run store costs on disk, what is already free +inside it, and what each session in it holds.** Everything on that page is +io-harness's own arithmetic — `page_size × page_count` for the file, and +`page_size × freelist_count` for the free part — read from the store rather than +computed here. + +The distinction between those two numbers is the whole reason this page exists. + +**A deletion does not shrink the file.** SQLite frees pages *into* the database +rather than out of it, so removing a session moves bytes from the file's size +into the free space inside it and the file on disk stays exactly the size it was. +Every store this product has ever created was made without `auto_vacuum`, so +there is no incremental reclamation either: a `VACUUM` is the only thing that +returns the space, and it is `/store compact`. + +That matters because `~/.io-cli/runs.db` has held every session, run, step, +event, provider call, snapshot and restore point since 0.15.0, with no retention +policy and no rotation. Until this release there was no way to look at it, and no +way to shrink it. + +Three verbs change it, and each one shows what it will do before it does it: + +| | | +|---|---| +| `/store rm ` | remove one session and everything keyed to it | +| `/store sweep ` | remove every session created before that timestamp | +| `/store compact` | rewrite the database without its free pages | + +**Each descends into a confirmation whose first row is "leave it".** That is the +row the cursor starts on, so the keystroke you give by reflex is the one that +changes nothing. + +**A removal is final and takes the session's restore points with it.** Its +*memory* stays — the agent's durable notes belong to the workspace rather than to +the session, so removing a session unlearns nothing — but the rewind for those +turns is gone, and that is the part that bites later. + +**A sweep refuses a session that still holds a resumable run**, and names it. A +date is a policy applied to sessions nobody looked at, and a crash-resumable tree +that vanished because it was old would be the worst thing this command could do. + +**The sweep asks you to agree to the rule rather than to a count**, and the +reason is a gap in io-harness rather than a choice: `sweep_sessions` filters on +`sessions.created_at` and nothing exposes that column, so the set a date selects +cannot be counted until the sweep has run. The nearest substitute — a session's +first turn — is always *later* than the session itself, so a count built on it +would under-state what is about to be deleted. The figures are reported the +moment it finishes, refusals included. Filed as io-harness#216. + +**Compaction is not free while it runs.** It rewrites the whole database and +needs roughly the file's own size in free disk space to do it, so it is a thing +you ask for rather than something a deletion does on your behalf. It reports the +bytes the file actually shrank by, measured, not inferred. + +Nothing here happens on its own — there is no retention setting, no threshold and +no sweep at startup — and no model can reach any of it. + +## Putting work back + +**`/undo` is the size of the mistake.** Until this release the only instrument was +the rewind chord, which undoes a whole turn, and that is the wrong size for *this +one file went wrong*. + +| | | +|---|---| +| `/undo ` | put one file back as it was before the run | +| `/undo step ` | reverse-apply what one step wrote | +| `/undo` | the whole turn — the same thing the rewind chord does | + +**One file has four possible answers and they are four different sentences.** It +came back; it was *removed*, because the run had created it; nothing changed +because the previous contents were not kept; nothing changed because this run +never wrote that path. The last two both mean your file is untouched, and they +mean it for different reasons. + +**Undoing a step is order-sensitive.** Reverse-applying one step's diff while a +later step's change still sits on the same lines finds context that has moved, +and io-harness leaves the file alone rather than fuzzy-matching it into +something nobody wrote. Undo the newest step first and it applies — and io says +so when it happens, because "nothing changed" without that sentence reads as a +bug. + +**A restore does not know about an edit you made afterwards.** The file comes +back from the snapshot taken before the run first wrote it, and that snapshot is +not compared against what is on disk now, so a change you made by hand after the +turn is overwritten. The confirmation says so before you agree to it. + +Every restore goes through the same path policy a write does, and a *removal* +asks the policy separately and refuses anything that is not an outright allow. + +## Taking the work out + +**`/export` writes this conversation as markdown, and `/export trace` writes one +run's canonical trace.** For the review that happens in a pull request, or a text +editor, or a message to somebody who was not there. + +| | | +|---|---| +| `/export` | the conversation, as markdown | +| `/export ` | the same, where you say | +| `/export trace` | the last run's canonical trace | +| `/export trace ` | the same, where you say | + +Both are written into the workspace under the session's own path policy, and +**an existing file is refused rather than overwritten** — an export is a +snapshot, and the next one you take is a different snapshot. + +**The trace is written exactly as io-harness produced it.** Its whole value is +that it is canonical: io-harness leaves wall-clock stamps, measured durations, an +ephemeral sandbox path and autoincrement ids out of it so that two runs of one +case can be compared. A trace io-cli reformatted would compare against nothing, +so io-cli does not parse it, reserialise it, or pretty-print it. It is +pipe-delimited text rather than JSON, and it takes a `.txt` extension for that +reason. + +A turn that never finished is written as a turn that never finished, rather than +as one the agent had nothing to say to. + ## What this release is not The configuration file has reached your terminal since 0.14.0: every section of diff --git a/src/undo.rs b/src/undo.rs index 71638a5..63d836d 100644 --- a/src/undo.rs +++ b/src/undo.rs @@ -25,7 +25,7 @@ //! **1. Reverting a step is order-sensitive, and io-harness will not guess.** //! Reverse-applying a hunk while a *later* step's change still sits on top of it //! finds context that has moved, and the honest answer is -//! [`Reverted::Stale`](io_harness::Reverted::Stale) with the file untouched — +//! [`io_harness::Reverted::Stale`] with the file untouched — //! never a fuzzy match that quietly corrupts it. Newest step first is the order //! that works, and [`step_advice`] is what says so on screen when a stale answer //! comes back, because "nothing happened" without that sentence reads as a bug. From d094bc472d4bf23ac2d4d252c806879970a424d5 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Fri, 28 Aug 2026 18:18:00 +0530 Subject: [PATCH 8/8] fix: the adversarial review's findings, four of them high MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reviewers, split by area, over a fully green suite. The tenth consecutive release in which that gate has paid for itself, and the two most severe were found by both independently. **A sweep date was never validated, and the comparison is lexical.** `sweep_sessions` filters a text column, so `/store sweep 2026-8-1` — an unpadded month — sorts above every real timestamp, as does `/store sweep yesterday`. Either would have removed every session in the store, across every workspace, while the confirmation echoed back exactly what was typed and read as correct. The boundary is now parsed to a shape and normalised, or refused by name. **The session io is running could be deleted from inside itself.** The /store page lists every session with its root, which is what makes the operator's own the recognisable one; io-harness refuses nothing and enforces no foreign keys, so the process would have carried on writing turns into a session that no longer existed and every later /undo and /export would have reported it empty. **The export was the whole turn tree, not the conversation.** session_turns is documented as the tree; a rewind moves the head without deleting the turn, so an undone turn was being exported, unmarked, into the one artifact whose purpose is being read by somebody who was not there. It walks Session::history now. **The whole-turn undo reported through the one-slot footer**, so every "left as the turn left it" warning was overwritten and an operator was told a restore had happened that had not. It records into the scrollback, like its two siblings. Also: /export wrote under the file's policy rather than the session's effective posture, so it wrote while the footer said read only; the step revert did the same, against a contract io-harness documents; a dangling symlink read as an empty path and a write would have followed it out of the workspace; export::write now refuses an existing file itself rather than trusting a check taken before the confirmation; Reverted::NoHunk printed a Rust debug string; a sweep never said how many restore points it took; and the /store page's session total counted sessions its own list could not show. A gate that could not fail is fixed: the sweep confirmation's "carries no count" assertion had a loop-invariant `&&` that made it hold for any title at all. The three new commands' argued forms had no test whatsoever — including the two refusal families whose entire purpose is that a mistyped verb never reaches something destructive. undo::whole_run and Grain::label are gone. Both were public, tested, and called by nothing — the shape this codebase shipped in 0.20.0, 0.21.0 and 0.26.0. And several comments claimed src/events.rs had an unreachable arm for Rewound and Reverted. It has no arm for either: both are Silent by decision and route to the rewind summary. What the observed forms buy is that the events exist at all. --- src/export.rs | 47 ++++++++-- src/main.rs | 103 +++++++++++++++----- src/store.rs | 139 ++++++++++++++++++++++++++- src/undo.rs | 69 +++++++------- tests/commands.rs | 107 ++++++++++++++++++++- tests/export.rs | 132 ++++++++++++++++++++++++-- tests/live.rs | 234 ++++++++++++++++++++++++++++++++++++++++++++++ tests/narrow.rs | 7 +- tests/store.rs | 178 ++++++++++++++++++++++++++++++++--- tests/undo.rs | 44 ++++++--- 10 files changed, 958 insertions(+), 102 deletions(-) diff --git a/src/export.rs b/src/export.rs index de49b81..12af4f3 100644 --- a/src/export.rs +++ b/src/export.rs @@ -100,8 +100,11 @@ impl Refused { "{path} is already there — an export is a snapshot, so this one is \ not written over it; name another path" ), + // "no turns", not "no finished turns": `conversation` answers `None` + // only when the conversation is empty, and a turn that never finished + // is exported with a line saying so. Refused::Nothing => { - "there is nothing to export yet — this session has no finished turns".to_string() + "there is nothing to export yet — this conversation has no turns".to_string() } } } @@ -126,8 +129,20 @@ pub struct Written { /// Every turn the store holds for the session, in the order it returns them, /// each with its prompt and — when the turn finished — its reply. A turn that /// never finished says so. -pub fn conversation(store: &Store, session_id: i64) -> Result, Error> { - let turns = store.session_turns(session_id)?; +pub fn conversation(store: &Store, session: &io_harness::Session) -> Result, Error> { + // **`Session::history` and never `Store::session_turns`.** io-harness + // documents the second as "the whole tree, not one path through it" + // (`state/sessions.rs:265-266`), and a rewind moves the head back **without + // deleting the turn** (`src/rewind.rs`), so a session that was undone once + // and carried on holds turns that are not part of the conversation at all. + // + // Exporting the tree flat would put those turns in the document, in + // sequence, with nothing marking them — in the one artifact whose stated + // purpose is being read by somebody who was not there. It would + // misrepresent both what the agent was asked and what it answered. Found by + // the adversarial review; the first implementation had exactly that defect. + let turns = session.history(store)?; + let session_id = session.id(); if turns.is_empty() { return Ok(None); } @@ -190,12 +205,18 @@ pub fn trace_path(run_id: i64) -> String { /// under is the one that applies and a path outside the workspace is io-harness's /// refusal rather than a check invented here. /// -/// The existence check is [`Workspace::read_bytes`] — the same reader -/// `crate::rewind` uses to tell a missing file from an empty one — and it is -/// deliberately asked *through the workspace* too, so a path the policy denies -/// is refused by the policy rather than answered by a filesystem probe that -/// leaked whether the file exists. +/// **The refusal is checked here, immediately before the write, and not only by +/// the caller.** The confirmation an operator agrees to is shown after an earlier +/// [`occupied`] call, and between the two keystrokes another `io`, an editor +/// autosave or a build can create the file — after which the write would +/// silently replace it while reporting a creation. The doc on [`Written::wrote`] +/// promised `Wrote::Created` and only this check makes that true. Found by the +/// adversarial review, which is also where the earlier version of this comment — +/// describing a `read_bytes` check that was not in the function — was caught. pub fn write(workspace: &Workspace, path: &str, content: &str) -> Result { + if occupied(workspace, path)? { + return Err(Error::Config(Refused::Exists(path.to_string()).said())); + } let wrote = workspace.write_file(path, content)?; Ok(Written { path: path.to_string(), @@ -227,7 +248,15 @@ pub fn write(workspace: &Workspace, path: &str, content: &str) -> Result Result { - Ok(workspace.resolve(path)?.exists()) + let resolved = workspace.resolve(path)?; + // **`symlink_metadata` and not `exists()`.** `exists` follows the link, so a + // **dangling** symlink inside the workspace answers "nothing is there" — and + // `write_file` then follows it and creates the file wherever it points, + // outside the root. `Workspace::resolve` cannot catch that: it is purely + // lexical, and `check_path`'s own symlink branch is guarded by a + // `canonicalize` that fails on a broken link. Asking about the link itself is + // what closes it. Found by the adversarial review. + Ok(resolved.symlink_metadata().is_ok()) } /// The confirmation `/export` shows before it writes. diff --git a/src/main.rs b/src/main.rs index e0c1e33..d27c151 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2498,12 +2498,23 @@ async fn loop_over Result>( // would not see it. Pick::StoreRemove { id } => { if io_cli::store::acts(index) { - match io_cli::store::remove(&store, *id) { - Ok(removed) => { + match io_cli::store::remove(&store, *id, session.id()) { + Ok(io_cli::store::Removal::Done(removed)) => { for line in io_cli::store::removed_report(&removed) { app.record(Tone::Muted, line); } } + // Unreachable from this surface, because + // `confirm_remove` offers no acting row for the + // live session. Kept because the guard belongs + // to the library and a second caller must not + // be able to route around it. + Ok(io_cli::store::Removal::Live) => app.record( + Tone::Refused, + "that is the conversation you are in; io will not \ + remove it" + .to_string(), + ), Err(error) => app.record( Tone::Error, format!( @@ -2532,10 +2543,12 @@ async fn loop_over Result>( } } } - // The two new granularities. Both go through `observing`, - // which is what puts `EventKind::Reverted` on the screen — - // an event this product has never once emitted, because - // only the `_observed` form emits it. + // The two new granularities. The step form goes through + // `observing`, which is what makes `EventKind::Reverted` + // fire at all — an event this product had never emitted, + // because only the `_observed` form emits it. The file + // form does not: `io_harness::rewind` has no observed + // twin and emits nothing, which is why it takes none. Pick::UndoFile { run_id, path } => { if io_cli::store::acts(index) { let workspace = io_harness::tools::Workspace::new(session.root()); @@ -2600,9 +2613,14 @@ async fn loop_over Result>( } Pick::Export { path, content } => { if io_cli::store::acts(index) { + let effective = approval::session_policy( + &policy, + app.posture(), + app.remembered(), + ); let workspace = io_harness::tools::Workspace::with_policy( session.root(), - policy.clone(), + effective, ); match io_cli::export::write(&workspace, path, content) { Ok(written) => { @@ -2805,9 +2823,7 @@ async fn loop_over Result>( // The second. This is where the operator's files change. // **Through `observing` since 0.27.0, which is what finally emits // `EventKind::Rewound`.** The call was `rewind_run`, whose observed - // twin is the only thing that emits it — so the arm rendering that - // event in `src/events.rs` was unreachable from the day it was - // written until this one. + // twin is the only thing that emits it. Command::Rewind => { undo_whole_turn(&mut app, screen, &mut session, &store, &seen)?; } @@ -3770,7 +3786,8 @@ async fn loop_over Result>( Action::Store(Some(keep)) => match keep { commands::Keep::Remove(id) => match io_cli::store::sized(&store, id) { Ok(sized) => { - let (title, rows) = io_cli::store::confirm_remove(id, &sized); + let (title, rows) = + io_cli::store::confirm_remove(id, &sized, session.id()); picker = Some((Picker::new(title, rows), Pick::StoreRemove { id })); } Err(error) => app.record( @@ -3786,10 +3803,21 @@ async fn loop_over Result>( // advance — see io-harness#216 and `US-IO-CLI-0.27.0-I02`. // The operator agrees to the rule and the report carries the // figures. - commands::Keep::Sweep(date) => { - let (title, rows) = io_cli::store::confirm_sweep(&date); - picker = Some((Picker::new(title, rows), Pick::StoreSweep { date })); - } + // **Validated before the confirmation is even built.** The + // comparison io-harness makes is lexical against a text + // column, so `2026-8-1` or `yesterday` sorts above every real + // timestamp and sweeps the entire store — while the + // confirmation echoes back exactly what was typed and reads + // as correct. Both adversarial reviewers found this + // independently, which is the strongest signal that gate + // gives. + commands::Keep::Sweep(date) => match io_cli::store::boundary(&date) { + Ok(date) => { + let (title, rows) = io_cli::store::confirm_sweep(&date); + picker = Some((Picker::new(title, rows), Pick::StoreSweep { date })); + } + Err(said) => app.record(Tone::Error, said), + }, commands::Keep::Compact => match store.store_size() { Ok(size) => { let (title, rows) = io_cli::store::confirm_compact(&size); @@ -3901,7 +3929,7 @@ async fn loop_over Result>( io_harness::tools::Workspace::with_policy(session.root(), policy.clone()); let built = match &taken { commands::Taken::Conversation(path) => { - match io_cli::export::conversation(&store, session.id()) { + match io_cli::export::conversation(&store, &session) { Ok(Some(text)) => Ok(Some(( path.clone().unwrap_or_else(|| { io_cli::export::conversation_path(session.id()) @@ -5621,8 +5649,9 @@ fn commit_transcript( /// /// **Through [`observing`] since 0.27.0, which is what finally emits /// `EventKind::Rewound`.** The call underneath was `rewind_run`, whose observed -/// twin is the only thing that emits it — so the arm rendering that event in -/// `src/events.rs` was unreachable from the day it was written until this one. +/// twin is the only thing that emits it, so the event had never fired. It draws +/// no line — `rewound` is `Disposition::Silent` and routes to the summary this +/// function commits — but it now reaches hooks, `io exec --json` and the trace. fn undo_whole_turn( app: &mut App, screen: &mut Screen>, @@ -5639,11 +5668,20 @@ fn undo_whole_turn( app.forget_fleet(); // As above: the last request belonged to the undone turn. seen.forget(); + // **`record`, never `say`.** `App::say` is a one-slot footer notice + // that the next keystroke clears, and `undone_lines` returns at least + // two lines plus one per file that was NOT put back — so every + // "left as the turn left it" warning was being overwritten and the + // operator was told a restore had happened that had not. This is + // 0.26.0's recorded rule (`record`, always, for anything an operator + // must still be able to read) arriving one release late, and it is + // the most destructive act in the product having the least durable + // report. Found by the adversarial review. for (tone, line) in io_cli::rewind::undone_lines(&undone, &app.theme.glyphs) { - app.say(tone, line); + app.record(tone, line); } } - Ok(None) => app.say(Tone::Muted, "there is no turn to undo"), + Ok(None) => app.record(Tone::Muted, "there is no turn to undo".to_string()), // **`failure::said`, and not the raw `Display`.** Since 0.23.0 the undo // can lose a head race with another `io`, and `Error::Conflict`'s own // text calls the session id a run id and renders an expiry that a head @@ -5656,7 +5694,7 @@ fn undo_whole_turn( // write, so a conflict leaves the operator's files back as they were with // the conversation head where the other process put it. Saying nothing // happened would send them looking for changes that are already gone. - Err(error) => app.say( + Err(error) => app.record( Tone::Error, format!("the undo did not finish: {}", io_cli::failure::said(&error)), ), @@ -5666,14 +5704,24 @@ fn undo_whole_turn( /// Run something that wants an `Observer`, and commit whatever it emitted. /// -/// **This is what makes `EventKind::Rewound` and `EventKind::Reverted` reach the -/// screen at all.** Both are emitted only by the `_observed` forms of the rewind +/// **This is what makes `EventKind::Rewound` and `EventKind::Reverted` exist at +/// all.** Both are emitted only by the `_observed` forms of the rewind /// functions, and every one of those forms takes an observer — but an undo /// happens between turns, at a keystroke, where the driver's own observer /// composition does not exist. So one is built for the act and drained /// immediately afterwards. /// -/// **The events are rendered and never fed to [`App::event`].** That method +/// **It does not put either event on screen, and an earlier version of this +/// comment said it did.** Both kinds are `Disposition::Silent` in +/// [`io_cli::triage`] and route to io-cli's own rewind summary; `src/events.rs` +/// has no arm for either and correctly renders nothing. What the observed forms +/// buy is that the events reach `[[hook]]` observers, the `io exec --json` +/// stream and the durable trace — none of which they had ever reached. The +/// drain below is what carries anything a *future* arm renders, and what makes +/// this the one place such an arm would need no further wiring. Found by the +/// adversarial review. +/// +/// **What is drained is rendered and never fed to [`App::event`].** That method /// folds into the status line, the fleet and the token totals, which belong to a /// *run*; an undo is not a run, and 0.20.0 recorded what happens when foreign /// events go through it — inflated session totals, a replaced provider, and a @@ -5697,6 +5745,13 @@ fn observing( Ok(out) } +/// The last run of this session, if it has had one. +/// +/// **Its doc comment was taken by an insertion in this release** — `undo_whole_turn` +/// and `observing` were added directly beneath it, so this sentence became their +/// summary line and this function was left with none. Found by the adversarial +/// review, and worth noting because nothing in the suite can see a rustdoc +/// summary attached to the wrong item. fn last_run(session: &Session, store: &Store) -> Option { session .transcript(store) diff --git a/src/store.rs b/src/store.rs index 78d0a97..2c173a2 100644 --- a/src/store.rs +++ b/src/store.rs @@ -233,15 +233,105 @@ impl Removed { /// /// Deleting a session the store does not have succeeds and reports nothing, which /// is io-harness's own behaviour and is not softened here. -pub fn remove(store: &Store, session_id: i64) -> Result { +pub fn remove(store: &Store, session_id: i64, live: i64) -> Result { + // Before anything is read, let alone deleted. See [`Removal`]. + if session_id == live { + return Ok(Removal::Live); + } let before = store.store_size()?; let pruned = store.delete_session(session_id)?; let after = store.store_size()?; - Ok(Removed { + Ok(Removal::Done(Box::new(Removed { pruned, before, after, - }) + }))) +} + +/// A timestamp the sweep may safely be given. +/// +/// **This exists because the comparison is lexical and the blast radius is the +/// whole store.** `Store::sweep_sessions` selects `WHERE created_at < ?1` against +/// a `%Y-%m-%dT%H:%M:%fZ` **text** column, so any string sorting above `'2'` +/// selects every session io-harness has ever written. Two entirely plausible +/// inputs do that: +/// +/// - `/store sweep 2026-8-1` — an unpadded month. `'2026-08-28T…' < '2026-8-1'` +/// is true at index five, `'0'` against `'8'`, so *today's* sessions go too. +/// - `/store sweep yesterday` — every ISO stamp starts with `'2'` and every +/// letter sorts above it. The entire store, across every workspace. +/// +/// Neither is a typo an operator would catch by reading the confirmation back, +/// because the confirmation echoes the string they typed and it looks like what +/// they meant. So the shape is checked here, before anything is offered. +/// +/// Two spellings are accepted and one of them is normalised: a bare +/// `YYYY-MM-DD`, which becomes midnight of that day, and a full +/// `YYYY-MM-DDTHH:MM:SS(.mmm)Z` as the store writes them. Everything else is +/// refused by name. +/// +/// **No date library, and no clock.** This validates a *shape* — it does not ask +/// whether the thirty-first of February exists, because io-harness's comparison +/// does not either, and a `2026-02-31` boundary sorts exactly where an operator +/// would expect it to. What it refuses is the class of input that means +/// something entirely different from what was typed. +pub fn boundary(input: &str) -> Result { + let digits = |s: &str| s.chars().all(|c| c.is_ascii_digit()); + let date = input.get(..10).unwrap_or_default(); + let ok_date = date.len() == 10 + && digits(&date[0..4]) + && &date[4..5] == "-" + && digits(&date[5..7]) + && &date[7..8] == "-" + && digits(&date[8..10]); + if !ok_date { + return Err(format!( + "`{input}` is not a date. The store compares timestamps as text, so a \ + word or an unpadded month would sweep far more than you meant — write \ + it as 2026-08-01, or in full as 2026-08-01T00:00:00.000Z" + )); + } + if input.len() == 10 { + // Midnight of that day, which is what "before the first of August" means + // to the person who typed it. + return Ok(format!("{date}T00:00:00.000Z")); + } + let rest = &input[10..]; + let shaped = rest.len() >= 10 + && rest.starts_with('T') + && rest.ends_with('Z') + && digits(&rest[1..3]) + && &rest[3..4] == ":" + && digits(&rest[4..6]) + && &rest[6..7] == ":" + && digits(&rest[7..9]); + if !shaped { + return Err(format!( + "`{input}` is not a timestamp the store would have written. Use \ + 2026-08-01, or the full form 2026-08-01T00:00:00.000Z" + )); + } + Ok(input.to_string()) +} + +/// Whether one session may be removed at all. +/// +/// **An operator can see their own session on the `/store` page — with its root, +/// which is the very thing that makes it recognisable — and there is nothing in +/// io-harness to stop them deleting it.** `delete_session` refuses nothing; the +/// resumable-run refusal lives only in `sweep_sessions`. No foreign keys are +/// enforced, so the removal succeeds, the process carries on writing turn rows +/// into a session that no longer exists, and every later `/undo` and `/export` +/// reports an empty session — a false report, from a session that is running. +/// +/// So the live session is refused here, by the one caller that knows which it +/// is. This is io-cli's own rule and not io-harness's, and it is stated as such. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Removal { + /// It went. + Done(Box), + /// It is the conversation this process is having, and was not touched. + Live, } /// What a date sweep did. @@ -428,6 +518,37 @@ pub fn committed( view.rows.len() ))); } + // **The total and the list count different things, and the difference is + // real rather than a rounding.** `store_size().sessions` counts every row in + // `sessions`; the list comes from `crate::sessions::recent`, which walks runs + // back to sessions and therefore cannot see a session that was opened and + // never used — every `io` somebody started and quit out of. Those sessions + // are invisible here and are exactly what a sweep will remove, so the page + // says how many there are rather than letting the two numbers disagree in + // silence. Found by the adversarial review, not by a test. + let listed = view.rows.len() as u64; + if !view.cut && view.size.sessions > listed { + rows.push(crate::page::Row::caveat(format!( + "{} session{} held no run and {} not listed above — an `io` that was \ + opened and never used. A sweep removes {} too", + view.size.sessions - listed, + if view.size.sessions - listed == 1 { + "" + } else { + "s" + }, + if view.size.sessions - listed == 1 { + "is" + } else { + "are" + }, + if view.size.sessions - listed == 1 { + "it" + } else { + "them" + }, + ))); + } rows.push(crate::page::Row::Blank); rows.push(crate::page::Row::note( @@ -471,7 +592,17 @@ fn session_figures(sized: &Sized) -> String { /// something specific rather than of a keystroke. The restore-point sentence is /// here rather than in the report for the same reason: it is the fact an /// operator needs *before* they agree, not the one they discover afterwards. -pub fn confirm_remove(id: i64, sized: &Sized) -> (String, Vec) { +pub fn confirm_remove(id: i64, sized: &Sized, live: i64) -> (String, Vec) { + if id == live { + return ( + format!("Session {id} is the conversation you are in"), + vec![crate::picker::Row::with_detail( + LEAVE_IT, + "io will not remove the session it is running; quit and remove it \ + from another one", + )], + ); + } let title = match sized { Sized::Holds(size) => format!( "Remove session {id}? {} · {} turn{} · {} run{}", diff --git a/src/undo.rs b/src/undo.rs index 63d836d..8890b17 100644 --- a/src/undo.rs +++ b/src/undo.rs @@ -16,9 +16,15 @@ //! emitted only inside `rewind_run_observed`** (`run.rs:804`). So in this //! product's entire history that event has never fired once. The same is true of //! `EventKind::Reverted`, which only `rewind_step_observed` emits (`run.rs:1011`). -//! Pointing both at the observed forms is not a stylistic change: it is the -//! difference between an undo that announces itself like every other thing this -//! interface draws and one that happens in silence. +//! +//! **What that buys is not a line on screen, and an earlier draft of this file +//! claimed it was.** Both kinds are `Disposition::Silent` in `crate::triage` and +//! route to io-cli's own rewind summary — written from the value the call +//! returned, which `crate::rewind` explains at length — so `src/events.rs` has +//! no arm for either and correctly draws nothing. What the observed forms buy is +//! that the events now **exist**: they reach `[[hook]]` observers, the +//! `io exec --json` stream and the durable trace, none of which they had ever +//! reached. Recorded because the adversarial review found the overclaim. //! //! # Three traps, and the middle one will bite an operator //! @@ -54,7 +60,7 @@ //! a refusal. use io_harness::tools::Workspace; -use io_harness::{Error, Observer, Reverted, Rewind, Rewound, Store}; +use io_harness::{Error, Observer, Reverted, Rewind, Store}; /// Which granularity an undo was asked for. #[derive(Debug, Clone, PartialEq, Eq)] @@ -67,16 +73,9 @@ pub enum Grain { Run, } -impl Grain { - /// What to call it in a sentence. - pub fn label(&self) -> String { - match self { - Grain::File(path) => format!("the file {path}"), - Grain::Step(step) => format!("step {step}"), - Grain::Run => "the whole run".to_string(), - } - } -} +// `Grain` carries no `label()`. The confirmations build their own sentences — +// `confirm_file` and `confirm_step` — and the run arm never renders a label at +// all, so a method that formatted one would be reachable only from a test. /// Put one file back, from the snapshot taken before this run first wrote it. /// @@ -108,20 +107,14 @@ pub fn one_step( io_harness::rewind_step_observed(workspace, store, run_id, step, observer) } -/// Undo everything the run did, announcing it. -/// -/// **The observed form, so `EventKind::Rewound` fires — for the first time in -/// this product's history.** `src/rewind.rs` has called the plain `rewind_run` -/// since 0.4.0, which is why the event has an arm in `src/events.rs` that has -/// never been reached. -pub fn whole_run( - workspace: &Workspace, - store: &Store, - run_id: i64, - observer: &dyn Observer, -) -> Result { - io_harness::rewind_run_observed(workspace, store, run_id, observer) -} +// **There is deliberately no `whole_run` here.** The whole-turn undo is +// `crate::rewind::last_turn`, which does what this module cannot: it moves the +// conversation head back, by compare-and-swap, and re-opens the session. A thin +// wrapper over `rewind_run_observed` beside it would be a second entry point to +// one act, and the first draft of this module shipped exactly that — public, +// tested, and called by nothing. This codebase has shipped tested-but-unreachable +// surfaces in 0.20.0, 0.21.0 and 0.26.0; the adversarial review caught this one +// before it became the fourth. /// What one file's answer says, in the operator's words. /// @@ -145,10 +138,22 @@ pub fn step_said(path: &str, answer: &Reverted) -> String { match answer { Reverted::Applied(_) => format!("{path} is back to what it was before that step"), Reverted::Stale(why) => format!("{path} is unchanged: {why}"), - // `Reverted` is `#[non_exhaustive]`-shaped in spirit — a third and fourth - // variant already exist — so the wildcard reports the honest conservative - // thing rather than guessing that something was applied. - other => format!("{path} is unchanged: {other:?}"), + // **`NoHunk` is the third variant and it is not exotic.** io-harness names + // it for a row written before hunks were kept, and for a file whose + // previous contents were not kept at all — over the snapshot cap, or not + // text. That is the same class `Rewind::NotKept` gets a written sentence + // for one function up, and it deserves one here rather than a Rust debug + // string with quotes in it. The first version of this function let it + // fall to a `{other:?}` wildcard, and the adversarial review found it. + Reverted::NoHunk(why) => format!( + "{path} is unchanged: no diff was stored for that step ({why}) — \ + `/undo {path}` puts the whole file back instead" + ), + // `Reverted` may gain a fourth variant in a later io-harness. Reporting + // the conservative thing — nothing changed — is the arm that cannot + // mislead, because the alternative is claiming a restore this build did + // not understand. + _ => format!("{path} is unchanged: this build does not know that answer"), } } diff --git a/tests/commands.rs b/tests/commands.rs index 9069d18..83e82ad 100644 --- a/tests/commands.rs +++ b/tests/commands.rs @@ -344,7 +344,12 @@ fn f12_resume_says_it_answers_a_parked_run_rather_than_merely_reopening_a_sessio #[test] fn f1_effort_parses_three_levels_the_absence_and_the_question() { use io_cli::commands::Reasoning; - let parse = |text: &str| commands::parse(text, &defaults(), &DARK); + // `parse` takes the word without its slash — the driver strips it before the + // call, which is why every other test in this file types `continue` rather + // than `/continue`. Written as a helper so the cases below read as the + // operator types them. + let parse = + |text: &str| commands::parse(text.strip_prefix('/').unwrap_or(text), &defaults(), &DARK); assert_eq!( parse("effort low"), @@ -1471,3 +1476,103 @@ fn gates_is_a_configure_command_that_resolves_to_its_own_action() { "`/gates`'s row is advertised and inert", ); } + +/// **The three new commands' argued forms, which had no gate at all.** +/// +/// The adversarial review found that nothing in `tests/` typed `/store`, +/// `/undo` or `/export` with an argument — so the release's most load-bearing +/// decision, that an unparseable verb must **never** fall through to something +/// destructive, was asserted nowhere. +/// +/// Both refusal families exist for a stated reason and both are checked here: +/// +/// - `Action::UndoNoStep` — *an operator who typed a step and got the entire run +/// undone would have lost work they never asked to lose.* +/// - `Keep::{NoId, NoDate, Unknown}` — *somebody who typed a delete and got a +/// report would believe the delete had happened.* +#[test] +fn f10_an_unparseable_verb_never_falls_through_to_something_destructive() { + use io_cli::commands::{Keep, Taken}; + use io_cli::undo::Grain; + + // `parse` takes the word without its slash — the driver strips it before the + // call, which is why every other test in this file types `continue` rather + // than `/continue`. Written as a helper so the cases below read as the + // operator types them. + let parse = + |text: &str| commands::parse(text.strip_prefix('/').unwrap_or(text), &defaults(), &DARK); + + // `/undo` — the one that must never become a whole-run undo. + assert_eq!(parse("/undo"), Action::Undo(Grain::Run)); + assert_eq!( + parse("/undo src/a.rs"), + Action::Undo(Grain::File("src/a.rs".into())), + ); + assert_eq!(parse("/undo step 4"), Action::Undo(Grain::Step(4))); + for wrong in [ + "/undo step", + "/undo step -1", + "/undo step 3.5", + "/undo step nine", + "/undo step 99999999999999999999", + "/undo step 0x10", + ] { + assert_eq!( + parse(wrong), + Action::UndoNoStep, + "`{wrong}` must be refused by name and must never become Grain::Run", + ); + } + + // `/store` — the verbs, and the three refusals. + assert_eq!(parse("/store"), Action::Store(None)); + assert_eq!(parse("/store rm 7"), Action::Store(Some(Keep::Remove(7)))); + assert_eq!(parse("/store compact"), Action::Store(Some(Keep::Compact))); + assert_eq!( + parse("/store sweep 2026-08-01"), + Action::Store(Some(Keep::Sweep("2026-08-01".into()))), + ); + for wrong in [ + "/store rm", + "/store rm all", + "/store rm -1x", + "/store delete", + ] { + assert!( + matches!(parse(wrong), Action::Store(Some(Keep::NoId))), + "`{wrong}` must ask for an id rather than act: {:?}", + parse(wrong), + ); + } + assert!(matches!( + parse("/store sweep"), + Action::Store(Some(Keep::NoDate)) + )); + // The typo that matters: a near-miss verb must not reach the page, because a + // report where a deletion was asked for reads as a deletion that happened. + for typo in ["/store swep 2026-08-01", "/store purge", "/store vacume"] { + assert!( + matches!(parse(typo), Action::Store(Some(Keep::Unknown(_)))), + "`{typo}` must be named as unknown rather than showing the page: {:?}", + parse(typo), + ); + } + + // `/export` — `trace` is a word and everything else is a path. + assert_eq!(parse("/export"), Action::Export(Taken::Conversation(None))); + assert_eq!( + parse("/export notes.md"), + Action::Export(Taken::Conversation(Some("notes.md".into()))), + ); + assert_eq!(parse("/export trace"), Action::Export(Taken::Trace(None))); + assert_eq!( + parse("/export trace run.txt"), + Action::Export(Taken::Trace(Some("run.txt".into()))), + ); + // The documented ambiguity, and its documented escape. + assert_eq!( + parse("/export ./trace"), + Action::Export(Taken::Conversation(Some("./trace".into()))), + "a file literally called `trace` is reachable as `./trace`", + ); +} diff --git a/tests/export.rs b/tests/export.rs index b4634d4..f554c92 100644 --- a/tests/export.rs +++ b/tests/export.rs @@ -24,6 +24,11 @@ use io_cli::export::{ use io_harness::tools::{Workspace, Wrote}; use io_harness::Store; +/// The session, as a `Session`, for the reader that walks its history. +fn reopened(store: &Store, id: i64) -> io_harness::Session { + io_harness::Session::reopen(store, id).expect("the session reopens") +} + /// A store in a real file, plus a workspace to write into. /// /// One `TempDir` for both, because an export writes into the workspace the @@ -52,6 +57,7 @@ fn fixture() -> (tempfile::TempDir, Store, Workspace) { fn seed(store: &Store, prompts: &[(&str, Option<&str>)]) -> (i64, Vec) { let session = store.create_session("/tmp/work").expect("a session opens"); let mut runs = Vec::new(); + let mut parent: Option = None; for (prompt, reply) in prompts { let run = store.start_run(prompt, "/tmp/work").expect("a run starts"); for step in 1..=2u32 { @@ -70,8 +76,17 @@ fn seed(store: &Store, prompts: &[(&str, Option<&str>)]) -> (i64, Vec) { .expect("a step records"); } let turn = store - .record_turn(session, None, run, prompt) + .record_turn(session, parent, run, prompt) .expect("a turn records"); + // **The head is moved as a real turn would move it.** `conversation` + // walks `Session::history` — the path from the head back to the root — + // rather than `Store::session_turns`, which io-harness documents as the + // whole tree. A fixture that left the head unset would export nothing, + // and one that parented every turn at the root would export a fan. + store + .set_session_head(session, Some(turn)) + .expect("the head moves"); + parent = Some(turn); // A `None` reply is a turn that did not finish, and it is written as one. // The turn is still closed, because an unfinished *reply* and an // unfinished *turn* are different states and only the first is on trial. @@ -105,7 +120,7 @@ fn f8_every_prompt_and_reply_appears_in_the_stores_order() { ], ); - let markdown = conversation(&store, session) + let markdown = conversation(&store, &reopened(&store, session)) .expect("the conversation is readable") .expect("there is one"); @@ -138,7 +153,7 @@ fn f8_a_turn_that_never_answered_says_so_rather_than_showing_an_empty_reply() { &[("answered", Some("here you are")), ("never answered", None)], ); - let markdown = conversation(&store, session) + let markdown = conversation(&store, &reopened(&store, session)) .expect("the conversation is readable") .expect("there is one"); @@ -159,7 +174,7 @@ fn a_session_with_no_turns_is_refused_rather_than_written_empty() { let session = store.create_session("/tmp/work").expect("a session opens"); assert_eq!( - conversation(&store, session).expect("readable"), + conversation(&store, &reopened(&store, session)).expect("readable"), None, "there is no conversation to write", ); @@ -243,7 +258,7 @@ fn the_trace_is_not_reformatted() { fn f8_a_path_outside_the_workspace_is_refused() { let (_dir, store, workspace) = fixture(); let (session, _runs) = seed(&store, &[("a question", Some("an answer"))]); - let markdown = conversation(&store, session) + let markdown = conversation(&store, &reopened(&store, session)) .expect("readable") .expect("there is one"); @@ -267,7 +282,7 @@ fn an_existing_file_is_refused_rather_than_overwritten() { let (_dir, store, workspace) = fixture(); let (session, _runs) = seed(&store, &[("a question", Some("an answer"))]); let path = conversation_path(session); - let markdown = conversation(&store, session) + let markdown = conversation(&store, &reopened(&store, session)) .expect("readable") .expect("there is one"); @@ -314,7 +329,7 @@ fn the_confirmation_declines_at_row_zero_and_names_the_path() { fn the_report_names_the_file_and_its_size() { let (_dir, store, workspace) = fixture(); let (session, _runs) = seed(&store, &[("a question", Some("an answer"))]); - let markdown = conversation(&store, session) + let markdown = conversation(&store, &reopened(&store, session)) .expect("readable") .expect("there is one"); let written = @@ -325,3 +340,106 @@ fn the_report_names_the_file_and_its_size() { assert!(said.contains(&written.bytes.to_string()), "{said}"); assert_eq!(written.bytes, markdown.len()); } + +/// **`write` refuses an existing file itself, not only through its caller.** +/// +/// The confirmation an operator agrees to is shown after an earlier `occupied` +/// call, and between the two keystrokes another `io`, an editor autosave or a +/// build can create the file. Without a check inside `write` the export would +/// silently replace it and report a creation — and `Written::wrote` promises +/// `Wrote::Created`. Found by the adversarial review, which also found that the +/// test named for this refusal never actually called `write` twice. +#[test] +fn f8_write_refuses_an_existing_file_rather_than_overwriting_it() { + let (_dir, store, workspace) = fixture(); + let (session, _runs) = seed(&store, &[("a question", Some("an answer"))]); + let path = conversation_path(session); + let markdown = conversation(&store, &reopened(&store, session)) + .expect("readable") + .expect("there is one"); + + write(&workspace, &path, &markdown).expect("the first export is written"); + let again = write(&workspace, &path, "something else entirely"); + + assert!(again.is_err(), "the second write is refused"); + assert_eq!( + std::fs::read_to_string(workspace.root().join(&path)).expect("the file"), + markdown, + "and the first export is untouched", + ); +} + +/// **A dangling symlink is occupied, so the write cannot follow it out of the +/// workspace.** +/// +/// `Workspace::resolve` is purely lexical and `Path::exists()` follows the link, +/// so a broken symlink answered "nothing is there" — and `write_file` then +/// followed it and created the file wherever it pointed. `check_path`'s own +/// symlink branch cannot help: it is guarded by a `canonicalize` that fails on a +/// broken link. Asking about the link itself is what closes it. Found by the +/// adversarial review. +#[cfg(unix)] +#[test] +fn f8_a_dangling_symlink_is_not_mistaken_for_an_empty_path() { + let (_dir, _store, workspace) = fixture(); + let link = workspace.root().join("io-session-9.md"); + std::os::unix::fs::symlink("../../escaped.md", &link).expect("the link is made"); + + assert!( + !link.exists(), + "the link is dangling, which is the whole point", + ); + assert!( + occupied(&workspace, "io-session-9.md").expect("askable"), + "a dangling symlink is something, and a write would follow it out of the \ + workspace", + ); + assert!( + write(&workspace, "io-session-9.md", "anything").is_err(), + "so the write is refused", + ); +} + +/// **The export is the conversation, not the whole turn tree.** +/// +/// `Store::session_turns` is documented as "the whole tree, not one path through +/// it", and a rewind moves the head back **without deleting the turn**. So a +/// session that was undone once and carried on holds turns that are not part of +/// the conversation at all — and exporting them flat, unmarked, in the one +/// artifact whose purpose is being read by somebody who was not there, would +/// misrepresent both what the agent was asked and what it answered. +/// +/// Found by the adversarial review. `Session::history` is the path. +#[test] +fn f8_an_undone_turn_is_not_in_the_export() { + let (_dir, store, _ws) = fixture(); + let (session, _runs) = seed( + &store, + &[ + ("the first question", Some("the first answer")), + ("the abandoned question", Some("the abandoned answer")), + ], + ); + + // Move the head back to the first turn, exactly as a rewind does — the turn + // row stays, off the head path. + let turns = store.session_turns(session).expect("readable"); + let first = turns.first().expect("a first turn").id; + store + .set_session_head(session, Some(first)) + .expect("the head moves back"); + + let markdown = conversation(&store, &reopened(&store, session)) + .expect("readable") + .expect("there is a conversation"); + + assert!( + markdown.contains("the first question"), + "the conversation that stands is exported:\n{markdown}", + ); + assert!( + !markdown.contains("the abandoned question"), + "a turn that was undone is not part of the conversation and must not be \ + in the document:\n{markdown}", + ); +} diff --git a/tests/live.rs b/tests/live.rs index e900d0c..3fb7843 100644 --- a/tests/live.rs +++ b/tests/live.rs @@ -3395,3 +3395,237 @@ async fn live_f5_an_unreachable_primary_falls_through_to_the_provider_underneath answered — it is what io-harness turns into `EventKind::FellBackTo`", ); } + +/// **O3 / F6 / F7 — an undo of a file a real agent actually wrote.** +/// +/// The fixture tests build their restore points through io-harness's own run +/// loop with a scripted provider, which is the right instrument for asserting +/// the four answers. What no fixture can settle is whether a *real* model's +/// `write_file` produces a snapshot this crate can rewind — that depends on the +/// tool the model chooses, and a model may edit, patch or rewrite. +/// +/// So this drives a real turn, asserts the file changed, then puts it back and +/// asserts the bytes on disk are the ones that were there before the agent ran. +#[tokio::test] +#[ignore = "live: needs OPENROUTER_API_KEY"] +async fn live_f6_f7_a_real_agents_write_is_put_back() { + let key = key(); + let dir = tempfile::tempdir().expect("a workspace"); + let root = dir.path(); + let before = "the original line\n"; + std::fs::write(root.join("subject.txt"), before).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( + "Replace the entire contents of subject.txt with exactly the word: replaced. \ + Then say what you did in one sentence.", + &provider, + &store, + &policy, + &io_harness::ApproveAll, + &observer, + &inbox, + ) + .await + .expect("the turn runs"); + println!("outcome: {:?}", result.outcome); + + let after = std::fs::read_to_string(root.join("subject.txt")).expect("the file is there"); + println!("after the turn: {after:?}"); + assert_ne!(after, before, "the agent has to have changed something"); + + let head = session.head().expect("the turn is on the head"); + let run_id = store + .session_turn(head) + .expect("readable") + .expect("there") + .run_id; + + // The undo, through the same call `/undo ` makes. + let workspace = io_harness::tools::Workspace::new(root); + let answer = + io_cli::undo::one_file(&workspace, &store, run_id, "subject.txt").expect("the undo runs"); + println!( + "the undo said: {}", + io_cli::undo::said("subject.txt", &answer) + ); + + assert!( + matches!(answer, io_harness::Rewind::Restored(_)), + "a real agent's write leaves a restore point this crate can use: {answer:?}", + ); + assert_eq!( + std::fs::read_to_string(root.join("subject.txt")).expect("still there"), + before, + "the bytes on disk are the ones that were there before the agent ran", + ); + + // And the whole-turn form emits the event that had never fired before + // 0.27.0 — through `rewind::last_turn`, which is the production path. + let watcher = Collector { + events: Arc::new(Mutex::new(Vec::new())), + }; + let undone = io_cli::rewind::last_turn(&mut session, &store, &watcher) + .expect("the whole-turn undo runs"); + println!("undone: {undone:?}"); + let saw = watcher + .events + .lock() + .expect("not poisoned") + .iter() + .any(|event| matches!(event.kind, io_harness::EventKind::Rewound { .. })); + assert!(saw, "EventKind::Rewound reached the observer on a real run"); +} + +/// **O3 / F8 — an export of a conversation that actually happened.** +/// +/// A fixture's trace is assembled from rows a test wrote. This asserts the +/// export against a run a real model drove: the markdown carries the prompt and +/// the reply that were really exchanged, and the trace file is byte-identical to +/// what io-harness produced for that run. +#[tokio::test] +#[ignore = "live: needs OPENROUTER_API_KEY"] +async fn live_f8_a_real_conversation_exports() { + let key = key(); + let dir = tempfile::tempdir().expect("a workspace"); + let root = dir.path(); + + 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 prompt = "In one short sentence, say what a canonical trace is for."; + session + .turn_steered( + prompt, &provider, &store, &policy, &DenyAll, &observer, &inbox, + ) + .await + .expect("the turn runs"); + + let markdown = io_cli::export::conversation(&store, &session) + .expect("readable") + .expect("a conversation that happened"); + println!("--- exported markdown ---\n{markdown}"); + assert!( + markdown.contains(prompt), + "the operator's own words are in the export", + ); + + let head = session.head().expect("a turn"); + let run_id = store + .session_turn(head) + .expect("readable") + .expect("there") + .run_id; + + let trace = io_cli::export::trace(&store, run_id).expect("a trace"); + assert!( + !trace.is_empty(), + "a run that really happened has a trace; an empty one would make the \ + byte comparison below vacuous, which is the defect this release found \ + in its own fixture", + ); + + let workspace = io_harness::tools::Workspace::new(root); + let written = io_cli::export::write(&workspace, &io_cli::export::trace_path(run_id), &trace) + .expect("the trace is written"); + let back = std::fs::read_to_string(root.join(&written.path)).expect("the file"); + assert_eq!( + back, + store.canonical_trace(run_id).expect("readable"), + "the file on disk is io-harness's own string, byte for byte", + ); +} + +/// **O3 — whichever of the two conditional events a real run emits.** +/// +/// `Speculated` fires only when a provider reports finished calls and something +/// was actually started; `CacheMarked` only when a marked prefix advances. +/// Neither can be provoked, so this **reports** rather than asserts: it prints +/// what a real run emitted so the release record can say what was observed +/// rather than what was hoped for. +/// +/// The one thing it does assert is that if `Speculated` arrives with something +/// discarded, the renderer draws a line for it — which is the arm 0.27.0 added. +#[tokio::test] +#[ignore = "live: needs OPENROUTER_API_KEY"] +async fn live_f9_whichever_conditional_events_a_real_run_emits() { + let key = key(); + let dir = tempfile::tempdir().expect("a workspace"); + let root = dir.path(); + for name in ["one.txt", "two.txt", "three.txt"] { + std::fs::write(root.join(name), format!("contents of {name}\n")).expect("a 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), + }; + + session + .turn_steered( + "Read one.txt, two.txt and three.txt, then tell me in one sentence what \ + they have in common.", + &provider, + &store, + &policy, + &DenyAll, + &observer, + &inbox, + ) + .await + .expect("the turn runs"); + + let events = collected.lock().expect("not poisoned").clone(); + let mut drew = 0usize; + for event in &events { + match &event.kind { + io_harness::EventKind::Speculated { + started, + used, + discarded, + } => { + println!("live: Speculated started={started} used={used} discarded={discarded}"); + if *discarded > 0 { + let mut renderer = io_cli::events::Events::new(io_cli::theme::DARK); + let lines = renderer.event(event, std::time::Duration::ZERO); + assert!( + !lines.is_empty(), + "a real discarded read must draw the line 0.27.0 added", + ); + drew += 1; + } + } + io_harness::EventKind::CacheMarked { + through_step, + prefix_bytes, + } => println!("live: CacheMarked through_step={through_step} bytes={prefix_bytes}"), + _ => {} + } + } + println!( + "live: drew {drew} speculation line(s) from {} events", + events.len() + ); +} diff --git a/tests/narrow.rs b/tests/narrow.rs index 9ac32e9..8854be8 100644 --- a/tests/narrow.rs +++ b/tests/narrow.rs @@ -2280,7 +2280,7 @@ fn n2_every_store_confirmation_fits_and_keeps_its_declining_row() { "unicode" }; for (what, (title, rows)) in [ - ("remove", io_cli::store::confirm_remove(session, &sized)), + ("remove", io_cli::store::confirm_remove(session, &sized, 0)), ( "sweep", io_cli::store::confirm_sweep("2026-08-01T00:00:00.000Z"), @@ -2350,7 +2350,10 @@ fn n2_the_store_page_and_the_reports_fit_eighty_columns() { // The reports are committed as plain sentences, so they are measured as // strings rather than drawn — `App::record` wraps them the way every // other committed line is wrapped. - let removed = io_cli::store::remove(&store, session).expect("the removal runs"); + let removed = match io_cli::store::remove(&store, session, 0).expect("the removal runs") { + io_cli::store::Removal::Done(removed) => *removed, + io_cli::store::Removal::Live => unreachable!("0 is not a session id"), + }; for line in io_cli::store::removed_report(&removed) { assert!( !line.contains('\n'), diff --git a/tests/store.rs b/tests/store.rs index 2ffc835..80849c6 100644 --- a/tests/store.rs +++ b/tests/store.rs @@ -31,7 +31,7 @@ //! up on the other side of the same wall. use io_cli::store::{ - acts, committed, compact, confirm_compact, confirm_remove, confirm_sweep, freed_report, remove, + acts, committed, compact, confirm_compact, confirm_remove, confirm_sweep, freed_report, removed_report, sized, sweep, swept_report, view, Sized, LEAVE_IT, }; use io_harness::Store; @@ -86,6 +86,17 @@ fn seed_resumable(store: &Store, root: &str) -> i64 { session } +/// Remove a session that is not the live one, and unwrap the removal. +/// +/// The live-session id is `0`, which no store ever allocates, so every test that +/// is not *about* the guard is unaffected by it. `f6`'s own test names a real id. +fn removed(store: &Store, session: i64) -> io_cli::store::Removed { + match io_cli::store::remove(store, session, 0).expect("the removal runs") { + io_cli::store::Removal::Done(removed) => *removed, + io_cli::store::Removal::Live => panic!("session {session} is not the live one"), + } +} + /// A timestamp after every session any of these tests can create. const AFTER_EVERYTHING: &str = "2999-01-01T00:00:00.000Z"; @@ -128,7 +139,7 @@ fn f1_a_removal_frees_pages_into_the_file_and_the_file_does_not_move() { let doomed = seed(&store, "/tmp/doomed", 12); seed(&store, "/tmp/kept", 2); - let removed = remove(&store, doomed).expect("the session is removed"); + let removed = removed(&store, doomed); assert_eq!(removed.pruned.sessions, 1); assert!(removed.pruned.turns >= 12, "every turn is accounted for"); @@ -143,8 +154,21 @@ fn f1_a_removal_frees_pages_into_the_file_and_the_file_does_not_move() { ); assert!( removed.after.free_bytes >= removed.before.free_bytes, - "the freed pages are inside the file", + "free space inside the file never goes down on a removal", ); + // **`>=` and not `>`, and that is a fact about SQLite rather than a weak + // assertion.** The adversarial review proposed strengthening this to a strict + // `>`; it was tried and it is **false**. A deletion frees *rows*, and pages + // reach the freelist only when a page empties — a twelve-turn session does + // not fill one, so `free_bytes` is unchanged and a compaction afterwards + // returns zero. Both were run before this comment was written. + // + // The claim this criterion actually makes is the equality above: the file on + // disk did not move. That the freed pages go *into* the file is asserted by + // `f2_only_compact_shrinks_the_file_and_reports_the_measured_difference`, at + // a size where whole pages are freed and the compaction has something to + // return. Asserting it here as well would be asserting arithmetic about + // SQLite's page layout, which is 0.26.0's recorded mistake in a new place. } /// **F1 — the report says the file did not move, and names the compaction.** @@ -155,7 +179,7 @@ fn f1_a_removal_frees_pages_into_the_file_and_the_file_does_not_move() { fn f1_the_report_says_the_file_is_unchanged_and_what_would_change_it() { let (_dir, store) = on_disk(); let doomed = seed(&store, "/tmp/doomed", 6); - let removed = remove(&store, doomed).expect("the session is removed"); + let removed = removed(&store, doomed); let report = removed_report(&removed).join("\n"); assert!(report.contains("removed 1 session"), "{report}"); @@ -184,7 +208,7 @@ fn f2_only_compact_shrinks_the_file_and_reports_the_measured_difference() { let (_dir, store) = on_disk(); let doomed = seed(&store, "/tmp/doomed", 40); seed(&store, "/tmp/kept", 1); - remove(&store, doomed).expect("the session is removed"); + removed(&store, doomed); let freed = compact(&store).expect("the store compacts"); @@ -256,7 +280,7 @@ fn f3_absent_is_not_the_same_answer_as_empty() { /// for a session the store does not have would be a button that cannot work. #[test] fn f3_the_confirmation_for_a_missing_session_offers_nothing_to_do() { - let (title, rows) = confirm_remove(9_999, &Sized::Absent); + let (title, rows) = confirm_remove(9_999, &Sized::Absent, 0); assert!(title.contains("no session 9999"), "{title}"); assert_eq!(rows.len(), 1, "only the declining row"); @@ -273,7 +297,7 @@ fn f3_removing_a_session_that_is_not_there_reports_nothing() { let (_dir, store) = on_disk(); seed(&store, "/tmp/kept", 1); - let removed = remove(&store, 9_999).expect("removing nothing is not an error"); + let removed = removed(&store, 9_999); assert_eq!(removed.pruned.sessions, 0); assert_eq!(removed.pruned.turns, 0); @@ -402,7 +426,7 @@ fn f5_row_zero_declines_in_every_confirmation() { let held = sized(&store, session).expect("a size is readable"); for (what, rows) in [ - ("remove", confirm_remove(session, &held).1), + ("remove", confirm_remove(session, &held, 0).1), ("sweep", confirm_sweep("2026-08-01").1), ("compact", confirm_compact(&size).1), ] { @@ -433,7 +457,7 @@ fn f5_declining_leaves_the_store_byte_identical() { // decision and it lives in the library precisely so this can be asserted — // `src/main.rs` is a binary nothing under `tests/` can link. if acts(0) { - remove(&store, session).expect("unreachable"); + removed(&store, session); } let after = store.store_size().expect("a size is readable"); @@ -452,7 +476,7 @@ fn f5_the_removal_confirmation_states_the_cost_and_the_restore_points() { let session = seed(&store, "/tmp/one", 5); let held = sized(&store, session).expect("a size is readable"); - let (title, rows) = confirm_remove(session, &held); + let (title, rows) = confirm_remove(session, &held, 0); assert!(title.contains(&session.to_string()), "{title}"); assert!( @@ -497,7 +521,7 @@ fn the_page_names_the_file_the_free_space_and_the_three_verbs() { let (_dir, store) = on_disk(); let doomed = seed(&store, "/tmp/doomed", 8); seed(&store, "/tmp/kept", 1); - remove(&store, doomed).expect("the session is removed"); + removed(&store, doomed); let theme = io_cli::theme::THEMES[0]; let lines = committed(&store, &theme, 100).expect("the page renders"); @@ -562,3 +586,135 @@ fn o4_no_store_gate_names_a_real_home() { "and a real file, because the subject is page arithmetic on one", ); } + +// --------------------------------------------------------------------------- +// The date the sweep is given, and the session it is not allowed to remove +// --------------------------------------------------------------------------- + +/// **F4 — a sweep boundary that is not a timestamp is refused before anything +/// is offered.** +/// +/// The comparison io-harness makes is **lexical**, against a +/// `%Y-%m-%dT%H:%M:%fZ` text column, so any string sorting above `'2'` selects +/// every session in the store. Two entirely plausible inputs do that, and +/// neither looks wrong when the confirmation echoes it back: +/// +/// - `2026-8-1`, an unpadded month, which sorts above `2026-08-28T…` +/// - `yesterday`, or any word, because every letter sorts above `'2'` +/// +/// **Both adversarial reviewers found this independently**, which is the +/// strongest signal that gate gives. +#[test] +fn f4_a_boundary_that_is_not_a_timestamp_is_refused() { + use io_cli::store::boundary; + + // The two that would have swept the whole store. + for wrong in [ + "2026-8-1", + "yesterday", + "last-week", + "now", + "9", + "", + "2026/08/01", + ] { + let refused = boundary(wrong).expect_err(&format!("`{wrong}` must be refused")); + assert!( + refused.contains(wrong) || wrong.is_empty(), + "the refusal names what was typed: {refused}", + ); + } + + // A bare date is normalised to midnight, which is what the operator meant. + assert_eq!( + boundary("2026-08-01").expect("a bare date is accepted"), + "2026-08-01T00:00:00.000Z", + ); + // A full stamp is taken as written. + assert_eq!( + boundary("2026-08-01T12:30:00.000Z").expect("a full stamp is accepted"), + "2026-08-01T12:30:00.000Z", + ); + + // **The property that matters: an accepted boundary never sorts above a real + // timestamp unless it is meant to.** A store's stamps look like this one. + let real = "2026-08-28T09:15:22.417Z"; + assert!( + boundary("2026-08-01").expect("accepted").as_str() < real, + "a boundary before today must sort before today's sessions", + ); + assert!( + boundary("2026-09-01").expect("accepted").as_str() > real, + "and one after it must sort after", + ); +} + +/// **The session io is running is refused, and the confirmation offers no way to +/// do it.** +/// +/// `/store` lists every session **with its root**, which is exactly what makes +/// the operator's own workspace the recognisable one. io-harness refuses nothing +/// — `delete_session` has no guard, the resumable-run refusal lives only in +/// `sweep_sessions`, and no foreign keys are enforced — so the removal would +/// succeed, the process would carry on writing turns into a session that no +/// longer exists, and every later `/undo` and `/export` would report an empty +/// session. A false report, from a session that is still running. +/// +/// Found by both adversarial reviewers. +#[test] +fn the_live_session_cannot_be_removed() { + let (_dir, store) = on_disk(); + let live = seed(&store, "/tmp/live", 3); + let other = seed(&store, "/tmp/other", 2); + + assert_eq!( + io_cli::store::remove(&store, live, live).expect("not an error"), + io_cli::store::Removal::Live, + "the conversation this process is having is not removable", + ); + assert!( + matches!(sized(&store, live), Ok(Sized::Holds(_))), + "and it is still in the store", + ); + + // The confirmation does not offer it either, so the refusal is visible before + // the keystroke rather than after it. + let held = sized(&store, live).expect("a size"); + let (title, rows) = confirm_remove(live, &held, live); + assert!(title.contains("the conversation you are in"), "{title}"); + assert_eq!(rows.len(), 1, "there is nothing to choose but leaving it"); + assert_eq!(rows[0].label, LEAVE_IT); + + // Any other session is unaffected. + assert!(matches!( + io_cli::store::remove(&store, other, live).expect("not an error"), + io_cli::store::Removal::Done(_) + )); +} + +/// **A sweep says how many restore points went, as a removal does.** +/// +/// `removed_report` has said it since it was written and the module preamble +/// calls it the sentence that bites later. A sweep removes many sessions' +/// restore points at once and said nothing, before or after. Found by the +/// adversarial review. +#[test] +fn f4_the_sweep_report_names_the_restore_points_it_took() { + let (_dir, store) = on_disk(); + seed(&store, "/tmp/one", 3); + + let swept = sweep(&store, AFTER_EVERYTHING).expect("the sweep runs"); + let report = swept_report(&swept).join("\n"); + + assert!(report.contains("swept"), "{report}"); + if swept.pruned.restore_points > 0 { + assert!( + report.contains("restore point"), + "a sweep that took restore points has to say so: {report}", + ); + assert!( + report.contains("memory did not"), + "and say what survived: {report}", + ); + } +} diff --git a/tests/undo.rs b/tests/undo.rs index 4b955d3..bbe1fdc 100644 --- a/tests/undo.rs +++ b/tests/undo.rs @@ -20,7 +20,7 @@ mod support; use std::sync::Mutex; use io_cli::undo::{ - confirm_file, confirm_step, one_file, one_step, said, step_advice, step_said, whole_run, Grain, + confirm_file, confirm_step, one_file, one_step, said, step_advice, step_said, Grain, }; use io_harness::tools::Workspace; use io_harness::{ @@ -293,21 +293,33 @@ async fn f7_one_step_is_undone_and_the_rest_of_the_run_stands() { /// **F7 — undoing the whole run emits `EventKind::Rewound`.** /// /// The event this product has never fired. `src/rewind.rs:200` called the plain -/// `rewind_run` from 0.4.0 until this release, so the arm rendering this in -/// `src/events.rs` was unreachable from the day it was written. +/// `rewind_run` from 0.4.0 until this release, so the event had never fired. +/// +/// **What that buys is not a line on screen.** `rewound` is `Disposition::Silent` +/// and routes to io-cli's own rewind summary; `src/events.rs` has no arm for it +/// and correctly draws nothing. What the observed form buys is that the event +/// exists at all — it reaches `[[hook]]` observers, `io exec --json` and the +/// durable trace, none of which it had ever reached. An earlier draft of this +/// file claimed there was an unreachable renderer arm; there is not, and the +/// adversarial review caught the overclaim. +/// +/// It goes through `rewind::last_turn`, which is the production path: that is +/// what moves the conversation head back, and a thin wrapper beside it would be +/// a second entry point to one act. #[tokio::test] async fn f7_the_whole_run_emits_rewound() { let mut fixture = Fixture::new(); std::fs::write(fixture.dir.path().join("one.rs"), "before\n").expect("a starting file"); - let run = fixture + fixture .turn_writing("a whole run", &[("one.rs", "after\n")]) .await; let watcher = Collected::default(); - let rewound = - whole_run(&fixture.workspace(), &fixture.store, run, &watcher).expect("the undo runs"); + let undone = io_cli::rewind::last_turn(&mut fixture.session, &fixture.store, &watcher) + .expect("the undo runs") + .expect("there was a turn to undo"); - assert!(!rewound.files.is_empty(), "there was a file to put back"); + assert!(!undone.restored.is_empty(), "there was a file to put back"); assert_eq!( fixture.read("one.rs").as_deref(), Some("before\n"), @@ -425,10 +437,18 @@ fn the_file_confirmation_discloses_what_a_restore_overwrites() { assert!(detail.contains("overwritten"), "{detail}"); } -/// The grains name themselves in the words the surface uses. +/// `Grain` is a description of what was asked for and nothing more. +/// +/// It carries no `label()`: the confirmations build their own sentences and the +/// run arm never renders one, so a formatter here would be reachable only from +/// this file — the tested-but-unreachable shape this codebase has shipped three +/// times. The adversarial review caught it before it became a fourth. #[test] -fn the_grains_name_themselves() { - assert!(Grain::File("src/a.rs".into()).label().contains("src/a.rs")); - assert!(Grain::Step(7).label().contains('7')); - assert_eq!(Grain::Run.label(), "the whole run"); +fn the_grains_carry_only_what_was_asked_for() { + assert_eq!( + Grain::File("src/a.rs".into()), + Grain::File("src/a.rs".into()) + ); + assert_ne!(Grain::Step(1), Grain::Step(2)); + assert_ne!(Grain::Run, Grain::Step(1)); }