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