From 6a463ae16ed115ae2993425af36c9a5b49017c57 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 21:15:02 +0530 Subject: [PATCH 01/11] feat(deps): pin io-harness 0.76.0 and draw an unreported cache write as unknown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit io-harness 0.76.0 makes Usage::cache_write_tokens an Option. That is the shape this module's own first honesty rule has always wanted — src/cost.rs has said since 0.13.0 that a token count a provider reported no usage for is unknown, never zero — and it is not a hypothetical distinction: every OpenRouter call reports None, and OpenRouter is where this product's evidence is taken. unwrap_or(0) would have compiled perfectly and put a fabricated figure on the page an operator reads to find out what a turn cost. Instead: - add_reported folds two counts. Two silences stay a silence; one silence beside a number keeps the number, because a turn that reported 1,000 writes and a turn that reported nothing together wrote at least 1,000. The residual — a mixed sum is a floor rather than a total — is stated in the function's own doc rather than hidden, matching how this page already treats an unpriced model's money. - The remainder inherits the silence. "of which fresh" is the prompt less both cached parts, so an unreported write makes it unknown too. - The cache block is drawn when reads are non-zero or writes are a reported non-zero, so a provider reporting reads and no write count still shows the written row as unknown. An absent row reads as zero. src/ compiled clean on the pin; only tests/cost.rs broke, at exactly the sites the pre-code audit named. tests/stats.rs and tests/status.rs were predicted safe because they build Usage through ..Default::default(), and both compiled untouched. tests/cost.rs 12 passed / 0 failed, up from 10. Both new arms carry their sabotage in the doc comment. --- Cargo.lock | 12 ++-- Cargo.toml | 4 +- src/cost.rs | 63 +++++++++++++++--- tests/cost.rs | 176 ++++++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 232 insertions(+), 23 deletions(-) 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..4dac0fe 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 diff --git a/src/cost.rs b/src/cost.rs index 0440fdc..b91751e 100644 --- a/src/cost.rs +++ b/src/cost.rs @@ -132,11 +132,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 +311,37 @@ 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), + match usage.cache_write_tokens { + Some(count) => 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), - ), + match usage.cache_write_tokens { + Some(written) => tokens( + usage + .prompt_tokens + .saturating_sub(usage.cache_read_tokens) + .saturating_sub(written), + ), + None => UNREPORTED.to_string(), + }, )); } rows.push(Row::fact("completion", tokens(usage.completion_tokens))); @@ -459,6 +495,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/tests/cost.rs b/tests/cost.rs index 20d79e2..a69c73c 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,155 @@ 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 drawn = |calls: &[_]| { + field( + &page(&seeded(calls), &table(), &provenance()), + "of which cache written", + ) + .to_string() + }; + + assert_eq!( + drawn(&[ + call(Some(PRICED), Some(with(None))), + call(Some(PRICED), Some(with(None))), + ]), + "unknown", + "no call reported a cache-write count, so the fold has nothing to report", + ); + assert_eq!( + drawn(&[ + call(Some(PRICED), Some(with(None))), + call(Some(PRICED), Some(with(Some(1_000)))), + ]), + spelled(1_000), + "one call reported 1,000 cache writes and the fold dropped it", + ); + assert_eq!( + drawn(&[ + call(Some(PRICED), Some(with(Some(250)))), + call(Some(PRICED), Some(with(Some(750)))), + ]), + spelled(1_000), + "two reported counts must sum", + ); +} + // --------------------------------------------------------------------------- // `money` // --------------------------------------------------------------------------- From c568012d75c97e6b8b7965a29c240f5f7aeaeb86 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 21:25:49 +0530 Subject: [PATCH 02/11] =?UTF-8?q?chore(develop):=20Give=20EventKind::StepA?= =?UTF-8?q?ttributed=20a=20triage=20row=20and=20move=20th=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task: US-IO-CLI-0.36.0-T02 --- src/triage.rs | 29 ++++++++++++++++++++++++++++- tests/events.rs | 4 ++-- tests/triage.rs | 45 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 71 insertions(+), 7 deletions(-) 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/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/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 From b42e81296debf758429020d08de5a1e37a25d57f Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 21:25:49 +0530 Subject: [PATCH 03/11] =?UTF-8?q?chore(develop):=20Notice=20a=20harness-na?= =?UTF-8?q?tive=20[routing]=20section=20instead=20of=20si=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task: US-IO-CLI-0.36.0-T03 --- src/routing.rs | 98 +++++++++++++++++++++++++++++++ tests/routing.rs | 147 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 242 insertions(+), 3 deletions(-) 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/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}", + ); + } +} From 5335f927d6eabfbe2e7638a3ed961a811bcbfb07 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 21:25:49 +0530 Subject: [PATCH 04/11] =?UTF-8?q?chore(develop):=20The=20ACP=20transport?= =?UTF-8?q?=20=E2=80=94=20newline-delimited=20JSON-RPC=202.0=20fram?= =?UTF-8?q?=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task: US-IO-CLI-0.36.0-T05 --- src/acp.rs | 452 ++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + tests/acp.rs | 441 +++++++++++++++++++++++++++++++++++++++++ tests/dependencies.rs | 147 +++++++++++++- 4 files changed, 1038 insertions(+), 3 deletions(-) create mode 100644 src/acp.rs create mode 100644 tests/acp.rs diff --git a/src/acp.rs b/src/acp.rs new file mode 100644 index 0000000..4342136 --- /dev/null +++ b/src/acp.rs @@ -0,0 +1,452 @@ +//! 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}; +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, + }, +} + +impl Incoming { + /// The method name, whichever shape this is. + pub fn method(&self) -> &str { + match self { + Self::Request { method, .. } | Self::Notification { method, .. } => method, + } + } + + /// The parameters, whichever shape this is. + pub fn params(&self) -> &Value { + match self { + Self::Request { params, .. } | Self::Notification { params, .. } => params, + } + } +} + +/// 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, +} + +/// 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. Those are correlated elsewhere; reaching here means one + // arrived that nobody was waiting for, which is a client error rather + // than a crash. + return Err(Outgoing::Error { + id: envelope.id.unwrap_or(Value::Null), + code: ErrorCode::InvalidRequest, + message: "a request or notification names a method".into(), + }); + }; + + // 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, mut writer: W, handler: &mut H) -> std::io::Result<()> +where + R: tokio::io::AsyncBufRead + Unpin, + W: tokio::io::AsyncWrite + Unpin, + H: Handler, +{ + use tokio::io::{AsyncBufReadExt, AsyncWriteExt}; + + let mut reader = reader; + let mut line = String::new(); + + loop { + line.clear(); + // Zero bytes is end of input: the client closed the pipe, which is how an + // ACP session ends normally. It is not an error and must not be answered. + if reader.read_line(&mut line).await? == 0 { + return Ok(()); + } + + let answers = match decode(&line) { + Ok(incoming) => handler.handle(incoming).await, + Err(refusal) => vec![refusal], + }; + + for answer in &answers { + writer.write_all(encode(answer).as_bytes()).await?; + writer.write_all(b"\n").await?; + } + // Flushed once per frame batch rather than once per frame: the batch is + // what a client is waiting on, and a `session/prompt` answer arrives + // behind its own notifications either way. + writer.flush().await?; + } +} + +/// 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/lib.rs b/src/lib.rs index d8c6061..d5003cc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -57,6 +57,7 @@ //! //! [io-harness]: https://docs.rs/io-harness +pub mod acp; pub mod adapt; pub mod app; pub mod approval; diff --git a/tests/acp.rs b/tests/acp.rs new file mode 100644 index 0000000..9ac227f --- /dev/null +++ b/tests/acp.rs @@ -0,0 +1,441 @@ +//! 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. + +use io_cli::acp::{self, AgentCapabilities, ErrorCode, Incoming, Outgoing, GATED, SERVED}; +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", + ), + ( + r#"{"jsonrpc":"2.0","id":1}"#, + ErrorCode::InvalidRequest, + "a request naming no method", + ), + ]; + + 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"); +} + +/// 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 mut out: Vec = Vec::new(); + acp::serve(input.as_bytes(), &mut out, &mut handler) + .await + .expect("the loop ends on end-of-input, not on an error"); + + // 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", + ); + + let written = String::from_utf8(out).expect("frames are UTF-8"); + let lines: Vec<&str> = written.lines().collect(); + assert_eq!( + lines.len(), + 4, + "expected the initialize result, the parse error, and the prompt's \ + notification and result; got:\n{written}", + ); + + let parsed: Vec = lines + .iter() + .map(|line| serde_json::from_str(line).expect("every written line is one JSON document")) + .collect(); + + 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: {}", + lines[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 mut out: Vec = Vec::new(); + acp::serve(input.as_bytes(), &mut out, &mut handler) + .await + .expect("end-of-input is not an error"); + + assert_eq!(handler.seen, vec!["initialize"]); + let written = String::from_utf8(out).expect("frames are UTF-8"); + assert!( + written.ends_with('\n'), + "every emitted frame is terminated, whatever the client sent: {written:?}", + ); +} + +/// **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/dependencies.rs b/tests/dependencies.rs index 8b2cee1..b342430 100644 --- a/tests/dependencies.rs +++ b/tests/dependencies.rs @@ -1128,15 +1128,28 @@ 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 +1272,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 +1295,126 @@ 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, + ); +} + #[test] fn n1_the_binary_is_named_io_and_there_is_exactly_one() { let manifest = manifest(); From 77e9dc8a1d020359ba1f9d8feabb5d2dbd4fffd1 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 21:37:15 +0530 Subject: [PATCH 05/11] =?UTF-8?q?chore(develop):=20Re-point=20the=20twenty?= =?UTF-8?q?-three=20pinned=20io-harness=20source=20cita=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task: US-IO-CLI-0.36.0-T04 --- src/adapt.rs | 12 ++++++------ src/commands.rs | 6 +++--- src/configure.rs | 24 ++++++++++++------------ src/contract.rs | 4 ++-- src/events.rs | 2 +- src/exec.rs | 4 ++-- src/home.rs | 2 +- src/import.rs | 2 +- src/manage.rs | 2 +- src/memory.rs | 14 +++++++------- src/recall.rs | 4 ++-- src/reload.rs | 2 +- tests/adapt.rs | 2 +- tests/contract.rs | 2 +- tests/docs.rs | 33 +++++++++++++++++++++++++++++++-- tests/hooks.rs | 2 +- tests/instructions.rs | 2 +- tests/reload.rs | 2 +- 18 files changed, 75 insertions(+), 46 deletions(-) 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/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/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/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/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/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/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/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 { From 70b5153d841bea0097d6cb02a97b9e2f552c1fec Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 21:37:15 +0530 Subject: [PATCH 06/11] =?UTF-8?q?chore(develop):=20Translate=20io-harness?= =?UTF-8?q?=20RunEvents=20into=20ACP=20session/update=20=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task: US-IO-CLI-0.36.0-T06 --- src/acp_map.rs | 555 +++++++++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + tests/acp.rs | 349 +++++++++++++++++++++++++++++++ 3 files changed, 905 insertions(+) create mode 100644 src/acp_map.rs diff --git a/src/acp_map.rs b/src/acp_map.rs new file mode 100644 index 0000000..5988655 --- /dev/null +++ b/src/acp_map.rs @@ -0,0 +1,555 @@ +//! 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}; +use serde_json::{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, + "this becomes a `session/request_permission` **request** rather than a \ + notification, which is a different frame and is sent by the handler", + ), + ( + "approval_decided", + Update::ToolCallUpdate, + "the answer resolves the call the approval was about", + ), + ( + "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"), + }), + EventKind::ApprovalDecided { .. } => json!({ + "sessionUpdate": "tool_call_update", + "toolCallId": call_id(event), + "status": "in_progress", + }), + 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/lib.rs b/src/lib.rs index d5003cc..2bf9fa0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -58,6 +58,7 @@ //! [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/tests/acp.rs b/tests/acp.rs index 9ac227f..e3b1707 100644 --- a/tests/acp.rs +++ b/tests/acp.rs @@ -5,7 +5,11 @@ //! 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_harness::{EventKind, RunEvent}; use serde_json::{json, Value}; /// The frame a well-formed request arrives as. @@ -279,6 +283,351 @@ fn a_request_carrying_no_params_is_well_formed() { 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 handler that records what it was asked and answers from a script. struct Recorder { seen: Vec, From 96ca3c47dfd831d5a52c666accfa0d0829c2a25e Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 22:05:53 +0530 Subject: [PATCH 07/11] =?UTF-8?q?chore(develop):=20Wire=20the=20permission?= =?UTF-8?q?=20round=20trip,=20and=20prove=20io=20exec=20still=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task: US-IO-CLI-0.36.0-T07 --- src/acp.rs | 547 +++++++++++++++++++++++++++++++++++++++++++++++--- tests/acp.rs | 232 ++++++++++++++++++++- tests/exec.rs | 46 +++++ 3 files changed, 800 insertions(+), 25 deletions(-) diff --git a/src/acp.rs b/src/acp.rs index 4342136..2d28799 100644 --- a/src/acp.rs +++ b/src/acp.rs @@ -54,6 +54,11 @@ 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. @@ -152,20 +157,38 @@ pub enum Incoming { method: String, params: Value, }, + /// An answer to something **this** adapter asked the client. + /// + /// A frame with an id and no method. `session/request_permission` is the only + /// request this adapter makes in 0.36.0, and its answer arrives here; the + /// correlator matches it back to the waiting run by id. + /// + /// `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, whichever shape this is. + /// 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, whichever shape this is. + /// 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), } } } @@ -203,6 +226,10 @@ struct Envelope { method: Option, #[serde(default)] params: Option, + #[serde(default)] + result: Option, + #[serde(default)] + error: Option, } /// Read one frame. @@ -240,14 +267,28 @@ pub fn decode(line: &str) -> Result { let Some(method) = envelope.method else { // A frame with an id and no method is a *response* to something this - // adapter asked. Those are correlated elsewhere; reaching here means one - // arrived that nobody was waiting for, which is a client error rather - // than a crash. - return Err(Outgoing::Error { - id: envelope.id.unwrap_or(Value::Null), - code: ErrorCode::InvalidRequest, - message: "a request or notification names a method".into(), - }); + // adapter asked — `session/request_permission` is the only such request + // in 0.36.0. The correlator matches it back to the run that is waiting. + 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 @@ -401,7 +442,34 @@ pub trait Handler { /// /// 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, mut writer: W, handler: &mut H) -> std::io::Result<()> +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, + 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 +/// `session/request_permission` **request** an approval raises. 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, W: tokio::io::AsyncWrite + Unpin, @@ -413,29 +481,462 @@ where let mut line = String::new(); loop { - line.clear(); - // Zero bytes is end of input: the client closed the pipe, which is how an - // ACP session ends normally. It is not an error and must not be answered. - if reader.read_line(&mut line).await? == 0 { - return Ok(()); - } + let answers = tokio::select! { + // Biased so a frame already queued by a running turn is written before + // the next read is started. Without it a busy client can starve the + // run's own output for as long as it keeps sending, and the visible + // symptom is an editor that shows nothing while the agent works. + biased; + + frame = outbound.recv() => match frame { + Some(frame) => vec![frame], + // The sender is gone, which means the run is over. That is not a + // reason to stop serving: the client may still send another + // prompt. Disable this arm by waiting forever on a channel that + // will never speak again. + None => std::future::pending().await, + }, - let answers = match decode(&line) { - Ok(incoming) => handler.handle(incoming).await, - Err(refusal) => vec![refusal], + read = reader.read_line(&mut line) => { + // 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. + if read? == 0 { + return Ok(()); + } + let answers = match decode(&line) { + Ok(incoming) => handler.handle(incoming).await, + Err(refusal) => vec![refusal], + }; + line.clear(); + answers + } }; for answer in &answers { writer.write_all(encode(answer).as_bytes()).await?; writer.write_all(b"\n").await?; } - // Flushed once per frame batch rather than once per frame: the batch is - // what a client is waiting on, and a `session/prompt` answer arrives - // behind its own notifications either way. + // Flushed once per batch rather than once per frame: the batch is what a + // client is waiting on. A buffered answer is a hang, because the client is + // blocked reading the line it asked for. writer.flush().await?; } } +/// 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(), + }, + &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. +/// +/// **This is where 0.36.0 stops short, and the record says so rather than the +/// code implying otherwise.** The frame goes out, the options are the three +/// io-harness can carry out, and `permission_answer` is built and tested — but +/// routing the client's reply back into this future is not wired, so the decision +/// taken here is a denial. +/// +/// Denying is the safe direction and it is what `io exec` already does: no +/// permission is granted that the operator did not grant. The client is told, in +/// the tool call's own update, that the action needed an approval this agent +/// could not route — a refusal an operator can see beats one they cannot. +struct Consulting { + session_id: String, + outbound: tokio::sync::mpsc::UnboundedSender, +} + +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(); + Box::pin(async move { + let _ = outbound.send(Outgoing::Notification { + method: "session/update".into(), + params: json!({ + "sessionId": session_id, + "update": { + "sessionUpdate": "tool_call_update", + "toolCallId": target.clone(), + "status": "failed", + "title": format!( + "{act} {target} needs an approval this agent cannot yet route" + ), + } + }), + }); + io_harness::Decision::deny(crate::approval::REFUSED_BY_OPERATOR) + }) + } +} + /// The `initialize` result. /// /// `authMethods` is empty and that is legal. `io setup` is a wizard that writes diff --git a/tests/acp.rs b/tests/acp.rs index e3b1707..6cbd678 100644 --- a/tests/acp.rs +++ b/tests/acp.rs @@ -9,6 +9,7 @@ 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}; @@ -196,9 +197,12 @@ fn a_frame_that_cannot_be_understood_is_answered_rather_than_fatal() { "no jsonrpc member", ), ( - r#"{"jsonrpc":"2.0","id":1}"#, + // 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 request naming no method", + "a frame that is none of the three shapes", ), ]; @@ -628,6 +632,230 @@ fn a_tool_name_maps_to_one_of_the_nine_acp_tool_kinds() { ); } +/// **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", + ); +} + /// A handler that records what it was asked and answers from a script. struct Recorder { seen: Vec, 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.", + ); + } +} From eae55102817f89a900a2f7725e422d0a88f7d8a4 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 22:05:54 +0530 Subject: [PATCH 08/11] =?UTF-8?q?chore(develop):=20Open=20the=20argv=20doo?= =?UTF-8?q?r=20=E2=80=94=20io=20acp=20as=20the=20eighth=20subcommand,=20o?= =?UTF-8?q?=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task: US-IO-CLI-0.36.0-T08 --- src/cli.rs | 12 +++++++ src/main.rs | 34 ++++++++++++++++--- tests/dependencies.rs | 78 +++++++++++++++++++++++++++++++++++++++++++ tests/manage.rs | 48 ++++++++++++++++++++++++++ 4 files changed, 168 insertions(+), 4 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index a225ff7..5d78acb 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. + /// + /// It is the first non-interactive door of this product that can *answer* an + /// approval rather than refuse one, because an ACP client is a person in an + /// editor. `io exec` is unchanged and still refuses every approval — 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/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/tests/dependencies.rs b/tests/dependencies.rs index b342430..b0aed0e 100644 --- a/tests/dependencies.rs +++ b/tests/dependencies.rs @@ -1415,6 +1415,76 @@ fn n3_one_future_is_driven_and_it_is_the_binarys_entry_point() { ); } +/// **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(); @@ -2067,6 +2137,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/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 From 6d1c7603f04a4ebafd78600decd3322c97056514 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 22:15:11 +0530 Subject: [PATCH 09/11] =?UTF-8?q?chore(develop):=20Documentation=20?= =?UTF-8?q?=E2=80=94=20the=20new=20door,=20the=20changed=20approval=20limi?= =?UTF-8?q?=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task: US-IO-CLI-0.36.0-T09 --- CHANGELOG.md | 77 ++++++++++++++++++++++++++++++++++- README.md | 1 + docs/CAPABILITIES.md | 1 + docs/CONTRACT.md | 21 ++++++++++ docs/guide/editors.md | 93 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 192 insertions(+), 1 deletion(-) create mode 100644 docs/guide/editors.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 7efd9b7..e72aa22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,80 @@ 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 reaches a person for the first time on a non-terminal door.** `docs/CONTRACT.md` has +said since 0.5.0 that io "declines every approval rather than deferring one, in a session and +headless alike". An ACP client is a person in an editor, so an approval now becomes a +`session/request_permission` frame with three options: allow once, allow for this session, deny. +Three and not ACP's four — `io_harness::Decision::Deny` carries a reason and no rules, so a +remembered refusal cannot be recorded and `reject_always` would be a promise this agent cannot +keep. + +### 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 + +- **The client's permission answer does not reach the run.** The request is sent and the run + denies, and the tool call's update says the action needed an approval this agent could not + route. Nothing is granted that was not granted, but an editor session behaves as though every + grey-tier action were denied. +- **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 +3320,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/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..1bf76dc 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,19 @@ 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. +**0.36.0 narrows that sentence rather than retiring it.** `io acp` is the first door with a +person behind it that is not a terminal, so it is the first that could raise an approval and be +answered. It raises one: an approval becomes a `session/request_permission` frame naming the act +and its target, with three options — allow once, allow for this session, deny. Three and not +ACP's four, because `io_harness::Decision::Deny` carries a reason and no rules, so a remembered +refusal cannot be recorded and offering `reject_always` would be a promise this agent cannot +keep. + +**What 0.36.0 does not yet do is route the client's answer back into the waiting run**, so the +decision taken is a denial and the client is told so in the tool call's own update. No permission +is granted that was not granted. `io exec` is unchanged and still declines every approval: 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..ee7a8cb --- /dev/null +++ b/docs/guide/editors.md @@ -0,0 +1,93 @@ +# 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). + +## Permissions + +When the policy puts an action in the grey tier, io raises `session/request_permission` naming the +act and its target, with three options: allow once, allow for this session, deny. + +Three rather than 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. + +**In 0.36.0 the answer you choose does not yet reach the run.** The request is sent and the run +denies, and the tool call's update says the action needed an approval this agent could not route. +Nothing is granted that you did not grant, and nothing happens silently — but an editor session +behaves, for now, as though every grey-tier action were denied. Configure the posture you want +instead: see [Configuration](configuration.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. From d6856c80a727f6ddb5e0d1c4c7f95d0895680744 Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 23:50:47 +0530 Subject: [PATCH 10/11] fix(acp): stream while the turn runs, and stop claiming a request that is not sent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adversarial review found four High behind a fully green suite. Three of the ACP ones shared a root cause. The serve loop awaited handler.handle() inside a tokio::select! arm. For a session/prompt that call IS the whole turn, so the outbound channel was not polled again until the turn returned: every session/update queued and the client received the entire conversation AFTER being told the turn had ended. The guide promises "the answer, streamed as the provider returns it". `biased` could not help, because the loop was not at the select point to be biased about. The writer now runs on its own task and owns the writer outright, so a notification sent from inside a running turn is written while the turn is still running. No Mutex: a lock held across an await on a pipe a slow client is not draining is a deadlock with no error message. Verified against a real provider — message chunks and a thought chunk arrive before the result. The suite could not see it. Nothing 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. Both loop tests now run over a duplex pipe, and a new test asserts the wire ORDER across a handler that sends while it works. Two more from the same review: - ApprovalDecided reported in_progress for every decision, discarding the approve/deny/defer the harness carries. Since the ACP approver always denies, every grey-tier action left a cell spinning for ever on a call that would never complete. - The refusal notification put the target PATH in toolCallId, so it named a cell that had never been announced and a conforming client had nothing to apply it to. It is built from the run and step now, as every tool_call frame is. And the false claims. Five documents said io raises session/request_permission. It does not: it sends a session/update refusal. Raising the request means awaiting the answer, and routing that back into the run is not wired — a prompt whose outcome is ignored is worse than no prompt. CONTRACT.md, the guide, the CHANGELOG, --help and three comments now say what the code does. The model is told the interface could not route the approval, not that the operator denied it, because the operator was never asked. Separately, on the pin side: a mixed cache-write fold drew "of which fresh" as an exact number when it is a ceiling. add_reported folds one reporting call and one silent one to Some(n), and n is a floor; subtracting a floor from the prompt gives an upper bound, too large by exactly the amount nobody measured. That is verbatim the failure the adjacent comment says must not happen — the guard only distinguished "no call reported" from "some call reported". Total gains unreported_writes, the written row reads "1.0k or more", and fresh reads unknown. Reachable in practice: openai_wire reports no cache-write count and Anthropic reports one only where a cache was created. 104 binaries / 1,896 passed / 0 failed / 37 ignored. fmt, clippy -D warnings and doctests clean. --- CHANGELOG.md | 24 ++--- docs/CONTRACT.md | 26 +++--- docs/guide/editors.md | 32 +++++-- src/acp.rs | 212 +++++++++++++++++++++++++++++------------- src/acp_map.rs | 41 ++++++-- src/cli.rs | 8 +- src/cost.rs | 43 +++++++-- tests/acp.rs | 164 ++++++++++++++++++++++++++------ tests/cost.rs | 101 ++++++++++++++++---- tests/dependencies.rs | 14 ++- 10 files changed, 504 insertions(+), 161 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e72aa22..aba8f14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,13 +28,13 @@ answer streams back as `session/update` notifications — message chunks, reason 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 reaches a person for the first time on a non-terminal door.** `docs/CONTRACT.md` has -said since 0.5.0 that io "declines every approval rather than deferring one, in a session and -headless alike". An ACP client is a person in an editor, so an approval now becomes a -`session/request_permission` frame with three options: allow once, allow for this session, deny. -Three and not ACP's four — `io_harness::Decision::Deny` carries a reason and no rules, so a -remembered refusal cannot be recorded and `reject_always` would be a promise this agent cannot -keep. +**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 @@ -66,10 +66,12 @@ changed; the ordering did. ### Known limitations -- **The client's permission answer does not reach the run.** The request is sent and the run - denies, and the tool call's update says the action needed an approval this agent could not - route. Nothing is granted that was not granted, but an editor session behaves as though every - grey-tier action were denied. +- **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. diff --git a/docs/CONTRACT.md b/docs/CONTRACT.md index 1bf76dc..1215ae4 100644 --- a/docs/CONTRACT.md +++ b/docs/CONTRACT.md @@ -72,18 +72,20 @@ 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. -**0.36.0 narrows that sentence rather than retiring it.** `io acp` is the first door with a -person behind it that is not a terminal, so it is the first that could raise an approval and be -answered. It raises one: an approval becomes a `session/request_permission` frame naming the act -and its target, with three options — allow once, allow for this session, deny. Three and not -ACP's four, because `io_harness::Decision::Deny` carries a reason and no rules, so a remembered -refusal cannot be recorded and offering `reject_always` would be a promise this agent cannot -keep. - -**What 0.36.0 does not yet do is route the client's answer back into the waiting run**, so the -decision taken is a denial and the client is told so in the tool call's own update. No permission -is granted that was not granted. `io exec` is unchanged and still declines every approval: an -unattended run has nobody to ask, and an approver that blocked there would hang forever. +**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` diff --git a/docs/guide/editors.md b/docs/guide/editors.md index ee7a8cb..e057f5d 100644 --- a/docs/guide/editors.md +++ b/docs/guide/editors.md @@ -49,21 +49,35 @@ is `cancelled`, one the policy refused is `refusal`, and a budget or step ceilin 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 -When the policy puts an action in the grey tier, io raises `session/request_permission` naming the -act and its target, with three options: allow once, allow for this session, deny. +**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. -Three rather than 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 +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. -**In 0.36.0 the answer you choose does not yet reach the run.** The request is sent and the run -denies, and the tool call's update says the action needed an approval this agent could not route. -Nothing is granted that you did not grant, and nothing happens silently — but an editor session -behaves, for now, as though every grey-tier action were denied. Configure the posture you want -instead: see [Configuration](configuration.md). +## 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 diff --git a/src/acp.rs b/src/acp.rs index 2d28799..845c69a 100644 --- a/src/acp.rs +++ b/src/acp.rs @@ -159,9 +159,12 @@ pub enum Incoming { }, /// An answer to something **this** adapter asked the client. /// - /// A frame with an id and no method. `session/request_permission` is the only - /// request this adapter makes in 0.36.0, and its answer arrives here; the - /// correlator matches it back to the waiting run by id. + /// 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 @@ -196,9 +199,19 @@ impl Incoming { /// 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 }, + Result { + id: Value, + result: Value, + }, + Error { + id: Value, + code: ErrorCode, + message: String, + }, + Notification { + method: String, + params: Value, + }, } impl Outgoing { @@ -267,8 +280,9 @@ pub fn decode(line: &str) -> Result { let Some(method) = envelope.method else { // A frame with an id and no method is a *response* to something this - // adapter asked — `session/request_permission` is the only such request - // in 0.36.0. The correlator matches it back to the run that is waiting. + // 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, @@ -445,7 +459,7 @@ pub trait Handler { 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, + W: tokio::io::AsyncWrite + Unpin + Send + 'static, H: Handler, { let (_unused, rx) = tokio::sync::mpsc::unbounded_channel(); @@ -456,7 +470,7 @@ where /// /// 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 -/// `session/request_permission` **request** an approval raises. Both originate +/// 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. /// @@ -472,55 +486,89 @@ pub async fn serve_with( ) -> std::io::Result<()> where R: tokio::io::AsyncBufRead + Unpin, - W: tokio::io::AsyncWrite + 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), + } - loop { - let answers = tokio::select! { - // Biased so a frame already queued by a running turn is written before - // the next read is started. Without it a busy client can starve the - // run's own output for as long as it keeps sending, and the visible - // symptom is an editor that shows nothing while the agent works. - biased; - - frame = outbound.recv() => match frame { - Some(frame) => vec![frame], - // The sender is gone, which means the run is over. That is not a - // reason to stop serving: the client may still send another - // prompt. Disable this arm by waiting forever on a channel that - // will never speak again. - None => std::future::pending().await, - }, - - read = reader.read_line(&mut line) => { - // 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. - if read? == 0 { - return Ok(()); - } - let answers = match decode(&line) { - Ok(incoming) => handler.handle(incoming).await, - Err(refusal) => vec![refusal], - }; - line.clear(); - answers - } + let answers = match decode(&line) { + Ok(incoming) => handler.handle(incoming).await, + Err(refusal) => vec![refusal], }; - for answer in &answers { - writer.write_all(encode(answer).as_bytes()).await?; - writer.write_all(b"\n").await?; + 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; + } } - // Flushed once per batch rather than once per frame: the batch is what a - // client is waiting on. A buffered answer is a hang, because the client is - // blocked reading the line it asked for. - writer.flush().await?; - } + }; + + // 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 @@ -650,10 +698,7 @@ impl io_harness::Observer for Reporter { // 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) - { + if self.cancelled.load(std::sync::atomic::Ordering::Relaxed) { return io_harness::Flow::Cancel; } @@ -789,7 +834,11 @@ impl Handler for Editing<'_, P> { } impl Editing<'_, P> { - async fn request(&mut self, method: &str, params: &Value) -> Result { + async fn request( + &mut self, + method: &str, + params: &Value, + ) -> Result { match method { "initialize" => Ok(initialize_result(env!("CARGO_PKG_VERSION"))), "session/new" => { @@ -853,6 +902,8 @@ impl Editing<'_, P> { &Consulting { session_id: session_id.to_string(), outbound: self.outbound.clone(), + run_id: self.session.id(), + step: std::sync::atomic::AtomicU32::new(0), }, &reporter, ) @@ -893,19 +944,33 @@ pub fn prompt_text(params: &Value) -> Option { /// The approver for an ACP turn. /// -/// **This is where 0.36.0 stops short, and the record says so rather than the -/// code implying otherwise.** The frame goes out, the options are the three -/// io-harness can carry out, and `permission_answer` is built and tested — but -/// routing the client's reply back into this future is not wired, so the decision -/// taken here is a denial. +/// **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. The client is told, in -/// the tool call's own update, that the action needed an approval this agent -/// could not route — a refusal an operator can see beats one they cannot. +/// 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 { @@ -917,6 +982,17 @@ impl io_harness::Approver for Consulting { 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(), @@ -924,7 +1000,7 @@ impl io_harness::Approver for Consulting { "sessionId": session_id, "update": { "sessionUpdate": "tool_call_update", - "toolCallId": target.clone(), + "toolCallId": id, "status": "failed", "title": format!( "{act} {target} needs an approval this agent cannot yet route" @@ -932,11 +1008,19 @@ impl io_harness::Approver for Consulting { } }), }); - io_harness::Decision::deny(crate::approval::REFUSED_BY_OPERATOR) + 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 diff --git a/src/acp_map.rs b/src/acp_map.rs index 5988655..6925529 100644 --- a/src/acp_map.rs +++ b/src/acp_map.rs @@ -39,7 +39,13 @@ //! and the durable trace instead. use io_harness::{EventKind, RunEvent}; -use serde_json::{json, Value}; +// 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)] @@ -106,13 +112,17 @@ pub const MAPPING: &[(&str, Update, &str)] = &[ ( "approval_requested", Update::None, - "this becomes a `session/request_permission` **request** rather than a \ - notification, which is a different frame and is sent by the handler", + "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 resolves the call the approval was about", + "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", @@ -227,7 +237,11 @@ pub const MAPPING: &[(&str, Update, &str)] = &[ ), ("handle_killed", Update::ToolCallUpdate, "the handle ended"), ("handle_exited", Update::ToolCallUpdate, "the handle ended"), - ("handle_orphaned", Update::ToolCallUpdate, "the handle ended"), + ( + "handle_orphaned", + Update::ToolCallUpdate, + "the handle ended", + ), ( "reviewed", Update::None, @@ -499,10 +513,23 @@ pub fn translate(event: &RunEvent) -> Option { "status": "failed", "title": format!("{act} {target} refused"), }), - EventKind::ApprovalDecided { .. } => json!({ + // **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": "in_progress", + "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", diff --git a/src/cli.rs b/src/cli.rs index 5d78acb..f7f8a96 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -94,10 +94,10 @@ pub enum Command { /// the protocol, so nothing else may be written there; diagnostics go to /// stderr. /// - /// It is the first non-interactive door of this product that can *answer* an - /// approval rather than refuse one, because an ACP client is a person in an - /// editor. `io exec` is unchanged and still refuses every approval — an - /// unattended run has nobody to ask. + /// 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. /// diff --git a/src/cost.rs b/src/cost.rs index b91751e..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 @@ -322,9 +342,14 @@ fn section(total: &Total) -> Vec { )); rows.push(Row::fact( "of which cache written", - match usage.cache_write_tokens { - Some(count) => tokens(count), - None => UNREPORTED.to_string(), + // 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 @@ -333,14 +358,20 @@ fn section(total: &Total) -> Vec { // that was never measured, and it would look like a fact. rows.push(Row::fact( "of which fresh", - match usage.cache_write_tokens { - Some(written) => 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), ), - None => UNREPORTED.to_string(), + _ => UNREPORTED.to_string(), }, )); } diff --git a/tests/acp.rs b/tests/acp.rs index 6cbd678..071bbf9 100644 --- a/tests/acp.rs +++ b/tests/acp.rs @@ -258,8 +258,9 @@ fn an_id_is_what_separates_a_request_from_a_notification() { 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"); + 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:?}"), @@ -480,7 +481,10 @@ fn f3_every_outcome_maps_to_one_of_the_five_acp_stop_reasons() { question_id: 1, steps: 1, }, - io_harness::RunOutcome::AwaitingPlan { plan_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 { @@ -658,9 +662,10 @@ fn a_response_is_correlated_not_refused() { 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"); + 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"), @@ -803,7 +808,10 @@ fn f5_a_cancel_reaches_the_observer_and_silences_it_at_once() { 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["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:?}"), @@ -856,6 +864,109 @@ fn a_no_op_kind_produces_no_frame_at_all() { ); } +/// 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, @@ -918,10 +1029,8 @@ async fn the_serve_loop_answers_a_bad_frame_and_keeps_reading() { ], }; - let mut out: Vec = Vec::new(); - acp::serve(input.as_bytes(), &mut out, &mut handler) - .await - .expect("the loop ends on end-of-input, not on an error"); + 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. @@ -931,20 +1040,13 @@ async fn the_serve_loop_answers_a_bad_frame_and_keeps_reading() { "the loop stopped reading at the malformed frame, or passed it to the handler", ); - let written = String::from_utf8(out).expect("frames are UTF-8"); - let lines: Vec<&str> = written.lines().collect(); assert_eq!( - lines.len(), + parsed.len(), 4, "expected the initialize result, the parse error, and the prompt's \ - notification and result; got:\n{written}", + notification and result; got: {parsed:?}", ); - let parsed: Vec = lines - .iter() - .map(|line| serde_json::from_str(line).expect("every written line is one JSON document")) - .collect(); - assert_eq!(parsed[0]["id"], json!(1)); assert_eq!(parsed[0]["result"]["ok"], json!(true)); @@ -957,7 +1059,7 @@ async fn the_serve_loop_answers_a_bad_frame_and_keeps_reading() { assert!( parsed[2].get("id").is_none(), "a notification carries no id: {}", - lines[2], + parsed[2], ); assert_eq!(parsed[3]["id"], json!(2)); assert_eq!(parsed[3]["result"]["stopReason"], json!("end_turn")); @@ -980,17 +1082,16 @@ async fn a_final_frame_with_no_trailing_newline_is_still_read() { }]], }; - let mut out: Vec = Vec::new(); - acp::serve(input.as_bytes(), &mut out, &mut handler) - .await - .expect("end-of-input is not an error"); + let (_updates, outbound) = tokio::sync::mpsc::unbounded_channel(); + let frames = drive(&input, &mut handler, outbound).await; assert_eq!(handler.seen, vec!["initialize"]); - let written = String::from_utf8(out).expect("frames are UTF-8"); - assert!( - written.ends_with('\n'), - "every emitted frame is terminated, whatever the client sent: {written:?}", + 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.** @@ -1007,7 +1108,10 @@ fn the_served_methods_are_the_ones_this_release_implements() { !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("fs/read_text_file"), + "a client method, not ours" + ); assert!(!acp::serves("nonsense/method")); assert_eq!( diff --git a/tests/cost.rs b/tests/cost.rs index a69c73c..be0eea5 100644 --- a/tests/cost.rs +++ b/tests/cost.rs @@ -616,37 +616,106 @@ fn f10_folding_an_unreported_count_with_a_reported_one_keeps_the_number() { ..Default::default() }; let spelled = io_cli::status::format_tokens; - let drawn = |calls: &[_]| { - field( - &page(&seeded(calls), &table(), &provenance()), - "of which cache written", - ) - .to_string() + 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(&[ - call(Some(PRICED), Some(with(None))), - call(Some(PRICED), Some(with(None))), - ]), + drawn(&none_reported), "unknown", "no call reported a cache-write count, so the fold has nothing to report", ); assert_eq!( - drawn(&[ + 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)))), ]), - spelled(1_000), - "one call reported 1,000 cache writes and the fold dropped it", + "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!( - drawn(&[ + fresh(&[ call(Some(PRICED), Some(with(Some(250)))), call(Some(PRICED), Some(with(Some(750)))), ]), - spelled(1_000), - "two reported counts must sum", + spelled(2_000 - 200 - 1_000), + "every call reported, so nothing is in doubt and fresh is exact", ); } diff --git a/tests/dependencies.rs b/tests/dependencies.rs index b0aed0e..f5727ae 100644 --- a/tests/dependencies.rs +++ b/tests/dependencies.rs @@ -1149,7 +1149,11 @@ fn f7_the_configuration_is_read_through_the_harness_and_never_parsed_here() { /// 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("acp.rs"), 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 } @@ -1341,7 +1345,13 @@ fn n3_no_second_async_executor_is_in_the_tree() { // 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"] { + 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 \ From ee15cc7d85ce2802385c5255a1c26e7c6b300c9d Mon Sep 17 00:00:00 2001 From: aakashpawar1999 Date: Thu, 3 Sep 2026 23:51:13 +0530 Subject: [PATCH 11/11] build(deps): give tokio io-util and io-std for the ACP adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adapter reads newline-delimited JSON-RPC off stdin and writes frames to stdout, which is AsyncBufReadExt::read_line over tokio::io::stdin(). Features of a name already in the tree rather than a name of its own, so the direct dependency set stays at ten and tests/dependencies.rs is untouched. The argument for hand-writing the wire instead of taking agent-client-protocol lives beside the feature list: that crate's graph brings a second async executor family — async-io, async-process, async-task, blocking, polling — with its stdio transport putting blocking::Unblock over the real stdin and stdout. src/stdin.rs exists because of what a second owner of stdin cost in 0.13.1. --- Cargo.toml | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 4dac0fe..c2a2635 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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