diff --git a/CHANGELOG.md b/CHANGELOG.md index 7efd9b7..aba8f14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,82 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +## [0.36.0] - 2026-09-03 + +The release that makes io reachable from an editor. The Agent Client Protocol is the field's +integration surface — Zed's, JetBrains's, and a registry of agents that answer it — and having +none of it kept this product out of every editor that has adopted one. `io acp` is a protocol +adapter over a door that already existed: ACP is newline-delimited JSON-RPC 2.0 over stdio, which +is byte for byte the output discipline `io exec --json` has kept since 0.5.0. + +The wire is served with the dependencies already in the tree. The official +`agent-client-protocol` crate was read and declined: it brings twenty-six further lockfile crates +including `async-io`, `blocking` and `polling`, a second async executor family reading real stdin +beside this one — and a second owner of stdin is not a hypothetical hazard here. The direct +dependency set is still exactly ten names. + +### Added + +**`io acp` serves the Agent Client Protocol on stdin and stdout.** An editor spawns it and speaks +JSON-RPC at it: `initialize`, `session/new`, `session/prompt`, `session/cancel`. The agent's +answer streams back as `session/update` notifications — message chunks, reasoning on its own +channel so a client can fold it, tool calls with ACP's own kinds, and plan changes. A turn answers +with ACP's `stopReason`. See the new [From an editor](docs/guide/editors.md) guide. + +**An approval in an editor session is refused, and the client is told which action and why.** A +`session/update` marks the tool call failed and says the approval could not be routed; the model +is told the same, rather than that the operator denied it, because the operator was never asked. +io does **not** send `session/request_permission` in 0.36.0 — raising that request means awaiting +the answer, and routing it back into the running turn is not wired, so a prompt whose outcome is +ignored would be worse than no prompt. `docs/CONTRACT.md`'s standing sentence — io "declines every +approval rather than deferring one" — therefore still holds on every door this release ships. + +### Changed + +**The io-harness pin moves to 0.76.0**, two minor versions. + +**A cache-write count the provider never reported now reads `unknown`, where it previously read a +number that was wrong.** io-harness 0.76.0 makes `Usage::cache_write_tokens` an `Option`, which is +the shape `/cost`'s own first honesty rule has wanted since 0.13.0: a count a provider reported no +usage for is unknown, never zero. **Every OpenRouter call reports none**, so this is most turns +rather than an edge case. The "of which fresh" figure inherits the silence, because it is computed +from the count that does not exist. + +**`io exec --json` writes a second object per committed step.** io-harness 0.75.0 added +`EventKind::StepAttributed` — a per-step timing breakdown — and the `--json` stream forwards every +event without a filter. A consumer that counted objects, or assumed one object per step, sees +different output. The variant draws no line in the transcript: it arrives beside every step and a +line would restate the span the step line already draws. + +**A harness-native `[routing]` section is now reported rather than silently discarded.** 0.76.0 +lets a user-scope `io.toml` carry `[routing]`, and io-cli then applies `[app.io-cli.routing]` +through a builder that **replaces** rather than merges — so an operator with both lost the +harness's without being told, and an operator with only the harness's saw it in no io-cli surface. +io now says which is in force. The precedence is unchanged: deciding it here would be this crate +taking a second opinion about somebody else's section. + +**Four more tool calls can now run concurrently and arrive out of order** in the transcript: +0.75.0 made `list_dir`, `git_log`, `git_status` and `git_diff` speculable. Nothing in io-cli +changed; the ordering did. + +### Known limitations + +- **An editor session refuses every grey-tier action and does not ask.** `session/request_permission` + is not sent; the tool call is marked failed with the reason. Nothing is granted that was not + granted, but the posture has to be configured rather than approved in the moment. +- **`session/cancel` takes effect between turns, not during one.** A turn in flight runs to its own + end, so `stopReason: "cancelled"` is reachable only where the cancel arrived before the turn + began. +- **The client's filesystem and terminal are not used.** io-harness owns the disk and process + execution inside its own sandbox and publishes no seam to route either through the client, so io + cannot see unsaved editor buffers. This is an upstream absence rather than a deferral. +- One ACP session per process; a second `session/new` is refused with a sentence. +- `session/load` and `session/set_mode` are not served, and `loadSession` is declared unsupported. +- A `plan` update carries no entries — a client is told the plan moved, which is true, rather than + a fabricated list. +- A turn that errors answers `end_turn` with the reason on stderr, because ACP has no stop reason + for a transport failure. + ## [0.35.0] - 2026-09-02 The release that renders a boundary somebody measured. This interface has drawn the @@ -3246,7 +3322,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.35.0...HEAD +[Unreleased]: https://github.com/initorigin/io-cli/compare/v0.36.0...HEAD +[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 [0.34.1]: https://github.com/initorigin/io-cli/compare/v0.34.0...v0.34.1 [0.34.0]: https://github.com/initorigin/io-cli/compare/v0.33.0...v0.34.0 diff --git a/Cargo.lock b/Cargo.lock index 7175a50..4c5c1fe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1850,7 +1850,7 @@ dependencies = [ [[package]] name = "io-cli" -version = "0.35.0" +version = "0.36.0" dependencies = [ "clap", "crossterm", @@ -1867,9 +1867,9 @@ dependencies = [ [[package]] name = "io-harness" -version = "0.74.0" +version = "0.76.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e279891225dc1fe491c1deffe1d50f1bfc145df19b1f79fad86354065c409ca" +checksum = "d6fd17b013a3db32112f92782b9ea4a0218755774beb9ee9cde0bd4291d2b28c" dependencies = [ "base64 0.23.1", "calamine", @@ -3469,9 +3469,9 @@ dependencies = [ [[package]] name = "rust_xlsxwriter" -version = "0.98.2" +version = "0.99.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "093d730a6f64f9620e047b2de2876e61a691fbcaca3aaf436edade9b897c7a27" +checksum = "c073c0bd3868defc46035bf95b2e70389cefc59c528b8674fe90594c9c2b3f6f" dependencies = [ "zip", ] @@ -4057,7 +4057,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", - "getrandom 0.4.3", + "getrandom 0.3.4", "once_cell", "rustix", "windows-sys 0.61.2", diff --git a/Cargo.toml b/Cargo.toml index e2ec170..c2a2635 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "io-cli" -version = "0.35.0" +version = "0.36.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.74", features = ["media", "browser", "documents"] } +io-harness = { version = "0.76", 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 @@ -98,7 +98,28 @@ ratatui = { version = "0.30", default-features = false, features = [ ] } crossterm = "0.29" clap = { version = "4", features = ["derive"] } -tokio = { version = "1", features = ["rt-multi-thread", "macros", "sync", "time"] } +# `io-util` and `io-std` are 0.36.0's, for the ACP adapter: it reads +# newline-delimited JSON-RPC frames off stdin and writes them to stdout, and +# `AsyncBufReadExt::read_line` over `tokio::io::stdin()` is what does it. They are +# features of a name already here rather than a name of their own, which is the +# whole reason `src/acp.rs` speaks the wire itself instead of taking the official +# `agent-client-protocol` crate — that one is correct and well made, and its +# dependency graph brings a second async executor family beside this one: +# `async-io`, `async-process`, `async-task`, `blocking` and `polling`, with its +# stdio transport putting `blocking::Unblock` over the real stdin and stdout. +# (The crate count that walk produced is not repeated here: it came from +# crates.io metadata rather than a resolve, and a number this file cannot check +# is the kind of claim that goes stale unread. The five names are the argument.) +# Two owners of stdin is not a hypothetical failure here; see `src/stdin.rs`, +# which exists because of what it cost in 0.13.1. +tokio = { version = "1", features = [ + "rt-multi-thread", + "macros", + "sync", + "time", + "io-util", + "io-std", +] } serde = { version = "1", features = ["derive"] } # The tenth name, and the first added since 0.3.0. `io exec --json` writes each # `io_harness::RunEvent` as one line of JSON, and the event type derives diff --git a/README.md b/README.md index 2c42fa6..fcf4891 100644 --- a/README.md +++ b/README.md @@ -155,6 +155,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 | | [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/docs/CAPABILITIES.md b/docs/CAPABILITIES.md index c51f255..a0015d6 100644 --- a/docs/CAPABILITIES.md +++ b/docs/CAPABILITIES.md @@ -35,6 +35,7 @@ the limits that capability actually has. | [Pictures and documents](guide/media.md) | Attaching an image, drawing the real thing only where the terminal can, and the twelve document tools of which six write | | [Reading it without seeing it](guide/accessibility.md) | `--plain`: nothing turns, nothing moves, and every state change is committed as text | | [Headless](guide/headless.md) | `io exec` and `io resume`, the seven exit codes, `--json`, and managing MCP servers, bundles, skills and configuration from a shell | +| [From an editor](guide/editors.md) | `io acp` serving the Agent Client Protocol on stdio, the session updates an editor is sent, the three permission options, and the client-side filesystem io deliberately does not use | | [Configuration](guide/configuration.md) | One `io.toml` over io-harness's scopes, every section that bounds a turn, changing it without leaving the session, and where io keeps your things | | [What the store is holding](guide/store.md) | Reading the run store, undo at three grains, and exporting a conversation or a canonical trace | | [What this release is not](guide/limits.md) | The limits, stated plainly — including the ones that are decisions rather than gaps | diff --git a/docs/CONTRACT.md b/docs/CONTRACT.md index d5673b2..1215ae4 100644 --- a/docs/CONTRACT.md +++ b/docs/CONTRACT.md @@ -31,6 +31,7 @@ With no subcommand, `io` opens an interactive session. | `io plugin …` | Manage capability bundles and marketplaces | | `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 | `--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. @@ -39,6 +40,13 @@ nothing and animates nothing already, so there is no second thing for the flag t `--policy ` and `--provider `. **`--policy ask-writes` is refused**, because nothing headless can answer an approval. +`io acp` takes no arguments of its own — the client speaks the protocol, not the command line — +and the global flags reach it, which is how an editor points io at the workspace it has open. +It is not run by hand: an ACP client spawns it and speaks newline-delimited JSON-RPC 2.0 at its +stdin. **stdout is the protocol**, so nothing else is written there and diagnostics go to stderr. +It always exits `0` when the client closes the pipe, because the protocol reports a run's outcome +in its own `stopReason` and a process exit code would be a second, disagreeing answer. + ## Exit codes These are the contract a CI job depends on. **A code never changes meaning under a dependency @@ -64,6 +72,21 @@ 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. + +`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. + **Every boundary refusal reaches `2`, from either headless door.** A run whose provider endpoint lands in the `ask` tier is refused by io-harness before the run's first step, and `io resume` re-authorizes that same endpoint before it carries anything on, so the refusal reaches both doors. diff --git a/docs/guide/editors.md b/docs/guide/editors.md new file mode 100644 index 0000000..e057f5d --- /dev/null +++ b/docs/guide/editors.md @@ -0,0 +1,107 @@ +# From an editor + +`io acp` serves the [Agent Client Protocol](https://agentclientprotocol.com) on stdin and stdout, +so an editor that speaks ACP runs io against your own configuration, your own provider chain and +your own bundles — without a terminal. + +It is not a command you run. An ACP client spawns `io acp` as a child process and speaks +newline-delimited JSON-RPC 2.0 at it. + +## Pointing an editor at it + +The client needs the path to the `io` binary and the argument `acp`. In Zed, that is an entry in +`settings.json`: + +```json +{ + "agent_servers": { + "io": { + "command": "io", + "args": ["acp"] + } + } +} +``` + +Global flags reach the subcommand, so `["acp", "-C", "/path/to/repo"]` points a session at a +workspace other than the one the editor launched io from, and `["acp", "-m", ""]` picks the +model for that session. + +Configuration is read exactly as every other door reads it: `$IO_CONFIG`, else +`$IO_CONFIG_HOME/io.toml`, else `~/.config/io/io.toml`. There is no editor-specific settings file +and no second way to be configured — the adapter adds a protocol, not a second product. + +## What the editor sees + +| ACP update | What io sends | +| --- | --- | +| `agent_message_chunk` | The answer, streamed as the provider returns it | +| `agent_thought_chunk` | Reasoning, where the provider returns it separately, so your editor can fold it | +| `tool_call` | A tool starting, with its ACP kind — a read, an edit, a search, an execution | +| `tool_call_update` | That call finishing, failing, or being refused by the policy | +| `plan` | The agent's plan changed | + +A turn answers with ACP's own `stopReason`. A run that finished is `end_turn`, one you cancelled +is `cancelled`, one the policy refused is `refusal`, and a budget or step ceiling is +`max_turn_requests` or `max_tokens`. + +**A run that stopped to ask you something also answers `end_turn`, not `refusal`.** It has +refused nothing — it stopped with work outstanding. `io resume` carries it on from the terminal; +see [When a run stops for you](resume.md). + +`cancelled` is reachable only where a cancel arrived before the turn began — see +[Cancelling](#cancelling). + +## 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 +[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 +one: a turn in flight runs to its own end. Stopping a turn mid-flight is what the terminal's +`Ctrl+C` does; see [While it works](the-session.md). + +## What is not carried + +**Your editor's unsaved buffers.** ACP lets an agent read and write files *through* the client, +which is how it sees edits you have not saved. io does not: io-harness owns the filesystem inside +its own sandbox and publishes no seam to route a read through somebody else. io reads what is on +disk. Save before you ask about a file. + +**A terminal the client owns.** Same reason — io runs programs inside its own sandbox, and handing +a spawn to the editor would put the run outside the boundary io exists to show you. + +**More than one session per process.** A second `session/new` is refused with a sentence. Run a +second `io acp` for a second conversation. + +**Loading an earlier conversation.** `session/load` is not served and `loadSession` is declared +unsupported, so a conforming client will not offer it. Runs are still in the store and `io resume` +reaches them. + +## When something looks wrong + +Diagnostics go to **stderr**, which most clients surface as a log for the agent process. stdout +carries the protocol and nothing else — a stray byte there is a frame boundary in the wrong place, +and every message after it is read at the wrong offset. + +If the session goes silent, check the agent log for the sentence io printed. A missing provider +credential, a configuration file the harness refused, and a workspace io was not pointed at all +report there. diff --git a/src/acp.rs b/src/acp.rs new file mode 100644 index 0000000..845c69a --- /dev/null +++ b/src/acp.rs @@ -0,0 +1,1037 @@ +//! The Agent Client Protocol wire: newline-delimited JSON-RPC 2.0 over stdio. +//! +//! An ACP client — Zed, a JetBrains IDE — spawns `io acp` as a child process and +//! speaks JSON-RPC at its stdin, reading answers off its stdout. This module is +//! the framing and the dispatch. It decides nothing about a run: the translation +//! of io-harness's events into protocol notifications is [`crate::acp_map`], and +//! the run itself is io-harness's as it is everywhere else in this crate. +//! +//! # Why the transport is written here rather than taken +//! +//! There is an official `agent-client-protocol` crate, it is Apache-2.0, it ships +//! a real `Agent` trait, and it was read before this was written. It arrives with +//! twenty-six further lockfile crates, among them `async-io`, `async-process`, +//! `async-task`, `blocking` and `polling` — a second async executor family beside +//! this crate's tokio, with its stdio transport putting `blocking::Unblock` over +//! the real stdin and stdout. A second owner of stdin is not a theoretical +//! hazard in this product: [`crate::stdin`] exists, with its own history, because +//! of what one cost. The protocol is JSON-RPC 2.0 in newline-delimited frames, +//! `serde_json` and `tokio` are already here, and the direct dependency set stays +//! at ten names. +//! +//! The cost of that choice is that protocol drift is this crate's to track, and +//! it is stated rather than discovered: [`PROTOCOL_VERSION`] is answered +//! explicitly and every method below is written against the published v1 +//! specification at . +//! +//! # The framing, which is not LSP's +//! +//! ACP does **not** use `Content-Length` headers. Its transport page is explicit: +//! *"Messages are delimited by newlines (`\n`), and MUST NOT contain embedded +//! newlines."* So a frame is one line, and the obligation runs both ways — this +//! module must never emit a message containing an interior newline, which is why +//! [`encode`] goes through `serde_json::to_string` and never a pretty-printer. +//! `to_string` escapes a newline inside a string value as `\n`, two characters, +//! so a frame stays one line however hostile its content. +//! +//! Diagnostics go to stderr. stdout is the protocol, and anything written there +//! that is not a JSON-RPC message corrupts the session — that separation is a +//! correctness property rather than a convention. +//! +//! # Why this file may deserialize JSON +//! +//! `tests/dependencies.rs` permits `serde_json::from_str` in `src/import.rs` and +//! `src/adapt.rs`, and now here, by exact path. The rule's own argument is that a +//! module deciding what somebody else's *file* means is a second opinion about a +//! question that already has one — the operator's Claude configuration and a +//! stranger's plugin manifest each have one reader for that reason. **A protocol +//! this process is the server for is not somebody else's file.** There is no +//! other reader of an ACP frame to disagree with, no on-disk format whose meaning +//! is being re-decided, and refusing to parse here would mean not speaking the +//! protocol at all. The exemption is granted for that reason and is held to the +//! properties `tests/dependencies.rs` asserts beside it. + +use std::fmt; + +use serde::{Deserialize, Serialize}; +// One name per line: `tests/dependencies.rs` forbids `use serde_json::{` +// everywhere, permitted modules included, because a name spelled around is a +// parse that appears in no sweep. This file is permitted to parse and therefore +// writes `serde_json::from_str` out in full where it does. +use serde_json::json; +use serde_json::Value; + +/// The ACP major version this adapter speaks. +/// +/// An integer rather than a string, and a major rather than a triple — the +/// specification versions the protocol by a single integer that a client and an +/// agent must agree on at `initialize`. v2 exists as a draft behind the reference +/// implementation's `unstable_protocol_v2` feature and is deliberately not spoken +/// here: answering a version this module has not implemented is worse than +/// declining one it has. +pub const PROTOCOL_VERSION: u32 = 1; + +/// Every method this adapter dispatches. +/// +/// The list is public and asserted against the declared capabilities in both +/// directions, because the failure this product has actually shipped is the other +/// one: 0.30.0 documented `io skill` while the argv door had no variant for it and +/// 1,609 tests passed over the gap, all of them entering one layer below the door. +/// A capability promising a method nobody serves is the same defect in the +/// protocol's own vocabulary. +pub const SERVED: &[&str] = &[ + "initialize", + "session/new", + "session/prompt", + "session/cancel", +]; + +/// Agent methods a capability must declare before a client may call them, and the +/// capability that declares each. +/// +/// Only the gated ones belong here. `session/new`, `session/prompt` and +/// `session/cancel` are the protocol's baseline and no capability governs them, +/// so a table that listed them would make the agreement below vacuous by +/// asserting something that cannot fail. +pub const GATED: &[(&str, &str)] = &[("session/load", "loadSession")]; + +/// Does this adapter serve `method`? +pub fn serves(method: &str) -> bool { + SERVED.contains(&method) +} + +/// The JSON-RPC error codes this module can answer with. +/// +/// The four standard ones from the JSON-RPC 2.0 specification. `ParseError` and +/// `InvalidRequest` are answered with a null id, because a frame that did not +/// parse has no id to answer to and the specification says so. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ErrorCode { + Parse, + InvalidRequest, + MethodNotFound, + InvalidParams, + Internal, +} + +impl ErrorCode { + /// The wire number. + pub fn code(self) -> i32 { + match self { + Self::Parse => -32700, + Self::InvalidRequest => -32600, + Self::MethodNotFound => -32601, + Self::InvalidParams => -32602, + Self::Internal => -32603, + } + } +} + +impl fmt::Display for ErrorCode { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let word = match self { + Self::Parse => "parse error", + Self::InvalidRequest => "invalid request", + Self::MethodNotFound => "method not found", + Self::InvalidParams => "invalid params", + Self::Internal => "internal error", + }; + f.write_str(word) + } +} + +/// A frame that arrived. +/// +/// A request carries an id and is answered; a notification carries none and is +/// not. The distinction is the protocol's and it matters at exactly one place — +/// answering a notification is a protocol violation, and `session/cancel` is a +/// notification. +#[derive(Debug, Clone, PartialEq)] +pub enum Incoming { + Request { + id: Value, + method: String, + params: Value, + }, + Notification { + method: String, + params: Value, + }, + /// 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. + /// + /// `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 + /// must not treat a protocol error as a permission decision. + Response { + id: Value, + outcome: Result, + }, +} + +impl Incoming { + /// The method name, or `""` for a response, which names none. + pub fn method(&self) -> &str { + match self { + Self::Request { method, .. } | Self::Notification { method, .. } => method, + Self::Response { .. } => "", + } + } + + /// The parameters, or the result for a response that carried one. + pub fn params(&self) -> &Value { + // A `const` rather than a temporary, so the `Err` arm has something with + // the right lifetime to hand back. + const NOTHING: &Value = &Value::Null; + match self { + Self::Request { params, .. } | Self::Notification { params, .. } => params, + Self::Response { outcome, .. } => outcome.as_ref().unwrap_or(NOTHING), + } + } +} + +/// A frame being sent. +#[derive(Debug, Clone, PartialEq)] +pub enum Outgoing { + Result { + id: Value, + result: Value, + }, + Error { + id: Value, + code: ErrorCode, + message: String, + }, + Notification { + method: String, + params: Value, + }, +} + +impl Outgoing { + /// An error answering a request whose id is not known. + /// + /// JSON-RPC 2.0 requires a null id when the id could not be determined, which + /// is every frame that failed to parse. + pub fn unattributed(code: ErrorCode, message: impl Into) -> Self { + Self::Error { + id: Value::Null, + code, + message: message.into(), + } + } +} + +/// What a frame looked like on the wire, before it was understood. +#[derive(Debug, Deserialize)] +struct Envelope { + #[serde(default)] + jsonrpc: String, + #[serde(default)] + id: Option, + #[serde(default)] + method: Option, + #[serde(default)] + params: Option, + #[serde(default)] + result: Option, + #[serde(default)] + error: Option, +} + +/// Read one frame. +/// +/// **Every failure here is answerable rather than fatal.** A client that sends a +/// malformed frame gets a JSON-RPC error and the session continues, because the +/// alternative — exiting — turns one bad frame into a lost conversation. The +/// caller distinguishes the two by the `Result`: `Err` is a frame to answer, +/// never a reason to stop reading. +pub fn decode(line: &str) -> Result { + let trimmed = line.trim(); + if trimmed.is_empty() { + return Err(Outgoing::unattributed( + ErrorCode::Parse, + "an empty line is not a JSON-RPC message", + )); + } + + let envelope: Envelope = serde_json::from_str(trimmed) + .map_err(|err| Outgoing::unattributed(ErrorCode::Parse, err.to_string()))?; + + // The version is checked rather than assumed. A client speaking a different + // JSON-RPC revision at this socket is a misconfiguration worth naming once, + // and it is cheap: the field is required by the specification. + if envelope.jsonrpc != "2.0" { + return Err(Outgoing::Error { + id: envelope.id.unwrap_or(Value::Null), + code: ErrorCode::InvalidRequest, + message: format!( + "jsonrpc must be \"2.0\"; this frame said {:?}", + envelope.jsonrpc + ), + }); + } + + let Some(method) = envelope.method else { + // A frame with an id and no method is a *response* to something this + // adapter asked. 0.36.0 asks nothing, so this arm is reached only by a + // client answering a request an older or newer `io` made; it is decoded + // rather than refused, and the handler ignores it. + let Some(id) = envelope.id else { + return Err(Outgoing::unattributed( + ErrorCode::InvalidRequest, + "a frame with neither a method nor an id is neither a request, a \ + notification nor a response", + )); + }; + // An `error` object and a `result` are different answers and the run must + // not read the first as a permission decision: a client that refused the + // request has not decided anything, and treating that as consent is the + // one mistake this seam cannot be allowed to make. + let outcome = match envelope.error { + Some(error) => Err(error + .get("message") + .and_then(Value::as_str) + .unwrap_or("the client refused the request") + .to_string()), + None => Ok(envelope.result.unwrap_or(Value::Null)), + }; + return Ok(Incoming::Response { id, outcome }); + }; + + // Absent params is `{}` rather than an error. The specification makes the + // member optional, and several methods take none. + let params = envelope.params.unwrap_or(Value::Object(Default::default())); + + match envelope.id { + Some(id) => Ok(Incoming::Request { id, method, params }), + None => Ok(Incoming::Notification { method, params }), + } +} + +/// Render a frame as the single line it must be. +/// +/// `serde_json::to_string` and never `to_string_pretty`: the transport delimits +/// messages by newline and forbids an embedded one, so a pretty-printer here +/// would split one message into as many frames as it has lines and desynchronise +/// the session permanently. The returned string carries no trailing newline — +/// the writer adds exactly one. +pub fn encode(frame: &Outgoing) -> String { + let value = match frame { + Outgoing::Result { id, result } => serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "result": result, + }), + Outgoing::Error { id, code, message } => serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "error": { "code": code.code(), "message": message }, + }), + Outgoing::Notification { method, params } => serde_json::json!({ + "jsonrpc": "2.0", + "method": method, + "params": params, + }), + }; + + // A serialization failure on a value this module built is not reachable — + // every branch above is objects, strings and numbers — but answering it with + // a panic would take the session down for a fault that is ours. The fallback + // is a well-formed internal error, still one line. + serde_json::to_string(&value).unwrap_or_else(|_| { + String::from( + r#"{"jsonrpc":"2.0","id":null,"error":{"code":-32603,"message":"internal error"}}"#, + ) + }) +} + +/// What this adapter tells a client it can do. +/// +/// **Everything omitted is unsupported, and that is the specification's rule +/// rather than this module's**: *"Clients and Agents MUST treat all capabilities +/// omitted in the `initialize` request as UNSUPPORTED."* So the honest way to +/// decline a capability is to say `false` or say nothing, which is what the two +/// excluded families do. +/// +/// `loadSession` is `false` because `session/load` is not served. io has `io +/// resume` and the mapping is plausible, but the correspondence between an ACP +/// session id and a stored run has to be designed rather than guessed, and +/// declaring a capability whose method returns "method not found" is the defect +/// this module's own [`SERVED`]/[`GATED`] agreement exists to prevent. +/// +/// The filesystem and terminal families are the client's to implement, not the +/// agent's, and this adapter never calls them — io-harness owns the disk inside +/// its own sandbox and publishes no seam to route a read through the client. +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct AgentCapabilities { + pub load_session: bool, + pub prompt_capabilities: PromptCapabilities, +} + +/// What a `session/prompt` may carry into this agent. +/// +/// All three are `false` in 0.36.0. io-harness accepts images and documents and +/// this adapter will carry them, but a capability is a promise about a wire shape +/// that has to be translated and tested, and promising one before it is is how a +/// client discovers a gap at runtime. +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct PromptCapabilities { + pub image: bool, + pub audio: bool, + pub embedded_context: bool, +} + +impl Default for AgentCapabilities { + fn default() -> Self { + Self { + load_session: false, + prompt_capabilities: PromptCapabilities { + image: false, + audio: false, + embedded_context: false, + }, + } + } +} + +impl AgentCapabilities { + /// Is the capability named `name` declared supported? + /// + /// By name rather than by field, so the agreement in `tests/acp.rs` can walk + /// [`GATED`] and ask this the same way a reader of the specification would. + /// An unknown name is not supported, which is the specification's own default + /// for anything omitted. + pub fn declares(&self, name: &str) -> bool { + match name { + "loadSession" => self.load_session, + "image" => self.prompt_capabilities.image, + "audio" => self.prompt_capabilities.audio, + "embeddedContext" => self.prompt_capabilities.embedded_context, + _ => false, + } + } +} + +/// What answers a decoded frame. +/// +/// A method may produce several frames — `session/prompt` streams +/// `session/update` notifications and then returns a result — so a handler +/// answers with a list rather than an `Option`. An empty list is the correct +/// answer to a notification, and [`serve`] does not check that for the handler: +/// a dispatcher that answered `session/cancel` would be sending an unsolicited +/// response mid-turn, and that is the handler's rule to keep because only the +/// handler knows which method it is serving. +/// +/// Native `async fn` in a trait, used generically and never behind `dyn`. The +/// crate's MSRV is well past what that needs, and it keeps the alternative — an +/// `async-trait` dependency — out of a tree whose whole point in this release is +/// that it did not grow. +pub trait Handler { + /// Answer one frame. + fn handle(&mut self, incoming: Incoming) -> impl std::future::Future>; +} + +/// Read frames until the client closes stdin, answering each. +/// +/// **Every error is answered rather than fatal, and that is the loop's whole +/// shape.** A malformed frame produces a JSON-RPC error and the session +/// continues; only end-of-input and an unreadable stream stop it. The alternative +/// — exiting on a bad frame — turns one client bug into a lost conversation, and +/// a conversation here is an operator's actual work. +/// +/// Generic over the streams so a test can drive the real loop with in-memory +/// buffers. That is deliberate rather than incidental: the stdout-backed +/// [`crate::term::Screen`] taught this crate that a transport reachable only from +/// the binary is a transport no test has ever run, and 0.32.0 spent a task +/// undoing exactly that. +/// +/// The writer is flushed after every frame. A client is blocked reading the line +/// 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, + W: tokio::io::AsyncWrite + Unpin + Send + 'static, + H: Handler, +{ + let (_unused, rx) = tokio::sync::mpsc::unbounded_channel(); + serve_with(reader, writer, handler, rx).await +} + +/// [`serve`], plus a channel the agent writes frames on without being asked. +/// +/// Two things travel that way and neither is an answer to an incoming frame: the +/// `session/update` notifications a run streams while it is working, and the +/// refusal an approval raises through [`Consulting`]. Both originate +/// inside a turn, on another task, while this loop is blocked reading — so they +/// cannot be returns from [`Handler::handle`] and there has to be a second way in. +/// +/// The two sources are joined with `select!` rather than by a writer task of their +/// own, because a `Mutex` over the writer is the alternative and a lock held +/// across an `await` on a pipe a slow client is not draining is a deadlock with no +/// error message. +pub async fn serve_with( + reader: R, + mut writer: W, + handler: &mut H, + mut outbound: tokio::sync::mpsc::UnboundedReceiver, +) -> std::io::Result<()> +where + R: tokio::io::AsyncBufRead + Unpin, + // `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. + W: tokio::io::AsyncWrite + Unpin + Send + 'static, + H: Handler, +{ + use tokio::io::{AsyncBufReadExt, AsyncWriteExt}; + + // **The writer runs on its own task, and that is the whole of why anything + // streams.** The obvious shape — one loop that `select!`s over the reader and + // the outbound channel — does not work here, and the first version of this + // function shipped that bug: `handler.handle(...).await` for a + // `session/prompt` *is the entire turn*, minutes long, and it runs inside the + // select arm's body. `select!` resolves once and then the body runs to + // completion, so the outbound channel is not polled again until the turn + // returns. Every `session/update` the run produced queued behind the result, + // and the client received the whole conversation after being told the turn had + // ended. `biased` cannot help: the loop is not at the select point to be + // biased about. + // + // A task that owns the writer is polled by the runtime independently of what + // this loop is doing, so a notification sent from inside a running turn is + // written while the turn is still running. It owns the writer outright rather + // than sharing it behind a `Mutex`, because a lock held across an `await` on a + // pipe a slow client is not draining is a deadlock with no error message. + let (frames, mut queue) = tokio::sync::mpsc::unbounded_channel::(); + let pump = tokio::spawn(async move { + while let Some(frame) = queue.recv().await { + if writer.write_all(encode(&frame).as_bytes()).await.is_err() + || writer.write_all(b"\n").await.is_err() + || writer.flush().await.is_err() + { + // The client closed the pipe. Not an error worth propagating: the + // read side sees end-of-input and ends the session. + return; + } + } + }); + + // Everything the run sends goes to the same writer, so the two sources cannot + // interleave inside a frame. + let relay = frames.clone(); + let forward = tokio::spawn(async move { + while let Some(frame) = outbound.recv().await { + if relay.send(frame).is_err() { + return; + } + } + }); + + 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), + } + + let answers = match decode(&line) { + Ok(incoming) => handler.handle(incoming).await, + Err(refusal) => vec![refusal], + }; + + for answer in answers { + // A closed writer means the client is gone; stop reading rather than + // serving a conversation nobody receives. + if frames.send(answer).is_err() { + break; + } + } + }; + + // Dropping both senders ends the pump, which flushes what it already holds + // before it returns — so a frame queued by the last turn is not lost when the + // client closes the pipe a moment later. + drop(frames); + forward.abort(); + let _ = pump.await; + outcome +} + +/// The options an ACP client is offered for one approval, as +/// `(optionId, name, kind)`. +/// +/// **Three, and the missing fourth is the point.** ACP names four +/// `PermissionOption` kinds — `allow_once`, `allow_always`, `reject_once` and +/// `reject_always` — and this adapter offers three, because +/// `io_harness::Decision` has no way to express the fourth. `Decision::Approve` +/// carries a `remember: Vec`, which is what makes `allow_always` real; +/// `Decision::Deny` carries a reason and nothing else, so a remembered refusal +/// cannot be recorded and a later matching action would ask again. +/// +/// Offering `reject_always` anyway and quietly behaving as `reject_once` is the +/// defect shape this product keeps finding in itself — a surface that accepts an +/// instruction it cannot carry out and reports success. So it is not offered, the +/// omission is documented in the guide, and the ask goes upstream as an issue. +/// +/// The ids are io-cli's own words rather than the ACP kind names, because the +/// kind is a separate field and a client renders `name`. They map one-to-one onto +/// [`crate::approval::Answer`], which is the vocabulary the interactive overlay +/// has used since 0.2.0 — one set of permissions for the product, not two. +pub const PERMISSION_OPTIONS: &[(&str, &str, &str)] = &[ + ("allow-once", "Allow once", "allow_once"), + ("allow-session", "Allow for this session", "allow_always"), + ("deny", "Deny", "reject_once"), +]; + +/// The `session/request_permission` params for one pending approval. +/// +/// `title` is the act and its target in the words io-harness used, never a +/// rewrite: the operator is being asked to authorise a specific action and the +/// sentence they read has to be the one the run will perform. +pub fn permission_params(session_id: &str, act: &str, target: &str, tool_call_id: &str) -> Value { + json!({ + "sessionId": session_id, + "toolCall": { + "toolCallId": tool_call_id, + "title": format!("{act} {target}"), + "status": "pending", + }, + "options": PERMISSION_OPTIONS + .iter() + .map(|(id, name, kind)| json!({ "optionId": id, "name": name, "kind": kind })) + .collect::>(), + }) +} + +/// What the client's answer means, or `None` when it is not an option that was +/// offered. +/// +/// **An unrecognised option is not a silent approval.** A client that answers +/// with an id this adapter never offered has said something it cannot have meant, +/// and the caller turns `None` into a denial — the direction a permission surface +/// must fail in. +pub fn answer_for(option_id: &str) -> Option { + use crate::approval::Answer; + match option_id { + "allow-once" => Some(Answer::Once), + "allow-session" => Some(Answer::Session), + "deny" => Some(Answer::Deny), + _ => None, + } +} + +/// Read the client's `session/request_permission` result. +/// +/// The specification's outcome is `{outcome: "cancelled"}` or +/// `{outcome: "selected", optionId}`. A cancellation is a refusal rather than a +/// pause: the run is mid-turn and holding it open on a client that has walked +/// away is how a session hangs with no error. +pub fn permission_answer(result: &Value) -> crate::approval::Answer { + use crate::approval::Answer; + match result.get("outcome").and_then(Value::as_str) { + Some("selected") => result + .get("optionId") + .and_then(Value::as_str) + .and_then(answer_for) + .unwrap_or(Answer::Deny), + // Every other shape — "cancelled", a missing outcome, a member this + // adapter has never seen — is a denial. There is exactly one safe + // direction to be wrong in here. + _ => Answer::Deny, + } +} + +/// The observer a run reports through, and the flag `session/cancel` sets. +/// +/// **It lives in the library and not in `src/main.rs`, and that is not a +/// preference.** Nothing under `tests/` links the driver, so a cancellation +/// written there could be neither tested nor sabotaged — the rule `AGENTS.md` +/// states and that 0.33.0 paid for with three acceptance criteria gated by +/// nothing. The driver holds wiring; the decision is here. +/// +/// The cancel flag is an `AtomicBool` behind an `Arc` rather than a channel, +/// because `Observer::event` is `&self` and synchronous, running on the run's own +/// task: it has to answer immediately, and it may be asked from several agents in +/// a contained tree at once. That is the same shape `io_harness::Approver` +/// documents for itself. +pub struct Reporter { + session_id: String, + updates: tokio::sync::mpsc::UnboundedSender, + cancelled: std::sync::Arc, +} + +impl Reporter { + /// A reporter for one session, and the handle that cancels it. + pub fn new( + session_id: impl Into, + updates: tokio::sync::mpsc::UnboundedSender, + ) -> (Self, std::sync::Arc) { + let cancelled = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + ( + Self { + session_id: session_id.into(), + updates, + cancelled: std::sync::Arc::clone(&cancelled), + }, + cancelled, + ) + } +} + +impl io_harness::Observer for Reporter { + fn event(&self, event: &io_harness::RunEvent) -> io_harness::Flow { + // The flag is read **before** the event is translated, so a cancellation + // takes effect on the first event after it is set rather than after one + // more notification has been written. A client that cancelled and then + // read another chunk would reasonably think the cancel was ignored. + if self.cancelled.load(std::sync::atomic::Ordering::Relaxed) { + return io_harness::Flow::Cancel; + } + + 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 + // about a broken pipe: the work is the operator's and the stream is a + // report on it. + let _ = self.updates.send(Outgoing::Notification { + method: "session/update".into(), + params: json!({ "sessionId": self.session_id, "update": update }), + }); + } + io_harness::Flow::Continue + } +} + +/// Serve ACP on the process's real stdin and stdout. +/// +/// The door `io acp` opens. It builds the same store, session, policy and +/// provider chain every other door builds — this adapter adds a protocol, not a +/// second way of being configured — then reads frames until the client closes +/// the pipe. +/// +/// **Nothing else in this process may touch stdin or stdout.** stdout is the +/// protocol and stdin is the frame stream; a `Screen`, raw mode or a cursor query +/// would take frames away from the reader and put bytes in the middle of a +/// message. `src/main.rs` returns here before any of that is built. +pub async fn main( + config: io_harness::Config, + root: std::path::PathBuf, + model_override: Option, +) -> Result { + // As `io exec` does, and for the same reason: this door returns from `main` + // before the interactive path resolves anything, so a bundle's declared + // program reaches no `PATH` unless it is placed here. 0.34.0 shipped with + // exactly this missing on two headless doors and the live gate found it. + for notice in crate::bundle_path::install_for(&config) { + eprintln!("{notice}"); + } + + let spec = crate::exec::spec_for(None, &config, model_override.as_deref())?; + let store = crate::settings::store_path().ok_or("no place to keep the run store")?; + let store = io_harness::Store::open(&store).map_err(|error| error.to_string())?; + let session = io_harness::Session::open(&store, &root).map_err(|error| error.to_string())?; + let policy = crate::exec::policy_for(&config, None); + + crate::provider::build( + spec, + model_override, + Editor { + store, + session, + config, + policy, + }, + ) + .await? +} + +/// The ACP session, as something [`crate::provider::build`] can run. +struct Editor { + store: io_harness::Store, + session: io_harness::Session, + config: io_harness::Config, + policy: io_harness::Policy, +} + +impl crate::provider::WithProvider for Editor { + type Out = Result; + + async fn call( + mut self, + make: impl Fn(&str) -> Result, + model: String, + ) -> Self::Out { + let provider = make(&model)?; + let (outbound, rx) = tokio::sync::mpsc::unbounded_channel(); + let mut handler = Editing { + store: &self.store, + session: &mut self.session, + config: &self.config, + policy: &self.policy, + provider: &provider, + outbound, + session_id: None, + cancel: None, + }; + + let stdin = tokio::io::BufReader::new(tokio::io::stdin()); + serve_with(stdin, tokio::io::stdout(), &mut handler, rx) + .await + .map_err(|error| error.to_string())?; + Ok(0) + } +} + +/// The live session's state, between frames. +struct Editing<'a, P: io_harness::Provider> { + store: &'a io_harness::Store, + session: &'a mut io_harness::Session, + config: &'a io_harness::Config, + policy: &'a io_harness::Policy, + provider: &'a P, + outbound: tokio::sync::mpsc::UnboundedSender, + /// The one session this adapter holds. ACP permits an agent to hold several; + /// 0.36.0 holds one and refuses a second rather than multiplexing badly. + session_id: Option, + cancel: Option>, +} + +impl Handler for Editing<'_, P> { + async fn handle(&mut self, incoming: Incoming) -> Vec { + match incoming { + Incoming::Request { id, method, params } => { + match self.request(&method, ¶ms).await { + Ok(result) => vec![Outgoing::Result { id, result }], + Err((code, message)) => vec![Outgoing::Error { id, code, message }], + } + } + Incoming::Notification { method, params } => { + self.notify(&method, ¶ms); + // A notification is never answered. Answering one is a protocol + // violation and would put an unsolicited response mid-turn. + Vec::new() + } + // 0.36.0 asks the client nothing it then waits on, so a response + // arriving here is one nobody is waiting for. Ignored rather than + // answered: the protocol has no response to a response. + Incoming::Response { .. } => Vec::new(), + } + } +} + +impl Editing<'_, P> { + async fn request( + &mut self, + method: &str, + params: &Value, + ) -> Result { + match method { + "initialize" => Ok(initialize_result(env!("CARGO_PKG_VERSION"))), + "session/new" => { + if self.session_id.is_some() { + return Err(( + ErrorCode::InvalidRequest, + "this agent holds one session per process; start another `io acp` \ + for a second" + .into(), + )); + } + let id = format!("io-{}", self.session.id()); + self.session_id = Some(id.clone()); + Ok(json!({ "sessionId": id })) + } + "session/prompt" => { + let Some(session_id) = self.session_id.clone() else { + return Err(( + ErrorCode::InvalidRequest, + "no session; send `session/new` first".into(), + )); + }; + let goal = prompt_text(params).ok_or(( + ErrorCode::InvalidParams, + "a prompt carries at least one text content block".to_string(), + ))?; + let reason = self.run(&session_id, goal).await; + Ok(json!({ "stopReason": reason })) + } + _ => Err(( + ErrorCode::MethodNotFound, + format!("`{method}` is not served by this agent"), + )), + } + } + + fn notify(&mut self, method: &str, _params: &Value) { + if method == "session/cancel" { + // Set the flag the reporter reads. Cancelling a turn that is not + // running is not an error — a client racing its own prompt would + // otherwise be refused for doing the right thing. + if let Some(cancel) = &self.cancel { + cancel.store(true, std::sync::atomic::Ordering::Relaxed); + } + } + } + + /// 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 contract = crate::exec::contract(self.config, self.session, goal, None); + let outcome = self + .session + .turn_bounded_observed( + &contract, + self.provider, + self.store, + self.policy, + &Consulting { + session_id: session_id.to_string(), + outbound: self.outbound.clone(), + run_id: self.session.id(), + step: std::sync::atomic::AtomicU32::new(0), + }, + &reporter, + ) + .await; + + self.cancel = None; + match outcome { + Ok(result) => crate::acp_map::stop_reason(&result.outcome), + // A turn that failed did not refuse anything and did not run out of + // budget, so `end_turn` with the reason on stderr is the honest + // answer until ACP grows a shape for it. + Err(error) => { + eprintln!("io: {error}"); + "end_turn" + } + } + } +} + +/// The text of a `session/prompt`, joined from its content blocks. +/// +/// ACP carries a prompt as a list of typed blocks. 0.36.0 declares no image, +/// audio or embedded-context capability, so a conforming client sends text — +/// and anything else is skipped rather than rendered as a placeholder the model +/// would try to read. +pub fn prompt_text(params: &Value) -> Option { + let blocks = params.get("prompt")?.as_array()?; + let text: Vec<&str> = blocks + .iter() + .filter(|block| block.get("type").and_then(Value::as_str) == Some("text")) + .filter_map(|block| block.get("text").and_then(Value::as_str)) + .collect(); + if text.is_empty() { + return None; + } + Some(text.join("\n")) +} + +/// The approver for an ACP turn. +/// +/// **0.36.0 stops short here, and this comment says exactly where — an earlier +/// draft of it did not, and the adversarial review caught five documents +/// describing behaviour the code did not have.** What is sent is a +/// `session/update` **notification** telling the client the action was refused. +/// It is *not* a `session/request_permission` request: raising one would mean +/// waiting for the answer, and routing that answer back into this future is the +/// piece that is not wired. Sending a request nobody reads the reply to would be +/// worse than not sending one — the client would render a prompt whose outcome +/// is ignored, which is a lie told in an interface rather than in prose. +/// +/// [`permission_params`], [`PERMISSION_OPTIONS`] and [`permission_answer`] are +/// built and tested and have no production caller yet. They are kept rather than +/// deleted because the shape of the request is settled — three options, because +/// `io_harness::Decision::Deny` carries no rules — and the piece missing is the +/// correlation, not the design. **`tests/acp.rs` asserts they agree with each +/// other and cannot assert a caller exists**, which is the same gap that let +/// 0.30.0 ship `io skill`; the release record carries it as a known limitation. +/// +/// Denying is the safe direction and it is what `io exec` already does: no +/// permission is granted that the operator did not grant, and a refusal an +/// operator can see beats one they cannot. +struct Consulting { + session_id: String, + outbound: tokio::sync::mpsc::UnboundedSender, + /// The run this is approving for, so the refusal lands on the right cell. + run_id: i64, + step: std::sync::atomic::AtomicU32, +} + +impl io_harness::Approver for Consulting { + fn decide<'a>( + &'a self, + request: &'a io_harness::Request, + ) -> io_harness::approve::DecisionFuture<'a> { + let act = format!("{:?}", request.act).to_lowercase(); + let target = request.target.clone(); + let session_id = self.session_id.clone(); + let outbound = self.outbound.clone(); + // **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 — + // which made the "a refusal an operator can see" claim above false. The + // id is built the same way `acp_map::call_id` builds it, from the run and + // the step, because that is what every `tool_call` frame carried. + let id = format!( + "{}-{}", + self.run_id, + 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) + }) + } +} + +/// 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. +pub const NOT_ROUTED: &str = "this interface could not route the approval, so it was refused"; + +/// The `initialize` result. +/// +/// `authMethods` is empty and that is legal. `io setup` is a wizard that writes +/// io-harness's configuration file, not an ACP authentication method, and +/// credentials stay exactly where they are — an empty list says "this agent needs +/// no authentication step from you", which is true. +pub fn initialize_result(agent_version: &str) -> Value { + serde_json::json!({ + "protocolVersion": PROTOCOL_VERSION, + "agentInfo": { "name": "io", "version": agent_version }, + "agentCapabilities": AgentCapabilities::default(), + "authMethods": [], + }) +} diff --git a/src/acp_map.rs b/src/acp_map.rs new file mode 100644 index 0000000..6925529 --- /dev/null +++ b/src/acp_map.rs @@ -0,0 +1,582 @@ +//! io-harness events, in the vocabulary an ACP client understands. +//! +//! [`crate::acp`] is the wire. This is the translation: one io-harness +//! [`io_harness::RunEvent`] in, at most one ACP `session/update` notification +//! out, plus the `stopReason` a `session/prompt` answers with. +//! +//! # Why this is a table and not only a match +//! +//! `EventKind` is `#[non_exhaustive]`, so the compiler can never prove a match +//! over it exhaustive and a `_ => {}` arm is mandatory. That arm is the defect +//! this module is most likely to ship: an editor that silently never shows that +//! the agent edited a file, behind a green suite, because a variant the harness +//! added went to the wildcard. It has happened here in a smaller way already — +//! io-harness 0.73.0 gave `read_skill` an optional field and started announcing +//! it in preference to the old one, and io-cli's existing test stayed green over +//! garbage output. +//! +//! So the same answer [`crate::triage`] reached applies, for the same reason: +//! [`MAPPING`] is keyed by the snake-case wire name, `tests/acp.rs` reads the +//! locked harness's own enum and fails **by name** when the two sets differ, and +//! a kind that reaches no `session/update` has to say so in the table with its +//! reason. A no-op that is written down is a decision the next release can check; +//! a no-op that falls through a wildcard is a silence nobody chose. +//! +//! `src/exec.rs` deliberately does the opposite — it forwards every event +//! verbatim rather than matching, and its own comment says why. That is right for +//! a raw NDJSON stream, whose contract is io-harness's shape. It is not available +//! here, because ACP has a fixed vocabulary and a translation is exactly what a +//! client is asking for. +//! +//! # What the client is told, and what it is not +//! +//! ACP's `session/update` carries agent output, agent thinking, tool calls and +//! plans. It has no vocabulary for a sandbox being created, a spend draw, a cache +//! mark or a bundle loading — and inventing one by pushing those through +//! `agent_message_chunk` would put io-cli's internal bookkeeping into the +//! conversation an operator reads, which is the opposite of a translation. Those +//! kinds are no-ops here and reach a machine consumer through `io exec --json` +//! and the durable trace instead. + +use io_harness::{EventKind, RunEvent}; +// One name per line rather than a brace list. `tests/dependencies.rs` forbids +// `use serde_json::{` everywhere — the permitted modules included — because +// spelling the name around is the same act as writing it, and a parse hidden +// behind an alias appears in no sweep. This module deserializes nothing at all; +// it takes the macro and the type and keeps the import shape the gate requires. +use serde_json::json; +use serde_json::Value; + +/// What a kind becomes on the wire. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Update { + /// `session/update` with `sessionUpdate: "agent_message_chunk"` — the + /// agent's answer, as the operator reads it. + MessageChunk, + /// `session/update` with `sessionUpdate: "agent_thought_chunk"` — reasoning + /// the provider returned separately. A distinct channel in ACP precisely so + /// a client can fold or hide it, which is why it is not a message chunk. + ThoughtChunk, + /// `session/update` with `sessionUpdate: "tool_call"` — a call beginning. + ToolCall, + /// `session/update` with `sessionUpdate: "tool_call_update"` — a call whose + /// status changed. + ToolCallUpdate, + /// `session/update` with `sessionUpdate: "plan"`. + Plan, + /// Nothing goes to the client. The reason column says where the fact goes + /// instead, and a row here without one is a silence nobody chose. + None, +} + +/// Every kind the locked io-harness declares, in its own declaration order, with +/// what it becomes and why. +/// +/// The order is the enum's rather than alphabetical, so this table can be read +/// down the side of `observe.rs` when the pin moves — the same convention +/// [`crate::triage::TRIAGE`] keeps, for the same reason. +pub const MAPPING: &[(&str, Update, &str)] = &[ + ( + "started", + Update::None, + "the client already knows: it sent the prompt that started this", + ), + ( + "recovery_paused", + Update::ToolCallUpdate, + "a call that failed and is being retried is a call whose status changed", + ), + ( + "step", + Update::None, + "a step boundary is io-harness's unit of work and has no ACP counterpart; \ + what happened in it is already reported by the calls and chunks inside it", + ), + ( + "step_attributed", + Update::None, + "a per-step timing breakdown, which ACP has no vocabulary for; `io exec \ + --json` and `Store::step_attributions` carry it, as `triage` records", + ), + ( + "tool_call", + Update::ToolCall, + "the call itself, with io-harness's tool name mapped onto an ACP `ToolKind`", + ), + ( + "refused", + Update::ToolCallUpdate, + "the policy said no, so the call it names ends failed rather than silently \ + not happening", + ), + ( + "approval_requested", + Update::None, + "the approver answers this, and in 0.36.0 it refuses — the client learns \ + of it through the `approval_decided` update that follows, not through a \ + second frame here. When the request round trip is wired this becomes a \ + `session/request_permission`, which is a request rather than a \ + notification and so is not a row in this table at all", + ), + ( + "approval_decided", + Update::ToolCallUpdate, + "the answer ends the call the approval was about, and the decision decides \ + the status — an approval that was refused must not leave a cell spinning", + ), + ( + "spend_draw", + Update::None, + "money is io-cli's own surface and ACP has no field for it; `/cost` and \ + `io exec --json` carry it", + ), + ( + "retry", + Update::None, + "a provider retry is below the conversation and a client that drew one per \ + attempt would report a slow network as agent activity", + ), + ( + "fell_back_to", + Update::None, + "which provider answered is io-cli's routing surface, not the client's", + ), + ( + "replan", + Update::Plan, + "the plan changed, and the plan is a first-class ACP update", + ), + ( + "stalled", + Update::None, + "reported through the run's `stopReason`, not as a notification", + ), + ( + "spawned", + Update::None, + "the agent tree is io-cli's `/fleet` surface; ACP models one session and a \ + child's own events arrive under it", + ), + ("child_detached", Update::None, "as `spawned`"), + ("child_collected", Update::None, "as `spawned`"), + ("spawn_refused", Update::None, "as `spawned`"), + ("fleet", Update::None, "as `spawned`"), + ( + "memory_wrote", + Update::None, + "the operator's memory file is io-cli's `/memory` surface", + ), + ("memory_forgot", Update::None, "as `memory_wrote`"), + ( + "todo_wrote", + Update::Plan, + "the agent's own task list is what ACP's plan update is for", + ), + ( + "question_asked", + Update::None, + "an intent question needs an answer, so it is a request rather than a \ + notification; 0.36.0 does not carry one and the run parks, which the \ + `stopReason` reports", + ), + ("questions_asked", Update::None, "as `question_asked`"), + ( + "question_answered", + Update::None, + "as `question_asked` — nothing asked it through this door", + ), + ( + "plan_proposed", + Update::Plan, + "the plan, which is a first-class ACP update", + ), + ( + "plan_decided", + Update::Plan, + "the plan's state changed and the client redraws it", + ), + ( + "reasoning", + Update::ThoughtChunk, + "the provider returned thinking separately and ACP has a separate channel \ + for it, so a client can fold it", + ), + ( + "server_tool_used", + Update::ToolCall, + "a search or fetch the provider ran for the model is still a tool call the \ + operator should see", + ), + ( + "token", + Update::MessageChunk, + "the answer itself, streamed; concatenation is the property that matters \ + and the chunk is passed through untouched", + ), + ( + "sandbox", + Update::None, + "the boundary is io-cli's status line and `/status`; ACP has no field for a \ + backend or a probe, and a client cannot act on one", + ), + ( + "mcp", + Update::None, + "a server reaching the run is configuration, not conversation; an MCP \ + *call* arrives as `tool_call`", + ), + ( + "handle_started", + Update::ToolCall, + "a background shell is a long-running call and is drawn as one", + ), + ( + "handle_polled", + Update::None, + "a poll is not an ending and carries no output, as `triage` records", + ), + ("handle_killed", Update::ToolCallUpdate, "the handle ended"), + ("handle_exited", Update::ToolCallUpdate, "the handle ended"), + ( + "handle_orphaned", + Update::ToolCallUpdate, + "the handle ended", + ), + ( + "reviewed", + Update::None, + "a verification verdict is reported through the run's `stopReason`", + ), + ( + "routed", + Update::None, + "which model answered is io-cli's routing surface. **Read the note below \ + before drawing this**: 0.75.0 emits it with an empty `from`", + ), + ( + "plugin_loaded", + Update::None, + "a bundle loading is configuration and fires at step 0 of every turn", + ), + ("lsp_started", Update::None, "as `plugin_loaded`"), + ( + "browser_started", + Update::None, + "the browser's own activity arrives as `tool_call`", + ), + ("browser_navigated", Update::None, "as `browser_started`"), + ( + "speculated", + Update::None, + "speculation is an optimisation the conversation does not describe", + ), + ("plugin_dropped", Update::None, "as `plugin_loaded`"), + ( + "rewound", + Update::None, + "a rewind is io-cli's `/undo` and reaches no ACP session", + ), + ("reverted", Update::None, "as `rewound`"), + ( + "answered", + Update::None, + "the answer already streamed as `token` chunks; this names the turn it \ + belongs to and would be a duplicate", + ), + ( + "compacted", + Update::None, + "folding older context is bookkeeping the conversation does not describe", + ), + ("cache_marked", Update::None, "as `compacted`"), + ( + "prompt_composed", + Update::None, + "what was sent is `/context`'s surface, and putting it in the conversation \ + would echo the operator's own prompt back at them", + ), + ( + "contained", + Update::None, + "as `sandbox` — the boundary is io-cli's status line", + ), + ( + "dialed", + Update::None, + "reaching the provider is below the conversation", + ), + ( + "finished", + Update::None, + "the run's end is the `session/prompt` **result** and its `stopReason`, not \ + a notification; sending both would end the turn twice", + ), +]; + +/// What `name` becomes, or `None` when the locked harness declares a kind this +/// table has never heard of. +/// +/// `None` and [`Update::None`] are different answers and the difference is the +/// whole point: the second is a decision, the first is a gap. +pub fn update_for(name: &str) -> Option { + MAPPING + .iter() + .find(|(kind, ..)| *kind == name) + .map(|(_, update, _)| *update) +} + +/// Every tool io-harness declares, and the ACP `ToolKind` it is drawn as. +/// +/// The nine kinds are the specification's: `read`, `edit`, `delete`, `move`, +/// `search`, `execute`, `think`, `fetch` and `other`. +/// +/// **`other` appears in this table, and that is the point of having a table.** +/// It is a correct answer for several harness tools — asking the operator a +/// question is not a read, an edit or an execution in ACP's vocabulary — so a +/// gate asserting "no harness tool is `other`" would be false. What must not +/// happen is a tool reaching `other` because nobody looked at it. Listing every +/// name here makes the two cases different states: an entry saying `other` is a +/// decision, and a name absent from the table is a gap, and `tests/acp.rs` reads +/// the locked harness's own tool names and fails on the second. +/// +/// The wildcard in [`tool_kind`] therefore serves only names this crate has never +/// seen — a bundle's own tool, an MCP server's tool — where `other` is the only +/// honest answer and inferring `edit` from a substring would tell a client a read +/// was a write. +pub const TOOL_KINDS: &[(&str, &str)] = &[ + // Reads. + ("read_file", "read"), + ("list_dir", "read"), + ("view_image", "read"), + ("read_skill", "read"), + ("read_messages", "read"), + ("xlsx_read", "read"), + ("xlsx_sheets", "read"), + ("docx_read", "read"), + ("pptx_read", "read"), + ("pdf_read", "read"), + ("barcode_decode", "read"), + ("git_log", "read"), + ("git_status", "read"), + ("git_diff", "read"), + ("browser_read", "read"), + ("lsp_definition", "read"), + ("lsp_references", "read"), + ("lsp_symbols", "read"), + ("lsp_hover", "read"), + // Writes. `remember` is an edit because it appends to the operator's memory + // file, which is a file on their disk. + ("write_file", "edit"), + ("edit_file", "edit"), + ("patch_file", "edit"), + ("xlsx_write", "edit"), + ("xlsx_set_cell", "edit"), + ("docx_write", "edit"), + ("pdf_write", "edit"), + ("pdf_fill_form", "edit"), + ("pdf_watermark", "edit"), + ("lsp_rename", "edit"), + ("remember", "edit"), + // `forget` removes one, which ACP has its own kind for. + ("forget", "delete"), + // Searches. + ("grep", "search"), + ("find", "search"), + // Executions. A git write is an execution rather than an edit: it changes the + // repository's state through a program, and a client offering to preview a + // diff for `git_commit` would be offering the wrong thing. + ("exec", "execute"), + ("shell", "execute"), + ("shell_start", "execute"), + ("shell_poll", "execute"), + ("shell_kill", "execute"), + ("check", "execute"), + ("git_add", "execute"), + ("git_commit", "execute"), + ("git_branch", "execute"), + ("git_worktree", "execute"), + ("spawn_agent", "execute"), + // Fetches — the browser reaches something outside the workspace. + ("browser_navigate", "fetch"), + ("browser_screenshot", "fetch"), + ("browser_click", "fetch"), + ("browser_type", "fetch"), + ("browser_scroll", "fetch"), + // Thinking. Planning is the closest of the nine and it is what a client folds + // by default, which is the behaviour these deserve. + ("propose_plan", "think"), + ("todo_write", "think"), + // Genuinely `other`, decided rather than defaulted. Asking the operator a + // question and messaging a sibling agent are neither reads, edits nor + // executions in ACP's vocabulary, and forcing one of the eight onto them + // would mislabel them in the one surface a client renders. + ("ask_question", "other"), + ("ask_questions", "other"), + ("send_message", "other"), +]; + +/// Is `name` a tool this adapter has classified deliberately? +/// +/// The distinction [`TOOL_KINDS`] exists to make: a name in the table answering +/// `other` is a decision, a name absent from it is a gap. +pub fn classified(name: &str) -> bool { + TOOL_KINDS.iter().any(|(tool, _)| *tool == name) +} + +/// io-harness's tool name as one of ACP's nine `ToolKind`s. +/// +/// A name this crate has never seen — a bundle's own tool, an MCP server's tool — +/// is `other`, which is the only honest answer for it. See [`TOOL_KINDS`]. +pub fn tool_kind(name: &str) -> &'static str { + TOOL_KINDS + .iter() + .find(|(tool, _)| *tool == name) + .map(|(_, kind)| *kind) + .unwrap_or("other") +} + +/// 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 +/// several outcomes share an answer — which is correct, because a client acts on +/// the reason and only these five distinctions change what it does. +/// +/// **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 +/// be a lie about the agent's willingness. 0.36.0 carries no way to answer those +/// through ACP — `session/prompt` returns and the run is resumable with +/// `io resume`, which the guide says. +pub fn stop_reason(outcome: &io_harness::RunOutcome) -> &'static str { + use io_harness::RunOutcome as O; + match outcome { + O::Success { .. } | O::Finished { .. } => "end_turn", + O::Cancelled { .. } => "cancelled", + O::Denied { .. } | O::Refused { .. } | O::PlanRejected { .. } => "refusal", + O::StepCapReached { .. } + | O::VerificationFailed { .. } + | O::Stalled { .. } + | O::Escalated { .. } => "max_turn_requests", + O::TimeBudgetExceeded { .. } + | O::CostBudgetExceeded { .. } + | O::BudgetCeilingReached { .. } => "max_tokens", + O::AwaitingRecovery { .. } + | O::AwaitingApproval { .. } + | O::AwaitingAnswer { .. } + | O::AwaitingPlan { .. } => "end_turn", + // `RunOutcome` is `#[non_exhaustive]`. An outcome this build has not seen + // ends the turn rather than claiming a reason it cannot know — and + // `tests/acp.rs` reads the locked enum and fails by name, so this arm is + // the behaviour between a harness release and the release that maps it, + // never the permanent answer. + _ => "end_turn", + } +} + +/// The `session/update` params for one event, or `None` when this kind is +/// deliberately not sent. +/// +/// The `sessionId` is the caller's — this function does not know it — so the +/// returned object carries the `update` member alone and the handler wraps it. +pub fn translate(event: &RunEvent) -> Option { + let update = match &event.kind { + EventKind::Token { text } => json!({ + "sessionUpdate": "agent_message_chunk", + "content": { "type": "text", "text": text }, + }), + EventKind::Reasoning { text, .. } => json!({ + "sessionUpdate": "agent_thought_chunk", + "content": { "type": "text", "text": text }, + }), + 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 + // call no id of its own — the step is the finest grain available and + // it is what `src/events.rs` already uses to close an open cell. + "toolCallId": call_id(event), + "title": if target.is_empty() { name.clone() } else { format!("{name} {target}") }, + "kind": tool_kind(name), + "status": "in_progress", + }), + EventKind::ServerToolUsed { .. } => json!({ + "sessionUpdate": "tool_call", + "toolCallId": call_id(event), + "title": "provider tool", + "kind": "fetch", + "status": "in_progress", + }), + EventKind::Refused { act, target, .. } => json!({ + "sessionUpdate": "tool_call_update", + "toolCallId": call_id(event), + "status": "failed", + "title": format!("{act} {target} refused"), + }), + // **The decision decides the status, and reporting `in_progress` for all + // three was a defect the adversarial review found.** io-harness carries + // `"approve"`, `"deny"` or `"defer"` on this variant. Because 0.36.0's + // approver always denies, an unconditional `in_progress` meant *every* + // grey-tier action in an editor session drew a cell that spins for ever + // on a call that will never complete — the opposite of the refusal the + // operator needs to see. + EventKind::ApprovalDecided { decision, .. } => json!({ + "sessionUpdate": "tool_call_update", + "toolCallId": call_id(event), + "status": match decision.as_str() { + "approve" => "in_progress", + // A defer is not a completion and not a failure; the run stops + // and the call never happened, which for a client's purposes is + // the same visible end as a refusal. + _ => "failed", + }, + }), + EventKind::RecoveryPaused { .. } => json!({ + "sessionUpdate": "tool_call_update", + "toolCallId": call_id(event), + "status": "pending", + }), + EventKind::HandleStarted { .. } => json!({ + "sessionUpdate": "tool_call", + "toolCallId": call_id(event), + "title": "background shell", + "kind": "execute", + "status": "in_progress", + }), + EventKind::HandleExited { .. } + | EventKind::HandleKilled { .. } + | EventKind::HandleOrphaned { .. } => json!({ + "sessionUpdate": "tool_call_update", + "toolCallId": call_id(event), + "status": "completed", + }), + EventKind::PlanProposed { .. } + | EventKind::PlanDecided { .. } + | EventKind::Replan { .. } + | EventKind::TodoWrote { .. } => json!({ + "sessionUpdate": "plan", + // The entries are deliberately empty in 0.36.0: io-harness's plan + // shapes are four different types and rendering them into ACP's + // `PlanEntry` is a translation of its own. A client is told the plan + // moved, which is true, rather than a fabricated list. + "entries": [], + }), + // Everything else is a deliberate no-op, and `MAPPING` above says which + // and why. The wildcard is mandatory because `EventKind` is + // `#[non_exhaustive]`; what stops it becoming a silent drop is that + // `tests/acp.rs` walks the locked harness's own enum against `MAPPING` + // and fails by name on a kind neither of them has heard of. + _ => return None, + }; + Some(update) +} + +/// The handle a client correlates a call and its later update by. +/// +/// io-harness gives a tool call no id of its own, so the step is the finest grain +/// available — which is also what `src/events.rs` already uses to close an open +/// cell, so the two surfaces agree by construction rather than by coincidence. +/// The run id is in it because a contained turn's children have their own. +fn call_id(event: &RunEvent) -> String { + format!("{}-{}", event.run_id, event.step) +} diff --git a/src/adapt.rs b/src/adapt.rs index e632f07..926cf96 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.74.0/src/plugin.rs:1097`), and the reason is the one this +//! (`io-harness-0.76.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.74.0/src/template.rs:279`), so what is translated is the +/// (`io-harness-0.76.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"; @@ -497,7 +497,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.74.0/src/plugin.rs:1097`), because +/// `templates` in every scope (`io-harness-0.76.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 @@ -632,7 +632,7 @@ pub fn generate(bundle: &Path, name: &str, into: &Path) -> 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.74.0/src/plugin.rs:977`), because a +/// manifest in every scope (`io-harness-0.76.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 +1184,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.74.0/src/mcp.rs:301`), so a bundle declaring `sse` is declaring +/// (`io-harness-0.76.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/cli.rs b/src/cli.rs index a225ff7..f7f8a96 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -87,6 +87,18 @@ pub enum Command { Plugin(Manage), /// Read or write one configuration key, without opening a session. Config(Manage), + /// Serve the Agent Client Protocol on stdin and stdout, for an editor. + /// + /// **Not run by hand.** An ACP client — Zed, a JetBrains IDE — spawns this as + /// a child process and speaks newline-delimited JSON-RPC 2.0 at it. stdout is + /// 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. + Acp, /// Install, list or remove a skill, without opening a session. /// /// **Missing until 0.30.1, and `io skill add` did not exist because of it.** diff --git a/src/commands.rs b/src/commands.rs index e2fd735..5ca47a3 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1431,7 +1431,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.74.0/src/config.rs:2680`). +/// (`read_instructions`, `io-harness-0.76.0/src/config.rs:2799`). pub const UNREAD_MARK: &str = "-"; /// What one row of the memory page stands for. @@ -1647,8 +1647,8 @@ 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.74.0/src/config.rs:190`, read at -/// `:2120`) and there is nothing on any +/// (`DEFAULT_INSTRUCTIONS`, `io-harness-0.76.0/src/config.rs:190`, read at +/// `:2808`) and there is nothing on any /// other surface to pull on. pub fn instruction_said( file: &crate::memory::Instruction, diff --git a/src/configure.rs b/src/configure.rs index fd99ef9..896058a 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -255,8 +255,8 @@ 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.74.0/src/policy.rs:129`) and each spelling is `Effect::as_str` -/// (`:143`), which is the word io-harness's own deserializer reads. +/// (`io-harness-0.76.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 /// and a `match` mapping them to three string literals — and the copy was the @@ -266,7 +266,7 @@ pub enum Kind { /// left it compiling and writing a word the schema rejects. Taking the string from /// the variant closes the one hole the census could not. /// -/// `Effect` is not `#[non_exhaustive]` (`policy.rs:90-92`), so `ALL` is a census +/// `Effect` is not `#[non_exhaustive]` (`policy.rs:92-94`), so `ALL` is a census /// io-harness itself keeps complete rather than a list that can quietly fall /// behind, and there is no wildcard on this side to swallow a fourth effect. /// `tests/configure.rs` still round-trips every string through io-harness's own @@ -282,10 +282,10 @@ pub fn effects() -> Vec { /// The `ExecMode` variants, spelled by io-harness itself. /// -/// **The list is `ExecMode::ALL` (`io-harness-0.74.0/src/sandbox.rs:453`) and the -/// spellings are `ExecMode::as_str` (`:431`).** io-cli wrote the variant list out +/// **The list is `ExecMode::ALL` (`io-harness-0.76.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:378-381`), and +/// choice made here: `ExecMode` is `#[non_exhaustive]` (`sandbox.rs:407`), and /// a caller outside the defining crate cannot enumerate such an enum without a /// wildcard arm that silently swallows the next variant. The issue this crate /// filed asking for the enumeration is io-harness#218, and 0.71.0 answers it — @@ -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.74.0/src/contract.rs:652,670,686`), +/// `DEFAULT_MAX_RETRIES` = 2 (`io-harness-0.76.0/src/contract.rs:673,691,707`), /// 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.74.0/src/pricing.rs:268`) +/// out of the files.** `PriceTable::models` (`io-harness-0.76.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.74.0/src/config.rs:605`), so a second discovery re-runs +/// it reads (`io-harness-0.76.0/src/config.rs:613`), 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.74.0/src/config.rs:2509`). Matched rather than the whole + /// (`io-harness-0.76.0/src/config.rs:2628`). 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.74.0/src/config.rs:2719`, the `cmd` arm at -/// `:2241`); this comment claimed two until +/// (`substitute`, `io-harness-0.76.0/src/config.rs:2838`, the `cmd` arm at +/// `:2958`); 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/contract.rs b/src/contract.rs index 122efb9..a561e08 100644 --- a/src/contract.rs +++ b/src/contract.rs @@ -535,7 +535,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.74.0/src/skills.rs`), and `TaskContract::discover_skills` +/// (`io-harness-0.76.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. @@ -560,7 +560,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.74.0/src/config.rs:2719` — there is no tilde +/// (`substitute`, `io-harness-0.76.0/src/config.rs:2838` — 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 0440fdc..df49747 100644 --- a/src/cost.rs +++ b/src/cost.rs @@ -78,6 +78,23 @@ pub struct Total { pub unknown: u64, /// Calls whose model the table does not price. pub unpriced: u64, + /// Calls that reported usage but no cache-write count. + /// + /// **The field the mixed fold needs, and the release that added `Option` to + /// that count shipped without it first.** `add_reported` folds one reporting + /// call and one silent one to `Some(n)`, and `n` is a **floor** — at least + /// that many writes happened. Drawn as a plain number, and used to compute + /// "of which fresh" as the prompt less both cached parts, the second figure + /// becomes a *ceiling* presented as a fact, too large by exactly the amount + /// nobody measured. That is verbatim the failure the comment in `section` + /// says must not happen; the guard there distinguished only "no call + /// reported" from "some call reported", so a mixed fold took the confident + /// arm. + /// + /// Not hypothetical: `openai_wire` reports no cache-write count at all and + /// Anthropic reports one only where a cache was created, so any session that + /// changes model across those two vendors folds both shapes into one total. + pub unreported_writes: u64, /// The token split, summed. Cache and reasoning are breakdowns of the prompt /// and completion figures beside them, never additions to them. pub usage: Usage, @@ -102,6 +119,9 @@ impl Total { total.unknown += 1; continue; }; + if usage.cache_write_tokens.is_none() { + total.unreported_writes += 1; + } add(&mut total.usage, &usage); let priced = call .model @@ -132,11 +152,33 @@ fn add(into: &mut Usage, from: &Usage) { into.completion_tokens += from.completion_tokens; into.total_tokens += from.total_tokens; into.cache_read_tokens += from.cache_read_tokens; - into.cache_write_tokens += from.cache_write_tokens; + into.cache_write_tokens = add_reported(into.cache_write_tokens, from.cache_write_tokens); into.reasoning_tokens += from.reasoning_tokens; into.server_tool_requests += from.server_tool_requests; } +/// Sum two counts either of which the provider may not have reported. +/// +/// io-harness 0.76.0 made `cache_write_tokens` an `Option`, which is the +/// shape this module's own first honesty rule has always wanted: a count the +/// provider reported no usage for is unknown, never zero. Every OpenRouter call +/// reports `None`, and OpenRouter is where this product's evidence is taken, so +/// the difference is not theoretical. +/// +/// **Unknown is absorbing in one direction only.** Two silences stay a silence. +/// One silence beside a number does not erase the number — a turn that reported +/// 400 cache writes and a turn that reported nothing sum to *at least* 400, and +/// answering `unknown` there would hide a figure the provider did give us. The +/// residual dishonesty is that the sum is a floor rather than a total, which is +/// the same shape as an unpriced model's money on this page, and is said out +/// loud in the same way rather than papered over. +fn add_reported(into: Option, from: Option) -> Option { + match (into, from) { + (None, None) => None, + (a, b) => Some(a.unwrap_or(0) + b.unwrap_or(0)), + } +} + /// Where the prices came from, for the line at the foot of the page. /// /// **The claim behind every `$` on the page**, and it is carried rather than @@ -289,23 +331,48 @@ fn section(total: &Total) -> Vec { // being named as parts of it rather than by being drawn further right — this // page has no columns to indent into, and a reader in `--plain` has only the // words. - if usage.cache_read_tokens > 0 || usage.cache_write_tokens > 0 { + // A provider that did not report cache writes at all is a third state beside + // "wrote none" and "wrote some", and the block is drawn for it: an operator + // seeing cache reads and no written line would read the absence as zero, + // which is the reading this module exists to prevent. + if usage.cache_read_tokens > 0 || usage.cache_write_tokens.is_some_and(|n| n > 0) { rows.push(Row::fact( "of which cache read", tokens(usage.cache_read_tokens), )); rows.push(Row::fact( "of which cache written", - tokens(usage.cache_write_tokens), + // A mixed fold is a floor, not a count, so it is drawn as one. The + // arm that mattered is the middle case: `Some(n)` with at least one + // call silent means "at least n", and a bare number there is a claim + // nobody can support. + match (usage.cache_write_tokens, total.unreported_writes) { + (Some(count), 0) => tokens(count), + (Some(count), _) => format!("{} or more", tokens(count)), + (None, _) => UNREPORTED.to_string(), + }, )); + // Fresh is the prompt less both cached parts, so a cache-write count + // nobody reported makes the remainder unknown too. Substituting zero + // here would draw a fresh figure that is too large by exactly the amount + // that was never measured, and it would look like a fact. rows.push(Row::fact( "of which fresh", - tokens( - usage - .prompt_tokens - .saturating_sub(usage.cache_read_tokens) - .saturating_sub(usage.cache_write_tokens), - ), + // **`unreported_writes` is the condition, not just `None`.** Fresh is + // the prompt less both cached parts, so it inherits every doubt the + // write count carries. With one call silent the subtraction is from a + // floor, which makes this a ceiling — and a ceiling drawn as a plain + // number is the exact shape of the defect the row above avoids. There + // is no honest number here, so there is no number. + match (usage.cache_write_tokens, total.unreported_writes) { + (Some(written), 0) => tokens( + usage + .prompt_tokens + .saturating_sub(usage.cache_read_tokens) + .saturating_sub(written), + ), + _ => UNREPORTED.to_string(), + }, )); } rows.push(Row::fact("completion", tokens(usage.completion_tokens))); @@ -459,6 +526,15 @@ pub fn money(micros: u64) -> String { } /// A token count, in the spelling the status line already uses. +/// What a count the provider never reported is drawn as. +/// +/// A word rather than a dash or an empty cell, because this page is read in +/// `--plain` where a dash is indistinguishable from a rendering artefact, and +/// because the value has to be legible when the page is quoted into a bug report. +/// It is deliberately not `0`: see `add_reported` and this module's first +/// honesty rule. +const UNREPORTED: &str = "unknown"; + fn tokens(count: u64) -> String { crate::status::format_tokens(count) } diff --git a/src/events.rs b/src/events.rs index d79b651..30fd582 100644 --- a/src/events.rs +++ b/src/events.rs @@ -1880,7 +1880,7 @@ impl Events { // brag about concurrency. // // **Only when something was discarded.** io-harness emits this - // whenever `started > 0` (`run/step.rs:1272`), so a step that + // whenever `started > 0` (`run/step.rs:1401`), 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 8dae83f..b933493 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -1483,8 +1483,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.74.0/src/run.rs:1790`, `:2089`, `:3003`). Recovery has - // `resume_with_recovery_observed` (`:2551`) and nothing tree-aware, so it + // (`io-harness-0.76.0/src/run.rs:1790`, `:2109`, `:3145`). Recovery has + // `resume_with_recovery_observed` (`:2571`) 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/home.rs b/src/home.rs index abab417..a973ce5 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.74.0/src/config.rs:2096`, the Windows branch at `:1910`), and +//! (`io-harness-0.76.0/src/config.rs:2146`, the Windows branch at `:2164`), 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 diff --git a/src/import.rs b/src/import.rs index bddb589..d45196b 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.74.0/src/policy.rs:63`). So the nearest thing to a faithful +//! (`Act::Exec`, `io-harness-0.76.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 d8c6061..2bf9fa0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -57,6 +57,8 @@ //! //! [io-harness]: https://docs.rs/io-harness +pub mod acp; +pub mod acp_map; pub mod adapt; pub mod app; pub mod approval; diff --git a/src/main.rs b/src/main.rs index 212d583..dd9927c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -214,6 +214,20 @@ async fn run(report: &mut Vec) -> Result { return io_cli::exec::main(args, config, root, cli.model).await; } + // **`io acp` leaves here too, and it is the strictest of the headless doors.** + // stdout is the protocol from this point on: a line of prose there is not + // noise a client tolerates, it is a frame boundary in the wrong place, and + // every message after it is parsed against the wrong offset. So the migration + // report goes to stderr like the others, and nothing else in this process may + // write to stdout or read from stdin — no `Screen`, no raw mode, no cursor + // query. See `src/stdin.rs` for what a second reader of stdin cost in 0.13.1. + if let Some(Subcommand::Acp) = cli.command { + for line in report.drain(..) { + eprintln!("{line}"); + } + return io_cli::acp::main(config, root, cli.model).await; + } + // `io resume` leaves by the same door, for the same two reasons: it is // headless, and `io resume --list` in CI has no terminal to be refused for. // It answers a missing provider with a sentence of its own, so it must pass @@ -6941,10 +6955,22 @@ fn forms(app: &App) -> (bool, io_cli::term::Graphics) { /// hands the turn its caps as `contained.then_some(…)` — so the mid-turn door /// passes `containment.is_some()` and says exactly what the idle one says. fn routing_notices(app: &mut App, config: &Config, contained: bool) { - let Some(routing) = io_cli::settings::stored(config) - .0 - .and_then(|stored| stored.routing) - else { + let stored = io_cli::settings::stored(config).0; + + // **Above the `let-else`, and that placement is the whole of whether this + // works.** io-harness 0.76.0 grew a `[routing]` table of its own, and the + // case that most needs saying is the one where the operator has *only* that + // one — no `[app.io-cli.routing]` at all. Below the early return, that case + // is unreachable: the `else` fires first and the function is over. Found by + // reading the control flow rather than by the notice failing to appear, + // which it would have done silently. + if let Some(said) = + io_cli::routing::native_notice(config, stored.as_ref().and_then(|s| s.routing.as_ref())) + { + app.record(Tone::Warning, said); + } + + let Some(routing) = stored.and_then(|stored| stored.routing) else { return; }; if let Some(said) = io_cli::routing::describe(&routing) { diff --git a/src/manage.rs b/src/manage.rs index ded385e..4d79f8b 100644 --- a/src/manage.rs +++ b/src/manage.rs @@ -1394,7 +1394,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.74.0/src/mcp.rs:439-447`: the + // silent no-op on an HTTP server (`io-harness-0.76.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/memory.rs b/src/memory.rs index fdeb187..4d76af7 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -27,12 +27,12 @@ //! //! Worth knowing before a surface promises otherwise. `read_instructions` joins //! each name in `[instructions] files` to the **discovery root** -//! (`io-harness-0.74.0/src/config.rs:2680`), and the default list is exactly -//! `["AGENTS.md"]` (`DEFAULT_INSTRUCTIONS`, `config.rs:159`). 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, because a -//! relative name is resolved against the workspace and the home is not the -//! workspace. **This module writes the file. Making the harness read it is +//! (`io-harness-0.76.0/src/config.rs:2799`), 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, +//! because a relative name is resolved against the workspace and the home is not +//! the workspace. **This module writes the file. Making the harness read it is //! `[instructions]`' business and the caller's.** [`install`] is where that //! business is settled, and [`view`] is how a surface reports the outcome //! honestly rather than optimistically. @@ -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.74.0/src/config.rs:2680`). +/// (`read_instructions`, `io-harness-0.76.0/src/config.rs:2799`). /// /// 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 diff --git a/src/recall.rs b/src/recall.rs index 357115b..8e21c84 100644 --- a/src/recall.rs +++ b/src/recall.rs @@ -579,8 +579,8 @@ pub fn pin( /// - **The entry's own `run_id`** — the run that wrote it, which /// [`Remembered::run_id`] already carries — is the tempting one and is the /// trap. `memory_snapshots` is unique on `(run_id, workspace, key)` -/// (`src/state/schema.rs:763-764`) and the insert is `INSERT OR IGNORE` -/// (`src/state/memory.rs:839`), so the restore point that run took when it +/// (`src/state/schema.rs:848-849`) and the insert is `INSERT OR IGNORE` +/// (`src/state/memory.rs:871`), so the restore point that run took when it /// *wrote* the entry already owns that slot and this one is silently dropped. /// The row that stays says the entry was **absent** before that run, so a /// rewind would delete it rather than put it back — the exact opposite of what diff --git a/src/reload.rs b/src/reload.rs index a36bb0b..59dfcb6 100644 --- a/src/reload.rs +++ b/src/reload.rs @@ -2,7 +2,7 @@ //! //! **io-harness takes the instruction files as a snapshot and never takes //! another.** `read_instructions` runs inside `Config::discover` -//! (`io-harness-0.74.0/src/config.rs:2680`), composes each named file into a +//! (`io-harness-0.76.0/src/config.rs:2799`), composes each named file into a //! constraint, and stores the result in a private `Config.instructions` field. //! There is no `Config::reload`, and `Config::with_profile` clones the field //! rather than going back to disk — so a `Config` is exactly as old as the call diff --git a/src/routing.rs b/src/routing.rs index 82f50b5..453d0c5 100644 --- a/src/routing.rs +++ b/src/routing.rs @@ -92,6 +92,35 @@ //! disclosure, and [`inert_under_containment`] is it. It is deliberately narrow: //! see that function for why warning every operator with a routing section would //! be worse than warning none. +//! +//! # io-harness grew a `[routing]` table of its own in 0.76.0 +//! +//! `Config::apply_to` now merges a **user-scope** `[routing]` onto the contract +//! key by key (`config.rs:2076`): `escalate_after`/`escalate_to`, +//! `downshift_under`/`downshift_to`, `require_primary`, and `mechanical` — which +//! names the model that reads the whole transcript when a fold summarises it, and +//! for which io-cli offers no key at all. io-cli builds its own section *after* +//! that merge (`contract.rs:356`) through `TaskContract::with_routing`, whose body +//! is `self.routing = Some(routing)` (`contract.rs:1292`). It replaces. So a +//! `[app.io-cli.routing]` that names a rule takes the whole of `[routing]` back +//! off the contract, `mechanical` included, and until this release it did so in +//! silence. +//! +//! **io-cli does not merge the two, and that is the decision rather than the +//! omission.** Merging would make this crate choose precedence between its own +//! section and the dependency's — which key of which table wins when both name an +//! escalation — and that is a second opinion about what a `Routing` means, which +//! is the shape `tests/dependencies.rs` exists to keep out of this crate. The +//! behaviour is therefore unchanged: io-cli's section wins. What changes is that +//! [`native_notice`] says so. +//! +//! The collision can only be written in the user scope, which is why the test for +//! it needs a user-scoped fixture rather than `Config::from_toml`. `routing` is in +//! io-harness's `REFUSED_SECTIONS` (`config.rs:2394`), so `io.toml`, +//! `io.local.toml` and a `[profile]` body may not declare it; the match is +//! `contains_key("routing")` against the **top-level** table (`config.rs:2600`), +//! so `[app.io-cli.routing]` — nested under `app` — is untouched by that rule in +//! every scope. use serde::{Deserialize, Serialize}; @@ -430,3 +459,72 @@ pub fn inert_under_containment(settings: &Settings, contained: bool) -> Option) -> Option { + // The harness's table is top-level, so `app.io-cli.routing.*` — recorded by + // `origins` under `app` — matches neither arm. The bare key is tested as well + // as the dotted prefix because `origins` records leaves: a `routing` that is + // not a table is one key rather than a prefix, and a rule spelled to catch only + // the prefix would be a rule with a hole exactly where a malformed file is. + if !config + .origins() + .any(|(key, _)| key == "routing" || key.starts_with("routing.")) + { + return None; + } + if settings + .and_then(|settings| routing(settings).ok().flatten()) + .is_some() + { + return Some( + "[routing] and [app.io-cli.routing] are both configured, and only \ + [app.io-cli.routing] reaches the contract: io-harness merges [routing] onto it \ + key by key, io-cli then sets its own section through with_routing, and that \ + builder replaces rather than merges, so every key of [routing] is dropped, \ + mechanical included. Write the rules in one section or the other." + .to_string(), + ); + } + Some( + "[routing] is configured, and no io-cli surface lists it: /config edits \ + [app.io-cli.routing] only, and every routing notice this product prints describes \ + that section alone. io-harness merges [routing] onto the contract itself, so it \ + reaches the run through the dependency rather than through anything here." + .to_string(), + ) +} diff --git a/src/triage.rs b/src/triage.rs index 12b8ad7..ab03b53 100644 --- a/src/triage.rs +++ b/src/triage.rs @@ -41,7 +41,7 @@ pub enum Disposition { Silent, } -/// Every kind io-harness 0.72 declares, in its own declaration order. +/// Every kind io-harness 0.76 declares, in its own declaration order. /// /// The order is the enum's rather than alphabetical so that this table can be /// read down the side of `observe.rs` when the pin moves. @@ -57,6 +57,33 @@ pub const TRIAGE: &[(&str, Disposition, &str)] = &[ Disposition::Line, "the step line and its tool cells", ), + // **0.36.0 — a breakdown emitted beside every step, and the one place a line + // would cost more than it says.** io-harness 0.75.0 emits this from the same + // place as `step` above, for every committed step, carrying the span the step + // line already draws plus the phases that span divides into. Drawn as a line + // it would put a second row under every step row, restating a number the row + // above it carries — a transcript twice as long to say the same thing once + // more. + // + // **The route is the machine surfaces, and it says so rather than naming an + // operator surface that does not draw these numbers.** `/store` and + // `/context` are both registered commands, so a route naming either would + // have satisfied `f9_every_silent_route_names_a_surface_that_exists` — which + // checks that a named command exists, not that it renders the fact — and been + // a claim this build does not honour. io-harness's own declaration points an + // observer at `Store::step_attributions`, which is the reader that has the + // phases and the time to first token this event deliberately leaves off. + ( + "step_attributed", + Disposition::Silent, + "`io exec --json`, which forwards it verbatim, and the durable trace; the harness's own \ + declaration points an observer at `Store::step_attributions` for the same figures. \ + **Silent is a close call and it is recorded as one.** Where a slow step spent its wall \ + clock is exactly the kind of fact this interface argues should be visible — but it \ + arrives beside every committed step, so a line is a second row under every step row \ + restating that step's span, and no surface in this release renders the phase breakdown \ + that would earn the room", + ), ( "tool_call", Disposition::Silent, diff --git a/tests/acp.rs b/tests/acp.rs new file mode 100644 index 0000000..071bbf9 --- /dev/null +++ b/tests/acp.rs @@ -0,0 +1,1122 @@ +//! The ACP wire: framing, dispatch agreement, and the capability handshake. +//! +//! These tests drive the transport rather than describe it. A protocol adapter +//! is the one part of this product whose correctness is entirely about bytes +//! somebody else will read, so an assertion here that inspects a configuration +//! or a constant instead of an emitted frame is asserting the wrong thing. + +mod support; + +use io_cli::acp::{self, AgentCapabilities, ErrorCode, Incoming, Outgoing, GATED, SERVED}; +use io_cli::acp_map::{self, Update}; +use io_cli::approval::Answer; +use io_harness::{EventKind, RunEvent}; +use serde_json::{json, Value}; + +/// The frame a well-formed request arrives as. +fn request(id: Value, method: &str, params: Value) -> String { + json!({ "jsonrpc": "2.0", "id": id, "method": method, "params": params }).to_string() +} + +/// **F1 — a frame is one line, and nothing this module emits contains an +/// interior newline.** +/// +/// ACP's transport page is explicit: *"Messages are delimited by newlines +/// (`\n`), and MUST NOT contain embedded newlines."* So the obligation runs both +/// ways, and the emitting half is the half this crate can break. A pretty-printer +/// in place of `serde_json::to_string` would split one message into as many +/// frames as it has lines and desynchronise the session permanently — every +/// subsequent frame parsed against the wrong boundary. +/// +/// **The content is hostile on purpose.** A message that merely round-trips +/// proves nothing: the interesting case is a payload that itself contains +/// newlines, tabs and quotes, because that is what a real agent's output and a +/// real error string look like, and it is where an escaping bug shows up. +/// +/// Sabotage: `serde_json::to_string_pretty` in `acp::encode`. It fails here and +/// nowhere else in the suite. +#[test] +fn f1_no_emitted_frame_carries_an_interior_newline() { + let hostile = "first line\nsecond line\r\nthird\ttab \"quoted\" \\ backslash"; + + let frames = [ + Outgoing::Result { + id: json!(1), + result: json!({ "text": hostile, "nested": { "more": hostile } }), + }, + Outgoing::Error { + id: json!("abc"), + code: ErrorCode::Internal, + message: hostile.to_string(), + }, + Outgoing::Notification { + method: "session/update".into(), + params: json!({ "chunk": hostile, "list": [hostile, hostile] }), + }, + Outgoing::unattributed(ErrorCode::Parse, hostile), + ]; + + for frame in &frames { + let line = acp::encode(frame); + assert!( + !line.contains('\n'), + "an emitted frame carries a newline, so a client reading by line would \ + read it as two messages and every frame after it against the wrong \ + boundary: {line:?}", + ); + assert!( + !line.contains('\r'), + "an emitted frame carries a carriage return: {line:?}", + ); + // And it must still be the message it was. A frame made single-line by + // dropping the content would pass the assertion above and be useless. + let parsed: Value = + serde_json::from_str(&line).expect("an emitted frame is a JSON document"); + assert_eq!(parsed["jsonrpc"], "2.0", "every frame names the version"); + } + + // The escaping is real rather than incidental: the hostile text survives the + // round trip with its newlines intact, which is what distinguishes escaping + // from stripping. + let line = acp::encode(&frames[0]); + let parsed: Value = serde_json::from_str(&line).expect("a JSON document"); + assert_eq!( + parsed["result"]["text"], hostile, + "the payload was altered to make it fit on one line, rather than escaped", + ); +} + +/// **F2 — the declared capabilities and the served methods agree, in both +/// directions.** +/// +/// One direction is the obvious one: a capability declared supported whose method +/// answers "method not found" is a promise the adapter cannot keep, and a client +/// discovers it mid-session. +/// +/// The other direction is the one this repository has actually shipped. 0.30.0 +/// documented `io skill` while the argv door had no variant for it, and 1,609 +/// tests passed over the gap because every one of them entered a layer below the +/// door. A method served but never declared is the same defect wearing the +/// protocol's clothes: the specification says *"Clients and Agents MUST treat all +/// capabilities omitted in the `initialize` request as UNSUPPORTED"*, so a client +/// will never call it and the code is dead. +/// +/// Asserted as one equality per gated method, so whichever side moves, the +/// failure names the method. +/// +/// Sabotage: flip `load_session` to `true` while `session/load` stays out of +/// `SERVED`. Or add `"session/load"` to `SERVED` with the capability left +/// `false`. Either fails here alone. +#[test] +fn f2_every_gated_method_is_served_exactly_when_its_capability_is_declared() { + let capabilities = AgentCapabilities::default(); + + // The control: a table with no rows would make the loop below vacuous, and a + // capability lookup that answered `false` for everything would make half of + // each equality trivially true. + assert!( + !GATED.is_empty(), + "no gated method is listed, so the agreement below asserts nothing", + ); + + for (method, capability) in GATED { + assert_eq!( + acp::serves(method), + capabilities.declares(capability), + "`{method}` is served: {}, but capability `{capability}` is declared: {}. \ + A capability without its method is a promise the adapter breaks at \ + runtime; a method without its capability is code no client will ever \ + call, because the specification says an omitted capability is \ + unsupported.", + acp::serves(method), + capabilities.declares(capability), + ); + } +} + +/// **F2's other half — `initialize` answers the version this module implements.** +/// +/// The version is an integer major and the client refuses a mismatch, so an +/// adapter that answered a version it had not implemented would be worse than one +/// that declined. v2 exists as a draft and is deliberately not spoken. +#[test] +fn f2_initialize_answers_protocol_version_one_and_needs_no_authentication() { + let result = acp::initialize_result("0.36.0"); + + assert_eq!( + result["protocolVersion"], 1, + "this adapter implements ACP v1 and must say so", + ); + assert_eq!(result["agentInfo"]["name"], "io"); + assert_eq!(result["agentInfo"]["version"], "0.36.0"); + + // An empty list is legal and is the honest answer: `io setup` writes + // io-harness's configuration file, it is not an ACP authentication method, and + // credentials stay where they are. + assert_eq!( + result["authMethods"], + json!([]), + "no authentication step is required of the client", + ); + + // Everything omitted is unsupported by the specification's own rule, so the + // three prompt capabilities being present and false is a statement rather + // than an oversight. + let prompt = &result["agentCapabilities"]["promptCapabilities"]; + for name in ["image", "audio", "embeddedContext"] { + assert_eq!( + prompt[name], + json!(false), + "`{name}` is not carried into a run yet and must not be promised", + ); + } + assert_eq!(result["agentCapabilities"]["loadSession"], json!(false)); +} + +/// **A malformed frame is answered, never fatal.** +/// +/// A client that sends one bad frame must not lose its conversation. The caller +/// distinguishes the two cases by the `Result`, and `Err` here is a frame to +/// write back rather than a reason to stop reading — so every arm below produces +/// something sendable. +#[test] +fn a_frame_that_cannot_be_understood_is_answered_rather_than_fatal() { + let cases = [ + ("", ErrorCode::Parse, "an empty line"), + (" ", ErrorCode::Parse, "whitespace only"), + ("not json at all", ErrorCode::Parse, "not JSON"), + ("{\"jsonrpc\":\"2.0\"", ErrorCode::Parse, "truncated"), + ( + r#"{"jsonrpc":"1.0","id":1,"method":"initialize"}"#, + ErrorCode::InvalidRequest, + "the wrong JSON-RPC revision", + ), + ( + r#"{"id":1,"method":"initialize"}"#, + ErrorCode::InvalidRequest, + "no jsonrpc member", + ), + ( + // Neither a method nor an id: not a request, not a notification, and + // not a response either. A frame carrying an id and no method *is* a + // response and is handled in `a_response_is_correlated_not_refused`. + r#"{"jsonrpc":"2.0"}"#, + ErrorCode::InvalidRequest, + "a frame that is none of the three shapes", + ), + ]; + + for (line, expected, why) in cases { + let answer = acp::decode(line).expect_err(why); + match answer { + Outgoing::Error { code, .. } => assert_eq!( + code, expected, + "{why} was answered with the wrong error code", + ), + other => panic!("{why} produced {other:?} rather than an error"), + } + // The answer has to be sendable, which is the property that makes this a + // recoverable condition rather than a described one. + let encoded = acp::encode(&answer); + assert!(!encoded.contains('\n'), "the error frame is one line"); + } +} + +/// **A parse failure answers a null id, because there is no id to answer to.** +/// +/// JSON-RPC 2.0 requires it, and the temptation is to invent one or reuse the +/// last — either of which correlates an error to a request that did not cause it. +#[test] +fn a_frame_that_did_not_parse_is_answered_with_a_null_id() { + let answer = acp::decode("{ this is not json }").expect_err("not JSON"); + let encoded = acp::encode(&answer); + let parsed: Value = serde_json::from_str(&encoded).expect("a JSON document"); + assert_eq!( + parsed["id"], + Value::Null, + "a frame whose id could not be read is answered with null, never a guess", + ); +} + +/// **A request and a notification are different things, and answering a +/// notification is a protocol violation.** +/// +/// The distinction is the presence of an id and nothing else. It matters at one +/// place in this adapter and it is the place that would be easy to get wrong: +/// `session/cancel` is a notification, so a dispatcher that answered everything +/// would send an unsolicited response mid-turn. +#[test] +fn an_id_is_what_separates_a_request_from_a_notification() { + let req = acp::decode(&request(json!(7), "session/prompt", json!({ "x": 1 }))) + .expect("a well-formed request"); + match req { + Incoming::Request { id, method, params } => { + assert_eq!(id, json!(7)); + assert_eq!(method, "session/prompt"); + assert_eq!(params, json!({ "x": 1 })); + } + other => panic!("a frame with an id is a request, got {other:?}"), + } + + let note = + acp::decode(r#"{"jsonrpc":"2.0","method":"session/cancel","params":{"sessionId":"s"}}"#) + .expect("a well-formed notification"); + match note { + Incoming::Notification { method, .. } => assert_eq!(method, "session/cancel"), + other => panic!("a frame with no id is a notification, got {other:?}"), + } + + // A string id is as valid as a number and must survive unchanged — a client + // that uses UUIDs gets its own id back, not a number this adapter coined. + let string_id = acp::decode(&request(json!("req-9f3a"), "initialize", json!({}))) + .expect("a string id is valid"); + match string_id { + Incoming::Request { id, .. } => assert_eq!(id, json!("req-9f3a")), + other => panic!("expected a request, got {other:?}"), + } +} + +/// **Absent `params` is an empty object, not an error.** +/// +/// The specification makes the member optional and several methods take none, so +/// refusing a frame without it would reject well-formed traffic. +#[test] +fn a_request_carrying_no_params_is_well_formed() { + let decoded = acp::decode(r#"{"jsonrpc":"2.0","id":1,"method":"initialize"}"#) + .expect("params is an optional member"); + assert_eq!(decoded.params(), &json!({})); + assert_eq!(decoded.method(), "initialize"); +} + +/// **F4 — every kind the locked harness declares reaches a mapping or a listed +/// no-op, and the table names nothing the harness has stopped emitting.** +/// +/// This is the assertion the release stands on. `EventKind` is +/// `#[non_exhaustive]`, so `acp_map::translate`'s wildcard arm is mandatory and +/// the compiler will never complain about a variant that falls through it. The +/// failure that produces is the worst one this release can ship: an editor that +/// silently never shows that the agent edited a file, behind a fully green suite. +/// io-harness 0.73.0 did a smaller version of it to `read_skill` one release ago +/// and the existing test stayed green over garbage output. +/// +/// So the set is read from the **locked harness's own source**, the same way +/// `tests/triage.rs` reads it, and compared by name in both directions. A count +/// alone would be satisfied by a table that dropped one kind and invented +/// another. +/// +/// Sabotage: delete any row from `MAPPING`. This fails naming that kind, rather +/// than a number going down. +#[test] +fn f4_the_mapping_is_total_over_the_locked_harnesss_event_kinds() { + let declared = support::harness_event_kinds(); + let mapped: Vec<&str> = acp_map::MAPPING.iter().map(|(name, ..)| *name).collect(); + + // The control. A source reader that returned nothing would satisfy the + // untranslated check below while asserting nothing at all. + assert!( + declared.len() > 40, + "the locked harness reader found {} kinds, which is not a plausible enum; \ + every assertion below is vacuous until this is fixed", + declared.len(), + ); + + let untranslated: Vec<&String> = declared + .iter() + .filter(|name| !mapped.contains(&name.as_str())) + .collect(); + assert!( + untranslated.is_empty(), + "io-harness emits kinds this adapter has no answer for: {untranslated:?}. \ + Decide for each whether it becomes a `session/update` or is a no-op, and \ + add it to `acp_map::MAPPING` with the reason. A kind left out reaches an \ + ACP client as nothing at all, and no client can report what it was never \ + sent.", + ); + + let gone: Vec<&&str> = mapped + .iter() + .filter(|name| !declared.contains(&(**name).to_string())) + .collect(); + assert!( + gone.is_empty(), + "these names are no longer io-harness event kinds: {gone:?}", + ); + + // Every no-op carries its reason. A row with an empty one is a silence + // nobody chose, which is the state this table exists to make impossible. + for (name, update, why) in acp_map::MAPPING { + assert!( + !why.trim().is_empty(), + "{name} maps to {update:?} and says nothing about why", + ); + } +} + +/// **F4's second half — the table and the translator agree.** +/// +/// A row can say `MessageChunk` while `translate` sends nothing for it, and the +/// table would still be total. That is the same defect the table exists to +/// prevent, one level down: documentation density is not coverage, which 0.32.0's +/// sabotage pass established here by finding two criteria whose only evidence was +/// prose. +/// +/// So each row is driven through the real translator with a constructed event, +/// and the answer's presence and its `sessionUpdate` string are checked against +/// what the row claims. +/// +/// Sabotage: change one row's `Update` without changing `translate`. This fails +/// naming the kind and both spellings. +#[test] +fn f4_every_row_claims_what_the_translator_actually_sends() { + let cases: &[(EventKind, &str, &str)] = &[ + ( + EventKind::Token { + text: "hello".into(), + }, + "token", + "agent_message_chunk", + ), + ( + EventKind::Reasoning { + text: "thinking".into(), + tokens: 3, + }, + "reasoning", + "agent_thought_chunk", + ), + ( + EventKind::ToolCall { + name: "read_file".into(), + target: "src/main.rs".into(), + }, + "tool_call", + "tool_call", + ), + ( + EventKind::Refused { + act: "write".into(), + target: "/etc/hosts".into(), + rule: None, + layer: None, + }, + "refused", + "tool_call_update", + ), + ]; + + for (kind, name, expected) in cases { + let claimed = acp_map::update_for(name).unwrap_or_else(|| panic!("{name} is in MAPPING")); + assert_ne!( + claimed, + Update::None, + "{name} is listed as a no-op but this case expects it on the wire", + ); + + let sent = acp_map::translate(&RunEvent::new(1, 2, kind.clone())) + .unwrap_or_else(|| panic!("{name} claims {claimed:?} and the translator sent nothing")); + assert_eq!( + sent["sessionUpdate"], *expected, + "{name} was sent as {} where the table claims {claimed:?}", + sent["sessionUpdate"], + ); + } + + // And the other direction, on a row that claims nothing: a kind the table + // calls a no-op must actually send nothing. `finished` is the one worth + // asserting — sending it *and* the prompt result would end the turn twice. + assert_eq!(acp_map::update_for("finished"), Some(Update::None)); + let finished = RunEvent::new( + 1, + 9, + EventKind::Finished { + outcome: "success".into(), + steps: 3, + tokens: 100, + }, + ); + assert!( + acp_map::translate(&finished).is_none(), + "`finished` reached the client as a notification as well as the prompt's \ + own result, which ends the turn twice", + ); +} + +/// **F3 — every `RunOutcome` the locked harness declares maps to one of ACP's +/// five stop reasons.** +/// +/// Read out of the locked source rather than listed here, so a harness that gains +/// an outcome fails this instead of quietly taking the wildcard. That is the +/// discipline `tests/exec.rs` already uses for the exit-code table. +/// +/// The five are fixed by the specification, so an answer outside them is a frame +/// a client will reject. +/// +/// Sabotage: return `"done"` for any arm. Fails here naming the outcome. +#[test] +fn f3_every_outcome_maps_to_one_of_the_five_acp_stop_reasons() { + const REASONS: &[&str] = &[ + "end_turn", + "max_tokens", + "max_turn_requests", + "refusal", + "cancelled", + ]; + + let outcomes = [ + io_harness::RunOutcome::Success { steps: 1 }, + io_harness::RunOutcome::StepCapReached { steps: 1 }, + io_harness::RunOutcome::VerificationFailed { steps: 1 }, + io_harness::RunOutcome::AwaitingRecovery { + attempt_id: 1, + steps: 1, + }, + io_harness::RunOutcome::TimeBudgetExceeded { steps: 1 }, + io_harness::RunOutcome::CostBudgetExceeded { steps: 1 }, + io_harness::RunOutcome::Denied { steps: 1 }, + io_harness::RunOutcome::AwaitingApproval { + request_id: 1, + steps: 1, + }, + io_harness::RunOutcome::AwaitingAnswer { + question_id: 1, + steps: 1, + }, + io_harness::RunOutcome::AwaitingPlan { + plan_id: 1, + steps: 1, + }, + io_harness::RunOutcome::PlanRejected { steps: 1 }, + io_harness::RunOutcome::Stalled { steps: 1 }, + io_harness::RunOutcome::Escalated { + steps: 1, + retryable: true, + }, + io_harness::RunOutcome::BudgetCeilingReached { steps: 1 }, + io_harness::RunOutcome::Refused { steps: 1 }, + io_harness::RunOutcome::Cancelled { steps: 1 }, + io_harness::RunOutcome::Finished { steps: 1 }, + ]; + + // The list above is hand-built because `RunOutcome` is an enum with no + // iterator, so it can go stale. This is what stops it: the locked source's own + // variant count must match, and a harness that adds one fails here by number + // with the file to read named in the message. + assert_eq!( + outcomes.len(), + support::harness_run_outcomes().len(), + "the locked io-harness declares {} run outcomes and this test constructs \ + {}. Read `RunOutcome` in the pinned source and map the new one — the \ + wildcard in `acp_map::stop_reason` will otherwise answer `end_turn` for \ + an outcome that is not one.", + support::harness_run_outcomes().len(), + outcomes.len(), + ); + + for outcome in &outcomes { + let reason = acp_map::stop_reason(outcome); + assert!( + REASONS.contains(&reason), + "{outcome:?} maps to {reason:?}, which is not one of ACP's five stop \ + reasons; a client rejects the frame", + ); + } + + // The distinctions that change what a client does are asserted by name rather + // than left to the loop above, which any single constant would satisfy. + assert_eq!( + acp_map::stop_reason(&io_harness::RunOutcome::Cancelled { steps: 1 }), + "cancelled", + ); + assert_eq!( + acp_map::stop_reason(&io_harness::RunOutcome::Denied { steps: 1 }), + "refusal", + ); + assert_eq!( + acp_map::stop_reason(&io_harness::RunOutcome::Success { steps: 1 }), + "end_turn", + ); + // A pause is not a refusal. The run stopped with work outstanding and has + // refused nothing; telling a client otherwise is a claim about the agent's + // willingness that is not true. + assert_eq!( + acp_map::stop_reason(&io_harness::RunOutcome::AwaitingAnswer { + question_id: 1, + steps: 1, + }), + "end_turn", + "a parked run was reported to the client as a refusal", + ); +} + +/// **A tool name becomes an ACP `ToolKind`, and an unknown one becomes `other` +/// rather than a guess.** +/// +/// The nine kinds are the specification's. `other` is a legal answer: a bundle's +/// own tool and an MCP server's tool are names this crate has never seen, and +/// inferring `edit` from a substring would tell a client a read was a write. +#[test] +fn a_tool_name_maps_to_one_of_the_nine_acp_tool_kinds() { + const KINDS: &[&str] = &[ + "read", "edit", "delete", "move", "search", "execute", "think", "fetch", "other", + ]; + + for name in [ + "read_file", + "write_file", + "grep", + "exec", + "browser_navigate", + "some_bundle__tool", + "", + ] { + assert!( + KINDS.contains(&acp_map::tool_kind(name)), + "`{name}` mapped to `{}`, which is not an ACP ToolKind", + acp_map::tool_kind(name), + ); + } + + // **The real gate: every tool io-harness declares is classified + // deliberately.** Asserted on `classified` and never on `tool_kind != other`, + // because `other` is the correct answer for several of them — asking the + // operator a question is not a read, an edit or an execution in ACP's + // vocabulary. The two states that must stay distinguishable are "listed as + // other" and "nobody looked at it", and only a table can tell them apart. + // Read from the locked source, so a pin that adds a tool fails here rather + // than waiting for a reviewer to notice a call drawn unclassified. + let harness_tools = support::harness_tool_names(); + assert!( + harness_tools.len() > 20, + "the locked harness reader found {} tool names, which is not plausible; \ + the assertion below is vacuous until this is fixed", + harness_tools.len(), + ); + let unclassified: Vec<&String> = harness_tools + .iter() + .filter(|name| !acp_map::classified(name)) + .collect(); + assert!( + unclassified.is_empty(), + "io-harness declares tools this adapter has never looked at: \ + {unclassified:?}. Each needs a row in `acp_map::TOOL_KINDS` — writing \ + `other` there is a decision and is fine; leaving the name out is a gap, \ + and the symptom is an editor drawing a write as an unclassified call.", + ); + + // And the table names nothing the harness has stopped declaring, so a tool + // removed upstream does not sit here forever as a row nobody can reach. + let stale: Vec<&str> = acp_map::TOOL_KINDS + .iter() + .map(|(name, _)| *name) + .filter(|name| !harness_tools.iter().any(|tool| tool == name)) + .collect(); + assert!( + stale.is_empty(), + "these are no longer io-harness tools: {stale:?}", + ); + + assert_eq!(acp_map::tool_kind("read_file"), "read"); + assert_eq!(acp_map::tool_kind("edit_file"), "edit"); + assert_eq!(acp_map::tool_kind("grep"), "search"); + assert_eq!(acp_map::tool_kind("shell"), "execute"); + assert_eq!( + acp_map::tool_kind("github__create_issue"), + "other", + "a server's tool is a name this crate has never seen and must not be guessed at", + ); + assert_eq!( + acp_map::tool_kind("git_status"), + "read", + "a git reader is a read; 0.75.0 made it speculable for exactly that reason", + ); + assert_eq!( + acp_map::tool_kind("git_commit"), + "execute", + "a commit writes to the repository and is not a read", + ); +} + +/// **A frame with an id and no method is an answer to something this adapter +/// asked, and is correlated rather than refused.** +/// +/// `session/request_permission` is the only request this adapter makes, so its +/// answer arrives this way. A transport that refused it would make the permission +/// round trip impossible while every other test still passed. +/// +/// The `error` case is asserted beside the `result` case because they must stay +/// distinguishable: a client that refused the request has not decided anything, +/// and reading a protocol error as a permission decision is the one mistake this +/// seam cannot be allowed to make. +#[test] +fn a_response_is_correlated_not_refused() { + let ok = acp::decode(r#"{"jsonrpc":"2.0","id":4,"result":{"outcome":"cancelled"}}"#) + .expect("a response is a well-formed frame"); + match ok { + Incoming::Response { id, outcome } => { + assert_eq!(id, json!(4), "the id is what correlates it to the request"); + assert_eq!( + outcome.expect("a result, not an error")["outcome"], + json!("cancelled"), + ); + } + other => panic!("expected a response, got {other:?}"), + } + + let refused = acp::decode( + r#"{"jsonrpc":"2.0","id":5,"error":{"code":-32601,"message":"no such method"}}"#, + ) + .expect("an error response is still a well-formed frame"); + match refused { + Incoming::Response { outcome, .. } => assert_eq!( + outcome.expect_err("an error object is not a result"), + "no such method", + "the client's own message is carried, not a substitute", + ), + other => panic!("expected a response, got {other:?}"), + } +} + +/// **F6 — the options offered are the ones io-harness can actually carry out, +/// and every answer maps to a decision.** +/// +/// ACP names four `PermissionOption` kinds. Three are offered, and the missing +/// fourth is the assertion that matters: `io_harness::Decision::Deny` carries a +/// reason and nothing else, while `Approve` carries `remember: Vec` — so +/// `allow_always` is expressible and `reject_always` is not. Offering it anyway +/// and behaving as `reject_once` would be a surface accepting an instruction it +/// cannot carry out and reporting success, which is the defect shape this product +/// has now found in itself several times. +/// +/// Sabotage: add a `reject_always` row to `PERMISSION_OPTIONS`. Fails here. +#[test] +fn f6_three_permission_options_are_offered_and_reject_always_is_not() { + let kinds: Vec<&str> = acp::PERMISSION_OPTIONS + .iter() + .map(|(_, _, kind)| *kind) + .collect(); + + assert_eq!(kinds, vec!["allow_once", "allow_always", "reject_once"]); + assert!( + !kinds.contains(&"reject_always"), + "`reject_always` is offered but io-harness cannot remember a refusal: \ + `Decision::Deny` carries a reason and no rules, so a later matching \ + action would ask again and the operator was told otherwise", + ); + + // Every offered id maps to an answer, and the three are distinct — an option + // list where two ids meant the same thing would be a choice that is not one. + let answers: Vec = acp::PERMISSION_OPTIONS + .iter() + .map(|(id, ..)| acp::answer_for(id).unwrap_or_else(|| panic!("{id} maps to an answer"))) + .collect(); + assert_eq!(answers, vec![Answer::Once, Answer::Session, Answer::Deny]); + + // And the ids in the emitted frame are the ids the mapping accepts. They are + // written twice — in the table and in the match — and a disagreement between + // them is a client answer that is silently a denial. + let params = acp::permission_params("s-1", "write", "/tmp/x", "1-2"); + for option in params["options"].as_array().expect("an options array") { + let id = option["optionId"].as_str().expect("an optionId"); + assert!( + acp::answer_for(id).is_some(), + "the frame offers `{id}`, which `answer_for` does not recognise, so \ + choosing it would be read as a denial", + ); + } + assert_eq!(params["sessionId"], "s-1"); + assert_eq!(params["toolCall"]["title"], "write /tmp/x"); +} + +/// **F6's other half — every answer that is not an offered selection is a +/// denial.** +/// +/// There is exactly one safe direction to be wrong in on a permission surface. +/// A cancellation, a missing outcome, an option id this adapter never offered, +/// and a shape the specification does not name all mean the same thing here, and +/// each is asserted rather than left to a wildcard nobody checked. +/// +/// Sabotage: make the unknown-option arm `Answer::Once`. Fails on three of these. +#[test] +fn f6_anything_that_is_not_a_selection_is_a_denial() { + let cases = [ + (json!({ "outcome": "cancelled" }), "the client cancelled"), + (json!({}), "no outcome at all"), + (json!({ "outcome": "selected" }), "selected, with no option"), + ( + json!({ "outcome": "selected", "optionId": "allow-everything-forever" }), + "an option that was never offered", + ), + ( + json!({ "outcome": "something-new" }), + "an outcome this adapter has never seen", + ), + (json!("not even an object"), "not an object"), + ]; + + for (result, why) in cases { + assert_eq!( + acp::permission_answer(&result), + Answer::Deny, + "{why} was not read as a denial", + ); + } + + // The control: a real selection is not a denial, or every assertion above is + // satisfied by a function that always denies. + assert_eq!( + acp::permission_answer(&json!({ "outcome": "selected", "optionId": "allow-once" })), + Answer::Once, + ); + assert_eq!( + acp::permission_answer(&json!({ "outcome": "selected", "optionId": "allow-session" })), + Answer::Session, + ); +} + +/// **F5 — a cancel stops the run, and it stops it before the next notification is +/// written.** +/// +/// Asserted on the flag reaching `Flow::Cancel`, never on a timing: +/// `tests/timing.rs` forbids a clock in any test and is right — a wall-clock +/// assertion is flaky and says nothing about *why* a path stopped. +/// +/// The ordering half is the part worth writing. Reading the flag after +/// translating would send one more `session/update` after the client asked to +/// stop, and a client that cancelled and then received another chunk would +/// reasonably conclude the cancel was ignored. +/// +/// Sabotage: move the flag check below the translate. The `Flow` assertions still +/// pass; the "nothing further was written" one fails. +#[test] +fn f5_a_cancel_reaches_the_observer_and_silences_it_at_once() { + use io_harness::{Flow, Observer}; + use std::sync::atomic::Ordering; + + let (updates, mut drain) = tokio::sync::mpsc::unbounded_channel(); + let (reporter, cancelled) = acp::Reporter::new("s-7", updates); + + // Before the cancel: a token is a message chunk and the run continues. + let token = RunEvent::new( + 1, + 1, + EventKind::Token { + text: "hello".into(), + }, + ); + assert_eq!(reporter.event(&token), Flow::Continue); + let first = drain.try_recv().expect("a chunk was written"); + match first { + Outgoing::Notification { method, params } => { + assert_eq!(method, "session/update"); + assert_eq!( + params["sessionId"], "s-7", + "the session is named on every update" + ); + assert_eq!(params["update"]["sessionUpdate"], "agent_message_chunk"); + } + other => panic!("expected a notification, got {other:?}"), + } + + // The client cancels. + cancelled.store(true, Ordering::Relaxed); + + assert_eq!( + reporter.event(&token), + Flow::Cancel, + "the run was told to continue after the client cancelled", + ); + assert!( + drain.try_recv().is_err(), + "a `session/update` was written after the cancel; the client asked to stop \ + and was sent more of the thing it stopped", + ); + + // And it stays cancelled. A flag that reset itself would let the run resume + // on the next event, which is worse than never having stopped. + assert_eq!(reporter.event(&token), Flow::Cancel); +} + +/// **A kind the table calls a no-op writes nothing at all.** +/// +/// The reporter is where `MAPPING`'s no-ops become real. A `Dialed` event that +/// reached the client as an empty update would be a frame a client must parse and +/// can do nothing with. +#[test] +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); + + assert_eq!(acp_map::update_for("dialed"), Some(Update::None)); + reporter.event(&RunEvent::new( + 1, + 0, + EventKind::Dialed { + host: "openrouter.ai".into(), + port: 443, + allowed: true, + }, + )); + assert!( + drain.try_recv().is_err(), + "a kind the mapping calls a no-op still put a frame on the wire", + ); +} + +/// Run the loop against a real pipe and hand back every line it wrote. +/// +/// The writer moves onto its own task inside `serve_with`, so it must be `Send + +/// '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. +async fn drive( + input: &str, + handler: &mut impl acp::Handler, + outbound: tokio::sync::mpsc::UnboundedReceiver, +) -> Vec { + use tokio::io::AsyncReadExt as _; + + let (theirs, ours) = tokio::io::duplex(64 * 1024); + acp::serve_with(input.as_bytes(), ours, handler, outbound) + .await + .expect("the loop ends on end-of-input, not on an error"); + + let mut written = String::new(); + let mut theirs = theirs; + theirs + .read_to_string(&mut written) + .await + .expect("the written frames are UTF-8"); + + written + .lines() + .map(|line| serde_json::from_str(line).expect("every written line is one JSON document")) + .collect() +} + +/// **A `session/update` sent from inside a running turn is written while the turn +/// is still running.** +/// +/// **This is the release's headline property and the first version of the loop +/// did not have it.** `handler.handle(...).await` for a `session/prompt` *is* the +/// whole turn, and it ran inside a `tokio::select!` arm's body — so the outbound +/// channel was not polled again until the turn returned, every notification +/// queued behind the result, and the client was sent the entire conversation +/// *after* being told the turn had ended. `docs/guide/editors.md` promises "the +/// answer, streamed as the provider returns it". +/// +/// The suite could not see it: no test drove `serve_with` with a live channel, +/// and the loop test's updates came from the handler's **return value**, which is +/// a path the real handler never uses. Found by the adversarial review. +/// +/// So this asserts the wire ORDER, from the channel, across a handler that sends +/// while it is working. Sabotage: move the writer back into the read loop. The +/// result then precedes its own updates and the ordering assertion fails. +#[tokio::test] +async fn a_notification_sent_during_a_turn_is_written_before_that_turns_result() { + struct Working { + updates: tokio::sync::mpsc::UnboundedSender, + } + + impl acp::Handler for Working { + async fn handle(&mut self, incoming: Incoming) -> Vec { + let Incoming::Request { id, .. } = incoming else { + return Vec::new(); + }; + // Exactly what a turn does: it reports progress on the channel while + // it works, and answers only at the end. + for chunk in ["one", "two", "three"] { + let _ = self.updates.send(Outgoing::Notification { + method: "session/update".into(), + params: json!({ "chunk": chunk }), + }); + // Yield so the writer task is genuinely given the chance to run. + // Without this the test could pass on buffering rather than on + // the property. + tokio::task::yield_now().await; + } + vec![Outgoing::Result { + id, + result: json!({ "stopReason": "end_turn" }), + }] + } + } + + let (updates, outbound) = tokio::sync::mpsc::unbounded_channel(); + let mut handler = Working { + updates: updates.clone(), + }; + let input = request(json!(1), "session/prompt", json!({})) + "\n"; + drop(updates); + + let frames = drive(&input, &mut handler, outbound).await; + + assert_eq!(frames.len(), 4, "three updates and one result: {frames:?}"); + for (at, chunk) in ["one", "two", "three"].iter().enumerate() { + assert_eq!( + frames[at]["params"]["chunk"], *chunk, + "the updates a turn sent while working did not reach the client in \ + order, before its result: {frames:?}", + ); + } + assert_eq!( + frames[3]["result"]["stopReason"], "end_turn", + "the turn's result must come last; a client that has been told the turn \ + ended does not expect more of it: {frames:?}", + ); +} + +/// A handler that records what it was asked and answers from a script. +struct Recorder { + seen: Vec, + answers: Vec>, +} + +impl acp::Handler for Recorder { + async fn handle(&mut self, incoming: Incoming) -> Vec { + self.seen.push(incoming.method().to_string()); + if self.answers.is_empty() { + return Vec::new(); + } + self.answers.remove(0) + } +} + +/// **The loop reads every frame in the stream, answers each, and ends on +/// end-of-input rather than on a bad frame.** +/// +/// This drives the real `serve` against in-memory buffers. A transport reachable +/// only from the binary is a transport no test has ever run — 0.32.0 spent a +/// whole task undoing exactly that shape, and the generic streams here exist so +/// this release does not repeat it. +/// +/// The stream deliberately mixes good and bad: a valid request, then a frame that +/// is not JSON, then a notification, then another valid request. A loop that +/// exited on the bad frame would answer two of the four and the session would be +/// silently lost. +#[tokio::test] +async fn the_serve_loop_answers_a_bad_frame_and_keeps_reading() { + let input = [ + request(json!(1), "initialize", json!({})), + "{ not json }".to_string(), + r#"{"jsonrpc":"2.0","method":"session/cancel","params":{}}"#.to_string(), + request(json!(2), "session/prompt", json!({})), + ] + .join("\n") + + "\n"; + + let mut handler = Recorder { + seen: Vec::new(), + answers: vec![ + vec![Outgoing::Result { + id: json!(1), + result: json!({ "ok": true }), + }], + // A notification is answered with nothing. Answering one is a + // protocol violation, and the loop must not add a frame of its own. + Vec::new(), + vec![ + Outgoing::Notification { + method: "session/update".into(), + params: json!({ "chunk": "hello" }), + }, + Outgoing::Result { + id: json!(2), + result: json!({ "stopReason": "end_turn" }), + }, + ], + ], + }; + + let (_updates, outbound) = tokio::sync::mpsc::unbounded_channel(); + let parsed = drive(&input, &mut handler, outbound).await; + + // The handler saw the three decodable frames, in order. The malformed one + // never reached it — it was answered by the transport. + assert_eq!( + handler.seen, + vec!["initialize", "session/cancel", "session/prompt"], + "the loop stopped reading at the malformed frame, or passed it to the handler", + ); + + assert_eq!( + parsed.len(), + 4, + "expected the initialize result, the parse error, and the prompt's \ + notification and result; got: {parsed:?}", + ); + + assert_eq!(parsed[0]["id"], json!(1)); + assert_eq!(parsed[0]["result"]["ok"], json!(true)); + + // The parse error is second, sits between the two answers, and carries a null + // id because the frame it refers to had none to read. + assert_eq!(parsed[1]["error"]["code"], json!(-32700)); + assert_eq!(parsed[1]["id"], Value::Null); + + assert_eq!(parsed[2]["method"], json!("session/update")); + assert!( + parsed[2].get("id").is_none(), + "a notification carries no id: {}", + parsed[2], + ); + assert_eq!(parsed[3]["id"], json!(2)); + assert_eq!(parsed[3]["result"]["stopReason"], json!("end_turn")); +} + +/// **A stream that ends without a trailing newline still delivers its last +/// frame.** +/// +/// `read_line` returns the remainder with no `\n` on it, and a loop that treated +/// a missing delimiter as a malformed frame would drop the last message of every +/// client that does not send one. +#[tokio::test] +async fn a_final_frame_with_no_trailing_newline_is_still_read() { + let input = request(json!(1), "initialize", json!({})); + let mut handler = Recorder { + seen: Vec::new(), + answers: vec![vec![Outgoing::Result { + id: json!(1), + result: json!({}), + }]], + }; + + let (_updates, outbound) = tokio::sync::mpsc::unbounded_channel(); + let frames = drive(&input, &mut handler, outbound).await; + + assert_eq!(handler.seen, vec!["initialize"]); + assert_eq!( + frames.len(), + 1, + "the last frame was dropped because the client sent no trailing newline", + ); + assert_eq!(frames[0]["id"], json!(1)); +} + +/// **The served list is the dispatch table, and it is not empty.** +/// +/// A control on every other assertion that walks `SERVED`. +#[test] +fn the_served_methods_are_the_ones_this_release_implements() { + assert!(acp::serves("initialize")); + assert!(acp::serves("session/new")); + assert!(acp::serves("session/prompt")); + assert!(acp::serves("session/cancel")); + + assert!( + !acp::serves("session/load"), + "`session/load` is excluded from 0.36.0 and must not answer", + ); + assert!( + !acp::serves("fs/read_text_file"), + "a client method, not ours" + ); + assert!(!acp::serves("nonsense/method")); + + assert_eq!( + SERVED.len(), + 4, + "the dispatch table changed without this test being updated", + ); +} diff --git a/tests/adapt.rs b/tests/adapt.rs index fb00815..070dff8 100644 --- a/tests/adapt.rs +++ b/tests/adapt.rs @@ -493,7 +493,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.74.0/src/plugin.rs:1097`): a manifest contributes a +/// included (`io-harness-0.76.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] diff --git a/tests/contract.rs b/tests/contract.rs index 75afa52..68f65df 100644 --- a/tests/contract.rs +++ b/tests/contract.rs @@ -1725,7 +1725,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.66.0/src/config.rs` — so a `~` an +/// no tilde branch anywhere in `io-harness-0.76.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 20d79e2..be0eea5 100644 --- a/tests/cost.rs +++ b/tests/cost.rs @@ -73,12 +73,27 @@ fn split() -> Usage { completion_tokens: 2_000, total_tokens: 12_000, cache_read_tokens: 4_000, - cache_write_tokens: 1_000, + cache_write_tokens: Some(1_000), reasoning_tokens: 500, server_tool_requests: 3, } } +/// The split fixture's cache-write count, unwrapped. +/// +/// io-harness 0.76.0 made `cache_write_tokens` an `Option`, and `split()` is +/// deliberately a provider that *did* report one — so every arithmetic assertion +/// below is about a known number and reads the way it did before the pin. +/// +/// **The unknown case does not share this helper and must not.** A test that +/// unwrapped its way past `None` would assert nothing about the state this +/// release added; the three-state arms below construct their own usage. +fn written(usage: &Usage) -> u64 { + usage + .cache_write_tokens + .expect("the split fixture reports a cache-write count") +} + fn call(model: Option<&str>, usage: Option) -> ProviderCall { ProviderCall { step: 1, @@ -200,7 +215,7 @@ fn provenance() -> Provenance { #[test] fn the_total_is_the_rates_times_the_split_and_not_a_figure_typed_here() { let usage = split(); - let fresh = usage.prompt_tokens - usage.cache_read_tokens - usage.cache_write_tokens; + let fresh = usage.prompt_tokens - usage.cache_read_tokens - written(&usage); assert_eq!(fresh, 5_000, "the fixture's fresh prompt tokens"); // Per-million everywhere except the server tool requests, which vendors bill @@ -208,7 +223,7 @@ fn the_total_is_the_rates_times_the_split_and_not_a_figure_typed_here() { let mtok = fresh as u128 * INPUT as u128 + usage.completion_tokens as u128 * OUTPUT as u128 + usage.cache_read_tokens as u128 * CACHE_READ as u128 - + usage.cache_write_tokens as u128 * CACHE_WRITE as u128; + + written(&usage) as u128 * CACHE_WRITE as u128; assert_eq!( mtok % 1_000_000, 0, @@ -429,9 +444,9 @@ fn a_model_the_table_does_not_price_is_a_floor_and_says_so() { #[test] fn the_cache_figures_are_inside_the_prompt_and_never_added_to_it() { let usage = split(); - let fresh = usage.prompt_tokens - usage.cache_read_tokens - usage.cache_write_tokens; + let fresh = usage.prompt_tokens - usage.cache_read_tokens - written(&usage); assert_eq!( - usage.cache_read_tokens + usage.cache_write_tokens + fresh, + usage.cache_read_tokens + written(&usage) + fresh, usage.prompt_tokens, "the fixture's own parts do not sum to its prompt", ); @@ -452,7 +467,7 @@ fn the_cache_figures_are_inside_the_prompt_and_never_added_to_it() { ); assert_eq!( field(&rows, "of which cache written"), - spelled(usage.cache_write_tokens), + spelled(written(&usage)), ); assert_eq!( field(&rows, "of which fresh"), @@ -486,6 +501,224 @@ fn the_cache_figures_are_inside_the_prompt_and_never_added_to_it() { ); } +/// **F10 — a cache-write count nobody reported is a third state, and it is drawn +/// as one.** +/// +/// io-harness 0.76.0 made `Usage::cache_write_tokens` an `Option`, which is +/// the shape this module's first honesty rule has always wanted: a count the +/// provider reported no usage for is unknown, never zero. It is not a +/// hypothetical distinction — **every OpenRouter call reports `None`**, and +/// OpenRouter is the provider this product's own evidence is taken on, so the +/// obvious `unwrap_or(0)` would put a fabricated `0` on the page an operator +/// reads to find out what a turn cost. +/// +/// Three states, three assertions, and the pairwise distinctness asserted +/// explicitly — the shape `tests/status.rs`'s +/// `f14_a_probe_and_a_state_nobody_reached_are_different_words` uses for exactly +/// the same reason. +/// +/// Sabotage: `unwrap_or(0)` in `cost::page`. Under it the unknown arm draws the +/// same string as the reported-zero arm, the distinctness assertion fails by +/// name, and no other test in this file moves. +#[test] +fn f10_an_unreported_cache_write_is_unknown_and_a_reported_zero_is_zero() { + let spelled = io_cli::status::format_tokens; + + let usage_with = |cache_write| Usage { + prompt_tokens: 10_000, + completion_tokens: 2_000, + total_tokens: 12_000, + cache_read_tokens: 4_000, + cache_write_tokens: cache_write, + ..Default::default() + }; + let written_row = |cache_write| { + let fixture = seeded(&[call(Some(PRICED), Some(usage_with(cache_write)))]); + field( + &page(&fixture, &table(), &provenance()), + "of which cache written", + ) + .to_string() + }; + + let unknown = written_row(None); + let zero = written_row(Some(0)); + let some = written_row(Some(1_000)); + + assert_eq!( + some, + spelled(1_000), + "a provider that reported a cache-write count must have it drawn", + ); + assert_eq!( + zero, + spelled(0), + "a provider that reported zero cache writes reported a number, and it is drawn as one", + ); + assert_eq!( + unknown, "unknown", + "a provider that reported no cache-write count at all must not be drawn a number", + ); + + // The distinctness is the property, and it is asserted rather than left to + // follow from the three equalities above: a later change that made both + // render `0` would break two of those and this one, and this is the one whose + // failure names the actual defect. + assert_ne!( + unknown, zero, + "unknown and reported-zero drew the same string, so the page cannot tell an operator \ + which one happened", + ); + + // And the remainder inherits the silence. Fresh is the prompt less both + // cached parts, so substituting zero for the unknown write would draw a fresh + // figure too large by exactly the amount nobody measured — and it would look + // like a fact. + let fixture = seeded(&[call(Some(PRICED), Some(usage_with(None)))]); + let rows = page(&fixture, &table(), &provenance()); + assert_eq!( + field(&rows, "of which fresh"), + "unknown", + "the fresh figure was computed from a cache-write count that does not exist", + ); + assert_eq!( + field(&rows, "of which cache read"), + spelled(4_000), + "the read count was reported and stays a number", + ); +} + +/// **F10's other half — summing an unknown with a known does not erase the +/// known.** +/// +/// `cost::add` folds many calls into one `Usage`. Two silences stay a silence; +/// one silence beside a number keeps the number, because a turn that reported +/// 1,000 cache writes and a turn that reported nothing together wrote *at least* +/// 1,000, and answering `unknown` there would hide a figure the provider did give +/// us. +/// +/// Sabotage: make the mixed case `None`. The page then reads `unknown` for a run +/// where one call reported a real count, and this fails while the single-call +/// arms above all still pass. +#[test] +fn f10_folding_an_unreported_count_with_a_reported_one_keeps_the_number() { + // A non-zero read count is load-bearing on the fixture rather than + // decoration: with no reads and no reported writes the page draws no cache + // block at all — correctly, since there is nothing to say — and the row this + // test reads would not exist. The reads are what put the block on the page so + // the *write* row's three states can be compared. + let with = |cache_write| Usage { + prompt_tokens: 1_000, + completion_tokens: 100, + total_tokens: 1_100, + cache_read_tokens: 100, + cache_write_tokens: cache_write, + ..Default::default() + }; + let spelled = io_cli::status::format_tokens; + let row = |calls: &[_], name: &str| { + field(&page(&seeded(calls), &table(), &provenance()), name).to_string() + }; + let drawn = |calls: &[_]| row(calls, "of which cache written"); + + let none_reported = [ + call(Some(PRICED), Some(with(None))), + call(Some(PRICED), Some(with(None))), + ]; + let mixed = [ + call(Some(PRICED), Some(with(None))), + call(Some(PRICED), Some(with(Some(1_000)))), + ]; + let all_reported = [ + call(Some(PRICED), Some(with(Some(250)))), + call(Some(PRICED), Some(with(Some(750)))), + ]; + + assert_eq!( + drawn(&none_reported), + "unknown", + "no call reported a cache-write count, so the fold has nothing to report", + ); + assert_eq!( + drawn(&all_reported), + spelled(1_000), + "two reported counts must sum, and neither is in doubt", + ); + + // **The mixed fold is a FLOOR and is drawn as one.** The number is kept — + // one call really did report 1,000 writes and answering `unknown` would hide + // a figure the provider gave us — but it is not a count, because how many + // more the silent call made is unknown. This assertion is the fix for a High + // the adversarial review found: the first version drew a bare `1.0k` here. + assert_eq!( + drawn(&mixed), + format!("{} or more", spelled(1_000)), + "a fold of one reporting call and one silent one is a floor, and drawing \ + it as a plain number claims a count nobody can support", + ); +} + +/// **F10's third half — "of which fresh" carries every doubt the write count +/// carries.** +/// +/// Fresh is the prompt less both cached parts, so subtracting a **floor** yields +/// a **ceiling** — and a ceiling drawn as a plain number is a fact that is too +/// large by exactly the amount nobody measured. `src/cost.rs`'s own comment says +/// that must not happen, and the first version of this release's change let it: +/// the guard distinguished only "no call reported" from "some call reported", so +/// a mixed fold took the confident arm and drew an exact-looking number. +/// +/// **Found by the adversarial review, not by the suite** — the mixed-fold test +/// above existed and read only the written row, never this one. +/// +/// Reachable in practice rather than in theory: `openai_wire` reports no +/// cache-write count at all and Anthropic reports one only where a cache was +/// created, so any session that changes model across those two vendors folds both +/// shapes into one total. +/// +/// Sabotage: key the fresh row on `cache_write_tokens` alone, as it was. This +/// fails and the written-row assertion above still passes. +#[test] +fn f10_a_fresh_figure_computed_from_a_floor_is_not_drawn_as_a_number() { + let with = |cache_write| Usage { + prompt_tokens: 1_000, + completion_tokens: 100, + total_tokens: 1_100, + cache_read_tokens: 100, + cache_write_tokens: cache_write, + ..Default::default() + }; + let fresh = |calls: &[_]| { + field( + &page(&seeded(calls), &table(), &provenance()), + "of which fresh", + ) + .to_string() + }; + + assert_eq!( + fresh(&[ + call(Some(PRICED), Some(with(None))), + call(Some(PRICED), Some(with(Some(1_000)))), + ]), + "unknown", + "the fresh figure was computed by subtracting a floor, which makes it a \ + ceiling — and it was drawn as though it were a measurement", + ); + + // The control: with every call reporting, fresh is a real number again. + // Without this, a function that always answered `unknown` would pass. + let spelled = io_cli::status::format_tokens; + assert_eq!( + fresh(&[ + call(Some(PRICED), Some(with(Some(250)))), + call(Some(PRICED), Some(with(Some(750)))), + ]), + spelled(2_000 - 200 - 1_000), + "every call reported, so nothing is in doubt and fresh is exact", + ); +} + // --------------------------------------------------------------------------- // `money` // --------------------------------------------------------------------------- diff --git a/tests/dependencies.rs b/tests/dependencies.rs index 8b2cee1..f5727ae 100644 --- a/tests/dependencies.rs +++ b/tests/dependencies.rs @@ -1128,15 +1128,32 @@ fn f7_the_configuration_is_read_through_the_harness_and_never_parsed_here() { /// The modules permitted to turn somebody else's JSON into a value. /// -/// **Two, and they are files rather than modules.** `src/import.rs` has read the -/// operator's own `~/.claude.json` and Codex settings since 0.21.0; +/// **Three, and they are files rather than modules.** `src/import.rs` has read +/// the operator's own `~/.claude.json` and Codex settings since 0.21.0; /// `src/adapt.rs` reads the three foreign plugin manifest formats. Sorted here /// for the reason `spawning_modules` is sorted: `read_dir` answers in whatever /// order the filesystem holds, and a test that passes on one machine and fails on /// another is worse than no test. +/// +/// **`src/acp.rs` is 0.36.0's, and it is admitted on a different ground rather +/// than on a relaxed one.** The rule this list enforces is stated in the sweep +/// below: a module deciding what somebody else's *file* means is a second opinion +/// about a question that already has one, which is why the operator's Claude +/// configuration and a stranger's plugin manifest each get exactly one reader. An +/// ACP frame is not somebody else's file. It is a request addressed to this +/// process, arriving on this process's own stdin, with no on-disk format whose +/// meaning is being re-decided and no other reader in the crate to disagree with +/// — and declining to parse it would mean not speaking the protocol at all. That +/// is a different question from the one the rule answers, so the exemption is +/// granted for a reason and the reason is written here rather than left as a +/// third entry somebody later reads as precedent for a fourth. fn json_reading_modules() -> Vec { let src = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); - let mut permitted = vec![src.join("adapt.rs"), src.join("import.rs")]; + let mut permitted = vec![ + src.join("acp.rs"), + src.join("adapt.rs"), + src.join("import.rs"), + ]; permitted.sort(); permitted } @@ -1259,6 +1276,14 @@ fn n1_the_permitted_json_set_is_exact_paths_and_never_a_substring() { // is asserted for both rather than for the new one only. src.join("importer.rs"), src.join("import").join("mod.rs"), + // 0.36.0's permitted path gets the same three shapes, and the first of + // them is a file that really exists: `src/acp_map.rs` is the event + // translation, it sits beside the transport, and it deserializes nothing. + // A stem-prefix match would admit it silently — which is the whole reason + // this arm is a live near-miss here rather than an invented name. + src.join("acp_map.rs"), + src.join("acp").join("mod.rs"), + src.join("acpx.rs"), ] { let mut widened = permitted.clone(); widened.push(near.clone()); @@ -1274,6 +1299,202 @@ fn n1_the_permitted_json_set_is_exact_paths_and_never_a_substring() { } } +/// **N3 — the ACP adapter runs on tokio, and no second async executor entered +/// the tree behind it.** +/// +/// This is the criterion the hand-written transport in `src/acp.rs` exists to +/// keep. The official `agent-client-protocol` crate is correct and well made and +/// was declined for one reason: it arrives with `async-io`, `async-process`, +/// `async-task`, `blocking` and `polling`, and its stdio transport puts +/// `blocking::Unblock` over the real stdin and stdout. This binary already has a +/// reader of stdin and an owner for it — `src/stdin.rs`, which exists, with its +/// own history, because of what a second one cost in 0.13.1. +/// +/// **Asserted on the lockfile rather than the manifest, and that is the whole +/// point.** The hazard arrives transitively: nothing would appear in +/// `Cargo.toml`, `ALLOWED` would still hold at ten names, and the second reactor +/// would be in the binary anyway. The lesson is already written down in this +/// repository from the ratatui 0.30 cursor query — read the lockfile, not the +/// facade's manifest. +/// +/// Sabotage: add `agent-client-protocol` to `[dependencies]`. `ALLOWED` fails +/// too, which is correct and is not this arm's job; this one fails naming the +/// executor crates, which is the fact a reader needs. +#[test] +fn n3_no_second_async_executor_is_in_the_tree() { + let lock = std::fs::read_to_string(Path::new(env!("CARGO_MANIFEST_DIR")).join("Cargo.lock")) + .expect("Cargo.lock exists; this crate is a binary and commits its lockfile"); + + let locked: Vec<&str> = lock + .lines() + .filter_map(|line| { + line.strip_prefix("name = \"") + .and_then(|rest| rest.strip_suffix('"')) + }) + .collect(); + + // The control. Without it a lockfile this parser failed to read at all would + // satisfy every assertion below by containing nothing. + assert!( + locked.contains(&"tokio"), + "the lockfile parser found no `tokio`, so it is not reading the lockfile and \ + the absences below mean nothing", + ); + + // `futures-lite` and `async-lock` are deliberately not in this list: they are + // plumbing that a tokio-only tree can legitimately acquire through some other + // path. These five are the reactor and its process and thread plumbing — + // arriving together, they are a second executor and not a coincidence. + for reactor in [ + "async-io", + "async-task", + "async-process", + "blocking", + "polling", + ] { + assert!( + !locked.contains(&reactor), + "`{reactor}` is in Cargo.lock. io-cli runs one async runtime, and \ + `src/acp.rs` speaks the ACP wire itself rather than taking a crate \ + that brings a second one over the real stdin. If this is deliberate, \ + the argument belongs in the release record and in `Cargo.toml` \ + beside the name that pulled it in.", + ); + } +} + +/// **N3's other half — there is exactly one place a future is driven, and it is +/// the entry point.** +/// +/// A source-text gate, because the defect it guards has no runtime symptom until +/// a client is attached: a `block_on` reached from inside an async task is how a +/// task ends up waiting on a reactor that is not running it, and with a protocol +/// reader on stdin the symptom is a session that hangs rather than one that +/// fails. +/// +/// **A count, not a `contains`, and not a per-file ban.** `src/main.rs:51` is +/// `runtime.block_on(run(&mut report))` — the one legitimate site, where the +/// binary crosses from sync `fn main` into the async program, and a blanket ban +/// would be a gate written from imagination that fails on correct code. Counting +/// is the stronger property anyway: it stays exactly as meaningful when a second +/// site is added for a bad reason, which exempting the file by path would not. +/// +/// Sabotage: add a `block_on` anywhere — in `src/acp.rs` to await a permission +/// answer, which is the plausible wrong fix for the round trip. The count goes to +/// two and this fails naming the file. +#[test] +fn n3_one_future_is_driven_and_it_is_the_binarys_entry_point() { + let mut sites: Vec<(String, usize)> = Vec::new(); + + for (path, text) in sources() { + // Comments are stripped first, for the reason every sweep in this file + // strips them: a gate that reads prose forbids a file from explaining + // itself, and this test's own subject has to be nameable in a doc comment. + let code = code_of(&text); + + let count = code.matches("block_on").count(); + if count > 0 { + sites.push((path.display().to_string(), count)); + } + + // These two bring a foreign reactor rather than driving this one, and + // neither has a legitimate site in this crate at any count. + for spelling in ["futures::executor", "async_io::", "async_std::"] { + assert!( + !code.contains(spelling), + "{} names `{spelling}`, which is a second async runtime. io-cli runs \ + one, and `src/acp.rs` speaks the ACP wire itself rather than taking \ + a crate that brings another over the real stdin.", + path.display(), + ); + } + } + + let total: usize = sites.iter().map(|(_, count)| count).sum(); + assert_eq!( + total, 1, + "a future is driven at {total} sites: {sites:?}. Exactly one is correct — \ + `src/main.rs`, where sync `fn main` enters the async program. Every other \ + path is `async fn` all the way down, and a `block_on` reached from inside \ + a task waits on a reactor that is not running it.", + ); + assert!( + sites[0].0.ends_with("main.rs"), + "the one site that drives a future is {}, not the binary's entry point", + sites[0].0, + ); +} + +/// **N4 — the ACP adapter owns stdin and stdout alone.** +/// +/// stdout is the protocol: a byte written there that is not part of a JSON-RPC +/// frame is a frame boundary in the wrong place, and every message after it is +/// parsed against the wrong offset. stdin is the frame stream: a second reader +/// takes frames away from the protocol. +/// +/// This product has already lost time to exactly one of those. [`crate::stdin`] +/// — `src/stdin.rs` — exists because a keyboard reader and a cursor query fought +/// over the terminal, and 0.13.1 is what that cost. An ACP session with a +/// `Screen` in the same process is the same defect with a worse symptom, because +/// a corrupted protocol stream does not look like a rendering fault. +/// +/// A source-text gate over `src/acp.rs`, because the property is about what the +/// module does *not* do and there is no runtime moment at which to observe an +/// absence. Comments are stripped first, so the module's own documentation can +/// name what it avoids. +/// +/// Sabotage: build a `Screen` in `acp::main`. Fails here naming the spelling. +#[test] +fn n4_the_acp_adapter_takes_no_terminal_and_no_second_reader_of_stdin() { + let acp = code_of( + &std::fs::read_to_string(Path::new(env!("CARGO_MANIFEST_DIR")).join("src/acp.rs")) + .expect("src/acp.rs exists"), + ); + + // **`eprintln!` contains `println!`.** The first version of this gate banned + // the second literal and failed on the module's own diagnostics, which go to + // stderr and are exactly what this file is supposed to use. Removing the + // stderr spelling before the sweep is the fix; a needle that cannot tell the + // two apart is a gate that forbids the correct behaviour. + let acp = acp.replace("eprintln!", "").replace("eprint!", ""); + + for spelling in [ + "Screen", + "enable_raw_mode", + "crossterm", + "ratatui", + "stdin::", + "println!", + "print!", + ] { + assert!( + !acp.contains(spelling), + "`src/acp.rs` names `{spelling}`. stdout is the protocol and stdin is \ + the frame stream: a terminal, a raw-mode switch, a cursor query or a \ + write to stdout corrupts the session, and the corruption does not \ + look like a rendering fault. See `src/stdin.rs` for what a second \ + reader cost in 0.13.1.", + ); + } + + // The driver's own arm must leave before anything takes the terminal. Read as + // text because nothing under `tests/` links `src/main.rs`, and said plainly + // rather than implied: this is a source-text gate and that is what it can be. + let driver = std::fs::read_to_string(Path::new(env!("CARGO_MANIFEST_DIR")).join("src/main.rs")) + .expect("src/main.rs exists"); + let acp_at = driver + .find("Subcommand::Acp") + .expect("`src/main.rs` routes the acp subcommand"); + let screen_at = driver + .find("Screen::") + .expect("`src/main.rs` builds a Screen somewhere"); + assert!( + acp_at < screen_at, + "`src/main.rs` builds a `Screen` before it dispatches `io acp`, so the \ + adapter would inherit a process that has already taken the terminal", + ); +} + #[test] fn n1_the_binary_is_named_io_and_there_is_exactly_one() { let manifest = manifest(); @@ -1926,6 +2147,14 @@ fn f9_every_door_that_reaches_a_run_places_a_bundles_programs() { assert_eq!( sites, vec![ + // 0.36.0's fourth door. `acp::main` returns from `src/main.rs` before + // the interactive path resolves anything — the same reason the two + // below need their own calls — so an editor session would otherwise + // be the third place a bundle's declared program is silently not on + // `PATH`. **This gate is what said so**: the arm was added, the count + // did not match, and the missing call was named rather than + // discovered in a live run the way 0.34.0's was. + (src.join("acp.rs"), 1), // `install_for` twice: `exec::main` and `exec::resume_main`. Both are // in the library, so unlike the driver's they can be reached from a // test at all. diff --git a/tests/docs.rs b/tests/docs.rs index 2aed12f..ddfac13 100644 --- a/tests/docs.rs +++ b/tests/docs.rs @@ -1170,14 +1170,32 @@ fn f19_no_source_cites_an_io_harness_this_crate_does_not_pin() { // carrying two citations was checked once and the second could name any version // at all; and a flat `read_dir` covers `src/` only for as long as `src/` stays // flat, which is not a property anybody is maintaining. + // + // **0.36.0 — `tests/` is swept too, and it was not before.** The pin to + // 0.76.0 left a stale `io-harness-0.74.0/src/plugin.rs:1097` in + // `tests/adapt.rs` and this gate passed over it, because the walk started at + // `src/` alone. A test file's citation is read by exactly the same person for + // exactly the same reason as a source file's, and the argument above — that a + // citation into an unpinned version cannot even be checked — does not care + // which directory the comment is in. let mut files: Vec = Vec::new(); - let mut dirs = vec![repo().join("src")]; + let mut dirs = vec![repo().join("src"), repo().join("tests")]; while let Some(dir) = dirs.pop() { for entry in std::fs::read_dir(&dir).expect("a source directory") { let path = entry.expect("a directory entry").path(); if path.is_dir() { dirs.push(path); - } else if path.extension().and_then(|e| e.to_str()) == Some("rs") { + } else if path.extension().and_then(|e| e.to_str()) == Some("rs") + // **This file is exempt from its own sweep, and the exemption is + // the point rather than a convenience.** A gate that reads prose + // forbids a file from explaining itself — a rule this repository + // has now paid for four times, most visibly when the stale-claim + // sweep hit a CHANGELOG entry written to say a placeholder had + // been removed. This test cannot state which citation form it + // looks for, quote the nine that motivated it, or build its own + // needle with `format!`, without matching itself. + && path.file_name().and_then(|n| n.to_str()) != Some("docs.rs") + { files.push(path); } } @@ -1191,6 +1209,17 @@ fn f19_no_source_cites_an_io_harness_this_crate_does_not_pin() { // vendored source tree. A bare "io-harness 0.70.0" is history — a // sentence about when something changed — and is not a citation. for (at, _) in line.match_indices("io-harness-") { + // A version literal begins with a digit. Without this the sweep + // also flags the machinery that *builds* such a path — + // `format!("io-harness-{version}")` in `tests/support/mod.rs` + // resolves at runtime to whatever is pinned and is the opposite + // of a stale citation. Checking the shape is better than + // exempting the file: the exemption would also hide a real + // citation that file later grew. + let rest = &line[at + "io-harness-".len()..]; + if !rest.starts_with(|c: char| c.is_ascii_digit()) { + continue; + } if !line[at..].starts_with(&wanted) { stale.push(format!( "{}:{}: {}", diff --git a/tests/events.rs b/tests/events.rs index 50dce47..6925649 100644 --- a/tests/events.rs +++ b/tests/events.rs @@ -615,8 +615,8 @@ fn f8_this_release_has_seen_every_kind_io_harness_emits() { let declared = support::harness_event_kinds(); assert_eq!( declared.len(), - 52, - "the locked io-harness declares fifty-two event kinds; found {}", + 53, + "the locked io-harness declares fifty-three event kinds; found {}", declared.len(), ); } diff --git a/tests/exec.rs b/tests/exec.rs index 7a5aaba..5e7cdf6 100644 --- a/tests/exec.rs +++ b/tests/exec.rs @@ -2891,3 +2891,49 @@ fn f10_a_configuration_with_no_templates_passes_every_plain_goal_through() { "a template named where none are configured is refused, not rendered empty", ); } + +/// **F7 — `io exec` still refuses every approval, and 0.36.0 is the release that +/// could have changed that by accident.** +/// +/// This release gives io-cli its first non-interactive door that *can* answer an +/// approval: an ACP client is a person in an editor, so `src/acp.rs` raises +/// `session/request_permission` and routes the answer to `io_harness::Approver`. +/// `io exec` is not that. It is an unattended run with nobody to ask, and the +/// argument at `src/exec.rs:614-621` — that an ask becomes a refusal the agent is +/// told about and adapts to, because an approver that blocked would hang forever +/// — is unchanged and correct. +/// +/// The failure mode this guards is the new wiring reaching the old door while +/// every ACP test stays green. So it is a **count over the call sites**, never a +/// `contains`: `src/exec.rs` drives five turns — the ordinary one and four resume +/// entry points — and a `contains` is satisfied forever by any one of them. That +/// is the discipline `AGENTS.md` states, and 0.30.0 records what happens when a +/// gate is satisfied by one door out of two. +/// +/// Sabotage: replace any single `&DenyAll` with the ACP approver. The count drops +/// to four and this fails; nothing in `tests/acp.rs` moves. +#[test] +fn f7_every_headless_turn_still_refuses_every_approval() { + let text = source("exec.rs"); + + let sites = text.matches("&DenyAll").count(); + assert_eq!( + sites, 5, + "`src/exec.rs` passes `&DenyAll` at {sites} sites and five are expected — \ + the ordinary turn and the four resume entry points. If a turn was added, \ + it needs one too: an unattended run has nobody to ask, and an approver \ + that blocks there hangs forever. If one was removed, say which door can \ + now answer an approval and how.", + ); + + // And the adapter's own approval path is not reachable from this file. The + // two doors are deliberately different and naming the ACP seam here would be + // the first step of merging them. + for spelling in ["acp::", "permission_answer", "request_permission"] { + assert!( + !text.contains(spelling), + "`src/exec.rs` names `{spelling}`. The headless door refuses approvals \ + and the ACP door answers them; they do not share a decision.", + ); + } +} diff --git a/tests/hooks.rs b/tests/hooks.rs index 865abc4..22500a5 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.69.0/src/hooks.rs`) — deliberately, so that +// value is constructed (`io-harness-0.76.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 dd9c340..2ce548b 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.69.0/src/config.rs:158` — the automatic set is exactly +//! (`io-harness-0.76.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 diff --git a/tests/manage.rs b/tests/manage.rs index 997a51b..76bd8d5 100644 --- a/tests/manage.rs +++ b/tests/manage.rs @@ -1294,6 +1294,54 @@ fn every_managed_surface_has_a_subcommand_clap_will_route() { } } +/// **F8 — `io acp` exists at the argv door and clap routes it.** +/// +/// Asked of `clap::CommandFactory` for the reason the gate above it exists: the +/// door is the `Subcommand` enum, nothing under `tests/` links `src/main.rs`, and +/// 0.30.0 shipped a documented subcommand the door had no variant for while 1,609 +/// tests passed over the gap. A release that adds a subcommand and no gate for it +/// is that release again. +/// +/// `acp` takes no arguments of its own — the client speaks the protocol, not the +/// command line — so this asserts routing and the absence of a positional, which +/// is the shape a client's spawn depends on. The global flags stay available, +/// because `-C` and `--profile` are how an editor points io at the workspace it +/// has open. +/// +/// Sabotage: delete `Acp` from `cli::Subcommand`. Only this fails. +#[test] +fn f8_the_acp_subcommand_exists_and_clap_routes_it() { + use clap::CommandFactory as _; + + let command = io_cli::cli::Cli::command(); + let acp = command + .get_subcommands() + .find(|sub| sub.get_name() == "acp") + .expect( + "clap does not route `acp`, so `io acp` answers `unrecognized subcommand` \ + for the door this release exists to open", + ); + + assert!( + acp.get_positionals().next().is_none(), + "`io acp` takes a positional argument. The client speaks the protocol on \ + stdin; anything on the command line is a second way to say the same thing.", + ); + + // The global flags an editor needs are still reachable on this subcommand. + // `-C` is how a client points io at the workspace it has open, and a + // subcommand that dropped it would work only from the directory io was + // launched in. + let command = io_cli::cli::Cli::command(); + for flag in ["dir", "model", "profile", "plain"] { + assert!( + command.get_arguments().any(|arg| arg.get_id() == flag), + "`--{flag}` is not a global argument any more, so `io acp` cannot be \ + pointed at a workspace", + ); + } +} + /// **F3 — a mistyped verb names the verbs that surface does take.** /// /// The bare-surface gate above covers `io skill` with nothing after it. This diff --git a/tests/reload.rs b/tests/reload.rs index 5c95043..322debd 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.69.0/src/config.rs:1893-1896`), so the +/// `AGENTS.md`:\n` (`io-harness-0.76.0/src/config.rs:2822-2825`), 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 8fe3822..1056f1b 100644 --- a/tests/routing.rs +++ b/tests/routing.rs @@ -1,9 +1,13 @@ -//! F3 and F4 — what `[app.io-cli.routing]` puts on the contract, and what a -//! contained session is told about it. +//! F3, F4 and F11 — what `[app.io-cli.routing]` puts on the contract, what a +//! contained session is told about it, and what an operator who also wrote +//! io-harness's own `[routing]` is told. //! //! F3 is the section reaching an `io_harness::Routing`: the keys an operator //! types, the two rules they name, and the run's behaviour under them. F4 is the -//! disclosure that those rules do not fire for a contained turn. +//! disclosure that those rules do not fire for a contained turn. F11 is the +//! disclosure the io-harness 0.76.0 pin owes: two routing tables can now be +//! written in one file, io-cli's replaces the harness's without a word, and the +//! four combinations are asserted here because only one of them is a loss. //! //! Two things here are asserted the way the implementation would be wrong rather //! than the way it is right. @@ -20,6 +24,8 @@ //! both leave the contract's routing unset; building a `Routing::default()` for //! the first is the mistake that looks harmless, so it gets its own test. +mod support; + use io_cli::routing::{self, Downshift, Escalation, Settings}; /// The models the fixtures route between. Names, not real models: nothing here @@ -386,3 +392,138 @@ fn every_refusal_says_which_key_is_wrong_and_that_the_turn_is_not_routed() { "an obeyable section has nothing to explain", ); } + +// --------------------------------------------------------------------------- +// F11 — io-harness's own [routing] table beside io-cli's section +// --------------------------------------------------------------------------- + +/// io-harness's own table, in the smallest form that loads. +/// +/// `mechanical` rather than a rule, for two reasons. It is the one key with no +/// partner — `Config::check_routing` refuses `escalate_after` without +/// `escalate_to` — so the fixture needs nothing it is not asserting over. And it +/// is the key io-cli offers no equivalent for at all, which makes it the clearest +/// thing a collision silently takes away: an operator who loses it cannot get it +/// back by rewriting the rule in io-cli's spelling, because there is no spelling. +const NATIVE: &str = "[routing]\nmechanical = \"vendor/folding-model\"\n"; + +/// A user-scope file that configures something which is not routing. +/// +/// Not the empty string: an empty file exercises the "nothing was written +/// anywhere" path as well as the "no routing was written" one, and a detector +/// keyed on the configuration being non-empty would pass against it. A file with +/// one unrelated key leaves only the question this asserts. +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 +/// `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 +/// the collision this criterion is about can be written. +/// +/// Sabotage: return a sentence before the `origins` scan, under which a +/// configuration that names no routing at all is told about one. +#[test] +fn f11_neither_routing_section_configured_is_told_nothing() { + let scope = support::user_scope(UNRELATED); + assert_eq!(routing::native_notice(&scope.config, None), None); +} + +/// Sabotage: key the notice on `[app.io-cli.routing]` instead of on `[routing]`, +/// under which this test is told about a harness table that is not there. +#[test] +fn f11_an_io_cli_section_with_no_harness_table_beside_it_is_told_nothing() { + let scope = support::user_scope(UNRELATED); + assert_eq!( + routing::native_notice(&scope.config, Some(&both())), + None, + "io-cli's own section is what every other notice in this file already covers", + ); +} + +/// A `[routing]` alone loses nothing and is still invisible, so it says so. +/// +/// io-harness merges the table onto the contract and io-cli leaves the contract's +/// routing alone, so the operator's rules are intact. What they do not have is any +/// surface that shows them: `/config` lists four keys and every one is +/// `app.io-cli.routing.*`, and `describe`, `notice` and `inert_under_containment` +/// all read that section alone. +/// +/// Sabotage: return `None` unless io-cli's section also reaches the contract — +/// the shape a "warn on collision" reading of the criterion produces — under which +/// this operator is told nothing and the invisibility stands. +#[test] +fn f11_a_harness_table_alone_says_no_io_cli_surface_lists_it() { + let scope = support::user_scope(NATIVE); + let notice = routing::native_notice(&scope.config, None) + .expect("a table no surface here lists has to be named by one of them"); + assert!( + notice.contains("[routing]"), + "the notice names the section the operator has to open: {notice}", + ); + assert!( + notice.contains("/config"), + "the notice names the surface that will not show it: {notice}", + ); + assert!( + !notice.contains("both"), + "nothing was overwritten here, so this must not be the collision sentence: {notice}", + ); + assert!( + notice.is_ascii(), + "this renders under NO_COLOR and through the ASCII glyph set: {notice}", + ); +} + +/// 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 +/// `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 +/// nothing said so before this release. +/// +/// **The second half is the branch a plausible wrong implementation gets wrong.** A +/// section that does not survive `routing::routing` — present and empty, or refused +/// for half a rule — leaves `contract::configured` on its `None` arm, so the merged +/// `[routing]` is still exactly what the contract carries and the operator has lost +/// nothing. Those must read as the other sentence. +/// +/// Sabotage: ask `settings.is_some()` instead of asking `routing::routing`, under +/// which the loop below reports a loss to two operators who suffered none. +#[test] +fn f11_both_sections_configured_names_the_one_that_reaches_the_contract() { + let scope = support::user_scope(NATIVE); + let notice = routing::native_notice(&scope.config, Some(&both())) + .expect("a section that takes [routing] off the contract has to say so"); + assert!( + notice.contains("both"), + "the operator wrote two tables and hears that they did: {notice}", + ); + assert!( + notice.contains("[routing]") && notice.contains("[app.io-cli.routing]"), + "the notice names both tables, or it cannot say which one won: {notice}", + ); + assert!( + notice.contains("mechanical"), + "the key io-cli has no equivalent for is the one worth naming: {notice}", + ); + assert!( + notice.is_ascii(), + "this renders under NO_COLOR and through the ASCII glyph set: {notice}", + ); + + let empty: Settings = toml::from_str("").expect("a present but empty section parses"); + let half_a_rule: Settings = toml::from_str("[escalate_after]\nfailures = 3\n") + .expect("half a rule parses and is refused by name"); + for standing in [empty, half_a_rule] { + let notice = routing::native_notice(&scope.config, Some(&standing)) + .expect("the harness table is still there and still unlisted"); + assert!( + !notice.contains("both"), + "a section that never reaches the contract overwrites nothing: {notice}", + ); + } +} diff --git a/tests/triage.rs b/tests/triage.rs index a2093f9..deb1d2b 100644 --- a/tests/triage.rs +++ b/tests/triage.rs @@ -75,8 +75,8 @@ fn the_table_names_every_kind_the_locked_harness_declares() { // reading the enum and handed back a shorter list that happens to be a subset. assert_eq!( declared.len(), - 52, - "the locked io-harness declares fifty-two event kinds; found {}", + 53, + "the locked io-harness declares fifty-three event kinds; found {}", declared.len(), ); } @@ -106,6 +106,36 @@ fn a_batched_ask_is_a_line_because_no_overlay_carries_the_batch() { ); } +/// **0.36.0 — the per-step timing breakdown is silent because the step line above +/// it already draws the span.** +/// +/// io-harness 0.75.0 emits `StepAttributed` from the same place as `Step`, for +/// every committed step. A `Line` row would put a second row under every step +/// row restating that step's span, which doubles the transcript to say once more +/// what the row above it said. The route is the machine surfaces — `io exec +/// --json` forwards it verbatim — and the harness's own declaration points an +/// observer at `Store::step_attributions`, which is where the phases are read. +/// +/// Sabotage: make the row `Line`. That is the plausible wrong answer, because +/// where a slow step went is the most actionable figure a run produces, and it +/// fails here by name rather than as a doubled transcript nobody reads twice. +/// Sabotage the route instead — swap the machine surfaces for a `/command` that +/// draws none of these numbers — and `f9_every_silent_route_names_a_surface_that_exists` +/// stays green, which is why the reader is asserted here as well. +#[test] +fn a_step_attribution_is_silent_because_the_step_line_already_draws_its_span() { + assert_eq!( + triage::disposition("step_attributed"), + Some(Disposition::Silent), + "a breakdown emitted beside every step draws a second row under every step row", + ); + let route = triage::route("step_attributed").expect("the step_attributed row"); + assert!( + route.contains("step_attributions"), + "the route does not name the reader that has the same figures: {route}", + ); +} + #[test] fn the_table_has_no_duplicate_and_every_row_records_a_route() { let mut seen: Vec<&str> = Vec::new(); @@ -120,7 +150,7 @@ fn the_table_has_no_duplicate_and_every_row_records_a_route() { "{name} has no route recorded, so nobody can check whether its fact reaches anyone", ); } - assert_eq!(seen.len(), 52); + assert_eq!(seen.len(), 53); } /// A `Line` kind with no arm behind it is the old defect wearing the new table's @@ -277,6 +307,13 @@ fn the_route_column_no_longer_says_a_session_has_no_verification_gate() { #[test] fn a_status_or_silent_kind_commits_nothing() { let quiet = vec![ + EventKind::StepAttributed { + span_ms: 1_400, + provider_ms: Some(900), + tool_ms: Some(400), + gate_ms: Some(50), + store_ms: None, + }, EventKind::ToolCall { name: "read_file".into(), target: "notes.txt".into(), @@ -432,7 +469,7 @@ fn the_dispositions_are_the_three_the_contract_names() { .count(); assert_eq!( lines + status + silent, - 52, + 53, "every kind is exactly one of the three: {lines} lines, {status} status, {silent} silent", ); // The release's own claim, and the reason it exists: most kinds are not