diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c76f28f..e797d25 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -206,10 +206,29 @@ jobs: irm https://raw.githubusercontent.com/initorigin/io-cli/main/install.ps1 | iex ``` + Or with a package manager: + + ```sh + brew tap initorigin/io-cli https://github.com/initorigin/io-cli + brew install initorigin/io-cli/io + ``` + + ```powershell + scoop bucket add io-cli https://github.com/initorigin/io-cli + scoop install io + ``` + + The tap and the bucket are updated from this Release's own + `SHA256SUMS` shortly after it is published, so they may serve the + previous version for a few minutes. + Both scripts verify the artifact against `SHA256SUMS` before unpacking it and install into a per-user directory with no administrator rights. The checksum defends against a truncated download and a tampered asset; `curl | sh` is a trust-the-publisher model regardless. + + `io upgrade` prints the command that updates whichever of these + installed the binary you are running. EOF cat notes.md diff --git a/AGENTS.md b/AGENTS.md index c634b56..2261a54 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -70,7 +70,7 @@ the only dev-dependency. Adding one is a deliberate, argued act — read the com - **No agent loop.** The sweep strips comments first, so a doc comment containing "while" is not a finding; `src/provider.rs` is exempted by path and held to four properties of its own. -**Module map (`src/`, 79 files).** +**Module map (`src/`, 82 files).** - Session surface — `app.rs` (state), `term.rs` (an inline viewport, never the alternate screen), `events.rs` + `triage.rs` (what each io-harness event does; a kind with no @@ -82,7 +82,8 @@ the only dev-dependency. Adding one is a deliberate, argued act — read the com field for field, asserted by Debug equality. - Configuration — `edit.rs` (a format-preserving writer that replaces one value's bytes), `settings.rs`, `configure.rs` (`/config`), `servers.rs` (`/mcp`), `providers.rs`, - `home.rs` (`~/.io-cli`), `reload.rs`, `memory.rs`, `recall.rs`. + `home.rs` (`~/.io-cli`), `reload.rs`, `memory.rs`, `recall.rs`, `upgrade.rs` (which of the + three installers placed the running binary, read from its own path and never from a network). - Work — `exec.rs` (headless, and the exit-code mapping), `resume.rs`, `lock.rs`, `queue.rs`, `compact.rs`, `context.rs`, `fleet.rs`, `plan.rs`, `intent.rs`, `gates.rs`, `routing.rs`, `provider.rs`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 04480e3..bd2b2aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,69 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +## [0.38.0] - 2026-09-04 + +The release that lets you install io the way you install everything else. Distribution was a +script piped from a URL and nothing else — a checksum-verified, no-sudo, no-lifecycle-script +path, and also the path a security-conscious developer's employer has told them not to take. A +Homebrew formula and a scoop manifest now point at the same four artifacts the installers +already fetch, so nothing about what is distributed changes; what changes is how many ways there +are to reach it. Both live in this repository rather than in two of their own, which costs one +longer `brew tap` line and saves two public repositories and a cross-repository push credential. + +`io upgrade` prints the command that updates the binary you are running — `brew upgrade io`, +`scoop update io`, or the installer line — decided from the running binary's own path. It prints +and does not run, and it asks nothing about what the newest version is. There is still no +version check: this crate has no HTTP client and `tests/dependencies.rs` fails the build if it +grows one. + +And three things this product published as limitations stop being true. `io acp` asks the editor +for permission instead of denying every approval. `/cost`'s grouped rows say when a cache-write +count is a floor rather than a fact. And declining a bundle re-install leaves the installed +adapter alone, which the guide has claimed in writing since 0.35.0 while the code did the +opposite. + +### Added + +- A Homebrew formula (`Formula/io.rb`) and a scoop manifest (`bucket/io.json`), served from this + repository as its own tap and bucket. `scripts/update-tap.sh` regenerates both from a published + Release's own `SHA256SUMS`, so no digest is ever hand-copied. +- `io upgrade`, the ninth argv variant. It classifies the running binary's path as a Homebrew, + scoop or script install and names the matching command; a path none of the three would have + produced is told so rather than given advice that might replace a binary something else placed. +- `session/request_permission` in `io acp`, with the three options `io_harness::Decision` can + express. The answer decides the call, and `allow-session` remembers exactly that act on exactly + that target — the same rule the terminal's overlay writes, from one shared function. +- A cache-write floor on `/cost`'s per-model and per-day rows, in the words the totals above them + have used since 0.36.0. + +### Changed + +- **`RunOutcome::SchemaUnsatisfied` has an exit code, a description and an ACP stop reason.** The + variant arrived in io-harness 0.77.0 and that crate's changelog never declares it; the enum is + `#[non_exhaustive]`, so a run that failed its output schema exited `5` where its sibling + `VerificationFailed` exits `6`, and reported `end_turn` to an editor. It now exits `6`. +- The bundle adapter is swapped **after** consent rather than before it, so a declined re-install + leaves the installed adapter byte-identical. +- `io acp`'s reader runs on its own task. It awaited the turn inline, which meant the answer to a + question that turn asked could not be read until the turn returned — a deadlock, and the reason + the permission round trip was never wired. + +### Fixed + +- Forty-one io-harness source citations across `src/` and `tests/` re-anchored to the pinned + tree. They were **three generations deep** — some naming io-harness 0.69-era line numbers, some + 0.74-era — and had been asserting false `file:line` facts for seven or more releases. + `tests/docs.rs`'s `f19` never saw them: it checks that a citation naming a version names the + *pinned* version, and it does not check the line at all, so an unversioned citation is invisible + to it. + +### Dependencies + +- io-harness **0.76.0 → 0.78.0**. `EventKind::ToolCall` gained `origin`, which this release takes + and deliberately does not consume; `EventKind` is 53 variants in both, so no mapping table grew + a row. + ## [0.37.0] - 2026-09-04 The release that says where your context went. `/context` has read the real outgoing request @@ -3397,7 +3460,8 @@ client, tool, sandbox, policy engine or session store of its own. - There is no crates.io publish and `cargo install` is not an install path. - No test in this release asserts on wall-clock time. -[Unreleased]: https://github.com/initorigin/io-cli/compare/v0.37.0...HEAD +[Unreleased]: https://github.com/initorigin/io-cli/compare/v0.38.0...HEAD +[0.38.0]: https://github.com/initorigin/io-cli/compare/v0.37.0...v0.38.0 [0.37.0]: https://github.com/initorigin/io-cli/compare/v0.36.0...v0.37.0 [0.36.0]: https://github.com/initorigin/io-cli/compare/v0.35.0...v0.36.0 [0.35.0]: https://github.com/initorigin/io-cli/compare/v0.34.1...v0.35.0 diff --git a/Cargo.lock b/Cargo.lock index 6b64a04..418deb9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1850,7 +1850,7 @@ dependencies = [ [[package]] name = "io-cli" -version = "0.37.0" +version = "0.38.0" dependencies = [ "clap", "crossterm", @@ -1867,9 +1867,9 @@ dependencies = [ [[package]] name = "io-harness" -version = "0.76.0" +version = "0.78.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d6fd17b013a3db32112f92782b9ea4a0218755774beb9ee9cde0bd4291d2b28c" +checksum = "dd8b7f233c0ef74d0da98f5c9983021688379c2652bfe825cfad44e5bd5aeb02" dependencies = [ "base64 0.23.1", "calamine", @@ -4057,7 +4057,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", - "getrandom 0.3.4", + "getrandom 0.4.3", "once_cell", "rustix", "windows-sys 0.61.2", diff --git a/Cargo.toml b/Cargo.toml index 456fa9d..ba51311 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "io-cli" -version = "0.37.0" +version = "0.38.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 @@ -63,7 +63,7 @@ path = "src/lib.rs" # still correct about what this *renderer* can be handed; it is simply no longer # load-bearing on the build. Turning `barcode` back off is what would restore # it, and `documents` would then have to be spelled out as its other four. -io-harness = { version = "0.76", features = ["media", "browser", "documents"] } +io-harness = { version = "0.78", features = ["media", "browser", "documents"] } # **`tui-textarea` is gone as of 0.18.0, and it is why this pair was frozen.** # The comment that stood here said ratatui stayed at 0.29 and crossterm at 0.28 # because the composer was built out of that widget and the 0.30 line carried diff --git a/Formula/io.rb b/Formula/io.rb new file mode 100644 index 0000000..c33b909 --- /dev/null +++ b/Formula/io.rb @@ -0,0 +1,68 @@ +# The Homebrew formula for io. The repository is the tap, so it is added by URL: +# +# brew tap initorigin/io-cli https://github.com/initorigin/io-cli +# brew install initorigin/io-cli/io +# +# The formula ships the Release's own prebuilt artifacts and has no build step. +# io links io-harness, which carries rusqlite with a bundled C SQLite: building +# from source here would put a C toolchain and several minutes between +# `brew install` and a working binary, and it would hand the user bytes nobody +# published a checksum for. The three archives named below are the ones the +# release workflow built, and every sha256 is copied out of that Release's +# SHA256SUMS by `scripts/update-tap.sh`. Never by hand — a hand-copied digest is +# how a formula comes to name an artifact that does not exist. +# +# The version the URLs name is the newest RELEASED one, which during a release is +# one behind the version being built: an artifact that has not been uploaded yet +# has no checksum to name. The generator runs after the Release is cut and its +# change goes in through an ordinary pull request, because the `pull_request` +# ruleset on `main` and `develop` bypasses for `OrganizationAdmin` alone and the +# release workflow is not one. +# +# **There is deliberately no `version` stanza.** Homebrew scans the version out +# of the url, `brew audit --strict` calls an explicit one redundant, and it is +# worse than redundant: it is a second declaration of a fact the urls already +# carry, and the two can disagree. A formula whose `version` said 0.37.0 over +# urls naming 0.36.0 would install 0.36.0 and report the other number from its +# own `test` block. +class Io < Formula + desc "Terminal agent that shows what it may do, what it spends and what it refused" + homepage "https://github.com/initorigin/io-cli" + license "Apache-2.0" + + on_macos do + if Hardware::CPU.arm? + url "https://github.com/initorigin/io-cli/releases/download/v0.37.0/io-0.37.0-aarch64-apple-darwin.tar.gz" + sha256 "93fa6ea5a85535457056207cf8ddde44c3a2b1f58dd644e78b26853f4169069f" + end + + if Hardware::CPU.intel? + url "https://github.com/initorigin/io-cli/releases/download/v0.37.0/io-0.37.0-x86_64-apple-darwin.tar.gz" + sha256 "f070a8d2afed9498a8a08bbf49683744b5741dda90319291430b99bcb98e08f8" + end + end + + # Linux is musl and x86_64 only, exactly as the release matrix is. There is no + # Linux arm64 artifact to point at, so that machine gets Homebrew's own refusal + # rather than a URL that 404s halfway through an install. + on_linux do + if Hardware::CPU.intel? + url "https://github.com/initorigin/io-cli/releases/download/v0.37.0/io-0.37.0-x86_64-unknown-linux-musl.tar.gz" + sha256 "1196630b787c343e96953a4091e78908d812c39a10876655c4afe6c3dbc2faf3" + end + end + + # Each archive holds one versioned directory — `io--/` with the + # binary beside LICENSE, NOTICE, README.md and CHANGELOG.md — and Homebrew + # descends into that single directory before this runs. Only the binary is + # installed; the licence and the notice travel with the Release itself. + def install + bin.install "io" + end + + # The binary's own words, not the formula's: proof that what was unpacked runs + # here and is the version this file claims to have installed. + test do + assert_match version.to_s, shell_output("#{bin}/io --version") + end +end diff --git a/README.md b/README.md index fcf4891..b0da6d7 100644 --- a/README.md +++ b/README.md @@ -95,15 +95,37 @@ Windows, in PowerShell: irm https://raw.githubusercontent.com/initorigin/io-cli/main/install.ps1 | iex ``` +Or with a package manager you already have. macOS and Linux: + +```sh +brew tap initorigin/io-cli https://github.com/initorigin/io-cli +brew install initorigin/io-cli/io +``` + +Windows: + +```powershell +scoop bucket add io-cli https://github.com/initorigin/io-cli +scoop install io +``` + +The tap and the bucket live in this repository rather than in two of their own, +which is why `brew tap` and `scoop bucket add` name a URL. They install the same +checksum-verified artifacts the scripts do. + Both scripts pick the right build for your machine, **verify it against the published `SHA256SUMS` before unpacking it**, and install into a directory you own — `~/.local/bin`, or `%LOCALAPPDATA%\io\bin` on Windows. Neither needs administrator rights and neither edits your shell profile or your `PATH`: if the directory is not on your `PATH`, the script prints the line to add. -Re-running the script is how you update. There is no auto-update and no version -check: a terminal tool that contacts a server you did not ask it to contact is -one of this product's stated non-goals. +`io upgrade` prints the command that updates the binary you are running: +`brew upgrade io` from a Homebrew install, `scoop update io` from a scoop one, +and the installer line above from a script install. It prints and does not run, +and it asks nothing about what the newest version is. There is still no +auto-update and no version check: a terminal tool that contacts a server you did +not ask it to contact is one of this product's stated non-goals, and `io upgrade` +reads the path of the running binary rather than a network. Set `IO_VERSION` to install a specific version, and `IO_INSTALL_DIR` to install somewhere else. @@ -155,7 +177,7 @@ the limits that capability actually has. | [Pictures and documents](docs/guide/media.md) | Attaching an image, drawing it where the terminal can, and the twelve document tools | | [Reading it without seeing it](docs/guide/accessibility.md) | `--plain`, and what a screen reader, a braille display or a log gets | | [Headless](docs/guide/headless.md) | `io exec`, the exit codes, the JSON, resuming without a terminal, and managing the configuration from a shell | -| [From an editor](docs/guide/editors.md) | `io acp`, pointing Zed or a JetBrains IDE at it, what the editor is sent, and the two things it is not | +| [From an editor](docs/guide/editors.md) | `io acp`, pointing Zed or a JetBrains IDE at it, what the editor is sent, how it is asked for permission, and what it is still not | | [Configuration](docs/guide/configuration.md) | Every section of `io.toml`, changing it without leaving the session, and where io keeps your things | | [What the store is holding](docs/guide/store.md) | Reading the store, putting work back, and taking the work out | | [What this release is not](docs/guide/limits.md) | The limits, stated plainly, and the ones that are decisions rather than gaps | diff --git a/bucket/io.json b/bucket/io.json new file mode 100644 index 0000000..19516b7 --- /dev/null +++ b/bucket/io.json @@ -0,0 +1,28 @@ +{ + "version": "0.37.0", + "description": "Terminal agent that shows what it may do, what it spends and what it refused", + "homepage": "https://github.com/initorigin/io-cli", + "license": "Apache-2.0", + "architecture": { + "64bit": { + "url": "https://github.com/initorigin/io-cli/releases/download/v0.37.0/io-0.37.0-x86_64-pc-windows-msvc.zip", + "hash": "a6011df7775bdcc4d3a8b5e026690c0b010ee42c7d018c595196cb323d21ce57", + "extract_dir": "io-0.37.0-x86_64-pc-windows-msvc" + } + }, + "bin": "io.exe", + "checkver": { + "github": "https://github.com/initorigin/io-cli" + }, + "autoupdate": { + "architecture": { + "64bit": { + "url": "https://github.com/initorigin/io-cli/releases/download/v$version/io-$version-x86_64-pc-windows-msvc.zip", + "extract_dir": "io-$version-x86_64-pc-windows-msvc", + "hash": { + "url": "https://github.com/initorigin/io-cli/releases/download/v$version/SHA256SUMS" + } + } + } + } +} diff --git a/docs/CONTRACT.md b/docs/CONTRACT.md index e0fd9bf..58fcb72 100644 --- a/docs/CONTRACT.md +++ b/docs/CONTRACT.md @@ -32,6 +32,7 @@ With no subcommand, `io` opens an interactive session. | `io config …` | Read and write configuration keys | | `io skill …` | Add, list and remove skills | | `io acp` | Serves the Agent Client Protocol on stdin and stdout, for an editor | +| `io upgrade` | Prints the command that updates this binary, for the way it was installed | `--plain` reaches an interactive session and stops there. `io exec` builds no theme, draws nothing and animates nothing already, so there is no second thing for the flag to switch off. @@ -72,17 +73,26 @@ produce that pause today at all** — it declines every approval rather than def session and headless alike — so the carve-out is what would be printed if it ever did, and is written down because the code that would print it exists and is reached by nothing. -**That sentence is still true of every door 0.36.0 ships, `io acp` included.** An ACP session is -the first surface with a person behind it that is not a terminal, so it is the first that *could* -raise an approval and be answered — but 0.36.0 does not raise one. An approval in an ACP session -is refused, and the client is told so by a `session/update` naming the tool call, its act and its -target, with `status: "failed"`. - -It is a notification and **not** a `session/request_permission` request. Raising a request means -waiting for the answer, and routing that answer back into the run is not wired; a request whose -reply is ignored would render a prompt in the editor whose outcome does nothing, which is worse -than not asking. The model is told the interface could not route the approval — not that the -operator denied it, because the operator was never asked. +**That sentence is true of `io exec`. It stopped being true of `io acp` in 0.38.0.** An ACP +session is the first surface with a person behind it that is not a terminal, so it is the first +that can raise an approval and be answered — and it does. An approval becomes a +`session/request_permission` request naming the tool call, its act and its target, offering three +options: allow once, allow for this session, and deny. + +The answer decides the call. `allow-session` remembers exactly that act on exactly that target +for the rest of the run, which is the same rule the terminal's own approval overlay writes — +both surfaces build it in one place, so they cannot come to mean different things by the same +word. An option id that was never offered, a cancellation, and a JSON-RPC error are all +denials: there is one safe direction to be wrong in. + +**A client that disconnects mid-question denies rather than hangs, and there is no timeout.** +When the reader stops, every outstanding request is abandoned and each waiter becomes a denial. +A timeout would have been a number invented here — a minute is too short for someone reading a +diff, and an hour is indistinguishable from a hang. The connection ending is the real event. + +Where nobody answered, the model is told the interface could not route the approval, never that +the operator denied it: the operator was not asked. Where the operator *was* asked and said no, +the model is told they refused, because that is what happened. `io exec` is unchanged and declines every approval for its own reason: an unattended run has nobody to ask, and an approver that blocked there would hang forever. diff --git a/docs/RELEASE_PROCESS.md b/docs/RELEASE_PROCESS.md index 746262e..474adce 100644 --- a/docs/RELEASE_PROCESS.md +++ b/docs/RELEASE_PROCESS.md @@ -1,10 +1,14 @@ # Release process — IO CLI There is no registry. `publish = false` in `Cargo.toml`, and the GitHub Release — four -cross-compiled binaries plus `SHA256SUMS` — is the entire distribution channel. `install.sh` and -`install.ps1` read it. So "released" here means the Release exists and its assets verify, not +cross-compiled binaries plus `SHA256SUMS` — is the entire distribution channel. `install.sh`, +`install.ps1`, the Homebrew formula at `Formula/io.rb` and the scoop manifest at `bucket/io.json` +all read that one Release. So "released" here means the Release exists and its assets verify, not that anything was uploaded to a package index. +The formula and the manifest are generated from a Release rather than written by hand, and +therefore updated **after** it is cut — step 10 below. + ## Versioning The product stays on `0.x` until its owner says otherwise. Within that: @@ -44,6 +48,20 @@ irreversible in front of something reviewable. `SHA256SUMS`, run it, and exercise the release's own headline surface — not just `--version`. 0.30.1 exists because 0.30.0's smoke checked the version string and nothing else, over a subcommand that did not exist. +10. **Point the tap and the bucket at the new Release.** `scripts/update-tap.sh ` reads + that Release's own `SHA256SUMS` and rewrites `Formula/io.rb` and `bucket/io.json`; open a pull + request with the result and merge it. Until it lands, `brew install io` and `scoop install io` + serve the previous version — the two files are a function of a **published** Release, so they + cannot be updated in the release commit itself: the artifacts they name do not exist yet. + + **The workflow does not do this**, and cannot. `main` and `develop` both carry a + `pull_request` ruleset whose only bypass actor is `OrganizationAdmin`, and GitHub Actions may + not create pull requests here, so an automated tap update would need a granted ruleset bypass + or a stored admin credential on a public repository. Neither is worth one merge click per + release. + + `brew` reads a tap's **default branch**, which is `develop`, so the tap is live when that pull + request merges and needs no `main` merge of its own. ## What the workflow builds diff --git a/docs/guide/accounting.md b/docs/guide/accounting.md index 036e246..71e0736 100644 --- a/docs/guide/accounting.md +++ b/docs/guide/accounting.md @@ -30,6 +30,15 @@ that and each is on the page rather than in this file: says so** — in that word, with the number of calls it applies to, and on the row itself in the grouped sections so a reader scanning for the largest figure can see which of them is incomplete without reading past the list. +- **A wire that reports no cache-write counter makes the cache-write figure a + floor too, and since 0.38.0 the grouped rows say that as well.** They stated + the money floor and stayed silent about this one, while the totals above them + had named both since 0.36.0 — so a per-model row could show a cache-write + number that was a floor and read as a fact. A row whose group is partly + unreported says `cache written N or more`; one whose group reported nothing + says `cache written unknown`, in the same words the total uses, because two + surfaces on one page inventing two vocabularies for one fact is worse than + either wording alone. `/stats` is the other half: runs by outcome, runs by day, the first-try counts, gate failures by phase, recovery, the slowest calls and the time to first token diff --git a/docs/guide/editors.md b/docs/guide/editors.md index e057f5d..3326062 100644 --- a/docs/guide/editors.md +++ b/docs/guide/editors.md @@ -54,25 +54,35 @@ see [When a run stops for you](resume.md). ## Permissions -**In 0.36.0 an editor session refuses every action the policy puts in the grey tier, and does not -ask you about it.** The tool call's update says the action needed an approval this agent could not -route, and the agent is told the same — not that you denied it, because you were never asked. - -io does not send `session/request_permission`. Raising that request means waiting for your answer, -and routing the answer back into the running turn is the piece 0.36.0 does not have; a prompt in -your editor whose outcome is ignored would be worse than no prompt. - -So configure the posture you want rather than relying on being asked — see +**An editor session asks you about every action the policy puts in the grey tier.** io sends +`session/request_permission` naming the tool call, its act and its target, and your answer decides +the call. Through 0.37.0 it asked nobody and refused them all; that is what 0.38.0 changed. + +The request carries three options — allow once, allow for this session, deny — and not ACP's +four. There is no *reject always*, because io-harness records a remembered **approval** and has +nowhere to record a remembered refusal: a later matching action would ask again, and an option +that quietly means something narrower than its name is worse than one that is absent. + +*Allow for this session* remembers exactly that act on exactly that target for the rest of the +run — not the act on its own, which would allow every write once you had allowed one. It is the +same rule the terminal's own approval overlay writes, from one shared function, so the two +surfaces cannot come to mean different things by the same word. + +**If your client never answers, the action is denied rather than left hanging.** There is no +timer: when the connection ends, every question still outstanding becomes a denial. A timeout +would have been a number invented here — a minute is too short for someone reading a diff, and an +hour is indistinguishable from a hang. An option id io never offered, a cancellation, and a +protocol error are denials too; there is one safe direction to be wrong in. + +Where nobody answered, the agent is told the interface could not route the approval, never that +you denied it. Where you were asked and said no, it is told you refused, because that is what +happened. + +You can still configure the posture you want rather than being asked at all — see [Configuration](configuration.md). A `workspace` posture lets the agent write inside the workspace without an approval; `read-only` refuses writes outright. Nothing is granted that you did not grant, and nothing happens silently. -When the request does land, it will carry three options — allow once, allow for this session, deny -— and not ACP's four. There is no *reject always*, because io-harness records a remembered -**approval** and has nowhere to record a remembered refusal: a later matching action would ask -again, and an option that quietly means something narrower than its name is worse than one that is -absent. - ## Cancelling `session/cancel` is served, and in 0.36.0 it takes effect **between** turns rather than during diff --git a/scripts/update-tap.sh b/scripts/update-tap.sh new file mode 100755 index 0000000..d2b91ab --- /dev/null +++ b/scripts/update-tap.sh @@ -0,0 +1,177 @@ +#!/bin/sh +# Point Formula/io.rb and bucket/io.json at a released version. +# +# scripts/update-tap.sh 0.38.0 +# +# Run it after the Release is cut, then open an ordinary pull request with the +# two files it changed. The release workflow cannot do this itself: `main` and +# `develop` both carry a `pull_request` ruleset whose only bypass actor is +# `OrganizationAdmin`, so a push from Actions is refused and Actions may not open +# the pull request either. +# +# It fetches that Release's SHA256SUMS, refuses anything it cannot read as four +# 64-character lowercase digests for the four artifacts the workflow builds, and +# only then rewrites the version and the checksums in those two files. The +# refusal is the point: a formula that names a digest for an artifact nobody +# uploaded installs nothing, and it fails on the user's machine rather than here. +# +# Every comparison is guarded by `[ -n "$want" ]` first. Two absent values +# compare equal, and this repository has printed a success line over exactly that +# — an empty digest matching an empty digest is not a match, it is a missing file. +# +# Running it twice over the same Release changes no byte the second time, so +# `git diff` after a run shows the release and nothing else. +# +# Environment: +# IO_BASE_URL fetch SHA256SUMS from here instead of the GitHub Release. Only +# the fetch moves: the URLs written into the two files are always +# the real Release ones, whatever this is set to. + +set -eu + +REPO="initorigin/io-cli" +DOWNLOADS="https://github.com/$REPO/releases/download" + +MAC_ARM="aarch64-apple-darwin" +MAC_INTEL="x86_64-apple-darwin" +LINUX="x86_64-unknown-linux-musl" +WINDOWS="x86_64-pc-windows-msvc" + +die() { printf 'update-tap: %s\n' "$*" >&2; exit 1; } + +[ $# -eq 1 ] || die "usage: scripts/update-tap.sh " +version="$1" +case "$version" in + ""|*[!0-9.]*|.*|*.) die "'$version' is not a version number (expected e.g. 0.38.0)" ;; +esac + +root=$(cd "$(dirname "$0")/.." && pwd) +formula="$root/Formula/io.rb" +manifest="$root/bucket/io.json" +[ -f "$formula" ] || die "no such file: $formula" +[ -f "$manifest" ] || die "no such file: $manifest" + +if command -v curl >/dev/null 2>&1; then + fetch() { curl -fsSL "$1" -o "$2"; } +elif command -v wget >/dev/null 2>&1; then + fetch() { wget -qO "$2" "$1"; } +else + die "this script needs curl or wget and neither is on PATH" +fi + +base="${IO_BASE_URL:-$DOWNLOADS/v$version}" + +work=$(mktemp -d 2>/dev/null || mktemp -d -t io-update-tap) || + die "could not make a temporary directory" +cleanup() { rm -rf "$work"; } +trap cleanup EXIT INT TERM + +sums="$work/SHA256SUMS" +fetch "$base/SHA256SUMS" "$sums" || die "could not download $base/SHA256SUMS" +grep -q '[^[:space:]]' "$sums" || die "$base/SHA256SUMS is empty; nothing was changed" + +# The digest for one artifact, or nothing. `$2 == name` rather than a substring +# match, so a line for a different artifact cannot answer for this one. +digest_for() { + awk -v name="$1" '$2 == name { print $1; exit }' "$sums" +} + +# One artifact's digest, or a refusal naming the artifact. Absence and a +# malformed value are the same answer here — neither is something to write into a +# file an installer trusts. +want() { + _archive="io-$version-$1.$2" + _digest=$(digest_for "$_archive") + [ -n "$_digest" ] || die "SHA256SUMS does not mention $_archive; nothing was changed" + # The character set is written out rather than as `[!0-9a-f]`. A range in a + # bracket expression is resolved by the locale's collating order, and in the + # common UTF-8 locales `a-f` spans `aAbBcCdDeEfF` — so the range spelling + # accepts an UPPERCASE digest, which is the one shape a case-sensitive + # comparison against sha256sum's output would then never match. + case "$_digest" in + *[!0123456789abcdef]*) die "the digest for $_archive is not lowercase hex: $_digest" ;; + esac + [ ${#_digest} -eq 64 ] || + die "the digest for $_archive is ${#_digest} characters, not 64: $_digest" + printf '%s\n' "$_digest" +} + +mac_arm=$(want "$MAC_ARM" tar.gz) +mac_intel=$(want "$MAC_INTEL" tar.gz) +linux=$(want "$LINUX" tar.gz) +windows=$(want "$WINDOWS" zip) + +# Nothing is written until all four have been read, so a Release missing one +# artifact leaves both files exactly as they were. +# +# Both rewrites replace the quoted VALUE on a line and leave the rest of the line +# alone, which is what keeps indentation, trailing commas and every untouched key +# byte-identical across runs. +rewrite() { + _file="$1" + shift + awk "$@" "$_file" > "$work/out" || die "could not rewrite $_file" + cat "$work/out" > "$_file" +} + +rewrite "$formula" \ + -v v="$version" -v downloads="$DOWNLOADS" \ + -v arm="$MAC_ARM" -v intel="$MAC_INTEL" -v musl="$LINUX" \ + -v h_arm="$mac_arm" -v h_intel="$mac_intel" -v h_musl="$linux" ' + BEGIN { sha[arm] = h_arm; sha[intel] = h_intel; sha[musl] = h_musl } + # No version rule: the formula declares no `version` stanza, because + # Homebrew scans it out of the url and `brew audit --strict` refuses a + # second declaration of it. Rewriting the urls below is therefore what + # moves the formula to a new version. + /^[[:space:]]*url "/ { + target = "" + if (index($0, arm)) target = arm + else if (index($0, intel)) target = intel + else if (index($0, musl)) target = musl + if (target == "") { + print "update-tap: this url names no known target: " $0 > "/dev/stderr" + exit 1 + } + sub(/"[^"]*"/, + "\"" downloads "/v" v "/io-" v "-" target ".tar.gz\"") + pending = target + print + next + } + /^[[:space:]]*sha256 "/ { + if (pending == "") { + print "update-tap: a sha256 with no url above it: " $0 > "/dev/stderr" + exit 1 + } + sub(/"[^"]*"/, "\"" sha[pending] "\"") + pending = "" + print + next + } + { print } +' + +# The autoupdate block is a template for the version AFTER this one, so its +# `$version` placeholders are left alone. A line carrying one is copied through +# untouched — substituting it would turn the template into a second copy of the +# concrete URL and scoop would stop being able to follow the next Release. +rewrite "$manifest" \ + -v v="$version" -v downloads="$DOWNLOADS" -v target="$WINDOWS" -v hash="$windows" ' + index($0, "$version") { print; next } + /"version":/ { sub(/: "[^"]*"/, ": \"" v "\""); print; next } + /"url": "/ { + sub(/: "[^"]*"/, + ": \"" downloads "/v" v "/io-" v "-" target ".zip\"") + print + next + } + /"hash": "/ { sub(/: "[^"]*"/, ": \"" hash "\""); print; next } + /"extract_dir": "/ { sub(/: "[^"]*"/, ": \"io-" v "-" target "\""); print; next } + { print } +' + +printf '%s\n' "Formula/io.rb and bucket/io.json now name $version:" +printf ' %s %s\n' "$mac_arm" "io-$version-$MAC_ARM.tar.gz" +printf ' %s %s\n' "$mac_intel" "io-$version-$MAC_INTEL.tar.gz" +printf ' %s %s\n' "$linux" "io-$version-$LINUX.tar.gz" +printf ' %s %s\n' "$windows" "io-$version-$WINDOWS.zip" diff --git a/src/acp.rs b/src/acp.rs index 845c69a..5eacfa5 100644 --- a/src/acp.rs +++ b/src/acp.rs @@ -159,12 +159,16 @@ pub enum Incoming { }, /// An answer to something **this** adapter asked the client. /// - /// A frame with an id and no method. **0.36.0 makes no request, so nothing - /// arrives here in practice** — the shape is decoded rather than refused - /// because a transport that answered a well-formed response with an error - /// would have to be changed before a request could ever be sent, and because - /// a conforming client may answer a request a later release adds while an - /// older `io` is still on the path. + /// A frame with an id and no method. **Since 0.38.0 this is the ordinary way + /// an approval is answered**, and it is handled by the read task inside + /// [`serve_with`] rather than by a [`Handler`]: settling a waiter needs no + /// dispatch and no `&mut` anything, and doing it on the reader is what lets an + /// answer be read while the turn that asked for it is still waiting. + /// + /// Through 0.37.0 nothing arrived here, because nothing was ever asked. The + /// shape was decoded rather than refused even then — a transport that answered + /// a well-formed response with an error would have had to change before a + /// request could ever be sent. /// /// `outcome` carries the client's `error` object as an `Err`, so a client that /// refuses the request is distinguishable from one that answered it — the run @@ -196,6 +200,93 @@ impl Incoming { } } +/// The requests this adapter has asked the client and is still waiting on. +/// +/// **This is the piece 0.36.0 was missing.** The frame builders were written, +/// tested and shipped with no production caller, and the module's own doc said +/// the missing piece was the correlation rather than the design. It is: a +/// request needs an id nobody else will use, somewhere to park the waiting side +/// of it, and a reader that can settle it *while the turn that raised it is +/// still running*. +/// +/// The last of those is why [`serve_with`] reads on its own task. Awaiting the +/// handler inline in the read loop — which is what 0.36.0 did — means the answer +/// to a question that turn asked sits unread in the pipe until the turn that is +/// waiting for it returns. That is not a slow path, it is a deadlock, and it is +/// the same shape as the writer starvation 0.36.0 shipped one layer up in this +/// same function. +/// +/// A `std::sync::Mutex` and not tokio's: the lock is taken to insert or remove +/// one map entry and is never held across an `await`, which is the only thing +/// the async mutex would buy. +#[derive(Debug, Default)] +pub struct Correlator { + next: std::sync::atomic::AtomicI64, + waiting: std::sync::Mutex< + std::collections::HashMap>>, + >, +} + +impl Correlator { + /// A correlator with nothing outstanding. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + /// Claim an id and the receiving half of its answer. + /// + /// Ids ascend from 1 and are never reused within a process. A client that + /// answers the same id twice settles the first and is dropped by the second, + /// because the sender is taken out of the map when it is used. + #[must_use] + pub fn issue(&self) -> (i64, tokio::sync::oneshot::Receiver>) { + let id = self + .next + .fetch_add(1, std::sync::atomic::Ordering::Relaxed) + .saturating_add(1); + let (tx, rx) = tokio::sync::oneshot::channel(); + if let Ok(mut waiting) = self.waiting.lock() { + waiting.insert(id, tx); + } + (id, rx) + } + + /// Hand a client's answer to whoever is waiting for it. + /// + /// A response naming an id nobody is waiting for is dropped. That is the + /// correct answer and not a gap: the protocol has no response to a response, + /// so there is nothing to send back, and a client that answers twice or + /// invents an id must not be able to make this adapter say anything. + pub fn settle(&self, id: &Value, outcome: Result) { + let Some(id) = id.as_i64() else { + return; + }; + let waiting = self.waiting.lock().ok().and_then(|mut map| map.remove(&id)); + if let Some(sender) = waiting { + let _ = sender.send(outcome); + } + } + + /// Give up on every outstanding request. + /// + /// Called when the reader stops, which is the only way a request that was + /// sent can come to have no answer coming. Dropping each sender makes every + /// waiter's `recv` fail, and the caller turns that into a denial — **so a + /// client that disconnects mid-approval denies rather than hangs, and this + /// adapter needs no timeout to guarantee it.** + /// + /// A timeout was the obvious alternative and it would have been a number + /// invented here. An approval is answered by a person: a minute is too short + /// for someone reading a diff and an hour is indistinguishable from a hang. + /// The connection ending is the real event, and it is observable. + pub fn abandon(&self) { + if let Ok(mut waiting) = self.waiting.lock() { + waiting.clear(); + } + } +} + /// A frame being sent. #[derive(Debug, Clone, PartialEq)] pub enum Outgoing { @@ -212,6 +303,18 @@ pub enum Outgoing { method: String, params: Value, }, + /// Something **this** adapter is asking the client, which it will wait for an + /// answer to. + /// + /// The id is allocated by [`Correlator`] and is a number, deliberately: the + /// client's ids are its own and may be anything JSON-RPC permits, so an + /// adapter that reused their shape could collide with one. Two id spaces + /// travel in opposite directions on one pipe and only one of them is ours. + Request { + id: Value, + method: String, + params: Value, + }, } impl Outgoing { @@ -339,6 +442,12 @@ pub fn encode(frame: &Outgoing) -> String { "method": method, "params": params, }), + Outgoing::Request { id, method, params } => serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "method": method, + "params": params, + }), }; // A serialization failure on a value this module built is not reachable — @@ -458,12 +567,22 @@ pub trait Handler { /// it is waiting for, so a buffered answer is a hang. pub async fn serve(reader: R, writer: W, handler: &mut H) -> std::io::Result<()> where - R: tokio::io::AsyncBufRead + Unpin, + R: tokio::io::AsyncBufRead + Unpin + Send + 'static, W: tokio::io::AsyncWrite + Unpin + Send + 'static, H: Handler, { let (_unused, rx) = tokio::sync::mpsc::unbounded_channel(); - serve_with(reader, writer, handler, rx).await + // A correlator of its own, because this door asks the client nothing: a + // handler that raises no request never issues an id, so the map stays empty + // and abandoning it at end-of-input is a no-op. + serve_with( + reader, + writer, + handler, + rx, + std::sync::Arc::new(Correlator::new()), + ) + .await } /// [`serve`], plus a channel the agent writes frames on without being asked. @@ -483,9 +602,13 @@ pub async fn serve_with( mut writer: W, handler: &mut H, mut outbound: tokio::sync::mpsc::UnboundedReceiver, + correlator: std::sync::Arc, ) -> std::io::Result<()> where - R: tokio::io::AsyncBufRead + Unpin, + // `Send + 'static` for the same reason the writer needs it: the reader moves + // onto its own task too, so that a response can be read while the turn that + // is waiting for it runs. See the note on the read task below. + R: tokio::io::AsyncBufRead + Unpin + Send + 'static, // `Send + 'static` because the writer moves onto its own task. That bound is // the visible cost of the fix below and it is worth paying: a borrowed writer // is what forced the single-loop shape that could not stream. @@ -536,19 +659,65 @@ where } }); - let mut reader = reader; - let mut line = String::new(); + // **The reader runs on its own task, and that is why an approval can be + // answered at all.** The 0.36.0 shape awaited `handler.handle(...)` inline + // here, and for a `session/prompt` that call *is* the whole turn — so a + // `session/request_permission` raised by that turn could never be answered: + // the client's reply sat unread in the pipe until the turn returned, and the + // turn was waiting for the reply. Not a slow path, a deadlock, and exactly + // the shape of the writer starvation this same function shipped one layer up. + // + // Splitting it puts the settling of a response — which needs no handler and + // no `&mut` anything — on a task that keeps reading, while requests and + // notifications queue for the loop below to take one at a time. Queueing them + // is right rather than a limitation: ACP runs one turn per session, and a + // second prompt arriving mid-turn must not start a second one. + let (dispatch, mut arrivals) = + tokio::sync::mpsc::unbounded_channel::>(); + let settling = std::sync::Arc::clone(&correlator); + let read = tokio::spawn(async move { + let mut reader = reader; + let mut line = String::new(); + let outcome = loop { + line.clear(); + // Zero bytes is end of input: the client closed the pipe, which is + // how an ACP session ends normally. Not an error, and not answered. + match reader.read_line(&mut line).await { + Ok(0) => break Ok(()), + Ok(_) => {} + Err(error) => break Err(error), + } + + match decode(&line) { + // Settled here rather than forwarded, because this is the whole + // point of the split: a waiter parked inside a running turn is + // released without the loop below having to reach this line. + Ok(Incoming::Response { id, outcome }) => settling.settle(&id, outcome), + Ok(incoming) => { + if dispatch.send(Ok(incoming)).is_err() { + break Ok(()); + } + } + Err(refusal) => { + if dispatch.send(Err(refusal)).is_err() { + break Ok(()); + } + } + } + }; + // Every outstanding request now has no answer coming. Abandoning them + // turns each waiter into a denial instead of a hang — the reason this + // adapter needs no approval timeout. + settling.abandon(); + outcome + }); + let outcome = loop { - line.clear(); - // Zero bytes is end of input: the client closed the pipe, which is how an - // ACP session ends normally. Not an error, and not answered. - match reader.read_line(&mut line).await { - Ok(0) => break Ok(()), - Ok(_) => {} - Err(error) => break Err(error), - } + let Some(arrival) = arrivals.recv().await else { + break Ok(()); + }; - let answers = match decode(&line) { + let answers = match arrival { Ok(incoming) => handler.handle(incoming).await, Err(refusal) => vec![refusal], }; @@ -568,7 +737,17 @@ where drop(frames); forward.abort(); let _ = pump.await; - outcome + + // **The read task owns the real outcome.** The loop above ends when its + // channel closes, which happens because the reader stopped — so its own + // `Ok(())` says only "there is nothing more to serve" and would swallow the + // io error that caused it. A join failure is reported as `Ok(())` for the + // same reason end-of-input is: the session is over either way and there is no + // second thing to say about it. + match read.await { + Ok(from_reader) => from_reader, + Err(_) => outcome, + } } /// The options an ACP client is offered for one approval, as @@ -672,22 +851,45 @@ pub struct Reporter { session_id: String, updates: tokio::sync::mpsc::UnboundedSender, cancelled: std::sync::Arc, + /// The step the last observed event belonged to. + /// + /// **The approver needs this and has no other way to learn it.** A cell's id + /// on this wire is `{run_id}-{step}` ([`crate::acp_map`]), and an approval + /// request has to name the cell the client already has — but an + /// [`io_harness::Approver`] is handed a [`io_harness::Request`], which + /// carries the act and the target and no step at all. The observer is the + /// only thing in this module that sees one. + step: std::sync::Arc, +} + +/// What a turn's observer and its approver share. +/// +/// Two flags rather than a struct with methods, because each has exactly one +/// writer and one reader and the whole of the coupling is that they are the same +/// allocation. +pub struct Shared { + /// Set by `session/cancel`, read before each event is translated. + pub cancelled: std::sync::Arc, + /// Written by the observer on every event, read when an approval is raised. + pub step: std::sync::Arc, } impl Reporter { - /// A reporter for one session, and the handle that cancels it. + /// A reporter for one session, and the handles that steer and read it. pub fn new( session_id: impl Into, updates: tokio::sync::mpsc::UnboundedSender, - ) -> (Self, std::sync::Arc) { + ) -> (Self, Shared) { let cancelled = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let step = std::sync::Arc::new(std::sync::atomic::AtomicU32::new(0)); ( Self { session_id: session_id.into(), updates, cancelled: std::sync::Arc::clone(&cancelled), + step: std::sync::Arc::clone(&step), }, - cancelled, + Shared { cancelled, step }, ) } } @@ -702,6 +904,13 @@ impl io_harness::Observer for Reporter { return io_harness::Flow::Cancel; } + // Recorded before the translation and unconditionally, because an event + // this module draws nothing for still moved the run on — and the next + // approval must name the step the agent is actually on, not the last one + // that happened to render. + self.step + .store(event.step, std::sync::atomic::Ordering::Relaxed); + if let Some(update) = crate::acp_map::translate(event) { // A closed receiver means the session is gone. That is not a reason // to cancel the run here — the same argument `src/exec.rs` makes @@ -777,6 +986,10 @@ impl crate::provider::WithProvider for Editor { ) -> Self::Out { let provider = make(&model)?; let (outbound, rx) = tokio::sync::mpsc::unbounded_channel(); + // One correlator for the session, shared three ways: the read task + // settles into it, `Consulting` waits on it, and it is created here + // because both of those are built from this frame. + let correlator = std::sync::Arc::new(Correlator::new()); let mut handler = Editing { store: &self.store, session: &mut self.session, @@ -786,10 +999,11 @@ impl crate::provider::WithProvider for Editor { outbound, session_id: None, cancel: None, + correlator: std::sync::Arc::clone(&correlator), }; let stdin = tokio::io::BufReader::new(tokio::io::stdin()); - serve_with(stdin, tokio::io::stdout(), &mut handler, rx) + serve_with(stdin, tokio::io::stdout(), &mut handler, rx, correlator) .await .map_err(|error| error.to_string())?; Ok(0) @@ -808,6 +1022,10 @@ struct Editing<'a, P: io_harness::Provider> { /// 0.36.0 holds one and refuses a second rather than multiplexing badly. session_id: Option, cancel: Option>, + /// Where an approval this session raises will be answered. Handed to each + /// turn's [`Consulting`], and shared with the read task inside + /// [`serve_with`], which is the only thing that settles it. + correlator: std::sync::Arc, } impl Handler for Editing<'_, P> { @@ -888,8 +1106,8 @@ impl Editing<'_, P> { /// Drive one turn, streaming updates, and answer with its stop reason. async fn run(&mut self, session_id: &str, goal: String) -> &'static str { - let (reporter, cancel) = Reporter::new(session_id, self.outbound.clone()); - self.cancel = Some(cancel); + let (reporter, shared) = Reporter::new(session_id, self.outbound.clone()); + self.cancel = Some(std::sync::Arc::clone(&shared.cancelled)); let contract = crate::exec::contract(self.config, self.session, goal, None); let outcome = self @@ -903,7 +1121,8 @@ impl Editing<'_, P> { session_id: session_id.to_string(), outbound: self.outbound.clone(), run_id: self.session.id(), - step: std::sync::atomic::AtomicU32::new(0), + step: std::sync::Arc::clone(&shared.step), + correlator: std::sync::Arc::clone(&self.correlator), }, &reporter, ) @@ -968,9 +1187,20 @@ pub fn prompt_text(params: &Value) -> Option { struct Consulting { session_id: String, outbound: tokio::sync::mpsc::UnboundedSender, - /// The run this is approving for, so the refusal lands on the right cell. + /// The run this is approving for, so the request lands on the right cell. run_id: i64, - step: std::sync::atomic::AtomicU32, + /// The step the run is on, written by [`Reporter`] as it observes. + /// + /// **Shared with the observer rather than owned here, and that is a fix.** + /// It was an `AtomicU32` this struct constructed at zero and only ever read — + /// never stored — so every request named cell `{run_id}-0` whatever step + /// raised it. Through 0.37.0 that mis-addressed a failure notice; from + /// 0.38.0 it would mis-address the dialog an operator acts on, attaching the + /// question to a call that already finished. + step: std::sync::Arc, + /// Where the answer will come back. Shared with the read task, which is the + /// only thing that can settle it. + correlator: std::sync::Arc, } impl io_harness::Approver for Consulting { @@ -978,10 +1208,12 @@ impl io_harness::Approver for Consulting { &'a self, request: &'a io_harness::Request, ) -> io_harness::approve::DecisionFuture<'a> { - let act = format!("{:?}", request.act).to_lowercase(); + let act_name = format!("{:?}", request.act).to_lowercase(); + let act = request.act; let target = request.target.clone(); let session_id = self.session_id.clone(); let outbound = self.outbound.clone(); + let correlator = std::sync::Arc::clone(&self.correlator); // **The id has to be the one the client already has.** An earlier draft // put the target *path* here, so the update named a cell that had never // been announced and a conforming client had nothing to apply it to — @@ -994,31 +1226,78 @@ impl io_harness::Approver for Consulting { self.step.load(std::sync::atomic::Ordering::Relaxed) ); Box::pin(async move { - let _ = outbound.send(Outgoing::Notification { - method: "session/update".into(), - params: json!({ - "sessionId": session_id, - "update": { - "sessionUpdate": "tool_call_update", - "toolCallId": id, - "status": "failed", - "title": format!( - "{act} {target} needs an approval this agent cannot yet route" - ), - } - }), - }); - io_harness::Decision::deny(NOT_ROUTED) + ask_permission( + &correlator, + &outbound, + &session_id, + act, + &act_name, + &target, + &id, + ) + .await }) } } +/// Ask the client for one permission, and wait for the answer. +/// +/// **A free function and not a method, so a test can assert what it returns.** +/// The criterion this satisfies is about the `Decision` the run receives, and +/// the run receives it from an `Approver` that needs a whole session to build. +/// 0.36.0 shipped the frame builders with no production caller and gates that +/// asserted the parts against each other; a test that watched a frame go out +/// would repeat that mistake one level up. This is the thing the decision comes +/// out of, and it is callable with a correlator and a channel. +/// +/// **Every failure is a denial, and there is exactly one safe direction to be +/// wrong in here.** A closed writer means the question was never asked. A +/// dropped sender means the reader stopped with this request outstanding — a +/// disconnected client, which [`Correlator::abandon`] turns into a denial rather +/// than a hang, and which is why this adapter needs no approval timeout. A +/// client that answers with a JSON-RPC error has reported a protocol failure, +/// which must not be read as a decision about a permission. +pub async fn ask_permission( + correlator: &Correlator, + outbound: &tokio::sync::mpsc::UnboundedSender, + session_id: &str, + act: io_harness::Act, + act_name: &str, + target: &str, + tool_call_id: &str, +) -> io_harness::Decision { + let (request_id, answer) = correlator.issue(); + if outbound + .send(Outgoing::Request { + id: json!(request_id), + method: "session/request_permission".into(), + params: permission_params(session_id, act_name, target, tool_call_id), + }) + .is_err() + { + return io_harness::Decision::deny(NOT_ROUTED); + } + + match answer.await { + Ok(Ok(result)) => crate::approval::decision(permission_answer(&result), act, target), + Ok(Err(_)) | Err(_) => io_harness::Decision::deny(NOT_ROUTED), + } +} + /// What the model is told when an ACP approval could not be routed. /// /// Deliberately **not** `approval::REFUSED_BY_OPERATOR`. That sentence says the -/// operator denied it, and in 0.36.0 the operator was never asked — telling the -/// model otherwise puts a false statement about a human into the transcript the -/// agent then reasons from. +/// operator denied it, and every branch that reaches this one is a branch where +/// nobody answered: the writer was gone before the question was sent, the reader +/// stopped with the question outstanding, or the client replied with a protocol +/// error rather than a decision. Telling the model a human refused would put a +/// false statement about a person into the transcript the agent then reasons +/// from. +/// +/// **An operator who was asked and said no gets `REFUSED_BY_OPERATOR`**, from +/// `approval::decision`, because that is what happened. Since 0.38.0 that is the +/// ordinary path and this constant is the exception; through 0.37.0 it was the +/// only path there was. pub const NOT_ROUTED: &str = "this interface could not route the approval, so it was refused"; /// The `initialize` result. diff --git a/src/acp_map.rs b/src/acp_map.rs index 6925529..a23c2e4 100644 --- a/src/acp_map.rs +++ b/src/acp_map.rs @@ -438,10 +438,15 @@ pub fn tool_kind(name: &str) -> &'static str { /// The `stopReason` for a run that ended this way. /// /// ACP names five: `end_turn`, `max_tokens`, `max_turn_requests`, `refusal` and -/// `cancelled`. io-harness names seventeen outcomes, so this is a narrowing and +/// `cancelled`. io-harness names eighteen outcomes, so this is a narrowing and /// several outcomes share an answer — which is correct, because a client acts on /// the reason and only these five distinctions change what it does. /// +/// `SchemaUnsatisfied` joins the attempts bucket rather than `refusal`: a run +/// that could not produce the requested shape has exhausted the attempts it was +/// allowed, and telling a client it was refused would misreport the agent's +/// willingness the same way a pause would. +/// /// **A pause is `end_turn` and not `refusal`**, and that is the one mapping worth /// arguing. A run waiting on a question or a plan has not refused anything; it /// has stopped with work outstanding, and telling a client it was refused would @@ -456,6 +461,7 @@ pub fn stop_reason(outcome: &io_harness::RunOutcome) -> &'static str { O::Denied { .. } | O::Refused { .. } | O::PlanRejected { .. } => "refusal", O::StepCapReached { .. } | O::VerificationFailed { .. } + | O::SchemaUnsatisfied { .. } | O::Stalled { .. } | O::Escalated { .. } => "max_turn_requests", O::TimeBudgetExceeded { .. } @@ -489,7 +495,7 @@ pub fn translate(event: &RunEvent) -> Option { "sessionUpdate": "agent_thought_chunk", "content": { "type": "text", "text": text }, }), - EventKind::ToolCall { name, target } => json!({ + EventKind::ToolCall { name, target, .. } => json!({ "sessionUpdate": "tool_call", // The id is the step it happened on. A client needs a handle to // correlate a later `tool_call_update` with, and io-harness gives a diff --git a/src/adapt.rs b/src/adapt.rs index 926cf96..6bd7579 100644 --- a/src/adapt.rs +++ b/src/adapt.rs @@ -70,7 +70,7 @@ //! `self.root.join(d)` and `Path::join` of an absolute argument discards the base. //! io-harness 0.74.0 refuses that spelling for `skills` and for `templates` in //! **every** scope, `Scope::User` included -//! (`io-harness-0.76.0/src/plugin.rs:1097`), and the reason is the one this +//! (`io-harness-0.78.0/src/plugin.rs:1097`), and the reason is the one this //! product would give: every `*.md` under such a directory is read into the //! model's system prompt on every turn, so a manifest contributes a directory it //! *ships* rather than one it points at somewhere else on the machine. Copying is @@ -377,7 +377,7 @@ pub const SKILLS_DIR: &str = "skills"; /// io-harness calls the same thing a **template**, and the two formats are the /// same file: markdown, optional frontmatter, `$ARGUMENTS` for what the operator /// typed. `Templates::discover` takes every `*.md` in a directory -/// (`io-harness-0.76.0/src/template.rs:279`), so what is translated is the +/// (`io-harness-0.78.0/src/template.rs:279`), so what is translated is the /// directory and never the files in it — the bytes are copied verbatim into /// [`TEMPLATES_DIR`] and nothing in them is rewritten. pub const COMMANDS_DIR: &str = "commands"; @@ -434,11 +434,16 @@ pub const MCP_FILE: &str = ".mcp.json"; /// other `${...}` is refused by name; see [`expanded`]. pub const PLUGIN_ROOT: &str = "${CLAUDE_PLUGIN_ROOT}"; -/// One generated manifest: where it is, what io-harness reads out of it, and what -/// the bundle carried that it does not. +/// One generated manifest: where it goes, what io-harness reads out of it, and +/// what the bundle carried that it does not. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Adapter { - /// The `plugin.toml` that was written. + /// The `plugin.toml` this adapter becomes, at the destination + /// [`Staged::commit`] puts it at. + /// + /// **Not a file that exists yet.** [`generate`] leaves the adapter staged, so + /// until the caller commits, the bytes are under [`Staged::staging`] and this + /// path either holds the adapter of the *previous* install or holds nothing. pub manifest: PathBuf, /// What the bundle contributes, in io-harness's own words and its own fixed /// order. @@ -463,6 +468,77 @@ pub struct Adapter { /// their edit inside the clone changed nothing is owed the sentence that says /// so. Which surface says it, and in what words, is that surface's business. pub copied: Vec, + /// The built adapter, beside its destination and not yet in it. + pub staged: Staged, +} + +/// An adapter built in a staging directory, waiting to be put in place or thrown +/// away. +/// +/// **The swap is the caller's, because the consent is the caller's.** Through +/// 0.37.0 [`generate`] ended by replacing the destination itself, which was +/// invisible while an install was the only thing that ran it. Since 0.35.0 +/// installing again *is* the update path, so that swap happened before the +/// operator had answered: declining a refresh left the new adapter in place over +/// the one a `[[plugin]]` entry already named, and `docs/guide/plugins.md` +/// promised the opposite in writing. The bytes now stay in the staging directory +/// until somebody who holds the answer calls [`Staged::commit`], and the other +/// answer is [`Staged::discard`]. +/// +/// Exactly one of the two runs on every path — the type carries no flag saying +/// which, because a flag would be a third state nothing checks. What is left +/// behind if neither runs is one directory named for this process, which is the +/// same window `fetch::clone_at` leaves and for the same reason. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Staged { + /// Where the built adapter is now: a sibling of [`Staged::into`], hidden and + /// named for it and for this process, so two `io` processes adapting one + /// bundle cannot stage into each other. + pub staging: PathBuf, + /// Where it goes when the operator consents. Untouched until then, on a first + /// install and on an update alike. + pub into: PathBuf, +} + +impl Staged { + /// Put the built adapter in place, replacing whatever is there. + /// + /// **Wiped and renamed rather than written over.** An adapter is rebuilt on + /// every install and since 0.35.0 it holds copies of the bundle's markdown and + /// not only a manifest — so a file the bundle's author deleted, or a whole + /// directory the new manifest no longer names, would otherwise survive in io's + /// home and go on being read into the model's prompt on every turn. + /// [`Staged::into`] is a directory this module owns by the layout [`at`] + /// states and nothing an operator wrote is in it. + /// + /// The window this leaves is one rename wide — a process that dies between the + /// remove and the rename loses the adapter — where before 0.35.0 it was the + /// whole of `generate` including every refusal. It is not closed entirely + /// because closing it means a third path to restore on failure, and + /// `plugin add` is the one command that puts it all back. + /// + /// # Errors + /// + /// A string naming the destination and what the rename said. The staging + /// directory is removed on that path too, so a failed commit leaves nothing + /// beside the adapter for the marketplace listing to walk. + pub fn commit(&self) -> Result<(), String> { + let _ = std::fs::remove_dir_all(&self.into); + std::fs::rename(&self.staging, &self.into).map_err(|error| { + let _ = std::fs::remove_dir_all(&self.staging); + format!("{}: {error}", self.into.display()) + }) + } + + /// Take the built adapter back off the disk, because it is not wanted. + /// + /// Best effort by design: nothing was declared and [`Staged::into`] was never + /// touched, so a staging directory that cannot be removed is a stale cache + /// rather than anything the operator consented to, and a refusal reported + /// *after* a refusal is noise about io's own housekeeping. + pub fn discard(&self) { + let _ = std::fs::remove_dir_all(&self.staging); + } } /// The adapter directory for one bundle, under an adapters root. @@ -481,14 +557,19 @@ pub fn at(root: &Path, owner: &str, repo: &str, name: &str) -> PathBuf { root.join(owner).join(repo).join(name) } -/// Write the `plugin.toml` that makes `bundle` loadable, and report what it says. +/// Build the `plugin.toml` that makes `bundle` loadable, and report what it says. /// /// `bundle` is a directory inside a clone and is **only read**; `into` is where /// the manifest goes, and it is regenerated rather than edited, so an adapter that -/// is already there is replaced. `name` is the id the manifest declares and the -/// word every name the bundle contributes is namespaced by — it goes through +/// is already there is replaced whole. `name` is the id the manifest declares and +/// the word every name the bundle contributes is namespaced by — it goes through /// [`normalised`], which refuses far more than it maps and says why. /// +/// **`into` is not written here.** What comes back is [`Adapter::staged`], the +/// built adapter sitting beside its destination; the caller holding the operator's +/// answer calls [`Staged::commit`] or [`Staged::discard`]. See [`Staged`] for what +/// that ordering costs and what it buys. +/// /// # The four kinds, and the one rule that decides the paths /// /// `skills/` becomes `skills`, `commands/` becomes `templates`, each @@ -497,7 +578,7 @@ pub fn at(root: &Path, owner: &str, repo: &str, name: &str) -> PathBuf { /// /// **The two directories are copied into `into` and named relative to it.** /// io-harness 0.74.0 refuses an absolute or a `..`-carrying `skills` or -/// `templates` in every scope (`io-harness-0.76.0/src/plugin.rs:1097`), because +/// `templates` in every scope (`io-harness-0.78.0/src/plugin.rs:1097`), because /// every `*.md` under one is read into the model's system prompt — so a bundle /// contributes a directory it ships. An adapter comes to ship one by copying it, /// and [`Adapter::copied`] reports which moved. The copy refuses a symbolic link @@ -531,8 +612,9 @@ pub fn at(root: &Path, owner: &str, repo: &str, name: &str) -> PathBuf { /// io-harness does not speak, a manifest io-harness itself refuses, and a /// contributed directory io will not copy — a symbolic link, an entry that is /// neither a plain file nor a directory, or one past [`MOST_BYTES`] or -/// [`MOST_ENTRIES`]. **`into` is removed on every one of them**, so a half-copied -/// adapter is never left where an operator could go on to declare it. +/// [`MOST_ENTRIES`]. **The staging directory is removed on every one of them**, so +/// a half-copied adapter is never left where an operator could go on to declare +/// it, and `into` is exactly as it was found on every one of them too. pub fn generate(bundle: &Path, name: &str, into: &Path) -> Result { let id = normalised(name).ok_or_else(|| { format!( @@ -632,7 +714,7 @@ pub fn generate(bundle: &Path, name: &str, into: &Path) -> Result Result Result Result Option { /// adapted, so expanding it reads nothing of the host — it writes down a path io /// already knew. `${env:…}`, `${file:…}`, `${cmd:…}` and every shell variable a /// bundle writes are the opposite: io-harness refuses each of them inside a -/// manifest in every scope (`io-harness-0.76.0/src/plugin.rs:977`), because a +/// manifest in every scope (`io-harness-0.78.0/src/plugin.rs:977`), because a /// bundle is a third party's directory even when the file naming it is the /// operator's own. /// @@ -1184,7 +1252,7 @@ fn unquoted(said: &str) -> &str { /// /// **A transport io-harness does not speak is a refusal naming the server**, not /// a server quietly left out. io-harness speaks stdio and streamable HTTP -/// (`io-harness-0.76.0/src/mcp.rs:301`), so a bundle declaring `sse` is declaring +/// (`io-harness-0.78.0/src/mcp.rs:301`), so a bundle declaring `sse` is declaring /// a capability no adapter can deliver — and a manifest carrying one fewer server /// than the bundle does is exactly the silent absence this module's docs exist to /// end. The whole generation stops, because a bundle is installed for what it diff --git a/src/app.rs b/src/app.rs index b187476..a6bf4bc 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2626,7 +2626,7 @@ fn gate_said( /// `session.rs:1125-1127` turns classification on whenever the contract carries /// `Verification::None`, which `TaskContract::workspace` does — so a greeting has /// always come back after one completion with **no steps row, no gate attempt, no -/// checkpoint, no snapshot and no tool loop** (`run/step.rs:312-320`). What reached +/// checkpoint, no snapshot and no tool loop** (`run/step.rs:362-370`). What reached /// the operator was silence, because every line this product draws about a turn is /// drawn from events a conversational turn does not emit. /// @@ -2931,6 +2931,7 @@ mod tests { EventKind::ToolCall { name: io_harness::tools::GIT_BRANCH_TOOL.to_string(), target: "feat/0.25.0".to_string(), + origin: None, }, ), Duration::ZERO, @@ -2950,6 +2951,7 @@ mod tests { EventKind::ToolCall { name: io_harness::tools::GIT_BRANCH_TOOL.to_string(), target: io_harness::tools::GIT_BRANCH_TOOL.to_string(), + origin: None, }, ), Duration::ZERO, diff --git a/src/approval.rs b/src/approval.rs index f88d789..5f481bd 100644 --- a/src/approval.rs +++ b/src/approval.rs @@ -293,21 +293,11 @@ impl Approval { /// Answer it, and let the run go on. pub fn answer(self, answer: Answer) { - let decision = match answer { - Answer::Once => Decision::approve(), - Answer::Session => Decision::Approve { - modified: None, - // The harness applies this for the rest of the *run*, which is one - // turn. Carrying it into the next turn is the caller's job and is - // what F5 asserts; see `remembered`. - remember: vec![self.remembered()], - }, - Answer::Deny => Decision::deny(REFUSED_BY_OPERATOR), - }; + let decision = decision(answer, self.ask.act(), self.ask.target()); self.ask.answer(decision); } - /// The rule *allow this session* means. + /// The rule this approval's *allow this session* means. /// /// The narrowest thing a person means when they say yes to the same question /// twice: this act, on this target. Not the act alone, which would allow every @@ -680,6 +670,40 @@ fn fit_line(line: Line<'static>, width: usize, theme: &Theme) -> Line<'static> { Line::from(spans) } +/// What an answer means as an io-harness decision. +/// +/// **One place, because two surfaces answer approvals now.** The session's +/// overlay asks a person at a keyboard and `io acp` asks an editor's client; +/// both arrive here with the same three answers and must mean the same three +/// things by them. Left inside the overlay, "allow for this session" would have +/// been re-derived in `src/acp.rs` — and a second derivation that quietly +/// remembered a *wider* rule is a permission the operator did not grant, which +/// no test comparing either surface to itself could see. +/// +/// `remember` is applied by the harness for the rest of the **run**, which is one +/// turn. Carrying it into the next turn is the caller's job. +/// +/// The rule is the narrowest thing a person means when they say yes to the same +/// question twice: this act, on this target. Not the act alone, which would allow +/// every write; and the pattern is the target as written, because a bare filename +/// is matched against a basename too and would allow the same name anywhere in +/// the tree. +#[must_use] +pub fn decision(answer: Answer, act: Act, target: &str) -> Decision { + match answer { + Answer::Once => Decision::approve(), + Answer::Session => Decision::Approve { + modified: None, + remember: vec![Rule { + act, + effect: Effect::Allow, + pattern: target.to_string(), + }], + }, + Answer::Deny => Decision::deny(REFUSED_BY_OPERATOR), + } +} + pub fn act_word(act: Act) -> &'static str { match act { Act::Read => "read", diff --git a/src/attach.rs b/src/attach.rs index c378fad..6dd9991 100644 --- a/src/attach.rs +++ b/src/attach.rs @@ -272,7 +272,7 @@ pub fn viewed( graphics: crate::term::Graphics, width: u16, ) -> Option { - let EventKind::ToolCall { name, target } = &event.kind else { + let EventKind::ToolCall { name, target, .. } = &event.kind else { return None; }; if name != VIEW_IMAGE_TOOL { diff --git a/src/cli.rs b/src/cli.rs index f7f8a96..f3f1d02 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -94,10 +94,11 @@ pub enum Command { /// the protocol, so nothing else may be written there; diagnostics go to /// stderr. /// - /// An approval in an ACP session is refused and the client is told which - /// action and why: 0.36.0 does not raise `session/request_permission`, so - /// configure the posture rather than expecting to be asked. `io exec` refuses - /// approvals too, for its own reason — an unattended run has nobody to ask. + /// An approval in an ACP session is put to the client as + /// `session/request_permission`, and its answer decides the call. A client + /// that does not answer — one that disconnects mid-question — denies rather + /// than hangs. `io exec` refuses every approval instead, for its own reason: + /// an unattended run has nobody to ask. Acp, /// Install, list or remove a skill, without opening a session. /// @@ -109,6 +110,16 @@ pub enum Command { /// `tests/` links `src/main.rs` and nothing tested clap's routing, so the /// whole suite was green over a door that did not open. Skill(Manage), + /// Print the command that updates this binary, for the way it was + /// installed. + /// + /// **It prints and does not run.** io has no HTTP client and asks nothing + /// about what the latest version is: the answer is read from the running + /// binary's own path, so a Homebrew install is told `brew upgrade io`, a + /// scoop install `scoop update io`, and a script install the installer line. + /// A path none of the three would have produced is told so rather than given + /// advice that might replace a binary something else placed. + Upgrade, } /// The three management subcommands' arguments, handed through untouched. diff --git a/src/commands.rs b/src/commands.rs index 2800ace..8474ff9 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1489,7 +1489,7 @@ pub const READ_MARK: &str = "+"; /// The case this page exists for. A file that is there and is not read looks, /// from everywhere else in this product, exactly like one that is read: nothing /// warns, and a missing or skipped instruction file is passed over in silence -/// (`read_instructions`, `io-harness-0.76.0/src/config.rs:2799`). +/// (`read_instructions`, `io-harness-0.78.0/src/config.rs:2911`). pub const UNREAD_MARK: &str = "-"; /// What one row of the memory page stands for. @@ -1705,7 +1705,7 @@ pub fn memory_notes(view: &crate::recall::View, glyphs: &crate::glyphs::Glyphs) /// writing: a file that exists and is not read. It names what would make it /// read rather than only reporting the fact, because `[instructions] files` /// **replaces** the default list rather than adding to it -/// (`DEFAULT_INSTRUCTIONS`, `io-harness-0.76.0/src/config.rs:190`, read at +/// (`DEFAULT_INSTRUCTIONS`, `io-harness-0.78.0/src/config.rs:190`, read at /// `:2808`) and there is nothing on any /// other surface to pull on. pub fn instruction_said( diff --git a/src/configure.rs b/src/configure.rs index 896058a..6119e89 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -255,7 +255,7 @@ pub enum Kind { /// /// **Both halves are the dependency's since io-harness 0.71.0, and neither is /// written here any more**: the list is `Effect::ALL` -/// (`io-harness-0.76.0/src/policy.rs:129`) and each spelling is `Effect::as_str` +/// (`io-harness-0.78.0/src/policy.rs:129`) and each spelling is `Effect::as_str` /// (`:145`), which is the word io-harness's own deserializer reads. /// /// Until this release io-cli held a copy of both — an array naming three variants @@ -282,7 +282,7 @@ pub fn effects() -> Vec { /// The `ExecMode` variants, spelled by io-harness itself. /// -/// **The list is `ExecMode::ALL` (`io-harness-0.76.0/src/sandbox.rs:453`) and the +/// **The list is `ExecMode::ALL` (`io-harness-0.78.0/src/sandbox.rs:453`) and the /// spellings are `ExecMode::as_str` (`:460`).** io-cli wrote the variant list out /// by hand until this release for a reason that was the dependency's and not a /// choice made here: `ExecMode` is `#[non_exhaustive]` (`sandbox.rs:407`), and @@ -386,7 +386,7 @@ pub fn kind_of(key: &str) -> Option { /// preference — but half of the old reason is now false and the correction is /// worth writing down.** io-harness 0.71.0 names its own defaults: /// `DEFAULT_MAX_STEPS` = 8, `DEFAULT_WORKSPACE_MAX_STEPS` = 12 and -/// `DEFAULT_MAX_RETRIES` = 2 (`io-harness-0.76.0/src/contract.rs:673,691,707`), +/// `DEFAULT_MAX_RETRIES` = 2 (`io-harness-0.78.0/src/contract.rs:693,711,727`), /// re-exported at the crate root. "There is nothing to read" was true when this /// was written and is not true now. What is still true is that none of it anchors /// *this* ladder: @@ -537,7 +537,7 @@ pub fn shape_of(key: &str, config: &Config) -> Option { /// The models `[prices.models]` names, across every scope, sorted and deduplicated. /// /// **Read from the dependency's own table since io-harness 0.71.0, not scraped -/// out of the files.** `PriceTable::models` (`io-harness-0.76.0/src/pricing.rs:268`) +/// out of the files.** `PriceTable::models` (`io-harness-0.78.0/src/pricing.rs:268`) /// lists every model the table can actually price, and [`Config::prices`] has /// always built that table out of the three scopes — so the merged question this /// used to hand-roll is precisely the one the accessor answers, and the gap filed @@ -563,7 +563,7 @@ pub fn shape_of(key: &str, config: &Config) -> Option { /// /// **This takes the `Config` the caller already holds, and must never re-discover /// one.** `Config::discover` resolves every `${env:}`, `${file:}` and `${cmd:}` as -/// it reads (`io-harness-0.76.0/src/config.rs:613`), so a second discovery re-runs +/// it reads (`io-harness-0.78.0/src/config.rs:621`), so a second discovery re-runs /// an operator's credential commands — which for a `${cmd:}` fetching a key out of /// a keychain means a Touch-ID prompt raised in order to draw a menu, every time /// the picker opens. Taking a `&Config` is not an optimisation; it is the @@ -641,7 +641,7 @@ pub fn destination(config: &Config, key: &str) -> (Scope, bool) { #[must_use] pub fn widens_workspace(key: &str, value: &str) -> bool { /// The clause io-harness's widening refusal always carries - /// (`io-harness-0.76.0/src/config.rs:2628`). Matched rather than the whole + /// (`io-harness-0.78.0/src/config.rs:2740`). Matched rather than the whole /// sentence, which interpolates the path, the key and the destination scope. const WIDENS: &str = "widens the boundary"; @@ -776,8 +776,8 @@ fn is_credential(path: &str) -> bool { /// /// **There are three substitution forms and not two.** io-harness resolves /// `${env:...}`, `${file:...}` **and** `${cmd:...}` -/// (`substitute`, `io-harness-0.76.0/src/config.rs:2838`, the `cmd` arm at -/// `:2958`); this comment claimed two until +/// (`substitute`, `io-harness-0.78.0/src/config.rs:2950`, the `cmd` arm at +/// `:3070`); this comment claimed two until /// 0.21.0, and the sentence it claimed it in was the argument for which forms /// pass through here. The third is deliberately not one of them: a `${env:}` or /// `${file:}` reference is a *name*, and the name is the whole of what an diff --git a/src/context.rs b/src/context.rs index 57883a4..33e91e1 100644 --- a/src/context.rs +++ b/src/context.rs @@ -128,7 +128,7 @@ pub const SKILLS_HEAD: &str = "Skills available to you"; /// **The directive is glued to the last catalogue line with no newline between /// them, so a line scan cannot see the boundary.** `compose` builds the prompt as /// `with_skill_catalog(..)` and then `out.push_str(&directive)` -/// (io-harness-0.76.0/src/run/prompts.rs:73-76); `Skills::catalog()` ends with its +/// (io-harness-0.78.0/src/run/prompts.rs:73-76); `Skills::catalog()` ends with its /// last `- name: description` line carrying **no trailing newline** /// (src/skills.rs:470-476); and `planning_directive` returns a string beginning /// with a space (src/run/gate.rs:282-286). So with a plan gate registered — which diff --git a/src/contract.rs b/src/contract.rs index da498b3..7461290 100644 --- a/src/contract.rs +++ b/src/contract.rs @@ -336,7 +336,7 @@ pub fn configured( // `io exec` is where it matters most.** An unattended run is the one that // cannot notice a model failing its gate four times and reach for a better // one, and the headless arm uses io-harness's flat loop — the only loop that - // consults the rules at all (`run/step.rs:1097`). + // consults the rules at all (`run/step.rs:1289`). // // A section that names no rule leaves the contract's routing unset rather than // putting a default `Routing` on it; `routing::routing` owns that decision and @@ -424,7 +424,7 @@ pub fn buying(contract: TaskContract, effort: Option) -> Tas /// sits ahead of a cache breakpoint and removing a definition would save its /// tokens once and pay a cache *write* on every later turn (`src/tools/mod.rs:40`). /// A mask in fact **adds** a sentence to the user prompt naming the withheld tools -/// (`src/run/prompts.rs:976`, `withheld_sentence`), placed after the observations +/// (`src/run/prompts.rs:1133`, `withheld_sentence`), placed after the observations /// precisely so it costs no cache entry. A turn that withholds three tools is /// marginally more expensive than the same turn without the mask, not less. #[must_use] @@ -581,7 +581,7 @@ pub fn skills_dir(config: &Config, capabilities: &Capabilities, root: PathBuf) - /// **The existence test is not caution, it is the whole of what makes this /// default safe.** `Skills::discover` does not return early on a directory that /// is not there — it returns `Error::Config("skills directory … does not exist")` -/// (`io-harness-0.76.0/src/skills.rs`), and `TaskContract::discover_skills` +/// (`io-harness-0.78.0/src/skills.rs`), and `TaskContract::discover_skills` /// propagates it from `run.rs` at run start, before the first completion. A /// contract that named this directory unconditionally would therefore fail every /// turn of every operator who has never made one, which is almost all of them. @@ -606,7 +606,7 @@ fn default_skills() -> Option { /// /// **One expansion for two keys, applied after both have had their say.** /// io-harness substitutes `${env:…}`, `${file:…}` and `${cmd:…}` and nothing else -/// (`substitute`, `io-harness-0.76.0/src/config.rs:2838` — there is no tilde +/// (`substitute`, `io-harness-0.78.0/src/config.rs:2950` — there is no tilde /// branch anywhere in it, and 0.71.0 narrowed the forms rather than widening /// them: a plugin manifest now refuses all three), so a `~` an operator wrote in /// `[run] skills` or `[app.io-cli] skills` diff --git a/src/cost.rs b/src/cost.rs index df49747..e797505 100644 --- a/src/cost.rs +++ b/src/cost.rs @@ -434,6 +434,39 @@ fn grouped(spend: Vec) -> Vec { // have made the numbers look like they disagreed. value.push_str(&format!(" · {} not priced", group.unpriced_calls)); } + if group.unreported_cache_writes > 0 { + // **The money is not the only figure on this row that is a floor.** + // io-harness counts the calls whose wire carried no cache-write + // counter, and a group with one of those wrote *at least* what it + // reported — the same shape `unpriced_calls` gives the money directly + // above, and said out loud for the same reason. A row that stated one + // floor and stayed silent about the other would report an unknown as a + // fact, which is the failure the whole module is built around. + // + // **The words are `section`'s words, deliberately: "or more" for a + // mixed group and `UNREPORTED` for a wholly silent one.** The two + // halves of this page describe the same calls, and one of them + // inventing a second vocabulary for one fact would read as two + // different claims about a single number. The counters agree over + // every call that reported usage — io-harness's + // `Spend::unreported_cache_writes` and this module's + // `Total::unreported_writes` both count a call that reported usage and + // no write counter, and neither counts a call that reported no usage + // at all. That last call is the one the two sides file differently: + // io-harness puts it in `unpriced_calls` and `Total` puts it in + // `unknown`, which is exactly the divergence the "not priced" wording + // above exists for, and it leaves this qualifier alone. + // + // The count of silent calls is not drawn beside the figure. A second + // bare number on a row that already carries money, calls and tokens + // reads as another measurement, and the operator-facing fact is the + // direction — that the figure is a floor — rather than how many calls + // put it there. + value.push_str(&match group.usage.cache_write_tokens { + Some(count) => format!(" · cache written {} or more", tokens(count)), + None => format!(" · cache written {UNREPORTED}"), + }); + } rows.push(Row::fact(group.key, value)); } rows diff --git a/src/edit.rs b/src/edit.rs index 7966778..8261927 100644 --- a/src/edit.rs +++ b/src/edit.rs @@ -670,7 +670,7 @@ pub fn value_at(text: &str, path: &str) -> Option { /// Windows path is full of backslashes and `\U` opens an escape in a basic /// string, so `format!("[\"{}\"]", path)` is either a parse error or a /// different path — and a path that parses to something else is skipped in -/// silence by `config.rs:1886`, which is the quietest failure this crate can +/// silence by `config.rs:2925`, which is the quietest failure this crate can /// ship. `toml`'s own serializer knows the rules; a format string does not. /// /// This module is the crate's only TOML speller by rule diff --git a/src/events.rs b/src/events.rs index 30fd582..0e0529c 100644 --- a/src/events.rs +++ b/src/events.rs @@ -847,7 +847,7 @@ impl Events { lines.push(Line::from(spans)); lines } - EventKind::ToolCall { name, target } => { + EventKind::ToolCall { name, target, .. } => { // Nothing is committed here, and that is the point: this event is // emitted before the call runs, so a line written now could only // say what the agent was about to do. The call is held open, shown @@ -1880,7 +1880,7 @@ impl Events { // brag about concurrency. // // **Only when something was discarded.** io-harness emits this - // whenever `started > 0` (`run/step.rs:1401`), so a step that + // whenever `started > 0` (`run/step.rs:1476`), so a step that // speculated perfectly would otherwise put a line in every transcript // saying nothing happened. A run where speculation always pays is a // run with nothing to report, which is the same rule the sandbox arm diff --git a/src/exec.rs b/src/exec.rs index b933493..1afd327 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -168,6 +168,22 @@ pub fn code(outcome: &RunOutcome) -> u8 { // here at all. RunOutcome::VerificationFailed { .. } => UNVERIFIED, + // io-harness 0.77.0, and **its own CHANGELOG never declares it** — not + // under Breaking, not under Added. `RunOutcome` is `#[non_exhaustive]`, + // so the wildcard below took it silently and answered `UNFINISHED` for a + // run that was judged. It belongs beside `VerificationFailed` by that + // variant's own argument: the work produced an answer, something checked + // it, and it did not hold up. The harness makes the same distinction one + // layer down — `run.rs:324-338` says reporting a schema failure as a step + // cap "would send an operator to raise `max_steps`, which buys more + // attempts at the same failure". + // + // Reachable here, not theoretical. io-cli sets no schema itself, but + // `[run] output_schema` is a configuration key (`config.rs:901`) that + // `Config::apply_to` applies (`config.rs:2226`), and both arms call it — + // so an operator who writes one gets this outcome on an ordinary turn. + RunOutcome::SchemaUnsatisfied { .. } => UNVERIFIED, + // `AwaitingApproval` stays unreachable from here while approvals are // denied rather than deferred; the other two are reachable, because a // question about intent and a proposed plan pass through no approver at @@ -367,6 +383,10 @@ pub fn describe(outcome: &RunOutcome) -> String { "stopped at the step cap, and its verification failed", steps, ), + RunOutcome::SchemaUnsatisfied { steps } => ( + "never produced the shape its output schema asked for", + steps, + ), RunOutcome::TimeBudgetExceeded { steps } => ("stopped at the time budget", steps), RunOutcome::CostBudgetExceeded { steps } => ("stopped at the token budget", steps), RunOutcome::BudgetCeilingReached { steps } => { @@ -1483,8 +1503,8 @@ impl WithProvider for Resuming { // other pause kind has both forms — `resume_tree_with_answer` beside // `resume_with_answer`, `resume_tree_with_plan_decision` beside its flat // one, `resume_tree_with_decision` beside `resume_with_decision` - // (`io-harness-0.76.0/src/run.rs:1790`, `:2109`, `:3145`). Recovery has - // `resume_with_recovery_observed` (`:2571`) and nothing tree-aware, so it + // (`io-harness-0.78.0/src/run.rs:1806`, `:2140`, `:3184`). Recovery has + // `resume_with_recovery_observed` (`:2602`) and nothing tree-aware, so it // is the one pause a contained run cannot be resumed from. Not an oversight // this crate can route around: a fleet's shared ceiling lives in the tree // entry points, and resuming through the flat one would drop it. diff --git a/src/export.rs b/src/export.rs index 12af4f3..720d9e5 100644 --- a/src/export.rs +++ b/src/export.rs @@ -23,7 +23,7 @@ //! **It is not JSON, and this release's plan said it was.** The format is //! pipe-delimited lines built by hand — `step {n} | tokens {n} | decision … ` //! per step, then `context {step} | {kind} | {detail}` per context event -//! (`state/trace.rs:1019-1036`). io-harness offers no JSON alternative; +//! (`state/trace.rs:1234-1251`). io-harness offers no JSON alternative; //! `StepRecord` does not even derive `Serialize`. io-cli *could* build one — //! `serde_json` is among its ten dependencies — and must not, because a document //! this crate assembled would be a second format that compares against nothing, diff --git a/src/gates.rs b/src/gates.rs index b42b497..6b59848 100644 --- a/src/gates.rs +++ b/src/gates.rs @@ -492,7 +492,7 @@ pub fn proposed_command(root: &Path, config: &Config) -> Option> { /// **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 +/// (`run/step.rs:1985`), 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 diff --git a/src/home.rs b/src/home.rs index a973ce5..7b5a831 100644 --- a/src/home.rs +++ b/src/home.rs @@ -9,7 +9,7 @@ //! This module names one. It is deliberately **not** a second configuration //! system: io-harness resolves `$IO_CONFIG`, then `$IO_CONFIG_HOME`, then the //! platform's own place, reading the environment at call time -//! (`io-harness-0.76.0/src/config.rs:2146`, the Windows branch at `:2164`), and +//! (`io-harness-0.78.0/src/config.rs:2241`, the Windows branch at `:2259`), and //! there is no caller-supplied home //! anywhere in `Config`'s public surface — no `discover_in`, no builder. So the //! one lever is `$IO_CONFIG_HOME`, set once, before the first @@ -347,7 +347,7 @@ pub fn in_force() -> Option<(PathBuf, Origin)> { /// Expand a leading `~` against the operator's home directory. /// /// io-harness substitutes `${env:…}` and `${file:…}` and nothing else -/// (`config.rs:1965`), so a `~` an operator writes in a path reaches the code that +/// (`config.rs:3004`), so a `~` an operator writes in a path reaches the code that /// uses it verbatim — and `Skills::discover` would then look in a directory /// literally named `~`. Expanding it here rather than asking the harness to keeps /// the substitution rules the harness's own. diff --git a/src/import.rs b/src/import.rs index d45196b..75544fe 100644 --- a/src/import.rs +++ b/src/import.rs @@ -30,7 +30,7 @@ //! decision="allow")` and Claude's `permissions.ask` says `Bash(cargo yank *)` — //! both of which match a *command line*. io-harness's `Act::Exec` matches a //! **binary name and nothing else**; it has no argument matching at all -//! (`Act::Exec`, `io-harness-0.76.0/src/policy.rs:63`). So the nearest thing to a faithful +//! (`Act::Exec`, `io-harness-0.78.0/src/policy.rs:63`). So the nearest thing to a faithful //! import of `bun install` is a blanket allow on `bun`, which is a wider //! permission than the operator ever granted, written by a tool they were //! trusting to be careful. A boundary half-imported is worse than one left alone, diff --git a/src/lib.rs b/src/lib.rs index 2bf9fa0..ac773aa 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -134,5 +134,6 @@ pub mod theme; pub mod transcript; pub mod triage; pub mod undo; +pub mod upgrade; pub mod verify; pub mod wizard; diff --git a/src/main.rs b/src/main.rs index b246537..4e7f565 100644 --- a/src/main.rs +++ b/src/main.rs @@ -104,6 +104,31 @@ async fn run(report: &mut Vec) -> Result { io_cli::exec::OK })); }); + // `io upgrade` leaves before anything else happens at all — before the home + // is adopted, before a configuration is discovered, before a working + // directory is even resolved. It reads this process's own path and prints, + // so taking a home or a store on the way would be side effects a command + // that answers one question has no business having. + // + // The whole decision is `upgrade::advice`, in the library, because nothing + // under `tests/` links this file. What is written here is the read of + // `current_exe` and the printing, and neither is a decision. + if let Some(Subcommand::Upgrade) = cli.command { + // A process that cannot say where its own executable is has no path to + // classify, so it hands on an empty one and takes the unrecognised arm + // rather than guessing at a location. `cfg!` and not a runtime probe: + // the installer that could have placed this binary is the one built for + // the platform it was built for. + let exe = std::env::current_exe().unwrap_or_default(); + for line in io_cli::upgrade::advice(&exe, cfg!(windows)) { + println!("{line}"); + } + // `OK` rather than a bare zero: this is the exit table + // `docs/CONTRACT.md` publishes, and a literal here would be a second + // opinion about what success is. + return Ok(io_cli::exec::OK); + } + let root = match cli.dir { Some(dir) => dir, None => std::env::current_dir().map_err(|error| error.to_string())?, @@ -1139,7 +1164,7 @@ async fn loop_over Result>( // **The one thing a routing section cannot say about itself**, said before the // configuration is handed on so it reads from the settings actually in force // rather than from a copy of them. io-harness consults the rules only in its - // flat loop (`run/step.rs:1097`), so a contained turn parses them, carries them + // flat loop (`run/step.rs:1289`), so a contained turn parses them, carries them // and never fires them. // // Conditional on the session actually being contained, and that is F4's whole @@ -2192,16 +2217,26 @@ async fn loop_over Result>( id, scope, edits, - made, + staged, } => { if io_cli::store::acts(index) { - // **The write happens here and nowhere earlier.** - // `edits` is `manage::plan`'s own single - // `pluginview::add`, so the entry lands switched - // on, in one append, and every unrelated section - // comes through byte for byte — see `src/edit.rs`, - // which splices a span and copies the rest. - match io_cli::configure::write(session.root(), *scope, edits) { + // **The write happens here and nowhere earlier, and + // so does the swap.** `edits` is `manage::plan`'s + // own single `pluginview::add`, so the entry lands + // switched on, in one append, and every unrelated + // section comes through byte for byte — see + // `src/edit.rs`, which splices a span and copies + // the rest. + // + // `marketplace::make` first, because a refresh + // whose entry is already in the file has no edits + // at all and the swap is then the whole of what + // consent performs — and because a commit that + // fails must not leave an entry naming a directory + // the adapter never reached. + match io_cli::marketplace::make(staged.as_ref()).and_then(|()| { + io_cli::configure::write(session.root(), *scope, edits) + }) { Ok(()) => { // **Re-read, or `/plugin` will not list what // was just installed.** `Action::Plugin` @@ -2245,16 +2280,21 @@ async fn loop_over Result>( // nothing on disk naming it, so there is now nothing // to leave behind and nothing to undo. // **And the adapter goes with the decline.** For a - // Claude Code or Codex bundle io had to write a + // Claude Code or Codex bundle io had to build a // manifest before io-harness could read the bundle // at all, so there *is* something to take back // here even though the operator's configuration - // was never opened. `marketplace::unmake` is the - // one place that decision lives, and `Esc` on this - // same picker calls it too — the standard way out - // of a modal must not be the one that leaves - // something behind. - io_cli::marketplace::unmake(made.as_deref()); + // was never opened. It was built beside its + // destination and never in it, so this takes back + // the staging directory alone and an adapter the + // operator already had is exactly as it was — + // which is what `docs/guide/plugins.md` has said + // since 0.35.0 and what 0.38.0 makes true. + // `marketplace::unmake` is the one place that + // decision lives, and `Esc` on this same picker + // calls it too — the standard way out of a modal + // must not be the one that leaves something behind. + io_cli::marketplace::unmake(staged.as_ref()); app.record( Tone::Muted, format!( @@ -4291,15 +4331,15 @@ async fn loop_over Result>( Outcome::Cancelled => { // **`Esc` is a decline, and one picker has something to take // back.** An install disclosure is drawn over an adapter io - // already wrote — io-harness has no loader that takes a + // already built — io-harness has no loader that takes a // foreign manifest, so there is nothing to disclose until the - // generated one exists. The "leave it" row removes it; leaving - // `Esc` out would make the standard way out of every other - // modal in this product the one that leaves a directory nobody - // agreed to under the operator's home, listed by no surface - // and removed by no verb. - if let Some((_, Pick::PluginInstall { made, .. })) = &picker { - io_cli::marketplace::unmake(made.as_deref()); + // generated one exists. The "leave it" row discards it; + // leaving `Esc` out would make the standard way out of every + // other modal in this product the one that leaves a directory + // nobody agreed to under the operator's home, listed by no + // surface and removed by no verb. + if let Some((_, Pick::PluginInstall { staged, .. })) = &picker { + io_cli::marketplace::unmake(staged.as_ref()); } picker = None; } @@ -4910,11 +4950,20 @@ async fn loop_over Result>( id: disclosure.id.clone(), scope: plan.scope, edits: plan.edits.clone(), - made: plan.made.clone(), + staged: plan.staged.clone(), }, )); } Ok((request, Some(plan))) => { + // **No `marketplace::make` here, and the guard above is + // why.** `manage::plan` fills `staged` only where it + // fills `disclosure` — both come off the one `Chosen` + // for `plugin add ` out of a marketplace — so a + // plan carrying a staged adapter is answered on the + // confirmation arm and never reaches this one. Calling + // it here as well would put a second opinion about what + // consent performs in the driver, which is the one + // place nothing under `tests/` can gate. match io_cli::configure::write(&root, plan.scope, &plan.edits) { Err(refusal) => app.record(Tone::Refused, refusal), Ok(()) => { @@ -6844,7 +6893,7 @@ async fn loop_over Result>( // **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 + // when it stops — `run/step.rs:1985` 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 @@ -9676,15 +9725,21 @@ enum Pick { id: String, scope: io_harness::config::Scope, edits: Vec, - /// The adapter directory io generated so that io-harness could read this - /// bundle at all, removed when the operator declines. + /// The adapter io built so that io-harness could read this bundle at all, + /// put in place when the operator accepts and thrown away when they + /// decline. /// /// `None` for a native bundle, which needed nothing generated. A Claude /// Code or Codex bundle has no manifest io-harness reads, so there is - /// literally nothing to disclose until io has written one — the file is + /// literally nothing to disclose until io has built one — the file is /// io's own, in io's own home, and the operator's configuration is /// untouched either way. - made: Option, + /// + /// **It stands beside its destination rather than in it**, so this + /// picker's two answers are the two answers the disk gets: an operator who + /// declines a refresh keeps exactly the adapter they already had. See + /// `io_cli::adapt::Staged`. + staged: Option, }, /// The second half of removing a bundle: which entry, and are you sure. /// @@ -10048,6 +10103,16 @@ async fn manage_main( disclosure.id, ); } + // **The consent this door has already been given, performed.** An adapter is + // built beside its destination and swapped in by whoever holds the answer + // (`io_cli::adapt::Staged`); on this door the typed command is the answer, as + // the sentence above says, so it commits. There is no decline path here to + // discard on — naming a bundle *is* the yes — and a plan carrying nothing + // staged, which is every other request this door routes, passes through. + // + // Before the write, so a commit that fails leaves no entry naming a directory + // the adapter never reached; `?` takes the refusal to the exit code. + io_cli::marketplace::make(plan.staged.as_ref())?; io_cli::configure::write(root, plan.scope, &plan.edits)?; // **The policy preflight, after the write and on stderr.** After, because the // report is a disclosure and not a veto: refusing to write an entry because diff --git a/src/manage.rs b/src/manage.rs index 4d79f8b..9a9236b 100644 --- a/src/manage.rs +++ b/src/manage.rs @@ -257,17 +257,19 @@ pub enum ConfigVerb { /// one, and that is a property worth keeping rather than a shape worth widening. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Plan { - /// A directory io created while building this plan, which a **declined** - /// install must take back. + /// An adapter io built while building this plan, which an **accepted** install + /// puts in place and a **declined** one throws away. /// /// `Some` for exactly one request: `plugin add ` resolving to a Claude /// Code or Codex bundle, where the generated manifest has to exist before /// io-harness can read the bundle and there is therefore something to disclose - /// at all. The operator's configuration is never opened either way — this is - /// io's own file, in io's own home — and [`declined`] is the one place it is - /// removed, so two doors cannot disagree about whether saying no leaves - /// something behind. - pub made: Option, + /// at all. It is built beside its destination and not in it, so the answer + /// decides whether the adapter the operator already has is replaced — see + /// [`crate::adapt::Staged`]. The operator's configuration is never opened + /// either way, and [`crate::marketplace::make`] and + /// [`crate::marketplace::unmake`] are the one place each answer is performed, + /// so two doors cannot disagree about what saying yes or no does. + pub staged: Option, /// The scope whose file is written. pub scope: Scope, /// What to write, applied together or not at all. @@ -712,7 +714,7 @@ pub fn plan( scope: *scope, edits: vec![crate::servers::add(server)], disclosure: None, - made: None, + staged: None, }, Request::Mcp(McpVerb::Edit { id, key, value }) => { let at = declared_server(root, id)?; @@ -731,7 +733,7 @@ pub fn plan( scope: at.scope, edits: vec![edit], disclosure: None, - made: None, + staged: None, } } Request::Mcp(McpVerb::Remove { id }) => { @@ -740,7 +742,7 @@ pub fn plan( scope: at.scope, edits: vec![crate::servers::remove(&at)], disclosure: None, - made: None, + staged: None, } } // **The entry is found and then edited, never removed and re-added.** @@ -799,18 +801,28 @@ pub fn plan( // they do not cross. The author's own directory is where the hooks it // declares are, and naming them is the whole of what io owes an // operator who would otherwise reasonably assume they run. + // + // **`chosen.read()` and not `chosen.dir()` for the directory handed to + // io-harness (0.38.0).** An adapted bundle's adapter is staged beside + // its destination until the operator answers, so `dir()` is either a + // directory that does not exist yet or one still holding the *previous* + // install — and a disclosure read from it would describe last week's + // bundle while the operator answers about this week's. `dir()` stays + // the argument the operator is shown and the `[[plugin]]` entry names, + // because that is where the adapter is a moment after they say yes. let disclosure = chosen .discloses() .then(|| { crate::marketplace::adapted_disclosure( *scope, chosen.dir(), + chosen.read(), (chosen.from() != chosen.dir()).then(|| chosen.from()), chosen.copied(), ) }) .transpose() - .inspect_err(|_| crate::marketplace::unmake(chosen.made()))?; + .inspect_err(|_| crate::marketplace::unmake(chosen.staged()))?; // **And nothing is written at all when the entry is already there // (0.35.0).** Since the adapter became a copy of what it contributes, // **installing again is the update path** — and `pluginview::add` @@ -821,15 +833,17 @@ pub fn plan( // the operator's file grew an ignored entry on each update, quietly, // forever. // - // There is no edit to make: the adapter was regenerated on disk by - // `chosen` above, before this point, and the declaration that names it - // is already correct. The disclosure still travels, because what the - // refresh moved is exactly what the operator needs to see. + // There is no edit to make: the declaration that names the adapter is + // already correct, and the refreshed adapter itself lands when the + // operator accepts. The disclosure still travels, because what the + // refresh moves is exactly what they need to see before they answer — + // and on this path it is the *only* thing that travels, so a plan with + // no edits still carries an act. let already = declared .iter() .any(|(_, path)| path.as_path() == chosen.dir()); Plan { - made: chosen.made().map(Path::to_path_buf), + staged: chosen.staged().cloned(), scope: *scope, // One entry, switched on, written once. Through 0.29.0 a // marketplace bundle was written `enabled = false` first and @@ -851,14 +865,14 @@ pub fn plan( scope, edits: vec![crate::pluginview::remove(index)], disclosure: None, - made: None, + staged: None, } } Request::Config(ConfigVerb::Set { key, value, scope }) => Plan { scope: decided_scope(root, key, *scope), edits: vec![Edit::set(key.clone(), value.clone())], disclosure: None, - made: None, + staged: None, }, Request::Config(ConfigVerb::Unset { key, scope }) => Plan { scope: decided_scope(root, key, *scope), @@ -870,7 +884,7 @@ pub fn plan( // operator's entire block. See `Edit::unset`. edits: vec![Edit::unset(key.clone())], disclosure: None, - made: None, + staged: None, }, // **A marketplace verb plans no write, and it is here beside the reads // rather than given a shape of its own.** `add` and `remove` are not @@ -916,7 +930,7 @@ fn switched(root: &Path, id: &str, on: bool) -> Result { scope: at.scope, edits: vec![crate::servers::switch(&at, on)], disclosure: None, - made: None, + staged: None, }) } @@ -1394,7 +1408,7 @@ fn mcp_add(args: &Args) -> Result { // Built by hand rather than through `McpServer::stdio(…).with_args(…)`, // because `env` and `headers` have no builder at all and `with_args` is a - // silent no-op on an HTTP server (`io-harness-0.76.0/src/mcp.rs:439-447`: the + // silent no-op on an HTTP server (`io-harness-0.78.0/src/mcp.rs:439-447`: the // body writes only into the `Stdio` arm) — a constructor chain here would drop // the arguments of half the servers it was handed and say nothing. // Asked of the harness rather than written as literals, the way `servers::add` diff --git a/src/marketplace.rs b/src/marketplace.rs index fa08a5d..5df58e3 100644 --- a/src/marketplace.rs +++ b/src/marketplace.rs @@ -1462,18 +1462,21 @@ pub struct Prepared { /// The bundle's own directory — the author's files. What the disclosure reads /// hooks from, because the generated manifest carries none. pub from: PathBuf, - /// The directory io created **for this install**, if it created one. Removed - /// when the operator declines, so a refused install leaves nothing behind. + /// The adapter io built **for this install**, standing beside + /// [`Prepared::declare`] and not yet in it. `None` for a native bundle, which + /// needed nothing generated. /// - /// **`None` for an adapter that was already there**, and that is the whole of - /// what makes a re-install safe. Installing a bundle a second time is the - /// update path (see [`prepared`]), so by the time the operator is asked to - /// consent there is an adapter directory that a `[[plugin]]` entry already - /// names — and answering "leave it" would otherwise delete the bundle they - /// installed last week, out of a file this module never opened and cannot see. - /// A decline then takes back what this install created, which for an update is - /// nothing. - pub made: Option, + /// **This replaced `made`, which was the directory a decline deleted**, and + /// the reason it could be replaced is the reason the field existed. `made` was + /// `None` for an adapter that was already there, because the generator had + /// swapped the new adapter in before anyone was asked and deleting the + /// directory on a decline would have taken away the bundle the operator + /// installed last week. That distinction has no work left to do: the swap is + /// [`crate::adapt::Staged::commit`] and it runs *after* the answer, so on a + /// first install and on an update alike a decline discards a staging directory + /// and `declare` is untouched either way. One field, one answer, and no case + /// where declining reaches a directory a `[[plugin]]` entry names. + pub staged: Option, /// The directories the adapter copied out of the clone, in /// [`crate::adapt::Adapter::copied`]'s own words — `skills`, `templates`, or /// both. Empty for a native bundle, which is declared where it sits. @@ -1512,13 +1515,15 @@ pub struct Prepared { /// what moves it across. [`Prepared::copied`] is what moved, and /// [`adapted_disclosure`] says so before the operator answers. /// -/// **The adapter is written before consent and removed if consent is refused**, -/// and that is the honest order rather than a convenience. The disclosure is -/// io-harness reading the bundle, and io-harness has no loader that takes a -/// foreign manifest — there is nothing to read until the manifest io generates -/// exists. What the operator's *configuration* never sees is any of it: -/// `Prepared::made` is what a declined install removes, and the scope file is not -/// opened either way. +/// **The adapter is built before consent and put in place after it.** The +/// disclosure is io-harness reading the bundle, and io-harness has no loader that +/// takes a foreign manifest — so there is nothing to read until the manifest io +/// generates exists, and building it is not a thing consent can come before. What +/// consent decides is the *swap*: until 0.38.0 [`crate::adapt::generate`] ended by +/// replacing the destination, so declining a refresh left the new adapter over the +/// one the operator already had. [`Prepared::staged`] is what a declined install +/// discards, [`Prepared::declare`] is untouched until an accepted one commits, and +/// the operator's configuration file is not opened either way. pub fn prepared( markets: &[Market], query: &str, @@ -1540,7 +1545,7 @@ pub fn prepared( return Ok(Prepared { declare: from.clone(), from, - made: None, + staged: None, copied: Vec::new(), }); } @@ -1556,31 +1561,47 @@ pub fn prepared( ) })?; let into = crate::adapt::at(adapters, &market.named.owner, &market.named.repo, &name); - // **Asked before the generator answers, because the generator removes it.** - // `adapt::generate` rebuilds the adapter directory from scratch on every run — - // that is what makes an update an update — so after the call there is no way - // left to tell an install from a re-install. See [`Prepared::made`]: the - // difference decides whether a decline deletes a bundle the operator installed - // some other day. - let already = into.is_dir(); + // **An install and a re-install are no longer told apart here, and nothing + // downstream asks.** Until 0.38.0 `into.is_dir()` was read *before* the + // generator, because the generator swapped the new adapter in on the way past + // and there was no way to tell the two cases apart afterwards — and the answer + // decided whether declining was allowed to delete the directory. `generate` + // now leaves the adapter staged, so declining reaches `into` in neither case + // and the question has no consequence left to have. let written = crate::adapt::generate(&from, &name, &into)?; Ok(Prepared { - declare: into.clone(), + declare: into, from, - made: (!already).then_some(into), + staged: Some(written.staged), copied: written.copied, }) } -/// Take back what an install created, because the operator said no. +/// Put what an install built in place, because the operator said yes. /// -/// A no-op for a native bundle, which created nothing. Best effort by design: the -/// operator has already declined and the entry was never written, so a directory -/// that cannot be removed is a stale cache rather than anything they consented to, -/// and a refusal reported *after* a refusal is noise about io's own housekeeping. -pub fn unmake(made: Option<&Path>) { - if let Some(dir) = made { - let _ = std::fs::remove_dir_all(dir); +/// A no-op for a native bundle, which built nothing: it is declared where it sits. +/// The pair of [`unmake`], and one of the two runs on every path out of a +/// disclosure — see [`crate::adapt::Staged`]. +/// +/// # Errors +/// +/// [`crate::adapt::Staged::commit`]'s, naming the destination and what the rename +/// said. A caller that gets one must not go on to write the `[[plugin]]` entry: +/// there is no adapter at the path it would name. +pub fn make(staged: Option<&crate::adapt::Staged>) -> Result<(), String> { + staged.map_or(Ok(()), crate::adapt::Staged::commit) +} + +/// Throw away what an install built, because the operator said no. +/// +/// A no-op for a native bundle, which built nothing. Best effort by design: the +/// operator has already declined, the entry was never written and the adapter they +/// already had was never touched, so a directory that cannot be removed is a stale +/// cache rather than anything they consented to, and a refusal reported *after* a +/// refusal is noise about io's own housekeeping. +pub fn unmake(staged: Option<&crate::adapt::Staged>) { + if let Some(staged) = staged { + staged.discard(); } } @@ -1630,12 +1651,36 @@ impl Chosen { } } - /// What a declined install must take back, if anything was created. + /// The directory io-harness is asked to load **now**, which for an adapted + /// bundle is not [`Chosen::dir`]. + /// + /// A staged adapter is not at its destination yet: on a first install that + /// destination does not exist, and on an update it still holds the *previous* + /// adapter — so a disclosure read from [`Chosen::dir`] would describe either + /// nothing or last week's bundle, and the operator would be answering about a + /// directory that is not the one they are being offered. [`Chosen::dir`] stays + /// what the `[[plugin]]` entry names and what the operator is shown, because + /// that is where the adapter is a moment after they say yes. #[must_use] - pub fn made(&self) -> Option<&Path> { + pub fn read(&self) -> &Path { + match self { + Self::Path(dir) => dir, + Self::Held(prepared) => prepared + .staged + .as_ref() + .map_or(prepared.declare.as_path(), |staged| { + staged.staging.as_path() + }), + } + } + + /// What an accepted install commits and a declined one discards, if this + /// install built anything. + #[must_use] + pub fn staged(&self) -> Option<&crate::adapt::Staged> { match self { Self::Path(_) => None, - Self::Held(prepared) => prepared.made.as_deref(), + Self::Held(prepared) => prepared.staged.as_ref(), } } @@ -1774,7 +1819,7 @@ impl Disclosure { /// one that *does* show, the separator between a row's two fields, is applied by /// the door in [`Disclosure::said`]. pub fn disclosure(scope: Scope, dir: &Path) -> Result { - adapted_disclosure(scope, dir, None, &[]) + adapted_disclosure(scope, dir, dir, None, &[]) } /// The sentence every withheld hook carries, spelled once. @@ -1793,8 +1838,8 @@ pub const NOT_CARRIED: &str = "io does not run this — a hook here is a shell l /// [`disclosure`], plus the hooks a foreign bundle declares and io will not carry. /// /// `from` is the bundle's **own** directory — the stranger's checkout — where -/// `dir` is what io-harness is asked to load, which for an adapted bundle is the -/// generated manifest's directory. The two differ precisely because the adapter +/// `dir` is the adapter, which for an adapted bundle is the generated manifest's +/// directory rather than the author's. The two differ precisely because the adapter /// carries no hooks: asking io-harness what the bundle contributes therefore /// cannot answer what it *declared*, and reading the author's own file is the only /// way to name what is being left behind. @@ -1808,16 +1853,45 @@ pub const NOT_CARRIED: &str = "io does not run this — a hook here is a shell l /// an operator asks a week later: they edited a skill inside the clone, or pulled /// the repository, and the session went on using what io copied. Empty for every /// bundle that copied nothing, where the row would be a warning about nothing. +/// +/// **`read` is the directory io-harness is handed and `dir` is the one the +/// operator is told about, and they differ for exactly one reason.** A staged +/// adapter ([`crate::adapt::Staged`]) is not at `dir` yet — the swap is what +/// consent buys — so reading `dir` on an update would disclose the bundle the +/// operator already has rather than the one they are being offered, and on a first +/// install would refuse a directory that is not there. [`Chosen::read`] is the one +/// place that answer is worked out. Pass `dir` twice where nothing is staged. +/// +/// Every path io-harness answers with is then re-rooted from `read` onto `dir`, so +/// what the operator is shown is where the adapter will be rather than where it +/// was read from — the two rows would otherwise name two different directories for +/// one bundle. pub fn adapted_disclosure( scope: Scope, dir: &Path, + read: &Path, from: Option<&Path>, copied: &[String], ) -> Result { - let plugin = io_harness::Plugins::inspect(scope, dir).map_err(|error| error.to_string())?; + let plugin = io_harness::Plugins::inspect(scope, read).map_err(|error| error.to_string())?; // `true`, because nothing has been written: this is what the directory *is*, // not what some `[[plugin]]` entry said about it. See `pluginview::copy_out`. - let listed = crate::pluginview::copy_out(&plugin, true); + let mut listed = crate::pluginview::copy_out(&plugin, true); + // **Re-rooted onto the destination, because io-harness answered about the + // directory it was handed.** `detail` draws the contributed directories as + // whole paths, and for a staged adapter every one of them is under a hidden + // sibling that exists only until the operator answers — so the rows would send + // them somewhere that is gone a moment later, and would disagree with the + // `copied` row below, which names where the adapter lands. The prefix stripped + // is `Plugin::root` rather than `read`, so the swap holds whether or not + // io-harness canonicalised what it was given. Nothing else in `listed` is a + // path under the adapter: an `[[mcp]]` argv points into the clone, and the + // generated manifest declares no `[[bin]]` and no `[[hook]]` at all. + if read != dir { + let under = std::mem::replace(&mut listed.root, dir.to_path_buf()); + listed.skills = listed.skills.take().map(|at| rerooted(&at, &under, dir)); + listed.templates = listed.templates.take().map(|at| rerooted(&at, &under, dir)); + } let mut rows = crate::pluginview::detail(&listed, u16::MAX, &crate::glyphs::ASCII); // A row rather than a field of its own, because a field is a line only a door // that knows about it prints — and both doors already print every row of this, @@ -1843,6 +1917,16 @@ pub fn adapted_disclosure( }) } +/// `path` with its `from` prefix replaced by `to`, or unchanged where it has none. +/// +/// Unchanged rather than refused, because this answers a consent surface: a path +/// that is one directory out of date is worth less than the disclosure it would +/// otherwise take down. +fn rerooted(path: &Path, from: &Path, to: &Path) -> PathBuf { + path.strip_prefix(from) + .map_or_else(|_| path.to_path_buf(), |rest| to.join(rest)) +} + /// One line per hook the bundle at `from` declares, with its command unshortened. /// /// **The command is filtered and never bounded**, which is the one place this diff --git a/src/memory.rs b/src/memory.rs index 4d76af7..9a1ac3f 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -27,7 +27,7 @@ //! //! Worth knowing before a surface promises otherwise. `read_instructions` joins //! each name in `[instructions] files` to the **discovery root** -//! (`io-harness-0.76.0/src/config.rs:2799`), and the default list is exactly +//! (`io-harness-0.78.0/src/config.rs:2911`), and the default list is exactly //! `["AGENTS.md"]` (`DEFAULT_INSTRUCTIONS`, `config.rs:190`). So `AGENTS.md` is read //! by every project with no configuration at all; `AGENTS.local.md` is read only //! where a file names it; and `IO.md` is not reachable by a bare name at all, @@ -464,7 +464,7 @@ fn bullet(line: &str) -> Option { /// The order the three files are named in, and it is the order they reach the /// model in: `read_instructions` pushes one constraint per name in list order -/// (`read_instructions`, `io-harness-0.76.0/src/config.rs:2799`). +/// (`read_instructions`, `io-harness-0.78.0/src/config.rs:2911`). /// /// Project, then local, then user — widest audience first, so the operator's own /// standing note is the last thing said. It also matches the order the three @@ -475,7 +475,7 @@ const ORDER: [Scope; 3] = [Scope::Project, Scope::Local, Scope::User]; /// /// **Not the same thing as [`path`], and the difference is the whole reason /// `IO.md` needs its own case.** io-harness resolves every name it is given with -/// `root.join(&name)` against the discovery root (`config.rs:1885`), so a bare +/// `root.join(&name)` against the discovery root (`config.rs:2924`), so a bare /// name means "in the workspace". That is exactly right for the two project /// files and unreachable for `IO.md`, which lives in io-cli's home — a directory /// that is not the workspace and has no relative spelling from it. So the user @@ -483,7 +483,7 @@ const ORDER: [Scope; 3] = [Scope::Project, Scope::Local, Scope::User]; /// /// Absolute and not `${env:HOME}/.io-cli/IO.md`, which would read as the tidier /// answer and is a trap: the harness does substitute `${env:…}`, and an unset -/// variable is a **hard error** that fails the whole parse (`config.rs:1983-1989`). +/// variable is a **hard error** that fails the whole parse (`config.rs:3022-3028`). /// On Windows `HOME` is routinely unset. A configuration file that refuses to /// parse is a session that does not start, and it would fail on the machine of /// whoever copied the file rather than on the machine that wrote it. `~` is not @@ -500,13 +500,13 @@ fn entry(root: &Path, scope: Scope) -> Option { /// **`AGENTS.md` is always in it, and that is a correctness requirement rather /// than a courtesy.** `files` REPLACES the default list, it does not add to it: /// `read_instructions` falls back to `DEFAULT_INSTRUCTIONS` — exactly -/// `["AGENTS.md"]` (`config.rs:158`) — only when the table is absent or names +/// `["AGENTS.md"]` (`config.rs:190`) — only when the table is absent or names /// nothing, and otherwise takes `Some(files) => files.clone()` verbatim -/// (`config.rs:1879-1882`). So a list written to reach `AGENTS.local.md` and +/// (`config.rs:2918-2921`). So a list written to reach `AGENTS.local.md` and /// `IO.md` and no more would **silently stop the repository's own `AGENTS.md` /// being read** — no error, no warning, just a model that no longer knows the /// project's rules. Nothing on the surface would show it, because a missing -/// instruction file is skipped without a word (`config.rs:1886`). +/// instruction file is skipped without a word (`config.rs:2925`). /// /// Two entries rather than three where there is no home to put `IO.md` in, which /// is the same answer [`path`] gives for the same reason. The project pair never @@ -548,17 +548,17 @@ fn declared(path: &Path) -> Option { /// `io.toml` is committed, so writing it there would put one person's /// `/Users/somebody/.io-cli/IO.md` into everybody else's checkout: harmless to /// them only because the file does not exist and is skipped in silence -/// (`config.rs:1886`), and a leak of the operator's account name to every reader +/// (`config.rs:2925`), and a leak of the operator's account name to every reader /// of the repository regardless. `~/.io-cli/io.toml` is the one file in this /// product that is never committed, so it is the only honest place for a path /// that is true on exactly one machine. /// /// The cost is stated rather than hidden: `["instructions","files"]` is not in -/// io-harness's `APPENDING` set (`config.rs:2052`), so a project or local file +/// io-harness's `APPENDING` set (`config.rs:3130`), so a project or local file /// that names `files` itself replaces this list wholesale rather than adding to /// it — the scopes are merged in the order they are listed at -/// `config.rs:688-693`, and a later one's value simply overwrites -/// (`config.rs:2142-2144`), so Local > Project > User. That is the +/// `config.rs:1078-1083`, and a later one's value simply overwrites +/// (`config.rs:3222-3224`), so Local > Project > User. That is the /// harness's rule and this module does not fight it — [`view`] reports the /// result instead, so an operator whose project overrode the list sees that it /// did rather than being told all three are read. @@ -609,14 +609,14 @@ pub fn install(root: &Path) -> Result { /// /// Recovered from [`Config::instructions`] rather than by re-deriving the /// precedence rules, and that choice is the point: every entry is wrapped -/// `"Project instructions from `{name}`:\n{text}"` (`config.rs:1895-1898`), so +/// `"Project instructions from `{name}`:\n{text}"` (`config.rs:2934-2937`), so /// the list of names is right there in the strings the harness built. Reading it /// back says what **was read**; re-implementing the merge would say what io-cli /// believes it asked for, and the two differ in exactly the case a surface /// exists to show — a project file that replaced the list. /// /// `name` is the name as it was written in `files`, not the resolved location, -/// so it is joined to the discovery root here the same way `config.rs:1885` +/// so it is joined to the discovery root here the same way `config.rs:2924` /// joins it. `Path::join` returns an absolute argument unchanged, so the one /// expression covers both the two relative names and `IO.md`'s absolute one. fn reading(root: &Path, config: &Config) -> Vec { @@ -661,9 +661,9 @@ pub struct Instruction { /// The two right-hand columns are deliberately independent. A file that exists /// and is not being read is the whole reason this exists, and it happens for /// three ordinary causes: a project `[instructions] files` replaced the user -/// list (`config.rs:2052` — this key does not append), [`install`] was never +/// list (`config.rs:3130` — this key does not append), [`install`] was never /// run, or the file holds nothing but whitespace and was skipped -/// (`config.rs:1892`). A view that inferred `read` from `exists` would report +/// (`config.rs:2931`). A view that inferred `read` from `exists` would report /// all three green in every one of those cases, which is worse than no view: it /// would be the surface an operator trusts while their `AGENTS.md` goes unread. #[must_use] @@ -678,7 +678,7 @@ pub fn view(root: &Path, config: &Config) -> Vec { Some(Instruction { scope, // `is_file`, the same question io-harness asks at - // `config.rs:1886`, and not "did the read succeed": a file this + // `config.rs:2925`, and not "did the read succeed": a file this // process cannot read is still there, and reporting it absent // would send the operator to create one that already exists. exists: at.is_file(), diff --git a/src/preflight.rs b/src/preflight.rs index 3d58c7f..2020fbf 100644 --- a/src/preflight.rs +++ b/src/preflight.rs @@ -68,16 +68,16 @@ //! exactly those three, and it is the same value the next turn runs under. //! //! On the ordinary path that is the *whole* policy `McpSession::connect` is handed: -//! `run.rs:3935` passes the caller's policy through untouched. Two layers exist that +//! `run.rs:4245` passes the caller's policy through untouched. Two layers exist that //! an embedder cannot read, and neither one is silently in force here: //! //! * `net::provider_layer` (`net.rs:334`, `pub(crate)`) allows exactly the provider's //! own `host:port`, and is merged into the policy that reaches `connect` only on -//! the path that resumes a run after a network approval (`run.rs:2780`, `:3122`). +//! the path that resumes a run after a network approval (`run.rs:3000`, `:3370`). //! It widens, and it widens for one host that is not an MCP server's. -//! * `plan_lock` (`run/gate.rs:194`, `pub(super)`) denies `write` and `exec` outright +//! * `plan_lock` (`run/gate.rs:237`, `pub(super)`) denies `write` and `exec` outright //! while a plan is unreviewed — but it is merged inside the step loop -//! (`run/step.rs:637`, `run/tree.rs:312`), *after* the servers have connected, so it +//! (`run/step.rs:750`, `run/tree.rs:331`), *after* the servers have connected, so it //! gates the tools an MCP server offers and never the server's own spawn. //! //! So this module can be wrong in one direction — it can say a server starts where a diff --git a/src/provider.rs b/src/provider.rs index 41acc92..01419dc 100644 --- a/src/provider.rs +++ b/src/provider.rs @@ -246,7 +246,7 @@ impl Provider for Vendor { /// /// **`last_served` answers `None` when the head served, and that is a deliberate /// difference from `Fallback`.** io-harness emits `EventKind::FellBackTo` for any -/// `Some` (`run/step.rs:503`), while `Fallback::last_served` answers `Some` for its +/// `Some` (`run/step.rs:577`), while `Fallback::last_served` answers `Some` for its /// *primary* too — so a chain built from that type would report "the provider fell /// over" on every step of every run, including the ones where nothing did. Reporting /// only a link that is not the head makes the event mean what its name says, and diff --git a/src/providers.rs b/src/providers.rs index e12275d..dab183b 100644 --- a/src/providers.rs +++ b/src/providers.rs @@ -37,7 +37,7 @@ //! only [`At::of`] builds — by counting entries in the file's own bytes. //! //! `[[provider]]` is deliberately **not** one of io-harness's appending keys -//! (`config.rs:2052`): the winning scope replaces the chain whole, "because a +//! (`config.rs:3130`): the winning scope replaces the chain whole, "because a //! half-appended fallback chain is not a chain". So exactly one file decides the //! whole array, and [`declared_at`] finds that file through [`decided`] — the //! same origin [`chain`] already reads the credentials out of, so the position @@ -46,7 +46,7 @@ //! # Exactly one of `preset` and `base_url` //! //! io-harness refuses a `compatible` entry that names both or neither, by index, -//! at load (`config.rs:456`). [`crate::configure::write`] would catch it on the +//! at load (`config.rs:571`). [`crate::configure::write`] would catch it on the //! round trip and roll back, which is a good failure — but [`add`] takes an //! [`Endpoint`] whose two `compatible` shapes are separate variants, so the entry //! that fails cannot be constructed at all. @@ -633,7 +633,7 @@ impl Key<'_> { /// checked against, and a list nothing can disagree with is decoration that goes /// stale in silence — the same argument [`PRESETS`] answers by being provable. /// The derivation matches what io-harness's own documentation writes -/// (`config.rs:430` spells `${env:GROQ_API_KEY}`), and it is an *offer*: a +/// (`config.rs:546` spells `${env:GROQ_API_KEY}`), and it is an *offer*: a /// vendor whose variable is spelled differently is one [`Key::Indirect`] away, /// and [`variable_is_set`] means a name nobody's shell carries is never the /// default that gets taken. @@ -683,7 +683,7 @@ pub fn variable_is_set(variable: &str) -> bool { /// a different vendor is a different link, and [`remove`] plus [`add`] says so /// in words. Allowing them here would also make the one entry io-harness /// refuses expressible again: a `preset` written onto a `base_url` entry names -/// both bases, which fails at load by index (`config.rs:456`) — a loud failure, +/// both bases, which fails at load by index (`config.rs:571`) — a loud failure, /// but a loud failure produced by a control that looked like renaming a field. /// - `auth`, `name`, `reference_prices` — nothing asks for them, and the last /// turns on an outbound request to a host the file did not name. A control diff --git a/src/recall.rs b/src/recall.rs index 8e21c84..86e6727 100644 --- a/src/recall.rs +++ b/src/recall.rs @@ -17,7 +17,7 @@ //! **The operator's two levers** — [`pin`], [`forget`], [`unforget`] — are the //! only writes here, and every one of them is an *act*, reached from a keystroke //! and never from a draw. io-harness says so itself: `memory_pin` is documented -//! as a caller's act and never a run's (`src/state.rs:1764-1767`), because the +//! as a caller's act and never a run's (`src/state.rs:2016-2019`), because the //! whole point of a pin is that it survives what the agent does next. Pinning is //! also what makes the caps survivable — an entry an operator pinned is exempt //! from being overwritten by a run (`src/state/memory.rs:430`) and from being @@ -36,7 +36,7 @@ //! **1. The bucket is a canonicalised path, and io-cli has to canonicalise it //! itself.** io-harness computes the key in `memory_key()` at //! `src/run/memory.rs:14-19` — `std::fs::canonicalize(root)`, falling back to the -//! path as given when that fails — and the function is `pub(super)`, so it cannot +//! path as given when that fails — and the function is `pub(crate)`, so it cannot //! be called from here. [`workspace_key`] reproduces it exactly, fallback //! included. This matters more than it looks: a checkout reached through a //! symlink (`/var` on macOS resolves to `/private/var`, a home-directory link to @@ -47,7 +47,7 @@ //! green. //! //! **2. There are two buckets.** The workspace's own, and the literal -//! [`GLOBAL_MEMORY_WORKSPACE`] — `""`, `src/state.rs:2833` — which holds +//! [`GLOBAL_MEMORY_WORKSPACE`] — `""`, `src/state.rs:3112` — which holds //! what the agent believes everywhere. Both are listed, and every row carries the //! [`Scope`] it came from, because "is this true here, or true everywhere" is the //! only question the two scopes exist to answer and a merged list destroys it. @@ -61,7 +61,7 @@ //! //! **4. Eviction, pin-refusal and recall emit no `EventKind` at all.** io-harness //! records all three as [`io_harness::ContextEvent`] rows deliberately — -//! `src/state.rs:2996-3002` spells out why: the question they answer +//! `src/state.rs:3275-3281` spells out why: the question they answer //! (*did my pin hold?*) is asked //! afterwards by somebody reading the store, not during the run by an observer. //! A *write* does emit `EventKind::MemoryWrote`, which is exactly what makes the @@ -74,18 +74,18 @@ //! # The one thing that makes a naive *write* wrong //! //! **There are two removals in io-harness and only one of them can be undone.** -//! [`Store::memory_delete`] (`src/state/memory.rs:863`) is a bare +//! [`Store::memory_delete`] (`src/state/memory.rs:896`) is a bare //! `DELETE FROM memory` — the embedder's blunt removal, with no restore point //! and no tidying of the evidence the entry accrued. [`Store::memory_forget`] -//! (`src/state/memory.rs:821`) writes a `memory_snapshots` row *before* it +//! (`src/state/memory.rs:853`) writes a `memory_snapshots` row *before* it //! removes anything and then clears the key's `memory_recalls` -//! (`src/state/memory.rs:838-855`). +//! (`src/state/memory.rs:887-890`). //! //! [`forget`] uses the second, and the difference is not stylistic. io-cli //! already reports `memory_restored` / `memory_removed` on its undo path //! (`src/rewind.rs:89-92`, `:219-220`, `:294-301`), and that path is //! `io_harness::rewind_run`, which puts entries back **only** from -//! `memory_snapshots` (`src/run.rs:749`, `:761-777`). An entry removed through +//! `memory_snapshots` (`src/run.rs:823`, `:835-851`). An entry removed through //! `memory_delete` has no row there, so nothing can put it back — and the //! operator who forgot the wrong key finds that out at the one moment they //! wanted it. The two implementations are indistinguishable from the store's @@ -109,7 +109,7 @@ use io_harness::{ /// /// The same ceiling, for the same reason, as [`crate::sessions::MAX_RUNS_SCANNED`]: /// io-harness exposes no public per-workspace draw count — `Store::memory_draws` -/// exists (`src/state/memory.rs:682`) and is `pub(crate)` — so the only public +/// exists (`src/state/memory.rs:713`) and is `pub(crate)` — so the only public /// path is [`Store::memory_recalls`], which is keyed by run. Counting how many /// distinct runs drew on an entry therefore costs one indexed query per run, and /// this bounds that cost. @@ -142,7 +142,7 @@ impl Scope { /// The key a workspace's durable memory is stored under. /// /// **A reproduction of io-harness's own `memory_key`** (`src/run/memory.rs:14-19`), -/// which is `pub(super)` and so unreachable from here. Both halves are +/// which is `pub(crate)` and so unreachable from here. Both halves are /// load-bearing and both are copied on purpose: /// /// - `canonicalize`, so the same directory reached by two different paths is one @@ -164,9 +164,9 @@ pub fn workspace_key(root: &Path) -> String { /// The stored spelling of a memory kind. /// -/// `MemoryKind::as_str` is **private** in io-harness (`src/state.rs:1812`), so the +/// `MemoryKind::as_str` is **private** in io-harness (`src/state.rs:2066`), so the /// two words are spelled here. The enum is `#[non_exhaustive]` -/// (`src/state.rs:1800`) — the crate documents a third kind it intends to add — +/// (`src/state.rs:2054`) — the crate documents a third kind it intends to add — /// so the wildcard arm is required rather than defensive, and it says *unknown* /// rather than guessing `"fact"`: a kind this build cannot name is not a fact, it /// is a row written by a newer harness than the one this binary was compiled @@ -202,7 +202,7 @@ pub struct Remembered { /// How many **distinct runs** have drawn on it. /// /// Distinct runs and not recall rows, matching the evidence io-harness itself - /// evicts by (`src/state/memory.rs:639-644`): a recall row is written once per + /// evicts by (`src/state/memory.rs:686-687`): a recall row is written once per /// carried key per *step*, so counting rows would let one two-hundred-step run /// outvote fifty runs that each leaned on the note once, and would make the /// number monotone in age rather than in usefulness. @@ -263,7 +263,7 @@ impl Happened { } } - /// The kind string io-harness stores, `src/state.rs:2982-3009`. + /// The kind string io-harness stores, `src/state.rs:3261-3290`. fn of(kind: &str) -> Option { match kind { "memory_evict" => Some(Happened::Evicted), @@ -297,7 +297,7 @@ pub struct View { pub workspace: String, /// Every note in both buckets: the workspace's, then the global one, each in /// the order io-harness returns it (`created_at ASC, key ASC`, - /// `src/state/memory.rs:764`). + /// `src/state/memory.rs:796`). pub entries: Vec, /// The caps, one row per scope. See the module note, trap 3. pub caps: Vec, @@ -431,7 +431,7 @@ pub fn trace(store: &Store, run_id: i64) -> Result, Error> { /// Which runs have drawn on each `(bucket, key)`, and whether the scan was cut. /// /// io-harness's own per-workspace answer, `Store::memory_draws` -/// (`src/state/memory.rs:682`), is `pub(crate)`. The public surface is +/// (`src/state/memory.rs:713`), is `pub(crate)`. The public surface is /// [`Store::memory_recalls`], which is keyed by run — so the count has to be /// assembled from the other side, one indexed query per run, bounded by /// [`MAX_RUNS_SCANNED`]. @@ -480,7 +480,7 @@ const FORGET_GOAL: &str = "an operator withdrew one memory entry"; /// /// Deliberately **not** [`io_harness::SUCCESS_OUTCOME`]: `Store::finish_run` /// (`src/state/runs.rs:145`) hands the string to `write_summary`, which stores -/// `success = (outcome == "success")` (`src/state/memory.rs:151-180`) — so a +/// `success = (outcome == "success")` (`src/state/memory.rs:160-189`) — so a /// bookkeeping row named `"success"` would quietly inflate every success rate /// computed off this store. Any other string still marks the run `completed`, /// which is the half that matters: a run left `running` is a *resumable* run, @@ -498,9 +498,9 @@ const FORGET_STEP: u32 = 0; /// What pinning or unpinning one entry did. /// /// Two variants and not a `bool`, because `Store::memory_pin` returns -/// `n > 0` from an `UPDATE` (`src/state/memory.rs:562-569`) and `false` there +/// `n > 0` from an `UPDATE` (`src/state/memory.rs:593-598`) and `false` there /// means *there was no such entry* — not "the pin failed". io-harness is explicit -/// that it will not invent one to pin (`src/state/memory.rs:556-558`). A `bool` +/// that it will not invent one to pin (`src/state/memory.rs:569-570`). A `bool` /// at a call site reads as "did it work", and the surface that believes it worked /// shows a pin the store does not hold. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -513,7 +513,7 @@ pub enum Pinned { /// What withdrawing one entry did. /// -/// The three outcomes of [`Store::memory_forget`] (`src/state.rs:1972-1982`), +/// The three outcomes of [`Store::memory_forget`] (`src/state.rs:2226-2235`), /// kept apart because **only one of them is success**. Collapsing them into a /// `bool` — or worse, reporting the refusal as a removal — tells an operator /// their note is gone while it sits in the store being carried into every later @@ -610,7 +610,7 @@ pub fn forget(store: &Store, root: &Path, scope: Scope, key: &str) -> Result= failures` - /// (`contract.rs:1813`), so zero is true at the first request of every run — + /// (`contract.rs:2057`), so zero is true at the first request of every run — /// the stronger model is used unconditionally, from the start, and /// `downshift_under` is never reached because escalation is checked first. An /// operator writing it means "escalate readily" and gets "never use the model @@ -354,9 +354,9 @@ pub fn notice(settings: &Settings) -> Option { /// Escalation is stated first because it is the rule that overrides the other, /// and the two sentences an operator needs after that are both io-harness's /// documented behaviour rather than io-cli's: escalation wins over downshifting -/// when both conditions hold (`contract.rs:1808`, and the ordering of the two -/// checks in `model_for` at `contract.rs:1812`), and it happens once and does not -/// come back down (`contract.rs:1753`) — because a run oscillating between two +/// when both conditions hold (`contract.rs:2052`, and the ordering of the two +/// checks in `model_for` at `contract.rs:2056`), and it happens once and does not +/// come back down (`contract.rs:1962`) — because a run oscillating between two /// models mid-flight is a behaviour nobody asked for. Both clauses are stated /// only when they can bite: a section with no escalation rule cannot escalate, /// and one with only an escalation rule has nothing for it to win over. diff --git a/src/servers.rs b/src/servers.rs index 24167e2..d2dc7fd 100644 --- a/src/servers.rs +++ b/src/servers.rs @@ -477,7 +477,7 @@ pub fn rows(servers: &[Server]) -> Vec { /// /// **The type exists so that a row number cannot be spelled as a position.** The /// two are the same integer often enough to look interchangeable — `[[mcp]]` is -/// *not* one of io-harness's appending keys (`config.rs:2052`), so the winning +/// *not* one of io-harness's appending keys (`config.rs:3130`), so the winning /// scope replaces the array whole and [`servers`] does list it in file order — /// and that is exactly the shape of a bug that survives every test written on a /// one-scope fixture. `pluginview` learned it the expensive way in 0.20.0. Here diff --git a/src/settings.rs b/src/settings.rs index 8e70a6f..3e11e7d 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -196,7 +196,7 @@ pub struct CliSettings { /// io-cli's own type rather than `io_harness::Routing`, which is the one place /// this section differs from `[app.io-cli.containment]`: `Containment` /// deserializes straight into the harness's type, and `Routing` derives no - /// serde at all and is `#[non_exhaustive]` besides (`contract.rs:1745`). So the + /// serde at all and is `#[non_exhaustive]` besides (`contract.rs:1954-1956`). So the /// shape an operator writes is [`crate::routing::Settings`] and the conversion /// goes through the harness's three builders. #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/src/status.rs b/src/status.rs index aa5b51d..3342ae1 100644 --- a/src/status.rs +++ b/src/status.rs @@ -818,7 +818,7 @@ impl Status { if event.depth > 0 { return; } - let io_harness::EventKind::ToolCall { name, target } = &event.kind else { + let io_harness::EventKind::ToolCall { name, target, .. } = &event.kind else { return; }; // **`target == name` is a call that named nothing, not a branch called diff --git a/src/undo.rs b/src/undo.rs index 8890b17..d310799 100644 --- a/src/undo.rs +++ b/src/undo.rs @@ -13,9 +13,9 @@ //! # The whole-run case changed too, and that is what makes two events reachable //! //! `src/rewind.rs` called the plain `rewind_run`, and **`EventKind::Rewound` is -//! emitted only inside `rewind_run_observed`** (`run.rs:804`). So in this +//! emitted only inside `rewind_run_observed`** (`run.rs:878`). So in this //! product's entire history that event has never fired once. The same is true of -//! `EventKind::Reverted`, which only `rewind_step_observed` emits (`run.rs:1011`). +//! `EventKind::Reverted`, which only `rewind_step_observed` emits (`run.rs:1085`). //! //! **What that buys is not a line on screen, and an earlier draft of this file //! claimed it was.** Both kinds are `Disposition::Silent` in `crate::triage` and diff --git a/src/upgrade.rs b/src/upgrade.rs new file mode 100644 index 0000000..136ca95 --- /dev/null +++ b/src/upgrade.rs @@ -0,0 +1,151 @@ +//! Where this binary came from, and the one command that updates it. +//! +//! Three install paths reach the same four artifacts — the scripts, a Homebrew +//! tap, and a scoop bucket — and each is updated by a different command. A +//! binary that cannot say which one placed it has made updating harder than the +//! single-path release it replaced. +//! +//! **This module contacts nothing and runs nothing.** It reads +//! [`std::env::current_exe`] and prints. Asking GitHub for the latest version +//! would need an HTTP client, which `tests/dependencies.rs` fails the build +//! over; running the upgrade would need a third permitted process spawn beside +//! `src/shell.rs` and `src/fetch.rs`, and widening that set — compared with `==` +//! precisely so it cannot widen itself — to save an operator a copy and paste is +//! the worst trade available here. The README's standing sentence, that this +//! product contacts no server it was not asked to, stays true because the +//! decision is taken from a path. +//! +//! The decision is a free function over a path rather than a read of the real +//! executable, because nothing under `tests/` links `src/main.rs` and a branch +//! only reachable from an installed binary is a branch no test can drive. The +//! driver supplies the path; everything decidable is here. + +use std::path::Path; + +/// How the running binary was installed, as far as its own path can say. +/// +/// `Unknown` is a real answer and not a failure. A binary copied to +/// `/usr/local/bin` by hand, installed by a distribution package, or built from +/// source is none of the three, and telling such an operator to re-run the +/// installer would be advice that replaces a binary the installer never placed. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Installed { + /// Under a Homebrew Cellar or prefix, on macOS or Linux. + Homebrew, + /// Under a scoop `apps` directory. + Scoop, + /// Where `install.sh` and `install.ps1` put it — `~/.local/bin`, or + /// `%LOCALAPPDATA%\io\bin`. + Installer, + /// Somewhere none of the three put it. + Unknown, +} + +/// The command that updates a Homebrew install. +pub const HOMEBREW: &str = "brew upgrade io"; + +/// The command that updates a scoop install. +pub const SCOOP: &str = "scoop update io"; + +/// The command that updates a script install on macOS or Linux. +pub const INSTALLER_UNIX: &str = + "curl -fsSL https://raw.githubusercontent.com/initorigin/io-cli/main/install.sh | sh"; + +/// The command that updates a script install on Windows. +pub const INSTALLER_WINDOWS: &str = + "irm https://raw.githubusercontent.com/initorigin/io-cli/main/install.ps1 | iex"; + +/// Which of the three placed the binary at `exe`. +/// +/// **Homebrew is recognised by `Cellar` and by the prefix both**, because +/// `current_exe` does not promise a resolved path. On Linux it comes from +/// `/proc/self/exe` and is fully resolved, so a tap install reads as +/// `…/Cellar/io//bin/io`; on macOS it can come back as the symlink in +/// `/opt/homebrew/bin`, which contains no `Cellar` component at all. Matching +/// either spelling costs one more arm and covers both platforms' truth. +/// +/// The comparison is on whole path components and never on a substring. A +/// directory called `scoop-notes` under `$HOME` is not a scoop install, and a +/// substring match is a classifier that widens itself — the same argument +/// `tests/dependencies.rs` makes for its permitted-path set. +#[must_use] +pub fn installed(exe: &Path) -> Installed { + let parts: Vec<&str> = exe + .components() + .filter_map(|part| part.as_os_str().to_str()) + .collect(); + let has = |name: &str| parts.contains(&name); + + if has("Cellar") || has("homebrew") || has(".linuxbrew") { + return Installed::Homebrew; + } + // scoop keeps every package under `/apps///`, and the + // root is relocatable through `$SCOOP` — so the marker is the pair, not the + // default location. `apps` alone is far too common a directory name to + // decide anything on. + if has("scoop") && has("apps") { + return Installed::Scoop; + } + if let Some(dir) = exe.parent() { + let ends_with = |a: &str, b: &str| { + let mut tail = dir + .components() + .rev() + .filter_map(|p| p.as_os_str().to_str()); + tail.next() == Some(b) && tail.next() == Some(a) + }; + // `install.sh` writes `$HOME/.local/bin` and `install.ps1` writes + // `%LOCALAPPDATA%\io\bin`. Both are matched by their last two + // components rather than by an absolute path, because `IO_INSTALL_DIR` + // moves the first and the second is under a variable this process would + // have to re-read to spell. + if ends_with(".local", "bin") || ends_with("io", "bin") { + return Installed::Installer; + } + } + Installed::Unknown +} + +/// What `io upgrade` prints for a binary at `exe`. +/// +/// The first line is the command, alone on its line so it can be copied without +/// picking prose out of it. Anything else is context beneath it. +#[must_use] +pub fn advice(exe: &Path, windows: bool) -> Vec { + let installer = if windows { + INSTALLER_WINDOWS + } else { + INSTALLER_UNIX + }; + match installed(exe) { + Installed::Homebrew => vec![ + HOMEBREW.to_string(), + String::new(), + "io was installed by Homebrew, from the tap in this repository.".to_string(), + ], + Installed::Scoop => vec![ + SCOOP.to_string(), + String::new(), + "io was installed by scoop, from the bucket in this repository.".to_string(), + ], + Installed::Installer => vec![ + installer.to_string(), + String::new(), + "io was installed by the script; re-running it is how it updates.".to_string(), + ], + // Named as a limit of what a path can tell rather than as a default, so + // an operator whose binary is somewhere unexpected is told that this + // command did not recognise it — which is a different statement from + // "use the installer", and the only honest one available. + Installed::Unknown => vec![ + installer.to_string(), + String::new(), + format!( + "io is at {}, which is not where Homebrew, scoop or the \ + installer put a binary — so this is a guess. If a package \ + manager placed it, update it with that.", + exe.display() + ), + ], + } +} diff --git a/tests/acp.rs b/tests/acp.rs index 071bbf9..64e033c 100644 --- a/tests/acp.rs +++ b/tests/acp.rs @@ -388,6 +388,7 @@ fn f4_every_row_claims_what_the_translator_actually_sends() { EventKind::ToolCall { name: "read_file".into(), target: "src/main.rs".into(), + origin: None, }, "tool_call", "tool_call", @@ -495,6 +496,7 @@ fn f3_every_outcome_maps_to_one_of_the_five_acp_stop_reasons() { io_harness::RunOutcome::Refused { steps: 1 }, io_harness::RunOutcome::Cancelled { steps: 1 }, io_harness::RunOutcome::Finished { steps: 1 }, + io_harness::RunOutcome::SchemaUnsatisfied { steps: 1 }, ]; // The list above is hand-built because `RunOutcome` is an enum with no @@ -793,7 +795,8 @@ fn f5_a_cancel_reaches_the_observer_and_silences_it_at_once() { use std::sync::atomic::Ordering; let (updates, mut drain) = tokio::sync::mpsc::unbounded_channel(); - let (reporter, cancelled) = acp::Reporter::new("s-7", updates); + let (reporter, shared) = acp::Reporter::new("s-7", updates); + let cancelled = shared.cancelled; // Before the cancel: a token is a message chunk and the run continues. let token = RunEvent::new( @@ -846,7 +849,7 @@ fn a_no_op_kind_produces_no_frame_at_all() { use io_harness::Observer; let (updates, mut drain) = tokio::sync::mpsc::unbounded_channel(); - let (reporter, _cancel) = acp::Reporter::new("s-8", updates); + let (reporter, _shared) = acp::Reporter::new("s-8", updates); assert_eq!(acp_map::update_for("dialed"), Some(Update::None)); reporter.event(&RunEvent::new( @@ -870,17 +873,38 @@ fn a_no_op_kind_produces_no_frame_at_all() { /// 'static` — a `&mut Vec` is neither. A `duplex` gives an owned half that is /// both, and it is closer to what a client actually is than a buffer: it can fill, /// and a writer that never flushed would hang here rather than silently passing. +/// +/// **Since 0.38.0 the reader moves onto a task too**, so the input is a duplex as +/// well rather than the `&[u8]` this helper used to borrow from its argument. A +/// borrowed slice is not `'static` either, and the bound is the visible cost of +/// the fix that lets a permission be answered at all: a reader parked inside the +/// turn cannot read the answer that turn is waiting for. async fn drive( input: &str, handler: &mut impl acp::Handler, outbound: tokio::sync::mpsc::UnboundedReceiver, ) -> Vec { - use tokio::io::AsyncReadExt as _; + use tokio::io::{AsyncReadExt as _, AsyncWriteExt as _}; let (theirs, ours) = tokio::io::duplex(64 * 1024); - acp::serve_with(input.as_bytes(), ours, handler, outbound) + let (mut client, agent) = tokio::io::duplex(64 * 1024); + client + .write_all(input.as_bytes()) .await - .expect("the loop ends on end-of-input, not on an error"); + .expect("the fixture fits the pipe"); + // Dropping the client's half is end-of-input, which is how an ACP session + // ends. Without it the loop would wait for a line that is never coming. + drop(client); + + acp::serve_with( + tokio::io::BufReader::new(agent), + ours, + handler, + outbound, + std::sync::Arc::new(acp::Correlator::new()), + ) + .await + .expect("the loop ends on end-of-input, not on an error"); let mut written = String::new(); let mut theirs = theirs; @@ -1120,3 +1144,374 @@ fn the_served_methods_are_the_ones_this_release_implements() { "the dispatch table changed without this test being updated", ); } + +// --------------------------------------------------------------------------- +// 0.38.0 F9, F10, F11 — the permission round trip +// --------------------------------------------------------------------------- + +/// A scripted client on the other end of a real pipe. +/// +/// It reads whatever the adapter writes, answers the first +/// `session/request_permission` with `answer`, and hands back every frame it +/// saw. `answer` is the whole `result` object, so a test can send a selection, +/// a cancellation, or a shape the specification does not contain. +async fn scripted_client( + agent_reads: tokio::io::DuplexStream, + mut agent_writes: tokio::io::DuplexStream, + answer: Option, +) -> Vec { + use tokio::io::{AsyncBufReadExt as _, AsyncWriteExt as _}; + + let mut agent_reads = agent_reads; + let mut seen = Vec::new(); + let mut lines = tokio::io::BufReader::new(&mut agent_writes).lines(); + + while let Ok(Some(line)) = lines.next_line().await { + let frame: Value = + serde_json::from_str(&line).expect("a written line is one JSON document"); + let method = frame.get("method").and_then(Value::as_str).unwrap_or(""); + let is_request = frame.get("id").is_some() && !method.is_empty(); + seen.push(frame.clone()); + + if is_request && method == "session/request_permission" { + let Some(result) = answer.clone() else { + // Answering nothing and closing is a client that walked away + // mid-question, which must deny rather than hang. + break; + }; + let reply = serde_json::json!({ + "jsonrpc": "2.0", + "id": frame.get("id").cloned().unwrap_or(Value::Null), + "result": result, + }); + let mut line = serde_json::to_string(&reply).expect("the reply serialises"); + line.push('\n'); + let _ = agent_reads.write_all(line.as_bytes()).await; + let _ = agent_reads.flush().await; + // **Stop here rather than draining to end-of-input.** This half is + // the agent's *input*, so the agent only reaches end-of-input when it + // is dropped — and it is held by this function, which would be + // waiting for the agent to close its output first. Each side waiting + // for the other is a deadlock, and it is the fixture's, not the + // adapter's: the answer is already on the wire, which is everything + // this client exists to deliver. + break; + } + } + seen +} + +/// Drive one approval end to end and return what the run was told, beside every +/// frame the client saw. +/// +/// The approval is raised through `acp::ask_permission`, which is what +/// `Consulting::decide` calls — so this exercises the production path rather +/// than a re-implementation of it, while staying reachable without a store, a +/// provider or a session. +async fn one_approval(answer: Option) -> (io_harness::Decision, Vec) { + let (client_side, agent_side) = tokio::io::duplex(64 * 1024); + let (agent_out, client_in) = tokio::io::duplex(64 * 1024); + let (outbound, rx) = tokio::sync::mpsc::unbounded_channel(); + let correlator = std::sync::Arc::new(acp::Correlator::new()); + + // The loop, with a handler that is never reached: no frame in this test is + // addressed to it, and the point is that the *reader* settles the response + // without the loop having to. + let serving = { + let correlator = std::sync::Arc::clone(&correlator); + tokio::spawn(async move { + let mut handler = Silent; + let _ = acp::serve_with( + tokio::io::BufReader::new(agent_side), + agent_out, + &mut handler, + rx, + correlator, + ) + .await; + }) + }; + + let client = tokio::spawn(scripted_client(client_side, client_in, answer)); + + let decision = acp::ask_permission( + &correlator, + &outbound, + "sess-1", + io_harness::Act::Write, + "write", + "src/lib.rs", + "7-1", + ) + .await; + + drop(outbound); + let seen = client.await.expect("the scripted client finishes"); + serving.abort(); + (decision, seen) +} + +/// A handler that answers nothing. The permission tests drive the reader, not +/// the dispatch table. +struct Silent; + +impl acp::Handler for Silent { + async fn handle(&mut self, _incoming: acp::Incoming) -> Vec { + Vec::new() + } +} + +/// **F9 — the client is asked, and its answer decides the call.** +/// +/// **The assertion is the `Decision` the run receives, never that a frame was +/// sent.** 0.36.0 shipped `permission_params`, `PERMISSION_OPTIONS` and +/// `permission_answer` with no production caller and gates that asserted the +/// three against each other; a test that watched the request go out would repeat +/// that mistake one level up. What the run is handed is the only thing that +/// changes what happens. +#[tokio::test] +async fn f9_an_allow_once_answer_approves_the_call() { + let (decision, seen) = one_approval(Some(serde_json::json!({ + "outcome": "selected", + "optionId": "allow-once", + }))) + .await; + + assert!( + matches!(decision, io_harness::Decision::Approve { ref remember, .. } if remember.is_empty()), + "allow-once approves this call and remembers no rule: {decision:?}" + ); + + // And it really went out as a request rather than a notification — a + // notification is what 0.36.0 sent, and a client cannot answer one. + let request = seen + .iter() + .find(|frame| { + frame.get("method").and_then(Value::as_str) == Some("session/request_permission") + }) + .expect("the client was asked"); + assert!( + request.get("id").is_some(), + "a permission request without an id cannot be answered: {request}" + ); + assert_eq!( + request["params"]["toolCall"]["toolCallId"], "7-1", + "the request must name the cell the client already has" + ); +} + +/// **F9's negative control — the same run answered `deny` is denied.** +/// +/// Without this arm, an `ask_permission` that ignored the answer and approved +/// unconditionally would pass the test above. The pair is what makes the answer +/// load-bearing. +#[tokio::test] +async fn f9_a_deny_answer_denies_the_call() { + let (decision, _) = one_approval(Some(serde_json::json!({ + "outcome": "selected", + "optionId": "deny", + }))) + .await; + + let io_harness::Decision::Deny { reason } = decision else { + panic!("deny must deny: {decision:?}"); + }; + assert_eq!( + reason, + io_cli::approval::REFUSED_BY_OPERATOR, + "an operator who was asked and said no refused it; `NOT_ROUTED` would \ + say nobody was asked, which is a false statement about a person" + ); +} + +/// **F9 — `allow-session` remembers exactly this act on exactly this target.** +/// +/// The rule is built by `approval::decision`, which the session's own overlay +/// uses, so the two surfaces cannot mean different things by the same answer. A +/// second derivation here that remembered the act alone would allow every write +/// for the rest of the run — a permission the operator did not grant. +#[tokio::test] +async fn f9_an_allow_session_answer_remembers_this_act_on_this_target() { + let (decision, _) = one_approval(Some(serde_json::json!({ + "outcome": "selected", + "optionId": "allow-session", + }))) + .await; + + let io_harness::Decision::Approve { remember, .. } = decision else { + panic!("allow-session approves: {decision:?}"); + }; + assert_eq!(remember.len(), 1, "one rule, not a widening: {remember:?}"); + assert_eq!(remember[0].act, io_harness::Act::Write); + assert_eq!(remember[0].effect, io_harness::Effect::Allow); + assert_eq!( + remember[0].pattern, "src/lib.rs", + "the rule is this target and not the act alone" + ); +} + +/// **F10 — an answer that arrives while the turn is waiting is read.** +/// +/// **This is the release's structural property and the test would hang before +/// the fix rather than fail.** Through 0.37.0 `serve_with` awaited +/// `handler.handle(...)` inline in its read loop, and for a `session/prompt` +/// that call *is* the whole turn — so the client's answer to a question raised +/// by that turn sat unread in the pipe until the turn returned, and the turn was +/// waiting for the answer. A deadlock, not a slow path. +/// +/// The timeout is the assertion. `ask_permission` completing at all is only +/// possible if something read the response while it was outstanding, which is +/// the reader task; without it this future never resolves. +#[tokio::test] +async fn f10_an_answer_arriving_during_the_wait_is_read() { + let answered = tokio::time::timeout( + std::time::Duration::from_secs(10), + one_approval(Some(serde_json::json!({ + "outcome": "selected", + "optionId": "allow-once", + }))), + ) + .await; + + let (decision, _) = answered.expect( + "the answer was never read: the reader is parked behind the turn that is \ + waiting for it, which is the defect this release exists to fix", + ); + assert!(matches!(decision, io_harness::Decision::Approve { .. })); +} + +/// **F11 — a client that walks away mid-question denies rather than hangs.** +/// +/// The mechanism is `Correlator::abandon`: when the reader stops, every +/// outstanding request loses its sender, each waiter's `recv` fails, and the +/// failure is a denial. That is why this adapter needs no approval timeout — a +/// number invented here would have been too short for someone reading a diff and +/// indistinguishable from a hang if it were long enough not to be. +#[tokio::test] +async fn f11_a_client_that_never_answers_denies_rather_than_hangs() { + let answered = + tokio::time::timeout(std::time::Duration::from_secs(10), one_approval(None)).await; + + let (decision, _) = answered.expect("an unanswered approval must not hang the turn"); + let io_harness::Decision::Deny { reason } = decision else { + panic!("an unanswered approval denies: {decision:?}"); + }; + assert_eq!( + reason, + acp::NOT_ROUTED, + "nobody answered, so the model must not be told a human refused" + ); +} + +/// **F11 — an option id this adapter never offered is a denial.** +/// +/// A client that answers with something it was not offered has said something it +/// cannot have meant, and there is exactly one safe direction to be wrong in. +#[tokio::test] +async fn f11_an_unoffered_option_id_is_a_denial() { + let (decision, _) = one_approval(Some(serde_json::json!({ + "outcome": "selected", + "optionId": "allow-always-everywhere", + }))) + .await; + assert!( + matches!(decision, io_harness::Decision::Deny { .. }), + "an unoffered option must not approve: {decision:?}" + ); +} + +/// **F11 — a cancellation is a denial, not a pause.** +#[tokio::test] +async fn f11_a_cancelled_outcome_is_a_denial() { + let (decision, _) = one_approval(Some(serde_json::json!({ "outcome": "cancelled" }))).await; + assert!( + matches!(decision, io_harness::Decision::Deny { .. }), + "a cancelled approval must not approve: {decision:?}" + ); +} + +/// **F9 — the permission request names the cell the run is actually on.** +/// +/// **This is the release's own adversarial-review finding.** `Consulting` held an +/// `AtomicU32` it constructed at zero and only ever read — nothing stored it — so +/// every request named cell `{run_id}-0` whatever step raised it. Through 0.37.0 +/// that mis-addressed a `tool_call_update` on a failed call; from 0.38.0 it would +/// mis-address the dialog an operator acts on, attaching the question to a call +/// that had already finished. +/// +/// The step now comes from the observer, which is the only thing in the module +/// that sees one — an `Approver` is handed a `Request` carrying an act and a +/// target and no step at all. +/// +/// Asserted against `acp_map`'s own `toolCallId` for the same event rather than +/// against a literal, so the two cannot drift into two spellings of one id. +#[test] +fn f9_the_permission_cell_id_is_the_one_the_run_is_on() { + let updates = tokio::sync::mpsc::unbounded_channel().0; + let (reporter, shared) = acp::Reporter::new("sess-1", updates); + + // Nothing has been observed yet, so the run is on its first step. + assert_eq!(shared.step.load(std::sync::atomic::Ordering::Relaxed), 0); + + // Drive an event on a later step through the observer, exactly as a run does. + let event = RunEvent::new( + 7, + 4, + EventKind::ToolCall { + name: "write_file".into(), + target: "src/lib.rs".into(), + origin: None, + }, + ); + let _ = io_harness::Observer::event(&reporter, &event); + + assert_eq!( + shared.step.load(std::sync::atomic::Ordering::Relaxed), + 4, + "the observer did not record the step, so an approval raised now would \ + name the wrong cell", + ); + + // And the id built from it is the one the client already has for that call. + let drawn = acp_map::translate(&event).expect("a tool call draws an update"); + let announced = drawn["toolCallId"] + .as_str() + .expect("an announced cell has an id"); + assert_eq!( + format!( + "{}-{}", + event.run_id, + shared.step.load(std::sync::atomic::Ordering::Relaxed) + ), + announced, + "the approval would name a cell the client never saw announced", + ); +} + +/// **F11 — a response nobody is waiting for changes nothing.** +/// +/// The correlator drops it. That is the correct answer rather than a gap: the +/// protocol has no response to a response, so there is nothing to send back, and +/// a client that answers twice or invents an id must not be able to make this +/// adapter say anything. +#[tokio::test] +async fn f11_a_response_naming_an_unknown_id_is_dropped() { + let correlator = acp::Correlator::new(); + correlator.settle( + &serde_json::json!(9_999), + Ok(serde_json::json!({ "outcome": "selected" })), + ); + + // The real proof is that the next issued request still resolves normally — + // an implementation that panicked or poisoned its map on an unknown id would + // take the session down with it. + let (id, answer) = correlator.issue(); + correlator.settle( + &serde_json::json!(id), + Ok(serde_json::json!({ "outcome": "cancelled" })), + ); + assert!( + answer.await.is_ok(), + "the correlator still settles its own ids" + ); +} diff --git a/tests/adapt.rs b/tests/adapt.rs index 070dff8..fcc589f 100644 --- a/tests/adapt.rs +++ b/tests/adapt.rs @@ -40,6 +40,23 @@ fn file(root: &Path, rel: &str, text: &str) -> PathBuf { path } +/// Build an adapter and put it in place, which is what an **accepted** install is. +/// +/// **Two calls rather than one, because since 0.38.0 they are two acts.** +/// `adapt::generate` builds the adapter in a staging directory and touches `into` +/// on no path at all; `Staged::commit` is the swap, and the caller holding the +/// operator's answer is the one that runs it. Every test below that reads the +/// adapter is a test about an install somebody said yes to, and says so by calling +/// this; the ones that assert on a refusal call `generate` directly, because a +/// refusal never reaches the swap. The answer itself — declining leaves the +/// installed adapter byte-identical, accepting still applies the refresh — is +/// gated in `tests/marketplace.rs`, where the consent lives. +fn installed(bundle: &Path, name: &str, into: &Path) -> Result { + let written = adapt::generate(bundle, name, into)?; + written.staged.commit()?; + Ok(written) +} + /// The index shape `zeroonething/ultraship` publishes — one plugin at the root. const ONE_AT_THE_ROOT: &str = r#"{ "name": "ultraship", @@ -493,7 +510,7 @@ fn loaded(into: &Path) -> Plugins { /// that io wrote until 0.35.0. The generator still runs and the fixture still /// builds — and `dropped()` carries the whole bundle, because io-harness 0.74.0 /// refuses an absolute `skills` or `templates` in every scope, `Scope::User` -/// included (`io-harness-0.76.0/src/plugin.rs:1097`): a manifest contributes a +/// included (`io-harness-0.78.0/src/plugin.rs:1097`): a manifest contributes a /// directory it ships rather than one it points at somewhere else on the machine, /// since every `*.md` under it is read into the model's system prompt. #[test] @@ -507,7 +524,7 @@ fn f7_a_four_kind_bundle_becomes_a_manifest_io_harness_loads_with_nothing_droppe "rust-review", ); - let written = adapt::generate(&bundle, "rust-review", &into).expect("the adapter is written"); + let written = installed(&bundle, "rust-review", &into).expect("the adapter is written"); assert_eq!( written.manifest, @@ -626,7 +643,7 @@ fn f7_a_bundle_carrying_none_of_the_four_kinds_contributes_nothing() { ); let into = root.join("adapters/bare"); - adapt::generate(&bundle, "bare", &into).expect("a bundle carrying nothing still adapts"); + installed(&bundle, "bare", &into).expect("a bundle carrying nothing still adapts"); let plugins = loaded(&into); let plugin = plugins @@ -656,7 +673,7 @@ fn f8_the_generated_manifest_round_trips_and_an_invented_key_refuses_the_bundle( let (_dir, root) = clone(); let bundle = four_kinds(&root); let into = root.join("adapters/rust-review"); - adapt::generate(&bundle, "rust-review", &into).expect("the adapter is written"); + installed(&bundle, "rust-review", &into).expect("the adapter is written"); let manifest = into.join(PLUGIN_FILE); let written = std::fs::read_to_string(&manifest).expect("the manifest io wrote"); @@ -714,7 +731,7 @@ fn a_refused_refresh_leaves_the_installed_adapter_untouched() { let bundle = four_kinds(&root); let into = root.join("adapters/rust-review"); - adapt::generate(&bundle, "rust-review", &into).expect("the first install writes an adapter"); + installed(&bundle, "rust-review", &into).expect("the first install writes an adapter"); let skill = into.join("skills/review/SKILL.md"); let before = std::fs::read_to_string(&skill).expect("the adapter ships the bundle's skill"); @@ -751,6 +768,56 @@ fn a_refused_refresh_leaves_the_installed_adapter_untouched() { ); } +/// **A built adapter stands beside its destination and never in it, until it is +/// committed.** +/// +/// The property the whole consent ordering rests on, asserted one layer below the +/// consent. Until 0.38.0 `generate` ended by replacing `into`, so by the time a +/// disclosure was drawn there was nothing left for the operator to decide: a +/// declined refresh had already replaced the adapter a `[[plugin]]` entry named, +/// while `docs/guide/plugins.md` said in writing that it would not. +/// +/// Sabotage: put the `remove_dir_all` and `rename` back at the end of `generate`. +/// The first assertion here fails, and so does F13 in `tests/marketplace.rs` — +/// this is the site that says which line the answer depends on. +#[test] +fn a_generated_adapter_stands_beside_its_destination_until_it_is_committed() { + let (_dir, root) = clone(); + let bundle = four_kinds(&root); + let into = root.join("adapters/rust-review"); + + let written = adapt::generate(&bundle, "rust-review", &into).expect("the adapter is built"); + + assert!( + !into.exists(), + "`generate` wrote the destination. Nobody has consented to anything yet, and on \ + an update the adapter the operator already had would be gone by now", + ); + assert!( + written.staged.staging.join(PLUGIN_FILE).is_file(), + "the adapter is nowhere at all: not at its destination and not staged either", + ); + assert_eq!( + written.staged.into, into, + "and it carries where it goes, or the caller holding the answer cannot perform it", + ); + + written + .staged + .commit() + .expect("the adapter is put in place"); + + assert!( + into.join(PLUGIN_FILE).is_file() && into.join("skills/review/SKILL.md").is_file(), + "the commit moves the whole adapter and not only the manifest", + ); + assert!( + !written.staged.staging.exists(), + "and the staging directory is gone: a half-built adapter under a path the \ + marketplace listing walks reads exactly like a working one", + ); +} + /// A server that is a script inside the bundle — named the only way a bundle can /// name one, because the bundle does not know where it will be cloned to. const PLUGIN_ROOT_SERVER: &str = r#"{ @@ -777,7 +844,7 @@ fn f9_the_one_expandable_substitution_is_written_out_and_no_dollar_brace_survive file(&bundle, ".mcp.json", PLUGIN_ROOT_SERVER); let into = root.join("adapters/local"); - adapt::generate(&bundle, "local", &into).expect("the adapter is written"); + installed(&bundle, "local", &into).expect("the adapter is written"); // Scanned in the bytes, because the bytes are what io-harness refuses. What // the generator meant to expand is not the question. @@ -873,7 +940,7 @@ fn every_directory_the_generated_manifest_names_is_one_plain_component() { let (_dir, root) = clone(); let bundle = four_kinds(&root); let into = root.join("adapters/rust-review"); - adapt::generate(&bundle, "rust-review", &into).expect("the adapter is written"); + installed(&bundle, "rust-review", &into).expect("the adapter is written"); let text = std::fs::read_to_string(into.join(PLUGIN_FILE)).expect("the manifest"); let named: Vec<&str> = text @@ -921,7 +988,7 @@ fn f7_the_adapter_ships_the_bundle_s_directories_rather_than_pointing_at_them() let bundle = four_kinds(&root); let into = root.join("adapters/rust-review"); - let written = adapt::generate(&bundle, "rust-review", &into).expect("the adapter is written"); + let written = installed(&bundle, "rust-review", &into).expect("the adapter is written"); assert_eq!(written.copied, ["skills", "templates"]); assert_eq!( @@ -954,7 +1021,7 @@ fn a_regenerated_adapter_replaces_what_it_holds_rather_than_merging_into_it() { let (_dir, root) = clone(); let bundle = four_kinds(&root); let into = root.join("adapters/rust-review"); - adapt::generate(&bundle, "rust-review", &into).expect("the adapter is written"); + installed(&bundle, "rust-review", &into).expect("the adapter is written"); assert!(into.join("skills/review/SKILL.md").is_file()); // The upstream edit: one skill withdrawn, another published. @@ -965,7 +1032,7 @@ fn a_regenerated_adapter_replaces_what_it_holds_rather_than_merging_into_it() { "# audit\n\nHow we audit.\n", ); - adapt::generate(&bundle, "rust-review", &into).expect("the adapter is regenerated"); + installed(&bundle, "rust-review", &into).expect("the adapter is regenerated"); assert!( !into.join("skills/review").exists(), @@ -1000,7 +1067,7 @@ fn a_skills_key_that_climbs_out_of_the_bundle_copies_nothing_and_contributes_not ); let into = root.join("adapters/greedy"); - adapt::generate(&bundle, "greedy", &into).expect("the bundle still adapts"); + installed(&bundle, "greedy", &into).expect("the bundle still adapts"); let plugin = Plugins::inspect(Scope::User, &into).expect("the manifest io wrote parses"); assert_eq!( diff --git a/tests/attach.rs b/tests/attach.rs index bf2d9e6..be7cbae 100644 --- a/tests/attach.rs +++ b/tests/attach.rs @@ -235,6 +235,7 @@ mod viewed { EventKind::ToolCall { name: name.to_string(), target: target.to_string(), + origin: None, }, ) } diff --git a/tests/configure.rs b/tests/configure.rs index 6323616..0bb2a92 100644 --- a/tests/configure.rs +++ b/tests/configure.rs @@ -718,7 +718,7 @@ fn f4_a_project_scoped_widening_is_refused_with_the_harness_s_sentence() { /// /// **Asserted on which FILE the refusal names, not merely that it refused.** The /// project sentence and the local sentence differ in exactly that clause -/// (`config.rs:2429`, `untrusted`), so a needle of "narrow it and never widen it" +/// (`config.rs:2605`, `untrusted`), so a needle of "narrow it and never widen it" /// alone would pass just as well against a write that had gone to `io.toml` — and /// then this test and `f4_a_project_scoped_widening_is_refused_with_the_harness_s_ /// sentence` would be one test written twice. @@ -1195,7 +1195,7 @@ fn f2_the_exec_modes_are_spelled_by_the_dependency() { assert_eq!(modes, vec!["read-only", "workspace-write", "full-access"]); // **In the user scope, because `Config::from_toml` parses as the PROJECT // scope and `full-access` is refused there.** io-harness's `PROJECT_WIDENING` - // (`config.rs:1759-1769`) refuses five (key, value) pairs in a committed file, + // (`config.rs:2425-2458`) refuses twelve (key, value) pairs in a committed file, // `sandbox.mode = "full-access"` among them, because `io.toml` arrives with a // `git clone`. Every mode io-cli offers is a mode io-harness accepts *somewhere*, // and that is what this asserts; where each one is legal is diff --git a/tests/context.rs b/tests/context.rs index fea531e..6f2bda9 100644 --- a/tests/context.rs +++ b/tests/context.rs @@ -890,7 +890,7 @@ fn f9_allow_reports_a_no_op_and_a_clear_names_what_returned() { /// **The defect this replaces was the safety lever being silently inert.** /// io-harness keeps an unknown mask name rather than rejecting it, deliberately, /// so a mask stays portable across builds with different cargo features -/// (`io-harness-0.76.0/src/tools/mod.rs:55-58`). That means `mask_gate` matches on +/// (`io-harness-0.78.0/src/tools/mod.rs:55-58`). That means `mask_gate` matches on /// the exact string and a misspelling withholds nothing — while the operator was /// told "calling it will be refused before anything starts" and `/context` drew it /// on the withheld row. `/context withhold Docx_Write` and the file gets written. diff --git a/tests/context_share.rs b/tests/context_share.rs index a68a70b..21531a9 100644 --- a/tests/context_share.rs +++ b/tests/context_share.rs @@ -307,6 +307,7 @@ fn only_a_committed_step_makes_the_line_read_the_trace() { EventKind::ToolCall { name: "read".into(), target: "src/lib.rs".into(), + origin: None, }, EventKind::Stalled, ] { diff --git a/tests/contract.rs b/tests/contract.rs index d997cb2..fb3dd23 100644 --- a/tests/contract.rs +++ b/tests/contract.rs @@ -91,7 +91,7 @@ fn discovered(files: &[(&str, &str)]) -> (tempfile::TempDir, Config) { /// /// **Not `support::user_scope`**, and the difference is the `files` argument: a /// contract reads `[instructions]` off disk, relative to the *discovery root* -/// (`config.rs:2673`), so the workspace still has to hold `GUIDE.md` while the +/// (`config.rs:2904`), so the workspace still has to hold `GUIDE.md` while the /// configuration lives outside it. `support::user_scope` hands back an empty /// workspace by design. /// @@ -1829,7 +1829,7 @@ fn f5_servers_in_both_scopes_are_merged_and_a_collision_is_named() { /// **F7 — a tilde is a home directory, never a directory named `~`.** /// /// io-harness substitutes `${env:…}` and `${file:…}` and nothing else — there is -/// no tilde branch anywhere in `io-harness-0.76.0/src/config.rs` — so a `~` an +/// no tilde branch anywhere in `io-harness-0.78.0/src/config.rs` — so a `~` an /// operator writes in `[run] skills` reaches `Skills::discover` verbatim and the /// harness looks inside a directory whose name is one character long. The /// operator's skills sit exactly where they said they would, and the session diff --git a/tests/cost.rs b/tests/cost.rs index be0eea5..9a9c484 100644 --- a/tests/cost.rs +++ b/tests/cost.rs @@ -719,6 +719,188 @@ fn f10_a_fresh_figure_computed_from_a_floor_is_not_drawn_as_a_number() { ); } +/// The token split the two F12 tests below group over. +/// +/// A non-zero cache **read** is load-bearing rather than decoration, for the same +/// reason it is in the F10 fixtures: `section` draws the cache block only where +/// something about the cache is known, so a fixture with no reads and no reported +/// writes has no `of which cache written` row for the grouped row to be compared +/// against. +fn cached(cache_write: Option) -> Usage { + Usage { + prompt_tokens: 1_000, + completion_tokens: 100, + total_tokens: 1_100, + cache_read_tokens: 100, + cache_write_tokens: cache_write, + ..Default::default() + } +} + +/// One `by model` row, as a reader sees it. +/// +/// The grouped rows are keyed by model id, so the fixture's own model name is the +/// label — and `field` takes the first row carrying it, which on this page is the +/// `by model` row rather than anything in the two total sections above it. +fn grouped_row(calls: &[ProviderCall]) -> String { + field(&page(&seeded(calls), &table(), &provenance()), PRICED).to_string() +} + +/// **F12 — a grouped row states the cache-write floor, and names it as one.** +/// +/// `Total`'s section has drawn both floors since 0.36.0: the money is a floor +/// where a model has no rate, and the cache-write count is a floor where a call +/// reported usage without a write counter. The rows under `by model` and `by day` +/// drew only the money one, so the same silent call that made the section say +/// "or more" left the row below it reading like a complete measurement. +/// +/// Reachable rather than theoretical, and the grouped rows are where it is most +/// likely: `openai_wire` reports no cache-write count at all while Anthropic +/// reports one only where a cache was created, and `by day` groups a day that used +/// both vendors into one row. +/// +/// The three arms are the three states, and the zero arm is what makes the test +/// able to fail in the other direction: a row that appended the qualifier +/// unconditionally would put "or more" over a group every call of which reported, +/// which is the same invented uncertainty as an invented number. +/// +/// Sabotage: append the qualifier without its label — a bare `1.0k` beside the +/// money — and the segment assertion fails by name while the `contains` arms would +/// not have noticed. That is the 0.36.0 defect exactly: a figure drawn on this +/// page without saying which direction it errs in. +#[test] +fn f12_a_grouped_row_states_the_cache_write_floor() { + let spelled = io_cli::status::format_tokens; + + let mixed = grouped_row(&[ + call(Some(PRICED), Some(cached(Some(1_000)))), + call(Some(PRICED), Some(cached(None))), + ]); + assert!( + mixed.contains(&format!("cache written {} or more", spelled(1_000))), + "a group holding one silent call drew its cache writes as a count:\n{mixed}", + ); + + // **The label is the assertion, not decoration.** The row already carries + // money, a call count and a token total, and a fourth figure appended bare + // reads as a fifth measurement rather than as a qualification of one of them. + // Splitting on the row's own separator is how the test can tell a labelled + // floor from a number sitting beside the money. + let floor: Vec<&str> = mixed + .split(" · ") + .filter(|part| part.contains("or more")) + .collect(); + assert_eq!( + floor.len(), + 1, + "the floor is not on the row exactly once:\n{mixed}", + ); + assert!( + floor[0].starts_with("cache written "), + "the row draws a floor beside the money without saying which figure it \ + qualifies:\n{mixed}", + ); + + // Every call reported: nothing is in doubt, so nothing is said. A qualifier + // here would claim an uncertainty the group does not have. + let all_reported = grouped_row(&[ + call(Some(PRICED), Some(cached(Some(250)))), + call(Some(PRICED), Some(cached(Some(750)))), + ]); + assert!( + !all_reported.contains("cache written"), + "a group whose every call reported a cache-write count was qualified \ + anyway:\n{all_reported}", + ); + assert!( + !all_reported.contains("or more"), + "a complete count was drawn as a floor:\n{all_reported}", + ); + // The control on that pair of negations: a `grouped` that drew an empty value, + // or a `field` that found the wrong row, would satisfy both of them forever. + assert!( + all_reported.contains("2 calls"), + "the row under test is not the group's row at all:\n{all_reported}", + ); + + // No call reported, so there is no number to put a floor under, and the row + // says the word rather than a figure — `unknown`, which is what `UNREPORTED` + // is and what the section above draws for the same fold. + let silent = grouped_row(&[ + call(Some(PRICED), Some(cached(None))), + call(Some(PRICED), Some(cached(None))), + ]); + assert!( + silent.contains("cache written unknown"), + "a group no call of which reported a cache-write count did not say \ + so:\n{silent}", + ); + assert!( + !silent.contains("or more"), + "a group with nothing reported drew a floor under a number it does not \ + have:\n{silent}", + ); + assert_ne!( + mixed, silent, + "a mixed group and a wholly silent one drew the same row, so the page \ + cannot tell an operator which one happened", + ); +} + +/// **F12's other half — one page, one vocabulary for one floor.** +/// +/// The grouped rows and the `Total` sections above them describe the same calls +/// out of the same store. io-harness's `Spend::unreported_cache_writes` and this +/// module's `Total::unreported_writes` count identically over every call that +/// reported usage, so a fixture whose only group is the run's own calls has one +/// fold drawn twice — and drawing it in two vocabularies would read as two +/// different facts about a single number. +/// +/// Asserted as containment of the section's own rendered value rather than against +/// a literal: a release that changed "or more" to something else in one place has +/// to change it in both or this fails, which is the property. A literal on both +/// sides would let the two surfaces drift together past the assertion. +/// +/// Sabotage: spell the grouped qualifier "at least 1.0k" or "unmeasured". Every +/// assertion in the test above still passes — they read the grouped row alone — +/// and this one fails. +#[test] +fn f12_the_grouped_row_and_the_total_say_the_floor_in_the_same_words() { + let both = |calls: &[ProviderCall]| { + let rows = page(&seeded(calls), &table(), &provenance()); + ( + field(&rows, "of which cache written").to_string(), + field(&rows, PRICED).to_string(), + ) + }; + + let (section_mixed, row_mixed) = both(&[ + call(Some(PRICED), Some(cached(Some(1_000)))), + call(Some(PRICED), Some(cached(None))), + ]); + let (section_silent, row_silent) = both(&[ + call(Some(PRICED), Some(cached(None))), + call(Some(PRICED), Some(cached(None))), + ]); + + // The two states have to be different words on the section before agreement + // with the rows below means anything: if both sections said "unknown", a + // grouped row that always said "unknown" would agree with both. + assert_ne!( + section_mixed, section_silent, + "the section itself does not distinguish a mixed fold from a silent \ + one, so agreement below it proves nothing", + ); + + for (section, row) in [(section_mixed, row_mixed), (section_silent, row_silent)] { + assert!( + row.contains(&format!("cache written {section}")), + "the grouped row and the total describe one fold in two \ + vocabularies: the section says `{section}` and the row reads:\n{row}", + ); + } +} + // --------------------------------------------------------------------------- // `money` // --------------------------------------------------------------------------- diff --git a/tests/docs.rs b/tests/docs.rs index c23e018..ddb99f5 100644 --- a/tests/docs.rs +++ b/tests/docs.rs @@ -2517,9 +2517,9 @@ fn f6_the_headless_guide_carves_out_the_provider_endpoint() { /// the risk is a sentence rather than a branch.** io-harness offers a masked turn a /// byte-identical catalogue on purpose: the tool array sits ahead of the provider's /// cache breakpoint, so dropping a definition would save its tokens once and pay a -/// cache *write* on every later turn (`io-harness-0.76.0/src/tools/mod.rs:33-42`). +/// cache *write* on every later turn (`io-harness-0.78.0/src/tools/mod.rs:33-42`). /// Withholding in fact makes the request marginally **larger**, by one sentence -/// naming what is withheld (`src/run/prompts.rs:976`). +/// naming what is withheld (`src/run/prompts.rs:1133`). /// /// The roadmap entry 0.37.0 was planned from assumed the opposite and said so in /// its headline. That framing is what a writer reaches for, because "withhold" means diff --git a/tests/events.rs b/tests/events.rs index 2d9aef0..5da2a89 100644 --- a/tests/events.rs +++ b/tests/events.rs @@ -173,6 +173,7 @@ fn f8_every_styled_kind_renders_its_own_facts() { &event(EventKind::ToolCall { name: "exec".into(), target: "cargo test".into(), + origin: None, }), Duration::ZERO, ); @@ -340,6 +341,7 @@ fn f8_a_line_kind_commits_one_though_a_token_and_a_tool_call_are_deferred() { EventKind::ToolCall { name: "read_file".into(), target: String::new(), + origin: None, }, EventKind::Refused { act: "a".into(), @@ -592,7 +594,8 @@ fn the_kind_name_is_the_serde_tag() { assert_eq!( kind_name(&EventKind::ToolCall { name: String::new(), - target: String::new() + target: String::new(), + origin: None }), "tool_call", ); @@ -694,6 +697,7 @@ fn f2_a_tool_call_commits_nothing_until_its_step_lands() { &event(EventKind::ToolCall { name: "read_file".into(), target: "src/lib.rs".into(), + origin: None, }), Duration::ZERO, ); @@ -721,6 +725,7 @@ fn f2_a_tool_cell_commits_its_result_and_its_observed_duration() { &event(EventKind::ToolCall { name: "read_file".into(), target: "src/lib.rs".into(), + origin: None, }), Duration::ZERO, ); @@ -784,6 +789,7 @@ fn f2_a_refused_call_does_not_close_as_a_completed_one() { &event(EventKind::ToolCall { name: "write_file".into(), target: "src/main.rs".into(), + origin: None, }), Duration::ZERO, ); @@ -835,6 +841,7 @@ fn f2_an_unfinished_call_closes_without_inventing_a_duration() { &event(EventKind::ToolCall { name: "read_file".into(), target: "src/lib.rs".into(), + origin: None, }), Duration::ZERO, ); @@ -865,6 +872,7 @@ fn f2_two_calls_before_one_step_each_close_with_their_own_duration() { &event(EventKind::ToolCall { name: "read_file".into(), target: "src/a.rs".into(), + origin: None, }), Duration::ZERO, ); @@ -872,6 +880,7 @@ fn f2_two_calls_before_one_step_each_close_with_their_own_duration() { &event(EventKind::ToolCall { name: "read_file".into(), target: "src/b.rs".into(), + origin: None, }), Duration::from_millis(100), ); @@ -1488,6 +1497,7 @@ fn f6_an_open_call_names_that_act_and_its_target() { &event(EventKind::ToolCall { name: "write_file".into(), target: "/work/io-cli/src/lib.rs".into(), + origin: None, }), Duration::ZERO, ); @@ -1540,6 +1550,7 @@ fn f6_a_pending_approval_outranks_an_open_call_and_a_thought() { &event(EventKind::ToolCall { name: "write_file".into(), target: "src/lib.rs".into(), + origin: None, }), Duration::ZERO, ); @@ -1604,6 +1615,7 @@ fn cell(events: &mut Events, name: &str, target: &str) -> String { &event(EventKind::ToolCall { name: name.into(), target: target.into(), + origin: None, }), Duration::ZERO, ); @@ -1711,6 +1723,7 @@ fn f4_the_result_says_what_it_adds_and_not_what_the_cell_already_said() { &event(EventKind::ToolCall { name: tool.into(), target: target.into(), + origin: None, }), Duration::ZERO, ); @@ -1841,6 +1854,7 @@ fn f8_a_designed_block_and_the_next_prompt_are_not_one_block() { &event(EventKind::ToolCall { name: "read_file".into(), target: "src/lib.rs".into(), + origin: None, }), Duration::ZERO, ); @@ -2723,6 +2737,7 @@ fn a_tool_targets_path_is_never_translated_however_it_is_spelled() { EventKind::ToolCall { name: "read_file".to_string(), target: path.to_string(), + origin: None, }, ); // The call is held open and committed by the `Step` that follows it, so @@ -2749,6 +2764,7 @@ fn a_skills_target_is_still_drawn_the_way_the_operator_reads_it() { &event(EventKind::ToolCall { name: io_cli::events::READ_SKILL.to_string(), target: "ultraship__brainstorm".to_string(), + origin: None, }), Duration::ZERO, ); @@ -2772,6 +2788,7 @@ fn skill_cell(target: &str, decision: &str) -> String { &event(EventKind::ToolCall { name: io_cli::events::READ_SKILL.to_string(), target: target.to_string(), + origin: None, }), Duration::ZERO, ); @@ -2867,6 +2884,7 @@ fn f11_a_companion_path_is_drawn_intact_and_the_skill_is_still_named() { &event(EventKind::ToolCall { name: io_cli::events::READ_SKILL.to_string(), target: path.to_string(), + origin: None, }), Duration::ZERO, ); @@ -2925,6 +2943,7 @@ fn f6_an_mcp_tool_cell_names_the_server_and_the_tool() { &event(EventKind::ToolCall { name: "mcp__github__create_issue".to_string(), target: "mcp__github__create_issue".to_string(), + origin: None, }), Duration::ZERO, ); @@ -2949,6 +2968,7 @@ fn f6_a_server_id_containing_the_separator_still_splits_at_the_first_join() { &event(EventKind::ToolCall { name: "mcp__deep__nested__tool".to_string(), target: "mcp__deep__nested__tool".to_string(), + origin: None, }), Duration::ZERO, ); @@ -2973,6 +2993,7 @@ fn an_mcp_calls_measured_duration_survives_the_translation() { &event(EventKind::ToolCall { name: "mcp__github__create_issue".to_string(), target: "mcp__github__create_issue".to_string(), + origin: None, }), Duration::ZERO, ); @@ -3134,7 +3155,7 @@ fn f5_the_echoed_line_reads_as_a_command_and_not_as_a_prompt() { /// never resolves, which is what an operator would actually see. /// /// io-harness feeds its own sentence back as the step's decision -/// (`"{tool} refused: withheld from this turn"`, `run/read.rs:1230`), so the cell +/// (`"{tool} refused: withheld from this turn"`, `run/read.rs:1318`), so the cell /// pairs to that rather than to a word io-cli invented. What this asserts is the /// pairing survives: one call, one decision, so `paired` holds and the cell /// carries the harness's sentence instead of falling through to the step verdict. @@ -3151,6 +3172,7 @@ fn f7_a_withheld_call_is_one_refused_cell_that_names_the_mask() { &event(EventKind::ToolCall { name: "docx_write".into(), target: "report.docx".into(), + origin: None, }), Duration::ZERO, ); diff --git a/tests/export.rs b/tests/export.rs index f554c92..f810349 100644 --- a/tests/export.rs +++ b/tests/export.rs @@ -45,7 +45,7 @@ fn fixture() -> (tempfile::TempDir, Store, Workspace) { /// /// **Every run records two steps, and that is not decoration.** The first /// version of this fixture recorded none, and `Store::canonical_trace` is built -/// entirely from `steps` and `context_events` (`state/trace.rs:1019-1036`) — so +/// entirely from `steps` and `context_events` (`state/trace.rs:1234-1251`) — so /// it returned the empty string, and the byte-for-byte gate below was comparing /// `""` against `""`. It passed, and it would have gone on passing over any /// defect whatsoever. diff --git a/tests/glyphs.rs b/tests/glyphs.rs index ea2eac4..2c69142 100644 --- a/tests/glyphs.rs +++ b/tests/glyphs.rs @@ -400,6 +400,7 @@ fn every_event_this_release_renders_draws_in_ascii() { EventKind::ToolCall { name: "read_file".into(), target: "src/main.rs".into(), + origin: None, }, ); assert!( diff --git a/tests/hooks.rs b/tests/hooks.rs index 22500a5..9ec8f3a 100644 --- a/tests/hooks.rs +++ b/tests/hooks.rs @@ -410,7 +410,7 @@ fn a_hook_naming_an_event_that_does_not_exist_is_refused_rather_than_installed() // --------------------------------------------------------------------------- // // `io_harness::Hooks::new` creates every `append` path it is given, empty, as the -// value is constructed (`io-harness-0.76.0/src/hooks.rs`) — deliberately, so that +// value is constructed (`io-harness-0.78.0/src/hooks.rs`) — deliberately, so that // "the filter matched nothing" stays distinguishable from "the hook was never // installed". That makes *constructing* a `Hooks` a write to the filesystem, and // it resolves a relative `append` against the directory it was handed. Handed diff --git a/tests/instructions.rs b/tests/instructions.rs index 2ce548b..ed77376 100644 --- a/tests/instructions.rs +++ b/tests/instructions.rs @@ -5,7 +5,7 @@ //! other half and `tests/memory.rs` covers it. This file covers the half that //! decides whether any of it reaches a model: `[instructions] files`, which is //! the only mechanism io-harness has for finding two of the three -//! (`io-harness-0.76.0/src/config.rs:190` — the automatic set is exactly +//! (`io-harness-0.78.0/src/config.rs:190` — the automatic set is exactly //! `["AGENTS.md"]`). //! //! Every test sets `IO_CONFIG_HOME`, because that is the only way to move the @@ -37,7 +37,7 @@ const SCOPES: [Scope; 3] = [Scope::User, Scope::Project, Scope::Local]; /// Point io's home at `home` and clear the variable that would win over it. /// /// `IO_CONFIG` names a file outright and beats `IO_CONFIG_HOME` -/// (`config.rs:1668-1673`), so a developer who has one exported would otherwise +/// (`config.rs:2253-2258`), so a developer who has one exported would otherwise /// have this suite writing into their own configuration. fn home_at(home: &Path) { std::env::remove_var(io_harness::config::CONFIG_VAR); @@ -77,13 +77,13 @@ fn row(view: &[Instruction], scope: Scope) -> &Instruction { /// says which one broke. /// /// `files` REPLACES io-harness's default rather than adding to it -/// (`config.rs:1879-1882`: `Some(files) => files.clone()`), so a list written to +/// (`config.rs:2918-2921`: `Some(files) => files.clone()`), so a list written to /// reach `AGENTS.local.md` and `IO.md` alone would silently stop the /// repository's own `AGENTS.md` being read — no error, because a name that -/// resolves to nothing is skipped in silence (`config.rs:1886`). +/// resolves to nothing is skipped in silence (`config.rs:2925`). /// /// And `IO.md` cannot be named relatively at all: every name is resolved -/// `root.join(&name)` against the discovery root (`config.rs:1885`), and io's +/// `root.join(&name)` against the discovery root (`config.rs:2924`), and io's /// home is not the workspace. #[test] fn the_list_names_the_committed_file_and_reaches_the_home_by_absolute_path() { @@ -192,7 +192,7 @@ fn installing_a_list_that_is_already_correct_writes_nothing() { /// with a `~`. /// /// io-harness does substitute `${env:…}`, and an unset variable is a **hard -/// error** that fails the whole parse (`config.rs:1983-1989`) — on Windows +/// error** that fails the whole parse (`config.rs:3022-3028`) — on Windows /// `HOME` frequently is unset, and a configuration that refuses to parse is a /// session that does not start. `~` is worse: nothing in `config.rs` expands /// one, so it would be taken as a directory literally named `~`. @@ -275,9 +275,9 @@ fn the_view_reports_what_each_file_holds_and_that_it_is_read() { /// read. /// /// `["instructions","files"]` is not in io-harness's `APPENDING` set -/// (`config.rs:2052`), so a later scope replaces the array wholesale rather than +/// (`config.rs:3130`), so a later scope replaces the array wholesale rather than /// adding to it — the scopes are merged in the order listed at -/// `config.rs:688-693` and a later value overwrites (`config.rs:2142-2144`), so +/// `config.rs:1078-1083` and a later value overwrites (`config.rs:3222-3224`), so /// Local beats Project beats User. /// This is the case a view inferring `read` from `exists` gets wrong, and it is /// the case the view exists for: the operator's `IO.md` is on disk, was @@ -328,7 +328,7 @@ fn a_project_list_replaces_the_user_one_and_the_unread_files_say_so() { /// absent rather than unread-for-some-other-reason. /// /// io-harness skips a named file that does not exist in silence -/// (`config.rs:1886`), so nothing downstream fails — which is exactly why the +/// (`config.rs:2925`), so nothing downstream fails — which is exactly why the /// view has to distinguish the two. #[test] fn a_named_file_that_is_absent_is_skipped_and_reported_absent() { @@ -363,7 +363,7 @@ fn a_named_file_that_is_absent_is_skipped_and_reported_absent() { /// A file that exists and holds nothing but whitespace is not read, and the view /// does not pretend it is. /// -/// `read_instructions` trims and skips an empty result (`config.rs:1891-1894`). +/// `read_instructions` trims and skips an empty result (`config.rs:2930-2933`). /// The row an operator sees is "there, and not reaching the model", which is the /// only wording that explains why nothing changed after they created it. #[test] diff --git a/tests/live.rs b/tests/live.rs index 9e9b17a..4569438 100644 --- a/tests/live.rs +++ b/tests/live.rs @@ -3630,7 +3630,7 @@ async fn live_f1_a_reasoning_level_reaches_the_wire_and_the_turn_still_finishes( /// /// The tail is the real provider, so a pass means an answer that actually came /// from the second link, and `last_served` names it. That name is also what -/// io-harness turns into `EventKind::FellBackTo` (`run/step.rs:503`), which is the +/// io-harness turns into `EventKind::FellBackTo` (`run/step.rs:577`), which is the /// scrollback arm this release makes reachable for the first time. #[tokio::test] #[ignore = "live: needs OPENROUTER_API_KEY"] @@ -4059,7 +4059,7 @@ async fn live_f6_a_withheld_tool_is_refused_by_the_mask_and_says_so() { // working mask, which is the finding worth keeping: io-harness *announces* the // mask in the user prompt — "Unavailable this turn — these tools are listed // above but calling one is refused and starts nothing: write_file" - // (`run/prompts.rs:976`) — so a compliant model never attempts the call and + // (`run/prompts.rs:1133`) — so a compliant model never attempts the call and // never produces the refusal. The run above said so in its own reasoning: "the // previous turns show write_file was refused, so I used a shell redirect // instead". That is the mask working at its best, not evidence of absence. @@ -4096,3 +4096,183 @@ async fn live_f6_a_withheld_tool_is_refused_by_the_mask_and_says_so() { result.outcome, ); } + +// --------------------------------------------------------------------------- +// 0.38.0 O4 — the ACP permission round trip, through the built binary +// --------------------------------------------------------------------------- + +/// Speak ACP to a real `io acp` child and answer its permission request. +/// +/// Returns `(the file the agent was asked to write exists, every frame read)`. +/// +/// **Through the binary, because nothing under `tests/` links `src/main.rs`.** +/// `io acp`'s door is opened in the driver, so an adapter that worked perfectly +/// in the library and was never reached by the subcommand would pass every +/// offline arm — which is the shape 0.30.0 shipped and 0.34.0 shipped again. +fn acp_permission_run(answer: &str) -> (bool, Vec) { + use std::io::{BufRead, BufReader, Write}; + use std::process::{Command, Stdio}; + + let config = tempfile::tempdir().expect("a config directory"); + let workspace = tempfile::tempdir().expect("a workspace"); + // **`write = "ask"` is the whole fixture.** A posture that allows writes + // raises no approval, so the arm would pass without the feature existing. + std::fs::write( + config.path().join("io.toml"), + format!( + "[[provider]]\nkind = \"openrouter\"\nmodel = {:?}\n\n\ + [policy.defaults]\nread = \"allow\"\nwrite = \"ask\"\n\ + exec = \"deny\"\nnet = \"deny\"\n", + model() + ), + ) + .expect("the configuration is written"); + + let mut child = Command::new(env!("CARGO_BIN_EXE_io")) + .arg("-C") + .arg(workspace.path()) + .arg("acp") + .env("IO_CONFIG", config.path().join("io.toml")) + .env("OPENROUTER_API_KEY", key()) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::inherit()) + .spawn() + .expect("the built binary serves acp"); + + let mut stdin = child.stdin.take().expect("the child takes frames"); + let stdout = BufReader::new(child.stdout.take().expect("the child writes frames")); + + let send = |stdin: &mut std::process::ChildStdin, value: serde_json::Value| { + let mut line = value.to_string(); + line.push('\n'); + stdin.write_all(line.as_bytes()).expect("a frame is sent"); + stdin.flush().expect("the frame is flushed"); + }; + + send( + &mut stdin, + serde_json::json!({ + "jsonrpc": "2.0", "id": 1, "method": "initialize", + "params": { "protocolVersion": 1, "clientCapabilities": {} }, + }), + ); + send( + &mut stdin, + serde_json::json!({ + "jsonrpc": "2.0", "id": 2, "method": "session/new", + "params": { "cwd": workspace.path(), "mcpServers": [] }, + }), + ); + + let mut seen: Vec = Vec::new(); + let mut prompted = false; + + for line in stdout.lines() { + let Ok(line) = line else { break }; + let Ok(frame) = serde_json::from_str::(&line) else { + continue; + }; + seen.push(frame.clone()); + + // `session/new`'s result carries the id the prompt has to name. + if frame.get("id") == Some(&serde_json::json!(2)) { + if let Some(id) = frame["result"]["sessionId"].as_str() { + send( + &mut stdin, + serde_json::json!({ + "jsonrpc": "2.0", "id": 3, "method": "session/prompt", + "params": { + "sessionId": id, + "prompt": [{ + "type": "text", + "text": "Create a file called approved.txt in this \ + directory containing the single word yes. \ + Use your file-writing tool.", + }], + }, + }), + ); + prompted = true; + } + } + + // The request this release exists to raise. Answering it is the test. + if frame.get("method") == Some(&serde_json::json!("session/request_permission")) { + send( + &mut stdin, + serde_json::json!({ + "jsonrpc": "2.0", + "id": frame["id"].clone(), + "result": { "outcome": "selected", "optionId": answer }, + }), + ); + } + + // The prompt's own result ends the turn. + if prompted && frame.get("id") == Some(&serde_json::json!(3)) { + break; + } + } + + drop(stdin); + let _ = child.wait(); + (workspace.path().join("approved.txt").exists(), seen) +} + +/// **0.38.0 O4 — an editor answers a permission request and the write happens.** +/// +/// **The assertion is that the write committed, not that a frame was sent.** +/// 0.36.0 shipped the frame builders with gates that asserted them against each +/// other, and a live arm that watched for the request would repeat that mistake +/// against a real provider. What changed for an operator is that answering yes +/// makes the file appear. +/// +/// The negative control is the sibling below, and it is what makes this arm mean +/// anything: a model that wrote the file without ever asking would pass this one +/// alone. 0.34.0 shipped exactly that shape — a live arm passing over a feature +/// that placed nothing, because the model reached the goal another way. +#[test] +#[ignore = "live"] +fn live_an_acp_client_that_allows_a_write_gets_the_write() { + let (wrote, seen) = acp_permission_run("allow-once"); + + let asked = seen + .iter() + .any(|f| f.get("method") == Some(&serde_json::json!("session/request_permission"))); + println!("asked for permission: {asked} wrote the file: {wrote}"); + + assert!( + asked, + "no `session/request_permission` was raised, so the posture never reached \ + the approver and this arm proves nothing", + ); + assert!( + wrote, + "the client allowed the write and it did not happen — the answer did not \ + reach the run", + ); +} + +/// **0.38.0 O4's negative control — denying stops the write.** +/// +/// Run with the same fixture and the same prompt; only the answer differs. If +/// this one also wrote the file, the answer is not deciding anything and the arm +/// above is passing on a model's own behaviour rather than on the feature. +#[test] +#[ignore = "live"] +fn live_an_acp_client_that_denies_a_write_stops_it() { + let (wrote, seen) = acp_permission_run("deny"); + + let asked = seen + .iter() + .any(|f| f.get("method") == Some(&serde_json::json!("session/request_permission"))); + println!("asked for permission: {asked} wrote the file: {wrote}"); + + assert!(asked, "no permission was raised, so nothing was denied"); + assert!( + !wrote, + "the client denied the write and the file exists anyway — the denial did \ + not reach the run", + ); +} diff --git a/tests/marketplace.rs b/tests/marketplace.rs index da73973..4abbdc2 100644 --- a/tests/marketplace.rs +++ b/tests/marketplace.rs @@ -1370,14 +1370,21 @@ fn install(file: &Path, work: &Path, markets: &[Market], word: &str) -> Result PathBuf { dir } -/// **Installing a bundle again is the update path, and it re-copies rather than -/// merges.** +/// Every file under `dir`, its path relative to `dir` and its bytes, sorted. +/// +/// **A tree as a value, and the bytes are the point.** F13 says an installed +/// adapter is *unchanged* after a refused refresh, and a check that only counted +/// names would pass a refresh that replaced every file with a different one of the +/// same name — which is precisely what a rebuilt adapter is. +fn tree(dir: &Path) -> Vec<(String, Vec)> { + let mut found = Vec::new(); + let mut stack = vec![dir.to_path_buf()]; + while let Some(at) = stack.pop() { + let Ok(entries) = std::fs::read_dir(&at) else { + continue; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + stack.push(path); + } else if let Ok(bytes) = std::fs::read(&path) { + let rel = path + .strip_prefix(dir) + .expect("under the root it was walked from"); + // Separators normalised: a Windows checkout walks the same tree + // with `\` in it, and a sorted comparison of two runs on one + // machine must not be the only thing this can assert. + found.push((rel.display().to_string().replace('\\', "/"), bytes)); + } + } + } + found.sort(); + found +} + +/// One install of `rust-review` out of the marketplaces under `homes`, up to the +/// point where the operator is asked. +/// +/// Nothing is in place when this returns: since 0.38.0 the adapter is staged and +/// `marketplace::make` or `marketplace::unmake` is what the answer performs. +fn built(homes: &Homes, markets: &[Market]) -> marketplace::Prepared { + marketplace::prepared( + markets, + "rust-review", + &homes.marketplaces, + &homes.staging, + &homes.adapters, + ) + .expect("the foreign bundle adapts") +} + +/// Whatever is left beside the adapter directory, which must be nothing. +/// +/// The staging directory is a sibling of the adapter and it is hidden, so nothing +/// lists it and no verb removes it: one that outlives an answer outlives every +/// answer, and `plugin marketplace` walks the tree it sits in. +fn strays(adapter: &Path) -> Vec { + let named = adapter.file_name().and_then(std::ffi::OsStr::to_str); + let mut found: Vec = std::fs::read_dir(adapter.parent().expect("a parent")) + .expect("the adapters directory is readable") + .filter_map(Result::ok) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .filter(|name| Some(name.as_str()) != named) + .collect(); + found.sort(); + found +} + +/// **F13 — declining a bundle re-install leaves the installed adapter +/// byte-identical.** +/// +/// Installing again is the update path (see F14), and until 0.38.0 +/// `adapt::generate` ended by swapping the rebuilt adapter into place — so by the +/// time the operator was shown the disclosure they were being asked about a +/// directory that had already been replaced, and "leave it" left them with the new +/// bundle and no way back to the old one. `docs/guide/plugins.md` has said in +/// writing since 0.35.0 that a refused refresh leaves the installed adapter +/// exactly as it was; this is that sentence, asserted. +/// +/// The withdrawn skill is what makes the assertion sharp: it is present in the +/// installed adapter and absent from the rebuilt one, so it exists in exactly one +/// of the two states and a decline that lands in the wrong one cannot look right. +/// +/// **F13 and F14 are a pair and neither is a gate alone.** An implementation that +/// makes declining safe by making accepting a no-op passes this test and fails +/// F14; one that keeps the old swap passes F14 and fails this. Read them together +/// or neither says anything. +/// +/// Sabotage: put the `remove_dir_all` and `rename` back at the end of +/// `adapt::generate`. `one.md` is gone and `two.md` has arrived under an adapter +/// nobody consented to. +/// +/// Second sabotage: make `marketplace::unmake` remove `Staged::into` rather than +/// `Staged::staging`. The tree comparison then fails against an adapter that is +/// not there at all, which is the older defect this replaced. +#[test] +fn f13_declining_a_re_install_leaves_the_installed_adapter_byte_identical() { + let (_dir, root) = clone_dir(); + let homes = Homes::under(&root); + let clone = homes.marketplaces.join("zeroonething").join("ultraship"); + let bundle = foreign(&clone, "rust-review"); + let markets = marketplace::markets(&homes.marketplaces); + + // The install the operator said yes to, last week. + let first = built(&homes, &markets); + marketplace::make(first.staged.as_ref()).expect("the accepted install is put in place"); + let installed = tree(&first.declare); + assert!( + installed + .iter() + .any(|(rel, _)| rel.as_str() == "skills/one.md"), + "the fixture carries no skill for the author to withdraw, so every assertion \ + below passes over an update that changes nothing", + ); + + // The author withdraws that skill and publishes another, and the operator + // pulls the clone and runs the install again to pick it up. + std::fs::remove_file(bundle.join("skills").join("one.md")).expect("the withdrawal"); + write( + &bundle, + "skills/two.md", + "---\nname: two\n---\nthe second\n", + ); + + let again = built(&homes, &markets); + assert_eq!( + again.declare, first.declare, + "the refresh is offered against a second directory, so the entry in the \ + operator's file names neither what they have nor what they are being shown", + ); + assert_eq!( + tree(&first.declare), + installed, + "the adapter changed before the operator was asked anything. Everything below \ + this line is about their answer, and by here there is nothing left to answer", + ); + + // And the answer is no. + marketplace::unmake(again.staged.as_ref()); + + assert_eq!( + tree(&first.declare), + installed, + "declining a refresh changed the installed adapter. The `[[plugin]]` entry \ + naming it was never opened, so what the operator declined is now what they \ + are running", + ); + assert!( + first.declare.join("skills").join("one.md").is_file() + && !first.declare.join("skills").join("two.md").exists(), + "the withdrawn skill is gone and the pulled one has arrived, which is the \ + accepted install (F14) and not the declined one", + ); + assert!( + strays(&first.declare).is_empty(), + "a declined refresh left {:?} beside the adapter: a staging directory that \ + outlives an answer is listed by no surface and removed by no verb", + strays(&first.declare), + ); +} + +/// **F14 — accepting a re-install still applies the refresh, and says what moved.** /// -/// Since 0.35.0 an adapter *ships* the `skills/` and `commands/` a foreign bundle +/// Installing a bundle again is the update path and it is the only one. Since +/// 0.35.0 an adapter *ships* the `skills/` and `commands/` a foreign bundle /// contributes — io-harness 0.74.0 refuses a `skills` or `templates` pointing out /// of the bundle in every scope, because every `*.md` under one reaches the model's /// system prompt. So what io holds is a snapshot, and an operator who pulls the /// clone has changed nothing this session can see until the install runs again. /// -/// Three things have to hold and each fails alone: the second install rebuilds the -/// adapter rather than adding to it, so a skill the author **withdrew** cannot -/// survive in io's home; the second install claims nothing to take back, so -/// declining it cannot delete an adapter a `[[plugin]]` entry already names; and -/// the operator is told which directories moved, in the disclosure they are already -/// reading, rather than discovering it a week later. -/// -/// Sabotage: have `adapt::generate` write over the adapter instead of rebuilding -/// it. `two.md` still arrives and every other assertion passes; the withdrawn -/// `one.md` is the one that fails, which is the file that would have gone on being -/// read into the prompt. -/// -/// Second sabotage: keep `made: Some(into)` for an adapter that was already there. -/// `unmake` then deletes a live install when the operator answers "leave it" to an -/// update — the entry stays in their file and io-harness drops it on the next turn. +/// Three things have to hold and each fails alone: the accepted install rebuilds +/// the adapter rather than adding to it, so a skill the author **withdrew** cannot +/// survive in io's home; it lands at the directory the `[[plugin]]` entry names, +/// rather than staying in the staging directory it was built in; and the operator +/// is told which directories moved, in the disclosure they are already reading, +/// rather than discovering it a week later. +/// +/// **The pair of F13, and the note there applies here.** A fix that makes +/// declining safe by making accepting do nothing passes F13 alone. +/// +/// Sabotage: have `adapt::generate` write over the staged adapter instead of +/// rebuilding it. `two.md` still arrives and every other assertion passes; the +/// withdrawn `one.md` is the one that fails, which is the file that would have gone +/// on being read into the prompt. +/// +/// Second sabotage: make `marketplace::make` a no-op. The refresh never leaves the +/// staging directory, F13 goes on passing, and this fails on both files at once. #[test] -fn a_second_install_re_copies_the_adapter_and_says_what_moved() { +fn f14_accepting_a_re_install_applies_the_refresh_and_says_what_moved() { let (_dir, root) = clone_dir(); let homes = Homes::under(&root); let clone = homes.marketplaces.join("zeroonething").join("ultraship"); let bundle = foreign(&clone, "rust-review"); - let markets = marketplace::markets(&homes.marketplaces); - let installed = |markets: &[Market]| { - marketplace::prepared( - markets, - "rust-review", - &homes.marketplaces, - &homes.staging, - &homes.adapters, - ) - }; - let first = installed(&markets).expect("the foreign bundle adapts"); + let first = built(&homes, &markets); assert_eq!( first.copied, ["skills", "templates"], "the adapter reports what it shipped in the generator's own words", ); - assert_eq!( - first.made.as_deref(), - Some(first.declare.as_path()), - "the first install created the adapter directory, so a decline takes it back", - ); + marketplace::make(first.staged.as_ref()).expect("the accepted install is put in place"); assert!( first.declare.join("skills").join("one.md").is_file() && first.declare.join("templates").join("go.md").is_file(), @@ -2805,35 +2958,24 @@ fn a_second_install_re_copies_the_adapter_and_says_what_moved() { "---\nname: two\n---\nthe second\n", ); - let again = installed(&markets).expect("the bundle adapts again"); + let again = built(&homes, &markets); assert_eq!( again.declare, first.declare, - "the update wrote a second adapter beside the first, so the declared entry \ + "the update built a second adapter beside the first, so the declared entry \ still names the old one", ); - assert!( - !first.declare.join("skills").join("one.md").exists(), - "a skill the author withdrew survived the update. An adapter is rebuilt and \ - never merged for exactly this reason: every `*.md` under a contributed \ - `skills/` is read into the model's system prompt on every turn", - ); - assert!( - first.declare.join("skills").join("two.md").is_file(), - "the skill the operator pulled did not reach the session", - ); - assert_eq!( - again.made, None, - "an update claims to have created the adapter, so declining one deletes a \ - bundle the operator installed some other day — out of a configuration file \ - this module never opened", - ); - // And what the operator is told before they answer. + // What the operator is told, read where the adapter actually is. `Chosen::read` + // is the one place that answer is worked out, and the disclosure is drawn from + // the refresh rather than from the install it replaces — on this path + // `again.declare` still holds last week's bundle. + let held = marketplace::Chosen::Held(again.clone()); let said = marketplace::adapted_disclosure( Scope::User, - &again.declare, - Some(&again.from), - &again.copied, + held.dir(), + held.read(), + Some(held.from()), + held.copied(), ) .expect("io-harness reads the generated manifest") .said(&io_cli::glyphs::UNICODE) @@ -2848,17 +2990,42 @@ fn a_second_install_re_copies_the_adapter_and_says_what_moved() { "both ends are named or the sentence cannot be acted on — the clone an edit \ would go into, and the directory io is actually reading: {said}", ); + assert!( + !said.contains(".staging."), + "the operator is sent to the staging directory, which is gone a moment after \ + they answer: {said}", + ); assert!( said.contains("installed again"), "the operator is told the copy is a snapshot but not what refreshes it, \ which is the whole question they will have: {said}", ); + // And the answer is yes. + marketplace::make(again.staged.as_ref()).expect("the accepted refresh is put in place"); + + assert!( + !first.declare.join("skills").join("one.md").exists(), + "a skill the author withdrew survived the update. An adapter is rebuilt and \ + never merged for exactly this reason: every `*.md` under a contributed \ + `skills/` is read into the model's system prompt on every turn", + ); + assert!( + first.declare.join("skills").join("two.md").is_file(), + "the skill the operator pulled did not reach the session — an accepted \ + refresh that changes nothing is what makes F13 free to pass", + ); + assert!( + strays(&first.declare).is_empty(), + "an accepted refresh left {:?} beside the adapter", + strays(&first.declare), + ); + // The control: a bundle that copied nothing says none of this. A row about a // copy that did not happen is a warning about nothing. let native = root.join("native"); manifest(&native, "name = \"native\"\n"); - let quiet = marketplace::adapted_disclosure(Scope::User, &native, Some(&native), &[]) + let quiet = marketplace::adapted_disclosure(Scope::User, &native, &native, Some(&native), &[]) .expect("io-harness loads the fixture") .said(&io_cli::glyphs::UNICODE) .join("\n"); @@ -2869,6 +3036,41 @@ fn a_second_install_re_copies_the_adapter_and_says_what_moved() { ); } +/// **The answer has a caller on both doors, and the caller is the driver.** +/// +/// `marketplace::make` and `marketplace::unmake` are the two things an operator's +/// answer performs, and nothing under `tests/` can reach either at the point it is +/// given: `src/main.rs` is the only file that holds an answer and no test links it. +/// F3 in `tests/dependencies.rs` names the shape this prevents — three releases +/// running shipped a unit that was correct behind a door that never opened it, and +/// a staged adapter nobody commits is a `plugin add` that installs nothing. +/// +/// **Counted, never `contains`.** Both doors install and only one of them can +/// decline: on the argv door naming a bundle *is* the consent, so it commits and +/// has no second answer; in the session the picker commits on "install it", +/// discards on "leave it", and discards again on `Esc` — the standard way out of a +/// modal must not be the one that leaves a staging directory behind. One site +/// satisfies a `contains` forever, and the site that would go missing is whichever +/// one was written second. +#[test] +fn both_install_doors_perform_the_answer_they_were_given() { + let driver = code_of("src/main.rs"); + assert_eq!( + driver.matches("marketplace::make(").count(), + 2, + "the swap an accepted install performs has a call site for each door — the \ + session's confirmation and `io plugin add` — and a door missing it writes a \ + `[[plugin]]` entry naming a directory the adapter was never moved into", + ); + assert_eq!( + driver.matches("marketplace::unmake(").count(), + 2, + "a declined install discards what it built at both ways out of the picker, \ + the confirmation row and `Esc`, or the staging directory outlives the \ + answer under a home nothing lists and no verb cleans", + ); +} + /// **A repository fetched for an index entry is not a marketplace of its own.** /// /// `marketplace::fetched` clones into `marketplaces/.entries//`, and diff --git a/tests/namespacing.rs b/tests/namespacing.rs index 0869166..6ced7d8 100644 --- a/tests/namespacing.rs +++ b/tests/namespacing.rs @@ -185,6 +185,7 @@ fn call(name: &str, target: &str) -> RunEvent { EventKind::ToolCall { name: name.into(), target: target.into(), + origin: None, }, ) } diff --git a/tests/packaging.rs b/tests/packaging.rs new file mode 100644 index 0000000..371d72e --- /dev/null +++ b/tests/packaging.rs @@ -0,0 +1,559 @@ +//! F1 — the formula and the manifest name the artifacts the release workflow builds. +//! F2 — both of them name one version, and it is a version this repository released. +//! F3 — every checksum in them is a distinct lowercase hex digest. +//! F4 — the generator is idempotent and writes those two files and nothing else. +//! F5 — the generator refuses a checksum file it cannot verify. +//! +//! `Formula/io.rb` and `bucket/io.json` restate, in two more files, a fact that +//! already lives in `.github/workflows/release.yml`: what the release builds and +//! what it is called. Nothing at install time compares them — `brew install` +//! fetches the URL the formula names and fails on the user's machine, days later, +//! if that artifact was never uploaded. So the comparison happens here. +//! +//! The version in those two files is deliberately the newest RELEASED one, which +//! during a release is one behind `CARGO_PKG_VERSION`: an artifact that has not +//! been uploaded has no checksum to name. F2 is therefore written against +//! `CHANGELOG.md`'s released headings and NOT against the crate version — a gate +//! spelled the other way would demand a digest that cannot exist yet, and the +//! release could not satisfy its own criterion. +//! +//! F4 and F5 run `scripts/update-tap.sh` against a `file://` release, the way +//! `tests/installers.rs` runs `install.sh`. The two files are staged into a +//! temporary tree first, so the run edits copies and the working tree it was +//! started from is never touched. + +#![cfg(unix)] + +use std::collections::{BTreeMap, BTreeSet}; +use std::path::{Path, PathBuf}; +use std::process::Command; + +/// The version the fixtures release. Not a version this repository has, so a run +/// that leaves the files unchanged cannot pass for a run that rewrote them. +const FIXTURE_VERSION: &str = "9.9.9"; + +const FORMULA: &str = "Formula/io.rb"; +const MANIFEST: &str = "bucket/io.json"; +const GENERATOR: &str = "scripts/update-tap.sh"; + +const DOWNLOADS: &str = "https://github.com/initorigin/io-cli/releases/download"; + +fn repo() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) +} + +/// Read a checked-in file with line endings normalised. +/// +/// git hands Windows a CRLF working copy, and every fragment matched below would +/// then be found at the wrong offset or not at all. +fn read(relative: &str) -> String { + std::fs::read_to_string(repo().join(relative)) + .unwrap_or_else(|error| panic!("{relative}: {error}")) + .replace("\r\n", "\n") +} + +/// The quoted strings on one line, in order. +fn quoted(line: &str) -> Vec<&str> { + line.split('"').skip(1).step_by(2).collect() +} + +/// `(target, archive extension)` for every row of the release matrix, read out of +/// the workflow rather than restated here — the workflow is what actually builds +/// them, so a row added or renamed there has to reach these two files. +fn matrix() -> Vec<(String, String)> { + let workflow = read(".github/workflows/release.yml"); + let mut rows = Vec::new(); + let mut target: Option = None; + for line in workflow.lines() { + let line = line.trim(); + if let Some(value) = line.strip_prefix("- target:") { + target = Some(value.trim().to_string()); + } else if let Some(value) = line.strip_prefix("archive:") { + if let Some(target) = target.take() { + rows.push((target, value.trim().to_string())); + } + } + } + assert_eq!( + rows.len(), + 4, + "the release matrix no longer has four rows; this test read {rows:?}", + ); + rows +} + +/// Every URL in a packaging file, with scoop's `$version` template resolved. +/// +/// The autoupdate block is a template for the NEXT release, so its URLs carry +/// `$version` rather than a number. Substituting it here is what lets the same +/// assertion cover the concrete URL and the template it will be replaced by. +fn urls(text: &str, version: &str) -> Vec { + text.lines() + .filter(|line| { + let line = line.trim(); + line.starts_with("url \"") || line.starts_with("\"url\":") + }) + .filter_map(|line| { + quoted(line) + .last() + .map(|url| url.replace("$version", version)) + }) + .collect() +} + +/// The version a packaging file declares. +/// +/// **The formula declares none, and that is deliberate.** Homebrew scans the +/// version out of the url and `brew audit --strict` refuses an explicit +/// `version` stanza beside it as redundant — which it is, and worse: a second +/// declaration of a fact the urls already carry is one that can disagree with +/// them. So the formula's version is read back out of its own urls, and the +/// disagreement this function used to be able to report is one the file can no +/// longer express. The manifest still declares `"version"`, because scoop has +/// no equivalent scan. +fn declared(text: &str, relative: &str) -> String { + for line in text.lines() { + let trimmed = line.trim(); + if trimmed.starts_with("\"version\":") { + if let Some(value) = quoted(trimmed).last() { + return (*value).to_string(); + } + } + } + + // Every url in the formula is `…/download/v/io--…`, + // and they must all name one version — asserted here rather than assumed, + // because a generator that rewrote three urls and missed the fourth is + // exactly the failure this file exists to catch. + let mut seen: Vec = Vec::new(); + for line in text.lines() { + let trimmed = line.trim(); + if !trimmed.starts_with("url \"") { + continue; + } + let url = quoted(trimmed).last().copied().unwrap_or_default(); + let Some(tail) = url.split("/download/v").nth(1) else { + panic!("{relative}: {url} names no release directory"); + }; + let version = tail.split('/').next().unwrap_or_default().to_string(); + if !seen.contains(&version) { + seen.push(version); + } + } + assert_eq!( + seen.len(), + 1, + "{relative} names more than one version in its urls: {seen:?}" + ); + seen.pop().expect("a formula declares at least one url") +} + +/// Every checksum a packaging file names: `sha256 "…"` in the formula and +/// `"hash": "…"` in the manifest. The manifest's autoupdate hash is a URL inside +/// an object rather than a value, so it is not one of these and is not expected +/// to be. +fn digests(text: &str) -> Vec { + text.lines() + .filter(|line| { + let line = line.trim(); + line.starts_with("sha256 \"") || line.starts_with("\"hash\": \"") + }) + .filter_map(|line| quoted(line).last().map(|digest| (*digest).to_string())) + .collect() +} + +/// F1 — the two packaging files name artifacts the release workflow really builds. +/// +/// The failure this exists to catch is a target renamed in the workflow, or an +/// archive extension swapped, leaving a formula that points at a name the Release +/// never had. Nothing else compares the two: `brew install` discovers it as a 404 +/// on somebody else's machine. +/// +/// The target sets are compared with `==` rather than `contains`. A missing arm +/// is an architecture that cannot install at all, and a stray one is a URL that +/// 404s — a subset check would report neither. +#[test] +fn f1_the_formula_and_the_manifest_name_the_artifacts_the_release_workflow_builds() { + let rows = matrix(); + let formula = read(FORMULA); + let manifest = read(MANIFEST); + let version = declared(&formula, FORMULA); + + let extensions: BTreeMap<&str, &str> = rows + .iter() + .map(|(target, archive)| (target.as_str(), archive.as_str())) + .collect(); + + let base = format!("{DOWNLOADS}/v{version}/"); + let mut seen: BTreeMap<&str, BTreeSet> = BTreeMap::new(); + for (relative, text) in [(FORMULA, &formula), (MANIFEST, &manifest)] { + let mut targets = BTreeSet::new(); + for url in urls(text, &version) { + // Every URL either names an artifact or is the SHA256SUMS beside it, + // and both live under the Release for the version the file declares. + // A URL under some other base is a download this repository did not + // publish, whatever it is called. + let name = url + .strip_prefix(&base) + .unwrap_or_else(|| panic!("{relative}: {url} is not under {base}")); + if name == "SHA256SUMS" { + continue; + } + let target = extensions + .iter() + .find(|(target, archive)| name == format!("io-{version}-{target}.{archive}")) + .map(|(target, _)| (*target).to_string()) + .unwrap_or_else(|| { + panic!( + "{relative}: {name} is not io-{version}-. for any target \ + the release matrix declares ({rows:?})", + ) + }); + // A set, not a count: the manifest names its one artifact twice on + // purpose, once concretely and once as the autoupdate template that + // replaces it. A target named twice and another not at all is caught + // by the comparison below, where it belongs. + targets.insert(target); + } + seen.insert(relative, targets); + } + + let unix: BTreeSet = rows + .iter() + .filter(|(target, _)| !target.contains("windows")) + .map(|(target, _)| target.clone()) + .collect(); + let windows: BTreeSet = rows + .iter() + .filter(|(target, _)| target.contains("windows")) + .map(|(target, _)| target.clone()) + .collect(); + + assert_eq!( + seen[FORMULA], unix, + "the formula and the workflow disagree about which unix targets exist", + ); + assert_eq!( + seen[MANIFEST], windows, + "the scoop manifest and the workflow disagree about the Windows target", + ); +} + +/// F2 — both files name the same version, and it is one this repository released. +/// +/// Deliberately NOT `CARGO_PKG_VERSION`. These files carry the digests of +/// uploaded artifacts, so during the release that builds a version they still +/// name the one before it; a gate written against the crate version would demand +/// a checksum for a file that does not exist yet. +/// +/// `## [Unreleased]` is in the same list of headings and is excluded by the shape +/// of the number, not by its name — a heading that is not `x.y.z` is not a +/// release, whatever it is called. +#[test] +fn f2_the_formula_and_the_manifest_agree_on_one_released_version() { + let formula = read(FORMULA); + let manifest = read(MANIFEST); + + let formula_version = declared(&formula, FORMULA); + let manifest_version = declared(&manifest, MANIFEST); + assert_eq!( + formula_version, manifest_version, + "{FORMULA} and {MANIFEST} name different versions; one of them installs \ + an artifact the other does not", + ); + + let released: BTreeSet = read("CHANGELOG.md") + .lines() + .filter_map(|line| line.trim().strip_prefix("## [").map(str::to_string)) + .filter_map(|rest| rest.split(']').next().map(str::to_string)) + .filter(|heading| { + let parts: Vec<&str> = heading.split('.').collect(); + parts.len() == 3 + && parts + .iter() + .all(|part| !part.is_empty() && part.chars().all(|c| c.is_ascii_digit())) + }) + .collect(); + assert!( + released.len() > 1, + "CHANGELOG.md yielded {} release headings; this test read it wrongly", + released.len(), + ); + assert!( + released.contains(&formula_version), + "the packaging files name {formula_version}, which CHANGELOG.md does not \ + record as released. The digests of an unreleased version cannot exist: \ + run scripts/update-tap.sh AFTER the Release is cut.", + ); +} + +/// F3 — every checksum is a distinct 64-character lowercase hex digest. +/// +/// Both halves matter and both are silent failures. A generator that fails to +/// substitute leaves whatever was there — a placeholder, or the previous +/// release's digest — and one that reuses a variable writes one target's digest +/// under another's URL. Neither looks wrong in a diff; both install nothing. +#[test] +fn f3_every_checksum_is_a_distinct_lowercase_hex_digest() { + let formula = digests(&read(FORMULA)); + let manifest = digests(&read(MANIFEST)); + assert_eq!( + formula.len(), + 3, + "the formula names {} checksums, not one per unix target", + formula.len(), + ); + assert_eq!( + manifest.len(), + 1, + "the manifest names {} checksums, not one for the Windows artifact", + manifest.len(), + ); + + let all: Vec = formula.into_iter().chain(manifest).collect(); + for digest in &all { + assert_eq!(digest.len(), 64, "{digest} is not 64 characters"); + // Spelled as a set of characters rather than as ranges. `sha256sum` writes + // lowercase and every comparison against it is case-sensitive, so an + // uppercase digest is a checksum nothing will ever match — and the range + // spellings for hex quietly admit one. + assert!( + digest.chars().all(|c| "0123456789abcdef".contains(c)), + "{digest} is not lowercase hex", + ); + } + let distinct: BTreeSet<&String> = all.iter().collect(); + assert_eq!( + distinct.len(), + all.len(), + "two artifacts share a checksum, so at least one of them is wrong: {all:?}", + ); +} + +/// A digest that is valid in shape and different for every row. +/// +/// Letters rather than digits, because F5 uppercases one of these to build the +/// malformed fixture and `'1'.to_uppercase()` is `'1'` — a fixture of digits +/// makes that arm assert nothing, which is how it first passed here. +fn fixture_digest(row: usize) -> String { + "abcdef" + .chars() + .nth(row) + .expect("a hex letter") + .to_string() + .repeat(64) +} + +/// A `SHA256SUMS` covering every artifact of the release matrix. +fn fixture_sums(version: &str) -> String { + matrix() + .iter() + .enumerate() + .map(|(row, (target, archive))| { + format!("{} io-{version}-{target}.{archive}\n", fixture_digest(row)) + }) + .collect() +} + +/// Copy the two packaging files and the generator into a throwaway tree. +/// +/// The generator resolves its files from its own directory, so a copy of it edits +/// the copies beside it. `README.md` is there as a file it must not touch: "it +/// wrote the two files" and "it wrote ONLY the two files" are different claims, +/// and a tree with one thing in it can only support the first. +fn stage(root: &Path) { + for directory in ["Formula", "bucket", "scripts"] { + std::fs::create_dir_all(root.join(directory)).expect("a staged directory"); + } + for relative in [FORMULA, MANIFEST, GENERATOR] { + std::fs::copy(repo().join(relative), root.join(relative)) + .unwrap_or_else(|error| panic!("staging {relative}: {error}")); + } + std::fs::write( + root.join("README.md"), + "a file the generator must not touch\n", + ) + .expect("the bystander file"); +} + +fn run(root: &Path, release: &Path, version: &str) -> std::process::Output { + Command::new("sh") + .arg(root.join(GENERATOR)) + .arg(version) + .env("IO_BASE_URL", format!("file://{}", release.display())) + .output() + .expect("the generator runs") +} + +/// Every file under `root`, by path and by bytes. +fn snapshot(root: &Path) -> BTreeMap> { + let mut files = BTreeMap::new(); + let mut pending = vec![root.to_path_buf()]; + while let Some(directory) = pending.pop() { + for entry in std::fs::read_dir(&directory).expect("a readable directory") { + let path = entry.expect("an entry").path(); + if path.is_dir() { + pending.push(path); + } else { + let relative = path + .strip_prefix(root) + .expect("under the root") + .to_string_lossy() + .into_owned(); + files.insert(relative, std::fs::read(&path).expect("a readable file")); + } + } + } + files +} + +/// Build a staged tree and a release directory holding `sums`. +fn fixture(dir: &Path, sums: &str) -> (PathBuf, PathBuf) { + let root = dir.join("root"); + let release = dir.join("release"); + std::fs::create_dir_all(&root).expect("a staged root"); + std::fs::create_dir_all(&release).expect("a release directory"); + stage(&root); + std::fs::write(release.join("SHA256SUMS"), sums).expect("SHA256SUMS"); + (root, release) +} + +/// F4 — a second run over the same Release changes nothing, and no third file moves. +/// +/// Idempotence alone is not the property, because a generator that does nothing +/// is perfectly idempotent. The first run is asserted to have actually rewritten +/// both files to the fixture's version and the fixture's digests; the second is +/// asserted to be a byte-for-byte no-op on the whole tree. Together they say the +/// generator is safe to run twice and that `git diff` after it shows the release +/// and nothing else. +#[test] +fn f4_the_generator_is_idempotent_and_writes_only_those_two_files() { + let dir = tempfile::tempdir().expect("a temporary directory"); + let (root, release) = fixture(dir.path(), &fixture_sums(FIXTURE_VERSION)); + + let before = snapshot(&root); + let output = run(&root, &release, FIXTURE_VERSION); + assert!( + output.status.success(), + "the generator failed\nstdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + let after = snapshot(&root); + + assert_eq!( + before.keys().collect::>(), + after.keys().collect::>(), + "the generator created or removed a file", + ); + let changed: BTreeSet<&String> = after + .iter() + .filter(|(path, bytes)| before.get(*path) != Some(*bytes)) + .map(|(path, _)| path) + .collect(); + let expected: BTreeSet<&String> = [FORMULA, MANIFEST] + .iter() + .map(|relative| { + after + .keys() + .find(|key| key.as_str() == *relative) + .expect("a staged file") + }) + .collect(); + assert_eq!( + changed, expected, + "the generator changed a different set of files than the two it packages", + ); + + // It really rewrote them: the version and every digest are the fixture's. + let formula = String::from_utf8_lossy(&after[FORMULA]).into_owned(); + let manifest = String::from_utf8_lossy(&after[MANIFEST]).into_owned(); + assert_eq!(declared(&formula, FORMULA), FIXTURE_VERSION); + assert_eq!(declared(&manifest, MANIFEST), FIXTURE_VERSION); + let written: BTreeSet = digests(&formula) + .into_iter() + .chain(digests(&manifest)) + .collect(); + let fixtured: BTreeSet = (0..matrix().len()).map(fixture_digest).collect(); + assert_eq!( + written, fixtured, + "the generator did not write the fixture's digests into both files", + ); + + let again = run(&root, &release, FIXTURE_VERSION); + assert!(again.status.success(), "the second run failed"); + assert_eq!( + snapshot(&root), + after, + "a second run over the same Release changed something", + ); +} + +/// F5 — a checksum file the generator cannot verify stops it, before it writes. +/// +/// Three shapes, because each takes a different branch and each is a real way for +/// a Release to be wrong: an upload that did not finish leaves an artifact +/// unmentioned, a workflow that failed after creating the file leaves it empty, +/// and a digest of the wrong shape is a checksum nothing will ever match. In all +/// three the tree has to come out byte-identical: reading four checksums before +/// writing any of them is what keeps a half-updated formula off disk. +/// +/// Each arm is held to the words of ITS OWN branch rather than to "it exited +/// non-zero". The refusals are over-determined — a missing line leaves an empty +/// digest, which is also not 64 characters long — so an arm that asserted only +/// the status stayed green with the absent-value guard deleted, and the guard +/// this repository has already once printed a success line without would have +/// been unprotected by the test written to protect it. +#[test] +fn f5_the_generator_refuses_a_checksum_file_it_cannot_verify() { + let windows = matrix() + .into_iter() + .find(|(target, _)| target.contains("windows")) + .expect("a Windows row in the release matrix"); + let full = fixture_sums(FIXTURE_VERSION); + let missing: String = full + .lines() + .filter(|line| !line.contains(windows.0.as_str())) + .map(|line| format!("{line}\n")) + .collect(); + let uppercased = full.replacen(&fixture_digest(0), &fixture_digest(0).to_uppercase(), 1); + + for (what, sums, says) in [ + ( + "an artifact with no checksum line", + missing.as_str(), + format!( + "does not mention io-{FIXTURE_VERSION}-{}.{}", + windows.0, windows.1 + ), + ), + ("an empty SHA256SUMS", "", "is empty".to_string()), + ( + "a digest that is not lowercase hex", + uppercased.as_str(), + "is not lowercase hex".to_string(), + ), + ] { + let dir = tempfile::tempdir().expect("a temporary directory"); + let (root, release) = fixture(dir.path(), sums); + + let before = snapshot(&root); + let output = run(&root, &release, FIXTURE_VERSION); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !output.status.success(), + "{what} was accepted\nstdout: {}", + String::from_utf8_lossy(&output.stdout), + ); + assert!( + stderr.contains("update-tap:") && stderr.contains(&says), + "{what}: the refusal must say {says:?} on stderr, so that the branch \ + that refused is the one this arm is holding open: {stderr}", + ); + assert_eq!( + snapshot(&root), + before, + "{what}: the generator wrote something before it refused", + ); + } +} diff --git a/tests/provider.rs b/tests/provider.rs index 40d94ab..e4c14cc 100644 --- a/tests/provider.rs +++ b/tests/provider.rs @@ -390,7 +390,7 @@ async fn f5_the_head_is_asked_first_and_a_link_below_it_is_not_asked_at_all() { async fn f5_a_head_that_answered_reports_no_fallover() { // **The whole reason this crate does not build on `io_harness::Fallback`.** // io-harness emits `EventKind::FellBackTo` for any `Some` from `last_served` - // (`run/step.rs:503`), and `Fallback::last_served` answers `Some` for its own + // (`run/step.rs:577`), and `Fallback::last_served` answers `Some` for its own // primary — so a chain built from that type would tell the operator "the // provider fell over" on every step of every run. let (first, _) = link("first", None); diff --git a/tests/providers.rs b/tests/providers.rs index f22749e..1372191 100644 --- a/tests/providers.rs +++ b/tests/providers.rs @@ -249,7 +249,7 @@ fn f7_removing_the_head_promotes_the_next_link_and_removing_the_only_one_leaves_ #[test] fn f7_a_compatible_entry_naming_both_bases_or_neither_cannot_be_written() { // io-harness takes EXACTLY ONE of `preset` and `base_url` and refuses both - // and neither, by index, at load (`config.rs:456`). `configure::write`'s + // and neither, by index, at load (`config.rs:571`). `configure::write`'s // round trip would catch it and roll back, which is a good failure — but the // pair of `Option`s that made it expressible is gone: `Endpoint` has one // variant per shape, so three of the four combinations cannot be spelled. diff --git a/tests/recall.rs b/tests/recall.rs index f2182eb..4432eda 100644 --- a/tests/recall.rs +++ b/tests/recall.rs @@ -7,7 +7,7 @@ //! //! 1. **The bucket is a canonicalised path.** io-harness keys a workspace's //! memory on `std::fs::canonicalize(root)` — `src/run/memory.rs:14-19`, and -//! the function is `pub(super)`, so io-cli cannot call it and has to +//! the function is `pub(crate)`, so io-cli cannot call it and has to //! reproduce it. Key on the root as given and the panel is empty beside an //! agent writing a note every turn, **and only when the checkout is reached //! through a symlink**, which is the defect that ships green. `/var` on macOS @@ -21,7 +21,7 @@ //! its own, so a run drawing on both may carry up to twice `max_entries`. One //! number reported as *the* cap is half the real ceiling. //! 4. **Eviction, refusal and recall emit no `EventKind` at all.** io-harness -//! records them as `ContextEvent` rows on purpose (`src/state.rs:2996-3002`). +//! records them as `ContextEvent` rows on purpose (`src/state.rs:3275-3281`). //! An implementation reaching for the observer stream reports that none has //! ever happened, which is indistinguishable from a healthy store. The real //! run below watches the stream and asserts the *absence*, so that sabotage @@ -180,7 +180,7 @@ fn f6_lists_both_buckets_and_keeps_them_apart() { assert_eq!( view.entries[1].kind, "decision", "the two kinds are spelled here because `MemoryKind::as_str` is private \ - in io-harness (src/state.rs:1812)", + in io-harness (src/state.rs:2066)", ); } @@ -500,7 +500,7 @@ async fn f8_eviction_refusal_and_recall_are_read_from_the_trace() { assert!( !seen.iter().any(|k| k.contains(forbidden)), "SABOTAGE: io-harness emits no EventKind for {forbidden} \ - (src/state.rs:2996-3002), so an implementation reading evictions off \ + (src/state.rs:3275-3281), so an implementation reading evictions off \ the observer stream reports that none has ever happened and looks \ perfectly healthy. These must come from Store::context_events.", ); @@ -815,7 +815,7 @@ fn f7_pinning_and_unpinning_act_on_the_bucket_the_scope_names() { ); // "Nothing to pin" is an outcome of its own. io-harness will not invent an - // entry to carry the pin (`src/state/memory.rs:556-558`), so a `bool` read as + // entry to carry the pin (`src/state/memory.rs:569-570`), so a `bool` read as // "did it work" would leave a surface showing a pin the store does not hold. assert_eq!( recall::pin( @@ -940,7 +940,7 @@ fn f7_a_pinned_entry_survives_eviction_at_the_cap_and_an_unpinned_one_does_not() // `plain` first, so it is also the first eviction candidate: nothing has // recalled either, and the candidate order falls through to `created_at ASC, - // id ASC` (`src/state/memory.rs:653-660`). The pin is therefore the only + // id ASC` (`src/state/memory.rs:691`). The pin is therefore the only // thing that can decide which of the two goes. remember(&store, &bucket, "plain", "an older note", MemoryKind::Fact); remember( @@ -1019,7 +1019,7 @@ async fn f7_forgetting_removes_the_entry_and_leaves_a_way_back() { // Written before the turn so the turn carries it into its prompt and the key // accrues real recall rows. `Store::record_memory_recall` is `pub(crate)` - // (`src/state/memory.rs:786`), so a real turn is the only way to make one — + // (`src/state/memory.rs:607`), so a real turn is the only way to make one — // which is why this reuses the `Remembering` harness above rather than // hand-writing a row io-harness would never have written. remember( @@ -1079,9 +1079,9 @@ async fn f7_forgetting_removes_the_entry_and_leaves_a_way_back() { doomed_recalls(turn.run_id), 0, "SABOTAGE: `Store::memory_delete` is a bare `DELETE FROM memory` \ - (io-harness `src/state/memory.rs:863-869`) and leaves every recall row \ + (io-harness `src/state/memory.rs:896-903`) and leaves every recall row \ standing. The evidence a withdrawn note accrued would keep voting in \ - the eviction order (`src/state/memory.rs:653-660`) on behalf of a note \ + the eviction order (`src/state/memory.rs:684-691`) on behalf of a note \ that no longer exists, and a draw count read back for a later note of \ the same key would carry the dead one's history.", ); @@ -1092,8 +1092,8 @@ async fn f7_forgetting_removes_the_entry_and_leaves_a_way_back() { recall::unforget(&store, workspace.path(), restore).expect("the rewind runs"), ["doomed"], "SABOTAGE: `memory_delete` writes no `memory_snapshots` row (io-harness \ - `src/state/memory.rs:838-848`), so `rewind_run` finds nothing to put \ - back (`src/run.rs:749`) and this is empty. The operator who withdrew \ + `src/state/memory.rs:896-903`), so `rewind_run` finds nothing to put \ + back (`src/run.rs:823`) and this is empty. The operator who withdrew \ the wrong note would have no way back at all — and would find that out \ at the one moment they wanted it.", ); @@ -1102,7 +1102,7 @@ async fn f7_forgetting_removes_the_entry_and_leaves_a_way_back() { assert_eq!( back.value, "the flaky test is in parser rs", "byte for byte, from the restore point taken BEFORE the removal \ - (io-harness `src/state/memory.rs:838-848`)", + (io-harness `src/state/memory.rs:866-873`)", ); assert!( !back.pinned, @@ -1122,7 +1122,7 @@ async fn f7_forgetting_removes_the_entry_and_leaves_a_way_back() { assert_eq!( record.undid_step, None, "a whole-run rewind rather than a step revert (io-harness \ - `src/state.rs:3436-3440`)", + `src/state.rs:3726-3731`)", ); } @@ -1150,7 +1150,7 @@ fn f7_a_pinned_entry_is_refused_and_an_unknown_key_is_absent() { .expect("the call succeeds; the withdrawal does not"), Forgotten::Refused, "SABOTAGE: `MemoryForget::Pinned` means the harness REFUSED \ - (io-harness `src/state/memory.rs:829-831`), and it is the one outcome \ + (io-harness `src/state/memory.rs:863-864`), and it is the one outcome \ that looks like the other two if it is folded into a `bool`. Reported \ as success it tells an operator their note is gone while it stays in \ the store and is carried into every later prompt — the same failure the \ diff --git a/tests/reload.rs b/tests/reload.rs index 322debd..f2116de 100644 --- a/tests/reload.rs +++ b/tests/reload.rs @@ -79,7 +79,7 @@ fn workspace(io_toml: &str, agents: &str) -> (tempfile::TempDir, Configuration) /// Whether any composed instruction carries `line`. /// /// io-harness words each instruction file as `Project instructions from -/// `AGENTS.md`:\n` (`io-harness-0.76.0/src/config.rs:2822-2825`), so the +/// `AGENTS.md`:\n` (`io-harness-0.78.0/src/config.rs:2934-2937`), so the /// assertion is a containment and not an equality — this file is not the place /// that pins the harness's wording. fn instructs(held: &Configuration, line: &str) -> bool { diff --git a/tests/routing.rs b/tests/routing.rs index 1056f1b..7023c7f 100644 --- a/tests/routing.rs +++ b/tests/routing.rs @@ -292,7 +292,7 @@ fn a_rule_missing_half_of_itself_parses_and_is_refused_by_name() { /// **The three values that are writable and disastrous.** /// -/// io-harness obeys the thresholds literally (`contract.rs:1811-1822`), and none +/// io-harness obeys the thresholds literally (`contract.rs:2055-2067`), and none /// of these is a shape TOML can refuse: /// /// * `failures = 0` satisfies `consecutive_gate_failures >= 0` at the first request @@ -417,7 +417,7 @@ const UNRELATED: &str = "[run]\nmax_steps = 7\n"; /// **`Config::from_toml` cannot build any of these fixtures**, which is worth /// stating where the first one is written. That constructor hard-codes -/// `Scope::Project` (`config.rs:1128`), `[routing]` is in io-harness's +/// `Scope::Project` (`config.rs:1166`), `[routing]` is in io-harness's /// `REFUSED_SECTIONS` for every scope but the user's, and so a parsed-text fixture /// carrying `[routing]` does not load at all. `support::user_scope` writes the file /// outside the discovery root and points `IO_CONFIG` at it, which is the only place @@ -480,9 +480,9 @@ fn f11_a_harness_table_alone_says_no_io_cli_surface_lists_it() { /// Both tables written, and the notice says which one the contract carries. /// /// The whole of F11. `Config::apply_to` merges `[routing]` onto the contract -/// (`config.rs:2076`) and `contract::configured` then calls +/// (`config.rs:2168`) and `contract::configured` then calls /// `TaskContract::with_routing`, whose body is `self.routing = Some(routing)` -/// (`contract.rs:1292`) — so `mechanical`, which io-cli has no key for, is gone and +/// (`contract.rs:1314`) — so `mechanical`, which io-cli has no key for, is gone and /// nothing said so before this release. /// /// **The second half is the branch a plausible wrong implementation gets wrong.** A diff --git a/tests/servers.rs b/tests/servers.rs index 79d9ff1..dd12742 100644 --- a/tests/servers.rs +++ b/tests/servers.rs @@ -1024,7 +1024,7 @@ fn f6_the_file_a_row_names_is_the_file_its_position_is_read_from() { #[test] fn f6_a_shadowed_server_is_not_offered_for_editing_because_it_is_not_running() { - // `[[mcp]]` is NOT one of io-harness's appending keys (`config.rs:2052`): + // `[[mcp]]` is NOT one of io-harness's appending keys (`config.rs:3130`): // the winning scope replaces the array WHOLE, "because a half-merged MCP // server definition is not a server". So an entry named only in the project // file, while a local file declares any `[[mcp]]` at all, is not diff --git a/tests/status.rs b/tests/status.rs index 8093d8b..074649b 100644 --- a/tests/status.rs +++ b/tests/status.rs @@ -133,6 +133,7 @@ fn f4_the_branch_is_drawn_by_both_renderers_and_survives_the_conversation() { status.note_branch(&event(EventKind::ToolCall { name: "git_branch".into(), target: "agent/fix-the-flake".into(), + origin: None, })); assert_eq!(status.branch.as_deref(), Some("agent/fix-the-flake")); @@ -193,11 +194,13 @@ fn f4_a_workspace_with_no_branch_draws_no_branch_field_anywhere() { status.note_branch(&event(EventKind::ToolCall { name: "git_branch".into(), target: String::new(), + origin: None, })); // And so does every other tool call, whatever it points at. status.note_branch(&event(EventKind::ToolCall { name: "apply_patch".into(), target: "src/lib.rs".into(), + origin: None, })); assert_eq!( status.branch, None, diff --git a/tests/triage.rs b/tests/triage.rs index deb1d2b..d725eda 100644 --- a/tests/triage.rs +++ b/tests/triage.rs @@ -317,6 +317,7 @@ fn a_status_or_silent_kind_commits_nothing() { EventKind::ToolCall { name: "read_file".into(), target: "notes.txt".into(), + origin: None, }, EventKind::SpendDraw { tokens: 12, diff --git a/tests/undo.rs b/tests/undo.rs index bbe1fdc..3b07f33 100644 --- a/tests/undo.rs +++ b/tests/undo.rs @@ -2,8 +2,8 @@ //! //! **The assertion that matters most here is that an event *arrives*.** //! `EventKind::Rewound` is emitted only inside `rewind_run_observed` -//! (`run.rs:804`) and `EventKind::Reverted` only inside `rewind_step_observed` -//! (`run.rs:1011`). `src/rewind.rs` called the plain `rewind_run` from 0.4.0 +//! (`run.rs:878`) and `EventKind::Reverted` only inside `rewind_step_observed` +//! (`run.rs:1085`). `src/rewind.rs` called the plain `rewind_run` from 0.4.0 //! until this release, so neither has ever fired in this product's history — and //! `src/events.rs` has had an arm for one of them the whole time, unreachable. //! diff --git a/tests/upgrade.rs b/tests/upgrade.rs new file mode 100644 index 0000000..b69ac37 --- /dev/null +++ b/tests/upgrade.rs @@ -0,0 +1,207 @@ +//! 0.38.0 F6, F7 — `io upgrade` names the command for the way the binary was +//! installed, and the argv door really opens. +//! +//! The classification is a free function over a path, so every case is reachable +//! here without installing anything. That is the point of its shape: a branch +//! only a Homebrew install could reach is a branch no test on this machine could +//! ever drive, and `src/main.rs` links from no test binary at all. +//! +//! The last test spawns the real binary. Nothing under `tests/` links +//! `src/main.rs`, so a subcommand can be declared, implemented, documented and +//! still answer `unrecognized subcommand` — which is exactly what 0.30.0 shipped +//! for `io skill` behind 1,609 green tests. + +use io_cli::upgrade::{self, Installed}; +use std::path::PathBuf; + +/// F6 — the path decides, and the table is written for what it must NOT claim. +/// +/// The three negative rows carry the weight. `/usr/local/bin/io` is where a +/// hand-copied binary lands, `target/release/io` is a build, and +/// `scoop-notes/bin/io` exists to fail a classifier written with `contains` +/// instead of a component comparison — the same widening a substring match makes +/// of `tests/dependencies.rs`'s permitted-path set. +#[test] +fn f6_a_path_decides_which_installer_placed_the_binary() { + let table = [ + ("/opt/homebrew/Cellar/io/0.38.0/bin/io", Installed::Homebrew), + ("/opt/homebrew/bin/io", Installed::Homebrew), + ("/usr/local/Cellar/io/0.38.0/bin/io", Installed::Homebrew), + ("/home/me/.linuxbrew/bin/io", Installed::Homebrew), + ("/c/Users/me/scoop/apps/io/current/io.exe", Installed::Scoop), + ("/srv/scoop/apps/io/0.38.0/io.exe", Installed::Scoop), + ("/home/me/.local/bin/io", Installed::Installer), + ( + "/c/Users/me/AppData/Local/io/bin/io.exe", + Installed::Installer, + ), + ("/usr/local/bin/io", Installed::Unknown), + ("/home/me/src/io-cli/target/release/io", Installed::Unknown), + ("/home/me/scoop-notes/bin/io", Installed::Unknown), + ]; + for (path, want) in table { + assert_eq!( + upgrade::installed(&PathBuf::from(path)), + want, + "{path} should be classified {want:?}" + ); + } +} + +/// F6 — the command is the first line and stands alone on it. +/// +/// An operator copies the first line. A line carrying prose beside the command +/// is a line that does not run when pasted, so this asserts the shape rather +/// than the wording. +#[test] +fn f6_the_command_is_the_first_line_and_nothing_else_is_on_it() { + for path in [ + "/opt/homebrew/Cellar/io/0.38.0/bin/io", + "/srv/scoop/apps/io/0.38.0/io.exe", + "/home/me/.local/bin/io", + "/usr/local/bin/io", + ] { + let lines = upgrade::advice(&PathBuf::from(path), false); + let first = &lines[0]; + assert!( + [upgrade::HOMEBREW, upgrade::SCOOP, upgrade::INSTALLER_UNIX].contains(&first.as_str()), + "{path} led with {first:?}, which is not one of the three commands" + ); + assert_eq!(lines[1], "", "the command should stand alone on its line"); + } +} + +/// F6 — an unrecognised path is named as a guess, and names itself. +/// +/// A guess presented as an instruction is how an operator ends up with two +/// installs of one binary, one of which never updates again. +#[test] +fn f6_an_unrecognised_path_is_named_as_a_guess() { + let lines = upgrade::advice(&PathBuf::from("/usr/local/bin/io"), false); + let said = lines.join("\n"); + assert!(said.contains("this is a guess"), "{said}"); + assert!( + said.contains("/usr/local/bin/io"), + "the operator is not told which path was not recognised: {said}" + ); +} + +/// F6 — the installer line follows the platform. +/// +/// The wrong one is not merely unhelpful: it names an interpreter the operator +/// does not have. +#[test] +fn f6_the_installer_line_follows_the_platform() { + let path = PathBuf::from("/home/me/.local/bin/io"); + assert_eq!(upgrade::advice(&path, false)[0], upgrade::INSTALLER_UNIX); + assert_eq!(upgrade::advice(&path, true)[0], upgrade::INSTALLER_WINDOWS); +} + +/// F6 — no arm claims a package manager placed a binary it did not. +/// +/// The complement of the table above, asserted over the rendered text rather +/// than over the enum: a classifier that answered `Unknown` correctly and then +/// printed "installed by Homebrew" would pass every test above this one. +/// +/// **The needle is the claim and not the word**, and the first draft of this +/// test had it the other way round and failed on correct output. The +/// unrecognised arm names Homebrew and scoop deliberately — it says the path is +/// *not* where either of them puts a binary, which is the sentence that stops an +/// operator running the wrong updater. Banning the vendor's name would forbid +/// the module from explaining itself, which is the same shape as a prose gate +/// that forbids a file from naming what it does not do. +#[test] +fn f6_no_advice_claims_a_manager_that_did_not_place_it() { + for path in [ + "/usr/local/bin/io", + "/home/me/src/io-cli/target/release/io", + "/home/me/.local/bin/io", + ] { + let said = upgrade::advice(&PathBuf::from(path), false).join("\n"); + for claim in ["installed by Homebrew", "installed by scoop"] { + assert!( + !said.contains(claim), + "{path} was told it was {claim}: {said}" + ); + } + } + // And the control, so the two negations above cannot pass vacuously: the + // arms that ARE a package manager say so in exactly those words. + let brew = upgrade::advice(&PathBuf::from("/opt/homebrew/bin/io"), false).join("\n"); + assert!(brew.contains("installed by Homebrew"), "{brew}"); + let scoop = upgrade::advice(&PathBuf::from("/srv/scoop/apps/io/1/io.exe"), false).join("\n"); + assert!(scoop.contains("installed by scoop"), "{scoop}"); +} + +/// F7 — the argv door opens, on the real binary. +/// +/// **This one spawns the binary and it has to.** `Subcommand::Upgrade` is +/// matched in `src/main.rs`, which no integration test links, so every assertion +/// above this line passes whether or not clap routes the word. `io skill` was +/// documented, implemented and shipped in 0.30.0 with no clap variant, and the +/// whole suite was green over a door that answered `unrecognized subcommand`. +/// +/// The command needs no configuration, no store, no provider and no terminal, so +/// it is safe to run here with nothing set up — which is itself the property +/// being asserted by the exit status. +#[test] +fn f7_the_argv_door_prints_a_command_and_exits_zero() { + let out = std::process::Command::new(env!("CARGO_BIN_EXE_io")) + .arg("upgrade") + .output() + .expect("the built binary runs"); + + assert!( + out.status.success(), + "`io upgrade` exited {:?}\nstdout: {}\nstderr: {}", + out.status.code(), + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + + let stdout = String::from_utf8_lossy(&out.stdout); + let first = stdout.lines().next().unwrap_or_default(); + assert!( + [ + upgrade::HOMEBREW, + upgrade::SCOOP, + upgrade::INSTALLER_UNIX, + upgrade::INSTALLER_WINDOWS + ] + .contains(&first), + "`io upgrade` led with {first:?}, which is not one of the commands" + ); +} + +/// N4 — the command reaches no network, and the module says so rather than +/// being trusted to. +/// +/// A source-text gate over `src/upgrade.rs`, normalised at the read because git +/// hands Windows a CRLF working copy and a gate that does not normalise fails +/// there for a reason unrelated to its subject — twice in this repository +/// already. `tests/dependencies.rs` bans the network types across all of `src/`; +/// this adds the spellings a version check would arrive as. +#[test] +fn n4_the_upgrade_module_asks_nobody_what_the_latest_version_is() { + let source = std::fs::read_to_string("src/upgrade.rs") + .expect("this crate's source is readable") + .replace("\r\n", "\n"); + let code: String = source + .lines() + .filter(|line| !line.trim_start().starts_with("//")) + .collect::>() + .join("\n"); + + for needle in [ + "releases/latest", + "api.github.com", + "Command::new", + "reqwest", + "TcpStream", + ] { + assert!( + !code.contains(needle), + "`src/upgrade.rs` names {needle:?}; this command reads a path and prints" + ); + } +}