diff --git a/.gitignore b/.gitignore index 3cfa106..ff38111 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,8 @@ node_modules/ # gate evidence are the maintainer's working notes, written by tooling rather # than by hand. They are not a contributor-facing surface and are never committed. .ultraship/ + +# The live rehearsal harness, copied to the root from the gitignored evidence +# directory to run `cargo test --test live`. It briefly entered history in +# 0.24.0 because a `git add -A` ran while a copy was sitting here. +/drive.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 06cbd3c..e208e72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,44 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +## [0.24.0] - 2026-08-28 + +A turn proves its work instead of asserting it. io-harness has carried a +verification pillar since long before this interface existed — a contract holds a +criterion, the run executes it after the agent stops, and the run comes back as +`Success` rather than `Finished` when it passed. io-cli has never once supplied +one, which is why every clean run this product has ever reported said `finished` +and meant "the agent stopped", not "the work holds up". + +**You say what done means for this repository, in the repository's own +language.** A command that must exit zero, a file that must exist and say +something, or a rubric a second model answers. The command is proposed from what +the repository actually is: io-harness reads its marker files and names its own +test command, so a Rust checkout offers `cargo test`, a `package.json` offers +whatever its lockfile implies, and a repository with no marker offers nothing +rather than guessing. io-cli holds no list of test commands and no list of marker +files. + +**The verdict is on screen while it happens.** The status line carries the gate's +standing and, once there has been more than one, which attempt this is. After a +gated turn the scrollback takes the phase that ran, what it answered and what the +command printed — a gate that failed because the tests went red reads differently +from one that failed because the policy would not let the program run at all. + +**A failing gate sends the agent back to work with the failure in hand**, up to a +retry budget you set and defaulting to one. The harness's own loop does not do +this: it records the failure and takes another step without telling the model +anything, so a run can spend its whole budget failing the same gate for the same +reason. What io-cli does instead is drive a follow-up turn carrying the criterion +and the output it produced. + +**`io exec` gains exit `6`** — the agent finished and the work does not hold up. +It is distinct from `5`, the agent stopping without finishing, and from `3`, the +ceiling a failing gate used to hide behind. No existing code changed meaning. + +Also: `/mcp` can edit a server at last, closing a limitation this product has +carried and stated since 0.21.0. + ## [0.23.0] - 2026-08-28 A run that paused is answered rather than abandoned. Everything needed to do it diff --git a/Cargo.lock b/Cargo.lock index d0a0488..d2affcc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1850,7 +1850,7 @@ dependencies = [ [[package]] name = "io-cli" -version = "0.23.0" +version = "0.24.0" dependencies = [ "clap", "crossterm", diff --git a/Cargo.toml b/Cargo.toml index f323fe2..14573d0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "io-cli" -version = "0.23.0" +version = "0.24.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 0af535c..d2c4611 100644 --- a/README.md +++ b/README.md @@ -493,6 +493,7 @@ above a row that ranked there for reasons having nothing to do with it. | `/mcp` | the MCP servers configured, and what this session has seen of each | | `/provider` | the providers configured, in the order a turn tries them | | `/plugin` | the capability bundles loaded, what each contributed, and the ones that failed | +| `/gates` | the check a turn must pass before it is done: a command, a file, or a rubric | | `/import` | bring instructions, MCP servers, skills and a model across from another agent | `/plugin` is new in 0.20.0 and sits beside those two because it is the third @@ -505,6 +506,16 @@ 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). +`/gates` is new in 0.24.0 and is under **configure** for that same reason and not +because of what its first screen shows. It opens on the criterion in force and the +last turn's verdict, which reads like an inspection — and then writes +`[app.io-cli.gates]`, which is the one thing **inspect** promises it never does. +That promise is worth more here than it is for a server list: a gate set by +accident does not merely change what the next turn talks to, it can spend a whole +extra turn against a real model deciding the first one was not finished. It sits +before `/import` because it is one you come back to. See [Verification +gates](#verification-gates). + `/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 @@ -1100,6 +1111,104 @@ default. Bare `/plan` says which one you are in and changes nothing. `[app.io-cli.containment]`, so configuring a fan-out silently made every turn stop and propose first. If that is what you wanted, `/plan on` is where it lives now. +## Verification gates + +**An agent that stops is not an agent that is done.** Every release before 0.24.0 +took the model's own word for it: the turn ended, the interface said so, and +whether the tests still passed was a question you asked afterwards. A gate is +where you say what "done" means *for this repository*, once, and the turn is not +finished until the criterion passes or the retry budget is spent. + +`/gates` writes it and shows what the last turn was judged on. It is a section of +your configuration file like any other, so it can also be typed by hand: + +```toml +[app.io-cli.gates] +command = ["cargo", "test", "--all"] +retries = 1 +``` + +There are three kinds and you get exactly one, because a `TaskContract` in +io-harness holds one `Verification` and not a list. Naming none, or naming two, is +refused where you can still see what you typed rather than silently picking a +winner: + +- **A command** that must exit a status you name — zero unless you say otherwise. + It is an argv and never a shell line, because io-harness checks `argv[0]` + against your permission boundary and runs it without a shell. This is the cheap + kind: it costs a process, it is objective, and it is the same thing you would + have run yourself. +- **A file** that must exist, and optionally must contain some text. Nearly free + and deliberately narrow — it answers "did the change actually get written down", + which is the failure a passing test suite is worst at catching. +- **A rubric** a second model answers: a sentence saying what the work has to be, + judged by a reviewer you name. + +**io-cli holds no list of test commands.** `/gates` offers you the one the +repository's own toolchain proposes — io-harness detects that from the project, so +a Rust checkout is offered a `cargo` line and a Node one is not — and accepting the +offer writes that command into your file, where you can read it and change it. What +is written is always a concrete argv: `command` is the program and its arguments, +never a shell line and never blank. That detection is the dependency's, and it is +deliberately not reimplemented here: a table of build tools inside this crate would +be a second opinion that goes stale the first time somebody's project does not look +like the ones it was written against. + +**The criterion is run by io-harness, in the sandbox, and not by io-cli.** That is +not squeamishness. A criterion run from here could not be handed the cache +directories a real run gets from the detected toolchain, so a `cargo test` gate +would fail on a registry write that io-harness's own gate would have allowed. It +also keeps a rule worth keeping: exactly one module in io-cli starts a process at +all, and it is the one behind `!`. + +The single exception is a `file` criterion with no `contains`. io-harness has no +criterion for bare existence — the nearest one treats a missing file and an empty +needle as a pass — so io-cli answers that one itself, with the reader that tells a +missing file from an empty one. It runs no process to do it, and it is the reason +that criterion costs nothing at all. + +**`retries` defaults to 1, and `0` means report-only.** A failing gate sends the +agent back to work with the failure text — the compiler's output, the missing +file, the reviewer's sentence — because the failure *is* the instruction, and an +agent told "it did not pass" without being told what did not pass is being asked +to guess. One retry is the default because a retry is a whole turn against a real +model and not a loop counter; if you want several, say so. Set `retries = 0` and +the verdict is drawn and recorded and nothing is re-driven, which is what you want +in a run you are watching and what you want in a run you are only measuring. + +**The criterion runs after every step, not once when the turn ends — and for a +rubric every one of those is a billed completion.** This is the number to know +before you configure anything. io-harness evaluates the contract's criterion at the +bottom of its step loop and keeps going until it passes, so a turn that takes nine +steps runs your command nine times, and a rubric on that turn is nine calls to the +reviewer rather than one. It is not "the agent finishes and then the work is +checked": it is checked continuously, and the run ends the moment the check +succeeds. + +That is what makes a command criterion worth choosing carefully. `cargo test --all` +after every step of a long turn is a great deal of compilation, and a narrow +command — one test, one binary, one lint — is usually the right gate. For a rubric +the cost is money rather than time, which is the reason the three kinds are named +in `/gates`'s own one-line description rather than left to the surface. The call is +io-harness's, so it lands in the run's usage like any other and `/cost` counts it. + +**A rubric needs a `reviewer`, and it is refused without one.** io-harness answers +a missing reviewer with a configuration error at run start — before the first +billed call, on every turn, in a place on screen nowhere near the keystroke that +caused it — so `/gates` refuses it while you are still looking at what you typed. +The reviewer is also never defaulted to the model doing the work. A model marking +its own paper is a decision rather than a convenience, and it is spelled +`allow_self_review = true`; without it, naming the working model as the judge is +the second refusal. + +**What a gate is not is a test runner.** io-cli does not discover your tests, does +not parse their output, and does not decide what a passing suite looks like. It +carries one criterion on the contract and reports the verdict io-harness came +back with. + +`io exec` gains exit `6` for this — the agent finished and the work does not hold +up. See [Exit status](#exit-status). + ## Pictures **Drag a picture onto the prompt, or copy it and paste.** That is the whole of @@ -1301,18 +1410,35 @@ what it already does with a policy refusal. | `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, proposed a plan, or was interrupted in the middle of a call | | `5` | it ended without finishing: stalled, escalated, or cancelled | +| `6` | the agent finished and the work does not hold up: a gate you configured did not pass | 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 `6` is new in 0.24.0 and it is the only one that is.** It says something no +other row could: the run ended the way `0` ends, of its own accord, and then the +criterion you set in [`[app.io-cli.gates]`](#verification-gates) failed anyway — +the tests did not pass, the file was not written, the reviewer said no. It is not +`1`, because nothing went wrong with the invocation; it is not `5`, because the +agent did not stall or give up; and it is emphatically not `0`, which is the +status a build script reads as permission to carry on. A run that never had a gate +configured can never return it. + +**No exit code was renumbered, and `6` is the first one added since `io exec` +shipped.** `0` through `5` have meant exactly what they mean in the table above +since 0.5.0, and they mean it unchanged here: a script branching on them is a +script this release did not break. What changes is that a script branching on `0` +alone now has a sixth answer to handle, which is the point — before this release +there was no status a gated run could return that said the work was not good +enough, because there were no gates. + **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. +on and the `io resume` that decides it. That release renumbered nothing and added +nothing: `4` had been given to a pause that could not yet be answered for exactly +that 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 @@ -1490,7 +1616,7 @@ bundles](#capability-bundles). **`/profile`** switches to a named `[profile.]` for the session, and `--profile ` picks one for a single run without writing anything. -Eight keys live there, and six tables: +Eight keys live there, and seven tables: | Key | Is | | --- | --- | @@ -1507,6 +1633,7 @@ Eight keys live there, and six tables: | `[[app.io-cli.mcp]]` | MCP servers for the turn, in io-harness's own shape. Merged with the top-level `[[mcp]]`, and wins a collision of ids. | | `[[app.io-cli.lsp]]` | language servers for this workspace. Merged with the top-level `[[lsp]]`, and wins a collision of ids. | | `[app.io-cli.browser]` | a browser the agent may drive. Never downloaded — it is one you already have. | +| `[app.io-cli.gates]` | what "done" means for this repository: one of `command` (with `expect_exit`), `file` (with `contains`), or `rubric` (with `reviewer`, and `allow_self_review` if the judge may be the model that did the work), plus `retries`, which defaults to 1 and is report-only at 0. Naming none of the three, or more than one, is refused rather than resolved by precedence. See [Verification gates](#verification-gates). | | `[app.io-cli.prices]` | where the rates in `[prices]` came from: `source_url` names a catalogue to read instead of io-harness's default, and `source` and `models` record what the last read was and how many models it priced. The last two are written by a fetch rather than by hand. See [Where a price comes from](#where-a-price-comes-from). | Because the section is unvalidated by design, an unrecognised *value* reads as the @@ -1639,11 +1766,16 @@ line beside what has been drawn against them. surface the omission was waiting on: `/plugin` for what a bundle brought and what was dropped, and io-harness's own refusal sentence for a hook a project file may not declare. **`[prices]` is read from 0.22.0**, which is the release that reads -the provider-call rows it prices. That leaves one section and one key still -unapplied, and each has a reason: there is no `[verify]` section to apply, and -giving a session verification gates needs its own surface; and `run.templates` is -the thirteenth `[run]` key, reachable only through its own accessor. Neither is a -silent omission — this is where they are named. +the provider-call rows it prices. **A contract carries a verification criterion +from 0.24.0**, which was the last of these to be named here and was named with its +reason: it needed a surface of its own, and `/gates` is that surface. There is +still no `[verify]` section, because there is none in io-harness's schema to +apply — what a session carries comes from `[app.io-cli.gates]`, which is io-cli's +own. See [Verification gates](#verification-gates). + +That leaves one key still unapplied, and it has a reason too: `run.templates` is +the thirteenth `[run]` key, reachable only through its own accessor. It is not a +silent omission — this is where it is named. **A price is never invented, and a missing one is never a zero.** io-cli compiles no rates in and estimates nothing, so an install that has connected no provider diff --git a/docs/config.example.toml b/docs/config.example.toml index 77350e7..86ad83b 100644 --- a/docs/config.example.toml +++ b/docs/config.example.toml @@ -345,7 +345,7 @@ net = "deny" # --- io-cli's own section --------------------------------------------------- # -# Eight keys and six tables. io-harness does not validate any of it, so an +# Eight keys and seven tables. io-harness does not validate any of it, so an # unrecognised VALUE reads as the default rather than stopping a session. A # table io-harness cannot parse at all is different: the session starts on the # defaults and says so, carrying the harness's own message. @@ -549,3 +549,98 @@ fleet = "ctrl+f" # # [app.io-cli.prices] # source_url = "https://openrouter.ai/api/v1/models" + +# --- what "done" means for this repository (0.24.0) ------------------------- +# +# A gate is the answer to a question only you can answer: what has to be true +# before this turn is finished. io-harness has run a verification after the agent +# stops since long before io-cli asked it to — a task contract carries one, the +# run loop executes it inside the sandbox, and the run comes back a success only +# when it passed. Until this release io-cli supplied none, so every clean turn +# reported "finished", which was an honest report of a contract that asked for +# nothing. +# +# ABSENT MEANS NO GATE, AND THAT IS NOT A DEFAULT WORTH WRITING. Nothing is +# verified for an operator who has not said what verifying would mean, and there +# is no criterion this file could pick for you that would not be a guess about +# somebody else's repository. +# +# EXACTLY ONE OF `command`, `file` AND `rubric` MAY BE SET. They are the three +# kinds, and the key you write is what selects one. Naming none, or naming two, +# is refused where you can still see what you typed rather than resolved by a +# precedence rule — a precedence rule here would quietly gate every turn on +# something you did not choose, which is the one failure a gate must not have. +# +# `retries` is how many further turns a failing gate may buy, and it defaults to +# 1. ZERO MEANS REPORT AND DO NOT RE-DRIVE: the failure is shown and the turn +# ends, which is what you want when a person is watching. It is a small number on +# purpose — a retry is a whole turn against a real model, not a loop iteration — +# so an operator who wants several says so. +# +# --- 1. a command that must exit a status --------------------------------- +# +# The argv of a program, not a shell line: io-harness checks argv[0] against this +# session's `exec` policy and runs it without a shell, so `&&`, a pipe and a glob +# are not available and were never going to be. `expect_exit` defaults to 0. +# +# `command` IS ALWAYS A CONCRETE ARGV. An empty list is refused, and an absent one +# is not a criterion at all — this key never means "work it out for me". +# +# What detection is for is the OFFER: `/gates` asks io-harness what this repository +# builds and tests with — it is the half of this that knows a Cargo workspace from +# a package.json — and offers you that command as a row you can accept, which +# writes it here in full. io-cli holds no list of its own to consult, deliberately: +# a list of build tools shipped inside a TUI is wrong for somebody the day it is +# written and stale for everybody a year later. So the file always says exactly +# what will run, and you can read it and narrow it. +# +# AND IT RUNS AFTER EVERY STEP, NOT ONCE AT THE END. io-harness evaluates the +# criterion at the bottom of its step loop and keeps going until it passes, so a +# nine-step turn runs this command nine times. `cargo test --all-features` after +# every step of a long turn is a lot of compiling; the gate you want is usually +# the narrowest command that would still catch the mistake. +# +# [app.io-cli.gates] +# retries = 1 +# command = ["cargo", "test", "--all-features"] +# expect_exit = 0 +# +# --- 2. a file that must exist, and optionally say something -------------- +# +# The path is relative to the workspace root. `contains` is plain text the file +# must hold; absent, existence alone is the whole criterion. This is the cheap +# gate — nothing is run and nothing is billed — and it is the right one for work +# whose product is a file: a migration written, a changelog entry added, a +# generated artefact that has to appear. +# +# [app.io-cli.gates] +# file = "CHANGELOG.md" +# contains = "## 0.24.0" +# +# --- 3. a rubric a second model answers ----------------------------------- +# +# For work no exit status can judge. `rubric` is what the reviewer is asked to +# decide; `reviewer` is the model that decides it. +# +# A RUBRIC REQUIRES A REVIEWER, AND THE REVIEWER MAY NOT BE THE MODEL DOING THE +# WORK UNLESS `allow_self_review = true`. Both are refused when the section is +# written rather than when the turn runs, and that is the whole reason they are +# stated here: io-harness answers each of them with a hard configuration error at +# RUN START — before the first billed call, on every turn — so a file that says +# either one turns a configuration edit into a session that will not start, with +# the failure on screen nowhere near the key that caused it. Self review is +# allowed when you ask for it and never by default: a judge marking its own paper +# is a decision, not an oversight. +# +# AND A REVIEW CRITERION IS A REAL BILLED COMPLETION AFTER EVERY STEP — NOT ONCE +# PER TURN. The criterion is evaluated at the bottom of io-harness's step loop and +# keeps being evaluated until it passes, so a nine-step turn asks the reviewer nine +# times, and each retry the gate buys starts that again. The other two kinds cost a +# process and a file read; this one costs money per step, which is worth knowing +# before it is in a file that runs in CI. Keep the rubric to something a model can +# decide quickly, and prefer a command or a file where one would do. +# +# [app.io-cli.gates] +# rubric = "every public item changed in this turn has a doc comment saying why" +# reviewer = "anthropic/claude-sonnet-4.5" +# allow_self_review = false diff --git a/src/app.rs b/src/app.rs index 85a446a..b4831df 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2094,3 +2094,194 @@ impl App { } } } + +// --------------------------------------------------------------------------- +// The verification gate, as a surface and a driver read it +// --------------------------------------------------------------------------- +// +// **These live here rather than in `src/main.rs`, and that placement is the +// whole reason they are functions at all.** Nothing under `tests/` links the +// binary, so a decision written as a branch in the driver is one no test can +// drive and no sabotage can make fail. `crate::gates` owns everything that is a +// property of a criterion; what is left is the handful of answers a *surface* +// needs — which attempts a turn is judged by, what one line of scrollback says +// about them, and what a retried turn is told. Each is pure over its arguments, +// reads no clock, and opens no store. + +// --------------------------------------------------------------------------- +// `/mcp`'s edit verb, as it travels through the composer +// --------------------------------------------------------------------------- + +/// What `/mcp`'s edit verb writes on the prompt in front of the server's id. +/// +/// **An id and not an index, and the whole verb rests on that.** `mcp[3].command` +/// is a position in one file's `[[mcp]]` array; the operator is about to type a +/// value into a composer that any other keystroke can leave, and a file edited in +/// another window in between moves the array under it. `mcp..` names the +/// entry by the one thing that is stable, so the driver resolves +/// [`crate::servers::At`] again — from the file's own bytes — when the line comes +/// back. That is the same rule `/mcp`'s removal verb has followed since 0.21.0, +/// and it is the rule 0.20.0's wrong delete was shipped by breaking. +/// +/// It is spelled here rather than in the driver because nothing under `tests/` +/// links `src/main.rs`: the prefix the composer writes and the prefix the driver +/// matches on must be one constant, or the verb goes quietly dead the first time +/// either is retyped. +pub const SERVER_KEY: &str = "mcp."; + +/// The server id and the key a `mcp..` line addresses. +/// +/// `None` for anything that is not that shape, including a bare `mcp.` and an id +/// with no key after it. An id containing a dot splits at the LAST one, because +/// the key is the part this crate knows the spelling of — [`crate::servers::KEYS`] +/// holds none with a dot in it, and an MCP server may be called anything at all. +pub fn server_key(line: &str) -> Option<(&str, &str)> { + let rest = line.strip_prefix(SERVER_KEY)?; + let (id, key) = rest.rsplit_once('.')?; + (!id.is_empty() && !key.is_empty()).then_some((id, key)) +} + +/// The TOML source for a value an operator typed after an `[[mcp]]` key. +/// +/// [`crate::servers::edit`] takes TOML **source**, and what arrives from a +/// composer is a person's typing. The gap is not cosmetic: `format!("\"{typed}\"")` +/// — the spelling every call site reaches for — is a parse error or a different +/// value the moment the text carries a quote or a backslash, and a Windows command +/// path is full of the second. +/// +/// Four shapes, because `[[mcp]]` has four kinds of value: +/// +/// * `args` is a list, and an operator writes a list of arguments as a command +/// line. Split on whitespace, which is the same trade every shell makes and the +/// reason an argument with a space in it needs the file rather than this verb. +/// * `timeout_secs` is a number, and a number is already TOML. +/// * `env` and `headers` are inline tables, which have exactly one spelling — +/// `{ KEY = "value" }` — so what the operator typed is passed through and +/// `crate::edit::apply` refuses it if it is not TOML. +/// * everything else is a string, escaped by [`crate::servers::quoted`]. +pub fn server_value(key: &str, typed: &str) -> String { + let typed = typed.trim(); + match key { + "args" => crate::edit::array(&typed.split_whitespace().collect::>()), + "timeout_secs" | "env" | "headers" => typed.to_string(), + _ => crate::servers::quoted(typed), + } +} + +/// The row `/gates` draws for the command this repository proposes for itself. +/// +/// A sentinel and not a key, for the reason [`crate::configure::REFRESH_PRICES`] +/// is one: every other row on that surface names a setting and puts it in the +/// composer, and this one does something. It is spelled here rather than in the +/// driver so a test can reach it. +pub const PROPOSED_GATE: &str = "!proposed-gate"; + +/// What the failing gate actually said, or `None` when it said nothing. +/// +/// Two sources because a gate has two ways of speaking and neither covers the +/// other. A command's output is **not** in its row — `GateAttempt::detail` is +/// empty for every non-review criterion — it arrives as `gate_output` sandbox +/// events, which is what [`crate::gates::output`] reads, filtered to the step this +/// attempt ran after so a retried gate carries the failure that caused the retry +/// rather than the one two turns ago. A review's reasons and an errored gate's +/// cause are the opposite: they are in the row and there is no sandbox event at +/// all, because nothing was executed. +fn gate_said( + attempts: &[io_harness::GateAttempt], + events: &[io_harness::SandboxEvent], +) -> Option { + let last = attempts.last()?; + // `trim`, because io-harness writes `review.reasons.join("; ")` verbatim and a + // reviewer that answered with blanks would otherwise produce a report line + // ending in a dangling colon and a retry prompt whose "this is what it + // reported" section is empty. Nothing to say is `None`, and `gate_retry` + // already has a sentence for that. + crate::gates::output(events, last.step) + .or_else(|| (!last.detail.trim().is_empty()).then(|| last.detail.clone())) +} + +/// The one line a turn's gate commits into the scrollback, with its tone. +/// +/// `None` for a turn nothing gated, which is the ordinary case and not a thing to +/// say: a session with no `[app.io-cli.gates]` section would otherwise earn a +/// line under every turn saying so. +/// +/// **Scrollback and not the footer.** The verdict is an account of the turn above +/// it and outlives the keystroke that follows; `App::say` is answered by the next +/// key press. The driver is what calls [`App::record`] with this — the tone +/// travels with the sentence so the driver holds no branch about it. +pub fn gate_report( + attempts: &[io_harness::GateAttempt], + events: &[io_harness::SandboxEvent], +) -> Option<(Tone, String)> { + let standing = crate::gates::standing(attempts)?; + // `GateOutcome::as_str` verbatim, never a word of io-cli's own: the status + // line, the exit code and this sentence all have to spell one verdict the + // same way, and `as_str` is where that spelling is decided. + let word = standing.outcome.as_str(); + let tone = match standing.outcome { + io_harness::GateOutcome::Passed => Tone::Success, + // **`Tone::Warning` and emphatically not `Tone::Refused`, whose rendered + // word is the literal `refused`.** That word belongs to the permission + // boundary, and this release moved the failing review off that tone in + // `src/events.rs` for exactly this reason — only for the scrollback line + // one row below to put it back. An operator would read `warning: the gate + // ran and did not pass` and directly beneath it `refused: gate failed`, + // which collapses "the policy would not run my gate" into "your work did + // not meet the bar". Those are the two facts a gate most has to keep + // apart, and they need opposite responses. + io_harness::GateOutcome::Failed => Tone::Warning, + // `Errored`, and whatever a later io-harness adds — the enum is + // `#[non_exhaustive]`, and an outcome this release has not seen is not a + // pass. + _ => Tone::Error, + }; + let mut line = format!("gate {word} ({} criterion", standing.phase); + if standing.attempt > 1 { + line.push_str(&format!(", attempt {}", standing.attempt)); + } + line.push(')'); + if let Some(said) = gate_said(attempts, events) { + line.push_str(": "); + line.push_str(said.trim()); + } + Some((tone, line)) +} + +/// The prompt a failing gate drives the next turn with. +/// +/// **It takes no goal, and that absence is the whole design.** A retry that +/// re-sent the original prompt would look like a working retry — a second turn +/// runs, the model does more work — while telling the agent nothing about what +/// went wrong, and the criterion it is judged by would fail again for the same +/// reason. There is no parameter here it could be passed through, so that +/// implementation cannot be written. What the turn is told is what the criterion +/// asks, in [`crate::gates::Criterion::describe`]'s words — the same words the +/// first turn was judged by — and what the gate reported when it said no. +/// +/// The last sentence is not decoration. A gate is a command, a file or a rubric +/// the operator wrote; an agent handed a failing check and no instruction about it +/// will sometimes edit the check. +pub fn gate_retry( + criterion: &crate::gates::Criterion, + attempts: &[io_harness::GateAttempt], + events: &[io_harness::SandboxEvent], +) -> String { + let asks = criterion.describe(); + match gate_said(attempts, events) { + Some(said) => format!( + "The verification gate for this work did not pass. It asks: {asks}.\n\nThis is \ + what it reported:\n\n{}\n\nChange the work so that the gate passes. Do not \ + change the gate itself.", + said.trim() + ), + // A gate that failed silently — a command that printed nothing, an + // existence check on a file nobody wrote — still has a criterion to + // state, and stating it is more than the turn had before. + None => format!( + "The verification gate for this work did not pass. It asks: {asks}. It reported \ + nothing further.\n\nChange the work so that the gate passes. Do not change the \ + gate itself." + ), + } +} diff --git a/src/commands.rs b/src/commands.rs index b104cd5..a26644f 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -247,21 +247,37 @@ pub const COMMANDS: &[(&str, &str)] = &[ "/stats", "commit how the runs have gone: outcomes, first-try, gates, latency", ), + // Beside `/stats` because `/stats` is the only other row that says the word, + // and the two are the halves of one thing: that page counts how the gates + // went, and until this release nothing in the product could say what a gate + // *was*. An operator reading a gate column over a session that configured + // none is reading zeroes and cannot tell them from a gate that never failed. + // + // The three kinds are named in the description rather than left to the + // surface, because the choice between them is the whole decision — a command + // is free and objective, a file is nearly free and narrow, and a rubric is a + // billed completion on every gated turn. A row that said only "what done + // means" would hide the one of the three that costs money. + ( + "/gates", + "the check a turn must pass before it is done: a command, a file, or a rubric", + ), ]; /// What an operator is doing when they reach for a command. /// /// **Grouped by the operator's intent rather than by which part of the harness /// answers**, because the second is an implementation detail and the first is -/// the only thing somebody scanning a list of twenty-six is holding in their +/// the only thing somebody scanning a list of thirty-one is holding in their /// head. /// /// Four groups and none longer than ten, which is the bound `tests/commands.rs` -/// asserts. A flat list of twenty-six is a list nobody reads; 0.16.0 is the +/// asserts. A flat list of thirty-one is a list nobody reads; 0.16.0 is the /// release that grouped them, at twenty, and every release since has added to a /// group rather than to a list. **The count in this paragraph is the one number /// here that goes stale on its own** — it said twenty through 0.17.0, which had -/// twenty-three — so it is written out rather than left as "a few". +/// twenty-three, and twenty-six through 0.23.0, which had thirty — so it is +/// written out rather than left as "a few". #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] pub enum Group { /// The conversation itself: start one, leave one, come back to one. @@ -376,6 +392,21 @@ pub const GROUPS: &[(Group, &[&str])] = &[ "/mcp", "/provider", "/plugin", + // **`/gates` is here because it WRITES `[app.io-cli.gates]`, and that + // is the whole of the argument.** Its first screen reads the section + // back and says what the last turn's verdict was, which is the same + // first screen `/mcp` and `/provider` had when 0.19.0 filed them under + // `Inspect` — and the same mistake, for the same reason: an operator + // who opened it to look is one keystroke from changing what every + // later turn has to prove before it is allowed to finish. That is the + // sentence `Inspect` promises it will never say, and it is a louder + // sentence here than for a server list, because a gate an operator did + // not mean to set spends a whole extra turn against a real model. + // + // It takes `Configure` to nine of ten, so the next command that + // belongs here forces the same question 0.22.0 answered rather than + // dodged: re-file what is in the wrong group, do not widen the bound. + "/gates", // **`/import` writes files, so `Configure` is the only group it can // be in** — and `Inspect` is full at nine besides. It is last because // it is the one command here an operator uses once: the others are @@ -1026,6 +1057,27 @@ pub enum Action { /// recorded. A key that was never on disk must not reach disk as a side /// effect of trying a new program. Import, + /// Say what "done" means for this repository, and write it down. + /// + /// **No argument, and that is a decision rather than an omission.** A + /// criterion is an argv, or a path and the text a file must contain, or a + /// whole sentence of rubric — see [`crate::gates::Criterion`] for the three. + /// None of those is a word that fits after a slash, and giving them one would + /// mean io-cli inventing a little language for a section io-harness already + /// spells in TOML: `/gates cargo test --all` is either a shell line this crate + /// would have to split — which is exactly what `Criterion::Command` refuses to + /// be, because io-harness matches `argv[0]` against the policy and runs no + /// shell — or an argv with nowhere to put the exit status it must report. So + /// the word opens the surface and anything after it is ignored, the way + /// `/model gpt-5` ignores its argument. + /// + /// **The surface is also the only place the two refusals can be seen.** A + /// rubric with no reviewer, and a reviewer that is the model doing the work, + /// are both `Error::Config` at run start in io-harness — before the first + /// billed call, on every turn, and disconnected on screen from the keystroke + /// that caused them. [`crate::gates::Refusal`] names both while the operator + /// is still looking at what they typed. + Gates, } /// What `/copy` was asked for. @@ -1613,6 +1665,16 @@ pub fn parse(input: &str, keys: &Keys, theme: &Theme) -> Action { // idea what this one calls things. Neither spelling is an alias for a // second screen: both open the one surface. "import" | "migrate" => Action::Import, + // `/gate` is admitted for the mirror of the reason `/plugins` and + // `/servers` are. A contract carries exactly one criterion, so the + // singular is what a hand reaches for at least as often as the plural, + // and refusing the other spelling teaches nobody anything. Neither + // spelling is an alias for a second screen: both open the one surface. + // + // `/verify` is deliberately not taken, under the rule that already + // refuses `/fold`: it is the word io-harness uses for the pillar + // internally, and nobody has typed it at this prompt. + "gates" | "gate" => Action::Gates, // **An alias earns no row of its own.** `/usage` is what an operator // coming from another agent types for "what is this costing me", and a // second row for one screen reads as a second screen — so this is diff --git a/src/configure.rs b/src/configure.rs index dfddaf6..af5bd4e 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -159,6 +159,25 @@ pub const CATALOGUE: &[&str] = &[ // setting, and it is reached the way `/provider` and `/mcp` reach a list. "prices.as_of", "app.io-cli.prices.source_url", + // What "done" means here. Every key of the section and not a chosen few, + // because the section refuses rather than defaults: exactly one of `command`, + // `file` and `rubric` may be set, and an operator meeting three of the eight + // on this surface would be reading a partial list as the whole schema and + // writing an ambiguous section from it. `expect_exit`, `contains` and + // `allow_self_review` each qualify exactly one of those three, and a + // qualifier nobody can see is a qualifier nobody sets. + // + // Unlike `prices.source`, none of these is written by machinery — a gate is + // typed by the operator or it does not exist — so there is no key here that + // is listed only to be read. + "app.io-cli.gates.retries", + "app.io-cli.gates.command", + "app.io-cli.gates.expect_exit", + "app.io-cli.gates.file", + "app.io-cli.gates.contains", + "app.io-cli.gates.rubric", + "app.io-cli.gates.reviewer", + "app.io-cli.gates.allow_self_review", ]; /// The `/config` row that re-reads the price catalogue. diff --git a/src/contract.rs b/src/contract.rs index e859417..e095764 100644 --- a/src/contract.rs +++ b/src/contract.rs @@ -271,12 +271,137 @@ pub fn configured(text: impl Into, root: PathBuf, config: &Config) -> Ta Some(hooks) => contract.with_tool_hooks(std::sync::Arc::new(hooks)), None => contract, }; + // **The operator's own criterion, here rather than in [`session`], and the + // placement is the whole of F1.** A gate applied in the session's half would + // hold a terminal to a standard and let CI run to green with nothing checked + // — the 0.14.0 asymmetry again, and the one this release is least able to + // afford, because the headless arm is where an unverified success does the + // most damage. + // + // A refusal leaves the contract ungated on purpose. `/gates` refuses both + // mistakes at the moment the file is written (F5), so a section that still + // resolves to one here was hand-edited afterwards — and the honest response + // to an unusable criterion is a run with no gate plus a notice, never a run + // io-harness kills at start with `Error::Config` before the first billed + // call. [`gate_notice`] is what puts that on screen. + let contract = match criterion_for(config) { + Some((criterion, reviewer)) => { + let contract = contract.with_verification(criterion.verification()); + let contract = match reviewer { + Some(reviewer) => contract.with_reviewer(reviewer), + None => contract, + }; + // **A gate must not turn "hello" into a run, and by default it does.** + // io-harness infers whether a turn may be answered conversationally + // from `matches!(contract.verify, Verification::None)` — so attaching + // any criterion silently switches greeting handling off for the whole + // session, and every idle question becomes a full agent run that then + // runs the operator's test suite after each of its steps. The harness's + // own field documents this as the way back for exactly a surface like + // this one. Set only where a criterion was attached, so an operator + // with no gate reproduces the contract this crate built before the + // release, field for field, which is what `tests/contract.rs` asserts. + contract.with_conversational_turns(true) + } + None => contract, + }; // `[run] skills` has had its say, and `io exec` reads no other key that can // name one — so for the headless arm this is already the point after every // key. [`session`] calls this again once `[app.io-cli]` has had its own. resolve_skills(contract) } +/// The criterion this configuration resolves to, with its reviewer already built. +/// +/// `None` covers every case in which the run must not be gated: no section, a +/// section that names no criterion, a section that names two, and — the one that +/// matters most — a review criterion whose reviewer cannot be constructed. **A +/// `Verification::Review` reaching a contract with no reviewer beside it is +/// `Error::Config` at run start, on every turn**, so the criterion and its +/// reviewer are decided together here and either both go on or neither does. +/// The criterion this configuration resolves to, with no reviewer built. +/// +/// What a *reader* wants: the surfaces that report a verdict need to know which +/// criterion was in force, and building a provider to tell them would resolve a +/// credential for a question that does not need one. `criterion_for` is the +/// half that also builds the reviewer, for the contract that will run it. +/// +/// This is what lets `io exec` fold the gate rows the same way a session does — +/// without it the headless arm reads io-harness's `phase = "none"` bookkeeping as +/// a failed gate, and a bare `file` criterion is never evaluated at all. +pub fn criterion_of(config: &Config) -> Option { + let gates = crate::settings::stored(config).0?.gates?; + let working = config + .provider_spec() + .map(crate::provider::model_of) + .unwrap_or_default(); + gates.criterion(working).ok()? +} + +fn criterion_for( + config: &Config, +) -> Option<( + crate::gates::Criterion, + Option>, +)> { + let gates = crate::settings::stored(config).0?.gates?; + // The model the work is done by, which is what the self-review refusal is + // decided against. An empty string means it is not knowable from here, and + // `Settings::criterion` treats that as "cannot clash" rather than guessing. + let working = config + .provider_spec() + .map(crate::provider::model_of) + .unwrap_or_default(); + let criterion = gates.criterion(working).ok()??; + match &criterion { + crate::gates::Criterion::Review { reviewer, .. } => { + let spec = config.provider_spec()?; + let built = crate::reviewer::build(spec, reviewer).ok()?; + Some((criterion.clone(), Some(built))) + } + _ => Some((criterion, None)), + } +} + +/// Why the configured criterion is not gating this run, if it is not. +/// +/// Separate from [`configured`] because that function returns a contract and a +/// refusal is not a contract. A surface calls this to say the one sentence an +/// operator needs: the section is there, and it is not doing anything. +pub fn gate_notice(config: &Config) -> Option { + // **The commonest mistake is a value of the wrong shape, and it takes the + // whole table with it.** `command = "cargo test"` — a string where an array + // belongs — fails to deserialize `[app.io-cli]` entirely, so `stored` answers + // `(None, Some(complaint))`: no theme, no keys, no ceilings and no gate. + // Reading only `.0` and giving up would leave `/gates` saying "no gate is in + // force" while the section is plainly in the operator's file, which is the one + // surface they opened to ask why it is not running. + let (stored, complaint) = crate::settings::stored(config); + let Some(stored) = stored else { + return complaint; + }; + let gates = stored.gates?; + let working = config + .provider_spec() + .map(crate::provider::model_of) + .unwrap_or_default(); + match gates.criterion(working) { + Err(refusal) => Some(refusal.to_string()), + Ok(Some(crate::gates::Criterion::Review { reviewer, .. })) => { + match config.provider_spec() { + None => Some(format!( + "the gate asks {reviewer} to review the work, but no provider is configured to \ + reach it — this turn is not gated" + )), + Some(spec) => crate::reviewer::build(spec, &reviewer) + .err() + .map(|why| format!("the gate's reviewer could not be built: {why}")), + } + } + Ok(_) => None, + } +} + /// The hooks a run should be observed by, for the caller that installs the /// fan-out. /// diff --git a/src/events.rs b/src/events.rs index 014922a..f6ff761 100644 --- a/src/events.rs +++ b/src/events.rs @@ -1299,19 +1299,36 @@ impl Events { } // The reasons are carried rather than summarised, for io-harness's own // reason: a refusal a human cannot argue with is a gate nobody trusts - // twice. + // twice. They are the operator's only account of why the work was + // judged insufficient — the store has the same sentence joined by + // semicolons, and a session that dropped them would be sending + // somebody to a database to read a paragraph the run already said. + // + // **The failing verdict is a warning and not `Tone::Refused`, from + // 0.24.0.** That tone carries the literal word `refused`, which + // everywhere else in this transcript means the permission boundary + // stopped an act; a reviewer that read the work and judged it did not + // stop anything, and giving both the same word would make the two + // things an operator most needs to tell apart — the policy would not + // run my gate, versus my work did not meet the bar — read identically. + // + // **A review that did not happen is not drawn here, because it is not + // emitted.** io-harness sends this event only for a verdict somebody + // gave; a transport failure or an unparseable answer emits nothing and + // is `GateOutcome::Errored` in the store. So this line always means a + // review that ran, and its absence is not evidence that one passed. EventKind::Reviewed { passed, reasons } => { let mut lines = self.flush_text(); lines.push(theme.notice( if *passed { Tone::Success } else { - Tone::Refused + Tone::Warning }, if *passed { "the review passed" } else { - "the review did not pass" + "the review ran and did not pass" }, )); for reason in reasons { @@ -1357,28 +1374,54 @@ impl Events { // `triage::TRIAGE` records that route. Vec::new() } - // **What isolated the work, said in the operator's words rather than - // inferred from a tool cell that happened to succeed.** Seven kinds - // reach this channel and four of them are drawn. + // **What isolated the work, and how the criterion it was isolated + // for answered, said in the operator's words rather than inferred + // from a tool cell that happened to succeed.** Seven kinds reach this + // channel and six of them are drawn. // - // **`dial` is not one of them.** io-harness builds it as a `destroy` - // event with the kind overwritten and emits it immediately beside - // `EventKind::Dialed` for the same outbound connection, so a session - // drawing both would put every dial in the transcript twice — and - // the copy here is the poorer one, carrying the word and nothing + // **`dial` is the one that is not.** io-harness builds it as a + // `destroy` event with the kind overwritten and emits it immediately + // beside `EventKind::Dialed` for the same outbound connection, so a + // session drawing both would put every dial in the transcript twice — + // and the copy here is the poorer one, carrying the word and nothing // else where the dial itself carries the host, the port and the - // verdict. `gate_phase_failed` and `gate_output` are the other two: - // they belong to an execution gate, and every contract this crate - // builds — see `crate::contract` — leaves `Verification::None` on it, - // because nothing here ever calls `with_verification`. So - // neither can arrive in a session before 0.24.0 gives one a gate, - // and neither is given a sentence written in advance of the release - // that could check it. - // - // An empty `Vec` is this module's own word for "nothing yet" and is - // the honest way to say all three: a caller cannot tell it from a + // verdict. An empty `Vec` is this module's own word for "nothing yet" + // and is the honest way to say it: a caller cannot tell it from a // dropped event because there is nothing there to tell apart. // + // **`gate_phase_failed` and `gate_output` are drawn from 0.24.0.** + // Before this release every contract this crate built left + // `Verification::None` on it, so neither kind could arrive in a + // session and neither was given a sentence written in advance of the + // release that could check it. An operator now configures one + // criterion — `[app.io-cli.gates]`, resolved by `crate::gates` — + // io-harness runs it after the agent stops, and these two are the + // whole of what it says about a criterion that did not hold on the + // channel a session is already watching. A verdict that lives only in + // the store is a verdict nobody reads. + // + // **Neither line carries the fact that made it worth emitting, and + // that is io-harness's shape rather than a choice made here.** The + // failing phase and the failing command's bounded output are both in + // `SandboxEvent::detail`, and `EventKind::Sandbox` carries the kind + // and the backend alone. So a session learns that the gate ran and + // said no, and the text stays in the run's `sandbox_events` rows + // where a diagnosis reads it. Naming a phase here, or printing an + // empty string as though it were the command's output, would be this + // module writing the half it was not given. + // + // **A gate that did not hold is not a refusal, and neither line says + // that word.** `Tone::Refused` in this transcript means the + // permission boundary stopped an act — which is exactly what a + // criterion whose program the policy will not run produces: an + // `EventKind::Refused` with act `exec`, and no gate event at all, + // because nothing ran and nothing judged anything. That is + // `GateOutcome::Errored` in the store, the one verdict io-harness + // will retry. A gate line therefore only ever stands for a criterion + // that ran and answered, and the two need opposite responses from + // whoever is reading — fix the policy, or fix the work — so they are + // told apart by the words and never by a colour. + // // **`cap_hit` is a limit reached and not a failure.** The sandbox // did exactly what its configuration told it to; reporting that // through the error path would tell an operator their run broke at @@ -1400,6 +1443,11 @@ impl Events { "exec" => ("a command ran in the sandbox", Tone::Muted), "cap_hit" => ("the sandbox reached a limit it was given", Tone::Warning), "destroy" => ("the sandbox was torn down", Tone::Muted), + // `ran and` is load-bearing: it is the whole of what + // separates a criterion that judged the work from one that + // never got to. + "gate_phase_failed" => ("the gate ran and did not pass", Tone::Warning), + "gate_output" => ("the gate command printed output", Tone::Muted), _ => return Vec::new(), }; let mut text = said.to_string(); @@ -1747,9 +1795,14 @@ fn format_millis(duration: Duration) -> String { /// The vocabulary is io-harness's, and the distinction that matters here is /// between `success` and `finished`: `success` means a verification criterion /// passed, and `finished` means a run with no criterion ended on its own terms. -/// **Every io-cli turn ends `finished`**, because every contract this crate -/// builds — see [`crate::contract`] — leaves `Verification::None` on it: nothing -/// here calls `with_verification`, on either arm. +/// +/// **Both are reachable from 0.24.0, and until this release only one was.** Every +/// contract this crate built left `Verification::None` on it, so every io-cli +/// turn ended `finished` and `success` could not be produced from this interface +/// at all. An operator now configures one criterion — see [`crate::gates`] — and +/// a turn that carries one ends `success` when it holds. Neither word is a +/// failure and both are drawn as the same thing; what separates them is how much +/// the run is claiming, and that claim is the operator's to read. /// /// Found in a live run, not in the suite: treating anything that was not /// `success` as a warning meant every ordinary, completely successful turn ended diff --git a/src/exec.rs b/src/exec.rs index fb4ba42..80ca05c 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -62,6 +62,22 @@ pub const CEILING: u8 = 3; pub const PAUSED: u8 = 4; /// It ended without finishing. pub const UNFINISHED: u8 = 5; +/// The agent finished and the work does not hold up. +/// +/// Added in 0.24.0, the release that let an operator say what "done" means. It +/// renumbers nothing: the six below have meant what they mean since 0.5.0, and +/// this is the first ending this subcommand has ever been able to tell apart +/// from them. +/// +/// It cannot be decided by [`code`], because io-harness has no `RunOutcome` +/// variant for a run whose criterion answered no. Such a run surfaces as +/// `StepCapReached` — the criterion is re-evaluated after every step and the loop +/// only ends early when it *passes*, so a gate that never passes spends the whole +/// budget. (Not as `Finished`: that variant requires `Verification::None`, so a +/// gated contract can never return it.) The verdict is therefore read from the +/// store after the run and applied by [`verified_code`]. Reported upstream as +/// io-harness#212. +pub const UNVERIFIED: u8 = 6; /// The exit status for a run that reached the harness. /// @@ -80,12 +96,15 @@ pub const UNFINISHED: u8 = 5; /// that claimed success or a crash would both be inventions. /// /// Two of the mappings are the release's research rather than its taste. -/// `Finished` is `OK` because a contract with no verification criterion is what -/// this subcommand always builds, so `Finished` — and never `Success` — is what -/// a clean run returns; a table that treated only `Success` as zero would fail -/// every successful run. And the four ceilings need codes at all only because the -/// harness returns them as `Ok`, so a status read off the `Result` reports -/// success on every one of them. +/// `Finished` is `OK` because a contract with no verification criterion returns +/// `Finished` and never `Success`, and a table that treated only `Success` as zero +/// would fail every successful ungated run. **0.24.0 makes `Success` reachable +/// and changes neither mapping**: an operator who configures `[app.io-cli.gates]` +/// gets a real criterion on the contract from [`crate::contract::configured`], and +/// a run that passes it comes back `Success`. Both still mean the run ended of its +/// own accord, which is what `0` says. And the four ceilings need codes at all +/// only because the harness returns them as `Ok`, so a status read off the +/// `Result` reports success on every one of them. pub fn code(outcome: &RunOutcome) -> u8 { match outcome { RunOutcome::Success { .. } | RunOutcome::Finished { .. } => OK, @@ -133,6 +152,29 @@ pub fn code(outcome: &RunOutcome) -> u8 { } } +/// The exit status once the operator's own criterion has had its say. +/// +/// A wrapper around [`code`] rather than an arm inside it, and deliberately so. +/// `tests/exec.rs` extracts that function's body by splitting on its exact +/// signature line, and its table is a statement about `RunOutcome` alone — which +/// carries no verification verdict. Putting this decision inside it would both +/// reformat the one function a test reads as text and make a table about +/// outcomes answer a question outcomes do not contain. +/// +/// **Only a `GateOutcome::Failed` earns [`UNVERIFIED`].** A gate that +/// `Errored` never answered at all — the criterion could not run, the reviewer +/// returned nothing parsable, the program was refused — and reporting that as +/// "the work does not hold up" would claim a judgement nobody made. Such a run +/// keeps whatever its outcome maps to, which is the honest report that it ran +/// and was not verified either way. A run with no criterion configured has no +/// standing at all and is untouched. +pub fn verified_code(outcome: &RunOutcome, standing: Option<&crate::gates::Standing>) -> u8 { + match standing { + Some(standing) if matches!(standing.outcome, io_harness::GateOutcome::Failed) => UNVERIFIED, + _ => code(outcome), + } +} + /// One line for stderr, naming the outcome and the harness's own step count. /// /// The number is read off the returned value and never recounted from the store: @@ -588,10 +630,89 @@ impl WithProvider for Headless { if let Some(parked) = parked(&result.outcome, result.run_id) { eprintln!("io: {parked}"); } - Ok(code(&result.outcome)) + // **A run that was meant to be gated and was not says so, on the arm where + // nobody is watching a screen.** A hand-edited section that refuses, or a + // reviewer that will not build, leaves the contract ungated — and until + // this line the only surfaces that reported it were `/gates` and the + // `/config` write, both of which need a terminal. An unattended job that + // silently stopped verifying is the exact failure this release exists to + // prevent, arriving through the release itself. + if let Some(notice) = crate::contract::gate_notice(&self.config) { + eprintln!("io: {notice}"); + } + // **The criterion has the last word on the exit status, and it is read + // from the store rather than from the outcome.** io-harness has no + // `RunOutcome` variant for a run whose gate answered no, so a run that + // spent its budget failing one reports `StepCapReached` and a run that + // stopped early believing itself done reports `Finished` — `3` and `0`, + // neither of which is what happened. Reported upstream as + // io-harness#212. + let standing = gate_standing( + &self.store, + result.run_id, + &self.config, + self.session.root(), + ); + if let Some(standing) = &standing { + eprintln!("io: {}", gate_line(standing)); + } + Ok(verified_code(&result.outcome, standing.as_ref())) } } +/// What a run's own gate attempts say, if it had a criterion at all. +/// +/// **The criterion has to be resolved here, and reading the rows without it is +/// how this release nearly shipped exit `6` for every operator alive.** +/// io-harness evaluates the contract's criterion after every step on which the +/// agent called a tool, and for `Verification::None` that evaluation is `false` — +/// so an ungated run leaves `phase = "none", Failed` rows behind, and a naive +/// read of them says the gate failed on a session that never had one. +/// [`crate::gates::gate_attempts`] is the fold that drops them, and it is the same +/// call the interactive arm makes: one path, not two. +/// +/// It is also what makes a bare `file` criterion work headlessly at all. That one +/// maps to `Verification::None`, so io-harness records nothing for it and io-cli +/// answers it itself — without this the terminal would hold an operator to a +/// standard that CI quietly ignored, which is the 0.14.0 asymmetry this product +/// deleted once already. +/// +/// A store that cannot be read answers `None`, which leaves the exit status +/// exactly what it was before this release. A failure to read the verdict is not +/// evidence that the work is bad. +fn gate_standing( + store: &Store, + run_id: i64, + config: &Config, + root: &std::path::Path, +) -> Option { + let attempts = store.gate_attempts(run_id).ok()?; + let criterion = crate::contract::criterion_of(config); + crate::gates::standing(&crate::gates::gate_attempts( + attempts, + criterion.as_ref(), + root, + )) +} + +/// One line for stderr naming what the gate did. +/// +/// The phase is the harness's own word for the criterion that ran, and the +/// outcome is `GateOutcome::as_str` passed through rather than re-spelled — a +/// second vocabulary for the same fact is how two surfaces come to disagree. +fn gate_line(standing: &crate::gates::Standing) -> String { + let attempt = if standing.attempt > 1 { + format!(" on attempt {}", standing.attempt) + } else { + String::new() + }; + format!( + "the {} gate {}{attempt}", + standing.phase, + standing.outcome.as_str() + ) +} + /// 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 @@ -1109,6 +1230,15 @@ impl WithProvider for Resuming { if let Some(parked) = parked(&resumed.outcome, resumed.run_id) { eprintln!("io: {parked}"); } - Ok(code(&resumed.outcome)) + // A resumed run is gated by the same criterion the original was — the + // contract is rebuilt from the same configuration — so its exit status + // answers the same question. Reading the standing off the run this + // resume drove rather than off the original is the point: the whole + // reason to resume is that the work is not the same work any more. + let standing = gate_standing(&self.store, resumed.run_id, &self.config, &self.root); + if let Some(standing) = &standing { + eprintln!("io: {}", gate_line(standing)); + } + Ok(verified_code(&resumed.outcome, standing.as_ref())) } } diff --git a/src/gates.rs b/src/gates.rs new file mode 100644 index 0000000..b42b497 --- /dev/null +++ b/src/gates.rs @@ -0,0 +1,595 @@ +//! What "done" means for this repository, and whether the turn proved it. +//! +//! io-harness has carried a verification pillar since long before this interface +//! existed: a `TaskContract` holds one `Verification`, the run loop executes it +//! after the agent stops, and the run comes back as `RunOutcome::Success` when it +//! passed. Until this release io-cli supplied none, so every contract it built +//! carried `Verification::None` and every clean run returned `Finished` — which +//! `crate::exec` already documents as an honest report of a contract that asked +//! for nothing. +//! +//! This module is where the operator's answer lives. [`Settings`] is the +//! `[app.io-cli.gates]` section as written; [`Criterion`] is the one criterion +//! that section resolves to, once it has been checked for the two mistakes the +//! file itself cannot express. +//! +//! # One criterion, not a suite +//! +//! `TaskContract`'s verification is a single value rather than a list, so a +//! contract carries one criterion. That is the whole surface this module offers, +//! and the section is a table rather than an array so that a list, if the +//! dependency ever grows one, is an addition rather than a break. +//! +//! # Why the criterion is not run here +//! +//! io-harness executes it, inside the sandbox, with `argv[0]` checked against the +//! operator's `Act::Exec` policy. io-cli could run one itself — `Verification` +//! exposes the calls — but `ExecGuard::with_writable_roots` is `pub(crate)`, so a +//! criterion run from here cannot be given the cache directories a real run gets +//! from the detected toolchain, and a `cargo test` gate would fail on a registry +//! write the harness's own gate would have allowed. The criterion rides the +//! contract; the dependency runs it. That is also what keeps `crate::shell` the +//! only module in this crate that spawns anything. +//! +//! # The refusals happen when the file is written +//! +//! A review criterion with no reviewer, or one whose reviewer is the model doing +//! the work, is `Error::Config` at run start in io-harness — before the first +//! billed call, on every turn, with the failure disconnected on screen from the +//! keystroke that caused it. So [`Refusal`] exists to catch both while the +//! operator is still looking at the surface that wrote them. + +use std::fmt; +use std::path::{Path, PathBuf}; + +use io_harness::{Config, GateAttempt, GateOutcome, SandboxEvent, Verification}; +use serde::{Deserialize, Serialize}; + +/// The `[app.io-cli.gates]` section exactly as an operator wrote it. +/// +/// Flat rather than tagged, matching every other section this crate reads: TOML +/// spells a flat table far more naturally than an internally tagged enum, and a +/// key nobody set is simply absent. Exactly one of [`Settings::command`], +/// [`Settings::file`] and [`Settings::rubric`] decides the kind; naming none or +/// more than one is a [`Refusal`] rather than a silent precedence rule, because a +/// precedence rule here would quietly gate a turn on something the operator did +/// not choose. +/// +/// **`deny_unknown_fields`, unlike its parent, and the asymmetry is deliberate.** +/// `CliSettings` cannot refuse an unknown key: `[app.io-cli]` is a section +/// io-harness does not validate, and a newer io-cli's key must not stop an older +/// one from starting. This table is a leaf with eight names and one job, and the +/// commonest way to get it wrong is a typo — `comand = ["cargo", "test"]` +/// deserializes to the default, compares equal to it, and answers `Ok(None)`: no +/// refusal, no notice, no gate. The whole point of [`Refusal::Empty`] is that a +/// section which plainly means to be a gate and is not one says so, and a typo is +/// exactly that case. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Settings { + /// How many further turns a failing gate may buy, defaulting to one. + /// + /// Zero means the gate reports and nothing is re-driven. It is a small + /// number on purpose: a retry is a whole turn against a real model, and an + /// operator who wants several says so. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub retries: Option, + + /// The argv of a command that must exit [`Settings::expect_exit`]. + /// + /// An argv rather than a command line, because io-harness checks `argv[0]` + /// against the policy and runs it without a shell. An empty vector means the + /// command the repository's own toolchain proposes. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub command: Option>, + + /// The exit status the command must report, defaulting to zero. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub expect_exit: Option, + + /// A workspace-relative file that must exist. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub file: Option, + + /// Text [`Settings::file`] must contain. Absent asserts existence alone. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub contains: Option, + + /// What a second model is asked to judge the work against. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub rubric: Option, + + /// The model that answers [`Settings::rubric`]. + /// + /// Required whenever a rubric is set, and never defaulted to the model doing + /// the work — a judge marking its own paper is a decision the operator makes + /// explicitly through [`Settings::allow_self_review`]. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub reviewer: Option, + + /// Whether the reviewer may be the same model that did the work. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub allow_self_review: Option, +} + +/// The one criterion a [`Settings`] resolves to. +/// +/// Distinct from [`Settings`] because a section is what was written and a +/// criterion is what survived being checked. Everything downstream — the +/// contract, the status line, the retry, the exit code — takes this. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Criterion { + /// A command that must exit a named status, run by io-harness in its sandbox. + Command { + /// The program and its arguments. Never a shell line. + argv: Vec, + /// The status that counts as passing. + expect_exit: i32, + }, + /// A workspace-relative file that must exist, and optionally say something. + File { + /// The path, relative to the workspace root. + file: PathBuf, + /// Text the file must contain, or `None` to assert existence alone. + contains: Option, + }, + /// A rubric a second model answers. + Review { + /// What the reviewer is asked to judge. + rubric: String, + /// The model that answers it. + reviewer: String, + /// Whether that model may be the one that did the work. + allow_self_review: bool, + }, +} + +/// Why a section cannot become a [`Criterion`]. +/// +/// Each of these is refused where the operator can still see what they typed. +/// The last two exist because io-harness answers them with `Error::Config` at run +/// start, which would turn writing a configuration file into a session that will +/// not start. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Refusal { + /// The section names no criterion at all. + Empty, + /// The section names more than one kind, and there is no right answer. + Ambiguous, + /// A rubric was given with no model to answer it. + ReviewerMissing, + /// A command criterion was written with no program in it. + /// + /// Its own arm rather than [`Refusal::Empty`], because the operator plainly + /// meant a command and the fix is to name one. io-harness answers an empty + /// argv with `Error::Config` when the gate first runs — every turn, before + /// anything useful happens — so this is the same class of mistake as a rubric + /// with no reviewer and is caught in the same place. + NoProgram, + /// The reviewer is the model doing the work, and that was not asked for. + SelfReview { + /// The model named as both the worker and the judge. + model: String, + }, +} + +impl Settings { + /// The one criterion this section resolves to, or why it cannot. + /// + /// `Ok(None)` is the answer for a section that is entirely absent — every key + /// unset. That is not the same as [`Refusal::Empty`], which is a section an + /// operator plainly meant to be a gate (`retries` is set, or `contains`, or a + /// `reviewer`) and which names no kind: the first is "no gate was asked for" + /// and the second is "a gate was asked for and half-written", and answering + /// both with silence would let a typo turn a gate off without a word. + /// + /// `turn_model` is the model the session will actually run with, passed in + /// rather than read here, so this function stays a pure decision over its + /// arguments and the tests need no configuration on disk. An empty string + /// means the caller does not yet know which model will run — the wizard, a + /// fresh home directory — and the self-review refusal cannot fire, because + /// there is nothing to compare the reviewer against. It is not defaulted to + /// "no clash": the caller that knows the model is the caller that must say it. + /// + /// The order of the checks is the order an operator can act on. Ambiguity is + /// answered before emptiness because the two cannot both be true; a missing + /// reviewer is answered before self-review because a reviewer that was never + /// named cannot be compared to anything. + pub fn criterion(&self, turn_model: &str) -> Result, Refusal> { + let kinds = u8::from(self.command.is_some()) + + u8::from(self.file.is_some()) + + u8::from(self.rubric.is_some()); + if kinds > 1 { + return Err(Refusal::Ambiguous); + } + if kinds == 0 { + // Compared against the default rather than against a hand-written + // list of the remaining keys: a key added to `Settings` later joins + // this check by existing, instead of by somebody remembering to add + // it here and shipping a silent hole when they do not. + return if self == &Settings::default() { + Ok(None) + } else { + Err(Refusal::Empty) + }; + } + + if let Some(argv) = &self.command { + // An empty argv is not a criterion. Left to io-harness it becomes + // `Error::Config` at the first gated step of every turn, recorded as + // `Errored` — and because an errored gate is worth another turn, the + // retry then buys a second one that fails identically. + if argv.is_empty() { + return Err(Refusal::NoProgram); + } + return Ok(Some(Criterion::Command { + argv: argv.clone(), + expect_exit: self.expect_exit.unwrap_or(0), + })); + } + if let Some(file) = &self.file { + return Ok(Some(Criterion::File { + file: file.clone(), + contains: self.contains.clone(), + })); + } + + let rubric = self + .rubric + .clone() + .expect("one kind is set and it is the rubric"); + let Some(reviewer) = self.reviewer.clone() else { + return Err(Refusal::ReviewerMissing); + }; + let allow_self_review = self.allow_self_review.unwrap_or(false); + if !allow_self_review && !turn_model.is_empty() && reviewer == turn_model { + return Err(Refusal::SelfReview { model: reviewer }); + } + Ok(Some(Criterion::Review { + rubric, + reviewer, + allow_self_review, + })) + } + + /// How many further turns a failing gate may buy, with the default applied. + /// + /// One, not zero and not three. Zero would make the whole retry half of this + /// release opt-in, and an operator who configured a gate has already said they + /// want the work checked; more than one spends real money on a model that has + /// already been told once what it got wrong. + pub fn retries(&self) -> u8 { + self.retries.unwrap_or(1) + } +} + +impl Criterion { + /// The criterion as the dependency's own type, ready for `with_verification`. + /// + /// Three of the four mappings are direct. The fourth — a file criterion with + /// no needle — is the one that has no honest counterpart, and it is worth + /// saying exactly why rather than picking the nearest variant: + /// + /// `Verification::WorkspaceFileContains` reads its file with + /// `read_to_string(..).unwrap_or_default()`, so a file that is not there is + /// the empty string, and every string contains the empty needle. Mapping + /// existence to an empty needle would therefore produce a gate that passes on + /// a file nobody ever wrote — a criterion that can never fail, which is worse + /// than no criterion at all, because a run reports `Success` on it. + /// `DocumentContains` does error on a file it cannot read, but it errors on + /// nearly every file: it reads four office formats by extension and refuses + /// the rest, so it would report a criterion that could not be evaluated rather + /// than one that failed. Nothing else in the enum reads a path. + /// + /// So existence is answered by [`Criterion::satisfied_in`], here, with a + /// reader that tells a missing file from an empty one — and this returns + /// `Verification::None`, which is the truthful statement that io-harness was + /// asked to check nothing. A caller must therefore ask + /// [`Criterion::checked_here`] before treating a run's own outcome as the + /// whole verdict; that is what the flag is for. + pub fn verification(&self) -> Verification { + match self { + Criterion::Command { argv, expect_exit } => Verification::Command { + argv: argv.clone(), + expect_exit: *expect_exit, + }, + Criterion::File { + file, + contains: Some(needle), + } => Verification::WorkspaceFileContains { + file: file.clone(), + needle: needle.clone(), + }, + Criterion::File { contains: None, .. } => Verification::None, + Criterion::Review { + rubric, + allow_self_review, + .. + } => Verification::Review { + rubric: rubric.clone(), + allow_self_review: *allow_self_review, + }, + } + } + + /// Whether this crate evaluates the criterion itself rather than the run loop. + /// + /// True for exactly one shape — a file criterion with no needle — and the + /// reason it is a question a surface can ask without a workspace root is that + /// "the run finished and its gate passed" is not the whole verdict for such a + /// criterion. A caller that reads [`Criterion::verification`] and nothing else + /// would see `Verification::None` and report an ungated run. + pub fn checked_here(&self) -> bool { + matches!(self, Criterion::File { contains: None, .. }) + } + + /// The verdict for a criterion this crate owns, or `None` when io-harness does. + /// + /// The read goes through `io_harness::tools::Workspace::read_bytes`, and the + /// choice of reader is the whole check: `Workspace::read_file` answers `Ok("")` + /// for a file that is not there, so a criterion written on top of it passes on + /// a file that was never created. `read_bytes` is the one that errors. Going + /// through `Workspace` rather than `std::fs` also keeps the path resolution + /// the rest of this crate uses, so a `file` that climbs out of the workspace + /// is refused here exactly as it is everywhere else. + /// + /// A directory at that path is not a satisfied criterion: reading it fails, + /// which is the answer an operator who wrote a file name wanted. + pub fn satisfied_in(&self, root: &Path) -> Option { + let Criterion::File { + file, + contains: None, + } = self + else { + return None; + }; + // Separators normalised the way io-harness normalises them before it + // resolves a workspace-relative path, so a path an operator typed with + // backslashes means the same file on either platform. + let relative = file.to_string_lossy().replace('\\', "/"); + Some( + io_harness::tools::Workspace::new(root) + .read_bytes(&relative) + .is_ok(), + ) + } + + /// What the criterion asks, in one sentence, for a prompt or a surface. + /// + /// Delegates to the dependency for everything it can express, so the words a + /// retried turn reads are the same words the first turn was judged by. The + /// existence criterion has no `Verification` to delegate to and says so here. + pub fn describe(&self) -> String { + if let Criterion::File { + file, + contains: None, + } = self + { + return format!("the file {} must exist in the workspace", file.display()); + } + self.verification().describe() + } +} + +/// The gate's standing after a turn, as a surface draws it. +/// +/// Read off the stored rows rather than remembered, because a resumed session did +/// not run the turn whose gate it has to report, and a field that is only correct +/// when this process happened to watch the run is a field that lies after every +/// `/resume`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Standing { + /// Which criterion ran, as io-harness recorded it — one of `command`, + /// `review`, `compiles`, `document`, `contains`, `none`. Taken from the row + /// rather than from the configured [`Criterion`]: the row is what actually + /// ran, and a configuration edited mid-session would otherwise relabel a gate + /// that was never it. + pub phase: String, + /// How the last attempt ended. + pub outcome: GateOutcome, + /// Which attempt that was, counting from one. + pub attempt: usize, +} + +/// The gate's standing for a run, or `None` when nothing has been gated. +/// +/// Pure over the rows the caller already read, so nothing here opens a store, and +/// nothing here asks what time it is — the attempt number is a count of rows, not +/// an interval. `attempts` is expected in the ascending row order +/// `Store::gate_attempts` returns; the last row is the standing. +pub fn standing(attempts: &[GateAttempt]) -> Option { + let last = attempts.last()?; + Some(Standing { + phase: last.phase.clone(), + outcome: last.outcome, + attempt: attempts.len(), + }) +} + +/// What a failing gate printed on `step`, or `None` if it printed nothing. +/// +/// The output of a gate command is not in the gate row — `GateAttempt::detail` +/// carries a verdict's reasons — it arrives as sandbox events of kind +/// `gate_output`, already bounded in size by io-harness. Filtered by step so that +/// a retried gate's prompt carries the failure that caused the retry rather than +/// the first one, which is the difference between telling the model what is wrong +/// now and telling it what was wrong two turns ago. +/// +/// More than one row for a step is joined in order: io-harness records one per +/// failing phase, and dropping all but the last would drop the half that explains +/// the other. +pub fn output(events: &[SandboxEvent], step: u32) -> Option { + let text: Vec<&str> = events + .iter() + .filter(|event| event.kind == "gate_output" && event.step == step) + .filter_map(|event| event.detail.as_deref()) + .collect(); + if text.is_empty() { + None + } else { + Some(text.join("\n")) + } +} + +/// Whether a failing gate has a retry left under `retries`. +/// +/// **Deliberately not `GateOutcome::is_retryable`, which answers a different +/// question.** That method asks whether re-running the *same* criterion over the +/// *same* tree could honestly say something else, and for `Failed` the answer is +/// no. This asks whether it is worth driving *another turn* — after which the +/// tree is not the same, because the agent has been handed the failure and sent +/// back to work. `Errored` retries too: a gate that could not be evaluated has +/// judged nothing. +/// +/// No attempts at all is not a retry: nothing has failed yet. +pub fn may_retry(attempts: &[GateAttempt], retries: u8) -> bool { + let Some(standing) = standing(attempts) else { + return false; + }; + standing.outcome != GateOutcome::Passed && standing.attempt <= usize::from(retries) +} + +/// The test command this repository proposes for itself, if it has one. +/// +/// **This crate holds no list of marker filenames and no list of test commands, +/// and that is the point of the function.** Both lists live in +/// `io_harness::toolchain`, which is where the ecosystems are added; a copy here +/// would be a second list that drifts, and the way it drifts is by offering a +/// Rust command in a repository with no Rust in it. `Config::toolchain` then +/// layers whatever the operator wrote in their own `[toolchain.*]` section over +/// the detected answer, so an operator who runs a different test runner is offered +/// theirs rather than the ecosystem's default. +/// +/// `None` means the repository said nothing about itself, and nothing is proposed. +/// There is deliberately no fallback: a proposal is a suggestion an operator +/// accepts with a keystroke, and one that is wrong is worse than one that is +/// absent, because it is accepted just as easily. +pub fn proposed_command(root: &Path, config: &Config) -> Option> { + let tuned = config.toolchain(io_harness::toolchain::detect(root)?); + // An override can name an empty command, which is an operator saying this + // ecosystem has no test step here. Proposing an empty argv would produce a + // criterion with no program in it. + (!tuned.test.is_empty()).then_some(tuned.test) +} + +// **The reviewer is built in `crate::provider` and not here, and the gate that +// says so is `tests/provider.rs`.** Every vendor type must be constructed in +// exactly one place outside the wizard's handshake and the `/provider` panel, so +// that the interactive and the headless entry points cannot drift apart — and a +// reviewer built here would have been a second site with its own key resolution, +// which is precisely the drift that gate exists to catch. It caught this release +// writing one. See `crate::provider::reviewer`. + +/// The gate attempts a turn is judged by, including the one io-harness never made. +/// +/// **Here rather than in `crate::app`, because `io exec` needs it and may not +/// reach the interface.** `tests/exec.rs` forbids the headless modules from naming +/// `crate::app` at all — a renderer on that path is how ANSI ends up in a stream +/// something is parsing — and the first version of this release put the fold there +/// and had the headless arm call it. That gate caught it. The fold is gate +/// semantics, not interface, and this is where the rest of those live. +/// +/// **A run with no criterion has no standing, and saying otherwise is how this +/// release nearly shipped a catastrophe.** io-harness evaluates the contract's +/// criterion after *every* step on which the agent called a tool +/// (`run/step.rs:1654`), and for `Verification::None` the evaluation is +/// `Ok(false)` (`verify.rs:1361`) — so it records `phase = "none", +/// GateOutcome::Failed`, once per tool-calling step, on **every ungated run this +/// product has ever driven**. Read back naively that says the gate failed: the +/// status line would read `gate failed` under every ordinary turn, and `io exec` +/// would exit `6` for every operator who has never configured a gate at all. +/// +/// Dropping every `none` row is the whole of the fix, and it needs to know nothing +/// about what is configured: an ungated run's rows are *all* `none`, so filtering +/// them empties the list for exactly those sessions, while a caller reporting a +/// past run still gets what io-harness actually recorded. +/// +/// (An earlier draft of this comment said a `none` row records *passed*. It +/// records **failed**. That difference is the whole defect.) +/// +/// The other half is the criterion io-cli owns. [`Criterion::verification`] maps a +/// bare existence criterion — `file` with no `contains` — to `Verification::None`, +/// because io-harness has no honest counterpart for it, so nothing in the store +/// ever speaks for it. It is evaluated here through [`Criterion::satisfied_in`], +/// the reader that tells a missing file from an empty one, and appended as an +/// attempt of its own — after which it flows through [`standing`], [`may_retry`] +/// and the report exactly as a recorded one does. One path, not two. +/// +/// Every other criterion is io-harness's to judge and the rows come back +/// untouched. +pub fn gate_attempts( + mut recorded: Vec, + criterion: Option<&Criterion>, + root: &Path, +) -> Vec { + recorded.retain(|attempt| attempt.phase != "none"); + // `satisfied_in` answers `None` for every criterion the run loop evaluates, + // so this one call is both the question "is this ours?" and its answer — + // asking `checked_here` first would be the same test written twice. + let Some(satisfied) = criterion.and_then(|criterion| criterion.satisfied_in(root)) else { + return recorded; + }; + let step = recorded.last().map_or(0, |attempt| attempt.step); + recorded.push(GateAttempt { + // Not a stored row and never read back as one: `id` addresses a row in + // `gate_attempts` and this attempt was made here, so it has none. + id: 0, + step, + // Not `none`, which is what io-harness would have called it, and not + // `contains`, which is the phase of a criterion that reads a file's + // contents. This one asserts existence and says so. + phase: "exists".to_string(), + outcome: if satisfied { + GateOutcome::Passed + } else { + GateOutcome::Failed + }, + // Empty on both arms. The row's `detail` is what the report reads back as + // "this is what it reported", and for this criterion the only thing io-cli + // could put there is the criterion itself — which the report line and the + // retry prompt have already said one clause earlier. + detail: String::new(), + // A stored timestamp this crate never parses and never compares, and the + // one field that would need a clock. `src/main.rs` is the only module + // allowed one, so it stays empty rather than being invented here. + at: String::new(), + }); + recorded +} + +/// One sentence an operator can act on, for each way a section is refused. +/// +/// ASCII throughout — no quotation marks that are not the typewriter kind, no +/// arrows, no ellipsis character. These render on the plain renderer, under +/// `NO_COLOR`, and through the ASCII glyph set, and a refusal that arrives as a +/// replacement character is a refusal nobody reads. +/// +/// Each one names the key to change, because the operator is looking at the file. +impl fmt::Display for Refusal { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Refusal::Empty => f.write_str( + "this gates section sets no criterion: name a command, a file or a \ + rubric, or delete the section", + ), + Refusal::Ambiguous => f.write_str( + "this gates section names more than one criterion: keep exactly one \ + of command, file and rubric", + ), + Refusal::ReviewerMissing => f.write_str( + "a rubric needs a model to answer it: set reviewer, or delete the \ + rubric", + ), + Refusal::NoProgram => f.write_str( + "this gates section sets an empty command: name the program to run, \ + such as command = [\"cargo\", \"test\"]", + ), + Refusal::SelfReview { model } => write!( + f, + "the reviewer {model} is the model doing the work: name a different \ + reviewer, or set allow_self_review = true to accept a model \ + marking its own paper" + ), + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 380e077..ef54c3a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -51,6 +51,7 @@ pub mod failure; pub mod fanout; pub mod fleet; pub mod fuzzy; +pub mod gates; pub mod glyphs; pub mod home; pub mod import; @@ -71,6 +72,7 @@ pub mod queue; pub mod recall; pub mod reload; pub mod resume; +pub mod reviewer; pub mod rewind; pub mod servers; pub mod sessions; diff --git a/src/main.rs b/src/main.rs index cfbcf43..1bfc088 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1504,6 +1504,10 @@ async fn loop_over Result>( vec![ Row::new("leave it as it is"), Row::new("remove this server"), + Row::with_detail( + "change one setting", + io_cli::servers::KEYS.join(", "), + ), ], ), Pick::McpRemove { @@ -1517,7 +1521,25 @@ async fn loop_over Result>( } // Row 0 is "leave it", the default that does nothing — // the shape `/skills` and `/plugin` both already use. + // Row 2 is the edit verb, and it descends rather than + // acting: which key is half the decision, and the other + // half is a value only the operator can type. Pick::McpRemove { id, at } => { + if index == 2 { + descended = Some(( + Picker::new( + format!("Change what about {id}?"), + io_cli::servers::KEYS + .iter() + .map(|key| Row::new(*key)) + .collect(), + ), + Pick::McpEdit { + id: id.clone(), + at: *at, + }, + )); + } if index == 1 { let edit = io_cli::servers::remove(at); match io_cli::configure::write(session.root(), at.scope, &[edit]) { @@ -1550,6 +1572,37 @@ async fn loop_over Result>( } } } + // **The key goes into the composer and the value is + // typed, which is `/config`'s own shape.** A picker + // cannot ask for a URL or a command line, and this + // product has exactly one surface that takes free text. + // + // The line carries the server's **id**, never `at`'s + // index: the composer holds it while the operator types, + // and a file they edit in another window in between would + // move the array under a carried index. `servers::At` is + // resolved again, from the file's own bytes, when the + // line comes back — which is also what decides the scope + // the write goes to, so no scope question is asked for a + // change to an entry that lives in exactly one file. + Pick::McpEdit { id, at } => { + if let Some(key) = io_cli::servers::KEYS.get(index) { + app.record( + Tone::Muted, + format!( + "{id}'s {key} is declared in the {} scope; type the value \ + after the key and press Enter", + io_cli::configure::Decided::File { + scope: at.scope, + path: Default::default(), + } + .word() + ), + ); + let prefix = io_cli::app::SERVER_KEY; + app.composer.set(&format!("/config {prefix}{id}.{key} ")); + } + } // Says what the skill is and offers the one change there // is. The view is read again rather than carried: between // the row being drawn and this keystroke the operator may @@ -2007,6 +2060,76 @@ async fn loop_over Result>( Some(key) => app.composer.set(&format!("/config {key} ")), None => {} }, + // The gates surface. Every row but the first names a key + // and goes to the composer, which is `/config`'s own + // shape and reaches `/config`'s own write — the value of + // a rubric or a path is the operator's to type, and a + // picker cannot ask for one. + Pick::Gates { keys, proposed } => { + match keys.get(index).map(String::as_str) { + Some(io_cli::app::PROPOSED_GATE) => { + // **Refused where the operator can still see + // what they asked for, rather than written + // and discovered at run start.** A section + // that already names a file or a rubric gains + // a second kind from this write, and + // `Settings::criterion` answers `Ambiguous` — + // which `contract::configured` honours by + // running every later turn with NO gate at + // all. That is a config edit that silently + // turns verification off, which is the exact + // failure F5 exists to make impossible. + // + // Checked against a prospective section built + // here rather than against the file, so the + // answer is about the write being offered and + // not about the one already there. The real + // working model goes in, because the refusal + // that compares a reviewer against it cannot + // fire on a guess. + let working = config + .provider_spec() + .map(io_cli::provider::model_of) + .unwrap_or_default() + .to_string(); + let stored = io_cli::settings::stored(&config) + .0 + .and_then(|stored| stored.gates) + .unwrap_or_default(); + let prospective = io_cli::gates::Settings { + command: proposed.clone(), + ..stored + }; + match prospective.criterion(&working) { + Err(refusal) => { + app.record(Tone::Refused, refusal.to_string()) + } + Ok(_) => { + // The argv as TOML, built by + // `edit::array` rather than by a + // format string: an argument with a + // quote or a backslash in it is + // either a parse error or a + // different command. + let argv = proposed.clone().unwrap_or_default(); + let items: Vec<&str> = + argv.iter().map(String::as_str).collect(); + // `descended`, not `picker`: the + // assignment at the bottom of this + // block is unconditional, and `kind` + // still borrows `picker` here. + descended = Some(write_where( + session.root(), + "app.io-cli.gates.command".to_string(), + io_cli::edit::array(&items), + )); + } + } + } + Some(key) => app.composer.set(&format!("/config {key} ")), + None => {} + } + } Pick::ConfigScope { key, value, paths } => { // An index past the end puts nothing in the file, // which is the same answer every other picker arm @@ -2039,6 +2162,34 @@ async fn loop_over Result>( .word() ), ); + // **The one place a gates + // write is checked against + // the model that will do the + // work.** `/gates` sends its + // key rows through this arm, + // and the value is the + // operator's own typing — so + // a rubric with no reviewer, + // or a reviewer that IS the + // working model, is refused + // by `Settings::criterion` + // here rather than by + // io-harness at run start, + // where the failure arrives + // disconnected from the + // keystroke that caused it. + // Read off the reloaded + // configuration, so what is + // judged is the file as it + // now stands. Silent when + // the section is fine or + // absent, which is every + // other write. + if let Some(notice) = + io_cli::contract::gate_notice(&config) + { + app.record(Tone::Refused, notice); + } } Err(error) => app.record(Tone::Error, error), } @@ -2423,6 +2574,151 @@ async fn loop_over Result>( )); } } + // **Three questions, answered before anything is offered, because + // an operator opens this surface for exactly one of them.** What + // is being asked of a turn; why a section that is plainly in the + // file is not asking it; and how the last turn's gate actually + // went. The first is a fresh clone's question, the second is a + // typo's, and the third is the one asked after a turn came back + // red — and answering only the first would leave the other two + // discoverable nowhere in the product. + // + // **No contract is built here, deliberately.** `tests/contract.rs` + // counts `contract::session(` in this file by binding name *and* + // in total, and a sixth site is a new arm by that test's own + // reckoning. A surface that reads a criterion needs the `Config` + // and the workspace root, which is all `gates::Settings` and + // `gates::proposed_command` take between them. + Action::Gates => { + let root = session.root().to_path_buf(); + // The model the work will be done by, and the one thing the + // self-review refusal is decided against. Empty where no + // provider is configured, which `Settings::criterion` reads + // as "cannot clash" rather than as a model of that name — + // and it is read here rather than defaulted, because the + // caller that knows the model is the caller that must say it. + let working = config + .provider_spec() + .map(io_cli::provider::model_of) + .unwrap_or_default() + .to_string(); + let section = io_cli::settings::stored(&config) + .0 + .and_then(|stored| stored.gates) + .unwrap_or_default(); + // Every key of the section, resolved by the same reader + // `/config` uses, so the two surfaces cannot disagree about + // what is in force or about which file decided it. + let settings: Vec = io_cli::configure::CATALOGUE + .iter() + .filter(|key| key.starts_with("app.io-cli.gates.")) + .map(|key| io_cli::configure::setting(&config, key)) + .collect(); + // Whichever key a file actually names decides the scope word, + // because that is the file the operator has to open. Three + // files are called `io.toml` and only one of them is theirs. + // **Only a key that names a criterion may decide it.** The + // catalogue's first entry is `retries`, which is a preference + // and not a gate — so an operator with `retries` in their user + // file and `command` in the project's would be sent to the + // wrong one of the three files, by the very line written to + // stop that happening. + let decided = settings + .iter() + .find(|setting| { + setting.value.is_some() + && matches!( + setting.path.rsplit('.').next(), + Some("command" | "file" | "rubric") + ) + }) + .map(|setting| setting.decided.word()) + .unwrap_or("default"); + // A refusal is NOT reported here. `gate_notice` below is the + // one place this product says why a section is not gating a + // run, and it covers both the refusals and the reviewer that + // could not be built — saying it twice would read as two + // different problems. + let criterion = section.criterion(&working).ok().flatten(); + match &criterion { + Some(criterion) => app.record( + Tone::Muted, + format!( + "the gate asks that {}, from the {decided} scope; a turn that \ + fails it earns {} more", + criterion.describe(), + match section.retries() { + 0 => "no turns".to_string(), + 1 => "one turn".to_string(), + many => format!("{many} turns"), + }, + ), + ), + None => app.record( + Tone::Muted, + "no gate is in force, so a turn that stops is a turn that finished", + ), + } + if let Some(notice) = io_cli::contract::gate_notice(&config) { + app.record(Tone::Refused, notice); + } + // How the last turn's gate went, read off the store rather + // than remembered — a resumed session did not run the turn + // whose gate it has to report, and a field that is only right + // when this process watched the run lies after every + // `/resume`. Folded through `app::gate_attempts` for the same + // reason the driver folds it: an existence criterion has no + // recorded row at all, and reading the store alone would + // report an ungated run. + if let Some(run_id) = last_run(&session, &store).map(|turn| turn.run_id) { + // **`None`, not today's criterion, and the difference is + // whether this is a fact.** Folding the configured + // criterion in here would evaluate `satisfied_in` against + // the filesystem *now* and report the answer as the last + // turn's verdict — so a turn that ran before any gate + // existed would be reported as having passed one, and a + // turn that genuinely passed would read as failed the + // moment the file was deleted. What io-harness recorded + // for that run is the only thing here that is a statement + // about the past. + let attempts = io_cli::gates::gate_attempts( + store.gate_attempts(run_id).unwrap_or_default(), + None, + &root, + ); + let events = store.sandbox_events(run_id).unwrap_or_default(); + if let Some((tone, line)) = io_cli::app::gate_report(&attempts, &events) { + app.record(tone, format!("the last turn's {line}")); + } + } + + // **The repository's own proposal first, and it is the one + // row that is an offer rather than a key.** `cargo test`, + // `npm test`, `pytest` — the command the toolchain detected, + // which io-cli holds no list of and could not invent. A gate + // an operator accepts with one keystroke is the difference + // between a gate configured and a gate intended, and a + // proposal that is absent is better than one that is wrong. + let mut rows = Vec::new(); + let mut keys: Vec = Vec::new(); + let proposed = io_cli::gates::proposed_command(&root, &config); + if let Some(argv) = &proposed { + rows.push(Row::with_detail( + format!("gate on `{}`", argv.join(" ")), + "the command this repository proposes for itself".to_string(), + )); + keys.push(io_cli::app::PROPOSED_GATE.to_string()); + } + // Then every key, drawn by `configure` rather than by rows + // spelled here: nothing under `tests/` links this file, so a + // label written in the driver is one no test can assert. + rows.extend(io_cli::configure::rows(&settings)); + keys.extend(settings.iter().map(|setting| setting.path.clone())); + picker = Some(( + Picker::new("What does done mean here?", rows), + Pick::Gates { keys, proposed }, + )); + } // **Both directories, and a failed discovery is drawn rather than // swallowed.** `view` reads the enabled set through the same // `Skills::discover` the run makes, so what this lists is what the @@ -2657,36 +2953,86 @@ async fn loop_over Result>( format!("{key} is {what} ({})", setting.decided.word()), ); } - Action::Config(Some((key, value))) => { - // Every scope, whether or not its file exists yet — writing a - // key into a scope for the first time is how this is used. + // **`/mcp`'s edit verb comes back through here, and it comes back + // addressed by the server's own id.** The composer line `/mcp` + // puts up is `mcp..`, never `mcp[3].`: an index is a + // position in one file's `[[mcp]]` array, and the operator is + // about to be asked which file to write into. Handing one list's + // index to another file's array is precisely the silent wrong + // delete 0.20.0 shipped in `pluginview::rows`, and `servers::At` + // exists so it cannot be spelled. So the entry is found again, by + // content, in whichever scope declares it — and the write goes to + // that scope with no question asked, because there is only one + // file the entry is in. + Action::Config(Some((key, value))) + if key.starts_with(io_cli::app::SERVER_KEY) && !value.is_empty() => + { let root = session.root().to_path_buf(); - let paths: Vec<(io_harness::config::Scope, std::path::PathBuf)> = [ - io_harness::config::Scope::User, - io_harness::config::Scope::Project, - io_harness::config::Scope::Local, - ] - .into_iter() - .filter_map(|scope| { - io_cli::configure::scope_path(&root, scope).map(|p| (scope, p)) - }) - .collect(); - - let rows: Vec = paths - .iter() - .map(|(scope, path)| { - let word = io_cli::configure::Decided::File { - scope: *scope, - path: path.clone(), - }; - Row::with_detail(word.word().to_string(), path.display().to_string()) - }) - .collect(); - - picker = Some(( - Picker::new(format!("Write {key} where?"), rows), - Pick::ConfigScope { key, value, paths }, - )); + match io_cli::app::server_key(&key) { + None => app.record( + Tone::Refused, + format!("{key} names no MCP server and no key of one"), + ), + Some((id, field)) => match io_cli::servers::declared_in(&root, id) { + None => app.record( + Tone::Refused, + format!( + "no configuration file in force declares {id}, so there is \ + nothing here to change" + ), + ), + // **Reported, never dropped.** `[[mcp]]` is not held + // to `deny_unknown_fields`, so a key io-harness does + // not know is written, accepted and ignored — the + // operator would be told their change landed while + // the server went on running the old value. That is + // the whole reason `servers::edit` returns an option. + Some(at) => match io_cli::servers::edit( + &at, + field, + &io_cli::app::server_value(field, &value), + ) { + None => app.record( + Tone::Refused, + format!( + "{field} is not a key an [[mcp]] entry may carry; the \ + keys are {}", + io_cli::servers::KEYS.join(", ") + ), + ), + Some(edit) => { + match io_cli::configure::write(&root, at.scope, &[edit]) { + Ok(()) => { + // Both halves, or the next turn talks + // to the server the file described at + // startup. + match io_cli::configure::reload(&root) { + Ok((fresh, stored)) => { + capabilities = + io_cli::contract::Capabilities::stored( + stored.as_ref(), + ); + config = fresh; + app.record( + Tone::Success, + format!( + "{id}'s {field} is now {value}; the \ + next turn talks to it that way" + ), + ); + } + Err(error) => app.record(Tone::Error, error), + } + } + Err(refusal) => app.record(Tone::Refused, refusal), + } + } + }, + }, + } + } + Action::Config(Some((key, value))) => { + picker = Some(write_where(session.root(), key, value)); } // A line typed with nothing after the word. Answered with what to // type rather than by opening a picker over three files it would @@ -3227,6 +3573,15 @@ async fn loop_over Result>( // pass rather than the queue being drained into a list up front. Command::Submit(text) => { let mut next = Some(text); + // **Every gate attempt this chain of turns has made, oldest + // first, and it is carried because io-harness cannot carry it.** + // Every turn is its own run, so `Store::gate_attempts` starts + // again at one on each retry — and `gates::may_retry`, which asks + // how many attempts the *work* has had, would then buy another + // turn for every one of them, forever. Accumulating here is what + // makes `retries = 1` mean one further turn rather than an + // unbounded loop against a real model. + let mut gated: Vec = Vec::new(); while let Some(text) = next.take() { // Rebuilt every turn rather than kept, because `remembered` // grows as the operator answers and the harness's own @@ -3322,7 +3677,7 @@ async fn loop_over Result>( .0; let effective = approval::session_policy(&policy, app.posture(), app.remembered()); - let stopped = turn( + let turned = turn( screen, inputs, &mut app, @@ -3367,7 +3722,7 @@ async fn loop_over Result>( // of the operator. Firing the queue here would turn one press // into three more turns against a conversation they had just // decided to steer somewhere else. - if stopped { + if turned.stopped { let dropped = app.forget_queued_prompts(); if dropped > 0 { let dash = app.theme.glyphs.dash; @@ -3377,6 +3732,105 @@ async fn loop_over Result>( ); } } + + // **The gate, read after the turn and before the next + // prompt.** Nothing that arrives through the event stream can + // say any of this: `EventKind::Sandbox` carries no detail + // payload at all, so the phase, the verdict and the output are + // the store's — which is also what makes it right after a + // `/resume`, because the rows outlive the process that watched + // the run. + // + // Every decision below is a library call. The two `if`s here + // are wiring — *did a run happen* and *did the operator stop + // it* — because nothing under `tests/` links this file and a + // decision written as a branch in the driver is one no test + // can drive and no sabotage can make fail. + let mut retry = None; + if let Some(run_id) = turned.ran { + let working = config + .provider_spec() + .map(io_cli::provider::model_of) + .unwrap_or_default() + .to_string(); + let section = io_cli::settings::stored(&config) + .0 + .and_then(|stored| stored.gates) + .unwrap_or_default(); + let criterion = section.criterion(&working).ok().flatten(); + // **The fold, and it is the one thing here that cannot be + // skipped.** io-harness is handed `Verification::None` for + // a bare existence criterion — there is no honest + // counterpart for it in its enum — and its step loop + // returns `Finished` without ever reaching the gate, so + // the store holds NO row. Reading `gate_attempts` alone + // would report an ungated run as a run that passed. + // `app::gate_attempts` evaluates that one criterion here + // and appends the attempt io-harness never made. + // **One row per TURN, and taking them all is how the + // retry died in review.** io-harness evaluates the + // criterion after *every* step the agent takes, not once + // when it stops — `run/step.rs:1654` sits inside the step + // loop — so one nine-step turn writes nine rows. Extending + // by all of them makes `standing.attempt` the step count, + // so `may_retry` compares nine against a budget of one and + // answers no, and the headline feature of this release + // never fires for any criterion except the bare-existence + // one. The run's last row is its verdict; the count of + // those is the count of turns. + gated.extend( + io_cli::gates::gate_attempts( + store.gate_attempts(run_id).unwrap_or_default(), + criterion.as_ref(), + session.root(), + ) + .pop(), + ); + let events = store.sandbox_events(run_id).unwrap_or_default(); + if let Some(standing) = io_cli::gates::standing(&gated) { + // `GateOutcome::as_str` passed through verbatim: it + // spells `passed`, `failed` and `errored`, and the + // status line, the exit code and the scrollback all + // have to say one verdict the same way. + app.status.gate = Some(standing.outcome.as_str().to_string()); + app.status.gate_attempt = u32::try_from(standing.attempt).ok(); + } + // `record` and not `say`: a verdict is an account of the + // turn above it and would be gone at the next keystroke. + if let Some((tone, line)) = io_cli::app::gate_report(&gated, &events) { + app.record(tone, line); + } + // **A turn the operator stopped is never retried**, which + // is the same rule that drops the rest of the queue eight + // lines up: one press of the stop key must not start + // another turn. `retries = 0` drives nothing because + // `may_retry` answers no — the budget is not consulted + // here, it is passed in. + if !turned.stopped && io_cli::gates::may_retry(&gated, section.retries()) { + retry = criterion.as_ref().map(|criterion| { + io_cli::app::gate_retry(criterion, &gated, &events) + }); + } + } + // **The retry rides the queue this loop already drains.** It + // goes to the FRONT, so it is the next turn and the prompts an + // operator typed during this one keep their order behind it. + // A second loop written beside this one would be a second + // place `Ctrl+C`, the picture drain and the configuration + // refresh could drift. + if let Some(prompt) = retry { + app.requeue_prompts(vec![prompt]); + let dash = app.theme.glyphs.dash; + app.record( + Tone::Muted, + format!("the gate gets one more turn {dash} it is told what failed"), + ); + } else { + // The chain is over and the next prompt starts its own. + // Attempts carried across it would charge a fresh turn for + // a gate that failed two prompts ago. + gated.clear(); + } next = app.next_queued_prompt(); } } @@ -3504,12 +3958,22 @@ fn commit_viewed( } } -/// One turn, with the keyboard live throughout so `Ctrl+C` can reach it. +/// What the caller of [`turn`] still has to decide about. /// -/// Returns whether the operator asked this turn to stop — the one thing about a -/// turn the caller cannot see from outside it, and the thing the caller now has -/// to decide about, because a prompt queue that fired after a stop would make the -/// stop key start three more turns. +/// Two facts, and neither is visible from outside the turn. The stop is the older +/// one: a prompt queue that fired after `Ctrl+C` would make one press of the stop +/// key start three more turns. The run id is 0.24.0's, and it is here rather than +/// re-derived from the transcript because a turn that ended on an error has no run +/// to report a gate for — asking the store for "the last run" would find the +/// *previous* turn's and report its verdict under this one. +struct Turned { + /// Whether the operator asked this turn to stop. + stopped: bool, + /// The run that served it, where one ran to a result at all. + ran: Option, +} + +/// One turn, with the keyboard live throughout so `Ctrl+C` can reach it. #[allow(clippy::too_many_arguments)] async fn turn( screen: &mut Screen>, @@ -3542,7 +4006,7 @@ async fn turn( fold: bool, text: String, started: Instant, -) -> Result { +) -> Result { let (observer, mut events) = bridge::channel(); // The one way a turn is stopped from the interface, contained or not. Both // arms take a contract **and** a steer inbox since 0.17.0, and the stop key @@ -4189,6 +4653,39 @@ async fn turn( ); } + // The run this turn served, for the gate the caller reads afterwards. Only a + // turn that ran to a result has one: a run that came back `Err` was not judged, + // and a turn the operator abandoned has whatever io-harness had written when + // the future was dropped — neither is a verdict on the work. + // + // **And `Ok` is not the same as ended, which is a defect this nearly shipped.** + // `Some(Ok(_))` also covers `AwaitingAnswer`, `AwaitingPlan`, + // `AwaitingRecovery`, `Denied`, `Refused`, `PlanRejected`, `Stalled`, + // `Escalated` and `Cancelled`. A parked run judged by the gate is told + // `/resume opens it` four lines below and then, in the same pass, buried + // under a fresh billed retry turn that tells the model its work failed — + // while the question the operator was asked is never answered. A gate is a + // verdict on work that finished, so only the outcomes that mean the run + // reached its own end are handed over. `exec::code` already owns that + // classification and is reused rather than re-derived. + // + // **A ceiling counts, and the live rehearsal is why.** A run whose criterion + // keeps failing spends its whole step budget doing so and comes back + // `StepCapReached` — that is what 0.24.0's own live arm recorded, `Failed` at + // attempt six. Excluding ceilings would mean the commonest failing-gate ending + // was never judged and never retried, which is the opposite of the intent. + let ran = match &outcome { + Some(Ok(result)) + if matches!( + io_cli::exec::code(&result.outcome), + io_cli::exec::OK | io_cli::exec::CEILING + ) => + { + Some(result.run_id) + } + _ => None, + }; + match outcome { // The operator's sentence in front of the harness's own line — see // `io_cli::failure`. A provider that will not take an image says so in the @@ -4222,7 +4719,7 @@ async fn turn( } app.status.elapsed = started.elapsed(); paint(screen, app)?; - Ok(stopped) + Ok(Turned { stopped, ran }) } /// Draw what a step changed, by asking the store what it recorded. @@ -4909,10 +5406,40 @@ enum Pick { /// rather than read off a label because a label is a rendered string and the /// key is what a write addresses. Config(Vec), - /// The MCP servers, in the order `servers::rows` drew them. Read-only in - /// this release: the write verbs are reached through `/config`, and the two - /// the roadmap named that io-harness cannot express are not offered at all. + /// The MCP servers, in the order `servers::rows` drew them. + /// + /// **No longer read-only.** Through 0.23.0 this surface could show a server + /// and remove one, and every other change had to be made by hand in the file + /// — a limitation the product had stated since 0.21.0 while + /// `servers::edit` sat in the library, tested and reachable from no + /// keystroke. 0.24.0 wires it: the row descends into [`Pick::McpRemove`], + /// which offers the edit as well as the removal, and the key goes to the + /// composer so the value can be typed. Both verbs address the entry through + /// its own [`io_cli::servers::At`] and never through a row index. Mcp, + /// One MCP server and the keys of it that may be changed, in `servers::KEYS` + /// order. The position is carried rather than re-derived for the reason + /// [`Pick::McpRemove`] carries one: this surface's row index addresses a + /// merged view across three scopes, and the `[[mcp]]` array a write is + /// spliced into is a different list entirely. + McpEdit { + id: String, + at: io_cli::servers::At, + }, + /// The gates surface, in the order its rows were drawn, carried as the key + /// each row stands for — parallel to the rows and built in the same pass, the + /// arrangement [`Pick::Config`] uses and for the same reason: a label is a + /// rendered string and a key is what a write addresses. + /// + /// One row is not a key: [`io_cli::app::PROPOSED_GATE`] is the sentinel for + /// the command this repository proposes for itself, which is an act rather + /// than a setting. `proposed` is the argv behind it, carried so the write and + /// the row cannot disagree — re-detecting it on the keystroke would re-read + /// the filesystem underneath the operator. + Gates { + keys: Vec, + proposed: Option>, + }, /// The provider chain, in the order `providers::rows` drew it — which is /// the order a turn tries it. Provider, @@ -5022,6 +5549,44 @@ enum Pick { }, } +/// The picker that asks which file a `key = value` write goes into. +/// +/// **Two steps rather than one because *which scope* is half the decision and this +/// product has three of them** — a write that guessed would put an operator's +/// credential in the file a repository ships. `/config` has asked it since 0.9.0 +/// and `/gates` asks exactly the same question about exactly the same three files, +/// so it is one function: two copies would be two answers the first time a scope +/// moved. +/// +/// Every scope is offered whether or not its file exists yet, because writing a +/// key into a scope for the first time is how this is used. +fn write_where(root: &std::path::Path, key: String, value: String) -> (Picker, Pick) { + let paths: Vec<(io_harness::config::Scope, std::path::PathBuf)> = [ + io_harness::config::Scope::User, + io_harness::config::Scope::Project, + io_harness::config::Scope::Local, + ] + .into_iter() + .filter_map(|scope| io_cli::configure::scope_path(root, scope).map(|p| (scope, p))) + .collect(); + + let rows: Vec = paths + .iter() + .map(|(scope, path)| { + let word = io_cli::configure::Decided::File { + scope: *scope, + path: path.clone(), + }; + Row::with_detail(word.word().to_string(), path.display().to_string()) + }) + .collect(); + + ( + Picker::new(format!("Write {key} where?"), rows), + Pick::ConfigScope { key, value, paths }, + ) +} + /// The completion picker over one directory of the workspace, or `None` when the /// policy leaves nothing in it to offer. /// diff --git a/src/provider.rs b/src/provider.rs index 0c899db..2fe76d4 100644 --- a/src/provider.rs +++ b/src/provider.rs @@ -230,6 +230,112 @@ impl Provider for Watched

{ } } +/// A provider that can be printed, which is the only thing io-harness's own +/// reviewer is missing. +/// +/// `ModelReviewer

` implements `Reviewer` — the trait `TaskContract::with_reviewer` +/// takes — only for `P: Provider + Debug + Send + Sync`, because `ModelReviewer` +/// derives `Debug` and the derive puts the bound on `P`. **None of io-harness's +/// own provider types implements `Debug`**, so a `ModelReviewer` built over one of +/// them cannot be handed to `with_reviewer` at all, and the model-judged half of +/// the verification pillar is unreachable from any crate that uses the providers +/// the harness ships. Reported upstream as io-harness#213; this is io-cli +/// shipping around it rather than waiting. +/// +/// The `Debug` impl prints the vendor name and **never the credential**. That is +/// the whole reason it is written by hand rather than derived: a derive here would +/// put an API key into every error that formatted a reviewer. +/// +/// Every method delegates, for the reason [`Watched`] gives at length — a default +/// that fired here would change behaviour silently. This wrapper records nothing; +/// a reviewer's request is not the operator's conversation and has no business in +/// what `/context` reports. +pub struct Printable

{ + inner: P, +} + +impl

Printable

{ + /// Wrap `inner` so it can satisfy a `Debug` bound. + pub fn new(inner: P) -> Self { + Self { inner } + } +} + +impl std::fmt::Debug for Printable

{ + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // The name and nothing else. `endpoint()` is safe but uninformative here, + // and every other accessor either carries or is derived from the key. + f.debug_struct("Printable") + .field("provider", &self.inner.name()) + .finish() + } +} + +impl Provider for Printable

{ + fn complete( + &self, + request: CompletionRequest, + ) -> impl std::future::Future> + Send { + self.inner.complete(request) + } + + fn complete_streaming( + &self, + request: CompletionRequest, + on_token: &(dyn Fn(&str) + Send + Sync), + ) -> impl std::future::Future> { + self.inner.complete_streaming(request, on_token) + } + + fn complete_streaming_calls( + &self, + request: CompletionRequest, + on_token: &(dyn Fn(&str) + Send + Sync), + on_call: &(dyn Fn(usize, &ToolCall) + Send + Sync), + ) -> impl std::future::Future> { + self.inner + .complete_streaming_calls(request, on_token, on_call) + } + + fn models( + &self, + ) -> impl std::future::Future>> + Send { + self.inner.models() + } + + fn reachable(&self) -> impl std::future::Future> + Send { + self.inner.reachable() + } + + fn model_hint(&self) -> Option<&str> { + self.inner.model_hint() + } + + fn name(&self) -> &str { + self.inner.name() + } + + fn prompt_family(&self) -> PromptFamily { + self.inner.prompt_family() + } + + fn accepts_images(&self) -> bool { + self.inner.accepts_images() + } + + fn endpoint(&self) -> Option<&str> { + self.inner.endpoint() + } + + fn endpoints(&self) -> Vec<&str> { + self.inner.endpoints() + } + + fn last_served(&self) -> Option { + self.inner.last_served() + } +} + /// The same maker, making watched providers. /// /// A maker in and a maker out, rather than a provider in and a provider out, diff --git a/src/reviewer.rs b/src/reviewer.rs new file mode 100644 index 0000000..9fd43f9 --- /dev/null +++ b/src/reviewer.rs @@ -0,0 +1,114 @@ +//! The second model, the one that judges the work rather than doing it. +//! +//! **A module of its own, and `tests/provider.rs` is the reason.** That gate says +//! every vendor constructor is written exactly once outside the wizard's +//! handshake and the `/provider` panel, so that the interactive and the headless +//! entry points cannot drift apart — one site builds the provider a turn runs on. +//! A reviewer is not that provider. It is a third construction, like +//! [`crate::verify`]'s credential ping: it answers a question and is thrown away, +//! it never drives a turn, and it deliberately runs a *different* model from the +//! one doing the work. So it is excluded by name, exactly as the handshake is, +//! and held to that boundary by a test rather than trusted. +//! +//! The gate caught this release writing the construction into +//! [`crate::gates`] first, then into [`crate::provider`] beside the real one. Both +//! would have been a second site with its own key resolution. This is the third +//! answer and the honest one. +//! +//! **Nothing here calls a provider.** It constructs `ModelReviewer`, which holds +//! its own provider and its own model, and hands it to io-harness to call when the +//! run loop reaches the gate. That is what keeps `tests/dependencies.rs`'s count of +//! provider calls in this crate at one — the wizard's ping — which is the gate that +//! says this crate has not grown an agent loop. + +use std::sync::Arc; + +use io_harness::{ + Anthropic, Compatible, ModelReviewer, OpenAi, OpenRouter, ProviderSpec, Reviewer, +}; + +use crate::provider::{key_for, Printable}; + +/// A reviewer that asks `model` through `spec`, ready for `with_reviewer`. +/// +/// `Provider` is not dyn-compatible, so each variant is constructed in its own arm +/// and boxed as `Arc` afterwards; `Reviewer` *is* dyn-compatible, +/// which is what makes one return type possible at all. Each provider is wrapped +/// in [`Printable`] to satisfy the `Debug` bound io-harness's own provider types +/// cannot — see that type for the upstream report. +/// +/// The key falls back to the vendor's own environment variable through +/// [`key_for`], the same resolver every other provider in this crate gets: a +/// reviewer configured the way the session's provider is configured should +/// authenticate the way the session does. +/// +/// The error is the provider's own message wherever there is one. A refusal an +/// operator can act on names the endpoint and the key; "could not build a +/// reviewer" names neither. +pub fn build(spec: &ProviderSpec, model: &str) -> Result, String> { + match spec { + ProviderSpec::OpenRouter { api_key, .. } => { + let key = key_for(api_key.clone(), "OPENROUTER_API_KEY")?; + Ok(Arc::new(ModelReviewer::new( + Printable::new(OpenRouter::new(key, model)), + model, + ))) + } + ProviderSpec::Anthropic { api_key, .. } => { + let key = key_for(api_key.clone(), "ANTHROPIC_API_KEY")?; + Ok(Arc::new(ModelReviewer::new( + Printable::new(Anthropic::new(key, model)), + model, + ))) + } + ProviderSpec::OpenAi { api_key, .. } => { + let key = key_for(api_key.clone(), "OPENAI_API_KEY")?; + Ok(Arc::new(ModelReviewer::new( + Printable::new(OpenAi::new(key, model)), + model, + ))) + } + ProviderSpec::Compatible { + base_url, + preset, + api_key, + auth, + .. + } => { + // The model is the reviewer's, never the endpoint's configured one: + // the entire criterion is that a *second* model reads the work, and + // reusing the spec's model is the mistake it exists to prevent. + let secret = api_key.clone().unwrap_or_default(); + let compatible = match (preset, base_url) { + (Some(preset), _) => { + Compatible::preset(preset, secret, model).map_err(|error| error.to_string())? + } + (None, Some(base)) => Compatible::new( + base, + auth.unwrap_or(io_harness::Auth::Bearer), + secret, + model, + ), + (None, None) => { + return Err("this endpoint names neither a preset nor a base URL".into()) + } + }; + Ok(Arc::new(ModelReviewer::new( + Printable::new(compatible), + model, + ))) + } + // `ProviderSpec` is `#[non_exhaustive]`. A provider this release has not + // seen cannot be built into a reviewer, and refusing where the operator is + // still looking at what they typed is the whole point of refusing at all. + // + // **The spec is never formatted into the message.** Every variant holds + // `api_key: Option` verbatim and `ProviderSpec` derives `Debug`, + // so `{other:?}` would put the operator's credential into a refusal that + // this crate then records into the scrollback. That is the same trap + // `Printable`'s hand-written `Debug` exists to avoid two files away. + _ => Err( + "this release does not know how to review with that kind of provider yet".to_string(), + ), + } +} diff --git a/src/settings.rs b/src/settings.rs index b1530b4..8c6f9fa 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -140,6 +140,32 @@ pub struct CliSettings { /// model belongs here and nowhere else. #[serde(default, skip_serializing_if = "Option::is_none")] pub prices: Option, + + /// What "done" means for this repository: `[app.io-cli.gates]`. + /// + /// **The section as written, and deliberately not the criterion that runs.** + /// io-harness's `TaskContract` carries one `Verification`, and a section can + /// say things a `Verification` cannot be built from — no criterion at all, two + /// kinds at once, a rubric with nobody to answer it, a reviewer that is the + /// model doing the work. Deserializing straight into the harness's own type + /// would put that check nowhere, so this field is io-cli's own + /// [`crate::gates::Settings`] and [`crate::gates::Criterion`] is what survives + /// being checked. The difference is where the operator hears about a mistake: + /// io-harness answers the last two with a hard configuration error at run + /// start, which turns editing a file into a session that will not start. + /// + /// It is also **not a preference and not a ceiling**. The three ceilings above + /// bound a turn that would have run anyway; this decides whether a turn that + /// finished is finished, and a review criterion spends a real completion on + /// every gated turn. So an absent section is no gate rather than a default + /// one — nothing is verified for an operator who never said what verifying + /// would mean. + /// + /// A table rather than an array because the contract's verification is one + /// value, not a suite; if the dependency ever grows a list, a list here is an + /// addition rather than a break. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub gates: Option, } /// Where prices come from, and what the last read was. @@ -501,6 +527,15 @@ pub fn render( // operator who runs the wizard and one who adds a provider from // `/provider` later. prices: None, + // Left out for the same reason as the caps, and with the same + // force: a gate decides whether a finished turn counts as + // finished, and a file that arrived with one already in it would + // hold back every turn of somebody who never said what "done" + // means here — a review criterion by spending a second model's + // completion to do it. The wizard asks about none of that, and + // there is no default worth writing: "no gate" is the honest + // answer for an operator who has not answered yet. + gates: None, }, }, }; diff --git a/src/status.rs b/src/status.rs index 82df576..b1ce377 100644 --- a/src/status.rs +++ b/src/status.rs @@ -326,6 +326,47 @@ pub struct Status { /// `false` renders as nothing at all, on the rule this line already holds: /// a session that has not asked to plan is not a session planning zero times. pub planning: bool, + /// Where this turn's verification gate stands, in one word. + /// + /// **A word and never a mark**, for the reason [`WORDS`] is a list of words + /// and [`SPINNER`] is explicitly not a state: the standing has to survive + /// `--plain`, `NO_COLOR` and the ASCII glyph set, and a tick that means + /// *passed* only in green is a fact a monochrome terminal does not carry at + /// all. It is the choice 0.23.0 made for the resume marks, made again. + /// + /// **A `String` this file never interprets, and that is the decision rather + /// than the shortcut it looks like.** The words are io-harness's own — + /// `GateOutcome::as_str` spells the three outcomes a gate can end on, and the + /// driver supplies the one the harness has no outcome for, a criterion that + /// is being evaluated right now. Holding the gate's own type here would make + /// this line the second place that decides what a standing is called; + /// matching on the word to pick a tone would make it the third, and would + /// leave a literal in this file that a rename on the other side walks + /// silently past. So the word arrives composed and is drawn as it arrives — + /// the same rule the containment word is held to, and the reason no backend + /// name is written into this module either. + /// + /// `None` is a session with no criterion configured, which is nearly all of + /// them, and it draws **nothing at all** — not `none`, not an empty label, + /// not a zero. A turn nobody asked to verify has not failed verification. + /// + /// Cleared by [`Status::forget_run`] and by [`Status::start_run`]: a standing + /// is an account of the turn that was gated and of no other turn. + pub gate: Option, + /// Which attempt that standing was taken on, where more than one was made. + /// + /// **Absent below two, which is the absent-rather-than-zero rule sharpened by + /// one.** Every gate that ran at all ran once, so `attempt 1` would be on + /// every gated line in the world and would tell an operator nothing — the + /// argument [`Budgets::in_force`] already makes about io-cli's own step + /// floor, which is a cap on every turn and therefore a ceiling nobody chose. + /// What is worth a field is that the turn is being *retried*, and that starts + /// at the second attempt. + /// + /// `None` where the driver has no number to report, which is every session + /// with no gate and the first attempt of every session with one. The + /// rendering is [`Status::gate_field`]'s, so the threshold is decided once. + pub gate_attempt: Option, /// What this turn has drawn against the tree's shared ceiling, and what is /// left of it: `(drawn, remaining)`. /// @@ -502,6 +543,8 @@ impl Status { spend: None, plan: None, planning: false, + gate: None, + gate_attempt: None, jobs: 0, mcp: (0, 0), lsp: 0, @@ -569,6 +612,15 @@ impl Status { self.containment = None; self.spend = None; self.plan = None; + // **A standing belongs to the turn that was gated, and this is the field + // where keeping it would be worst.** Every other run fact left behind is + // a stale *number*; this one is a verdict on work that is no longer on + // the screen, so a `/resume` onto a conversation nobody ever gated would + // inherit `gate passed` and read as verified. The attempt count goes with + // it rather than separately: a retry count with no standing beside it is + // a number about nothing. + self.gate = None; + self.gate_attempt = None; // A handle belongs to the run that started it. Leaving the count behind // would have `/resume`, `/fork` and a rewind assert that another run's // jobs are still alive — and there would be no event left that could ever @@ -597,6 +649,26 @@ impl Status { // `run_tokens` above is held to: a turn that has not called a provider yet // has not cost zero, it has not cost anything anybody can report. self.cost = None; + // **Here as well as in `forget_run`, because `forget_run` does not run + // between two ordinary turns.** That method is reached by `/resume`, + // `/fork`, `/clear` and a rewind; the common case — the operator types a + // second prompt — reaches only this one, and a standing left behind by it + // would have the line asserting turn one's verdict over the whole of turn + // two, which is the per-turn field this codebase has already shipped + // stale once. + // + // **A gate retry IS affected, and the honest note is worth more than the + // reassuring one.** An earlier draft of this comment said the retries + // happen inside io-harness's own turn under a single `start_run`. They do + // not: io-cli drives the retry as a fresh turn through the driver's own + // queue, so it arrives here and the standing is cleared for the duration + // of the very turn it explains. What stays on screen is the scrollback + // record, which is permanent and says what failed; the footer field + // reappears when the retry is judged in its turn. Keeping the word across + // the chain would mean not clearing it here, and that is the same door the + // stale-per-turn defect above came through. + self.gate = None; + self.gate_attempt = None; } /// Set [`Status::cost`] from what this run has actually called, priced by @@ -884,6 +956,42 @@ impl Status { self.cost.map(crate::cost::money) } + /// Where the gate stands, drawn, or nothing at all. + /// + /// **One method, reached by both renderers**, which is the shape + /// [`Status::budgets_left`], [`Status::queued_left`] and + /// [`Status::cost_field`] already have and for the reason written where they + /// are: this file has shipped a field into one renderer twice — 0.8.0's spend + /// and 0.12.0's planning phase — and both times it was green in a unit test + /// and nowhere on screen, because the binary draws the footer on every + /// terminal seven rows or taller. + /// + /// `Option` rather than a `String` that is sometimes empty, so the absence is + /// decided here once and cannot be forgotten by a caller: both renderers push + /// whatever this returns and neither one asks about [`Status::gate`] itself. + /// + /// **The attempt is part of the same field rather than a second one**, because + /// the two are one fact — `failed` and `failed for the third time` are + /// different situations for an operator, and an `attempt 3` that could be + /// dropped away from the word it qualifies would be a number about nothing. + /// It costs ten cells and appears only from the second attempt, which is to + /// say on the lines where a turn is visibly not converging. + /// + /// **No tone is chosen here and none is chosen by the callers.** Both draw the + /// word in `Tone::Normal`, the tone the planning phase and the background-job + /// count wear, because deciding between them would mean this module comparing + /// the standing against literal outcome words that belong to io-harness — see + /// [`Status::gate`]. Nothing is lost by it: the rule this line has held since + /// its first release is that the word is the state and a colour only ever + /// agrees with it, so a standing spelled out in full needs no second channel. + pub fn gate_field(&self) -> Option { + let standing = self.gate.as_ref()?; + Some(match self.gate_attempt { + Some(attempt) if attempt > 1 => format!("gate {standing} attempt {attempt}"), + _ => format!("gate {standing}"), + }) + } + /// The fields, most important first. pub fn fields(&self, theme: &Theme) -> Vec { // The WORD is the state, and the animation is only beside it. A spinner @@ -996,6 +1104,25 @@ impl Status { if self.planning { fields.push(Field::new("planning".to_string(), Tone::Normal)); } + // **Immediately right of the planning phase and left of every counter, + // which is a decision about a narrow terminal and not a grouping.** The + // rule this row already states is that a standing mode which stops the + // agent writing outranks what the last turn spent; a gate that has not + // passed is the same class of fact from the other end — it is why the + // turn is not finished, and on a retry it is why the agent is doing the + // work a second time. If a narrow terminal can hold one of `gate failed + // attempt 2` and `14.2k tok`, it should hold the one that explains what + // is happening rather than the one that measures it. + // + // Right of `planning` rather than left, because `planning` is a standing + // choice that holds until `/plan off` while this is an account of one + // turn: where both are on the line and only one can survive, the one that + // is still true after the turn ends is the one that stays. + // + // `Normal` rather than `Muted`, for the reason `planning` and `bg N` are: + // it is a fact about what the turn is still doing, not a footnote about + // what it did. + fields.extend(self.gate_field().map(|text| Field::new(text, Tone::Normal))); // Beside the token count rather than anywhere else, because the two are // the same kind of fact — what this run has spent — and they used to sit // together in the `Finished` row this release removed. @@ -1172,6 +1299,20 @@ impl Status { // there is something to count, so a session that has run nothing carries // an almost empty row rather than a row of zeroes. let mut counts: Vec = Vec::new(); + // **First in the group, and that is the same ordering decision + // `Status::fields` makes, arrived at through this row's own mechanism.** + // The narrowing below pops counters off the right until the right-hand + // group fits, so position in this vector *is* priority — and the standing + // that says why the turn is not finished outranks every number beside it, + // exactly as the planning phase outranks them in the group on the right. + // Pushed after `steps` it would have been among the first things a + // crowded row gave up, which is the inversion the release before this one + // spent a live capture finding. + // + // Here as well as on `Status::line` and out of the same method: this is + // the row the binary draws at an ordinary prompt, so a gate added to + // `line` alone would be a gate no operator ever saw. + counts.extend(self.gate_field()); if let Some(steps) = self.steps { counts.push(format!("{steps} step{}", if steps == 1 { "" } else { "s" })); } @@ -1787,6 +1928,15 @@ pub fn committed( // a reader cannot trust is worse than none. The line and the footer redraw, // so they are where a depth that changes belongs — which is also why N3 names // those two renderers and not this one. + // + // **`Status::gate` is not a row here either, and for that same argument.** A + // standing is the verdict on one turn: `gate failed attempt 2` committed into + // a scrollback goes on saying so under the passing turn that follows it, and a + // row that is false a turn later on a page whose whole claim is that it can be + // trusted is worse than no row. What a reader wants from *this* page about a + // gate is the criterion the next turn will be held to, which is configuration + // and belongs beside the budgets — a row this release does not draw rather + // than one it draws wrongly. // Three of whatever the set draws a rule with, at both ends — the same edge // `crate::transcript` gives a committed conversation, and for the same diff --git a/src/triage.rs b/src/triage.rs index b376a29..8b775f0 100644 --- a/src/triage.rs +++ b/src/triage.rs @@ -128,11 +128,14 @@ pub const TRIAGE: &[(&str, Disposition, &str)] = &[ ( "sandbox", Disposition::Line, - "the sandbox line, for the four kinds an ordinary session can see — `create`, `exec`, \ - `cap_hit` and `destroy`. The other three reach no line and each has its own route: \ - `dial` is drawn by `dialed`, which carries the host, the port and the verdict this kind \ - does not, and `gate_phase_failed` and `gate_output` belong to a verification gate no \ - session has until 0.24.0", + "the sandbox line, for six of the seven kinds. `create`, `exec`, `cap_hit` and `destroy` \ + say what isolated the work; `gate_phase_failed` and `gate_output` say that the criterion \ + the operator configured under `[app.io-cli.gates]` ran and did not hold, which from \ + 0.24.0 is a thing a session can see. `dial` is the one kind that reaches no line: it is \ + drawn by `dialed`, which carries the host, the port and the verdict this kind does not. \ + Neither gate line names the phase or quotes the output — `EventKind::Sandbox` carries \ + the kind and the backend alone, and the `detail` holding both stays in the run's own \ + `sandbox_events` rows, which is where a diagnosis reads it", ), ( "mcp", @@ -155,7 +158,10 @@ pub const TRIAGE: &[(&str, Disposition, &str)] = &[ ( "reviewed", Disposition::Line, - "the verdict line and its reasons", + "the verdict line and its reasons, in the reviewer's own words. Emitted only for a review \ + that happened: one that could not run emits nothing here at all and is recorded as \ + `GateOutcome::Errored`, so an absent verdict line is a review that never answered rather \ + than one that said yes", ), ( "routed", diff --git a/tests/commands.rs b/tests/commands.rs index e6dc28e..3a5c6af 100644 --- a/tests/commands.rs +++ b/tests/commands.rs @@ -89,6 +89,13 @@ fn the_commands_are_the_commands() { // this list, which is the rule every alias here follows. "/cost", "/stats", + // 0.24.0 — beside `/stats` because `/stats` is the only other row + // that says the word: that page counts how the gates went, and until + // this release nothing in the product could say what a gate was. It + // is under **configure** and not **inspect**, because opening it is + // one keystroke from writing `[app.io-cli.gates]` and changing what + // every later turn has to prove. + "/gates", ], "the fuzzy palette is still 0.7.0; this list is written out so that adding \ a command is a decision somebody makes rather than a line somebody adds", @@ -261,11 +268,15 @@ fn f12_resume_says_it_answers_a_parked_run_rather_than_merely_reopening_a_sessio 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. + // inventory is the size the other gates in this file assert. The number is + // 30 plus 0.24.0's one addition, and the two halves are named separately on + // purpose: a total that merely went up by one would be satisfied by 0.23.0 + // growing a command and 0.24.0 losing one. assert_eq!( COMMANDS.len(), - 30, - "0.23.0 adds no command; a thirty-first here means one arrived unrecorded", + 31, + "0.23.0 adds no command and 0.24.0 adds exactly one, `/gates`; a \ + thirty-second here means one arrived unrecorded", ); } @@ -1155,3 +1166,83 @@ fn f17_usage_answers_and_is_never_listed() { "a second row for one screen reads as a second screen", ); } + +/// 0.24.0 — `/gates` is a **configure** command, resolves to its own action, and +/// its row is reachable. +/// +/// Every claim here is written against the sabotage that would satisfy the +/// generic gates in this file while leaving the command broken. None of them is +/// a restatement of a gate that already covers it. +/// +/// `no_listed_command_falls_through_to_the_help_listing` proves only that the +/// word is not the unknown-command listing; a `parse` arm that returned +/// `Action::Config(None)` — the surface next door, and the one a careless hand +/// reaches for — would pass it and would open the wrong screen. So the action is +/// asserted by name. +/// +/// `f13_every_command_is_in_exactly_one_group` proves only that it has *a* home. +/// `Inspect` would satisfy it, and `Inspect`'s own documentation says nothing +/// under it changes what a turn does — which a surface that writes +/// `[app.io-cli.gates]` falsifies for every later turn at once. So the group is +/// asserted by name rather than left to the count, exactly as the `/mcp` and +/// `/provider` move is. +/// +/// And the description is asserted to name a cost, because `tests/docs.rs` copies +/// it into the README verbatim and it is the only sentence most operators will +/// ever read about this command. A rubric is a real billed completion on every +/// gated turn; a one-liner that said "what done means" and stopped would hide +/// that behind a word that sounds free. +#[test] +fn gates_is_a_configure_command_that_resolves_to_its_own_action() { + use io_cli::commands::{group_of, palette, palette_pick, Chosen, Group}; + use io_harness::Templates; + + assert!( + COMMANDS.iter().any(|(name, _)| *name == "/gates"), + "a surface nobody is told about is a surface nobody uses", + ); + assert_eq!( + commands::parse("gates", &defaults(), &DARK), + Action::Gates, + "`/gates` must open its own surface, not the one next door", + ); + // The singular is the spelling a hand reaches for when there is exactly one + // criterion, and it must be the same screen rather than an unknown command. + assert_eq!(commands::parse("gate", &defaults(), &DARK), Action::Gates); + // Arguments are tolerated and the first word decides, as everywhere else — + // and this is the command most likely to be typed with one, because the + // thing an operator wants to set is a command line. + assert_eq!( + commands::parse("gates cargo test", &defaults(), &DARK), + Action::Gates, + ); + + assert_eq!( + group_of("/gates"), + Some(Group::Configure), + "`/gates` writes [app.io-cli.gates], and Inspect promises it never writes \ + — a gate an operator did not mean to set spends a whole extra turn \ + against a real model", + ); + + let (_, said) = COMMANDS + .iter() + .find(|(name, _)| *name == "/gates") + .expect("/gates is listed"); + assert!( + said.contains("rubric"), + "the one line most operators read must name the kind that costs money on \ + every gated turn: {said}", + ); + + let rows = palette(&Templates::none(), &[]); + let index = rows + .iter() + .position(|row| row.label == "gates") + .expect("`/gates` has no palette row"); + assert_eq!( + palette_pick(&Templates::none(), &[], index), + Some(Chosen::Command("/gates")), + "`/gates`'s row is advertised and inert", + ); +} diff --git a/tests/docs.rs b/tests/docs.rs index 2223b21..9abf306 100644 --- a/tests/docs.rs +++ b/tests/docs.rs @@ -245,6 +245,22 @@ fn the_readme_documents_every_key_of_the_io_cli_section() { source: Some("the reference catalogue".into()), models: Some(417), }), + // Written out field by field rather than as a `Default::default()`, which + // would serialize to `{}` and satisfy the row check just as well. The + // point is the compile error: `[app.io-cli.gates]` is the one nested + // table whose keys are explained in README prose rather than in a row of + // their own, so a key added to `gates::Settings` has to break something + // to be noticed. This is that something. + gates: Some(io_cli::gates::Settings { + retries: Some(2), + command: Some(vec!["cargo".into(), "test".into()]), + expect_exit: Some(0), + file: Some("CHANGELOG.md".into()), + contains: Some("## [0.24.0]".into()), + rubric: Some("the change is covered by a test that fails without it".into()), + reviewer: Some("a-reviewing-model".into()), + allow_self_review: Some(false), + }), }; let value = serde_json::to_value(&every).expect("[app.io-cli] serializes"); let keys = value.as_object().expect("a table"); @@ -856,6 +872,37 @@ fn the_readme_exit_table_is_the_exit_table() { "the README describes {code} differently from the code: {what}", ); } + + // 0.24.0's `6`, and it is asserted as a NUMBER rather than through a constant + // on purpose. What a script branches on is the integer, and the one mistake + // this release could make that a caller cannot recover from is landing the + // gate code on a number that already meant something else. So the claim is + // that `6` is the code immediately after `UNFINISHED`, which is where the six + // that shipped in 0.5.0 stopped. Move any of those six and this fails; give + // the gate a number one of them already holds and it fails too. + assert_eq!( + io_cli::exec::UNFINISHED + 1, + 6, + "the gate code sits after the six that shipped in 0.5.0; a code that moved \ + one of them cannot be migrated by fixing forward", + ); + assert!( + readme.contains("| `6` |"), + "the README's exit table has no row for the gate that did not pass", + ); + assert!( + readme.contains("does not hold up"), + "the README should say what `6` means in the words the release uses: the \ + agent finished and the work does not hold up", + ); + // And the half a reader most needs: nothing above it moved. A release that + // renumbered `5` would leave every script branching on it silently wrong, + // which is the failure the sentence exists to rule out. + assert!( + readme.contains("No exit code was renumbered"), + "the README should say, where the table is, that no existing code changed \ + meaning", + ); } #[test] diff --git a/tests/events.rs b/tests/events.rs index 8e40f87..a4cf448 100644 --- a/tests/events.rs +++ b/tests/events.rs @@ -414,10 +414,13 @@ fn a_turn_that_ended_well_does_not_end_the_transcript_with_a_warning() { use io_cli::events::outcome_tone; use io_cli::theme::Tone; - // `finished` is what EVERY io-cli turn returns: a steerable turn is built on - // TaskContract::workspace, which carries Verification::None, so there is no - // criterion to pass and `success` is unreachable from this interface. Reading - // it as a warning was a real defect and a live run is what found it. + // Two words for a turn that ended well, and both are reachable from 0.24.0. + // `finished` is a turn with no criterion ending on its own terms, which was + // every io-cli turn until this release: a contract built here carried + // Verification::None and `success` could not be produced from this interface + // at all. A turn whose operator configured a gate — `io_cli::gates` — ends + // `success` when it holds. Reading either as a warning was a real defect and + // a live run is what found it. assert_eq!(outcome_tone("finished"), Tone::Success); assert_eq!(outcome_tone("success"), Tone::Success); @@ -2122,9 +2125,9 @@ fn f8_a_sandbox_draws_its_four_kinds_and_carries_a_backend_only_where_one_exists /// it immediately beside `EventKind::Dialed` for the same connection, so a /// session drawing both would put every dial in the transcript twice — and the /// copy here is the poorer one, carrying the word and neither the host, the port -/// nor the verdict. `gate_phase_failed` and `gate_output` are the other two that -/// draw nothing: they belong to an execution gate, and a session has none until -/// 0.24.0. +/// nor the verdict. It is now the **only** kind of the seven that draws nothing: +/// `gate_phase_failed` and `gate_output` stood beside it here until 0.24.0 gave a +/// session a criterion to fail, and the test below is where they went. /// /// Sabotage: give `"dial"` a sentence of its own in the `Sandbox` arm, under /// which only this fails, on one dial arriving as two rows that disagree about @@ -2132,19 +2135,77 @@ fn f8_a_sandbox_draws_its_four_kinds_and_carries_a_backend_only_where_one_exists #[test] fn f8_the_dial_kind_of_a_sandbox_event_is_left_to_the_dial_itself() { let mut events = Events::new(DARK); - for kind in ["dial", "gate_phase_failed", "gate_output"] { - let lines = events.event( - &event(EventKind::Sandbox { - kind: kind.into(), - backend: None, - }), - Duration::ZERO, + let lines = events.event( + &event(EventKind::Sandbox { + kind: "dial".into(), + backend: None, + }), + Duration::ZERO, + ); + assert!(lines.is_empty(), "dial drew a line: {lines:?}"); + // Drawn nowhere is not the same as undecided. `sandbox` has a disposition, so + // this does not reach the counter that exists for a kind nobody has decided + // about. + assert_eq!( + events.unknown(), + 0, + "a kind the table holds was counted as one this release has never seen", + ); +} + +/// **0.24.0 — a gate that ran and said no reaches the screen.** +/// +/// Until this release both kinds returned an empty `Vec` from the `Sandbox` arm, +/// which was correct while no contract this crate built carried a criterion: an +/// event that cannot arrive needs no sentence. It can arrive now, and a verdict +/// that lives only in the store is a verdict nobody reads. +/// +/// **Neither line can carry the phase or the output**, and that is asserted here +/// rather than merely commented: `EventKind::Sandbox` has `kind` and `backend` +/// and nothing else, while `SandboxEvent::detail` — which holds the failing phase +/// for one and the command's bounded output for the other — never reaches this +/// channel. A line that named a phase would have invented it. +/// +/// Sabotage: put either kind back on the `_ => return Vec::new()` arm. Only this +/// fails, and it fails on the line's absence with `Events::unknown` still at +/// zero — a triaged kind is never counted there — which is a gate that judged the +/// work and told nobody. +#[test] +fn a_gate_that_ran_and_did_not_pass_says_so_on_the_channel_a_session_is_watching() { + let mut events = Events::new(DARK); + + let phase = rendered( + &mut events, + EventKind::Sandbox { + kind: "gate_phase_failed".into(), + backend: None, + }, + ); + // **That it ran is the load-bearing half.** "did not pass" alone is the same + // sentence a criterion that never executed would deserve, and those two need + // opposite responses from whoever reads the row. + assert!(phase.contains("ran and"), "{phase:?}"); + assert!(phase.contains("did not pass"), "{phase:?}"); + + let output = rendered( + &mut events, + EventKind::Sandbox { + kind: "gate_output".into(), + backend: None, + }, + ); + assert!(!output.trim().is_empty(), "gate_output drew nothing"); + assert!(output.contains("printed output"), "{output:?}"); + + // Neither event carries a backend and neither line may name one — the same + // rule `cap_hit` and `destroy` are already held to. + for line in [&phase, &output] { + assert!( + !line.contains("sandbox-exec") && !line.contains("none"), + "a gate event carries no backend and this line invented one: {line:?}", ); - assert!(lines.is_empty(), "{kind} drew a line: {lines:?}"); } - // Drawn nowhere is not the same as undecided. `sandbox` has a disposition, - // so none of these reaches the counter that exists for a kind nobody has - // decided about. + assert_eq!( events.unknown(), 0, @@ -2152,6 +2213,185 @@ fn f8_the_dial_kind_of_a_sandbox_event_is_left_to_the_dial_itself() { ); } +/// **0.24.0 — a criterion that answered and one that never ran do not read +/// alike.** +/// +/// `GateOutcome::Failed` and `GateOutcome::Errored` are the two verdicts io-cli +/// has to keep apart on screen, and only one of them has an event: a gate that +/// ran and said no emits `gate_phase_failed`, while a gate whose program the +/// policy would not run emits `EventKind::Refused` with act `exec` and no gate +/// event at all — nothing executed, so nothing judged anything. io-harness will +/// retry the second and never the first. +/// +/// So the words have to be disjoint, which is what is asserted: the refusal keeps +/// `refused` and never claims a criterion ran, and the gate line never borrows the +/// word the permission boundary owns. A reader who cannot tell them apart fixes +/// the wrong thing — the policy, or the work. +/// +/// Sabotage: draw `gate_phase_failed` through `Tone::Refused`, which is the tone +/// the failing review carried until this release. The line reads `refused: …`, +/// only this test fails, and it fails on the two facts having become one word. +#[test] +fn a_gate_that_could_not_run_does_not_read_like_a_gate_that_ran_and_failed() { + let mut events = Events::new(DARK); + + // Errored: the policy refused the criterion's program. This is the whole of + // what the stream says about it — there is no gate event behind it. + let errored = rendered( + &mut events, + EventKind::Refused { + act: "exec".into(), + target: "cargo".into(), + rule: Some("exec.deny".into()), + layer: Some("workspace".into()), + }, + ); + assert!(errored.contains("refused"), "{errored:?}"); + assert!( + !errored.contains("did not pass"), + "a criterion that never ran did not fail: {errored:?}", + ); + + // Failed: it ran, and it said no. + let failed = rendered( + &mut events, + EventKind::Sandbox { + kind: "gate_phase_failed".into(), + backend: None, + }, + ); + assert!( + !failed.contains("refused"), + "the permission boundary owns that word and nothing stopped this gate: {failed:?}", + ); + // Nor the error path. Nothing broke: a criterion that holds the work to a + // standard and reports that it was not met did its job. + assert!(!failed.contains("error"), "{failed:?}"); + assert_ne!(errored, failed); +} + +/// **0.24.0 — a review's reasons reach the scrollback, in the reviewer's words.** +/// +/// The reasons are the operator's only account of why the work was judged +/// insufficient. The store keeps the same sentences joined by semicolons, so a +/// session that dropped them would be sending somebody to a database to read a +/// paragraph the run had already said out loud. +/// +/// **The failing verdict is a warning and not a refusal**, which is the half that +/// changed this release. `Tone::Refused` carries the literal word `refused`, and +/// everywhere else in this transcript that word means the permission boundary +/// stopped an act. A reviewer that read the work and judged it stopped nothing. +/// +/// Sabotage: restore `Tone::Refused` on the failing arm. Only this fails, on a +/// judged verdict and a blocked act arriving under one word. +#[test] +fn a_review_carries_its_reasons_and_a_failing_verdict_is_not_a_refusal() { + let mut events = Events::new(DARK); + + let failed = rows(events.event( + &event(EventKind::Reviewed { + passed: false, + reasons: vec![ + "the new branch has no test".into(), + "README still documents the old flag".into(), + ], + }), + Duration::ZERO, + )); + let said = failed.join("\n"); + assert!(said.contains("the review ran and did not pass"), "{said:?}"); + // Every reason, whole. Asserted per row rather than over the join, so a + // second reason silently swallowed by the first cannot pass. + assert!( + failed + .iter() + .any(|row| row.contains("the new branch has no test")), + "{failed:?}", + ); + assert!( + failed + .iter() + .any(|row| row.contains("README still documents the old flag")), + "{failed:?}", + ); + assert!( + !said.contains("refused"), + "a review that read the work and judged it stopped nothing: {said:?}", + ); + + let passed = flatten(events.event( + &event(EventKind::Reviewed { + passed: true, + reasons: Vec::new(), + }), + Duration::ZERO, + )); + assert!(passed.contains("the review passed"), "{passed:?}"); + assert!( + !passed.contains("did not"), + "the two verdicts must not be one sentence apart from a negation this reader \ + can miss: {passed:?}", + ); +} + +/// **F5 for the lines this release added.** Nothing outside ASCII reaches the +/// terminal under the ASCII set, and the meaning survives the substitution. +/// +/// Swept by `char::is_ascii` over the whole rendered output rather than by +/// looking for the marks this release happens to know about — `tests/glyphs.rs` +/// says why: a `contains` over the code points somebody remembered is green the +/// day a twelfth one is typed into a new line. The review's reasons are the rows +/// that carry a glyph here, through `Glyphs::bullet`. +/// +/// Plain mode is the other axis and is asserted beside it, because a plain +/// session is the one that has no status line to fall back on: if a gate verdict +/// were dropped there it would reach nobody at all. +#[test] +fn the_gate_and_review_lines_survive_the_ascii_set_and_plain_mode() { + use io_cli::glyphs::ASCII; + + for plain in [false, true] { + let mut events = Events::new(DARK.with_glyphs(ASCII)); + events.set_plain(plain); + + let mut said = String::new(); + for kind in [ + EventKind::Sandbox { + kind: "gate_phase_failed".into(), + backend: None, + }, + EventKind::Sandbox { + kind: "gate_output".into(), + backend: None, + }, + EventKind::Reviewed { + passed: false, + reasons: vec!["the diff does not build".into()], + }, + ] { + said.push_str(&rendered(&mut events, kind)); + said.push('\n'); + } + + assert!( + said.is_ascii(), + "a character the ASCII set cannot draw reached the terminal in \ + plain={plain}: {said:?}", + ); + // And the meaning survived it. A set that mapped every mark to a space + // would pass the sweep above and destroy the product. + assert!( + said.contains("ran and did not pass"), + "plain={plain}: {said:?}" + ); + assert!(said.contains("printed output"), "plain={plain}: {said:?}"); + assert!( + said.contains("the diff does not build"), + "plain={plain}: {said:?}", + ); + } +} + /// 0.14.0 F9 — a stalled agent is on screen before anybody interrupts it. /// /// `EventKind::Stalled` is a unit variant carrying nothing at all, so the line is diff --git a/tests/exec.rs b/tests/exec.rs index 69975c3..f5e4f84 100644 --- a/tests/exec.rs +++ b/tests/exec.rs @@ -107,9 +107,15 @@ async fn f1_a_clean_run_finishes_rather_than_succeeding() { .expect("the turn runs"); // Not an incidental assertion. `TaskContract::workspace` sets - // `Verification::None`, so `Success` is unreachable from this subcommand and - // `Finished` is what every good run returns. A table that mapped only + // `Verification::None`, and this fixture configures no criterion, so + // `Finished` is what a good run returns here. A table that mapped only // `Success` to zero would fail here — and only here, on the ordinary case. + // + // **0.24.0 makes `Success` reachable and this assertion narrower rather than + // false.** An operator who writes `[app.io-cli.gates]` gets a real + // criterion on the contract and a passing gate returns `Success`. What stays + // true is that a run with NO criterion finishes rather than succeeding, which + // is the case this fixture builds and the case every operator has today. assert!( matches!(result.outcome, RunOutcome::Finished { .. }), "a contract with no verification criterion finishes, it does not succeed: {:?}", @@ -178,16 +184,89 @@ fn f2_every_outcome_maps_to_its_documented_code() { } // The six codes are distinct and cover 0..=5, so a script can branch on them. + // + // **`6` is deliberately absent from this list and that is the assertion.** + // 0.24.0 added `UNVERIFIED`, and it is not reachable from an outcome — + // io-harness has no `RunOutcome` variant for a criterion that answered no, so + // the verdict is read from the store and applied by `exec::verified_code`. + // This test is about `exec::code`, whose input is an outcome and nothing + // else; widening it to admit 6 would make it a weaker statement about a + // function that cannot produce one. `verified_code` has its own test below. let mut codes: Vec = cases.iter().map(|(_, code)| *code).collect(); codes.sort_unstable(); codes.dedup(); assert_eq!( codes, vec![0, 2, 3, 4, 5], - "0, 2, 3, 4 and 5 are reachable from an outcome; 1 is reserved for never reaching one" + "0, 2, 3, 4 and 5 are reachable from an outcome; 1 is reserved for never reaching one, \ + and 6 is decided after the run by `verified_code`" ); } +/// A gate that ran and said no is exit `6`, whatever the run itself reported. +/// +/// The two outcomes below are the ones a gate-failed run actually ends in, and +/// they map to different codes without a gate — `StepCapReached` to `3` when the +/// agent spends its budget failing the criterion, `Finished` to `0` when it stops +/// early believing itself done. Both become `6`, which is the whole point: the +/// operator asked a question and got the same answer either way. +/// +/// Sabotage: treat `Errored` as `UNVERIFIED` too — under which the `Errored` arm +/// fails, and it fails by reporting a judgement that was never made. +#[test] +fn f8_a_failing_gate_is_exit_six_and_a_gate_that_never_answered_is_not() { + use io_cli::gates::Standing; + use io_harness::GateOutcome; + + let standing = |outcome| Standing { + phase: "command".into(), + outcome, + attempt: 1, + }; + + for outcome in [ + RunOutcome::StepCapReached { steps: 12 }, + RunOutcome::Finished { steps: 3 }, + ] { + assert_eq!( + exec::verified_code(&outcome, Some(&standing(GateOutcome::Failed))), + exec::UNVERIFIED, + "{outcome:?} with a failed gate is the work not holding up", + ); + + // A criterion that could not run has judged nothing. The run keeps the + // code its own outcome earned. + assert_eq!( + exec::verified_code(&outcome, Some(&standing(GateOutcome::Errored))), + exec::code(&outcome), + "{outcome:?} with an errored gate keeps its own code", + ); + + // A passing gate is not a reason to change anything either: `Success` + // already maps to `OK` through the table. + assert_eq!( + exec::verified_code(&outcome, Some(&standing(GateOutcome::Passed))), + exec::code(&outcome), + "{outcome:?} with a passing gate keeps its own code", + ); + + // And an operator who configured no criterion sees exactly what they saw + // in 0.23.0. + assert_eq!( + exec::verified_code(&outcome, None), + exec::code(&outcome), + "{outcome:?} with no gate configured is unchanged", + ); + } + + // `6` is outside the range `code` can produce, so a script branching on the + // old six cannot mistake it for one of them. Asserted against `UNFINISHED` + // rather than as a bare literal, so that moving any existing code reddens + // here too. + assert_eq!(exec::UNVERIFIED, exec::UNFINISHED + 1); + assert_eq!(exec::UNVERIFIED, 6); +} + #[test] fn f2_a_paused_run_names_what_was_parked_and_nothing_else_does() { // Exit 4 is reachable, and the first end-to-end run of the release binary is diff --git a/tests/gates.rs b/tests/gates.rs new file mode 100644 index 0000000..3b5f08e --- /dev/null +++ b/tests/gates.rs @@ -0,0 +1,813 @@ +//! F2, F3, F4 and F5 — what `[app.io-cli.gates]` resolves to, what it refuses, +//! what the repository proposes for itself, and what a file criterion actually +//! asserts. +//! +//! Every assertion here is written against the way the implementation would be +//! wrong rather than the way it is right, because the shapes this module gets +//! wrong all *pass* a naive test: +//! +//! * an existence criterion built on a reader that answers `Ok("")` for a file +//! that is not there passes on a file nobody wrote, and a test that only checks +//! the happy path — the file exists, the gate says yes — never notices; +//! * a command criterion that hardcodes exit zero satisfies every test written +//! with the default; +//! * a toolchain proposal that falls back to a Rust command is correct in this +//! repository and wrong in every other one, so it has to be asserted against a +//! directory that is not this repository; +//! * a refusal that is silently `Ok(None)` looks exactly like a section nobody +//! wrote. +//! +//! So the file criterion is asserted against a *missing* file and against an +//! *empty* one separately, the exit status is asserted at a value the default is +//! not, and the toolchain proposal is asserted against four temporary directories +//! of which one holds nothing at all. + +use std::path::{Path, PathBuf}; + +use io_cli::gates::{self, Criterion, Refusal, Settings}; +use io_harness::{Config, GateAttempt, GateOutcome, SandboxEvent, Verification}; + +/// The model the session would run with. Only the self-review refusal compares +/// against it, and only by equality. +const TURN_MODEL: &str = "vendor/worker-model"; + +/// A section with one key set, everything else absent. +fn section() -> Settings { + Settings::default() +} + +/// One recorded gate evaluation. `at` is a stored string this crate never parses +/// and never compares, so a fixture can leave it empty. +fn attempt(step: u32, phase: &str, outcome: GateOutcome) -> GateAttempt { + GateAttempt { + id: 1, + step, + phase: phase.into(), + outcome, + detail: String::new(), + at: String::new(), + } +} + +/// An empty configuration: no `[toolchain.*]` overrides, so a proposal is exactly +/// what the repository said about itself. +fn plain() -> Config { + Config::from_toml("").expect("an empty configuration parses") +} + +// --------------------------------------------------------------------------- +// Resolving the section +// --------------------------------------------------------------------------- + +/// A section nobody wrote is not a refusal, and a section somebody started is. +/// +/// The pair is the test. Answering both with `Ok(None)` would be the natural +/// implementation — "no kind is named, so there is no gate" — and it turns a +/// half-written gate into a silently ungated session. +#[test] +fn f2_an_absent_section_resolves_to_nothing_and_a_half_written_one_is_refused() { + assert_eq!( + section().criterion(TURN_MODEL), + Ok(None), + "a section with every key unset asked for no gate" + ); + + for started in [ + Settings { + retries: Some(2), + ..section() + }, + Settings { + contains: Some("ok".into()), + ..section() + }, + Settings { + reviewer: Some("vendor/judge".into()), + ..section() + }, + Settings { + expect_exit: Some(1), + ..section() + }, + Settings { + allow_self_review: Some(true), + ..section() + }, + ] { + assert_eq!( + started.criterion(TURN_MODEL), + Err(Refusal::Empty), + "{started:?} names no kind but is plainly meant to be a gate, and \ + resolving it to nothing would gate the session on nothing" + ); + } +} + +/// Two kinds is a refusal and never a precedence rule. +/// +/// Asserted for every pair and for all three, because a precedence rule written +/// as an `if` chain refuses none of them and quietly gates the turn on whichever +/// branch happened to come first. +#[test] +fn f2_more_than_one_kind_is_refused_rather_than_ordered() { + let command = Some(vec!["cargo".into(), "test".into()]); + let file = Some(PathBuf::from("REPORT.md")); + let rubric = Some("is it done".into()); + + for both in [ + Settings { + command: command.clone(), + file: file.clone(), + ..section() + }, + Settings { + command: command.clone(), + rubric: rubric.clone(), + reviewer: Some("vendor/judge".into()), + ..section() + }, + Settings { + file: file.clone(), + rubric: rubric.clone(), + reviewer: Some("vendor/judge".into()), + ..section() + }, + Settings { + command, + file, + rubric, + reviewer: Some("vendor/judge".into()), + ..section() + }, + ] { + assert_eq!( + both.criterion(TURN_MODEL), + Err(Refusal::Ambiguous), + "{both:?} names more than one criterion and there is no right answer" + ); + } +} + +/// A rubric with no reviewer is refused here, not at run start. +/// +/// io-harness answers this with `Error::Config` before the first billed call, so +/// an implementation that resolves it to a `Review` criterion has moved the +/// failure from a keystroke the operator is looking at to every turn afterwards. +#[test] +fn f5_a_rubric_with_no_reviewer_is_refused() { + let written = Settings { + rubric: Some("the change is covered by a test".into()), + ..section() + }; + assert_eq!(written.criterion(TURN_MODEL), Err(Refusal::ReviewerMissing)); + + // And the refusal survives `allow_self_review`, which is about *which* model + // judges and cannot supply one. + let permissive = Settings { + rubric: Some("the change is covered by a test".into()), + allow_self_review: Some(true), + ..section() + }; + assert_eq!( + permissive.criterion(TURN_MODEL), + Err(Refusal::ReviewerMissing), + "allowing self-review does not name a reviewer" + ); +} + +/// A reviewer that is the model doing the work is refused unless it was asked for. +/// +/// The three cases together are the test: refused by default, allowed when the +/// operator said so, and untouched when the reviewer is somebody else. An +/// implementation that never fires the refusal passes the second and third alone. +#[test] +fn f5_a_reviewer_equal_to_the_turn_model_is_refused_unless_allowed() { + let judging_itself = Settings { + rubric: Some("the change is covered by a test".into()), + reviewer: Some(TURN_MODEL.into()), + ..section() + }; + assert_eq!( + judging_itself.criterion(TURN_MODEL), + Err(Refusal::SelfReview { + model: TURN_MODEL.into(), + }), + "a model marking its own paper is a decision, not a default" + ); + + let asked_for = Settings { + allow_self_review: Some(true), + ..judging_itself.clone() + }; + assert_eq!( + asked_for.criterion(TURN_MODEL), + Ok(Some(Criterion::Review { + rubric: "the change is covered by a test".into(), + reviewer: TURN_MODEL.into(), + allow_self_review: true, + })), + "the operator said so, and the flag has to reach the criterion" + ); + + let second_model = Settings { + reviewer: Some("vendor/judge-model".into()), + ..judging_itself.clone() + }; + assert_eq!( + second_model.criterion(TURN_MODEL), + Ok(Some(Criterion::Review { + rubric: "the change is covered by a test".into(), + reviewer: "vendor/judge-model".into(), + allow_self_review: false, + })) + ); + + // A caller that does not yet know which model will run cannot be told there + // is a clash, and must not be told there is none either — it is told nothing, + // which is what an empty model means. + // Asserted on the criterion it resolves to, not on `is_ok()`. `Ok(None)` — a + // silently ungated run, the exact outcome every refusal here exists to + // prevent — satisfies `is_ok()` just as happily as the right answer does. + assert!( + matches!( + judging_itself.criterion(""), + Ok(Some(Criterion::Review { .. })) + ), + "with no turn model there is nothing to compare the reviewer against, so \ + the rubric stands: {:?}", + judging_itself.criterion(""), + ); +} + +// --------------------------------------------------------------------------- +// Mapping a criterion onto the dependency's type +// --------------------------------------------------------------------------- + +/// The exit status the operator wrote is the exit status the gate carries. +/// +/// Asserted at one rather than at zero, because zero is the default and a mapping +/// that ignores the key entirely is indistinguishable from a correct one at that +/// value. The default is asserted separately, in the same test, so neither half +/// can pass on its own. +#[test] +fn f2_a_command_criterion_carries_the_exit_status_it_was_given() { + let lint_found_nothing = Settings { + command: Some(vec!["npm".into(), "run".into(), "lint".into()]), + expect_exit: Some(1), + ..section() + }; + let criterion = lint_found_nothing + .criterion(TURN_MODEL) + .expect("a command criterion resolves") + .expect("and it is a criterion"); + assert_eq!( + criterion, + Criterion::Command { + argv: vec!["npm".into(), "run".into(), "lint".into()], + expect_exit: 1, + } + ); + + let Verification::Command { argv, expect_exit } = criterion.verification() else { + panic!("a command criterion maps to a command verification"); + }; + assert_eq!(argv, ["npm", "run", "lint"]); + assert_eq!( + expect_exit, 1, + "a hardcoded zero would pass every other test" + ); + + let defaulted = Settings { + command: Some(vec!["make".into(), "check".into()]), + ..section() + }; + assert_eq!( + defaulted + .criterion(TURN_MODEL) + .expect("it resolves") + .expect("to a criterion"), + Criterion::Command { + argv: vec!["make".into(), "check".into()], + expect_exit: 0, + }, + "an unwritten exit status is zero" + ); + + // A command gate is io-harness's to run, whatever else this crate can check. + assert!(!criterion.checked_here()); + assert_eq!(criterion.satisfied_in(Path::new("/nonexistent")), None); +} + +/// A file criterion with a needle is the dependency's contains criterion. +#[test] +fn f4_a_file_with_a_needle_maps_to_the_contains_criterion() { + let written = Settings { + file: Some(PathBuf::from("docs/REPORT.md")), + contains: Some("## Findings".into()), + ..section() + }; + let criterion = written + .criterion(TURN_MODEL) + .expect("it resolves") + .expect("to a criterion"); + + let Verification::WorkspaceFileContains { file, needle } = criterion.verification() else { + panic!("a file criterion with a needle maps to the contains verification"); + }; + assert_eq!(file, PathBuf::from("docs/REPORT.md")); + assert_eq!(needle, "## Findings"); + + assert!( + !criterion.checked_here(), + "the dependency can express this one, so it runs it" + ); + assert_eq!(criterion.satisfied_in(Path::new("/nonexistent")), None); +} + +/// A file criterion with no needle is never a contains criterion with no text. +/// +/// This is the whole of F4's sabotage. The dependency reads its file with a +/// reader that answers the empty string for a file that is not there, and every +/// string contains the empty needle — so the obvious mapping is a gate that can +/// never fail, on a file that need never exist. +#[test] +fn f4_existence_is_never_an_empty_needle() { + let written = Settings { + file: Some(PathBuf::from("REPORT.md")), + ..section() + }; + let criterion = written + .criterion(TURN_MODEL) + .expect("it resolves") + .expect("to a criterion"); + assert_eq!( + criterion, + Criterion::File { + file: PathBuf::from("REPORT.md"), + contains: None, + } + ); + + // Asserted as an equality rather than by ruling out one variant. Naming only + // `WorkspaceFileContains` leaves `FileContains("")` and `DocumentContains` + // with an empty needle passing — the same empty-needle bug wearing a different + // variant, which is precisely what this assertion is for. + assert!( + matches!(criterion.verification(), Verification::None), + "bare existence has no honest io-harness criterion, so nothing is asked of \ + the harness and io-cli answers it itself; got {:?}", + criterion.verification(), + ); + assert!( + criterion.checked_here(), + "the dependency cannot express existence, so this crate must own it, and \ + a caller has to be able to ask" + ); +} + +/// Existence tells a missing file from an empty one. +/// +/// The empty file is the assertion that matters. Every reader in the dependency +/// that answers `Ok` for a missing file answers `Ok` for an empty one too, so a +/// test that only checks a file with text in it passes on both readers and +/// distinguishes nothing. +#[test] +fn f4_existence_tells_a_missing_file_from_an_empty_one() { + let root = tempfile::tempdir().expect("a temporary directory"); + let criterion = Criterion::File { + file: PathBuf::from("REPORT.md"), + contains: None, + }; + + assert_eq!( + criterion.satisfied_in(root.path()), + Some(false), + "nobody wrote the file, so the gate has to say no" + ); + + std::fs::write(root.path().join("REPORT.md"), "").expect("an empty file is written"); + assert_eq!( + criterion.satisfied_in(root.path()), + Some(true), + "the file exists; a reader that could not tell this from the case above \ + would have passed the assertion before it too" + ); + + // A directory at the path is not a file that was written. + let directory = Criterion::File { + file: PathBuf::from("docs"), + contains: None, + }; + std::fs::create_dir(root.path().join("docs")).expect("a directory is created"); + assert_eq!( + directory.satisfied_in(root.path()), + Some(false), + "an operator who named a file did not mean a directory" + ); + + // And the criterion cannot be used to read outside the workspace. + let escaping = Criterion::File { + file: PathBuf::from("../../etc/hosts"), + contains: None, + }; + assert_eq!( + escaping.satisfied_in(root.path()), + Some(false), + "a path that climbs out of the workspace is refused, not followed" + ); +} + +/// A review criterion carries the rubric and the flag, and not the reviewer. +/// +/// The reviewer is attached to the contract separately, through +/// `TaskContract::with_reviewer`, because the dependency's `Review` variant has +/// no field for it. A mapping that dropped `allow_self_review` would re-refuse at +/// run start the configuration this module just accepted. +#[test] +fn f5_a_review_criterion_carries_the_rubric_and_the_flag() { + let criterion = Criterion::Review { + rubric: "no public item lost its documentation".into(), + reviewer: "vendor/judge-model".into(), + allow_self_review: true, + }; + let Verification::Review { + rubric, + allow_self_review, + } = criterion.verification() + else { + panic!("a review criterion maps to a review verification"); + }; + assert_eq!(rubric, "no public item lost its documentation"); + assert!( + allow_self_review, + "dropping the flag would make the run refuse what this module accepted" + ); + assert!(!criterion.checked_here()); +} + +/// Every criterion describes itself, including the one with no verification. +#[test] +fn f7_every_criterion_says_what_it_asks() { + let existence = Criterion::File { + file: PathBuf::from("REPORT.md"), + contains: None, + }; + let described = existence.describe(); + assert!( + described.contains("REPORT.md") && described.contains("exist"), + "an existence criterion has no verification to delegate to and must still \ + be able to tell a retried turn what it wants: {described:?}" + ); + assert!( + !described.contains("no automated check"), + "delegating this one to the dependency would describe it as ungated" + ); + + let command = Criterion::Command { + argv: vec!["make".into(), "check".into()], + expect_exit: 0, + }; + assert!( + command.describe().contains("make check"), + "the words a retried turn reads are the words it was judged by" + ); +} + +// --------------------------------------------------------------------------- +// Retries +// --------------------------------------------------------------------------- + +/// The retry budget defaults to one and zero is honoured as zero. +/// +/// Zero is the half that needs asserting: an implementation reaching for +/// `unwrap_or` on a count and then treating a falsy value as unset turns "do not +/// retry" into "retry once", which spends a turn against a real model the +/// operator explicitly declined. +#[test] +fn f7_the_retry_budget_defaults_to_one_and_zero_means_zero() { + assert_eq!(section().retries(), 1); + assert_eq!( + Settings { + retries: Some(0), + ..section() + } + .retries(), + 0, + "an operator who wrote zero asked for no retry at all" + ); + assert_eq!( + Settings { + retries: Some(3), + ..section() + } + .retries(), + 3 + ); + + let failed = [attempt(1, "command", GateOutcome::Failed)]; + assert!( + !gates::may_retry(&failed, 0), + "zero retries must not buy one" + ); + assert!( + gates::may_retry(&failed, 1), + "one retry buys the second turn" + ); + + let twice = [ + attempt(1, "command", GateOutcome::Failed), + attempt(2, "command", GateOutcome::Failed), + ]; + assert!( + !gates::may_retry(&twice, 1), + "one retry buys one further turn, not one further failure" + ); + + assert!( + !gates::may_retry(&[attempt(1, "command", GateOutcome::Passed)], 3), + "a gate that passed is not retried however much budget is left" + ); + assert!( + gates::may_retry(&[attempt(1, "review", GateOutcome::Errored)], 1), + "a gate that could not be evaluated has judged nothing" + ); + assert!( + !gates::may_retry(&[], 3), + "nothing has failed, so there is nothing to retry" + ); +} + +/// The standing is the last attempt, counted from one. +#[test] +fn f6_the_standing_is_the_last_attempt_and_its_number() { + assert_eq!(gates::standing(&[]), None, "an ungated run has no standing"); + + let attempts = [ + attempt(1, "command", GateOutcome::Failed), + attempt(2, "command", GateOutcome::Passed), + ]; + let standing = gates::standing(&attempts).expect("two attempts have a standing"); + assert_eq!( + standing.outcome, + GateOutcome::Passed, + "the last one, not the first" + ); + assert_eq!( + standing.attempt, 2, + "counted from one, so two attempts is two" + ); + assert_eq!(standing.phase, "command"); +} + +/// A failing gate's output is read per step, so a retry carries its own failure. +#[test] +fn f7_the_recorded_output_is_the_one_for_the_step_asked_about() { + let events = [ + SandboxEvent::gate_output(7, 1, "first failure"), + SandboxEvent::exec(7, 1, "macos-sandbox-exec", "cargo test"), + SandboxEvent::gate_output(7, 2, "second failure"), + SandboxEvent::gate_output(7, 2, "and its second phase"), + ]; + + assert_eq!( + gates::output(&events, 2).as_deref(), + Some("second failure\nand its second phase"), + "the retry is told what is wrong now, and no phase of it is dropped" + ); + assert_eq!(gates::output(&events, 1).as_deref(), Some("first failure")); + assert_eq!( + gates::output(&events, 3), + None, + "a step that printed nothing is nothing, not an empty string" + ); + assert_eq!(gates::output(&[], 1), None); +} + +// --------------------------------------------------------------------------- +// What the repository proposes for itself +// --------------------------------------------------------------------------- + +/// The proposal is the repository's own, for four different repositories. +/// +/// The bare directory is the assertion the other three cannot make: a fallback to +/// a Rust command is correct in this repository and in the first case below, and +/// wrong everywhere else, so only a directory that says nothing about itself +/// catches it. +#[test] +fn f3_the_test_command_is_proposed_by_the_repository_and_never_defaulted() { + let rust = tempfile::tempdir().expect("a temporary directory"); + std::fs::write(rust.path().join("Cargo.toml"), "[package]\nname = \"x\"\n") + .expect("the marker is written"); + assert_eq!( + gates::proposed_command(rust.path(), &plain()), + Some(vec!["cargo".into(), "test".into()]) + ); + + let node = tempfile::tempdir().expect("a temporary directory"); + std::fs::write(node.path().join("package.json"), "{}").expect("the marker is written"); + std::fs::write(node.path().join("pnpm-lock.yaml"), "").expect("the lockfile is written"); + assert_eq!( + gates::proposed_command(node.path(), &plain()), + Some(vec!["pnpm".into(), "test".into()]), + "the lockfile beside the marker chooses the manager, and this crate holds \ + no opinion about which" + ); + + let python = tempfile::tempdir().expect("a temporary directory"); + std::fs::write( + python.path().join("pyproject.toml"), + "[project]\nname = \"x\"\n", + ) + .expect("the marker is written"); + let proposed = gates::proposed_command(python.path(), &plain()) + .expect("a Python project proposes a test command"); + assert!( + proposed.first().is_some_and(|program| program != "cargo"), + "a Rust command was offered in a repository with no Rust in it: {proposed:?}" + ); + assert_eq!(proposed, ["python", "-m", "pytest"]); + + let bare = tempfile::tempdir().expect("a temporary directory"); + assert_eq!( + gates::proposed_command(bare.path(), &plain()), + None, + "a repository that said nothing about itself gets no proposal; a fallback \ + here is a wrong command an operator accepts with one keystroke" + ); +} + +/// The operator's own override wins over what was detected. +#[test] +fn f3_an_operator_override_replaces_the_detected_command() { + let rust = tempfile::tempdir().expect("a temporary directory"); + std::fs::write(rust.path().join("Cargo.toml"), "[package]\nname = \"x\"\n") + .expect("the marker is written"); + + let tuned = Config::from_toml( + "\ +[toolchain.cargo] +test = [\"cargo\", \"nextest\", \"run\"] +", + ) + .expect("the fixture parses"); + + assert_eq!( + gates::proposed_command(rust.path(), &tuned), + Some(vec!["cargo".into(), "nextest".into(), "run".into()]), + "detection is layered under the file, not instead of it" + ); + + let silenced = Config::from_toml( + "\ +[toolchain.cargo] +test = [] +", + ) + .expect("the fixture parses"); + assert_eq!( + gates::proposed_command(rust.path(), &silenced), + None, + "an empty override is an operator saying there is no test step here, and \ + an empty argv is a criterion with no program in it" + ); +} + +/// The module holds no list of marker files and no list of test commands. +/// +/// The sweep is over the *comment-stripped* source: this crate argues its design +/// in prose, and prose naming what it deliberately does not hold is a module +/// explaining itself rather than a module hiding a list. Code is what is checked. +/// +/// A marker name appearing here is the copy of the dependency's table that drifts +/// — and the way it drifts is by proposing an ecosystem's command in a repository +/// that stopped being that ecosystem two commits ago. +#[test] +fn f3_the_module_names_no_marker_file_and_no_test_command() { + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("src/gates.rs"); + let source = std::fs::read_to_string(&path).expect("the module is readable"); + let code = strip_comments(&source); + + for marker in [ + "Cargo.toml", + "package.json", + "pyproject.toml", + "requirements.txt", + "go.mod", + "deno.json", + "Gemfile", + "pom.xml", + "build.gradle", + "mix.exs", + "composer.json", + "Package.swift", + "CMakeLists.txt", + "Makefile", + ".csproj", + ] { + assert!( + !code.contains(marker), + "src/gates.rs names the marker file {marker:?} in code; detection \ + belongs to io_harness::toolchain, and a second copy of that table is \ + a table that drifts" + ); + } + + for command in [ + "cargo\", \"test", + "npm\", \"test", + "pnpm\", \"test", + "go\", \"test", + "pytest", + "dotnet", + ] { + assert!( + !code.contains(command), + "src/gates.rs spells the test command {command:?} itself, which is the \ + fallback F3 forbids" + ); + } + + // The stripper has to actually strip, or the assertions above are vacuous for + // any module that mentions a marker in prose. + assert!( + strip_comments("let x = 1; // Cargo.toml\n").trim() == "let x = 1;", + "the stripper removes a trailing line comment" + ); + assert!( + !strip_comments("//! about Cargo.toml\ncode\n").contains("Cargo.toml"), + "the stripper removes a module doc line" + ); +} + +/// Everything from a `//` to the end of the line, and every block comment. +/// +/// Deliberately naive about a `//` inside a string literal: the only way that +/// errs is by stripping more than it should, which can weaken this sweep and can +/// never make it fail on a module that is correct. A stripper that tried to parse +/// Rust strings here would be a parser nobody maintains guarding a one-line rule. +fn strip_comments(source: &str) -> String { + let mut out = String::with_capacity(source.len()); + let mut rest = source; + // Block comments first, so a `//` inside one cannot terminate a line early. + while let Some(start) = rest.find("/*") { + out.push_str(&rest[..start]); + match rest[start..].find("*/") { + Some(end) => rest = &rest[start + end + 2..], + None => rest = "", + } + } + out.push_str(rest); + + out.lines() + .map(|line| match line.find("//") { + Some(at) => &line[..at], + None => line, + }) + .collect::>() + .join("\n") +} + +// --------------------------------------------------------------------------- +// What the operator is told +// --------------------------------------------------------------------------- + +/// Every refusal has a sentence, it is ASCII, and it names the key to change. +/// +/// ASCII because these render through the plain renderer, under `NO_COLOR` and +/// through the ASCII glyph set — a refusal that arrives as a replacement +/// character is a refusal nobody reads. Naming the key because the operator is +/// looking at the file when they get it. +#[test] +fn f5_every_refusal_has_an_ascii_sentence_naming_the_key_to_change() { + let sentences = [ + (Refusal::Empty, vec!["command", "file", "rubric"]), + (Refusal::Ambiguous, vec!["command", "file", "rubric"]), + (Refusal::ReviewerMissing, vec!["reviewer", "rubric"]), + ( + Refusal::SelfReview { + model: TURN_MODEL.into(), + }, + vec![TURN_MODEL, "allow_self_review"], + ), + ]; + + for (refusal, keys) in sentences { + let text = refusal.to_string(); + assert!( + text.is_ascii(), + "{refusal:?} renders as {text:?}, which is not ASCII and will not \ + survive the ASCII glyph set" + ); + assert!( + !text.is_empty() && text.len() > 20, + "{refusal:?} renders as {text:?}, which tells an operator nothing" + ); + for key in keys { + assert!( + text.contains(key), + "{refusal:?} renders as {text:?} without naming {key:?}, and the \ + operator is looking at the file" + ); + } + } +} diff --git a/tests/live.rs b/tests/live.rs index 9182067..4762561 100644 --- a/tests/live.rs +++ b/tests/live.rs @@ -2631,3 +2631,280 @@ async fn live_f2_a_parked_question_is_answered_and_the_run_carries_on() { "the resumed run drove no step, so the answer reached nothing", ); } + +/// The `[app.io-cli.gates]` section, built in memory rather than through a file. +/// +/// `Config::from_toml` is public and takes the whole document, so the live arms +/// need no `IO_CONFIG` and therefore no environment lock — which matters here +/// because these tests run in one process and an environment variable is +/// process-wide. +fn gated(section: &str) -> Config { + Config::from_toml(section).expect("the gates section parses") +} + +/// **0.24.0 F2 — a command criterion that passes makes the run `Success`.** +/// +/// The whole release in one assertion. Until now every contract this crate built +/// carried `Verification::None`, so `Finished` was the only thing a clean run +/// could return and `Success` was unreachable from this interface. A criterion the +/// operator wrote is what makes the difference, and the run has to come back +/// saying so. +/// +/// `true` and `false` are used as the gate programs because they are the only +/// commands whose exit status is their entire contract. The policy is permissive +/// so that `Act::Exec` admits them — a refused program is a different fact, and +/// `live_f2_a_refused_gate_program_is_not_a_failing_gate` is where that belongs. +#[tokio::test] +#[ignore = "live: needs OPENROUTER_API_KEY"] +#[cfg(unix)] +async fn live_f2_a_command_criterion_that_passes_makes_the_run_succeed() { + 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").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 config = gated("[app.io-cli.gates]\ncommand = [\"true\"]\n"); + let contract = io_cli::contract::configured( + "Replace the line `old line` in notes.md with `new line`. Nothing else.", + root.to_path_buf(), + &config, + ) + .with_max_steps(12); + + // The criterion actually reached the contract. Asserted before the run, + // because a run that succeeded with no criterion on it would look identical + // from the outcome alone — which is exactly the confusion this release exists + // to end. + assert!( + matches!( + contract.verify, + io_harness::Verification::Command { ref argv, expect_exit } + if argv == &vec!["true".to_string()] && expect_exit == 0 + ), + "the configured criterion did not reach the contract: {:?}", + contract.verify, + ); + + let result = session + .turn_bounded_observed( + &contract, + &provider, + &store, + &Policy::permissive(), + &io_harness::ApproveAll, + &io_harness::Ignore, + ) + .await + .expect("the turn runs"); + + println!("live 0.24.0 F2: outcome {:?}", result.outcome); + + assert!( + matches!(result.outcome, io_harness::RunOutcome::Success { .. }), + "a passing criterion should return Success, not {:?} — if this is Finished \ + the criterion never ran", + result.outcome, + ); + + let attempts = store + .gate_attempts(result.run_id) + .expect("the gate attempts"); + println!("live 0.24.0 F2: {} gate attempt(s)", attempts.len()); + assert!( + io_cli::gates::standing(&attempts) + .is_some_and(|standing| matches!(standing.outcome, io_harness::GateOutcome::Passed)), + "the store should record a passing gate attempt, found {attempts:?}", + ); + + // And the exit status a headless run would have reported is unchanged by a + // gate that passed. + assert_eq!( + io_cli::exec::verified_code(&result.outcome, io_cli::gates::standing(&attempts).as_ref()), + io_cli::exec::OK, + ); +} + +/// **0.24.0 F2 + F8 — a failing criterion is recorded, and it is exit `6`.** +/// +/// The negative half, and the one that pays for the release: without it an +/// unattended run exits `0` over work nothing checked. `false` cannot be satisfied +/// by any amount of agent effort, so this asserts the gate's verdict rather than +/// the model's competence — the run is allowed to do whatever it likes with the +/// file and still must not be reported as verified. +#[tokio::test] +#[ignore = "live: needs OPENROUTER_API_KEY"] +#[cfg(unix)] +async fn live_f2_f8_a_failing_criterion_is_recorded_and_exits_six() { + 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").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 config = gated("[app.io-cli.gates]\ncommand = [\"false\"]\n"); + let contract = io_cli::contract::configured( + "Replace the line `old line` in notes.md with `new line`. Nothing else.", + root.to_path_buf(), + &config, + ) + .with_max_steps(6); + + let result = session + .turn_bounded_observed( + &contract, + &provider, + &store, + &Policy::permissive(), + &io_harness::ApproveAll, + &io_harness::Ignore, + ) + .await + .expect("the turn runs"); + + println!("live 0.24.0 F8: outcome {:?}", result.outcome); + + assert!( + !matches!(result.outcome, io_harness::RunOutcome::Success { .. }), + "a criterion that cannot pass must never return Success", + ); + + let attempts = store + .gate_attempts(result.run_id) + .expect("the gate attempts"); + let standing = io_cli::gates::standing(&attempts).expect("a gate ran and answered"); + println!( + "live 0.24.0 F8: phase {:?}, outcome {:?}, attempt {}", + standing.phase, standing.outcome, standing.attempt, + ); + assert_eq!(standing.phase, "command"); + assert!(matches!(standing.outcome, io_harness::GateOutcome::Failed)); + + // The point of the release: whatever the run itself reported, the operator's + // own criterion decides the status a script branches on. + assert_eq!( + io_cli::exec::verified_code(&result.outcome, Some(&standing)), + io_cli::exec::UNVERIFIED, + "a run whose gate said no must exit 6, not {}", + io_cli::exec::code(&result.outcome), + ); + + // And the retry has something real to say. An empty failure text would make + // the follow-up turn no better informed than the first, which is the whole + // reason io-cli drives one at all. + let events = store.sandbox_events(result.run_id).expect("sandbox events"); + let step = attempts.last().map(|last| last.step).unwrap_or_default(); + println!( + "live 0.24.0 F7: recorded output {:?}", + io_cli::gates::output(&events, step), + ); +} + +/// **0.24.0 F5 — a rubric is judged by a second model, and the verdict arrives.** +/// +/// The one criterion that costs a provider call, and the only one whose verdict +/// reaches the event stream at all: `EventKind::Reviewed` is emitted for a review +/// and for nothing else. The rubric is written so that a correct run passes it and +/// the judgement is still a real one — a rubric no model could satisfy would prove +/// only that the reviewer answered. +/// +/// The reviewer is named explicitly and is the same model here, with +/// `allow_self_review` set, because this asserts the mechanism rather than the +/// independence. An operator who does not set it is refused, which is asserted in +/// the offline suite. +#[tokio::test] +#[ignore = "live: needs OPENROUTER_API_KEY"] +async fn live_f5_a_rubric_is_judged_by_a_second_model() { + 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").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()); + + // The provider block is what `contract::configured` builds the reviewer from, + // so the section carries both. The key reaches the file only in memory. + let config = gated(&format!( + "[[provider]]\nkind = \"openrouter\"\nmodel = \"{model}\"\napi_key = \"{key}\"\n\n\ + [app.io-cli.gates]\nrubric = \"notes.md contains the line `new line`\"\n\ + reviewer = \"{model}\"\nallow_self_review = true\n", + model = model(), + )); + + let contract = io_cli::contract::configured( + "Replace the line `old line` in notes.md with `new line`. Nothing else.", + root.to_path_buf(), + &config, + ) + .with_max_steps(12); + + assert!( + matches!(contract.verify, io_harness::Verification::Review { .. }), + "the rubric did not reach the contract: {:?}", + contract.verify, + ); + assert!( + contract.reviewer.is_some(), + "a Review criterion with no reviewer is Error::Config at run start", + ); + + let collected = Arc::new(Mutex::new(Vec::new())); + let observer = Collector { + events: Arc::clone(&collected), + }; + + let result = session + .turn_bounded_observed( + &contract, + &provider, + &store, + &Policy::permissive(), + &io_harness::ApproveAll, + &observer, + ) + .await + .expect("the turn runs"); + + println!("live 0.24.0 F5: outcome {:?}", result.outcome); + + let events = collected.lock().expect("not poisoned").clone(); + let reviewed: Vec<_> = events + .iter() + .filter_map(|event| match &event.kind { + EventKind::Reviewed { passed, reasons } => Some((*passed, reasons.clone())), + _ => None, + }) + .collect(); + println!("live 0.24.0 F5: reviewed {reviewed:?}"); + + assert!( + !reviewed.is_empty(), + "a review criterion must emit EventKind::Reviewed; the second model was \ + never asked", + ); + + // A refusal has to carry its reasons, because the reasons are the only thing + // the operator can act on and the only thing the retry can carry. + for (passed, reasons) in &reviewed { + if !passed { + assert!( + !reasons.is_empty(), + "a failing review with no reasons tells the operator nothing", + ); + } + } + + let attempts = store + .gate_attempts(result.run_id) + .expect("the gate attempts"); + let standing = io_cli::gates::standing(&attempts).expect("the review answered"); + assert_eq!(standing.phase, "review"); +} diff --git a/tests/provider.rs b/tests/provider.rs index d112078..8a26667 100644 --- a/tests/provider.rs +++ b/tests/provider.rs @@ -43,6 +43,22 @@ const HANDSHAKE: &str = "verify.rs"; /// below — without which this exemption would be a hole rather than a boundary. const PANEL: &str = "providers.rs"; +/// The verification gate's second model, excluded for the handshake's reason. +/// +/// 0.24.0 lets an operator have a rubric judged by a model that is **not** the one +/// doing the work, and `TaskContract::with_reviewer` takes an `Arc` +/// built over a provider of its own. That is a third construction, like the +/// wizard's ping: it never drives a turn, it is deliberately a different model +/// from the session's, and merging it into `build`'s maker would mean the +/// reviewer changing model whenever `/model` did — which is the one thing a second +/// opinion must not do. +/// +/// **This exemption was earned twice over.** The construction was written into +/// `src/gates.rs` first and into `src/provider.rs` second, and this gate refused +/// both. Excluded by name like the other two, and held to the distinction by +/// [`f10_the_reviewer_judges_and_never_drives`] below. +const REVIEW: &str = "reviewer.rs"; + fn sources() -> Vec<(PathBuf, String)> { fn walk(dir: &Path, out: &mut Vec<(PathBuf, String)>) { for entry in std::fs::read_dir(dir).expect("src is readable").flatten() { @@ -68,7 +84,9 @@ fn f10_each_provider_is_constructed_in_exactly_one_place() { for constructor in CONSTRUCTORS { let sites: Vec<(PathBuf, usize)> = sources .iter() - .filter(|(path, _)| !path.ends_with(HANDSHAKE) && !path.ends_with(PANEL)) + .filter(|(path, _)| { + !path.ends_with(HANDSHAKE) && !path.ends_with(PANEL) && !path.ends_with(REVIEW) + }) .map(|(path, text)| (path.clone(), text.matches(constructor).count())) .filter(|(_, count)| *count > 0) .collect(); @@ -76,9 +94,9 @@ fn f10_each_provider_is_constructed_in_exactly_one_place() { let total: usize = sites.iter().map(|(_, count)| count).sum(); assert_eq!( total, 1, - "`{constructor}` should be written exactly once outside {HANDSHAKE} \ - and {PANEL}, so that the interactive and the headless entry points \ - cannot drift apart. Found {sites:?}", + "`{constructor}` should be written exactly once outside {HANDSHAKE}, \ + {PANEL} and {REVIEW}, so that the interactive and the headless entry \ + points cannot drift apart. Found {sites:?}", ); // Naming the file as well as the count is what makes this fail while the @@ -167,3 +185,67 @@ fn f10_both_entry_points_reach_a_provider_through_that_site() { ); } } + +/// The reviewer module judges work and never drives a turn. +/// +/// Without this, excluding `reviewer.rs` from the construction count would be a +/// hole rather than a boundary: anything at all could be built there and the gate +/// would say nothing. The same shape as +/// [`f10_the_panel_interrogates_presets_and_never_runs_one`], for the same reason. +/// +/// Sabotage: give `src/reviewer.rs` a `WithProvider` impl or a `Session` — under +/// which only this fails, and it fails by letting a second driving path grow +/// inside the one exemption that was granted for not being one. +#[test] +fn f10_the_reviewer_judges_and_never_drives() { + let sources = sources(); + + let (_, text) = sources + .iter() + .find(|(path, _)| path.ends_with(REVIEW)) + .expect("src/reviewer.rs exists, or the exemption above names nothing"); + + // It is what it claims to be: io-harness's own judge, built here and handed + // over. Without this the file could be exempt and empty of the one thing the + // exemption was granted for. + assert!( + text.contains("ModelReviewer"), + "src/reviewer.rs is exempted because it builds a reviewer; it names none", + ); + + // And it is nothing else. Each of these would make it a second path a turn + // could run on, which is exactly what the count exists to prevent. + for driving in [ + "WithProvider", + "Watched", + "Session", + "Store", + "TaskContract", + "turn_bounded", + ".complete(", + ] { + assert!( + !text.contains(driving), + "src/reviewer.rs names `{driving}` — it is exempted from the one-site \ + rule precisely because it never drives a turn", + ); + } + + // Reachable from the contract and from nowhere else, so the exemption cannot + // become a back door into the turn path. + let callers: Vec = sources + .iter() + .filter(|(_, text)| text.contains("reviewer::build")) + .map(|(path, _)| { + path.file_name() + .expect("a file name") + .to_string_lossy() + .into_owned() + }) + .collect(); + assert_eq!( + callers, + vec!["contract.rs".to_string()], + "the reviewer is built for the contract's criterion and for nothing else", + ); +} diff --git a/tests/servers.rs b/tests/servers.rs index 63ae346..3fb2522 100644 --- a/tests/servers.rs +++ b/tests/servers.rs @@ -884,3 +884,189 @@ fn f6_a_shadowed_server_is_not_offered_for_editing_because_it_is_not_running() { array whole, so `docs` is not a server this workspace runs", ); } + +// --- F10: the edit verb, reachable from a keystroke at last -------------------- +// +// `servers::edit` has been in the library and tested since 0.21.0 with nothing +// calling it — a limitation the product has *stated* since 0.21.0. 0.24.0 wires +// it to `/mcp`, and the wiring is the part that can be got wrong: the value has +// to survive a trip through a composer, and the entry has to be found again +// afterwards by its own content rather than by the row index that showed it. + +/// The composer line `/mcp` writes carries the server's **id**, never its index. +/// +/// **This is the assertion that stops 0.20.0's wrong delete being re-shipped one +/// verb over.** `mcp[1].command` is a position in one file's `[[mcp]]` array; the +/// operator is about to type a value into a composer any other keystroke can +/// leave, and a file edited under them in between moves the array. So the line +/// names the entry by the one stable thing about it, and the driver resolves the +/// position again from the file's own bytes when the line comes back. +/// +/// Sabotage: put `at.index()` in the composer line instead. The round trip still +/// "works" on every one-server fixture and on this one — and silently edits the +/// wrong entry the moment the array has moved, which is exactly the shape that +/// survives every test written against a single-entry file. +#[test] +fn f10_the_edit_line_addresses_a_server_by_id_and_reresolves_its_position() { + let dir = tempfile::tempdir().expect("a temporary directory"); + std::fs::write( + dir.path().join(io_harness::config::PROJECT_FILE), + TWO_SERVERS, + ) + .expect("the project file"); + + // What the surface puts on the prompt, spelled exactly as the driver spells + // it — one constant, or the verb goes quietly dead the first time either half + // is retyped. + let line = format!("{}search.command", io_cli::app::SERVER_KEY); + let (id, key) = io_cli::app::server_key(&line).expect("the line names a server and a key"); + assert_eq!(id, "search"); + assert_eq!(key, "command"); + assert!( + !line.contains('['), + "the line carries an id, not a position in an array" + ); + + // And the position comes from the file, not from the line. + let at = servers::declared_in(dir.path(), id).expect("the project file declares it"); + assert_eq!( + at.index(), + 1, + "`search` is the second entry, found by content" + ); + assert_eq!( + at.scope, + Scope::Project, + "the scope comes from the lookup too, so the write needs no second question", + ); + + // An id with a dot in it still splits at the key, because the KEY is the part + // this crate knows the spelling of and a server may be called anything. + assert_eq!( + io_cli::app::server_key("mcp.some.dotted.id.url"), + Some(("some.dotted.id", "url")), + ); + // And the shapes that are not an edit at all. + for not_one in ["mcp.", "mcp.docs", "app.io-cli.theme", "mcp.docs."] { + assert_eq!( + io_cli::app::server_key(not_one), + None, + "{not_one} is not an `mcp..` line and must not be treated as one", + ); + } +} + +/// A key that is not one an `[[mcp]]` entry may carry is refused, not written. +/// +/// **The refusal is the whole reason `servers::edit` returns an option.** `[[mcp]]` +/// is not held to `deny_unknown_fields`, so `comand = "mcp-find"` is written, +/// accepted by io-harness, and ignored — `configure::write`'s round trip parses it +/// happily and the operator is told their change landed while the server goes on +/// running the old command. There is no other write in this crate whose failure +/// says nothing at all. +#[test] +fn f10_a_key_no_mcp_entry_carries_is_refused_rather_than_written() { + let at = At::of(Scope::User, TWO_SERVERS, "docs").expect("the fixture names `docs`"); + assert!( + servers::edit(&at, "comand", "\"mcp-find\"").is_none(), + "a typo'd key must not become a line io-harness silently ignores", + ); + for key in servers::KEYS { + assert!( + servers::edit(&at, key, "\"x\"").is_some(), + "{key} is an editable key and the verb must reach it", + ); + } +} + +/// What an operator typed becomes TOML, and the four kinds are four shapes. +/// +/// **A composer hands over a person's typing, and `servers::edit` takes TOML +/// source.** The spelling every call site reaches for — `format!("\"{typed}\"")` — +/// is a parse error or a *different value* the moment the text carries a quote or +/// a backslash, and a Windows command path is full of the second. So the assertion +/// that matters is the one on a value with a backslash in it: a naive +/// implementation passes every other case here. +#[test] +fn f10_a_typed_value_becomes_the_toml_its_key_needs() { + // A string, escaped. Nothing else in this test would catch a `format!`. + assert_eq!( + io_cli::app::server_value("command", r"C:\tools\mcp.exe"), + r#""C:\\tools\\mcp.exe""#, + "a backslash written raw is a different path, and a quote is a parse error", + ); + assert_eq!( + io_cli::app::server_value("url", "https://example.test/mcp"), + "\"https://example.test/mcp\"", + ); + // A list, written the way a person writes a command line. Asserted by shape + // rather than byte-for-byte: the spacing inside the brackets is `toml`'s own + // and an assertion on it would go blind the first time that crate reformatted. + let args = io_cli::app::server_value("args", "--index ./idx"); + assert!( + args.starts_with('[') && args.contains("\"--index\"") && args.contains("\"./idx\""), + "a command line becomes an array of arguments, not one string: {args}", + ); + // A number stays a number: quoting it would be a string io-harness refuses. + assert_eq!(io_cli::app::server_value("timeout_secs", " 30 "), "30"); + // An inline table has exactly one spelling and is passed through, so + // `edit::apply`'s own parse is what refuses a malformed one. + assert_eq!( + io_cli::app::server_value("env", "{ TOKEN = \"abc\" }"), + "{ TOKEN = \"abc\" }", + ); + + // End to end: the typing, the TOML, the edit, the file — and the file still + // parses, which is what a value that was quoted by hand would not guarantee. + let at = At::of(Scope::User, TWO_SERVERS, "docs").expect("the fixture names `docs`"); + let edit = servers::edit( + &at, + "command", + &io_cli::app::server_value("command", r"C:\tools\mcp.exe"), + ) + .expect("`command` is an editable key"); + let after = io_cli::edit::apply(TWO_SERVERS, &[edit]).expect("the write parses back"); + assert!(after.contains(r#"command = "C:\\tools\\mcp.exe""#)); + assert!( + after.contains("command = \"mcp-search\""), + "the other entry is untouched", + ); +} + +/// The driver reaches the verb, and reaches it through `At`. +/// +/// Nothing under `tests/` links `src/main.rs`, so the wiring is read as text — +/// the instrument `tests/contract.rs` and `tests/context_share.rs` already use for +/// exactly this. Two properties: the edit row exists at all, and the write is +/// aimed at the scope the lookup returned rather than at a scope somebody picked. +#[test] +fn f10_the_driver_writes_into_the_scope_the_lookup_found() { + let driver = std::fs::read_to_string( + std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src/main.rs"), + ) + .expect("the driver"); + // Whitespace removed, because rustfmt decides where a deeply nested call + // breaks and an assertion about where a newline sits would go blind the first + // time one of these gained an argument. + let flat: String = driver.chars().filter(|c| !c.is_whitespace()).collect(); + + assert!( + flat.contains("io_cli::servers::edit("), + "`servers::edit` is reachable from no keystroke, which is the limitation \ + 0.24.0 exists to remove", + ); + assert!( + flat.contains("io_cli::servers::declared_in(&root,id)"), + "the entry is found again by its id when the composer line comes back", + ); + assert!( + flat.contains("io_cli::configure::write(&root,at.scope,&[edit])"), + "the write goes to the scope the lookup returned; asking the operator \ + which file would let them aim an index at a different file's array", + ); + assert!( + flat.contains("io_cli::app::SERVER_KEY"), + "the composer line is built from the one constant the driver also matches \ + on, so the verb cannot go dead by either half being retyped", + ); +} diff --git a/tests/status.rs b/tests/status.rs index 6947ecb..e88a87f 100644 --- a/tests/status.rs +++ b/tests/status.rs @@ -2332,3 +2332,335 @@ fn the_cost_field_is_drawn_by_the_line_and_by_the_footer_alike() { ); } } + +// --------------------------------------------------------------------------- +// 0.24.0 — the verification gate's standing on the status line. +// --------------------------------------------------------------------------- + +/// A gated turn that failed its criterion and is being tried again. +/// +/// The standing is a word the driver hands over rather than one this test +/// invents a type for: `Status::gate` is plain data, so what a test can state +/// about it is exactly what an operator would see. +fn gated(standing: &str, attempt: Option) -> Status { + let mut status = Status::new("anthropic/claude-sonnet-4.5"); + status.gate = Some(standing.to_string()); + status.gate_attempt = attempt; + status +} + +/// **The standing reaches the terminal through BOTH renderers, out of one +/// method.** +/// +/// `Status::line` is the one-row form and has exactly one production caller — +/// the fallback for a terminal under seven rows. `Status::footer` is the +/// three-row form `Status::render` takes on everything taller, which is every +/// real terminal. This file has shipped a field into one of them twice: 0.8.0's +/// spend and 0.12.0's planning phase were each added to `line`, asserted against +/// `line`, went green, and were nowhere on screen in a live capture. +/// +/// The spelling is asserted against `Status::gate_field` rather than against a +/// literal repeated per arm, so the one place that composes it is the one place a +/// rename has to touch — a renderer that grew its own `format!` would fail here +/// even if both forms happened to say something. +/// +/// **And the standing is a word, so it survives every set.** The line is read in +/// `MONO` as well, which is the theme `NO_COLOR` and `--plain` resolve to: a +/// standing carried by a colour or a tick is a standing those readers do not +/// have. The whole field is ASCII, so no glyph set can change it. +/// +/// Sabotage: push `self.gate_field()` into `Status::fields` alone and drop the +/// `counts.extend(self.gate_field())` from the footer — under which the +/// `Status::line` arm below stays green and the binary shows nothing. +#[test] +fn the_gate_standing_is_drawn_by_the_line_and_by_the_footer_alike() { + for standing in ["passed", "failed", "errored", "running"] { + let status = gated(standing, None); + let spelling = status + .gate_field() + .expect("a configured criterion has a standing"); + assert_eq!( + spelling, + format!("gate {standing}"), + "the standing is spelled once, plainly, and in the harness's own word", + ); + for (renderer, text) in both_renderers(&status) { + assert!( + text.contains(&spelling), + "{renderer} does not say where the gate stands: {text:?}", + ); + } + } + + // Every character of it is ASCII, which is what makes the claim above true + // of a terminal that cannot draw braille as well as of one that can. + let status = gated("failed", Some(3)); + let spelling = status.gate_field().expect("a standing"); + assert!( + spelling.is_ascii(), + "the standing is not readable under the ASCII set: {spelling:?}", + ); + let mono = status.line(200, &MONO).to_string(); + assert!( + mono.contains(&spelling), + "the standing is gone from a terminal with no colour: {mono:?}", + ); +} + +/// **A gate with nothing behind it is absent rather than zero.** +/// +/// Most sessions configure no criterion at all, and one of them has not passed a +/// gate zero times — it has not been gated. So the field draws nothing: not +/// `none`, not `gate 0`, not an empty label with a separator either side of it, +/// which on this line is the same defect wearing a space. It is the rule +/// `tokens`, `spend`, `bg N` and the queue depth are already held to. +/// +/// Sabotage: render `self.gate.clone().unwrap_or_default()` — under which every +/// ungated session carries a bare `gate` with nothing after it, and only this +/// test fails. +#[test] +fn the_gate_field_with_nothing_behind_it_is_absent_rather_than_zero() { + let status = Status::new("opus-5"); + assert_eq!( + status.gate_field(), + None, + "a session nobody asked to verify composed a standing", + ); + for (renderer, text) in both_renderers(&status) { + assert!( + !text.contains("gate"), + "{renderer} drew a gate on a session with no criterion: {text:?}", + ); + assert!( + !text.contains("attempt"), + "{renderer} counted attempts at a gate that does not exist: {text:?}", + ); + } + + // An attempt number with no standing beside it is a number about nothing, + // and it must not summon the field on its own — this is the state a driver + // leaves behind if it reports the retry before the verdict. + let mut counted = Status::new("opus-5"); + counted.gate_attempt = Some(2); + assert_eq!( + counted.gate_field(), + None, + "an attempt count drew a gate that has no standing", + ); + for (renderer, text) in both_renderers(&counted) { + assert!( + !text.contains("attempt"), + "{renderer} drew a retry count with no verdict behind it: {text:?}", + ); + } +} + +/// **The attempt number appears from the second attempt and never on the first.** +/// +/// Every gate that ran at all ran once, so `attempt 1` would be on every gated +/// line in the world and would tell an operator nothing — the argument +/// `Budgets::in_force` already makes about io-cli's own step floor, which is a cap +/// on every turn and therefore a ceiling nobody chose. What is worth the ten cells +/// is that the turn is being *retried*: the criterion did not hold, the retry +/// budget is being spent, and the agent is doing the work again. +/// +/// The first arm asserts the absence of the word `attempt` outright rather than of +/// the string `attempt 1`, because `attempt 1` is absent from a line reading +/// `attempt 12` and an assertion satisfied by the bug is not an assertion. +/// +/// Sabotage: drop the `attempt > 1` guard — under which every gated line carries +/// `attempt 1`, the first arm of this test fails, and nothing else in the +/// repository does. +#[test] +fn the_gate_attempt_is_drawn_only_from_the_second_attempt() { + for first in [None, Some(1)] { + let status = gated("failed", first); + assert_eq!( + status.gate_field().as_deref(), + Some("gate failed"), + "the first attempt is every gate's first attempt and is not news", + ); + for (renderer, text) in both_renderers(&status) { + assert!( + !text.contains("attempt"), + "{renderer} numbered the attempt nobody needed numbering: {text:?}", + ); + } + } + + let status = gated("failed", Some(2)); + let spelling = status.gate_field().expect("a retried gate has a standing"); + assert_eq!( + spelling, "gate failed attempt 2", + "the retry is stated in words beside the standing it belongs to", + ); + for (renderer, text) in both_renderers(&status) { + assert!( + text.contains(&spelling), + "{renderer} does not say the turn is being retried: {text:?}", + ); + } + + // And it keeps counting rather than sticking at two, which is what a + // hard-coded second attempt would look like from the outside. + assert_eq!( + gated("errored", Some(7)).gate_field().as_deref(), + Some("gate errored attempt 7"), + ); +} + +/// **A standing belongs to the turn that was gated and outlives no other.** +/// +/// `Status::forget_run` clears the per-run facts when the conversation under the +/// line changes — `/resume` onto another session, `/fork` away from this one, a +/// rewind that undoes the turn that set one — and `Status::start_run` clears them +/// when the operator simply types again, which is the path `forget_run` never +/// sees. Both matter here for different reasons: a resumed conversation nobody +/// ever gated would otherwise inherit `gate passed` and read as verified, and an +/// ordinary second turn would spend its whole length under the first turn's +/// verdict. This codebase has shipped exactly that with a per-turn field before. +/// +/// The planning phase is set beside it so the test states the *difference* rather +/// than half of it: a standing choice survives both, an account of one turn +/// survives neither. +/// +/// Sabotage: clear `gate` and leave `gate_attempt` standing — under which a later +/// verdict on a later turn is drawn with the retry count of an earlier one, and +/// the last assertion of each half is what catches it. +#[test] +fn the_gate_standing_does_not_outlive_the_turn_that_was_gated() { + for clear in [Status::forget_run as fn(&mut Status), Status::start_run] { + let mut status = gated("failed", Some(2)); + // A standing choice, which is not a fact about the run and must survive. + status.planning = true; + for (renderer, text) in both_renderers(&status) { + assert!( + text.contains("gate failed attempt 2"), + "{renderer} never drew the standing this test is about: {text:?}", + ); + } + + clear(&mut status); + + assert_eq!(status.gate, None, "the verdict outlived its turn"); + assert_eq!( + status.gate_attempt, None, + "the retry count outlived the verdict it counted", + ); + assert!( + status.planning, + "a standing choice was cleared with the run" + ); + for (renderer, text) in both_renderers(&status) { + assert!( + !text.contains("gate"), + "{renderer} still reports the previous turn as gated: {text:?}", + ); + assert!( + !text.contains("attempt"), + "{renderer} kept the previous turn's retry count: {text:?}", + ); + assert!( + text.contains("planning"), + "{renderer} lost the mode the gate was not: {text:?}", + ); + } + } +} + +/// **The standing survives a width the counters do not, and the line still never +/// wraps.** +/// +/// Both renderers drop from the right, so position is priority, and the module +/// states which way this one goes: a standing mode that stops the agent writing +/// outranks what the last turn spent. A gate that has not passed is that fact from +/// the other end — it is *why* the turn is not finished, and on a retry it is why +/// the agent is doing the work a second time. So it sits right of the planning +/// phase, which is a choice that holds after the turn ends, and left of every +/// counter, which is an account of what the turn cost. +/// +/// The order is asserted on `Status::fields` directly as well as through a +/// rendered width, because an arithmetic assertion alone would move the day +/// somebody adds a field five cells wider, and the claim being made is about +/// order rather than about a hundred columns. +/// +/// Sabotage: push the gate after `ctx` in `Status::fields` and after `steps` in +/// the footer's counts — under which both forms still draw it at two hundred +/// columns, every other test in this file stays green, and the one field that +/// explains a turn that will not end is the first thing a narrow terminal gives +/// up. +#[test] +fn the_gate_standing_survives_a_width_the_counters_do_not() { + let mut status = full_row(); + status.gate = Some("failed".into()); + status.gate_attempt = Some(2); + + // Order first, where it is a claim about the list rather than about a width. + let fields = status.fields(&DARK); + let index = |needle: &str| { + fields + .iter() + .position(|field| field.text.contains(needle)) + .unwrap_or_else(|| panic!("no field says {needle:?}: {fields:?}")) + }; + assert!( + index("planning") < index("gate "), + "the standing outranked a choice that outlives the turn: {fields:?}", + ); + assert!( + index("gate ") < index("6 steps") + && index("gate ") < index("26.9k tok") + && index("gate ") < index("ctx 23%"), + "a counter outranked the reason the turn is not finished: {fields:?}", + ); + + // Wide enough for everything, so the narrowing below is the only thing the + // arms after it can be failing on. + let wide = rendered(&status, 200); + for fact in ["gate failed attempt 2", "6 steps", "26.9k tok", "ctx 23%"] { + assert!(wide.contains(fact), "{fact:?} was never drawn: {wide:?}"); + } + + // A hundred columns, which is the width the 0.21.0 live capture was taken at + // and the one an ordinary terminal is most likely to be. + let narrowed = rendered(&status, 100); + assert!(narrowed.chars().count() <= 100, "{narrowed:?}"); + assert!(!narrowed.contains('\n'), "the line wrapped: {narrowed:?}"); + assert!( + narrowed.contains("gate failed attempt 2"), + "the standing went before the counters it outranks: {narrowed:?}", + ); + for counter in ["6 steps", "26.9k tok", "ctx 23%"] { + assert!( + !narrowed.contains(counter), + "nothing was dropped, so this width proves nothing: {narrowed:?}", + ); + } + + // The footer narrows by its own mechanism — counters come off the right of + // the counts group until the right-hand group fits — so the same claim has to + // be made against it separately or half of it is untested. + let footer = footed(&status, 100); + assert!( + footer.contains("gate failed attempt 2") && footer.contains("planning"), + "the footer gave up the standing to keep a counter: {footer:?}", + ); + assert!( + !footer.contains("6 steps"), + "the footer row fitted whole, so it proves nothing: {footer:?}", + ); + + // And at a width that holds one field, it is still the model. Everything this + // task adds goes before it and after everything else. + let cramped = rendered(&status, 40); + assert!(cramped.chars().count() <= 40, "{cramped:?}"); + assert!(!cramped.contains('\n'), "the line wrapped: {cramped:?}"); + assert!( + cramped.contains("anthropic/claude-sonnet-4.5"), + "the model is the last field to go: {cramped:?}", + ); + assert!( + !cramped.contains("gate"), + "the gate outlasted the model: {cramped:?}", + ); +} diff --git a/tests/steer.rs b/tests/steer.rs index 49a1ce0..1d34e55 100644 --- a/tests/steer.rs +++ b/tests/steer.rs @@ -1,5 +1,12 @@ //! F5 — a queued line reaches the turn that is still running, on both arms. //! +//! **And F7 since 0.24.0, at the bottom of the file, because it is the same +//! queue.** A failing verification gate buys another turn by putting a prompt at +//! the FRONT of the queue this file is about — not by opening a second driver +//! loop beside the one that already drains it. So the retry's mechanism belongs +//! next to the mechanism it borrows, and the same instrument proves it: the +//! library halves are driven directly, and the driver's wiring is read as text. +//! //! **What this file can and cannot prove, stated first, because the gap is the //! whole reason the file is shaped like this.** //! @@ -347,3 +354,348 @@ fn f5_a_send_after_the_turn_has_ended_is_refused() { "and the other two go the same way", ); } + +// --------------------------------------------------------------------------- +// F7 — a failing gate buys another turn, through this same queue +// --------------------------------------------------------------------------- +// +// The retry is not a second driver loop. It is a prompt at the FRONT of the queue +// this file is otherwise about, so a turn earned by a gate and a turn typed by an +// operator go through one `turn` call, get one echo, one clock and one `Ctrl+C`. +// What is provable here is the mechanism — the queue's order, the prompt's +// contents, and the fact that the wiring in the driver exists at all. What is not +// is that a real model reads it, which is the live run's to close. + +/// One recorded gate evaluation. `at` is a stored string this crate never parses. +fn attempt(step: u32, phase: &str, outcome: io_harness::GateOutcome) -> io_harness::GateAttempt { + io_harness::GateAttempt { + id: 1, + step, + phase: phase.into(), + outcome, + detail: String::new(), + at: String::new(), + } +} + +/// F7 — the retry prompt carries the criterion and the failure, and not the goal. +/// +/// **A retry that re-sent the original prompt is the sabotage this criterion +/// names**, and it is the one that looks like it works: a second turn runs, the +/// model does more work, and the gate fails again for exactly the reason nobody +/// told it about. `app::gate_retry` has no parameter a goal could arrive through, +/// so that implementation cannot be written — and this asserts the two things it +/// must carry instead. +#[test] +fn f7_the_retry_prompt_carries_the_criterion_and_what_failed() { + let criterion = io_cli::gates::Criterion::Command { + argv: vec!["cargo".into(), "test".into()], + expect_exit: 0, + }; + let attempts = [attempt(4, "command", io_harness::GateOutcome::Failed)]; + let events = [ + io_harness::SandboxEvent::gate_output(9, 3, "an older failure, two turns ago"), + io_harness::SandboxEvent::gate_output(9, 4, "error[E0433]: failed to resolve `frobnicate`"), + ]; + + let prompt = io_cli::app::gate_retry(&criterion, &attempts, &events); + assert!( + prompt.contains("cargo test"), + "the retried turn is told what it is judged by, in the words it was judged \ + by: {prompt}", + ); + assert!( + prompt.contains("E0433") && prompt.contains("frobnicate"), + "a retry that does not carry the failure is a retry that repeats it: {prompt}", + ); + assert!( + !prompt.contains("two turns ago"), + "the output is the one for the step this attempt ran after, not the first \ + failure of the run: {prompt}", + ); + assert!( + prompt.contains("Do not change the gate"), + "an agent handed a failing check and no instruction about it will sometimes \ + edit the check", + ); + + // A review says its reasons in the ROW and prints nothing, so a reader that + // only looked at sandbox events would retry with no explanation at all. + let reviewed = io_cli::gates::Criterion::Review { + rubric: "every public item has a doc comment".into(), + reviewer: "vendor/judge".into(), + allow_self_review: false, + }; + let mut refused = attempt(2, "review", io_harness::GateOutcome::Failed); + refused.detail = "three functions carry no documentation".into(); + let prompt = io_cli::app::gate_retry(&reviewed, &[refused], &[]); + assert!( + prompt.contains("three functions carry no documentation"), + "a review's reasons are in its row, not in a sandbox event: {prompt}", + ); + + // And a gate that failed silently still states the criterion, which is more + // than the turn had before. + let quiet = io_cli::app::gate_retry(&criterion, &attempts, &[]); + assert!( + quiet.contains("cargo test") && !quiet.is_empty(), + "a command that printed nothing still has a criterion to state: {quiet}", + ); +} + +/// F7 — an existence criterion is judged here, or it is not judged at all. +/// +/// **This is the single easiest defect in the release to ship.** +/// `Criterion::verification` maps a bare `file` criterion to `Verification::None`, +/// because there is no honest counterpart for it in io-harness's enum — and a run +/// carrying `Verification::None` never reaches the gate: the step loop returns +/// `Finished` the moment the agent stops calling tools, so the store holds no row +/// at all. A caller that read `Store::gate_attempts` and stopped would draw +/// nothing, retry nothing, and report an ungated run. +/// +/// Sabotage: return the recorded rows unchanged. Every command and review gate in +/// the product still works, the suite stays green, and an operator who asked for a +/// file to exist gets a session that never once looks for it. +#[test] +fn f7_an_existence_criterion_is_evaluated_where_io_harness_did_not() { + let dir = tempfile::tempdir().expect("a temporary directory"); + let criterion = io_cli::gates::Criterion::File { + file: std::path::PathBuf::from("REPORT.md"), + contains: None, + }; + + // Nothing recorded, because nothing was asked of io-harness. + let missing = io_cli::gates::gate_attempts(Vec::new(), Some(&criterion), dir.path()); + let standing = io_cli::gates::standing(&missing) + .expect("an existence criterion that reports nothing is an ungated run"); + assert_eq!(standing.outcome, io_harness::GateOutcome::Failed); + assert!( + io_cli::gates::may_retry(&missing, 1), + "and a file that is not there earns the turn that writes it", + ); + + std::fs::write(dir.path().join("REPORT.md"), "done").expect("the report"); + let written = io_cli::gates::gate_attempts(Vec::new(), Some(&criterion), dir.path()); + assert_eq!( + io_cli::gates::standing(&written) + .expect("a standing") + .outcome, + io_harness::GateOutcome::Passed, + ); + + // **A `none` row is dropped, not kept — and the row shape here is the whole + // point.** io-harness evaluates the contract's criterion after every step on + // which the agent called a tool, and for `Verification::None` that evaluation + // is `Ok(false)` — so it records `phase = "none", **Failed**`, once per such + // step, on every ungated run this product has ever driven. An earlier draft of + // this test used `Passed`, a row production never emits, and so proved nothing + // about the case that actually exists: read naively those rows say the gate + // failed on a session nobody gated. + std::fs::remove_file(dir.path().join("REPORT.md")).expect("take it away again"); + let folded = io_cli::gates::gate_attempts( + vec![ + attempt(5, "none", io_harness::GateOutcome::Failed), + attempt(6, "none", io_harness::GateOutcome::Failed), + attempt(7, "none", io_harness::GateOutcome::Failed), + ], + Some(&criterion), + dir.path(), + ); + assert_eq!( + io_cli::gates::standing(&folded) + .expect("a standing") + .outcome, + io_harness::GateOutcome::Failed, + "a row for a criterion that checked nothing must not stand as the verdict", + ); + assert!( + folded.iter().all(|attempt| attempt.phase != "none"), + "the row io-harness wrote about checking nothing is not part of the count", + ); + + // Every other criterion is io-harness's to judge, and the rows come back + // untouched — including the empty list, which is an ungated run. + let command = io_cli::gates::Criterion::Command { + argv: vec!["make".into()], + expect_exit: 0, + }; + let rows = vec![attempt(1, "command", io_harness::GateOutcome::Passed)]; + assert_eq!( + io_cli::gates::gate_attempts(rows.clone(), Some(&command), dir.path()), + rows, + "a command gate is not re-judged here", + ); + assert!(io_cli::gates::gate_attempts(Vec::new(), None, dir.path()).is_empty()); + + // **The case that would have shipped exit 6 to every operator alive.** A + // session with no `[app.io-cli.gates]` at all still leaves `none`/`Failed` + // rows behind for any turn in which the model called a tool. Folded with no + // criterion they must come to nothing — no standing, no status word, no + // scrollback line, and in `io exec` no exit 6. + let ungated = vec![ + attempt(1, "none", io_harness::GateOutcome::Failed), + attempt(2, "none", io_harness::GateOutcome::Failed), + ]; + let folded = io_cli::gates::gate_attempts(ungated, None, dir.path()); + assert!( + folded.is_empty(), + "an ungated run's bookkeeping rows are not a verdict: {folded:?}", + ); + assert!( + io_cli::gates::standing(&folded).is_none(), + "a run nobody gated has no standing", + ); + assert_eq!( + io_cli::exec::verified_code( + &io_harness::RunOutcome::Finished { steps: 2 }, + io_cli::gates::standing(&folded).as_ref(), + ), + io_cli::exec::OK, + "an ungated headless run exits exactly what it exited before this release", + ); +} + +/// F6 — the scrollback line spells the verdict the way everything else does. +#[test] +fn f6_the_gate_line_names_the_phase_the_verdict_and_what_it_printed() { + let attempts = [ + attempt(3, "command", io_harness::GateOutcome::Failed), + attempt(4, "command", io_harness::GateOutcome::Failed), + ]; + let events = [io_harness::SandboxEvent::gate_output( + 9, + 4, + "2 tests failed", + )]; + let (tone, line) = + io_cli::app::gate_report(&attempts, &events).expect("a gated turn has a line"); + // **`Warning`, and `Refused` would be a defect rather than a preference.** + // `Tone::Refused` renders the literal word `refused`, which is the permission + // boundary's word — this release moved the failing review off that tone in + // `src/events.rs` for exactly that reason, and a scrollback line one row below + // spelling `refused: gate failed` would put the collision straight back. + // "The policy would not run my gate" and "your work did not meet the bar" + // need opposite responses from the operator. + assert_eq!( + tone, + io_cli::theme::Tone::Warning, + "a gate that answered no did not refuse anything: `refused` is the boundary's word", + ); + assert_ne!( + tone, + io_cli::theme::Tone::Refused, + "the gate's verdict must not borrow the permission boundary's vocabulary", + ); + assert!( + line.contains("failed") && line.contains("command") && line.contains("2 tests failed"), + "the phase, the verdict and what it printed: {line}", + ); + assert!( + line.contains("attempt 2"), + "the attempt is on the line once there has been more than one: {line}", + ); + + // `GateOutcome::as_str` verbatim, because the status line, the exit code and + // this sentence all have to spell one verdict the same way. + let (_, passed) = io_cli::app::gate_report( + &[attempt(1, "contains", io_harness::GateOutcome::Passed)], + &[], + ) + .expect("a line"); + assert!( + passed.contains(io_harness::GateOutcome::Passed.as_str()), + "the word is the harness's own: {passed}", + ); + assert!( + !passed.contains("attempt"), + "every gate that ran at all ran once, so `attempt 1` tells nobody anything", + ); + + assert!( + io_cli::app::gate_report(&[], &[]).is_none(), + "a turn nothing gated earns no line; a session with no gates section would \ + otherwise carry one under every turn", + ); +} + +/// F7 — the driver drives the retry through the queue it already drains. +/// +/// Nothing under `tests/` links `src/main.rs`, so the wiring is read as text, the +/// way `tests/contract.rs` and `tests/context_share.rs` read it. Four properties, +/// each of which is a way the retry can be wrong while every test above still +/// passes: +/// +/// 1. **One loop.** The retry goes back through `next`, so it gets the +/// configuration refresh, the picture drain and the stop key that every other +/// turn gets. A second loop beside this one is where those drift apart. +/// 2. **The budget is asked of the library.** `gates::may_retry` deliberately +/// differs from `GateOutcome::is_retryable` — both `Failed` and `Errored` earn +/// another *turn*, because the tree changes between turns — and a driver that +/// reached for the method would silently stop retrying the commonest failure +/// there is. +/// 3. **A stopped turn is never retried.** One press of the stop key must not +/// start another turn. +/// 4. **The prompt is composed by the library.** A sentence built in this file +/// would be one no test can read. +#[test] +fn f7_the_retry_rides_the_drivers_own_queue() { + let text = driver(); + let flat = squashed(&text); + + assert!( + flat.contains("next=app.next_queued_prompt();"), + "the retry is drained by the loop that was already there", + ); + assert!( + flat.contains("app.requeue_prompts(vec![prompt])"), + "and it goes to the FRONT of that queue, so it is the next turn and the \ + prompts typed during this one keep their order behind it", + ); + assert!( + flat.contains("io_cli::gates::may_retry(&gated,section.retries())"), + "the budget is the library's answer, asked with the operator's own number", + ); + assert!( + !text.contains("is_retryable"), + "`GateOutcome::is_retryable` answers a different question — whether the \ + SAME criterion over the SAME tree could say something else — and using it \ + would stop retrying every gate that merely failed", + ); + assert!( + flat.contains("!turned.stopped&&io_cli::gates::may_retry("), + "a turn the operator interrupted is not retried", + ); + assert!( + flat.contains("io_cli::app::gate_retry(criterion,&gated,&events)"), + "the prompt is composed in the library, where a test can read it", + ); + + // The attempts are accumulated across the chain. Every turn is its own run, so + // `Store::gate_attempts` restarts at one on each retry — a driver that passed + // only the current run's rows to `may_retry` would buy another turn for every + // one of them, forever, against a real model. + assert!( + flat.contains("gated.extend(io_cli::gates::gate_attempts("), + "the chain's attempts accumulate; `retries = 1` means one further turn, \ + not an unbounded loop", + ); + assert!( + flat.contains("gated.clear();"), + "and a chain that ends releases them, or the next prompt is charged for a \ + gate that failed two prompts ago", + ); + + // The standing comes off the STORE. `EventKind::Sandbox` carries no detail + // payload, so nothing in the event stream can say which phase failed or what + // it printed — and reading rows is also what makes this right after a + // `/resume`, where this process never watched the run. + assert!( + flat.contains("store.gate_attempts(run_id)") + && flat.contains("store.sandbox_events(run_id)"), + "the phase, the verdict and the output are the store's", + ); + assert!( + flat.contains("app.status.gate=Some(standing.outcome.as_str().to_string());"), + "the status word is the harness's own spelling, passed through", + ); +} diff --git a/tests/triage.rs b/tests/triage.rs index 7322619..92f6cce 100644 --- a/tests/triage.rs +++ b/tests/triage.rs @@ -145,6 +145,53 @@ fn the_three_kinds_this_release_draws_are_lines_rather_than_silent() { } } +/// **0.24.0.** The `sandbox` route said `gate_phase_failed` and `gate_output` +/// "belong to a verification gate no session has until 0.24.0". This is that +/// release. +/// +/// The route column is the only evidence anybody has that a fact reaches its +/// reader, so a sentence in it that has stopped being true is worse than a blank +/// one: a reviewer checking whether a kind is drawn reads this and stops. The +/// seam was left open here deliberately, and closing it means closing the +/// sentence that described it as open. +/// +/// Sabotage: restore either retired phrase to the row. Only this fails, and it +/// fails on the claim rather than on the sentence around it — which is the part +/// an author rewrites while leaving the claim standing. +#[test] +fn the_route_column_no_longer_says_a_session_has_no_verification_gate() { + let sandbox = triage::route("sandbox").expect("the sandbox row"); + for retired in ["no session has", "until 0.24.0"] { + assert!( + !sandbox.contains(retired), + "the sandbox route still says {retired:?}, which stopped being true in this \ + release: {sandbox}", + ); + } + // And says where the two of them go now, which is the whole job of this + // column: a `Line` disposition covers seven kinds at once, so the row is the + // only place that can record which of them the arm actually draws. + for kind in ["gate_phase_failed", "gate_output"] { + assert!( + sandbox.contains(kind), + "{kind} is drawn from this release and the route does not say so: {sandbox}", + ); + } + // `dial` is the one kind of the seven that still reaches no line, so the row + // has to go on naming the event that draws it instead. + assert!(sandbox.contains("dialed"), "{sandbox}"); + + // The asymmetry a reader of this table cannot work out from the enum: a + // review that never happened emits nothing at all, so a missing verdict line + // is a criterion that did not answer rather than one that said yes. + let reviewed = triage::route("reviewed").expect("the reviewed row"); + assert!( + reviewed.contains("Errored"), + "the reviewed route does not record that a review which never ran emits \ + nothing here: {reviewed}", + ); +} + /// The other direction, in behaviour rather than in source: a kind whose fact /// belongs to a status field or to another event commits no line. ///