diff --git a/CHANGELOG.md b/CHANGELOG.md index a5037f5..06cbd3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,154 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +## [0.23.0] - 2026-08-28 + +A run that paused is answered rather than abandoned. Everything needed to do it +has been public in io-harness since 0.10.0; what this interface did with a paused +run until now was print its id and walk away. + +**`/resume` reads what each session's last run stopped on and puts it on the row +you choose from.** A word rather than a symbol, so it survives `NO_COLOR`, +`--plain` and the ASCII glyph set: `asks` for a question nobody answered, `plan` +for an approach nobody decided, `tool` for a call whose outcome nobody recorded, +`died` for a process that went away leaving committed work behind, `ended` for a +turn the operator stopped themselves. A session with nothing outstanding carries +no mark, so the list is ragged by construction — which is what buys a mark that +says *which* state it is to somebody choosing on it with no legend on screen. +There is no new store read to pay for it: the walk that builds the list already +knows each session's newest run, because that run is what put the session where +it is in the list. + +**Choosing a marked session opens the same overlay the run would have opened +while it was live, and the answer carries that run on from the step it stopped +at.** Not a new run with the answer pasted into its goal: the observation ledger, +the token budget and the elapsed clock are the original run's. A plan is +approved, sent back with a correction, or cancelled — `cancel` ends the run as +`PlanRejected`, which is what "do not do this at all" means, rather than spending +the rest of the budget on the approach just refused. An interrupted call is +retried, abandoned, or asserted to have landed, and the operator's account of +what it returned is filed against the step the call was *made* on, so the resumed +run reads a transcript in which the tool answered where it was asked. A run whose +process merely died carries on from `last_step + 1`. `Esc` leaves any of them +parked exactly as it was found. + +**A turn the operator interrupted cannot be continued, and this release says so +instead of pretending.** `Ctrl+C` returns `Flow::Cancel`, io-harness records the +outcome `cancelled`, `finish_run` maps that to a *completed* status, and every +resume entry point short-circuits on a completed run and hands back the original +outcome having driven nothing. So the single most common way a turn stops is the +one way it cannot be resumed. `/resume` reports it as ended by you and offers +`/fork` from the preceding turn, which is the honest neighbouring answer; `io +resume` refuses it in the same sentence, before a provider is built. The +published io-harness documentation disagrees — `Steer::interrupt` says such a +turn "stays resumable" — and is contradicted by the run loop in the same crate. +Reported upstream; not worked around here, because a workaround for a +short-circuit that returns success would be this interface inventing an outcome. + +**`io resume` is the headless door.** `--list` enumerates the parked runs — a +row per run with what it is waiting on, the id that addresses that pause and the +step it stopped at, as NDJSON under `--json` — and one is carried on by id with +the decision on the command line: `--answer`, `--plan` with `--correction`, +`--recovery` with `--account`, or nothing at all for a run whose process died. It +takes the same `--json`, `--policy` and `--provider` an `io exec` takes, meaning +the same three things. There is deliberately no `--sandbox`: a resumed run +already started under a boundary, and a flag that widened it halfway through +would be a widening nobody asked for at the point nobody is watching. clap cannot +see which pause a run is on, so a flag for the wrong one is settled against the +store and **refused** rather than ignored — `--plan approve` at a run holding a +question is somebody acting on the wrong thing. + +**Exit `4` now names the pause and the invocation that answers it.** It named the +run id, which addresses none of the four pauses; the closing line now names the +`question_id`, `plan_id` or `attempt_id` and prints the `io resume` that decides +it. **No exit code was renumbered and none was added** — the six have meant what +they mean since 0.5.0, and `4` was given to a pause nothing could yet answer for +exactly this release. An approval remains the one pause `io resume` cannot take: +it is answered by the person the run asked, at the terminal it asked from, and +io-harness publishes no entry point that takes one. + +**A bare run needs `--goal`, and is refused without it.** `runs.goal` has no +public reader, so a contract cannot be rebuilt from a run alone. For a run that +served a session turn the operator's own words are recoverable from the turn; for +a run `io exec` or any other non-session caller started they are not, and +resuming against an empty goal would spend a budget on a task nobody set. A +`--goal` supplied for a run that has one wins, because that is an operator +re-aiming their own run. + +**One `io` at a time on one conversation.** One store serves the whole machine, +so two terminals in one repository is the ordinary case — and they are not in +conflict, because starting `io` creates a new session every time and each gets +its own. What two processes can genuinely contend over is a single *session*, +and that happens in one place: `/resume`, when one enters a session another +already has open. Nothing guarded it, so both advanced the same head and the +loser of that race had paid for a turn that was then orphaned off the head path +— still in the store, correctly parented, never shown again by a history that +walks back from the head. + +Each session is now held under an advisory whole-file lock through +`std::fs::File::try_lock`, which is stable on this crate's MSRV and needs no +dependency: `flock` on unix, `LockFileEx` on Windows, released by the kernel on +exit, on panic and on `kill -9`. There is no stale lock to reap and no pid file +to sweep, and a lock that cannot be taken for an ordinary filesystem reason +warns rather than refusing to start — trading the guard for "io will not run on +this machine" would be the worse failure. The lock a session takes when it opens +contests nothing, since that session did not exist a moment earlier; what it +does is write down who owns it, so the next process to reach for it can be told. +`/resume` into a session another `io` holds is refused. Two `io` on two +different sessions are not, `io exec` and `io resume` take no lock at all, and +for everything the lock does not see the guard of last resort is io-harness's +own compare-and-swap on the head. + +**What the refusal can say about the holder is what io-cli itself wrote beside +the lock**: the pid, the workspace root, the `io` version and the instant that +process started. It is not the operating system's account of that process, and +the release says so plainly rather than implying more. `/proc` is one platform's +answer to a three-platform question, `ps` and `tasklist` are two more, and the +dependency that would abstract them is one this crate's own gate forbids — so a +pid shown here is a number io wrote down. The record is a second file beside the +lock rather than the lock's contents, because a Windows byte-range lock is +mandatory and the refused process could not read the file it is being refused on. +The twelve-hour lease exists only for a home on a network filesystem, where +an advisory lock is not this program's business and the record's own timestamp is +the only evidence there is. + +**Two defects are fixed.** `/undo` wrote the session head unconditionally, so +undoing in one terminal silently clobbered a head another had just advanced, +orphaning a turn that had been asked for, answered and paid for; it is a +compare-and-swap now — `set_session_head_if` with the head the undo believed it +was replacing — and a lost race is **refused and reported** rather than retried +or forced, because which turn survives is the operator's call and the losing turn +is only recoverable while it is still on somebody's head. And `failure::advice` +had no arm for `Error::Conflict`, whose own text reads `run {run_id} is held by +another owner until {expires_at}` — on the head shape that calls a session id a +run and ends on the word "until", because a head conflict populates no expiry. +That variant is now matched on its value before any text is read, and the +harness's line is dropped rather than kept underneath it: the module's rule is +that terse text is worth putting in front of an operator, and it was never a rule +about text that is wrong. + +**An interactive turn that ends parked says so.** io-harness returns +`AwaitingAnswer`, `AwaitingPlan` and `AwaitingRecovery` as an ordinary `Ok`, so +through 0.22.0 the arm matched and dropped them, and the operator got their +prompt back with no sign that a run was sitting in the store waiting for a +sentence from them. Every other way a turn can end had a line. + +**No command was added and none changed group.** `/resume` was extended rather +than joined, so the palette still holds thirty commands in the same four groups, +and `/resume`'s own description is the one line in that table this release +rewrites. + +**Three limitations are named rather than left to be found.** An interrupted call +in a *contained* run has no resume entry point — io-harness 0.69 publishes +`resume_tree_with_answer` and `resume_tree_with_plan_decision` and no recovery +twin, and driving a tree root through the flat one would silently drop the +containment it was running under, so that case is refused. A conversational turn +that paused and was resumed reads back afterwards as a run, because +`Store::turn_kind` and `Store::set_turn_kind` are both `pub(crate)`. And +`io resume --list` classifies every run in the store rather than querying for the +parked ones, which is linear in the store's whole history: the store-side query +that would fix it is not published. + ## [0.22.0] - 2026-08-27 What the work cost and whether it worked — two questions this interface has been @@ -1900,6 +2048,6 @@ 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.1.1...HEAD +[Unreleased]: https://github.com/initorigin/io-cli/compare/v0.23.0...HEAD [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 0723a64..d0a0488 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1850,7 +1850,7 @@ dependencies = [ [[package]] name = "io-cli" -version = "0.22.0" +version = "0.23.0" dependencies = [ "clap", "crossterm", diff --git a/Cargo.toml b/Cargo.toml index cc25182..f323fe2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "io-cli" -version = "0.22.0" +version = "0.23.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 ff76570..0af535c 100644 --- a/README.md +++ b/README.md @@ -41,8 +41,9 @@ on one row and the keys and the posture on the next.](docs/screenshot.png) | **A fan-out you can watch** | Contained turns spawn children under one shared ceiling; `Ctrl+F` shows the tree and what it is costing | | **Your file, in force** | Every section of `io.toml` bounds a session turn as it bounds `io exec`; the budgets in force are on the status line with what is left of them, and `/status` commits the whole state — policy layers, sandbox backend, caps, budgets, connections — into the scrollback | | **Undo** | `Esc Esc` at an empty prompt rewinds the last turn — its files, its memory and the conversation head | -| **Conversations that survive** | `/resume` reopens an earlier session, `/fork` continues from an earlier turn, `/clear` starts fresh without leaving | -| **Headless** | `io exec` runs one goal to completion with documented exit codes and `--json` | +| **Conversations that survive** | `/resume` reopens an earlier session and answers whatever its last run stopped on, `/fork` continues from an earlier turn, `/clear` starts fresh without leaving | +| **A paused run, answered** | A question, a plan or an interrupted call is decided where it was left and the run carries on from the step it stopped at — from a session, or from a script with `io resume` | +| **Headless** | `io exec` runs one goal to completion with documented exit codes and `--json`, and `io resume` carries a parked one on | | **Readable without seeing it** | `--plain` animates nothing and commits every state change as text, for a screen reader, a braille display or a log | | **Markdown, rendered** | Headings, bullets, code and emphasis drawn as themselves rather than printed as notation | | **Documents, read and written** | Spreadsheets, Word, slide decks, PDFs and barcodes through io-harness's own tools — twelve of them, six of which write, every one under the same gate as any other read or write | @@ -449,7 +450,7 @@ above a row that ranked there for reasons having nothing to do with it. | Command | Does | | --- | --- | | `/clear` | start a new conversation; this one stays in /resume | -| `/resume` | reopen an earlier session where it stopped | +| `/resume` | reopen an earlier session and answer whatever its last run is waiting on | | `/fork` | continue from an earlier turn of this conversation | | `/setup` | run the first-run wizard again | | `/exit` | leave | @@ -504,6 +505,11 @@ here you use once: the others are returned to for the life of the install. It writes files, which is the whole reason it is under **configure**. See [Bringing your setup across](#bringing-your-setup-across). +`/resume` does more than reopen from 0.23.0: each row says what that session's +last run stopped on, and choosing one answers it. No command was added for that — +the one that was already there was extended. See [When a run stops for +you](#when-a-run-stops-for-you). + `/cost` and `/stats` are new in 0.22.0 and are two commands rather than one because they are two questions: `/cost` says what the work cost, `/stats` says whether it worked. See [What it costs](#what-it-costs). @@ -586,6 +592,96 @@ refuse a large payload without saying so. `/theme` and `/model` change this session only and say so. Making a choice permanent is `io setup`. +## When a run stops for you + +A turn does not always end on its own. The agent can ask what you meant, or +propose a plan before it works; a tool call can be interrupted in a way +io-harness records but cannot judge; and a process that goes away mid-loop leaves +a run with committed work and no ending. Through 0.22.0 all four were left where +they fell — the run was in the store, and nothing here would open it again. + +**`/resume` says what each session's last run stopped on, on the row you choose +it from.** The mark is a word rather than a symbol, so it survives `NO_COLOR`, +`--plain` and the ASCII glyph set: `asks` for a question nobody answered, `plan` +for an approach nobody decided, `tool` for a call whose outcome nobody recorded, +`died` for a process that went away and left committed work behind, `ended` for a +turn you stopped yourself. A session with nothing outstanding carries no mark at +all, so the list is ragged by construction and there is no column to read down. + +**Choosing a marked session opens the same overlay the run would have opened +while it was live**, and what you say carries *that* run on from the step it +stopped at: the observation ledger, the token budget and the elapsed clock are +the run's own rather than a new run's. A plan is approved, sent back with a +correction, or cancelled outright. An interrupted call is retried or abandoned +here — `r` and `a` — and can also be **asserted to have landed**, which takes an +account of what it returned and is therefore offered by `io resume --recovery +completed --account "…"` rather than by a keystroke. What you say it returned is +filed against the step the call was made on, not the step the run has now +reached, so the resumed run reads a transcript in which the tool answered where +it was asked. A run whose +process merely died carries on from its last committed step plus one. `Esc` +leaves any of them parked exactly as it was found. + +**A turn you interrupted is finished, not paused, and it is the one pause that +cannot be answered.** `Ctrl+C` makes io-harness record the outcome `cancelled`, +which is mapped to a *completed* run, and every one of its resume entry points +short-circuits on a completed run and hands back the original outcome having +driven nothing. So the most common way a turn stops is the one way it cannot be +continued. `/resume` reports such a session as ended by you and points at `/fork` +from the turn before it, which is the honest neighbouring answer rather than a +button that would quietly do nothing. io-harness's published documentation says +such a turn "stays resumable"; it is contradicted by the run loop in the same +crate, and that is reported upstream rather than worked around here. + +**A turn that ends parked now says so.** Through 0.22.0 the prompt came back with +no sign that a run was sitting in the store waiting for a sentence from you. + +### One `io` at a time on one conversation + +One store serves this whole machine, so two terminals in one repository is the +ordinary case rather than the exotic one — and they are **not** in conflict. +Starting `io` creates a new session every time, so each terminal gets its own +conversation and neither is refused. + +What two of them can genuinely contend over is a single *session*, and that +happens in one place: `/resume`, when one process enters a session another +already has open. Until 0.23.0 nothing guarded it — both advanced the same +conversation head, and the loser of that race had paid for a turn that was then +orphaned off the head path: still in the store, correctly parented, and never +shown again by a history that walks back from the head. + +Each session is held under an advisory whole-file lock, and `/resume` into one +another `io` is holding is refused rather than taken. The lock is the kernel's — +`flock` on unix, `LockFileEx` on Windows — so it is released on exit, on a panic +and on `kill -9`: there is no stale lock to reap and no pid file to sweep. The +lock a session takes when it starts never contests anything, because that session +did not exist a moment earlier; what it does is write down who owns it, so the +next process to reach for it can be told. + +**What the refusal can say about the holder is what io-cli itself wrote beside +the lock**, and no more: the process id, the workspace root, the `io` version and +the instant that process started. It is not the operating system's account of +that process. Asking the operating system means `/proc` on one platform, `ps` on +another and `tasklist` on a third, or a dependency this crate does not carry — so +the pid you are shown is a number `io` wrote down, and on a machine that has +since reused it, it names something that is not `io`. The lease exists only for +the case the kernel cannot cover, a home on a network filesystem, where an +advisory lock is not this program's business; there the record's own timestamp is +all the evidence there is. + +A lock that cannot be taken for an ordinary filesystem reason does not stop the +session — you are told, and it opens. The guard exists to prevent one specific +corruption, and trading it for "io will not start on this machine" would be the +worse failure. + +**What it does not cover.** Two `io` in one repository on two different sessions +are not in conflict and are not stopped. `io exec` and `io resume` take no lock +at all, so an `io resume` run beside a terminal holding the same session is not +refused by this. For everything the lock does not see, the guard of last resort +is io-harness's own compare-and-swap on the conversation head: the second writer +is refused, told, and its turn is not silently orphaned — which is the defect +`/undo` carried until this release. + ## What it costs **`/cost` commits what has been spent, and `/stats` commits how the runs have @@ -1203,13 +1299,26 @@ what it already does with a policy refusal. | `1` | it never got that far — no provider, a bad credential, an unreadable configuration, a usage error | | `2` | a boundary said no: denied, refused, or a rejected plan | | `3` | a ceiling was reached: steps, time, tokens, or the tree's shared budget | -| `4` | the run stopped needing a human: it asked a question, or proposed a plan | +| `4` | the run stopped needing a human: it asked a question, proposed a plan, or was interrupted in the middle of a call | | `5` | it ended without finishing: stalled, escalated, or cancelled | A ceiling is `3` and not `0` because io-harness returns one as a *successful call* whose outcome says a limit was hit; a status read off the result alone would call a truncated run a finished one. +**Exit `4` names the pause from 0.23.0, and the invocation that answers it.** +The closing line used to name the run id and nothing else, which addressed +none of the four pauses; it now names the question, plan or call the run stopped +on and the `io resume` that decides it. No exit code was renumbered and none was +added: the six have meant what they mean here since `io exec` shipped them in +0.5.0, and `4` was given to a pause that could not yet be answered for exactly +this release. + +An approval is the one pause `io resume` cannot take: it is answered by the +person the run asked, at the terminal it asked from, and there is no resume entry +point in io-harness that takes one. A headless run never reaches it, because +every approval there is declined. + **Give the goal an end condition.** How a run ends is the agent's behaviour, not this interface's: a goal with no clear stopping point can keep the agent working after the useful part is done, until io-harness's stall policy ends the run — and @@ -1248,6 +1357,63 @@ format io-cli owns rather than one it passes through. Progress, warnings and the closing summary go to stderr, so redirecting it leaves the data alone. +### Resuming without a terminal + +`io resume` is the headless door to a run that stopped for a person. `--list` +enumerates the runs waiting for one and drives nothing — it reads the store, +calls no provider and takes no lease on anything it lists. Naming a run by id +resumes it, with the decision on the command line: + +```sh +io resume --list +io resume --list --json | jq -r 'select(.waiting_on=="question") | .run_id' +io resume 41 --answer "use the parser that is already there" +io resume 41 --plan revise --correction "leave the public API alone" +io resume 41 --recovery completed --account "the tag was pushed; CI is green" +io resume 41 --goal "add a test for the parser and run it" +``` + +| Flag | Does | +| --- | --- | +| `--list` | list the runs waiting for a person and carry none of them on | +| `--answer ` | answer the question the run stopped on | +| `--plan ` | `approve`, `revise` or `cancel` the plan the run proposed | +| `--correction ` | what the plan should do differently; required by `--plan revise` and refused without it | +| `--recovery ` | `retry`, `abandon` or `completed` — what happened to the call the run was interrupted in the middle of | +| `--account ` | what that call returned; required by `--recovery completed` and refused without it | +| `--goal ` | what the run was asked to do, for a run whose goal cannot be recovered | +| `--json` | write the resumed run's events, and `--list`'s rows, as newline-delimited JSON | +| `--policy ` | the posture for the rest of this run; defaults to the one the run itself recorded | +| `--provider ` | `openrouter`, `anthropic` or `openai` — take the credential and model from the environment | + +**Each pause takes its own input, and exactly one.** clap cannot see which pause +a run is on, so a flag for the wrong one is checked against the store and refused +rather than ignored: `--plan approve` typed at a run holding a question is +somebody acting on the wrong thing, and the refusal says what that run is +actually waiting on and what to type. A run whose process merely died takes no +flag at all — `io resume ` carries it on from its last committed step. + +**There is no `--sandbox`.** A resumed run already started under a boundary, and +the confinement it carries on under is the project's `[sandbox]`. A flag that +widened it halfway through a run would be a widening nobody asked for at the +point nobody is watching. + +**`--goal` is required for a run that served no session turn** — one `io exec` +started, or any other non-session caller. io-harness publishes no reader for +`runs.goal`, so for a run that served a session turn your own words are +recoverable from the turn, and for a bare run they are not. Rather than resuming +against an empty goal and spending a budget on a task nobody set, `io resume` +asks for it. Supplying `--goal` for a run that has one is you re-aiming your own +run, and it wins. + +**A turn you interrupted is refused here in the same words the session uses**, +before a provider is built — see [When a run stops for +you](#when-a-run-stops-for-you) for why it cannot be carried on. + +The exit status is the table above: a resumed run that pauses again exits `4` +naming the new pause, and `io resume --list` exits `0` whether or not it found +anything. + ## Configuration io-cli has no configuration parser. io-harness owns discovery and layering, and @@ -1538,14 +1704,15 @@ parses no skill file: the five it ships from 0.19.0 are bodies it carries and writes to disk, and after that they are read the same way yours are — by the model, through the tool, under the policy. -**`io exec` runs one goal and stops, and a run that pauses stays paused.** An -agent that asks a question about what you meant, or proposes a plan, ends the run -at exit `4` with the question persisted in the store. That is io-harness's -behaviour and it is the right one — a machine answering a question about intent -on your behalf sends the agent down a path nobody chose — but there is no `io -resume` in this release to answer it and carry on, so the run is parked rather -than lost. The closing line names its id. Approvals are the one pause that cannot -happen, because they are declined rather than deferred. +**`io exec` runs one goal and stops, and a run that pauses is still not answered +by a machine.** An agent that asks a question about what you meant, or proposes a +plan, ends the run at exit `4` with the pause persisted in the store. That is +io-harness's behaviour and it is the right one — a machine answering a question +about intent on your behalf sends the agent down a path nobody chose — so `io +exec` parks the run and says which pause it is parked on. Answering it is a +person's job, and from 0.23.0 there is a door for that: `io resume` from a +script, `/resume` from a session. Approvals are the one pause that cannot happen +in a headless run, because they are declined rather than deferred. There are no `--max-steps`, `--timeout` or `--max-tokens` flags either: `[run]` in the configuration file expresses all three, and a CI job's limits belong with diff --git a/src/app.rs b/src/app.rs index c963666..85a446a 100644 --- a/src/app.rs +++ b/src/app.rs @@ -119,6 +119,21 @@ pub enum Command { /// The second `Esc`: undo the last turn — its files, its memory and the /// conversation head. Rewind, + /// A question opened from the store has been answered, and nobody took the + /// answer. + /// + /// **A command for exactly the reason [`Command::Attach`] is one**: what + /// this asks for lives in the harness's store, and the store belongs to the + /// driver. A *live* question's answer travels down the channel its run is + /// blocked on and never reaches here; a question a previous process left + /// behind has no such channel, so the answer comes back out to be delivered + /// with [`crate::resume::answer_question`]. `None` is a decline, which + /// leaves the run parked exactly as it was found. + Answered(Option), + /// A plan opened from the store has been decided, and nobody took the + /// verdict. See [`Command::Answered`] — this is the same arrangement for + /// [`crate::resume::decide_plan`]. + Decided(io_harness::PlanVerdict), } /// What a paste turned out to be. @@ -393,15 +408,35 @@ impl App { self.intent = Some(crate::intent::Intent::new(asked)); } + /// A question a *previous* process left in the store, opened to be answered + /// now. + /// + /// The same widget as [`Self::open_intent`], and deliberately the same field: + /// one `render`, one `key`, one [`Self::modal`], so the resumed surface + /// cannot drift from the live one. What differs is only where the answer + /// goes, which [`crate::intent::Intent`] carries with it — a live question + /// sends down the channel its run is waiting on, and a stored one has no + /// run listening, so its answer comes back out of [`Self::answer_intent`] + /// for the caller to deliver. + pub fn open_resumed_intent(&mut self, intent: crate::intent::Intent) { + self.intent = Some(intent); + } + /// Answer the open question, or decline it with `None`. /// /// Declining is not a refusal: io-harness persists the question and pauses /// the run, so the answer can arrive after this process has exited. What is /// said in the scrollback says which of the two happened. - pub fn answer_intent(&mut self, answer: Option) { - let Some(intent) = self.intent.take() else { - return; - }; + /// + /// **Returns what nobody took.** A live question's answer travels down the + /// channel its run is blocked on and this is `None`; a question opened from + /// the store has no such channel, so the answer comes back here and the + /// caller must hand it to [`crate::resume`]. Discarding it there would drop + /// the operator's answer in silence, which is why it is returned rather than + /// swallowed as it was before 0.23.0. + #[must_use = "a resumed question's answer is delivered by the caller, not by the overlay"] + pub fn answer_intent(&mut self, answer: Option) -> Option> { + let intent = self.intent.take()?; match &answer { Some(text) => self.record( Tone::Muted, @@ -415,7 +450,7 @@ impl App { ), ), } - intent.resolve(answer); + intent.resolve(answer) } /// A plan was proposed and nothing has been done about it yet. @@ -423,16 +458,29 @@ impl App { self.plan = Some(crate::plan::Review::new(proposed)); } + /// A plan a *previous* process left in the store, opened to be decided now. + /// + /// The same field and the same widget as [`Self::open_plan`], for the reason + /// [`Self::open_resumed_intent`] gives. + pub fn open_resumed_plan(&mut self, review: crate::plan::Review) { + self.plan = Some(review); + } + /// Decide the open plan. The overlay closes and the run acts on the verdict. /// /// The line committed here is io-cli's own note of what it sent; the /// harness's `PlanDecided` arrives separately on the event stream. They agree /// because the verdict travelled one way, and if they ever disagree this is /// where it shows. - pub fn decide_plan(&mut self, verdict: io_harness::PlanVerdict) { - let Some(plan) = self.plan.take() else { - return; - }; + /// + /// **Returns what nobody took**, for the reason [`Self::answer_intent`] + /// gives: a plan opened from the store has no run listening for its verdict. + #[must_use = "a resumed plan's verdict is delivered by the caller, not by the overlay"] + pub fn decide_plan( + &mut self, + verdict: io_harness::PlanVerdict, + ) -> Option> { + let plan = self.plan.take()?; let dash = self.theme.glyphs.dash; let (tone, said) = match &verdict { io_harness::PlanVerdict::Approve => ( @@ -447,7 +495,41 @@ impl App { } }; self.record(tone, said); - plan.resolve(Some(verdict)); + plan.resolve(Some(verdict)) + } + + /// Close a question or a plan opened from the store **without deciding it**, + /// leaving the run parked exactly as it was found. + /// + /// This exists because `Esc` means different things to the two overlays, and + /// both meanings are right for a live turn. On a question `Esc` declines, + /// which leaves the run parked — so backing out is already reachable. On a + /// plan `Esc` is [`io_harness::PlanVerdict::Cancel`], a real decision that + /// ends the run — so an operator who opened a parked plan to look at it had + /// no way back out that did not also throw the plan away. The interrupt key + /// is that way out, and it is the same key that means "get me out of this" + /// everywhere else in the interface. + /// + /// A no-op when nothing is open, and it never touches an approval: an + /// approval belongs to a run that is still running and blocked on it, so + /// there is no parked state to leave it in. + pub fn leave_resumed(&mut self) { + // **Both takes run.** Written with `||` this short-circuited: with a + // question open the plan was never taken, so a state where both were set + // would leave one of them on screen with nothing driving it. Nothing + // opens two today, and this must not be the line that makes that + // assumption load-bearing. + let had_intent = self.intent.take().is_some(); + let had_plan = self.plan.take().is_some(); + if had_intent || had_plan { + self.record( + Tone::Muted, + format!( + "left where it was {} the run is still parked, and /resume opens it again", + self.theme.glyphs.dash + ), + ); + } } /// Whether a modal surface owns the keyboard. @@ -1398,7 +1480,13 @@ impl App { // approval's and above everything else. if let Some(open) = self.intent.as_mut().filter(|_| !interrupting) { if let Some(answer) = open.key(key) { - self.answer_intent(answer); + // Handed back rather than dropped when the question came from the + // store: `answer_intent` returns whatever no run was waiting for, + // and the one thing that must never happen to an operator's + // answer is that it goes nowhere quietly. + if let Some(undelivered) = self.answer_intent(answer) { + return Command::Answered(undelivered); + } } return Command::None; } @@ -1407,7 +1495,12 @@ impl App { // cancel. if let Some(open) = self.plan.as_mut().filter(|_| !interrupting) { if let Some(verdict) = open.key(key) { - self.decide_plan(verdict); + // As above. A verdict on a plan nobody is waiting for is the + // driver's to deliver, and `Review::resolve` answers `Some` only + // on that path. + if let Some(Some(undelivered)) = self.decide_plan(verdict) { + return Command::Decided(undelivered); + } } return Command::None; } diff --git a/src/cli.rs b/src/cli.rs index 34aa99f..3258d46 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1,9 +1,10 @@ -//! The command line. A handful of flags and two subcommands, because everything -//! else is a slash command inside the session. +//! The command line. A handful of flags, and a subcommand for each thing that is +//! not a session, because everything else is a slash command inside one. //! //! The count is deliberately not stated. It said "five" from 0.2.0 until 0.6.0 -//! and was wrong for most of that time, which is the argument against writing a -//! number into prose that nothing checks. +//! and was wrong for most of that time, and it said "two subcommands" until +//! 0.23.0 added a third — which is the argument against writing a number into +//! prose that nothing checks. use std::path::PathBuf; @@ -74,6 +75,10 @@ pub enum Command { /// Run one goal to completion without a terminal, and exit with a status /// that says how the run ended. Exec(Exec), + /// List the runs parked in the store, or carry one of them on: answer its + /// question, decide its plan, or say what happened to the call it stopped + /// mid-way through. + Resume(Resume), } /// `io exec` — the headless entry point. @@ -122,6 +127,130 @@ pub struct Exec { pub provider: Option, } +/// `io resume` — the parked runs, and the way to carry one on. +/// +/// The register is [`Exec`]'s: a positional for the thing being acted on, then +/// flags, then the same `--json` / `--policy` / `--provider` trio a headless run +/// takes, meaning the same three things. What is deliberately absent is +/// `--sandbox`: a resumed run is one that already started under a boundary, and +/// the confinement it should carry on under is the project's `[sandbox]`, which +/// [`crate::contract::configured`] already puts on the contract. A flag that +/// widened it halfway through a run would be a widening nobody asked for at the +/// point nobody is watching. +/// +/// **Each pause takes its own input, and exactly one.** A question wants free +/// text, a plan wants a verdict and — for `revise` — a correction, an +/// interrupted call wants a decision and — for `completed` — the operator's +/// account of what it returned, and a run whose process merely died wants +/// nothing at all. clap cannot see which pause a run is on, so which flag was +/// the right one is settled against the store by [`crate::exec::decision_for`]. +#[derive(Debug, clap::Args)] +pub struct Resume { + /// The run to carry on. Omitted with `--list`. + #[arg(value_name = "RUN_ID", required_unless_present = "list")] + pub run: Option, + + /// List the runs waiting for a person and carry none of them on. + /// + /// Reads the store and calls no provider, so it costs nothing and takes no + /// lease on anything it lists. + #[arg( + long, + conflicts_with_all = ["run", "answer", "plan", "correction", "recovery", "account", "goal"] + )] + pub list: bool, + + /// The answer to the question the run stopped on. + #[arg(long, value_name = "TEXT")] + pub answer: Option, + + /// What to do with the plan the run proposed. + #[arg(long, value_enum, value_name = "VERDICT")] + pub plan: Option, + + /// What the plan should do differently. Required by `--plan revise` and + /// meaningless without it. + #[arg(long, value_name = "TEXT", requires = "plan")] + pub correction: Option, + + /// What happened to the call the run was interrupted in the middle of. + #[arg(long, value_enum, value_name = "DECISION")] + pub recovery: Option, + + /// What the call returned. Required by `--recovery completed` and + /// meaningless without it. + /// + /// Nothing validates it: the operator is asserting a fact about the outside + /// world that no code here can check. + #[arg(long, value_name = "TEXT", requires = "recovery")] + pub account: Option, + + /// What the run was asked to do. + /// + /// `runs.goal` has no public reader, so a contract cannot be rebuilt from a + /// run alone. For a run that served a session turn the operator's own words + /// are recoverable from the turn; for a *bare* run — one `io exec` or any + /// other non-session caller started — they are not, and this is the only way + /// to supply them. + #[arg(long, value_name = "TEXT")] + pub goal: Option, + + /// Write the resumed run's events to stdout as newline-delimited JSON, and + /// `--list`'s rows as one object per line, instead of prose. + #[arg(long)] + pub json: bool, + + /// The permission posture for the rest of this run. Defaults to what the run + /// itself recorded, and then to `[policy]`. + /// + /// `ask-writes` is refused for the reason `io exec` refuses it. + #[arg(long, value_enum, value_name = "POSTURE")] + pub policy: Option, + + /// Take the provider from the environment instead of a configuration file. + #[arg(long, value_enum, value_name = "NAME")] + pub provider: Option, +} + +/// `--plan`, in `io_harness::PlanVerdict`'s own words. +/// +/// `cancel` is a decision and not a refusal to decide: it ends the run as +/// `PlanRejected`, which is what "do not do this at all" means and is why it +/// goes through the plan entry point rather than being turned into a plain +/// resume that would spend the rest of the budget on the approach just refused. +#[derive(Debug, Clone, Copy, PartialEq, Eq, clap::ValueEnum)] +pub enum PlanFlag { + /// Carry it out. + #[value(name = "approve")] + Approve, + /// Not this one. `--correction` says what to change. + #[value(name = "revise")] + Revise, + /// Do not do this at all. + #[value(name = "cancel")] + Cancel, +} + +/// `--recovery`, for a call whose outcome only a person can establish. +/// +/// The names are the operator's rather than the harness's on one of the three: +/// io-harness spells the middle one `Abort`, which reads as *stop the program* +/// on a command line, and what it actually means is *do not make that call, and +/// stop*. +#[derive(Debug, Clone, Copy, PartialEq, Eq, clap::ValueEnum)] +pub enum RecoveryFlag { + /// Make the call again. For a call established not to have landed, or one + /// that is harmless to repeat. + #[value(name = "retry")] + Retry, + /// Do not make the call, and do not carry on. + #[value(name = "abandon")] + Abandon, + /// The call landed. `--account` is what the agent is told it returned. + #[value(name = "completed")] + Completed, +} + /// `--provider`, for a run with no configuration file. /// /// `compatible` is deliberately absent. io-harness gives it no `from_env` of its diff --git a/src/commands.rs b/src/commands.rs index e9e73f9..b104cd5 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -117,7 +117,10 @@ pub const COMMANDS: &[(&str, &str)] = &[ ("/setup", "run the first-run wizard again"), ("/theme", "change the theme for this session"), ("/model", "change the model the next turn is sent to"), - ("/resume", "reopen an earlier session where it stopped"), + ( + "/resume", + "reopen an earlier session and answer whatever its last run is waiting on", + ), ( "/fork", "continue from an earlier turn of this conversation", @@ -822,7 +825,15 @@ pub enum Action { Theme, /// Open the model picker. Model, - /// Open the picker over the sessions the store holds. + /// Open the picker over the sessions the store holds, and answer whatever + /// the chosen one's last run stopped on. + /// + /// **The second half is 0.23.0's and it is what the word now means.** Until + /// then this reopened a session and nothing more — a question the agent had + /// asked, a plan it had proposed and a call that never finished all stayed in + /// the store while the interface offered a fresh prompt. See + /// [`crate::resume`] for the four kinds of pause and for the one that cannot + /// be answered at all. Resume, /// Open the picker over the turns of the conversation that is open. Fork, diff --git a/src/exec.rs b/src/exec.rs index 5d150ee..fb4ba42 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -1,4 +1,13 @@ -//! `io exec` — one goal, run to completion, with no terminal. +//! `io exec` — one goal, run to completion, with no terminal — and, since +//! 0.23.0, `io resume`, which carries on a run that stopped for a person. +//! +//! The second subcommand lives here rather than in [`crate::resume`] for the +//! reason the first one lives here rather than in `main.rs`: that module is the +//! library half — it classifies a pause and drives the harness's resume entry +//! points — and this one is the half that reads a command line, chooses a +//! provider, writes to two streams and returns an exit status. The exit statuses +//! are the same six, through the same [`code`] table, because a resumed run ends +//! the way any other run ends. //! //! This is a second **consumer** of io-harness, not a second program. It opens //! the same store an interactive session opens, creates a session in it the same @@ -31,12 +40,13 @@ use std::io::Write; use std::sync::Mutex; use io_harness::{ - Config, DenyAll, ExecMode, Flow, Ignore, Observer, Policy, Provider, RunEvent, RunOutcome, - Session, Store, TaskContract, TurnResult, + Config, DenyAll, ExecMode, Flow, Ignore, Observer, PlanVerdict, Policy, Provider, ProviderSpec, + RecoveryDecision, RunEvent, RunOutcome, Session, Store, TaskContract, TurnResult, }; -use crate::cli::PolicyFlag; +use crate::cli::{PlanFlag, PolicyFlag, RecoveryFlag}; use crate::provider::{self, WithProvider}; +use crate::resume::Pending; use crate::settings::{self, Posture}; /// The run ended of its own accord. @@ -89,20 +99,30 @@ pub fn code(outcome: &RunOutcome) -> u8 { | RunOutcome::CostBudgetExceeded { .. } | RunOutcome::BudgetCeilingReached { .. } => CEILING, - // Not reachable while approvals are denied rather than deferred and there - // is no `io resume` to continue one. Mapped anyway so the table is total - // now and adding that subcommand later renumbers nothing. + // `AwaitingApproval` stays unreachable from here while approvals are + // denied rather than deferred; the other two are reachable, because a + // question about intent and a proposed plan pass through no approver at + // all. **The bet these codes were numbered on has now been settled.** + // 0.5.0 mapped all three when none of them could happen yet — its own + // release notes say the mapping exists "so that adding that subcommand + // later renumbers nothing". 0.23.0 added it, and nothing here moved. + // (Written as 0.13.0 here until 0.23.0, which was wrong: 0.13.0 is a + // later release and the CHANGELOG puts the sentence in 0.5.0's notes.) RunOutcome::AwaitingApproval { .. } | RunOutcome::AwaitingAnswer { .. } | RunOutcome::AwaitingPlan { .. } => PAUSED, // 0.65.0 — a resume that found a call started and never finished. It is a // pause needing a decision, so it belongs with the other three rather than - // with the failures. **Not unreachable, and the claim is deliberately not - // made:** a session turn registers no tool and no MCP server and cannot - // journal an attempt, but `io exec` applies the configuration to its own - // contract, so a configured `[[mcp]]` server puts a run of this subcommand - // exactly one interrupted call away from it. + // with the failures. **Not unreachable, and the reason given here was + // wrong until 0.23.0.** It said a session turn registers no tool and no + // MCP server and cannot journal an attempt. `crate::contract::session` + // attaches `[[mcp]]` servers, an MCP tool declares no `recovery` so it + // takes the harness's default of `Indeterminate`, and an attempt on one + // is therefore journalable on an ordinary interactive turn. The + // conclusion survives its reason: either arm reaches this outcome, and + // `io exec` reaches it the same way, since both build their contract from + // the same configuration. RunOutcome::AwaitingRecovery { .. } => PAUSED, RunOutcome::Stalled { .. } @@ -273,19 +293,46 @@ pub fn asks_nobody_can_answer(policy: &Policy) -> Option { )) } -/// The extra line a paused run gets, naming what was parked. +/// The extra line a paused run gets, naming the handle that addresses the pause +/// and the invocation that acts on it. +/// +/// **The run id is not that handle, and through 0.22.0 this line printed only the +/// run id.** Every resume entry point in io-harness takes a second number — the +/// question, the plan, the journalled call — and each is the row a compare-and-swap +/// is made against, which is what makes "was it me who answered" answerable at +/// all. The outcome carries that number and this function used to discard it, so +/// an operator was told where their run went and given nothing to reach it with. /// -/// A run that stops for a human is persisted and resumable in principle, but -/// this release has no `io resume` to continue it — so the honest thing is to -/// say where it went rather than to imply it is gone. `None` for every outcome -/// that did not pause. +/// `None` for every outcome that did not pause. pub fn parked(outcome: &RunOutcome, run_id: i64) -> Option { - (code(outcome) == PAUSED).then(|| { - format!( - "run {run_id} is parked in the store; answering it and carrying on \ - is not in this release" - ) - }) + let (waiting_on, how) = match outcome { + RunOutcome::AwaitingAnswer { question_id, .. } => ( + format!("question {question_id}"), + format!("io resume {run_id} --answer \"\""), + ), + RunOutcome::AwaitingPlan { plan_id, .. } => ( + format!("plan {plan_id}"), + format!("io resume {run_id} --plan approve"), + ), + RunOutcome::AwaitingRecovery { attempt_id, .. } => ( + format!("call {attempt_id}, whose outcome nobody recorded"), + format!("io resume {run_id} --recovery retry"), + ), + // The one pause with no `io resume` behind it. An approval is answered by + // the person the run asked, at the terminal it asked from, and there is no + // resume entry point that takes one — so this names the request and says + // that rather than offering an invocation that does not exist. + RunOutcome::AwaitingApproval { request_id, .. } => { + return Some(format!( + "run {run_id} is parked on approval {request_id}, which is answered \ + at the terminal that asked for it and not by `io resume`" + )) + } + _ => return None, + }; + Some(format!( + "run {run_id} is parked on {waiting_on}; carry it on with `{how}`" + )) } /// What reaches stdout once the turn is done. @@ -330,10 +377,20 @@ pub async fn turn( provider, store, policy, - // Nothing here can answer a question, so the harness's own documented - // choice for an unattended job is the right one: an ask becomes a + // **This is about approvals and nothing else, and until 0.23.0 it + // also spoke for questions, which it had no business doing.** An + // approver decides an *ask* raised by the policy, and nothing in an + // unattended run can answer one — so the harness's own documented + // choice for a headless job is the right one: the ask becomes a // refusal the agent is told about and adapts to, exactly as a policy - // refusal already does. An approver that blocks would hang forever. + // refusal already does, and the run carries on. An approver that + // blocked instead would hang forever. + // + // A question the agent asks the operator never reaches an approver at + // all. It ends the run at `AwaitingAnswer` with the question written + // to the store, which is a pause and not a refusal, and nothing here + // adapts to anything — see `parked` above, which is the line that + // names it and now names the way back in. &DenyAll, observer, ) @@ -386,6 +443,39 @@ pub fn widening(sandbox: Option) -> Option<&'static str> { .then_some("--sandbox full-access: commands in this run are not confined to the workspace") } +/// The provider a headless entry point runs on: the flag's, or the file's. +/// +/// `--provider` wins over the file, and is the only path that works when there is +/// no file at all — which is the CI case, and the case where an interactive `io` +/// would open the wizard nobody can answer. Shared by [`main`] and +/// [`resume_main`] rather than written twice, because the sentence a run with no +/// provider gets is the one thing both of them are certain to hit on a machine +/// that has never run `io setup`. +pub fn spec_for( + which: Option, + config: &Config, + model_override: Option<&str>, +) -> Result { + match (which, config.provider_spec().cloned()) { + (Some(which), _) => { + let (key_var, model_var) = which.vars(); + provider::spec_from( + which, + std::env::var(key_var).ok(), + model_override + .map(str::to_string) + .or_else(|| std::env::var(model_var).ok()), + ) + } + (None, Some(spec)) => Ok(spec), + (None, None) => Err( + "no provider is configured; run `io setup`, or pass `--provider` with \ + its credential in the environment" + .into(), + ), + } +} + /// `io exec`, from the command line to an exit status. pub async fn main( args: crate::cli::Exec, @@ -397,29 +487,7 @@ pub async fn main( // a refused posture costs nothing and leaves no run behind. let posture = args.policy.map(posture_for).transpose()?; - // `--provider` wins over the file, and is the only path that works when - // there is no file at all — which is the CI case, and the case where an - // interactive `io` would open the wizard nobody can answer. - let spec = match (args.provider, config.provider_spec().cloned()) { - (Some(which), _) => { - let (key_var, model_var) = which.vars(); - provider::spec_from( - which, - std::env::var(key_var).ok(), - model_override - .clone() - .or_else(|| std::env::var(model_var).ok()), - )? - } - (None, Some(spec)) => spec, - (None, None) => { - return Err( - "no provider is configured; run `io setup`, or pass `--provider` with \ - its credential in the environment" - .into(), - ) - } - }; + let spec = spec_for(args.provider, &config, model_override.as_deref())?; let store = settings::store_path().ok_or("no place to keep the run store")?; let store = Store::open(&store).map_err(|error| error.to_string())?; let session = Session::open(&store, &root).map_err(|error| error.to_string())?; @@ -523,3 +591,524 @@ impl WithProvider for Headless { Ok(code(&result.outcome)) } } + +/// One row of `io resume --list`, or `None` for a run nothing is waiting on. +/// +/// `json` splits the same two streams `io exec --json` splits: an object per +/// line on stdout, everything else on stderr, so `io resume --list --json | jq` +/// needs no filtering. +/// +/// **`step` means the same thing in all four shapes** — the last step that +/// committed. A question, a plan and an interrupted call each record the step +/// they stopped on, which is committed, and a run whose process went away +/// records the last one it got through; a resume of any of them starts at that +/// number plus one. +/// +/// An interrupted turn and an ended one are not rows here. Neither can be +/// carried on, so listing them under a heading that means "waiting for you" +/// would offer work that does not exist — [`decision_for`] is where an operator +/// who names one by hand is told why. +pub fn listed(run_id: i64, pending: &Pending, json: bool) -> Option { + let (waiting_on, id, step) = match pending { + Pending::Question { + question_id, step, .. + } => ("question", Some(*question_id), *step), + Pending::Plan { plan_id, step, .. } => ("plan", Some(*plan_id), *step), + Pending::Recovery { + attempt_id, step, .. + } => ("recovery", Some(*attempt_id), *step), + Pending::Died { last_step } => ("died", None, *last_step), + Pending::Interrupted | Pending::Finished => return None, + }; + Some(if json { + // Built through `serde_json` rather than formatted, so an answer holding + // a quote is escaped by the same code that escapes the event stream. + serde_json::json!({ + "run_id": run_id, + "waiting_on": waiting_on, + "id": id, + "step": step, + }) + .to_string() + } else { + match id { + Some(id) => format!("run {run_id} {waiting_on} {id} step {step}"), + None => format!("run {run_id} {waiting_on} step {step}"), + } + }) +} + +/// What the operator's flags mean, once checked against what the run is actually +/// waiting on, and carrying the id that addresses that pause. +/// +/// The id travels with the decision rather than beside it, for the reason +/// [`crate::resume::Pending`] keeps its four ids under four names: a question id +/// handed to the plan driver is one operator's answer delivered into somebody +/// else's run, and a shape in which that cannot be typed is worth more than a +/// check that it was not. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Decision { + /// Answer the question the run asked. + Answer { + /// The row the answer is written against. + question_id: i64, + /// What the operator said. + answer: String, + }, + /// Decide the plan the run proposed. + Plan { + /// The row the verdict is written against. + plan_id: i64, + /// The verdict. + verdict: PlanVerdict, + }, + /// Say what happened to the call the run stopped in the middle of. + Recovery { + /// The journal row the account is filed against. + attempt_id: i64, + /// What the operator established. + decision: RecoveryDecision, + }, + /// Nothing to decide: the process went away and the run simply has committed + /// work and no ending. + CarryOn, +} + +/// The plan verdict `--plan` and `--correction` name together. +/// +/// A correction is refused on the two verdicts that cannot carry one, rather than +/// dropped: `--plan approve --correction "…"` is somebody who meant `revise`, and +/// running the plan they were trying to change is the worst reading of it. +pub fn verdict_for(flag: PlanFlag, correction: Option<&str>) -> Result { + match (flag, correction) { + (PlanFlag::Revise, Some(correction)) if !correction.trim().is_empty() => { + Ok(PlanVerdict::revise(correction)) + } + (PlanFlag::Revise, _) => Err( + "`--plan revise` sends the plan back to be changed, so it needs to say what to \ + change: add `--correction \"\"`." + .into(), + ), + (_, Some(_)) => Err( + "`--correction` says what a plan should do differently, and only `--plan revise` \ + asks for a different plan. Use `--plan revise --correction \"…\"`, or drop it." + .into(), + ), + (PlanFlag::Approve, None) => Ok(PlanVerdict::Approve), + (PlanFlag::Cancel, None) => Ok(PlanVerdict::Cancel), + } +} + +/// The recovery decision `--recovery` and `--account` name together. +/// +/// `completed` is the one that needs words. io-harness files the operator's +/// account against the step the *call* was made on rather than the step the run +/// has now reached, so the resumed run reads a transcript in which the tool +/// answered where it was asked. Nothing validates it — this is an assertion about +/// the outside world — which is exactly why an empty one is refused. +pub fn recovery_for(flag: RecoveryFlag, account: Option<&str>) -> Result { + match (flag, account) { + (RecoveryFlag::Completed, Some(account)) if !account.trim().is_empty() => { + Ok(RecoveryDecision::Completed { + observation: account.to_string(), + }) + } + (RecoveryFlag::Completed, _) => Err( + "`--recovery completed` tells the agent the call landed, so it needs to say what \ + the call returned: add `--account \"\"`." + .into(), + ), + (_, Some(_)) => Err( + "`--account` is what a call returned, and only `--recovery completed` says a call \ + returned anything. Use `--recovery completed --account \"…\"`, or drop it." + .into(), + ), + (RecoveryFlag::Retry, None) => Ok(RecoveryDecision::Retry), + (RecoveryFlag::Abandon, None) => Ok(RecoveryDecision::Abort), + } +} + +/// What this run is waiting on, and the invocation that decides it. +/// +/// One sentence-maker for two questions an operator asks in the same breath — +/// "what does this run want" and "then what do I type" — so a missing flag and a +/// flag for the wrong pause are answered identically rather than by two sentences +/// that could drift apart. +fn waiting_on(run_id: i64, pending: &Pending) -> String { + match pending { + Pending::Question { question_id, .. } => format!( + "run {run_id} is waiting on question {question_id}; answer it with \ + `io resume {run_id} --answer \"\"`" + ), + Pending::Plan { plan_id, .. } => format!( + "run {run_id} is waiting on plan {plan_id}; decide it with \ + `io resume {run_id} --plan approve`" + ), + Pending::Recovery { + attempt_id, tool, .. + } => format!( + "run {run_id} stopped on call {attempt_id} to `{tool}`, whose outcome nobody \ + recorded; say what happened with `io resume {run_id} --recovery retry`" + ), + Pending::Died { last_step } => format!( + "run {run_id}'s process went away after step {last_step} and there is nothing to \ + decide; carry it on with `io resume {run_id}`" + ), + Pending::Interrupted | Pending::Finished => { + format!("there is nothing waiting on run {run_id}") + } + } +} + +/// The decision to drive, or the sentence that refuses to drive one. +/// +/// **Two refusals here are the point rather than a side effect.** +/// +/// A run the operator interrupted is *finished*, not paused: `Ctrl+C` returns +/// `Flow::Cancel`, the loop records the outcome `cancelled`, and every resume +/// entry point short-circuits on a completed run and hands back the original +/// outcome having driven nothing. So it is refused before a provider is built, +/// in the same words the interactive surface uses — which point at `/fork` from +/// the turn before, the honest neighbouring answer. +/// +/// And a flag for a pause the run is not on is refused rather than ignored. clap +/// cannot see which pause a run is waiting on; only the store can, and +/// `--plan approve` typed at a run holding a question is an operator acting on +/// the wrong thing. +pub fn decision_for( + run_id: i64, + pending: &Pending, + args: &crate::cli::Resume, +) -> Result { + let wanted = match pending { + Pending::Question { .. } => "--answer", + Pending::Plan { .. } => "--plan", + Pending::Recovery { .. } => "--recovery", + Pending::Died { .. } => "", + // Both sentences are `crate::resume::Failure`'s own, so the one an + // operator gets from `io resume` is the one they get from `/resume`. + Pending::Interrupted => { + return Err(crate::resume::Failure::Interrupted { run_id }.to_string()) + } + Pending::Finished => return Err(crate::resume::Failure::Ended { run_id }.to_string()), + }; + for (flag, given) in [ + ("--answer", args.answer.is_some()), + ("--plan", args.plan.is_some()), + ("--recovery", args.recovery.is_some()), + ] { + if given && flag != wanted { + return Err(format!( + "`{flag}` decides something run {run_id} is not waiting on. {}", + waiting_on(run_id, pending), + )); + } + } + + match pending { + Pending::Question { question_id, .. } => { + let answer = args + .answer + .clone() + .ok_or_else(|| waiting_on(run_id, pending))?; + Ok(Decision::Answer { + question_id: *question_id, + answer, + }) + } + Pending::Plan { plan_id, .. } => { + let flag = args.plan.ok_or_else(|| waiting_on(run_id, pending))?; + Ok(Decision::Plan { + plan_id: *plan_id, + verdict: verdict_for(flag, args.correction.as_deref())?, + }) + } + Pending::Recovery { attempt_id, .. } => { + let flag = args.recovery.ok_or_else(|| waiting_on(run_id, pending))?; + Ok(Decision::Recovery { + attempt_id: *attempt_id, + decision: recovery_for(flag, args.account.as_deref())?, + }) + } + Pending::Died { .. } => Ok(Decision::CarryOn), + // Both returned above; repeated here because the compiler asks and an + // `unreachable!()` in a function an operator's typing reaches is a panic + // waiting for the next variant. + Pending::Interrupted | Pending::Finished => { + Err(crate::resume::Failure::Ended { run_id }.to_string()) + } + } +} + +/// The goal a resumed run carries on under, or the sentence that refuses to +/// invent one. +/// +/// **A bare run has no recoverable goal, and running it against an empty one is +/// the failure this refuses.** `runs.goal` has no public reader, so +/// `crate::resume::goal_for` recovers the operator's own words from the session +/// turn a run served and answers `None` for a run that served none — which is +/// every run `io exec` starts. A contract built from `None` would hand the agent +/// a task nobody set and spend a budget on it, so the goal is asked for instead. +/// +/// `--goal` beats a recovered one when both are there. That is the operator +/// re-aiming their own run, which is theirs to do, and a flag silently ignored +/// because the store happened to hold something is worse than one that acts. +pub fn goal_or_refusal( + run_id: i64, + recovered: Option, + supplied: Option<&str>, +) -> Result { + if let Some(goal) = supplied.filter(|goal| !goal.trim().is_empty()) { + return Ok(goal.to_string()); + } + recovered.ok_or_else(|| { + format!( + "run {run_id} served no session turn, so what it was asked to do cannot be read \ + back — `runs.goal` has no public reader and only a turn keeps the operator's own \ + words. Carrying it on against an empty goal would set the agent a task nobody \ + asked for: say what it was for with \ + `io resume {run_id} --goal \"\"`." + ) + }) +} + +/// `io resume`, from the command line to an exit status. +/// +/// The listing happens before a provider is chosen and before a credential is +/// read, so `io resume --list` works on a machine that has never run `io setup` +/// and costs nothing. +pub async fn resume_main( + args: crate::cli::Resume, + config: Config, + root: std::path::PathBuf, + model_override: Option, +) -> Result { + // Refused first, as `main` refuses it, so a posture nothing can honour costs + // no store, no session and no provider. + let posture = args.policy.map(posture_for).transpose()?; + + let path = settings::store_path().ok_or("no place to keep the run store")?; + let store = Store::open(&path).map_err(|error| error.to_string())?; + + if args.list { + // One classification per run, each a handful of store reads and no + // provider call. That is linear in the store's whole history rather than + // in the parked runs, which is the right cost while a store holds + // hundreds; the way past it is a store-side query for the pending rows, + // which io-harness does not publish. + let mut out = std::io::stdout().lock(); + for run_id in store.runs().map_err(|error| error.to_string())? { + let pending = + crate::resume::pending_for(&store, run_id).map_err(|error| error.to_string())?; + if let Some(row) = listed(run_id, &pending, args.json) { + let _ = writeln!(out, "{row}"); + } + } + let _ = out.flush(); + return Ok(OK); + } + + // clap guarantees this through `required_unless_present`, and the sentence is + // here rather than an `expect` because a parser's guarantee is not a reason to + // panic in front of an operator if it ever stops holding. + let run_id = args + .run + .ok_or("`io resume` needs a run id, or `--list` to see which runs have one")?; + + // Everything that can refuse, before anything is built: the classification is + // a few store reads, and an operator who names the one run that cannot be + // carried on pays nothing to find out. + let pending = crate::resume::pending_for(&store, run_id).map_err(|error| error.to_string())?; + let decision = decision_for(run_id, &pending, &args)?; + let goal = goal_or_refusal( + run_id, + crate::resume::goal_for(&store, run_id).map_err(|error| error.to_string())?, + args.goal.as_deref(), + )?; + + let spec = spec_for(args.provider, &config, model_override.as_deref())?; + let policy = policy_for(&config, posture); + + provider::build( + spec, + model_override, + Resuming { + store, + config, + policy, + // `None` unless the operator named a posture, which is what lets + // `carry_on` use the boundary the run itself recorded rather than one + // chosen for it now — the only driver of the four that can. + chosen: posture.is_some(), + root, + run_id, + goal, + decision, + json: args.json, + }, + ) + .await? +} + +/// The resumed run, as something [`provider::build`] can run. +struct Resuming { + store: Store, + config: Config, + policy: Policy, + chosen: bool, + root: std::path::PathBuf, + run_id: i64, + goal: String, + decision: Decision, + json: bool, +} + +impl WithProvider for Resuming { + type Out = Result; + + async fn call( + self, + make: impl Fn(&str) -> Result, + model: String, + ) -> Self::Out { + let provider = make(&model)?; + + if let Some(line) = asks_nobody_can_answer(&self.policy) { + eprintln!("io: {line}"); + } + + let json = Ndjson::new(std::io::stdout()); + let observer: &dyn Observer = if self.json { &json } else { &Ignore }; + + // The same composition `Headless` builds, and for the same reason: a + // `[[hook]]` that fired on the run and then went quiet the moment it was + // carried on would leave an audit log with a hole in exactly the half + // nobody watched happen. + let hooks = crate::contract::hooks(&self.config, &self.root); + let mut observers: Vec<&dyn Observer> = vec![observer]; + if let Some(hooks) = &hooks { + observers.push(hooks); + } + let fanout = crate::fanout::Fanout::new(observers); + let durable = settings::store_path().and_then(|path| Store::open(&path).ok()); + let broadcast = durable.map(|store| io_harness::Broadcast::new(store, &fanout)); + let watcher: &dyn Observer = match &broadcast { + Some(broadcast) => broadcast, + None => &fanout, + }; + + // Read *before* anything is driven. `crate::resume` closes the turn with a + // compare-and-swap against this value, so a head taken afterwards would be + // the head this resume is about to move and the swap would never refuse — + // which is the lost update the swap exists to catch. + let head = self + .store + .turn_for_run(self.run_id) + .ok() + .flatten() + .and_then(|turn_id| self.store.session_turn(turn_id).ok().flatten()) + .and_then(|turn| self.store.session_head(turn.session_id).ok().flatten()); + + let contract = crate::contract::configured(self.goal, self.root.clone(), &self.config); + // `None` on every arm: a containment is a fleet's shared budget, this + // subcommand takes no flag that expresses one, and `crate::resume::recover` + // refuses a contained run outright because io-harness 0.69 publishes no + // tree-aware recovery entry point to keep those limits with. + let resumed = match self.decision { + Decision::Answer { + question_id, + answer, + } => { + crate::resume::answer_question( + &contract, + &provider, + &self.store, + self.run_id, + question_id, + &answer, + &self.policy, + &DenyAll, + None, + watcher, + head, + ) + .await + } + Decision::Plan { plan_id, verdict } => { + crate::resume::decide_plan( + &contract, + &provider, + &self.store, + self.run_id, + plan_id, + verdict, + &self.policy, + &DenyAll, + None, + watcher, + head, + ) + .await + } + Decision::Recovery { + attempt_id, + decision, + } => { + crate::resume::recover( + &contract, + &provider, + &self.store, + self.run_id, + attempt_id, + decision, + &self.policy, + &DenyAll, + None, + watcher, + head, + ) + .await + } + Decision::CarryOn => { + crate::resume::carry_on( + &contract, + &provider, + &self.store, + self.run_id, + self.chosen.then_some(&self.policy), + &DenyAll, + None, + watcher, + head, + ) + .await + } + } + .map_err(|failure| failure.to_string())?; + + // stdout is the data and stderr is everything else, the split `io exec` + // already makes. + if let Some(reply) = to_stdout(self.json, resumed.reply.as_deref()) { + let mut out = std::io::stdout().lock(); + let _ = writeln!(out, "{reply}"); + let _ = out.flush(); + } + // **`+ 1`, and the interactive arm has always had it.** `resumed_after` + // is the last step that had *committed* before anything was driven, so + // the step this resume carried on from is the one after it. Printed bare, + // this told the operator it resumed at 12 when it resumed at 13, and + // disagreed with what the session said about the same run. + eprintln!( + "io: carried run {} on from step {}", + resumed.run_id, + resumed.resumed_after + 1 + ); + eprintln!("io: {}", describe(&resumed.outcome)); + // A resumed run can pause again, on a second question or on a plan the + // agent proposes next, and the line that says so names the new handle. + if let Some(parked) = parked(&resumed.outcome, resumed.run_id) { + eprintln!("io: {parked}"); + } + Ok(code(&resumed.outcome)) + } +} diff --git a/src/failure.rs b/src/failure.rs index 5442752..352f532 100644 --- a/src/failure.rs +++ b/src/failure.rs @@ -26,20 +26,68 @@ use io_harness::Error; /// /// One `String`, because the caller commits one line — see `App::say`. The /// leading sentence is this crate's; everything after the dash is io-harness's. +/// +/// **One failure has its harness line dropped rather than kept underneath**, and +/// it is the only one. [`Error::Conflict`]'s own `Display` reads `run {run_id} is +/// held by another owner until {expires_at}`, which is written for the run-lease +/// shape. On the session-head shape `run_id` holds a *session* id under the word +/// "run" and `expires_at` is empty, so the line names the wrong noun and ends on +/// the word "until". This module's rule is that terse text is worth keeping in +/// front of the operator; it was never a rule about text that is wrong. pub fn said(error: &Error) -> String { match advice(error) { + Some(advice) if head_conflict(error) => advice, Some(advice) => format!("{advice}\n{error}"), None => error.to_string(), } } +/// Whether a conflict is the session-head shape rather than the run-lease one. +/// +/// **The two shapes are not fully separable from the value, so this errs towards +/// the head.** Every head conflict has an empty `owner` — `set_session_head_if` +/// writes it so, because a head has a value that moved rather than a holder — but +/// a *lease* conflict is built with an empty `owner` too when the lease row has +/// already gone by the time the refusal is described (`conflict_from`, in +/// io-harness's `state.rs`). An empty `owner` therefore means "this value knows of +/// no holder and no expiry", which is exactly the case where the lease sentence +/// cannot be said honestly either: it would name a holder that is not there and an +/// expiry that is empty. The head wording is the least wrong answer for both, and +/// neither of them calls the id a run. +fn head_conflict(error: &Error) -> bool { + matches!(error, Error::Conflict { owner, .. } if owner.is_empty()) +} + /// The sentence, if this release recognises the failure. /// /// Matched on the *text* rather than on a status code, and that is deliberate: /// the same condition arrives as a 404 from one gateway, a 400 from another and /// a 422 from a third, while the sentence they write is stable enough to match /// on. A miss is silent — the harness's own line is already correct, only terse. -pub fn advice(error: &Error) -> Option<&'static str> { +/// +/// [`Error::Conflict`] is the exception and is matched on the **value**, before +/// any text is looked at, because on that variant the text is the defect rather +/// than the evidence — see [`said`]. It is also why this returns an owned +/// `String`: a lost head race is worth naming the session it was lost on, and a +/// `&'static str` cannot. +pub fn advice(error: &Error) -> Option { + if let Error::Conflict { run_id, owner, .. } = error { + return Some(if owner.is_empty() { + // `run_id` is the session id here, and is called one. No expiry is + // rendered: a head does not have one, and the field is empty. + format!( + "another `io` moved session {run_id} on first. This was refused rather \ + than written over that process's turn — nothing retried and nothing \ + forced it. `/resume` re-reads the conversation as it now stands." + ) + } else { + // The shape the harness's own line was written for, so it says the + // owner and the expiry underneath and this only says what to do. + "another process is already running this. Wait for it to finish or for its \ + lease to lapse, then ask again." + .to_string() + }); + } let said = error.to_string().to_lowercase(); if said.contains("image input") || said.contains("does not support image") @@ -48,25 +96,28 @@ pub fn advice(error: &Error) -> Option<&'static str> { { return Some( "this model cannot look at pictures. Pick one that can with /model, or \ - ask the question without the attachment — the image was not sent.", + ask the question without the attachment — the image was not sent." + .to_string(), ); } if said.contains("insufficient") && (said.contains("credit") || said.contains("quota")) { - return Some("the account is out of credit. Nothing was spent on this turn."); + return Some("the account is out of credit. Nothing was spent on this turn.".to_string()); } if said.contains("rate limit") || said.contains("429") { - return Some("the provider is rate-limiting this key. Wait and ask again."); + return Some("the provider is rate-limiting this key. Wait and ask again.".to_string()); } if said.contains("no auth") || said.contains("unauthorized") || said.contains("401") { return Some( "the provider rejected the credential. `/provider` runs the setup again, \ - and the key it writes lives in io-harness's configuration file.", + and the key it writes lives in io-harness's configuration file." + .to_string(), ); } if said.contains("no endpoints found") || said.contains("no allowed providers") { return Some( "no provider behind this gateway will serve that model as asked. Pick a \ - different model with /model.", + different model with /model." + .to_string(), ); } if said.contains("context length") @@ -75,7 +126,8 @@ pub fn advice(error: &Error) -> Option<&'static str> { { return Some( "the conversation is longer than this model will take. `/clear` starts a \ - new one, and `/resume` still has this one.", + new one, and `/resume` still has this one." + .to_string(), ); } None diff --git a/src/intent.rs b/src/intent.rs index 516c450..a512a7f 100644 --- a/src/intent.rs +++ b/src/intent.rs @@ -12,8 +12,18 @@ //! question and pauses the run, resumable, which is what an operator who does not //! know the answer needs — and it is what a session with no responder at all does //! today, so `Esc` here leaves the run exactly where 0.9.0 would have left it. +//! +//! # Two ways in, one overlay (0.23.0) +//! +//! A question can also be met long after the run that asked it stopped, as a +//! `pending_questions` row read back off the store. [`Intent::resumed`] opens on +//! one, and from that point every keystroke, every drawn line and `Esc` itself +//! are the same code as the live path. The one difference is where the answer +//! goes once it is given: a live turn is awaiting it on a channel, a stored row +//! has nobody awaiting anything, so [`Intent::resolve`] hands it back to the +//! caller to deliver with `resume_with_answer_observed`. -use io_harness::{AnswerFuture, Question, Responder}; +use io_harness::{AnswerFuture, PendingQuestion, Question, Responder}; use ratatui::layout::Rect; use ratatui::text::Line; use ratatui::widgets::Paragraph; @@ -64,9 +74,55 @@ impl Responder for Answerer { } } -/// The overlay a question is asked through. +/// Where a decision goes once it is made. +/// +/// **The two paths resolve differently and the type says so.** A live turn is +/// parked on a `oneshot` and is released by a send; a stored row is a run that +/// already ended, with no receiver anywhere and nothing to release — its answer +/// has to travel back out to the caller, which delivers it through +/// `io_harness::resume_with_answer_observed`. Faking a `oneshot` for that second +/// case would compile and would be a lie: in the live path a channel whose +/// receiver is gone is precisely the shape that means *nobody can answer, pause +/// the run*, so a stored answer sent down one would be indistinguishable from an +/// abandoned question. +/// +/// Generic, and shared with [`crate::plan`], because a verdict has the identical +/// pair of destinations. A second copy of this enum is a second place the stored +/// path could quietly grow a channel nobody is holding. +#[derive(Debug)] +pub(crate) enum Destination { + /// A live turn is awaiting the value on the channel it handed over. + Turn(oneshot::Sender), + /// A run that already stopped. Nothing is awaiting; the value is returned. + Stored, +} + +impl Destination { + /// Deliver, and hand back whatever the caller must now deliver itself. + /// + /// `None` means it is already delivered — the turn had it the moment this + /// returned. `Some(value)` means this overlay was opened on a stored row and + /// the value has gone nowhere yet. + pub(crate) fn deliver(self, value: T) -> Option { + match self { + Self::Turn(sender) => { + let _ = sender.send(value); + None + } + Self::Stored => Some(value), + } + } + + /// Whether the run behind this overlay has already stopped. + pub(crate) fn parked(&self) -> bool { + matches!(self, Self::Stored) + } +} + +/// The overlay a question is asked through, live or resumed. pub struct Intent { - asked: Asked, + question: Question, + answer: Destination>, composer: Composer, } @@ -75,14 +131,33 @@ impl Intent { /// one of the agent's own choices is an answer the operator did not give. pub fn new(asked: Asked) -> Self { Self { - asked, + question: asked.question, + answer: Destination::Turn(asked.answer), + composer: Composer::new(), + } + } + + /// Open on a question a run already paused on, read back off the store. + /// + /// The row carries the answer and who gave it as well; neither is a question, + /// so neither reaches the overlay — an answered row is not something to ask + /// again, and refusing to open on one is the caller's job, which is the only + /// place that knows whether it is resuming or replaying. + pub fn resumed(pending: &PendingQuestion) -> Self { + Self { + question: Question { + question: pending.question.clone(), + context: pending.context.clone(), + choices: pending.choices.clone(), + }, + answer: Destination::Stored, composer: Composer::new(), } } /// The question on screen, for whatever has to show or assert it. pub fn question(&self) -> &Question { - &self.asked.question + &self.question } /// A keystroke while the overlay is up. @@ -101,32 +176,51 @@ impl Intent { } } - /// Send the answer back to the run. Consumes the overlay, because a question - /// answered twice is a run that receives an answer nobody typed. - pub fn resolve(self, answer: Option) { - let _ = self.asked.answer.send(answer); + /// Resolve the question. Consumes the overlay, because a question answered + /// twice is a run that receives an answer nobody typed. + /// + /// Returns `None` when the answer has been delivered — a live turn was + /// awaiting it and now has it. Returns `Some(answer)` when this overlay was + /// opened by [`Self::resumed`]: there was no turn to send to, so the answer + /// comes back out here and the caller delivers it with + /// `io_harness::resume_with_answer_observed`. Dropping that value drops the + /// operator's answer, which is why it is a return rather than a side effect. + pub fn resolve(self, answer: Option) -> Option> { + self.answer.deliver(answer) } /// The question, its context, the choices offered, and the prompt. /// /// The whole viewport, like an approval: the run is stopped, so there is /// nothing behind this worth half a screen. + /// + /// One line of it depends on which way in was taken, and it is the only one: + /// declining a live question defers it *within* a turn that is still running + /// and will carry on the moment it is answered, while declining a resumed one + /// leaves the run parked exactly as it was found — the operator opened it, so + /// "for later" would be a promise nothing behind the screen is keeping. That + /// difference is a word chosen here, not a second `render`. pub fn render(&self, frame: &mut Frame, area: Rect, theme: &Theme) { if area.height == 0 { return; } let mut lines: Vec> = Vec::new(); - lines.push(theme.notice(Tone::Warning, self.asked.question.question.clone())); - if let Some(context) = &self.asked.question.context { + lines.push(theme.notice(Tone::Warning, self.question.question.clone())); + if let Some(context) = &self.question.context { lines.push(theme.notice(Tone::Muted, context.clone())); } - for choice in &self.asked.question.choices { + for choice in &self.question.choices { lines.push(theme.notice(Tone::Muted, format!("{} {choice}", theme.glyphs.bullet))); } + let leaves = if self.answer.parked() { + "Esc leaves the run parked" + } else { + "Esc leaves it for later" + }; lines.push(theme.notice( Tone::Muted, format!( - "type an answer {} Enter sends it {} Esc leaves it for later", + "type an answer {} Enter sends it {} {leaves}", theme.glyphs.dash, theme.glyphs.dash ), )); diff --git a/src/lib.rs b/src/lib.rs index 7aa00d8..380e077 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -56,6 +56,7 @@ pub mod home; pub mod import; pub mod intent; pub mod keys; +pub mod lock; pub mod markdown; pub mod memory; pub mod page; @@ -69,6 +70,7 @@ pub mod providers; pub mod queue; pub mod recall; pub mod reload; +pub mod resume; pub mod rewind; pub mod servers; pub mod sessions; diff --git a/src/lock.rs b/src/lock.rs new file mode 100644 index 0000000..d8a55d8 --- /dev/null +++ b/src/lock.rs @@ -0,0 +1,465 @@ +//! One `io` at a time on one session, and a sentence when there are two. +//! +//! **The lock is keyed on the session id, because a conversation is the only +//! thing two processes can collide over.** `Session::open` creates a new row +//! every time it is called, so two terminals started in one repository are two +//! separate conversations that share a directory name and nothing else; a lock +//! keyed on the workspace would refuse the second one for a conflict that does +//! not exist. The real collision is narrower and arrives from one place: +//! `/resume` and `io resume` take an id that already exists, and two processes +//! holding one id advance one head. Through 0.22.0 nothing guarded that — both +//! opened the same `runs.db`, both advanced the same head, and the loser of the +//! race paid for a completed turn and was shown `run 7 is held by another owner +//! until`, in which 7 is a *session* id, "run" is the wrong noun, and the expiry +//! is empty, because a head conflict populates neither field. +//! +//! **So the acquisition at startup is not a contention at all — it is a +//! publication.** A freshly opened session is unique to the process that opened +//! it, and taking its lock always succeeds. What the acquisition buys is the +//! owner record written beside it, so that a later process asked to enter *that* +//! conversation can be refused and can name who holds it. +//! +//! **What this does not cover, said plainly.** Two `io` in one workspace on two +//! different sessions are not in conflict and are not refused. Nothing here sees +//! a process that reaches the store without coming through this module either. +//! The head compare-and-swap in the harness stays the guard of last resort for +//! everything the lock misses; this module exists so the ordinary case is +//! answered with a sentence instead of a paid-for turn lost to a race. +//! +//! The guard is an advisory whole-file lock and nothing else. +//! [`std::fs::File::try_lock`] is stable on this crate's MSRV; it is `flock` on +//! unix and `LockFileEx` on Windows, and the kernel releases it on exit, on +//! panic and on `kill -9`. So on a local filesystem there is no such thing as a +//! stale lock to reap, and none of the usual pid-file machinery — a liveness +//! probe, a timestamp, a staleness threshold — is needed for the ordinary case. +//! +//! Both of those are held per open file description or per handle rather than +//! per process, which is what makes two `io` on one session contend and what +//! lets `tests/lock.rs` prove it without a second process. Solaris is the one +//! target where the standard library reaches for `fcntl(F_SETLK)` instead, whose +//! record locks are per process; io-cli ships no Solaris artifact, and the day +//! it does this module needs reading again rather than trusting. +//! +//! **Naming the holder cannot mean naming the operating system's process.** +//! `tests/dependencies.rs` asserts the direct dependency set in both directions +//! and forbids `nix` outright, and a process spawn is permitted only in +//! `crate::shell`, which may not name a `Store` and is callable only from the +//! driver. (Spelled that way round deliberately: that gate sweeps the raw text +//! of every source file for the type's full name, comments included, so a module +//! that wrote it out to say it was *not* spawning would be listed as a module +//! that spawns.) So there is no `kill(pid, 0)`, no `ps` and no `tasklist`, and +//! `/proc` is one platform's answer to a three-platform question. What this +//! module can state truthfully is what it itself wrote beside the lock: the +//! pid, the workspace root, the `io` version, and the instant the holder +//! started. That record is a separate file, because a Windows byte-range lock +//! is mandatory and a refused reader could not open the locked file to read it. +//! +//! **The record carries no host, and on a shared home that matters.** The pid is +//! compared against this process's own to recognise a lock this process already +//! holds, which is sound wherever the home is local: a live holder on this +//! machine cannot have our pid. With `~/.io-cli` on a network filesystem shared +//! between machines, two hosts can carry the same pid, and a genuine second +//! process would then be admitted to a session another `io` is holding. That is +//! the same configuration the lease below exists for, and the same one where an +//! advisory lock is not this program's business either. +//! +//! The lease and its lapse exist only for the case the kernel cannot cover — a +//! network filesystem, where an advisory lock is the kernel's business and not +//! this program's. There the owner record's own timestamp is all there is, and +//! the module says so rather than implying a guarantee it does not have. +//! +//! No clock is read here. `tests/timing.rs` permits [`std::time::SystemTime`] +//! readings in the driver and nowhere else, so every instant this module +//! records or ages arrives as an argument — the same seam [`crate::sessions`] +//! documents for its own stamps. +//! +//! **The home is an argument too, and for the same kind of reason.** +//! [`crate::home::path`] answers `None` where the operator has no home +//! directory, and a module that resolved it for itself would have to invent an +//! answer or panic in the one place a lock must not do either. Handing the +//! directory in leaves that `None` where it belongs — with the caller, which is +//! also what makes every function here reachable from a test on a temporary +//! directory rather than only from the driver. + +use std::fs::File; +use std::io; +use std::path::{Path, PathBuf}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; + +/// What both files are named after, so a directory listing sorts them together +/// and an operator can see at a glance that they belong to this product's own +/// bookkeeping rather than to their configuration. +const STEM: &str = "session-"; + +/// The file that is only ever locked, and is never opened for its contents. +/// +/// **It holds no bytes and is never removed.** Unlinking a file another process +/// holds a lock on does not release anything: the next opener creates a fresh +/// inode, locks that, and two processes are then holding two different locks +/// while both believe they are alone. So the file stays, one per session, and +/// the emptiness is the point — a lock file with a payload is a lock file +/// somebody eventually parses, and on Windows the holder's own mandatory lock +/// would stop them. +const LOCK: &str = "lock"; + +/// The plain-text record beside the lock, saying who holds it. +/// +/// **A second file rather than the lock's own contents**, because a Windows +/// byte-range lock is mandatory: the process that is being refused could not +/// read the locked file at all, and a refusal that cannot say anything about the +/// holder is the sentence this release exists to replace. It is never locked, so +/// a reader always gets *something* — possibly a half-written line, which +/// [`Owner::parse`] is written to survive. +const OWNER: &str = "owner"; + +/// How long a lease runs before [`Owner::lapsed`] will call it lapsed. +/// +/// **This is a fallback for one case and a weaker guarantee than the lock.** On +/// a local filesystem the kernel releases the lock on exit, on panic and on +/// `kill -9`, so nothing here is ever consulted. On a network filesystem an +/// advisory lock is the kernel's business and not this program's, and the +/// record's own timestamp is the only evidence there is. +/// +/// Twelve hours because the instant recorded is when the holder *started*, not +/// when it last did anything: a session left open across a working day is doing +/// nothing wrong, and a threshold that reads it as abandoned would offer to +/// take over a live conversation. The caller still confirms with the operator — +/// this constant only decides when the question is worth asking. +/// +// ponytail: the record is written once at acquisition. If a tighter lease is +// ever wanted, rewrite it per turn rather than shortening this number. +pub const LEASE: Duration = Duration::from_secs(12 * 60 * 60); + +/// The lock file and the owner record for one session, in that order. +/// +/// **Named after the session id, and there is nothing to hash.** A +/// `Session::id()` is an `i64` the store issued, already unique and already +/// stable across processes and across toolchains — which is what a name two +/// separately-built `io` binaries have to agree on needs to be, and what the +/// workspace root was never able to be without inventing a hash to stand in for +/// it. The id is also the thing both processes know at the moment they can +/// actually contend, because contending means one of them resumed the other's +/// conversation. +#[must_use] +pub fn paths(home: &Path, session: i64) -> (PathBuf, PathBuf) { + ( + home.join(format!("{STEM}{session}.{LOCK}")), + home.join(format!("{STEM}{session}.{OWNER}")), + ) +} + +/// What `io` wrote about itself beside the lock it holds. +/// +/// **Every field is optional, and none of them may be invented.** The record can +/// be missing entirely, half-written, unreadable, or written by a version of +/// this product that recorded a different set of fields — four ordinary +/// outcomes, and a refusal that answered any of them with a guess would be +/// naming a process that does not exist. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct Owner { + /// The holder's process id, as it wrote it. + /// + /// **Never [`std::process::id`] of whoever is reading.** That value is + /// always available and always wrong here, and it is the one substitution + /// that would make a refusal look complete while telling the operator to go + /// and kill themselves. + pub pid: Option, + /// The workspace the holder is working in. + /// + /// **Recorded, not keyed on.** The lock is the session's ([`paths`]); this + /// field is here because "another `io` is working in ``" is the clause + /// that tells an operator with several terminals open which one to go to. + /// + /// Written through `to_string_lossy`, so a root that is not valid UTF-8 + /// arrives with the unrepresentable parts replaced. It is a sentence's + /// worth of identification and never a path anything opens. + pub root: Option, + /// The `io` version that took the lock. + pub version: Option, + /// When the holder started, as it was handed that instant. + /// + /// Whole seconds since the unix epoch. The lease is measured in hours, so + /// anything finer would be precision the decision cannot use. + pub started: Option, +} + +/// A key and its value from one line of the record, or nothing. +fn field(line: &str) -> Option<(&str, &str)> { + let (name, value) = line.split_once('=')?; + let value = value.trim(); + if value.is_empty() { + return None; + } + Some((name.trim(), value)) +} + +impl Owner { + /// This process's own claim, for the record it is about to write. + /// + /// `started` is handed in rather than read: see the module note. + #[must_use] + pub fn claiming(root: &Path, started: SystemTime) -> Self { + Self { + pid: Some(std::process::id()), + root: Some(root.to_path_buf()), + version: Some(env!("CARGO_PKG_VERSION").to_string()), + started: Some(started), + } + } + + /// The record's text: one `name = value` line per field that is known. + /// + /// A field that is `None` is left out rather than written as an empty value, + /// so the file says only what is true and [`Owner::parse`] reads back what + /// this wrote. Plain text and not TOML — `tests/dependencies.rs` permits + /// `toml::from_str` in `src/edit.rs` alone, and a lock record is four + /// scalars, which is not a configuration format's worth of problem. + #[must_use] + pub fn render(&self) -> String { + let mut out = String::new(); + if let Some(pid) = self.pid { + out.push_str(&format!("pid = {pid}\n")); + } + if let Some(root) = &self.root { + out.push_str(&format!("root = {}\n", root.to_string_lossy())); + } + if let Some(version) = &self.version { + out.push_str(&format!("version = {version}\n")); + } + if let Some(seconds) = self + .started + .and_then(|started| started.duration_since(UNIX_EPOCH).ok()) + { + out.push_str(&format!("started = {}\n", seconds.as_secs())); + } + out + } + + /// Read a record back, believing nothing. + /// + /// **Every way this can go wrong ends in a field that is `None`.** A line + /// with no `=` is skipped, which is what a truncated write leaves; a name + /// this version has never heard of is skipped, which is what an older or + /// newer `io` leaves; a value that will not parse leaves its own field alone + /// and no other. Nothing here panics and nothing here fails: a record is + /// evidence, and evidence that cannot be read is an absence rather than an + /// error. + #[must_use] + pub fn parse(text: &str) -> Self { + let mut owner = Self::default(); + for line in text.lines() { + let Some((name, value)) = field(line) else { + continue; + }; + match name { + "pid" => owner.pid = value.parse().ok(), + "root" => owner.root = Some(PathBuf::from(value)), + "version" => owner.version = Some(value.to_string()), + "started" => { + owner.started = value + .parse() + .ok() + .map(|seconds| UNIX_EPOCH + Duration::from_secs(seconds)); + } + _ => {} + } + } + owner + } + + /// Whether the holder's lease has run out, or nothing if it never said when + /// it started. + /// + /// **A fact, not a policy.** `Some(true)` means the record is older than + /// `lease` and nothing more: on a local filesystem the lock itself is still + /// the answer, and even on a network filesystem a lapsed lease is a reason + /// to *ask* the operator whether to take over, never a reason to do it. The + /// caller confirms; this only says whether there is anything to confirm. + /// + /// `None` where the record carries no instant, which is honest about the + /// only two ways that happens — a missing or truncated record, and one + /// written by a version that did not record the field. A missing timestamp + /// read as "old enough" would make every unreadable record a takeover. + /// + /// A record stamped in the future — two machines on one network filesystem + /// disagreeing about the time — is `Some(false)`: clock skew is not + /// evidence of an abandoned session. + #[must_use] + pub fn lapsed(&self, now: SystemTime, lease: Duration) -> Option { + let started = self.started?; + Some(now.duration_since(started).is_ok_and(|age| age > lease)) + } + + /// The refusal, in one line an operator can act on. + /// + /// ASCII throughout: `--plain` and `NO_COLOR` are the terminals this product + /// promises to be readable on, and a sentence that reaches for a dash the + /// font cannot draw is one this crate's own glyph sweep would fail. + /// + /// **It degrades by saying less, never by saying `unknown`.** With nothing + /// in the record it is the bare claim, which is still true and still tells + /// the operator what happened; each field that *is* there adds its own + /// clause and no others. Four `unknown`s in a row would read as a broken + /// product rather than as a missing file. + /// + /// The instant is deliberately not in it. A count of seconds since 1970 is + /// not a fact anybody can act on at a prompt, and its actual use is + /// [`Owner::lapsed`], where the caller has a `now` to age it against. + #[must_use] + pub fn sentence(&self) -> String { + let mut said = String::from("another `io` holds this session"); + if let Some(root) = &self.root { + said.push_str(&format!(" in {}", root.to_string_lossy())); + } + let mut named = Vec::new(); + if let Some(pid) = self.pid { + named.push(format!("pid {pid}")); + } + if let Some(version) = &self.version { + named.push(format!("io {version}")); + } + if !named.is_empty() { + said.push_str(&format!(" ({})", named.join(", "))); + } + said + } +} + +/// The lock, for as long as this value is alive. +/// +/// **The kernel is the guarantee and [`Drop`] is only the tidying.** `flock` and +/// `LockFileEx` are both released when the file handle closes, which happens on +/// a clean exit, on a panic, and on `kill -9` where no destructor runs at all. +/// So this implementation exists to take the owner record away promptly, not to +/// make the lock correct — a `mem::forget` here would leak the record and still +/// free the lock when the process ended. +#[derive(Debug)] +pub struct Guard { + /// Held open because closing it is what releases the lock. + file: File, + /// The record to take away, and whether this guard is the one that wrote it. + owner: PathBuf, + /// **False where the record could not be written**, and the reason `Drop` + /// consults it: removing a file this process did not create would delete + /// somebody else's evidence, and the whole point of the record is that it + /// says who is really there. + wrote: bool, +} + +impl Drop for Guard { + fn drop(&mut self) { + if self.wrote { + let _ = std::fs::remove_file(&self.owner); + } + // **The record goes first and the lock second, and the order is the + // point.** Released the other way round, the next `io` could take the + // lock and write its own record in the window before this one deleted + // the file — and the deletion would then take the new holder's record + // away, leaving a held lock with nothing beside it to name it. + // + // Best effort: an `unlock` that fails changes nothing, because closing + // the handle a line later releases it anyway. + let _ = self.file.unlock(); + } +} + +/// What an attempt to take the lock found. +#[derive(Debug)] +pub enum Taken { + /// It is this process's until the guard is dropped or the process ends. + Held(Guard), + /// Another `io` has it, and this is what that `io` said about itself. + Refused(Owner), +} + +/// Open a file this module owns, readable by its owner alone on unix. +/// +/// The mode is set at CREATE time rather than afterwards, which is the rule +/// [`crate::settings::write`] already follows for the file holding a credential: +/// a mode applied after the fact leaves a window in which the file is on disk +/// with the wrong one. The owner record names a workspace path, and the +/// directory around both is already `0700`. +fn open(path: &Path, truncate: bool) -> io::Result { + let mut options = std::fs::OpenOptions::new(); + options + .read(true) + .write(true) + .create(true) + .truncate(truncate); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + options.open(path) +} + +/// Take the lock for one session, or say who has it. +/// +/// `session` is what the lock is named after; `root` is only written into the +/// record, for the refusal to name. `started` is the instant to record, handed +/// in by the driver — the only thing in this crate permitted to read a clock. +/// +/// **On a session this process just opened it always succeeds**, because that id +/// has never existed anywhere else. The call is worth making anyway: succeeding +/// is what publishes the owner record, and the record is the whole of what a +/// later `io resume` of this id gets to say when it is refused. +/// +/// **`Err` means the attempt could not be made, and never that the lock is +/// held.** [`std::fs::File::try_lock`] answers with +/// [`std::fs::TryLockError::WouldBlock`] for a lock somebody else holds and with +/// [`std::fs::TryLockError::Error`] for an I/O failure, and the standard library +/// guarantees it does not put a `WouldBlock` error inside the second. Collapsing +/// the two either way is the defect this function is written around: an I/O +/// error reported as "held" locks an operator out of their own repository with a +/// sentence about a process that was never there, and a held lock reported as an +/// error lets two `io` processes into one session, which is the whole thing this +/// module exists to stop. +pub fn acquire(home: &Path, session: i64, root: &Path, started: SystemTime) -> io::Result { + crate::home::create(home)?; + let (lock, owner) = paths(home, session); + + // Not truncated: the file's contents are never read, and truncating one that + // another process holds a lock on would be a write for no reason. + let file = open(&lock, false)?; + match file.try_lock() { + Ok(()) => {} + Err(std::fs::TryLockError::WouldBlock) => { + return Ok(Taken::Refused(read_owner(&owner))); + } + Err(std::fs::TryLockError::Error(error)) => return Err(error), + } + + // **The lock is already ours before the record is written**, so a record + // that cannot be written costs the refusal its facts and nothing else. The + // alternative — treating it as a failure and giving the lock back — would + // turn a full disk into two `io` processes in one session. + let wrote = write_owner(&owner, &Owner::claiming(root, started)).is_ok(); + Ok(Taken::Held(Guard { file, owner, wrote })) +} + +/// The record as it is on disk, or an empty one. +/// +/// A record that is missing, unreadable, or not valid UTF-8 is an [`Owner`] with +/// nothing in it — the same shape a truncated one produces, because from the +/// refused process's seat they are the same fact: nothing is known about the +/// holder. +fn read_owner(path: &Path) -> Owner { + match std::fs::read_to_string(path) { + Ok(text) => Owner::parse(&text), + Err(_) => Owner::default(), + } +} + +/// Write the record, flushed before the guard is handed back. +/// +/// `sync_all` for the same reason [`crate::settings::write`] calls it: the +/// process that reads this file is a different one, and a record still sitting +/// in the page cache when the machine loses power is a lock with no holder named +/// beside it. +fn write_owner(path: &Path, owner: &Owner) -> io::Result<()> { + use std::io::Write as _; + let mut file = open(path, true)?; + file.write_all(owner.render().as_bytes())?; + file.sync_all() +} diff --git a/src/main.rs b/src/main.rs index 16c8ad2..cfbcf43 100644 --- a/src/main.rs +++ b/src/main.rs @@ -173,6 +173,20 @@ async fn run(report: &mut Vec) -> Result { return io_cli::exec::main(args, config, root, cli.model).await; } + // `io resume` leaves by the same door, for the same two reasons: it is + // headless, and `io resume --list` in CI has no terminal to be refused for. + // It answers a missing provider with a sentence of its own, so it must pass + // the wizard as well as the terminal check. + // + // A second `if let` on `cli.command` compiles because the arm above moves it + // only conditionally and returns; nothing reads it after this point. + if let Some(Subcommand::Resume(args)) = cli.command { + for line in report.drain(..) { + eprintln!("{line}"); + } + return io_cli::exec::resume_main(args, config, root, cli.model).await; + } + // A session draws, so it needs a terminal to draw on, and saying so is better // than half-working. The check sits AFTER the subcommand is known rather than // before it, because `io exec` is the answer to a non-TTY stdout rather than a @@ -307,7 +321,11 @@ async fn drive( // here: `adopt` moves files, so a second call would be a second migration, and // by the time there is an `App` to say this in the environment already names // the home — there would be nothing left to report. - report: Vec, + // + // **Mutable since 0.23.0**, because the session lock taken below has one + // thing worth saying that is the same kind of fact: what this process found + // when it arrived, said once, before the session it describes is running. + mut report: Vec, root: &std::path::Path, ) -> Result<(), String> { let Some(spec) = config.provider_spec().cloned() else { @@ -407,6 +425,60 @@ async fn drive( let store = Store::open(&store).map_err(|error| error.to_string())?; let session = Session::open(&store, root).map_err(|error| error.to_string())?; + // **One `io` at a time on one conversation, from here on — and a + // conversation is not a directory.** This product keeps one store for the + // whole machine, so two `io` in one repository is ordinary; they are not in + // conflict, because `Session::open` creates a new session row on every call + // and each terminal gets its own. What two processes can genuinely contend + // over is a single *session*, which happens only when one of them enters a + // session the other already has open — by `/resume`. See `io_cli::lock`. + // + // So this acquisition never fails: the id was created one line above and + // nobody else can be holding it. What it does is **publish the owner + // record**, so that a later process trying to enter this session can be + // refused and can say who has it. + // **Bound and never read**, which is the point: the guard is held for its + // `Drop`, and it must outlive every turn this process takes. `let _ = …` + // would release it on the next line, and a plain name would be a warning + // about the one thing that is deliberate here. + let _session_lock = match io_cli::home::path() { + Some(home) => { + // The only clock read on this path, and it is here because + // `src/main.rs` is the one file `tests/timing.rs` permits one in. + let now = std::time::SystemTime::now(); + match io_cli::lock::acquire(&home, session.id(), root, now) { + Ok(io_cli::lock::Taken::Held(guard)) => Some(guard), + // Not reachable for a session created a line ago, and said + // rather than swallowed precisely because it should not happen: + // it would mean the id was not fresh, which is a fact about the + // store worth putting in front of somebody. + Ok(io_cli::lock::Taken::Refused(owner)) => { + report.push(format!( + "this session was already locked by {} — that should not be possible \ + for a session just created", + owner.sentence() + )); + None + } + // A lock that cannot be taken for an ordinary filesystem reason is + // **not** a reason to refuse the session. The guard exists to stop + // a specific corruption, and trading it for "io will not start on + // this machine" is a worse failure than the one it prevents. + Err(error) => { + report.push(format!( + "this session could not be locked ({error}); if another io opens it \ + too, do not advance both" + )); + None + } + } + } + // No home means nowhere to keep a lock, which `home::adopt` has already + // said something about. Answered defensively rather than by refusing to + // start. + None => None, + }; + // Kept whole before the match consumes it, because `/model` needs a // `ProviderSpec` to ask `verify::catalogue` what this endpoint serves — the // same call the wizard's model step makes, rather than a second one. @@ -439,6 +511,7 @@ async fn drive( capabilities, skills, profile, + home: io_cli::home::path(), }, ) .await? @@ -696,6 +769,14 @@ struct Interactive<'a, 'b> { /// turn-boundary reload discovers the file afresh and would otherwise drop /// the overlay it produced. profile: Option, + /// The io home, or `None` where the operator has none. + /// + /// Carried only so `/resume` can take the lock on the session it is entering + /// — the one moment two `io` can genuinely contend, since every session this + /// process opens for itself is new and uncontested. `None` disables the check + /// rather than refusing the switch: a machine with nowhere to keep a lock is + /// not a machine to lock an operator out of. + home: Option, } impl provider::WithProvider for Interactive<'_, '_> { @@ -726,6 +807,7 @@ impl provider::WithProvider for Interactive<'_, '_> { self.capabilities, self.skills, self.profile, + self.home, model, ) .await @@ -772,6 +854,8 @@ async fn loop_over Result>( // it; it is re-applied after every turn-boundary reload, which goes back to // the file and knows nothing about either. mut profile: Option, + // See `Interactive::home`. Read only by the `/resume` arm. + home: Option, model: String, ) -> Result<(), String> { // Every request the session makes goes past this, and it is the only way @@ -901,6 +985,11 @@ async fn loop_over Result>( // stops where it stopped — so the reading a session-long `Instant` gave was // `22m12s` beside a turn six seconds old. Each turn is handed its own. let mut picker: Option<(Picker, Pick)> = None; + // The lock on a session this process entered by `/resume`, held until it + // enters another. `None` until the operator resumes for the first time — the + // session opened at startup is locked by `drive`, which holds that guard for + // the life of the process. + let mut entered: Option = None; // **The import offer, made once to everybody and never twice.** // @@ -1066,6 +1155,21 @@ async fn loop_over Result>( // sabotage could reach — `sessions::pick` is the same // line where a test can stand on it. Pick::Resume(ids) => match io_cli::sessions::pick(ids, index) { + // **The one moment two `io` can contend, and the only + // place the session lock ever refuses anything.** + // Every session this process opens for itself is new + // and uncontested; entering somebody else's is not. + // Taken before the session is swapped, so a refusal + // leaves the operator exactly where they were. + Some(id) + if !entering( + &home, + id, + &mut entered, + &store, + &session, + &mut app, + ) => {} Some(id) => match io_cli::sessions::resume(&store, id) { Ok(reopened) => { session = reopened; @@ -1087,6 +1191,63 @@ async fn loop_over Result>( Tone::Success, format!("resumed {}", session.root().display()), ); + // **And now the part that was missing until + // 0.23.0.** Reopening a session has never + // asked what its last run was waiting on, so + // a question the agent asked, a plan it + // proposed, or a call that never finished sat + // in the store while the interface offered a + // fresh prompt. The run is found from the + // head rather than from a second scan — the + // session's own head names the turn, and the + // turn names the run. `last_run` is the same + // walk `/expand` already uses, and it reads + // the on-path turn rather than the newest + // row, which is what makes it right after a + // fork or an undo. + let effective = approval::session_policy( + &policy, + app.posture(), + app.remembered(), + ); + if let Some(run_id) = + last_run(&session, &store).map(|turn| turn.run_id) + { + match io_cli::resume::pending_for(&store, run_id) { + // Nothing waiting is the ordinary + // case and says nothing: an operator + // reopening a finished session has + // asked for a prompt, not a report. + Ok(io_cli::resume::Pending::Finished) => {} + Ok(pending) => { + resume_pending( + screen, + inputs, + &mut app, + &provider, + &store, + &mut session, + &effective, + &config, + contained + .then_some(containment.as_ref()) + .flatten(), + &capabilities, + &seen, + run_id, + pending, + ) + .await?; + } + Err(error) => app.say( + Tone::Muted, + format!( + "that session's last run could not be \ + read: {error}" + ), + ), + } + } } Err(error) => app.say( Tone::Error, @@ -2181,6 +2342,18 @@ async fn loop_over Result>( Command::Attach(run_id) => { watch_child(screen, &mut app, &store, inputs, run_id).await?; } + // **Neither can arrive here, and the arms exist so that stays true + // by construction rather than by memory.** Both are produced only by + // an overlay opened from the store, and the only thing that opens + // one is `resume_pending`, which reads its own keys and consumes the + // answer before returning. If either ever reaches the idle loop, an + // operator's decision has been taken with nothing waiting for it — + // so it is said rather than dropped, which is the failure the + // `#[must_use]` on `App::answer_intent` is also there to prevent. + Command::Answered(_) | Command::Decided(_) => app.say( + Tone::Error, + "that decision arrived with no parked run waiting for it and was not delivered", + ), // The first `Esc`. Nothing has changed yet; this says what the second // one would change, in the turn's own words, so a confirmation is a // confirmation of something specific rather than of a keystroke. @@ -2204,7 +2377,24 @@ async fn loop_over Result>( } } Ok(None) => app.say(Tone::Muted, "there is no turn to undo"), - Err(error) => app.say(Tone::Error, format!("nothing was undone: {error}")), + // **`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)), + ), }, Command::Slash(text) => match commands::parse(&text, app.keys(), &app.theme) { Action::Print(lines) => { @@ -4010,7 +4200,25 @@ async fn turn( // work above is real and the turn did not finish. None if undone => {} None => app.say(Tone::Muted, "stopped"), - Some(Ok(_)) => {} + // **A turn that ended parked said nothing at all until 0.23.0.** The + // harness returns `AwaitingAnswer`, `AwaitingPlan` or `AwaitingRecovery` + // as an ordinary `Ok`, so this arm matched and dropped it — and the + // operator got their prompt back with no sign that a run was sitting in + // the store waiting for a sentence from them. Every other way a turn can + // end has a line; this is the one that pays for itself, because the work + // is still there to be finished. + Some(Ok(result)) => { + if io_cli::exec::code(&result.outcome) == io_cli::exec::PAUSED { + app.record( + Tone::Warning, + format!( + "this turn {} {} /resume opens it and carries it on", + io_cli::exec::describe(&result.outcome), + app.theme.glyphs.dash + ), + ); + } + } } app.status.elapsed = started.elapsed(); paint(screen, app)?; @@ -4871,6 +5079,597 @@ const ATTACH_POLL: Duration = Duration::from_millis(100); /// step draws exactly as it would have if its parent had waited for it. That is /// the property worth having: attaching changes when an operator sees a run, not /// what it looks like. +/// Take the lock on a session this process is about to enter, and say so when +/// somebody else has it. +/// +/// **This is the only place the session lock ever refuses anything**, and it is +/// the only place it could: every session `io` opens for itself is created by +/// that call and cannot be held by anybody, so the acquisition at startup is a +/// publication rather than a contest. Entering a session that already exists is +/// the contest. +/// +/// Returns whether the switch may go ahead. On success the new guard replaces the +/// one held for the session being left — dropped in the same statement, which is +/// what releases it for anybody waiting. +/// +/// **A refusal naming this very process is not a refusal.** The startup guard is +/// still holding the session this process opened, and an advisory lock is held per +/// open file description, so resuming *back* into it would be refused by our own +/// handle. The owner record says whose it is, so a pid equal to this one is read +/// as "already ours" and the switch proceeds. +/// +/// No home, or a lock that cannot be taken for an ordinary filesystem reason, +/// both allow the switch: a machine with nowhere to keep a lock is not a machine +/// to lock an operator out of their own work. +fn entering( + home: &Option, + id: i64, + held: &mut Option, + store: &Store, + session: &Session, + app: &mut App, +) -> bool { + let Some(home) = home else { + return true; + }; + // `src/main.rs` is the one file permitted a clock read; the lock module takes + // every instant as an argument for exactly that reason. + let now = std::time::SystemTime::now(); + // **The root of the session being entered, not the one being left.** Read + // from the store rather than from the `Session` in hand, because the switch + // has not happened yet — `session.root()` here is the *previous* + // conversation's workspace, and writing that into the owner record would + // point a second `io`'s refusal at the wrong directory, which is the one + // thing that clause exists to get right. A store that cannot answer falls + // back to the current root rather than refusing the switch. + let root = store + .session_root(id) + .ok() + .flatten() + .map(std::path::PathBuf::from) + .unwrap_or_else(|| session.root().to_path_buf()); + match io_cli::lock::acquire(home, id, &root, now) { + Ok(io_cli::lock::Taken::Held(guard)) => { + // Assigning drops whatever was held for the session being left, in + // this statement, which is what releases it for anyone waiting. + *held = Some(guard); + true + } + Ok(io_cli::lock::Taken::Refused(owner)) => { + if owner.pid == Some(std::process::id()) { + // **Ours already — and the guard for the session being left must + // still go.** Returning here without clearing it leaked a lock: + // resuming A → B → A left this process holding B's lock and B's + // owner record while working in A, so a second `io` asking for B + // was refused and sent to a terminal showing something else. + // + // The session this refusal names is held by `drive` for the whole + // process, so there is nothing to take — only the stale one to + // release, which this does. + *held = None; + return true; + } + let lapsed = owner.lapsed(now, io_cli::lock::LEASE) == Some(true); + app.say( + Tone::Refused, + format!( + "another io has that session open — {}.{} Two of them would advance one \ + conversation and orphan a turn somebody paid for.", + owner.sentence(), + if lapsed { + " Its lease has run out, so if that process is gone, close this one \ + and reopen — the lock goes with the process." + } else { + "" + } + ), + ); + false + } + Err(error) => { + // Opened anyway, for the reason the startup acquisition gives — but + // the guard for the session being left is still dropped, or this + // process would hold a lock on a conversation it is no longer in. + *held = None; + app.say( + Tone::Muted, + format!("that session could not be locked ({error}); opening it anyway"), + ); + true + } + } +} + +/// What the operator decided about a parked run, before anything is driven. +/// +/// Separate from the driving so the decision can be taken with the store's own +/// row on screen and nothing running, which is the state a parked run is in. +enum Decided { + /// An answer to the question the agent asked. + Answer(String), + /// A verdict on the plan it proposed. + Verdict(io_harness::PlanVerdict), + /// What to do about a call that was started and never finished. + Recovery(io_harness::RecoveryDecision), + /// Nothing to decide — the process died and the run simply needs driving. + CarryOn, + /// The operator backed out. The run stays exactly as it was found. + Left, +} + +/// Put the parked decision in front of the operator and wait for it. +/// +/// **The overlay is `App`'s own**, opened through `open_resumed_intent` / +/// `open_resumed_plan` and routed through `App::key` and `App::render` like any +/// other modal — one widget, one key map, one paint path, so the resumed surface +/// cannot drift from the live one. What comes back out is +/// [`io_cli::app::Command::Answered`] / [`io_cli::app::Command::Decided`], which +/// exist because a stored pause has no run listening for its answer. +async fn ask_parked( + screen: &mut Screen>, + inputs: &mut UnboundedReceiver, + app: &mut App, + store: &Store, + pending: &io_cli::resume::Pending, +) -> Result { + use io_cli::resume::Pending; + match pending { + // **The row is read again rather than rebuilt from the classification.** + // `Pending` carries the parts so a surface can draw a list without + // touching the store per row, but the overlay wants the harness's own + // type — and re-reading is both the shortest way to get one and the + // freshest answer, since another `io` may have resolved it since the + // list was drawn. + Pending::Question { question_id, .. } => match store.question(*question_id) { + Ok(Some(row)) => { + app.open_resumed_intent(io_cli::intent::Intent::resumed(&row)); + } + Ok(None) => { + app.say( + Tone::Muted, + format!("question {question_id} is no longer in the store"), + ); + return Ok(Decided::Left); + } + Err(error) => { + app.say( + Tone::Error, + format!("that question could not be read: {error}"), + ); + return Ok(Decided::Left); + } + }, + Pending::Plan { plan_id, .. } => match store.plan(*plan_id) { + Ok(Some(row)) => app.open_resumed_plan(io_cli::plan::Review::resumed(&row)), + Ok(None) => { + app.say( + Tone::Muted, + format!("plan {plan_id} is no longer in the store"), + ); + return Ok(Decided::Left); + } + Err(error) => { + app.say(Tone::Error, format!("that plan could not be read: {error}")); + return Ok(Decided::Left); + } + }, + // Neither of these opens a widget. A recovery decision is three words and + // a died run is a yes, so both are answered from the keys below with the + // question in the scrollback — an overlay for a one-key answer would be a + // surface built to be dismissed. + Pending::Recovery { tool, step, .. } => app.say( + Tone::Warning, + format!( + "{tool} was called at step {step} and never finished {} r retries it, \ + a abandons the run, Esc leaves it parked", + app.theme.glyphs.dash + ), + ), + Pending::Died { last_step } => app.say( + Tone::Warning, + format!( + "this run stopped after step {last_step} without finishing {} Enter carries \ + it on, Esc leaves it parked", + app.theme.glyphs.dash + ), + ), + // Neither reaches here — the caller answers both before asking. + Pending::Interrupted | Pending::Finished => return Ok(Decided::Left), + } + paint(screen, app)?; + loop { + match inputs.recv().await { + Some(Event::Key(key)) if key.kind == KeyEventKind::Press => { + // The two overlays answer through `App`; the two key prompts + // answer here. Split on which is open rather than on the pending + // kind, so a widget that failed to open cannot leave this loop + // reading keys nobody can see the effect of. + if app.modal() { + match app.key(key) { + Command::Answered(Some(answer)) => return Ok(Decided::Answer(answer)), + // A declined question. `Intent`'s own `Esc`, which means + // the same thing here as it does live: leave it for later. + Command::Answered(None) => return Ok(Decided::Left), + Command::Decided(verdict) => return Ok(Decided::Verdict(verdict)), + // **The way out of a plan that must not be cancelled.** + // `Esc` on a plan is `Cancel`, which is a real decision + // that ends the run — right for a live turn and wrong for + // an operator who opened a parked plan to read it. + Command::Interrupt | Command::Abandon | Command::Exit => { + app.leave_resumed(); + paint(screen, app)?; + return Ok(Decided::Left); + } + _ => {} + } + paint(screen, app)?; + continue; + } + let interrupting = + key.code == KeyCode::Char('c') && key.modifiers.contains(KeyModifiers::CONTROL); + match key.code { + KeyCode::Esc => return Ok(Decided::Left), + _ if interrupting => return Ok(Decided::Left), + KeyCode::Char('r') if matches!(pending, Pending::Recovery { .. }) => { + return Ok(Decided::Recovery(io_harness::RecoveryDecision::Retry)); + } + KeyCode::Char('a') if matches!(pending, Pending::Recovery { .. }) => { + return Ok(Decided::Recovery(io_harness::RecoveryDecision::Abort)); + } + KeyCode::Enter if matches!(pending, Pending::Died { .. }) => { + return Ok(Decided::CarryOn); + } + _ => {} + } + } + // A resize while a decision is up is a resize, exactly as it is in + // the other two loops in this file. Dropping it leaves the `Screen` + // believing in a width the terminal no longer has. + Some(Event::Resize(width, height)) => { + screen + .resize(width, height) + .map_err(|error| error.to_string())?; + paint(screen, app)?; + } + Some(_) => {} + // The terminal went away. Leaving the run parked is the only honest + // answer: nothing was decided. + None => return Ok(Decided::Left), + } + } +} + +/// Answer a run that stopped, and carry it on from the step it stopped at. +/// +/// The decision is taken first and the run is driven second, because a parked +/// run is not running and the operator is reading a row the store wrote. What is +/// driven is `crate::resume`'s own function for that kind of pause — never a new +/// turn, which is what `/resume` amounted to before 0.23.0. +/// +/// **The contract is rebuilt here and the goal comes off the turn row.** +/// io-harness stores no contract and publishes no reader for a run's goal, so a +/// run that served no session turn cannot be resumed from the interface at all; +/// `io resume --goal` is the door for those, and this says so rather than +/// resuming against an empty goal. +#[allow(clippy::too_many_arguments)] +async fn resume_pending( + screen: &mut Screen>, + inputs: &mut UnboundedReceiver, + app: &mut App, + provider: &P, + store: &Store, + session: &mut Session, + policy: &Policy, + config: &Config, + containment: Option<&io_harness::Containment>, + capabilities: &io_cli::contract::Capabilities, + // The same record the turn loop reads, and it is here for the same reason: a + // resumed run makes real completion calls, so `ctx N%` must move with them. + // A drain that read the per-step edits and not the assembly would leave the + // field describing the turn before the pause — which is the disagreement + // between the field and `/context` that a live run caught in 0.17.0, arriving + // through a new door. `tests/context_share.rs` counts the two against each + // other so this cannot be forgotten on a path added later. + seen: &io_cli::context::Seen, + run_id: i64, + pending: io_cli::resume::Pending, +) -> Result<(), String> { + use io_cli::resume::{self, Pending}; + match &pending { + // The one pause that cannot be answered, and the sentence is the whole + // of this arm's job. See `crate::resume` for why a cancelled run is + // terminal to every resume entry point in the pinned harness. + // The sentence is `sessions::note`'s and not a second copy of it: the + // picker row and this line describe the same state, and two spellings of + // one fact are two things to keep in step. + Pending::Interrupted => { + if let Some(note) = io_cli::sessions::note(&pending) { + app.say(Tone::Refused, note); + } + return Ok(()); + } + Pending::Finished => { + app.say( + Tone::Muted, + "that session's last run finished; nothing is waiting", + ); + return Ok(()); + } + _ => {} + } + let decided = ask_parked(screen, inputs, app, store, &pending).await?; + if matches!(decided, Decided::Left) { + paint(screen, app)?; + return Ok(()); + } + // The operator's own words, off the turn the run served. `goal_for` answers + // `None` for a run that served no turn, which an interactive session cannot + // have produced but a headless one can — and a store is shared between them. + let goal = match resume::goal_for(store, run_id) { + Ok(Some(goal)) => goal, + Ok(None) => { + app.say( + Tone::Refused, + format!( + "run {run_id} was not started by a session, so its goal is not in the \ + store — resume it with `io resume --goal`" + ), + ); + return Ok(()); + } + Err(error) => { + app.say( + Tone::Error, + format!("run {run_id} could not be read: {error}"), + ); + return Ok(()); + } + }; + // **A question asked by the resumed run parks it again rather than opening a + // second overlay.** The receiver is dropped, which is how io-harness is told + // nobody is here to answer — the same idiom the idle contract reads already + // use. Answering a fresh question mid-resume would mean this loop growing a + // second copy of the turn loop's overlay handling, and the operator can + // simply `/resume` again. + // **Bound as `continuing`, not as `contract`.** `tests/contract.rs` finds the + // turn's own builder by its binding name and asserts there is exactly one of + // it, because "one contract per turn" stopped being the same sentence as "one + // mention of the builder in this file" in 0.14.0. This is a third kind of + // site — it takes a run, so it is not one of the two reading sites either — + // and it gets its own name and its own assertion rather than widening a + // count that would then admit a genuine second arm. + // **`_` and never `_parked`.** A named binding, underscore-prefixed or not, + // lives to the end of the scope; only the bare wildcard drops here and now. + // Written as `_parked` this froze the session outright: the receiver stayed + // open, `Answerer::answer` sent into it and awaited a reply nobody would ever + // send, and a resumed run that asked a second question hung forever with + // nothing on screen. Dropped, the send fails, the `oneshot` goes with it, and + // `answer` resolves `None` — which is io-harness's own "nobody can answer", + // so the run parks again and `/resume` finds it. + let (answerer, _) = io_cli::intent::channel(); + let continuing = io_cli::contract::session( + goal, + session.root().to_path_buf(), + config, + capabilities, + std::sync::Arc::new(answerer), + // No plan gate. A resumed run that proposed a plan is one this surface + // is in the middle of answering; registering a gate would turn the + // planning phase back on for the continuation. + None, + ); + let (observer, mut events) = bridge::channel(); + let canceller = observer.canceller(); + let (approver, mut asks) = approval::channel(); + // Read before anything is driven, and handed down so the head write is a + // compare-and-swap against the head this process believed in. + let expected_head = session.head(); + let started = Instant::now(); + app.say(Tone::Muted, format!("resuming run {run_id}")); + // **The interface must know a run is in flight, and it did not.** Left at + // `Mode::Idle` this loop was a trap: `App::compose` only queues a prompt + // while a run is going, so a line typed during a resume was taken out of the + // composer and thrown away; and `interrupt_or_quit` took its *quit* branch, + // so `Ctrl+C` printed "press again to exit" and then did nothing at all, + // twice over. Saying a run has started fixes both — the prompt is queued and + // the key becomes an interrupt — and it is what a live turn says here too. + app.started(); + paint(screen, app)?; + let driving = async { + match decided { + Decided::Answer(answer) => match &pending { + Pending::Question { question_id, .. } => { + resume::answer_question( + &continuing, + provider, + store, + run_id, + *question_id, + &answer, + policy, + &approver, + containment, + &observer, + expected_head, + ) + .await + } + _ => unreachable!("an answer is only taken for a question"), + }, + Decided::Verdict(verdict) => match &pending { + Pending::Plan { plan_id, .. } => { + resume::decide_plan( + &continuing, + provider, + store, + run_id, + *plan_id, + verdict, + policy, + &approver, + containment, + &observer, + expected_head, + ) + .await + } + _ => unreachable!("a verdict is only taken for a plan"), + }, + Decided::Recovery(decision) => match &pending { + Pending::Recovery { attempt_id, .. } => { + resume::recover( + &continuing, + provider, + store, + run_id, + *attempt_id, + decision, + policy, + &approver, + containment, + &observer, + expected_head, + ) + .await + } + _ => unreachable!("a recovery decision is only taken for an open attempt"), + }, + Decided::CarryOn => { + resume::carry_on( + &continuing, + provider, + store, + run_id, + None, + &approver, + containment, + &observer, + expected_head, + ) + .await + } + Decided::Left => unreachable!("the caller returned on Left"), + } + }; + tokio::pin!(driving); + let mut ticker = tokio::time::interval(io_cli::app::TICK); + ticker.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); + let resumed = loop { + tokio::select! { + result = &mut driving => break result, + _ = ticker.tick() => { + if app.tick(started.elapsed()) { + paint(screen, app)?; + } + } + Some(event) = events.recv() => { + let at = started.elapsed(); + app.status.elapsed = at; + app.event(&event, at); + commit_edits(app, store, &event, screen.width()); + note_context(app, store, &event, seen, &continuing); + paint(screen, app)?; + } + // A resumed run asks for the same approvals a live one does, and it + // is the same overlay. Without this arm the run would stop inside + // `Approver::decide_in_context` with nothing on screen. + Some(ask) = asks.recv() => { + app.open_approval(ask); + paint(screen, app)?; + } + // **`Event::Key(_)` alone silently swallowed every resize**, because + // a `Resize` fails the pattern and `select!` consumes the event + // anyway — leaving the `Screen` believing in a width the terminal no + // longer has for the rest of the run. Both sibling loops in this file + // handle it; this one did not. + Some(input) = inputs.recv() => { + match input { + Event::Key(key) if key.kind == KeyEventKind::Press => { + match app.key(key) { + // Honoured at the next step boundary, through the + // observer's flag — the same mechanism a live turn's + // stop key uses, and the same sentence + // `interrupt_or_quit` has just put on screen. + // + // **Both presses cancel and neither drops the + // future**, which is where this differs from a live + // turn deliberately: the resume drivers close the + // session's turn and move its head *after* the loop + // returns, so abandoning the future mid-flight would + // leave the turn open and the head unmoved — the + // silent half-finished state this release exists to + // stop producing. + Command::Interrupt | Command::Abandon => { + canceller.store(true, std::sync::atomic::Ordering::Relaxed); + } + // Neither can arrive from a resumed overlay: the + // decision was taken before anything was driven, and + // an approval resolves through its own channel. + Command::Answered(_) | Command::Decided(_) => {} + _ => {} + } + paint(screen, app)?; + } + Event::Resize(width, height) => { + screen + .resize(width, height) + .map_err(|error| error.to_string())?; + paint(screen, app)?; + } + _ => {} + } + } + } + }; + // Anything typed while the run was carrying on. `App::compose` queued it + // rather than destroying it — which is what `App::started` above bought — + // and the queue belongs to the turn that is now over, so it is dropped here + // **and said**, exactly as a stopped turn's queue is. Silently is the one way + // it must not go. + let queued = app.forget_queued_prompts(); + if queued > 0 { + let dash = app.theme.glyphs.dash; + app.say( + Tone::Muted, + format!("{queued} typed while the run carried on {dash} not sent; type them again"), + ); + } + app.finished(); + match resumed { + Ok(done) => { + // The step it carried on from, said once. The per-step `skipped` + // markers io-harness writes are accurate and unreadable — thirty-nine + // of them for a run resumed at step forty — so they stay in the trace + // where `/expand` reaches them. + app.record( + Tone::Success, + format!( + "carried on from step {} {} {}", + done.resumed_after + 1, + app.theme.glyphs.dash, + io_cli::exec::describe(&done.outcome) + ), + ); + if let Some(reply) = &done.reply { + app.record(Tone::Normal, reply.clone()); + } + // The head moved underneath this `Session`, which caches it in a + // private field no setter reaches. Re-reading is what stops the next + // turn parenting onto the turn that was just closed. + match io_cli::sessions::resume(store, session.id()) { + Ok(reopened) => *session = reopened, + Err(error) => app.say( + Tone::Error, + format!("the session could not be re-read after the resume: {error}"), + ), + } + } + Err(failure) => app.record(Tone::Refused, failure.to_string()), + } + paint(screen, app)?; + Ok(()) +} + async fn watch_child( screen: &mut Screen>, app: &mut App, diff --git a/src/plan.rs b/src/plan.rs index eeae7b7..54d25c0 100644 --- a/src/plan.rs +++ b/src/plan.rs @@ -11,8 +11,17 @@ //! `Esc` cancels — so the destructive answer is the one key that never means //! anything else, and the correction is prose rather than a menu, because "not //! like that, like this" is the whole value of a gate. +//! +//! # Two ways in, one overlay (0.23.0) +//! +//! A plan can also be decided long after the run that proposed it stopped, as a +//! `plans` row read back off the store. [`Review::resumed`] opens on one, and +//! everything from there — the keys, the drawn steps, the footer — is the same +//! code as the live path. The one difference is where the verdict goes: +//! [`Review::resolve`] returns it rather than sending it, for the caller to +//! deliver with `resume_with_plan_decision_observed`. -use io_harness::{Plan, PlanGate, PlanReview, PlanVerdict}; +use io_harness::{PendingPlan, Plan, PlanGate, PlanReview, PlanVerdict}; use ratatui::layout::Rect; use ratatui::text::Line; use ratatui::widgets::Paragraph; @@ -20,6 +29,7 @@ use ratatui::Frame; use tokio::sync::{mpsc, oneshot}; use crate::composer::{Composer, Reply}; +use crate::intent::Destination; use crate::theme::{Theme, Tone}; /// One proposed plan, and the channel its verdict goes back down. @@ -57,9 +67,10 @@ impl PlanGate for Gate { } } -/// The overlay a plan is decided through. +/// The overlay a plan is decided through, live or resumed. pub struct Review { - proposed: Proposed, + plan: Plan, + verdict: Destination>, composer: Composer, } @@ -68,14 +79,29 @@ impl Review { /// anything is a correction the operator did not write. pub fn new(proposed: Proposed) -> Self { Self { - proposed, + plan: proposed.plan, + verdict: Destination::Turn(proposed.verdict), + composer: Composer::new(), + } + } + + /// Open on a plan a run already paused on, read back off the store. + /// + /// Only `plan` is taken. The row's `verdict` and `decided_by` describe a + /// decision already made, and a plan that has one is not one to decide again; + /// keeping that check in the caller keeps this constructor from being the + /// place a decided plan is quietly re-opened. + pub fn resumed(pending: &PendingPlan) -> Self { + Self { + plan: pending.plan.clone(), + verdict: Destination::Stored, composer: Composer::new(), } } /// The plan on screen. pub fn plan(&self) -> &Plan { - &self.proposed.plan + &self.plan } /// A keystroke while the overlay is up. `Some` closes it with that verdict. @@ -102,12 +128,24 @@ impl Review { } } - /// Send the verdict back to the run. - pub fn resolve(self, verdict: Option) { - let _ = self.proposed.verdict.send(verdict); + /// Resolve the plan. Consumes the overlay: a plan decided twice is a run + /// spending its budget on a decision nobody took. + /// + /// Returns `None` when the verdict has been delivered to a live turn, and + /// `Some(verdict)` when this overlay was opened by [`Self::resumed`] — there + /// was no turn awaiting it, so it comes back here for the caller to deliver + /// with `io_harness::resume_with_plan_decision_observed`. + pub fn resolve(self, verdict: Option) -> Option> { + self.verdict.deliver(verdict) } /// The steps as steps, and the three ways out. + /// + /// Identical for both ways in, footer included, and that is not an oversight: + /// `Esc` here is a verdict rather than a deferral. It cancels the plan on a + /// live turn and on a resumed one alike, so "Esc cancels" is true of both — + /// unlike [`crate::intent`], where declining defers and the two paths defer + /// to different places. pub fn render(&self, frame: &mut Frame, area: Rect, theme: &Theme) { if area.height == 0 { return; @@ -117,10 +155,10 @@ impl Review { format!( "a plan, before any of it runs {} {} steps", theme.glyphs.dash, - self.proposed.plan.steps.len() + self.plan.steps.len() ), )]; - for (index, step) in self.proposed.plan.steps.iter().enumerate() { + for (index, step) in self.plan.steps.iter().enumerate() { let owner = match &step.agent { Some(agent) => format!(" [{agent}]"), None => String::new(), diff --git a/src/resume.rs b/src/resume.rs new file mode 100644 index 0000000..931d04f --- /dev/null +++ b/src/resume.rs @@ -0,0 +1,937 @@ +//! Answering a run that paused, instead of abandoning it. +//! +//! Since 0.10.0 this interface has been able to ask the operator a question and +//! to propose a plan, and since 0.20.0 a tool call can be interrupted in a way +//! io-harness records but cannot judge. In every one of those cases the run +//! stops, the pending row is written to the store, and through 0.22.0 io-cli +//! walked away from it: [`crate::exec`] printed that carrying on was "not in +//! this release" and exited 4, and `/resume` — one line, `Session::reopen` — +//! swapped the session handle without ever asking what the last run was waiting +//! on. +//! +//! Everything needed to do better has been public in io-harness the whole time. +//! This module reads the pending rows back, drives the matching resume entry +//! point, and then does the session bookkeeping that entry point does not. +//! +//! # The four kinds, which look alike and are not +//! +//! A question, a plan, an interrupted tool call and a run whose process merely +//! died each have their own reader, their own resume function and their own +//! flat-versus-tree twin. They are kept apart on purpose: collapsing them into +//! one generic switch over an integer is how an operator's answer is delivered +//! into somebody else's run, so each pending kind is its own type carrying its +//! own id and no bare integer crosses a surface. +//! +//! # What a free resume does not do +//! +//! `Session::drive` is the only thing in io-harness that stitches a run to a +//! turn, and it is private. The free resume functions know nothing about turns, +//! so after one of them the `session_turns` row still reads `awaiting_answer` +//! with an empty reply and the session head has not moved. `Store::turn_for_run`, +//! `Store::finish_turn` and `Store::set_session_head_if` are all public, so this +//! module closes the turn itself — with the **compare-and-swap** head write and +//! never the unconditional one, which is the defect [`crate::rewind`] carried +//! until this release. +//! +//! The reply is taken off the last assistant turn in the store rather than +//! reconstructed. io-harness's own extraction is a private function splitting on +//! a `pub(crate)` sentinel, so reproducing it would mean hardcoding a literal +//! this crate cannot see change. +//! +//! # The one pause that cannot be answered +//! +//! A turn the operator interrupted is finished, not paused. `Ctrl+C` sets the +//! flag that returns `Flow::Cancel`, io-harness records the outcome `cancelled`, +//! `finish_run` maps that to a *completed* status, and every resume entry point +//! short-circuits on a completed run and returns the original outcome without +//! driving. So the most common way an io-cli turn stops is the one way it cannot +//! be continued. This module reports such a run as ended by the operator and +//! points at `/fork` from the turn before it, which is the honest neighbouring +//! answer. +//! +//! The published io-harness documentation disagrees — `Steer::interrupt` says +//! such a turn "stays resumable" — and it is wrong, contradicted by the run loop +//! in the same crate. Reported upstream; not worked around here. +//! +//! # What else this module works around rather than fixes +//! +//! * **The goal is not readable.** `runs.goal` has no public reader, so a +//! contract cannot be rebuilt from the run alone. For a run that served a +//! session turn the operator's own words are recoverable — that is +//! [`goal_for`] — and for a bare run they are not, so the caller supplies +//! them. Every driver here therefore takes a whole [`TaskContract`] rather +//! than a run id and a promise to reconstruct one, which also settles what +//! `Session::drive` being private would otherwise cost. +//! * **A stitched turn always reads back as `TurnKind::Run`.** +//! `Store::turn_kind` and `Store::set_turn_kind` are both `pub(crate)`, so +//! this module cannot say what kind of turn it closed and cannot read what +//! kind the original was. A conversational turn that paused and was resumed +//! here is therefore reported as a run. Nothing in this crate can do better +//! until the pair is published. +//! * **An interrupted call in a *contained* run has no entry point.** +//! io-harness 0.69 publishes `resume_tree_with_answer` and +//! `resume_tree_with_plan_decision` but no recovery twin, and the flat +//! `resume_with_recovery` ends in the flat loop — so driving a tree root +//! through it would silently drop the containment the tree was running under. +//! [`recover`] refuses instead, which is why it is the one driver here that +//! takes no [`Containment`]. +//! * **`Attach::answer_question` must not be used on this path.** On a run that +//! has already ended it resolves the row and the compare-and-swap inside +//! `resume_with_answer` then finds nothing left to answer, so the resume fails +//! with `Error::Resume` and the operator's answer is stranded in a row no run +//! will ever read. The answer travels as an argument to [`answer_question`] +//! and by no other road. +//! +//! # Where the sentences come from +//! +//! `Error::Resume` is one variant carrying a `reason` string, and four quite +//! different mistakes arrive in it: a question that belongs to another run, a +//! question somebody already answered, a plan that belongs to another run, and a +//! run whose recorded policy cannot be rebuilt. An interface that printed the +//! `Display` would hand an operator a sentence written for a library caller. So +//! each of the four is checked here, before the harness is asked, and each has +//! its own [`Failure`] variant with its own sentence. What is left — a storage +//! failure, a checkpoint format from the future — is carried through as +//! [`Failure::Harness`], which is the honest place for a cause this module has +//! nothing better to say about. + +use io_harness::{ + AssistantTurn, Containment, Error, Observer, PendingQuestion, PlanStep, PlanVerdict, Policy, + Provider, RecoveryDecision, RunOutcome, RunStatus, Store, TaskContract, +}; + +/// What a run stopped on, as data. Marks, wording and ordering belong to the +/// surface that shows it; nothing here is a sentence an operator reads. +/// +/// The four answerable kinds each carry their own id under its own name, so a +/// question id cannot be handed to the plan driver by a caller that got its +/// variables the wrong way round — the mistake that would deliver one operator's +/// answer into somebody else's run. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Pending { + /// The agent asked the operator something and nobody answered. + Question { + /// The row [`answer_question`] takes. + question_id: i64, + /// What the agent asked, in its own words. + question: String, + /// What it already knew, when it said. + context: Option, + /// The options it offered. An answer is not obliged to be one of them. + choices: Vec, + /// The step it asked on, which is committed — so the resume starts after + /// it and the question is not asked twice. + step: u32, + }, + /// The agent proposed an approach and nobody decided. + Plan { + /// The row [`decide_plan`] takes. + plan_id: i64, + /// The approach, step by step. + steps: Vec, + /// The step it was proposed on, committed for the same reason a + /// question's is. + step: u32, + }, + /// A call the harness cannot inspect was started and never finished, so + /// whether it landed is a fact only the operator can establish. + Recovery { + /// The journal row [`recover`] takes. + attempt_id: i64, + /// The tool that was called. + tool: String, + /// The step the call was made on. The operator's account is filed + /// against *this* step and not the current one, so the resumed run reads + /// a transcript in which the tool answered where it was asked. + step: u32, + }, + /// The process went away mid-loop. Nothing is waiting for a person; the run + /// simply has committed work and no ending. + Died { + /// The last step that committed. The resume starts at the one after it. + last_step: u32, + }, + /// The operator stopped the turn themselves. Terminal — see this module's + /// documentation for why, and [`Failure::Interrupted`] for what to offer + /// instead. + Interrupted, + /// There is nothing to answer and nothing to drive. + /// + /// **Including one case that is not quite that, and is worth knowing about.** + /// A run that paused on a question is recorded with the status `completed` — + /// only `awaiting_approval` and `awaiting_plan` are recorded as `paused` — so + /// a question another process answered out of band, through `Attach`, leaves + /// a run that reads as finished here while io-harness would still drive it. + /// That is why the drivers check their own pending row before they consult + /// this classification: an operator in that position needs to be told the + /// question was answered, not that the run has ended. + Finished, +} + +/// What one resume did, built from what the harness returned and from the one +/// number that has to be read before it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Resumed { + /// The run that was driven. + pub run_id: i64, + /// Why it stopped this time. + pub outcome: RunOutcome, + /// The last step that had committed **before** anything was driven, read off + /// the store at the moment the decision was taken. The first step this + /// resume drove is this plus one, and a caller that wants to assert where the + /// run picked up should do that arithmetic rather than trust a literal: the + /// number depends on how much the previous process got through, which is not + /// something a test fixture decides. + pub resumed_after: u32, + /// The turn this closed, or `None` for a bare run that served no turn. + pub turn_id: Option, + /// What the agent said, as filed on the turn. `None` when the run's last + /// word was a tool call rather than a message. + pub reply: Option, +} + +/// Why a resume did not happen, or did not finish. +/// +/// Every variant is a distinct thing an operator can act on. [`Self::HeadMoved`] +/// is the one that happens *after* the run has already been driven, and it is +/// kept apart from the refusals for that reason: the work was done and paid for, +/// and a surface that reported it as "the resume was refused" would invite the +/// operator to try again and pay twice. +#[derive(Debug)] +pub enum Failure { + /// The turn was interrupted by the operator, so it is finished rather than + /// paused and no resume entry point will drive it. + Interrupted { + /// The run that was asked about. + run_id: i64, + }, + /// The run ended. There is nothing waiting and nothing left to drive. + Ended { + /// The run that was asked about. + run_id: i64, + }, + /// No run with this id is in the store. + Unknown { + /// The id that was asked about. + run_id: i64, + }, + /// No question with this id is in the store. + NoSuchQuestion { + /// The id that was asked about. + question_id: i64, + }, + /// The question is real and belongs to a run that is not this one — and, for + /// a contained resume, not to any run in this tree either. + QuestionElsewhere { + /// The question that was named. + question_id: i64, + /// The run it actually belongs to. + owner: i64, + /// The run it was offered to. + run_id: i64, + }, + /// The question already has an answer, so the run has already acted on one. + /// Driving it again from a second answer is the silent double-answer the + /// store's compare-and-swap exists to make impossible. + QuestionAnswered { + /// The question that was named. + question_id: i64, + }, + /// No plan with this id is in the store. + NoSuchPlan { + /// The id that was asked about. + plan_id: i64, + }, + /// The plan is real and belongs to another run. + PlanElsewhere { + /// The plan that was named. + plan_id: i64, + /// The run it actually belongs to. + owner: i64, + /// The run it was offered to. + run_id: i64, + }, + /// The plan already has a verdict. + PlanDecided { + /// The plan that was named. + plan_id: i64, + }, + /// The attempt is not open on this run: it was never opened, belongs to + /// another run, or has already been decided. + AttemptNotOpen { + /// The attempt that was named. + attempt_id: i64, + /// The run it was offered to. + run_id: i64, + }, + /// A contained resume was asked for against a run that is not the top of its + /// own tree. Containment is a property of a whole tree, so it can only be + /// applied at the root. + NotTheRoot { + /// The run that was named. + run_id: i64, + /// The root of its tree, which is what a contained resume takes. + root: i64, + }, + /// A contained run's interrupted call has no entry point in this harness. + /// See this module's documentation. + ContainedRecovery { + /// The run that was named. + run_id: i64, + }, + /// Nothing recorded the boundary this run executed under, and none was + /// supplied, so resuming it would mean choosing one on the operator's behalf. + NoRecordedPolicy { + /// The run that was named. + run_id: i64, + }, + /// The run was driven and its turn was closed, but the session head had moved + /// on since the resume was decided, so the compare-and-swap refused. The + /// answer landed; where the conversation continues from did not change. + HeadMoved { + /// The session whose head was being moved. + session_id: i64, + /// The turn this resume closed, which is where the head would have gone. + turn_id: i64, + }, + /// Anything io-harness reported that this module has nothing better to say + /// about. + Harness(Error), +} + +impl std::fmt::Display for Failure { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Interrupted { run_id } => write!( + f, + "run {run_id} was stopped by you rather than paused, so there is nothing \ + waiting for an answer and no way to carry it on. Use /fork from the turn \ + before it to take the conversation somewhere else." + ), + Self::Ended { run_id } => { + write!(f, "run {run_id} has ended; there is nothing to continue") + } + Self::Unknown { run_id } => { + write!(f, "no run {run_id} in this store") + } + Self::NoSuchQuestion { question_id } => { + write!(f, "no question {question_id} in this store to answer") + } + Self::QuestionElsewhere { + question_id, + owner, + run_id, + } => write!( + f, + "question {question_id} was asked by run {owner}, not by run {run_id}; \ + answering it here would deliver your answer into another run" + ), + Self::QuestionAnswered { question_id } => write!( + f, + "question {question_id} has already been answered, and the run has already \ + acted on that answer; a second answer would drive it again from a decision \ + nothing recorded" + ), + Self::NoSuchPlan { plan_id } => { + write!(f, "no plan {plan_id} in this store to decide") + } + Self::PlanElsewhere { + plan_id, + owner, + run_id, + } => write!( + f, + "plan {plan_id} was proposed by run {owner}, not by run {run_id}; deciding it \ + here would authorise somebody else's work" + ), + Self::PlanDecided { plan_id } => write!( + f, + "plan {plan_id} has already been decided, and the run has already moved on it" + ), + Self::AttemptNotOpen { attempt_id, run_id } => write!( + f, + "run {run_id} has no open call {attempt_id}: it was never started, belongs to \ + another run, or somebody has already said what happened to it" + ), + Self::NotTheRoot { run_id, root } => write!( + f, + "run {run_id} is a child of run {root}; a limit on how far a fleet may spread \ + belongs to the whole tree, so resume it from {root}" + ), + Self::ContainedRecovery { run_id } => write!( + f, + "run {run_id} is part of a fleet and stopped on a call whose outcome is \ + unknown. This harness has no way to decide that call and keep the fleet's \ + limits, and carrying on without them is not something this release will do \ + quietly." + ), + Self::NoRecordedPolicy { run_id } => write!( + f, + "nothing recorded what run {run_id} was allowed to do, so carrying it on \ + would mean choosing its boundary for you; say which policy it ran under" + ), + Self::HeadMoved { + session_id, + turn_id, + } => write!( + f, + "the run was carried on and turn {turn_id} holds its answer, but this \ + conversation had already moved on somewhere else, so session {session_id} \ + still points where it did. Nothing was lost; the answer is on that turn." + ), + Self::Harness(error) => write!(f, "{error}"), + } + } +} + +impl std::error::Error for Failure { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::Harness(error) => Some(error), + _ => None, + } + } +} + +impl From for Failure { + fn from(error: Error) -> Self { + Self::Harness(error) + } +} + +/// Read what `run_id` stopped on, driving nothing. +/// +/// A handful of store reads and no provider call, so a picker can classify every +/// parked run in a store without spending anything or taking a lease. +/// +/// **The order of the tests is the whole of the correctness here.** An +/// interrupted run is asked about first, because `Ctrl+C` writes the outcome +/// `cancelled` while leaving whatever pending row the run happened to be holding +/// exactly where it was — so a run tested for a question before it is tested for +/// an interruption reads as answerable, and offering it would produce a resume +/// that returns the original outcome having driven nothing. The pending rows come +/// next and the status last, because a run paused on an answered-by-nobody +/// question is recorded with the status `completed`: `Store::finish_run` maps +/// only `awaiting_approval` and `awaiting_plan` to `paused`, so the status alone +/// would report the commonest pause of all as an ended run. +pub fn pending_for(store: &Store, run_id: i64) -> Result { + let Some(status) = store.run_status(run_id)? else { + return Err(Error::Resume { + reason: format!("no run with id {run_id} in the store"), + }); + }; + + // The raw outcome string rather than the status, and they are different + // columns: `Store::status` answers `running`/`paused`/`completed`/`failed` + // and `Store::outcome` answers what the loop actually recorded. Only the + // second can tell a cancelled run from any other completed one. + if store.outcome(run_id)?.as_deref() == Some(CANCELLED) { + return Ok(Pending::Interrupted); + } + + if let Some(question) = unresolved_question(store, run_id)? { + return Ok(Pending::Question { + question_id: question.id, + question: question.question, + context: question.context, + choices: question.choices, + step: question.step, + }); + } + if let Some(plan) = store.plans(run_id)?.into_iter().find(|p| !p.resolved) { + return Ok(Pending::Plan { + plan_id: plan.id, + steps: plan.plan.steps, + step: plan.step, + }); + } + if let Some(attempt) = store.open_attempts(run_id)?.into_iter().next() { + return Ok(Pending::Recovery { + attempt_id: attempt.id, + tool: attempt.tool, + step: attempt.step, + }); + } + + // A run still marked `running` with committed work is one whose process went + // away: the loop writes an outcome on every ending it reaches, including the + // ones it escalates on, so `running` after the fact means it reached none of + // them. With no committed step there is nothing to carry on from either — the + // run is a row and an intention — so it is reported as finished rather than + // offered. + let last_step = store.last_step(run_id)?; + match status { + RunStatus::Running if last_step > 0 => Ok(Pending::Died { last_step }), + _ => Ok(Pending::Finished), + } +} + +/// The prompt the operator typed for the turn `run_id` served, when it served +/// one. +/// +/// The only way to recover a goal from this side of the crate boundary: +/// `runs.goal` has no public reader, so a run that served no session turn cannot +/// have its goal recovered at all and the caller has to supply it. `None` says +/// exactly that rather than substituting something plausible. +pub fn goal_for(store: &Store, run_id: i64) -> Result, Error> { + let Some(turn_id) = store.turn_for_run(run_id)? else { + return Ok(None); + }; + Ok(store.session_turn(turn_id)?.map(|turn| turn.prompt)) +} + +/// Carry on a run whose agent asked the operator something. +/// +/// The answer travels as an argument and is written by the harness's own +/// compare-and-swap on the way in, which is what makes "was it me who answered" +/// answerable. Never write it through `Attach` first: on a run that has already +/// stopped that resolves the row, and the swap inside the resume then finds +/// nothing to answer and fails. +/// +/// `containment` turns this into a tree resume, which is the arm that matters +/// when a *child* asked: io-harness resolves the question against its own run +/// rather than against the root, so a question belonging to any run in the tree +/// is answerable here and one belonging to another tree is refused. +#[allow(clippy::too_many_arguments)] +pub async fn answer_question( + contract: &TaskContract, + provider: &P, + store: &Store, + run_id: i64, + question_id: i64, + answer: &str, + policy: &Policy, + approver: &dyn io_harness::Approver, + containment: Option<&Containment>, + observer: &dyn Observer, + expected_head: Option, +) -> Result { + // The question is checked before the run is, and the order is the difference + // between a useful sentence and a true one. A question somebody already + // answered leaves its run reading as ended — `Store::finish_run` records a + // question pause as `completed` — so a guard that asked about the run first + // would answer "this run has ended" to an operator whose actual problem is + // that their colleague answered it thirty seconds ago. + let question = store + .question(question_id)? + .ok_or(Failure::NoSuchQuestion { question_id })?; + // A flat resume records the answer against `run_id` itself; a tree resume + // looks the question's own run up first, so a child's question is answerable + // from the root. The check follows the same two shapes rather than a single + // stricter one, which would refuse exactly the case the tree arm exists for. + let owned = match containment { + Some(_) => store.run_root(question.run_id)? == run_id, + None => question.run_id == run_id, + }; + if !owned { + return Err(Failure::QuestionElsewhere { + question_id, + owner: question.run_id, + run_id, + }); + } + if question.resolved { + return Err(Failure::QuestionAnswered { question_id }); + } + refuse_ended(store, run_id)?; + root_or_refuse(store, run_id, containment)?; + + let resumed_after = store.last_step(run_id)?; + let result = match containment { + Some(containment) => { + io_harness::resume_tree_with_answer_observed( + contract, + provider, + store, + run_id, + question_id, + answer, + policy, + approver, + containment, + observer, + ) + .await? + } + None => { + io_harness::resume_with_answer_observed( + contract, + provider, + store, + run_id, + question_id, + answer, + policy, + approver, + observer, + ) + .await? + } + }; + finish( + store, + result.run_id, + result.outcome, + resumed_after, + expected_head, + ) +} + +/// Carry on a run whose agent proposed an approach. +/// +/// [`PlanVerdict::Cancel`] goes through this function like the other two and is +/// **not** mapped onto a plain resume. The plan entry point finishes the run as +/// `PlanRejected` without re-entering the loop, which is the difference between +/// "the operator said no" and "the operator said no and the agent was asked to +/// try again anyway" — the second costs the rest of the budget pursuing an +/// approach that was just refused. +/// +/// [`PlanVerdict::Revise`] carries the operator's correction into the run as an +/// observation and the loop continues, so it is a resume in the ordinary sense. +/// +/// Unlike a question, a plan is decided against the run that proposed it even on +/// the tree arm: io-harness records the verdict against the id it is given rather +/// than looking the owner up, so a child's plan cannot be decided from the root. +#[allow(clippy::too_many_arguments)] +pub async fn decide_plan( + contract: &TaskContract, + provider: &P, + store: &Store, + run_id: i64, + plan_id: i64, + verdict: PlanVerdict, + policy: &Policy, + approver: &dyn io_harness::Approver, + containment: Option<&Containment>, + observer: &dyn Observer, + expected_head: Option, +) -> Result { + // Plan first, run second, for the reason [`answer_question`] gives. + let plan = store + .plan(plan_id)? + .ok_or(Failure::NoSuchPlan { plan_id })?; + if plan.run_id != run_id { + return Err(Failure::PlanElsewhere { + plan_id, + owner: plan.run_id, + run_id, + }); + } + if plan.resolved { + return Err(Failure::PlanDecided { plan_id }); + } + refuse_ended(store, run_id)?; + root_or_refuse(store, run_id, containment)?; + + let resumed_after = store.last_step(run_id)?; + let result = match containment { + Some(containment) => { + io_harness::resume_tree_with_plan_decision_observed( + contract, + provider, + store, + run_id, + plan_id, + verdict, + policy, + approver, + containment, + observer, + ) + .await? + } + None => { + io_harness::resume_with_plan_decision_observed( + contract, provider, store, run_id, plan_id, verdict, policy, approver, observer, + ) + .await? + } + }; + finish( + store, + result.run_id, + result.outcome, + resumed_after, + expected_head, + ) +} + +/// Carry on a run that stopped on a call nobody can tell landed or not. +/// +/// [`RecoveryDecision::Completed`] is the one that needs a word. The operator's +/// account of what the call returned is filed by io-harness against the step the +/// *attempt* was made on, not against the step the run has now reached — so the +/// resumed run assembles a transcript in which the tool answered where it was +/// asked, which is the truth. Nothing validates the text: the operator is +/// asserting a fact about the outside world that no code here can check. +/// +/// **`containment` is accepted only to be refused.** See this module's +/// documentation: there is no tree-aware recovery entry point in this harness, +/// and driving a tree root through the flat one would drop the fleet's limits +/// without saying so. The parameter is here rather than absent so that a caller +/// holding a fleet's limits is told they cannot be kept, instead of quietly +/// getting a flat resume from a signature that never offered to take them. +#[allow(clippy::too_many_arguments)] +pub async fn recover( + contract: &TaskContract, + provider: &P, + store: &Store, + run_id: i64, + attempt_id: i64, + decision: RecoveryDecision, + policy: &Policy, + approver: &dyn io_harness::Approver, + containment: Option<&Containment>, + observer: &dyn Observer, + expected_head: Option, +) -> Result { + // A run with a parent is under somebody's containment whether or not this + // caller was holding the value, so both roads to a contained run are refused + // here rather than one of them being driven flat. + if containment.is_some() || store.run_root(run_id)? != run_id { + return Err(Failure::ContainedRecovery { run_id }); + } + if !store + .open_attempts(run_id)? + .iter() + .any(|attempt| attempt.id == attempt_id) + { + return Err(Failure::AttemptNotOpen { attempt_id, run_id }); + } + refuse_ended(store, run_id)?; + + let resumed_after = store.last_step(run_id)?; + let result = io_harness::resume_with_recovery_observed( + contract, provider, store, run_id, attempt_id, decision, policy, approver, observer, + ) + .await?; + finish( + store, + result.run_id, + result.outcome, + resumed_after, + expected_head, + ) +} + +/// Carry on a run whose process went away, with nothing to decide. +/// +/// `policy` is `None` for a run whose boundary this caller cannot rebuild, and +/// that is the case the stored-policy entry point exists for: io-harness reads +/// the policy the run was started under back off its own row. It fails where +/// nothing recorded one — a run written by an older harness — and that failure is +/// reported as [`Failure::NoRecordedPolicy`] rather than as a resume error, +/// because the answer is for the operator to say what the run was allowed to do +/// rather than to try again. +#[allow(clippy::too_many_arguments)] +pub async fn carry_on( + contract: &TaskContract, + provider: &P, + store: &Store, + run_id: i64, + policy: Option<&Policy>, + approver: &dyn io_harness::Approver, + containment: Option<&Containment>, + observer: &dyn Observer, + expected_head: Option, +) -> Result { + refuse_ended(store, run_id)?; + root_or_refuse(store, run_id, containment)?; + if policy.is_none() && store.run_policy(run_id)?.is_none() { + return Err(Failure::NoRecordedPolicy { run_id }); + } + + let resumed_after = store.last_step(run_id)?; + let result = match (containment, policy) { + (Some(containment), Some(policy)) => { + io_harness::resume_tree_observed( + contract, + provider, + store, + run_id, + policy, + approver, + containment, + observer, + ) + .await? + } + (Some(containment), None) => { + io_harness::resume_tree_from_stored_policy_observed( + contract, + provider, + store, + run_id, + approver, + containment, + observer, + ) + .await? + } + (None, Some(policy)) => { + io_harness::resume_with_observed( + contract, provider, store, run_id, policy, approver, observer, + ) + .await? + } + (None, None) => { + io_harness::resume_from_stored_policy_observed( + contract, provider, store, run_id, approver, observer, + ) + .await? + } + }; + finish( + store, + result.run_id, + result.outcome, + resumed_after, + expected_head, + ) +} + +/// The outcome string the run loop writes when an observer stops a run, which is +/// what `Ctrl+C` does. Named once here because it is the difference between a +/// turn that can be carried on and one that cannot, and a literal repeated at +/// each test would be a literal that could drift from the one this module reads. +pub const CANCELLED: &str = "cancelled"; + +/// The first unanswered question of a run, if it holds one. +fn unresolved_question(store: &Store, run_id: i64) -> Result, Error> { + Ok(store.questions(run_id)?.into_iter().find(|q| !q.resolved)) +} + +/// Refuse a run that no resume entry point will drive. +/// +/// Every refusal here is a cheap read and every one of them happens before a +/// lease is taken or a provider is built, so an operator who picks the one turn +/// in their history that cannot be continued pays nothing to find out. +fn refuse_ended(store: &Store, run_id: i64) -> Result<(), Failure> { + // Asked before `pending_for` rather than by reading the error it raises for + // an unknown run: a caller who typed a number wrong should be told that in + // those words, not handed a resume error whose reason they have to parse. + if store.run_status(run_id)?.is_none() { + return Err(Failure::Unknown { run_id }); + } + match pending_for(store, run_id)? { + Pending::Interrupted => Err(Failure::Interrupted { run_id }), + Pending::Finished => Err(Failure::Ended { run_id }), + _ => Ok(()), + } +} + +/// A containment applies to a whole tree, so refuse one offered against a child. +fn root_or_refuse( + store: &Store, + run_id: i64, + containment: Option<&Containment>, +) -> Result<(), Failure> { + if containment.is_none() { + return Ok(()); + } + let root = store.run_root(run_id)?; + if root == run_id { + return Ok(()); + } + Err(Failure::NotTheRoot { run_id, root }) +} + +/// Close the session bookkeeping a free resume does not do. +/// +/// The four drivers all end here, and they end here rather than each doing their +/// own half because every one of the three writes below is a write that only +/// `Session::drive` makes and `Session::drive` is private. A resume that skipped +/// them leaves the turn reading as though it were still waiting: an operator +/// scrolling back sees the question they answered an hour ago still open, and the +/// next turn is parented onto a head that never moved. +/// +/// `None` for the turn is not a failure. A run started by [`crate::exec`] or by +/// any caller that did not go through a `Session` served no turn, and there is +/// nothing to close — which is a fact about that run rather than a problem with +/// this one. +/// +/// **The head write is a compare-and-swap and never the unconditional one.** Two +/// processes on one store both moving a head is a lost update that errors +/// nowhere: the second write wins, and the first process's turn stays in +/// `session_turns` with its parent intact but off the head path — answered, +/// billed, and invisible to the next turn. `set_session_head_if` reports that as +/// `Error::Conflict`, and it is given its own [`Failure::HeadMoved`] here rather +/// than folded into the harness errors, because the two halves of the sentence +/// an operator needs are opposite: the resume worked, and the conversation did +/// not move. +fn finish( + store: &Store, + run_id: i64, + outcome: RunOutcome, + resumed_after: u32, + expected_head: Option, +) -> Result { + let Some(turn_id) = store.turn_for_run(run_id)? else { + return Ok(Resumed { + run_id, + outcome, + resumed_after, + turn_id: None, + reply: None, + }); + }; + + let reply = last_said(&store.step_turns(run_id)?); + // The outcome string the store itself wrote, read back rather than derived + // here from the `RunOutcome` this function was handed. Two spellings of one + // ending is how a transcript and an audit come to disagree about what + // happened, and the store's is the one every other reader sees. `running` for + // a run that paused again: `Store::finish_run` writes no summary until a run + // really ends, which is exactly what a turn waiting for a second answer is. + let recorded = store + .run_summary(run_id)? + .map(|summary| summary.outcome) + .unwrap_or_else(|| "running".to_string()); + store.finish_turn(turn_id, reply.as_deref(), &recorded)?; + + let session_id = store + .session_turn(turn_id)? + .ok_or_else(|| Error::Resume { + reason: format!("run {run_id} names turn {turn_id}, which the store does not hold"), + })? + .session_id; + match store.set_session_head_if(session_id, expected_head, Some(turn_id)) { + Ok(()) => {} + Err(Error::Conflict { .. }) => { + return Err(Failure::HeadMoved { + session_id, + turn_id, + }) + } + Err(error) => return Err(Failure::Harness(error)), + } + + Ok(Resumed { + run_id, + outcome, + resumed_after, + turn_id: Some(turn_id), + reply, + }) +} + +/// The last thing the agent wrote, taken off the run's own recorded turns. +/// +/// **Read from the store and never reconstructed.** io-harness extracts the same +/// text by splitting an observation on a sentinel that is `pub(crate)`, so the +/// only way to reproduce it here would be to hardcode the literal — a string this +/// crate cannot see change, in a function that would go on returning a plausible +/// answer for years after it stopped being the right one. +/// +/// Scanning backwards for the last turn that *said* something, rather than taking +/// the last turn and accepting its `None`, matches what io-harness's own +/// extraction does: it walks the ledger in reverse for the last message. A +/// resumed run whose final step was a tool call therefore reports the last thing +/// the agent actually wrote, which is what an operator reading the turn expects +/// to find there. +fn last_said(turns: &[AssistantTurn]) -> Option { + turns.iter().rev().find_map(|turn| { + let said = turn.text.as_deref()?.trim(); + (!said.is_empty()).then(|| said.to_string()) + }) +} diff --git a/src/rewind.rs b/src/rewind.rs index ab2ac73..416cbce 100644 --- a/src/rewind.rs +++ b/src/rewind.rs @@ -138,7 +138,7 @@ pub fn preview(session: &Session, store: &Store) -> Option { /// tried this three times" unanswerable, and would make the ledger disagree with /// the invoice. /// -/// # Why the head is moved through the store and the session re-opened +/// # Why the head is written conditionally, and the session re-opened /// /// The obvious way to move the head back is `Session::branch_from`, which is what /// the interface already uses to leave a turn behind. It cannot express this @@ -148,8 +148,27 @@ pub fn preview(session: &Session, store: &Store) -> Option { /// undone turn's id would leave the head on the turn whose work has just been /// deleted, which is the half-undo this module exists to avoid. /// -/// So the head is written directly, `store.set_session_head(id, parent)?`, where -/// `parent` is the undone turn's `parent_turn_id` and is `None` for a first +/// **That argument is about `branch_from`, and it was once read as licence for +/// the unconditional `Store::set_session_head`. It never covered that.** One +/// store serves this whole machine, so two `io` processes in two terminals is +/// ordinary, and an unconditional write is a lost update: the other process's +/// turn stays in `session_turns`, correctly parented, answered and paid for, but +/// off the head path — so `Session::history`, which walks parent links back from +/// the head, never shows it again. Nothing errors and nothing reaches the screen. +/// +/// `Store::set_session_head_if(id, expected, new)` is the same write with the +/// head the caller believed it was replacing, and it takes an `Option` on *both* +/// sides — so unlike `branch_from` it says "there is no turn" perfectly well, and +/// the case that motivated the direct write is expressible after all. The head +/// this function read at the top is what it passes as `expected`; if the store no +/// longer holds it the update matches no row and the store returns +/// `Error::Conflict` naming the session. That is returned as it stands: **the +/// undo is refused rather than retried or forced.** Which turn should survive is +/// the operator's call and not this function's, and the losing turn is only +/// recoverable while it is still on somebody's head. +/// +/// So the head is written as `store.set_session_head_if(id, Some(head), parent)?`, +/// where `parent` is the undone turn's `parent_turn_id` and is `None` for a first /// turn. The session is then re-read with `Session::reopen(store, id)?` rather /// than being patched in place, because `Session` caches its head in a private /// field that no public setter reaches: a session left holding the old value @@ -205,7 +224,10 @@ pub fn last_turn(session: &mut Session, store: &Store) -> Result, } let id = session.id(); - store.set_session_head(id, turn.parent_turn_id)?; + // `Some(head)` — the head this function read and built the whole undo from. + // A store that no longer holds it has had another `io` move the conversation + // on, and the conflict goes back to the caller unchanged: no retry, no force. + store.set_session_head_if(id, Some(head), turn.parent_turn_id)?; *session = Session::reopen(store, id)?; Ok(Some(Undone { diff --git a/src/sessions.rs b/src/sessions.rs index 054be58..9700612 100644 --- a/src/sessions.rs +++ b/src/sessions.rs @@ -14,18 +14,38 @@ //! binary opens a session, and keeping session state is what this product's first //! constraint forbids. Everything here is a read, and this module cannot reach //! the filesystem at all — it takes a `&Store` and nothing else. +//! +//! **Every row also says what its session stopped on.** The walk already knows +//! each session's newest run — it is the run that put that session where it is in +//! the list — and [`crate::resume::pending_for`] turns that id into the state the +//! run is holding: a question, a plan, an interrupted call, a process that died, +//! a turn the operator ended, or nothing at all. That state is the fact `/resume` +//! is chosen *on*, so it is drawn as a [`Row::mark`] rather than folded into the +//! label the matcher ranks or the detail a narrow terminal drops. Reading it is +//! the whole of what this module does with it; deciding anything about it belongs +//! to [`crate::resume`], and nothing here drives. use io_harness::Store; use crate::glyphs::Glyphs; use crate::picker::{fit, fit_left, Row}; +use crate::resume::{pending_for, Pending}; /// How many run ids the walk will look at before giving up. /// -/// The only bound left, and it bounds *cost* rather than the length of the list: -/// a session's runs are contiguous only if nothing else ran in between, so a -/// hundred sessions can sit behind far more than a hundred runs. Five hundred -/// point queries on an indexed column is the ceiling this release accepts. +/// The only bound left, and it bounds the **scan** rather than the length of the +/// list: a session's runs are contiguous only if nothing else ran in between, so +/// a hundred sessions can sit behind far more than a hundred runs. +/// +/// **What it costs is no longer five hundred queries, and this said so until +/// 0.23.0.** The walk itself is two point queries per run scanned. Every distinct +/// session it finds then costs three more to describe it, and since 0.23.0 a +/// further handful for [`crate::resume::pending_for`] to say what its last run +/// stopped on — the run's status and outcome, and its pending questions, plans +/// and open attempts. So a store whose newest five hundred runs belong to five +/// hundred *different* sessions issues thousands of point queries on one +/// keystroke, not five hundred. They are all indexed and all synchronous, and the +/// honest statement of the ceiling is the scan, which is what this bounds. /// /// There was a second bound until 0.7.0 — twenty sessions, and its own comment /// said it existed because a picker that could not be typed at was a list nobody @@ -41,6 +61,38 @@ pub const MAX_RUNS_SCANNED: usize = 500; /// How much of a workspace path a row keeps before the picker fits it again. const ROOT_ROOM: usize = 30; +/// The mark on a session holding a question the operator has not answered. +/// +/// **Words rather than symbols, and ASCII by rule.** A mark is text and not a +/// colour, so it survives `NO_COLOR`, `--plain` and the ASCII glyph set +/// unchanged — the same rule `crate::commands::PINNED_MARK` and its neighbours +/// are held to, and `tests/glyphs.rs` sweeps for. Those marks are one cell each +/// because they sit on *every* row of their page and a varying width would move +/// the label column; these do not. A finished session carries no mark at all, so +/// a `/resume` list is ragged by construction and there is no column to keep — +/// which buys a mark that says *which* state it is, to an operator who has no +/// legend on screen and is about to choose on it. +pub const QUESTION_MARK: &str = "asks"; + +/// The mark on a session holding a plan waiting for a verdict. +pub const PLAN_MARK: &str = "plan"; + +/// The mark on a session whose last run has a tool call nobody has ruled on: +/// io-harness could not establish whether it landed, and will not guess. +pub const RECOVERY_MARK: &str = "tool"; + +/// The mark on a session whose last run was never closed — the process died +/// mid-loop — and which is therefore resumable from its last committed step. +pub const DIED_MARK: &str = "died"; + +/// The mark on a session whose last turn the **operator** stopped. +/// +/// Not a resume target and drawn as one only over this product's dead body: a +/// cancelled run is recorded `completed`, every io-harness resume entry point +/// short-circuits on it, and a row that offered to continue it would be offering +/// something that cannot happen. [`note`] says so in words and names `/fork`. +pub const ENDED_MARK: &str = "ended"; + /// One session, as `/resume` shows it. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Recent { @@ -54,6 +106,14 @@ pub struct Recent { pub prompt: String, /// When its newest turn was recorded, as the store wrote it. pub at: String, + /// What the session's **newest run** stopped on. + /// + /// The newest run and not the newest turn: a turn is closed by the driver + /// and says nothing about a run that paused, and it is the run that holds the + /// question, the plan or the open call. Read through + /// [`crate::resume::pending_for`], which drives nothing — this is a list, and + /// building it must not answer anything. + pub pending: Pending, } /// Every session in this store that ran a turn, newest first. @@ -66,7 +126,12 @@ pub struct Recent { /// That is correct rather than a gap: there is nothing in it to resume. pub fn recent(store: &Store) -> Result<(Vec, bool), io_harness::Error> { let runs = store.runs()?; - let mut ids: Vec = Vec::new(); + // Each session paired with the newest run that served it, which costs + // nothing: `Store::runs` is newest-first, so the run that first mentions a + // session **is** that session's newest, and the dedupe below is already + // throwing every older one away. Re-deriving it afterwards would mean a + // second walk to find a number this one has already had in its hand. + let mut ids: Vec<(i64, i64)> = Vec::new(); let mut cut = false; for (scanned, run) in runs.iter().enumerate() { @@ -80,17 +145,17 @@ pub fn recent(store: &Store) -> Result<(Vec, bool), io_harness::Error> { let Some(turn) = store.session_turn(turn_id)? else { continue; }; - if ids.contains(&turn.session_id) { + if ids.iter().any(|(session, _)| *session == turn.session_id) { continue; } - ids.push(turn.session_id); + ids.push((turn.session_id, *run)); } let mut out = Vec::with_capacity(ids.len()); - for id in ids { - // Three queries per row, and none at all for a run the walk never - // reached — which is what splitting the walk from this loop buys, and why - // the scan ceiling is charged above rather than here. + for (id, newest_run) in ids { + // Three queries per row and the state read below, and none at all for a + // run the walk never reached — which is what splitting the walk from this + // loop buys, and why the scan ceiling is charged above rather than here. let Some(root) = store.session_root(id)? else { continue; }; @@ -104,6 +169,13 @@ pub fn recent(store: &Store) -> Result<(Vec, bool), io_harness::Error> { turns: turns.len(), prompt: first.prompt.clone(), at: stamp(&turns[turns.len() - 1].created_at), + // The read that makes the list worth having: without it every row + // looks alike, and the session actually waiting on an answer is + // indistinguishable from the twenty that finished. Charged per **row** + // rather than per run scanned, so the ceiling above still bounds the + // walk — and taken on the newest run, which the walk above kept for + // exactly this. + pending: pending_for(store, newest_run)?, }); } Ok((out, cut)) @@ -161,6 +233,63 @@ fn stamp(created_at: &str) -> String { cut.replace('T', " ") } +/// The mark a session's row carries, or `None` for one that finished. +/// +/// **`None` is the whole of how a finished session is reported, and that is +/// deliberate.** A mark saying *fine* would be on almost every row of almost +/// every list, and a mark that is nearly always there is one nobody reads — the +/// same argument [`cut_note`] is built on. The three states an operator has to +/// act on, and the two they have to know about, are the ones that get a mark; +/// everything else is the absence of one. +pub fn mark(pending: &Pending) -> Option<&'static str> { + match pending { + Pending::Question { .. } => Some(QUESTION_MARK), + Pending::Plan { .. } => Some(PLAN_MARK), + Pending::Recovery { .. } => Some(RECOVERY_MARK), + Pending::Died { .. } => Some(DIED_MARK), + Pending::Interrupted => Some(ENDED_MARK), + Pending::Finished => None, + } +} + +/// The same state as a sentence, for a surface with room for prose. +/// +/// A second rendering rather than a longer mark: a picker row has four columns +/// to spare and a status line or a `/resume` confirmation has a line, and the +/// state is worth saying properly wherever there is room to say it. `None` +/// wherever [`mark`] is `None`, so the two agree about what is worth reporting +/// and a finished session is silent on both. +/// +/// **The interrupted sentence is the one that has to be read carefully.** A turn +/// the operator stopped cannot be resumed — io-harness records it `completed` +/// and every resume entry point returns the original outcome without driving — +/// so this says who ended it and points at `/fork` from the turn before, which +/// is the neighbouring thing that *does* work. It must never read as an offer to +/// continue. +pub fn note(pending: &Pending) -> Option { + Some(match pending { + Pending::Question { question, .. } => { + format!("waiting on your answer: {question}") + } + Pending::Plan { steps, .. } => format!( + "waiting on your verdict on a plan of {} step{}", + steps.len(), + if steps.len() == 1 { "" } else { "s" } + ), + Pending::Recovery { tool, .. } => format!( + "waiting on your decision about an interrupted {tool} call — io-harness \ + cannot tell whether it landed" + ), + Pending::Died { last_step } => { + format!("the process died after step {last_step}; resumable from there") + } + Pending::Interrupted => "you ended this turn, and an ended turn cannot be \ + continued — /fork from the turn before it to carry on" + .to_string(), + Pending::Finished => return None, + }) +} + /// The picker rows, fitted for a terminal this wide. /// /// Content precedes metadata, so the label is what the session was asked to do. @@ -179,6 +308,19 @@ fn stamp(created_at: &str) -> String { /// only if the whole of it fits, in order of how much the operator needs it. A /// narrow terminal therefore loses the timestamp entirely, which is legible, /// instead of keeping a fragment of it, which is not. +/// +/// **The state goes on [`Row::mark`], and neither in the label nor in the +/// detail.** Not the label, because [`crate::picker::Picker`] ranks the label +/// and nothing else: a state folded into it would give every waiting session the +/// same first characters, under which no query is a prefix of a row and both of +/// `crate::fuzzy`'s top tiers stop being reachable — and it would also let a +/// query for `plan` return the sessions that are *stopped on* a plan rather than +/// the ones that were asked about one. Not the detail, because the detail is the +/// first thing the picker drops when the terminal is narrow, and the state is +/// precisely what an operator on a narrow terminal still has to see: 0.16.0 +/// marked a template and a skill in the detail alone and the distinction vanished +/// at the width that needed it most. The mark column is the one that survives +/// both. pub fn rows(recent: &[Recent], width: u16, glyphs: &Glyphs) -> Vec { // A third rather than a half. The label is the prompt, which is unbounded, and // every column it takes is one the workspace cannot have. @@ -188,13 +330,17 @@ pub fn rows(recent: &[Recent], width: u16, glyphs: &Glyphs) -> Vec { .iter() .map(|session| { let label = fit(&session.prompt, room, glyphs); + let kind = mark(&session.pending); // The picker's own arithmetic, mirrored: two cells of marker, the - // label, two cells of gap. Mirrored rather than guessed, because a - // budget that is one cell out puts the ellipsis back. Four in either - // glyph set — the marker is two cells in both, which is the property - // that lets this stay a number. + // mark and the space after it on a row that has one, the label, two + // cells of gap. Mirrored rather than guessed, because a budget that is + // one cell out puts the ellipsis back. Four in either glyph set — the + // marker is two cells in both, which is the property that lets that + // half stay a number; the mark's own width is counted rather than + // assumed, for the reason `picker::fit` measures the ellipsis. let mut left = (width as usize) .saturating_sub(4) + .saturating_sub(kind.map_or(0, |mark| mark.chars().count() + 1)) .saturating_sub(label.chars().count()); let path = fit_left(&session.root, ROOT_ROOM.min(left), glyphs); @@ -216,7 +362,10 @@ pub fn rows(recent: &[Recent], width: u16, glyphs: &Glyphs) -> Vec { detail.push_str(&at); } - Row::with_detail(label, detail) + match kind { + Some(mark) => Row::marked(mark, label, detail), + None => Row::with_detail(label, detail), + } }) .collect() } diff --git a/tests/commands.rs b/tests/commands.rs index 1682024..e6dc28e 100644 --- a/tests/commands.rs +++ b/tests/commands.rs @@ -225,6 +225,50 @@ fn f10_status_is_listed_as_a_command_and_resolves_to_its_own_action() { ); } +#[test] +fn f12_resume_says_it_answers_a_parked_run_rather_than_merely_reopening_a_session() { + // **The sabotage this defends against is changing what `/resume` does and + // leaving its one line saying what it used to do.** That line is the only + // thing most operators ever read about a command: it is the palette row, it + // is what `/help` prints, and `tests/docs.rs` copies it into the README + // table. A description that still promised a reopen would leave the whole of + // 0.23.0 undiscoverable from inside the product. + let (_, said) = COMMANDS + .iter() + .find(|(name, _)| *name == "/resume") + .expect("/resume is listed"); + assert!( + said.contains("answer"), + "0.23.0 made /resume answer what a run stopped on; the description still \ + describes 0.22.0's: {said}", + ); + // And it is still a reopen as well — the description must not have swung the + // other way and dropped the half that is true for the four sessions in five + // that are waiting on nothing at all. + assert!( + said.contains("session"), + "most sessions have nothing parked, and reopening one is still what the \ + command does for them: {said}", + ); + // Unchanged: the word still resolves, `/continue` is still the same action, + // and neither has become the help listing. + assert_eq!( + commands::parse("resume", &defaults(), &DARK), + Action::Resume, + ); + assert_eq!( + commands::parse("continue", &defaults(), &DARK), + Action::Resume, + ); + // No command was added for any of this — `/resume` was extended — so the + // inventory is the size the other gates in this file assert. + assert_eq!( + COMMANDS.len(), + 30, + "0.23.0 adds no command; a thirty-first here means one arrived unrecorded", + ); +} + #[test] fn an_unknown_command_says_what_does_exist() { let Action::Print(lines) = commands::parse("models", &defaults(), &DARK) else { diff --git a/tests/contract.rs b/tests/contract.rs index 1413cc4..2613b43 100644 --- a/tests/contract.rs +++ b/tests/contract.rs @@ -759,17 +759,42 @@ fn f6_both_arms_are_handed_one_contract() { 2, "`/status` and `/context` each read one contract to report with, and neither builds a turn", ); + // **A third kind of site, added in 0.23.0, and it is neither of the two + // above.** `resume_pending` takes a run — so it is not a reading site — but + // it does not build a turn either: it continues one io-harness already has, + // from the step it stopped at. It goes through this builder for the same + // reason the reading sites do: `contract::session` is the only call that + // merges the `[app.io-cli]` scope and resolves the step-cap precedence, and a + // resumed run that quietly dropped the MCP servers or the roster would behave + // differently after the pause than before it, which is worse than not + // resuming at all. + // + // Named and asserted once rather than folded into the count above, exactly as + // that assertion's own comment prescribes: counting to two there would admit a + // genuine second turn arm, which is the failure it exists to make + // unrepresentable. + assert_eq!( + text.matches("let continuing = io_cli::contract::session(") + .count(), + 1, + "one contract is rebuilt for a resume, and only `resume_pending` rebuilds one", + ); assert_eq!( text.matches("let opening = io_cli::contract::session(") .count(), 1, "the session reads one contract at startup to put the ceilings on the line, and runs none", ); + // **Five since 0.23.0, and the fifth is the resume.** The total is checked + // beside the four named counts rather than instead of them, so a site that + // arrived without a name of its own still fails here even though every named + // assertion above would pass. That is the whole value of keeping both: the + // named counts say what each site is, and this one says nothing else exists. assert_eq!( text.matches("io_cli::contract::session(").count(), - 4, - "the turn's contract, the startup reading and the two reporting pages are the only four, \ - so a fifth is a new arm", + 5, + "the turn's contract, the resume's, the startup reading and the two reporting pages are \ + the only five, so a sixth is a new arm", ); assert!( !text.contains("with_responder") && !text.contains("with_plan_gate"), diff --git a/tests/docs.rs b/tests/docs.rs index bb70725..2223b21 100644 --- a/tests/docs.rs +++ b/tests/docs.rs @@ -891,3 +891,106 @@ fn the_readme_lists_every_flag_io_exec_actually_takes() { ); } } + +/// The same gate for `io resume`, which is 0.23.0's subcommand and has three +/// times as many flags to go stale. +/// +/// The absent flag is asserted as well, and it is not symmetry for its own sake: +/// the README states in words that there is deliberately no `--sandbox` here, +/// because a resumed run already started under a boundary and widening it halfway +/// through is a widening nobody asked for at the point nobody is watching. A +/// `--sandbox` added later would leave that paragraph standing and wrong, which is +/// precisely the failure this file exists for. +#[test] +fn the_readme_documents_every_flag_io_resume_actually_takes() { + use clap::CommandFactory; + + let readme = read("README.md"); + let cli = io_cli::cli::Cli::command(); + let resume = cli + .get_subcommands() + .find(|sub| sub.get_name() == "resume") + .expect("`io resume` is a subcommand"); + + let flags: Vec = resume + .get_arguments() + .filter_map(|arg| arg.get_long().map(|long| format!("--{long}"))) + .filter(|long| long != "--help") + .collect(); + + assert!(!flags.is_empty(), "there should be flags to check"); + for flag in &flags { + assert!( + readme.contains(&format!("`{flag}")), + "`io resume` takes {flag} and the README does not mention it", + ); + } + + // Each pause's own input, because a table that lost one of these would leave + // an operator with a run they can see and cannot decide. + for promised in ["--list", "--answer", "--plan", "--recovery", "--goal"] { + assert!( + flags.iter().any(|flag| flag == promised), + "the README documents {promised} and `io resume` no longer takes it", + ); + } + assert!( + !flags.iter().any(|flag| flag == "--sandbox"), + "the README says `io resume` takes no --sandbox, and it now does", + ); +} + +/// The pause that cannot be resumed, said where an operator is deciding what to +/// do about one. +/// +/// **A gate on an absence would pass on a page that says nothing**, which is why +/// this reads the section rather than sweeping the file: a turn the operator +/// interrupted is recorded `cancelled`, mapped to a *completed* run, and every +/// io-harness resume entry point hands back the original outcome having driven +/// nothing. An operator who is not told that reads a `/resume` row for such a +/// session as a row they can choose, and the release's own answer — `/fork` from +/// the turn before — is the thing they never find. +/// +/// The marks are taken from [`io_cli::sessions`] rather than listed here, for the +/// reason the command table is generated: a sixth state, or a renamed one, fails +/// this test instead of leaving the README teaching a word that is no longer +/// drawn. +#[test] +fn the_readme_says_which_pause_cannot_be_resumed() { + use io_cli::sessions::{DIED_MARK, ENDED_MARK, PLAN_MARK, QUESTION_MARK, RECOVERY_MARK}; + + let readme = read("README.md"); + let (_, section) = readme + .split_once("## When a run stops for you") + .expect("the README should have a section about a run that stopped for a person"); + let section = section.split("\n## ").next().unwrap_or(section); + + for mark in [ + QUESTION_MARK, + PLAN_MARK, + RECOVERY_MARK, + DIED_MARK, + ENDED_MARK, + ] { + assert!( + section.contains(&format!("`{mark}`")), + "the README should name the `{mark}` mark and say what it means", + ); + } + + // The mechanism, then the consequence, then what to do instead. Short + // distinctive phrases rather than sentences, because the sentence around a + // claim is what an author rewrites while leaving the claim standing. + assert!( + section.contains("cancelled"), + "the README should say what io-harness records a Ctrl+C as", + ); + assert!( + section.contains("cannot be answered"), + "the README should say an interrupted turn is the one pause that cannot be resumed", + ); + assert!( + section.contains("`/fork`"), + "the README should offer /fork, which is what an ended turn leaves you", + ); +} diff --git a/tests/exec.rs b/tests/exec.rs index 92474ba..69975c3 100644 --- a/tests/exec.rs +++ b/tests/exec.rs @@ -11,6 +11,10 @@ //! N1 — no interface code on the headless path. //! N6 — the plain output is not composed to a width. //! +//! 0.23.0 F8 — `io resume`: the parked runs are listable, one is resumable by id, +//! the parked line names the id that addresses the pause, and the two pauses that +//! cannot be carried on are refused in words rather than by a failed drive. +//! //! The two mappings that look like taste are the release's research. A clean //! headless run ends in `RunOutcome::Finished` and never `Success`, because the //! contract this subcommand builds carries no verification criterion; and every @@ -116,10 +120,13 @@ async fn f1_a_clean_run_finishes_rather_than_succeeding() { #[test] fn f2_every_outcome_maps_to_its_documented_code() { - // All fifteen variants, written out rather than sampled. `RunOutcome` is not - // `#[non_exhaustive]` and `exec::code` has no `_` arm, so a variant added by a - // later harness breaks the build of both — which is the property that keeps a - // published table true across a pin bump. + // Fifteen variants, written out rather than sampled — `AwaitingRecovery` is + // the sixteenth and has a test of its own below. It used to be true that this + // list could not go stale: `RunOutcome` was exhaustive and `exec::code` had no + // `_` arm, so a variant a later harness added broke the build of both. io-harness + // 0.65 made the enum `#[non_exhaustive]` and took that away, which is why + // `the_outcome_table_names_every_outcome_the_locked_harness_declares` reads the + // variants out of the locked source instead. let cases: &[(RunOutcome, u8)] = &[ (RunOutcome::Success { steps: 3 }, exec::OK), (RunOutcome::Finished { steps: 3 }, exec::OK), @@ -197,11 +204,6 @@ fn f2_a_paused_run_names_what_was_parked_and_nothing_else_does() { ) .expect("a paused run names what was parked"); assert!(waiting.contains("41"), "{waiting}"); - assert!( - waiting.contains("not in this release"), - "it must say that answering it is not possible yet, rather than implying \ - the run is lost: {waiting}", - ); assert!(exec::parked( &RunOutcome::AwaitingPlan { @@ -219,6 +221,93 @@ fn f2_a_paused_run_names_what_was_parked_and_nothing_else_does() { assert_eq!(exec::parked(&RunOutcome::Cancelled { steps: 3 }, 7), None); } +/// 0.23.0 F8 — the parked line names the id that addresses the pause, and the +/// invocation that acts on it. +/// +/// **The run id is not that id, and until this release the line printed only the +/// run id.** Every resume entry point takes a second number — the question, the +/// plan, the journalled call — and `parked` was handed all three inside the +/// outcome and threw each of them away. So the assertions below are on the +/// *specific* id and not on the line having changed: a rewording that still drops +/// the number would pass the second and fail the first, which is the whole point. +#[test] +fn a_parked_run_names_the_id_that_addresses_its_pause_and_the_way_to_act_on_it() { + let answer = exec::parked( + &RunOutcome::AwaitingAnswer { + question_id: 219, + steps: 8, + }, + 41, + ) + .expect("a question pause names its question"); + assert!( + answer.contains("219"), + "the question id is what `resume::answer_question` takes, and the run id \ + does not address it: {answer}", + ); + assert!(answer.contains("io resume 41 --answer"), "{answer}"); + + let plan = exec::parked( + &RunOutcome::AwaitingPlan { + plan_id: 307, + steps: 2, + }, + 41, + ) + .expect("a plan pause names its plan"); + assert!(plan.contains("307"), "{plan}"); + assert!(plan.contains("io resume 41 --plan"), "{plan}"); + + let recovery = exec::parked( + &RunOutcome::AwaitingRecovery { + attempt_id: 512, + steps: 3, + }, + 41, + ) + .expect("a recovery pause names its attempt"); + assert!(recovery.contains("512"), "{recovery}"); + assert!(recovery.contains("io resume 41 --recovery"), "{recovery}"); + + // The fourth pause is the one with no `io resume` behind it. It still names + // its request, and it deliberately does not offer an invocation that does not + // exist: an approval is answered by the person the run asked. + let approval = exec::parked( + &RunOutcome::AwaitingApproval { + request_id: 88, + steps: 1, + }, + 41, + ) + .expect("an approval pause is still a pause"); + assert!(approval.contains("88"), "{approval}"); + assert!( + !approval.contains("io resume 41 --"), + "there is no resume entry point for an approval; offering one would send \ + an operator to a flag that cannot exist: {approval}", + ); +} + +/// 0.23.0 F8 — the six exit codes were pre-numbered for this subcommand, and it +/// moved none of them. +#[test] +fn adding_the_resume_subcommand_moved_no_exit_code() { + // 0.13.0 mapped the three pauses to 4 while there was nothing that could + // answer one, and said in `src/exec.rs` that it did so precisely to keep this + // release from renumbering anything. This is that claim, asserted. + assert_eq!( + ( + exec::OK, + exec::FAILED, + exec::REFUSED, + exec::CEILING, + exec::PAUSED, + exec::UNFINISHED, + ), + (0, 1, 2, 3, 4, 5), + ); +} + #[test] fn f2_every_outcome_describes_itself_with_the_harness_step_count() { // The summary line names the harness's own number rather than one recounted @@ -891,6 +980,25 @@ fn n1_the_headless_path_is_chosen_before_the_wizard_can_be_reached() { "`io exec` must leave before the non-TTY refusal, which is what it is \ the answer to rather than a victim of", ); + + // **`io resume` owes exactly the same two orderings, and for the same two + // reasons.** It is headless, so a non-TTY stdout is what it is for rather + // than something to be refused for; and it answers a missing provider with a + // sentence of its own, so reaching the wizard would sit a container at a + // prompt nobody can answer. Added with the subcommand in 0.23.0 — the gate + // above had covered only the arm that existed when it was written, which is + // how an ordering rule stops covering the next thing that needs it. + let resume_arm = main + .find("Subcommand::Resume(args)") + .expect("main routes the resume subcommand"); + assert!( + resume_arm < wizard, + "`io resume` must leave before the wizard can be reached", + ); + assert!( + resume_arm < refusal, + "`io resume` must leave before the non-TTY refusal", + ); } #[test] @@ -1198,3 +1306,530 @@ fn f6_a_configuration_that_cannot_be_read_still_says_what_moved() { "a run that never started wrote to the JSON stream: {stdout:?}", ); } + +// --------------------------------------------------------------------------- +// 0.23.0 F8 — `io resume`. +// +// **Nothing below drives a run, and that is deliberate.** Every one of the four +// resume entry points takes a `Provider` and spends on it, so the property "the +// answer reached the agent and the run carried on" cannot be established here +// without a live model. It belongs to the live suite, which runs against a real +// provider with a real key; what is asserted here is everything that happens +// *before* the provider is built — the parse, the classification, the two +// refusals and the row an operator reads — which is where every mistake this +// subcommand can make cheaply is made. + +/// The `Resume` args a command line parses to, so the flag wiring is under test +/// rather than assumed by a struct literal. +fn resume_args(argv: &[&str]) -> io_cli::cli::Resume { + use clap::Parser; + + let cli = io_cli::cli::Cli::try_parse_from(argv) + .unwrap_or_else(|error| panic!("{argv:?} should parse\n{error}")); + match cli.command { + Some(io_cli::cli::Command::Resume(args)) => args, + other => panic!("{argv:?} should be a resume command, got {other:?}"), + } +} + +/// 0.23.0 F8 — every form the subcommand documents parses, on either side of the +/// subcommand for the flags that are `global`. +#[test] +fn the_resume_subcommand_parses_in_every_form_it_offers() { + use clap::Parser; + + assert!(resume_args(&["io", "resume", "--list"]).list); + assert_eq!( + resume_args(&["io", "resume", "41", "--answer", "the blue one"]).answer, + Some("the blue one".to_string()), + ); + assert_eq!( + resume_args(&["io", "resume", "41", "--plan", "approve"]).plan, + Some(io_cli::cli::PlanFlag::Approve), + ); + let revised = resume_args(&[ + "io", + "resume", + "41", + "--plan", + "revise", + "--correction", + "start with the tests", + ]); + assert_eq!(revised.plan, Some(io_cli::cli::PlanFlag::Revise)); + assert_eq!(revised.correction.as_deref(), Some("start with the tests")); + let recovered = resume_args(&[ + "io", + "resume", + "41", + "--recovery", + "completed", + "--account", + "charge ch_9f21 captured", + ]); + assert_eq!( + recovered.recovery, + Some(io_cli::cli::RecoveryFlag::Completed) + ); + assert_eq!( + recovered.account.as_deref(), + Some("charge ch_9f21 captured") + ); + assert_eq!( + resume_args(&["io", "resume", "41", "--recovery", "abandon"]).recovery, + Some(io_cli::cli::RecoveryFlag::Abandon), + ); + // A run whose process merely died needs nothing at all. + let bare = resume_args(&["io", "resume", "41"]); + assert_eq!(bare.run, Some(41)); + assert!(bare.answer.is_none() && bare.plan.is_none() && bare.recovery.is_none()); + // The three flags a headless run already takes, meaning the same three things. + let shaped = resume_args(&[ + "io", + "resume", + "41", + "--json", + "--policy", + "read-only", + "--provider", + "openrouter", + ]); + assert!(shaped.json); + assert_eq!(shaped.policy, Some(PolicyFlag::ReadOnly)); + assert_eq!(shaped.provider, Some(FromEnv::OpenRouter)); + + // The global flags, on both sides. 0.5.0 shipped a defect where `-m` after + // the subcommand was rejected, and a new subcommand is exactly where that + // comes back. + for argv in [ + vec!["io", "-m", "a-model", "resume", "--list"], + vec!["io", "resume", "--list", "-m", "a-model"], + ] { + let cli = io_cli::cli::Cli::try_parse_from(&argv) + .unwrap_or_else(|error| panic!("{argv:?} should parse\n{error}")); + assert_eq!(cli.model.as_deref(), Some("a-model"), "{argv:?}"); + assert!(matches!(cli.command, Some(io_cli::cli::Command::Resume(_)))); + } + for argv in [ + vec!["io", "-C", "/tmp/x", "resume", "41", "--answer", "yes"], + vec!["io", "resume", "41", "--answer", "yes", "-C", "/tmp/x"], + ] { + let cli = io_cli::cli::Cli::try_parse_from(&argv) + .unwrap_or_else(|error| panic!("{argv:?} should parse\n{error}")); + assert_eq!(cli.dir.as_deref(), Some(std::path::Path::new("/tmp/x"))); + } + for argv in [ + vec!["io", "--plain", "resume", "--list"], + vec!["io", "resume", "--list", "--plain"], + vec!["io", "--profile", "ci", "resume", "--list"], + vec!["io", "resume", "--list", "--profile", "ci"], + ] { + assert!( + io_cli::cli::Cli::try_parse_from(&argv).is_ok(), + "{argv:?} should parse", + ); + } + + // And the shapes clap itself refuses, which are the ones no code below would + // otherwise have to think about. + for argv in [ + // Neither an id nor a listing. + vec!["io", "resume"], + // A listing of one run is not a thing. + vec!["io", "resume", "--list", "41"], + // A correction with no plan to correct, and an account with no call. + vec!["io", "resume", "41", "--correction", "do it differently"], + vec!["io", "resume", "41", "--account", "it landed"], + ] { + assert!( + io_cli::cli::Cli::try_parse_from(&argv).is_err(), + "{argv:?} should not parse", + ); + } +} + +/// 0.23.0 F8 — a turn the operator stopped is finished, not paused, and the +/// refusal points at `/fork` exactly as the interactive surface does. +#[test] +fn an_interrupted_turn_is_refused_and_offered_fork_from_the_turn_before() { + // `Ctrl+C` returns `Flow::Cancel`, the loop records `cancelled`, `finish_run` + // maps that to a *completed* status, and every resume entry point + // short-circuits on a completed run and hands back the original outcome + // having driven nothing. So the commonest way an io-cli turn stops is the one + // way it cannot be continued, and an operator who types the id anyway must be + // told that in words rather than by a resume that silently did nothing. + let refusal = exec::decision_for( + 41, + &io_cli::resume::Pending::Interrupted, + &resume_args(&["io", "resume", "41"]), + ) + .expect_err("an interrupted turn cannot be carried on"); + + assert!(refusal.contains("41"), "{refusal}"); + assert!( + refusal.contains("/fork"), + "the refusal must offer the honest neighbouring answer rather than only \ + decline: {refusal}", + ); + assert!( + refusal.contains("stopped by you"), + "it must say the operator ended it, not that something went wrong: {refusal}", + ); + + // The same sentence `crate::resume` gives, rather than a second wording of it. + assert_eq!( + refusal, + io_cli::resume::Failure::Interrupted { run_id: 41 }.to_string(), + ); + + // A run that simply ended is refused too, and says something different. + let ended = exec::decision_for( + 41, + &io_cli::resume::Pending::Finished, + &resume_args(&["io", "resume", "41"]), + ) + .expect_err("an ended run has nothing to carry on"); + assert!(ended.contains("has ended"), "{ended}"); +} + +/// 0.23.0 F8 — a bare run has no recoverable goal, so one is asked for rather +/// than invented. +#[test] +fn a_bare_run_with_no_supplied_goal_is_refused_rather_than_run_against_an_empty_one() { + // `runs.goal` has no public reader. `resume::goal_for` recovers the operator's + // own words from the session turn a run served and answers `None` for a run + // that served none — which is every run `io exec` starts. A contract built + // from that `None` would set the agent a task nobody asked for and spend a + // budget pursuing it. + let refusal = exec::goal_or_refusal(41, None, None) + .expect_err("a bare run's goal cannot be recovered, so it must be asked for"); + assert!(refusal.contains("41"), "{refusal}"); + assert!( + refusal.contains("--goal"), + "the refusal must name the way out: {refusal}", + ); + assert!( + refusal.contains("no session turn"), + "it must say why the goal is missing, or it reads as a bug: {refusal}", + ); + + // Supplied on the command line, and the run is resumable. + assert_eq!( + exec::goal_or_refusal(41, None, Some("write the note")).expect("a supplied goal is enough"), + "write the note", + ); + // Whitespace is not a goal. This is the shape a shell hands over for + // `--goal ""`, and accepting it is the empty contract by another road. + assert!(exec::goal_or_refusal(41, None, Some(" ")).is_err()); + + // A run that served a turn needs no flag, and a flag beats what was recovered + // — that is the operator re-aiming their own run. + assert_eq!( + exec::goal_or_refusal(41, Some("write the note".into()), None).expect("recovered"), + "write the note", + ); + assert_eq!( + exec::goal_or_refusal(41, Some("write the note".into()), Some("write two notes")) + .expect("supplied"), + "write two notes", + ); +} + +/// 0.23.0 F8 — a listed run names the id that addresses it, in both streams. +#[test] +fn the_listing_names_the_handle_each_parked_run_is_answered_through() { + use io_cli::resume::Pending; + + let question = Pending::Question { + question_id: 219, + question: "which environment?".into(), + context: None, + choices: vec![], + step: 12, + }; + let plain = exec::listed(41, &question, false).expect("a question is a row"); + assert!(plain.contains("41") && plain.contains("219"), "{plain}"); + + let row = exec::listed(41, &question, true).expect("a question is a row"); + let value: serde_json::Value = serde_json::from_str(&row).expect("one JSON object"); + assert_eq!(value["run_id"], 41); + assert_eq!(value["waiting_on"], "question"); + assert_eq!(value["id"], 219); + assert_eq!(value["step"], 12); + + let plan = exec::listed( + 41, + &Pending::Plan { + plan_id: 307, + steps: vec![], + step: 5, + }, + true, + ) + .expect("a plan is a row"); + let value: serde_json::Value = serde_json::from_str(&plan).expect("one JSON object"); + assert_eq!(value["waiting_on"], "plan"); + assert_eq!(value["id"], 307); + + let recovery = exec::listed( + 41, + &Pending::Recovery { + attempt_id: 512, + tool: "charge".into(), + step: 4, + }, + true, + ) + .expect("an interrupted call is a row"); + let value: serde_json::Value = serde_json::from_str(&recovery).expect("one JSON object"); + assert_eq!(value["waiting_on"], "recovery"); + assert_eq!(value["id"], 512); + + // A run whose process went away has no second id, and says so with a null + // rather than with a number it made up. + let died = + exec::listed(41, &Pending::Died { last_step: 6 }, true).expect("a died run is a row"); + let value: serde_json::Value = serde_json::from_str(&died).expect("one JSON object"); + assert_eq!(value["waiting_on"], "died"); + assert!(value["id"].is_null(), "{died}"); + assert_eq!(value["step"], 6); + + // Under `--json` every row is one object and nothing else, the same split + // `io exec --json` makes. + for pending in [&question, &Pending::Died { last_step: 6 }] { + let row = exec::listed(41, pending, true).expect("a row"); + assert!(row.starts_with('{') && row.ends_with('}'), "{row}"); + } +} + +/// 0.23.0 F8 — the two runs nobody is waiting on are not offered as work. +#[test] +fn a_run_that_cannot_be_carried_on_is_not_a_row_in_the_listing() { + // Listing them under a heading that means "waiting for you" would offer work + // that does not exist. An operator who names one by hand is told why by + // `decision_for`; a listing that showed them would send people there. + for pending in [ + io_cli::resume::Pending::Interrupted, + io_cli::resume::Pending::Finished, + ] { + assert_eq!(exec::listed(41, &pending, false), None, "{pending:?}"); + assert_eq!(exec::listed(41, &pending, true), None, "{pending:?}"); + } +} + +/// 0.23.0 F8 — each pause takes its own input, carrying its own id. +#[test] +fn each_pause_produces_the_decision_that_carries_its_own_id() { + use io_cli::exec::Decision; + use io_cli::resume::Pending; + use io_harness::{PlanVerdict, RecoveryDecision}; + + let question = Pending::Question { + question_id: 219, + question: "which environment?".into(), + context: None, + choices: vec![], + step: 12, + }; + assert_eq!( + exec::decision_for( + 41, + &question, + &resume_args(&["io", "resume", "41", "--answer", "the staging one"]) + ) + .expect("an answered question resumes"), + Decision::Answer { + question_id: 219, + answer: "the staging one".into(), + }, + ); + + let plan = Pending::Plan { + plan_id: 307, + steps: vec![], + step: 5, + }; + assert_eq!( + exec::decision_for( + 41, + &plan, + &resume_args(&["io", "resume", "41", "--plan", "approve"]) + ) + .expect("an approved plan resumes"), + Decision::Plan { + plan_id: 307, + verdict: PlanVerdict::Approve, + }, + ); + assert_eq!( + exec::decision_for( + 41, + &plan, + &resume_args(&[ + "io", + "resume", + "41", + "--plan", + "revise", + "--correction", + "start with the tests" + ]) + ) + .expect("a revised plan resumes"), + Decision::Plan { + plan_id: 307, + verdict: PlanVerdict::revise("start with the tests"), + }, + ); + + let recovery = Pending::Recovery { + attempt_id: 512, + tool: "charge".into(), + step: 4, + }; + assert_eq!( + exec::decision_for( + 41, + &recovery, + &resume_args(&[ + "io", + "resume", + "41", + "--recovery", + "completed", + "--account", + "charge ch_9f21 captured" + ]) + ) + .expect("an established call resumes"), + Decision::Recovery { + attempt_id: 512, + decision: RecoveryDecision::Completed { + observation: "charge ch_9f21 captured".into(), + }, + }, + ); + // `abandon` is the operator's word for the harness's `Abort`, which on a + // command line would read as *stop the program*. + assert_eq!( + exec::decision_for( + 41, + &recovery, + &resume_args(&["io", "resume", "41", "--recovery", "abandon"]) + ) + .expect("an abandoned call resumes"), + Decision::Recovery { + attempt_id: 512, + decision: RecoveryDecision::Abort, + }, + ); + + // A run whose process went away has nothing for anyone to decide. + assert_eq!( + exec::decision_for( + 41, + &Pending::Died { last_step: 6 }, + &resume_args(&["io", "resume", "41"]) + ) + .expect("a died run carries on"), + Decision::CarryOn, + ); +} + +/// 0.23.0 F8 — a flag for a pause the run is not on is refused, not ignored. +#[test] +fn a_flag_that_decides_the_wrong_pause_is_refused_and_names_the_right_one() { + use io_cli::resume::Pending; + + // clap cannot see which pause a run is holding; only the store can. So + // `--plan approve` typed at a run waiting on a question parses cleanly and is + // an operator authorising something they have not been shown. + let question = Pending::Question { + question_id: 219, + question: "which environment?".into(), + context: None, + choices: vec![], + step: 12, + }; + let wrong = exec::decision_for( + 41, + &question, + &resume_args(&["io", "resume", "41", "--plan", "approve"]), + ) + .expect_err("a plan verdict does not answer a question"); + assert!(wrong.contains("--plan"), "{wrong}"); + assert!( + wrong.contains("219") && wrong.contains("--answer"), + "the refusal must name what the run IS waiting on and how to answer it: {wrong}", + ); + + // The same flag missing altogether gets the same sentence, because it is the + // same question an operator is asking. + let missing = exec::decision_for(41, &question, &resume_args(&["io", "resume", "41"])) + .expect_err("a question needs an answer"); + assert!( + missing.contains("219") && missing.contains("--answer"), + "{missing}" + ); + + // And a run with nothing to decide takes none of the three. + let died = exec::decision_for( + 41, + &Pending::Died { last_step: 6 }, + &resume_args(&["io", "resume", "41", "--answer", "yes"]), + ) + .expect_err("a died run has no question to answer"); + assert!( + died.contains("--answer") && died.contains("step 6"), + "{died}" + ); +} + +/// 0.23.0 F8 — the two payload flags are refused where they would be dropped. +#[test] +fn a_payload_without_its_verdict_and_a_verdict_without_its_payload_are_both_refused() { + use io_cli::cli::{PlanFlag, RecoveryFlag}; + use io_harness::{PlanVerdict, RecoveryDecision}; + + // `--plan revise` with nothing to change is a plan sent back saying nothing, + // and `--plan approve --correction "…"` is somebody who meant `revise` — where + // dropping the correction runs the very plan they were trying to change. + assert!(exec::verdict_for(PlanFlag::Revise, None) + .expect_err("revise needs a correction") + .contains("--correction")); + assert!(exec::verdict_for(PlanFlag::Revise, Some(" ")) + .expect_err("whitespace is not a correction") + .contains("--correction")); + let misplaced = exec::verdict_for(PlanFlag::Approve, Some("do it differently")) + .expect_err("a correction does not belong to approve"); + assert!(misplaced.contains("--plan revise"), "{misplaced}"); + assert_eq!( + exec::verdict_for(PlanFlag::Approve, None).expect("approve"), + PlanVerdict::Approve, + ); + assert_eq!( + exec::verdict_for(PlanFlag::Cancel, None).expect("cancel"), + PlanVerdict::Cancel, + ); + + // `--recovery completed` with no account tells the agent a call landed and + // then says nothing about what it returned, which is the transcript hole the + // whole recovery pause exists to close. + assert!(exec::recovery_for(RecoveryFlag::Completed, None) + .expect_err("completed needs an account") + .contains("--account")); + assert!(exec::recovery_for(RecoveryFlag::Completed, Some("")) + .expect_err("an empty account is not an account") + .contains("--account")); + let stray = exec::recovery_for(RecoveryFlag::Retry, Some("it landed")) + .expect_err("an account does not belong to retry"); + assert!(stray.contains("--recovery completed"), "{stray}"); + assert_eq!( + exec::recovery_for(RecoveryFlag::Retry, None).expect("retry"), + RecoveryDecision::Retry, + ); + assert_eq!( + exec::recovery_for(RecoveryFlag::Abandon, None).expect("abandon"), + RecoveryDecision::Abort, + ); +} diff --git a/tests/failure.rs b/tests/failure.rs index 185260a..2af4492 100644 --- a/tests/failure.rs +++ b/tests/failure.rs @@ -113,3 +113,93 @@ fn the_image_sentence_wins_over_the_general_routing_one() { let advice = advice(&no_image_endpoint()).expect("a recognised failure"); assert!(advice.contains("pictures"), "{advice}"); } + +/// The session-head shape of `Error::Conflict`: `run_id` holds a **session** id, +/// and `owner` and `expires_at` are empty because a head is a value that moved +/// rather than something a process is holding. Written by +/// `Store::set_session_head_if`, which is what every head write in this product +/// now goes through. +fn head_moved_under_us() -> Error { + Error::Conflict { + run_id: 7, + owner: String::new(), + expires_at: String::new(), + } +} + +/// The run-lease shape, which is the one the variant's own `Display` was written +/// for: a real run, a named holder, a real expiry. +fn run_lease_held() -> Error { + Error::Conflict { + run_id: 7, + owner: "io-cli@another-terminal".to_string(), + expires_at: "2026-08-27T11:04:00.000Z".to_string(), + } +} + +/// **Asserted by absence, and it has to be.** `Error::Conflict`'s `Display` is +/// `run {run_id} is held by another owner until {expires_at}`, so on the head +/// shape it renders a session id under the word "run" and stops on the word +/// "until" with nothing after it — and the operator meets that sentence after a +/// turn they have already paid for. A test that only checked the new sentence was +/// present would pass with that line still sitting underneath it, which is +/// exactly the state this arm exists to end. +#[test] +fn a_lost_head_race_never_calls_the_session_a_run_or_prints_an_empty_expiry() { + let error = head_moved_under_us(); + let said = said(&error); + + assert!( + said.contains("session 7"), + "the id is named, and named as the session it is: {said}", + ); + assert!( + said.contains("another `io`"), + "and what happened is that another process moved first: {said}", + ); + + assert!( + !said.contains("run 7") && !said.contains("run "), + "the id must never be presented as a run id: {said}", + ); + assert!( + !said.to_lowercase().contains("until"), + "there is no expiry on a head, so nothing may be rendered as one: {said}", + ); + assert!( + !said.contains("owner"), + "and no holder, because a head has none: {said}", + ); + assert!( + !said.contains(&error.to_string()), + "this is the one line this module drops rather than keeps underneath — it \ + is wrong rather than terse: {said}", + ); +} + +/// The other shape, which the harness's line describes correctly, so it keeps it. +/// Rendering both shapes the same way would only move the lie: a real lease with a +/// real holder and a real expiry called a session would be the same defect the +/// other way round. +#[test] +fn a_run_another_process_holds_still_says_the_holder_and_the_expiry_it_has() { + let error = run_lease_held(); + let said = said(&error); + + assert!( + said.starts_with("another process is already running this"), + "the sentence says what to do about a held run: {said}", + ); + assert!( + said.contains(&error.to_string()), + "and the harness's own line is right for this shape, so it stays: {said}", + ); + assert!( + said.contains("2026-08-27T11:04:00.000Z"), + "including the expiry this one actually has: {said}", + ); + assert!( + !said.contains("session"), + "a held run is not a moved head: {said}", + ); +} diff --git a/tests/glyphs.rs b/tests/glyphs.rs index 1735fce..ab060c2 100644 --- a/tests/glyphs.rs +++ b/tests/glyphs.rs @@ -204,9 +204,10 @@ fn a_shortened_string_still_says_that_it_was_shortened() { fn the_two_sets_agree_on_every_width_the_layout_depends_on() { // The status line drops whole fields by counting the separator, the picker // places the terminal cursor just past the marker, and `sessions::rows` - // budgets four cells for the marker and the gap. All three are arithmetic - // over a constant, and they stay arithmetic over a constant only because - // these two agree. + // budgets four cells for the marker and the gap — plus, since 0.23.0, the + // width of the state mark on a row that carries one, which is measured + // rather than assumed. All three are arithmetic over a constant, and they + // stay arithmetic over a constant only because these two agree. assert_eq!( UNICODE.separator.chars().count(), ASCII.separator.chars().count(), @@ -662,6 +663,10 @@ fn the_resume_rows_draw_in_ascii_and_keep_the_facts_that_fit() { turns: 6, prompt: "make the retry loop back off instead of hammering the endpoint".into(), at: "2026-08-17 02:31".into(), + // As in `tests/narrow.rs`: `Finished` is the no-mark case, so this row's + // detail budget is unchanged and the sweep below still sees the same + // string it was written for. + pending: io_cli::resume::Pending::Finished, }]; let rows = io_cli::sessions::rows(&sessions, 80, &ASCII); let row = rows.first().expect("one session, one row"); diff --git a/tests/intent.rs b/tests/intent.rs index c530d4b..02f527e 100644 --- a/tests/intent.rs +++ b/tests/intent.rs @@ -13,8 +13,9 @@ use std::sync::Arc; use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; use io_cli::app::App; +use io_cli::intent::{Asked, Intent}; use io_cli::theme::DARK; -use io_harness::{Question, Responder}; +use io_harness::{PendingQuestion, Question, Responder}; fn key(code: KeyCode) -> KeyEvent { KeyEvent::new(code, KeyModifiers::NONE) @@ -235,3 +236,203 @@ async fn the_question_is_readable_and_answerable_with_no_colour() { Some("keep it".to_string()), ); } + +// ---- 0.23.0: the same overlay, opened on a run that already stopped ---- + +/// The question every test below is asked, live or stored — one fixture, so the +/// two construction paths are compared on identical material. +fn question() -> Question { + Question { + question: "drop the column or keep it?".to_string(), + context: Some("it has 40 rows and one caller".to_string()), + choices: vec!["drop".to_string(), "keep".to_string()], + } +} + +/// A `PendingQuestion` the store itself wrote and read back. +/// +/// **The fixture is authentic because io-harness built it.** `PendingQuestion` +/// has no public constructor, and a struct literal assembled here would skip the +/// JSON round-trip `choices` actually goes through — which is the one field a +/// resumed overlay could plausibly lose. So the row is written and read exactly +/// as a resume reads it. `Store::memory()` rather than a temporary directory: +/// same schema, same SQL, same encode and decode, and no directory any assertion +/// here looks at. +fn stored(question: &Question) -> PendingQuestion { + let store = io_harness::Store::memory().expect("an in-memory store"); + let run = store + .start_run("drop the column", "openrouter") + .expect("a run to hang the question off"); + let id = store + .put_question(run, 1, question) + .expect("the row is written"); + store + .question(id) + .expect("the row reads back") + .expect("the row is there") +} + +/// One overlay, opened on a live turn. The receiver comes back with it because a +/// `oneshot` whose receiver has been dropped is what the live path already reads +/// as "nobody can answer" — a fixture that dropped it would be testing the +/// stored path through the live constructor. +fn live(question: Question) -> (Intent, tokio::sync::oneshot::Receiver>) { + let (answer, reply) = tokio::sync::oneshot::channel(); + (Intent::new(Asked { question, answer }), reply) +} + +/// What the overlay draws, on its own rather than through the app: `App` can only +/// hold a live question, so a stored one has to be rendered directly, and holding +/// the live one to the same helper is what makes the comparison a comparison. +fn drawn_overlay(overlay: &Intent, theme: &io_cli::theme::Theme) -> String { + let (mut screen, _recorder) = support::screen_of(80, 12, 12); + screen + .draw(|frame| overlay.render(frame, frame.area(), theme)) + .expect("a frame"); + screen.viewport_text().to_string() +} + +/// Everything about a question overlay that does **not** depend on which way in +/// was taken. Run against both constructors, because a property asserted for a +/// live question and not a stored one is how the two paths drift apart. +fn the_properties_both_paths_owe(open: impl Fn() -> Intent) { + let screen = drawn_overlay(&open(), &DARK); + assert!(screen.contains("drop the column or keep it?"), "{screen}"); + assert!( + screen.contains("40 rows and one caller"), + "the context: {screen}", + ); + assert!( + screen.contains("drop") && screen.contains("keep"), + "the choices, as offers: {screen}", + ); + assert!(screen.contains("Esc"), "the way out is a word: {screen}"); + + let mut answering = open(); + assert_eq!( + answering.key(key(KeyCode::Enter)), + None, + "an empty prompt is a mis-key, not an answer", + ); + for ch in "keep it".chars() { + assert_eq!( + answering.key(key(KeyCode::Char(ch))), + None, + "typing does not close the overlay", + ); + } + assert_eq!( + answering.key(key(KeyCode::Enter)), + Some(Some("keep it".to_string())), + "and the answer is exactly what was typed", + ); + + let mut declining = open(); + assert_eq!( + declining.key(key(KeyCode::Esc)), + Some(None), + "Esc declines, and declining carries no answer", + ); +} + +/// **A question resumed off the store is answered by the same widget on the same +/// keys as a live one.** The whole point of the second constructor: an operator +/// answering yesterday's pause must not be looking at a different surface. +/// +/// Sabotage: give the stored path its own `render` and drop the choices from it — +/// under which only this test fails, and it fails on the stored arm alone while +/// every live-path test above still passes, which is precisely the drift this +/// shared block exists to catch. +#[test] +fn a_stored_question_answers_and_declines_and_draws_as_a_live_one_does() { + the_properties_both_paths_owe(|| live(question()).0); + the_properties_both_paths_owe(|| Intent::resumed(&stored(&question()))); +} + +/// **A live question resolves by sending; the caller is handed nothing back**, +/// because the turn parked on the channel already has it. +#[tokio::test] +async fn a_live_answer_goes_down_the_channel_and_leaves_the_caller_nothing_to_do() { + let (overlay, reply) = live(question()); + + assert_eq!( + overlay.resolve(Some("keep it".to_string())), + None, + "nothing comes back: the turn took it", + ); + assert_eq!( + reply.await.expect("the turn's end"), + Some("keep it".to_string()), + ); +} + +/// **A stored question resolves by returning**, because the run it belongs to has +/// already ended and there is no channel to send down — the answer has to reach +/// `resume_with_answer_observed`, and it does that as a value. +/// +/// Sabotage: build the stored path a `oneshot` of its own and send into it — under +/// which only this test fails, and it fails by dropping the operator's answer into +/// a channel with no receiver, which is the shape the live path reads as "nobody +/// answered, stay paused". +#[test] +fn a_stored_answer_comes_back_to_the_caller_that_has_to_deliver_it() { + let overlay = Intent::resumed(&stored(&question())); + + assert_eq!( + overlay.resolve(Some("keep it".to_string())), + Some(Some("keep it".to_string())), + "the caller resumes the run with this", + ); +} + +/// **Esc on a stored question declines without producing an answer**: the run was +/// found parked and is left parked, and what comes back says "no answer" rather +/// than an empty one, which the agent would read as information. +#[test] +fn esc_on_a_stored_question_leaves_the_run_parked_with_no_answer() { + let mut overlay = Intent::resumed(&stored(&question())); + + assert_eq!(overlay.key(key(KeyCode::Esc)), Some(None)); + assert_eq!( + overlay.resolve(None), + Some(None), + "no answer is delivered, and none is invented", + ); +} + +/// The one line that is allowed to differ, and the reason it differs: declining a +/// live question defers it inside a turn that is still running, while declining a +/// resumed one leaves a run parked exactly where it was found. Both name `Esc`; +/// neither promises something behind the screen is not doing. +#[test] +fn the_footer_says_what_esc_actually_leaves_behind_on_each_path() { + let live_screen = drawn_overlay(&live(question()).0, &DARK); + let stored_screen = drawn_overlay(&Intent::resumed(&stored(&question())), &DARK); + + assert!( + live_screen.contains("Esc leaves it for later"), + "{live_screen}", + ); + assert!( + stored_screen.contains("Esc leaves the run parked"), + "{stored_screen}", + ); + assert!( + !stored_screen.contains("for later"), + "a resumed question does not promise a turn will pick it up: {stored_screen}", + ); +} + +/// **Non-functional — a resumed question is readable with no colour at all.** The +/// stored path draws under `MONO` for the same reason the live one must: a tone +/// carries nothing there, so the question, the context, the choices and the way +/// out all have to be words. +#[test] +fn a_stored_question_is_readable_with_no_colour_at_all() { + let overlay = Intent::resumed(&stored(&question())); + let screen = drawn_overlay(&overlay, &io_cli::theme::MONO); + + assert!(screen.contains("drop the column or keep it?"), "{screen}"); + assert!(screen.contains("40 rows"), "{screen}"); + assert!(screen.contains("Esc"), "{screen}"); +} diff --git a/tests/live.rs b/tests/live.rs index 33bd3bb..9182067 100644 --- a/tests/live.rs +++ b/tests/live.rs @@ -2469,3 +2469,165 @@ async fn live_f5_an_ordinary_question_is_answered_rather_than_worked_on() { .count(); assert_eq!(calls, 0, "the turn made {calls} tool calls"); } + +/// 0.23.0's headline, against a real model: a run that stopped to ask is answered +/// afterwards and carries on from the step it stopped at. +/// +/// **Nothing in the offline suite can assert this.** `tests/resume.rs` proves the +/// arithmetic against a seeded store, but "the answer reached the agent and the +/// run went on to do the work" needs a provider — and it is the sentence the +/// whole release is written around, so it is asserted here or nowhere. +/// +/// The pause is produced honestly rather than by inserting a row: the responder's +/// receiver is dropped, which is how io-harness is told nobody is here to answer, +/// so the harness persists the question and ends the run at `AwaitingAnswer` +/// exactly as it does for a headless `io exec`. Whether the model asks at all is +/// still the model's choice, so the assertions are conditional in the shape +/// `live_f1_a_question_is_answered_on_an_uncontained_turn` already uses — and +/// what is unconditional is that this crate never drives a run it should not. +#[tokio::test] +#[ignore = "live: needs OPENROUTER_API_KEY"] +async fn live_f2_a_parked_question_is_answered_and_the_run_carries_on() { + use io_cli::contract::Capabilities; + use io_cli::resume::{self, Pending}; + + let key = key(); + let dir = tempfile::tempdir().expect("a workspace"); + let root = dir.path(); + std::fs::write( + root.join("notes.md"), + "# notes\n\nold line\n\n## archive\n\nold line\n", + ) + .expect("the fixture file"); + + let store = Store::open(root.join("runs.db")).expect("a store"); + let mut session = Session::open(&store, root).expect("a session"); + let provider = io_harness::OpenRouter::new(&key, model()); + let policy = workspace_policy(); + let collected = Arc::new(Mutex::new(Vec::new())); + let observer = Collector { + events: Arc::clone(&collected), + }; + + // **The receiver is dropped on purpose.** `Answerer::answer` awaits a reply + // that can never come and resolves `None`, which is io-harness's own signal + // that nobody can answer — so the question is written to `pending_questions` + // and the run ends parked. This is the `io exec` shape, reproduced here + // because it is the state a resume has to start from. + let (answerer, parked) = io_cli::intent::channel(); + drop(parked); + let goal = "notes.md contains the line `old line` twice. Replace exactly one of them with \ + `new line`. If it is not clear which one is meant, ask before editing."; + let contract = io_cli::contract::session( + goal, + root.to_path_buf(), + &no_configuration(), + &Capabilities::default(), + Arc::new(answerer), + None, + ) + .with_max_steps(12); + + let result = session + .turn_bounded_observed( + &contract, + &provider, + &store, + &policy, + &io_harness::ApproveAll, + &observer, + ) + .await + .expect("the turn runs"); + + let run_id = result.run_id; + let pending = resume::pending_for(&store, run_id).expect("the store answers"); + let Pending::Question { question_id, .. } = pending else { + // The model chose to edit rather than ask, which is a legal answer to + // this goal. Nothing to resume, and the release is not falsified by a + // model being decisive — but say so, so a run of this suite that never + // exercised the path cannot read as one that did. + eprintln!("live_f2: the model did not ask; nothing was parked to resume ({pending:?})"); + return; + }; + + // Read **before** the resume, because that is the only moment it means + // anything: afterwards it is the step the resumed run reached. + let before = store.last_step(run_id).expect("the last committed step"); + let expected_head = session.head(); + + let resumed = resume::answer_question( + &contract, + &provider, + &store, + run_id, + question_id, + "the one under the archive heading", + &policy, + &io_harness::ApproveAll, + None, + &observer, + expected_head, + ) + .await + .expect("the parked run is answered and driven"); + + assert_eq!( + resumed.resumed_after, before, + "the resume must carry on from the step the run stopped at, not from the beginning", + ); + + // The harness's own markers, read back out of the store rather than trusted: + // one `resume` for the continuation and one `skipped` per step already + // committed. This is what distinguishes a resume from a re-run. + let markers = store + .checkpoint_events(run_id) + .expect("the checkpoint events"); + let resumes = markers + .iter() + .filter(|event| event.kind == "resume") + .count(); + let skipped = markers + .iter() + .filter(|event| event.kind == "skipped") + .count(); + assert_eq!(resumes, 1, "exactly one resume marker, got {resumes}"); + assert_eq!( + skipped, before as usize, + "one skipped marker per committed step: {before} committed, {skipped} skipped", + ); + + // And the session bookkeeping a free resume does not do. A turn left open + // here is the failure that shows up weeks later as a missing turn. + let turn_id = resumed.turn_id.expect("a session turn was closed"); + let turn = store + .session_turn(turn_id) + .expect("the store answers") + .expect("the turn is held"); + assert!( + turn.outcome.is_some(), + "the resumed turn must be closed, not left reading awaiting_answer", + ); + // Unchanged, and that is the correct answer rather than a weak one: the turn + // already existed and the head already pointed at it — `Session::drive` closes + // both even for a run that paused. What the resume had to do was write the + // head *conditionally*, and a write that refused would have returned + // `Failure::HeadMoved` above rather than reaching here. + assert_eq!( + session.head(), + expected_head, + "the resumed turn is still the head this process believed in", + ); + + // The answer must have reached the model, which is the whole point and the + // one thing only a live run can say. + let events = collected.lock().expect("not poisoned"); + let steps = events + .iter() + .filter(|event| matches!(event.kind, EventKind::Step { .. })) + .count(); + assert!( + steps > 0, + "the resumed run drove no step, so the answer reached nothing", + ); +} diff --git a/tests/lock.rs b/tests/lock.rs new file mode 100644 index 0000000..96130d2 --- /dev/null +++ b/tests/lock.rs @@ -0,0 +1,538 @@ +//! F9 — one `io` at a time on one session, and a refusal that says who has it. +//! +//! **The lock is keyed on the session id, and the test that matters most here is +//! the one that says what does *not* contend.** `Session::open` creates a new +//! row on every call, so two terminals started in one repository hold two +//! different ids and are two conversations sharing a directory name. A lock +//! keyed on the workspace root refused the second of them — a hard regression +//! that every other assertion in this file passed happily. The contention that +//! is real is the same id reached twice, which is what `/resume` and `io resume` +//! do, and which is the only way two processes ever advance one head. +//! +//! **Every test here runs in one process, and that is not a compromise.** On +//! unix `flock` is held per open file description and on Windows `LockFileEx` is +//! held per handle, so two `File::open` calls on one path inside a single +//! process contend exactly as two processes would. That is what makes this file +//! possible at all: N1 forbids a test that sleeps, and a fixture that spawned a +//! second `io` and waited for it to reach the lock would have to. +//! +//! **That premise is a fact about the three platforms this product releases +//! for, and it is stated rather than assumed.** The standard library reaches for +//! `flock` on Linux, on Apple platforms and on the BSDs, and for `LockFileEx` on +//! Windows; both are held per description or per handle. Solaris alone uses +//! `fcntl(F_SETLK)`, whose record locks are held per *process* — two opens there +//! would not conflict, and neither this file nor `src/lock.rs` would be right +//! about it. io-cli ships no Solaris artifact. +//! +//! No clock is read either. Every instant below is built from the unix epoch and +//! a `Duration`, and the lapse decision takes the `now` it is aged against as an +//! argument — which is the whole reason `Owner::lapsed` has that shape. +//! +//! The sabotage this file is aimed at is a refusal that reports the *reader's* +//! own process id instead of the holder's. It always looks right, because in the +//! ordinary fixture the two are the same number. So the record is overwritten +//! with a pid no process on any of the three platforms can have, and the refusal +//! is made to name it. + +use std::path::{Path, PathBuf}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; + +use io_cli::lock::{self, Guard, Owner, Taken, LEASE}; + +/// The session every test works on unless it is about two of them. +const SESSION: i64 = 7; + +/// An io home that does not exist yet, so `acquire` is made to create it — which +/// is the same `home::create` the `0700` assertion at the bottom of this file +/// depends on. +fn home(dir: &Path) -> PathBuf { + dir.join(".io-cli") +} + +/// A fixed instant, so nothing in this file has to ask what time it is. +fn at(seconds: u64) -> SystemTime { + UNIX_EPOCH + Duration::from_secs(seconds) +} + +/// The instant every acquisition below records unless it says otherwise. +fn started() -> SystemTime { + at(1_700_000_000) +} + +fn guard(taken: Taken) -> Guard { + match taken { + Taken::Held(guard) => guard, + Taken::Refused(owner) => { + panic!( + "the lock should have been free; it was refused: {}", + owner.sentence() + ) + } + } +} + +fn refusal(taken: Taken) -> Owner { + match taken { + Taken::Refused(owner) => owner, + Taken::Held(_) => panic!("two `io` processes were let into one session"), + } +} + +/// **F9.** A second `io` on the same session is refused while the first is +/// alive, and let in the moment it is not. +/// +/// Both halves in one test on purpose: an implementation that refuses everything +/// passes the first assertion and is useless, and one that refuses nothing +/// passes the second and is the defect. +#[test] +fn f9_a_second_io_on_one_session_is_refused_while_the_first_holds_the_lock() { + let dir = tempfile::tempdir().expect("a temporary directory"); + let home = home(dir.path()); + let root = dir.path().join("repository"); + + let first = guard(lock::acquire(&home, SESSION, &root, started()).expect("the lock is free")); + let second = lock::acquire(&home, SESSION, &root, started()).expect("asking is not an error"); + let owner = refusal(second); + assert!( + owner.sentence().contains("another `io` holds this session"), + "the refusal has to say what happened: {}", + owner.sentence(), + ); + + drop(first); + + let third = + guard(lock::acquire(&home, SESSION, &root, started()).expect("the lock is free again")); + let (lock_path, owner_path) = lock::paths(&home, SESSION); + assert!( + lock_path.is_file(), + "the lock file itself is never removed; unlinking one another process holds \ + would give the next opener a fresh inode and two `io` would both think they \ + were alone", + ); + drop(third); + + assert!( + !owner_path.exists(), + "the owner record is taken away with the lock, so a later reader is not told \ + about a process that has gone", + ); + assert!(lock_path.is_file(), "and the lock file is still there"); +} + +/// **F9, and the case a workspace-keyed lock got wrong.** Two sessions in one +/// repository do not contend; one session does, wherever it is entered from. +/// +/// `Session::open` creates a new row every time it is called, so two terminals +/// started in one repository hold two different ids and share nothing but a +/// directory name. Refusing the second is refusing work that was never in +/// conflict — which is what keying the lock on the workspace root did, and what +/// the first half here would have failed on. The second half is the collision +/// that is real: the same id, which is what `/resume` and `io resume` hand in. +/// +/// The last assertion is the inverse property, and it is the sabotage: the root +/// must not enter the name at all. A holder is refused on its own id from a +/// *different* workspace, and the refusal reports the workspace the holder +/// recorded rather than the one the refused process was handed. +#[test] +fn f9_two_sessions_in_one_workspace_do_not_contend_but_one_session_does() { + let dir = tempfile::tempdir().expect("a temporary directory"); + let home = home(dir.path()); + let root = dir.path().join("repository"); + + let first = guard(lock::acquire(&home, 1, &root, started()).expect("the lock is free")); + let second = guard(lock::acquire(&home, 2, &root, started()).expect( + "a second terminal in one repository is a second conversation, not a conflict; \ + this is the acquisition a workspace-keyed lock refused", + )); + + assert_ne!( + lock::paths(&home, 1), + lock::paths(&home, 2), + "two sessions are two names, or they are one lock wearing two", + ); + let (lock_path, _) = lock::paths(&home, 1); + assert!( + lock_path + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with("session-1.")), + "the id is the name, legibly, so an operator can see which conversation a file \ + belongs to: {}", + lock_path.display(), + ); + + let resumed = + refusal(lock::acquire(&home, 1, &root, started()).expect("asking is not an error")); + assert!( + resumed + .sentence() + .contains("another `io` holds this session"), + "the same id twice is two processes on one head: {}", + resumed.sentence(), + ); + + // The same id from another workspace. It is still that conversation, so it is + // still refused, and the sentence names where the holder actually is. + let elsewhere = dir.path().join("another-repository"); + let away = + refusal(lock::acquire(&home, 1, &elsewhere, started()).expect("asking is not an error")); + assert_eq!( + away.root.as_deref(), + Some(root.as_path()), + "the refusal names the workspace the HOLDER recorded, never the one the refused \ + process was handed, and the root is no part of the key", + ); + + drop(first); + drop(second); +} + +/// **F9, and the sabotage.** The refusal carries what the *holder* wrote, and +/// specifically a process id that is not the reader's own. +/// +/// The first half asserts the record `acquire` writes is true of the holder. The +/// second overwrites it with four facts that are deliberately none of this +/// process's — a pid no operating system issues, another workspace, a version +/// this binary is not, and an instant a decade off — and requires every one of +/// them to come back. A refusal assembled from `std::process::id()`, +/// `env!("CARGO_PKG_VERSION")` and the root it was handed would pass the first +/// half and fail here, which is the point of writing both. +#[test] +fn f9_the_refusal_names_the_holder_that_wrote_the_record_and_never_the_reader() { + let dir = tempfile::tempdir().expect("a temporary directory"); + let home = home(dir.path()); + let root = dir.path().join("repository"); + + let held = guard(lock::acquire(&home, SESSION, &root, started()).expect("the lock is free")); + let mine = + refusal(lock::acquire(&home, SESSION, &root, started()).expect("asking is not an error")); + + assert_eq!(mine.pid, Some(std::process::id())); + assert_eq!( + mine.root.as_deref(), + Some(root.as_path()), + "the root is still recorded even though it is not the key; it is the clause that \ + tells an operator which terminal to go to", + ); + assert_eq!(mine.version.as_deref(), Some(env!("CARGO_PKG_VERSION"))); + assert_eq!( + mine.started, + Some(started()), + "the instant recorded is the one the driver handed in, not one this crate read", + ); + + // A pid larger than any of the three platforms issues, so it cannot be a + // process and it certainly cannot be this one. + let planted = Owner { + pid: Some(u32::MAX), + root: Some(PathBuf::from("/some/other/workspace")), + version: Some("0.0.1-not-this-binary".to_string()), + started: Some(at(1_234_567_890)), + }; + let (_, owner_path) = lock::paths(&home, SESSION); + std::fs::write(&owner_path, planted.render()).expect("the record is a plain file"); + + let read = + refusal(lock::acquire(&home, SESSION, &root, started()).expect("asking is not an error")); + assert_ne!( + planted.pid, + Some(std::process::id()), + "the fixture has to be a pid this process cannot have, or it proves nothing", + ); + assert_eq!(read, planted, "every field came from the file"); + + let said = read.sentence(); + assert!( + said.is_ascii(), + "the refusal is drawn on a terminal that may have no font: {said}" + ); + assert!( + said.contains(&u32::MAX.to_string()), + "the holder's pid is named: {said}" + ); + assert!( + said.contains("/some/other/workspace"), + "and its workspace: {said}" + ); + assert!( + said.contains("0.0.1-not-this-binary"), + "and its version: {said}" + ); + + drop(held); +} + +/// **F9.** A record that is missing, truncated, or written by a version that +/// recorded different fields still produces a refusal, and it claims only what +/// it can read. +/// +/// Four shapes, one property: nothing panics and nothing is invented. The +/// sentence is asserted for what it does *not* say as well — a refusal that +/// printed `unknown` four times would satisfy "does not claim facts it does not +/// have" and read as a broken product. +#[test] +fn f9_a_record_it_cannot_read_produces_a_refusal_that_claims_nothing_it_does_not_know() { + let dir = tempfile::tempdir().expect("a temporary directory"); + let home = home(dir.path()); + let root = dir.path().join("repository"); + let (_, owner_path) = lock::paths(&home, SESSION); + + let held = guard(lock::acquire(&home, SESSION, &root, started()).expect("the lock is free")); + + // Missing: removed out from under the holder, which is what a `tmpwatch` or + // an operator tidying their home directory does. + std::fs::remove_file(&owner_path).expect("the record is an ordinary file"); + let bare = + refusal(lock::acquire(&home, SESSION, &root, started()).expect("asking is not an error")); + assert_eq!( + bare, + Owner::default(), + "nothing was read, so nothing is claimed" + ); + assert_eq!( + bare.sentence(), + "another `io` holds this session", + "with nothing known the sentence is the bare claim and no placeholders", + ); + for absent in ["unknown", "None", "?"] { + assert!( + !bare.sentence().contains(absent), + "the sentence pads a missing fact with {absent:?}: {}", + bare.sentence(), + ); + } + + // Truncated: the write was cut off mid-line. What landed is kept and the + // half-line is not guessed at. + std::fs::write(&owner_path, "pid = 4242\nroot = /half/a/pa").expect("the record"); + let cut = + refusal(lock::acquire(&home, SESSION, &root, started()).expect("asking is not an error")); + assert_eq!(cut.pid, Some(4242), "the line that landed whole is read"); + assert_eq!( + cut.root.as_deref(), + Some(Path::new("/half/a/pa")), + "a final line with a value is taken at its word; there is nothing else to do", + ); + assert_eq!(cut.version, None); + assert_eq!(cut.started, None); + assert!(cut.sentence().contains("4242")); + + // Truncated inside a name, so the last line has no `=` at all. + std::fs::write(&owner_path, "pid = 4242\nver").expect("the record"); + let mid = + refusal(lock::acquire(&home, SESSION, &root, started()).expect("asking is not an error")); + assert_eq!(mid.pid, Some(4242)); + assert_eq!(mid.version, None, "half a field name is not a version"); + + // Another version's record: fields this one has never heard of, one it has, + // and one whose value will not parse. + std::fs::write( + &owner_path, + "pid = not-a-number\nhostname = build-07\nlease_seconds = 900\nversion = 0.99.0\n", + ) + .expect("the record"); + let other = + refusal(lock::acquire(&home, SESSION, &root, started()).expect("asking is not an error")); + assert_eq!( + other.version.as_deref(), + Some("0.99.0"), + "the field it does know is read" + ); + assert_eq!( + other.pid, None, + "a value that will not parse leaves its own field empty" + ); + assert_eq!(other.root, None); + assert_eq!(other.started, None); + assert!( + !other.sentence().contains("build-07"), + "a field this version does not model is not repeated back as if it were understood: {}", + other.sentence(), + ); + + // Not text at all. + std::fs::write(&owner_path, [0xff, 0xfe, 0x00, 0x80]).expect("the record"); + let binary = + refusal(lock::acquire(&home, SESSION, &root, started()).expect("asking is not an error")); + assert_eq!( + binary, + Owner::default(), + "bytes that are not text are an absence, not a panic" + ); + + drop(held); +} + +/// **F9.** A record this guard did not write is left alone. +/// +/// The fixture puts a directory where the record goes, which is the cheapest +/// thing on all three platforms that a write cannot succeed against — so the +/// guard is acquired with no record of its own. Dropping it must then remove +/// nothing. A `Drop` that unlinked the path unconditionally would take away +/// whatever was actually there, and the one time that path holds something this +/// process did not write is the one time it matters. +#[test] +fn f9_a_guard_that_could_not_write_a_record_removes_nothing_when_it_goes() { + let dir = tempfile::tempdir().expect("a temporary directory"); + let home = home(dir.path()); + let root = dir.path().join("repository"); + let (_, owner_path) = lock::paths(&home, SESSION); + + std::fs::create_dir_all(&owner_path).expect("something else is at the record's path"); + let marker = owner_path.join("not-ours"); + std::fs::write(&marker, "somebody else's").expect("the marker"); + + let held = + guard(lock::acquire(&home, SESSION, &root, started()).expect("the record is not the lock")); + let unreadable = + refusal(lock::acquire(&home, SESSION, &root, started()).expect("asking is not an error")); + assert_eq!( + unreadable, + Owner::default(), + "a record that cannot be read is an absence; the lock is still refused", + ); + + drop(held); + + assert!( + marker.is_file(), + "the guard removed a record it did not write" + ); +} + +/// **F9.** The lapse is computed from a `now` that is handed in, and it is +/// asserted on both sides of the threshold with no clock and nothing sleeping. +/// +/// It is a *fact* and not a policy: the caller confirms a takeover with the +/// operator, and this only says whether there is anything to confirm. So the +/// interesting cases are the two boundaries, the record that never said when it +/// started, and the clock that runs backwards. +#[test] +fn f9_the_lease_lapses_against_a_now_that_is_handed_in_and_never_read() { + let began = at(1_700_000_000); + let owner = Owner { + started: Some(began), + ..Owner::default() + }; + + assert_eq!( + owner.lapsed(began, LEASE), + Some(false), + "a lease that has just started has not lapsed", + ); + assert_eq!( + owner.lapsed(began + LEASE, LEASE), + Some(false), + "the threshold itself is inside the lease; a takeover offered at the exact \ + second is one offered to a session still doing its work", + ); + assert_eq!( + owner.lapsed(began + LEASE + Duration::from_secs(1), LEASE), + Some(true), + "and one second past it has lapsed", + ); + + assert_eq!( + owner.lapsed(began - Duration::from_secs(3_600), LEASE), + Some(false), + "a record stamped in the future is two machines disagreeing about the time, \ + which is not evidence that anybody abandoned anything", + ); + + assert_eq!( + Owner::default().lapsed(began + LEASE * 100, LEASE), + None, + "a record with no instant has no opinion; reading its absence as `old enough` \ + would make every unreadable record a takeover", + ); + + // The lease is a knob, not a constant the decision hides inside itself. + assert_eq!( + owner.lapsed(began + Duration::from_secs(60), Duration::from_secs(30)), + Some(true), + ); + assert!( + LEASE > Duration::from_secs(60 * 60), + "the instant recorded is when the holder STARTED, so a lease shorter than a \ + working session would offer to take over a live conversation", + ); +} + +/// The record survives its own text, which is what makes the refusal readable by +/// a version of `io` that is not the one that wrote it. +#[test] +fn the_owner_record_round_trips_through_the_plain_text_it_is_stored_as() { + let full = Owner { + pid: Some(4242), + root: Some(PathBuf::from("/Users/someone/work/io-cli")), + version: Some("0.23.0".to_string()), + started: Some(at(1_700_000_000)), + }; + assert_eq!(Owner::parse(&full.render()), full); + + // Every field absent writes an empty record rather than four empty values, + // so what comes back is an absence and not four fields that say nothing. + let empty = Owner::default(); + assert!(empty.render().is_empty()); + assert_eq!(Owner::parse(&empty.render()), empty); + + // And a partial one, which is what an older version's record looks like. + let partial = Owner { + pid: Some(9), + ..Owner::default() + }; + assert_eq!(Owner::parse(&partial.render()), partial); + + for line in full.render().lines() { + assert!( + line.contains(" = "), + "each fact is one `name = value` line: {line:?}" + ); + } +} + +/// **N3.** The lock and the record it names belong to the operator alone. +/// +/// The record carries a workspace path, and it sits in the directory +/// `home::create` already makes `0700` around a file holding a credential. The +/// mode is set at creation, the way `settings::write` sets it, so there is no +/// window in which either file is on disk world-readable. +#[cfg(unix)] +#[test] +fn n3_the_lock_and_its_record_are_readable_by_their_owner_alone() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().expect("a temporary directory"); + let home = home(dir.path()); + let root = dir.path().join("repository"); + + let held = guard(lock::acquire(&home, SESSION, &root, started()).expect("the lock is free")); + let (lock_path, owner_path) = lock::paths(&home, SESSION); + + for path in [&lock_path, &owner_path] { + let mode = std::fs::metadata(path) + .unwrap_or_else(|error| panic!("{} exists: {error}", path.display())) + .permissions() + .mode(); + assert_eq!( + mode & 0o777, + 0o600, + "{} is not the operator's alone", + path.display(), + ); + } + + assert_eq!( + std::fs::metadata(&home) + .expect("the home") + .permissions() + .mode() + & 0o777, + 0o700, + "and `acquire` made the home with `home::create`, rather than with a bare \ + `create_dir_all` that would leave it world-readable", + ); + + drop(held); +} diff --git a/tests/narrow.rs b/tests/narrow.rs index 594d461..9f2d563 100644 --- a/tests/narrow.rs +++ b/tests/narrow.rs @@ -358,6 +358,10 @@ fn n5_the_resume_picker_at_eighty_columns_keeps_the_end_of_the_path() { turns: 6, prompt: "make the retry loop back off instead of hammering the endpoint".into(), at: "2026-08-17 02:31".into(), + // A finished session carries no mark, so the row's budget here is the + // one this test was written against and every assertion below still + // measures what it measured before 0.23.0 added the state column. + pending: io_cli::resume::Pending::Finished, }]; let rows = io_cli::sessions::rows(&sessions, WIDTH, &io_cli::theme::DARK.glyphs); let mut picker = io_cli::picker::Picker::new("Resume which session?", rows); diff --git a/tests/plan.rs b/tests/plan.rs index 95c6c77..072a65e 100644 --- a/tests/plan.rs +++ b/tests/plan.rs @@ -14,8 +14,9 @@ use std::sync::Arc; use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; use io_cli::app::App; +use io_cli::plan::{Proposed, Review}; use io_cli::theme::DARK; -use io_harness::{Plan, PlanGate, PlanStep, PlanVerdict}; +use io_harness::{PendingPlan, Plan, PlanGate, PlanStep, PlanVerdict}; fn key(code: KeyCode) -> KeyEvent { KeyEvent::new(code, KeyModifiers::NONE) @@ -282,3 +283,169 @@ async fn the_plan_is_readable_with_no_colour_at_all() { "and the outcome is a word, not a colour", ); } + +// ---- 0.23.0: the same overlay, opened on a run that already stopped ---- + +/// The plan every test below decides, live or stored. +/// +/// One step is handed off, because `[agent]` is drawn only when there is an owner +/// and a fixture of steps that all have `None` cannot see it go missing on the way +/// through the store. +fn handed_off_plan() -> Plan { + Plan::new([ + PlanStep::new("read every caller of the old column"), + PlanStep::new("write the migration").by("writer"), + ]) +} + +/// A `PendingPlan` io-harness itself wrote and read back. +/// +/// **The fixture is authentic because the store built it.** `PendingPlan` has no +/// public constructor, and a struct literal assembled here would skip the JSON +/// encode and decode the steps actually travel through — which is exactly where +/// an optional `agent` is lost if it is going to be. `Store::memory()` rather +/// than a temporary directory: same schema, same SQL, same round-trip, and +/// nothing on disk any assertion here looks at. +fn stored(plan: &Plan) -> PendingPlan { + let store = io_harness::Store::memory().expect("an in-memory store"); + let run = store + .start_run("drop the column", "openrouter") + .expect("a run to hang the plan off"); + let id = store.put_plan(run, 1, plan).expect("the row is written"); + store + .plan(id) + .expect("the row reads back") + .expect("the row is there") +} + +/// One overlay, opened on a live turn. The receiver comes back with it: a +/// `oneshot` whose receiver has been dropped is what the live path reads as +/// "nobody decided", so a fixture that dropped it would be exercising the stored +/// path through the live constructor. +fn live(plan: Plan) -> (Review, tokio::sync::oneshot::Receiver>) { + let (verdict, reply) = tokio::sync::oneshot::channel(); + (Review::new(Proposed { plan, verdict }), reply) +} + +/// What the overlay draws, on its own rather than through the app: `App` can only +/// hold a live plan, so a stored one has to be rendered directly, and holding the +/// live one to the same helper is what makes the comparison a comparison. +fn drawn_overlay(overlay: &Review, theme: &io_cli::theme::Theme) -> String { + let (mut screen, _recorder) = support::screen_of(80, 14, 14); + screen + .draw(|frame| overlay.render(frame, frame.area(), theme)) + .expect("a frame"); + screen.viewport_text().to_string() +} + +/// Everything about a plan overlay that does **not** depend on which way in was +/// taken — which here is everything except where the verdict goes, the footer +/// included: `Esc` cancels a plan on both paths, so both say so. +fn the_properties_both_paths_owe(open: impl Fn() -> Review) { + let screen = drawn_overlay(&open(), &DARK); + assert!(screen.contains("1. read every caller"), "{screen}"); + assert!( + screen.contains("2. write the migration [writer]"), + "numbered, in order, and a handed-off step names its owner: {screen}", + ); + assert!(screen.contains("Enter approves"), "{screen}"); + assert!(screen.contains("Esc cancels"), "{screen}"); + + let mut approving = open(); + assert_eq!( + approving.key(key(KeyCode::Enter)), + Some(PlanVerdict::Approve), + "an empty prompt and Enter is the whole of agreeing", + ); + + let mut revising = open(); + for ch in "start with the tests".chars() { + assert_eq!(revising.key(key(KeyCode::Char(ch))), None); + } + assert_eq!( + revising.key(key(KeyCode::Enter)), + Some(PlanVerdict::revise("start with the tests")), + "text and Enter sends it back in the operator's own words", + ); + + let mut cancelling = open(); + assert_eq!( + cancelling.key(key(KeyCode::Esc)), + Some(PlanVerdict::Cancel), + "and Esc is the one key that means nothing else", + ); +} + +/// **A plan resumed off the store is decided by the same widget on the same keys +/// as a live one**, and draws the same steps in the same order with the same +/// owners. +/// +/// Sabotage: give the stored path its own `render` and drop `[agent]` from it — +/// under which only this test fails, and it fails on the stored arm alone while +/// every live-path test above still passes, which is the drift this shared block +/// exists to catch. +#[test] +fn a_stored_plan_draws_and_decides_as_a_live_one_does() { + the_properties_both_paths_owe(|| live(handed_off_plan()).0); + the_properties_both_paths_owe(|| Review::resumed(&stored(&handed_off_plan()))); +} + +/// **A live verdict resolves by sending; the caller is handed nothing back**, +/// because the turn parked on the channel already has it. +#[tokio::test] +async fn a_live_verdict_goes_down_the_channel_and_leaves_the_caller_nothing_to_do() { + let (overlay, reply) = live(handed_off_plan()); + + assert_eq!(overlay.resolve(Some(PlanVerdict::Approve)), None); + assert_eq!( + reply.await.expect("the turn's end"), + Some(PlanVerdict::Approve), + ); +} + +/// **All three verdicts come back out of the stored path as values**, because the +/// run that proposed the plan has already ended and `resume_with_plan_decision_observed` +/// is the only thing that can act on them. +/// +/// Sabotage: build the stored path a `oneshot` of its own and send into it — under +/// which only this test fails, and it fails by dropping a decision that spends the +/// rest of the budget into a channel with no receiver. +#[test] +fn every_verdict_comes_back_to_the_caller_from_a_stored_plan() { + for verdict in [ + PlanVerdict::Approve, + PlanVerdict::revise("start with the tests"), + PlanVerdict::Cancel, + ] { + let overlay = Review::resumed(&stored(&handed_off_plan())); + assert_eq!( + overlay.resolve(Some(verdict.clone())), + Some(Some(verdict)), + "the caller resumes the run with exactly this", + ); + } +} + +/// Declining to decide is still a decision the caller has to carry: a stored plan +/// left undecided returns `Some(None)`, which is "no verdict", and never an +/// approval by default. The one direction that must never be guessed. +#[test] +fn a_stored_plan_left_undecided_comes_back_as_no_verdict_at_all() { + let overlay = Review::resumed(&stored(&handed_off_plan())); + + assert_eq!(overlay.resolve(None), Some(None)); +} + +/// **Non-functional — a resumed plan is readable with no colour at all.** Tones +/// carry nothing under `NO_COLOR`, so the steps and all three ways out have to be +/// words on the stored path exactly as they are on the live one. +#[test] +fn a_stored_plan_is_readable_with_no_colour_at_all() { + let overlay = Review::resumed(&stored(&handed_off_plan())); + let screen = drawn_overlay(&overlay, &io_cli::theme::MONO); + + assert!(screen.contains("read every caller"), "{screen}"); + assert!(screen.contains("[writer]"), "{screen}"); + assert!(screen.contains("Enter approves"), "{screen}"); + assert!(screen.contains("Esc cancels"), "{screen}"); +} diff --git a/tests/resume.rs b/tests/resume.rs new file mode 100644 index 0000000..74dd532 --- /dev/null +++ b/tests/resume.rs @@ -0,0 +1,863 @@ +//! Carrying a paused run on — F2, F3, F4, F5 and F6. +//! +//! Every fixture here is a **real store on disk**, opened through `Store::open` +//! on a temporary directory, and every run in one either really paused or holds +//! rows written by the same public writer the run loop writes them with. Two of +//! the five states can be reached by driving: a question pause is what a +//! scripted `ask_question` call plus io-harness's own declining responder +//! produces, and an interruption is what `Store::finish_run` records. The other +//! three cannot be driven to from outside the crate at all — a plan pause needs +//! a model that proposes one, an interrupted call needs a process to die +//! mid-call, and a run whose process went away needs the process to go away — so +//! their rows are written with `Store::put_plan`, `Store::open_attempt` and +//! `Store::record`. Those are the same three writers the dispatch and the step +//! loop call, so the rows are authentic in the only sense that matters here: +//! nothing in this file writes a row a run could not have written. +//! +//! What each block would catch, stated so that a green run means something: +//! +//! * **The classification block** catches a reader that answers from the run's +//! status. Sabotage it by classifying on `Store::run_status` alone: a run +//! paused on a question is recorded as `completed` — `finish_run` maps only +//! `awaiting_approval` and `awaiting_plan` to `paused` — so the commonest +//! pause of all reads as an ended run and nothing is ever offered. +//! +//! * **The step-arithmetic block** is F2 and F3. It reads `Store::last_step` +//! before the resume and asserts the `resume` marker against that number plus +//! one. Sabotage it by writing the literal `2`: the assertion still passes on +//! this fixture and stops meaning anything the moment a run pauses at any other +//! step, which is every real run. It also counts the `skipped` markers, which +//! is what fails if a resume re-drives work that was already committed and +//! already paid for. +//! +//! * **The turn block** is F4. Sabotage it by deleting the `finish_turn` call: +//! nothing errors, the run is genuinely carried on, and the turn goes on +//! reading `awaiting_answer` with an empty reply for the rest of the +//! conversation's life — so an operator scrolling back sees the question they +//! answered still open, and the transcript disagrees with the run. +//! +//! * **The race block** is the half of F4 a happy path cannot reach. Sabotage it +//! by reaching for `set_session_head` instead of `set_session_head_if` and +//! every other assertion in this file stays green while a second process's +//! turn is silently taken off the head path — answered, billed, and invisible +//! to the next turn. Two `Store` handles on one file is the shape io-harness's +//! own lease suite uses; there is no second process and nothing waits. +//! +//! * **The interruption block** is F5. Sabotage it by letting the driver through +//! to `resume_with_observed`: the call returns `Ok` carrying the original +//! outcome, so a driver that trusted the `Result` would report a successful +//! resume of a run it never touched. The assertion is on the absence of a +//! `resume` marker rather than on the return value, for exactly that reason. +//! +//! * **The recovery block** is F6, and it asserts the step number rather than +//! the text. Sabotage it by filing the operator's account at the run's current +//! step: the text is still in the ledger and a `contains` assertion still +//! passes, while the model reads a transcript in which a call it made three +//! steps ago was answered after the fact — which is not what happened, and is +//! the one thing the operator was asked to establish. +//! +//! * **The cancelled-plan block** catches a `Cancel` mapped onto a plain resume. +//! Sabotage it that way and the outcome is no longer `plan_rejected`, the run +//! re-enters the loop, and the rest of the budget goes on the approach that was +//! just refused. +//! +//! * **The two wrong-owner blocks** catch a driver that trusts the ids it is +//! handed. Sabotage either by dropping the ownership check: io-harness refuses +//! it too, but as one `Error::Resume` whose `Display` is written for a library +//! caller, and the sentence an operator gets stops naming which run their +//! answer was about to reach. + +mod support; + +use io_cli::resume::{self, Failure, Pending}; +use io_harness::provider::{CompletionRequest, CompletionResponse, ToolCall}; +use io_harness::{ + ApproveAll, Ignore, Plan, PlanStep, PlanVerdict, Policy, Provider, RecoveryDecision, + RunOutcome, Session, StepRecord, Store, TaskContract, ToolRecovery, ASK_QUESTION_TOOL, +}; +use support::Scripted; + +const GOAL: &str = "tidy the configuration"; +const QUESTION: &str = "Which configuration file did you mean?"; +const CHOICE_A: &str = "io.toml"; +const CHOICE_B: &str = "io.local.toml"; +/// Deliberately unlike anything the loop writes on its own, so the assertion +/// about *where* it was filed cannot be satisfied by some other row. +const ACCOUNT: &str = "the charge landed, reference ch-9f21"; + +/// One `ask_question` call, with its arguments assembled as JSON and parsed. +/// +/// `ToolCall::arguments` is a `serde_json::Value`, which implements `FromStr`, so +/// the type is inferred from the field rather than named — the same route +/// `support::write_call` takes and for the same reason. +fn ask_call() -> ToolCall { + ToolCall { + name: ASK_QUESTION_TOOL.to_string(), + arguments: format!( + "{{\"question\":\"{QUESTION}\",\"choices\":[\"{CHOICE_A}\",\"{CHOICE_B}\"]}}" + ) + .parse() + .expect("the arguments were assembled as JSON and must parse as JSON"), + } +} + +/// A provider whose every completion asks the operator the same thing. +/// +/// It is called exactly once: the contract carries no responder, io-harness +/// stands in its own declining one, and the run pauses on the row the dispatch +/// has already written. A provider that could also finish would make the pause a +/// matter of timing rather than of the responder, which is the property these +/// fixtures rest on. +struct Asking; + +impl Provider for Asking { + async fn complete( + &self, + _request: CompletionRequest, + ) -> io_harness::Result { + Ok(CompletionResponse { + text: None, + tool_calls: vec![ask_call()], + ..Default::default() + }) + } +} + +/// A store on disk, a session over it, and one turn that really paused. +/// +/// On disk rather than in memory because one of the blocks below opens a second +/// handle on the same file, and an in-memory store is private to the connection +/// that made it. +struct Paused { + dir: tempfile::TempDir, + store: Store, + session: Session, + run_id: i64, + question_id: i64, +} + +impl Paused { + async fn new() -> Self { + let dir = tempfile::tempdir().expect("a temp dir"); + let store = Store::open(dir.path().join("runs.db")).expect("a store on disk"); + let mut session = Session::open(&store, dir.path()).expect("a session"); + let result = session + .turn_bounded( + &TaskContract::workspace(GOAL, dir.path()), + &Asking, + &store, + &Policy::permissive(), + &ApproveAll, + ) + .await + .expect("a turn that pauses is not a turn that failed"); + let (run_id, question_id) = match result.outcome { + RunOutcome::AwaitingAnswer { question_id, .. } => (result.run_id, question_id), + other => panic!("the turn was supposed to pause for an answer, and got {other:?}"), + }; + Self { + dir, + store, + session, + run_id, + question_id, + } + } + + /// The contract a resume of this run is driven under, rebuilt the way + /// `crate::resume` says a caller has to rebuild one. + fn contract(&self) -> TaskContract { + let goal = resume::goal_for(&self.store, self.run_id) + .expect("the store answers") + .expect("a session turn's goal is recoverable from its own prompt"); + TaskContract::workspace(goal, self.dir.path()) + } + + fn path(&self) -> std::path::PathBuf { + self.dir.path().join("runs.db") + } +} + +/// A store on disk and a bare run in it with `steps` committed steps. +/// +/// The steps are written with `Store::record`, which is the writer the step loop +/// itself calls, so `Store::last_step` answers about this run exactly as it would +/// about one whose process went away mid-loop — which, with no outcome recorded, +/// is what this run is. +fn bare_run(steps: u32) -> (tempfile::TempDir, Store, i64) { + let dir = tempfile::tempdir().expect("a temp dir"); + let store = Store::open(dir.path().join("runs.db")).expect("a store on disk"); + let run_id = store + .start_run("port the parser", &dir.path().display().to_string()) + .expect("a run row"); + for step in 1..=steps { + store + .record(run_id, &StepRecord::new(step, "wrote a file", "ok")) + .expect("a trace row"); + } + (dir, store, run_id) +} + +/// The steps a run's checkpoint markers of one kind name, in the order they were +/// written. +fn markers(store: &Store, run_id: i64, kind: &str) -> Vec { + store + .checkpoint_events(run_id) + .expect("the store answers") + .into_iter() + .filter(|event| event.kind == kind) + .map(|event| event.step) + .collect() +} + +/// Where a session's head points, read from the store rather than from a handle +/// that caches it. +fn head_of(store: &Store, session_id: i64) -> Option { + Session::reopen(store, session_id) + .expect("the session is still there") + .head() +} + +#[tokio::test] +async fn a_paused_question_is_reported_with_its_own_row_and_nothing_is_driven() { + let fixture = Paused::new().await; + let before = fixture + .store + .last_step(fixture.run_id) + .expect("a step count"); + + assert_eq!( + resume::pending_for(&fixture.store, fixture.run_id).expect("the store answers"), + Pending::Question { + question_id: fixture.question_id, + question: QUESTION.to_string(), + context: None, + choices: vec![CHOICE_A.to_string(), CHOICE_B.to_string()], + step: before, + }, + "the classification carries the row's own id and the agent's own words", + ); + assert!( + markers(&fixture.store, fixture.run_id, "resume").is_empty(), + "reading what a run stopped on must not drive it", + ); +} + +#[tokio::test] +async fn an_answer_drives_the_run_from_the_step_after_the_last_one_committed() { + let fixture = Paused::new().await; + // Read before anything is driven, and every number below is arithmetic on it. + // A literal here would pass on this fixture and mean nothing on a run that + // paused anywhere else, which is every run an operator actually has. + let before = fixture + .store + .last_step(fixture.run_id) + .expect("a step count"); + let head = fixture.session.head(); + + let resumed = resume::answer_question( + &fixture.contract(), + &Scripted::writing(&[]), + &fixture.store, + fixture.run_id, + fixture.question_id, + CHOICE_B, + &Policy::permissive(), + &ApproveAll, + None, + &Ignore, + head, + ) + .await + .expect("an answered question carries the run on"); + + assert_eq!( + resumed.resumed_after, before, + "the report names the step the run had reached when the answer was given", + ); + assert_eq!( + markers(&fixture.store, fixture.run_id, "resume"), + vec![before + 1], + "one resume marker, at the step after the last committed one", + ); + assert_eq!( + markers(&fixture.store, fixture.run_id, "skipped").len(), + before as usize, + "one skipped marker per step that was already committed and already paid for", + ); + assert_eq!( + fixture + .store + .last_step(fixture.run_id) + .expect("a step count"), + before + 1, + "the run really moved on", + ); +} + +#[tokio::test] +async fn a_resumed_turn_is_closed_with_the_outcome_and_the_reply_the_store_holds() { + let fixture = Paused::new().await; + let head = fixture.session.head(); + let turn_id = fixture + .store + .turn_for_run(fixture.run_id) + .expect("the store answers") + .expect("a session turn served this run"); + + // Before: the turn is closed with the pause, and that is what an operator + // scrolling back would still see if nothing here stitched it up. + let parked = fixture + .store + .session_turn(turn_id) + .expect("the store answers") + .expect("the turn"); + assert_eq!(parked.outcome.as_deref(), Some("awaiting_answer")); + assert_eq!(parked.reply, None); + + let resumed = resume::answer_question( + &fixture.contract(), + &Scripted::writing(&[]), + &fixture.store, + fixture.run_id, + fixture.question_id, + CHOICE_B, + &Policy::permissive(), + &ApproveAll, + None, + &Ignore, + head, + ) + .await + .expect("an answered question carries the run on"); + + assert_eq!(resumed.turn_id, Some(turn_id)); + assert!( + matches!(resumed.outcome, RunOutcome::Finished { .. }), + "the scripted provider says its piece and stops, and got {:?}", + resumed.outcome, + ); + + let closed = fixture + .store + .session_turn(turn_id) + .expect("the store answers") + .expect("the turn"); + assert_eq!( + closed.outcome.as_deref(), + Some("finished"), + "the turn carries the ending the store recorded, not the pause it was closed with", + ); + assert!( + closed.reply.as_deref().is_some_and(|said| !said.is_empty()), + "a resumed turn holds what the agent said, and got {:?}", + closed.reply, + ); + assert_eq!( + closed.reply, resumed.reply, + "the report and the row are the same reply rather than two readings of it", + ); + assert_eq!( + head_of(&fixture.store, fixture.session.id()), + Some(turn_id), + "the conversation continues from the turn that was carried on", + ); +} + +#[tokio::test] +async fn a_head_another_handle_moved_first_makes_the_head_write_refuse() { + let fixture = Paused::new().await; + let head = fixture.session.head(); + let session_id = fixture.session.id(); + let turn_id = fixture + .store + .turn_for_run(fixture.run_id) + .expect("the store answers") + .expect("a session turn served this run"); + + // A second handle on the same file — the shape io-harness's own lease suite + // uses. No second process is started and nothing waits on a clock: the race + // is decided by which connection writes first, and this one does. + let other = Store::open(fixture.path()).expect("a second handle on the same store"); + other + .set_session_head_if(session_id, head, None) + .expect("the other handle holds the head it expected, so its write lands"); + + let failure = resume::answer_question( + &fixture.contract(), + &Scripted::writing(&[]), + &fixture.store, + fixture.run_id, + fixture.question_id, + CHOICE_B, + &Policy::permissive(), + &ApproveAll, + None, + &Ignore, + head, + ) + .await + .expect_err("a head that moved under the resume must be reported, not overwritten"); + + assert!( + matches!( + failure, + Failure::HeadMoved { session_id: s, turn_id: t } if s == session_id && t == turn_id + ), + "the loser of the race is told which turn holds its answer, and got {failure:?}", + ); + + // The half that must survive being told the head did not move: the answer was + // given, the run was driven, and the turn holds what came back. A resume that + // rolled any of that back would destroy the one copy of what the model said. + let closed = fixture + .store + .session_turn(turn_id) + .expect("the store answers") + .expect("the turn"); + assert_eq!(closed.outcome.as_deref(), Some("finished")); + assert!(closed.reply.is_some()); + assert_eq!( + head_of(&fixture.store, session_id), + None, + "the head the other handle wrote stands; nothing here overwrote it", + ); +} + +#[tokio::test] +async fn a_question_somebody_else_answered_is_refused_before_the_run_is_driven() { + let fixture = Paused::new().await; + // What a second process attached to this run would have done. It is the + // dangerous case rather than a contrived one: the run has already acted on + // whatever this answer was, and driving it again from a second one is the + // silent double-answer the store's compare-and-swap exists to prevent. + assert!( + fixture + .store + .answer_question(fixture.question_id, CHOICE_A, "human") + .expect("the store answers"), + "the swap finds an unanswered question and takes it", + ); + + let failure = resume::answer_question( + &fixture.contract(), + &Scripted::writing(&[]), + &fixture.store, + fixture.run_id, + fixture.question_id, + CHOICE_B, + &Policy::permissive(), + &ApproveAll, + None, + &Ignore, + fixture.session.head(), + ) + .await + .expect_err("a question with an answer already on it cannot take a second"); + + let asked = fixture.question_id; + assert!( + matches!(failure, Failure::QuestionAnswered { question_id } if question_id == asked), + "the sentence names the question rather than the run, and got {failure:?}", + ); + assert!( + markers(&fixture.store, fixture.run_id, "resume").is_empty(), + "the refusal happens before anything drives", + ); +} + +#[tokio::test] +async fn an_interrupted_run_is_never_driven_and_is_pointed_at_fork_instead() { + let (dir, store, run_id) = bare_run(1); + // What `Ctrl+C` leaves behind: the observer stops the run and the loop writes + // this outcome, which `finish_run` then records as a *completed* status. + store + .finish_run(run_id, resume::CANCELLED) + .expect("the outcome is recorded"); + + assert_eq!( + resume::pending_for(&store, run_id).expect("the store answers"), + Pending::Interrupted, + "a cancelled run is the operator's own ending, not a pause", + ); + + let failure = resume::carry_on( + &TaskContract::workspace("port the parser", dir.path()), + &Scripted::writing(&[]), + &store, + run_id, + Some(&Policy::permissive()), + &ApproveAll, + None, + &Ignore, + None, + ) + .await + .expect_err("every resume entry point would return the original outcome having driven nothing"); + + assert!( + matches!(failure, Failure::Interrupted { run_id: r } if r == run_id), + "{failure:?}", + ); + assert!( + failure.to_string().contains("/fork"), + "the refusal names the neighbouring answer rather than only saying no: {failure}", + ); + // The assertion that matters. A driver that let this through would get `Ok` + // back carrying `Cancelled`, so a return value proves nothing; the absence of + // a resume marker proves the loop was never entered. + assert!( + markers(&store, run_id, "resume").is_empty(), + "nothing drove", + ); + assert_eq!(store.last_step(run_id).expect("a step count"), 1); +} + +#[tokio::test] +async fn a_run_whose_process_went_away_carries_on_from_its_last_committed_step() { + let (dir, store, run_id) = bare_run(2); + let before = store.last_step(run_id).expect("a step count"); + + assert_eq!( + resume::pending_for(&store, run_id).expect("the store answers"), + Pending::Died { last_step: before }, + ); + + let resumed = resume::carry_on( + &TaskContract::workspace("port the parser", dir.path()), + &Scripted::writing(&[]), + &store, + run_id, + Some(&Policy::permissive()), + &ApproveAll, + None, + &Ignore, + None, + ) + .await + .expect("a run with committed work and no ending is exactly what a resume is for"); + + assert_eq!(resumed.resumed_after, before); + assert_eq!( + markers(&store, run_id, "resume"), + vec![before + 1], + "the loop picks up after the last step that committed", + ); + assert_eq!( + resumed.turn_id, None, + "this run served no session turn, so there is no turn to close and that is not a failure", + ); + assert_eq!(resumed.reply, None); +} + +#[tokio::test] +async fn a_run_whose_boundary_nothing_recorded_is_refused_rather_than_given_one() { + let (dir, store, run_id) = bare_run(1); + + let failure = resume::carry_on( + &TaskContract::workspace("port the parser", dir.path()), + &Scripted::writing(&[]), + &store, + run_id, + None, + &ApproveAll, + None, + &Ignore, + None, + ) + .await + .expect_err("no recorded policy and none supplied means nobody knows what it could do"); + + assert!( + matches!(failure, Failure::NoRecordedPolicy { run_id: r } if r == run_id), + "{failure:?}", + ); + assert!( + markers(&store, run_id, "resume").is_empty(), + "the refusal happens before anything drives", + ); +} + +#[tokio::test] +async fn an_operators_account_of_an_interrupted_call_lands_on_the_step_it_was_made_on() { + let (dir, store, run_id) = bare_run(3); + // Written with `Store::open_attempt`, which is the writer the dispatch calls + // before every call it cannot replay — so this row is what a process dying + // mid-charge leaves behind, not an imitation of one. `Indeterminate` is the + // recovery class that journals at all; a `Replayable` call is not written. + let attempt_id = store + .open_attempt(run_id, 2, "charge_card", ToolRecovery::Indeterminate) + .expect("the store answers") + .expect("an indeterminate call is journalled"); + + assert_eq!( + resume::pending_for(&store, run_id).expect("the store answers"), + Pending::Recovery { + attempt_id, + tool: "charge_card".to_string(), + step: 2, + }, + ); + + let resumed = resume::recover( + &TaskContract::workspace("port the parser", dir.path()), + &Scripted::writing(&[]), + &store, + run_id, + attempt_id, + RecoveryDecision::Completed { + observation: ACCOUNT.to_string(), + }, + &Policy::permissive(), + &ApproveAll, + None, + &Ignore, + None, + ) + .await + .expect("a decided call lets the run carry on"); + + assert_eq!( + resumed.resumed_after, 3, + "the run had reached step 3, which is not the step the call was made on", + ); + + // The assertion is the step number. The text alone would be satisfied by an + // account filed wherever the run happened to be, which is a transcript in + // which a call made three steps ago was answered after the fact. + let filed: Vec = store + .observations(run_id) + .expect("the store answers") + .into_iter() + .filter(|observation| observation.text.contains(ACCOUNT)) + .map(|observation| observation.step) + .collect(); + assert_eq!( + filed, + vec![2], + "the operator's account belongs to the step the call was made on", + ); +} + +#[tokio::test] +async fn a_cancelled_plan_ends_the_run_without_re_entering_the_loop() { + let (dir, store, run_id) = bare_run(1); + let steps = vec![ + PlanStep::new("rewrite the parser"), + PlanStep::new("port the tests"), + ]; + // `Store::put_plan` is what the gate writes before it is consulted, which is + // the whole of io-harness's durability claim for a plan — so a row written + // this way is the row a process dying between the proposal and the verdict + // leaves behind. + let plan_id = store + .put_plan(run_id, 1, &Plan::new(steps.clone())) + .expect("a plan row"); + + assert_eq!( + resume::pending_for(&store, run_id).expect("the store answers"), + Pending::Plan { + plan_id, + steps, + step: 1, + }, + ); + + let resumed = resume::decide_plan( + &TaskContract::workspace("port the parser", dir.path()), + &Scripted::writing(&[]), + &store, + run_id, + plan_id, + PlanVerdict::Cancel, + &Policy::permissive(), + &ApproveAll, + None, + &Ignore, + None, + ) + .await + .expect("cancelling a plan is a decision, not a failure"); + + assert!( + matches!(resumed.outcome, RunOutcome::PlanRejected { .. }), + "a cancelled plan ends the run as rejected, and got {:?}", + resumed.outcome, + ); + assert_eq!( + store.outcome(run_id).expect("the store answers").as_deref(), + Some("plan_rejected"), + ); + assert!( + markers(&store, run_id, "resume").is_empty(), + "the loop is never re-entered, so the rest of the budget is not spent on \ + the approach that was just refused", + ); + assert_eq!( + store.last_step(run_id).expect("a step count"), + 1, + "no step was driven", + ); +} + +#[tokio::test] +async fn a_plan_belonging_to_another_run_is_refused_and_names_both_runs() { + let (dir, store, proposer) = bare_run(1); + let other = store + .start_run("write the release notes", &dir.path().display().to_string()) + .expect("a second run row"); + store + .record(other, &StepRecord::new(1, "read a file", "ok")) + .expect("a trace row"); + let plan_id = store + .put_plan( + proposer, + 1, + &Plan::new([PlanStep::new("rewrite the parser")]), + ) + .expect("a plan row"); + + let failure = resume::decide_plan( + &TaskContract::workspace("write the release notes", dir.path()), + &Scripted::writing(&[]), + &store, + other, + plan_id, + PlanVerdict::Approve, + &Policy::permissive(), + &ApproveAll, + None, + &Ignore, + None, + ) + .await + .expect_err("a verdict on somebody else's plan authorises somebody else's work"); + + assert!( + matches!( + failure, + Failure::PlanElsewhere { plan_id: p, owner, run_id } + if p == plan_id && owner == proposer && run_id == other + ), + "{failure:?}", + ); + assert!( + markers(&store, other, "resume").is_empty(), + "the run that was named was never driven", + ); + let untouched = store + .plan(plan_id) + .expect("the store answers") + .expect("the plan"); + assert!( + !untouched.resolved, + "and the plan that was named still has no verdict on it", + ); +} + +/// The driver read as text, because nothing here can link it. +/// +/// **Line endings normalised, and Windows is why.** A checkout on Windows gives +/// `\r\n`, so a needle written `"\n}\n"` — the one that finds where a function +/// ends — matches nothing there and the test panics on a green product. That is +/// the same defect 0.19.0 found when `include_str!` handed a frontmatter parser +/// CRLF: green on two platforms, red on the third, and the fault in the test +/// rather than in the code it guards. +fn driver() -> String { + std::fs::read_to_string(std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src/main.rs")) + .expect("the driver") + .replace("\r\n", "\n") +} + +/// The four properties of `resume_pending` that the adversarial review found +/// broken, asserted the only way this crate can assert anything about the driver. +/// +/// **Every one of these shipped green.** `src/main.rs` is not linked by any +/// integration test — the crate says so in five places — so a defect in the +/// resume loop is invisible to the 1,215 tests around it. What is checkable is +/// the driver's *text*, which is how `tests/context_share.rs`, +/// `tests/contract.rs` and `tests/compact.rs` already hold it to account. These +/// are weak assertions by construction; they are also the only ones available, +/// and each names a defect that was real. +#[test] +fn the_resume_loop_keeps_the_four_properties_the_review_found_missing() { + let driver = driver(); + let from = driver + .find("async fn resume_pending") + .expect("the driver resumes"); + let loop_body = &driver[from..]; + + // **A named binding lives; only `_` drops.** Written `_parked`, the + // responder's receiver stayed open, so a resumed run that asked a second + // question sent into a channel nobody drains and awaited a reply nobody + // would send — the session froze with nothing on screen and no way out. + assert!( + loop_body.contains("let (answerer, _) = io_cli::intent::channel();"), + "the resumed run's responder receiver must be dropped at once, or a second \ + question hangs the process", + ); + + // Without this the mode stays `Idle`, and at Idle `App::compose` does not + // queue: a prompt typed during a resumed run was taken out of the composer + // and destroyed, and `Ctrl+C` took the quit branch and did nothing. + assert!( + loop_body.contains("app.started();"), + "a resumed run must tell the interface a run is in flight, or a typed prompt \ + is destroyed and the stop key lies", + ); + + // The stop key has to reach the observer's flag, exactly as a live turn's does. + assert!( + loop_body.contains("canceller.store(true"), + "the stop key must reach the run, or a resumed run cannot be stopped at all", + ); + + // A `Some(Event::Key(_))` arm consumes a `Resize` and drops it, leaving the + // `Screen` believing in a width the terminal no longer has. + assert!( + loop_body.contains("Event::Resize(width, height)"), + "the resume loop must handle a resize, as both sibling loops in the driver do", + ); +} + +/// The `/resume` lock is released on **every** path that stops holding it. +/// +/// The leak the review found: entering a session, then resuming back into the one +/// this process opened at startup, returned early on "that pid is ours" without +/// clearing the guard — so the process went on holding a lock and an owner record +/// for a conversation it had left, and a second `io` was refused and sent to a +/// terminal showing something else. +#[test] +fn entering_a_session_releases_the_lock_on_the_one_being_left() { + let driver = driver(); + let from = driver + .find("fn entering(") + .expect("the driver takes the lock"); + let body = &driver[from..]; + let end = body.find("\n}\n").expect("the function ends"); + let body = &body[..end]; + + // Three paths leave this function without acquiring: ours-already, an + // ordinary filesystem failure, and the refusal. The first two must clear the + // guard; the refusal keeps it, because the switch does not happen. + let cleared = body.matches("*held = None;").count(); + assert_eq!( + cleared, 2, + "both non-acquiring paths that still switch session must drop the guard for \ + the session being left; found {cleared}", + ); + assert!( + body.contains("*held = Some(guard);"), + "and the acquiring path replaces it, which drops the old one in the same statement", + ); + // The owner record must name the session being entered, not the one being + // left — otherwise a second `io`'s refusal points at the wrong directory. + // `.session_root(id)` and not `store.session_root(id)`: rustfmt breaks the + // receiver onto its own line, so the joined-up spelling matches nothing. A + // needle is re-pointed at the shape the code actually has, never widened + // until it matches something. + assert!( + body.contains(".session_root(id)"), + "the root written into the owner record must be read for the session being entered", + ); +} diff --git a/tests/rewind.rs b/tests/rewind.rs index 577ef5f..ef647da 100644 --- a/tests/rewind.rs +++ b/tests/rewind.rs @@ -32,12 +32,22 @@ //! back. //! //! * **The two-turn block** catches an undo that moves files without moving the -//! conversation. Sabotage it by deleting the `set_session_head` call: the head +//! conversation. Sabotage it by deleting the `set_session_head_if` call: the head //! still points at the undone turn, so the next turn is assembled from a history //! containing a prompt whose work no longer exists. //! +//! * **The head-race block** catches an undo that writes the head *un*conditionally +//! — one store serves this whole machine, so two `io` processes in two terminals +//! is ordinary, and the second one's turn is then written off the head path while +//! still sitting in `session_turns`, answered, parented and paid for. Sabotage it +//! by restoring `Store::set_session_head`: **nothing errors, nothing prints, and +//! only the assertion on the refusal goes red**, which is why that assertion is +//! the one this block is built around rather than the state it leaves behind. Its +//! companion block, over a store nobody raced, catches the opposite mistake of +//! passing an `expected` that never matches and refusing every undo. +//! //! * **The only-turn block** is F10 and the one that matters most. Sabotage it by -//! reaching for `Session::branch_from` instead of `set_session_head` — the whole +//! reaching for `Session::branch_from` instead of `set_session_head_if` — the whole //! plausible-looking implementation — and it cannot even be written, because //! there is no parent turn to branch from. Sabotage it by keeping the session //! value instead of re-opening it and `session.head()` answers with the undone @@ -81,7 +91,7 @@ mod support; use io_cli::glyphs::UNICODE; use io_cli::rewind; use io_cli::theme::Tone; -use io_harness::{ApproveAll, MemoryKind, Policy, Session, Store}; +use io_harness::{ApproveAll, Error, MemoryKind, Policy, Session, Store}; use support::Scripted; /// A temporary workspace, an in-memory store, and a session over the two. @@ -135,6 +145,34 @@ impl Fixture { } } +/// Where a shared fixture keeps its database, under the workspace it also serves. +/// Never restored by a rewind: only paths a run took a snapshot of are visited. +const DB: &str = "store.sqlite3"; + +impl Fixture { + /// The same fixture over a **file-backed** store, which is what makes a head + /// race expressible at all. `Store::memory` cannot be shared — a second + /// handle opens its own empty database, so nothing it does is visible here + /// and no race can be staged. One file, two handles, one process, no sleeping. + fn on_disk() -> Self { + let dir = tempfile::tempdir().expect("a temp dir"); + let store = Store::open(dir.path().join(DB)).expect("a store on disk"); + let session = Session::open(&store, dir.path()).expect("a session"); + Self { + dir, + store, + session, + } + } + + /// The other terminal's `io`: a second handle on the same database. This + /// product keeps one store for the whole machine, so this is the ordinary + /// arrangement rather than an exotic one. + fn other_io(&self) -> Store { + Store::open(self.dir.path().join(DB)).expect("a second handle on the same file") + } +} + const BEFORE: &str = "what was there first\n"; const AFTER: &str = "what the agent wrote instead\n"; const INVENTED: &str = "a file the agent made up\n"; @@ -448,6 +486,107 @@ async fn rewinding_the_only_turn_leaves_the_session_with_no_head_at_all() { ); } +#[tokio::test] +async fn an_undo_is_refused_when_another_io_has_already_moved_the_head_on() { + let mut fixture = Fixture::on_disk(); + fixture + .turn_writing("draft a plan", &[("plan.md", BEFORE)]) + .await; + let mine = fixture.session.head().expect("this process took a turn"); + + // The other terminal, doing the most ordinary thing there is: its own handle + // on the same database, a turn parented on the head it read, and the head + // moved on to it. That is `Session::turn`'s own sequence with the provider + // taken out — the turn is real, in `session_turns`, and answered for. + let other = fixture.other_io(); + let workspace = fixture.workspace(); + let run = other + .start_run("what the other terminal asked", &workspace) + .expect("a run row"); + let theirs = other + .record_turn( + fixture.session.id(), + Some(mine), + run, + "and now the other thing", + ) + .expect("a turn row"); + other + .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) + .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 + // block defends against is the unconditional `Store::set_session_head`, which + // fails with no error, no message and no mark on screen. Restore it and this + // line is the only thing in the suite that goes red. + match &refused { + Error::Conflict { + run_id, + owner, + expires_at, + } => { + assert_eq!( + *run_id, + fixture.session.id(), + "the conflict carries the session whose head moved", + ); + assert!( + owner.is_empty() && expires_at.is_empty(), + "a head has a value that moved, not a holder with a lease: \ + {owner:?} until {expires_at:?}", + ); + } + unexpected => panic!("the loser of a head race must be told it lost, got {unexpected:?}"), + } + + // And the other process's turn is still on the head path — which is the whole + // of what was at stake. `Session::history` walks parent links back from the + // head, so a turn written off the head is one no later prompt can ever see + // again, however correctly it is parented and however much it cost. + let reread = + Session::reopen(&fixture.store, fixture.session.id()).expect("the session is readable"); + assert_eq!( + reread.head(), + Some(theirs), + "the head must be left exactly where the other `io` put it", + ); + assert_eq!( + reread + .history(&fixture.store) + .expect("the history is readable") + .len(), + 2, + "both turns stay on the head path; neither process's work is orphaned", + ); +} + +/// The other half of the conditional write: it must refuse a moved head **and** +/// still perform an undo nobody raced. An implementation that passed the wrong +/// `expected` — a turn id that never matches, say — would refuse every undo, and +/// the refusal block above would stay green while `/undo` had stopped working. +#[tokio::test] +async fn an_undo_over_a_store_no_other_io_touched_still_moves_the_head_back() { + let mut fixture = Fixture::on_disk(); + fixture + .turn_writing("draft a plan", &[("plan.md", BEFORE)]) + .await; + let first = fixture.session.head().expect("the first turn is the head"); + fixture + .turn_writing("now do it", &[("plan.md", AFTER)]) + .await; + + let undone = rewind::last_turn(&mut fixture.session, &fixture.store) + .expect("nothing raced, so the conditional write must match its row") + .expect("a turn was taken"); + + assert_eq!(undone.head, Some(first)); + assert_eq!(fixture.session.head(), Some(first)); + assert_eq!(fixture.bytes("plan.md"), BEFORE.as_bytes()); +} + #[test] fn a_session_that_has_taken_no_turns_undoes_nothing_and_does_not_fail() { let mut fixture = Fixture::new(); diff --git a/tests/sessions.rs b/tests/sessions.rs index 29ee671..5ae51ca 100644 --- a/tests/sessions.rs +++ b/tests/sessions.rs @@ -36,16 +36,45 @@ //! `Picker` with a query typed into it, because the index is only interesting once //! the drawn order and the caller's order have been made to disagree. //! +//! **F1, the half added this release — every row says what its session stopped +//! on.** The list is chosen from, and until now every row looked alike: the +//! session holding an unanswered question was indistinguishable from the twenty +//! that finished, and the operator learned which was which by opening them. The +//! state comes off the session's newest run through `io_cli::resume::pending_for` +//! and is drawn on `Row::mark` — **not** in the label, which is the only field the +//! fuzzy matcher ranks, and **not** in the detail, which is the first thing a +//! narrow terminal drops. The tests below assert the mark is present *and* that +//! the same string is absent from both of the other two fields, because "it is on +//! the row somewhere" is what 0.16.0 asserted about a template and a skill, and +//! the distinction vanished at exactly the width that needed it. +//! +//! **F7 — the one pause that is reported and must never be offered.** A turn the +//! operator interrupted is recorded `cancelled`, which `finish_run` maps to a +//! *completed* status, and every io-harness resume entry point short-circuits on +//! it. So the most common way a turn stops is the one way it cannot be continued. +//! It gets a mark and a sentence naming `/fork` from the turn before, and the test +//! asserts the sentence does not offer to resume. +//! +//! **The stores here are real files.** These fixtures reach for `Store::open` on a +//! `TempDir` rather than `Store::memory`, because what is being read back is what +//! a run loop leaves in a database another process wrote — and where io-harness +//! exposes no way to *reach* a state, the fixture writes the row through the +//! public writer the run loop itself calls, so the row is the one production +//! produces rather than one shaped to suit the assertion. +//! //! **No clock.** `Recent::at` is the store's own stamp, sliced. It is not a //! relative age, precisely so that neither the module nor this file has to read a //! clock — which `tests/timing.rs` forbids outright, and which a resume picker is //! the most tempting surface yet shipped to break. use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; -use io_cli::glyphs::UNICODE; +use io_cli::glyphs::{ASCII, UNICODE}; use io_cli::picker::{Outcome, Picker, Row}; -use io_cli::sessions::{cut_note, pick, pick_turn, recent, rows, turn_rows, MAX_RUNS_SCANNED}; -use io_harness::Store; +use io_cli::sessions::{ + cut_note, mark, note, pick, pick_turn, recent, rows, turn_rows, DIED_MARK, ENDED_MARK, + MAX_RUNS_SCANNED, PLAN_MARK, QUESTION_MARK, RECOVERY_MARK, +}; +use io_harness::{Plan, PlanStep, Question, StepRecord, Store, ToolRecovery}; /// More sessions than the twenty-row cap 0.7.0 removed, so a list of this length /// is itself the evidence the cap is gone. A literal rather than a constant: the @@ -99,12 +128,49 @@ fn add_turn_from(store: &Store, session: i64, parent: Option, prompt: &str) store .finish_turn(turn, Some("done"), "completed") .expect("a turn finishes"); + // The RUN is closed as well as the turn, which this fixture did not do until + // the state column existed. A run nothing ever finished is `running` in the + // store — which is what a process that died mid-loop leaves behind — and it + // reads as finished here only because it committed no step and so has nothing + // to be resumed *from*. That is one accident away from every session in this + // file drawing a `died` mark for reasons no test in it is about. The product + // closes the run when the turn returns; so does this now, and the fixture + // stops depending on a step count it never meant to assert. + store + .finish_run(run, "success") + .expect("the run finishes with it"); store .set_session_head(session, Some(turn)) .expect("the head moves to the newest turn"); turn } +/// One more turn on `session`, with its run left **open** and its id returned. +/// +/// The seam every state below is built through. A run is the thing a pause is +/// written under — the question, the plan and the tool journal all key on +/// `run_id`, none of them on a turn — so a fixture that could not name the newest +/// run could not put a session into any of these states at all. +/// +/// The turn is deliberately not finished: a turn whose run is still waiting has +/// not returned anything, and finishing it here would seed a shape the product +/// never writes. +fn open_run(store: &Store, session: i64, prompt: &str) -> i64 { + let parent = store + .session_turns(session) + .expect("a session's turns are readable") + .last() + .map(|turn| turn.id); + let run = store.start_run(prompt, "io-cli").expect("a run starts"); + let turn = store + .record_turn(session, parent, run, prompt) + .expect("a turn records"); + store + .set_session_head(session, Some(turn)) + .expect("the head moves to the newest turn"); + run +} + /// A session over `root` that has already run one turn per prompt, in order. fn seed(store: &Store, root: &str, prompts: &[&str]) -> i64 { let session = store.create_session(root).expect("a session opens"); @@ -118,6 +184,23 @@ fn store() -> Store { Store::memory().expect("an in-memory store opens") } +/// A store in a real file, and the directory that has to outlive it. +/// +/// `Store::open` rather than `Store::memory` for the state tests: what they read +/// back is what a run loop committed to a database, and the writers those states +/// go through — `put_question`, `put_plan`, `open_attempt` — are the ones whose +/// whole purpose is surviving the process that wrote them. Reading them out of a +/// connection that never touched a disk would be testing the half of the claim +/// that was never in doubt. +/// +/// The `TempDir` comes back with the store because dropping it deletes the +/// database underneath. +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) +} + #[test] fn f1_a_session_that_never_ran_a_turn_is_not_offered() { let store = store(); @@ -242,6 +325,293 @@ fn f1_the_turn_count_includes_the_turns_a_branch_left_behind() { ); } +/// Six sessions, one per state a newest run can be found in, seeded through the +/// writers the run loop itself calls. +/// +/// Returned as pairs of session id and the mark that state calls for, in seeding +/// order, so the assertions read as a table rather than as six copies of one +/// paragraph. A state missing from this fixture is a state nothing watches — and +/// the way a state goes wrong is by being drawn as another state's mark, which +/// only a fixture holding all of them at once can catch. +fn seed_every_state(store: &Store) -> Vec<(i64, Option<&'static str>)> { + // Waiting on an answer. `put_question` is the writer `run/dispatch.rs` calls + // when nothing in the process could answer, and `awaiting_answer` is the + // outcome the loop then finishes with. io-harness exposes no way to *reach* + // this state without running a model, so the row is written through the + // public writer that produces it — which is what makes it the row production + // leaves rather than one shaped to suit the assertion. + let asking = seed(store, "/work/importer", &["port the importer"]); + let run = open_run(store, asking, "which database?"); + store + .put_question(run, 4, &Question::new("postgres or sqlite?")) + .expect("a question is written"); + store + .finish_run(run, "awaiting_answer") + .expect("and the run stops on it"); + + // Waiting on a verdict about a plan. The plan row is written *before* the + // gate is consulted, which is the whole of io-harness's durability claim for + // it: a process that died between the proposal and the verdict leaves a row a + // human can still answer, and that row is what this list has to see. + let planning = seed(store, "/work/migration", &["rewrite the migration"]); + let run = open_run(store, planning, "here is how I would do it"); + store + .put_plan( + run, + 2, + &Plan::new([ + PlanStep::new("read the schema"), + PlanStep::new("write the up and down"), + ]), + ) + .expect("a plan is written"); + store + .finish_run(run, "awaiting_plan") + .expect("and the run stops on it"); + + // Waiting on a decision about a call io-harness cannot inspect. There is no + // outcome to write and no status to set here: the pause *is* the open row in + // the tool journal. `open_attempt` commits it outside every step transaction + // precisely so it survives the death of the process making the call, and + // `run/outcome.rs` reads exactly this back at each resume root. + let recovering = seed(store, "/work/billing", &["settle the invoice"]); + let run = open_run(store, recovering, "charge the customer"); + store + .open_attempt(run, 6, "charge_card", ToolRecovery::Indeterminate) + .expect("the journal takes the attempt") + .expect("an indeterminate call is journalled at all"); + + // Running, with a committed step and nothing pending: the process died + // mid-loop and nothing ever closed the row. The step is written rather than + // implied because it is what the run would resume *from* — a crashed run with + // no committed work and one with six steps behind it are not the same offer. + let crashed = seed(store, "/work/archive", &["reindex the archive"]); + let run = open_run(store, crashed, "carry on with the reindex"); + store + .record(run, &StepRecord::new(3, "wrote the index", "ok")) + .expect("a step commits"); + + // Finished, cleanly. The row that carries no mark at all. + let finished = seed(store, "/work/changelog", &["write the changelog"]); + + // Ended by the operator. `Ctrl+C` sets the flag that returns `Flow::Cancel`, + // io-harness records the outcome `cancelled`, and `finish_run` maps that to a + // **completed** status — which is why every resume entry point short-circuits + // on it, why it cannot be continued, and why it is reported rather than + // offered. + let ended = seed(store, "/work/columns", &["rename the columns"]); + let run = open_run(store, ended, "and now the indexes"); + store + .finish_run(run, "cancelled") + .expect("an interrupted turn ends"); + + vec![ + (asking, Some(QUESTION_MARK)), + (planning, Some(PLAN_MARK)), + (recovering, Some(RECOVERY_MARK)), + (crashed, Some(DIED_MARK)), + (finished, None), + (ended, Some(ENDED_MARK)), + ] +} + +#[test] +fn f1_each_row_carries_the_mark_its_last_run_state_calls_for() { + let (_dir, store) = on_disk(); + let expected = seed_every_state(&store); + + let (list, cut) = recent(&store).expect("the list reads"); + assert!(!cut, "six sessions is the whole store"); + assert_eq!( + list.len(), + expected.len(), + "one row per session, whatever state its newest run stopped in — a state \ + this list cannot represent must not become a session it drops: {list:?}", + ); + + // Both glyph sets, because a mark is TEXT and not a glyph. It has to say the + // same thing to a terminal that can draw nothing else, which is the whole + // reason the state rides in `Row::mark` as a word rather than as a colour or a + // symbol on the row. + for glyphs in [UNICODE, ASCII] { + let built = rows(&list, 100, &glyphs); + for (id, want) in &expected { + let index = list + .iter() + .position(|session| session.id == *id) + .unwrap_or_else(|| panic!("session {id} is listed, under {}", glyphs.name)); + let row = &built[index]; + assert_eq!( + row.mark, *want, + "the row for session {id} carries the mark its state calls for, \ + under {}: {row:?}", + glyphs.name, + ); + assert_eq!( + mark(&list[index].pending), + *want, + "and the row is drawn from `mark`, so the two cannot drift apart", + ); + // The completed session is the one asserted to carry NOTHING, and it + // is the assertion with teeth: a mark saying *fine* would be on almost + // every row of almost every list, and a mark that is nearly always + // there is one nobody reads. The absence is the signal. + assert_eq!( + note(&list[index].pending).is_some(), + want.is_some(), + "the prose and the mark report the same set of states, so a \ + surface with room for a sentence and one without agree: {:?}", + list[index].pending, + ); + } + + // **The criterion.** The mark is present in the one field that survives, + // and absent from both of the others. In the label it would be ranked by + // the fuzzy matcher — every waiting session sharing a first word, and a + // query for `plan` answering with the sessions *stopped on* a plan rather + // than the ones asked about one. In the detail it would be the first thing + // dropped on a narrow terminal, which is where a row is hardest to tell + // apart: 0.16.0 marked a template and a skill in the detail alone and the + // distinction vanished at exactly the width that needed it. + for row in &built { + let Some(mark) = row.mark else { continue }; + assert!( + !row.label.contains(mark), + "the state must not be in the label, which is what the matcher \ + ranks: {row:?}", + ); + let detail = row.detail.as_deref().expect("a session row has a detail"); + assert!( + !detail.contains(mark), + "the state must not be in the detail, which is the first thing a \ + narrow terminal drops: {row:?}", + ); + } + } + + // Every mark is ASCII, by the same rule the memory page's four are held to: a + // mark is text rather than a colour, so it has to survive `NO_COLOR`, + // `--plain` and a terminal whose locale does not claim UTF-8 unchanged. + for (_, want) in &expected { + let Some(mark) = want else { continue }; + assert!( + mark.chars().all(|character| character.is_ascii_graphic()), + "a mark that cannot be drawn is a state nobody is told about: {mark:?}", + ); + } +} + +#[test] +fn f7_a_turn_the_operator_ended_is_reported_and_never_offered_as_resumable() { + let (_dir, store) = on_disk(); + let session = seed(&store, "/work/columns", &["draft the release notes"]); + let run = open_run(&store, session, "and now rewrite the summary"); + store + .finish_run(run, "cancelled") + .expect("an interrupted turn ends"); + + let (list, _) = recent(&store).expect("the list reads"); + let pending = &list[0].pending; + + assert_eq!( + mark(pending), + Some(ENDED_MARK), + "an ended turn is REPORTED — the alternative is a row that looks finished \ + and a piece of work the operator believes they stopped and cannot find", + ); + + let note = note(pending).expect("an ended turn has something to say"); + assert!( + note.contains("you ended"), + "the sentence says who ended it: the operator did this, and a passive \ + sentence would read as something that went wrong: {note:?}", + ); + assert!( + note.contains("/fork"), + "and it names the neighbouring answer that actually works, from the turn \ + before — otherwise the report is a dead end: {note:?}", + ); + // The half with teeth. `cancelled` maps to a COMPLETED status in the store and + // every io-harness resume entry point short-circuits on a completed run, + // returning the original outcome without driving. So a sentence offering to + // resume would be offering something that cannot happen, and the operator + // would learn it by choosing the row and watching nothing occur. + assert!( + !note.to_lowercase().contains("resum"), + "an ended turn must never read as an offer to resume: {note:?}", + ); +} + +#[test] +fn f1_a_cut_list_still_marks_its_rows_and_its_note_still_stands_for_no_session() { + let (_dir, store) = on_disk(); + + // Seeded first, so it is the oldest and sits past the scan window. + let buried = seed(&store, "/work/buried", &["the session nobody reaches"]); + + // Runs that served no turn — what a headless run leaves — between the buried + // session and the one on screen. They are skipped by the walk and still + // COUNTED by it, which is the cheap way to reach the bound: the ceiling + // charges for looking at a run, not for finding a session behind it. + for n in 0..MAX_RUNS_SCANNED { + store + .start_run(&format!("headless {n}"), "io-cli") + .expect("a run starts"); + } + + // And the newest run of all is a session waiting on an answer, so the one row + // that survives the bound is a MARKED one. A cut list is exactly when the + // operator most needs to know what is on it. + let waiting = seed(&store, "/work/importer", &["port the importer"]); + let run = open_run(&store, waiting, "which database?"); + store + .put_question(run, 4, &Question::new("postgres or sqlite?")) + .expect("a question is written"); + store + .finish_run(run, "awaiting_answer") + .expect("and the run stops on it"); + + let (list, cut) = recent(&store).expect("the list reads"); + let ids: Vec = list.iter().map(|session| session.id).collect(); + + assert_eq!( + list.len(), + 1, + "only the session inside the window: {list:?}" + ); + assert_eq!(list[0].id, waiting); + assert!( + !ids.contains(&buried), + "a session past five hundred runs is beyond the walk", + ); + assert!(cut, "the walk gave up, so the list is cut"); + + let mut built = rows(&list, 80, &UNICODE); + assert_eq!( + built[0].mark, + Some(QUESTION_MARK), + "the surviving row still says what it is waiting on: {built:?}", + ); + + // The note is appended by the driver as an ordinary row, and it is not a + // session: no mark, because it has no state, and no id, because there is + // nothing behind it to reopen. The filter ranks it against the session rows + // like any other row and can put it under the marker, so `pick` has to keep + // answering `None` for it however the rows above it are now drawn. + built.push(Row::new( + cut_note(cut, built.len()).expect("a cut list carries a note"), + )); + assert_eq!( + built[1].mark, None, + "the note stands for no session, so it carries no session's state", + ); + assert_eq!( + pick(&ids, 1), + None, + "and it resolves to nothing, which the driver answers with a sentence", + ); +} + #[test] fn f10_the_list_is_as_long_as_the_walk_found_not_twenty() { let store = store();